Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions src/proxy/handler-resilience.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
* HTTP 400 + {"code":3007,...} in the JSON body (no captcha header) must
* be treated as a captcha challenge and retried with a fresh token.
* 2. Connect-retry freshness: after a connect-level failure, the retried
* dispatch must receive a FRESH Request (body not marked used). A mock
* fetch that throws-then-succeeds does NOT catch the bug (mocks don't
* enforce bodyUsed), so the mock asserts `req.bodyUsed === false` on
* every call per the review's test note.
* dispatch must receive a FRESH Request. Review follow-up #1 (PR #34):
* a `req.bodyUsed === false` assertion is vacuous in a mock (mock fetch
* never consumes the body), so freshness is pinned by OBJECT IDENTITY —
* the second call must receive a different Request instance.
*
* Both tests use the start-plan path with an injected captcha module via
* `mock.module("./captcha.js")` (same technique as captcha-pool.test.ts).
Expand Down Expand Up @@ -117,20 +117,25 @@ describe("proxyRequest — start-plan resilience (PR #34 review P1/P3)", () => {
expect(body.choices[0].message.content).toBe("resilience reply");
});

it("retries a connect failure with a FRESH Request (body not marked used)", async () => {
it("retries a connect failure with a FRESH Request (identity-pinned)", async () => {
// Upstream: first call = connect-level failure (the bug shape), second
// call = success. The mock asserts req.bodyUsed === false on EVERY call
// per the review's test note: mocks don't enforce bodyUsed the way real
// fetch does, so the freshness assertion must be explicit.
// call = success. Review follow-up #1 (PR #34): a mock fetch never
// consumes the Request body, so a `req.bodyUsed === false` assertion is
// vacuous — it passes even if the handler re-dispatches the SAME Request
// object. Pin freshness by OBJECT IDENTITY instead: the second call must
// receive a different Request instance than the first.
let calls = 0;
let firstReq: Request | null = null;
const fetchMock = mock(async (req: Request): Promise<Response> => {
calls += 1;
// The freshness assertion: if the handler re-dispatched the SAME
// Request object, this would be true on the second call.
expect(req.bodyUsed).toBe(false);
if (calls === 1) {
firstReq = req;
throw new Error("Unable to connect. Is the computer able to access the url?");
}
// The freshness assertion: re-dispatching the SAME Request object
// would fail this identity check.
expect(firstReq).not.toBeNull();
expect(req).not.toBe(firstReq);
return new Response(ANTHROPIC_OK, { status: 200, headers: { "content-type": "application/json" } });
});

Expand All @@ -144,8 +149,8 @@ describe("proxyRequest — start-plan resilience (PR #34 review P1/P3)", () => {

const resp = await proxyRequest(clientReq, "openai", { config: TEST_CONFIG, auth, fetchImpl: fetchMock as any });

// The connect failure was retried with a FRESH Request (bodyUsed false
// on the second call — the freshness assertion inside the mock passed).
// The connect failure was retried with a FRESH Request (identity check
// inside the mock passed).
expect(calls).toBe(2);
expect(resp.status).toBe(200);
const body = await resp.json();
Expand Down
5 changes: 5 additions & 0 deletions src/proxy/handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,11 @@ export async function proxyRequest(
upstreamResp = await dispatch(upstreamReq, upstreamHeaderPairs);
break;
} catch (err) {
// Review follow-up #2 (PR #34): if the ordered transport already
// wrote the full request to the wire before failing, the upstream
// may have processed it — resending could consume quota twice. Skip
// the retry for those post-write failures.
if ((err as { postWrite?: boolean }).postWrite) throw err;
if (attempt >= maxConnectAttempts) throw err;
const backoffMs = 500 * attempt;
if (debug) debugError(reqId, "upstream_connect_retry", `attempt ${attempt}/${maxConnectAttempts - 1} failed (${(err as Error).message}), retrying in ${backoffMs}ms`);
Expand Down
9 changes: 9 additions & 0 deletions src/proxy/ordered-transport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export async function sendOrderedUpstreamRequest(req: OrderedUpstreamRequest): P
return await new Promise<Response>((resolve, reject) => {
let headerBuffer: Uint8Array<ArrayBufferLike> = new Uint8Array(0);
let responseStarted = false;
let postWrite = false;
let bodyController: ReadableStreamDefaultController<Uint8Array> | null = null;
let chunkedDecoder: ChunkedDecoder | null = null;
let remainingContentLength: number | null = null;
Expand All @@ -40,6 +41,13 @@ export async function sendOrderedUpstreamRequest(req: OrderedUpstreamRequest): P
});

function fail(err: unknown): void {
if (!responseStarted && postWrite) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Mark clean pre-header closes as post-write failures

When the ordered upstream accepts the complete request and then closes with a clean FIN before returning response headers, the socket.once("end") branch rejects a new untagged error directly instead of calling fail(). Although postWrite is already true, the handler therefore treats this as a connect-stage failure and can resend the side-effecting LLM request twice, potentially consuming quota three times. Route that pre-header end error through the same post-write classification.

Useful? React with 👍 / 👎.

// Review follow-up #2 (PR #34): the full request (head + body) was
// already written to the wire, so the upstream may have processed it
// — resending could duplicate the LLM call and consume quota twice.
// Flag it so the connect-retry loop in handler.ts skips this error.
try { (err as { postWrite?: boolean }).postWrite = true; } catch {}
}
if (responseStarted) bodyController?.error(err);
else reject(err);
socket.destroy();
Expand Down Expand Up @@ -127,6 +135,7 @@ export async function sendOrderedUpstreamRequest(req: OrderedUpstreamRequest): P

socket.write(requestHead);
if (bodyBytes.byteLength > 0) socket.write(bodyBytes);
postWrite = true;
});
}

Expand Down