From a4fbd45d5d188a3af79327a969eff4b5350129c0 Mon Sep 17 00:00:00 2001 From: Psychotoxical <171614930+Psychotoxical@users.noreply.github.com> Date: Mon, 22 Jun 2026 15:25:39 +0200 Subject: [PATCH] =?UTF-8?q?fix(orbit):=20host-side=20session=20guards=20?= =?UTF-8?q?=E2=80=94=20settings=20default,=20maxUsers,=20push=20serialisat?= =?UTF-8?q?ion=20(#1157)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(orbit): unify session-settings default to the canonical preset updateOrbitSettings merged patches onto a hand-rolled { autoApprove: true, autoShuffle: true } fallback when a legacy session's blob predated the settings field. That literal disagreed with ORBIT_DEFAULT_SETTINGS (autoApprove: false), so toggling autoShuffle on such a session silently flipped autoApprove on, landing guest suggestions without host approval. Use ORBIT_DEFAULT_SETTINGS as the single source of truth for the fallback. Add a behavioural test for the settings-less, preserve-existing, and not-hosting paths. * fix(orbit): enforce maxUsers host-side in the participant rebuild The join gate only sees a one-tick-old state blob, so two guests can both pass the participants.length < maxUsers check and join past the cap at once; maxUsers was advisory. The host is the single writer of the canonical state, so enforce the cap there: admit at most maxUsers guests in the participant rebuild, earliest joiners winning (an established participant is never displaced by a newcomer) with a deterministic username tie-break for same-tick joins. Suggestions from an over-cap guest are ignored, consistent with their absence from the participant list. Reorders the fold so the admitted set gates the queue-additions loop. Add tests for the cap, the no-displacement rule, and the ignored over-cap suggestion. * fix(orbit): serialise host state pushes to stop overlapping ticks dropping writes pushState does several awaited round-trips (sweep -> resolve -> write) and is fired from three sources: the mount, the 2.5 s timer, and a play/pause subscription. With no in-flight lock these can overlap and race last-writer-wins: a slow run that already swept and cleared an outbox can lose its write to a faster run, dropping those suggestions from both the outbox and the published state. The lastPushedAtRef was only ever written, never used as a guard. Add makeCoalescedRunner: a pure helper that serialises an async task and coalesces any mid-flight request into a single rerun, so no trigger is lost. The host hook wraps pushState in it; the dead ref is removed. Add a unit test for the serialise + coalesce semantics. * docs(changelog): add Orbit host-side session guards (1157) --- CHANGELOG.md | 18 +++++++ src/hooks/useOrbitHost.ts | 20 ++++---- src/utils/orbit.ts | 1 + src/utils/orbit/helpers.test.ts | 38 +++++++++++++- src/utils/orbit/helpers.ts | 30 +++++++++++ src/utils/orbit/host.test.ts | 82 +++++++++++++++++++++++++++++++ src/utils/orbit/host.ts | 8 ++- src/utils/orbit/stateMath.test.ts | 55 +++++++++++++++++++++ src/utils/orbit/stateMath.ts | 80 +++++++++++++++++------------- 9 files changed, 287 insertions(+), 45 deletions(-) create mode 100644 src/utils/orbit/host.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b70ff18..c74c4402 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -232,6 +232,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * A guest who joined while radio was playing could have unrelated radio tracks appended to their queue, drifting them out of sync with the host. Automatic radio top-up is now paused while you are in an Orbit session. +### Orbit — auto-approve could switch on unexpectedly in older sessions + +**By [@Psychotoxical](https://github.com/Psychotoxical), PR [#1157](https://github.com/Psychotoxical/psysonic/pull/1157)** + +* In a long-running Orbit session created by an older build, toggling auto-shuffle could silently also turn on auto-approve, so guest suggestions started playing without the host approving them. The two settings are now independent again. + +### Orbit — sessions could go over their guest limit + +**By [@Psychotoxical](https://github.com/Psychotoxical), PR [#1157](https://github.com/Psychotoxical/psysonic/pull/1157)** + +* When several people joined at the same moment, a session could end up with more guests than its limit allowed. The host now enforces the limit, keeping the earliest joiners. + +### Orbit — an accepted suggestion could occasionally be dropped + +**By [@Psychotoxical](https://github.com/Psychotoxical), PR [#1157](https://github.com/Psychotoxical/psysonic/pull/1157)** + +* Under load the host's updates could overlap and overwrite each other, occasionally losing a guest suggestion that had just been picked up. Host updates are now serialised so none are lost. + ## [1.48.1] - 2026-06-15 ## Fixed diff --git a/src/hooks/useOrbitHost.ts b/src/hooks/useOrbitHost.ts index 4b4aa46e..c4080a83 100644 --- a/src/hooks/useOrbitHost.ts +++ b/src/hooks/useOrbitHost.ts @@ -1,6 +1,6 @@ import { getSong } from '../api/subsonicLibrary'; import { songToTrack } from '../utils/playback/songToTrack'; -import { useEffect, useRef } from 'react'; +import { useEffect } from 'react'; import { useOrbitStore } from '../store/orbitStore'; import { usePlayerStore } from '../store/playerStore'; import { @@ -9,6 +9,7 @@ import { applyOutboxSnapshotsToState, maybeShuffleQueue, effectiveShuffleIntervalMs, + makeCoalescedRunner, suggestionKey, } from '../utils/orbit'; import { @@ -49,10 +50,6 @@ export function useOrbitHost(): void { const sessionId = useOrbitStore(s => s.sessionId); const hostName = useOrbitStore(s => s.state?.host); - // Refs hold the last values we used to build the patch — cheap to - // recompute against, no need to subscribe to every playerStore tick. - const lastPushedAtRef = useRef(0); - const active = role === 'host' && phase === 'active' && !!sessionPlaylistId; useEffect(() => { @@ -138,7 +135,6 @@ export function useOrbitHost(): void { useOrbitStore.getState().setState(next); try { await writeOrbitState(sessionPlaylistId, next); - lastPushedAtRef.current = Date.now(); pushOrbitEvent('host:push', JSON.stringify({ track: next.currentTrack?.trackId ?? null, playing: next.isPlaying, @@ -221,11 +217,17 @@ export function useOrbitHost(): void { } }; + // Serialise pushState across its three triggers (mount, timer, play/pause + // flip): two bodies must never run concurrently, or a slow run that already + // swept+cleared an outbox can lose its write to a faster one. A request + // arriving mid-run coalesces into a single rerun so no trigger is lost. + const runPush = makeCoalescedRunner(pushState); + // Immediate push on mount so guests see fresh state without waiting // a full tick after the host comes online. - void pushState(); + void runPush(); - const id = window.setInterval(() => { void pushState(); }, STATE_TICK_MS); + const id = window.setInterval(() => { void runPush(); }, STATE_TICK_MS); // Event-driven push on play/pause flips. Without this the worst-case // delay between "host hits pause" and "guest stops" is two full polling @@ -239,7 +241,7 @@ export function useOrbitHost(): void { if (state.isPlaying === prevIsPlaying) return; prevIsPlaying = state.isPlaying; pushOrbitEvent('host:event-push', `isPlaying flip → ${state.isPlaying}`); - void pushState(); + void runPush(); }); return () => { diff --git a/src/utils/orbit.ts b/src/utils/orbit.ts index 5f1e7daa..3809f6a9 100644 --- a/src/utils/orbit.ts +++ b/src/utils/orbit.ts @@ -27,6 +27,7 @@ export { } from './orbit/constants'; export { generateSessionId, + makeCoalescedRunner, OrbitStateTooLarge, serialiseOrbitState, suggestionKey, diff --git a/src/utils/orbit/helpers.test.ts b/src/utils/orbit/helpers.test.ts index dab96c40..65787d92 100644 --- a/src/utils/orbit/helpers.test.ts +++ b/src/utils/orbit/helpers.test.ts @@ -7,7 +7,12 @@ import { type OrbitQueueItem, type OrbitState, } from '../../api/orbit'; -import { OrbitStateTooLarge, serialiseOrbitState, serialiseOrbitStateForWire } from './helpers'; +import { + makeCoalescedRunner, + OrbitStateTooLarge, + serialiseOrbitState, + serialiseOrbitStateForWire, +} from './helpers'; function baseState(): OrbitState { return makeInitialOrbitState({ sid: 'aaaa1111', host: 'host', name: 'sesh' }); @@ -67,3 +72,34 @@ describe('serialiseOrbitStateForWire', () => { expect((parsed!.playQueue ?? []).length).toBeLessThan(400); }); }); + +describe('makeCoalescedRunner', () => { + // Let the microtask queue drain so an awaiting do-while loop can advance. + const flush = async () => { await Promise.resolve(); await Promise.resolve(); }; + + it('never runs two task bodies concurrently and coalesces mid-flight calls into one rerun', async () => { + let starts = 0; + const releases: Array<() => void> = []; + const task = () => { + starts++; + return new Promise(res => { releases.push(res); }); + }; + const run = makeCoalescedRunner(task); + + void run(); // run #1 starts and blocks + void run(); // in-flight → flags a rerun, no second body + void run(); // in-flight → still just one pending rerun + expect(starts).toBe(1); + + releases[0](); // finish #1 → the coalesced rerun fires exactly one more body + await flush(); + expect(starts).toBe(2); + + releases[1](); // finish #2 → no rerun pending, runner goes idle + await flush(); + expect(starts).toBe(2); + + void run(); // lock released → a fresh call starts a new body + expect(starts).toBe(3); + }); +}); diff --git a/src/utils/orbit/helpers.ts b/src/utils/orbit/helpers.ts index 43803595..9b6a52c3 100644 --- a/src/utils/orbit/helpers.ts +++ b/src/utils/orbit/helpers.ts @@ -94,6 +94,36 @@ export function serialiseOutboxMeta(meta: OrbitOutboxMeta): string { export const suggestionKey = (q: OrbitQueueItem): string => `${q.addedBy}:${q.addedAt}:${q.trackId}`; +/** + * Wrap an async task so it never runs concurrently with itself. While a run is + * in flight, further calls do NOT start a second run — they flag a rerun, and + * exactly one more run fires after the current finishes (any number of + * mid-flight requests coalesce into a single catch-up). The host tick uses this + * because `pushState` does several awaited round-trips and is fired from a 2.5 s + * timer, the mount, and a play/pause subscription that can overlap; without + * serialisation a slow run that already swept+cleared an outbox can lose its + * write to a faster run (last-writer-wins), dropping those suggestions. + */ +export function makeCoalescedRunner(task: () => Promise): () => Promise { + let running = false; + let rerun = false; + return async () => { + if (running) { + rerun = true; + return; + } + running = true; + try { + do { + rerun = false; + await task(); + } while (rerun); + } finally { + running = false; + } + }; +} + /** Extract `` from a filename matching `__psyorbit__from___`. */ export function parseOutboxPlaylistName(name: string, sid: string): string | null { const prefix = `${ORBIT_PLAYLIST_PREFIX}${sid}_from_`; diff --git a/src/utils/orbit/host.test.ts b/src/utils/orbit/host.test.ts new file mode 100644 index 00000000..ee431f7c --- /dev/null +++ b/src/utils/orbit/host.test.ts @@ -0,0 +1,82 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + makeInitialOrbitState, + ORBIT_DEFAULT_SETTINGS, + type OrbitSettings, + type OrbitState, +} from '../../api/orbit'; + +const { writeOrbitState } = vi.hoisted(() => ({ writeOrbitState: vi.fn(() => Promise.resolve()) })); +const { orbitStore } = vi.hoisted(() => ({ + orbitStore: { + role: 'host' as 'host' | 'guest' | null, + state: null as OrbitState | null, + sessionPlaylistId: 'session-pl' as string | null, + setState: vi.fn(), + }, +})); + +vi.mock('./remote', () => ({ + writeOrbitState, + writeOrbitHeartbeat: vi.fn(() => Promise.resolve()), +})); +vi.mock('../../store/orbitStore', () => ({ useOrbitStore: { getState: () => orbitStore } })); +vi.mock('../../store/authStore', () => ({ useAuthStore: { getState: () => ({}) } })); +vi.mock('../../store/playerStore', () => ({ usePlayerStore: { getState: () => ({ enqueue: vi.fn() }) } })); +vi.mock('../../api/subsonicPlaylists', () => ({ createPlaylist: vi.fn(), deletePlaylist: vi.fn() })); +vi.mock('../../api/subsonicLibrary', () => ({ getSong: vi.fn() })); +vi.mock('../playback/songToTrack', () => ({ songToTrack: vi.fn() })); + +import { updateOrbitSettings } from './host'; + +function hostStateWith(settings: OrbitSettings | undefined): OrbitState { + const base = makeInitialOrbitState({ sid: 'aaaa1111', host: 'host', name: 'sesh' }); + return { ...base, settings: settings as OrbitSettings }; +} + +/** The settings object that updateOrbitSettings persisted on its last call. */ +function writtenSettings(): OrbitSettings { + const calls = writeOrbitState.mock.calls as unknown as Array<[string, OrbitState]>; + const lastCall = calls[calls.length - 1]; + return lastCall[1].settings as OrbitSettings; +} + +beforeEach(() => { + writeOrbitState.mockClear(); + orbitStore.setState.mockClear(); + orbitStore.role = 'host'; + orbitStore.sessionPlaylistId = 'session-pl'; +}); + +describe('updateOrbitSettings', () => { + it('does not silently flip autoApprove on a legacy settings-less session', async () => { + orbitStore.state = hostStateWith(undefined); + await updateOrbitSettings({ autoShuffle: false }); + + const settings = writtenSettings(); + // The patch only touched autoShuffle… + expect(settings.autoShuffle).toBe(false); + // …autoApprove must come from the canonical default (false), not flip true. + expect(settings.autoApprove).toBe(ORBIT_DEFAULT_SETTINGS.autoApprove); + expect(settings.autoApprove).toBe(false); + expect(settings.shuffleIntervalMin).toBe(ORBIT_DEFAULT_SETTINGS.shuffleIntervalMin); + }); + + it('preserves existing settings when patching one field', async () => { + orbitStore.state = hostStateWith({ autoApprove: true, autoShuffle: true, shuffleIntervalMin: 30 }); + await updateOrbitSettings({ autoShuffle: false }); + + const settings = writtenSettings(); + expect(settings.autoApprove).toBe(true); + expect(settings.autoShuffle).toBe(false); + expect(settings.shuffleIntervalMin).toBe(30); + }); + + it('is a no-op when not hosting', async () => { + orbitStore.role = 'guest'; + orbitStore.state = hostStateWith(undefined); + await updateOrbitSettings({ autoShuffle: false }); + expect(writeOrbitState).not.toHaveBeenCalled(); + }); +}); diff --git a/src/utils/orbit/host.ts b/src/utils/orbit/host.ts index 0934fafe..6dd7b2a7 100644 --- a/src/utils/orbit/host.ts +++ b/src/utils/orbit/host.ts @@ -9,6 +9,7 @@ import { orbitOutboxPlaylistName, orbitSessionPlaylistName, ORBIT_DEFAULT_MAX_USERS, + ORBIT_DEFAULT_SETTINGS, type OrbitQueueItem, type OrbitSettings, type OrbitState, @@ -163,8 +164,13 @@ export async function triggerOrbitShuffleNow(): Promise { export async function updateOrbitSettings(patch: Partial): Promise { const store = useOrbitStore.getState(); if (store.role !== 'host' || !store.state || !store.sessionPlaylistId) return; + // Fall back to the canonical defaults (not a hand-rolled literal) for + // legacy sessions whose blob predates the settings field — otherwise + // patching one field on a settings-less session would silently flip + // autoApprove on, since the old literal had it true while + // ORBIT_DEFAULT_SETTINGS (the popover's source of truth) has it false. const mergedSettings: OrbitSettings = { - ...(store.state.settings ?? { autoApprove: true, autoShuffle: true }), + ...(store.state.settings ?? ORBIT_DEFAULT_SETTINGS), ...patch, }; const next: OrbitState = { ...store.state, settings: mergedSettings }; diff --git a/src/utils/orbit/stateMath.test.ts b/src/utils/orbit/stateMath.test.ts index 3882b238..70dc2aea 100644 --- a/src/utils/orbit/stateMath.test.ts +++ b/src/utils/orbit/stateMath.test.ts @@ -57,3 +57,58 @@ describe('applyOutboxSnapshotsToState — queue history cap', () => { expect(next.queue.map(q => q.trackId)).toEqual(['t0', 't1']); }); }); + +describe('applyOutboxSnapshotsToState — maxUsers enforcement', () => { + const NOW = 5_000; + const fresh = (user: string, trackIds: string[] = []): OutboxSnapshot => ({ + user, + outboxPlaylistId: `ob-${user}`, + trackIds, + lastHeartbeat: NOW, + }); + + function hostState(maxUsers: number, participants: OrbitState['participants']): OrbitState { + return { + ...makeInitialOrbitState({ sid: 'aaaa1111', host: 'host', name: 'sesh', maxUsers }), + participants, + }; + } + + it('admits at most maxUsers guests, dropping the surplus', () => { + const state = hostState(2, []); + const next = applyOutboxSnapshotsToState( + state, + [fresh('aaa'), fresh('bbb'), fresh('ccc')], + NOW, + ); + // Three guests joined on the same tick; the deterministic username + // tie-break keeps the first two alphabetically. + expect(next.participants.map(p => p.user)).toEqual(['aaa', 'bbb']); + }); + + it('never displaces an established participant for a newcomer', () => { + // `zoe` joined earlier (smaller joinedAt) — she keeps her slot even though + // her name sorts last. + const state = hostState(2, [{ user: 'zoe', joinedAt: 1_000, lastHeartbeat: 1_000 }]); + const next = applyOutboxSnapshotsToState( + state, + [fresh('zoe'), fresh('aaa'), fresh('bbb')], + NOW, + ); + expect(next.participants.map(p => p.user)).toEqual(['zoe', 'aaa']); + }); + + it('ignores suggestions from an over-cap guest', () => { + const state = hostState(2, []); + const next = applyOutboxSnapshotsToState( + state, + [fresh('aaa', ['t-a']), fresh('bbb', ['t-b']), fresh('ccc', ['t-c'])], + NOW, + ); + const addedTracks = next.queue.map(q => q.trackId); + expect(addedTracks).toContain('t-a'); + expect(addedTracks).toContain('t-b'); + // ccc is over the cap → not a participant, suggestion ignored. + expect(addedTracks).not.toContain('t-c'); + }); +}); diff --git a/src/utils/orbit/stateMath.ts b/src/utils/orbit/stateMath.ts index bbbff1f8..6cfed6de 100644 --- a/src/utils/orbit/stateMath.ts +++ b/src/utils/orbit/stateMath.ts @@ -97,58 +97,70 @@ export function applyOutboxSnapshotsToState( snapshots: OutboxSnapshot[], nowMs: number = Date.now(), ): OrbitState { - // ── Queue additions ── - // Guest outboxes are append-only from the host's POV — the host reads the - // same playlist every sweep, so we must dedupe against anything already in - // `state.queue` (or currently playing) by (user, trackId). Without this, - // every host tick re-adds every outbox entry and the pending-approval list - // balloons indefinitely. A user re-suggesting the same track after it - // lands/plays is a rare enough case to live with for now. - const existingKeys = new Set( - state.queue.map(q => `${q.addedBy} ${q.trackId}`), - ); - if (state.currentTrack) { - existingKeys.add(`${state.currentTrack.addedBy} ${state.currentTrack.trackId}`); - } - - // Drop any new suggestion from a user the host has muted before the - // dedupe scan — they shouldn't count against the queue at all. - const blocked = new Set(state.suggestionBlocked ?? []); - const newItems: OrbitQueueItem[] = []; - for (const snap of snapshots) { - if (blocked.has(snap.user)) continue; - for (const trackId of snap.trackIds) { - const key = `${snap.user} ${trackId}`; - if (existingKeys.has(key)) continue; - existingKeys.add(key); - newItems.push({ trackId, addedBy: snap.user, addedAt: nowMs }); - } - } - // ── Soft-removed list aging ── // Drop entries older than the TTL so the list stays bounded and a long- // expired marker doesn't kick a freshly-rejoined user back out. const removed = (state.removed ?? []).filter(r => nowMs - r.at < ORBIT_REMOVED_TTL_MS); const removedUsers = new Set(removed.map(r => r.user)); - // ── Participants rebuild ── - // Soft-removed users stay out of `participants` even if their heartbeat is - // still fresh — gives them up to one read tick (~2.5s) to notice the - // `removed`-marker and tear down their guest hooks before the marker ages out. + // ── Participants rebuild (with maxUsers enforcement) ── + // Anyone with a fresh heartbeat who isn't kicked or soft-removed is + // *eligible*. Soft-removed users stay out even if their heartbeat is still + // fresh — gives them up to one read tick (~2.5s) to notice the `removed` + // marker and tear down their guest hooks before it ages out. + // + // The join gate (`joinOrbitSession`) only sees a one-tick-old blob, so two + // guests can both pass the `participants.length < maxUsers` check and slip + // in past the cap at once. The host is the single writer of the canonical + // state, so it's the only place the cap can truly hold — enforce it here by + // admitting at most `maxUsers` guests. Earliest joiners win (an established + // participant is never displaced by a newcomer; their `joinedAt` is older), + // with a deterministic username tie-break for guests that joined on the + // same tick. The host runs its own outbox heartbeat but is filtered out by + // the sweep, so `participants` is guests only — the cap matches `maxUsers`. const prev = new Map(state.participants.map(p => [p.user, p])); - const participants: OrbitParticipant[] = []; + const eligible: OrbitParticipant[] = []; for (const snap of snapshots) { if (state.kicked.includes(snap.user)) continue; if (removedUsers.has(snap.user)) continue; const fresh = snap.lastHeartbeat > 0 && (nowMs - snap.lastHeartbeat) < ORBIT_HEARTBEAT_ALIVE_MS; if (!fresh) continue; const existing = prev.get(snap.user); - participants.push({ + eligible.push({ user: snap.user, joinedAt: existing?.joinedAt ?? nowMs, lastHeartbeat: snap.lastHeartbeat, }); } + eligible.sort((a, b) => a.joinedAt - b.joinedAt || a.user.localeCompare(b.user)); + const participants = eligible.slice(0, Math.max(0, state.maxUsers)); + const admitted = new Set(participants.map(p => p.user)); + + // ── Queue additions ── + // Guest outboxes are append-only from the host's POV — the host reads the + // same playlist every sweep, so we dedupe against anything already in + // `state.queue` (or currently playing) by (user, trackId). Without this, + // every host tick re-adds every outbox entry and the pending-approval list + // balloons indefinitely. A suggestion only counts from an *admitted* guest + // who isn't muted — an over-cap guest's outbox is ignored, consistent with + // their absence from `participants`. + const existingKeys = new Set( + state.queue.map(q => `${q.addedBy} ${q.trackId}`), + ); + if (state.currentTrack) { + existingKeys.add(`${state.currentTrack.addedBy} ${state.currentTrack.trackId}`); + } + const blocked = new Set(state.suggestionBlocked ?? []); + const newItems: OrbitQueueItem[] = []; + for (const snap of snapshots) { + if (blocked.has(snap.user) || !admitted.has(snap.user)) continue; + for (const trackId of snap.trackIds) { + const key = `${snap.user} ${trackId}`; + if (existingKeys.has(key)) continue; + existingKeys.add(key); + newItems.push({ trackId, addedBy: snap.user, addedAt: nowMs }); + } + } return { ...state,