Skip to content

fix(ui): repopulate room_data.secrets on every state ingestion path (#251)#264

Merged
sanity merged 3 commits into
mainfrom
fix/issue-251-repopulate-secrets
May 17, 2026
Merged

fix(ui): repopulate room_data.secrets on every state ingestion path (#251)#264
sanity merged 3 commits into
mainfrom
fix/issue-251-repopulate-secrets

Conversation

@sanity
Copy link
Copy Markdown
Contributor

@sanity sanity commented May 17, 2026

Problem

Closes #251.

When a private-room invitee subscribes, the initial GET response often arrives before the owner's chat delegate runs the PR #245 back-fill of encrypted_secrets. The back-fill ships in a subsequent state update.

The decrypt-into-room_data.secrets loop was duplicated in get_response.rs and response_handler.rs::handle_load_rooms_response, but the network-update paths in room_synchronizer.rs (apply_delta_inner and update_room_state_inner) merged the new encrypted blobs into ChatRoomStateV1 and never decrypted them into the in-memory map that the renderer reads.

Net effect (Ivvor on Matrix, 2026-05-15):

  • Newly-joined member renders every message as [Encrypted message - secret v0 not available (have: [])].
  • Both sides "aren't syncing" — each sends messages encrypted at a version the other doesn't have in their HashMap.
  • Hard-refreshing fixes it, because reload re-runs the LoadRooms decryption loop.

Approach

Extract the decryption block into RoomData::repopulate_secrets_from_state(&mut self) -> usize:

  • No-op on public rooms.
  • Snapshots blobs in room_state.secrets.encrypted_secrets whose member_id == self and whose secret_version isn't already in self.secrets.
  • Decrypts each, calls self.set_secret(...).
  • Aligns self.current_secret_version with the contract's secrets.current_version (preserves prior get_response / load-rooms behaviour).

Call it from every state ingestion path:

  • get_response.rs:178+ (initial GET) — replaces the inline duplicate.
  • response_handler.rs::handle_load_rooms_response (delegate-load merge) — replaces the inline duplicate.
  • room_synchronizer.rs::apply_delta_inner — new call site, before the per-private-room actions_state rebuild so newly-decrypted action messages resolve in the same tick.
  • room_synchronizer.rs::update_room_state_inner — same.

The helper is idempotent (filtered by !self.secrets.contains_key(version)), so it's safe to call on every update without measurable cost on the steady-state case.

Testing

Three unit tests in ui/src/room_data.rs:

  • repopulate_secrets_after_delegate_backfill_picks_up_new_blob — direct reproduction of Ivvor's repro: invitee state starts with secrets empty and encrypted_secrets empty (initial GET); first call to helper is a no-op; owner delegate back-fills the blob; second call to helper decrypts it and populates the map. Pinned the idempotency case too (third call decrypts 0).
  • repopulate_secrets_skips_blobs_for_other_members — helper must not try to decrypt blobs sealed for a different member's key.
  • repopulate_secrets_is_noop_on_public_room — pinned the early-return for is_private() == false.

All 95 river-ui tests pass. Workspace cargo make test green. cargo clippy clean for the touched code (no new warnings).

Manual repro will be exercised in the batched Playwright run before the next River publish.

Other items from Ivvor's report

The "memory growth" complaint is tracked separately in #246. The "owner can't send messages" sub-symptom in the 3-member case is plausibly the same bug from the owner side — owner's tab also gets stuck if a rotation-driven update arrives that brings in a new current_version for which the owner already had the secret but the apply_delta path didn't refresh current_secret_version. This patch's current_secret_version = Some(current_version) line at the end of repopulate_secrets_from_state covers that case symmetrically.

[AI-assisted - Claude]

sanity and others added 2 commits May 16, 2026 20:57
…251)

Extract the secret-decryption block previously duplicated in get_response.rs
and response_handler.rs into RoomData::repopulate_secrets_from_state, and
call it from the two paths that were missing it — apply_delta_inner and
update_room_state_inner. Before this change those network-update paths
merged new encrypted_secrets blobs into ChatRoomStateV1 but never decrypted
them into the in-memory secrets HashMap that the renderer reads, so newly
joined private-room members saw "[Encrypted message - secret vN not
available]" until they hard-refreshed the tab.

The helper is called before the per-private-room actions_state rebuild in
both call sites so newly-decrypted action messages resolve in the same tick.

Adds three regression tests:
- repopulate_secrets_after_delegate_backfill_picks_up_new_blob — models
  Ivvor's repro exactly: empty initial GET → owner delegate back-fills →
  next state ingestion must decrypt.
- repopulate_secrets_skips_blobs_for_other_members
- repopulate_secrets_is_noop_on_public_room

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-2 fixes from the multi-model review of PR #264:

