Skip to content

Switch to graphql-transport-ws subprotocol - #821

Open
MetRonnie wants to merge 7 commits into
cylc:masterfrom
MetRonnie:graphql-transport-ws
Open

MetRonnie wants to merge 7 commits into
cylc:masterfrom
MetRonnie:graphql-transport-ws

Conversation

@MetRonnie

@MetRonnie MetRonnie commented May 13, 2026 •

Copy link
Copy Markdown
Member

Closes #690

Implements the graphql-transport-ws subprotocol as defined in https://github.com/enisdenjo/graphql-ws/blob/master/PROTOCOL.md

Sibling: cylc/cylc-ui#2548

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • No dependency changes
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added this to the 1.10.0 milestone May 13, 2026
@MetRonnie MetRonnie self-assigned this May 13, 2026
@dwsutherland dwsutherland mentioned this pull request May 21, 2026
8 tasks done
@MetRonnie
MetRonnie force-pushed the graphql-transport-ws branch from feb91ec to 9d7c5f8 Compare June 12, 2026 13:52
@MetRonnie
MetRonnie marked this pull request as ready for review June 12, 2026 14:50
@MetRonnie
MetRonnie requested a review from Copilot June 30, 2026 10:15
@MetRonnie

Copy link
Copy Markdown
Member Author

Requested a review from Copilot to try this out for the first time...

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Switches the UI Server’s WebSocket GraphQL subscriptions to the graphql-transport-ws subprotocol and updates the server-side message handling to follow the newer protocol semantics.

Changes:

  • Updated subscription WebSocket protocol constant and message handling (graphql-transport-ws, new message types, ping/pong, subscribe/next/complete).
  • Simplified subscription handler state management by making sub_statuses handler-local and updated resolvers context wiring accordingly.
  • Updated tests and added a changelog entry for the protocol change.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
cylc/uiserver/tests/test_handlers.py Updates the subprotocol selection test to the new protocol.
cylc/uiserver/handlers.py Switches the selected WebSocket subprotocol constant and adjusts subscription handler initialization/state.
cylc/uiserver/graphql/tornado_ws.py Implements graphql-transport-ws message types and updates subscription server message processing logic.
cylc/uiserver/authorise.py Tightens typing for AuthorizationMiddleware.auth.
cylc/uiserver/app.py Removes app-level sub_statuses plumbing now that subscription status is handler-local.
changes.d/821.feat.md Documents the protocol change in the changelog.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cylc/uiserver/handlers.py
Comment on lines 389 to +390
def select_subprotocol(self, subprotocols):
return GRAPHQL_WS
return WS_PROTOCOL

@MetRonnie MetRonnie Jun 30, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We are simultaneously upgrading the client (cylc/cylc-ui#2548) so this is not a problem (apart from the awkwardness of running development UI/UIS for a while when different branches have different subprotocols)

Comment thread cylc/uiserver/graphql/tornado_ws.py Resolved
Comment thread cylc/uiserver/graphql/tornado_ws.py Resolved
Comment thread cylc/uiserver/tests/test_handlers.py Resolved
Comment thread cylc/uiserver/graphql/tornado_ws.py Resolved
@MetRonnie
MetRonnie force-pushed the graphql-transport-ws branch from 7e7f23f to ffd0d7b Compare June 30, 2026 10:28
@MetRonnie
MetRonnie force-pushed the graphql-transport-ws branch from d87a94c to 029fd70 Compare July 21, 2026 12:36
@MetRonnie
MetRonnie force-pushed the graphql-transport-ws branch 2 times, most recently from d840246 to a5cab4d Compare September 2, 2026 11:14
@MetRonnie
MetRonnie force-pushed the graphql-transport-ws branch from a5cab4d to ab309e8 Compare September 2, 2026 11:33

@dwsutherland dwsutherland left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me 👍

Obviously you'll need the sibling in at the same time:

[E 2026-09-16 14:06:07.712 ServerApp] Uncaught exception GET /cylc/subscriptions (127.0.0.1)
    HTTPServerRequest(protocol='http', host='localhost:8888', method='GET', uri='/cylc/subscriptions', version='HTTP/1.1', remote_ip='127.0.0.1')
    Traceback (most recent call last):
.
.
.

Will post review there, seems like just something with GraphiQL to fix.

@oliver-sanders oliver-sanders modified the milestones: 1.10.0, 1.11.0 Sep 24, 2026
Comment on lines +238 to +239
case OperationType.PONG.value:
pass

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewers: Worth performing a functional test of websocket_ping_interval/timeout.

Changes here shouldn't impact it (implemented at the tornado layer), but worth checking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If you mean

# configure websocket pings (prevents clients / proxies closing seemingly idle
# connections and helps us to detect dead connections)
c.ServerApp.websocket_ping_interval = 5
c.ServerApp.websocket_ping_timeout = 5

Then I believe that is ping/pong at the websocket layer: https://www.rfc-editor.org/info/rfc6455/#section-5.5.2

Whereas the ping/pong here is the at the subprotocol layer, so is completely separate

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I don't think this would interact, but might be worth testing.

@oliver-sanders
oliver-sanders self-requested a review October 26, 2026 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

websocket: change subprotocol

4 participants