test: a dump of notifications from two triggers needs a barrier between them - #73
Merged
Merged
Conversation
…en them
vocab_property went red in the nightly's valgrind shard with the first
notification in the dump being urn:E2's, not urn:E1's. The notification id in
that entry was :0002 - the second one the broker built, landing first.
The dump is in arrival order, and arrival order is only the trigger order if
notification N has LANDED before the request that triggers N+1 is sent. The
test created E1, E2 and E3 back to back and then dumped: E1's POST was still
in flight (broker log: request .018, second request .035, both answered .048)
when E2's went out. ftClientWait exists for exactly this and its own docstring
names the failure; the test just did not use it. `sleep .2` before the dump
waits for the notifications to arrive but says nothing about the order they
arrive in.
Barriers after each of the three creates. Forced the race locally - valgrind
plus the broker pinned to two contended CPUs - and reproduced the nightly diff
byte for byte, 2 runs in 4; with the barriers, 6 of 6 green under the same
load.
Two more tests had the same shape - a single dump holding notifications from
two separate triggering requests, ordered by nothing:
- context_csub_notification: the csub's initial notification and the
notification the CSR registration fires (also dropping its `sleep .3`)
- csr_subscription_notify_ldjson: the same pair
A fan-out is not affected: ldSubscriptionNotify.c sends one subscription at a
time on the triggering thread and blocks for each response, so N subscriptions
matched by ONE request notify in subscription order. Only notifications from
different requests race.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016mp1jSeTQQsHBtnv5SLE5L
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.
vocab_propertywent red in the nightly's valgrind shard: the first notification in the dump wasurn:E2's, noturn:E1's. The notification id in that entry was:0002— the second one the broker built, landing first.The race
The dump is in arrival order, and arrival order equals trigger order only if notification N has landed before the request triggering N+1 is sent. The test created E1, E2 and E3 back to back and then dumped, so E1's POST was still in flight when E2's went out — visible in the captured broker log:
ftClientWaitexists for exactly this, and its own docstring names the failure (it is what fixedsubscription_type_star,json_propertyandcsr_subscription_csf); this test simply did not use it. Thesleep .2before the dump waits for the notifications to arrive — it says nothing about the order they arrive in.Verified against the unfixed test
Forced the race locally with valgrind plus the broker pinned to two contended CPUs:
Two more tests had the same shape
A single dump holding notifications from two separate triggering requests, ordered by nothing:
context_csub_notification— the csub's initial notification and the one the CSR registration fires (itssleep .3is dropped too)csr_subscription_notify_ldjson— the same pairA fan-out is not affected:
ldSubscriptionNotify.c:1201sends one subscription at a time on the triggering thread and blocks for each response, so N subscriptions matched by ONE request notify in subscription order. Only notifications from different requests race — which is why the other 7 candidates the scan turned up (subscription_notif_format,subscription_notify_pick_omit_lang,subscription_notify_batch_create, …) are correct as they stand.Test
corTest --tags notification,subscription,csr,csr-sub,ftclient→ 149/149 pass.🤖 Generated with Claude Code
https://claude.ai/code/session_016mp1jSeTQQsHBtnv5SLE5L