Second draft-20 review: backlog and the six high-severity fixes - #110
Merged
Merged
Conversation
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) <noreply@anthropic.com>
…IX update (§10.19, §10.20) 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) <noreply@anthropic.com>
…; AcceptRequest closes on §10.1 and §10.2.2 faults 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) <noreply@anthropic.com>
…missing an aged-out Object (§9.4, §11.4.3)
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) <noreply@anthropic.com>
…treams (§5.1.1, §5.1.3.1) 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) <noreply@anthropic.com>
…'s (§10.2.8, §12.5)
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) <noreply@anthropic.com>
…on uses (§10.20.1) 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) <noreply@anthropic.com>
… on a signal (§2.1, §10.13)
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) <noreply@anthropic.com>
….2.5) 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A second full compliance review against draft-ietf-moq-transport-20 (at
dbe571e). This PR records its findings as the new STATUS.md backlog and fixes the six high-severity items, plus two follow-ups. One commit per fix. Every regression test was run on the unpatched code first and seen failing, and every commit went through the moqt-reviewer until nothing was left open.Commits
7f3500b— backlog. Records the review in STATUS.md: session-layer, validation, relay and documentation items, each with the rule it misses.d03e6f9— auth bypass (§10.19, §10.20). Before this, a REQUEST_UPDATE that changed a SUBSCRIBE_NAMESPACE / SUBSCRIBE_TRACKS prefix was never authorized, so a subscriber could widen itself to(). Now:Session.VerifyTokens.4e6aff5— request loop (§3.3.2, §3.3.3, §10.1, §10.2.2). A request stream FINed or reset before its first message used to end the relay's request loop and leave the session open. Now it fails only that request.AcceptRequestalso closes the session itself on a Request ID violation (INVALID_REQUEST_ID) or a token-cache fault.cd91e77— dedup window (§9.4, §11.4.3). The window now holds 32 distinct Groups rather than 32 IDs of distance. An aged-out Object is:9a54303— cancellation (§5.1.1, §5.1.3.1). A cancelled subscription's open subgroup and fill streams are reset.5804df6,1ed76df— group order (§10.2.8, §12.5, §10.20.1). A subscription that omits GROUP_ORDER takes the track's DEFAULT_PUBLISHER_GROUP_ORDER, so its fill decodes correctly. A forwarded PUBLISH always states the order.90c9730— FETCH cache holes (§2.1, §10.13). The relay now treats an uncached Location as non-existent only on a signal: a Prior Gap, a Group or Track end, or an upstream's answer.426b1d9— FILL_TIMEOUT (§10.2.5). The budget now also bounds reading the upstream's response. When it runs out, what has arrived is served and the rest is Timed-Out.Decisions and interpretations (chosen with the maintainer)
{G, 2^64-1}. This replaces the old whole-range marker at the range Start. Interop with moxygen/moqtail is unverified; it is recorded as a Limitation.AcceptRequestcloses the session itself on §10.1 and §10.2.2 faults.Two further calls came out of review:
API changes
session.AcceptRequestnow closes the session on*ErrRequestIDParityViolation,*ErrDuplicateRequestIDand*TokenCacheError, and still returns them. Callers no longer need to close.session.Session.VerifyTokens.Authorizeris also called for prefix updates. The doc comment is updated.cache.ObjectCache.OldestRetained.GetRangenow skips expired Objects instead of marking them.Known gaps and consequences
cmd/video/cmd/clockcould set the END_OF_GROUP bit on single-subgroup Groups; that is not done here. This is recorded in Limitations.BenchmarkFetchFromCachegoes from 115 to 120 allocs/op: per FETCH, not per Object. Per-Object benchmarks are unchanged.🤖 Generated with Claude Code