Skip to content

refactor: extract subprocess cleanup into shared subproc helper#210

Open
iliaal wants to merge 1 commit intomvanhorn:mainfrom
iliaal:refactor/subprocess-helper
Open

refactor: extract subprocess cleanup into shared subproc helper#210
iliaal wants to merge 1 commit intomvanhorn:mainfrom
iliaal:refactor/subprocess-helper

Conversation

@iliaal
Copy link
Copy Markdown
Contributor

@iliaal iliaal commented Apr 10, 2026

Summary

bird_x.py and youtube_yt.py had four near-identical copies of the same subprocess cleanup dance. Each callsite did the same thing: spawn in a new process group via os.setsid, call communicate(timeout=N), catch TimeoutExpired, signal SIGTERM to the group via killpg, fall back to proc.kill() on signal errors, wait up to 5 seconds, then return or log an error.

I extracted it to scripts/lib/subproc.run_with_timeout():

  • 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, raise
  • Accepts an on_pid callback so bird_x.py can still register child PIDs with last30days.register_child_pid for whole-process cleanup tracking
  • Captures stdout and stderr as strings in a SubprocResult dataclass

With the shared helper in place, import signal and import subprocess became dead in both source files (plus import os in youtube_yt.py) and went with them.

Call sites migrated

  • bird_x._run_bird_search() (Bird Node.js CLI)
  • bird_x.search_handles() inner worker (per-handle Bird call)
  • youtube_yt.search_youtube() (yt-dlp search)
  • youtube_yt.fetch_transcript() (yt-dlp per-video transcript download)

Diff summary

  • scripts/lib/subproc.py: +94 new file
  • scripts/lib/bird_x.py: -51 net
  • scripts/lib/youtube_yt.py: -34 net
  • tests/test_subproc.py: +100 new file, 9 tests covering success, non-zero exit, stderr capture, timeout-raises, timeout-kills-group, missing-command, env passthrough, PID callback, callback-exception-suppression
  • tests/test_env_v3.py: +3 / -1 (patch subproc.run_with_timeout instead of the removed bird_x.subprocess)
  • tests/test_youtube_yt.py: +8 / -6 (same adjustment for yt-dlp flag assertions)

Test plan

  • 9 new subproc tests pass (full branch coverage of the helper)
  • All 7 bird_x tests pass
  • All 30 youtube_yt tests pass
  • Full suite: 1031 passed, 15 pre-existing failures (unchanged)
  • Live smoke test: X search returned 16 posts via Bird
  • Live smoke test: YouTube search found 5 videos for "React 19 tutorial" and fetched transcripts for all 5
  • I verified the timeout path: the first YouTube query stalled past 120s, SubprocTimeout fired, the retry with a different query succeeded.

bird_x.py and youtube_yt.py had four near-identical copies of the
os.setsid + Popen + communicate(timeout) + TimeoutExpired + killpg
cleanup dance. Extracted to scripts/lib/subproc.run_with_timeout().

The new helper:
- Runs the child in its own process group via os.setsid where available
- Raises SubprocTimeout on timeout (clean exception type, not subprocess.TimeoutExpired)
- Signals SIGTERM to the entire group on timeout, falls back to proc.kill()
- Always waits up to 5s for cleanup before returning
- Supports an on_pid callback so bird_x can still register child PIDs
  with last30days.register_child_pid for whole-process cleanup tracking

Net: bird_x.py -51, youtube_yt.py -34, with +94 in subproc.py + 9 new
tests for the helper (success path, timeouts, env passthrough, PID
callback, callback exceptions, missing commands, stderr capture).

Migrated 4 call sites: _run_bird_search, search_handles inner, YouTube
search, and YouTube transcript fetch. Two existing tests needed minor
updates to patch subproc.run_with_timeout instead of subprocess.Popen;
the test intent is preserved.

1031 tests pass (+9 from new subproc tests, 15 pre-existing failures
unchanged). Live smoke test verified: X search returned 16 posts via
Bird, YouTube search found 5 videos and fetched transcripts for all of
them, both through the new helper.
@iliaal iliaal force-pushed the refactor/subprocess-helper branch from 5069382 to cbc1291 Compare April 14, 2026 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant