From 7f3500b376f7397d7075ac9e98699605bd663a1b Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 17:42:20 +0500 Subject: [PATCH 1/9] docs(status): record the second draft-20 review backlog A second full review against draft-ietf-moq-transport-20, at dbe571e, checked the fixes for the first backlog and the code they added. Most of the first backlog is fixed. This records what remains and what the review newly found: six high-severity gaps, the session-layer, validation and relay items, and documentation drift, each with the draft rule it misses. The citations and quotes were checked against the draft-20 text by the moqt-reviewer. Co-Authored-By: Claude Opus 5.5 (1M context) --- STATUS.md | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/STATUS.md b/STATUS.md index d325b5fc..ae115b52 100644 --- a/STATUS.md +++ b/STATUS.md @@ -492,3 +492,131 @@ Known protocol gaps, roughly ordered by how load-bearing they are: the first copy of each {Group, Object} is forwarded and later ones are dropped unread. Comparing them would detect a malformed track (§2.4.2 condition 6), at a cost on every Object. + +### Draft-20 compliance review backlog + +A second full review against draft-ietf-moq-transport-20 (2026-09-26, at +`dbe571e`) found the gaps below. Each item names the rule it misses. Items +already listed as Limitations above are not repeated here. + +High: + +- A FETCH answered from the cache treats every Object missing above the + eviction floor as non-existent and FINs the stream, although a lost datagram, + a reset subgroup stream or a subgroup still in flight leaves such holes. §10.13: + a relay that meets an uncached Object of unknown status "MUST pause subsequent + delivery until it has confirmed the object's status upstream". +- The relay's 32-Group dedup window measures Group ID distance, so an Object of a + Group 32 or more IDs below the newest is dropped as already delivered and the + downstream stream then FINs. §9.4: "MUST NOT reorder or drop objects received on + a multi-object stream"; §11.4.3 requires a reset when Objects were not delivered. +- A request stream reset or FINed before its first message is complete ends the + relay's request loop without closing the session (§3.3.2, §3.3.3: that request + fails or is cancelled, nothing more). + A bad Request ID on an opener takes the same path instead of closing the session + with INVALID_REQUEST_ID (§10.1). +- A subscriber's cancellation does not reset the subscription's open subgroup + streams or fill fetch streams: the relay keeps forwarding, then FINs (§5.1.1: + "MUST reset any open streams associated with the SUBSCRIBE"; §5.1.3.1). +- A REQUEST_UPDATE carrying TRACK_NAMESPACE_PREFIX on SUBSCRIBE_NAMESPACE or + SUBSCRIBE_TRACKS (§10.9.2) is applied without authorization, and its tokens + are dropped. §10.19 and §10.20 say "The publisher MUST ensure the subscriber + is authorized to perform this namespace subscription", which an updated + prefix changes; §10.2.2 lets a REQUEST_UPDATE carry the token that + authorizes it. +- A fill stream on a subscription that omits GROUP_ORDER is written Ascending, + ignoring DEFAULT_PUBLISHER_GROUP_ORDER (§10.2.8, §10.2.15, §12.5). A subscriber + decoding it Descending gets wrong Group IDs (§11.4.4.1). + +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 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 + 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 + streams are open, `WriteObject` still succeeds after `Done`, and a subgroup + opened concurrently with `Done` is missing from the Stream Count (§10.12). +- `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 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). +- FETCH Serialization Flags ≥ 128 are read as field bits before being rejected, + so a reset or oversized length avoids the PROTOCOL_VIOLATION (§11.4.4). +- 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): + +- 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: + +- Any REQUEST_UPDATE turns INCLUDE_PROPERTIES=0 back off, so the subscriber + resolves the wrong default Publisher Priority. §10.9: a parameter absent from + REQUEST_UPDATE "remains unchanged", and INCLUDE_PROPERTIES cannot appear in + one (§10.2.21, §12.4). +- A merged Subgroup FINs when one contributor ends cleanly although its Objects + began after ones a reset contributor never delivered (§11.4.3). +- Replay streams (joiners, gap and properties reopens) lose the first Object's + delivery-timeout override (§8, §12.1, §12.2). +- FETCH_OK never sets End Of Track (§10.14). +- A cancelled FETCH keeps writing its data stream (§5.2: "MUST reset"). +- FILL_TIMEOUT does not bound an upstream FETCH once its data stream is open + (§10.2.5 SHOULD). +- Objects from an upstream FETCH are exempt from MAX_CACHE_DURATION, and cached + Objects age from when they were read whole rather than their beginning (§12.3). +- A fill range is evaluated against a later Largest Object than SUBSCRIBE_OK or + REQUEST_UPDATE_OK reported (§5.1.3). +- TRACK_STATUS returns DOES_NOT_EXIST for a PUBLISHed track with no properties + or Objects, which SUBSCRIBE accepts (§10.15: "treats it identically"). +- A client cannot SUBSCRIBE to a track it publishes under its own + PUBLISH_NAMESPACE (§5.1). +- RENDEZVOUS_TIMEOUT is ignored (§10.2.6 SHOULD hold the subscription; §9.5). +- REQUEST_ERROR MALFORMED_TRACK, defined for FETCH, answers SUBSCRIBE, PUBLISH + and REQUEST_UPDATE failures (§10.6.2). +- The relay keeps initiating requests on a session it sent GOAWAY to (§10.4 + SHOULD avoid), and closes with GOAWAY_TIMEOUT when it sent none (§3.5). +- A PUBLISH can follow PUBLISH_SKIPPED for the same upstream PUBLISH after a + prefix update moves away and back (§6.1). +- Upstream FETCHes to a publisher whose track is found malformed are not + cancelled (§2.4.2). +- Filters are not aggregated upstream (§6.3.1 SHOULD). + +Documentation: + +- Limitations: "Duplicate Objects … are not compared" is stale; the FETCH + GROUP_ORDER entry omits PUBLISH and FILL_PARAMETERS; 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.5, 10.2.6, 10.2.8, 10.2.9, 10.2.15, 10.2.18, 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 + PUBLISH_NAMESPACE stream (§10.17, §10.18). +- About a dozen stale `§` citations (padding, grease, fetch ordering, caching). + +Open questions for interop: whether an End of Range marker carries an Object +Payload Length (Figure 28 vs §11.4.4.2), and whether EXPIRES may appear in +TRACK_STATUS_OK (§10.15 vs §10.2.16). From d03e6f9e14a6dc68b1c7d9044d55fbe6c5449801 Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 17:50:55 +0500 Subject: [PATCH 2/9] =?UTF-8?q?fix(relay):=20authorize=20a=20namespace=20s?= =?UTF-8?q?ubscription's=20TRACK=5FNAMESPACE=5FPREFIX=20update=20(=C2=A710?= =?UTF-8?q?.19,=20=C2=A710.20)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A REQUEST_UPDATE that moved a SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS to a new prefix was applied after only the PREFIX_OVERLAP check, and the tokens it carried were resolved and discarded. A subscriber admitted under (tenantA) could update to () and receive NAMESPACE for every namespace on the relay, or with SUBSCRIBE_TRACKS a PUBLISH, with data, for every track. §10.19 and §10.20: "The publisher MUST ensure the subscriber is authorized to perform this namespace subscription". A prefix update now goes through the existing AuthorizeSubscribeNamespace / AuthorizeSubscribeTracks with the subscription as updated: the new prefix and the latest AUTHORIZATION_TOKENs the request or its updates carried (§10.9: an absent parameter "remains unchanged"; a DELETE authorizes nothing). Every update's tokens also go through the TokenVerifier, with or without a prefix change, via the new Session.VerifyTokens. A refusal is REQUEST_ERROR with the Authorizer's or verifier's code, and ends the request as any refused namespace update does (§10.9.1); the prefix is left unchanged. The Authorizer interface is unchanged (design chosen with the maintainer). The new tests fail on the unpatched code: both prefix widenings are accepted, both denied-token updates are accepted, and the Authorizer is never consulted on an update. The DELETE case was also seen failing with the exclusion removed. Update tokens on SUBSCRIBE, FETCH and PUBLISH_NAMESPACE are still resolved but not verified; that is recorded in the STATUS.md backlog. Co-Authored-By: Claude Opus 5.5 (1M context) --- STATUS.md | 9 +- pkg/moqt/session/token_verify.go | 10 +- pkg/relay/auth.go | 13 +- pkg/relay/handler_fetch.go | 2 +- pkg/relay/handler_namespace.go | 126 +++++++++++-- pkg/relay/handler_subscribe.go | 2 +- pkg/relay/namespace_update_auth_test.go | 238 ++++++++++++++++++++++++ pkg/relay/session_handler.go | 40 ++-- 8 files changed, 395 insertions(+), 45 deletions(-) create mode 100644 pkg/relay/namespace_update_auth_test.go diff --git a/STATUS.md b/STATUS.md index ae115b52..1ce71668 100644 --- a/STATUS.md +++ b/STATUS.md @@ -518,12 +518,6 @@ High: - A subscriber's cancellation does not reset the subscription's open subgroup streams or fill fetch streams: the relay keeps forwarding, then FINs (§5.1.1: "MUST reset any open streams associated with the SUBSCRIBE"; §5.1.3.1). -- A REQUEST_UPDATE carrying TRACK_NAMESPACE_PREFIX on SUBSCRIBE_NAMESPACE or - SUBSCRIBE_TRACKS (§10.9.2) is applied without authorization, and its tokens - are dropped. §10.19 and §10.20 say "The publisher MUST ensure the subscriber - is authorized to perform this namespace subscription", which an updated - prefix changes; §10.2.2 lets a REQUEST_UPDATE carry the token that - authorizes it. - A fill stream on a subscription that omits GROUP_ORDER is written Ascending, ignoring DEFAULT_PUBLISHER_GROUP_ORDER (§10.2.8, §10.2.15, §12.5). A subscriber decoding it Descending gets wrong Group IDs (§11.4.4.1). @@ -603,6 +597,9 @@ Relay: - Upstream FETCHes to a publisher whose track is found malformed are not cancelled (§2.4.2). - Filters are not aggregated upstream (§6.3.1 SHOULD). +- A REQUEST_UPDATE's AUTHORIZATION_TOKENs go through the TokenVerifier only on + SUBSCRIBE_NAMESPACE and SUBSCRIBE_TRACKS; on SUBSCRIBE, FETCH and + PUBLISH_NAMESPACE they are resolved but not verified (§10.2.2). Documentation: diff --git a/pkg/moqt/session/token_verify.go b/pkg/moqt/session/token_verify.go index 8d775c59..01fbcfa9 100644 --- a/pkg/moqt/session/token_verify.go +++ b/pkg/moqt/session/token_verify.go @@ -212,10 +212,16 @@ func (s *Session) applyToken(t *message.Token) (tok ResolvedToken, ok bool, err // can call it from their own request loop. It is safe to call with a req whose // Tokens slice is empty. func (s *Session) VerifyRequestTokens(ctx context.Context, req *Request) error { - if s.tokenVerifier == nil || len(req.Tokens) == 0 { + return s.VerifyTokens(ctx, req.Tokens) +} + +// VerifyTokens is [Session.VerifyRequestTokens] for tokens resolved by +// [Session.ProcessFollowupTokens], such as a REQUEST_UPDATE's (§10.2.2). +func (s *Session) VerifyTokens(ctx context.Context, toks []ResolvedToken) error { + if s.tokenVerifier == nil || len(toks) == 0 { return nil } - for _, tok := range req.Tokens { + for _, tok := range toks { if err := s.tokenVerifier.VerifyToken(ctx, s, tok); err != nil { if denied, ok := errors.AsType[*TokenDeniedError](err); ok { return denied diff --git a/pkg/relay/auth.go b/pkg/relay/auth.go index 279cb12c..deb61060 100644 --- a/pkg/relay/auth.go +++ b/pkg/relay/auth.go @@ -15,6 +15,13 @@ import ( // mutation; a non-nil return causes the relay to reply REQUEST_ERROR with // the [DeniedError]'s mapped code (see [DeniedError.RequestErrorCode]). // +// A REQUEST_UPDATE that changes a SUBSCRIBE_NAMESPACE's or SUBSCRIBE_TRACKS's +// TRACK_NAMESPACE_PREFIX is authorized again (§10.19, §10.20): the method +// receives the subscription as updated: the new prefix, and the +// AUTHORIZATION_TOKENs of the latest request or update that carried any (a +// DELETE authorizes nothing). A denial refuses the update and ends the +// subscription (§10.9.1). +// // The interface is split per request type for two reasons: // // - It lets a policy reject categories of request without having to @@ -130,9 +137,9 @@ func ReasonForAuthorizerError(err error) string { // // Production deployments SHOULD replace this with a token- or // session-attestation-aware implementation via [Config.Authorizer]. The relay -// only invokes the authorizer once per request before any state mutation, so -// the cost of policy evaluation is bounded by the request rate rather than -// the object rate. +// invokes the authorizer once per request, and per prefix update, before any +// state mutation, so the cost of policy evaluation is bounded by the request +// rate rather than the object rate. type AllowAllAuthorizer struct{} var _ Authorizer = AllowAllAuthorizer{} diff --git a/pkg/relay/handler_fetch.go b/pkg/relay/handler_fetch.go index f0ff9d18..e3aaafa3 100644 --- a/pkg/relay/handler_fetch.go +++ b/pkg/relay/handler_fetch.go @@ -198,7 +198,7 @@ func (h *sessionHandler) readFetchUpdates(ctx context.Context, req *session.Requ } // §10.2.2: an update may REGISTER/DELETE token aliases; // a cache fault there is session-fatal. - if !h.handleFollowupTokens(ctx, upd) { + if _, ok := h.handleFollowupTokens(ctx, upd); !ok { return false } h.handleFetchUpdate(ctx, req) diff --git a/pkg/relay/handler_namespace.go b/pkg/relay/handler_namespace.go index a7ede1d3..ee829f62 100644 --- a/pkg/relay/handler_namespace.go +++ b/pkg/relay/handler_namespace.go @@ -201,7 +201,7 @@ func (h *sessionHandler) handleSubscribeNamespace( h.spawn(entry.RunWriter) // Replies share the entry's queue, keeping their order with NAMESPACE. - h.serveNamespaceFollowups(ctx, req, enqueueReply(entry), h.namespaceUpdate(entry, &prefix)) + h.serveNamespaceFollowups(ctx, req, enqueueReply(entry), h.namespaceUpdate(entry, &prefix, msg)) } // handleSubscribeTracks implements SUBSCRIBE_TRACKS (§6.1, §10.20): @@ -265,7 +265,7 @@ func (h *sessionHandler) handleSubscribeTracks( } // Replies share the entry's queue with PUBLISH_SKIPPED, so each // PUBLISH_SKIPPED suffix matches the prefix the subscriber last saw. - h.serveNamespaceFollowups(ctx, req, enqueueReply(entry), h.tracksUpdate(entry, &prefix)) + h.serveNamespaceFollowups(ctx, req, enqueueReply(entry), h.tracksUpdate(entry, &prefix, msg)) } // subscribeTracksForwarding resolves a SUBSCRIBE_TRACKS's FORWARD (§10.2.18, @@ -290,15 +290,15 @@ func subscribeTracksForwarding(ps message.Parameters) (forward bool, groupOrder } // serveNamespaceFollowups holds a namespace request stream open and answers -// each REQUEST_UPDATE (§10.9), validating its Request ID (§10.1) and tokens. -// The subscriptions pass update, which applies it and replies; with update -// nil (PUBLISH_NAMESPACE) write sends a plain REQUEST_OK. Other follow-ups -// are ignored. +// each REQUEST_UPDATE (§10.9), validating its Request ID (§10.1) and resolving +// its tokens. The subscriptions pass update, which authorizes and applies it +// and replies; with update nil (PUBLISH_NAMESPACE) write sends a plain +// REQUEST_OK. Other follow-ups are ignored. func (h *sessionHandler) serveNamespaceFollowups( ctx context.Context, req *session.Request, write func(message.Message) error, - update func(context.Context, *message.RequestUpdate) bool, + update func(context.Context, *message.RequestUpdate, []session.ResolvedToken) bool, ) { stream := req.Stream scope := message.ScopeOfUpdate(req.First.Type()) @@ -323,12 +323,13 @@ func (h *sessionHandler) serveNamespaceFollowups( if !h.handleRequestUpdateLimit(ctx, updates) { return false } - if !h.handleFollowupTokens(ctx, upd) { + toks, ok := h.handleFollowupTokens(ctx, upd) + if !ok { return false } // false from update ends the request. if update != nil { - if !update(ctx, upd) { + if !update(ctx, upd, toks) { return false } updates.Responded() @@ -376,18 +377,33 @@ func (h *sessionHandler) updatePrefixParam(upd *message.RequestUpdate) (prefix w return prefix, true, true } -// namespaceUpdate answers a REQUEST_UPDATE on a SUBSCRIBE_NAMESPACE: a -// TRACK_NAMESPACE_PREFIX is applied (§10.9.2); anything else is acknowledged. +// namespaceUpdate answers a REQUEST_UPDATE on the SUBSCRIBE_NAMESPACE msg: a +// TRACK_NAMESPACE_PREFIX is authorized and applied (§10.9.2); anything else is +// acknowledged. A refused update ends the request (see [endAfterFinish]). func (h *sessionHandler) namespaceUpdate( e *registry.SubscriberEntry, cur *wire.TrackNamespace, -) func(context.Context, *message.RequestUpdate) bool { + msg *message.SubscribeNamespace, +) func(context.Context, *message.RequestUpdate, []session.ResolvedToken) bool { updatePrefix := h.prefixUpdater(e, &h.nsPrefixes, cur) - return func(ctx context.Context, upd *message.RequestUpdate) bool { + tokens := authorizingTokens(msg.Parameters) + return func(ctx context.Context, upd *message.RequestUpdate, toks []session.ResolvedToken) bool { prefix, found, ok := h.updatePrefixParam(upd) if !ok { return false } + updTokens := updatedTokens(tokens, upd.Parameters) + if rej := h.refuseUpdate(ctx, toks, found, func() error { + return h.auth.AuthorizeSubscribeNamespace(ctx, h.sess, &message.SubscribeNamespace{ + RequestID: msg.RequestID, + TrackNamespacePrefix: prefix, + Parameters: withTokens(msg.Parameters, updTokens), + }) + }); rej != nil { + e.Finish(rej) + return endAfterFinish(ctx, e) + } + tokens = updTokens if !found { e.Enqueue(&message.RequestOK{}) return true @@ -409,14 +425,29 @@ func (h *sessionHandler) namespaceUpdate( func (h *sessionHandler) tracksUpdate( e *registry.SubscriberEntry, cur *wire.TrackNamespace, -) func(context.Context, *message.RequestUpdate) bool { - return func(ctx context.Context, upd *message.RequestUpdate) bool { + msg *message.SubscribeTracks, +) func(context.Context, *message.RequestUpdate, []session.ResolvedToken) bool { + tokens := authorizingTokens(msg.Parameters) + return func(ctx context.Context, upd *message.RequestUpdate, toks []session.ResolvedToken) bool { prefix, hasPrefix, ok := h.updatePrefixParam(upd) if !ok { return false } before := e.TracksParams() - params, err := h.resolveTracksParams(mergeTracksUpdate(before.Params, upd.Parameters)) + merged := mergeTracksUpdate(before.Params, upd.Parameters) + updTokens := updatedTokens(tokens, upd.Parameters) + if rej := h.refuseUpdate(ctx, toks, hasPrefix, func() error { + return h.auth.AuthorizeSubscribeTracks(ctx, h.sess, &message.SubscribeTracks{ + RequestID: msg.RequestID, + TrackNamespacePrefix: prefix, + Parameters: withTokens(merged, updTokens), + }) + }); rej != nil { + e.Finish(rej) + return endAfterFinish(ctx, e) + } + tokens = updTokens + params, err := h.resolveTracksParams(merged) if err != nil { if _, ok := errors.AsType[*paramProtocolViolation](err); ok { _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) @@ -457,6 +488,69 @@ func (h *sessionHandler) tracksUpdate( } } +// refuseUpdate authorizes a REQUEST_UPDATE on a namespace subscription, +// returning the REQUEST_ERROR that refuses it, or nil. Its tokens (§10.2.2) +// go through the TokenVerifier as an opener's do. When it changes the prefix, +// authorize runs the Authorizer on the subscription it would become: §10.19 +// and §10.20 require that "the subscriber is authorized to perform this +// namespace subscription". +func (h *sessionHandler) refuseUpdate( + ctx context.Context, + toks []session.ResolvedToken, + prefixChanged bool, + authorize func() error, +) *message.RequestError { + if err := h.sess.VerifyTokens(ctx, toks); err != nil { + code, reason := tokenDenial(err) + return &message.RequestError{ErrorCode: code, ErrorReason: reason} + } + if !prefixChanged { + return nil + } + if err := authorize(); err != nil { + return &message.RequestError{ + ErrorCode: CodeForAuthorizerError(err), + ErrorReason: ReasonForAuthorizerError(err), + } + } + return nil +} + +// authorizingTokens is the AUTHORIZATION_TOKENs in ps that can authorize a +// request: all but DELETEs, which only retire an alias (§10.2.2). +func authorizingTokens(ps message.Parameters) message.Parameters { + var out message.Parameters + for _, p := range ps { + if p.Type != message.ParamAuthorizationToken { + continue + } + var tok message.Token + if tok.Parse(p.Bytes) == nil && tok.AliasType != message.AliasTypeDelete { + out = append(out, p) + } + } + return out +} + +// updatedTokens is the tokens a namespace subscription holds after an update +// carrying upd: its authorizing tokens replace cur when it has any; otherwise +// cur "remains unchanged" (§10.9). +func updatedTokens(cur, upd message.Parameters) message.Parameters { + if toks := authorizingTokens(upd); len(toks) > 0 { + return toks + } + return cur +} + +// withTokens is ps with its AUTHORIZATION_TOKENs replaced by toks: the +// subscription an update's Authorizer call judges. +func withTokens(ps, toks message.Parameters) message.Parameters { + out := slices.DeleteFunc(slices.Clone(ps), func(p message.Parameter) bool { + return p.Type == message.ParamAuthorizationToken + }) + return append(out, toks...) +} + // mergeTracksUpdate applies a REQUEST_UPDATE's parameters to a // SUBSCRIBE_TRACKS's: each type present in upd replaces every stored one of // that type (§10.9); a zero-length Range Filter removes it (§5.1.4). diff --git a/pkg/relay/handler_subscribe.go b/pkg/relay/handler_subscribe.go index 9211345d..62550a15 100644 --- a/pkg/relay/handler_subscribe.go +++ b/pkg/relay/handler_subscribe.go @@ -206,7 +206,7 @@ func (h *sessionHandler) readSubscribeUpdates( } // §10.2.2: an update may REGISTER/DELETE token aliases; // a cache fault there is session-fatal. - if !h.handleFollowupTokens(ctx, upd) { + if _, ok := h.handleFollowupTokens(ctx, upd); !ok { return false } h.handleSubscribeUpdate(ctx, sub, fullName, upd) diff --git a/pkg/relay/namespace_update_auth_test.go b/pkg/relay/namespace_update_auth_test.go new file mode 100644 index 00000000..2bcb4537 --- /dev/null +++ b/pkg/relay/namespace_update_auth_test.go @@ -0,0 +1,238 @@ +package relay_test + +import ( + "context" + "slices" + "sync" + "testing" + + "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" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// A TRACK_NAMESPACE_PREFIX update changes what a namespace subscription +// covers, so it is authorized like the subscription itself (§10.19, §10.20: +// "The publisher MUST ensure the subscriber is authorized to perform this +// namespace subscription"), with the update's own tokens (§10.2.2). + +// prefixAuthorizer admits namespace subscriptions under allowed only. +type prefixAuthorizer struct { + relay.AllowAllAuthorizer + + allowed wire.TrackNamespace +} + +func (a *prefixAuthorizer) check(prefix wire.TrackNamespace) error { + if !prefix.HasPrefix(a.allowed) { + return relay.DenyReason("outside the tenant") + } + return nil +} + +func (a *prefixAuthorizer) AuthorizeSubscribeNamespace( + _ context.Context, + _ *session.Session, + m *message.SubscribeNamespace, +) error { + return a.check(m.TrackNamespacePrefix) +} + +func (a *prefixAuthorizer) AuthorizeSubscribeTracks( + _ context.Context, + _ *session.Session, + m *message.SubscribeTracks, +) error { + return a.check(m.TrackNamespacePrefix) +} + +// TestNamespaceUpdate_PrefixOutsideAuthorizationRefused: a SUBSCRIBE_NAMESPACE +// cannot widen itself past what the Authorizer admits; the refused update ends +// the request (§10.9.1) without announcing the namespace. +func TestNamespaceUpdate_PrefixOutsideAuthorizationRefused(t *testing.T) { + t.Parallel() + pubSess, teardown := connectRelay(t, relay.Config{ + Authorizer: &prefixAuthorizer{allowed: ns("tenantA")}, + }) + defer teardown() + publishNS(t, pubSess, "secret", "x") + subSess := dialAnotherClient(t, pubSess) + nsSub, msgs := subscribeNS(t, subSess, "tenantA") + + sendPrefixUpdate(t, subSess, nsSub.Stream, "secret") + m := nextMessage(t, msgs) + rej, ok := m.(*message.RequestError) + if !ok || rej.ErrorCode != moqt.RequestUnauthorized { + t.Fatalf("got %T %+v, want REQUEST_ERROR UNAUTHORIZED", m, m) + } + requireStreamEnds(t, msgs) +} + +// TestSubscribeTracksUpdate_PrefixOutsideAuthorizationRefused: the same for +// SUBSCRIBE_TRACKS, whose widened prefix would forward every track under it. +func TestSubscribeTracksUpdate_PrefixOutsideAuthorizationRefused(t *testing.T) { + t.Parallel() + subSess, teardown := connectRelay(t, relay.Config{ + Authorizer: &prefixAuthorizer{allowed: ns("tenantA")}, + }) + defer teardown() + reqs := forwardedPublishes(t, subSess) + stream := subscribeTracks(t, subSess, ns("tenantA")) + publishVideoTrack(t, dialAnotherClient(t, subSess), "cam", 7) + + _, err := subSess.UpdateRequest(t.Context(), stream, + message.Parameters{message.TrackNamespacePrefixParam(ns("video"))}) + requireRejectedWithCode(t, err, moqt.RequestUnauthorized) + requireNoForward(t, reqs, "a refused prefix update") +} + +// openNamespaceSub opens a SUBSCRIBE_NAMESPACE, or a SUBSCRIBE_TRACKS when +// tracks, with params, and returns its request stream; it closes at cleanup. +func openNamespaceSub( + t *testing.T, + sess *session.Session, + tracks bool, + prefix wire.TrackNamespace, + params ...message.Parameter, +) session.Stream { + t.Helper() + if tracks { + return subscribeTracks(t, sess, prefix, params...) + } + s, err := sess.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{ + TrackNamespacePrefix: prefix, Parameters: params, + }) + if err != nil { + t.Fatalf("SubscribeNamespace: %v", err) + } + t.Cleanup(func() { _ = s.Close() }) + return s.Stream +} + +// tokenParam is a USE_VALUE AUTHORIZATION_TOKEN carrying v. +func tokenParam(v string) message.Parameter { + return message.AuthorizationTokenParam(message.Token{ + AliasType: message.AliasTypeUseValue, TokenType: 1, TokenValue: []byte(v), + }) +} + +// TestNamespaceUpdate_TokenVerified: an update's AUTHORIZATION_TOKEN goes +// through the TokenVerifier, with or without a prefix change, and its denial +// refuses the update with the verifier's code. +func TestNamespaceUpdate_TokenVerified(t *testing.T) { + t.Parallel() + verifier := session.TokenVerifierFunc( + func(_ context.Context, _ *session.Session, tok session.ResolvedToken) error { + if string(tok.Value) == "expired" { + return session.DenyToken(moqt.RequestExpiredAuthToken, "token expired") + } + return nil + }) + for _, tc := range []struct { + name string + tracks bool + update message.Parameters + }{ + {"SUBSCRIBE_NAMESPACE prefix", false, + message.Parameters{message.TrackNamespacePrefixParam(ns("video")), tokenParam("expired")}}, + {"SUBSCRIBE_NAMESPACE token only", false, message.Parameters{tokenParam("expired")}}, + {"SUBSCRIBE_TRACKS prefix", true, + message.Parameters{message.TrackNamespacePrefixParam(ns("video")), tokenParam("expired")}}, + {"SUBSCRIBE_TRACKS token only", true, message.Parameters{tokenParam("expired")}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + subSess, teardown := connectRelay(t, relay.Config{ + SessionOptions: []session.Option{session.WithTokenVerifier(verifier)}, + }) + defer teardown() + stream := openNamespaceSub(t, subSess, tc.tracks, ns("audio")) + _, err := subSess.UpdateRequest(t.Context(), stream, tc.update) + requireRejectedWithCode(t, err, moqt.RequestExpiredAuthToken) + }) + } +} + +// tokenRecorder records the AUTHORIZATION_TOKEN values each namespace +// subscription authorization carried. +type tokenRecorder struct { + relay.AllowAllAuthorizer + + mu sync.Mutex + seen [][]string +} + +func (a *tokenRecorder) record(ps message.Parameters) error { + toks, err := message.TokensFromParam(ps) + if err != nil { + return err + } + var vals []string + for _, tok := range toks { + vals = append(vals, string(tok.TokenValue)) + } + a.mu.Lock() + a.seen = append(a.seen, vals) + a.mu.Unlock() + return nil +} + +func (a *tokenRecorder) AuthorizeSubscribeNamespace( + _ context.Context, + _ *session.Session, + m *message.SubscribeNamespace, +) error { + return a.record(m.Parameters) +} + +func (a *tokenRecorder) AuthorizeSubscribeTracks( + _ context.Context, + _ *session.Session, + m *message.SubscribeTracks, +) error { + return a.record(m.Parameters) +} + +// TestNamespaceUpdate_AuthorizerSeesLatestTokens: a prefix update is +// authorized with the tokens of the latest request or update that carried +// any, since a parameter absent from REQUEST_UPDATE "remains unchanged" +// (§10.9). A DELETE only retires an alias, so it replaces nothing (§10.2.2). +func TestNamespaceUpdate_AuthorizerSeesLatestTokens(t *testing.T) { + t.Parallel() + for _, tracks := range []bool{false, true} { + t.Run(map[bool]string{false: "SUBSCRIBE_NAMESPACE", true: "SUBSCRIBE_TRACKS"}[tracks], func(t *testing.T) { + t.Parallel() + auth := &tokenRecorder{} + subSess, teardown := connectRelay(t, relay.Config{ + Authorizer: auth, + SessionOptions: []session.Option{session.WithMaxAuthTokenCacheSize(1024)}, + }) + defer teardown() + stream := openNamespaceSub(t, subSess, tracks, ns("a"), tokenParam("t0")) + for _, ps := range []message.Parameters{ + {message.TrackNamespacePrefixParam(ns("b"))}, + {tokenParam("t1")}, + {message.TrackNamespacePrefixParam(ns("c"))}, + {message.TrackNamespacePrefixParam(ns("d")), tokenParam("t2")}, + {message.AuthorizationTokenParam(message.Token{ + AliasType: message.AliasTypeRegister, TokenAlias: 7, TokenType: 1, TokenValue: []byte("t3"), + })}, + {message.TrackNamespacePrefixParam(ns("e")), message.AuthorizationTokenParam(message.Token{ + AliasType: message.AliasTypeDelete, TokenAlias: 7, + })}, + } { + if _, err := subSess.UpdateRequest(t.Context(), stream, ps); err != nil { + t.Fatalf("REQUEST_UPDATE %v: %v", ps, err) + } + } + auth.mu.Lock() + defer auth.mu.Unlock() + want := [][]string{{"t0"}, {"t0"}, {"t1"}, {"t2"}, {"t3"}} + if !slices.EqualFunc(auth.seen, want, slices.Equal) { + t.Fatalf("Authorizer saw tokens %q, want %q", auth.seen, want) + } + }) + } +} diff --git a/pkg/relay/session_handler.go b/pkg/relay/session_handler.go index 2e802eaa..aa48794b 100644 --- a/pkg/relay/session_handler.go +++ b/pkg/relay/session_handler.go @@ -456,14 +456,7 @@ func (h *sessionHandler) rejectExcessiveLoad(ctx context.Context, req *session.R // [moqt.RequestUnauthorized] — maps straight onto the wire reply. Like // rejectAuth, a write failure is logged and otherwise swallowed. func (h *sessionHandler) rejectTokenDenied(ctx context.Context, req *session.Request, denyErr error) { - code := moqt.RequestUnauthorized - reason := denyErr.Error() - if denied, ok := errors.AsType[*session.TokenDeniedError](denyErr); ok { - code = denied.RequestErrorCode() - if denied.Reason != "" { - reason = denied.Reason - } - } + code, reason := tokenDenial(denyErr) h.log.LogAttrs(ctx, slog.LevelDebug, "relay rejecting request on token verification", slog.String("err", denyErr.Error()), slog.Uint64("code", uint64(code))) if err := req.RejectError(code, reason); err != nil && !errors.Is(err, context.Canceled) { @@ -472,6 +465,18 @@ func (h *sessionHandler) rejectTokenDenied(ctx context.Context, req *session.Req } } +// tokenDenial is the REQUEST_ERROR code and reason for a token-verification +// denial (see [sessionHandler.rejectTokenDenied]). +func tokenDenial(denyErr error) (moqt.RequestErrorCode, string) { + if denied, ok := errors.AsType[*session.TokenDeniedError](denyErr); ok { + if denied.Reason != "" { + return denied.RequestErrorCode(), denied.Reason + } + return denied.RequestErrorCode(), denyErr.Error() + } + return moqt.RequestUnauthorized, denyErr.Error() +} + // handleFollowupRequestID validates a peer REQUEST_UPDATE's Request ID — // §10.1: an update consumes an ID from the sender's space, and the readers // that parse follow-ups directly bypass AcceptRequest's checking. A @@ -509,24 +514,27 @@ func (h *sessionHandler) handleRequestUpdateLimit(ctx context.Context, lim *sess // handleFollowupTokens routes a follow-up message's AUTHORIZATION_TOKEN // parameters through the session token cache — §10.2.2 allows REQUEST_UPDATE // to REGISTER or DELETE aliases, and the readers that parse follow-ups -// directly bypass AcceptRequest's processing. Returns false when a token -// fault closed the session, in which case the caller's read loop should -// stop. -func (h *sessionHandler) handleFollowupTokens(ctx context.Context, msg message.Message) bool { - _, err := h.sess.ProcessFollowupTokens(msg) +// directly bypass AcceptRequest's processing. It returns the resolved tokens, +// and ok false when a token fault closed the session, in which case the +// caller's read loop should stop. +func (h *sessionHandler) handleFollowupTokens( + ctx context.Context, + msg message.Message, +) (toks []session.ResolvedToken, ok bool) { + toks, err := h.sess.ProcessFollowupTokens(msg) if err == nil { - return true + return toks, true } if tce, ok := errors.AsType[*session.TokenCacheError](err); ok { h.log.LogAttrs(ctx, slog.LevelDebug, "relay closing session on follow-up token cache error", slog.String("err", err.Error()), slog.Uint64("code", uint64(tce.Code))) _ = h.sess.Close(tce.Code, tce.Error()) - return false + return nil, false } h.log.LogAttrs(ctx, slog.LevelDebug, "follow-up token processing failed", slog.String("err", err.Error())) - return false + return nil, false } // readRequestStream owns all reads on an established request stream: it From 4e6aff5df0ea3f2dd5ee0bbb1aeeb8a83ee0c08b Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 18:08:02 +0500 Subject: [PATCH 3/9] =?UTF-8?q?fix(session,relay):=20a=20truncated=20reque?= =?UTF-8?q?st=20opener=20fails=20only=20its=20request;=20AcceptRequest=20c?= =?UTF-8?q?loses=20on=20=C2=A710.1=20and=20=C2=A710.2.2=20faults?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AcceptRequest returned an error when a request stream was FINed or reset before its first message was complete. RequestMux.Run returned it, the relay's request loop exited, and nothing closed the session: a client that opened a SUBSCRIBE and cancelled it at once left its session open with no one reading its requests. §3.3.2 and §3.3.3 make that the failure or cancellation of one request. AcceptRequest now cancels that stream (CANCELLED) and moves on to the next. A wrong-parity or duplicate Request ID on an opener took the same path, so the relay never closed with INVALID_REQUEST_ID (§10.1). AcceptRequest now closes the session itself: with INVALID_REQUEST_ID for a §10.1 violation, and with the *TokenCacheError's code for a §10.2.2 cache fault, as it already did with PROTOCOL_VIOLATION. It still returns the typed error. This changes the documented contract (the caller no longer has to Close); the relay's own close for token faults is removed. Design chosen with the maintainer. The new and extended tests fail on the unpatched code: AcceptRequest returns "unexpected EOF" / "stream cancelled" for a truncated opener, the relay stops reading requests after one, and every Request ID and token cache test finds the session still open. Co-Authored-By: Claude Opus 5.5 (1M context) --- STATUS.md | 5 --- pkg/moqt/session/example_test.go | 6 +-- pkg/moqt/session/helpers_test.go | 13 ++++-- pkg/moqt/session/malformed_test.go | 49 ++++++++++++++------- pkg/moqt/session/request.go | 38 ++++++++++++----- pkg/moqt/session/request_test.go | 4 ++ pkg/moqt/session/requestmux.go | 11 ++--- pkg/moqt/session/token_verify.go | 5 ++- pkg/moqt/session/token_verify_test.go | 3 ++ pkg/relay/session_handler.go | 19 ++------- pkg/relay/session_handler_test.go | 61 +++++++++++++++++++++++++++ 11 files changed, 151 insertions(+), 63 deletions(-) diff --git a/STATUS.md b/STATUS.md index 1ce71668..d90a5d64 100644 --- a/STATUS.md +++ b/STATUS.md @@ -510,11 +510,6 @@ High: Group 32 or more IDs below the newest is dropped as already delivered and the downstream stream then FINs. §9.4: "MUST NOT reorder or drop objects received on a multi-object stream"; §11.4.3 requires a reset when Objects were not delivered. -- A request stream reset or FINed before its first message is complete ends the - relay's request loop without closing the session (§3.3.2, §3.3.3: that request - fails or is cancelled, nothing more). - A bad Request ID on an opener takes the same path instead of closing the session - with INVALID_REQUEST_ID (§10.1). - A subscriber's cancellation does not reset the subscription's open subgroup streams or fill fetch streams: the relay keeps forwarding, then FINs (§5.1.1: "MUST reset any open streams associated with the SUBSCRIBE"; §5.1.3.1). diff --git a/pkg/moqt/session/example_test.go b/pkg/moqt/session/example_test.go index 0b056aac..800ff8e3 100644 --- a/pkg/moqt/session/example_test.go +++ b/pkg/moqt/session/example_test.go @@ -228,9 +228,9 @@ func ExampleRequestMux() { _ = r.RejectError(moqt.RequestNotSupported, "unsupported request type") }) - // Run returns when ctx is cancelled or AcceptRequest fails. A session-fatal - // error (e.g. *session.ErrDuplicateRequestID) should be escalated by closing - // the session with the mapped code. + // Run returns when ctx is cancelled or AcceptRequest fails; a protocol + // violation (e.g. *session.ErrDuplicateRequestID) has already closed the + // session with the mapped code. _ = mux.Run(ctx, server) } diff --git a/pkg/moqt/session/helpers_test.go b/pkg/moqt/session/helpers_test.go index 2aed7347..99a63b72 100644 --- a/pkg/moqt/session/helpers_test.go +++ b/pkg/moqt/session/helpers_test.go @@ -166,19 +166,24 @@ func requireRefusedOpen(t *testing.T, want string, open func(context.Context) (* // requireClosedProtocolViolation waits for sess to close and checks the code. func requireClosedProtocolViolation(t *testing.T, sess *session.Session) { + t.Helper() + requireClosedCode(t, sess, moqt.SessionProtocolViolation) +} + +// requireClosedCode checks that sess closes itself with want. +func requireClosedCode(t *testing.T, sess *session.Session, want moqt.SessionErrorCode) { t.Helper() select { case <-sess.Done(): case <-time.After(2 * time.Second): - t.Fatal("session stayed open; want PROTOCOL_VIOLATION close") + t.Fatalf("session stayed open; want close with %#x", uint64(want)) } closed, ok := errors.AsType[*session.ClosedError](sess.Err()) if !ok { t.Fatalf("Err() = %v, want a *session.ClosedError", sess.Err()) } - if closed.Code != moqt.SessionProtocolViolation { - t.Errorf("closed with code %#x, want PROTOCOL_VIOLATION (%#x)", - uint64(closed.Code), uint64(moqt.SessionProtocolViolation)) + if closed.Code != want { + t.Errorf("closed with code %#x, want %#x", uint64(closed.Code), uint64(want)) } } diff --git a/pkg/moqt/session/malformed_test.go b/pkg/moqt/session/malformed_test.go index 393c32bc..6f70da00 100644 --- a/pkg/moqt/session/malformed_test.go +++ b/pkg/moqt/session/malformed_test.go @@ -66,24 +66,43 @@ func TestMalformedOpenerClosesSession(t *testing.T) { } } -// TestTruncatedOpenerResetsOnlyStream: a stream ending before its first frame -// is complete is the peer giving up, not a Length mismatch. +// TestTruncatedOpenerResetsOnlyStream: a stream ending or reset before its +// first frame is complete is the peer giving up on that request (§3.3.2, +// §3.3.3), not a Length mismatch: AcceptRequest moves on to the next request. func TestTruncatedOpenerResetsOnlyStream(t *testing.T) { t.Parallel() - _, server, cliConn, _ := openPairWithConns(t) - stream, err := cliConn.OpenStream() - if err != nil { - t.Fatalf("OpenStream: %v", err) - } - go func() { - // Type SUBSCRIBE, Length 16, then only two body bytes and a FIN. - _, _ = stream.Write([]byte{byte(message.TypeSubscribe), 0x00, 0x10, 0x00, 0x00}) - _ = stream.Close() - }() - if _, err := server.AcceptRequest(t.Context()); err == nil { - t.Fatal("AcceptRequest accepted a truncated opener") + for _, tc := range []struct { + name string + end func(session.Stream) + }{ + {"FIN", func(s session.Stream) { _ = s.Close() }}, + {"reset", func(s session.Stream) { s.CancelWrite(0) }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + client, server, cliConn, _ := openPairWithConns(t) + stream, err := cliConn.OpenStream() + if err != nil { + t.Fatalf("OpenStream: %v", err) + } + go func() { + // Type SUBSCRIBE, Length 16, then only two body bytes. + _, _ = stream.Write([]byte{byte(message.TypeSubscribe), 0x00, 0x10, 0x00, 0x00}) + tc.end(stream) + _, _ = session.OpenRequestForTest(client, &message.Subscribe{ + RequestID: 0, Namespace: videoNS, Name: []byte("next"), + }) + }() + req, err := server.AcceptRequest(t.Context()) + if err != nil { + t.Fatalf("AcceptRequest: %v, want the request after the truncated one", err) + } + if name := string(req.First.(*message.Subscribe).Name); name != "next" { + t.Fatalf("accepted %q, want the request after the truncated one", name) + } + requireStaysOpen(t, server, 100*time.Millisecond) + }) } - requireStaysOpen(t, server, 100*time.Millisecond) } func TestMalformedResponseClosesSession(t *testing.T) { diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index 52c83882..c5927d86 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -15,9 +15,10 @@ import ( "github.com/floatdrop/moq-go/pkg/moqt/track" ) -// ErrRequestIDParityViolation is returned by AcceptRequest when the peer sends -// a Request ID whose parity does not match the expected value per §10.1. -// The caller MUST close the session with SessionInvalidRequestID. +// ErrRequestIDParityViolation is returned by [Session.CheckPeerRequestID] when +// the peer sends a Request ID whose parity does not match the expected value +// per §10.1. [Session.AcceptRequest] has already closed the session with +// INVALID_REQUEST_ID; another caller of CheckPeerRequestID MUST. type ErrRequestIDParityViolation struct { RequestID uint64 ExpectedEven bool // true = expected even (peer is client), false = expected odd (peer is server) @@ -40,7 +41,8 @@ func (e *ErrRequestIDParityViolation) Error() string { // Request ID" MUST close the session with INVALID_REQUEST_ID). Cross-stream // delivery reordering is tolerated — an ID below the high-water mark counts // as a duplicate only once every unseen ID it could have been is accounted -// for. The caller MUST close the session with SessionInvalidRequestID. +// for. [Session.AcceptRequest] has already closed the session with +// INVALID_REQUEST_ID; another caller of CheckPeerRequestID MUST. type ErrDuplicateRequestID struct { RequestID uint64 MaxSeen uint64 @@ -239,8 +241,11 @@ type Request struct { // malformed (§10, wrapping [message.ErrMalformedMessage]), closes the session // with PROTOCOL_VIOLATION; the error is *ErrUnexpectedRequestOpener, // *ErrUnexpectedRequestUpdate, ErrUnexpectedPublishStateNotify or the parse -// error. A stream that ends before its first message is complete only resets -// that stream. +// error. A Request ID violation (§10.1) closes it with INVALID_REQUEST_ID and +// returns *ErrRequestIDParityViolation or *ErrDuplicateRequestID, and a token +// cache fault (§10.2.2) with the *TokenCacheError's Code. A stream that ends +// or is reset before its first message is complete fails only that request +// (§3.3.2, §3.3.3): it is reset and AcceptRequest moves on. func (s *Session) AcceptRequest(ctx context.Context) (*Request, error) { for { stream, err := s.conn.AcceptStream(ctx) @@ -252,16 +257,24 @@ func (s *Session) AcceptRequest(ctx context.Context) (*Request, error) { // accept loop past cancellation. msg, err := s.readResponse(ctx, stream) if err != nil { - resetStream(stream) if ctx.Err() != nil { + resetStream(stream) return nil, ctx.Err() } // §3.3: readResponse already closed the session; this only shapes // the error. if typ, ok := errors.AsType[message.ErrUnknownType](err); ok { + resetStream(stream) return nil, s.closeProtocolViolation(&ErrUnexpectedRequestOpener{Type: message.Type(typ)}) } - return nil, fmt.Errorf("moqt/session: parse request first message: %w", err) + if errors.Is(err, message.ErrMalformedMessage) { + resetStream(stream) + return nil, fmt.Errorf("moqt/session: parse request first message: %w", err) + } + // §3.3.2, §3.3.3: the peer ended or reset the stream first, which + // fails that request only. A closed session fails AcceptStream. + cancelRequest(stream) + continue } // §10.9, §3.3: REQUEST_UPDATE never opens a stream. @@ -295,16 +308,20 @@ func (s *Session) AcceptRequest(ctx context.Context) (*Request, error) { if m, ok := msg.(message.WithRequestID); ok { if err := s.CheckPeerRequestID(m.GetRequestID()); err != nil { resetStream(stream) + _ = s.Close(moqt.SessionInvalidRequestID, err.Error()) return nil, err } } // §10.2.2: REGISTER tokens commit before any rejection, so the alias // persists even if the request fails. A *TokenCacheError is - // session-fatal; the caller closes the session with its Code. + // session-fatal. tokens, err := s.processRequestTokens(msg) if err != nil { resetStream(stream) + if tce, ok := errors.AsType[*TokenCacheError](err); ok { + _ = s.Close(tce.Code, tce.Error()) + } return nil, err } @@ -350,8 +367,7 @@ func evictLowestGapsLocked(gaps map[uint64]struct{}, n int) { // for those). // // Two violations are session-fatal per §10.1, and the caller MUST close the -// session with [moqt.SessionInvalidRequestID] (AcceptRequest instead returns -// the error to its caller, which owns that decision): +// session with [moqt.SessionInvalidRequestID] (AcceptRequest does so itself): // // - wrong parity for the sender (*ErrRequestIDParityViolation); // - a duplicate ID (*ErrDuplicateRequestID). diff --git a/pkg/moqt/session/request_test.go b/pkg/moqt/session/request_test.go index 8358b78c..17ea51a6 100644 --- a/pkg/moqt/session/request_test.go +++ b/pkg/moqt/session/request_test.go @@ -237,6 +237,7 @@ func TestAcceptRequestDuplicateID(t *testing.T) { if dupErr.RequestID != 0 { t.Errorf("ErrDuplicateRequestID.RequestID = %d, want 0", dupErr.RequestID) } + requireClosedCode(t, server, moqt.SessionInvalidRequestID) } // TestAcceptRequestOutOfOrderID verifies §10.1's receiver rules under @@ -330,6 +331,7 @@ func TestAcceptRequestOutOfOrderID(t *testing.T) { if dupErr.MaxSeen != 4 { t.Errorf("ErrDuplicateRequestID.MaxSeen = %d, want 4", dupErr.MaxSeen) } + requireClosedCode(t, server, moqt.SessionInvalidRequestID) } // TestAcceptRequestMonotonicHappyPath verifies that multiple requests with @@ -428,6 +430,7 @@ func TestAcceptRequestParityViolation_ServerReceivesOddID(t *testing.T) { if !parityErr.ExpectedEven { t.Errorf("ErrRequestIDParityViolation.ExpectedEven = false, want true (server expects even IDs from client)") } + requireClosedCode(t, server, moqt.SessionInvalidRequestID) } // TestAcceptRequestParityViolation_ClientReceivesEvenID verifies that when the @@ -476,6 +479,7 @@ func TestAcceptRequestParityViolation_ClientReceivesEvenID(t *testing.T) { if parityErr.ExpectedEven { t.Errorf("ErrRequestIDParityViolation.ExpectedEven = true, want false (client expects odd IDs from server)") } + requireClosedCode(t, client, moqt.SessionInvalidRequestID) } // TestAcceptRequestParityHappyPath verifies that correct-parity IDs are diff --git a/pkg/moqt/session/requestmux.go b/pkg/moqt/session/requestmux.go index b908f970..3135ba6c 100644 --- a/pkg/moqt/session/requestmux.go +++ b/pkg/moqt/session/requestmux.go @@ -86,14 +86,9 @@ func (m *RequestMux) OnUnknown(f func(*Request)) { // until ctx is cancelled or [Session.AcceptRequest] returns an error, which Run // returns. // -// Some AcceptRequest errors are session-fatal protocol violations — a §10.1 -// Request-ID parity/monotonicity violation (*ErrRequestIDParityViolation / -// *ErrDuplicateRequestID) or a token-cache fault (*TokenCacheError) — that the -// caller MUST escalate by closing the session with the mapped code (see -// [Session.AcceptRequest]). Run surfaces the error unchanged so the caller can -// inspect it with errors.As and Close accordingly. A request stream opened by -// anything but a request message (§3.3, *ErrUnexpectedRequestOpener and -// friends) arrives with the session already closed. +// Run surfaces an AcceptRequest error unchanged. Any but ctx's means the +// session has ended: a protocol violation (§3.3, §10, §10.1, §10.2.2) arrives +// already closed with the mapped code (see [Session.AcceptRequest]). // // Dispatch is synchronous: a handler runs to completion before Run accepts the // next request, mirroring a hand-written accept loop and [Demux.Run]. A handler diff --git a/pkg/moqt/session/token_verify.go b/pkg/moqt/session/token_verify.go index 01fbcfa9..e80ee3f5 100644 --- a/pkg/moqt/session/token_verify.go +++ b/pkg/moqt/session/token_verify.go @@ -65,8 +65,9 @@ func (f TokenVerifierFunc) VerifyToken(ctx context.Context, sess *Session, tok R // inbound request's AUTHORIZATION_TOKEN parameters fails at the cache layer // (§10.2.2). These are session-level faults: a malformed token, a duplicate // REGISTER alias, a cache overflow, or a USE_ALIAS / DELETE referencing an -// unknown alias. Code is the SESSION_ERROR the caller should close the -// session with. +// unknown alias. Code is the SESSION_ERROR the session is closed with: +// AcceptRequest has already done so, and a caller of +// [Session.ProcessFollowupTokens] MUST. type TokenCacheError struct { // Code is the §10.2.2 SESSION_ERROR code to terminate the session with. Code moqt.SessionErrorCode diff --git a/pkg/moqt/session/token_verify_test.go b/pkg/moqt/session/token_verify_test.go index 308d309e..23da3611 100644 --- a/pkg/moqt/session/token_verify_test.go +++ b/pkg/moqt/session/token_verify_test.go @@ -137,6 +137,7 @@ func TestAcceptRequestDuplicateAliasIsSessionError(t *testing.T) { if tce.Code != moqt.SessionDuplicateAuthTokenAlias { t.Errorf("Code = 0x%X, want SessionDuplicateAuthTokenAlias", uint64(tce.Code)) } + requireClosedCode(t, server, moqt.SessionDuplicateAuthTokenAlias) } // TestAcceptRequestUnknownAliasIsSessionError verifies that USE_ALIAS for an @@ -159,6 +160,7 @@ func TestAcceptRequestUnknownAliasIsSessionError(t *testing.T) { if tce.Code != moqt.SessionUnknownAuthTokenAlias { t.Errorf("Code = 0x%X, want SessionUnknownAuthTokenAlias", uint64(tce.Code)) } + requireClosedCode(t, server, moqt.SessionUnknownAuthTokenAlias) } // TestAcceptRequestRegisterPersistsWhenAliasingProhibited verifies that with @@ -184,6 +186,7 @@ func TestAcceptRequestRegisterProhibitedIsOverflow(t *testing.T) { if tce.Code != moqt.SessionAuthTokenCacheOverflow { t.Errorf("Code = 0x%X, want SessionAuthTokenCacheOverflow", uint64(tce.Code)) } + requireClosedCode(t, server, moqt.SessionAuthTokenCacheOverflow) } // TestVerifyRequestTokensAllow verifies that a verifier returning nil diff --git a/pkg/relay/session_handler.go b/pkg/relay/session_handler.go index aa48794b..7bcd7bde 100644 --- a/pkg/relay/session_handler.go +++ b/pkg/relay/session_handler.go @@ -250,22 +250,11 @@ func (h *sessionHandler) run(ctx context.Context) error { // - the session emits an unrecoverable error from AcceptRequest, // - a non-shutdown read failure occurs. // -// Per-request failures (auth, rejected requests) do NOT terminate the loop. -// A stream opened by anything but a request message is session-fatal (§3.3); -// AcceptRequest has already closed the session. +// Per-request failures (auth, rejected requests, a stream ended before its +// request) do NOT terminate the loop. A protocol violation does, and +// AcceptRequest has already closed the session with its code. func (h *sessionHandler) runRequestLoop(ctx context.Context) error { - err := h.requestMux(ctx).Run(ctx, h.sess) - // A malformed / duplicate / overflowing / unknown AUTHORIZATION_TOKEN alias - // surfaces from AcceptRequest as a session-level fault per §10.2.2: close the - // session with the mapped SESSION_ERROR code rather than just tearing down - // the request loop. - if tce, ok := errors.AsType[*session.TokenCacheError](err); ok { - h.log.LogAttrs(ctx, slog.LevelDebug, "relay closing session on token cache error", - slog.String("err", err.Error()), - slog.Uint64("code", uint64(tce.Code))) - _ = h.sess.Close(tce.Code, tce.Error()) - } - return err + return h.requestMux(ctx).Run(ctx, h.sess) } // runDataLoop accepts inbound data streams and routes each by type: subgroup diff --git a/pkg/relay/session_handler_test.go b/pkg/relay/session_handler_test.go index c7f7b39e..192bd63e 100644 --- a/pkg/relay/session_handler_test.go +++ b/pkg/relay/session_handler_test.go @@ -5,6 +5,7 @@ import ( "errors" "sync/atomic" "testing" + "time" "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" @@ -224,3 +225,63 @@ func (a *denyAuthorizer) AuthorizeTrackStatus(context.Context, *session.Session, a.trackStatusCalls.Add(1) return a.err } + +// TestSessionHandler_TruncatedOpenerKeepsServing: a request stream that ends +// or is reset before its first message is complete fails that request only +// (§3.3.2, §3.3.3); the relay goes on serving the session. +func TestSessionHandler_TruncatedOpenerKeepsServing(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + end func(session.Stream) + }{ + {"FIN", func(s session.Stream) { _ = s.Close() }}, + {"reset", func(s session.Stream) { s.CancelWrite(0) }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + l := newPipeListener() + _, teardown := connectRelayOn(t, relay.Config{}, l) + defer teardown() + peer, conn := dialRaw(t, l) + stream, err := conn.OpenStream() + if err != nil { + t.Fatalf("OpenStream: %v", err) + } + // Type SUBSCRIBE, Length 16, then only two body bytes. + _, _ = stream.Write([]byte{byte(message.TypeSubscribe), 0x00, 0x10, 0x00, 0x00}) + tc.end(stream) + + // The pipe transport blocks the opener's write until the relay + // reads it, which no context bounds, so wait here instead. + errc := make(chan error, 1) + go func() { + _, err := peer.Subscribe(t.Context(), &message.Subscribe{Namespace: ns("video"), Name: []byte("cam1")}) + errc <- err + }() + select { + case err := <-errc: + requireRejectedWithCode(t, err, moqt.RequestDoesNotExist) + case <-time.After(2 * time.Second): + t.Fatal("relay stopped reading requests after the truncated one") + } + }) + } +} + +// TestSessionHandler_BadRequestIDClosesSession: a request opener with a +// wrong-parity Request ID closes the session (§10.1). +func TestSessionHandler_BadRequestIDClosesSession(t *testing.T) { + t.Parallel() + l := newPipeListener() + _, teardown := connectRelayOn(t, relay.Config{}, l) + defer teardown() + peer, conn := dialRaw(t, l) + stream, err := conn.OpenStream() + if err != nil { + t.Fatalf("OpenStream: %v", err) + } + // A client's Request IDs are even. + _ = message.Marshal(stream, &message.Subscribe{RequestID: 1, Namespace: ns("video"), Name: []byte("cam1")}) + requireSessionClosed(t, peer, "a SUBSCRIBE with an odd Request ID from a client") +} From cd91e77538346216ebc723fd17cae96e45f99535 Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 18:25:45 +0500 Subject: [PATCH 4/9] =?UTF-8?q?fix(relay):=20count=20the=20dedup=20window?= =?UTF-8?q?=20in=20Groups,=20and=20never=20FIN=20a=20stream=20missing=20an?= =?UTF-8?q?=20aged-out=20Object=20(=C2=A79.4,=20=C2=A711.4.3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The relay's §9.3 dedup ledger held Groups within 32 IDs of the newest and took an Object of any older Group for one already forwarded. Group IDs need not be consecutive (§2.3.1, §12.8), so a publisher that jumped 32 IDs, timestamp IDs for one, had the rest of any still-open Subgroup dropped, and the downstream stream then FINed: with END_OF_GROUP set, that FIN says the dropped Objects do not exist. §9.4: a relay "MUST NOT reorder or drop objects received on a multi-object stream". The ledger now holds the 32 most recent distinct Groups, pruning the lowest ID first (the doc marks the assumption that IDs mostly increase), and ClaimDelivered returns a Claim: Fresh, Redundant, or AgedOut when the Group is below every one held. For a subgroup Object, AgedOut is decided against the Subgroup's writer set: - above every Object forwarded through the set: forwarded, since it can neither repeat nor reorder one (§2.2); this covers a single upstream's open stream; - an end of the run of IDs forwarded, or inside an unbroken run: a known repeat, dropped like a Redundant one; - otherwise: dropped, and each stream it would have gone on is marked incomplete as admit would mark it (INTERNAL_ERROR where it was owed, the filter's own handling where it was not), so it resets rather than FINs. That drop is marked as a Deviation from §9.4 in the code. An aged-out datagram is dropped as a redundant one; it has no stream. admit's skip handling moves to subgroupWriter.skip, shared by both paths, and recordRedundant factors the §9.1 duplicate path out of the fanout and datagram handlers. The four new relay tests fail on the unpatched code: the Object after a Group ID jump and the next Object 32 Groups later are dropped and the stream FINs, and an Object below the run is dropped with a FIN instead of a reset. Mutation checks confirm the owed-only marking and the known-repeat case. The reset code is not asserted: sessiontest's reset error carries none. bench-quick allocs/op are unchanged. Design chosen with the maintainer (count the window, reset on a drop); the forward-above-the-run and known-repeat refinements came out of review. Co-Authored-By: Claude Opus 5.5 (1M context) --- STATUS.md | 4 - pkg/relay/dedup_window_test.go | 247 +++++++++++++++++++++ pkg/relay/handler_datagram.go | 12 +- pkg/relay/handler_duplicate.go | 13 ++ pkg/relay/handler_fanout.go | 115 ++++++++-- pkg/relay/internal/registry/ledger_test.go | 135 +++++++---- pkg/relay/internal/registry/track_entry.go | 120 +++++----- 7 files changed, 524 insertions(+), 122 deletions(-) create mode 100644 pkg/relay/dedup_window_test.go diff --git a/STATUS.md b/STATUS.md index d90a5d64..5fc9eee0 100644 --- a/STATUS.md +++ b/STATUS.md @@ -506,10 +506,6 @@ High: a reset subgroup stream or a subgroup still in flight leaves such holes. §10.13: a relay that meets an uncached Object of unknown status "MUST pause subsequent delivery until it has confirmed the object's status upstream". -- The relay's 32-Group dedup window measures Group ID distance, so an Object of a - Group 32 or more IDs below the newest is dropped as already delivered and the - downstream stream then FINs. §9.4: "MUST NOT reorder or drop objects received on - a multi-object stream"; §11.4.3 requires a reset when Objects were not delivered. - A subscriber's cancellation does not reset the subscription's open subgroup streams or fill fetch streams: the relay keeps forwarding, then FINs (§5.1.1: "MUST reset any open streams associated with the SUBSCRIBE"; §5.1.3.1). diff --git a/pkg/relay/dedup_window_test.go b/pkg/relay/dedup_window_test.go new file mode 100644 index 00000000..6560f159 --- /dev/null +++ b/pkg/relay/dedup_window_test.go @@ -0,0 +1,247 @@ +package relay_test + +import ( + "errors" + "io" + "slices" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" +) + +// The relay dedups Objects from redundant upstreams against the last 32 +// Groups (§9.3). Past that it forwards an Object only above every one it +// forwarded in its Subgroup; one it drops leaves its stream missing an Object, +// so that stream is reset, not FINed (§11.4.3). + +// readSubgroupsConcurrently reads every subgroup stream sess accepts to its +// end, each on its own goroutine, so an open stream does not hold up the +// others. +func readSubgroupsConcurrently(t *testing.T, sess *session.Session) <-chan subgroupRead { + t.Helper() + out := make(chan subgroupRead, 64) + go func() { + for { + ds, err := sess.AcceptDataStream(t.Context()) + if err != nil { + return + } + in, ok := ds.(*session.IncomingSubgroupStream) + if !ok { + continue + } + go func() { + r := subgroupRead{header: in.Header} + for { + o, err := in.ReadDecoded() + if err != nil { + r.end = err + out <- r + return + } + r.ids = append(r.ids, o.ObjectID) + } + }() + } + }() + return out +} + +// awaitGroupStream returns the first stream of group read from reads, +// failing after 5s. +func awaitGroupStream(t *testing.T, reads <-chan subgroupRead, group uint64) subgroupRead { + t.Helper() + deadline := time.After(5 * time.Second) + for { + select { + case r := <-reads: + if r.header.GroupID == group { + return r + } + case <-deadline: + t.Fatalf("no stream of Group %d ended within 5s", group) + } + } +} + +// writeOpenGroup0 opens Group 0's subgroup (END_OF_GROUP set, so its FIN also +// ends the Group) and writes Objects ids, leaving it open. +func writeOpenGroup0( + t *testing.T, + pubSess *session.Session, + alias uint64, + ids ...uint64, +) *session.OutgoingSubgroupStream { + t.Helper() + hdr := subgroupHeader(alias, 0) + hdr.EndOfGroup = true + sg, err := openSubgroupWaiting(t, pubSess, hdr) + if err != nil { + t.Fatalf("OpenSubgroup: %v", err) + } + writeIDs(t, sg, -1, ids...) + return sg +} + +// writeIDs writes Objects ids (ascending) on sg after Object prev, -1 for +// none (§11.4.2 delta encoding). +func writeIDs(t *testing.T, sg *session.OutgoingSubgroupStream, prev int64, ids ...uint64) { + t.Helper() + for _, id := range ids { + delta := id + if prev >= 0 { + delta = id - uint64(prev) - 1 + } + prev = int64(id) + if err := sg.WriteObject(&message.SubgroupObject{ObjectIDDelta: delta, Payload: []byte("x")}); err != nil { + t.Fatalf("WriteObject(%d): %v", id, err) + } + } +} + +// publishGroups publishes one Object in each of Groups 1..n, waiting for each +// to reach the subscriber. +func publishGroups(t *testing.T, pubSess *session.Session, alias uint64, reads <-chan subgroupRead, n uint64) { + t.Helper() + for g := uint64(1); g <= n; g++ { + publishObjects(t, pubSess, alias, g, 1) + awaitGroupStream(t, reads, g) + } +} + +// TestFanout_GroupIDJumpKeepsOpenSubgroup: Group IDs need not be consecutive +// (§2.3.1, §12.8), so a jump to Group 100 while Group 0's stream is still open +// does not age Group 0 out; its later Objects are forwarded. +func TestFanout_GroupIDJumpKeepsOpenSubgroup(t *testing.T) { + t.Parallel() + pubSess, alias := newCam1Publisher(t, nil) + subSess := dialAnotherClient(t, pubSess) + subscribeCam1(t, subSess) + reads := readSubgroupsConcurrently(t, subSess) + + sg := writeOpenGroup0(t, pubSess, alias, 0, 1) + publishObjects(t, pubSess, alias, 100, 1) + awaitGroupStream(t, reads, 100) + writeIDs(t, sg, 1, 2) + if err := sg.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + r := awaitGroupStream(t, reads, 0) + if !slices.Equal(r.ids, []uint64{0, 1, 2}) || !errors.Is(r.end, io.EOF) { + t.Fatalf("Group 0 stream carried %v and ended with %v, want [0 1 2] and a FIN", r.ids, r.end) + } +} + +// TestFanout_AgedOutNextObjectForwarded: once 32 newer Groups push Group 0 +// out of the dedup window, the next Object on its open stream is still +// forwarded: it is above every Object forwarded in its Subgroup, so it +// repeats none (§9.4: "MUST NOT reorder or drop objects received on a +// multi-object stream"). +func TestFanout_AgedOutNextObjectForwarded(t *testing.T) { + t.Parallel() + pubSess, alias := newCam1Publisher(t, nil) + subSess := dialAnotherClient(t, pubSess) + subscribeCam1(t, subSess) + reads := readSubgroupsConcurrently(t, subSess) + + sg := writeOpenGroup0(t, pubSess, alias, 0, 1) + publishGroups(t, pubSess, alias, reads, 32) + writeIDs(t, sg, 1, 2) + if err := sg.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + r := awaitGroupStream(t, reads, 0) + if !slices.Equal(r.ids, []uint64{0, 1, 2}) || !errors.Is(r.end, io.EOF) { + t.Fatalf("Group 0 stream carried %v and ended with %v, want [0 1 2] and a FIN", r.ids, r.end) + } +} + +// TestFanout_AgedOutObjectResetsStream: an aged-out Object below one already +// forwarded in its Subgroup (here Object 1, from a second stream, after 0 and +// 2) may be a repeat the relay can no longer detect, so it is dropped; the +// stream missing it is reset rather than FINed, which with END_OF_GROUP would +// say it does not exist (§11.4.2, §11.4.3). A subscription whose Start +// Location is past the dropped Object was not owed it, so its stream FINs. +func TestFanout_AgedOutObjectResetsStream(t *testing.T) { + t.Parallel() + pubSess, alias := newCam1Publisher(t, nil) + subSess := dialAnotherClient(t, pubSess) + subscribeCam1(t, subSess) + reads := readSubgroupsConcurrently(t, subSess) + lateSess := dialAnotherClient(t, pubSess) + subscribeCam1(t, lateSess, message.AbsoluteStartFilter(message.Location{Group: 0, Object: 2})) + lateReads := readSubgroupsConcurrently(t, lateSess) + // Its OBJECTID_FILTER omits Object 1, which resets its stream as it would + // had Object 1 been forwarded (§11.4.3). + filteredSess := dialAnotherClient(t, pubSess) + subscribeCam1(t, filteredSess, message.RangeFilterParam(&message.RangeFilter{ + Type: message.ParamObjectIDFilter, Ranges: []message.Range{{Start: 0, End: 0}, {Start: 2, End: 2}}, + })) + filteredReads := readSubgroupsConcurrently(t, filteredSess) + + first := writeOpenGroup0(t, pubSess, alias, 0, 2) + publishGroups(t, pubSess, alias, reads, 32) + hdr := subgroupHeader(alias, 0) + hdr.EndOfGroup, hdr.ReplayingSubgroup = true, true + second, err := openSubgroupWaiting(t, pubSess, hdr) + if err != nil { + t.Fatalf("OpenSubgroup: %v", err) + } + writeIDs(t, second, -1, 1) + for _, sg := range []*session.OutgoingSubgroupStream{second, first} { + if err := sg.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + } + + r := awaitGroupStream(t, reads, 0) + if !slices.Equal(r.ids, []uint64{0, 2}) || errors.Is(r.end, io.EOF) { + t.Fatalf("Group 0 stream carried %v and ended with %v, want [0 2] and a reset", r.ids, r.end) + } + late := awaitGroupStream(t, lateReads, 0) + if !slices.Equal(late.ids, []uint64{2}) || !errors.Is(late.end, io.EOF) { + t.Fatalf("Group 0 stream from {0, 2} carried %v and ended with %v, want [2] and a FIN", late.ids, late.end) + } + filtered := awaitGroupStream(t, filteredReads, 0) + if !slices.Equal(filtered.ids, []uint64{0, 2}) || errors.Is(filtered.end, io.EOF) { + t.Fatalf("filtered Group 0 stream carried %v and ended with %v, want [0 2] and a reset", + filtered.ids, filtered.end) + } +} + +// TestFanout_AgedOutRepeatDropped: an aged-out Object this Subgroup already +// forwarded, here Objects 1 and 2 again from a second stream, is a redundant +// copy (§9.3): dropped without resetting the streams that got the first. +func TestFanout_AgedOutRepeatDropped(t *testing.T) { + t.Parallel() + pubSess, alias := newCam1Publisher(t, nil) + subSess := dialAnotherClient(t, pubSess) + subscribeCam1(t, subSess) + reads := readSubgroupsConcurrently(t, subSess) + + first := writeOpenGroup0(t, pubSess, alias, 0, 1, 2) + publishGroups(t, pubSess, alias, reads, 32) + hdr := subgroupHeader(alias, 0) + hdr.EndOfGroup, hdr.ReplayingSubgroup = true, true + second, err := openSubgroupWaiting(t, pubSess, hdr) + if err != nil { + t.Fatalf("OpenSubgroup: %v", err) + } + writeIDs(t, second, -1, 1, 2) + if err := second.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + writeIDs(t, first, 2, 3) + if err := first.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + r := awaitGroupStream(t, reads, 0) + if !slices.Equal(r.ids, []uint64{0, 1, 2, 3}) || !errors.Is(r.end, io.EOF) { + t.Fatalf("Group 0 stream carried %v and ended with %v, want [0 1 2 3] and a FIN", r.ids, r.end) + } +} diff --git a/pkg/relay/handler_datagram.go b/pkg/relay/handler_datagram.go index 8e1e9cc8..db08c939 100644 --- a/pkg/relay/handler_datagram.go +++ b/pkg/relay/handler_datagram.go @@ -72,13 +72,15 @@ func (h *sessionHandler) handleDatagram(ctx context.Context, d *message.ObjectDa EndOfGroup: d.HasEndOfGroup(), Gaps: message.ObjectPriorGaps(d.Properties), } - fresh, err := entry.ClaimDelivered(info) + claim, err := entry.ClaimDelivered(info) if err != nil { h.endMalformedTrack(ctx, entry, h.sess, err) return } - if !fresh { - if err := checkDuplicate(entry.Cache, &cache.CachedObject{ + // An aged-out datagram is dropped like a redundant one: it has no stream + // to reset (§9.4 covers multi-object streams). + if claim != registry.ClaimFresh { + if err := recordRedundant(entry, info, &cache.CachedObject{ GroupID: d.GroupID, ObjectID: d.ObjectID, PublisherPriority: d.PublisherPriority, @@ -88,10 +90,6 @@ func (h *sessionHandler) handleDatagram(ctx context.Context, d *message.ObjectDa Payload: d.ObjectPayload, }); err != nil { h.endMalformedTrack(ctx, entry, h.sess, err) - return - } - if err := entry.RecordDuplicate(info); err != nil { - h.endMalformedTrack(ctx, entry, h.sess, err) } return } diff --git a/pkg/relay/handler_duplicate.go b/pkg/relay/handler_duplicate.go index c30bcb63..5e63c213 100644 --- a/pkg/relay/handler_duplicate.go +++ b/pkg/relay/handler_duplicate.go @@ -7,8 +7,21 @@ import ( "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/session" "github.com/floatdrop/moq-go/pkg/relay/cache" + "github.com/floatdrop/moq-go/pkg/relay/internal/registry" ) +// recordRedundant handles dup, a copy [registry.TrackEntry.ClaimDelivered] +// found redundant, described to the ledger by info: it is checked against the +// first copy ([checkDuplicate]) and then recorded +// ([registry.TrackEntry.RecordDuplicate]). An error wraps +// [session.ErrMalformedTrack]. +func recordRedundant(entry *registry.TrackEntry, info registry.ObjectInfo, dup *cache.CachedObject) error { + if err := checkDuplicate(entry.Cache, dup); err != nil { + return err + } + return entry.RecordDuplicate(info) +} + // checkDuplicate compares dup, a copy that lost the §9.3 dedup claim, with the // first copy in c. A different Forwarding Preference, Subgroup ID, Priority or // Payload (§9.1), or different Immutable Properties (§2.4.2, §12.7; one copy diff --git a/pkg/relay/handler_fanout.go b/pkg/relay/handler_fanout.go index 0e8a02f5..e8a769f1 100644 --- a/pkg/relay/handler_fanout.go +++ b/pkg/relay/handler_fanout.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "log/slog" + "math" "sync" "sync/atomic" "time" @@ -71,6 +72,11 @@ type subgroupWriterSet struct { // within the ledger's window. lowest uint64 forwarded bool + // runLo and runHi are the lowest and highest Object IDs forwarded + // through this set, if hasRun; unbroken reports that every ID between + // them was (see [subgroupWriterSet.admitAgedOut]). + runLo, runHi uint64 + hasRun, unbroken bool // sawClean records that some contributor ended cleanly, so the merged // stream FINs even if a peer reset; resetCode is used only when every @@ -87,9 +93,63 @@ func (s *subgroupWriterSet) claimFirst(objectID uint64, claimed bool) bool { if lowest { s.lowest, s.forwarded = objectID, true } + switch { + case !s.hasRun: + s.runLo, s.runHi, s.hasRun, s.unbroken = objectID, objectID, true, true + case s.runHi < math.MaxUint64 && objectID == s.runHi+1: + s.runHi = objectID + case s.runLo > 0 && objectID == s.runLo-1: + s.runLo = objectID + default: + s.runLo, s.runHi, s.unbroken = min(s.runLo, objectID), max(s.runHi, objectID), false + } return claimed && lowest } +// admitAgedOut reports whether an Object at objectID of the Subgroup hdr +// names, which [registry.TrackEntry.ClaimDelivered] returned as claim, may be +// forwarded. Only a [registry.ClaimAgedOut] one may not: with its Group out of +// the dedup window, it goes on only above every Object forwarded through this +// set, where it can neither repeat one nor arrive out of order (§2.2). That +// covers a single upstream's open stream. One this set is known to have +// forwarded (an end of its run, or inside an unbroken one) is a redundant +// copy, dropped as [registry.ClaimRedundant] would be. +// +// Any other is dropped, and each stream it would have gone on is marked +// incomplete as [subgroupWriter.admit] would mark it, so it resets rather +// than FIN (§11.4.3). Deviation: §9.4 says a relay "MUST NOT reorder or drop +// objects received on a multi-object stream"; the relay drops such an Object +// rather than risk forwarding a duplicate it can no longer detect (§9.3). A +// Subgroup whose Object IDs are not consecutive (its Group split across +// Subgroups) has no unbroken run, so there a redundant copy resets too. +// Callers hold sg.Mu. +func (s *subgroupWriterSet) admitAgedOut( + claim registry.Claim, + hdr message.SubgroupHeader, + objectID uint64, + props []byte, +) bool { + switch { + case claim != registry.ClaimAgedOut, s.hasRun && objectID > s.runHi: + return true + case s.hasRun && (objectID == s.runLo || objectID == s.runHi || + s.unbroken && s.runLo < objectID && objectID < s.runHi): + return false + } + for _, w := range s.writers { + if w == nil { + continue + } + v := w.sub.ForwardDecision(hdr.GroupID, objectID, hdr.SubgroupID, hdr.PublisherPriority, props) + if v == registry.Forward { + w.markIncomplete(moqt.StreamResetInternalError) + } else { + w.skip(v, objectID) + } + } + return false +} + // resolveImplicitSubgroupID handles §11.4.2 SUBGROUP_ID_MODE 0b01 (Subgroup ID // = first Object ID): it reads the first object and rewrites hdr to the // explicit form. The returned pending object must be processed as the @@ -362,13 +422,13 @@ func (h *sessionHandler) runFanout(ctx context.Context, stream *session.Incoming Status: obj.ObjectStatus, Gaps: message.ObjectPriorGaps(obj.Properties), } - fresh, err := entry.ClaimDelivered(info) + claim, err := entry.ClaimDelivered(info) if err != nil { malformed(err) return } - if !fresh { - if err := checkDuplicate(entry.Cache, &cache.CachedObject{ + if claim == registry.ClaimRedundant { + if err := recordRedundant(entry, info, &cache.CachedObject{ GroupID: hdr.GroupID, ObjectID: objectID, SubgroupID: hdr.SubgroupID, @@ -381,19 +441,19 @@ func (h *sessionHandler) runFanout(ctx context.Context, stream *session.Incoming malformed(err) return } - if err := entry.RecordDuplicate(info); err != nil { - malformed(err) - return - } continue // redundant copy already forwarded by a peer upstream. } - // Counted after the dedup claim, so redundant copies don't count. - h.metrics.ObjectReceived(ref, hdr.SubgroupID) - // Under sg.Mu: joiner detection, writer open and publish are atomic // against other contributors and the last-contributor teardown. sg.Mu.Lock() + if !set.admitAgedOut(claim, hdr, objectID, obj.Properties) { + sg.Mu.Unlock() + continue + } + + // Counted after the dedup claim, so redundant copies don't count. + h.metrics.ObjectReceived(ref, hdr.SubgroupID) // Cache before bumping LARGEST_OBJECT, so a FETCH that snapshots the // new watermark finds the object cached. @@ -580,9 +640,11 @@ type subgroupWriter struct { inboundReset bool // set under dropsMu inside close inboundResetCode moqt.StreamResetCode // §3.3.4 reset code when inboundReset; set inside close // incomplete records that this subscription skipped an Object after its - // Start Location (filter, Forward State 0, overflow or expiry), so its - // streams end with a reset, not a FIN (§11.4.3), with incompleteCode: - // EXCESSIVE_LOAD after any overflow, else CANCELLED. Set under dropsMu. + // Start Location (filter, Forward State 0, overflow, expiry, or an Object + // dropped as aged out of the dedup window), so its streams end with a + // reset, not a FIN (§11.4.3), with incompleteCode: EXCESSIVE_LOAD after any + // overflow, else the first recorded (INTERNAL_ERROR for an aged-out drop, + // else CANCELLED). Set under dropsMu. // // Interpretation: Objects published before the subscription joined count // as before its Start Location, so a joiner's stream may still FIN. @@ -612,11 +674,26 @@ func (w *subgroupWriter) admit( props []byte, ) (take, follows bool) { follows = w.lastPos.src == pos.src && w.lastPos.seq+1 == pos.seq - switch w.sub.ForwardDecision(hdr.GroupID, objectID, hdr.SubgroupID, hdr.PublisherPriority, props) { - case registry.Forward: + v := w.sub.ForwardDecision(hdr.GroupID, objectID, hdr.SubgroupID, hdr.PublisherPriority, props) + if v == registry.Forward { w.lastAdmitted, w.hasAdmitted = objectID, true w.lastPos = pos return true, follows + } + w.skip(v, objectID) + // §11.4.3: an Object that "did not pass the subscriber's filters" does + // not separate the ones either side of it. Forward State is one of them + // (§5.1.5). + if follows { + w.lastPos = pos + } + return false, false +} + +// skip records that w omits the Object at objectID for verdict v, other than +// [registry.Forward], closing w when it will take none again. +func (w *subgroupWriter) skip(v registry.ForwardVerdict, objectID uint64) { + switch v { case registry.SkipObject, registry.SkipPaused: // The stream stays open for later Objects, but the Subgroup is now // incomplete (§11.4.3). @@ -631,14 +708,8 @@ func (w *subgroupWriter) admit( if w.hasAdmitted && objectID > w.lastAdmitted { w.markIncomplete(moqt.StreamResetCancelled) } + case registry.Forward: } - // §11.4.3: an Object that "did not pass the subscriber's filters" does - // not separate the ones either side of it. Forward State is one of them - // (§5.1.5). - if follows { - w.lastPos = pos - } - return false, false } // markIncomplete sets subgroupWriter.incomplete; the first code recorded diff --git a/pkg/relay/internal/registry/ledger_test.go b/pkg/relay/internal/registry/ledger_test.go index 0ce7deb9..8ef614d3 100644 --- a/pkg/relay/internal/registry/ledger_test.go +++ b/pkg/relay/internal/registry/ledger_test.go @@ -15,11 +15,11 @@ func newTestEntry(name string) *registry.TrackEntry { return registry.NewTrackRegistry().GetOrCreate(newTestTrackName(name)) } -// mustClaim claims o and fails unless ClaimDelivered returns (wantFresh, nil). -func mustClaim(t *testing.T, e *registry.TrackEntry, o registry.ObjectInfo, wantFresh bool) { +// mustClaim claims o and fails unless ClaimDelivered returns (want, nil). +func mustClaim(t *testing.T, e *registry.TrackEntry, o registry.ObjectInfo, want registry.Claim) { t.Helper() - if fresh, err := e.ClaimDelivered(o); err != nil || fresh != wantFresh { - t.Fatalf("ClaimDelivered(%+v) = (%v, %v), want (%v, nil)", o, fresh, err, wantFresh) + if got, err := e.ClaimDelivered(o); err != nil || got != want { + t.Fatalf("ClaimDelivered(%+v) = (%v, %v), want (%v, nil)", o, got, err, want) } } @@ -40,32 +40,74 @@ func cacheAndClaim(t *testing.T, e *registry.TrackEntry, objs ...registry.Object } // TestTrackEntry_ClaimDelivered pins the §2.1 dedup ledger: the first claim of a -// {GroupID, ObjectID} wins, a repeat loses, distinct objects/groups are -// independent, and a group that has aged out of the window is treated as already -// delivered. +// {GroupID, ObjectID} wins, a repeat loses, and distinct objects/groups are +// independent. func TestTrackEntry_ClaimDelivered(t *testing.T) { t.Parallel() e := newTestEntry("dedup") for _, c := range []struct { group, object uint64 - fresh bool + want registry.Claim }{ - {0, 5, true}, - {0, 5, false}, - {0, 2, true}, // a distinct Object in a Group already seen - {1, 5, true}, // a distinct Group - // Group 1000 moves the window past Group 0, so a late straggler from - // Group 0 counts as already delivered. - {1000, 0, true}, - {0, 9, false}, - // The current Group still dedups after the window advanced. - {1000, 1, true}, - {1000, 1, false}, + {0, 5, registry.ClaimFresh}, + {0, 5, registry.ClaimRedundant}, + {0, 2, registry.ClaimFresh}, // a distinct Object in a Group already seen + {1, 5, registry.ClaimFresh}, // a distinct Group + {1, 5, registry.ClaimRedundant}, } { - mustClaim(t, e, registry.ObjectInfo{Group: c.group, Object: c.object}, c.fresh) + mustClaim(t, e, registry.ObjectInfo{Group: c.group, Object: c.object}, c.want) } } +// TestTrackEntry_ClaimDeliveredWindow: the ledger holds 32 Groups, counted +// rather than spanned by ID (Group IDs need not be consecutive, §2.3.1). A +// jump in Group ID keeps the older Group; an Object of a Group older than the +// 32 held is ClaimAgedOut, not taken for a duplicate. +func TestTrackEntry_ClaimDeliveredWindow(t *testing.T) { + t.Parallel() + e := newTestEntry("window") + mustClaim(t, e, registry.ObjectInfo{Group: 0, Object: 0}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 1000, Object: 0}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 0, Object: 1}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 0, Object: 1}, registry.ClaimRedundant) + + // 30 more Groups fill the window; the next pushes out Group 0. + for g := uint64(2000); g < 2030; g++ { + mustClaim(t, e, registry.ObjectInfo{Group: g, Object: 0}, registry.ClaimFresh) + } + mustClaim(t, e, registry.ObjectInfo{Group: 0, Object: 2}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 2030, Object: 0}, registry.ClaimFresh) + for _, o := range []registry.ObjectInfo{ + {Group: 0, Object: 1}, // held before, now pruned + {Group: 0, Object: 3}, + {Group: 5, Object: 0}, // never held, below the window + } { + mustClaim(t, e, o, registry.ClaimAgedOut) + } + // Group 1000 is now the lowest held, and still dedups. + mustClaim(t, e, registry.ObjectInfo{Group: 1000, Object: 0}, registry.ClaimRedundant) + // A new Group between those held pushes out the lowest, Group 1000. + mustClaim(t, e, registry.ObjectInfo{Group: 1500, Object: 0}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 1000, Object: 0}, registry.ClaimAgedOut) + mustClaim(t, e, registry.ObjectInfo{Group: 1500, Object: 0}, registry.ClaimRedundant) +} + +// TestTrackEntry_ClaimDeliveredWindowNotFull: until the window holds 32 +// Groups, a Group below every one held is added, and is then the first pruned. +func TestTrackEntry_ClaimDeliveredWindowNotFull(t *testing.T) { + t.Parallel() + e := newTestEntry("window-low") + mustClaim(t, e, registry.ObjectInfo{Group: 10, Object: 0}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 3, Object: 0}, registry.ClaimFresh) + for g := uint64(11); g < 41; g++ { // 30 more fill the window + mustClaim(t, e, registry.ObjectInfo{Group: g, Object: 0}, registry.ClaimFresh) + } + mustClaim(t, e, registry.ObjectInfo{Group: 3, Object: 0}, registry.ClaimRedundant) + mustClaim(t, e, registry.ObjectInfo{Group: 41, Object: 0}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 3, Object: 0}, registry.ClaimAgedOut) + mustClaim(t, e, registry.ObjectInfo{Group: 10, Object: 0}, registry.ClaimRedundant) +} + // TestTrackEntry_ClaimDeliveredGapProperties pins how the ledger treats the // Prior Group and Object ID Gaps (§12.8, §12.9) of earlier Objects. An Object // inside an announced gap is known not to exist (§2.1) and dropped (§9.1); a gap @@ -111,7 +153,7 @@ func TestTrackEntry_ClaimDeliveredGapProperties(t *testing.T) { e := newTestEntry("gaps") last := len(tc.claims) - 1 for i, c := range tc.claims { - fresh, err := e.ClaimDelivered(registry.ObjectInfo{Group: c.group, Object: c.object, Gaps: c.gaps}) + claim, err := e.ClaimDelivered(registry.ObjectInfo{Group: c.group, Object: c.object, Gaps: c.gaps}) got := forwarded switch { case err != nil: @@ -119,7 +161,7 @@ func TestTrackEntry_ClaimDeliveredGapProperties(t *testing.T) { if !errors.Is(err, session.ErrMalformedTrack) { t.Fatalf("claim %d %+v: %v, want it to wrap session.ErrMalformedTrack", i, c, err) } - case !fresh: + case claim != registry.ClaimFresh: got = dropped } want := forwarded @@ -142,23 +184,28 @@ func TestTrackEntry_ClaimDeliveredMalformedLeavesNoTrace(t *testing.T) { e := newTestEntry("gaps") groupGap := func(n uint64) message.PriorGaps { return message.PriorGaps{Group: n, HasGroup: true} } - mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 0, Gaps: groupGap(2)}, true) // Groups 7-8 absent + mustClaim( + t, + e, + registry.ObjectInfo{Group: 9, Object: 0, Gaps: groupGap(2)}, + registry.ClaimFresh, + ) // Groups 7-8 absent if _, err := e.ClaimDelivered(registry.ObjectInfo{Group: 9, Object: 1, Gaps: groupGap(3)}); err == nil { t.Fatal("a second Prior Group ID Gap value in Group 9 is not malformed") } - mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 1}, true) // Object 1 was not recorded - mustClaim(t, e, registry.ObjectInfo{Group: 6, Object: 0}, true) // nor the gap of 3 (Groups 6-8) - mustClaim(t, e, registry.ObjectInfo{Group: 8, Object: 0}, false) + mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 1}, registry.ClaimFresh) // Object 1 was not recorded + mustClaim(t, e, registry.ObjectInfo{Group: 6, Object: 0}, registry.ClaimFresh) // nor the gap of 3 (Groups 6-8) + mustClaim(t, e, registry.ObjectInfo{Group: 8, Object: 0}, registry.ClaimRedundant) if _, err := e.ClaimDelivered(registry.ObjectInfo{Group: 9, Object: 5, Priority: 7}); err == nil { t.Fatal("another Publisher Priority in Subgroup 0 of Group 9 is not malformed") } - mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 5}, true) + mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 5}, registry.ClaimFresh) // A duplicate records no Subgroup state: the §9.1 check may still reject // it (the caller's), so Subgroup 3 keeps no priority from it. - mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 5, Subgroup: 3, Priority: 9}, false) - mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 6, Subgroup: 3, Priority: 1}, true) + mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 5, Subgroup: 3, Priority: 9}, registry.ClaimRedundant) + mustClaim(t, e, registry.ObjectInfo{Group: 9, Object: 6, Subgroup: 3, Priority: 1}, registry.ClaimFresh) } // TestTrackEntry_FinalObjects pins the §2.4.2 conditions that need earlier @@ -174,8 +221,8 @@ func TestTrackEntry_FinalObjects(t *testing.T) { // passed, is recorded too. claim := func(o registry.ObjectInfo) step { return func(e *registry.TrackEntry) error { - fresh, err := e.ClaimDelivered(o) - if err == nil && !fresh { + claim, err := e.ClaimDelivered(o) + if err == nil && claim != registry.ClaimFresh { err = e.RecordDuplicate(o) } return err @@ -358,10 +405,15 @@ func TestTrackEntry_FinalObjects(t *testing.T) { func TestTrackEntry_RecordDuplicateChecksAgain(t *testing.T) { t.Parallel() e := newTestEntry("race") - mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 2}, true) + mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 2}, registry.ClaimFresh) eog := registry.ObjectInfo{Group: 1, Object: 2, Status: message.ObjectStatusEndOfGroup} - mustClaim(t, e, eog, false) // a duplicate; the caller's §9.1 check runs now - mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 5}, true) // meanwhile, another upstream + mustClaim( + t, + e, + eog, + registry.ClaimRedundant, + ) // a duplicate; the caller's §9.1 check runs now + mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 5}, registry.ClaimFresh) // meanwhile, another upstream if err := e.RecordDuplicate(eog); !errors.Is(err, session.ErrMalformedTrack) { t.Fatalf("RecordDuplicate = %v, want the Group ending below Object 5 to be malformed", err) } @@ -458,11 +510,16 @@ func TestTrackEntry_LowestForwarded(t *testing.T) { if _, ok := e.LowestForwarded(1, 0); ok { t.Fatal("a Subgroup nothing was forwarded in has a lowest Object") } - mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 5}, true) - mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 3}, true) - mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 1, Datagram: true}, true) - mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 0, Subgroup: 1}, true) - mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 9, Subgroup: 2, Status: message.ObjectStatusEndOfGroup}, true) + mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 5}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 3}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 1, Datagram: true}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: 0, Subgroup: 1}, registry.ClaimFresh) + mustClaim( + t, + e, + registry.ObjectInfo{Group: 1, Object: 9, Subgroup: 2, Status: message.ObjectStatusEndOfGroup}, + registry.ClaimFresh, + ) for _, tc := range []struct{ subgroup, want uint64 }{{0, 3}, {1, 0}, {2, 9}} { if low, ok := e.LowestForwarded(1, tc.subgroup); !ok || low != tc.want { t.Errorf("LowestForwarded(1, %d) = (%d, %v), want (%d, true)", tc.subgroup, low, ok, tc.want) diff --git a/pkg/relay/internal/registry/track_entry.go b/pkg/relay/internal/registry/track_entry.go index d3e6a323..502db9cb 100644 --- a/pkg/relay/internal/registry/track_entry.go +++ b/pkg/relay/internal/registry/track_entry.go @@ -135,11 +135,9 @@ type TrackEntry struct { // identity). It lives on the entry (not on a SharedSubgroup) so peers whose // streams do not temporally overlap — e.g. one origin's subgroup FINs before // the redundant origin's arrives — still dedup. Memory is bounded by - // [deliveredGroupWindow]: state for a group more than that many groups behind - // the largest seen group is pruned, and a stray object from such an aged-out - // group is treated as already-delivered (a peer lagging by that many groups - // is beyond any useful reorder window). deliveredMax/HasMax track the largest - // group seen, for the pruning window. + // [deliveredGroupWindow]: it holds that many Groups, the lowest ID pruned + // first, and an Object of a pruned or lower Group is [ClaimAgedOut]. + // deliveredFloor is the lowest Group held, while delivered is non-empty. // // It also holds the Prior Group and Object ID Gaps (§12.8, §12.9) seen: // each Group its Object ID gaps and Group gap value, and groupGaps the Group @@ -148,10 +146,9 @@ type TrackEntry struct { groupGaps []idRange // trackEnd is where the Track ends, its END_OF_TRACK (§2.4.2), if // hasTrackEnd; kept past the window. - trackEnd message.Location - hasTrackEnd bool - deliveredMax uint64 - deliveredHasMax bool + trackEnd message.Location + hasTrackEnd bool + deliveredFloor uint64 // subgroups holds the shared outbound fan-out state for each // (GroupID, SubgroupID) currently being produced by one or more upstreams. @@ -224,27 +221,46 @@ func (e *TrackEntry) CopySubgroups() []*SharedSubgroup { return slices.Collect(maps.Values(e.subgroups)) } -// deliveredGroupWindow bounds the §2.1 dedup ledger ([TrackEntry.delivered]): -// dedup state is retained for the most recent deliveredGroupWindow groups. An -// object whose group is more than this many groups behind the largest group -// seen is assumed already delivered. The window must comfortably exceed any -// realistic inter-publisher group lag (a redundant origin or relay running a -// few groups behind) while keeping per-track dedup memory bounded. +// deliveredGroupWindow bounds the §2.1 dedup ledger ([TrackEntry.delivered]) +// to that many distinct Groups, counted rather than spanned by ID, since Group +// IDs need not be consecutive (§2.3.1, §12.8). The window must comfortably +// exceed any realistic inter-publisher group lag (a redundant origin or relay +// running a few groups behind) while keeping per-track dedup memory bounded. +// +// It assumes Group IDs mostly increase, as §2.3.1 lets a publisher choose: +// the lowest ID is pruned first, and a Group below all 32 held is taken as +// old. A publisher whose IDs decrease loses its Objects past the first 32 +// Groups to [ClaimAgedOut]. const deliveredGroupWindow = 32 +// Claim is [TrackEntry.ClaimDelivered]'s verdict on an Object. +type Claim int + +const ( + // ClaimFresh: the first copy; forward it. + ClaimFresh Claim = iota + // ClaimRedundant: a copy of an Object already forwarded, or one inside a + // gap announced earlier; drop it. + ClaimRedundant + // ClaimAgedOut: its Group is older than every Group the window holds, so + // whether it was forwarded is unknown, and nothing is recorded. The + // caller forwards it only where it cannot be a repeat. + ClaimAgedOut +) + // ClaimDelivered is the dedup gate across multiple upstream publishers (§9.3). // It records (group, object) as forwarded and reports whether the caller is -// the first to do so (true → forward it) or it was already forwarded by a peer -// upstream (false → drop it). The ledger persists on the entry (not on a +// the first to do so ([ClaimFresh]) or it was already forwarded by a peer +// upstream ([ClaimRedundant]). The ledger persists on the entry (not on a // per-Subgroup structure) and is independent of the size-bounded Object Cache, // so redundant streams that do not temporally overlap, or peers lagging by more -// than the cache capacity, still dedup correctly. Memory is bounded to the most -// recent [deliveredGroupWindow] groups. +// than the cache capacity, still dedup correctly. Memory is bounded to +// [deliveredGroupWindow] Groups; an Object of an older one is [ClaimAgedOut]. // // gaps are the Object's Prior Group and Object ID Gaps (§12.8, §12.9), which // the ledger records for the whole track. An Object inside a gap announced -// earlier is known not to exist, and that is permanent (§2.1): false, since a -// caching relay "SHOULD NOT cache or forward" it (§9.1). A gap covering an +// earlier is known not to exist, and that is permanent (§2.1): ClaimRedundant, +// since a caching relay "SHOULD NOT cache or forward" it (§9.1). A gap covering an // Object already received is accepted: an Object may go from existing to not // existing (§2.1). // @@ -258,56 +274,60 @@ const deliveredGroupWindow = 32 // carrying two Prior Group ID Gap values (§12.8). An Object ClaimDelivered // rejects leaves no state in the ledger, and a duplicate records only its // gaps, since the caller's §9.1 check may still reject it. -func (e *TrackEntry) ClaimDelivered(o ObjectInfo) (bool, error) { +func (e *TrackEntry) ClaimDelivered(o ObjectInfo) (Claim, error) { group, object, gaps := o.Group, o.Object, o.Gaps e.deliveredMu.Lock() defer e.deliveredMu.Unlock() - // An object from a group already aged out of the window is treated as - // already delivered — a peer lagging that far behind is past any useful - // reorder window, and re-forwarding it would be a large out-of-order break. - if e.deliveredHasMax && group <= e.deliveredMax && e.deliveredMax-group >= deliveredGroupWindow { - return false, nil - } g := e.delivered[group] + if g == nil && len(e.delivered) >= deliveredGroupWindow && group < e.deliveredFloor { + return ClaimAgedOut, nil + } if gaps.HasGroup && g != nil && g.hasGroupGap && g.groupGap != gaps.Group { - return false, fmt.Errorf("%w: Group %d carries Prior Group ID Gaps %d and %d (§12.8)", + return ClaimRedundant, fmt.Errorf("%w: Group %d carries Prior Group ID Gaps %d and %d (§12.8)", session.ErrMalformedTrack, group, g.groupGap, gaps.Group) } if err := e.checkEndsLocked(g, o); err != nil { - return false, err + return ClaimRedundant, err } if e.announcedAbsentLocked(g, group, object) { - return false, nil - } - - if e.delivered == nil { - e.delivered = make(map[uint64]*deliveredGroup) - } - // Advance the window when a newer group appears, pruning groups that have - // fallen out of it. - if !e.deliveredHasMax || group > e.deliveredMax { - e.deliveredMax = group - e.deliveredHasMax = true - maps.DeleteFunc(e.delivered, func(id uint64, _ *deliveredGroup) bool { - return e.deliveredMax-id >= deliveredGroupWindow - }) - e.groupGaps = slices.DeleteFunc(e.groupGaps, func(r idRange) bool { - return e.deliveredMax-r.hi >= deliveredGroupWindow - }) + return ClaimRedundant, nil } if g == nil { g = &deliveredGroup{objects: make(map[uint64]struct{})} - e.delivered[group] = g + e.addDeliveredGroupLocked(group, g) } e.recordGapsLocked(g, group, object, gaps) if _, ok := g.objects[object]; ok { - return false, nil + return ClaimRedundant, nil } e.recordEndsLocked(g, o) g.objects[object] = struct{}{} - return true, nil + return ClaimFresh, nil +} + +// addDeliveredGroupLocked adds group's ledger entry g, first pruning the +// lowest Group, and the gaps announced below the new lowest, when the window +// is full. Runs once per Group, so its scan of the window is not per Object. +func (e *TrackEntry) addDeliveredGroupLocked(group uint64, g *deliveredGroup) { + if e.delivered == nil { + e.delivered = make(map[uint64]*deliveredGroup, deliveredGroupWindow) + } + if len(e.delivered) >= deliveredGroupWindow { + delete(e.delivered, e.deliveredFloor) + e.deliveredFloor = math.MaxUint64 + for id := range e.delivered { + e.deliveredFloor = min(e.deliveredFloor, id) + } + e.groupGaps = slices.DeleteFunc(e.groupGaps, func(r idRange) bool { + return r.hi < e.deliveredFloor + }) + } + if len(e.delivered) == 0 || group < e.deliveredFloor { + e.deliveredFloor = group + } + e.delivered[group] = g } // ObjectInfo is what [TrackEntry.ClaimDelivered] checks of an Object against From 9a543033b1f63609dd7ee53ff090f2717116a3de Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 18:42:07 +0500 Subject: [PATCH 5/9] =?UTF-8?q?fix(relay):=20reset=20a=20cancelled=20subsc?= =?UTF-8?q?ription's=20open=20subgroup=20and=20fill=20streams=20(=C2=A75.1?= =?UTF-8?q?.1,=20=C2=A75.1.3.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a subscriber cancelled a SUBSCRIBE, or the subscription a forwarded PUBLISH opened, the relay only removed it from the track. The subgroup streams it had open kept forwarding and then FINed, or stayed open while the publisher was idle, and an open fill fetch stream was written to the end. §5.1.1: the publisher "MUST reset any open streams associated with the SUBSCRIBE"; §5.1.3.1: "When the subscription is cancelled, the publisher MUST reset any open fill fetch streams". registry.DownstreamSub gains Cancel, run once the request ends (the subscriber cancelled, or the session is ending): it terminates the subscription and cancels its Cancelled context. Each subgroup writer's I/O context and each fill's context end with it, so their open streams are reset. A PUBLISH_DONE a termination left pending still goes out once those streams close, since before SUBSCRIBE_OK it is the request's only response (§3.3.2). streamFetchRange resets its stream when its context ends: CANCELLED for a cancelled subscription's fill, SESSION_CLOSED otherwise (§3.3.4); plain FETCH cancellation (§5.2) is still the backlog item it was. The new tests, over SUBSCRIBE and SUBSCRIBE_TRACKS, fail on the unpatched code: the idle stream stays open, the live one and the fill FIN. They wait for the relay to finish the cancelled subscription (a Metrics hook) before reading on; the fill test then pauses 100ms so the reset's goroutines run while the writer is held. 50/50 with and without -race. BenchmarkFetchFromCache goes from 115 to 118 allocs/op: the one AfterFunc registered per fetch stream, not per Object. Co-Authored-By: Claude Opus 5.5 (1M context) --- STATUS.md | 7 +- pkg/relay/handler_fanout.go | 25 ++- pkg/relay/handler_fanout_join_test.go | 8 +- pkg/relay/handler_fill.go | 11 +- pkg/relay/handler_forward.go | 3 + pkg/relay/handler_subscribe.go | 3 + pkg/relay/internal/registry/subscription.go | 25 +++ pkg/relay/session_handler.go | 27 +++ pkg/relay/subscribe_cancel_test.go | 200 ++++++++++++++++++++ 9 files changed, 286 insertions(+), 23 deletions(-) create mode 100644 pkg/relay/subscribe_cancel_test.go diff --git a/STATUS.md b/STATUS.md index 5fc9eee0..4ecef72c 100644 --- a/STATUS.md +++ b/STATUS.md @@ -108,9 +108,9 @@ 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.1 | Subscription state management | DONE | REQUEST_ERROR / STOP_SENDING / PUBLISH_DONE handling + cleanup. | +| 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. Not done: scheduling fills against their subscription (§7.2, see Limitations) and resetting open fills when the subscription is cancelled (§5.1.3.1). | +| 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). | | 5.1.4 | Range filters | DONE | Object filters (SUBGROUP/OBJECTID/PRIORITY/OBJECT_PROPERTY) enforced on SUBSCRIBE fanout, datagrams, and FETCH; TRACK_PROPERTY_FILTER gates PUBLISH forwarding on SUBSCRIBE_TRACKS; `MAX_FILTER_RANGES`/`INVALID_FILTER` gating in place. Object filters on a SUBSCRIBE_TRACKS apply to the subscriptions its forwarded PUBLISHes open. A zero-length filter is no filter; a subscription REQUEST_UPDATE replaces (or, zero-length, removes) the filter types it names and keeps the others. | | 5.1.5 | Combining filters | DONE | `ForwardDecision` ANDs Forward + Location + Range filters per object (§5.1.5); Range filters combine SetIDs via AND/OR. | | 5.1.6 | Joining an ongoing track | DONE | A Location Filter plus FILL_PARAMETERS, served as a fill fetch stream (draft-20 removed the Joining FETCH). | @@ -506,9 +506,6 @@ High: a reset subgroup stream or a subgroup still in flight leaves such holes. §10.13: a relay that meets an uncached Object of unknown status "MUST pause subsequent delivery until it has confirmed the object's status upstream". -- A subscriber's cancellation does not reset the subscription's open subgroup - streams or fill fetch streams: the relay keeps forwarding, then FINs (§5.1.1: - "MUST reset any open streams associated with the SUBSCRIBE"; §5.1.3.1). - A fill stream on a subscription that omits GROUP_ORDER is written Ascending, ignoring DEFAULT_PUBLISHER_GROUP_ORDER (§10.2.8, §10.2.15, §12.5). A subscriber decoding it Descending gets wrong Group IDs (§11.4.4.1). diff --git a/pkg/relay/handler_fanout.go b/pkg/relay/handler_fanout.go index e8a769f1..3f5f844f 100644 --- a/pkg/relay/handler_fanout.go +++ b/pkg/relay/handler_fanout.go @@ -576,12 +576,14 @@ func (h *sessionHandler) openWriterForSub( subHdr.InlinePriority = true } // cancelIO unblocks a writer wedged on a subscriber that stopped - // reading (see [joinWriters]). + // reading (see [joinWriters]), and resets the open stream of one that + // cancelled (§5.1.1). ioCtx, cancelIO := context.WithCancel(ctx) w := &subgroupWriter{ sub: sub, ctx: ioCtx, cancelIO: cancelIO, + unwatchSub: context.AfterFunc(sub.Cancelled(), cancelIO), hdr: subHdr, inbox: make(chan fwdObject, h.sendQueueSize), done: make(chan struct{}), @@ -615,15 +617,17 @@ type subgroupWriter struct { sub *registry.DownstreamSub // ctx bounds every blocking stream operation; cancelIO resets the // in-flight stream, unwedging a writer blocked on a stalled subscriber. - ctx context.Context - cancelIO context.CancelFunc - hdr message.SubgroupHeader // template; TrackAlias already remapped - out *session.OutgoingSubgroupStream // nil until run opens it lazily - unbridge func() bool // stops the current stream's ctx→Cancel bridge - inbox chan fwdObject - done chan struct{} - log *slog.Logger - metrics Metrics + // The subscriber's cancellation calls it too (§5.1.1), until unwatchSub. + ctx context.Context + cancelIO context.CancelFunc + unwatchSub func() bool + hdr message.SubgroupHeader // template; TrackAlias already remapped + out *session.OutgoingSubgroupStream // nil until run opens it lazily + unbridge func() bool // stops the current stream's ctx→Cancel bridge + inbox chan fwdObject + done chan struct{} + log *slog.Logger + metrics Metrics // ref labels every Metrics call; built once, since it allocates. ref TrackRef maxDropsBeforeReset int @@ -831,6 +835,7 @@ func (w *subgroupWriter) dropOut() { // draining, so publish never blocks. func (w *subgroupWriter) run() { defer close(w.done) + defer w.unwatchSub() var ( prevID uint64 diff --git a/pkg/relay/handler_fanout_join_test.go b/pkg/relay/handler_fanout_join_test.go index 1edd1c4c..adc4bcca 100644 --- a/pkg/relay/handler_fanout_join_test.go +++ b/pkg/relay/handler_fanout_join_test.go @@ -17,10 +17,12 @@ import ( func newWedgeableWriter(t *testing.T, cli *session.Session) *subgroupWriter { t.Helper() ioCtx, cancelIO := context.WithCancel(t.Context()) + sub := registry.NewDownstreamSub(1, cli, nil, 42) return &subgroupWriter{ - sub: registry.NewDownstreamSub(1, cli, nil, 42), - ctx: ioCtx, - cancelIO: cancelIO, + sub: sub, + ctx: ioCtx, + cancelIO: cancelIO, + unwatchSub: context.AfterFunc(sub.Cancelled(), cancelIO), hdr: message.SubgroupHeader{ SubgroupIDMode: message.SubgroupIDImplicitZero, TrackAlias: 42, diff --git a/pkg/relay/handler_fill.go b/pkg/relay/handler_fill.go index b6c7278d..ddd002fb 100644 --- a/pkg/relay/handler_fill.go +++ b/pkg/relay/handler_fill.go @@ -104,15 +104,16 @@ func (h *sessionHandler) maybeServeFill( } h.relayGo(func() { - h.serveFill(ctx, sub, requestID, entry, fullName, start, end, order, fillTimeout, rangeFilters) + // §5.1.3.1: "When the subscription is cancelled, the publisher MUST + // reset any open fill fetch streams". + fillCtx, cancel := context.WithCancelCause(ctx) + defer cancel(nil) + defer context.AfterFunc(sub.Cancelled(), func() { cancel(errRequestCancelled) })() + h.serveFill(fillCtx, sub, requestID, entry, fullName, start, end, order, fillTimeout, rangeFilters) }) return nil } -// TODO(draft-20): §5.1.3.1 "When the subscription is cancelled, the publisher -// MUST reset any open fill fetch streams" needs a watchdog on ctx cancellation -// mid-write. - // serveFill writes one fill fetch stream; the FIN signals completion // (§5.1.3.1), and [sessionHandler.streamFetchRange] resets it on a write error. func (h *sessionHandler) serveFill( diff --git a/pkg/relay/handler_forward.go b/pkg/relay/handler_forward.go index 1381c7fd..a09edd6f 100644 --- a/pkg/relay/handler_forward.go +++ b/pkg/relay/handler_forward.go @@ -96,6 +96,9 @@ func (h *sessionHandler) serveForwardedPublish( ref := h.trackRef(fullName) h.metrics.SubscriptionOpened(ref) defer h.metrics.SubscriptionClosed(ref) + // §5.1.1: once the subscriber cancels, or the session ends, reset the + // streams still open for it. + defer sub.Cancel() // §9.2: a forwarding subscriber resumes a paused upstream. if sub.ForwardState() == 1 { h.propagateForwardUpstream(ctx, fullName) diff --git a/pkg/relay/handler_subscribe.go b/pkg/relay/handler_subscribe.go index 62550a15..adaaeeac 100644 --- a/pkg/relay/handler_subscribe.go +++ b/pkg/relay/handler_subscribe.go @@ -120,6 +120,9 @@ func (h *sessionHandler) handleSubscribe(ctx context.Context, req *session.Reque defer h.metrics.SubscriptionClosed(subRef) defer h.tracks.RemoveDownstream(fullName, sub.ID) + // §5.1.1: once the subscriber cancels, or the session ends, reset the + // streams still open for it. + defer sub.Cancel() // §10.2.17: "If Objects have been published on this Track the Publisher // MUST include this parameter." diff --git a/pkg/relay/internal/registry/subscription.go b/pkg/relay/internal/registry/subscription.go index b67b876f..a27b27c2 100644 --- a/pkg/relay/internal/registry/subscription.go +++ b/pkg/relay/internal/registry/subscription.go @@ -443,6 +443,10 @@ type DownstreamSub struct { // datagramsSending counts datagram sends in flight. datagramsSending int pendingDone *pendingPublishDone + + // cancelled is done once the request ends; see [DownstreamSub.Cancel]. + cancelled context.Context + cancel context.CancelFunc } // pendingPublishDone is a termination's PUBLISH_DONE, held until the last of @@ -533,7 +537,10 @@ func (d *DownstreamSub) takeReadyDoneLocked() (*pendingPublishDone, uint64) { // installSubscribeParams overrides this to 0 only when the peer explicitly // sends FORWARD=0, and REQUEST_UPDATE can flip it later (§9.2 / §10.9). func NewDownstreamSub(id uint64, sess *session.Session, stream session.Stream, trackAlias uint64) *DownstreamSub { + cancelled, cancel := context.WithCancel(context.Background()) return &DownstreamSub{ + cancelled: cancelled, + cancel: cancel, state: SubEstablished, ID: id, Session: sess, @@ -773,6 +780,24 @@ func GroupOutOfRange(group uint64, f *message.LocationFilter) bool { return ok && group > end.Group } +// Cancel ends the subscription once its request has ended: the subscriber +// cancelled it (§3.3.3), or the session is ending. [DownstreamSub.Cancelled] +// is done, so the streams still open for it are reset ("It MUST reset any open +// streams associated with the SUBSCRIBE", §5.1.1; its fill fetch streams, +// §5.1.3.1). No new PUBLISH_DONE follows; one a termination left pending still +// goes out once those streams close, since it may be the request's only +// response (§3.3.2). Safe to call more than once, and after another +// termination. +func (d *DownstreamSub) Cancel() { + d.mu.Lock() + d.state = SubTerminated + d.mu.Unlock() + d.cancel() +} + +// Cancelled is done once [DownstreamSub.Cancel] has run. +func (d *DownstreamSub) Cancelled() context.Context { return d.cancelled } + // TerminateWithPublishDone ends this downstream subscription (§10.12): the // relay writes PUBLISH_DONE on the subscriber's request stream and FINs the // send side. If SUBSCRIBE_OK never went out, it answers with REQUEST_ERROR diff --git a/pkg/relay/session_handler.go b/pkg/relay/session_handler.go index 7bcd7bde..5221c31b 100644 --- a/pkg/relay/session_handler.go +++ b/pkg/relay/session_handler.go @@ -526,6 +526,19 @@ func (h *sessionHandler) handleFollowupTokens( return nil, false } +// errRequestCancelled is the cancellation cause of a context bound to a +// request its peer cancelled (§3.3.3); see [ctxResetCode]. +var errRequestCancelled = errors.New("relay: request cancelled") + +// ctxResetCode is the §3.3.4 code for a stream reset because ctx ended: +// CANCELLED for a cancelled request, SESSION_CLOSED otherwise. +func ctxResetCode(ctx context.Context) moqt.StreamResetCode { + if errors.Is(context.Cause(ctx), errRequestCancelled) { + return moqt.StreamResetCancelled + } + return moqt.StreamResetSessionClosed +} + // readRequestStream owns all reads on an established request stream: it // parses follow-up messages off the stream and dispatches each to onMsg // until the peer ends its side (FIN or reset), onMsg returns false, @@ -658,10 +671,20 @@ func (h *sessionHandler) streamFetchRange( // stream closes (§10.12). defer sub.StreamClosed() } + // ctx ending resets the stream rather than completing it: CANCELLED when + // its cause is errRequestCancelled (a fill's cancelled subscription, + // §5.1.3.1), else SESSION_CLOSED (§3.3.4). + cancelOut := func() { out.Cancel(ctxResetCode(ctx)) } + unwatch := context.AfterFunc(ctx, cancelOut) + defer unwatch() // Gather cached objects, stitching the below-floor portion from upstream // when the cache doesn't cover the whole range (§9.4). objs, refusal := h.stitchedFetchObjects(ctx, entry, fullName, start, end, order, fillTimeout) + if ctx.Err() != nil { + cancelOut() // before the deferred StreamClosed (§10.12) + return false + } if refusal != nil { // §2.5.1: with FETCH_OK (or SUBSCRIBE_OK) already sent, only a // reset is left (an interpretation: no Object was forwarded yet). @@ -697,6 +720,10 @@ func (h *sessionHandler) streamFetchRange( out.Cancel(moqt.StreamResetInternalError) return false } + if !unwatch() { + cancelOut() // ctx ended first; reset before the deferred StreamClosed + return false + } _ = out.Close() return true } diff --git a/pkg/relay/subscribe_cancel_test.go b/pkg/relay/subscribe_cancel_test.go new file mode 100644 index 00000000..dcf41df1 --- /dev/null +++ b/pkg/relay/subscribe_cancel_test.go @@ -0,0 +1,200 @@ +package relay_test + +import ( + "bytes" + "errors" + "fmt" + "io" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// A subscriber that cancels its SUBSCRIBE gets the subscription's open +// streams reset: §5.1.1 "It MUST reset any open streams associated with the +// SUBSCRIBE", and §5.1.3.1 "When the subscription is cancelled, the publisher +// MUST reset any open fill fetch streams". + +// readEnd reads next until it fails, returning that error, or fails the test +// after 2s. +func readEnd(t *testing.T, next func() error) error { + t.Helper() + end := make(chan error, 1) + go func() { + for { + if err := next(); err != nil { + end <- err + return + } + } + }() + select { + case err := <-end: + return err + case <-time.After(2 * time.Second): + t.Fatal("the stream stayed open after the subscription was cancelled") + return nil + } +} + +// subClosedMetrics signals each SubscriptionClosed, which the relay reports +// after it has acted on a subscription's end. +type subClosedMetrics struct { + relay.NopMetrics + + closed chan struct{} +} + +func (m *subClosedMetrics) SubscriptionClosed(relay.TrackRef) { m.closed <- struct{}{} } + +// newCancelTestRelay is [newCam1Publisher] on a relay whose closed channel +// receives once for each subscription it has finished. +func newCancelTestRelay(t *testing.T) (pubSess *session.Session, alias uint64, closed <-chan struct{}) { + t.Helper() + m := &subClosedMetrics{closed: make(chan struct{}, 8)} + pubSess, teardown := connectRelay(t, relay.Config{Metrics: m}) + t.Cleanup(teardown) + alias = 7 + publishVideoTrackProps(t, pubSess, "cam1", alias, nil) + return pubSess, alias, m.closed +} + +// cancelAndAwait cancels the subscription and waits for the relay to finish +// it, so what the test does next cannot overtake the cancellation. +func cancelAndAwait(t *testing.T, cancel func(), closed <-chan struct{}) { + t.Helper() + cancel() + select { + case <-closed: + case <-time.After(2 * time.Second): + t.Fatal("the relay did not finish the cancelled subscription") + } +} + +// subscribeVia subscribes sess to video/cam1 with params, directly or as the +// subscription a SUBSCRIBE_TRACKS on video gets through a forwarded PUBLISH, +// and returns what cancels it. +type subscribeVia func(t *testing.T, sess *session.Session, params ...message.Parameter) (cancel func()) + +var subscribeVias = []struct { + name string + sub subscribeVia +}{ + {"SUBSCRIBE", func(t *testing.T, sess *session.Session, params ...message.Parameter) func() { + sub := subscribeCam1(t, sess, params...) + return func() { _ = sub.Close() } + }}, + {"SUBSCRIBE_TRACKS", func(t *testing.T, sess *session.Session, params ...message.Parameter) func() { + reqs := forwardedPublishes(t, sess) + subscribeTracks(t, sess, ns("video"), params...) + in := acceptForwarded(t, awaitForwarded(t, reqs)) + return func() { _ = in.Close() } + }}, +} + +// TestSubscribe_CancelResetsOpenSubgroup: the subgroup stream the relay has +// open for a cancelled subscription is reset, whether the publisher goes on +// writing to it or not. +func TestSubscribe_CancelResetsOpenSubgroup(t *testing.T) { + t.Parallel() + for _, via := range subscribeVias { + for _, continues := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/publisher continues=%v", via.name, continues), func(t *testing.T) { + t.Parallel() + pubSess, alias, closed := newCancelTestRelay(t) + subSess := dialAnotherClient(t, pubSess) + cancel := via.sub(t, subSess) + + sg, err := openSubgroupWaiting(t, pubSess, subgroupHeader(alias, 0)) + if err != nil { + t.Fatalf("OpenSubgroup: %v", err) + } + wrote := make(chan struct{}) + go func() { + defer close(wrote) + _ = sg.WriteObject(&message.SubgroupObject{Payload: []byte("x")}) + }() + ds, err := subSess.AcceptDataStream(t.Context()) + if err != nil { + t.Fatalf("AcceptDataStream: %v", err) + } + in := ds.(*session.IncomingSubgroupStream) + if _, err := in.ReadDecoded(); err != nil { + t.Fatalf("ReadDecoded: %v", err) + } + + <-wrote + cancelAndAwait(t, cancel, closed) + if continues { + go func() { + for range 3 { + if sg.WriteObject(&message.SubgroupObject{Payload: []byte("x")}) != nil { + return + } + } + _ = sg.Close() + }() + } + if err := readEnd(t, func() error { _, err := in.ReadDecoded(); return err }); errors.Is(err, io.EOF) { + t.Fatal("the subgroup stream was FINed after the subscription was cancelled, want a reset") + } + }) + } + } +} + +// TestSubscribe_CancelResetsFill: a fill fetch stream still being written +// when its subscription is cancelled is reset, not completed. +func TestSubscribe_CancelResetsFill(t *testing.T) { + t.Parallel() + for _, via := range subscribeVias { + t.Run(via.name, func(t *testing.T) { + t.Parallel() + pubSess, alias, closed := newCancelTestRelay(t) + sg, err := openSubgroupWaiting(t, pubSess, subgroupHeader(alias, 0)) + if err != nil { + t.Fatalf("OpenSubgroup: %v", err) + } + // Enough bytes that the fill cannot be written ahead of the reader. + payload := bytes.Repeat([]byte("x"), 4096) + for range 32 { + if err := sg.WriteObject(&message.SubgroupObject{Payload: payload}); err != nil { + t.Fatalf("WriteObject: %v", err) + } + } + if err := sg.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + time.Sleep(50 * time.Millisecond) + + subSess := dialAnotherClient(t, pubSess) + cancel := via.sub(t, subSess, + message.NextObjectFilter(), + message.FillParametersParam(message.Parameters{message.UnfilteredFilter()}), + ) + ds, err := subSess.AcceptDataStream(t.Context()) + if err != nil { + t.Fatalf("AcceptDataStream: %v", err) + } + fs, ok := ds.(*session.IncomingFetchStream) + if !ok { + t.Fatalf("got %T, want the fill fetch stream", ds) + } + if _, err := fs.ReadDecoded(); err != nil { + t.Fatalf("ReadDecoded: %v", err) + } + + cancelAndAwait(t, cancel, closed) + // The fill is reset from a goroutine the cancellation starts; not + // reading meanwhile holds the writer on its next Object, which the + // synchronous test transport would otherwise let it race past. + time.Sleep(100 * time.Millisecond) + if err := readEnd(t, func() error { _, err := fs.ReadDecoded(); return err }); errors.Is(err, io.EOF) { + t.Fatal("the fill fetch stream was completed after the subscription was cancelled, want a reset") + } + }) + } +} From 5804df6e5eb92005c6073898b606a9cc7b81a746 Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 18:48:20 +0500 Subject: [PATCH 6/9] =?UTF-8?q?fix(relay):=20a=20subscription=20that=20omi?= =?UTF-8?q?ts=20GROUP=5FORDER=20takes=20the=20publisher's=20(=C2=A710.2.8,?= =?UTF-8?q?=20=C2=A712.5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A SUBSCRIBE, or a SUBSCRIBE_TRACKS's forwarded-PUBLISH subscription, that omitted GROUP_ORDER was treated as Ascending: the relay did not decode DEFAULT_PUBLISHER_GROUP_ORDER. Its fill fetch stream was therefore serialized with Ascending Group ID deltas, which a subscriber following §10.2.8 ("If omitted from SUBSCRIBE or SUBSCRIBE_TRACKS, the publisher's preference from the Track is used") decodes Descending (§11.4.4.1) on a Descending track: wrong Group IDs, or an underflow. The registry now decodes DEFAULT_PUBLISHER_GROUP_ORDER (TrackEntry.DefaultGroupOrder: Ascending when omitted, per §12.5, and for a value outside {1, 2} or malformed Properties). resolveGroupOrder sets an omitted GROUP_ORDER from it once, before the subscription is registered, so its fills (§10.2.15) and §7.2 stream scheduling follow it. GROUP_ORDER cannot appear in REQUEST_UPDATE and §7.1 fixes a subscription's order, so once is enough. A fill's own GROUP_ORDER still overrides it. The two relay tests fail on the unpatched code, decoding Groups [0, 2^64-1, 2^64-2] for [2, 1, 0]; removing the forwarded path's resolution fails its test. An out-of-range value still falls back to Ascending here; closing the session for it (§12.5) is a separate backlog item. Recorded as a Limitation: with INCLUDE_PROPERTIES=0 a subscriber that omits GROUP_ORDER cannot learn the order its fill is written in. Open: whether the forwarded PUBLISH should state the resolved GROUP_ORDER. Co-Authored-By: Claude Opus 5.5 (1M context) --- STATUS.md | 10 ++- pkg/relay/fill_group_order_test.go | 85 +++++++++++++++++++ pkg/relay/handler_forward.go | 1 + pkg/relay/handler_subscribe.go | 17 ++++ .../internal/registry/dynamic_groups_test.go | 37 ++++++++ pkg/relay/internal/registry/properties.go | 22 +++++ pkg/relay/internal/registry/subscription.go | 9 +- 7 files changed, 173 insertions(+), 8 deletions(-) create mode 100644 pkg/relay/fill_group_order_test.go diff --git a/STATUS.md b/STATUS.md index 4ecef72c..4cf1e624 100644 --- a/STATUS.md +++ b/STATUS.md @@ -483,6 +483,13 @@ Known protocol gaps, roughly ordered by how load-bearing they are: reads only the mutable list, so a LOC Timestamp and the like placed inside Immutable Properties is not found. Filling the fields from there would make `Append` re-emit them in the mutable list, changing what a relay forwards. +- **A Group Order the subscriber cannot learn (§10.2.8, §10.2.21)** — a + SUBSCRIBE that omits GROUP_ORDER takes the publisher's + DEFAULT_PUBLISHER_GROUP_ORDER, and its fill fetch stream is written in that + order (§11.4.4.1). With INCLUDE_PROPERTIES=0 the subscriber gets no Track + Properties, and neither SUBSCRIBE_OK nor the FETCH_HEADER carries a Group + Order, so it cannot tell a Descending fill from an Ascending one. A gap in the + draft; a subscriber that asks for a fill avoids it by sending GROUP_ORDER. - **Fill streams are not scheduled against their subscription (§7.2 rules 3 and 4)** — a subscription-delivered Object should go first when the fill's Group Order differs, and the fill-delivered one first within a Group. The @@ -506,9 +513,6 @@ High: a reset subgroup stream or a subgroup still in flight leaves such holes. §10.13: a relay that meets an uncached Object of unknown status "MUST pause subsequent delivery until it has confirmed the object's status upstream". -- A fill stream on a subscription that omits GROUP_ORDER is written Ascending, - ignoring DEFAULT_PUBLISHER_GROUP_ORDER (§10.2.8, §10.2.15, §12.5). A subscriber - decoding it Descending gets wrong Group IDs (§11.4.4.1). Session layer: diff --git a/pkg/relay/fill_group_order_test.go b/pkg/relay/fill_group_order_test.go new file mode 100644 index 00000000..8e57e2ec --- /dev/null +++ b/pkg/relay/fill_group_order_test.go @@ -0,0 +1,85 @@ +package relay_test + +import ( + "slices" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// A subscription that omits GROUP_ORDER takes the publisher's preference from +// the Track (§10.2.8, §12.5), and so does its fill (§10.2.15). The fill's +// Group ID deltas run in that order (§11.4.4.1), so a subscriber decoding it +// by the draft's rule gets the right Groups only if the relay agrees. + +// publishDescendingCam publishes video/cam on alias 7 with +// DEFAULT_PUBLISHER_GROUP_ORDER Descending and one Object in each of Groups +// 0-2, which the relay caches. +func publishDescendingCam(t *testing.T) *session.Session { + t.Helper() + pubSess, teardown := connectRelay(t, relay.Config{}) + t.Cleanup(teardown) + publishVideoTrackProps(t, pubSess, "cam", 7, + trackProp(message.PropertyDefaultPublisherGroupOrder, uint64(message.GroupOrderDescending))) + for g := range uint64(3) { + sendObjects(pubSess, 7, g, 1) + } + time.Sleep(50 * time.Millisecond) + return pubSess +} + +// requireDescendingFill reads the fill fetch stream sess accepts, decoding it +// Descending, and requires Groups 2, 1, 0. +func requireDescendingFill(t *testing.T, sess *session.Session) { + t.Helper() + ds, ok := tryAcceptDataStream(t, sess, 2*time.Second) + if !ok { + t.Fatal("no fill fetch stream") + } + fs, ok := ds.(*session.IncomingFetchStream) + if !ok { + t.Fatalf("got %T, want the fill fetch stream", ds) + } + var groups []uint64 + for _, o := range decodeFetchStream(t, fs, message.GroupOrderDescending) { + groups = append(groups, o.group) + } + if !slices.Equal(groups, []uint64{2, 1, 0}) { + t.Fatalf("fill decoded Descending gave Groups %v, want [2 1 0]", groups) + } +} + +// fillWholeTrack is a Next Object subscription's FILL_PARAMETERS for the whole +// track, with no GROUP_ORDER anywhere. +var fillWholeTrack = []message.Parameter{ + message.NextObjectFilter(), + message.FillParametersParam(message.Parameters{message.UnfilteredFilter()}), +} + +// TestSubscribe_FillFollowsPublisherGroupOrder: SUBSCRIBE. +func TestSubscribe_FillFollowsPublisherGroupOrder(t *testing.T) { + t.Parallel() + subSess := dialAnotherClient(t, publishDescendingCam(t)) + sub, err := subSess.Subscribe(t.Context(), &message.Subscribe{ + Namespace: ns("video"), Name: []byte("cam"), Parameters: fillWholeTrack, + }) + if err != nil { + t.Fatalf("Subscribe: %v", err) + } + t.Cleanup(func() { _ = sub.Close() }) + requireDescendingFill(t, subSess) +} + +// TestSubscribeTracks_FillFollowsPublisherGroupOrder: a forwarded PUBLISH's +// subscription (§10.20.1). +func TestSubscribeTracks_FillFollowsPublisherGroupOrder(t *testing.T) { + t.Parallel() + holder := dialAnotherClient(t, publishDescendingCam(t)) + reqs := forwardedPublishes(t, holder) + subscribeTracks(t, holder, ns("video"), fillWholeTrack...) + acceptForwarded(t, awaitForwarded(t, reqs)) + requireDescendingFill(t, holder) +} diff --git a/pkg/relay/handler_forward.go b/pkg/relay/handler_forward.go index a09edd6f..6f2e60fd 100644 --- a/pkg/relay/handler_forward.go +++ b/pkg/relay/handler_forward.go @@ -85,6 +85,7 @@ func (h *sessionHandler) serveForwardedPublish( sub.OpenedByPublish() // handleSubscribeTracks already refused parameters this would reject. _ = installSubscribeParams(sub, params) + resolveGroupOrder(sub, te) _, largest, has, added := h.tracks.AddDownstreamSnapshotLargest(fullName, sub) registered() if !added { diff --git a/pkg/relay/handler_subscribe.go b/pkg/relay/handler_subscribe.go index adaaeeac..30cbfd72 100644 --- a/pkg/relay/handler_subscribe.go +++ b/pkg/relay/handler_subscribe.go @@ -94,6 +94,11 @@ func (h *sessionHandler) handleSubscribe(ctx context.Context, req *session.Reque h.log.LogAttrs(ctx, slog.LevelDebug, "SUBSCRIBE serving from existing upstream") } + // Before registration, so the first stream opened for it already + // schedules in its Group Order (§7.2). + if cur, ok := h.tracks.Get(fullName.Key()); ok { + resolveGroupOrder(sub, cur) + } // Register and snapshot Largest atomically, so no object falls between // live delivery and the fill fetch stream. entry, snapshotLargest, snapshotHas, added = h.tracks.AddDownstreamSnapshotLargest(fullName, sub) @@ -557,6 +562,18 @@ func anyDownstreamForwards(entry *registry.TrackEntry) bool { func(d *registry.DownstreamSub) bool { return d.ForwardState() == 1 }) } +// resolveGroupOrder gives sub, when its request omitted GROUP_ORDER, the +// publisher's preference from entry's Track Properties (§10.2.8: "If omitted +// from SUBSCRIBE or SUBSCRIBE_TRACKS, the publisher's preference from the +// Track is used"; §12.5). Its fills (§10.2.15) and stream scheduling (§7.2) +// then follow it. GROUP_ORDER cannot appear in REQUEST_UPDATE, so this holds +// for the subscription's life. +func resolveGroupOrder(sub *registry.DownstreamSub, entry *registry.TrackEntry) { + if sub.GroupOrder == 0 { + sub.SetGroupOrder(uint8(entry.DefaultGroupOrder())) + } +} + // 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]). diff --git a/pkg/relay/internal/registry/dynamic_groups_test.go b/pkg/relay/internal/registry/dynamic_groups_test.go index 5c198b32..eea72b96 100644 --- a/pkg/relay/internal/registry/dynamic_groups_test.go +++ b/pkg/relay/internal/registry/dynamic_groups_test.go @@ -67,6 +67,43 @@ func TestTrackEntry_DynamicGroups(t *testing.T) { }) } +// TestTrackEntry_DefaultGroupOrder pins the §12.5 DEFAULT_PUBLISHER_GROUP_ORDER +// decode: Ascending when omitted, the value when allowed, and Ascending for a +// value outside {1, 2} or a malformed block, including one that would +// truncate to an allowed byte. +func TestTrackEntry_DefaultGroupOrder(t *testing.T) { + t.Parallel() + order := func(v uint64) []byte { + return message.AppendTrackProperties([]wire.KVPair{ + {Type: message.PropertyDefaultPublisherGroupOrder, IntVal: v}, + }) + } + for _, tc := range []struct { + name string + props []byte + want message.GroupOrder + }{ + {"absent", nil, message.GroupOrderAscending}, + {"Ascending", order(1), message.GroupOrderAscending}, + {"Descending", order(2), message.GroupOrderDescending}, + {"out of range", order(3), message.GroupOrderAscending}, + {"truncates to Descending", order(0x102), message.GroupOrderAscending}, + {"malformed block", []byte{0x40}, message.GroupOrderAscending}, + {"inside Immutable Properties", message.AppendTrackProperties([]wire.KVPair{ + {Type: message.PropertyImmutableProperties, ByteVal: order(2)}, + }), message.GroupOrderDescending}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + e := ®istry.TrackEntry{} + e.SetProperties(tc.props) + if got := e.DefaultGroupOrder(); got != tc.want { + t.Fatalf("DefaultGroupOrder() = %v, want %v", got, tc.want) + } + }) + } +} + // TestConsiderNewGroupRequest pins the §10.2.19 relay decision and its // outstanding-request bookkeeping. func TestConsiderNewGroupRequest(t *testing.T) { diff --git a/pkg/relay/internal/registry/properties.go b/pkg/relay/internal/registry/properties.go index 19c9e433..48b61c89 100644 --- a/pkg/relay/internal/registry/properties.go +++ b/pkg/relay/internal/registry/properties.go @@ -35,6 +35,10 @@ type decodedProperties struct { // property decodes to — so the zero DeliveryTimeouts is the correct // 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 message.GroupOrder } // decodeTrackProperties parses the raw Track Properties block once and pulls @@ -61,6 +65,11 @@ func decodeTrackProperties(raw []byte) decodedProperties { 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) + } } } return d @@ -112,3 +121,16 @@ func (e *TrackEntry) DeliveryTimeouts() message.DeliveryTimeouts { } return e.decoded.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. +func (e *TrackEntry) DefaultGroupOrder() message.GroupOrder { + e.mu.RLock() + defer e.mu.RUnlock() + if e.decoded.parseErr != nil || e.decoded.groupOrder == 0 { + return message.GroupOrderAscending + } + return e.decoded.groupOrder +} diff --git a/pkg/relay/internal/registry/subscription.go b/pkg/relay/internal/registry/subscription.go index a27b27c2..2f293aef 100644 --- a/pkg/relay/internal/registry/subscription.go +++ b/pkg/relay/internal/registry/subscription.go @@ -423,9 +423,9 @@ type DownstreamSub struct { // 0x1 = ascending, 0x2 = descending. It drives the group-order // tie-breaker in both reorder-capable paths (FETCH responses) and the // §7.2 rule-3 GroupKey of the subgroup-stream scheduling priority. - // Default per §7.1: the publisher's preference, which the relay does not - // currently track, so an unset GroupOrder is left at zero and treated as - // Ascending. + // Default per §7.1: the publisher's preference, which the relay fills in + // from the Track Properties before registering the subscription; zero + // until then, read as Ascending. GroupOrder uint8 // omitProperties records INCLUDE_PROPERTIES=0; see @@ -673,8 +673,7 @@ func (d *DownstreamSub) EffectiveStreamPriority( // §7.2 rule 3: Descending order means higher Group IDs are scheduled // first. Complementing the Group ID flips the numeric comparison so the // same "lower GroupKey is higher priority" rule yields that direction. - // An unset GROUP_ORDER (zero value) defaults to Ascending — §7.1 says - // the publisher's preference applies, which the relay does not track. + // An unresolved GROUP_ORDER (zero value) is read as Ascending. groupKey := groupID if order == message.GroupOrderDescending { groupKey = ^groupID From 1ed76dfbc124fdc5c0b0b29154112ebb7fe3ba3a Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 18:52:23 +0500 Subject: [PATCH 7/9] =?UTF-8?q?fix(relay):=20a=20forwarded=20PUBLISH=20sta?= =?UTF-8?q?tes=20the=20Group=20Order=20its=20subscription=20uses=20(=C2=A7?= =?UTF-8?q?10.20.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 5804df6. A SUBSCRIBE_TRACKS that omitted GROUP_ORDER got a forwarded PUBLISH with no GROUP_ORDER, although the relay now writes that subscription's fills in the track's DEFAULT_PUBLISHER_GROUP_ORDER. §10.20.1 says these parameters "are explicitly communicated in PUBLISH", and a holder with INCLUDE_PROPERTIES=0 had no other way to learn the order. The forwarded PUBLISH now always carries GROUP_ORDER: the holder's own, else the track's preference (§10.2.8, §12.5; Ascending when absent). The subscription takes its order from that PUBLISH, so what it was told and what its fills use cannot diverge if the track's Properties are first set in between. Chosen with the maintainer over "only when Descending". Both updated tests fail on the unpatched code (GROUP_ORDER absent where Ascending, and Descending, are expected). Co-Authored-By: Claude Opus 5.5 (1M context) --- pkg/relay/fill_group_order_test.go | 11 +++++++++-- pkg/relay/handler_forward.go | 6 +++++- pkg/relay/handler_publish.go | 11 ++++++++--- pkg/relay/session_publish_test.go | 11 ++++++----- 4 files changed, 28 insertions(+), 11 deletions(-) diff --git a/pkg/relay/fill_group_order_test.go b/pkg/relay/fill_group_order_test.go index 8e57e2ec..0fa6e846 100644 --- a/pkg/relay/fill_group_order_test.go +++ b/pkg/relay/fill_group_order_test.go @@ -74,12 +74,19 @@ func TestSubscribe_FillFollowsPublisherGroupOrder(t *testing.T) { } // TestSubscribeTracks_FillFollowsPublisherGroupOrder: a forwarded PUBLISH's -// subscription (§10.20.1). +// subscription (§10.20.1). The PUBLISH states the Group Order it resolved to, +// since §10.20.1 says these parameters "are explicitly communicated in +// PUBLISH". func TestSubscribeTracks_FillFollowsPublisherGroupOrder(t *testing.T) { t.Parallel() holder := dialAnotherClient(t, publishDescendingCam(t)) reqs := forwardedPublishes(t, holder) subscribeTracks(t, holder, ns("video"), fillWholeTrack...) - acceptForwarded(t, awaitForwarded(t, reqs)) + fwd := awaitForwarded(t, reqs) + if p, ok := fwd.First.(*message.Publish).Parameters.Find(message.ParamGroupOrder); !ok || + message.GroupOrder(p.Byte) != message.GroupOrderDescending { + t.Errorf("forwarded GROUP_ORDER = %d (present=%v), want Descending (0x2)", p.Byte, ok) + } + acceptForwarded(t, fwd) requireDescendingFill(t, holder) } diff --git a/pkg/relay/handler_forward.go b/pkg/relay/handler_forward.go index 6f2e60fd..7abb9e4c 100644 --- a/pkg/relay/handler_forward.go +++ b/pkg/relay/handler_forward.go @@ -85,7 +85,11 @@ func (h *sessionHandler) serveForwardedPublish( sub.OpenedByPublish() // handleSubscribeTracks already refused parameters this would reject. _ = installSubscribeParams(sub, params) - resolveGroupOrder(sub, te) + // The Group Order the PUBLISH stated (see publishParamsForSubscriber), so + // the subscription's fills follow what it was told (§10.20.1). + if p, ok := fwd.Parameters.Find(message.ParamGroupOrder); ok { + sub.SetGroupOrder(p.Byte) + } _, largest, has, added := h.tracks.AddDownstreamSnapshotLargest(fullName, sub) registered() if !added { diff --git a/pkg/relay/handler_publish.go b/pkg/relay/handler_publish.go index 1efdc5c2..34d8c57e 100644 --- a/pkg/relay/handler_publish.go +++ b/pkg/relay/handler_publish.go @@ -149,7 +149,10 @@ var notEchoedInPublish = []message.ParamID{ // Relays"): // - SUBSCRIBE_TRACKS parameters valid on PUBLISH are echoed (§10.20.1), // except AUTHORIZATION_TOKEN (§10.2.2); -// - FORWARD=0 and GROUP_ORDER only when the subscriber set them; +// - FORWARD=0 only when the subscriber set it; +// - GROUP_ORDER always: the subscriber's, else the publisher's preference +// (§10.2.8), which the subscription's fills follow; §10.20.1 has these +// "explicitly communicated in PUBLISH"; // - LARGEST_OBJECT is the relay's own watermark (§10.2.17). func publishParamsForSubscriber(tp *registry.TracksParams, entry *registry.TrackEntry) message.Parameters { var out message.Parameters @@ -164,9 +167,11 @@ func publishParamsForSubscriber(tp *registry.TracksParams, entry *registry.Track if !tp.Forward { out = append(out, message.ForwardParam(false)) } - if tp.GroupOrder != 0 { - out = append(out, message.GroupOrderParam(message.GroupOrder(tp.GroupOrder))) + order := message.GroupOrder(tp.GroupOrder) + if order == 0 { + order = entry.DefaultGroupOrder() } + out = append(out, message.GroupOrderParam(order)) if largest, ok := entry.GetLargest(); ok { out = append(out, message.LargestObjectParam(largest.Group, largest.Object)) } diff --git a/pkg/relay/session_publish_test.go b/pkg/relay/session_publish_test.go index fd72b87d..06a4187b 100644 --- a/pkg/relay/session_publish_test.go +++ b/pkg/relay/session_publish_test.go @@ -119,14 +119,15 @@ func TestPublish_ForwardsToSubscribeTracks(t *testing.T) { if pub.TrackAlias == 0 { t.Fatal("forwarded TrackAlias is 0; want one allocated on the subscriber's session") } - // §10.20.1: the SUBSCRIBE_TRACKS omitted FORWARD and GROUP_ORDER, so the - // forwarded PUBLISH carries neither (FORWARD defaults to 1, GROUP_ORDER to - // the publisher's preference). + // §10.20.1: the SUBSCRIBE_TRACKS omitted FORWARD and GROUP_ORDER. + // FORWARD defaults to 1 and is omitted; GROUP_ORDER is the publisher's + // preference (§10.2.8), Ascending for this track (§12.5), and stated. if p, ok := pub.Parameters.Find(message.ParamForward); ok { t.Errorf("forwarded FORWARD present (=%d), want omitted", p.Byte) } - if p, ok := pub.Parameters.Find(message.ParamGroupOrder); ok { - t.Errorf("forwarded GROUP_ORDER present (=%d), want omitted", p.Byte) + if p, ok := pub.Parameters.Find(message.ParamGroupOrder); !ok || + message.GroupOrder(p.Byte) != message.GroupOrderAscending { + t.Errorf("forwarded GROUP_ORDER = %d (present=%v), want Ascending (0x1)", p.Byte, ok) } } From 90c973030cabf3a9db54435844fdb15319f0417e Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 19:27:08 +0500 Subject: [PATCH 8/9] =?UTF-8?q?fix(relay):=20a=20FETCH=20from=20the=20cach?= =?UTF-8?q?e=20says=20an=20Object=20does=20not=20exist=20only=20on=20a=20s?= =?UTF-8?q?ignal=20(=C2=A72.1,=20=C2=A710.13)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A FETCH answered from the relay's cache split the range at the cache's eviction floor and took every uncached Location above it as non-existent: the response left it out and FINed, and a gap in a FIN-terminated FETCH says the Object does not exist (§10.13). A lost datagram, a subgroup stream reset part-way, a sibling subgroup still in flight, or simply a Group's tail left such holes. §2.1: "A gap in the observed Object IDs does not by itself convey any information about the skipped Objects", and §10.13: a relay meeting an uncached Object of unknown status "MUST pause subsequent delivery until it has confirmed the object's status upstream". The range is now classified (fetch_ranges.go). A Location is known not to exist only on a signal: a Prior Group or Object ID Gap (§12.8, §12.9), an END_OF_GROUP or END_OF_TRACK status (§11.2.1.1), an END_OF_GROUP bit on a FINed subgroup (§11.4.2, from the ledger: TrackEntry.KnownAbsent), or an upstream's FETCH answer. Every other uncached Location is unknown. With a fetch-capable upstream, one FETCH covers the span from the first unknown Location to the last, within FILL_TIMEOUT (§10.2.5): its Objects fill the holes, and what it marks unknown or timed out stays so where the relay has no signal of its own; the relay's cached Objects and known-absent Locations stand either way. Without one, or when it fails or times out, the cache is served with End of Unknown (or Timed-Out) Range markers over each unknown run (§11.4.4.2; §10.13 "indicate the range of unknown Objects and continue"). Decisions made with the maintainer: - only explicit signals establish non-existence; - one upstream FETCH over the span of the holes; - an End of Range marker covers the Locations after the previous element in the order the response carries them (Groups in its Group Order, Object IDs ascending within a Group), so in Descending order a Group's unknown tail is marked at {G, 2^64-1}. This replaces the descending whole-range marker at the range Start, and is recorded as a Limitation. Consequences, recorded as a Limitation: a publisher that marks no Group ends gets an End of Unknown Range after every Group in a cached FETCH or fill, or an upstream FETCH for them. mergeFetchObjects, the whole-range marker helpers and cache.OldestRetained are gone; GetRange skips expired Objects instead of marking them by the floor. An upstream element past its own FETCH_OK End Location now discards its answer (§10.14), and a hard end after Object 2^64-1 records nothing instead of wrapping to 0. The regression tests in fetch_holes_test.go fail on the unpatched code: a hole inside a Group and a Group tail without an end are served as plain gaps in both orders, and the upstream is never asked. Fetch test topologies that publish single-Object Groups now set END_OF_GROUP, and wait for their upstream to finish writing before fetching. -race clean; the fetch tests pass 10x. BenchmarkFetchFromCache goes from 118 to 120 allocs/op: the classification's slices, per FETCH, not per Object. Co-Authored-By: Claude Opus 5.5 (1M context) --- STATUS.md | 30 +- pkg/relay/cache/cache.go | 55 +- pkg/relay/cache/cache_test.go | 38 +- pkg/relay/cache_test.go | 15 +- pkg/relay/fetch_holes_test.go | 331 ++++++++++++ pkg/relay/fetch_ranges.go | 175 ++++++ pkg/relay/handler_fetch.go | 501 ++++++++---------- pkg/relay/handler_fetch_elem_test.go | 106 ++-- pkg/relay/handler_fetch_merge_test.go | 189 ------- pkg/relay/handler_fetch_ranges_test.go | 193 +++++++ pkg/relay/handler_fetch_split_test.go | 35 -- pkg/relay/handler_fetch_stitch_test.go | 13 +- pkg/relay/handler_fetch_unknown_test.go | 32 +- pkg/relay/handler_fetch_upstream_fail_test.go | 55 +- pkg/relay/helpers_fetch_test.go | 29 +- pkg/relay/inbound_goaway_test.go | 16 + pkg/relay/internal/registry/ledger_test.go | 17 + pkg/relay/internal/registry/track_entry.go | 45 +- pkg/relay/malformed_track_test.go | 7 +- pkg/relay/session_handler.go | 4 +- 20 files changed, 1145 insertions(+), 741 deletions(-) create mode 100644 pkg/relay/fetch_holes_test.go create mode 100644 pkg/relay/fetch_ranges.go delete mode 100644 pkg/relay/handler_fetch_merge_test.go create mode 100644 pkg/relay/handler_fetch_ranges_test.go diff --git a/STATUS.md b/STATUS.md index 4cf1e624..514048f2 100644 --- a/STATUS.md +++ b/STATUS.md @@ -62,8 +62,8 @@ By package, bottom-up along the dependency stack: - **`relay`** — routes objects through a track registry with per-subscription live fanout under a §8 slow-reader policy, merges multiple upstream publishers per track (§9.5) with §2.1 {Group, Object} dedup and survivor-continues - failover, serves FETCHes from a per-track cache (stitching evicted ranges from - an upstream FETCH), issues on-demand upstream SUBSCRIBEs to every matching + failover, serves FETCHes from a per-track cache (asking an upstream FETCH + about what the cache cannot vouch for), issues on-demand upstream SUBSCRIBEs to every matching publisher (local and, via a `DiscoveryStore` + `Dialer`, remote), reflects remote namespaces to local subscribers, gates requests through an `Authorizer` hook, emits telemetry through a `Metrics` hook, and drains sessions with @@ -197,7 +197,7 @@ By package, bottom-up along the dependency stack: | 10.10 | PUBLISH_STATE_NOTIFY | 0x22 | DONE | Only the publisher may send it; enforced by brokers and the relay. | | 10.11 | PUBLISH | 0x1D | DONE | | | 10.12 | PUBLISH_DONE | 0x0B | DONE | Sent once every stream of the subscription has closed and no datagram send is in progress, with the exact Stream Count; written on its own goroutine, so subscribers do not wait on each other. When a track's last upstream ends, its PUBLISH_DONE code reaches subscribers if it is about the track (TRACK_ENDED, MALFORMED_TRACK); codes about the relay's own upstream subscription become INTERNAL_ERROR. | -| 10.13 | FETCH | 0x16 | DONE | Standalone, the only kind in draft-20. | +| 10.13 | FETCH | 0x16 | DONE | Standalone, the only kind in draft-20. From the cache, a Location is non-existent only on a signal: a Prior Group or Object ID Gap, a Group's or the Track's end, or an upstream's FETCH. Other uncached Locations are FETCHed from a fetch-capable upstream in one span, within FILL_TIMEOUT, or else marked End of Unknown (or Timed-Out) Range. | | 10.14 | FETCH_OK | 0x18 | DONE | An End Location before the FETCH's Start closes the session. A Start relative to the Largest Object is compared through End ≤ Largest; an End of {0,0} is let through, as it cannot be told apart from "no content yet". | | 10.15 | TRACK_STATUS | 0x0D | DONE | Reply via REQUEST_OK, then FIN; any follow-up from the requester closes the session. | | 10.16 | PUBLISH_NAMESPACE | 0x06 | DONE | | @@ -222,7 +222,7 @@ By package, bottom-up along the dependency stack: | 11.4.3 | Closing subgroup streams | DONE | Relay forwards only the next object on a stream, otherwise reset+reopen: the next object is one ID greater, read next from the same upstream stream (only filtered-out objects between), or covered by its Prior Object ID Gap; an object the relay dropped, or one from another upstream, breaks the run. FINs on clean inbound EOF, resets on inbound reset, resets with MALFORMED_TRACK after a terminal EndOfGroup/EndOfTrack object (§2.4.2), marks reliable boundaries for RESET_STREAM_AT (`SetReliableBoundary`, transport-gated on `EnableStreamResetPartialDelivery`), and resets (not FINs) in-flight subgroups whose group falls out of range after a narrowing REQUEST_UPDATE. A subscription that skipped any Object of the Subgroup other than one before its Start Location (a filter, Forward State 0, a Start raised past Objects already sent, an inbox overflow with EXCESSIVE_LOAD, an expiry) gets resets, never a FIN, on that Subgroup's streams. Objects published before a subscription joined are treated as before its Start. | | 11.4.4 | Fetch header | DONE | Serialization Flags of 128 or more that are not an End of Range close the session. | | 11.4.4.1 | Fetch flags | DONE | All subgroup modes + delta/priority/properties/status flags. A first Object that references a prior Object's fields closes the session. | -| 11.4.4.2 | End of range | DONE | Non-existent (0x8C) / unknown (0x10C) handled. An Object after a leading marker that references a prior Subgroup ID or Priority closes the session. | +| 11.4.4.2 | End of range | DONE | Non-existent (0x8C) / unknown (0x10C) / timed-out (0x20C) handled; a marker covers the Locations after the previous element in the order the response carries them (see Limitations). An Object after a leading marker that references a prior Subgroup ID or Priority closes the session. | | 11.5 | Padding streams & datagrams | DONE | Recognised type IDs silently discarded. | ## §12 MOQT properties @@ -490,6 +490,20 @@ Known protocol gaps, roughly ordered by how load-bearing they are: Properties, and neither SUBSCRIBE_OK nor the FETCH_HEADER carries a Group Order, so it cannot tell a Descending fill from an Ascending one. A gap in the draft; a subscriber that asks for a fill avoids it by sending GROUP_ORDER. +- **FETCH End of Range markers in Descending order (§11.4.4.2)** — an + interpretation. A marker covers "Locations between the last serialized + Object, if any, and this Location"; the relay reads "between" in the order + the response carries Locations (Groups in its Group Order, Object IDs + ascending within a Group), so in Descending order a Group's unknown tail is + marked at {G, 2^64-1} after its Objects. The draft does not say which order + it means. Interop with moxygen and moqtail is unverified. +- **FETCH from the cache and publishers that do not mark Group ends (§2.1, + §10.13)** — the relay treats a Group's tail as unknown unless an + END_OF_GROUP or END_OF_TRACK status, an END_OF_GROUP bit on a FINed + subgroup, or a gap Property says where it ends. A publisher that marks none + gets an End of Unknown Range after every Group in a cached FETCH or fill, or, + behind a fetch-capable upstream, an upstream FETCH for them. Group ends known + only from a subgroup FIN are kept for the last 32 Groups. - **Fill streams are not scheduled against their subscription (§7.2 rules 3 and 4)** — a subscription-delivered Object should go first when the fill's Group Order differs, and the fill-delivered one first within a Group. The @@ -506,14 +520,6 @@ A second full review against draft-ietf-moq-transport-20 (2026-09-26, at `dbe571e`) found the gaps below. Each item names the rule it misses. Items already listed as Limitations above are not repeated here. -High: - -- A FETCH answered from the cache treats every Object missing above the - eviction floor as non-existent and FINs the stream, although a lost datagram, - a reset subgroup stream or a subgroup still in flight leaves such holes. §10.13: - a relay that meets an uncached Object of unknown status "MUST pause subsequent - delivery until it has confirmed the object's status upstream". - Session layer: - DUPLICATE_TRACK_ALIAS never closes the session: the relay answers REQUEST_ERROR diff --git a/pkg/relay/cache/cache.go b/pkg/relay/cache/cache.go index 231343d6..07ee9000 100644 --- a/pkg/relay/cache/cache.go +++ b/pkg/relay/cache/cache.go @@ -1,5 +1,5 @@ -// Package cache holds the relay's per-track Object Cache (§9.4 fetch -// support). Storage is a fixed-capacity circular ring buffer (FIFO) with +// Package cache holds the relay's per-track Object Cache (§9.1 caching +// relays). Storage is a fixed-capacity circular ring buffer (FIFO) with // an auxiliary {GroupID, ObjectID} → ring-slot index map for O(1) point // lookup and overwrite-in-place. // @@ -115,7 +115,7 @@ type cacheKey struct { // cache. Each [TrackEntry] holds one. // // Concurrency: an RWMutex guards the ring. Writes (Put / Delete) take the -// write lock and are O(1); reads (Get / GetRange / OldestRetained / Len) +// write lock and are O(1); reads (Get / GetRange / Len) // take the read lock. FETCH reads are // O(capacity) (default 1024), so the read lock lets concurrent FETCHes — the // flash-crowd-of-joining-subscribers case the relay is built for — scan in @@ -339,7 +339,6 @@ func (c *ObjectCache) GetRange(start, end message.Location, order message.GroupO return nil } c.mu.RLock() - floor, hasFloor := c.oldestRetainedLocked() out := make([]*CachedObject, 0) for _, obj := range c.ring { if obj == nil { @@ -350,13 +349,7 @@ func (c *ObjectCache) GetRange(start, end message.Location, order message.GroupO continue } if !c.notExpiredLocked(obj) { - // §12.3: expired state "becomes unknown". Above the floor a - // plain gap would assert non-existence (§11.4.4), so mark it; - // below it the caller accounts for the span (OldestRetained). - if hasFloor && floor.Less(loc) { - out = append(out, &CachedObject{GroupID: obj.GroupID, ObjectID: obj.ObjectID, EndOfUnknownRange: true}) - } - continue + continue // §12.3: its state "becomes unknown"; the caller treats it so } // Append the stored pointer directly — Put never recycles or // mutates a stored struct, so this never aliases storage a later @@ -368,46 +361,6 @@ func (c *ObjectCache) GetRange(start, end message.Location, order message.GroupO return out } -// OldestRetained returns the lowest Location currently held by the cache — -// the eviction floor — and a bool that is false when the cache holds no live -// object. -// -// Because the ring evicts oldest-first and objects are stored in (broadly -// increasing) arrival order, the retained set is a suffix of the track by -// Location: everything below OldestRetained has either been evicted by size -// or TTL pressure, or was never cached by this relay. Either way the relay -// does not hold it. A FETCH responder uses this boundary to decide which part -// of a requested range it can answer from cache and which part it must stitch -// from upstream — a gap below the floor is "maybe exists upstream", whereas a -// gap at or above the floor is ground-truth non-existence. -// -// Like [ObjectCache.GetRange], this is an O(capacity) scan; FETCH is not the -// hot path. -func (c *ObjectCache) OldestRetained() (message.Location, bool) { - c.mu.RLock() - defer c.mu.RUnlock() - return c.oldestRetainedLocked() -} - -// oldestRetainedLocked is OldestRetained with c.mu held. -func (c *ObjectCache) oldestRetainedLocked() (message.Location, bool) { - var ( - oldest message.Location - found bool - ) - for _, obj := range c.ring { - if obj == nil || !c.notExpiredLocked(obj) { - continue - } - loc := message.Location{Group: obj.GroupID, Object: obj.ObjectID} - if !found || loc.Less(oldest) { - oldest = loc - found = true - } - } - return oldest, found -} - // sortObjects sorts in-place by (group, object). Group direction is // controlled by order; objects within a group are always ascending. // An unknown GroupOrder falls back to ascending. diff --git a/pkg/relay/cache/cache_test.go b/pkg/relay/cache/cache_test.go index 846fc41f..0f3921f4 100644 --- a/pkg/relay/cache/cache_test.go +++ b/pkg/relay/cache/cache_test.go @@ -24,36 +24,6 @@ func locs(objs []*cache.CachedObject) []message.Location { return out } -// TestObjectCache_OldestRetained pins the eviction-floor accessor: false on an -// empty cache, the minimum live Location otherwise, and an advancing floor as -// size pressure evicts the oldest entries. -func TestObjectCache_OldestRetained(t *testing.T) { - t.Parallel() - - c := cache.NewObjectCache(0, 0) - if _, ok := c.OldestRetained(); ok { - t.Fatal("empty cache must report no oldest retained") - } - - // Insert out of order; the floor is the minimum Location regardless. - putAt(c, 5, 2) - putAt(c, 3, 0) - putAt(c, 7, 1) - if got, ok := c.OldestRetained(); !ok || got != (message.Location{Group: 3, Object: 0}) { - t.Fatalf("OldestRetained = %v, %v; want {3 0}, true", got, ok) - } - - // A size-bounded cache evicts oldest-first, so the floor advances as - // newer groups push the earliest insert out of the ring. - small := cache.NewObjectCache(2, 0) - putAt(small, 1, 0) - putAt(small, 2, 0) - putAt(small, 3, 0) // evicts {1,0} - if got, ok := small.OldestRetained(); !ok || got != (message.Location{Group: 2, Object: 0}) { - t.Fatalf("after eviction OldestRetained = %v, %v; want {2 0}, true", got, ok) - } -} - // TestObjectCache_GetRange_Order pins GetRange's sort: groups in the requested // order, Objects ascending within a group (§10.13). Inserts are scrambled so // FIFO order cannot pass. @@ -168,8 +138,8 @@ func TestObjectCache_Delete(t *testing.T) { } // TestPerObjectMaxCacheDuration: each Object keeps its upstream's -// MAX_CACHE_DURATION (§12.3). Expired above the oldest served Object it reads -// as End of Unknown Range; a present 0 is never served; absent means relay TTL. +// MAX_CACHE_DURATION (§12.3). An expired one is not returned; a present 0 is +// never served; absent means relay TTL. func TestPerObjectMaxCacheDuration(t *testing.T) { c := cache.NewObjectCache(16, 0) put := func(group uint64, maxAge time.Duration, has bool) *cache.CachedObject { @@ -182,7 +152,7 @@ func TestPerObjectMaxCacheDuration(t *testing.T) { c.Put(o) return o } - put(0, 10*time.Millisecond, true) // expires, below the floor + put(0, 10*time.Millisecond, true) // expires put(1, 0, false) // never expires short := put(2, 10*time.Millisecond, true) put(3, 0, false) @@ -197,7 +167,7 @@ func TestPerObjectMaxCacheDuration(t *testing.T) { } got = append(got, fmt.Sprintf("%d:%s", o.GroupID, kind)) } - want := []string{"1:obj", "2:unknown", "3:obj", "4:unknown"} + want := []string{"1:obj", "3:obj"} if !slices.Equal(got, want) { t.Fatalf("GetRange = %v, want %v", got, want) } diff --git a/pkg/relay/cache_test.go b/pkg/relay/cache_test.go index 03ddf918..c4d586c2 100644 --- a/pkg/relay/cache_test.go +++ b/pkg/relay/cache_test.go @@ -266,9 +266,18 @@ func TestRelay_MaxCacheDurationExpiredObjectIsUnknown(t *testing.T) { pubB := dialAnotherClient(t, pubA) publishVideoTrackProps(t, pubB, "cam1", 9, nil) newCam1Subscriber(t, pubA) - publishObjects(t, pubB, 9, 1, 1) - publishObjects(t, pubA, aliasA, 2, 1) - publishObjects(t, pubB, 9, 3, 1) + // END_OF_GROUP, so the Groups' ends are known and only the expired + // Object is unknown. + for _, p := range []struct { + sess *session.Session + alias, group uint64 + }{{pubB, 9, 1}, {pubA, aliasA, 2}, {pubB, 9, 3}} { + hdr := subgroupHeader(p.alias, p.group) + hdr.EndOfGroup = true + if err := writeSubgroup(p.sess, hdr, 1); err != nil { + t.Fatal(err) + } + } time.Sleep(3 * maxCacheMs * time.Millisecond) elems := fetchCam1(t, dialAnotherClient(t, pubA), 3) diff --git a/pkg/relay/fetch_holes_test.go b/pkg/relay/fetch_holes_test.go new file mode 100644 index 00000000..0d7ddb0a --- /dev/null +++ b/pkg/relay/fetch_holes_test.go @@ -0,0 +1,331 @@ +package relay_test + +import ( + "math" + "slices" + "sync/atomic" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// A FETCH served from the relay's cache says an Object does not exist only +// when the relay knows it (§2.1: "A gap in the observed Object IDs does not by +// itself convey any information about the skipped Objects"). Any other +// uncached Location is asked of an upstream (§10.13: "the relay MUST pause +// subsequent delivery until it has confirmed the object's status upstream"), +// or, with none to ask, marked End of Unknown Range (§11.4.4.2). + +// cam1Object is one Object of video/cam1 to publish: its Location, and its +// Object Properties. +type cam1Object struct { + group, object uint64 + props []byte +} + +// publishCam1Group writes objs, all of one Group and ascending, on one +// subgroup stream of alias, with END_OF_GROUP when ended, and FINs it. +func publishCam1Group(t *testing.T, sess *session.Session, alias uint64, ended bool, objs ...cam1Object) { + t.Helper() + hdr := subgroupHeader(alias, objs[0].group) + hdr.EndOfGroup = ended + hdr.Properties = slices.ContainsFunc(objs, func(o cam1Object) bool { return o.props != nil }) + sg, err := openSubgroupWaiting(t, sess, hdr) + if err != nil { + t.Fatalf("OpenSubgroup: %v", err) + } + for i, o := range objs { + delta := o.object + if i > 0 { + delta = o.object - objs[i-1].object - 1 + } + if err := sg.WriteObject(&message.SubgroupObject{ + ObjectIDDelta: delta, Properties: o.props, Payload: []byte("x"), + }); err != nil { + t.Fatalf("WriteObject: %v", err) + } + } + if err := sg.Close(); err != nil { + t.Fatalf("Close: %v", err) + } +} + +// fetchCam1Range FETCHes [start, end] of video/cam1 in order and returns the +// response elements. +func fetchCam1Range( + t *testing.T, + sess *session.Session, + start, end message.Location, + order message.GroupOrder, +) []fetchElem { + t.Helper() + params := message.Parameters{fetchRangeFilter(start, end)} + if order == message.GroupOrderDescending { + params = append(params, message.GroupOrderParam(order)) + } + fr, err := sess.Fetch(t.Context(), &message.Fetch{Namespace: ns("video"), Name: []byte("cam1"), Parameters: params}) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + defer fr.Close() + return collectFetchElems(t, sess, order, 3*time.Second) +} + +// obj and unknownAt are the fetchElems of an Object and of an End of Unknown +// Range marker. +func obj(g, o uint64) fetchElem { return fetchElem{Group: g, Object: o} } +func unknownAt(g, o uint64) fetchElem { + return fetchElem{Group: g, Object: o, Unknown: true, Marker: true} +} + +// timedOutAt is the fetchElem of an End of Timed-Out Range marker. +func timedOutAt(g, o uint64) fetchElem { return fetchElem{Group: g, Object: o, Marker: true} } + +// TestFetch_CacheHolesWithoutUpstream: with no upstream to ask, an uncached +// Location the relay knows nothing of is marked unknown, in the order the +// response carries it (Groups descending, Objects ascending within one); a +// known one is a plain gap. +func TestFetch_CacheHolesWithoutUpstream(t *testing.T) { + t.Parallel() + const maxID = math.MaxUint64 + for _, tc := range []struct { + name string + publish func(t *testing.T, sess *session.Session, alias uint64) + end message.Location + order message.GroupOrder + want []fetchElem + }{ + { + name: "hole inside a Group", + publish: func(t *testing.T, sess *session.Session, alias uint64) { + publishCam1Group(t, sess, alias, true, cam1Object{0, 0, nil}, cam1Object{0, 1, nil}, cam1Object{0, 3, nil}) + }, + end: message.Location{Group: 0, Object: 3}, + want: []fetchElem{obj(0, 0), obj(0, 1), unknownAt(0, 2), obj(0, 3)}, + }, + { + name: "Group tail with no end", + publish: func(t *testing.T, sess *session.Session, alias uint64) { + publishCam1Group(t, sess, alias, false, cam1Object{0, 0, nil}, cam1Object{0, 1, nil}) + publishCam1Group(t, sess, alias, false, cam1Object{1, 0, nil}) + }, + end: message.Location{Group: 1, Object: 0}, + want: []fetchElem{obj(0, 0), obj(0, 1), unknownAt(0, maxID), obj(1, 0)}, + }, + { + name: "Group tail with no end, Descending", + publish: func(t *testing.T, sess *session.Session, alias uint64) { + publishCam1Group(t, sess, alias, false, cam1Object{0, 0, nil}, cam1Object{0, 1, nil}) + publishCam1Group(t, sess, alias, false, cam1Object{1, 0, nil}) + }, + end: message.Location{Group: 1, Object: 0}, + order: message.GroupOrderDescending, + want: []fetchElem{obj(1, 0), obj(0, 0), obj(0, 1), unknownAt(0, maxID)}, + }, + { + name: "Groups ended by END_OF_GROUP", + publish: func(t *testing.T, sess *session.Session, alias uint64) { + publishCam1Group(t, sess, alias, true, cam1Object{0, 0, nil}, cam1Object{0, 1, nil}) + publishCam1Group(t, sess, alias, true, cam1Object{1, 0, nil}) + }, + end: message.Location{Group: 1, Object: 0}, + want: []fetchElem{obj(0, 0), obj(0, 1), obj(1, 0)}, + }, + { + name: "hole a Prior Object ID Gap announces", + publish: func(t *testing.T, sess *session.Session, alias uint64) { + publishCam1Group(t, sess, alias, true, cam1Object{0, 0, nil}, cam1Object{0, 1, nil}, + cam1Object{0, 3, priorGap(message.PropertyPriorObjectIDGap, 1)}) + }, + end: message.Location{Group: 0, Object: 3}, + want: []fetchElem{obj(0, 0), obj(0, 1), obj(0, 3)}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + pubSess, alias := newCam1Publisher(t, nil) + tc.publish(t, pubSess, alias) + time.Sleep(50 * time.Millisecond) // the relay caches them + + order := tc.order + if order == 0 { + order = message.GroupOrderAscending + } + got := fetchCam1Range(t, dialAnotherClient(t, pubSess), message.Location{}, tc.end, order) + if !slices.Equal(got, tc.want) { + t.Fatalf("FETCH elements %v, want %v", got, tc.want) + } + }) + } +} + +// TestFetch_CacheHoleAskedUpstream: with a fetch-capable upstream, the relay +// FETCHes an uncached Location of unknown status from it, and its answer +// decides: the Object it sends is served, and a gap under its FIN is one. +func TestFetch_CacheHoleAskedUpstream(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + serve bool // whether the upstream has Object {0, 2} + want []fetchElem + }{ + {"upstream has it", true, []fetchElem{obj(0, 0), obj(0, 1), obj(0, 2), obj(0, 3)}}, + {"upstream says it does not exist", false, []fetchElem{obj(0, 0), obj(0, 1), obj(0, 3)}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + upSess, teardown := connectRelay(t, relay.Config{}) + t.Cleanup(teardown) + if _, err := upSess.PublishNamespace( + t.Context(), + &message.PublishNamespace{Namespace: ns("video")}, + ); err != nil { + t.Fatalf("PublishNamespace: %v", err) + } + var asked atomic.Int32 + go func() { + for { + req, err := upSess.AcceptRequest(t.Context()) + if err != nil { + return + } + switch m := req.First.(type) { + case *message.Subscribe: + if req.Reply(&message.SubscribeOK{TrackAlias: 42}) != nil { + return + } + // The live stream misses Object 2. + publishCam1Group(t, upSess, 42, true, + cam1Object{0, 0, nil}, cam1Object{0, 1, nil}, cam1Object{0, 3, nil}) + case *message.Fetch: + asked.Add(1) + if req.Reply(&message.FetchOK{EndLocation: message.Location{Group: 0, Object: 3}}) != nil { + return + } + out, err := upSess.OpenFetchStream(message.FetchHeader{RequestID: m.RequestID}) + if err != nil { + return + } + if tc.serve { + _ = out.WriteObject(&message.FetchObject{ + SerializationFlags: message.FetchFlagGroupIDDelta | message.FetchFlagObjectIDDelta | + message.FetchFlagPriority | uint64(message.FetchSubgroupIDExplicit), + GroupIDDelta: 0, ObjectIDDelta: 2, ObjectPayload: []byte("x"), + }) + } + _ = out.Close() + } + } + }() + live := dialAnotherClient(t, upSess) + subscribeCam1(t, live) + go drainAll(t.Context(), live) + fc := dialAnotherClient(t, upSess) + waitRelayLargest(t, fc, ns("video"), []byte("cam1"), 0, 3) + + got := fetchCam1Range(t, fc, message.Location{}, message.Location{Group: 0, Object: 3}, + message.GroupOrderAscending) + if !slices.Equal(got, tc.want) { + t.Fatalf("FETCH elements %v, want %v", got, tc.want) + } + if asked.Load() != 1 { + t.Fatalf("the relay asked the upstream %d times, want once", asked.Load()) + } + }) + } +} + +// TestFetch_UpstreamUnknownKeepsWhatTheRelayKnows: an upstream that marks the +// whole span unknown, or timed out, does not unsay what the relay knows: its +// cached Objects are served, and a Group tail an END_OF_GROUP ended stays a +// gap. Only the holes are marked, with the upstream's kind, in either order. +func TestFetch_UpstreamUnknownKeepsWhatTheRelayKnows(t *testing.T) { + t.Parallel() + const maxID = math.MaxUint64 + for _, tc := range []struct { + name string + order message.GroupOrder + marker message.Location // the upstream's, at the span's end in stream order + flags uint64 + want []fetchElem + }{ + { + "Ascending", message.GroupOrderAscending, message.Location{Group: 1, Object: 1}, + message.FetchEndOfUnknownRange, + []fetchElem{obj(0, 0), unknownAt(0, 1), obj(0, 2), obj(1, 0), unknownAt(1, 1), obj(1, 2)}, + }, + { + "Descending", message.GroupOrderDescending, message.Location{Group: 0, Object: maxID}, + message.FetchEndOfUnknownRange, + []fetchElem{obj(1, 0), unknownAt(1, 1), obj(1, 2), obj(0, 0), unknownAt(0, 1), obj(0, 2)}, + }, + { + "Timed-Out", message.GroupOrderAscending, message.Location{Group: 1, Object: 1}, + message.FetchEndOfTimedOutRange, + []fetchElem{obj(0, 0), timedOutAt(0, 1), obj(0, 2), obj(1, 0), timedOutAt(1, 1), obj(1, 2)}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + upSess, teardown := connectRelay(t, relay.Config{}) + t.Cleanup(teardown) + if _, err := upSess.PublishNamespace( + t.Context(), + &message.PublishNamespace{Namespace: ns("video")}, + ); err != nil { + t.Fatalf("PublishNamespace: %v", err) + } + var asked atomic.Int32 + go func() { + for { + req, err := upSess.AcceptRequest(t.Context()) + if err != nil { + return + } + switch m := req.First.(type) { + case *message.Subscribe: + if req.Reply(&message.SubscribeOK{TrackAlias: 42}) != nil { + return + } + // Each Group misses Object 1 and ends after Object 2. + for g := range uint64(2) { + publishCam1Group(t, upSess, 42, true, cam1Object{g, 0, nil}, cam1Object{g, 2, nil}) + } + case *message.Fetch: + asked.Add(1) + _, end, _ := fetchRequestRange(m) + if req.Reply(&message.FetchOK{EndLocation: end}) != nil { + return + } + out, err := upSess.OpenFetchStream(message.FetchHeader{RequestID: m.RequestID}) + if err != nil { + return + } + _ = out.WriteObject(&message.FetchObject{ + SerializationFlags: tc.flags, + GroupIDDelta: tc.marker.Group, ObjectIDDelta: tc.marker.Object, + }) + _ = out.Close() + } + } + }() + live := dialAnotherClient(t, upSess) + subscribeCam1(t, live) + go drainAll(t.Context(), live) + fc := dialAnotherClient(t, upSess) + waitRelayLargest(t, fc, ns("video"), []byte("cam1"), 1, 2) + time.Sleep(50 * time.Millisecond) // the relay caches Group 0 too + + got := fetchCam1Range(t, fc, message.Location{}, message.Location{Group: 1, Object: 2}, tc.order) + if !slices.Equal(got, tc.want) { + t.Fatalf("FETCH elements %v, want %v", got, tc.want) + } + if asked.Load() != 1 { + t.Fatalf("the relay asked the upstream %d times, want once", asked.Load()) + } + }) + } +} diff --git a/pkg/relay/fetch_ranges.go b/pkg/relay/fetch_ranges.go new file mode 100644 index 00000000..6c43e36c --- /dev/null +++ b/pkg/relay/fetch_ranges.go @@ -0,0 +1,175 @@ +package relay + +import ( + "cmp" + "math" + "slices" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/relay/cache" + "github.com/floatdrop/moq-go/pkg/relay/internal/registry" +) + +// A FETCH response asserts what it does not carry: "Any gaps in the Group and +// Object IDs in the response stream indicate objects that do not exist" +// (§10.13), and "All signals that an Object does not exist are +// authoritative" (§2.1). The relay knows an Object does not exist only from a +// signal — a Prior Group or Object ID Gap (§12.8, §12.9), a Group's or the +// Track's end (§11.2.1.1, §11.4.2), or an upstream's FETCH response — never from a gap in +// what it happened to receive: "A gap in the observed Object IDs does not by +// itself convey any information about the skipped Objects" (§2.1). So a +// response is built from the requested range classified into Objects, known +// absent Locations, and the rest, whose status is unknown and is either asked +// of an upstream (§10.13) or marked with an End of Unknown Range (§11.4.4.2). +// +// Interpretation: an End of Range marker covers "Locations between the last +// serialized Object, if any, and this Location" (§11.4.4.2) in the order the +// response carries them: Groups in its Group Order, Object IDs ascending +// within a Group. In Descending order that is not Location order. + +// locSucc returns the Location right after l, and false when l is the last. +func locSucc(l message.Location) (message.Location, bool) { + switch { + case l.Object < math.MaxUint64: + return message.Location{Group: l.Group, Object: l.Object + 1}, true + case l.Group < math.MaxUint64: + return message.Location{Group: l.Group + 1}, true + } + return message.Location{}, false +} + +// uncovered returns the parts of [start, end] no range in known covers, in +// ascending order. It sorts known. +func uncovered(start, end message.Location, known []registry.LocRange) []registry.LocRange { + slices.SortFunc(known, func(a, b registry.LocRange) int { return a.Lo.Compare(b.Lo) }) + var out []registry.LocRange + cur := start + for _, k := range known { + if k.Hi.Less(cur) { + continue + } + if end.Less(k.Lo) { + break + } + if cur.Less(k.Lo) { + pred, _ := fetchPredecessor(k.Lo) // k.Lo > cur, so it has one + out = append(out, registry.LocRange{Lo: cur, Hi: pred}) + } + next, ok := locSucc(k.Hi) + if !ok || end.Less(next) { + return out + } + cur = next + } + return append(out, registry.LocRange{Lo: cur, Hi: end}) +} + +// knownFromCache is what objs, Objects the cache holds, establish: each +// Object's Location; from an END_OF_GROUP or END_OF_TRACK status, the rest of +// its Group or of the Track (§11.2.1.1); and the gaps its Prior Group and +// Object ID Gaps announce (§12.8, §12.9). [message.CheckObjectProperties] +// rejected a gap above an Object's ID on receipt. +func knownFromCache(objs []*cache.CachedObject) []registry.LocRange { + out := make([]registry.LocRange, 0, len(objs)) + for _, o := range objs { + loc := message.Location{Group: o.GroupID, Object: o.ObjectID} + r := registry.LocRange{Lo: loc, Hi: loc} + switch o.Status { + case message.ObjectStatusEndOfGroup: + r.Hi.Object = math.MaxUint64 + case message.ObjectStatusEndOfTrack: + r.Hi = message.Location{Group: math.MaxUint64, Object: math.MaxUint64} + } + out = append(out, r) + gaps := message.ObjectPriorGaps(o.Properties) + if gaps.HasObject && gaps.Object > 0 { + out = append(out, registry.LocRange{ + Lo: message.Location{Group: o.GroupID, Object: o.ObjectID - gaps.Object}, + Hi: message.Location{Group: o.GroupID, Object: o.ObjectID - 1}, + }) + } + if gaps.HasGroup && gaps.Group > 0 { + out = append(out, registry.LocRange{ + Lo: message.Location{Group: o.GroupID - gaps.Group}, + Hi: message.Location{Group: o.GroupID - 1, Object: math.MaxUint64}, + }) + } + } + return out +} + +// unknownIn returns the Locations of [start, end] whose status the relay does +// not know: neither among objs, the cached Objects of the range, nor known +// absent from them or entry's ledger. +func unknownIn( + entry *registry.TrackEntry, + objs []*cache.CachedObject, + start, end message.Location, +) []registry.LocRange { + return uncovered(start, end, append(knownFromCache(objs), entry.KnownAbsent()...)) +} + +// streamCompare orders Locations as a FETCH response in order carries them +// (§10.13): Groups in the Group Order, Object IDs ascending within a Group. +func streamCompare(a, b message.Location, order message.GroupOrder) int { + if order == message.GroupOrderDescending && a.Group != b.Group { + return cmp.Compare(b.Group, a.Group) + } + return a.Compare(b) +} + +// runEnds returns where each part of r a response in order carries without +// interruption ends, in stream order. Ascending, r is one run. Descending, a +// range across Groups is up to three: the start of its highest Group, the +// Groups strictly between, and the rest of its lowest Group. +func runEnds(r registry.LocRange, order message.GroupOrder) []message.Location { + if order != message.GroupOrderDescending || r.Lo.Group == r.Hi.Group { + return []message.Location{r.Hi} + } + ends := []message.Location{r.Hi} + if r.Hi.Group-r.Lo.Group > 1 { + ends = append(ends, message.Location{Group: r.Lo.Group + 1, Object: math.MaxUint64}) + } + return append(ends, message.Location{Group: r.Lo.Group, Object: math.MaxUint64}) +} + +// fetchElements orders objs for a response in order and adds, at the end of +// each run of unknown and timedOut Locations, an End of Unknown or Timed-Out +// Range marker (§11.4.4.2), so that a plain gap in the response only ever +// covers Locations known not to exist. unknown and timedOut must be disjoint +// and hold no Object of objs. A marker's run starts after the previous +// element, so it may also cover known-absent Locations: weaker, never false. +func fetchElements( + objs []*cache.CachedObject, + unknown, timedOut []registry.LocRange, + order message.GroupOrder, +) []*cache.CachedObject { + elems := slices.Clone(objs) + for _, r := range unknown { + for _, at := range runEnds(r, order) { + elems = append(elems, unknownRangeMarker(at)) + } + } + for _, r := range timedOut { + for _, at := range runEnds(r, order) { + elems = append(elems, timedOutRangeMarker(at)) + } + } + slices.SortStableFunc(elems, func(a, b *cache.CachedObject) int { + return streamCompare( + message.Location{Group: a.GroupID, Object: a.ObjectID}, + message.Location{Group: b.GroupID, Object: b.ObjectID}, + order) + }) + // A marker's coverage starts after the previous element, so a marker + // right after one of its own kind covers both: keep only the later. + out := elems[:0] + for i, e := range elems { + if i+1 < len(elems) && e.IsRangeMarker() && elems[i+1].IsRangeMarker() && + e.EndOfTimedOutRange == elems[i+1].EndOfTimedOutRange { + continue + } + out = append(out, e) + } + return out +} diff --git a/pkg/relay/handler_fetch.go b/pkg/relay/handler_fetch.go index e3aaafa3..09a21e3f 100644 --- a/pkg/relay/handler_fetch.go +++ b/pkg/relay/handler_fetch.go @@ -7,7 +7,6 @@ import ( "io" "log/slog" "math" - "slices" "time" "github.com/floatdrop/moq-go/pkg/moqt" @@ -21,19 +20,10 @@ import ( // defaultUpstreamFetchTimeout bounds an upstream stitch FETCH when the // downstream supplied no FILL_TIMEOUT. It keeps a fetch-capable upstream that // nonetheless stalls (or never answers FETCH) from wedging the downstream -// handler: the stitch degrades to cache-only once it elapses. +// handler: once it elapses, the cache is served with the unknown Locations +// marked Timed-Out. const defaultUpstreamFetchTimeout = 5 * time.Second -// handleFetch implements FETCH (§9.4, §10.13): validate the requested range, -// reply FETCH_OK, open a FETCH_HEADER uni-stream, and serialise the cached -// objects in the requested group order. Gaps in the response stream are how -// the spec signals "objects do not exist" (§11.4.4). -// -// The below-floor portion of the range — objects the relay evicted or never -// cached — is stitched from an upstream FETCH when one is reachable; see -// [sessionHandler.stitchedFetchObjects]. Whatever no source could vouch for -// is covered by §11.4.4.2 End of Unknown Range markers, so a gap always means -// authoritative non-existence. // trackKnown reports whether entry stands for a track the relay actually knows // of. Bare existence does not say so: subscribeUpstreamOnSession creates the // entry before the upstream round trip that would confirm the track, because @@ -59,6 +49,12 @@ func trackKnown(entry *registry.TrackEntry) bool { return len(entry.CopyUpstream()) > 0 || len(entry.CopyDownstream()) > 0 } +// handleFetch implements FETCH (§10.13): validate the requested range, reply +// FETCH_OK, open a FETCH_HEADER uni-stream, and serialise the cached objects +// in the requested group order. Gaps in the response stream are how the spec +// signals "objects do not exist" (§10.13), so what the cache cannot vouch for +// is asked of an upstream FETCH when one is reachable, or covered by §11.4.4.2 +// End of Range markers; see [sessionHandler.stitchedFetchObjects]. func (h *sessionHandler) handleFetch(ctx context.Context, req *session.Request, msg *message.Fetch) { if err := h.auth.AuthorizeFetch(ctx, h.sess, msg); err != nil { h.rejectAuth(ctx, req, "Fetch", err) @@ -259,22 +255,23 @@ func capFetchEndLocation(filter *message.LocationFilter, largest message.Locatio return end } -// stitchedFetchObjects answers a FETCH range from the relay's cache, filling -// the below-floor portion the relay does not hold from an upstream FETCH when -// one is reachable (§9.4 upstream stitching). +// stitchedFetchObjects answers a FETCH range [start, end] from the relay's +// cache, asking an upstream about the Locations whose status it does not know +// (§10.13: "If it encounters an object in the requested range that is not +// cached and has unknown status, the relay MUST pause subsequent delivery +// until it has confirmed the object's status upstream"). See fetch_ranges.go +// for what the relay knows. // -// Everything below the cache's eviction floor (see -// [cache.ObjectCache.OldestRetained]) was evicted or never cached, so a gap -// there might still exist upstream whereas a gap at/above the floor is -// ground-truth non-existence. The handler splits the request at the floor, -// fetches [requestStart, floor) from an established upstream, and concatenates -// it with the cached part — the two are disjoint by Location, so the result is -// correctly ordered. With no FETCH-able upstream (or on error/timeout) it -// serves what the cache has and covers the below-floor remainder with a -// §11.4.4.2 End of Unknown Range marker, since a plain gap would falsely -// assert non-existence (§11.4.4). Upstream-fetched objects are NOT cached -// back: the FIFO ring is keyed by arrival, so old backfill would evict live -// objects. +// With a fetch-capable upstream, one FETCH covers the span from the first +// unknown Location to the last, within the FILL_TIMEOUT budget (§10.2.5). Its +// Objects fill the holes, and what it marks unknown or timed out stays so where +// the relay does not know better; the cached Objects are served either way, +// and what the relay knows does not exist stays a gap. With no such upstream, or +// when its FETCH fails or times out, the unknown Locations are marked End of +// Unknown or Timed-Out Range (§11.4.4.2) and the cached Objects served: the +// relay can "indicate the range of unknown Objects and continue serving other +// known Objects" (§10.13). Upstream-fetched objects are NOT cached back: the +// FIFO ring is keyed by arrival, so old backfill would evict live objects. // // A non-nil refusal (see fetchUpstreamRange) means the track must not be // forwarded; no objects are returned. @@ -282,68 +279,71 @@ func (h *sessionHandler) stitchedFetchObjects( ctx context.Context, entry *registry.TrackEntry, fullName track.FullTrackName, - requestStart message.Location, - requestEndIncl message.Location, + start, end message.Location, order message.GroupOrder, fillTimeout time.Duration, ) (objs []*cache.CachedObject, refusal error) { - cacheObjs := entry.Cache.GetRange(requestStart, requestEndIncl, order) - - // Determine the inclusive upper bound of the below-floor sub-range the - // relay cannot answer from cache. - upEndIncl := requestEndIncl - if floor, hasFloor := entry.Cache.OldestRetained(); hasFloor { - pred, ok := fetchPredecessor(floor) - if !ok { - return cacheObjs, nil // floor == {0,0}: nothing exists below it - } - if pred.Less(upEndIncl) { - upEndIncl = pred - } + // An expired Object (§12.3) is not returned: its status is unknown. + cached := entry.Cache.GetRange(start, end, message.GroupOrderAscending) + unknown := unknownIn(entry, cached, start, end) + if len(unknown) == 0 { + return fetchElements(cached, nil, nil, order), nil } - if upEndIncl.Less(requestStart) { - return cacheObjs, nil // the request starts at/above the floor — no gap - } - - // GetRange and OldestRetained are two separate cache reads: an eviction - // or TTL expiry between them can raise the floor above snapshot entries, - // making the upstream sub-range [requestStart, upEndIncl] overlap the - // snapshot. mergeFetchObjects relies on the two sources being disjoint - // by Location (a duplicate would serialize a non-ascending Object ID), - // so clip the snapshot to strictly above the sub-range. - cacheObjs = slices.DeleteFunc(cacheObjs, func(o *cache.CachedObject) bool { - return !upEndIncl.Less(message.Location{Group: o.GroupID, Object: o.ObjectID}) - }) - up := h.pickFetchUpstream(entry) if up == nil { - // No reachable upstream: the below-floor sub-range has unknown - // status, not ground-truth non-existence. A plain gap in a - // FIN-terminated response asserts the latter (§11.4.4), so cover - // the sub-range with an End of Unknown Range marker instead. This - // is the unknown-status case, not the §10.2.5 budget case — nothing - // timed out, we simply have no source to ask. - return mergeFetchObjects(order, - unknownWholeRange(requestStart, upEndIncl, order), cacheObjs), nil - } - - upstreamObjs, refusal := h.fetchUpstreamRange( - ctx, up, fullName, requestStart, upEndIncl, order, fillTimeout, - ) + return fetchElements(cached, unknown, nil, order), nil + } + span := registry.LocRange{Lo: unknown[0].Lo, Hi: unknown[len(unknown)-1].Hi} + ans, refusal := h.fetchUpstreamRange(ctx, up, fullName, span, order, fillTimeout) if errors.Is(refusal, session.ErrMalformedTrack) { h.endMalformedTrack(ctx, entry, up.Session, refusal) } if refusal != nil { return nil, refusal } - if len(upstreamObjs) == 0 { - // A clean-FIN, uncapped, empty upstream response: the upstream - // authoritatively asserted the whole sub-range non-existent, which - // a plain gap encodes exactly. (Every unknown outcome returns at - // least a marker element.) - return cacheObjs, nil + switch ans.failed { + case upstreamUnknown: + return fetchElements(cached, unknown, nil, order), nil + case upstreamTimedOut: + return fetchElements(cached, nil, unknown, order), nil + case upstreamAnswered: + } + + // The upstream answered for the span: its Objects fill the holes, and + // under its FIN the rest does not exist, except what it marked unknown or + // timed out and the relay has no signal for either. + have := make(map[message.Location]bool, len(cached)) + for _, o := range cached { + have[message.Location{Group: o.GroupID, Object: o.ObjectID}] = true + } + merged := cached + for _, o := range ans.objs { + if !have[message.Location{Group: o.GroupID, Object: o.ObjectID}] { + merged = append(merged, o) + } } - return mergeFetchObjects(order, upstreamObjs, cacheObjs), nil + return fetchElements(merged, intersect(ans.unknown, unknown), intersect(ans.timedOut, unknown), order), nil +} + +// intersect returns the Locations both a and b hold, each a set of disjoint +// ranges. +func intersect(a, b []registry.LocRange) []registry.LocRange { + var out []registry.LocRange + for _, x := range a { + for _, y := range b { + lo, hi := x.Lo, x.Hi + if lo.Less(y.Lo) { + lo = y.Lo + } + if y.Hi.Less(hi) { + hi = y.Hi + } + if !hi.Less(lo) { + out = append(out, registry.LocRange{Lo: lo, Hi: hi}) + } + } + } + return out } // pickFetchUpstream returns an Established, fetch-capable upstream on a @@ -364,29 +364,32 @@ func (h *sessionHandler) pickFetchUpstream(entry *registry.TrackEntry) *registry return nil } -// fetchUpstreamRange issues a standalone FETCH for the inclusive range -// [start, endIncl] on the upstream's session, awaits the response stream via -// the relay's fetch router, and returns the decoded objects in the requested -// group order (the upstream FETCH carries the same GROUP_ORDER parameter). -// -// The returned slice preserves what the upstream did and did not vouch for, -// so the downstream response stays truthful under §11.4.4's gap rule (a gap -// in a FIN-terminated response asserts non-existence): -// -// - Upstream End of Unknown Range markers (§11.4.4.2, 0x10C) are kept as -// [cache.CachedObject] marker elements and re-emitted downstream. -// - End of Non-Existent Range markers (0x8C) are dropped: a plain gap in -// our FIN-terminated response is the semantically equivalent encoding -// (§9.1 lets relays re-represent missing ranges), and §11.4.4.2 prefers -// it outside known/unknown splits. -// - When the upstream vouches for less than the whole sub-range — FETCH -// rejected, response timeout, a mid-stream error (no FIN, so its gaps -// assert nothing), or a clean FIN whose FETCH_OK EndLocation was capped -// below endIncl — the unvouched-for remainder is covered by an unknown -// marker. The mid-stream-error and descending capped cases collapse to -// "whole sub-range unknown": exact per-gap markers are inexpressible in -// §11.4.4's delta encoding wherever the element after a marker would be -// a same-group, lower-Object-ID transition. +// upstreamFailure is how an upstream FETCH failed to answer at all. +type upstreamFailure uint8 + +const ( + upstreamAnswered upstreamFailure = iota + // upstreamUnknown: refused, reset, malformed or out of order; nothing it + // sent is vouched for. + upstreamUnknown + // upstreamTimedOut: the FILL_TIMEOUT budget ran out (§10.2.5). + upstreamTimedOut +) + +// upstreamAnswer is an upstream FETCH response for a span, as Location ranges: +// its Objects; the parts its End of Unknown and Timed-Out Range markers +// covered, and any past a capped FETCH_OK End Location; every other Location +// of the span, a gap under a clean FIN, is known not to exist (§10.13). +type upstreamAnswer struct { + objs []*cache.CachedObject + unknown, timedOut []registry.LocRange + failed upstreamFailure +} + +// fetchUpstreamRange issues a standalone FETCH for span on the upstream's +// session, awaits the response stream via the relay's fetch router, and reads +// it into an upstreamAnswer. End of Non-Existent Range markers need no +// record: under a clean FIN a gap already says so. // // It returns a refusal instead when the track MUST NOT be forwarded: a // FETCH_OK with unacceptable Track Properties (§2.5.1), or a response Object @@ -395,17 +398,14 @@ func (h *sessionHandler) fetchUpstreamRange( ctx context.Context, up *registry.UpstreamSub, fullName track.FullTrackName, - start, endIncl message.Location, + span registry.LocRange, order message.GroupOrder, fillTimeout time.Duration, -) (objs []*cache.CachedObject, refusal error) { - unknownWhole := unknownWholeRange(start, endIncl, order) - timedOutWhole := timedOutWholeRange(start, endIncl, order) - +) (ans upstreamAnswer, refusal error) { // §10.2.5: an explicit 0 means "MUST NOT wait for upstream delivery" // (fillTimeout is already resolved, see [resolveFillBudget]). if fillTimeout == 0 { - return timedOutWhole, nil + return upstreamAnswer{failed: upstreamTimedOut}, nil } params := message.Parameters{} @@ -415,7 +415,7 @@ func (h *sessionHandler) fetchUpstreamRange( // §5.1.2: the range rides in LOCATION_FILTER. EndGroupDelta is delta-encoded // from the start group, and EndObject makes the end Object-precise. params = append(params, message.AbsoluteRangeObjectFilter( - start, endIncl.Group-start.Group, endIncl.Object)) + span.Lo, span.Hi.Group-span.Lo.Group, span.Hi.Object)) fmsg := &message.Fetch{ Namespace: fullName.Namespace, Name: fullName.Name, @@ -423,7 +423,7 @@ func (h *sessionHandler) fetchUpstreamRange( } // Bound the upstream round-trip so a silent or non-FETCH-answering - // upstream degrades to cache-plus-unknown-gap instead of wedging the + // upstream degrades to cache-plus-marked-unknown instead of wedging the // downstream handler. FILL_TIMEOUT, when present, is the subscriber's // explicit budget; otherwise fall back to a default. fctx, cancel := context.WithTimeout(ctx, fillTimeout) @@ -436,12 +436,12 @@ func (h *sessionHandler) fetchUpstreamRange( // §2.5.1: Session.Fetch has cancelled it; the caller resets the // downstream stream. if isTrackPropertiesErr(err) { - return nil, err + return upstreamAnswer{}, err } if fctx.Err() != nil { - return timedOutWhole, nil + return upstreamAnswer{failed: upstreamTimedOut}, nil } - return unknownWhole, nil + return upstreamAnswer{failed: upstreamUnknown}, nil } defer fr.Close() @@ -457,129 +457,147 @@ func (h *sessionHandler) fetchUpstreamRange( case fs = <-ch: case <-fctx.Done(): h.log.LogAttrs(ctx, slog.LevelDebug, "upstream FETCH response timed out") - return timedOutWhole, nil + return upstreamAnswer{failed: upstreamTimedOut}, nil } if fs == nil { - return unknownWhole, nil + return upstreamAnswer{failed: upstreamUnknown}, nil } // ReadDecoded needs the response's group order to resolve cross-group // deltas (§11.4.4.1); the upstream serves in the order our FETCH asked // for. fs.GroupOrder = order - var ( - out []*cache.CachedObject - prevLoc message.Location - havePrev bool - ) + var prev *message.Location for { obj, err := fs.ReadDecoded() if errors.Is(err, io.EOF) { - break // clean FIN: the upstream's gaps are authoritative (§11.4.4) + break // clean FIN: the upstream's gaps are authoritative (§10.13) } if errors.Is(err, session.ErrMalformedTrack) { // §2.4.2: fr.Close (deferred) cancels the fetch; the caller // resets the downstream stream. fs.Cancel(moqt.StreamResetMalformedTrack) - return nil, err + return upstreamAnswer{}, err } if err != nil { // No FIN (or a FIN mid-object), so the gaps in what arrived - // assert nothing; declare the whole sub-range unknown rather - // than serve partial objects whose gaps would read as - // non-existence. + // assert nothing. h.log.LogAttrs(ctx, slog.LevelDebug, "upstream FETCH stream failed mid-read", slog.String("err", err.Error())) - return unknownWhole, nil - } - if obj.EndOfNonExistentRange { - // Dropped: a plain gap in our FIN-terminated response is the - // semantically equivalent encoding (§9.1). - continue + return upstreamAnswer{failed: upstreamUnknown}, nil } loc := message.Location{Group: obj.GroupID, Object: obj.ObjectID} - if !upstreamFetchElemOK(loc, prevLoc, havePrev, start, endIncl, order, - obj.EndOfUnknownRange || obj.EndOfTimedOutRange) { + // §10.14: nothing past its own End Location. + if !upstreamFetchElemOK(loc, prev, span, order) || fr.OK.EndLocation.Less(loc) { h.log.LogAttrs(ctx, slog.LevelDebug, "upstream FETCH element out of range or order", slog.Uint64("group", loc.Group), slog.Uint64("object", loc.Object)) - return unknownWhole, nil + return upstreamAnswer{failed: upstreamUnknown}, nil } - prevLoc, havePrev = loc, true - if obj.EndOfUnknownRange { - out = append(out, unknownRangeMarker(loc)) - continue - } - if obj.EndOfTimedOutRange { - out = append(out, timedOutRangeMarker(loc)) - continue - } - // The §11.4.4.1 Datagram bit carries the original wire shape - // across this relay hop, so the object is re-emitted downstream - // with the same forwarding preference it was published with. - // (Stitched objects are merged into the response only — they are - // not written back into the cache.) - pref := cache.ForwardingSubgroup - if obj.Datagram { - pref = cache.ForwardingDatagram + switch { + case obj.EndOfUnknownRange: + ans.unknown = append(ans.unknown, streamCovered(prev, loc, span, order)...) + case obj.EndOfTimedOutRange: + ans.timedOut = append(ans.timedOut, streamCovered(prev, loc, span, order)...) + case obj.EndOfNonExistentRange: + default: + // The §11.4.4.1 Datagram bit carries the original wire shape + // across this relay hop. + pref := cache.ForwardingSubgroup + if obj.Datagram { + pref = cache.ForwardingDatagram + } + ans.objs = append(ans.objs, &cache.CachedObject{ + GroupID: obj.GroupID, + ObjectID: obj.ObjectID, + SubgroupID: obj.SubgroupID, + PublisherPriority: obj.PublisherPriority, + ForwardingPref: pref, + Properties: obj.Properties, + Payload: obj.Payload, + }) } - out = append(out, &cache.CachedObject{ - GroupID: obj.GroupID, - ObjectID: obj.ObjectID, - SubgroupID: obj.SubgroupID, - PublisherPriority: obj.PublisherPriority, - ForwardingPref: pref, - Properties: obj.Properties, - Payload: obj.Payload, - }) + prev = &loc } - // A clean FIN asserts gaps only up to the FETCH_OK EndLocation (§11.4.4). - // If the upstream capped it below our sub-range end (§10.13: End beyond - // its Largest), the remainder has unknown status. - if authEnd := fr.OK.EndLocation; authEnd.Less(endIncl) { - if order == message.GroupOrderDescending { - // The unknown remainder precedes every object in descending - // stream order, and a leading marker cannot in general be - // followed by a same-group object with a lower ID (see the - // doc comment) — fall back to whole-sub-range unknown. - return unknownWhole, nil - } - out = append(out, unknownRangeMarker(endIncl)) + // A clean FIN asserts gaps only up to the FETCH_OK End Location (§10.13). + // If the upstream capped it below the span (§10.13: End beyond its + // Largest), what lies past it has unknown status. + // No element lay past it, and Session.Fetch refused one before the span. + if authEnd := fr.OK.EndLocation; authEnd.Less(span.Hi) { + next, _ := locSucc(authEnd) // below span.Hi, so it has one + ans.unknown = append(ans.unknown, registry.LocRange{Lo: next, Hi: span.Hi}) } - return out, nil + return ans, nil } -// upstreamFetchElemOK validates one kept element of an upstream FETCH -// response before it is re-serialized downstream. Every element must lie -// inside the requested sub-range [start, endIncl] — the merge with the -// cached part relies on Location disjointness — and an object must advance -// from the previous kept element the way §11.4.4's delta encoding can -// express: within a group, Object IDs strictly ascend; across groups, the -// Group ID moves in the response's order direction. Unknown-range markers -// carry absolute IDs and merely re-anchor the encoding, so only the range -// check applies to them. A violation means the upstream is nonconformant; -// trusting the element would corrupt the downstream delta stream (e.g. flip -// its group-direction inference), so the caller discards the response. +// upstreamFetchElemOK validates one element of an upstream FETCH response for +// span before it is re-served downstream: it lies inside span, and after the +// previous element prev (nil for the first) in the order the response carries +// them (see [streamCompare]), as §11.4.4's delta encoding requires. A +// violation means the upstream is nonconformant; trusting the element would +// corrupt the downstream stream, so the caller discards the response. func upstreamFetchElemOK( - loc, prev message.Location, - havePrev bool, - start, endIncl message.Location, + loc message.Location, + prev *message.Location, + span registry.LocRange, order message.GroupOrder, - isMarker bool, ) bool { - if loc.Less(start) || endIncl.Less(loc) { + if loc.Less(span.Lo) || span.Hi.Less(loc) { return false } - if isMarker || !havePrev { - return true + return prev == nil || streamCompare(*prev, loc, order) < 0 +} + +// streamCovered returns, as Location ranges, what an End of Range marker at at +// covers in a response to a FETCH of span in order: the Locations after the +// previous element prev (nil for the first) up to at, in the order the +// response carries them (see fetch_ranges.go). at is after prev. +func streamCovered( + prev *message.Location, + at message.Location, + span registry.LocRange, + order message.GroupOrder, +) []registry.LocRange { + if order != message.GroupOrderDescending { + from := span.Lo + if prev != nil { + from, _ = locSucc(*prev) // at is after prev, so it has one + } + return []registry.LocRange{{Lo: from, Hi: at}} } - if loc.Group == prev.Group { - return prev.Object < loc.Object + // Descending: Group g of span carries Objects lo(g) through hi(g). + lo := func(g uint64) uint64 { + if g == span.Lo.Group { + return span.Lo.Object + } + return 0 } - if order == message.GroupOrderDescending { - return loc.Group < prev.Group + hi := func(g uint64) uint64 { + if g == span.Hi.Group { + return span.Hi.Object + } + return math.MaxUint64 } - return prev.Group < loc.Group + var from message.Location + switch { + case prev == nil: + from = message.Location{Group: span.Hi.Group, Object: lo(span.Hi.Group)} + case prev.Object < hi(prev.Group): + from = message.Location{Group: prev.Group, Object: prev.Object + 1} + default: + from = message.Location{Group: prev.Group - 1, Object: lo(prev.Group - 1)} + } + if from.Group == at.Group { + return []registry.LocRange{{Lo: from, Hi: at}} + } + out := []registry.LocRange{{Lo: message.Location{Group: at.Group, Object: lo(at.Group)}, Hi: at}} + if from.Group-at.Group > 1 { + out = append(out, registry.LocRange{ + Lo: message.Location{Group: at.Group + 1}, + Hi: message.Location{Group: from.Group - 1, Object: math.MaxUint64}, + }) + } + return append(out, registry.LocRange{Lo: from, Hi: message.Location{Group: from.Group, Object: hi(from.Group)}}) } // unknownRangeMarker returns the serve-path element that streamFetchObjects @@ -603,99 +621,6 @@ func timedOutRangeMarker(loc message.Location) *cache.CachedObject { } } -// unknownWholeRange declares the whole inclusive sub-range [start, endIncl] -// unknown with a single marker, positioned for the response's stream order. -// The marker Location is the range's far end in stream direction (endIncl -// when ascending, start when descending), so §11.4.4.2's "between the last -// serialized Object, if any, and this Location, inclusive" coverage spans -// the sub-range. -func unknownWholeRange(start, endIncl message.Location, order message.GroupOrder) []*cache.CachedObject { - return wholeRange(unknownRangeMarker, start, endIncl, order) -} - -// timedOutWholeRange is [unknownWholeRange] with the §11.4.4.2 End of -// Timed-Out Range marker, for when the FILL_TIMEOUT budget is what stopped us -// (§10.2.5) rather than an unreachable or unhelpful upstream. -func timedOutWholeRange(start, endIncl message.Location, order message.GroupOrder) []*cache.CachedObject { - return wholeRange(timedOutRangeMarker, start, endIncl, order) -} - -// wholeRange covers [start, endIncl] with a single marker built by mark. The -// marker names the far end of the range in delivery order, since §11.4.4.2 -// markers cover everything from the previous element up to their own Location. -func wholeRange( - mark func(message.Location) *cache.CachedObject, - start, endIncl message.Location, - order message.GroupOrder, -) []*cache.CachedObject { - if order == message.GroupOrderDescending { - return []*cache.CachedObject{mark(start)} - } - return []*cache.CachedObject{mark(endIncl)} -} - -// mergeFetchObjects merges the below-floor (upstream) and at/above-floor -// (cache) slices in group order. The two are disjoint by Location and each is -// already sorted in order, so for ascending the lower range leads and for -// descending the higher (cache) range leads. -// -// Descending needs one more step: within a group, Object IDs always ascend -// (§11.4.3), and §11.4.4's delta encoding cannot express a same-group -// transition to a lower Object ID — so when the eviction floor splits a -// group across the two sources, the seam group's runs must be spliced into -// one contiguous ascending run, upstream part (lower Object IDs) first. -// Plain concatenation would put the cache's high-object run before the -// upstream's low-object run of the same group and serialize a wrapped -// delta. Unknown-range markers interleaved with the seam run's objects move -// with them (their coverage and delta re-anchoring stay as the upstream -// meant them); a marker-only prefix — the whole-sub-range unknown marker, -// whose coverage spans everything below the cache — stays after it. -func mergeFetchObjects(order message.GroupOrder, lower, upper []*cache.CachedObject) []*cache.CachedObject { - switch { - case len(lower) == 0: - return upper - case len(upper) == 0: - return lower - } - out := make([]*cache.CachedObject, 0, len(lower)+len(upper)) - if order != message.GroupOrderDescending { - out = append(out, lower...) - out = append(out, upper...) - return out - } - - // The only group the two sources can share is the cache's lowest - // (upper's last element) — the floor group. splice is the length of - // lower's leading seam-group run, markers included: an interleaved - // upstream 0x10C marker belongs with its neighbouring objects (its - // coverage and the delta re-anchoring stay exactly as the upstream - // meant them, and every spliced Location is below the cache's seam - // objects). A prefix with no objects at all is NOT spliced — that is - // the whole-sub-range unknown marker, whose coverage spans everything - // below the cache and must stay after it. - seamG := upper[len(upper)-1].GroupID - splice, seamHasObject := 0, false - for splice < len(lower) && lower[splice].GroupID == seamG { - seamHasObject = seamHasObject || !lower[splice].IsRangeMarker() - splice++ - } - if !seamHasObject { - splice = 0 - } - // cut is where upper's trailing seam-group run starts. A plain group - // comparison suffices: upper's only markers are GetRange's expired-Object - // markers, each at its own Location. - cut := len(upper) - for cut > 0 && upper[cut-1].GroupID == seamG { - cut-- - } - out = append(out, upper[:cut]...) - out = append(out, lower[:splice]...) - out = append(out, upper[cut:]...) - out = append(out, lower[splice:]...) - return out -} - // fetchPredecessor returns the Location immediately below loc in (group, // object) order, and false when loc is {0, 0} (nothing precedes it). The // object-underflow case rolls back to the end of the previous group. @@ -829,8 +754,8 @@ func streamFetchObjects( default: // Same group. §11.4.4 cannot express a non-ascending Object ID - // here — the delta only ever adds. The inputs are sorted and - // seam-spliced (mergeFetchObjects), so hitting this is an + // here — the delta only ever adds. The inputs are sorted in + // stream order (fetchElements), so hitting this is an // internal invariant violation; fail rather than emit a wrapped // delta the subscriber must treat as a session-fatal overflow. if o.ObjectID <= prevObject { diff --git a/pkg/relay/handler_fetch_elem_test.go b/pkg/relay/handler_fetch_elem_test.go index 8c4b8f0b..c419a426 100644 --- a/pkg/relay/handler_fetch_elem_test.go +++ b/pkg/relay/handler_fetch_elem_test.go @@ -4,105 +4,63 @@ import ( "testing" "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/relay/internal/registry" ) -// TestUpstreamFetchElemOK: an upstream FETCH element may be re-serialized only -// if it lies in [start, endIncl], Object IDs ascend within a group, and Group -// IDs move in the response's order; markers get the range check only -// (§11.4.4). A wrong one would re-encode to well-formed but wrong IDs. +// TestUpstreamFetchElemOK: an upstream FETCH element, Object or marker, may be +// re-served only if it lies in the span and comes after the previous element +// in the order the response carries them: Object IDs ascending within a +// Group, Groups in the response's order (§11.4.4). A wrong one would re-encode +// to well-formed but wrong IDs. func TestUpstreamFetchElemOK(t *testing.T) { t.Parallel() loc := func(g, o uint64) message.Location { return message.Location{Group: g, Object: o} } - // The requested sub-range for every case below. - start, endIncl := loc(10, 0), loc(20, 5) + span := registry.LocRange{Lo: loc(10, 0), Hi: loc(20, 5)} + at := func(g, o uint64) *message.Location { l := loc(g, o); return &l } for _, tc := range []struct { - name string - loc message.Location - prev message.Location - havePrev bool - order message.GroupOrder - isMarker bool - want bool + name string + loc message.Location + prev *message.Location + order message.GroupOrder + want bool }{ - // Range bounds. Inclusive at both ends: endIncl is the last - // serviceable Location, not one past it. - {name: "first element at start", loc: loc(10, 0), want: true}, - {name: "first element at endIncl", loc: loc(20, 5), want: true}, - {name: "below start by one object", loc: loc(9, 9), want: false}, - {name: "above endIncl by one object", loc: loc(20, 6), want: false}, - {name: "above endIncl by one group", loc: loc(21, 0), want: false}, + // Span bounds, inclusive at both ends. + {name: "first element at the start", loc: loc(10, 0), want: true}, + {name: "first element at the end", loc: loc(20, 5), want: true}, + {name: "below the start by one object", loc: loc(9, 9), want: false}, + {name: "above the end by one object", loc: loc(20, 6), want: false}, + {name: "above the end by one group", loc: loc(21, 0), want: false}, + {name: "no prev, mid-span", loc: loc(15, 3), want: true}, - // No predecessor: only the range check can apply. - {name: "no prev, mid-range", loc: loc(15, 3), want: true}, - - // Within one group, Object IDs must strictly ascend — in BOTH - // order directions. GROUP_ORDER sequences groups, not the objects - // inside them, so descending must not loosen this. - { - name: "same group ascending object", loc: loc(15, 4), - prev: loc(15, 3), havePrev: true, want: true, - }, - { - name: "same group repeated object", loc: loc(15, 3), - prev: loc(15, 3), havePrev: true, want: false, - }, + // Within a Group, Object IDs strictly ascend in both orders. + {name: "same group ascending object", loc: loc(15, 4), prev: at(15, 3), want: true}, + {name: "same group repeated object", loc: loc(15, 3), prev: at(15, 3), want: false}, + {name: "same group descending object", loc: loc(15, 2), prev: at(15, 3), want: false}, { - name: "same group descending object", loc: loc(15, 2), - prev: loc(15, 3), havePrev: true, want: false, - }, - { - name: "same group descending object, descending order", loc: loc(15, 2), - prev: loc(15, 3), havePrev: true, + name: "same group descending object, descending order", loc: loc(15, 2), prev: at(15, 3), order: message.GroupOrderDescending, want: false, }, - // Across groups the direction must match the response order. - { - name: "ascending order, group advances", loc: loc(16, 0), - prev: loc(15, 3), havePrev: true, want: true, - }, + // Across Groups the direction must match the response order. + {name: "ascending order, group advances", loc: loc(16, 0), prev: at(15, 3), want: true}, + {name: "ascending order, group goes backwards", loc: loc(14, 0), prev: at(15, 3), want: false}, { - name: "ascending order, group goes backwards", loc: loc(14, 0), - prev: loc(15, 3), havePrev: true, want: false, - }, - { - name: "descending order, group goes backwards", loc: loc(14, 0), - prev: loc(15, 3), havePrev: true, + name: "descending order, group goes backwards", loc: loc(14, 0), prev: at(15, 3), order: message.GroupOrderDescending, want: true, }, { - name: "descending order, group advances", loc: loc(16, 0), - prev: loc(15, 3), havePrev: true, + name: "descending order, group advances", loc: loc(16, 0), prev: at(15, 3), order: message.GroupOrderDescending, want: false, }, - - // Markers re-anchor the encoding with absolute IDs, so an ordering - // violation is not one for them — but they are still confined to - // the requested range. - { - name: "marker may break group direction", loc: loc(14, 0), - prev: loc(15, 3), havePrev: true, isMarker: true, want: true, - }, - { - name: "marker may repeat a location", loc: loc(15, 3), - prev: loc(15, 3), havePrev: true, isMarker: true, want: true, - }, - { - name: "marker outside the range is still rejected", loc: loc(21, 0), - prev: loc(15, 3), havePrev: true, isMarker: true, want: false, - }, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got := upstreamFetchElemOK( - tc.loc, tc.prev, tc.havePrev, start, endIncl, tc.order, tc.isMarker) - if got != tc.want { - t.Errorf("upstreamFetchElemOK(loc=%v prev=%v havePrev=%v order=%v marker=%v) = %v, want %v", - tc.loc, tc.prev, tc.havePrev, tc.order, tc.isMarker, got, tc.want) + if got := upstreamFetchElemOK(tc.loc, tc.prev, span, tc.order); got != tc.want { + t.Errorf("upstreamFetchElemOK(%v, prev %v, %v) = %v, want %v", tc.loc, tc.prev, tc.order, got, tc.want) } }) } diff --git a/pkg/relay/handler_fetch_merge_test.go b/pkg/relay/handler_fetch_merge_test.go deleted file mode 100644 index e8197b0f..00000000 --- a/pkg/relay/handler_fetch_merge_test.go +++ /dev/null @@ -1,189 +0,0 @@ -package relay - -import ( - "errors" - "io" - "slices" - "testing" - - "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/session/sessiontest" - "github.com/floatdrop/moq-go/pkg/relay/cache" -) - -// obj is a cached Object at {g, o}. -func obj(g, o uint64) *cache.CachedObject { - return &cache.CachedObject{GroupID: g, ObjectID: o, Payload: []byte{byte(o)}} -} - -// marker is a cached End of Unknown Range marker at {g, o}. -func marker(g, o uint64) *cache.CachedObject { - return &cache.CachedObject{GroupID: g, ObjectID: o, EndOfUnknownRange: true} -} - -// loc is a {Group, Object} Location. -type loc struct{ G, O uint64 } - -// locsOf returns the Locations of objs. -func locsOf(objs []*cache.CachedObject) []loc { - out := make([]loc, 0, len(objs)) - for _, o := range objs { - out = append(out, loc{o.GroupID, o.ObjectID}) - } - return out -} - -// TestMergeFetchObjects_DescendingSeamSplice: when the eviction floor splits a -// group, the descending merge splices it into one ascending run, upstream's -// lower Object IDs first, which §11.4.4's delta encoding can express. -func TestMergeFetchObjects_DescendingSeamSplice(t *testing.T) { - t.Parallel() - desc := message.GroupOrderDescending - - cases := []struct { - name string - lower, upper []*cache.CachedObject - want []loc - }{ - { - name: "seam group split across sources", - upper: []*cache.CachedObject{obj(7, 0), obj(6, 2), obj(6, 3)}, - lower: []*cache.CachedObject{obj(6, 0), obj(6, 1), obj(5, 0)}, - want: []loc{{7, 0}, {6, 0}, {6, 1}, {6, 2}, {6, 3}, {5, 0}}, - }, - { - name: "trailing unknown marker stays after everything", - upper: []*cache.CachedObject{obj(7, 0), obj(6, 2), obj(6, 3)}, - lower: []*cache.CachedObject{obj(6, 0), obj(6, 1), marker(2, 0)}, - want: []loc{{7, 0}, {6, 0}, {6, 1}, {6, 2}, {6, 3}, {2, 0}}, - }, - { - name: "marker-only lower is appended, never spliced", - upper: []*cache.CachedObject{obj(6, 2), obj(6, 3)}, - lower: []*cache.CachedObject{marker(6, 1)}, - want: []loc{{6, 2}, {6, 3}, {6, 1}}, - }, - { - name: "group-aligned floor concatenates", - upper: []*cache.CachedObject{obj(7, 0), obj(6, 0)}, - lower: []*cache.CachedObject{obj(5, 3), obj(4, 0)}, - want: []loc{{7, 0}, {6, 0}, {5, 3}, {4, 0}}, - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - got := locsOf(mergeFetchObjects(desc, tc.lower, tc.upper)) - if !slices.Equal(got, tc.want) { - t.Errorf("merged %v, want %v", got, tc.want) - } - }) - } - - // Ascending stays a plain lower-then-upper concatenation. - asc := locsOf(mergeFetchObjects(message.GroupOrderAscending, - []*cache.CachedObject{obj(6, 0), obj(6, 1)}, - []*cache.CachedObject{obj(6, 2), obj(7, 0)})) - if !slices.Equal(asc, []loc{{6, 0}, {6, 1}, {6, 2}, {7, 0}}) { - t.Errorf("ascending merge = %v", asc) - } -} - -// TestStreamFetchObjects_DescendingSeamRoundTrip: a descending stitched -// response with a split group decodes back to the merged Locations. -func TestStreamFetchObjects_DescendingSeamRoundTrip(t *testing.T) { - t.Parallel() - cli, srv := sessiontest.NewSessionPair(t) - - merged := mergeFetchObjects(message.GroupOrderDescending, - []*cache.CachedObject{obj(6, 0), obj(6, 1), obj(5, 0)}, // upstream stitch - []*cache.CachedObject{obj(7, 0), obj(6, 2), obj(6, 3)}, // cache tail - ) - - writeErr := make(chan error, 1) - go func() { - out, err := cli.OpenFetchStream(message.FetchHeader{RequestID: 0}) - if err != nil { - writeErr <- err - return - } - if _, err := streamFetchObjects(out, merged, nil); err != nil { - // Reset so the reader fails fast instead of hanging on a - // never-FIN'd stream. - out.Cancel(moqt.StreamResetInternalError) - writeErr <- err - return - } - writeErr <- out.Close() - }() - - ds, err := srv.AcceptDataStream(t.Context()) - if err != nil { - t.Fatalf("AcceptDataStream: %v", err) - } - fs, ok := ds.(*session.IncomingFetchStream) - if !ok { - t.Fatalf("AcceptDataStream returned %T", ds) - } - fs.GroupOrder = message.GroupOrderDescending - - var got []loc - for { - o, err := fs.ReadDecoded() - if errors.Is(err, io.EOF) { - break - } - if err != nil { - t.Fatalf("ReadDecoded: %v", err) - } - got = append(got, loc{o.GroupID, o.ObjectID}) - } - if err := <-writeErr; err != nil { - t.Fatalf("writer: %v", err) - } - - want := []loc{{7, 0}, {6, 0}, {6, 1}, {6, 2}, {6, 3}, {5, 0}} - if !slices.Equal(got, want) { - t.Fatalf("decoded %v, want %v", got, want) - } -} - -// TestMergeFetchObjects_SeamMarkersSpliced: an upstream marker among the seam -// group's Objects moves with them; a marker-only prefix stays after the cache. -func TestMergeFetchObjects_SeamMarkersSpliced(t *testing.T) { - t.Parallel() - desc := message.GroupOrderDescending - upper := []*cache.CachedObject{obj(7, 0), obj(6, 3), obj(6, 4)} - - cases := []struct { - name string - lower []*cache.CachedObject - want []loc - }{ - { - name: "marker between seam objects", - lower: []*cache.CachedObject{obj(6, 0), marker(6, 1), obj(6, 2), obj(5, 0)}, - want: []loc{{7, 0}, {6, 0}, {6, 1}, {6, 2}, {6, 3}, {6, 4}, {5, 0}}, - }, - { - name: "marker trailing the seam run", - lower: []*cache.CachedObject{obj(6, 0), marker(6, 2), obj(5, 0)}, - want: []loc{{7, 0}, {6, 0}, {6, 2}, {6, 3}, {6, 4}, {5, 0}}, - }, - { - name: "marker-only seam prefix stays after the cache", - lower: []*cache.CachedObject{marker(6, 2)}, - want: []loc{{7, 0}, {6, 3}, {6, 4}, {6, 2}}, - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - got := locsOf(mergeFetchObjects(desc, tc.lower, upper)) - if !slices.Equal(got, tc.want) { - t.Errorf("merged %v, want %v", got, tc.want) - } - }) - } -} diff --git a/pkg/relay/handler_fetch_ranges_test.go b/pkg/relay/handler_fetch_ranges_test.go new file mode 100644 index 00000000..4fc4ab0b --- /dev/null +++ b/pkg/relay/handler_fetch_ranges_test.go @@ -0,0 +1,193 @@ +package relay + +import ( + "errors" + "io" + "math" + "slices" + "testing" + + "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/session/sessiontest" + "github.com/floatdrop/moq-go/pkg/moqt/wire" + "github.com/floatdrop/moq-go/pkg/relay/cache" + "github.com/floatdrop/moq-go/pkg/relay/internal/registry" +) + +// at is the Location {g, o}; rng the range [lo, hi]. +func at(g, o uint64) message.Location { return message.Location{Group: g, Object: o} } +func rng(lo, hi message.Location) registry.LocRange { + return registry.LocRange{Lo: lo, Hi: hi} +} + +const maxID = math.MaxUint64 + +// TestUncovered: the parts of a range no known range covers, with overlapping, +// touching and out-of-range known ranges, across a Group boundary. +func TestUncovered(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + known []registry.LocRange + want []registry.LocRange + }{ + {"nothing known", nil, []registry.LocRange{rng(at(1, 0), at(3, 5))}}, + { + "points and a range", []registry.LocRange{rng(at(1, 0), at(1, 0)), rng(at(1, 2), at(1, maxID)), rng(at(3, 5), at(3, 5))}, + []registry.LocRange{rng(at(1, 1), at(1, 1)), rng(at(2, 0), at(3, 4))}, + }, + { + "overlapping and touching", []registry.LocRange{rng(at(0, 0), at(1, 3)), rng(at(1, 2), at(1, 5)), rng(at(1, 6), at(9, 0))}, + nil, + }, + {"covering the end exactly", []registry.LocRange{rng(at(2, 0), at(3, 5))}, []registry.LocRange{rng(at(1, 0), at(1, maxID))}}, + { + "known ranges outside", []registry.LocRange{rng(at(0, 0), at(0, maxID)), rng(at(4, 0), at(4, 0))}, + []registry.LocRange{rng(at(1, 0), at(3, 5))}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := uncovered(at(1, 0), at(3, 5), tc.known); !slices.Equal(got, tc.want) { + t.Fatalf("uncovered = %v, want %v", got, tc.want) + } + }) + } +} + +// TestKnownFromCache: what cached Objects establish, from status Objects and +// gap Properties (§11.2.1.1, §12.8, §12.9). +func TestKnownFromCache(t *testing.T) { + t.Parallel() + gapProps := func(typ message.PropertyType, n uint64) []byte { + return message.AppendTrackProperties([]wire.KVPair{{Type: typ, IntVal: n}}) + } + got := knownFromCache([]*cache.CachedObject{ + {GroupID: 1, ObjectID: 0, Payload: []byte("x")}, + {GroupID: 1, ObjectID: 4, Status: message.ObjectStatusEndOfGroup}, + {GroupID: 5, ObjectID: 3, Payload: []byte("x"), Properties: gapProps(message.PropertyPriorObjectIDGap, 2)}, + {GroupID: 8, ObjectID: 0, Payload: []byte("x"), Properties: gapProps(message.PropertyPriorGroupIDGap, 3)}, + {GroupID: 9, ObjectID: 2, Status: message.ObjectStatusEndOfTrack}, + }) + want := []registry.LocRange{ + rng(at(1, 0), at(1, 0)), + rng(at(1, 4), at(1, maxID)), + rng(at(5, 3), at(5, 3)), rng(at(5, 1), at(5, 2)), + rng(at(8, 0), at(8, 0)), rng(at(5, 0), at(7, maxID)), + rng(at(9, 2), at(maxID, maxID)), + } + if !slices.Equal(got, want) { + t.Fatalf("knownFromCache = %v, want %v", got, want) + } +} + +// TestStreamCovered: what an upstream's End of Range marker covers, in the +// order the response carries Locations (see fetch_ranges.go). +func TestStreamCovered(t *testing.T) { + t.Parallel() + span := rng(at(2, 3), at(5, 1)) + prev := func(g, o uint64) *message.Location { l := at(g, o); return &l } + for _, tc := range []struct { + name string + prev *message.Location + at message.Location + order message.GroupOrder + want []registry.LocRange + }{ + {"ascending, first element", nil, at(3, 4), message.GroupOrderAscending, []registry.LocRange{rng(at(2, 3), at(3, 4))}}, + {"ascending, after one", prev(3, 4), at(4, 0), message.GroupOrderAscending, []registry.LocRange{rng(at(3, 5), at(4, 0))}}, + { + "descending, first element, same Group", nil, at(5, 0), message.GroupOrderDescending, + []registry.LocRange{rng(at(5, 0), at(5, 0))}, + }, + { + "descending, across Groups", prev(5, 0), at(2, 7), message.GroupOrderDescending, + []registry.LocRange{rng(at(2, 3), at(2, 7)), rng(at(3, 0), at(4, maxID)), rng(at(5, 1), at(5, 1))}, + }, + { + "descending, from a Group's last Object", prev(5, 1), at(4, 2), message.GroupOrderDescending, + []registry.LocRange{rng(at(4, 0), at(4, 2))}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := streamCovered(tc.prev, tc.at, span, tc.order) + slices.SortFunc(got, func(a, b registry.LocRange) int { return a.Lo.Compare(b.Lo) }) + if !slices.Equal(got, tc.want) { + t.Fatalf("streamCovered = %v, want %v", got, tc.want) + } + }) + } +} + +// TestFetchElements_DescendingRoundTrip: a Descending response with unknown +// runs inside, at the head of, and across Groups encodes (§11.4.4) and decodes +// back to its elements, markers included, in stream order. +func TestFetchElements_DescendingRoundTrip(t *testing.T) { + t.Parallel() + o := func(g, id uint64) *cache.CachedObject { + return &cache.CachedObject{GroupID: g, ObjectID: id, Payload: []byte{byte(id)}} + } + elems := fetchElements( + []*cache.CachedObject{o(1, 0), o(3, 2), o(3, 4), o(6, 0)}, + []registry.LocRange{ + rng(at(1, 1), at(3, 1)), // tail of 1, Group 2, head of 3 + rng(at(3, 3), at(3, 3)), // inside 3 + rng(at(4, 0), at(5, maxID)), + }, + nil, message.GroupOrderDescending) + + type el struct { + g, o uint64 + unknown bool + } + want := []el{ + {6, 0, false}, + {3, 1, true}, // Groups 5 and 4 and the head of 3: one run, no Object between + {3, 2, false}, {3, 3, true}, {3, 4, false}, + {2, maxID, true}, // Group 2 (the tail of 1 follows Object {1, 0}) + {1, 0, false}, + {1, maxID, true}, + } + + cli, srv := sessiontest.NewSessionPair(t) + writeErr := make(chan error, 1) + go func() { + out, err := cli.OpenFetchStream(message.FetchHeader{RequestID: 0}) + if err != nil { + writeErr <- err + return + } + if _, err := streamFetchObjects(out, elems, nil); err != nil { + out.Cancel(moqt.StreamResetInternalError) + writeErr <- err + return + } + writeErr <- out.Close() + }() + ds, err := srv.AcceptDataStream(t.Context()) + if err != nil { + t.Fatalf("AcceptDataStream: %v", err) + } + fs := ds.(*session.IncomingFetchStream) + fs.GroupOrder = message.GroupOrderDescending + var got []el + for { + d, err := fs.ReadDecoded() + if errors.Is(err, io.EOF) { + break + } + if err != nil { + t.Fatalf("ReadDecoded: %v", err) + } + got = append(got, el{d.GroupID, d.ObjectID, d.EndOfUnknownRange}) + } + if err := <-writeErr; err != nil { + t.Fatalf("writer: %v", err) + } + if !slices.Equal(got, want) { + t.Fatalf("decoded %v, want %v", got, want) + } +} diff --git a/pkg/relay/handler_fetch_split_test.go b/pkg/relay/handler_fetch_split_test.go index f7f4727e..aaa4c2ae 100644 --- a/pkg/relay/handler_fetch_split_test.go +++ b/pkg/relay/handler_fetch_split_test.go @@ -2,11 +2,9 @@ package relay import ( "math" - "slices" "testing" "github.com/floatdrop/moq-go/pkg/moqt/message" - "github.com/floatdrop/moq-go/pkg/relay/cache" ) func TestFetchPredecessor(t *testing.T) { @@ -82,36 +80,3 @@ func TestCapFetchEndLocation(t *testing.T) { t.Errorf("whole end group = %v, want the capped %v", got, largest) } } - -func TestMergeFetchObjects(t *testing.T) { - lower := []*cache.CachedObject{{GroupID: 0}, {GroupID: 1}} - upper := []*cache.CachedObject{{GroupID: 5}, {GroupID: 6}} - - asc := groupIDs(mergeFetchObjects(message.GroupOrderAscending, lower, upper)) - if !slices.Equal(asc, []uint64{0, 1, 5, 6}) { - t.Errorf("ascending merge = %v; want [0 1 5 6] (lower leads)", asc) - } - desc := groupIDs(mergeFetchObjects(message.GroupOrderDescending, lower, upper)) - if !slices.Equal(desc, []uint64{5, 6, 0, 1}) { - t.Errorf("descending merge = %v; want [5 6 0 1] (upper leads)", desc) - } - - // Degenerate inputs pass through unchanged. - emptyLower := groupIDs(mergeFetchObjects(message.GroupOrderAscending, nil, upper)) - if !slices.Equal(emptyLower, []uint64{5, 6}) { - t.Errorf("empty lower = %v; want [5 6]", emptyLower) - } - emptyUpper := groupIDs(mergeFetchObjects(message.GroupOrderAscending, lower, nil)) - if !slices.Equal(emptyUpper, []uint64{0, 1}) { - t.Errorf("empty upper = %v; want [0 1]", emptyUpper) - } -} - -// groupIDs returns the Group IDs of objs. -func groupIDs(objs []*cache.CachedObject) []uint64 { - out := make([]uint64, len(objs)) - for i, o := range objs { - out[i] = o.GroupID - } - return out -} diff --git a/pkg/relay/handler_fetch_stitch_test.go b/pkg/relay/handler_fetch_stitch_test.go index 8980f035..ee0f1f56 100644 --- a/pkg/relay/handler_fetch_stitch_test.go +++ b/pkg/relay/handler_fetch_stitch_test.go @@ -22,7 +22,7 @@ func TestFetch_StitchesEvictedRangeFromUpstream(t *testing.T) { video := ns("video") name := []byte("cam1") - const liveLo, liveHi = uint64(5), uint64(9) // cached → floor = group 5 + const liveLo, liveHi = uint64(5), uint64(9) // cached from group 5 const upstreamAlias = uint64(42) if _, err := upSess.PublishNamespace(t.Context(), &message.PublishNamespace{Namespace: video}); err != nil { @@ -31,7 +31,7 @@ func TestFetch_StitchesEvictedRangeFromUpstream(t *testing.T) { // Upstream loop: answer the on-demand SUBSCRIBE by pushing the live tail // (which the relay caches), and answer the stitch FETCH with the older, - // below-floor range. + // uncached range. go func() { for { req, err := upSess.AcceptRequest(t.Context()) @@ -61,11 +61,10 @@ func TestFetch_StitchesEvictedRangeFromUpstream(t *testing.T) { } case *message.Fetch: // Serve exactly the group range the relay asks for. The relay - // requests precisely the below-floor part, and the floor isn't - // fixed (the upstream subscription uses the Next Object filter, so - // the relay may not cache the upstream's first pushed group) — - // honouring the requested range keeps the split gapless whatever - // the floor turns out to be. + // requests precisely the uncached part, which isn't fixed (the + // upstream subscription uses the Next Object filter, so the relay + // may not cache the upstream's first pushed group) — honouring the + // requested range keeps the answer gapless whatever it is. // draft-20 carries the range in LOCATION_FILTER (§5.1.2), and the // relay always sends the absolute four-field form. f, ferr := message.LocationFilterFromParam(m.Parameters) diff --git a/pkg/relay/handler_fetch_unknown_test.go b/pkg/relay/handler_fetch_unknown_test.go index 8c958357..cf08f840 100644 --- a/pkg/relay/handler_fetch_unknown_test.go +++ b/pkg/relay/handler_fetch_unknown_test.go @@ -2,6 +2,7 @@ package relay_test import ( "math" + "sync" "testing" "time" @@ -15,7 +16,7 @@ import ( // unknownGapTopology wires upstream publisher → relay ← live subscriber, with // onFetch answering the relay's upstream FETCH, and returns a fetch-only // client. The upstream pushes single-object groups liveLo..liveHi, so the cache -// floor is liveLo and a FETCH from group 0 has a below-floor part. +// cached tail starts at liveLo, so a FETCH from group 0 has an uncached part. func unknownGapTopology( t *testing.T, ns wire.TrackNamespace, @@ -32,6 +33,8 @@ func unknownGapTopology( t.Fatalf("PublishNamespace: %v", err) } + written := make(chan struct{}) + tailWritten := sync.OnceFunc(func() { close(written) }) go func() { for { req, err := upSess.AcceptRequest(t.Context()) @@ -46,6 +49,7 @@ func unknownGapTopology( for g := liveLo; g <= liveHi; g++ { sg, err := upSess.OpenSubgroup(message.SubgroupHeader{ SubgroupIDMode: message.SubgroupIDImplicitZero, + EndOfGroup: true, // its FIN ends the Group TrackAlias: upstreamAlias, GroupID: g, }) @@ -55,6 +59,7 @@ func unknownGapTopology( _ = sg.WriteObject(&message.SubgroupObject{Payload: []byte{byte('a' + g)}}) _ = sg.Close() } + tailWritten() case *message.Fetch: onFetch(upSess, req, m) } @@ -75,19 +80,20 @@ func unknownGapTopology( // would hand back a fetch client while the cache is still filling, and // every caller's expected answer is stated in terms of a *full* tail — so // a fetch that lands early gets a legitimately different answer, with the - // unknown-range floor sitting wherever the cache happened to reach. + // unknown range ending wherever the cache happened to reach. // // Wait on the relay's watermark (TRACK_STATUS, §10.2.17), not on the // subscriber: the relay may drop a lagging subscriber (§3.3.4) while the // cache is fully populated. go drainAll(t.Context(), live) + awaitTailCached(t, written) fetchClient := dialAnotherClient(t, upSess) waitRelayLargest(t, fetchClient, ns, name, liveHi, 0) return fetchClient } -// TestFetch_UnknownRangeMarkerWhenUpstreamRejects: a below-floor part the +// TestFetch_UnknownRangeMarkerWhenUpstreamRejects: an uncached part the // upstream refuses to serve is covered by an End of Unknown Range marker before // the cached Objects, not left as a gap (§11.4.4). func TestFetch_UnknownRangeMarkerWhenUpstreamRejects(t *testing.T) { @@ -104,8 +110,8 @@ func TestFetch_UnknownRangeMarkerWhenUpstreamRejects(t *testing.T) { for { elems := tryFetchElems(t, fc, video, name, liveHi, nil) if len(elems) > 0 && elems[0].Unknown && groupsEqual(realGroups(elems), liveLo, liveHi) { - // The marker covers [request start, cache floor): its Location - // is the floor's predecessor. + // The marker covers the uncached part: its Location is the end of + // the Group before the cached tail. if wantG := liveLo - 1; elems[0].Group != wantG || elems[0].Object != math.MaxUint64 { t.Fatalf("unknown marker at {%d,%d}, want {%d,%d}", elems[0].Group, elems[0].Object, wantG, uint64(math.MaxUint64)) @@ -126,8 +132,10 @@ func TestFetch_UnknownRangeMarkerWhenUpstreamRejects(t *testing.T) { } // TestFetch_UnknownRangeMarkerDescending is the descending-order counterpart: -// the unserviceable below-floor range comes last in stream order, so the -// marker must trail the cached objects, at the range's start Location. +// the unserviceable uncached range comes last in stream order, so the +// marker trails the cached objects, at the range's last Location in stream +// order: Group 0's last Object (Groups descending, Objects ascending within +// one; see fetch_ranges.go). func TestFetch_UnknownRangeMarkerDescending(t *testing.T) { video := ns("video") name := []byte("cam-unknown-desc") @@ -151,8 +159,8 @@ func TestFetch_UnknownRangeMarkerDescending(t *testing.T) { } if len(elems) > 0 && descOK { last := elems[len(elems)-1] - if !last.Unknown || last.Group != 0 || last.Object != 0 { - t.Fatalf("want trailing unknown marker at {0,0}, got %+v (elems %v)", last, elems) + if !last.Unknown || last.Group != 0 || last.Object != math.MaxUint64 { + t.Fatalf("want trailing unknown marker at {0, 2^64-1}, got %+v (elems %v)", last, elems) } return } @@ -265,7 +273,7 @@ func TestFetch_UnknownMarkerWhenUpstreamCapsEndLocation(t *testing.T) { groupsEqual(got[:upstreamHi+1], 0, upstreamHi) && groupsEqual(got[upstreamHi+1:], liveLo, liveHi) { // One unknown marker, between the stitched head and the cached - // tail, at the below-floor sub-range's inclusive end. + // tail, at the uncached sub-range's inclusive end. if len(elems) != len(got)+1 || !elems[upstreamHi+1].Unknown { t.Fatalf("want single unknown marker after group %d, got elems %v", upstreamHi, elems) } @@ -306,7 +314,7 @@ func TestFetch_DiscardsOutOfRangeUpstreamElements(t *testing.T) { return } // Rogue marker beyond the requested range (the relay asked for - // the below-floor part only, ending before group liveLo). + // the uncached part only, ending before group liveLo). _ = out.WriteObject(&message.FetchObject{ SerializationFlags: message.FetchEndOfUnknownRange, GroupIDDelta: liveHi - 2, @@ -319,7 +327,7 @@ func TestFetch_DiscardsOutOfRangeUpstreamElements(t *testing.T) { for { elems := tryFetchElems(t, fc, video, name, liveHi, nil) if len(elems) > 0 && elems[0].Unknown && groupsEqual(realGroups(elems), liveLo, liveHi) { - // The rogue marker must not appear; the below-floor range is + // The rogue marker must not appear; the uncached range is // covered by the relay's own whole-sub-range marker instead. if wantG := liveLo - 1; elems[0].Group != wantG || elems[0].Object != math.MaxUint64 { t.Fatalf("marker at {%d,%d}, want relay's own at {%d,%d}", diff --git a/pkg/relay/handler_fetch_upstream_fail_test.go b/pkg/relay/handler_fetch_upstream_fail_test.go index 4ebcdbe0..df91210b 100644 --- a/pkg/relay/handler_fetch_upstream_fail_test.go +++ b/pkg/relay/handler_fetch_upstream_fail_test.go @@ -2,6 +2,7 @@ package relay_test import ( "fmt" + "math" "sync" "testing" "time" @@ -79,7 +80,7 @@ func TestFetch_UpstreamOutcomeDecidesGapOrUnknown(t *testing.T) { if got := stitchMarkerOf(objs); got != tc.wantMarker { t.Fatalf("stitched response encoded %s, want %s", got, tc.wantMarker) } - // The below-floor part must never be served from an upstream + // The uncached part must never be served from an upstream // response the relay could not read to completion. Groups below // the cached tail would be exactly that. if groups := stitchedGroups( @@ -93,11 +94,11 @@ func TestFetch_UpstreamOutcomeDecidesGapOrUnknown(t *testing.T) { } } -// TestFetch_DescendingCappedUpstreamFallsBackToWholeUnknown: when the upstream -// caps FETCH_OK below the requested sub-range (§10.13), a descending response -// marks the whole sub-range unknown, anchored at its start, since a trailing -// marker cannot be placed. -func TestFetch_DescendingCappedUpstreamFallsBackToWholeUnknown(t *testing.T) { +// TestFetch_DescendingCappedUpstreamMarksRemainder: when the upstream caps +// FETCH_OK below the requested sub-range (§10.13), a descending response marks +// what lies past the cap unknown, and nothing below it: under the upstream's +// clean FIN the rest does not exist. +func TestFetch_DescendingCappedUpstreamMarksRemainder(t *testing.T) { t.Parallel() objs := runStitch(t, stitchOpts{ order: message.GroupOrderDescending, @@ -121,29 +122,18 @@ func TestFetch_DescendingCappedUpstreamFallsBackToWholeUnknown(t *testing.T) { }, }) - // Presence of a marker is not enough to tell the two encodings apart: the - // ascending path also emits one here. What separates them is WHERE it is - // anchored. unknownWholeRange anchors a descending marker at the - // sub-range START (group 0 — the whole below-floor range is unknown), - // while the per-remainder path appends one at endIncl, the top of that - // range. A marker in the wrong place is a well-formed response making a - // false claim about which objects are undetermined. - var marker *session.DecodedFetchObject + // The sub-range is Groups 0-4 (the cache holds 5-9); the cap is {3, 0}. + // Past it lie {3, 1} through Group 4, whose run ends, in stream order, + // with Group 3's last Object. Groups 0-2 and {3, 0} are a plain gap. + var markers []*session.DecodedFetchObject for _, o := range objs { if o.EndOfUnknownRange { - marker = o - break + markers = append(markers, o) } } - if marker == nil { - t.Fatalf("a capped descending upstream response produced no unknown marker; "+ - "the uncovered remainder was encoded as an authoritative gap (objects: %v)", - stitchedGroups(objs)) - } - if marker.GroupID != 0 { - t.Errorf("unknown marker anchored at group %d, want 0 — descending must fall back "+ - "to marking the WHOLE sub-range unknown, not just the uncovered remainder", - marker.GroupID) + if len(markers) != 1 || markers[0].GroupID != 3 || markers[0].ObjectID != math.MaxUint64 { + t.Fatalf("unknown markers %v, want one at {3, 2^64-1} covering what lies past the cap "+ + "(objects: %v)", markers, stitchedGroups(objs)) } } @@ -209,7 +199,7 @@ func TestFetch_StitchedObjectKeepsDatagramForwardingPreference(t *testing.T) { } const ( - stitchLiveLo = uint64(5) // cached live tail: groups 5..9, so the eviction floor is 5 + stitchLiveLo = uint64(5) // cached live tail: groups 5..9, so Groups 0-4 are uncached stitchLiveHi = uint64(9) ) @@ -246,8 +236,8 @@ func replyThen(end func(*session.OutgoingFetchStream)) func(*session.Session, *s } // runStitch runs the stitch topology of TestFetch_StitchesEvictedRangeFromUpstream -// (an upstream feeding a cached live tail, and a FETCH reaching below the -// eviction floor) and returns the stitched response's elements. +// (an upstream feeding a cached live tail, and a FETCH reaching the uncached +// Groups below it) and returns the stitched response's elements. func runStitch(t *testing.T, opts stitchOpts) []*session.DecodedFetchObject { t.Helper() upSess, teardown := connectRelay(t, relay.Config{}) @@ -288,6 +278,8 @@ func runStitch(t *testing.T, opts stitchOpts) []*session.DecodedFetchObject { return fmt.Sprintf("; the upstream goroutine had already stopped: %v", upFail) } + written := make(chan struct{}) + tailWritten := sync.OnceFunc(func() { close(written) }) go func() { for { req, err := upSess.AcceptRequest(t.Context()) @@ -306,6 +298,7 @@ func runStitch(t *testing.T, opts stitchOpts) []*session.DecodedFetchObject { for g := stitchLiveLo; g <= stitchLiveHi; g++ { sg, err := openSubgroupWaiting(t, upSess, message.SubgroupHeader{ SubgroupIDMode: message.SubgroupIDImplicitZero, + EndOfGroup: true, // its FIN ends the Group TrackAlias: upstreamAlias, GroupID: g, }) @@ -322,6 +315,7 @@ func runStitch(t *testing.T, opts stitchOpts) []*session.DecodedFetchObject { return } } + tailWritten() case *message.Fetch: if opts.onFetch == nil { continue // never answer: the relay must time out @@ -339,9 +333,10 @@ func runStitch(t *testing.T, opts stitchOpts) []*session.DecodedFetchObject { } t.Cleanup(func() { _ = liveReq.Close() }) go drainAll(t.Context(), live) + awaitTailCached(t, written) // Retry until the cached tail is present: before that the FETCH is either - // rejected or answers from an empty cache, and the below-floor split this + // rejected or answers from an empty cache, and the uncached part this // test is about has not happened yet. fc := dialAnotherClient(t, upSess) deadline := time.Now().Add(10 * time.Second) @@ -468,7 +463,7 @@ func TestFetch_RangeFilterKeepsTimedOutMarker(t *testing.T) { }) // onFetch nil + a short FILL_TIMEOUT is the §10.2.5 budget-exhausted path, - // which reports the below-floor span as an End of Timed-Out Range. + // which reports the uncached span as an End of Timed-Out Range. objs := runStitch(t, stitchOpts{ onFetch: nil, fillTimeout: 300 * time.Millisecond, diff --git a/pkg/relay/helpers_fetch_test.go b/pkg/relay/helpers_fetch_test.go index 1df96f88..e61fd08a 100644 --- a/pkg/relay/helpers_fetch_test.go +++ b/pkg/relay/helpers_fetch_test.go @@ -122,8 +122,22 @@ func fetchAndDrain( return reqStream.OK, decodeFetchStream(t, fs, order) } +// awaitTailCached waits, up to 5s, for an upstream to close written once it +// has written its live tail, then lets the relay cache the last of it. Writes +// over the test transport return once the relay has read them. +func awaitTailCached(t *testing.T, written <-chan struct{}) { + t.Helper() + select { + case <-written: + case <-time.After(5 * time.Second): + t.Fatal("the upstream never wrote its live tail") + } + time.Sleep(50 * time.Millisecond) +} + // decodeFetchStream reads fs to EOF, reversing the §11.4.4.1 delta encoding -// itself rather than through the session decoder. +// itself rather than through the session decoder. End of Range markers +// (§11.4.4.2) are skipped, but anchor the Object after them. func decodeFetchStream(t *testing.T, fs *session.IncomingFetchStream, order message.GroupOrder) []decodedFetchObject { t.Helper() var ( @@ -141,6 +155,10 @@ func decodeFetchStream(t *testing.T, fs *session.IncomingFetchStream, order mess } t.Fatalf("ReadObject: %v", err) } + if isEndOfRange(fo.SerializationFlags) { + prevGroup, prevObject, havePrev = fo.GroupIDDelta, fo.ObjectIDDelta, true + continue + } var g, o uint64 switch { @@ -171,6 +189,15 @@ func decodeFetchStream(t *testing.T, fs *session.IncomingFetchStream, order mess } } +// isEndOfRange reports whether flags are a §11.4.4.2 End of Range marker's. +func isEndOfRange(flags uint64) bool { + switch flags { + case message.FetchEndOfNonExistentRange, message.FetchEndOfUnknownRange, message.FetchEndOfTimedOutRange: + return true + } + return false +} + // fetchElem is one element of a FETCH response: an Object or a §11.4.4.2 // End of Range marker. type fetchElem struct { diff --git a/pkg/relay/inbound_goaway_test.go b/pkg/relay/inbound_goaway_test.go index e7bcb9b8..6234e0ae 100644 --- a/pkg/relay/inbound_goaway_test.go +++ b/pkg/relay/inbound_goaway_test.go @@ -1,9 +1,12 @@ package relay_test import ( + "sync" + "sync/atomic" "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/relay" @@ -108,7 +111,12 @@ func TestRelay_NoUpstreamFetchToGoingAwayPublisher(t *testing.T) { if _, err := pubSess.PublishNamespace(t.Context(), &message.PublishNamespace{Namespace: video}); err != nil { t.Fatalf("PublishNamespace: %v", err) } + // A FETCH before the GOAWAY may be stitched from the publisher (a Group + // still in flight is unknown to the relay); only one after it counts. + var goneAway atomic.Bool fetches := make(chan *session.Request, 4) + written := make(chan struct{}) + tailWritten := sync.OnceFunc(func() { close(written) }) go func() { for { req, err := pubSess.AcceptRequest(t.Context()) @@ -123,6 +131,7 @@ func TestRelay_NoUpstreamFetchToGoingAwayPublisher(t *testing.T) { for g := stitchLiveLo; g <= stitchLiveHi; g++ { sg, err := openSubgroupWaiting(t, pubSess, message.SubgroupHeader{ SubgroupIDMode: message.SubgroupIDImplicitZero, TrackAlias: 42, GroupID: g, + EndOfGroup: true, // so the cache alone answers the tail }) if err != nil { return @@ -130,7 +139,12 @@ func TestRelay_NoUpstreamFetchToGoingAwayPublisher(t *testing.T) { _ = sg.WriteObject(&message.SubgroupObject{Payload: []byte{byte('a' + g)}}) _ = sg.Close() } + tailWritten() case *message.Fetch: + if !goneAway.Load() { + _ = req.RejectError(moqt.RequestDoesNotExist, "not yet") + continue + } fetches <- req } } @@ -141,6 +155,7 @@ func TestRelay_NoUpstreamFetchToGoingAwayPublisher(t *testing.T) { t.Fatalf("Subscribe: %v", err) } go drainAll(t.Context(), live) + awaitTailCached(t, written) // Wait until the cache holds the live tail, with a FETCH the cache // answers alone. @@ -158,6 +173,7 @@ func TestRelay_NoUpstreamFetchToGoingAwayPublisher(t *testing.T) { time.Sleep(20 * time.Millisecond) } + goneAway.Store(true) if err := pubSess.SendGoaway(10*time.Second, ""); err != nil { t.Fatalf("SendGoaway: %v", err) } diff --git a/pkg/relay/internal/registry/ledger_test.go b/pkg/relay/internal/registry/ledger_test.go index 8ef614d3..33514cf9 100644 --- a/pkg/relay/internal/registry/ledger_test.go +++ b/pkg/relay/internal/registry/ledger_test.go @@ -2,6 +2,7 @@ package registry_test import ( "errors" + "math" "testing" "github.com/floatdrop/moq-go/pkg/moqt/message" @@ -526,3 +527,19 @@ func TestTrackEntry_LowestForwarded(t *testing.T) { } } } + +// TestTrackEntry_EndAfterLastObjectID: a hard end after Object 2^64-1 has +// nothing past it, so it records no absence rather than wrapping to Object 0. +func TestTrackEntry_EndAfterLastObjectID(t *testing.T) { + t.Parallel() + const last = math.MaxUint64 + e := newTestEntry("last-id") + mustClaim(t, e, registry.ObjectInfo{Group: 1, Object: last, Datagram: true, EndOfGroup: true}, registry.ClaimFresh) + mustClaim(t, e, registry.ObjectInfo{Group: 2, Object: last}, registry.ClaimFresh) + if err := e.SubgroupEnded(registry.ObjectInfo{Group: 2, Object: last}, true); err != nil { + t.Fatalf("SubgroupEnded: %v", err) + } + if got := e.KnownAbsent(); len(got) != 0 { + t.Fatalf("KnownAbsent = %v, want nothing", got) + } +} diff --git a/pkg/relay/internal/registry/track_entry.go b/pkg/relay/internal/registry/track_entry.go index 502db9cb..9946e9ad 100644 --- a/pkg/relay/internal/registry/track_entry.go +++ b/pkg/relay/internal/registry/track_entry.go @@ -432,6 +432,9 @@ func (e *TrackEntry) SubgroupEnded(lastObj ObjectInfo, endOfGroup bool) error { hard := lastObj.Status == message.ObjectStatusNormal at := lastObj.Object if hard { + if at == math.MaxUint64 { + return nil // nothing lies past it + } at++ } ends := end{at: at, set: true, hard: hard} @@ -519,15 +522,53 @@ func (e *TrackEntry) LowestForwarded(group, subgroup uint64) (uint64, bool) { return sg.minObject, sg.hasObject } +// LocRange is an inclusive range of Locations, Lo through Hi. +type LocRange struct{ Lo, Hi message.Location } + +// KnownAbsent reports the Locations the ledger knows do not exist (§2.1: +// "All signals that an Object does not exist are authoritative"), within the +// Groups it holds: each Group's end onward (§11.2.1.1, §11.4.2; a status end +// at M from M, since a status Object is not one a FETCH serializes), and the Object +// and Group ID gaps announced (§12.8, §12.9). The ranges may overlap and are +// in no particular order. A FIN or END_OF_GROUP bit that ended a Group is +// known only here, not from the cache. +func (e *TrackEntry) KnownAbsent() []LocRange { + e.deliveredMu.Lock() + defer e.deliveredMu.Unlock() + var out []LocRange + for id, g := range e.delivered { + if g.end.set { + out = append(out, LocRange{ + Lo: message.Location{Group: id, Object: g.end.at}, + Hi: message.Location{Group: id, Object: math.MaxUint64}, + }) + } + for _, r := range g.objectGaps { + out = append(out, LocRange{ + Lo: message.Location{Group: id, Object: r.lo}, + Hi: message.Location{Group: id, Object: r.hi}, + }) + } + } + for _, r := range e.groupGaps { + out = append(out, LocRange{ + Lo: message.Location{Group: r.lo}, + Hi: message.Location{Group: r.hi, Object: math.MaxUint64}, + }) + } + return out +} + // groupEnd reports where o ends its Group (see [end]), if it does: an // END_OF_GROUP or END_OF_TRACK status at M at M (§11.2.1.1); a datagram's // END_OF_GROUP bit on Object N at N+1, which §2.4.2's non-exhaustive list does -// not name but §11.3.1 defines alike. +// not name but §11.3.1 defines alike. After Object 2^64-1 nothing lies past, +// so that bit ends nothing. func groupEnd(o ObjectInfo) (at uint64, hard, ok bool) { switch { case o.Status == message.ObjectStatusEndOfGroup, o.Status == message.ObjectStatusEndOfTrack: return o.Object, false, true - case o.Datagram && o.EndOfGroup: + case o.Datagram && o.EndOfGroup && o.Object < math.MaxUint64: return o.Object + 1, true, true } return 0, false, false diff --git a/pkg/relay/malformed_track_test.go b/pkg/relay/malformed_track_test.go index 609a29f3..8cd14b4a 100644 --- a/pkg/relay/malformed_track_test.go +++ b/pkg/relay/malformed_track_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io" + "sync" "sync/atomic" "testing" "time" @@ -455,6 +456,8 @@ func refusedFetchResetsStream(t *testing.T, upstreamProps, objProps []byte) { if _, err := pubSess.PublishNamespace(t.Context(), &message.PublishNamespace{Namespace: video}); err != nil { t.Fatalf("PublishNamespace: %v", err) } + written := make(chan struct{}) + tailWritten := sync.OnceFunc(func() { close(written) }) go func() { for { req, err := pubSess.AcceptRequest(t.Context()) @@ -468,7 +471,7 @@ func refusedFetchResetsStream(t *testing.T, upstreamProps, objProps []byte) { } for g := stitchLiveLo; g <= stitchLiveHi; g++ { sg, err := openSubgroupWaiting(t, pubSess, message.SubgroupHeader{ - SubgroupIDMode: message.SubgroupIDImplicitZero, TrackAlias: 42, GroupID: g, + SubgroupIDMode: message.SubgroupIDImplicitZero, TrackAlias: 42, GroupID: g, EndOfGroup: true, }) if err != nil { return @@ -476,6 +479,7 @@ func refusedFetchResetsStream(t *testing.T, upstreamProps, objProps []byte) { _ = sg.WriteObject(&message.SubgroupObject{Payload: []byte{byte('a' + g)}}) _ = sg.Close() } + tailWritten() case *message.Fetch: _ = req.Reply(&message.FetchOK{ EndLocation: message.Location{Group: stitchLiveLo - 1}, @@ -503,6 +507,7 @@ func refusedFetchResetsStream(t *testing.T, upstreamProps, objProps []byte) { t.Fatalf("Subscribe: %v", err) } go drainAll(t.Context(), live) + awaitTailCached(t, written) fc := dialAnotherClient(t, pubSess) deadline := time.Now().Add(5 * time.Second) for { diff --git a/pkg/relay/session_handler.go b/pkg/relay/session_handler.go index 5221c31b..5ab66fb7 100644 --- a/pkg/relay/session_handler.go +++ b/pkg/relay/session_handler.go @@ -678,8 +678,8 @@ func (h *sessionHandler) streamFetchRange( unwatch := context.AfterFunc(ctx, cancelOut) defer unwatch() - // Gather cached objects, stitching the below-floor portion from upstream - // when the cache doesn't cover the whole range (§9.4). + // Gather cached objects, asking an upstream about what the cache cannot + // vouch for (§10.13). objs, refusal := h.stitchedFetchObjects(ctx, entry, fullName, start, end, order, fillTimeout) if ctx.Err() != nil { cancelOut() // before the deferred StreamClosed (§10.12) From 426b1d961f7aea119723357d8ebd02b5246c2316 Mon Sep 17 00:00:00 2001 From: Vsevolod Strukchinsky Date: Sat, 26 Sep 2026 19:32:40 +0500 Subject: [PATCH 9/9] =?UTF-8?q?fix(relay):=20FILL=5FTIMEOUT=20bounds=20an?= =?UTF-8?q?=20upstream=20FETCH's=20response=20too=20(=C2=A710.2.5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FILL_TIMEOUT budget bounded only the upstream FETCH's round trip and the wait for its data stream. An upstream that answered FETCH_OK, opened the stream and then stalled held the downstream FETCH, or a fill stream and with it the subscription's PUBLISH_DONE, past the budget, although §10.2.5 has the relay then report "any remaining unavailable Objects as Timed-Out gaps". It matters more since 90c9730: any cached FETCH over a Group whose end the relay does not know now asks upstream. When the budget runs out mid-read the upstream stream is cancelled; what arrived is served, and the rest of the span it neither sent nor marked is reported Timed-Out (0x20C), between the Objects it sent too: without a FIN its gaps assert nothing (§10.13), as for a response that fails mid-read. TestFetch_FillTimeoutBoundsUpstreamRead (the upstream sends two Objects of a three-Object hole and stalls) fails on the unpatched code: the downstream response never arrives. Keeping only what follows the last Object Timed-Out fails it too, serving the interior gap as non-existence. Co-Authored-By: Claude Opus 5.5 (1M context) --- STATUS.md | 6 ++-- pkg/relay/fetch_holes_test.go | 67 ++++++++++++++++++++++++++++++++++- pkg/relay/handler_fetch.go | 16 +++++++++ 3 files changed, 84 insertions(+), 5 deletions(-) diff --git a/STATUS.md b/STATUS.md index 514048f2..6ef3b075 100644 --- a/STATUS.md +++ b/STATUS.md @@ -163,7 +163,7 @@ By package, bottom-up along the dependency stack: | 10.2.2 | AUTHORIZATION_TOKEN | 0x03 | DONE | 4 alias types; session token cache resolves inbound. | | 10.2.3 | SUBGROUP_DELIVERY_TIMEOUT | 0x06 | PARTIAL| Parsed and resolved; the stream reset is not enforced on the bundled transports, and datagrams are not dropped (see §8). | | 10.2.4 | OBJECT_DELIVERY_TIMEOUT | 0x02 | DONE | | -| 10.2.5 | FILL_TIMEOUT | 0x0A | DONE | | +| 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. | @@ -575,8 +575,6 @@ Relay: delivery-timeout override (§8, §12.1, §12.2). - FETCH_OK never sets End Of Track (§10.14). - A cancelled FETCH keeps writing its data stream (§5.2: "MUST reset"). -- FILL_TIMEOUT does not bound an upstream FETCH once its data stream is open - (§10.2.5 SHOULD). - Objects from an upstream FETCH are exempt from MAX_CACHE_DURATION, and cached Objects age from when they were read whole rather than their beginning (§12.3). - A fill range is evaluated against a later Largest Object than SUBSCRIBE_OK or @@ -605,7 +603,7 @@ Documentation: GROUP_ORDER entry omits PUBLISH and FILL_PARAMETERS; 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.5, 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.8, 10.2.9, 10.2.15, 10.2.18, 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/relay/fetch_holes_test.go b/pkg/relay/fetch_holes_test.go index 0d7ddb0a..51b2a925 100644 --- a/pkg/relay/fetch_holes_test.go +++ b/pkg/relay/fetch_holes_test.go @@ -7,6 +7,7 @@ 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/relay" @@ -60,9 +61,10 @@ func fetchCam1Range( sess *session.Session, start, end message.Location, order message.GroupOrder, + extra ...message.Parameter, ) []fetchElem { t.Helper() - params := message.Parameters{fetchRangeFilter(start, end)} + params := append(message.Parameters{fetchRangeFilter(start, end)}, extra...) if order == message.GroupOrderDescending { params = append(params, message.GroupOrderParam(order)) } @@ -329,3 +331,66 @@ func TestFetch_UpstreamUnknownKeepsWhatTheRelayKnows(t *testing.T) { }) } } + +// TestFetch_FillTimeoutBoundsUpstreamRead: FILL_TIMEOUT is the budget for the +// whole upstream FETCH, its response included (§10.2.5). An upstream that +// sends part of its answer and stalls has what it sent served, and the rest +// reported "as Timed-Out gaps" — between what it sent too, since without a FIN +// its gaps assert nothing (§10.13). +func TestFetch_FillTimeoutBoundsUpstreamRead(t *testing.T) { + t.Parallel() + upSess, teardown := connectRelay(t, relay.Config{}) + t.Cleanup(teardown) + if _, err := upSess.PublishNamespace(t.Context(), &message.PublishNamespace{Namespace: ns("video")}); err != nil { + t.Fatalf("PublishNamespace: %v", err) + } + go func() { + for { + req, err := upSess.AcceptRequest(t.Context()) + if err != nil { + return + } + switch m := req.First.(type) { + case *message.Subscribe: + if req.Reply(&message.SubscribeOK{TrackAlias: 42}) != nil { + return + } + // The live stream misses Objects 2 to 4. + publishCam1Group(t, upSess, 42, true, + cam1Object{0, 0, nil}, cam1Object{0, 1, nil}, cam1Object{0, 5, nil}) + case *message.Fetch: + _, end, _ := fetchRequestRange(m) + if req.Reply(&message.FetchOK{EndLocation: end}) != nil { + return + } + out, err := upSess.OpenFetchStream(message.FetchHeader{RequestID: m.RequestID}) + if err != nil { + return + } + // Objects 2 and 4, then nothing: the stream stays open. + _ = out.WriteObject(&message.FetchObject{ + SerializationFlags: message.FetchFlagGroupIDDelta | message.FetchFlagObjectIDDelta | + message.FetchFlagPriority | uint64(message.FetchSubgroupIDExplicit), + GroupIDDelta: 0, ObjectIDDelta: 2, ObjectPayload: []byte("x"), + }) + _ = out.WriteObject(&message.FetchObject{ + SerializationFlags: message.FetchFlagObjectIDDelta | uint64(message.FetchSubgroupIDPrior), + ObjectIDDelta: 2, ObjectPayload: []byte("x"), + }) + t.Cleanup(func() { out.Cancel(moqt.StreamResetCancelled) }) + } + } + }() + live := dialAnotherClient(t, upSess) + subscribeCam1(t, live) + go drainAll(t.Context(), live) + fc := dialAnotherClient(t, upSess) + waitRelayLargest(t, fc, ns("video"), []byte("cam1"), 0, 5) + + got := fetchCam1Range(t, fc, message.Location{}, message.Location{Group: 0, Object: 5}, + message.GroupOrderAscending, message.FillTimeoutParam(300*time.Millisecond)) + want := []fetchElem{obj(0, 0), obj(0, 1), obj(0, 2), timedOutAt(0, 3), obj(0, 4), obj(0, 5)} + if !slices.Equal(got, want) { + t.Fatalf("FETCH elements %v, want %v", got, want) + } +} diff --git a/pkg/relay/handler_fetch.go b/pkg/relay/handler_fetch.go index 09a21e3f..94b99403 100644 --- a/pkg/relay/handler_fetch.go +++ b/pkg/relay/handler_fetch.go @@ -7,6 +7,7 @@ import ( "io" "log/slog" "math" + "slices" "time" "github.com/floatdrop/moq-go/pkg/moqt" @@ -466,6 +467,9 @@ func (h *sessionHandler) fetchUpstreamRange( // deltas (§11.4.4.1); the upstream serves in the order our FETCH asked // for. fs.GroupOrder = order + // §10.2.5: the budget covers the response too; when it runs out, what + // has arrived is kept and the rest reported Timed-Out. + defer context.AfterFunc(fctx, func() { fs.Cancel(moqt.StreamResetCancelled) })() var prev *message.Location for { @@ -479,6 +483,18 @@ func (h *sessionHandler) fetchUpstreamRange( fs.Cancel(moqt.StreamResetMalformedTrack) return upstreamAnswer{}, err } + if err != nil && fctx.Err() != nil { + // Without a FIN its gaps assert nothing (§10.13): all of the span + // it did not send or mark is Timed-Out. + h.log.LogAttrs(ctx, slog.LevelDebug, "upstream FETCH response timed out mid-read") + known := slices.Concat(ans.unknown, ans.timedOut) + for _, o := range ans.objs { + loc := message.Location{Group: o.GroupID, Object: o.ObjectID} + known = append(known, registry.LocRange{Lo: loc, Hi: loc}) + } + ans.timedOut = append(ans.timedOut, uncovered(span.Lo, span.Hi, known)...) + return ans, nil + } if err != nil { // No FIN (or a FIN mid-object), so the gaps in what arrived // assert nothing.