Skip to content

fix(ffi): test shutdown delivery with explicit Lite 07 - #5558

Merged
kixelated merged 2 commits into
mainfrom
quest/m1/test-flakes-2/ffi-shutdown
Oct 15, 2026
Merged

kixelated merged 2 commits into
mainfrom
quest/m1/test-flakes-2/ffi-shutdown

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem

The FFI shutdown fixtures required application delivery while negotiating default Lite 06, which drains to transport ACKs. Selecting Lite 07 alone still failed under load because TRACK_INFO satisfies demand.used() before SUBSCRIBE starts.

Approach

Select moq-lite-07-wip explicitly on both peers and receive a priming frame before finishing a distinct final group and immediately shutting down. Keep the final payload, clean end, one-second DeliveryTimeout, and both publish directions. Bound previously unbounded stages with the existing timeout. Add a deterministic TRACK_INFO-only demand proof and clarify the shutdown contract across bindings.

Impact

  • Public signatures and wire behavior are unchanged. Lite 07 waits for subscriber tail completion; released Lite drafts retain transport-ACK drain. Native IETF already drains to transport ACKs; JavaScript IETF currently withdraws announcements only.
  • The Rustdoc correction changes UniFFI’s generated shutdown checksum from 64390 to 1301; regenerate matching bindings. The checked-in Dart snapshot is regenerated.
  • No production ownership or shutdown logic changes. No retries, sleeps, or widened grace/timeouts.

Validation

The original fixture failed under 24 busy workers; explicit Lite 07 alone also failed. After the readiness correction, all 30 loaded iterations pass (60/60 tests), alongside all 5 targeted shutdown tests and the 4 deterministic readiness/FIN/legacy boundary tests.

All 2,720 focused net/Tokio/FFI tests pass (nine skipped). The full scoped check passed JS/WASM and 1,119 native tests before stopping at the unchanged CLI same-instance failure addressed by draft #5545. Native lint/rustdoc, Tokio feature matrix, 840 runtime-free tests, browser/WASM, media boundaries, Python/Kotlin/Go and the remaining C++/OBS/root checks pass. Swift is skipped on Linux without its toolchain. The initial CI command found a stale Dart snapshot; after regeneration, Dart analysis, all 19 tests and package checks pass. just test interop --all passes its complete matrix, denials, browser refusal close code, CLI FETCH and finite-tail assertions. Quest graph and whitespace checks pass.

Alternatives

Preserve released protocol behavior and test the approved explicit Lite 07 contract rather than introduce a new acknowledgement protocol.

Follow-ups

The explicit Tokio test-util dev-dependency is identical to #5549; retain it once in either landing order. Subscriber completion follows the existing staleness policy; this change does not alter missing-data grace.

(Written by GPT-6.1)

kixelated and others added 2 commits October 14, 2026 16:13
Maintainer approved application delivery only for explicit lite-07-wip; retain older Lite transport-ACK behavior and correct tests/docs without a wire change.

Co-Authored-By: GPT-6.1 <noreply@openai.com>
Co-Authored-By: GPT-6.1 <noreply@openai.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Implemented the maintainer's approved explicit Lite 07 shutdown scope. Released Lite behavior and all production shutdown/ownership logic remain unchanged; public signatures and wire bytes are unchanged.

The load reproduction showed that selecting Lite 07 alone was insufficient: TRACK_INFO interest could satisfy demand.used() before SUBSCRIBE. The deterministic query-only regression now proves that boundary, and both FFI fixtures receive a priming frame before starting their shutdown scenario. The distinct final tail, clean end, subscriber-FIN checks, one-second DeliveryTimeout and original timeouts remain required. The corrected fixtures passed all 30 loaded iterations (60 tests), plus the targeted and deterministic boundary tests.

All 2,720 focused net/Tokio/FFI tests pass (nine skipped). The full scoped check passed JS/WASM and 1,119 native tests before stopping at the unchanged CLI same-instance failure addressed by draft #5545. Native lint/rustdoc, Tokio feature matrix, 840 runtime-free tests, browser/WASM, media boundaries, Python/Kotlin/Go and the remaining C++/OBS/root checks pass. Swift is skipped on Linux without its toolchain. The initial CI command found a stale Dart snapshot; after regeneration, Dart analysis, all 19 tests and package checks pass. just test interop --all passes its complete matrix, denials, browser refusal close code, CLI FETCH and finite-tail assertions. Quest graph and whitespace checks pass.

The Tokio test-util dev-dependency matches #5549 exactly and should remain once in either landing order. This PR stays draft.

(Written by GPT-6.1)

@coderabbitai

coderabbitai Bot commented Oct 15, 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 23 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: 0d04368e-d5d5-4d78-8df4-a6c31891373b
📥 Commits

Reviewing files that changed from the base of the PR and between 53fe8ad and 5fe4520.

📒 Files selected for processing: 26
  • cpp/obs/src/moq-output.cpp
  • dart/moq/lib/src/client.dart
  • dart/moq_ffi/lib/src/moq.dart
  • doc/bin/obs.md
  • doc/lib/cpp/index.md
  • doc/lib/dart/index.md
  • doc/lib/go/index.md
  • doc/lib/js/net.md
  • doc/lib/kt/index.md
  • doc/lib/py/index.md
  • doc/lib/rs/moq-net.md
  • doc/lib/swift/index.md
  • go/wrapper/session.go
  • js/net/src/connection/established.ts
  • kt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/Moq.kt
  • py/moq-rs/README.md
  • py/moq-rs/moq/session.py
  • quest/m1/test-flakes-2/README.md
  • quest/m1/test-flakes-2/ffi-shutdown.md
  • rs/moq-ffi/Cargo.toml
  • rs/moq-ffi/src/session.rs
  • rs/moq-ffi/src/test.rs
  • rs/moq-net/src/session.rs
  • rs/moq-net/tests/session_close.rs
  • rs/moq-tokio/src/connection.rs
  • swift/Sources/Moq/Client.swift
  • 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 15, 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-15T15:28:27.719601Z 5fe4520 Draft marked ready
ℹ️ 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.

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: 5fe4520.

No actionable bugs found. Explicit Lite 07 plus receipt of a separate priming group is a sound readiness check: TRACK_INFO demand alone cannot prove SUBSCRIBE is serving. The tests still finish a distinct final group immediately before shutdown, assert its payload and clean end in both directions, and retain the one-second DeliveryTimeout contract. This fixes the fixture without broadening production shutdown behavior or hiding it behind retries.

The binding documentation now distinguishes transport ACKs from Lite 07 subscriber-tail completion and JavaScript IETF withdrawal-only close. The Dart checksum update is included; the other binding build paths regenerate their bindings.

Verification: static GitHub review of changes, shutdown/readiness implementation, and binding generation paths. No tests, load runs, or binding regeneration executed; the numeric generated checksum and reported test results were not independently reproduced.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary: Codex reviewed 5fe4520 with no findings and CI is green. No changes were needed; the branch merges cleanly with main, and the merge queue re-tests it against the latest trunk.

Decisions carried in this PR: the shutdown fixtures negotiate moq-lite-07-wip explicitly and prime the subscription with a frame, since TRACK_INFO satisfies demand before SUBSCRIBE. Released Lite drafts keep transport-ACK drain. The tokio test-util dev-dependency added here is the single declaration that #5560, #5559 and #5562 will reuse.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Oct 15, 2026
Merged via the queue into main with commit bed9f43 Oct 15, 2026
30 checks passed
@kixelated
kixelated deleted the quest/m1/test-flakes-2/ffi-shutdown branch October 15, 2026 16:12
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