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.
This commit is contained in:
@@ -118,6 +118,16 @@ public abstract class AudioPlayerService : IPlayerService, IAsyncDisposable
|
|||||||
IsPlaying = true;
|
IsPlaying = true;
|
||||||
IsPaused = false;
|
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)
|
if (!result.Success)
|
||||||
@@ -128,7 +138,7 @@ public abstract class AudioPlayerService : IPlayerService, IAsyncDisposable
|
|||||||
{
|
{
|
||||||
ErrorMessage = null;
|
ErrorMessage = null;
|
||||||
}
|
}
|
||||||
|
|
||||||
await NotifyStateChanged();
|
await NotifyStateChanged();
|
||||||
}
|
}
|
||||||
catch (Exception ex)
|
catch (Exception ex)
|
||||||
|
|||||||
@@ -629,6 +629,15 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS
|
|||||||
await DrainActiveStreamingTaskAsync();
|
await DrainActiveStreamingTaskAsync();
|
||||||
oldCts?.Dispose();
|
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
|
try
|
||||||
{
|
{
|
||||||
// Update UI immediately
|
// Update UI immediately
|
||||||
@@ -649,7 +658,15 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS
|
|||||||
{
|
{
|
||||||
var technicalError = mediaResult.GetMessage() ?? "Failed to load audio from position";
|
var technicalError = mediaResult.GetMessage() ?? "Failed to load audio from position";
|
||||||
_logger.LogError("Failed to get track media from offset {Offset}: {Error}", byteOffset, technicalError);
|
_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;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -660,7 +677,15 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS
|
|||||||
if (!reinitResult.Success)
|
if (!reinitResult.Success)
|
||||||
{
|
{
|
||||||
_logger.LogError("Failed to reinitialize for offset streaming: {Error}", reinitResult.Error);
|
_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;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -682,7 +707,7 @@ public class StreamingAudioPlayerService : AudioPlayerService, IStreamingPlayerS
|
|||||||
// still the active seek — if _streamingCancellation has been replaced, a
|
// still the active seek — if _streamingCancellation has been replaced, a
|
||||||
// newer seek is in progress and owns the flag.
|
// newer seek is in progress and owns the flag.
|
||||||
_logger.LogDebug("Seek beyond buffer cancelled");
|
_logger.LogDebug("Seek beyond buffer cancelled");
|
||||||
if (ReferenceEquals(_streamingCancellation, seekCts))
|
if (IsStillActiveSeek())
|
||||||
{
|
{
|
||||||
IsSeekingBeyondBuffer = false;
|
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
|
// 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.
|
// the state and the OCE catch above handles its own teardown.
|
||||||
_logger.LogError(ex, "Error during seek beyond buffer to position {Position}", seekPosition);
|
_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));
|
await RecoverFromFailedRefill(seekPosition, StreamingErrorHandler.GetUserFriendlyMessage(ex.Message));
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -65,18 +65,26 @@ function assertTrue(cond: boolean, msg?: string): void {
|
|||||||
class FakeScheduler {
|
class FakeScheduler {
|
||||||
private offset: number;
|
private offset: number;
|
||||||
private total: 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 clearedForSeek = false;
|
||||||
public stoppedAllSources = false;
|
public stoppedAllSources = false;
|
||||||
public offsetSetTo: number | null = null;
|
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; }
|
getPlaybackOffset(): number { return this.offset; }
|
||||||
getTotalDuration(): number { return this.total; }
|
getTotalDuration(): number { return this.total; }
|
||||||
|
hasBuffers(): boolean { return this._hasBuffers; }
|
||||||
stopAllSources(): void { this.stoppedAllSources = true; }
|
stopAllSources(): void { this.stoppedAllSources = true; }
|
||||||
// seekWithinBuffer calls playFromPosition only when wasPlaying; isPlaying is false in these
|
// seekWithinBuffer calls playFromPosition only when wasPlaying; isPlaying is false in these
|
||||||
// unit constructions, so it is never invoked — present for completeness.
|
// unit constructions, so it is never invoked — present for completeness.
|
||||||
playFromPosition(_position: number): void { /* no-op */ }
|
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; }
|
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');
|
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 -------------------------------------------------------------------------------------
|
// --- run -------------------------------------------------------------------------------------
|
||||||
if (failures.length > 0) {
|
if (failures.length > 0) {
|
||||||
console.error(failures.join('\n'));
|
console.error(failures.join('\n'));
|
||||||
|
|||||||
@@ -440,21 +440,28 @@ export class AudioPlayer {
|
|||||||
const bufferEnd = this.scheduler.getTotalDuration() + bufferStart;
|
const bufferEnd = this.scheduler.getTotalDuration() + bufferStart;
|
||||||
|
|
||||||
// The window-miss test for BOTH directions, and the 21.3 refill trigger for backward seeks.
|
// 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 must be within [bufferStart, bufferEnd] AND the scheduler must hold buffers to
|
||||||
// - position >= bufferStart : UC3 — seek back within the retained back-window. Served from
|
// resolve from the retained window:
|
||||||
// buffer with NO network refetch. (The lower bound is load-bearing: after eviction or a
|
// - position >= bufferStart AND hasBuffers : UC3 — seek back within the retained back-window.
|
||||||
// prior seek-beyond-buffer, bufferStart > 0, and a target below it would otherwise produce
|
// Served from buffer with NO network refetch. (The lower bound is load-bearing: after
|
||||||
// a negative bufferRelativePosition in seekWithinBuffer, silently clamping to position 0.)
|
// 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).
|
// - 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
|
// 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
|
// 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.
|
// the same per-path resolver + reinit a forward seek-beyond-buffer uses, no new mechanism.
|
||||||
// - position > bufferEnd : UC2/UC5 — forward seek beyond buffer, unchanged.
|
// - 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);
|
return this.seekWithinBuffer(position);
|
||||||
} else {
|
} else {
|
||||||
// Seeking outside the retained window - signal C# to fetch a new stream from the resolved
|
// Seeking outside the retained window, or to any position in an empty scheduler —
|
||||||
// offset (earlier for a back-past-window refill, later for a forward seek).
|
// signal C# to fetch a new stream from the resolved offset.
|
||||||
return this.seekBeyondBuffer(position);
|
return this.seekBeyondBuffer(position);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user