amadan
Record the metered replicator's first review: both reviewers reject; nothing fixed yet
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3 files changed,
+210
−0
docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review1.md+60 −0docs/superpowers/specs/DISPOSITIONS-metered-replicator.md+22 −0docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review1.md+128 −0
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review1.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review1.md| new file mode 100644 |
| index 000000000..fb4a3de7d |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review1.md |
| @@ -0,0 +1,60 @@ |
| +All locations refer to `ebab1a37`. Source review only; no tests were run. |
| + |
| +1. **P1 — Ownership expires, but the recorder does not.** `internal/ops/litestream_metered.go:43–64`; `internal/ops/litestream_owners.go:39–43`. |
| + |
| + Discovery reads the registry, ownership resolution reads it again, and `meter(o)` binds once for the store’s lifetime. If a route moves from account A to B while that client remains open, subsequent uploads still reach A’s recorder. Deleting and recreating the route at the same database path has the same problem. A change between the two registry reads can also combine discovery from one route set with ownership from another. Neither case produces a hold. |
| + |
| + **Smallest acceptable fix:** use one witnessed ownership observation for discovery and binding, then fence that binding against ownership or database-instance changes throughout replication. Stop or hold affected requests before rebinding; polling and comparing final rows alone cannot detect a delete/recreate interval. |
| + |
| +2. **P1 — Filesystem aliases bypass conflicting-owner detection.** `internal/ops/litestream_owners.go:66–76`; `internal/lease/meteredstore.go:78–83`. |
| + |
| + Account A can name `/state/a/main.db` while account B names a symlink or hard link to that same file. `filepath.Clean` leaves those names distinct. Discovery follows symlinks, but ownership matching and duplicate detection compare strings. Both databases are accepted, each with a different owner’s recorder. A symlink inside A’s unit tree pointing into B’s tree likewise makes lexical containment look like ownership. |
| + |
| + **Smallest acceptable fix:** resolve and compare physical file identities across discovered databases and route claims, refusing conflicting claims and unresolved identities. Protect that identity against replacement while the binding remains usable. |
| + |
| +3. **P2 — Discovery errors become successful partial coverage.** `internal/ops/litestream_owners.go:39–41`. |
| + |
| + The newly adopted discovery function suppresses filesystem failures: `treeDBs` returns an empty result on `ReadDir` failure (`internal/ops/litestream.go:280–282`), and the final filter skips every `Stat` error (`:427–429`). An unreadable unit state directory therefore disappears, while the readable registry lets `MeteredFleetStore` succeed. Registry query errors are propagated correctly; filesystem read errors are not. |
| + |
| + **Smallest acceptable fix:** provide strict discovery for this constructor. Return errors for unreadable configured paths, subtrees and incomplete traversal; distinguish explicitly permitted absence from inability to establish coverage. |
| + |
| +4. **P2 — Unknown databases are silently discarded before the refusal check.** `internal/ops/litestream_metered.go:43`; `internal/ops/litestream_owners.go:47–49`. |
| + |
| + The constructor throws away `skipped`. A database still replicated by the external configuration, whose route has disappeared, can consequently be omitted without an error or a returned diagnostic. The new path does not use the existing `CoverageShortfall` guard. A concrete platform case is a registry opened as `<dataDir>/fleet.db`: discovery and platform classification recognise only `<dataDir>/registry.db`, despite `reg.Path()` identifying the actual registry. |
| + |
| + **Smallest acceptable fix:** retain skipped paths and require an explicit coverage decision before starting. Compare the replacement inventory with the previous replication inventory, and identify the registry using `reg.Path()`. Unknown databases must remain visible and block unexplained removal. |
| + |
| +5. **P2 — The returned store exposes an ordinary unmetered registration path.** `internal/lease/meteredstore.go:61,100–102`; `internal/ops/litestream_metered.go:70`. |
| + |
| + Both constructors return unrestricted `*litestream.Store`. Its `RegisterDB` method accepts a database with an ordinary unmetered replica client and opens it without checking for a recorder (`third_party/litestream/store.go:269–315`). `DBs()` also exposes live database pointers whose `Replica.Client` can be replaced. Thus a caller adding a newly discovered database through the returned store’s normal API bypasses every constructor refusal. |
| + |
| + **Smallest acceptable fix:** return a wrapper that keeps the store and mutable clients private, with database registration available only through the same mandatory metering and ownership checks. The constructors’ existing explicit error returns themselves correctly return nil stores. |
| + |
| +6. **P2 — Platform ownership records cannot bind directly to the real ledger.** `internal/ops/litestream_owners.go:53–55`. |
| + |
| + Registry and console databases receive `Account: OpsAccount` and `Platform: true`, but empty `App` and `Host`. `ReplicationAttribution.Validate` requires valid, nonempty app and host values even for `PurposePlatform` (`internal/usage/durability.go:110–125`). A callback translating this ownership record into the production ledger’s attribution therefore fails, and the whole fleet store refuses to start. The no-op recorder in the fleet test hides this mismatch. |
| + |
| + **Smallest acceptable fix:** define explicit, valid identities for platform databases—or a deliberately supported platform attribution shape—and test these returned records against the real ledger. Do not substitute a nearby tenant’s identity. |
| + |
| +7. **P2 — The replacement drops the established seven-day recovery window.** `internal/lease/meteredstore.go:100–102`. |
| + |
| + The store inherits Litestream’s 24-hour snapshot retention, with retention enforcement enabled (`third_party/litestream/store.go:60–61,143–148`). The external fleet configuration explicitly uses seven days (`internal/ops/litestream.go:69–72`). After replacement, corruption discovered several days later can lack the historical snapshot the fleet previously retained. |
| + |
| + **Smallest acceptable fix:** apply the fleet’s snapshot interval and retention policy before opening the store, and verify the resulting retention behavior. |
| + |
| +8. **P3 — The tests establish narrower properties than their comments claim.** `internal/ops/litestream_metered_test.go:40–66`; `internal/lease/meteredstore_test.go:35–58,73`. |
| + |
| + The fleet test checks callback arguments and database counts using `nopRecorder`; it would still pass if the constructed clients omitted the recorder options. Its refusal cases discard returned stores, so they would also miss an error accompanied by a usable store. The lease test genuinely exercises the SDK and detects missing metering for its three small PUTs, but never opens a database or exercises background replication, multipart uploads, restore, or durable ledger holds. Its duplicate-path fixture uses `filepath.Join`, which removes `.` before the constructor sees it; removing the constructor’s path cleaning would not break that case. |
| + |
| + **Smallest acceptable fix:** add fleet-to-ledger request assertions, assert nil values on every refusal, and exercise the opened store’s lifecycle. Supply an actually unclean path for the normalization case. Keep fake-provider evidence explicitly distinguished from backend proof. |
| + |
| +Claims the code contradicts or does not establish: |
| + |
| +- **“Every replica client” and “no unmetered way in”** (`internal/lease/meteredstore.go:26–33`; `meteredstore_test.go:62–63`) hold for initial construction, not the unrestricted returned store. |
| +- **“Every request … is counted”** overstates the middleware contract. Recording failures and saturation can allow requests through while marking evidence incomplete (`internal/lease/requestmeter.go:141–181`). That behavior is compatible with the required hold rule, but it is not an exact-count guarantee. |
| +- **“Never dropped”** does not describe the fleet constructor’s treatment of skipped databases or filesystem discovery failures. |
| +- **“The most specific claim wins”** (`internal/ops/litestream_owners.go:57–60`) is not implemented. `singleOwner` requires all claims to agree on account and app, then chooses the lexicographically first host. The conservative conflict refusal is preferable; correct the comment. |
| +- **The attestation prerequisites exceed OUTSTANDING item 1.** Beyond its listed work, completion needs ownership and file-identity fencing, coverage-preserving inventory reconciliation, enforced metered registration, valid platform attribution, a fresh replica-lineage cutover with retained recovery history, and a ledger publication topology that cannot overwrite another writer’s box-day. The plan already identifies the last issue at `docs/superpowers/plans/2026-10-01-durability-attestation-path.md:28`; these constructors do not resolve it. |
| +- **No hidden retry, multipart, region-discovery or restore client bypass was found in the initial client graph.** Both S3 constructors receive `ClientOptions`, the uploader uses that instrumented client, and replica restore uses the existing client. That source evidence does not establish ownership correctness or end-to-end attestation. |
| + |
| +**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| new file mode 100644 |
| index 000000000..d745e87cd |
| --- /dev/null |
| +++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| @@ -0,0 +1,22 @@ |
| +# Metered replicator (path B, first pieces): review dispositions |
| + |
| +Branch `feat/carloku-metered-replicator-20261001`. Nothing calls this code and nothing attests. Replication-write completeness stays a launch blocker. |
| + |
| +## Review one (reviewed `ebab1a37`; Astra rejected, Opus rejected) |
| + |
| +Reports: `CODEX-FINDINGS-metered-replicator-review1.md`, `OPUS-REVIEW-metered-replicator-review1.md`. Nothing below is fixed yet. |
| + |
| +| Finding | Disposition | |
| +| --- | --- | |
| +| Both, P1: the owner of a database is resolved once, when its client is built, and never again. A route moved to another account, or an instance deleted and made again, goes on being counted to the first owner. | Accepted. Open. Ownership has to be bound to the registry generation it was read at and re-checked, with the client refused or rebuilt on a change. | |
| +| Both, P1: ownership and the duplicate-file check compare paths as text. A tenant symlink, or two spellings of one path, bills one tenant for another's database. | Accepted. Open. Resolve to the file's identity (device and inode, opened beneath the data root) before comparing. | |
| +| Astra P2, Opus P3-5: a discovery error, or an unreadable or vanished database, becomes successful partial coverage | Accepted. Open. Must refuse or hold. | |
| +| Astra P2: unknown databases are dropped before the refusal check | Accepted. Open. | |
| +| Astra P2, Opus P2-1: "refuses anything it cannot meter" is true only while the store is built; the returned store still exposes an ordinary registration path | Accepted. Open. | |
| +| Astra P2: platform ownership records cannot bind to the real ledger as written | Accepted. Open. | |
| +| Astra P2: the replacement drops the seven-day recovery window the external replicator keeps | Accepted. Open. | |
| +| Opus P2-2: nothing enforces that the replica prefix is not the external binary's lineage | Accepted. Open. | |
| +| Opus P2-4: the fleet store drops `skipped` and has no shrink guard | Accepted. Open. | |
| +| Both, tests: none shows the right recorder on the right client or a store-driven request being metered; several only exercise a fake | Accepted. Open. | |
| +| Opus P3-1 to P3-4: two non-atomic registry reads; an arbitrary instance for one account and app; prefix uniqueness by exact string; a typed-nil recorder | Accepted. Open. | |
| +| 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. | |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review1.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review1.md| new file mode 100644 |
| index 000000000..d70e826c9 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review1.md |
| @@ -0,0 +1,128 @@ |
| +# Review: feat/carloku-metered-replicator-20261001 at ebab1a37 (base d5705a63) |
| + |
| +## Findings |
| + |
| +### P1-1. The owner is resolved once and never checked again, so a change of owner bills the wrong account |
| +`internal/ops/litestream_metered.go:55-64`, `internal/ops/litestream_owners.go:38-81` |
| + |
| +`meter(o)` is called once, when the store is built, and the recorder stays fixed to that DB's client for as long as the store lives. Nothing in `OwnedDB`, `MeteredDB` or the recorder contract carries a registry witness. |
| + |
| +Scenario: instance `a.example.com` (account `bac`) is deleted. It is recreated under account `bae`, or the route moves to `bae`. The DB path is still `<dataDir>/a.example.com.db`. Every PutObject from then on is counted to `bac` until something rebuilds the store, and nothing does. |
| + |
| +The same applies to: |
| +- **A deleted route whose file remains.** It keeps streaming, billed to an owner who has left. |
| +- **A route that flips to `Hibernate`.** The activator's metered replicator and this store then both write one WAL. The writes are double-counted and the two replicators fight over checkpoints. |
| + |
| +**Smallest fix:** |
| +- Read the routes once, record a witness in `OwnedDB` (the registry `data_version` or serial, plus the route's revision), and pass it to `meter`. |
| +- The recorder (or a wrapper here) fails `Start` and latches `Incomplete` once the witness is stale. |
| +- Add a test: change the owner after the build, make a request, and require a hold rather than a count for the old owner. |
| + |
| +### P1-2. Ownership and the duplicate-file check compare paths as text, so a tenant symlink bills one tenant for another's database |
| +`internal/ops/litestream_owners.go:66-68`, `internal/lease/meteredstore.go:78-83` |
| + |
| +`treeDBs` follows symlinks through `os.Stat`. Ownership is a lexical `strings.HasPrefix` on the link's path, and `NewMeteredStore` removes duplicates by `filepath.Clean` only. |
| + |
| +Scenario: unit tenant A (it owns its StateDirectory) creates `link.db → /var/lib/carlos-app/B/main.db`, or `→ /data/console/console.db`. The root replicator then opens B's database, or the console's, a second time. It streams the copy into A's prefix and meters it to A. This is wrong-owner billing, two litestream DBs fight over one WAL, and the console's or B's data is copied into A's replica lineage. |
| + |
| +A related root-in-tenant-directory hazard: `meteredstore.go:92` puts the meta directory beside the DB, inside the tenant-writable directory, under a predictable name. The tenant can plant `.replicator-main.db-litestream` as a symlink, and the root replicator will write and delete LTX files through it. |
| + |
| +**Smallest fix:** |
| +- In `FleetDBOwners`, run `filepath.EvalSymlinks` on both the file and the claim root, and require the resolved file to lie under the resolved root. Mark anything else unowned. |
| +- In `NewMeteredStore`, remove duplicates by `os.SameFile` (device and inode) and refuse two entries for one file. |
| +- Put the meta directory in a root-owned tree, for example `<dataDir>/.replicator/<replicaKey>`, not beside the DB. |
| + |
| +### P2-1. "Refuses anything it cannot meter" holds only while the store is being built |
| +`internal/lease/meteredstore.go:61,100`, `internal/ops/litestream_metered.go:39` |
| + |
| +Both functions return a raw `*litestream.Store`, and three public routes on it lead to unmetered writes: |
| +- `Store.RegisterDB(db)` (`third_party/litestream/store.go:269`) accepts any DB with any client. |
| +- `DBs()[i].Replica.Client` is an exported field that can be replaced. |
| +- `ClientOptions` is read lazily in `Init` (`s3/replica_client.go:461`), so `client.ClientOptions = nil` before `Open` removes the meter. |
| + |
| +The future wiring that calls this, or a later "register the new tenant DB" change, gets an unmetered way in with no compile error. |
| + |
| +Error paths are clean: every refusal returns `nil` and leaves nothing usable behind. |
| + |
| +**Smallest fix:** return an opaque `*MeteredStore` that exposes only `Open`, `Close` and, if needed, `SyncDB`. Keep the litestream store unexported. |
| + |
| +### P2-2. The rule that the replica prefix must not be the external binary's lineage is not enforced |
| +`internal/ops/litestream_metered.go:30-33,53-63` |
| + |
| +`prefix == ""` is accepted, which gives the key `replicaKey(path)` with no prefix. Any value is accepted, including the `<prefix>` the generated `/etc/litestream.yml` already uses (`LitestreamConfig`, `litestream.go:140`). A caller who passes the existing prefix gets two formats and two metadata directories on one lineage. That is the untested restore case the comment warns about. |
| + |
| +**Smallest fix:** refuse an empty prefix. Either take the external config's prefix as an argument and refuse a match, or nest the lineage under a fixed segment such as `metered/v1/`. |
| + |
| +### P2-3. The tests never show that the right recorder sits on the right client, and never show a store-driven request being metered |
| +- **`litestream_metered_test.go`**: |
| + - `nopRecorder` is never driven by a request. |
| + - A mutation that swaps recorders between DBs, or installs the first recorder on every DB, still passes. |
| + - The prefix result (`"/metered/v1/"` → `metered/v1/<key>`) is never asserted, so dropping the prefix also passes. |
| +- **`meteredstore_test.go:15-60`**: |
| + - It calls `Client.WriteLTXFile` by hand. The store is never `Open`ed, so sync, compaction, L0 retention, snapshot, DeleteObjects, LIST and multipart are untested. |
| + - `newFakeS3` (`endpoint_test.go:53`) speaks only GET, PUT and DELETE on one key, so it cannot run the store's lifecycle at all. |
| + - "Nothing reaches the bucket that no meter saw" is therefore proven only for the three requests the test makes itself. |
| + - Attribution is inferred from the counts 1 and 2, not from the keys each request wrote. |
| +- **`meteredstore_test.go:73`**: `filepath.Join(dir, ".", "a.db")` is already clean, so the "listed twice" case passes without the `filepath.Clean` in the code under test. |
| +- **No symlink, owner-change or hibernate-flip cases** anywhere. |
| + |
| +**Smallest fix:** |
| +- Give each DB a distinct recorder in `MeteredFleetStore`, send a request through each DB's client, and assert which recorder saw it and under which key prefix. |
| +- Add one test that opens the store against a fake that can list (or MinIO, as `TestDeliveredContractMinio` does), writes to both DBs, syncs, closes, and requires the fake's request log to equal the union of the meters. |
| + |
| +### P2-4. The fleet store drops `skipped` and has no shrink guard |
| +`internal/ops/litestream_metered.go:43` |
| + |
| +`MeteredFleetStore` discards `skipped`. Once this store replaces the external binary, any DB the current, possibly hand-kept, config replicates but `FleetDBs` skips will silently stop being replicated. `CoverageShortfall` is the net that the existing verb relies on. |
| + |
| +**Smallest fix:** return `skipped`. Make the eventual cutover refuse when `CoverageShortfall(liveConfig, owned∪unowned)` is non-empty. |
| + |
| +### P3-1. Two non-atomic registry reads |
| +`litestream_owners.go:39,43` |
| + |
| +`FleetDBsWithSkipped` and `FleetDBOwners` each call `reg.List()`. A route that changes between the two reads produces a set whose hibernation exclusion comes from one snapshot and whose ownership comes from the other. |
| + |
| +Read errors are handled correctly: they return an error and nothing is built. |
| + |
| +**Fix:** list once and pass the routes into a shared helper. |
| + |
| +### P3-2. Same account and app, different instances: the instance chosen is arbitrary |
| +`litestream_owners.go:91-101` |
| + |
| +When two hosts of one account and app claim the same file, `Host` is "first by name". If the ledger ever keys by instance, that is an arbitrary choice. Either refuse, or document that `Host` is informational only. |
| + |
| +### P3-3. Prefix uniqueness is checked as an exact string |
| +`meteredstore.go:80` |
| + |
| +`replicas/a` and `replicas/a/` (or `/replicas/a`) pass as different prefixes but name one lineage. |
| + |
| +**Fix:** apply `path.Clean` and trim `/` before the uniqueness check, and refuse one prefix nested inside another. |
| + |
| +### P3-4. A typed-nil recorder passes both nil checks |
| +`litestream_metered.go:57`, `meteredstore.go:76` |
| + |
| +A `(*T)(nil)` recorder passes both checks and panics in `Hold` on the request path. It fails closed, with no write, but it crashes the replicator. Either check with reflection, or document that the callback must return a real value. |
| + |
| +### P3-5. Unreadable or vanished databases are dropped silently |
| +Unit DB paths that fail `os.Stat`, and unreadable subtrees in `treeDBs`, are dropped with nothing named (`litestream.go:282,427-429`). This predates the branch. Once this store is the only writer, though, those databases are unreplicated, and the comment at `litestream_owners.go:33-37` says nothing is dropped. |
| + |
| +## Still missing before this could attest, beyond OUTSTANDING item 1 |
| + |
| +1. **An ownership witness, enforced on every request** (P1-1). Also a rebuild, or a hold, when the registry serial changes. New tenant databases created after start are otherwise unreplicated. |
| +2. **A defined handoff when `Hibernate` changes between the activator and this store.** Today both can hold one WAL. |
| +3. **Resolving every path to a real file, and a root-owned meta directory** (P1-2). |
| +4. **The ledger topology choice.** The plan (path B paragraph) says it is not made: the same ledger as the tenant replicator, or a labelled second ledger plus a fold change. `meter` cannot be wired honestly until it is made. |
| +5. **A cutover guard.** Run `CoverageShortfall` against the live config, cut over the flagship's hand-kept config first, and stop the external unit before `Open`, so two replicators never share a WAL during the switch. |
| +6. **A store-lifecycle metering test against a real S3-compatible server** (P2-3), watched to fail with one client left unmetered. This is the AGENTS.md rule that an absent thing has to be checked by looking for it. |
| + |
| +## Claims the code contradicts |
| + |
| +- **`meteredstore.go:17-19`, "Exec-backed routes are replicated by the activator … and are metered."** Only *hibernating* exec routes are. Non-hibernating exec DBs are in `FleetDBs` and go to the external binary, as the plan's own scope rules say. |
| +- **`meteredstore.go:31-33` and `:58-60`, "There is no unmetered fallback" and "refuses … two databases that share a file."** Both hold only at construction (P2-1), and the duplicate-file check is lexical (P1-2). |
| +- **`litestream_owners.go:33-37`, "never dropped and never guessed."** Symlinked paths are guessed lexically, and stat failures upstream are dropped. |
| +- **`litestream_metered.go:21-24`, "every database FleetDBs lists."** True only at build time. |
| +- **`litestream_metered.go:30-33`, the prefix "must not be one the external binary has written to."** Not enforced, and `""` is accepted. |
| +- **`litestream_owners.go:33-34`, "a file two routes of different accounts both claim."** The code also refuses one account with two apps. That is stricter, so it is safe, but the comment is inaccurate. |
| +- **OUTSTANDING item 1 at d5705a63 cites branch `2140eb7b`.** That is not an ancestor of ebab1a37, which is a rebased copy carrying the same message. The handoff should cite ebab1a37. |
| + |
| +**Verdict:** reject. The two P1s are a wrong-owner count when an owner changes, and a cross-tenant symlink that this code bills to the wrong owner. Both have to be fixed and tested before the pieces are wired. |