amadan
S3 change index spec rev 2: answer Astra round 1 (11 findings accepted) (#336)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+248
−101
docs/superpowers/specs/2026-10-21-s3-change-index-design.md+225 −101docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-R1.md+23 −0
CI cancelled — superseded by a newer push — run details
CI log
=== gate === cd launchctx && go vet ./... && go test ./... -count=1 ok amadan.net/carlos/platform.git/launchctx 0.012s ? amadan.net/carlos/platform.git/launchctx/controlname [no test files] ? amadan.net/carlos/platform.git/launchctx/wireid [no test files] go vet ./... gofmt -l .: clean go test ./... -count=1 -timeout 30m amadan: job cancelled — a newer push superseded it
diff --git a/docs/superpowers/specs/2026-10-21-s3-change-index-design.md b/docs/superpowers/specs/2026-10-21-s3-change-index-design.md| index ce056e743..fc52fb64e 100644 |
| --- a/docs/superpowers/specs/2026-10-21-s3-change-index-design.md |
| +++ b/docs/superpowers/specs/2026-10-21-s3-change-index-design.md |
| @@ -1,7 +1,9 @@ |
| # S3 change index: stop re-walking the deployment bucket |
| Amadan discussion: carlos/platform #336. Branch `s3-polling-336`. |
| -Status: draft rev 1, 2026-10-21. |
| +Status: draft rev 2, 2026-10-21. Rev 2 answers Astra round 1 |
| +(`CODEX-FINDINGS-S3-CHANGE-INDEX-R1.md`); accepted findings are marked |
| +**(codex N)**, and all eleven were accepted. |
| ## 0. Problem and evidence |
| @@ -104,21 +106,40 @@ A pure function `ChangeScope(key) (scope string, ok bool)` in |
| | `accounts/<a>/…` | `accounts/<a>` | |
| | `control/certs/…` | `control/certs` | |
| | `control/edges/…` | `control/edges` | |
| -| everything else (including `control/changes.json` and `control/serial`) | none | |
| +| everything else (including `control/changes.json`, `control/serial` and `control/edge-mesh-key*`) | none | |
| The data-prefix list is a denylist on purpose: a missing exclusion costs |
| an extra re-read, a missing inclusion costs staleness until the backstop. |
| `runs/` is excluded because run state that readers act on lives in the |
| instance record (`ActiveRun`), which is in scope. The exact data-prefix set |
| is fixed in the plan from the writer map (#336 thread) and pinned by a |
| -table test. |
| +table test. Keys outside every scope keep the readers they have today |
| +(§5 for the mesh keys). |
| ### 3.3 Write path |
| -The bump lives in the store, not at call sites. A wrapper |
| -`release.WithChangeIndex(s Store) Store` decorates the deployment store; |
| -after a successful `Put` or `Delete` whose key has a scope, it bumps that |
| -scope: |
| +**The bump lives inside the two concrete stores**, not in a decorator |
| +**(codex 3, 6, 11)**. `S3Store` and `DirStore` gain an opt-in |
| +`EnableChangeIndex()`; once enabled, each of their four mutation methods, |
| +`Put`, `PutFrom`, `Delete` and `DeleteIfMatch`, bumps the key's scope after |
| +the mutation succeeds. Because the hook is in the concrete type: |
| + |
| +- every optional extension (`ConditionalDeleter`, `PutStreamer`, `Opener`, |
| + `SizeLister`, `PageLister`, `BucketListDirs`, `ModLister`) is untouched, |
| + and every mutation path, including `release.DeleteIfMatch` |
| + (`internal/release/conditional_delete.go:29`) and streaming `PutFrom`, |
| + bumps; |
| +- callers that type-assert `*release.S3Store`, such as box publication |
| + (`internal/host/boxpublish.go:259`), still match; |
| +- outer wrappers (`NewPrefixStore`, the hostops mutation boundary, |
| + `internal/hostops/mutation_boundary.go:108,188`) sit above the hook, so |
| + their writes bump, and the index write itself goes straight to the |
| + bucket from inside the store, never through the boundary's classifier |
| + (`internal/hostops/mutation_classifier.go:47,168`). That is deliberate: |
| + the index carries no authority, so the boundary has nothing to decide |
| + about it. |
| + |
| +The bump: |
| 1. `Get control/changes.json` (absent = empty, schema 1). |
| 2. Set `entries[scope] = old + 1` (or 1). |
| @@ -137,19 +158,24 @@ Properties: |
| else bumps the same scope. |
| - **Coalescing** (plan detail): a process may batch several scopes into one |
| read-modify-write, provided each scope's bump still follows its write. |
| -- The wrapper forwards the optional extensions (`ModLister`, the new |
| - `ETagLister`, `ConditionalGetter`) so wrapping never removes a |
| - capability (today `NewPrefixStore` drops them, |
| - `internal/release/store.go:215-217`). |
| + Bursts (a purge deleting hundreds of keys under one app, run churn) are |
| + expected to coalesce to one bump per scope per batch; contention on the |
| + single object is otherwise the same class as `control/serial` today. |
| -Every opener of the deployment store wraps it: the console |
| +Every opener of the deployment store enables the index: the console |
| (`cmd/console/main.go`), the box agent and edge (`internal/host/agent.go`, |
| `internal/edge/edge.go`), the CLI's direct-bucket `openStore()` |
| (`cli.go:2065`) and every other `release.NewS3Store` call site that opens |
| the deployment bucket. A test walks the AST for `release.NewS3Store`/ |
| `NewS3StoreIn` call sites and requires each to be listed as either |
| -"deployment, wrapped" or "other bucket" with a reason, so a new opener |
| -cannot silently skip the index. |
| +"deployment, index enabled" or "other bucket" with a reason, so a new |
| +opener cannot silently skip the index. |
| + |
| +IAM: every role that writes the deployment bucket needs `s3:PutObject` and |
| +`s3:GetObject` on `control/changes.json`. The plan checks the |
| +`deployment-buckets` and `region-host` module policies and the Tito |
| +deployment's equivalents; a denied bump is a failed bump (logged, covered |
| +by the backstop), never a failed write. |
| Writes that still bypass the index: binaries built before this change |
| (old CLIs in direct-bucket mode, boxes mid-roll), and any tool writing the |
| @@ -157,11 +183,28 @@ bucket directly. The backstop (§4.4) covers them. |
| ### 3.4 Read path |
| -Optional extension `ConditionalGetter.GetIfChanged(ctx, key, etag)` returns |
| -`(body, etag, changed, err)`; S3 answers 304 Not Modified for an unchanged |
| -object. A reader polls `control/changes.json` with it and diffs `entries` |
| -against the generations it last acted on. A 304 costs one GET ($0.40 per |
| -million), so a 2s poll is ~43k GETs per process per day. |
| +Two new optional read extensions, implemented by both concrete stores: |
| + |
| +- `ConditionalGetter.GetIfChanged(ctx, key, etag)` returns |
| + `(body, etag, changed, err)`; S3 answers 304 Not Modified for an |
| + unchanged object. A 304 costs one GET ($0.40 per million), so a 2s poll |
| + is ~43k GETs per process per day. |
| +- `ETagLister.ListETags(ctx, prefix)` returns each key with its ETag. S3 |
| + returns the ETag with every ListObjectsV2 entry; `DirStore` returns the |
| + same ETag its `Get` returns. ETags avoid the one-second LastModified |
| + resolution trap `ClaimCache` needs a settle window for. |
| + |
| +The hostops mutation boundary and `NewPrefixStore` forward both read |
| +extensions **(codex 6)**. |
| + |
| +**Capability fallback (codex 7).** A reader uses the index only when its |
| +store answers both extensions and the index read succeeds. A store without |
| +them (the fleet store door, `internal/edge/storedoor.go:149,431`, and test |
| +fakes), a denied read, or any index error means "unknown", never "empty |
| +and unchanged": that pass walks as today. Customer fleet boxes therefore |
| +keep today's polling; a tenant-filtered index for the door is a follow-up, |
| +because exposing the global index would disclose other tenants' account |
| +and app names. |
| ## 4. The record view (box agent and console) |
| @@ -172,9 +215,13 @@ A `RecordView` in `internal/provision` holds, for the whole deployment: |
| - per account: known/soft-deleted, from `account.json`, with the |
| generation it was read at; |
| - per app: the `instances/` and `sidecars/` key listings with each key's |
| - ETag, the parsed record bodies, the trash marker and the `manifest.json` |
| - facts Reconcile reads per pass (`reconcile.go:983-1004`), and the app's |
| - generation; |
| + ETag; the parsed record bodies; the trash marker; the instances manifest |
| + `instances.json` (`ManifestKey`, `internal/provision/provision.go:321`; |
| + seeds and update policy, `reconcile.go:995`) and the sidecars manifest |
| + `sidecars.json` (`internal/provision/sidecar.go:122`; placement, |
| + `sidecar_reconcile.go:328`), each through its existing parser and |
| + keeping its distinct missing and error semantics **(codex 5)**; and the |
| + app's applied generation; |
| - `complete` and `unreadable` flags with today's meanings (strict |
| consumers fail on an incomplete view exactly as they fail on a partial |
| walk today). |
| @@ -186,57 +233,88 @@ consume it without changing their logic. |
| ### 4.2 Refresh |
| -`Refresh(ctx)`: |
| -1. Conditional GET of the index. Unchanged: done. |
| -2. For each scope whose generation moved (or is new): re-read that scope. |
| - An app scope re-lists `instances/` and `sidecars/` with |
| - `ETagLister.ListETags` and GETs only keys that are new or whose ETag |
| - changed; it re-reads the trash marker and manifest. An account scope |
| - re-reads `account.json`. |
| -3. Record the new generations only for scopes re-read successfully. A |
| - failed scope keeps its old generation (so the next refresh retries) and |
| - marks the view incomplete. |
| - |
| -`ETagLister` is a new optional extension: S3 returns the ETag with every |
| -ListObjectsV2 entry; `DirStore` returns the same ETag its `Get` returns. |
| -Using ETags instead of LastModified avoids the one-second resolution trap |
| -that `ClaimCache` needs a settle window for. |
| - |
| -### 4.3 Ordering and monotonicity |
| - |
| -- The view only moves forward: each scope is replaced by a newer listing, |
| - never an older one. |
| -- In the agent, `Refresh` for a Reconcile pass runs inside `reconcileMu`, |
| - so the snapshot a pass uses is never older than the previous pass's (the |
| - reason `reconcileMu` exists, `agent.go:1529-1542`). |
| -- One full pass refreshes once and feeds Reconcile, ReconcileSidecars and |
| - the moves pass from the same view, replacing three walks with one |
| - refresh. The moves pass, which runs under its own `w.pass` lock, reads a |
| - copy taken after the Reconcile refresh; it is at least as fresh as its |
| - own walk would have been when it started. |
| +The view keeps two generation maps **(codex 4)**: `observed` (the latest |
| +index it has read) and `applied` (per scope, the generation its current |
| +contents reflect). `Refresh(ctx)`: |
| + |
| +1. Conditional GET of the index. Changed: update `observed`. Unchanged |
| + (304): keep `observed`. |
| +2. For every scope where `applied < observed`, or that is new: re-read that |
| + scope. Step 2 runs on a 304 too, so a scope whose earlier re-read failed |
| + is retried on the next refresh. An app scope re-lists `instances/` and |
| + `sidecars/` with `ListETags`, GETs only keys that are new or whose ETag |
| + changed, and re-reads the trash marker and both manifests. An account |
| + scope re-reads `account.json`. |
| +3. Set `applied[scope]` to the observed generation only for scopes re-read |
| + successfully. The view is incomplete while any scope has |
| + `applied < observed`. |
| + |
| +### 4.3 Publication, snapshots and lock order |
| + |
| +**(codex 1, 2)** |
| + |
| +- **One refresh lock.** `RecordView.mu` serializes every mutation of the |
| + view: incremental `Refresh` and the backstop rebuild (§4.4) alike. A |
| + publication therefore always reflects reads that started after the |
| + previous publication's reads, so the view never moves backwards, and a |
| + slow backstop cannot publish over a newer incremental read. This is the |
| + same serialization the walk has today, where Reconcile walks inside |
| + `reconcileMu` (`internal/host/agent.go:1529-1542,1570`). |
| +- **Immutable snapshots.** Each publication produces an immutable |
| + snapshot with a monotonically increasing epoch, swapped in atomically. |
| + Consumers read snapshots; they never hold `mu`. |
| +- **Reconcile and sidecars.** Inside `reconcileMu`, the pass calls |
| + `Refresh` and then takes the current snapshot, so a pass's snapshot is |
| + never older than the previous pass's. |
| +- **Moves.** Today the moves pass walks inside `w.pass` |
| + (`internal/host/movespass.go:1061,1079`). It keeps that shape: inside |
| + `w.pass` it calls `Refresh` and takes the current snapshot, and it |
| + records the epoch it consumed and refuses a snapshot with a lower epoch |
| + (a guard; it cannot happen while publication is monotonic). It does not |
| + take a copy handed over from the Reconcile leg. |
| +- **Lock order:** `reconcileMu` → `RecordView.mu`, and `w.pass` → |
| + `reconcileMu` → `RecordView.mu`. `RecordView.mu` is a leaf: nothing is |
| + acquired while holding it. Move operations that take `reconcileMu` |
| + inside `w.pass` (`internal/host/movesoffline.go:1065`) keep their |
| + existing order, and nothing holds `reconcileMu` across the moves pass. |
| ### 4.4 Backstop walk |
| -A full strict walk (today's code path, with ETag reuse for bodies) rebuilds |
| -the view from scratch every `changeIndexBackstop` (default 30 minutes, |
| -jittered ±10%) per process, and at start. Its result replaces the view |
| -wholesale and resets every generation to the index's current values read |
| -**before** the walk began (so a bump that lands during the walk triggers a |
| -re-read afterwards, never a skip). |
| +A full strict walk (today's code path, with `ListETags` reuse for bodies) |
| +rebuilds the view every `changeIndexBackstop` (30 minutes, R3, jittered |
| +±10%) per process, and at start. Under `RecordView.mu` it reads the index |
| +first, walks, publishes the rebuilt view, and sets `observed` and |
| +`applied` to the index generations read **before** the walk began, so a |
| +bump that lands during the walk triggers a re-read afterwards, never a |
| +skip. Holding `mu` for the walk blocks incremental refreshes for its |
| +duration, which is no worse than today, where every Reconcile pass walks |
| +inside `reconcileMu`. |
| -The backstop is what bounds staleness for writers that bypass the index. |
| -Today those writers (box run/seed/move writes, direct-bucket CLI writes) |
| -already rely on the 5-minute floor; with the index they converge in |
| -seconds, and only failures or old binaries wait for the backstop. |
| +The backstop bounds staleness for writers that bypass the index. Today |
| +those writers (box run/seed/move writes, direct-bucket CLI writes) rely on |
| +the 5-minute floor; with the index they converge in seconds, and only |
| +failures and old binaries wait for the backstop. |
| -### 4.5 What triggers a pass |
| +### 4.5 What triggers a pass, and what it reads |
| - `fastTick` keeps reading `control/serial` every 2s as today and also |
| - polls the index (§3.4). Either change triggers a full pass, which is now |
| - a `Refresh` plus the existing reconcile logic. The 5s clamp stays. |
| + polls the index (§3.4). The 5s clamp stays. |
| +- **A serial change** triggers today's full pass, with the record walks |
| + replaced by `Refresh`. Its adopt and config-sync legs keep their current |
| + scope (hosted routes), because the serial is also bumped for things |
| + outside the index's scopes. |
| +- **An index-only change** triggers a **scoped pass** **(codex 10)**: |
| + `Refresh`, Reconcile and ReconcileSidecars over the snapshot, the moves |
| + pass, and then adopt (`release.Pass`, `internal/release/adopt.go:682`) |
| + and config sync (`internal/appconfig/syncexec.go:174`) restricted to |
| + hosted routes whose app scope moved. Changing one app re-reads that app |
| + only. A witness test asserts that an index bump for one app causes no |
| + read under any other app's prefix. |
| - `instancesLoop`'s 5–6 minute floor stays, now as a `Refresh` (one |
| - conditional GET when nothing changed) rather than a walk. |
| -- The backstop is separate (§4.4). |
| + conditional GET when nothing changed) followed by a scoped pass over |
| + whatever moved. |
| +- The backstop (§4.4) ends with today's unrestricted full pass, so every |
| + leg is re-run over everything at least every 30 minutes. |
| ### 4.6 Console sweeps |
| @@ -253,40 +331,57 @@ seconds, and only failures or old binaries wait for the backstop. |
| ## 5. GET loops |
| +Every gated read below also has a **maximum successful-read age equal to |
| +the backstop interval**, a read on first use, and a retry on error, |
| +independent of index movement **(codex 9)**. |
| + |
| - **`configGenLoop`**: GETs an app's `config-gen.json` only when the app's |
| - scope generation moved, or when its last read is older than the backstop |
| - interval. `genCache` (`agent.go:1641`) stays as the ETag comparison. |
| + scope generation moved, or when its last successful read is older than |
| + the backstop interval. `genCache` (`agent.go:1641`) stays as the ETag |
| + comparison. |
| - **Canary pointers** (`fastMembers` → `PassFilter`): canary routes stay |
| members of every tick, but `PassFilter` reads a channel pointer only when |
| - its app's generation moved or its last read is older than the backstop. |
| - Direct-bucket promotes from new CLIs bump the app scope, so canaries keep |
| - adopting in seconds; old CLIs fall back to the backstop. |
| -- **`telemetryPass`**: app and account `telemetry.json` reads are gated on |
| - their scope generations, and the account file is read once per pass, not |
| - once per app. |
| + its app's generation moved or its last successful read is older than the |
| + backstop. Direct-bucket promotes from new CLIs bump the app scope, so |
| + canaries keep adopting in seconds; old CLIs fall back to the backstop |
| + (R4). |
| +- **`telemetryPass`** (`internal/host/telemetry_pass.go:35`): app and |
| + account `telemetry.json` reads are gated on their scope generations plus |
| + the age rule, and the account file is read once per pass, not once per |
| + app. |
| - **Cert mirror** (`certsync.go:627`): polls the index; when |
| - `control/certs` moves, `ListETags` and GET only new or changed certs; the |
| - full rebuild that drops spent challenge tokens runs at the backstop and |
| - whenever a key disappears from the listing. The boot refresh before the |
| - listeners bind is unchanged. |
| -- **Home map** (`internal/edge/homemap.go`): the same rule for |
| - `control/edges`. |
| + `control/certs` moves, `ListETags` and GET only new or changed certs; a |
| + full rebuild, the one that drops spent challenge tokens, runs at the |
| + backstop and whenever a key disappears from the listing. The boot |
| + refresh before the listeners bind is unchanged. |
| +- **Home map** (`internal/edge/homemap.go:320`): only the hosts half is |
| + gated on `control/edges`. The mesh keys (`control/edge-mesh-key` and |
| + `.next`, `internal/edgeproof/edgeproof.go:58`) keep their independent |
| + 30s poll (two GETs per box per 30s), and `RefreshAttempted` is still |
| + recorded on every poll, unchanged ones included, so the five-minute |
| + refresh heartbeat (`internal/lbhealth/lbhealth.go:36,174`) holds |
| + **(codex 8)**. |
| ## 6. Provider boundary |
| Everything is core: conditional PUT and GET and ListObjectsV2 ETags are |
| plain S3 API, supported by MinIO and implemented by `DirStore`. No AWS SDK |
| outside `internal/release`'s existing S3 client. A deployment that never |
| -arms the readers behaves exactly as today. `TestDeliveredContractMinio` |
| -gains coverage for `GetIfChanged` and `ListETags`. |
| +arms the readers behaves exactly as today, and so does any store that |
| +lacks the read extensions (§3.4). `TestDeliveredContractMinio` gains |
| +coverage for `GetIfChanged` and `ListETags`. |
| ## 7. Rollout |
| Writers ship first and always bump (cost: ~one extra GET and PUT per |
| relevant write, ~$3/month). Readers are armed per process by |
| `CARLOS_CHANGE_INDEX=read` (box `host.env`, console environment), off by |
| -default, so a mixed fleet behaves as today. |
| +default, so a mixed fleet behaves as today. Writers that bump never change |
| +any read path, so shipping them is safe with readers disarmed; this |
| +includes box publication, which keeps its concrete `*release.S3Store` |
| +(§3.3). |
| +0. Check IAM (§3.3) on dev, flagship and Tito before the release. |
| 1. Release with the writers and dormant readers. Roll dev boxes, dev |
| console, flagship boxes, flagship console (roll order rule). |
| 2. Arm dev (boxes then console). Check: passes still converge (instance |
| @@ -300,27 +395,48 @@ default, so a mixed fleet behaves as today. |
| walking, writers keep bumping harmlessly. |
| The census overlay for the touched packages is owed per the plan14 |
| -process (#280). |
| +process (#280). Release notes tell operators to update the CLI (R4). |
| ## 8. Testing |
| -- **Production-wired witnesses**: the writer wrapper is exercised through |
| - the real openers (console, agent, CLI direct mode) against `DirStore`, |
| - not through a fake; the AST opener test from §3.3. |
| +- **Production-wired witnesses**: the write hook is exercised through |
| + the real openers (console, agent, CLI direct mode) and through the final |
| + claim-enabled store handles (hostops boundary, prefix stores) against |
| + `DirStore`, not a fake **(codex 6)**; box publication runs through the |
| + index-enabled opener **(codex 11)**; the AST opener test from §3.3. |
| +- **Capability preservation**: every optional extension the concrete |
| + stores implement today is still implemented with the index enabled, and |
| + every mutation method bumps **(codex 3)**. |
| - **Invariant simulation**: random interleavings of writers (some |
| - bumping, some bypassing, some failing their bump), concurrent readers |
| - refreshing, and backstop walks, on `DirStore`. Invariants: (a) after a |
| - `Refresh` that observed generation g for a scope, every write to that |
| - scope that bumped to ≤ g is visible in the view; (b) after a backstop |
| - walk, the view equals a fresh strict walk; (c) the view never moves |
| - backwards; (d) a strict consumer never receives an incomplete view as |
| - complete. One 3000-seed sweep per revision, niced, per the CPU budget. |
| - Mutation checks: remove bump-after-write ordering, record generations |
| - for failed scopes, skip the before-walk index read in §4.4; each must |
| - fail the simulation. |
| -- Table tests for `ChangeScope`. |
| -- Unit tests for `GetIfChanged` and `ListETags` on both stores, plus MinIO |
| - in the delivered-contract test. |
| + bumping, some bypassing, some failing their bump), incremental |
| + refreshes, backstop walks and consumers, on `DirStore`. Invariants: |
| + (a) after a `Refresh` that observed generation g for a scope, every write |
| + to that scope that bumped to ≤ g is visible in the view; (b) after a |
| + backstop walk, the view equals a fresh strict walk; (c) published epochs |
| + only increase and no scope's contents ever revert to an older read; |
| + (d) a strict consumer never receives an incomplete view as complete; |
| + (e) a scope whose re-read failed is retried on the next refresh even |
| + when the index is unchanged. Named interleavings that must be in the |
| + seed set: the backstop lists an app before a record appears while an |
| + incremental refresh sees and publishes it **(codex 2)**; a pass captures |
| + a snapshot, stalls, and another pass consumes a newer one before the |
| + first reaches `w.pass` **(codex 1)**; an app refresh fails after the |
| + index moved and the next index read is a 304 **(codex 4)**. One |
| + 3000-seed sweep per revision, niced, per the CPU budget. Mutation |
| + checks: bump before write; record generations for failed scopes; |
| + skip the before-walk index read in §4.4; publish the backstop outside |
| + `mu`; consume a handed-over snapshot in the moves pass. Each must fail |
| + the simulation. |
| +- **Scoped-pass witness**: one app's bump causes no reads under any other |
| + app's prefix **(codex 10)**. |
| +- **Gated-read ages**: telemetry, config-gen, canary pointers and certs |
| + are re-read at the age bound with the index unchanged **(codex 9)**; |
| + mesh-key rotation reaches the home map with `control/edges` unchanged, |
| + and the health heartbeat survives unchanged polls **(codex 8)**. |
| +- **Fallback**: a store without the read extensions, and a denied index |
| + read, both produce today's walk, never an empty view **(codex 7)**. |
| +- Table tests for `ChangeScope`; unit tests for `GetIfChanged` and |
| + `ListETags` on both stores, plus MinIO in the delivered-contract test. |
| ## 9. Expected effect |
| @@ -329,7 +445,7 @@ process (#280). |
| | Record walks (LIST) | ~2.0M | ~70k (backstop 48/day/process + changed apps) | |
| | Index + serial polls (GET) | 0.13M | ~0.35M | |
| | Record and manifest bodies (GET) | ~2M | changed only | |
| -| config-gen + canary + certs + accounts (GET) | ~4.8M | changed only + backstop | |
| +| config-gen + canary + certs + accounts (GET) | ~4.8M | changed only + age-bound re-reads | |
| Expected flagship S3: ~$50–90/month (storage $16, transfer ~$25, |
| litestream ~$26, requests ~$10–25), flat as apps are added. |
| @@ -344,4 +460,12 @@ litestream ~$26, requests ~$10–25), flat as apps are added. |
| seconds as today (`configGenLoop`'s 3s poll, canary pointers read every |
| tick) until operators update the CLI. Console and new-CLI writes are |
| unaffected. |
| - Recommendation: accept, and note it in the release notes. |
| + |
| +## 11. Review log |
| + |
| +- Round 1 (Astra, bbae135c): reject, 3 P1 and 8 P2. All accepted: |
| + codex 1 (moves snapshot ordering, §4.3), 2 (backstop publication |
| + ordering, §4.3–4.4), 3 (capabilities, §3.3), 4 (304 retry, §4.2), |
| + 5 (manifests, §4.1), 6 (hostops order, §3.3–3.4), 7 (fleet door, |
| + §3.4), 8 (mesh keys and heartbeat, §5), 9 (telemetry age, §5), |
| + 10 (per-app fan-out, §4.5), 11 (box publication, §3.3, §7). |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-R1.md b/docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-R1.md| new file mode 100644 |
| index 000000000..da3a49e8b |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-R1.md |
| @@ -0,0 +1,23 @@ |
| +1. **P1 — A shared snapshot can move the moves worker backwards (§4.3).** A pass can capture snapshot A after reconciliation, pause, and acquire `w.pass` after another pass has already processed newer snapshot B. Today, the record walk happens **inside** `w.pass` ([internal/host/movespass.go:1061](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/movespass.go:1061), [:1079](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/movespass.go:1079)). This matters because move state selects physical operations, including landing and reverting ([internal/host/movesoffline_land.go:63](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/movesoffline_land.go:63)). Taking a copy “after Reconcile” does not preserve that ordering. **Require:** select and validate the snapshot under `w.pass`, reject superseded snapshots, and define the lock order. Holding `reconcileMu` across the moves pass is unsafe because move operations acquire it themselves ([internal/host/movesoffline.go:1065](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/movesoffline.go:1065)). |
| + |
| +2. **P1 — The backstop can overwrite a newer view (§§4.3–4.4).** The specified wholesale replacement permits this interleaving: a backstop lists an app before a record appears; an incremental refresh sees the creation and reconciliation mints its route; the older backstop finishes and publishes a view without that record. Reading generations before the walk prevents swallowing a concurrent bump, but does not prevent publishing stale absence. Reconcile treats absence as retirement ([internal/provision/reconcile.go:1533](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/reconcile.go:1533)); the existing serialization explicitly protects against this grant-losing race ([internal/host/agent.go:1529](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/agent.go:1529)). **Require:** serialize backstop acquisition/publication with incremental refreshes and consuming passes, or discard/revalidate any rebuild overtaken by a newer publication. Add this exact interleaving to §8. |
| + |
| +3. **P1 — The wrapper omits mutation and streaming capabilities required today (§§3.3, 7).** Forwarding only the three named extensions loses `ConditionalDeleter`: `DeleteIfMatch` explicitly refuses stores without it ([internal/release/conditional_delete.go:29](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/release/conditional_delete.go:29)), and the hostops boundary implements even ordinary deletion through that extension ([internal/hostops/mutation_boundary.go:188](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/hostops/mutation_boundary.go:188)). Merely forwarding conditional deletion would also bypass a hook attached only to `Delete`. `PutFrom` is another independent mutation path, and losing streaming introduces a 64 MiB fallback limit ([internal/release/store.go:260](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/release/store.go:260)). These regressions ship even with indexed readers disabled. **Require:** preserve all existing capabilities, including `ConditionalDeleter`, `PutStreamer`, `Opener`, `SizeLister`, `PageLister`, and `BucketListDirs`; bump after successful mutations through every applicable method. |
| + |
| +4. **P2 — A 304 prevents failed scopes from retrying (§4.2).** After observing index generation 42, suppose an app refresh fails. Its applied generation correctly remains 41, but the next conditional index GET can return 304. Step 1 then returns before retrying the outstanding scope, contradicting step 3. Existing strict readers reject failed record reads rather than returning a complete set ([internal/provision/reconcile.go:1962](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/reconcile.go:1962)). **Require:** retain the observed index separately from successfully applied generations; retry outstanding scopes on 304, preserving incompleteness until they succeed. |
| + |
| +5. **P2 — The view names the wrong manifest and omits the sidecar manifest (§§4.1–4.2).** Provisioning reads `instances.json`, not `manifest.json` ([internal/provision/provision.go:321](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/provision.go:321)). Its contents supply seeds and update policy ([internal/provision/reconcile.go:995](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/reconcile.go:995)). Sidecars separately read `sidecars.json` ([internal/provision/sidecar.go:122](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/sidecar.go:122)); that manifest affects placement and therefore which records enter the removal-protecting set ([internal/provision/sidecar_reconcile.go:328](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/sidecar_reconcile.go:328)). **Require:** explicitly cache both manifests through their existing parsers, retaining their distinct missing/error semantics. |
| + |
| +6. **P2 — The hostops integration is unspecified (§3.3).** Wrapping outside the mutation boundary sends the index PUT through a classifier that has no `control/changes.json` family and rejects unmatched keys ([internal/hostops/mutation_classifier.go:47](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/hostops/mutation_classifier.go:47), [:168](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/hostops/mutation_classifier.go:168)). Wrapping underneath avoids that rejection, but the outer boundary must also expose the new read extensions; its current forwarding surface is explicit ([internal/hostops/mutation_boundary.go:108](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/hostops/mutation_boundary.go:108)). The console constructs multiple layers and a separate control writer ([internal/console/claim_runtime.go:33](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/console/claim_runtime.go:33)). **Require:** specify the exact wrapper order and bump authority, then test through the final claim-enabled store handles. |
| + |
| +7. **P2 — Fleet-door readers cannot read the index (§§3.4, 6–7).** The door allows selected control keys and tenant-scoped prefixes, but not `control/changes.json` ([internal/edge/storedoor.go:431](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/edge/storedoor.go:431)). Its object GET also implements neither conditional GET nor the proposed ETag listing ([internal/edge/storedoor.go:149](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/edge/storedoor.go:149)). Simply exposing the global index would disclose other tenants’ account/app names. **Require:** define a tenant-filtered index protocol with suitable capabilities, or explicitly retain ordinary polling for fleet-door stores. A denied index read must not look like an empty, unchanged deployment. |
| + |
| +8. **P2 — Home-map gating misses mesh-key changes (§§3.2, 5).** `homeMap.Refresh` refreshes both hosts and authentication keys ([internal/edge/homemap.go:320](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/edge/homemap.go:320)). Those keys are `control/edge-mesh-key` and `.next`, outside every proposed scope ([internal/edgeproof/edgeproof.go:58](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/edgeproof/edgeproof.go:58)). Gating the refresh on `control/edges` therefore delays key creation and rotation independently of whether their writers are upgraded. **Require:** give keys an explicit invalidation path or retain their independent polling. Preserve the refresh health heartbeat on unchanged polls too: it expires after five minutes ([internal/lbhealth/lbhealth.go:36](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/lbhealth/lbhealth.go:36), [:174](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/lbhealth/lbhealth.go:174)). |
| + |
| +9. **P2 — Telemetry has no specified backstop (§5).** Unlike config and canary reads, telemetry is gated only on generations. A bypassed or failed bump can therefore leave the cached setting unchanged indefinitely; rebuilding `RecordView` does not refresh telemetry bodies. Today each pass resolves telemetry for every served app ([internal/host/telemetry_pass.go:35](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/telemetry_pass.go:35)). **Require:** a maximum successful-read age for both app and account telemetry, plus first-use reads and error retries independent of index movement. |
| + |
| +10. **P2 — Every index change still causes unrelated per-app reads (§§2, 4.5, 9).** The existing full pass invokes unrestricted adoption and config synchronization ([internal/host/agent.go:2827](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/agent.go:2827)). Adoption reads each distinct app/channel pointer ([internal/release/adopt.go:682](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/release/adopt.go:682)); config synchronization fetches app bundles ([internal/appconfig/syncexec.go:174](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/appconfig/syncexec.go:174)). Optimizing record walks and canary polling leaves this fan-out intact, while adding index-triggered full passes. **Require:** carry changed scopes into these legs or cache their reads with appropriate invalidation and backstops. Demonstrate that changing one app does not re-read every unrelated app. |
| + |
| +11. **P2 — Wrapping the deployment opener breaks box publication (§§3.3, 7).** The publication command uses `openDeployStoreEnv` ([internal/host/boxpublish.go:326](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/boxpublish.go:326)), then requires the returned value to be exactly `*release.S3Store` ([internal/host/boxpublish.go:259](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/boxpublish.go:259)). A decorator fails that assertion even with readers disarmed. **Require:** replace the concrete-type test with an explicit backend capability/identity check and cover publication through the wrapped opener. |
| + |
| +**Verdict: reject at `bbae135c`. P1 findings 1–3 block acceptance: moves ordering, backstop publication ordering, and preservation of existing store operations.** |
| \ No newline at end of file |