From 3ae1fce806deb4e3da7cf40d24e5518553f65c57 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Fri, 18 Sep 2026 10:23:22 +0200 Subject: [PATCH] Fix device takeover leaving both devices on the session with no working controls Two gaps around an already-correct takeover mechanism, not the mechanism itself. First: Player.ts/MusicBar.ts both wired onClose to a no-op, so the moment a takeover force-disconnected the old device its video/hls.js instance (or the persistent music bar) just kept running forever on a dead connection - both devices looked fully active, since nothing ever told the old one to stop or navigate away. Second: the obvious fix (call the same leave path Quitter uses) ran into a worse bug - POST /leave removes a member by userId alone with no staleness guard, so the old devices own leave call would have ended the NEW devices membership too. Fixed both: onClose now tears down local playback and leaves whenever this routes own explicit teardown has not already run, and the leave route (plus the client, which now sends its own deviceId) gained the same staleness guard the socket handlers already had, via a new PartyService.isStaleDevice. Co-Authored-By: Claude Sonnet 5 --- README.md | 4 ++ client/src/api/party.ts | 8 +++- client/src/musicBar/MusicBar.ts | 19 ++++++++- client/src/views/player/Player.ts | 27 ++++++++++++- docs/architecture.md | 63 +++++++++++++++++++++++++++++ server/src/index.ts | 13 ++++-- server/src/services/PartyService.ts | 24 +++++++++++ server/test/partyService.test.ts | 35 ++++++++++++++++ 8 files changed, 187 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index f6fbb57..20cb971 100644 --- a/README.md +++ b/README.md @@ -395,6 +395,10 @@ Then put it behind your existing reverse proxy like everything else - [x] Linked whatever's playing (video or music) to the OS's own media controls (Windows' SMTC, macOS' Now Playing, ...) — title/artwork, play/pause/seek/next — see docs/architecture.md +- [x] Fixed switching a party between devices leaving both "on the + session" with no working controls — the old device never reacted + to being kicked, and the leave route had no way to tell a stale + device apart from the current one — see docs/architecture.md - [ ] Try a full party end-to-end with two real accounts (still the main gap) - [ ] Try casting to a real Chromecast (only the SDK's own bootstrap is confirmed here — no real device/session) diff --git a/client/src/api/party.ts b/client/src/api/party.ts index 86436a1..181cf67 100644 --- a/client/src/api/party.ts +++ b/client/src/api/party.ts @@ -39,8 +39,14 @@ export function joinParty(partyId: string): Promise { return apiFetch(`/api/parties/${encodeURIComponent(partyId)}/join`, { method: 'POST' }); } +/** Sends this device's own deviceId so the server can tell a stale + * device (already replaced by a takeover on another one - see + * getDeviceId's own comment) apart from the currently-active one, the + * same way the socket "join"/"leave"/"disconnect" handlers already do + * via the live connection itself - a REST call has no connection to + * compare, so this is what lets the server apply that same guard here. */ export function leaveParty(partyId: string): Promise<{ ok: true }> { - return apiFetch(`/api/parties/${partyId}/leave`, { method: 'POST' }); + return apiFetch(`/api/parties/${partyId}/leave`, { method: 'POST', body: JSON.stringify({ deviceId: getDeviceId() }) }); } export function getParty(partyId: string): Promise { diff --git a/client/src/musicBar/MusicBar.ts b/client/src/musicBar/MusicBar.ts index bf228e2..14bc293 100644 --- a/client/src/musicBar/MusicBar.ts +++ b/client/src/musicBar/MusicBar.ts @@ -266,7 +266,24 @@ export function joinMusicParty(partyId: string, party: PartyPublicState): void { else leaveMusicParty(); }, onError: (message) => showToast(message, { variant: 'error' }), - onClose: () => {} + // Fires for every disconnect, intentional or not - leaveMusicParty + // itself already clears activePartyId synchronously, well before its + // own conn.leave() actually closes the socket, so seeing it still set + // here means this device's connection died some other way (most + // often: replaced by a takeover on another device - see + // SocketGateway's "join" handler). Without this, the bar just sat + // there looking/sounding fully live forever on the old device, on a + // conn that was actually already dead underneath it - reported + // directly as "both devices end up on the session, controls never + // transfer." leaveMusicParty() is safe to call here even though the + // party already moved on: leaveParty's own deviceId check (see + // api/party.ts) makes its REST call a no-op for a stale device + // instead of ending the CURRENT device's membership by mistake. + onClose: () => { + if (!activePartyId) return; + showToast('Connexion à la party interrompue.', { variant: 'error' }); + leaveMusicParty(); + } }); } diff --git a/client/src/views/player/Player.ts b/client/src/views/player/Player.ts index a380a01..2b24964 100644 --- a/client/src/views/player/Player.ts +++ b/client/src/views/player/Player.ts @@ -229,6 +229,14 @@ function renderVideoPlayer(root: HTMLElement, partyId: string, party: PartyPubli let isHost = false; let hasNextQueued = false; + // Set at the top of cleanup() below, which every intentional way of + // leaving this route (the "Quitter" button, a sidebar link, browser + // back, ...) already runs synchronously before the socket actually + // closes - so by the time the async "disconnect" this connection's + // own onClose reacts to actually arrives, this is already true for + // every one of those. Still false there only means an unprompted + // close (most often: a device takeover elsewhere - see onClose below). + let leftIntentionally = false; let conn: PartyConnection | null = null; let hls: HlsInstance | null = null; let currentItemId: string | null = null; @@ -1277,7 +1285,23 @@ function renderVideoPlayer(root: HTMLElement, partyId: string, party: PartyPubli else navigate('/home'); }, onError: (message) => showToast(message, { variant: 'error' }), - onClose: () => {} + // Fires for every disconnect, intentional or not - leftIntentionally + // is already true by the time this runs for any of those (see its + // own comment), so still false here means an unprompted close, most + // often a device takeover elsewhere (SocketGateway's "join" handler) + // - without this, the video/hls.js instance just kept running + // forever on the old device, on a conn that was actually already + // dead underneath it, reported directly as "both devices end up on + // the session, controls never transfer." navigate() runs this same + // route's cleanup() below, which is now safe to do even though the + // party already moved on - leaveParty's own deviceId check (see + // api/party.ts) makes its REST call a no-op for a stale device + // instead of ending the CURRENT device's membership by mistake. + onClose: () => { + if (leftIntentionally) return; + showToast('Connexion à la party interrompue.', { variant: 'error' }); + navigate('/home'); + } }); } @@ -1291,6 +1315,7 @@ function renderVideoPlayer(root: HTMLElement, partyId: string, party: PartyPubli // other way (sidebar link, browser back, ...). This is the fix for the // actual leak: previously, only the "Quitter" button ran any of this. return function cleanup() { + leftIntentionally = true; if (heartbeat) clearInterval(heartbeat); if (progressTicker) clearInterval(progressTicker); if (requestBannerTimer) clearTimeout(requestBannerTimer); diff --git a/docs/architecture.md b/docs/architecture.md index e491416..c7fcfd5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -4415,6 +4415,69 @@ call sites already gives), and the actual OS-level popup itself - the whole point - isn't something this sandbox can display or click regardless. Worth a real Windows/Chrome check once deployed. +## Device takeover: both devices "on the session", neither one's controls working + +Reported directly: switching a watch/listen party from one device to +another left both devices looking active, with no controls actually +transferring. The device-takeover mechanism itself - `SocketGateway`'s +"join" handler force-disconnecting the old device's socket once the new +one confirms - was already correct and already guarded against the +obvious failure mode (its own comment on the "leave"/"disconnect" +handlers: a stale connection's own eventual disconnect must not be +mistaken for a real leave). The actual bug was two gaps *around* that, +neither in the takeover mechanism itself. + +**Gap 1 - the old device never reacted to being kicked.** Both +`Player.ts` and `MusicBar.ts` wired `onClose: () => {}` - a complete +no-op - to the connection's "disconnect" event. So the moment a takeover +force-disconnected the old device's socket, that device's own video/ +hls.js instance (or the persistent music bar) just kept running exactly +as before: the socket was dead, but nothing told the local `