Hardened WebSocket streaming and incremental delivery by xperiandri · Pull Request #608 · fsprojects/FSharp.Data.GraphQL

xperiandri · 2026-09-18T13:47:17Z

Summary

Contains only WebSocket-specific work extracted semantically from the former mixed branch.

Included

WebSocket subscription cleanup and lifecycle synchronization.
Incremental payload transport and terminal error preservation.
WebSocket error sanitization/deduplication and focused serialization tests.

Review scope

Please review only WebSocket transport, subscription lifecycle, and WebSocket payload/error serialization. The response-data contract is in #611, and generic streaming/middleware fixes are in #612. Those areas should not be reviewed again here.

Dependency chain: #611 → #612 → this PR → #609.

Verification

Relevant projects build successfully; the full unit suite passed (663 passed, 5 pre-existing skips). git diff --check is clean.

Copilot

🟡 Changes recommended

Unresolved WebSocket send-serialization and subscription-cleanup findings block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens GraphQL WebSocket incremental delivery, subscription lifecycle handling, nullable response contracts, and error processing.

Changes:

Adds incremental payload paths, completion markers, and safer error serialization.
Improves subscription cleanup, cancellation, and async enumeration handling.
Updates nullable response APIs, middleware behavior, tests, and release notes.

File summaries

File	Summary
`tests/FSharp.Data.GraphQL.Tests/TaskSeqFieldTests.fs`	Tests streaming and nullable TaskSeq behavior.
`tests/FSharp.Data.GraphQL.Tests/Relay/ConnectionTests.fs`	Updates nullable direct-response assertions.
`tests/FSharp.Data.GraphQL.Tests/MutationTests.fs`	Updates nullable mutation-result assertions.
`tests/FSharp.Data.GraphQL.Tests/MiddlewareTests.fs`	Covers middleware coercion errors.
`tests/FSharp.Data.GraphQL.Tests/LazyEnumerationExceptionTests.fs`	Updates lazy enumeration and null-data assertions.
`tests/FSharp.Data.GraphQL.Tests/Helpers and Extensions/ObservableExtensionsTests.fs`	Tests async enumeration failure and cancellation.
`tests/FSharp.Data.GraphQL.Tests/ExecutorMiddlewareTests.fs`	Updates nullable executor-result assertions.
`tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs`	Covers null data and execution errors.
`tests/FSharp.Data.GraphQL.Tests/ErrorHelpers.fs`	Adds nullable direct-result helpers.
`tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs`	Updates deferred nullable-error expectations.
`tests/FSharp.Data.GraphQL.Tests/AspNetCore/SerializationTests.fs`	Tests WebSocket serialization and error handling.
`src/FSharp.Data.GraphQL.Shared/WebSockets.fs`	Updates nullable incremental payload models.
`src/FSharp.Data.GraphQL.Shared/TypeSystem.fs`	Supports nullable TaskSeq resolver handling.
`src/FSharp.Data.GraphQL.Shared/SchemaDefinitions.fs`	Adds nullable TaskSeq field overloads and documentation.
`src/FSharp.Data.GraphQL.Server/ObservableExtensions.fs`	Hardens async enumeration lifecycle handling.
`src/FSharp.Data.GraphQL.Server/IO.fs`	Updates nullable response contracts.
`src/FSharp.Data.GraphQL.Server/Executor.fs`	Maps nullable execution results.
`src/FSharp.Data.GraphQL.Server/Execution.fs`	Preserves partial data and incremental errors.
`src/FSharp.Data.GraphQL.Server/ErrorMessages.fs`	Updates enumerable error helpers.
`src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs`	Handles middleware coercion failures as request errors.
`src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs`	Updates incremental delivery, sanitization, and lifecycle handling.
`src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLSubscriptionsManagement.fs`	Synchronizes subscription registry operations.
`src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs`	Adapts HTTP response conversion to nullable contracts.
`RELEASE_NOTES.md`	Documents response and WebSocket changes.

Review details

Suppressed comments (3)

src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLSubscriptionsManagement.fs:46

If one subscription's unsubscribe action or Dispose throws, Seq.iter stops here and the remaining snapshot entries are never disposed. Because the dictionary was already cleared, those subscriptions cannot be recovered on a later cleanup, leaking active streams on disconnect; cleanup should dispose every entry independently while preserving/reporting failures.

    subscriptionsToDispose
    |> Seq.iter (fun struct (id, subscription) -> subscription |> executeOnUnsubscribeAndDispose id)

src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs:469

