Forced job_cancel releases the operation locks the aborted worker stranded

[plusky]

Closes #405.

The bug

A forced job_cancel drops the worker future mid-await, so the flow's "unlock always" section never runs and /var/lock/mtui.lock stays held on every host the job had locked — until session eviction (up to ~6h under http, process exit under stdio) or a manual unlock. The ?-free unlock sections in operation.rs and the update flow guard against early return; nothing survives a dropped future.

Why releasing at all is sound

After an abort mtui cannot observe whether the host-side operation ended: no channel close is even sent (russh's Channel has no Drop), lastexit() is a stale snapshot, and no liveness probe exists. The release follows the codebase's own command-timeout precedent — the flows already unlock after a timed-out host op they equally cannot observe, refusing only the automated remediation — and the transaction itself is serialized by the package manager's own system lock where the operation is a package transaction. Accordingly the pass releases locks and does nothing else at the hosts, and the reply keeps the "may still be running on the host" caveat.

What the release is scoped to (the part adversarial review reshaped)

The first implementation released the whole group's locks non-forced. Review showed is_mine() is per-process (user+pid) while /var/lock/mtui.lock is per-host, so that would have removed a tester's deliberate lock reservation, another live job's lock on a cross-template shared refhost, and — under http — another MCP session's lock. The shipped design releases only what the cancelled job's own group actually took:

• TargetLock now records its own acquisitions in memory (held); a load() read never sets it — a read is not a take. Each template's group has its own Target instances, so cross-template, cross-session, and subset-lock (run -t h1) cases are separated structurally.
• Comment-marked locks are skipped: a non-empty comment is the wire contract's own exclusive-reservation marker, and it is exactly the deliberate long-lived holds (PI-assignment relocks, commented lock) that carry one. A bare lock with no comment remains indistinguishable from an operation lock — documented residual.
• The wire format and the owner-checked, fail-closed TargetLock::unlock policy are untouched (lock_format.rs unchanged).

The pass, and its three serialization hazards

Job records its template scope at mint (start_jobs also pins the dispatch argv to that record — incidentally fixing a latent parse error where the multi-template branch prepended -T to an argv already carrying the conflicting --all-templates). On the forced arm, after the aborted worker has fully unwound:

1. Unconditional canonical-guard release first. An aborted exclusive dispatch leaves the canonical session's active entry guard held; a lingering guard makes every later scoped dispatch on that template silently run against the null report (activate is try-lock based and the driver ignores the failure). Review caught that the first version only released it inside the per-template loop — a busy registry gate could skip it entirely, leaving the session poisoned while the reply's own list_locks advice would return a false clean via the same null report.
2. Per template: the same gate-share + per-RRID lock a dispatch takes, so no new dispatch can activate into the entry the pass holds — then the entry lock, then the held-only unlock fan-out.
3. One ABORT_UNLOCK_BUDGET deadline over everything, including the empty-scope fallback that must read the loaded template list under the session mutex (review found that read sitting outside the budget, where an in-flight get/put transfer would have blocked job_cancel unbounded). A bounded cancel outranks a complete release; expiry is reported per-template as unknown, partial results deliberately discarded rather than over-claimed, and the locks stay claimable by unlock.

The reply appends a truthful verdict — released hosts, hosts still locked by another owner (with the unlock --force remedy, since after a process kill "another owner" is usually the tester's own dead-pid debris), failed releases by reason, and expired templates. The cooperative reply and the forced prefix are byte-identical to before.

Tests

Fifteen new tests across both crates, every assertion observed red first — including seven mutants of the fix itself, four of which beat the first-draft tests and forced stronger assertions (a vacuous "a read is not a take" anchor, a scope pin that only held by wall-clock, an unbounded-preamble mutant that hung instead of failing, and an argv-pin test that supplied the pin itself). The concurrency scenarios pinned: cross-template shared host untouched, commented reservation untouched, subset locks, a live sibling job's locks untouched (semantic pin, not timing), the exclusive-path guard release (with the null-report probe), budget expiry attributable to the fan-out (200ms budget vs 400ms injected delay), and the guard-release-on-stalled-preamble reasoning. Suite stress-tested under single-core oversubscription — no new flakes.

Residuals (documented, deliberately out of scope)

• The registry gate is writer-preferring, so a forced cancel racing a load_template can burn the budget and release nothing (reported truthfully as unknown).
• A dropped foreground http request future aborts the dispatch via AbortOnDrop with no job_cancel to run the release — same hole, different entry point.
• REPL Ctrl-C during an update kills the process outright: the stranded lock then has a dead pid, reads as foreign, and only unlock --force or the 24h stale reap clears it (follow-up issue).
• Two pre-existing null-report windows (command_lock's resolve-then-gate gap; close_with_timeout locking entries gate-free) — the new pass narrows its own comment claims accordingly.
• Pre-existing timing flake run_with_heartbeat_send_failure_does_not_mask_result under CPU starvation (follow-up issue).

Process

Three scouts surveyed the abort machinery, lock semantics, and test landscape (and corrected the issue's own "the entry is lockable after the abort" claim); implementation; then three independent read-only adversarial reviews (concurrency, test-vacuity, contracts) over the uncommitted tree — seventeen findings, including the three blocker-class ones above, all fixed before the first commit existed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.32%. Comparing base (49ad1be) to head (af011a0).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #440   +/-   ##
=======================================
  Coverage   96.31%   96.32%           
=======================================
  Files         193      193           
  Lines       42170    42260   +90     
=======================================
+ Hits        40615    40705   +90     
  Misses       1555     1555           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.