security: add catch block to async game.end() call (SEC-05)#3948
security: add catch block to async game.end() call (SEC-05)#3948berkelmali wants to merge 2 commits into
Conversation
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.
WalkthroughThe GameManager.tick() method's GamePhase.Finished handling replaces a synchronous game.end() call with an asynchronous game.end().catch(...) pattern. Promise rejections from the end operation are now explicitly logged via this.log.error instead of being captured by the surrounding try/catch block. ChangesGame End Error Handling
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/GameManager.ts (1)
133-139: ⚡ Quick winThe try-catch block no longer serves a purpose.
Since
game.end()returns a promise and is not awaited, the try-catch block cannot catch errors from the async operation. Promise rejections are only handled by the.catch()handler on line 134.If fire-and-forget is the intended pattern, remove the try-catch block for clarity:
🧹 Simplified code without redundant try-catch
- if (phase === GamePhase.Finished) { - try { - game.end().catch((error: any) => { - this.log.error(`error ending game ${id}: ${error}`); - }); - } catch (error) { - this.log.error(`error ending game ${id}: ${error}`); - } - } else { + if (phase === GamePhase.Finished) { + game.end().catch((error: any) => { + this.log.error(`error ending game ${id}: ${error}`); + }); + } else {If cleanup must complete before removing the game from the manager, await the promise instead:
⏳ Alternative: await cleanup before removing game
- if (phase === GamePhase.Finished) { - try { - game.end().catch((error: any) => { - this.log.error(`error ending game ${id}: ${error}`); - }); - } catch (error) { - this.log.error(`error ending game ${id}: ${error}`); - } - } else { + if (phase === GamePhase.Finished) { + try { + await game.end(); + } catch (error) { + this.log.error(`error ending game ${id}: ${error}`); + } + } else {Note: This would require marking
tick()as async.🤖 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/GameManager.ts` around lines 133 - 139, Remove the redundant try-catch around the fire-and-forget call to game.end(): keep the promise and its .catch(...) handler on game.end() and delete the outer try { ... } catch { ... } block in GameManager.tick (or wherever this appears) so promise rejections are handled only by the existing .catch; if instead you require synchronous cleanup, change tick to async and replace game.end().catch(...) with await game.end() inside a try/catch to properly await and handle errors.
🤖 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/GameManager.ts`:
- Around line 133-139: Remove the redundant try-catch around the fire-and-forget
call to game.end(): keep the promise and its .catch(...) handler on game.end()
and delete the outer try { ... } catch { ... } block in GameManager.tick (or
wherever this appears) so promise rejections are handled only by the existing
.catch; if instead you require synchronous cleanup, change tick to async and
replace game.end().catch(...) with await game.end() inside a try/catch to
properly await and handle errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f7cd55b-e6ca-4cc8-aa97-8ccb177bb136
📒 Files selected for processing (1)
src/server/GameManager.ts
1 participant
Description: In
TransportShipExecution.ts, a call togame.end()was not awaited and lacked a.catch()block. This could lead to unhandled promise rejections if the game end logic fails. Fix: Added a.catch(e => logger.error(e))block to thegame.end()call.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires