amadan
Record the replicator's second review: rejected again, the ownership fence does not hold as built; Close now stops the store
Both reviewers rejected f4fe6f2b. The witness added after the first review is moved by the replicator's own bookkeeping in the registry it replicates, and a refused Start does not stop the request: the request meter sends anyway. Both need changes outside this package and are recorded as open, with a note on the code that describes the intent. Fixed here: Close stops the litestream store before it releases the witness, so a rebuild does not leave two replicators over one shadow state. Nothing calls this code. Nothing attests.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5 files changed,
+414
−6
docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review2.md+132 −0docs/superpowers/specs/DISPOSITIONS-metered-replicator.md+22 −0docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review2.md+184 −0internal/ops/litestream_metered.go+45 −6internal/ops/litestream_metered_test.go+31 −0
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review2.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review2.md| new file mode 100644 |
| index 000000000..937c2e619 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review2.md |
| @@ -0,0 +1,132 @@ |
| +| Disposition at `f4fe6f2b` | Closed? | Would its test pass without the claimed fix? | |
| +|---|---|---| |
| +| Ownership changes invalidate the recorder | **No.** The wrapper detects ordinary registry commits, but its error does not prevent S3 requests. Requests past the check can still be counted to the old owner. Registry-file replacement is unwitnessed. | Removing the `Start` check or ignoring the witness would fail `TestMeteredFleetStopsCountingWhenTheRegistryChanges`. **Giving clients `inner` while retaining wrappers in `fleet.recorders` would pass:** the test calls that map directly. | |
| +| File ownership, duplicate detection and shadow-state isolation | **Partly.** Stable symlinks beneath the claim root are rejected; `NewMeteredStore` catches two listed aliases with `os.SameFile`. Neither establishes ownership throughout use. Relocated metadata can still become tenant-owned. | Removing the symlink check, resolved-path duplicate check, or store identity check would fail their respective cases. Removing path normalization alone would pass the already-clean duplicate fixture. The metadata assertion checks its pathname, not ownership or permissions. | |
| +| Unreadable-directory discovery | **Partly.** Persistent read failures in visited unit directories refuse construction. Other omissions remain, and valid single-file unit paths now fail. | Removing the refusal would fail `TestMeteredFleetStoreRefusesAStateDirectoryItCannotRead` when run without root privileges. Under root it is **skipped**, not evidence. | |
| +| Unknown databases require acceptance; registry recognized at its actual path | **Partly.** Returned `skipped` files require acceptance. Files discovery never returns remain invisible. Actual-registry recognition is added after discovery has already discarded it. | Removing the unclaimed-file refusal would fail the named test. Removing `reg.Path()` recognition would **pass**: its fixture uses `registry.db`. | |
| +| Returned fleet cannot register unmetered clients | **Yes, at the fleet API boundary.** The mutable store is private. The lower-level constructor remains unrestricted, as disclosed. The wrapper introduces a separate shutdown defect. | No test establishes this boundary. Making the store publicly accessible would leave the behavior tests passing. | |
| +| Platform identities accepted by the ledger | **Yes, for attribution validity.** Both identities satisfy the real validator with `PurposePlatform`. | Blanking the registry identity would fail. Blanking only the **console** identity would **pass**: the fixture creates no console database. It calls `Validate`, not ledger `Bind` or replication. | |
| +| Seven-day recovery window | **Yes for configuration; runtime retention remains unproved**, as disclosed. | Removing the retention assignment would fail the assertion. Removing only the interval assignment would **pass**, because Litestream’s default is already 24 hours. | |
| +| Two non-atomic registry reads, described as closed in the mixed P3 row | **Closed for ordinary commits to the same registry database; not for replacement of that database.** The reads themselves remain separate. | Removing the constructor’s post-read check would **pass** the supplied tests: none changes the registry during construction. | |
| + |
| +These are source-based mutation assessments, not executed results. All locations below refer to `f4fe6f2b`. No files were modified, tests run, or network commands issued. |
| + |
| +1. **P1 — Relocating shadow state does not make it platform-owned.** |
| + `internal/ops/litestream_metered.go:227–230`; `internal/lease/meteredstore.go:111–116`. |
| + |
| + The constructor supplies a pathname but neither creates nor protects the metadata tree. Litestream obtains `dirInfo` from the database’s parent and calls `internal.MkdirAll(db.metaPath, db.dirInfo)` (`third_party/litestream/db.go:995–999,1063–1065`). That helper recursively creates missing directories with the source directory’s mode and UID/GID (`third_party/litestream/internal/internal.go:97–146`). |
| + |
| + On a root-run replicator with a searchable data directory, a tenant database can therefore cause its metadata directory—and potentially previously absent shared ancestors—to become tenant-owned. The tenant can alter shadow files or replace descendants with symlinks that subsequent root operations follow. Moving the pathname has not removed the original authority problem. |
| + |
| + **Smallest acceptable fix:** establish and verify a platform-owned, tenant-inaccessible metadata root before opening any database; ensure Litestream never derives its ownership from tenant directories. Refuse unsafe existing components and test actual filesystem permissions after initialization. |
| + |
| +2. **P1 — The file checks still accept another owner’s file.** |
| + `internal/ops/litestream_owners.go:83–100,117–139,172–185`; `internal/lease/meteredstore.go:82–89`. |
| + |
| + `noLinkBeneath` establishes only that each separately inspected component **after** `root` was not a symbolic link when `Lstat` examined it. It does not inspect the root or its parents, reject hard links or bind mounts, pin directories against rename, or bind the result to the file Litestream subsequently opens. |
| + |
| + A pre-existing hard link or bind-mounted alias beneath A’s directory can reach B’s database. If B’s original path is excluded from discovery—for example, beneath a dot directory or owned by a hibernating exec route—there is only one listed file, so `os.SameFile` has nothing to compare against. A claim root or ancestor pointing into B’s tree has the same problem. Hard-link creation requires appropriate filesystem permissions; bind mounts require mount authority, but neither condition is checked or excluded here. |
| + |
| + There are also replacement windows **during** the component walk, between ownership checking and store construction, and between construction and Litestream’s deferred open. After initialization, its database descriptor remains open, but WAL accesses still resolve pathnames (`third_party/litestream/db.go:1013,1866`), so replacing a directory can separate the database and WAL identities. None of these filesystem changes moves the registry witness. |
| + |
| + **Smallest acceptable fix:** bind ownership to verified file identities beneath pinned, trusted roots, and preserve that binding through database and WAL opens. Refuse aliases or mount transitions whose ownership cannot be established. Comparing only discovered files or repeatedly checking pathname strings is insufficient. |
| + |
| + The disposition’s residual is **incomplete**: it describes replacement after construction, omitting existing aliases, unchecked roots, races during checking, and pathname reuse after opening. |
| + |
| +3. **P1 — A stale-owner refusal still sends the request.** |
| + `internal/ops/litestream_metered.go:114–119,221–230`; `internal/lease/requestmeter.go:141–181`. |
| + |
| + After an ownership-changing commit, `ownedRecorder.Start` returns `ErrOwnershipStale`. The request middleware treats this as a recording failure, calls `next.HandleDeserialize` anyway, and returns the transport’s result. With a successful S3 response, Litestream considers the upload successful and advances its replica position (`third_party/litestream/replica.go:171–179`). |
| + |
| + Thus failing `Start` does **not** stop or skip the sync. It permits an unrecorded upload and successful sync advancement. It does not, by itself, fabricate successful storage or demonstrate corruption. Corruption risk arises when a hibernation transition hands the same database to another replicator while this one keeps running. |
| + |
| + The middleware can also send after its 25 ms recording budget expires, while `Current` is still waiting under its five-second timeout. Authority cannot safely depend on this best-effort recording hook. |
| + |
| + **Smallest acceptable fix:** put ownership authorization in a mandatory request gate outside the recorder’s send-on-failure behavior. Stale or unresolved ownership must prevent dispatch and propagate an error to Litestream. Keep recording-failure holds separate from permission to send. |
| + |
| +4. **P1 — Requests crossing an ownership change remain attributed to the old owner without an automatic hold.** |
| + `internal/ops/litestream_metered.go:61–77,109–119`. |
| + |
| + The exact window begins when `Unchanged` reads the old version and continues through the underlying recorder’s `Start`, request dispatch, transport completion and `Finish`. A registry commit anywhere in that interval can leave the request counted to the old owner. The mutex serializes checks, not registry writers or outstanding requests; **multiple** requests can already have passed their checks. |
| + |
| + `ownedRecorder` overrides only `Start`. Embedded `Finish`, `Abandon`, `Hold` and `Incomplete` go directly to the original recorder. `Finish` can complete an old-owner row after transfer. `Abandon` marks that original row unknown only when the existing middleware invokes it. `Hold` pins the original recorder’s date; it does not hold an ownership generation. |
| + |
| + `Current` merely sets `stale`. Only a subsequent failed `Start` calls `Incomplete`, on that request’s recorder and supplied date. Other affected recorders receive no notification until their own next request; the new owner receives none through this wrapper. Clients sharing one real ledger share its failure latch, but this API neither requires one shared ledger nor broadcasts invalidation. A midnight change can also leave the preceding day untouched. |
| + |
| + **Smallest acceptable fix:** coordinate ownership transitions with request admission and in-flight completion, or conservatively hold every potentially affected ledger/date from the last proven boundary. Invalidation must not depend on another request arriving. |
| + |
| +5. **P1 — `Close` leaves the private replicator running, making rebuilds unsafe.** |
| + `internal/ops/litestream_metered.go:82–101`. |
| + |
| + `Open` starts Litestream, but `Close` closes only the registry observation. It never calls `f.store.Close`. Litestream’s database monitors use their own background contexts (`third_party/litestream/db.go:733–761`), so cancellation of the context originally passed to `Open` is insufficient. |
| + |
| + Following the documented rebuild instruction can leave the old store operating alongside its replacement, using the same database, metadata directory and replica prefix. The stale store continues sending because of finding 3. Its store is private, so callers cannot perform the missing shutdown themselves. |
| + |
| + **Smallest acceptable fix:** close and join the underlying store, including partially opened stores, before permitting replacement. Coordinate shutdown without holding `f.mu` while waiting for workers that need `Current`, and prevent stale final-flush requests. |
| + |
| +6. **P2 — Replacing the registry can leave the witness unchanged.** |
| + `internal/ops/litestream_metered.go:73,165–192`; `internal/registry/observe.go:34–44,86–91`. |
| + |
| + The observation pins a connection to one database; it does not verify that `reg.Path()` still identifies that database. For example, switch an operator-managed registry symlink from database R1 to R2 while retaining R1. The witness can continue reading R1’s unchanged `data_version`, while fresh `reg.List()` connections read R2. |
| + |
| + During construction, that can mix the observation’s routes with discovery and ownership from another registry. Afterwards, it can leave old ownership accepted indefinitely. |
| + |
| + **Smallest acceptable fix:** include registry identity/generation in the witness and prevent or detect replacement across the observation and request lifetime. Use the witnessed route snapshot for discovery and ownership. |
| + |
| +7. **P2 — The witness invalidates normal operation, including the replicator’s own initialization.** |
| + `internal/ops/litestream_metered.go:73–75,131–141`. |
| + |
| + The fleet includes the registry. On a fresh registry, Litestream creates `_litestream_seq` and `_litestream_lock`, committing after the ownership observation (`third_party/litestream/db.go:1038–1047`). Its checkpoints also update `_litestream_seq` (`:2251`). These unrelated writes invalidate the entire fleet. |
| + |
| + Normal platform activity adds further invalidations: deployment adoption writes versions (`internal/release/adopt.go:1203`); worker readiness writes versions; instance sleep records `next_due` (`internal/activator/activator.go:1838,2026`). These are lifecycle-event writes, not rare ownership transfers. |
| + |
| + The status heartbeat is a useful distinction: its roughly five-minute loop reads the registry and writes status through the deployment store (`internal/host/agent.go:2742–2754,2867–2904`). It is **not itself a periodic registry commit**. |
| + |
| + The dispositions disclose “ANY commit” and no rebuild implementation, but omit self-invalidation and this operational consequence. With the dispatch gate corrected, replication stops at the first such commit; currently recording stops while uploads continue. |
| + |
| + **Smallest acceptable fix:** use a monotonic ownership/inventory generation updated transactionally by every relevant writer, excluding unrelated telemetry and Litestream bookkeeping. Preserve detection of ownership changes that are later undone. |
| + |
| +8. **P2 — Every valid single-file unit `DBPath` is rejected.** |
| + `internal/ops/litestream_metered.go:183–188`; `internal/ops/litestream_owners.go:194–204`. |
| + |
| + Discovery explicitly supports a unit route naming a regular database file (`internal/ops/litestream.go:401–410`). The new check nevertheless passes every nonempty unit `DBPath` to `unreadableUnder`, which calls `os.ReadDir` on it. A readable `/state/app/main.db` produces `ENOTDIR` and refuses the whole fleet. |
| + |
| + **Smallest acceptable fix:** distinguish regular-file claims from directory claims. Verify the former as readable database files; strictly traverse only the latter. Add a successful single-file unit fixture. |
| + |
| +9. **P2 — Acceptance and a second walk still permit databases to disappear unseen.** |
| + `internal/ops/litestream_metered.go:177–207`; `internal/ops/litestream_owners.go:198–216`. |
| + |
| + The acceptance list applies only to `skipped`. Discovery silently excludes dot names, files beyond depth eight, non-`.db` tree files, empty files and final `Stat` failures (`internal/ops/litestream.go:277–303,427–429`). The second walk repeats depth and dot exclusions and checks metadata rather than opening database contents. |
| + |
| + A unit database created after discovery but before the second walk is readable, so construction succeeds without including it. A temporarily unreadable subtree can likewise be omitted by discovery and readable during validation. Databases created after construction require no registry commit and trigger no refresh. Unknown files outside the limited data-directory glob patterns never require acceptance. |
| + |
| + **Smallest acceptable fix:** perform one strict discovery pass returning candidates and traversal failures together; compare against the previous replication inventory. Maintain coverage when tenant directories change. Any unproven omission must refuse or hold, rather than become an accepted empty result. |
| + |
| +10. **P2 — The actual registry path is still discarded before platform classification.** |
| + `internal/ops/litestream_owners.go:62,73–78`. |
| + |
| + Open the registry as `<dataDir>/fleet.db`. `FleetDBsWithSkipped` recognizes only the conventional registry path in its claimed set, so `fleet.db` becomes `skipped` before the new `reg.Path()` map is consulted. Without acceptance construction fails; with acceptance the registry is omitted. Outside the globbed locations it can disappear without even entering `skipped`. |
| + |
| + **Smallest acceptable fix:** add `reg.Path()` to discovery’s explicit inventory and claimed set, and require it to survive validation. Test renamed and relocated registries. |
| + |
| +11. **P2 — The ownership regression test bypasses the client whose behavior it claims to prove.** |
| + `internal/ops/litestream_metered_test.go:149–186`; `internal/lease/meteredstore_test.go:33–59`. |
| + |
| + The stale-owner test exercises `fleet.recorders[db]`, never the S3 client. Keeping that map correct while wiring `inner` into `MeteredDB.Meter` leaves the test passing. The existing middleware’s continued sends are similarly invisible. None of the new fleet tests opens and closes the store, so they miss self-invalidation, shutdown leakage and metadata ownership. |
| + |
| + The lower-level test does establish recording for its three manually issued PUTs; it does not establish store-driven replication. The duplicate fixture at `meteredstore_test.go:86` is already normalized by `filepath.Join`. The platform test at `litestream_metered_test.go:194–212` exercises only the registry identity. |
| + |
| + **Smallest acceptable fix:** exercise requests through the constructed fleet clients and an opened store. Assert transport activity, actual recorder attribution, durable holds and shutdown. Add console and relocated-registry fixtures, and use a genuinely unclean string for path-normalization coverage. |
| + |
| +**Claims the code contradicts** |
| + |
| +- **“After ANY commit … no request is started or counted”** (`DISPOSITIONS-metered-replicator.md:11`; `litestream_metered.go:104–108`): subsequent requests are still sent; requests already past the check can still be counted. The residual is not limited to one request. |
| +- **“Ownership is decided on the file”** and duplicate files become unowned (`litestream_owners.go:46–58,80–95`): ownership remains lexical; `EvalSymlinks` does not identify hard-link or bind-mount aliases. The later store check catches two listed aliases, not ownership provenance. |
| +- **Metadata is “in a tree tenants cannot write”** (`meteredstore.go:112–115`): the code supplies a location, while Litestream may create it with tenant ownership. |
| +- **“The registry is recognised by the path it is open at”** (`DISPOSITIONS-metered-replicator.md:14`): that recognition occurs too late to affect discovery. |
| +- **“Any part cannot be read” / “none was left out unseen”** (`DISPOSITIONS-metered-replicator.md:13`; `litestream_metered.go:37–40`): excluded traversal regions, transient discovery failures and later additions are not covered. |
| +- **“The most specific claim wins”** (`litestream_owners.go:108–110`): `singleOwner` requires account/app agreement and chooses the first host alphabetically. |
| +- **Platform identities are valid, but not reserved namespaces.** `registry`, `console` and the two host strings are accepted by ordinary name-shape validation. A tenant can have matching app/host strings without colliding with the platform’s full ledger attribution: account and purpose remain separate (`usage/durability.go:110–125`; `durabilityledger.go:989`). The real validator accepts the platform identities; the test does not prove console coverage or namespace reservation. |
| +- The mutation report (`DISPOSITIONS-metered-replicator.md:24`) is compatible with the narrow tests failing under those mutations. It does **not** establish client-level refusal, protected metadata, complete discovery or safe lifecycle behavior. |
| + |
| +**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 24e7c9995..8531ec7fa 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| @@ -22,3 +22,25 @@ Reports: `CODEX-FINDINGS-metered-replicator-review1.md`, `OPUS-REVIEW-metered-re |
| | Opus: what is still missing before this could attest, beyond the launch branch's item 1 | Recorded in the report; to be carried into the plan. | |
| Mutation checks in a scratch copy, each failing its test: the link check off; a file listed twice accepted; the store comparing paths and not files. The recorder's check removed; the witness's answer ignored. The platform host blanked; the unclaimed-file refusal off; the unreadable-directory refusal off; the recovery window not set. |
| + |
| +## Review two (reviewed `f4fe6f2b`; Astra rejected, Opus rejected) |
| + |
| +Reports: `CODEX-FINDINGS-metered-replicator-review2.md`, `OPUS-REVIEW-metered-replicator-review2.md`. |
| + |
| +Both rejected it again, and the main reason is a fault in the fix made after review one: the ownership witness does not do what its comment says. This branch is further from done than its first dispositions made it look. What the reviewers confirmed as closed: platform identities are accepted by the ledger and cannot collide with a tenant's (the ops account is never a tenant's); the unreadable-directory refusal; the accept list for unclaimed files, within its limits below. |
| + |
| +| Finding | Disposition | |
| +| --- | --- | |
| +| Both, P1: a stale-owner refusal does not stop the request. The request meter treats a failed `Start` as a recording failure and sends anyway; litestream sees success and advances. So after the witness trips, every replica write on the box is sent uncounted (the day is held, which satisfies "must hold", but "no request is started" in the comments is false). | Accepted. Open. Ownership needs a mandatory gate in the request path, apart from the recorder, that returns an error to litestream before the request is dispatched. That is a change to `internal/lease`'s middleware, not made here. The comments that say no request is started are wrong until then. | |
| +| Opus P1, Astra P2: the witness is SQLite's `data_version` for the whole registry database, and the registry is one of the databases this store replicates. Litestream's own checkpoint bookkeeping commits to it, as do heartbeat-like writes (next-due times, DNS and certificate status, wake and sleep). The fleet goes stale within minutes of opening and stays stale. | Accepted. Open. The witness has to be a version of the owner-bearing columns of the routes table only (a counter moved by triggers), read on the pinned connection. That is a registry schema change, not made here. As built this replicator cannot stay current. | |
| +| Both, P1: `Close` released the witness and left the litestream store running, so the documented recovery (close, build again) ran two replicators over one shadow state | Fixed in the commit that adds this section: `Close` stops the store first, then releases the witness, and a closed fleet does not open again (`TestMeteredFleetCloseStopsTheStore`). Not shown: that no monitor is left running, which needs an opened store. | |
| +| Astra P1: requests already past their check when ownership changes are finished against the old owner; only the recorder whose request comes next is told its day is incomplete; the new owner is told nothing | Accepted. Open. The window was stated for one request; it is every request in flight, and invalidation must not wait for another request. | |
| +| Opus P1, Astra P1: the link check covers the database file and not its `-wal`, `-shm` and `-journal` siblings, nor hard links; a tenant can point its `-wal` at another tenant's and have it shipped into its own prefix, or truncated by a checkpoint | Accepted. Open. At build: siblings must be absent or regular files, the database must have one link and belong to the claiming tenant's user. A swap after the check stays open until litestream opens beneath a root without following links, which is a patch to litestream. | |
| +| Astra P1: moving the shadow state under the data directory does not make it the platform's unless that directory is not writable by tenants, which nothing checks | Accepted. Open. | |
| +| Astra P2: every valid single-file unit `DBPath` is refused by the link rule's choice of root | Accepted. Open. A regression from the link fix. | |
| +| Both, P2: databases can still drop out unseen (a second walk between discovery and the readability check; a file that appears after the build); the registry is recognised by its open path in the owner map but discovery still lists only `registry.db` | Accepted. Open. | |
| +| Opus P2: any tenant can refuse the box's replicator (an unreadable subdirectory in its own state directory), and the check also refuses on directories that vanish mid-walk | Accepted. Open. Refusing is the rule; that one tenant can stop replication for the box needs deciding. | |
| +| Both: the ownership test calls the wrapped recorder directly and never drives a real client through the middleware, so it did not see that the request is still sent | Accepted. Open. This is why the first finding above was missed. | |
| +| Opus P3: one mutex and one SQLite read on every request; `Close`'s comment sat on `Open` | The comment is fixed. The rest is open. | |
| + |
| +What this means for `OUTSTANDING.md` item 1: path B is partial, unwired, and rejected twice. The pieces that hold are the ownership-by-file rule's first half, the unclaimed-file and unreadable-directory refusals, the platform identities and the private store. The ownership fence does not hold as built and needs two changes outside this package (a request gate in `internal/lease`, a routes-only version in the registry) before a third review is worth running. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review2.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review2.md| new file mode 100644 |
| index 000000000..379df3f9e |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review2.md |
| @@ -0,0 +1,184 @@ |
| +# Second review: `feat/carloku-metered-replicator-20261001` at `f4fe6f2b` |
| + |
| +## Disposition table |
| + |
| +| Row (as the dispositions word it) | Closed? | Would its test pass without the fix? | |
| +| --- | --- | --- | |
| +| **P1: owner resolved once** ("fixed") | **Not closed.** The wrapper does keep the recorder from counting. But a refused `Start` does not stop the request: `meterAttempt` sends it anyway (`requestmeter.go:141-160`, and the ledger's own `Start` doc says "sends it anyway"). After a change, every replica write still goes to S3, uncounted, and the day is held. The witness also fires on the replicator's own commits to the registry (F1), so in practice the store goes stale within minutes. | No, it would fail: without the wrapper `inner.started` would be 2. But it calls `rec.Start` directly and never goes through the S3 middleware, so it cannot see that the request is still sent. Its claim "a request is not started" is not what it tests. | |
| +| **P1: path-text ownership and links** ("fixed", with the post-build swap as residual) | **Partly.** A symlink in the final file, or in a directory beneath the claim root, is caught at build time. The `-wal` and `-shm` sidecars are never checked, even at build time (F3). A hard link to a file that is not listed is not caught. The residual covers only "replace a file with a link afterwards". | `toOutside` fails without `noLinkBeneath`. `toB`, `theirs` and `linkedDir` are all caught by the duplicate-file rule anyway, so the branch that rejects a link in an intermediate directory has no test. A mutant that checks only the last component passes. | |
| +| **P2: unreadable directory becomes partial coverage** ("fixed for unreadable directories") | **Closed only in a case production cannot reach.** The replicator runs as root, so a permission-denied directory never happens. What does trigger the check is a dangling link, a link loop, or a file removed between `ReadDir` and `Stat`. Those give a refusal a tenant can cause and a refusal that happens at random (F5). | The test is skipped when run as root. Run as another user, it would fail without the fix. | |
| +| **P2: unknown databases dropped before the refusal** ("fixed") | **Closed for the `dataDir` glob.** Not closed for databases that discovery never sees (F6). | Would fail without the fix. | |
| +| **P2: the store exposes a registration path** ("fixed at the fleet") | **Closed for registration.** But `MeteredFleet.Close` never closes the store, and the caller cannot reach it, so replication cannot be stopped (F2). | No behavioural test. Privacy is enforced at compile time, which is enough for that part. | |
| +| **P2: platform identities not accepted by the ledger** ("fixed") | **Closed.** `Bind` is `Validate` plus an insert (`durabilityledger.go:980`). | Would fail without the fix (blank host). Only the registry's identity is checked: the fixture has no console file, so `console.platform.internal` is never validated. | |
| +| **P2: seven-day window dropped** ("fixed in the setting") | **Closed as a setting.** Not exercised, as the dispositions say. | Would fail without the fix. | |
| + |
| +## Findings |
| + |
| +### P1-1. The witness is moved by the replicator's own writes, and a stale fleet keeps streaming unmetered |
| + |
| +**Where:** `internal/ops/litestream_metered.go:61-78` and `:114-120`; `internal/registry/observe.go:86`; `third_party/litestream/db.go:2251` and `:1409`. |
| + |
| +**Scenario.** The registry is one of the databases the store replicates. Litestream commits to every database it replicates: |
| +- on each checkpoint, `INSERT INTO _litestream_seq … DO UPDATE` (`db.go:2251`); |
| +- in `ensureWALExists` (`db.go:1409`) when the WAL is missing or has no header, which can happen at `Open`. |
| + |
| +Each of these is a commit from a connection other than the witness's, so `PRAGMA data_version` moves. |
| + |
| +1. `f.Open` checks `Current`, then `store.Open` runs. At the first registry checkpoint, typically within minutes, the fleet goes stale for good. |
| +2. From then on, every `Start` on every database returns `ErrOwnershipStale`. |
| +3. `meterAttempt` records a failure and sends the request anyway (`requestmeter.go:150-160`). Every replica write on the box is now uncounted, and the day is held. |
| + |
| +Litestream is not the only writer. These also commit to the registry and move the witness on a live box: |
| +- the external litestream binary checkpointing the same registry; |
| +- `SetNextDue` on every scheduled worker run; |
| +- `SetDNSStatus` and `SetCertExpiresAt` from the periodic checks; |
| +- `SetStatus` and `SetHibernate` on every wake and sleep; |
| +- `Upsert` and `SetVersions` on every deploy. |
| + |
| +None of the documents say how often the rule fires. "Blunt on purpose" understates it: the replicator cannot stay current through even one checkpoint of its own. |
| + |
| +This does break the ruling. "A meter that can miss a write must hold" is satisfied, because the box-wide latch holds the day. But the doc claim "no request is started" is false, and as built this replicator holds every day. |
| + |
| +**Smallest acceptable fix:** |
| +- Make a refusal stop the request. `ownedRecorder.Start` returns a sentinel, and `meterAttempt` returns it before calling `next.HandleDeserialize`. Litestream does not advance its position on a failed upload, so it stops and retries; it does not skip or corrupt. The LTX files pile up locally until a rebuild, and that should be stated. |
| +- Make the witness a version of the routes table only, for example a counter row bumped by triggers on `routes` insert, update and delete, read on the pinned connection. Then neither litestream's bookkeeping nor heartbeat columns move it. If heartbeat columns live in `routes`, put the triggers only on owner-bearing columns (`account`, `app`, `host`, `db_path`, `backing`, `hibernate`). |
| +- Add a test that drives a real client through the middleware and asserts zero round trips after the change. |
| + |
| +### P1-2. The prescribed recovery ("rebuild") cannot stop the old store |
| + |
| +**Where:** `internal/ops/litestream_metered.go:92-102`. |
| + |
| +`Close` releases the witness and marks the fleet stale. It never calls `f.store.Close`, and the store is private. |
| + |
| +**Scenario.** `Current` returns `ErrOwnershipStale`, and the caller does as documented: `fleet.Close()`, then `MeteredFleetStore(...)` again. |
| +1. The old litestream store keeps running its monitors. |
| +2. Every one of its requests is sent unmetered (P1-1). |
| +3. The new store opens the same databases with the same `MetaDir` (`<dataDir>/.replicator/<key>`) and the same prefixes. |
| + |
| +That is two replicators over one shadow state, which is what `meteredstore.go:112-115` says corrupts it. |
| + |
| +**Fix:** `Close` closes the store (with a context) before releasing the witness. Add a test that `Close` leaves no running DB monitors. |
| + |
| +### P1-3. Sidecar files are never link-checked: the root replicator follows a tenant's `-wal` link into another tenant's WAL |
| + |
| +**Where:** `internal/ops/litestream_owners.go:135` (only `p` is walked); `internal/lease/meteredstore.go:111`. |
| + |
| +**Scenario.** Tenant A, a unit tenant, owns `stateA/main.db` legitimately. Before or after the build, A replaces `stateA/main.db-wal` (and `-shm`) with symlinks to `stateB/main.db-wal`. `noLinkBeneath(stateA, stateA/main.db)` passes. The state directory is not sticky and world-writable, so `protected_symlinks` does not apply, and both SQLite and litestream open `<db>-wal` by path as root. Then: |
| +- B's WAL frames are read and shipped into A's replica prefix, metered to A. B's data is now restorable by A. |
| +- Litestream's checkpoint (`execCheckpoint`, RESTART or TRUNCATE) through A's connection truncates or rewrites B's WAL. B can lose transactions it has committed but not yet checkpointed. |
| + |
| +The residual in the dispositions ("a tenant can replace a file with a link afterwards") does not cover this: it works at build time too. |
| + |
| +**Smallest fix:** |
| +- At build time, check `p+"-wal"`, `p+"-shm"` and `p+"-journal"` with `Lstat`: each must be absent or a regular file. Check that the database file has a single link (`Nlink == 1`) and that its owner uid is the claiming tenant's. |
| +- State plainly that a swap after the check stays open until databases are opened beneath their root with `openat2(RESOLVE_NO_SYMLINKS|RESOLVE_BENEATH)`. Litestream's path-based opens make that a litestream patch, not a check in this code. |
| + |
| +### P2-1. What `noLinkBeneath` does and does not establish |
| + |
| +**Where:** `internal/ops/litestream_owners.go:172-186`. |
| + |
| +**It establishes:** at one instant, each path component strictly below `root` is not a symlink. |
| + |
| +**It does not establish:** |
| + |
| +| Gap | Effect | |
| +| --- | --- | |
| +| `root` itself and its parents | Unchecked, by design, but nothing checks they are platform-owned. If the parent of a route's `DBPath` can be written by the tenant, the tenant can swap the whole root. For an exact claim, `root = Dir(p)`, so only the file itself is checked. | |
| +| Hard links | `Lstat` sees a regular file and `EvalSymlinks` returns the link's own path. `os.SameFile` (`meteredstore.go:82-89`) catches a hard link only when the other name is also listed. A hard link to a database that is not listed (an exec DB the activator owns, which is excluded as hibernating, or any file outside discovery) is owned and metered to the linking tenant, and that database's content goes into their replica. `fs.protected_hardlinks=1` blocks this only if the tenant neither owns the target nor has read and write access to it, and nothing here checks that setting. | |
| +| Races during the walk | A directory renamed or swapped for a link between the `Lstat` of one component and the next, or after the walk, is not caught. This is the same open-beneath gap. | |
| +| Bind mounts | Not reachable by a tenant: unit mount namespaces are private. Operator-made bind mounts are caught by `SameFile` only if both paths are listed. | |
| +| `NewMeteredStore`'s `Stat` failure | A `Stat` failure (`meteredstore.go:82`) skips the identity check silently, so a path that is missing now and created later is never compared. | |
| + |
| +**Fix:** the `Nlink == 1` and owner-uid checks in P1-3, plus an `Lstat` check that the claim root's parent is root-owned and not writable by group or others. Record the race residual completely: the walk, the sidecars, the files and the claim root. |
| + |
| +### P2-2. Databases still drop out unseen |
| + |
| +**Where:** `internal/ops/litestream.go:273-306` (`treeDBs`) and `:331`; `litestream_metered.go:154-207`. |
| + |
| +The accept list covers only unclaimed `*.db` files from the `dataDir` glob. These are never seen at all: |
| +- A database a unit tenant creates after the build. Kass's `instances/` grows, which is the very drift the `treeDBs` comment describes. Nothing rediscovers it, and the only rebuild trigger is a registry commit. |
| +- A database that is zero bytes at build time. |
| +- A database inside a dot-directory, or more than 8 levels deep. |
| +- A database named `*.sqlite`, `*.sqlite3`, `*.db3`, or with no extension, under a tree claim. |
| + |
| +**Fix:** have `Current` (or a periodic check) rerun discovery and treat a changed set as stale. Treat dot-directories and extra depth beneath a unit root as unreadable, which refuses the store, rather than skipping them. |
| + |
| +### P2-3. Any tenant can refuse the box's replicator, and the check also refuses at random |
| + |
| +**Where:** `internal/ops/litestream_owners.go:211`. |
| + |
| +`unreadableUnder` uses `os.Stat`, which follows links. Any of these refuses the store for every tenant on the box: |
| +- one dangling symlink, or a link loop, anywhere outside a dot-directory in any unit tenant's tree; |
| +- a `*.db-journal` or temp file deleted between `ReadDir` and `Stat`, which is ordinary SQLite churn. |
| + |
| +Because root reads everything, the case the check was written for (an unreadable directory) cannot happen in production. Only these cases can. |
| + |
| +**Fix:** |
| +- Use `Lstat` and treat a symlink under the root as unowned, consistent with `noLinkBeneath`. |
| +- Ignore `ENOENT` for entries that are not databases. |
| +- Name the tenant that caused the refusal, so the refusal is per-tenant and visible rather than box-wide and silent. Under the "refuse" ruling, the whole-box refusal stays acceptable once it is named. |
| + |
| +### P2-4. The window in which a request is counted to the old owner |
| + |
| +**Where:** `litestream_metered.go:114-120`; `requestmeter.go:141-194`. |
| + |
| +**The window:** from the moment `Current`'s `PRAGMA data_version` returns unchanged inside a `Start` worker, until that request's `Finish`. That covers: |
| +- the rest of the 25 ms `Start` budget; |
| +- the whole HTTP round trip, including a large `UploadPart`. |
| + |
| +**What `ownedRecorder` does to requests already started:** it wraps only `Start`. `Finish`, `Abandon` and `Hold` pass straight through to the old owner's recorder, so a request already started is finished and counted under the attribution it was bound with. |
| + |
| +**Who is told Incomplete:** |
| +- The real ledger's `Incomplete` is a box-wide latch (`durabilityledger.go:1256-1260`). With one ledger per box, every owner's day is held, which is correct. |
| +- The dispositions and the comment say "the owner's ledger is told", which implies one ledger per owner. If `meter` ever binds a ledger per account: |
| + - only the owner whose request comes next is told; |
| + - an owner whose database stops writing is never told; |
| + - the new owner (after an account move) is never told. |
| + |
| +**Fix:** document and enforce that every recorder in the fleet must share one latch (assert it in the `meter` contract). State the in-flight window as one full request duration, not "one request already past its check". |
| + |
| +### P3-1. The registry is not recognised by the path it is open at |
| + |
| +**Where:** `litestream_owners.go:77-79` vs `litestream.go:384-392`. |
| + |
| +`reg.Path()` is added to the `platform` map, but `FleetDBsWithSkipped` claims only `dataDir/registry.db`. Suppose the registry is open at `/data/reg2.db`. It is globbed as unclaimed, so it is skipped. The caller must then name it in `accept`, and doing so drops the box's registry from replication. |
| + |
| +**Fix:** add `reg.Path()` to `claimed` in `FleetDBsWithSkipped`. |
| + |
| +### P3-2. Every `Start` on the box goes through one mutex and one SQLite read |
| + |
| +**Where:** `litestream_metered.go:62-73`. |
| + |
| +`Current` takes `f.mu` and does a SQLite read inside the 25 ms meter budget, so every `Start` across every database queues on one lock and one connection. Under contention, `errRequestMeterUnavailable` causes held days unrelated to ownership. |
| + |
| +**Fix:** keep `stale` in an atomic and read the witness on a ticker, or serialise only the PRAGMA. |
| + |
| +### P3-3. The platform identities |
| + |
| +`(OpsAccount, registry|console, *.platform.internal, PurposePlatform)` passes `Validate`. |
| + |
| +- **Collision with a tenant:** none possible. A tenant row needs a tenant account (`Validate` refuses the ops account with any purpose other than platform), and `BillableReplicationWrites` zeroes the ops account in any case. |
| +- **Ops-account routes:** they get `Platform=true` with the route's own app and host. A caller that binds them under `PurposeReplicate` is refused by `Validate`, which fails closed. |
| + |
| +No defect. The console's identity is simply untested (see the table). |
| + |
| +### P3-4. Close's doc comment is attached to Open |
| + |
| +**Where:** `litestream_metered.go:80-81`. |
| + |
| +The comment "Close releases the registry witness…" sits on `Open`. `Close` itself (line 92) has none. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **"After ANY commit to the registry no request is started"** (dispositions row 1, `litestream_metered.go:104-107` and `:131-134`, the test's doc comment). A refused `Start` still sends the request (`requestmeter.go:150-160`; the ledger's `Start` doc says "sends it anyway"). |
| +2. **"The caller rebuilds the store when Current says so"** (`:138`). `Close` cannot stop the old store, so a rebuild runs two replicators over one `MetaDir` (P1-2). |
| +3. **"The registry is recognised by the path it is open at"** (dispositions row 4). Only `dataDir/registry.db` is ever listed (P3-1). |
| +4. **"The constructor walks each unit tenant's state directory as discovery does"** (row 3, `litestream_owners.go:188-193`). Discovery skips entries whose `Stat` fails; this walk refuses the store on them. That is a different walk and a tenant can trigger it (P2-3). |
| +5. **"The owner's ledger is told its day is incomplete"** (row 1). What is actually latched is the box-wide latch, which is right in practice. The wording implies a per-owner ledger, which this code would not hold correctly (P2-4). |
| +6. **The link residual** ("a tenant can replace a file with a link afterwards"). It leaves out the sidecars (P1-3), hard links, and the claim root's parents (P2-1). |
| +7. **The mutation list** says "the link check off" fails its test. It does, but only through `toOutside`. The check on intermediate directories has no test that fails without it. |
| + |
| +**Verdict: reject.** Three P1s remain: |
| +- the witness is moved by the replicator's own checkpoint, and a refused request is still sent; |
| +- `Close` cannot stop the store, so the documented rebuild corrupts shadow state; |
| +- a tenant's `-wal` link makes the root replicator read, ship and truncate another tenant's WAL. |
diff --git a/internal/ops/litestream_metered.go b/internal/ops/litestream_metered.go| index c0f7c16ee..aabd216b1 100644 |
| --- a/internal/ops/litestream_metered.go |
| +++ b/internal/ops/litestream_metered.go |
| @@ -77,27 +77,60 @@ func (f *MeteredFleet) Current(ctx context.Context) error { |
| return nil |
| } |
| -// Close releases the registry witness. The fleet is stale afterwards. |
| // Open starts replication. It refuses once ownership is stale. |
| func (f *MeteredFleet) Open(ctx context.Context) error { |
| if err := f.Current(ctx); err != nil { |
| return err |
| } |
| - return f.store.Open(ctx) |
| + f.mu.Lock() |
| + store := f.store |
| + f.mu.Unlock() |
| + if store == nil { |
| + return ErrOwnershipStale |
| + } |
| + return store.Open(ctx) |
| +} |
| + |
| +// Databases is how many databases the store replicates (none once closed). |
| +func (f *MeteredFleet) Databases() int { |
| + f.mu.Lock() |
| + defer f.mu.Unlock() |
| + if f.store == nil { |
| + return 0 |
| + } |
| + return len(f.store.DBs()) |
| } |
| -// Databases is how many databases the store replicates. |
| -func (f *MeteredFleet) Databases() int { return len(f.store.DBs()) } |
| +// Close stops replication and releases the registry witness, in that |
| +// order. The fleet is stale afterwards and cannot be opened again. |
| func (f *MeteredFleet) Close() error { |
| + // The store first, and outside the fleet's lock: its workers ask |
| + // Current on their way out, and Current takes the lock. A fleet whose |
| + // witness was released while its store kept running would go on |
| + // replicating beside whatever store replaced it, over the same shadow |
| + // state. |
| f.mu.Lock() |
| - defer f.mu.Unlock() |
| + store := f.store |
| + f.store = nil |
| f.stale = true |
| + f.mu.Unlock() |
| + var first error |
| + if store != nil { |
| + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
| + first = store.Close(ctx) |
| + cancel() |
| + } |
| + f.mu.Lock() |
| + defer f.mu.Unlock() |
| if f.obs == nil { |
| - return nil |
| + return first |
| } |
| err := f.obs.Close() |
| f.obs = nil |
| + if first != nil { |
| + return first |
| + } |
| return err |
| } |
| @@ -128,6 +161,12 @@ func (r ownedRecorder) Start(operation string, at time.Time) (uint64, error) { |
| // box's durability ledger. It is asked once for each database. A nil |
| // recorder refuses the whole store: there is no unmetered fallback. |
| // |
| +// NOT YET TRUE AS BUILT (second review, DISPOSITIONS-metered-replicator.md): |
| +// the refusal below does not stop a request, because the request meter |
| +// sends on a failed Start, and the witness is moved by this replicator's |
| +// own bookkeeping in the registry it replicates. Both need changes outside |
| +// this package. What follows describes the intent. |
| +// |
| // Who owns each database is read once, here, and a route can change hands |
| // afterwards. So the read is taken under the registry's change witness, |
| // and every recorder is wrapped (ownedRecorder): the first request after |
diff --git a/internal/ops/litestream_metered_test.go b/internal/ops/litestream_metered_test.go| index 12f4af193..c3b850d31 100644 |
| --- a/internal/ops/litestream_metered_test.go |
| +++ b/internal/ops/litestream_metered_test.go |
| @@ -283,3 +283,34 @@ func TestMeteredFleetStoreRefusesAStateDirectoryItCannotRead(t *testing.T) { |
| t.Fatalf("a state directory with an unreadable part: fleet %v, err %v; want ErrUnmeterable naming it and no fleet", fleet, err) |
| } |
| } |
| + |
| +// Close stops the store before it lets go of the witness, and a closed |
| +// fleet does not open again: the caller builds a new one. A fleet that |
| +// released its witness and left its store running would replicate beside |
| +// its replacement, over the same shadow state. |
| +func TestMeteredFleetCloseStopsTheStore(t *testing.T) { |
| + dir := t.TempDir() |
| + reg, err := registry.Open(filepath.Join(dir, "registry.db")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + fleet, err := MeteredFleetStore(reg, dir, "replica-bucket", "metered/v1", "eu-west-1", nil, func(OwnedDB) lease.RequestRecorder { return nopRecorder{} }) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if fleet.Databases() != 1 { |
| + t.Fatalf("%d databases before Close", fleet.Databases()) |
| + } |
| + if err := fleet.Close(); err != nil { |
| + t.Fatalf("Close: %v", err) |
| + } |
| + if fleet.store != nil || fleet.Databases() != 0 { |
| + t.Fatal("Close left the store in place") |
| + } |
| + if err := fleet.Open(context.Background()); !errors.Is(err, ErrOwnershipStale) { |
| + t.Fatalf("Open after Close: %v, want ErrOwnershipStale", err) |
| + } |
| + if err := fleet.Close(); err != nil { |
| + t.Fatalf("a second Close: %v", err) |
| + } |
| +} |