Skip to content

fix(tui): allow longer quiet reasoning waits#3972

Open
Hmbown wants to merge 2 commits into
mainfrom
codex/0867-expand-stream-idle-timeout
Open

fix(tui): allow longer quiet reasoning waits#3972
Hmbown wants to merge 2 commits into
mainfrom
codex/0867-expand-stream-idle-timeout

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • raise the default streamed-response idle timeout from 300s to 900s
  • make the TUI stalled-turn watchdog respect the configured stream idle budget plus a short grace period
  • update /config stream_chunk_timeout_secs default copy and configuration docs

Refs #2487 (one concrete quiet-model-wait / stalled-turn path; the umbrella issue remains broader).

Tests

  • cargo fmt --all --check
  • git diff --check
  • cargo test -p codewhale-tui --bin codewhale-tui --locked stream_chunk_timeout
  • cargo test -p codewhale-tui --bin codewhale-tui --locked turn_liveness

Signed-off-by: Hmbown <101357273+Hmbown@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 3, 2026 15:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 2m 50s —— View job


Review complete ✅

  • Gather context (PR diff, CLAUDE.md/AGENTS.md guidance)
  • Review correctness of timeout/watchdog changes
  • Check provider/model route safety implications (N/A — no provider/model logic touched)
  • Check for reuse/simplification issues
  • Check test coverage
  • Post final review

Summary

Small, well-scoped change: raises the default stream idle timeout 300s → 900s and makes the TUI's stalled-turn watchdog track that budget (turn_stall_watchdog_timeout() in crates/tui/src/tui/ui.rs:5832-5836, max(TURN_STALL_WATCHDOG_TIMEOUT, stream_chunk_timeout_secs + 30s grace)), so a long quiet-reasoning model pause no longer gets force-recovered as a stalled turn. Docs, config default copy, and both changelogs were updated consistently.

Correctness — logic checks out:

  • No overflow risk: stream_chunk_timeout_secs is always clamped to 1..=3600 before it reaches App (crates/tui/src/config.rs:3915-3930), and the saturating_add for the grace period is defensive but not load-bearing given that clamp.
  • The updated doc comment on TURN_STALL_WATCHDOG_TIMEOUT (now describing it as a minimum, not a max) correctly reflects the new max(...) semantics.
  • turn_stall_watchdog_timeout() is a small, well-placed pure helper next to reconcile_turn_liveness.

Provider/model route safety — not applicable; nothing here touches provider/model selection or prefix parsing.

Reuse/simplification — no issues; no duplicated logic introduced.

Tests — good coverage overall: turn_liveness_respects_stream_idle_budget_for_quiet_model_waits (new) and turn_liveness_recovers_stalled_in_progress_turn (updated to compute the threshold via turn_stall_watchdog_timeout(&app)) exercise both sides of the new behavior. One minor, non-blocking note: turn_liveness_uses_recent_turn_activity_not_turn_start and turn_liveness_does_not_abort_running_tool (crates/tui/src/tui/ui/tests.rs:3872, :3890) still compute their now offset from the bare TURN_STALL_WATCHDOG_TIMEOUT constant without setting app.stream_chunk_timeout_secs. Since create_test_app() uses Config::default(), the effective watchdog in those tests is now ~930s (900 + 30 grace), not 300s. The assertions (!recovered) still pass because they're gated on other conditions (recent activity / running tool), but the tests no longer meaningfully exercise the old ~300s boundary they were written around. Not a functional bug — just something to be aware of if those tests are touched again.

Security — no concerns; no new secret handling, exec, or input-validation paths.

Note: I was unable to execute cargo test/cargo fmt in this sandbox (commands require approval not available in this run), so I could not independently re-verify the test commands listed in the PR description — the diff-level trace-through above is what this review is based on.

Signed-off-by: Hmbown <101357273+Hmbown@users.noreply.github.com>
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 38s —— View job


Review complete ✅

  • Gather context (diff since last review, new commit)
  • Review correctness of new changes
  • Post final review

Summary

This synchronize only added one new commit (aa9b177) since the prior review, and it's changelog-only: both CHANGELOG.md and crates/tui/CHANGELOG.md now link the stalled-turn timeout fix to (#2487) instead of leaving the reference unlinked. No code, config, or test changes in this update — the substantive diff (36e1374) is unchanged from what was already reviewed.

Correctness — trivial, no issues. Both changelog files were updated consistently (same wording, same issue reference added in both places).

Everything from the prior full review still stands:

  • turn_stall_watchdog_timeout() correctly implements max(TURN_STALL_WATCHDOG_TIMEOUT, stream_chunk_timeout_secs + 30s grace), and the doc comment update (minimum vs. max) matches the new semantics.
  • stream_chunk_timeout_secs is clamped to 1..=3600 before reaching App, so the saturating_add for the grace period is defensive-only, not load-bearing.
  • Provider/model route safety — not applicable, no provider/model logic touched.
  • Minor non-blocking note (unchanged from before): turn_liveness_uses_recent_turn_activity_not_turn_start and turn_liveness_does_not_abort_running_tool (crates/tui/src/tui/ui/tests.rs:3872, :3890) still derive now from the bare TURN_STALL_WATCHDOG_TIMEOUT constant rather than turn_stall_watchdog_timeout(&app). They still pass (gated on other conditions), but no longer exercise the ~300s boundary they were originally written around.
  • Security — no concerns.

No new findings from this update. Nothing blocking.

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.

2 participants