fix: singleflight follower retry on leader disconnect + quiet disconnect handling (review M1/L1/L2)

Addresses the independent reviewer's APPROVE-WITH-CHANGES findings on PR #149:

M1 (MEDIUM, F2 regression) — when a singleflight LEADER disconnected while queued,
its RequestDisconnectedError rejected the SHARED promise, so live followers fell
into respondUpstreamError's generic branch and got a spurious 500 on a healthy
socket. Fix: keys.mjs singleflight() gains an optional follower-side `retryIf`
predicate. When a follower joins an existing flight and the shared promise rejects
with an error retryIf() accepts, the follower does NOT inherit the rejection — it
re-enters singleflight with its OWN fn (the map entry is guaranteed already deleted:
the delete-finally is attached upstream of the promise followers await), becoming
the new leader or joining a retrying sibling's fresh flight. The leader's own
rejection is never retried (it IS that client's disconnect). server.mjs passes
retryIf = (err) => err instanceof RequestDisconnectedError && !res.destroyed, so a
follower whose own client is also gone still propagates quietly. Callers without
retryIf keep byte-for-byte pre-existing share-everything semantics (pinned by the
existing failure-fan-out test).

L1 (LOW) — a disconnect-while-queued on the non-streaming paths was recorded as a
usage FAILURE row and logged as a [proxy] error: metric noise for a non-error.
Both non-streaming catch blocks now early-return on RequestDisconnectedError
without recordUsage(success:false) and without console.error — mirroring the
streaming path, which returns silently. The disconnect remains observable at info
level (concurrency_wait_cancelled, now also emitted with path:"tui" from
callClaudeTui for parity with acquireClaudeSlot's -p log).

L2 (LOW, test gap) — added a unit test for the abort-after-grant race: a waiter
granted its slot whose signal aborts afterward must see no rejection, no queue
corruption, and its slot released exactly once via the normal path (the semaphore
detaches the abort listener at grant; the onAbort idx===-1 guard is the in-dispatch
backstop).

Tests: +3 (2× M1 in the singleflight section, 1× L2 in the F2 section) — suite is
now 247/247 green.

ALIGNMENT.md: unchanged declaration — still local concurrency/dedup infrastructure
with no cli.js wire analogue; no endpoint, header, request field, or response field
added or changed; /health shape untouched. cli.js citation declared ABSENT per
CLAUDE.md hard-requirement #1 (not a Class A mirror change, not a Class B
contract change).

Verification: node --check server.mjs lib/tui/semaphore.mjs keys.mjs && npm test
— 247 passed, 0 failed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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(() => {

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
+    if (err instanceof SemaphoreAbortError) {
-      ? new RequestDisconnectedError("client disconnected while waiting for a TUI concurrency slot")
+      // L1: client-driven cancellation, not an upstream failure — info, not error (mirrors
-      : err;
+      // 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) {

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", () => {