From a4e36e216aad18290a55b8649993af4033a9439d Mon Sep 17 00:00:00 2001 From: Duncan Date: Tue, 11 Aug 2026 10:15:44 -0400 Subject: [PATCH] fix(desktop): treat busy-path switch_model sent ack as provisional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The harness acks status:"sent" the moment a live model switch is delivered to an in-flight turn, before the adapter is consulted — the real verdict (failure/unsupported_model, or silence on success) lands later from the requeued session. awaitLiveSwitchOutcome counted sent toward the success threshold, so a single-channel switch resolved "ok" and unsubscribed before the terminal frame could arrive, popping a false "Model switched" toast on an adapter-rejected switch. Make sent provisional: it no longer counts toward success and keeps the subscription alive, so a later failure/unsupported_model settles the pick and the 8s timeout is the no-terminal-frame success fallback. The terminal-success statuses (switched/turn_ending/no_active_turn) keep their per-channel count semantics. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../agents/lib/liveSwitchOutcome.test.mjs | 68 +++++++++++++++++-- .../features/agents/lib/liveSwitchOutcome.ts | 32 +++++++-- .../src/features/agents/ui/ModelPicker.tsx | 10 +-- 3 files changed, 94 insertions(+), 16 deletions(-) diff --git a/desktop/src/features/agents/lib/liveSwitchOutcome.test.mjs b/desktop/src/features/agents/lib/liveSwitchOutcome.test.mjs index 6024aa1a42..75bbddf6fc 100644 --- a/desktop/src/features/agents/lib/liveSwitchOutcome.test.mjs +++ b/desktop/src/features/agents/lib/liveSwitchOutcome.test.mjs @@ -88,15 +88,17 @@ test("awaitLiveSwitchOutcome resolves ok only after the last channel acks", asyn } }; - h.push(frame("sent")); + // The three terminal-success statuses (`switched` / `turn_ending` / + // `no_active_turn`) each count for one channel; only the third settles "ok". + h.push(frame("switched")); await drainMicrotasks(); assert.equal(settled, false, "must not resolve on the first ack"); - h.push(frame("switched")); + h.push(frame("turn_ending")); await drainMicrotasks(); assert.equal(settled, false, "must not resolve before the last ack"); - h.push(frame("turn_ending")); + h.push(frame("no_active_turn")); assert.equal(await h.outcome, "ok"); }); @@ -131,6 +133,60 @@ test("awaitLiveSwitchOutcome settles failed immediately on an adapter-refused fr assert.equal(h.unsubscribeCalls, 1, "no double-unsubscribe on a late frame"); }); +test("awaitLiveSwitchOutcome stays unsettled after a provisional sent, then settles failed when the adapter rejection arrives", async () => { + // The real busy-path producer order: the harness acks `sent` immediately + // (the switch was delivered to the in-flight turn), then — after the requeued + // session consults the adapter — emits `failure` seconds later. With one + // active channel a first-ack-resolves impl would settle "ok" on `sent` and + // detach before `failure` arrives; this regresses that. + const h = harness(1); + let settled = false; + void h.outcome.then(() => { + settled = true; + }); + const drainMicrotasks = async () => { + for (let i = 0; i < 5; i++) { + await Promise.resolve(); + } + }; + + h.push(frame("sent")); + await drainMicrotasks(); + assert.equal(settled, false, "provisional `sent` must not resolve the pick"); + assert.equal(h.unsubscribeCalls, 0, "subscription stays alive after `sent`"); + assert.equal(h.cancelTimeoutCalls, 0, "timeout still armed after `sent`"); + + h.push(frame("failure")); + assert.equal(await h.outcome, "failed"); + assert.equal(h.cancelTimeoutCalls, 1, "timeout cancelled, not awaited"); + assert.equal(h.unsubscribeCalls, 1); +}); + +test("awaitLiveSwitchOutcome stays unsettled after a provisional sent, then resolves ok via the timeout when no terminal frame arrives", async () => { + // Busy-path success emits no positive confirmation frame — after `sent`, the + // backend stays silent. The pick must remain unsettled until the fallback + // timeout resolves "ok". + const h = harness(1); + let settled = false; + void h.outcome.then(() => { + settled = true; + }); + const drainMicrotasks = async () => { + for (let i = 0; i < 5; i++) { + await Promise.resolve(); + } + }; + + h.push(frame("sent")); + await drainMicrotasks(); + assert.equal(settled, false, "provisional `sent` must not resolve the pick"); + assert.equal(h.cancelTimeoutCalls, 0, "timeout still armed after `sent`"); + + h.fireTimeout(); + assert.equal(await h.outcome, "ok"); + assert.equal(h.unsubscribeCalls, 1, "timeout fallback unsubscribes"); +}); + test("awaitLiveSwitchOutcome ignores frames for a different model or control type", async () => { const h = harness(1); h.push(frame("sent", { modelId: "some-other-model" })); @@ -156,9 +212,11 @@ test("awaitLiveSwitchOutcome resolves ok via the timeout fallback when the harne test("awaitLiveSwitchOutcome fires the per-channel sends after subscribing", async () => { const h = harness(1); // The subscription is registered before the sends fire, so a frame arriving - // mid-send is never dropped. Awaiting sendStarted proves sends ran. + // mid-send is never dropped. Awaiting sendStarted proves sends ran. Uses a + // terminal-success frame (`switched`) since the provisional `sent` no longer + // settles the pick on its own. await h.sendStarted; - h.push(frame("sent")); + h.push(frame("switched")); assert.equal(await h.outcome, "ok"); }); diff --git a/desktop/src/features/agents/lib/liveSwitchOutcome.ts b/desktop/src/features/agents/lib/liveSwitchOutcome.ts index efb3006385..af20fd0d11 100644 --- a/desktop/src/features/agents/lib/liveSwitchOutcome.ts +++ b/desktop/src/features/agents/lib/liveSwitchOutcome.ts @@ -10,12 +10,21 @@ import type { ControlResultFrame } from "@/shared/api/types"; * - `unsupported_model` → the target model isn't available for this agent. * - `failure` → the adapter refused the switch (the session stays * on its current model). - * The causes differ, so they resolve to distinct outcomes (`"unsupported"` vs - * `"failed"`) the caller can message separately. Every other status - * (`sent` / `switched` / `turn_ending`) must arrive from every channel before - * resolving success. If the harness never replies, the fallback timeout - * resolves `"ok"` — the override still rides the requeued/next session, we just - * can't confirm it synchronously. + * Their causes differ, so they resolve to distinct outcomes (`"unsupported"` + * vs `"failed"`) the caller can message separately. + * + * `sent` is the busy-path PROVISIONAL ack: the switch was delivered to the + * in-flight turn, but the adapter isn't consulted until the requeued session + * runs. The real verdict lands later as a `failure` / `unsupported_model` frame, + * or — on success — as silence (the backend emits no positive confirmation). So + * `sent` never counts toward success; the subscription stays alive so a later + * terminal frame can still settle the pick. + * + * The remaining statuses — `switched` / `turn_ending` / `no_active_turn` — are + * terminal success for their channel and must arrive from every channel before + * resolving `"ok"`. When no terminal frame settles the pick (the common + * busy-path success case), the fallback timeout resolves `"ok"` — the override + * still rides the requeued/next session, we just can't confirm it synchronously. * * The counting lives here, isolated from React and the relay so it can be unit * tested with synthetic frames and a fake clock. The caller injects the @@ -64,7 +73,16 @@ export async function awaitLiveSwitchOutcome({ finish("failed"); return; } - // sent / switched / turn_ending — count as success for this channel. + if (frame.status === "sent") { + // Busy-path provisional ack: the switch was delivered to the in-flight + // turn, but the adapter isn't consulted until the requeued session runs. + // Don't count it — a later `failure`/`unsupported_model` may still + // settle the pick, and on success the backend stays silent, so the + // timeout fallback is what confirms. + return; + } + // switched / turn_ending / no_active_turn — terminal success for this + // channel. remaining -= 1; if (remaining <= 0) { finish("ok"); diff --git a/desktop/src/features/agents/ui/ModelPicker.tsx b/desktop/src/features/agents/ui/ModelPicker.tsx index 6fda3be4df..bb93a3c5c2 100644 --- a/desktop/src/features/agents/ui/ModelPicker.tsx +++ b/desktop/src/features/agents/ui/ModelPicker.tsx @@ -110,8 +110,10 @@ export function ModelPicker({ // Send a live `switch_model` frame to each channel the agent is working in // and wait for the harness to acknowledge. A single `unsupported_model` // (model unavailable) or `failure` (adapter refused) result rejects the whole - // pick immediately; all other statuses must arrive from every channel before - // resolving success. + // pick immediately. The busy-path `sent` ack is provisional (the adapter + // isn't consulted until the requeued session), so a later terminal frame can + // still reject; the remaining success statuses must arrive from every channel, + // else the timeout fallback confirms. const sendLiveSwitch = React.useCallback( (modelId: string) => { const channelIds = activeTurns.map((turn) => turn.channelId); @@ -127,8 +129,8 @@ export function ModelPicker({ ), ); }, - // No reply in time: treat as sent. The override still rides the - // requeued/next session; we just can't confirm synchronously. + // No terminal frame in time: treat as success. The override still + // rides the requeued/next session; we just can't confirm synchronously. scheduleTimeout: (onTimeout) => { const timeout = window.setTimeout(onTimeout, 8_000); return () => window.clearTimeout(timeout);