amadan
The replicator's eighth build: the seventh review's bounded findings
An owned file that has vanished is doubt from the last clean look, not a clean look. A relative link target is walked from the link's directory as spelled, never cleaned first, so a traversal through a tenant's link and back is seen. When a chain latches, each fleet between its root and its last fleet is given its predecessor's window on its own recorders, once; a latched fleet lets its chain go, and the walks stop at the first latched fleet. Header reads are refused off unix rather than done by an open that follows links. What stays open, litestream's own opens and the walk by directory descriptor above all, is in the dispositions, review seven.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8 files changed,
+619
−39
docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review7.md+204 −0docs/superpowers/specs/DISPOSITIONS-metered-replicator.md+18 −0docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review7.md+191 −0internal/ops/litestream_metered.go+45 −13internal/ops/litestream_metered_test.go+73 −1internal/ops/litestream_owners.go+28 −22internal/ops/litestream_owners_unix_test.go+51 −0internal/ops/sqliteopen_other.go+9 −3
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review7.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review7.md| new file mode 100644 |
| index 000000000..2ce5ab7fd |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review7.md |
| @@ -0,0 +1,204 @@ |
| +All locations below are at `6b648c0c`. Test counterfactuals are source-based; no tests or mutations were run. |
| + |
| +| Review-six finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P2-1:** relative systemd state link refused | **Closed for the normal systemd shape.** Relative targets are accepted. The added normalization introduces finding 5 below. | **No.** The relative fixture at `internal/ops/litestream_owners_unix_test.go:99` distinguishes the old refusal. | |
| +| **Opus P2-2:** registry-first invalidation loses the filesystem boundary | **Closed for that sequence**, at `internal/ops/litestream_metered.go:289`. Missing files can still corrupt the saved boundary; finding 3. | **No.** `TestARegistryFindingHoldsFromTheLastCleanLookToo` distinguishes the old `goStale`. It calls the helper directly, rather than driving a registry change. | |
| +| **Opus P2-3:** missing `O_NOCTTY` | **Closed for controlling-terminal acquisition**, at `internal/ops/sqliteopen_unix.go:22`. Other unsafe-open effects remain. | **No regression test.** Existing tests would pass with this flag removed. | |
| +| **Opus P3-1:** two unopened intermediates lose the eventual replacement’s window | **Closed for the reported sequence.** Inheritance now waits for every predecessor. Intermediate recorders remain a separate gap; finding 4. | **No.** `TestAHoldReachesThroughTwoUnopenedFleets` distinguishes immediate-predecessor-only checking. | |
| +| **Opus P3-2:** link-above fixture does not reproduce the defect | **Fixture corrected**, at `internal/ops/litestream_owners_unix_test.go:181`. | **No** against removal of both ancestry protections. **Yes** if only the final `EvalSymlinks` check is removed: the resolver independently rejects this fixture. | |
| +| **Opus P3-3:** ineffective or absent regression tests | **Partly closed.** The new start-versus-end test distinguishes both storing the end and failing to advance `lastClean`. Open/Close interleaving, retained handles, worker exit, final-disappearance branches and the watch’s filesystem invocation remain unproved. | **No** for those two `lastClean` mutations. **Yes** for several remaining mutations; finding 7. | |
| +| **Opus P3-4:** temporary-root assumptions | **Partly closed.** Ordinary relative ancestor links no longer fail merely for being relative. Basename owner overrides, synthetic UID collisions and permission assumptions remain. | No dedicated environmental regression matrix. Tests can still fail with correct production code; finding 10. | |
| +| **Opus P3-5:** platform-UID tenant accepted outside its tenant directory | **Open and documented.** | No isolation test; a current fixture explicitly expects platform-user links to be accepted. | |
| +| **Opus P3-6:** independent or forked replacement chains overlap | **Closed as the requested documentation alternative**, at `internal/ops/litestream_metered.go:106`. There is still no enforcement. | No exclusivity test. Removing the comment changes no behavior. | |
| +| **Opus P3-7:** remaining silent drops | **Closed for sidecar-header databases and vanished claimed glob matches.** Hibernating exec routes are explicitly excluded. Other inventory limits remain. | **No** for restoring sidecar suppression. **Yes** for removing `globbed[p]` from final-disappearance reporting; that race remains untested. | |
| +| **Astra 1:** Litestream retains a substituted database | **Open.** Finding 1. | No regression test for constrained actual opens. | |
| +| **Astra 2:** registry-first invalidation and missing files lose doubtful dates | **Partly closed.** Registry-first invalidation consults `lastClean`; the missing-path boundary advance remains. | **No** for reverting the new registry-boundary helper. Current tests still permit—and explicitly expect—missing files to cause drift without a hold. | |
| +| **Astra 3:** two unopened intermediates lose a later predecessor hold | **Closed for delivery to the final replacement.** Finding 4 concerns the skipped intermediate recorders. | **No** for reverting the full-chain latch condition. The new test has no intermediate recorders and cannot detect finding 4. | |
| +| **Astra 4:** tenant-replaceable ancestry | **Open.** Finding 2. | Existing tests pass with this ancestry unchecked. | |
| +| **Astra 5:** normalization hides a traversed tenant link | **Partly closed.** Absolute targets are walked before cleaning; relative targets are still cleaned first. | **Yes for the reported normalization defect.** The cited fixture uses an absolute `hop/choice` target without a cancelling `..`; it does not distinguish the new component walk from the preceding canonical-target check. | |
| +| **Astra 6:** database header suppressed by sidecar name | **Closed.** Sidecar suppression was removed. | **No.** The database-header-bearing `live.db-journal` fixture now requires reporting. | |
| +| **Astra 7:** vanished claimed exec glob match disappears | **Closed in code**, at `internal/ops/litestream.go:500`. | **Yes.** No deterministic glob-to-final-`Stat` race test exists. | |
| +| **Astra 8:** unsafe header opens | **Partly closed.** `O_NOCTTY` is present; ancestor traversal, device-open effects and unchecked non-Unix opening remain. | Existing FIFO/link tests pass without `O_NOCTTY`; they also fail to isolate inode checking from no-follow checking. | |
| +| **Astra 9:** tests establish weaker properties than advertised | **Partly closed.** The new boundary and transitive-window tests help. Finding 7 remains. | **Yes** for the material mutations listed there. | |
| +| **Opus claims** | **Mixed.** Relative-link handling, the registry boundary and downstream inheritance improved. Request-refusal, clean-close, missing-file, resolved-path and completeness claims remain overstated. | Detailed below. | |
| +| **Astra claims** | **Mixed.** Four-trigger wording is corrected. Some older contradictory statements remain, and relative-target normalization is still unsafe. | Detailed below. | |
| + |
| +The requested `6b648c0c..6b648c0c` diff is empty. This review evaluates the complete `72b4f59a..6b648c0c` branch without substituting another incremental range. |
| + |
| +1. **P1 — The verified pathname still does not constrain the files Litestream actually opens.** |
| + |
| + **Locations:** `internal/lease/meteredstore.go:127`; `internal/ops/litestream_owners.go:268`; `third_party/litestream/db.go:1004`, `:1014`, `:1866`. |
| + |
| + Tenant A replaces its validated database path with a link to V’s database while Litestream initializes its SQLite connection and persistent descriptor. A restores the original pathname before the next inventory. The inventory sees the original inode, while Litestream retains V’s opened file. Its SQLite initialization can also modify that database. |
| + |
| + This remains larger than a polling interval. The exposure includes initialization retries and later WAL opens, not just the first successful open. |
| + |
| + **Smallest acceptable fix:** carry verified directory authority through the actual SQLite, database and sibling opens, using constrained resolution and checking the identities actually opened. A later descriptor comparison alone cannot prevent the unauthorized open or its effects. |
| + |
| +2. **P1 — Platform-owned endpoints do not establish protected ancestry.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:395`, `:460`, `:626`, `:652`; `internal/ops/litestream_metered.go:824`. |
| + |
| + Let tenant T own `/srv/T`, containing root-owned, protected `bridge`. A platform-made link points to `/srv/T/bridge/A`, whose directory and database belong to T. The immediate-parent and chain checks pass. T can nevertheless rename `bridge` through `/srv/T` and replace the pathname. |
| + |
| + The same construction works for a platform-owned `dataDir` beneath a tenant-writable ancestor. Private `.replicator` directories do not protect the pathname above them. |
| + |
| + A platform link whose target’s **immediate parent** is tenant-owned is refused. The accepted case needs the protected intermediate directory; that distinction matters. |
| + |
| + **Smallest acceptable fix:** verify and pin the complete trusted ancestry for database claims, authorized links and shadow state. An independently enforced immutable hierarchy could supply that precondition, but this constructor does not establish it. The unresolved scope is broader than `dataDir`. |
| + |
| +3. **P1 — A missing owned pathname still advances `lastClean`, erasing an earlier doubtful day.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:548`, `:560`, `:289`, `:315`. |
| + |
| + A concrete sequence: |
| + |
| + - The last clean look starts at 23:57 on October 2. |
| + - A substitution causes doubtful replication at 23:58. The substituted pathname is then removed. |
| + - At 00:02 on October 3, `lookAgain` finds that pathname absent. It continues at line 550 and records 00:02 as a clean boundary. |
| + - Registry proofs remain current. A registry change is detected at 00:03. |
| + - The new `goStale` correctly consults `lastClean`, but that boundary is now wrong. With the one-minute margin, it holds October 3 only. |
| + |
| + October 2 remains unheld on the old fleet and its replacements. An open descriptor or pending shadow data can survive pathname disappearance; absence does not prove that nothing was shipped. |
| + |
| + **Smallest acceptable fix:** treat disappearance of an already-owned file as uncertainty from the previous clean boundary, and do not advance that boundary through an unverified file. Add the disappearance-across-midnight sequence. The existing removal case at `internal/ops/litestream_metered_test.go:838` currently requires drift without a hold. |
| + |
| +4. **P2 — Passing the window through an unopened fleet does not hold that fleet’s own recorders.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:375`, `:418`, `:430`, `:655`; test at `internal/ops/litestream_metered_test.go:1493`. |
| + |
| + A sequence using distinct recorders: |
| + |
| + - A runs while the database belongs to account `a`. |
| + - Ownership changes to `b`. B is constructed with `Replaces: A` and a recorder for `b`. |
| + - B closes unopened while A remains open. B’s own witness is unchanged; `inheritHold` returns because A is not closed. |
| + - An attempt admitted by A before the change dispatches during `b`’s ownership. |
| + - Ownership changes to `c`. A detects staleness and closes. |
| + - C replaces B and opens. |
| + |
| + C receives A’s interval through `B.staleWindow()`. B’s recorder receives nothing: delegation returns dates but never invokes `Incomplete` on B’s recorders, and `closeOnce` prevents another shutdown from revisiting inheritance. |
| + |
| + Thus the interval survives to the final fleet, but a day remains unheld on the intermediate owner’s recorder. The API explicitly says it does not rely on every recorder sharing one ledger. |
| + |
| + **Smallest acceptable fix:** finalize pending inheritance through closed intermediates, applying each predecessor window to that intermediate’s recorders exactly once before forwarding it. Give B and C distinct recorders in the regression test; the current two-intermediate fixture gives neither any. |
| + |
| +5. **P2 — Relative platform-link targets still undergo unsafe lexical normalization.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:640`, `:645`, `:343`, `:359`; `internal/ops/litestream_owners_unix_test.go:155`. |
| + |
| + Consider a root-owned `/public/A` link with this **relative** target: |
| + |
| + ```text |
| + ../srv/T/choice/../../safe/A |
| + ``` |
| + |
| + T controls `/srv/T/choice` and points it to `/victims/V/x/y`. |
| + |
| + The filesystem reaches `/victims/V/safe/A`. But `filepath.Join(prefix, target)` first cleans the target to `/srv/safe/A`. `noLinkInPath` therefore never sees `choice`. |
| + |
| + If `/srv/safe/A` contains a qualifying platform-owned directory and database, `checkOwnedFile` can approve those substituted facts. The platform-owned starting-directory exception permits proceeding despite the original file observation having another UID. `OwnedDB.Path` still contains `/public/A/main.db`, which Litestream opens into V’s tree. |
| + |
| + This requires no concurrent swap. |
| + |
| + **Smallest acceptable fix:** preserve the relative target’s components when anchoring it to the link directory; walk them before any cleaning. Add both absolute and relative targets containing a tenant-controlled link followed by `..`, with different files at the lexical and physical destinations. |
| + |
| +6. **P2 — Inventory remains incomplete outside the newly repaired candidate paths.** |
| + |
| + **Locations:** `internal/ops/litestream.go:385`, `:395`, `:420`, `:482`, `:500`; `internal/ops/litestream_owners.go:519`, `:529`, `:538`. |
| + |
| + A non-hibernating exec route names `/srv/custom/archive.sqlite`. The route makes that path *claimed*, but nothing adds it to `matches`: it is outside both globs and is not a unit route. It appears in neither `Owned`, `Skipped` nor `Problems`. |
| + |
| + Other remaining exclusions include empty tree files, entries disappearing before their first `Lstat`, and non-database-named links to regular files. A relocated registry disappearing before final validation can also miss the provenance conditions at line 500. The absent-console exception is broader than an explicit distinction between “no console” and “configured console database missing.” |
| + |
| + There is still no external-inventory comparison to detect such omissions at cutover. |
| + |
| + **Smallest acceptable fix:** enumerate explicit non-hibernating claims independently of glob discovery, retain candidate provenance, and distinguish optional absent platform databases from expected ones. Require an explicit comparison with the external replicator’s inventory before cutover. Intentional placeholder/churn exclusions should remain explicitly qualified. |
| + |
| +7. **P2 — Material regression tests still pass without the behavior they claim to prove.** |
| + |
| + **Locations:** `internal/ops/litestream_metered_test.go:687`, `:889`, `:952`, `:973`, `:1334`, `:1393`, `:1493`; `internal/ops/litestream_owners_unix_test.go:275`. |
| + |
| + Source inspection supports these counterfactuals: |
| + |
| + - Removing `store.Close` still satisfies assertions based on a cleared pointer and a closed request gate. Neither retained descriptors nor worker termination is asserted. |
| + - Removing the watch’s filesystem invocation leaves tests that invoke `lookAgain` directly intact. |
| + - The real opened-store test checks positive request counts on both recorders, not a per-database correspondence. Its “counts to settle” assertion checks only server inactivity. |
| + - Removing Open’s lifecycle lock is not tested through a forced Open/Close interleaving. |
| + - Removing inheritance’s once-only protection is not checked through exact delivery counts; day maps collapse duplicates. |
| + - The retry table checks exact counts for recovered/exhausted errors, but not immediate stopping on witnessed changes. It does not exercise transient recovery through Close. |
| + - Removing only `O_NOFOLLOW` still leaves the different-inode link fixture rejected by the identity comparison. Removing only the identity comparison leaves the tested link/FIFO cases rejected by other checks. |
| + - The new normalization test cited in the dispositions contains no cancelling `..`. |
| + |
| + The blocked-Close test does improve coverage. Removing `closeOnce` alone would eventually call its hook twice and panic on `close(entered)`; that is still not an assertion that callers receive the same cached shutdown error. |
| + |
| + **Smallest acceptable fix:** add synchronized tests of these production paths and assert their actual effects, including intermediate recorders, retained handles, worker completion, request attribution and exact retry/inheritance counts. |
| + |
| +8. **P3 — Header inspection still opens potentially harmful objects before rejecting them.** |
| + |
| + **Locations:** `internal/ops/sqliteopen_unix.go:22`, `:34`; `internal/ops/sqliteopen_other.go:9`. |
| + |
| + After `Lstat` observes regular `T/d/watchdog`, T substitutes a parent link to `/dev`. The subsequent open can operate on `/dev/watchdog` before `fstat` rejects it. `O_NOCTTY` closes the terminal-acquisition case, but `O_NONBLOCK` does not prevent all device-open effects or filesystem stalls. |
| + |
| + On non-Unix builds, the helper still performs an unchecked `os.Open`. Later ownership refusal does not undo that earlier open. |
| + |
| + `sidecarOf` has been removed. A sidecar-shaped file is now header-read independently of its base, so a linked base no longer suppresses that inspection. |
| + |
| + **Smallest acceptable fix:** constrain traversal through pinned directories and prevent special-device opens before post-open validation. Unsupported runtime platforms should refuse inspection rather than use an unchecked fallback. |
| + |
| +9. **P3 — Platform-UID tenants and route-to-user binding remain unenforced preconditions.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:325`, `:359`, `:379`, `:436`, `:629`. |
| + |
| + A root-run tenant route naming `/var/lib/fail2ban/fail2ban.db` can qualify when its directory and parent are protected root-owned directories. Its own root-owned links can also qualify as platform-made links. |
| + |
| + This is not literally acceptance “from anywhere”: directory modes, parent ownership, regular-file shape, link count, filesystem and sibling checks still apply. On a root-running box, the climb stops at the immediate root-owned parent rather than climbing through all root-owned ancestors. |
| + |
| + Separately, the tenant UID is inferred from the file. An operator-set route directly naming another tenant’s otherwise-valid tree can qualify if no competing claim exposes it. |
| + |
| + **Smallest acceptable fix:** bind each non-platform unit route to an independently verified tenant UID/root, and refuse platform-UID tenants unless explicitly classified as trusted platform workloads. Test fixtures are not a reason to leave this runtime precondition unenforced. |
| + |
| +10. **P3 — Tests still depend on scheduler timing and temporary-root details.** |
| + |
| + **Locations:** `internal/ops/litestream_metered_test.go:783`, `:1367`, `:1474`; `internal/ops/litestream_owners_unix_test.go:25`, `:45`. |
| + |
| + `TestTheLastCleanLookIsItsStart` fails if the goroutine is descheduled for more than 100 ms between its `before` timestamp and entering `lookAgain`, despite correct code. The 300 ms second-Close observation does not establish that the second caller was scheduled. |
| + |
| + Under umask `077`, the supposedly public `.replicator` fixture created with mode `0755` becomes `0700`, defeating its intended refusal. `withOwners` overrides every matching basename, including ancestors outside the fixture, and assumes UIDs `44001/44002` differ from the process UID. Unreadable-directory coverage is skipped as root. |
| + |
| + **Smallest acceptable fix:** use controlled clock/read barriers, explicit fixture modes, and owner mappings scoped by file identity or full fixture path. Choose synthetic UIDs distinct from the running process. |
| + |
| +The remaining requested checks hold with these limits: |
| + |
| +- **A/J:** On a stable canonical resolved path, the inferred `tenantFile` chain rejects a distinct non-platform user’s directory and tenant-made links. A final-component `DBPath` symlink is refused at `internal/ops/litestream_owners.go:321`. Every accepting `tenantFile` path reaches the top check at line 384. Findings 2, 5 and 9 prevent turning this into an unconditional tenant-isolation claim. |
| +- **B/I:** The checks before locking, under `mu`, and after the witness read close the reported admission interleavings. Close cannot invalidate `obs` during the read because it needs the same mutex. The final stale load at `internal/ops/litestream_metered.go:234` is the grant point; staleness can occur immediately afterwards, before dispatch. |
| +- **Retries:** `witnessUnchanged` makes three attempts, delays only between errors, accepts recovery, and immediately returns a witnessed `false`. Its configured read-and-delay allowance is 15.4 seconds, not an end-to-end bound: mutex waiting and filesystem operations are outside it. The request context is not checked again after acquiring the mutex. |
| +- **C:** The four triggers at `internal/registry/registry.go:599` cover the six columns used by current discovery/attribution, including raw SQL from older builds. Changed-value updates exclude no-ops; the rowid trigger rejects the replacement bypass. “Nothing else moves it” needs qualification: database-free inserts/deletes, and watched-column changes on irrelevant rows, also invalidate. |
| +- **D/I:** `life`, `closeOnce`, store shutdown and full predecessor traversal prevent a replacement **within the declared chain** opening beside a draining store. Close skips its final witness read if already stale. Independent/forked chains remain caller-controlled. Inheritance is applied at most once per eligible fleet; finding 4 explains where eligibility leaves a recorder untouched. |
| +- **E:** Remote lineage and shadow state use account plus cleaned path at `internal/ops/litestream_metered.go:777`, `:801`. Different valid account IDs do not share keys, including after a rename. Same-account/path reuse is deliberate, including reuse by a different app within that account. |
| +- **F/L:** Case-variant database names are recognized; uppercase `.DB` is skipped rather than replicated. Unrecognized nonempty regular files—including sidecar names—are header-read. Recognized database names are classified without a header read. Unreadable directories, relative/no-path unit routes, missing unit claims, empty explicit candidates and discovered files failing final validation are reported. |
| +- **G/M:** The crash gap remains correctly open: a completed request can survive without a persisted transition hold. The one-minute margin also remains insufficient for an admitted attempt delayed longer than that interval; for example, admission and finding before 23:59, followed by dispatch/recording after midnight, can leave the later day outside the hold. These require durable transition state and tracking admitted attempts through completion. Same-filesystem bind mounts remain undetected by device-number comparison when mount authority is visible in the replicator’s namespace. External-inventory comparison and external-prefix separation remain unenforced. |
| + |
| +Claims the code contradicts: |
| + |
| +| Claim and location | Assessment | |
| +|---|---| |
| +| Every replica request is refused “from then on” — `internal/ops/litestream_metered.go:33` | **Still false.** Already-granted attempts can dispatch later. The qualified paragraph below does not remove the contradictory sentence. | |
| +| Every doubtful day reaches every relevant recorder — `internal/ops/litestream_metered.go:686` | **Too broad.** Findings 3–4, delayed admissions and the disclosed crash gap remain. | |
| +| Closing a previously current fleet holds nobody’s day — `internal/ops/litestream_metered.go:593` | **False without qualification.** Its final witness read can discover a change or exhaust retries; it can also inherit a predecessor’s hold. | |
| +| A missing pathname means nothing is being shipped — `internal/ops/litestream_metered.go:549` | **False.** Descriptors and pending shadow data can survive it. | |
| +| Every file was verified before advancing `lastClean` — `internal/ops/litestream_metered.go:558` | **False.** Missing files were explicitly skipped. | |
| +| Link targets are walked before cleaning — dispositions `:159`; `internal/ops/litestream_owners.go:642` | **False for relative targets.** `filepath.Join` already cleaned them at line 640. | |
| +| Platform-link targets must be absolute — `internal/ops/litestream_owners.go:333`, `:603` | **Stale.** Relative targets are now accepted. | |
| +| Inventory reports the resolved path — `internal/ops/litestream_owners_unix_test.go:113` | **False.** Its assertion and `OwnedDB.Path` retain the spelled path; only checked facts use the resolved destination. | |
| +| Every regular file is header-read — dispositions `:165` | **Literally false.** Empty files and recognized database names bypass header inspection. The sidecar-specific repair is correct. | |
| +| Strict discovery passes nothing over silently — `internal/ops/litestream_owners.go:486` | **Too broad.** Placeholder, churn, link and discovery-scope exclusions remain. | |
| +| The generation moves only for actual ownership changes — `internal/registry/registry.go:580` | **Overstated.** Database-free route changes can move it. The explicit trigger description and dispositions `:86` are more accurate. | |
| +| Four triggers — dispositions `:75`, `:171` | **Corrected.** Insert, delete, changed-value update and rowid immutability. | |
| +| Only an operator/systemd can create an undetected bind mount — dispositions `:95` | **Still present and too narrow.** The newer correction at `:171` correctly names namespace-visible mount authority. | |
| +| Exposing opened identity alone is an alternative preventive fix — dispositions `:84` | **Still present and false.** The newer correction at `:171` correctly limits it to detection after opening. | |
| +| Temporary-directory umask can add group-write permission — `internal/ops/litestream_owners_test.go:16` | **False.** Umask removes permissions. The newer explanation does not correct this comment. | |
| +| The relative-link regression fixture establishes normalization safety — dispositions `:159` | **False.** Its adversarial target contains no cancelling `..`. | |
| +| Package tests pass under both temporary roots — dispositions `:173` | **Not independently established here.** This source-only review found remaining environmental assumptions. | |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md| index e52a96829..c51ecfa08 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| @@ -171,3 +171,21 @@ Reports: `OPUS-REVIEW-metered-replicator-review6.md`, `CODEX-FINDINGS-metered-re |
| | Astra claims: the migration adds four triggers, not three; the umask explanation; "namespace-visible mount authority" for bind mounts; the older disposition that offered an identity comparison as a preventive fix for the first-open swap. | Corrected: four triggers (insert, delete, changed-value update, rowid); the umask sentence; bind mounts are undetected when whoever holds mount authority in the namespace makes one; the identity comparison detects a swap after the fact and prevents nothing, as the first-open comment says. | this file | | |
| Package tests pass under `/tmp` and under `~/.cache`. The full gate on this tip is next; the seventh build has not been read by a reviewer. |
| + |
| +## Review seven (reviewed `6b648c0c`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-metered-replicator-review7.md`, `CODEX-FINDINGS-metered-replicator-review7.md`. Both found the seventh build's fixes present and the same three gaps in them: a vanished owned file advanced the clean boundary, a relative link target was cleaned before it was walked, and a fleet between the chain's root and its last fleet never held its own recorders. Astra's P1s on litestream's own opens and on ancestry stand. This section records the eighth build, which is unreviewed. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P2-1, Astra 3 (P1): an owned file that had vanished was passed over as "nothing is being shipped" and the look counted as clean, so a swap made and removed between looks moved the clean boundary past itself. | Fixed: a vanished owned file is doubt from the last clean look like any other change; the boundary never advances through it. The removal case in the inventory table is `ErrOwnershipStale` now, not drift. | `litestream_metered.go` `lookAgain` | `TestAVanishedFileIsDoubtNotACleanLook` | |
| +| Opus P3-1, Astra 5 (P2): a relative link target was joined, and so cleaned, before it was walked, so a traversal through a tenant's link followed by `..` was folded away. | Fixed: the target is walked from the link's directory component by component as spelled, with no cleaning; the fixture spells its target as a string (a `filepath.Join` would have cleaned the link away before the test began, which is how the seventh build's test missed it) and checks the kernel's own resolution first. | `litestream_owners.go` `walkWithoutLinks` | `TestAPlatformLinksRelativeTargetIsWalkedNotCleaned` | |
| +| Astra 4 (P2): a fleet between the root of a chain and its last fleet, closed before the chain had closed, never held its own recorders when the chain latched. | Fixed: when a fleet latches, each unlatched fleet between it and the root is given its own predecessor's window on its own recorders, once. | `litestream_metered.go` `inheritHold` | `TestIntermediateFleetsRecordersAreHeldToo` | |
| +| Opus P3-2: every fleet kept its whole chain of predecessors alive and walked it. | Fixed: a fleet that has latched lets its chain go (`replaces = nil`), and the walks stop at the first latched fleet, whose predecessors were closed when it latched. | `litestream_metered.go` | the inheritance tests (unchanged results) | |
| +| Opus P2-2, Astra 8 (P3): the header open can still be steered to a device by a directory swapped higher up (`O_NOFOLLOW` covers the last component), which on a box with a watchdog device is a reboot primitive. | Open, stated plainly now: a walk by directory descriptor (`openat2` with `RESOLVE_BENEATH|RESOLVE_NO_SYMLINKS|RESOLVE_NO_MAGICLINKS`, or `os.Root`) is the fix and is required before this is wired; the `!unix` build now refuses header reads instead of opening by path. | `sqliteopen_other.go`; this file | | |
| +| Opus P3-3, Astra 9 (P3): ownership inferred from the file, not tied to the route; a tenant sharing the platform's uid. | Open, as before, with approval H: tie a unit route to its host's state directory and uid; refuse a platform-uid owner unless the route is the ops account. | | | |
| +| Astra 1, 2 (P1): litestream's own opens by path; `dataDir` and shadow ancestry. | Open, as before. | | | |
| +| Astra 6 (P2): an exec route's claimed file outside the globs is in no list; empty tree files; entries vanishing before their first look; no external-inventory comparison. | Open, stated: the explicit non-hibernating claim outside the globs is a real omission and is the next bounded item; the comparison against the external replicator's inventory stays a cutover requirement. | | | |
| +| Opus P3-4, Astra 7 (P2), Astra 10 (P3): tests that pass without their fixes (retained handles, worker exit, the watch's own call, exact counts, `O_NOFOLLOW` alone); scheduler timing; temp-root names in the seam. | Open in part, as stated; the eighth build adds the vanished-file, intermediate-recorder and relative-target cases, each of which fails without its fix. | | | |
| + |
| +Package tests pass under `/tmp` and under `~/.cache`. The full gate on this tip is next; the eighth build has not been read by a reviewer. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review7.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review7.md| new file mode 100644 |
| index 000000000..a5a96a44b |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review7.md |
| @@ -0,0 +1,191 @@ |
| +I read the seventh build as `d63f81df..6b648c0c` because the brief's `git diff 6b648c0c 6b648c0c` is empty, and the whole branch as `72b4f59a..6b648c0c`. Everything was read with `git show`; nothing was run or changed. Line numbers are at `6b648c0c`. |
| + |
| +# Seventh review: metered replicator at `6b648c0c` (Opus) |
| + |
| +**Verdict: accept with fixes.** |
| +- No new P1. |
| +- Most of review six's findings are closed, and their new tests pin them. |
| +- Two P2s: |
| + - A swap followed by a deletion is still never held. |
| + - The header read can still be steered to a device. Adding `O_NOCTTY` closed only the terminal case. |
| +- Astra 5 is fixed only for absolute link targets. |
| +- Astra 1 (swaps at litestream's opens) and Astra 4 (ancestry) still block any attestation. |
| + |
| +## Review six, finding by finding |
| + |
| +| Finding | Now | Would its test pass without its fix? | |
| +|---|---|---| |
| +| Opus P2-1: systemd's relative state link | **Closed** (`litestream_owners.go:639-641`) | **No.** The relative fixture (`unix_test.go:99`) fails under the old absolute-only rule. | |
| +| Opus P2-2 / Astra 2: a registry finding held only from the last proof | **Closed** (`litestream_metered.go:288-290`, `:310-315`) | **No.** Without the fix the hold starts at 00:00:29 on 10-03, so 10-02 is not held. | |
| +| Opus P2-3 / Astra 8: no `O_NOCTTY` on the header open | **Closed for terminals** (`sqliteopen_unix.go:22`). Devices that act when opened are still reached (P2-2 below). | No test, as stated. | |
| +| Opus P3-1 / Astra 3: a hold lost through two unopened fleets | **Closed** (`litestream_metered.go:418-422`) | **No.** With only the nearest fleet checked, C latches nothing and D holds nothing. Exactly-once delivery is still not asserted. | |
| +| Opus P3-2: the link-above test passed without its fix | **Partly.** The fixture now has a real directory below the link. | The test now fails without `resolvePlatformLinks` (it refuses `T/a` at `:626`). It still passes without the check it is named for (`:384`, `:392`), and nothing can pin that check: see H. | |
| +| Opus P3-3: tests that did not pin their fixes | **Partly.** `TestTheLastCleanLookIsItsStart` fails if the look's end is stored, or if `:560` is deleted. | Still untested: `Open`'s `life` lock, retained handles, worker exit, and the watch calling `lookAgain` itself. | |
| +| Opus P3-4: tests depend on the temp root | **Partly.** Relative links in the temp path (macOS `/var`) now resolve. | The name-keyed seam and the link-under-a-foreign-`/` dependence remain (H). | |
| +| Opus P3-5: a tenant with the platform's uid | **Open**, stated as an operator rule. | n/a | |
| +| Opus P3-6: two fleets outside one chain | **Stated** in `MeteredFleetOptions.Replaces` (`:106-109`) | n/a | |
| +| Opus P3-7: silent drops | **Closed** for a vanished glob match (`litestream.go:426-431`, `:500`) and for databases under sidecar names. Hibernating routes are unlisted by design, as stated. | **Yes** for the glob case: deleting `globbed[p]` passes every test (none exists). **No** for sidecar names: `beside-journal` with the database header is now asserted reported. | |
| +| Astra 1 (P1): litestream's opens by path | **Open**, stated | n/a | |
| +| Astra 4 (P1): ancestry of `dataDir`, shadow state and platform links | **Open**, stated | n/a | |
| +| Astra 5 (P2): cleaning a link target can hide a tenant's link | **Closed for absolute targets only.** `filepath.Join` at `:640` cleans a relative target before `noLinkInPath` walks it (P3-1 below). | **Yes.** The fixture's target `/…/hop/choice` has no `..` in it, so `Clean` followed by `EvalSymlinks` (the old code) also refuses it with the same message. | |
| +| Astra 6 (P2): a sidecar suffix hides a database | **Closed** (`:543-554`) | **No.** The WAL fixture now carries the real WAL magic. | |
| +| Astra 7 (P2): a claimed exec database vanishes silently | **Closed in code** | No test. | |
| +| Astra 9 (P2): weak tests | **Partly**, as Opus P3-3 | | |
| +| Claims, both reviewers | **Corrected:** four triggers, the umask sentence, the bind-mount wording, the `O_NOCTTY` reasoning. **Still wrong:** see Claims below. | | |
| + |
| +## Findings |
| + |
| +### P2-1. A swap followed by a deletion is treated as a clean look, so it is never held (M) |
| + |
| +- **Where:** `litestream_metered.go:548-551`, then `:560`. |
| +- **Scenario:** |
| + 1. A unit route owns `T/x.db`. After look N, T swaps it for a link to V's database. |
| + 2. Litestream opens the path while the swap is in place, as any new pooled connection or reopen does (this is Astra 1's opening). |
| + 3. Litestream reads V's pages, snapshots them into T's prefix, and meters them to T. |
| + 4. T deletes the link before look N+1. |
| + 5. Look N+1 finds the path gone, hits `continue` ("nothing is being shipped from it"), and moves `lastClean` forward to its own start. |
| + 6. The changed inventory sets `drift`, so the caller rebuilds and closes the old fleet as current. Nothing is ever held. |
| +- **Why this is a finding:** the dispositions already concede a swap that is put back before the next look. Here the look has the evidence (the path has gone) and discards it. Litestream also keeps the descriptor it opened, which is the false claim both reviewers flagged in review six. |
| +- **Fix:** |
| + - A vanished owned path makes the fleet stale: `goStaleFrom(f.lastCleanLook(), time.Now())`. |
| + - Or, at least, a vanished path does not advance `lastClean`, and `drift` is set. |
| + - Correct the comment at `:549`. |
| + - Add a test that removes an owned path between two looks and asserts the hold. |
| + |
| +### P2-2. The header read can still be steered to a device, and the stated open item understates it (K) |
| + |
| +- **Where:** `sqliteopen_unix.go:22`, reached from the walk at `litestream_owners.go:549`. |
| +- **Scenario:** |
| + 1. The walk `Lstat`s `T/d/watchdog` and finds a small regular file. |
| + 2. T flips `d` to a link to `/dev`. `O_NOFOLLOW` covers only the last component. |
| + 3. The root process opens `/dev/watchdog`. The `fstat` afterwards refuses it, but the device is already armed. Closing it without the magic character leaves the timer running, so the box reboots wherever such a device exists and nothing else holds it. |
| + 4. The look runs every 30 s, so the race can be retried indefinitely. |
| +- **Disposition:** "a directory swapped higher up cannot steer the open to a device" undersells this. It is a reboot primitive for any tenant. |
| +- **Fix, before wiring:** |
| + - Open with `openat2(treeRoot, rel, RESOLVE_BENEATH|RESOLVE_NO_SYMLINKS|RESOLVE_NO_MAGICLINKS)`. |
| + - Or walk by directory descriptor with `openat` and `O_NOFOLLOW|O_DIRECTORY` per component. |
| + - `os.Root` (Go 1.26 is in use) also refuses the escape. |
| + - Make `!unix` refuse rather than call `os.Open`. |
| + |
| +### P3-1. Astra 5 is still open for relative link targets (J) |
| + |
| +- **Where:** `litestream_owners.go:639-647`. `filepath.Join(prefix, target)` calls `Clean`. |
| +- **Scenario:** |
| + 1. There is a root-made link `/pub/A -> ../srv/T/choice/../../safe/A`, and T makes `/srv/T/choice` a link to `/victims/V/x/y`. |
| + 2. `Join` gives `/srv/safe/A`, which `noLinkInPath` passes. |
| + 3. The kernel resolves the same target to `/victims/V/safe/A`. |
| + 4. `OwnedDB.Path` stays the spelled `/pub/A/…` (`:190`), which litestream opens. |
| +- **Limit:** it needs a non-canonical relative link made by root. systemd's `../private/…` is canonical. |
| +- **Comment contradicted:** `:637-638` says "the lexical join is the kernel's path". That is true only for a leading `..`. |
| +- **Fix:** `target = prefix + "/" + target`, without cleaning, before `noLinkInPath`. Add a fixture with a relative target that passes through a tenant's link and then `..`. |
| + |
| +### P3-2. Every fleet keeps its whole chain of predecessors alive, and walks it (D, I) |
| + |
| +- **Where:** `litestream_metered.go:418`, `:468`, `:376`. |
| +- **Scenario:** |
| + 1. Each rebuild sets `Replaces` to the previous fleet, and every route add or drift is a rebuild. |
| + 2. After weeks, the chain runs back to the first fleet ever built. |
| + 3. Every fleet in it keeps its routes, facts and recorders, which may hold ledger handles. `Open` and `inheritHold` walk all of it. |
| +- **Fix:** |
| + - A latched fleet's predecessors are all closed by construction, so both loops can stop at the first latched fleet. |
| + - Clear `replaces` once a fleet has latched (under `staleMu`). |
| + |
| +### P3-3. Ownership is still inferred from the file, not tied to the route (A, J; carried) |
| + |
| +- **Platform-uid tenants, and boxes whose units run as root:** |
| + - `:359-367` and `:379` stop the climb at once. |
| + - A file is accepted from any root-owned directory with a mode of 0755 or stricter whose parent is the same. |
| + - Any root-owned link counts as "the platform's" (`:629`). |
| +- **A `DBPath` naming another tenant's file directly:** |
| + - It is accepted and billed to the naming route unless the other tenant's route lists the same file. |
| + - Example: route A names `/var/lib/private/carlos-app/B/other.db`, while B's route names only `app.db`. |
| +- **Limit:** `DBPath` is set by the operator. Stated (review four P3-6, review six P3-5). |
| +- **Fix, with approval H:** |
| + - Tie a unit route to its host's state directory and to the unit's uid. |
| + - Refuse a platform-uid owner unless the route is the ops account. |
| + |
| +### P3-4. Tests (H) |
| + |
| +- **Astra 5 fixture** (`unix_test.go:146-161`): passes with `Clean` restored (see the table). |
| +- **`:384` and `:392` cannot be reached** without a race. `resolvePlatformLinks` returns a path with no link in it, `start` is `Lstat`-checked, and the climb `Lstat`s every parent. So `TestUnitFileChainRefusesALinkAboveIt` cannot pin them. Say they are defence in depth, or drop them. |
| +- **Untested:** the `globbed[p]` branch, `O_NOCTTY`, and `Open` racing `Close` (removing `f.life.Lock()` from `Open` still passes). The watch's own `lookAgain` call, retained handles and worker exit are unchanged since review six. |
| +- **Temp-root dependence:** |
| + - `resolvePlatformLinks` checks the directory holding any link in the temp root's path. |
| + - A runner whose `/` belongs to `nobody` under a user namespace (the shape of the CI failure) refuses every tenant-file fixture if `TMPDIR` passes through a link. |
| + - `withOwners` overrides owners by base name over every component, temp-root names included. |
| +- **Wall clock:** `TestTheLastCleanLookIsItsStart` uses windows of 100 ms and 200 ms. That is low risk, but it depends on the scheduler. |
| + |
| +## A–M |
| + |
| +- **A. Unit-file chain climb.** |
| + - A tenant with its own user cannot get a link or another user's directory into its chain: |
| + - links are refused by the resolver (`:626-631`) and by the walk (`:417`); |
| + - another user's directory stops the climb and must then pass `platformOwned` (`:395`); |
| + - a platform-owned start directory needs a platform-owned file (`:436`). |
| + - A platform-uid tenant, or units running as root: see P3-3. |
| + - The top-of-chain checks are reached on every accepting path. |
| +- **B. Request gate and admission.** |
| + - A request admitted before a change still reaches the network. This is inherent and covered by the margin. |
| + - The stale re-check under the lock (`:220`, `:234`) is complete up to the instant of return. |
| + - Witness retries are bounded at 3 × 5 s + 2 × 200 ms. Only exhausted errors count as a change, and a witnessed `false` is immediate. |
| + - The request's context is not re-checked after waiting for `f.mu`. This is minor. |
| +- **C. Ownership generation.** |
| + - Unchanged this build, and correct. |
| + - Upsert's `DO UPDATE` fires the column trigger. |
| + - `replication` is not read by discovery, so it rightly does not move the generation. |
| +- **D. Holds handed between fleets.** |
| + - No reading is taken on a stale fleet. |
| + - The inherited window is applied once per fleet (the CAS) and is now chain-complete. |
| + - The only gaps are P2-1 and P3-2. |
| +- **E. Lineage and shadow state.** |
| + - Keyed by account plus path (`:801-803`). Different accounts never share or overwrite. |
| + - A path renamed between owners starts a new key. |
| + - The same account (any app) reuses the lineage, by design. |
| + - A→B→A resumes A's old lineage and shadow state. That is worth stating. |
| +- **F. Strict tree walk.** |
| + - Names in other cases are skipped, not dropped. |
| + - Every non-empty regular file is read for the header. |
| + - An unreadable directory, a unit route with no `DBPath`, and a relative unit path are problems. |
| + - An empty claimed file is skipped. |
| +- **G. Open items.** |
| + - Stated correctly: the crash gap, bind mounts, the external inventory, ancestry. |
| + - Larger than stated: |
| + - the first-open swap covers every litestream reopen, and the deletion case (P2-1); |
| + - device opens (P2-2); |
| + - Astra 5 for relative targets (P3-1). |
| +- **I. Admission, `witnessUnchanged`, `closeOnce`.** |
| + - After the file finding, nothing is admitted except at the instant between `:234` and the return. |
| + - `closed` is published only after `store.Close` and the last reading, so no replacement in the chain can open beside a draining store. |
| +- **J. Chain start and the platform-made link.** |
| + - A platform link whose target's parent is not the platform's is refused. The target's own ancestry is unchecked (Astra 4). |
| + - A `DBPath` that is itself a link is refused at `:322`. The duplicate-file check then also marks the linked file unowned, which fails closed. |
| + - Relative targets: P3-1. |
| +- **K. `openRegularNoFollow` and sidecars.** |
| + - A swap between the `Lstat` and the open is caught by the device-and-inode check, but only after the open has done its work (P2-2). |
| + - `sidecarOf` is gone. A sidecar whose base is a link fails the sibling check (`:447`). |
| + - On `!unix` the open is a plain `os.Open` with no checks. |
| +- **L. Inventory reports.** |
| + - Silent still: |
| + - hibernating routes (by design, stated); |
| + - exec routes outside the globs (stated); |
| + - entries that vanish during the walk (churn; drift catches them on the next look). |
| + - Nothing else is dropped. |
| +- **M. A day in doubt left unheld.** P2-1. The crash case and requests held longer than the margin remain, as stated. |
| + |
| +## Claims the code contradicts |
| + |
| +- **`ErrOwnershipStale`** (`litestream_metered.go:28-35`): still says "Every replica request is refused from then on". This is the fourth round, and the doc comment is duplicated. |
| +- **`Close` doc** (`:591-595`): "Closing a fleet that was current holds nobody's day". `:648-651` goes stale. |
| +- **`lookAgain`** (`:549`): "Gone altogether: nothing is being shipped from it" (P2-1). |
| +- **`goStale` doc** (`:276`): "For a change in the registry that instant is the last proof". It is now the earlier of that proof and the last clean look. |
| +- **`goStaleFrom`** (`:309`): "at most one look's span of extra hold". The hold reaches further when a look errors (`:538-541` does not advance `lastClean`), or when `Open` comes long after the build. |
| +- **`resolvePlatformLinks` doc** (`:603-604`) and **`checkOwnedFile`** (`:333-334`): "its target is absolute". Relative targets are now accepted. |
| +- **`:637-638`**: "the lexical join is the kernel's path" (P3-1). |
| +- **`:387-391`**: "a tenant's link above the chain … is caught here". The resolver catches it, and this check cannot be reached. |
| +- **`strictTreeDBs` doc** (`:491-492`): "a file with no extension that begins with SQLite's header". Every regular file is read. |
| +- **`unix_test.go:113`**: "reports the file by its resolved path". `:121-123` asserts the spelled path. |
| +- **Dispositions, Astra 5 row:** "walked, not cleaned … same test". True for absolute targets only, and the cited test passes without the change. |
| + |
| +**Verdict: accept with fixes.** |
| +- Before merge: P2-1, P3-1, P3-2, and the claims. |
| +- Before any attestation or wiring: P2-2 (the walk by descriptor), P3-3, Astra 1 and Astra 4. |
diff --git a/internal/ops/litestream_metered.go b/internal/ops/litestream_metered.go| index 017020b26..b0a21f906 100644 |
| --- a/internal/ops/litestream_metered.go |
| +++ b/internal/ops/litestream_metered.go |
| @@ -370,10 +370,10 @@ func (f *MeteredFleet) staleWindow() (from, at time.Time, stale bool) { |
| f.staleMu.Lock() |
| own, ownFrom, ownAt := f.stale.Load(), f.staleFrom, f.staleAt |
| inherited, inhFrom, inhAt := f.inheritedStale, f.inheritedFrom, f.inheritedAt |
| - latched := f.inherited.Load() |
| + latched, replaces := f.inherited.Load(), f.replaces |
| f.staleMu.Unlock() |
| - if !latched && f.replaces != nil { |
| - inhFrom, inhAt, inherited = f.replaces.staleWindow() |
| + if !latched && replaces != nil { |
| + inhFrom, inhAt, inherited = replaces.staleWindow() |
| } |
| switch { |
| case own && inherited: |
| @@ -416,17 +416,47 @@ func (f *MeteredFleet) inheritHold() { |
| // closed and said its last, and latching before then would latch |
| // nothing. |
| for prev := f.replaces; prev != nil; prev = prev.replaces { |
| + if prev.inherited.Load() { |
| + break // its own predecessors were closed when it latched |
| + } |
| if !prev.closed.Load() { |
| return |
| } |
| } |
| + // The fleets between this one and the root of the chain closed before |
| + // the chain had, and could not latch then: each is given its own |
| + // predecessor's window now, once, so a day in doubt reaches every |
| + // owner's recorder and not only the last fleet's. |
| + for prev := f.replaces; prev != nil && prev.replaces != nil; { |
| + next := prev.replaces |
| + if prev.inherited.CompareAndSwap(false, true) { |
| + from, at, stale := next.staleWindow() |
| + prev.staleMu.Lock() |
| + if stale { |
| + prev.inheritedFrom, prev.inheritedAt, prev.inheritedStale = from, at, true |
| + } |
| + prev.replaces = nil |
| + prev.staleMu.Unlock() |
| + if stale { |
| + holdDays(prev.recorders, from, at) |
| + } |
| + } |
| + prev = next |
| + } |
| if !f.inherited.CompareAndSwap(false, true) { |
| return |
| } |
| - if from, at, stale := f.replaces.staleWindow(); stale { |
| - f.staleMu.Lock() |
| + from, at, stale := f.replaces.staleWindow() |
| + f.staleMu.Lock() |
| + if stale { |
| f.inheritedFrom, f.inheritedAt, f.inheritedStale = from, at, true |
| - f.staleMu.Unlock() |
| + } |
| + // Latched: what the chain had to say is kept here, and the chain |
| + // itself is let go, so a box rebuilt many times does not keep every |
| + // fleet it ever had alive behind the current one. |
| + f.replaces = nil |
| + f.staleMu.Unlock() |
| + if stale { |
| holdDays(f.recorders, from, at) |
| } |
| } |
| @@ -466,6 +496,9 @@ func (f *MeteredFleet) Open(ctx context.Context) error { |
| // a fleet closed without opening passes its predecessor on, and two |
| // stores over one database would share its shadow state and prefix. |
| for prev := f.replaces; prev != nil; prev = prev.replaces { |
| + if prev.inherited.Load() { |
| + break // its own predecessors were closed when it latched |
| + } |
| if !prev.closed.Load() { |
| return errors.New("ops: the replicator this one replaces has not been closed") |
| } |
| @@ -545,13 +578,12 @@ func (f *MeteredFleet) lookAgain() { |
| if still && now.Dev == was.Dev && now.Ino == was.Ino { |
| continue |
| } |
| - if _, err := os.Lstat(path); errors.Is(err, os.ErrNotExist) { |
| - // Gone altogether: nothing is being shipped from it. |
| - continue |
| - } |
| - // Held from before the last look that found every file as it was |
| - // began: the file may have changed at any moment since, this |
| - // look's own span included. |
| + // Changed, or gone: a file that is no longer there is not proof |
| + // that nothing was shipped from its path since the last clean look |
| + // (a handle litestream holds outlives the name, and a swap made and |
| + // removed in between is unseen), so its absence is doubt from the |
| + // last clean look like any other change, and never advances the |
| + // clean boundary. |
| f.goStaleFrom(f.lastCleanLook(), time.Now()) |
| return |
| } |
diff --git a/internal/ops/litestream_metered_test.go b/internal/ops/litestream_metered_test.go| index 55dcd252c..01a8429dd 100644 |
| --- a/internal/ops/litestream_metered_test.go |
| +++ b/internal/ops/litestream_metered_test.go |
| @@ -836,10 +836,12 @@ func TestMeteredFleetLooksAtTheFilesAgain(t *testing.T) { |
| writeFile(t, filepath.Join(f.state, ".hidden", "x.db")) |
| }, ErrInventoryChanged}, |
| "the database is removed": {func(t *testing.T, f fixture) { |
| + // A file that is gone is doubt, not drift: a handle outlives the |
| + // name and a swap made and removed in between is unseen. |
| if err := os.Remove(f.db); err != nil { |
| t.Fatal(err) |
| } |
| - }, ErrInventoryChanged}, |
| + }, ErrOwnershipStale}, |
| "another file is put at the database's path": {func(t *testing.T, f fixture) { |
| other := writeFile(t, filepath.Join(f.state, "other")) |
| if err := os.Rename(other, f.db); err != nil { |
| @@ -1509,3 +1511,73 @@ func TestAHoldReachesThroughTwoUnopenedFleets(t *testing.T) { |
| t.Fatalf("D holds %v; want A's day, through B and C", got) |
| } |
| } |
| + |
| +// A fleet between the root of a chain and its last fleet, closed before the |
| +// chain had closed, is given its predecessor's window on its own recorders |
| +// when the chain finally latches: the day in doubt reaches every owner's |
| +// recorder, not only the last fleet's. |
| +func TestIntermediateFleetsRecordersAreHeldToo(t *testing.T) { |
| + day := func(d int) time.Time { return time.Date(2026, 10, d, 12, 0, 0, 0, time.UTC) } |
| + a := &MeteredFleet{} |
| + bRec := &countingRecorder{} |
| + b := &MeteredFleet{replaces: a, recorders: []lease.RequestRecorder{bRec}} |
| + b.inheritHold() // A open: nothing yet |
| + b.closed.Store(true) |
| + a.proven.Store(day(2).UnixNano()) |
| + a.goStale(day(2)) |
| + a.closed.Store(true) |
| + cRec := &countingRecorder{} |
| + c := &MeteredFleet{replaces: b, recorders: []lease.RequestRecorder{cRec}} |
| + c.inheritHold() |
| + if got := cRec.days(); !got["2026-10-02"] { |
| + t.Fatalf("C holds %v; want A's day", got) |
| + } |
| + if got := bRec.days(); !got["2026-10-02"] { |
| + t.Fatalf("B, the fleet between, holds %v; want A's day on its own recorder", got) |
| + } |
| + // Once: a second latch by a later fleet adds nothing to B. |
| + _, _, before := bRec.counts() |
| + d := &MeteredFleet{replaces: c, recorders: []lease.RequestRecorder{&countingRecorder{}}} |
| + c.closed.Store(true) |
| + d.inheritHold() |
| + if _, _, after := bRec.counts(); after != before { |
| + t.Fatalf("B was told again (%d -> %d)", before, after) |
| + } |
| +} |
| + |
| +// A file that disappears from its owned path does not advance the clean |
| +// boundary: it is doubt from the last clean look, like a changed file. |
| +func TestAVanishedFileIsDoubtNotACleanLook(t *testing.T) { |
| + newFakeBucket(t) |
| + dir := platformDir(t) |
| + reg := openRegistry(t, filepath.Join(dir, "registry.db")) |
| + db := writeFile(t, filepath.Join(dir, "a.example.com.db")) |
| + upsert(t, reg, registry.Route{Host: "a.example.com", Account: "bac", App: "one", DBPath: db}) |
| + recs := &recorders{} |
| + o := fleetOptions(reg, dir) |
| + o.Meter = recs.meter |
| + fleet, err := MeteredFleetStore(o) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer fleet.Close() |
| + lastClean := time.Now().UTC().Add(-48 * time.Hour) |
| + fleet.lastClean.Store(lastClean.UnixNano()) |
| + fleet.proven.Store(time.Now().UnixNano()) |
| + if err := os.Remove(db); err != nil { |
| + t.Fatal(err) |
| + } |
| + fleet.lookAgain() |
| + if !fleet.stale.Load() { |
| + t.Fatal("a vanished owned file left the fleet current") |
| + } |
| + if got := time.Unix(0, fleet.lastClean.Load()); !got.Equal(lastClean) { |
| + t.Fatalf("the clean boundary moved to %v", got) |
| + } |
| + want := lastClean.Format("2006-01-02") |
| + for path, rec := range recs.all() { |
| + if !rec.days()[want] { |
| + t.Errorf("%s: the last clean look's day (%s) is not held after the file vanished; held %v", path, want, rec.days()) |
| + } |
| + } |
| +} |
diff --git a/internal/ops/litestream_owners.go b/internal/ops/litestream_owners.go| index 203c0cb44..2d15208a4 100644 |
| --- a/internal/ops/litestream_owners.go |
| +++ b/internal/ops/litestream_owners.go |
| @@ -569,32 +569,36 @@ func sqliteName(name string) bool { |
| return false |
| } |
| -// noLinkInPath says why path cannot be taken as it is spelled: some |
| -// component of it, walked from the root without cleaning, is a link or |
| -// cannot be looked at. "" when every component is a real directory (or, |
| -// for the last, anything that is not a link). |
| -func noLinkInPath(path string) string { |
| - parts := strings.Split(strings.TrimPrefix(path, string(filepath.Separator)), string(filepath.Separator)) |
| - prefix := string(filepath.Separator) |
| - for _, part := range parts { |
| +// walkWithoutLinks walks rel (an absolute or relative path, as spelled) |
| +// from start, one component at a time, looking at each with Lstat: a |
| +// component that is a link, or cannot be looked at, refuses; ".." steps |
| +// up only from a directory already looked at. The result is the path the |
| +// kernel would reach, with no cleaning done before the walk, so a |
| +// traversal through a link followed by ".." is not folded away. |
| +func walkWithoutLinks(start, rel string) (string, string) { |
| + at := filepath.Clean(start) |
| + if filepath.IsAbs(rel) { |
| + at = string(filepath.Separator) |
| + } |
| + for _, part := range strings.Split(rel, string(filepath.Separator)) { |
| if part == "" || part == "." { |
| continue |
| } |
| if part == ".." { |
| - prefix = filepath.Dir(prefix) |
| + at = filepath.Dir(at) |
| continue |
| } |
| - next := filepath.Join(prefix, part) |
| + next := filepath.Join(at, part) |
| fi, err := os.Lstat(next) |
| if err != nil { |
| - return next + " cannot be looked at" |
| + return "", next + " cannot be looked at" |
| } |
| if fi.Mode()&os.ModeSymlink != 0 { |
| - return next + " is a link" |
| + return "", next + " is a link" |
| } |
| - prefix = next |
| + at = next |
| } |
| - return "" |
| + return at, "" |
| } |
| // resolvePlatformLinks resolves the links the platform made in a spelled |
| @@ -634,18 +638,20 @@ func resolvePlatformLinks(dir string) (string, string) { |
| return "", "a link in its path cannot be read" |
| } |
| // systemd makes the state link relative (../private/carlos-app/<host>); |
| - // a relative target is taken from the link's own directory, which |
| - // is link-free by now, so the lexical join is the kernel's path. |
| + // a relative target starts from the link's own directory, which is |
| + // link-free by now. The target is walked component by component, |
| + // never cleaned first, so a traversal through a link the tenant |
| + // controls (choice/../x) is seen for what it is, and ".." steps up |
| + // only through directories already looked at. |
| + start := string(filepath.Separator) |
| if !filepath.IsAbs(target) { |
| - target = filepath.Join(prefix, target) |
| + start = prefix |
| } |
| - // The target itself holds no link in any component: it is walked, |
| - // not cleaned, so a traversal through a link the tenant controls |
| - // (choice/../x) is seen for what it is. |
| - if why := noLinkInPath(target); why != "" { |
| + resolvedTarget, why := walkWithoutLinks(start, target) |
| + if why != "" { |
| return "", "a link in its path leads through another link: " + why |
| } |
| - target = filepath.Clean(target) |
| + target = resolvedTarget |
| if tfi, terr := os.Lstat(target); terr != nil || !tfi.IsDir() { |
| return "", "a link in its path does not lead to a directory" |
| } |
diff --git a/internal/ops/litestream_owners_unix_test.go b/internal/ops/litestream_owners_unix_test.go| index fcee18fe0..7bcf8450c 100644 |
| --- a/internal/ops/litestream_owners_unix_test.go |
| +++ b/internal/ops/litestream_owners_unix_test.go |
| @@ -276,3 +276,54 @@ func TestStrictTreeDoesNotBlockOnAFIFO(t *testing.T) { |
| t.Fatalf("a link in a file's place was followed: %v %v", isDB, err) |
| } |
| } |
| + |
| +// A platform link's relative target is walked as spelled, not cleaned: a |
| +// target that passes through a link the tenant controls and then steps |
| +// back with ".." reaches, on the filesystem, wherever that link points, |
| +// while a lexical cleaning would hide the link. Such a target is refused, |
| +// whether the lexical and the physical destinations hold different files |
| +// or the same. |
| +func TestAPlatformLinksRelativeTargetIsWalkedNotCleaned(t *testing.T) { |
| + const tenant, other = 44001, 44002 |
| + top := platformDir(t) |
| + public := filepath.Join(top, "public") |
| + srv := filepath.Join(top, "srv") |
| + victims := filepath.Join(top, "victims") |
| + for _, d := range []string{public, filepath.Join(srv, "T"), filepath.Join(srv, "safe", "A"), filepath.Join(victims, "V", "x", "y"), filepath.Join(victims, "V", "safe", "A")} { |
| + if err := os.MkdirAll(d, 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + // The lexical destination holds a platform-looking database; the |
| + // physical one, V's. |
| + lexical := writeFile(t, filepath.Join(srv, "safe", "A", "main.db")) |
| + physical := writeFile(t, filepath.Join(victims, "V", "safe", "A", "main.db")) |
| + if err := os.Symlink(filepath.Join(victims, "V", "x", "y"), filepath.Join(srv, "T", "choice")); err != nil { |
| + t.Fatal(err) |
| + } |
| + // Root's link in a root directory, with a relative target through T's |
| + // link. (Spelled as a string: filepath.Join would clean the ".." |
| + // away and make a different link.) |
| + if err := os.Symlink("../srv/T/choice/../../safe/A", filepath.Join(public, "A")); err != nil { |
| + t.Fatal(err) |
| + } |
| + if real, err := filepath.EvalSymlinks(filepath.Join(public, "A")); err != nil || real != filepath.Join(victims, "V", "safe", "A") { |
| + t.Fatalf("fixture: the kernel resolves the link to %q %v; want V's directory", real, err) |
| + } |
| + withOwners(t, map[string]uint32{"public": 0, "srv": 0, "safe": 0, "A": 0, "main.db": 0, "T": tenant, "link:choice": tenant, "link:A": 0, "victims": 0, "V": other, "x": other, "y": other}) |
| + spelled := filepath.Join(public, "A", "main.db") |
| + _, why := checkOwnedFile(filepath.Dir(spelled), spelled, tenantFile) |
| + if why == "" || !strings.Contains(why, "another link") { |
| + t.Fatalf("a relative target through the tenant's link: %q (lexical %s, physical %s)", why, lexical, physical) |
| + } |
| + // The same target with no link in it resolves and is accepted. |
| + if err := os.Remove(filepath.Join(public, "A")); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := os.Symlink("../srv/T/../safe/A", filepath.Join(public, "A")); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, why := checkOwnedFile(filepath.Dir(spelled), spelled, tenantFile); why != "" { |
| + t.Fatalf("a relative target with a plain .. : %q", why) |
| + } |
| +} |
diff --git a/internal/ops/sqliteopen_other.go b/internal/ops/sqliteopen_other.go| index c9ce8000c..a0ed9bb7d 100644 |
| --- a/internal/ops/sqliteopen_other.go |
| +++ b/internal/ops/sqliteopen_other.go |
| @@ -2,8 +2,14 @@ |
| package ops |
| -import "os" |
| +import ( |
| + "errors" |
| + "os" |
| +) |
| // openRegularNoFollow: see sqliteopen_unix.go. The replicator never runs |
| -// where this is built; the ordinary open keeps the CLI compiling. |
| -func openRegularNoFollow(path string, _ os.FileInfo) (*os.File, error) { return os.Open(path) } |
| +// where this is built, and nothing is owned here (factsOf answers false); |
| +// a header read is refused rather than done by an open that follows links. |
| +func openRegularNoFollow(path string, _ os.FileInfo) (*os.File, error) { |
| + return nil, &os.PathError{Op: "open", Path: path, Err: errors.New("header reads are not supported on this platform")} |
| +} |