{{ message }}
Fix a 500 on dictation Unload before any backend is resident - #9013
Merged
Conversation
stt_unload passes expected_model positionally:
_, unload_stt = _stt_lifecycle()
failed = await asyncio.to_thread(unload_stt, engines, model)
_stt_lifecycle() returns the orchestrator's unload_stt_model when a backend is
resident and stt_registry.unload when one is not. Only the first takes
expected_model positionally; on the registry it sits behind a `*`:
def unload(engines = None, *, wait = True, expected_model = None)
def unload_stt_model(self, engines = None, expected_model = None)
So with nothing loaded yet, which is what a fresh process is, Unload raises
TypeError and the route answers 500. Reproduced outside pytest:
peek_inference_backend() fresh process: None
stt_unload -> TypeError: unload() takes from 0 to 1 positional arguments
but 2 were given
Pass it by keyword, which both callables accept.
Found while profiling CI, not by the tests, and the tests are why: this file's
two unload tests passed only because an earlier test in the full suite had left
a backend resident. Standalone they failed, and in their own file they failed;
they went green only inside the full serial run. That order dependence hid a
live bug. The new test drives the no-backend path directly, so neither the
signature nor the call site can drift back.
before: 2 failed, 53 passed (file alone)
after: 56 passed
This was referenced Aug 16, 2026
danielhanchen
added a commit
that referenced
this pull request
Aug 17, 2026
test_disconnected_raw_transcription_cancels_its_sidecar passes in its file and
fails on its own, on any machine that has not downloaded an STT model:
assert raised.value.status_code == 499
E assert 409 == 499
E + where 409 = HTTPException(status_code=409, detail="STT model 'small' is
not downloaded. Download it in Settings, then Voice, before loading it.")
Introduced by #7984 (d20db3f, 2026-08-11), which added this test AND the
implicit registry load in _transcribe_audio_result it fails on. The test was
incomplete from birth: it stubs _stt_sidecar_for and _resolve_serving_stt_engine
but not the load, so the disconnect it exists to check is only reached when an
earlier test left an engine resident or the host has the model cached.
The sibling test directly above it, added by the same PR, does stub
_stt_lifecycle. This does the same. Cancellation, not download state, is the
subject.
Worth noting that #7984 is the same PR behind the stt_unload 500 (#9013) and the
two test_stt_ggml_sidecar unload tests that fail standalone: three defects in one
area, each invisible because a serial run happened to leave the right state
behind.
Robustification: the stub now records what it was asked to load, and the test
asserts it. Otherwise, if the route ever stops loading through the registry, the
stub becomes dead code and the test stays green for a path it no longer covers.
Checked by mutating the route to skip the load:
the transcribe path did not load through the registry: []
Verified by forcing the no-snapshot condition:
main, test alone: 1 failed (409 == 499)
main, whole file: 9 passed <- the order dependency
fixed, no snapshot: 9 passed
fixed, cached host: 9 passed
The bug fixed in the previous commit exists because _stt_lifecycle returns two
different callables and the route has a single call site:
def unload(engines = None, *, wait = True, expected_model = None) # stt_registry
def unload_stt_model(self, engines = None, expected_model = None) # orchestrator
A call that suits one is a TypeError on the other, and which one runs depends on
whether a backend happens to be resident, so the broken half only appears on a
fresh process. That will recur the next time either signature is edited.
This binds BOTH real signatures against the arguments the route actually passes.
It does not demand they be identical, only that one call site can serve both.
Checked against two mutations:
registry grows a keyword-only param -> passes (benign, and it should)
registry drops expected_model -> fails, "got an unexpected keyword
argument 'expected_model'"
Paired with the test beside it: that one covers the call site (route -> registry
on a cold process), this one covers the two callees staying compatible.
57 passed.
for more information, see https://pre-commit.ci
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Member
Author
danielhanchen
added a commit
that referenced
this pull request
Aug 17, 2026
test_disconnected_raw_transcription_cancels_its_sidecar passes in its file and
fails on its own, on any machine that has not downloaded an STT model:
assert raised.value.status_code == 499
E assert 409 == 499
E + where 409 = HTTPException(status_code=409, detail="STT model 'small' is
not downloaded. Download it in Settings, then Voice, before loading it.")
Introduced by #7984 (d20db3f, 2026-08-11), which added this test AND the
implicit registry load in _transcribe_audio_result it fails on. The test was
incomplete from birth: it stubs _stt_sidecar_for and _resolve_serving_stt_engine
but not the load, so the disconnect it exists to check is only reached when an
earlier test left an engine resident or the host has the model cached.
The sibling test directly above it, added by the same PR, does stub
_stt_lifecycle. This does the same. Cancellation, not download state, is the
subject.
Worth noting that #7984 is the same PR behind the stt_unload 500 (#9013) and the
two test_stt_ggml_sidecar unload tests that fail standalone: three defects in one
area, each invisible because a serial run happened to leave the right state
behind.
Robustification: the stub now records what it was asked to load, and the test
asserts it. Otherwise, if the route ever stops loading through the registry, the
stub becomes dead code and the test stays green for a path it no longer covers.
Checked by mutating the route to skip the load:
the transcribe path did not load through the registry: []
Verified by forcing the no-snapshot condition:
main, test alone: 1 failed (409 == 499)
main, whole file: 9 passed <- the order dependency
fixed, no snapshot: 9 passed
fixed, cached host: 9 passed
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Unload for dictation raises
TypeErrorand the route answers 500 whenever no inference backend is resident, which is the state a freshly started process is in.Reproduced outside pytest, on main:
Cause
stt_unloadpassesexpected_modelpositionally:_stt_lifecycle()returns two different callables:Only the orchestrator's takes
expected_modelpositionally. On the registry it sits behind a*. The registry branch is the one taken whenpeek_inference_backend()isNone, so the call blows up exactly when nothing has loaded yet.Introduced in #7984 (
d20db3f1f), which added both signatures.Fix
Pass it by keyword. Both callables accept that, so the call is correct on either branch.
Why the tests did not catch it
They were order dependent, and that is the interesting part.
The two existing unload tests in
test_stt_ggml_sidecar.pyfail standalone and fail in their own file. They only went green inside the full serial run, because an earlier test in another file had left a backend resident, which routed the call down the orchestrator branch where positional works.So the order dependence was not a nuisance, it was hiding a live bug.
The new test drives the no-backend path directly, stubbing
peek_inference_backendtoNoneand asserting the registry receivesexpected_model, so neither the signature nor the call site can drift back.Verification
Mutation checked. Reverting the one-line fix while keeping the new test reproduces the production error:
Full backend suite on a CPU-only shape, same command the workflow runs:
The 2 that flip are exactly these unload tests. The remaining 38 are pre-existing and unrelated to this change (missing optional
rag/video/diffusionextras and network-blocked probes in my local venv).Robustification
The fix is one keyword, but the shape that produced it will recur:
_stt_lifecyclehands back two different callables and the route has one call site, so which signature must be satisfied depends on whether a backend happens to be resident.A second test binds both real signatures against the arguments the route actually passes. It does not demand they be identical, only that one call site can serve both. Checked against two mutations:
expected_modelgot an unexpected keyword argument 'expected_model'Paired with the first test, which covers the call site on a cold process, the two cover both directions of the drift.
Related
This PR's root cause, #7984 (
d20db3f1f), also accounts for two other order-masked defects I hit while profiling: the disconnect-cancellation test that never stubs the implicit registry load (#9031), and the twotest_stt_ggml_sidecar.pyunload tests noted above. Three defects in one area, each invisible because a serial run left the right state behind.Found while profiling CI runtime, not by a failing check.