Repository navigation
test(gateway): ordered delivery waits for room B's send, and for every send to settle - #6298
mybelovedstar123 wants to merge 1 commit into
Conversation
…y send to settle test_a_reply_waits_for_its_rooms_earlier_send_and_another_room_does_not checked room B's post as soon as the pass returned, but the pass waits for that worker only DELIVERY_PASS_WAIT_S (0.2 s here). On a loaded runner the post lands later, the check sees [], and the late send then posts into the next test's fake, which failed test_a_result_that_posts_nothing_is_not_held_behind_its_rooms_send too. The check now polls for the post. Each test's cleanup waits until nothing is delivering, before its patches are undone. Test-only; the gateway is unchanged. Fixes sonichi#6279 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi-dot
left a comment
There was a problem hiding this comment.
Reviewed exact head 7deb72ac8a0318b50d46d1d1a4690c1b8b07865c. No actionable findings; LGTM, non-blocking.
Checked the complete diff, test harness, and production delivery bookkeeping. The cleanup runs before patch teardown, and waiting for room B's post preserves the room-order assertions.
Validation:
python3 tests/gateway-ordered-result-delivery.test.py -v: 12 tests passed, including the four hash-seed subprocesses.- Forced
DELIVERY_PASS_WAIT_S = 0.0in memory and ran the two affected tests: the parent test reproduced both failures ([]instead of room B's post, then the leaked room-B post in the next test); HEAD passed both. review-checks.sh --diff: passed.
Worst-case disruption is additional test waiting or a cleanup failure for a stuck worker; production behavior is unchanged. This is focused Linux validation, not a full local suite or cross-platform run. CI was still running at review time, so merge readiness remains subject to completed required checks and the repository's approval/CLA gates.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
sonichi
left a comment
There was a problem hiding this comment.
Read 7deb72a (test-only, +9 -0). The room B check now polls up to 10 s for calls == [b1] instead of asserting the instant the pass returns, so a late post on a loaded runner no longer fails it. The assertFalse(second.is_alive()) line still proves the pass never waits on room A. The new cleanup waits until _DELIVERING and _DELIVERING_ROOMS are empty, read under their mutex. addCleanup runs last-in first-out, so it runs before mock.patch.stopall, and a straggling send posts into its own test's fake rather than the next one's. The room A release is in a finally, so the cleanup's 30 s wait only costs time on a real leak. No findings.
liususan091219
left a comment
There was a problem hiding this comment.
Approving 7deb72a (test-only, +9 -0, fixes #6279).
- Approve bar: @sonichi (owner) approved this exact head. No change request stands.
- What I checked: the room B assertion now polls
_until(... == [b1], 10)before the exact-equality check, andassertFalse(second.is_alive())still proves the pass does not wait on room A. The cleanup is registered aftermock.patch.stopall, so it runs first and waits on_DELIVERING/_DELIVERING_ROOMSunder_DELIVERING_MUTEX. A late send therefore posts into its own test's fake, not the next one's. The test still imports and drives the real gateway, with no copy of the logic. - Evidence: the body's forced
DELIVERY_PASS_WAIT_S=0.0run reproduces both CI failures on main and passes with the fix. @sonichi-dot reproduced that independently. - CI and CLA: 43 checks pass, 1 skipped.
license/clasuccess. No unresolved threads.
— Sutando (susans-MacBook-Pro)
Fixes #6279
Test-only; the gateway is unchanged. This implements the fix the issue suggests.
Problem
In
test_a_reply_waits_for_its_rooms_earlier_send_and_another_room_does_not, the test checkedfake.calls == [b1]as soon as_post_ready_resultsreturned. WithDELIVERY_PASS_WAIT_Spatched to 0.2 s, the pass waits for room B's worker only that long, then lets it "go on alone".On a loaded runner the post lands after the deadline. The check then sees
[], and the late send posts into the next test'sFakeGateway, becausesetUppatches a newgw._reqbefore the worker sends. That is what madetest_a_result_that_posts_nothing_is_not_held_behind_its_rooms_sendfail with a leading'task-slow-b1'.Change (
tests/gateway-ordered-result-delivery.test.py)_until(lambda: self.fake.calls == [b1], 10).assertFalse(second.is_alive()), which proves the pass doesn't wait on room A, is kept.Harnessregisters a cleanup that waits until nothing is delivering (_DELIVERINGand_DELIVERING_ROOMSempty, read under_DELIVERING_MUTEX) and fails if a delivery outlives its test. It is registered aftermock.patch.stopall, so it runs before the patches are undone.Evidence (WSL2)
The issue notes that forcing the pass deadline to 0.0 s reproduces both CI assertions every time. I ran each tree as committed, and again with
DELIVERY_PASS_WAIT_Spatched to0.0in a temporary copy of the test:The file takes longer, about 41 s locally against 29 s on main. The cleanup now waits for sends that used to run on into the next test.
hermetic-env-structuralpasses, and so do the static checks (ruff, py3.9 compat, everylint-*with--diff,review-checks.sh, src-map, agents-md-sync, workspace-leak).Author: discord username :: slovertop :: agent-assisted (Claude)