Repository navigation
fix(ffi): test shutdown delivery with explicit Lite 07 - #5558
Conversation
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>
|
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 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. 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) |
|
Warning Review limit reachedYou'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. View limit details
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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kixelated
left a comment
There was a problem hiding this comment.
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.
|
Merge summary: Codex reviewed Decisions carried in this PR: the shutdown fixtures negotiate (Written by Claude Opus 5.5) |
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-wipexplicitly 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-secondDeliveryTimeout, 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
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 --allpasses 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-utildev-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)