feat: Prevent GameServer from restarting after ending by introducin…
#2923
Conversation
…g a `_hasEnded` flag and adding lifecycle tests.
|
|
WalkthroughA private Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/server/GameServer.ts`:
- Around line 74-75: The file GameServer.ts has Prettier formatting issues; run
the formatter or apply Prettier rules to fix formatting so CI passes: run npx
prettier --write src/server/GameServer.ts (or run project-wide prettier) and
commit the changes; ensure formatting is consistent in the GameServer class
(e.g., around the private _hasEnded property and surrounding lines) so the
Prettier check in CI no longer fails.
🧹 Nitpick comments (2)
tests/server/GameLifecycle.test.ts (1)
6-94: Consider adding a test for the timeout scenario.The tests cover the core fix, but the PR objective mentions a
setTimeoutthat schedulesgame.start()after 2 seconds inGameManager.tick(). Since you have fake timers set up, consider adding a test that simulates this scenario:it("should not start when delayed start fires after end()", async () => { const game = new GameServer(/* ... */); // Simulate the setTimeout from GameManager.tick() const delayedStart = setTimeout(() => game.start(), 2000); // End the game before the timeout fires await game.end(); // Advance timers to trigger the delayed start vi.advanceTimersByTime(2000); // start() should have been a no-op expect((game as any).endTurnIntervalID).toBeUndefined(); });This would directly mirror the bug scenario from the PR description.
src/server/GameServer.ts (1)
639-691: Consider adding re-entry guard for robustness.While setting
_hasEnded = truefirst helps, you could add an early return if_hasEndedis already true to skip the cleanup logic entirely on repeated calls:♻️ Optional: Add explicit re-entry guard
async end() { + if (this._hasEnded) { + return; + } this._hasEnded = true; // Close all WebSocket connections if (this.endTurnIntervalID) {This is optional since the current code handles multiple calls safely (clearInterval on undefined is harmless, and iterating empty sets is fine). The explicit guard just makes the intent clearer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/server/GameServer.tstests/server/GameLifecycle.test.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/server/GameServer.tstests/server/GameLifecycle.test.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/server/GameServer.tstests/server/GameLifecycle.test.ts
📚 Learning: 2026-01-09T06:10:44.499Z
Learnt from: plazmaezio
Repo: openfrontio/OpenFrontIO PR: 2716
File: tests/AllianceAcceptNukes.test.ts:85-87
Timestamp: 2026-01-09T06:10:44.499Z
Learning: In tests under tests/**, objects may stack on the same tile coordinates (e.g., MissileSilo). When writing tests that involve building/placing structures or cooldowns, assume multiple items can share coordinates; adjust assertions to account for stacking, and explicitly manage or reset the stack state where necessary.
Applied to files:
tests/server/GameLifecycle.test.ts
🧬 Code graph analysis (1)
tests/server/GameLifecycle.test.ts (1)
src/server/GameServer.ts (1)
GameServer(34-1022)
🪛 GitHub Actions: 🧪 CI
src/server/GameServer.ts
[error] 1-1: Prettier formatting check failed. Code style issues found in 'src/server/GameServer.ts'. Run 'npx prettier --write .' to fix.
🔇 Additional comments (7)
tests/server/GameLifecycle.test.ts (4)
10-28: Good test setup with fake timers and mocks.The setup is clean. One small note:
vi.clearAllTimers()inafterEachclears pending timers, butvi.useRealTimers()is not called. This is fine sincevi.restoreAllMocks()handles cleanup, but consider addingvi.useRealTimers()for clarity if other tests in the suite rely on real timers.
30-50: Test covers the core fix well.This test directly addresses the PR objective: preventing
start()from running afterend()has been called. The assertions correctly verify both the interval state and thehasStarted()return value.
52-77: Good coverage of normal lifecycle flow.This test validates that
start()creates the interval andend()properly cleans up. The inclusion ofgameMapandgameMapSizein the config ensuresstart()can succeed.
79-94: Nice idempotency test.Testing that multiple
end()calls do not throw ensures robustness. The use ofresolves.toBeUndefined()is the right way to assert an async function completes without error.src/server/GameServer.ts (3)
74-75: Field declaration is clean.The
_hasEndedflag follows the same naming pattern as_hasStartedand_hasPrestarted. The pipeline failure mentions Prettier formatting—this might be related to the blank line at line 75. Runnpx prettier --write src/server/GameServer.tsto fix.
536-539: Core fix is correct.Adding
this._hasEndedto the guard condition instart()directly addresses the bug. Ifend()has been called, any subsequentstart()call (including from a pendingsetTimeout) will be a no-op.
639-645: Proper placement of the_hasEndedflag.Setting
_hasEnded = trueas the first action inend()ensures:
- Any concurrent
start()call will be blocked immediately- Multiple
end()calls are safe (idempotent cleanup)The explicit
this.endTurnIntervalID = undefinedafterclearIntervalis good practice—it makes the state change visible and enables tests to verify cleanup.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 129: The package.json file has a Prettier formatting failure; run the
formatter to fix it by executing npx prettier --write package.json (or your
project's lint-staged/precommit prettier script) and commit the updated
package.json so CI formatting checks pass; target the package.json file itself
and ensure no other unrelated changes are included in the commit.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.jsonsrc/server/GameServer.tstests/server/GameLifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/GameServer.ts
- tests/server/GameLifecycle.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T05:03:18.745Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/core/ApiSchemas.ts:2-2
Timestamp: 2025-08-09T05:03:18.745Z
Learning: After migrating to Zod v4 in the OpenFrontIO project, the standard import statement `import { z } from "zod"` should be used across the entire codebase, not `import { z } from "zod/v4"`.
Applied to files:
package.json
🪛 GitHub Actions: 🧪 CI
package.json
[warning] 1-1: Prettier reported code style issues in package.json. Run 'npx prettier --write package.json' to fix.
🔇 Additional comments (1)
package.json (1)
15-15: Nice: targeted server test script.
Line 15 adds a focused test command, which makes server test runs clearer and faster.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
#2919
In GameManager.tick(), when a game becomes active but hasn't started, a setTimeout for game.start() is scheduled with a 2-second delay. If the game finishes or is cancelled within those 2 seconds, game.end() is called, which clears the existing interval. However:
1.The 2-second timeout still fires. game.start() executes.
2. A NEW setInterval is created for turn execution.
3.Since the game is already ending/finished, it's removed from GameManager.games, but the interval continues to run forever in the background
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
codimo