fix(ui): stop a legacy delegate generation from deleting or dropping rooms

[sanity]

Two ways the legacy-migration merge loses rooms permanently. Both are live on main. Found reviewing #587, which would have widened the first.

1. A legacy generation's tombstone deletes a live room (#590)

migrate_legacy_per_room pushes whatever slot it parses into slots, including RoomSlot::Tombstone. reconstruct_rooms turns those into removed_rooms. hydrate_loaded_rooms passes the whole Rooms to merge_from_source, which unions the incoming tombstones and immediately evicts:

for vk in other.removed_rooms { self.removed_rooms.insert(vk); }
self.map.retain(|vk, _| !self.removed_rooms.contains(vk));

So an older generation's tombstone deletes a room the current delegate holds Present, and do_save_rooms_to_delegate's tombstone pass then CAS-writes that tombstone over the current slot. self_sk cannot be re-derived from the network — the room is gone for good.

No failure is required. Leave a room under generation G; a WASM bump makes G legacy; rejoin the room under the new generation; any later load whose current-delegate index is empty fires the fan-out and destroys it.

Root cause

The merge treated every responding generation as a peer. It isn't — a legacy generation is a strictly older snapshot. source_rank was already threaded through, but consulted only for self_sk conflicts, so nothing constrained what an old snapshot could delete.

merge_from_source now takes a MergeAuthority:

• Authoritative — the current delegate, or a deliberate in-session action. Unchanged: its tombstones remove rooms.
• OlderSnapshot — a legacy generation. Additive only. It may contribute a tombstone for a room the live set does not hold, so "a room the user left stays left" survives; never for one it does.

The asymmetry is deliberate and one-directional. Worst case a left room reappears and the user leaves it again; the alternative is unrecoverable.

The justification this replaces — the receiver's tombstone set is authoritative "because legacy delegates predate the tombstone field" (#247) — had outlived its premise. legacy_delegates.toml gains an entry on every WASM bump, so recent legacy generations carry per-room tombstones routinely. That comment was written when "legacy" meant only the pre-tombstone blob generations.

2. One room's merge failure drops all the rest (#591)

Two bare ? on the fallible per-room ChatRoomStateV1::merge returned from the whole function, so every room later in other.map's iteration order was never inserted. other.map is a HashMap, so which rooms are lost is arbitrary and unstable between runs, and the caller only logs.

Worse than the immediate loss: a room absent from the map is exactly where the #527 rank check does not run — merge_from_source compares ranks only when the room is already present — so the next legacy generation's copy is adopted unranked, and reconcile_room_present's diverged-identity branch CAS-writes that older identity over the current one.

Now per-room isolated: failures accumulate, the loop continues, and an aggregate is returned so nothing is swallowed. Same shape as do_save_rooms_to_delegate, which accumulates per-key errors for exactly this reason.

Testing

Three behavioural tests for #590 — a legacy tombstone cannot evict a room the live set holds; it still applies to a room the live set lacks; an authoritative tombstone still removes — plus a pin for #591's loop shape.

Mutation-checked, each turning a test red:

• the A legacy generation's tombstone can permanently delete a room the current delegate holds Present #590 guard disabled (the bug restored);
• the guard made over-broad, dropping all legacy tombstones — which would silently resurrect rooms the user left, the failure mode in the opposite direction;
• the authority hard-coded Authoritative at the hydrate call site;
• the per-room merge aborting the loop again;
• accumulated errors swallowed.

The #527 wiring pin is extended to require the authority be derived from is_legacy_delegate, never hard-coded.

824 river-ui tests, full workspace green.

Scope

UI-only — no delegate/contract WASM, Cargo.toml or Cargo.lock change, so no migration entry is required.

This is the first of the root-cause fixes for the family in #586. It addresses the destruction class. Convergence (the unbounded hoard across delegate generations, which is what actually causes the "Migrating your rooms…" banner) is separate and follows.

Closes #590
Closes #591

[AI-assisted - Claude]

[sanity]

Review record — two blind reviewers, four rounds, design changed twice

The change that opened this PR is not the change in it. Recording why, because the two redesigns were both driven by findings I could not have reached alone.

Round 1 — the guard was rank-blind (both reviewers, independently)

My first fix guarded on "is the room in the map right now". Both reviewers showed that misses the likely interleaving: probes dispatch oldest-generation-first, so the oldest generation usually answers while the map is still empty, its stale tombstone lands unopposed, and the newest generation's Present is then dropped by the tombstone check. It also regressed the mirror case — between two legacy generations nothing asked which source put the room there, so a newer generation's genuine leave was discarded.

Root cause one level down: a legacy generation's absences are older evidence exactly as its presences are, and only presences were ranked. Tombstones are now ranked observations too.

Round 2 — the redesign introduced a new bug

Ranking tombstones created a path that never existed: a room can come back out of removed_rooms. The pre-merge filter deciding which rooms get subscribed and synced was blind to it, so a rescued room arrived inert — in the list, never subscribed, signing key never migrated. Rescued from deletion, then broken, for exactly the users this helps.

Fixed by extracting the survival rule as MergeRanks::presence_survives_tombstone so the merge and the filter share one implementation rather than the filter re-deriving it.

Round 3 — an in-session leave could be undone by a source that agreed with it

leave_room records an unranked tombstone, treated as maximally protected. But or_insert let a legacy generation that agreed the room was gone stamp its own rank over it, after which a higher-ranked Present resurrected the room and the re-save persisted it. Fixed in the merge (an existing unranked tombstone is authoritative) and at the leave site, so the invariant does not depend on call sites remembering.

Notably, one reviewer had cleared this case as safe, then probed it and reported it as real, unprompted.

Round 4 — two tests that could not prove their own claims

• rank_for had no test: collapsing Authoritative => source_rank left all 826 green. Worse, the test written to prove "the authority, not the number, grants removal power" ran with an empty ranks map, so the comparison never happened. Seeded; the mutation now fails two tests.
• The leave/rejoin pairing had no test — the hole a reviewer had actively cleared, which is exactly the kind that gets tidied away later.

The sufficiency argument, in the code

The filter answers ahead of a deferred merge, so an individual answer can be wrong either way. It is still sufficient, and the reason is now a comment rather than folklore: a tombstone rank only ever rises or is removed, and removal happens solely in the restore branch — which only a response carrying Present for that room can take, and whose own filter therefore included it. So the union across the fan-out is complete even when individual answers are wrong. Confirmed by probe, not just by argument.

Mutation coverage

Every fix has a test that fails when the fix is removed: the eviction rank check; a newer Present overriding an older tombstone; tombstones never recorded; the authority hard-coded at the hydrate call site; the legacy call site's true flipped to false; the restore filter removed; the per-room merge aborting; accumulated errors swallowed; rank_for tidied; both halves of the leave/rejoin pairing.

Three of those were green on the first attempt and needed the test rewritten, not the fix — including one where my replacement was also vacuous.

829 river-ui tests, full workspace green.

Deliberately not in this PR

Collapsing is_legacy_delegate + authority into a named LoadSource enum. is_legacy_delegate currently carries four unrelated decisions and the blob case proves they are independent; the fact that two source-scan pins were needed to hold the pair consistent is the tell. It is a refactor, this PR fixes live data loss, and growing it further is how the earlier attempts went wrong. Tracked for the convergence work.

[AI-assisted - Claude]

[sanity]

Round 5 — a third rank-blind spot, and this one loses private-room history

Found by the data-loss reviewer at 1271688b, verified in the code before acting, fixed at d4c0eea8.

The non-adopt branch merged room_state and nothing else

When the identities MATCH — the common case, since generations usually agree who you are — the branch did exactly one thing: merge room_state. Every other field of the incoming RoomData was dropped. So for everything except room_state, the first source to answer won permanently, regardless of rank — and probes dispatch oldest-generation-first, making the systematic winner the OLDEST copy. The merge even recorded the newer source's rank while keeping the older copy's fields, which is the tell.

invitation_secrets is the field that loses data. It is the fallback for exactly the case where the owner-signed encrypted_secrets blob is unavailable — a private-room invitee reading history before the owner delegate back-fills. An older generation answering first with an empty map discarded a newer generation's {version: secret}, the migration re-saved the room without it, and if the owner-signed blob for that version never arrives or has been pruned, those messages are undecryptable for good. repopulate_secrets_from_state cannot recover it — it reads the contract blob, which is precisely what is missing.

Why this is a defect and not a design choice

The codebase already states the opposite rule in both sibling paths. The adopt branch unions the secrets, with a comment that dropping them "would leave a private-room member unable to decrypt messages sealed under that version". The save-side reconcile_room_present unions them too, calling them "identity-INDEPENDENT recovery state". Only the non-adopt load branch — the most-travelled of the three — did not.

Fixed with the same two lines the adopt branch uses, local winning collisions to match both siblings. self_nickname gets the adopt branch's existing "keep local, take theirs if we have none" rule for the same reason — losing it drops a member to a generated handle.

notification_modes, room_order and last_read_message_id are deliberately left first-writer-wins, now with a comment saying so by design, not by omission — they are local user preferences where "this device wins" is the intended behaviour.

Pre-existing on main; not a regression from this PR.

Mutation coverage

Removing the union fails a_matching_identity_merge_unions_invitation_secrets. Inverting the collision rule to incoming-wins fails a_matching_identity_merge_keeps_the_local_secret_on_collision.

831 river-ui tests, full workspace green.

On the LoadSource refactor, deferred to the convergence work

Recording the reviewer's caveat here so it is not lost in a thread, because it would bite whoever does it: is_legacy_delegate is not a synonym for authority. It independently gates set_load_state_if_current(Migrating), the decide_current_room_restore cursor-restore skip, and the legacy re-save. The blob caller is deliberately (false, OlderSnapshot) precisely so it gets snapshot semantics without announcing Migrating, restoring a stale cursor, or firing a re-save. So a LoadSource enum has to carry all four behaviours as methods — collapsing it to authority alone would silently give the blob path a legacy re-save, which is worse than what it replaces.

The argument for doing it eventually: there are exactly three valid (is_legacy_delegate, authority) combinations in use, and the fourth — (true, Authoritative) — is bug #590 itself. An enum makes the bug unrepresentable, which beats a source-scan pin asserting the derivation's exact text.

[AI-assisted - Claude]

[sanity]

Rounds 6–8 — three more gaps, all in the tests rather than the fix

The production change has not moved since a0c0cd61. Verified mechanically, not by reading the diff: splitting each of the three touched files at mod tests { and diffing the production half shows it byte-identical across a0c0cd61, 6522528d, 8d43b3e2 and f45ea594. Everything below is coverage.

Round 6 — self_nickname had no test (data-loss lens). A carry-over I had added on my own initiative. Its inversion is destructive rather than merely useless: copying the adopt branch's condition verbatim from nine lines up produces a line that fires when the incoming copy has no nickname and assigns that None over the local one. Both mutations now red.

Round 7 — the drain pin claimed more than it tested (code-first lens). drain < mark is textual order only, so calling mark_room_rejoined inside the ranks closure satisfied it — and that re-locks a non-reentrant Mutex, which on single-threaded WASM hangs the tab rather than failing a test. The pin now requires the collect-out-of-closure shape with the mark after the closure closes. Added a_ranked_resurrection_survives_into_the_delegate_write: merges a G3 tombstone and a G20 Present through the shared registry, drains, marks, and asserts reconcile_room_present overwrites a stored Tombstone with Present.

Round 8 — the rewrite dropped a property while improving the pin (data-loss lens). drain < mark was replaced rather than added to, and nothing anchored the drain to merge_from_source, which populates the set. Hoisting the drain to the top of the function — beside the tombstoned computation, which also takes the ranks lock, so consolidating them is the natural refactor — preserves both new assertions, compiles, and leaves the suite green while draining an empty set: nothing marked, back to AbortAdoptLeave, #590 silently restored. Anchor added; the hoist now fails it.

Also recorded at the drain: this widens REJOINED_THIS_SESSION from "the user deliberately did something" to also mean "the ranks concluded this room should come back", so a wrong resurrection is persisted rather than session-local. The known wrong case is a version downgrade (rank proxies wall-clock). Accepted deliberately — a wrongly-resurrected room is one the user leaves again, whereas #590 destroys self_sk, which is unrecoverable — but the blast radius grew and the code now says so.

What these rounds were actually about

Three of the four gaps were assertions pinning a shape where the property that mattered was a dependency between two pieces of code: a literal string, a call site's presence, an ordering between two tokens. Each survives a refactor that preserves the shape. The tests that held up drive the behaviour and assert the outcome.

Round 8 is a distinct failure mode and the more interesting one: the property was lost because the revision was good. Fixing the re-entrancy hole meant rewriting the assertion, and the rewrite silently dropped something nobody was tracking as separate. The diff looks like strengthening and the suite stays green. The guard is to treat replacing an assertion as needing the same care as deleting one — enumerate what the old one proved, then check each item is still proved by something.

Every mutation cited above was run, not argued. Two crossed on the wire: the round-8 anchor was pushed before the finding arrived, and the hoist took three attempts before it compiled — reporting "caught" from a mutation that never built would have been the same class of false result this PR kept producing.

Lenses: code-first and data-loss, both blind, eight rounds.

[AI-assisted - Claude]