Stop the orphan-scan tests asking the host whether an invented PID is alive by danielhanchen · Pull Request #9280 · unslothai/unsloth · GitHub
Skip to content

Stop the orphan-scan tests asking the host whether an invented PID is alive - #9280

Merged
danielhanchen merged 3 commits into
mainfrom
fix-orphan-scan-host-pid
Aug 19, 2026
Merged

Stop the orphan-scan tests asking the host whether an invented PID is alive#9280
danielhanchen merged 3 commits into
mainfrom
fix-orphan-scan-host-pid

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

test_orphan_cleanup_kills_under_real_root[psutil] failed on a staging runner
while passing on the org queue for the same commit, on two unrelated PRs
neither of which touches this code.

The harness builds its fake process with os.getpid() + 888, a PID invented on
the assumption that nothing owns it. _kill_orphaned_servers skips any candidate
whose parent is alive, and _pid_parent_is_alive answers that by looking the PID
up for real -- psutil.Process(pid).ppid(), then psutil.pid_exists(ppid). The
harness stubs psutil.process_iter and nothing else, so that lookup goes to the
actual machine.

On a quiet runner nothing owns the PID, NoSuchProcess comes back, the candidate
is an orphan and gets killed. On a busier one it is a real process with a real
live parent, the candidate is skipped, and the test reports assert 0 == 1
having exercised the ownership logic perfectly correctly. The pass was the
accident, not the failure.

Reproduced by keeping the original harness and forcing the answer a busy host
gives, which yields assert 0 == 1 -- the staging failure exactly.

Both tests in this pair are about OWNERSHIP: the link tree is spared, the real
root is reaped. Parent liveness is incidental to both, so it is pinned rather
than left to whatever else is running on the box. The fake is an orphan by
construction and the harness now says so.

This is the established pattern here rather than a new one:
test_llama_cpp_wait_for_vram_settle.py stubs _pid_parent_is_alive at every one
of its ten call sites. This harness stubbed the sibling _reap_recorded_pid and
missed this one, so the coverage it loses is coverage it never had -- parent
liveness has its own tests in that file.

… alive

test_orphan_cleanup_kills_under_real_root[psutil] failed on a staging runner
while passing on the org queue for the same commit, on two unrelated PRs
neither of which touches this code.

The harness builds its fake process with os.getpid() + 888, a PID invented on
the assumption that nothing owns it. _kill_orphaned_servers skips any candidate
whose parent is alive, and _pid_parent_is_alive answers that by looking the PID
up for real -- psutil.Process(pid).ppid(), then psutil.pid_exists(ppid). The
harness stubs psutil.process_iter and nothing else, so that lookup goes to the
actual machine.

On a quiet runner nothing owns the PID, NoSuchProcess comes back, the candidate
is an orphan and gets killed. On a busier one it is a real process with a real
live parent, the candidate is skipped, and the test reports `assert 0 == 1`
having exercised the ownership logic perfectly correctly. The pass was the
accident, not the failure.

Reproduced by keeping the original harness and forcing the answer a busy host
gives, which yields `assert 0 == 1` -- the staging failure exactly.

Both tests in this pair are about OWNERSHIP: the link tree is spared, the real
root is reaped. Parent liveness is incidental to both, so it is pinned rather
than left to whatever else is running on the box. The fake is an orphan by
construction and the harness now says so.

This is the established pattern here rather than a new one:
test_llama_cpp_wait_for_vram_settle.py stubs _pid_parent_is_alive at every one
of its ten call sites. This harness stubbed the sibling _reap_recorded_pid and
missed this one, so the coverage it loses is coverage it never had -- parent
liveness has its own tests in that file.
@chatgpt-codex-connector

Copy link
Copy Markdown

@danielhanchen
danielhanchen merged commit d496d34 into main Aug 19, 2026
33 checks passed
@danielhanchen
danielhanchen deleted the fix-orphan-scan-host-pid branch August 19, 2026 12:03
danielhanchen added a commit that referenced this pull request Aug 20, 2026
…9200)

Reduced to the one fix of the original three that is still needed. The other two
landed by other routes while this sat: the orphan-scan flake was fixed by #9280,
and the extensionless `./mmproj-fallback` import is already `.ts` on main. Both
are verified identical to main here rather than assumed.

On #9280 taking a different approach: it pins `_pid_parent_is_alive` to a
constant for the whole test, where this PR stubbed it only for the invented PID
and delegated every other PID to the real implementation. That is a real
difference in coverage, so I checked whether main lost anything before dropping
mine. It did not: `test_reap_recorded_pid_spares_live_server` in
test_llama_cpp_wait_for_vram_settle.py exercises the ownership gate for real,
against a process the test genuinely owns. So the gate is still covered and
#9280's version stands.

What is left, and why it is still needed
------------------------------------------------------------------------
`saveMarkdownAsProjectSource` fires `watchIngestion` without awaiting it, and
that watcher polls for up to 300 seconds. Watchers from earlier tests therefore
outlive them and keep calling the shared fetch stub, which by then belongs to
whichever test is running now. The stub answered any `/jobs/` URL and the uploads
shared a filename, so a stale watcher emitted a toast byte-identical to the one
the running test was waiting on, under a different project id.

The test then waited on the toast and read the announce count without polling.
The watcher toasts and announces with nothing awaited between them
(save-markdown-source.ts, `toast.error(...)` then
`announceProjectSourcesUpdated(...)`), so a tick landing in that window saw 1
instead of 2.

Each job handler now answers only its own job id and 404s anything else, so a
stale watcher dies quietly instead of borrowing the running test's answers. The
save under test gets its own filename, and the test polls on the announce, which
the watcher does last, then asserts the toast synchronously.

Verified
------------------------------------------------------------------------
Gap sweep, inserting a 150 ms delay into the source between the toast and the
announce to widen the window the race needs: main's version of the test fails 6
of 6 runs, this version passes 6 of 6. Both pass 6 of 6 on an idle box with no
delay, which is why this reads as an intermittent CI failure rather than a
reproducible one.

Mutation-tested: deleting `announceProjectSourcesUpdated` from the watcher fails
this test (9 passed, 1 failed), so it still proves what it did before rather than
having been loosened into passing.

Full frontend suite: 4087 passed, 0 failed.
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