Skip to content

fix(sources): bound the HTTP timeouts so a stalled peer cannot hang a command - #221

Open
luantaraschi wants to merge 1 commit into
eljulians:masterfrom
luantaraschi:fix/http-timeouts
Open

fix(sources): bound the HTTP timeouts so a stalled peer cannot hang a command#221
luantaraschi wants to merge 1 commit into
eljulians:masterfrom
luantaraschi:fix/http-timeouts

Conversation

@luantaraschi

@luantaraschi luantaraschi commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Closes #196.

UreqClient::new only configured redirect_auth_headers, and ureq 3.4 leaves every entry of Timeouts at None, so a peer that accepts the socket and never answers holds the command open with no way out.

What changed:

  • connect 10s, recv_response 30s and recv_body 60s on the agent config.
  • a timeout now reads timed out fetching <url> (receive response) instead of ureq's own timeout: receive response fetching <url>. The same helper covers post_json and post_json_with_bearer, which had the identical catch-all.

Two shape decisions, both easy to reverse if you disagree:

Per phase instead of timeout_global. Your sketch suggests timeout_connect plus timeout_global. A global cap also bounds legitimate transfers, so a large directory entry on a slow link would start failing once it crosses the cap. Bounding each phase kills the hang (the reported case is "accepts and never answers", which is recv_response) without putting a ceiling on healthy downloads. Worst case against a wedged endpoint is now about 100s per request instead of forever. Say the word and I swap it for a global timeout.

No env override. The issue says "possibly env-overridable". I left it out: it would be a new public knob to carry in the README and SPEC, and with per phase timeouts nothing legitimate is cut off. Happy to add SKILLFILE_HTTP_TIMEOUT if you want the escape hatch.

Tests

Three unit tests in crates/sources/src/http.rs. The first two were written before the fix:

  • new_client_bounds_every_transport_phase reads the agent config back. On master it fails with left: None, right: Some(10s).
  • get_bytes_gives_up_on_a_peer_that_never_answers is your repro, as a TcpListener that accepts and stalls. Measured with the timeouts removed from the agent config: the call waited 3.001s, which is exactly how long the test peer stayed alive, so against a real wedged server it waits forever. With the fix and a 500ms configured timeout it returns an error in 0.5s.

The private with_timeouts constructor exists so that second test does not have to sit through the 30s default.

  • a_refused_connection_keeps_the_existing_wording points all four entry points at a closed loopback port and pins the non timeout wording (fetching <url>, posting to <url>), so the new error arm cannot quietly swallow the ordinary failures. Added after the first CI run, where codecov/patch flagged those arms as uncovered.

Gates

Run in a rust:1.97 container against this branch:

  • cargo fmt --check clean
  • cargo clippy --workspace --all-targets -- -D warnings clean
  • cargo test --workspace clean, 18 test binaries, no failures

cargo deny and cargo machete were not run here. The change adds no dependency.

This contribution was made with AI assistance. I reviewed the change and ran every command quoted above.

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.53%. Comparing base (bcaccc4) to head (0940ac7).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   92.31%   92.53%   +0.21%     
==========================================
  Files          42       42              
  Lines       23186    23274      +88     
==========================================
+ Hits        21405    21537     +132     
+ Misses       1781     1737      -44     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

… command

ureq leaves every timeout at None, so any endpoint that accepts the socket
and then never answers blocks install, add and search forever, with a
spinner on a TTY and in silence otherwise. search_all queries registries in
sequence, so one wedged registry also blocks the others.

Bound connect, receive response and receive body on the agent config, and
name the timeout in the error instead of leaking ureq's phrasing.

The phases are bounded one by one rather than with a single global timeout
so that a slow but healthy download is not cut off midway.

Closes eljulians#196
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.

no HTTP timeouts anywhere - a stalled server hangs install/add/search forever

1 participant