Added Define.TaskSeqField for lazy async collection streaming support (IAsyncEnumerable) - #607
Merged
Merged
Conversation
This was referenced Sep 18, 2026
A field resolved from `IAsyncEnumerable<'T>` is enumerated into a list without directives, delivered as a whole with `@defer`, and streamed item by item with `@stream`. Streaming pulls the sequence lazily, cancels the enumeration when the subscriber disposes, and reports an enumeration error as a deferred error after the items already produced, so buffered items and sibling deferred streams are not lost. `StreamBatching` groups streamed items into batches of a fixed size or of a size computed from the sequence. The `preferredBatchSize` argument of `@stream` takes precedence. Azure `AsyncPageable<T>` does not expose its page size, so tests cover both a plain pageable and one that keeps the page size hint. The `graphql-transport-ws` middleware now sends deferred and streamed payloads immediately with `path` and `hasNext`, followed by a final `hasNext: false` payload, instead of after a fixed 5 second delay. It no longer casts payloads to a dictionary and no longer drops initial payload errors. `SubscriptionExecutionResult.Data` became `obj Skippable` and the record got `Path` and `HasNext`. `FSharp.Data.GraphQL.Shared` references `Microsoft.Bcl.AsyncInterfaces` for `netstandard2.0`. The Star Wars sample got a `Human.friendsStream` field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`BufferedStreamOptions.Interval` and `BufferedStreamOptions.PreferredBatchSize` are now `int voption`, so the batch size computed for a `Define.TaskSeqField` sequence is used without converting between `option` and `voption`. The `@stream` planning and buffering code and the stream event filtering follow. `Define.Input` still takes the default value of a `Nullable IntType` argument as `int option`. The optional callbacks of the `TestObserver` and `SuspendingAsyncEnumerable` test helpers are struct optional parameters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tests checking that disposing a subscription stops the enumeration blocked the test thread with `Thread.Sleep` and `ManualResetEventSlim.Wait`. They now return `Task`, await `TaskCompletionSource` signals through the new `waitForTask` helper, which fails the test with a message on timeout, and wait with `Task.Delay`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y, lazy batching Fixes the three review comments on PR #598 (commit 764cc07): - An enumeration failure of a streamed TaskSeqField could overtake an earlier item that was still resolving asynchronously, because the failure was merged as an immediately-completing observable alongside still-running item resolutions. `Observable.ofAsyncEnumerableResolved` now awaits every resolution started before the failure before emitting it, so it always arrives last. - The same function bounds how many items are pulled from the source and resolved at the same time to `maxConcurrency`, a new optional parameter on `Define.TaskSeqField` (default `Environment.ProcessorCount`), so a fast or infinite source can no longer accumulate unbounded resolver work while streaming. - `StreamBatching.FromSource`'s callback ran whenever a TaskSeqField resolver was wrapped, so it also ran for ordinary and `@defer` queries. `IAsyncEnumerableFieldValue.GetPreferredBatchSize` now computes it lazily, only when `streamed` needs it: for a `@stream` query that does not itself supply `preferredBatchSize`. `Resolve.TaskSeq` now carries a `TaskSeqStreamingOptions` record (batching policy + max concurrency) instead of a bare `StreamBatchingPolicy`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…a, subscription race Fixes the five review comments on PR #598 (commit faaacb9): - `ofAsyncEnumerable`, `ofAsyncEnumerableResolved` and `AsyncEnumerableExtensions.toArrayAsync` acquired their enumerator before the try block, so a source throwing from `GetAsyncEnumerator` bypassed the failure handling and faulted the returned Task directly. For `ofAsyncEnumerableResolved` that meant `OnError` on the merged deferred observable of the whole query instead of `DeferredErrors` for just this field, which can drop sibling deferred results and the final `hasNext: false` payload. Acquisition now happens inside the try, and a shared `disposeEnumerator` helper also routes a throwing `DisposeAsync` through the same failure path, preferring an earlier enumeration failure if there was one. - `sendSubscriptionResponseOutput` discarded the partial data the executor can return alongside `SubscriptionErrors` and sent `data: null`; it now forwards both. `applyPlanExecutionResult`'s `Direct` branch dropped the execution errors the HTTP handler forwards; it now sends them too, with a warning log matching the other branches. - `addClientSubscription` subscribed before registering the subscription id, so a deferred observable completing synchronously ran its removal callback while the id was still absent; the helper then added the already-completed subscription, stranding the id permanently (a later `Subscribe` with the same id was rejected as already taken). A `SingleAssignmentDisposable` is now registered first and assigned after subscribing, so synchronous completion can find and remove it; assigning `Disposable` on an already-disposed instance disposes the assigned value too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TaskSeqFieldTests.fs and ObservableExtensionsTests.fs had each grown their own copies of the same async-enumerable test sources: ThrowingAsyncEnumerable, an "endless numbers" source recording pulls and disposal, "one item then the enumeration fails", "one item then DisposeAsync throws", the synchronous asyncItems/asyncRange sequence, and a delay helper differing only in argument order. Moved all of them into the shared, auto-opened Helpers module next to the existing SuspendingAsyncEnumerable and waitForTask, and updated both test files to use the shared versions instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ailure, placeholder cleanup ObservableExtensions.fs: ofAsyncEnumerableResolved kept every asynchronously resolved item's Task in a ResizeArray until the whole source ended, so a long or infinite @stream source retained one task per delivered item despite maxConcurrency. It also released a resolution's concurrency slot only after both awaiting it and emitting succeeded, so a failed resolution or an observer throwing while a result was delivered left the slot held forever; with maxConcurrency = 1 this deadlocked the enumeration. Replaced the task list with an in-flight counter plus a TaskCompletionSource signalled once enumeration has ended and every started resolution has settled, and moved the slot release into a finally so it always runs. A resolution failure now stops pulling further items and is delivered through onFailure the same way an enumeration failure is, after every resolution already started settles. GraphQLWebsocketMiddleware.fs: addClientSubscription registered its SingleAssignmentDisposable placeholder before calling Subscribe, so a stream throwing synchronously from Subscribe left the placeholder registered forever, permanently occupying the subscription id. Subscribe is now wrapped so a synchronous failure removes the placeholder (a no-op if a synchronous completion already did) before rethrowing for the existing per-message error handling to report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…active's own subscription teardown The previous commit's new test subscribed a raw IObserver<'T> whose OnNext throws to reproduce the concurrency-slot leak Copilot flagged. That part of the fix is correct (confirmed by an isolated repro run 300 times without System.Reactive: the semaphore slot is always released via the finally block). But through the real System.Reactive Subscribe(IObserver<'T>) call used by the operator, and confirmed with another isolated repro against the actual System.Reactive package, an observer's OnNext throwing makes Rx tear the subscription down itself: it disposes the subscription (cancelling the enumeration's token) before rethrowing. ofAsyncEnumerableResolved correctly treats that as "nobody is listening anymore" and skips both the onFailure delivery and OnCompleted, exactly as it does when disposed for any other reason. The test's assertion that OnCompleted still fires and onFailure still gets delivered was therefore wrong, and it hung for the test's full timeout on CI (Timeout waiting for OnCompleted), failing the build on all three OS runners. Replaced it with a test that doesn't depend on OnCompleted: it uses a source that signals a TaskCompletionSource from DisposeAsync, and waits (bounded) for that instead, which still proves the enumeration reaches disposal without hanging on the concurrency slot. Corrected the doc comment and RELEASE_NOTES.md, which both overstated that onFailure is delivered in this case. Also split the RELEASE_NOTES.md bullet about graphql-transport-ws discarding partial data into its two separate, more precise fixes (subscription data vs. Direct-result errors), per Copilot's fourth review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ofAsyncEnumerableResolved only checked failed()/cancellation at the top of the while loop. With maxConcurrency = 1 the loop parks in slots.WaitAsync while the single in-flight resolution runs; when that resolution fails, its finally releases the slot and the loop resumes straight into MoveNextAsync, so one more item is pulled and, if it resolves synchronously, emitted before the failure that already happened - contradicting the doc comment's claim that a failed resolution "stops the enumeration". Rechecked both conditions right after acquiring the slot, releasing it and stopping without pulling when either is set. Confirmed the race and the fix with an isolated fsi repro of the operator (100/100 runs emitted [2; -1] before this change, [-1] after), since the project's test suite can't be run standalone here (see the third-review commit's message on the AspNetCore-only compiler bug being addressed on struct-optional-params). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d item-error semantics ofAsyncEnumerableResolved only rechecked the stop conditions right after acquiring a concurrency slot, not after the following MoveNextAsync. A background resolution can fail while that move is still suspended; once it completes, the code went straight to resolving the item it produced, so with maxConcurrency > 1 an item pulled after a failure could still be resolved and, if synchronous, emitted before it. Factored the two checks into one `stopped ()` predicate and used it after both awaits. Confirmed the race and the fix with the same isolated fsi repro approach as the fifth review's fix (100/100 runs emitted the extra item before this change, 0/100 after). Also addressed the review's other thread: it read "a failed item resolution stops the enumeration" (from the doc comment, RELEASE_NOTES.md and the PR description) as meaning any per-item GraphQL error should end the stream, since resolveStreamedItem wraps every ResolverResult, including Error, into a plain StreamedItem. That wording described the AsyncVal computation itself throwing (a bug in the resolution plumbing, treated like a source failure), not an ordinary resolver error, which executeResolvers already turns into a normal, non-throwing ResolverResult.Error value - the same value @stream on an ordinary list turns into that item's DeferredErrors while continuing to stream the rest (see DeferredTests."Resolver list error"). Kept that behavior, since diverging from ordinary lists here would be surprising and isn't what GraphQL's per-field error semantics call for, and reworded the doc comment, RELEASE_NOTES.md and docs/type-system.md to make the distinction explicit. Added an execution-level regression test that pins the intended behavior: an item's own field error is delivered on its own path and the following items keep streaming. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e after Direct/RequestError
GraphQLWebsocketMiddleware.fs sent a batch of streamed items (grouped by the
field's batching policy or the query's preferredBatchSize) as one payload
addressed by a path ending in the list of the batch's own indices, such as
["numbers", [0, 1]]. No graphql-transport-ws client can merge that into the
response tree: a batch isn't addressable by any single index, only its
individual items are. Added IncrementalPayloadSplitting, a small pure module
that recognises such a path and splits the batch into one payload per item,
addressed the same way a field that streams one item at a time already is
(a one-element data array at a path ending in that item's own index), in
the batch's own order. Each item's own errors are attributed by checking
which item's path they start with, since every error the engine attaches to
a batch already carries the full path of the specific item it came from -
no cross-message state is needed. This keeps the engine's batching (still
one buffered/merged event upstream) while making every item addressable on
the wire. Verified the splitting logic with an isolated fsi repro of the
algorithm (out-of-order batch, and a batch with one item's own field error)
before adding the xUnit test.
Also fixed graphql-transport-ws never sending complete after the single
next of a Direct (query/mutation) or RequestError result, which the
protocol requires ("Server dispatches the Complete message indicating that
the execution has completed" after "at most one Next message").
A newer incremental-delivery wire format (pending/incremental/completed,
matching graphql-js 17 and Apollo Client's GraphQL17Alpha9Handler) was
considered and is worth adopting, but requires a per-field completion
signal threaded through the engine's whole merged deferred/streamed/live
observable, which touches the ~25 pre-existing, TaskSeqField-unrelated
tests in DeferredTests.fs (exact payload positions and counts) that this PR
otherwise leaves alone. Tracked as follow-up work on a separate branch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…onventions
Request (validation) errors were sent over graphql-transport-ws as a Next
followed by Complete, which the protocol reserves for results; a client
reading it would see a successful, null-data result instead of the
operation being terminated by an error. Routed RequestError through the
terminal Error message instead, with no Complete after it (Direct still
gets Next + Complete: it is a real, if partly erroneous, result).
That required ServerMessage.Error and ServerRawPayload.ErrorMessages to
carry GQLProblemDetails list instead of NameValueLookup list, so the error
payload is a standard GraphQL error array as the protocol's ExecutionError
requires, rather than an arbitrary object. Replaced the one other Error call
site (the subscription catch-all's hand-built NameValueLookup, which had no
message field and so was not a valid GraphQL error either) with
GQLProblemDetails.Create.
Found while wiring this up: RawServerMessageConverter.Write serialized the
ErrorMessages and CustomResponse payloads without a preceding
WritePropertyName ("payload"), unlike the ExecutionResult branch.
Utf8JsonWriter throws when a value is written where a property name is
expected, so every "error" message, and every "pong" carrying a payload,
failed to serialize - nothing covered those two write paths. Fixed, and
added two regression tests; verified the throw and the fix against a bare
Utf8JsonWriter first.
Made IncrementalPayloadSplitting internal instead of public - it was public
only so the test project could reach it, which committed obj list paths and
tuple results to the package's supported surface for no reason. Exposed it
to the test assembly the same way Shared and Server already do
(InternalsVisibleTo), rather than the transport implementation detail. Also
replaced the one list-append (@) this PR introduced with a list expression,
per the project's collection conventions.
Added IcedTasks to the test project only (no transitive dependency for
consumers) and rewrote SuspendingAsyncEnumerable.MoveNextAsync as a
valueTask CE instead of manually wrapping a Task in a ValueTask, per the
async conventions; verified the valueTask CE against the real IcedTasks
package before wiring it in.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ecution result, not a request error GQLResponseContent.RequestError was produced both before execution (validation, planning, variable coercion, a middleware, or the executor itself failing) and after it, whenever executeOperation's non-null root field failed and the error propagated to the root. The previous commit routed every RequestError through graphql-transport-ws's terminal Error message, which is wrong for the second case: per the spec, a response with a failed non-null root field is still an execution result (data is null, but present), not a request rejected before execution, so it must be sent as Next + Complete like any other result. The HTTP handler had the same conflation the other way: GQLResponse.RequestError omits data for both, so such a response was missing "data" entirely instead of carrying null. Represented the root failure as Direct (null, errs) instead of adding a new case: RequestError now means pre-execution only, and both transports already handle Direct correctly. Updated the six tests that asserted RequestError for this case (three in ExecutionTests.fs, one each in LazyEnumerationExceptionTests.fs and TaskSeqFieldTests.fs) to assert Direct with null data instead; the other 21 ensureRequestError call sites are genuine pre-execution validation/coercion/middleware failures and are unchanged. Documented the distinction on the two GQLResponseContent cases and reworded the middleware's comments and log messages to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…estError executeQueryOrMutation's final Error branch (added when the ninth review's fix made a failed non-null root field produce Direct(null, errs)) was also being reached by executeRootOperation's own getArgumentValues failure for inline (literal) root field arguments, since both errors flowed into the same collectFields-aggregated Result. That reclassified inline argument and input object coercion/validation failures as execution results with null data instead of RequestError, breaking InputObjectValidatorTests's "Execute handles validation of invalid inline input records with all fields" on CI. Inline argument coercion is now checked for every root field up front, mirroring Executor.eval's coerceVariables step for variables: if any root field's arguments fail to coerce, the whole request is rejected as RequestError before any resolver runs. Only a genuine resolver failure on a non-null root field now reaches the Direct(null, errs) branch. As a side effect, a mutation with an invalid literal argument on a later root field no longer executes the earlier root fields' resolvers first. Verified with a full solution build and the complete unit test suite locally (dotnet build FSharp.Data.GraphQL.slnx + dotnet test), matching CI's approach, instead of the previous partial per-project builds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The tenth Copilot review claimed Execution.collectItems' chunk branch omits a failed item's index from `indicies` while still reserving its slot in `data`, so GraphQLWebsocketMiddleware.splitBatch's List.map2 would throw on a batch mixing a successful and a failed item. It does not: both arms of collectItems' `merge` prepend the item's index (`box index :: indicies`), and only the Ok arm additionally writes into `data` - so `indicies` and `data` always end up exactly `chunk.Length` long, with a failed item's slot left null. List.map2 never sees mismatched lengths. Added two regression tests pinning this shape rather than changing behavior: one drives the real engine through a @stream query with Fixed batching where item 0's own field resolution fails and item 1 succeeds, asserting the single DeferredErrors event this produces (TaskSeqFieldTests.fs); the other exercises splitBatch directly with a null data slot (IncrementalPayloadSplittingTests.fs). Both pass. Also addressed the same review's suppressed comment: the "emits each item as soon as its fields are resolved" test's ordering assertion depended on the default maxConcurrency (Environment.ProcessorCount), which could fail on a single-CPU runner; maxConcurrency is now set explicitly on that test. Verified with a full solution build and the complete unit test suite locally, matching CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d AsyncEnumerable module Avoid allocating reference tuples in the per-item and per-batch stream collection paths in Execution.fs by switching to struct tuples. In ObservableExtensions.fs, switch every enumeration/resolution loop from task to backgroundTask so continuations never resume on a subscriber's or caller's synchronization context, rename AsyncEnumerableExtensions to AsyncEnumerable, and replace the ExceptionDispatchInfo round-trip with ex.Reraise(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
xperiandri
force-pushed
the
task-seq-field-core
branch
from
September 18, 2026 13:50
810ff5a to
46809e6
Compare
xperiandri
changed the title
TaskSeqField streaming support
xperiandri
changed the title
TaskSeqField streaming supportDefine.TaskSeqField for lazy async collection streaming support (IAsyncEnumerable)
Test Results 9 files 9 suites 13m 4s ⏱️ Results for commit 34023fb. ♻️ This comment has been updated with latest results. |
Copilot stopped reviewing on behalf of
xperiandri due to an error
September 18, 2026 14:18
Contributor
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Pull request overview
This PR adds first-class support for list fields resolved from IAsyncEnumerable<'T> (via Define.TaskSeqField) and updates incremental delivery over graphql-transport-ws to properly stream/defer with path + hasNext, while aligning error classification (request rejection vs execution result) and switching buffered stream options to voption.
Changes:
- Introduces
Define.TaskSeqField+ streaming options (StreamBatching,maxConcurrency) and supporting execution/planning logic. - Fixes
graphql-transport-wsincremental delivery: immediate incremental payloads, payload splitting for batched paths, correct request-error vs result handling, and serialization fixes. - Expands test coverage across execution, observables, websocket middleware, and serialization; updates docs and release notes.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/FSharp.Data.GraphQL.Tests/TaskSeqFieldTests.fs | New end-to-end tests for Define.TaskSeqField, batching, max concurrency, and error ordering. |
| tests/FSharp.Data.GraphQL.Tests/LazyEnumerationExceptionTests.fs | Aligns expectations for non-null root failures as Direct with data = null. |
| tests/FSharp.Data.GraphQL.Tests/Helpers.fs | Adds async-enumerable test helpers and switches observer callback optional arg to voption. |
| tests/FSharp.Data.GraphQL.Tests/Helpers and Extensions/ObservableExtensionsTests.fs | Adds tests for ofAsyncEnumerable / ofAsyncEnumerableResolved disposal, ordering, and failure behavior. |
| tests/FSharp.Data.GraphQL.Tests/FSharp.Data.GraphQL.Tests.fsproj | Adds packages and compiles new tests. |
| tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs | Updates root-error expectations and adds inline argument coercion guard regression coverage. |
| tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs | Updates buffered stream options initialization to voption. |
| tests/FSharp.Data.GraphQL.Tests/AspNetCore/SerializationTests.fs | Adds coverage for incremental payload JSON shape and error/pong payload serialization regressions. |
| tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalPayloadSplittingTests.fs | New tests for batch path detection and batch splitting semantics. |
| src/FSharp.Data.GraphQL.Shared/WebSockets.fs | Expands subscription payload model to support incremental delivery (path, hasNext) and standardizes error payloads. |
| src/FSharp.Data.GraphQL.Shared/TypeSystem.fs | Adds Resolve.TaskSeq, batching/concurrency options, and IAsyncEnumerableFieldValue wrapper. |
| src/FSharp.Data.GraphQL.Shared/Serialization/JsonConverters.fs | Fixes missing "payload" property name for error and pong messages. |
| src/FSharp.Data.GraphQL.Shared/SchemaDefinitionsExtensions.fs | Disallows resolve middleware for TaskSeqField. |
| src/FSharp.Data.GraphQL.Shared/SchemaDefinitions.fs | Adds Define.TaskSeqField overloads with batching and concurrency options. |
| src/FSharp.Data.GraphQL.Shared/FSharp.Data.GraphQL.Shared.fsproj | Adds Microsoft.Bcl.AsyncInterfaces dependency for netstandard2.0. |
| src/FSharp.Data.GraphQL.Server/Schema.fs | Converts buffered stream option defaults to option where required by directive arg definitions. |
| src/FSharp.Data.GraphQL.Server/Planning.fs | Switches stream directive parsing to voption path. |
| src/FSharp.Data.GraphQL.Server/ObservableExtensions.fs | Adds async-enumerable observable helpers and async enumerable draining utilities. |
| src/FSharp.Data.GraphQL.Server/IO.fs | Clarifies RequestError vs Direct semantics via comments. |
| src/FSharp.Data.GraphQL.Server/Execution.fs | Implements IAsyncEnumerable execution for direct + streamed results, batching, and pre-coercion gating for root fields. |
| src/FSharp.Data.GraphQL.Server/ErrorMessages.fs | Updates enumerable error message to mention async enumerables. |
| src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs | Implements incremental delivery (path/hasNext), correct request error routing, and batch splitting for client mergeability. |
| src/FSharp.Data.GraphQL.Server.AspNetCore/FSharp.Data.GraphQL.Server.AspNetCore.fsproj | Exposes internals to tests for incremental splitting test coverage. |
| samples/star-wars-api/star-wars-api.fsproj | Adds FSharp.Control.TaskSeq to support streaming sample. |
| samples/star-wars-api/Schema.fs | Adds friendsStream TaskSeqField demonstrating @stream. |
| docs/type-system.md | Documents Define.TaskSeqField, batching, and concurrency behavior. |
| README.md | Updates stream options snippet to voption. |
| Packages.props | Adds versions for Azure.Core, TaskSeq, IcedTasks, and Bcl.AsyncInterfaces. |
| RELEASE_NOTES.md | Documents breaking changes and new streaming/incremental delivery capabilities. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
xperiandri
force-pushed
the
task-seq-field-core
branch
from
September 18, 2026 15:05
f160c25 to
9a926ff
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A pending source move can hang after resolution failure, stream defaults are mishandled, and the diff includes work reserved for the stacked follow-up.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
src/FSharp.Data.GraphQL.Server/Execution.fs:303
ValueNonehere means only thatpreferredBatchSizewas omitted from the query AST, not that the schema has no configured default.DefaultWithBufferedStreamstores defaults on the directive definition, butgetStreamBufferModereads only explicitly supplied AST arguments. Consequently an omitted argument lets the field-level batching policy override the schema default, and configured default intervals are also dropped. Resolve the directive's effective argument values, including defaults, during planning before applying the field fallback.
src/FSharp.Data.GraphQL.Server/Execution.fs:548- This changes the response contract for every query and mutation, not only
TaskSeqField: inline argument coercion is moved ahead of all root execution, and a non-null root failure becomesDirect(null, errors). The PR description assigns response-contract changes to stacked PR #608, so move this behavior and its tests/release notes there, or revise the documented base/follow-up scope to match the actual stack.
src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs:256 - The incremental WebSocket transport work here (
path/hasNext, batch splitting, and related delivery changes) is explicitly excluded from this PR's review scope and assigned to stacked PR #608. This file also includes the follow-up's subscription-registration lifecycle fix. Please restack these WebSocket changes and their shared contracts/tests/release notes into #608, or update the PR scope and stack so reviewers are not evaluating the same work twice.
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Cleanup can mask resolution failures, and middleware coercion errors remain misclassified.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/FSharp.Data.GraphQL.Server/IO.fs:80
- The new contract says middleware rejection is a
RequestError, butObjectListFilterMiddleware.reportMiddlewarestill returnsGQLExecutionResult.Direct(..., null, errors)when its pre-execution inline-filter coercion fails (src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs:133-137). Over WebSocket this is therefore sent asnextpluscomplete, not the terminalerrorused for other pre-execution failures. ReturnGQLExecutionResult.Errorfrom that middleware path and add coverage for this request-error case.
src/FSharp.Data.GraphQL.Server/ObservableExtensions.fs:231 - If an item resolution fails and
DisposeAsyncthen throws while the stream is stopping,disposeEnumeratorputs the disposal exception intofailure. The fallback on line 231 selects that exception beforeresolutionFailure, so clients receive "Boom disposing" instead of the resolution error that actually stopped the stream. Track the pre-disposal enumeration failure separately: preserve a genuine enumeration failure, otherwise preferresolutionFailureover an exception raised only during cleanup.
src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs:218
- The stated review scope assigns WebSocket lifecycle, incremental payload, and response-contract work to stacked follow-up #608, but this hunk implements the synchronous-completion registration race fix and this PR also contains the related WebSocket delivery changes. Move the follow-up-only implementation, tests, and release notes to #608, or update the PR description and stack so this behavior is explicitly in scope for review.
// Registered before subscribing, so a stream that completes synchronously (from inside Subscribe) still
// finds the id when its onCompleted callback above runs; only then is it safe to remove and dispose it.
// Assigning Disposable on an already-disposed SingleAssignmentDisposable disposes the assigned value too.
let placeholder = new System.Reactive.Disposables.SingleAssignmentDisposable ()
subscriptions
|> GraphQLSubscriptionsManagement.addSubscription (id, placeholder, (fun _ -> ()))
try
placeholder.Disposable <- streamSource.Subscribe (observer)
with _ ->
// Nothing will ever complete this subscription now, so the id is freed here instead; a no-op if the
// synchronous completion above already removed it. Rethrown for the caller to report the failure.
subscriptions |> GraphQLSubscriptionsManagement.removeSubscription id
reraise ()
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Balanced
xperiandri
added a commit
that referenced
this pull request
Sep 18, 2026
…rt (`IAsyncEnumerable`) (#607) * Added `Define.TaskSeqField` with streaming of `IAsyncEnumerable` results A field resolved from `IAsyncEnumerable<'T>` is enumerated into a list without directives, delivered as a whole with `@defer`, and streamed item by item with `@stream`. Streaming pulls the sequence lazily, cancels the enumeration when the subscriber disposes, and reports an enumeration error as a deferred error after the items already produced, so buffered items and sibling deferred streams are not lost. `StreamBatching` groups streamed items into batches of a fixed size or of a size computed from the sequence. The `preferredBatchSize` argument of `@stream` takes precedence. Azure `AsyncPageable<T>` does not expose its page size, so tests cover both a plain pageable and one that keeps the page size hint. The `graphql-transport-ws` middleware now sends deferred and streamed payloads immediately with `path` and `hasNext`, followed by a final `hasNext: false` payload, instead of after a fixed 5 second delay. It no longer casts payloads to a dictionary and no longer drops initial payload errors. `SubscriptionExecutionResult.Data` became `obj Skippable` and the record got `Path` and `HasNext`. `FSharp.Data.GraphQL.Shared` references `Microsoft.Bcl.AsyncInterfaces` for `netstandard2.0`. The Star Wars sample got a `Human.friendsStream` field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Changed `BufferedStreamOptions` and stream helpers to use `voption` `BufferedStreamOptions.Interval` and `BufferedStreamOptions.PreferredBatchSize` are now `int voption`, so the batch size computed for a `Define.TaskSeqField` sequence is used without converting between `option` and `voption`. The `@stream` planning and buffering code and the stream event filtering follow. `Define.Input` still takes the default value of a `Nullable IntType` argument as `int option`. The optional callbacks of the `TestObserver` and `SuspendingAsyncEnumerable` test helpers are struct optional parameters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Rewrote subscription disposal tests as asynchronous `task` tests The tests checking that disposing a subscription stops the enumeration blocked the test thread with `Thread.Sleep` and `ManualResetEventSlim.Wait`. They now return `Task`, await `TaskCompletionSource` signals through the new `waitForTask` helper, which fails the test with a message on timeout, and wait with `Task.Delay`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Addressed Copilot review: ordered stream failures, bounded concurrency, lazy batching Fixes the three review comments on PR #598 (commit 764cc07): - An enumeration failure of a streamed TaskSeqField could overtake an earlier item that was still resolving asynchronously, because the failure was merged as an immediately-completing observable alongside still-running item resolutions. `Observable.ofAsyncEnumerableResolved` now awaits every resolution started before the failure before emitting it, so it always arrives last. - The same function bounds how many items are pulled from the source and resolved at the same time to `maxConcurrency`, a new optional parameter on `Define.TaskSeqField` (default `Environment.ProcessorCount`), so a fast or infinite source can no longer accumulate unbounded resolver work while streaming. - `StreamBatching.FromSource`'s callback ran whenever a TaskSeqField resolver was wrapped, so it also ran for ordinary and `@defer` queries. `IAsyncEnumerableFieldValue.GetPreferredBatchSize` now computes it lazily, only when `streamed` needs it: for a `@stream` query that does not itself supply `preferredBatchSize`. `Resolve.TaskSeq` now carries a `TaskSeqStreamingOptions` record (batching policy + max concurrency) instead of a bare `StreamBatchingPolicy`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Addressed second Copilot review: enumerator lifecycle, WS partial data, subscription race Fixes the five review comments on PR #598 (commit faaacb9): - `ofAsyncEnumerable`, `ofAsyncEnumerableResolved` and `AsyncEnumerableExtensions.toArrayAsync` acquired their enumerator before the try block, so a source throwing from `GetAsyncEnumerator` bypassed the failure handling and faulted the returned Task directly. For `ofAsyncEnumerableResolved` that meant `OnError` on the merged deferred observable of the whole query instead of `DeferredErrors` for just this field, which can drop sibling deferred results and the final `hasNext: false` payload. Acquisition now happens inside the try, and a shared `disposeEnumerator` helper also routes a throwing `DisposeAsync` through the same failure path, preferring an earlier enumeration failure if there was one. - `sendSubscriptionResponseOutput` discarded the partial data the executor can return alongside `SubscriptionErrors` and sent `data: null`; it now forwards both. `applyPlanExecutionResult`'s `Direct` branch dropped the execution errors the HTTP handler forwards; it now sends them too, with a warning log matching the other branches. - `addClientSubscription` subscribed before registering the subscription id, so a deferred observable completing synchronously ran its removal callback while the id was still absent; the helper then added the already-completed subscription, stranding the id permanently (a later `Subscribe` with the same id was rejected as already taken). A `SingleAssignmentDisposable` is now registered first and assigned after subscribing, so synchronous completion can find and remove it; assigning `Disposable` on an already-disposed instance disposes the assigned value too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Moved the shared TaskSeq test sources into Helpers TaskSeqFieldTests.fs and ObservableExtensionsTests.fs had each grown their own copies of the same async-enumerable test sources: ThrowingAsyncEnumerable, an "endless numbers" source recording pulls and disposal, "one item then the enumeration fails", "one item then DisposeAsync throws", the synchronous asyncItems/asyncRange sequence, and a delay helper differing only in argument order. Moved all of them into the shared, auto-opened Helpers module next to the existing SuspendingAsyncEnumerable and waitForTask, and updated both test files to use the shared versions instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Addressed third Copilot review: in-flight tracking, slot release on failure, placeholder cleanup ObservableExtensions.fs: ofAsyncEnumerableResolved kept every asynchronously resolved item's Task in a ResizeArray until the whole source ended, so a long or infinite @stream source retained one task per delivered item despite maxConcurrency. It also released a resolution's concurrency slot only after both awaiting it and emitting succeeded, so a failed resolution or an observer throwing while a result was delivered left the slot held forever; with maxConcurrency = 1 this deadlocked the enumeration. Replaced the task list with an in-flight counter plus a TaskCompletionSource signalled once enumeration has ended and every started resolution has settled, and moved the slot release into a finally so it always runs. A resolution failure now stops pulling further items and is delivered through onFailure the same way an enumeration failure is, after every resolution already started settles. GraphQLWebsocketMiddleware.fs: addClientSubscription registered its SingleAssignmentDisposable placeholder before calling Subscribe, so a stream throwing synchronously from Subscribe left the placeholder registered forever, permanently occupying the subscription id. Subscribe is now wrapped so a synchronous failure removes the placeholder (a no-op if a synchronous completion already did) before rethrowing for the existing per-message error handling to report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fixed CI: the observer-throws regression test deadlocked on System.Reactive's own subscription teardown The previous commit's new test subscribed a raw IObserver<'T> whose OnNext throws to reproduce the concurrency-slot leak Copilot flagged. That part of the fix is correct (confirmed by an isolated repro run 300 times without System.Reactive: the semaphore slot is always released via the finally block). But through the real System.Reactive Subscribe(IObserver<'T>) call used by the operator, and confirmed with another isolated repro against the actual System.Reactive package, an observer's OnNext throwing makes Rx tear the subscription down itself: it disposes the subscription (cancelling the enumeration's token) before rethrowing. ofAsyncEnumerableResolved correctly treats that as "nobody is listening anymore" and skips both the onFailure delivery and OnCompleted, exactly as it does when disposed for any other reason. The test's assertion that OnCompleted still fires and onFailure still gets delivered was therefore wrong, and it hung for the test's full timeout on CI (Timeout waiting for OnCompleted), failing the build on all three OS runners. Replaced it with a test that doesn't depend on OnCompleted: it uses a source that signals a TaskCompletionSource from DisposeAsync, and waits (bounded) for that instead, which still proves the enumeration reaches disposal without hanging on the concurrency slot. Corrected the doc comment and RELEASE_NOTES.md, which both overstated that onFailure is delivered in this case. Also split the RELEASE_NOTES.md bullet about graphql-transport-ws discarding partial data into its two separate, more precise fixes (subscription data vs. Direct-result errors), per Copilot's fourth review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Rechecked stop conditions after acquiring a concurrency slot ofAsyncEnumerableResolved only checked failed()/cancellation at the top of the while loop. With maxConcurrency = 1 the loop parks in slots.WaitAsync while the single in-flight resolution runs; when that resolution fails, its finally releases the slot and the loop resumes straight into MoveNextAsync, so one more item is pulled and, if it resolves synchronously, emitted before the failure that already happened - contradicting the doc comment's claim that a failed resolution "stops the enumeration". Rechecked both conditions right after acquiring the slot, releasing it and stopping without pulling when either is set. Confirmed the race and the fix with an isolated fsi repro of the operator (100/100 runs emitted [2; -1] before this change, [-1] after), since the project's test suite can't be run standalone here (see the third-review commit's message on the AspNetCore-only compiler bug being addressed on struct-optional-params). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Addressed sixth Copilot review: recheck after MoveNextAsync, clarified item-error semantics ofAsyncEnumerableResolved only rechecked the stop conditions right after acquiring a concurrency slot, not after the following MoveNextAsync. A background resolution can fail while that move is still suspended; once it completes, the code went straight to resolving the item it produced, so with maxConcurrency > 1 an item pulled after a failure could still be resolved and, if synchronous, emitted before it. Factored the two checks into one `stopped ()` predicate and used it after both awaits. Confirmed the race and the fix with the same isolated fsi repro approach as the fifth review's fix (100/100 runs emitted the extra item before this change, 0/100 after). Also addressed the review's other thread: it read "a failed item resolution stops the enumeration" (from the doc comment, RELEASE_NOTES.md and the PR description) as meaning any per-item GraphQL error should end the stream, since resolveStreamedItem wraps every ResolverResult, including Error, into a plain StreamedItem. That wording described the AsyncVal computation itself throwing (a bug in the resolution plumbing, treated like a source failure), not an ordinary resolver error, which executeResolvers already turns into a normal, non-throwing ResolverResult.Error value - the same value @stream on an ordinary list turns into that item's DeferredErrors while continuing to stream the rest (see DeferredTests."Resolver list error"). Kept that behavior, since diverging from ordinary lists here would be surprising and isn't what GraphQL's per-field error semantics call for, and reworded the doc comment, RELEASE_NOTES.md and docs/type-system.md to make the distinction explicit. Added an execution-level regression test that pins the intended behavior: an item's own field error is delivered on its own path and the following items keep streaming. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Addressed seventh Copilot review: addressable batch payloads, complete after Direct/RequestError GraphQLWebsocketMiddleware.fs sent a batch of streamed items (grouped by the field's batching policy or the query's preferredBatchSize) as one payload addressed by a path ending in the list of the batch's own indices, such as ["numbers", [0, 1]]. No graphql-transport-ws client can merge that into the response tree: a batch isn't addressable by any single index, only its individual items are. Added IncrementalPayloadSplitting, a small pure module that recognises such a path and splits the batch into one payload per item, addressed the same way a field that streams one item at a time already is (a one-element data array at a path ending in that item's own index), in the batch's own order. Each item's own errors are attributed by checking which item's path they start with, since every error the engine attaches to a batch already carries the full path of the specific item it came from - no cross-message state is needed. This keeps the engine's batching (still one buffered/merged event upstream) while making every item addressable on the wire. Verified the splitting logic with an isolated fsi repro of the algorithm (out-of-order batch, and a batch with one item's own field error) before adding the xUnit test. Also fixed graphql-transport-ws never sending complete after the single next of a Direct (query/mutation) or RequestError result, which the protocol requires ("Server dispatches the Complete message indicating that the execution has completed" after "at most one Next message"). A newer incremental-delivery wire format (pending/incremental/completed, matching graphql-js 17 and Apollo Client's GraphQL17Alpha9Handler) was considered and is worth adopting, but requires a per-field completion signal threaded through the engine's whole merged deferred/streamed/live observable, which touches the ~25 pre-existing, TaskSeqField-unrelated tests in DeferredTests.fs (exact payload positions and counts) that this PR otherwise leaves alone. Tracked as follow-up work on a separate branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Addressed eighth Copilot review: WS error message, internal helper, conventions Request (validation) errors were sent over graphql-transport-ws as a Next followed by Complete, which the protocol reserves for results; a client reading it would see a successful, null-data result instead of the operation being terminated by an error. Routed RequestError through the terminal Error message instead, with no Complete after it (Direct still gets Next + Complete: it is a real, if partly erroneous, result). That required ServerMessage.Error and ServerRawPayload.ErrorMessages to carry GQLProblemDetails list instead of NameValueLookup list, so the error payload is a standard GraphQL error array as the protocol's ExecutionError requires, rather than an arbitrary object. Replaced the one other Error call site (the subscription catch-all's hand-built NameValueLookup, which had no message field and so was not a valid GraphQL error either) with GQLProblemDetails.Create. Found while wiring this up: RawServerMessageConverter.Write serialized the ErrorMessages and CustomResponse payloads without a preceding WritePropertyName ("payload"), unlike the ExecutionResult branch. Utf8JsonWriter throws when a value is written where a property name is expected, so every "error" message, and every "pong" carrying a payload, failed to serialize - nothing covered those two write paths. Fixed, and added two regression tests; verified the throw and the fix against a bare Utf8JsonWriter first. Made IncrementalPayloadSplitting internal instead of public - it was public only so the test project could reach it, which committed obj list paths and tuple results to the package's supported surface for no reason. Exposed it to the test assembly the same way Shared and Server already do (InternalsVisibleTo), rather than the transport implementation detail. Also replaced the one list-append (@) this PR introduced with a list expression, per the project's collection conventions. Added IcedTasks to the test project only (no transitive dependency for consumers) and rewrote SuspendingAsyncEnumerable.MoveNextAsync as a valueTask CE instead of manually wrapping a Task in a ValueTask, per the async conventions; verified the valueTask CE against the real IcedTasks package before wiring it in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Addressed ninth Copilot review: a failed non-null root field is an execution result, not a request error GQLResponseContent.RequestError was produced both before execution (validation, planning, variable coercion, a middleware, or the executor itself failing) and after it, whenever executeOperation's non-null root field failed and the error propagated to the root. The previous commit routed every RequestError through graphql-transport-ws's terminal Error message, which is wrong for the second case: per the spec, a response with a failed non-null root field is still an execution result (data is null, but present), not a request rejected before execution, so it must be sent as Next + Complete like any other result. The HTTP handler had the same conflation the other way: GQLResponse.RequestError omits data for both, so such a response was missing "data" entirely instead of carrying null. Represented the root failure as Direct (null, errs) instead of adding a new case: RequestError now means pre-execution only, and both transports already handle Direct correctly. Updated the six tests that asserted RequestError for this case (three in ExecutionTests.fs, one each in LazyEnumerationExceptionTests.fs and TaskSeqFieldTests.fs) to assert Direct with null data instead; the other 21 ensureRequestError call sites are genuine pre-execution validation/coercion/middleware failures and are unchanged. Documented the distinction on the two GQLResponseContent cases and reworded the middleware's comments and log messages to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix ninth-round regression: inline argument coercion must stay a RequestError executeQueryOrMutation's final Error branch (added when the ninth review's fix made a failed non-null root field produce Direct(null, errs)) was also being reached by executeRootOperation's own getArgumentValues failure for inline (literal) root field arguments, since both errors flowed into the same collectFields-aggregated Result. That reclassified inline argument and input object coercion/validation failures as execution results with null data instead of RequestError, breaking InputObjectValidatorTests's "Execute handles validation of invalid inline input records with all fields" on CI. Inline argument coercion is now checked for every root field up front, mirroring Executor.eval's coerceVariables step for variables: if any root field's arguments fail to coerce, the whole request is rejected as RequestError before any resolver runs. Only a genuine resolver failure on a non-null root field now reaches the Direct(null, errs) branch. As a side effect, a mutation with an invalid literal argument on a later root field no longer executes the earlier root fields' resolvers first. Verified with a full solution build and the complete unit test suite locally (dotnet build FSharp.Data.GraphQL.slnx + dotnet test), matching CI's approach, instead of the previous partial per-project builds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add regression coverage for a mixed success/error stream batch The tenth Copilot review claimed Execution.collectItems' chunk branch omits a failed item's index from `indicies` while still reserving its slot in `data`, so GraphQLWebsocketMiddleware.splitBatch's List.map2 would throw on a batch mixing a successful and a failed item. It does not: both arms of collectItems' `merge` prepend the item's index (`box index :: indicies`), and only the Ok arm additionally writes into `data` - so `indicies` and `data` always end up exactly `chunk.Length` long, with a failed item's slot left null. List.map2 never sees mismatched lengths. Added two regression tests pinning this shape rather than changing behavior: one drives the real engine through a @stream query with Fixed batching where item 0's own field resolution fails and item 1 succeeds, asserting the single DeferredErrors event this produces (TaskSeqFieldTests.fs); the other exercises splitBatch directly with a null data slot (IncrementalPayloadSplittingTests.fs). Both pass. Also addressed the same review's suppressed comment: the "emits each item as soon as its fields are resolved" test's ordering assertion depended on the default maxConcurrency (Environment.ProcessorCount), which could fail on a single-CPU runner; maxConcurrency is now set explicitly on that test. Verified with a full solution build and the complete unit test suite locally, matching CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Streamline streaming execution: struct tuples, backgroundTask, renamed AsyncEnumerable module Avoid allocating reference tuples in the per-item and per-batch stream collection paths in Execution.fs by switching to struct tuples. In ObservableExtensions.fs, switch every enumeration/resolution loop from task to backgroundTask so continuations never resume on a subscriber's or caller's synchronization context, rename AsyncEnumerableExtensions to AsyncEnumerable, and replace the ExceptionDispatchInfo round-trip with ex.Reraise(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * PR review fix * Address PR review feedback Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com> * Cancel MoveNextAsync on resolution failure Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com> * Prefer resolution failure over cancellation Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com> * Use `CanceledIndependently` active pattern --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.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.
Summary
This is the base implementation split out of the original #598. It adds native
Define.TaskSeqFieldsupport forIAsyncEnumerable<'T>and the server-side streaming execution pipeline.Included
Define.TaskSeqFieldfor async-enumerable list fields.@streamexecution support and related response handling.Review scope
Please review the TaskSeqField and core streaming execution changes in this PR. WebSocket lifecycle, incremental WebSocket payload formatting, and WebSocket error cleanup are intentionally excluded and are covered by the stacked follow-up PR.
Stacked follow-up PR: #608
Original superseded PR: #598