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

@@ -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
 

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 "",
+    )

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:

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()

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()

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)