Repository navigation
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
unwrapscan: policy reinstall testsCI'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 readThe test read pre-migration specs from
53e2304,c7cf6a2andad06abd. 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, sogit showfailed with "invalid object name".The test now reads from their rebased copies on
main:3c45720a,7a202380and93b513c4. 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 onlymainhistory, 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_outboundclosed the request-body reader as soon assendresolved, 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 returnClosedinstead ofWouldBlockforever.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_roundtripfailed about one run in four this way (3/12 onmain, 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)failed_outbound_stream_closes_the_request_readerandearly_outbound_response_closes_the_request_reader: 10/10temper-wasm: 190 passed.policy_reinstall: 10 passedtemper-wasm,temper-platformandtemper-spec(all targets,-D warnings),cargo fmt --check, the readability ratchet, and CI'sunwrapscan run verbatim: all passReview
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
Closedonce 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
Closedafter 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 onWouldBlockuntil the invocation's time limit. Both versions fail that flow; the new one never delivers a truncated request. Current TemperPaw callers (LLM POSTs,$valuePUTs) 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
The PR appears safe to merge; no actionable defects were found.
What we checked:
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.
unwrap()calls with descriptiveexpect()calls.rita-agaexplicitly acknowledged large early-echo stalls, swallowed response-body errors, redirect behavior, and tasks surviving dropped handles. Those cases were excluded from findings.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]Reviews (1) · Last reviewed commit: "fix(wasm): keep the request body open af..."