Studio: confirm an Audio model swap instead of dead-ending on the backend 409 - #9092
Conversation
…kend 409 Audio called /load and /unload directly, so the 409 the backend raises when chats are still generating arrived as an error toast with nothing to act on. Route both through the same confirmation Chat uses and force from the answer.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…irmed after Audio hides Without the gate a queue can materialize while the dialog is open, outside the snapshot the answer was given for. pendingTtsLoad is also still null while the dialog is open, so the deactivation effect could not abort a load confirmed from a page that had gone away; queue it for the activation replay instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea04e27e99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
When a direct pick or staged-download completion enters this preflight, only ttsLoadInFlight and Chat's modelLoading are set; Audio's Generate button and handleGenerate check busy/busyRef instead. Because confirmStopRunningChatsIfNeeded() awaits the backend snapshot before opening any dialog, the user can start a TTS request during that window. That request is absent from stopDecision, so a decision with forceCancelActive: false recreates the /load 409, while a forced decision cancels audio the user never agreed to stop. Claim Audio's busy gate before the confirmation await, or make generation reject while ttsLoadInFlight is set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Mechanism is real and I reproduced it, but the only unguarded window is the pre-dialog snapshot fetch, tens of milliseconds. AlertDialog takes no modal prop and is unconditionally modal: it traps focus and sets pointer-events: none outside, so Generate is unreachable for the dialog's whole life. In the count === 0 branch no dialog opens at all and the outcome is the unforced 409, identical to pre-PR, so there is no regression to fix. Sources: Radix AlertDialog docs and radix-ui/primitives#2122.
Review summaryVerdict: useful, no fix needed. The tree is clean - the two review items with real, non-adversarial paths were already absorbed by head, and the third does not clear the bar. Before / afterBefore, Audio's TTS load sent After, both paths call EvidenceBoth installs built from their own SHAs (BEFORE
BEFORE is the plain Audio page. AFTER is the modal "Stop 1 running chat?", naming the actual conversation "Lighthouse keeper story", with Keep generating / Stop and unload.
This is the half that shows why the issue is real rather than cosmetic. BEFORE, the user is shown the raw backend text: "Unloading the model would stop 1 chat that is still generating. Stop them first, or retry with force_cancel_active." - instructing them to set a flag no UI exposes. AFTER there is no toast at all, because nothing is sent until the question is answered.
Worth noting the first attempt correctly failed the identical-output guard with On the three itemsItems 1 and 2 were filed at 11:40 UTC against Item 3 is rejected, and the reason is structural rather than a judgement call: Does it break anything
Simulation52 of 52 pass, driving the real Covered: the pre-PR baseline (409 dead end, staged download destroyed); load path with confirm, decline, Escape and outside-click; a chat finishing mid-dialog (stale count only, nothing extra stopped); a chat or queue starting mid-dialog (the lease aborts it); two Audio swaps racing; the page hidden mid-dialog and the pick replayed against a fresh snapshot; gate contention; backend unreachable; the eject path including decline preserving the staged download; and a leak sweep over 13 named exit paths plus 200 randomised sequences. No case leaks the lifecycle gate, which was the deadlock I most expected to find.
Browsers: nothing here is engine-dependent, so I am not claiming a matrix. The diff adds no browser API - zustand store reads, refs, and one Radix Below the bar, noted
CIBoth failures are infrastructure. |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea04e27e99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if (lifecycleLease === null) { | ||
| ttsLoadInFlight.current = false; | ||
| pendingRoutedTtsPick.current = null; |
There was a problem hiding this comment.
When a deferred pick is replayed as Audio becomes active while another surface is still loading a model, replayQueuedTtsPick() clears the queued value before calling this function, and this branch then discards the request permanently. The same loss occurs when a staged download completes during a Chat model load because its pending entry is cleared before this call. Preserve the current { repoId, ggufFilename, loadId } and retry it after the lifecycle lease becomes available; otherwise the requested model never loads and the user must select it again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one, though it is a fair reading of the flow. The branch is reached with the lifecycle lease held by another surface, and it is a refusal, not a silent drop: the user gets "Wait for the current model to finish loading." The file already treats every refusal the same way, and says so at the decline branch a few lines below, where the comment is that declining refuses the swap so a queued pick must not reopen the dialog.
Preserving the pick is also not the small change it looks like. Nothing calls replayQueuedTtsPick when the blocking load belongs to Chat, so a preserved pick would not retry when the lease frees; it would sit until Audio is next activated. That is worse than losing it: the user, having been told to wait, picks a different model and loads it, and then navigating away and back replays the stale pick and swaps the model underneath them. A correct retry has to be tied to the lease becoming free and guarded against a newer pick superseding it, which is a design change to the lifecycle path this PR just hardened, not a fix to fold into it.
If you want it, the shape I would take is a single-slot retry armed on lease release and dropped whenever any later pick, decline or mode change occurs. Happy to do it as its own PR rather than widening this one.
|
Merged current Why a merge rather than a code change: that check cannot have been failing on this PR's content. The diff is exactly three frontend files ( So the push is the remedy: it dispatches a current run and clears the drift at the same time. A merge, not a rebase, so nobody's local copy breaks. No conflicts. After the merge: typecheck clean, frontend suite 3781 passed with one failure, |
|
@codex review |
|
@codex review |
|
@codex review |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91071e1c73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!stopDecision.proceed) { | ||
| setBusy(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If the user invokes Speak eject, navigates away with the root-level confirmation still open (for example via Cmd/Ctrl+Shift+O), and then confirms, the hidden Audio page proceeds to invalidate its staged load and unload the shared model. The deactivation effect cannot cancel this path because it only handles pendingTtsLoad, so the newly visible page unexpectedly loses its model; mirror the load path's activeRef.current check after the confirmation before performing teardown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not a bug here. The user clicked Eject and then confirmed stopping the running chats, so the unload is exactly the action that was asked for, and the lifecycle lease is held across the dialog, which means no other surface can swap active_model in that window and the eject cannot hit the wrong model. Cmd/Ctrl+Shift+O opens an empty new chat with nothing generating, so nothing is lost by the confirmed unload landing after the navigation.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
loadModel prepares the stored Hugging Face token before it sends /load, and returns without sending when that token is invalid and the user picks replace or dismisses the warning. Cancelling the accepted sends and queued prompts before that call discarded them for a swap that never happened, leaving the previous model resident and the work gone. Move both cancellations into the existing onRequestStart callback, which runs only once the request is issued.
|
@codex review |
2 participants
Summary
Loading or ejecting a model from the Audio page could fail with an error toast the user had no way to act on:
force_cancel_activeis a request field, not something anyone can click, so the toast was a dead end: the only way forward was to find every generating chat by hand and stop it.Cause
Audio calls
/loadand/unloaddirectly. Chat routes both throughconfirmStopRunningChatsIfNeeded, which counts what is running, asks about it, and setsforce_cancel_activeonly once the user has said yes. Audio never picked that up, so it saw the raw 409 from_raise_or_cancel_active_generationsinstead.Read aloud makes this easy to hit. A studio TTS preview holds the model for the whole request and registers as an active generation, so a couple of previews are enough for the next load to refuse. The existing dialog already handles that:
hasNonChatwords audio and embedding requests as requests rather than chats, so the count stops claiming there are conversations that do not exist.Changes
force_cancel_activefrom the answer.confirmStopRunningChatsIfNeededandcancelPreStreamRunReservationsare exported from the chat barrel for this.The backend is unchanged. The 409 still guards every other caller, including a second tab and curl.
Testing
studio/frontend/tests/audio-model-eject.test.ts: the eject assertion that pinned the unforcedunloadModel({ model_path: activeModel })now pins the confirm-then-force flow, plus new cases for the confirmation ordering on eject, the load prompt, the slot claimed before the await, and the declined-load cleanup.npm run typecheckclean,npm run buildclean.biome checkandeslinton the touched files are unchanged from the pre-change baseline.To reproduce the original behaviour: start a chat generating on a local model, open Audio, and pick a TTS model. Before this change that returns the 409 toast above. After it, the same prompt Chat raises appears, confirming goes through, and declining leaves the loaded model and any staged download alone.