Fix: exit a forked worker child once its parent is gone - #1495
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughForked worker mailbox handling is centralized with parent-death detection and shutdown cleanup. A Linux regression test verifies child exit after parent termination. New documentation describes host-build graph execution contracts, storage, lifecycle, fallback behavior, and roadmap constraints. ChangesWorker orphan reaping
Graph execution documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
python/simpler/worker.py (2)
1862-1868: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNested worker teardown only runs on the clean SHUTDOWN/orphan path.
Unlike
_sub_worker_loopand_run_chip_main_loop, there is nofinallyhere, so an exception escaping the loop leavesinner_workerun-closed in the forked child. Guarding withfinallyplus an idempotentclose()makes teardown unconditional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/simpler/worker.py` around lines 1862 - 1868, Wrap the `_run_mailbox_loop` invocation in the nested worker path with a `finally` block that always calls `inner_worker.close()`, including when the loop raises an exception. Ensure `close()` remains safe if it is also invoked during normal shutdown, matching the teardown behavior of `_sub_worker_loop` and `_run_chip_main_loop`.
1650-1654: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
_CTRL_OP_NAMEShere.The chip control error path hand-rolls the register/unregister naming that the new module-level map already provides, so the two can drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/simpler/worker.py` around lines 1650 - 1654, Update the chip control error path to derive the operation name from the existing _CTRL_OP_NAMES map instead of manually selecting “register” or “unregister” based on sub_cmd. Preserve the current formatted hash, device, and exception details in the _format_exc message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/simpler/worker.py`:
- Line 1534: In the comment near the N×40B expression, replace the ambiguous
multiplication symbol “×” with the plain ASCII “x” while preserving the
comment’s meaning.
- Around line 1054-1055: Capture the original parent PID before the fork and
pass it into the child worker initialization instead of sampling it with
os.getppid() after fork. Update the worker setup around parent_pid and
liveness_countdown to use this inherited PID, while preserving the existing
liveness polling behavior.
In `@src/a2a3/runtime/host_build_graph/docs/GRAPH_EXECUTION.md`:
- Line 130: Update the alias-partition mismatch behavior in the GRAPH EXECUTION
documentation to reject cached-graph replay and fall back to the ordinary
execution path, replacing the current LOG_WARN-and-proceed behavior. Keep the
existing recomputation and comparison of rep[i] unchanged, and document the
mismatch as structural metadata drift rather than a recoverable warning.
- Around line 285-292: Update the cache-hit section describing rt_submit_graph
to state that cache misses execute fn and record its graph, while cache hits
replay the recorded definition without rerunning the host callable. Explicitly
document that fn must be deterministic and side-effect-free except for graph
construction; otherwise host-side effects and state reads occur only on the
first invocation.
In `@tests/ut/py/test_worker/test_orphan_child_reaping.py`:
- Around line 69-97: Update the cleanup in the test’s finally block to also
terminate and reap the parent subprocess when it remains running, including when
parent.wait raises TimeoutExpired. Preserve the existing reported-child cleanup,
and guard parent cleanup so it is safe if startup or waiting fails before parent
is assigned.
---
Nitpick comments:
In `@python/simpler/worker.py`:
- Around line 1862-1868: Wrap the `_run_mailbox_loop` invocation in the nested
worker path with a `finally` block that always calls `inner_worker.close()`,
including when the loop raises an exception. Ensure `close()` remains safe if it
is also invoked during normal shutdown, matching the teardown behavior of
`_sub_worker_loop` and `_run_chip_main_loop`.
- Around line 1650-1654: Update the chip control error path to derive the
operation name from the existing _CTRL_OP_NAMES map instead of manually
selecting “register” or “unregister” based on sub_cmd. Preserve the current
formatted hash, device, and exception details in the _format_exc message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5acb7c40-4c7a-47c9-8602-d1a3ddfa39a5
📒 Files selected for processing (3)
python/simpler/worker.pysrc/a2a3/runtime/host_build_graph/docs/GRAPH_EXECUTION.mdtests/ut/py/test_worker/test_orphan_child_reaping.py
| parent_pid = os.getppid() | ||
| liveness_countdown = _PARENT_LIVENESS_POLL_INTERVAL |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Parent PID sampled after fork leaves a small orphan window.
If the parent dies between fork() and this first os.getppid(), the sampled parent_pid is already the reparenting init/subreaper, so it never changes and the child spins forever — precisely the case #1493 targets. Capturing the parent's PID before the fork and passing it in closes the window.
🛡️ Sketch
def _run_mailbox_loop(
buf,
state_addr: int,
*,
handle_task,
handle_control,
on_shutdown=None,
+ parent_pid: int | None = None,
) -> None:
@@
- parent_pid = os.getppid()
+ # Prefer the pid captured by the forking parent; falling back to
+ # getppid() here can already observe the reparented ancestor.
+ parent_pid = parent_pid if parent_pid is not None else os.getppid()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/simpler/worker.py` around lines 1054 - 1055, Capture the original
parent PID before the fork and pass it into the child worker initialization
instead of sampling it with os.getppid() after fork. Update the worker setup
around parent_pid and liveness_countdown to use this inherited PID, while
preserving the existing liveness polling behavior.
| _rewrite_blob_host_addrs(buf, _OFF_TASK_ARGS_BLOB, host_buf_ranges) | ||
| # Hand the mailbox bytes straight to C++ (zero-copy zero-decode): | ||
| # the blob layout is what `write_blob` already wrote, so re-parsing | ||
| # it in Python is N×40B of avoidable work and a permanent |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff RUF003: ambiguous × in comment.
Flagged by static analysis on this changed line; replace with x to keep the lint gate clean.
✏️ Fix
- # it in Python is N×40B of avoidable work and a permanent
+ # it in Python is Nx40B of avoidable work and a permanent📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # it in Python is N×40B of avoidable work and a permanent | |
| # it in Python is Nx40B of avoidable work and a permanent |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 1534-1534: Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/simpler/worker.py` at line 1534, In the comment near the N×40B
expression, replace the ambiguous multiplication symbol “×” with the plain ASCII
“x” while preserving the comment’s meaning.
Source: Linters/SAST tools
| | Contract | Detection | Action on mismatch | Why | | ||
| | -------- | --------- | ------------------ | --- | | ||
| | **Structural metadata** — `shapes[]`, `strides[]`, `ndims`, `dtype`, size, `tag`, `manual_dep`, `is_contiguous` | field compare against the recorded signature | **`LOG_WARN` and fall back to the ordinary submit path** for this call (the cache is not used) | The recording baked `total_output_size`, `node_offsets[]`, and `required_heap` from these values. Replaying with a different shape would slice a block sized for the old one — a memory-safety fault, not a bounded wrong value. | | ||
| | **Alias partition** — which boundary slots share a buffer | recompute `rep[i] = min{ j<=i : args[j] shares args[i]'s buffer }` and compare | **`LOG_WARN` and proceed with the cached graph** | An internal tensor is matched to its boundary slot by address; when two slots alias, first-match resolves it by lowest index, valid only for the recorded alias pattern. The blast radius is bounded (a mis-bound pointer, not a wrong size) and the case is rare. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not replay the cached graph after an alias-partition mismatch.
When boundary slots alias differently from the recording, the documented first-match lookup can bind an internal tensor to the wrong boundary slot and therefore the wrong caller-owned buffer. This is not merely a bounded warning; it can produce incorrect results or overwrite another tensor. Treat this mismatch like structural metadata drift and fall back to the ordinary path.
Proposed documentation fix
- | **Alias partition** — which boundary slots share a buffer | recompute `rep[i] = min{ j<=i : args[j] shares args[i]'s buffer }` and compare | **`LOG_WARN` and proceed with the cached graph** |
+ | **Alias partition** — which boundary slots share a buffer | recompute `rep[i] = min{ j<=i : args[j] shares args[i]'s buffer }` and compare | **`LOG_WARN` and fall back to the ordinary submit path** |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | **Alias partition** — which boundary slots share a buffer | recompute `rep[i] = min{ j<=i : args[j] shares args[i]'s buffer }` and compare | **`LOG_WARN` and proceed with the cached graph** | An internal tensor is matched to its boundary slot by address; when two slots alias, first-match resolves it by lowest index, valid only for the recorded alias pattern. The blast radius is bounded (a mis-bound pointer, not a wrong size) and the case is rare. | | |
| | **Alias partition** — which boundary slots share a buffer | recompute `rep[i] = min{ j<=i : args[j] shares args[i]'s buffer }` and compare | **`LOG_WARN` and fall back to the ordinary submit path** | An internal tensor is matched to its boundary slot by address; when two slots alias, first-match resolves it by lowest index, valid only for the recorded alias pattern. The blast radius is bounded (a mis-bound pointer, not a wrong size) and the case is rare. | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/a2a3/runtime/host_build_graph/docs/GRAPH_EXECUTION.md` at line 130,
Update the alias-partition mismatch behavior in the GRAPH EXECUTION
documentation to reject cached-graph replay and fall back to the ordinary
execution path, replacing the current LOG_WARN-and-proceed behavior. Keep the
existing recomputation and comparison of rep[i] unchanged, and document the
mismatch as structural metadata drift rather than a recoverable warning.
| **Record (cache miss).** `rt_submit_graph` runs the function normally; every | ||
| submitted task is captured *and* executes as usual, so a miss costs nothing | ||
| beyond the recording. At scope end the recording is compacted into a definition. | ||
|
|
||
| **Submit (cache hit).** The Orchestrator allocates `required_heap`, computes only | ||
| the Graph's *external* dependencies, wires the boundary exactly like an ordinary | ||
| task, and places one `GRAPH` descriptor in the task window. The upload carries | ||
| the compact definition plus this call's boundary args — no expanded node array. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Define the orchestration callable’s cache-hit contract.
On a cache miss, rt_submit_graph executes fn; on a cache hit, it replays the recorded definition instead of rerunning the host body. Unless fn is explicitly required to be deterministic and side-effect-free apart from graph construction, host-side effects and state reads will occur only on the first invocation.
Proposed documentation addition
Record (cache miss). `rt_submit_graph` runs the function normally; every
submitted task is captured and executes as usual, so a miss costs nothing
beyond the recording. At scope end the recording is compacted into a definition.
Submit (cache hit). The Orchestrator allocates `required_heap`, computes only
the Graph's **external** dependencies, wires the boundary exactly like an ordinary
task, and places one `GRAPH` descriptor in the task window.+ The orchestration callable must be deterministic and side-effect-free except
+ for constructing and submitting graph tasks. Per-invocation values must arrive
+ through `L0TaskArgs` or the configuration channel; the callable is not rerun
+ on cache hits.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Record (cache miss).** `rt_submit_graph` runs the function normally; every | |
| submitted task is captured *and* executes as usual, so a miss costs nothing | |
| beyond the recording. At scope end the recording is compacted into a definition. | |
| **Submit (cache hit).** The Orchestrator allocates `required_heap`, computes only | |
| the Graph's *external* dependencies, wires the boundary exactly like an ordinary | |
| task, and places one `GRAPH` descriptor in the task window. The upload carries | |
| the compact definition plus this call's boundary args — no expanded node array. | |
| **Record (cache miss).** `rt_submit_graph` runs the function normally; every | |
| submitted task is captured *and* executes as usual, so a miss costs nothing | |
| beyond the recording. At scope end the recording is compacted into a definition. | |
| **Submit (cache hit).** The Orchestrator allocates `required_heap`, computes only | |
| the Graph's *external* dependencies, wires the boundary exactly like an ordinary | |
| task, and places one `GRAPH` descriptor in the task window. The upload carries | |
| the compact definition plus this call's boundary args — no expanded node array. | |
| The orchestration callable must be deterministic and side-effect-free except | |
| for constructing and submitting graph tasks. Per-invocation values must arrive | |
| through `L0TaskArgs` or the configuration channel; the callable is not rerun | |
| on cache hits. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/a2a3/runtime/host_build_graph/docs/GRAPH_EXECUTION.md` around lines 285 -
292, Update the cache-hit section describing rt_submit_graph to state that cache
misses execute fn and record its graph, while cache hits replay the recorded
definition without rerunning the host callable. Explicitly document that fn must
be deterministic and side-effect-free except for graph construction; otherwise
host-side effects and state reads occur only on the first invocation.
| try: | ||
| with out_path.open("w") as out: | ||
| parent = subprocess.Popen([sys.executable, "-c", _PARENT_SRC % NUM_SUB_WORKERS], stdout=out, stderr=out) | ||
| parent.wait(timeout=PARENT_EXIT_TIMEOUT_S) | ||
|
|
||
| reported = [int(tok) for tok in out_path.read_text().split() if tok.isdigit()] | ||
| assert len(reported) >= NUM_SUB_WORKERS, ( | ||
| f"expected at least {NUM_SUB_WORKERS} forked children, parent reported {reported}\n" | ||
| f"parent output:\n{out_path.read_text()}" | ||
| ) | ||
|
|
||
| deadline = time.monotonic() + REAP_TIMEOUT_S | ||
| survivors = reported | ||
| while survivors and time.monotonic() < deadline: | ||
| time.sleep(0.1) | ||
| survivors = [p for p in survivors if _alive(p)] | ||
|
|
||
| assert not survivors, ( | ||
| f"{len(survivors)} of {len(reported)} children outlived their SIGKILLed parent by " | ||
| f"{REAP_TIMEOUT_S:.0f}s: {survivors} — a child is not noticing that its parent is gone" | ||
| ) | ||
| finally: | ||
| # A regression leaves spinning processes behind; never hand them on to | ||
| # the next test, whether the assertion passed, failed, or never ran. | ||
| for pid in reported: | ||
| try: | ||
| os.kill(pid, 9) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Timed-out parent is never cleaned up.
If parent.wait() raises TimeoutExpired, reported is still empty, so the finally kills nothing and both the parent subprocess and its forked children leak into the rest of the session. Kill the parent in the finally as well.
🧹 Suggested cleanup
out_path = tmp_path / "children.txt"
reported: list[int] = []
+ parent = None
try:
with out_path.open("w") as out:
parent = subprocess.Popen([sys.executable, "-c", _PARENT_SRC % NUM_SUB_WORKERS], stdout=out, stderr=out)
parent.wait(timeout=PARENT_EXIT_TIMEOUT_S)
@@
finally:
# A regression leaves spinning processes behind; never hand them on to
# the next test, whether the assertion passed, failed, or never ran.
+ if parent is not None and parent.poll() is None:
+ parent.kill()
+ parent.wait(timeout=5)
for pid in reported:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| with out_path.open("w") as out: | |
| parent = subprocess.Popen([sys.executable, "-c", _PARENT_SRC % NUM_SUB_WORKERS], stdout=out, stderr=out) | |
| parent.wait(timeout=PARENT_EXIT_TIMEOUT_S) | |
| reported = [int(tok) for tok in out_path.read_text().split() if tok.isdigit()] | |
| assert len(reported) >= NUM_SUB_WORKERS, ( | |
| f"expected at least {NUM_SUB_WORKERS} forked children, parent reported {reported}\n" | |
| f"parent output:\n{out_path.read_text()}" | |
| ) | |
| deadline = time.monotonic() + REAP_TIMEOUT_S | |
| survivors = reported | |
| while survivors and time.monotonic() < deadline: | |
| time.sleep(0.1) | |
| survivors = [p for p in survivors if _alive(p)] | |
| assert not survivors, ( | |
| f"{len(survivors)} of {len(reported)} children outlived their SIGKILLed parent by " | |
| f"{REAP_TIMEOUT_S:.0f}s: {survivors} — a child is not noticing that its parent is gone" | |
| ) | |
| finally: | |
| # A regression leaves spinning processes behind; never hand them on to | |
| # the next test, whether the assertion passed, failed, or never ran. | |
| for pid in reported: | |
| try: | |
| os.kill(pid, 9) | |
| except OSError: | |
| pass | |
| out_path = tmp_path / "children.txt" | |
| reported: list[int] = [] | |
| parent = None | |
| try: | |
| with out_path.open("w") as out: | |
| parent = subprocess.Popen([sys.executable, "-c", _PARENT_SRC % NUM_SUB_WORKERS], stdout=out, stderr=out) | |
| parent.wait(timeout=PARENT_EXIT_TIMEOUT_S) | |
| reported = [int(tok) for tok in out_path.read_text().split() if tok.isdigit()] | |
| assert len(reported) >= NUM_SUB_WORKERS, ( | |
| f"expected at least {NUM_SUB_WORKERS} forked children, parent reported {reported}\n" | |
| f"parent output:\n{out_path.read_text()}" | |
| ) | |
| deadline = time.monotonic() + REAP_TIMEOUT_S | |
| survivors = reported | |
| while survivors and time.monotonic() < deadline: | |
| time.sleep(0.1) | |
| survivors = [p for p in survivors if _alive(p)] | |
| assert not survivors, ( | |
| f"{len(survivors)} of {len(reported)} children outlived their SIGKILLed parent by " | |
| f"{REAP_TIMEOUT_S:.0f}s: {survivors} — a child is not noticing that its parent is gone" | |
| ) | |
| finally: | |
| # A regression leaves spinning processes behind; never hand them on to | |
| # the next test, whether the assertion passed, failed, or never ran. | |
| if parent is not None and parent.poll() is None: | |
| parent.kill() | |
| parent.wait(timeout=5) | |
| for pid in reported: | |
| try: | |
| os.kill(pid, 9) | |
| except OSError: | |
| pass |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 70-70: Command coming from incoming request
Context: subprocess.Popen([sys.executable, "-c", _PARENT_SRC % NUM_SUB_WORKERS], stdout=out, stderr=out)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.21)
[error] 71-71: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/ut/py/test_worker/test_orphan_child_reaping.py` around lines 69 - 97,
Update the cleanup in the test’s finally block to also terminate and reap the
parent subprocess when it remains running, including when parent.wait raises
TimeoutExpired. Preserve the existing reported-child cleanup, and guard parent
cleanup so it is safe if startup or waiting fails before parent is assigned.
de0bffe to
848cb87
Compare
|
CI caught a defect in the regression test itself — the fix under test is unaffected: Two problems, both from enumerating children with
Fixed by taking the pids from the Worker's own With Re-verified red-to-green against the parent branch's
|
ae21520 to
fe35353
Compare
|
Rebased onto That file should never have been in #1494. It is documentation from the open #1444 ( Also hardened the regression test after it failed once in ~22 local runs with an unhelpful Verification after the rebase:
|
A forked sub/child/chip worker never checked whether its parent was still alive. When the parent died without `Worker.close()` — SIGKILL from a `timeout`, an OOM kill, a cancelled CI job, an editor killing a wedged pytest — every child was reparented and kept polling a mailbox nobody would ever write to. Because that poll is an unbounded busy-wait, each survivor held a full core indefinitely. This is not theoretical. On a shared dev box six such processes belonging to two different users had been alive 5-7 days each at ~99.6% CPU, six cores permanently consumed. Four of the six carried `tests/ut/py/test_worker/test_startup_readiness.py` on their command line: runs whose parent was killed rather than allowed to finish. `_run_mailbox_loop` now samples `getppid()` on its idle path and leaves by the same SHUTDOWN route once it changes, so a child tears down exactly as it would on a clean shutdown. Compare against the pid captured at loop entry rather than testing for pid 1: a subreaper (container init, a systemd user session) adopts orphans instead of init, so the pid changes but never becomes 1. A live parent's pid cannot change, so the check cannot fire spuriously. Sampling every `_PARENT_LIVENESS_POLL_INTERVAL` idle polls puts a `getppid()` roughly every 100 us against a ~0.1 us poll — fast enough that an orphan goes away before it is noticeable, cheap enough to be lost in the noise of the poll itself. Residual gap: a parent that dies between `os.fork()` and the child reaching the loop is already gone when the pid is captured, so that child is not detected. Closing it needs the pid handed in from before the fork, or `PR_SET_PDEATHSIG`, which is Linux-only and therefore an optimisation on top of this rather than the mechanism. `test_orphan_child_reaping.py` SIGKILLs a subprocess parent and asserts every child is gone within 20 s. Three details are load-bearing: - It reads the pids from the Worker's own `_sub_pids` rather than `pgrep -P`. Enumerating children externally also catches the transient shell that runs pgrep, and inside a container's shallow pid namespace it picks up unrelated low pids: on CI that produced "1 of 6 children outlived ... [3]", waiting on and then SIGKILLing a container process that was never ours. - It writes to a file and never reads a pipe. Orphans inherit the parent's stdout, so on regression they hold a pipe's write end open and reading one would hang the test for as long as the bug survives instead of failing it. - Its `finally` kills whatever survived, so a regression does not hand spinning processes to the next test. - It asserts the parent died from its own SIGKILL. A parent that fails before forking otherwise surfaces as an unexplained "expected 3 pids, got []", which reads like a defect in the code under test rather than an environment problem in the harness. It runs on macOS as well as Linux: reparenting on parent death is POSIX behaviour, and nothing here is platform-specific once the pid list comes from the Worker. Also drop `src/a2a3/runtime/host_build_graph/docs/GRAPH_EXECUTION.md`, which hw-native-sys#1494 added by accident: it is documentation from the open hw-native-sys#1444 (`feat/graph-execution`) that happened to be sitting untracked in the working tree, and a `git add -A` swept it into that commit. Removing it restores main to the state hw-native-sys#1444 expects to merge into. It is unrelated to everything else here and can be reviewed independently of the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…log (#1504) `test_orphan_child_reaping` scraped the forked children's pids out of the parent subprocess's combined stdout+stderr, taking every digit-only token it found. That stream also carries log lines and warnings, so any bare number in them was read as a pid. On CI it collected a fourth entry: expected exactly 3 forked sub-worker pids, parent reported [6778, 6779, 6780, 3] The three real pids were correct — `Worker._sub_pids` is not at fault — and the run was otherwise healthy, so the test failed on main for a parsing artifact rather than a defect. It also meant the `finally` block would have sent SIGKILL to whatever process that stray number named. Give the parent a dedicated output file, named on its command line, and keep stdout+stderr as a separate log used only for diagnostics when an assertion fails. Parsing a channel that carries nothing but the pids removes the class of failure rather than filtering harder against it, which is the same mistake in a different shape. Also drop the `isdigit()` filter, now that a non-numeric token in the pid file would be a real defect worth surfacing rather than noise to skip. 15/15 local runs pass; red-to-green still holds against the pre-#1495 `worker.py` ("3 of 3 children outlived their SIGKILLed parent by 20s", now with exactly three pids and no phantom entry).
After hw-native-sys#1499 took a single `Worker.run()` from 150 us to ~51 us, the obvious follow-up was whether another factor of three was available in what remained, and whether a blocking wakeup primitive (hw-native-sys#1498) was how to get it. Measured, and the answer to both is no. Recording it so the next person does not re-derive it. A bare fork+shm mailbox round trip with no simpler runtime in it costs 3.5 us against `Worker.run()`'s 53.8 us, which reads as "94% of dispatch latency is our code, not the IPC". That headline is an artifact of benchmarking a single task. Sweeping tasks per `run()` from 1 to 256 splits it into **43.3 us fixed per run() and 8.08 us marginal per task**: the cost is entering and leaving an orchestration, not dispatching work, and a production run submits a graph per `run()` rather than one task. At 256 tasks the fixed part is 2% of the invocation. The same numbers settle hw-native-sys#1498 against itself on latency grounds. A pipe wake measures 4.4 us one way on this box, so replacing the poll with a blocking wait adds more than the entire 3.5 us IPC floor to every dispatch, in exchange for CPU, while the latency it was meant to recover is not in the poll at all. What survives is narrower and unrelated: a child spinning while nothing is outstanding holds a core for as long as its parent lives — a CPU question for narrow-core hosts, already smaller since hw-native-sys#1495 made orphans exit with their parent. Also records the reconsideration triggers, since both conclusions are conditional: a workload that issues many small `run()` calls stops amortizing the fixed cost, and the CPU argument for a spin-then-block hybrid remains open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
After #1499 took a single `Worker.run()` from 150 us to ~51 us, the obvious follow-up was whether another factor of three was available in what remained, and whether a blocking wakeup primitive (#1498) was how to get it. Measured, and the answer to both is no. Recording it so the next person does not re-derive it. A bare fork+shm mailbox round trip with no simpler runtime in it costs 3.5 us against `Worker.run()`'s 53.8 us, which reads as "94% of dispatch latency is our code, not the IPC". That headline is an artifact of benchmarking a single task. Sweeping tasks per `run()` from 1 to 256 splits it into **43.3 us fixed per run() and 8.08 us marginal per task**: the cost is entering and leaving an orchestration, not dispatching work, and a production run submits a graph per `run()` rather than one task. At 256 tasks the fixed part is 2% of the invocation. The same numbers settle #1498 against itself on latency grounds. A pipe wake measures 4.4 us one way on this box, so replacing the poll with a blocking wait adds more than the entire 3.5 us IPC floor to every dispatch, in exchange for CPU, while the latency it was meant to recover is not in the poll at all. What survives is narrower and unrelated: a child spinning while nothing is outstanding holds a core for as long as its parent lives — a CPU question for narrow-core hosts, already smaller since #1495 made orphans exit with their parent. Also records the reconsideration triggers, since both conclusions are conditional: a workload that issues many small `run()` calls stops amortizing the fixed cost, and the CPU argument for a spin-then-block hybrid remains open.
1 participant
Fixes #1493. Stacked on #1494 (
unify-child-mailbox-loops) — the check needs the single_run_mailbox_loopthat PR introduces, so review/merge that one first. The diff here is 27 lines plus a test.Summary
A forked sub/child/chip worker never checked whether its parent was still alive. When the parent died without
Worker.close()— SIGKILL from atimeout, an OOM kill, a cancelled CI job, an editor killing a wedged pytest — every child was reparented and kept polling a mailbox nobody would ever write to. Because that poll is an unbounded busy-wait, each survivor held a full core indefinitely.Not theoretical: on a shared dev box, six such processes belonging to two different users had been alive 5-7 days each at ~99.6% CPU — six cores permanently consumed. Four of the six carried
tests/ut/py/test_worker/test_startup_readiness.pyon their command line: runs whose parent was killed rather than allowed to finish.Fix
_run_mailbox_loopsamplesgetppid()on its idle path and leaves by the same SHUTDOWN route once it changes, so a child tears down exactly as it would on a clean shutdown.Why not test for pid 1. A subreaper — container init, a systemd user session — adopts orphans instead of init, so the pid changes but never becomes
1. Comparing against the pid captured at loop entry covers both cases, and a live parent's pid cannot change, so the check cannot fire spuriously.Cost. Sampling every
_PARENT_LIVENESS_POLL_INTERVAL(1000) idle polls puts agetppid()roughly every 100 us against a ~0.1 us poll — fast enough that an orphan goes away before it is noticeable, cheap enough to be lost in the noise of the poll itself.Residual gap, stated rather than papered over. A parent that dies between
os.fork()and the child reaching the loop is already gone when the pid is captured, so that child is not detected. Closing it needs the pid handed in from before the fork, orPR_SET_PDEATHSIG— which is Linux-only and therefore an optimisation on top of this, not the mechanism.Regression test
test_orphan_child_reaping.pySIGKILLs a subprocess parent and asserts every child is gone within 20 s.Two details are load-bearing, both learned by getting them wrong first:
subprocess.run(capture_output=True)and hung for 120 s instead of failing.finallykills whatever survived. That first version raised before it had parsed the pids, so its cleanup never ran and it leaked three spinning processes per run — a leak test that leaks.Red-to-green:
Verification
pytest tests/ut/py -q-> 825 passed, 2 skipped (824 baseline + the new test).pytest examples tests/st --platform a2a3sim --device 0-7 -q-> 108 passed, 2 skipped, 0 failed, 60/60 groups — identical to the Refactor: fold the three child mailbox loops into one state machine #1494 baseline. The chip loop shares this code path, so the device run is the one that matters.pre-commitclean.What this does not fix
The busy-wait itself. With the poll replaced by a blocking trigger an orphan would cost 0% CPU — but it would still be a leaked process holding fds and shm, so that work is complementary, not a substitute. This PR is what bounds the leak in time.
🤖 Generated with Claude Code