- Codex P2: also call repopulate_secrets_from_state from the
  `is_existing_room` branch in `get_response.rs` (both the
  imported-room wholesale-replace path and the existing-room
  merge path). A refresh/suspension GET that happens to be the
  first response carrying our back-filled or rotated blob would
  otherwise leave the in-memory map stale until the next
  subscription update.

- Codex P3: re-snapshot `room_secrets` AFTER repopulate in
  apply_delta_inner and update_room_state_inner. The pre-merge
  snapshot was stale when the same delta carried both a new
  blob AND new private messages, so the notification preview
  decrypted with the empty map and rendered the encrypted
  placeholder even though the UI rendered correctly.
  update_room_state_inner's pending_notification tuple is
  widened to carry the post-repopulate secrets out of the
  with_mut block.

- Testing reviewer (blocking): add a source-grep pin test
  `repopulate_secrets_call_sites_pinned` so removing the call
  from any state-ingestion path fails CI. Mirrors the
  "source-grep pin test in the driver's test module" pattern
  from freenet's bug-prevention-patterns.md.

- Code-first reviewer: expand docstring on
  repopulate_secrets_from_state to explain that the
  unconditional current_secret_version assignment relies on the
  RoomSecretsV1 invariant that current_version is monotonically
  non-decreasing under merge.

- Big-picture reviewer: AGENTS.md "Private Room Support" gets a
  new bullet documenting the in-memory secret repopulation
  invariant and pointing future authors at the source-grep pin
  test.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented May 17, 2026

Round 2 of review findings addressed in 27016a6:

Codex [P2] — Repopulate secrets for existing-room GETs. Fixed in get_response.rs::is_existing_room. Both sub-branches (imported-room wholesale-replace, existing-room merge) now call repopulate_secrets_from_state() so a refresh/suspension GET that's the first arrival of our back-filled blob populates the in-memory map.

Codex [P3] — Stale room_secrets snapshot in notifications. Fixed in both apply_delta_inner and update_room_state_inner. The pre-merge clone is gone; room_secrets is captured AFTER repopulate, so a delta carrying both a new secret AND new private messages can decrypt the notification preview correctly. update_room_state_inner's pending_notification tuple was widened to carry the secrets out of with_mut.

Testing reviewer (blocking) — regression test doesn't pin the call sites. Added repopulate_secrets_call_sites_pinned: a source-grep pin test that uses include_str! to read each ingestion-path file and assert the expected count of repopulate_secrets_from_state( call sites. Removing the call from any path fails CI. Mirrors the "source-grep pin test in the driver's test module" pattern from bug-prevention-patterns.md.

Code-first reviewer — current_secret_version downshift. Docstring expanded to explain the unconditional reassignment relies on the RoomSecretsV1 invariant (current_version == max(versions), monotonically non-decreasing under merge), so the merge that immediately precedes the call cannot move the pointer backwards.

Big-picture reviewer — AGENTS.md update. Added a bullet under "Private Room Support" documenting the in-memory-repopulation invariant and pointing future authors at the source-grep pin test.

Skeptical reviewer findings not addressed:

96 tests pass (the 3 new unit tests + the new pin test); clippy clean for touched code; the only remaining clippy warning in the touched region is a pre-existing type_complexity on the apply_delta_inner pending_notification tuple, not introduced by this PR.

Re-running multi-model review on the new HEAD SHA per the per-CODE-CONTENT rule.

[AI-assisted - Claude]

Pure comment-only changes; no functional change. Per
multi-model-review.md "Reviewers do NOT need to re-run for pure-
additive commits where the new code has clearly trivial scope
(... a `// TODO: x` comment)".

- get_response.rs: the previous comment said the wholesale
  `room_data.room_state = retrieved_state` assignment "wipes
  secrets", which is incorrect — that assignment doesn't touch
  the in-memory `secrets` HashMap (it's a separate
  #[serde(skip)] field on RoomData). Clarify why the repopulate
  is still necessary.

- room_data.rs: document the literal-substring match in the
  pin test so a future contributor doesn't accidentally inflate
  the count by mentioning the function name in a comment with
  trailing parens.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@sanity sanity merged commit 9a17f2a into main May 17, 2026
5 checks passed
@sanity sanity deleted the fix/issue-251-repopulate-secrets branch May 17, 2026 02:22
sanity added a commit that referenced this pull request May 17, 2026
Publishes the merged tip of main containing:
- #263 (issue #255): legacy delegate response no longer overwrites the
  user's currently-selected room.
- #264 (issue #251): repopulate room_data.secrets on every network
  state ingestion path so newly-joined private-room members don't
  render encrypted placeholders until refresh.
- #265 (issue #261): hide stale DM threads from the left rail
  (local-view filter with multi-device sync via OutboundDmStore).

Counter 30000306 -> 30000307.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.

Private room: new members see encrypted-only messages until refresh — secrets HashMap not repopulated on network state updates

1 participant