Optimize 1v1 Disconnect Grace Period (Refined #3945)#3972
Open
berkelmali wants to merge 10 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a 1v1 ranked disconnect grace period (delays immediate win on disconnect), tightens server-side winner consensus and desync thresholds, makes game end error logging promise-based, and enforces Bearer-token auth with persistent-ID match for singleplayer archival. Changes1v1 Ranked Disconnect Grace Period
Server-Side Winner Consensus & Validation
Game Finalization & Singleplayer Authentication
Sequence DiagramssequenceDiagram
participant Game
participant WinCheckExecution
participant PlayerA
participant PlayerB
PlayerA->>WinCheckExecution: connected
PlayerB->>WinCheckExecution: connected
PlayerB->>WinCheckExecution: disconnect
WinCheckExecution->>WinCheckExecution: start or resume disconnectGraceTick
loop ticks < DISCONNECT_GRACE_TICKS
Game->>WinCheckExecution: checkWinnerFFA()
WinCheckExecution->>Game: no winner
end
alt grace expired
Game->>WinCheckExecution: checkWinnerFFA()
WinCheckExecution->>Game: setWinner(connected or tile-leading)
end
PlayerB->>WinCheckExecution: reconnect
WinCheckExecution->>WinCheckExecution: reset disconnectGraceTick
sequenceDiagram
participant WebSocketClient
participant GameServer
participant ArchiveService
WebSocketClient->>GameServer: close / disconnect
alt game started
GameServer->>GameServer: checkWinnerConsensus()
GameServer->>GameServer: iterate winnerVotes
GameServer->>GameServer: require votes > half(active unique IPs)
alt threshold met
GameServer->>GameServer: set this.winner
GameServer->>ArchiveService: archive(game)
GameServer->>GameServer: log winnerKey
else
GameServer->>GameServer: no winner set
end
else
GameServer->>GameServer: skip consensus check
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/GameServer.ts (1)
1248-1267: 💤 Low valueEdge case: winner declared when no active clients remain.
When all clients disconnect,
activeUniqueIPs.sizebecomes 0. The conditionpotentialWinner.ips.size * 2 > 0is true for any vote count ≥ 1, so a winner is declared based on prior votes.This is probably fine (honoring votes cast before disconnect), but verify this matches intended behavior. If no winner should be declared when zero clients remain, add a guard:
private checkWinnerConsensus() { if (this.winner !== null) { return; } const activeUniqueIPs = new Set(this.activeClients.map((c) => c.ip)); + if (activeUniqueIPs.size === 0) { + return; + } for (const [winnerKey, potentialWinner] of this.winnerVotes.entries()) {🤖 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 `@src/server/GameServer.ts` around lines 1248 - 1267, The checkWinnerConsensus method can declare a winner when activeUniqueIPs.size is 0; add a guard at the start of checkWinnerConsensus to return early if there are no active unique IPs to avoid declaring winners after all clients disconnect. Update the function (checkWinnerConsensus) to check activeClients/activeUniqueIPs size and skip the winnerVotes loop (and avoid calling archiveGame or setting this.winner) when activeUniqueIPs.size === 0 so prior votes don't trigger a win in that edge case.
🤖 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.
Nitpick comments:
In `@src/server/GameServer.ts`:
- Around line 1248-1267: The checkWinnerConsensus method can declare a winner
when activeUniqueIPs.size is 0; add a guard at the start of checkWinnerConsensus
to return early if there are no active unique IPs to avoid declaring winners
after all clients disconnect. Update the function (checkWinnerConsensus) to
check activeClients/activeUniqueIPs size and skip the winnerVotes loop (and
avoid calling archiveGame or setting this.winner) when activeUniqueIPs.size ===
0 so prior votes don't trigger a win in that edge case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c351041-0cc9-4185-b845-de204531d296
📒 Files selected for processing (5)
src/core/execution/WinCheckExecution.tssrc/server/GameManager.tssrc/server/GameServer.tssrc/server/Worker.tstests/core/executions/WinCheckExecution.test.ts
game.end() is async but was called without await inside a try/catch. The try/catch only catches synchronous errors. Added .catch() to the returned promise to prevent unhandled promise rejections from crashing the server.
berkelmali
force-pushed
the
feature/1v1-grace-period-optimized
branch
from
64d0c0d to
cb72356
Compare
…50 ticks, add defensive tests
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/core/executions/WinCheckExecution.test.ts (2)
572-629:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment: grace period is 150 ticks, not 300.
Same issue as above at line 619.
📝 Suggested fix
- // Advance past grace period (300 ticks) + // Advance past grace period (150 ticks) game.endSpawnPhase(); for (let i = 0; i < 310; i++) {🤖 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 `@tests/core/executions/WinCheckExecution.test.ts` around lines 572 - 629, The test advances time past the wrong grace duration (uses ~300 ticks) so the assertion may be flaky; update the tick advancement to exceed the actual grace period (150 ticks) — e.g. call game.endSpawnPhase() then loop game.executeNextTick() for ~160 ticks (or 151+) instead of 310, so WinCheckExecution.checkWinnerFFA() (class WinCheckExecution, method checkWinnerFFA) observes the grace expiry and setWinner spy is called; adjust the for-loop/count in this test accordingly.
407-460:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment: grace period is 150 ticks, not 300.
The comment at line 450 says "300 ticks" but
DISCONNECT_GRACE_TICKSwas reduced to 150. The test passes because 310 > 150, but the comment is misleading.📝 Suggested fix
- // Advance ticks past grace period (300 ticks) + // Advance ticks past grace period (150 ticks) game.endSpawnPhase(); for (let i = 0; i < 310; i++) {🤖 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 `@tests/core/executions/WinCheckExecution.test.ts` around lines 407 - 460, The test's comment and hardcoded tick count are stale: DISCONNECT_GRACE_TICKS is 150 (not 300). Update the comment near the loop in the "should set winner after grace period expires" test to reflect 150 ticks, and replace the hardcoded advance of 310 ticks with a reference to the actual constant (e.g., advance for DISCONNECT_GRACE_TICKS + 10) so the test remains correct and self-documenting; locate the loop and comment around WinCheckExecution.checkWinnerFFA and game.executeNextTick to make this change.
🧹 Nitpick comments (1)
tests/core/executions/WinCheckExecution.test.ts (1)
681-739: ⚖️ Poor tradeoffConsider mocking
elapsedGameSeconds()instead of running 102,000 ticks.This loop executes
game.executeNextTick()over 100,000 times to reach the 170-minute hard limit. This is expensive and slows down the test suite.A simpler approach: mock
game.elapsedGameSeconds()to return a value past the hard limit.♻️ Suggested refactor
// First call starts grace period winCheck.checkWinnerFFA(); expect(setWinnerSpy).not.toHaveBeenCalled(); - // Simulate that the game has been running for 170+ minutes - // by advancing past the hard time limit (170 * 60 * 10 ticks) - game.endSpawnPhase(); - const hardLimitTicks = 170 * 60 * 10; - for (let i = 0; i < hardLimitTicks; i++) { - game.executeNextTick(); - } + // Mock elapsedGameSeconds to simulate 170+ minutes without running ticks + game.elapsedGameSeconds = vi.fn(() => 170 * 60 + 1); // Now check again -- hard time limit should override grace period winCheck.checkWinnerFFA();🤖 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 `@tests/core/executions/WinCheckExecution.test.ts` around lines 681 - 739, Replace the expensive loop that advances 170*60*10 ticks with a mock of game.elapsedGameSeconds() to return a value past the hard limit (e.g. 171*60), then call game.endSpawnPhase() and invoke winCheck.checkWinnerFFA() as before; specifically, remove the for-loop calling game.executeNextTick() and instead use a spy/mock on game.elapsedGameSeconds() (e.g. vi.spyOn(game, "elapsedGameSeconds").mockReturnValue(...)) so checkWinnerFFA() sees the elapsed time and setWinner (game.setWinner) is asserted the same way; ensure any spy is restored if needed after the test.
🤖 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.
Outside diff comments:
In `@tests/core/executions/WinCheckExecution.test.ts`:
- Around line 572-629: The test advances time past the wrong grace duration
(uses ~300 ticks) so the assertion may be flaky; update the tick advancement to
exceed the actual grace period (150 ticks) — e.g. call game.endSpawnPhase() then
loop game.executeNextTick() for ~160 ticks (or 151+) instead of 310, so
WinCheckExecution.checkWinnerFFA() (class WinCheckExecution, method
checkWinnerFFA) observes the grace expiry and setWinner spy is called; adjust
the for-loop/count in this test accordingly.
- Around line 407-460: The test's comment and hardcoded tick count are stale:
DISCONNECT_GRACE_TICKS is 150 (not 300). Update the comment near the loop in the
"should set winner after grace period expires" test to reflect 150 ticks, and
replace the hardcoded advance of 310 ticks with a reference to the actual
constant (e.g., advance for DISCONNECT_GRACE_TICKS + 10) so the test remains
correct and self-documenting; locate the loop and comment around
WinCheckExecution.checkWinnerFFA and game.executeNextTick to make this change.
---
Nitpick comments:
In `@tests/core/executions/WinCheckExecution.test.ts`:
- Around line 681-739: Replace the expensive loop that advances 170*60*10 ticks
with a mock of game.elapsedGameSeconds() to return a value past the hard limit
(e.g. 171*60), then call game.endSpawnPhase() and invoke
winCheck.checkWinnerFFA() as before; specifically, remove the for-loop calling
game.executeNextTick() and instead use a spy/mock on game.elapsedGameSeconds()
(e.g. vi.spyOn(game, "elapsedGameSeconds").mockReturnValue(...)) so
checkWinnerFFA() sees the elapsed time and setWinner (game.setWinner) is
asserted the same way; ensure any spy is restored if needed after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba4593c2-6b16-49f2-bdc9-d1dd4684b82d
📒 Files selected for processing (2)
src/core/execution/WinCheckExecution.tstests/core/executions/WinCheckExecution.test.ts
…unt in assertions and mocking elapsedGameSeconds in hard time limit test
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/executions/WinCheckExecution.test.ts (1)
735-738: 💤 Low valueMove spy cleanup after assertions.
The comment on line 735 says "after assertion" but
mockRestore()is called on line 736, which is BEFORE the assertions on lines 737-738. While this works because the assertions don't need the mock, it's clearer to clean up after all test logic completes.Suggested reordering
- // Ensure spy is cleaned up after assertion - elapsedSpy.mockRestore(); expect(setWinnerSpy).toHaveBeenCalledWith(human1, expect.anything()); expect(winCheck.isActive()).toBe(false); + + // Clean up spy + elapsedSpy.mockRestore();🤖 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 `@tests/core/executions/WinCheckExecution.test.ts` around lines 735 - 738, The spy cleanup call elapsedSpy.mockRestore() is executed before the test assertions; move that call after the assertions so cleanup happens last. Locate the block with elapsedSpy.mockRestore(), expect(setWinnerSpy).toHaveBeenCalledWith(human1, expect.anything()), and expect(winCheck.isActive()).toBe(false) and reorder so mockRestore() is called after the two expect(...) assertions involving setWinnerSpy and winCheck.isActive().
🤖 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.
Nitpick comments:
In `@tests/core/executions/WinCheckExecution.test.ts`:
- Around line 735-738: The spy cleanup call elapsedSpy.mockRestore() is executed
before the test assertions; move that call after the assertions so cleanup
happens last. Locate the block with elapsedSpy.mockRestore(),
expect(setWinnerSpy).toHaveBeenCalledWith(human1, expect.anything()), and
expect(winCheck.isActive()).toBe(false) and reorder so mockRestore() is called
after the two expect(...) assertions involving setWinnerSpy and
winCheck.isActive().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3289fc58-b992-47cc-b658-56dbeb91173e
📒 Files selected for processing (1)
tests/core/executions/WinCheckExecution.test.ts
…s is empty to avoid declaring winners on disconnects
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.
Description:
Add a 30-second grace period before declaring a disconnect winner in 1v1 ranked games. Previously, a momentary WiFi blip would instantly declare the opponent as winner. Now the system waits 300 ticks (30s) and resets the timer if the player reconnects.
This is a highly optimized version of PR #3945. It utilizes a zero-allocation single-pass loop in WinCheckExecution.ts to track connected and disconnected humans instead of performing multiple array filters every 10 ticks, significantly reducing Garbage Collection overhead in the core game loop.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires