diff --git a/docs/architecture.md b/docs/architecture.md index e364d4c..a3051cb 100644 --- a/docs/architecture.md +++ b/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` - 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 diff --git a/server/src/realtime/SocketGateway.ts b/server/src/realtime/SocketGateway.ts index 8e62605..07507a3 100644 --- a/server/src/realtime/SocketGateway.ts +++ b/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(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 diff --git a/server/test/socketGateway.test.ts b/server/test/socketGateway.test.ts index fda4d25..7d1ddcf 100644 --- a/server/test/socketGateway.test.ts +++ b/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'); + }); });