Code Review

This pull request correctly implements per-task timeouts by adding the necessary logic to CmdLineRunner and TaskExecutor. The end-to-end tests have also been updated to cover the new functionality, which is great. I've identified a potential race condition in the timeout handling logic within CmdLineRunner concerning spurious wakeups and memory ordering. I've provided specific suggestions to address these correctness issues. Overall, this is a solid implementation of the feature.

This review was AI-generated.

Nice first Rust PR! The overall approach is sound — adding a timer thread with condvar cancellation for per-task timeouts and the SIGTERM→SIGKILL escalation. A few issues to address though:

Blocking Issues

1. Mutex deadlock during SIGKILL grace period (up to 5s hang)

After sending SIGTERM, the timer thread holds the mutex while doing a second cvar.wait_timeout(guard, 5s):

let _ = nix::sys::signal::kill(pid, nix::sys::signal::Signal::SIGTERM);
let (_guard, result) = cvar.wait_timeout(guard, Duration::from_secs(5)).unwrap();

Meanwhile, the main thread (after the process exits from SIGTERM) tries to acquire the same mutex to set the cancel flag:

*lock.lock().unwrap() = true;  // blocks — timer thread holds the lock
cvar.notify_one();             // never reached

So the timer thread is waiting on a condvar notification that can't be sent because the main thread is blocked on the mutex. This is a deadlock for up to 5 seconds. The timer thread's wait_timeout will eventually expire, but the main thread is unnecessarily stalled.

Fix: either (a) don't hold the mutex during the SIGKILL grace period (use a simple thread::sleep), or (b) restructure so the cancel flag uses an AtomicBool checked after each wait_timeout instead of holding the mutex across both waits.

2. Race condition: process completes at timeout boundary → false "timed out" error

If the timeout expires at nearly the same instant the process exits successfully:

1. Timer thread: wait_timeout returns, timed_out() == true
2. Timer thread: timed_out.store(true, ...)
3. Main thread: receives ExitStatus(success), exits rx loop
4. Main thread: sets cancel (too late)
5. Main thread: checks timed_out → true → bails with "timed out" even though the task succeeded

The window is narrow but real. Consider checking the exit status before checking timed_out, or using a single source of truth for the outcome.

3. execute_raw doesn't get timeout support

When raw = true (either via --raw flag or settings.raw), tasks go through execute_raw() which has no timeout logic. Tasks in raw mode will silently ignore their timeout configuration.

What looks good

• SIGTERM→5s grace→SIGKILL escalation is the right pattern
• Condvar-based cancellation (vs busy-polling or thread::sleep) is the right approach, just needs the mutex issue fixed
• E2e tests now exercise real timeout behavior instead of TODO placeholders
• The with_timeout builder API is clean and consistent with the rest of CmdLineRunner

This review was AI-generated.

Nice first Rust PR! The architecture is solid. A couple of notes before looking at issues:

Re: jdx's blocking issues — both appear to be already fixed in the current diff:

• Deadlock: The timer thread calls drop(guard) before thread::sleep(5s), so the main thread can freely acquire the lock and set the cancel flag during the grace period. No deadlock.
• execute_raw: The diff does add TimeoutGuard::new to execute_raw. Both paths are covered.

jdx's reviews were likely based on an earlier draft. The race condition (#2) is still real — see below.

Issues

1. Spurious wakeups can silently swallow the timeout

Condvar::wait_timeout can return early with timed_out() == false due to a spurious wakeup. The current code treats a spurious wakeup identically to a genuine cancellation — it exits the timer thread and the process never gets killed:

let (guard, wait_result) = cvar.wait_timeout(guard, timeout).unwrap();
if !wait_result.timed_out() {
    return;  // BUG: spurious wakeup exits here; timeout never fires
}

The correct pattern is to loop, track remaining time, and check the cancel predicate explicitly:

let mut guard = lock.lock().unwrap();
let deadline = std::time::Instant::now() + timeout;
loop {
    if *guard { return; }  // cancelled
    let remaining = deadline.saturating_duration_since(std::time::Instant::now());
    if remaining.is_zero() { break; }
    let (g, result) = cvar.wait_timeout(guard, remaining).unwrap();
    guard = g;
    if result.timed_out() { break; }
    // spurious wakeup: loop and re-check
}

