Repository navigation
Keep transport cause in registry lookup errors (#1650) - #1652
Conversation
A failed upstream registry lookup quoted only reqwest's top-level text, "error sending request for url (...)", which drops the part that says what went wrong (refused, reset, DNS, TLS, timeout). The nightly poetry hosted-rollback flake in #1650 could not be classified for that reason, and a user whose hosted rollback is refused gets the same unhelpful message. Format the send and body-read errors in UpstreamClient::get_json and registry_fetch::download with api::client::network_error_detail, the helper the patch API client already uses. It appends the cause chain and redacts URLs. Add a test that a refused connection's error now says "refused". Refs #1650 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012VpH5sUqHcYE49yNX1k2Fw
|
bugbot run Generated by Claude Code |
|
Generated by Claude Code |
|
Burn-down agent: labeled Ready for review.
Slack announcement not sent: this run has no Slack send tool. Generated by Claude Code |
|
Burn-down agent: labeled Ready for review at
Generated by Claude Code |
Final review brief (for the approver)What it does. Registry lookups in the hosted upstream client and the vendored download path now format transport errors with the existing Risk: low. Three one-line Look here
Verified. Read the full diff and the helper; checked that Changes I made. None. Open questions. None. Auto-merge is armed, so approving sends it straight to the merge queue. Generated by Claude Code |
|
One test detail at 4f7319c: a_failed_lookup_keeps_the_transport_cause drops its TcpListener before either request, so the comment describing connection refusal as deterministic is stronger than the endpoint ownership establishes. Another process can bind the released port, and the two requests can then observe different failures or an unexpected response. For this diagnostics test, a controlled loopback server that retains the port and deliberately fails each exchange would make the cause preservation assertion independent of port reuse. Connection refused can remain a separate classification case where the harness can guarantee endpoint ownership. An explicit deadline would also bound a failed test if the peer behaves unexpectedly. This is a source review observation about the new test, not a reproduced failure of the PR or an explanation of the original PyPI flake. |
The test dropped its TcpListener before connecting, so another process could bind the freed port and the two requests could see a different failure or a real response. Serve the port from a held loopback listener that reads each request head and hangs up, bound each request with a 30 s deadline, and assert hyper's "connection closed before message completed" cause instead of "refused". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012VpH5sUqHcYE49yNX1k2Fw
|
Thanks, you're right: dropping the listener frees the port, so "deterministic" was overstated. Fixed in e63ec16. The test now keeps a loopback listener for its whole run. The listener reads each request head and then hangs up. Each request is bounded by a 30 s Generated by Claude Code |
|
bugbot run Generated by Claude Code |
1 similar comment
|
bugbot run Generated by Claude Code |
|
Burn-down agent: labeled Ready for review at
Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e63ec16. Configure here.
Final review briefWhat it does. Registry lookup and download errors in the hosted/redirect path used to stop at reqwest's "error sending request for url (...)". They now go through the existing Risk: low. Three-file change to error-string formatting. The helper is the one the patch API client already uses; the only API change is making it Look here
Verified
Changes I made: none. Auto-merge is armed, so approving sends this straight to the merge queue. Generated by Claude Code |
|
Checked the revised test at That addresses my fixture-ownership concern; I have no further request from this source review. I have not rerun the Rust test or your 200-trial check. The original PyPI failure class remains a separate question for the new diagnostics. Thanks for the focused correction. |
Problem
The nightly leg
e2e-extended (ubuntu-latest, e2e_vex_build, poetry:: --ignored, 1.1.15)failed once (run 39101574188, job 117599202945, 2026-10-18). The failing test waspoetry::poetry_hosted_fresh_install_then_manifestless_vex. Its rollback was refused:That is 1 failure in the last 12 nightlies. It is tracked in #1650. It is the only flake seen on main, merge_group or PR runs since 2026-10-17. The only other failures were the uv 0.13.2 pipenv pylock break, already fixed on main by #1605, and the real behavior failures on #1646.
Root cause (why it can't be fixed yet)
UpstreamClientandregistry_fetch::downloadformat a transport error with reqwest'sDisplay. That text ends at "error sending request for url (...)" and drops the cause chain. So the log can't tell us whether PyPI refused the connection, reset it, failed DNS or timed out. #1650 proposes a bounded retry, but only if the failure turns out to be a dropped connection or a 429/503. Adding a retry before we know the error class would be guessing.Fix
This PR adds diagnostics only (step 1 of #1650). The send and body-read errors in
UpstreamClient::get_json_authorizedandregistry_fetch::downloadnow go throughapi::client::network_error_detail. The patch API client already uses that helper: it appends the cause chain and redacts URLs. The only visibility change is thatnetwork_error_detailbecomespub(crate). Users also benefit: a refused hosted restore now says why the registry lookup failed.No retry is added and no test is removed, moved or weakened. Workflows and required checks are unchanged.
Proof
upstream::client::tests::a_failed_lookup_keeps_the_transport_causesendsget_jsonanddownloadto a held loopback server that hangs up after each request head, and requires hyper'sconnection closed before message completedcause in the error (200/200 local runs pass).GET http://127.0.0.1:43369/...: error sending request for url (...).cargo test -p socket-patch-core --lib -- upstream::client::tests api::client::tests::network_error_detail vendor::registry_fetch: 52 passed.rustfmt --checkpasses on the 3 touched files.cargo clippy -p socket-patch-core --all-targets -- -D warningsreports nothing in the touched files. With local toolchain 1.93.1 it reports 26 errors that already exist onorigin/main, in other files such asjvm_jar.rsandbun_binary.rs.Refs #1650
Generated by Claude Code
Note
Low Risk
Diagnostics-only change to error string formatting on failed HTTP sends; no retry, auth, or success-path behavior changes.
Overview
Registry lookup and artifact download failures now surface full reqwest transport cause chains (e.g. connection reset, DNS, TLS) instead of stopping at generic “error sending request for url” text.
Upstream
get_json_authorizedsend/body errors and registry_fetchdownloadsend errors route through the existingnetwork_error_detailhelper (nowpub(crate)), which appends causes and redacts URLs like the patch API client already does. No retry logic is added—this is diagnostics-only for #1650.A new integration test asserts that a loopback server that hangs up mid-request yields errors containing hyper’s
connection closed before message completedfor both JSON GET and download paths.Reviewed by Cursor Bugbot for commit e63ec16. Configure here.
Generated by Claude Code