Skip to content

feat(ws-topic)!: reject a topic subscription on binary-encoding mismatch - #3160

Open
phil-opp wants to merge 1 commit into
mainfrom
feat/ws-topic-protocol-version
Open

feat(ws-topic)!: reject a topic subscription on binary-encoding mismatch#3160
phil-opp wants to merge 1 commit into
mainfrom
feat/ws-topic-protocol-version

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Follow-up to #3153, which noted this gap but left it out of scope.

The WebSocket topic-data channel pushes subscription_id ++ <encoded Timestamped<InterDaemonEvent>> with no per-frame envelope. Both the old (bincode) and current (postcard) encodings are positional, so a peer on the wrong one doesn't fail to decode — it misparses, yielding plausible-looking garbage. That was the one break in #3153 that couldn't fail loudly.

The subscription handshake now carries protocol_version in both directions and either side refuses on mismatch:

{"TopicSubscribe":  {"dataflow_id": "", "topics": [], "protocol_version": 2}}
{"TopicSubscribed": {"subscription_id": "", "protocol_version": 2}}

Both fields are #[serde(default)], so a peer predating the handshake omits them, deserializes to None, and is rejected for the same reason a wrong number is. That covers both skew directions: an old coordinator's ack has no version and the CLI refuses it; an old CLI's request has no version and the coordinator refuses it before creating anything.

TOPIC_DATA_PROTOCOL_VERSION lives in dora-message (1 = bincode, 2 = postcard). Bump it whenever the binary payload encoding changes; the JSON handshake is self-describing and doesn't need it.

Rejections name both versions so an operator can tell which side is old without reading the source:

topic data protocol mismatch: client speaks version 1, coordinator speaks 2.
Binary frames are positionally encoded, so subscribing would silently misparse
rather than fail. Upgrade whichever side is older.

Note on the coordinator-side subscription

On mismatch the CLI refuses the ack but deliberately does not send TopicUnsubscribe. Only a pre-handshake coordinator reaches that branch having actually created a subscription — a current one rejects before creating anything — and all four subscribe_topics callers propagate the error with ?, dropping the WsSession, which closes the connection and makes the coordinator tear its subscriptions down. Threading an outgoing sender into handle_response (already at its argument limit) to save an already-closing subscription didn't seem to earn its keep. Reasoning is recorded at the call site.

Tests

  • only_our_exact_protocol_version_is_accepted pins the accept/reject decision itself — check_topic_protocol is split out of the request handler so deleting or inverting the guard fails a test rather than only changing a message.
  • topic_subscribe_ack_rejects_older_protocol_version / …_missing_protocol_version assert the CLI refuses and that the subscription is not registered for frame dispatch.
  • The two message tests assert both arms name the peer's version and say why a mismatch is fatal.
  • tests/ws-cli-e2e.rs's mock coordinator now advertises the current version, which is what proves the happy path still connects end to end.

Docs and the guide mirror gain a Protocol version section; the changelog entry and the docs/websocket-topic-data-channel.md note that both said "there is no version handshake on this channel" are corrected.

The topic data channel pushes `subscription_id ++ <encoded
Timestamped<InterDaemonEvent>>` with no per-frame envelope. Both the old
(bincode) and current (postcard) encodings are positional, so a peer on
the wrong one does not fail to decode — it misparses, yielding
plausible-looking garbage. That was the one break in #3153 that could
not fail loudly.

The subscription handshake now carries `protocol_version` in both
directions and either side refuses on mismatch. Both fields are
`#[serde(default)]`, so a peer predating the handshake omits them,
deserializes to `None`, and is rejected for the same reason a wrong
number is — covering both skew directions.

`Hello`/`check_cli_version` does not make this redundant: third-party
subscribers never send `Hello`, and `versions_compatible` is
semver-caret, so a future encoding change inside 1.x would be waved
through.

Refs #3153
@phil-opp phil-opp added this to the 1.0 milestone Aug 13, 2026
@trunk-io

trunk-io Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — generated fully automatically, no human in the loop. Verify before acting.

No issues found.

I traced the new protocol_version handshake end to end:

  • The handshake travels as JSON (Message::Text / format_response_json), which is self-describing, so adding #[serde(default)] protocol_version: Option<u16> to TopicSubscribe/TopicSubscribed is backward-compatible on the wire and an omitted field correctly deserializes to None.
  • Coordinator (ws_control.rs): check_topic_protocol is evaluated before any subscription work, including before the existing-subscription dedup path, and only Some(TOPIC_DATA_PROTOCOL_VERSION) is accepted — None and any other number are rejected. Both success replies carry Some(TOPIC_DATA_PROTOCOL_VERSION).
  • CLI (ws_client.rs): the ack branch refuses and does not register the subscription on mismatch; the reasoning for not sending TopicUnsubscribe (only a pre-handshake coordinator reaches that branch with a live subscription, and dropping WsSession closes the connection) is sound.
  • The tests are non-vacuous: topic_subscribe_ack_rejects_older_protocol_version/_missing_protocol_version and only_our_exact_protocol_version_is_accepted would all pass Ok/register the subscription if the guard were reverted, so they fail against the old behavior. The e2e mock server was updated to advertise the version, consistent with the new requirement.

Generated by Claude Code

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.

1 participant