test(tui): stabilize the forced-idle-timeout worker-reuse test on Windows (#5898) - #5913
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ffb7ad54-d3dc-4358-be24-0509911b47cd) |
|
Claude finished @Hmbown's task in 1m 44s —— View job Review: stabilize forced-idle-timeout worker-reuse test on WindowsSummary: Solid, narrowly-scoped fix. I traced the claimed race through
Scope check:
Provider/model safety (EPIC #2608): N/A — Minor, non-blocking observations:
I was not able to execute No changes requested. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is confined to test helpers/assertions and is consistent with the task manager’s biased polling behavior, reducing flakiness without altering production logic.
Pull request overview
This PR stabilizes the Windows-flaky TUI task-manager regression test for forced idle timeouts by making the follow-up (“worker reuse”) task complete deterministically under scheduler stalls, without changing any production task-manager behavior.
Changes:
- In the test-only
PromptRouterExecutor, the follow-up (non-hang) path now completes with no.awaitpoints, emitting at most a singleevents.try_send(...)and immediately returningCompleted. - Improves failure diagnostics by including full terminal records in assertion messages.
- Adds an assertion pinning the follow-up task’s
terminal_reasonto"completed".
File summaries
| File | Description |
|---|---|
crates/tui/src/task_manager.rs |
Adjusts a test-only executor to eliminate await points in the follow-up branch and strengthens the test’s assertions/diagnostics to remove Windows CI flakiness. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
The PR stabilizes the forced-idle-timeout worker-reuse test by making the non-hang follow-up executor branch await-free and by strengthening the failing assertions to print terminal records and pin the follow-up terminal_reason to "completed". The change is confined to the test module.
Findings
- [INFO] Status event delivery is best-effort and can be silently dropped (
crates/tui/src/task_manager.rs:3730)
The new non-hang branch useslet _ = events.try_send(...), so a full or closed events channel will drop the only status event emitted for the follow-up task. This is intentional to avoid awaits and no assertion depends on this event, but the adjacent comment claims the released worker's event pipeline is exercised; whentry_sendfails, it is not.
Assessment
Looks good. The test-only change correctly removes await points from the follow-up path so the shortened idle timeout budgets cannot interrupt it, and the added terminal_reason assertion pins the expected outcome. No production execution path is affected.
Advisory review by Codewhale (codewhale review --pr 5913 --post, head aabf8e7fd4f2a7cef375e4c28d9400ec0bd891aa). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| // reason -> `Failed` (issue #5898). `try_send` keeps the | ||
| // released worker's event pipeline exercised without | ||
| // suspending this future. | ||
| let _ = events.try_send(TaskExecutionEvent::Status { |
There was a problem hiding this comment.
[INFO] Status event delivery is best-effort and can be silently dropped
The new non-hang branch uses let _ = events.try_send(...), so a full or closed events channel will drop the only status event emitted for the follow-up task. This is intentional to avoid awaits and no assertion depends on this event, but the adjacent comment claims the released worker's event pipeline is exercised; when try_send fails, it is not.
Root cause (from CI job 101421344166, run 34008939993 attempt 1):
the second assert in forced_idle_timeout_releases_the_worker_for_later_tasks
panicked with left: Failed, right: Completed in 3.177s — not a wait-budget
timeout. The follow-up task ("run after hang") ran under the same
short_for_tests budgets (idle 150ms, wall 400ms, grace 50ms) that exist to
force-terminalize the *stuck* task, and its MockExecutor path performs real
awaits (3x send().await, a 50ms sleep, a post-sleep cancel check). When a
loaded Windows runner stalls the tokio scheduler or the fsync-bound event
processing for >=150ms while the follow-up task is mid-flight (and before its
events are queued, so the drain-on-idle-interrupt rescue in run_task cannot
retract the interrupt), the guard interrupts with IdleTimeout (or WallTimeout
past 400ms), cancels the token, MockExecutor observes the cancellation and
returns Canceled, and preserve_timeout_reason rewrites it to the timeout
reason -> TaskStatus::Failed. Once note_interrupt fires, progress can no
longer retract it, so the race is one-sided against the test.
Fix (test module only; production TaskExecutionLimits and MockExecutor
untouched): the follow-up branch of PromptRouterExecutor now completes with
zero await points — one synchronous events.try_send(Status) to keep the
released worker's event pipeline exercised, then an immediate Completed
result. run_task polls the executor future first in its biased select and the
first guard evaluate() runs with elapsed ~= 0, so an await-free future always
finishes before any interrupt can be recorded, deterministically, regardless
of machine load. The stuck-task branch (std::future::pending) is unchanged,
so the test still proves a forcibly idle-timed-out task releases its worker
for later tasks. Per the issue, both asserts now print the full terminal
record on failure, and a new assert pins the follow-up's terminal_reason to
"completed" so any future regression shows which reason won.
Verification (macOS local; no Windows runner available here):
- cargo test -p codewhale-tui task_manager: 44 passed / 0 failed
- cargo test -p codewhale-tui forced_idle_timeout -- --test-threads=1 x5: 5/5 pass
- forced_idle_timeout x10 with 8 CPU burners (scheduler-starvation stress,
emulating the CI contention mechanism): 10/10 pass
- full task_manager suite x3 under the same load: 3x 44/44 pass
- cargo fmt -p codewhale-tui -- --check: clean
Residual risk: the hang task itself could theoretically exceed the 10s
wait_for_terminal_state budget only under extreme multi-second stalls, which
the drain + grace design already tolerates; nothing in this change increases
that exposure.
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
) The Windows matrix also flaked on task_manager::tests::heartbeat_status_does_not_refresh_idle_timeout with `left: Some("wall_timeout"), right: Some("idle_timeout")`. The guard checked the wall limit before the idle limit on every tick, so a scheduler stall that carried the watchdog past both test budgets (150 ms idle, 400 ms wall) reported the wall limit even though the idle limit had expired first. ExecutionGuard::evaluate now compares the two deadlines and reports the one that was crossed first; a tie keeps the wall limit's precedence, and a limit whose deadline does not fit in Instant never fires. The reason a task carries no longer depends on when the watchdog happened to wake, which is also the truthful reason in production. Tests: a new guard unit test covers both orderings under a starved tick, and the existing progress-refreshes-idle test now keeps progress flowing up to the wall deadline, which is what its name claims. cargo test -p codewhale-tui task_manager::tests: 45 passed, 0 failed. Signed-off-by: CodeWhale Bot <hmbown@gmail.com>
|
Second Windows flake in the same family, fixed on this branch: |
aabf8e7 to
5ad5c76
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_dd37999f-d8d8-4d8b-8215-eab697a72955) |
|
Claude finished @Hmbown's task in 3m 12s —— View job Review: Windows watchdog-timeout flake fixes (commits f2431c2 + 5ad5c76)
Summary: Two real fixes for the same starved-watchdog family, and the second one is a genuine production-code change, not "test helpers only." 1. Scope note: the description is now stale (please update before merge)The PR body/AI overview still say "Changes are confined to test helpers and assertions... no production execution path is modified." That was true for commit 2. Correctness of the new attribution logic — traced and looks rightOld code ( The new Also confirmed the un-changed 3. Minor, non-blocking
4. Provider/model safetyN/A — no provider/model/routing code is touched by either commit. Verification limitsSame constraint as the earlier automated pass on this PR: I couldn't execute No changes requested from me — the logic is sound. Just flagging the description/scope mismatch for a maintainer to reconcile before this merges as "test-only." |
There was a problem hiding this comment.
Codewhale review
The PR stabilizes the Windows forced-idle-timeout worker-reuse test by making the follow-up test executor await-free and improves timeout attribution in ExecutionGuard. The await-free follow-up should prevent scheduler-stall-driven interrupts, but the PR also changes production timeout-reason behavior and reduces async follow-up coverage.
Findings
- [WARNING] Production timeout attribution change is broader than a test stabilization (
crates/tui/src/task_manager.rs:541)
ExecutionGuard::evaluate now chooses wall vs idle timeouts by comparing Instant deadlines and reports whichever limit actually expired first; when both are elapsed, idle wins unless its deadline was pushed past wall. This is a user-visible behavior change to task-manager terminal reasons under scheduler delay, not just a test-only fix. It should be explicitly justified or separated from the Windows flake fix. - [WARNING] Follow-up test path no longer exercises the real async executor (
crates/tui/src/task_manager.rs:3745)
The non-hang branch now returns a synthetic Completed result after one synchronous try_send instead of delegating to MockExecutor. The forced-idle test still proves a hung task releases a worker, but it no longer verifies that a released worker can run a normal task with awaited sends and sleep. Regressions in async follow-up execution could pass unnoticed. - [INFO] Missing tie-case coverage for wall/idle deadline precedence (
crates/tui/src/task_manager.rs:3864)
The new comment says ties keep wall precedence, but the new ExecutionGuard test covers idle-expired-first and idle-pushed-past-wall, not equal deadlines. A tie case would pin the documented behavior.
Suggestions
crates/tui/src/task_manager.rs:3895— Add an explicit tie case to execution_guard_reports_the_limit_that_expired_first_when_both_elapsed that sets both deadlines equal and expects WallTimeout, so the wall-precedence behavior is protected.crates/tui/src/task_manager.rs:3745— Keep the await-free follow-up for this flake fix, but add a separate test that runs a normal non-hang task through MockExecutor on a reused worker with normal or longer budgets to retain async execution coverage.
Assessment
The test flake fix is likely effective and the improved assert messages are useful. However, the PR includes a production behavior change that should be explicitly accepted rather than hidden in a test-stabilization PR, and the follow-up path weakens executor coverage. I would approve only after confirming the ExecutionGuard change is intended and adding at least tie and async follow-up coverage.
Advisory review by Codewhale (codewhale review --pr 5913 --post, head 5ad5c76d7c3f768865f88e65f19c263b9f768673). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| } else { | ||
| None | ||
| // Attribute the timeout to the limit that was crossed first, not | ||
| // to the one this tick happens to check first. When the watchdog |
There was a problem hiding this comment.
[WARNING] Production timeout attribution change is broader than a test stabilization
ExecutionGuard::evaluate now chooses wall vs idle timeouts by comparing Instant deadlines and reports whichever limit actually expired first; when both are elapsed, idle wins unless its deadline was pushed past wall. This is a user-visible behavior change to task-manager terminal reasons under scheduler delay, not just a test-only fix. It should be explicitly justified or separated from the Windows flake fix.
| let _ = events.try_send(TaskExecutionEvent::Status { | ||
| message: format!("running after forced release {}", task.id), | ||
| }); | ||
| TaskExecutionResult { |
There was a problem hiding this comment.
[WARNING] Follow-up test path no longer exercises the real async executor
The non-hang branch now returns a synthetic Completed result after one synchronous try_send instead of delegating to MockExecutor. The forced-idle test still proves a hung task releases a worker, but it no longer verifies that a released worker can run a normal task with awaited sends and sleep. Regressions in async follow-up execution could pass unnoticed.
| } | ||
|
|
||
| #[test] | ||
| fn execution_guard_reports_the_limit_that_expired_first_when_both_elapsed() { |
There was a problem hiding this comment.
[INFO] Missing tie-case coverage for wall/idle deadline precedence
The new comment says ties keep wall precedence, but the new ExecutionGuard test covers idle-expired-first and idle-pushed-past-wall, not equal deadlines. A tie case would pin the documented behavior.
| } | ||
| other => panic!("expected wall interrupt, got {other:?}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Add an explicit tie case to execution_guard_reports_the_limit_that_expired_first_when_both_elapsed that sets both deadlines equal and expects WallTimeout, so the wall-precedence behavior is protected.
| result_text: Some("done after hang".to_string()), | ||
| error: None, | ||
| terminal_reason: TaskTerminalReason::Completed, | ||
| } |
There was a problem hiding this comment.
Keep the await-free follow-up for this flake fix, but add a separate test that runs a normal non-hang task through MockExecutor on a reused worker with normal or longer budgets to retain async execution coverage.
Closes #5898.
Root cause (from the failing CI log, job 101421344166): the panic was
left: Failed, right: Completedin 3.177s — not a wait-budget timeout. The follow-up task ("run after hang") inherits the sameshort_for_testsbudgets that exist to force-terminalize the stuck task (idle_progress150ms,wall_time400ms,cancel_grace50ms), and the mock executor's follow-up path performs real awaits. Under a ≥150ms scheduler stall mid-flight — before its events queue, so the drain-on-idle-interrupt rescue cannot retract it — the guard interrupts with IdleTimeout, the executor observes cancellation and returns Canceled, andpreserve_timeout_reasonrewrites it to the timeout reason →Failed.Fix (test module only): the follow-up branch of the test executor now completes with zero await points (one synchronous
events.try_sendto keep the released worker's event pipeline exercised, then immediate Completed).run_task's biased select polls the executor future first and the first evaluate runs at elapsed ≈ 0, so an await-free future always finishes before any interrupt can be recorded — deterministic under any load, closed by construction rather than by margin. Both asserts now print the full terminal record on failure, and a new assert pins the follow-up'sterminal_reasonto"completed".Evidence: this exact test blocked #5899 and #5905 Windows matrices today. Local: task_manager suite 44/44; forced_idle_timeout ×5 single-threaded 5/5; ×10 under 8 CPU-burner starvation 10/10; full suite ×3 under the same load 3×44/44;
cargo fmtclean. The hang branch (std::future::pending) is unchanged, so the test still proves a forcibly idle-timed-out task releases its worker.Note
Medium Risk
Changes production
ExecutionGuardtimeout reason selection inrun_taskand engine turns; behavior is more accurate but could alter which terminal reason users see when both limits elapse under scheduler delay.Overview
Fixes flaky forced idle timeout / worker reuse coverage on loaded CI (including Windows) by tightening timeout attribution and making the follow-up test executor deterministic.
ExecutionGuard::evaluateno longer picks wall vs idle timeout from check order alone. It comparesInstantdeadlines (checked_addon wall and idle budgets) so a starved watchdog still reports whichever limit actually expired first; when both are past due, idle wins unless late progress pushed idle past wall (ties keep wall precedence per #5898).The
PromptRouterExecutortest follow-up path stops delegating toMockExecutor(multipleawaits plus sleep). It now finishes without await points—one synchronoustry_sendand an immediateCompleted—sorun_task’s biased poll completes before short test budgets can interrupt and rewrite the result viapreserve_timeout_reason.Adds a unit test for dual-expired guard behavior, extends the progress/wall guard test, and improves integration asserts (including
terminal_reason: completedon the follow-up task).Reviewed by Cursor Bugbot for commit 5ad5c76. Bugbot is set up for automated code reviews on this repo. Configure here.