Skip to content

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the release This pull request contains a release label Dec 4, 2025
@claude
Copy link

claude bot commented Dec 4, 2025

🔴 Critical

EntityManager Fork Timing in Socket Constructor (src/index.ts:53)

The Socket class now receives a forked EntityManager in the constructor:

app.context.wss = new Socket(server, (app.context.em as EntityManager).fork())

However, the Socket class uses RequestContext.create(this.em, ...) in handleConnection and handleMessage methods. This pattern expects this.em to be the base EntityManager, not a fork. When you call RequestContext.create() with an already-forked EM, it can cause identity map conflicts and stale data issues.

Fix: Pass the base EM instead:

app.context.wss = new Socket(server, app.context.em as EntityManager)

The RequestContext will handle forking internally when needed.


🟡 Major

Module-Level Singleton Pattern Issues

In EventAPIService and GameStatAPIService, queue handlers are now module-level singletons:

let queueHandler: FlushEventsQueueHandler

export default class EventAPIService extends APIService {
  constructor() {
    super()
    if (!queueHandler) {
      queueHandler = new FlushEventsQueueHandler()
    }
  }
}

This works but creates a hidden dependency. If the service is ever instantiated before the module loads, or if there's any dynamic import/reload scenario, this could cause subtle bugs.

Suggested improvement: Move to a factory function or make the queue handler initialization more explicit. However, since koa-clay likely instantiates services once during route setup, this is probably safe in practice.


Parallel EntityManager Usage (src/lib/queues/game-metrics/flush-metrics-queue-handler.ts:27-28)

Changed from sequential with fresh EM to parallel with shared EM:

const promises = players.map((player) => player.checkGroupMemberships(em))
await Promise.all(promises)

This shares a single EntityManager across parallel async operations. MikroORM's EM is not designed for concurrent operations on the same instance. This could lead to identity map conflicts or race conditions when flushing.

Fix: Fork the EM for each player:

const promises = players.map((player) => player.checkGroupMemberships(em.fork()))
await Promise.all(promises)

Or keep sequential execution as before.


🔵 Minor

Result Cache Duration Hardcoded

Added result caching in multiple places with hardcoded 1000ms TTL:

  • src/socket/socketConnection.ts:48
  • src/socket/listeners/gameChannelListeners.ts:28

Consider extracting cache durations to constants for easier tuning.


Simplified Cleanup Logic May Need Monitoring

The cleanupOnlinePlayers task was drastically simplified - the old version had complex logic for inferring session end times from socket events. The new version just deletes stale sessions. This is cleaner but ensure monitoring is in place to catch any unfinished sessions that should have been closed properly.

@tudddorrr tudddorrr merged commit 9c78fbe into main Dec 4, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release This pull request contains a release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants