diff --git a/README.md b/README.md index a049e06..1adf8c3 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,10 @@ including why this replaced an earlier Emby-plugin attempt (still in is running, it's already on the list, updating in real time. 3. Pick a movie/episode tile and hit "Lancer une watch party" (or drill into a Series' seasons/episodes first) — you become the host — or hit - "Rejoindre" on any party already listed on the home page. + "Rejoindre" on any party already listed on the home page. Already + connected to that same party from another device? You're asked to + confirm before it gets disconnected — reconnecting from the same + device (a reload, a second tab) never asks. 4. The host's play/pause/seek drives everyone else's player automatically; starting from an episode queues the rest of the series so the host can just hit "Épisode suivant" instead of restarting a party each time. If @@ -199,7 +202,11 @@ Then put it behind your existing reverse proxy like everything else client-side) — see docs/architecture.md - [x] Live "watch parties en cours" list (host, elapsed time, member count), live host handoff, no join codes -- [x] Presence overlay (avatars) + "request pause" in the player +- [x] Presence overlay (avatars) + a request toolbar (pause/resume) in the + player — see docs/architecture.md +- [x] Device detection: reconnecting from the same device is silent, a + genuinely different device asks to confirm before disconnecting the + other one — see docs/architecture.md - [x] Server-side logging of the party lifecycle (create/join/leave/destroy, host handoff, play/pause, pause requests) — see docs/architecture.md - [x] Music: dedicated player, Artist/Album/Track browsing, participative diff --git a/client/src/api/party.ts b/client/src/api/party.ts index b35e4ab..913182a 100644 --- a/client/src/api/party.ts +++ b/client/src/api/party.ts @@ -7,6 +7,7 @@ // // No join codes: joining happens straight from the home page's live list // (connectLobby below) by party id - see docs/architecture.md. +import { getDeviceId } from '../deviceId'; import type { ItemChangedMsg, LobbyPartyDto, MusicPublicState, MusicTrackInput, PartyPublicState, PauseRequestedMsg, ResumeRequestedMsg, SocketMember } from '../types'; async function apiFetch(path: string, options: RequestInit = {}): Promise { @@ -78,6 +79,12 @@ export interface PartyConnectionCallbacks { onPauseRequested?: (msg: PauseRequestedMsg) => void; onResumeRequested?: (msg: ResumeRequestedMsg) => void; onQueueUpdated?: (state: MusicPublicState) => void; + /** This account is already actively connected to this party from a + * DIFFERENT device (see client/src/deviceId.ts) - the socket is still + * connected but inert (not joined to the party) until the caller either + * calls confirmDeviceTakeover() or gives up (e.g. navigates away). See + * SocketGateway's "join" handler. */ + onDeviceConflict?: () => void; onError?: (message: string) => void; onClose?: () => void; } @@ -97,6 +104,10 @@ export interface PartyConnection { voteSkip(): void; /** Music only: no host check server-side, unlike sendPlaystate above. */ setMusicPaused(isPaused: boolean): void; + /** Re-sends "join" with force:true after the user confirms they want to + * disconnect their other device and take over here - call this from + * onDeviceConflict's handler once the user says yes. */ + confirmDeviceTakeover(): void; leave(): void; } @@ -107,14 +118,18 @@ export interface PartyConnection { * alive via closures, never gets released. */ export function connectParty(partyId: string, callbacks: PartyConnectionCallbacks = {}): PartyConnection { - const { onMembers, onPlaystate, onItemChanged, onHostChanged, onPauseRequested, onResumeRequested, onQueueUpdated, onError, onClose } = callbacks; + const { onMembers, onPlaystate, onItemChanged, onHostChanged, onPauseRequested, onResumeRequested, onQueueUpdated, onDeviceConflict, onError, onClose } = callbacks; const socket = io(); + const deviceId = getDeviceId(); // Reconnects (Socket.IO retries automatically) re-fire "connect", so this // also covers rejoining after a transient network drop, not just the - // first connection. + // first connection. force is deliberately NOT set here even on a + // reconnect - a drop-and-retry re-sends the same deviceId as before, so + // it never conflicts with itself; force only ever comes from + // confirmDeviceTakeover() below, after a real "deviceConflict". socket.on('connect', () => { - socket.emit('join', { partyId }); + socket.emit('join', { partyId, deviceId }); }); if (onMembers) socket.on('members', (m) => onMembers(m as SocketMember[])); @@ -124,6 +139,7 @@ export function connectParty(partyId: string, callbacks: PartyConnectionCallback if (onPauseRequested) socket.on('pauseRequested', (m) => onPauseRequested(m as PauseRequestedMsg)); if (onResumeRequested) socket.on('resumeRequested', (m) => onResumeRequested(m as ResumeRequestedMsg)); if (onQueueUpdated) socket.on('queueUpdated', (m) => onQueueUpdated(m as MusicPublicState)); // music parties only - see musicBar/MusicBar.ts + if (onDeviceConflict) socket.on('deviceConflict', () => onDeviceConflict()); if (onError) socket.on('errorMessage', (m) => onError((m as { message: string }).message)); if (onClose) socket.on('disconnect', () => onClose()); @@ -146,6 +162,9 @@ export function connectParty(partyId: string, callbacks: PartyConnectionCallback setMusicPaused(isPaused) { socket.emit('musicPlaystate', { isPaused }); }, + confirmDeviceTakeover() { + socket.emit('join', { partyId, deviceId, force: true }); + }, leave() { socket.emit('leave'); socket.disconnect(); diff --git a/client/src/deviceId.ts b/client/src/deviceId.ts new file mode 100644 index 0000000..b4ef401 --- /dev/null +++ b/client/src/deviceId.ts @@ -0,0 +1,40 @@ +'use strict'; + +/** + * A persistent id for THIS browser/device, used only to tell "same account, + * same device reconnecting" apart from "same account, a genuinely different + * device" - see SocketGateway's "join" handler (server/src/realtime/ + * SocketGateway.ts) and docs/architecture.md. Not an identity/auth + * mechanism - the session cookie already handles who the user is; this is + * purely "which of that user's devices is this". + * + * Persisted in localStorage (not sessionStorage) so it survives a reload + * and stays stable across tabs of the same browser - which is exactly what + * lets two tabs of the same browser count as "the same device" (no + * takeover prompt), while a different browser or a different computer gets + * a fresh id (a real conflict). + */ +const KEY = 'watchparty_device_id'; + +let cached: string | null = null; + +export function getDeviceId(): string { + if (cached) return cached; + try { + let id = localStorage.getItem(KEY); + if (!id) { + id = crypto.randomUUID(); + localStorage.setItem(KEY, id); + } + cached = id; + return id; + } catch { + // Private browsing / storage disabled: fall back to a per-page-load id. + // Every socket "join" from this tab still sends the same value (kept in + // `cached`), so it behaves fine as long as the page stays open - it + // just won't survive a reload, which only means a reload always looks + // like "a different device" to the server's conflict check above. + cached = crypto.randomUUID(); + return cached; + } +} diff --git a/client/src/musicBar/MusicBar.ts b/client/src/musicBar/MusicBar.ts index b923c22..87eab11 100644 --- a/client/src/musicBar/MusicBar.ts +++ b/client/src/musicBar/MusicBar.ts @@ -122,6 +122,16 @@ export function joinMusicParty(partyId: string, party: PartyPublicState): void { conn = connectParty(partyId, { onMembers: renderMembers, onQueueUpdated: applyMusicState, + onDeviceConflict: () => { + // Same take-over prompt as views/player/Player.ts - see its own + // comment. Declining here means leaving the bar entirely (there's no + // "page" to just navigate away from, unlike the video player). + const takeOver = confirm( + "Tu es déjà connecté à cette party depuis un autre appareil.\n\nContinuer ici va déconnecter l'autre appareil." + ); + if (takeOver) conn?.confirmDeviceTakeover(); + else leaveMusicParty(); + }, onError: (message) => alert(message), onClose: () => {} }); diff --git a/client/src/views/player/Player.ts b/client/src/views/player/Player.ts index 71da66d..071f7eb 100644 --- a/client/src/views/player/Player.ts +++ b/client/src/views/player/Player.ts @@ -456,6 +456,18 @@ function renderVideoPlayer(root: HTMLElement, partyId: string, party: PartyPubli onHostChanged: () => becomeHost(true), onPauseRequested, onResumeRequested, + onDeviceConflict: () => { + // This socket is still connected but inert (see SocketGateway's + // "join" handler / api/party.ts) - a confirm() is fine here even + // though it blocks: this only ever fires right after opening the + // party, before any real playback state has built up locally to + // interrupt. + const takeOver = confirm( + "Tu es déjà connecté à cette party depuis un autre appareil.\n\nContinuer ici va déconnecter l'autre appareil." + ); + if (takeOver) conn?.confirmDeviceTakeover(); + else navigate('/home'); + }, onError: (message) => alert(message), onClose: () => {} }); diff --git a/docs/architecture.md b/docs/architecture.md index 2116ab9..fa62fb9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1534,22 +1534,124 @@ 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). +## Device detection: telling a second tab apart from a second device + +Requested directly, alongside the crash below (same underlying gap - see +its own comment on the connection between the two): "same account, same +device already in the party" should never offer "Rejoindre" friction at +all, and "same account, a genuinely *different* device" should ask before +disconnecting it, not silently steal the slot. `party.members` is +`Map` - one entry per Emby account, no concept of +*which physical device* holds it, so before this there was no way to even +ask the question "is this the same device or not." + +- **Identity**: `client/src/deviceId.ts` - a UUID (`crypto.randomUUID()`) + generated once and kept in `localStorage`, not `sessionStorage`. That + choice is what makes "device" mean "browser," not "tab": two tabs of the + same browser share one `localStorage`, so they share one deviceId and + never conflict with each other, while a different browser or a + different computer always gets a fresh one - a real conflict. A + save/read failure (private browsing, storage disabled) falls back to a + per-page-load id, kept only in memory - correct behavior degrades to + "every reload looks like a new device," not a crash. +- **Protocol**: `"join"` now carries `{partyId, deviceId, force?}` instead + of just `{partyId}` (`ClientToServerEvents`/`api/party.ts`), and + `PartyMember` gained `deviceId: string | null` (null until a real socket + "join" ever succeeds - a member the REST join/create routes just + created has no live connection yet, so nothing to compare against). The + `"join"` handler in `SocketGateway` now reasons about three cases, not + one: + - No existing connection, or the same `deviceId` reconnecting (a reload, + a second tab, a dropped connection retrying) → attach silently, and if + an old connection for that same device is still technically open, + boot it first - it's the same physical device asking again, nothing to + confirm. + - A *different* `deviceId`, `force` not set → emit `"deviceConflict"` to + the *new* socket only (never broadcast) and stop there. The socket + stays connected but inert - not joined to the party room, the member's + `connection` untouched - until the client either re-sends `"join"` + with `force:true` or gives up. + - A different `deviceId` with `force:true` (the client re-sending after + the user confirms) → boot the old connection + (`errorMessage` + `disconnect(true)`) and take the slot, exactly like + the reconnect case above. +- **Client UX**: the answer to "what should happen" was asked directly - + a plain `confirm()` in both `Player.ts` and `MusicBar.ts`'s + `onDeviceConflict` callback ("...continuer ici va déconnecter l'autre + appareil"), calling the new `PartyConnection.confirmDeviceTakeover()` + (re-emits `"join"` with `force:true`) on yes, or backing out (`navigate + ('/home')` for video, `leaveMusicParty()` for music - the bar has no + page to just walk away from) on no. Kept as a native blocking dialog + deliberately: this only ever fires once, right after opening a party, + before there's any local playback state a modal could awkwardly + interrupt. +- **A second, independent bug closed by the same pass**: `PartyService + .join()` used to unconditionally build a fresh member on *every* call, + even for a userId already present - so the REST "Rejoindre" route (which + calls this every time regardless of whether the caller is already a + member) silently reset `connection`/`isPaused`/`deviceId` on a re-click, + a page reload, or a retried request. `join()` is now idempotent: an + existing member is returned as-is. This isn't just tidiness - it's the + other half of the crash below's root cause (two connections silently + sharing one map slot), fixed independently of the guard that stops the + crash itself. +- **Generalizing the crash's own fix**: `"leave"`/`"disconnect"` used to + act on `party` (the connection's own closure variable) alone. With a + device takeover now a real possibility, the *superseded* connection's + own eventual `"disconnect"` (Socket.IO fires this automatically right + after the server calls `.disconnect(true)` on it) would otherwise still + be read as a real leave - ending the *new* connection's membership, + wrong host handoff, a party destroyed under a still-present member. Both + handlers now also check `party.members.get(userId)?.connection === + socket` before acting - only the connection actually holding the slot + can end it. +- **What this deliberately does *not* do**: change what the home page's + live party list shows *before* you click "Rejoindre" - it still can't + tell "you're already in this one, same device" from "you're already in + this one, a different device" from "you're not in it at all," because + `LobbyPartyDto`'s one shared snapshot (`SocketGateway.lobbySnapshot`, + broadcast identically to the whole `"lobby"` room) carries no per-viewer + membership at all - giving it that would mean computing and sending a + personalized snapshot per subscriber instead of one shared broadcast, a + real protocol change of its own, not asked for here. In practice this + doesn't cost the feature much: for **video**, the only way to already be + "in" a party while looking at the home page at all is a second + tab/device (the player is a full-screen takeover on the one you're + using - see "Music bar polish" above for why music's simpler, same-tab + case could just be hidden client-side and video's can't), so the + conflict check above already *is* the meaningful moment - clicking + "Rejoindre" reaches it immediately, same-device reconnects silently, + a different device asks first. The outcome asked for (no friction for + your own device, a confirmation before disconnecting another) is there; + it just resolves one click later than a home-page button label would, + rather than needing a second, heavier protocol change to preempt it. + +Verified with new `SocketGateway`/`PartyService` tests exercising the +real conflict/takeover/reconnect paths (a different device gets +`deviceConflict` and the existing connection is left untouched; `force: +true` boots the old connection and the new one takes the slot, and its +superseded predecessor's own later `"disconnect"` no longer destroys the +party; the same `deviceId` reconnecting never conflicts with itself), plus +a dedicated `PartyService.join()` idempotency test, and confirmed +`deviceId.ts`'s `localStorage` persistence survives a real page reload +against the compiled bundle in a real browser. + ## 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. +one connection. At the time this was found, `party.members` was a +`Map` with no per-device identity at all - the same +gap the "Device detection" section right above eventually closed, once +that was actually asked for; this section describes the state of things +before that, and the immediate fix that had to stand on its own regardless +of whether a fuller fix ever followed. 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 @@ -1594,11 +1696,12 @@ 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. +slot in this same pass - that real design work (identifying a device, +deciding what "same account, different device" should actually do +instead of silently overwriting) landed separately, right above, once +it had actually been asked for. This pass alone was enough to stop it +from ever being able to crash the server again in the meantime, +regardless of how - or whether - that got resolved. ## Known limitations diff --git a/server/src/realtime/SocketGateway.ts b/server/src/realtime/SocketGateway.ts index dca1cdd..b4d0333 100644 --- a/server/src/realtime/SocketGateway.ts +++ b/server/src/realtime/SocketGateway.ts @@ -17,7 +17,18 @@ import type { AppServer, LeaveResult, LobbyPartyDto, Party, SocketMemberDto } fr * "subscribeLobby" joins the "lobby" room * "parties" (server -> client) full snapshot, sent on subscribe * and again whenever any party changes - * "join" {partyId} joins that party's own room + * "join" {partyId, deviceId, force?} joins that party's own room - + * deviceId identifies which physical + * device/browser this is (see + * client/src/deviceId.ts); force:true + * is a confirmed takeover, re-sent + * after "deviceConflict" below + * "deviceConflict" (server -> client, to the joining socket only) + * this account is already connected + * from a *different* deviceId - the + * socket stays connected but inert + * until the client re-sends "join" + * with force:true or gives up * "members" (server -> client) that party's member list * * -- mediaType: 'video' parties -- @@ -73,7 +84,7 @@ class SocketGatewayClass { socket.emit('parties', this.lobbySnapshot()); })); - socket.on('join', this.safe((msg: { partyId: string }) => { + socket.on('join', this.safe((msg: { partyId: string; deviceId: string; force?: boolean }) => { const p = PartyService.getById(msg?.partyId); const member = p?.members.get(userId); if (!p || !member) { @@ -81,8 +92,32 @@ class SocketGatewayClass { socket.disconnect(true); return; } + const deviceId = msg?.deviceId || null; + // Same account, a DIFFERENT device already actively connected as + // this member - ask the new socket to confirm a takeover instead of + // silently booting the other device. member.deviceId is null until + // the first real socket "join" ever succeeds (a member just created + // by the REST join/create routes has no live connection yet), so + // there's nothing to conflict with the very first time. + if (member.connection && member.deviceId && deviceId && member.deviceId !== deviceId && !msg?.force) { + socket.emit('deviceConflict', { partyId: p.id }); + // This socket stays connected but inert: not joined to the party + // room, member.connection untouched - until the client re-sends + // "join" with force:true or gives up. + return; + } + // Either a confirmed takeover (force:true) or the same device + // reconnecting (reload, dropped connection) - boot whatever + // connection currently holds the slot before taking its place, so + // ITS eventual "disconnect" can't be mistaken for a real leave (see + // the connection===socket guard on "leave"/"disconnect" below). + if (member.connection && member.connection !== socket) { + member.connection.emit('errorMessage', { message: 'Tu as rejoint depuis un autre appareil.' }); + member.connection.disconnect(true); + } party = p; member.connection = socket; + member.deviceId = deviceId; socket.join(party.id); this.broadcastMembers(io, party); })); @@ -198,17 +233,29 @@ class SocketGatewayClass { })); socket.on('leave', this.safe(() => { - if (!party) return; - this.applyLeave(io, party, userId); + // Only actually leave if THIS socket is still the connection the + // member map has on file. A device takeover (see "join" above) + // reassigns member.connection to the new socket and force- + // disconnects the old one; without this guard the old socket's own + // "leave"/"disconnect" would incorrectly end the NEW connection's + // membership (wrong host handoff, party destroyed under a still- + // present member, etc). + if (party && party.members.get(userId)?.connection === socket) { + 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 + // own heartbeat - see docs/architecture.md), a device takeover's + // forced disconnect of the OLD connection, 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. + // Same stale-connection guard as "leave" above. socket.on('disconnect', this.safe(() => { - if (party) this.applyLeave(io, party, userId); + if (party && party.members.get(userId)?.connection === socket) { + this.applyLeave(io, party, userId); + } })); }); } diff --git a/server/src/services/PartyService.ts b/server/src/services/PartyService.ts index 319960d..39f1bd9 100644 --- a/server/src/services/PartyService.ts +++ b/server/src/services/PartyService.ts @@ -71,7 +71,23 @@ class PartyServiceClass { return this.partiesById.get(partyId) || null; } + /** + * Idempotent for a userId already in `party`: returns the *existing* + * member as-is rather than resetting it. This matters because the REST + * "Rejoindre" flow calls this every time regardless of whether the + * caller is already a member - re-clicking it, a page reload, a second + * device/tab, a stale retry. Blindly overwriting used to reset + * connection/isPaused/deviceId on every one of those, which is exactly + * what let two connections for one userId silently share (and lose + * track of) one map slot - see docs/architecture.md's server-crash + * section. A genuinely new member still goes through here fresh; only + * the "already present" case is now a no-op. Device-aware conflict + * handling for an *already-connected* member lives in SocketGateway's + * "join" handler instead - this method has no socket to reason about. + */ join(party: Party, userId: string, userName: string): PartyMember { + const existing = party.members.get(userId); + if (existing) return existing; const member = this.newMember(userId, userName); party.members.set(userId, member); party.lastActivityUtc = Date.now(); @@ -201,6 +217,7 @@ class PartyServiceClass { userId, userName, connection: null, + deviceId: null, isPaused: true, positionTicks: 0, lastUpdatedUtc: Date.now() diff --git a/server/src/types.ts b/server/src/types.ts index 815f715..40c22d9 100644 --- a/server/src/types.ts +++ b/server/src/types.ts @@ -21,7 +21,12 @@ import type { Server as IoServer, Socket as IoSocket } from 'socket.io'; export interface ClientToServerEvents { subscribeLobby: () => void; - join: (msg: { partyId: string }) => void; + /** deviceId: persisted client-side (see client/src/deviceId.ts) - lets + * the "join" handler tell "this same account, same device reconnecting" + * apart from "this same account, a genuinely different device" (see + * SocketGateway's own comment). force: true re-sends this after the + * user confirmed a "deviceConflict" takeover. */ + join: (msg: { partyId: string; deviceId: string; force?: boolean }) => void; playstate: (msg: { isPaused: boolean; positionTicks: number }) => void; next: () => void; voteSkip: () => void; @@ -58,6 +63,12 @@ export interface ServerToClientEvents { hostChanged: () => void; pauseRequested: (msg: { userId: string; userName: string }) => void; resumeRequested: (msg: { userId: string; userName: string }) => void; + /** Sent only to the joining socket itself (never broadcast) when this + * account is already actively connected from a different deviceId - + * see SocketGateway's "join" handler. The socket stays connected but + * inert (not attached to the party) until the client either re-sends + * "join" with force:true or gives up. */ + deviceConflict: (msg: { partyId: string }) => void; errorMessage: (msg: { message: string }) => void; } @@ -170,6 +181,11 @@ export interface PartyMember { userId: string; userName: string; connection: AppSocket | null; + /** Whichever deviceId last successfully attached `connection` - null + * until the first real socket "join" (a member freshly created by the + * REST join/create routes has no live connection yet, so nothing to + * compare a new one against). See SocketGateway's "join" handler. */ + deviceId: string | null; isPaused: boolean; positionTicks: number; lastUpdatedUtc: number; diff --git a/server/test/partyService.test.ts b/server/test/partyService.test.ts index ed5bf11..0b1d285 100644 --- a/server/test/partyService.test.ts +++ b/server/test/partyService.test.ts @@ -52,6 +52,25 @@ describe('PartyService', () => { assert.equal(PartyService.getById(party.id), null); }); + it('join() is idempotent: re-joining an existing member returns it as-is instead of resetting it', () => { + const party = PartyService.create('u-alice', 'Alice', 'm1', 'Un Film', '/img/m1', null, 'video'); + const bob = PartyService.join(party, 'u-bob', 'Bob'); + bob.isPaused = false; + bob.positionTicks = 12345; + bob.deviceId = 'device-1'; + + // The REST "Rejoindre" route (and a second device/tab's socket "join") + // call this every time regardless of whether the caller is already a + // member - this must not reset any of the live state just set above + // (see this method's own comment for the crash that used to cause). + const bobAgain = PartyService.join(party, 'u-bob', 'Bob'); + assert.equal(bobAgain, bob, 'must be the exact same member object, not a fresh one'); + assert.equal(bobAgain.isPaused, false, 'a re-join must not reset live state'); + assert.equal(bobAgain.positionTicks, 12345); + assert.equal(bobAgain.deviceId, 'device-1'); + assert.equal(party.members.size, 2, 'still exactly one entry for bob, not a duplicate'); + }); + it('removeIdleParties sweeps a party whose lastActivityUtc is past the cutoff', () => { const party = PartyService.create('u-dave', 'Dave', 'm1', 'Un Film', '/img/m1', null, 'video'); party.lastActivityUtc = Date.now() - 5 * 60 * 60 * 1000; diff --git a/server/test/socketGateway.test.ts b/server/test/socketGateway.test.ts index 8f78222..9f08975 100644 --- a/server/test/socketGateway.test.ts +++ b/server/test/socketGateway.test.ts @@ -259,7 +259,7 @@ describe('SocketGateway', () => { 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) => { + it('a stale connection\'s next heartbeat after a device takeover destroys the party does not crash the process (regression - see docs/architecture.md)', (t) => { const crashed: unknown[] = []; t.mock.method(console, 'error', (..._args: unknown[]) => { crashed.push(_args); }); @@ -269,17 +269,18 @@ describe('SocketGateway', () => { const socketA = fakeSocket('u-alice'); io.trigger('connection', socketA as unknown as AppSocket); - socketA.trigger('join', { partyId: party.id }); + socketA.trigger('join', { partyId: party.id, deviceId: 'device-A' }); - // 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'); + // A confirmed device takeover (see the "join" handler below) hands + // socketA's map slot to socketB - the exact "two connections for the + // same userId share one slot" shape the original crash needed, now + // reached through the real takeover flow instead of a raw + // PartyService.join() call (that method is idempotent now - see its + // own test in partyService.test.ts - so a second call to it would no + // longer reproduce this). const socketB = fakeSocket('u-alice'); io.trigger('connection', socketB as unknown as AppSocket); - socketB.trigger('join', { partyId: party.id }); + socketB.trigger('join', { partyId: party.id, deviceId: 'device-B', force: true }); // socketB (currently occupying the slot) leaves - the only member, so // the party is destroyed; hostUserId is never reassigned since @@ -295,4 +296,73 @@ describe('SocketGateway', () => { 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'); }); + + it('a second device for the same account gets a deviceConflict instead of silently taking over', () => { + 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, deviceId: 'device-A' }); + + const socketB = fakeSocket('u-alice'); + io.trigger('connection', socketB as unknown as AppSocket); + const roomEmitsBefore = io.roomEmits.length; + socketB.trigger('join', { partyId: party.id, deviceId: 'device-B' }); + + assert.ok(socketB.emitted.some(([event]) => event === 'deviceConflict'), 'the new device should be told about the conflict'); + assert.equal(party.members.get('u-alice')?.connection, socketA, 'the existing connection must stay in place until the conflict is confirmed'); + assert.equal(io.roomEmits.length, roomEmitsBefore, 'a conflicting join must not broadcast "members" - the socket never actually joined'); + assert.ok(!socketA.emitted.some(([event]) => event === 'errorMessage'), 'the existing device must not be kicked yet'); + }); + + it('re-joining with force:true confirms the takeover: kicks the old device, attaches the new one', () => { + 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, deviceId: 'device-A' }); + + const socketB = fakeSocket('u-alice'); + io.trigger('connection', socketB as unknown as AppSocket); + socketB.trigger('join', { partyId: party.id, deviceId: 'device-B' }); // conflict - the client would show a confirm() here + socketB.trigger('join', { partyId: party.id, deviceId: 'device-B', force: true }); // user confirmed + + assert.ok(socketA.emitted.some(([event]) => event === 'errorMessage'), 'the old device should be told it got disconnected'); + assert.equal(party.members.get('u-alice')?.connection, socketB, 'the new device now holds the slot'); + assert.equal(party.members.get('u-alice')?.deviceId, 'device-B'); + + // The old connection's own eventual "disconnect" - Socket.IO fires this + // automatically after a server-side .disconnect(true), simulated here + // manually like this file's other tests - must NOT be mistaken for a + // real leave: it no longer holds the member's connection slot (see the + // "join" handler's own comment on why, and the guard on "leave"/ + // "disconnect" below). + socketA.trigger('disconnect'); + assert.equal(PartyService.getById(party.id), party, 'the party must survive the superseded connection\'s disconnect'); + assert.ok(party.members.has('u-alice'), 'the member must still be present, still attached to socketB'); + }); + + it('the same device reconnecting (matching deviceId) takes over silently - no deviceConflict', () => { + 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, deviceId: 'device-A' }); + + // e.g. a page reload on the same browser: a fresh socket, but the same + // deviceId persisted in localStorage (see client/src/deviceId.ts). + const socketA2 = fakeSocket('u-alice'); + io.trigger('connection', socketA2 as unknown as AppSocket); + socketA2.trigger('join', { partyId: party.id, deviceId: 'device-A' }); + + assert.ok(!socketA2.emitted.some(([event]) => event === 'deviceConflict'), 'the same device should never conflict with itself'); + assert.equal(party.members.get('u-alice')?.connection, socketA2); + assert.equal(party.members.get('u-alice')?.deviceId, 'device-A'); + }); });