amadan
The replicator's seventh build: the sixth review's bounded findings
A relative state link, as systemd makes it, is taken from the link's own directory; the link's target is walked, not cleaned, so a traversal through a link the tenant controls is refused. Every finding, by the registry or the files, holds from the earlier of the last proof and the last clean look, and the last clean look is its start. A fleet latches what it inherits only when every fleet in the chain has closed. The header open sets O_NOCTTY. Every regular file is read for the header, sidecar names included, since a real sidecar never carries it. A claimed glob match that vanished before its final look is said. The tests make a look slow to tell its start from its end, drive a registry finding with the last clean look before midnight, put a real directory between a tenant's link and the tree it reaches, and pass a hold through two unopened fleets. What stays open is in the dispositions, review six.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
10 files changed,
+614
−63
docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review6.md+201 −0docs/superpowers/specs/DISPOSITIONS-metered-replicator.md+26 −3docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review6.md+200 −0internal/ops/litestream.go+10 −7internal/ops/litestream_metered.go+43 −10internal/ops/litestream_metered_test.go+73 −0internal/ops/litestream_owners.go+37 −30internal/ops/litestream_owners_test.go+11 −6internal/ops/litestream_owners_unix_test.go+8 −6internal/ops/sqliteopen_unix.go+5 −1
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review6.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review6.md| new file mode 100644 |
| index 000000000..f9d91f6ca |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review6.md |
| @@ -0,0 +1,201 @@ |
| +Reviewed `72b4f59a..d63f81df`. The second requested diff, `d63f81df..d63f81df`, is empty. All locations below refer to `d63f81df`. Nothing was modified or executed beyond read-only inspection; test counterfactuals are source-based, not executed mutation checks. |
| + |
| +| Review-five finding | Closed? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P2-1:** wrong filesystem hold boundary | **Partly.** `lastClean` fixes the reported boundary, but registry-first invalidation can prevent filesystem uncertainty from being held at all; finding 2. | **No** for the new `TestAFileSwapIsHeldFromTheLastCleanLook`. **Yes** for the retained older `TestAFileSwapIsHeldFromBeforeTheLookBegan`. | |
| +| **Opus P3-1:** unopened replacements lose windows or allow overlap | **Partly.** Open checks every predecessor. Window propagation still fails through two unopened intermediates; finding 3. | **No** for reverting the predecessor traversal or the tested inheritance fixes. The longer inheritance sequence is untested. | |
| +| **Opus P3-2:** Open races Close | **Closed in code.** Both hold `life` across the store operation. | **Yes.** The added tests do not force Open past its admission and then race its store opening against Close. | |
| +| **Opus P3-3:** links above the inferred chain | **Closed for the reported canonical-path reproduction.** The new resolver introduces a different acceptance bypass; finding 5. Mutable ancestry also remains. | **No** for reverting the resolver entirely. Removing only the final `EvalSymlinks(root)` check can pass: the earlier resolver already rejects the tested link. | |
| +| **Opus P3-4:** writable platform start directory; same-UID tenant | **Partly.** Writable platform start directories are refused. Same-UID tenants remain accepted, with the limitation documented. | **No** for removing the permission check. No same-UID isolation test exists. | |
| +| **Opus P3-5:** header opening through substituted ancestors | **Open.** Finding 8. | No ancestor/device regression test. The existing FIFO/link tests do not establish this property. | |
| +| **Opus P3-6:** sidecar-shaped databases and final disappearance | **Partly.** The specific text-base case and vanished unit/discovered files are fixed. Other silent omissions remain; findings 6–7. | **No** for reverting the text-base fix. **Yes** for removing the new final-`ENOENT` reporting branch; no deterministic test exercises it. | |
| +| **Opus P3-7:** ineffective tests | **Partly.** The new boundary, inheritance and retry fixtures help. Significant gaps remain; finding 9. | Several material mutations still pass, listed below. | |
| +| **Astra 1:** retained substituted database | **Open.** Finding 1. | No regression test. | |
| +| **Astra 2:** affected dates escape holds | **Partly.** The saved clean boundary and interval union exist. Their production invocation remains incomplete; finding 2. Delayed-attempt and crash gaps remain disclosed. | **No** for removing the newly tested boundary/union operations. **Yes** for retaining the production short-circuit that prevents widening. | |
| +| **Astra 3:** local staleness hides inherited uncertainty | **Closed for that exact sequence.** `staleWindow` unions both intervals. Broader inheritance remains incomplete; finding 3. | **No.** `TestInheritedWindowIsTheUnion` distinguishes the old behavior. | |
| +| **Astra 4:** untrusted ancestry and intermediate link resolution | **Partly.** The reported intermediate-link target is refused. Directory ancestry remains unchecked, and target normalization creates another bypass; findings 4–5. | **No** for reverting the tested intermediate-link refusal. Existing tests pass with mutable ancestry unchecked. | |
| +| **Astra 5:** replacement overlap and concurrent Open | **Closed for the reported lifecycle sequences.** | **No** for reverting all-predecessor checking. **Yes** for removing `life` while retaining `closeOnce`; the Open/Close interleaving is untested. | |
| +| **Astra 6:** nested files beneath DynamicUser links refused | **Closed for canonical platform-made links.** | **No.** The nested fixture at `litestream_owners_unix_test.go:134` fails with the old immediate-directory-only handling. | |
| +| **Astra 7:** final `ENOENT` silently drops discovered/unit files | **Closed for those candidates.** The exec-candidate variant remains; finding 7. | **Yes.** No deterministic final-inspection race test exists. | |
| +| **Astra 8:** sidecar suffix suppresses a database | **Partly.** A text `archive` no longer hides `archive-journal`; a database-like base still does. | **No** for reverting the text-base fix. The test still explicitly requires a database-header-bearing `live.db-wal` to disappear. | |
| +| **Astra 9:** tests overclaim their evidence | **Partly improved, still open.** Finding 9. | **Yes** for several advertised properties. | |
| +| **Opus claims** | **Mixed.** The successful-look boundary now exists; retry coverage now exists. Request-refusal, shutdown, resolved-path, discovery and trigger-count wording remains wrong or too broad. | Prose has no behavioral test; detailed statuses follow the findings. | |
| +| **Astra claims** | **Mixed.** First-open, crash and same-account lineage qualifications remain corrected. Hold completeness, transitive inheritance, discovery completeness and several older assertions remain overstated. | Detailed statuses follow the findings. | |
| + |
| +The requested checks that otherwise hold: |
| + |
| +- **A/J:** On a canonical resolved path, a distinct tenant UID cannot pass the ordinary inferred chain through another non-platform UID’s directory or a tenant-made link. The final-component `DBPath` link is refused at `internal/ops/litestream_owners.go:321`. Every accepting `tenantFile` path reaches the top check at `:384`. This is not proof that the file belongs to the route’s tenant: the UID is inferred from the file, and route-to-root/UID binding remains absent. |
| +- **Same-UID/root units:** They do not receive tenant isolation. On a root-running box with root-owned unit files, the climb normally stops at the immediate root-owned parent. Start-directory permissions, the parent check, file shape and sibling checks still apply, so acceptance is not literally “from anywhere.” An untrusted tenant must not share the platform UID. |
| +- **B/I:** The checks before locking, under the lock and after the witness read close the reported admission interleavings. The final stale load at `litestream_metered.go:228` is a possible grant point; staleness can occur immediately afterwards, before return or dispatch. An already-admitted request can still reach the network after ownership changes. |
| +- **Retries:** `witnessUnchanged` makes three attempts, sleeps only between errors, returns immediately on a witnessed change, and accepts a recovered error. Its configured read-and-delay allowance is 15.4 seconds, **not an end-to-end bound**: mutex waiting and non-context-bound filesystem operations are outside it. The request context is not checked again after waiting for the mutex. |
| +- **C:** `internal/registry/registry.go:569` covers the six fields current discovery and attribution use. The insert/delete/update triggers cover raw SQL from older builds; changed-value updates exclude no-ops. Host replacement moves the generation, and `:609` rejects the rowid replacement bypass. “Nothing else” requires qualification: database-free inserts/deletes also move it. |
| +- **D/I:** `closeOnce`, `life`, store shutdown and the complete predecessor traversal prevent the reported draining-store overlaps. Close skips its final witness read when already stale. Inheritance is applied at most once per fleet, but that does not establish that the latched interval is final; finding 3. |
| +- **E:** Account plus cleaned path keys both remote lineage and shadow state at `litestream_metered.go:744` and `:768`. Different valid accounts do not share the key, including after a rename. Same-account/path reuse is deliberate. |
| +- **F/L:** Uppercase database extensions are recognized; uppercase `.DB` is reported as skipped rather than replicated. Unreadable directories, relative unit paths and missing unit paths are reported. Empty explicit unit files are skipped. Empty tree files and specified churn remain excluded. |
| + |
| +1. **P1 — Litestream still opens and retains files outside the verified ownership boundary.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:268`; `internal/lease/meteredstore.go:127`; `third_party/litestream/db.go:1004`, `:1014`, `:1866`. |
| + |
| + Tenant A substitutes a link to V’s database after inventory validation but before Litestream initializes its SQLite connection and persistent descriptor. A restores the original pathname before the next inventory. Subsequent checks see the expected inode while Litestream retains V’s opened file. WAL opens also continue by pathname. |
| + |
| + This is larger than a polling interval and is not confined to the first initialization: later initialization attempts and sibling opens also resolve paths independently. |
| + |
| + **Smallest acceptable fix:** preserve verified authority through the actual SQLite, database and sibling opens using pinned directories and constrained resolution, and verify the identities actually opened. Detecting the wrong descriptor afterwards cannot prevent the initial unauthorized operation. |
| + |
| +2. **P1 — Registry-first invalidation prevents the filesystem hold from being widened.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:220`, `:292`, `:501`, `:522`, `:615`. |
| + |
| + A concrete sequence: |
| + |
| + - A long inventory checks a database shortly before midnight, then continues examining other files. |
| + - The database is substituted after that check, and replication records substituted data on October 2. |
| + - The inventory finishes after 00:02 on October 3, still carrying the original observation for that database. |
| + - Requests have kept the registry proof current. An ownership change is noticed next, so `goStale` holds October 3 only. |
| + - Every subsequent `lookAgain` returns immediately because `stale` is already true. Close also skips the witness and performs no final filesystem check. |
| + |
| + `goStaleFrom` can now widen an interval, but no filesystem finding reaches it in this ordering. October 2 remains unheld on the old fleet and its replacements. |
| + |
| + There is another unsafe boundary advance at `:515`: a missing owned pathname is skipped, after which `:527` records the scan as clean. Retained descriptors or pending shadow data can survive that disappearance; a later reappearance can therefore be held from an unjustifiably recent boundary. |
| + |
| + **Smallest acceptable fix:** continue collecting filesystem uncertainty after registry invalidation, through shutdown, or conservatively include the filesystem boundary when stopping. Do not advance `lastClean` when an owned file was absent or otherwise unverified. Test registry-first invalidation through `lookAgain`, rather than calling the widening helper directly. |
| + |
| +3. **P1 — Two unopened intermediates can still discard a predecessor’s later hold.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:355`, `:393`, `:622`. |
| + |
| + The sixth build checks only the immediate predecessor before latching inheritance: |
| + |
| + 1. A is running and current. |
| + 2. B replaces A and closes unopened. B correctly remains unlatched because A is open. |
| + 3. C replaces B and closes unopened. B is closed, so C latches. B currently delegates to A, which has no hold; C permanently records an empty inheritance. |
| + 4. A subsequently becomes stale on October 2 and closes. |
| + 5. D replaces C and opens after all predecessors have closed. |
| + |
| + D passes the full lifecycle check, but C no longer delegates through B to A. **October 2 is unheld on D’s recorders.** No concurrent calls or crash are needed. |
| + |
| + **Smallest acceptable fix:** latch inheritance only after the entire predecessor dependency chain is final, not merely after the nearest fleet is closed. Preserve delegation until then. Extend the test to two unopened intermediates closed before A’s finding. |
| + |
| +4. **P1 — Platform-owned endpoints still permit tenant-replaceable ancestry.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:395`, `:460`, `:645`; `internal/ops/litestream_metered.go:791`. |
| + |
| + Let `/srv/T` belong to tenant T, containing a root-owned `bridge` directory. A platform-made link in a protected public directory points to `/srv/T/bridge/A`, where A’s directory and database belong to T. |
| + |
| + The target has no links; its immediate parent is root-owned and protected. The chain and platform-link checks pass. But T can rename `bridge` through `/srv/T` and replace that pathname before Litestream opens it. |
| + |
| + The same structure works for a platform-owned `dataDir` below a tenant-writable ancestor. Checking `.replicator` and its descendants does not protect their pathname from replacement above them. |
| + |
| + **Smallest acceptable fix:** verify and pin the complete trusted ancestry for claims, authorized links and shadow state. Until that exists, require an independently enforced immutable hierarchy. The unresolved scope is broader than `dataDir` alone. |
| + |
| +5. **P2 — Cleaning a platform link’s target can hide the tenant-controlled link it actually traverses.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:637`, `:343`, `:398`; the original spelling remains in `OwnedDB.Path` at `:190`. |
| + |
| + Consider a root-owned link `/public/A` with this absolute target: |
| + |
| + ```text |
| + /srv/T/choice/../../safe/A |
| + ``` |
| + |
| + T owns `/srv/T` and makes `choice` point to `/victims/V/x/y`. |
| + |
| + Actual filesystem resolution reaches: |
| + |
| + ```text |
| + /victims/V/safe/A |
| + ``` |
| + |
| + But `filepath.Clean` first changes the target to: |
| + |
| + ```text |
| + /srv/safe/A |
| + ``` |
| + |
| + If `/srv/safe/A` contains a qualifying root-owned database, the helper validates that different directory and file. The platform-owned start-directory exception permits this despite the initial file observation having V’s UID. Inventory still hands Litestream `/public/A/main.db`, which opens V’s database. |
| + |
| + Thus the claimed “target contains no further link” property is false even without a concurrent swap. |
| + |
| + **Smallest acceptable fix:** reject noncanonical link targets before cleaning them, or resolve their components with filesystem semantics under pinned authority. Also ensure the identity approved is the identity associated with the spelling passed downstream. |
| + |
| +6. **P2 — A sidecar suffix still hides a complete SQLite database.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:543`, `:590`; `internal/ops/litestream_owners_test.go:511`. |
| + |
| + Create a text file `notes.db` and a SQLite database `notes.db-journal`. The base’s extension is sufficient for `sidecarOf` to suppress all inspection of the second file. A real database beside another real database has the same problem. |
| + |
| + The sixth build fixes `archive` plus `archive-journal`, but does not implement Astra’s requested rule that a SQLite database header must win over the sidecar-name heuristic. The test still writes a database header into `live.db-wal` and calls it a “real sidecar.” |
| + |
| + **Smallest acceptable fix:** inspect a potential sidecar for a SQLite database header before excluding it. Report a database header regardless of its neighbor. Use actual sidecar contents in the exclusion fixture. Uppercase suffixes now fall through to header inspection; a symlink base is also correctly refused as a base. |
| + |
| +7. **P2 — A claimed exec database found by the glob can still disappear silently at final validation.** |
| + |
| + **Locations:** `internal/ops/litestream.go:425`, `:494`, `:498`. |
| + |
| + An exec route claims `/data/a.db`. The glob finds it, but it is renamed before the final `Stat`. `claimed[p]` is true, while both `discovered[p]` and `unitPaths[p]` are false. Consequently the new `ENOENT` branch does not report it. |
| + |
| + The file can return after construction, leaving the fleet without that database and without a skipped/problem entry. |
| + |
| + **Smallest acceptable fix:** retain candidate provenance for globbed claimed files as well as unit-walk files, and report their disappearance. Preserve the intentional absent-console exception explicitly. This is separate from the already-disclosed exclusion of exec paths outside the globs. |
| + |
| +8. **P3 — Header inspection still performs unsafe opens before determining what it opened.** |
| + |
| + **Locations:** `internal/ops/sqliteopen_unix.go:18`, `:30`; `internal/ops/sqliteopen_other.go:9`; `internal/ops/litestream_owners.go:507`. |
| + |
| + After the walk observes regular `T/d/watchdog`, T replaces `d` with a link to `/dev`. The subsequent open can operate on `/dev/watchdog` before `fstat` rejects it. A direct final-component substitution with a device has the same open-before-rejection problem. `O_NONBLOCK` is not a universal protection against device effects or filesystem stalls; `O_NOCTTY` is absent. |
| + |
| + On non-Unix builds the helper is simply `os.Open`, with no type or identity verification. Later ownership refusal does not prevent earlier inventory reads through this helper. |
| + |
| + **Smallest acceptable fix:** constrain directory traversal through pinned descriptors, add `O_NOCTTY`, and enforce a policy that prevents opening special devices before post-open validation. Unsupported platforms should refuse inspection before an unchecked open. |
| + |
| +9. **P2 — Several regression tests still establish weaker properties than their comments claim.** |
| + |
| + **Locations:** `internal/ops/litestream_metered_test.go:687`, `:889`, `:952`, `:973`, `:1088`, `:1251`, `:1334`, `:1393`; `internal/ops/litestream_owners_unix_test.go:273`. |
| + |
| + Concrete gaps remain: |
| + |
| + - Removing `store.Close` still satisfies tests that observe a cleared store pointer and a closed request gate. They do not inspect retained handles or worker termination. |
| + - Removing the watch’s filesystem invocation still passes tests that call `lookAgain` themselves. |
| + - Misassigning some real store-driven requests can pass the positive-count assertions. The “counts to settle” assertion only checks server inactivity. |
| + - The older file-boundary test still passes with `lookStart` ignored. |
| + - The new inheritance test does not cover finding 3 or assert exactly-once delivery of inherited days. |
| + - Removing `life` is not caught by a forced Open/Close interleaving. |
| + - The blocked-Close test improves coverage, but `life` also makes the second caller wait. Removing `closeOnce` alone would eventually trip the hook’s double `close(entered)`, rather than independently demonstrating cached shutdown results. |
| + - Retry tests do not assert call counts for witnessed-change cases, nor exercise recovered errors specifically through Close. |
| + - Removing only `O_NOFOLLOW` still leaves the tested different-inode symlink rejected by the identity check. Conversely, removing only the device/inode comparison leaves the FIFO/link cases rejected by other checks. |
| + |
| + **Smallest acceptable fix:** add synchronized cases for the missing production paths and assert their actual effects: closed retained handles, stopped workers, request attribution by database, exact retry counts, and predecessor-window propagation. |
| + |
| + The old distant-temp-root climb is fixed for the normal fixtures. Remaining environmental assumptions include synthetic UIDs `44001/44002` potentially equalling the process UID, root-skipped unreadable-directory tests, and ancestor symlinks rejected by the new resolver. The new boundary test uses relative wall time with a wide separation, so ordinary midnight crossing does not erase its distinction. Integration sleeps, the 300 ms blocking observation, 500 ms quiet period and 15-second shutdown threshold remain scheduler-dependent. |
| + |
| +The remaining disclosed limitations are correctly open, with these qualifications: |
| + |
| +- **Crash gap:** correctly described at `litestream_metered.go:655`: settled rows can survive without a transition hold, and replacement recorders receive no persisted uncertainty. |
| +- **Delayed admissions:** a request paused beyond the one-minute margin remains untracked. The second stale check does not close that interval. |
| +- **Same-filesystem bind mounts:** device-number comparison cannot detect them. The relevant prerequisite is mount authority visible in the replicator’s namespace, not specifically being “the operator or systemd.” |
| +- **External inventory and lineage:** no comparison proves that cutover preserves the external replicator’s complete database set, and no guard proves that the supplied remote prefix is separate from its lineage. These are correctly listed as unfinished. No deployment or approval-H work was evaluated. |
| + |
| +| Claim from review five or the sixth-build dispositions | Status at `d63f81df` | |
| +|---|---| |
| +| Every replica request is refused “from then on” — `litestream_metered.go:33` | **Still false.** Already-granted attempts remain able to dispatch. The qualified paragraph below it does not replace the old claim. | |
| +| The filesystem hold uses the last successful look | **Implemented at `:522`, but incomplete overall.** Finding 2 prevents that boundary from being consulted in another valid ordering. | |
| +| A later finding widens the held window — dispositions `:138` | **True of the helper, overstated as production behavior.** A later filesystem scan is skipped once stale. | |
| +| Unopened replacements preserve inherited windows — dispositions `:139` | **Still false without qualification.** Finding 3 supplies a sequential counterexample. | |
| +| A replacement cannot open beside its draining predecessor — `litestream_metered.go:566` | **Now supported for the declared replacement chain.** Lifecycle locking and all-predecessor checks close the reported paths. | |
| +| Closing a current fleet holds nobody’s day — `litestream_metered.go:560` | **Still too broad.** The last witness reading can discover a change or exhaust its retries. | |
| +| A missing database pathname means nothing is being shipped — `litestream_metered.go:516` | **Still false.** Open descriptors and pending shadow files can outlive it. | |
| +| The entire tenant-file chain belongs to the tenant — `litestream_owners.go:237` | **Qualified by the implementation.** Platform-owned starting directories are allowed; ownership is inferred from the file rather than independently bound to the route. | |
| +| The platform link target contains no further link — `litestream_owners.go:633` and dispositions `:141` | **False for noncanonical targets.** Cleaning before resolution permits finding 5. | |
| +| Inventory reports the resolved path — `litestream_owners_unix_test.go:112` | **Still false.** The assertion at `:122` and `OwnedDB.Path` use the spelled path. Only the check operates on the substituted resolved spelling. | |
| +| Strict discovery passes nothing over / inspects every other regular file — `litestream_owners.go:486`, dispositions `:87` | **Still too broad.** Findings 6–7 remain, alongside documented empty/churn exclusions. | |
| +| Retry tests do not exist | **Corrected.** `TestWitnessReadsAreRetriedBeforeTheyCount` now exists; Close-specific and immediate-stop assertions remain missing. | |
| +| The old close-once test proves waiting for shutdown | **Still false of that test.** The newer blocked-shutdown test adds useful evidence, with the limitations in finding 9. | |
| +| First-open exposure, the settled-row crash gap, and same-account/path lineage reuse are disclosed | **Corrected and retained.** The older disposition at `:84` still incorrectly presents exposing opened identity alone as an alternative preventive fix. | |
| +| Only ownership moves the registry generation — `registry.go:580` | **Semantically overstated.** Database-free route insertion/deletion also invalidates; dispositions `:86` correctly acknowledge this. | |
| +| The migration adds three triggers — dispositions `:75` | **Still wrong.** It adds four. | |
| +| Only an operator or systemd can make an undetected bind mount — dispositions `:95` | **Still overstated.** Namespace-visible mount authority is the condition. | |
| +| Fixtures require umask `022` — dispositions `:148`, following Opus’s umask claim | **Incorrect explanation.** `0755 & ~0002` remains `0755`; umask cannot add group-write permission. `platformDir` also explicitly chmods its result. | |
| +| Package tests pass under both temporary roots — dispositions `:150` | **Not independently established by this source-only review.** The prior climb defect is removed, but that does not prove every supported layout or environment. | |
| + |
| +**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 60953d96b..e52a96829 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| @@ -72,7 +72,7 @@ Found on the way: `registry.ObserveRoutes` panicked on any failure after its con |
| Mutation checks in a scratch copy, twenty-five. Twenty-two failed their tests: the gate letting a refused attempt through; the gate inside the meter; the generation moving on any write of an owner column; hibernation dropped from the columns; the witness ignoring a replaced file; the fleet's clients given a gate that always admits; the fleet asking the whole-database witness; a stale fleet telling one recorder; the watch not asking; siblings unchecked; a second name unchecked; the directory above a tenant unchecked; shadow directories unverified; shadow directories left to litestream; the second look not comparing the file; a replacement not inheriting the hold; discovery not told where the registry is; a replacement opening beside the fleet it replaces; the hold not reaching back before the last proof; the walk ignoring every link; a stale close retrying; the walk passing over a hidden database. Three survived, and are stated as untested: a link at the shadow root (it is refused by the mode check instead, so the explicit check is redundant on Linux); the owner check beneath a tenant's tree and the filesystem check (a test needs root, or a second filesystem). |
| -Not built, and still needed before this could replace the external binary: the replica credentials out of everything else's reach; a restore path; the prefix-lineage guard; the shrink guard; opening databases beneath a pinned root; a caller that rebuilds the fleet; the registry migration reviewed as a schema change on every box (it adds one table and three triggers, and an older build writing to a migrated registry still moves the counter). |
| +Not built, and still needed before this could replace the external binary: the replica credentials out of everything else's reach; a restore path; the prefix-lineage guard; the shrink guard; opening databases beneath a pinned root; a caller that rebuilds the fleet; the registry migration reviewed as a schema change on every box (it adds one table and four triggers, and an older build writing to a migrated registry still moves the counter). |
| ## Review three (reviewed `c8627694`; Opus accepted with fixes, Astra rejected) |
| @@ -142,9 +142,32 @@ Reports: `OPUS-REVIEW-metered-replicator-review5.md`, `CODEX-FINDINGS-metered-re |
| | Opus P3-4: a platform-owned start directory was not checked for world-writability; a tenant whose uid is the platform's has no isolation. | Fixed for the first: a platform-owned start directory must be writable by nobody else. The second is stated: an untrusted tenant must not share the platform's uid (operator rule, with approval H). | `litestream_owners.go` | `TestUnitFileInAWorldWritablePlatformDirectory` | |
| | Opus P3-6, Astra 8 (P2): a database under a sidecar name beside an unrelated regular file was never read. | Fixed: a sidecar name is a sidecar only beside a database (by name or by header). | `litestream_owners.go` `sidecarOf` | `TestStrictTreeReadsAStandaloneSidecarName` | |
| | Opus P3-6, Astra 7 (P2): a claimed or discovered file that vanished before its final look disappeared. | Fixed: it is listed as skipped. | `litestream.go` | no deterministic test (the race between the walk and the final look is not simulated); stated | |
| -| Opus P3-5: the header read's `O_NOFOLLOW` covers the last component only; a directory swapped higher up can lead the open to a device; no `O_NOCTTY`. | Open: a walk by directory descriptor (`openat`, `openat2` with `RESOLVE_NO_SYMLINKS`) is the fix and is not built; `O_NOCTTY` is not added either, since the open is still by path. Stated. | | | |
| +| Opus P3-5: the header read's `O_NOFOLLOW` covers the last component only; a directory swapped higher up can lead the open to a device; no `O_NOCTTY`. | Open in part: a walk by directory descriptor (`openat`, `openat2` with `RESOLVE_NO_SYMLINKS`) is the fix and is not built; `O_NOCTTY` was added by the seventh build (review six showed it does not depend on how the path is resolved). | | | |
| | Opus P3-7, Astra 9 (P2): tests that passed without their fixes (the swap's lower bound; the close-once; no retry tests). | Fixed: the swap test drives `lookAgain` with bounds that tell the two apart; the close test holds the shutdown open; the witness retries have a table (recovered error, exhausted errors, a change, a change after an error). Still open: retained database handles and worker termination are not asserted; attribution per database is not compared; the watch's own invocation of the look is not exercised. | tests | as named | |
| | Astra 1 (P1): litestream retains a substituted database opened by path. | Open, as before; needs a litestream change. Nothing is attested until it is. | | | |
| -| Astra claims (dispositions line about dropping the old name first; `ErrOwnershipStale` "from then on"; the test fixtures' umask and base-name seam). | The order is CREATE then DROP, stated; the fixtures depend on umask 022 (stated here); the seam keys on base names, safe for these fixtures and said so. | this file | | |
| +| Astra claims (dispositions line about dropping the old name first; `ErrOwnershipStale` "from then on"; the test fixtures' umask and base-name seam). | The order is CREATE then DROP, stated; the fixtures create their directories with an explicit mode (`platformDir` chmods 0755; `MkdirAll(0o755)` is narrowed by a restrictive umask such as 077, so the fixtures assume umask 022 or 002, not that umask could add permission); the seam keys on base names, safe for these fixtures and said so. | this file | | |
| Package tests pass under `/tmp` and under `~/.cache`. The full gate on this tip is next; the sixth build has not been read by a reviewer. |
| + |
| +## Review six (reviewed `d63f81df`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-metered-replicator-review6.md`, `CODEX-FINDINGS-metered-replicator-review6.md`. Both found the sixth build's bounded fixes present and the same gaps in them: the platform-link rule refused the relative link systemd actually makes, a registry finding held from the last proof alone, the inherited window latched one level deep, the header open lacked `O_NOCTTY`, and two tests did not pin their fixes. Astra's P1s on litestream's own opens and shadow ancestry stand. This section records the seventh build, which is unreviewed. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P2-1: systemd's state link is relative (`../private/carlos-app/<host>`) and the rule refused it, so every DynamicUser tenant was unowned. | Fixed: a relative target is taken from the link's own directory, which is link-free by then; the fixture's link is relative. | `litestream_owners.go` `resolvePlatformLinks` | `TestUnitFileThroughAPlatformMadeStateLink` | |
| +| Astra 5 (P2): cleaning the target lexically could hide a traversal through a link the tenant controls (`choice/../x`). | Fixed: the target's components are walked, not cleaned, and any link among them refuses. | `litestream_owners.go` `noLinkInPath` | same test (a platform link whose target passes through the tenant's link) | |
| +| Opus P2-2, Astra 2 (P1): a finding by the registry held from the last proof alone, so a swap made after the last clean look and before the finding was unheld, and no further look ran. | Fixed: every finding holds from the earlier of the last proof and the last clean look. Not fixed: requests tracked to completion and the crash case, as before. | `litestream_metered.go` `goStale` | `TestARegistryFindingHoldsFromTheLastCleanLookToo` | |
| +| Opus P3-3, Astra 9 (P2): the last-clean test did not tell the look's start from its end. | Fixed: a slow look (a test seam) shows the start is recorded, and a swap found by the next look is held from before it. | `litestream_metered_test.go` | `TestTheLastCleanLookIsItsStart` | |
| +| Opus P3-1, Astra 3 (P1): a fleet two back in the chain that went stale after the fleets between had closed never reached the new one (the intermediate latched an empty inheritance when its nearest predecessor closed). | Fixed: a fleet latches only when every fleet in the chain has closed; until then it answers for its predecessors through `staleWindow`. | `litestream_metered.go` `inheritHold` | `TestAHoldReachesThroughTwoUnopenedFleets` | |
| +| Opus P2-3, Astra 8 (P3): the header open had no `O_NOCTTY`, so a tenant could make a pty the replicator's controlling terminal and hang it up. | Fixed: `O_NOCTTY` is set. Still open: a walk by directory descriptor, so a directory swapped higher up cannot steer the open to a device; the dispositions' earlier sentence that `O_NOCTTY` depended on how the path was resolved was wrong and is withdrawn. | `sqliteopen_unix.go` | none (the flag's effect is the kernel's); stated | |
| +| Opus P3-2: the link-above test passed without its fix. | Fixed: the fixture puts a real directory between the link and the other tenant's tree. | tests | `TestUnitFileChainRefusesALinkAboveIt` | |
| +| Astra 6, Opus P3-7: a database under a sidecar name beside a real database was not read. | Fixed: every regular file is read for the header, sidecar names included; a real sidecar never carries the database header (a write-ahead log begins with its own magic), so none is misreported. | `litestream_owners.go` | `TestStrictTreeReadsAStandaloneSidecarName` | |
| +| Astra 7, Opus P3-7: a claimed glob match that vanished between the glob and the final look was dropped. | Fixed: it is listed as skipped. Hibernating routes' databases remain unlisted by design (the activator replicates them); said here. | `litestream.go` | none (the race is not simulated); stated | |
| +| Opus P3-5: a tenant whose uid is the platform's is accepted anywhere the platform owns; a link it makes passes as the platform's. | Open, stated as the operator rule: an untrusted tenant must not share the platform's uid. Refusing a platform-uid owner for unit routes would refuse every fixture that uses the test user, and a route-level rule (the ops account only) is the right shape; with approval H. | | | |
| +| Opus P3-6: two fleets outside one chain can open over one shadow directory and prefix. | Stated in `MeteredFleetOptions.Replaces`: the caller keeps the chain single. | `litestream_metered.go` | | |
| +| Opus P3-3: `Open` racing `Close` has no test; retained handles and worker exit not asserted; the watch's own call of the look not exercised. | Open, as stated. | | | |
| +| Astra 1 (P1), Astra 4 (P1): litestream's opens by path; `dataDir` and the shadow directories' ancestry; the ancestry of the directory holding a platform link and of its target's parent. | Open, as before. | | | |
| +| 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. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review6.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review6.md| new file mode 100644 |
| index 000000000..bbbd3b2fc |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review6.md |
| @@ -0,0 +1,200 @@ |
| +# Sixth review: metered replicator at `d63f81df` (Opus) |
| + |
| +**Verdict: accept with fixes.** |
| +- No P1 found. |
| +- Three P2s: |
| + - The platform-link rule refuses systemd's real DynamicUser link. |
| + - Once the registry makes the fleet stale, the files are never looked at again, so a swap made just before it can leave a day unheld. |
| + - The header open is still a root open by path with no `O_NOCTTY`. |
| +- The code is still unwired. Astra 1 (the swap during litestream's first open) and Astra 4 (ancestry) still block any attestation. |
| + |
| +**Method.** |
| +- The brief's "sixth build" diff (`d63f81df d63f81df`) is empty, so I read `1b3cc29e..d63f81df` as the sixth build and `72b4f59a..d63f81df` as the whole branch. |
| +- I read everything with `git show`. Nothing was run or modified. |
| +- Line numbers are at `d63f81df`. |
| + |
| +## Review five, finding by finding |
| + |
| +| Finding | Now | Would its test pass without the fix? | |
| +|---|---|---| |
| +| Opus P2-1 / Astra 2 (lower bound of a file-swap hold; no widening) | **Closed for the reported sequence** (`litestream_metered.go:522`, `:527`, `:685`). Widening works (`:299-317`). A new gap: a registry finding ends all file looks (P2-2). | **No.** With `began`, the hold starts an hour ago and the 48-hour-old day is not held. **Yes** for storing the look's *end* at `:527` instead of its start, and for deleting `:527`. | |
| +| Opus P3-1 / Astra 3 (union; latch before the predecessor closes) | **Closed one level deep.** Two unopened fleets in a row still latch early (P3-1). | **No** for the union, and **no** for the latch gate (`b2`/`c2`). | |
| +| Opus P3-2 / Astra 5 (`Open` racing `Close`) | **Closed in code** (the `life` lock, `:429`, `:573`). | **No test.** Removing `f.life.Lock()` from `Open` passes every test. | |
| +| Opus P3-3 / Astra 4 (a link above the chain top; the start link's intermediate resolution) | **Closed** (`resolvePlatformLinks`, `litestream_owners.go:607-651`; the top check at `:392`). The absolute-target rule regresses the DynamicUser case (P2-1). | **Yes** for `TestUnitFileChainRefusesALinkAboveIt` (P3-2). **No** for the `via → hop/choice` case. | |
| +| Opus P3-4 (platform start directory's mode; a tenant with the platform's uid) | **Closed** for the mode (`:359-365`). The uid half is stated but not enforced. | **No** for the 1777 case. | |
| +| Opus P3-5 / Astra 7 (device open through a parent swap; `O_NOCTTY`) | **Open.** The dispositions' reason for not adding `O_NOCTTY` does not hold (P2-3). | n/a | |
| +| Opus P3-6 / Astra 8 (a sidecar name beside an unrelated file) | **Closed** for a base that is not a database (`:580-599`). A file with SQLite's header beside a real database is still suppressed; SQLite would reset it as a WAL, so this is minor. | **No** (`archive` / `archive-journal`). | |
| +| Opus P3-6 / Astra 7 (a vanished claimed or discovered file) | **Closed** (`litestream.go:498-503`). | **No test.** | |
| +| Opus P3-7 / Astra 9 (tests) | **Partly.** | Retry table: **no**, it fails without the retries. Close test: it fails only if **both** `closeOnce` and `life` are removed. Each alone serialises the second caller, which is acceptable. Still untested: `discovered[p]`, the device-and-inode branch, retained handles, worker exit, per-database attribution, and the watch calling `lookAgain`. | |
| +| Astra 1 (first-open swap), Astra 4 (ancestry of `dataDir` and shadow state) | **Open, stated.** | n/a | |
| +| Astra 6 (a file nested beneath the DynamicUser link) | **Closed in code.** | **No** (the nested fixture). The fixture uses an absolute link, though (P2-1). | |
| +| Claims (both reviewers) | **Corrected:** the `goStale` doc, the `TestUnitFileThroughAPlatformMadeStateLink` comment, and "the admission tests" (they exist now). **Still wrong:** see the Claims section. | | |
| + |
| +## Findings |
| + |
| +### P2-1. The platform-link rule refuses the link systemd actually makes |
| + |
| +- **Where:** `litestream_owners.go:633-636` (`!filepath.IsAbs(target)` refuses). |
| +- **Background:** for `DynamicUser=` with `StateDirectory=carlos-app/<host>`, systemd creates the link with `symlink_idempotent(pp, p, true)`. The `true` means make it relative, so `/var/lib/carlos-app/<host> -> ../private/carlos-app/<host>`. Check one box with `readlink`, but this is systemd's documented shape. |
| +- **Scenario:** |
| + 1. A DynamicUser unit's route names `/var/lib/carlos-app/<host>/registry.db`. |
| + 2. `resolvePlatformLinks` reaches the link, reads a relative target, and returns "not an absolute link". |
| + 3. The file is `Unowned`, and `MeteredFleetStore` refuses the whole box with `ErrUnmeterable`. |
| +- **Impact:** this fails closed, not with a wrong bill. But it undoes review four's P2-1 for every DynamicUser tenant. The tests pass only because `TestUnitFileThroughAPlatformMadeStateLink` creates an absolute link (`unix_test.go:98`). |
| +- **Note:** review five (mine) suggested requiring an absolute target. That advice was wrong for this shape. |
| +- **Fix:** resolve a relative target against the link's directory: `target = filepath.Join(prefix, target)`. `prefix` is already link-free at that point, so lexical `..` matches the kernel's. Keep the no-further-link, is-a-directory and parent-is-platform checks. Make the fixture's link relative, as systemd makes it. |
| + |
| +### P2-2. Once the registry makes the fleet stale, the files are never looked at again, so a swap made just before is never held (M) |
| + |
| +- **Where:** |
| + - `litestream_metered.go:282-283`: `goStale` passes no `lookStart`, so the hold starts at `proven − margin`. |
| + - `:501-502`: `lookAgain` returns once stale. |
| + - `:615`: `Close` skips its last reading when stale and takes no last look at the files. |
| +- **Scenario:** |
| + 1. A big tenant tree makes one look take about 4 minutes. Look N runs 23:58:00 to 00:02:00 and checks `x.db` at 23:58:05, before any swap. |
| + 2. The tenant swaps `x.db` at 23:58:30. Litestream streams from the swapped path on day D, counted to the tenant. |
| + 3. At 00:01:30 an unrelated route is added. A worker's `admit` finds the change, so `goStale(00:01:30)` holds from about 00:00:29 (`proven` is about a second old under traffic). |
| + 4. Look N finishes clean, because it saw `x.db` before the swap. No further look runs. |
| + 5. Day D holds a settled row for bytes the code's own model says are in doubt. |
| +- **Why it slips:** the widening added this round only works while a look is already in flight *and* that look reaches the file after the swap. |
| +- **Fix:** have `goStale` hold from `earlier(proven, lastClean)`, i.e. `goStaleFrom(time.Unix(0, f.lastClean.Load()), at)`. That costs at most one look's span of extra hold. Add a test that drives the registry finding with `lastClean` before midnight and `proven` after it. |
| + |
| +### P2-3. The header read is a root open by path with no `O_NOCTTY`, and a tenant can steer it to a tty (K) |
| + |
| +- **Where:** `sqliteopen_unix.go:18`. |
| +- **Scenario:** |
| + 1. The walk `Lstat`s `T/d/3`, a small regular file with no `.db` name, so it goes to the header read. |
| + 2. T flips `d` to a link to `/dev/pts` in a loop. With `d` real, the `Lstat` sees a regular file; with `d` a link, the open (`O_NOFOLLOW` covers only the last component) opens `/dev/pts/3`. That is a pty slave T made from `/dev/ptmx`. |
| + 3. A systemd service is a session leader with no controlling tty. Without `O_NOCTTY`, Linux makes the pty its controlling terminal; the later `fstat` refuses the file, but the controlling tty stays. |
| + 4. T closes the master, and the kernel sends `SIGHUP` to the session leader. |
| + 5. Go exits on `SIGHUP` unless it is `Notify`'d (only the edge does that). A tenant can kill the root replicator every 30 s. |
| +- **Also:** devices that act on open (a watchdog, tape rewind) are still reached, as in review five. |
| +- **Claim:** the dispositions say `O_NOCTTY` "is not added either, since the open is still by path". `O_NOCTTY` does not depend on how the path is resolved. |
| +- **Fix:** |
| + - Now: add `syscall.O_NOCTTY`. It exists on every unix target, including darwin, so the cross-compile gate is fine. |
| + - Before any attestation: walk by directory descriptor, with `openat` and `O_NOFOLLOW|O_DIRECTORY`, or `openat2` with `RESOLVE_NO_SYMLINKS`. |
| + |
| +### P3-1. A fleet replacing an unopened, unlatched fleet latches before the chain's root has closed (D, M) |
| + |
| +- **Where:** `litestream_metered.go:393`. `inheritHold` checks only `f.replaces.closed`, not the whole chain the way `Open` does at `:438`. |
| +- **Scenario:** |
| + 1. A is open. B (`Replaces: A`) is built and closed: A is open, so B does not latch. |
| + 2. C (`Replaces: B`) is built and closed. B is closed, so C latches `B.staleWindow()`, which delegates to A. A is not stale yet, so C latches "nothing". |
| + 3. A goes stale (day X) and closes. |
| + 4. D (`Replaces: C`) opens: the whole chain is closed. D inherits C's latched empty window, so day X never reaches D's recorders. |
| +- **Fix:** latch only when every fleet in the chain is closed (reuse `Open`'s loop). Add a test for this sequence. |
| + |
| +### P3-2. `TestUnitFileChainRefusesALinkAboveIt` passes without the fix (H) |
| + |
| +- **Where:** `unix_test.go` (the new test). |
| +- **Why:** the link `a` is V's parent itself. At `1b3cc29e` the climb broke at `apps/T/a`, and `platformOwned(apps/T/a)` `Lstat`s a link, so it already refused. Review five's scenario needs a directory between the link and V: `a → private`, spelled `apps/T/a/carlos-app/V/main.db`. |
| +- **Fix:** add that component. |
| + |
| +### P3-3. Other tests that do not pin their fixes (H) |
| + |
| +- **`lastClean`:** |
| + - Storing the look's end at `:527` survives every test, and that mutation reopens review five's P2-1. |
| + - Deleting `:527` also survives. |
| + - Fix: a test with two looks where the second finds the swap and the first must have recorded its *start*. |
| +- **`Open`'s `life` lock:** no test pauses `Open` before `store.Open` while `Close` runs. |
| +- **Unchanged since review five:** |
| + - The vanished-file branch, `discovered[p]`, and the device-and-inode branch. |
| + - Store handles and worker exit. |
| + - The watch's own call of `lookAgain`. |
| + |
| +### P3-4. Tests depend on the temp root's layout (H) |
| + |
| +- **Relative links:** `resolvePlatformLinks` walks every component from `/`. On macOS, `/var -> private/var` and `/tmp -> private/tmp` are relative links, so every tenant-file fixture fails there with "not an absolute link". Any relative link in `$TMPDIR`'s path does the same on Linux. The P2-1 fix removes this. |
| +- **Seam names:** `withOwners` keys on base names over every component it is asked about. A temp path containing `private`, `data`, `apps`, `shared`, `hop`, `A`, `T` or `V` has that component's owner overridden. That holds on macOS (`/private`). |
| +- **Umask:** the fixtures depend on umask 022. This is stated. |
| + |
| +### P3-5. A tenant with the platform's uid is still accepted almost anywhere the platform owns, and its own links count as the platform's (A, J) |
| + |
| +- **Where:** `litestream_owners.go:359-365`, `:379`, `:630`. |
| +- **Scenario:** a root-run unit's route names `/var/lib/fail2ban/fail2ban.db`. The start directory is root's and 0755, its parent is root's, and the file is root's with one name, so it is accepted and streamed into that tenant's replica. Any link the tenant itself makes in a root-owned directory passes as "the platform's" at `:630`. |
| +- **Limits:** DBPath is set by the operator (`litestream.go:362`), and the dispositions state the operator rule. It is still unenforced. |
| +- **Fix:** for `tenantFile`, refuse an owner uid that is the platform's unless the route is the ops account. |
| + |
| +### P3-6. Nothing prevents two live fleets outside a `Replaces` chain (I) |
| + |
| +- **Where:** `Open` (`:438`) guards only the chain it was given. |
| +- **Scenario:** B and C, both `Replaces: A`, both open after A closes. Or a fleet is built with `Replaces: nil` beside a running one. Either way, two stores run over one shadow directory and one prefix. |
| +- **Fix:** a process-wide registry of the open fleet per `dataDir`/prefix, or say in `MeteredFleetOptions.Replaces` that the caller must keep it single. |
| + |
| +### P3-7. Remaining silent drops (L) |
| + |
| +- **Where:** `litestream.go:494-509`. |
| +- **What:** |
| + - A claimed glob match from an exec or flat route that vanishes between `Glob` and `Stat` falls through every case. This is the activator's directory, so it is low-risk churn. |
| + - Hibernating routes' databases (`:512`) are dropped without being named. |
| + - A file with SQLite's header under a sidecar name beside a real database is not read. |
| +- **Otherwise reported:** |
| + - New this build: a relative unit path, a missing unit claim, and a discovered or unit file that vanishes before its final look. |
| + - Unchanged since review five: case-variant names (as skipped), unreadable directories, and no-path unit routes. |
| + |
| +## A–M, briefly |
| + |
| +- **A.** |
| + - A tenant with its own user: no. Any non-platform link anywhere in the spelled path is refused (`:627-631`). A directory of another non-platform user stops the climb and fails `platformOwned` (`:395`). A platform-owned start directory requires a platform-owned file (`:436`). |
| + - A tenant with the platform's uid, and a box whose units run as root: the climb never climbs. A file is accepted from any root-owned, non-group/world-writable directory whose parent is the same, with no link in the path (P3-5). |
| + - The top-of-chain link check (`:384`) and the resolved-equals-spelled check (`:392`) are reached on every accepting path. Every other exit refuses. |
| +- **B.** |
| + - Unchanged since review five. A request admitted before a change still reaches the network after it (inherent, stated, covered by the margin). |
| + - The re-check under the lock (`:214`, `:228`) is complete for the read window. The context is not re-checked after waiting for `f.mu`; that is minor. |
| + - Retries are bounded at 3 × 5 s + 2 × 200 ms. Only exhausted errors count as a change; a witnessed `false` is immediate. The new table tests both. |
| +- **C.** No change this build. Four triggers: insert, delete, changed-value update, and the rowid trigger (`registry.go:599-609`). They cover an older build's raw SQL. A database-free insert or delete moves the generation too, as disclosed. |
| +- **D.** |
| + - No reading is taken on a stale fleet (`:615`). |
| + - The inherited window is applied once (the CAS) and passed on as a union. |
| + - It is lost only in the P3-1 sequence. |
| + - `Close` waits for litestream's workers whatever the context does (`store.go:240-241`), so `closed` is not published early. |
| +- **E.** No change. Lineage and shadow state are keyed by account plus cleaned path. Different accounts never share. A path moved between accounts starts a new key. The same account at the same path deliberately reuses it. |
| +- **F.** As in review five, with the sidecar fix. Case variants are skipped; every other regular non-empty file is header-read; an unreadable directory is a problem; a no-path unit route is a problem; an empty claimed file is skipped. |
| +- **G.** |
| + - **First-open swap:** correct. It is slightly larger than stated, because any later litestream reopen by path is the same. |
| + - **Ancestors of `dataDir` and shadow state:** correct, and this build adds a sibling. `resolvePlatformLinks` checks only the directory holding a platform link and the target's parent, not their ancestry. |
| + - **Crash gap:** correct. |
| + - **Bind mounts:** the substance is correct; the wording is not (Claims). |
| + - **External inventory:** correct. |
| + - **Missing from the list:** P2-1 and P2-2. P2-3 is understated. |
| +- **I.** |
| + - No admission after the file finding, except the instant between `:228` and `return`, which is covered by the margin. |
| + - No replacement in the chain opens beside a draining store. |
| + - Outside the chain: P3-6. |
| +- **J.** |
| + - A platform link to a directory the tenant can replace: refused when the target's parent is not the platform's. Its ancestry is unchecked (G). |
| + - A platform-uid tenant: P3-5. |
| + - A DBPath that is itself a link to another tenant's file: refused (`:321-323`). |
| + - A DBPath naming another tenant's file directly, not through a link: accepted unless both routes list it. This is review four's P3-6, still open. |
| +- **K.** |
| + - The race between `Lstat` and open is caught by the device-and-inode check, after the open's side effects (P2-3). |
| + - A sidecar's base that is a link, or becomes one: `Lstat` or `O_NOFOLLOW` fails, and the name is read as an ordinary file. |
| + - Every unix target has `O_NOFOLLOW`. `!unix` uses a plain open and owns nothing. |
| +- **L.** P3-7. |
| +- **M.** P2-2 and P3-1. The crash case and requests tracked across midnight remain, as stated. |
| + |
| +## Claims the code contradicts |
| + |
| +- **`ErrOwnershipStale` (`litestream_metered.go:33`):** still says "Every replica request is refused from then on" above the newer paragraph. Dispositions row `:124` says this was corrected. This is the third round. |
| +- **`Close` doc (`:560-562`):** "Closing a fleet that was current holds nobody's day". `:616-617` goes stale on exhausted retries. |
| +- **`lookAgain` (`:516`):** "Gone altogether: nothing is being shipped from it". Litestream keeps its descriptors (Astra). |
| +- **`strictTreeDBs` doc (`litestream_owners.go:486`, `:491-492`):** |
| + - "passes nothing over in silence" (P3-7). |
| + - "a file with no extension that begins with SQLite's header": it reads every regular file. |
| +- **`goStaleFrom` doc (`:286-289`):** "by the files after the registry … must not be lost". After a registry finding no file look runs (P2-2). |
| +- **Dispositions:** |
| + - `:75`: "three triggers". There are four. |
| + - `:95`: "only an operator or systemd" can make a bind mount. The condition is mount authority, not the actor. |
| + - Sixth-build `O_NOCTTY` row: the reason given is wrong (P2-3). |
| + - Union row: "nothing is dropped between fleets" (P3-1). |
| + - Opus P3-4 row: an unenforced operator rule. |
| +- **`TestUnitFileThroughAPlatformMadeStateLink` / `resolvePlatformLinks` comments:** "systemd's DynamicUser shape" with an absolute target. systemd makes the link relative (P2-1). |
| + |
| +**Verdict: accept with fixes.** Before merge: |
| +- Fix P2-1 (accept relative platform links, with a relative fixture). |
| +- Fix P2-2 (hold from `lastClean` on a registry finding too). |
| +- Add `O_NOCTTY` (P2-3). |
| +- Fix P3-1 and P3-2. |
| +- Correct the claims. |
| + |
| +Before any attestation: P3-5, the descriptor walk, Astra 1 and Astra 4. |
diff --git a/internal/ops/litestream.go b/internal/ops/litestream.go| index 88386b633..75ec3b807 100644 |
| --- a/internal/ops/litestream.go |
| +++ b/internal/ops/litestream.go |
| @@ -422,15 +422,17 @@ func fleetDBsFromRoutes(routes []registry.Route, registryPath, dataDir string, t |
| claimed[filepath.Clean(r.DBPath)] = true |
| } |
| } |
| - var globbed []string |
| + var kept []string |
| + globbed := map[string]bool{} |
| for _, p := range matches { |
| if claimed[filepath.Clean(p)] { |
| - globbed = append(globbed, p) |
| + kept = append(kept, p) |
| + globbed[filepath.Clean(p)] = true |
| continue |
| } |
| skipped = append(skipped, filepath.Clean(p)) |
| } |
| - matches = globbed |
| + matches = kept |
| // Unit-tenant DBPaths are added on top of the glob: they typically live |
| // entirely outside dataDir, at their StateDirectory path. A DBPath naming |
| @@ -495,11 +497,12 @@ func fleetDBsFromRoutes(routes []registry.Route, registryPath, dataDir string, t |
| switch { |
| case err != nil && !errors.Is(err, fs.ErrNotExist): |
| problems = append(problems, p+": cannot be looked at") |
| - case err != nil && (discovered[p] || unitPaths[p] != ""): |
| + case err != nil && (discovered[p] || unitPaths[p] != "" || globbed[p]): |
| // Found a moment ago and gone now: a tenant renaming a |
| - // database under the look. Said, so a database that |
| - // comes back after the build is known to have been |
| - // unreplicated until the next look. |
| + // database under the look, or an activator directory |
| + // in churn. Said, so a database that comes back after |
| + // the build is known to have been unreplicated until |
| + // the next look. |
| skipped = append(skipped, p) |
| case err == nil && !fi.Mode().IsRegular(): |
| problems = append(problems, p+": not a regular file") |
diff --git a/internal/ops/litestream_metered.go b/internal/ops/litestream_metered.go| index fd0a00331..017020b26 100644 |
| --- a/internal/ops/litestream_metered.go |
| +++ b/internal/ops/litestream_metered.go |
| @@ -102,7 +102,11 @@ type MeteredFleetOptions struct { |
| // it asks the witness one last time. At Open, if that fleet went |
| // stale, every recorder of this one is told the same days are |
| // incomplete: the new owner of a database that changed hands is one |
| - // of them, and the old fleet never had its recorder. |
| + // of them, and the old fleet never had its recorder. The caller keeps |
| + // the chain single: one fleet per data directory and prefix at a time, |
| + // each built to replace the last. Nothing here stops two fleets built |
| + // beside each other, or two built to replace the same one, from |
| + // opening over one shadow directory and one prefix. |
| Replaces *MeteredFleet |
| // SyncInterval is litestream's replica sync tick. Zero keeps its |
| // default. |
| @@ -156,8 +160,10 @@ type MeteredFleet struct { |
| // an Open in progress. |
| life sync.Mutex |
| // closingHook, when set by a test, runs inside the one shutdown after |
| - // the store is taken and before it is closed. |
| + // the store is taken and before it is closed. lookDelay, when set by a |
| + // test, is how long a look at the files takes after it has begun. |
| closingHook func() |
| + lookDelay time.Duration |
| staleMu sync.Mutex |
| staleFrom, staleAt time.Time |
| @@ -280,15 +286,27 @@ func (f *MeteredFleet) witnessUnchanged() (ok bool, err error) { |
| // (ownershipMargin). A request held between admission and recording for |
| // longer than the margin is outside this, as the constant says. |
| func (f *MeteredFleet) goStale(at time.Time) { |
| - f.goStaleFrom(time.Time{}, at) |
| + f.goStaleFrom(f.lastCleanLook(), at) |
| +} |
| + |
| +// lastCleanLook is the start of the last look at the files that found |
| +// every one as it was, or the zero time for a fleet that never looked (a |
| +// fleet built by hand in a test). |
| +func (f *MeteredFleet) lastCleanLook() time.Time { |
| + if n := f.lastClean.Load(); n != 0 { |
| + return time.Unix(0, n) |
| + } |
| + return time.Time{} |
| } |
| // goStaleFrom makes the fleet stale (or, if it is stale already, widens |
| -// what it holds: a second finding, by the files after the registry or the |
| -// other way round, can reach further back than the first, and must not be |
| -// lost to the first). lookStart, when set, is the start of the last look |
| -// at the files that found nothing; the hold runs from the earlier of that |
| -// and the last registry proof, less the margin. |
| +// what it holds: a second finding can reach further back than the first, |
| +// and must not be lost to the first). lookStart is the start of the last |
| +// look at the files that found nothing; the hold runs from the earlier of |
| +// that and the last registry proof, less the margin, whichever of the two |
| +// made the finding: a registry finding ends the looks at the files too, |
| +// and a swap made after the last clean look and before it is in doubt as |
| +// well. That costs at most one look's span of extra hold. |
| func (f *MeteredFleet) goStaleFrom(lookStart, at time.Time) { |
| sound := time.Unix(0, f.proven.Load()) |
| if !lookStart.IsZero() && lookStart.Before(sound) { |
| @@ -390,7 +408,19 @@ func later(a, b time.Time) time.Time { |
| // its predecessor does not latch, and answers for its predecessor through |
| // staleWindow until a later fleet reads it. |
| func (f *MeteredFleet) inheritHold() { |
| - if f.replaces == nil || !f.replaces.closed.Load() || !f.inherited.CompareAndSwap(false, true) { |
| + if f.replaces == nil { |
| + return |
| + } |
| + // Every fleet in the chain, not only the nearest: a fleet closed |
| + // without opening answers for its predecessor until that one has |
| + // closed and said its last, and latching before then would latch |
| + // nothing. |
| + for prev := f.replaces; prev != nil; prev = prev.replaces { |
| + if !prev.closed.Load() { |
| + return |
| + } |
| + } |
| + if !f.inherited.CompareAndSwap(false, true) { |
| return |
| } |
| if from, at, stale := f.replaces.staleWindow(); stale { |
| @@ -502,6 +532,9 @@ func (f *MeteredFleet) lookAgain() { |
| return |
| } |
| began := time.Now() |
| + if f.lookDelay > 0 { |
| + time.Sleep(f.lookDelay) |
| + } |
| inv, err := fleetInventory(f.routes, f.registryPath, f.dataDir) |
| if err != nil { |
| f.drift.Store(true) |
| @@ -519,7 +552,7 @@ func (f *MeteredFleet) lookAgain() { |
| // 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. |
| - f.goStaleFrom(time.Unix(0, f.lastClean.Load()), time.Now()) |
| + f.goStaleFrom(f.lastCleanLook(), time.Now()) |
| return |
| } |
| // Every file is as it was: nothing before this look's start is in |
diff --git a/internal/ops/litestream_metered_test.go b/internal/ops/litestream_metered_test.go| index cb9ce15f8..55dcd252c 100644 |
| --- a/internal/ops/litestream_metered_test.go |
| +++ b/internal/ops/litestream_metered_test.go |
| @@ -1436,3 +1436,76 @@ func TestWitnessReadsAreRetriedBeforeTheyCount(t *testing.T) { |
| }) |
| } |
| } |
| + |
| +// A change found in the registry holds from the last clean look at the |
| +// files as well as from the last proof: a swap made after that look and |
| +// before the registry finding is in doubt too, and no further look runs. |
| +func TestARegistryFindingHoldsFromTheLastCleanLookToo(t *testing.T) { |
| + day := func(d int, h, m, s int) time.Time { return time.Date(2026, 10, d, h, m, s, 0, time.UTC) } |
| + a := &countingRecorder{} |
| + f := &MeteredFleet{recorders: []lease.RequestRecorder{a}} |
| + f.lastClean.Store(day(2, 23, 58, 0).UnixNano()) |
| + f.proven.Store(day(3, 0, 1, 29).UnixNano()) |
| + f.goStale(day(3, 0, 1, 30)) |
| + if got := a.days(); !got["2026-10-02"] || !got["2026-10-03"] { |
| + t.Fatalf("held %v; want the day of the last clean look and the day of the finding", got) |
| + } |
| +} |
| + |
| +// The last clean look is recorded by its start, not its end: a swap made |
| +// while that look was running, after it had passed the file, is held. |
| +func TestTheLastCleanLookIsItsStart(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}) |
| + fleet, err := MeteredFleetStore(fleetOptions(reg, dir)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer fleet.Close() |
| + fleet.lookDelay = 300 * time.Millisecond |
| + before := time.Now() |
| + fleet.lookAgain() // clean, and slow |
| + after := time.Now() |
| + fleet.lookDelay = 0 |
| + recorded := time.Unix(0, fleet.lastClean.Load()) |
| + if recorded.Before(before) || recorded.After(before.Add(100*time.Millisecond)) || !recorded.Before(after.Add(-200*time.Millisecond)) { |
| + t.Fatalf("the last clean look is recorded at %v; the look began at %v and ended at %v: want its start", recorded, before, after) |
| + } |
| + // And a swap found by the next look is held from that start. |
| + fleet.proven.Store(time.Now().UnixNano()) |
| + swapped := writeFile(t, filepath.Join(platformDir(t), "other.db")) |
| + if err := os.Rename(swapped, db); err != nil { |
| + t.Fatal(err) |
| + } |
| + fleet.lookAgain() |
| + from, _, stale := fleet.staleWindow() |
| + if !stale || from.After(before.Add(-ownershipMargin).Add(100*time.Millisecond)) { |
| + t.Fatalf("held from %v; want from before the clean look's start %v less the margin", from, before) |
| + } |
| +} |
| + |
| +// A fleet two or more back in the chain that goes stale after the fleets |
| +// between it and the new one have closed still reaches the new one: the |
| +// fleets between latch nothing until the whole chain has closed. |
| +func TestAHoldReachesThroughTwoUnopenedFleets(t *testing.T) { |
| + day := func(d int) time.Time { return time.Date(2026, 10, d, 12, 0, 0, 0, time.UTC) } |
| + a := &MeteredFleet{} |
| + b := &MeteredFleet{replaces: a} |
| + b.inheritHold() // A is open: nothing |
| + b.closed.Store(true) |
| + c := &MeteredFleet{replaces: b} |
| + c.inheritHold() // B is closed, A is not: still nothing latched |
| + c.closed.Store(true) |
| + a.proven.Store(day(2).UnixNano()) |
| + a.goStale(day(2)) |
| + a.closed.Store(true) |
| + d := &countingRecorder{} |
| + df := &MeteredFleet{replaces: c, recorders: []lease.RequestRecorder{d}} |
| + df.inheritHold() |
| + if got := d.days(); !got["2026-10-02"] { |
| + t.Fatalf("D holds %v; want A's day, through B and C", got) |
| + } |
| +} |
diff --git a/internal/ops/litestream_owners.go b/internal/ops/litestream_owners.go| index 78de12e50..203c0cb44 100644 |
| --- a/internal/ops/litestream_owners.go |
| +++ b/internal/ops/litestream_owners.go |
| @@ -540,15 +540,12 @@ func strictTreeDBs(root string) (dbs, skipped, problems []string) { |
| dbs = append(dbs, p) |
| case sqliteName(name): |
| skipped = append(skipped, p) |
| - case sidecarOf(p) != "": |
| - // A database's own sidecar, which travels with it: only |
| - // when the database it would belong to is beside it. A |
| - // file so named with no database beside it is any other |
| - // file, read for its header below. |
| default: |
| // Any other regular file is read for SQLite's header: a |
| - // database under some other name is a database. One |
| - // that cannot be read is a place a database could be. |
| + // database under some other name is a database, a sidecar |
| + // name (-wal, -shm, -journal) included, since a real |
| + // sidecar never begins with the database header. One that |
| + // cannot be read is a place a database could be. |
| switch isDB, rerr := sqliteHeader(p, fi); { |
| case rerr != nil: |
| problems = append(problems, p+": cannot be read") |
| @@ -572,28 +569,30 @@ func sqliteName(name string) bool { |
| return false |
| } |
| -// sidecarOf names the database a file would be the sidecar of (its -wal, |
| -// -shm or -journal), when that database exists beside it as a regular |
| -// file that is a database (by name, or by its header); "" otherwise. A |
| -// regular file beside it that is not a database makes it no sidecar, and |
| -// it is read for its own header like any other file. |
| -func sidecarOf(p string) string { |
| - for _, suffix := range []string{"-wal", "-shm", "-journal"} { |
| - if !strings.HasSuffix(p, suffix) { |
| +// 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 { |
| + if part == "" || part == "." { |
| continue |
| } |
| - base := p[:len(p)-len(suffix)] |
| - fi, err := os.Lstat(base) |
| - if err != nil || !fi.Mode().IsRegular() { |
| - return "" |
| + if part == ".." { |
| + prefix = filepath.Dir(prefix) |
| + continue |
| } |
| - if sqliteName(filepath.Base(base)) { |
| - return base |
| + next := filepath.Join(prefix, part) |
| + fi, err := os.Lstat(next) |
| + if err != nil { |
| + return next + " cannot be looked at" |
| } |
| - if isDB, herr := sqliteHeader(base, fi); herr == nil && isDB { |
| - return base |
| + if fi.Mode()&os.ModeSymlink != 0 { |
| + return next + " is a link" |
| } |
| - return "" |
| + prefix = next |
| } |
| return "" |
| } |
| @@ -631,14 +630,22 @@ func resolvePlatformLinks(dir string) (string, string) { |
| return "", "a link in its path is not the platform's" |
| } |
| target, rerr := os.Readlink(next) |
| - if rerr != nil || !filepath.IsAbs(target) { |
| - return "", "a link in its path is not an absolute link" |
| + if rerr != nil { |
| + return "", "a link in its path cannot be read" |
| } |
| - target = filepath.Clean(target) |
| - real, eerr := filepath.EvalSymlinks(target) |
| - if eerr != nil || filepath.Clean(real) != target { |
| - return "", "a link in its path leads through another link" |
| + // 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. |
| + if !filepath.IsAbs(target) { |
| + target = filepath.Join(prefix, target) |
| } |
| + // 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 != "" { |
| + return "", "a link in its path leads through another link: " + why |
| + } |
| + target = filepath.Clean(target) |
| 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_test.go b/internal/ops/litestream_owners_test.go| index 5cb6611e9..e886ad2ff 100644 |
| --- a/internal/ops/litestream_owners_test.go |
| +++ b/internal/ops/litestream_owners_test.go |
| @@ -508,7 +508,10 @@ func TestStrictTreeReadsAStandaloneSidecarName(t *testing.T) { |
| if err := os.WriteFile(beside, header, 0o644); err != nil { |
| t.Fatal(err) |
| } |
| - if err := os.WriteFile(beside+"-wal", header, 0o644); err != nil { |
| + // A real write-ahead log begins with SQLite's WAL magic, not the |
| + // database header: it is read and passed over. |
| + wal := append([]byte{0x37, 0x7f, 0x06, 0x82}, make([]byte, 60)...) |
| + if err := os.WriteFile(beside+"-wal", wal, 0o644); err != nil { |
| t.Fatal(err) |
| } |
| dbs, skipped, problems := strictTreeDBs(root) |
| @@ -521,14 +524,16 @@ func TestStrictTreeReadsAStandaloneSidecarName(t *testing.T) { |
| if named(skipped, beside+"-wal", "") || named(problems, beside+"-wal", "") { |
| t.Fatalf("a real sidecar is reported: skipped %v problems %v", skipped, problems) |
| } |
| - // A regular file beside it that is not a database makes it no sidecar: |
| - // "archive" (text) and "archive-journal" (a database) are a text file |
| - // and a database, and the database is reported. |
| + // A database under a sidecar name beside a real database is a |
| + // database all the same, and is reported; so is one beside a text file. |
| + if err := os.WriteFile(beside+"-journal", header, 0o644); err != nil { |
| + t.Fatal(err) |
| + } |
| if err := os.WriteFile(filepath.Join(root, "archive"), []byte("notes, not a database"), 0o644); err != nil { |
| t.Fatal(err) |
| } |
| _, skipped, problems = strictTreeDBs(root) |
| - if !named(skipped, alone, "") { |
| - t.Fatalf("a database named archive-journal beside a text file named archive is not reported: skipped %v problems %v", skipped, problems) |
| + if !named(skipped, beside+"-journal", "") || !named(skipped, alone, "") { |
| + t.Fatalf("a database under a sidecar name is not reported: skipped %v problems %v", skipped, problems) |
| } |
| } |
diff --git a/internal/ops/litestream_owners_unix_test.go b/internal/ops/litestream_owners_unix_test.go| index 7495cd2f7..fcee18fe0 100644 |
| --- a/internal/ops/litestream_owners_unix_test.go |
| +++ b/internal/ops/litestream_owners_unix_test.go |
| @@ -95,7 +95,8 @@ func TestUnitFileThroughAPlatformMadeStateLink(t *testing.T) { |
| } |
| real := writeFile(t, filepath.Join(private, "keymail.dev", "registry.db")) |
| link := filepath.Join(public, "keymail.dev") |
| - if err := os.Symlink(filepath.Dir(real), link); err != nil { |
| + // Relative, as systemd makes it: ../private/carlos-app/<host>. |
| + if err := os.Symlink(filepath.Join("..", "private", "carlos-app", "keymail.dev"), link); err != nil { |
| t.Fatal(err) |
| } |
| withOwners(t, map[string]uint32{"carlos-app": 0, "private": 0, "keymail.dev": tenant, "registry.db": tenant, "link:keymail.dev": 0}) |
| @@ -174,17 +175,18 @@ func TestUnitFileChainRefusesALinkAboveIt(t *testing.T) { |
| t.Fatal(err) |
| } |
| } |
| - // V's tree, under a platform directory; T's directory holds a link to |
| - // the platform directory above V's tree. |
| - theirs := writeFile(t, filepath.Join(private, "V", "main.db")) |
| + // V's tree, under a platform directory two levels down; T's directory |
| + // holds a link to the platform directory above, so the spelled path |
| + // reaches V's tree through a real directory below the link. |
| + theirs := writeFile(t, filepath.Join(private, "carlos-app", "V", "main.db")) |
| if err := os.MkdirAll(filepath.Join(apps, "T"), 0o755); err != nil { |
| t.Fatal(err) |
| } |
| if err := os.Symlink(private, filepath.Join(apps, "T", "a")); err != nil { |
| t.Fatal(err) |
| } |
| - withOwners(t, map[string]uint32{"apps": 0, "private": 0, "T": tenant, "link:a": tenant, "V": other, "main.db": other}) |
| - spelled := filepath.Join(apps, "T", "a", "V", "main.db") |
| + withOwners(t, map[string]uint32{"apps": 0, "private": 0, "carlos-app": 0, "T": tenant, "link:a": tenant, "V": other, "main.db": other}) |
| + spelled := filepath.Join(apps, "T", "a", "carlos-app", "V", "main.db") |
| if _, why := checkOwnedFile(filepath.Dir(spelled), spelled, tenantFile); why == "" { |
| t.Fatalf("V's file reached through T's link was accepted (%s)", theirs) |
| } |
diff --git a/internal/ops/sqliteopen_unix.go b/internal/ops/sqliteopen_unix.go| index eab6f1761..9ebff8283 100644 |
| --- a/internal/ops/sqliteopen_unix.go |
| +++ b/internal/ops/sqliteopen_unix.go |
| @@ -15,7 +15,11 @@ import ( |
| // place between the look and the open gets an error, not a hang and not a |
| // read of something else. |
| func openRegularNoFollow(path string, looked os.FileInfo) (*os.File, error) { |
| - fd, err := syscall.Open(path, syscall.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK|syscall.O_CLOEXEC, 0) |
| + // O_NOCTTY: a path steered to a terminal (a pty the tenant made) must |
| + // not become this process's controlling terminal, whose hangup would |
| + // end it; the fstat below refuses the file, but a controlling terminal |
| + // would stay. |
| + fd, err := syscall.Open(path, syscall.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK|syscall.O_CLOEXEC|syscall.O_NOCTTY, 0) |
| if err != nil { |
| return nil, &os.PathError{Op: "open", Path: path, Err: err} |
| } |