Scope the llama.cpp update test's Popen patch to the installer - #10105
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd03599434
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # subprocess.run() uses `with Popen(...)`, and without these it raises into | ||
| # _pid_identity's bare except, silently disabling the identity check. | ||
| def __enter__(self): | ||
| return self |
There was a problem hiding this comment.
When this helper intercepts the forced macOS ps lookup, subprocess.run() calls process.communicate(...) after entering the context, but _FakeInstallerPopen still has no communicate method or args attribute. The resulting AttributeError is swallowed by process_lifetime._pid_identity, so the new test only proves that construction of the ps process was attempted while silently disabling the identity lookup it claims to exercise. Implement the remaining protocol with representative ps output, or delegate non-installer commands to the original Popen, so failures after that spawn cannot remain hidden.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it undercut the claim I made in the description. Fixed in be6458d.
Reproduced it directly: driving subprocess.run through the double raises AttributeError: '_FakeInstallerPopen' object has no attribute 'communicate', so __enter__/__exit__ alone were not enough and _pid_identity was still swallowing the failure.
Took the delegation option rather than growing the fake, since a stand-in owes subprocess.run the whole protocol and the next missing method would fail the same silent way. That also matches the dominant idiom in this suite (_REAL_POPEN bound at module scope, filter on the command, delegate the rest, as in test_llama_cpp_placement.py). __enter__/__exit__ are gone again, since only the installer sees the fake now and that path uses Popen directly.
The regression test also asserts the lookup completes rather than merely starts:
assert process_lifetime._pid_identity(os.getpid()), "the ps lookup did not complete"
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
test_start_update_happy_pathfails on macOS, and has since 2026-08-09:The update itself is fine. The same run logs the installer starting with the right arguments and finishing on the new tag:
Only the assertion is wrong.
What happens
_patch_installer_popendoesmonkeypatch.setattr(upd.subprocess, "Popen", ...).subprocessis a shared module object, so that replacesPopenfor the whole process and the double fireson_startfor every spawn.on_startwritescaptured["cmd"] = cmd, last one wins.That was harmless when the phase spawned exactly one thing. #8170 changed it:
_run_llama_phasenow callsadopt_pid(proc.pid)right after starting the installer, which reachesprocess_lifetime._pid_identity. On Linux that reads/proc/<pid>/statand spawns nothing, so the test passes. macOS has no/procand shells out tops -o lstart=,comm= -p <pid>, a second spawn, after the installer, which overwrites the capture.dab0b7767Popenpatchb2b1dcd6aadopt_pidplus the macOSpsbranchc6ad59a7dNothing in CI runs
studio/backend/tests/on macOS, so this only ever showed up running the suite locally on a Mac.Why the test, and not the code
adopt_pidand thepsidentity check are right. A recorded pid has to be provably the same process before anything signals it, and on macOSps -o lstart=is the only cheap source of that. Nothing there should change to suit a test double.The fix is also already in the tree.
_patch_llama_installerintest_combined_update.pygates on the installer command, added in #7095, a month before #8170 landed:test_llama_cpp_update.pynever got the same treatment. This applies it.Changes
_patch_installer_popenonly fireson_startandcaptured_kwargsfor the installer command._FakeInstallerPopengains__enter__/__exit__.subprocess.runuseswith Popen(...), so without them a non-installer call raisedAttributeErrorinto_pid_identity's bareexcept Exception: return None, silently disabling the identity check the fake had just intercepted.No product code is touched.
Verification
On
main, withprocess_lifetime.sys.platformforced todarwin:After the change, forced and unforced:
The new test was run against
main's unfixed helper first and fails there, so it is not a test that passes either way. It also asserts apsspawn actually happened, so a green run cannot come from never reaching the ordering it guards.test_llama_cpp_update.py,test_combined_update.pyandtest_orphaned_children.pytogether: 190 passed, 1 skipped.