Skip to content

test(ffi): raw_audio_decode_delay reads every frame it wrote however late - #8566

Merged
kixelated merged 4 commits into
mainfrom
quest/m0/test-flakes-2/ffi-raw-audio-decode-delay
Nov 12, 2026
Merged

kixelated merged 4 commits into
mainfrom
quest/m0/test-flakes-2/ffi-raw-audio-decode-delay

Conversation

@kixelated

@kixelated kixelated commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

Finishes quest/m0/test-flakes-2/ffi-raw-audio-decode-delay.md: moq-ffi's test::raw_audio_decode_delay (feature audio) is re-enabled and its sh/gh/test-inventory.allow entry removed. Quarantined by moq-dev/moq#8558 after run 43198904021.

Cause

The same mechanism moq-dev/moq#8050 found in the Python test. The fixed consumer asked for max: 0, widened by the 80 ms Opus warmup. When a read reached the track more than 80 ms after group 1 arrived (wall clock), group 0 was stale and skipped, group 1 became a mid-stream join, and the warmup trimmed its 20 ms. The test never finishes the track before reading, so the read waited until the 10 s timeout, which KIO_DENY_HOST_TIMERS=1 turns into the SIGABRT seen in CI.

Reproduction

A blocking std::thread::sleep(PAUSE_MS) between the writes and the reads (not committed):

PAUSE before after
0 pass pass
200 ms timed out at the first read (test.rs:927) after 10 s pass
2 s not run pass

Fix

Both consumers pass MoqDelay { min, max: None }: no subscriber limit, so the publisher's max age bounds it. The fixed consumer keeps min = 80 ms; the estimated one keeps an unstated min, which plays as zero as before.

Choices made without asking

  • Fixed the test, not the consumer (the quest's open question). Skipping a stale group at max delay zero and trimming warmup after a mid-stream join are correct for a live player; holding the newest frame would undo the max delay a caller asked for. Same conclusion as fix(py): the audio playout-delay test reads every frame it wrote #8050.
  • No mocked clock. After the fix no timer decides the outcome: the only wall-clock dependency was the staleness check, which max: None removes, so the 2 s pause passes. The remaining timeout is only a hang guard.

Caused by

  • moq-dev/moq#7311 - wrote the test with max: Some(Duration::ZERO), which makes any read later than the warmup stall.

Impact

Public API: none. Wire: none. Test only.

Validation

  • just check: passed.
  • Pause reproduction above, before and after.

Follow-ups

None.

Models

Size: S
Implementer: Claude Code, Opus 5.5, low effort

(Written by Opus 5.5 with low)

Review

Rewritten by quest review after each round.

  • Reviewer: Codex, gpt-6.1-sol, medium effort
  • Session: 01a1d06a-308c-7880-be28-e97a21549567, round 1
  • Head: 1397741
  • Base: origin/main at 997ae11
  • Score: 9/10, The change removes stale-group skipping from both test consumers without changing production behavior; source review found no actionable defects, and builds and tests were not run as instructed.
  • Findings: none

kixelated and others added 2 commits November 12, 2026 07:33
…late

A max delay of zero made a late read find group 0 stale, and the mid-join
warmup trimmed the remaining 20 ms, so the read never returned. Both
consumers now ask for no max delay. Re-enables the test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title moq-ffi's raw audio decode delay test holds up test(ffi): raw_audio_decode_delay reads every frame it wrote however late Nov 12, 2026
@kixelated
kixelated marked this pull request as ready for review November 12, 2026 15:40
@coderabbitai

coderabbitai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The raw_audio_decode_delay test is no longer ignored. Both subscriptions set MoqDelay.max to None. The test inventory entry and the documents describing the flaky test and its investigation steps are removed.

Merge Risk: Minimal · up to 9da88

This re-enables a previously quarantined FFI audio test and removes its quarantine records. It does not change shipped behavior, and no actionable merge-blocking risk remains.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the test (raw_audio_decode_delay), the crate scope (ffi), and the behavior the fix establishes (reads every frame it wrote regardless of lateness). The changeset removes the test's ign…
Description check Passed The description states the purpose (re-enabling raw_audio_decode_delay and removing its inventory entry), gives the root cause, a reproduction table, the fix (MoqDelay max: None for both consumers), v…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the test (raw_audio_decode_delay), the crate scope (ffi), and the behavior the fix establishes (reads every frame it wrote regardless of lateness). The changeset removes the test's ignore, changes both consumers to use max: None, and removes the quarantine entries, so the title matches the main change.

Full details: Description check

Explanation

The description states the purpose (re-enabling raw_audio_decode_delay and removing its inventory entry), gives the root cause, a reproduction table, the fix (MoqDelay max: None for both consumers), validation, and impact. It matches the changeset files, which remove the quest doc, the allowlist entry, the README quarantine note, and update the test.

Finishing Touches

Simplify code

  • Commit to this branch
  • Create a new PR
  • 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

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 495ba8b2-3c20-45ad-9de0-1776c5e812d6
Commits

Reviewing files that changed from the base of the PR and between 1397741 and 9da8867.

Files selected for processing: 2

  • quest/m0/test-flakes-2/README.md
  • sh/gh/test-inventory.allow
Files with no reviewable changes: 2

  • quest/m0/test-flakes-2/README.md
  • sh/gh/test-inventory.allow

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No code issues — two documentation loose ends.

Reviewed changes

  • Re-enabled the FFI test: test::raw_audio_decode_delay loses its #[ignore], its quest child is deleted, and its sh/gh/test-inventory.allow entry goes with it (correct — the test now has a CI caller under just rs test -p moq-ffi, whose default features include audio).
  • Removed the subscriber max delay: both consumers in the test's subscribe closure now pass MoqDelay { min, max: None }, so a read that lands after group 1 is bounded by the track's max age instead of skipping group 0 and letting the mid-join warmup trim the only audio. max: None resolves to the publisher's hang::container::MAX_AGE (30 s) in both track::Delay::budget and moq_mux's consumer, comfortably past the test's 10 s hang guard; min: None on the estimated consumer is inert (Consumer::new's min_delay reads it as zero, and nothing else reads a subscriber min to pick a start). The flake's mechanism and the fix match #8050.
  • Verified locally: the test passes, and 40 parallel runs of the built binary are clean.

ℹ️ The epic's flake record still says the test is quarantined

quest/m0/test-flakes-2/README.md:275 still reads "It is quarantined with #[ignore] until its child fixes it", but this PR removes the #[ignore] and deletes the child, so the record is now false. The surrounding entries record resolutions in place — the moq-ffi warmup entry ends "moved it to the paused test clock, re-enabled it, and deleted its child" — so this paragraph should be updated the same way.

Technical details
# Update the epic's flake record for the re-enabled test

## Affected sites
- `quest/m0/test-flakes-2/README.md:275-279` — the "Added 2026-11-12" paragraph for
  `moq-ffi`'s `raw_audio_decode_delay` still says it is quarantined with `#[ignore]`
  until its child fixes it.

## Required outcome
- The paragraph no longer claims the test is quarantined, and records the resolution
  (no max delay in the test, `#[ignore]` removed, child deleted) with this PR linked,
  matching how the sibling entries read (e.g. the warmup entry at `:216-221`).

## Context
- The quarantine commit `e8f60f1` added this paragraph, so the fix should mirror it.
- No `## Required` list in the epic README names the child, so this is the only
  inbound reference left after the file deletion.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Comment thread sh/gh/test-inventory.allow Resolved
…wlist

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

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new code issues — one documentation loose end from the previous review still stands.

Reviewed changes

  • Dropped the empty allowlist header: sh/gh/test-inventory.allow no longer ends with the # Flaked under load; disabled until fixed at its cause. comment, which introduced nothing once its sole entry was removed. allowlist() in sh/gh/test-inventory.py skips blank and # lines, so the inventory check parses the file exactly as before.
  • No other delta: the remaining ranges in the range-diff are base-branch drift (new moq-video entries landing on main); the branch itself only contains the header deletion on top of the round-1 commit.

ℹ️ The epic's flake record still says the test is quarantined

quest/m0/test-flakes-2/README.md:275-279 still reads "It is quarantined with #[ignore] until its child fixes it", but this PR removes the #[ignore] and deletes the child, so the record is now false. The sibling entries record resolutions in place (the moq-ffi warmup entry ends "moved it to the paused test clock, re-enabled it, and deleted its child"), so this paragraph should be updated the same way.

Technical details
# Update the epic's flake record for the re-enabled test

## Affected sites
- `quest/m0/test-flakes-2/README.md:275-279` — the "Added 2026-11-12" paragraph for
  `moq-ffi`'s `raw_audio_decode_delay` still says it is quarantined with `#[ignore]`
  until its child fixes it.

## Required outcome
- The paragraph no longer claims the test is quarantined, and records the resolution
  (no max delay in the test, `#[ignore]` removed, child deleted) with this PR linked,
  matching how the sibling entries read (e.g. the warmup entry at `:216-221`).

## Context
- The quarantine commit `e8f60f1` added this paragraph, so the fix should mirror it.
- No `## Required` list in the epic README names the child, so this is the only
  inbound reference left after the file deletion.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

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

Copy link
Copy Markdown
Collaborator Author

Pullfrog's note on the epic README is fixed in 9da8867: the record that called raw_audio_decode_delay quarantined is gone, since this PR re-enables it.

(Written by Claude Opus 5.5)

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Removed the epic's quarantine record: quest/m0/test-flakes-2/README.md no longer carries the "Added 2026-11-12" paragraph saying moq-ffi's raw_audio_decode_delay is quarantined with #[ignore] until its child fixes it. That was the last false claim the earlier reviews flagged, and the file again ends with the standard Public API: none. Wire: none. footer.
  • No other delta: the range-diff against 8ddcb78 is that six-line deletion alone; the test fix and the sh/gh/test-inventory.allow cleanup are unchanged. No quest doc references the flake any more, and the Quest check passes.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@kixelated
kixelated added this pull request to the merge queue Nov 12, 2026
Merged via the queue into main with commit 9bee10f Nov 12, 2026
6 checks passed
@kixelated
kixelated deleted the quest/m0/test-flakes-2/ffi-raw-audio-decode-delay branch November 12, 2026 16:46
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.

1 participant