-
Notifications
You must be signed in to change notification settings - Fork 3.5k
test(tui): stabilize the forced-idle-timeout worker-reuse test on Windows (#5898) #5913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -529,16 +529,28 @@ impl ExecutionGuard { | |
|
|
||
| let wall_elapsed = now.saturating_duration_since(self.started_at); | ||
| let idle_elapsed = now.saturating_duration_since(self.last_progress_at); | ||
| // A limit whose deadline does not fit in `Instant` can never fire. | ||
| let wall_deadline = self.started_at.checked_add(self.limits.wall_time); | ||
| let idle_deadline = self.last_progress_at.checked_add(self.limits.idle_progress); | ||
| let pending = if shutdown { | ||
| Some(TaskTerminalReason::Shutdown) | ||
| } else if cancel { | ||
| Some(TaskTerminalReason::Canceled) | ||
| } else if wall_elapsed >= self.limits.wall_time { | ||
| Some(TaskTerminalReason::WallTimeout) | ||
| } else if idle_elapsed >= self.limits.idle_progress { | ||
| Some(TaskTerminalReason::IdleTimeout) | ||
| } 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 | ||
| // is starved past both deadlines (a >=250 ms scheduler stall on a | ||
| // loaded CI runner is enough with the test budgets), the idle | ||
| // limit that expired earlier is still the truthful reason; a tie | ||
| // keeps the wall limit's precedence (issue #5898). | ||
| match (wall_deadline, idle_deadline) { | ||
| (Some(wall), Some(idle)) if now >= wall && wall <= idle => { | ||
| Some(TaskTerminalReason::WallTimeout) | ||
| } | ||
| (_, Some(idle)) if now >= idle => Some(TaskTerminalReason::IdleTimeout), | ||
| (Some(wall), _) if now >= wall => Some(TaskTerminalReason::WallTimeout), | ||
| _ => None, | ||
| } | ||
| }; | ||
| if let Some(reason) = pending { | ||
| return GuardAction::Interrupt { reason }; | ||
|
|
@@ -3709,12 +3721,33 @@ mod tests { | |
| &self, | ||
| task: ExecutionTask, | ||
| events: mpsc::Sender<TaskExecutionEvent>, | ||
| cancel: CancellationToken, | ||
| _cancel: CancellationToken, | ||
| ) -> TaskExecutionResult { | ||
| if task.prompt.starts_with("hang ") { | ||
| std::future::pending().await | ||
| } else { | ||
| MockExecutor.execute(task, events, cancel).await | ||
| // The follow-up task must complete without a single await | ||
| // point: `run_task` polls the executor future before its | ||
| // guard can observe the (test-shortened) idle/wall budgets, | ||
| // so an await-free future always finishes first and an | ||
| // interrupt can never be recorded against it. The previous | ||
| // MockExecutor delegation (`send(...).await` x4 plus a 50 ms | ||
| // sleep) left windows where CI scheduler/storage stalls of | ||
| // >=150 ms tripped the idle watchdog mid-flight; the executor | ||
| // then observed the cancellation and returned `Canceled`, | ||
| // which `preserve_timeout_reason` rewrote into the timeout | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [INFO] Status event delivery is best-effort and can be silently dropped The new non-hang branch uses |
||
| message: format!("running after forced release {}", task.id), | ||
| }); | ||
| TaskExecutionResult { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| status: TaskStatus::Completed, | ||
| result_text: Some("done after hang".to_string()), | ||
| error: None, | ||
| terminal_reason: TaskTerminalReason::Completed, | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -3827,6 +3860,40 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn execution_guard_reports_the_limit_that_expired_first_when_both_elapsed() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| let start = Instant::now(); | ||
| let limits = TaskExecutionLimits::short_for_tests(); | ||
| let guard = ExecutionGuard::new(limits, start); | ||
| // A starved watchdog that first ticks after both budgets ran out must | ||
| // still report the idle limit, which expired first. | ||
| match guard.evaluate( | ||
| start + limits.wall_time + limits.idle_progress, | ||
| false, | ||
| false, | ||
| ) { | ||
| GuardAction::Interrupt { reason } => { | ||
| assert_eq!(reason, TaskTerminalReason::IdleTimeout); | ||
| } | ||
| other => panic!("expected idle interrupt, got {other:?}"), | ||
| } | ||
|
|
||
| // Late progress pushes the idle deadline past the wall deadline, so | ||
| // the same starved tick reports the wall limit instead. | ||
| let mut guard = ExecutionGuard::new(limits, start); | ||
| guard.note_progress(start + limits.wall_time - Duration::from_millis(1)); | ||
| match guard.evaluate( | ||
| start + limits.wall_time + limits.idle_progress, | ||
| false, | ||
| false, | ||
| ) { | ||
| GuardAction::Interrupt { reason } => { | ||
| assert_eq!(reason, TaskTerminalReason::WallTimeout); | ||
| } | ||
| other => panic!("expected wall interrupt, got {other:?}"), | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| #[test] | ||
| fn execution_guard_progress_refreshes_idle_until_wall_timeout() { | ||
| let start = Instant::now(); | ||
|
|
@@ -3838,6 +3905,9 @@ mod tests { | |
| GuardAction::Run { .. } => {} | ||
| other => panic!("progress should keep idle from firing, got {other:?}"), | ||
| } | ||
| // Progress keeps arriving, so the idle deadline never expires before | ||
| // the wall deadline does. | ||
| guard.note_progress(start + limits.wall_time - (limits.idle_progress / 2)); | ||
| match guard.evaluate(start + limits.wall_time, false, false) { | ||
| GuardAction::Interrupt { reason } => { | ||
| assert_eq!(reason, TaskTerminalReason::WallTimeout); | ||
|
|
@@ -4104,14 +4174,27 @@ mod tests { | |
| .await?; | ||
| let finished = | ||
| wait_for_terminal_state(&manager, &stuck.id, Duration::from_secs(10)).await?; | ||
| assert_eq!(finished.terminal_reason.as_deref(), Some("idle_timeout")); | ||
| assert_eq!( | ||
| finished.terminal_reason.as_deref(), | ||
| Some("idle_timeout"), | ||
| "stuck task terminal record: {finished:?}" | ||
| ); | ||
|
|
||
| let next = manager | ||
| .add_task(NewTaskRequest::from_prompt("run after hang")) | ||
| .await?; | ||
| let completed = | ||
| wait_for_terminal_state(&manager, &next.id, Duration::from_secs(10)).await?; | ||
| assert_eq!(completed.status, TaskStatus::Completed); | ||
| assert_eq!( | ||
| completed.status, | ||
| TaskStatus::Completed, | ||
| "follow-up task terminal record: {completed:?}" | ||
| ); | ||
| assert_eq!( | ||
| completed.terminal_reason.as_deref(), | ||
| Some("completed"), | ||
| "follow-up task terminal record: {completed:?}" | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.