fix(test): reset the create-rate-limit bucket between session-control tests - #7852
fix(test): reset the create-rate-limit bucket between session-control tests#7852timwukp wants to merge 1 commit into
Conversation
… tests The bucket is process-wide module state keyed per caller. Every test in this file builds its caller as _slot(state, "chat-1"), so all 36 create_session call sites share one bucket key against a budget of 20 per 300s window. The file runs in ~1.5s, far inside the window, so the creates accumulate and a later test is refused a create it is the first to ask for. Running the file whole fails deterministically on test_the_created_agent_name_is_sanitized_before_storage and test_the_audit_write_does_not_run_on_the_event_loop. It reads as an intermittent CI failure only because pytest-split distributes the file across groups, so whether a group crosses the budget depends on the split. Adds an autouse reset fixture, matching test_create_rate_limit.py and test_chat_folder_cap.py which already reset this bucket.
github-actions
Bot
added
fork
github-actions
Bot
added
readiness: checking
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsThe function exists and does exactly what the fixture relies on. The diff is a test-only autouse fixture that resets process-wide bucket state. No candidates to falsify, and nothing in the diff introduces a defect, removes a guard, or touches a rule-governed surface. No findings. [OPUS-REVIEWED] 7b07d58 |
Design Review (Fable 5, fork) — 🔴 BLOCK (blocking)Design-level review of Design-Verdict: BLOCK Superseded: #7840 already landed this exact fix in Blockers
Suggestions
[DESIGN-REVIEWED] 7b07d58 |
github-actions
Bot
added
readiness: action required
Closing — already landed on
|
github-actions
Bot
removed
the
readiness: action required
2 participants
Problem / Motivation
Backend Tests (Windows) (3)fails intermittently onmainwith two tests refused a session create:Observed on
mainin 2 of the 9ci.ymlruns that completed that job in a ~5.5-hour window — runs 33604886863 (be2ee947) and 33599188871 (aaad3571), with byte-identical annotations.It is not actually intermittent.
create_rate_limit._bucketsis process-wide module state keyed(verb, caller_key). Every test intest/test_session_control.pybuilds its caller as_slot(state, "chat-1"), so_key()returns the samecaller_keyfor all of them, and the file has 36create_session(call sites against a budget of 20 per 300 s (MAX_SESSION_CREATES_PER_WINDOW/WINDOW_SECS). The file runs in ~1.5 s — three orders of magnitude inside the window — so the creates accumulate and the 21st onward is refused.Running the file whole fails deterministically:
What makes it look intermittent in CI is
pytest-split: it distributes this file across 4 groups, so whether any one group carries more than 20 creates depends on the split, and the split shifts as tests are added. That also explains why the same shard number passes on 3.10/3.12 while Windows fails — nothing about the failure is platform-specific, only which tests land together.The file already resets one piece of process-wide state (
stop_retry) in an autouse fixture; this bucket was simply missed.Why it matters
The failure is attributed to whichever test happens to be past the boundary rather than to the cause, so it reads as a defect in
test_the_audit_write_does_not_run_on_the_event_loop— a test about SEL construction threading — when nothing in that test is broken. A reader who trusts the failure location debugs the wrong subsystem.It also costs real throughput: the job is part of the
PR Readinessaggregate, so an unrelated PR inherits a red readiness signal roughly a fifth of the time, and re-running is a ~78% coin flip rather than a fix. Fork contributors cannot even do that —gh run rerun --failedreturnsMust have admin rights to Repository.And it degrades the guard's own test value: once the bucket is saturated, later tests in the file exercise the rate-limit refusal path instead of the behaviour they were written for, so a regression in that behaviour would not be caught.
What changed (motivation → approach → change)
Motivation: the tests must be independent of each other; a create budget consumed by an earlier test is not a property any of these tests is asserting.
Approach: reset the bucket per test rather than raising the budget or spreading the caller keys. Raising
MAX_SESSION_CREATES_PER_WINDOWwould weaken a production security control to accommodate a test artifact — that budget is deliberately sized (the module docstring explains it bounds an auto-approved verb against a creation loop). Giving each test a distinct caller key would work but touches 93 call sites and would silently stop covering the shared-caller case. Resetting is what the repo already does elsewhere:test_create_rate_limit.pyandtest_chat_folder_cap.pyboth callcreate_rate_limit.reset_for_tests(), andreset_for_tests()exists for exactly this ("module state would otherwise leak across tests").Change: one import plus one autouse fixture in
test/test_session_control.py, placed beside the existing_fresh_stop_windowsfixture and shaped identically. No production code changes; no test assertions changed.Tests
Reproduced first, then fixed — the before/after is the evidence:
Confirmed no interaction with the two files that already reset this bucket:
And under CI's own sharding, all four groups green:
No new test is added: the fixture's correctness is demonstrated by the 2 pre-existing failures it clears. A test asserting "the bucket is empty at test start" would assert the fixture against itself.
Manual verification
Run on macOS arm64 (Python 3.12) against
mainat8ebf6a87. Because the repo's dev extras carry a pre-existing pin conflict (kirocrew:devpinspytest-asyncio==0.20.3while[dev]wants>=0.21), the suite was run against the package's owninstall_requiresrather than the dev extra:I did not reproduce on Windows. The mechanism is platform-independent (module-level state, a monotonic window, and the split), and the identical annotation on Linux
mainruns supports that, but I am stating the gap rather than implying I checked it.Related Issues
None — this flake does not have an issue filed. Raising one to immediately supersede it with this PR seemed like noise; happy to file one if maintainers prefer the paper trail.
Noticed while investigating #7522 / #7553, where this same job was the only red on a fork PR whose diff cannot reach
session_control. That is context, not a dependency: this stands alone and touches no file either of those does.Pattern harvest
Rule candidate:
review-promptPattern: a test file that exercises a guard backed by process-wide module state must reset that state per test, or the guard's own budget leaks between tests and the failure surfaces on an unrelated test past the boundary. The tell is a module-level mutable (
_buckets,_last_sweep) plus areset_for_tests()helper that some test files call and others do not.The generalizable check: for each module exposing
reset_for_tests(), every test file that transitively drives it should reset it.create_rate_limithad 2 of 3 callers doing so. That is mechanically checkable and would have caught this before it reached CI.A second, sharper observation:
pytest-splitconverts order-dependent failures into apparently flaky ones, which is worse than a hard failure because the usual response is a re-run. Anything that fails deterministically when a file runs whole but intermittently under sharding is this class, and the fix is never a re-run.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)