SSE registerClient races client.id insertion vs close handler

[jakebromberg]

Problem

ServerEventsManager.registerClient has a TOCTOU between the close-handler registration and the final this.clients.set(client.id, client) insertion. The order is: (1) generate client.id, (2) set inactivity timer keyed by client.id, (3) register client.res.on('close', ...) handler, (4) write SSE headers, (5) write the initial connection event, (6) insert into this.clients. If the socket closes between steps 3 and 6, the close handler's unsubAll(client.id) runs on a not-yet-inserted client — it clears the timer (the only timer that exists for this id) but the this.clients.delete(client.id) and this.clientTopics.delete(client.id) are no-ops. Then step 6 inserts the (already-dead) client into this.clients with no live socket and no inactivity timer at all. Because no timer exists, the leak is permanent until process restart, not bounded by INACTIVITY_TIMEOUT_MS.

A subscribe call from another path that looks up the client by id will find it, register topic subscriptions, and every subsequent broadcast attempts to write to the dead socket. The write throws, the catch arm calls unsubAll, the client is finally cleaned up — but only after the first broadcast for one of its topics, which may be hours later on quiet topics.

Evidence

apps/backend/utils/serverEvents.ts:80-118:

registerClient = (res: Response): EventClient => {
  const client: EventClient = {
    id: crypto.randomUUID(),
    res: res,
  };

  this.resetTimeout(client.id);

  client.res.on('close', () => {
    const timeoutId = this.clientTimeouts.get(client.id);
    if (timeoutId) {
      clearTimeout(timeoutId);
      this.clientTimeouts.delete(client.id);
    }
    this.unsubAll(client.id);
  });

  // Send SSE headers
  client.res.writeHead(200, { ... });

  // Send initial connection event with client ID
  const connectionEvent = { ... };
  client.res.write(`data: ${JSON.stringify(connectionEvent)}\n\n`);

  this.clients.set(client.id, client);

  return client;
};

unsubAll (lines 183-206) deletes from clientTopics + clients + clientTimeouts maps; all no-ops if the id was never inserted.

Reproduction

1. Open an SSE connection to a registerClient route, then close the underlying TCP socket immediately (before the server's response is flushed).
2. Verify on the server: clients map contains the (now dead) entry and clientTimeouts does not have a corresponding key.
3. Issue a subscribe + broadcast to a quiet topic — observe the dead client receives the write attempt, error, gets cleaned up only at first broadcast.

Acceptance criteria

• Insert client into this.clients BEFORE registering the close handler (or use an "added" flag the close handler checks).
• Add a guard in the close handler that detects the not-yet-inserted case and refuses to no-op, instead marking the client to be cleaned up if/when registration completes.
• Unit test covering the close-before-insert race (mock a Response whose on('close') fires synchronously during construction or before the final clients.set runs).

Related

• Adjacent bug filed separately: SSE has no heartbeat; quiet topics disconnect healthy clients via inactivity timer.

[jakebromberg]

PR: #1405