From 643985d2a8d4800889ab329676b7a2360bec09b9 Mon Sep 17 00:00:00 2001 From: Jake Date: Thu, 14 May 2026 19:19:52 +0100 Subject: [PATCH] =?UTF-8?q?agent:=20lumotia=20=E2=80=94=20Phase=20B.2=20fi?= =?UTF-8?q?x=20misleading=20"force-abort"=20doc=20+=20test=20name=20on=20s?= =?UTF-8?q?upervisor=20shutdown?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase B.2 audit of commit 1068ad9 (hotkey supervisor rearchitecture — Race-1, Race-2, TOCTOU). The production behaviour is correct and the integration tests in crates/hotkey/tests/listener_lifecycle.rs cover Race-1 (per-device listener sender-clone drop on stop) and Race-2 (forwarder join on reconfigure) at the public-API level. The TOCTOU window is closed by construction (insert-before-spawn under one mutex hold) and the original author's // TODO(test): note at linux.rs:576 explicitly explains why a deterministic test would require faking evdev::Device::open, which the crate doesn't expose — honoured. One real residual: two satellite places in supervisor.rs claim that a stuck task is "force-aborted" after SHUTDOWN_TIMEOUT, but the code does NOT abort. `tokio::time::timeout(d, handle).await` consumes the JoinHandle by value; when the timeout fires, the inner future (the JoinHandle) is dropped, and dropping a JoinHandle DETACHES the task rather than aborting it. The shutdown() doc-comment and the warn-log message both correctly say "detached", but: * The SHUTDOWN_TIMEOUT const doc-comment said "we give up and abort it ... force-aborted with a warning". * The test name was shutdown_force_aborts_stuck_tasks_after_timeout. A future maintainer trusting either of these would either insert handle.abort() to make the implementation match (changing shutdown semantics — abort skips cooperative cleanup) or conclude the doc was wrong and need to retrace which is authoritative. Same B.1-class hazard (comment claims one ordering, code does another). Fix is doc + test-name only: * SHUTDOWN_TIMEOUT doc-comment now spells out detach-not-abort with the technical reason. * Test renamed to shutdown_does_not_block_on_stuck_tasks_after_timeout with a doc-comment clarifying that the elapsed-bounded assertion is what guards against a regression that reintroduces an unbounded handle.await — and that detach behaviour itself cannot be asserted from the test because register() moves the handle. No production behaviour change; semantics already correct. Verification: * cargo test -p lumotia-hotkey --lib --tests → 6 unit + 2 integration = 8/8 pass (unchanged). * cargo fmt --check → clean. * cargo clippy -p lumotia-hotkey --all-targets -- -D warnings → clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/hotkey/src/supervisor.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/crates/hotkey/src/supervisor.rs b/crates/hotkey/src/supervisor.rs index 3c6274f..b8a5c74 100644 --- a/crates/hotkey/src/supervisor.rs +++ b/crates/hotkey/src/supervisor.rs @@ -26,10 +26,12 @@ use tokio::task::JoinHandle; use tokio::time::timeout; /// How long to wait for any single task to drain on `shutdown()` before -/// we give up and abort it. Two seconds is generous for cooperative -/// shutdown via the broadcast channel — anything slower is treated as a -/// stuck task and force-aborted with a warning so the operator can -/// investigate. +/// we give up on it. Two seconds is generous for cooperative shutdown +/// via the broadcast channel — anything slower is treated as a stuck +/// task and detached (NOT aborted: `timeout(d, handle).await` consumes +/// the `JoinHandle` by value and dropping a `JoinHandle` detaches the +/// task, so the task keeps running until the tokio runtime tears it +/// down). A warning is logged so the operator can investigate. const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(2); /// Capacity of the broadcast shutdown channel. Eight is far more than we @@ -207,8 +209,16 @@ mod tests { ); } + /// A stuck task — one that does not subscribe to broadcast shutdown + /// and would otherwise run forever — must not block `shutdown()` + /// past the per-task timeout. Note: the supervisor does NOT abort + /// the task; it detaches it. Verifying detach behaviour directly is + /// not possible from this test because `register()` moves the + /// `JoinHandle` into the supervisor's inner Vec. The bounded-elapsed + /// assertion below is what guards against a regression that + /// reintroduces an unbounded `handle.await` in `shutdown()`. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn shutdown_force_aborts_stuck_tasks_after_timeout() { + async fn shutdown_does_not_block_on_stuck_tasks_after_timeout() { let sup = SupervisorHandle::new(); let handle = tokio::spawn(async move { // Sleep forever — does NOT subscribe to shutdown.