{{ message }}
Stop the orphan-scan tests asking the host whether an invented PID is alive - #9280
Merged
Conversation
… 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.
for more information, see https://pre-commit.ci
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.
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.

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 == 1having 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.