Harden StreamPage primitives and make cursor wire formats culture-invariant by michaelstaib · Pull Request #10 · michaelstaib/graphql-platform

michaelstaib · 2026-09-28T03:21:38Z

This hardens the StreamPage primitives and closes the culture bugs found in review. StreamPage<T> no longer carries lifecycle internals: the source owns priming, pages can only be built already primed (private constructors, CreatePrimedAsync for single pages, and an internal CreateForBatch used only by the batch pump, guarded by a reflection test), and EnumerateEntriesAsync is renamed to GetEntriesAsync. Every failure path now releases the source and the lifetime without masking the original exception. Each disposal runs in its own try, and disposal errors are attached to the original fault instead of replacing it. The batch pump throws when a key reappears after its page completed, instead of silently dropping those rows. Cursors are now culture-invariant: end cursors and the date and time key serializers format and parse with the invariant culture, so a cursor written under th-TH no longer decodes to the year 2569. That serializer bug also exists on main. Invariant-culture output is byte-identical, so existing cursors keep working.

await foreach (var entry in page.GetEntriesAsync())   // was EnumerateEntriesAsync
{
    var cursor = page.CreateCursor(entry);
}

Closes the Copilot threads on #1 (inline exceptions in StreamPageBuffer.cs:73 and StreamPage.cs:157), #2 (maxCursors: 0 still returning a cursor, GreenDonutStreamPageExtensions.cs:129), #3 (lifetime skipped when source disposal throws, StreamBatchPump.cs:87), #6 (disposal masking the stream fault, StreamBatchPump.cs:169 and StreamPageBuffer.cs:218) and #8 (no test for the priming read throwing, StreamBatchPump.cs:100). Known follow-up: one batch-pump test lost its mid-drain assertion when the test-only accessor was removed.

Stack: sits on #8 (mst/primitives-batch-priming, which now also carries #9).

Copilot

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Review effort: Lite
Findings: 2 · 2

Open (4)

Buffered rows bypass recorded source fault · New
Unexpected row key bypasses fault and release handling · New
Count failure can leak caller-supplied lifetime · New
Drain selector failure leaks page and leaves batch unfaulted · New

What changed in this PR

Hardens streaming page lifecycle management and makes cursor serialization culture-invariant.

Changes:

Restricts page construction to primed/internal factories and renames entry enumeration APIs.
Improves disposal, fault propagation, and batch key-order validation.
Uses invariant culture for temporal and end-cursor formats.

File	Description
src/GreenDonut/test/GreenDonut.Data.Tests/StreamPageTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/StreamPageCursorTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/StreamBatchPumpTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/ScriptedAsyncSourceTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/ScriptedAsyncSource.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/PageTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/OrderedDisposalTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/Cursors/Serializers/TimeOnlyCursorKeySerializerTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/Cursors/Serializers/DateTimeOffsetCursorKeySerializerTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/Cursors/Serializers/DateTimeCursorKeySerializerTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/Cursors/Serializers/DateOnlyCursorKeySerializerTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.Tests/Cursors/CursorParserTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/StreamRelativeCursorTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/StreamPagingTotalCountTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/StreamPagingHelperTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/StreamPagingFaultTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/SnapshotExtensions.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/EndCursorTests.cs	Updated as part of this pull request.
src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/CursorInteropTests.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Internal/ThrowHelper.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Extensions/StreamPageCursorExtensions.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Extensions/GreenDonutStreamPageExtensions.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Extensions/GreenDonutPageExtensions.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/TimeOnlyCursorKeySerializer.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/DateTimeOffsetCursorKeySerializer.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/DateTimeCursorKeySerializer.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/DateOnlyCursorKeySerializer.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data/Cursors/CursorFormatter.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/ValueCursorStreamPage.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/StreamPage.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/Internal/ThrowHelper.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/Internal/StreamPageSourceBase.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/Internal/StreamPagePump.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/Internal/StreamPageBuffer.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/Internal/StreamBatchPump.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/Internal/OrderedDisposal.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/Internal/IStreamPageSource.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/Internal/ElementProjectingSource.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.Primitives/ElementCursorStreamPage.cs	Updated as part of this pull request.
src/GreenDonut/src/GreenDonut.Data.EntityFramework/Extensions/StreamPagingQueryableExtensions.cs	Updated as part of this pull request.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

michaelstaib · 2026-09-28T08:27:06Z

                _fault = ExceptionDispatchInfo.Capture(ex);
