Fix FfiClient listener use-after-free during Room teardown - #171
Merged
alan-george-lk merged 4 commits intoJun 15, 2026
Merged
Conversation
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Track each listener in a ListenerSlot with active-callback draining so removeListener and shutdown block until in-flight FFI events finish before the owning object (e.g. Room) is destroyed. Add unit tests that reproduce the reported event-vs-destruction race. Co-authored-by: Cursor <cursoragent@cursor.com>
alan-george-lk
marked this pull request as ready for review
June 15, 2026 01:12
alan-george-lk
requested review from
stephen-derosa and
xianshijing-lk
as code owners
June 15, 2026 01:12
stephen-derosa
approved these changes
Jun 15, 2026
stephen-derosa
left a comment
Copy link
Copy Markdown
Collaborator
There was a problem hiding this comment.
nice this looks good -- thank you for driving this down!
testing is king ofc, and the unit tests seem pretty wholesome! Just given that we know this should addresses instability and that a customer will immediately use this, I would suggest a more thorough body of the PR and adding something about a Listener state machine (or something similar) to the commit.
Sorry, something went wrong.
Comment thread
src/ffi_client.cpp
Comment thread
src/ffi_client.cpp
Outdated
Comment thread
src/ffi_client.cpp
Comment thread
src/ffi_client.cpp
Comment thread
src/ffi_client.cpp
Outdated
Comment thread
src/ffi_client.cpp
Comment thread
src/tests/unit/test_ffi_client.cpp
Outdated
Comment thread
src/tests/unit/test_ffi_client.cpp
Outdated
Comment thread
src/ffi_client.cpp
Comment thread
src/ffi_client.cpp
alan-george-lk
added a commit
that referenced
this pull request
Jun 22, 2026
Fixes a use-after-free crash when a Room (or any listener owner) is destroyed while the FFI thread is still dispatching an event into its FfiClient listener. Observed both in flaky integration tests and user-reported. * Introduce ListenerSlot (additional state around Listener) to track in-flight listener callbacks; removeListener() and shutdown() block until active callbacks finish before teardown proceeds * Set a removed flag before draining so stale pushEvent snapshots do not start new callbacks after unregister * Replace the initialized_ bool with a LifecycleState enum and compare_exchange_strong for single-flight initialize() / shutdown(), and to drop events / reject new listeners during shutdown * Added 2 unit tests to verify fixes are resiliant
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a use-after-free crash when a Room (or any listener owner) is destroyed while the FFI thread is still dispatching an event into its FfiClient listener. Observed both in flaky integration tests and user-reported.