Rebuild the ordered view on same-generation content-version re-resolutions - #680
Conversation
…ix stale published order
…e requested scope
…fix stale published order
…ithout publishing out-of-range rows
There was a problem hiding this comment.
Pull request overview
Fixes stale ordered-view ordering/filtering and potential out-of-range faults when a log is re-resolved in-place (same generation, higher ContentVersion) by ensuring the ordered-view rebuilds appropriately for replace scenarios (including grow/shrink) and by hardening rebuild/catch-up logic around changing reader counts.
Changes:
- Thread replacement provenance (
isReplace) through shadow effects → writer → state, and trigger rebuilds for same-generation content replacements (including grow-replace vs append differentiation). - Add shrink-replace quarantine via
LiveIndexInvalidatedand clamp rebuild loops to reader counts to avoid out-of-range access during row-removing re-resolutions. - Add
ReconcileLogStaleOrderTestscovering same-count/grow/shrink replacements, in-flight rebuild supersession, and concurrent activity during shrink rebuild.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/EventLogExpert.Runtime.Tests/LogTable/OrderedView/ReconcileLogStaleOrderTests.cs | Adds regression tests validating rebuild signaling and correct republishing across replace scenarios (same-count/grow/shrink) including concurrency windows. |
| src/EventLogExpert.Runtime/LogTable/OrderedView/OrderedViewWriter.cs | Routes replace provenance into reconciliation, adds rebuild forcing/latching, and introduces invalidation retry behavior during shrink-replace rebuild windows. |
| src/EventLogExpert.Runtime/LogTable/OrderedView/OrderedViewState.cs | Detects content replacements vs appends, updates coverage accordingly (including shrink), quarantines live index on shrink, and clamps rebuild iteration to reader counts. |
| src/EventLogExpert.Runtime/LogTable/OrderedView/OrderedViewShadowEffects.cs | Passes isReplace correctly based on ingest/load modes when enqueueing reconcile commands. |
| src/EventLogExpert.Runtime/LogTable/OrderedView/OrderedViewScopeState.cs | Adds SetCoverage to support coverage resets on content replacement (vs monotonic advance). |
Suppressed comments (1)
src/EventLogExpert.Runtime/LogTable/OrderedView/OrderedViewWriter.cs:562
PublishNow()currently bails out only whenLiveIndexInvalidatedis set. When a replace arrives while a rebuild is already in-flight (_pendingRebuilds > 0), the writer sets_replaceAwaitingRebuild = truebut does not prevent publishing; subsequent appends/flushes can still callPublishNow()and republish a snapshot that is known to be stale until the forced rebuild completes.
private void PublishNow()
{
if (_state.LiveIndexInvalidated) { return; }
_state.Publish();
_dirty = false;
_sincePublish = 0;
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…t and drain-raise races
…ace cannot leave stale order
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/EventLogExpert.Runtime/LogTable/OrderedView/OrderedViewWriter.cs:311
- When a replace arrives during an in-flight rebuild, this path calls SupersedeInFlight(), which guarantees the current build’s adopt will be dropped as stale (generation mismatch). Letting the build continue is therefore wasted work and prolongs the time until the corrective rebuild can run (increasing the window where the ordered view can remain stale). Consider canceling the running build here, consistent with the restamp path (OrderedViewWriter.cs:517-519).
if (_pendingRebuilds > 0)
{
_replaceAwaitingRebuild = true;
_state.SupersedeInFlight();
}
tests/Unit/EventLogExpert.Runtime.Tests/LogTable/OrderedView/OrderedViewWriterFaultStateTests.cs:358
- This early
return;is redundant (the remaining code is only a local function declaration/comment) and can be confusing in a test because it looks like the test exits early. Removing it keeps the control flow straightforward while still allowing the local function to remain at the end of the method.
return;
Summary
Fixes a family of bugs where a same-generation, higher-
ContentVersionre-resolution of a log (aReplace/LoadEvents, i.e. the raw store is rebuilt in place rather than appended) left the published ordered-view order and filter stale, and for row-removing re-resolutions could crash on out-of-range rows.The ordered-view writer only re-sorted on an append (which grows coverage). A re-resolution that keeps the same generation but changes content was admitted into
_latestReadersyet never triggered a re-sort, so the published snapshot kept the previous ordering. This lands as focused commits, each with its own review, sharing one branch and PR.Commits
433cc14fsame-count content-version replace - a same-count, higher-CV reader is unambiguously a replace (an append always grows count); signal a rebuild via arequiresRebuildout-param, with a_replaceAwaitingRebuildlatch +SupersedeInFlightfor a replace that races an in-flight rebuild.7ff91f73replace of a log entering the requested scope (S-1) - a log being replaced while entering a new view's scope is gated on the requested scope, not just the adopted scope.cb409aafgrow replace - an append and a grow-replace are indistinguishable by count+CV, so replacement provenance (isReplace) is threaded from the shadow effects through the writer command into the state; a grow-replace rebuilds, a live-tail append does not (no perf regression on the hot path).fa454c0ashrink replace - admits a previously-dropped row-removing re-resolution and makes it crash-safe: a single_liveIndexInvalidatedflag quarantines both the live-insert loop and publishing until the shrunk index is adopted; coverage is decreased; build + catch-up loops clamp to the reader count; shrink-to-zero is handled; the rebuild routing latches only on an actually-pending build and otherwise force-rebuilds (bounded).ed7375bftest hardening - determinizes a pre-existing racy fault-state test (ATailReplayThatThrows...) that this PR surfaced under CI load: a gate forces the intended tail-replay path, and a bounded wait replaces an eager post-drain read. Test-only.9bfd0de6grow-replace via the coalesce/seed path - closes the last staleness gap: a coalesced view-request seed cannot observe replacement provenance, so any coalesced build that adopts now re-derives the order with a corrective rebuild (a grow-replace can no longer leave prior rows stale).Testing
ReconcileLogStaleOrderTestscovering same-count, S-1, grow (replace vs append), shrink, shrink-to-zero, a gated concurrency harness (no fault when publishing/appending during the shrink rebuild window), and the coalesced grow-replace corrective rebuild.EventLogExpert.Runtime.Tests: 2446 passed, 0 failed. Build clean.