refactor: extract subprocess cleanup into shared subproc helper#210
Open
iliaal wants to merge 1 commit intomvanhorn:mainfrom
Open
refactor: extract subprocess cleanup into shared subproc helper#210iliaal wants to merge 1 commit intomvanhorn:mainfrom
iliaal wants to merge 1 commit intomvanhorn:mainfrom
Conversation
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.
5069382 to
cbc1291
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bird_x.pyandyoutube_yt.pyhad four near-identical copies of the same subprocess cleanup dance. Each callsite did the same thing: spawn in a new process group viaos.setsid, callcommunicate(timeout=N), catchTimeoutExpired, signalSIGTERMto the group viakillpg, fall back toproc.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():os.setsidwhere availableSubprocTimeouton timeoutSIGTERMthe group, fall back toproc.kill(), wait up to 5s, raiseon_pidcallback sobird_x.pycan still register child PIDs withlast30days.register_child_pidfor whole-process cleanup trackingSubprocResultdataclassWith the shared helper in place,
import signalandimport subprocessbecame dead in both source files (plusimport osinyoutube_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 filescripts/lib/bird_x.py: -51 netscripts/lib/youtube_yt.py: -34 nettests/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-suppressiontests/test_env_v3.py: +3 / -1 (patchsubproc.run_with_timeoutinstead of the removedbird_x.subprocess)tests/test_youtube_yt.py: +8 / -6 (same adjustment for yt-dlp flag assertions)Test plan
subproctests pass (full branch coverage of the helper)SubprocTimeoutfired, the retry with a different query succeeded.