From bbf892aecca14fdd08a28d5077451b7a6fd925a5 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sat, 25 Apr 2026 17:17:47 -0400 Subject: [PATCH] refactor: extract subprocess cleanup into shared subproc helper (#210) bird_x.py and youtube_yt.py had four near-identical copies of the same subprocess cleanup dance (Popen + os.setsid + communicate(timeout) + SIGTERM via killpg + proc.kill() fallback + wait(5)). Extract to lib.subproc.run_with_timeout(), which: - runs the child in its own process group via os.setsid where available - raises SubprocTimeout on timeout - on timeout: SIGTERM the group, fall back to proc.kill(), wait up to 5s - accepts an on_pid callback so bird_x can still register child PIDs with last30days.register_child_pid for whole-process cleanup - captures stdout/stderr as strings in a SubprocResult dataclass Migrated call sites: _run_bird_search, search_handles inner worker, search_youtube, fetch_transcript. With the helper in place, the signal and subprocess imports became dead in both files (plus os in youtube_yt) and went with them. Tests: 9 new subproc tests cover success, non-zero exit, stderr capture, timeout-raises, timeout-kills-group, missing-command, env passthrough, PID callback, and callback-exception suppression. test_env_v3 and test_youtube_yt patch subproc.run_with_timeout instead of the removed bird_x.subprocess and yt-dlp subprocess. --- skills/last30days/scripts/lib/bird_x.py | 129 ++++++++------------ skills/last30days/scripts/lib/subproc.py | 94 ++++++++++++++ skills/last30days/scripts/lib/youtube_yt.py | 54 ++------ tests/test_env_v3.py | 5 +- tests/test_subproc.py | 102 ++++++++++++++++ tests/test_youtube_yt.py | 14 ++- 6 files changed, 268 insertions(+), 130 deletions(-) create mode 100644 skills/last30days/scripts/lib/subproc.py create mode 100644 tests/test_subproc.py diff --git a/skills/last30days/scripts/lib/bird_x.py b/skills/last30days/scripts/lib/bird_x.py index d2b723f..72f9bab 100644 --- a/skills/last30days/scripts/lib/bird_x.py +++ b/skills/last30days/scripts/lib/bird_x.py @@ -7,13 +7,11 @@ See scripts/lib/vendor/bird-search/package.json for authoritative version. import json import os -import signal import shutil -import subprocess import sys from pathlib import Path -from . import http, log +from . import http, log, subproc from datetime import datetime from typing import Any, Dict, List, Optional, Tuple @@ -168,62 +166,51 @@ def _run_bird_search(query: str, count: int, timeout: int) -> Dict[str, Any]: "--json", ] - # Use process groups for clean cleanup on timeout/kill - preexec = os.setsid if hasattr(os, 'setsid') else None + pid_holder: list[int] = [] - try: - proc = subprocess.Popen( - cmd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - encoding="utf-8", - errors="replace", - preexec_fn=preexec, - env=_subprocess_env(), - ) - - # Register for cleanup tracking (if available) + def _register(pid: int) -> None: + pid_holder.append(pid) try: - from last30days import register_child_pid, unregister_child_pid - register_child_pid(proc.pid) + from last30days import register_child_pid + register_child_pid(pid) except ImportError: pass - try: - stdout, stderr = proc.communicate(timeout=timeout) - except subprocess.TimeoutExpired: - # Kill the entire process group - try: - os.killpg(os.getpgid(proc.pid), signal.SIGTERM) - except (ProcessLookupError, PermissionError, OSError): - proc.kill() - proc.wait(timeout=5) - return {"error": f"Search timed out after {timeout}s", "items": []} - finally: + try: + result = subproc.run_with_timeout( + cmd, + timeout=timeout, + env=_subprocess_env(), + on_pid=_register, + ) + except subproc.SubprocTimeout: + return {"error": f"Search timed out after {timeout}s", "items": []} + except Exception as e: + return {"error": str(e), "items": []} + finally: + if pid_holder: try: from last30days import unregister_child_pid - unregister_child_pid(proc.pid) + unregister_child_pid(pid_holder[0]) except Exception: pass - if proc.returncode != 0: - error = stderr.strip() if stderr else "Bird search failed" - return {"error": error, "items": []} + if result.returncode != 0: + error = result.stderr.strip() or "Bird search failed" + return {"error": error, "items": []} - output = stdout.strip() if stdout else "" - if not output: - return {"items": []} + output = result.stdout.strip() + if not output: + return {"items": []} + try: parsed = json.loads(output) - if isinstance(parsed, list): - return {"items": parsed} - return parsed - except json.JSONDecodeError as e: return {"error": f"Invalid JSON response: {e}", "items": []} - except Exception as e: - return {"error": str(e), "items": []} + + if isinstance(parsed, list): + return {"items": parsed} + return parsed def search_x( @@ -330,47 +317,29 @@ def search_handles( "--json", ] - preexec = os.setsid if hasattr(os, 'setsid') else None + try: + result = subproc.run_with_timeout(cmd, timeout=15, env=_subprocess_env()) + except subproc.SubprocTimeout: + _log(f"Handle search timed out for @{handle}") + return [] + except OSError as e: + _log(f"Handle search error for @{handle}: {e}") + return [] + + if result.returncode != 0: + _log(f"Handle search failed for @{handle}: {result.stderr.strip()}") + return [] + + output = result.stdout.strip() + if not output: + return [] try: - proc = subprocess.Popen( - cmd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - encoding="utf-8", - errors="replace", - preexec_fn=preexec, - env=_subprocess_env(), - ) - - try: - stdout, stderr = proc.communicate(timeout=15) - except subprocess.TimeoutExpired: - try: - os.killpg(os.getpgid(proc.pid), signal.SIGTERM) - except (ProcessLookupError, PermissionError, OSError): - proc.kill() - proc.wait(timeout=5) - _log(f"Handle search timed out for @{handle}") - return [] - - if proc.returncode != 0: - _log(f"Handle search failed for @{handle}: {(stderr or '').strip()}") - return [] - - output = (stdout or "").strip() - if not output: - return [] - response = json.loads(output) - return parse_bird_response(response, query=core_topic) - except json.JSONDecodeError: _log(f"Invalid JSON from handle search for @{handle}") - except (OSError, subprocess.SubprocessError) as e: - _log(f"Handle search error for @{handle}: {e}") - return [] + return [] + return parse_bird_response(response, query=core_topic) from concurrent.futures import ThreadPoolExecutor, as_completed diff --git a/skills/last30days/scripts/lib/subproc.py b/skills/last30days/scripts/lib/subproc.py new file mode 100644 index 0000000..bc5772e --- /dev/null +++ b/skills/last30days/scripts/lib/subproc.py @@ -0,0 +1,94 @@ +"""Subprocess helpers: safe timeout + process-group cleanup. + +Used by bird_x.py (Node.js Bird search) and youtube_yt.py (yt-dlp search +and transcript download). Both need the same os.setsid/killpg cleanup +dance on timeout to avoid orphaning child processes. +""" + +from __future__ import annotations + +import os +import signal +import subprocess +from dataclasses import dataclass +from typing import Optional, Sequence + + +class SubprocTimeout(Exception): + """Raised when a subprocess exceeds its timeout and is killed.""" + + +@dataclass +class SubprocResult: + """Result of a subprocess run that captured stdout and stderr.""" + + returncode: int + stdout: str + stderr: str + + +def run_with_timeout( + cmd: Sequence[str], + *, + timeout: int, + env: Optional[dict] = None, + on_pid: Optional[callable] = None, +) -> SubprocResult: + """Run a subprocess with process-group cleanup on timeout. + + Spawns ``cmd`` inside its own process group via ``os.setsid`` where + available. If ``communicate(timeout=...)`` raises ``TimeoutExpired``, + signals ``SIGTERM`` to the entire group, falls back to ``proc.kill()`` + if the signal fails, then waits up to 5 seconds for cleanup, and + raises ``SubprocTimeout``. + + Args: + cmd: Command and arguments to spawn. + timeout: Timeout in seconds passed to ``communicate()``. + env: Optional environment dict. If None, inherits parent env. + on_pid: Optional callable invoked with the child PID right after + spawn. Used by bird_x.py to register child PIDs for cleanup + tracking. Exceptions raised by the callback are suppressed. + + Returns: + SubprocResult with returncode, stdout, and stderr as strings. + + Raises: + SubprocTimeout: If the process exceeded ``timeout``. + FileNotFoundError: If the executable is not found. + OSError: For other spawn failures. + """ + preexec = os.setsid if hasattr(os, "setsid") else None + + proc = subprocess.Popen( + list(cmd), + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + encoding="utf-8", + errors="replace", + preexec_fn=preexec, + env=env, + ) + + if on_pid is not None: + try: + on_pid(proc.pid) + except Exception: + pass + + try: + stdout, stderr = proc.communicate(timeout=timeout) + except subprocess.TimeoutExpired: + try: + os.killpg(os.getpgid(proc.pid), signal.SIGTERM) + except (ProcessLookupError, PermissionError, OSError): + proc.kill() + proc.wait(timeout=5) + raise SubprocTimeout(f"Command {cmd[0]} timed out after {timeout}s") + + return SubprocResult( + returncode=proc.returncode, + stdout=stdout or "", + stderr=stderr or "", + ) diff --git a/skills/last30days/scripts/lib/youtube_yt.py b/skills/last30days/scripts/lib/youtube_yt.py index 7d71f72..03001c2 100644 --- a/skills/last30days/scripts/lib/youtube_yt.py +++ b/skills/last30days/scripts/lib/youtube_yt.py @@ -8,11 +8,8 @@ Inspired by Peter Steinberger's toolchain approach (yt-dlp + summarize CLI). import json import math -import os import re -import signal import shutil -import subprocess import sys import tempfile import urllib.error @@ -37,7 +34,7 @@ TRANSCRIPT_LIMITS = { # Max words to keep from each transcript TRANSCRIPT_MAX_WORDS = 5000 -from . import http, log +from . import http, log, subproc from .relevance import token_overlap_relevance as _compute_relevance @@ -227,30 +224,16 @@ def search_youtube( "--no-download", ] - preexec = os.setsid if hasattr(os, 'setsid') else None - try: - proc = subprocess.Popen( - cmd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - preexec_fn=preexec, - ) - try: - stdout, stderr = proc.communicate(timeout=120) - except subprocess.TimeoutExpired: - try: - os.killpg(os.getpgid(proc.pid), signal.SIGTERM) - except (ProcessLookupError, PermissionError, OSError): - proc.kill() - proc.wait(timeout=5) - _log("YouTube search timed out (120s)") - return {"items": [], "error": "Search timed out"} + result = subproc.run_with_timeout(cmd, timeout=120) + except subproc.SubprocTimeout: + _log("YouTube search timed out (120s)") + return {"items": [], "error": "Search timed out"} except FileNotFoundError: return {"items": [], "error": "yt-dlp not found"} - if not (stdout or "").strip(): + stdout = result.stdout + if not stdout.strip(): _log("YouTube search returned 0 results") return {"items": []} @@ -452,25 +435,10 @@ def _fetch_transcript_ytdlp(video_id: str, temp_dir: str) -> Optional[str]: f"https://www.youtube.com/watch?v={video_id}", ] - preexec = os.setsid if hasattr(os, 'setsid') else None - try: - proc = subprocess.Popen( - cmd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - preexec_fn=preexec, - ) - try: - proc.communicate(timeout=30) - except subprocess.TimeoutExpired: - try: - os.killpg(os.getpgid(proc.pid), signal.SIGTERM) - except (ProcessLookupError, PermissionError, OSError): - proc.kill() - proc.wait(timeout=5) - return None + subproc.run_with_timeout(cmd, timeout=30) + except subproc.SubprocTimeout: + return None except FileNotFoundError: return None @@ -556,7 +524,7 @@ def fetch_transcripts_parallel( vid = futures[future] try: results[vid] = future.result() - except (OSError, subprocess.SubprocessError) as exc: + except OSError as exc: _log(f"Transcript fetch error for {vid}: {exc}") results[vid] = None except Exception as exc: diff --git a/tests/test_env_v3.py b/tests/test_env_v3.py index c5a5b86..a79ec69 100644 --- a/tests/test_env_v3.py +++ b/tests/test_env_v3.py @@ -30,8 +30,11 @@ class EnvV3Tests(unittest.TestCase): self.assertEqual("b", bird_x._credentials["CT0"]) def test_bird_auth_never_checks_browser_cookies(self): + # The guarantee: is_bird_authenticated() must not spawn any child + # process to probe for cookies. All subprocess paths in bird_x go + # through subproc.run_with_timeout, so patching that covers it. with mock.patch("lib.bird_x.is_bird_installed", return_value=True), mock.patch( - "lib.bird_x.subprocess.run", + "lib.bird_x.subproc.run_with_timeout", side_effect=AssertionError("browser-cookie whoami should not run"), ): bird_x._credentials.clear() diff --git a/tests/test_subproc.py b/tests/test_subproc.py new file mode 100644 index 0000000..e9f939d --- /dev/null +++ b/tests/test_subproc.py @@ -0,0 +1,102 @@ +"""Tests for scripts/lib/subproc.py. + +Covers the process-group cleanup path, timeout behavior, success path, +PID callback wiring, and environment inheritance. +""" + +import sys +import unittest +from pathlib import Path +from unittest.mock import patch + +sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "scripts")) + +from lib import subproc + + +class TestRunWithTimeout(unittest.TestCase): + def test_success_returns_stdout(self): + result = subproc.run_with_timeout( + ["sh", "-c", "echo hello"], + timeout=5, + ) + self.assertEqual(result.returncode, 0) + self.assertEqual(result.stdout.strip(), "hello") + self.assertEqual(result.stderr, "") + + def test_nonzero_exit_returns_returncode_not_exception(self): + result = subproc.run_with_timeout( + ["sh", "-c", "exit 3"], + timeout=5, + ) + self.assertEqual(result.returncode, 3) + + def test_captures_stderr(self): + result = subproc.run_with_timeout( + ["sh", "-c", "echo err >&2"], + timeout=5, + ) + self.assertEqual(result.stderr.strip(), "err") + + def test_timeout_raises_subproctimeout(self): + with self.assertRaises(subproc.SubprocTimeout): + subproc.run_with_timeout( + ["sh", "-c", "sleep 10"], + timeout=1, + ) + + def test_timeout_kills_process_group(self): + """A slow child inside a shell should be killed when the group is signaled.""" + with self.assertRaises(subproc.SubprocTimeout): + # Parent shell spawns a child that sleeps long. + # Without process-group cleanup, the child would orphan. + subproc.run_with_timeout( + ["sh", "-c", "sleep 10 & wait"], + timeout=1, + ) + + def test_missing_command_raises_oserror(self): + """Missing executables raise FileNotFoundError (or PermissionError on + some filesystems if a same-named junk file exists).""" + with self.assertRaises(OSError): + subproc.run_with_timeout( + ["/nonexistent-path/last30days-test-no-such-bin"], + timeout=5, + ) + + def test_env_is_passed_through(self): + result = subproc.run_with_timeout( + ["sh", "-c", "echo $LAST30DAYS_TEST_VAR"], + timeout=5, + env={"LAST30DAYS_TEST_VAR": "custom_value", "PATH": "/usr/bin:/bin"}, + ) + self.assertEqual(result.stdout.strip(), "custom_value") + + def test_on_pid_callback_receives_pid(self): + seen_pids = [] + subproc.run_with_timeout( + ["sh", "-c", "true"], + timeout=5, + on_pid=lambda pid: seen_pids.append(pid), + ) + self.assertEqual(len(seen_pids), 1) + self.assertIsInstance(seen_pids[0], int) + self.assertGreater(seen_pids[0], 0) + + def test_on_pid_callback_exceptions_are_suppressed(self): + """If the PID callback raises, the subprocess should still run to completion.""" + def raising_callback(pid): + raise RuntimeError("boom") + + # Should not raise, callback exception is swallowed. + result = subproc.run_with_timeout( + ["sh", "-c", "echo ok"], + timeout=5, + on_pid=raising_callback, + ) + self.assertEqual(result.returncode, 0) + self.assertEqual(result.stdout.strip(), "ok") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_youtube_yt.py b/tests/test_youtube_yt.py index a68ab86..6128d6a 100644 --- a/tests/test_youtube_yt.py +++ b/tests/test_youtube_yt.py @@ -63,24 +63,26 @@ class TestYouTubeEngagementZero(unittest.TestCase): class TestYtDlpFlags(unittest.TestCase): + def _fake_result(self, stdout: str = "", returncode: int = 0): + from lib.subproc import SubprocResult + return SubprocResult(returncode=returncode, stdout=stdout, stderr="") + def test_search_ignores_global_config_and_browser_cookies(self): - proc = _DummyProc() with mock.patch.object(youtube_yt, "is_ytdlp_installed", return_value=True), \ - mock.patch.object(youtube_yt.subprocess, "Popen", return_value=proc) as popen_mock: + mock.patch.object(youtube_yt.subproc, "run_with_timeout", return_value=self._fake_result()) as run_mock: youtube_yt.search_youtube("Claude Code", "2026-02-01", "2026-03-01") - cmd = popen_mock.call_args.args[0] + cmd = run_mock.call_args.args[0] self.assertIn("--ignore-config", cmd) self.assertIn("--no-cookies-from-browser", cmd) def test_transcript_fetch_ignores_global_config_and_browser_cookies(self): - proc = _DummyProc() with tempfile.TemporaryDirectory() as temp_dir, \ mock.patch.object(youtube_yt, "is_ytdlp_installed", return_value=True), \ - mock.patch.object(youtube_yt.subprocess, "Popen", return_value=proc) as popen_mock: + mock.patch.object(youtube_yt.subproc, "run_with_timeout", return_value=self._fake_result()) as run_mock: youtube_yt.fetch_transcript("abc123", temp_dir) - cmd = popen_mock.call_args.args[0] + cmd = run_mock.call_args.args[0] self.assertIn("--ignore-config", cmd) self.assertIn("--no-cookies-from-browser", cmd)