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 <DataDir>/.moves/<x>/ (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 <DataDir>/.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.