From 247f897d74fd41796db4816fe0699eebe797318c Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 28 Aug 2026 13:16:54 -0400 Subject: [PATCH] fix(desktop): survive rejected send in permission retransmit loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retransmit orchestrator installed its listener and interval, then awaited the first send outside the loop's settle path. A rejected first send propagated to the caller with the loop never torn down: the listener and interval stayed live firing unhandled `void send()`, while both UI callers re-enabled the buttons — so a retry with the opposite option left two loops retransmitting different optionIds under one nonce. Route the first send through the same guarded transmit path as every tick and swallow a rejected send: a rejection is the transport failure this loop exists to survive, so the next tick retries. The orchestrator never rejects, guaranteeing exactly one loop per click and no unhandled promise on any path. Permanent send failure ends like silence — retry until `expiresAt`, then the card fails closed. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../lib/retransmitPermissionDecision.test.mjs | 64 ++++++++++++++++++- .../lib/retransmitPermissionDecision.ts | 40 +++++++----- .../LifecycleActivity.tsx | 6 +- .../src/shared/ui/permission-request-card.tsx | 6 +- 4 files changed, 92 insertions(+), 24 deletions(-) diff --git a/desktop/src/features/agents/lib/retransmitPermissionDecision.test.mjs b/desktop/src/features/agents/lib/retransmitPermissionDecision.test.mjs index 667fac6732..b901b9d49f 100644 --- a/desktop/src/features/agents/lib/retransmitPermissionDecision.test.mjs +++ b/desktop/src/features/agents/lib/retransmitPermissionDecision.test.mjs @@ -17,20 +17,26 @@ function frame(overrides = {}) { /** * Controllable harness mirroring the real wiring: a single-listener pub/sub * whose unsubscribe genuinely detaches, a manual retransmit tick, a manual - * deadline flag, and a send counter. + * deadline flag, and a send counter. `failSends` rejects the first N send + * attempts, mirroring a socket that is briefly down. */ -function harness({ nonce = NONCE } = {}) { +function harness({ nonce = NONCE, failSends = 0 } = {}) { let listener = null; let tickCb = null; let unsubscribeCalls = 0; let cancelRetransmitCalls = 0; let sendCalls = 0; let expired = false; + let remainingFailures = failSends; const outcome = retransmitPermissionDecision({ requestNonce: nonce, send: () => { sendCalls += 1; + if (remainingFailures > 0) { + remainingFailures -= 1; + return Promise.reject(new Error("send failed: socket down")); + } return Promise.resolve(); }, subscribe: (fn) => { @@ -144,3 +150,57 @@ test("retransmitPermissionDecision ignores a control_result for a different nonc h.push(frame()); assert.equal(await h.outcome, "acked"); }); + +test("retransmitPermissionDecision survives a rejected first send and acks when a later tick's send resolves", async () => { + // The causal case: the owner clicks while the relay socket is down. The first + // send rejects, but the loop must stay live so a later retransmit — once the + // socket recovers — delivers and acks. + const unhandled = []; + const onUnhandled = (reason) => unhandled.push(reason); + process.on("unhandledRejection", onUnhandled); + try { + const h = harness({ failSends: 1 }); + await drainMicrotasks(); + assert.equal(h.sendCalls, 1, "first send fired and rejected"); + + // A later tick, after the socket recovers, resends successfully. + h.tick(); + await drainMicrotasks(); + assert.equal(h.sendCalls, 2, "the loop retries after a rejected send"); + + h.push(frame()); + assert.equal(await h.outcome, "acked"); + assert.equal(h.unsubscribeCalls, 1); + assert.equal(h.cancelRetransmitCalls, 1); + } finally { + process.off("unhandledRejection", onUnhandled); + } + assert.deepEqual(unhandled, [], "a rejected send must not surface unhandled"); +}); + +test("retransmitPermissionDecision expires cleanly when every send rejects", async () => { + const unhandled = []; + const onUnhandled = (reason) => unhandled.push(reason); + process.on("unhandledRejection", onUnhandled); + try { + // Every send rejects (permanent transport failure). The loop must not throw + // — it keeps retrying until the deadline, then resolves "expired". + const h = harness({ failSends: Infinity }); + await drainMicrotasks(); + assert.equal(h.sendCalls, 1, "first send fired and rejected"); + + h.tick(); + await drainMicrotasks(); + assert.equal(h.sendCalls, 2, "keeps retrying through rejection"); + + h.expire(); + h.tick(); + assert.equal(await h.outcome, "expired"); + assert.equal(h.sendCalls, 2, "no resend past the deadline"); + assert.equal(h.unsubscribeCalls, 1, "listener torn down at expiry"); + assert.equal(h.cancelRetransmitCalls, 1, "scheduler torn down at expiry"); + } finally { + process.off("unhandledRejection", onUnhandled); + } + assert.deepEqual(unhandled, [], "rejected sends must not surface unhandled"); +}); diff --git a/desktop/src/features/agents/lib/retransmitPermissionDecision.ts b/desktop/src/features/agents/lib/retransmitPermissionDecision.ts index 079583656c..9192adb1f7 100644 --- a/desktop/src/features/agents/lib/retransmitPermissionDecision.ts +++ b/desktop/src/features/agents/lib/retransmitPermissionDecision.ts @@ -25,11 +25,19 @@ import type { ControlResultFrame } from "@/shared/api/types"; * concurrent `control_result` for a different card carries a different nonce and * is inert, mirroring the `requestId` guard in `awaitLiveSwitchOutcome`. * + * A send rejection is the transport failure this loop exists to survive, so the + * orchestrator never throws: the first send and every retransmit run through + * the same guarded path, and a rejected attempt is swallowed — the next tick + * retries. This guarantees exactly one loop per click (the UI's disabled state + * guards double-click) and no unhandled rejection on any path. Permanent send + * failure therefore ends the same way as silence: the loop retries until + * `expiresAt`, then resolves `"expired"` and the card fails closed. + * * The loop is isolated from React and the relay so it can be unit tested with a * fake clock and synthetic frames. The caller injects the send, the * `control_result` subscription, and the retransmit scheduler. */ -export async function retransmitPermissionDecision({ +export function retransmitPermissionDecision({ requestNonce, send, subscribe, @@ -53,7 +61,7 @@ export async function retransmitPermissionDecision({ */ deadlineReached: () => boolean; }): Promise<"acked" | "expired"> { - const settled = new Promise<"acked" | "expired">((resolve) => { + return new Promise<"acked" | "expired">((resolve) => { let unsubscribe = () => {}; let cancelRetransmit = () => {}; const finish = (outcome: "acked" | "expired") => { @@ -61,6 +69,16 @@ export async function retransmitPermissionDecision({ unsubscribe(); resolve(outcome); }; + // Attempt one transmit, respecting the deadline and swallowing a rejected + // send. A rejection is a failed transmit, not a fatal error: the next tick + // retries, so the loop survives a socket that is briefly down. + const transmit = () => { + if (deadlineReached()) { + finish("expired"); + return; + } + void send().catch(() => {}); + }; unsubscribe = subscribe((frame) => { // A `control_result` for THIS nonce means the harness received the // decision and has an authoritative answer — stop retransmitting. Frames @@ -74,19 +92,9 @@ export async function retransmitPermissionDecision({ } finish("acked"); }); - cancelRetransmit = scheduleRetransmit(() => { - // Never resend past the card's expiry: it times out on its own and a - // late decision would be rejected. Stop and report the deadline. - if (deadlineReached()) { - finish("expired"); - return; - } - void send(); - }); + cancelRetransmit = scheduleRetransmit(transmit); + + // Fire the first send immediately, then let the scheduler drive resends. + transmit(); }); - - // Fire the first send immediately, then let the scheduler drive resends. - await send(); - - return settled; } diff --git a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx index 548700550b..278b36efb4 100644 --- a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx +++ b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx @@ -153,9 +153,9 @@ function PermissionDecisionButtons({ optionId, deadlineSecs, }).catch(() => { - // First send rejected by the relay. Re-enable so the user can - // retry; the harness's 300 s fail-closed timeout handles - // permanent loss. + // The delivery loop never rejects on a failed send — it retries + // until ack or the card's 300 s fail-closed expiry. This only + // fires on an unexpected error; re-enable so the user can retry. setPending(null); }); }} diff --git a/desktop/src/shared/ui/permission-request-card.tsx b/desktop/src/shared/ui/permission-request-card.tsx index df1ca3a731..2283b48f7c 100644 --- a/desktop/src/shared/ui/permission-request-card.tsx +++ b/desktop/src/shared/ui/permission-request-card.tsx @@ -145,9 +145,9 @@ function PermissionButtons({ optionId, deadlineSecs: request.expiresAt, }).catch(() => { - // First send rejected by the relay. Re-enable so the user - // can retry; the retransmit loop only starts once the send - // resolves. + // The delivery loop never rejects on a failed send — it + // retries until ack or expiry. This only fires on an + // unexpected error; re-enable so the user can retry. setSubmitted(null); }); }}