test: make parallel input guardrail overlap deterministic - #4158
Conversation
`test_mixed_blocking_and_parallel_guardrails` and its streaming twin proved that a `run_in_parallel=True` input guardrail overlaps the model call by comparing `time.time()` samples taken around a fixed `asyncio.sleep(0.03)` in each guardrail, then asserting `model_called <= parallel_end`. That assertion has a zero-width margin: it only holds while the runner reaches the model within the 30ms the parallel guardrail spends sleeping. Under `make tests-parallel` the pytest-xdist workers saturate the CPU, the process gets descheduled, the guardrail's timer expires first, and the guardrail resumes before the model call is issued. The tests then fail even though the runner behaved correctly and still scheduled the model concurrently with the guardrail. Both variants have been observed failing this way. Drive the overlap with `asyncio.Event` rendezvous instead. The parallel guardrail now stays suspended until the model call sets `model_called`, and the model call records whether the blocking guardrail had finished and whether the parallel guardrail was still in flight. The ordering is now enforced by happens-before edges rather than by wall-clock margins, so scheduling latency cannot flip the result. The tests keep their teeth: if parallel guardrails stopped overlapping the model call, the guardrail's wait would never be satisfied and the test would fail on the timeout, and the blocking-before-model and blocking-before-parallel checks still fail if a blocking guardrail stopped completing first.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
The flaky wall-clock assertion is a real problem, and replacing it with event synchronization is the right direction. However, both updated tests can still pass if the model runs before the parallel guardrail starts: parallel_finished.is_set() is also false before the guardrail has begun, and the later guardrail sees model_called already set.
Please add a parallel_started event in both variants, set it at the start of the parallel guardrail, and have the tracked model await it before setting model_called and invoking the original model method. This creates a two-way rendezvous and proves that the guardrail is genuinely in flight without relying on timing. After that focused change and green checks, this should be ready for another review.
|
Your contribution here is included in #4187; thanks again for suggesting the change! |
2 participants
Summary
tests/test_guardrails.py::test_mixed_blocking_and_parallel_guardrailsand its streaming twin fail intermittently undermake tests-parallel, while passing reliably in isolation.Both tests proved that a
run_in_parallel=Trueinput guardrail overlaps the model call by samplingtime.time()around a fixedasyncio.sleep(MEDIUM_DELAY)in each guardrail, then assertingtimestamps["model_called"] <= timestamps["parallel_end"].That assertion has a zero-width margin: it only holds while the runner reaches the model within the ~30ms the parallel guardrail spends sleeping. Under pytest-xdist the workers saturate the CPU, the process gets descheduled, the guardrail's timer expires first, and the guardrail resumes before the model call is issued — so the test fails even though the runner behaved correctly and did schedule the model concurrently with the guardrail:
This is test timing-sensitivity, not a runtime bug.
run.pycreates the model task and the parallel guardrail task before gathering both, andrun_internal/run_loop.pystarts the parallel guardrail task before awaitingrun_single_turn_streamed; the overlap itself is correct.This PR drives the overlap with explicit
asyncio.Eventsynchronization instead. The parallel guardrail stays in flight until the model call setsmodel_called, and the model call records whether the blocking guardrail had already finished and whether the parallel guardrail was still running. Ordering is now enforced by happens-before edges rather than wall-clock margins, so scheduling latency cannot flip the result. This matches theasyncio.Eventrendezvous idiom already used elsewhere in this file, for exampletest_parallel_guardrail_trip_before_tool_execution_stops_streaming_turn.The tests keep their teeth. Forcing
run_in_parallel=Trueguardrails to run blocking (patchingrun.pyandrun_internal/run_loop.pyso every input guardrail lands insequential_guardrails) makes both tests fail, because the guardrail's wait for the model call is never satisfied andwait_forraises. The blocking-before-parallel and blocking-before-model checks are unchanged in intent and still fail if a blocking guardrail stops completing first.Test plan
uv run pytest tests/test_guardrails.py -q -n 8 -k mixed_blockingwhile saturating the machine with 24 CPU-bound processes: 2 failures across 21 iterations onmain, hitting both the streaming and non-streaming variant.TimeoutErrorfrom the guardrail rendezvous. Reverted afterwards.-p no:randomly) the streaming test takes 0.11s before and after, so the 1s timeout is never approached.bash .agents/skills/code-change-verification/scripts/run.sh— all commands passed (make format,make lint,make typecheck,make tests).Only
tests/test_guardrails.pyis touched; no runtime code changes.Issue number
N/A — flake found while running the suite; no existing issue.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR