Repository navigation
fix(ffi)!: request draining shutdown synchronously - #6436
Conversation
Quest-Claim: 218c559c5bf0b025
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
kixelated
changed the title
|
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 27 seconds. 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 |
|
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 14 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. |
|
Completed at 95d9a36 and marked ready. Final CI passed (375 Rust/C tests, 104 Python tests, binding and documentation checks), followed sequentially by full cross-language interop and the real OBS runtime suite. Swift validation was explicitly skipped because this host has no Swift toolchain. The final OBS check confirms a stopped, immediately destroyed output still delivers its queued finished tail. Synchronous owned shutdown and awaited (Written by GPT-6) |
# Conflicts: # cpp/moq/MIGRATE.md # dart/moq/MIGRATE.md # dart/moq_ffi/MIGRATE.md # go/ffi/MIGRATE.md # go/wrapper/MIGRATE.md # kt/moq-ffi/MIGRATE.md # kt/moq/MIGRATE.md # py/moq-ffi/MIGRATE.md # py/moq-rs/MIGRATE.md # rs/moq-c/MIGRATE.md # rs/moq-ffi/MIGRATE.md # swift/MIGRATE.md # swift/ffi/MIGRATE.md
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e62e4ccab3
ℹ️ 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".
…-sync # Conflicts: # cpp/moq/MIGRATE.md # dart/moq/MIGRATE.md # dart/moq_ffi/MIGRATE.md # dart/moq_ffi/lib/src/moq.dart # doc/lib/dart/index.md # doc/lib/go/index.md # doc/lib/kt/index.md # go/ffi/MIGRATE.md # go/wrapper/MIGRATE.md # kt/moq-ffi/MIGRATE.md # kt/moq/MIGRATE.md # py/moq-ffi/MIGRATE.md # py/moq-rs/MIGRATE.md # quest/m1/README.md # rs/moq-c/MIGRATE.md # rs/moq-ffi/MIGRATE.md # swift/MIGRATE.md # swift/ffi/MIGRATE.md
moq_ffi_shutdown() drops every runtime task, so an owned drain still running when OBS unloads the module on exit was cancelled. Keep each retired session's closed() waiter and let the destructor wait them out. Note in the Kotlin migration that moq.shutdown() no longer releases the client. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged main (resolved the MIGRATE, binding-doc, and quest README conflicts; regenerated the Dart bindings) and addressed both Codex findings:
Checks: (Written by Claude Opus 5.5) |
# Conflicts: # cpp/moq/MIGRATE.md # dart/moq/MIGRATE.md # dart/moq_ffi/MIGRATE.md # dart/moq_ffi/lib/src/moq.dart # doc/lib/c/index.md # go/ffi/MIGRATE.md # go/wrapper/MIGRATE.md # kt/moq-ffi/MIGRATE.md # kt/moq/MIGRATE.md # py/moq-ffi/MIGRATE.md # py/moq-rs/MIGRATE.md # py/moq-rs/moq/session.py # quest/m1/README.md # quest/m1/ffi-shutdown-sync.md # rs/moq-c/MIGRATE.md # rs/moq-c/src/test.rs # rs/moq-ffi/MIGRATE.md # rs/moq-ffi/src/session.rs # swift/MIGRATE.md # swift/Sources/Moq/Client.swift # swift/ffi/MIGRATE.md
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull 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".
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".
| public func shutdown() { | ||
| ffi.shutdown() |
There was a problem hiding this comment.
For users upgrading through swift/MIGRATE.md, the unreleased instructions still say that Session.shutdown() is async throws and drains before returning (line 194), which this change makes false. Following that guidance no longer waits for delivery or observes a drain failure, and exiting immediately afterward can discard queued tails. Update that existing paragraph to require shutdown() followed by try await closed(); the API-audit quest also incorrectly claims these migration entries are already synchronized. The repository explicitly requires outdated documentation to be fixed inline.
AGENTS.md reference: AGENTS.md:L32-L32
Useful? React with 👍 / 👎.
…grate Keeps this PR's deletion of quest/m1/api-audit/migrate.md and main's deletion of quest/m1/ffi-shutdown-sync.md (finished by #6436). Main's #6436 made binding shutdown a synchronous drain request again, so the shutdown entries now describe the net change from release's cancel(0): shutdown() drains, closed() reports the result, cancel(0) closes at once. Also drops the js/publish linger entry #6522 made moot and names the abort error #6482 left behind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
An unpolled or dropped
shutdown()future could cancel a session instead of draining its finished tracks. OBS compensated by retaining futures. Accepted C sessions also aborted after a local close request despite the documented drain contract.Approach
Register one synchronous, owned drain on the FFI runtime, retaining the native session and origins.
closed()shares its completion and delivery error across waiters. Serialize publication with existing waiters so stopping the client loop cannot hide the drain result. Update every binding, wrapper, and affected example; OBS no longer needs a retained future to keep the drain alive; its destructor still waits on each retired session'sclosed(), because module unload stops the runtime and would cancel a drain still in flight.Impact
shutdown()returns synchronously without a result; call it, then awaitclosed(). Python, Swift, Kotlin, Go, Dart, and C++ follow that shape. Convenience close/context-manager paths still await the drain. Kotlin'smoq.shutdown()no longer releases the client; close theMoqafterclosed().closed()reports a requested drain's result, including delivery timeout, for client and accepted sessions. Dropping a waiter or disposing the session does not abort the owned drain.MoqErrorgainsClonefor stable repeated completion results.moq_session_close()now drains and reports failures through its terminal callback.Migration
Each package's note, for its
MIGRATE.mdat the breaking cut. Upgrading from the release line's additive asyncdrain()backport: replace the awaiteddrain()withshutdown()followed by awaitingclosed();mainhas no separatedrain()method.rs/moq-ffi:MoqSession::shutdown()requests the drain synchronously. Useshutdown(); closed().await?to wait and report delivery failures, orcancel(0)for immediate cancellation.py/moq-ffi,swift/ffi,kt/moq-ffi,go/ffi,dart/moq_ffi:shutdown()requests a drain synchronously; awaitingclosed()(Closed(ctx)in Go, suspendingclosed()in Kotlin) waits for its result and reports delivery failures. Regenerate bindings and link the matching native library together.py/moq-rs:session.shutdown()requests the drain synchronously. Usesession.shutdown(); await session.closed()to wait and raise delivery failures, orsession.cancel(0)for immediate cancellation. A cleanasync withexit now waits for the drain.swift:session.shutdown()is synchronous and requests a drain. Follow it withtry await session.closed()to wait and report delivery failures; usesession.cancel(code: 0)for immediate cancellation. The 0.4 section's "Session.shutdown()isasync throwsand drains before returning" becomes "Session.shutdown()requests a drain synchronously;try await session.closed()waits for its result."kt/moq:session.shutdown()andmoq.shutdown()request a drain synchronously. Awaitsession.closed()(ormoq.session.closed()) to wait and report delivery failures.moq.shutdown()no longer releases the client, so close theMoq(or leave itsuse { }) afterclosed()returns;Moq.close()still cancels immediately.go/wrapper:session.Shutdown()requests a drain synchronously with no context or error result. Callsession.Closed(ctx)to wait and report delivery failures, orsession.Cancel(0)for immediate cancellation.Client.Close()now waits for the drain and returns an error.dart/moq:session.shutdown()requests a drain synchronously;await session.closed()waits for its result.Moq.close()returnsFuture<void>and drains before stopping the client, so await it. Usesession.cancel(code: 0)for immediate cancellation.cpp/moq:session->shutdown()returnsvoidand requests the drain synchronously. Usesession->closed().get()to wait and report delivery failures, orsession->cancel(0)to cancel immediately. A shutdown request survives session disposal and discarded waiters.rs/moq-c:moq_session_close()keeps its synchronous request/terminal-callback ABI; no function is renamed. Accepted sessions now drain finished tracks too; a failed drain is reported by the terminalMOQ_CONNECTION_PHASE_CLOSEDstate snapshot. Finish or abort live published tracks before requesting close.Alternatives
Keeping an async method still requires its first poll. Adding
begin_shutdown()duplicates one action. Retaining futures in OBS only leaves every other caller exposed.Validation
nix develop --command just cipassed: 375 Rust/C tests, 104 Python tests, Kotlin JVM checks, Go race tests, Dart tests, C++ probes, OBS compilation, and JS/docs/lints. Swift's check explicitly skipped because this host has no Swift toolchain.nix develop --command just test interop --allpassed, including the full language matrix, FETCH, and finite/compressed/dropped tails.nix develop --command bash sh/obs/unit.shpassed, including stopping and immediately destroying an output with a queued 2 MiB tail. CI, interop, and OBS builds ran sequentially against the final source.just checkpassed;just obs test(TSan) passed every MoQOutput test, then stalled in the unrelatedmoq-source-teston a loaded host and was stopped;just test interop --allpassed 171 of 172 cases; the one failure (Rust publisher to js-native-node, slow-group skip at load average 34) touches no FFI code and passed on rerun.MIGRATE.mdhunks per docs: write MIGRATE.md at the breaking cut, not per PR #6610 (notes moved to Migration above), adapting to the sessionstate()API from refactor: observe binding connection state coherently #6487, and regenerating the Dart bindings:just checkpassed;just test interop --allpassed 171 of 172; the one failure was the knownrust -> js-native-nodeaudio timestamp gap (quest).Follow-ups
The existing release backport quest remains separate. Its plan and the affected migrations now map the additive async
drain()backport toshutdown()followed by awaitedclosed()at the breaking cut. No new quests or open decisions.(Written by GPT-6; updated by Claude Opus 5.5)