From af4cb186f3c732fcd00154eb0fba1ebfb668dabe Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 23 Jun 2026 23:43:17 -0400 Subject: [PATCH 1/2] Phase 21.3: seek-back-past-window refill + clean refill-failure recovery Seek-back past the retained tail reuses the existing seek-beyond-buffer Range path (per-path resolver). A failed refill now halts the scheduler into a paused-but-loaded state (AC6) instead of a silent false end. --- .../Services/AudioInteropService.cs | 12 + .../Services/StreamingAudioPlayerService.cs | 48 +++- .../Interop/audio/AudioPlayer.test.ts | 253 ++++++++++++++++++ DeepDrftPublic/Interop/audio/AudioPlayer.ts | 61 ++++- DeepDrftPublic/Interop/audio/index.ts | 9 + 5 files changed, 370 insertions(+), 13 deletions(-) create mode 100644 DeepDrftPublic/Interop/audio/AudioPlayer.test.ts diff --git a/DeepDrftPublic.Client/Services/AudioInteropService.cs b/DeepDrftPublic.Client/Services/AudioInteropService.cs index d463aa3..92e0fc7 100644 --- a/DeepDrftPublic.Client/Services/AudioInteropService.cs +++ b/DeepDrftPublic.Client/Services/AudioInteropService.cs @@ -183,6 +183,18 @@ public class AudioInteropService : IAsyncDisposable return await InvokeJsAsync("DeepDrftAudio.reinitializeFromOffset", playerId, totalStreamLength, seekPosition); } + /// + /// Phase 21.3 / AC6 clean-failure recovery: after a window-miss refill (seek-back past the + /// retained tail) fails its Range fetch or reinit, halt the starved scheduler and leave the + /// player paused-but-loaded at so no silent false end fires and a + /// retry is possible. Routes through so an interop failure during + /// recovery still yields a failure result rather than throwing into the seek path. + /// + public async Task RecoverFromFailedRefill(string playerId, double seekPosition) + { + return await InvokeJsAsync("DeepDrftAudio.recoverFromFailedRefill", playerId, seekPosition); + } + public async Task SetVolumeAsync(string playerId, double volume) { return await InvokeJsAsync("DeepDrftAudio.setVolume", playerId, volume); diff --git a/DeepDrftPublic.Client/Services/StreamingAudioPlayerService.cs b/DeepDrftPublic.Client/Services/StreamingAudioPlayerService.cs index 08c0bec..3f1dfca 100644 --- a/DeepDrftPublic.Client/Services/StreamingAudioPlayerService.cs +++ b/DeepDrftPublic.Client/Services/StreamingAudioPlayerService.cs @@ -649,8 +649,7 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS { var technicalError = mediaResult.GetMessage() ?? "Failed to load audio from position"; _logger.LogError("Failed to get track media from offset {Offset}: {Error}", byteOffset, technicalError); - ErrorMessage = StreamingErrorHandler.GetUserFriendlyMessage(technicalError); - IsSeekingBeyondBuffer = false; + await RecoverFromFailedRefill(seekPosition, StreamingErrorHandler.GetUserFriendlyMessage(technicalError)); return; } @@ -661,8 +660,7 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS if (!reinitResult.Success) { _logger.LogError("Failed to reinitialize for offset streaming: {Error}", reinitResult.Error); - ErrorMessage = "Failed to seek to position"; - IsSeekingBeyondBuffer = false; + await RecoverFromFailedRefill(seekPosition, "Failed to seek to position"); return; } @@ -691,13 +689,49 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS } catch (Exception ex) { + // A refill fetch can fail deep into a long mix (the listener didn't initiate it). Recover + // into a clean paused-but-loaded state (AC6) rather than leaving the starved scheduler to + // fire a silent false end. Only when we are still the active seek — a superseding seek owns + // the state and the OCE catch above handles its own teardown. _logger.LogError(ex, "Error during seek beyond buffer to position {Position}", seekPosition); - ErrorMessage = StreamingErrorHandler.GetUserFriendlyMessage(ex.Message); - IsSeekingBeyondBuffer = false; - await NotifyStateChanged(); + if (ReferenceEquals(_streamingCancellation, seekCts)) + { + await RecoverFromFailedRefill(seekPosition, StreamingErrorHandler.GetUserFriendlyMessage(ex.Message)); + } } } + /// + /// Clean-failure recovery for a window-miss refill (Phase 21.3 / AC6). A backward seek past the + /// retained tail re-fetches via the existing Range path; that mid-stream fetch the listener did not + /// initiate can fail deep into a long mix. When it does, the pre-seek loop has already been + /// cancelled and drained, but the JS scheduler is still holding stale pre-seek buffers and still + /// "playing" — left alone it drains them and fires a silent false end (the wedged/starved state AC6 + /// forbids). This halts the scheduler into a paused-but-loaded state at , + /// surfaces a clear error, and leaves the track loaded so the listener can retry the seek or pick + /// another track. Mirrors PlaybackScheduler.playFromPosition's end-of-buffer recovery: stop + /// pretending to play. + /// + private async Task RecoverFromFailedRefill(double seekPosition, string userFacingError) + { + // Halt the starved scheduler JS-side (stop sources, drop stale buffers, anchor at the target). + // Best-effort: if even this interop fails the player is no worse off, and we still surface the + // error and settle C# state below. + var recovered = await _audioInterop.RecoverFromFailedRefill(PlayerId, seekPosition); + if (!recovered.Success) + { + _logger.LogWarning("Refill-failure recovery interop did not succeed: {Error}", recovered.Error); + } + + // Settle C# into the matching recoverable state: not playing, paused at the target, still loaded. + ErrorMessage = userFacingError; + IsPlaying = false; + IsPaused = true; + CurrentTime = seekPosition; + IsSeekingBeyondBuffer = false; + await NotifyStateChanged(); + } + /// /// Single method to reset all state - called by both Stop and Unload. /// diff --git a/DeepDrftPublic/Interop/audio/AudioPlayer.test.ts b/DeepDrftPublic/Interop/audio/AudioPlayer.test.ts new file mode 100644 index 0000000..f9931e3 --- /dev/null +++ b/DeepDrftPublic/Interop/audio/AudioPlayer.test.ts @@ -0,0 +1,253 @@ +/** + * AudioPlayer window-miss refill tests (Phase 21.3) — the seek-dispatch TRIGGER and the AC6 + * clean-failure recovery. + * + * What this pins (the genuinely-new 21.3 work): + * - The window-miss TRIGGER. AudioPlayer.seek() routes by whether the target falls inside the + * retained window [playbackOffset, playbackOffset + totalDuration]. After 21.1 partial eviction + * playbackOffset is the absolute start of the retained back-window tail, so: + * * seek back WITHIN the tail -> seekWithinBuffer, NO refetch (UC3 / AC4), + * * seek back PAST the tail -> seekBeyondBuffer with the EARLIER resolved offset (UC4 / AC5), + * using whichever resolver the active path ships (WAV calculateByteOffset; Opus + * resolveOpusByteOffset over the sidecar index), + * * seek forward past the decoded end -> seekBeyondBuffer forward, unchanged (UC2/UC5). + * - The AC6 recovery. recoverFromFailedRefill() halts the scheduler (clearForSeek), anchors the + * offset at the seek target, and leaves the player paused-but-loaded so no silent false end fires. + * + * The seek dispatch and recovery are pure given the scheduler + active decoder, so they are testable + * in Node by white-box-injecting fakes for `scheduler`, `streamDecoder`, and `opusDecoder` (the same + * private-field injection idiom the scheduler/Opus tests use). The AudioPlayer constructor itself is + * Node-safe: it builds AudioContextManager/StreamDecoder/PlaybackScheduler, none of which touch Web + * Audio until initialize(). No AudioContext, no WebCodecs. + * + * Same harness convention as the sibling tests (no runner in this repo); run a copy from the COMPILED + * output so the `.js` import specifiers resolve: + * + * dotnet build DeepDrftPublic/DeepDrftPublic.csproj + * cp DeepDrftPublic/Interop/audio/AudioPlayer.test.ts DeepDrftPublic/wwwroot/js/audio/ + * node DeepDrftPublic/wwwroot/js/audio/AudioPlayer.test.ts + * + * A thrown error / non-zero exit signals failure; "ALL TESTS PASSED" signals success. + * Excluded from the production tsc build via tsconfig `exclude: Interop/ ** /*.test.ts`. + */ + +import { AudioPlayer } from './AudioPlayer.js'; +import { parseSidecar } from './OpusSidecar.js'; +import type { OpusSeekData } from './OpusSidecar.js'; + +// --- tiny inline harness (no dependencies) --------------------------------------------------- +let passed = 0; +const failures: string[] = []; +function test(name: string, fn: () => void): void { + try { + fn(); + passed++; + } catch (e) { + failures.push(`FAIL: ${name}\n ${(e as Error).message}`); + } +} +function assertEqual(actual: unknown, expected: unknown, msg?: string): void { + if (actual !== expected) { + throw new Error(`${msg ?? 'assertEqual'}: expected ${String(expected)}, got ${String(actual)}`); + } +} +function assertTrue(cond: boolean, msg?: string): void { + if (!cond) throw new Error(msg ?? 'assertTrue failed'); +} + +// --- fakes ----------------------------------------------------------------------------------- + +/** + * A scheduler stand-in exposing only what AudioPlayer.seek / seekWithinBuffer / seekBeyondBuffer / + * recoverFromFailedRefill read or call. The retained window is [offset, offset + total]. Records the + * methods that mutate so the recovery test can assert the cleanup happened. + */ +class FakeScheduler { + private offset: number; + private total: number; + public clearedForSeek = false; + public stoppedAllSources = false; + public offsetSetTo: number | null = null; + constructor(offset: number, total: number) { this.offset = offset; this.total = total; } + + getPlaybackOffset(): number { return this.offset; } + getTotalDuration(): number { return this.total; } + stopAllSources(): void { this.stoppedAllSources = true; } + // seekWithinBuffer calls playFromPosition only when wasPlaying; isPlaying is false in these + // unit constructions, so it is never invoked — present for completeness. + playFromPosition(_position: number): void { /* no-op */ } + clearForSeek(): void { this.clearedForSeek = true; } + setPlaybackOffset(o: number): void { this.offset = o; this.offsetSetTo = o; } +} + +/** A StreamDecoder stand-in for the WAV path: a format is parsed and byte math is identity-scaled. */ +class FakeStreamDecoder { + private hasFormat: boolean; + private bytesPerSecond: number; + public requestedOffsetFor: number | null = null; + constructor(hasFormat: boolean, bytesPerSecond: number) { this.hasFormat = hasFormat; this.bytesPerSecond = bytesPerSecond; } + getFormatInfo(): unknown { return this.hasFormat ? { ok: true } : null; } + calculateByteOffset(position: number): number { + this.requestedOffsetFor = position; + return Math.round(position * this.bytesPerSecond); + } +} + +function makePlayer(): AudioPlayer { + // Constructor is Node-safe (no Web Audio until initialize()). + return new AudioPlayer(); +} + +/** Inject the seek-relevant private fields and put the player in a loaded/streaming/playing state. */ +function arm( + player: AudioPlayer, + opts: { + scheduler: FakeScheduler; + duration: number; + streamDecoder?: FakeStreamDecoder; + opusDecoder?: object | null; + sidecar?: OpusSeekData | null; + } +): void { + const priv = player as unknown as Record; + priv.scheduler = opts.scheduler; + priv.duration = opts.duration; + priv.isStreamingMode = true; + priv.isPlaying = false; // keep dispatch pure (no real playFromPosition needed) + if (opts.streamDecoder) priv.streamDecoder = opts.streamDecoder; + priv.opusDecoder = opts.opusDecoder ?? null; + priv.activeOpusSidecar = opts.sidecar ?? null; +} + +/** Read back private fields the recovery sets. */ +function priv(player: AudioPlayer): Record { + return player as unknown as Record; +} + +// A minimal real sidecar (parsed) so the Opus resolver returns a deterministic page offset. +// Index: t=0 -> byte 4096, t=1s -> byte 9000 (granule uses 48 kHz + preSkip). +function makeOpusSidecar(): OpusSeekData { + const setup = [0x4f, 0x70, 0x75, 0x73, 0x48, 0x65, 0x61, 0x64]; + const SEEK_INDEX_HEADER_SIZE = 24; + const SEEK_POINT_SIZE = 16; + const preSkip = 312; + const points = [ + { granule: preSkip, byteOffset: 4096 }, // t = 0 + { granule: preSkip + 48000, byteOffset: 9000 }, // t = 1 s + ]; + const total = 4 + setup.length + SEEK_INDEX_HEADER_SIZE + points.length * SEEK_POINT_SIZE; + const bytes = new Uint8Array(total); + const view = new DataView(bytes.buffer); + view.setUint32(0, setup.length, true); + bytes.set(setup, 4); + let p = 4 + setup.length; + const writeU64 = (off: number, v: number) => { + view.setUint32(off, v >>> 0, true); + view.setUint32(off + 4, Math.floor(v / 0x100000000), true); + }; + writeU64(p, 500_000); + view.setFloat64(p + 8, 100, true); + view.setUint32(p + 16, points.length, true); + view.setUint16(p + 20, preSkip, true); + p += SEEK_INDEX_HEADER_SIZE; + for (const pt of points) { writeU64(p, pt.granule); writeU64(p + 8, pt.byteOffset); p += SEEK_POINT_SIZE; } + const parsed = parseSidecar(bytes); + if (!parsed) throw new Error('test setup: sidecar failed to parse'); + return parsed; +} + +// --- TRIGGER: within-window vs past-tail vs forward ------------------------------------------ + +// UC3 / AC4: a backward seek INTO the retained tail resolves from buffer — NO seekBeyondBuffer, +// NO refetch signal. Window is [30, 60); target 40 is inside. +test('seek back within retained tail resolves in-buffer (no refetch) — AC4', () => { + const player = makePlayer(); + const scheduler = new FakeScheduler(30, 30); // retained window [30, 60) + arm(player, { scheduler, duration: 120, streamDecoder: new FakeStreamDecoder(true, 1000) }); + + const result = player.seek(40); + assertEqual(result.success, true, 'seek succeeds'); + assertEqual(result.seekBeyondBuffer ?? false, false, 'within-window seek does NOT signal a refetch'); + // No clearForSeek / no offset request — the retained window served it. + assertEqual(scheduler.clearedForSeek, false, 'no clearForSeek for an in-buffer seek'); +}); + +// UC4 / AC5 (WAV): a backward seek PAST the retained tail signals a refill at the EARLIER resolved +// offset, using the WAV resolver. Window [30, 60); target 10 is before the tail. +test('seek back past retained tail refetches at the WAV-resolved earlier offset — AC5', () => { + const player = makePlayer(); + const scheduler = new FakeScheduler(30, 30); // retained window [30, 60) + const wav = new FakeStreamDecoder(true, 2000); // 2000 bytes/sec + arm(player, { scheduler, duration: 120, streamDecoder: wav }); + + const result = player.seek(10); // earlier than the retained tail start (30) + assertEqual(result.success, true, 'seek succeeds'); + assertEqual(result.seekBeyondBuffer, true, 'past-tail back seek signals a refill (window miss)'); + assertEqual(wav.requestedOffsetFor, 10, 'WAV resolver consulted for the EARLIER target'); + assertEqual(result.byteOffset, 20000, 'refill offset is the WAV-resolved earlier byte offset'); +}); + +// UC4 / AC5 (Opus): the same window miss on the Opus path uses resolveOpusByteOffset over the +// sidecar index (the live seek), not WAV byte math. Target 0.3 s resolves to the t=0 page (4096). +test('seek back past retained tail refetches at the Opus index-resolved offset — AC5', () => { + const player = makePlayer(); + const scheduler = new FakeScheduler(30, 30); // retained window [30, 60) + arm(player, { + scheduler, + duration: 100, + opusDecoder: {}, // presence routes seekBeyondBuffer down the Opus branch + sidecar: makeOpusSidecar(), + }); + + const result = player.seek(0.3); // earlier than the retained tail (30) -> window miss + assertEqual(result.success, true, 'seek succeeds'); + assertEqual(result.seekBeyondBuffer, true, 'past-tail back seek signals a refill on Opus too'); + assertEqual(result.byteOffset, 4096, 'Opus index resolved the t=0 page start for the earlier target'); + // The landing time of the resolved page is captured for the decoder lead-trim (AC9 reuse). + assertEqual(priv(player)._seekLandingTime, 0, 'landing time of the resolved page captured for lead-trim'); +}); + +// UC2/UC5: a forward seek past the decoded end still routes to seekBeyondBuffer forward, unchanged. +test('forward seek past decoded end still routes to seekBeyondBuffer (unchanged)', () => { + const player = makePlayer(); + const scheduler = new FakeScheduler(30, 30); // decoded [30, 60) + const wav = new FakeStreamDecoder(true, 1500); + arm(player, { scheduler, duration: 120, streamDecoder: wav }); + + const result = player.seek(90); // past the decoded end (60) + assertEqual(result.seekBeyondBuffer, true, 'forward-beyond-buffer still signals a fetch'); + assertEqual(wav.requestedOffsetFor, 90, 'forward target resolved through the same WAV resolver'); + assertEqual(result.byteOffset, 135000, 'forward offset is the resolved later byte offset'); +}); + +// --- AC6: clean-failure recovery ------------------------------------------------------------- + +// A failed refill must leave the player recoverable: scheduler halted (clearForSeek), offset anchored +// at the seek target, paused-but-loaded — never a starved "playing" scheduler that fires a false end. +test('recoverFromFailedRefill halts the scheduler and leaves a paused-but-loaded state — AC6', () => { + const player = makePlayer(); + const scheduler = new FakeScheduler(30, 30); + arm(player, { scheduler, duration: 120, streamDecoder: new FakeStreamDecoder(true, 1000) }); + // Simulate the pre-failure "playing" state the drained pre-seek loop leaves behind. + priv(player).isPlaying = true; + priv(player).isPaused = false; + priv(player).streamingStarted = true; + + const result = player.recoverFromFailedRefill(15); + assertEqual(result.success, true, 'recovery succeeds'); + assertTrue(scheduler.clearedForSeek, 'stale buffers dropped (no false end can fire)'); + assertEqual(scheduler.offsetSetTo, 15, 'offset anchored at the seek target for a retry'); + assertEqual(priv(player).isPlaying, false, 'not playing after recovery'); + assertEqual(priv(player).isPaused, true, 'paused after recovery'); + assertEqual(priv(player).pausePosition, 15, 'pause anchor is the seek target'); + assertEqual(priv(player).streamingStarted, false, 'streaming flagged not-started for a clean retry'); +}); + +// --- run ------------------------------------------------------------------------------------- +if (failures.length > 0) { + console.error(failures.join('\n')); + console.error(`\n${failures.length} FAILED, ${passed} passed`); + process.exit(1); +} else { + console.log(`ALL ${passed} TESTS PASSED`); +} diff --git a/DeepDrftPublic/Interop/audio/AudioPlayer.ts b/DeepDrftPublic/Interop/audio/AudioPlayer.ts index 6fea8dc..33ead74 100644 --- a/DeepDrftPublic/Interop/audio/AudioPlayer.ts +++ b/DeepDrftPublic/Interop/audio/AudioPlayer.ts @@ -432,18 +432,29 @@ export class AudioPlayer { return { success: false, error: 'Invalid seek position' }; } + // bufferStart is the absolute track time at which buffers[0] begins. Under Phase 21.1 + // partial eviction this is the start of the RETAINED BACK-WINDOW TAIL — eviction advances + // playbackOffset as it drops played buffers off the front — so [bufferStart, bufferEnd] is + // exactly the window currently held in memory. const bufferStart = this.scheduler.getPlaybackOffset(); const bufferEnd = this.scheduler.getTotalDuration() + bufferStart; - // Position must be within [bufferStart, bufferEnd] to use buffered content. - // A lower-bound check is required: after a seek-beyond-buffer, bufferStart is - // set to the prior seek position. Seeking to a position below bufferStart would - // produce a negative bufferRelativePosition in seekWithinBuffer, silently - // clamping to position 0 of the offset buffer instead of the requested time. + // The window-miss test for BOTH directions, and the 21.3 refill trigger for backward seeks. + // Position must be within [bufferStart, bufferEnd] to resolve from the retained buffers: + // - position >= bufferStart : UC3 — seek back within the retained back-window. Served from + // buffer with NO network refetch. (The lower bound is load-bearing: after eviction or a + // prior seek-beyond-buffer, bufferStart > 0, and a target below it would otherwise produce + // a negative bufferRelativePosition in seekWithinBuffer, silently clamping to position 0.) + // - position < bufferStart : UC4 — seek back PAST the retained tail (the window was evicted). + // Falls through to seekBeyondBuffer, which is the existing Range path run toward an EARLIER + // offset. This is the 21.3 window-miss refill: "a seek the listener didn't initiate" reuses + // the same per-path resolver + reinit a forward seek-beyond-buffer uses, no new mechanism. + // - position > bufferEnd : UC2/UC5 — forward seek beyond buffer, unchanged. if (position >= bufferStart && position <= bufferEnd) { return this.seekWithinBuffer(position); } else { - // Seeking outside buffered window - signal C# to fetch new stream + // Seeking outside the retained window - signal C# to fetch a new stream from the resolved + // offset (earlier for a back-past-window refill, later for a forward seek). return this.seekBeyondBuffer(position); } } @@ -580,6 +591,44 @@ export class AudioPlayer { } } + /** + * Recover the player into a clean, paused-but-loaded state after a window-miss REFILL failed + * (Phase 21.3 / AC6). A refill is "a seek the listener didn't initiate"; when its Range fetch or + * reinit fails mid-stream, the pre-seek loop has already been cancelled and drained, but the + * scheduler is still holding stale pre-seek buffers and is still `isActive_`. Left alone it would + * play the retained tail to exhaustion and fire `onPlaybackEnded` — a SILENT FALSE END (the + * "wedged playing with a starved scheduler" AC6 forbids). + * + * The recovery mirrors `PlaybackScheduler.playFromPosition`'s end-of-buffer recovery in spirit: + * stop pretending to play. We stop all sources and clear the buffers for a seek (clearForSeek + * keeps no stale audio but is ready to accept a fresh continuation), set the offset to the + * requested seek position, and leave the player paused there. The track stays loaded so the + * listener can retry the seek or pick another track — no new transport control, only a recoverable + * stop (C4). A subsequent seek to the same target re-enters seekBeyondBuffer cleanly because the + * offset names the seek position and the scheduler is empty (so it routes to a fresh fetch). + * + * @param seekPosition The seek target the failed refill was aiming for; becomes the resume anchor. + */ + recoverFromFailedRefill(seekPosition: number): AudioResult { + try { + this.stopProgressTracking(); + // Halt the starved scheduler and drop the stale pre-seek buffers so no false end can fire. + this.scheduler.clearForSeek(); + this.scheduler.setPlaybackOffset(seekPosition); + + // Paused-but-loaded: not playing, not mid-seek-stream. pausePosition anchors a retry. + this.isPlaying = false; + this.isPaused = true; + this.pausePosition = seekPosition; + this.streamingStarted = false; + this.streamingCompleted = false; + + return { success: true }; + } catch (error) { + return { success: false, error: (error as Error).message }; + } + } + // ==================== Volume ==================== setVolume(volume: number): AudioResult { diff --git a/DeepDrftPublic/Interop/audio/index.ts b/DeepDrftPublic/Interop/audio/index.ts index 136b329..731abe8 100644 --- a/DeepDrftPublic/Interop/audio/index.ts +++ b/DeepDrftPublic/Interop/audio/index.ts @@ -131,6 +131,15 @@ const DeepDrftAudio = { return player.reinitializeFromOffset(totalStreamLength, seekPosition); }, + // Phase 21.3 / AC6: recover into a clean paused-but-loaded state after a window-miss refill + // (seek-back past the retained tail) failed its Range fetch or reinit. Prevents the starved + // scheduler from firing a silent false end; leaves the track loaded so a retry is possible. + recoverFromFailedRefill: (playerId: string, seekPosition: number): AudioResult => { + const player = audioPlayers.get(playerId); + if (!player) return { success: false, error: 'Player not found' }; + return player.recoverFromFailedRefill(seekPosition); + }, + setVolume: (playerId: string, volume: number): AudioResult => { const player = audioPlayers.get(playerId); if (!player) return { success: false, error: 'Player not found' }; From b93881cd66e7c6d8a333781d6861280e9bcd43e9 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 23 Jun 2026 23:55:28 -0400 Subject: [PATCH 2/2] 21.3 review fixes: guard superseded-seek failures; restore post-recovery retry C6/AC8: IsStillActiveSeek() predicate guards all three SeekBeyondBuffer failure exits, so a superseded seek never recovers over a newer seek's state. AC6: empty scheduler routes to seekBeyondBuffer so a same-target retry (seek or play) refetches instead of no-oping. --- .../Services/AudioPlayerService.cs | 12 ++++- .../Services/StreamingAudioPlayerService.cs | 33 +++++++++++-- .../Interop/audio/AudioPlayer.test.ts | 49 ++++++++++++++++++- DeepDrftPublic/Interop/audio/AudioPlayer.ts | 23 ++++++--- 4 files changed, 102 insertions(+), 15 deletions(-) diff --git a/DeepDrftPublic.Client/Services/AudioPlayerService.cs b/DeepDrftPublic.Client/Services/AudioPlayerService.cs index 778cc1d..5221e3c 100644 --- a/DeepDrftPublic.Client/Services/AudioPlayerService.cs +++ b/DeepDrftPublic.Client/Services/AudioPlayerService.cs @@ -118,6 +118,16 @@ public abstract class AudioPlayerService : IPlayerService, IAsyncDisposable IsPlaying = true; IsPaused = false; } + else if (IsPaused) + { + // Play failed while the player is paused — the scheduler may be empty after a + // failed refill (AC6 recovery). Re-issue a seek at the current position: the + // seek path routes to seekBeyondBuffer when the scheduler is empty (Phase 21.3 + // fix), triggering a real refetch rather than returning "Streaming not ready". + // We return early here; Seek owns its own state mutations and NotifyStateChanged. + await Seek(CurrentTime); + return; + } } if (!result.Success) @@ -128,7 +138,7 @@ public abstract class AudioPlayerService : IPlayerService, IAsyncDisposable { ErrorMessage = null; } - + await NotifyStateChanged(); } catch (Exception ex) diff --git a/DeepDrftPublic.Client/Services/StreamingAudioPlayerService.cs b/DeepDrftPublic.Client/Services/StreamingAudioPlayerService.cs index 3f1dfca..d3b6439 100644 --- a/DeepDrftPublic.Client/Services/StreamingAudioPlayerService.cs +++ b/DeepDrftPublic.Client/Services/StreamingAudioPlayerService.cs @@ -629,6 +629,15 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS await DrainActiveStreamingTaskAsync(); oldCts?.Dispose(); + // Single-writer discipline (C6/AC8): all three failure exits must share the same guard. + // TrackMediaClient.GetTrackMedia swallows OperationCanceledException and returns + // Success==false, so a superseded seek lands in the media-fetch-fail branch below + // rather than in the OCE catch. Without the guard those branches would call + // RecoverFromFailedRefill — running clearForSeek + setPlaybackOffset against the player + // state the NEWER seek now owns. A local predicate keeps all three exits symmetric so a + // future exit cannot forget the check. + bool IsStillActiveSeek() => ReferenceEquals(_streamingCancellation, seekCts); + try { // Update UI immediately @@ -649,7 +658,15 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS { var technicalError = mediaResult.GetMessage() ?? "Failed to load audio from position"; _logger.LogError("Failed to get track media from offset {Offset}: {Error}", byteOffset, technicalError); - await RecoverFromFailedRefill(seekPosition, StreamingErrorHandler.GetUserFriendlyMessage(technicalError)); + // Guard: a superseded seek must NOT touch shared state. The newer seek owns teardown. + if (IsStillActiveSeek()) + { + await RecoverFromFailedRefill(seekPosition, StreamingErrorHandler.GetUserFriendlyMessage(technicalError)); + } + else + { + _logger.LogDebug("Media-fetch failed on superseded seek to {Position} — newer seek owns state, skipping recovery", seekPosition); + } return; } @@ -660,7 +677,15 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS if (!reinitResult.Success) { _logger.LogError("Failed to reinitialize for offset streaming: {Error}", reinitResult.Error); - await RecoverFromFailedRefill(seekPosition, "Failed to seek to position"); + // Guard: same single-writer discipline — only recover when we are still the active seek. + if (IsStillActiveSeek()) + { + await RecoverFromFailedRefill(seekPosition, "Failed to seek to position"); + } + else + { + _logger.LogDebug("Reinit failed on superseded seek to {Position} — newer seek owns state, skipping recovery", seekPosition); + } return; } @@ -682,7 +707,7 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS // still the active seek — if _streamingCancellation has been replaced, a // newer seek is in progress and owns the flag. _logger.LogDebug("Seek beyond buffer cancelled"); - if (ReferenceEquals(_streamingCancellation, seekCts)) + if (IsStillActiveSeek()) { IsSeekingBeyondBuffer = false; } @@ -694,7 +719,7 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS // fire a silent false end. Only when we are still the active seek — a superseding seek owns // the state and the OCE catch above handles its own teardown. _logger.LogError(ex, "Error during seek beyond buffer to position {Position}", seekPosition); - if (ReferenceEquals(_streamingCancellation, seekCts)) + if (IsStillActiveSeek()) { await RecoverFromFailedRefill(seekPosition, StreamingErrorHandler.GetUserFriendlyMessage(ex.Message)); } diff --git a/DeepDrftPublic/Interop/audio/AudioPlayer.test.ts b/DeepDrftPublic/Interop/audio/AudioPlayer.test.ts index f9931e3..1bf4d15 100644 --- a/DeepDrftPublic/Interop/audio/AudioPlayer.test.ts +++ b/DeepDrftPublic/Interop/audio/AudioPlayer.test.ts @@ -65,18 +65,26 @@ function assertTrue(cond: boolean, msg?: string): void { class FakeScheduler { private offset: number; private total: number; + // hasBuffers reflects whether the scheduler holds decoded audio. Starts true when total > 0 + // (a populated window), set to false by clearForSeek() (recovery drains the buffers). + private _hasBuffers: boolean; public clearedForSeek = false; public stoppedAllSources = false; public offsetSetTo: number | null = null; - constructor(offset: number, total: number) { this.offset = offset; this.total = total; } + constructor(offset: number, total: number) { + this.offset = offset; + this.total = total; + this._hasBuffers = total > 0; + } getPlaybackOffset(): number { return this.offset; } getTotalDuration(): number { return this.total; } + hasBuffers(): boolean { return this._hasBuffers; } stopAllSources(): void { this.stoppedAllSources = true; } // seekWithinBuffer calls playFromPosition only when wasPlaying; isPlaying is false in these // unit constructions, so it is never invoked — present for completeness. playFromPosition(_position: number): void { /* no-op */ } - clearForSeek(): void { this.clearedForSeek = true; } + clearForSeek(): void { this.clearedForSeek = true; this._hasBuffers = false; } setPlaybackOffset(o: number): void { this.offset = o; this.offsetSetTo = o; } } @@ -243,6 +251,43 @@ test('recoverFromFailedRefill halts the scheduler and leaves a paused-but-loaded assertEqual(priv(player).streamingStarted, false, 'streaming flagged not-started for a clean retry'); }); +// --- AC6 retry contract: same-target seek after recovery refetches ------------------------- + +// After recoverFromFailedRefill the scheduler is empty (clearForSeek was called). A seek to +// the SAME position (seekPosition == playbackOffset) must route to seekBeyondBuffer — not +// seekWithinBuffer, which would be a silent no-op against the degenerate [P,P] empty window. +test('same-target seek after recovery routes to seekBeyondBuffer (AC6 retry)', () => { + const player = makePlayer(); + const wav = new FakeStreamDecoder(true, 1000); + // Start with a populated window [30, 60), then simulate recovery at position 15: + // clearForSeek empties the scheduler; setPlaybackOffset anchors it to 15. + const scheduler = new FakeScheduler(30, 30); + arm(player, { scheduler, duration: 120, streamDecoder: wav }); + // Drive recovery state manually (the same state recoverFromFailedRefill leaves). + player.recoverFromFailedRefill(15); + // At this point: scheduler.hasBuffers() == false, playbackOffset == 15, totalDuration == 0. + // A seek to 15 (the recovery anchor) must refetch, not silently resolve from the empty window. + const result = player.seek(15); + assertEqual(result.success, true, 'seek succeeds after recovery'); + assertEqual(result.seekBeyondBuffer, true, 'same-target seek after recovery signals a refetch (AC6 retry)'); + assertEqual(wav.requestedOffsetFor, 15, 'WAV resolver used for the retry offset'); +}); + +// AC4 not regressed: a seek within a POPULATED retained window still resolves from buffer. +// This is the same test as the existing AC4 test but named explicitly to confirm the +// hasBuffers() guard does not affect the populated case. +test('seek within populated retained window still resolves in-buffer — AC4 not regressed', () => { + const player = makePlayer(); + // Populated window [30, 60) — hasBuffers() starts true (total=30 > 0). + const scheduler = new FakeScheduler(30, 30); + arm(player, { scheduler, duration: 120, streamDecoder: new FakeStreamDecoder(true, 1000) }); + + const result = player.seek(45); // inside [30, 60) + assertEqual(result.success, true, 'seek succeeds'); + assertEqual(result.seekBeyondBuffer ?? false, false, 'populated in-window seek does NOT signal a refetch'); + assertEqual(scheduler.clearedForSeek, false, 'scheduler not cleared for an in-buffer seek (no refetch)'); +}); + // --- run ------------------------------------------------------------------------------------- if (failures.length > 0) { console.error(failures.join('\n')); diff --git a/DeepDrftPublic/Interop/audio/AudioPlayer.ts b/DeepDrftPublic/Interop/audio/AudioPlayer.ts index 33ead74..d33572f 100644 --- a/DeepDrftPublic/Interop/audio/AudioPlayer.ts +++ b/DeepDrftPublic/Interop/audio/AudioPlayer.ts @@ -440,21 +440,28 @@ export class AudioPlayer { const bufferEnd = this.scheduler.getTotalDuration() + bufferStart; // The window-miss test for BOTH directions, and the 21.3 refill trigger for backward seeks. - // Position must be within [bufferStart, bufferEnd] to resolve from the retained buffers: - // - position >= bufferStart : UC3 — seek back within the retained back-window. Served from - // buffer with NO network refetch. (The lower bound is load-bearing: after eviction or a - // prior seek-beyond-buffer, bufferStart > 0, and a target below it would otherwise produce - // a negative bufferRelativePosition in seekWithinBuffer, silently clamping to position 0.) + // Position must be within [bufferStart, bufferEnd] AND the scheduler must hold buffers to + // resolve from the retained window: + // - position >= bufferStart AND hasBuffers : UC3 — seek back within the retained back-window. + // Served from buffer with NO network refetch. (The lower bound is load-bearing: after + // eviction or a prior seek-beyond-buffer, bufferStart > 0, and a target below it would + // otherwise produce a negative bufferRelativePosition in seekWithinBuffer, silently clamping + // to position 0.) // - position < bufferStart : UC4 — seek back PAST the retained tail (the window was evicted). // Falls through to seekBeyondBuffer, which is the existing Range path run toward an EARLIER // offset. This is the 21.3 window-miss refill: "a seek the listener didn't initiate" reuses // the same per-path resolver + reinit a forward seek-beyond-buffer uses, no new mechanism. // - position > bufferEnd : UC2/UC5 — forward seek beyond buffer, unchanged. - if (position >= bufferStart && position <= bufferEnd) { + // - !hasBuffers (degenerate [P,P] window post-recovery): the window check above would + // spuriously route ANY target to seekWithinBuffer (bufferStart==bufferEnd==seekPosition + // after recoverFromFailedRefill). Force seekBeyondBuffer so a same-target retry actually + // refetches (AC6 retry contract). The !hasBuffers guard only fires in the degenerate case — + // a populated retained window has buffers and is unaffected (AC4 not regressed). + if (position >= bufferStart && position <= bufferEnd && this.scheduler.hasBuffers()) { return this.seekWithinBuffer(position); } else { - // Seeking outside the retained window - signal C# to fetch a new stream from the resolved - // offset (earlier for a back-past-window refill, later for a forward seek). + // Seeking outside the retained window, or to any position in an empty scheduler — + // signal C# to fetch a new stream from the resolved offset. return this.seekBeyondBuffer(position); } }