From dfa0a3d03efcd307814c2f29bce433202c1d4a28 Mon Sep 17 00:00:00 2001 From: KoalaDev <6156589+Shik3i@users.noreply.github.com> Date: Sun, 28 Jun 2026 02:25:11 +0200 Subject: [PATCH] =?UTF-8?q?fix(host-control-mode):=20audit=20findings=20?= =?UTF-8?q?=E2=80=94=20host=20dedup=20demotion,=20episode-lobby=20freeze,?= =?UTF-8?q?=20+cleanups?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings recovered from the multi-agent audit (verification phase was cut off by a usage limit; verified inline against the code): HIGH: - Host was demoted on peerId dedup (fast reconnect / second tab): the dedup path removes the old socket and the kicked socket's 'disconnect' both ran the host-leave fallback before the same peerId re-joined → room silently unlocked on every host network blip. Now skip the fallback while a join for that peerId is in flight (peerJoinLocks). Regression test added. (A long real disconnect still falls back — that's the deferred host-grace EC-10.) - Episode auto-advance froze a gated guest: in host-only the guest's EPISODE_LOBBY is dropped server-side, but the guest still self-paused (PAUSE_FOR_LOBBY) waiting for readies that never came → 60s freeze. A host-only guest now skips creating a lobby (the host drives episode sync). LOW / cleanup: - GET_HCM_STRINGS could return a raw key name ("HCM_DIALOG_TITLE") if the locale dictionary failed to load (getMessage returns the key on miss) → omit it so content keeps its English fallback. - hcmReset now also clears the snap-back cooldown + buffering grace, so a stale cooldown can't swallow the first snap-back after a room/host change. - Popup play/pause labels reset on lock too (not just unlock), so a button can't freeze on "Playing…" when host-only activates mid-click. - Fix a stale comment (heartbeat now carries 'desynced'). Co-Authored-By: Claude Opus 4.8 --- extension/background.js | 33 ++++++++++++++++++++++++--------- extension/content.js | 2 ++ extension/popup.js | 16 +++++++--------- scripts/test-server-ws.mjs | 19 +++++++++++++++++++ server/index.js | 8 +++++++- 5 files changed, 59 insertions(+), 19 deletions(-) diff --git a/extension/background.js b/extension/background.js index d9fa4fa..1494877 100644 --- a/extension/background.js +++ b/extension/background.js @@ -1236,9 +1236,10 @@ function handleServerEvent(event, data) { peer.mediaTitle = data.mediaTitle !== undefined ? data.mediaTitle : peer.mediaTitle; peer.volume = data.volume !== undefined ? data.volume : peer.volume; peer.muted = data.muted !== undefined ? data.muted : peer.muted; - // Only update when present — the background-driven keepAlive - // heartbeat omits 'desynced', and clobbering it to false there - // would make the host's "Solo" badge flicker between heartbeats. + // Only update when present. Our own heartbeats now carry + // 'desynced', but other PEER_STATUS variants (server join + // broadcast, future/old clients) omit it — and clobbering it + // to false there would flicker the host's "Solo" badge. if (data.desynced !== undefined) peer.desynced = data.desynced === true; const timeSinceReactive = peer.lastReactiveUpdate ? (Date.now() - peer.lastReactiveUpdate) : Infinity; @@ -1746,13 +1747,17 @@ async function handleAsyncMessage(message, sender, sendResponse) { const settings = await chrome.storage.local.get(['locale']); const lang = settings.locale || getSystemLanguage(); await loadLocale(lang); + // getMessage returns the key name itself if the dictionary failed to load. + // Return undefined in that case so content keeps its English fallback rather + // than rendering a raw key like "HCM_DIALOG_TITLE". + const m = (k) => { const v = getMessage(k); return v === k ? undefined : v; }; sendResponse({ - title: getMessage('HCM_DIALOG_TITLE'), - body: getMessage('HCM_DIALOG_BODY'), - stay: getMessage('HCM_DIALOG_STAY'), - solo: getMessage('HCM_DIALOG_SOLO'), - badge: getMessage('HCM_BADGE_SOLO'), - resync: getMessage('HCM_BADGE_RESYNC') + title: m('HCM_DIALOG_TITLE'), + body: m('HCM_DIALOG_BODY'), + stay: m('HCM_DIALOG_STAY'), + solo: m('HCM_DIALOG_SOLO'), + badge: m('HCM_BADGE_SOLO'), + resync: m('HCM_BADGE_RESYNC') }); } else if (message.type === 'HCM_DESYNC_STATE') { // content.js tells us whether the local user chose to watch on their own. @@ -2139,6 +2144,16 @@ async function handleAsyncMessage(message, sender, sendResponse) { return; } + // Host Control Mode: a gated guest must NOT initiate an episode lobby — the + // server drops the guest's EPISODE_LOBBY, so the lobby would never complete + // and the guest would self-pause (PAUSE_FOR_LOBBY) into a 60s freeze. In + // host-only the host drives episode sync; the guest just follows / snaps back. + if (controlMode === CONTROL_MODES.HOST_ONLY && !amHost()) { + addLog(`Episode change ("${newTitle}") — host-only guest, not creating a lobby (host drives).`, 'info'); + sendResponse({ status: 'host_only_guest_skip' }); + return; + } + // Variant A: alone in the room → no one to wait for. Skip the lobby // entirely so the next episode just plays through (no pause, no traffic). // Live peer check, so the moment someone joins the next transition syncs. diff --git a/extension/content.js b/extension/content.js index 733233b..1353950 100644 --- a/extension/content.js +++ b/extension/content.js @@ -390,6 +390,8 @@ const wasDesynced = hcmDesynced; hcmDesynced = false; hcmDeferredSnapPending = false; + hcmSnapBackCooldownUntil = 0; // don't let a stale cooldown swallow the next snap-back + hcmBufferingUntil = 0; hcmRemoveDialog(); hcmRemoveBadge(); // If we were desynced, notify background so it stops reporting us as diff --git a/extension/popup.js b/extension/popup.js index 63da0f4..5c29a92 100644 --- a/extension/popup.js +++ b/extension/popup.js @@ -355,15 +355,13 @@ function setRemoteControlsLocked(locked) { btn.style.cursor = locked ? 'not-allowed' : ''; btn.title = locked ? (getMessage('NOTICE_HOST_CONTROLS') || 'The host controls playback for everyone.') : ''; }); - // When unlocking, also restore the default labels. The action handlers leave - // the text in a transitional state ("Playing..." / "Pausing...") and the 2.5s - // safety reset skips the refresh while we were guest-locked, so without this - // the button can be re-enabled with stale text after the host disables - // host-only (L-1). - if (!locked) { - if (elements.playBtn) elements.playBtn.textContent = getMessage('BTN_PLAY') || 'Play'; - if (elements.pauseBtn) elements.pauseBtn.textContent = getMessage('BTN_PAUSE') || 'Pause'; - } + // Always restore the default labels. The action handlers leave the text in a + // transitional state ("Playing..." / "Pausing...") and the 2.5s safety reset + // skips the refresh while guest-locked — so reset on BOTH transitions: on lock + // (host enabled host-only just as the guest clicked → don't freeze "Playing...") + // and on unlock (L-1). + if (elements.playBtn) elements.playBtn.textContent = getMessage('BTN_PLAY') || 'Play'; + if (elements.pauseBtn) elements.pauseBtn.textContent = getMessage('BTN_PAUSE') || 'Pause'; } if (elements.hostControlToggle) { diff --git a/scripts/test-server-ws.mjs b/scripts/test-server-ws.mjs index 6f0fd4a..cdf4377 100644 --- a/scripts/test-server-ws.mjs +++ b/scripts/test-server-ws.mjs @@ -151,6 +151,25 @@ try { assert.ok(dSenderResync && dSenderResync.controlMode==='host-only', 'debounced toggle re-syncs sender to actual state'); close(); + resetConnectionRate(); + + // --- Host role survives peerId dedup (reconnect / second tab) --- + const hdrid = 'dedup-host-'+Date.now(); + const hd1 = await c(), hd2 = await c(); + await j(hd1, hdrid, 'dhost'); await j(hd2, hdrid, 'dguest'); hd1._m.length = hd2._m.length = 0; + s(hd1,'set_control_mode',{controlMode:'host-only'}); + await w(hd1,'control_mode'); await w(hd2,'control_mode'); + hd1._m.length = hd2._m.length = 0; + // The host's peerId re-joins on a fresh socket → server dedupes the old socket. + // This must NOT demote the host or reset the mode (a network blip / second tab). + const hd3 = await c(); + s(hd3,'join_room',{roomId:hdrid,peerId:'dhost',protocolVersion:'1.0.0'}); + const hdrd = await a(hd3); + assert.equal(hdrd[0],'room_data'); + assert.ok(hdrd[1].controlMode === 'host-only' && hdrd[1].hostPeerId === 'dhost', + 'host role + host-only mode survive peerId dedup (reconnect/second tab)'); + close(); + resetConnectionRate(); // --- Password room --- const prid = 'pw-'+Date.now(); diff --git a/server/index.js b/server/index.js index 678f6af..419b9e1 100644 --- a/server/index.js +++ b/server/index.js @@ -228,7 +228,13 @@ function removePeerFromRoom(socketId, roomId, reason) { // socket), fall back to 'everyone' so the room never gets stuck locked, and // reassign host to the earliest remaining peer so the feature stays usable. // (v1: immediate fallback, no grace period — see host-control-mode docs.) - if (!isPeerStillConnected && room.hostPeerId === peerId && room.peers.size > 0) { + // Skip while a join for this peerId is in flight (peerJoinLocks holds it): + // that's a reconnect / second tab where the same peerId is being re-added + // right after — covers both the explicit 'dedupe' removal AND the + // 'disconnect' the kicked old socket fires. Demoting there would silently + // unlock the room on every host network blip. + const peerRejoining = peerJoinLocks.has(peerId); + if (!peerRejoining && !isPeerStillConnected && room.hostPeerId === peerId && room.peers.size > 0) { const nextPeerData = room.peerData.values().next().value; room.hostPeerId = nextPeerData ? nextPeerData.peerId : null; room.controlMode = CONTROL_MODES.EVERYONE;