Fix native P2P startup adapter race#6614
Conversation
📝 WalkthroughWalkthrough
ChangesGame store initialization
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/stores/gameStore.ts`:
- Around line 418-433: The initialization transaction around
adapter.initializeGame and the first getSnapshot/commit must roll back all
matching session state on any failure, not just clear adapter. Extend the
try/catch through the initial snapshot and commit, and on failure remove the
adapter plus restore or clear any game state published by this initialization,
while preserving unrelated newer session updates by checking the adapter/session
identity before rollback; ensure async state updates during initialization
cannot leave partial committed state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d131170-a8d7-4dde-8d03-35d197d1c975
📒 Files selected for processing (2)
client/src/stores/__tests__/gameStore.test.tsclient/src/stores/gameStore.ts
| set({ adapter }); | ||
| let initResult; | ||
| try { | ||
| initResult = await adapter.initializeGame( | ||
| deckData, | ||
| formatConfig, | ||
| playerCount, | ||
| matchConfig, | ||
| firstPlayer, | ||
| ); | ||
| } catch (error) { | ||
| // A failed initialization must not leave a transport that never | ||
| // produced a playable game attached to the store. | ||
| if (get().adapter === adapter) set({ adapter: null }); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make initialization cleanup transactional.
The catch only clears the adapter. If initializeGame publishes a snapshot and then rejects, committed game state can remain while adapter becomes null. Conversely, if getSnapshot() fails at Line 437, the adapter remains attached despite initialization failure. Extend the failure scope through the first successful snapshot/commit and roll back the matching session state, or stage remote commits until initialization succeeds.
As per path instructions, async races and state updates during initialization must be checked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/stores/gameStore.ts` around lines 418 - 433, The initialization
transaction around adapter.initializeGame and the first getSnapshot/commit must
roll back all matching session state on any failure, not just clear adapter.
Extend the try/catch through the initial snapshot and commit, and on failure
remove the adapter plus restore or clear any game state published by this
initialization, while preserving unrelated newer session updates by checking the
adapter/session identity before rollback; ensure async state updates during
initialization cannot leave partial committed state.
Source: Path instructions
1 participant
Summary by CodeRabbit
Bug Fixes
Tests