Fix: read the orphan test's child pids from a file, not the parent's log - #1504
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
`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-hw-native-sys#1495
`worker.py` ("3 of 3 children outlived their SIGKILLed parent by 20s",
now with exactly three pids and no phantom entry).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correction to this PR's description, now that the sibling PRs' logs are readable: in CI this is deterministic, not intermittent. Three independent runs, three different real pid sets, always the same trailing phantom: Every So |
mainis currently red onutbecause of this. My test from #1495, not the fix it guards.What happened
test_orphan_child_reapingscraped the forked children's pids out of the parent subprocess's combined stdout+stderr, taking every digit-only token. That stream also carries log lines and warnings, so any bare number in them was read as a pid:The three real pids are correct —
Worker._sub_pidsis not at fault, and the run was otherwise healthy. The test failed for a parsing artifact.Worse than a false failure: the
finallyblock would then have sentSIGKILLto whatever process that stray number named.Fix
Give the parent a dedicated pid file, named on its command line. stdout+stderr stay captured 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 — filtering harder is the same mistake in a different shape, and it is the third time this test has been bitten by how it identifies children:
pgrep -Pinside the parentos.popenspawns, and container pid 3Worker._sub_pidsprinted to stdoutWorker._sub_pidswritten to its own fileThe
isdigit()filter goes too: a non-numeric token in a dedicated pid file would be a real defect worth surfacing, not noise to skip past.Verification
worker.py:3 of 3 children outlived their SIGKILLed parent by 20s— now exactly three pids, no phantom entry.pre-commitclean.🤖 Generated with Claude Code