memory/shared: eliminate flaky race in ConcurrentAccess test - #835
Open
ahmed0mousa wants to merge 1 commit into
Open
memory/shared: eliminate flaky race in ConcurrentAccess test#835ahmed0mousa wants to merge 1 commit into
ahmed0mousa wants to merge 1 commit into
Conversation
ahmed0mousa
requested review from
LittleHuba,
bemerybmw,
castler,
crimson11,
hoe-jo and
limdor
as code owners
August 4, 2026 14:15
LittleHuba
requested changes
Aug 4, 2026
LittleHuba
left a comment
LittleHuba
left a comment
Contributor
There was a problem hiding this comment.
IMHO this will not remove the flakiness.
Scenario:
- Reader updates inserted_before
- Writer UpdatesKnownRegion + sets inserted
- Reader updates bounds
- Writer RemoveKnownRegion + sets inserted
- Reader updates inserted_after
Result: bounds.has_value() == true, but both inserted_before and inserted_after are false.
ahmed0mousa
force-pushed
the
ahmo_concurrent_access_flaky_test
branch
from
August 5, 2026 11:49
0555adc to
25a17e1
Compare
LittleHuba
reviewed
Aug 5, 2026
ahmed0mousa
force-pushed
the
ahmo_concurrent_access_flaky_test
branch
from
August 6, 2026 10:40
25a17e1 to
fbbbee0
Compare
LittleHuba
previously approved these changes
Aug 7, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 7, 2026
Replace the per-region shared_mutex approach (which serialized reader lookups against writer mutations for the same region, masking the very reader/writer overlap this test exists to exercise) with a fully lock-free design: - The test is split into two phases (insert all regions, then remove them all), joined by a single-use barrier. - The writer publishes progress via atomic start/finish counters per phase, updated with release semantics right before/after touching the map. - Readers take acquire loads of these counters immediately before and after their own unsynchronized GetBoundsFromAddress() call, and use the resulting happens-before relationship (not a timing assumption) to assert only when the expected outcome is provably determined. When the writer genuinely overlaps a lookup, no plausibility assertion is made, only that a found region resolves to the correct bounds.
ahmed0mousa
force-pushed
the
ahmo_concurrent_access_flaky_test
branch
from
August 7, 2026 12:45
fbbbee0 to
d6292f1
Compare
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
2 participants
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.
The test's inserted_ bookkeeping flag was updated by the writer as a
separate step from the actual UpdateKnownRegion()/RemoveKnownRegion()
call, racing against the reader's plausibility check. A reader could
be paused between its own steps long enough for the writer to fully
insert and remove the same region, making the flag read false on both
sides of a bounds-check that returned true.
Use one shared_mutex per region: the writer holds it exclusively for
its whole insert-or-remove transaction, and the reader holds it shared
across its whole before-flag/bounds-check/after-flag triplet. This
makes the region's state provably frozen during a reader's check while
different regions remain fully concurrent, so the map's lock-free
behaviour is still genuinely exercised.