fix: draft-20 publication and defaults backlog (§3.3.4, §10.3.1.4, §2.5.1, §10.12, §10.2.17) - #113
Merged
Merged
Conversation
…3.3.4)
"The application SHOULD use a relevant error code when resetting or
sending STOP_SENDING on any stream." A rejected request stopped reading
with INTERNAL_ERROR after its REQUEST_ERROR was written. It now uses
CANCELLED ("The stream was cancelled by either endpoint"), in:
- session: Request.Reject, and so RejectError and AcceptRequest's
pre-Request rejections;
- relay: DownstreamSub's REQUEST_ERROR for a SUBSCRIBE terminated
before its SUBSCRIBE_OK.
A REQUEST_ERROR that cannot be written is still a failure and resets
the stream with INTERNAL_ERROR. The relay path used to label that case
CANCELLED too; it now resets like Reject does.
Verified red first: TestRejectStopsSendingWithCancelled, the new
STOP_SENDING assertion in
TestDownstreamSub_TerminateBeforeOKAnswersWithRequestError, and
TestDownstreamSub_TerminateBeforeOKWriteFailureResets.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…0.3.1.4) Writer.KVPairs sorted by Type for the delta encoding with the unstable slices.SortFunc. Past 12 pairs, pdqsort can reorder pairs of the same Type. Which of a SETUP's AUTHORIZATION TOKEN REGISTERs fit the peer's MAX_AUTH_TOKEN_CACHE_SIZE depends on their order (§10.3.1.3, §10.3.1.4), and heldSetupAliases replays them in the order the caller gave. So the sender's SetupTokenAliases could name aliases the peer never held, and miss ones it did. It now uses slices.SortStableFunc. The sort is still in place with no allocations, and bench-quick allocs/op match the baseline for every codec benchmark. The other KVPairs users, Track Properties and LOC properties, gain order-preserving encoding as well. Any order within a Type is valid on the wire, so none of them relied on the old order. Verified red first: TestKVPairsKeepOrderWithinAType (wire) and TestSetupTokensHeldMatchesPeerWithManyTokens (session). The session test uses 24 tokens interleaved with other options; unpatched, the server held alias 17 and others while the client believed it held 1-6. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ult (§2.5.1)
"When an endpoint receives a Mandatory Track Property in PUBLISH,
SUBSCRIBE_OK, or FETCH_OK that it does not understand, it MUST NOT
process or forward that track." A session without
WithKnownMandatoryTrackProperties skipped the check entirely, and the
tests called that pass-through "the correct default for relays and
forwarding endpoints", which is what the draft forbids.
A nil or empty set now means that no Mandatory Track Property is
known:
- a PUBLISH carrying one is refused with UNSUPPORTED_EXTENSION;
- a SUBSCRIBE_OK, FETCH_OK or TRACK_STATUS_OK carrying one fails the
request with *ErrUnsupportedMandatoryTrackProperty.
The relay is unchanged: it always passed its own set.
The same check parses the properties, so Track Properties that don't
parse now fail these requests by default too. Before, that happened
only when the option was set. A Key-Value-Pair that "cannot be parsed"
makes the track malformed (§12.7), and a subscriber "MUST cancel any
corresponding subscription or fetches" (§2.4.2), so the old default did
not comply either.
Assumption: TRACK_STATUS_OK is checked like SUBSCRIBE_OK. §2.5.1 does
not list it, but §10.15 says it carries the Track Properties "it would
have set in a SUBSCRIBE_OK". A plain client's TrackStatus on such a
track now returns the error instead of the OK.
Verified red first: TestDefaultRefusesUnknownMandatoryTrackProperty
(SUBSCRIBE_OK, FETCH_OK, TRACK_STATUS_OK, PUBLISH) replaces the three
*DefaultNoEnforcement tests; all four cases failed on the unpatched
code.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…very stream (§10.12, §11.4.3)
"A sender MUST NOT send PUBLISH_DONE until it has closed all streams it
will ever open", and the Stream Count is "the number of data streams
the publisher opened for this subscription". Publication.Done sent
PUBLISH_DONE with subgroups still open; writes after it still went
through; and subgroups opened concurrently with it could be missing
from the count.
As chosen: Done resets the open subgroups with CANCELLED (§11.4.3,
ending early), refuses later opens and writes, gives an exact Stream
Count, and waits for nothing but the header writes it cancels.
- OpenSubgroup and Done are ordered under a lock. Done marks the
publication ended, cancels in-flight header writes, waits for them,
then resets every subgroup still open and writes PUBLISH_DONE.
- A stream counts once its whole header is written, since the peer can
attribute it from then on; that includes one Done reset just after.
The subscriber may still see fewer streams than counted, when a
reset discards a header already written, which §10.12 has
subscribers allow for.
- Close and Cancel drop a subgroup from the reset set through a hook
bound to the registered subgroup, which WithDeliveryTimeouts copies
share. A subgroup FINished through such a copy, which is the relay's
path, is therefore never reset after its FIN.
- As chosen: before each reset, including the header write Done
cancels, what was written is marked reliable (RESET_STREAM_AT), so
the header survives and the subscriber can "accurately account for
reset data streams when handling PUBLISH_DONE" (§11.4.3). The
relay does the same.
- A subgroup's WriteObject after Done returns ErrPublicationEnded.
OpenSubgroupContext keeps its public behaviour on top of a new internal
openSubgroup.
Verified red first:
- TestPublicationDoneResetsOpenSubgroups and
TestPublicationDoneStreamCountExact at HEAD (the count was short by
5 to 8 under the race);
- TestPublicationDoneKeepsFINOfDeliveryTimeoutCopy against the first
version of this change;
- TestPublicationDoneResetKeepsHeaderReliable before the reliable
boundary.
The count test passes 30x and under -race; allocs are unchanged.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…BJECT it announced (§10.2.17) "If Objects have been published on this Track the Publisher MUST include this parameter" (§10.2.17), and "The REQUEST_UPDATE_OK will include the LARGEST_OBJECT parameter" (§10.9.1). A session Publication reported one only for Objects it wrote itself through OpenSubgroup, not the one its own SUBSCRIBE_OK or PUBLISH had already announced. Its first REQUEST_UPDATE_OK could therefore omit LARGEST_OBJECT, or report a smaller one. newPublication now seeds the Publication's largest Location from the parameters this side sent: the SUBSCRIBE_OK in AcceptSubscribe, and the PUBLISH in Publish. A larger Object written later still wins; a smaller one does not. Verified red first: TestPublicationUpdateOKCarriesAnnouncedLargest (SUBSCRIBE_OK and PUBLISH cases) on the unpatched code. The case where a smaller Object is written was checked against a mutant that stops keeping the maximum. 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.
This works through the "publication and defaults" batch of the draft-20 compliance backlog in
STATUS.md. It is five commits, one per item. Each one has a regression test that failed before the fix, at HEAD or against a mutant, and each went throughmoqt-reviewer. Every commit passes-raceand lint.Commits
Request.Rejectand the relay's REQUEST_ERROR for a SUBSCRIBE terminated before its SUBSCRIBE_OK. When the REQUEST_ERROR can't be written, the stream is still reset with INTERNAL_ERROR, as a real failure.wire.Writer.KVPairssorted with the unstableslices.SortFunc. With enough AUTHORIZATION TOKEN options mixed among other options, the peer's cache held different REGISTERs thanSetupTokenAliasesreported. A session test with 24 interleaved tokens reproduced it. The sort is nowslices.SortStableFunc; allocs/op match the baseline.WithKnownMandatoryTrackProperties, no Mandatory Track Property is known:*ErrUnsupportedMandatoryTrackProperty.Publication.Donecloses every stream before PUBLISH_DONE (§10.12, §11.4.3). Done:WriteObjectcalls withErrPublicationEnded;OpenSubgroupraces it, and waits only for the header writes it cancels.OpenSubgroupcounted.Behaviour changes and assumptions to review
WithKnownMandatoryTrackPropertiesused to pass Mandatory Track Properties through. §2.5.1 forbids processing or forwarding a track with one the endpoint doesn't understand. The three tests that pinned pass-through as "the correct default for relays" are replaced.API changes
WithKnownMandatoryTrackProperties: nil or empty now means "none known". It no longer means "don't check".Publication:OutgoingSubgroupStream.WriteObject*returnsErrPublicationEndedafterDone.Seen while testing, unrelated
Each of these failed once under full-suite load and passed on reruns:
TestSubscribeTracks_ForwardsTrackGainedBySubscribe(relay; reported in fix(message,session): reject invalid FETCH Serialization Flags before reading fields (§11.4.4) #112);TestConnConformance(wtconn).Test plan
go test ./...go test -race ./pkg/moqt/session/... ./pkg/relay/...golangci-lint run ./...make bench-quick: allocs/op unchanged🤖 Generated with Claude Code