Skip to content

fix(core): retry an HTTP response read interrupted by a signal - #252

Open
rominf wants to merge 1 commit into
mainfrom
fix/http-read-retry-eintr
Open

fix(core): retry an HTTP response read interrupted by a signal#252
rominf wants to merge 1 commit into
mainfrom
fix/http-read-retry-eintr

Conversation

@rominf

@rominf rominf commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator
  • If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No matching rows.)

Summary

  • Treat EINTR as retryable in the bounded HTTP response reader instead of failing the request.
  • Why: a signal delivered to the reading thread makes an otherwise healthy endpoint report as unreachable. A managed service health probe can fail for no reason, and because a failed inference probe is latched onto the service record for the 15s retry interval, one interrupted read also makes every later readiness check in that window report the service as not ready.
  • Risk: low. One added match arm on an error kind that previously aborted the read; the existing deadline still bounds the total wait, so a signal storm cannot spin forever.

Root cause

read_http_response_bounded handled WouldBlock and TimedOut and treated every other read error as a transport failure. A signal delivered while the client is parked in read fails it with EINTR, which fell into that catch-all.

SA_RESTART does not prevent it. Linux never restarts a socket read that has a receive timeout set (signal(7), "Interruption of system calls"), and this reader sets one on every pass so the total wait stays bounded. Any signal handler in the process is enough to trigger it, and crossterm's SIGWINCH hook is linked into the CLI.

EINTR says nothing is wrong with the connection, so the read should simply be retried — which is what std's own read_to_end does, and why the sibling read_tcp_stream_to_string helper was never affected.

Verification

Found by instrumenting the client while reproducing a flaky test:

listing ERROR http://127.0.0.1:34091/v1 -> failed to read TCP stream
Caused by: Interrupted system call (os error 4)

Server-side timing showed 42ms from accept to read, ruling out a slow peer or a starved thread.

The regression test drives real signals at the reading thread with pthread_kill while it waits on a deliberately late response, with the handler installed using SA_RESTART to show the flag is not what protects the read. It fails without the fix, with the same "Broken pipe" symptom the original flake produced (the client aborts, so the server's write hits EPIPE).

In the test suite this surfaced as providers::tests::local_provider_default_chat_requires_builtin_qwen_assistant failing three different ways — Broken pipe, no_ready_local_service, and no_ready_builtin_assistant_service, all explained by that one aborted read. Over 20 consecutive cargo test -p rocm --bin rocm runs:

before after
failures 3/20 0/20

Also run: cargo clippy --locked --workspace --all-targets -- -D warnings, cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings, cargo test --workspace --all-targets --exclude e2e-cucumber, prek run --all-files.

Scope and gaps

  • No Gherkin scenario. The behaviour is user-observable in principle (a probe no longer fails at random), but it needs a signal to land inside a specific syscall window, which a scenario cannot trigger deterministically. Covered by the unit test instead; flagging the gap rather than leaving it implied.
  • The regression test is Linux-only. The guarantee it exercises — a receive timeout defeating SA_RESTART — is documented for Linux; BSD-derived kernels may restart the read, which would leave the test passing without reaching the retry. CI has no macOS runner to tell the difference. The fix itself is unconditional.
  • Not fixed here: read_close_delimited_sse_body in apps/rocm/src/providers.rs has the same gap on the chat-streaming path. Same class of bug, but I have no reproduction for it, so it is not folded into a fix whose repro I can demonstrate. Happy to follow up.

The bounded response reader treated every unrecognised `read` error as a
transport failure, so a signal delivered to the reading thread — which
fails the read with `EINTR` — reported a healthy endpoint as
unreachable. `SA_RESTART` does not prevent this: Linux never restarts a
socket read that has a receive timeout set, and the reader sets one on
every pass. Any handler in the process is enough, and `crossterm`'s
`SIGWINCH` hook is linked into the CLI.

`EINTR` says nothing is wrong with the connection, so read again; the
existing deadline still bounds the total wait.

The visible cost was a service health probe failing at random. Because a
failed inference probe is latched onto the service record for the
15s retry interval, one interrupted read also made every later check in
that window report the service as not ready. In the test suite this
showed up as `local_provider_default_chat_requires_builtin_qwen_assistant`
failing three ways — "Broken pipe", `no_ready_local_service`, and
`no_ready_builtin_assistant_service` — in 3 of 20 suite runs, and in
none of 20 after the change.

The regression test drives a real signal at the reading thread with a
handler installed with `SA_RESTART`, and fails without the fix.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf requested a review from a team as a code owner August 13, 2026 13:48
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.

1 participant