fix(httpx): close request-body completion race in egress recorder - #162
Conversation
net/http writes the request body on a separate goroutine and returns from RoundTrip as soon as the response headers arrive. The receipt recorder read hashingBody.complete synchronously right after RoundTrip returned, so on a loaded runner it could snapshot mid-write and record a fully-sent body as incomplete (ReqBytes=0, ReqSHA256="", request-body-incomplete note). This is what flaked TestRecordsRequestHashAndSize in CI, and could occasionally under-report real egress metadata. Wait for the transport to close the request body (a conformant RoundTripper always does, after the write settles) before snapshotting, bounded by the request context. Adds a deterministic regression test that models net/http's async write ordering plus a concurrent real-transport stress test.
Sigilix is reviewing this pull request. A structured overview will replace this note when retrieval and specialist review complete. |
QualityMax ReviewVerdict: COMMENT · Confidence: evidence-backed scan Files eligible: 2 · Files reviewed: 2 · Files with findings: 0 · Findings: 0 · Inline cards: 0 Priority findings
Review gates
Important files
Change diagram — Flowflowchart TD
A[RoundTrip Start] --> B[Wrap Body in hashingBody]
B --> C[t.base.RoundTrip]
C --> D{Wait for Body Close?}
D -- Success --> E[Wait on hb.closed or Context]
E --> F[Snapshot Body Hash]
D -- Error --> F
Review lifecycleUse the inline cards to inspect evidence and suggested remediation. Re-run the QualityMax review after pushing a fix; unchanged cards are identified by their stable finding marker. Dismiss with a reason through the existing QualityMax/GitHub review feedback flow. 0 prior card(s) are stale/resolved on this head. Proof legend: VERIFIED independently judged patch · REPRODUCED verified finding · GROUNDED deterministic evidence · MODEL-ONLY model judgment. QualityMax project results are available in the configured project. Receipt · commit |
|
| Gate | Result |
|---|---|
| 🔍 AI diff review | ✅ Clean · gemini-3.1-flash-lite · completed · 2 eligible / 2 reviewed · gemini-3.1-flash-lite |
| 🔍 SAST | completed · 2 eligible / 2 reviewed · gemini-3.1-pro-preview |
| 🔍 Canonical PR review delivery | completed · 0 eligible / 0 reviewed · exact-head review #4885982229 and overview #5221223149 confirmed |
| 🧪 Repo Tests | ✅ 581/581 passed (go) |
| 🤖 AI Tests |
Powered by QualityMax — AI-Powered Test Automation
Bump Version to 1.23.0 and promote the [Unreleased] changelog section under a dated [1.23.0] heading. This is the first tagged release to ship the coding-plan usage-window tracking feature (#157) — hence a minor bump — along with the OpenCode backend fixes and the httpx egress body-hash race fix (#162). Also corrects changelog ordering: the empty [1.22.2] heading now sits below [1.23.0] in newest-first order.
What broke
CI on
main(release: v1.22.2) went red on a single test:(The
tar: Cannot open: File existsspam and theQualityMax report failed (HTTP 401)line in that run are unrelated, non-fatal noise from the cache-restore and reporting steps.)The test passes locally at that exact commit, including
-count=500,-race, and-cpu=1— it's a timing flake, not a regression.Root cause
net/httpwrites the request body on a separate goroutine and returns fromRoundTripas soon as the response headers arrive.receiptTransport.RoundTripreadhashingBody.completesynchronously right afterRoundTripreturned, so on a loaded runner it could snapshot mid-write — before the body-write goroutine had read the body throughio.EOFand setcomplete. Result: a fully-sent body recorded as incomplete (ReqBytes=0,ReqSHA256="",request-body-incompletenote).This is mostly a flaky test, but the same window could occasionally under-report real egress metadata in the receipt.
Fix
A conformant
RoundTripperalways closesreq.Bodyafter the write settles (including the final EOF read). SohashingBodynow closes aclosedchannel onClose(), and on a successful round tripRoundTripwaits on it (bounded by the request context, which the clientTimeoutcancels) before snapshotting. Transport-error and no-body paths are unchanged; the genuine "server responded before consuming the body" case still records incomplete, becauseClosestill fires withcomplete == false.Tests
TestFullBodyRecordedWhenTransportFinishesWritingAfterResponse— deterministic: models net/http's async-write ordering with a fake transport that closes the body afterRoundTripreturns. Reproduces the exact CI symptom (bytes:0 hash:"") without the barrier and passes with it.TestConcurrentRealRequestsRecordCompleteBody— 64 concurrent real-loopback requests, each asserting a complete byte count + hash.TestIncompleteRequestBodyIsNotSignedAsACompleteHash's fake to close the body (models a conformantRoundTripper); it still correctly records incomplete.Full
internal/httpxsuite passes under-race -count=3.