From 5aa99674e9d88d802cd708df0d448df2edbf4772 Mon Sep 17 00:00:00 2001 From: Christian Pillsbury Date: Mon, 22 Jun 2026 06:44:36 -0700 Subject: [PATCH] refactor(spf): drop the vestigial reschedule retry-on-error path The RecurringRunner propagates a rejected run as the recurrence's failure (Promise.all short-circuits before any retry verdict lands), so the retry-on-error affordance in delayedReschedule and mediaPlaylistReloadDelay was dead code. Remove it: delayedReschedule awaits the run directly (a rejection now rejects the reschedule), and mediaPlaylistReloadDelay takes a non-optional current track. Transient-fetch-failure recovery belongs at the fetch layer, not in the cadence. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../spf/src/core/tasks/delayed-reschedule.ts | 16 ++++------- .../tasks/tests/delayed-reschedule.test.ts | 28 +++++++------------ packages/spf/src/media/hls/reload-policy.ts | 20 ++++++------- .../src/media/hls/tests/reload-policy.test.ts | 9 ++---- 4 files changed, 26 insertions(+), 47 deletions(-) diff --git a/packages/spf/src/core/tasks/delayed-reschedule.ts b/packages/spf/src/core/tasks/delayed-reschedule.ts index 41cc1eca..0560fe01 100644 --- a/packages/spf/src/core/tasks/delayed-reschedule.ts +++ b/packages/spf/src/core/tasks/delayed-reschedule.ts @@ -12,21 +12,17 @@ import type { Reschedule } from './task'; * RFC 8216 §6.3.4's "measured from the last time the client began loading"). If * the run takes longer than the cadence, the next run starts immediately. * - * A `null` cadence stops the recurrence. An errored run passes `current` as - * `undefined`, so the cadence function can choose to retry (return a delay) or - * stop (return `null`). + * A `null` cadence stops the recurrence. A rejected run rejects this reschedule, + * which the `RecurringRunner` propagates as the recurrence's failure — error + * recovery (e.g. retrying transient fetch failures) belongs below, at the fetch + * layer, not in the cadence. */ export function delayedReschedule( - cadence: (current: TValue | undefined, previous: TValue | undefined) => number | null + cadence: (current: TValue, previous: TValue | undefined) => number | null ): Reschedule { return async (task) => { const startedAt = Date.now(); - let current: TValue | undefined; - try { - current = await task.run(); - } catch { - current = undefined; - } + const current = await task.run(); // `task.previous` is the prior successful value (carried by the runner's // clone); read-only for the cadence, hence the cast off `DeepReadonly`. const ms = cadence(current, task.previous as TValue | undefined); diff --git a/packages/spf/src/core/tasks/tests/delayed-reschedule.test.ts b/packages/spf/src/core/tasks/tests/delayed-reschedule.test.ts index a6493c01..cbc62062 100644 --- a/packages/spf/src/core/tasks/tests/delayed-reschedule.test.ts +++ b/packages/spf/src/core/tasks/tests/delayed-reschedule.test.ts @@ -82,26 +82,18 @@ describe('delayedReschedule', () => { await expect(reschedule(fakeTask(async () => 1))).resolves.toBe(false); }); - it('passes undefined to the cadence when the run errors (so it can retry)', async () => { - vi.useFakeTimers(); - const cadence = vi.fn(() => 50); // retry on error + it('rejects (propagating the run failure) without consulting the cadence', async () => { + const cadence = vi.fn(() => 50); const reschedule = delayedReschedule(cadence); - let resolved: boolean | undefined; - const done = reschedule( - fakeTask(async () => { - throw new Error('boom'); - }) - ).then((v) => { - resolved = v; - }); - - await vi.advanceTimersByTimeAsync(0); - expect(cadence).toHaveBeenCalledWith(undefined, undefined); - - await vi.advanceTimersByTimeAsync(50); - await done; - expect(resolved).toBe(true); + await expect( + reschedule( + fakeTask(async () => { + throw new Error('boom'); + }) + ) + ).rejects.toThrow('boom'); + expect(cadence).not.toHaveBeenCalled(); }); it('rejects when aborted during the wait', async () => { diff --git a/packages/spf/src/media/hls/reload-policy.ts b/packages/spf/src/media/hls/reload-policy.ts index b5bb32ce..d5785fa4 100644 --- a/packages/spf/src/media/hls/reload-policy.ts +++ b/packages/spf/src/media/hls/reload-policy.ts @@ -8,34 +8,30 @@ function snapshotSignature(track: ResolvedTrack): string { return `${getMediaPlaylistMetadata(track)?.mediaSequence ?? 0}:${track.segments.length}`; } -function targetDurationOf(track: ResolvedTrack | undefined): number { - return (track && getMediaPlaylistMetadata(track)?.targetDuration) || FALLBACK_TARGET_DURATION; +function targetDurationOf(track: ResolvedTrack): number { + return getMediaPlaylistMetadata(track)?.targetDuration || FALLBACK_TARGET_DURATION; } /** * Live media-playlist reload cadence, per RFC 8216bis §6.3.4 — a * {@link RecurrencePolicy} for a `RecurringRunner` re-resolving the selected * track. Structurally matches `RecurrencePolicy` without - * importing it (media stays core-free): `current` is the freshly resolved track - * (`undefined` if the reload errored), `previous` the prior resolved snapshot. + * importing it (media stays core-free): `current` is the freshly resolved track, + * `previous` the prior resolved snapshot. * * - Complete playlist (VoD, or live that hit `#EXT-X-ENDLIST`) → `null`: stop. * Keys off `Track.duration` (finite once complete), the single completeness * source of truth. - * - Errored reload (`current` undefined) → retry at the last-known target- - * duration cadence (fallback when unknown), keeping the loop alive across - * transient fetch failures. * - Unchanged window (same media sequence + segment count as `previous`) → poll * at half the target duration; a moved/grown window (or the first reload) → * full target duration. * + * A failed reload doesn't reach here — the rejection propagates through the + * `RecurringRunner`; transient-failure recovery belongs at the fetch layer. + * * Returned delays are milliseconds. */ -export function mediaPlaylistReloadDelay( - current: ResolvedTrack | undefined, - previous: ResolvedTrack | undefined -): number | null { - if (!current) return targetDurationOf(previous) * 1000; +export function mediaPlaylistReloadDelay(current: ResolvedTrack, previous: ResolvedTrack | undefined): number | null { if (Number.isFinite(current.duration)) return null; const target = targetDurationOf(current); diff --git a/packages/spf/src/media/hls/tests/reload-policy.test.ts b/packages/spf/src/media/hls/tests/reload-policy.test.ts index d87dca87..5019ae68 100644 --- a/packages/spf/src/media/hls/tests/reload-policy.test.ts +++ b/packages/spf/src/media/hls/tests/reload-policy.test.ts @@ -43,12 +43,7 @@ describe('mediaPlaylistReloadDelay', () => { expect(mediaPlaylistReloadDelay(slid, prev)).toBe(4000); }); - it('retries at the last-known cadence when a reload errors (current undefined)', () => { - const prev = track({ targetDuration: 8 }); - expect(mediaPlaylistReloadDelay(undefined, prev)).toBe(8000); - }); - - it('falls back to 6s when no target duration / prior snapshot is available', () => { - expect(mediaPlaylistReloadDelay(undefined, undefined)).toBe(6000); + it('falls back to 6s when the playlist carries no usable target duration', () => { + expect(mediaPlaylistReloadDelay(track({ targetDuration: 0 }), undefined)).toBe(6000); }); });