Skip to content

Stop ten test assertions from measuring the runner instead of the code - #10897

Open
danielhanchen wants to merge 10 commits into
mainfrom
ci/timing-assertions-that-measure-the-runner
Open

Stop ten test assertions from measuring the runner instead of the code#10897
danielhanchen wants to merge 10 commits into
mainfrom
ci/timing-assertions-that-measure-the-runner

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

What this is

#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. 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 under pytest -n 4 on 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_settle asks for and asserts on those. The tightest bound was elapsed < 0.05 around 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 a threading.Barrier, so this uses one too, sized off _PROBE_FANOUT so lowering the fan-out to 1 fails the test rather than quietly satisfying a Barrier(1).

test_mcp_http_sessions.py (three). Two chats running concurrently needed a counter that spans clients, because 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. The eviction test now asserts the caller returned while the victim's 1.5s teardown was still running, which is the property elapsed < 0.5 was 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.py x2, test_update_release_notes.py x2, 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 _growth and _assert_linear move 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 test_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() returned False, the stub raises if detection is started.

test_backend_ci_parallel_isolation.py scans 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:

sabotage caught by
neither cold-start short-circuit fires test_cold_start_returns_immediately_without_probing, test_stale_kill_skips_wait
sleep no longer clipped to the deadline test_max_wait_respected_when_probe_is_slow
wait loop runs at most once three settle tests
_PROBE_FANOUT = 1 test_a_listing_probes_its_unknown_repos_together
_MAX_CLOSE_THREADS = 1 test_closing_many_sessions_does_not_run_serially
two synthetic quadratic sweeps assert_linear, at 14.1x and 31.3x

Backend suite run the way studio-backend-ci.yml runs it, three times on this branch and once on main: identical failure set (all pre-existing local dependency gaps, sqlite-vec and fastmcp), same pass count.

Lint gates run locally: ruff check, lint_exec_literals.py, lint_duplicate_definitions.py, verify_import_hoist.py --self-test.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-14T08:22:09.253060Z 279d0b2 Manual request
🔒 Security Review Completed 2026-09-14T03:56:25.119363Z 561ce78 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@danielhanchen

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 561ce781f7

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

danielhanchen and others added 6 commits September 14, 2026 05:02
#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.
… 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.
@danielhanchen
danielhanchen force-pushed the ci/timing-assertions-that-measure-the-runner branch from 197d740 to 76fd12e Compare September 14, 2026 05:05
chatgpt-codex-connector[bot]

This comment was marked as resolved.

…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.
chatgpt-codex-connector[bot]

This comment was marked as resolved.

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.
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
danielhanchen added a commit to shimmyshimmer/unsloth-staging-4 that referenced this pull request Sep 14, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

…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.
chatgpt-codex-connector[bot]

This comment was marked as resolved.

… 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.
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Sep 14, 2026
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment on lines +737 to +742
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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

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