amadan
Metered replicator: the eighth review's two reports
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 files changed,
+326
−0
docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review8.md+174 −0docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review8.md+152 −0
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review8.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review8.md| new file mode 100644 |
| index 000000000..49d76925f |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review8.md |
| @@ -0,0 +1,174 @@ |
| +All locations are at `b5aecf13`. Test counterfactuals below are source-based; no tests or mutations were run. The requested incremental diff, `b5aecf13..b5aecf13`, is empty; this reviews `72b4f59a..b5aecf13`. |
| + |
| +| Review-seven finding | Closed? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P2-1:** vanished owned file advances the clean boundary | **Yes.** Disappearance now makes the fleet stale without advancing `lastClean`. | **No.** `TestAVanishedFileIsDoubtNotACleanLook` asserts staleness, the unchanged boundary and the earlier day’s hold. | |
| +| **Opus P2-2:** header inspection can open a device | **Partly.** The non-Unix fallback now refuses; Unix ancestor substitution remains possible. | **Yes for the remaining gap.** No test prevents opening through a substituted ancestor. | |
| +| **Opus P3-1:** relative link target cleaned before traversal | **Yes, for the reported sequence.** `walkWithoutLinks` examines the original components. | **No.** The new relative-target fixture contains the cancelling `..` and distinguishes lexical from physical destinations. | |
| +| **Opus P3-2:** predecessor chains retained indefinitely | **Retention fixed, but the stopping condition introduces finding 1.** | **Yes.** Removing predecessor detachment leaves the inheritance assertions satisfied; chain length/reachability is not asserted. | |
| +| **Opus P3-3:** ownership inferred from files; platform-UID tenants | **No.** Still an unenforced operator precondition. | No isolation regression test; a fixture explicitly accepts platform-user links. | |
| +| **Opus P3-4:** ineffective tests and environmental assumptions | **Partly.** The three new regression fixtures improve coverage. The remaining omissions persist. | **Yes** for several material mutations described below. | |
| +| **Opus claims** | **Mixed.** Missing-file and relative-normalization claims are repaired. Lifecycle, refusal, timing and pathname claims still overstate the code. | Comments themselves have no behavioral test. | |
| +| **Astra 1:** Litestream opens and retains substituted files | **No.** | No test constrains Litestream’s actual opens. | |
| +| **Astra 2:** protected endpoints with tenant-replaceable ancestry | **No.** | Existing tests pass with that ancestry unchecked. | |
| +| **Astra 3:** missing pathname erases a doubtful day | **Yes.** Same fix as Opus P2-1. | **No.** Both the new vanished-file test and the updated removal case distinguish the fix. | |
| +| **Astra 4:** unopened intermediate’s recorders never receive the inherited hold | **Closed for the reported unlatched chain. Not closed generally:** finding 1 prematurely latches through a live, already-inherited predecessor. | **No** for removing intermediate delivery: `TestIntermediateFleetsRecordersAreHeldToo` checks B’s recorder. **Yes** for the newly introduced premature-latch defect. | |
| +| **Astra 5:** unsafe relative-target normalization | **Yes.** Same fix as Opus P3-1. | **No.** `TestAPlatformLinksRelativeTargetIsWalkedNotCleaned` distinguishes it. | |
| +| **Astra 6:** inventory omissions outside repaired candidate paths | **No.** Explicit exec claims outside the globs remain absent. | No regression test for that omission. | |
| +| **Astra 7:** tests prove weaker properties than advertised | **Partly.** Intermediate delivery and vanished-file coverage improved; shutdown, attribution, watch and lifecycle gaps remain. | **Yes** for the remaining mutations below. | |
| +| **Astra 8:** harmful header opens and unsafe non-Unix fallback | **Partly.** Non-Unix refusal is fixed; Unix traversal remains unsafe. | No dedicated fallback regression test. Existing Unix tests do not establish safe ancestor traversal. | |
| +| **Astra 9:** platform UID and route-to-user binding | **No.** | No isolation test; existing acceptance fixtures do not establish this property. | |
| +| **Astra 10:** scheduler and temporary-root dependence | **No.** | These can fail with correct production code; there is no environmental regression matrix. | |
| +| **Astra claims** | **Mixed.** Four-trigger wording is correct. Older contradictions remain, and the new chain-shortening claim misses finding 1. | Detailed below. | |
| + |
| +**1. P1 — An inherited predecessor is mistaken for a closed predecessor, allowing overlapping stores and lost holds.** |
| + |
| +**Locations:** `internal/ops/litestream_metered.go:419`, `:422`, `:446`, `:457`, `:499`, `:502`. |
| + |
| +Both predecessor loops inspect `prev.inherited` **before** `prev.closed`. Inheritance proves that the predecessor’s predecessors closed; it does not prove that the predecessor itself closed. |
| + |
| +Two concrete failures follow: |
| + |
| +- A closes cleanly. B replaces A and opens, setting `B.inherited=true`. C replaces B. `C.Open` breaks at line 499 without checking B’s closure, inherits B’s current window, and opens beside B. This also works while B’s `Close` is blocked draining its store. |
| +- A closes cleanly; B replaces A and opens. At **23:59:40 on October 2**, ownership changes from `b` to `c`, after B’s last clean look at 23:59:30. C is constructed with owner `c` and `Replaces: B`, then closes unopened before B notices. C’s own witness remains current. Its inheritance loop sees `B.inherited=true`, takes B’s still-empty window, and clears `C.replaces`. An already-admitted B attempt can complete during `c`’s ownership. B subsequently detects the change and holds October 2–3, then closes. D replaces C and opens later. **C’s recorder and D’s recorders never receive B’s window.** |
| + |
| +This second sequence loses a doubtful day without a crash, long scheduling delay, forked chain or overlapping `Open`. |
| + |
| +**Smallest acceptable fix:** in **both** loops, require `prev.closed` before using `prev.inherited` to stop traversing its ancestors. Add a three-generation opened/draining predecessor test and the unopened-C sequence above, using distinct recorders and checking the exact inherited days. The existing blocked-Close fixture starts with a predecessor that has never inherited, so it misses this regression. |
| + |
| +**2. P1 — Validation 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`, `:1032`, `:1866`. |
| + |
| +Tenant A substitutes a link to V’s database after validation, while Litestream opens its SQLite connection and persistent descriptor, then restores A’s original pathname before the next inventory. Subsequent checks see A’s original inode; Litestream can retain V’s descriptor. Initialization also enables WAL and creates bookkeeping tables in the opened database. |
| + |
| +The exposure includes initialization retries and subsequent WAL opens. It is larger than “the first open” or one polling interval. |
| + |
| +**Smallest acceptable fix:** carry verified directory authority through the actual database, SQLite and sibling opens, constrain resolution, and verify the opened identities. Comparing an identity after an unrestricted open cannot prevent that open’s effects. |
| + |
| +**3. P1 — Platform-owned endpoints still permit tenant-replaceable ancestry.** |
| + |
| +**Locations:** `internal/ops/litestream_owners.go:395`, `:460`, `:630`, `:658`; `internal/ops/litestream_metered.go:856`. |
| + |
| +Let T own `/srv/T`, containing protected root-owned directory `bridge`. A platform-made link points to `/srv/T/bridge/A`, with A’s directory and database owned by T. The immediate-parent checks pass, but T can rename `bridge` through `/srv/T` and substitute the pathname. |
| + |
| +The same problem applies to a protected `dataDir` below a tenant-writable ancestor: checking `dataDir` and the directories beneath `.replicator` does not protect their access path. |
| + |
| +**Smallest acceptable fix:** establish and pin protected ancestry for database claims, authorized links and shadow state. An independently enforced immutable hierarchy could provide this precondition, but the constructor does not establish it. The open item extends beyond ancestors of `dataDir`. |
| + |
| +**4. P2 — Explicit exec claims can still disappear from every inventory category.** |
| + |
| +**Locations:** `internal/ops/litestream.go:385`, `:395`, `:420`, `:437`, `:500`. |
| + |
| +A non-hibernating exec route names `/srv/custom/archive.sqlite`. Its path enters `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 qualifications remain: |
| + |
| +- Empty files encountered within a tree are ignored. |
| +- Entries disappearing before their first `Lstat` are ignored. |
| +- Non-database-named links to regular files are ignored. |
| +- A missing relocated registry can evade final reporting because it lacks glob/unit/discovery provenance. |
| +- An absent console file is treated as optional without distinguishing a box with no console from a configured console whose database is missing. |
| + |
| +**Smallest acceptable fix:** enumerate explicit non-hibernating claims independently of the globs, preserve candidate provenance, and distinguish optional platform databases from required ones. Require an external-inventory comparison before cutover. |
| + |
| +**5. P2 — Unix header inspection can operate on a device before rejecting it.** |
| + |
| +**Locations:** `internal/ops/sqliteopen_unix.go:22`, `:34`; `internal/ops/litestream_owners.go:549`. |
| + |
| +The walk observes regular `T/d/watchdog`. T replaces `d` with a link to `/dev`. The subsequent root open follows the ancestor link and can open `/dev/watchdog` on a box with an available watchdog device. The later regular-file/identity check runs after the device’s open-side effects. |
| + |
| +`O_NOFOLLOW` protects only the final component. `O_NOCTTY` addresses controlling-terminal acquisition; `O_NONBLOCK` is not a general prohibition on device effects or filesystem stalls. The eighth build correctly removes the unchecked non-Unix fallback. |
| + |
| +**Smallest acceptable fix:** resolve from pinned directory descriptors with constrained traversal, preventing ancestor escape before opening the target. Keep unsupported platforms refusing inspection. |
| + |
| +**6. P2 — Material tests still survive removal of the behavior they claim to establish.** |
| + |
| +**Locations:** `internal/ops/litestream_metered_test.go:687`, `:891`, `:952`, `:975`, `:1297`, `:1336`, `:1395`, `:1538`; `internal/ops/litestream_owners_unix_test.go:275`. |
| + |
| +Concrete counterfactuals: |
| + |
| +- Removing `store.Close` still satisfies assertions based on the cleared store pointer and closed request gate. Retained database handles and worker termination are not asserted. |
| +- Removing the watch’s `lookAgain` invocation leaves the filesystem tests intact because they call `lookAgain` directly. |
| +- Swapping recorders in the opened-store test still satisfies positive counts on both recorders. “Counts to settle” checks server inactivity, not equality between received and recorded attempts. |
| +- Removing `Open`’s lifecycle lock is not challenged by a forced same-fleet Open/Close interleaving. |
| +- The predecessor tests do not use an **already-inherited, still-open** predecessor—the state that triggers finding 1. |
| +- The new intermediate test checks that B receives a day and is not notified again after detachment. It does not reject duplicate initial delivery. |
| +- Retry tests assert counts for recovered/exhausted errors, but not immediate stopping on a witnessed change or transient recovery through `Close`. |
| +- The header fixtures do not independently isolate no-follow from identity checking. Nor do they exercise an ancestor swap. |
| +- Removing chain detachment preserves the existing inheritance results. |
| + |
| +The synchronized admission-during-read test is useful: removing the post-read stale check makes it fail. The three new eighth-build fixtures also distinguish their respective reported defects. |
| + |
| +**Smallest acceptable fix:** test the production interleavings and their actual effects: retained handles, worker completion, per-database request correspondence, watch-driven file detection, inherited live predecessors, and exact initial delivery/retry counts. Redundant defensive checks need not each have an artificial mutation test. |
| + |
| +**7. P3 — Unit ownership remains inferred from the file, rather than bound to the route.** |
| + |
| +**Locations:** `internal/ops/litestream_owners.go:325`, `:359`, `:379`, `:436`, `:633`. |
| + |
| +A root-run tenant route can name `/var/lib/fail2ban/fail2ban.db` and qualify when the file and protected parent directories satisfy the checks. A tenant sharing the platform UID can also create links that qualify as platform-made. |
| + |
| +This is not literally acceptance “from anywhere”: directory modes, parent ownership, regular-file shape, link count, filesystem and sibling checks still apply. On a box whose units run as root, the climb stops at the immediate root-owned parent instead of climbing through all root-owned ancestors. |
| + |
| +Separately, route A can directly name B’s otherwise-valid database tree, and be billed for it, if no competing claim exposes the conflict. The expected UID/root is not independently obtained from A’s unit. |
| + |
| +**Smallest acceptable fix:** bind non-platform unit routes to an independently verified tenant UID and state root; refuse platform-UID tenants unless explicitly classified as trusted platform workloads. Deployment approval H remains separate from implementing these checks. |
| + |
| +**8. P3 — Tests still depend on temporary-root details, umask and scheduler timing.** |
| + |
| +**Locations:** `internal/ops/litestream_owners_unix_test.go:25`, `:45`, `:310`; `internal/ops/litestream_metered_test.go:784`, `:1369`, `:1476`. |
| + |
| +Examples: |
| + |
| +- Under umask `077`, the supposedly public `.replicator` directory requested as `0755` becomes `0700`; the test expects rejection of a fixture that is now valid. |
| +- `withOwners` overrides every matching basename, including components outside the fixture. It assumes UIDs `44001/44002` differ from the process UID. |
| +- A temporary path through a symlink still invokes platform-link ancestry rules. A foreign-owned `/` under a user namespace can therefore matter. |
| +- The new relative-target fixture compares `EvalSymlinks`’ resolved result with an unresolved `top`-derived spelling. A symlink in the temporary-root path can make that fixture assertion fail. |
| +- A descheduling interval exceeding 100 ms between `before` and entry to `lookAgain` fails the clean-start test. |
| +- Waiting 300 ms does not prove the second Close caller was scheduled. |
| +- Unreadable-directory coverage is skipped as root. |
| + |
| +**Smallest acceptable fix:** use explicit fixture modes, fixture-scoped ownership mappings and distinct synthetic UIDs; canonicalize fixture reference paths; replace timing assumptions with controlled clocks and synchronization barriers. |
| + |
| +The remaining requested checks have these results: |
| + |
| +| Area | Result | |
| +|---|---| |
| +| **A/J — tenant-file chain** | On a stable resolved path, the starting directory rejects a distinct non-platform owner at `litestream_owners.go:366`. The climb stops at a foreign parent, which must then pass `platformOwned`; tenant-made links are rejected by the resolver. Every accepting `tenantFile` path reaches the top check at `:384`. Its static-link fixture is rejected earlier by the resolver, so it does not isolate that defensive check. Findings 2, 3 and 7 prevent an unconditional isolation claim. | |
| +| **J — platform-made link and linked DBPath** | A platform link directly targeting a directory whose parent is tenant-owned is refused at `:658`. Replaceable higher ancestry remains finding 3. A final-component DBPath symlink is refused at `:322`; directly naming another tenant’s regular file remains finding 7. | |
| +| **B/I — admission** | The pre-lock, under-lock and post-witness stale checks close the reported interleavings. If the file check sets stale during the witness read, admission is refused at `litestream_metered.go:234`. Staleness can still occur immediately after that final load, and an already-granted attempt can subsequently reach the network. | |
| +| **B — retries** | `witnessUnchanged` makes at most three reads, sleeps only between errors, accepts recovery, and immediately returns a witnessed `false`. The configured allowance is 15 seconds of read deadlines plus 400 ms of delays—not an end-to-end bound on mutex waiting or filesystem calls. Request cancellation is not rechecked after acquiring `mu`. | |
| +| **C — generation** | The four triggers at `registry/registry.go:599` cover inserts, deletes and changed values of all six columns used by current discovery/attribution. Upserts and older-build raw DML still trigger them. The rowid trigger at `:609` blocks the `UPDATE OR REPLACE` deletion bypass. No-op watched updates and unrelated-column/table updates do not bump it. “Nothing else” nevertheless needs qualification: database-free route inserts/deletes and watched changes on irrelevant rows also invalidate. | |
| +| **D/I — Close and inheritance** | `closeOnce` correctly serializes shutdown and returns the cached result. Close skips its final witness read when already stale and uses the shared retry policy otherwise. `closed` is published after store shutdown. **The replacement loops bypass that correct publication through finding 1.** With all relevant predecessors actually closed, the intermediate-delivery fix works; repeated inheritance is prevented by the latch and detachment. | |
| +| **E — lineage** | `ownedReplicaKey` at `litestream_metered.go:833` uses account plus cleaned path for both remote lineage and shadow state. Different valid account IDs do not share keys, including after rename. Same-account/path reuse is deliberate, even across apps; A→B→A resumes A’s previous key. | |
| +| **F/K/L — walk and reports** | Case-variant names are recognized; uppercase `.DB` is skipped rather than replicated. Only nonempty regular files not already classified by database name receive header reads. `sidecarOf` is gone: a sidecar-shaped file is inspected independently of whether its base is a link. Unreadable directories, relative/no-path unit routes, missing unit claims, empty explicit candidates and discovered candidates failing final validation are reported. Finding 4 lists the remaining omissions. | |
| +| **G — same-filesystem bind mounts** | Device comparison does not detect them when only one alias is inventoried. The precondition is mount authority visible in the replicator’s namespace, not specifically “operator or systemd.” | |
| +| **G/M — crash and delayed admission** | These remain correctly disclosed limits. A completed old-owner attempt followed by a crash before invalidation leaves no persisted transition hold. Separately, an attempt admitted before a 23:58 finding and delayed until after midnight exceeds the one-minute margin and can leave the later day unheld. Closing these requires durable transition state and tracking granted attempts through completion. Finding 1 adds a hold-loss sequence requiring neither condition. | |
| + |
| +**Claims the code contradicts** |
| + |
| +| Claim and location | Assessment | |
| +|---|---| |
| +| A replacement cannot open beside a draining store — `internal/ops/litestream_metered.go:631`; dispositions `:140` | **False in this build.** An inherited live predecessor bypasses its closure check. | |
| +| Inheritance takes effect only after the predecessor closes — `internal/ops/litestream_metered.go:405`; dispositions `:162`, `:184` | **False in this build.** The shortcut proves only that predecessor’s ancestors closed. | |
| +| Every relevant intermediate recorder receives the window — dispositions `:183` | **True for the new fixture, false generally.** Premature inheritance can detach the intermediate before the window exists. | |
| +| Every replica request is refused “from then on” — `internal/ops/litestream_metered.go:33` | **Still false.** Already-granted attempts can dispatch later. The duplicate qualified comment does not remove this sentence. | |
| +| Every day an admitted request could fall in is held — `internal/ops/litestream_metered.go:39`, `:718` | **Too broad.** Finding 1, unbounded post-admission delay and the disclosed crash gap remain. | |
| +| Closing a previously current fleet holds nobody’s day — `internal/ops/litestream_metered.go:625` | **False without qualification.** Its final witness read can discover uncertainty, and it can inherit a hold. | |
| +| Registry invalidation starts from the last proof — `internal/ops/litestream_metered.go:276` | **Stale.** The implementation uses the earlier of that proof and the last clean filesystem look. | |
| +| Extra hold costs at most one look’s span — `internal/ops/litestream_metered.go:309` | **False.** Failed looks or a long build-to-Open delay can leave `lastClean` much older. | |
| +| Platform-link targets must be absolute — `internal/ops/litestream_owners.go:333`, `:607` | **Stale.** Relative targets are supported. | |
| +| The final ancestry comparison catches the ordinary tenant-link fixture — `internal/ops/litestream_owners.go:387` | **Misleading.** The resolver rejects that stable-path fixture first; the final check is defensive against intervening changes. | |
| +| Inventory reports the resolved pathname — `internal/ops/litestream_owners_unix_test.go:113` | **False.** The assertion and inventory retain the spelled path; the checked facts describe the resolved file. | |
| +| Every regular file is header-read — dispositions `:165` | **False literally.** Empty files and recognized database names bypass header inspection. Sidecar-name suppression is correctly removed. | |
| +| Strict discovery passes nothing over silently — `internal/ops/litestream_owners.go:486` | **Too broad.** Explicitly documented exclusions and finding 4 remain. | |
| +| A device substitution yields an error without harmful effects — `internal/ops/sqliteopen_unix.go:14` | **Too broad.** Rejection happens after the open. | |
| +| Ownership alone moves the generation — `internal/registry/registry.go:580` | **Overstated.** Irrelevant route insertions/deletions also move it; dispositions `:86` accurately qualify this. | |
| +| Identity comparison alone can prevent the first-open swap — dispositions `:84` | **Still present and false.** The later correction at `:171` correctly limits it to detection. | |
| +| Only an operator/systemd can create the undetected bind mount — dispositions `:95` | **Still present and too narrow.** The correction at `:171` is accurate. | |
| +| Umask can leave `t.TempDir` group-writable — `internal/ops/litestream_owners_test.go:16` | **False explanation.** Umask removes permissions. | |
| +| Missing owned files are clean; relative targets are cleaned safely | **Repaired.** The disappearance branch and unsafe relative join are gone, with distinguishing regression fixtures. | |
| +| Four triggers; non-Unix header reads refuse | **Correct.** | |
| +| Package tests pass under both temporary roots — dispositions `:191` | **Not independently verified by this source-only review.** Remaining environmental assumptions are identified above. | |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review8.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review8.md| new file mode 100644 |
| index 000000000..084a9de99 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review8.md |
| @@ -0,0 +1,152 @@ |
| +# Eighth review: metered replicator at `b5aecf13` (Opus) |
| + |
| +The brief's `git diff b5aecf13 b5aecf13` is empty, so I read the eighth build as `6b648c0c..b5aecf13` and the whole branch as `72b4f59a..b5aecf13`. I read everything with `git show`, and ran and changed nothing. Line numbers are at `b5aecf13`. The registry, request gate and store are unchanged since `6b648c0c`, so for B, C, E and F I rechecked the code and carry review seven's conclusions where nothing moved. |
| + |
| +**Verdict: reject.** The fix for Opus P3-2 (the walks stop at the first latched fleet) removed the check that a replacement cannot open beside a running predecessor. Every other finding from review seven that was marked fixed is fixed. |
| + |
| +## Review seven, finding by finding |
| + |
| +| Finding | Now | Would its test pass without the fix? | |
| +|---|---|---| |
| +| Opus P2-1 / Astra 3: a vanished file advanced `lastClean` | **Closed** (`litestream_metered.go:576-589`) | **No.** Without the fix, `TestAVanishedFileIsDoubtNotACleanLook` sees `stale == false`. | |
| +| Opus P3-1 / Astra 5: a relative link target was cleaned before it was walked | **Closed** (`litestream_owners.go:578-602`, `:646-654`) | **No.** With the old code, the fixture cleans to `/srv/safe/A`. The spelled `Lstat` at `:321` reads V's `main.db` (uid 0 in the seam), so the file is accepted. The test's second half blesses a `..` through a tenant's directory (see P3-2). | |
| +| Astra 4: intermediate fleets' recorders were never held | **Closed** (`:426-445`) | **No.** Without the loop, `bRec` holds nothing. The "once" check is pinned by either the CAS or `replaces = nil`, so removing just one of them passes. | |
| +| Opus P3-2: every fleet kept its whole chain alive | **Fixed, but the fix introduced P1-1** | **Yes.** No test asserts `replaces == nil` after a latch. Deleting `:438` and `:457` passes. | |
| +| Opus P2-2 / Astra 8: the header open can be steered to a device | **Open**, as stated. `!unix` now refuses (`sqliteopen_other.go:12-14`). | No test. | |
| +| Opus P3-3 / Astra 9: ownership is inferred from the file; a tenant may share the platform's uid | **Open**, as stated (approval H) | n/a | |
| +| Opus P3-4 / Astra 7 / Astra 10: tests | **Open in part.** The three new tests discriminate as shown above. Retained handles, worker exit, the watch's own `lookAgain` call, the `globbed[p]` branch, `O_NOCTTY`, and `Open`'s `life` lock are still untested. | as stated | |
| +| Astra 1 and 2 (P1): litestream's opens by path; ancestry | **Open**, as stated. G explains why Astra 2 is understated. | n/a | |
| +| Astra 6: exec claims outside the globs, empty files, no external inventory | **Open**, as stated | n/a | |
| +| Claims from both reviewers | **Mostly not corrected.** See "Claims the code contradicts". | | |
| + |
| +## Findings |
| + |
| +### P1-1. A replacement opens beside a running predecessor once that predecessor has latched, and loses its hold (A/D/I/M) |
| + |
| +- **Where:** `litestream_metered.go:498-505` (`Open`) and `:418-425` (`inheritHold`). Both loops `break` on `prev.inherited` before they check `prev.closed`. |
| +- **How a fleet latches:** a fleet latches at its own `Open` (`:512`) as soon as its predecessors are closed. In steady state, every running fleet after the second build is therefore latched. |
| +- **Scenario:** |
| + 1. A runs. B is built with `Replaces: A`. A closes. `B.Open` latches B (`B.inherited = true`, `B.replaces = nil`), and B streams. |
| + 2. Ownership of `x.db` moves from `a` to `b`. C is built with `Replaces: B` and a recorder for `b`. |
| + 3. `C.Open` is called before `B.Close` has finished: an ordering bug in the caller, or a concurrent close. |
| + - The loop at `:498` sees `B.inherited` and breaks. Nothing is refused. |
| + - C's `inheritHold` breaks at B the same way, latches, and reads `B.staleWindow()`. B is still current, so C latches nothing. |
| + - C opens a second litestream store over the same shadow directory and replica prefix as B. |
| + 4. B's `Close` then runs its last witness read (`:680-683`), finds the change, and holds B's recorders only. C has latched and let its chain go, so `b`'s recorder never holds that day. |
| +- **Same failure without opening:** `C.Close` on an unopened C while B is still open latches the same way, and drops the window B later finds. |
| +- **Data race in the same code:** `f.inherited` becomes true (`:446`) before the window is stored and before `f.replaces` is cleared (`:450-458`). The loops read `prev.replaces` without `staleMu` (`:418`, `:431`, `:498`), and those fields are written under it (`:438`, `:457`). A successor running between those lines reads `latched && !inheritedStale` and drops the window. |
| +- **Not caught by tests:** `TestOpenRefusesWhileAnyPredecessorIsOpen` and `TestMeteredFleetReplacementInheritsTheHold` only use unlatched predecessors. A root fleet never latches (`:411`). |
| +- **Smallest fix:** |
| + - In both loops, test `!prev.closed.Load()` first, then `break` on `inherited`. |
| + - Add a test: A → B, close A, open B, build C. `C.Open` must refuse, and `C.Close` on an unopened C must leave B's later finding reaching D. |
| + |
| +### P3-1. Units running as root, or a tenant on the platform's uid: files are accepted from any protected root directory (A/J; carried as Opus P3-3) |
| + |
| +- **Where:** `litestream_owners.go:359-368` and `:379`. |
| +- **What happens:** |
| + - The climb stops at once when `pf.UID == 0`. |
| + - A root-owned file is accepted if its directory and that directory's parent are root-owned and 0755 or stricter, which describes most of `/var/lib`. |
| + - Root-owned links count as the platform's (`:633`). |
| +- **What changed:** 01f2b072 introduced this. Before it, the climb reached `/` and refused (`:372`). So for root-run units this is a move from refusing to accepting, made for tests and single-user boxes. |
| +- **Scenario:** a unit route with `DBPath=/var/lib/fail2ban/fail2ban.db`, or a root unit that writes anywhere, is accepted and billed to that route. |
| +- **Fix (approval H):** refuse a `tenantFile` whose owner uid is 0 or `euid` unless the route is the ops account. Tests should use the `factsOf` seam rather than the real uid. |
| + |
| +### P3-2. `..` after a tenant-owned component of a platform link's target is still accepted (J) |
| + |
| +- **Where:** `litestream_owners.go:587-589`. The test at `litestream_owners_unix_test.go:320-327` asserts that this is accepted. |
| +- **Scenario:** |
| + 1. A root link `/pub/A -> ../srv/T/sub/../../safe/A` passes while `sub` is T's real directory. |
| + 2. T owns `/srv/T`, so T can later rename `sub` and put a link in its place. |
| + 3. Every later kernel resolution of `/pub/A/main.db` then goes wherever T points. |
| +- **Limit:** this is the general swap window again (`lookAgain` catches a swap that stays in place). It needs a non-canonical link made by root. |
| +- **Fix:** |
| + - Refuse a target with `..` anywhere but its leading run, or after any component that is not the platform's. |
| + - Flip the test's second half to expect a refusal. |
| + |
| +### P3-3. Any tenant's file deletion now holds every account's days on the box (G) |
| + |
| +- **Where:** `litestream_metered.go:587` calls `holdDays(f.recorders…)` over all recorders. |
| +- **Scenario:** a tenant deletes its own `.db` (an app reset). Every recorder on the box holds the span from the last clean look. Repeated every 30 s, this keeps every account's days in doubt. |
| +- **Context:** a rename (new inode) could already do this, so the vector is not new. A deletion is ordinary, though. |
| +- **Fix:** state this in the dispositions as a known availability cost, or scope a file finding to the changed path's owner plus the inheritors. |
| + |
| +### P3-4. Tests (H) |
| + |
| +- Nothing pins `replaces = nil` or the `inherited` break (P1-1). |
| +- The "once" check in `TestIntermediateFleetsRecordersAreHeldToo` passes with the intermediate CAS removed. |
| +- The relative-target test still depends on the temp root resolving under `resolvePlatformLinks` (any link in `$TMPDIR` must be root's). |
| +- `withOwners` still keys owners by base name (`A`, `T`, `V`, `x`, `y`, `safe`) over every component, temp-root names included. |
| +- The new tests add no wall-clock dependence. `TestTheLastCleanLookIsItsStart` and the 300 ms second-`Close` wait remain. |
| + |
| +## A–M |
| + |
| +- **A. Chain climb.** |
| + - A tenant with its own uid cannot get a link or another user's directory into its chain: |
| + - links are refused by `resolvePlatformLinks` and the walk (`:417`); |
| + - another user's parent ends the climb and then fails `platformOwned` (`:395`); |
| + - a platform-owned start directory needs a file matching the top's uid (`:436`). |
| + - A platform-uid tenant, and root-run units: P3-1. |
| + - The top-of-chain checks (`:384`, `:392`, `:395`) are reached on every accepting path. `:384` and `:392` are still unreachable without a race. |
| +- **B. Request gate and admission.** Unchanged. |
| + - The stale check before the lock, under it (`:220`) and after the read (`:234`) is complete up to the return. |
| + - A request granted at `:234` can still go out after a finding; the margin covers that. |
| + - Retries are bounded (3 × 5 s + 2 × 200 ms). Only an exhausted error, or a witnessed `false`, counts as a change. |
| + - The request context is not re-checked after `f.mu`. |
| +- **C. Ownership generation.** Unchanged and correct. |
| + - Insert, delete, changed-value update on six columns, and an immutable rowid. |
| + - No migration rebuilds `routes`, so the triggers cannot be dropped silently. |
| + - A route change unrelated to any database also moves it: over-invalidation, which is safe. |
| +- **D. Handover between fleets.** |
| + - In an ordered chain (close, then open), no window is lost and no reading is taken on a stale fleet. |
| + - Each fleet's inherited window is applied once (CAS). |
| + - Broken by P1-1 whenever a successor's `Open` or `Close` runs before its latched predecessor has closed. |
| + - Recorders of intermediate fleets are now called after their own `Close` has returned. The doc should say recorders must outlive the chain's latch. |
| +- **E. Lineage and shadow state.** Unchanged. |
| + - Keyed by account plus path (`:833-835`), so different accounts never share or overwrite. |
| + - A path renamed to another owner starts a new key. |
| + - A→B→A resumes A's lineage. Worth stating. |
| +- **F. Strict walk.** Unchanged. |
| + - Names in other cases are skipped, not dropped. |
| + - Every non-empty unrecognised regular file is header-read. Recognised names and empty files are not. |
| + - An unreadable directory is a problem. |
| + - A unit route with no `DBPath` is reported. An empty claimed file is skipped. |
| +- **G. Open items.** |
| + - The crash gap, bind mounts and the external inventory are stated correctly. |
| + - The litestream-open swap is larger than stated: every reopen and WAL open, not just the first. |
| + - Ancestry is larger than "dataDir and shadow": `platformOwned(Dir(top))` and `platformOwned(Dir(target))` check one level only (Astra 2's `/srv/T/bridge`). |
| + - P1-1 adds a new two-stores case that the dispositions do not list. |
| +- **I. Admission and `closeOnce`.** |
| + - No admission gets through after the file finding except at the `:234` instant. |
| + - A replacement **can** open beside a running or draining store (P1-1). |
| +- **J. Chain start and the platform-made link.** |
| + - The platform link's target is now walked as spelled. |
| + - A `..` after a tenant-owned component is accepted (P3-2). |
| + - A platform-uid tenant: P3-1. |
| + - A `DBPath` that is itself a link is refused at `:321-323`, and `SameFile` at `:133` fails closed. |
| + - A `DBPath` naming another tenant's file through an operator link is accepted (carried). |
| +- **K. `openRegularNoFollow`.** |
| + - A swap between the `Lstat` and the open is caught by the inode check, but only after the open has done its work (the device primitive, open). |
| + - `sidecarOf` is gone, and a sidecar is checked independently. |
| + - `!unix` now refuses the read, and `factsOf` returns false there, so nothing is owned. That is consistent. |
| +- **L. Inventory reports.** |
| + - Silent still: hibernating routes (by design); exec claims outside the globs (stated as the next item); empty files; entries that vanish before their first `Lstat` in the strict walk. |
| + - An owned file that vanishes is no longer silent. |
| +- **M. A day in doubt left unheld.** |
| + - P1-1 is a concrete sequence. A is closed and B is latched and open. C is opened or closed before `B.Close`. B's last read finds the move to `b`. C's recorder for `b` never holds that day. |
| + - Otherwise: the crash gap, and requests held longer than `ownershipMargin`, as stated. |
| + |
| +## Claims the code contradicts |
| + |
| +- **`litestream_metered.go:419-420`, `:499-500`, and the dispositions' Opus P3-2 row:** "its own predecessors were closed when it latched" is true, but the loops then skip the latched fleet's own `closed` check (P1-1). |
| +- **`:556-559`, the `lookAgain` doc:** "one that has gone … is ErrInventoryChanged". It is `ErrOwnershipStale` now. |
| +- **`:28-35`:** a duplicated doc comment that still says "Every replica request is refused from then on" (fifth round). |
| +- **`:625-627`, the `Close` doc:** "holds nobody's day". Its last witness read can go stale, and it can inherit a hold. This doc is duplicated too. |
| +- **`:276`:** "For a change in the registry that instant is the last proof". It is the earlier of that proof and `lastClean`. |
| +- **`:309`:** "at most one look's span of extra hold". The hold reaches further after an errored look, or when `Open` comes long after the build. |
| +- **`litestream_owners.go:333-334` and `:607`:** "its target is absolute". Relative targets are accepted. |
| +- **`:387-391`:** "a tenant's link above the chain is caught here". The resolver catches it first. |
| +- **`:491-492`:** the `strictTreeDBs` doc says only files with no extension are header-read. Every unrecognised non-empty file is. |
| +- **`:644-645`:** "`..` steps up only through directories already looked at" is true only at that instant. The test then accepts `T/..` (P3-2). |
| +- **Dispositions:** "Package tests pass under `/tmp` and under `~/.cache`" is not established by this review, which ran nothing. |
| + |
| +**Verdict: reject.** Fix P1-1 by moving the `closed` check ahead of the `inherited` break in both loops, add its test, and correct the claims. With that, this would be accept with fixes. P3-1, P3-2, the walk by directory descriptor, Astra 1 and Astra 2 still block wiring and attestation. |