Stop six backend tests waiting on the clock instead of on a signal - #8975
Conversation
These six files spend most of their runtime asleep rather than working, and the waiting buys nothing: every one of them is waiting out a fixed window when it could be waiting for the event it actually cares about. Measured on a fresh worktree off main, the six files together: 118.0s and 118.8s before, 56.5s and 55.3s after. 404 passed and 3 skipped in every run, before and after. Backend tests runs on four Python versions, so that is about 4 runner-minutes a run. No production constant is lowered. The diff is six files, all under studio/backend/tests/. Where a delay was reachable from production code the test now passes an existing parameter at the call site: _terminate_validation_server already takes grace, defaulted to 5.0, and the sibling test in the same commit already passes 0.2, so this one passes 1.0 instead of taking the default four times over. The measured margin there is 25 to 50x, since with the kill mutated out the sentinel appears in 20 to 40ms. test_tool_output_streaming.py is the bulk of it, 61.5s to 32.1s. Seven grandchild processes were fused with sleep 3 and the test then waited 4s or 11s for them. The fuse is now a gate file the test owns, so the grandchild lives indefinitely and the assertion is stronger than "3 is greater than 1". test_cloudflare_tunnel.py is a correctness fix that happens to save 5s. monkeypatch.setattr(ct.threading, "Thread", ...) sets the attribute on the threading module itself, so it replaced threading.Thread process-wide rather than just the tunnel's stdout reader. utils/process_lifetime._Spawner then started a helper thread that never ran, burned its full 5s readiness backstop, and fell back to spawning inline, which silently gives up the PR_SET_PDEATHSIG guarantee that helper thread exists to provide. No assertion is masked today because Popen is faked too, but the patch reaches far outside what the test meant to stub. It is now a shim that overrides Thread and proxies the rest. Detection power is unchanged, and that is measured rather than asserted. Neutering _killpg_captured in core/inference/tools.py fails 7 of the 55 tests in test_tool_output_streaming.py on this branch, and the identical mutation on unpatched main fails the same 7 by name. Same for the others: the installer test fails when the post-SIGKILL group escalation goes, the probe test fails when the probe runs inline, the child-lifetime test fails when PDEATHSIG is dropped, and the cloudflare test fails when the pending-stop record is not cleared. Attribution, since these were deliberate in some cases and accidental in others: #7083 added the grandchild fuses, #8170 the installer grace and child lifetime, #6494 the startup probe, #7875 the cloudflare backstop, #7454 the cold scan. Two of those were sound decisions and are strengthened here rather than weakened.
![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: 6c07f1a6a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| while time.monotonic() < deadline: | ||
| assert not sentinel.exists(), ( | ||
| "a grandchild survived the process-group kill and wrote its sentinel" | ||
| ) | ||
| time.sleep(0.02) |
There was a problem hiding this comment.
When a leaked grandchild writes the sentinel during the final sleep(0.02), the loop exits because the deadline has elapsed without checking the file again, so all seven rewritten process-group tests can incorrectly pass. This is especially plausible on a heavily loaded CI runner where the surviving grandchild is scheduled near the end of the one-second window; add a final absence assertion after the loop or structure the polling so every sleep is followed by a check.
Useful? React with 👍 / 👎.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e209c263d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
If any rewritten test raises, times out, or is interrupted before _assert_grandchild_was_killed creates the gate, this grandchild now loops forever; unlike the previous three-second fuse, it has no independent exit condition. Because it also inherits and holds the test process's stdout pipe open, such a failure can leave processes contaminating subsequent tests or prevent a CI harness from observing EOF after pytest exits. Keep the gate-based fast path, but add a generous safety timeout or ensure every caller opens the gate and reaps the process in finally.
Useful? React with 👍 / 👎.
1 participant
Six backend test files spend most of their runtime asleep rather than working, waiting out a fixed window when they could be waiting for the event they actually care about.
Measurement
Same worktree, off current
main, six files together:404 passed, 3 skippedin every run, before and after.Backend testsruns on four Python versions, so this is roughly 4 runner-minutes per run.What changed
test_tool_output_streaming.py, 61.5s to 32.1s, is the bulk of it. Seven grandchild processes were fused withsleep 3and the test then waited 4s or 11s for them. The fuse is now a gate file the test owns, so the grandchild lives indefinitely and the assertion is stronger than "3 is greater than 1".test_orphaned_children.py, 17.2s to 4.9s. The test took_terminate_validation_server'sgracedefault of 5.0 and paid it four times. It now passesgrace = 1.0; the sibling test added in the same commit already passes 0.2. The production default is untouched.test_startup_llama_probe_non_blocking.py7.2s to 1.6s,test_child_lifetime_boundary.py6.6s to 1.7s,test_openai_auto_download.py6.35s to 1.9s. All three were sleeping inside fakes the tests define themselves; they now block on an event the test releases, which is an unbounded stall rather than a bounded one, so the guarantee is stronger.One correctness fix, in
test_cloudflare_tunnel.pyThis one is worth reading even though it only saves 5s.
monkeypatch.setattr(ct.threading, "Thread", ...)sets the attribute on thethreadingmodule object, so it replacedthreading.Threadprocess-wide, not just the tunnel's stdout reader that the test meant to stub.utils/process_lifetime._Spawnerthen started a helper thread whosestart()did nothing, sousable()burned its full_ready.wait(5.0)and returned False, andspawn_on_lifetime_threadfell back to spawning inline on the calling thread. That fallback silently gives up thePR_SET_PDEATHSIGguarantee the helper thread exists to provide, which its own docstring explains.No assertion is masked today, because
Popenis faked too. The point is that the patch reaches far outside what the test intended. It is now a shim that overridesThreadand proxies everything else.Detection power is measured, not asserted
Neutering
_killpg_capturedincore/inference/tools.pyfails 7 of the 55 tests intest_tool_output_streaming.pyon this branch. The identical mutation on unpatched main fails the same 7 by name, 48 passed either way. So the rewrite costs nothing in what it catches.The others, each a single unique-string replace:
test_orphaned_children.pyinstall_llama_prebuilt.pyremovedtest_cloudflare_tunnel.py_tunnels_pending_stop.discard(tunnel)removedWorth noting on the
grace = 1.0change: with the kill mutated out the sentinel appears in 20 to 40ms, so 1.0s is a 25 to 50x margin. I did not shave it further for another 4s, since a missed regression costs more than the seconds.Scope
No production constant is lowered anywhere. The diff is six files, all under
studio/backend/tests/.Attribution, since some of these delays were deliberate and some were not: #7083 added the grandchild fuses, #8170 the installer grace and child lifetime, #6494 the startup probe, #7875 the cloudflare backstop, #7454 the cold scan. Two were sound decisions and are strengthened here rather than removed.