Skip to content

fix(httpx): close request-body completion race in egress recorder - #162

Merged
Desperado merged 1 commit into
mainfrom
fix/httpx-body-hash-race
Aug 7, 2026
Merged

fix(httpx): close request-body completion race in egress recorder#162
Desperado merged 1 commit into
mainfrom
fix/httpx-body-hash-race

Conversation

@Desperado

Copy link
Copy Markdown
Contributor

What broke

CI on main (release: v1.22.2) went red on a single test:

internal/httpx  TestRecordsRequestHashAndSize
  httpx_test.go:77: req_bytes = 0, want 7000
  httpx_test.go:81: req_sha256 = "", want "3e5a27be…"

(The tar: Cannot open: File exists spam and the QualityMax 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/http writes the request body on a separate goroutine and returns from RoundTrip as soon as the response headers arrive. receiptTransport.RoundTrip read hashingBody.complete synchronously right after RoundTrip returned, so on a loaded runner it could snapshot mid-write — before the body-write goroutine had read the body through io.EOF and set complete. Result: a fully-sent body recorded as incomplete (ReqBytes=0, ReqSHA256="", request-body-incomplete note).

This is mostly a flaky test, but the same window could occasionally under-report real egress metadata in the receipt.

Fix

A conformant RoundTripper always closes req.Body after the write settles (including the final EOF read). So hashingBody now closes a closed channel on Close(), and on a successful round trip RoundTrip waits on it (bounded by the request context, which the client Timeout cancels) before snapshotting. Transport-error and no-body paths are unchanged; the genuine "server responded before consuming the body" case still records incomplete, because Close still fires with complete == false.

Tests

  • TestFullBodyRecordedWhenTransportFinishesWritingAfterResponse — deterministic: models net/http's async-write ordering with a fake transport that closes the body after RoundTrip returns. 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.
  • Updated TestIncompleteRequestBodyIsNotSignedAsACompleteHash's fake to close the body (models a conformant RoundTripper); it still correctly records incomplete.

Full internal/httpx suite passes under -race -count=3.

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

sigilix Bot commented Aug 7, 2026

Copy link
Copy Markdown

Posted · 3193b17 · 0 findings — View review
Sigilix · 0 of 50 reviews used in past 5h

Sigilix is reviewing this pull request.

A structured overview will replace this note when retrieval and specialist review complete.

== star harbor ==
       .           *              .              *
                 .        __..----..__       .
         *             .-'   .  .     '-.
                    .-'   *       .       '-.
      .            /   .      .------.       \          .
                  ;          /  dock  \       ;
           .      |     *   /__________\  .   |    *
                  |        /|  []  [] |\      |
      *           ;       /_|__________|_\     ;
                   \        .  .  .  .        /
         .          '-.  every ship returns .-'      .
                       '-.  with a signal .-'
              *           '--..____..--'        *
       .                .              .              .

every small ship returns with light
some carry warnings, some carry peace

@sigilix sigilix Bot added the bug Something isn't working label Aug 7, 2026

@qualitymaxapp qualitymaxapp Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

QualityMax Review — canonical overview updated; inline findings are attached to this review.

@qualitymaxapp

qualitymaxapp Bot commented Aug 7, 2026

Copy link
Copy Markdown

QualityMax Review

Verdict: COMMENT · Confidence: evidence-backed scan

Files eligible: 2 · Files reviewed: 2 · Files with findings: 0 · Findings: 0 · Inline cards: 0

Priority findings

priority location finding
No blocking findings

Review gates

gate status
AI diff review completed · eligible 2, reviewed 2 · LLM · served gemini-3.1-flash-lite
SAST completed · eligible 2, reviewed 2 · hybrid · served gemini-3.1-pro-preview
Overall review evidence clean
Inline evidence not needed

Important files

file risk note next step
No findings

Change diagram — Flow

flowchart 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
Loading

Review lifecycle

Use 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. @qmax Q&A is tracked separately.

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 3193b17b19432f74f0b9435b1e5fe440d0de59a7 · run 2026-08-07T19:30:25+00:00 · model served gemini-3.1-pro-preview, gemini-3.1-flash-lite · model requested gemini-3.1-pro-preview, gemini-3.1-flash-lite · model review substantive — 958 model output tokens · model source repository ai_review_preferences.preferred_model · re-review 1 · proof counts {}

@Desperado
Desperado merged commit f4cb1b7 into main Aug 7, 2026
6 of 7 checks passed
@Desperado
Desperado deleted the fix/httpx-body-hash-race branch August 7, 2026 19:34
@qualitymaxapp

qualitymaxapp Bot commented Aug 7, 2026

Copy link
Copy Markdown

⚠️ QualityMax Pipeline

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 ⚠️ 51/56 passed

Powered by QualityMax — AI-Powered Test Automation

@Desperado Desperado mentioned this pull request Aug 8, 2026
Desperado added a commit that referenced this pull request Aug 8, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working qualitymax:reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant