fix: flush Python inject shards from fork workers - #289
Conversation
463082a to
da24a3c
Compare
Independent review + verificationAn independent reviewer went over this branch, and I re-verified the load-bearing claims myself. Marking clearly which is which, since two of my own first attempts at verification used a broken harness. 1.
|
Update: the SIGTERM fix was wrong, and is replacedThe previous push ( A Python signal handler only runs when the interpreter reaches a bytecode boundary. A worker inside a long C call — BLAS, zlib, pickle — latches the signal and never dies, and both What replaced itNo signal handling at all. The fork child writes its shard eagerly, right after forking, and the existing
This covers strictly more than the handler did: SIGKILL and The boundary, stated honestlyImports a worker makes after forking are lost if it is killed before exiting. Closing that needs incremental journaling, not an exit hook. Coverage for a worker killed while still bootstrapping is best-effort — I measured runs where a forked worker died before its after-fork hook ran. The pool test therefore asserts against workers that actually ran a task, rather than an exact shard count, which would be flaky. Verification
Also from the re-review, not addressed here and worth their own consideration: worker teardown cost ( 🤖 Generated with Claude Code |
8fe3308 to
0c5de2f
Compare
Review found the finalizer covered only half the teardown paths.
`Pool.__exit__` is `terminate()`, which SIGTERMs every worker, and
`util._exit_function` does the same to surviving daemon children --
which is what DataLoader creates. SIGTERM's default disposition kills
the process outright, so neither the Finalize callback nor atexit runs.
Measured under real injection, 4 workers:
with ctx.Pool(4) as p: p.map(...) 1 shard (0 of 4 workers)
p.close(); p.join() 5 shards (4 of 4)
So the common idiom -- and the `num_proc` case the finalizer's own
docstring cites -- reported nothing at all. It now reports 4 of 4, and a
daemon child is captured too.
The handler is installed only in a fork child, and only where nothing
else owns the signal, so a workload's own SIGTERM handling is never
displaced. It restores the default disposition and re-raises, so the
process still dies of SIGTERM and still reports exit status -15. One of
our own handlers may be superseded, so a re-install or a second tracker
does not leave a stale one writing the wrong shard.
`signal` is imported in the parent so the child's import is a
sys.modules hit; importing for the first time inside a fork child can
deadlock on the import lock.
Two tests, each verified to fail against the code without its fix:
- the wiring test goes through `install()` rather than the private
method. Deleting the single line that wires this into `install()`
previously left all 125 tests passing -- and that line sits in the
merge-conflict region with #287, so a conflict resolution could drop
it silently.
- the Pool test pins the terminated case.
A third asserts a workload-owned SIGTERM handler is not displaced.
The documented boundary now matches the code: termination by signal is
covered; `os._exit` and SIGKILL are not, and cannot be without
incremental import journaling.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the SIGTERM handler from the previous commit, which was worse
than the bug it fixed.
A Python signal handler only runs when the interpreter reaches a bytecode
boundary. A worker inside a long C call -- BLAS, zlib, pickle -- latches
the signal and never dies, and both `Pool._terminate_pool` and
`util._exit_function` join workers with no timeout. So the handler could
hang the workload indefinitely:
worker in zlib.compress, on terminate()
without roar exit -15, joined in 0.02s
with the handler still alive after 8s, needed SIGKILL
A missing shard is a thin record. A hung pipeline is a stopped campaign.
Instead the fork child writes its shard eagerly, right after forking, and
keeps the existing Finalize to rewrite the same per-PID path on an orderly
exit. A worker that is killed -- SIGTERM, SIGKILL -- or that calls
os._exit still contributes the state it inherited at fork, rather than
nothing. An orderly worker upgrades that with whatever it imported while
running. merge_inject_logs unions both, so the upgrade is free.
This also covers strictly more than the handler did: SIGKILL and os._exit
were previously uncoverable, and both now leave a valid shard.
The boundary, stated honestly and pinned by a test: imports a worker makes
*after* forking are lost if it is killed before exiting. Closing that
needs incremental journaling, not an exit hook. Coverage for a worker
killed while still bootstrapping is best-effort, so the pool test asserts
against workers that actually ran a task rather than an exact shard count.
Verified: hang gone; `with Pool(...)`, close()/join(), bare Process and
daemon children all report; 127 runtime tests pass; removing the install()
wiring, the eager write, or the Finalize each fails a test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3febfd2 to
c6c9d14
Compare
3 participants
Problem
PR #265 made Python inject reports per-PID and merged those shards after tracing, but a plain Linux
multiprocessingfork worker never produced a shard: multiprocessing terminates workers throughos._exit, bypassing Pythonatexit, whereRuntimeInjectionTracker.write_logis registered.Consequently a package imported only inside a fork worker—and native libraries loaded only by that import—was absent from lineage. Parent imports and native file tracing could mask the gap in typical framework workloads.
Fix
Register an after-fork callback with
multiprocessing.util. Inside each fork child, that callback creates a child-localmultiprocessing.util.Finalizewhich writes the existing PID-specific inject shard. Multiprocessing executes these finalizers during orderly worker shutdown beforeos._exit; PR #265's existing merger then consumes the report without changes.Best-effort registration preserves tracing if multiprocessing hooks are unavailable.
Scope boundary
This repairs orderly multiprocessing fork shutdown. It does not claim crash safety for direct
os._exit, SIGKILL, fatal signals, or host loss. A regression test explicitly documents thatos._exitstill produces no shard; closing that class requires incremental import/native-load journaling plus parent-side completeness detection.Tests
forkworker, not a fabricated shard:inject-log.<actual-child-pid>;os._exitdocuments the remaining boundary.Composition with #287
Validated locally with this PR composed with #287 using isolated
uv toolRoar (Python 3.14) and a separate workload venv (Python 3.12):import numpy-> exactlynumpy==2.5.2;blake3absent; 4 dpkg native dependencies retained; no unmanaged.sowarning.import blake3-> exactlyblake3==1.0.9; 2 dpkg native dependencies retained; no unmanaged.sowarning.This PR intentionally remains independent of #287: it guarantees report production; #287 attributes package/native contents within reports.