2. Race condition at timeout boundary (confirming jdx's #2)

If the timeout fires at nearly the same instant the process exits successfully:

1. Timer sets timed_out = true, sends SIGTERM
2. Process was already exiting — SIGTERM is moot
3. Main thread receives ExitStatus(success), exits rx loop
4. Main thread calls g.cancel() (too late — already set)
5. Main thread checks timed_out() → true → bails "timed out" even though the task succeeded

The fix is simpler than it may seem — just rearrange the existing checks so timed_out() is only checked when the exit status is non-success (in both execute and execute_raw):

// Before:
if timeout_guard.as_ref().is_some_and(|g| g.timed_out()) {
    bail!("timed out after {:?}", self.timeout.unwrap());
}
if !status.success() {
    self.on_error(combined_output, status)?;
}

// After:
if !status.success() {
    if timeout_guard.as_ref().is_some_and(|g| g.timed_out()) {
        bail!("timed out after {:?}", self.timeout.unwrap());
    }
    self.on_error(combined_output, status)?;
}

If the process exited successfully, we don't care whether the timer also fired — the task completed its work. If it exited with failure and the timer fired, SIGTERM killed it so we report "timed out". ~2-line change in two places.

3. Kill by PID, not PGID — child processes may survive the timeout

The existing kill_all and signal forwarding use killpg() to kill the entire process group. The timeout guard only does kill(pid, SIGTERM).

If a task spawns sub-processes (e.g. run = "npm run build"), only the top-level process is killed — children survive.

Note: setpgid is only called when stdin is not a TTY (and not at all in execute_raw), so killpg() won't always work. Should try killpg() first with a fallback to kill(), matching the existing kill_all pattern:

let pgid = nix::unistd::Pid::from_raw(pid as i32);
if nix::sys::signal::killpg(pgid, Signal::SIGTERM).is_err() {
    let _ = nix::sys::signal::kill(pgid, Signal::SIGTERM);
}

4. SIGKILL grace period can't be cancelled (minor)

After SIGTERM kills the process, the main thread calls g.cancel(), but cancel() only notifies the condvar — and at that point the timer thread is inside thread::sleep(5s), not waiting on the condvar. The notification is missed; the timer thread always sleeps the full 5 seconds before checking the cancel flag.

Result: a zombie timer thread lingers for up to 5 seconds after every timed-out task. There's no correctness problem (SIGKILL on a dead PID returns ESRCH, silently ignored), just wasted resources. Could be fixed by replacing thread::sleep with another cvar.wait_timeout so the grace period is also cancellable.

What looks good

• The let _ = tx.send(...) change throughout execute() is necessary and correct — with timeout, the receiver may be dropped before all output is drained, and .unwrap() would panic.
• with_timeout builder API fits naturally with the existing CmdLineRunner pattern.
• task_executor.rs: t.min(g) for the effective timeout (task vs. global) is clean.
• SIGTERM → 5s grace → SIGKILL escalation is the right pattern for Unix process cleanup.

This review was AI-generated.

The earlier AI reviews covered most issues and the current diff addresses the spurious wakeup and race condition concerns they raised. One thing that hasn't been called out clearly:

Global task_timeout vs per-task timeout interaction is misleading

The PR description says:

mise run --timeout=1m deploy  # killed after 30s (global setting still wins)

But this "lower wins" behavior isn't actually implemented. The global task_timeout / --timeout flag is a tokio wall-clock timeout wrapping the entire parallelize_tasks call in src/cli/run.rs:337-340. The new per-task timeout is a per-process TimeoutGuard in CmdLineRunner. These are completely independent timers at different levels — they don't compare against each other.

In task_executor.rs, only the task-level timeout is applied:

let effective_timeout = task.timeout.as_ref().and_then(|s| match duration::parse_duration(s) { ... });

There's no min(task_timeout, per_task_timeout) logic. Either the PR description should be updated to reflect the actual behavior, or the code should implement the advertised semantics.

This review was AI-generated.

The earlier AI reviews covered most issues and the current diff addresses the spurious wakeup and race condition concerns they raised. Reiterating one point that was raised before:

Global task_timeout vs per-task timeout interaction is misleading

The PR description says:

mise run --timeout=1m deploy  # killed after 30s (global setting still wins)

But this "lower wins" behavior isn't actually implemented. The global task_timeout / --timeout flag is a tokio wall-clock timeout wrapping the entire parallelize_tasks call in src/cli/run.rs:337-340. The new per-task timeout is a per-process TimeoutGuard in CmdLineRunner. These are completely independent timers at different levels — they don't compare against each other.

In task_executor.rs, only the task-level timeout is applied:

let effective_timeout = task.timeout.as_ref().and_then(|s| match duration::parse_duration(s) { ... });

There's no min(task_timeout, per_task_timeout) logic. Either the PR description should be updated to reflect the actual behavior, or the code should implement the advertised semantics.

This review was AI-generated.

Correction to my previous comment — the "lower wins" behavior described in the PR does work in practice, just not via explicit min() logic. The global task_timeout / --timeout fires as a tokio wall-clock timeout on the entire mise run invocation (run.rs:337), while the per-task timeout fires as a per-process TimeoutGuard. They're independent timers that race, and whichever is shorter naturally fires first.

So the described behavior is correct. It might be worth documenting that these are two different mechanisms (whole-run vs per-process), but functionally there's no bug here. Apologies for the noise.