The new finally is the primary protection against leaking active subscriptions when the receive loop exits, but the changed test set has no WebSocket middleware regression test for disconnect/request cancellation or the synchronous Subscribe completion/throw races this PR fixes. Add coverage that asserts every registered subscription is disposed and its id is released on those paths.

            finally
                subscriptions
                |> GraphQLSubscriptionsManagement.removeAllSubscriptions

src/FSharp.Data.GraphQL.Shared/TypeSystem.fs:2506

The new ValueNone branch is not exercised by the added test, which only resolves ValueSome. This branch is the nullable contract and should be verified through execution (including a streamed or deferred request) so a regression cannot turn a missing sequence into an enumerable/type error instead of GraphQL null.

            | ValueNone -> box valueNone

Files reviewed: 24/24 changed files
Comments generated: 5
Review effort level: Lite

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

github-actions · 2026-09-18T16:56:07Z

Test Results

9 files 9 suites 13m 22s ⏱️
776 tests 771 ✅ 5 💤 0 ❌
2 328 runs 2 313 ✅ 15 💤 0 ❌

Results for commit 075abb6.

♻️ This comment has been updated with latest results.

Add the WebSocket-specific transport, subscription lifecycle, and payload/error serialization changes on top of the generic streaming and data-contract fixes: the SubscriptionExecutionResult.Data contract change to obj voption Skippable and related lifecycle handling in Shared/WebSockets.fs; GraphQLSubscriptionsManagement.fs subscription bookkeeping; the ObservableErrorHandling sanitization/deduplication module and remaining lifecycle/serialization changes in GraphQLWebsocketMiddleware.fs; the RELEASE_NOTES.md entry documenting the SubscriptionExecutionResult.Data change; and the new/updated WebSocket wire-format and error-sanitization tests in SerializationTests.fs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved error sanitization, performance, fallback-handling, and lifecycle test issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 2

Open (4)

