# Final whole-branch review, fix wave R1: report Branch `s3-polling-336`, base of this wave `4512018a` (the review findings commit). Findings: `docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-CODE-R1.md`. All seven findings are fixed under the controller's binding rulings. Nothing pushed. ## Commits | Commit | Finding(s) | Subject | |---|---|---| | `cd418ebf` | F3 | release, provision, host: any failed index poll leaves the watch unknown; record view polls are bounded; no canary gate on a tick that could not read the index | | `c1d29378` | F2, F4 | provision: a snapshot pass decides removals from the bucket's own walk | | `0c3e3ebc` | F1 | host: the moves pass acts only on its own walk of the bucket; the record view only says whether there is move work | | `46a738be` | F5 | host: with no change index armed the telemetry pass is main's, one resolve per app | | `6ca3bf3c` | F6 | host: a canary group is settled only by its own pointer read, at the generation and age the gate asks | | `5e0af511` | F7 | edge: an unchanged cert poll retries a failed local copy from the snapshot it holds | | `25eac78c` | F2, F4, census | provision: Reconcile and ReconcileSidecars stay whole units (scan and retire closures); census overlays re-pinned for the review fixes | `25eac78c` reshapes `c1d29378`: the first version split the record walk and the rest of each pass into new functions, which renamed legacy-accepted census units (B8 and plan14) and would have made the census owner re-rule them. The final version keeps `Reconcile` and `ReconcileSidecars` whole, with the walk and the removal phase as closures; behaviour and tests are identical, and only digests and basis move in the census. ## F1 (P1): the moves pass acts only on live data Ruling applied as written. The snapshot is used only to decide whether there is any move work. - `internal/host/movespass.go:1077` `idle := w.movesIdle(ctx, s)`, computed inside `w.pass` before the outbox flushes; `:1092` returns after the flushes (they still run every pass, as on main) when idle; otherwise `:1097-1098` walks `provision.ListAllRecordsStrict(ctx, s)` over live, and the pass runs exactly as on main. - `:1296` `walkedIdle` is set at the end of a walked pass: no record with a move, nothing undecodable, and `localMoveState()` false. Every early return leaves it false (set false before the walk). - `:1307 movesIdle`: idle only when the view vouches (`viewStore` ok), the last pass walked and was idle, the snapshot's strict listing has no record with `Move` set and no undecodable record, and the worker holds no local state. - `:1324 localMoveState`: wake fences (`fenced`), the strict set, the hold view (unbuilt, or any claim/unknown), the offline keep view (unbuilt, failed, or any move/unreadable host), T5 archive verdicts, landing leases, and `:1356 movesDirHolds` on disk: anything under `/.moves//` (op journals; parked journals leave the tree) or the page outbox. An unreadable directory counts as holding something. Not counted, on purpose: the move-log memo, `nudged`, `trouble`, `notices` (dedup and log throttles; they never need a walk to be correct). - Epoch guard: **removed** (field `lastEpoch` and its refusal), since no move step reads the snapshot any more. The view's monotonic publication is still asserted by the simulation (invariant c). - `refreshOfflineView` always reads live: `internal/host/agent.go:1636` now passes `s`, not the view store (start-up call at `agent.go:2985` was already live). - Spec §4.3 "Moves" bullet rewritten to match; `viewStore`'s doc comment in `internal/host/changeindex.go` updated. Tests (`internal/host/changeindex_test.go`): - `TestMovesPassNeverActsOnAnOlderSnapshot` (the regression Astra asked for, legacy-abort flavour): an abort-complete write whose bump fails; an unrelated scope fails to re-read so the pass walks live (fallback, sees no move); a new move of the host is requested with its bump failing and staging fills; the unrelated scope recovers, so the view is Complete again holding the old `aborting` state (asserted). The pass must not delete the new move's staging. RED on HEAD's moves pass: `the pass acted on the snapshot's old abort and deleted the new move's staging: [delete data-uk/moves/a.go.example.com/generations/g/snapshot delete data-ie/move-receipts/a.go.example.com.json]`. GREEN after. - `TestMovesPassSkipsTheWalkOnlyWithNoMoveWork`: an idle armed pass makes no record-family listing; a worker holding a wake fence walks. RED on HEAD: `a worker holding a wake fence skipped its walk`. GREEN after. - `TestMovesPassDecidesFromItsOwnRefresh` replaces `TestMovesPassTakesSnapshotUnderPass` (it asserted the removed epoch): pass A paused after its reconcile, a move requested, A's moves leg sees it from its own refresh under `w.pass` and walks. GREEN. - `TestMovesPassRefusesOlderEpoch` deleted with the guard. - `TestReconcileUsesViewWhenArmed` updated: the unbuilt offline keep view now lists the bucket once (from live), and two later passes list nothing. ## F2 + F4 (P1): removals over a snapshot are decided by the bucket's walk Ruling applied: per-row confirmations replaced by one live view of the facts per pass, built on the first removal candidate, by the walk's own logic. - `internal/provision/reconcile.go:835` `scan := func(st release.Store)`: the record-collection phase, unchanged, over a store argument. `:1120` `trashOver(st, pre)`: the trash lookup over a store (the pass's own over `s` with its prefetch; the live one over the bucket with none). `:1542` `retire(recs, protect, trashFor, stopAtFirst)`: the removal phase, unchanged except that with `stopAtFirst` it removes nothing and reports whether any row would go. Over a snapshot (`onSnapshot(s)`) the pass calls `retire(..., true)`; on the first candidate it runs `scan(freshStore(s))` (`:1614`) and, if that walk read cleanly, `retire` over the live facts and `trashOver(live, nil)`, which decides every removal that pass (including rows that were not candidates on the snapshot). A live walk that errs or sets `readFailed` keeps every row that pass. - `internal/provision/sidecar_reconcile.go:210` `scan`, which also returns the per-app sidecars manifest lookup (the mint loop keeps using the snapshot's), so live placement uses the bucket's record and manifest against `cfg.Inbound`. `:499 retire`, `:538` the live re-scan. - Snapshot passes with no candidate read nothing more; non-snapshot stores walk once, as before. - `confirmRecordGone`, `confirmSidecarGone`, `confirmTrashed` deleted from `internal/provision/recordview_store.go`; their reasoning now sits on `onSnapshot` (`:331`); `walkErrSuffix` (`:339`) formats the log line. - Spec §4.4 paragraph "Removals are confirmed in the bucket" rewritten as "Removals are decided by the bucket's walk". Tests (`internal/provision/recordview_test.go`, new `removalWorld` fixture over a bumping store plus a non-bumping handle on the same directory): - `TestSnapshotSidecarPlacementChangeEvacuates/record` and `/manifest`: placement becomes no-inbound in the record or in the sidecars manifest, bumps succeed, inbound box. RED: `a no-inbound sidecar stayed on an inbound box over the snapshot` (both). GREEN. The same test asserts a snapshot pass with nothing to remove makes no bucket listing. - `TestSnapshotRemovalProtectsAnInvalidOutOfScopeRecord`: the live record (unbumped) has an empty channel and an out-of-scope region. RED: `route one.example.com missing`. GREEN (kept, and the plain walk agrees). - `TestSnapshotRemovalKeepsARehomedHost`: the same host moves to another app's key without a bump. RED: `route one.example.com missing`. GREEN; the same test then deletes it everywhere and the row is removed (really gone still removed). - `TestRemovalOverTheBucketWalksOnce`: disarmed path unchanged, one `List accounts/` per pass and the removal happens. GREEN before and after (a guard). - Existing `TestSnapshotAbsenceDoesNotRetireALiveRecord`, `TestSnapshotTrashMarkerIsConfirmedBeforeRemoval`, `TestSnapshotAbsenceDoesNotRetireALiveSidecar` pass unchanged. ## F3 (P1): stalled index GET, frozen observation - `internal/release/changewatch.go:76-81`: a Poll whose ctx has already ended or that gives up waiting for the poll slot now calls `w.fail`, so the watch is unknown (`Gen`/`Observed` not ok, `Usable` false) until the next good poll. ETag and entries are kept, so recovery is a 304. - `internal/provision/recordview.go:201` `RecordView.poll` bounds every index poll with `pollTimeout` (`release.ChangeIndexPollTimeout`, set in `NewRecordView` at `:62`, a field so provision tests can shorten it), derived from the caller's ctx. Refresh's failed poll walks without polling again (`rebuildLocked(ctx, false)`, `:126`, `:130`). - Agent: **both** protections. The watch is unknown after any failed poll (so the canary gate finds every group due), and `fastTick` installs the canary pointer gate only when its own index observation succeeded (`internal/host/agent.go:2749`, `d.canaryGate != nil && indexOK`), which also covers another caller's poll succeeding in between. Tests: - `TestChangeWatchPollLockWaitHonoursContext` updated to the new rule (the give-up makes the watch unknown; after the stuck poll answers, the generation is back). RED: `a give-up on the lock left the watch usable: its observation is frozen`. New `TestChangeWatchCancelledPollIsUnknownThenA304`. RED: `a cancelled Poll left the watch usable`. Both GREEN. - `TestViewStalledIndexIsBounded` (provision): index GET hangs while the bucket answers; Refresh and Rebuild each return, walk, publish a Complete snapshot with the new record, poll the index exactly once each, the watch is unknown; after the stall, a refresh recovers and the next quiet refresh is one conditional GET. RED: `Refresh over a stalled index did not return: its poll is unbounded`. GREEN. - `TestStalledIndexGetKeepsReconcileWalking` (host): armed agent, a stalled index GET; `reconcileSerialized` returns within the bound and mints the new record (it walked), the watch is unknown, a gate marked before the stall is due again, and the next good poll recovers the watch. RED against HEAD's RecordView: `a reconcile pass hung behind a stalled index GET`. GREEN. - `TestCanaryPointerReadWhenTheTicksIndexPollFails` (host): another caller's index GET hangs holding the slot; the tick gives up and must read the canary pointer; after the stall, the quiet tick is gated again. RED against HEAD's ChangeWatch: `a tick whose index poll gave up skipped the pointer read (1 reads, want 2)`. GREEN. - Edge tests re-checked unchanged: `TestHomeMapStampsRefreshWhileTheWatchIsStuck`, `TestCertMirrorPollsPastAStuckWatch` and the rest of `-run 'Cert|HomeMap|Mirror|Mesh|Health'` pass; they assert the poll and the heartbeat finish, which still holds. ## F5 (P2): telemetry unchanged while disarmed - `internal/host/telemetry_pass.go:87`: `telemetrySwitchesWith` over a nil watch (`r == nil || r.w == nil`) runs `telemetrySwitchesResolve` (`:136`), main's `telemetrySwitches` body verbatim (one `telemetry.Resolve` per account/app). Memo and cross-pass values apply only when armed. - `TestTelemetryAccountReadOncePerPass` now runs over a polled watch (armed). - New `TestTelemetryDisarmedResolvesEachAppOnItsOwn`: three apps of one account, the first account read fails once; disarmed, the account file is read three times and two apps stay on. RED: `three apps read the account's telemetry.json 1 time(s) disarmed, want 3`. GREEN. ## F6 (P2): one canary channel suppressing another's retry - `internal/host/agent.go:2201` `canaryRead{head, gen, at}` stored per (account, app, channel) in `canaryHeads`; the read callback records the generation `Due` returned and `now`. `:2823 canarySettled(k, routes, gen, now)` requires the group's own `gen` to equal the one `Due` returned and its own read to be younger than `ReadGate.MaxAge()` (new, `internal/release/changewatch.go:204`), then every route live on its head. - New `TestCanaryGroupFreshnessIsPerChannel`: canary/dev and canary/beta of one app; dev's promote is held out by a restart hold while beta reads and marks the app scope; when dev's hold ends it reads again and adopts v2. RED: `dev's pointer read 1 time(s) after its hold ended, want 2: beta's read settled dev's group`. GREEN. ## F7 (P3): cert local-copy retry - `internal/edge/certsync.go`: `syncLocal` returns whether any entry failed; `publish` stores it in `localPending` (`:932`); the armed poll's unchanged branch retries `syncLocal` against `m.snapshot()` when pending (`:763-764`). No bucket read. - New `TestCertMirrorRetriesAFailedLocalCopyOnAnUnchangedPoll`: local cache refuses Puts on the first poll; the next unchanged poll writes it with zero bucket reads; a settled unchanged poll writes nothing. RED: `an unchanged poll after a failed local copy left the local cache without it: acme/autocert: certificate cache miss`. GREEN. ## Census changes (for the #280 request amendment) All in this branch's own unratified overlays, still `PROPOSED-s3-change-index`. `internal/provision/testdata/plan14-root-reads-s3-change-index.tsv` (seal in `plan14_root_reads_test.go` moves from `7d4b63ec…` to `1eccb15a9cc0a215c4c3addb818b6310f9dd76fc6f8aba38038254b6037dfee6`): - Re-pinned (class unchanged, `legacy`/`accepted-legacy`; new digests, basis rewritten for what they now do): `internal/provision/reconcile.go:Reconcile` (scan and retire closures; a snapshot removal is decided by a second walk over the bucket; relative to main a removal can only be withheld), `internal/provision/sidecar_reconcile.go:ReconcileSidecars` (same, with live placement), `internal/host/agent.go:fastTick` (canary gate only on a tick whose index observation succeeded), `fastDeps.canaryPointerGate` and `fastDeps.canarySettled` (per-channel generation and age), `internal/host/telemetry_pass.go:telemetrySwitchesWith` (hands the disarmed pass to main's body). - Removed (branch-own rows, functions deleted): `internal/provision/recordview_store.go:confirmRecordGone`, `confirmSidecarGone` (both the `@legacy` declaration and the entry). - Added: `internal/host/movespass.go:movesDirHolds`, `legacy`/ `accepted-legacy`, basis root-input (J4, as `movesWorker.sweepFinalJournals`): two `file:ReadDir` sites listing `/.moves` and its first level, only to decide whether to walk. `internal/host/telemetry_pass.go:telemetrySwitchesResolve`, `legacy`/ `accepted-legacy`, basis J1 root-route: main's `telemetrySwitches` body under a new name, same six sites at the same relative offsets. - Reworded: the `@removed` H3 row for `telemetrySwitches` (still declared, still reads nothing; it now reaches main's body through `telemetrySwitchesWith` when disarmed). - Envelopes moved: `internal/edge`, `internal/host`, `internal/provision`, `internal/release`. Header line "What changed" updated. `internal/provision/s3_change_index_census_test.go` (B8 record writes): re-pins `Reconcile` (`b93306e1…`) and `ReconcileSidecars` (`cc2aa4db…`), class `key-site` and kinds unchanged; the comment describes the closures and the live re-walk instead of the confirmations. Console authority and invariant census: no change needed (passes). ## Covering runs All with the sandbox off, GOCACHE default, serial and niced, at `25eac78c`. - `go test ./internal/release/ -run 'ChangeWatch|ReadGate|ChangeIndex|PassFilter|Adopt'`: ok (3.5s). - `go test ./internal/provision/ -run 'TestView|TestSnapshot|TestRemoval|Reconcile|Sidecar|RecordView|Census|Plan14|Plan9'` (includes `TestRecordViewSimulation` at its default 50 seeds, the plan14 root-read census and the B8 record-write census): ok (31.1s). - `go test ./internal/host/ -run 'Moves|Offline|ChangeIndex|Canary|ConfigGen|Telemetry|FastTick|Scoped|Backstop|Reconcile|Instances|Stalled|Gates'`: ok (145.3s). - `go test ./internal/edge/ -run 'Cert|HomeMap|Mirror|Mesh|Health'`: ok (9.8s). - `go test ./internal/console/ -run 'RunSweep|SeedSweep|Census|Authority|Invariant'`: ok (195.6s), run after every code change. - `nice -n 19 go test ./internal/provision/ -run TestRecordViewSimulation -seeds 3000 -count=1`: ok (286s), one sweep for this revision. - `go vet ./...`: clean. `gofmt -l internal cmd`: empty. - `GOOS=darwin GOARCH=arm64 go build ./...` and `GOOS=windows GOARCH=amd64 go build ./...`: both build. ## Concerns 1. F1 cost and limit, as the ruling accepts: a move started by a write whose bump failed is not seen until the backstop rebuild (or any pass that walks for another reason). A box that keeps local move state on disk (an op journal that never reaches its stamp, a root-owned page entry) walks every pass, which is main's cost, not worse. The disk check counts any non-empty first-level directory under `.moves`, so unrelated leftovers there would also keep the walk on; I judged over-walking the safe direction. 2. F1 offline flows: skipping the walk also skips the per-pass refresh of the offline keep view, which keeps the last walked state (no moves, nothing undecodable, by the idle rule). A new offline move whose request bump failed cannot flip without the source box's report, and the source only reports from a pass that walked, so I see no path to a row being removed early; worth a reviewer's second look. 3. F2/F4: on a snapshot pass with a candidate, log lines from the removal phase ("not removing X: participant ...", "protected") can appear twice, once from the snapshot facts and once from the live ones. Cosmetic. 4. F2/F4 granularity: the live re-walk is a whole-deployment walk, once per pass that has any removal candidate (the ruling's cost). Candidates that persist (a row whose live walk keeps it, e.g. a protected record) make every snapshot pass walk until the record is fixed; that is main's cost for those passes. 5. F3: `TestStalledIndexGetKeepsReconcileWalking` and `TestCanaryPointerReadWhenTheTicksIndexPollFails` each wait out the real 5s `ChangeIndexPollTimeout` (host serial and parallel phases, about 10s added in total); the provision test shortens the bound through the new unexported `RecordView.pollTimeout` field. 6. Census: all changes are in this branch's own PROPOSED overlays and need the #280 request amended (list above). New rows `movesDirHolds` and `telemetrySwitchesResolve` are legacy/accepted-legacy like their siblings in this overlay; the census owner may prefer different bases. 7. Pre-existing, not touched: the branch's earlier tests in `recordview_test.go` (`TestSnapshotAbsenceDoesNotRetireALiveRecord`, `TestSnapshotTrashMarkerIsConfirmedBeforeRemoval`, `TestSnapshotHoldRereadReadsTheBucket`) reuse the old `setup` fixture, which uses tenant-like names (`tito`, `go.tito.io`) against the "no tenant names" rule. My new tests use `shop`/`example.com`; the move regression reuses the moves harness's `a.go.example.com` and account `bab`.