amadan
Record the replicator's third review reports (Opus accept with fixes, Astra reject); dispositions follow
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 files changed,
+262
−0
docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review3.md+143 −0docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review3.md+119 −0
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review3.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review3.md| new file mode 100644 |
| index 000000000..bdf8fc0fb |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review3.md |
| @@ -0,0 +1,143 @@ |
| +| Review-two finding | Disposition at `c8627694` | Would its test pass without the fix? | |
| +|---|---|---| |
| +| A refused `Start` still sends the request | **Closed for an actual gate refusal.** `RequestGate` precedes the meter on the client Litestream uses. Refusal returns before dispatch; an upload error prevents replica-position advancement. Admission races remain below. | **No:** the option-order, retry, store-client and fleet-client tests would detect removing the gate or allowing dispatch after refusal. | |
| +| Whole-registry witness invalidates on ordinary activity and Litestream bookkeeping | **Closed for ordinary writers.** The six watched columns cover the current discovery and attribution inputs. Unchanged-value updates, versions, status, scheduling, DNS, certificates and `_litestream_*` writes do not move the generation. **Not complete against arbitrary raw statements:** finding 5. | **No:** substituting `data_version`, dropping hibernation, or removing the changed-value predicate breaks relevant cases. **Yes** for dropping `host` from the trigger: no host-change case exercises it. | |
| +| Requests crossing ownership changes retain the old attribution; only the next recorder receives a hold | **Partly closed.** The watch broadcasts to every existing recorder without waiting for traffic. Replacement construction copies the recorded interval to its recorders. The interval is insufficient, and the transfer is not durable: finding 3. | Removing broadcasting, watching or inheritance breaks their focused tests. **The tests still pass with the timing defect:** they manufacture `proven` and `found` timestamps rather than delay a real admitted request. | |
| +| `Close` leaves Litestream running | **Closed in the normal sequential implementation.** It closes and joins the store before releasing the observation. A stale shutdown disables the long final-sync retry window. | **Yes:** removing the actual `store.Close` call while retaining `store=nil` and `closed=true` defeats neither shutdown test. The closed gate hides the surviving monitors. | |
| +| Database siblings and hard links are unchecked | **Partly closed.** Existing database hard links and nonregular or multiply linked siblings are refused. Filesystem authority is still not established throughout use, and some stable aliases pass: finding 1. | Removing the sibling or link-count checks fails the relevant fixtures. Removing tenant UID or device checks survives, as disclosed. The tests do not cover the remaining roots, ancestors or same-filesystem bind mounts. | |
| +| Relocated shadow state can remain tenant-accessible | **Partly closed.** Precreated private directories fix ordinary Litestream ownership inheritance beneath a trusted data-directory path. That trusted ancestry is not established: finding 2. | Removing precreation or privacy validation breaks tests. Leaving ancestors unchecked passes. Removing the explicit shadow-root symlink rejection also passes because another check rejects the fixture. | |
| +| Valid single-file unit `DBPath` is refused | **Closed in source.** A file is handled as a named file; only directories are walked. | **The constructor regression remains untested:** the `single` fixture calls `FleetDBOwners`, not `MeteredFleetStore`. Reintroducing a constructor-only directory-read refusal would pass it. | |
| +| Discovery and readability checks permit unseen omissions | **Still open, reduced.** Hidden, deep and additional SQLite names are reported, and a second inventory detects persistent additions. Final filtering and failed header reads still silently discard candidates: finding 6. | Removing hidden/deep/name reporting fails the strict-walk fixture. The remaining silent-drop paths are untested. | |
| +| Unreadable directories and tenant-triggered box-wide refusal | **Partly closed.** Traversal failures name the claiming host. Harmless dangling links and disappearing entries no longer automatically refuse construction. A tenant can still prevent a rebuild, as disclosed; disappearing database entries are also silently ignored. | Removing unreadable-directory refusal fails when run without root privileges. Under root that case is **skipped**. The link fixture detects removing its refusal logic. | |
| +| Actual registry pathname is discarded during discovery | **Closed for this replicator.** The witnessed registry pathname enters both discovery and attribution. The external config generator retains its previous behavior. | Removing actual-path discovery fails the renamed/relocated owner fixtures. Those fixtures do not test the fleet constructor’s pathname wiring. | |
| +| Registry replacement leaves the observation valid | **Closed for a standing replacement observed by `OwnershipUnchanged`; not an atomic identity guarantee.** Construction compares pathname identity before and after reading. A swap away and back remains undetected; replacement after a check can still precede dispatch. | Removing the later identity check fails the replacement test. Removing the constructor’s before/after identity comparison would pass: the test replaces the file after construction. | |
| +| Ownership tests bypass the client | **Substantially closed.** Tests now drive the stored replica clients, and one opens Litestream over real databases. Shutdown and full request-accounting assertions remain weaker than their claims: finding 7. | Removing client gate wiring fails. Removing shutdown or the unasserted complete-count property does not necessarily fail. | |
| +| `NewMeteredStore` silently accepts a failed `Stat` | **Closed.** Failed inspection now refuses construction. | **No:** the absent-file fixture detects its removal. | |
| +| One mutex and SQLite read consume the recording budget | **Budget problem closed; serialization remains.** The gate runs outside the meter’s 25 ms budget, but every attempt still queues for a lock and a read with a five-second timeout. | No contention or budget-boundary test proves this change. | |
| +| Platform identities | **Closed.** Registry and console identities validate with `PurposePlatform`; `Bind` uses that validator. Their strings are not reserved, but matching tenant app/host strings do not collide with the full account/purpose attribution. | Blanking either platform identity now fails the fixture, which includes both databases. | |
| +| Private-store boundary and recovery settings | **Closed at the API/configuration level.** The store remains private; seven-day retention and daily snapshots are assigned. Runtime recovery is not demonstrated. | Store privacy is a compile-time boundary. Removing retention fails; removing the interval assignment passes because Litestream’s default is already 24 hours. | |
| +| Inherited prefix-lineage and external-inventory shrink guards | **Open, accurately disclosed.** Neither is enforced. | No regression test establishes either guard. | |
| + |
| +These are source-based mutation assessments, not executed results. Source was read at `c8627694`; no files were modified, tests run or network commands issued. Locations below therefore refer to **`c8627694`**, not `f4fe6f2b`, which was the second-review commit. |
| + |
| +1. **P1 — Stable aliases still pass the ownership checks, independently of the disclosed swap race.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:174`, `:264`, `:277`, `:297`, `:324`; `internal/ops/litestream.go:429`. |
| + |
| + `noLinkBeneath` has been replaced by `checkOwnedFile`. The replacement establishes individual observations of descendant components, database link count, device identity, selected directory permissions and sibling shape. These observations are neither atomic nor bound to Litestream’s later opens. |
| + |
| + In particular: |
| + |
| + - `root` is inspected with `Stat`; the `Lstat` loop starts **below** it. |
| + - A tree-root symlink can pass when its spelled and resolved parents pass `platformOwned`. |
| + - Those parent checks do not inspect every ancestor. |
| + - `namedFile` checks neither directory authority nor database UID. |
| + - Equal device numbers do not exclude a bind mount from the same filesystem. A bind-mounted file can retain `Nlink == 1`. |
| + |
| + For example, a unit route naming `/state/A/current/main.db` is classified as `namedFile`. If A can replace `current` with a link to B’s directory, the final database is regular and singly linked, and its device matches the resolved parent. With B’s original pathname absent from this fleet’s inventory, duplicate detection provides no protection. A is assigned B’s file. |
| + |
| + Separately, renaming directories during the walk or replacing the database/WAL after inspection remains possible. Litestream opens the database by pathname and repeatedly opens its WAL by pathname (`third_party/litestream/db.go:1004`, `:1014`, `:1866`). The thirty-second second look detects some persistent changes; it cannot prevent a read, upload or checkpoint through a substituted path. |
| + |
| + **Smallest acceptable fix:** derive authority from a trusted root bound to the tenant, pin that root, and preserve the verified identity through database and sibling opens. Reject unauthorized mount crossings, including same-filesystem bind mounts. Until those opens are safe, refuse the affected paths. The disposition’s residual must include stable root/ancestor aliases, not only changes after inspection. |
| + |
| +2. **P1 — Shadow-state protection assumes trustworthy ancestors without checking them.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:540–575`; `internal/ops/litestream_owners.go:348`. |
| + |
| + `prepareShadowState` correctly makes the ordinary `.replicator` hierarchy private. Litestream’s `MkdirAll` fast path leaves those existing directories unchanged, so normal descendant files carrying tenant ownership remain inaccessible through that private hierarchy. |
| + |
| + However, `platformOwned(dataDir)` checks only the final directory. A root-owned `dataDir` beneath a tenant-writable parent passes. After preparation, the tenant can rename that directory and substitute another tree. Litestream subsequently resolves the original metadata pathname; missing metadata components can again be created with tenant-derived ownership and permissions. This happens during local initialization, before an S3 refusal can protect anything (`third_party/litestream/db.go:1064`; `third_party/litestream/internal/internal.go:124–146`). |
| + |
| + Thus the old creation/chown problem is fixed for a trusted, stable `/data` hierarchy, but the constructor does not establish that prerequisite. |
| + |
| + **Smallest acceptable fix:** verify and pin the data-directory ancestry from a trusted root, then create and use shadow state relative to that authority. Existing shadow contents must also be safe to reuse; checking only directories down to the replica key cannot establish the safety of arbitrary preexisting descendants. |
| + |
| +3. **P1 — The hold interval does not cover every request admitted before a change and sent afterwards.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:52–57`, `:181–205`, `:219–235`, `:511–517`; `internal/lease/requestmeter.go:131–160`. |
| + |
| + There are failures at **both ends** of the interval. |
| + |
| + **After its end:** a request passes the gate just before midnight. Another request or the watch discovers an ownership change before midnight and calls `goStale`. The admitted request resumes after midnight, enters `meterAttempt`, records its start on the new day and sends. `holdDays` ended at the previous day’s `staleAt`; `goStale` runs only once. The newly recorded day receives no ownership hold. |
| + |
| + **Before its beginning:** a request starts before midnight but waits before sending. Other successful admissions advance `proven`. Ownership changes after midnight, more than a minute after that request started. The hold now begins on the new day, although the delayed request’s ledger row belongs to the previous day. The 25 ms recorder budget does not bound goroutine scheduling or transport delay; Litestream’s HTTP client allows operations lasting up to 24 hours. |
| + |
| + File invalidation has another boundary problem: `lookAgain` uses `proven`, which records successful **registry** checks, not the last successful filesystem inspection. Requests can keep advancing that timestamp during a long filesystem walk. |
| + |
| + There is no `ownedRecorder` in this build. The original recorder receives `Start`, `Finish`, `Abandon` and `Hold` directly. A successful `Finish` can finalize the old attribution after a transfer; `Abandon` is invoked for recording failures, not ownership changes. `Hold` temporarily pins the attempt’s start date—it does not permanently invalidate its attribution. |
| + |
| + Crash handling remains incomplete too. The fleet’s transition interval and replacement relationship exist only in memory. Unfinished ledger rows become unknown, and the real shared ledger has additional crash-gap protection. But requests can already have finished, and a fresh fleet cannot recover the old ownership interval or notify a separate new-owner ledger from `Replaces`. |
| + |
| + **Smallest acceptable fix:** track admitted attempts and their dates through completion, holding every affected date until they are resolved. Persist ownership-transition uncertainty before it can be lost, and replay it during replacement/restart. Filesystem uncertainty needs its own proof boundary. A fixed retrospective minute is insufficient. |
| + |
| +4. **P1 — An admission queued before filesystem invalidation can succeed after the fleet is stale.** |
| + |
| + **Location:** `internal/ops/litestream_metered.go:160–184`. |
| + |
| + `admit` checks `stale` before acquiring `f.mu`, but does not check it again after acquiring the lock or reading the witness. |
| + |
| + A request can pass that first check and wait behind another admission. Meanwhile, `lookAgain` detects a substituted file and calls `goStale` without taking `f.mu`. The waiting request then acquires the lock. Its registry generation is unchanged, so it updates `proven`, returns success and reaches the meter and network despite the fleet already being stale. |
| + |
| + This differs from an attempt whose admission completed before invalidation: the pending admission itself succeeds afterwards. |
| + |
| + **Smallest acceptable fix:** synchronize the stale transition with admission and recheck stale state inside that protocol before granting permission. Add a deterministic test that queues an admission, invalidates a file, then releases the queued admission. |
| + |
| +5. **P2 — The trigger set does not witness every raw route mutation.** |
| + |
| + **Location:** `internal/registry/registry.go:599–602`. |
| + |
| + The current setters and `Upsert` are covered. The universal “whichever statement” claim is not. |
| + |
| + `routes` is a rowid table. A writer with recursive triggers disabled can execute: |
| + |
| + ```sql |
| + UPDATE OR REPLACE routes |
| + SET rowid = (SELECT rowid FROM routes WHERE host = 'b.example.com') |
| + WHERE host = 'a.example.com'; |
| + ``` |
| + |
| + SQLite deletes the conflicting B row. The replacement deletion does not fire DELETE triggers with recursive triggers disabled; no INSERT occurs; and the UPDATE names none of the six watched columns. The ownership generation remains unchanged. The fleet retains B’s witnessed route and can keep streaming its database indefinitely. |
| + |
| + **Smallest acceptable fix:** prohibit route-rowid changes with a database trigger, or witness them in a way that also covers conflict-induced deletion. Do not rely on a connection setting that an older/raw writer need not enable. Add this raw-statement case. |
| + |
| + For rollout, the ordinary compatibility path is otherwise sound: this build’s `Open` migrates an older registry before returning, and older DML writers inherit the installed triggers. `ObserveRoutes` itself does not migrate; an unmigrated database or missing counter produces an error, including for its existing metrics caller. That is failure to establish a witness, not silent acceptance. |
| + |
| + The generation is also deliberately broader than semantic ownership changes: adding a database-free route, or changing a unit route’s hibernation flag, invalidates the fleet. Conversely, routine deploy-version, scheduling, DNS, certificate and status writes no longer do. The status heartbeat reads the registry and publishes externally; it is not itself a periodic registry commit. |
| + |
| +6. **P2 — The “strict” inventory still turns failed inspection into successful omission.** |
| + |
| + **Locations:** `internal/ops/litestream.go:428–442`, `:464–466`; `internal/ops/litestream_owners.go:395–429`, `:449–457`. |
| + |
| + The strict walk does not own the entire discovery pipeline. |
| + |
| + A unit claim whose initial `Stat` fails falls through as a file candidate; the final `Stat` failure then silently discards it. A candidate returned by the strict walk can disappear or become inaccessible before that final filter and vanish without entering `Problems`, `Unowned` or `Skipped`. The walk also ignores `ENOENT` for database entries, not just disposable journal files. |
| + |
| + An extensionless SQLite database whose header cannot be opened or read is treated as “not SQLite.” Databases under arbitrary extensions and unclaimed files outside the limited data-directory globs remain invisible. These omissions never reach `Accept`. |
| + |
| + Persistent later additions generally produce `ErrInventoryChanged`, which deliberately continues replication without a hold. There is still no comparison against the external replicator’s inventory. Consequently this code cannot establish that the proposed replacement covers every previous replication writer. |
| + |
| + **Smallest acceptable fix:** preserve inspection failures and discovered candidates through final validation. Separate legitimate optional absence from failed inspection of an explicit claim or already-discovered database. Return header-read errors as uncertainty, and require comparison against the previous replication inventory before cutover. |
| + |
| +7. **P2 — The new integration test still cannot prove shutdown or complete request accounting.** |
| + |
| + **Locations:** `internal/ops/litestream_metered_test.go:658–695`, `:914–944`, `:998–1012`. |
| + |
| + The unopened shutdown test checks that the fleet discards its store and refuses retained clients. The opened test closes an already-stale fleet, then checks that no further requests reach the server. Both can pass with `store.Close` removed: the gate prevents traffic while Litestream’s database monitors remain alive. |
| + |
| + The opened test’s comment says every server request was counted, but the assertion beneath it only waits for `inFlight == 0`. Earlier checks require positive counts for both databases, not equality between received attempts and recorded attempts. Swapping both recorders would also preserve those positive-count assertions, although the separate direct-client test does catch misattribution for its tenant request. |
| + |
| + The periodic filesystem check has a similar gap: the filesystem test invokes `lookAgain` directly. Removing its invocation from `watch` leaves that test passing. |
| + |
| + **Smallest acceptable fix:** retain database handles and assert they are closed after shutdown; test a current opened fleet as well as a stale one. Compare settled server attempts with recorded attempts per database, and exercise filesystem invalidation through the running watch. |
| + |
| +**Claims the code contradicts** |
| + |
| +- **“Nothing from the claim root down is a link” / “no link or mount”:** the root can be followed, ancestors remain unchecked, and device equality does not exclude same-filesystem bind mounts. |
| +- **The margin “covers every request” crossing a change:** neither the earliest admitted attempt nor the last delayed start/send is bounded by that interval. |
| +- **“Every replica request is refused from then on”:** queued admissions can pass after filesystem invalidation, and already-admitted requests continue. |
| +- **Every writer moves the counter “whichever statement it uses”:** conflict deletion through a rowid update bypasses the trigger set. |
| +- **The strict walk “passes nothing over in silence”:** failed header reads and several discovery/filtering paths do exactly that. |
| +- **A vanished pathname means “nothing is being shipped from it”:** Litestream can retain an open database descriptor and pending shadow files. |
| +- **The opened test establishes complete accounting and stopped monitors:** its assertions establish neither property. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review3.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review3.md| new file mode 100644 |
| index 000000000..787bf4115 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review3.md |
| @@ -0,0 +1,119 @@ |
| +No P1 from review two is left unfixed, but I found one new P1 and one stated residual that is worse than the dispositions say. Verdict: accept with fixes, and only as unwired code. |
| + |
| +Line numbers are at **c8627694**. f4fe6f2b, which the brief names for line numbers, is review two's commit and not an ancestor of this branch, so its line numbers don't match this code. Sources were read with `git show` only. The sandbox would not start in this worktree (`bwrap` could not find `.claude/settings.json`), so the read-only git commands ran outside it. |
| + |
| +## Review two, finding by finding |
| + |
| +| Review two finding | Now | Would its test pass without the fix? | |
| +|---|---|---| |
| +| P1: a refused request still sent | **Closed.** `RequestGate` sits in the Deserialize step, before the meter in either option order. That step runs on every attempt, after retry and signing and before the transport sends. The vendored litestream (`third_party/litestream/s3/replica_client.go:461,620`) applies `ClientOptions` to every S3 client it builds: the main client, the region lookup, and the uploader built on `c.s3`. So every request litestream makes passes the gate. A refusal comes back to `WriteLTXFile` as an error, and litestream does not advance its position. | No. `TestMeteredFleetStopsSendingWhenOwnershipChanges` writes through the store's real client and counts what reaches the bucket. | |
| +| P1: witness moved by every commit | **Closed for the routes table.** It moves on insert, on delete, and on an update of host, account, app, db_path, backing or hibernate. Those are every routes column that `fleetDBsFromRoutes` and `fleetInventory` read. Triggers are part of the schema, so they fire for any build's writes and for a raw statement from an older build. Nothing in the registry's history drops or rebuilds `routes`. `INSERT OR REPLACE` fires the insert trigger even when recursive triggers are off. It is wider than "exactly ownership": see P2-2. | No. The "stays" cases fail on the whole-database witness. | |
| +| P2: registry file replaced | **Closed for a replacement that stays in place.** The pinned connection keeps the old inode open, so its number cannot be reused while the observation lives. A swap away and back between the first look and the open is stated. | No. | |
| +| P1: in-flight requests, and only one owner told | **Partly.** `goStale` now holds days on every recorder, the watch asks every second, and `Replaces` passes the hold on. The handover misses when the new fleet is built before the old one notices (P2-1). The hold's upper end is wrong (P3-1). A crash before the rebuild is stated. | Mostly not, but `TestMeteredFleetReplacementInheritsTheHold` calls `old.Current()` first, which forces detection and hides P2-1. | |
| +| P1: `Close` left the store running | **Closed.** | No. | |
| +| P1: siblings, hard links, claim root | **Closed for tree claims, at one instant.** Single-file claims are open (P1-1). A swap during litestream's first open defeats `lookAgain` permanently (P1-2). | No for the listed facts. There is no test with a link above a named file. | |
| +| P1: shadow state | **Closed for everything under MetaDir.** What litestream does in the tenant's directory by path is still open (§5). | Yes for the root-is-a-link case, as stated. | |
| +| P2: single-file `DBPath` refused | **Closed as a regression, but accepted with a weaker check (P1-1).** | No. | |
| +| P2: databases drop out unseen | **Partly** (P2-3). | No for the cases it tests. | |
| +| P2: registry's real path | **Closed.** | No. | |
| +| Opus P2: one tenant can block the box | **Open, a stated decision.** | Not applicable. | |
| +| Tests bypassed the client | **Closed.** | Not applicable. | |
| +| P3: one lock and one read per request | **Changed.** Any witness error, including a transient busy read, makes the fleet stale for good (P3-2). | None. | |
| +| `Stat` failure in `NewMeteredStore` | **Closed.** | No. | |
| +| Prefix-lineage guard, shrink guard | **Open.** | Not applicable. | |
| + |
| +### Answers to questions (1) to (6) |
| + |
| +1. **Yes.** A refused attempt never reaches the network or the meter on any client litestream builds. |
| +2. **The generation moves for every change of ownership.** It is not exact: adding or removing any route moves it, including routes with no database. The trigger set covers every writer. The registry's file path is covered by the identity check. Ownership inputs outside the registry are not in the witness: `CONSOLE_DB` and the files themselves, which `lookAgain` handles. |
| +3. **The hold is wide enough at the bottom and not at the top** (P3-1). If the process dies, the hold goes with it. Requests admitted before the change and finished before the crash, a window of milliseconds, are left as complete rows on a day nobody held, not as started rows. |
| +4. **Each look establishes the facts at that instant only.** |
| + - For a tree claim, `checkOwnedFile` establishes: no link from the root down; the same filesystem; one name; the file belongs to the tree's user; siblings are plain files with one name; the directory above the tree is the platform's. |
| + - `strictTreeDBs` finds `.db` files and lists the other SQLite names. The header check runs only on files with no extension. |
| + - `lookAgain` adds a re-check every 30 seconds of the path's current file, not of what litestream has open. |
| + - For a tenant who can write the directory, a swap between looks remains, and so does a swap during litestream's first open, which stays forever (P1-2). |
| +5. **Under `.replicator`, no:** a root-owned 0700 directory stops the tenant from reaching anything. **In the tenant's own directory, yes, by path, as root:** |
| + - it opens the database (`db.go:1014`); |
| + - the SQLite connection creates or opens `-wal` and `-shm`; |
| + - it sets `journal_mode = wal`; |
| + - it writes litestream's tables into the database; |
| + - it truncates the WAL. |
| + |
| + All of these follow a link swapped in after the check, unless SQLite's own open refuses links, which this branch doesn't establish. |
| +6. **The rollout is safe.** |
| + - An older build writing to a migrated registry fires the triggers, which is conservative. |
| + - Rolling back leaves the triggers behind, and they do no harm. |
| + - This build creates the table and triggers when it opens a registry. A registry it cannot migrate, or one without the counter, makes `ObserveRoutes` fail, so the fleet refuses. |
| + - A restore or `VACUUM INTO`-and-rename changes the file's identity, so the fleet goes stale. A restored registry gets its triggers back from its own schema. |
| + - Every `registry.Open` now also runs an `INSERT OR IGNORE`. That is a write on every open, which per my notes `Open` already does. |
| + |
| +## Findings |
| + |
| +**P1-1. A route that names one file follows links above it, and nobody's user is checked.** |
| +`internal/ops/litestream_owners.go:174-178`, `:226-228`, `:264` (`os.Stat(root)`) and `:324`; `internal/ops/litestream.go:428-443`. |
| +- **How:** for a `namedFile` claim the root is `Dir(p)`, reached through `os.Stat`, which follows links. Only the file's own name gets `Lstat`. Nothing above it is checked, there is no UID check, and the filesystem check compares the file against the root after the link is followed. |
| +- **Scenario:** a unit route has `-db /var/lib/carlos-app/a.example.com/data/data.db`. Tenant A renames `data` and puts a link in its place pointing at another directory that holds a `data.db` nobody else lists, for example: |
| + - a unit tenant without `-db`; |
| + - a file the activator replicates. |
| + |
| + Every check passes. The other tenant's database is streamed into A's prefix and billed to A. If the target is listed elsewhere, the same-file check makes both unowned, and the whole box is refused instead. |
| +- **Variant:** a unit route's `DBPath` is never tied to that route's own state directory. A route that claims another tenant's tree, as a tree, with no other claimant, passes the UID check, because the files belong to the tree's own user. |
| +- **Smallest fix:** check a unit route's file the way a tree claim is checked. The root is the tenant's state directory, which must be platform-owned above. `Lstat` every part down from it, and require the file's UID to match. Refuse a unit `DBPath` outside `/var/lib/carlos-app/<host>`. Add a test with a link in a parent directory. |
| + |
| +**P1-2. A swap during litestream's first open pins it to the wrong file for good, and `lookAgain` cannot see it.** |
| +`internal/ops/litestream_metered.go:297-321`; `third_party/litestream/db.go:1014`. |
| +- **How:** litestream opens the database file and its SQLite connection by path when it first syncs, and keeps both open. `lookAgain` checks what the path names now, not what litestream has open. |
| +- **Scenario:** tenant A swaps `main.db` for a link to `/data/console/console.db` (or to another tenant's database) for the instant of that first open, then swaps it back. Litestream keeps streaming the console database into A's prefix, metered to A, and two replicators then work on one database. Every later look passes. |
| +- **Against the claims:** the dispositions say the second look "narrows the window". For this case it does not. |
| +- **Smallest fix:** litestream is vendored (`replace => ./third_party/litestream`), so the patch is in reach. Expose the identity of the open database file and compare it with `facts` in `lookAgain`. Better, open by file descriptor beneath a pinned root. |
| + |
| +**P2-1. The new owner is told nothing if the new fleet is built before the old one notices.** |
| +`internal/ops/litestream_metered.go:514-518` (`staleWindow` is read once, at build). |
| +- **Scenario:** a caller rebuilds straight after a `carlos add` or a move, or for an inventory change, less than a second after the commit. The new fleet reads the routes after the change and is current. The old fleet goes stale later and holds only its own recorders. The new owner's day is never held. |
| +- **Smallest fix:** in `Close`, read the witness one last time before releasing it. Read `Replaces.staleWindow()` in `Open`, which already requires the old fleet to be closed, rather than at build. Test it without calling `old.Current()` first. |
| + |
| +**P2-2. Any route added or removed on the box stops the replicator and holds every account's day. This is not stated.** |
| +`internal/registry/registry.go:587-609`; `internal/ops/litestream_metered.go:197-206`. |
| +- **Who writes:** the domain reconcile (`domains/reconcile.go:207,356`), the provision reconcile (`:1090`), the sidecar reconcile (`:442`), temporary sites, and instance creation all insert or delete routes. Most of those routes have no database. |
| +- **Effect:** each one stops replication for every database until something rebuilds the fleet, and nothing does. Each one also marks that day incomplete for every account on the box, so busy boxes will seldom attest a day. |
| +- **Smallest fix:** at minimum, state it in the dispositions. Better: |
| + - bump the generation on insert or delete only when the row has a `db_path` or unit backing; |
| + - on a change, hold only the recorders whose owner, path or backing actually changed. |
| + |
| +**P2-3. Databases can still drop out without anyone being told.** |
| +- `litestream.go:357-367`: a unit route without `DBPath` is never walked, and nothing refuses it. |
| +- `litestream_owners.go:426-431`: names are matched case-sensitively, so `App.DB` is missed. The header check runs only on files with no extension, so `store.data` is missed. |
| +- `litestream.go:409-421` against `:374-388`: an exec route whose `DBPath` is outside the two globs is claimed but never listed. |
| +- `litestream.go:464-466`: a missing or empty named file is dropped at build. |
| +- **Smallest fix:** |
| + - refuse a unit route that has no `DBPath`; |
| + - check the SQLite header on every regular file in a tenant's tree; |
| + - turn the last two into problems or skipped files, not silent drops. |
| + |
| +**P2-4. The replica prefix and shadow state are keyed by path, not by owner.** |
| +`litestream_metered.go:495-500`; `litestream.go:170`. |
| +- **How:** a path that changes hands (moved account, or deleted and made again) continues the old owner's lineage. |
| +- **Effect:** the old owner's data can be restored from under the new owner, and the retention deletes and compaction of the old owner's objects are billed to the new owner, which is "whoever is nearest". |
| +- **Smallest fix:** put the account in the key and in the MetaDir, or refuse to resume a lineage whose owner differs. |
| + |
| +**P3-1. The hold's upper end is wrong.** |
| +`litestream_metered.go:197-206`, `:52-57`. |
| +- The requests in doubt started after the change, which is after the last proof, so the one-minute margin at the bottom is never needed. |
| +- The top of the window is `staleAt`. A request admitted before the change and recorded after `goStale` ran, across midnight, falls on a day nobody held. |
| +- **Fix:** hold up to `staleAt` plus the meter's budget, or hold the next day too when the stale time is within a margin of midnight. |
| + |
| +**P3-2. One transient error makes the fleet stale for good.** |
| +`litestream_metered.go:175-180`. A busy or recovering registry read is treated like a change: every recorder is held and the fleet must be rebuilt. Separate a read error from a witnessed change, and retry the read before deciding. |
| + |
| +**P3-3. Same-filesystem bind mounts are not caught.** The filesystem check compares device numbers only, and a bind mount from the same filesystem has the same one. Only an operator or systemd can make such a mount, so state it as a residual. |
| + |
| +## Claims the code contradicts |
| + |
| +- **The link rule's comment and the dispositions** ("nothing from the claim root down is a link"; a single-file route "is checked as a file"). The root itself is followed, and for a single-file claim nothing above the file is checked (P1-1). |
| +- **`lookAgain` and the dispositions** ("narrows the window"). Not for a swap during the first open, which persists (P1-2). |
| +- **`MeteredFleetOptions.Replaces` and the dispositions** ("the fleet built to replace it tells its own recorders the same days"). Only if the old fleet had already noticed (P2-1). |
| +- **The `ownershipMargin` and `goStale` comments** ("the margin ... covers every request admitted before a change and sent after it"). The margin covers the bottom of the window, which never needed it. The top is uncovered (P3-1). |
| +- **The dispositions on a crash** ("the started rows it leaves are held by the ledger"). Rows finished between the change and the crash are complete rows on a day nobody held. The window is milliseconds. |
| +- **The witness is described as moving "only when ownership changes".** It moves on every route added or removed, including routes with no database (P2-2). |
| + |
| +**Verdict: accept with fixes.** Fix P1-1 and P2-1 and state P2-2 before merging. P1-2 blocks any attestation, and the vendored litestream makes it fixable. The branch stays unwired and unattested. |