test(init): close the mock registry connection gracefully - #617
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — a test-only flake fix that replaces the mock registry's abortive socket close with a graceful one, so a client mid-read of the tarball body can no longer see a transport error instead of the response.
- Extract the connection handler into
serve_one— the inline closure in the accept loop becomes a fallible function returningio::Result<()>, with errors swallowed at the call site since handler threads are detached and their panics could never fail the test. - Read the request to its end before responding — accumulates until
\r\n\r\ninstead of taking a single 4096-byte chunk, so no unread bytes remain in the receive buffer to make the final close abortive. The scan rechecks the whole accumulated buffer, so a terminator split across tworead()calls is still found. - Half-close then drain to EOF —
flush()→shutdown(Shutdown::Write)→ read until EOF, so the close has nothing queued to abort over. Routing and the 404 fallback are carried over unchanged. - Force blocking mode and 10s timeouts on the accepted socket —
set_nonblocking(false)guards against inheriting the listener's non-blocking flag, which would have turned the request read into an instantWouldBlockand served a 404.
I traced the client that actually drives this fixture to confirm the new sequence can't trade one flake for another: aube-registry's reqwest client is HTTP/1.1 here (plain http://, no http2_prior_knowledge, and tarballs are http1_only()), sends its request immediately on connect, and honors connection: close. Both new loops are bounded by the 10s timeouts the old code never set, and write_all + flush complete before the FIN, so the drain step can't race the delivered response. rustfmt --edition 2024 --check is clean on the file.
ℹ️ The same abortive-close shape lives in a dozen sibling registry fixtures
The mechanism this PR fixes is not specific to init_cmd.rs. The mock registries in aube's resolver tests use the identical shape — read one chunk, write the response, drop the socket — and those run on Windows CI through aube-parity.yml, which gates pull requests. Their bodies are small JSON packuments rather than tarballs, so they sit well inside the send buffer and the exposure is lower, but the probe cited in the PR description lost responses on all three platforms once the body grew.
Nothing here blocks this PR; it's worth knowing where else the pattern is if the flake class resurfaces.
Technical details
# Sibling fixtures with the same abortive-close pattern
## Affected sites
- `vendor/aube/crates/aube-resolver/src/tests.rs:1375-1384` — read one 2048-byte chunk, `write_all(&body)`, task ends and drops the socket.
- `vendor/aube/crates/aube-resolver/src/tests.rs:1455-1464` — same shape.
- `vendor/aube/crates/aube-resolver/src/tests.rs:1564-1583` — same shape, with corgi/full body selection.
- `vendor/aube/crates/aube-resolver/src/tests.rs:2407-2430`, `2504-2523`, plus further sites around 1683, 2020, 2202, 6511.
- Not the same class: `vendor/aube/crates/aube-registry/src/client/retry_tests.rs:620-642` deliberately withholds the body to force a client-side timeout, so its close behavior is intentional.
## Required outcome
- No action required in this PR. If the flake class reappears on the Windows `aube-parity` leg, these are the sites to harden first, and a shared helper mirroring `serve_one` would avoid fixing the same bug a dozen times.
## Open questions for the human
- Whether hardening these is worth a follow-up at all, given the small bodies. Note it would be a `vendor/aube` edit, so fork-discipline applies and the root workspace does not fmt/clippy those files.
Claude Opus ｜ 𝕏
Self-review found three false claims in the comment. The abortive close is not Windows-specific: the CI probe measured 0/15 against 15/15 on windows, ubuntu and macos alike. Windows surfaces it as ConnectionReset 10054; macOS stalls to timeout instead. `set_nonblocking(false)` is not belt-and-braces for Windows. BSD `accept` inherits the listener's non-blocking flag onto the accepted socket and Linux does not, so this is macOS-specific and required — without it the read returns WouldBlock before the bytes land and the fixture serves a 404 for a package it holds. That is a second, independent bug the same commit fixed. And the tarballs are 144-146 bytes against a ~622-byte packument, so "the tarball is the largest body" was backwards; every body here fits one buffer, which is why size was never the mechanism.
There was a problem hiding this comment.
ℹ️ No new issues in the code — the delta is comments only, with one wording nit inline.
Reviewed changes — commit dccd63e6 touches nothing but the serve_one doc comment, retracting three claims the previous commit made about why the graceful close is needed. The code reviewed in the prior round is byte-identical.
- Retracted the Windows-specific framing — the abortive close is now described as cross-platform, citing the probe's 0/15-vs-15/15 result on windows, ubuntu and macos, with the platform-specific surfacing (
ConnectionReset10054 vs. a stall to timeout) named rather than assumed. - Reclassified
set_nonblocking(false)from belt-and-braces to required — reattributed from Windows to macOS on the grounds that BSDacceptinherits the listener's non-blocking flag while Linux does not. - Dropped the body-size rationale — the previous comment leaned on the tarball being the largest body served; that claim is gone.
I checked the two load-bearing premises rather than taking them on faith. The fixture listener is non-blocking (init_cmd.rs:117), so the BSD-inheritance path has a real trigger, and Linux accept(2)'s own portability note documents exactly the split the comment describes — the claim is sound, though the macOS 404 it predicts is a latent race (on loopback the request has normally landed by the time the handler reads, and the macOS test leg only runs on push:main/nightly per ci.yml:131), not something the CI history shows biting. I also measured the fixture bodies from their base64 blobs: the tarballs are 140–146 bytes against a larger packument, so the retraction is correct and every body does fit one buffer.
ℹ️ The durable rationale no longer explains the failure that motivated the fix
The PR description still says the test failed "fetching the tarball — the largest body served," which this commit retracts. That matters beyond tidiness: with size out of the picture, nothing in a merged artifact explains the original Windows transport error. The probe I read on probe/win-socket-close is explicit that it forced both conditions the real fixture lacks — a deliberately split request and a 4 MiB body — and that why the real fixture only tripped on Windows is inferred, not proven. That caveat currently lives only on a throwaway branch.
Technical details
# The retracted rationale survives in the PR description, and the caveat that replaces it does not survive at all
## Affected sites
- PR #617 description, paragraphs 1-2 — "fetching the tarball — the largest body served" is the claim commit `dccd63e6` retracts. Measured: fixture tarballs are 140-146 bytes (`init_cmd.rs:60-97` base64 blobs), the packument is larger, and every body fits one 4096-byte buffer.
- `probe/win-socket-close` — `tests/win-socket-close/README.md` holds the honest scoping: the probe's client sends the request in two segments with a 20ms gap and serves a 4 MiB body, so the cross-platform 0/15 result establishes the *mechanism* is not platform-specific, not that the real fixture was losing responses on ubuntu or macos. The README states outright that why the real fixture only tripped on Windows is inferred. This branch is throwaway.
## Required outcome
- The PR description matches the corrected rationale, so the merge commit does not preserve a claim the branch already disproved.
- The inferred-vs-proven boundary is recorded somewhere that outlives the probe branch — the PR body is the cheapest place.
## Open questions for the human
- Is the Windows flake considered closed by this fix, or watched? The graceful close is unconditionally correct fixture hygiene and strictly safer, so it is worth landing either way, but it is not a proven fix for the observed failure — if the Windows leg trips again, the cause is still open.
- Round 1 flagged ~12 sibling fixtures in `vendor/aube/crates/aube-resolver/src/tests.rs` with the same read-one-chunk-then-drop shape and downplayed them because their bodies are small. With size retracted as the mechanism, that reasoning no longer holds: unread *request* bytes are the trigger and those fixtures read one chunk exactly as this one did. Whether that changes the follow-up calculus is your call.
Claude Opus ｜ 𝕏
| /// read returns `WouldBlock` before the bytes land, the path parses as `/`, and | ||
| /// the fixture serves a 404 for a package it holds — a second, independent bug. |
There was a problem hiding this comment.
The 404 outcome belongs to the old handler, not this one. It came from read(...).unwrap_or(0) swallowing the error into zero bytes; in serve_one a WouldBlock propagates through ?, so dropping set_nonblocking(false) would abort the handler before anything is written rather than serve a 404. Worth naming the mechanism the current code would actually hit, since the comment exists to tell the next reader what breaks if the line goes away.
| /// read returns `WouldBlock` before the bytes land, the path parses as `/`, and | |
| /// the fixture serves a 404 for a package it holds — a second, independent bug. | |
| /// read can return `WouldBlock` before the bytes land — which under the old | |
| /// handler's `unwrap_or(0)` parsed as `/` and served a 404 for a package the | |
| /// fixture holds, and here would abort the handler with no response at all. |
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
The age-floor install test failed on Windows CI with a transport error, not an HTTP status, fetching the tarball — the largest body served.
Cause: the fixture read one 4096-byte chunk of the request, wrote the response, and dropped the socket. Closing a socket that still holds unread bytes is an abortive close: it discards the send buffer, so a client mid-read never gets the rest.
The handler now reads the request to its end, half-closes, drains to EOF, and forces blocking mode on the accepted socket.
A probe on branch
probe/win-socket-close(4 MiB body, 15 requests, run 30473314496) lost every response on Windows, Ubuntu and macOS with the old close, none with the new.Refs #602