Merge pull request 'Fix device takeover leaving both devices on the session with no working controls' (#23) from fix/device-takeover-stale-connection into master

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)

client/src/api/party.ts

@@ -39,8 +39,14 @@ export function joinParty(partyId: string): Promise<PartyPublicState> {
   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<PartyPublicState> {

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();
+    }
   });
 }
 

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);

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 `<video>`/
+`<audio>` to stop, and nothing navigated away or reset `isHost`. From
+the old device's own screen, everything still looked (and for local-only
+playback, sounded) fully live - exactly "both devices on the session."
+Fixed: both now react to an *unprompted* close (one that didn't already
+run this route/module's own explicit teardown - `leftIntentionally` in
+Player.ts, `activePartyId` already being null in MusicBar.ts, both set
+synchronously by that explicit teardown well before the async
+"disconnect" this checks against ever arrives) by tearing down local
+playback and leaving, the same as if the user had clicked "Quitter"
+there themselves.
+
+**Gap 2 - reacting to the kick had no safe way to do that.** The obvious
+fix for gap 1 - call the same leave path "Quitter" already uses - ran
+straight into a second, worse bug: `POST /api/parties/:partyId/leave`
+removes a member by `userId` alone, with no equivalent of the socket
+handlers' own staleness guard (there's no live connection for a REST
+call to compare against). Calling it from the *old*, superseded device
+would have ended the *new* device's membership too - silently undoing
+the takeover it was supposed to be cleaning up after. Fixed by giving
+this route the same guard via `deviceId` instead of a socket reference:
+new `PartyService.isStaleDevice(member, deviceId)`, and `leaveParty()`
+(client) now sends its own `getDeviceId()` for the server to compare.
+Only ever blocks on positive evidence of a mismatch - a member with no
+recorded deviceId yet, or a caller that sent none, still leaves exactly
+as before this existed.
+
+Verified: full `typecheck`/`test`/`build` on both projects (153/153
+server tests - new coverage for `isStaleDevice` covering a matching
+deviceId, a mismatched one, an unset member deviceId, an unset caller
+deviceId, and an unknown member, each checked against the *reason* it
+should or shouldn't block, not just the outcome). The client-side
+onClose fix was checked against the exact reported scenario, not just
+reasoned about: a fake `io()` standing in for Socket.IO (real `.on`/
+`.emit`/a scriptable `.disconnect()` that fires real listeners) let
+`joinMusicParty`/`leaveMusicParty` run for real. Firing `disconnect`
+right after a real `leaveMusicParty()` call produced no toast and left
+the bar hidden (no spurious "connection lost" message on an intentional
+leave); firing it with no prior leave - the takeover-kick shape -
+produced the "Connexion à la party interrompue" toast and correctly
+tore the bar down. Player.ts's own `leftIntentionally` fix mirrors this
+exact shape (confirmed by reading, not re-verified with a second live
+harness - hls.js/a full player DOM would need mocking for no more
+signal than the MusicBar case already gives, given both share the same
+logic shape end to end).
+
 ## Known limitations
 
 - Playback always goes through `hls.js` against whatever Emby's

server/src/index.ts

@@ -615,9 +615,16 @@ app.post('/api/parties/:partyId/join', requireSession, (req, res) => {
 app.post('/api/parties/:partyId/leave', requireSession, (req, res) => {
   const party = PartyService.getById(param(req, 'partyId'));
   if (party) {
-    // Shared with the socket "leave"/"disconnect" handlers so the host-
-    // handoff notification and the lobby refresh only live in one place.
-    SocketGateway.applyLeave(io, party, req.embySession!.userId);
+    const member = party.members.get(req.embySession!.userId);
+    const deviceId = typeof req.body?.deviceId === 'string' ? req.body.deviceId : null;
+    // See PartyService.isStaleDevice's own comment for why this check
+    // exists at all - a REST call has no live connection to guard this
+    // with, unlike the socket "leave"/"disconnect" handlers.
+    if (!PartyService.isStaleDevice(member, deviceId)) {
+      // Shared with the socket "leave"/"disconnect" handlers so the host-
+      // handoff notification and the lobby refresh only live in one place.
+      SocketGateway.applyLeave(io, party, req.embySession!.userId);
+    }
   }
   res.json({ ok: true });
 });

server/src/services/PartyService.ts

@@ -144,6 +144,30 @@ class PartyServiceClass {
     return { itemId: party.itemId, positionTicks: host.positionTicks, isPaused: host.isPaused };
   }
 
+  /**
+   * Whether a request claiming to be `deviceId` is stale for `member` -
+   * already replaced by a takeover on a *different* device (see
+   * SocketGateway's "join" handler). Used by the REST /leave route
+   * (index.ts), which - unlike the socket "leave"/"disconnect" handlers,
+   * guarded by comparing `member.connection` against the live socket
+   * itself - has no connection to compare against instead. Without this,
+   * a device already superseded by a takeover elsewhere could still call
+   * /leave and end the CURRENT device's membership by mistake - confirmed
+   * as the real cause of a "both devices end up on the session, controls
+   * never transfer" report: the stale device's own client didn't react to
+   * being disconnected at all (see Player.ts/MusicBar.ts's onClose), so
+   * its still-rendered "Quitter" button stayed clickable and reached here.
+   *
+   * Only ever returns true on positive evidence of a mismatch (both sides
+   * known and different) - a member with no recorded deviceId yet, or a
+   * caller that didn't send one, is never treated as stale, mirroring the
+   * socket handlers' own "nothing to conflict with yet" reasoning for a
+   * null deviceId.
+   */
+  isStaleDevice(member: PartyMember | undefined, deviceId: string | null): boolean {
+    return !!(member?.deviceId && deviceId && member.deviceId !== deviceId);
+  }
+
   /**
    * Removes a member. If they were the host, promotes whoever's left
    * (arbitrary but deterministic-ish - good enough for v1); if nobody's

server/test/partyService.test.ts

@@ -155,6 +155,41 @@ describe('PartyService', () => {
     assert.equal(called, false);
   });
 
+  it('isStaleDevice(): false when the deviceId matches the member\'s own', () => {
+    const party = PartyService.create('u-alice', 'Alice', 'm1', 'Un Film', '/img/m1', null, 'video');
+    const alice = party.members.get('u-alice')!;
+    alice.deviceId = 'device-a';
+
+    assert.equal(PartyService.isStaleDevice(alice, 'device-a'), false);
+  });
+
+  it('isStaleDevice(): true when the deviceId differs from the member\'s own - the actual takeover case', () => {
+    const party = PartyService.create('u-alice', 'Alice', 'm1', 'Un Film', '/img/m1', null, 'video');
+    const alice = party.members.get('u-alice')!;
+    alice.deviceId = 'device-a'; // device B's takeover already moved this forward
+
+    assert.equal(PartyService.isStaleDevice(alice, 'device-b'), true);
+  });
+
+  it('isStaleDevice(): false when the member has no recorded deviceId yet - nothing to conflict with, same reasoning as the socket "join" handler', () => {
+    const party = PartyService.create('u-alice', 'Alice', 'm1', 'Un Film', '/img/m1', null, 'video');
+    const alice = party.members.get('u-alice')!; // deviceId still null - no socket "join" has succeeded yet
+
+    assert.equal(PartyService.isStaleDevice(alice, 'device-a'), false);
+  });
+
+  it('isStaleDevice(): false when the caller sent no deviceId at all - only ever blocks on positive evidence of a mismatch', () => {
+    const party = PartyService.create('u-alice', 'Alice', 'm1', 'Un Film', '/img/m1', null, 'video');
+    const alice = party.members.get('u-alice')!;
+    alice.deviceId = 'device-a';
+
+    assert.equal(PartyService.isStaleDevice(alice, null), false);
+  });
+
+  it('isStaleDevice(): false for an unknown member (already left some other way)', () => {
+    assert.equal(PartyService.isStaleDevice(undefined, 'device-a'), false);
+  });
+
   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');