carlos / platform Public

The replicator's ninth build: the eighth review's P1 and bounded findings

A latched predecessor was taken for a closed one by Open and inheritHold
(both reviewers, P1): the chain walk now reads each fleet's own closed
flag before trusting its latch, and the latch stores the window, clears
the chain and sets the flag under the stale lock in one step. A ".." in a
platform link's target is taken only in its leading run. Fixtures set
their modes explicitly and the clean-start test assumes no scheduling
bound. Comments both reviewers listed are corrected; one tenant's file
deletion holding every recorder is stated as a known cost. Dispositions
"Review eight".
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
claude agent, operated by paul@keymail.dev 142ef5d02b4d1201b089b1b584e8de73d5a92a3e parent b5aecf1
8 files changed, +394 −150
  • docs/superpowers/specs/DISPOSITIONS-metered-replicator.md +25 −8
  • internal/ops/litestream_metered.go +135 −82
  • internal/ops/litestream_metered_test.go +123 −6
  • internal/ops/litestream_owners.go +32 −17
  • internal/ops/litestream_owners_test.go +41 −6
  • internal/ops/litestream_owners_unix_test.go +28 −28
  • internal/ops/sqliteopen_unix.go +6 −2
  • internal/registry/registry.go +4 −1

CI passed — run details

