BLOCKED: fix(ui): revive the interrupted-migration recovery (blocked on #590, #591)

[sanity]

Scope narrowed twice during review. This began as a fix for "Migrating your rooms…" on every visit (#586) and included a sweep that deleted superseded room copies. Review found that sweep's safety argument false, and then found that making the migration seal durable was itself unsafe. What is left is the one change that is both safe and independently valuable. It does not, on its own, stop the banner — see "What still fixes #586".

Problem

The #345 interrupted-migration recovery has never run in production.

It exists to re-fill rooms stranded by a re-save that was cut short (Nacho's "Freenet Devs" disappeared-after-update), and it is gated on is_legacy_migration_in_progress(), which reads localStorage. The gateway serves the app in sandbox="allow-scripts allow-forms allow-popups …" with no allow-same-origin — pinned by a test in freenet-core's path_handlers.rs, so this is a property of the server, not an accident of one deployment. The document therefore has an opaque origin. Measured live against try.freenet.org:

origin: "null"
localStorage -> THROWS: SecurityError: Failed to read the 'localStorage' property
from 'Window': The document is sandboxed and lacks the 'allow-same-origin' flag.

window.local_storage() maps that to Err, which the reader silently treated as "flag absent". So the marker was never written, the reader was permanently false, and the recovery was dead code. Nothing surfaces that — a stranded room just looks gone.

It is the only mechanism. On the PerRoom path no legacy probe fires at all without this recovery, so a room stranded by a partial migration is stranded permanently today.

Approach

The marker moves into the chat delegate's own key/value store, read back from the current delegate's existing startup ListResponse, so the cold path costs no extra round trip. A delegate-WASM bump mints an empty store, which is the correct default; the LEGACY_DELEGATES fingerprint stays in the key name because legacy_delegates.toml can gain a back-filled entry without the WASM changing. localStorage remains a best-effort mirror, consulted present-wins when the delegate answer is ABSENT — so a same-origin build (dx serve) whose tab closed before the delegate write landed still recovers, which is exactly the case #345 exists for.

Three deliberate constraints:

• The seal stays session-only. It has the same defect, but making it durable is unsafe for two independent reasons. The fixed probes and ListRequests in fire_legacy_migration_request are raw sends holding no LoadWorkerGuard, so PENDING_LOADS == 0 does not prove the fan-out finished and schedule_legacy_seal can fire while a generation is still going to answer. And freenet_synchronizer.rs sets LEGACY_SEAL_PENDING on any "delegate not found" error — which the fan-out provokes within milliseconds on any node missing an old delegate WASM — so a transient all-error fan-out could seal the very node holding the data. A durable seal is permanent with no unseal path, and buys little: a user whose re-save succeeded takes the PerRoom path next session and never probes anyway. Tracked as perf(ui): a user with no rooms re-fires all 81 legacy-delegate probes on every page load #588; the_seal_is_not_delegate_persisted pins the scope.
• A seed never downgrades a marker written earlier in the same session. The delegate write is fire-and-forget, so a reconnect's ListResponse can overtake it. With two responding generations: gen 25's success clears the marker, gen 21 re-marks it, and an absent-authoritative seed would store ABSENT — after which schedule_legacy_seal's "never seal over a migration that is mid-flight or has FAILED" guard reads false and seals anyway.
• The write is raw, not via send_delegate_request. Nothing consumes the response, and registering a waiter would put the StoreRequest and the later DeleteRequest under the same single-waiter correlation key, so the Delete would evict the Store's waiter. It also keeps a 10s timeout off the migration path.

Risks this change carries

Stated explicitly, because reviving a dormant path is not risk-free:

• It turns on, in production for the first time, a fan-out at users who DO have current-delegate data. fire_legacy_migration_request's own doc says it "must only be called once the current delegate has confirmed it has no rooms_data" (bug: old delegates overwriting the current active delegate #253), because a legacy response can trigger a save that overwrites newer state. The Multi-tab room loss: chat delegate rooms_data is a blind full-blob overwrite (last-write-wins across tabs) #345 recovery deliberately violates that precondition; its safety rests on the re-save being per-room CAS read-merge-write, so an already-present room merges rather than being clobbered. That argument has never been exercised in production, because the path has never run.
• It ships the first DeleteRequest the UI has ever sent. delegates/chat-delegate/src/handlers.rs::handle_delete_request has never executed in production. It is correct on inspection (removes the secret, rewrites the key index) and the marker is not CAS-tracked, so versioning.rs's documented ABA hazard does not apply.
• backstop_terminal maps Migrating -> LoadFailed. A recovery that sets Migrating and stalls past the 60s hard max now flips a previously-Loaded user to LoadFailed. Invisible in the room list (rooms present renders List) but it disables Import Identity for the session. Previously unreachable.
• For the fix(ui): "Migrating your rooms…" shows on every visit — the legacy hoard fills the hosted per-user quota and the seal can never persist #586 cohort it adds one small StoreRequest to a delegate whose owner is at 3,998.5 KiB of 4,096 KiB, and the matching Delete never fires because their re-save always fails. Negligible in bytes, but on the wrong side of the quota that is the root cause.
• The marker is one boolean shared across concurrently-migrating generations. The no-downgrade rule protects the seed path; the mutator path is not refcounted, so gen 21 marking, gen 25 clearing, then gen 21 failing leaves it absent. Identical under localStorage, so not a regression — but inert then and live now. Worst case is "recovery does not run", never data loss.

What still fixes #586

The banner recurs because a long-standing user's 4 MiB per-user quota on the hosted node is consumed by River's own duplicate room copies across 27 delegate generations, so the migration's re-save is refused and every visit migrates from scratch. Measured: one user pinned at 3,998.5 KiB of 4 MiB, four full copies of the same rooms in generations 21–25, and not one room slot on the current delegate.

That needs (a) reclaiming redundant copies — with a rule that proves redundancy rather than inferring it from generation rank, which is where the first attempt was wrong — and (b) one-off headroom for users already pinned. Design recorded on #586; a follow-up PR.

Testing

834 river-ui unit tests; full workspace green.

Behavioural: the seeding decision on the real marker key including the no-downgrade rule; which request records which state; marker classification from a key list; fingerprint scoping; load-plan isolation; the reader's cache resolution; and the import-gate window mutators.

Pins, because the native build has no window and cannot execute the localStorage branches at all: the marker is seeded before the load plan is classified; the seeder is unconditional and stores the computed value; both mutators update the cache and persist in opposite directions; persisting stays fire-and-forget, registers no waiter, and never touches the load state; the import gate tracks the live recovery window rather than the durable marker, and that window closes on quiescence rather than at the call site; and the seal stays session-only.

Mutation-checked — each turns a test red: seeder gutted; seeding inverted; Store/Delete swapped; the reader discarding the resolved answer; the reader consulting localStorage first; the mirror fallback short-circuiting on ABSENT; clear not persisting; the seed moved after the plan match; the import gate reading the durable marker; the recovery window never closed; the window mutators swapped; the arming block deleted; and the ! dropped from either quiescence guard.

That last one also closed a pre-existing hole in schedule_legacy_seal's pin, where the same inversion means sealing while a generation is still answering — the #527 third cause.

Scope

UI-only — no delegate/contract WASM, Cargo.toml or Cargo.lock change, so no migration entry is required. Deliberate: minting another generation would add one more duplicate copy to the quota problem. .claude/rules/river-publish.md is updated, since it documented the marker as a localStorage key.

Refs #586, #588

[AI-assisted - Claude]

[sanity]

Review round 1 — four independent blind Claude reviewers (code-first, data-loss, skeptical, testing)

Full tier: this touches delegate storage and, as originally written, deleted user data. Two blockers landed on the deletion sweep and it has been removed from this PR entirely. The scope is now the durable seal only; the PR body has been rewritten to match.

Blocker 1 — the sweep's safety argument was false (code-first, independently confirmed by data-loss)

I claimed #527's generation rank made older copies unreachable state. Verified against ui/src/room_data.rs::merge_from_source — source_rank is consulted only inside the existing.self_sk != room_data.self_sk branch. For rooms whose identity matches (the normal case) room_state is merged as a CRDT union from every responding generation, and the adopt-incoming branch deliberately folds the lower-ranked copy's state in. invitation_secrets is unioned with an explicit comment that dropping older versions "would leave a private-room member unable to decrypt messages sealed under that version".

So deleting older copies is a data decision, not reclamation. The rooms_data / outbound_dms variant is worse: both are whole-collection blobs pruned at key granularity, and sender-side DM plaintext exists nowhere else.

Blocker 2 — "holds K" was inferred from the key list, never from a value read

record_legacy_generation_keys credited a generation from its ListResponse. A generation that lists room:X whose GET returns value: None (a state the loader already handles) or whose bytes fail to parse would have caused readable copies in lower generations to be deleted. handle_delete_request removes the secret before rewriting the index, so a failed index write produces exactly that shape.

Disposition: sweep removed. Reclamation needs the airtight rule instead — delete a generation's key only after the merged union is durably written to the current delegate, and only for keys whose values were actually read and parsed. Tracked separately; it is an efficiency fix, not part of the reported bug.

Findings fixed in the code that stays

Finding	Reviewer	Fix
The in-progress marker could be downgraded by a stale ListResponse, unsealing schedule_legacy_seal's "never seal over a migration that is mid-flight or has FAILED" guard and stranding a generation's rooms	code-first (#3), data-loss (#3)	both flags now share one no-downgrade rule (seeded_flag)
A durable seal written from the speculative doors makes a partial (quota-limited) migration permanent and hides the rooms that never fit — strictly worse than the banner it replaces	data-loss (#4)	mark_legacy_migration_done() is session-only; the durable seal_legacy_migration_done() is module-private and called only from the quiescence-gated completion point
seed_migration_flags_from_current_keys could be made a no-op, or its two key arguments swapped, with CI fully green	testing (P1)	computation extracted to the pure seeded_flags_from_keys, tested on the real key names; pin that the seeder is unconditional and stores both flags
The reader pin was an identifier-order oracle — let _ = …load(…) defeated it	testing (P2)	pin anchored on resolved_flag(<ATOMIC>.load(, so a mention that discards the value fails
persist_delegate_flag had no pin against being awaited or routing failures into mark_fetch_failure	testing (gap 9)	pin added
A new test was inserted between a doc comment and the test it documented, orphaning the V24→V25 rationale	code-first (#7)	adjacency restored
The localStorage-mirror claim was inaccurate — a dx serve user's existing seal was ignored	code-first (#5)	the mirror is consulted when seeding
Stale doc comment asserting is_legacy_migration_in_progress() is "a localStorage read that is always false off-WASM"	testing (P2)	updated, with a warning against testing via the globals

Not adopted

• Two source-scan pins prove presence, not execution (testing P3). Wrapping a call in a runtime-false condition still passes. Accepted: there are no WASM tests in this repo (grep -rn wasm_bindgen_test ui/ is empty), so source scans are the only available net for these paths. Deleting a call outright is caught.
• Testing P2's flake reproduction (a test calling mark_legacy_migration_in_progress() breaks rooms_recovery_in_progress_tracks_pending_loads, 3/3). Real, and the reason no #[cfg(test)] reset_migration_flags() helper was added: rather than mutate process globals under cargo test's parallel threads, the behaviour is covered through pure helpers. The affected test's doc comment now records the hazard.

Reviewers were told not to rubber-stamp and each reported explicitly on categories where they found nothing. Both mutation-testing reviewers left the tree clean (verified: git status empty, md5s match).

[AI-assisted - Claude]

[sanity]

Review round 2 — findings addressed at 5b0135d5

Round 2 reviewed f2cad5e0, which still made the migration seal durable. Two independent arguments killed that, and the seal is now out of scope entirely (the_seal_is_not_delegate_persisted pins it):

1. PENDING_LOADS == 0 does not prove the fan-out finished — the fixed probes and ListRequests in fire_legacy_migration_request are raw sends holding no LoadWorkerGuard, so schedule_legacy_seal can fire while a generation is still going to answer.
2. freenet_synchronizer.rs sets LEGACY_SEAL_PENDING on any "delegate … not found" error — which the fan-out provokes within milliseconds on any node missing an old delegate WASM — so a transient all-error fan-out could durably seal the very node holding the data.

A durable seal is permanent with no unseal path, and buys little: a user whose re-save succeeded takes the PerRoom path next session and never probes anyway. Recorded on #588.

Regression I introduced, found in review

rooms_recovery_in_progress() read the marker directly. That was safe only because the marker was always false in the deployed app — the very defect this PR fixes. Made durable, it would latch: a migration that can never complete (repeatedly quota-refused saves, or a legacy delegate no longer installed) leaves the key set forever, permanently disabling Import Identity — for exactly the cohort whose migration is stuck.

A marker from an earlier session means "the room set may be incomplete", which is what decide_per_room_load_action needs; it does not mean a recovery is running now, which is all the #414 gate cares about. The gate now tracks RECOVERY_PENDING (opened before the recovery, closed when it returns) plus PENDING_LOADS, which covers the workers the recovery spawns. Pinned both ways.

Four mutations that survived round 2, now caught

Mutation	Fix
Seeder gutted to let _ = keys; — the whole PR silently reverted	seeding decision extracted to the pure seeded_marker_from_keys, plus a pin that the seeder is unconditional and stores the computed value
Seeding classification inverted (!marker_present_in_keys) — an interrupted migration read as complete, i.e. the #345 data loss	unit-tested on the real marker key
Store/Delete direction swapped — mark clears the marker and clear sets it, backwards across sessions	extracted to the pure marker_write_request, unit-tested both directions
Reader degraded to let _ = resolved_flag(…) — kept the anchor while ignoring the answer	pin now requires the whole if let Some(known) = … { return known; } shape, and that the return precedes any localStorage read

The reviewer was right that the previous pin's own doc claimed a property the code did not have; both that doc and resolved_flag's were corrected.

Also corrected: the module note claimed localStorage behaviour was "exactly as before" for same-origin builds. It is not — once a seed lands, the delegate is authoritative and a marker existing only in localStorage is ignored. That is the intended direction, but it is a change, and the comment now says so.

Not adopted

Source-scan pins prove presence, not execution — wrapping a call in a runtime-false condition still passes. Accepted: grep -rn wasm_bindgen_test ui/ is empty, so for the localStorage branches source scans are the only available net. Deleting a call outright is caught.

Full mutation set, each turning a test red: seeder gutted; seeding inverted; Store/Delete swapped; reader discarding the answer; reader consulting localStorage first; clear not persisting; seed moved after the plan match; import gate reading the durable marker; recovery window never closed; seal made durable again.

828 river-ui tests, full workspace green. A fresh reviewer is reading 5b0135d5 blind.

[AI-assisted - Claude]

[sanity]

Round-2 verification pass — C1 was wrong, now fixed at 1920eae7

The reviewer re-ran the four mutations that had survived and confirmed all four are now caught, then found my fix for C1 was itself wrong. Two problems, both real:

C1a — the window closed on dispatch, not on recovery. For a per-room user (exactly who reaches the recover branch) the current delegate has no rooms_data blob, so migrate_current_blob_to_per_room(true) lands in the no-blob arm, dispatches the ~81-request legacy fan-out and returns immediately. The stranded rooms arrive later as legacy ListResponses. So end_recovery_window() ran, load_rooms_per_room returned and dropped its guard, and the gate was fully open while 27 generations were still in flight and ROOMS was still missing the rooms being recovered — the precise state #414 forbids.

The reviewer also pointed out my doc claimed PENDING_LOADS covers that fan-out, while the same file 45 lines later argues the opposite — and that argument is the one used to kill the durable seal. If it holds for the seal it holds for the gate; I had it both ways.

Fixed: the window now closes on the same quiescence signal the seal uses — schedule_recovery_window_close, armed from on_load_worker_settled, LOAD_IDLE_MS with idle_should_apply, re-armed by any later responder's settle. It still closes when nothing answers, so it cannot latch. end_recovery_window is now private and the call site is pinned not to close it.

C1b — swapping the two window mutators survived all 828 tests. One edit that both opens the gate during a recovery and latches it shut afterwards, invisibly. Now covered behaviourally, folded into the existing rooms_recovery_in_progress_tracks_pending_loads rather than added alongside it — both touch process globals and libtest runs tests in parallel in one binary, so a separate test would race it. Mutation confirmed red.

C1c — a stale precondition made newly reachable. migrate_current_blob_to_per_room's send-failure arm justified mark_fetch_failure() with "we only reach this fn from LoadPlan::MigrateCurrentBlob, so the blob exists". The recovery branch also calls it, for a user whose index never listed rooms_data, so a send failure there says nothing about stored data — and the recovery is a background re-fill that owns no display state. Now gated on !recovery, with the comment corrected.

I also self-reported to the reviewers that RECOVERY_PENDING looked redundant, since load_rooms_per_room holds a guard across the recover branch. That was right about the inner window and is why closing at the call site bought nothing; the fix is the quiescence close, which covers the window that actually matters.

Rebased onto 240947f2 (main moved: #585 plus its publish). Clean rebase, no conflicts, no mid-sequence edits. 831 river-ui tests, full workspace green; CI re-running on the rebased head.

[AI-assisted - Claude]

[sanity]

Round-3 review — data-loss lens found a HIGH issue this change newly enabled

Fixed at 468bd20d. Reviewed blind by two fresh lenses (big-picture, data-loss) against the final content, since everything before this reviewed a materially different change.

HIGH — the recovery could permanently overwrite a room's identity

Sequence: the marker is PRESENT (the interrupted migration this PR exists to detect). load_rooms_per_room lists rooms A and B; B's slot GET fails, or returns a definitive value: None — which the loader explicitly does not count as a fetch failure. ROOMS = {A}, state stays Loaded, and the recovery fires the legacy fan-out. A surviving generation returns B under an older self_sk. Rooms::merge_from_source runs its #527 rank check only if let Some(existing) = self.map.get(&vk) — B is vacant, so no rank check at all and the legacy copy is inserted. The re-save then reaches reconcile_room_present, whose diverged-identity branch keeps the saving session's copy (pinned by reconcile_room_present_diverged_identity_keeps_local), CAS-writing the legacy identity over the current one. self_sk cannot be re-derived from the network.

The delegate had B all along — only this client's read failed. I verified the merge_from_source and reconcile_room_present behaviour directly rather than taking the report on trust.

Fix: recovery_is_safe_to_run(listed, materialised, had_fetch_error) — the recovery runs only when every listed room materialised and no fetch error was seen. Deferring is now safe, because the marker is durable: a genuine strand is recovered on the next session whose load comes back clean.

The other four

Finding	Fix
My !recovery gate on mark_fetch_failure rested on a false premise — plan_load_from_keys prefers PerRoom whenever any per-room key exists and a blob-explosion leaves the blob as a rollback fallback, so per-room keys and a real blob routinely coexist	gate on has_blob, which plan_load_from_keys already computed and discarded; now carried through LoadPlan::PerRoom
A generation whose re-save fails assumed the marker was still set, but a sibling generation's success may have cleared it — leaving the next session to conclude the per-room set is authoritative while this generation's rooms were never written	the failing arm re-asserts the marker
clear sent a DeleteRequest unconditionally; handle_delete_request rewrites the delegate's whole key index even when the key is absent (unlike Store, which rewrites only when adding a new key), and this is the first DeleteRequest the UI has ever sent	only sent when the marker is believed present
RECOVERY_PENDING's doc claimed an invariant that holds for the reconnect scenarios but not on the recovery path, which deliberately leaves the state Loaded	corrected, and the early-close exposure stated rather than implied

Mutation results

All five turn a test red: completeness gate always-true; gate consulted but not negated; the mark gate reverted to !recovery; the failing arm no longer re-asserting; clear deleting unconditionally. The middle two survived the first attempt and needed pins of their own — recorded because "I fixed it" and "a test would catch it being un-fixed" are different claims.

From the big-picture lens

Verified the premise independently from freenet-core source — the missing allow-same-origin is pinned by a test in path_handlers.rs, so it is a property of the server, not one deployment. Confirmed this is not theatre: on the PerRoom path no legacy probe fires at all without this recovery, so a room stranded by a partial migration is stranded permanently today, and this is the only mechanism. Also confirmed the PR's claim that it does not fix #586 is accurate, and that reviving the recovery cannot introduce the banner for any cohort that doesn't already see it.

Their asks are done: .claude/rules/river-publish.md corrected (it still documented the marker as a localStorage key), the mirror now falls through present-wins so same-origin builds keep the coverage they had, and the PR body carries a risk section covering #253's deliberately-violated precondition, the first-ever DeleteRequest, and backstop_terminal's newly-reachable Migrating -> LoadFailed.

One dismissal, put back to the reviewer rather than closed quietly: fire_legacy_migration_request's mark is deliberately not recovery-gated. The arm I did gate rested on a precondition that is false under recovery; "a probe failed to send, so we could not check this generation" is equally true there, and holding the #414 import gate shut is the safe direction when rooms may still be missing. Reasoning is in the code at the call site.

838 river-ui tests, full workspace green. Rebased onto 3904382f.

[AI-assisted - Claude]

[sanity]

BLOCKED — do not merge. Reviving the #345 recovery is not safe as written.

The data-loss reviewer answered the question I was blocked on — is "every listed slot parsed" sufficient to exclude the vacant-slot path? — with no, and the reasoning is structural rather than a missed case:

recovery_is_safe_to_run is computed from a count of fetch results, taken synchronously. The hazard is merge completeness, and the merge is deferred, so it has not run when the gate is evaluated. The gate measures the wrong stage. Any gate on the read side will be a whitelist of the drop paths you happened to think of.

That is correct, and it invalidates my fix as a general solution rather than just finding a gap in it.

The worse case: a legacy tombstone deletes a live room — filed as #590

Verified in code, not taken on report: migrate_legacy_per_room pushes RoomSlot::Tombstone into slots like any other slot; reconstruct_rooms turns those into removed_rooms; hydrate_loaded_rooms passes loaded_rooms whole to merge_from_source, which unions the incoming tombstones and immediately retains the map against the combined set. So a legacy generation's tombstone evicts a room the CURRENT delegate holds Present, and the re-save then CAS-writes that tombstone through. The room's self_sk cannot be re-derived from the network.

No failure is required. A user who left a room under one delegate generation and rejoined it under a later one is enough.

#590 is reachable on main today, via the ProbeLegacy path, independent of this PR — which is why it is filed separately and matters more than this PR does. This PR would extend it to the PerRoom path, i.e. to users who definitely have current-delegate data.

hydrate_loaded_rooms's justification for trusting incoming tombstones — "legacy delegates predate the tombstone field (#247)" — is stale: legacy_delegates.toml gains an entry on every WASM bump, so recent legacy generations carry per-room tombstones routinely.

Also: #591, merge_from_source aborts mid-loop

Two ? on a fallible merge inside the per-room loop mean one merge error drops every room later in a HashMap's arbitrary iteration order. Those become vacant slots, which is precisely where the #527 rank check does not run — so they take the original identity-overwrite path.

Disposition

I am not merging this. The change is correct about the defect it names — the #345 recovery genuinely has never run in production, and the big-picture reviewer independently confirmed it is the only mechanism for a room stranded by a partial migration — but turning it on is what makes #590 and #591 reachable for users who currently are not exposed to them. Shipping a fix whose effect is to enable permanent room deletion for a realistic user is not a trade worth making, and the correct fix is not a read-side gate at all.

The reviewer's suggested shape, which I think is right: scope the recovery's write rather than predict the completeness of the read — have the recovery's re-save touch only rooms the recovery itself introduced. That closes the identity-overwrite path and #591's consequence without enumerating merge behaviour. #590 needs its own fix regardless, and should land first since it is live.

Staying open as a draft, blocked on #590 and #591.

Confirmed good, for whoever picks this up

• has_blob threaded through LoadPlan::PerRoom is the right predicate, and the gate refusing before arm_legacy_migration_recovery() matters — a refusal must not burn the once-per-session arm.
• Not touching the delegate WASM for handle_delete_request's unconditional index rewrite was the right call: fixing a latent handler bug by minting another delegate generation triggers the exact failure mode it guards against. Client-side mitigation only. Wants a tracking issue so it lands with the next delegate bump that happens for other reasons.
• One thing still unfixed here: the blob-explosion Err arm does not re-assert the marker, while the legacy re-save arm now does. Same bug, same file, ~700 lines apart — exactly what a pin on one arm passes through.
• H6's cross-tab variant survives both fixes: process globals are per-tab, the delegate store is not.

[AI-assisted - Claude]

[sanity]

Full-tier review result: DO NOT MERGE — the suite cannot detect whether this change works

Rebased onto current main (e26a6499, clean rebase, no conflicts). CI green except Playwright. Reviewed by four independent blind lenses — code-first, testing, adversarial, big-picture — plus a migration-safety pass run in-session. 854 unit tests pass.

The finding that decides it

The marker can be neutered at all 7 mark sites and all 854 tests still pass. Verified directly: inserting clear_legacy_migration_in_progress() immediately after every mark_legacy_migration_in_progress() makes the marker dead on arrival and this PR a complete behavioural no-op. Suite stays green.

The testing lens ran 29 mutations; 10 survived (11 of 36 including the ranking sweep below):

#	Mutation	Consequence
C1	clear_…() hoisted above the save (response_handler.rs:1959)	clears on FAILURE
C2	blob-explosion clear deleted (:1132)	recovery re-runs forever — the "migrating every visit" symptom
C3	clear inserted after every mark	entire PR is a no-op
M4	seeder stores FLAG_ABSENT keeping both pinned tokens	recovery still never runs — the exact bug this PR exists to fix
M5	gate args slots_len → listed_count (:957)	completeness check becomes a tautology
M7	if was_present Delete gate dropped	needless whole-index rewrite
M11	seal persisted via one level of indirection	the_seal_is_not_delegate_persisted does not pin the seal
M13b	early return before the recovery-close arming	RECOVERY_PENDING latches; Import Identity disabled all session
M16	persist_migration_marker never sends	degrades to an in-memory flag that dies on reload
M26	persist writes a key the seeder never reads	marker never observed across sessions; orphan key accrues

M4 and M5 are BLOCKERs on their own. M5 in particular unpins the gate guarding the path where a vacant room slot adopts a legacy copy with no rank check and the re-save CAS-writes a legacy self_sk over the current one — not recoverable from the network.

The pattern is exact: every pure function is tested; every wiring step between them is a token-presence pin. The clear side has no behavioural coverage whatsoever — the only clear-side assertion (response_handler.rs:2400) is a contains(...) over a slice, which cannot see position or condition.

Why this is worse than a normal test gap

No delegate holds the marker key today, so at rollout every user seeds FLAG_ABSENT and behaves exactly as on main. The recovery only goes live for migrations interrupted on this build or later. That is genuine de-risking for the deploy itself — and it also means none of these findings would be observable after deploying. First evidence arrives at the next delegate-WASM bump, when the migration cohort is largest.

So both verification channels are blind at once: the tests cannot tell us it works, and production will not exercise it for a long time. That is the wrong combination for a path whose failure mode is unrecoverable key loss.

Must fix before merge

1. Seed → reader driven end to end. the_seeder_stores_the_computed_marker (chat_delegate.rs:945) asserts three tokens and does not exclude the gutted body its own docstring names. The testing lens prototyped a replacement and confirmed it kills 5 survivors. Must be ONE test (process globals + parallel libtest threads).
2. Pin the gate's full call literal at :957, as the sibling pin already does for migrate_current_blob_to_per_room(true, has_blob).await; — that one does catch its argument mutation.
3. Real clear-side coverage. No source pin can express this: the property is a temporal relation between the fan-out's completion and one global write. It needs an explicit outstanding-sources counter. That is also the fix for the strand below.
4. persist_migration_marker should choose its own key rather than taking it as a parameter, so the existing round-trip test covers production's choice for free (kills M26).
5. One coupling test for room_data.rs:1993 — the arm protecting an in-session-created (unranked, live) room from a legacy tombstone. The recovery fan-out merges as OlderSnapshot, so a room created while the recovery window is open is exactly that case, and a just-created room's self_sk exists nowhere but memory. Deleting this arm passes all 854 tests.
6. Fix the inverted comments (see below).

The strand bug

clear_legacy_migration_in_progress() (response_handler.rs:1959) clears unconditionally, with no knowledge of a generation that already failed. Generation A's save fails and re-asserts; B's succeeds and clears; next session sees no marker → mark_done → A's rooms stranded permanently. The Err-arm re-assert this PR adds correctly closes success→failure; failure→success is open. One boolean cannot express "every source discharged".

Same shape one layer down: if B is cut short by the tab closing, no Err arm runs at all, and the in-memory no-downgrade rule is session-scoped — the session is gone. Tab-close mid-migration is the literal scenario #345 exists for.

Comments that actively mislead — raised above cosmetic

recovery_is_safe_to_run's docstring (chat_delegate.rs:6706-6712) says the gate leaves "two known holes it does not close: #590 … and #591". Both are fixed in this branch's own base (#593, 01aac14c) — #590 by the ranked-tombstone logic at room_data.rs:1957-2036, #591 by the per-room error accumulation at :2038. The comment therefore tells a future editor that the ranking is not load-bearing for #587, when it is the only thing standing between the recovery fan-out and a self_sk overwrite. That is precisely the misreading that would produce the item-5 deletion above.

Also: :6714-6717's "load-bearing" claim that merge_from_source skips any vk in removed_rooms is now conditional post-#593 (presence_survives_tombstone re-admits on a strictly newer source); and the header at :6994-7000 claims the delegate is authoritative once seeded while the reader short-circuits only on Some(true), so a seeded ABSENT falls through and localStorage wins. The code is the safer of the two — the comment, .claude/rules/river-publish.md:200, and the pin-test doc at :800 are what need correcting. Plus the doc splice at :6718 that left decide_per_room_load_action undocumented.

Newly reachable dead ends — flagged, not fixed here

Both are UX traps rather than data loss, and fixing them means touching the load-state machine. Proposed as follow-up issues rather than widening this PR:

• A single failed probe among the ~81 sets SAW_FETCH_FAILURE, which disables Import Identity for the session. For users who have rooms, room_list_display_state returns List before consulting load state, so the Retry button (room_list.rs:458) is unreachable. Clears only on the next reconnect; nothing tells the user.
• migrate_legacy_per_room writes Migrating unconditionally (response_handler.rs:1254), so the 60s backstop maps Migrating → LoadFailed (chat_delegate.rs:7355) and can flip a resolved Loaded user to LoadFailed.

Also pre-existing and worth its own issue: recovery_is_safe_to_run's docstring says a definitive value: None means the delegate still holds a room we failed to read, while the handler 130 lines away treats it as a legitimate skip (:844-848, pinned at :2939). If the new doc is right, Import Identity stays ENABLED while ROOMS is knowingly incomplete — the #414 loss. This PR documents the hole without closing it.

Corrections made during review

Two claims raised mid-review were checked and withdrawn — recorded so they are not carried forward:

• "The blob arm strands exactly the cohort this change serves." Overstated. No production code path writes rooms_data (both write sites are in mod tests), so a legacy→per-room migration cannot leave a blob on the current delegate. A blob implies a pre-Multi-tab room loss: chat delegate rooms_data is a blind full-blob overwrite (last-write-wins across tabs) #345 build, which routes to MigrateCurrentBlob — meaning the marker was set by the blob explosion and re-exploding it is the correct recovery. Reachable residual is only the thin blob-listed-but-value-gone case. Still worth fixing the unconditional clear; not the primary risk.
• "A future editor could loosen the fix(ui): stop a legacy delegate generation from deleting or dropping rooms #593 ranking and all tests stay green." Wrong for the legs named. Seven ranking mutations were run; six go red, several tripping multiple tests. resolve_identity_conflict, presence_survives_tombstone, and the incoming_rank <= live_rank skip are each well pinned. The single gap is the in-session-created arm at room_data.rs:1993 — hence item 5, narrowed from a sweep to one test.

What is NOT wrong with this change

The diagnosis is solid and independently verified: the gateway serves River in a sandbox iframe with no allow-same-origin, window.origin === "null", every localStorage access throws SecurityError, and River's if let Ok(Some(storage)) readers silently treat that as "absent" — so the #345 recovery has never run in production. The scope claim ("does not, on its own, stop the banner") is true. No tests were removed or weakened; the four changed assertions are strict strengthenings. No delegate re-key: three files touched, no delegates/ source, no .wasm, no legacy_delegates.toml, both committed WASMs byte-identical to main, and both migration checks pass. The risk section in the PR body is unusually candid.

The change earns its place. It is not verifiable yet, and that is what needs doing before it lands.

Review lenses: code-first, testing (mutation-driven), adversarial, big-picture, migration-safety.

[AI-assisted - Claude]