Skip to content

Handle out-of-order split-include rows to prevent child collection loss under concurrent inserts#38590

Draft
AndriySvyryd with Copilot wants to merge 9 commits into
mainfrom
copilot/issue-478-fix-concurrent-insert-issue
Draft

Handle out-of-order split-include rows to prevent child collection loss under concurrent inserts#38590
AndriySvyryd with Copilot wants to merge 9 commits into
mainfrom
copilot/issue-478-fix-concurrent-insert-issue

Conversation

Copilot AI commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

AsSplitQuery collection includes can drop valid children when a concurrent insert introduces unrelated parent/child rows between split queries. The current stitching path stops at the first non-matching child row, even when matching rows for the current parent still appear later in the ordered stream.

  • Materialization fix (split include path)

    • Updated split include population (PopulateSplitIncludeCollection / async variant) to distinguish:
      • next-parent boundary (should stop and hand off), vs.
      • out-of-order unrelated row caused by concurrent insert (should skip and continue).
    • Sort direction is now detected at compile time in SelectExpression.GetParentIdentifierSortOrder: checks whether the leading ORDER BY columns exactly match the parent identifier (PK) columns and returns 1 (ascending), -1 (descending), or null (unknown/non-PK order — skipping disabled).
    • ParentIdentifierSortOrder is stored in RelationalSplitCollectionShaperExpression and passed as a compile-time constant into PopulateSplitIncludeCollection at code generation time, avoiding the flawed runtime inference approach.
    • Added comparable-identifier fallback logic (TryCompareIdentifiers) to safely detect relative ordering without changing key equality semantics.
  • Regression coverage

    • Added a relational ad-hoc split-query test reproducing the issue pattern:
      • load parents with Include(...).AsSplitQuery().OrderByDescending(...)
      • inject concurrent unrelated entity+children during materialization
      • assert existing parents still get their full child collections.
if (!CompareIdentifiers(identifierValueComparers, parentIdentifier, currentChildIdentifier))
{
    if (ShouldSkipOutOfOrderChild(dataReaderContext, parentIdentifier, currentChildIdentifier))
    {
        dataReaderContext.HasNext = null; // consume unrelated row and keep scanning
        continue;
    }

    dataReaderContext.HasNext = true; // reached boundary for another parent
    return;
}

Copilot AI and others added 3 commits July 9, 2026 01:25
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix concurrent insert issue with AsSplitQuery in EF Core Handle out-of-order split-include rows to prevent child collection loss under concurrent inserts Jul 9, 2026
Copilot AI requested a review from AndriySvyryd July 9, 2026 01:34
@AndriySvyryd AndriySvyryd requested a review from Copilot July 9, 2026 17:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request addresses a split-query (AsSplitQuery) include materialization edge case where child rows can be skipped when concurrent inserts cause “out-of-order” rows to appear in the streamed results. The fix adds lightweight state to the split include data-reader context and adjusts the stitching loop to skip unrelated out-of-order rows while still detecting true next-parent boundaries. A regression test reproduces the concurrency pattern and asserts child collections are fully populated.

Changes:

  • Update split-include collection population (sync/async) to detect and skip out-of-order unrelated child rows instead of prematurely stopping.
  • Add per-reader state (PreviousParentIdentifier, inferred parent sort direction) to support the new boundary/skip decision.
  • Add an ad-hoc relational split-query regression test covering concurrent insertion during materialization.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
test/EFCore.Relational.Specification.Tests/Query/AdHocQuerySplittingQueryTestBase.cs Adds a regression test reproducing child collection loss under concurrent inserts with split include + descending order.
src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs Implements out-of-order row skip logic and sort-direction inference used by split include stitching.
src/EFCore.Relational/Query/Internal/SplitQueryDataReaderContext.cs Adds new per-reader fields used to infer/track identifier ordering during split include population.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI and others added 2 commits July 14, 2026 20:57
…runtime inference

Replace the flawed runtime sort-direction inference (UpdateParentIdentifierSortOrder)
with compile-time detection in SelectExpression.GetParentIdentifierSortOrder.

The runtime approach failed when ORDER BY was on a non-PK column (e.g.
OrderBy(l2 => -l2.Level1_Required_Id)): multiple parents with the same
sort-key value have PK values in arbitrary order, causing the skip logic to
wrongly discard valid child rows.

The new approach checks at query-compilation time whether the first N ORDER BY
expressions exactly match the N parent identifier columns (all in the same
direction). If yes, the sort order is deterministic and the skip-on-concurrent-
insert logic can safely fire. If no (non-PK order, mixed directions, or
insufficient orderings), ParentIdentifierSortOrder is null and no skipping occurs.

Changes:
- SelectExpression.cs: Add GetParentIdentifierSortOrder static helper
- SelectExpression.Helper.cs: Add ParentIdentifierSortOrder to SplitCollectionInfo;
  pass it through ClientProjectionRemappingExpressionVisitor
- RelationalSplitCollectionShaperExpression.cs: Add ParentIdentifierSortOrder
  property and optional constructor parameter; preserve in Update()
- ShaperProcessingExpressionVisitor.cs: Pass ParentIdentifierSortOrder constant
  in generated PopulateSplitIncludeCollection call
- ClientMethods.cs: Add parentIdentifierSortOrder parameter to
  PopulateSplitIncludeCollection/Async; set dataReaderContext field at entry;
  remove UpdateParentIdentifierSortOrder (no longer needed)
- SplitQueryDataReaderContext.cs: Remove PreviousParentIdentifier (no longer needed)

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
…rn for null isAscending, clarify doc comments

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
public void RemapNonEntityNullabilityMarker(Expression oldShaper, Expression newShaper, Expression newMarkerBinding)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Fix this file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The TryGetNonEntityNullabilityMarker and RemapNonEntityNullabilityMarker methods had been moved to SelectExpression.Helper.cs but the originals were left in SelectExpression.cs, causing a duplicate-member build error. Removed the duplicates from SelectExpression.cs in commit $(git rev-parse --short HEAD).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e806636. The TryGetNonEntityNullabilityMarker and RemapNonEntityNullabilityMarker methods were moved to SelectExpression.Helper.cs but the originals were left in SelectExpression.cs, causing a duplicate-member build error. Removed the duplicates from SelectExpression.cs.

…arker and RemapNonEntityNullabilityMarker methods moved to SelectExpression.Helper.cs

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concurrent insert can cause AsSplitQuery to fail to populate unrelated child collections

3 participants