[lonexreb]

Description

RayClientBidiReactor::OnDone() and RayServerBidiReactor::OnDone() release the reactor's self-owning reference (self_ref_, introduced in #57641) from a handler posted to io_context_. gRPC can still invoke OnReadDone/OnWriteDone right before that, and the syncer can call DoDisconnect/OnCancel; each of those posts a handler capturing a raw this to the same io_context_. If the OnDone handler happens to run first, self_ref_.reset() destroys the reactor and the handler queued earlier then dereferences freed memory. That matches both ASAN reports in #62008: allocation in RaySyncer::Connect(), free at ray_syncer_client.cc:60 (self_ref_.reset()), use from a later io_context_ handler.

#57641 guarded only the batch timer (with a weak_ptr). This PR captures a strong copy of self_ref_ in every other dispatched handler (OnWriteDone, both dispatches in OnReadDone, the client's DoDisconnect, the server's DoDisconnect/OnCancel) so none of them can outlive the reactor. The two OnDone handlers also capture self, so the reactor stays alive through cleanup_cb_ and the last owning reference is dropped when that handler returns rather than in the middle of it. The batch timer keeps its weak_ptr so a pending timer does not extend the lifetime of a finished reactor. No behavior changes otherwise.

Related issues

Fixes #62008

@Kunchd asked for a PR on the issue; the reporter declined. The maintainer refactor in #58856 (making reactors shared_from_this) was closed by the stale bot in Dec 2025, and nothing open touches ray_syncer lifetime handling (gh pr list --state open --search "62008 in:body" is empty; recent ray_syncer PRs are about batching/leader election).

Additional information

I could not reproduce the race deterministically in a unit test (it needs gRPC to deliver a callback between OnDone and its posted handler), so this PR is justified by the ownership analysis above rather than a new test. The existing syncer tests pass; they set self_ref_ to a non-owning shared_ptr on stack reactors, which the added captures tolerate.

Test commands run locally (macOS arm64, bazel 7.5.0):

bazel test --dynamic_mode=off //src/ray/ray_syncer/tests:ray_syncer_test   # PASSED

AI assistance (Claude Code) was used to draft this change; every line was reviewed and tested by the submitter.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request prevents potential use-after-free issues in RaySyncerBidiReactor and its subclasses by capturing a strong reference (self = self_ref_) in lambdas dispatched to io_context_, ensuring the reactor remains alive during asynchronous callbacks. The reviewer recommends adding (void)self; inside these lambda bodies to avoid potential compiler warnings about unused lambda captures (e.g., -Wunused-lambda-capture in Clang).

[gemini-code-assist]

To prevent potential compiler warnings about unused lambda captures (such as -Wunused-lambda-capture in Clang), it is recommended to explicitly cast the captured self to void inside the lambda body. This also clearly documents that the capture is intended solely for lifetime management.

Suggested change

	void OnWriteDone(bool ok) override {
	io_context_.dispatch(
	[this, disconnected = IsDisconnected(), ok]() {
	[this, self = self_ref_, disconnected = IsDisconnected(), ok]() {
	if (*disconnected) {
	void OnWriteDone(bool ok) override {
	io_context_.dispatch(
	[this, self = self_ref_, disconnected = IsDisconnected(), ok]() {
	(void)self;
	if (*disconnected) {

[lonexreb]

Not adding (void)self;: clang's -Wunused-lambda-capture doesn't fire for captures whose type has a non-trivial destructor like std::shared_ptr, since holding the object alive is a legitimate use. The macOS build compiles with -Werror (.bazelrc) and bazel test //src/ray/ray_syncer/tests:ray_syncer_test built and passed cleanly with clang. The comment in ray_syncer_bidi_reactor.h documents that these captures exist for lifetime.

[gemini-code-assist]

To prevent potential compiler warnings about unused lambda captures (such as -Wunused-lambda-capture in Clang), it is recommended to explicitly cast the captured self to void inside the lambda body. This also clearly documents that the capture is intended solely for lifetime management.

  io_context_.dispatch(
      [this, self = self_ref_, status]() {
        (void)self;
        cleanup_cb_(this, !status.ok());
        self_ref_.reset();

[lonexreb]

Not adding (void)self;: clang's -Wunused-lambda-capture doesn't fire for captures whose type has a non-trivial destructor like std::shared_ptr, since holding the object alive is a legitimate use. The macOS build compiles with -Werror (.bazelrc) and bazel test //src/ray/ray_syncer/tests:ray_syncer_test built and passed cleanly with clang. The comment in ray_syncer_bidi_reactor.h documents that these captures exist for lifetime.

[gemini-code-assist]

To prevent potential compiler warnings about unused lambda captures (such as -Wunused-lambda-capture in Clang), it is recommended to explicitly cast the captured self to void inside the lambda body. This also clearly documents that the capture is intended solely for lifetime management.

  io_context_.dispatch(
      [this,
       self = self_ref_,
       cleanup_cb = cleanup_cb_,
       remote_node_id = GetRemoteNodeID()]() {
        (void)self;
        cleanup_cb(this, false);
        self_ref_.reset();

[lonexreb]

Not adding (void)self;: clang's -Wunused-lambda-capture doesn't fire for captures whose type has a non-trivial destructor like std::shared_ptr, since holding the object alive is a legitimate use. The macOS build compiles with -Werror (.bazelrc) and bazel test //src/ray/ray_syncer/tests:ray_syncer_test built and passed cleanly with clang. The comment in ray_syncer_bidi_reactor.h documents that these captures exist for lifetime.

[hikaru-212]

Independently reviewed the reactor ownership changes at 2011efd5a.

The source audit confirms that all eight dispatched handlers retain an owning shared_ptr under the normal factory paths, including through OnDone cleanup. The batch timer uses weak_ptr.lock() before accessing the reactor.

I also rebuilt and ran ray_syncer_test on Linux ARM64 with test caching disabled: 24/24 passed.

Specifically, five SyncerTest cases exercised real gRPC connections, message propagation, explicit reconnection, and disconnect cleanup. Seven authentication tests covered the production factory's acceptance and rejection paths.

No reactor ownership gap was identified within the reviewed paths. The tests do not directly verify destructor timing or reproduce the original #62008 ASAN UAF.