Bound a tmux command that may never return - #735
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## docs/lift-automation-patterns #735 +/- ##
=================================================================
+ Coverage 52.40% 52.56% +0.16%
=================================================================
Files 26 26
Lines 3729 3744 +15
Branches 747 749 +2
=================================================================
+ Hits 1954 1968 +14
Misses 1472 1472
- Partials 303 304 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6c1c5b7 to
a09ab07
Compare
f657e4c to
0c4f728
Compare
a09ab07 to
d4c8f7f
Compare
0c4f728 to
24085b2
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
d4c8f7f to
80c8797
Compare
24085b2 to
39e87af
Compare
80c8797 to
3f81cf8
Compare
39e87af to
d172f7c
Compare
Code reviewFound 1 issue, fixed in d172f7c:
Both 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
3f81cf8 to
5d304cb
Compare
d172f7c to
e4e1f92
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
d0f7a2b to
9378bed
Compare
Code reviewFound 1 issue, fixed in 9378bed:
https://github.com/tmux-python/libtmux/blob/e4e1f920d3b8a5c7f9e1d2a4b6c8e0f2a4c6d8e0/CHANGES#L64-L68 All five now say the same thing: the kill covers the tmux client libtmux spawned, and work the command started keeps running. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
5d304cb to
c5aa46c
Compare
9378bed to
044fa42
Compare
c5aa46c to
21de528
Compare
044fa42 to
12c2afc
Compare
21de528 to
8d35933
Compare
why: A tmux command that blocks on something that may never happen blocks the calling process for its whole life. `wait-for` is a rendezvous with no clock on it, and `communicate()` was called with no ceiling, so there was no bound anywhere beneath it. what: - Add keyword-only `timeout` to `tmux_cmd`, forwarded to `Popen.communicate()`. `None`, the default, waits as long as tmux takes, so existing callers are unaffected. - Kill and reap the tmux process before raising. `communicate()` leaves the child running when its timeout expires, and `kill()` alone leaves a zombie, so an unhandled expiry leaks one process per call. - Add `TmuxCommandTimeout`, carrying the killed command line and the bound it exceeded. It subclasses `WaitTimeout`, so handlers that already catch libtmux's timeout keep working, and it deliberately does not subclass `subprocess.TimeoutExpired` -- the stdlib type never crosses the library boundary, including by chaining.
why: `Server.wait_for()` wraps `tmux wait-for`, the poll-free rendezvous that is the right way to wait on a command you authored. Whatever was going to signal the channel can die first -- the command is killed, the shell exits, the window closes -- and the wait then never ends, so the primitive could not be recommended in documentation. what: - Add keyword-only `timeout` to `Server.wait_for()`, and to `cmd()` on `Server`, `Session`, `Window` and `Pane`, forwarding to `tmux_cmd`. Every entry point defaults to `None`, which is unbounded. - The bound rides on the call rather than the object: a tmux command set is not homogeneous, so one server-wide number cannot be right for both `display-message` and `attach-session`. - Cover the reported case (keys typed but never entered, so nothing ever signals), that a signalled channel still resolves, that the error is a libtmux one carrying the killed command, and that every entry point still defaults to unbounded. Fixes #732
why: The page taught polling with a bound, which costs a tmux round-trip per read and a sleep paid whether the command finished or not. tmux offers a rendezvous instead, and it is worth teaching next to the poll so a reader can see the trade. what: - Add "Waiting for a signal instead of polling": a pane signals with `tmux wait-for -S` and the script waits with `Server.wait_for()`, bounded so a signal that never arrives is catchable. - Record two properties of the rendezvous a reader will otherwise meet the hard way. A wait that times out is abandoned rather than withdrawn, and tmux only remembers a signal when nothing is waiting, so the next signal on that channel is spent waking the abandoned wait -- one signal, after which the channel behaves normally. A fresh channel name per rendezvous avoids it. - Note that a returning wait is not proof the work succeeded: tmux releases every waiter on server shutdown exactly as if the channel had been signalled, so a pane that died and a command that finished look the same from the wait alone.
why: `Server.wait_for()` could block for the life of the process, and downstream code was already working around it by killing the tmux child itself. what: - Record the `timeout` parameter and `TmuxCommandTimeout` under `What's new`, naming what the kill covers and what it does not.
12c2afc to
2a58783
Compare
Closes #732. Stacked on #734, which owns
docs/topics/automation_patterns.md.What this does
Server.wait_for()wrapstmux wait-for, a rendezvous with no clock on it. When whatever was going to signal the channel dies first — the command is killed, the shell exits, the window closes — the wait never ends, and there was no ceiling anywhere beneath it either:communicate()was called without one.timeoutis keyword-only and available onwait_for()and oncmd()forServer,Session,WindowandPane, forwarding totmux_cmd. Every entry point defaults toNone, which is unbounded, so no existing call changes behaviour.Why the bound rides on the call
An instance-level default (
Server(timeout=30)) was built and measured before this was chosen. It works, and it breaks blocking-by-design commands: onServer(timeout=1.0),server.cmd("attach-session", ...)is killed at ~1.06s. Mitigating it needs a hand-maintained deny-list that by construction cannot cover a command a user dispatches themselves.The reason is that the analogy to HTTP clients does not hold.
httpxgets away with a client-level timeout because requests are homogeneous; tmux's command set is not.display-messageshould take milliseconds andattach-sessionshould take hours, so no single per-server number is correct, and it is wrong precisely where being wrong hurts most.Per-call also stays well-defined under batching, which matters for the engine work: a timeout carried by the request has an unambiguous meaning in a batch, where an engine-level default or an ambient scope has to choose between bounding the batch and bounding each command in it.
Killing is not enough
communicate()does not kill the child when its timeout expires, andkill()alone leaves a zombie. Both are covered, because the difference is invisible to the obvious assertion —os.kill(pid, 0)succeeds against a zombie. The test pinsreturncode == -SIGKILLand thatos.waitpidraisesChildProcessError.TmuxCommandTimeoutcarries the killed command line and the bound it exceeded. It subclassesWaitTimeout, so handlers that already catch libtmux's timeout keep working, and it deliberately does not subclasssubprocess.TimeoutExpired— the stdlib type never crosses the library boundary, including by chaining.Two tmux behaviours documented
Both were measured against tmux 3.7b while building this, and both are properties of the rendezvous rather than of this change.
A wait that times out is abandoned rather than withdrawn. tmux only remembers a signal when nothing is waiting for the channel (
cmd-wait-for.c#L148), so the next signal is spent waking the abandoned wait — one signal, after which the channel behaves normally. A fresh channel name per rendezvous avoids it.A returning wait is also weaker evidence than it looks. tmux releases every waiter when the server shuts down (
cmd-wait-for.c#L244), and does so exactly as if the channel had been signalled, so a pane that died and a command that finished are indistinguishable from the wait alone. When the outcome matters, have the command report its own exit status.Verification
ruff check,ruff format,mypy(69 files),py.test --reruns 0(1468 passed, 1 skipped) andjust build-docsall clean.