From 01641ed8be2fd186806defe3705d94087971a01c Mon Sep 17 00:00:00 2001 From: randrini Date: Wed, 7 Oct 2026 07:17:22 +0200 Subject: [PATCH] fix(playback): bound stale-pin recovery re-list with the shared damper The stale-pin recovery re-lists the provider directly to bypass the provider floor, but did so outside virtualRecoveryRelists, the damper that bounds the sibling fallback and the other floor-bypassing recovery resolves. A burst of dead-pin cold starts could therefore keep a failing provider hot without the shared backpressure. Route the recovery's re-list through virtualRecoveryRelists with the fallback's acquire/accounting semantics: acquire before listing, clear once a listing answers with candidates, and let an empty or failed answer keep accumulating toward the bound. An exhausted budget preserves the caller's terminal so the version-fallback walk proceeds to its alternates, matching the sibling fallback's deterministic yield. The 500ms recovery sub-budget, single-retry bound, transient-only semantics, session-bound refusal, and fingerprint matching are unchanged. Add a dedicated test proving the damper engages on the recovery path (budget exhausted -> recovery skips the listing with no side effects, no sleeps) plus one pinning the clear-on-answer accounting. Refs #237 --- .../handlers/playback_virtual_stale_pin.go | 23 +++++- .../playback_virtual_stale_pin_test.go | 76 +++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/internal/api/handlers/playback_virtual_stale_pin.go b/internal/api/handlers/playback_virtual_stale_pin.go index dbaf9779cf..9087a03cdf 100644 --- a/internal/api/handlers/playback_virtual_stale_pin.go +++ b/internal/api/handlers/playback_virtual_stale_pin.go @@ -162,6 +162,20 @@ func (h *PlaybackHandler) recoverStaleIdentityLessPinV3( // the generation order the start order, and every write below carries this // token. generation := nextVirtualCacheGeneration() + // The recovery is a declared re-list: it bypasses the provider floor, so it + // must draw on the same per-listing budget as the sibling fallback and the + // other floor-bypassing recovery resolves. Without this bound a burst of + // dead-pin cold starts would re-list a failing provider without the shared + // backpressure the rest of recovery honors. Acquire before listing; a + // listing that answers with candidates clears the budget below, matching the + // fallback's accounting. An exhausted budget preserves the caller's terminal + // so the walk falls through to the same alternate rows it would otherwise. + recoveryKey := virtualRecoveryRelistKey(neutralKey, file.VirtualOwnerInstallationID) + if !virtualRecoveryRelists.allow(recoveryKey) { + slog.WarnContext(ctx, "virtual stale-pin recovery re-list budget exhausted; preserving the terminal", + "component", "api", "file_id", file.ID, "neutral_key", neutralKey) + return resolvedVirtualPlaybackSource{}, false + } // Bound the whole recovery well under the walk's decision budget so the // alternates the walk still has to try keep their time. recoveryCtx, cancel := context.WithTimeout(ctx, virtualStalePinRecoveryBudget) @@ -171,9 +185,16 @@ func (h *PlaybackHandler) recoverStaleIdentityLessPinV3( ) if err != nil || len(streams) == 0 { // An empty (or failed) listing is a transient outage, not a renumber. - // Preserve the caller's terminal. + // Preserve the caller's terminal. It deliberately does not clear the + // budget, so a provider that keeps answering [] still accumulates + // toward the bound instead of being re-listed on every press. return resolvedVirtualPlaybackSource{}, false } + // The provider answered with candidates, so this re-list is no longer + // defeating a provider fail-fast: clear the budget so a later failure starts + // from a full window. Cleared before the downstream gates below, exactly as + // the sibling fallback clears once a listing answers. + virtualRecoveryRelists.clear(recoveryKey) // The pin must be absent from the live listing. If it is still listed the // ordinary resolve owns it and a recovery would only mask a different fault. for _, stream := range streams { diff --git a/internal/api/handlers/playback_virtual_stale_pin_test.go b/internal/api/handlers/playback_virtual_stale_pin_test.go index 04d4bdc1fe..c63a906340 100644 --- a/internal/api/handlers/playback_virtual_stale_pin_test.go +++ b/internal/api/handlers/playback_virtual_stale_pin_test.go @@ -1038,3 +1038,79 @@ func TestStalePinRecoveryPersistsOnlyAgainstExistingOwner(t *testing.T) { } }) } + +// TestStalePinRecoveryReListHonorsSharedDamper proves the recovery's re-list is +// bounded by the same per-provider budget the sibling fallback uses: once the +// damper is exhausted the recovery yields deterministically instead of listing, +// with no sleep and no side effects. It also proves the recovery is what spends +// that budget, so a burst of dead-pin starts cannot re-list a failing provider +// without the shared backpressure. +func TestStalePinRecoveryReListHonorsSharedDamper(t *testing.T) { + resetVirtualRecoveryRelists(t) + const neutral = "virtual://movie/tt-stale-damper" + row := stalePinRow(25, "movie-stale-damper", neutral, "STALE") + live := []VirtualPlaybackStream{stalePinLiveStream(neutral, "NEW", row.FileSize, "h264")} + + // A full budget admits the recovery once and spends exactly one re-list. + // A successful listing clears the key, so use a first recovery that answers + // empty to keep the spent mark, then a second recovery must be denied. + fxEmpty := newStalePinRecoveryFixture(neutral+"?result=STALE", stalePinRecoveryHandlerOpts{probeDuration: row.Duration}) + for i := 0; i < virtualRecoveryRelistMax; i++ { + if _, ok := fxEmpty.handler.recoverStaleIdentityLessPinV3(stalePinRecoveryRequest(), row, "profile-1"); ok { + t.Fatalf("recovery %d answered an empty listing but reported success", i+1) + } + } + if got := fxEmpty.listCalls.Load(); got != int32(virtualRecoveryRelistMax) { + t.Fatalf("listings = %d, want %d: an empty answer must accumulate toward the damper", got, virtualRecoveryRelistMax) + } + + // The budget is now exhausted. A fingerprint-matched recovery must not even + // list: the damper is what stops it, deterministically and without sleeping. + fx := newStalePinRecoveryFixture(neutral+"?result=STALE", stalePinRecoveryHandlerOpts{ + live: live, + probeDuration: row.Duration, + }) + recovered, ok := fx.handler.recoverStaleIdentityLessPinV3(stalePinRecoveryRequest(), row, "profile-1") + if ok { + t.Fatalf("recovery = %#v, want the terminal preserved once the shared damper is exhausted", recovered) + } + if got := fx.listCalls.Load(); got != 0 { + t.Fatalf("listings = %d, want 0: an exhausted budget must not re-list the provider", got) + } + if got := fx.resolveCalls.Load(); got != 0 { + t.Fatalf("resolves = %d, want 0 after a denied recovery re-list", got) + } + if got := fx.handler.peekVirtualSticky(fx.stickyKey(row)); got != "" { + t.Fatalf("sticky pin = %q, want none after a denied recovery re-list", got) + } + if got := virtualResultCandidateID(row.FilePath); got != "STALE" { + t.Fatalf("catalog row pin = %q, want the durable STALE unchanged", got) + } +} + +// TestStalePinRecoveryReListClearsBudgetOnAnswer proves the recovery uses the +// fallback's accounting exactly: a listing that answers with candidates is no +// longer defeating a provider fail-fast, so the recovery clears the key and a +// later failure starts from a full budget. A listing that answers empty +// deliberately keeps the mark, which the damper test above exercises. +func TestStalePinRecoveryReListClearsBudgetOnAnswer(t *testing.T) { + resetVirtualRecoveryRelists(t) + const neutral = "virtual://movie/tt-stale-clear-budget" + row := stalePinRow(26, "movie-stale-clear-budget", neutral, "STALE") + live := []VirtualPlaybackStream{stalePinLiveStream(neutral, "NEW", row.FileSize, "h264")} + fx := newStalePinRecoveryFixture(neutral+"?result=STALE", stalePinRecoveryHandlerOpts{ + live: live, + probeDuration: row.Duration, + }) + + if _, ok := fx.handler.recoverStaleIdentityLessPinV3(stalePinRecoveryRequest(), row, "profile-1"); !ok { + t.Fatal("the unique fingerprint match should recover") + } + recoveryKey := virtualRecoveryRelistKey(neutral, row.VirtualOwnerInstallationID) + virtualRecoveryRelists.mu.Lock() + _, marked := virtualRecoveryRelists.marks[recoveryKey] + virtualRecoveryRelists.mu.Unlock() + if marked { + t.Fatal("an answered recovery listing must clear the key so a later failure starts from a full budget") + } +}