Conversation
The ensure_room_migrated() function was fully implemented but never called. When bundled WASM changed (e.g., after a release), riverctl would try to GET/UPDATE the new contract key which had no state yet, causing timeouts. Two bugs fixed: 1. Wire ensure_room_migrated() into get_room() and subscribe_and_stream() so migration runs automatically before any room operation. 2. Fix the migration-needed check: load_rooms() already regenerates the stored key to match current WASM, so comparing stored vs expected was always equal. Now checks previous_contract_key instead. Closes #192 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace unwrap_or("unknown") with unwrap() since we already early-return
when previous_contract_key_str is None
- Simplify test assertions using as_deref() pattern
- Remove unnecessary mut binding in test
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
subscribe_and_stream called ensure_room_migrated then get_room which also calls ensure_room_migrated. Removed the explicit call since get_room handles migration internally. Also added assertion that previous_contract_key is None for rooms with current WASM. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Clear previous_contract_key after ALL successful migration paths (timeout and catch-all), not just the "already exists" path. Prevents redundant migration re-attempts on subsequent calls. 2. Restore "Room not found" guard in subscribe_and_stream that was lost when refactoring to use get_room for migration. 3. Fix misleading test comment about load_rooms behavior during add_room (file doesn't exist yet, so no regeneration occurs). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ensure_room_migrated()was fully implemented but never called from any room operation. When the bundled room contract WASM changed (e.g., after a release), riverctl would compute the new contract key and try to GET/UPDATE against it — but no state existed at that key yet, causing 30-second timeouts on every operation (message send,member list, etc.).Additionally, the migration-needed check was a no-op:
load_rooms()already regeneratescontract_keyto match the current WASM beforeensure_room_migrated()reads it, so thestored_key == expected_keycomparison was always true.Approach
Wire
ensure_room_migrated()intoget_room()andsubscribe_and_stream()— these are the two entry points that all room operations flow through. Migration runs transparently before the first GET.Fix the migration-needed check — instead of comparing the (already-regenerated) stored key against expected, check whether
previous_contract_keyis set.load_rooms()sets this field when it detects a key mismatch, which is exactly the signal that migration is needed.Testing
test_load_rooms_sets_previous_contract_key_on_mismatchvalidates thatload_rooms()correctly detects WASM changes and populatesprevious_contract_keycargo clippyandcargo fmtcleanCloses #192
[AI-assisted - Claude]