Skip to content

Keep transport cause in registry lookup errors (#1650) - #1652

Merged
Mikola Lysenko (mikolalysenko) merged 2 commits into
mainfrom
ci-janitor/upstream-error-cause-chain
Oct 22, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 2 commits into
mainfrom
ci-janitor/upstream-error-cause-chain

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

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 was poetry::poetry_hosted_fresh_install_then_manifestless_vex. Its rollback was refused:

six==1.16.0: GET https://pypi.org/pypi/six/1.16.0/json: error sending request for url (https://pypi.org/pypi/six/1.16.0/json)

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)

UpstreamClient and registry_fetch::download format a transport error with reqwest's Display. 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_authorized and registry_fetch::download now go through api::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 that network_error_detail becomes pub(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

  • New test upstream::client::tests::a_failed_lookup_keeps_the_transport_cause sends get_json and download to a held loopback server that hangs up after each request head, and requires hyper's connection closed before message completed cause in the error (200/200 local runs pass).
    • Without the fix it fails: GET http://127.0.0.1:43369/...: error sending request for url (...).
    • With the fix it passes.
  • cargo test -p socket-patch-core --lib -- upstream::client::tests api::client::tests::network_error_detail vendor::registry_fetch: 52 passed.
  • rustfmt --check passes on the 3 touched files.
  • cargo clippy -p socket-patch-core --all-targets -- -D warnings reports nothing in the touched files. With local toolchain 1.93.1 it reports 26 errors that already exist on origin/main, in other files such as jvm_jar.rs and bun_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_authorized send/body errors and registry_fetch download send errors route through the existing network_error_detail helper (now pub(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 completed for both JSON GET and download paths.

Reviewed by Cursor Bugbot for commit e63ec16. Configure here.


Generated by Claude Code

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
@mikolalysenko Mikola Lysenko (mikolalysenko) added the ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) label Oct 21, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor 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.

Stale Bugbot comment from a previous run.

@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

coverage (and therefore ci-ok) failed on 4f7319c in e2e_redirect_cargo_build. The runner could not reach static.crates.io (Could not connect to server, 3 cargo retries). That is an external network failure. This PR only changes how registry lookup errors are worded in socket-patch-core and does not touch that test. The failed jobs were already re-run once at 04:40Z, and the flake itself is tracked by #1654. Bugbot found no issues.


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 21, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review.

  • Head: 4f7319c
  • CI: 35/35 checks green (19 skipped); an earlier coverage failure on this head was a crates.io/static.crates.io network blip and passed on re-run
  • Bugbot: reviewed this head (check Cursor Bugbot success), no findings, no open review threads
  • Mergeable, no conflicts; no CHANGELOG.md change

Slack announcement not sent: this run has no Slack send tool.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at 4f7319c.

  • CI: 35/35 checks green on the head (19 skipped), no main-wide failures.
  • Bugbot: reviewed 4f7319c, no findings; no open review threads.
  • Mergeable against main, no CHANGELOG changes.
  • Slack announcement not sent this run (no Slack send tool available); the next run will retry.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

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 network_error_detail helper, so the error carries the cause chain ("connection refused", reset, timeout, DNS) instead of stopping at reqwest's "error sending request for url (...)". Diagnostics only (step 1 of #1650): no retry, no behavior change beyond the error text.

Risk: low. Three one-line map_err changes plus a visibility bump (fn → pub(crate) fn). URLs in the added cause text go through the same redact_urls_in the patch API client already uses, and registry_fetch::download still redacts the whole message afterwards.

Look here

  • upstream/client.rs:253 and :261:``` send and body-read errors
  • registry_fetch.rs:961: vendored download send error
  • api/client.rs:41: the reused helper (cause chain + redaction)
  • upstream/client.rs:906-924:```` new test against a closed local port; asserts "refused" in both get_json and `download` errors (Windows' "actively refused it" text also matches)

Verified. Read the full diff and the helper; checked that download's outer redaction still applies and that no test or workflow changed. CI 54/54 settled on 4f7319c (35 success, 19 path-skipped), ci-ok and clippy green. Bugbot reviewed 4f7319c: no findings; no review threads. Mergeable, up to date with main, CHANGELOG.md untouched.

Changes I made. None.

Open questions. None.

Auto-merge is armed, so approving sends it straight to the merge queue.


Generated by Claude Code

@manfromnowhere143

Copy link
Copy Markdown

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.

@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 21, 2026
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
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

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 tokio::time::timeout, and the assertion checks hyper's connection closed before message completed cause, which the old error text dropped. Without the fix, the test still fails with the bare error sending request for url (...). With the fix, it passed 200 of 200 runs locally.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

1 similar comment
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 21, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at e63ec16.

  • CI: 34/34 checks green on the head (19 skipped), no main-wide failures. Merges cleanly.
  • Bugbot: reviewed e63ec16, no findings (the earlier review of 4f7319c had none either).
  • Reviewer note: manfromnowhere143 pointed out that the cause-chain test released its port too early. Fixed in e63ec16: the test now keeps the loopback listener open.
  • Slack: not announced. This run had no Slack send tool.

Generated by Claude Code

@cursor cursor 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.

✅ 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.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What 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 network_error_detail helper, so the message keeps the transport cause (refused, reset, DNS, TLS, timeout) with URLs redacted. Diagnostics only, for the #1650 nightly flake; no retry or behavior change.

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 pub(crate).

Look here

  • upstream/client.rs:253 and :261:``` send / body-read errors
  • registry_fetch.rs:961: download send error
  • upstream/client.rs:906-950:```` new test with a held loopback listener that hangs up
  • api/client.rs:41: visibility change

Verified

  • Read the full diff and both commits. CHANGELOG.md is untouched. No debug leftovers.
  • Ran the new test locally: passes. With the two map_err changes reverted it fails with the old "error sending request for url" text, so the test does cover the fix.
  • The test holds its port for its whole run, with 30 s deadlines, so it doesn't race other tests for the port.
  • CI: 35/35 green on e63ec16b (19 skipped). ci-ok and clippy pass. Cursor Bugbot passes. No unresolved threads.

Changes I made: none.
Open questions: none.

Auto-merge is armed, so approving sends this straight to the merge queue.


Generated by Claude Code

@manfromnowhere143

Copy link
Copy Markdown

Checked the revised test at e63ec16b62c784aa0c959ac457ec1cf68ef2f415. Moving the listener into the serving task keeps the port owned across both exchanges, and each client operation has its own 30-second timeout. The assertion now checks the cause produced by that controlled peer.

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.

Merged via the queue into main with commit bc307ce Oct 22, 2026
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants