fix: close the session on the draft-20 protocol violations from the compliance backlog - #111
Merged
Merged
Conversation
…parameter value (§5.1.2, §10.2.8, §10.2.18, §1.4.3)
Values the draft makes session-fatal were checked only on some paths:
GROUP_ORDER outside {1, 2} (§10.2.8) on SUBSCRIBE and SUBSCRIBE_TRACKS
but not on PUBLISH, FETCH or inside FILL_PARAMETERS; FORWARD above 1
(§10.2.18) not in PUBLISH_STATE_NOTIFY or AcceptPublish; and a
LOCATION_FILTER whose StartGroup + EndGroupDelta overflows (§5.1.2:
"MUST close the session with a PROTOCOL_VIOLATION") answered per request
with MALFORMED_TRACK, INVALID_FILTER or a fill reset, a documented
deviation.
Parameters.CheckScope, which Session.CheckPeerParams runs on every
receive path (openers, responses, REQUEST_UPDATE, PUBLISH_STATE_NOTIFY),
now also validates, in the list and inside its FILL_PARAMETERS:
GROUP_ORDER, FORWARD, INCLUDE_PROPERTIES and LOCATION_FILTER. An end
Group overflow is PROTOCOL_VIOLATION. A LOCATION_FILTER or a
FILL_PARAMETERS nested list that does not match its serialization is
KEY_VALUE_FORMATTING_ERROR (§1.4.3, new message.ErrValueFormatting); an
unknown parameter or a Type past 2^64-1 inside FILL_PARAMETERS stays
PROTOCOL_VIOLATION (§10.2). Following the MUST over the old deviation was
the maintainer's choice.
The relay's per-path checks (checkForwardParam, checkGroupOrderParam, the
deviation branch) and AcceptSubscribe's FORWARD check are gone as
unreachable. The only per-request parameter error the relay has left is a
Range Filter's, INVALID_FILTER (§5.1.4, §10.2.12), which also replaces
the FETCH-only MALFORMED_TRACK there. The "Out-of-range GROUP_ORDER on
FETCH" Limitation is resolved.
The new session tests assert each close code, and the relay test covers
PUBLISH, FETCH, SUBSCRIBE, FILL_PARAMETERS and a REQUEST_UPDATE; both
fail on the unpatched code. Four relay tests that pinned the deviation now
trigger their per-request failure with an invalid Range Filter.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ty value (§12.5, §12.6)
"If an endpoint receives a value outside this range, it MUST close the
session with a PROTOCOL_VIOLATION": DEFAULT_PUBLISHER_GROUP_ORDER outside
{1, 2} (§12.5) and DYNAMIC_GROUPS above 1 (§12.6). The session checks
on receipt of SUBSCRIBE_OK, FETCH_OK, TRACK_STATUS_OK and
SUBSCRIBE_TRACKS_OK, and on a PUBLISH in AcceptRequest, before the
application can reject it. Values inside Immutable Properties are
searched too (§12.7), and a mutable value is still checked when the
Immutable Properties next to it do not parse.
The relay registry no longer carries its own out-of-range path: an
out-of-range DYNAMIC_GROUPS used to only decline a NEW_GROUP_REQUEST.
The session now guarantees the range.
Assumption: §10.5 does not say whether SUBSCRIBE_TRACKS_OK carries Track
Properties. Since §12.5/§12.6 apply to any received value, they are
checked there too. If the draft means otherwise, only that check goes.
Verified red first: all 30 session subtests (6 paths x 5 values), both
relay PUBLISH cases, and the registry case where a mutable Ascending
wins over an immutable Descending (§12.7).
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
"If a subscriber receives a PUBLISH or SUBSCRIBE_OK that uses the same Track Alias as a different Track with an Established subscription, it MUST close the session with error DUPLICATE_TRACK_ALIAS." Until now RegisterInboundTrack only returned *ErrDuplicateTrackAlias and left the close to its callers. Session.Subscribe and AcceptPublish returned the error, and the relay answered the PUBLISH with REQUEST_ERROR MALFORMED_TRACK. RegisterInboundTrack now closes the session itself. A PUBLISH registers its alias before REQUEST_OK, while it is still Pending (Publisher), so objects that arrive before PUBLISH_OK route (§10.11). A second PUBLISH that reuses that alias for another Track therefore closes the session too. The publisher has broken §11.1's "MUST NOT be used ... simultaneously" either way. Releasing an alias when a session-layer Subscription or IncomingPublication ends is a separate change; it stays in the backlog. Verified red first: TestRegisterInboundTrackAlias, the four SUBSCRIBE_OK/PUBLISH orderings in TestDuplicateTrackAliasCloses, and TestPublish_DuplicateAliasClosesSession. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rminated (§5.1, §11.1)
§11.1 closes the session only when a Track Alias is reused for "a different
Track with an Established subscription". A session-layer Subscription or
IncomingPublication never released its alias, so after its subscription
ended, a publisher's legitimate reuse closed the session with
DUPLICATE_TRACK_ALIAS.
A subscription is Terminated by the subscriber's STOP_SENDING or by the
publisher's "PUBLISH_DONE and closing the stream" (§5.1). The handle
releases its alias once, on the first of these:
- its own Close or its broker's Close;
- the publisher's FIN, read through the handle (it now overrides Read)
or by its broker's Serve;
- Serve cancelling the stream.
An onMsg stop does not release: the subscription may still be live. Once
the publisher's FIN has been read, Close FINs rather than resets (§3.3.2).
The release is once per handle, because subscriptions to one Track may
share an alias (§5.1) and each holds its own registration.
The handle keeps the alias and an atomic flag, and the broker points back
at the handle, so the change adds no closures. BenchmarkControlRoundTrip
goes from 46 to 47 allocs/op: its Close now releases the alias, so each
iteration registers it anew, which allocates the channel that wakes
awaitInboundTrack.
AcceptPublish also releases the alias when its REQUEST_OK cannot be
written: that PUBLISH never became Established.
The relay's own SUBSCRIBE now reads through the Subscription's broker:
registry.NewUpstreamSub takes the broker, and the relay's manual
UnregisterInboundTrackAlias on that path is gone. The accepted-PUBLISH
path still registers and releases by hand.
Verified red first:
- TestAliasReleasedWhenTerminated (5 terminations x 2 paths),
TestCloseAfterPublisherFinFINs and
TestAcceptPublishWriteFailureReleasesAlias at HEAD;
- TestAliasReleasedOncePerSubscription against a release that is not
once-only (no CompareAndSwap);
- the "read" and Close-FIN cases against mutants without the Read
override and without the broker's peerFinished;
- TestSubscribe_UpstreamAliasReusableAfterTeardown with the broker
release removed.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…est stream (§10.4) "The endpoint MUST close the session with a PROTOCOL_VIOLATION if it receives more than one GOAWAY on the control stream or on a single request stream", and "If a server receives a GOAWAY with a non-zero New Session URI Length it MUST close the session with a PROTOCOL_VIOLATION". The control stream already enforced both; request streams let any number of GOAWAYs through to the reader. session.RequestGoaways is a zero-value checker for one request stream. Its URI rule is the one the control stream uses (checkGoawayURI). RequestBroker.Serve and the relay's readRequestStream (the downstream SUBSCRIBE, FETCH and namespace streams) keep one per stream, on the stack, so the readers allocate nothing more. A single GOAWAY still reaches the reader. Migrating the request (a SHOULD) is not done. Still open, and recorded in STATUS.md: a GOAWAY that arrives before a request's initial response, or on a SUBSCRIBE_TRACKS stream read with ReadPublishSkipped, is an error rather than being checked. Serve's REQUEST_UPDATE handling moved into receiveUpdate, unchanged, to keep Serve under the gocyclo limit. The relay test asserts only that the session closes: the in-process pipe does not carry the close code to the peer. The session tests pin PROTOCOL_VIOLATION. Verified red first: TestRequestStreamGoawayViolationCloses (a second GOAWAY to the client, a second to the server, a URI to the server) and the two violating TestRelay_RequestStreamGoaway cases. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A Redirect was dropped after parsing. The session did not close on the
two cases §10.6.1 makes fatal, and the application could not follow
one:
- "If a server receives a Redirect with a non-zero Connect URI Length
it MUST close the session with a PROTOCOL_VIOLATION";
- a non-empty Track Name "in a Redirect for a namespace-scoped request"
(SUBSCRIBE_NAMESPACE, PUBLISH_NAMESPACE, SUBSCRIBE_TRACKS) MUST
close the session with a PROTOCOL_VIOLATION.
RequestRejectedError now carries the Redirect, and every place a
REQUEST_ERROR is read enforces both rules:
- awaitRequestResponse and AwaitPublishOK;
- an Update's answer, read directly or routed by the broker;
- an unsolicited REQUEST_ERROR reaching Serve's callback.
An answer to a REQUEST_UPDATE gets only the Connect URI rule, because
its reader does not know the request.
Request.Reject used to refuse the REDIRECT code outright. It now sends
one with its Redirect. It still refuses a code/Redirect mismatch
(§10.6.2), and anything the peer would have to close for. An
UpdateHandler's REDIRECT is sent as INTERNAL_ERROR, since §10.6.2 does
not list REQUEST_UPDATE among the requests REDIRECT answers. Before,
that REDIRECT went out without its block and the peer closed the
session on the malformed message.
The relay still turns an upstream REDIRECT into INTERNAL_ERROR. Its
readRequestStream does not check a REQUEST_ERROR arriving after the
response; that gap is recorded in STATUS.md.
Verified red first:
- TestReceivedRedirectViolationCloses (5 paths),
TestReceivedRedirectViolationClosesOnUpdate (3 modes) and
TestUpdateHandlerRedirectSentAsInternalError at HEAD;
- TestRedirectFollowable and TestRejectRefusesSessionFatalRedirect need
the new field, so they were checked against mutants instead: without
Reject's sender guard, and without Serve's pre-route check (which
fails exactly the unsolicited case).
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…or SUBSCRIBE_TRACKS (§10.19, §10.20) "If the subscriber receives any message other than a REQUEST_OK or a REQUEST_ERROR as the first message on the response half of the stream, then it MUST close the session with a PROTOCOL_VIOLATION." Before, the request just failed. It still does, and the session now closes too. Other requests keep only the failed request. §5.1 and §6.2 put their first-response MUST on the sender and give the receiver no MUST to close. TestOtherFirstResponseKeepsSession pins that carve-out for SUBSCRIBE and PUBLISH_NAMESPACE: it fails if every request type closes. Assumption: a GOAWAY sent as the first message also closes the session. §10.4 says "A GOAWAY MAY also be sent on a request stream", but §10.19/§10.20 say "any message other than" REQUEST_OK/REQUEST_ERROR. We take the literal MUST, so a peer that migrates a SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS with a GOAWAY before answering it is disconnected. If the draft means otherwise, the GOAWAY case moves to the §10.4 check. Verified red first: all six TestNamespaceScopedFirstResponseCloses cases (two requests, each answered with NAMESPACE, GOAWAY or SUBSCRIBE_OK). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…LISH_OK (§5.1)
"A publisher MUST send exactly one SUBSCRIBE_OK or REQUEST_ERROR in
response to a SUBSCRIBE. A subscriber MUST send exactly one PUBLISH_OK
or REQUEST_ERROR in response to a PUBLISH. The peer SHOULD close the
session with a protocol error if it receives more than one." We follow
the SHOULD.
The check applies only where this side sent the request, because only
there has the peer already answered.
- Session: RequestBroker.Serve on a Subscription's broker, or on the
broker of a Publication from Session.Publish. A SUBSCRIBE_OK on a
Subscription closes. A REQUEST_OK or REQUEST_ERROR closes if no
REQUEST_UPDATE has been sent. PUBLISH_OK is a REQUEST_OK on the wire,
so it can't be told apart from an update's answer; and §10.9's
coalescing makes counting unsafe once an update is out. After an
Update, including one that gave up, such a response still reaches
the callback.
- Relay: readSubscribeUpdates on a forwarded PUBLISH, where the relay
sends no REQUEST_UPDATE, closes on a REQUEST_OK or REQUEST_ERROR.
- Streams this side answered keep HEAD's behaviour, and so does a
plain NewRequestBroker. On those the peer is the requester, so none
of these is a second response.
Verified red first:
- at HEAD: TestSecondResponseCloses (a second SUBSCRIBE_OK, and a
REQUEST_ERROR after SUBSCRIBE_OK or PUBLISH_OK, a second PUBLISH_OK)
and TestRelay_SecondResponseCloses (REQUEST_OK and REQUEST_ERROR);
- against mutants: TestLateUpdateAnswerKeepsSession (never set
updated), TestResponderStreamResponseKeepsSession (every broker
checked) and TestSubscribeOKOnPublishKeepsSession (SUBSCRIBE_OK
checked on a PUBLISH).
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… brokers (§10.19, §10.9, §10.10)
Apps read SUBSCRIBE_NAMESPACE streams with raw message.Parse (the Example
did too), so the session could enforce none of these MUSTs:
- "If a subscriber receives a NAMESPACE_DONE before the corresponding
NAMESPACE, it MUST close the session with a 'PROTOCOL_VIOLATION'"
(§10.19);
- a PUBLISH_STATE_NOTIFY for anything but a subscription (§10.10);
- a REQUEST_UPDATE from other than the request's sender (§10.9).
ReadPublishSkipped reported such messages as ordinary errors, and treated
a legal GOAWAY as one too.
NamespaceSubscription and TrackSubscription now embed requestHandle,
like Subscription, and gain Broker() and Update(). Close is unchanged.
Their brokers:
- refuse PUBLISH_STATE_NOTIFY and REQUEST_UPDATE from the publisher;
- on a SUBSCRIBE_NAMESPACE, track the full namespaces NAMESPACE
announced, across Serve calls, and close on a NAMESPACE_DONE for
one that is not announced.
Suffixes resolve against the prefix in force, which switches at the
REQUEST_OK that accepts a TRACK_NAMESPACE_PREFIX update (§10.9.2). An
Update that gives up drops its waiter (the rule from 3341989), so later
answers no longer pair reliably. After that the check stops rather than
close on a conforming peer.
Update on both handles always goes through the broker, since reading
the answer directly would take a NAMESPACE for it. The broker's Serve
must be running for Update to return.
GOAWAY state now lives on the handle, shared by ReadPublishSkipped and
the broker. Serve's read-error handling moved into readFailed to keep it
under the gocyclo limit, and the Example reads through the broker.
Assumption, as chosen: ReadPublishSkipped skips a single GOAWAY instead
of returning it. A caller that would re-issue the request at its New
Session URI (§10.4 SHOULD) reads with the broker, whose Serve hands the
GOAWAY over.
Verified red first:
- at HEAD: the ReadPublishSkipped cases of
TestTrackSubscriptionPeerMessagesClose and TestReadPublishSkippedGoaway;
- on the working tree before its fix:
TestNamespaceSubscriptionLostUpdateShiftsPairing;
- against mutants, for the broker cases that need the new API:
no namespace tracking, a broker that allows publisher messages,
tracking reset per Serve call, no prefix switch, and a lost prefix
update that keeps checking.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.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
1 participant
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.
Works through the "protocol violations that MUST close the session" part of the draft-20 compliance backlog in
STATUS.md. There are eight commits, and each one:-raceandgolangci-lint;moqt-reviewer.Commits
CheckPeerParamsnow validates the values of GROUP_ORDER, FORWARD, INCLUDE_PROPERTIES, LOCATION_FILTER (including end-group overflow) and FILL_PARAMETERS:RegisterInboundTrackcloses the session with DUPLICATE_TRACK_ALIAS. The relay no longer answers REQUEST_ERROR MALFORMED_TRACK.SubscriptionorIncomingPublicationreleases its alias once the subscription is Terminated, so a later reuse is legal:RequestBroker.Serveand in the relay's request readers.RequestRejectedError.Redirectcarries the redirect, andRequest.Rejectcan now send one.NamespaceSubscriptionandTrackSubscriptionget typed brokers:ReadPublishSkipped;ReadPublishSkippedskips a single GOAWAY instead of failing on it.API changes
RequestRejectedError.Redirectis a new field.Request.Rejectnow accepts code REDIRECT when a Redirect is given.session.RequestGoawaysis a new type, for callers that read a request stream themselves.registry.NewUpstreamSubnow takes the broker. The relay's upstream SUBSCRIBE uses theSubscription's own broker.RegisterInboundTrackcloses the session on a duplicate, so callers no longer have to.SubscriptionandIncomingPublicationrelease their Track Alias on termination, so callers no longer unregister it.NamespaceSubscriptionandTrackSubscriptionembed the request handle, so they gainBroker()andUpdate().Updategoes through the broker, so the broker'sServemust be running for it to return.Assumptions to review
ReadPublishSkippedskips a single GOAWAY instead of returning it. A caller that wants to re-issue the request at the GOAWAY's URI (§10.4 SHOULD) should read with the broker, which hands the GOAWAY to its callback.Updategives up. The dropped waiter makes later answers pair unreliably (the rule from 3341989), so where a prefix switch lands is no longer known.Benchmarks
BenchmarkControlRoundTripgoes from 46 to 47 allocs/op. ItsClosenow releases the alias, so each iteration re-registers it and allocates theawaitInboundTrackwake channel. Everything else matches the draft-20 base.Still in the backlog
These are listed in
STATUS.md:Test plan
go test ./...go test -race ./pkg/moqt/session/... ./pkg/relay/...golangci-lint run ./...make bench-quickallocs/op compared against the draft-20 base🤖 Generated with Claude Code