Skip to content

Fix player session race conditions#871

Merged
tudddorrr merged 1 commit intodevelopfrom
fix-session-race-conditions
Feb 28, 2026
Merged

Fix player session race conditions#871
tudddorrr merged 1 commit intodevelopfrom
fix-session-race-conditions

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the fix This pull request contains a fix label Feb 28, 2026
@codecov
Copy link

codecov bot commented Feb 28, 2026

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.00%. Comparing base (a26dc43) to head (2c1b32c).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/tasks/cleanupOnlinePlayers.ts 84.61% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #871      +/-   ##
===========================================
- Coverage    97.01%   97.00%   -0.02%     
===========================================
  Files          388      388              
  Lines         6137     6139       +2     
  Branches       785      785              
===========================================
+ Hits          5954     5955       +1     
  Misses          95       95              
- Partials        88       89       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude
Copy link

claude bot commented Feb 28, 2026

This PR is well-structured and the fixes are correct. No critical or major issues found.


Code Quality

The refactoring of getActivePlayerAliasIds() into a shared helper that both cleanup functions consume from a single snapshot is a clear improvement. Previously, each function independently iterated socket connections at different points in time — the shared snapshot ensures consistency between cleanupStaleOnlinePresence and cleanupUnfinishedSessions.


Potential Bugs

🔵 presenceMap query does not populate playerAlias (lines 150–172 in cleanupOnlinePlayers.ts)

The presenceMap is built with populate: ['player'] only, but the new condition accesses presence.playerAlias.id:

const presenceMap = await em.repo(PlayerPresence).find(
  { player: { id: { $in: playerIds } } },
  { populate: ['player'] },  // playerAlias not populated
)
// ...
if (presence && activePlayerAliasIds.has(presence.playerAlias.id)) {  // relies on lazy FK

This works because MikroORM exposes the primary key of unloaded ManyToOne references via their internal reference wrapper. However, it is an implicit behaviour. Adding 'playerAlias' to the populate array would make the intent explicit and match the pattern used in cleanupStaleOnlinePresence. Since the same pattern appears in the pre-existing cleanupStaleOnlinePresence code it is not blocking, but worth aligning now that this function uses it too.


Security / Performance / Test Coverage

No issues found. The test correctly targets the documented race condition (presence forced offline while socket is still alive), and all CI shards pass.

@tudddorrr tudddorrr merged commit 21d756a into develop Feb 28, 2026
8 of 10 checks passed
@tudddorrr tudddorrr deleted the fix-session-race-conditions branch February 28, 2026 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix This pull request contains a fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant