diff --git a/CHANGES b/CHANGES index 36712c604..5f3a2fa2e 100644 --- a/CHANGES +++ b/CHANGES @@ -40,6 +40,18 @@ $ uvx --from 'libtmux' --prerelease allow python _Notes on the upcoming release will go here._ +### Fixes + +- `pytest_plugin`: the `server` and `TestServer` fixture finalizers now + unlink the tmux socket file from `/tmp/tmux-/` in addition to + calling `server.kill()`. tmux does not reliably `unlink(2)` its + socket on non-graceful exit, so `/tmp/tmux-/` would otherwise + accumulate stale `libtmux_test*` entries across test runs (10k+ + observed on long-lived dev machines). A new internal helper + `_reap_test_server` centralizes the kill + unlink flow and + suppresses cleanup-time errors so a finalizer failure can no longer + mask the real test failure (#661, fixes #660). + ### Documentation - Visual improvements to API docs from [gp-sphinx](https://gp-sphinx.git-pull.com)-based Sphinx packages (#658) diff --git a/src/libtmux/pytest_plugin.py b/src/libtmux/pytest_plugin.py index 75ad6f400..00a841584 100644 --- a/src/libtmux/pytest_plugin.py +++ b/src/libtmux/pytest_plugin.py @@ -24,6 +24,40 @@ USING_ZSH = "zsh" in os.getenv("SHELL", "") +def _reap_test_server(socket_name: str | None) -> None: + """Kill the tmux daemon on ``socket_name`` and unlink the socket file. + + Invoked from the :func:`server` and :func:`TestServer` fixture + finalizers to guarantee teardown even when the daemon has already + exited (``kill`` is a no-op then) and the socket file was left on + disk. tmux does not reliably ``unlink(2)`` its socket on + non-graceful exit, so ``/tmp/tmux-/`` otherwise accumulates + stale entries across test runs. + + Conservative: suppresses ``LibTmuxException`` / ``OSError`` on both + the kill and the unlink. A finalizer that raises replaces the real + test failure with a cleanup error, and cleanup failures are not + actionable (socket already gone, permissions changed, race with a + concurrent pytest-xdist worker). + """ + if not socket_name: + return + + with contextlib.suppress(exc.LibTmuxException, OSError): + srv = Server(socket_name=socket_name) + if srv.is_alive(): + srv.kill() + + # ``Server(socket_name=...)`` does not populate ``socket_path`` — + # the Server class only derives the path when neither ``socket_name`` + # nor ``socket_path`` was supplied. Recompute the location tmux uses + # so we can unlink the file regardless of daemon state. + tmux_tmpdir = pathlib.Path(os.environ.get("TMUX_TMPDIR", "/tmp")) + socket_path = tmux_tmpdir / f"tmux-{os.geteuid()}" / socket_name + with contextlib.suppress(OSError): + socket_path.unlink(missing_ok=True) + + @pytest.fixture(scope="session") def home_path(tmp_path_factory: pytest.TempPathFactory) -> pathlib.Path: """Temporary `/home/` path.""" @@ -141,7 +175,7 @@ def server( server = Server(socket_name=f"libtmux_test{next(namer)}") def fin() -> None: - server.kill() + _reap_test_server(server.socket_name) request.addfinalizer(fin) @@ -295,11 +329,9 @@ def socket_name_factory() -> str: return f"libtmux_test{next(namer)}" def fin() -> None: - """Kill all servers created with these sockets.""" + """Kill all servers created with these sockets and unlink their sockets.""" for socket_name in created_sockets: - server = Server(socket_name=socket_name) - if server.is_alive(): - server.kill() + _reap_test_server(socket_name) request.addfinalizer(fin) diff --git a/tests/test_pytest_plugin.py b/tests/test_pytest_plugin.py index 59040fe85..23acbcfe9 100644 --- a/tests/test_pytest_plugin.py +++ b/tests/test_pytest_plugin.py @@ -2,17 +2,19 @@ from __future__ import annotations +import contextlib +import os +import pathlib import textwrap import time import typing as t -if t.TYPE_CHECKING: - import pathlib +from libtmux.pytest_plugin import _reap_test_server +from libtmux.server import Server +if t.TYPE_CHECKING: import pytest - from libtmux.server import Server - def test_plugin( pytester: pytest.Pytester, @@ -156,3 +158,66 @@ def test_test_server_multiple(TestServer: t.Callable[..., Server]) -> None: assert any(s.session_name == "test2" for s in server2.sessions) assert not any(s.session_name == "test1" for s in server2.sessions) assert not any(s.session_name == "test2" for s in server1.sessions) + + +def _libtmux_socket_dir() -> pathlib.Path: + """Resolve the tmux socket directory tmux uses for this uid.""" + tmux_tmpdir = pathlib.Path(os.environ.get("TMUX_TMPDIR", "/tmp")) + return tmux_tmpdir / f"tmux-{os.geteuid()}" + + +def test_reap_test_server_unlinks_socket_file() -> None: + """``_reap_test_server`` kills the daemon *and* unlinks the socket. + + Regression for #660: tmux does not reliably ``unlink(2)`` its socket + on non-graceful exit. Before this fix the plugin's finalizer only + called ``server.kill()``, so ``/tmp/tmux-/`` accumulated stale + ``libtmux_test*`` socket files over time. + + This test boots a real tmux daemon on a unique socket, confirms the + socket file exists, invokes the reaper, and asserts the file is + gone. + """ + server = Server(socket_name="libtmux_test_reap_unlink") + server.new_session(session_name="reap_probe") + socket_path = _libtmux_socket_dir() / "libtmux_test_reap_unlink" + try: + assert socket_path.exists(), ( + f"expected tmux to have created {socket_path}, but it is missing" + ) + + _reap_test_server("libtmux_test_reap_unlink") + + assert not socket_path.exists(), ( + f"_reap_test_server should have unlinked {socket_path}" + ) + finally: + # Belt-and-braces: if the assertion above fired before the + # unlink, don't leak the socket the next run of this test. + with contextlib.suppress(OSError): + socket_path.unlink(missing_ok=True) + + +def test_reap_test_server_is_noop_when_socket_missing() -> None: + """Reaping a non-existent socket succeeds silently. + + Finalizers run even when the fixture failed before the daemon ever + started; the reaper must tolerate the case where the socket file + never existed. + """ + bogus_name = "libtmux_test_reap_never_existed_xyz" + socket_path = _libtmux_socket_dir() / bogus_name + assert not socket_path.exists() + + # Should not raise. + _reap_test_server(bogus_name) + + +def test_reap_test_server_tolerates_none() -> None: + """``_reap_test_server(None)`` is a no-op, not a crash. + + The ``server`` fixture's finalizer passes ``server.socket_name``, + which is typed ``str | None``. Tolerate ``None`` for symmetry with + other nullable paths in the API. + """ + _reap_test_server(None)