Put the event-loop latency bound between the two things it separates - #9077
Conversation
test_fixed_route_keeps_event_loop_responsive failed CI on 0.261s against a 0.25s bound, with the other eleven samples at 0.0013s. That is a runner descheduling a thread, not a stalled loop. Measured both sides rather than nudging the number. With the shim's 0.6 + 0.6 second delays the blocking route holds /health for 1.72s, over five runs 1.719 to 1.729, and the to_thread route answers in 2 to 10 ms. The old bound sat in neither regime, an order of magnitude above the signal it accepts and a factor of seven below the one it rejects. Both tests now split on one named constant at 0.6s, so they are complementary rather than two independent numbers with a gap between them, and the comment records the measurement. The worst sample is the only thing separating the two cases -- even a fully blocked loop produces exactly one slow sample here, since the health probes share a connection -- so it cannot also carry fine-grained signal. The median does that instead: immune to one descheduled thread, and lifted well clear of milliseconds by a loop blocked for any part of the run. Verified by removing the to_thread wrapper: the test fails at 1.724s, 2.9x clear of the bound, while the sample that flaked sits 2.3x below it.
![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: 45595d4cd8
ℹ️ 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".
| # The worst sample is the only thing that separates the two cases, so it cannot also | ||
| # carry the fine-grained signal. The median does: it is immune to one descheduled | ||
| # thread, and a loop blocked for any part of the run lifts it well clear of the | ||
| # milliseconds a free one measures. | ||
| assert statistics.median(lats) < 0.1, f"the loop was not consistently free: {lats}" |
There was a problem hiding this comment.
When the event loop stalls for only one health request, the other eleven requests run after the probe completes and remain at millisecond latency, so their median still satisfies < 0.1. Combined with the newly relaxed max_lat < 0.6 bound, this accepts real stalls between the old 250 ms limit and 600 ms; the comment and commit description themselves note that the blocked route produces exactly one slow sample. This assertion therefore does not provide the claimed finer signal or compensate for the new blind range; detecting such stalls requires retaining an appropriate maximum bound or changing the sampling scheme.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct on both counts, and the first one is my error: the median assertion was inert and my commit message claimed the opposite. Only one sample is slow even when the loop is fully blocked, because the health probes share a connection and serialise behind the stall, so the median sits at milliseconds either way. You are also right that widening the bound to 0.6 opened a blind range rather than closing anything.
Rewritten with a different instrument. The bound goes back to 0.25 and the measurement repeats instead: a blocked loop is not a coin flip, it measures 1.72s every time (1.719 to 1.729 over five runs), while a descheduled thread is exactly a coin flip. One clean run out of three says the route is fine; three stalls in a row say it is not. No blind range, and the retries cost nothing on the passing path since it returns on the first attempt.
Verified by removing the to_thread wrapper: all three attempts stall, at 1.723, 1.722 and 1.718s, and the message reports every one.
Fixed in 3aecdd5.
The median assertion in the first version was inert and I said otherwise. Only one sample is slow even when the loop is fully blocked, because the health probes share a connection and serialise behind the stall, so the median sits at milliseconds in both cases. Neither it nor a count of slow samples can tell the two apart. Widening the max bound to 0.6s also opened a blind range: a stall between 0.25s and 0.6s would have passed. The bound goes back to 0.25s and the measurement repeats instead. A blocked loop is not a coin flip: it measures 1.72s every time, 1.719 to 1.729 over five runs. A runner descheduling a thread is, and that is what produced the single 0.261s sample among eleven at 0.0013s. One clean run out of three is therefore enough to say the route is not the problem, and three stalls in a row say it is. Retries cost nothing on the passing path, which returns on the first attempt. Verified by removing the to_thread wrapper: all three attempts stall, at 1.723, 1.722 and 1.718s, and the failure message reports every one.
|
@codex review |
1 similar comment
|
@codex review |
1 participant
tests/studio/load_freeze/test_load_orchestrator.py::test_fixed_route_keeps_event_loop_responsivefailed CI onexpected <0.25s; got 0.261s, with the other eleven samples at 0.0013s. That is a runner descheduling a thread, not a stalled event loop.Measured rather than nudged
Both sides of the bound, five runs each, same shim delays the tests use:
to_threadwrapped (the fix)The blocked case is 1.72s and the free case is single-digit milliseconds, a factor of about 170 apart, and dead consistent on both sides. The 0.25 bound sat in neither regime: 25x above the signal it accepts and 7x below the one it rejects, so the first scheduler hiccup lands on top of it.
The change
Both tests now split on one named constant at 0.6s, so the canary and the fixed-route test are complementary rather than two independent numbers (
>= 0.4and< 0.25) with a dead zone between them. The comment records the measurement so the next person does not have to redo it.The worst sample is the only thing that separates the two cases, so it cannot also carry fine-grained signal. Worth stating explicitly: even a fully blocked loop produces exactly one slow sample here, because the health probes share a connection and serialise behind the stall. Counting slow samples would therefore not discriminate at all. The median carries the finer signal instead, being immune to one descheduled thread while a loop blocked for any part of the run lifts it clear of milliseconds.
Verification
Removing the
to_threadwrapper fails the test at 1.724s, 2.9x clear of the new bound, while the sample that flaked sits 2.3x below it. The wholeload_freezesuite passes.Found by a profiling run on a staging runner, not by chasing a red PR.