Repository navigation
fix(proxy): pin Request freshness by identity; retry only connect-stage failures - #35
Conversation
…ge 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).
|
@TriDefender This closes out the two non-blocking follow-ups you left on #34 — the identity-pinned freshness test and the postWrite retry guard, exactly along the lines you suggested. Merging #34 made this branch's base (3961cc8), so it applies cleanly. Whenever you get a chance — no rush. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dd6818fed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| function fail(err: unknown): void { | ||
| if (!responseStarted && postWrite) { |
There was a problem hiding this comment.
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 👍 / 👎.
…lures Codex review of PR TriDefender#35: the socket 'end' branch rejected a fresh untagged error when the upstream closed cleanly before response headers, bypassing the postWrite classification — even though the full request head+body had already been written, so the upstream may have processed the LLM call and a resend could consume quota twice. Flag that error the same way fail() flags post-write errors, and add a real-socket regression test: server accepts the full request, closes with a clean FIN, no response — the rejection must carry postWrite: true so the handler's connect-retry loop skips the resend.
|
Addressed the Codex review point in 60a446c: the |
Summary
Follows up on the two non-blocking items from the PR #34 review (both confirmed non-blocking by @TriDefender before merging #34).
1. The freshness regression test is now load-bearing
handler-resilience.test.tsassertedreq.bodyUsed === falseinside the mockfetchImpl. That assertion is vacuous: a mock fetch never consumes the Request body, sobodyUsedstaysfalseon every call — including under the original Request-reuse bug the test is meant to pin. Freshness is now asserted by object identity: the mock records the first Request and asserts the retried dispatch receives a different Request instance (expect(req).not.toBe(firstReq)), which fails under a regression back to Request reuse.2. The connect-retry loop no longer resends post-write failures
dispatchcan reject after the ordered transport has already written the full request head+body to the wire (socket closed before response headers). In that window the upstream may have processed the LLM request, so resending is not side-effect-free — it could submit the completion again and consume quota up to three times. Changes:ordered-transport.ts:fail()marks errors raised aftersocket.write(requestHead + body)completed witherr.postWrite = true(only when the failure precedes response headers — post-headers failures already surface as stream errors, which retry never sees)handler.ts: the connect-retry loop skips flagged errors (if (err.postWrite) throw err), narrowing retries to genuine connect-stage failuresPlain-fetch post-headers-write failures remain undetectable at this layer — unchanged, consistent with the review's scope.
Testing
bun x tsc --noEmitcleanbun test: 589 pass / 0 fail (identity assertion now fails if the handler re-dispatches the same Request — verified by the assertion semantics, not just by luck)Context