Stop storing managed entities in socket connections

[tudddorrr]

No description provided.

[claude]

🔍 PR Review: Stop storing managed entities in socket connections

This PR refactors socket connections to store primitive values instead of managed MikroORM entities, which is a solid architectural improvement. Here are the issues that need attention:

---

🟡 Code Quality & Best Practices

Incorrect cache key in sendMessages (src/socket/messages/socketMessage.ts:37)

The comment says "pass empty object as data since we already have the serialised message," but this creates a subtle bug. If the data object contains an errorCode property, it's needed for event tracking in SocketConnection.sendMessage() at line 108:

code: 'errorCode' in data ? (data.errorCode as SocketErrorCode) : null

By passing {} as T, error codes won't be tracked properly in telemetry.

Fix: Either pass the original data object, or extract errorCode before serialization:

const errorCode = 'errorCode' in data ? (data.errorCode as SocketErrorCode) : null
const message = JSON.stringify({ res: type, data })
await Promise.all(conns.map((conn) => conn.sendMessage(type, {} as T, message, errorCode)))

---

🔴 Performance Concerns

Cache collision in gameChannelListeners (src/socket/listeners/gameChannelListeners.ts:28)

The cache key channel-listener-members-${data.channel.id} doesn't include gameId. If two different games have channels with the same ID (e.g., channel ID 1), they'll share the same cache entry, causing cross-game data leakage.

Fix:

...getResultCacheOptions(`channel-listener-members-${conn.gameId}-${data.channel.id}`, 1000)

Cache collision in SocketConnection.getPlayerAlias (src/socket/socketConnection.ts:48)

Same issue: socket-connection-alias-${this.playerAliasId} doesn't include gameId. PlayerAlias IDs could collide across games.

Fix:

getResultCacheOptions(`socket-connection-alias-${this.gameId}-${this.playerAliasId}`, 1000)

---

🟡 Security & Data Integrity

Short cache TTL with managed entities (multiple locations)

The 1-second cache TTL (1000ms) for entity queries is risky. If a GameChannel is deleted or a PlayerAlias is removed from a channel, connections could hold stale references for up to 1 second, potentially allowing unauthorized message delivery.

While 1 second seems short, in high-throughput scenarios this could lead to messages being sent to channels after a player has been removed or to deleted player aliases.

Recommendation: Either accept this risk with documentation, or use cache invalidation when channels/memberships change.

---

✅ Test Coverage

No test changes included. Given this is a refactor affecting core socket behavior (entity reference changes, caching strategy), tests should verify:

• Cache keys are properly scoped by game
• Error codes are tracked correctly in telemetry
• Concurrent operations don't cause cache-related issues

---

Priority: Address the cache collision bugs (🔴) first, then the errorCode tracking issue (🟡).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.21%. Comparing base (330f5bc) to head (f887d10).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #721   +/-   ##
========================================
  Coverage    99.21%   99.21%           
========================================
  Files          225      225           
  Lines        16602    16616   +14     
  Branches      1536     1536           
========================================
+ Hits         16471    16485   +14     
  Misses         128      128           
  Partials         3        3           

☔ 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.