Skip to content

fix(migration): durable interrupted-migration marker (#760) + perf(delegate): byte fields as CBOR byte strings (#757) - #758

Draft
sanity wants to merge 8 commits into
mainfrom
perf/delegate-bytes-757
Draft

sanity wants to merge 8 commits into
mainfrom
perf/delegate-bytes-757

Conversation

@sanity

@sanity sanity commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Part of #757: fix 4 ("PR B"). Also fixes #760, a production data-stranding bug in delegate migration that the rehearsal for this re-key exposed. It ships here because the fix has to be live no later than the first migration into the new generation. PR A (fixes 1-3, UI only) is separate.

1. Delegate message byte fields as CBOR byte strings

ChatDelegateRequestMsg / ChatDelegateResponseMsg byte fields were serialized by ciborium as CBOR integer arrays. Every byte >= 0x18 costs 2 bytes, so delegate round-trips were ~1.9x their payload: a 1.18 MB stored room arrived as a 2.32 MB GetResponse on every page load.

What changes. The byte fields are now encoded through a small hand-rolled cbor_bytes helper in common/src/chat_delegate.rs:

  • requests: StoreRequest.value, CasStoreRequest.value, and all eight Sign* payloads;
  • responses: GetResponse.value, GetVersionedResponse.value, CasStoreResult::Conflict.current_value. These three Option fields also get #[serde(default)], because with drops serde's "absent Option is None".

What stays an integer array.

  • ChatDelegateKey. It is the one field sent to frozen predecessor delegates, and it is only a few dozen bytes.
  • SignResponse.signature (64 bytes).
  • No room-contract state type is touched.

Why not serde_bytes. Adding that dependency to river-core re-keyed the room contract (measured a3e63c8c… -> 48d91e7b…). A river-core version bump does the same (c29522a5…). With the hand-rolled helper and no version bump, room_contract.wasm stays byte-identical, so this is a delegate-only re-key and riverctl does not ship.

Compatibility, both directions, under ciborium 0.2.2. Every delegate generation in git embeds 0.2.2.

  • The new UI decodes legacy integer-array replies, because deserialize_byte_buf accepts an array.
  • A legacy delegate decodes byte strings, because its plain Vec<u8> goes through deserialize_seq, which accepts bytes.
  • Requests sent to predecessors (GetRequest, ListRequest) are byte-identical to before.
  • Data at rest is opaque bytes inside the versioning.rs envelope, so nothing stored changes.
  • riverctl does not use these types.

2. Interrupted migrations no longer strand rooms (#760)

This activates recovery logic that has never run in production. The interrupted-migration recovery (#345 follow-up: decide_per_room_load_action → re-run the legacy fill) keyed off a flag in localStorage. The gateway serves River in an iframe whose sandbox omits allow-same-origin, so localStorage throws SecurityError (confirmed in the rehearsal) and the flag was never written.

Reproduced with b5e08b0, before this fix: I killed the tab right after the first per-room CAS write of the migration. Every later load showed 1 of 4 rooms; the other 3 stayed stranded in the predecessor.

The fix: a marker in the successor delegate's KV store, __river_legacy_migration_in_progress__.

  • Write. It is stored once per session, acknowledged and retried once, before the first per-room write of a legacy re-save or current-blob explosion.
    • An unacknowledged store does not abort the re-save. The merged rooms are already in ROOMS, and other save paths (room sync, the walk's flush) write them anyway, so aborting would only make a complete set less likely.
  • Delete. Only the existing legacy fan-out quiescence seal deletes it, and only when no re-save is in flight and none failed.
    • Several generations re-save concurrently and share the marker, so it is never deleted when one of them finishes.
    • A recovery that finds nothing to add converges at the same seal.
    • The delete re-checks under a lock that no late re-save started.
  • Read. It is read from the current delegate's ListResponse, so there is no extra round trip.
    • A freenet-migrate walk wip marker without its done marker also counts as interrupted, because the walk's flush writes per-room keys too and is the only importer when the sweep's send fails.
  • Serialisation. Marker Store and Delete are serialised: they share one correlation slot in the single-waiter registry.
  • Session state. Session state is the pure, unit-tested MigrationMarkerState (present / in_flight / failed / written). The Import Identity should allow overwriting an existing room identity (warn, don't refuse) #414 identity-import gate reads its in_progress().
  • Forward copy. is_migration_marker_key excludes the marker, so it never travels to a later generation.
  • Cost. One extra Store per migrating session and one Delete at the seal. Nothing on non-migrating loads.

A recovery re-run never clobbers newer data. That covers a lost delete, a crash before the seal, and app use in between. The legacy copy is merged with MergeAuthority::OlderSnapshot (room_data.rs merge_from_source, #590), which is additive only:

  • It may add a room the live set lacks.
  • An identity conflict keeps the live identity, and messages are CRDT-merged.
  • A tombstone from the current delegate outranks a legacy Present, so a room left since stays left.
  • The save is per-room CAS read-merge-write.

a_recovery_rerun_only_adds_rooms_the_live_set_lacks pins this, and I watched it fail with resolve_identity_conflict forced to AdoptIncoming.

Not fixed here: #761. The legacy "done" seal is still dead localStorage, so an account with no rooms re-probes all 29 legacy delegates on every load. Making that seal effective means activating a permanent "never probe again" decision, which needs its own review.

Tests

common/src/chat_delegate.rs, against a legacy mirror of the pre-change types. The mirror has its own key type, so an encoding change to ChatDelegateKey would show up.

Test What it pins
every_byte_field_encodes_as_a_cbor_byte_string all 13 fields, checked on the decoded CBOR value; signature stays an array
byte_fields_decode_legacy_integer_array_responses / byte_fields_legacy_decoder_accepts_byte_strings both directions, for Some, empty and None
predecessor_bound_requests_are_wire_identical_to_legacy requests sent to frozen WASM are unchanged
get_response_golden_vectors, request_and_null_golden_vectors literal wire bytes
absent_optional_byte_field_decodes_as_none all three Option fields
legacy_array_… / byte_string_with_lying_length_header_errors_not_panics lying-length headers fail cleanly

UI tests:

  • MigrationMarkerState tests: overlapping re-saves keep the marker; a failed re-save blocks the seal; a recovery with nothing to add converges; store once and a late re-save cancels the delete; a listing never hides a running re-save; the import gate follows the state.
  • marker_store_outcome table (all 4 branches).
  • a_walk_wip_marker_without_done_counts_as_interrupted.
  • plan_load_from_keys with the marker.
  • a_recovery_rerun_only_adds_rooms_the_live_set_lacks.
  • Source pins: mark before save, no abort, end after save on both re-save paths; only the quiescence seal deletes the marker (needle built from parts so the pin can't match itself); the listing seeds the state before the plan.

Every guard was watched failing under a mutation.

Artifacts (rebuilt from source at the final head)

scripts/sync-wasm.sh's co-build gives chat_delegate.wasm ea5005e9… and room_contract.wasm a3e63c8c…, both identical to the committed files. Review lenses independently reproduced these hashes from git archive copies at b5e08b0 and 485d7a6. Later commits touch only UI code, tests and docs, and the rebuild at the final head is unchanged.

  • V32 entry. code_hash c2e60638… is the delegate inside the webapp currently live on the network: I fetched raAqMh… via 7509 and hashed contracts/chat_delegate.wasm. Its delegate_key is c15cbbb7… = BLAKE3(code_hash).
  • Pointer. The river.chat-delegate pointer record is re-signed at v3 for ea5005e9….
  • Crate versions. freenet-migrate and freenet-migrate-build versions are unchanged, and there is no Cargo.toml or Cargo.lock change.

Measurement (isolated network-mode node, real WASM)

  • Node: freenet network --is-gateway --skip-load-from-network (freenet 0.2.142), with no peers, its own dirs and telemetry off.
  • App frame: the app runs in the gateway's sandboxed iframe, where localStorage is unavailable, as in production.
  • Signing: every UI build was signed with the same throwaway container key (contract FDfXQ61p…). Delegate KV is scoped by the webapp origin, so this is what lets each build see the previous one's data, exactly as successive production publishes do. Delegate keys are the real production ones.
  • Account: 4 rooms (3 created in the UI with signing keys, plus 1 joined room with 70 messages of ~9 KB each, a ~1.2 MB delegate slot), one outbound DM and one inbound DM.

Steady-state WS bytes per page load. I ran two loads of each and they were identical:

up down
main UI (V32 delegate) 5,181,003 6,312,381
this PR (after migration) 5,183,850 3,794,589

Download falls by 2.52 MB per load (-40%), which matches the large room's ~1.2 MB slot being read twice per load (fix 2 in PR A removes the second read). Upload is unchanged; it is the room re-PUTs that PR A removes. The first load after the upgrade moves 6.49 MB up and 6.35 MB down, because legacy replies are still integer arrays.

Rehearsal (currently published delegate -> this PR)

The "old" UI was built from main 7b3ba90 and embeds the live delegate (c2e60638…). Its node dir E1ohZwYq… is base58 of V32's delegate_key. Before each upgrade, the successor dir 3tzJ9m93… was moved aside so it started empty. Each step names the build it ran on; 7cd6991 is the final code.

  1. First load after the upgrade (empty successor).
    • The sweep migrated per-room slots, signing keys and outbound_dms. The legacy 226-byte integer-array reply decoded: "Hydrated 1 outbound-DM entries … from legacy delegate".
    • The freenet-migrate walk reported each generation that holds data as Imported and sealed it. It also visited the absent generations (Unresponsive, not sealed) and did not stop on them.
  2. Populated successor (every later load). The rooms load from the new delegate. The steady-state measurement above comes from these loads.
  3. Rooms split across generations. I built the V31-era UI (8c0ca8c3^, delegate a44c6401), published it under the same container key, and created a room ("Delta") that only V31 holds. The upgrade recovered 5 rooms: Delta from V31 plus 4 from V32. Walk gens 27 and 28 were both Imported. So rooms stranded by an earlier interrupted migration come back on this re-key, because the first load visits every registered generation.
  4. Interrupted at the first per-room write (7cd6991).
    • The next load listed __river_legacy_migration_in_progress__, logged "Prior migration was interrupted — re-running", recovered all 5 rooms, and at quiescence logged "deleted the migration-in-progress marker".
    • The load after that has no marker and 5 rooms.
    • With b5e08b0 (before the fix), the same kill left 1 of 4 rooms permanently.
  5. Interrupted after both generations' re-saves finished but before quiescence (7cd6991). The kill came at 12.4 s, during the walk's import.
    • The next load still saw the marker, because it is no longer deleted per re-save. It recovered, then deleted the marker.
    • Both generations' re-saves coalesce into one save_rooms_to_delegate pass here (both finish at 6.37 s). So the strict "kill inside the second generation's own save" window did not occur on this node, and the pre-fix build did not lose rooms under the same kill. The overlap ordering is pinned by unit tests instead.
  6. Signing keys are in the NEW delegate (7cd6991).
    • On a fresh load, GetPublicKeyResponse … present came back for all 5 rooms, and EnsureRoomSubscriptionResponse ok: true for the 4 owned rooms.
    • Creating an invite issued a real SignMember (byte-string payload) to the new delegate WASM and got SignResponse ok: true, with no "Delegate signing failed" fallback.
  7. DMs (7cd6991). The outbound DM plaintext (stored only in outbound_dms) renders after migration, and the inbound DM decrypts.
  8. The V32 store is never written by migration. It did gain one 42-byte secret from its own room subscription when a room updated, which is pre-existing behaviour for any re-key.

Not exercised end to end: a walk-only import, where the sweep's send to a predecessor fails. I could not force that on a healthy node, so a_walk_wip_marker_without_done_counts_as_interrupted covers it at unit level.

Publish procedure (coordinator, from main after merge, in this order)

  1. UI only: cargo make publish-river, not publish-all. riverctl embeds only room_contract.wasm, which is byte-identical, so there is no riverctl release and no check-cli-wasm bump. Set up the build environment from the river-publish skill's Step 6: export PATH=$HOME/.cargo/bin:$PATH RUSTC_WRAPPER= CARGO_BUILD_RUSTC_WRAPPER=, check that dx --version matches dioxus 0.7.9, and grep the publish log for incompatible.
  2. Counter PR: land the bumped published-contract/contract-version.txt through its own PR (.claude/rules/river-publish.md Step 6) and merge it before step 3. The pointer script refuses a dirty tree and requires being on main with CI green.
  3. Pointer: from main, per river-publish.md Step 7:
    POINTER_NODE_PORT=<a network node of yours, NOT 7509> \
    POINTER_WASM=~/code/freenet/freenet-migrate/contracts/pointer-contract/pointer-v1.wasm \
      cargo make publish-pointer-records
    
    Its pre-flight checks that the record's hash (ea5005e9…) matches the delegate inside the live webapp, so it only passes after step 1.
  4. No river-core version bump. A bump re-keys the room contract. river-core 0.1.21 on crates.io keeps the old encoding until the next bump, which is wire-compatible both ways for ciborium users. FREENET.md now tells other-library integrators to accept both forms.
  5. Verify. On the live deploy, rooms are still listed, a second load shows no "migration-in-progress marker" log line, and the console reports "Attempting to migrate data from N legacy delegate(s)" on a first load, where N is the number of [[entry]] rows in legacy_delegates.toml (29 at this PR).

Also in this PR

  • ui/.gitignore. public/ excluded the directory, so git never evaluated the !public/contracts/*.wasm negations, and the documented git add … ui/public/contracts/ … exited non-zero. It is now public/*, !public/contracts/, public/contracts/*, plus the two WASM negations. Stray files in ui/public/contracts/ (e.g. an upgrade_assistant.wasm) stay ignored, and the two committed WASMs are addable.
  • Rules. .claude/rules/delegate-migration.md and river-publish.md say riverctl ships only for room-contract changes, and now record that a river-core dependency or version change re-keys the room contract. The add-*-migration.sh hints include the pointer re-sign step.

[AI-assisted - Claude]

https://claude.ai/code/session_01MzSwWofxEmdfdRJvbHxeuE

sanity added 2 commits October 9, 2026 10:38
…ngs (#757)

ChatDelegateRequestMsg / ChatDelegateResponseMsg byte fields (Store/CAS
values, Get/GetVersioned values, CAS conflict values, Sign* payloads)
serialized via ciborium as CBOR integer arrays, roughly doubling every
delegate round-trip: a 1.18 MB stored room arrived as a 2.32 MB
GetResponse on every page load.

They now encode as CBOR byte strings through a small hand-rolled
`cbor_bytes` helper that still decodes the legacy integer-array form,
so the new UI reads legacy delegates during migration, and ciborium's
plain Vec<u8> decoder already accepts byte strings, so legacy
delegates decode the new form too. ChatDelegateKey stays an integer
array: it is the one field sent to frozen predecessor delegates.

Hand-rolled rather than the serde_bytes crate because adding that
dependency to river-core re-keyed the room contract (measured); with
this helper room_contract.wasm is byte-identical (a3e63c8c...).

Delegate re-key: V32 registered in legacy_delegates.toml before the
rebuild, WASM synced, chat-delegate pointer record re-signed (v3),
UI pins updated.

Claude-Session: https://claude.ai/code/session_01MzSwWofxEmdfdRJvbHxeuE
…docs

- Test every changed field encodes as a CBOR byte string (not just
  GetResponse.value), add golden wire vectors, a lying byte-string
  header case, and an absent-field case.
- `#[serde(default)]` on the three `cbor_bytes::option` fields: `with`
  dropped serde's "missing Option is None". Response-only, so the
  delegate WASM is unchanged (still ea5005e9...).
- Correct the doc comments (SignResponse.signature and keys stay
  integer arrays; the 4096 cap is payload_bytes', not serde's).
- Rules: riverctl ships only for room-contract WASM changes; record the
  measured fact that a river-core dependency or version bump re-keys the
  room contract.
- ui/.gitignore: `public/` excluded the directory, so the `!public/contracts/`
  negations never applied and the documented `git add ui/public/contracts/`
  failed outright. `public/*` lets them apply.
- FREENET.md: note the byte-string change for non-ciborium integrators.

Claude-Session: https://claude.ai/code/session_01MzSwWofxEmdfdRJvbHxeuE

@sanity sanity left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive PR Review: #758 (round 1)

Summary

  • Type: perf (wire format + chat-delegate re-key)
  • CI at reviewed head 69cd88f: all green (build, ui-playwright-tests, check-delegate-migration, check-room-contract-migration, check-pointer-freshness, check-wasm-sync)
  • Linked issue: #757 (fix 4)
  • Review tier: Full (wire format/serialization, delegate WASM re-key)
  • Reviewers run: code-first, testing, skeptical, big-picture, plus two extra Claude lenses: wire-compat/serialization, migration data-loss.
  • External model: Codex (codex review --base origin/main) failed with "You've hit your usage limit". It will be retried once on the re-review. If it is still down, the extra lenses stand in for it and this comment records that.

What the reviewers verified independently

  • The V32 entry is right: code_hash c2e60638… = BLAKE3 of main's committed chat_delegate.wasm, and delegate_key c15cbbb7… = BLAKE3(code_hash).
  • Every historical delegate WASM in git hashes to a registry entry.
  • Rebuilding from the PR tree gives room_contract.wasm a3e63c8c… (unchanged) and chat_delegate.wasm ea5005e9… (matching the commit, the UI pin and pointer record v3).
  • ciborium 0.2.2 is the only version any generation could have used; all 29 delegate blobs in git history embed it. Its deserialize_byte_buf accepts arrays and its deserialize_seq accepts byte strings.
  • The compat lens linked three river-core versions (PR head, main, Jan 2026) side by side and ran 148 checks: old→new and new→old for every changed field, Some/empty/None, tags, indefinite lengths, and a 1.18 MB value (2,249,412 bytes legacy vs 1,180,052 new).
  • Predecessors only ever receive key-only GetRequest/ListRequest, which stay byte-identical. Markers and CAS writes go to the successor only.
  • Data at rest is opaque bytes in the versioning.rs envelope. riverctl does not use these types.

Findings and dispositions

Must Fix

None.

Should Fix (all fixed in b5e08b0)

  1. Only GetResponse.value was pinned as a byte string. Dropping serde(with) from any other field would pass every test. Fixed: every_byte_field_encodes_as_a_cbor_byte_string checks all 13 fields on the decoded ciborium::Value (Bytes/Null). Watched it fail with SignBan.ban_bytes un-annotated. The PR body's "each watched failing (attribute removed)" was overstated for round 1; that is now true.
  2. Doc said "every Vec<u8> field", but SignResponse.signature and keys stay integer arrays. Fixed: the doc now names both exceptions and tells future fields to use the helper and the test.
  3. Rules contradicted a delegate-only re-key. delegate-migration.md and river-publish.md said riverctl must ship on any WASM change. Fixed: now "room-contract WASM", since riverctl embeds only room_contract.wasm.
  4. The dependency/version re-key finding lived only in a code comment. Fixed: new section in delegate-migration.md with measurements. Adding serde_bytes moved the room contract to 48d91e7b…; a river-core 0.1.21→0.1.22 bump moved it to c29522a5… (measured for this review, so the UI comment's claim is now a measurement).
  5. PR body lacked measurement, rehearsal and post-merge steps. Fixed in the PR body.

Consider

  • Absent value field. serde(with) turned a missing field into an error instead of None. Fixed: #[serde(default)] on the three Option fields, pinned by absent_optional_byte_field_decodes_as_none. These are response-only, so the delegate WASM is unchanged.
  • Wrong cap comment. The 4096 cap is payload_bytes', not serde's. Fixed.
  • Private intra-doc link. Fixed: the doc now uses plain code formatting.
  • "ListRequest" in the key note. Fixed: reworded.
  • Stale "27 entries … V30" comment. Fixed: now 29 entries, V1..V32.
  • Golden vectors and lying byte-string header. Added.
  • Missing pointer-records.toml in river-publish.md's git add. Fixed, and add-migration.sh's hint now includes sign-pointer-records.
  • Non-ciborium integrators. Added a note to FREENET.md.
  • river-core 0.1.21 on crates.io now differs from source. Noted in the PR body. This is deliberate: a bump re-keys the room contract (measured), and the change is wire-compatible both ways for ciborium users.
  • debug_assert that predecessors get only key-only requests. Not done. The invariant is already pinned on the wire by predecessor_bound_requests_are_wire_identical_to_legacy, and even if it broke, old delegates decode byte strings (ciborium deserialize_seq), so an assert would guard a failure that cannot lose data.
  • Duplicate helper vs payload_bytes. Kept separate. Sharing it would mean editing room_state/content.rs, and a line shift there re-keys the room contract. The new helper's doc points at payload_bytes.
  • Pre-existing, not this PR: a tab still running the old UI after another tab migrates keeps writing to the old delegate, and the old delegate's own room subscription keeps writing its member-set secret after the re-key. The rehearsal saw that second one: one 42-byte secret appeared in the V32 store when a room updated.

Found while fixing

ui/.gitignore had public/, which excludes the directory, so the !public/contracts/*.wasm negations never applied. The documented git add ui/public/contracts/ … therefore failed and staged nothing. Fixed with public/*.

Verdict

State: Needs Changes — Re-review Required After Fix. More than three Should Fix items were addressed and the diff grew by more than 30 lines, so a full re-review runs on b5e08b0.
HEAD SHA reviewed: 69cd88f

[AI-assisted - Claude]

sanity added 2 commits October 9, 2026 11:47
…t localStorage

The interrupted-migration recovery (#345 follow-up) keyed off
a "migration in progress" flag in localStorage. The gateway serves the app
in an iframe whose sandbox omits allow-same-origin, so localStorage is
unavailable and the flag was never written in production. Reproduced on
the #757 rehearsal node (sandboxed iframe, real WASM): closing the tab
right after the first per-room CAS write left 1 of 4 rooms on every later
load, the other 3 stranded in the predecessor delegate.

The marker now lives in the current (successor) delegate's KV store under
`__river_legacy_migration_in_progress__`:
- written and ACKED before the first per-room write of a legacy re-save
  or current-blob explosion; if the delegate does not ack it, the save is
  skipped (nothing written, so the next load migrates from scratch);
- deleted after a full successful save;
- read from the current delegate's ListResponse at load, which feeds the
  existing decide_per_room_load_action -> recovery path unchanged;
- excluded from the freenet-migrate walk's import via
  is_migration_marker_key, so it never travels to a later generation.

Recovery itself is unchanged: the existing registry-driven sweep plus the
freenet-migrate walk. No migration logic is added.

Also from the #758 round-2 review: absent-field test for
CasStoreResult::Conflict, request and null golden vectors, a pin that
SignResponse.signature stays an integer array; tighter ui/.gitignore so
stray files in ui/public/contracts stay ignored; pointer re-sign step in
add-room-contract-migration.sh's hint; hedged rule text.

Claude-Session: https://claude.ai/code/session_01MzSwWofxEmdfdRJvbHxeuE
With the durable in-progress marker, a crash mid-save or a lost marker
delete re-runs the legacy fill on top of rooms the user kept using. Pin
that the OlderSnapshot merge restores a stranded room, keeps a room
joined since, keeps a room left since left, and keeps the live identity.

Claude-Session: https://claude.ai/code/session_01MzSwWofxEmdfdRJvbHxeuE
@sanity sanity changed the title perf(delegate): encode delegate message byte fields as CBOR byte strings (#757) perf(delegate): byte fields as CBOR byte strings (#757); durable interrupted-migration marker Oct 9, 2026

@sanity sanity left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive PR Review: #758 (round 2, at b5e08b0)

  • Reviewers: code-first, testing, skeptical, big-picture, wire-compat and migration-data-loss. These were resumed from round 1, so each re-reviewed the full diff.
  • Codex: retried once and failed again with "You've hit your usage limit". The six Claude lenses stand in for it.
  • CI at b5e08b0: green.

Verified independently this round

  • Three lenses rebuilt the WASMs from git archive copies (69cd88f and b5e08b0) with the sync-wasm co-build. All got chat_delegate ea5005e9… and room_contract a3e63c8c…, identical to the committed files.
  • The compat lens re-ran its 151-check, three-generation probe. The absent-field behaviour now matches the old types, and no other wire bytes moved.

Findings and dispositions

Must Fix: none.

Should Fix:

  1. The migration lens's finding 4 was REAL, and is fixed in b730d0e. The interrupted-migration flag lived in localStorage, which is unavailable in the gateway's sandboxed iframe, so it was never written. On the rehearsal node, a tab killed after the first per-room write left 1 of 4 rooms permanently.
    • The marker now lives in the successor delegate. It is acked before the first write, deleted after a full save, and read from the listing.
    • The rehearsal now recovers all 4 rooms.
    • a_recovery_rerun_only_adds_rooms_the_live_set_lacks pins that a re-run (for example after a lost marker delete) only adds rooms. See the PR body, section 2.
  2. "Signing keys survived" was not proven by a posted message (migration lens). Fixed with real evidence. Against the NEW delegate:
    • GetPublicKeyResponse present for all 4 rooms;
    • EnsureRoomSubscriptionResponse ok for the 3 owned rooms;
    • a real SignMember returned SignResponse ok: true with no fallback.
  3. The rehearsal needed its localStorage state stated (migration lens). Done: the probe in the app frame reports unavailable: SecurityError, as in production.
  4. Conflict.current_value default was untested (testing lens). Added to absent_optional_byte_field_decodes_as_none; I watched it fail with default removed.
  5. .gitignore public/* un-ignored stray WASMs (skeptical lens). Fixed with public/contracts/* plus the two negations, and verified with git check-ignore --no-index.
  6. The PR body's publish steps were incomplete (big-picture and code-first lenses). Fixed:
    • the pointer environment;
    • the merge order (counter PR before the pointer);
    • the .gitignore explanation;
    • the rebuild hashes;
    • the legacy-count check, derived from the TOML.

Consider:

  • Request golden vector, null golden vector, SignResponse.signature pin. Added.
  • Mislabelled "populated successor" case. Relabelled.
  • Same throwaway key across builds. Stated: the scope is origin-based.
  • Hedging. The serde_bytes mechanism and the "byte-identical" generalisation in the rules are now hedged. "40-line" is now "~55-line". The river-core-bump exception is documented.
  • Stale hint. add-room-contract-migration.sh now includes the pointer step.
  • Users stranded by earlier re-keys (from the lead's question). Verified: a room held only by V31 plus four by V32 all came back on this upgrade, with walk gens 27 and 28 both Imported.
  • debug_assert for predecessor-bound request types. Still declined; the round-1 reason stands.

Verdict

State: Needs Changes — Re-review Required After Fix. b730d0e adds migration-critical code, so round 3 runs on the new head.
HEAD SHA reviewed: b5e08b0

[AI-assisted - Claude]

sanity added 2 commits October 9, 2026 12:00
…save

Round-3 review of #758:
- Several legacy generations re-save concurrently and share one marker.
  Deleting it when the first finished left a slower generation's rooms
  unprotected; a crash then would strand them again. The marker is now
  deleted only by the existing quiescence seal (schedule_legacy_seal),
  once no re-save is in flight and none failed.
- A recovery that found nothing to add never reached a delete, and the
  seal itself was gated on is_legacy_migration_in_progress(), which a
  merely-present marker kept true: the marker stuck forever. The seal now
  gates on MigrationMarkerState::may_seal (in-flight/failed only), so a
  recovery converges.
- Session state is a pure MigrationMarkerState (present / in_flight /
  failed) behind one mutex, unit-tested on local instances. This replaces
  a test that toggled the process-wide flag under parallel tests (flaky).
- A marker store that is not acknowledged no longer leaves the rail on
  "Migrating..." when rooms are already in memory.
- marker_store_outcome is a pure, table-tested reply interpretation;
  plan_load_from_keys is tested with the marker; source pins cover
  mark-before-save, abort-on-failure and end-after-save on both re-save
  paths, and that only the seal deletes the marker.

Claude-Session: https://claude.ai/code/session_01MzSwWofxEmdfdRJvbHxeuE
Round-3 review of #758 (#760):
- Marker Store and Delete share one bare-key correlation slot in the
  single-waiter registry; they are now serialised behind MARKER_OPS, and
  the store happens once per session (later re-saves reuse it).
- The delete re-checks under that lock that no late re-save started (and
  stored the marker) after the seal decided to delete.
- An unacknowledged marker store no longer aborts the re-save. The merged
  rooms are already in ROOMS and other save paths (room sync, the walk's
  flush) write them regardless, so aborting only made a complete per-room
  set less likely. The store is retried once.
- A freenet-migrate walk wip marker without its done marker now counts as
  an interrupted migration: the walk's flush writes per-room keys too, and
  it is the only importer when the sweep's send to a predecessor failed.
- Tests: store-once and late-resave delete cancel, import gate follows the
  marker state, walk wip-without-done detection. Rule doc updated.

Claude-Session: https://claude.ai/code/session_01MzSwWofxEmdfdRJvbHxeuE
@sanity sanity changed the title perf(delegate): byte fields as CBOR byte strings (#757); durable interrupted-migration marker fix(migration): durable interrupted-migration marker (#760) + perf(delegate): byte fields as CBOR byte strings (#757) Oct 9, 2026

@sanity sanity left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive PR Review: #758 (round 3, at 485d7a6)

The six lenses from round 2 re-ran on the full diff: code-first, testing, skeptical, big-picture, wire-compat and migration-data-loss. Codex was still unavailable ("usage limit"), so the Claude lenses stood in for it.

Findings and dispositions (fixed in d85e386 and 7cd6991)

Must Fix (testing lens). A test toggled the process-wide marker flag while another test asserted it was false, which is a parallel-test race.
Fixed: session state is now a pure MigrationMarkerState, tested only on local instances.

Should Fix and elevated items (all lenses):

  1. A marker shared by concurrent generations was deleted on the first save's completion (migration, testing, skeptical, code-first). Fixed: only the existing quiescence seal deletes it, and only when nothing is in flight and nothing failed. The delete re-checks under a lock.
  2. A stale marker never converged. A recovery with nothing to add never deleted it, and the seal was gated on is_legacy_migration_in_progress(), which a merely-present marker kept true. Fixed: the seal now gates on in-flight/failed only.
  3. A reconnect listing could clear the flag mid-migration (skeptical, migration, code-first). Fixed: a listing can no longer hide a running re-save.
  4. Abort on a failed marker store. The premise was false, because other save paths write the merged rooms anyway, and the abort left "Migrating…" on screen and the import gate shut (skeptical, migration, big-picture, code-first). Fixed: the re-save always runs; the store is retried once.
  5. Store and Delete shared one correlation slot (skeptical). Fixed: they are serialised behind MARKER_OPS, and the store happens once per session.
  6. The walk's flush wrote rooms with no marker (migration). Fixed: a walk wip marker without its done marker counts as interrupted. This uses the crate's own durable markers.
  7. Marker paths were covered only by source scrapes (testing). Fixed:
    • a table test for marker_store_outcome;
    • plan_load_from_keys tests with the marker;
    • state-machine tests;
    • pins on both re-save paths;
    • an "only the seal deletes" pin whose needle is built from parts so it can't match itself.
  8. Docs.
    • Fixed: the stale localStorage comments, the rule doc, the publish env citation, and the PR title and body, which now say this activates recovery that was dead in production and give the extra round-trip cost.
    • Filed: #760 for the stranding bug (fixed here) and #761 for the done seal, which is also dead localStorage and is out of scope; the reason is in the issue.

Consider:

  • Added: the "never rename" note on the marker constant, a kept-room state check in the recovery merge test, and the import-gate test.
  • Done: the rehearsal re-ran on 7cd6991 with interrupt-and-reload, an interruption after both generations' saves, keys and signing, and DMs. The literal "kill inside the second generation's save" window does not occur, because the two generations' saves coalesce into one pass. That is stated in the PR body, and unit tests cover the overlap ordering.

Verdict

State: Needs Changes — Re-review Required After Fix. Round 4 runs on 7cd6991.
HEAD SHA reviewed: 485d7a6

[AI-assisted - Claude]

Only conflict: the chat_delegate import list in response_handler.rs
(main added claim_list_load / request_outbound_dms_once /
seed_saved_slots_from_load and still imported the localStorage-era
clear_legacy_migration_in_progress, which this branch replaced).

Merged rather than rebased so the PR branch never needs a force-push.

Claude-Session: https://claude.ai/code/session_01MzSwWofxEmdfdRJvbHxeuE
@sanity

sanity commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Converted to draft. This is blocked on migration completeness in freenet-migrate: freenet/freenet-migrate#37.

Round 4 (6 fresh Claude lenses; Codex was out of quota, so they stood in) found no problem with part 1, the CBOR byte-string wire change: cross-decoding was verified in both directions, and the delegate and room-contract hashes rebuilt from source. Part 2, the durable interrupted-migration marker for #760, still decides "migration complete" in-session. That decision fails in several durable-state cases, detailed in freenet-migrate#37:

  • an undecodable predecessor item leaves the walk's wip marker forever;
  • a timed-out slot fetch lets the seal delete the marker;
  • an absent generation's instant "not found" can trigger the seal before V32 answers;
  • reconnect listings can raise or lower the session state.

The fix is a verifiable expected-set completeness mechanism, and it belongs in freenet-migrate rather than in River. Shipping this re-key before that lands would expose every user to #760, so the branch is kept as-is (head 51366d9, which includes PR A #759 via a merge).

When the crate support lands, this PR needs to:

  • adopt it, replacing the River-side marker in part 2;
  • re-run the rehearsal on that build;
  • go through a fresh Full-tier review.

[AI-assisted - Claude]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interrupted delegate migration strands rooms in the previous generation (in-progress flag lived in localStorage)

1 participant