fix(inference): idle-timeout recovery + guaranteed terminal event on a stalled stream

[M3gA-Mind]

What & why

A streaming turn with no tools/sub-agents could freeze mid-generation and orphan itself with no terminal event (#4761): the base inference stream streamed some text, went silent, emitted only heartbeats, and the connection was dropped ~250s later with no chat_done and no chat_error → empty final reply.

Root cause

sse_bytes_to_chunks (src/openhuman/inference/provider/compatible_stream.rs) read the upstream byte stream with a bare while let Some(item) = bytes_stream.next().await and no idle timeout. When the backend established the SSE stream and then stalled (connection held open, no further bytes), next().await blocked forever:

• the read loop never exited, so the terminal final_chunk() never ran;
• the consumer (compatible_provider_impl.rs, then the tinyagents ProviderModel adapter that wraps openhuman's Provider) blocked waiting for a chunk that never came;
• no terminal event was ever produced.

reqwest had no read/idle timeout configured either, so nothing caught it until the far end dropped the socket much later. The harness lives in tinyagents but the actual backend byte-streaming is openhuman's Provider, so the gap — and the fix — are here.

Fix

• compatible_stream.rs — a small generic helper read_next_or_idle wraps each byte read in tokio::time::timeout. On a stall it emits a terminal StreamError::IdleTimeout and returns — so the success final_chunk() is not emitted after the error and a stalled generation can no longer be masked as a normal finish_reason: stop. The same convergence is applied to the other terminal error branches (UTF-8 decode, HTTP transport, SSE parse): each now returns after sending its Err, so final_chunk() is reached only on a clean EOF. A terminal event is now guaranteed on stall/drop. The idle window reuses the single shared per-chunk inactivity watchdog (stream_idle_timeout(), Research agent intermittently hangs mid-run #4269) rather than a second divergent resolver, so OPENHUMAN_INFERENCE_STREAM_IDLE_TIMEOUT_SECS means the same thing on both the SSE and native-stream paths: default 90s, bounded 1..=3600, with a missing/non-numeric/out-of-range value (e.g. 0 or 99999) falling back to the 90s default — the guard can't be typo'd into disabling or wedging a turn indefinitely. Every streaming method routes through sse_bytes_to_chunks, so this covers the whole streaming surface.
• traits.rs — new StreamError::IdleTimeout(Duration) variant.
• error_classify.rs — classifies IdleTimeout as retryable (a stall is a transient upstream condition): the reliable layer can retry a not-yet-committed stall on a fresh connection, and once retries are exhausted it still surfaces as a terminal chat_error — the whole point being that the turn can no longer hang with no terminal event.
• compatible.rs / compatible_timeout.rs — widen the module + stream_idle_timeout() getter to pub(crate) so the shared bounded resolver is reachable from the SSE path (compatible_stream is included as a module under both provider and provider::compatible, so the getter is referenced by its crate::-absolute path, which resolves in both inclusions).

Tests

Four unit tests on the idle helper read_next_or_idle: stall → IdleTimedOut, data → Item, clean end → Ended, and guard-disabled (None) still yields items. The shared resolver's bounds/parse behaviour is covered by the existing compatible_timeout tests.

The ~19s TTFT noted in the issue is upstream/staging infra (called out there as not the code gap) and is not addressed here.

Closes #4761

Summary by CodeRabbit

• New Features

  • Added an idle-timeout safeguard for OpenAI-compatible streaming to detect stalled mid-response reads and end promptly when configured.

• Bug Fixes

  • Introduced an IdleTimeout streaming error and ensured it’s classified as retryable.
  • Improved stream handling so stalled/terminal failures can’t be overridden by a later “final” chunk emission.

• Tests

  • Added unit coverage for item arrival, clean end-of-stream, and stalled reads (including behavior when idle-timeout is disabled).

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a mid-stream idle-timeout guard to the OpenAI-compatible SSE stream reader, introducing a new StreamError::IdleTimeout variant, timeout-bounded reads, retry classification, terminal error returns, and unit tests.

Changes

Idle Stream Timeout Handling

Layer / File(s)	Summary
IdleTimeout error contract and retry classification src/openhuman/inference/provider/traits.rs, src/openhuman/inference/provider/error_classify.rs, src/openhuman/inference/provider/compatible.rs, src/openhuman/inference/provider/compatible_timeout.rs	Adds StreamError::IdleTimeout(Duration), classifies it as retryable, and widens idle-timeout configuration visibility within the crate.
Idle-aware SSE stream processing src/openhuman/inference/provider/compatible_stream.rs	Adds timeout-aware reads, reports stalled streams, and prevents final success chunks after decoding or transport errors.
Idle-read behavior validation src/openhuman/inference/provider/compatible_stream.rs	Tests stalled, item, ended, and disabled-timeout outcomes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• tinyhumansai/openhuman#4393: Implements a similar per-chunk SSE inactivity watchdog and idle-timeout error path.

Suggested labels: rust-core, bug

Suggested reviewers: sanil-23

Poem

A rabbit watched the stream go still,
Then hopped to wake the sleepy hill.
If bytes don’t come, the timeout sings,
And retry hops follow little rings.
🐇⏱️

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The change implements idle-timeout recovery and terminal-error propagation for stalled core inference streams, matching issue #4761's main code requirement.
Out of Scope Changes check	✅ Passed	The visibility tweaks and retry classification support the stream-timeout fix and don't introduce unrelated functionality.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main fix: idle-timeout handling and terminal event guarantees for stalled inference streams.

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d6bb05dc0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[M3gA-Mind]

CI guardian: ran cargo fmt on src/openhuman/inference/provider/compatible_stream.rs (the only cargo fmt --check diff) — fixes the Rust Quality fmt failure.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/inference/provider/compatible_stream.rs`:
- Around line 78-90: The error-handling in compatible_stream::stream still falls
through to final_chunk() in the String::from_utf8(...) and HTTP Err(e) branches,
which can misreport failures as normal completion. Update those branches in the
stream processing logic to return immediately after sending Err(...) to tx,
matching the IdleRead::IdleTimedOut path, so decoding/transport errors terminate
the stream without emitting a success chunk.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7130f521-dd46-4f47-bb67-04b16b99a37d

📥 Commits

Reviewing files that changed from the base of the PR and between 7d6bb05 and dc7ddea.

📒 Files selected for processing (3)

• src/openhuman/inference/provider/compatible_stream.rs
• src/openhuman/inference/provider/error_classify.rs
• src/openhuman/inference/provider/traits.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/openhuman/inference/provider/traits.rs
• src/openhuman/inference/provider/error_classify.rs

[M3gA-Mind]

CI guardian: fixed the E0433 my review-fix introduced — the SSE path referenced super::compatible_timeout::stream_idle_timeout() but that resolver lives at compatible::compatible_timeout and was pub(super). Widened the module + fn to pub(crate) and corrected the path so both stream paths share the one bounded resolver.

[M3gA-Mind]

CI guardian: root-caused the E0433 — compatible_stream is declared as a module under both provider (mod.rs) and provider::compatible (compatible.rs), so any super::-relative path to the resolver only compiles in one inclusion. Switched to the crate::-absolute path (resolver already widened to pub(crate)), which compiles in both.

[YellowSnnowmann]

Verdict: comment-only — the feature is satisfied and CI is green, but one substantive reviewer thread is still open and the description drifted from the merged code.

Feature satisfaction: yes. The #4761 invariant ("a terminal event must always fire on a stalled/dropped stream") is delivered. read_next_or_idle wraps each byte read in a tokio::time::timeout; on a stall it sends Err(StreamError::IdleTimeout) and, importantly, returns so the unconditional Ok(final_chunk()) is not emitted after the error. The terminal StreamError propagates through the consumers (compatible_provider_impl.rs:1166-1170, reliable.rs:853-857) to the harness, which maps it to chat_error (task_dispatcher/registry.rs:58, executor.rs:245). Error handling matches repo convention — an upstream stall is surfaced as a terminal error, not swallowed. Tests cover stall/item/clean-end/disabled/parse.

Findings:

• Minor (pre-existing, not introduced here): the String::from_utf8 UTF-8 branch and the outer HTTP Err(e) branch in compatible_stream.rs still break and fall through to tx.send(Ok(final_chunk())). Since consumers forward chunks without stopping on Err, a real decode/transport failure can be followed by a finish_reason: stop. git blame confirms both breaks predate this PR (commit 7741c5815); this PR did not touch or widen them, and its own new path correctly uses return. This is exactly CodeRabbit's still-open Major thread and matches your own deferral note. The fix is break->return on those two arms — ~2 lines, directly adjacent to the changed code. Either fold it in here or reply on the CodeRabbit thread with the follow-up so it isn't left silently unresolved.
• Nit: the description no longer matches the code. It says the idle window defaults to 120s, that 0 disables the guard, that the env var is new, and that the reliable layer retries the stall. Actual: default 90s, 0 falls back to 90 (MIN=1, never disables), OPENHUMAN_INFERENCE_STREAM_IDLE_TIMEOUT_SECS is pre-existing (Research agent intermittently hangs mid-run #4269), and retry only applies to a pre-commit stall — the [agent][inference] Base inference stream stalls mid-generation, no keepalive/recovery → orphaned turn + empty reply #4761 mid-generation stall is guaranteed-terminal but not retried. Please update so operators aren't misled.

Thread status:

• codex P2 (suppress final_chunk after idle timeout) — addressed via return, resolved.
• codex P2 (bound the env override) — addressed by reusing the bounded stream_idle_timeout() (1..=3600), resolved.
• author deferral note (other break arms) — accurate, deferred.
• CodeRabbit Major (return from the other error branches) — still unresolved; valid but pre-existing/out-of-scope. Resolve or reply-and-link before merge.

[YellowSnnowmann]

Approving — the previously-flagged blocker is fixed, CI green, CodeRabbit re-approved, all threads resolved, feature satisfied.

[M3gA-Mind]

⚠️ Rebase onto main is blocked — needs a maintainer decision (not a mechanical conflict).

Main's #4784 (c58fe10b5, Phase 3 P3-B — crate-native turn path + delete compatible*.rs [WIP]) deleted the entire hand-rolled SSE parser this PR fixes. All three files that carry the fix — compatible.rs, compatible_stream.rs, compatible_timeout.rs — no longer exist on main; the reqwest bytes_stream() path was replaced by a crate-native (async-openai) turn path. The idle-bounded read (read_next_or_idle/IdleRead), the shared stream_idle_timeout() resolver, and the stall→terminal-error handling all attach to code that is gone.

Re-applying the fix is therefore a design task, not a rebase: either (a) re-implement idle-timeout + stall-termination in the new crate-native streaming path (first confirming #4761 still reproduces there), or (b) resurrecting compatible\*.rs — which would revert #4784's intentional refactor and is not appropriate. Since #4784 is marked [WIP], the target streaming architecture may still be in flux.

Holding the rebase pending a maintainer call on which streaming path #4761's idle-timeout guard should live in. The two surviving one-liners in this PR (error_classify.rs, traits.rs) rebase cleanly but are inert without the stream logic.

[M3gA-Mind]

Closing this PR. It could not be cleanly rebased onto the current main — the base inference streaming path was substantially reworked by the recent orchestration refactor (#4738, hosted-only brain), producing a semantic conflict rather than a mechanical one. Rather than force a risky reconciliation of a stale fix against the new architecture, we're closing this and its issue #4761; if the stream-stall/keepalive gap still reproduces on the new hosted-brain inference path, it should be re-diagnosed and fixed fresh against that code.