Skip to content

fix(playback): bound stale-pin recovery re-list with the shared damper - #238

Merged
randrini merged 1 commit into
mainfrom
fix/stale-pin-relist-damper
Oct 7, 2026
Merged

randrini merged 1 commit into
mainfrom
fix/stale-pin-relist-damper

Conversation

@randrini

@randrini randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Problem

Closes #237
Follow-up from PR #234 (merged). The stale-pin recovery re-list ran outside the virtualRecoveryRelists damper bounding the sibling fallback.

Approach

Route the recovery re-list through the shared damper with identical acquire/accounting semantics: exhausted budget yields deterministically to alternates (terminal preserved); answered listings clear the key; empty/failed answers accumulate. 500ms sub-budget, single-retry bound, transient-only semantics, session-bound refusal, and fingerprint matching unchanged.

Validation

  • New damper tests: exhausted budget yields with zero resolve/pin (no sleeps); answered listing clears the key
  • Full handlers suite green incl. -race; gofmt/vet clean
  • Full CI green on the final head required

Risks

Under damper exhaustion the recovery skips its re-list where it previously attempted one — same deterministic yield as the sibling fallback. No API, migration, or config impact.

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

AI Disclosure

  • Harness: OpenCode
  • Tool(s): OpenCode (oracle, fixer subagents)
  • Model(s): opencode-go/muse-spark-1.3-contributor
  • Involvement: AI-assisted

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
@randrini
randrini requested a review from drondeseries October 7, 2026 10:49
@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Ready for review: damper-bound recovery re-list with shared accounting, full suites + -race green. Awaiting dev verdict + CI confirmation — no merge until approved.

@drondeseries

Copy link
Copy Markdown
Collaborator

Review — APPROVE

Small, safe, one-concern fix. Verified against head 01641ed8:

  • Shared damper key matches the sibling paths: virtualRecoveryRelistKey(neutralKey, ownerID) with neutralKey = virtualPlaybackNeutralKey(file.FilePath) — same identity as resolveVirtualPlaybackSource (:2008) and the stale fallback (:4972). Same budget, same window.
  • Acquire-before-list / clear-on-answer / accumulate-on-empty matches fallback (playback_virtual.go:5006) and resolver (:2194). Clear placement before the pin-absent/fingerprint/resolve gates is correct — a listing that answered is no longer defeating fail-fast.
  • Exhausted budget returns {}, false with WarnContext, terminal preserved, no sticky/durable side effects (pinned by the new test).

Tests: TestStalePinRecoveryReListHonorsSharedDamper (exhausted → zero listings/resolves/sticky writes, durable pin unchanged) and TestStalePinRecoveryReListClearsBudgetOnAnswer cover the accounting both directions. All 8 CI checks green.

Non-blocking nits:

  • Generation is allocated before the damper check here but after it in resolveVirtualPlaybackSource (:2023). A denied recovery burns one generation number — harmless, just inconsistent.

Safe to merge as-is.

@randrini
randrini merged commit 2b87fa2 into main Oct 7, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(playback): bound stale-pin recovery re-list with virtualRecoveryRelists damper

2 participants