Skip to content

fix: green fork CI and stop outbound streams cutting request bodies short - #9

Open
rita-aga wants to merge 3 commits into
mainfrom
claude/fork-ci-fixes
Open

rita-aga wants to merge 3 commits into
mainfrom
claude/fork-ci-fixes

Conversation

@rita-aga

@rita-aga rita-aga commented Oct 6, 2026 •

Copy link
Copy Markdown

Fixes the two CI failures on arni-labs main (found in #8, the fork's first CI run) and the flaky outbound streaming test that blocked #8's first push. That flake was a real request-truncation bug.

1. unwrap scan: policy reinstall tests

CI's "No unwrap() in production code" step treats src/**/mod_test/ as production code. The policy reinstall tests from #7 were the only files there that call .unwrap(). They now use .expect("…"), like the sibling test modules. No behavior change.

2. migration_differential: commits a fork cannot read

The test read pre-migration specs from 53e2304, c7cf6a2 and ad06abd. Those branch commits were rebased before merging, so they exist only on nerdsane side branches. nerdsane CI passes because its checkout fetches every branch; the fork's checkout does not, so git show failed with "invalid object name".

The test now reads from their rebased copies on main: 3c45720a, 7a202380 and 93b513c4. For all ten specs, the parent of each new commit holds a byte-identical blob, so the test checks exactly what it did. In a clone holding only main history, the old test fails the same way CI did and the new one passes (3/3).

3. Outbound streaming cut request bodies short

The bridge in ProductionWasmHost::http_stream_begin_outbound closed the request-body reader as soon as send resolved, which happens on the response head. nerdsane nerdsane#488 added that close so a failed send or an early rejection (such as 413) makes the guest's next write return Closed instead of WouldBlock forever.

But a server may send a success head before it has read the body (a streaming echo, a full-duplex API). Closing the reader then ended the body stream cleanly mid-upload, and the server received a shorter, well-formed request. outbound_streaming_1mib_roundtrip failed about one run in four this way (3/12 on main, missing 6 of 64 chunks in one failure).

Now the reader closes before the head is published only when the send failed or the status is not 2xx, so nerdsane#488's guarantee still holds. After a success head the body keeps flowing, and the reader closes when the exchange ends.

New test outbound_streaming_body_continues_after_early_success_head: it waits for the head, then writes. It failed 10/10 before the fix (Closed) and passes 10/10 after.

Verification (local)

  • http_stream_outbound: 40/40 runs of all 5 tests pass (the 1 MiB test failed about a quarter of runs before)
  • fix(wasm): close failed outbound request streams nerdsane/temper#488's failed_outbound_stream_closes_the_request_reader and early_outbound_response_closes_the_request_reader: 10/10
  • temper-wasm: 190 passed. policy_reinstall: 10 passed
  • clippy on temper-wasm, temper-platform and temper-spec (all targets, -D warnings), cargo fmt --check, the readability ratchet, and CI's unwrap scan run verbatim: all pass

Review

An independent reviewer found no blocking defect. It reproduced the fork case for item 2 and ran a matrix of server behaviors against both versions of the bridge: normal upload, echo, early 2xx with the body never read, 3xx, 303/307, 100 Continue and connection refused. Outcomes match the old code except in three cases:

  • Echo, head first: the bug this fixes. Before, writing stopped at the first chunk; now the whole body is echoed.

  • Early 200, small reply, body never read: before, the guest saw a transport failure (status 0); now it gets the real 200, and its writes return Closed once the reply has ended.

  • Large body, early 2xx, guest reads nothing until it has written everything. This needs a server that sends a 2xx head early and echoes as it reads, and a body larger than the buffers (about 7 MiB in the experiment). Before, the guest got Closed after about 2 ms, but only because the request had been cut short and the server had already received a truncated, well-formed request. Now the guest waits on WouldBlock until the invocation's time limit. Both versions fail that flow; the new one never delivers a truncated request. Current TemperPaw callers (LLM POSTs, $value PUTs) talk to servers that read the whole body before replying, so they are not affected.

Already true before this PR and not changed here: the bridge turns a response-body transport error into a clean end of file (Err(_) => break); the external client follows POST 301/302 and 303 redirects as a GET with an empty body; and a guest that drops an exchange without closing its handles leaves the bridge task alive. Not verified: HTTP/2, or a real WASM guest in the engine.

🤖 Generated with Claude Code

RetriggerConfidence Score: 5/5

The PR appears safe to merge; no actionable defects were found.

What we checked:

  • Failed requests still stop writes: Failed sends and non-success responses close the request reader before the guest receives the head. Closing that reader rejects later writes.
  • Fork checkout includes replacement commits: All three replacement commits are ancestors of the supplied base. CI fetches full history, so these fixtures do not depend on upstream side branches.
Summary

This PR keeps outbound request bodies open after a successful response head. Failed sends and non-success responses still close the request reader immediately.

  • Adds a test that writes request bytes only after receiving a successful head.
  • Uses migration commits available in the fork’s main history.
  • Replaces policy-test unwrap() calls with descriptive expect() calls.
  • No blocking defects or rule violations were found.
  • rita-aga explicitly acknowledged large early-echo stalls, swallowed response-body errors, redirect behavior, and tasks surviving dropped handles. Those cases were excluded from findings.
  • Review checks covered code and Git history; no runtime tests were run.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Send streamed request] --> B{Response head}
  B -->|Failed send| C[Close request reader]
  C --> D[Publish error head and close response writer]
  B -->|Non-success| E[Close request reader before publishing head]
  E --> G[Stream response body]
  B -->|Success| F[Publish head and keep request reader open]
  F --> G
  G --> H[Response finishes or streaming stops]
  H --> I[Close both bridge handles]
Loading

Reviews (1) · Last reviewed commit: "fix(wasm): keep the request body open af..."

rita-aga and others added 3 commits October 6, 2026 10:08
CI's unwrap scan treats src/**/mod_test/ as production code, and the
policy reinstall tests from #7 were the only files there that call
.unwrap(). Use .expect with a message, as the sibling test modules do.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The differential read pre-migration specs from 53e2304, c7cf6a2 and
ad06abd. Those are branch commits that were rebased before the merge,
so they exist only on side branches and a fork's CI cannot read them.
Use their rebased copies on main (3c45720, 7a20238, 93b513c); the
parent of each holds byte-identical sources for all ten specs.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The outbound stream bridge closed the request-body reader as soon as
`send` resolved, which happens on the response head. A server that
answers before it has read the body (a streaming echo, a full-duplex
API) then lost the rest: the body stream ended cleanly and the server
received a shorter, well-formed request. outbound_streaming_1mib_roundtrip
failed about one run in four this way.

Close the reader before publishing the head only when the send failed
or the head is not a success, which keeps nerdsane#488's guarantee that a
rejected stream returns Closed on the next write. After a success head
the reader stays open while the server reads, and closes when the
exchange ends.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant