From 7dd6818fedecd208cd0cdb8e5a4c4bde7c28e485 Mon Sep 17 00:00:00 2001 From: demian Date: Sun, 30 Aug 2026 02:03:58 +0800 Subject: [PATCH] fix(proxy): pin Request freshness by identity; retry only connect-stage failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two non-blocking follow-ups from the PR #34 review: 1. The freshness regression test asserted req.bodyUsed === false inside a mock fetchImpl — vacuous, because a mock never consumes the Request body, so it passed even under the original reuse bug. Pin freshness by object identity instead: record the first Request and assert the retried dispatch receives a different instance. 2. The connect-retry loop re-sent the request on ANY dispatch throw, including failures after the ordered transport already wrote the full request head+body to the wire (socket closed before response headers) — resending in that window could submit the completion twice and consume quota up to three times. sendOrderedUpstreamRequest now flags errors raised after the write completed (err.postWrite), and the retry loop skips flagged errors, narrowing retries to genuine connect-stage failures. Plain-fetch post-headers-write failures remain undetectable at this layer (unchanged, per review). --- src/proxy/handler-resilience.test.ts | 31 ++++++++++++++++------------ src/proxy/handler.ts | 5 +++++ src/proxy/ordered-transport.ts | 9 ++++++++ 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/src/proxy/handler-resilience.test.ts b/src/proxy/handler-resilience.test.ts index 371314b..327fab7 100644 --- a/src/proxy/handler-resilience.test.ts +++ b/src/proxy/handler-resilience.test.ts @@ -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). @@ -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 => { 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" } }); }); @@ -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(); diff --git a/src/proxy/handler.ts b/src/proxy/handler.ts index 648a015..9d6a5cc 100644 --- a/src/proxy/handler.ts +++ b/src/proxy/handler.ts @@ -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`); diff --git a/src/proxy/ordered-transport.ts b/src/proxy/ordered-transport.ts index ffb55b5..ad93b82 100644 --- a/src/proxy/ordered-transport.ts +++ b/src/proxy/ordered-transport.ts @@ -26,6 +26,7 @@ export async function sendOrderedUpstreamRequest(req: OrderedUpstreamRequest): P return await new Promise((resolve, reject) => { let headerBuffer: Uint8Array = new Uint8Array(0); let responseStarted = false; + let postWrite = false; let bodyController: ReadableStreamDefaultController | null = null; let chunkedDecoder: ChunkedDecoder | null = null; let remainingContentLength: number | null = null; @@ -40,6 +41,13 @@ export async function sendOrderedUpstreamRequest(req: OrderedUpstreamRequest): P }); function fail(err: unknown): void { + if (!responseStarted && postWrite) { + // 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(); @@ -127,6 +135,7 @@ export async function sendOrderedUpstreamRequest(req: OrderedUpstreamRequest): P socket.write(requestHead); if (bodyBytes.byteLength > 0) socket.write(bodyBytes); + postWrite = true; }); }