[projects/git-slug] Naprawa wycieku procesów git/ssh przy Ctrl-C
arekm
arekm at pld-linux.org
Tue May 12 15:08:45 CEST 2026
commit 64c315a29e52ab47c6b1c1f7d9001b6a35703b0d
Author: Arkadiusz Miśkiewicz <arekm at maven.pl>
Date: Tue Apr 7 09:16:48 2026 +0200
Naprawa wycieku procesów git/ssh przy Ctrl-C
Wcześniejszy mechanizm TrackedPopen + start_new_session=True nie działał
z multiprocessing.Pool: rejestracja procesów odbywała się w forkowanych
workerach, więc parent's _active był zawsze pusty i kill_tracked_subprocesses()
nie miał czego zabić. Dodatkowo workery ignorują SIGINT (signal.SIG_IGN
w _pool_worker_init), a dzieci git/ssh dziedziczą tę dyspozycję przez exec
(restore_signals przywraca tylko SIGPIPE/SIGXFZ/SIGXFSZ, nie SIGINT).
W rezultacie po Ctrl-C w terminalu pool.terminate() zabijał tylko workery,
a procesy git/ssh przeżywały jako sieroty w tle.
Rozwiązanie: w child procesie przywrócić domyślną obsługę SIGINT przed
exec za pomocą preexec_fn=_reset_sigint. Wtedy Ctrl-C w terminalu trafia
w cały foreground process group: git/ssh giną od razu, workery (SIG_IGN)
żyją wystarczająco długo by parent złapał KeyboardInterrupt i zamknął
pool czysto.
Zmiany:
- git_slug/gitrepo.py: command() używa Popen(..., preexec_fn=_reset_sigint),
init_gitdir() używa subprocess.call(..., preexec_fn=_reset_sigint)
(init_gitdir jest wołane z initpackage() w worker pool podczas
clone i update --new)
- slug.py: git_passthrough_worker() używa subprocess.Popen z preexec_fn,
importuje _reset_sigint z gitrepo, kill_tracked_subprocesses() usunięty
z handlera KeyboardInterrupt
- git_slug/proctrack.py: usunięty (TrackedPopen i kill_all już niepotrzebne)
- tests/test_proctrack.py: usunięty
- tests/test_passthrough.py: FakeTrackedPopen przemianowany na FakePopen,
monkeypatch celuje w slug.subprocess.Popen
- tests/test_failure_paths.py: usunięty mock kill_tracked_subprocesses
- tests/test_gitrepo.py: stuby subprocess.call akceptują **kwargs
- README.md: usunięte odwołania do proctrack
README.md | 3 +-
git_slug/gitrepo.py | 19 +++-
git_slug/proctrack.py | 96 --------------------
slug.py | 18 ++--
tests/test_failure_paths.py | 1 -
tests/test_gitrepo.py | 6 +-
tests/test_passthrough.py | 16 ++--
tests/test_proctrack.py | 207 --------------------------------------------
8 files changed, 40 insertions(+), 326 deletions(-)
---
diff --git a/README.md b/README.md
index e5b0634..7872262 100644
--- a/README.md
+++ b/README.md
@@ -70,7 +70,6 @@ Current test coverage is split roughly by responsibility:
- `tests/test_slug_commands.py` checks `update`, `clone`, `list`, and `init` command wiring.
- `tests/test_failure_paths.py` checks common error paths that previously regressed.
- `tests/test_refsdata.py` and `tests/test_gitrepo.py` cover small helper-module behavior.
-- `tests/test_proctrack.py` tests subprocess tracking for clean Ctrl-C shutdown.
- `tests/test_benchmark.py` performance benchmarks (pytest-benchmark).
## Setup
@@ -212,7 +211,7 @@ git pld update --help # slug update options
## Project structure
- `slug.py` — client CLI entry point
-- `git_slug/` — client library (gitrepo, refsdata, proctrack, constants)
+- `git_slug/` — client library (gitrepo, refsdata, constants)
- `server/` — server-side components (gitolite hooks, slug_watch daemon, admin commands); see `server/README.md`
- `doc/man/` — man page sources (asciidoc); build with `make man`
- `tests/` — test suite
diff --git a/git_slug/gitrepo.py b/git_slug/gitrepo.py
index 336edce..d077a67 100644
--- a/git_slug/gitrepo.py
+++ b/git_slug/gitrepo.py
@@ -1,11 +1,21 @@
from .gitconst import EMPTYSHA1, REMOTE_NAME, REFFILE
-from .proctrack import TrackedPopen
import os
-from subprocess import PIPE
+import signal
+from subprocess import PIPE, Popen
import subprocess
import sys
+
+def _reset_sigint():
+ """Restore default SIGINT handling before exec in child processes.
+
+ Worker processes ignore SIGINT (so only the parent handles Ctrl-C),
+ but children inherit that disposition across exec. Without this,
+ git/ssh subprocesses would survive Ctrl-C as background orphans.
+ """
+ signal.signal(signal.SIGINT, signal.SIG_DFL)
+
class GitRepoError(Exception):
pass
@@ -30,7 +40,8 @@ class GitRepo:
self._loose_refs = {}
def command(self, clist):
- return TrackedPopen(self.command_prefix + clist, stdout=PIPE, stderr=PIPE, bufsize=-1)
+ return Popen(self.command_prefix + clist, stdout=PIPE, stderr=PIPE,
+ bufsize=-1, preexec_fn=_reset_sigint)
def commandio(self, clist):
return self.command(clist).communicate()
@@ -76,7 +87,7 @@ class GitRepo:
clist.append(self.wtree)
else:
clist.extend(['--bare', self.gdir])
- if subprocess.call(clist):
+ if subprocess.call(clist, preexec_fn=_reset_sigint):
raise GitRepoError(self.gdir)
def init(self, remotepull, remotepush = None, remotename=REMOTE_NAME):
diff --git a/git_slug/proctrack.py b/git_slug/proctrack.py
deleted file mode 100644
index c8351c7..0000000
--- a/git_slug/proctrack.py
+++ /dev/null
@@ -1,96 +0,0 @@
-"""Subprocess tracker for clean Ctrl-C shutdown.
-
-git-pld uses ThreadPool for parallel git operations because the work is
-entirely I/O-bound (waiting for git subprocesses). ThreadPool cannot kill
-blocked worker threads, so we track every spawned git subprocess here and
-kill them all on interrupt — unblocking the worker threads so the pool can
-be joined promptly.
-
-Each subprocess is started in its own session (start_new_session=True) so
-that os.killpg() kills the entire process group — git plus any helpers it
-spawns (ssh, credential helpers, etc.).
-
-This affects ALL git operations globally (fetch, checkout, config, show...),
-not just passthrough. Since git-pld is batch-only with no TTY interaction,
-running in a separate session is safe — no git helper needs terminal access.
-
-Usage:
- # In worker code — use TrackedPopen instead of subprocess.Popen.
- # It auto-registers on creation, auto-unregisters on communicate()/wait().
- proc = TrackedPopen(cmd, stdout=PIPE, stderr=PIPE)
- out, err = proc.communicate()
-
- # On Ctrl-C — kill all tracked process groups from the main thread.
- kill_all()
-"""
-
-import os
-import signal
-import subprocess
-import threading
-
-# Thread-safe set of currently running TrackedPopen instances.
-_active = set()
-_lock = threading.Lock()
-
-
-class TrackedPopen(subprocess.Popen):
- """Popen subclass that auto-registers for Ctrl-C tracking.
-
- Automatically registers on __init__, unregisters on communicate(),
- wait(), and poll() — covering all calling patterns in the codebase:
- - commandexc()/commandio() use communicate()
- - refsdata.py uses wait()
- - showfile() callers use poll() + stdout.read()
-
- Callers MUST call communicate() or wait() to unregister. All current
- callers do. showfile() callers are covered by poll() which unregisters
- once the process is finished.
-
- No __del__ safety net: _active holds strong references so __del__ would
- never fire while the object is tracked (gc can't collect reachable objects).
- """
-
- def __init__(self, *args, **kwargs):
- # Start in own session so kill_all()'s killpg() reaches helper
- # children (ssh, credential helpers, etc.), not just the git process.
- kwargs.setdefault('start_new_session', True)
- super().__init__(*args, **kwargs)
- with _lock:
- _active.add(self)
-
- def _untrack(self):
- with _lock:
- _active.discard(self)
-
- def communicate(self, *args, **kwargs):
- try:
- return super().communicate(*args, **kwargs)
- finally:
- self._untrack()
-
- def wait(self, *args, **kwargs):
- try:
- return super().wait(*args, **kwargs)
- finally:
- self._untrack()
-
- def poll(self):
- ret = super().poll()
- if ret is not None:
- self._untrack()
- return ret
-
-
-def kill_all():
- """Kill all tracked process groups — called on Ctrl-C.
-
- Sends SIGTERM to each process group (not just the parent PID) so that
- child processes like ssh are also terminated promptly.
- """
- with _lock:
- for proc in list(_active):
- try:
- os.killpg(proc.pid, signal.SIGTERM)
- except (OSError, ProcessLookupError):
- pass
diff --git a/slug.py b/slug.py
index 35b6489..5ec173b 100755
--- a/slug.py
+++ b/slug.py
@@ -35,8 +35,7 @@ from multiprocessing import Pool as WorkerPool
from git_slug.gitconst import (GITLOGIN, GITSERVER, GIT_REPO, GIT_REPO_PUSH,
REMOTE_NAME, REMOTEREFS)
-from git_slug.gitrepo import GitRepo, GitRepoError
-from git_slug.proctrack import TrackedPopen, kill_all as kill_tracked_subprocesses
+from git_slug.gitrepo import GitRepo, GitRepoError, _reset_sigint
from git_slug.refsdata import GitArchiveRefsData, NoMatchedRepos, RemoteRefsError
@@ -406,9 +405,15 @@ def run_worker(function, options, args):
pool.join()
except KeyboardInterrupt:
print('Keyboard interrupt received, finishing...', file=sys.stderr)
- pool.terminate()
- kill_tracked_subprocesses()
- pool.join()
+ # Block SIGINT during cleanup — pool.terminate() does IPC (pickling
+ # into a Queue) which can itself be interrupted by a second Ctrl-C,
+ # producing ugly tracebacks from multiprocessing internals.
+ prev_handler = signal.signal(signal.SIGINT, signal.SIG_IGN)
+ try:
+ pool.terminate()
+ pool.join()
+ finally:
+ signal.signal(signal.SIGINT, prev_handler)
sys.exit(1)
return ret
@@ -462,7 +467,8 @@ def git_passthrough_worker(repo_dir, git_command, git_args, config_pairs, quiet)
"""
directory = os.path.basename(repo_dir)
cmd = build_git_cmd(repo_dir, git_command, git_args, config_pairs)
- proc = TrackedPopen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+ proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+ preexec_fn=_reset_sigint)
stdout_bytes, stderr_bytes = proc.communicate()
stdout_str = stdout_bytes.decode('utf-8', errors='replace')
stderr_str = stderr_bytes.decode('utf-8', errors='replace')
diff --git a/tests/test_failure_paths.py b/tests/test_failure_paths.py
index df56343..a1a2664 100644
--- a/tests/test_failure_paths.py
+++ b/tests/test_failure_paths.py
@@ -169,7 +169,6 @@ def test_run_worker_handles_keyboard_interrupt(monkeypatch, make_options, capsys
state["joined"] = True
monkeypatch.setattr(slug, "WorkerPool", FakePool)
- monkeypatch.setattr(slug, "kill_tracked_subprocesses", lambda: None)
with pytest.raises(SystemExit) as exc:
slug.run_worker(object(), make_options(jobs=3), [("dummy",)])
diff --git a/tests/test_gitrepo.py b/tests/test_gitrepo.py
index 996c11f..d5bfb46 100644
--- a/tests/test_gitrepo.py
+++ b/tests/test_gitrepo.py
@@ -60,7 +60,8 @@ def test_fetch_adds_depth_when_requested(monkeypatch):
def test_init_gitdir_uses_worktree_when_gitdir_is_inside_worktree(monkeypatch):
calls = []
- monkeypatch.setattr(gitrepo_module.subprocess, "call", lambda clist: calls.append(clist) or 0)
+ monkeypatch.setattr(gitrepo_module.subprocess, "call",
+ lambda clist, **kwargs: calls.append(clist) or 0)
GitRepo("/work/pkg").init_gitdir()
@@ -69,7 +70,8 @@ def test_init_gitdir_uses_worktree_when_gitdir_is_inside_worktree(monkeypatch):
def test_init_gitdir_uses_bare_mode_for_external_gitdir(monkeypatch):
calls = []
- monkeypatch.setattr(gitrepo_module.subprocess, "call", lambda clist: calls.append(clist) or 0)
+ monkeypatch.setattr(gitrepo_module.subprocess, "call",
+ lambda clist, **kwargs: calls.append(clist) or 0)
GitRepo("/work/pkg", "/srv/git/pkg.git").init_gitdir()
diff --git a/tests/test_passthrough.py b/tests/test_passthrough.py
index b3c0f5c..658e8e3 100644
--- a/tests/test_passthrough.py
+++ b/tests/test_passthrough.py
@@ -24,8 +24,8 @@ def test_build_git_cmd_includes_config_and_repo_dir():
]
-class FakeTrackedPopen:
- """Fake TrackedPopen for testing git_passthrough_worker output handling."""
+class FakePopen:
+ """Fake Popen for testing git_passthrough_worker output handling."""
def __init__(self, returncode, stdout, stderr):
self.returncode = returncode
self._stdout = stdout.encode() if isinstance(stdout, str) else stdout
@@ -37,9 +37,9 @@ class FakeTrackedPopen:
def test_git_passthrough_worker_prints_all_output_for_failures(monkeypatch, capsys):
monkeypatch.setattr(
- slug,
- "TrackedPopen",
- lambda *args, **kwargs: FakeTrackedPopen(1, "stdout line\n", "stderr line\n"),
+ slug.subprocess,
+ "Popen",
+ lambda *args, **kwargs: FakePopen(1, "stdout line\n", "stderr line\n"),
)
result = slug.git_passthrough_worker(
@@ -58,9 +58,9 @@ def test_git_passthrough_worker_prints_all_output_for_failures(monkeypatch, caps
def test_git_passthrough_worker_quiet_suppresses_success_stdout_only(monkeypatch, capsys):
monkeypatch.setattr(
- slug,
- "TrackedPopen",
- lambda *args, **kwargs: FakeTrackedPopen(0, "all clean\n", "warning message\n"),
+ slug.subprocess,
+ "Popen",
+ lambda *args, **kwargs: FakePopen(0, "all clean\n", "warning message\n"),
)
result = slug.git_passthrough_worker(
diff --git a/tests/test_proctrack.py b/tests/test_proctrack.py
deleted file mode 100644
index a2385af..0000000
--- a/tests/test_proctrack.py
+++ /dev/null
@@ -1,207 +0,0 @@
-"""Tests for git_slug.proctrack — TrackedPopen and kill_all."""
-
-import os
-import signal
-import subprocess
-import sys
-import time
-
-import pytest
-
-from git_slug.proctrack import TrackedPopen, kill_all, _active, _lock
-
-
-def _clear_active():
- """Remove all entries from the active tracking set."""
- with _lock:
- _active.clear()
-
-
-class TestTrackedPopenRegistration:
- """TrackedPopen auto-registers on init and unregisters on communicate/wait/poll."""
-
- def setup_method(self):
- _clear_active()
-
- def teardown_method(self):
- _clear_active()
-
- def test_register_on_init_unregister_on_communicate(self):
- proc = TrackedPopen(
- [sys.executable, "-c", "print('hello')"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
- # Registered immediately after creation
- with _lock:
- assert proc in _active
-
- proc.communicate()
-
- # Unregistered after communicate()
- with _lock:
- assert proc not in _active
-
- def test_register_on_init_unregister_on_wait(self):
- proc = TrackedPopen(
- [sys.executable, "-c", "pass"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
- with _lock:
- assert proc in _active
-
- proc.wait()
-
- with _lock:
- assert proc not in _active
-
- def test_unregister_on_poll_when_finished(self):
- proc = TrackedPopen(
- [sys.executable, "-c", "pass"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
- # Wait for the process to actually finish
- proc.wait()
- # Re-add to simulate the scenario where only poll() is used
- with _lock:
- _active.add(proc)
-
- ret = proc.poll()
-
- assert ret is not None
- with _lock:
- assert proc not in _active
-
- def test_poll_keeps_tracked_while_running(self):
- # Use a process that sleeps briefly so poll returns None
- proc = TrackedPopen(
- [sys.executable, "-c", "import time; time.sleep(5)"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
- try:
- ret = proc.poll()
- # Process still running — poll returns None, stays tracked
- assert ret is None
- with _lock:
- assert proc in _active
- finally:
- proc.terminate()
- proc.wait()
-
- def test_communicate_returns_output(self):
- proc = TrackedPopen(
- [sys.executable, "-c", "import sys; print('out'); print('err', file=sys.stderr)"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
- out, err = proc.communicate()
- assert b"out" in out
- assert b"err" in err
-
- def test_start_new_session_default(self):
- """TrackedPopen starts in its own session by default."""
- proc = TrackedPopen(
- [sys.executable, "-c", "pass"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
- proc.wait()
- # The process should have its own session (pgid == pid)
- # We can't easily test this after the process exits, but we
- # can verify start_new_session was set by checking the process
- # ran without error.
- assert proc.returncode == 0
-
-
-class TestKillAll:
- """kill_all() sends SIGTERM to entire process groups."""
-
- def setup_method(self):
- _clear_active()
-
- def teardown_method(self):
- _clear_active()
-
- def test_kill_all_terminates_running_process(self):
- """kill_all() terminates a tracked process via process group signal."""
- proc = TrackedPopen(
- [sys.executable, "-c", "import time; time.sleep(60)"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
- with _lock:
- assert proc in _active
-
- kill_all()
-
- # Process should be terminated (may take a moment)
- proc.wait(timeout=5)
- # SIGTERM results in negative returncode on POSIX
- assert proc.returncode != 0
-
- def test_kill_all_reaches_child_processes(self):
- """kill_all() kills the process group, including child processes."""
- # Parent spawns a child that also sleeps. Both should be killed
- # because they share a process group (start_new_session=True makes
- # the parent the group leader).
- script = (
- "import subprocess, time; "
- "subprocess.Popen(['sleep', '60']); "
- "time.sleep(60)"
- )
- proc = TrackedPopen(
- [sys.executable, "-c", script],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
-
- # Brief pause so the child subprocess actually starts
- time.sleep(0.2)
-
- kill_all()
-
- proc.wait(timeout=5)
- assert proc.returncode != 0
-
- def test_kill_all_ignores_already_dead_processes(self):
- """kill_all() doesn't crash on processes that already exited."""
- proc = TrackedPopen(
- [sys.executable, "-c", "pass"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
- proc.wait()
- # Process is dead but still tracked (wait() unregistered it,
- # so re-add to test the edge case)
- with _lock:
- _active.add(proc)
-
- # Should not raise
- kill_all()
-
-
-class TestWithGitRepo:
- """TrackedPopen works correctly when used via GitRepo."""
-
- def setup_method(self):
- _clear_active()
-
- def teardown_method(self):
- _clear_active()
-
- def test_commandexc_unregisters_after_completion(self):
- """GitRepo.commandexc() properly unregisters the TrackedPopen."""
- from git_slug.gitrepo import GitRepo
-
- repo = GitRepo(None, None)
- # 'git --version' should succeed everywhere
- repo.command_prefix = ["git"]
- out, err = repo.commandexc(["--version"])
-
- assert b"git version" in out
- # After commandexc returns, the process should be untracked
- with _lock:
- assert len(_active) == 0
================================================================
---- gitweb:
http://git.pld-linux.org/gitweb.cgi/projects/git-slug.git/commitdiff/4a7e426b8f1a3571094b5dc89412bc49b8f29666
More information about the pld-cvs-commit
mailing list