Stop ten test assertions from measuring the runner instead of the code - #10897
Stop ten test assertions from measuring the runner instead of the code#10897danielhanchen wants to merge 10 commits into
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. |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
#10868 replaced the linearity check in test_pr5624_regressions.py after it failed on #10825 and again on #10864 at 6.56 against a 6.0 bound, both times on branches that touch none of that code. The same shape is elsewhere in the suite: sub-100ms budgets, two separately-taken durations compared against each other, and concurrency proved by how long four threads took. All of them run under pytest -n 4 on a shared runner, where descheduling is indistinguishable from the regression they guard. Each one is rewritten to assert the thing it is named for. test_llama_cpp_wait_for_vram_settle.py has five, all of them claims about the helper's sleeping: did it skip the wait, nap once per poll, clip the last nap to the deadline. Record the durations it asks for and assert on those. The tightest was elapsed < 0.05 around a short-circuit, which one GC pause exceeds. test_account_model_access.py proved eight probes ran together by taking under 0.4s. Its own sibling two functions up already uses a threading.Barrier for the same claim, so use one here too, sized off _PROBE_FANOUT so lowering the fan-out fails this rather than satisfying a Barrier(1). test_mcp_http_sessions.py had three. Two chats running concurrently needed a counter that spans clients, since no single client's max_live can see an overlap between two of them; six parallel closes get a Barrier(6), so a serial close deadlocks and says so instead of merely running slowly; and the eviction test asserts the caller returned while the victim's teardown was still running, which is the property the 0.5s bound was standing in for. test_stream_cancel_registration_timing.py compared two separately-taken durations, exactly what #10868 had to remove from _growth. The two patterns differ in chunks seen, deterministically: the blocking loop never lets the cancel through and drains all eight, the awaiting loop stops at the first. Four guards against a quadratic blow-up carried absolute budgets (test_tool_xml_strip.py, test_update_release_notes.py, test_rag_store.py). Backtracking is superlinear by definition, so growth is the property, and #10868's paired-ratio statistics already exist for it. Lift _growth and _assert_linear out of test_pr5624_regressions.py into tests/_shared/growth.py, generalised over the function under measurement, next to real_accelerator.py and for the same reason. test_profile_stats.py and the two liveness files keep a wall-clock bound, because "it did not block" is what they mean, but a generous one: the sharp claim in each is already asserted next to it (the row count is zero, stop() returned False, the stub raises if detection starts). test_backend_ci_parallel_isolation.py scans for this shape and required three named files to keep exhibiting it, which made it a second, invisible reason those files could not be fixed: rewriting the vram-settle bounds failed there, in a file about CI topology, with a message about a sample. It builds all three shapes itself now. Not touched: test_health_answers_within_probe_budget.py. Its bounds are the subject, derived from the real desktop probe timeout rather than from a guess, and it already asserts on state where state was the honest answer. Each rewritten assertion was checked against a deliberately broken implementation: no cold-start short-circuit, an unclipped sleep, a one-shot wait loop, _PROBE_FANOUT of 1, _MAX_CLOSE_THREADS of 1, and two synthetic quadratic sweeps. Full backend suite run the way CI runs it, three times: identical failure set to main.
…eilings Two review findings, both right. assert_linear measures units AND units * factor, so replacing an absolute budget means passing the old size divided by factor. Three of the five conversions did that; the backtick-run and orphan-opens ones did not, and their big leg ran at 4x anything previously measured. That matters only on the regression each guards: the backtick parser took over a minute at 20k when broken, so at 80k it would take about sixteen, and the job timeout would kill the run before the ratio could say why. The big < 60.0 backstop cannot help, because it is checked after the measurement rather than during it. Both call sites now put the previously measured size on the big leg, and assert_linear says so where a caller reads it. The liveness ceilings were widened to a number with no relation to the product. commands.rs gives each watchdog probe HEALTH_PROBE_TIMEOUT, ten seconds, so a route that blocked for 10 to 29 seconds passed the test while timing out every real probe. Read that constant, as test_health_answers_within_probe_budget.py already does for its own budget, and use half of it: ten times the headroom of the 0.5s it replaces, and provably inside what the launcher allows.
The eviction test's victim slept 1.5s, which a caller only had to outwait part of to read as prompt; it now holds its teardown on an event the test releases, so a caller that waits on it at all cannot return, and the remaining ceiling comes from _SESSION_CLOSE_TIMEOUT rather than being picked. The writer shutdown test now records the timeout stop() hands its join instead of allowing 5s of wall clock, so a stop() that waits whole seconds fails even though the elapsed budget would have passed it.
for more information, see https://pre-commit.ci
… does not need The never-settles case only checked that the helper napped more than once, so a loop capped at two iterations recorded [0.1, 0.1] and passed while the VRAM was still moving. Both ends are now read off the recorded naps. The terminal-callback bound goes back to 1.0s: studio-backend-ci.yml ignores test_profile_stats.py in the -n 4 leg and reruns it serially, so a looser bound only lets a stalling callback through.
197d740 to
76fd12e
Compare
…chdog Half the watchdog's probe budget is 5s, which passes a route that grew seconds of synchronous work on the event loop. The probe snippets now serve a route that only returns a dict through the same app and client, so both pay the same interpreter and scheduler and what is left is the route's own cost. The watchdog bound stays as the second half of the pair: a relative bound cannot see an absolute timeout, since a control that took seconds would scale with it.
Half of _SESSION_CLOSE_TIMEOUT is five seconds, which passes a caller that blocks for a second before handing the victim to the cleanup worker. The cap is now applied after a control call on a second URL, so the two differ only in whether an eviction happens and the claim is measured rather than budgeted.
…un, drop a live dependency The slow-probe test asked for a nap that a correct helper need not make: if the scheduler ate the 0.1s left of the budget it returned at the deadline check instead, and the assertion failed on a good run. It now runs on a clock the test moves, so the probe charges its cost instead of sleeping and every sample is exact. growth() takes abort_over_s, so a big leg already over the backstop stops the repeats instead of measuring the same known-bad path three times. The parallel-isolation meta-test no longer requires the live backend suite to still contain a fragile assertion; the synthetic files cover every shape, and that requirement would have turned cleaning the last one into a failure.
… thread A control taken in its own batch shares no scheduler delay with the request it is compared against, so a pause landing on the measurement alone was not divided out and the 50ms floor made that stricter than the bound it replaced. Both liveness snippets now alternate control and measurement and take the best of three on each side. The never-settles test moves to the same fake clock as the slow-probe one: its lower bound says the helper did not stop early, and real time spent outside sleep() retires the deadline without appearing in the naps. The eviction test asks which thread calls close() instead of how long the caller took. __aexit__ cannot answer it, since that always runs on the session's own loop thread, but the caller of close() is exactly the claim in the test's name.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 279d0b2272
ℹ️ 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".
| """A popular HTTP server holds a session per chat, and close runs on the | ||
| request thread during an edit or delete.""" | ||
| closes = [] | ||
| overlapping = threading.Barrier(6, timeout = 30) |
There was a problem hiding this comment.
Respect the configured close fan-out in the barrier
If _MAX_CLOSE_THREADS is configured below six, _close_all correctly runs closes concurrently in bounded batches, but every worker blocks at this six-party barrier until its 30-second timeout and the test reports the implementation as serial. This unnecessarily requires all six sessions to overlap rather than verifying the named property that more than one close runs concurrently; size the synchronization to the configured fan-out or record peak concurrency instead.
Useful? React with 👍 / 👎.
| assert closed_by and caller not in closed_by, ( | ||
| "the calling thread closed the evicted session itself, so an unrelated chat's " | ||
| f"teardown is spending the deadline meant for this tool call: {closed_by}" | ||
| ) | ||
| assert not closed.is_set(), "the caller waited out an unrelated eviction" | ||
| assert elapsed < 30.0, f"the call never came back: {elapsed:.2f}s" |
There was a problem hiding this comment.
Verify the caller never waits on the cleanup worker
When eviction starts close() on a background thread but then waits on that worker with a bounded join, these assertions still pass: closed_by excludes the caller, closed remains unset, and any partial wait under 30 seconds satisfies the elapsed ceiling. For example, a five-second worker.join(timeout=5) would consume five seconds of the tool-call deadline while this regression test succeeds, so the test should also detect waiting on the cleanup worker rather than only waiting for teardown to finish completely.
Useful? React with 👍 / 👎.
What this is
#10868 replaced the linearity check in
test_pr5624_regressions.pyafter it failed on #10825 and again on #10864 at 6.56 against a 6.0 bound, both times on branches that touch none of that code. Going through the last 40 PRs for other red checks, the same shape turns up elsewhere in the suite: sub-100ms budgets, two separately-taken durations compared against each other, and concurrency proved by how long four threads took. They all run underpytest -n 4on a shared runner, where being descheduled is indistinguishable from the regression they guard.Ten of them are rewritten to assert the thing they are named for. No thresholds are loosened to make a failure go away; where a wall-clock bound is still the honest answer it stays, with enough headroom that only a real hang trips it, and the sharp claim sits next to it.
The rewrites
test_llama_cpp_wait_for_vram_settle.py(five). Every claim in the file is about the helper's sleeping: did it skip the wait, nap once per poll, clip the last nap to the deadline. It now records the durations_wait_for_vram_settleasks for and asserts on those. The tightest bound waselapsed < 0.05around a short-circuit, which one GC pause exceeds while the helper did exactly the right thing.test_account_model_access.py. Eight probes running together was asserted as "under 0.4s" while eight threads each slept 0.1s. Its own sibling two functions up already proves the same claim with athreading.Barrier, so this uses one too, sized off_PROBE_FANOUTso lowering the fan-out to 1 fails the test rather than quietly satisfying aBarrier(1).test_mcp_http_sessions.py(three). Two chats running concurrently needed a counter that spans clients, because no single client'smax_livecan see an overlap between two of them. Six parallel closes get aBarrier(6), so a serial close deadlocks and says so instead of merely running slowly. The eviction test now asserts the caller returned while the victim's 1.5s teardown was still running, which is the propertyelapsed < 0.5was standing in for.test_stream_cancel_registration_timing.py. This compared two separately-taken durations, the exact shape #10868 had to remove from_growth. The two patterns differ in chunks seen, deterministically: the blocking loop never lets the cancel through and drains all eight, the awaiting loop stops at the first.Four quadratic guards (
test_tool_xml_strip.pyx2,test_update_release_notes.pyx2,test_rag_store.py). Backtracking and rescanning are superlinear by definition, so growth is the property and an absolute budget is a proxy for it. #10868's paired-ratio statistics already exist, so_growthand_assert_linearmove out oftest_pr5624_regressions.pyintotests/_shared/growth.py, generalised over the function under measurement, next toreal_accelerator.pyand for the same reason.test_profile_stats.pyand the twotest_liveness_*files keep a wall-clock bound, because "it did not block" is what they mean. The bound is generous now, and in each case the sharp claim is already asserted beside it: the locked connection still sees zero rows,stop()returnedFalse, the stub raises if detection is started.test_backend_ci_parallel_isolation.pyscans the backend suite for this shape and required three named files to keep exhibiting it, as samples proving the scan was not vacuous. That made it a second, invisible reason those files could not be fixed: rewriting the vram-settle bounds failed there, in a file about CI topology, with a message about a sample. It builds all three shapes itself now, and also checks it does not fire on a bound with real headroom.Not changed
test_health_answers_within_probe_budget.py. Its bounds are the subject of the test, and they are derived from the real desktop probe timeout and the real health budget constant rather than from a guess. It already asserts on state wherever state was the honest answer, and says so in its comments.Verification
Every rewritten assertion was checked against a deliberately broken implementation, one at a time:
test_cold_start_returns_immediately_without_probing,test_stale_kill_skips_waittest_max_wait_respected_when_probe_is_slow_PROBE_FANOUT = 1test_a_listing_probes_its_unknown_repos_together_MAX_CLOSE_THREADS = 1test_closing_many_sessions_does_not_run_seriallyassert_linear, at 14.1x and 31.3xBackend suite run the way
studio-backend-ci.ymlruns it, three times on this branch and once onmain: identical failure set (all pre-existing local dependency gaps,sqlite-vecandfastmcp), same pass count.Lint gates run locally:
ruff check,lint_exec_literals.py,lint_duplicate_definitions.py,verify_import_hoist.py --self-test.