Sanitize executor failures before sending WebSocket errors · New
These callbacks can run on background threads while the receive loop sends…
Fallback when AggregateException has no problem details · New
This filters the full error list once per item, making a batch with many item errors O(items ×…

Resolved since last review (3)

This updated regression comment repeats the indicies typo; use indices in both references so…
The newly introduced local is misspelled as indicies; rename it to indices so the batch-index…
The new parameter name is misspelled as indetifier; use identifier to keep this helper's API…

Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>

* Introduced `vtryItem` for Seq, List, and Array to provide ValueOption-based safe indexing. * Updated tryGetPathItemIndex to use `List.vtryItem` for improved safety and consistency. * Replaced `Seq.choose` with `Seq.vchoose` in `splitBatch`. * Added `InternalsVisibleTo` for `FSharp.Data.GraphQL.Server.AspNetCore` in the shared project.

Copilot

Copilot review overview

🟡 Changes recommended

WebSocket closes remain unsynchronized with sends, and one disposal failure can prevent cleanup of remaining subscriptions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Serialize socket close operations with sends · New
Dispose all subscriptions even when one disposal fails · New

Resolved since last review (4)

Sanitize executor failures before sending WebSocket errors
These callbacks can run on background threads while the receive loop sends…
Fallback when AggregateException has no problem details
This filters the full error list once per item, making a batch with many item errors O(items ×…

Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

Unbounded close handshakes can prevent cleanup, and sanitization currently removes useful server-side diagnostics.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Bound CloseAsync to prevent indefinite shutdown blocking · New

Resolved since last review (2)

Serialize socket close operations with sends
Dispose all subscriptions even when one disposal fails

Previously missed (1)

In code that hasn't changed since last review

Preserve original backend details in server logs

src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs:419

Sanitizing before logging removes the original backend message and exception from server-side diagnostics. The client should receive the sanitized list, but the trusted server log should retain problemDetails, as the other exception paths in this middleware do.

Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>

Copilot · 2026-09-19T01:05:45Z

Copilot review overview...

Addressed in 6486c2d.

Copilot

Copilot review overview

🟢 Approval recommended

The reviewed WebSocket transport, lifecycle, serialization, and focused tests are consistent with the stated scope and contain no unresolved issues.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Bound CloseAsync to prevent indefinite shutdown blocking

This was referenced Sep 18, 2026

Added Define.TaskSeqField for lazy async collection streaming support (IAsyncEnumerable) #607

Merged

Added Define.TaskSeqField with streaming of IAsyncEnumerable results #598

Closed

xperiandri force-pushed the task-seq-field-core branch from 810ff5a to 46809e6 Compare September 18, 2026 13:50

xperiandri mentioned this pull request Sep 18, 2026

Adopt pending/incremental/completed delivery format #609

Merged

xperiandri force-pushed the task-seq-field-websocket branch from 60987fd to 3ec49f7 Compare September 18, 2026 14:05

Base automatically changed from task-seq-field-core to dev September 18, 2026 16:42

xperiandri force-pushed the task-seq-field-websocket branch from 3ec49f7 to 765ac78 Compare September 18, 2026 16:47

xperiandri requested a lite review from Copilot September 18, 2026 16:47

Copilot started reviewing on behalf of xperiandri September 18, 2026 16:48 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

xperiandri mentioned this pull request Sep 18, 2026

Use [<Struct>] ? optional parameters and voption in Define/DefineRec and the introspection chain #604

Merged

xperiandri force-pushed the task-seq-field-websocket branch from 765ac78 to 73d00c3 Compare September 18, 2026 17:48

xperiandri mentioned this pull request Sep 18, 2026

Fixed streaming execution and middleware behavior #612

Merged

xperiandri changed the base branch from dev to task-seq-field-streaming-fixes September 18, 2026 17:49

xperiandri force-pushed the task-seq-field-streaming-fixes branch from 8fad834 to 551f6c7 Compare September 18, 2026 19:54

xperiandri force-pushed the task-seq-field-websocket branch from 73d00c3 to 500d88a Compare September 18, 2026 19:54

xperiandri force-pushed the task-seq-field-streaming-fixes branch 2 times, most recently from bbd883f to b57bf2d Compare September 18, 2026 23:05

xperiandri force-pushed the task-seq-field-websocket branch from 500d88a to f0ad702 Compare September 18, 2026 23:26

Base automatically changed from task-seq-field-streaming-fixes to dev September 18, 2026 23:49

xperiandri force-pushed the task-seq-field-websocket branch from f0ad702 to 19b5f35 Compare September 18, 2026 23:53

xperiandri requested a lite review from Copilot September 18, 2026 23:55

Copilot started reviewing on behalf of xperiandri September 18, 2026 23:56 View session

xperiandri force-pushed the task-seq-field-websocket branch from 19b5f35 to 3e3dbb6 Compare September 18, 2026 23:59

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Comment thread src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs

Comment thread src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs Outdated

xperiandri force-pushed the task-seq-field-websocket branch from 3e3dbb6 to 4faa338 Compare September 19, 2026 00:00

Copilot started work on behalf of xperiandri September 19, 2026 00:02 View session

Fix WebSocket review feedback

92db2a8

Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>

Copilot finished work on behalf of xperiandri September 19, 2026 00:09

xperiandri added 2 commits September 19, 2026 02:29

Stop treating null elements as missing in vtryHead and vtryLast

075abb6

xperiandri requested a balanced review from Copilot September 19, 2026 00:39

Copilot started reviewing on behalf of xperiandri September 19, 2026 00:40 View session

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Comment thread src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs

Comment thread src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLSubscriptionsManagement.fs Outdated

Copilot started work on behalf of xperiandri September 19, 2026 00:46 View session

Serialize websocket closes and cleanup all subscriptions

9f66d7a

Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>

xperiandri requested a balanced review from Copilot September 19, 2026 00:53

Copilot finished work on behalf of xperiandri September 19, 2026 00:53

Copilot started reviewing on behalf of xperiandri September 19, 2026 00:54 View session

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Comment thread src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs Outdated

Copilot started work on behalf of xperiandri September 19, 2026 01:00 View session

Bound websocket closes and preserve request logs

6486c2d

Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>

Copilot finished work on behalf of xperiandri September 19, 2026 01:08

xperiandri requested a balanced review from Copilot September 19, 2026 01:24

Copilot started reviewing on behalf of xperiandri September 19, 2026 01:25 View session

Copilot AI reviewed Sep 19, 2026

View reviewed changes

xperiandri changed the title ~~Harden WebSocket streaming and incremental delivery~~ Hardened WebSocket streaming and incremental delivery Sep 19, 2026

xperiandri merged commit d326a30 into dev Sep 19, 2026
5 checks passed

xperiandri deleted the task-seq-field-websocket branch September 19, 2026 01:45

Navigation Menu

Hardened WebSocket streaming and incremental delivery - #608

Hardened WebSocket streaming and incremental delivery#608
xperiandri merged 6 commits into
devfrom
task-seq-field-websocket

xperiandri commented Sep 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI commented Sep 19, 2026

Copilot review overview...

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Summary

Included

Review scope

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI commented Sep 19, 2026

Copilot review overview...

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xperiandri commented Sep 18, 2026 •

edited

Loading

github-actions Bot commented Sep 18, 2026 •

edited

Loading