Skip to content

Commit 50f82a7

Browse files
kixelatedclaude
andcommitted
fix(json): roll a snapshot before a delta evicts its own base
`delta_allowed` gated only on the delta ratio, so with a nonzero ratio a large patch could be admitted into a group that no longer has room for it. The group cache evicts from the front, and frame 0 is the snapshot every delta applies to, so a ~4 MiB snapshot followed by a ~29 MiB patch dropped the base and left a late subscriber with `Lagged` instead of the current value. The budget is now part of the decision: a patch that would push the group past the cache rolls a fresh snapshot instead. Checked before the window advances, so falling back is clean, and measured on the plaintext, which bounds the compressed frame. Also preserves the fields belonging to a mode or compression this build does not recognize. The draft and docs claim such an entry round-trips verbatim, but only the discriminant survived: `{"mode":"windowed","window":10}` kept `windowed` and dropped `window`, so a relay that reparsed and republished emitted an entry nothing can act on, which is worse than dropping it outright. Both configs now carry a flattened `extra`, and the browser schemas are loose, matching what `Container` already does for an unknown container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 002fa69 commit 50f82a7

7 files changed

Lines changed: 69 additions & 2 deletions

File tree

js/hang/src/catalog/binary.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,11 @@ import { TimelineSchema } from "./timeline";
1010
* The entry says how to read the track, so a consumer needs the track name and nothing else about
1111
* the application. Prefer a {@link JsonConfig} track when the payloads are JSON, so a generic
1212
* consumer can read them.
13+
*
14+
* A *loose* object: fields this build doesn't recognize pass through untouched, so an entry using
15+
* a future mode or compression round-trips rather than losing the fields that describe it.
1316
*/
14-
export const BinaryConfigSchema = z.object({
17+
export const BinaryConfigSchema = z.looseObject({
1518
// Optional reference to another broadcast that publishes this track, expressed
1619
// relative to the broadcast that served this catalog (e.g. "./source").
1720
// If unset, the track lives in the same broadcast as the catalog.

js/hang/src/catalog/data.test.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,16 @@ test("the known modes and compressions are recognized", () => {
6767
expect(modeSupported("stream")).toBe(true);
6868
expect(compressionSupported("deflate")).toBe(true);
6969
});
70+
71+
test("fields belonging to an unrecognized mode survive a round trip", () => {
72+
// Preserving the mode string alone is not enough: a future mode comes with fields describing it,
73+
// and a relay that reparsed and republished would otherwise strip them, leaving an entry nothing
74+
// can act on. Same guarantee the container schema gives an unknown container.
75+
const parsed = RootSchema.parse({
76+
json: { tracks: { future: { mode: "windowed", window: 10 } } },
77+
binary: { tracks: { future: { mode: "windowed", chunk: "4k" } } },
78+
});
79+
80+
expect(parsed.json?.tracks.future).toMatchObject({ mode: "windowed", window: 10 });
81+
expect(parsed.binary?.tracks.future).toMatchObject({ mode: "windowed", chunk: "4k" });
82+
});

js/hang/src/catalog/json.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,11 @@ import { TimelineSchema } from "./timeline";
99
*
1010
* The entry says how to read the track, so a consumer needs the track name and nothing else about
1111
* the application.
12+
*
13+
* A *loose* object: fields this build doesn't recognize pass through untouched, so an entry using
14+
* a future mode or compression round-trips rather than losing the fields that describe it.
1215
*/
13-
export const JsonConfigSchema = z.object({
16+
export const JsonConfigSchema = z.looseObject({
1417
// Optional reference to another broadcast that publishes this track, expressed
1518
// relative to the broadcast that served this catalog (e.g. "./source").
1619
// If unset, the track lives in the same broadcast as the catalog.

rs/hang/src/catalog/binary.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,15 @@ pub struct BinaryConfig {
8282
/// The companion timeline track indexing this track's groups, if the publisher offers one.
8383
#[serde(default)]
8484
pub timeline: Option<Timeline>,
85+
86+
/// Fields this build doesn't recognize, kept so the entry round-trips.
87+
///
88+
/// A future [`Mode`] or [`Compression`] almost certainly comes with fields describing it, and
89+
/// preserving the discriminant without them would leave a relay republishing an entry nothing
90+
/// can act on. Same reason [`Container`](crate::catalog::Container) keeps an unknown container
91+
/// verbatim.
92+
#[serde(flatten)]
93+
pub extra: serde_json::Map<String, serde_json::Value>,
8594
}
8695

8796
impl BinaryConfig {
@@ -93,6 +102,7 @@ impl BinaryConfig {
93102
compression: None,
94103
mime: None,
95104
timeline: None,
105+
extra: Default::default(),
96106
}
97107
}
98108
}

rs/hang/src/catalog/json.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,15 @@ pub struct JsonConfig {
8080
/// The companion timeline track indexing this track's groups, if the publisher offers one.
8181
#[serde(default)]
8282
pub timeline: Option<Timeline>,
83+
84+
/// Fields this build doesn't recognize, kept so the entry round-trips.
85+
///
86+
/// A future [`Mode`] or [`Compression`] almost certainly comes with fields describing it, and
87+
/// preserving the discriminant without them would leave a relay republishing an entry nothing
88+
/// can act on. Same reason [`Container`](crate::catalog::Container) keeps an unknown container
89+
/// verbatim.
90+
#[serde(flatten)]
91+
pub extra: serde_json::Map<String, serde_json::Value>,
8392
}
8493

8594
impl JsonConfig {
@@ -91,6 +100,7 @@ impl JsonConfig {
91100
compression: None,
92101
schema: None,
93102
timeline: None,
103+
extra: Default::default(),
94104
}
95105
}
96106
}

rs/hang/src/catalog/root.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -507,6 +507,23 @@ mod test {
507507
assert_eq!(parsed, reparsed, "re-encoded catalog did not round-trip");
508508
}
509509

510+
/// Preserving the mode string alone is not enough: a future mode comes with fields describing
511+
/// it, and a relay that reparsed and republished would otherwise strip them, leaving an entry
512+
/// nothing can act on.
513+
#[test]
514+
fn unknown_mode_fields_round_trip() {
515+
let encoded = r#"{"json":{"tracks":{"future":{"mode":"windowed","window":10}}}}"#;
516+
517+
let parsed = Catalog::from_str(encoded).expect("failed to decode");
518+
let future = parsed.json.tracks.get("future").expect("missing track");
519+
assert_eq!(future.mode, Mode::Unknown("windowed".to_string()));
520+
assert_eq!(future.extra.get("window"), Some(&serde_json::json!(10)));
521+
522+
let output = parsed.to_json().expect("failed to encode");
523+
assert!(output.contains(r#""window":10"#), "unknown fields dropped: {output}");
524+
assert_eq!(Catalog::from_str(&output).expect("re-decode"), parsed);
525+
}
526+
510527
/// There is no safe default: reading a stream as a snapshot silently drops every record but the
511528
/// last, so an entry without a mode is malformed rather than assumed.
512529
#[test]

rs/moq-json/src/snapshot/encoder.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,17 @@ impl<T: Serialize> Encoder<T> {
287287
if self.config.compression && bytes.len() as u64 > moq_flate::DEFAULT_MAX_FRAME_SIZE {
288288
return Err(moq_flate::Error::TooLarge(moq_flate::DEFAULT_MAX_FRAME_SIZE).into());
289289
}
290+
// A delta is only readable while the group still holds the snapshot it applies to, and the
291+
// group cache evicts from the front. Admitting a patch that pushes the group past that budget
292+
// would drop frame 0, leaving a late subscriber with a base-less group (`Lagged`) instead of
293+
// the current value. Roll a fresh snapshot instead, which is cheap next to losing the value.
294+
//
295+
// Measured on the plaintext, which bounds the compressed frame, and checked here because
296+
// nothing has mutated yet: the window only advances on the line below.
297+
if self.snapshot_len + self.delta_bytes + bytes.len() as u64 > moq_net::group::MAX_GROUP_CACHE {
298+
return self.snapshot(value).map(Some);
299+
}
300+
290301
let payload = match self.flate.as_mut() {
291302
Some(flate) => flate.frame(&bytes),
292303
None => Bytes::from(bytes),

0 commit comments

Comments
 (0)