From efc8d9d81df5c8e1600510ddc0f175b1ef04267f Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 20:00:11 +0500 Subject: [PATCH 1/9] =?UTF-8?q?fix(message,session,relay):=20close=20the?= =?UTF-8?q?=20session=20on=20every=20session-fatal=20parameter=20value=20(?= =?UTF-8?q?=C2=A75.1.2,=20=C2=A710.2.8,=20=C2=A710.2.18,=20=C2=A71.4.3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- STATUS.md | 22 ++----- pkg/moqt/message/fill.go | 9 ++- pkg/moqt/message/filter.go | 10 ++- pkg/moqt/message/param_scope.go | 38 +++++++++-- pkg/moqt/message/params.go | 6 +- pkg/moqt/session/param_scope_test.go | 98 ++++++++++++++++++++++++++++ pkg/moqt/session/request.go | 19 +++--- pkg/relay/handler_fetch.go | 28 ++------ pkg/relay/handler_fill.go | 17 ++--- pkg/relay/handler_namespace.go | 35 +++------- pkg/relay/handler_publish.go | 6 -- pkg/relay/handler_subscribe.go | 91 ++++---------------------- pkg/relay/helpers_test.go | 8 +++ pkg/relay/param_values_test.go | 78 ++++++++++++++++++++++ pkg/relay/publish_done_test.go | 14 ++-- pkg/relay/session_update_test.go | 11 +--- pkg/relay/subscribe_tracks_test.go | 16 +++-- 17 files changed, 303 insertions(+), 203 deletions(-) create mode 100644 pkg/relay/param_values_test.go diff --git a/STATUS.md b/STATUS.md index 6ef3b075..87fa8048 100644 --- a/STATUS.md +++ b/STATUS.md @@ -166,8 +166,8 @@ By package, bottom-up along the dependency stack: | 10.2.5 | FILL_TIMEOUT | 0x0A | DONE | The budget for a FETCH's or fill's upstream FETCH, its response included: when it runs out, what arrived is served and the rest is an End of Timed-Out Range; 0 asks no upstream. Default 5s. | | 10.2.6 | RENDEZVOUS_TIMEOUT | 0x04 | DONE | | | 10.2.7 | SUBSCRIBER_PRIORITY | 0x20 | DONE | | -| 10.2.8 | GROUP_ORDER | 0x22 | DONE | Ascending/Descending validated. | -| 10.2.9 | LOCATION_FILTER | 0x21 | DONE | Overflow-checked. | +| 10.2.8 | GROUP_ORDER | 0x22 | DONE | A value outside {1, 2} closes the session, wherever it appears, FILL_PARAMETERS included. | +| 10.2.9 | LOCATION_FILTER | 0x21 | DONE | An end Group overflowing 2^64-1 closes the session with PROTOCOL_VIOLATION (§5.1.2); a value that does not parse, with KEY_VALUE_FORMATTING_ERROR (§1.4.3). | | 10.2.10 | SUBGROUP_FILTER | 0x25 | DONE | Enforced per object in the fanout/FETCH. | | 10.2.11 | OBJECTID_FILTER | 0x26 | DONE | Enforced per object in the fanout/FETCH. | | 10.2.12 | PRIORITY_FILTER | 0x27 | DONE | Enforced per object (subgroup priority); >255 rejected INVALID_FILTER. | @@ -176,7 +176,7 @@ By package, bottom-up along the dependency stack: | 10.2.15 | FILL_PARAMETERS | 0x23 | PARTIAL| Inner Table 6 scope and duplicates checked; omitted Range Filters are not inherited from the subscription (see §5.1.3). | | 10.2.16 | EXPIRES | 0x08 | DONE | | | 10.2.17 | LARGEST_OBJECT | 0x09 | DONE | Monotonic constraint applied. | -| 10.2.18 | FORWARD | 0x10 | DONE | | +| 10.2.18 | FORWARD | 0x10 | DONE | A value above 1 closes the session, in every message that may carry it. | | 10.2.19 | NEW_GROUP_REQUEST | 0x32 | DONE | | | 10.2.20 | TRACK_NAMESPACE_PREFIX | 0x34 | DONE | Applied on REQUEST_UPDATE; SUBSCRIBE_NAMESPACE reconciles its announced set. | | 10.2.21 | INCLUDE_PROPERTIES | 0x35 | DONE | A value other than 0 or 1 closes the session. With 0 the relay sends empty Track Properties in SUBSCRIBE_OK, FETCH_OK, TRACK_STATUS_OK and forwarded PUBLISH, and writes the priority inline on that subscription's subgroups and datagrams, since the subscriber cannot inherit DEFAULT_PUBLISHER_PRIORITY. | @@ -333,10 +333,6 @@ Known protocol gaps, roughly ordered by how load-bearing they are: self-limit *outbound* REQUEST_UPDATEs against a peer's advertised value for the same reason: `UpdateRequest`/`RequestBroker.Update` are synchronous write-then-read, so they never exceed any limit ≥ 1. -- **Out-of-range GROUP_ORDER on FETCH (§10.2.8)** — the SUBSCRIBE and - SUBSCRIBE_TRACKS paths now close the session with PROTOCOL_VIOLATION on an - out-of-range GROUP_ORDER/FORWARD (§10.2.8/§10.2.18), but the FETCH path - still reads a bad GROUP_ORDER as Ascending pending the same promotion. - **Late publisher pickup (§9.5)** — multiple publishers per track are merged and deduplicated, and a local PUBLISH_NAMESPACE that arrives after a track's upstream set is established is SUBSCRIBEd for each matching track. Two @@ -553,15 +549,8 @@ Session layer: Validation (values that MUST close the session): -- GROUP_ORDER on PUBLISH, inside FILL_PARAMETERS, and in `AcceptSubscribe` / - `AcceptPublish` (§10.2.8); the FETCH case is the Limitation above. -- FORWARD in PUBLISH_STATE_NOTIFY, in a publisher's REQUEST_UPDATE, and in - `AcceptPublish` (§10.2.18). - DEFAULT_PUBLISHER_GROUP_ORDER outside {1, 2} and DYNAMIC_GROUPS above 1 in Track Properties (§12.5, §12.6). -- A LOCATION_FILTER whose StartGroup + EndGroupDelta overflows: REQUEST_ERROR - MALFORMED_TRACK on SUBSCRIBE, REQUEST_UPDATE and SUBSCRIBE_TRACKS, INVALID_FILTER - on FETCH, a fill reset in FILL_PARAMETERS (§5.1.2). Relay: @@ -599,11 +588,10 @@ Relay: Documentation: -- Limitations: "Duplicate Objects … are not compared" is stale; the FETCH - GROUP_ORDER entry omits PUBLISH and FILL_PARAMETERS; the LOC entry names +- Limitations: "Duplicate Objects … are not compared" is stale; the LOC entry names `PropAudioLevel = 0x0A` (it is 0x0C); "Handles the application reads itself" says `CheckPeerParams` checks roles; "Inbound GOAWAY" omits request streams. -- Table rows 10.2.6, 10.2.8, 10.2.9, 10.2.15, 10.2.18, 10.2.21, 12.3, +- Table rows 10.2.6, 10.2.15, 10.2.21, 12.3, 12.5 and 12.6 overstate what is done (see the items above), and the package summary still lists joining FETCH. - `session/namespace.go` says NAMESPACE / NAMESPACE_DONE go on a diff --git a/pkg/moqt/message/fill.go b/pkg/moqt/message/fill.go index 2438d1ad..abe34548 100644 --- a/pkg/moqt/message/fill.go +++ b/pkg/moqt/message/fill.go @@ -1,6 +1,7 @@ package message import ( + "errors" "fmt" "slices" @@ -47,7 +48,13 @@ func FillParametersFromParam(ps Parameters) (inner Parameters, ok bool, err erro return nil, false, nil } if err := inner.parse(wire.NewReader(p.Bytes)); err != nil { - return nil, true, fmt.Errorf("moqt/message: FILL_PARAMETERS: %w", err) + // §10.2: an unknown parameter, or a Type past 2^64-1, is a + // PROTOCOL_VIOLATION here as in any message; anything else is a + // value that does not parse (§1.4.3). + if errors.Is(err, ErrUnknownParameter) || errors.Is(err, errParamTypeOverflow) { + return nil, true, fmt.Errorf("moqt/message: FILL_PARAMETERS: %w", err) + } + return nil, true, fmt.Errorf("%w: FILL_PARAMETERS: %w", ErrValueFormatting, err) } for _, ip := range inner { if !slices.Contains(fillParamsAllowed, ip.Type) { diff --git a/pkg/moqt/message/filter.go b/pkg/moqt/message/filter.go index 86eda102..ba0c6106 100644 --- a/pkg/moqt/message/filter.go +++ b/pkg/moqt/message/filter.go @@ -1,6 +1,7 @@ package message import ( + "errors" "fmt" "math" @@ -78,13 +79,16 @@ func (f *LocationFilter) Validate() error { return fmt.Errorf("moqt/message: LOCATION_FILTER has %d fields, want 0-4 (§5.1.2)", f.Fields) } if f.HasEnd() && f.StartGroup > math.MaxUint64-f.EndGroupDelta { - return fmt.Errorf( - "moqt/message: LOCATION_FILTER end group overflow (start=%d delta=%d) (PROTOCOL_VIOLATION §5.1.2)", - f.StartGroup, f.EndGroupDelta) + return fmt.Errorf("%w (start=%d delta=%d)", errEndGroupOverflow, f.StartGroup, f.EndGroupDelta) } return nil } +// errEndGroupOverflow is a LOCATION_FILTER whose StartGroup + EndGroupDelta +// exceeds 2^64 - 1: a PROTOCOL_VIOLATION (§5.1.2), unlike a value that does +// not parse ([ErrValueFormatting]). +var errEndGroupOverflow = errors.New("moqt/message: LOCATION_FILTER end group overflow (PROTOCOL_VIOLATION §5.1.2)") + // Start resolves the first Location that passes the filter, given the // publisher's current Largest Object. hasLargest is false before anything has // been published on the track, which §5.1.2 pins to {0, 0}. diff --git a/pkg/moqt/message/param_scope.go b/pkg/moqt/message/param_scope.go index 0b47a002..a13f3a75 100644 --- a/pkg/moqt/message/param_scope.go +++ b/pkg/moqt/message/param_scope.go @@ -218,10 +218,18 @@ func (e *ParamScopeError) Error() string { return fmt.Sprintf("moqt/message: %s not allowed in %s (PROTOCOL_VIOLATION §10.2.1)", e.Type, e.Scope) } +// ErrValueFormatting marks a parameter whose value does not match the +// serialization its type defines. §1.4.3: the receiver "MUST close the session +// with error code KEY_VALUE_FORMATTING_ERROR". +var ErrValueFormatting = errors.New("moqt/message: value does not match its type's serialization") + // CheckScope reports the first parameter of ps not allowed in a message of the // given scope (§10.2.1) or repeated where it may not be (§10.2), and validates -// the FILL_PARAMETERS (§10.2.15) and INCLUDE_PROPERTIES (§10.2.21) values. -// Every error is a session-level PROTOCOL_VIOLATION. +// the values a receiver must close the session for, in ps and inside its +// FILL_PARAMETERS (§10.2.15): GROUP_ORDER (§10.2.8), FORWARD (§10.2.18), +// INCLUDE_PROPERTIES (§10.2.21) and LOCATION_FILTER (§5.1.2). Every error is a +// session-level PROTOCOL_VIOLATION, except one wrapping [ErrValueFormatting], +// a KEY_VALUE_FORMATTING_ERROR. func (ps Parameters) CheckScope(scope ParamScope) error { for _, p := range ps { allowed := paramScopes[p.Type] @@ -236,13 +244,35 @@ func (ps Parameters) CheckScope(scope ParamScope) error { if t, dup := ps.firstDuplicate(); dup { return &ParamScopeError{Type: t, Scope: scope, Duplicate: true} } - if _, _, err := FillParametersFromParam(ps); err != nil { + inner, _, err := FillParametersFromParam(ps) + if err != nil { return err } + if err := inner.checkValues(); err != nil { + return fmt.Errorf("FILL_PARAMETERS: %w", err) + } + return ps.checkValues() +} + +// checkValues validates the values in ps a receiver must close the session +// for; see [Parameters.CheckScope]. +func (ps Parameters) checkValues() error { + if p, ok := ps.Find(ParamGroupOrder); ok && + GroupOrder(p.Byte) != GroupOrderAscending && GroupOrder(p.Byte) != GroupOrderDescending { + return fmt.Errorf("moqt/message: GROUP_ORDER value %d outside {1,2} (PROTOCOL_VIOLATION §10.2.8)", p.Byte) + } + if p, ok := ps.Find(ParamForward); ok && p.Byte > 1 { + return fmt.Errorf("moqt/message: FORWARD value %d outside {0,1} (PROTOCOL_VIOLATION §10.2.18)", p.Byte) + } if _, err := IncludePropertiesFromParam(ps); err != nil { return err } - return nil + switch _, err := LocationFilterFromParam(ps); { + case err == nil, errors.Is(err, errEndGroupOverflow): + return err + default: + return fmt.Errorf("%w: LOCATION_FILTER: %w", ErrValueFormatting, err) + } } // firstDuplicate reports the first parameter type repeated in ps where its diff --git a/pkg/moqt/message/params.go b/pkg/moqt/message/params.go index 11211fde..8f66f34d 100644 --- a/pkg/moqt/message/params.go +++ b/pkg/moqt/message/params.go @@ -358,6 +358,10 @@ func (ps Parameters) append(w *wire.Writer) { } } +// errParamTypeOverflow is a parameter Type delta taking the Type past 2^64-1: +// a PROTOCOL_VIOLATION (§10.2), unlike a value that does not parse. +var errParamTypeOverflow = errors.New("moqt/message: parameter type delta overflow") + // parse reads a Number-of-Parameters varint followed by that many parameters // from r. func (ps *Parameters) parse(r *wire.Reader) error { @@ -376,7 +380,7 @@ func (ps *Parameters) parse(r *wire.Reader) error { return err } if delta > ^uint64(0)-prev { - return errors.New("moqt/message: parameter type delta overflow") + return errParamTypeOverflow } t := prev + delta p := Parameter{Type: ParamID(t)} diff --git a/pkg/moqt/session/param_scope_test.go b/pkg/moqt/session/param_scope_test.go index 67dc9176..c9aaa1da 100644 --- a/pkg/moqt/session/param_scope_test.go +++ b/pkg/moqt/session/param_scope_test.go @@ -1,9 +1,11 @@ package session_test import ( + "math" "testing" "time" + "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/session" ) @@ -248,3 +250,99 @@ func TestIncludePropertiesOutOfRangeCloses(t *testing.T) { }) } } + +// TestParamValueOutOfRangeCloses: a value the draft makes session-fatal closes +// the receiver, in an opener and inside its FILL_PARAMETERS: GROUP_ORDER +// outside {1, 2} (§10.2.8), FORWARD above 1 (§10.2.18), and a LOCATION_FILTER +// whose end Group overflows (§5.1.2) are PROTOCOL_VIOLATION; a LOCATION_FILTER +// or FILL_PARAMETERS that does not parse is KEY_VALUE_FORMATTING_ERROR (§1.4.3). +func TestParamValueOutOfRangeCloses(t *testing.T) { + t.Parallel() + overflow := message.AbsoluteRangeFilter(message.Location{Group: math.MaxUint64}, 1) + unparsable := message.BytesParam(message.ParamLocationFilter, []byte{0xFF}) + fill := func(inner ...message.Parameter) message.Parameter { return message.FillParametersParam(inner) } + cases := []struct { + name string + msg message.WithRequestID + want moqt.SessionErrorCode + }{ + {"GROUP_ORDER 0 in SUBSCRIBE", &message.Subscribe{ + Name: []byte("t"), + Parameters: message.Parameters{ + message.ByteParam(message.ParamGroupOrder, 0), + }, + }, moqt.SessionProtocolViolation}, + {"GROUP_ORDER 3 in PUBLISH", &message.Publish{ + Name: []byte("t"), + TrackAlias: 1, + Parameters: message.Parameters{ + message.ByteParam(message.ParamGroupOrder, 3), + }, + }, moqt.SessionProtocolViolation}, + {"GROUP_ORDER 5 in FETCH", &message.Fetch{ + Name: []byte("t"), + Parameters: message.Parameters{ + message.ByteParam(message.ParamGroupOrder, 5), + }, + }, moqt.SessionProtocolViolation}, + {"GROUP_ORDER 7 inside FILL_PARAMETERS", &message.Subscribe{ + Name: []byte("t"), + Parameters: message.Parameters{ + fill(message.ByteParam(message.ParamGroupOrder, 7)), + }, + }, moqt.SessionProtocolViolation}, + {"FORWARD 2 in PUBLISH", &message.Publish{Name: []byte("t"), TrackAlias: 1, + Parameters: message.Parameters{message.ByteParam(message.ParamForward, 2)}}, moqt.SessionProtocolViolation}, + {"LOCATION_FILTER overflow in SUBSCRIBE", &message.Subscribe{Name: []byte("t"), + Parameters: message.Parameters{overflow}}, moqt.SessionProtocolViolation}, + {"LOCATION_FILTER overflow in FETCH", &message.Fetch{Name: []byte("t"), + Parameters: message.Parameters{overflow}}, moqt.SessionProtocolViolation}, + {"LOCATION_FILTER overflow inside FILL_PARAMETERS", &message.SubscribeTracks{TrackNamespacePrefix: videoNS, + Parameters: message.Parameters{fill(overflow)}}, moqt.SessionProtocolViolation}, + {"LOCATION_FILTER that does not parse", &message.Subscribe{Name: []byte("t"), + Parameters: message.Parameters{unparsable}}, moqt.SessionKeyValueFormattingError}, + {"unknown parameter inside FILL_PARAMETERS", &message.Subscribe{Name: []byte("t"), + Parameters: message.Parameters{fill(message.VarintParam(0x3E, 1))}}, moqt.SessionProtocolViolation}, + {"FILL_PARAMETERS that does not parse", &message.Subscribe{Name: []byte("t"), + Parameters: message.Parameters{message.BytesParam(message.ParamFillParameters, []byte{0x05})}}, + moqt.SessionKeyValueFormattingError}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + client, server := openPair(t) + go func() { + tc.msg.SetRequestID(0) + _, _ = session.OpenRequestForTest(client, tc.msg) + }() + _, _ = server.AcceptRequest(t.Context()) + requireClosedCode(t, server, tc.want) + }) + } +} + +// TestParamValueOutOfRangeInFollowupsCloses: the same holds for the +// follow-ups a broker reads: FORWARD in PUBLISH_STATE_NOTIFY and in a +// REQUEST_UPDATE (§10.2.18). +func TestParamValueOutOfRangeInFollowupsCloses(t *testing.T) { + t.Parallel() + bad := message.Parameters{message.ByteParam(message.ParamForward, 3)} + t.Run("PUBLISH_STATE_NOTIFY", func(t *testing.T) { + t.Parallel() + client, server := openPair(t) + sub, pub := subscribePair(t, client, server) + go func() { _ = message.Marshal(pub.Stream, &message.PublishStateNotify{Parameters: bad}) }() + go func() { _ = sub.Broker().Serve(t.Context(), nil) }() + requireClosedProtocolViolation(t, client) + }) + t.Run("REQUEST_UPDATE", func(t *testing.T) { + t.Parallel() + client, server := openPair(t) + sub, pub := subscribePair(t, client, server) + go func() { + _ = message.Marshal(sub.Stream, &message.RequestUpdate{RequestID: client.AllocRequestID(), Parameters: bad}) + }() + go func() { _ = pub.Broker().Serve(t.Context(), nil) }() + requireClosedProtocolViolation(t, server) + }) +} diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index c5927d86..159dee1e 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -674,8 +674,10 @@ func (s *Session) UpdateRequest( } // CheckPeerParams checks the Message Parameters of a peer message m against -// scope (§10.2.1) and §10.2's duplicate rule. On a violation it closes the -// session with PROTOCOL_VIOLATION and returns the error. +// scope (§10.2.1), §10.2's duplicate rule and the values the draft makes +// session-fatal (see [message.Parameters.CheckScope]). On a violation it +// closes the session, with KEY_VALUE_FORMATTING_ERROR for a value that does +// not parse (§1.4.3) and PROTOCOL_VIOLATION otherwise, and returns the error. // // The session checks the messages it reads itself; callers that read a // request stream with [message.Parse] call it for what they read. @@ -685,6 +687,10 @@ func (s *Session) CheckPeerParams(scope message.ParamScope, m message.Message) e return nil } if err := params.CheckScope(scope); err != nil { + if errors.Is(err, message.ErrValueFormatting) { + _ = s.Close(moqt.SessionKeyValueFormattingError, err.Error()) + return err + } return s.closeProtocolViolation(err) } return nil @@ -784,19 +790,12 @@ func (r *Request) Reject(rej *RequestRejectedError) error { // MUST be a *message.Subscribe. // // ok may be nil for the all-default reply; a zero TrackAlias is allocated with -// [Session.AllocOutboundTrackAlias]. A FORWARD value above 1 closes the -// session with PROTOCOL_VIOLATION (§10.2.18). +// [Session.AllocOutboundTrackAlias]. func (r *Request) AcceptSubscribe(ok *message.SubscribeOK) (*Publication, error) { sub, isSub := r.First.(*message.Subscribe) if !isSub { return nil, fmt.Errorf("moqt/session: AcceptSubscribe on a %s request", r.First.Type()) } - // §10.2.18. - if f, found := sub.Parameters.Find(message.ParamForward); found && f.Byte > 1 { - resetStream(r.Stream) - return nil, r.s.closeProtocolViolation( - fmt.Errorf("moqt/session: FORWARD value %d in SUBSCRIBE", f.Byte)) - } if ok == nil { ok = &message.SubscribeOK{} } diff --git a/pkg/relay/handler_fetch.go b/pkg/relay/handler_fetch.go index 94b99403..a507d448 100644 --- a/pkg/relay/handler_fetch.go +++ b/pkg/relay/handler_fetch.go @@ -80,12 +80,9 @@ func (h *sessionHandler) handleFetch(ctx context.Context, req *session.Request, // draft-20 moved the FETCH range out of the message and into the // LOCATION_FILTER parameter (§5.1.2), inclusive at both ends. An absent - // filter fetches the whole track up to Largest Object. - filter, err := message.LocationFilterFromParam(msg.Parameters) - if err != nil { - _ = req.RejectError(moqt.RequestInvalidFilter, "relay: malformed LOCATION_FILTER") - return - } + // filter fetches the whole track up to Largest Object. AcceptRequest has + // validated it (see [message.Parameters.CheckScope]). + filter, _ := message.LocationFilterFromParam(msg.Parameters) if filter == nil { filter = &message.LocationFilter{} } @@ -218,22 +215,11 @@ func (h *sessionHandler) handleFetchUpdate(ctx context.Context, req *session.Req } } -// TODO: §10.2.8: an out-of-range GROUP_ORDER MUST close the session, as -// [checkGroupOrderParam] does for SUBSCRIBE; the FETCH path reads it as -// Ascending. -// -// fetchGroupOrder pulls the GROUP_ORDER parameter (§10.2.8) out of a -// FETCH's Parameters list. Defaults to ascending when omitted; the -// FETCH responder uses this to choose between ascending and descending -// traversal through the cache. +// fetchGroupOrder is a FETCH's GROUP_ORDER (§10.2.8): Ascending when omitted. +// AcceptRequest has closed the session on a value outside {1, 2}. func fetchGroupOrder(ps message.Parameters) message.GroupOrder { - p, ok := ps.Find(message.ParamGroupOrder) - if !ok { - return message.GroupOrderAscending - } - g := message.GroupOrder(p.Byte) - if g == message.GroupOrderDescending { - return g + if p, ok := ps.Find(message.ParamGroupOrder); ok { + return message.GroupOrder(p.Byte) } return message.GroupOrderAscending } diff --git a/pkg/relay/handler_fill.go b/pkg/relay/handler_fill.go index ddd002fb..d4a45df6 100644 --- a/pkg/relay/handler_fill.go +++ b/pkg/relay/handler_fill.go @@ -20,9 +20,9 @@ import ( // requestID is the Request ID of the message that asked for the fill; the // FETCH_HEADER carries it, so one subscription can have several fills open. // -// It returns an error only for a malformed FILL_PARAMETERS, which the caller -// MUST turn into a session-level PROTOCOL_VIOLATION (§10.2.15). Any other -// failure resets the fill stream and leaves the subscription unaffected. +// A failure resets the fill stream, leaves the subscription unaffected, and +// is returned for the log. AcceptRequest has closed the session on a malformed +// FILL_PARAMETERS (§10.2.15; see [message.Parameters.CheckScope]). func (h *sessionHandler) maybeServeFill( ctx context.Context, sub *registry.DownstreamSub, @@ -31,10 +31,7 @@ func (h *sessionHandler) maybeServeFill( requestID uint64, ps message.Parameters, ) error { - inner, requested, err := message.FillParametersFromParam(ps) - if err != nil { - return err - } + inner, requested, _ := message.FillParametersFromParam(ps) if !requested { return nil } @@ -62,10 +59,7 @@ func (h *sessionHandler) maybeServeFill( // §5.1.3: the fill range comes from the LOCATION_FILTER inside // FILL_PARAMETERS, falling back to the subscription's own filter, and to // the whole track when neither is present. - filter, err := message.LocationFilterFromParam(inner) - if err != nil { - return fail(err) - } + filter, _ := message.LocationFilterFromParam(inner) if filter == nil { filter = sub.GetFilter() } @@ -94,6 +88,7 @@ func (h *sessionHandler) maybeServeFill( // would (§5.1.4); the other types are inherited. rangeFilters := sub.GetRangeFilters() if slices.ContainsFunc(inner, func(p message.Parameter) bool { return message.IsRangeFilterParam(p.Type) }) { + var err error rangeFilters, err = rangeFilters.Update(inner) if err == nil && rangeFilters != nil { err = rangeFilters.Validate(h.sess.MaxFilterRanges()) diff --git a/pkg/relay/handler_namespace.go b/pkg/relay/handler_namespace.go index ee829f62..6dd4ad00 100644 --- a/pkg/relay/handler_namespace.go +++ b/pkg/relay/handler_namespace.go @@ -270,15 +270,8 @@ func (h *sessionHandler) handleSubscribeTracks( // subscribeTracksForwarding resolves a SUBSCRIBE_TRACKS's FORWARD (§10.2.18, // default true) and GROUP_ORDER (§10.2.8, 0 when omitted), which §10.20.1 -// copies onto forwarded PUBLISHes. An out-of-range value is a -// *paramProtocolViolation. -func subscribeTracksForwarding(ps message.Parameters) (forward bool, groupOrder byte, err error) { - if err := checkForwardParam(ps); err != nil { - return false, 0, err - } - if err := checkGroupOrderParam(ps); err != nil { - return false, 0, err - } +// copies onto forwarded PUBLISHes. +func subscribeTracksForwarding(ps message.Parameters) (forward bool, groupOrder byte) { forward = true if p, ok := ps.Find(message.ParamForward); ok { forward = p.Byte != 0 @@ -286,7 +279,7 @@ func subscribeTracksForwarding(ps message.Parameters) (forward bool, groupOrder if p, ok := ps.Find(message.ParamGroupOrder); ok { groupOrder = p.Byte } - return forward, groupOrder, nil + return forward, groupOrder } // serveNamespaceFollowups holds a namespace request stream open and answers @@ -449,15 +442,7 @@ func (h *sessionHandler) tracksUpdate( tokens = updTokens params, err := h.resolveTracksParams(merged) if err != nil { - if _, ok := errors.AsType[*paramProtocolViolation](err); ok { - _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) - return false - } - code := moqt.RequestMalformedTrack - if errors.Is(err, message.ErrInvalidFilter) { - code = moqt.RequestInvalidFilter - } - e.Finish(&message.RequestError{ErrorCode: code, ErrorReason: err.Error()}) + e.Finish(&message.RequestError{ErrorCode: moqt.RequestInvalidFilter, ErrorReason: err.Error()}) return endAfterFinish(ctx, e) } oldPrefix := *cur @@ -571,15 +556,11 @@ func mergeTracksUpdate(stored, upd message.Parameters) message.Parameters { return out } -// resolveTracksParams validates a SUBSCRIBE_TRACKS's parameters on a -// SUBSCRIBE's terms (§10.20.1; errors as for -// [sessionHandler.refuseSubscriptionParams]) and MAX_FILTER_RANGES (§5.1.4), -// and resolves what forwarding needs. +// resolveTracksParams validates a SUBSCRIBE_TRACKS's Range Filters on a +// SUBSCRIBE's terms (§10.20.1) and MAX_FILTER_RANGES (§5.1.4), and resolves +// what forwarding needs. An error is INVALID_FILTER. func (h *sessionHandler) resolveTracksParams(ps message.Parameters) (*registry.TracksParams, error) { - forward, groupOrder, err := subscribeTracksForwarding(ps) - if err != nil { - return nil, err - } + forward, groupOrder := subscribeTracksForwarding(ps) rangeFilters, err := message.RangeFiltersFromParams(ps) if err == nil && rangeFilters != nil { err = rangeFilters.Validate(h.sess.MaxFilterRanges()) diff --git a/pkg/relay/handler_publish.go b/pkg/relay/handler_publish.go index 34d8c57e..5ee9035f 100644 --- a/pkg/relay/handler_publish.go +++ b/pkg/relay/handler_publish.go @@ -39,12 +39,6 @@ func (h *sessionHandler) handlePublish(ctx context.Context, req *session.Request slog.String("name", string(msg.Name)), slog.Uint64("alias", msg.TrackAlias)) - // §10.2.18: an out-of-range FORWARD closes the session. - if err := checkForwardParam(msg.Parameters); err != nil { - _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) - return - } - // §2.5.1: refuse an unknown Mandatory Track Property. MALFORMED_TRACK for // unparseable Track Properties is this repo's choice; the draft is silent. if err := h.sess.CheckTrackProperties(msg.TrackProperties, "PUBLISH"); err != nil { diff --git a/pkg/relay/handler_subscribe.go b/pkg/relay/handler_subscribe.go index 30cbfd72..e25cb3d3 100644 --- a/pkg/relay/handler_subscribe.go +++ b/pkg/relay/handler_subscribe.go @@ -160,8 +160,8 @@ func (h *sessionHandler) handleSubscribe(ctx context.Context, req *session.Reque slog.String("name", string(msg.Name)), slog.Uint64("alias", alias)) - // §5.1.3: FILL_PARAMETERS asks for a fill fetch stream; a malformed one - // was already rejected by installSubscribeParams. + // §5.1.3: FILL_PARAMETERS asks for a fill fetch stream; AcceptRequest + // has closed the session on a malformed one. if err := h.maybeServeFill(ctx, sub, entry, fullName, msg.RequestID, msg.Parameters); err != nil { h.log.LogAttrs(ctx, slog.LevelDebug, "fill fetch stream not opened", slog.String("err", err.Error())) @@ -238,23 +238,12 @@ func (h *sessionHandler) handleSubscribeUpdate( ) { prevForward := sub.ForwardState() if err := installSubscribeParams(sub, upd.Parameters); err != nil { - if _, ok := errors.AsType[*paramProtocolViolation](err); ok { - // §10.2.8 / §10.2.18: session-level even in a REQUEST_UPDATE. - h.log.LogAttrs(ctx, slog.LevelDebug, "REQUEST_UPDATE parameter protocol violation", - slog.String("err", err.Error())) - _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) - return - } - h.log.LogAttrs(ctx, slog.LevelDebug, "REQUEST_UPDATE parameter parse failed", + h.log.LogAttrs(ctx, slog.LevelDebug, "REQUEST_UPDATE range filter rejected", slog.String("err", err.Error())) // §10.9.1: REQUEST_ERROR, then PUBLISH_DONE / UPDATE_FAILED. Writes go // through the sub's lock. - code := moqt.RequestMalformedTrack - if errors.Is(err, message.ErrInvalidFilter) { - code = moqt.RequestInvalidFilter - } _ = sub.WriteMessage(&message.RequestError{ - ErrorCode: code, + ErrorCode: moqt.RequestInvalidFilter, ErrorReason: err.Error(), }) sub.TerminateWithPublishDone(moqt.PublishDoneUpdateFailed, err.Error()) @@ -576,22 +565,14 @@ func resolveGroupOrder(sub *registry.DownstreamSub, entry *registry.TrackEntry) // installSubscribeParams records the subscription parameters present in ps // (§10.2) on sub, leaving absent ones unchanged. The Largest snapshot is the -// caller's (see [registry.TrackRegistry.AddDownstreamSnapshotLargest]). +// caller's (see [registry.TrackRegistry.AddDownstreamSnapshotLargest]). The +// session has already closed on a value the draft makes session-fatal (see +// [message.Parameters.CheckScope]); an error here is a Range Filter's, which +// is INVALID_FILTER (§5.1.4). func installSubscribeParams(sub *registry.DownstreamSub, ps message.Parameters) error { - filter, err := message.LocationFilterFromParam(ps) - if err != nil { - return fmt.Errorf("location filter: %w", err) - } - if filter != nil { - if err := filter.Validate(); err != nil { - return err - } + if filter, _ := message.LocationFilterFromParam(ps); filter != nil { sub.SetFilter(filter) } - - if err := checkForwardParam(ps); err != nil { - return err - } if p, ok := ps.Find(message.ParamForward); ok { sub.SetForwardState(int(p.Byte)) } @@ -599,9 +580,6 @@ func installSubscribeParams(sub *registry.DownstreamSub, ps message.Parameters) if p, ok := ps.Find(message.ParamSubscriberPriority); ok { sub.SetPriority(p.Byte) } - if err := checkGroupOrderParam(ps); err != nil { - return err - } if p, ok := ps.Find(message.ParamGroupOrder); ok { sub.SetGroupOrder(p.Byte) } @@ -634,61 +612,18 @@ func installSubscribeParams(sub *registry.DownstreamSub, ps message.Parameters) } // refuseSubscriptionParams answers a SUBSCRIBE or SUBSCRIBE_TRACKS whose -// subscription parameters [installSubscribeParams] rejected. +// Range Filters [installSubscribeParams] rejected: malformed or over the limit, +// INVALID_FILTER (§5.1.4, §10.6). func (h *sessionHandler) refuseSubscriptionParams(ctx context.Context, req *session.Request, err error) { - if _, ok := errors.AsType[*paramProtocolViolation](err); ok { - // §10.2.8 / §10.2.18: an out-of-range GROUP_ORDER/FORWARD is a - // session-level PROTOCOL_VIOLATION. - h.log.LogAttrs(ctx, slog.LevelDebug, "subscription parameter protocol violation", - slog.String("err", err.Error())) - _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) - return - } - // §5.1.4 / §10.6: a malformed or over-limit Range Filter is INVALID_FILTER. - if errors.Is(err, message.ErrInvalidFilter) { - h.log.LogAttrs(ctx, slog.LevelDebug, "subscription range filter rejected", - slog.String("err", err.Error())) - _ = req.RejectError(moqt.RequestInvalidFilter, err.Error()) - return - } - // Deviation: §5.1.2 makes a malformed LOCATION_FILTER a session-level - // PROTOCOL_VIOLATION; the relay scopes it to the request. - h.log.LogAttrs(ctx, slog.LevelDebug, "subscription parameter parse failed", + h.log.LogAttrs(ctx, slog.LevelDebug, "subscription range filter rejected", slog.String("err", err.Error())) - _ = req.RejectError(moqt.RequestMalformedTrack, err.Error()) + _ = req.RejectError(moqt.RequestInvalidFilter, err.Error()) } // errPeerGoingAway reports a request the relay did not send because the peer // sent GOAWAY (§10.4; see [peerSentGoaway]). var errPeerGoingAway = errors.New("relay: peer sent GOAWAY; no new requests to it (§10.4)") -// paramProtocolViolation marks a parameter value that closes the session with -// PROTOCOL_VIOLATION: an out-of-range GROUP_ORDER (§10.2.8) or FORWARD -// (§10.2.18). -type paramProtocolViolation struct{ reason string } - -func (e *paramProtocolViolation) Error() string { return e.reason } - -// checkForwardParam enforces the §10.2.18 FORWARD range (0 or 1). -func checkForwardParam(ps message.Parameters) error { - if p, ok := ps.Find(message.ParamForward); ok && p.Byte > 1 { - return ¶mProtocolViolation{fmt.Sprintf("invalid FORWARD value 0x%X (§10.2.18)", p.Byte)} - } - return nil -} - -// checkGroupOrderParam enforces the §10.2.8 GROUP_ORDER range (0x1 or 0x2). -func checkGroupOrderParam(ps message.Parameters) error { - if p, ok := ps.Find(message.ParamGroupOrder); ok { - switch message.GroupOrder(p.Byte) { - case message.GroupOrderAscending, message.GroupOrderDescending: - default: - return ¶mProtocolViolation{fmt.Sprintf("invalid GROUP_ORDER value 0x%X (§10.2.8)", p.Byte)} - } - } - return nil -} - // includeProperties reports whether INCLUDE_PROPERTIES (§10.2.21) asks for // Track Properties: yes unless it is 0. func includeProperties(ps message.Parameters) bool { diff --git a/pkg/relay/helpers_test.go b/pkg/relay/helpers_test.go index c54fe6fc..0e6f6e97 100644 --- a/pkg/relay/helpers_test.go +++ b/pkg/relay/helpers_test.go @@ -32,6 +32,14 @@ func trackProp(typ, v uint64) []wire.KVPair { return []wire.KVPair{{Type: typ, IntVal: v}} } +// badRangeFilter is a PRIORITY_FILTER ranging past 255, which the relay +// refuses per request with INVALID_FILTER (§10.2.12). +func badRangeFilter() message.Parameter { + return message.RangeFilterParam(&message.RangeFilter{ + Type: message.ParamPriorityFilter, Ranges: []message.Range{{Start: 0, End: 300}}, + }) +} + // dynamicGroupsProperties is Track Properties carrying DYNAMIC_GROUPS = value. func dynamicGroupsProperties(value uint64) []byte { return message.AppendTrackProperties(trackProp(message.PropertyDynamicGroups, value)) diff --git a/pkg/relay/param_values_test.go b/pkg/relay/param_values_test.go new file mode 100644 index 00000000..82104640 --- /dev/null +++ b/pkg/relay/param_values_test.go @@ -0,0 +1,78 @@ +package relay_test + +import ( + "math" + "testing" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// TestRelay_SessionFatalParamValuesClose: a request carrying a value the draft +// makes session-fatal closes the relay's session, on every path the relay +// serves: GROUP_ORDER outside {1, 2} on PUBLISH and FETCH and inside +// FILL_PARAMETERS (§10.2.8), and a LOCATION_FILTER whose end Group overflows +// (§5.1.2, which the relay used to answer per request), on an opener and in a +// REQUEST_UPDATE. +func TestRelay_SessionFatalParamValuesClose(t *testing.T) { + t.Parallel() + badOrder := message.ByteParam(message.ParamGroupOrder, 5) + overflow := message.AbsoluteRangeFilter(message.Location{Group: math.MaxUint64}, 1) + for _, tc := range []struct { + name string + // send sends the violation from another goroutine; prepare, if set, + // runs first on the test goroutine and returns what send needs. + prepare func(t *testing.T, sess *session.Session) *session.Subscription + send func(t *testing.T, sess *session.Session, sub *session.Subscription) + }{ + {name: "GROUP_ORDER on PUBLISH", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Publish(t.Context(), &message.Publish{ + Namespace: ns("video"), Name: []byte("cam1"), TrackAlias: 1, Parameters: message.Parameters{badOrder}, + }) + }}, + {name: "GROUP_ORDER on FETCH", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Fetch(t.Context(), &message.Fetch{ + Namespace: ns("video"), Name: []byte("cam1"), Parameters: message.Parameters{badOrder}, + }) + }}, + {name: "GROUP_ORDER inside FILL_PARAMETERS", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Subscribe(t.Context(), &message.Subscribe{ + Namespace: ns("video"), Name: []byte("cam1"), + Parameters: message.Parameters{message.FillParametersParam(message.Parameters{badOrder})}, + }) + }}, + {name: "LOCATION_FILTER overflow on SUBSCRIBE", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Subscribe(t.Context(), &message.Subscribe{ + Namespace: ns("video"), Name: []byte("cam1"), Parameters: message.Parameters{overflow}, + }) + }}, + {name: "LOCATION_FILTER overflow on FETCH", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Fetch(t.Context(), &message.Fetch{ + Namespace: ns("video"), Name: []byte("cam1"), Parameters: message.Parameters{overflow}, + }) + }}, + { + name: "LOCATION_FILTER overflow in a REQUEST_UPDATE", + prepare: func(t *testing.T, sess *session.Session) *session.Subscription { + publishVideoTrack(t, dialAnotherClient(t, sess), "cam1", 1) + return subscribeCam1(t, sess) + }, + send: func(t *testing.T, sess *session.Session, sub *session.Subscription) { + _, _ = sess.UpdateRequest(t.Context(), sub, message.Parameters{overflow}) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + sess, teardown := connectRelay(t, relay.Config{}) + defer teardown() + var sub *session.Subscription + if tc.prepare != nil { + sub = tc.prepare(t, sess) + } + go tc.send(t, sess, sub) + requireSessionClosed(t, sess, tc.name) + }) + } +} diff --git a/pkg/relay/publish_done_test.go b/pkg/relay/publish_done_test.go index 257404e9..5ac42f5c 100644 --- a/pkg/relay/publish_done_test.go +++ b/pkg/relay/publish_done_test.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "io" - "math" "testing" "time" @@ -241,9 +240,8 @@ func TestPublishDone_EndedSubscriptionStopsAtNextObject(t *testing.T) { }() // A request-scoped malformed update: REQUEST_ERROR, then UPDATE_FAILED. - _, err = subSess.UpdateRequest(t.Context(), subReq, - message.Parameters{message.AbsoluteRangeFilter(message.Location{Group: math.MaxUint64}, 1)}) - requireRejectedWithCode(t, err, moqt.RequestMalformedTrack) + _, err = subSess.UpdateRequest(t.Context(), subReq, message.Parameters{badRangeFilter()}) + requireRejectedWithCode(t, err, moqt.RequestInvalidFilter) close(rejected) // the upstream subgroup goes on, and its stream stays open if pd := awaitPublishDone(t, subReq); pd.StatusCode != moqt.PublishDoneUpdateFailed { @@ -393,12 +391,10 @@ func TestPublishDone_AfterFailedFill(t *testing.T) { t.Fatal("the object never reached the relay") } - // A five-field LOCATION_FILTER in FILL_PARAMETERS does not parse, which - // fails the fill after SUBSCRIBE_OK. + // An invalid Range Filter in FILL_PARAMETERS fails the fill after + // SUBSCRIBE_OK. subSess := dialAnotherClient(t, pubSess) - subReq := subscribeCam1(t, subSess, message.FillParametersParam(message.Parameters{ - message.BytesParam(message.ParamLocationFilter, []byte{0, 0, 0, 0, 0}), - })) + subReq := subscribeCam1(t, subSess, message.FillParametersParam(message.Parameters{badRangeFilter()})) ds, err := subSess.AcceptDataStream(t.Context()) if err != nil { t.Fatalf("AcceptDataStream (fill): %v", err) diff --git a/pkg/relay/session_update_test.go b/pkg/relay/session_update_test.go index 1714d32b..26276007 100644 --- a/pkg/relay/session_update_test.go +++ b/pkg/relay/session_update_test.go @@ -2,7 +2,6 @@ package relay_test import ( "context" - "math" "testing" "time" @@ -84,13 +83,9 @@ func TestRequestUpdate_MalformedRejectedWithUpdateFailed(t *testing.T) { } defer subStream.Close() - // An AbsoluteRange filter whose end-group delta overflows the start group - // (§5.1.2) fails installSubscribeParams' filter validation. Unlike an - // out-of-range GROUP_ORDER/FORWARD, a bad filter stays request-scoped, so - // the relay answers REQUEST_ERROR rather than closing the session. - _, err = subSess.UpdateRequest(t.Context(), subStream, - message.Parameters{message.AbsoluteRangeFilter(message.Location{Group: math.MaxUint64}, 1)}) - requireRejectedWithCode(t, err, moqt.RequestMalformedTrack) + // An invalid Range Filter is refused per request (§5.1.4). + _, err = subSess.UpdateRequest(t.Context(), subStream, message.Parameters{badRangeFilter()}) + requireRejectedWithCode(t, err, moqt.RequestInvalidFilter) // §10.9: the failed update is followed by a PUBLISH_DONE with // UPDATE_FAILED on the same stream. diff --git a/pkg/relay/subscribe_tracks_test.go b/pkg/relay/subscribe_tracks_test.go index fba88389..76abfcfa 100644 --- a/pkg/relay/subscribe_tracks_test.go +++ b/pkg/relay/subscribe_tracks_test.go @@ -253,17 +253,19 @@ func TestForwardedPublish_RepublishedTrackForwardedAgain(t *testing.T) { } // TestForwardedPublish_SubscribeTracksParametersValidated: SUBSCRIBE_TRACKS -// parameters are validated as on SUBSCRIBE (§10.20.1): a malformed -// LOCATION_FILTER is MALFORMED_TRACK. +// parameters are validated as on SUBSCRIBE (§10.20.1): a LOCATION_FILTER that +// does not parse closes the session (§1.4.3). func TestForwardedPublish_SubscribeTracksParametersValidated(t *testing.T) { t.Parallel() subSess, teardown := connectRelay(t, relay.Config{}) defer teardown() - _, err := subSess.SubscribeTracks(t.Context(), &message.SubscribeTracks{ - TrackNamespacePrefix: ns("video"), - Parameters: message.Parameters{message.BytesParam(message.ParamLocationFilter, []byte{0xFF})}, - }) - requireRejectedWithCode(t, err, moqt.RequestMalformedTrack) + go func() { + _, _ = subSess.SubscribeTracks(t.Context(), &message.SubscribeTracks{ + TrackNamespacePrefix: ns("video"), + Parameters: message.Parameters{message.BytesParam(message.ParamLocationFilter, []byte{0xFF})}, + }) + }() + requireSessionClosed(t, subSess, "a SUBSCRIBE_TRACKS with a LOCATION_FILTER that does not parse") } // A REQUEST_UPDATE on SUBSCRIBE_TRACKS applies to the PUBLISHes sent from then From 94f0c136d4dc9c5c03fbb871e7580480f8c0b877 Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 20:13:10 +0500 Subject: [PATCH 2/9] =?UTF-8?q?fix(session,relay):=20close=20the=20session?= =?UTF-8?q?=20on=20a=20session-fatal=20Track=20Property=20value=20(=C2=A71?= =?UTF-8?q?2.5,=20=C2=A712.6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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) --- STATUS.md | 14 +-- pkg/moqt/message/properties.go | 23 ++++ pkg/moqt/session/namespace.go | 4 + pkg/moqt/session/request.go | 17 ++- pkg/moqt/session/track_properties.go | 41 ++++-- pkg/moqt/session/track_properties_test.go | 119 ++++++++++++++++++ pkg/relay/handler_publish.go | 2 + pkg/relay/handler_subscribe.go | 4 +- .../internal/registry/dynamic_groups_test.go | 14 +-- pkg/relay/internal/registry/properties.go | 51 +++----- pkg/relay/param_values_test.go | 15 ++- 11 files changed, 236 insertions(+), 68 deletions(-) diff --git a/STATUS.md b/STATUS.md index 87fa8048..9aa3343c 100644 --- a/STATUS.md +++ b/STATUS.md @@ -233,8 +233,8 @@ By package, bottom-up along the dependency stack: | 12.2 | OBJECT_DELIVERY_TIMEOUT | 0x02 | DONE | Track + Object Property; first-object override, as §12.1. | | 12.3 | MAX_CACHE_DURATION | 0x04 | DONE | Per Object: each carries the value of the upstream it arrived through (captured with the Track Alias in `session.InboundTrack`), and is not forwarded live or served from the cache past it; a present 0 is never served from the cache. In FETCH and fill an expired Object is an End of Unknown Range, whether it expired before the snapshot or while the stream was written. | | 12.4 | DEFAULT_PUBLISHER_PRIORITY | 0x0E | DONE | | -| 12.5 | DEFAULT_PUBLISHER_GROUP_ORDER | 0x22 | DONE | Validated. | -| 12.6 | DYNAMIC_GROUPS | 0x30 | DONE | Property defined & scope-validated (flow: see §5.1.6.1). | +| 12.5 | DEFAULT_PUBLISHER_GROUP_ORDER | 0x22 | DONE | A value outside {1, 2} closes the session, also inside Immutable Properties; an omitted one is Ascending. | +| 12.6 | DYNAMIC_GROUPS | 0x30 | DONE | A value above 1 closes the session, also inside Immutable Properties. | | 12.7 | Immutable properties | 0x0B | DONE | Relays cache & forward verbatim, never add. Property lookups search its contents too (`message.ExpandImmutable`), the mutable value winning: delivery timeouts, MAX_CACHE_DURATION, DYNAMIC_GROUPS, Mandatory Track Property screening, and property Range Filters. | | 12.8 | Prior group ID gap | 0x3C | PARTIAL| Object-scope; encoder in `msf/groupid.go`. More than one, or one past the Group ID, makes the track malformed (`message.CheckObjectProperties`), and the relay also ends the track for two values in one Group. Against the last 32 Groups of any upstream (`registry.TrackEntry.ClaimDelivered`), the relay neither forwards nor caches an Object in a Group announced absent; a gap covering a received Group is accepted (§2.1, §9.1, see Limitations). Not in upstream FETCH responses, nor in a session that is not a relay's. | | 12.9 | Prior object ID gap | 0x3E | PARTIAL| Object-scope. More than one, or one past the Object ID, makes the track malformed. The relay neither forwards nor caches an Object announced absent, and accepts a gap covering a received Object, as for §12.8. | @@ -547,11 +547,6 @@ Session layer: - SETUP options are sorted unstably, so with more than 12 the Token order on the wire can differ from the order `heldSetupAliases` replays (§10.3.1.4). -Validation (values that MUST close the session): - -- DEFAULT_PUBLISHER_GROUP_ORDER outside {1, 2} and DYNAMIC_GROUPS above 1 in - Track Properties (§12.5, §12.6). - Relay: - Any REQUEST_UPDATE turns INCLUDE_PROPERTIES=0 back off, so the subscriber @@ -591,9 +586,8 @@ Documentation: - Limitations: "Duplicate Objects … are not compared" is stale; the LOC entry names `PropAudioLevel = 0x0A` (it is 0x0C); "Handles the application reads itself" says `CheckPeerParams` checks roles; "Inbound GOAWAY" omits request streams. -- Table rows 10.2.6, 10.2.15, 10.2.21, 12.3, - 12.5 and 12.6 overstate what is done (see the items above), and the package - summary still lists joining FETCH. +- Table rows 10.2.6, 10.2.15, 10.2.21 and 12.3 overstate what is done (see + the items above), and the package summary still lists joining FETCH. - `session/namespace.go` says NAMESPACE / NAMESPACE_DONE go on a PUBLISH_NAMESPACE stream (§10.17, §10.18). - About a dozen stale `§` citations (padding, grease, fetch ordering, caching). diff --git a/pkg/moqt/message/properties.go b/pkg/moqt/message/properties.go index 4906376e..875b080d 100644 --- a/pkg/moqt/message/properties.go +++ b/pkg/moqt/message/properties.go @@ -195,6 +195,29 @@ func AppendTrackProperties(pairs []wire.KVPair) []byte { return w.Bytes() } +// CheckTrackPropertyValues reports a Track Property in pairs (Immutable +// Properties expanded, see [ExpandImmutable]) whose value the draft makes +// session-fatal: DEFAULT_PUBLISHER_GROUP_ORDER outside {1, 2} (§12.5) or +// DYNAMIC_GROUPS above 1 (§12.6). The caller MUST close the session with +// PROTOCOL_VIOLATION. +func CheckTrackPropertyValues(pairs []wire.KVPair) error { + for _, kv := range pairs { + switch kv.Type { + case PropertyDefaultPublisherGroupOrder: + if kv.IntVal != uint64(GroupOrderAscending) && kv.IntVal != uint64(GroupOrderDescending) { + return fmt.Errorf( + "moqt/message: DEFAULT_PUBLISHER_GROUP_ORDER value %d outside {1,2} (PROTOCOL_VIOLATION §12.5)", + kv.IntVal) + } + case PropertyDynamicGroups: + if kv.IntVal > 1 { + return fmt.Errorf("moqt/message: DYNAMIC_GROUPS value %d above 1 (PROTOCOL_VIOLATION §12.6)", kv.IntVal) + } + } + } + return nil +} + // FirstUnknownMandatoryTrackProperty returns the first Mandatory Track // Property (range 0x4000–0x7FFF) in pairs whose type is not in knownTypes, // and whether one was found — the offending type is what callers need to diff --git a/pkg/moqt/session/namespace.go b/pkg/moqt/session/namespace.go index 68b37dbe..710e4e6c 100644 --- a/pkg/moqt/session/namespace.go +++ b/pkg/moqt/session/namespace.go @@ -115,6 +115,10 @@ func (s *Session) SubscribeNamespace( func (s *Session) SubscribeTracks(ctx context.Context, m *message.SubscribeTracks) (*TrackSubscription, error) { return awaitRequestResponse(ctx, s, m, func(stream Stream, ok *message.RequestOK) (*TrackSubscription, error) { + if err := s.checkTrackPropertyValues(ok.TrackProperties, "SUBSCRIBE_TRACKS_OK"); err != nil { + cancelRequest(stream) + return nil, err + } return &TrackSubscription{Stream: stream, OK: ok, s: s}, nil }) } diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index 159dee1e..87b41692 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -237,9 +237,10 @@ type Request struct { // requests for session-level tracks and namespaces". AcceptRequest loops until // it has an application-visible request to return. // -// A stream not opened by a request message (§3.3), or whose first message is -// malformed (§10, wrapping [message.ErrMalformedMessage]), closes the session -// with PROTOCOL_VIOLATION; the error is *ErrUnexpectedRequestOpener, +// A stream not opened by a request message (§3.3), whose first message is +// malformed (§10, wrapping [message.ErrMalformedMessage]), or a PUBLISH with a +// session-fatal Track Property value (§12.5, §12.6), closes the session with +// PROTOCOL_VIOLATION; the error is *ErrUnexpectedRequestOpener, // *ErrUnexpectedRequestUpdate, ErrUnexpectedPublishStateNotify or the parse // error. A Request ID violation (§10.1) closes it with INVALID_REQUEST_ID and // returns *ErrRequestIDParityViolation or *ErrDuplicateRequestID, and a token @@ -303,6 +304,13 @@ func (s *Session) AcceptRequest(ctx context.Context) (*Request, error) { resetStream(stream) return nil, err } + // §12.5, §12.6: on receipt, whatever the application answers. + if pub, ok := msg.(*message.Publish); ok { + if err := s.checkTrackPropertyValues(pub.TrackProperties, "PUBLISH"); err != nil { + resetStream(stream) + return nil, err + } + } // §10.1 parity and duplicate check. if m, ok := msg.(message.WithRequestID); ok { @@ -815,7 +823,8 @@ func (r *Request) AcceptSubscribe(ok *message.SubscribeOK) (*Publication, error) // Track Properties that fail validation (see // [WithKnownMandatoryTrackProperties]) are rejected with REQUEST_ERROR — // UNSUPPORTED_EXTENSION for an unknown Mandatory Track Property (§2.5.1), -// MALFORMED_TRACK for ones that do not parse — and the error returned. On an +// MALFORMED_TRACK for ones that do not parse — and the error returned. A +// session-fatal value (§12.5, §12.6) closed the session in AcceptRequest. On an // alias collision *ErrDuplicateTrackAlias is returned without replying; the // caller MUST close the session with [moqt.SessionDuplicateTrackAlias] // (§11.1). diff --git a/pkg/moqt/session/track_properties.go b/pkg/moqt/session/track_properties.go index 0ba9eab9..4fdea66e 100644 --- a/pkg/moqt/session/track_properties.go +++ b/pkg/moqt/session/track_properties.go @@ -77,22 +77,28 @@ func ValidateTrackProperties( return pairs, nil } -// CheckTrackProperties validates raw Track Properties against the types -// configured with [WithKnownMandatoryTrackProperties] (§2.5.1), returning +// CheckTrackProperties validates raw Track Properties: a value the draft makes +// session-fatal closes the session (§12.5, §12.6), and against the types +// configured with [WithKnownMandatoryTrackProperties] (§2.5.1) it returns // *ErrUnsupportedMandatoryTrackProperty or an error wrapping // [ErrMalformedTrackProperties]; see [TrackPropertiesRejectCode]. It is for // callers that bypass [Request.AcceptPublish] and the outbound openers, which -// already check. Without that option it returns nil. +// already check. Without that option only the values are checked. func (s *Session) CheckTrackProperties(raw []byte, context string) error { return s.validateTrackProperties(raw, context) } -// validateTrackProperties is a session-level convenience that uses the -// session's configured set of known mandatory track property types. -// -// If WithKnownMandatoryTrackProperties was never called (the map is nil), -// the check is skipped, for endpoints that pass Track Properties through. +// validateTrackProperties checks Track Properties received in context. A value +// the draft makes session-fatal (see [message.CheckTrackPropertyValues]) +// closes the session with PROTOCOL_VIOLATION. Then, against the session's +// configured set of known mandatory track property types, an unknown one is +// refused (§2.5.1); if WithKnownMandatoryTrackProperties was never called (the +// map is nil), that check is skipped, for endpoints that pass Track +// Properties through. func (s *Session) validateTrackProperties(raw []byte, context string) error { + if err := s.checkTrackPropertyValues(raw, context); err != nil { + return err + } if s.knownMandatoryTrackProperties == nil { return nil // not configured — skip enforcement } @@ -100,6 +106,25 @@ func (s *Session) validateTrackProperties(raw []byte, context string) error { return err } +// checkTrackPropertyValues closes the session with PROTOCOL_VIOLATION when +// Track Properties received in context hold a value the draft makes +// session-fatal (see [message.CheckTrackPropertyValues]): "If an endpoint +// receives a value outside this range, it MUST close the session" (§12.5, +// §12.6). Immutable Properties are searched when they parse (§12.7); Track +// Properties that do not parse are left to the caller. +func (s *Session) checkTrackPropertyValues(raw []byte, context string) error { + pairs, parseErr := message.ParseTrackProperties(raw) + if parseErr == nil { + if all, err := message.ExpandImmutable(pairs); err == nil { + pairs = all + } + } + if err := message.CheckTrackPropertyValues(pairs); err != nil { + return s.closeProtocolViolation(fmt.Errorf("%s: %w", context, err)) + } + return nil +} + // TrackPropertiesRejectCode is the REQUEST_ERROR code for a Track Properties // validation error: UNSUPPORTED_EXTENSION (§2.5.1) or MALFORMED_TRACK. func TrackPropertiesRejectCode(err error) moqt.RequestErrorCode { diff --git a/pkg/moqt/session/track_properties_test.go b/pkg/moqt/session/track_properties_test.go index 4c58f702..6321e724 100644 --- a/pkg/moqt/session/track_properties_test.go +++ b/pkg/moqt/session/track_properties_test.go @@ -778,3 +778,122 @@ func TestMandatoryTrackPropertyBoundaries(t *testing.T) { }) } } + +// TestTrackPropertyValueOutOfRangeCloses: a DEFAULT_PUBLISHER_GROUP_ORDER +// outside {1, 2} (§12.5) or a DYNAMIC_GROUPS above 1 (§12.6), also inside +// Immutable Properties (§12.7), closes the receiver with PROTOCOL_VIOLATION on +// each path Track Properties arrive by, whether or not it enforces Mandatory +// Track Properties. +func TestTrackPropertyValueOutOfRangeCloses(t *testing.T) { + t.Parallel() + prop := func(typ message.PropertyType, v uint64) []wire.KVPair { + return []wire.KVPair{{Type: typ, IntVal: v}} + } + values := []struct { + name string + props []byte + }{ + { + "DEFAULT_PUBLISHER_GROUP_ORDER 3", + message.AppendTrackProperties(prop(message.PropertyDefaultPublisherGroupOrder, 3)), + }, + { + "DEFAULT_PUBLISHER_GROUP_ORDER 0", + message.AppendTrackProperties(prop(message.PropertyDefaultPublisherGroupOrder, 0)), + }, + {"DYNAMIC_GROUPS 2", message.AppendTrackProperties(prop(message.PropertyDynamicGroups, 2))}, + {"inside Immutable Properties", message.AppendTrackProperties([]wire.KVPair{ + { + Type: message.PropertyImmutableProperties, + ByteVal: message.AppendTrackProperties(prop(message.PropertyDynamicGroups, 7)), + }, + })}, + {"next to Immutable Properties that do not parse", message.AppendTrackProperties([]wire.KVPair{ + {Type: message.PropertyDynamicGroups, IntVal: 2}, + {Type: message.PropertyImmutableProperties, ByteVal: []byte{0x40}}, + })}, + } + ns := wire.TrackNamespace{[]byte("ns")} + paths := []struct { + name string + // run makes the receiver get props, returning it. + run func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session + }{ + {"SUBSCRIBE_OK", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.SubscribeOK{TrackAlias: 7, TrackProperties: props}) + } + }() + go func() { _, _ = cli.Subscribe(t.Context(), &message.Subscribe{Namespace: ns, Name: []byte("t")}) }() + return cli + }}, + {"FETCH_OK", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.FetchOK{EndLocation: message.Location{Group: 1}, TrackProperties: props}) + } + }() + go func() { _, _ = cli.Fetch(t.Context(), &message.Fetch{Namespace: ns, Name: []byte("t")}) }() + return cli + }}, + {"TRACK_STATUS_OK", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.RequestOK{TrackProperties: props}) + } + }() + go func() { _, _ = cli.TrackStatus(t.Context(), &message.TrackStatus{Namespace: ns, Name: []byte("t")}) }() + return cli + }}, + {"SUBSCRIBE_TRACKS_OK", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.RequestOK{TrackProperties: props}) + } + }() + go func() { + _, _ = cli.SubscribeTracks(t.Context(), &message.SubscribeTracks{TrackNamespacePrefix: ns}) + }() + return cli + }}, + { + "PUBLISH, rejected by the application", + func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + _, _ = cli.Publish(t.Context(), &message.Publish{ + Namespace: ns, Name: []byte("t"), TrackAlias: 1, TrackProperties: props, + }) + }() + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.RejectError(moqt.RequestUnauthorized, "no") + } + }() + return srv + }, + }, + {"PUBLISH", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + _, _ = cli.Publish(t.Context(), &message.Publish{ + Namespace: ns, Name: []byte("t"), TrackAlias: 1, TrackProperties: props, + }) + }() + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _, _ = r.AcceptPublish() + } + }() + return srv + }}, + } + for _, p := range paths { + for _, v := range values { + t.Run(p.name+"/"+v.name, func(t *testing.T) { + t.Parallel() + cli, srv := openPair(t) + requireClosedProtocolViolation(t, p.run(t, cli, srv, v.props)) + }) + } + } +} diff --git a/pkg/relay/handler_publish.go b/pkg/relay/handler_publish.go index 5ee9035f..13b9f805 100644 --- a/pkg/relay/handler_publish.go +++ b/pkg/relay/handler_publish.go @@ -41,6 +41,8 @@ func (h *sessionHandler) handlePublish(ctx context.Context, req *session.Request // §2.5.1: refuse an unknown Mandatory Track Property. MALFORMED_TRACK for // unparseable Track Properties is this repo's choice; the draft is silent. + // AcceptRequest has closed the session on a session-fatal value (§12.5, + // §12.6). if err := h.sess.CheckTrackProperties(msg.TrackProperties, "PUBLISH"); err != nil { _ = req.RejectError(session.TrackPropertiesRejectCode(err), err.Error()) return diff --git a/pkg/relay/handler_subscribe.go b/pkg/relay/handler_subscribe.go index e25cb3d3..115ef202 100644 --- a/pkg/relay/handler_subscribe.go +++ b/pkg/relay/handler_subscribe.go @@ -298,8 +298,8 @@ func (h *sessionHandler) propagateNewGroupUpstream( dynamic, err := entry.DynamicGroups() if err != nil { - // §12.6: a bad DYNAMIC_GROUPS only declines the request here. - h.log.LogAttrs(ctx, slog.LevelDebug, "NEW_GROUP_REQUEST: bad DYNAMIC_GROUPS property", + // Unparseable Track Properties only decline the request here. + h.log.LogAttrs(ctx, slog.LevelDebug, "NEW_GROUP_REQUEST: unparseable Track Properties", slog.String("err", err.Error())) return } diff --git a/pkg/relay/internal/registry/dynamic_groups_test.go b/pkg/relay/internal/registry/dynamic_groups_test.go index eea72b96..a05de61a 100644 --- a/pkg/relay/internal/registry/dynamic_groups_test.go +++ b/pkg/relay/internal/registry/dynamic_groups_test.go @@ -19,8 +19,8 @@ func dynamicGroupsProps(t *testing.T, value uint64) []byte { // TestTrackEntry_DynamicGroups pins the §12.6 DYNAMIC_GROUPS decode that // SetProperties performs once and caches: an absent property and value 0 are -// false, value 1 is true, and value > 1 is a PROTOCOL_VIOLATION surfaced as an -// error. +// false, and value 1 is true. The session closes on a value above 1 (§12.6) +// before Properties reach the registry. func TestTrackEntry_DynamicGroups(t *testing.T) { t.Parallel() @@ -51,12 +51,6 @@ func TestTrackEntry_DynamicGroups(t *testing.T) { t.Fatalf("got (%v, %v), want (true, nil)", got, err) } }) - t.Run("value > 1 is an error (§12.6)", func(t *testing.T) { - t.Parallel() - if _, err := setProps(dynamicGroupsProps(t, 2)).DynamicGroups(); err == nil { - t.Fatal("got nil error, want §12.6 protocol-violation error") - } - }) t.Run("malformed block is an error", func(t *testing.T) { t.Parallel() // 0x40 is the first byte of a 2-byte varint with no second byte, so @@ -92,6 +86,10 @@ func TestTrackEntry_DefaultGroupOrder(t *testing.T) { {"inside Immutable Properties", message.AppendTrackProperties([]wire.KVPair{ {Type: message.PropertyImmutableProperties, ByteVal: order(2)}, }), message.GroupOrderDescending}, + {"mutable Ascending over immutable Descending", message.AppendTrackProperties([]wire.KVPair{ + {Type: message.PropertyDefaultPublisherGroupOrder, IntVal: 1}, + {Type: message.PropertyImmutableProperties, ByteVal: order(2)}, + }), message.GroupOrderAscending}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() diff --git a/pkg/relay/internal/registry/properties.go b/pkg/relay/internal/registry/properties.go index 48b61c89..a61a29c6 100644 --- a/pkg/relay/internal/registry/properties.go +++ b/pkg/relay/internal/registry/properties.go @@ -1,7 +1,6 @@ package registry import ( - "fmt" "slices" "github.com/floatdrop/moq-go/pkg/moqt/message" @@ -23,11 +22,9 @@ type decodedProperties struct { // set, no field below is meaningful, so every accessor reports it. parseErr error - // dynamicGroups is DYNAMIC_GROUPS=1 (§12.6). dynamicGroupsErr is a §12.6 - // PROTOCOL_VIOLATION (a DYNAMIC_GROUPS value > 1) by the upstream - // publisher. - dynamicGroups bool - dynamicGroupsErr error + // dynamicGroups is DYNAMIC_GROUPS=1 (§12.6). The session closed on a + // value above 1. + dynamicGroups bool // deliveryTimeouts is the publisher's Track-level OBJECT_DELIVERY_TIMEOUT // (§12.2) and SUBGROUP_DELIVERY_TIMEOUT (§12.1) pair. Per §8 a zero value @@ -36,15 +33,15 @@ type decodedProperties struct { // reading of a track that declares neither. deliveryTimeouts message.DeliveryTimeouts - // groupOrder is DEFAULT_PUBLISHER_GROUP_ORDER (§12.5) when it holds an - // allowed value, else zero. + // groupOrder is DEFAULT_PUBLISHER_GROUP_ORDER (§12.5), zero when omitted. + // The session closed on a value outside {1, 2}. groupOrder message.GroupOrder } // decodeTrackProperties parses the raw Track Properties block once and pulls // out the fields the relay acts on. A structural parse failure short-circuits -// to a parseErr that every accessor surfaces; per-property value violations -// (e.g. §12.6) are recorded on the matching field's error. +// to a parseErr that every accessor surfaces. The session has closed on a +// session-fatal value (§12.5, §12.6) before Properties reach here. func decodeTrackProperties(raw []byte) decodedProperties { pairs, err := message.ParseTrackProperties(raw) if err == nil { @@ -60,48 +57,32 @@ func decodeTrackProperties(raw []byte) decodedProperties { // branch here for each new property. switch kv.Type { case message.PropertyDynamicGroups: - d.dynamicGroups, d.dynamicGroupsErr = decodeDynamicGroups(kv.IntVal) + d.dynamicGroups = kv.IntVal == 1 case message.PropertyObjectDeliveryTimeout: d.deliveryTimeouts.Object = message.MillisecondTimeout(kv.IntVal) case message.PropertySubgroupDeliveryTimeout: d.deliveryTimeouts.Subgroup = message.MillisecondTimeout(kv.IntVal) case message.PropertyDefaultPublisherGroupOrder: - d.groupOrder = 0 - if kv.IntVal == uint64(message.GroupOrderAscending) || kv.IntVal == uint64(message.GroupOrderDescending) { - d.groupOrder = message.GroupOrder(kv.IntVal) + d.groupOrder = message.GroupOrderAscending + if kv.IntVal == uint64(message.GroupOrderDescending) { + d.groupOrder = message.GroupOrderDescending } } } return d } -// decodeDynamicGroups interprets a DYNAMIC_GROUPS value (§12.6): 0 is false, -// 1 is true, and anything greater is a PROTOCOL_VIOLATION so the caller can -// decline to act on it. -func decodeDynamicGroups(v uint64) (bool, error) { - switch v { - case 0: - return false, nil - case 1: - return true, nil - default: - return false, fmt.Errorf( - "relay: DYNAMIC_GROUPS value %d > 1 (§12.6 PROTOCOL_VIOLATION)", v) - } -} - // DynamicGroups reports whether the track advertised DYNAMIC_GROUPS=1 (§12.6), -// using the value decoded once when Properties was set. The error is a §12.6 -// PROTOCOL_VIOLATION (a DYNAMIC_GROUPS value > 1), or a structural failure -// parsing the Properties block; either way the §10.2.19 caller declines the -// NEW_GROUP_REQUEST rather than acting on it. +// using the value decoded once when Properties was set. The error is a +// structural failure parsing the Properties block; the §10.2.19 caller then +// declines the NEW_GROUP_REQUEST rather than acting on it. func (e *TrackEntry) DynamicGroups() (bool, error) { e.mu.RLock() defer e.mu.RUnlock() if e.decoded.parseErr != nil { return false, e.decoded.parseErr } - return e.decoded.dynamicGroups, e.decoded.dynamicGroupsErr + return e.decoded.dynamicGroups, nil } // DeliveryTimeouts returns the publisher's Track-level delivery timeouts (§8), @@ -125,7 +106,7 @@ func (e *TrackEntry) DeliveryTimeouts() message.DeliveryTimeouts { // DefaultGroupOrder is the publisher's Group Order preference, its // DEFAULT_PUBLISHER_GROUP_ORDER Track Property (§12.5): Ascending when it is // omitted ("If omitted, the publisher's preference is Ascending"), and when -// it is not an allowed value or the Properties are malformed. +// the Properties are malformed. func (e *TrackEntry) DefaultGroupOrder() message.GroupOrder { e.mu.RLock() defer e.mu.RUnlock() diff --git a/pkg/relay/param_values_test.go b/pkg/relay/param_values_test.go index 82104640..7de2052f 100644 --- a/pkg/relay/param_values_test.go +++ b/pkg/relay/param_values_test.go @@ -12,7 +12,8 @@ import ( // TestRelay_SessionFatalParamValuesClose: a request carrying a value the draft // makes session-fatal closes the relay's session, on every path the relay // serves: GROUP_ORDER outside {1, 2} on PUBLISH and FETCH and inside -// FILL_PARAMETERS (§10.2.8), and a LOCATION_FILTER whose end Group overflows +// FILL_PARAMETERS (§10.2.8), a DYNAMIC_GROUPS or DEFAULT_PUBLISHER_GROUP_ORDER +// Track Property out of range (§12.5, §12.6), and a LOCATION_FILTER whose end Group overflows // (§5.1.2, which the relay used to answer per request), on an opener and in a // REQUEST_UPDATE. func TestRelay_SessionFatalParamValuesClose(t *testing.T) { @@ -52,6 +53,18 @@ func TestRelay_SessionFatalParamValuesClose(t *testing.T) { Namespace: ns("video"), Name: []byte("cam1"), Parameters: message.Parameters{overflow}, }) }}, + {name: "DYNAMIC_GROUPS 5 in PUBLISH Track Properties", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Publish(t.Context(), &message.Publish{ + Namespace: ns("video"), Name: []byte("cam1"), TrackAlias: 1, + TrackProperties: message.AppendTrackProperties(trackProp(message.PropertyDynamicGroups, 5)), + }) + }}, + {name: "DEFAULT_PUBLISHER_GROUP_ORDER 3 in PUBLISH Track Properties", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Publish(t.Context(), &message.Publish{ + Namespace: ns("video"), Name: []byte("cam1"), TrackAlias: 1, + TrackProperties: message.AppendTrackProperties(trackProp(message.PropertyDefaultPublisherGroupOrder, 3)), + }) + }}, { name: "LOCATION_FILTER overflow in a REQUEST_UPDATE", prepare: func(t *testing.T, sess *session.Session) *session.Subscription { From ff309fc281add6566fe9283285acefb87c4ddb07 Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 20:18:39 +0500 Subject: [PATCH 3/9] =?UTF-8?q?fix(session,relay):=20close=20the=20session?= =?UTF-8?q?=20on=20a=20duplicate=20Track=20Alias=20(=C2=A711.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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) --- STATUS.md | 10 +- pkg/moqt/session/request.go | 7 +- pkg/moqt/session/subscribe_test.go | 127 +++++++++--------- pkg/moqt/session/trackalias.go | 22 ++- pkg/relay/handler_publish.go | 5 +- .../handler_publish_alias_window_test.go | 13 +- pkg/relay/session_publish_test.go | 22 +-- 7 files changed, 108 insertions(+), 98 deletions(-) diff --git a/STATUS.md b/STATUS.md index 9aa3343c..4e91fee4 100644 --- a/STATUS.md +++ b/STATUS.md @@ -211,7 +211,7 @@ By package, bottom-up along the dependency stack: | § | Feature | Status | Notes | |----------|--------------------------------------|---------|-------| -| 11.1 | Track alias | DONE | In subgroup header + datagram; validated. | +| 11.1 | Track alias | DONE | In subgroup header + datagram; validated. A SUBSCRIBE_OK or PUBLISH reusing the alias of a different registered Track closes the session with DUPLICATE_TRACK_ALIAS (`Session.RegisterInboundTrack`). | | 11.2 | Objects / object header | DONE | All header fields encoded. | | 11.2.1.1 | Object status | DONE | Normal / EndOfGroup / EndOfTrack. | | 11.2.1.2 | Object properties | DONE | Length-prefixed KV pairs. | @@ -518,10 +518,10 @@ already listed as Limitations above are not repeated here. Session layer: -- DUPLICATE_TRACK_ALIAS never closes the session: the relay answers REQUEST_ERROR - MALFORMED_TRACK, `Session.Subscribe` and `AcceptPublish` return an error (§11.1). - A session-layer subscriber also never releases an alias when its subscription - ends. +- A session-layer `Subscription` or `IncomingPublication` never releases its + Track Alias when it ends, so a publisher reusing it for another Track after + that closes the session with DUPLICATE_TRACK_ALIAS, which §11.1 reserves for + an alias of "a different Track with an Established subscription". - A GOAWAY on a request stream is ignored: a second one, or one carrying a New Session URI sent to a server, does not close the session (§10.4). - A REQUEST_ERROR Redirect is dropped after parsing: a server receiving a Connect diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index 87b41692..d15b972e 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -824,10 +824,9 @@ func (r *Request) AcceptSubscribe(ok *message.SubscribeOK) (*Publication, error) // [WithKnownMandatoryTrackProperties]) are rejected with REQUEST_ERROR — // UNSUPPORTED_EXTENSION for an unknown Mandatory Track Property (§2.5.1), // MALFORMED_TRACK for ones that do not parse — and the error returned. A -// session-fatal value (§12.5, §12.6) closed the session in AcceptRequest. On an -// alias collision *ErrDuplicateTrackAlias is returned without replying; the -// caller MUST close the session with [moqt.SessionDuplicateTrackAlias] -// (§11.1). +// session-fatal value (§12.5, §12.6) closed the session in AcceptRequest. An +// alias collision closes the session with DUPLICATE_TRACK_ALIAS and returns +// *ErrDuplicateTrackAlias (§11.1). func (r *Request) AcceptPublish() (*IncomingPublication, error) { pub, isPub := r.First.(*message.Publish) if !isPub { diff --git a/pkg/moqt/session/subscribe_test.go b/pkg/moqt/session/subscribe_test.go index 1438e273..24bc9a67 100644 --- a/pkg/moqt/session/subscribe_test.go +++ b/pkg/moqt/session/subscribe_test.go @@ -237,8 +237,8 @@ func TestSubscribe_ContextCancelUnblocksResponseWait(t *testing.T) { // TestRegisterInboundTrackAlias verifies the Session.RegisterInboundTrackAlias // method directly: same alias + same track is counted (§5.1 lets concurrent // subscriptions to one Track share an alias), and stays registered until each -// registration is released; same alias + different track returns -// *ErrDuplicateTrackAlias. +// registration is released; same alias + different track closes the session +// with DUPLICATE_TRACK_ALIAS and returns *ErrDuplicateTrackAlias (§11.1). func TestRegisterInboundTrackAlias(t *testing.T) { cli, _ := openPair(t) @@ -255,16 +255,6 @@ func TestRegisterInboundTrackAlias(t *testing.T) { t.Fatalf("second register: %v", err) } - // Same alias, different track → ErrDuplicateTrackAlias. - err := cli.RegisterInboundTrackAlias(42, keyB) - var dupErr *session.ErrDuplicateTrackAlias - if !errors.As(err, &dupErr) { - t.Fatalf("register different track: error = %v (%T), want *session.ErrDuplicateTrackAlias", err, err) - } - if dupErr.Alias != 42 { - t.Errorf("Alias = %d, want 42", dupErr.Alias) - } - // Different alias, same track → fine (multiple aliases can point to the same track). if err := cli.RegisterInboundTrackAlias(99, keyA); err != nil { t.Fatalf("different alias same track: %v", err) @@ -286,63 +276,74 @@ func TestRegisterInboundTrackAlias(t *testing.T) { // Unregistering a non-existent alias is a no-op. cli.UnregisterInboundTrackAlias(12345) -} -// TestSubscribeDuplicateTrackAlias verifies that when the server assigns the -// same Track Alias to two different tracks via SUBSCRIBE_OK, the second -// Subscribe call returns *ErrDuplicateTrackAlias. -func TestSubscribeDuplicateTrackAlias(t *testing.T) { - cli, srv := openPair(t) - ctx := t.Context() + // Same alias, different track → DUPLICATE_TRACK_ALIAS. + err := cli.RegisterInboundTrackAlias(99, keyB) + dupErr, ok := errors.AsType[*session.ErrDuplicateTrackAlias](err) + if !ok { + t.Fatalf("register different track: error = %v (%T), want *session.ErrDuplicateTrackAlias", err, err) + } + if dupErr.Alias != 99 { + t.Errorf("Alias = %d, want 99", dupErr.Alias) + } + requireClosedCode(t, cli, moqt.SessionDuplicateTrackAlias) +} - const sharedAlias = uint64(7) +// TestDuplicateTrackAliasCloses: a SUBSCRIBE_OK or PUBLISH that reuses the +// Track Alias of a different Track with an Established subscription closes the +// session with DUPLICATE_TRACK_ALIAS (§11.1), whichever message bound it +// first. +func TestDuplicateTrackAliasCloses(t *testing.T) { + const alias = uint64(7) + ns := wire.TrackNamespace{[]byte("ns")} - // Helper: run one subscribe round-trip. - doSubscribe := func(ns wire.TrackNamespace, name []byte) error { - var ( - wg sync.WaitGroup - srvErr error - cliErr error - ) - wg.Go(func() { - r, err := srv.AcceptRequest(ctx) - if err != nil { - srvErr = err - return + // subscribe has srv answer cli's SUBSCRIBE to name with alias, and + // returns cli's result. + subscribe := func(t *testing.T, cli, srv *session.Session, name string) error { + t.Helper() + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.SubscribeOK{TrackAlias: alias}) } - srvErr = r.Reply(&message.SubscribeOK{TrackAlias: sharedAlias}) - }) - wg.Go(func() { - stream, err := cli.Subscribe(ctx, &message.Subscribe{ - Namespace: ns, - Name: name, - }) - if err != nil { - cliErr = err - return + }() + _, err := cli.Subscribe(t.Context(), &message.Subscribe{Namespace: ns, Name: []byte(name)}) + return err + } + // publish has srv PUBLISH name to cli with alias, and returns cli's + // AcceptPublish result. + publish := func(t *testing.T, cli, srv *session.Session, name string) error { + t.Helper() + go func() { + _, _ = srv.Publish(t.Context(), &message.Publish{Namespace: ns, Name: []byte(name), TrackAlias: alias}) + }() + r, err := cli.AcceptRequest(t.Context()) + if err != nil { + return err + } + _, err = r.AcceptPublish() + return err + } + + for _, tc := range []struct { + name string + first, second func(t *testing.T, cli, srv *session.Session, name string) error + }{ + {"SUBSCRIBE_OK after SUBSCRIBE_OK", subscribe, subscribe}, + {"PUBLISH after PUBLISH", publish, publish}, + {"PUBLISH after SUBSCRIBE_OK", subscribe, publish}, + {"SUBSCRIBE_OK after PUBLISH", publish, subscribe}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + if err := tc.first(t, cli, srv, "trackA"); err != nil { + t.Fatalf("first: %v", err) } - _ = stream.Close() + err := tc.second(t, cli, srv, "trackB") + if dupErr, ok := errors.AsType[*session.ErrDuplicateTrackAlias](err); !ok || dupErr.Alias != alias { + t.Fatalf("second: error = %v (%T), want *session.ErrDuplicateTrackAlias for %d", err, err, alias) + } + requireClosedCode(t, cli, moqt.SessionDuplicateTrackAlias) }) - wg.Wait() - if srvErr != nil { - t.Fatalf("server: %v", srvErr) - } - return cliErr - } - - // First subscribe: alias 7 → (ns, "trackA"). Should succeed. - if err := doSubscribe(wire.TrackNamespace{[]byte("ns")}, []byte("trackA")); err != nil { - t.Fatalf("first Subscribe: %v", err) - } - - // Second subscribe: alias 7 → (ns, "trackB"). Should fail with ErrDuplicateTrackAlias. - err := doSubscribe(wire.TrackNamespace{[]byte("ns")}, []byte("trackB")) - var dupErr *session.ErrDuplicateTrackAlias - if !errors.As(err, &dupErr) { - t.Fatalf("second Subscribe: error = %v (%T), want *session.ErrDuplicateTrackAlias", err, err) - } - if dupErr.Alias != sharedAlias { - t.Errorf("Alias = %d, want %d", dupErr.Alias, sharedAlias) } } diff --git a/pkg/moqt/session/trackalias.go b/pkg/moqt/session/trackalias.go index d515ba9f..fe501f8e 100644 --- a/pkg/moqt/session/trackalias.go +++ b/pkg/moqt/session/trackalias.go @@ -5,6 +5,7 @@ import ( "fmt" "time" + "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/track" ) @@ -22,7 +23,8 @@ func (s *Session) AllocOutboundTrackAlias() uint64 { // ErrDuplicateTrackAlias is returned by [Session.RegisterInboundTrack] when the // peer assigns a Track Alias that is already in use for a different track -// (§11.1). The caller MUST close the session with SessionDuplicateTrackAlias. +// (§11.1). The session is already closed with +// [moqt.SessionDuplicateTrackAlias]. type ErrDuplicateTrackAlias struct { Alias uint64 Existing track.Key @@ -64,20 +66,28 @@ type InboundTrack struct { // a shared alias carries, and a release cannot tell which registration it // ends, so the survivor may keep a released one's properties. // -// If alias is registered for a different track, *ErrDuplicateTrackAlias is -// returned and the caller MUST close the session with -// SessionDuplicateTrackAlias (§11.1). +// If alias is registered for a different track, the session is closed with +// DUPLICATE_TRACK_ALIAS and *ErrDuplicateTrackAlias returned: "it MUST close +// the session with error DUPLICATE_TRACK_ALIAS" (§11.1). func (s *Session) RegisterInboundTrack(alias uint64, key track.Key, trackProperties []byte) error { in := InboundTrack{ Key: key, DefaultPublisherPriority: message.TrackDefaultPublisherPriority(trackProperties), } in.MaxCacheDuration, in.HasMaxCacheDuration = message.TrackMaxCacheDuration(trackProperties) + if err := s.registerInboundTrack(alias, in); err != nil { + _ = s.Close(moqt.SessionDuplicateTrackAlias, err.Error()) + return err + } + return nil +} + +func (s *Session) registerInboundTrack(alias uint64, in InboundTrack) error { s.mu.Lock() defer s.mu.Unlock() if existing, ok := s.inboundAliases[alias]; ok { - if existing.Key != key { - return &ErrDuplicateTrackAlias{Alias: alias, Existing: existing.Key, New: key} + if existing.Key != in.Key { + return &ErrDuplicateTrackAlias{Alias: alias, Existing: existing.Key, New: in.Key} } s.inboundAliases[alias] = in s.inboundAliasRefs[alias]++ diff --git a/pkg/relay/handler_publish.go b/pkg/relay/handler_publish.go index 13b9f805..c3b2cd11 100644 --- a/pkg/relay/handler_publish.go +++ b/pkg/relay/handler_publish.go @@ -7,7 +7,6 @@ import ( "slices" "sync/atomic" - "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/session" "github.com/floatdrop/moq-go/pkg/moqt/track" @@ -61,15 +60,13 @@ func (h *sessionHandler) handlePublish(ctx context.Context, req *session.Request _, createdEntry := h.tracks.GetOrCreateNew(fullName) // §11.1: register the publisher's chosen alias so the fanout path can map - // it back to the track and duplicates are detected. A duplicate alias is a - // session-level error per spec, but we scope the failure to this request. + // it back to the track. A duplicate closed the session. if err := h.sess.RegisterInboundTrack(msg.TrackAlias, fullName.Key(), msg.TrackProperties); err != nil { h.log.LogAttrs(ctx, slog.LevelDebug, "PUBLISH alias registration failed", slog.String("err", err.Error())) if createdEntry { h.tracks.DeleteIfUnused(fullName) } - _ = req.RejectError(moqt.RequestMalformedTrack, err.Error()) return } if hook := testHookAfterAliasRegistered.Load(); hook != nil { diff --git a/pkg/relay/handler_publish_alias_window_test.go b/pkg/relay/handler_publish_alias_window_test.go index 40f56a1e..b9e183ce 100644 --- a/pkg/relay/handler_publish_alias_window_test.go +++ b/pkg/relay/handler_publish_alias_window_test.go @@ -81,9 +81,10 @@ func TestPublish_TrackEntryPrecedesAliasRouting(t *testing.T) { waitRelayLargest(t, probe, video, name, groupID, 0) } -// TestPublish_RejectedAliasLeavesTrackUnknown: a rejected PUBLISH leaves no -// track entry behind, so a FETCH is DOES_NOT_EXIST (§10.6), not INVALID_RANGE. -func TestPublish_RejectedAliasLeavesTrackUnknown(t *testing.T) { +// TestPublish_DuplicateAliasLeavesTrackUnknown: a PUBLISH whose duplicate +// alias closes the session leaves no track entry behind, so a FETCH is +// DOES_NOT_EXIST (§10.6), not INVALID_RANGE. +func TestPublish_DuplicateAliasLeavesTrackUnknown(t *testing.T) { video := ns("video") const alias = uint64(91) first := []byte("cam-alias-taken") @@ -100,12 +101,12 @@ func TestPublish_RejectedAliasLeavesTrackUnknown(t *testing.T) { } t.Cleanup(func() { _ = pub.Close() }) - // §11.1: the alias is taken, so this PUBLISH is rejected — after the - // relay has already created the entry for `second`. + // §11.1: the alias is taken, so this PUBLISH closes the session — after + // the relay has already created the entry for `second`. if _, err := pubSess.Publish(t.Context(), &message.Publish{ Namespace: video, Name: second, TrackAlias: alias, }); err == nil { - t.Fatal("duplicate Track Alias PUBLISH was accepted, want rejection") + t.Fatal("duplicate Track Alias PUBLISH was accepted, want the session closed") } fetcher := dialAnotherClient(t, pubSess) diff --git a/pkg/relay/session_publish_test.go b/pkg/relay/session_publish_test.go index 06a4187b..941111ce 100644 --- a/pkg/relay/session_publish_test.go +++ b/pkg/relay/session_publish_test.go @@ -5,7 +5,6 @@ import ( "testing" "time" - "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/session" "github.com/floatdrop/moq-go/pkg/moqt/track" @@ -35,20 +34,23 @@ func TestPublish_AcceptedAndRegistered(t *testing.T) { } } -// TestPublish_DuplicateAliasRejected: reusing a Track Alias on the session for -// another track refuses the PUBLISH (§11.1). -func TestPublish_DuplicateAliasRejected(t *testing.T) { +// TestPublish_DuplicateAliasClosesSession: reusing the Track Alias of a +// published track on the session for another track closes the session with +// DUPLICATE_TRACK_ALIAS (§11.1). +func TestPublish_DuplicateAliasClosesSession(t *testing.T) { t.Parallel() clientSess, teardown := connectRelay(t, relay.Config{}) defer teardown() publishVideoTrack(t, clientSess, "cam1", 7) - _, err := clientSess.Publish(t.Context(), &message.Publish{ - Namespace: ns("video"), - Name: []byte("cam2"), - TrackAlias: 7, - }) - requireRejectedWithCode(t, err, moqt.RequestMalformedTrack) + go func() { + _, _ = clientSess.Publish(t.Context(), &message.Publish{ + Namespace: ns("video"), + Name: []byte("cam2"), + TrackAlias: 7, + }) + }() + requireSessionClosed(t, clientSess, "a PUBLISH reusing another track's alias") } // TestPublish_SavesLargestObjectFromPublish: LARGEST_OBJECT on an inbound From 05abc1bba67de27fa2aeb295ebe43ea7c778c4dc Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 20:40:25 +0500 Subject: [PATCH 4/9] =?UTF-8?q?fix(session,relay):=20release=20a=20Track?= =?UTF-8?q?=20Alias=20when=20its=20subscription=20is=20Terminated=20(?= =?UTF-8?q?=C2=A75.1,=20=C2=A711.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §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) --- STATUS.md | 6 +- pkg/moqt/session/broker.go | 25 ++- pkg/moqt/session/publish.go | 3 + pkg/moqt/session/request.go | 75 ++++++- pkg/moqt/session/subscribe.go | 18 +- pkg/moqt/session/subscribe_test.go | 204 ++++++++++++++++++ pkg/moqt/session/trackalias.go | 4 +- pkg/relay/handler_publish.go | 8 +- pkg/relay/handler_subscribe.go | 10 +- pkg/relay/internal/registry/broker_test.go | 3 +- pkg/relay/internal/registry/subscription.go | 12 +- .../internal/registry/subscription_test.go | 10 +- pkg/relay/session_upstream_test.go | 30 +++ 13 files changed, 363 insertions(+), 45 deletions(-) diff --git a/STATUS.md b/STATUS.md index 4e91fee4..76876bc7 100644 --- a/STATUS.md +++ b/STATUS.md @@ -211,7 +211,7 @@ By package, bottom-up along the dependency stack: | § | Feature | Status | Notes | |----------|--------------------------------------|---------|-------| -| 11.1 | Track alias | DONE | In subgroup header + datagram; validated. A SUBSCRIBE_OK or PUBLISH reusing the alias of a different registered Track closes the session with DUPLICATE_TRACK_ALIAS (`Session.RegisterInboundTrack`). | +| 11.1 | Track alias | DONE | In subgroup header + datagram; validated. A SUBSCRIBE_OK or PUBLISH reusing the alias of a different registered Track closes the session with DUPLICATE_TRACK_ALIAS (`Session.RegisterInboundTrack`). A `Subscription` or `IncomingPublication` releases its alias once Terminated (§5.1): on its own or its broker's `Close`, when the publisher's FIN is read through it or its broker's `Serve`, or when `Serve` cancels the stream. | | 11.2 | Objects / object header | DONE | All header fields encoded. | | 11.2.1.1 | Object status | DONE | Normal / EndOfGroup / EndOfTrack. | | 11.2.1.2 | Object properties | DONE | Length-prefixed KV pairs. | @@ -518,10 +518,6 @@ already listed as Limitations above are not repeated here. Session layer: -- A session-layer `Subscription` or `IncomingPublication` never releases its - Track Alias when it ends, so a publisher reusing it for another Track after - that closes the session with DUPLICATE_TRACK_ALIAS, which §11.1 reserves for - an alias of "a different Track with an Established subscription". - A GOAWAY on a request stream is ignored: a second one, or one carrying a New Session URI sent to a server, does not close the session (§10.4). - A REQUEST_ERROR Redirect is dropped after parsing: a server receiving a Connect diff --git a/pkg/moqt/session/broker.go b/pkg/moqt/session/broker.go index 795b41a9..d63ae096 100644 --- a/pkg/moqt/session/broker.go +++ b/pkg/moqt/session/broker.go @@ -68,6 +68,10 @@ type RequestBroker struct { // See [RequestBroker.PeerMessages]. noPeerUpdate bool noPeerNotify bool + + // handle is the typed handle this broker came from, if any: Serve and + // Close report the subscription's end to it (see requestHandle.terminated). + handle *requestHandle } // PeerMessages declares whether the peer may send REQUEST_UPDATE (§10.9) and @@ -321,9 +325,14 @@ func (b *RequestBroker) closeUpdates() { // Close cancels the request (§3.3.3): pending and future Updates fail with // [ErrRequestStreamClosed] and both directions are reset with code, which -// unblocks a running Serve. Serialized against in-flight writes; idempotent. +// unblocks a running Serve. On a broker from a [Subscription] or +// [IncomingPublication] the subscription is Terminated (§5.1) and its Track +// Alias released (§11.1). Serialized against in-flight writes; idempotent. // Must not be called with locks that Serve's callback might need held. func (b *RequestBroker) Close(code moqt.StreamResetCode) { + if b.handle != nil { + b.handle.terminated() + } b.closeUpdates() b.mu.Lock() defer b.mu.Unlock() @@ -350,8 +359,18 @@ func (b *RequestBroker) Close(code moqt.StreamResetCode) { // also closes the session with PROTOCOL_VIOLATION (§10). Serve returns nil on // a clean FIN or an onMsg stop, ctx.Err() on cancellation, and the read/token // error otherwise. +// +// On a broker from a [Subscription] or [IncomingPublication], every exit but +// an onMsg stop Terminates the subscription (§5.1) and releases its Track +// Alias (§11.1). func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) bool) error { defer b.closeUpdates() + stopped := false // by onMsg: the stream may still carry the subscription + defer func() { + if !stopped && b.handle != nil { + b.handle.terminated() + } + }() stop := context.AfterFunc(ctx, func() { b.stream.CancelRead(uint64(moqt.StreamResetSessionClosed)) }) @@ -368,6 +387,9 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b case ctx.Err() != nil: return ctx.Err() case errors.Is(err, io.EOF): + if b.handle != nil { + b.handle.peerFinished() + } return nil case errors.Is(err, message.ErrMalformedMessage): // §10, and §10.2 for an unknown parameter. @@ -450,6 +472,7 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b } if onMsg != nil && !onMsg(msg) { + stopped = true return nil } } diff --git a/pkg/moqt/session/publish.go b/pkg/moqt/session/publish.go index d197d026..05d05f0d 100644 --- a/pkg/moqt/session/publish.go +++ b/pkg/moqt/session/publish.go @@ -211,6 +211,9 @@ func (p *Publication) Done(code moqt.PublishDoneCode, reason string) error { // stays open for follow-ups — PUBLISH_DONE from the publisher, or a // REQUEST_UPDATE this side sends via [IncomingPublication.Update] to adjust // forwarding (§10.9). Close it to end the reception. +// +// Its Track Alias is released (§11.1) when the subscription is Terminated +// (§5.1), as for a [Subscription]. type IncomingPublication struct { // requestHandle carries the PUBLISH request stream — still open for // follow-up traffic (inbound PUBLISH_DONE, outbound REQUEST_UPDATE; diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index d15b972e..b6e80407 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "math" "slices" "sync" @@ -484,15 +485,62 @@ type requestHandle struct { // its REQUEST_UPDATEs; applied to the broker on creation. peerUpdate, peerNotify bool updateScope message.ParamScope + + // inboundAlias is the inbound Track Alias this request registered + // (§11.1), when holdsAlias: the receiving side of a subscription. It is + // released once the subscription is Terminated (§5.1); see terminated. + inboundAlias uint64 + holdsAlias bool + released atomic.Bool + // peerDone records that the publisher's FIN was read. + peerDone atomic.Bool +} + +// Read reads the request stream. On the receiving side of a subscription +// ([Subscription], [IncomingPublication]) the publisher's FIN ends it — "by +// sending PUBLISH_DONE and closing the stream" (§5.1) — which releases the +// Track Alias (§11.1). +func (h *requestHandle) Read(p []byte) (int, error) { + n, err := h.Stream.Read(p) + if errors.Is(err, io.EOF) { + h.peerFinished() + } + return n, err +} + +// peerFinished records the publisher's FIN on the receiving side of a +// subscription, which Terminates it (§5.1). +func (h *requestHandle) peerFinished() { + if h.holdsAlias { + h.peerDone.Store(true) + h.terminated() + } +} + +// terminated releases the inbound Track Alias the first time the subscription +// is Terminated: a handle can see that more than once (Close after the FIN), +// and a second release would take a registration another subscription +// sharing the alias holds (§5.1). +func (h *requestHandle) terminated() { + if h.holdsAlias && h.released.CompareAndSwap(false, true) { + h.s.UnregisterInboundTrackAlias(h.inboundAlias) + } } // Close cancels the request by resetting both stream directions (§3.3.3). If // this side already sent its final message and FIN (e.g. [Publication.Done]), -// only reading is stopped, so that message is not lost. +// only reading is stopped, so that message is not lost. A subscription is then +// Terminated (§5.1), so its Track Alias is released. // -// Close does not know whether the peer already completed the request; after -// that, §3.3.2 says the requester SHOULD FIN, so use Stream.Close instead. +// Once a subscription's publisher has completed it (its FIN was read through +// the handle or its broker), Close FINs instead, as §3.3.2 asks once nothing +// further will be sent. Otherwise Close cannot know whether the peer completed the +// request; after that, use Stream.Close to FIN. func (h *requestHandle) Close() error { + h.terminated() + if h.peerDone.Load() { + return h.Stream.Close() + } if h.finished.Load() { h.Stream.CancelRead(uint64(moqt.StreamResetCancelled)) return nil @@ -516,6 +564,7 @@ func (h *requestHandle) Broker() *RequestBroker { b := h.s.NewRequestBroker(h.Stream) b.PeerMessages(h.peerUpdate, h.peerNotify) b.UpdateScope(h.updateScope) + b.handle = h h.broker.Store(b) }) return h.broker.Load() @@ -530,7 +579,7 @@ func (h *requestHandle) Update(ctx context.Context, params message.Parameters) ( if b := h.broker.Load(); b != nil { return b.Update(ctx, params) } - return h.s.UpdateRequest(ctx, h.Stream, params) + return h.s.UpdateRequest(ctx, h, params) // h, so Read sees a FIN } // writeThenClose writes msg and FINs the send side, through the broker's @@ -841,17 +890,21 @@ func (r *Request) AcceptPublish() (*IncomingPublication, error) { return nil, err } if err := message.Marshal(r.Stream, &message.RequestOK{}); err != nil { + // Never Established (§5.1), so the alias is not held (§11.1). + r.s.UnregisterInboundTrackAlias(pub.TrackAlias) return nil, fmt.Errorf("moqt/session: write PUBLISH REQUEST_OK: %w", err) } // The publisher may send REQUEST_UPDATE (§10.9) and PUBLISH_STATE_NOTIFY // (§10.10). return &IncomingPublication{ - Stream: r.Stream, - s: r.s, - requestID: pub.RequestID, - peerUpdate: true, - peerNotify: true, - updateScope: message.ScopeUpdateFromPublisher, - alias: pub.TrackAlias, + Stream: r.Stream, + s: r.s, + requestID: pub.RequestID, + peerUpdate: true, + peerNotify: true, + updateScope: message.ScopeUpdateFromPublisher, + inboundAlias: pub.TrackAlias, + holdsAlias: true, + alias: pub.TrackAlias, }, nil } diff --git a/pkg/moqt/session/subscribe.go b/pkg/moqt/session/subscribe.go index 45b2282a..87387112 100644 --- a/pkg/moqt/session/subscribe.go +++ b/pkg/moqt/session/subscribe.go @@ -13,6 +13,12 @@ import ( // publisher-assigned Track Alias — so the caller can send REQUEST_UPDATE via // [Subscription.Update] without holding them separately. It is returned by // [Session.Subscribe]. +// +// The Track Alias stays registered while the subscription is Established and +// is released (§11.1) once it is Terminated (§5.1): on [Subscription.Close] or +// its broker's [RequestBroker.Close], when the publisher's FIN is read through +// the Subscription or by its broker's [RequestBroker.Serve], or when Serve +// cancels the stream. type Subscription struct { // requestHandle carries the SUBSCRIBE request stream — still open for // follow-up traffic (REQUEST_UPDATE and inbound PUBLISH_DONE; Close it @@ -55,11 +61,13 @@ func (s *Session) Subscribe(ctx context.Context, m *message.Subscribe) (*Subscri // The publisher may send PUBLISH_STATE_NOTIFY (§10.10) but not // REQUEST_UPDATE (§10.9). return &Subscription{ - Stream: stream, - s: s, - requestID: m.RequestID, - peerNotify: true, - OK: ok, + Stream: stream, + s: s, + requestID: m.RequestID, + peerNotify: true, + inboundAlias: ok.TrackAlias, + holdsAlias: true, + OK: ok, }, nil }) } diff --git a/pkg/moqt/session/subscribe_test.go b/pkg/moqt/session/subscribe_test.go index 24bc9a67..51e70457 100644 --- a/pkg/moqt/session/subscribe_test.go +++ b/pkg/moqt/session/subscribe_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "sync" "testing" "time" @@ -347,6 +348,209 @@ func TestDuplicateTrackAliasCloses(t *testing.T) { } } +// aliasHandle is the receiving side of a subscription: a [session.Subscription] +// or a [session.IncomingPublication]. +type aliasHandle interface { + io.Reader + Close() error + Broker() *session.RequestBroker +} + +// finishPublication has pub end with PUBLISH_DONE and FIN, and h consume them: +// through its broker when served, else by reading h itself. +func finishPublication(t *testing.T, h aliasHandle, pub *session.Publication, served bool) { + t.Helper() + done := make(chan error, 1) + go func() { done <- pub.Done(moqt.PublishDoneTrackEnded, "") }() + var err error + if served { + err = h.Broker().Serve(t.Context(), nil) + } else { + for err == nil { + _, err = message.Parse(h) + } + if errors.Is(err, io.EOF) { + err = nil + } + } + if err != nil { + t.Fatalf("consuming PUBLISH_DONE and FIN: %v", err) + } + if err := <-done; err != nil { + t.Fatalf("Done: %v", err) + } +} + +// establishOnAlias establishes a subscription to ns/name whose publisher, srv, +// assigns alias: cli's SUBSCRIBE answered with SUBSCRIBE_OK, or srv's PUBLISH +// accepted by cli. It returns both ends. +func establishOnAlias( + t *testing.T, cli, srv *session.Session, viaPublish bool, name string, alias uint64, +) (aliasHandle, *session.Publication) { + t.Helper() + ns := wire.TrackNamespace{[]byte("ns")} + pubs := make(chan *session.Publication, 1) + if viaPublish { + go func() { + p, err := srv.Publish(t.Context(), &message.Publish{Namespace: ns, Name: []byte(name), TrackAlias: alias}) + if err != nil { + t.Errorf("Publish: %v", err) + } + pubs <- p + }() + r, err := cli.AcceptRequest(t.Context()) + if err != nil { + t.Fatalf("AcceptRequest: %v", err) + } + in, err := r.AcceptPublish() + if err != nil { + t.Fatalf("AcceptPublish: %v", err) + } + return in, <-pubs + } + go func() { + r, err := srv.AcceptRequest(t.Context()) + if err != nil { + t.Errorf("AcceptRequest: %v", err) + pubs <- nil + return + } + p, err := r.AcceptSubscribe(&message.SubscribeOK{TrackAlias: alias}) + if err != nil { + t.Errorf("AcceptSubscribe: %v", err) + } + pubs <- p + }() + sub, err := cli.Subscribe(t.Context(), &message.Subscribe{Namespace: ns, Name: []byte(name)}) + if err != nil { + t.Fatalf("Subscribe: %v", err) + } + return sub, <-pubs +} + +// TestAliasReleasedWhenTerminated: a Subscription or IncomingPublication +// releases its Track Alias once the subscription is Terminated (§5.1), so the +// publisher may then use it for a different Track (§11.1) without the session +// closing with DUPLICATE_TRACK_ALIAS. +func TestAliasReleasedWhenTerminated(t *testing.T) { + const alias = uint64(7) + for _, tc := range []struct { + name string + terminate func(t *testing.T, h aliasHandle, pub *session.Publication) + }{ + {"subscriber Close", func(_ *testing.T, h aliasHandle, _ *session.Publication) { _ = h.Close() }}, + {"subscriber Broker Close", func(_ *testing.T, h aliasHandle, _ *session.Publication) { + h.Broker().Close(moqt.StreamResetCancelled) + }}, + {"PUBLISH_DONE and FIN, served", func(t *testing.T, h aliasHandle, pub *session.Publication) { + finishPublication(t, h, pub, true) + }}, + {"PUBLISH_DONE and FIN, read", func(t *testing.T, h aliasHandle, pub *session.Publication) { + finishPublication(t, h, pub, false) + }}, + {"Serve cancelled", func(t *testing.T, h aliasHandle, _ *session.Publication) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _ = h.Broker().Serve(ctx, nil) + }}, + } { + for _, viaPublish := range []bool{false, true} { + name := tc.name + ", SUBSCRIBE_OK" + if viaPublish { + name = tc.name + ", PUBLISH" + } + t.Run(name, func(t *testing.T) { + cli, srv := openPair(t) + h, pub := establishOnAlias(t, cli, srv, viaPublish, "trackA", alias) + tc.terminate(t, h, pub) + + establishOnAlias(t, cli, srv, false, "trackB", alias) + if err := cli.Err(); err != nil { + t.Fatalf("reusing a Terminated subscription's alias closed the session: %v", err) + } + }) + } + } +} + +// TestCloseAfterPublisherFinFINs: once the publisher has ended the +// subscription with PUBLISH_DONE and its FIN, Close FINs this side rather than +// resetting it (§3.3.2: the requester SHOULD FIN), whether the FIN was read +// through the handle or by its broker. +func TestCloseAfterPublisherFinFINs(t *testing.T) { + for _, served := range []bool{false, true} { + for _, viaPublish := range []bool{false, true} { + t.Run(fmt.Sprintf("served=%v publish=%v", served, viaPublish), func(t *testing.T) { + cli, srv := openPair(t) + h, pub := establishOnAlias(t, cli, srv, viaPublish, "trackA", 7) + finishPublication(t, h, pub, served) + if err := h.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if _, err := message.Parse(pub); !errors.Is(err, io.EOF) { + t.Fatalf("publisher read after Close: %v, want io.EOF (a FIN)", err) + } + }) + } + } +} + +// TestAcceptPublishWriteFailureReleasesAlias: a PUBLISH whose REQUEST_OK +// cannot be written never becomes Established (§5.1), so its alias is free for +// a different Track (§11.1). +func TestAcceptPublishWriteFailureReleasesAlias(t *testing.T) { + const alias = uint64(7) + cli, srv := openPair(t) + streams := make(chan session.Stream, 1) + go func() { + stream, err := srv.OpenPublish(&message.Publish{ + Namespace: wire.TrackNamespace{[]byte("ns")}, Name: []byte("trackA"), TrackAlias: alias, + }) + if err != nil { + t.Errorf("OpenPublish: %v", err) + } + streams <- stream + }() + r, err := cli.AcceptRequest(t.Context()) + if err != nil { + t.Fatalf("AcceptRequest: %v", err) + } + (<-streams).CancelRead(uint64(moqt.StreamResetCancelled)) + if _, err := r.AcceptPublish(); err == nil { + t.Fatal("AcceptPublish wrote REQUEST_OK to a stream the publisher stopped reading") + } + + establishOnAlias(t, cli, srv, false, "trackB", alias) + if err := cli.Err(); err != nil { + t.Fatalf("reusing the alias of a PUBLISH never accepted closed the session: %v", err) + } +} + +// TestAliasReleasedOncePerSubscription: two subscriptions to one Track share an +// alias (§5.1). Terminating one twice releases only its own registration, so +// the other keeps the alias Established and a different Track reusing it still +// closes the session (§11.1). +func TestAliasReleasedOncePerSubscription(t *testing.T) { + const alias = uint64(7) + cli, srv := openPair(t) + first, _ := establishOnAlias(t, cli, srv, false, "trackA", alias) + establishOnAlias(t, cli, srv, false, "trackA", alias) + + _ = first.Close() + first.Broker().Close(moqt.StreamResetCancelled) + _ = first.Broker().Serve(t.Context(), nil) + + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.SubscribeOK{TrackAlias: alias}) + } + }() + _, _ = cli.Subscribe(t.Context(), &message.Subscribe{ + Namespace: wire.TrackNamespace{[]byte("ns")}, Name: []byte("trackB"), + }) + requireClosedCode(t, cli, moqt.SessionDuplicateTrackAlias) +} + // TestSubscribeSameTrackAliasShared verifies that subscribing to the same // track twice with the same alias succeeds: §5.1 lets concurrent // subscriptions to one Track share an alias. diff --git a/pkg/moqt/session/trackalias.go b/pkg/moqt/session/trackalias.go index fe501f8e..5008726d 100644 --- a/pkg/moqt/session/trackalias.go +++ b/pkg/moqt/session/trackalias.go @@ -108,7 +108,9 @@ func (s *Session) RegisterInboundTrackAlias(alias uint64, key track.Key) error { // UnregisterInboundTrackAlias releases one registration of alias (see // [Session.RegisterInboundTrack]); the last release removes it. Call it once -// the subscription or publication is torn down, after a grace period (§11.1). +// the subscription is Terminated (§5.1), for an alias registered with +// RegisterInboundTrack directly: a [Subscription] or [IncomingPublication] +// releases its own. // // Unregistering an alias that was never registered is a no-op. func (s *Session) UnregisterInboundTrackAlias(alias uint64) { diff --git a/pkg/relay/handler_publish.go b/pkg/relay/handler_publish.go index c3b2cd11..cd26f353 100644 --- a/pkg/relay/handler_publish.go +++ b/pkg/relay/handler_publish.go @@ -73,8 +73,12 @@ func (h *sessionHandler) handlePublish(ctx context.Context, req *session.Request (*hook)(fullName) } - // The publisher sent the PUBLISH, so it may send REQUEST_UPDATE (§10.9). - sub := registry.NewUpstreamSub(h.allocSubID(), h.sess, req.Stream, msg.TrackAlias, msg.RequestID, true) + // The publisher sent the PUBLISH, so it may send REQUEST_UPDATE (§10.9), + // in the scope an accepted PUBLISH's publisher has. + broker := h.sess.NewRequestBroker(req.Stream) + broker.PeerMessages(true, true) + broker.UpdateScope(message.ScopeUpdateFromPublisher) + sub := registry.NewUpstreamSub(h.allocSubID(), h.sess, req.Stream, broker, msg.TrackAlias, msg.RequestID) // §5.1: the PUBLISH sets the initial Forward State (default 1). if f, ok := msg.Parameters.Find(message.ParamForward); ok && f.Byte == 0 { sub.SetForwardState(0) diff --git a/pkg/relay/handler_subscribe.go b/pkg/relay/handler_subscribe.go index 115ef202..040b51ad 100644 --- a/pkg/relay/handler_subscribe.go +++ b/pkg/relay/handler_subscribe.go @@ -475,10 +475,10 @@ func (h *sessionHandler) subscribeUpstreamOnSession( (*hook)(fullName) } - // The relay is the requester, so the publisher may not send - // REQUEST_UPDATE (§10.9). - upstreamSub := registry.NewUpstreamSub( - h.allocSubID(), sess, upstreamStream, upstreamStream.OK.TrackAlias, subMsg.RequestID, false) + // The Subscription's broker: the publisher may not send REQUEST_UPDATE + // (§10.9), and the session releases the alias when it ends (§11.1). + upstreamSub := registry.NewUpstreamSub(h.allocSubID(), sess, upstreamStream, upstreamStream.Broker(), + upstreamStream.OK.TrackAlias, subMsg.RequestID) upstreamSub.SetFilter(filter) // Match the Forward=0 sent upstream: NewUpstreamSub starts at 1, and a // later §9.2 resume skips upstreams already at 1. @@ -498,8 +498,6 @@ func (h *sessionHandler) subscribeUpstreamOnSession( h.relayGo(func() { h.serveUpstreamStream(upstreamStream.Context(), upstreamSub) h.tracks.RemoveUpstream(fullName, upstreamSub.ID) - // Drop the alias with the subscription, so a peer may reuse it (§11.1). - sess.UnregisterInboundTrackAlias(upstreamStream.OK.TrackAlias) }) return entry, upstreamSub, nil diff --git a/pkg/relay/internal/registry/broker_test.go b/pkg/relay/internal/registry/broker_test.go index 0d7611c6..bb56cfce 100644 --- a/pkg/relay/internal/registry/broker_test.go +++ b/pkg/relay/internal/registry/broker_test.go @@ -14,7 +14,8 @@ import ( // with [session.ErrRequestStreamClosed]. func TestUpstreamSub_UpdateDelegatesToBroker(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(1, nil, stubStream{}, 0, 7, false) + var sess *session.Session + sub := registry.NewUpstreamSub(1, nil, stubStream{}, sess.NewRequestBroker(stubStream{}), 0, 7) sub.CloseOnDemand() if !sub.IsTerminated() { diff --git a/pkg/relay/internal/registry/subscription.go b/pkg/relay/internal/registry/subscription.go index 2f293aef..601879a3 100644 --- a/pkg/relay/internal/registry/subscription.go +++ b/pkg/relay/internal/registry/subscription.go @@ -322,20 +322,16 @@ func (u *UpstreamSub) Cancel(code moqt.StreamResetCode) { // The Forward State starts at 1: an omitted FORWARD means 1 (§10.2.18), and // the relay's upstream requests never carry it. // -// peerMayUpdate says whether the upstream publisher may send REQUEST_UPDATE: -// §10.9 allows it only from "The sender of a request", so true for an -// accepted PUBLISH and false for the relay's own SUBSCRIBE. +// broker owns the request stream's reads, set up by the caller: the +// [session.Subscription]'s own for the relay's SUBSCRIBE, so the session +// releases its Track Alias when it ends (§11.1). func NewUpstreamSub( id uint64, sess *session.Session, stream session.Stream, + broker *session.RequestBroker, trackAlias, requestID uint64, - peerMayUpdate bool, ) *UpstreamSub { - broker := sess.NewRequestBroker(stream) - broker.PeerMessages(peerMayUpdate, true) - // Only an accepted PUBLISH's publisher may update here (§10.9). - broker.UpdateScope(message.ScopeUpdateFromPublisher) return &UpstreamSub{ state: SubEstablished, ID: id, diff --git a/pkg/relay/internal/registry/subscription_test.go b/pkg/relay/internal/registry/subscription_test.go index 79c9b9da..df355832 100644 --- a/pkg/relay/internal/registry/subscription_test.go +++ b/pkg/relay/internal/registry/subscription_test.go @@ -37,7 +37,7 @@ func TestSubscription_BornEstablished(t *testing.T) { IsEstablished() bool IsTerminated() bool }{ - registry.NewUpstreamSub(1, nil, nil, 0, 0, false), + registry.NewUpstreamSub(1, nil, nil, nil, 0, 0), registry.NewDownstreamSub(1, nil, nil, 0), } { if !sub.IsEstablished() || sub.IsTerminated() { @@ -52,7 +52,7 @@ func TestSubscription_BornEstablished(t *testing.T) { // reports false. func TestSubscription_TerminateLatch(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(1, nil, nil, 0, 0, false) + sub := registry.NewUpstreamSub(1, nil, nil, nil, 0, 0) if !sub.Terminate() { t.Fatal("first Terminate returned false, want true") @@ -74,7 +74,7 @@ func TestSubscription_TerminateLatch(t *testing.T) { // and the relay's upstream requests never carry FORWARD. func TestSubscription_ForwardState(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(1, nil, nil, 0, 0, false) + sub := registry.NewUpstreamSub(1, nil, nil, nil, 0, 0) if got := sub.ForwardState(); got != 1 { t.Fatalf("initial ForwardState = %d, want 1 (§10.2.18 default)", got) } @@ -91,7 +91,7 @@ func TestSubscription_ForwardState(t *testing.T) { // TestUpstreamSub_FilterRoundTrip pins the upstream filter accessor pair. func TestUpstreamSub_FilterRoundTrip(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(7, nil, nil, 42, 0, false) + sub := registry.NewUpstreamSub(7, nil, nil, nil, 42, 0) if sub.GetFilter() != nil { t.Fatal("initial filter not nil") } @@ -214,7 +214,7 @@ func TestDownstreamSub_EffectiveStreamPriority(t *testing.T) { // - the final state is Terminated. func TestSubscription_ConcurrentTerminate(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(1, nil, nil, 0, 0, false) + sub := registry.NewUpstreamSub(1, nil, nil, nil, 0, 0) const goroutines = 32 var winners atomic.Int32 diff --git a/pkg/relay/session_upstream_test.go b/pkg/relay/session_upstream_test.go index b509415e..4724145d 100644 --- a/pkg/relay/session_upstream_test.go +++ b/pkg/relay/session_upstream_test.go @@ -273,6 +273,36 @@ func TestSubscribe_LastDownstreamTearsDownUpstream(t *testing.T) { } } +// TestSubscribe_UpstreamAliasReusableAfterTeardown: once the relay ends its +// upstream subscription, the publisher may use its Track Alias for a different +// Track (§11.1) without the relay closing the session with +// DUPLICATE_TRACK_ALIAS. +func TestSubscribe_UpstreamAliasReusableAfterTeardown(t *testing.T) { + t.Parallel() + pubSess := namespacePublisher(t, relay.Config{}) + subscriptionEnded := acceptUpstreamSubscribe(t, pubSess, 77) + + subSess := dialAnotherClient(t, pubSess) + _ = subscribeCam1(t, subSess).Close() + select { + case <-subscriptionEnded: + case <-time.After(2 * time.Second): + t.Fatal("publisher's subscription still open 2s after the last downstream left") + } + + acceptUpstreamSubscribe(t, pubSess, 77) + if _, err := subSess.Subscribe(t.Context(), &message.Subscribe{ + Namespace: ns("video"), Name: []byte("cam2"), + }); err != nil { + t.Fatalf("Subscribe cam2 on the reused alias: %v", err) + } + select { + case <-pubSess.Done(): + t.Fatal("relay closed the publisher's session on a reused alias") + case <-time.After(50 * time.Millisecond): + } +} + // TestSubscribe_UpstreamRejects_PropagatesRejection: an upstream REQUEST_ERROR // code about the track passes downstream; one about the relay's own hop becomes // INTERNAL_ERROR (§10.6.2). The Retry Interval is kept either way. From f5480b926eb78552c0f1180bc974203f9694af8f Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 20:52:02 +0500 Subject: [PATCH 5/9] =?UTF-8?q?fix(session,relay):=20close=20the=20session?= =?UTF-8?q?=20on=20a=20GOAWAY=20violation=20on=20a=20request=20stream=20(?= =?UTF-8?q?=C2=A710.4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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) --- STATUS.md | 7 ++- pkg/moqt/session/broker.go | 75 +++++++++++++--------- pkg/moqt/session/goaway.go | 44 +++++++++++-- pkg/moqt/session/request_goaway_test.go | 84 +++++++++++++++++++++++++ pkg/relay/request_goaway_test.go | 49 +++++++++++++++ pkg/relay/session_handler.go | 7 +++ 6 files changed, 228 insertions(+), 38 deletions(-) create mode 100644 pkg/moqt/session/request_goaway_test.go create mode 100644 pkg/relay/request_goaway_test.go diff --git a/STATUS.md b/STATUS.md index 76876bc7..4dc6c523 100644 --- a/STATUS.md +++ b/STATUS.md @@ -188,7 +188,7 @@ By package, bottom-up along the dependency stack: | 10.3.1.5| MOQT_IMPLEMENTATION | 0x07 | DONE | Advisory. | | 10.3.1.6| MAX_FILTER_RANGES | 0x06 | DONE | `WithMaxFilterRanges` advertises it; relay rejects over-limit/prohibited filters with INVALID_FILTER. The relay advertises `relay.DefaultMaxFilterRanges` (16) rather than inheriting the session default of 0, which would prohibit the Range Filters it fully implements; `relay.Config.MaxFilterRanges` overrides, negative to prohibit. | | 10.3.1.7| MAX_REQUEST_UPDATES | 0x08 | DONE | `WithMaxRequestUpdates` advertises the per-stream limit; enforced on inbound follow-ups via `RequestUpdateLimiter`, closing with `TOO_MANY_REQUEST_UPDATES` on overflow. | -| 10.4 | GOAWAY | 0x10 | DONE | Same encoding on control and request streams (draft-19 dropped the Request ID field); callback. As recipient the relay initiates no new SUBSCRIBE, FETCH or PUBLISH to the peer and leaves closing the session to the sender. | +| 10.4 | GOAWAY | 0x10 | DONE | Same encoding on control and request streams (draft-19 dropped the Request ID field); callback. As recipient the relay initiates no new SUBSCRIBE, FETCH or PUBLISH to the peer and leaves closing the session to the sender. On a request stream (`RequestBroker.Serve` and the relay's readers after the response; `session.RequestGoaways` for callers that read one themselves) a second GOAWAY, or one with a New Session URI received by a server, closes the session with PROTOCOL_VIOLATION; a single one is handed to the reader, and neither side migrates the request. | | 10.5 | REQUEST_OK | 0x07 | DONE | Shared OK for PUBLISH/UPDATE/TRACK_STATUS/namespace reqs. Track Properties where they must be empty close the session on receipt and are refused on send (`ErrTrackPropertiesNotAllowed`). | | 10.6 | REQUEST_ERROR (+ Redirect) | 0x05 | DONE | Redirect required only when code==REDIRECT. `Request.Reject` sends a Retry Interval; the relay invites a jittered ~1 s retry on EXCESSIVE_LOAD and passes an upstream SUBSCRIBE rejection on by meaning, Retry Interval kept. | | 10.7 | SUBSCRIBE | 0x03 | DONE | | @@ -518,8 +518,9 @@ already listed as Limitations above are not repeated here. Session layer: -- A GOAWAY on a request stream is ignored: a second one, or one carrying a New - Session URI sent to a server, does not close the session (§10.4). +- A GOAWAY on a request stream before its initial response, or on a + SUBSCRIBE_TRACKS stream read with `ReadPublishSkipped`, is an error rather + than a legal message checked against §10.4. - A REQUEST_ERROR Redirect is dropped after parsing: a server receiving a Connect URI, or a Track Name on a namespace-scoped request, does not close the session, and the application cannot follow it (§10.6.1). diff --git a/pkg/moqt/session/broker.go b/pkg/moqt/session/broker.go index d63ae096..9f2e38cc 100644 --- a/pkg/moqt/session/broker.go +++ b/pkg/moqt/session/broker.go @@ -30,6 +30,8 @@ import ( // [RequestBroker.HandleUpdates], or declined with NOT_SUPPORTED when there // is none, since acknowledging an unapplied update would misstate the // request's state. +// - A second GOAWAY on the stream, or one with a New Session URI received +// by a server, closes the session (§10.4; see [RequestGoaways]). // - Everything else (PUBLISH_DONE, unsolicited responses, …) is handed to // Serve's callback. // @@ -344,6 +346,43 @@ func (b *RequestBroker) Close(code moqt.StreamResetCode) { b.stream.CancelWrite(uint64(code)) } +// receiveUpdate checks and answers a peer REQUEST_UPDATE for Serve (§10.9). +// A non-nil error means Serve stops; a violation closed the session. +func (b *RequestBroker) receiveUpdate(m *message.RequestUpdate, updates *RequestUpdateLimiter) error { + if b.noPeerUpdate { + return b.sess.closeProtocolViolation(errors.New( + "moqt/session: REQUEST_UPDATE from a peer that may not send one")) + } + if b.updateScope != 0 { + if err := b.sess.CheckPeerParams(b.updateScope, m); err != nil { + return err + } + } + // §10.1: a REQUEST_UPDATE consumes a Request ID from the sender's space; + // a wrong-parity or duplicate ID is session-fatal. + if err := b.sess.CheckPeerRequestID(m.RequestID); err != nil { + _ = b.sess.Close(moqt.SessionInvalidRequestID, err.Error()) + return err + } + // §10.3.1.7: reject a REQUEST_UPDATE that exceeds the per-stream + // MAX_REQUEST_UPDATES limit before acting on it. + if err := updates.Received(); err != nil { + _ = b.sess.Close(moqt.SessionTooManyRequestUpdates, err.Error()) + return err + } + // §10.9: "MUST respond with exactly one REQUEST_OK or REQUEST_ERROR". + // onMsg still observes the update. + accepted, err := b.answerUpdate(m) + if err != nil { + return err + } + updates.Responded() + if !accepted && b.onUpdateFailed != nil { + b.onUpdateFailed() + } + return nil +} + // Serve owns every read on the request stream until the peer tears it down // (EOF / reset), ctx is cancelled (the read side is then reset to unblock // the parse), or onMsg returns false. On exit, pending and future Update @@ -379,6 +418,7 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b // §10.3.1.7: per-stream MAX_REQUEST_UPDATES enforcement. One limiter per // stream, since the limit is scoped to a single request stream. updates := b.sess.NewRequestUpdateLimiter() + var goaways RequestGoaways for { msg, err := message.Parse(b.stream) @@ -428,6 +468,10 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b continue } // Unsolicited response — surface via onMsg below. + case *message.Goaway: + if err := goaways.Received(b.sess, m); err != nil { + return err + } case *message.PublishStateNotify: if b.noPeerNotify { return b.sess.closeProtocolViolation(errors.New( @@ -437,38 +481,9 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b return err } case *message.RequestUpdate: - if b.noPeerUpdate { - return b.sess.closeProtocolViolation(errors.New( - "moqt/session: REQUEST_UPDATE from a peer that may not send one")) - } - if b.updateScope != 0 { - if err := b.sess.CheckPeerParams(b.updateScope, m); err != nil { - return err - } - } - // §10.1: a REQUEST_UPDATE consumes a Request ID from the - // sender's space; a wrong-parity or duplicate ID is - // session-fatal. - if err := b.sess.CheckPeerRequestID(m.RequestID); err != nil { - _ = b.sess.Close(moqt.SessionInvalidRequestID, err.Error()) - return err - } - // §10.3.1.7: reject a REQUEST_UPDATE that exceeds the per-stream - // MAX_REQUEST_UPDATES limit before acting on it. - if err := updates.Received(); err != nil { - _ = b.sess.Close(moqt.SessionTooManyRequestUpdates, err.Error()) + if err := b.receiveUpdate(m, updates); err != nil { return err } - // §10.9: "MUST respond with exactly one REQUEST_OK or - // REQUEST_ERROR". onMsg still observes the update. - accepted, err := b.answerUpdate(m) - if err != nil { - return err - } - updates.Responded() - if !accepted && b.onUpdateFailed != nil { - b.onUpdateFailed() - } } if onMsg != nil && !onMsg(msg) { diff --git a/pkg/moqt/session/goaway.go b/pkg/moqt/session/goaway.go index d0db817d..6f569ad8 100644 --- a/pkg/moqt/session/goaway.go +++ b/pkg/moqt/session/goaway.go @@ -2,6 +2,7 @@ package session import ( "errors" + "fmt" "time" "github.com/floatdrop/moq-go/pkg/moqt/message" @@ -86,12 +87,9 @@ func (s *Session) handleGoaway(m *message.Goaway) error { s.mu.Unlock() return errors.New("duplicate GOAWAY on control stream") } - // §10.4: a client cannot direct a server to migrate, so a non-empty URI - // from a client is a PROTOCOL_VIOLATION. From our perspective, that means - // if we are the server we must reject a GOAWAY with a URI. - if s.role == roleServer && len(m.NewSessionURI) > 0 { + if err := s.checkGoawayURI(m); err != nil { s.mu.Unlock() - return errors.New("GOAWAY from client carries non-empty URI") + return err } s.goawayReceived = m // Snapshot the registered handler under the lock and mark it fired so a @@ -110,3 +108,39 @@ func (s *Session) handleGoaway(m *message.Goaway) error { } return nil } + +// checkGoawayURI enforces §10.4 "If a server receives a GOAWAY with a +// non-zero New Session URI Length it MUST close the session with a +// PROTOCOL_VIOLATION": a client cannot direct a server to migrate. +func (s *Session) checkGoawayURI(m *message.Goaway) error { + if s.role == roleServer && len(m.NewSessionURI) > 0 { + return errors.New("GOAWAY from client carries non-empty URI") + } + return nil +} + +// RequestGoaways checks the GOAWAYs one request stream carries (§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 a server closes on one carrying a New Session URI. The zero value is +// ready for a stream; callers that read a request stream with [message.Parse] +// keep one per stream. +// +// Not safe for concurrent use, which matches the single reader of a request +// stream ([RequestBroker.Serve] and the relay's per-stream readers). +type RequestGoaways struct { + seen bool +} + +// Received checks a GOAWAY read off the stream. On a violation it closes s +// with PROTOCOL_VIOLATION and returns the error. +func (g *RequestGoaways) Received(s *Session, m *message.Goaway) error { + if g.seen { + return s.closeProtocolViolation(errors.New("moqt/session: second GOAWAY on a request stream")) + } + g.seen = true + if err := s.checkGoawayURI(m); err != nil { + return s.closeProtocolViolation(fmt.Errorf("moqt/session: request stream %w", err)) + } + return nil +} diff --git a/pkg/moqt/session/request_goaway_test.go b/pkg/moqt/session/request_goaway_test.go new file mode 100644 index 00000000..af5137f4 --- /dev/null +++ b/pkg/moqt/session/request_goaway_test.go @@ -0,0 +1,84 @@ +package session_test + +import ( + "context" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" +) + +// TestRequestStreamGoawayViolationCloses: a second GOAWAY on one request +// stream, or a GOAWAY carrying a New Session URI received by the server, +// closes the session with PROTOCOL_VIOLATION (§10.4), read by the receiving +// handle's broker. +func TestRequestStreamGoawayViolationCloses(t *testing.T) { + uri := &message.Goaway{NewSessionURI: []byte("https://relay.example/moq")} + for _, tc := range []struct { + name string + toServer bool + sent []*message.Goaway + }{ + {"second GOAWAY to the client", false, []*message.Goaway{{}, uri}}, + {"second GOAWAY to the server", true, []*message.Goaway{{}, {}}}, + {"New Session URI to the server", true, []*message.Goaway{uri}}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, cli, srv) + var from session.Stream = pub + receiver, broker := cli, sub.Broker() + if tc.toServer { + from = sub + receiver, broker = srv, pub.Broker() + } + go func() { + for _, m := range tc.sent { + if err := message.Marshal(from, m); err != nil { + return + } + } + }() + // Bounded: a GOAWAY that is ignored leaves the stream open. + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _ = broker.Serve(ctx, nil) + requireClosedCode(t, receiver, moqt.SessionProtocolViolation) + }) + } +} + +// TestRequestStreamGoawayOnceIsDelivered: one GOAWAY on a request stream, with +// a New Session URI when sent to the client, is legal (§10.4) and reaches +// Serve's callback. +func TestRequestStreamGoawayOnceIsDelivered(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, cli, srv) + go func() { + if err := message.Marshal( + pub, + &message.Goaway{NewSessionURI: []byte("https://relay.example/moq")}, + ); err != nil { + return + } + _ = pub.Done(moqt.PublishDoneGoingAway, "") + }() + var got []message.Message + if err := sub.Broker().Serve(t.Context(), func(m message.Message) bool { + got = append(got, m) + return true + }); err != nil { + t.Fatalf("Serve: %v", err) + } + if len(got) == 0 { + t.Fatal("Serve's callback never saw the GOAWAY") + } + if _, ok := got[0].(*message.Goaway); !ok { + t.Fatalf("first message = %T, want *message.Goaway", got[0]) + } + if err := cli.Err(); err != nil { + t.Fatalf("one GOAWAY on a request stream closed the session: %v", err) + } +} diff --git a/pkg/relay/request_goaway_test.go b/pkg/relay/request_goaway_test.go new file mode 100644 index 00000000..55baa78e --- /dev/null +++ b/pkg/relay/request_goaway_test.go @@ -0,0 +1,49 @@ +package relay_test + +import ( + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// TestRelay_RequestStreamGoaway: on a subscriber's request stream the relay +// (the server) closes the session for a second GOAWAY or for one carrying a +// New Session URI (§10.4), and tolerates a single plain one. +func TestRelay_RequestStreamGoaway(t *testing.T) { + t.Parallel() + uri := &message.Goaway{NewSessionURI: []byte("https://relay.example/moq")} + for _, tc := range []struct { + name string + sent []*message.Goaway + closes bool + }{ + {"second GOAWAY", []*message.Goaway{{}, {}}, true}, + {"New Session URI from a client", []*message.Goaway{uri}, true}, + {"one GOAWAY", []*message.Goaway{{}}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + pubSess, teardown := connectRelay(t, relay.Config{}) + t.Cleanup(teardown) + publishVideoTrack(t, pubSess, "cam1", 1) + subSess := dialAnotherClient(t, pubSess) + sub := subscribeCam1(t, subSess) + for _, m := range tc.sent { + if err := message.Marshal(sub, m); err != nil { + t.Fatalf("write GOAWAY: %v", err) + } + } + if tc.closes { + requireSessionClosed(t, subSess, tc.name) + return + } + select { + case <-subSess.Done(): + t.Fatalf("relay closed the session on %s: %v", tc.name, subSess.Err()) + case <-time.After(100 * time.Millisecond): + } + }) + } +} diff --git a/pkg/relay/session_handler.go b/pkg/relay/session_handler.go index 5ab66fb7..36daee47 100644 --- a/pkg/relay/session_handler.go +++ b/pkg/relay/session_handler.go @@ -559,6 +559,7 @@ func readRequestStream( // done carries the fin result, so nothing else escapes to the heap. done := make(chan bool, 1) go func() { + var goaways session.RequestGoaways for { m, err := message.Parse(stream) if err != nil { @@ -577,6 +578,12 @@ func readRequestStream( done <- eof return } + // §10.4: a second GOAWAY, or one with a URI from a client, + // closed the session. + if g, ok := m.(*message.Goaway); ok && goaways.Received(sess, g) != nil { + done <- false + return + } if !onMsg(m) { done <- false return From 4031db09dc7605242fe669ef9cc402f466929f9f Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 21:04:10 +0500 Subject: [PATCH 6/9] =?UTF-8?q?fix(session):=20enforce=20and=20expose=20a?= =?UTF-8?q?=20REQUEST=5FERROR=20Redirect=20(=C2=A710.6.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- STATUS.md | 10 +- pkg/moqt/session/broker.go | 18 +- pkg/moqt/session/publish.go | 2 +- pkg/moqt/session/redirect_test.go | 229 ++++++++++++++++++++++++ pkg/moqt/session/request.go | 70 ++++++-- pkg/moqt/session/request_reject_test.go | 6 +- 6 files changed, 311 insertions(+), 24 deletions(-) create mode 100644 pkg/moqt/session/redirect_test.go diff --git a/STATUS.md b/STATUS.md index 4dc6c523..da9af7c1 100644 --- a/STATUS.md +++ b/STATUS.md @@ -190,7 +190,7 @@ By package, bottom-up along the dependency stack: | 10.3.1.7| MAX_REQUEST_UPDATES | 0x08 | DONE | `WithMaxRequestUpdates` advertises the per-stream limit; enforced on inbound follow-ups via `RequestUpdateLimiter`, closing with `TOO_MANY_REQUEST_UPDATES` on overflow. | | 10.4 | GOAWAY | 0x10 | DONE | Same encoding on control and request streams (draft-19 dropped the Request ID field); callback. As recipient the relay initiates no new SUBSCRIBE, FETCH or PUBLISH to the peer and leaves closing the session to the sender. On a request stream (`RequestBroker.Serve` and the relay's readers after the response; `session.RequestGoaways` for callers that read one themselves) a second GOAWAY, or one with a New Session URI received by a server, closes the session with PROTOCOL_VIOLATION; a single one is handed to the reader, and neither side migrates the request. | | 10.5 | REQUEST_OK | 0x07 | DONE | Shared OK for PUBLISH/UPDATE/TRACK_STATUS/namespace reqs. Track Properties where they must be empty close the session on receipt and are refused on send (`ErrTrackPropertiesNotAllowed`). | -| 10.6 | REQUEST_ERROR (+ Redirect) | 0x05 | DONE | Redirect required only when code==REDIRECT. `Request.Reject` sends a Retry Interval; the relay invites a jittered ~1 s retry on EXCESSIVE_LOAD and passes an upstream SUBSCRIBE rejection on by meaning, Retry Interval kept. | +| 10.6 | REQUEST_ERROR (+ Redirect) | 0x05 | DONE | Redirect required only when code==REDIRECT, and exposed as `RequestRejectedError.Redirect`; `Request.Reject` sends one. A Connect URI received by a server, or a Track Name for SUBSCRIBE_NAMESPACE / PUBLISH_NAMESPACE / SUBSCRIBE_TRACKS, closes the session (§10.6.1), and `Reject` refuses to send either; on a REQUEST_UPDATE's answer only the Connect URI is checked, as the reader does not know the request, and an update handler's REDIRECT is sent as INTERNAL_ERROR (§10.6.2 does not list REQUEST_UPDATE). The relay does not follow a Redirect: an upstream REDIRECT becomes INTERNAL_ERROR downstream. `Request.Reject` sends a Retry Interval; the relay invites a jittered ~1 s retry on EXCESSIVE_LOAD and passes an upstream SUBSCRIBE rejection on by meaning, Retry Interval kept. | | 10.7 | SUBSCRIBE | 0x03 | DONE | | | 10.8 | SUBSCRIBE_OK | 0x04 | DONE | Registers inbound track alias. | | 10.9 | REQUEST_UPDATE | 0x02 | DONE | A REQUEST_UPDATE opening a request stream closes the session with PROTOCOL_VIOLATION (`ErrUnexpectedRequestUpdate`). | @@ -518,12 +518,14 @@ already listed as Limitations above are not repeated here. Session layer: +- The relay's `readRequestStream` does not check a REQUEST_ERROR arriving on a + request stream after the response (e.g. on a forwarded PUBLISH after + PUBLISH_OK), so a Connect URI in one does not close the session as + `RequestBroker.Serve` does (§10.6.1). The draft does not define such a + REQUEST_ERROR. - A GOAWAY on a request stream before its initial response, or on a SUBSCRIBE_TRACKS stream read with `ReadPublishSkipped`, is an error rather than a legal message checked against §10.4. -- A REQUEST_ERROR Redirect is dropped after parsing: a server receiving a Connect - URI, or a Track Name on a namespace-scoped request, does not close the session, - and the application cannot follow it (§10.6.1). - A first response other than REQUEST_OK / REQUEST_ERROR to SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS does not close the session (§10.19, §10.20). - `Publication`'s automatic PUBLISH_DONE UPDATE_FAILED is sent while its subgroup diff --git a/pkg/moqt/session/broker.go b/pkg/moqt/session/broker.go index 9f2e38cc..29bdf838 100644 --- a/pkg/moqt/session/broker.go +++ b/pkg/moqt/session/broker.go @@ -94,7 +94,8 @@ func (b *RequestBroker) UpdateScope(s message.ParamScope) { b.updateScope = s } // UpdateHandler decides a peer's REQUEST_UPDATE (§10.9). It returns the // REQUEST_OK to send, or an error: a *[RequestRejectedError] is sent as // REQUEST_ERROR with its code, reason and Retry Interval, any other error as -// INTERNAL_ERROR. +// INTERNAL_ERROR. So is a REDIRECT, which cannot answer a REQUEST_UPDATE +// (§10.6.2). type UpdateHandler func(upd *message.RequestUpdate) (*message.RequestOK, error) // HandleUpdates installs the handler that decides peer REQUEST_UPDATEs, @@ -138,7 +139,7 @@ func (b *RequestBroker) answerUpdate(upd *message.RequestUpdate) (bool, error) { return true, nil } rej, isRej := errors.AsType[*RequestRejectedError](err) - if !isRej { + if !isRej || rej.Code == moqt.RequestRedirect { rej = &RequestRejectedError{Code: moqt.RequestInternalError, Reason: err.Error()} } if werr := b.WriteMessage(&message.RequestError{ @@ -184,7 +185,7 @@ func (s *Session) mapUpdateResponse(msg message.Message) (*message.RequestOK, er } return m, nil case *message.RequestError: - return nil, &RequestRejectedError{Code: m.ErrorCode, Reason: m.ErrorReason, RetryInterval: m.RetryInterval} + return nil, s.rejection(m, message.TypeRequestUpdate) default: return nil, fmt.Errorf("moqt/session: unexpected %s in REQUEST_UPDATE response", msg.Type()) } @@ -461,6 +462,17 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b if err := b.sess.checkRequestOKTrackProperties(nil, m); err != nil { return err } + // §10.6.1, before routing: an unsolicited REQUEST_ERROR reaches + // onMsg instead of an Update. + if e, isErr := m.(*message.RequestError); isErr { + if err := redirectViolation( + e.Redirect, + message.TypeRequestUpdate, + b.sess.role == roleServer, + ); err != nil { + return b.sess.closeProtocolViolation(fmt.Errorf("moqt/session: received %w", err)) + } + } if err := b.sess.CheckPeerParams(message.ScopeRequestUpdateOK, m); err != nil { return err } diff --git a/pkg/moqt/session/publish.go b/pkg/moqt/session/publish.go index 05d05f0d..2c992c47 100644 --- a/pkg/moqt/session/publish.go +++ b/pkg/moqt/session/publish.go @@ -291,7 +291,7 @@ func (s *Session) AwaitPublishOK(ctx context.Context, stream Stream) (*message.R } return m, nil case *message.RequestError: - return nil, &RequestRejectedError{Code: m.ErrorCode, Reason: m.ErrorReason, RetryInterval: m.RetryInterval} + return nil, s.rejection(m, message.TypePublish) default: return nil, fmt.Errorf("moqt/session: unexpected %s in PUBLISH response", resp.Type()) } diff --git a/pkg/moqt/session/redirect_test.go b/pkg/moqt/session/redirect_test.go new file mode 100644 index 00000000..94356d7f --- /dev/null +++ b/pkg/moqt/session/redirect_test.go @@ -0,0 +1,229 @@ +package session_test + +import ( + "errors" + "reflect" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/moqt/wire" +) + +// redirectError is a REDIRECT REQUEST_ERROR carrying rd. +func redirectError(rd message.Redirect) *message.RequestError { + return &message.RequestError{ErrorCode: moqt.RequestRedirect, ErrorReason: "elsewhere", Redirect: &rd} +} + +// answerRaw has sess accept the next request and write resp on its stream as +// is, bypassing the checks [session.Request.Reject] applies. +func answerRaw(t *testing.T, sess *session.Session, resp message.Message) { + t.Helper() + go func() { + r, err := sess.AcceptRequest(t.Context()) + if err != nil { + return + } + _ = message.Marshal(r.Stream, resp) + }() +} + +// TestRedirectFollowable: a REDIRECT's Redirect reaches the requester on its +// RequestRejectedError (§10.6.1), sent with Reject, and a Connect URI to the +// client or a Track Name for a SUBSCRIBE keeps the session open. +func TestRedirectFollowable(t *testing.T) { + cli, srv := openPair(t) + want := &message.Redirect{ + ConnectURI: []byte("https://other.example/moq"), + Namespace: wire.TrackNamespace{[]byte("elsewhere")}, + TrackName: []byte("t2"), + } + rejected := make(chan error, 1) + go func() { + r, err := srv.AcceptRequest(t.Context()) + if err != nil { + rejected <- err + return + } + rejected <- r.Reject(&session.RequestRejectedError{Code: moqt.RequestRedirect, Reason: "moved", Redirect: want}) + }() + _, err := cli.Subscribe(t.Context(), &message.Subscribe{Name: []byte("t")}) + if err := <-rejected; err != nil { + t.Fatalf("Reject: %v", err) + } + rej, ok := errors.AsType[*session.RequestRejectedError](err) + if !ok || rej.Code != moqt.RequestRedirect { + t.Fatalf("Subscribe = %v, want a REDIRECT", err) + } + if !reflect.DeepEqual(rej.Redirect, want) { + t.Fatalf("Redirect = %+v, want %+v", rej.Redirect, want) + } + requireStaysOpen(t, cli, 50*time.Millisecond) +} + +// TestReceivedRedirectViolationCloses: a Redirect with a Connect URI received +// by a server, or with a Track Name for a namespace-scoped request, closes the +// session with PROTOCOL_VIOLATION (§10.6.1), on every path a REQUEST_ERROR is +// read. +func TestReceivedRedirectViolationCloses(t *testing.T) { + uri := message.Redirect{ConnectURI: []byte("https://other.example/moq")} + name := message.Redirect{Namespace: wire.TrackNamespace{[]byte("ns")}, TrackName: []byte("t")} + ns := wire.TrackNamespace{[]byte("ns")} + for _, tc := range []struct { + name string + // toServer: the server is the requester and receives the REDIRECT. + toServer bool + rd message.Redirect + open func(t *testing.T, requester *session.Session) error + }{ + {"Connect URI at the server, SUBSCRIBE", true, uri, func(t *testing.T, s *session.Session) error { + _, err := s.Subscribe(t.Context(), &message.Subscribe{Name: []byte("t")}) + return err + }}, + {"Connect URI at the server, AwaitPublishOK", true, uri, func(t *testing.T, s *session.Session) error { + stream, err := s.OpenPublish(&message.Publish{Name: []byte("t"), TrackAlias: 1}) + if err != nil { + return err + } + _, err = s.AwaitPublishOK(t.Context(), stream) + return err + }}, + {"Track Name for SUBSCRIBE_NAMESPACE", false, name, func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{TrackNamespacePrefix: ns}) + return err + }}, + {"Track Name for PUBLISH_NAMESPACE", false, name, func(t *testing.T, s *session.Session) error { + _, err := s.PublishNamespace(t.Context(), &message.PublishNamespace{Namespace: ns}) + return err + }}, + {"Track Name for SUBSCRIBE_TRACKS", false, name, func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeTracks(t.Context(), &message.SubscribeTracks{TrackNamespacePrefix: ns}) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + requester, responder := cli, srv + if tc.toServer { + requester, responder = srv, cli + } + answerRaw(t, responder, redirectError(tc.rd)) + _ = tc.open(t, requester) + requireClosedCode(t, requester, moqt.SessionProtocolViolation) + }) + } +} + +// TestReceivedRedirectViolationClosesOnUpdate: a Redirect carrying a Connect +// URI on a server's request stream after the response closes the session +// (§10.6.1): answering a REQUEST_UPDATE read by Update itself or routed by the +// broker, or unsolicited, handed to Serve's callback. +func TestReceivedRedirectViolationClosesOnUpdate(t *testing.T) { + for _, tc := range []struct { + name string + served, apply bool + }{ + {"read by Update", false, true}, + {"routed by the broker", true, true}, + {"unsolicited", true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, srv, cli) + go func() { + if tc.apply { + if _, err := message.Parse(pub); err != nil { + return + } + } + _ = message.Marshal( + pub, + redirectError(message.Redirect{ConnectURI: []byte("https://other.example/moq")}), + ) + }() + if tc.served { + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + } + if tc.apply { + _, _ = sub.Update(t.Context(), nil) + } + requireClosedCode(t, srv, moqt.SessionProtocolViolation) + }) + } +} + +// TestRejectRefusesSessionFatalRedirect: Reject does not send a Redirect its +// peer would have to close the session for (§10.6.1), so the request can still +// be refused another way. +func TestRejectRefusesSessionFatalRedirect(t *testing.T) { + ns := wire.TrackNamespace{[]byte("ns")} + for _, tc := range []struct { + name string + toServer bool + rd *message.Redirect + open func(t *testing.T, requester *session.Session) error + }{ + {"Connect URI to the server", true, &message.Redirect{ConnectURI: []byte("https://other.example/moq")}, + func(t *testing.T, s *session.Session) error { + _, err := s.Subscribe(t.Context(), &message.Subscribe{Name: []byte("t")}) + return err + }}, + {"Track Name for SUBSCRIBE_NAMESPACE", false, &message.Redirect{Namespace: ns, TrackName: []byte("t")}, + func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{TrackNamespacePrefix: ns}) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + requester, responder := cli, srv + if tc.toServer { + requester, responder = srv, cli + } + refused := make(chan error, 1) + go func() { + r, err := responder.AcceptRequest(t.Context()) + if err != nil { + refused <- err + return + } + refused <- r.Reject(&session.RequestRejectedError{Code: moqt.RequestRedirect, Redirect: tc.rd}) + _ = r.RejectError(moqt.RequestDoesNotExist, "no") + }() + err := tc.open(t, requester) + if rej, ok := errors.AsType[*session.RequestRejectedError]( + err, + ); !ok || + rej.Code != moqt.RequestDoesNotExist { + t.Fatalf("request = %v, want the DOES_NOT_EXIST sent after the refused REDIRECT", err) + } + if err := <-refused; err == nil { + t.Fatal("Reject sent a Redirect the peer must close the session for") + } + requireStaysOpen(t, requester, 50*time.Millisecond) + }) + } +} + +// TestUpdateHandlerRedirectSentAsInternalError: REDIRECT cannot answer a +// REQUEST_UPDATE (§10.6.2), so an UpdateHandler's REDIRECT goes out as +// INTERNAL_ERROR rather than as a REDIRECT the peer cannot parse, and both +// sessions stay open. +func TestUpdateHandlerRedirectSentAsInternalError(t *testing.T) { + client, server := openPair(t) + sub, pub := subscribePair(t, client, server) + b := pub.Broker() + b.HandleUpdates(func(*message.RequestUpdate) (*message.RequestOK, error) { + return nil, &session.RequestRejectedError{Code: moqt.RequestRedirect, Reason: "moved"} + }) + go func() { _ = b.Serve(t.Context(), nil) }() + + _, err := sub.Update(t.Context(), message.Parameters{message.ForwardParam(false)}) + if rej, ok := errors.AsType[*session.RequestRejectedError](err); !ok || rej.Code != moqt.RequestInternalError { + t.Fatalf("Update = %v, want INTERNAL_ERROR", err) + } + requireStaysOpen(t, client, 50*time.Millisecond) +} diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index b6e80407..df41d68d 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -170,6 +170,44 @@ type RequestRejectedError struct { // RetryInterval is the raw Retry Interval (§10.6.2); see // [RequestRejectedError.RetryAfter]. RetryInterval uint64 + // Redirect is where to retry the request (§10.6.1), present exactly when + // Code is REDIRECT: an empty ConnectURI means this session's URI. + Redirect *message.Redirect +} + +// rejection converts a REQUEST_ERROR answering a message of type req into a +// *RequestRejectedError; for a REQUEST_UPDATE the request it updates is not +// known, so only the Connect URI is checked. A Redirect that §10.6.1 makes +// session-fatal closes the session with PROTOCOL_VIOLATION, and that error is +// returned instead. +func (s *Session) rejection(m *message.RequestError, req message.Type) error { + if err := redirectViolation(m.Redirect, req, s.role == roleServer); err != nil { + return s.closeProtocolViolation(fmt.Errorf("moqt/session: received %w", err)) + } + return &RequestRejectedError{ + Code: m.ErrorCode, + Reason: m.ErrorReason, + RetryInterval: m.RetryInterval, + Redirect: m.Redirect, + } +} + +// redirectViolation reports a Redirect for a request of type req that the +// receiver, a server when atServer, MUST close the session for (§10.6.1): "If +// a server receives a Redirect with a non-zero Connect URI Length", or a +// non-empty Track Name "in a Redirect for a namespace-scoped request". +func redirectViolation(rd *message.Redirect, req message.Type, atServer bool) error { + switch { + case rd == nil: + return nil + case atServer && len(rd.ConnectURI) > 0: + return errors.New("a Redirect with a Connect URI at a server (§10.6.1)") + case len(rd.TrackName) > 0 && + (req == message.TypeSubscribeNamespace || req == message.TypePublishNamespace || + req == message.TypeSubscribeTracks): + return fmt.Errorf("a Redirect with a Track Name for %s (§10.6.1)", req) + } + return nil } // RetryAfter decodes RetryInterval (§10.6.2): whether the request may be @@ -695,11 +733,7 @@ func awaitRequestResponse[OK message.Message, R any]( } _ = stream.Close() if rerr, isErr := resp.(*message.RequestError); isErr { - return zero, &RequestRejectedError{ - Code: rerr.ErrorCode, - Reason: rerr.ErrorReason, - RetryInterval: rerr.RetryInterval, - } + return zero, s.rejection(rerr, m.Type()) } return zero, fmt.Errorf("moqt/session: unexpected %s in %s response", resp.Type(), m.Type()) } @@ -823,18 +857,28 @@ func (r *Request) RejectError(code moqt.RequestErrorCode, reason string) error { return r.Reject(&RequestRejectedError{Code: code, Reason: reason}) } -// Reject is [Request.RejectError] with rej's Code, Reason and RetryInterval -// (§10.6.2). REDIRECT is refused and nothing is written, since Reject has no -// Redirect structure to send. +// Reject is [Request.RejectError] with rej's Code, Reason, RetryInterval and, +// for REDIRECT, Redirect (§10.6.2). Nothing is written, and an error returned, +// when the Redirect does not match the code, or is one the peer MUST close the +// session for (§10.6.1): a Connect URI sent to a server, or a Track Name for a +// namespace-scoped request. func (r *Request) Reject(rej *RequestRejectedError) error { - if rej.Code == moqt.RequestRedirect { - return errors.New("moqt/session: Reject cannot send REDIRECT: it has no Redirect structure (§10.6.2)") - } - if err := message.Marshal(r.Stream, &message.RequestError{ + m := &message.RequestError{ ErrorCode: rej.Code, RetryInterval: rej.RetryInterval, ErrorReason: rej.Reason, - }); err != nil { + Redirect: rej.Redirect, + } + if err := m.ValidateRedirect(); err != nil { + return fmt.Errorf("moqt/session: Reject: %w", err) + } + // Refuse what the peer would have to close the session for. + if rej.Redirect != nil { + if err := redirectViolation(rej.Redirect, r.First.Type(), r.s.role == roleClient); err != nil { + return fmt.Errorf("moqt/session: Reject: %w", err) + } + } + if err := message.Marshal(r.Stream, m); err != nil { resetStream(r.Stream) return err } diff --git a/pkg/moqt/session/request_reject_test.go b/pkg/moqt/session/request_reject_test.go index b0e0283e..a96ec9d9 100644 --- a/pkg/moqt/session/request_reject_test.go +++ b/pkg/moqt/session/request_reject_test.go @@ -100,9 +100,9 @@ func TestRejectSendsRetryInterval(t *testing.T) { } } -// TestRejectRefusesRedirect: a REDIRECT needs a Redirect structure (§10.6.2) -// that RequestRejectedError cannot carry, so Reject refuses without writing -// and the request can still be refused another way. +// TestRejectRefusesRedirect: a REDIRECT needs a Redirect structure (§10.6.2), +// so Reject refuses one without it and writes nothing, and the request can +// still be refused another way. func TestRejectRefusesRedirect(t *testing.T) { client, server := openPair(t) refused := make(chan error, 1) From 1cc405751cfc46e2ade1d3a35bc2d66ee0a9af49 Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 21:08:31 +0500 Subject: [PATCH 7/9] =?UTF-8?q?fix(session):=20close=20on=20a=20wrong=20fi?= =?UTF-8?q?rst=20response=20to=20SUBSCRIBE=5FNAMESPACE=20or=20SUBSCRIBE=5F?= =?UTF-8?q?TRACKS=20(=C2=A710.19,=20=C2=A710.20)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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) --- STATUS.md | 14 ++--- pkg/moqt/session/first_response_test.go | 80 +++++++++++++++++++++++++ pkg/moqt/session/namespace.go | 6 +- pkg/moqt/session/request.go | 14 ++++- 4 files changed, 102 insertions(+), 12 deletions(-) create mode 100644 pkg/moqt/session/first_response_test.go diff --git a/STATUS.md b/STATUS.md index da9af7c1..0ed11c4b 100644 --- a/STATUS.md +++ b/STATUS.md @@ -203,8 +203,8 @@ By package, bottom-up along the dependency stack: | 10.16 | PUBLISH_NAMESPACE | 0x06 | DONE | | | 10.17 | NAMESPACE | 0x08 | DONE | Per namespace, counted over local and remote sources. | | 10.18 | NAMESPACE_DONE | 0x0E | DONE | Never before its NAMESPACE. | -| 10.19 | SUBSCRIBE_NAMESPACE | 0x50 | DONE | | -| 10.20 | SUBSCRIBE_TRACKS | 0x51 | DONE | §10.20.1: its SUBSCRIBE parameters become each forwarded PUBLISH's subscription; an out-of-range value closes the session (§10.2.8/§10.2.18). A REQUEST_UPDATE merges into them for later PUBLISHes (§10.2.18: "Existing subscriptions are unaffected"), and existing tracks that newly match by prefix or Range Filter are forwarded then. | +| 10.19 | SUBSCRIBE_NAMESPACE | 0x50 | DONE | A first response other than REQUEST_OK / REQUEST_ERROR, a GOAWAY included, closes the session with PROTOCOL_VIOLATION. | +| 10.20 | SUBSCRIBE_TRACKS | 0x51 | DONE | A first response other than REQUEST_OK / REQUEST_ERROR, a GOAWAY included, closes the session with PROTOCOL_VIOLATION. §10.20.1: its SUBSCRIBE parameters become each forwarded PUBLISH's subscription; an out-of-range value closes the session (§10.2.8/§10.2.18). A REQUEST_UPDATE merges into them for later PUBLISHes (§10.2.18: "Existing subscriptions are unaffected"), and existing tracks that newly match by prefix or Range Filter are forwarded then. | | 10.21 | PUBLISH_SKIPPED | 0x0F | DONE | Prohibition scoped to a single PUBLISH (§6.1) — not sticky across re-PUBLISHes. | ## §11 Data streams and datagrams @@ -523,11 +523,11 @@ Session layer: PUBLISH_OK), so a Connect URI in one does not close the session as `RequestBroker.Serve` does (§10.6.1). The draft does not define such a REQUEST_ERROR. -- A GOAWAY on a request stream before its initial response, or on a - SUBSCRIBE_TRACKS stream read with `ReadPublishSkipped`, is an error rather - than a legal message checked against §10.4. -- A first response other than REQUEST_OK / REQUEST_ERROR to SUBSCRIBE_NAMESPACE or - SUBSCRIBE_TRACKS does not close the session (§10.19, §10.20). +- A GOAWAY on a request stream before its initial response (other than to + SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS, where §10.19/§10.20 make it a + PROTOCOL_VIOLATION), or on a SUBSCRIBE_TRACKS stream read with + `ReadPublishSkipped`, is an error rather than a legal message checked + against §10.4. - `Publication`'s automatic PUBLISH_DONE UPDATE_FAILED is sent while its subgroup streams are open, `WriteObject` still succeeds after `Done`, and a subgroup opened concurrently with `Done` is missing from the Stream Count (§10.12). diff --git a/pkg/moqt/session/first_response_test.go b/pkg/moqt/session/first_response_test.go new file mode 100644 index 00000000..4ef41ab1 --- /dev/null +++ b/pkg/moqt/session/first_response_test.go @@ -0,0 +1,80 @@ +package session_test + +import ( + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/moqt/wire" +) + +// TestNamespaceScopedFirstResponseCloses: a first response other than +// REQUEST_OK or REQUEST_ERROR to SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS closes +// the session with PROTOCOL_VIOLATION (§10.19, §10.20), even one that is legal +// later on the stream. +func TestNamespaceScopedFirstResponseCloses(t *testing.T) { + prefix := wire.TrackNamespace{[]byte("ns")} + requests := []struct { + name string + open func(t *testing.T, s *session.Session) error + }{ + {"SUBSCRIBE_NAMESPACE", func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{TrackNamespacePrefix: prefix}) + return err + }}, + {"SUBSCRIBE_TRACKS", func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeTracks(t.Context(), &message.SubscribeTracks{TrackNamespacePrefix: prefix}) + return err + }}, + } + responses := []struct { + name string + msg message.Message + }{ + {"NAMESPACE", &message.Namespace{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a")}}}, + {"GOAWAY", &message.Goaway{}}, + {"SUBSCRIBE_OK", &message.SubscribeOK{TrackAlias: 1}}, + } + for _, req := range requests { + for _, resp := range responses { + t.Run(req.name+" answered with "+resp.name, func(t *testing.T) { + cli, srv := openPair(t) + answerRaw(t, srv, resp.msg) + if err := req.open(t, cli); err == nil { + t.Fatalf("%s succeeded on a %s", req.name, resp.name) + } + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + }) + } + } +} + +// TestOtherFirstResponseKeepsSession: no MUST covers the first response to +// other requests (§5.1, §6.2 put theirs on the sender), so an unexpected one +// fails the request and leaves the session open. +func TestOtherFirstResponseKeepsSession(t *testing.T) { + for _, tc := range []struct { + name string + open func(t *testing.T, s *session.Session) error + }{ + {"SUBSCRIBE", func(t *testing.T, s *session.Session) error { + _, err := s.Subscribe(t.Context(), &message.Subscribe{Name: []byte("t")}) + return err + }}, + {"PUBLISH_NAMESPACE", func(t *testing.T, s *session.Session) error { + _, err := s.PublishNamespace(t.Context(), &message.PublishNamespace{Namespace: wire.TrackNamespace{[]byte("ns")}}) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + answerRaw(t, srv, &message.Namespace{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a")}}) + if err := tc.open(t, cli); err == nil { + t.Fatalf("%s succeeded on a NAMESPACE", tc.name) + } + requireStaysOpen(t, cli, 50*time.Millisecond) + }) + } +} diff --git a/pkg/moqt/session/namespace.go b/pkg/moqt/session/namespace.go index 710e4e6c..d11a4f96 100644 --- a/pkg/moqt/session/namespace.go +++ b/pkg/moqt/session/namespace.go @@ -94,7 +94,8 @@ func (s *Session) PublishNamespace( // // On success a [NamespaceSubscription] is returned whose embedded stream stays // open (the caller will receive NAMESPACE / NAMESPACE_DONE messages on it). On -// REQUEST_ERROR the stream is closed and a *RequestRejectedError is returned. +// REQUEST_ERROR the stream is closed and a *RequestRejectedError is returned; +// any other first response closes the session with PROTOCOL_VIOLATION (§10.19). func (s *Session) SubscribeNamespace( ctx context.Context, m *message.SubscribeNamespace, @@ -111,7 +112,8 @@ func (s *Session) SubscribeNamespace( // // On success a [TrackSubscription] is returned whose embedded stream stays open // for PUBLISH_SKIPPED follow-ups (read via [TrackSubscription.ReadPublishSkipped]). -// On REQUEST_ERROR the stream is closed and a *RequestRejectedError is returned. +// On REQUEST_ERROR the stream is closed and a *RequestRejectedError is returned; +// any other first response closes the session with PROTOCOL_VIOLATION (§10.20). func (s *Session) SubscribeTracks(ctx context.Context, m *message.SubscribeTracks) (*TrackSubscription, error) { return awaitRequestResponse(ctx, s, m, func(stream Stream, ok *message.RequestOK) (*TrackSubscription, error) { diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index df41d68d..49fcd332 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -697,8 +697,9 @@ func (s *Session) readResponse(ctx context.Context, stream Stream) (message.Mess // awaitRequestResponse opens a request stream for m and awaits the initial // response. An OK is handed to onOK, which then owns the stream; REQUEST_ERROR -// (§10.6) becomes a *RequestRejectedError; anything else is an error. On -// either failure the stream is closed. +// (§10.6) becomes a *RequestRejectedError; anything else is an error, and for +// SUBSCRIBE_NAMESPACE and SUBSCRIBE_TRACKS also closes the session (§10.19, +// §10.20). On either failure the stream is closed. func awaitRequestResponse[OK message.Message, R any]( ctx context.Context, s *Session, @@ -735,7 +736,14 @@ func awaitRequestResponse[OK message.Message, R any]( if rerr, isErr := resp.(*message.RequestError); isErr { return zero, s.rejection(rerr, m.Type()) } - return zero, fmt.Errorf("moqt/session: unexpected %s in %s response", resp.Type(), m.Type()) + err = fmt.Errorf("moqt/session: unexpected %s in %s response", resp.Type(), m.Type()) + // §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." + if t := m.Type(); t == message.TypeSubscribeNamespace || t == message.TypeSubscribeTracks { + return zero, s.closeProtocolViolation(err) + } + return zero, err } // UpdateRequest sends a REQUEST_UPDATE (§10.9) with a fresh Request ID (§10.1) From 8771ff0f7b0bb0b449867701b6e152bf0ff32132 Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 21:21:56 +0500 Subject: [PATCH 8/9] =?UTF-8?q?fix(session,relay):=20close=20the=20session?= =?UTF-8?q?=20on=20a=20second=20SUBSCRIBE=5FOK=20or=20PUBLISH=5FOK=20(?= =?UTF-8?q?=C2=A75.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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) --- STATUS.md | 13 ++- pkg/moqt/session/broker.go | 48 +++++++-- pkg/moqt/session/publish.go | 4 +- pkg/moqt/session/request.go | 5 + pkg/moqt/session/second_response_test.go | 125 +++++++++++++++++++++++ pkg/moqt/session/subscribe.go | 1 + pkg/relay/handler_forward.go | 2 +- pkg/relay/handler_subscribe.go | 17 ++- pkg/relay/second_response_test.go | 42 ++++++++ 9 files changed, 240 insertions(+), 17 deletions(-) create mode 100644 pkg/moqt/session/second_response_test.go create mode 100644 pkg/relay/second_response_test.go diff --git a/STATUS.md b/STATUS.md index 0ed11c4b..04e8af8b 100644 --- a/STATUS.md +++ b/STATUS.md @@ -107,7 +107,7 @@ By package, bottom-up along the dependency stack: | § | Feature | Status | Notes | |---------|----------------------------------|--------|-------| -| 5.1 | Subscriptions | DONE | Subscribe/Publish/OK/Error state machine in `pubsub.go`. | +| 5.1 | Subscriptions | DONE | Subscribe/Publish/OK/Error state machine in `subscribe.go` and `publish.go`. A second response to this side's SUBSCRIBE or PUBLISH closes the session with PROTOCOL_VIOLATION: on a `Subscription`'s or `Session.Publish` `Publication`'s broker, a SUBSCRIBE_OK, or a REQUEST_OK / REQUEST_ERROR before any Update (after one it may answer an Update that gave up); in the relay, a REQUEST_OK / REQUEST_ERROR on a forwarded PUBLISH. | | 5.1.1 | Subscription state management | DONE | REQUEST_ERROR / STOP_SENDING / PUBLISH_DONE handling + cleanup. The relay resets a cancelled subscription's open subgroup and fill streams. | | 5.1.2 | Location filters | DONE | Every start/end form (unfiltered, Next Object, relative and absolute start, absolute range) + `Matches`. | | 5.1.3 | Fill semantics | PARTIAL | Fill fetch streams from FILL_PARAMETERS on SUBSCRIBE / REQUEST_UPDATE (`handler_fill.go`), and on SUBSCRIBE_TRACKS, one per forwarded PUBLISH's subscription, keyed to the PUBLISH's Request ID (§10.1). A fill inherits the subscription's Range Filters; the ones inside FILL_PARAMETERS override per type. A cancelled subscription's open fills are reset (§5.1.3.1). Not done: scheduling fills against their subscription (§7.2, see Limitations). | @@ -518,11 +518,11 @@ already listed as Limitations above are not repeated here. Session layer: -- The relay's `readRequestStream` does not check a REQUEST_ERROR arriving on a - request stream after the response (e.g. on a forwarded PUBLISH after - PUBLISH_OK), so a Connect URI in one does not close the session as - `RequestBroker.Serve` does (§10.6.1). The draft does not define such a - REQUEST_ERROR. +- On a request stream the relay answered, a REQUEST_OK or REQUEST_ERROR from + the requester is ignored rather than closing the session, so a Connect URI in + one is not checked (§10.6.1). The draft defines no such message; only on a + forwarded PUBLISH, where it is a second response, does the relay close the + session (§5.1). - A GOAWAY on a request stream before its initial response (other than to SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS, where §10.19/§10.20 make it a PROTOCOL_VIOLATION), or on a SUBSCRIBE_TRACKS stream read with @@ -537,7 +537,6 @@ Session layer: - `ReadPublishSkipped` does not close the session on a REQUEST_UPDATE or PUBLISH_STATE_NOTIFY from the publisher (§10.9, §10.10), and nothing enforces NAMESPACE_DONE-before-NAMESPACE on a namespace subscription (§10.19). -- A second SUBSCRIBE_OK is handed to the application (§5.1 SHOULD close). - A rejected request sends STOP_SENDING with INTERNAL_ERROR (§3.3.4 SHOULD use a relevant code). - Mandatory Track Property enforcement is off unless configured (§2.5.1). diff --git a/pkg/moqt/session/broker.go b/pkg/moqt/session/broker.go index 29bdf838..ae710d89 100644 --- a/pkg/moqt/session/broker.go +++ b/pkg/moqt/session/broker.go @@ -30,10 +30,14 @@ import ( // [RequestBroker.HandleUpdates], or declined with NOT_SUPPORTED when there // is none, since acknowledging an unapplied update would misstate the // request's state. +// - On the broker of a [Subscription], or of a [Publication] from +// [Session.Publish], a REQUEST_OK / REQUEST_ERROR before any Update, or a +// SUBSCRIBE_OK on a Subscription, is a second response to the request and +// closes the session (§5.1). // - A second GOAWAY on the stream, or one with a New Session URI received // by a server, closes the session (§10.4; see [RequestGoaways]). -// - Everything else (PUBLISH_DONE, unsolicited responses, …) is handed to -// Serve's callback. +// - Everything else (PUBLISH_DONE, other unsolicited responses, …) is +// handed to Serve's callback. // // Obtain one from a typed request handle's Broker method (e.g. // [Publication.Broker]) or [Session.NewRequestBroker]; from then on every @@ -56,6 +60,9 @@ type RequestBroker struct { // e.g. a PUBLISH_DONE after the peer tore its side down. updatesClosed bool streamClosed bool + // updated records that Update sent a REQUEST_UPDATE, the only thing a + // REQUEST_OK or REQUEST_ERROR read here can answer. + updated bool // onUpdate decides each peer REQUEST_UPDATE; nil declines it. // onUpdateFailed runs after a declined update (§10.9.1). Both are set @@ -152,6 +159,15 @@ func (b *RequestBroker) answerUpdate(upd *message.RequestUpdate) (bool, error) { return false, nil } +// answered is the request type (SUBSCRIBE or PUBLISH) of a broker on this +// side's request, whose response the peer has sent; zero otherwise. +func (b *RequestBroker) answered() message.Type { + if b.handle == nil { + return 0 + } + return b.handle.answered +} + // updateResult carries one §10.9 response to a waiting Update call. type updateResult struct { ok *message.RequestOK @@ -226,6 +242,7 @@ func (b *RequestBroker) Update(ctx context.Context, params message.Parameters) ( }) if err == nil { b.waiters = append(b.waiters, ch) + b.updated = true } b.mu.Unlock() if err != nil { @@ -391,9 +408,10 @@ func (b *RequestBroker) receiveUpdate(m *message.RequestUpdate, updates *Request // // Responses route to Update waiters; a token cache fault closes the session // (§10.2.2); peer REQUEST_UPDATEs are answered as described on -// [RequestBroker]. Every other message, including each REQUEST_UPDATE and any -// unsolicited response, is passed to onMsg (nil means "discard"); return false -// from onMsg to stop serving. +// [RequestBroker], and a second response to this side's SUBSCRIBE or PUBLISH +// closes the session (§5.1). Every other message, including each +// REQUEST_UPDATE and any other unsolicited response, is passed to onMsg (nil +// means "discard"); return false from onMsg to stop serving. // // A read error resets the read side with INTERNAL_ERROR; a malformed follow-up // also closes the session with PROTOCOL_VIOLATION (§10). Serve returns nil on @@ -455,10 +473,28 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b } switch m := msg.(type) { + case *message.SubscribeOK: + // §5.1: the response to this side's SUBSCRIBE was read already, + // and "The peer SHOULD close the session with a protocol error if + // it receives more than one." + if b.answered() == message.TypeSubscribe { + return b.sess.closeProtocolViolation(errors.New("moqt/session: SUBSCRIBE_OK after the response")) + } case *message.RequestOK, *message.RequestError: // The request's own response was read before the broker // attached, so every REQUEST_OK here is a REQUEST_UPDATE_OK - // (§10.5), even one whose Update gave up. + // (§10.5), even one whose Update gave up. Before any Update, on + // this side's SUBSCRIBE or PUBLISH, it is a second response to + // the request, a second PUBLISH_OK among them (§5.1). + if b.answered() != 0 { + b.mu.Lock() + updated := b.updated + b.mu.Unlock() + if !updated { + return b.sess.closeProtocolViolation( + fmt.Errorf("moqt/session: %s after the response, before any REQUEST_UPDATE", m.Type())) + } + } if err := b.sess.checkRequestOKTrackProperties(nil, m); err != nil { return err } diff --git a/pkg/moqt/session/publish.go b/pkg/moqt/session/publish.go index 2c992c47..fe464ee4 100644 --- a/pkg/moqt/session/publish.go +++ b/pkg/moqt/session/publish.go @@ -250,7 +250,9 @@ func (s *Session) Publish(ctx context.Context, m *message.Publish) (*Publication func(stream Stream, _ *message.RequestOK) (*Publication, error) { // The PUBLISH sets the initial Forward State (§5.1); PUBLISH_OK // carries no subscription parameters. - return newPublication(s, stream, m.RequestID, m.TrackAlias, m.Parameters), nil + p := newPublication(s, stream, m.RequestID, m.TrackAlias, m.Parameters) + p.answered = message.TypePublish + return p, nil }) } diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index 49fcd332..d9872486 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -532,6 +532,11 @@ type requestHandle struct { released atomic.Bool // peerDone records that the publisher's FIN was read. peerDone atomic.Bool + + // answered is TypeSubscribe or TypePublish for a request this side sent, + // whose response was read: its broker closes the session on another + // (§5.1). Zero otherwise. + answered message.Type } // Read reads the request stream. On the receiving side of a subscription diff --git a/pkg/moqt/session/second_response_test.go b/pkg/moqt/session/second_response_test.go new file mode 100644 index 00000000..c040008e --- /dev/null +++ b/pkg/moqt/session/second_response_test.go @@ -0,0 +1,125 @@ +package session_test + +import ( + "context" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" +) + +// TestSecondResponseCloses: a second response to a SUBSCRIBE or PUBLISH read +// by the requester's broker closes the session with PROTOCOL_VIOLATION (§5.1: +// "The peer SHOULD close the session with a protocol error if it receives more +// than one"), as does a REQUEST_ERROR where no REQUEST_UPDATE was sent. +func TestSecondResponseCloses(t *testing.T) { + for _, tc := range []struct { + name string + resp message.Message + // viaPublish: the server PUBLISHes and the client answers twice; + // else the client SUBSCRIBEs and the server answers twice. + viaPublish bool + }{ + {"second SUBSCRIBE_OK", &message.SubscribeOK{TrackAlias: 1}, false}, + {"REQUEST_ERROR after SUBSCRIBE_OK", &message.RequestError{ErrorCode: moqt.RequestInternalError}, false}, + {"second PUBLISH_OK", &message.RequestOK{}, true}, + {"REQUEST_ERROR after PUBLISH_OK", &message.RequestError{ErrorCode: moqt.RequestInternalError}, true}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + requester := cli + var ( + from session.Stream + broker *session.RequestBroker + ) + if tc.viaPublish { + requester = srv + h, pub := establishOnAlias(t, cli, srv, true, "t", 7) + from, broker = h.(*session.IncomingPublication), pub.Broker() + } else { + sub, pub := subscribePair(t, cli, srv) + from, broker = pub, sub.Broker() + } + go func() { _ = message.Marshal(from, tc.resp) }() + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _ = broker.Serve(ctx, nil) + requireClosedCode(t, requester, moqt.SessionProtocolViolation) + }) + } +} + +// TestResponderStreamResponseKeepsSession: on a stream this side answered, +// the peer is the requester, so a REQUEST_OK or REQUEST_ERROR from it is no +// second response (§5.1) and reaches Serve's callback. +func TestResponderStreamResponseKeepsSession(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, cli, srv) + go func() { _ = message.Marshal(sub, &message.RequestError{ErrorCode: moqt.RequestInternalError}) }() + got := make(chan message.Message, 1) + go func() { + _ = pub.Broker().Serve(t.Context(), func(m message.Message) bool { + got <- m + return false + }) + }() + select { + case m := <-got: + if _, ok := m.(*message.RequestError); !ok { + t.Fatalf("Serve's callback got %T, want *message.RequestError", m) + } + case <-time.After(2 * time.Second): + t.Fatal("the REQUEST_ERROR never reached Serve's callback") + } + requireStaysOpen(t, srv, 50*time.Millisecond) +} + +// TestSubscribeOKOnPublishKeepsSession: a SUBSCRIBE_OK is no response to a +// PUBLISH (§5.1), so on this side's PUBLISH it reaches Serve's callback. +func TestSubscribeOKOnPublishKeepsSession(t *testing.T) { + cli, srv := openPair(t) + h, pub := establishOnAlias(t, cli, srv, true, "t", 7) + go func() { _ = message.Marshal(h.(*session.IncomingPublication), &message.SubscribeOK{TrackAlias: 1}) }() + got := make(chan message.Message, 1) + go func() { + _ = pub.Broker().Serve(t.Context(), func(m message.Message) bool { + got <- m + return false + }) + }() + select { + case m := <-got: + if _, ok := m.(*message.SubscribeOK); !ok { + t.Fatalf("Serve's callback got %T, want *message.SubscribeOK", m) + } + case <-time.After(2 * time.Second): + t.Fatal("the SUBSCRIBE_OK never reached Serve's callback") + } + requireStaysOpen(t, srv, 50*time.Millisecond) +} + +// TestLateUpdateAnswerKeepsSession: the answer to a REQUEST_UPDATE whose +// Update gave up still answers a REQUEST_UPDATE that was sent (§10.9), so it +// does not read as a second response. +func TestLateUpdateAnswerKeepsSession(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, cli, srv) + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + answered := make(chan struct{}) + go func() { + defer close(answered) + if _, err := message.Parse(pub); err != nil { + return + } + _ = message.Marshal(pub, &message.RequestOK{}) + }() + _, _ = b.Update(ctx, nil) + <-answered + requireStaysOpen(t, cli, 100*time.Millisecond) +} diff --git a/pkg/moqt/session/subscribe.go b/pkg/moqt/session/subscribe.go index 87387112..bc7ee607 100644 --- a/pkg/moqt/session/subscribe.go +++ b/pkg/moqt/session/subscribe.go @@ -67,6 +67,7 @@ func (s *Session) Subscribe(ctx context.Context, m *message.Subscribe) (*Subscri peerNotify: true, inboundAlias: ok.TrackAlias, holdsAlias: true, + answered: message.TypeSubscribe, OK: ok, }, nil }) diff --git a/pkg/relay/handler_forward.go b/pkg/relay/handler_forward.go index 7abb9e4c..d015059f 100644 --- a/pkg/relay/handler_forward.go +++ b/pkg/relay/handler_forward.go @@ -127,5 +127,5 @@ func (h *sessionHandler) serveForwardedPublish( if p, ok := params.Find(message.ParamNewGroupRequest); ok { h.propagateNewGroupUpstream(ctx, fullName, p.Varint) } - h.readSubscribeUpdates(ctx, stream, sub, fullName) + h.readSubscribeUpdates(ctx, stream, sub, fullName, true) } diff --git a/pkg/relay/handler_subscribe.go b/pkg/relay/handler_subscribe.go index 040b51ad..bb9560ec 100644 --- a/pkg/relay/handler_subscribe.go +++ b/pkg/relay/handler_subscribe.go @@ -176,7 +176,7 @@ func (h *sessionHandler) handleSubscribe(ctx context.Context, req *session.Reque h.propagateForwardUpstream(ctx, fullName) } - h.readSubscribeUpdates(ctx, req.Stream, sub, fullName) + h.readSubscribeUpdates(ctx, req.Stream, sub, fullName, false) h.log.LogAttrs(ctx, slog.LevelDebug, "SUBSCRIBE stream ended", slog.String("name", string(msg.Name))) } @@ -185,18 +185,31 @@ func (h *sessionHandler) handleSubscribe(ctx context.Context, req *session.Reque // SUBSCRIBE's stream to [sessionHandler.handleSubscribeUpdate] until the // subscriber cancels, the stream turns undecodable (see [readRequestStream]) // or ctx ends. A subscriber FIN is not a cancellation (§3.3.2): the -// subscription lives on in [awaitRequestEnd]. +// subscription lives on in [awaitRequestEnd]. forwarded marks a PUBLISH the +// relay forwarded, which the subscriber answered. func (h *sessionHandler) readSubscribeUpdates( ctx context.Context, stream session.Stream, sub *registry.DownstreamSub, fullName track.FullTrackName, + forwarded bool, ) { updates := h.sess.NewRequestUpdateLimiter() fin := readRequestStream(ctx, h.sess, stream, func(m message.Message) bool { if h.isPeerStateNotify(m) { return false } + switch m.(type) { + case *message.RequestOK, *message.RequestError: + // On a forwarded PUBLISH the subscriber answered already, and + // the relay sends no REQUEST_UPDATE here: a second PUBLISH_OK + // or REQUEST_ERROR (§5.1). + if forwarded { + _ = h.sess.Close(moqt.SessionProtocolViolation, + fmt.Sprintf("%s after the PUBLISH_OK", m.Type())) + return false + } + } if upd, ok := m.(*message.RequestUpdate); ok { // §10.2.1: parameters outside the scope of a subscriber's // update are session-fatal. diff --git a/pkg/relay/second_response_test.go b/pkg/relay/second_response_test.go new file mode 100644 index 00000000..da2914b0 --- /dev/null +++ b/pkg/relay/second_response_test.go @@ -0,0 +1,42 @@ +package relay_test + +import ( + "context" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// TestRelay_SecondResponseCloses: the relay sends no REQUEST_UPDATE on a +// forwarded PUBLISH, so a REQUEST_OK or REQUEST_ERROR the subscriber sends +// after its PUBLISH_OK is a second response, and the relay closes the session +// (§5.1). +func TestRelay_SecondResponseCloses(t *testing.T) { + t.Parallel() + for _, resp := range []message.Message{&message.RequestOK{}, &message.RequestError{}} { + t.Run(resp.Type().String()+" after PUBLISH_OK on a forwarded PUBLISH", func(t *testing.T) { + t.Parallel() + subSess, teardown := connectRelay(t, relay.Config{}) + t.Cleanup(teardown) + subscribeTracks(t, subSess, ns("video")) + publish(t, dialAnotherClient(t, subSess), &message.Publish{ + Namespace: ns("video", "cam7"), Name: []byte("rtp"), TrackAlias: 99, + }) + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + req, err := subSess.AcceptRequest(ctx) + if err != nil { + t.Fatalf("AcceptRequest: %v", err) + } + if _, err := req.AcceptPublish(); err != nil { + t.Fatalf("AcceptPublish: %v", err) + } + if err := message.Marshal(req.Stream, resp); err != nil { + t.Fatalf("write second response: %v", err) + } + requireSessionClosed(t, subSess, "a second response to a forwarded PUBLISH") + }) + } +} From ba7f31cc837a4f914a03f3b78deae7ccfc284bdf Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 21:48:33 +0500 Subject: [PATCH 9/9] =?UTF-8?q?fix(session):=20enforce=20namespace-subscri?= =?UTF-8?q?ption=20follow-ups=20through=20typed=20brokers=20(=C2=A710.19,?= =?UTF-8?q?=20=C2=A710.9,=20=C2=A710.10)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- STATUS.md | 10 +- pkg/moqt/session/broker.go | 147 +++++++-- pkg/moqt/session/example_test.go | 11 +- pkg/moqt/session/namespace.go | 111 ++++--- pkg/moqt/session/namespace_followup_test.go | 339 ++++++++++++++++++++ pkg/moqt/session/request.go | 12 + 6 files changed, 551 insertions(+), 79 deletions(-) create mode 100644 pkg/moqt/session/namespace_followup_test.go diff --git a/STATUS.md b/STATUS.md index 04e8af8b..d12262f3 100644 --- a/STATUS.md +++ b/STATUS.md @@ -203,9 +203,9 @@ By package, bottom-up along the dependency stack: | 10.16 | PUBLISH_NAMESPACE | 0x06 | DONE | | | 10.17 | NAMESPACE | 0x08 | DONE | Per namespace, counted over local and remote sources. | | 10.18 | NAMESPACE_DONE | 0x0E | DONE | Never before its NAMESPACE. | -| 10.19 | SUBSCRIBE_NAMESPACE | 0x50 | DONE | A first response other than REQUEST_OK / REQUEST_ERROR, a GOAWAY included, closes the session with PROTOCOL_VIOLATION. | +| 10.19 | SUBSCRIBE_NAMESPACE | 0x50 | DONE | A first response other than REQUEST_OK / REQUEST_ERROR, a GOAWAY included, closes the session with PROTOCOL_VIOLATION. `NamespaceSubscription.Broker` closes it on a NAMESPACE_DONE for a suffix no NAMESPACE announced, and on a PUBLISH_STATE_NOTIFY or REQUEST_UPDATE from the publisher (§10.9, §10.10); `NamespaceSubscription.Update` updates the subscription through the broker. The NAMESPACE_DONE check resolves suffixes against the prefix in force, switching at the REQUEST_OK that accepts a TRACK_NAMESPACE_PREFIX update (§10.9.2); once any Update on the subscription gives up, responses no longer pair reliably, so the prefix is unknown and the check stops. A caller reading the stream with `message.Parse` gets none of these checks. | | 10.20 | SUBSCRIBE_TRACKS | 0x51 | DONE | A first response other than REQUEST_OK / REQUEST_ERROR, a GOAWAY included, closes the session with PROTOCOL_VIOLATION. §10.20.1: its SUBSCRIBE parameters become each forwarded PUBLISH's subscription; an out-of-range value closes the session (§10.2.8/§10.2.18). A REQUEST_UPDATE merges into them for later PUBLISHes (§10.2.18: "Existing subscriptions are unaffected"), and existing tracks that newly match by prefix or Range Filter are forwarded then. | -| 10.21 | PUBLISH_SKIPPED | 0x0F | DONE | Prohibition scoped to a single PUBLISH (§6.1) — not sticky across re-PUBLISHes. | +| 10.21 | PUBLISH_SKIPPED | 0x0F | DONE | Prohibition scoped to a single PUBLISH (§6.1) — not sticky across re-PUBLISHes. `TrackSubscription.ReadPublishSkipped` and its broker close the session on a PUBLISH_STATE_NOTIFY or REQUEST_UPDATE from the publisher (§10.9, §10.10); `ReadPublishSkipped` skips a single GOAWAY (§10.4). | ## §11 Data streams and datagrams @@ -525,8 +525,7 @@ Session layer: session (§5.1). - A GOAWAY on a request stream before its initial response (other than to SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS, where §10.19/§10.20 make it a - PROTOCOL_VIOLATION), or on a SUBSCRIBE_TRACKS stream read with - `ReadPublishSkipped`, is an error rather than a legal message checked + PROTOCOL_VIOLATION) is an error rather than a legal message checked against §10.4. - `Publication`'s automatic PUBLISH_DONE UPDATE_FAILED is sent while its subgroup streams are open, `WriteObject` still succeeds after `Done`, and a subgroup @@ -534,9 +533,6 @@ Session layer: - `Publication`'s REQUEST_UPDATE_OK carries LARGEST_OBJECT only for Objects it wrote itself, not the one its SUBSCRIBE_OK or PUBLISH reported (§10.2.17, §10.9.1). -- `ReadPublishSkipped` does not close the session on a REQUEST_UPDATE or - PUBLISH_STATE_NOTIFY from the publisher (§10.9, §10.10), and nothing enforces - NAMESPACE_DONE-before-NAMESPACE on a namespace subscription (§10.19). - A rejected request sends STOP_SENDING with INTERNAL_ERROR (§3.3.4 SHOULD use a relevant code). - Mandatory Track Property enforcement is off unless configured (§2.5.1). diff --git a/pkg/moqt/session/broker.go b/pkg/moqt/session/broker.go index ae710d89..e5eb06bd 100644 --- a/pkg/moqt/session/broker.go +++ b/pkg/moqt/session/broker.go @@ -10,6 +10,8 @@ import ( "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/track" + "github.com/floatdrop/moq-go/pkg/moqt/wire" ) // RequestBroker owns an established request stream's read side and @@ -34,6 +36,8 @@ import ( // [Session.Publish], a REQUEST_OK / REQUEST_ERROR before any Update, or a // SUBSCRIBE_OK on a Subscription, is a second response to the request and // closes the session (§5.1). +// - On a [NamespaceSubscription]'s broker, a NAMESPACE_DONE for a suffix no +// NAMESPACE announced closes the session (§10.19). // - A second GOAWAY on the stream, or one with a New Session URI received // by a server, closes the session (§10.4; see [RequestGoaways]). // - Everything else (PUBLISH_DONE, other unsolicited responses, …) is @@ -53,7 +57,7 @@ type RequestBroker struct { // arrive in request order, so the waiter queue order must match the // write order. mu sync.Mutex - waiters []chan updateResult + waiters []updateWaiter // updatesClosed is latched when the stream's reader exits (or Close is // called); subsequent Update calls fail immediately instead of queueing // a waiter nothing will ever answer. Plain WriteMessage stays allowed — @@ -81,6 +85,18 @@ type RequestBroker struct { // handle is the typed handle this broker came from, if any: Serve and // Close report the subscription's end to it (see requestHandle.terminated). handle *requestHandle + + // Serve's per-stream state, kept across calls: the stream's GOAWAYs + // (§10.4) when there is no handle to keep them, and on a + // SUBSCRIBE_NAMESPACE the current prefix and the full namespaces a + // NAMESPACE announced and no NAMESPACE_DONE withdrew (§10.19, §10.9.2). + goaways RequestGoaways + nsPrefix wire.TrackNamespace + namespaces map[track.Key]struct{} + // nsPrefixLost is set, under mu, once any Update gave up: responses no + // longer pair reliably, so the prefix is unknown and NAMESPACE_DONEs go + // unchecked. + nsPrefixLost bool } // PeerMessages declares whether the peer may send REQUEST_UPDATE (§10.9) and @@ -159,6 +175,47 @@ func (b *RequestBroker) answerUpdate(upd *message.RequestUpdate) (bool, error) { return false, nil } +// goawayState is the stream's GOAWAY checker: the handle's, shared with its +// other readers (e.g. [TrackSubscription.ReadPublishSkipped]), or the +// broker's own. +func (b *RequestBroker) goawayState() *RequestGoaways { + if b.handle != nil { + return &b.handle.goaways + } + return &b.goaways +} + +// namespaceFollowup tracks a NAMESPACE or NAMESPACE_DONE on a +// SUBSCRIBE_NAMESPACE's broker: "If a subscriber receives a NAMESPACE_DONE +// before the corresponding NAMESPACE, it MUST close the session with a +// 'PROTOCOL_VIOLATION'" (§10.19). Other brokers do not track them. +func (b *RequestBroker) namespaceFollowup(m message.Message) error { + if b.handle == nil || !b.handle.namespaces { + return nil + } + b.mu.Lock() + lost := b.nsPrefixLost + b.mu.Unlock() + if lost { + return nil + } + switch m := m.(type) { + case *message.Namespace: + if b.namespaces == nil { + b.namespaces = make(map[track.Key]struct{}) + } + b.namespaces[track.NewKey(slices.Concat(b.nsPrefix, m.TrackNamespaceSuffix), nil)] = struct{}{} + case *message.NamespaceDone: + key := track.NewKey(slices.Concat(b.nsPrefix, m.TrackNamespaceSuffix), nil) + if _, ok := b.namespaces[key]; !ok { + return b.sess.closeProtocolViolation(fmt.Errorf( + "moqt/session: NAMESPACE_DONE for %v before its NAMESPACE", m.TrackNamespaceSuffix)) + } + delete(b.namespaces, key) + } + return nil +} + // answered is the request type (SUBSCRIBE or PUBLISH) of a broker on this // side's request, whose response the peer has sent; zero otherwise. func (b *RequestBroker) answered() message.Type { @@ -168,6 +225,15 @@ func (b *RequestBroker) answered() message.Type { return b.handle.answered } +// updateWaiter is one sent REQUEST_UPDATE awaiting its §10.9 response and, +// when the update sets TRACK_NAMESPACE_PREFIX, the prefix it switches a +// namespace subscription to once accepted (§10.9.2). +type updateWaiter struct { + ch chan updateResult + prefix wire.TrackNamespace + setsPrefix bool +} + // updateResult carries one §10.9 response to a waiting Update call. type updateResult struct { ok *message.RequestOK @@ -241,7 +307,12 @@ func (b *RequestBroker) Update(ctx context.Context, params message.Parameters) ( Parameters: params, }) if err == nil { - b.waiters = append(b.waiters, ch) + w := updateWaiter{ch: ch} + if p, ok := params.Find(message.ParamTrackNamespacePrefix); ok { + prefix, perr := message.TrackNamespacePrefixFromParam(p) + w.prefix, w.setsPrefix = prefix, perr == nil + } + b.waiters = append(b.waiters, w) b.updated = true } b.mu.Unlock() @@ -254,7 +325,13 @@ func (b *RequestBroker) Update(ctx context.Context, params message.Parameters) ( return res.ok, res.err case <-ctx.Done(): b.mu.Lock() - if i := slices.Index(b.waiters, ch); i >= 0 { + if i := slices.IndexFunc(b.waiters, func(w updateWaiter) bool { return w.ch == ch }); i >= 0 { + // Its late answer will pair with the next update, so on a + // SUBSCRIBE_NAMESPACE where a prefix update's REQUEST_OK falls + // among the NAMESPACEs (§10.9.2) is no longer known. + if b.handle != nil && b.handle.namespaces { + b.nsPrefixLost = true + } b.waiters = slices.Delete(b.waiters, i, i+1) } b.mu.Unlock() @@ -313,7 +390,7 @@ func (b *RequestBroker) route(msg message.Message) bool { b.mu.Unlock() return false } - var recipients []chan updateResult + var recipients []updateWaiter if _, isErr := msg.(*message.RequestError); isErr { recipients, b.waiters = b.waiters, nil } else { @@ -322,9 +399,14 @@ func (b *RequestBroker) route(msg message.Message) bool { b.mu.Unlock() ok, err := b.sess.mapUpdateResponse(msg) + if err == nil && recipients[0].setsPrefix { + // §10.9.2: suffixes after this REQUEST_OK are relative to the new + // prefix. Only Serve's goroutine reads nsPrefix. + b.nsPrefix = recipients[0].prefix + } res := updateResult{ok: ok, err: err} - for _, ch := range recipients { - ch <- res + for _, w := range recipients { + w.ch <- res } return true } @@ -338,8 +420,8 @@ func (b *RequestBroker) closeUpdates() { b.waiters = nil b.updatesClosed = true b.mu.Unlock() - for _, ch := range waiters { - ch <- updateResult{err: ErrRequestStreamClosed} + for _, w := range waiters { + w.ch <- updateResult{err: ErrRequestStreamClosed} } } @@ -401,6 +483,29 @@ func (b *RequestBroker) receiveUpdate(m *message.RequestUpdate, updates *Request return nil } +// readFailed ends Serve on a read error: nil on the peer's FIN, ctx.Err() on +// cancellation, and err otherwise, after resetting the read side. +func (b *RequestBroker) readFailed(ctx context.Context, err error) error { + switch { + case ctx.Err() != nil: + return ctx.Err() + case errors.Is(err, io.EOF): + if b.handle != nil { + b.handle.peerFinished() + } + return nil + case errors.Is(err, message.ErrMalformedMessage): + // §10, and §10.2 for an unknown parameter. + b.stream.CancelRead(uint64(moqt.StreamResetInternalError)) + return b.sess.closeProtocolViolation(err) + default: + // Covers peer resets too (a STOP_SENDING on an already-reset stream + // is a transport no-op). + b.stream.CancelRead(uint64(moqt.StreamResetInternalError)) + return err + } +} + // Serve owns every read on the request stream until the peer tears it down // (EOF / reset), ctx is cancelled (the read side is then reset to unblock // the parse), or onMsg returns false. On exit, pending and future Update @@ -437,29 +542,11 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b // §10.3.1.7: per-stream MAX_REQUEST_UPDATES enforcement. One limiter per // stream, since the limit is scoped to a single request stream. updates := b.sess.NewRequestUpdateLimiter() - var goaways RequestGoaways for { msg, err := message.Parse(b.stream) if err != nil { - switch { - case ctx.Err() != nil: - return ctx.Err() - case errors.Is(err, io.EOF): - if b.handle != nil { - b.handle.peerFinished() - } - return nil - case errors.Is(err, message.ErrMalformedMessage): - // §10, and §10.2 for an unknown parameter. - b.stream.CancelRead(uint64(moqt.StreamResetInternalError)) - return b.sess.closeProtocolViolation(err) - default: - // Covers peer resets too (a STOP_SENDING on an - // already-reset stream is a transport no-op). - b.stream.CancelRead(uint64(moqt.StreamResetInternalError)) - return err - } + return b.readFailed(ctx, err) } // §10.2.2: follow-ups may REGISTER/DELETE token aliases; skipping @@ -517,7 +604,11 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b } // Unsolicited response — surface via onMsg below. case *message.Goaway: - if err := goaways.Received(b.sess, m); err != nil { + if err := b.goawayState().Received(b.sess, m); err != nil { + return err + } + case *message.Namespace, *message.NamespaceDone: + if err := b.namespaceFollowup(m); err != nil { return err } case *message.PublishStateNotify: diff --git a/pkg/moqt/session/example_test.go b/pkg/moqt/session/example_test.go index 800ff8e3..75f172e9 100644 --- a/pkg/moqt/session/example_test.go +++ b/pkg/moqt/session/example_test.go @@ -436,18 +436,17 @@ func ExampleSession_SubscribeNamespace() { } defer annStream.Close() - for { - msg, err := message.Parse(annStream) - if err != nil { - return - } + // The broker enforces the session-level rules, such as a NAMESPACE_DONE + // only after its NAMESPACE (§10.19), and returns when the stream ends. + _ = annStream.Broker().Serve(ctx, func(msg message.Message) bool { switch m := msg.(type) { case *message.Namespace: fmt.Printf("announced: %v\n", m.TrackNamespaceSuffix) case *message.NamespaceDone: fmt.Printf("done: %v\n", m.TrackNamespaceSuffix) } - } + return true + }) } // Accepting requests on the server side. A server (or relay) pulls inbound diff --git a/pkg/moqt/session/namespace.go b/pkg/moqt/session/namespace.go index d11a4f96..702c2b72 100644 --- a/pkg/moqt/session/namespace.go +++ b/pkg/moqt/session/namespace.go @@ -30,45 +30,54 @@ func (p *NamespacePublication) Close() error { // NamespaceSubscription is an established SUBSCRIBE_NAMESPACE request (§10.19). // It embeds the still-open request stream and carries the peer's REQUEST_OK; -// NAMESPACE / NAMESPACE_DONE notifications arrive on the embedded stream. -// Read it with [RequestBroker.Serve], which enforces the session-level rules -// (§10, §10.2.1); a caller using message.Parse must apply them itself. +// NAMESPACE / NAMESPACE_DONE notifications arrive on the embedded stream. Read +// it with its [NamespaceSubscription.Broker]'s [RequestBroker.Serve], which +// enforces the session-level rules: §10 and §10.2.1, a NAMESPACE_DONE only +// after its NAMESPACE (§10.19), and no PUBLISH_STATE_NOTIFY or REQUEST_UPDATE +// from the publisher (§10.9, §10.10). The NAMESPACE_DONE check follows a +// TRACK_NAMESPACE_PREFIX update from the REQUEST_OK that accepts it (§10.9.2). +// A caller using message.Parse must apply these rules itself. +// [NamespaceSubscription.Update] modifies the subscription (§10.9); Close ends +// it (§6.1, §3.3.3). type NamespaceSubscription struct { - // Stream is the SUBSCRIBE_NAMESPACE request stream, still open to receive - // NAMESPACE / NAMESPACE_DONE notifications. [NamespaceSubscription.Close] - // ends the subscription. - Stream + // requestHandle carries the SUBSCRIBE_NAMESPACE request stream, still + // open to receive NAMESPACE / NAMESPACE_DONE notifications. + requestHandle // OK is the REQUEST_OK the peer replied with. OK *message.RequestOK } -// Close ends the subscription by cancelling the request (§6.1, §3.3.3). -func (n *NamespaceSubscription) Close() error { - cancelRequest(n.Stream) - return nil -} - // TrackSubscription is an established SUBSCRIBE_TRACKS request (§10.20). It // embeds the still-open request stream and carries the peer's REQUEST_OK. // Follow-up PUBLISH_SKIPPED notifications are read via -// [TrackSubscription.ReadPublishSkipped]. +// [TrackSubscription.ReadPublishSkipped], or its [TrackSubscription.Broker]'s +// [RequestBroker.Serve], never both at once. Both close the session on a +// PUBLISH_STATE_NOTIFY or REQUEST_UPDATE from the publisher (§10.9, §10.10). +// [TrackSubscription.Update] modifies the subscription (§10.9); Close ends it +// (§6.1, §3.3.3). type TrackSubscription struct { - // Stream is the SUBSCRIBE_TRACKS request stream, still open to receive - // PUBLISH_SKIPPED follow-ups. [TrackSubscription.Close] ends the - // subscription. - Stream + // requestHandle carries the SUBSCRIBE_TRACKS request stream, still open + // to receive PUBLISH_SKIPPED follow-ups. + requestHandle // OK is the REQUEST_OK the peer replied with. OK *message.RequestOK +} - s *Session +// Update sends a REQUEST_UPDATE (§10.9), e.g. a new TRACK_NAMESPACE_PREFIX +// (§10.9.2), through the subscription's [NamespaceSubscription.Broker], whose +// Serve must be running to deliver the answer. Reading the answer directly +// would take a NAMESPACE for it. +func (n *NamespaceSubscription) Update(ctx context.Context, params message.Parameters) (*message.RequestOK, error) { + return n.Broker().Update(ctx, params) } -// Close ends the subscription by cancelling the request (§6.1, §3.3.3). -func (t *TrackSubscription) Close() error { - cancelRequest(t.Stream) - return nil +// Update sends a REQUEST_UPDATE (§10.9) through the subscription's +// [TrackSubscription.Broker], whose Serve must be running to deliver the +// answer, as for [NamespaceSubscription.Update]. +func (t *TrackSubscription) Update(ctx context.Context, params message.Parameters) (*message.RequestOK, error) { + return t.Broker().Update(ctx, params) } // PublishNamespace opens a PUBLISH_NAMESPACE request stream (§10.16) and @@ -102,7 +111,10 @@ func (s *Session) SubscribeNamespace( ) (*NamespaceSubscription, error) { return awaitRequestResponse(ctx, s, m, func(stream Stream, ok *message.RequestOK) (*NamespaceSubscription, error) { - return &NamespaceSubscription{Stream: stream, OK: ok}, nil + return &NamespaceSubscription{ + Stream: stream, s: s, requestID: m.RequestID, + namespaces: true, nsPrefix: m.TrackNamespacePrefix, OK: ok, + }, nil }) } @@ -121,7 +133,7 @@ func (s *Session) SubscribeTracks(ctx context.Context, m *message.SubscribeTrack cancelRequest(stream) return nil, err } - return &TrackSubscription{Stream: stream, OK: ok, s: s}, nil + return &TrackSubscription{Stream: stream, s: s, requestID: m.RequestID, OK: ok}, nil }) } @@ -226,20 +238,43 @@ func (r *Request) AcceptSubscribeTracks() (*IncomingTrackSubscription, error) { // message names the track the publisher couldn't push; the caller's sanctioned // recovery is to issue an explicit SUBSCRIBE for it. // -// It blocks until a message arrives or the stream ends. A non-PUBLISH_SKIPPED -// message is reported as an error, as is the underlying read error (e.g. +// It cannot be combined with [TrackSubscription.Update], whose answer only the +// broker's Serve delivers. +// +// It blocks until a PUBLISH_SKIPPED arrives or the stream ends. A single +// GOAWAY is skipped (§10.4); a caller that would re-issue the request at its +// New Session URI reads with the broker instead, whose Serve hands it over. A +// second GOAWAY, and a PUBLISH_STATE_NOTIFY or REQUEST_UPDATE from the +// publisher (§10.9, §10.10), close the session with PROTOCOL_VIOLATION. Any +// other message is reported as an error, as is the underlying read error (e.g. // io.EOF when the publisher FINs the SUBSCRIBE_TRACKS stream). func (t *TrackSubscription) ReadPublishSkipped() (*message.PublishSkipped, error) { - m, err := message.Parse(t.Stream) - if errors.Is(err, message.ErrMalformedMessage) { - return nil, t.s.closeProtocolViolation(fmt.Errorf("moqt/session: read SUBSCRIBE_TRACKS follow-up: %w", err)) - } - if err != nil { - return nil, fmt.Errorf("moqt/session: read SUBSCRIBE_TRACKS follow-up: %w", err) - } - pb, ok := m.(*message.PublishSkipped) - if !ok { - return nil, fmt.Errorf("moqt/session: unexpected %s on SUBSCRIBE_TRACKS stream, want PUBLISH_SKIPPED", m.Type()) + for { + m, err := message.Parse(t) + if errors.Is(err, message.ErrMalformedMessage) { + return nil, t.s.closeProtocolViolation(fmt.Errorf("moqt/session: read SUBSCRIBE_TRACKS follow-up: %w", err)) + } + if err != nil { + return nil, fmt.Errorf("moqt/session: read SUBSCRIBE_TRACKS follow-up: %w", err) + } + switch m := m.(type) { + case *message.PublishSkipped: + return m, nil + case *message.Goaway: + // §10.4: legal once; the request is not migrated. + if err := t.goaways.Received(t.s, m); err != nil { + return nil, err + } + case *message.PublishStateNotify, *message.RequestUpdate: + // §10.10: PUBLISH_STATE_NOTIFY "applies only to subscriptions"; + // §10.9: the publisher did not send this request. + return nil, t.s.closeProtocolViolation( + fmt.Errorf("moqt/session: %s on a SUBSCRIBE_TRACKS stream", m.Type())) + default: + return nil, fmt.Errorf( + "moqt/session: unexpected %s on SUBSCRIBE_TRACKS stream, want PUBLISH_SKIPPED", + m.Type(), + ) + } } - return pb, nil } diff --git a/pkg/moqt/session/namespace_followup_test.go b/pkg/moqt/session/namespace_followup_test.go new file mode 100644 index 00000000..c9b9d449 --- /dev/null +++ b/pkg/moqt/session/namespace_followup_test.go @@ -0,0 +1,339 @@ +package session_test + +import ( + "context" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/moqt/wire" +) + +// namespaceSubscribePair has cli SUBSCRIBE_NAMESPACE srv, and returns the +// subscription and the stream srv answers on. +func namespaceSubscribePair(t *testing.T, cli, srv *session.Session) (*session.NamespaceSubscription, session.Stream) { + t.Helper() + streams := make(chan session.Stream, 1) + go func() { + defer close(streams) + r, err := srv.AcceptRequest(t.Context()) + if err != nil { + return + } + if in, err := r.AcceptSubscribeNamespace(); err == nil { + streams <- in.Stream + } + }() + sub, err := cli.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{ + TrackNamespacePrefix: wire.TrackNamespace{[]byte("ns")}, + }) + must(t, err) + stream, ok := <-streams + if !ok { + t.Fatal("server failed to accept the SUBSCRIBE_NAMESPACE") + } + return sub, stream +} + +// trackSubscribePair is namespaceSubscribePair for SUBSCRIBE_TRACKS. +func trackSubscribePair(t *testing.T, cli, srv *session.Session) (*session.TrackSubscription, session.Stream) { + t.Helper() + streams := make(chan session.Stream, 1) + go func() { + defer close(streams) + r, err := srv.AcceptRequest(t.Context()) + if err != nil { + return + } + if in, err := r.AcceptSubscribeTracks(); err == nil { + streams <- in.Stream + } + }() + sub, err := cli.SubscribeTracks(t.Context(), &message.SubscribeTracks{ + TrackNamespacePrefix: wire.TrackNamespace{[]byte("ns")}, + }) + must(t, err) + stream, ok := <-streams + if !ok { + t.Fatal("server failed to accept the SUBSCRIBE_TRACKS") + } + return sub, stream +} + +// send writes msgs to stream in the background, in order. +func send(stream session.Stream, msgs ...message.Message) { + go func() { + for _, m := range msgs { + if err := message.Marshal(stream, m); err != nil { + return + } + } + }() +} + +func nsAnnounce(suffix string) *message.Namespace { + return &message.Namespace{TrackNamespaceSuffix: wire.TrackNamespace{[]byte(suffix)}} +} + +func nsDone(suffix string) *message.NamespaceDone { + return &message.NamespaceDone{TrackNamespaceSuffix: wire.TrackNamespace{[]byte(suffix)}} +} + +// serveBriefly runs b.Serve until it returns or 2s pass. +func serveBriefly(t *testing.T, b *session.RequestBroker, onMsg func(message.Message) bool) { + t.Helper() + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _ = b.Serve(ctx, onMsg) +} + +// TestNamespaceSubscriptionViolationCloses: on a NamespaceSubscription's +// broker a NAMESPACE_DONE with no NAMESPACE before it (§10.19), and a +// PUBLISH_STATE_NOTIFY (§10.10) or REQUEST_UPDATE (§10.9) from the publisher, +// close the session with PROTOCOL_VIOLATION. +func TestNamespaceSubscriptionViolationCloses(t *testing.T) { + for _, tc := range []struct { + name string + msgs []message.Message + }{ + {"NAMESPACE_DONE first", []message.Message{nsDone("a")}}, + {"NAMESPACE_DONE for another suffix", []message.Message{nsAnnounce("a"), nsDone("b")}}, + {"NAMESPACE_DONE twice", []message.Message{nsAnnounce("a"), nsDone("a"), nsDone("a")}}, + {"PUBLISH_STATE_NOTIFY", []message.Message{&message.PublishStateNotify{}}}, + {"REQUEST_UPDATE", []message.Message{&message.RequestUpdate{RequestID: 1}}}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) + send(stream, tc.msgs...) + serveBriefly(t, sub.Broker(), nil) + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + }) + } +} + +// TestNamespaceSubscriptionFollowupsDelivered: NAMESPACE and NAMESPACE_DONE in +// order reach Serve's callback, including a NAMESPACE_DONE read by a later +// Serve call than its NAMESPACE, and a re-announced suffix. +func TestNamespaceSubscriptionFollowupsDelivered(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) + b := sub.Broker() + send(stream, nsAnnounce("a"), nsDone("a"), nsAnnounce("a"), nsDone("a")) + + var got []message.Type + collect := func(n int) func(message.Message) bool { + return func(m message.Message) bool { + got = append(got, m.Type()) + return len(got) < n + } + } + serveBriefly(t, b, collect(1)) // stops after the first NAMESPACE + serveBriefly(t, b, collect(4)) + want := []message.Type{ + message.TypeNamespace, + message.TypeNamespaceDone, + message.TypeNamespace, + message.TypeNamespaceDone, + } + if len(got) != len(want) { + t.Fatalf("callback saw %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("callback saw %v, want %v", got, want) + } + } + requireStaysOpen(t, cli, 50*time.Millisecond) +} + +// TestTrackSubscriptionPeerMessagesClose: a PUBLISH_STATE_NOTIFY (§10.10) or +// REQUEST_UPDATE (§10.9) from the publisher of a SUBSCRIBE_TRACKS closes the +// session with PROTOCOL_VIOLATION, read by ReadPublishSkipped or the broker. +func TestTrackSubscriptionPeerMessagesClose(t *testing.T) { + for _, served := range []bool{false, true} { + for _, msg := range []message.Message{&message.PublishStateNotify{}, &message.RequestUpdate{RequestID: 1}} { + name := msg.Type().String() + " read by ReadPublishSkipped" + if served { + name = msg.Type().String() + " served" + } + t.Run(name, func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := trackSubscribePair(t, cli, srv) + send(stream, msg) + if served { + serveBriefly(t, sub.Broker(), nil) + } else if _, err := sub.ReadPublishSkipped(); err == nil { + t.Fatalf("ReadPublishSkipped returned a PUBLISH_SKIPPED for a %s", msg.Type()) + } + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + }) + } + } +} + +// TestReadPublishSkippedGoaway: ReadPublishSkipped skips a single GOAWAY +// (§10.4) to return the PUBLISH_SKIPPED after it, and closes the session on a +// second. +func TestReadPublishSkippedGoaway(t *testing.T) { + skipped := &message.PublishSkipped{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a")}, TrackName: []byte("t")} + t.Run("one", func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := trackSubscribePair(t, cli, srv) + send(stream, &message.Goaway{}, skipped) + got, err := sub.ReadPublishSkipped() + if err != nil { + t.Fatalf("ReadPublishSkipped: %v", err) + } + if string(got.TrackName) != "t" { + t.Fatalf("PUBLISH_SKIPPED for %q, want \"t\"", got.TrackName) + } + requireStaysOpen(t, cli, 50*time.Millisecond) + }) + t.Run("two", func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := trackSubscribePair(t, cli, srv) + send(stream, &message.Goaway{}, &message.Goaway{}, skipped) + if _, err := sub.ReadPublishSkipped(); err == nil { + t.Fatal("ReadPublishSkipped returned past a second GOAWAY") + } + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + }) +} + +// TestNamespaceSubscriptionUpdate: the subscriber may update a +// SUBSCRIBE_NAMESPACE (§10.9), and its broker pairs the answer. +func TestNamespaceSubscriptionUpdate(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) + go func() { + if _, err := message.Parse(stream); err != nil { + return + } + _ = message.Marshal(stream, &message.RequestOK{}) + }() + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + if _, err := sub.Update(ctx, nil); err != nil { + t.Fatalf("Update: %v", err) + } +} + +// TestNamespaceSubscriptionPrefixUpdate: NAMESPACE_DONE suffixes after the +// REQUEST_OK accepting a TRACK_NAMESPACE_PREFIX update are relative to the new +// prefix (§10.9.2), so the §10.19 check resolves them there; before it, or +// after a REQUEST_ERROR, against the old one. +func TestNamespaceSubscriptionPrefixUpdate(t *testing.T) { + ab := &message.Namespace{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a"), []byte("b")}} + for _, tc := range []struct { + name string + answer message.Message + done *message.NamespaceDone + closes bool + }{ + {"accepted, new-prefix suffix", &message.RequestOK{}, nsDone("b"), false}, + {"accepted, old-prefix suffix", &message.RequestOK{}, + &message.NamespaceDone{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a"), []byte("b")}}, true}, + {"rejected, old-prefix suffix", &message.RequestError{ErrorCode: moqt.RequestInternalError}, + &message.NamespaceDone{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a"), []byte("b")}}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) // prefix {ns} + go func() { + if message.Marshal(stream, ab) != nil { + return + } + if _, err := message.Parse(stream); err != nil { // the REQUEST_UPDATE + return + } + _ = message.Marshal(stream, tc.answer) + _ = message.Marshal(stream, tc.done) + }() + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _, _ = sub.Update(ctx, message.Parameters{ + message.TrackNamespacePrefixParam(wire.TrackNamespace{[]byte("ns"), []byte("a")}), + }) + if tc.closes { + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + } else { + requireStaysOpen(t, cli, 100*time.Millisecond) + } + }) + } +} + +// TestNamespaceSubscriptionLostPrefixUpdate: once an Update gives up, here a +// prefix update, where a new prefix applies is unknown, so NAMESPACE_DONEs are +// no longer checked rather than risking a close on a conforming peer. +func TestNamespaceSubscriptionLostPrefixUpdate(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + updated := make(chan struct{}) + go func() { + if _, err := message.Parse(stream); err != nil { + return + } + <-updated // answer only after the Update gave up + _ = message.Marshal(stream, &message.RequestOK{}) + _ = message.Marshal(stream, nsDone("b")) + }() + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _, _ = sub.Update(ctx, message.Parameters{ + message.TrackNamespacePrefixParam(wire.TrackNamespace{[]byte("ns"), []byte("a")}), + }) + close(updated) + requireStaysOpen(t, cli, 100*time.Millisecond) +} + +// TestNamespaceSubscriptionLostUpdateShiftsPairing: once any Update gives up, +// its late answer pairs with the next update, so a prefix switch could land a +// response early. The check then stops rather than resolve an old-prefix +// NAMESPACE_DONE against the new prefix and close on a conforming peer. +func TestNamespaceSubscriptionLostUpdateShiftsPairing(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) // prefix {ns} + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + + ab := wire.TrackNamespace{[]byte("a"), []byte("b")} + gaveUp := make(chan struct{}) + peer := make(chan struct{}) + go func() { + defer close(peer) + if message.Marshal(stream, &message.Namespace{TrackNamespaceSuffix: ab}) != nil { + return + } + for range 2 { // the abandoned update, then the prefix update + if _, err := message.Parse(stream); err != nil { + return + } + } + <-gaveUp + _ = message.Marshal(stream, &message.RequestOK{}) // the first update's + _ = message.Marshal(stream, &message.NamespaceDone{TrackNamespaceSuffix: ab}) // old prefix + _ = message.Marshal(stream, &message.RequestOK{}) // the prefix update's + }() + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _, _ = sub.Update(ctx, nil) + close(gaveUp) + ctx2, cancel2 := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel2() + _, _ = sub.Update(ctx2, message.Parameters{ + message.TrackNamespacePrefixParam(wire.TrackNamespace{[]byte("ns"), []byte("a")}), + }) + <-peer + requireStaysOpen(t, cli, 100*time.Millisecond) +} diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index d9872486..535e4d76 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -14,6 +14,7 @@ import ( "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/track" + "github.com/floatdrop/moq-go/pkg/moqt/wire" ) // ErrRequestIDParityViolation is returned by [Session.CheckPeerRequestID] when @@ -533,6 +534,16 @@ type requestHandle struct { // peerDone records that the publisher's FIN was read. peerDone atomic.Bool + // namespaces marks a SUBSCRIBE_NAMESPACE, whose broker checks each + // NAMESPACE_DONE against the NAMESPACEs before it (§10.19); nsPrefix is + // the Track Namespace Prefix it was sent with. + namespaces bool + nsPrefix wire.TrackNamespace + + // goaways checks the GOAWAYs this stream carries (§10.4), for every + // reader of the handle. + goaways RequestGoaways + // answered is TypeSubscribe or TypePublish for a request this side sent, // whose response was read: its broker closes the session on another // (§5.1). Zero otherwise. @@ -608,6 +619,7 @@ func (h *requestHandle) Broker() *RequestBroker { b.PeerMessages(h.peerUpdate, h.peerNotify) b.UpdateScope(h.updateScope) b.handle = h + b.nsPrefix = h.nsPrefix h.broker.Store(b) }) return h.broker.Load()