Scoping a Server no longer kills it - #728
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #728 +/- ##
==========================================
- Coverage 52.45% 52.35% -0.10%
==========================================
Files 26 26
Lines 3729 3732 +3
Branches 747 747
==========================================
- Hits 1956 1954 -2
- Misses 1469 1474 +5
Partials 304 304 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
846fb7c to
7ea0444
Compare
Code reviewFound 1 issue:
libtmux/docs/topics/context_managers.md Lines 117 to 141 in 7ea0444 "Nested context managers" still says the block gives you "every layer torn down for you", and item 4 reads "The server is killed when exiting its context". The libtmux/docs/topics/context_managers.md Lines 142 to 149 in 7ea0444 "Benefits" then says "you never manually call the {meth} The doctests cannot catch this: the nested block asserts no output, so the prose can drift from the behaviour silently. Lines 145 to 150 in 7ea0444 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
600c845 to
c023026
Compare
why: A Server is a handle on a socket, not a connection -- the same handle addresses a daemon whether or not this process started it, and a bare Server() addresses whichever tmux the caller has open. Scoping one to a block was destroying it, along with every session in it. Inferring ownership from is_alive() was considered and rejected. That predicate folds every way of failing to reach a server into False by contract, so a transient tmux error at __enter__ would have read as "this block started it" and killed a daemon that predated the block -- the same bug through a different door. Fixes #724. what: - Default kill_on_exit to False; __exit__ kills only when asked - __enter__ returns self and probes nothing - Say in the Server docstring that a server, unlike a session, window or pane, does not tear itself down, and why - Rework docs/topics/context_managers.md, which promised unconditional teardown for all four classes - Test the default, the opt-in, and a bare Server() on the default socket -- the shape the report was actually about, which the TestServer factory cannot reproduce
why: The page promised unconditional teardown for all four classes, so a reader following it would expect `with Server()` to kill a server it did not start. what: - Say that a `Server` context leaves the daemon running unless it was built with `kill_on_exit=True`, and show both. - Correct the nested-context walkthrough, whose summary still listed the server among the layers torn down.
c023026 to
60c8056
Compare
why: A context manager that kills a server it did not start can take down the tmux a user has been working in all day, so the change of behaviour needs to be findable before someone upgrades into it. what: - Record the `kill_on_exit` breaking change under `Breaking changes`, with the migration a caller relying on the old teardown needs.
60c8056 to
9114092
Compare
Summary
with Server() as server:destroying a tmux daemon it had nothing to do with.__exit__wasif self.is_alive(): self.kill(), unconditionally — so a block that did no work at all took out the reader's default server and every session in it.kill_on_exitfor when teardown is what you want.Server,__exit__, and indocs/topics/context_managers.md.Why scoping is not ownership
A
Serveris a handle on a socket, not a connection. The same handle addresses a daemon whether or not this process started it, and a bareServer()addresses the default socket — usually the tmux the reader has been working in all day. Entering awithblock says "I want this scoped", not "I am entitled to destroy this".Session,WindowandPaneare different in kind, and stay as they are: you got them by callingnew_session()orsplit(), so the block really did create them.Before / After
Ask for teardown explicitly:
Design decisions
Inference was implemented, then rejected. An earlier revision of this branch had
__enter__record whether the daemon was already alive and__exit__kill only one the block started. It reads well and has real precedent —zipfile._filePassed,tarfile._extfileobj,gzip.myfileobjall decide cleanup by whether they opened the resource. It was dropped for two reasons.The first is a defect. The ownership signal was
is_alive(), which catches bareExceptionand returnsFalseby contract — every way of failing to reach a server is a "no". So a transient tmux failure at__enter__would have read as "this block started it", and__exit__would have killed a daemon that predated the block. That is this issue's bug, reachable through a different door, and closing it would have meant hardening a deliberately lenient predicate into a strict one.The second is that the stdlib's inference cases infer from provenance — what the object did — not from liveness.
Serverhas no provenance to record: the socket does not say who started the daemon. Inference here was a guess wearing the costume of a fact.Explicit opt-in has its own precedent, including in languages built around ownership.
subprocess.Popen.__exit__unambiguously owns its child and still does not kill it.sqlite3.Connection.__exit__commits or rolls back and never closes. Rust'sstd::process::Childhas noimpl Dropat all — "The standard library does not automatically wait on child processes (not even if theChildis dropped), it is up to the application developer to do so."kill_on_exitis a plainbool, not a tri-state. With no inference there is no third state to spell. It also matches the codebase: every one of libtmux's 188bool | Noneparameters is consumed asif x is not None and x:, soNonealready meansFalsehere — aNonemeaning "maybe kill" would have inverted the house idiom in the destructive direction.The name is action-shaped on purpose. libtmux already ships
Pane.display_popup(close_on_exit=...), and tmux's own vocabulary isremain-on-exit,exit-empty. An ownership-shaped name (owns_server) would assert a fact libtmux cannot verify.No
temp_server()helper.temp_serveris already a doc anchor for theTestServerfixture, which creates auto-cleaned servers. A scoped-server helper can be designed on its own merits later.Test plan
test_server_context_manager— the default leaves a server alive, including one the block started. This previously asserted the opposite; the change is the breaking change made visible in the suitetest_server_context_manager_spares_pre_existing_server— a pre-existing server survives an empty blocktest_server_context_manager_spares_the_default_socket— a bareServer()under a monkeypatched$TMUX_TMPDIR. Every other test routes through theTestServerfactory, which mints a private socket and structurally cannot reproduce this issue's shapetest_server_context_manager_kill_on_exit_true— the opt-in kills a server the block did not starttest_server_context_manager_kill_on_exit_true_when_block_started_it— and one it didServer,__enter__,__exit__, plus the reworkeddocs/topics/context_managers.md— all collected and runlibtmux.pytest_plugin, which leans on server teardownuv run ruff check ./uv run ruff format ./uv run mypycleanjust build-docscleanFollow-up, not in this PR
Session.from_session_id,Window.from_window_idandPane.from_pane_idall exist, so a child handle can be obtained without creating the object —with Window.from_window_id(...)kills a window you did not make. Same bug class, smaller blast radius, and worth its own issue rather than widening this one.Note for the merger
CHANGESwill conflict with the sibling PRs for #723, #725 and #726, which all add to the same block — keep both sides. This one lands under### Breaking changes.