Fix server crash: stale connection's heartbeat reading a deleted member

Reported directly from the container's crash output: TypeError: Cannot
read properties of undefined (reading 'isPaused') inside SocketGateway's
"playstate" handler, taking down the entire Node process - every active
party, not just one connection.

Root cause: party.members is keyed purely by userId, with no per-device
identity yet (the gap the "same account, different device" feature also
needs - see docs/architecture.md). Two connections for the same userId
share one map slot; PartyService.join/create both unconditionally
overwrite it. When the connection currently occupying that slot is the
party's last member and leaves, leave() deletes the entry and - by
design, since there's no successor - never reassigns the now-stale
party.hostUserId. The OTHER connection for that same userId, still fully
connected with its own closure referencing the same now-destroyed Party
object, sends its regular 3s heartbeat next: the host check passes
(stale userId equals stale hostUserId on both sides), then
party.members.get(userId)! finds nothing at all. That's the crash.

Fixed at two levels:
- server/src/realtime/SocketGateway.ts: the "playstate" handler's
  unchecked `!` is now a real `if (!member) return`, matching the guard
  every other handler already used for the same lookup.
- Every socket.on(...) in attach() now goes through a new safe()
  wrapper (log-and-swallow on an unexpected exception) - Socket.IO has
  no equivalent of Express's error-handling middleware (asyncRoute()
  already covers every REST route for exactly this), so a synchronous
  throw from any listener, this one or a future one, would otherwise
  keep crashing the whole process outright.
- server/test/socketGateway.test.ts: a regression test reproducing the
  exact sequence. Verified it's meaningful, not vacuous, by temporarily
  reverting just the specific guard and confirming that same test
  correctly fails before restoring the fix.

Deliberately not changing how join/leave share that one map slot in this
pass - that's the real design work the fuller device-detection feature
still needs. This stops it from being able to crash the server in the
meantime, regardless of how that gets resolved.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

docs/architecture.md

@@ -1510,6 +1510,72 @@ before the video player ever renders; a party's host still sees the
 settings gear open/close correctly inside that same run (see the `.hide`
 fix above).
 
