Skip to content

fix(moq-mux): judge a source's clock on slack the floors saw - #5714

Merged
kixelated merged 2 commits into
mainfrom
quest/m1/ts-drift-startup
Oct 16, 2026
Merged

kixelated merged 2 commits into
mainfrom
quest/m1/ts-drift-startup

Conversation

@kixelated

@kixelated kixelated commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Completes quest/m1/ts-drift-startup.md. Closes #5564.

Summary

jitter::Buffer::steer read the slack now (gap) from the floors' upper envelope extrapolated to now. Once a move in latency stops, the hull edge it moved along runs on past the floors that hold after it. lost then kept growing although no frame lost or gained any slack, and a source on our clock failed with "the source's clock runs ... ppm off ours". The slack is now clamped to the range of floors seen in the window. Past that range the envelope is a guess at the rate, not observed slack. Steering, the startup gap and lost all read the clamped value. The rate estimate and the exit rule are unchanged.

Reproduction and evidence

  • I ran the issue's loop 11 times locally (just test ts --full, debug, loaded machine) with a temporary per-floor trace. None of the runs exited on drift. Two failed on an unrelated tstd "PID 257 B underflow". The traces still show the mechanism. All three tracks' floors move together by up to 0.44 s over about 12 s: one run built from +0.09 to -0.35 between 13 s and 25 s, then recovered. While a move like that spans the window's mean, the hull reads it as -44000 ppm. That run reached lost -0.406 against the -0.45 limit.
  • Every failure reported in flaky: just test ts --full exits on the release clock's first rate estimates (~1 in 5 local runs) #5564 exits on a positive rate (+32045, +9750, +21347.9 ppm). That is the draining direction. In mocked time, a single-track 0.3 s drain over 20 s from 3 s fails at 37.6 s on unfixed code. Its floors hold at 0.300 while the extrapolated at climbs from 0.30 to 0.499. A 0.4 s build over 5 s from 3 s also fails (-80000 ppm at 9.1 s), because the extrapolation runs ahead of the last floor.

Tests

  • jitter::start_up_latency_never_fails_one_track and jitter::start_up_latency_never_fails_a_full_multiplex cover a 0 ppm source through a 900 ms buffer. Its latency builds or drains by 0.3 or 0.4 s, over 5, 10 or 20 s, starting at 0, 3 or 6 s, and then holds. The multiplex variant is shaped like --full: video sent 0.35 to 0.65 s ahead and wandering, plus two audio tracks sent as they decode. No case fails or drops a frame. Both tests fail without the fix.
  • feed now takes a Shape (delay and tracks). Existing callers pass PAIR, which is the old 500 ms buffer with two tracks.
  • All existing jitter tests pass, including a_source_far_off_real_time_fails_within_seconds, a_source_past_the_drift_limit_fails, and the day-long drift test.
  • just check passes. I re-ran just test ts --full 11 times; 0 runs exited on drift, but that is also true on main locally, so this does not prove the CI flake is gone.

Public API / wire impact

  • Public API: none. jitter::Buffer is pub(crate).
  • Wire: none.

Decisions

  1. The quest's 500 ms regressions. The plan asked for builds and drains of 300 to 500 ms at --full's 900 ms delay. These tests cover 300 and 400 ms. A move past half the delay (450 ms) after the rate's first measurement is a real change in observed slack. If the floors so far trace it as a line (a build starting at the first floor, or a drain after a flat start, whose flat floors the upper hull treats as queueing), it is identical to a source that far off at the moment it crosses the limit. Exiting later would need a minimum span or sample count, or a wider limit, and the plan rules out all three. I prototyped "exit only when every hull edge in the window reads past 30 ppm on the same side". It saves a 0.5 s build after a flat start, but not a 0.5 s drain after a flat start or a line from the first floor, so I left it out.
    • Decided by the maintainer: accept regression coverage of 300 to 400 ms moves as the tested bound, rather than the plan's 500 ms. "A move under half the delay never fails" is the boundary documented on Buffer.
  2. The out_of_tolerance count still counts the point estimate. Early in a run it reads thousands of ppm from latency wander. The sibling fix(ts): start an export on a whole group and count every drop #5713 saw -100 to -250 ppm and a climbing count in default runs, which matches this misfit. Left as is here.

Suggested follow-ups

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 2 commits October 16, 2026 08:26
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The release clock read the slack now from the floors' upper envelope
extrapolated to the present. Once a move in latency stops, the edge it
moved along runs on past the floors that hold after it, so `lost` grew
with no frame losing or gaining slack and a source on our clock failed
with "the source's clock runs ... ppm off ours". The slack is now kept
within the floors seen.

Closes #5564.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Outcome: implemented as a draft. I reproduced the mechanism in mocked time (the extrapolated envelope charges the rate for slack no floor showed) and fixed it by keeping the slack within the floors seen. just check passes. One open decision is for the maintainer: these regressions cover moves of 300 to 400 ms, not the plan's 500 ms. A move past half the delay that the floors trace as a line cannot be told apart from an out-of-tolerance source without a minimum span or a wider limit, and the plan rules out both. My recommendation is to accept "a move under half the delay never fails" as the boundary.

(Written by Claude Opus 5.5)

@kixelated
kixelated marked this pull request as ready for review October 16, 2026 16:21
@kixelated

Copy link
Copy Markdown
Collaborator Author

@codex review

(Written by Claude Opus 5.5)

@coderabbitai

coderabbitai Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 54 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9ef93958-f725-44d8-b0e4-adeb714ad8c3
📥 Commits

Reviewing files that changed from the base of the PR and between b4eab0d and d33985d.

📒 Files selected for processing: 3
  • quest/m1/README.md
  • quest/m1/ts-drift-startup.md
  • rs/moq-mux/src/jitter.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T16:23:13.291531Z d33985d Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: d33985d2da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merging. Summary:

  • jitter::Buffer::steer now clamps the slack-now estimate to the range of floors in the rate window, so the upper envelope's extrapolation past the last floor no longer charges the source rate for slack no frame had. This fixes the "TS full multiplex at its own rate" ppm exits (flaky: just test ts --full exits on the release clock's first rate estimates (~1 in 5 local runs) #5564). The rate estimate and exit rule are unchanged.
  • Regression tests start_up_latency_never_fails_one_track and start_up_latency_never_fails_a_full_multiplex fail without the fix.
  • Decision (maintainer): accept regression coverage of 300 to 400 ms latency moves as the tested bound rather than the plan's 500 ms; "a move under half the delay never fails" is the documented boundary on Buffer.
  • No public API or wire change. CI is green and Codex reviewed d33985d with no findings.

(Written by Claude Opus 5.5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky: just test ts --full exits on the release clock's first rate estimates (~1 in 5 local runs)

1 participant