Fix EventColumnView._byKey unsafe publication; add sort tie-break tests#641
Open
jschick04 wants to merge 1 commit into
Open
Fix EventColumnView._byKey unsafe publication; add sort tie-break tests#641jschick04 wants to merge 1 commit into
jschick04 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses a potential unsafe-publication race in EventColumnView.ResolveByKey by changing how the lazily built _byKey map is published under concurrency, and adds focused regression tests that pin deterministic sort tie-break behavior in the column-direct sort kernel.
Changes:
- Publish
EventColumnView._byKeywithVolatile.Read+Interlocked.CompareExchangeto ensure safe visibility on weak-memory CPUs under concurrentResolveByKey. - Add a full-tie corpus test to ensure the final physical-index tie-break actively reorders even when the primary sort is descending.
- Add a grouped-chain test matrix that independently verifies group-order negation vs within-group-order negation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Unit/EventLogExpert.Runtime.Tests/LogTable/ColumnDirectSortKernelTests.cs | Adds two direct regression tests covering index tie-break and grouped-chain negation behavior. |
| src/EventLogExpert.Runtime/LogTable/EventColumnView.cs | Fixes unsafe publication of the lazily built _byKey dictionary via Volatile + Interlocked. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
What — Fixes an unsafe-publication data race in
EventColumnView._byKeyand adds two direct sort-kernel regression tests.Why —
ResolveByKeypublished the lazily-built_byKeydictionary via_byKey ??= BuildByKey()with no memory barrier — unlike every sibling M6 cache (TemplateFieldSchema,CombinedColumnView._index, the column store/pool) which useVolatile/Interlocked/Lock. Under concurrentResolveByKey, a reader could observe the non-null reference before the dictionary's internal writes are visible on a weak-memory CPU (ARM64) — yielding a wrong/absent result or a throw.Fix — Publish via
Volatile.Read+Interlocked.CompareExchange(ref _byKey, byKey, null)(mirrorsTemplateFieldSchema.GetOrBuildIndex).BuildByKeyis idempotent over the immutable order, so a benign build-race is harmless.Tests — grouped group/within-order negation (pins the two
-Math.Signsites independently of the AoS parity oracle); full-tie corpus with a reversed survivor input, so the ascending physical-index tie-break must actively reorder regardless of descending (mutation-verified: the test fails whenWithIndexTieBreakis broken).Validation — Full
EventLogExpert.Runtime.Tests: 1641/1641 pass. Surfaced by a post-release audit of the M6 columnar store; the race is latent today (ResolveByKeyhas no production caller yet — only parity tests).