diff --git a/keys.mjs b/keys.mjs index 2aedf8a..1999ed7 100644 --- a/keys.mjs +++ b/keys.mjs @@ -382,11 +382,25 @@ export function getCacheStats() { // Per ADR 0005 / spec D4: in-process scope only (single Node process per host). const inflightMap = new Map(); -export function singleflight(hash, fn) { +// `retryIf` (optional, audit finding M1): a predicate applied on the FOLLOWER path only. +// When a follower joins an existing flight and the shared promise rejects with an error for +// which retryIf(err) is true (in practice: the LEADER's client disconnected while queued — +// an error that is personal to the leader, not a verdict about the upstream), the follower +// does NOT inherit that rejection. Instead it re-enters singleflight with its OWN fn: it +// either becomes the new leader (the map entry is already deleted — see the finally below, +// which runs before any follower's catch because it is attached upstream of the promise the +// followers await) or joins a flight another retrying follower just created. The leader's +// own rejection is never retried here — its error belongs to it (leader path returns the +// bare promise). Callers that pass no retryIf get the exact pre-M1 share-everything behavior. +export function singleflight(hash, fn, retryIf) { const existing = inflightMap.get(hash); if (existing) { existing.requesters++; - return existing.promise; + if (!retryIf) return existing.promise; + return existing.promise.catch((err) => { + if (!retryIf(err)) throw err; + return singleflight(hash, fn, retryIf); + }); } // Wrap fn() in Promise.resolve().then() so synchronous throws don't escape. const promise = Promise.resolve().then(fn).finally(() => { diff --git a/server.mjs b/server.mjs index 42317c9..9f3c7bd 100644 --- a/server.mjs +++ b/server.mjs @@ -1297,9 +1297,15 @@ async function callClaudeTui(model, messages, _conversationId, _keyName, res) { await tuiSemaphore.acquire(signal); } catch (err) { detach(); - throw err instanceof SemaphoreAbortError - ? new RequestDisconnectedError("client disconnected while waiting for a TUI concurrency slot") - : err; + if (err instanceof SemaphoreAbortError) { + // L1: client-driven cancellation, not an upstream failure — info, not error (mirrors + // acquireClaudeSlot's concurrency_wait_cancelled on the -p path). + logEvent("info", "concurrency_wait_cancelled", { + reason: "client_disconnected", path: "tui", inflight: tuiSemaphore.inflight, queued: tuiSemaphore.queued, + }); + throw new RequestDisconnectedError("client disconnected while waiting for a TUI concurrency slot"); + } + throw err; } detach(); // release() runs in a finally so any throw from runTuiTurn (tmux spawn failure, @@ -2302,12 +2308,24 @@ async function handleChatCompletions(req, res) { const c = await upstreamCall(model, messages, conversationId, req._authKeyName, res); try { setCachedResponse(req._cacheHash, model, c); } catch (e) { logEvent("error", "cache_write_failed", { error: e.message }); } return c; - }); + }, + // M1: if the LEADER disconnected while queued (F2), its RequestDisconnectedError is + // personal to the leader — a live follower must not inherit it as a spurious 500. + // retryIf makes this follower re-enter singleflight with its OWN fn (own res, own + // disconnect signal), becoming the new leader or joining a retrying sibling's flight — + // but only while OUR client is still connected. If our client is also gone, the + // rejection propagates and the RDE early-return in the catch below ends it quietly. + (err) => err instanceof RequestDisconnectedError && !res.destroyed); const id = `chatcmpl-${randomUUID()}`; completionResponse(res, id, model, content); try { recordUsage({ keyId: req._authKeyId, keyName: req._authKeyName, model, promptChars, responseChars: content.length, elapsedMs: Date.now() - t0Usage, success: true }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); } return; } catch (err) { + // L1: a client disconnect while queued is NOT an upstream failure — mirror the + // streaming path (which returns without recording anything): no usage-failure row, + // no [proxy] error log, no error response (the socket is gone). The disconnect is + // already logged at info level (concurrency_wait_cancelled) by acquireClaudeSlot. + if (err instanceof RequestDisconnectedError) { try { res.end(); } catch {} return; } try { recordUsage({ keyId: req._authKeyId, keyName: req._authKeyName, model, promptChars, responseChars: 0, elapsedMs: Date.now() - t0Usage, success: false }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); } console.error(`[proxy] error: ${err.message}`); if (res.headersSent || res.writableEnded || res.destroyed) { @@ -2325,6 +2343,9 @@ async function handleChatCompletions(req, res) { completionResponse(res, id, model, content); try { recordUsage({ keyId: req._authKeyId, keyName: req._authKeyName, model, promptChars, responseChars: content.length, elapsedMs: Date.now() - t0Usage, success: true }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); } } catch (err) { + // L1: disconnect-while-queued — same quiet non-error outcome as the singleflight + // path above and the streaming path (see acquireClaudeSlot's info-level log). + if (err instanceof RequestDisconnectedError) { try { res.end(); } catch {} return; } try { recordUsage({ keyId: req._authKeyId, keyName: req._authKeyName, model, promptChars, responseChars: 0, elapsedMs: Date.now() - t0Usage, success: false }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); } console.error(`[proxy] error: ${err.message}`); if (res.headersSent || res.writableEnded || res.destroyed) { diff --git a/test-features.mjs b/test-features.mjs index d4555b5..9fc09b0 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -463,6 +463,52 @@ async function runSingleflightTests() { assert.equal(r1, 1); assert.equal(r2, 2); }); + + // 7. M1: leader disconnect while queued must not poison live followers. server.mjs passes + // retryIf = (err) => err instanceof RequestDisconnectedError && !res.destroyed — here we + // model that with a tagged error class. The leader (no retryIf on its own promise — the + // rejection is ITS OWN disconnect) sees the error; the live follower re-executes its OWN + // fn and gets a real result instead of a spurious inherited failure. + await asyncTest("M1: leader disconnects while queued → live follower re-executes and gets a real result", async () => { + class FakeDisconnectError extends Error {} + const leaderGate = Promise.withResolvers(); + let leaderRuns = 0; + let followerRuns = 0; + const leaderFn = async () => { leaderRuns++; await leaderGate.promise; throw new FakeDisconnectError("leader client gone"); }; + const followerFn = async () => { followerRuns++; return "real-execution"; }; + const retryIf = (err) => err instanceof FakeDisconnectError; + + const leaderP = singleflight("sf-m1-leader-dc", leaderFn); // becomes leader + const followerP = singleflight("sf-m1-leader-dc", followerFn, retryIf); // joins as follower + leaderGate.resolve(); // leader "disconnects" while holding the flight + + await assert.rejects(leaderP, FakeDisconnectError, "the leader itself still sees its own disconnect"); + assert.equal(await followerP, "real-execution", "follower got a REAL execution, not the leader's disconnect"); + assert.equal(leaderRuns, 1, "leader fn ran once"); + assert.equal(followerRuns, 1, "follower re-executed exactly once (as the new leader)"); + assert.equal(getInflightStats().inflight, 0, "map fully cleaned up after the retry flight settles"); + }); + + // 8. M1 guard: a follower whose retryIf returns false (server.mjs: its OWN client is also + // gone) inherits the rejection unchanged — no retry, no masked error. And a follower with + // NO retryIf keeps the exact pre-M1 share-everything behavior (test 2 pins the fan-out; + // this pins the predicate=false path specifically for the disconnect error). + await asyncTest("M1: follower with retryIf=false (own client also gone) inherits the leader's rejection, no retry", async () => { + class FakeDisconnectError extends Error {} + const gate = Promise.withResolvers(); + let followerRuns = 0; + const leaderFn = async () => { await gate.promise; throw new FakeDisconnectError("leader client gone"); }; + const followerFn = async () => { followerRuns++; return "should-never-run"; }; + + const leaderP = singleflight("sf-m1-both-dc", leaderFn); + const followerP = singleflight("sf-m1-both-dc", followerFn, () => false); // own client dead → no retry + gate.resolve(); + + await assert.rejects(leaderP, FakeDisconnectError); + await assert.rejects(followerP, FakeDisconnectError, "rejection propagates unchanged when retryIf says no"); + assert.equal(followerRuns, 0, "follower fn never executed — no wasted spawn for a dead client"); + assert.equal(getInflightStats().inflight, 0); + }); } await runSingleflightTests(); @@ -2294,6 +2340,42 @@ await asyncTest("F2: cancelling one queued waiter preserves FIFO order for the o assert.equal(sem.inflight, 0); }); +await asyncTest("F2/L2: abort AFTER grant is a no-op — waiter keeps its slot, no rejection, slot released exactly once", async () => { + const sem = new TuiSemaphore(1, { maxQueue: 16 }); + const g1 = deferred(); + const t1 = sem.run(async () => { await g1.p; }); // holds the only slot + await new Promise((r) => setImmediate(r)); + + const controller = new AbortController(); + let granted = false; + const acq = sem.acquire(controller.signal).then(() => { granted = true; }); + await new Promise((r) => setImmediate(r)); + assert.equal(sem.queued, 1, "waiter queued behind t1"); + + // t1 finishes → release() shifts the waiter out and grants it the slot (waiter() detaches + // the abort listener before resolving). + g1.resolve(); + await t1; + await acq; + assert.equal(granted, true, "waiter was granted the slot"); + assert.equal(sem.inflight, 1, "granted waiter holds the slot"); + assert.equal(sem.queued, 0); + + // The client disconnects AFTER the grant — the abort-after-grant race. onAbort must be a + // no-op (the waiter is no longer in _waiters; idx===-1 guard): no rejection materializes, + // the queue is untouched, and the slot is still owned by the (already-resolved) acquirer. + controller.abort(); + await new Promise((r) => setImmediate(r)); + assert.equal(sem.inflight, 1, "abort after grant did NOT revoke or double-free the slot"); + assert.equal(sem.queued, 0, "abort after grant did not corrupt queue accounting"); + + // The slot is released exactly once via the normal path and is immediately reusable. + sem.release(); + assert.equal(sem.inflight, 0, "slot released exactly once via the normal path"); + await sem.run(async () => {}); // prove the semaphore is fully healthy afterward + assert.equal(sem.inflight, 0); +}); + console.log("\nTUI drift observability (C-5):"); test("recordTuiEntrypoint: observed 'cli' is NOT a mismatch and sets lastEntrypoint", () => {