fix: release upstream subscriptions for idle cached tracks, and stop FIN-ing subgroup streams mid-object (draft-14)

[englishm-cloudflare]

What this changes

Backport of #196 to draft-ietf-moq-transport-14.

#191 was reported against the draft-14 deployment, so the fix needs to land on that branch as well as main. main is 112 commits ahead of this base and the relay was substantially reworked in between — ANNOUNCE became PUBLISH_NAMESPACE, Locals grew a pull-through cache, subscribed.rs went from 463 to 1337 lines — so nothing cherry-picked cleanly. This is a re-implementation of the same fixes against draft-14's structure. Where the shape had to change, it is called out below.

1. Reset subgroup streams instead of FIN-ing mid-object

quinn::SendStream::drop calls finish() — a clean FIN at the current offset. subscribed.rs built SubgroupOutput::Stream(Writer::new(send_stream)) and never called finish() explicitly, so every early return from the forwarding loop sent a FIN wherever it happened to stop.

When that offset is inside an object, the receiver has already been promised payload_length bytes by the object header. It sees a subgroup that ended cleanly mid-object and treats the track as malformed.

Two triggers in a relay:

• Downstream UNSUBSCRIBE — recv_unsubscribe → remove_subscribe → ObjectForwarderRecv dropped → the state.lock_mut().ok_or(ServeError::Done)? in serve_subgroup_objects fails after the object header was already encoded → Writer dropped → FIN mid-object.
• Upstream failing mid-object — SubgroupObjectWriter dropped with remain != 0 sets ServeError::Size, producing the same truncating FIN downstream.

draft-14 §10.4.3 ("Closing Subgroup Streams") permits a FIN only when every object in the subgroup has been delivered to the QUIC stream, and names UNSUBSCRIBE as a case that MUST use RESET_STREAM. The requirement is unchanged from draft-16, so the fix is the same:

• A SubgroupStream wrapper that resets on drop unless explicitly finished, so the spec-safe outcome is the default and async cancellation is covered without a separate code path.
• Track owed payload bytes and refuse to FIN while mid-object.
• Move the liveness/location check ahead of the object header encode, so a cancellation in that window stops on an object boundary rather than after the promise.
• Add the reset code registry as DataStreamResetCode and map failures onto it.

The regression test was verified to actually catch the bug: reverting only the check reordering makes unsubscribe_mid_subgroup_resets_at_an_object_boundary fail with "no partial object should follow the last complete one" — the same failure it produces on main.

Draft-14 difference — the reset code registry is smaller. draft-14 §13.1.8 defines only four codes: INTERNAL_ERROR (0x0), CANCELLED (0x1), DELIVERY_TIMEOUT (0x2), SESSION_CLOSED (0x3). MALFORMED_TRACK (0x12) does not exist yet, so the case #196 mapped to it — an upstream object that ran short of its declared payload_length — collapses into INTERNAL_ERROR here, with a comment at the mapping recording why. CANCELLED and SESSION_CLOSED map identically to main. This is the least-wrong option available on the wire for draft-14.

