Skip to content

test(tui): name the command a dispatch smoke stalls on, and skip /pin on Windows #5922

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.

Sign up for GitHub

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

Merged
Hmbown merged 1 commit into from
Sep 6, 2026
Merged

test(tui): name the command a dispatch smoke stalls on, and skip /pin on Windows #5922

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 73 additions & 23 deletions crates/tui/src/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1587,8 +1587,78 @@ mod tests {
/// `scoped_home` (snapshot repo init shells out to git, which races
/// against parallel-running tests). Skip it here so this smoke test
/// stays parallel-safe.
///
/// `/pin` is skipped on Windows only. Its handler is not a state toggle:
/// it drives the *host terminal window* through Win32 (see
/// `tui::window_control`), and the resolved `HWND` belongs to another
/// process. `SetWindowPos`/`ShowWindow` against a foreign window are
/// delivered to that window's thread and block until it pumps them, so on
/// a headless CI window station the call never returns — this is the
/// 600 s nextest timeout in #5919 (the CI breadcrumb stalled on `/pin`
/// and on its `/mini` alias, the same handler). On macOS and Linux
/// `toggle_pin()` is a compiled-out no-op, so dispatch coverage for
/// `/pin` is kept there.
fn skip_in_dispatch_smoke(name: &str) -> bool {
name == "restore"
name == "restore" || (cfg!(windows) && name == "pin")
}

/// Upper bound on a single command dispatch in the smoke tests.
///
/// Generous next to the millisecond each handler actually takes, and far
/// below nextest's 600 s test timeout, so a handler that blocks fails the
/// test *by name* instead of burning a ten-minute CI slot with no
/// attribution (#5919).
const DISPATCH_WATCHDOG: std::time::Duration = std::time::Duration::from_secs(30);

/// Dispatch one command under a per-command watchdog and return the
/// handler's message.
///
/// The app is built and the command executed on a dedicated thread; the
/// test thread waits on the result with a timeout. A handler that never
/// returns leaves its thread parked, but the test itself fails
/// immediately, naming the invocation. A handler that panics still
/// surfaces as that panic — the smoke tests are the repo's only
/// panic-in-a-handler net, so the payload is resumed rather than
/// swallowed.
fn dispatch_under_watchdog(command_name: &str, alias_or_name: &str) -> Option<String> {
let label = format!("/{alias_or_name}");
let (tx, rx) = std::sync::mpsc::channel();
let name = command_name.to_string();
let alias = alias_or_name.to_string();
let handle = std::thread::Builder::new()
.name(format!("dispatch-smoke-{alias_or_name}"))
// Command handlers are deeply recursive in debug builds; match the
// 16 MiB the CI runner sets via RUST_MIN_STACK for the main thread.
.stack_size(16 * 1024 * 1024)
.spawn(move || {
let (mut app, tmpdir, _guard) = create_isolated_test_app();

@claude claude Bot Sep 6, 2026

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.

create_isolated_test_app() → ConfigPathGuard::new() acquires lock_test_env(), a process-wide Mutex<()> (crates/tui/src/test_env_lock.rs:159) shared by dozens of other tests (runtime_api/tests.rs, model_inventory.rs, config_persistence.rs, …).

That guard now lives inside the spawned worker thread's closure. On the RecvTimeoutError::Timeout path, dispatch_under_watchdog panic!s on the main test thread and returns — but the worker thread that's actually stuck in the blocking handler is never joined or killed, so its ConfigPathGuard is never dropped and the global env mutex is never released.

Under cargo nextest run this is contained (nextest runs each test in its own process, so a leaked thread in a doomed process can't affect anything else). But under plain cargo test — which this repo also uses, including the verification commands quoted in this PR's own description (cargo test -p codewhale-tui --lib -- commands::tests::every_) — all tests in this binary share one process and its threads. If a future handler hangs (the exact scenario this watchdog exists to catch), the offending test now fails fast and by name as intended, but it leaves the shared mutex permanently locked for the rest of that cargo test invocation, so every other test that later calls lock_test_env() (and there are ~30+ call sites) hangs too — trading one attributable timeout for an unattributed pile of them.

Suggest acquiring the env-lock guard (and building App/tempdir) on the main thread before spawning, and only moving the already-built App into the worker for the execute() call — that way the guard drops promptly regardless of whether the worker ever returns. Worth double-checking App: Send if going this route.

let invocation = invocation_for(&name, &alias, tmpdir.path());
let result = execute(&invocation, &mut app);
let _ = tx.send(result.message);
})
.expect("spawn dispatch smoke thread");

let started = std::time::Instant::now();
match rx.recv_timeout(DISPATCH_WATCHDOG) {
Ok(message) => {
let _ = handle.join();

@codewhale-agent codewhale-agent Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[INFO] Worker panic after sending a result is swallowed

In the Ok(message) arm, let _ = handle.join(); discards the join result. If the worker thread panics after sending the result (for example while dropping _guard or tmpdir), the test does not fail even though the stated goal is to surface all handler panics. Consider resuming a join error here as well.

// Quiet on the common path; a handler heading for the
// watchdog still leaves a named breadcrumb in the log.
let elapsed = started.elapsed();
if elapsed > std::time::Duration::from_secs(1) {
eprintln!("dispatch smoke: {label} took {elapsed:?}");
}
message
}
Err(std::sync::mpsc::RecvTimeoutError::Timeout) => panic!(
"{label} did not return within {DISPATCH_WATCHDOG:?}: its handler blocks. \
Fix the handler or add it to skip_in_dispatch_smoke with a reason."
),
Comment on lines +1653 to +1656
Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => match handle.join() {
Ok(()) => panic!("{label} dispatch thread ended without producing a result"),
Err(payload) => std::panic::resume_unwind(payload),
},
}
}

#[test]
Expand Down Expand Up @@ -1690,18 +1760,7 @@ mod tests {
if skip_in_dispatch_smoke(command.name) {
continue;
}
let (mut app, tmpdir, _guard) = create_isolated_test_app();
let invocation = invocation_for(command.name, command.name, tmpdir.path());
// Breadcrumb for a terminated run: the last line names the handler
// that never returned.
eprintln!("dispatch smoke: {invocation}");
let started = std::time::Instant::now();
let result = execute(&invocation, &mut app);
eprintln!(
"dispatch smoke: {invocation} returned in {:?}",
started.elapsed()
);
if let Some(msg) = &result.message {
if let Some(msg) = dispatch_under_watchdog(command.name, command.name) {
assert!(
!msg.contains("Unknown command"),
"/{} fell through to the unknown-command branch: {msg}",
Expand All @@ -1720,16 +1779,7 @@ mod tests {
continue;
}
for alias in command.aliases {
let (mut app, tmpdir, _guard) = create_isolated_test_app();
let invocation = invocation_for(command.name, alias, tmpdir.path());
eprintln!("dispatch smoke: {invocation}");
let started = std::time::Instant::now();
let result = execute(&invocation, &mut app);
eprintln!(
"dispatch smoke: {invocation} returned in {:?}",
started.elapsed()
);
if let Some(msg) = &result.message {
if let Some(msg) = dispatch_under_watchdog(command.name, alias) {
assert!(
!msg.contains("Unknown command"),
"/{alias} (alias of /{}) fell through to unknown: {msg}",
Expand Down
Loading