Skip to content

Commit 9b43ff0

Browse files
authored
fix(#312): タップ生成失敗時に上限付き再試行し Spectrum 復旧を回復 (#323)
- previous は常に確定しつつ、失敗時は failedAttempts/retrying の再試行バジェットで同一 tick を dedup 通過させ再試行(同一ソース最大3回、以降ソース変化まで諦め) - 失敗を stderr にログ(#318 の慣習) - ユニットテスト2件追加、CLAUDE.md に設計判断を追記、version 2.21.3
1 parent df29e71 commit 9b43ff0

4 files changed

Lines changed: 94 additions & 8 deletions

File tree

.claude/CLAUDE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,8 @@ Presenters subscribe to Interactors via Combine. Interactors access UseCases via
298298

299299
**Spectrum 120 Hz integral-decay retune (#306)**: real-hardware verification of #299 found the leaky-integral term still drained faster in wall-clock time at 120 Hz than at 60 Hz, despite the framerate-independence framework. Root cause: the integral's per-frame decay used `reduction / integralMod` where `integralMod = framerateMod^0.1` — an empirically-tuned cava approximation, not a wall-clock-exact derivation. Replaced with `reduction ^ integralExponent` where `integralExponent = 60 / fps`, which is exactly invariant (`(reduction^(60/fps))^fps == reduction^60` for any fps) and reduces to exponent `1` at 60 fps — the exact pre-#299 hardcoded-60fps decay, with no divisor at all. The gravity/fall term (`gravityScale`) was left unchanged; it was already the well-compensated half of the pair. Empirically confirmed post-fix: heights sampled at the same wall-clock instant now agree to ~1e-6 across 60/120/240 Hz (`SpectrumPresenterTests.decayIsWallClockInvariantAcrossFramerates`). Note the 60 Hz decay value itself shifted slightly (`reduction / 1.1^0.1 ≈ 0.7627` → exactly `reduction = 0.77`, ~1% difference) — an intentional correction, not a regression; visually indistinguishable. **PR review follow-up**: a decay-only fix leaves the integral's *input* term unscaled — a sustained (non-falling) `value` still gets added once per frame with no frame-rate compensation, so at higher fps more additions land per wall-clock second and the sustained steady state (`value / (1 - reduction)`, cava's documented ≈4× at 0.77) overshoots further the higher the refresh rate (120 Hz reached ≈8× instead of ≈4× — worse than the pre-#306 approximation's ≈30% overshoot). `tick()` now precomputes `integralInputScale = (1 - integralDecay) / (1 - reduction)` once per frame (also fixing a Copilot-flagged per-bar `pow` call) and `stepped()` applies it as `mem*integralDecay + value*integralInputScale`, which is the scaling that makes the sustained steady state itself, not just the post-release decay, wall-clock invariant — verified in `SpectrumPresenterTests.sustainedInputIsWallClockInvariantAcrossFramerates`.
300300

301+
**Spectrum capture retry on tap-creation failure (#312)**: `SpectrumInteractorImpl`'s single for-await loop dedupes the now-playing helper's periodic identical ticks via `previous` (a live tap must not be needlessly rebuilt). The bug: `previous` was settled *before* `spectrum.startCapture(pid:)`'s result was known, so a transient tap-creation failure right after an app switch — the new pid not yet in `kAudioHardwarePropertyProcessObjectList` (empty process list → `ProcessTapEngine.init?` returns nil), or HAL resource contention because the old tap's synchronous `stop()` teardown wasn't fully drained by coreaudiod — became **permanent**: every subsequent identical tick was swallowed by `guard source != previous`, so the spectrum stayed dead until a daemon restart. Fix: `previous` is still set to the attempted source, but after a failed start a bounded retry budget (`failedAttempts`, counted per source) deliberately lets the next identical tick back through the `source != previous` dedup and re-attempts — **up to `maxCaptureAttempts` (3) consecutive attempts per source**, then it gives up until the source changes, so a *permanent* denial (pre-14.4 OS, TCC) can't hit CoreAudio / log every tick forever. Retry rides the helper's tick cadence rather than an in-loop backoff sleep on purpose: it keeps the single-consumer serialization intact (a sleep would delay a genuinely-new event) and lets a real source change preempt the retry (a new `AudioSourceState` resets the counter). Each failure is logged to stderr (`lyra: spectrum: startCapture(pid: N) failed (attempt k/3); …`), matching the #318 stderr convention — the interactor/tap chain was previously fully silent. Verified by `SpectrumInteractorImplTests.failedCaptureRetriesOnNextTick` and `.failedCaptureGivesUpAfterBoundedRetries`.
302+
301303
**AI processing indicator (#57)**: While the AI (LLM) extractor resolves title/artist on a cache miss, the header scrambles in a configurable color so the user sees that work is happening. `TrackInteractorImpl.resolveTrack` emits an extra `TrackUpdate(aiResolving: true)` after the debounce only when an `[ai]` endpoint is configured **and** `MetadataUseCase.isAIMetadataCached(track:)` returns `false` (an LLM cache hit means no API round-trip, so no indicator). `HeaderPresenter` maps `aiResolving` to `DecodeEffectState.startLoading` (the indefinite scramble, distinct from `decode`'s settle) and swaps `titleColor` / `artistColor` to `DecodeEffect.processingColor` (default green `#4ADE80FF`, config key `text.decode_effect.processing_color`, solid or gradient). The resolved (non-`aiResolving`) update settles the scramble and restores the normal color. `HeaderView` reads the effective `titleColor` / `artistColor` (`@Published`) rather than the static `titleStyle.color`.
302304

303305
**Confidence-based metadata+lyrics resolution (#308)**: `MetadataRepositoryImpl.resolve()` no longer short-circuits when the LLM cache/DataSource succeeds — LLM, MusicBrainz, and Regex are always all queried (cache-or-datasource per source) and merged (`llmCandidates + mbCandidates + regexCandidates + [track]`), so a bad LLM guess can no longer permanently starve the other sources from ever being tried. `LyricsRepositoryImpl.fetchLyrics(candidates:)` tries three tiers across *all* candidates before giving up — Tier A (`LyricsDataSource.get()`, LRCLIB's own exact match, trusted as-is), Tier B (`LyricsDataSource.search()` fuzzy match, now gated by the new `LyricsMatchValidator` — title similarity via normalized Levenshtein distance, plus duration tolerance when both sides have it), and Tier C (`customScriptLyricsDataSource`, a second `LyricsDataSource` DI key backed by `CustomScriptLyricsDataSourceImpl`, which shells out to a user-configured `[lyrics] fallback_command` argv array with a timeout, expanding `$LYRA_CONFIG_DIR`/`$LYRA_CACHE_DIR` placeholders (both `$VAR` and `${VAR}` forms; literal substitution, not shell interpolation) in every argv element *before* the absolute-executable guard so configs stay machine-portable, mirroring `YouTubeWallpaperDataSourceImpl`'s test-injectable `processRunner` pattern). Whichever tier validates first stores the result under *the matched candidate's* title/artist (fixing a latent bug where the cache was always keyed by `candidates.first` regardless of which candidate actually matched); `LyricsResult.trackName`/`artistName`/`withDisplay()` already carried enough shape to make the confirmed candidate's identity part of the cached lyrics entry itself, so no separate cross-repository "joint commit" coordinator was needed. When no tier validates, nothing is cached and `TrackInteractorImpl.resolveTrack(from:)` falls back to the raw (unprocessed) title/artist rather than an unvalidated candidate guess — a targeted 2-line fix, not a new orchestration layer. The same unvalidated-candidate-guess bug was independently found in `TrackHandlerImpl.infoWithLyrics` (the CLI `lyra track -l` path), which fell back to `candidates.first`'s title/artist on a lyrics miss instead of the raw `track.title`/`track.artist`; fixed identically. The cache **read** side of `LyricsRepositoryImpl.fetchLyrics(candidates:)` had the mirror bug — it checked only `candidates.first`'s cache key, so an entry written under a later-matched candidate (per the write-side fix above) was unreachable on a subsequent read; the read path now loops over every candidate's cache key before falling through to Tier A/B/C. **Review round 2 (cache coherence)**: (a) the MusicBrainz cache stores **all** candidate recordings per query, not just the first — `MetadataDataStore<[MusicBrainzMetadata]>`, with the v5 migration rebuilding `musicbrainz_cache` without its UNIQUE(query_title, query_artist) constraint (one row per candidate; write = delete-then-insert) — because a cache hit that truncated the candidate set made lyrics cached under a later candidate unreachable on replays; (b) candidate dedup keys on a `Hashable` (title, artist, duration) struct — duration participates because LRCLIB exact match and `LyricsMatchValidator` discriminate on it, and the struct removes the `\u{1}`-sentinel collision; (c) the lyrics cache **re-validates entries on read** so pre-#308 poisoned rows can't short-circuit the tiers (invalid entries are skipped and later overwritten by a validated match); (d) `displayAdjusted` fills title and artist independently (a Tier C script may omit `artist_name` — the matched candidate's artist is used, not the raw fallback); (e) `timeout_ms` clamps to a finite 1 ms…1 h window before `Int` conversion (`Int(Double)` traps on NaN/±inf/overflow); (f) `[ai]`/`[lyrics]` decode leniently at runtime but `ConfigDataSourceImpl.tryDecode()` (the healthcheck path) probes both sections strictly, so a malformed section fails validation instead of silently disabling the feature. **Live-device verification fix**: Tier C results were never cached — `GRDBLyricsDataStore.write` guarded on `result.id != nil`, and script results carry no LRCLIB id, so every replay of a script-resolved track silently re-paid the full cascade (all LRCLIB lookups + the script run). Id-less lyric-bearing results are now stored under a stable FNV-1a-derived **negative** synthetic id (genuine LRCLIB ids are positive, so the namespaces can never collide; the stable hash makes rewrites converge on one row), while id-less results with no lyrics content remain unstored.

Sources/SpectrumInteractor/SpectrumInteractorImpl.swift

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,21 +64,51 @@ extension SpectrumInteractorImpl: SpectrumInteractor {
6464
// A single for-await loop is the serialization point: events apply
6565
// one at a time, so a rapid pause→play burst can never interleave
6666
// capture start/stop calls. `previous` dedupes the helper's periodic
67-
// ticks — restarting the capture rebuilds the tap, so repeats must
68-
// not pass.
67+
// ticks — restarting a live capture rebuilds the tap, so repeats must
68+
// not pass. A failed tap creation is the exception: `previous` is still
69+
// set to the attempted source, but `failedAttempts` tracks consecutive
70+
// failures for it and the `retrying` budget deliberately lets an
71+
// identical repeat back through the dedup, so the next tick re-attempts
72+
// — up to `maxCaptureAttempts` per source, then it gives up until the
73+
// source changes. That recovers the transient app-switch race (empty
74+
// process list / HAL contention) without spinning forever on a
75+
// permanent denial (pre-14.4 OS, TCC) (#312).
6976
let candidate = Task {
77+
let maxCaptureAttempts = 3
7078
var previous: AudioSourceState?
79+
var failedAttempts = 0
7180
for await info in playback.observeNowPlaying() {
7281
let source = AudioSourceState(
7382
pid: info?.pid, isPlaying: (info?.playbackRate ?? 0) > 0)
74-
guard source != previous else { continue }
83+
let isNewSource = source != previous
84+
let retrying =
85+
!isNewSource
86+
&& (1..<maxCaptureAttempts).contains(failedAttempts)
87+
guard isNewSource || retrying else { continue }
88+
// A new source starts its own retry budget; a retry keeps the
89+
// running count. A stop is always a new source, so it resets
90+
// here too.
91+
failedAttempts = isNewSource ? 0 : failedAttempts
7592
previous = source
7693
guard let pid = source.pid, source.isPlaying else {
7794
await spectrum.stopCapture()
7895
subject.send(false)
7996
continue
8097
}
81-
subject.send(await spectrum.startCapture(pid: pid))
98+
let started = await spectrum.startCapture(pid: pid)
99+
subject.send(started)
100+
failedAttempts = started ? 0 : failedAttempts + 1
101+
guard started else {
102+
let giveUp = failedAttempts >= maxCaptureAttempts
103+
fputs(
104+
"lyra: spectrum: startCapture(pid: \(pid)) failed "
105+
+ "(attempt \(failedAttempts)/\(maxCaptureAttempts)); "
106+
+ (giveUp
107+
? "giving up until the source changes\n"
108+
: "retrying on next now-playing tick\n"),
109+
stderr)
110+
continue
111+
}
82112
}
83113
// Upstream finished (helper EOF): tear the capture down. A
84114
// cancelled task skips this — stop() owns that teardown.
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
2.21.2
1+
2.21.3

Tests/SpectrumInteractorTests/SpectrumInteractorImplTests.swift

Lines changed: 57 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,46 @@ struct SpectrumInteractorImplTests {
117117
#expect(harness.spectrum.startedPids == [4242])
118118
}
119119

120+
@Test("a failed capture is retried on the next identical now-playing tick (#312)")
121+
func failedCaptureRetriesOnNextTick() async {
122+
let harness = Harness()
123+
// First tap creation fails (transient app-switch race), the retry
124+
// succeeds. The failure leaves the retry budget open, so an identical
125+
// repeat is let back through the dedup instead of being swallowed.
126+
harness.spectrum.startResults = [false, true]
127+
harness.interactor.start()
128+
harness.send(pid: 4242, playbackRate: 1)
129+
// The helper re-emits the same pid+playing state on its periodic tick.
130+
// Pre-fix, a repeat was unconditionally swallowed by the dedup, so a
131+
// failed start could never recover until a daemon restart; the retry
132+
// budget now lets this identical event back through to re-attempt.
133+
harness.send(pid: 4242, playbackRate: 1)
134+
135+
await harness.pollUntil { harness.spectrum.startedPids == [4242, 4242] }
136+
#expect(harness.spectrum.startedPids == [4242, 4242])
137+
await harness.pollUntil { harness.capturing.value == true }
138+
#expect(harness.capturing.value == true)
139+
}
140+
141+
@Test("retries are bounded so a permanent failure does not spin forever (#312)")
142+
func failedCaptureGivesUpAfterBoundedRetries() async {
143+
let harness = Harness()
144+
// A permanent failure (old OS / TCC denial) never succeeds.
145+
harness.spectrum.startResults = Array(repeating: false, count: 8)
146+
harness.interactor.start()
147+
// Five identical ticks, but the source is retried at most 3 times
148+
// (maxCaptureAttempts) before the dedup gives up until the source
149+
// changes — so a denied capture cannot log/hit CoreAudio every tick.
150+
for _ in 0..<5 { harness.send(pid: 4242, playbackRate: 1) }
151+
// The pause is a new source: it lands strictly after the five identical
152+
// ticks were evaluated and marks the end of the retry window.
153+
harness.send(pid: 4242, playbackRate: 0)
154+
155+
await harness.pollUntil { harness.spectrum.stopCount > 0 }
156+
#expect(harness.spectrum.startedPids == [4242, 4242, 4242])
157+
#expect(harness.capturing.value == false)
158+
}
159+
120160
@Test("second start() while running never spawns a competing processor")
121161
func startTwiceKeepsSingleProcessor() async {
122162
let harness = Harness()
@@ -223,17 +263,31 @@ private final class CurrentValueBox: @unchecked Sendable {
223263

224264
private final class FakeSpectrumUseCase: SpectrumUseCase, @unchecked Sendable {
225265
private let state = OSAllocatedUnfairLock(
226-
initialState: (started: [Int](), stops: 0, style: SpectrumStyle?.none, bars: Int?.none))
266+
initialState: (
267+
started: [Int](), stops: 0, style: SpectrumStyle?.none, bars: Int?.none,
268+
results: [Bool]()
269+
))
227270
var magnitudesResult: [Float] = []
228271

229272
var startedPids: [Int] { state.withLock { $0.started } }
230273
var stopCount: Int { state.withLock { $0.stops } }
231274
var lastStyle: SpectrumStyle? { state.withLock { $0.style } }
232275
var lastBarCount: Int? { state.withLock { $0.bars } }
233276

277+
/// Scripted return values for successive `startCapture` calls, consumed in
278+
/// order; once exhausted the capture succeeds. Default empty → always
279+
/// succeeds, keeping the happy-path tests unchanged.
280+
var startResults: [Bool] {
281+
get { state.withLock { $0.results } }
282+
set { state.withLock { $0.results = newValue } }
283+
}
284+
234285
func startCapture(pid: Int) async -> Bool {
235-
state.withLock { $0.started.append(pid) }
236-
return true
286+
state.withLock {
287+
$0.started.append(pid)
288+
guard !$0.results.isEmpty else { return true }
289+
return $0.results.removeFirst()
290+
}
237291
}
238292

239293
func stopCapture() async {

0 commit comments

Comments
 (0)