amadan

carlos / platform Public

The replicator's fifth build: the fourth review's bounded findings

An admission checks the stale flag again after its witness read, so a
file swap found during the read refuses it. One witness read, with the
retry policy and no trailing sleep, serves admission and the last look at
Close. Close runs once for every caller and publishes closed only when
the store has stopped. A hold inherited from a replaced fleet is kept and
passed on, by a fleet that closed without opening too. A file swap is
held from before the look that found it began. The unit-file chain is
checked from the file's own directory, which must be the owner's; a
platform-made link at that directory (systemd's DynamicUser shape) is
followed to the resolved chain, a tenant-made one refused; owners the
test user cannot create are given through a seam. The header read opens
without following a link or blocking, and reads only the file that was
looked at. A sidecar name is a sidecar only beside its database. A
relative unit path is a problem, a missing unit claim is said, and a file
the walk found is reported when its final look fails. What stays open,
litestream's first open above all, is in the dispositions, review four.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
claude 1b3cc29ed2fb0dad06b751795134e4c9dd253d3a parent 01f2b07
13 files changed, +1043 −78
  • docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review4.md +151 −0
  • docs/superpowers/specs/DISPOSITIONS-metered-replicator.md +27 −1
  • docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review4.md +182 −0
  • internal/ops/filefacts_other.go +1 −1
  • internal/ops/filefacts_unix.go +5 −1
  • internal/ops/litestream.go +21 −3
  • internal/ops/litestream_metered.go +133 −49
  • internal/ops/litestream_metered_test.go +147 −2
  • internal/ops/litestream_owners.go +79 −12
  • internal/ops/litestream_owners_test.go +56 −9
  • internal/ops/litestream_owners_unix_test.go +197 −0
  • internal/ops/sqliteopen_other.go +9 −0
  • internal/ops/sqliteopen_unix.go +35 −0

CI passed — run details

diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review4.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review4.md
new file mode 100644
index 000000000..4424c16a0
--- /dev/null
+++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review4.md
@@ -0,0 +1,151 @@
+All locations below are at **`01f2b072`**. These are source-based assessments of both requested diffs; no files were changed, tests run, or network commands issued.
+
+| Review-three finding | Closed? | Would its test pass without its fix? |
+|---|---|---|
+| **Opus P1-1 / Astra P1-1:** single-file unit ownership | **Partly.** The immediate-directory symlink reproduction is fixed. The inferred chain can still contain another UID’s directory, and ownership is not tied to the route’s expected tenant UID. | **No** if the top-directory link check is removed. **Yes** if climbing or UID enforcement is weakened while that link check remains: the fixture uses one UID throughout. |
+| **Opus P1-2:** substitution during Litestream’s first open | **Open**, correctly identified as potentially persistent. | No regression test covers the substitution. |
+| **Opus P2-1:** replacement built before invalidation loses the hold | **Closed for an ordered, single-caller handover.** `Close` makes the final observation; `Open` inherits afterward. Concurrent shutdown remains unsafe, below. | **No:** removing the final observation or inheritance fails the new “noticed only as it closed” case. **Yes:** removing the inheritance-once guard survives; inheritance is not exercised twice. |
+| **Opus P2-2:** database-free route changes invalidate everyone | **Documented, not narrowed.** This meets the review’s minimum requested disposition. | The existing unrelated-route insertion case deliberately expects invalidation. There is no narrowing fix to test. |
+| **Opus P2-3 / Astra P2-6:** inventory omissions | **Partly.** Missing unit paths, uppercase SQLite extensions, arbitrary-extension headers and explicit empty files receive better handling. Silent omissions remain below. | **No** for removing missing-unit-path or explicit-empty-file reporting. **Yes** for reverting case folding, restricting header checks to extensionless files, or swallowing header errors: the cited strict-walk fixture exercises none of those new cases. |
+| **Opus P2-4:** different owners reuse one lineage | **Closed for different account IDs.** Both remote and shadow keys include account plus path. Renaming a database between accounts does not carry its old shadow directory or remote prefix. This does not create a fresh lineage for every same-account app or instance replacement. | **No** for reverting the complete key change: shadow-path assertions fail. **Yes** for reverting only the remote-prefix change: server assertions identify database basenames, not account prefixes. |
+| **Opus P3-1:** hold ends before a delayed start crosses midnight | **The narrow boundary is fixed; the general timing problem remains.** | **Yes.** The advertised “found just before midnight” test case does not exist. All four timestamp cases pass without the added upper margin. |
+| **Opus P3-2:** one transient observation error permanently invalidates | **Partly.** Admission retries; the final observation in `Close` does not. | No focused retry/error-injection test exists. |
+| **Astra P1-4:** admission succeeds after filesystem invalidation | **Partly.** Invalidation while waiting for the mutex is caught. Invalidation during the observation still permits admission. | No regression test exercises either interleaving. |
+| **Astra P2-5:** raw rowid replacement bypasses generation | **Closed.** The immutable-rowid trigger aborts the statement before its conflict deletion. | **No:** `TestRoutesRowidIsImmutable` requires the named trigger error and both surviving rows. |
+| **Astra P2-7:** shutdown, accounting and watch assertions are insufficient | **Open**, as disclosed. | **Yes:** the relevant assertions still survive removing `store.Close`, omitting complete-count comparisons, or removing the watch’s filesystem-check call. |
+| **Astra P1-2:** shadow-directory ancestors | **Open**, as disclosed. The deployment ancestry is an assumption, not an established property. | Existing shadow tests still pass with ancestors unchecked. |
+| **Astra P1-3:** delayed attempts, filesystem proof boundary and crash gap | **Open.** The latest disposition describes the crash gap more accurately, but does not cover all the live-process timing failures. | Timestamp-only tests pass with those failures present. |
+| **Opus P3-3 / Astra P1-1, part:** same-filesystem bind mounts | **Open.** Device equality cannot detect them. | No relevant mount fixture exists. |
+
+The generation covers the current discovery and attribution columns: `host`, `account`, `app`, `db_path`, `backing` and `hibernate`. Changed-value predicates exclude unchanged updates; status, scheduling, versions, DNS, certificates and Litestream bookkeeping do not advance it. Installed triggers also cover older builds’ ordinary raw statements, and `INSERT OR REPLACE` advances it through insertion. However, “nothing else” is too strong semantically: every route insertion/deletion advances it, including database-free routes, as now disclosed. The host-update case remains absent from the ownership-witness test.
+
+1. **P1 — Litestream can retain a substituted database after every later ownership check passes.**
+
+ **Locations:** `internal/ops/litestream_owners.go:268`, `internal/ops/litestream_metered.go:350`, `third_party/litestream/db.go:1004`, `:1014`, `:1866`.
+
+ Tenant A temporarily substitutes a link to another database during Litestream’s initial SQLite/file opens, then restores the original pathname. Litestream retains the opened handles; `lookAgain` compares the restored pathname against the original inventory and accepts it. WAL access continues by pathname as well.
+
+ The disposition correctly says this can persist indefinitely. Its proposed identity comparison would detect some substitutions afterward, but would not prevent the initial read, upload or checkpoint against the wrong database.
+
+ **Smallest acceptable fix:** preserve verified filesystem authority through the database, SQLite and sibling opens, using a pinned trusted root and refusing links. Comparing the opened identity is useful additional detection; it is not sufficient protection against the initial operation.
+
+2. **P1 — Admission can still return success after the fleet becomes stale.**
+
+ **Locations:** `internal/ops/litestream_metered.go:187`, `:201`, `:214`, `:359`; `internal/lease/requestgate.go:58`.
+
+ An admission acquires `f.mu`, passes the new stale check, and begins its observation or retry delay. Concurrently, `lookAgain` calls `goStale` without that mutex. The registry itself is unchanged, so the observation succeeds. Admission then updates `proven` and returns `nil`, despite the fleet already being stale.
+
+ This is distinct from an admission that completed before the change. Such an already-admitted request can also reach the network afterward: the gate releases control before recording and transport dispatch.
+
+ **Smallest acceptable fix:** serialize the final admission decision with filesystem invalidation under one protocol, including a final stale check before granting admission. Add deterministic cases for invalidation both while waiting for the mutex and while the observation is underway. Handling requests already granted admission additionally requires tracking their lifetime.
+
+3. **P1 — The hold still misses affected dates, independently of the newly added upper margin.**
+
+ **Locations:** `internal/ops/litestream_metered.go:214`, `:230`, `:350`, `:468`; `internal/lease/requestmeter.go:131`.
+
+ A request starts at 23:59:30 and waits before transport dispatch. Other successful observations advance `proven`. Ownership changes after 00:02; the delayed request then sends under the old attribution. The hold begins after midnight, omitting the request’s previous-day start date. The recording budget does not bound that wait.
+
+ There is a separate filesystem case: a slow or delayed inventory walk discovers a substitution made before midnight, while successful registry observations keep advancing `proven` after midnight. A registry observation proves nothing about when the file changed.
+
+ Finally, a request can finish after transfer but before the process records the ownership uncertainty. A crash then loses the in-memory transition. The latest crash disposition acknowledges this; the source comment still describes only unfinished rows.
+
+ **Smallest acceptable fix:** track affected attempts and their dates through completion, retain a separate filesystem proof boundary, and persist transition uncertainty before it can be lost. A fixed minute at either end cannot establish these properties.
+
+4. **P1 — Private shadow directories remain replaceable through unchecked ancestry.**
+
+ **Locations:** `internal/ops/litestream_metered.go:603`, `:625`; `internal/ops/litestream_owners.go:408`; `third_party/litestream/db.go:1064`.
+
+ A root-owned data directory beneath a tenant-writable parent passes `platformOwned`. After construction, the tenant renames it and substitutes another tree. Litestream later resolves the metadata pathname through that replacement and can recreate missing metadata directories using the database directory’s ownership.
+
+ This residual is correctly disclosed for `dataDir`. The same limitation applies to trusting a purported platform-owned directory above a database claim: checking that directory’s final component does not establish its ancestry.
+
+ **Smallest acceptable fix:** establish and pin trusted ancestry, then create and access metadata relative to it. Alternatively, refuse configurations outside an explicitly verified immutable hierarchy.
+
+5. **P2 — The unit-file climb can accept a chain containing another user’s directory.**
+
+ **Locations:** `internal/ops/litestream_owners.go:325`, `:338`, `:359`, `:384`.
+
+ Consider:
+
+ - `/srv/apps` belongs to root;
+ - `/srv/apps/A` belongs to tenant A;
+ - `/srv/apps/A/shared` belongs to B and permits A to create a file;
+ - `/srv/apps/A/shared/main.db` belongs to A.
+
+ The climb starts at `shared`, but examines its **parent’s** UID first. It climbs to `A`, then stops at `/srv/apps`. The downward walk checks links and devices, but not directory UIDs. The final database UID matches `A`, so the B-owned immediate directory is accepted.
+
+ Stable links **within the selected chain** are rejected. The top-directory link check is reached on every accepting `tenantFile` path; earlier returns only reject. The CI fix did not bypass that check.
+
+ When the database UID is root or the process UID, the climb stops at the first platform-owned parent. That removes the CI fixture’s dependence on distant temporary-directory ancestors, but also means that root-owned unit databases receive only this short local ancestry check. A route can name a qualifying root/platform-owned file outside its intended state directory. This is not literally acceptance “from anywhere”: parent permissions, file shape, UID equality, links and devices still constrain it. It nevertheless establishes no isolation for a tenant sharing the platform UID.
+
+ **Smallest acceptable fix:** validate the starting directory’s UID and every directory retained in the tenant chain. Bind claims to a trusted route-specific root/UID rather than inferring tenant authority from the claimed file. Explicitly distinguish trusted platform services from isolated tenants.
+
+6. **P2 — Strict discovery still silently omits real databases and failed inspections.**
+
+ **Locations:** `internal/ops/litestream_owners.go:486`, `:491`; `internal/ops/litestream.go:470`, `:476`.
+
+ A standalone SQLite database named `archive-journal` is ignored solely because of its suffix. No header is read and no `Skipped` entry requires acknowledgment. The same applies to standalone files ending in `-wal` or `-shm`; the code does not establish that they are companions of another database.
+
+ Separately, the strict walk can discover `/state/A/main.db`, then the final `Stat` can fail. Only `/state/A` is present in `claimed`, so `claimed[p]` is false and the discovered file disappears without a problem. The fourth-build fix protects explicit file claims, not every candidate returned by the walk.
+
+ Uppercase names such as `App.DB` are now **reported as skipped**, not replicated. Arbitrary-extension SQLite headers are reported, unreadable directories produce problems, and explicit empty unit files are reported. Missing explicit files, empty tree-discovered files and exec paths outside the globs still disappear as disclosed. The absent external-inventory comparison means these omissions cannot be ruled out at cutover.
+
+ **Smallest acceptable fix:** preserve all discovered candidates through strict final validation; report failed inspection regardless of exact membership in `claimed`. Check headers before excluding presumed companion files. Require an explicit coverage comparison before replacement of the external replicator.
+
+7. **P2 — A tenant can block the inventory watch and shutdown with a file-type substitution.**
+
+ **Locations:** `internal/ops/litestream_owners.go:466`, `:497`, `:525`; `internal/ops/litestream_metered.go:323`, `:422`.
+
+ The walker observes an ordinary file such as `notes.data`. Before `sqliteHeader` opens it, the tenant substitutes a FIFO. `os.Open` can then wait indefinitely for a writer. The single watch goroutine stops checking ownership and files, and `Close` waits indefinitely for it.
+
+ This exceeds the disclosed behavior of refusing an unreadable tree: construction or a running watch can hang instead of returning a named problem.
+
+ **Smallest acceptable fix:** open header candidates without following links or blocking on special files, verify the opened descriptor is regular, and give inventory work a cancellable lifecycle. Keep that guarantee when traversing parent directories too.
+
+8. **P2 — Concurrent `Close` calls can publish completion before the store has stopped.**
+
+ **Locations:** `internal/ops/litestream_metered.go:403`, `:418`, `:440`, `:277`.
+
+ The first caller removes `f.store` and starts the potentially slow `store.Close`. A second caller sees no store, stops the watch, releases the observation and sets `closed=true`. A replacement now passes its `Open` check while the first caller is still draining the previous store.
+
+ Thus the ordered handover fix works, but the `closed` flag is not a reliable completion barrier under concurrent shutdown. Two stores can overlap their use of the same database and shadow state.
+
+ **Smallest acceptable fix:** make shutdown execute once, with all callers waiting for its completion and receiving its result. Publish `closed` only after that operation finishes; serialize lifecycle operations accordingly.
+
+9. **P2 — The tests still do not establish several properties claimed for this build.**
+
+ **Locations:** `internal/ops/litestream_owners_test.go:357`, `:409`; `internal/ops/litestream_metered_test.go:480`, `:685`, `:887`, `:970`, `:1032`.
+
+ The mutation gaps in the opening table are material. In particular, discarding `f.store` and closing the gate satisfies the shutdown assertions even if Litestream monitors remain alive. Waiting for server `inFlight == 0` proves neither complete accounting nor correct per-database attribution. Calling `lookAgain` directly does not establish that the watch invokes it.
+
+ The unit-file fixture uses only the process UID, so the CI correction now keeps its checks inside the fixture but leaves the distinct-tenant climb untested. Permission-denied directory cases are skipped under root. The timestamp fixture is deterministic, but it contains no upper-bound midnight case or delayed real attempt. Integration checks still rely on fixed sleeps, a 500 ms quiet period and a 15-second shutdown threshold.
+
+ **Smallest acceptable fix:** assert retained database handles are closed; compare settled received and recorded attempts per database; trigger filesystem invalidation through the watch; add controlled UID/layout fixtures and synchronized admission/transport tests. Add the specifically advertised timestamp, case-folding and header-error cases.
+
+10. **P3 — Shutdown bypasses the new transient-error policy.**
+
+ **Locations:** `internal/ops/litestream_metered.go:199`, `:206`, `:433`.
+
+ Admission makes **three total attempts**, with five-second contexts, and sleeps 200 ms after every failure—including the final failure. A recovered error does not invalidate the fleet; an observed generation change stops immediately; request cancellation is kept separate from the witness context. Those distinctions are sound.
+
+ This is approximately 15.6 seconds of configured observation time, not a bound on complete admission: mutex acquisition is not cancellable, and pathname inspection does not use the context.
+
+ At shutdown, however, a single transient observation error immediately calls `goStale`, holding the old and replacement fleets’ days despite no ownership change.
+
+ **Smallest acceptable fix:** share the bounded observation/retry policy with the final shutdown observation, skip the final unnecessary sleep, and test recovered error, exhausted errors and witnessed change separately.
+
+The following claims still contradict the code:
+
+- **Dispositions `:84` says the first-open exception is stated in `checkOwnedFile`’s comment.** That comment still says polling “narrows the window” at `litestream_owners.go:273`, without identifying the persistent-open exception.
+- **The whole tenant chain belongs to the tenant** (`litestream_owners.go:237`). The starting directory’s UID can be skipped.
+- **Every other regular file receives a header check** (dispositions `:87`). Companion-looking suffixes are excluded without verifying their role.
+- **“Found just before midnight” is a reproduction** (dispositions `:89`). It is absent from `TestStaleFleetHoldsEveryDayInDoubt`.
+- **Requests are recorded no later than a moment after detection** (`litestream_metered.go:226`). No such scheduling or transport bound exists.
+- **A crash leaves started rows** (`litestream_metered.go:472`). Some affected attempts can already have finished; the later disposition correctly acknowledges this.
+- **A missing pathname means nothing is being shipped** (`litestream_metered.go:356`). Open descriptors and pending shadow files can survive removal.
+- **Deleting and recreating an instance starts a new lineage** (`litestream_metered.go:535`). An unchanged account/path pair reuses the same key; app, instance lifetime and ownership epoch are absent.
+- **Only ownership moves the generation** (`registry.go:580`). Database-free route insertion/deletion also moves it. The dispositions acknowledge this, but the source wording remains broader.
+- **The migration adds three triggers** (dispositions `:75`). It now adds four.
+- **Only an operator or systemd can create an undetected bind mount** (dispositions `:95`). The actual prerequisite is mount authority in the namespace visible to the replicator; a privileged tenant is not excluded by this code.
+
+**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 4a5602cbb..82feabc0d 100644
--- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md
+++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md
@@ -101,4 +101,30 @@ The fourth build is not read by a reviewer. The branch stays unwired and unattes
### CI on the fourth build (b53f2a93)
-Amadan CI failed on `b53f2a93`: `TestFleetDBOwnersNamesAnOwnerOrSaysThereIsNone` and `TestFleetDBOwnersCheckAUnitRoutesNamedFileLikeATree`, with "/ belongs to a user other than root or the platform's". The unit-file chain climb (`checkOwnedFile`, kind `tenantFile`) climbed while each parent belonged to the file's owner; where the file's owner is the platform's own user (every test, a single-user box) that climbs past the fixture to wherever the temp root's chain stops, which on the runner is `/` under a user namespace, and locally depends on `TMPDIR` (it passed under `~/.cache`, where the chain stops at `/home`, and fails under `/tmp`). The local gate on `b53f2a93` (`gate-replicator-b53f2a93.txt`, `TMPDIR` under `/var/tmp/ck`) failed the same way, so the fourth build's "ops tests pass" claim above held only for one temp root. Fixed in the next commit: the climb also stops at the first parent that is the platform's (root's or this process's user's), which is then required to be platform-owned as before. For a tenant whose user is not the platform's nothing changes: the chain still ends at the first directory that is not the tenant's, and that directory must be the platform's. The ops tests now pass under both `/tmp` and `~/.cache`. Rule kept for the morning: a package test must be run under the gate's temp root, not only the sandbox's, before its result is written down.
+Amadan CI failed on `b53f2a93`: `TestFleetDBOwnersNamesAnOwnerOrSaysThereIsNone` and `TestFleetDBOwnersCheckAUnitRoutesNamedFileLikeATree`, with "/ belongs to a user other than root or the platform's". The unit-file chain climb (`checkOwnedFile`, kind `tenantFile`) climbed while each parent belonged to the file's owner; where the file's owner is the platform's own user (every test, a single-user box) that climbs past the fixture to wherever the temp root's chain stops, which on the runner is `/` under a user namespace, and locally depends on `TMPDIR` (it passed under `~/.cache`, where the chain stops at `/home`, and fails under `/tmp`). The local gate on `b53f2a93` (`gate-replicator-b53f2a93.txt`, `TMPDIR` under `/var/tmp/ck`) failed the same way, so the fourth build's "ops tests pass" claim above held only for one temp root. Fixed in the next commit: the climb also stops at the first parent that is the platform's (root's or this process's user's), which is then required to be platform-owned as before. For a tenant whose user is not the platform's nothing changes: the chain still ends at the first directory that is not the tenant's, and that directory must be the platform's. The ops tests now pass under both `/tmp` and `~/.cache`. Rule kept for the morning: a package test must be run under the gate's temp root, not only the sandbox's, before its result is written down. The claim "for a tenant whose user is not the platform's nothing changes" was untested when written; review four said so, and the fifth build tests it through a seam (TestUnitFileChainWithATenantOfItsOwn).
+
+## Review four (reviewed `01f2b072`; Opus accepted with fixes, Astra rejected)
+
+Reports: `OPUS-REVIEW-metered-replicator-review4.md`, `CODEX-FINDINGS-metered-replicator-review4.md`. Both found the third review's bounded findings closed except as noted; Astra's four P1s are the deep ones (litestream's own opens, admission after staleness, the hold's dates, shadow ancestry) and two of them stay open below. This section records the fifth build, which is unreviewed.
+
+| Finding | Disposition | Where | Reproduction |
+| --- | --- | --- | --- |
+| Opus P2-2, Astra 2 (P1): an admission whose witness read overlapped the file check's `goStale` was still admitted. | Fixed in part: the stale flag is checked again after the read, before the proof is advanced and nil returned, so no admission that saw the fleet current before its read and stale after it succeeds. Not fixed: a request admitted before the change and sent after it; that is the hold's job (goStale), as stated, and tracking such requests to completion is not built. | `litestream_metered.go` `admit` | `TestAdmissionIsRefusedWhenTheFleetGoesStaleDuringTheRead` (a witness read held open, the fleet made stale meanwhile, the read released: refused) |
+| Opus P2-3, Astra 7 (P2): the header read opened every regular file by path after an `Lstat`, so a FIFO, a device or a link swapped in would hang the walk, the watch and `Close`. | Fixed: the header read opens with `O_NOFOLLOW|O_NONBLOCK` and reads only the file that was looked at (same device and inode, regular), else reports it; unix only, the CLI's other platforms keep an ordinary open and never run the replicator. | `litestream_owners.go` `sqliteHeader`, `sqliteopen_unix.go` | `TestStrictTreeDoesNotBlockOnAFIFO` (a FIFO in a file's place; a link in a file's place) |
+| Astra 5 (P2): the climb examined each parent's owner but never the file's own directory, so a directory of another user's immediately above the file was accepted. | Fixed: the file's own directory is checked first (the owner's or the platform's, a directory, not a link), then the climb as before; a test with owners the test user cannot create (the `factsOf` seam) covers a tenant of its own, another user's directory in the chain, another user's file, and the tenant's own link. | `litestream_owners.go`; `filefacts_unix.go` (`factsOf` is a variable) | `TestUnitFileChainWithATenantOfItsOwn` |
+| Opus P2-1: a unit route naming a file in a DynamicUser state directory (`/var/lib/carlos-app/<host>`, a root-made link to `/var/lib/private/...`) was always unowned, so the whole fleet refused. | Fixed: a link at the file's own directory is followed when the link, its parent and the resolved directory's parent are all the platform's; the chain is then the resolved one; the file stays owned under the path the route spells. A tenant-made link is refused on its owner. | `litestream_owners.go` | `TestUnitFileThroughAPlatformMadeStateLink`; `TestFleetDBOwnersCheckAUnitRoutesNamedFileLikeATree` (the platform-user shape) |
+| Opus P2-4, Astra 6 (P2) in part: a relative unit `DBPath` and a missing unit claim were dropped in silence; a file the strict walk found that failed its final look vanished. | Fixed: a relative unit path is a problem; a missing unit claim is listed as skipped for the platform's replicator; files the walk found count as claimed for the final look's reporting. Not fixed: there is still no comparison against an external replicator's inventory at cutover (open, as before). | `litestream.go` | `TestFleetInventoryNamesWhatItCannotReplicate` |
+| Opus P3-5, Astra 6 in part: a file named `*-wal`, `*-shm` or `*-journal` was passed over without a header read. | Fixed: such a name is a sidecar only when the database it would belong to is beside it; otherwise it is read like any other file. | `litestream_owners.go` `sidecarOf` | `TestStrictTreeReadsAStandaloneSidecarName` |
+| Astra 8 (P2): concurrent `Close` calls could publish `closed` before the store had stopped. | Fixed: `Close` runs once; every caller waits for the one shutdown and gets its result; `closed` is set inside it after the store has closed. | `litestream_metered.go` `Close` | `TestCloseRunsOnceForEveryCaller` |
+| Opus P3-7, Astra 10 (P3): `Close`'s last reading had no retry; the retry loop slept after its last attempt. | Fixed: one `witnessUnchanged` with the retry policy serves the admission and the last reading; no sleep after the last attempt. | `litestream_metered.go` | the admission tests; no separate case for the last reading's retry (stated) |
+| Opus P3-1: the hold test passed without the upper margin, and the dispositions cited a case that did not exist. | Fixed: the case exists now ("found just before midnight": 23:59:30 proven, 23:59:40 found, holds the next day); the test's comment says what it tests. | `litestream_metered_test.go` | `TestStaleFleetHoldsEveryDayInDoubt` |
+| Opus P3-2, Astra 3 (P1) in part: a file swap was held from the last registry proof, which says nothing about when a file changed. | Fixed: a swap found by the second look is held from before that look began (`goStaleFrom`). Not fixed: a request admitted at 23:59:30 and sent after a change past midnight falls outside the margin, as the constant's comment says; and a request finished before the change was noticed and the process then dying leaves a settled row held by nothing (the constructor's doc now says so). Tracking requests to completion and persisting the transition before it is lost are not built. | `litestream_metered.go` | `TestAFileSwapIsHeldFromBeforeTheLookBegan` |
+| Opus P3-4: an inherited window was not passed on by a fleet closed without ever opening. | Fixed: the window is inherited at Open or at Close, whichever comes first, kept, and passed on by `staleWindow`. | `litestream_metered.go` `inheritHold` | `TestInheritedHoldPassesThroughAFleetNeverOpened` |
+| Opus P3-3: the lineage comment said an instance deleted and remade starts a new lineage. | Corrected: within one account the path is the lineage; across accounts it is new. | comment | none |
+| Opus claims: the first-open comment, `ErrOwnershipStale`'s "from then on", the crash wording, the CI section's untested claim. | Corrected: the first-open swap is described plainly as unseen by both looks; `ErrOwnershipStale` says what is refused and what is held; the crash wording names the settled row held by nothing; the distinct-tenant climb is now tested through the seam. | comments, this file | |
+| Astra 1 (P1): a swap during litestream's first open is retained after every later check. | Open, as before. Needs litestream to open beneath a pinned root without following links and to keep the opened identity; not a change this branch makes. Nothing is attested until it is. | | |
+| Astra 4 (P1): a root-owned directory under a tenant-writable parent passes `platformOwned`; shadow directories reachable through unchecked ancestry. | Open, as before, for `dataDir` and for the directory above a claim: `platformOwned` checks the directory itself, not its ancestry. Requiring a verified immutable hierarchy is a box configuration question (approval H). | | |
+| Opus P3-6: a unit `DBPath` is not tied to its host's state directory. | Open, now stated: operator-set, not tenant-set; requiring `/var/lib/carlos-app/<host>` is a CLI rule to add with approval H. | | |
+| Astra 9 (P2): tests that do not establish several claims (retained handles closed, per-database attribution, the watch invoking the look, fixed sleeps). | Open in part: the owner-chain tests now run with distinct owners; the admission race has a deterministic case; the rest (handles, attribution, the watch's own invocation) are as stated. | | |
+
+Package tests pass under `/tmp` and under `~/.cache` (the CI failure of the fourth build was the lesson). The full gate on this tip is the next step; the fifth build has not been read by a reviewer.
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review4.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review4.md
new file mode 100644
index 000000000..83146720d
--- /dev/null
+++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review4.md
@@ -0,0 +1,182 @@
+# Fourth review: metered replicator at `01f2b072` (Opus)
+
+**Verdict: accept with fixes.** This is unwired code, so none of the findings below misbills anyone today.
+
+- No P1 found.
+- The review-three P1 is closed for a tenant with its own user.
+- Four P2s: one is a regression, and one is an incomplete fix for Astra's P1-4.
+
+Line numbers are at `01f2b072`. Sources were read with `git show` only. Nothing was run or modified, and the scratch copy has been deleted.
+
+## Review three, finding by finding
+
+| Finding | Now | Would its test pass without the fix? |
+|---|---|---|
+| Opus P1-1 / Astra P1 (unit named file) | **Closed** for a tenant whose user is its own, at one instant. The link-in-place-of-`data` scenario is refused (`litestream_owners.go:340` or `:365`). New regression for DynamicUser units (P2-1 below). The variant where a unit `DBPath` is not tied to its host's state directory is neither fixed nor listed as open (P3-6). | **No** for the link at the file's own directory. But the test runs as one user, so the climb, the chain-UID rule and "the directory above the chain is the platform's" are never exercised. |
+| Opus P2-1 (hold handed over at build) | **Closed**: `Close` does a last reading (`:433-438`), and `Open` applies the old window once (`:286-290`). One transitive gap remains (P3-4). | **No.** The "noticed only as it closed" subtest fails without the fix. |
+| Opus P2-2 (every route insert or delete makes the fleet stale) | **Stated**, accurately. | n/a |
+| Opus P2-3 / Astra P2-6 (silent drops) | **Partly.** Fixed: a unit route with no path is reported, an empty claimed file is skipped, and headers are read. Still silent: a relative unit `DBPath`, a missing tree root (P2-4) and `*-wal`-named files (P3-5). | **No** for the no-path and empty cases. **Yes** for case-insensitive matching and for the header read on files with an extension: the fixture's `store` has no extension, so the old code already caught it, and no fixture has an upper-case name. |
+| Opus P2-4 (lineage keyed by path) | **Closed between accounts.** Not within one account (P3-3). | The tests fail only because their paths changed. No test puts two owners on one path. |
+| Opus P3-1 (top of the hold) | **Fixed in code.** | **Yes.** All four cases in `TestStaleFleetHoldsEveryDayInDoubt` (`litestream_metered_test.go:481-487`) pass with `to = at`. The "found just before midnight" case the dispositions cite does not exist. |
+| Opus P3-2 (one transient error) | **Fixed in `admit`.** `Close`'s last reading has no retry (P3-7). | No test. |
+| Astra P1-4 (queued admission) | **Incomplete** (P2-2). | No test. |
+| Astra P2-5 (rowid) | **Closed.** `host` is the only unique key, and a change to `host` fires the update trigger. | **No.** |
+| Astra P2-7 (tests) | **Open, as stated.** | Unchanged. |
+| Astra P1-2, P1-3, finding 6; Opus P1-2, P3-3 | Open and stated. See G. | n/a |
+
+## Answers to A–H
+
+**A. The unit-file chain climb.**
+- **A tenant with its own user:** no link or other user's directory can get a file accepted at the instant of the check.
+ - The climb continues only through parents that are the file owner's and are not links (`:335`).
+ - The top of the chain is always `Lstat`'d (`:340`). That check is reached on every path out of the loop.
+ - Every component from the top down is `Lstat`'d (`:359-366`), and the file's UID must equal the top's (`:384`).
+ - The file's own directory is never UID-checked against the tenant. Exploiting that needs another user's directory inside the tenant's 0700 tree, which gains the tenant nothing.
+- **A tenant whose user is the platform's (root):** the climb stops at the file's directory, so acceptance needs only:
+ - a root-owned file with one name;
+ - a non-link directory;
+ - a grandparent that is the platform's.
+
+ That accepts almost any root file an operator names. No rendered unit runs as root (DynamicUser, or a static `User=` that is never root), so this matters only as operator trust.
+- **A box where units run as root:** the same as above, and nothing more is checked. Under `b53f2a93` such a box refused every unit file.
+- **The real-box problem** is DynamicUser, not root (P2-1).
+
+**B. The request gate and the admission under the lock.**
+- **A request admitted before an ownership change still reaches the network.** This is inherent and is stated.
+- **The stale re-check is incomplete** (P2-2).
+- **The retries are bounded:** 3 × (5 s + 200 ms), held under `f.mu`, so every request can stall about 15.6 s.
+- **Errors and changes are told apart correctly.** Only an error is retried. A replaced registry file shows up as `ok=false` on the retry, and a witnessed `false` is never retried.
+
+**C. The ownership generation.**
+- **Closed.** The six watched columns are what discovery reads.
+- Insert, delete and changed-value update all bump it, through triggers in the schema, so an older build's writes and raw statements are covered too.
+- The rowid trigger is sound, and nothing else bumps the generation.
+
+**D. The hold between fleets.**
+- **No window is lost** for the sequence build, close old, open new.
+- **No reading is taken on a stale fleet.**
+- **The inherited window is applied exactly once** (CAS).
+- Gaps: P3-4 (a replacement that is never opened) and P3-7 (no retry at `Close`).
+
+**E. Owner-keyed lineage.**
+- **Different accounts never share a lineage.**
+- **The same account does.** A host deleted and recreated, or moved between apps, continues its old lineage (P3-3).
+- **A path renamed into another owner's tree** gets a new key, so it does not carry the old owner's lineage.
+
+**F. The strict tree walk.** As in the table, plus P2-3 (the header read follows a swapped-in link or FIFO), P2-4 and P3-5.
+
+**G. The open items.**
+- **Swap during litestream's first open:** stated correctly in the dispositions, but not in the code comment the dispositions cite (claims below).
+- **Ancestors of `dataDir`:** stated too narrowly. Every `platformOwned` call checks only one directory. That covers the chain tops (`/var/lib/carlos-app`), tree-root parents and platform database directories, not only `dataDir`. On a real box these are all root's, so the risk is the same class, not a larger one.
+- **The crash gap:** stated correctly in the dispositions, but the code comment at `litestream_metered.go:471-474` still has the old wording.
+- **Same-filesystem bind mounts:** stated correctly.
+- **No comparison against the external replicator's inventory:** stated correctly.
+- **Omitted:**
+ - the DynamicUser refusal (P2-1);
+ - silent drops of a relative unit `DBPath` and a missing tree root (P2-4);
+ - the lower bound of a hold for a file swap (P3-2), which Astra raised in review three and the dispositions do not carry.
+
+**H. Tests.**
+- **Tests that would pass without their fix:** the P3-1 hold test, and the case-insensitive and header-on-every-file paths.
+- **No test:** the witness retries, the stale re-check, a climb with a real tenant uid, and a DynamicUser state-directory link.
+- **Temp-root dependence:** removed. The platform-user stop means fixtures never climb past the test's own directory. The flip side is that the climb is never tested.
+- **Wall clock:** tests that use real time assert only zero or non-zero hold counts, so they are safe across midnight. `:452` is a sleep-then-negative assertion: weak, not flaky.
+
+## Findings
+
+**P2-1. A unit route naming a file in a DynamicUser state directory is always unowned, so the whole fleet refuses to build.**
+- **Where:** `internal/ops/litestream_owners.go:326-342`; the DynamicUser shape is at `internal/caps/converge.go:668-690`.
+- **Scenario:**
+ 1. A unit route uses the documented shape: `carlos add --backing unit --db /var/lib/carlos-app/keymail.dev/registry.db`.
+ 2. Under DynamicUser, `/var/lib/carlos-app/keymail.dev` is a systemd link to `/var/lib/private/carlos-app/…`.
+ 3. The climb breaks at `/var/lib/carlos-app` (root's), and the top, `…/keymail.dev`, is a link.
+ 4. Result: "the top of its directory chain is a link". That puts the file in `Unowned`, and `MeteredFleetStore` refuses at `litestream_metered.go:526`.
+- **Impact:** it fails closed, but every box with such a tenant is refused. It worked as a `namedFile` at `c8627694`, and no test has the link shape.
+- **Smallest fix:** treat a top that is a link the way `tenantTree` treats its root.
+ - Accept it only when its parent is the platform's and its resolved parent (`EvalSymlinks`) is the platform's.
+ - Then climb and walk on the resolved path.
+ - Better: anchor the chain at the route's own `/var/lib/carlos-app/<host>`, which also closes P3-6.
+ - Add a fixture with the state directory as a platform-made link.
+
+**P2-2. A request whose admission is in its witness read when the file check makes the fleet stale is still admitted. Astra P1-4 is not closed.**
+- **Where:** `litestream_metered.go:187-215`; `goStale` runs without the lock at `:359`.
+- **Scenario:**
+ 1. An admission takes `f.mu`, sees `stale == false` at `:187`, and starts reading the witness.
+ 2. The read takes milliseconds, or up to about 15.6 s with retries.
+ 3. Meanwhile the watch's `lookAgain` finds a database swapped for a link and calls `goStale` without the lock.
+ 4. The read comes back unchanged, and `admit` returns nil.
+ 5. The upload of the swapped path is sent and metered to the old owner after the fleet is stale. This contradicts "every replica request is refused from then on" (`:33`).
+- **Smallest fix:** re-check `f.stale` after the read, before `proven.Store` and `return nil`. Or take `f.mu` around `goStale` in `lookAgain`, with the inventory computed outside the lock. Add a test that blocks a witness read, makes the fleet stale, and then releases the read.
+
+**P2-3. The header read opens every tenant file as root, by path, after an `Lstat`, so a swapped-in FIFO or link hangs the replicator.**
+- **Where:** `litestream_owners.go:466`, `:497`, `:525`.
+- **Scenario:**
+ 1. A tenant loops `rename(fifo-or-link, x.bin)` over a regular file.
+ 2. The walk's `Lstat` sees a regular file, and `os.Open` then opens the FIFO and blocks forever with no writer. A link to a device also opens the device as root.
+ 3. In the constructor, the build hangs.
+ 4. In `lookAgain`, the watch goroutine hangs, ownership ticks stop, and `Close` blocks forever in `f.watching.Wait()` (`litestream_metered.go:422`), so the witness is never released.
+- **Scope:** this existed for files with no extension. The fourth build widened it to every regular file.
+- **Smallest fix:** open with `O_RDONLY|O_NOFOLLOW|O_NONBLOCK`, `fstat` the descriptor, and require a regular file whose device and inode match the `Lstat`. Otherwise report it as a problem.
+
+**P2-4. Two silent drops remain in the strict inventory.**
+- **Where:** `litestream.go:366-368`, `:434-449`, `:470-486`.
+- **Relative unit `DBPath`:** the CLI allows a relative `--db` for `--backing unit` (`cli.go`, the "StateDirectory= makes relative paths real" refusal applies only to non-unit routes). Here it is `Stat`'d against the replicator's cwd, gets `ENOENT`, and is dropped with no problem raised. That tenant is replicated by nobody, and nobody is told.
+- **Missing tree root:** a unit `DBPath` that names a state directory which does not exist yet is likewise dropped. The dispositions say only "a claimed file that is missing".
+- **Smallest fix:**
+ - report a relative unit `DBPath` as a problem (or resolve it against `/var/lib/carlos-app/<host>`);
+ - report an `ENOENT` unit claim as skipped (or as a problem) when the platform's replicator is the caller.
+
+**P3-1. The test for the P3-1 fix passes without it, and the case the dispositions cite does not exist.**
+- **Where:** `litestream_metered_test.go:476-487`.
+- **Detail:** with `to = at`, all four cases still pass. The test's own doc comment still says the hold runs "to the instant it was found changed".
+- **Fix:** add a case where the change is found at 23:59:30, which must hold the next day as well.
+
+**P3-2. For a file swap, the hold's lower bound is the last registry proof, not the last file look, and the fourth build lengthens that gap.**
+- **Where:** `litestream_metered.go:234`, `:345-359`.
+- **Scenario:**
+ 1. A tenant with many files makes each walk slow, since every file is now opened for the header read.
+ 2. A swap made just after the previous look is found more than 90 s later.
+ 3. `from = proven − 1 min` comes after the swap.
+ 4. Requests across midnight in that gap fall on an unheld day.
+- **Fix:** record the start of the last successful file look, and hold from that minus the margin when `lookAgain` makes the fleet stale.
+
+**P3-3. The lineage key is the account, so it does not separate instances within one account.**
+- **Where:** `litestream_metered.go:535-539`, `:580-582`.
+- **Scenario:** an account deletes `a.example.com` and recreates it, or moves it to another app in the same account. The new database continues the old lineage, and the old data can be restored under the new instance.
+- **Fix:** add the app (or an instance id) to the key, or correct the comment.
+
+**P3-4. An inherited window is not passed on transitively.**
+- **Where:** `litestream_metered.go:243-249`, `:286`.
+- **Scenario:**
+ 1. Fleet B is built to replace a stale fleet A.
+ 2. B is closed without ever being opened.
+ 3. C is built with `Replaces: B`.
+ 4. A's window never reaches the new owner's recorder.
+- **Fix:** have `staleWindow` include the window inherited from the fleet it replaced, or apply it at `Close` as well as at `Open`.
+
+**P3-5. A database renamed `*-wal`, `*-shm` or `*-journal` is passed over without a header read.**
+- **Where:** `litestream_owners.go:491`.
+- **Fix:** skip the header read only when the base file exists beside it.
+
+**P3-6. A unit `DBPath` is still not tied to its host's state directory (the P1-1 variant).**
+- **Detail:** this is operator-set (`carlos add --db`), not tenant-set, but it is neither fixed nor listed as open.
+- **Fix:** state it, or require `/var/lib/carlos-app/<host>`.
+
+**P3-7. `Close`'s last reading has no retry.**
+- **Where:** `litestream_metered.go:434-437`.
+- **Scenario:** one busy read at `Close` holds every account's days on this fleet and on its replacement, which is exactly what P3-2's fix was meant to stop.
+- **Fix:** use the same retry loop as `admit`.
+
+## Claims the code contradicts
+
+- **Dispositions, Opus P1-2 row ("Stated in `checkOwnedFile`'s comment … the second look does not narrow this case"):** the comment at `litestream_owners.go:268-276` says the timer "narrows the window" and never mentions a swap during the first open.
+- **Dispositions, Opus P3-1 row:** the reproduction is a case ("found just before midnight") that the test does not contain, and the existing cases pass without the fix.
+- **Dispositions, Astra P1-4 row ("the stale flag is checked again under the lock"):** it is checked before the read, not after (P2-2).
+- **`ErrOwnershipStale` ("every replica request is refused from then on"):** contradicted by P2-2.
+- **`MeteredFleetStore`'s comment at `:535-539`** ("an instance deleted and made again under its name starts a new lineage"): true only when the account differs.
+- **`goStale`'s comment ("admitted no earlier than … the last proof less the margin"):** does not hold for a file swap (P3-2).
+- **`MeteredFleetStore`'s doc at `:471-474` on a crash:** keeps the "started rows, which the ledger holds" wording. The dispositions say it was "stated more exactly", but the code comment was not updated.
+- **`strictTreeDBs` ("passes nothing over in silence"):** contradicted by P3-5 and by the entries it ignores when they vanish. Together with P2-4, the pipeline does not deliver the dispositions' "fixed in part" claim either.
+- **The CI section ("For a tenant whose user is not the platform's nothing changes"):** true as code, but no test runs that path. "The ops tests now pass" says nothing about the climb.
+
+**Verdict: accept with fixes.** Before merging, fix P2-1 through P2-4 and correct the two disposition rows (Opus P1-2 and P3-1). The branch stays unwired and unattested. Opus P1-2 (the first-open swap) still blocks any attestation.
diff --git a/internal/ops/filefacts_other.go b/internal/ops/filefacts_other.go
index fa268986d..d535997a8 100644
--- a/internal/ops/filefacts_other.go
+++ b/internal/ops/filefacts_other.go
@@ -13,4 +13,4 @@ type fileFacts struct {
UID uint32
}
-func factsOf(fs.FileInfo) (fileFacts, bool) { return fileFacts{}, false }
+var factsOf = func(fs.FileInfo) (fileFacts, bool) { return fileFacts{}, false }
diff --git a/internal/ops/filefacts_unix.go b/internal/ops/filefacts_unix.go
index e09b27bf9..78360f3ed 100644
--- a/internal/ops/filefacts_unix.go
+++ b/internal/ops/filefacts_unix.go
@@ -18,7 +18,11 @@ type fileFacts struct {
UID uint32
}
-func factsOf(fi fs.FileInfo) (fileFacts, bool) {
+// factsOf is a variable so a test can give a chain of directories owners
+// the test's own user cannot create.
+var factsOf = statFacts
+
+func statFacts(fi fs.FileInfo) (fileFacts, bool) {
st, ok := fi.Sys().(*syscall.Stat_t)
if !ok {
return fileFacts{}, false
diff --git a/internal/ops/litestream.go b/internal/ops/litestream.go
index 7ddd07979..03fc23633 100644
--- a/internal/ops/litestream.go
+++ b/internal/ops/litestream.go
@@ -363,7 +363,12 @@ func fleetDBsFromRoutes(routes []registry.Route, registryPath, dataDir string, t
// replicator learns where they are. For the platform's own
// replicator a unit route with none is a place a database
// could be that nobody can look at.
- if r.DBPath != "" {
+ if r.DBPath != "" && !filepath.IsAbs(r.DBPath) && tree != nil {
+ // A relative path is relative to nothing this replicator
+ // knows (the unit's own working directory, at best): it
+ // would be looked for from here, not found, and dropped.
+ problems = append(problems, r.Host+": a unit route with a relative database path ("+r.DBPath+")")
+ } else if r.DBPath != "" {
if _, seen := unitPaths[filepath.Clean(r.DBPath)]; !seen {
unitPaths[filepath.Clean(r.DBPath)] = r.Host
}
@@ -431,20 +436,33 @@ func fleetDBsFromRoutes(routes []registry.Route, registryPath, dataDir string, t
// entirely outside dataDir, at their StateDirectory path. A DBPath naming
// a DIRECTORY is a tree — see treeDBs. These are claimed by construction:
// a route named them.
+ discovered := map[string]bool{}
for p, host := range unitPaths {
- if fi, err := os.Stat(p); err == nil && fi.IsDir() {
+ fi, err := os.Stat(p)
+ if err == nil && fi.IsDir() {
if tree == nil {
matches = append(matches, treeDBs(p)...)
continue
}
found, passed, bad := tree(p)
matches = append(matches, found...)
+ for _, f := range found {
+ discovered[filepath.Clean(f)] = true
+ }
skipped = append(skipped, passed...)
for _, b := range bad {
problems = append(problems, b+" (in the state directory of "+host+")")
}
continue
}
+ if tree != nil && errors.Is(err, fs.ErrNotExist) {
+ // A unit route's database (or state directory) that is not
+ // there yet: nothing to replicate today, said, not passed
+ // over, so a tenant whose database appears later is known to
+ // have been unreplicated until the next look.
+ skipped = append(skipped, filepath.Clean(p))
+ continue
+ }
matches = append(matches, p)
}
// The registry at the path it is open at, when that is not the
@@ -473,7 +491,7 @@ func fleetDBsFromRoutes(routes []registry.Route, registryPath, dataDir string, t
// the platform's own replicator an explicitly claimed file in
// that state is said, not passed over (an empty placeholder
// becomes a database without anyone being told).
- if tree != nil && claimed[p] {
+ if tree != nil && (claimed[p] || discovered[p]) {
switch {
case err != nil && !errors.Is(err, fs.ErrNotExist):
problems = append(problems, p+": cannot be looked at")
diff --git a/internal/ops/litestream_metered.go b/internal/ops/litestream_metered.go
index cf41924ed..e91755060 100644
--- a/internal/ops/litestream_metered.go
+++ b/internal/ops/litestream_metered.go
@@ -33,6 +33,10 @@ var ErrUnmeterable = errors.New("ops: a replicated database has no owner to mete
// 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).
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
@@ -133,6 +137,9 @@ type MeteredFleet struct {
mu sync.Mutex
store *litestream.Store
obs *registry.RouteObservation
+ // witness reads the ownership witness; nil means obs.OwnershipUnchanged.
+ // Tests set it to hold a reading open.
+ witness func(context.Context) (bool, error)
stale atomic.Bool
drift atomic.Bool
@@ -142,11 +149,19 @@ type MeteredFleet struct {
staleMu sync.Mutex
staleFrom, staleAt time.Time
+ // inheritedFrom and inheritedAt are the window the fleet this one
+ // replaces held, once read (inherited): passed on again to the fleet
+ // that replaces this one, so a fleet closed without ever opening does
+ // not drop it.
+ inheritedFrom, inheritedAt time.Time
+ inheritedStale bool
watchOnce sync.Once
stopOnce sync.Once
stop chan struct{}
watching sync.WaitGroup
+ closeOnce sync.Once
+ closeErr error
}
// Current reports whether the fleet may go on as it is: nil,
@@ -188,50 +203,82 @@ func (f *MeteredFleet) admit(ctx context.Context) error {
return ErrOwnershipStale
}
asked := time.Now()
- // Not the request's context: a request cancelled while the witness is
- // being read must not be mistaken for a registry that cannot be read.
- // A reading that fails is tried again a few times before it is taken
- // for a change: a busy registry is ordinary, and one error must not
- // stop the box's replication for good. One that keeps failing is not
- // proof of no change, and counts as one.
- var ok bool
- var err error
- for attempt := 0; attempt < witnessAttempts; attempt++ {
- wctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
- ok, err = f.obs.OwnershipUnchanged(wctx)
- cancel()
- if err == nil {
- break
- }
- time.Sleep(witnessRetryDelay)
- }
+ ok, err := f.witnessUnchanged()
if err != nil || !ok {
f.goStale(time.Now())
return ErrOwnershipStale
}
+ // And once more after the read: the second look at the files
+ // (lookAgain) makes the fleet stale without the lock, and may have
+ // done so while the registry was being read. A registry that has not
+ // changed says nothing about a file that has; an admission that saw
+ // the fleet current before the read and stale after it is refused.
+ if f.stale.Load() {
+ return ErrOwnershipStale
+ }
// The instant before the read, not after it: what was shown is that
// nothing had changed by some moment during the read.
f.proven.Store(asked.UnixNano())
return nil
}
+// witnessUnchanged reads the ownership witness, under f.mu. Not the
+// request's context: a request cancelled while the witness is being read
+// must not be mistaken for a registry that cannot be read. A reading that
+// fails is tried again a few times before it is taken for a change: a busy
+// registry is ordinary, and one error must not stop the box's replication
+// for good. One that keeps failing is not proof of no change, and counts
+// as one. The same reading, with the same patience, serves the admission
+// and the last look at Close.
+func (f *MeteredFleet) witnessUnchanged() (ok bool, err error) {
+ read := f.witness
+ if read == nil {
+ read = f.obs.OwnershipUnchanged
+ }
+ for attempt := 0; attempt < witnessAttempts; attempt++ {
+ if attempt > 0 {
+ time.Sleep(witnessRetryDelay)
+ }
+ wctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ ok, err = read(wctx)
+ cancel()
+ if err == nil {
+ return ok, nil
+ }
+ }
+ return false, err
+}
+
// goStale stops the fleet for good and holds the days in doubt on every
// recorder it was given. It does not wait for a request: the caller may be
// the watch, with no request in sight.
//
-// The days in doubt run from a margin before the last instant ownership
-// was shown unchanged to a margin after the instant the change was found.
-// A request sent before the registry changed was sent for the owner of
+// 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 nothing (lookStart): a registry proof says nothing about when
+// a file changed, so a swap found by a slow look is held from before that
+// look began. 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 the last proof
-// less the margin, and recorded no later than a moment after the finding
-// (ownershipMargin).
+// 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
+// (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)
+}
+
+func (f *MeteredFleet) goStaleFrom(lookStart, at time.Time) {
if !f.stale.CompareAndSwap(false, true) {
return
}
- from, to := time.Unix(0, f.proven.Load()).Add(-ownershipMargin), at.Add(ownershipMargin)
+ sound := time.Unix(0, f.proven.Load())
+ if !lookStart.IsZero() && lookStart.Before(sound) {
+ sound = lookStart
+ }
+ from, to := sound.Add(-ownershipMargin), at.Add(ownershipMargin)
f.staleMu.Lock()
f.staleFrom, f.staleAt = from, to
f.staleMu.Unlock()
@@ -239,14 +286,36 @@ func (f *MeteredFleet) goStale(at time.Time) {
}
// staleWindow is the span a stale fleet held, for the fleet that replaces
-// it.
+// it; a fleet that never went stale itself passes on the window it
+// inherited from the one it replaced, so a fleet closed without ever
+// opening does not drop it.
func (f *MeteredFleet) staleWindow() (from, at time.Time, stale bool) {
- if !f.stale.Load() {
- return time.Time{}, time.Time{}, false
- }
f.staleMu.Lock()
defer f.staleMu.Unlock()
- return f.staleFrom, f.staleAt, true
+ if f.stale.Load() {
+ return f.staleFrom, f.staleAt, true
+ }
+ if f.inheritedStale {
+ return f.inheritedFrom, f.inheritedAt, true
+ }
+ return time.Time{}, time.Time{}, false
+}
+
+// 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, whichever comes first, after 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).
+func (f *MeteredFleet) inheritHold() {
+ if f.replaces == nil || !f.inherited.CompareAndSwap(false, true) {
+ return
+ }
+ if from, at, stale := f.replaces.staleWindow(); stale {
+ f.staleMu.Lock()
+ f.inheritedFrom, f.inheritedAt, f.inheritedStale = from, at, true
+ f.staleMu.Unlock()
+ holdDays(f.recorders, from, at)
+ }
}
// holdDays tells every recorder that each UTC day touched by [from, to] is
@@ -283,11 +352,7 @@ func (f *MeteredFleet) Open(ctx context.Context) error {
// database that changed hands has a new owner, whose recorder the old
// fleet never had. Read at build it would be missed whenever the new
// fleet is built before the old one notices.
- if f.replaces != nil && f.inherited.CompareAndSwap(false, true) {
- if from, at, stale := f.replaces.staleWindow(); stale {
- holdDays(f.recorders, from, at)
- }
- }
+ f.inheritHold()
if err := f.admit(ctx); err != nil {
return err
}
@@ -342,6 +407,7 @@ func (f *MeteredFleet) lookAgain() {
if f.closed.Load() || f.stale.Load() {
return
}
+ began := time.Now()
inv, err := fleetInventory(f.routes, f.registryPath, f.dataDir)
if err != nil {
f.drift.Store(true)
@@ -356,7 +422,9 @@ func (f *MeteredFleet) lookAgain() {
// Gone altogether: nothing is being shipped from it.
continue
}
- f.goStale(time.Now())
+ // Held from before this look began: the file may have changed at
+ // any moment since the last look that found it as it was.
+ f.goStaleFrom(began, time.Now())
return
}
if inv.render() != f.inventory {
@@ -394,7 +462,16 @@ func (f *MeteredFleet) Databases() int {
// 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.
func (f *MeteredFleet) Close() error {
+ f.closeOnce.Do(func() { f.closeErr = f.closeNow() })
+ return f.closeErr
+}
+
+func (f *MeteredFleet) closeNow() error {
// The store first, and outside the fleet's lock: its workers ask the
// gate on their way out, and the gate takes the lock. A fleet whose
// witness was released while its store kept running would go on
@@ -429,14 +506,17 @@ func (f *MeteredFleet) Close() error {
// One last reading before the witness is released: a change that
// landed after the last admission, and that no watch tick saw, is
// still this fleet's to hold, and the fleet that replaces it reads the
- // hold at Open.
+ // hold at Open. The reading has the same patience as an admission's:
+ // one busy read must not hold every account's days on this fleet and
+ // its replacement.
if !f.stale.Load() {
- wctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
- if ok, err := f.obs.OwnershipUnchanged(wctx); err != nil || !ok {
+ if ok, err := f.witnessUnchanged(); err != nil || !ok {
f.goStale(time.Now())
}
- cancel()
}
+ // A fleet closed without ever opening still passes on what the fleet
+ // it replaced held.
+ f.inheritHold()
f.closed.Store(true)
err := f.obs.Close()
f.obs = nil
@@ -469,11 +549,13 @@ func (f *MeteredFleet) Close() error {
// 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
-// between the change and the rebuild takes that hold with it: the
-// requests it had started are left as started rows, which the ledger
-// holds, and the new owner's recorder is told nothing. Closing that needs
-// the registry's writer and the replicator to share a lock, or the hold to
-// be written down before the request is sent; neither is built.
+// 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
+// noticed is a settled row, held by nothing; and the new owner's recorder
+// is told nothing. Closing that needs the registry's writer and the
+// replicator to share a lock, or the hold to be written down before the
+// request is sent; neither is built.
//
// The store is returned unopened, and nothing here attests anything. A box
// may say that every replica write was counted only when this is the only
@@ -532,11 +614,13 @@ func MeteredFleetStore(o MeteredFleetOptions) (fleet *MeteredFleet, err error) {
if err := built.admit(context.Background()); err != nil {
return nil, fmt.Errorf("%w (while ownership was being read)", err)
}
- // A replica lineage and its shadow state belong to an owner, not to a
- // path: a path that changes hands (an account moved, an instance
- // deleted and made again under its name) starts a new lineage, so the
- // old owner's data is never restored from under the new one and the
- // old owner's retention and compaction are never billed to the new.
+ // A replica lineage and its shadow state belong to an owner and a
+ // path, not to a path alone: a path that changes account starts a new
+ // lineage, so the old owner's data is never restored from under the
+ // new one and the old owner's retention and compaction are never
+ // billed to the new. Within one account the path is the lineage: an
+ // instance deleted and made again under its name, in the same
+ // account, continues it.
keys := make([]string, 0, len(inv.Owned))
for _, owned := range inv.Owned {
keys = append(keys, ownedReplicaKey(owned))
diff --git a/internal/ops/litestream_metered_test.go b/internal/ops/litestream_metered_test.go
index 3f0c79564..ee61b385e 100644
--- a/internal/ops/litestream_metered_test.go
+++ b/internal/ops/litestream_metered_test.go
@@ -475,8 +475,9 @@ func TestMeteredFleetWatchHoldsEveryRecorderWithoutARequest(t *testing.T) {
}
// The days a stale fleet holds run from a margin before the last instant
-// ownership was shown unchanged to the instant it was found changed, every
-// UTC day in between, on every recorder.
+// ownership was shown unchanged to a margin after the instant it was found
+// changed, every UTC day in between, on every recorder: a change found
+// just before midnight holds the next day too.
func TestStaleFleetHoldsEveryDayInDoubt(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) }
for name, tc := range map[string]struct {
@@ -487,6 +488,7 @@ func TestStaleFleetHoldsEveryDayInDoubt(t *testing.T) {
"found after midnight": {day(2, 23, 59, 50), day(3, 0, 0, 5), []string{"2026-10-02", "2026-10-03"}},
"proven just after midnight": {day(3, 0, 0, 20), day(3, 0, 0, 40), []string{"2026-10-02", "2026-10-03"}},
"a watch that did not run for some days": {day(2, 12, 0, 0), day(4, 12, 0, 0), []string{"2026-10-02", "2026-10-03", "2026-10-04"}},
+ "found just before midnight": {day(2, 23, 59, 30), day(2, 23, 59, 40), []string{"2026-10-02", "2026-10-03"}},
} {
t.Run(name, func(t *testing.T) {
a, b := &countingRecorder{}, &countingRecorder{}
@@ -1039,3 +1041,146 @@ func TestMeteredFleetOpenedReplicatesCountsAndStops(t *testing.T) {
t.Fatal("requests reached the bucket after Close")
}
}
+
+// An admission that saw the fleet current before its reading of the
+// witness, and finds it stale after (the second look at the files made it
+// so while the registry was being read), is refused: a registry that has
+// not changed says nothing about a file that has.
+func TestAdmissionIsRefusedWhenTheFleetGoesStaleDuringTheRead(t *testing.T) {
+ newFakeBucket(t)
+ dir := platformDir(t)
+ reg := openRegistry(t, filepath.Join(dir, "registry.db"))
+ db := writeFile(t, filepath.Join(dir, "a.example.com.db"))
+ upsert(t, reg, registry.Route{Host: "a.example.com", Account: "bac", App: "one", DBPath: db})
+ recs := &recorders{}
+ o := fleetOptions(reg, dir)
+ o.Meter = recs.meter
+ fleet, err := MeteredFleetStore(o)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer fleet.Close()
+ started, release := make(chan struct{}), make(chan struct{})
+ fleet.witness = func(context.Context) (bool, error) {
+ close(started)
+ <-release
+ return true, nil
+ }
+ result := make(chan error, 1)
+ go func() { result <- fleet.admit(context.Background()) }()
+ <-started
+ // The file check finds a swap while the registry is being read.
+ fleet.goStaleFrom(time.Now().Add(-time.Second), time.Now())
+ close(release)
+ if err := <-result; !errors.Is(err, ErrOwnershipStale) {
+ t.Fatalf("an admission whose read the file check overlapped: %v, want ErrOwnershipStale", err)
+ }
+ for path, rec := range recs.all() {
+ if _, _, inc := rec.counts(); inc == 0 {
+ t.Errorf("%s: not told the days in doubt", path)
+ }
+ }
+}
+
+// A file swap is held from before the look that found it began, not from
+// the last registry proof: a registry proof says nothing about when a file
+// changed.
+func TestAFileSwapIsHeldFromBeforeTheLookBegan(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}}
+ // The registry was proved sound after midnight; the look that found
+ // the swap began before it.
+ f.proven.Store(day(3, 0, 0, 30).UnixNano())
+ f.goStaleFrom(day(2, 23, 58, 0), day(3, 0, 1, 0))
+ got := a.days()
+ if !got["2026-10-02"] || !got["2026-10-03"] {
+ t.Fatalf("held %v, want the day the look began and the day the swap was found", got)
+ }
+}
+
+// A hold passes through a fleet that was built to replace a stale one and
+// closed without ever opening: the fleet that replaces that one is told at
+// its Open.
+func TestInheritedHoldPassesThroughAFleetNeverOpened(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)
+ first, err := MeteredFleetStore(o)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := reg.SetAccount("a.example.com", "bae"); err != nil {
+ t.Fatal(err)
+ }
+ if err := first.Current(context.Background()); !errors.Is(err, ErrOwnershipStale) {
+ t.Fatalf("the first fleet after the change: %v", err)
+ }
+ if err := first.Close(); err != nil {
+ t.Fatal(err)
+ }
+ o.Replaces = first
+ second, err := MeteredFleetStore(o)
+ if err != nil {
+ t.Fatal(err)
+ }
+ // Closed without being opened.
+ if err := second.Close(); err != nil {
+ t.Fatal(err)
+ }
+ recs := &recorders{}
+ o.Meter, o.Replaces = recs.meter, second
+ third, err := MeteredFleetStore(o)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer third.Close()
+ if err := third.Open(context.Background()); err != nil {
+ t.Fatal(err)
+ }
+ for path, rec := range recs.all() {
+ if _, _, inc := rec.counts(); inc == 0 {
+ t.Errorf("%s: the hold did not pass through the fleet that never opened", path)
+ }
+ }
+}
+
+// Close runs once: concurrent callers all wait for the one shutdown and
+// get its result, and the fleet reads as closed only once the store has
+// stopped.
+func TestCloseRunsOnceForEveryCaller(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)
+ }
+ if err := fleet.Open(context.Background()); err != nil {
+ t.Fatal(err)
+ }
+ var wg sync.WaitGroup
+ results := make(chan error, 3)
+ for i := 0; i < 3; i++ {
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+ results <- fleet.Close()
+ }()
+ }
+ wg.Wait()
+ close(results)
+ for err := range results {
+ if err != nil {
+ t.Fatalf("Close: %v", err)
+ }
+ }
+ if !fleet.closed.Load() || fleet.store != nil || fleet.obs != nil {
+ t.Fatal("the fleet is not closed after every caller returned")
+ }
+}
diff --git a/internal/ops/litestream_owners.go b/internal/ops/litestream_owners.go
index 4a987e492..86aacd658 100644
--- a/internal/ops/litestream_owners.go
+++ b/internal/ops/litestream_owners.go
@@ -268,12 +268,16 @@ const (
// What it does not establish: any of that a moment later. A tenant who can
// write the directory can replace the file, a sibling or a directory with
// a link after this look, and litestream opens by path afterwards, for the
-// database, for its WAL on every sync, and for its shared memory. The
-// fleet looks again on a timer and stops if a file is no longer the file
-// it was, which narrows the window and does not close it. Closing it needs
-// litestream to open beneath a pinned root without following links
-// (openat2 with RESOLVE_BENEATH and RESOLVE_NO_SYMLINKS), which is a
-// change to litestream and is not made.
+// database, for its WAL on every sync, and for its shared memory. In
+// particular, a swap made between this look and litestream's first open of
+// the database, and swapped back before the next look, is never seen: the
+// opened handles are the other file's for as long as litestream keeps
+// them, and the second look (lookAgain) compares pathnames and finds the
+// original restored. The fleet's second look stops it only when the swap
+// is still in place when the look runs. Closing that needs litestream to
+// open beneath a pinned root without following links (openat2 with
+// RESOLVE_BENEATH and RESOLVE_NO_SYMLINKS) and to keep the opened
+// identity, which is a change to litestream and is not made.
func checkOwnedFile(root, path string, kind claimKind) (fileFacts, string) {
rootInfo, err := os.Stat(root)
if err != nil || !rootInfo.IsDir() {
@@ -322,6 +326,47 @@ func checkOwnedFile(root, path string, kind claimKind) (fileFacts, string) {
if !ok {
return fileFacts{}, "file ownership cannot be read on this platform"
}
+ // The file's own directory first: it is the bottom of the chain
+ // and has to be the owner's (or the platform's) like every other
+ // directory in it. A state directory the platform made as a link
+ // (systemd's DynamicUser shape: /var/lib/carlos-app/<host> is a
+ // root-made link to /var/lib/private/...) is followed when the
+ // link itself, its parent and the resolved directory's parent are
+ // all the platform's; the chain is then the resolved one. A link
+ // the tenant made (its own, in its own directory) is refused.
+ start, serr := os.Lstat(root)
+ if serr != nil {
+ return fileFacts{}, "its directory cannot be looked at"
+ }
+ if start.Mode()&os.ModeSymlink != 0 {
+ if why := platformOwned(filepath.Dir(root)); why != "" {
+ return fileFacts{}, "its directory is a link in a directory that is not the platform's: " + why
+ }
+ if lf, ok := factsOf(start); !ok || (lf.UID != 0 && int(lf.UID) != os.Geteuid()) {
+ return fileFacts{}, "its directory is a link that is not the platform's"
+ }
+ real, rerr := filepath.EvalSymlinks(root)
+ if rerr != nil {
+ return fileFacts{}, "its directory is a link that cannot be resolved"
+ }
+ if why := platformOwned(filepath.Dir(real)); why != "" {
+ return fileFacts{}, "its directory is a link to a directory that is not under the platform's: " + why
+ }
+ root, path = real, filepath.Join(real, filepath.Base(path))
+ if start, serr = os.Lstat(root); serr != nil {
+ return fileFacts{}, "its directory cannot be looked at"
+ }
+ }
+ sf, ok := factsOf(start)
+ if !ok {
+ return fileFacts{}, "file ownership cannot be read on this platform"
+ }
+ if !start.IsDir() || start.Mode()&os.ModeSymlink != 0 {
+ return fileFacts{}, "its directory is not a directory"
+ }
+ if sf.UID != ownerFacts.UID && sf.UID != 0 && int(sf.UID) != os.Geteuid() {
+ return fileFacts{}, "its directory belongs to a user other than its own or the platform's"
+ }
for {
parent := filepath.Dir(root)
if parent == root {
@@ -488,13 +533,16 @@ func strictTreeDBs(root string) (dbs, skipped, problems []string) {
dbs = append(dbs, p)
case sqliteName(name):
skipped = append(skipped, p)
- case strings.HasSuffix(strings.ToLower(name), "-wal") || strings.HasSuffix(strings.ToLower(name), "-shm") || strings.HasSuffix(strings.ToLower(name), "-journal"):
- // A database's own sidecar, which travels with it.
+ 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.
- switch isDB, rerr := sqliteHeader(p); {
+ switch isDB, rerr := sqliteHeader(p, fi); {
case rerr != nil:
problems = append(problems, p+": cannot be read")
case isDB:
@@ -517,12 +565,31 @@ 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; "" otherwise.
+func sidecarOf(p string) string {
+ lower := strings.ToLower(p)
+ for _, suffix := range []string{"-wal", "-shm", "-journal"} {
+ if strings.HasSuffix(lower, suffix) {
+ base := p[:len(p)-len(suffix)]
+ if fi, err := os.Lstat(base); err == nil && fi.Mode().IsRegular() {
+ return base
+ }
+ }
+ }
+ return ""
+}
+
// sqliteHeader reports whether the file begins with SQLite's header. A
// file too short to hold one is not a database; a file that cannot be
// opened or read is an error, which the caller treats as a place it could
-// not look.
-func sqliteHeader(path string) (bool, error) {
- f, err := os.Open(path)
+// not look. It opens without following a link and without blocking (a
+// FIFO put in the file's place would otherwise hold the walk, and with it
+// the watch and Close, for ever), and reads only the file that was looked
+// at: one that changed between the look and the open is an error.
+func sqliteHeader(path string, looked os.FileInfo) (bool, error) {
+ f, err := openRegularNoFollow(path, looked)
if err != nil {
return false, err
}
diff --git a/internal/ops/litestream_owners_test.go b/internal/ops/litestream_owners_test.go
index 78989b2bf..5de9be7a9 100644
--- a/internal/ops/litestream_owners_test.go
+++ b/internal/ops/litestream_owners_test.go
@@ -403,9 +403,10 @@ func TestStrictTreeWalkReportsWhatDiscoveryPassesOver(t *testing.T) {
}
// A unit route that names one file is checked as the tenant's tree would
-// be, from the file's own directory: a link in the directory above the
-// file, which the tenant can make, leads to a database nobody listed, and
-// must not be owned.
+// be, from the file's own directory. The test's own user is the platform's
+// user here, so a link it makes is a platform-made link and is followed;
+// the tenant-made link, which must be refused, needs a tenant of its own
+// (TestUnitFileChainWithATenantOfItsOwn, unix only).
func TestFleetDBOwnersCheckAUnitRoutesNamedFileLikeATree(t *testing.T) {
dir := platformDir(t)
reg := openRegistry(t, filepath.Join(dir, "registry.db"))
@@ -432,9 +433,14 @@ func TestFleetDBOwnersCheckAUnitRoutesNamedFileLikeATree(t *testing.T) {
if err != nil {
t.Fatal(err)
}
- if _, ok := ownedByPath(inv)[own]; ok || !named(inv.Unowned, own, "") {
- t.Fatalf("a named file reached through a link above it: owned %+v, unowned %v", inv.Owned, inv.Unowned)
+ // The link is the platform user's (this process's), in a directory of
+ // the platform user's, to a directory under one of the platform
+ // user's: the DynamicUser shape, followed. The file is then its
+ // resolved self.
+ if o, ok := ownedByPath(inv)[own]; !ok || o.Account != "bac" {
+ t.Fatalf("a platform-made link to the state directory: owned %+v, unowned %v", inv.Owned, inv.Unowned)
}
+ _ = elsewhere
}
// A unit route with no database path, and a claimed file that is missing,
@@ -465,13 +471,54 @@ func TestFleetInventoryNamesWhatItCannotReplicate(t *testing.T) {
if !named(inv.Skipped, empty, "") {
t.Errorf("an empty claimed file is not listed as skipped: %v", inv.Skipped)
}
- // A missing file is nothing to replicate and nothing to look at; a
- // file that appears later is the second look's to report.
- if named(inv.Problems, missing, "") || named(inv.Skipped, missing, "") {
- t.Errorf("a missing claimed file is reported: problems %v skipped %v", inv.Problems, inv.Skipped)
+ // A missing file is nothing to replicate today, and is said: a tenant
+ // whose database is not there yet is known to be unreplicated until
+ // the next look, not passed over.
+ if !named(inv.Skipped, missing, "") {
+ t.Errorf("a missing claimed file is not listed as skipped: skipped %v problems %v", inv.Skipped, inv.Problems)
+ }
+ // A unit route with a relative path is a problem: it is relative to
+ // nothing this replicator knows.
+ upsert(t, reg, registry.Route{Host: "relative.example.com", Account: "bac", App: "four", DBPath: "data/app.db", Backing: registry.BackingUnit})
+ inv, err = FleetDBOwners(reg, dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !named(inv.Problems, "relative.example.com", "relative database path") {
+ t.Errorf("a relative unit path is not a problem: %v", inv.Problems)
}
// The config generator's own discovery is unchanged by any of this.
if dbs, err := FleetDBs(reg, dir); err != nil || len(dbs) != 1 {
t.Fatalf("FleetDBs: %v %v", dbs, err)
}
}
+
+// A file named like a database's sidecar (-wal, -shm, -journal) is a
+// sidecar only when the database it would belong to is beside it; on its
+// own it is any other file, read for its header, and a database under
+// such a name is reported.
+func TestStrictTreeReadsAStandaloneSidecarName(t *testing.T) {
+ root := platformDir(t)
+ header := append([]byte("SQLite format 3\x00"), make([]byte, 100)...)
+ alone := filepath.Join(root, "archive-journal")
+ if err := os.WriteFile(alone, header, 0o644); err != nil {
+ t.Fatal(err)
+ }
+ beside := filepath.Join(root, "live.db")
+ if err := os.WriteFile(beside, header, 0o644); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(beside+"-wal", header, 0o644); err != nil {
+ t.Fatal(err)
+ }
+ dbs, skipped, problems := strictTreeDBs(root)
+ if len(dbs) != 1 || dbs[0] != beside {
+ t.Fatalf("dbs = %v", dbs)
+ }
+ if !named(skipped, alone, "") {
+ t.Fatalf("a database named archive-journal with no archive beside it is not reported: skipped %v problems %v", skipped, problems)
+ }
+ if named(skipped, beside+"-wal", "") || named(problems, beside+"-wal", "") {
+ t.Fatalf("a real sidecar is 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
new file mode 100644
index 000000000..6efe6d4f7
--- /dev/null
+++ b/internal/ops/litestream_owners_unix_test.go
@@ -0,0 +1,197 @@
+//go:build unix
+
+package ops
+
+import (
+ "os"
+ "path/filepath"
+ "strings"
+ "syscall"
+ "testing"
+ "time"
+
+ "github.com/carlosframework/platform/internal/registry"
+)
+
+// withOwners gives files owners by name for one test, through the factsOf
+// seam: a test cannot chown, and the climb's rules are about whose a
+// directory is. A link and a directory of the same name are told apart by
+// mode. Names not listed keep their real owner.
+func withOwners(t *testing.T, uids map[string]uint32) {
+ t.Helper()
+ real := factsOf
+ factsOf = func(fi os.FileInfo) (fileFacts, bool) {
+ f, ok := statFacts(fi)
+ key := fi.Name()
+ if fi.Mode()&os.ModeSymlink != 0 {
+ key = "link:" + key
+ }
+ if uid, set := uids[key]; set {
+ f.UID = uid
+ }
+ return f, ok
+ }
+ t.Cleanup(func() { factsOf = real })
+}
+
+// A unit tenant with a user of its own: its file's chain must be its own
+// from the file's directory to the top, every directory included, and the
+// directory above the top must be the platform's. A directory of another
+// user's inside the chain, a link the tenant made, and a file that is not
+// the tenant's are each refused; a platform-made link to the state
+// directory (systemd's DynamicUser shape) is followed.
+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)
+ }
+ 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 {
+ t.Fatalf("the tenant's own chain: %q", why)
+ }
+ // Another user's directory immediately above the file (Astra 5).
+ inShared := writeFile(t, filepath.Join(apps, "A", "shared", "main.db"))
+ if _, why := checkOwnedFile(filepath.Dir(inShared), inShared, tenantFile); !strings.Contains(why, "belongs to a user other than") {
+ t.Fatalf("a file in another user's directory: %q", why)
+ }
+ // A file of another user's in the tenant's chain.
+ theirs := writeFile(t, filepath.Join(apps, "A", "data", "theirs.db"))
+ // Refused at the chain's bottom: the directory is not the file's
+ // owner's (the chain is the file's owner's, and this file's owner is
+ // not the tenant).
+ if _, why := checkOwnedFile(filepath.Dir(theirs), theirs, tenantFile); !strings.Contains(why, "belongs to a user other than") {
+ t.Fatalf("another user's file: %q", why)
+ }
+ // The tenant swaps its data directory for a link to a directory
+ // elsewhere: a link the tenant owns, in a directory the tenant owns.
+ elsewhere := writeFile(t, filepath.Join(platformDir(t), "not-listed", "main.db"))
+ if err := os.Rename(filepath.Join(apps, "A", "data"), filepath.Join(apps, "A", "data-was")); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Symlink(filepath.Dir(elsewhere), filepath.Join(apps, "A", "data")); err != nil {
+ t.Fatal(err)
+ }
+ if _, why := checkOwnedFile(filepath.Dir(own), own, tenantFile); !strings.Contains(why, "link") {
+ t.Fatalf("the tenant's link above its file: %q", why)
+ }
+}
+
+// systemd's DynamicUser shape: /var/lib/carlos-app/<host> is a root-made
+// link to /var/lib/private/carlos-app/<host>, which the dynamic user owns.
+// A unit route naming a file through the link is owned, by its resolved
+// path; the same shape with the link made by the tenant is not.
+func TestUnitFileThroughAPlatformMadeStateLink(t *testing.T) {
+ const tenant = 44001
+ 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)
+ }
+ }
+ 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 {
+ t.Fatal(err)
+ }
+ withOwners(t, map[string]uint32{"carlos-app": 0, "private": 0, "keymail.dev": tenant, "registry.db": tenant, "link:keymail.dev": 0})
+ spelled := filepath.Join(link, "registry.db")
+ facts, why := checkOwnedFile(filepath.Dir(spelled), spelled, tenantFile)
+ if why != "" {
+ t.Fatalf("the DynamicUser shape: %q", why)
+ }
+ if realFi, err := os.Lstat(real); err != nil {
+ t.Fatal(err)
+ } 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.
+ 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})
+ inv, err := FleetDBOwners(reg, dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ // Owned under the path the route spells (which is what litestream
+ // opens); the facts are the resolved file's.
+ if o, ok := ownedByPath(inv)[spelled]; !ok || o.Account != "bac" {
+ t.Fatalf("through the platform's link: owned %+v, unowned %v", inv.Owned, inv.Unowned)
+ }
+ // The same link made by the tenant is refused.
+ withOwners(t, map[string]uint32{"carlos-app": 0, "private": 0, "keymail.dev": tenant, "registry.db": tenant, "link:keymail.dev": tenant})
+ if _, why := checkOwnedFile(filepath.Dir(spelled), spelled, tenantFile); !strings.Contains(why, "link that is not the platform's") {
+ t.Fatalf("the tenant's own link in the platform's directory: %q", why)
+ }
+}
+
+// A FIFO put where a file was looked at does not hold the walk: the header
+// read opens without blocking and reports the file as one it could not
+// read.
+func TestStrictTreeDoesNotBlockOnAFIFO(t *testing.T) {
+ root := platformDir(t)
+ fifo := filepath.Join(root, "notes.data")
+ if err := syscall.Mkfifo(fifo, 0o644); err != nil {
+ t.Skip("no fifo here:", err)
+ }
+ done := make(chan []string, 1)
+ go func() {
+ _, _, problems := strictTreeDBs(root)
+ done <- problems
+ }()
+ select {
+ case problems := <-done:
+ // A FIFO is not a regular file and is passed over by mode before
+ // any open; the point is that the walk returned. A regular file
+ // swapped for a FIFO between the look and the open is the open's
+ // own refusal (openRegularNoFollow), exercised below.
+ _ = problems
+ case <-time.After(10 * time.Second):
+ t.Fatal("the walk blocked on a FIFO")
+ }
+ regular := filepath.Join(root, "swap.data")
+ if err := os.WriteFile(regular, []byte("not a database, long enough to read"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ looked, err := os.Lstat(regular)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Remove(regular); err != nil {
+ t.Fatal(err)
+ }
+ if err := syscall.Mkfifo(regular, 0o644); err != nil {
+ t.Fatal(err)
+ }
+ finished := make(chan error, 1)
+ go func() {
+ _, err := sqliteHeader(regular, looked)
+ finished <- err
+ }()
+ select {
+ case err := <-finished:
+ if err == nil {
+ t.Fatal("a FIFO in a file's place was read as the file")
+ }
+ case <-time.After(10 * time.Second):
+ t.Fatal("the header read blocked on a FIFO")
+ }
+ // And a link swapped into the file's place is not followed.
+ if err := os.Remove(regular); err != nil {
+ t.Fatal(err)
+ }
+ target := filepath.Join(root, "target.data")
+ if err := os.WriteFile(target, []byte("SQLite format 3\x00 and more bytes after it"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Symlink(target, regular); err != nil {
+ t.Fatal(err)
+ }
+ if isDB, err := sqliteHeader(regular, looked); err == nil || isDB {
+ t.Fatalf("a link in a file's place was followed: %v %v", isDB, err)
+ }
+}
diff --git a/internal/ops/sqliteopen_other.go b/internal/ops/sqliteopen_other.go
new file mode 100644
index 000000000..c9ce8000c
--- /dev/null
+++ b/internal/ops/sqliteopen_other.go
@@ -0,0 +1,9 @@
+//go:build !unix
+
+package ops
+
+import "os"
+
+// openRegularNoFollow: see sqliteopen_unix.go. The replicator never runs
+// where this is built; the ordinary open keeps the CLI compiling.
+func openRegularNoFollow(path string, _ os.FileInfo) (*os.File, error) { return os.Open(path) }
diff --git a/internal/ops/sqliteopen_unix.go b/internal/ops/sqliteopen_unix.go
new file mode 100644
index 000000000..eab6f1761
--- /dev/null
+++ b/internal/ops/sqliteopen_unix.go
@@ -0,0 +1,35 @@
+//go:build unix
+
+package ops
+
+import (
+ "errors"
+ "os"
+ "syscall"
+)
+
+// openRegularNoFollow opens path for reading without following a link in
+// 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.
+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)
+ if err != nil {
+ return nil, &os.PathError{Op: "open", Path: path, Err: err}
+ }
+ f := os.NewFile(uintptr(fd), path)
+ fi, err := f.Stat()
+ if err != nil {
+ f.Close()
+ return nil, err
+ }
+ want, ok1 := factsOf(looked)
+ got, ok2 := factsOf(fi)
+ if !fi.Mode().IsRegular() || !ok1 || !ok2 || want.Dev != got.Dev || want.Ino != got.Ino {
+ f.Close()
+ return nil, &os.PathError{Op: "open", Path: path, Err: errors.New("not the regular file that was looked at")}
+ }
+ return f, nil
+}