fix(proxy): pin Request freshness by identity; retry only connect-stage failures

Two non-blocking follow-ups from the PR TriDefender#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).

Author: demian-bot

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<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" } });
     });
 
@@ -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();

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`);

src/proxy/ordered-transport.ts

@@ -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;
@@ -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;
   });
 }