diff --git a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md
index c51ecfa08..3f8d195f0 100644
--- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md
+++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md
@@ -81,7 +81,7 @@ Reports: `OPUS-REVIEW-metered-replicator-review3.md`, `CODEX-FINDINGS-metered-re
| Finding | Disposition | Where | Reproduction |
| --- | --- | --- | --- |
| Opus P1-1, Astra P1: a unit route that names one file was checked as a named file: `Dir(p)` reached through `Stat`, nothing above it checked, no owner check. A tenant who renames the directory and puts a link in its place has another tenant's database streamed as its own. | Fixed. A unit route's file is a `tenantFile`: the check climbs from the file's directory while the directory above is the same user's and not a link, requires the directory above the top of that chain to be the platform's, refuses a link anywhere from the top down (the top included), and requires the file to be the chain's user's. An exec route's file stays a `namedFile` (its directory is the activator's). | `litestream_owners.go` `checkOwnedFile` | `TestFleetDBOwnersCheckAUnitRoutesNamedFileLikeATree` |
-| Opus P1-2: a swap during litestream's first open pins it to the wrong file for good; `lookAgain` compares the path's current file, not what litestream has open. | Open. Needs litestream to expose the identity of the file it opened, or to open beneath a pinned root; a litestream patch, not made. Stated in `checkOwnedFile`'s comment and here: the second look does not narrow this case. | | |
+| Opus P1-2: a swap during litestream's first open pins it to the wrong file for good; `lookAgain` compares the path's current file, not what litestream has open. | Open. Needs litestream to expose the identity of the file it opened, or to open beneath a pinned root; a litestream patch, not made. Stated in `checkOwnedFile`'s comment and here: the second look does not narrow this case. [Review six withdrew the identity comparison as a preventive fix: it detects a swap after the open, it does not prevent one; and review eight: every reopen and WAL open litestream makes by path is exposed, not the first open alone.] | | |
| Opus P2-1: the hold was passed to the replacement at build, so a fleet built before the old one noticed inherited nothing. | Fixed. `Close` asks the witness one last time before releasing it, and the replacement reads the old fleet's window at `Open`, which already requires the old fleet closed. | `litestream_metered.go` `Close`, `Open` | `TestMeteredFleetReplacementInheritsTheHold` (a case where the old fleet notices only as it closes) |
| Opus P2-2: any route added or removed stops the replicator and holds every day; most such routes have no database. Not stated. | Stated. The generation moves on every insert and delete of a route on purpose: whether a new route claims a database is not known until its files are looked at, and a rebuild is how they are. Narrowing it to rows with a `db_path` or unit backing is a later refinement, with the hold narrowed to the recorders affected; neither is built. On a busy box this means frequent rebuilds, which the caller that nothing yet is must do. | this file | |
| Opus P2-3, Astra P2-6: databases still drop out unseen: a unit route with no `DBPath`; names matched by case; a file with an extension other than `.db`; a claimed file that is missing or empty; a header read that fails. | Fixed in part: a unit route with no path is a problem; names are matched in any case; every other regular file in a tenant's tree is read for SQLite's header, and one that cannot be read is a problem; an explicitly claimed file that is empty is listed as skipped and one that cannot be looked at is a problem. Still open: a claimed file that is missing is passed over (nothing to replicate), an exec route whose path is outside the globs, and there is still no comparison against the external replicator's own inventory before cutover. | `litestream.go`, `litestream_owners.go` | `TestFleetInventoryNamesWhatItCannotReplicate`, `TestStrictTreeWalkReportsWhatDiscoveryPassesOver` |
@@ -92,7 +92,7 @@ Reports: `OPUS-REVIEW-metered-replicator-review3.md`, `CODEX-FINDINGS-metered-re
| Astra P2-5: an `UPDATE OR REPLACE` that moves a row onto another's rowid deletes that row without its delete trigger and names no watched column. | Fixed: a trigger refuses any change to a route's rowid. | `registry.go` `routes_rowid_immutable` | `TestRoutesRowidIsImmutable` |
| Astra P1-2: `platformOwned(dataDir)` checks the data directory and not its ancestors; a tenant-writable parent could swap the whole directory. | Open, stated. The data directory's ancestors on a box are `/data` and `/`, root's; the check does not establish that. | | |
| Astra P1-3 (rest), Opus (crash): what a process that dies between the change and the rebuild leaves; `Finish` and `Abandon` of an admitted request go to the old owner's recorder; the transition exists only in memory. | Open, stated more exactly: requests admitted before the change and finished before the crash are complete rows on a day nobody held; writing the transition down before the request is sent is the fix, not built. | | |
-| Opus P3-3, Astra P1 (part): a bind mount from the same filesystem has the same device number. | Open, stated: only an operator or systemd can make one. | | |
+| Opus P3-3, Astra P1 (part): a bind mount from the same filesystem has the same device number. | Open, stated: undetected whenever whoever holds mount authority in the replicator's namespace makes one [corrected in review six; the earlier "only an operator or systemd" was too narrow]. | | |
| Astra P2-7: the shutdown tests pass with `store.Close` removed; the opened test does not compare attempts received with attempts recorded; the second look is not exercised through the watch. | Open; the tests are not strengthened in this build. | | |
| Opus: comments claimed more than the code (the link rule, "narrows the window", the margin). | Corrected where the code changed; the rest stated above as open. | | |
@@ -137,7 +137,7 @@ Reports: `OPUS-REVIEW-metered-replicator-review5.md`, `CODEX-FINDINGS-metered-re
| --- | --- | --- | --- |
| Opus P2-1, Astra 2 (P1): the hold for a file swap ran from the start of the look that found it, not from the last look that found every file as it was; a second finding could not widen the first's window. | Fixed: `lastClean` is the start of the last look that found nothing (seeded by the constructor's inventory); a swap is held from before it; a later finding widens the window and holds only the days it adds. Not fixed: requests tracked to completion across midnight, and the crash case, as stated before. | `litestream_metered.go` `lookAgain`, `goStaleFrom` | `TestAFileSwapIsHeldFromTheLastCleanLook` (driven through `lookAgain`, with the registry proof an hour old and the last clean look two days old: the clean look's day is held; a finding within the window adds nothing; one further back widens) |
| Astra 3 (P1), Opus P3-1 in part: a replacement that went stale after inheriting passed on its own window alone; a fleet closed before its predecessor latched an empty inheritance. | Fixed: `staleWindow` is the union of own and inherited; `inheritHold` latches only once the predecessor has closed, and an unlatched fleet answers for its predecessor through `staleWindow`. | `litestream_metered.go` | `TestInheritedWindowIsTheUnion` (B stale after inheriting A: C holds both days; B closed before A: C holds A's later day through B) |
-| Opus P3-1, Astra 5 (P2): `Open` checked one predecessor; `Open` racing `Close` could open a store already torn down. | Fixed: `Open` requires every fleet in the chain it replaces to be closed; `Open` and the one shutdown hold a lifecycle lock, and `Open` after `Close` is `ErrOwnershipStale`. | `litestream_metered.go` | `TestOpenRefusesWhileAnyPredecessorIsOpen`, `TestCloseBlocksEveryCallerUntilTheStoreHasStopped` (a shutdown held open by a hook: a second caller does not return, a replacement cannot open, the fleet does not read as closed, until it finishes) |
+| Opus P3-1, Astra 5 (P2): `Open` checked one predecessor; `Open` racing `Close` could open a store already torn down. | Fixed: `Open` requires every fleet in the chain it replaces to be closed; `Open` and the one shutdown hold a lifecycle lock, and `Open` after `Close` is `ErrOwnershipStale`. [Review eight: untrue from the eighth build to the ninth for a predecessor that had latched; see "Review eight".] | `litestream_metered.go` | `TestOpenRefusesWhileAnyPredecessorIsOpen`, `TestCloseBlocksEveryCallerUntilTheStoreHasStopped` (a shutdown held open by a hook: a second caller does not return, a replacement cannot open, the fleet does not read as closed, until it finishes) |
| Opus P3-3, Astra 4 (P1) in part: a link inside the spelled path above the chain's top was followed, so another tenant's tree was reached under a tenant's route; the platform-made start link resolved through whatever its target passed through. | Fixed: the spelled directory path is resolved one component at a time, following only the platform's links (the link and its parent the platform's, an absolute target with no further link, the target's parent the platform's) and refusing any other; the chain top's ancestry must then equal its resolution. A file nested at any depth beneath a platform-made link is owned by its resolved path (Astra 6). Not fixed: `dataDir`'s own ancestry and the shadow directories (Astra 4): `platformOwned` still checks a directory, not its ancestry; a verified immutable hierarchy is approval H's question. | `litestream_owners.go` `resolvePlatformLinks` | `TestUnitFileChainRefusesALinkAboveIt`, `TestUnitFileThroughAPlatformMadeStateLink` (nested file; a platform link whose target passes through the tenant's link, refused) |
| 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` |
@@ -159,10 +159,10 @@ Reports: `OPUS-REVIEW-metered-replicator-review6.md`, `CODEX-FINDINGS-metered-re
| 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 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`. [Review eight: the eighth build's chain walk stopped at a latched fleet without asking whether it had closed; the ninth build asks.] | `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 6, Opus P3-7: a database under a sidecar name beside a real database was not read. | Fixed: every non-empty regular file not already recognised by its name is read for the header, sidecar names included (an empty file holds nothing and is passed over); 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` | |
@@ -180,12 +180,29 @@ Reports: `OPUS-REVIEW-metered-replicator-review7.md`, `CODEX-FINDINGS-metered-re
| --- | --- | --- | --- |
| Opus P2-1, Astra 3 (P1): an owned file that had vanished was passed over as "nothing is being shipped" and the look counted as clean, so a swap made and removed between looks moved the clean boundary past itself. | Fixed: a vanished owned file is doubt from the last clean look like any other change; the boundary never advances through it. The removal case in the inventory table is `ErrOwnershipStale` now, not drift. | `litestream_metered.go` `lookAgain` | `TestAVanishedFileIsDoubtNotACleanLook` |
| Opus P3-1, Astra 5 (P2): a relative link target was joined, and so cleaned, before it was walked, so a traversal through a tenant's link followed by `..` was folded away. | Fixed: the target is walked from the link's directory component by component as spelled, with no cleaning; the fixture spells its target as a string (a `filepath.Join` would have cleaned the link away before the test began, which is how the seventh build's test missed it) and checks the kernel's own resolution first. | `litestream_owners.go` `walkWithoutLinks` | `TestAPlatformLinksRelativeTargetIsWalkedNotCleaned` |
-| Astra 4 (P2): a fleet between the root of a chain and its last fleet, closed before the chain had closed, never held its own recorders when the chain latched. | Fixed: when a fleet latches, each unlatched fleet between it and the root is given its own predecessor's window on its own recorders, once. | `litestream_metered.go` `inheritHold` | `TestIntermediateFleetsRecordersAreHeldToo` |
-| Opus P3-2: every fleet kept its whole chain of predecessors alive and walked it. | Fixed: a fleet that has latched lets its chain go (`replaces = nil`), and the walks stop at the first latched fleet, whose predecessors were closed when it latched. | `litestream_metered.go` | the inheritance tests (unchanged results) |
+| Astra 4 (P2): a fleet between the root of a chain and its last fleet, closed before the chain had closed, never held its own recorders when the chain latched. | Fixed: when a fleet latches, each unlatched fleet between it and the root is given its own predecessor's window on its own recorders, once. [Review eight: true once the chain has in fact closed; a successor closed over a latched, still open fleet latched through it in the eighth build and let it go empty; the ninth build does not.] | `litestream_metered.go` `inheritHold` | `TestIntermediateFleetsRecordersAreHeldToo` |
+| Opus P3-2: every fleet kept its whole chain of predecessors alive and walked it. | Fixed: a fleet that has latched lets its chain go (`replaces = nil`), and the walks stop at the first latched fleet, whose predecessors were closed when it latched. [Review eight, both reviewers, P1: stopping there skipped that fleet's own closed flag, so a replacement could open beside a latched running fleet; fixed in the ninth build, see "Review eight".] | `litestream_metered.go` | the inheritance tests (unchanged results) |
| Opus P2-2, Astra 8 (P3): the header open can still be steered to a device by a directory swapped higher up (`O_NOFOLLOW` covers the last component), which on a box with a watchdog device is a reboot primitive. | Open, stated plainly now: a walk by directory descriptor (`openat2` with `RESOLVE_BENEATH|RESOLVE_NO_SYMLINKS|RESOLVE_NO_MAGICLINKS`, or `os.Root`) is the fix and is required before this is wired; the `!unix` build now refuses header reads instead of opening by path. | `sqliteopen_other.go`; this file | |
| Opus P3-3, Astra 9 (P3): ownership inferred from the file, not tied to the route; a tenant sharing the platform's uid. | Open, as before, with approval H: tie a unit route to its host's state directory and uid; refuse a platform-uid owner unless the route is the ops account. | | |
| Astra 1, 2 (P1): litestream's own opens by path; `dataDir` and shadow ancestry. | Open, as before. | | |
| Astra 6 (P2): an exec route's claimed file outside the globs is in no list; empty tree files; entries vanishing before their first look; no external-inventory comparison. | Open, stated: the explicit non-hibernating claim outside the globs is a real omission and is the next bounded item; the comparison against the external replicator's inventory stays a cutover requirement. | | |
| Opus P3-4, Astra 7 (P2), Astra 10 (P3): tests that pass without their fixes (retained handles, worker exit, the watch's own call, exact counts, `O_NOFOLLOW` alone); scheduler timing; temp-root names in the seam. | Open in part, as stated; the eighth build adds the vanished-file, intermediate-recorder and relative-target cases, each of which fails without its fix. | | |
-Package tests pass under `/tmp` and under `~/.cache`. The full gate on this tip is next; the eighth build has not been read by a reviewer.
+Package tests pass under `/tmp` and under `~/.cache` (the author's runs; no reviewer ran them). The full gate on this tip is next; the eighth build has not been read by a reviewer.
+
+## Review eight (reviewed `b5aecf13`; Opus rejected, Astra rejected)
+
+Reports: `OPUS-REVIEW-metered-replicator-review8.md`, `CODEX-FINDINGS-metered-replicator-review8.md`. Both found the same P1 in the eighth build's own fix for Opus P3-2 of review seven: the chain walks stopped at the first latched fleet without asking whether that fleet had closed, and after the second build every running fleet is latched, so a replacement could open beside a running or draining fleet, and a fleet closed unopened over one latched through it, took its still-empty window and let it go. The ninth build fixes that and the bounded items below; the deep items stand as before.
+
+| Finding | Disposition | Where | Reproduction |
+| --- | --- | --- | --- |
+| Opus P1-1, Astra 1 (P1): a latched predecessor was taken for a closed one by `Open` and by `inheritHold`. | Fixed: the chain walk (`chainClosed`) reads each fleet's own closed flag first and trusts its latch only for the fleets behind it; the latch itself (`latch`) stores the window, clears the chain and sets the flag under the stale lock in one step, and the walks read the chain under that lock, so no reader sees a latched fleet without its window (the data race Opus names). | `litestream_metered.go` `chainClosed`, `link`, `latch`, `inheritHold`, `Open` | `TestALatchedPredecessorMustStillBeClosed` (C over a latched open B, then a latched draining B, refused; after B closes, opened); `TestAnUnopenedFleetDoesNotLatchThroughALiveLatchedPredecessor` (Astra's sequence: C closed unopened over a live latched B stays unlatched; B's later days reach C's recorder and D's). Both fail with the walk's old order (mutation run). |
+| Opus P3-2: a `..` after a tenant-owned component of a platform link's target was accepted. | Fixed: a `..` is taken only in the target's leading run (systemd's `../private/...`); one after any named component refuses. | `litestream_owners.go` `walkWithoutLinks` | `TestAPlatformLinksRelativeTargetIsWalkedNotCleaned`, second half flipped to a refusal, third half accepts the leading run; fails with the rule removed (mutation run). |
+| Opus P3-3: one tenant's file deletion holds every account's days on the box, every look until the rebuild. | Stated as a known availability cost, in `lookAgain`'s comment and here: a finding on one file holds every recorder the fleet has, because the fleet is one unit of doubt and the inherited window carries no paths. Holding by owner needs the window to carry which paths it is about; not built. The direction is safe (over-holding). | `litestream_metered.go` `lookAgain` | |
+| Opus P3-4, Astra 6 and 8 (tests): nothing pinned `replaces = nil`; the "once" check passed without the intermediate CAS; the clean-start test assumed 100 ms of scheduling; fixtures depended on the umask and on the temp root. | Fixed in part: the latch's effect on the chain is asserted (`link()` after the latch); the once check also covers a second successor of the same fleet (the CAS is gone: the latch under the lock is the once); the clean-start test asserts the look's whole delay fits between the recorded instant and the look's end, and that the hold runs from exactly that instant less the margin, with no bound on scheduling; every fixture directory is made 0755 by an explicit chmod (`mkdirs`) and the "root is open to others" shadow fixture too; `platformDir` resolves its path once. The package passes under `umask 077` and under the race detector (author's runs). Open: the 300 ms wait for a second `Close` caller (a negative check: it cannot fail wrongly, only pass wrongly on a very slow box); `withOwners` keys by base name (the seam takes a `FileInfo`, which carries no path; the fixture names are chosen not to occur in a temp root's components). | tests | |
+| Opus P3-1, Astra 7 (P3): units running as root, or a tenant sharing the platform's uid. | Open, as before, with approval H. | | |
+| Astra 2 (P1), Astra 3 (P1), Astra 5 (P2): litestream's own opens by path (every reopen and WAL open, not the first alone); protected endpoints under tenant-replaceable ancestry; the header open steered by a swapped ancestor. | Open, as before: a walk by directory descriptor and a litestream change; required before wiring. Stated more exactly now in `openRegularNoFollow`'s comment and in the row for Opus P1-2 of review three. | | |
+| Astra 4 (P2): an exec route's claimed file outside the globs is in no list, and the other omissions listed. | Open, as before; the next bounded item after this one. | | |
+| Claims (both lists): `ErrOwnershipStale` "from then on" and its duplicated comment; `Close` "holds nobody's day" and its duplicate; `goStale` "the last proof"; "at most one look's span"; `lookAgain` "gone is ErrInventoryChanged"; "its target is absolute" (twice); "a tenant's link above the chain is caught here"; the strict-walk doc on which files are header-read; the `..` doc; `openRegularNoFollow` "without harmful effects"; the registry witness "only ownership moves"; `platformDir`'s umask sentence; "reports the file by its resolved path"; the dispositions rows named by line (the identity comparison, the bind-mount sentence, "a replacement can never open beside a draining store", "every regular file", "package tests pass"). | Corrected, each in place; the `Replaces` doc now says a fleet's recorders must outlive its `Close` until the chain has latched. | | |
+
+Package tests pass under `/tmp` and under `~/.cache`, under `umask 077`, and under `-race` (the author's runs). The full gate on this tip is next; the ninth build has not been read by a reviewer.
diff --git a/internal/ops/litestream_metered.go b/internal/ops/litestream_metered.go
index b0a21f906..e74648348 100644
--- a/internal/ops/litestream_metered.go
+++ b/internal/ops/litestream_metered.go
@@ -30,13 +30,11 @@ var ErrUnmeterable = errors.New("ops: a replicated database has no owner to mete
// route was added or removed, or changed account, app, database path,
// backing or hibernation), or the registry can no longer be read, or a
// database is no longer the file it was when its ownership was checked.
-// Every replica request is refused from then on, every recorder's day is
-// held, and the store has to be closed and built again. A closed fleet
-// answers the same.
-// ErrOwnershipStale: ownership of the replicated databases can no longer be
-// vouched for. Every admission after the finding is refused; a request
-// admitted before it and sent after it is counted to the owner it was
-// admitted for, and the days it could fall in are held (goStale).
+// Every admission after the finding is refused; a request admitted before
+// it and sent after it is counted to the owner it was admitted for, and
+// the days it could fall in are held (goStale), within the margin that
+// constant states. The store has to be closed and built again. A closed
+// fleet answers the same.
var ErrOwnershipStale = errors.New("ops: ownership of the replicated databases can no longer be vouched for; the replicator must be rebuilt")
// ErrInventoryChanged: the databases on the box are no longer the ones
@@ -106,7 +104,10 @@ type MeteredFleetOptions struct {
// 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.
+ // opening over one shadow directory and one prefix. A fleet's
+ // recorders must outlive its Close until the chain has latched: a
+ // fleet closed before the chain had closed is told its predecessor's
+ // window when a later fleet latches, after its own Close has returned.
Replaces *MeteredFleet
// SyncInterval is litestream's replica sync tick. Zero keeps its
// default.
@@ -272,14 +273,15 @@ func (f *MeteredFleet) witnessUnchanged() (ok bool, err error) {
// the watch, with no request in sight.
//
// The days in doubt run from a margin before the last instant the fleet
-// was shown sound to a margin after the instant the change was found. For
-// a change in the registry that instant is the last proof (the witness).
-// For a change in the files it is the start of the last look at them that
-// found every file as it was (lastClean): a registry proof says nothing
-// about when a file changed, and the look that finds a swap says nothing
-// about how long before it the swap was made, so the hold runs from
-// before the last look that found nothing. A request sent before the change was sent for the owner of
-// that moment and is counted to them. The ones in doubt were admitted
+// was shown sound to a margin after the instant the change was found.
+// Whichever made the finding, the registry or the files, the instant
+// shown sound is the earlier of the last proof (the witness) and the
+// start of the last look at the files that found every one as it was
+// (lastClean): a registry proof says nothing about when a file changed,
+// and the look that finds a swap says nothing about how long before it
+// the swap was made, so the hold runs from before the last look that
+// found nothing. A request sent before the change was sent for the owner
+// of that moment and is counted to them. The ones in doubt were admitted
// before the change and recorded after it; they were admitted no earlier
// than a moment before the change, which is later than that instant less
// the margin, and recorded no later than a moment after the finding
@@ -306,7 +308,10 @@ func (f *MeteredFleet) lastCleanLook() time.Time {
// 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.
+// well. The extra hold is the span since that look began: one look's span
+// when the looks succeed, more after looks that errored, and more again
+// on a fleet opened long after it was built (its first clean look is the
+// constructor's inventory).
func (f *MeteredFleet) goStaleFrom(lookStart, at time.Time) {
sound := time.Unix(0, f.proven.Load())
if !lookStart.IsZero() && lookStart.Before(sound) {
@@ -370,7 +375,7 @@ func (f *MeteredFleet) staleWindow() (from, at time.Time, stale bool) {
f.staleMu.Lock()
own, ownFrom, ownAt := f.stale.Load(), f.staleFrom, f.staleAt
inherited, inhFrom, inhAt := f.inheritedStale, f.inheritedFrom, f.inheritedAt
- latched, replaces := f.inherited.Load(), f.replaces
+ replaces, latched := f.replaces, f.inherited.Load()
f.staleMu.Unlock()
if !latched && replaces != nil {
inhFrom, inhAt, inherited = replaces.staleWindow()
@@ -400,63 +405,100 @@ func later(a, b time.Time) time.Time {
return a
}
+// chainClosed reports whether every fleet this one replaces, however far
+// back, has closed. A fleet that has latched (inheritHold) stands for its
+// own predecessors, which had closed when it latched; it does not stand
+// for itself, so its own closed flag is read before its latch is trusted.
+// A latched fleet that is still open is the one case the two checks tell
+// apart, and it is the ordinary state of a running fleet after the
+// second build.
+func (f *MeteredFleet) chainClosed() bool {
+ for prev := f.predecessor(); prev != nil; {
+ if !prev.closed.Load() {
+ return false
+ }
+ next, latched := prev.link()
+ if latched {
+ break
+ }
+ prev = next
+ }
+ return true
+}
+
+// predecessor and link read the chain under the stale lock: latching
+// clears the link under it (latch), and a successor walking the chain
+// must see either the fleet before the latch, with its predecessor, or
+// after it, with none, never the two halves of it.
+func (f *MeteredFleet) predecessor() *MeteredFleet {
+ prev, _ := f.link()
+ return prev
+}
+
+func (f *MeteredFleet) link() (prev *MeteredFleet, latched bool) {
+ f.staleMu.Lock()
+ defer f.staleMu.Unlock()
+ return f.replaces, f.inherited.Load()
+}
+
+// latch keeps the window the fleet this one replaces held, once, and lets
+// the chain go: what the chain had to say is kept here, so a box rebuilt
+// many times does not keep every fleet it ever had alive behind the
+// current one. It reports whether this call was the one that latched;
+// the window is read from the predecessor under no lock of this fleet's,
+// and stored with the latch flag under the stale lock in one step, so no
+// reader sees a latched fleet without its window.
+func (f *MeteredFleet) latch() (from, at time.Time, stale, first bool) {
+ prev, latched := f.link()
+ if latched || prev == nil {
+ return time.Time{}, time.Time{}, false, false
+ }
+ from, at, stale = prev.staleWindow()
+ f.staleMu.Lock()
+ defer f.staleMu.Unlock()
+ if f.inherited.Load() {
+ return time.Time{}, time.Time{}, false, false
+ }
+ if stale {
+ f.inheritedFrom, f.inheritedAt, f.inheritedStale = from, at, true
+ }
+ f.replaces = nil
+ f.inherited.Store(true)
+ return from, at, stale, true
+}
+
// inheritHold reads, once, the window the fleet this one replaces held,
// tells this fleet's recorders, and keeps it to pass on. It is called at
-// Open and at Close, and takes effect only once the old fleet has closed
-// (closing asks the witness one last time, so whatever the old fleet had
-// to say about its last days it has said by then): a fleet closed before
-// its predecessor does not latch, and answers for its predecessor through
-// staleWindow until a later fleet reads it.
+// Open and at Close, and takes effect only once every fleet in the chain
+// has closed (closing asks the witness one last time, so whatever the old
+// fleet had to say about its last days it has said by then): a fleet
+// closed before 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 {
- 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.inherited.Load() {
- break // its own predecessors were closed when it latched
- }
- if !prev.closed.Load() {
- return
- }
+ if f.predecessor() == nil || !f.chainClosed() {
+ return
}
// The fleets between this one and the root of the chain closed before
// the chain had, and could not latch then: each is given its own
// predecessor's window now, once, so a day in doubt reaches every
- // owner's recorder and not only the last fleet's.
- for prev := f.replaces; prev != nil && prev.replaces != nil; {
- next := prev.replaces
- if prev.inherited.CompareAndSwap(false, true) {
- from, at, stale := next.staleWindow()
- prev.staleMu.Lock()
- if stale {
- prev.inheritedFrom, prev.inheritedAt, prev.inheritedStale = from, at, true
- }
- prev.replaces = nil
- prev.staleMu.Unlock()
- if stale {
- holdDays(prev.recorders, from, at)
- }
+ // owner's recorder and not only the last fleet's. Its recorders are
+ // told after its own Close has returned, so a recorder must outlive
+ // the fleet it was given to until the chain has latched.
+ for prev := f.predecessor(); prev != nil; {
+ next, latched := prev.link()
+ if latched || next == nil {
+ break
+ }
+ if from, at, stale, first := prev.latch(); first && stale {
+ holdDays(prev.recorders, from, at)
}
prev = next
}
- if !f.inherited.CompareAndSwap(false, true) {
- return
- }
- from, at, stale := f.replaces.staleWindow()
- f.staleMu.Lock()
- if stale {
- f.inheritedFrom, f.inheritedAt, f.inheritedStale = from, at, true
- }
- // Latched: what the chain had to say is kept here, and the chain
- // itself is let go, so a box rebuilt many times does not keep every
- // fleet it ever had alive behind the current one.
- f.replaces = nil
- f.staleMu.Unlock()
- if stale {
+ if from, at, stale, first := f.latch(); first && stale {
holdDays(f.recorders, from, at)
}
}
@@ -495,13 +537,11 @@ func (f *MeteredFleet) Open(ctx context.Context) error {
// Every fleet this one replaces, however far back, must have closed:
// a fleet closed without opening passes its predecessor on, and two
// stores over one database would share its shadow state and prefix.
- for prev := f.replaces; prev != nil; prev = prev.replaces {
- if prev.inherited.Load() {
- break // its own predecessors were closed when it latched
- }
- if !prev.closed.Load() {
- return errors.New("ops: the replicator this one replaces has not been closed")
- }
+ // A predecessor that has latched is still asked whether it has closed
+ // (chainClosed): its latch says its own predecessors had closed, not
+ // that it has.
+ if !f.chainClosed() {
+ return errors.New("ops: the replicator this one replaces has not been closed")
}
// The fleet this one replaces has closed, and closing asks the witness
// one last time, so whatever it had to say about its last days it has
@@ -554,9 +594,17 @@ func (f *MeteredFleet) watch() {
// ownership was checked (another inode at its path, or a check that now
// fails: a link where a file was, a second name, a sibling replaced) makes
// the fleet stale, with the hold that goes with it: what has been shipped
-// from that path since can no longer be said to be its owner's. Any other
-// difference (a new database, a new file to be named, one that has gone)
-// is ErrInventoryChanged.
+// from that path since can no longer be said to be its owner's. So does
+// one that has gone: its absence is not proof that nothing was shipped
+// from its path since the last clean look. Any other difference (a new
+// database, a new file to be named) is ErrInventoryChanged.
+//
+// A finding on one file holds every recorder this fleet has, not only
+// the changed file's owner's: the fleet is one unit of doubt and is
+// rebuilt whole. That over-holds the other owners' days (an ordinary
+// deletion by one tenant, found every look until the rebuild, keeps every
+// account's day on the box in doubt); holding by owner needs the window
+// to carry which paths it is about, and is not built.
//
// This is a second look, not a guard. Between two looks a tenant can swap
// a file and swap it back. See checkOwnedFile.
@@ -620,15 +668,17 @@ func (f *MeteredFleet) Databases() int {
return len(f.store.DBs())
}
-// Close stops replication, then the watch, then releases the registry
-// witness, in that order. The fleet answers ErrOwnershipStale afterwards
-// and cannot be opened again. Closing a fleet that was current holds
-// nobody's day: its last requests are admitted and counted like any
-// others, and only when the store has stopped does the gate shut.
-// Close stops replication and releases the witness. It runs once: a second
-// caller, concurrent or later, waits for the first shutdown to finish and
-// gets its result, and the fleet reads as closed only when the store has
-// stopped, so a replacement can never open beside a store still draining.
+// Close stops replication, then the watch, then asks the witness one last
+// time and releases it, in that order. The fleet answers ErrOwnershipStale
+// afterwards and cannot be opened again. Closing a fleet that was current
+// holds a day only if that last reading finds a change, or if the chain
+// it replaces has closed with a window to pass on (inheritHold); its last
+// requests are admitted and counted like any others, and only when the
+// store has stopped does the gate shut. It runs once: a second caller,
+// concurrent or later, waits for the first shutdown to finish and gets its
+// result, and the fleet reads as closed only when the store has stopped,
+// so a replacement cannot open beside a store still draining (Open asks
+// chainClosed, which reads that flag on a latched predecessor too).
func (f *MeteredFleet) Close() error {
f.closeOnce.Do(func() { f.closeErr = f.closeNow() })
return f.closeErr
@@ -715,8 +765,11 @@ func (f *MeteredFleet) closeNow() error {
//
// What that leaves. A request admitted a moment before the change and
// sent a moment after it is counted to the owner it was admitted for; the
-// days it could fall in are held on every recorder (goStale), and on the
-// recorders of the fleet that replaces this one. A process that dies
+// days it could fall in within ownershipMargin of the change are held on
+// every recorder (goStale), and on the recorders of the fleet that
+// replaces this one; one held between admission and recording for longer
+// than the margin (a 23:58 admission recorded after midnight) can fall on
+// a day nobody holds. A process that dies
// between the change and the rebuild takes that hold with it: a request
// it had started and not finished is left as a started row, which the
// ledger holds; one it had finished (recorded) before the change was
diff --git a/internal/ops/litestream_metered_test.go b/internal/ops/litestream_metered_test.go
index 01a8429dd..80c0b2bea 100644
--- a/internal/ops/litestream_metered_test.go
+++ b/internal/ops/litestream_metered_test.go
@@ -781,9 +781,14 @@ func TestMeteredFleetKeepsShadowStateOutOfTenantsReach(t *testing.T) {
}
},
"the root is open to others": func(t *testing.T, dir string) {
+ // The mode set explicitly: a restrictive umask would narrow
+ // Mkdir's and make the fixture sound.
if err := os.Mkdir(filepath.Join(dir, ".replicator"), 0o755); err != nil {
t.Fatal(err)
}
+ if err := os.Chmod(filepath.Join(dir, ".replicator"), 0o755); err != nil {
+ t.Fatal(err)
+ }
},
"a directory beneath the root is open to others": func(t *testing.T, dir string) {
if err := os.MkdirAll(filepath.Join(dir, ".replicator"), 0o700); err != nil {
@@ -1467,16 +1472,23 @@ func TestTheLastCleanLookIsItsStart(t *testing.T) {
t.Fatal(err)
}
defer fleet.Close()
- fleet.lookDelay = 300 * time.Millisecond
+ const delay = 300 * time.Millisecond
+ fleet.lookDelay = delay
before := time.Now()
fleet.lookAgain() // clean, and slow
after := time.Now()
fleet.lookDelay = 0
+ // The start is recorded before the look's delay runs, so the whole
+ // delay fits between the recorded instant and the look's end; an end
+ // recording would leave no room for it. No bound on how long the
+ // scheduler took between before and the look's entry is assumed.
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)) {
+ if recorded.Before(before) || recorded.Add(delay).After(after) {
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.
+ // And a swap found by the next look is held from exactly that start,
+ // less the margin (the proof is later than the look, so the look's
+ // start is the earlier instant).
fleet.proven.Store(time.Now().UnixNano())
swapped := writeFile(t, filepath.Join(platformDir(t), "other.db"))
if err := os.Rename(swapped, db); err != nil {
@@ -1484,8 +1496,8 @@ func TestTheLastCleanLookIsItsStart(t *testing.T) {
}
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)
+ if !stale || !from.Equal(recorded.Add(-ownershipMargin)) {
+ t.Fatalf("held from %v; want the clean look's start %v less the margin", from, recorded)
}
}
@@ -1535,8 +1547,16 @@ func TestIntermediateFleetsRecordersAreHeldToo(t *testing.T) {
if got := bRec.days(); !got["2026-10-02"] {
t.Fatalf("B, the fleet between, holds %v; want A's day on its own recorder", got)
}
- // Once: a second latch by a later fleet adds nothing to B.
+ // Latched fleets let the chain go: neither B nor C reaches A any more.
+ for name, f := range map[string]*MeteredFleet{"B": b, "C": c} {
+ if prev, latched := f.link(); prev != nil || !latched {
+ t.Fatalf("%s after the latch: predecessor %v, latched %v", name, prev, latched)
+ }
+ }
+ // Once: a second successor of B, and a later fleet, add nothing to B.
_, _, before := bRec.counts()
+ c2 := &MeteredFleet{replaces: b, recorders: []lease.RequestRecorder{&countingRecorder{}}}
+ c2.inheritHold()
d := &MeteredFleet{replaces: c, recorders: []lease.RequestRecorder{&countingRecorder{}}}
c.closed.Store(true)
d.inheritHold()
@@ -1545,6 +1565,103 @@ func TestIntermediateFleetsRecordersAreHeldToo(t *testing.T) {
}
}
+// A fleet that has latched stands for its predecessors, not for itself: a
+// replacement built over a latched fleet that is still open, or still
+// draining, cannot open beside it, and a fleet closed unopened over one
+// does not latch through it. (Review eight, both reviewers: the walks
+// broke at the first latched fleet before asking whether it had closed,
+// which after the second build is every running fleet.)
+func TestALatchedPredecessorMustStillBeClosed(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})
+ o := fleetOptions(reg, dir)
+ a, err := MeteredFleetStore(o)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := a.Close(); err != nil {
+ t.Fatal(err)
+ }
+ o.Replaces = a
+ b, err := MeteredFleetStore(o)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer b.Close()
+ if err := b.Open(context.Background()); err != nil {
+ t.Fatal(err)
+ }
+ if prev, latched := b.link(); !latched || prev != nil {
+ t.Fatalf("B opened over a closed A: latched %v, predecessor %v", latched, prev)
+ }
+ o.Replaces = b
+ c, err := MeteredFleetStore(o)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer c.Close()
+ if err := c.Open(context.Background()); err == nil || !strings.Contains(err.Error(), "not been closed") {
+ t.Fatalf("C opened beside B, latched and open: %v", err)
+ }
+ // While B drains, the same.
+ entered, release := make(chan struct{}), make(chan struct{})
+ b.closingHook = func() {
+ close(entered)
+ <-release
+ }
+ closing := make(chan error, 1)
+ go func() { closing <- b.Close() }()
+ <-entered
+ if err := c.Open(context.Background()); err == nil || !strings.Contains(err.Error(), "not been closed") {
+ t.Fatalf("C opened beside B, latched and draining: %v", err)
+ }
+ close(release)
+ if err := <-closing; err != nil {
+ t.Fatal(err)
+ }
+ if err := c.Open(context.Background()); err != nil {
+ t.Fatalf("C after B closed: %v", err)
+ }
+}
+
+// Astra's sequence (review eight): A closes; B replaces A and opens, so B
+// is latched. Ownership moves after B's last clean look; C is built over
+// B for the new owner and closed, unopened, before B notices. B then
+// finds the change, holds its days and closes; D replaces C and opens.
+// C's recorder and D's must hold B's days: C, closed over a latched but
+// open B, must not have latched an empty window and let B go.
+func TestAnUnopenedFleetDoesNotLatchThroughALiveLatchedPredecessor(t *testing.T) {
+ day := func(d int) time.Time { return time.Date(2026, 10, d, 23, 59, 40, 0, time.UTC) }
+ a := &MeteredFleet{}
+ a.closed.Store(true)
+ b := &MeteredFleet{replaces: a, recorders: []lease.RequestRecorder{&countingRecorder{}}}
+ b.inheritHold() // as Open does: A is closed, so B latches, with nothing to hold
+ if prev, latched := b.link(); !latched || prev != nil {
+ t.Fatalf("B over a closed A: latched %v, predecessor %v", latched, prev)
+ }
+ cRec := &countingRecorder{}
+ c := &MeteredFleet{replaces: b, recorders: []lease.RequestRecorder{cRec}}
+ c.inheritHold() // as Close does on an unopened fleet: B is open, so nothing
+ if prev, latched := c.link(); latched || prev != b {
+ t.Fatalf("C closed over a live B: latched %v, predecessor %v; want unlatched, still holding B", latched, prev)
+ }
+ c.closed.Store(true)
+ b.proven.Store(day(2).UnixNano())
+ b.goStale(day(2)) // 23:59:40 plus the margin: the 2nd and the 3rd
+ b.closed.Store(true)
+ dRec := &countingRecorder{}
+ d := &MeteredFleet{replaces: c, recorders: []lease.RequestRecorder{dRec}}
+ d.inheritHold() // as Open does
+ for name, rec := range map[string]*countingRecorder{"C": cRec, "D": dRec} {
+ if got := rec.days(); !got["2026-10-02"] || !got["2026-10-03"] {
+ t.Fatalf("%s holds %v; want B's days, the 2nd and the 3rd", name, got)
+ }
+ }
+}
+
// A file that disappears from its owned path does not advance the clean
// boundary: it is doubt from the last clean look, like a changed file.
func TestAVanishedFileIsDoubtNotACleanLook(t *testing.T) {
diff --git a/internal/ops/litestream_owners.go b/internal/ops/litestream_owners.go
index 2d15208a4..be42576d0 100644
--- a/internal/ops/litestream_owners.go
+++ b/internal/ops/litestream_owners.go
@@ -330,8 +330,9 @@ func checkOwnedFile(root, path string, kind claimKind) (fileFacts, string) {
// a time: a link the platform made (systemd's DynamicUser shape:
// /var/lib/carlos-app/<host> is a root-made link to
// /var/lib/private/...) is followed when the link itself and its
- // parent are the platform's, its target is absolute and holds no
- // further link, and the target's parent is the platform's; the
+ // parent are the platform's, its target (absolute, or relative to
+ // the link's own directory) holds no further link, and the
+ // target's parent is the platform's; the
// chain is then the resolved one, and a file named beneath such a
// link at any depth is owned by its resolved path. A link anywhere
// else in the path, the tenant's or anyone's, is refused.
@@ -385,10 +386,11 @@ func checkOwnedFile(root, path string, kind claimKind) (fileFacts, string) {
return fileFacts{}, "the top of its directory chain is a link or cannot be looked at"
}
// Nothing above the top of the chain is a link either: a link
- // higher in the spelled path would have been resolved above under
- // the platform's rules or refused, so the resolved and spelled
- // paths now agree, and a tenant's link above the chain (one that
- // leads into another tenant's tree) is caught here.
+ // higher in the spelled path was resolved above under the
+ // platform's rules or refused there, so the resolved and spelled
+ // paths agree. This is a second check against a link put in place
+ // between the resolution and now; a tenant's link in a stable
+ // path is refused by resolvePlatformLinks first.
if real, rerr := filepath.EvalSymlinks(root); rerr != nil || filepath.Clean(real) != filepath.Clean(root) {
return fileFacts{}, "a directory above its chain is a link"
}
@@ -488,8 +490,10 @@ const strictTreeMaxDepth = 32
//
// - a database it would not replicate is reported in skipped: one in a
// dot-directory, one deeper than treeDBWalkDepth, one under another
-// name SQLite files go by (.sqlite, .sqlite3, .db3), and a file with no
-// extension that begins with SQLite's header;
+// name SQLite files go by (.sqlite, .sqlite3, .db3), and a file under
+// any other name that begins with SQLite's header (every non-empty
+// regular file not already recognised by its name is read for the
+// header; an empty file is passed over, it holds nothing);
// - a directory that cannot be read, and one deeper than
// strictTreeMaxDepth, is a problem;
// - a link is never followed. One that is named like a database, or that
@@ -571,23 +575,33 @@ func sqliteName(name string) bool {
// walkWithoutLinks walks rel (an absolute or relative path, as spelled)
// from start, one component at a time, looking at each with Lstat: a
-// component that is a link, or cannot be looked at, refuses; ".." steps
-// up only from a directory already looked at. The result is the path the
-// kernel would reach, with no cleaning done before the walk, so a
-// traversal through a link followed by ".." is not folded away.
+// component that is a link, or cannot be looked at, refuses. ".." is
+// allowed only in the target's leading run (systemd's ../private/...),
+// where it steps up from start, a directory already looked at and
+// link-free; a ".." after any named component refuses, because the
+// component before it may be a directory whose owner can later put a
+// link in its place, and every resolution after that goes where they
+// point. The result is the path the kernel would reach, with no cleaning
+// done before the walk, so a traversal through a link followed by ".." is
+// not folded away.
func walkWithoutLinks(start, rel string) (string, string) {
at := filepath.Clean(start)
if filepath.IsAbs(rel) {
at = string(filepath.Separator)
}
+ leading := true
for _, part := range strings.Split(rel, string(filepath.Separator)) {
if part == "" || part == "." {
continue
}
if part == ".." {
+ if !leading {
+ return "", rel + " steps up after a named component"
+ }
at = filepath.Dir(at)
continue
}
+ leading = false
next := filepath.Join(at, part)
fi, err := os.Lstat(next)
if err != nil {
@@ -604,9 +618,10 @@ func walkWithoutLinks(start, rel string) (string, string) {
// resolvePlatformLinks resolves the links the platform made in a spelled
// directory path, one component at a time, and refuses any other link. A
// link is the platform's when the link itself and the directory holding
-// it are the platform's, its target is an absolute path holding no further
-// link, and the target's parent is the platform's. The result is a path
-// with no link in it, or the reason it could not be.
+// it are the platform's, its target (absolute, or relative to the link's
+// directory) holds no further link and no ".." past its leading run, and
+// the target's parent is the platform's. The result is a path with no
+// link in it, or the reason it could not be.
func resolvePlatformLinks(dir string) (string, string) {
dir = filepath.Clean(dir)
if !filepath.IsAbs(dir) {
@@ -641,8 +656,8 @@ func resolvePlatformLinks(dir string) (string, string) {
// a relative target starts from the link's own directory, which is
// link-free by now. The target is walked component by component,
// never cleaned first, so a traversal through a link the tenant
- // controls (choice/../x) is seen for what it is, and ".." steps up
- // only through directories already looked at.
+ // controls (choice/../x) is seen for what it is, and ".." is taken
+ // only in the leading run, before any named component.
start := string(filepath.Separator)
if !filepath.IsAbs(target) {
start = prefix
diff --git a/internal/ops/litestream_owners_test.go b/internal/ops/litestream_owners_test.go
index e886ad2ff..2681433f2 100644
--- a/internal/ops/litestream_owners_test.go
+++ b/internal/ops/litestream_owners_test.go
@@ -12,12 +12,17 @@ import (
)
// platformDir is a temporary directory as the platform's directories are on
-// a box: its owner's, and writable by nobody else. t.TempDir alone follows
-// the umask, which on a developer's machine often leaves the group able to
-// write, and the ownership checks refuse a directory like that.
+// a box: its owner's, 0755, and spelled by a path with no link in it.
+// t.TempDir makes a 0700 directory, and the mode is set here explicitly
+// rather than through MkdirAll, whose mode the umask narrows; the path is
+// resolved once (a temporary root reached through a link would otherwise
+// meet the platform-link rules in every fixture).
func platformDir(t *testing.T) string {
t.Helper()
- dir := t.TempDir()
+ dir, err := filepath.EvalSymlinks(t.TempDir())
+ if err != nil {
+ t.Fatal(err)
+ }
if err := os.Chmod(dir, 0o755); err != nil {
t.Fatal(err)
}
@@ -26,15 +31,45 @@ func platformDir(t *testing.T) string {
func writeFile(t *testing.T, p string) string {
t.Helper()
- if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
+ mkdirs(t, filepath.Dir(p))
+ if err := os.WriteFile(p, []byte("x"), 0o644); err != nil {
t.Fatal(err)
}
- if err := os.WriteFile(p, []byte("x"), 0o644); err != nil {
+ if err := os.Chmod(p, 0o644); err != nil {
t.Fatal(err)
}
return p
}
+// mkdirs makes each directory, and every directory it had to make on the
+// way, 0755 explicitly: MkdirAll's mode is narrowed by the umask, and a
+// fixture that means a directory to be public must not depend on the
+// developer's.
+func mkdirs(t *testing.T, dirs ...string) {
+ t.Helper()
+ for _, d := range dirs {
+ d = filepath.Clean(d)
+ var made []string
+ for p := d; ; p = filepath.Dir(p) {
+ if _, err := os.Lstat(p); err == nil {
+ break
+ }
+ made = append(made, p)
+ if filepath.Dir(p) == p {
+ break
+ }
+ }
+ if err := os.MkdirAll(d, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ for _, p := range made {
+ if err := os.Chmod(p, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ }
+ }
+}
+
func openRegistry(t *testing.T, path string) *registry.Registry {
t.Helper()
reg, err := registry.Open(path)
diff --git a/internal/ops/litestream_owners_unix_test.go b/internal/ops/litestream_owners_unix_test.go
index 7bcf8450c..7444ae4d6 100644
--- a/internal/ops/litestream_owners_unix_test.go
+++ b/internal/ops/litestream_owners_unix_test.go
@@ -44,9 +44,7 @@ func TestUnitFileChainWithATenantOfItsOwn(t *testing.T) {
// Not the test user's own uid, which is the platform's here.
const tenant, other = 44001, 44002
apps := filepath.Join(platformDir(t), "apps")
- if err := os.MkdirAll(apps, 0o755); err != nil {
- t.Fatal(err)
- }
+ mkdirs(t, apps)
withOwners(t, map[string]uint32{"apps": 0, "A": tenant, "data": tenant, "shared": other, "main.db": tenant, "theirs.db": other, "link:data": tenant})
own := writeFile(t, filepath.Join(apps, "A", "data", "main.db"))
if facts, why := checkOwnedFile(filepath.Dir(own), own, tenantFile); why != "" || facts.Ino == 0 {
@@ -88,11 +86,7 @@ func TestUnitFileThroughAPlatformMadeStateLink(t *testing.T) {
top := platformDir(t)
public := filepath.Join(top, "carlos-app")
private := filepath.Join(top, "private", "carlos-app")
- for _, d := range []string{public, private} {
- if err := os.MkdirAll(d, 0o755); err != nil {
- t.Fatal(err)
- }
- }
+ mkdirs(t, public, private)
real := writeFile(t, filepath.Join(private, "keymail.dev", "registry.db"))
link := filepath.Join(public, "keymail.dev")
// Relative, as systemd makes it: ../private/carlos-app/<host>.
@@ -110,7 +104,8 @@ func TestUnitFileThroughAPlatformMadeStateLink(t *testing.T) {
} else if rf, _ := statFacts(realFi); rf.Ino != facts.Ino {
t.Fatalf("the facts are not the resolved file's")
}
- // The whole inventory agrees, and reports the file by its resolved path.
+ // The whole inventory agrees, and reports the file by the path the
+ // route spells; the facts it checked are the resolved file's.
dir := platformDir(t)
reg := openRegistry(t, filepath.Join(dir, "registry.db"))
upsert(t, reg, registry.Route{Host: "keymail.dev", Account: "bac", App: "one", DBPath: spelled, Backing: registry.BackingUnit})
@@ -145,9 +140,7 @@ func TestUnitFileThroughAPlatformMadeStateLink(t *testing.T) {
// tenant controls is refused: the target must hold no further link.
withOwners(t, map[string]uint32{"carlos-app": 0, "private": 0, "keymail.dev": tenant, "registry.db": tenant, "link:keymail.dev": 0, "link:choice": tenant, "hop": tenant})
hop := filepath.Join(top, "hop")
- if err := os.MkdirAll(hop, 0o755); err != nil {
- t.Fatal(err)
- }
+ mkdirs(t, hop)
if err := os.Symlink(filepath.Dir(real), filepath.Join(hop, "choice")); err != nil {
t.Fatal(err)
}
@@ -170,18 +163,12 @@ func TestUnitFileChainRefusesALinkAboveIt(t *testing.T) {
top := platformDir(t)
apps := filepath.Join(top, "apps")
private := filepath.Join(top, "private")
- for _, d := range []string{apps, private} {
- if err := os.MkdirAll(d, 0o755); err != nil {
- t.Fatal(err)
- }
- }
+ mkdirs(t, apps, private)
// 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)
- }
+ mkdirs(t, filepath.Join(apps, "T"))
if err := os.Symlink(private, filepath.Join(apps, "T", "a")); err != nil {
t.Fatal(err)
}
@@ -289,11 +276,7 @@ func TestAPlatformLinksRelativeTargetIsWalkedNotCleaned(t *testing.T) {
public := filepath.Join(top, "public")
srv := filepath.Join(top, "srv")
victims := filepath.Join(top, "victims")
- for _, d := range []string{public, filepath.Join(srv, "T"), filepath.Join(srv, "safe", "A"), filepath.Join(victims, "V", "x", "y"), filepath.Join(victims, "V", "safe", "A")} {
- if err := os.MkdirAll(d, 0o755); err != nil {
- t.Fatal(err)
- }
- }
+ mkdirs(t, public, filepath.Join(srv, "T"), filepath.Join(srv, "safe", "A"), filepath.Join(victims, "V", "x", "y"), filepath.Join(victims, "V", "safe", "A"))
// The lexical destination holds a platform-looking database; the
// physical one, V's.
lexical := writeFile(t, filepath.Join(srv, "safe", "A", "main.db"))
@@ -316,14 +299,31 @@ func TestAPlatformLinksRelativeTargetIsWalkedNotCleaned(t *testing.T) {
if why == "" || !strings.Contains(why, "another link") {
t.Fatalf("a relative target through the tenant's link: %q (lexical %s, physical %s)", why, lexical, physical)
}
- // The same target with no link in it resolves and is accepted.
+ // The same target with no link in it is refused all the same: its ".."
+ // comes after T, a directory T owns and can later swap for a link, and
+ // every resolution after that swap goes where T points. A ".." is
+ // taken only in the target's leading run.
if err := os.Remove(filepath.Join(public, "A")); err != nil {
t.Fatal(err)
}
if err := os.Symlink("../srv/T/../safe/A", filepath.Join(public, "A")); err != nil {
t.Fatal(err)
}
- if _, why := checkOwnedFile(filepath.Dir(spelled), spelled, tenantFile); why != "" {
- t.Fatalf("a relative target with a plain .. : %q", why)
+ if _, why := checkOwnedFile(filepath.Dir(spelled), spelled, tenantFile); !strings.Contains(why, "steps up after a named component") {
+ t.Fatalf("a relative target with .. after a tenant's directory: %q", why)
+ }
+ // And the leading run alone, systemd's shape, is taken.
+ if err := os.Remove(filepath.Join(public, "A")); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Symlink("../srv/safe/A", filepath.Join(public, "A")); err != nil {
+ t.Fatal(err)
+ }
+ if facts, why := checkOwnedFile(filepath.Dir(spelled), spelled, tenantFile); why != "" {
+ t.Fatalf("a relative target with a leading .. only: %q", why)
+ } else if lf, err := os.Lstat(lexical); err != nil {
+ t.Fatal(err)
+ } else if sf, _ := statFacts(lf); sf.Ino != facts.Ino {
+ t.Fatalf("the facts are not the lexical file's (%s)", lexical)
}
}
diff --git a/internal/ops/sqliteopen_unix.go b/internal/ops/sqliteopen_unix.go
index 9ebff8283..367f09de1 100644
--- a/internal/ops/sqliteopen_unix.go
+++ b/internal/ops/sqliteopen_unix.go
@@ -12,8 +12,12 @@ import (
// its last element and without blocking on a special file, and requires
// the opened file to be the regular file that was looked at (same device
// and inode). A tenant who swaps a FIFO, a device or a link into a file's
-// place between the look and the open gets an error, not a hang and not a
-// read of something else.
+// place between the look and the open gets an error and no read of
+// something else; the error comes after the open, though, and O_NOFOLLOW
+// covers the last element only: a directory swapped for a link higher in
+// the path steers the open itself, and a device reached that way has had
+// its open-side effects by the time it is refused. The walk by directory
+// descriptor that closes that is not built (dispositions).
func openRegularNoFollow(path string, looked os.FileInfo) (*os.File, error) {
// O_NOCTTY: a path steered to a terminal (a pty the tenant made) must
// not become this process's controlling terminal, whose hangup would
diff --git a/internal/registry/registry.go b/internal/registry/registry.go
index 673de6221..5bbe8e328 100644
--- a/internal/registry/registry.go
+++ b/internal/registry/registry.go
@@ -578,7 +578,10 @@ var routeOwnershipColumns = []string{"host", "account", "app", "db_path", "backi
// DNS check, a wake or a sleep's status, and the bookkeeping a replicator
// writes into the very database it replicates. A reader that must stop
// when ownership changes, and must not stop otherwise, needs a witness
-// that only ownership moves.
+// that those commits do not move. What moves it is every insert and
+// delete of a route, whether or not that route has a database, and every
+// change to the six columns ownership is read from; a reader stops on
+// some changes that moved no owner, and on none that did.
//
// The counter is moved by triggers, so every writer moves it whatever
// build it is and whichever statement it uses, and a change that is made