agent: lumotia — Phase B.2 fix misleading "force-abort" doc + test name on supervisor shutdown
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) <noreply@anthropic.com>
This commit is contained in:
@@ -26,10 +26,12 @@ use tokio::task::JoinHandle;
|
|||||||
use tokio::time::timeout;
|
use tokio::time::timeout;
|
||||||
|
|
||||||
/// How long to wait for any single task to drain on `shutdown()` before
|
/// 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
|
/// we give up on it. Two seconds is generous for cooperative shutdown
|
||||||
/// shutdown via the broadcast channel — anything slower is treated as a
|
/// via the broadcast channel — anything slower is treated as a stuck
|
||||||
/// stuck task and force-aborted with a warning so the operator can
|
/// task and detached (NOT aborted: `timeout(d, handle).await` consumes
|
||||||
/// investigate.
|
/// 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);
|
const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(2);
|
||||||
|
|
||||||
/// Capacity of the broadcast shutdown channel. Eight is far more than we
|
/// 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)]
|
#[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 sup = SupervisorHandle::new();
|
||||||
let handle = tokio::spawn(async move {
|
let handle = tokio::spawn(async move {
|
||||||
// Sleep forever — does NOT subscribe to shutdown.
|
// Sleep forever — does NOT subscribe to shutdown.
|
||||||
|
|||||||
Reference in New Issue
Block a user