Added Define.TaskSeqField with streaming of IAsyncEnumerable results - #598
xperiandri wants to merge 25 commits into
Conversation
Test Results 9 files 9 suites 12m 54s ⏱️ Results for commit dc28bff. ♻️ This comment has been updated with latest results. |
9dda1df to
764cc07
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Stream failures can overtake prior items, batching callbacks run for non-streamed queries, and item resolution concurrency is unbounded.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds native IAsyncEnumerable field resolution, streaming/batching support, and incremental WebSocket delivery.
Changes:
- Introduces
Define.TaskSeqFieldand configurable stream batching. - Adds async-enumerable execution, cancellation, and error handling.
- Updates WebSocket payloads, documentation, samples, and tests.
File summaries
| File | Description |
|---|---|
TaskSeqFieldTests.fs |
Tests async sequence fields and streaming. |
Helpers.fs |
Adds a suspending async-enumerable test helper. |
ObservableExtensionsTests.fs |
Tests async-enumerable observables. |
FSharp.Data.GraphQL.Tests.fsproj |
Adds test dependencies and source file. |
SerializationTests.fs |
Tests incremental payload serialization. |
WebSockets.fs |
Expands WebSocket execution payloads. |
TypeSystem.fs |
Adds async-sequence resolver and batching types. |
SchemaDefinitionsExtensions.fs |
Rejects unsupported resolver middleware. |
SchemaDefinitions.fs |
Adds TaskSeqField overloads. |
FSharp.Data.GraphQL.Shared.fsproj |
Adds async-interface compatibility dependency. |
ObservableExtensions.fs |
Adds async-enumerable observable adapters. |
Execution.fs |
Executes and streams async sequence items. |
ErrorMessages.fs |
Updates enumerable type error text. |
GraphQLWebsocketMiddleware.fs |
Sends incremental payloads immediately. |
star-wars-api.fsproj |
Adds TaskSeq dependency. |
Schema.fs |
Demonstrates streamed friends. |
RELEASE_NOTES.md |
Documents features and breaking changes. |
Packages.props |
Centrally versions new dependencies. |
docs/type-system.md |
Documents asynchronous sequence fields. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d0a9fb8 to
b0d54ed
Compare
b0d54ed to
ae7055d
Compare
7a079a1 to
9a01491
Compare
…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>
There was a problem hiding this comment.
🟡 Changes recommended
Async-enumerator lifecycle failures and synchronous WebSocket completion can terminate or strand incremental operations, while some WebSocket errors are still discarded.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs:205
- Direct executions may contain partial data together with field errors, but the errors are ignored here even though
Createaccepts them. Forward the returned error list so WebSocket clients receive the same execution result as other transports.
| Direct (data, _) -> do! SubscriptionExecutionResult.Create (data, []) |> sendOutput id
src/FSharp.Data.GraphQL.Server/ObservableExtensions.fs:129
DisposeAsynccan fail, but this await is outside the captured-failure path. Such a failure faults the observable and terminates the entire deferred result stream rather than producing the field-levelStreamFailure; this is inconsistent with the non-streamed path and can suppress sibling incremental results. Capture disposal failures (while preserving an earlier enumeration failure) and route them throughonFailure.
// Captured items no longer need the enumerator, so it is disposed before waiting for their resolutions
do! enumerator.DisposeAsync ()
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Balanced
…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>
There was a problem hiding this comment.
🟡 Changes recommended
Pending resolution tasks can leak memory or deadlock, and synchronous subscription failures can strand operation IDs.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
It changes public APIs, concurrent execution, and WebSocket protocol behavior without a successful full regression run.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
RELEASE_NOTES.md:305
- The previous
Directbranch already forwardeddata; it only discarded the accompanying field errors. This release note incorrectly says that partial data was discarded for both result kinds. Describe the subscription partial-data fix and the direct-result error fix separately.
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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>
There was a problem hiding this comment.
🟡 Changes recommended
WebSocket disconnects do not dispose registered deferred subscriptions, allowing infinite asynchronous sequences to continue running.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent subscription-map cleanup can throw and leave active streams undisposed.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Observable errors can leak operation IDs, and the public API omits the established struct-nullable field variant.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs:198
- A source that terminates through
OnErrornever invokesOnCompleted, and the current error callback only logs. The newly registered placeholder therefore remains insubscriptions, so the operation ID cannot be reused until the whole socket disconnects. Remove the subscription from the error callback (in afinallyso logging cannot prevent cleanup).
src/FSharp.Data.GraphQL.Shared/SchemaDefinitions.fs:1242 - These nullable overloads only accept
option, soTaskSeqFieldcannot be used with the repository's establishedStructNullable (ListOf ...)wrapper even though ordinary fields support it andvoptionis the preferred nullable representation. Add correspondingIAsyncEnumerable<'Item> voptionoverloads and teach the TaskSeq quotation boxifier to unwrapValueSome/ValueNone.
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
@copilot fix that |
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A resolver failure can leave streaming permanently blocked in an outstanding MoveNextAsync.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 30/30 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Public response types declare data as non-nullable while the new execution paths intentionally produce null values.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 30/30 changed files
- Comments generated: 2
- Review effort level: Balanced
| /// An execution result. Data is null when a non-null root field failed during execution and the error | ||
| /// propagated to the root, exactly as it would for a non-null nested field, rather than being rejected as a | ||
| /// RequestError. |
| /// Result data: an object for complete and initial payloads, or a deferred or streamed value for incremental payloads. | ||
| /// It is omitted from the final payload of an incremental delivery. | ||
| Data : obj Skippable |
Summary
Adds native support for list fields resolved from
IAsyncEnumerable<'T>, such astaskSeq { }, C# async iterators or Azure SDKAsyncPageable<T>, and makes@defer/@streamresults usable overgraphql-transport-ws.Define.TaskSeqField@deferNullable (ListOf …)overloads, which are included)@streammaxConcurrencyitems resolving at the same timeseqtoday: nullable field →null+ field error, non-nullable → error propagates;@streamit is delivered asDeferredErrorsfor the field after every item already pulled has been resolved and delivered, so a slower item can never be overtaken by the error that follows it, and buffered items and sibling deferred streams are not lost. An item resolution that throws stops the enumeration and is delivered the same way, once every resolution already started has settled, and no item pulled after such a failure is resolved; its concurrency slot is always released, whether the resolution itself failed or delivering its result did.DeferredErrors, and streaming continues with the items after it — exactly as@streamalready behaves on an ordinary list.@streamon ordinary lists keeps its behavior.maxConcurrency(optional,Environment.ProcessorCountby default) bounds how many items are pulled from the source and resolved at the same time for@stream; pulling the next item waits for a free slot, and only items still in flight are tracked, so a long or infinite source does not retain what it already delivered. It applies to@streamonly.Batching of streamed items
StreamBatching<'Item>is set on the field:StreamBatching.Fixed sizeStreamBatching.FromSource (IAsyncEnumerable<'Item> -> int voption)computes the size from the resolved sequence instance.The
preferredBatchSizeargument of@streamtakes precedence over the field-level policy. Thebatchingcallback itself is now evaluated lazily, only when a query actually streams the field without supplying its ownpreferredBatchSize— it used to run eagerly for every query shape (including@deferand no directive), so a throwingFromSourcecallback could break a non-streamed query.Note: Azure
AsyncPageable<T>does not expose a page size (it is only a hint passed toAsPages), soFromSourcecan batch by pages only when the application keeps the hint, for example in a subclass ofAsyncPageable<T>. Tests cover both a plainAsyncPageable<T>and one that keeps the hint.A batch is grouped by the engine into one internal event, but over
graphql-transport-wsit is still sent as one independently addressednextpayload per item, in the batch's own order — a payload addressed by a list of indices (path: ["numbers", [0, 1]]) isn't mergeable by any client. The splitting logic (IncrementalPayloadSplitting, internal) is unit tested directly.WebSocket (
graphql-transport-ws) deliverypathandhasNext; the initial payload hashasNext: true, and a final{"hasNext": false}payload precedescomplete.Dictionary<string, obj>, which failed for streamed items and scalars.SubscriptionErrorsresult no longer discards its partialdata; it is forwarded alongside the errors instead of always sendingnull.Direct(non-subscription) result no longer discards its field errors; they are now logged and forwarded like the other branches.SingleAssignmentDisposable) beforeSubscriberuns, so a synchronous completion can still find and clean up the id.Subscribeitself threw synchronously, the placeholder used to stay registered forever too. It is now removed (or left alone if a synchronous completion already removed it) before the failure is rethrown for the existing per-message error handling to report.completenever being sent after the singlenextof aDirect(query/mutation) result.nextresult followed bycomplete, instead of the terminalerrormessage the protocol requires for it;error's payload is now a standard GraphQL error array (GQLProblemDetails list, see Breaking changes) rather than an arbitrary object. A query or mutation whose non-null root field fails during execution is not a request error — it is still sent asnext(withdata: null) +complete, like any other result.errormessages andpongmessages carrying a payload failing to serialize at all (Utf8JsonWriterthrowing), because neither was written under thepayloadproperty name.Example with the Star Wars sample (
Human.friendsStreamwas added to demonstrate@stream):Follow-up (separate branch): the wire format above (
data/path/hasNext) predates this PR and is not the format any current client speaks. A newer incremental-delivery format (pending/incremental/completed, the one graphql-js 17 and Apollo Client'sGraphQL17Alpha9Handleruse, where a client appends streamed items itself and nopathindex is needed at all) is worth adopting, but requires a per-field completion signal threaded through the engine's whole merged deferred/streamed/live observable — which touches roughly 25 pre-existing tests inDeferredTests.fsthat predate this PR and aren't specific toTaskSeqField. Given that size and blast radius, it's being done separately rather than folded into this PR.Breaking changes
SubscriptionExecutionResult.Datais nowobj Skippable, and the record has newPathandHasNextfields. Use theCreate,CreateErrors,CreateInitial,CreateIncrementalandCreateCompletedfactory members.BufferedStreamOptions.IntervalandBufferedStreamOptions.PreferredBatchSizeare nowint voption. Construct the options withValueSome/ValueNone, for exampleSchemaConfig.DefaultWithBufferedStream { Interval = ValueSome 2000; PreferredBatchSize = ValueNone }.ServerMessage.ErrorandServerRawPayload.ErrorMessagesnow carryGQLProblemDetails listinstead ofNameValueLookup list.GQLResponseContent.Direct(execution) result withnulldata instead ofRequestError, which is now only ever produced for a request rejected before execution (validation, planning, variable or inline argument coercion, a middleware, or the executor itself failing). HTTP andgraphql-transport-wsresponses for such a failure now carrydata: nullas the spec requires, instead of omittingdataentirely. Inline argument/input object coercion failures on a root field remainRequestErrorand are now checked for every root field before any of them execute, the same as variable coercion — a mutation no longer runs an earlier root field's resolver before rejecting the request over a later field's invalid literal argument.Dependencies
FSharp.Data.GraphQL.SharedreferencesMicrosoft.Bcl.AsyncInterfacesfornetstandard2.0only.FSharp.Control.TaskSeq,Azure.CoreandIcedTasksexplicitly.SDK
devnow pins SDK10.0.303instead of10.0.401. The F# compiler in SDK10.0.4xx(verified on10.0.400and10.0.401) compiles resumable state machines incorrectly in Debug, so ataskSeq { }that awaits returns no further items: dotnet/fsharp#20466, fixed inmainby dotnet/fsharp#20469 but not shipped yet (also fsprojects/FSharp.Control.TaskSeq#473).10.0.303is the latest SDK without the regression. The--always-inline+workaround andOptimize=truealone did not help on10.0.401.The code in this PR still builds with SDK
10.0.4xx. Passing avoptionto a struct optional parameter through?name = valueis supported only by the10.0.4xxcompiler, so that call is written as a match.Known limitations
taskSeq { }block usinglet!oryield!must live in a separate function called from the resolver. This is documented.WithResolveMiddlewareis not supported forTaskSeqFieldand throwsNotSupportedException.10.0.4xxin Debug hit the compiler regression above in their owntaskSeq { }blocks. Tests that need a real await use a hand-written suspendingIAsyncEnumerable, so they pass on any SDK.addClientSubscriptionfix (synchronous-completion race, synchronousSubscribefailure) is covered by an automated test: it is a private middleware function and there is no in-processgraphql-transport-wstest harness in this repo yet. The disposal semantics they rely on (SingleAssignmentDisposabledisposing an already-assigned/already-disposed instance) were verified in isolation instead.OnNextitself throws while a streamed item is delivered (for example a WebSocket send failing mid-stream),System.Reactivetears the subscription down as soon as that happens — this is standardSubscribe(IObserver<T>)behavior, not specific to this operator. The concurrency slot is still always released (no deadlock), but noonFailure/OnCompletedfollows, since the subscription is already gone by then.data/path/hasNextshape, not a format any current client library implements as-is; see the Follow-up note above.Testing
TaskSeqFieldTests: draining,@defer,@streamordering and early delivery, query/fixed/source batching and precedence, enumeration errors (nullable, non-nullable, streamed, including failures acquiring the enumerator), an item's own resolver error not ending the stream, a batch mixing a failed item with a succeeding one, cancellation,nullsequence, AzureAsyncPageable<T>,maxConcurrencybounding and validation, lazy evaluation ofFromSource.ObservableExtensionsTestsforofAsyncEnumerable,ofAsyncEnumerableResolved(ordering, bounded concurrency, disposal, failures raised acquiring/enumerating/disposing the source, a failed resolution not overtaken by another item pulled right after it — whether that item was waiting on a concurrency slot or still being produced by the source — and an observer throwing while a result is delivered) andwithCompletionMarker;IncrementalPayloadSplittingTestsfor the batch-splitting logic (out-of-order batches, item-level error attribution, a failed item'snulldata slot);SerializationTestsfor the new WebSocket payload shapes, includingerrormessages and a payload-carryingpong.ExecutionTests,LazyEnumerationExceptionTestsandTaskSeqFieldTests(6 tests total) to assertDirectwithnulldata for a failed non-null root field, instead ofRequestError. A follow-up commit added a regression test (ExecutionTests) pinning that an inline argument/input object coercion failure on one root field staysRequestErrorand pre-empts every root field's resolver, since that case was initially — and incorrectly — folded into the sameDirect (null, ...)change;InputObjectValidatorTests's existing inline-validation test is the same scenario at the single-field level. The other pre-existingensureRequestErrorassertions in the suite are genuine pre-execution failures and are unchanged.splitBatch(GraphQLWebsocketMiddleware.fs) on a batch mixing a successful and a failed item; tracingExecution.collectItemsshowed the claimed length mismatch betweenindicesanddatacannot occur (both are always exactlychunk.Length, a failed item just leaves anullslot), and two new regression tests (TaskSeqFieldTests,IncrementalPayloadSplittingTests, both listed above) confirm this end to end and at thesplitBatchlevel.10.0.303, ubuntu/windows/macOS) is green onea3709d7: full solution build (dotnet build FSharp.Data.GraphQL.slnx) and the full unit test suite (dotnet test, 660 tests) pass on all three runners, verified locally with the same commands before pushing.Utf8JsonWriterpayload-property fix, and theIcedTasks-basedvalueTaskCE were all verified with isolateddotnet fsirepros against the real packages before being wired in or committed — the concurrency ones running the exact race hundreds of times and confirming both the failure before and the fix after, since these are timing-sensitive and easy to get subtly wrong.🤖 Generated with Claude Code