test(postgresql): make the registry enumeration test observe the concurrent window - #421
Merged
Merged
Conversation
…urrent window enumerating_while_registrations_land_neither_throws_nor_tears raced the writer to the finish: it read in a `while (!writer.IsCompleted)` loop and then asserted `passes > 0`. When the pool scheduled the writer promptly it landed all 5,000 registrations before the first IsCompleted check, so the loop body never ran and the test asserted nothing about the registry -- green locally, and red in CI on the trailing `passes > 0` with `Shouldly.ShouldAssertException : passes` (#418, Postgres 15.3-alpine net9.0 job, passed on rerun with no code change). The concurrent window is now established by handshake rather than by scheduling luck. The writer signals after its first registration and the reader blocks on that before taking any pass; the reader then takes a fixed number of passes and signals when done; the writer, after registering its new keys, keeps re-registering them until that signal arrives. It never blocks, so the map is being actively written to for the whole of the reader's passes, and the vacuous `passes > 0` assertion is gone. The invariants the test exists to protect are unchanged: a read never sees fewer entries than were seeded and never sees a null. Re-registering existing keys does not move the final count, so `Count == seeded + added` still holds -- and still fails under the non-atomic read-modify-write setter this test guards against, where a stale-snapshot write during the churn can drop keys outright. Verified with 200 consecutive local runs, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The flake
NpgsqlTypeMappingRegistryTests.enumerating_while_registrations_land_neither_throws_nor_tearsfailed on the "Postgres postgres:15.3-alpine net9.0 Case Sensitive false" job of #418 withShouldly.ShouldAssertException : passes, and passed on rerun with no code change. It passes consistently locally.The race was in the test, not the product code. The reader ran
while (!writer.IsCompleted) { ...; passes++; }and then assertedpasses.ShouldBeGreaterThan(0). When the thread pool scheduled the writer promptly and it completed all 5,000 registrations before the main thread's firstIsCompletedevaluation, the loop body never ran,passesstayed 0, and the test failed — while asserting nothing whatsoever about the registry. Nothing about that outcome indicatesNpgsqlTypeMappingRegistrymisbehaved.The fix
The concurrent window is now established by handshake instead of by scheduling luck, using two
ManualResetEventSlims:writingHasStartedafter its first registration; the reader blocks on it before taking any pass, so no pass is taken before writes are landing.readerIsFinished(in afinally, so a failed assertion releases the writer rather than hanging the run). The writer, after its 5,000 new-key registrations, drops into a churn loop re-registering those same keys until that flag is set.The writer never blocks — it is assigning into the map for the entire duration of the reader's passes — so the overlap is guaranteed by construction. The vacuous
passes > 0assertion is gone.Invariants preserved
Count >= seededand no null entries, now taken a fixed number of times rather than a scheduling-dependent one.Count == seeded + added: churn re-registers existing keys, so it does not move the count. It also slightly strengthens the assertion — under the non-atomic read-modify-write setter this test guards against (weasel#406), a stale-snapshot write during churn can drop keys outright, not just lose new ones.Verification
🤖 Generated with Claude Code