Skip to content

Commit 33aafa0

Browse files
lidge-junclaude
andcommitted
fix(server): finalize native-passthrough request log on client cancel (#44)
Native-passthrough SSE is inspected on a teed background stream; Codex disconnects the instant it finishes reading, aborting that stream. consumeForInspection took the cancelled path and dropped the /api/logs entry (early-abort returned before onDone; mid-drain abort suppressed onTerminal). Add an onCancel finalizer fired on both abort branches (idempotent via finalizeNativePassthroughLog's logged guard), run onDone on early-abort, and stop reportNativeTerminal from downgrading a real detected terminal to a cancel — so a turn logs completed/failed when it finished, 499 client_cancel only on a pure cancel. Exactly one entry per turn. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 6b5daee commit 33aafa0

2 files changed

Lines changed: 73 additions & 6 deletions

File tree

src/server.ts

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -444,15 +444,22 @@ async function handleResponses(
444444
linkAbortSignal(upstream, turnAc.signal);
445445
registerTurn(turnAc);
446446
if (recordTerminalOutcomes) {
447+
// A real terminal was parsed from the (teed) inspection stream — record it as the outcome
448+
// even if the client has already disconnected: the turn genuinely reached that terminal, so
449+
// it must log as completed/failed, not be dropped or downgraded to a cancel (#44). A pure
450+
// client-cancel (no terminal seen) is finalized separately via consumeForInspection's onCancel.
447451
const reportNativeTerminal = (status: ResponsesTerminalStatus) => {
448-
if (options.abortSignal?.aborted) {
449-
options.onNativePassthroughCancel?.();
450-
return;
451-
}
452452
terminalRecorder?.(status);
453453
options.onNativePassthroughTerminal?.(status);
454454
};
455-
consumeForInspection(inspectBody, reportNativeTerminal, turnAc.signal, () => unregisterTurn(turnAc), logCtx);
455+
consumeForInspection(
456+
inspectBody,
457+
reportNativeTerminal,
458+
turnAc.signal,
459+
() => unregisterTurn(turnAc),
460+
logCtx,
461+
() => options.onNativePassthroughCancel?.(),
462+
);
456463
} else {
457464
consumeForResponseLogMetadata(inspectBody, logCtx, turnAc.signal, () => unregisterTurn(turnAc));
458465
}
@@ -1302,12 +1309,13 @@ export function relaySseWithHeartbeat(
13021309
* Background-consume an SSE stream purely for terminal-outcome inspection (quota tracking).
13031310
* Does not produce output; safe to ignore errors (the client-facing stream is separate).
13041311
*/
1305-
function consumeForInspection(
1312+
export function consumeForInspection(
13061313
body: ReadableStream<Uint8Array>,
13071314
onTerminal: (status: ResponsesTerminalStatus) => void,
13081315
signal?: AbortSignal,
13091316
onDone?: () => void,
13101317
logCtx?: RequestLogContext,
1318+
onCancel?: () => void,
13111319
): void {
13121320
const reader = body.getReader();
13131321
const decoder = new TextDecoder();
@@ -1316,13 +1324,21 @@ function consumeForInspection(
13161324
let cancelled = false;
13171325
if (signal) {
13181326
if (signal.aborted) {
1327+
// Aborted before we could read anything (Codex disconnects the instant it finishes reading).
1328+
// Finalize as a client-cancel and release the turn — the early return skips pump()'s finally,
1329+
// so onDone/onCancel must run here or the entry is silently dropped (#44).
13191330
cancelled = true;
13201331
reader.cancel(signal.reason).catch(() => {});
1332+
onCancel?.();
1333+
onDone?.();
13211334
return;
13221335
}
13231336
signal.addEventListener("abort", () => {
1337+
// Mid-drain disconnect: record a client-cancel entry (idempotent downstream) instead of the
1338+
// suppressed onTerminal path. onDone still fires via pump()'s finally after the read rejects.
13241339
cancelled = true;
13251340
reader.cancel(signal.reason).catch(() => {});
1341+
onCancel?.();
13261342
}, { once: true });
13271343
}
13281344
const pump = async () => {
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { consumeForInspection } from "../src/server";
3+
4+
// Regression for issue #44: native-passthrough turns are inspected on a teed background stream.
5+
// Codex disconnects the instant it finishes reading, so the inspection stream is frequently
6+
// aborted. The cancel path must finalize (onCancel) and release the turn (onDone) instead of
7+
// silently dropping the /api/logs entry.
8+
9+
function pendingStream(): ReadableStream<Uint8Array> {
10+
// A stream whose read never resolves on its own — only reader.cancel() (via abort) ends it.
11+
return new ReadableStream<Uint8Array>({ start() {}, pull() { /* never enqueue/close */ } });
12+
}
13+
14+
function closingStream(): ReadableStream<Uint8Array> {
15+
return new ReadableStream<Uint8Array>({ start(c) { c.close(); } });
16+
}
17+
18+
const tick = () => new Promise(r => setTimeout(r, 5));
19+
20+
describe("consumeForInspection cancel finalization (#44)", () => {
21+
test("already-aborted signal → onCancel + onDone fire, onTerminal does not", () => {
22+
const ac = new AbortController();
23+
ac.abort();
24+
let terminal = 0, cancel = 0, done = 0;
25+
consumeForInspection(pendingStream(), () => terminal++, ac.signal, () => done++, undefined, () => cancel++);
26+
expect(cancel).toBe(1);
27+
expect(done).toBe(1);
28+
expect(terminal).toBe(0);
29+
});
30+
31+
test("mid-drain abort → onCancel + onDone fire, onTerminal suppressed", async () => {
32+
const ac = new AbortController();
33+
let terminal = 0, cancel = 0, done = 0;
34+
consumeForInspection(pendingStream(), () => terminal++, ac.signal, () => done++, undefined, () => cancel++);
35+
ac.abort();
36+
await tick();
37+
expect(cancel).toBe(1);
38+
expect(done).toBe(1);
39+
expect(terminal).toBe(0);
40+
});
41+
42+
test("clean close without a terminal payload → onTerminal(incomplete), not a cancel", async () => {
43+
let terminalStatus: string | null = null;
44+
let cancel = 0, done = 0;
45+
consumeForInspection(closingStream(), s => { terminalStatus = s; }, undefined, () => done++, undefined, () => cancel++);
46+
await tick();
47+
expect(terminalStatus).toBe("incomplete");
48+
expect(cancel).toBe(0);
49+
expect(done).toBe(1);
50+
});
51+
});

0 commit comments

Comments
 (0)