GH-3399: wait on the fact the test asserts, not on a count of envelopes - #4179
Merged
Merged
Conversation
batched_handler_actually_executes failed in full-suite runs (2/3, and 3/3 in some
configurations) while passing 5/5 in isolation. Nothing was broken: probed immediate=0,
eventual=1 -- the batched handler runs, just after the tracked session released.
The wait was counting the wrong thing. WaitForExecutionOf<T>(count) counts ENVELOPES of a
type, deduped by envelope id, and under MultipleHandlerBehavior.Separated a batch produces
THREE ItemDeleted3399[] envelopes, not two -- the batch lands on its own execution queue and
is then relayed to each sticky handler queue. Measured:
ExecutionFinished env=...5310 dest=local://coretests.bugs.itemdeleted3399/ <- relay source
ExecutionFinished env=...b332 dest=local://coretests.bugs.otherdeletedhandler3399/ <- sibling
Received env=...a676 dest=local://coretests.bugs.telemetryhandler3399/ <- never ran
The first two satisfy a count of 2 between them, so the session was released while the
handler under assertion had only been received.
This is the second time this test has been fixed by making the wait stricter, and the second
time the wait still did not name the thing being asserted: GH-4167 replaced
WaitForMessageToBeReceivedAt (released at receipt) with the execution count. A count of
executions cannot express "TelemetryHandler3399 ran", so stop counting -- gate on the
counter itself via an ITrackedCondition.
Surfaced by JasperFx 2.57.1 shifting timing, exactly as #714 did for GH-4167. Both
times a JasperFx change stopped masking this race rather than causing it; 2.57.1 and
Wolverine 6.30.3 are both clean. Verified by holding the JasperFx pin as the only variable.
Full CoreTests 3/3 green at 2666 (previously 0/3 or 1/3), Wolverine.Http.Tests 997.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test-only. No product change.
Symptom
CoreTests.Bugs.Bug_3399_batched_message_separated_handler_codegen.batched_handler_actually_executesfailed in full-suite runs — 2/3, and 3/3 in some configurations — while passing 5/5 in isolation. That combination is why CI stayed green.Nothing was broken
Probed at the moment the tracked session completes, then polled:
immediate=0 eventual=1. The batched handler runs; the test was reading its static counter too early. Generated code for all four Bug_3399 chains is byte-identical across the builds involved.The wait was counting the wrong thing
WaitForExecutionOf<T>(count)counts envelopes of a type, deduped by envelope id. UnderMultipleHandlerBehavior.Separateda single batch produces threeItemDeleted3399[]envelopes, not two — the batch lands on its own execution queue and is then relayed to each sticky handler queue. Measured fromsession.AllRecordsInOrder():The first two satisfy a count of 2 between them, so the session was released while the handler the assertion depends on had only been received.
The fix
Stop counting. Gate on the observable being asserted, via an
ITrackedConditiononTelemetryHandler3399.Batched > 0.This is the second time this test has been fixed by tightening its wait, and the second time the wait still did not name what it asserts — GH-4167 replaced
WaitForMessageToBeReceivedAt(released at receipt) with the execution count. A count of executions cannot express "TelemetryHandler3399 ran", so the test no longer tries to express it that way. The comment now records the measured envelope trace so the next person doesn't have to rediscover it.Why it surfaced now
JasperFx 2.57.1 shifted timing, exactly as #714 did for GH-4167. Both times a JasperFx change stopped masking this race rather than causing it — see the
BlockAllowSynchronousContinuationshistory on GH-4167, where this same test was one of three that surfaced.Confirmed by holding the JasperFx pin as the only variable, three full runs per configuration:
JasperFx 2.57.1 and Wolverine 6.30.3 are both clean. An initial bisect that varied the priming change and the JasperFx pin together pointed at #4178; isolating the pin corrected that.
Verification
Full CoreTests 3/3 green at 2666 (previously 0/3 or 1/3), Wolverine.Http.Tests 997. Three runs rather than one, because a ⅔ failure rate is precisely what a single green run fails to detect.
🤖 Generated with Claude Code