+## A real server crash: one userId, two connections, one map slot
+
+Reported directly from the running container's own crash output, not
+found by testing: `TypeError: Cannot read properties of undefined
+(reading 'isPaused')` inside `SocketGateway`'s `"playstate"` handler,
+which took the entire Node process down - every active party, not just
+one connection. `party.members` is a `Map<userId, PartyMember>` - there's
+no per-device identity at all yet (the same gap behind the "same account,
+different device" feature asked for around the same time - see the
+Home/party-list section above for the simpler, already-handled half of
+that: your own listen party no longer offers "Rejoindre" for *this* tab,
+but nothing yet distinguishes a genuinely different device sharing your
+account). Two connections for the same userId - two tabs, a second
+device, a `POST /api/parties/:id/join` retried - end up sharing that one
+map slot: `PartyService.join`/`.create` both just `members.set(userId,
+newMember)`, unconditionally overwriting whichever member was there.
+
+The actual crash needed one more step, and it's a real gap in
+`PartyService.leave`, not just the sharing itself: when the connection
+*currently occupying the slot* is the party's last member and leaves,
+`leave()` deletes it, finds no successor to hand the host role to, and -
+by design, per its own comment ("this stale value is never read again",
+since the party is destroyed in the same call) - never updates
+`party.hostUserId`. That assumption held as long as nothing else could
+still reach the now-destroyed `Party` object. Something could: the
+*other* connection for that same userId, still fully connected, whose own
+closure captured the exact same `Party` object reference back when it
+first joined. Its regular 3-second heartbeat (`"playstate"`) fires next,
+checks `userId !== party.hostUserId` - both sides of that comparison are
+the same stale userId, so it passes - and then reads
+`party.members.get(userId)!` on a map that no longer has that entry at
+all. `undefined!.isPaused` is where it actually threw.
+
+Fixed at two levels, not just the one line:
+
+- The immediate cause: `party.members.get(userId)!` in the `"playstate"`
+  handler is now a real `if (!member) return` guard, matching the
+  pattern every *other* handler already used for the same lookup
+  (`voteSkip`/`musicPlaystate`/`requestPause`) - this one was just missed.
+- The systemic gap: nothing in `SocketGateway` had ever needed the
+  equivalent of `index.ts`'s `asyncRoute()` before, because nothing else
+  here had thrown - but Socket.IO has no built-in equivalent of Express's
+  error-handling middleware at all. A synchronous throw from *any*
+  listener propagates straight out of `Socket.emit()` and crashes the
+  whole process, unconditionally - not just this bug, any future one,
+  in any handler. Every `socket.on(...)` registration in `attach()` now
+  goes through a new `safe()` wrapper (log-and-swallow on an unexpected
+  exception) - the same "don't let one bad case take down every active
+  party" instinct `asyncRoute()` already applied to REST routes, just
+  never extended to the realtime side until this forced the question.
+
+Verified with a real regression test reproducing the exact sequence
+(two joins for one userId, the occupying one leaving as the party's last
+member, the other's next heartbeat) - confirmed it doesn't throw, and
+specifically confirmed it's the *guard* doing that (a clean early return)
+rather than the generic `safe()` net merely catching and hiding an actual
+throw, by temporarily reverting just the guard and watching that same
+test correctly fail before putting it back.
+
+Deliberately not "fixed" by changing how `join`/`leave` share that map
+slot in this same pass - that's the real design work the fuller device-
+detection feature above still needs (identifying a device, deciding what
+"same account, different device" should actually do instead of silently
+overwriting). This pass stops it from ever being able to crash the
+server again in the meantime, regardless of how that gets resolved.
+
 ## Known limitations
 
 - Playback always goes through `hls.js` against whatever Emby's

server/src/realtime/SocketGateway.ts

@@ -63,12 +63,12 @@ class SocketGatewayClass {
       const userId = session.userId;
       let party: Party | null = null;
 
-      socket.on('subscribeLobby', () => {
+      socket.on('subscribeLobby', this.safe(() => {
         socket.join('lobby');
         socket.emit('parties', this.lobbySnapshot());
-      });
+      }));
 
-      socket.on('join', (msg) => {
+      socket.on('join', this.safe((msg: { partyId: string }) => {
         const p = PartyService.getById(msg?.partyId);
         const member = p?.members.get(userId);
         if (!p || !member) {
@@ -80,13 +80,24 @@ class SocketGatewayClass {
         member.connection = socket;
         socket.join(party.id);
         this.broadcastMembers(io, party);
-      });
+      }));
 
-      socket.on('playstate', (msg) => {
+      socket.on('playstate', this.safe((msg: { isPaused: boolean; positionTicks: number }) => {
         if (!party || userId !== party.hostUserId) {
           return; // only the host drives playback - see docs/architecture.md
         }
-        const member = party.members.get(userId)!;
+        // Not just defensive dead code: confirmed as a real crash. Two
+        // connections for the same userId (two tabs/devices, no per-device
+        // identity yet - see docs/architecture.md) share the one
+        // userId-keyed map slot; if the connection currently occupying it
+        // is the last member and leaves, the party is destroyed and this
+        // slot is gone, but party.hostUserId is never reassigned (there's
+        // no successor to hand off to) - so the OTHER, still-connected
+        // socket's next 3s heartbeat still passes the host check above
+        // (its stale userId still equals the stale hostUserId) and used
+        // to crash the whole process on the `!` right here.
+        const member = party.members.get(userId);
+        if (!member) return;
         const wasPaused = member.isPaused;
         member.isPaused = !!msg?.isPaused;
         member.positionTicks = Number(msg?.positionTicks) || 0;
@@ -108,9 +119,9 @@ class SocketGatewayClass {
         }
 
         socket.to(party.id).emit('playstate', { isPaused: member.isPaused, positionTicks: member.positionTicks });
-      });
+      }));
 
-      socket.on('next', () => {
+      socket.on('next', this.safe(() => {
         if (!party || userId !== party.hostUserId) {
           return; // only the host can advance the queue
         }
@@ -130,23 +141,23 @@ class SocketGatewayClass {
           // join/leave) happens to refresh it next.
           this.broadcastLobby(io);
         }
-      });
+      }));
 
       // Majority of currently CONNECTED members, not everyone who's ever
       // joined - see MusicQueueService.
-      socket.on('voteSkip', () => {
+      socket.on('voteSkip', this.safe(() => {
         const member = party?.mediaType === 'music' ? party.members.get(userId) : undefined;
         if (!party || !member) return;
         const connectedCount = Array.from(party.members.values()).filter((m) => m.connection).length;
         MusicQueueService.voteSkip(party, userId, member.userName, connectedCount);
         this.broadcastLobby(io);
-      });
+      }));
 
       // No host check, unlike video's "playstate" - see MusicQueueService.
-      socket.on('musicPlaystate', (msg) => {
+      socket.on('musicPlaystate', this.safe((msg: { isPaused: boolean }) => {
         if (!party || party.mediaType !== 'music' || !party.members.get(userId)) return;
         MusicQueueService.setPaused(party, !!msg?.isPaused);
-      });
+      }));
 
       // Anyone (not just followers - harmless either way) can ask the host
       // to pause; the presence overlay (the video player) is what triggers
@@ -156,7 +167,7 @@ class SocketGatewayClass {
       // extra click - or anyone poking the socket directly - would
       // bypass). Video only: a music party has no host to ask - everyone
       // already has direct pause access via "musicPlaystate".
-      socket.on('requestPause', () => {
+      socket.on('requestPause', this.safe(() => {
         if (!party || party.mediaType === 'music') return;
         const member = party.members.get(userId);
         if (!member) return;
@@ -165,24 +176,47 @@ class SocketGatewayClass {
         member.lastPauseRequestUtc = now;
         Logger.party(`pauseRequested partyId=${party.id} user=${Logger.who(userId, member.userName)}`);
         io.to(party.id).emit('pauseRequested', { userId, userName: member.userName });
-      });
+      }));
 
-      socket.on('leave', () => {
+      socket.on('leave', this.safe(() => {
         if (!party) return;
         this.applyLeave(io, party, userId);
         party = null;
-      });
+      }));
 
       // Fires on a clean "leave", a dead-connection timeout (Socket.IO's
       // own heartbeat - see docs/architecture.md), or any other way the
       // socket goes away - the one cleanup path that always runs, unlike
       // relying on the client to always get a chance to emit "leave" first.
-      socket.on('disconnect', () => {
+      socket.on('disconnect', this.safe(() => {
         if (party) this.applyLeave(io, party, userId);
-      });
+      }));
     });
   }
 
+  /**
+   * Wraps a socket event handler so an unexpected exception logs and gets
+   * swallowed instead of crashing the whole process. Confirmed as a real
+   * failure mode, not theoretical: Socket.IO's own internals have no
+   * equivalent of Express's error-handling middleware (which asyncRoute()
+   * in index.ts already routes every REST handler's rejection/throw
+   * through) - a synchronous throw from a listener here propagates
+   * straight out of `Socket.emit()` and takes the entire Node process
+   * down with it, every active party, not just the one connection at
+   * fault. See docs/architecture.md for the specific bug this was added
+   * after (a stale connection's next heartbeat reading a since-deleted
+   * party.members entry).
+   */
+  private safe<A extends unknown[]>(handler: (...args: A) => void): (...args: A) => void {
+    return (...args: A) => {
+      try {
+        handler(...args);
+      } catch (err) {
+        console.error('[watchparty] socket handler crashed', err);
+      }
+    };
+  }
+
   /**
    * Removes `userId` from `party` and handles everything that follows from
    * it: if they were the host, the new host (if any) gets a personal

server/test/socketGateway.test.ts

@@ -239,4 +239,41 @@ describe('SocketGateway', () => {
     socket.trigger('next');
     assert.equal(lobbyRefreshCount(), 2, 'advancing the queue resets position/pause for the new item - the home page needs that immediately, not just whenever some other event next refreshes it');
   });
+
+  it('a stale connection\'s next heartbeat after the map slot it shared gets deleted does not crash the process (regression - see docs/architecture.md)', (t) => {
+    const crashed: unknown[] = [];
+    t.mock.method(console, 'error', (..._args: unknown[]) => { crashed.push(_args); });
+
+    const io = fakeIo();
+    attach(io);
+    const party = PartyService.create('u-alice', 'Alice', 'm1', 'Un Film', '/img/m1', null, 'video');
+
+    const socketA = fakeSocket('u-alice');
+    io.trigger('connection', socketA as unknown as AppSocket);
+    socketA.trigger('join', { partyId: party.id });
+
+    // A second join for the same userId (a second device/tab, before any
+    // real per-device identity - see docs/architecture.md) re-runs
+    // PartyService.join, which overwrites the one map slot this userId
+    // occupies - orphaning socketA's own member record from the map even
+    // though socketA itself is still very much "connected".
+    PartyService.join(party, 'u-alice', 'Alice');
+    const socketB = fakeSocket('u-alice');
+    io.trigger('connection', socketB as unknown as AppSocket);
+    socketB.trigger('join', { partyId: party.id });
+
+    // socketB (currently occupying the slot) leaves - the only member, so
+    // the party is destroyed; hostUserId is never reassigned since
+    // there's no successor to hand off to (see PartyService.leave).
+    socketB.trigger('leave');
+    assert.equal(PartyService.getById(party.id), null, 'sanity check: the party really is gone');
+
+    // socketA's regular 3s heartbeat fires next, completely unaware any
+    // of this happened: its own closure still references the same
+    // (now-destroyed) Party object, and its stale userId still equals
+    // the stale hostUserId, so the host check alone doesn't catch this -
+    // party.members.get(userId) is what has to.
+    assert.doesNotThrow(() => socketA.trigger('playstate', { isPaused: true, positionTicks: 0 }));
+    assert.equal(crashed.length, 0, 'should be a clean early return (the specific guard), not the generic safe() net silently swallowing an actual throw');
+  });
 });