fix(tts): warn on the probe's failing edge, not on every cycle after it (gh-#338) - #342
Merged
Merged
Conversation
…it (gh-#338) On the gh-#213 Pi 5, kokoro is profile-disabled by the piper-only overlay, so its probe can never succeed. The api logged this every 30s, forever: warn: kokoro health probe failed (Name or service not known (kokoro:8880)) -- 29 consecutive failures, cached verdict is now unhealthy [full stack trace] The verdict flipped at failure 2. Failure 29 changed nothing, and the wording claimed a transition each time. At the F70.2 default cadence that is ~2,880 warnings a day carrying an identical stack trace, plus 5,760 info lines from the typed client -- ~8,600 stack traces a day for a condition reported once and permanent by construction. RecordFailure's own doc comment already stated the intended contract: "Only the failure that actually flips the verdict is a warning." The code did not honour it. It does now. * Warn once, with the exception, on the probe that flips the verdict. * Repeats while the verdict is unchanged: Debug, without the exception. The trace is identical every cycle and the reason string carries the cause. * Recovery: one Information line on the way back up. Previously the up-edge was logged nowhere at all, so an operator saw "kokoro unhealthy" and never learned it came back -- the down-edge alone is only half a transition model. * appsettings: the KokoroHealthProbe/PiperHealthProbe/OllamaHealthProbe typed clients drop to Warning. A health probe's own request/response pair is not application-interesting, and it was 2 of the 3 stack traces per cycle. The edge is TRACKED (a per-dependency ConcurrentDictionary of the reason already warned about), not derived from ConsecutiveFailureCount == UnhealthyThreshold. The threshold is re-read live every cycle (gh-#125), so an operator lowering it mid-outage flips the verdict at a count already past the new threshold, and the arithmetic version would log that flip at Debug and lose it. There is a spec for exactly that. Deliberately NOT re-warned: a reason that changes mid-outage. Re-warning on it would reopen this hole for any dependency whose failure mode oscillates. Sub-threshold Debug keeps its stack trace -- it is the first observation of a real problem and is bounded at threshold-1 per outage. Only the unbounded stream is stripped. 10 specs; 4 fail against the old prober.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #338.
🔊 The field symptom
On the gh-#213 Pi 5, kokoro is profile-disabled by the piper-only overlay, so its probe can never succeed. The api logged this every 30 s, forever:
The verdict flipped at failure 2. Failure 29 changed nothing, and the wording claimed a transition each time.
warn+ stack traceinfofrom the typed client + stack tracesFor a condition reported once and permanent by construction.
📐 It was already the stated contract
RecordFailure's own doc comment:That was always the intent. The code branched on
verdict.Healthy— which is false for the flip and every cycle after it — so it never honoured it. It does now.🔧 The change
KokoroHealthProbe/PiperHealthProbe/OllamaHealthProbetyped clients drop toWarning. A health probe's own request/response pair isn't application-interesting, and it was 2 of the 3 stack traces per cycle.🪤 The trap, and why the edge is tracked rather than derived
ConsecutiveFailureCount == UnhealthyThresholdlooks like a free edge test. It's wrong: the threshold is re-read live every cycle (gh-#125), so an operator lowering it mid-outage flips the verdict at a count already past the new threshold — and the arithmetic version logs that flip at Debug and loses it.So the edge is tracked explicitly, in a per-dependency
ConcurrentDictionaryof the reason already warned about.TryAddsucceeds exactly once per outage and is atomic.ScenarioTheThresholdIsRetunedWhileTheDependencyIsDownpins exactly this case.Equally, suppression keys on "have we warned since the verdict last changed", not "is the verdict unhealthy" — otherwise a dependency that drops, recovers and drops again goes silent on the second drop. That has its own spec too.
🚫 Deliberately not changed
threshold-1per outage (default: one). Only the unbounded stream is stripped.Llm:Endpoint, F34.2 — a declaration, not a fault), and now also can't produce a phantom "recovered" line for an outage nobody was told about.✅ Verification
Gh338_ProbeWarningIsEdgeTriggered.cs— 10 specs drivingRunCycleAsyncone cycle at a time, so every fact is about which cycle logged what. No timers, no clock, no waiting. Uses a small in-file recording logger rather than adding a package.dotnet build GenWave.sln— 0 warnings, 0 errors.dotnet test GenWave.sln— all five green (140 / 199 / 414 / 1286 / 632, 0 failed).📋 Note
This needs a release to reach the Pi — both changed files ship inside the api image, unlike the compose/launch.sh fixes earlier today.