From 7ea12574b649158f09720b4e83e4ed53efa4a04b Mon Sep 17 00:00:00 2001 From: HangPlayerEngineer Date: Thu, 8 Oct 2026 18:16:58 +0000 Subject: [PATCH] fix(moq-net): accept END_OF_GROUP when the header also marks the group end The End-of-Group header bit and an explicit END_OF_GROUP object (status 0x03) are two independent ways to state where a group ends, and draft-20 makes neither conditional on the other. A publisher may legally use both: the bit says this stream carries the group to its end, the object says which object that end is at. Frame.decode accepted status 0x03 only on the branch where the header did NOT mark the end. The combination matched no branch and fell through to `Unsupported object status: 3`, which propagates out of handleGroup's read loop; the catch then closes the group with that error. So a publisher that stated the end twice -- correctly, both times -- lost the whole group, including the objects already delivered on that stream, not just the one redundant marker. Accept GROUP_END inside the hasEnd branch, returning the same payload-less Frame the other branch already returns: the subscriber breaks its read loop on `payload === undefined`. This stays distinct from the `status === 0` case, which is an empty frame rather than a group end; the two are different objects and are not merged. Tested at both levels, because they make different claims. The codec test pins both header shapes plus the status-0 case. The subscriber test pins that the group closes cleanly rather than merely stopping -- a torn-down group also stops producing frames, so asserting "nothing more arrives" would pass on the bug. Both go red with the fix reverted. --- js/net/src/ietf/ietf.test.ts | 37 ++++++++++++++++++++++++++++ js/net/src/ietf/object.ts | 13 ++++++++++ js/net/src/ietf/subscriber.test.ts | 39 ++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+) diff --git a/js/net/src/ietf/ietf.test.ts b/js/net/src/ietf/ietf.test.ts index d16c7b5bee2..f9b11be26be 100644 --- a/js/net/src/ietf/ietf.test.ts +++ b/js/net/src/ietf/ietf.test.ts @@ -1755,6 +1755,43 @@ test("Frame object time: draft-16 starts delta property types", async () => { expect(decoded.timestamp?.scale).toBe(Timescale.MILLI); }); +// draft-20 makes the End-of-Group header bit and an explicit END_OF_GROUP (status 0x03) +// object independent, so a publisher may legally send both. Accepting 0x03 only when the +// header did NOT mark the end left that combination matching no branch, so it threw and +// tore down the whole group stream over a redundant end marker. A group ends on a +// payload-less frame, which is what both branches return. +test("Frame: END_OF_GROUP is accepted whether or not the header marks the end", async () => { + const makeFlags = (hasEnd: boolean): GroupFlags => ({ + hasExtensions: false, + hasSubgroup: false, + hasSubgroupObject: false, + hasEnd, + hasPriority: true, + firstObject: true, + }); + + // idDelta 0, zero-length payload, then the END_OF_GROUP status. + const wire = new Uint8Array([0x00, 0x00, 0x03]); + + for (const hasEnd of [false, true]) { + const decoded = await new Reader(undefined, wire, Version.DRAFT_20).decode((c) => + Frame.decode(c, makeFlags(hasEnd), Timescale.MILLI), + ); + + // No payload is the group-end signal the subscriber breaks its read loop on. + expect(decoded.payload).toBeUndefined(); + expect(decoded.endOfTrack).toBe(false); + } + + // The hasEnd + status-0 case must keep meaning "empty frame", not "group end" -- the two + // are different objects and this fix must not merge them. + const empty = await new Reader(undefined, new Uint8Array([0x00, 0x00, 0x00]), Version.DRAFT_20).decode((c) => + Frame.decode(c, makeFlags(true), Timescale.MILLI), + ); + expect(empty.payload).toEqual(new Uint8Array(0)); + expect(empty.endOfTrack).toBe(false); +}); + test("Frame decodes objects split at every byte", async () => { const flags: GroupFlags = { hasExtensions: true, diff --git a/js/net/src/ietf/object.ts b/js/net/src/ietf/object.ts index 84ade416538..916b742bafc 100644 --- a/js/net/src/ietf/object.ts +++ b/js/net/src/ietf/object.ts @@ -371,6 +371,19 @@ export class Frame { if (flags.hasEnd) { // Empty frame if (status === 0) return new Frame({ payload: new Uint8Array(0), timestamp }); + + // A publisher may both set the End-of-Group header bit and terminate the subgroup + // with an explicit END_OF_GROUP object. Draft-20 makes neither conditional on the + // other, so the two are legal together. Without this branch that combination + // matches nothing below and throws, killing the whole group stream over a + // redundant -- not contradictory -- end marker. + // + // The payload-less Frame is what ends the group: the subscriber breaks its read + // loop on `frame.payload === undefined`, the same path the `else` branch below + // already takes for a header that does not mark the end. Note this stays distinct + // from the `status === 0` case above, which is an *empty* frame rather than a + // group end. + if (status === GROUP_END) return new Frame(); } else if (status === 0 || status === GROUP_END) { // TODO status === 0 should be an empty frame, but moq-rs seems to be sending it incorrectly on group end. return new Frame(); diff --git a/js/net/src/ietf/subscriber.test.ts b/js/net/src/ietf/subscriber.test.ts index e67b56d016a..8a24496728d 100644 --- a/js/net/src/ietf/subscriber.test.ts +++ b/js/net/src/ietf/subscriber.test.ts @@ -1681,3 +1681,42 @@ test("an object datagram is a datagram group", async () => { ).toBeInstanceOf(ProtocolViolation); track.close(); }); + +/** + * The End-of-Group header bit and the terminal END_OF_GROUP object are two independent ways + * to state where a group ends, and the draft lets a publisher use both: the bit says this + * stream carries the group to its end, the object says which object that end is at. + * + * Accepting the object only when the bit was clear left a publisher that stated the end + * twice -- correctly, both times -- with the whole group stream torn down by + * `Unsupported object status: 3`. The test is at the subscriber rather than the codec + * because the loss is a group, not a parse: `Frame.decode` throwing propagates out of + * `handleGroup`'s read loop and the catch closes the group with that error, so the objects + * that already arrived on the stream go with it. + */ +test("a group whose header declares the end may also send END_OF_GROUP", async () => { + const { subscriber, track } = await subscribeTrack(); + + // groupFlags already sets hasEnd, which is the case under test. One real object, then an + // END_OF_GROUP: delta 0, zero payload length, status 0x03. + await subscriber.handleGroup( + new GroupMessage({ + trackAlias: ALIAS, + groupId: 0, + subGroupId: 0, + publisherPriority: 0, + flags: groupFlags(true), + }), + new Reader(undefined, new Uint8Array([...encodeObjects([0]), 0, 0, 0x03]), VERSION), + ); + + const group = await track.ordered().nextGroup(); + expect((await group?.readFrame())?.payload).toEqual(new TextEncoder().encode("object 0")); + + // The group must close cleanly rather than merely stop producing frames: a torn-down + // group also stops producing frames, so asserting "nothing more arrives" would pass on + // the bug. + expect(await group?.readFrame()).toBeUndefined(); + expect(group?.closed.peek() ?? null).toBeNull(); + track.close(); +});