fix(agent): arm harness wall-clock ceiling so a wedged turn can't ship an empty reply#4751
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a configurable per-turn wall-clock timeout for web chat turns, applies it to main and parallel execution, classifies timeout failures as retryable ChangesWeb turn timeout backstop
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Client
participant TurnRunner as start_chat / spawn_parallel_turn
participant Deadline as run_turn_under_cancel_and_deadline
participant Task as run_chat_task
participant Classifier as classify_inference_error
Client->>TurnRunner: initiate chat turn
TurnRunner->>Deadline: run with cancellation and deadline
Deadline->>Task: await turn future
alt turn completes before deadline
Task-->>Deadline: turn result
Deadline-->>TurnRunner: forward result
TurnRunner-->>Client: terminal chat event
else deadline elapses
Deadline-->>TurnRunner: synthetic turn timeout error
TurnRunner->>Classifier: classify timeout
Classifier-->>TurnRunner: retryable turn_timeout
TurnRunner-->>Client: terminal chat_error
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 270febc1f6
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/openhuman/channels/providers/web_errors.rs (1)
160-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment omits two existing
error_typevalues.The enumerated list is missing
session_expired(returned at Line 544) andnetwork(returned at Line 888) — pre-existing gaps, but since this line is being edited to addturn_timeoutanyway, worth completing the list while touching it.🤖 Prompt for 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. In `@src/openhuman/channels/providers/web_errors.rs` at line 160, The doc comment for the `error_type` list is incomplete; update the `web_errors.rs` comment near the existing enum documentation to include all supported values. When editing the `error_type` documentation, add the missing `session_expired` and `network` entries alongside `capability_unsupported`, `empty_response`, `turn_timeout`, and `inference` so the comment matches the values returned by the relevant error handling paths.src/openhuman/channels/providers/web/ops.rs (2)
62-113: 🩺 Stability & Availability | 🔵 TrivialSolid backstop design; one operational caveat worth flagging.
drive_turn_with_deadlinecorrectly drops the wedged future on elapse, which cooperatively tears down anything polled directly withinrun_chat_task's own future tree. However, ifrun_chat_task(or a delegated sub-agent) uses detachedtokio::spawninternally rather than being polled inline, dropping this outer future won't cancel that background work — it will keep running (and could still touch shared state, IN_FLIGHT-adjacent resources, or emit late events) even after the client has already received theturn_timeoutchat_error. The linked issue explicitly scopes "underlying sub-agent slowness" out, so this is likely accepted as-is, but it's worth confirming no detached spawns exist on the hot path this backstop targets, to avoid stray late side effects after the terminal event is sent.🤖 Prompt for 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. In `@src/openhuman/channels/providers/web/ops.rs` around lines 62 - 113, `drive_turn_with_deadline` only cancels the outer future, so detached work spawned inside `run_chat_task` or any delegated sub-agent may keep running after a `turn_timeout` is emitted. Review the hot path in `run_chat_task` and related web channel task/sub-agent code for any `tokio::spawn` usage, and either eliminate those detached spawns or wire them into explicit cancellation so the timeout backstop fully stops late side effects after the terminal event.
844-862: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared select!+deadline+run_chat_task wiring.
This block is a near-exact duplicate of the
start_chatwiring at Lines 613-631 (sameselect! { biased; cancel => None, drive_turn_with_deadline(web_turn_deadline(), with_origin(...)) => Some(res) }shape, differing only in theforkflag and queue handle). Since this PR touches both sites identically, it's a good opportunity to extract a small helper (e.g.run_turn_under_cancel_and_deadline(cancel_token, origin, approval_ctx, fut)) to avoid the two copies drifting apart later.🤖 Prompt for 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. In `@src/openhuman/channels/providers/web/ops.rs` around lines 844 - 862, The cancel/deadline/run_chat_task wiring in the web channel ops code is duplicated between the shared turn path and start_chat, so extract it into a small helper to keep both call sites aligned. Refactor the repeated select!/drive_turn_with_deadline/with_origin/APPROVAL_CHAT_CONTEXT.scope pattern into a reusable function or wrapper (for example around run_turn_under_cancel_and_deadline), and keep the only differences as the fork flag and queue handle passed into run_chat_task.src/openhuman/channels/providers/web_tests.rs (1)
1986-2041: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup (test hook + env var) isn't panic-safe.
If any
assert!/assert_eq!/.expect()between lines 2019-2033 panics, execution never reachesset_test_run_chat_task_block(None)/env::remove_varat Line 2039-2040. The leaked global test hook andOPENHUMAN_WEB_TURN_TIMEOUT_SECS=1env var would then leak into whatever test runs next in this binary, turning one failure into a cascade of unrelated flaky failures.Wrap the setup/teardown in an RAII guard so cleanup runs on unwind too.
♻️ Proposed fix using an RAII guard
+struct BackstopTestGuard; +impl Drop for BackstopTestGuard { + fn drop(&mut self) { + // Best-effort synchronous reset; async cleanup (`set_test_run_chat_task_block`) + // still needs an explicit await somewhere reachable, or move the hook to a + // sync `OnceLock`/`Mutex` so Drop can clear it directly. + std::env::remove_var("OPENHUMAN_WEB_TURN_TIMEOUT_SECS"); + } +} + async fn wedged_turn_hits_wall_clock_backstop_and_emits_turn_timeout_chat_error() { let _serial = FORCED_ERROR_TEST_LOCK.lock().await; std::env::set_var("OPENHUMAN_WEB_TURN_TIMEOUT_SECS", "1"); + let _guard = BackstopTestGuard; let block = make_block(); set_test_run_chat_task_block(Some(block.clone())).await;🤖 Prompt for 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. In `@src/openhuman/channels/providers/web_tests.rs` around lines 1986 - 2041, The test setup for FORCED_ERROR_TEST_LOCK, set_test_run_chat_task_block, and OPENHUMAN_WEB_TURN_TIMEOUT_SECS is not panic-safe, so a failure in the assertions or timeout path can leak the global hook and env var into later tests. Wrap the temporary state in an RAII guard inside the backstop test so teardown happens on unwind, and make the guard clear the test block and remove the env var automatically even if the assertions around recv/error_type/error_retryable panic.
🤖 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.
Nitpick comments:
In `@src/openhuman/channels/providers/web_errors.rs`:
- Line 160: The doc comment for the `error_type` list is incomplete; update the
`web_errors.rs` comment near the existing enum documentation to include all
supported values. When editing the `error_type` documentation, add the missing
`session_expired` and `network` entries alongside `capability_unsupported`,
`empty_response`, `turn_timeout`, and `inference` so the comment matches the
values returned by the relevant error handling paths.
In `@src/openhuman/channels/providers/web_tests.rs`:
- Around line 1986-2041: The test setup for FORCED_ERROR_TEST_LOCK,
set_test_run_chat_task_block, and OPENHUMAN_WEB_TURN_TIMEOUT_SECS is not
panic-safe, so a failure in the assertions or timeout path can leak the global
hook and env var into later tests. Wrap the temporary state in an RAII guard
inside the backstop test so teardown happens on unwind, and make the guard clear
the test block and remove the env var automatically even if the assertions
around recv/error_type/error_retryable panic.
In `@src/openhuman/channels/providers/web/ops.rs`:
- Around line 62-113: `drive_turn_with_deadline` only cancels the outer future,
so detached work spawned inside `run_chat_task` or any delegated sub-agent may
keep running after a `turn_timeout` is emitted. Review the hot path in
`run_chat_task` and related web channel task/sub-agent code for any
`tokio::spawn` usage, and either eliminate those detached spawns or wire them
into explicit cancellation so the timeout backstop fully stops late side effects
after the terminal event.
- Around line 844-862: The cancel/deadline/run_chat_task wiring in the web
channel ops code is duplicated between the shared turn path and start_chat, so
extract it into a small helper to keep both call sites aligned. Refactor the
repeated
select!/drive_turn_with_deadline/with_origin/APPROVAL_CHAT_CONTEXT.scope pattern
into a reusable function or wrapper (for example around
run_turn_under_cancel_and_deadline), and keep the only differences as the fork
flag and queue handle passed into run_chat_task.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18578e58-a3bb-4997-bfb3-f89aa0d98db8
📒 Files selected for processing (3)
src/openhuman/channels/providers/web/ops.rssrc/openhuman/channels/providers/web_errors.rssrc/openhuman/channels/providers/web_tests.rs
M3gA-Mind
changed the title
|
Pushed a follow-up (2dd12b1) that moves the fix to the root cause after checking Finding: the tinyagents harness is not buggy — it already bounds every model/tool/sub-agent call with What changed since the first push:
No changes to the vendored crate — the fix is entirely in the openhuman adapter, so this stays a single openhuman PR (no separate tinyagents PR). |
|
CI guardian: ran rustfmt on the wedged-turn retry |
M3gA-Mind
force-pushed
the
fix/GH-4746-turn-wall-clock-backstop
branch
from
9b06422 to
c2dd5d7
Compare
Verdict: changes requested (feature works, one Sentry-taxonomy issue to fix)Feature: satisfied. The RCA is correct and the fix targets the true root cause. Major — turn_timeout still pages Sentry as an unexpected error
So every turn that trips the ceiling emits a spurious Sentry event — contradicting this PR's own framing of Minor — parked-approval cleanup on external teardown (codex P2)Real interaction, and you've correctly triaged it as an out-of-scope follow-up: when the backstop drops the future while a tool is parked in Nit
CI is fully green. |
|
Thanks for the thorough review @YellowSnnowmann — addressed in Major — turn_timeout no longer pages Sentry. You're right: the Fix: extracted a pure Minor — parked-approval cleanup follow-up filed: #4774 (RCA, blast radius, proposed Nit — |
…nal event A web chat turn ran in a tokio::select! with only cancel + run_chat_task arms, so if the turn future never resolved (a main agent wedged mid tool-call, or a delegated sub-agent that streams but never returns a completion), run_chat_task hung forever: the progress bridge kept beating inference_heartbeat until the socket died and NO chat_done/chat_error was ever emitted, delivering an empty string to the client. Wrap both the primary and parallel/fork turn futures in a wall-clock backstop (default 600s, env OPENHUMAN_WEB_TURN_TIMEOUT_SECS, 0 disables). On elapse the wedged future is dropped (cooperative teardown at its next await) and a synthetic turn_timeout error flows through the existing Err arm, guaranteeing a graceful, retryable chat_error terminal event. Add a dedicated turn_timeout classification branch so the user sees actionable copy instead of the generic error. The iteration-cap path already degrades gracefully (core.rs synthesizes a checkpoint/summary); this closes the remaining never-returns gap. Closes tinyhumansai#4746
…p an empty reply
Root cause (openhuman adapter, not the harness): run_policy_for set the
iteration caps but left RunLimits.max_wall_clock_ms = None. The tinyagents
harness already bounds every model AND tool/sub-agent call by the run's
remaining wall-clock budget (with_call_budget -> tokio::time::timeout), but
ONLY when a deadline is configured; with None, call_budget() returns None and
each call is awaited UNBOUNDED. So a hung/slow model stream or a delegated
sub-agent that never returned left the loop parked inside an await, the
between-call deadline check never ran, and no terminal event ever fired ->
the client received an empty string while inference_heartbeat beat on until
the socket died.
Primary fix: run_policy_for now arms policy.limits.max_wall_clock_ms
(OPENHUMAN_AGENT_TURN_TIMEOUT_SECS, default 600s, 0 disables). This activates
the harness's per-call timeout so a wedged call is interrupted mid-flight and
the turn degrades to a graceful Timeout -> chat_error. It also bounds
sub-agents: the parent's remaining budget wraps the sub-agent tool call.
Supporting:
- web_errors: classify the harness Timeout ("run timed out: ... wall-clock
budget/deadline") plus the outer backstop marker as a dedicated, retryable
turn_timeout chat_error with actionable copy.
- web/ops: keep the channel-level wall-clock backstop as an OUTER net (raised
to 900s, above the 600s harness ceiling) so a hang outside the harness run
still ends in a terminal event.
Tests: run_policy_for arms the ceiling; env override parsing (0=unbounded,
garbage=default); harness Timeout + backstop marker both classify as
turn_timeout.
Closes tinyhumansai#4746
Add the previously-omitted `session_expired` and `network` values to the `error_type` doc list so it matches every token `classify_inference_error` actually returns (addresses @coderabbitai on web_errors.rs:160).
Collapse the identical `select!{biased; cancel => None, drive_turn_with_deadline(
with_origin(scope(run_chat_task))) => Some}` block duplicated between
`start_chat` and `spawn_parallel_turn` into `run_turn_under_cancel_and_deadline`,
so the two call sites can't drift apart. Only the fork flag and run-queue handle
differ per site (addresses @coderabbitai on web/ops.rs:844).
Wrap the process-global `OPENHUMAN_WEB_TURN_TIMEOUT_SECS=1` override in an RAII `EnvGuard` so an assertion unwind can't leak the 1s backstop into unrelated tests sharing the binary (addresses @coderabbitai on web_tests.rs:1986).
…nnowmann) A wedged web turn that trips the wall-clock ceiling is a deterministic, user-surfaced, retryable outcome — the client already receives a graceful `turn_timeout` chat_error. The run_chat_task Err branch only suppressed the Sentry emit for the max-iteration cap, so every turn_timeout fell into report_error_or_expected and paged Sentry as an unexpected error, contradicting this PR's own framing of turn_timeout as a graceful agent-loop outcome (same tier as MaxIterationsExceeded / EmptyProviderResponse, both explicitly demoted). Extract sentry_suppression_reason() — a pure predicate over the formatted error string — and gate the emit site on it, suppressing both the max-iteration cap and the wall-clock backstop / harness Timeout (is_turn_timeout_error). Add a unit test asserting turn_timeout (marker + harness renderings) is suppressed while a genuine provider error still pages.
|
Rebased onto main, resolved one conflict in |
M3gA-Mind
force-pushed
the
fix/GH-4746-turn-wall-clock-backstop
branch
from
42340d4 to
5455964
Compare
coderabbitai
Bot
added
the
rust-core
There was a problem hiding this comment.
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/channels/providers/web_tests.rs`:
- Around line 2036-2112: Serialize access to the process-wide run_chat_task test
hook used by
wedged_turn_hits_wall_clock_backstop_and_emits_turn_timeout_chat_error across
all tests that call start_chat/run_chat_task, not only tests in web_tests.rs.
Reuse the existing FORCED_ERROR_TEST_LOCK or establish a shared lock at the
common hook boundary so TEST_RUN_CHAT_TASK_BLOCK and
OPENHUMAN_WEB_TURN_TIMEOUT_SECS cannot overlap with concurrent tests.
🪄 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: b8970fa6-71ac-4589-a7d6-d11a298180bb
📒 Files selected for processing (4)
src/openhuman/channels/providers/web/mod.rssrc/openhuman/channels/providers/web/ops.rssrc/openhuman/channels/providers/web_errors.rssrc/openhuman/channels/providers/web_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/openhuman/channels/providers/web_errors.rs
- src/openhuman/channels/providers/web/ops.rs
…k boundary The wall-clock backstop test (and the other forced-block/forced-error tests) drive process-global toggles — TEST_RUN_CHAT_TASK_BLOCK, TEST_FORCED_RUN_CHAT_TASK_ERROR, OPENHUMAN_WEB_TURN_TIMEOUT_SECS. They were serialized only through a file-local FORCED_ERROR_TEST_LOCK in web_tests.rs, so any test in another module that calls start_chat/run_chat_task could still race these globals and flake. Define the serialization lock (RUN_CHAT_TASK_TEST_LOCK) at the shared hook boundary in web::ops where the hooks live, and have web_tests.rs acquire that shared lock instead of its own. Any test exercising the same path now serializes on one lock regardless of which module it lives in.
|
W4 update — pushed Review thread resolved; only the Core coverage job is finishing. |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Review ✅ — approve
RCA is correct and verified against the pinned tinyagents SHA: run_policy_for left RunLimits::max_wall_clock_ms = None, which disables the harness per-call tokio::time::timeout (its budget is Some only when config timeout_ms OR policy max_wall_clock_ms is set). Arming it (600s) bounds both reported wedge shapes — with_call_budget wraps the streaming model call, the buffered call, and every tool/sub-agent call. The 900s channel backstop is defense-in-depth for hangs outside the harness run.
Verified
- Timers nest correctly (inner 600 < outer 900): harness fires first, returns
Errvia normal unwind, outer never double-fires → exactly onechat_error{turn_timeout, retryable}, client stops spinning. Closes the symptom. - Terminal event reliable: both harness timeout strings + the synthetic marker route to
turn_timeoutin the classifier.
Non-blocking
- [low] The two ceilings are independent env vars; if an operator sets
OPENHUMAN_WEB_TURN_TIMEOUT_SECS<OPENHUMAN_AGENT_TURN_TIMEOUT_SECS, the outer backstop fires first and drops the whole harness future instead of letting it unwind gracefully (widening the #4774 approval-leak window). Defaults are safe. Consider clamping outer tomax(web,agent)+slackor a debug-assert/log on inversion. - [low] The RCA interrupt path isn't asserted openhuman-side end-to-end — the integration test drives the outer backstop via a test block that replaces
run_chat_task, so the harness never runs; coverage relies on the config-arm test + classifier units + tinyagents' own suite. Fine given the fix is a one-line config arm, worth noting.
Dangling-resource seam on forced teardown correctly scoped out to #4774 (approval-gate slice = #4819). CodeRabbit majors (error_type doc, shared wiring, cross-module test lock) all resolved in-branch.
4 participants
Problem (issue #4746)
A delegated turn shipped an empty reply with no
chat_doneand nochat_error— the client spun oninference_heartbeatuntil the socket died (~443s). Two evidence cases: a sub-agent that streamed but never emitted a completion, and a main agent wedged mid tool-call.Root cause — in openhuman's adapter, not the tinyagents harness
I checked
vendor/tinyagentsfirst. The harness is correct and robust:with_call_budgetwraps every model call (incl. the streaming path,agent_loop/model_call.rs) and every tool/sub-agent call (agent_loop/tools.rs) intokio::time::timeout(remaining_wall_clock), interrupting a hung call and returningTinyAgentsError::Timeout.The catch:
call_budget()returnsNone— i.e. calls are awaited unbounded — when neither the run config'stimeout_msnor the harness policy'smax_wall_clock_msis set. And openhuman'stinyagents::run_policy_forsets the iteration caps but nevermax_wall_clock_ms. So the harness's per-call timeout was disabled: a hung model stream / a sub-agent that never returned parked the loop inside anawait, the between-call deadline check never ran, and no terminal event ever fired.No tinyagents change needed — the fix is a one-liner in the openhuman adapter that arms the ceiling.
Fix
tinyagents/mod.rs(root cause) —run_policy_fornow setspolicy.limits.max_wall_clock_ms(OPENHUMAN_AGENT_TURN_TIMEOUT_SECS, default 600s,0disables). This arms the harness's existing per-call timeout so a wedged model/tool/sub-agent call is interrupted mid-flight and the turn degrades to a gracefulTimeout → chat_error. It also bounds sub-agents — the parent's remaining budget wraps the sub-agent tool call, and a child turn with no per-run timeout inherits this policy-level cap.web_errors.rs— a dedicated, retryableturn_timeoutclassification branch matching both the harnessTimeoutmessage (run timed out: … wall-clock budget/deadline) and the outer backstop marker, so the user gets actionable copy instead of the generic "something went wrong".web/ops.rs(defense-in-depth) — a channel-level wall-clock backstop around the whole turn future, as an outer net (raised to 900s, above the 600s harness ceiling) so a hang outside the harness run (session assembly / persistence plumbing) still ends in a terminalchat_error. The harness ceiling is the primary mechanism and fires first.The iteration-cap path already degrades gracefully (
core.rssynthesizes a checkpoint/summary); this closes the remaining never-returns gap.Tests
run_policy_for_arms_the_wall_clock_ceiling+agent_turn_wall_clock_ms_parses_env_override(0=unbounded, garbage=default).classify_inference_error_turn_timeout_gets_dedicated_branch+classify_inference_error_harness_wall_clock_timeout_is_turn_timeout.wedged_turn_hits_wall_clock_backstop_and_emits_turn_timeout_chat_error— parks a turn, asserts a terminalchat_error{turn_timeout, retryable}and cooperative teardown.Checks
Per an active no-local-builds directive (disk pressure on the runner), CI verifies.
cargo check --libpassed on an earlier run of the initial change. Note: the lib test target has pre-existing compile errors in several unrelated domains at this base (reproduced on clean main with these changes stashed) — left to CI.Closes #4746
Summary by CodeRabbit