2. Release upstream subscriptions for idle cached tracks (#191)

A relay caches tracks it does not publish itself so several downstream subscribers can share one upstream subscription. Nothing counted how many subscribers were actually using a cached reader, so the upstream subscription lived until the upstream session died — after the last subscriber left, the relay kept receiving a track nobody was watching.

New TrackInterest / TrackInterestGuard: a reference count backed by a watch channel that can also await "unwatched for a while". Subscribers hold a guard while being served, so the count is maintained by RAII and a cancelled subscriber cannot leak a reference. Once an entry has been unwatched for the grace period it is evicted and the upstream Subscribe is dropped, sending UNSUBSCRIBE.

Both cache layers are covered: the pull-through cache reached through TracksReader::subscribe, and the cross-relay track cache in RemoteManager.

Three details this depends on, unchanged from #196:

• Guards hold a strong reference to the counter, not a cache key. An outstanding guard from an evicted entry decrements the counter it came from rather than making a same-named replacement look busy.
• Guards are created under the same lock the idle check is made under. A subscriber racing eviction is therefore either counted (eviction abandoned) or misses the cache and requests a fresh entry.
• The entry is cleared before the upstream subscription is dropped, so a subscriber arriving in that window re-subscribes rather than attaching to a reader about to go silent.

Draft-14 difference — TrackInterest lives in moq_transport::serve, not the relay crate. On main the pull-through cache is in Locals inside moq-relay-ietf, so interest tracking naturally went there. On draft-14 the cache is TracksState inside moq-transport, and the second invariant above requires the guard be taken under the same lock as the idle check — which is that crate's lock. Putting the guard in the relay crate would mean taking it after releasing the cache lock, reopening exactly the eviction race the design exists to close. So it goes in moq-transport alongside the state it guards. #180 independently reached the same placement on the same base.

This is what makes the API break land on moq-transport rather than moq-relay-ietf — see Compatibility.

3. Bound the upstream subscribe handshake

Related to the above, and part of the same problem: subscribe_open waits for SUBSCRIBE_OK, which an upstream is under no obligation to ever send. Subscribe::ok() loops on a state notify until the ack arrives or the session closes, so the wait was effectively unbounded. This predates the PR — the previous code called Subscriber::subscribe, which is subscribe_open().await? followed by closed().await, so the same wait was there one frame deeper — but it undercuts the point of the change above, since a never-acked subscribe pins the task and the TrackWriter it carries until the session dies.

Bounded two ways, because they cover different failures:

• lease.released() — nobody downstream is waiting for this track any more. This is the condition that actually matters and needs no arbitrary constant. remote.rs already raced its handshake against connection cancellation; the local path was the odd one out.
• --subscribe-timeout (default 10s) — a subscriber is still waiting, but the upstream never answers. Applied symmetrically to both the local pull-through path and the cross-relay path.

Ten seconds is a policy choice rather than a protocol constant, since MoQ does not bound SUBSCRIBE_OK. A downstream subscriber is blocked for the whole window, and upstream is normally a nearby relay or origin where a control round trip is well under a second.

Either mechanism drops the in-flight future, which drops the Subscribe, whose Drop sends UNSUBSCRIBE — so giving up mid-handshake leaves nothing dangling upstream.

4. Stream namespace paths into the formatter

Display for TrackNamespace called to_utf8_path() internally and built a String, so it was no cheaper than calling to_utf8_path() directly — and the %namespace.to_utf8_path() pattern at tracing call sites allocated on every call, including at log levels that were not enabled. Display now streams into the formatter and to_utf8_path() delegates to it, so there is one implementation of the path format. Tracing call sites become %namespace, which is both lazy and allocation-free. Callers that genuinely want an owned String (the moq-pub catalog field, the file coordinator, coordinator tests) keep using to_utf8_path().

Not backported

The lock-poisoning and broadcast-lag metrics from #196 do not apply: the lease registry and broadcast resync paths they instrument were added after draft-14 and have no equivalent here. The one piece that does apply — the missing describe_counter! for moq_relay_cache_idle_evictions_total, which this branch emits — is folded into part 2 so the counter ships with Prometheus HELP text.

Configuration

Flag	Default	Meaning
--cache-idle-timeout	30 (seconds)	How long a cached track with no downstream subscribers is retained before its upstream subscription is released. 0 disables eviction, restoring the previous behaviour.
--subscribe-timeout	10 (seconds)	How long to wait for an upstream to acknowledge a SUBSCRIBE. Must be at least 1 — an unbounded wait on a peer is what the timeout exists to prevent, so there is deliberately no "disabled" setting. Rejected by clap, and again by Relay::new_with_tuning for embedders that bypass the CLI.

New metrics: moq_relay_cache_idle_evictions_total and moq_relay_subscribe_timeouts_total, both labelled by source (local, remote) so the two cache layers can be told apart.

Testing

cargo fmt --check, cargo clippy --no-deps --all-targets, cargo test (180 passing), cargo machete — all clean.

New coverage: subgroup FIN-vs-RESET termination and reset-code mapping; the draft-14 §13.1.8 code values as a registry test; interest counting, grace-period restart and generation identity; idle eviction, warm-cache reuse within the grace period, cancelled-requester safety, zero-timeout opt-out, and stale-guard isolation on both the local and remote paths; Display/to_utf8_path/Debug equivalence including the non-UTF-8 lossy case; the documented tuning defaults, that each timeout builder sets only its own knob (both are Duration, so crossing them would compile silently), that a zero subscribe timeout is rejected, and that a zero cache idle timeout still is not.

Not covered by a new test: timeout expiry itself is tokio::time::timeout over an existing call, and the drop-sends-UNSUBSCRIBE behaviour it relies on is Subscribe's existing tested Drop. A call-site test would need a full session mock for no additional assurance.

Compatibility

This is a source-breaking change to moq-transport — note that this differs from #196, which broke moq-relay-ietf instead, for the reason given in part 2 above.

Method	Before	After
TracksReader::subscribe	Option<TrackReader>	Option<(TrackReader, Option<TrackInterestGuard>)>
TracksRequest::next	Option<TrackWriter>	Option<TrackRequest>

TrackRequest implements Deref<Target = TrackWriter>, so .name, .namespace and .info still work unchanged on the yielded value; only code that needs to move the writer has to reach for .writer.

Guard-less variants of the two methods above were not kept as an additive migration path. The design's correctness depends on the interest guard being created under the same lock as the idle check, so a method that returns a cached reader without a guard hands back an entry that registers no interest — it looks idle immediately and can be evicted while it is still being served. That is the bug class this PR fixes, so a compile error at each call site is preferable to an API that silently reintroduces it.

Preserved deliberately: Tracks struct literals, Tracks::new, Tracks::produce, TracksWriter::create / remove, get_track_reader, and TracksState: Default are all unchanged, and the idle timeout is opt-in through Tracks::produce_with_cache_idle_timeout.

moq-relay-ietf is additive. RelayConfig is untouched, so embedders constructing it as an exhaustive struct literal are unaffected — that is why tuning is passed separately rather than as config fields. Relay::new(config) keeps working on defaults. New public surface: RelayTuning (#[non_exhaustive], with Default and builders), Relay::new_with_tuning, DEFAULT_SUBSCRIBE_TIMEOUT, and RemoteManager::with_cache_idle_timeout / with_subscribe_timeout.

Relationship to #180

#180 is an independent fix for #191 on this same branch, and it converged on the same TrackRequest { writer, lease } shape and the same moq_transport::serve placement — useful corroboration that the placement is forced by draft-14's structure rather than a matter of taste. Thanks @thexeos.

This PR additionally covers the cross-relay RemoteManager cache, the subgroup FIN/RESET issue, and the handshake bounding described above, and keeps the backport aligned with what landed on main in #196.