-                await ReleaseCoreAsync().ConfigureAwait(false);
+
+                try
+                {
+                    await ReleaseCoreAsync().ConfigureAwait(false);


Agree. The fault is checked before a staged row is returned, so a sibling's next pull rethrows the original exception instead of yielding buffered rows; regression with a queued sibling row added. Follow-up PR (task hc-fork-1-dno.13).

michaelstaib · 2026-09-28T08:27:08Z

+        if (channel.Completed && !channel.Abandoned)
+        {
+            // the key's run already completed (a later key was seen, or the source reached its
+            // end) and it was not abandoned, so this row is a source ordering violation: it would
+            // land in a queue no page will ever read again. Fault exactly like a mid-stream source
+            // exception so the source and the lifetime are released and every later pull, for any
+            // key, rethrows. A disposal failure while releasing is attached to this fault instead
+            // of replacing it.
+            var fault = ThrowHelper.StreamBatchPump_SourceNotGroupedByKey(row.Key);


Agree. The unrequested-key violation goes through the same fault-and-release path as the ordering violation (fault recorded, source and lifetime released, every later pull rethrows). Follow-up PR (task hc-fork-1-dno.13).

michaelstaib · 2026-09-28T08:27:09Z

+            try
+            {
+                await enumerator.DisposeAsync().ConfigureAwait(false);
+            }
+            catch (Exception disposeException)
+            {
+                // the enumerator's own disposal failure is what the caller must observe; the
+                // lifetime is still released, with a failure releasing it attached instead of
+                // replacing the original failure.
+                try
+                {
+                    await DisposeLifetimeAsync(lifetime).ConfigureAwait(false);
+                }
+                catch (Exception lifetimeException)
+                {
+                    OrderedDisposal.Attach(disposeException, lifetimeException);


Agree. The lifetime release moves into a cleanup path that also covers the fallback CountAsync; the count exception stays primary with disposal failures attached. Follow-up PR (task hc-fork-1-o47.14).

michaelstaib · 2026-09-28T08:27:10Z

+    private static async ValueTask DrainAsync<TValue>(
+        StreamPage<TValue> page,
+        CancellationToken cancellationToken)
+    {
+        await foreach (var _ in page.GetEntriesAsync(cancellationToken).ConfigureAwait(false))
+        {
+            // draining only for its buffering side effect; the entries themselves are unused here.
+        }
    }


Agree. DrainAsync catches a selector/drain failure, disposes the drained page so its key releases its count, attaches cleanup failures and rethrows the original. Follow-up PR (task hc-fork-1-dno.13).

…s for stream pumps

…med page

…ellation

…urce

…-SE round-trip test

…disposal faults

…ge factory surface

…ulture round-trip test signature

Copilot AI lite review requested due to automatic review settings September 28, 2026 03:21

github-actions Bot added the 🌶️ green donut label Sep 28, 2026

Copilot started reviewing on behalf of michaelstaib September 28, 2026 03:22 View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Base automatically changed from mst/cursors-async-helpers to mst/primitives-batch-priming September 28, 2026 08:34

michaelstaib mentioned this pull request Sep 28, 2026

Prime only the first batch row and report 0 for an empty StreamPage #8

Merged

michaelstaib force-pushed the mst/primitives-fault-cleanup branch from ccbe696 to 6ef2248 Compare September 28, 2026 08:39

michaelstaib mentioned this pull request Sep 28, 2026

Add ToBatchStreamPageAsync and harden the EF streaming paging paths #12

Merged

michaelstaib added this pull request to stack #13 September 28, 2026 14:28

Base automatically changed from mst/primitives-batch-priming to mst/stream-v2 September 28, 2026 14:41

michaelstaib added 17 commits September 28, 2026 16:41

Throw when a batch source produces a row for an already-completed key

3c1b934

Give IStreamPageSource one entries loop and derive values from it

a3597ed

Route two inline Primitives exceptions through ThrowHelper

10d8515

Preserve the original fault when a stream pump fails to dispose

7e8b764

Let scripted async test fakes throw on dispose

da6f191

Add fault-preserving disposal, priming-fault, and atomic-factory test…

66ba419

…s for stream pumps

Defer stream pump source disposal to release and dispose a failed pri…

9a48697

…med page

Extend stream page and pump fault tests for release ordering and canc…

aec778c

…ellation

Remove lifecycle internals from StreamPage and move priming to its so…

4386f13

…urce

Format end cursor page info culture-invariantly

ef9f783

Route end cursor buffer errors through ThrowHelper and tighten the sv…

62fe380

…-SE round-trip test

Restrict stream page construction and preserve disposal faults

6a5002a

Seal stream page construction behind primed factories and never mask …

43673da

…disposal faults

Format and parse cursor date and time keys culture-invariantly

1833f8d

Add culture round-trip tests for date and time cursor key serializers

d1b444e

Remove the test-only Source accessor from StreamPage and guard the pa…

d51a0b6

…ge factory surface

Add EF stream paging fault-path tests for priming and empty-row disposal

2d60f02

michaelstaib force-pushed the mst/primitives-fault-cleanup branch from 6ef2248 to 2d60f02 Compare September 28, 2026 14:41

Wrap long lines and name boolean arguments in the stream page primitives

90a6ce2

michaelstaib added 8 commits September 28, 2026 19:44

Wrap long lines in stream page tests

a02981e

Move trailing ConfigureAwait calls onto their own line and wrap the c…

417b52f

…ulture round-trip test signature

Split remaining multi-argument continuation lines in stream page tests

38c9246

Trim narrative comments to their contract sentences

4d8a1a1

Restore the page completion contract in the stream page pump release doc

05366ab

Wrap CreatePrimedAsync calls one argument per line

dc152d9

Trim narrative comments and XML docs to their contract sentences

3c67825

Merge mst/stream-v2 into mst/primitives-fault-cleanup

734b2c5

michaelstaib merged commit 4684132 into mst/stream-v2 Sep 29, 2026
1 check passed

Navigation Menu

Repository navigation

Harden StreamPage primitives and make cursor wire formats culture-invariant - #10

Harden StreamPage primitives and make cursor wire formats culture-invariant#10
michaelstaib merged 26 commits into
mst/stream-v2from
mst/primitives-fault-cleanup

michaelstaib commented Sep 28, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

michaelstaib Sep 28, 2026

Uh oh!

michaelstaib Sep 28, 2026

Uh oh!

michaelstaib Sep 28, 2026

Uh oh!

michaelstaib Sep 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

michaelstaib commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

michaelstaib Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

michaelstaib Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

michaelstaib Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

michaelstaib Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

michaelstaib commented Sep 28, 2026 •

edited

Loading