[core] Fix task event loss during shutdown #60247
Draft
+583
−8
Conversation
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
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request aims to fix task event loss during shutdown by ensuring that buffered events are flushed before the io_service is stopped. The changes in TaskEventBuffer correctly implement a synchronous wait for gRPC calls to complete. However, the same fix for RayEventRecorder is incomplete, as StopExportingEvents remains asynchronous, which could still lead to event loss. I've also found an issue in one of the new tests. My review includes suggestions to address these points.
ruoliu2
force-pushed
the
ray-event-loss-during-shutdown
branch
4 times, most recently
from
0ffd54a to
438e741
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ruoliu2
force-pushed
the
ray-event-loss-during-shutdown
branch
3 times, most recently
from
9cc8050 to
b6b0dc8
Compare
ruoliu2
force-pushed
the
ray-event-loss-during-shutdown
branch
2 times, most recently
from
396248b to
7bd049c
Compare
ruoliu2
force-pushed
the
ray-event-loss-during-shutdown
branch
3 times, most recently
from
c05dd88 to
a8ae6c3
Compare
During shutdown, TaskEventBufferImpl::Stop() and RayEventRecorder were losing buffered events because the io_service was stopped immediately after calling async gRPC flush methods. This fix: - Adds a synchronous flush with configurable timeout in TaskEventBuffer::Stop() - Adds StopExportingEvents() method to RayEventRecorder for graceful shutdown - Calls StopExportingEvents() from GcsServer::Stop() before stopping io_service - Adds new config option task_events_shutdown_flush_timeout_ms (default 5s) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: ruo <[email protected]>
Changes: - Wait for in-flight gRPC to complete before final flush - Perform final flush to send all buffered events - Wait for flush gRPC to complete before shutdown - Add payload checks to avoid sending empty gRPC requests - Use lambda helper for cleaner wait logic - Fix race condition: signal under mutex to avoid lost wakeup - Fix overlapping gRPC: skip export if gRPC already in progress - Fix re-enable window: use stopping_ flag instead of re-enabling to prevent new events during shutdown flush This ensures no events are lost during shutdown while respecting timeouts to avoid hanging indefinitely. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: ruo <[email protected]>
ruoliu2
force-pushed
the
ray-event-loss-during-shutdown
branch
from
a8ae6c3 to
6d38b2d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Fixes #60218
During shutdown,
TaskEventBufferImpl::Stop()andRayEventRecorderwere losing buffered events because the io_service was stopped immediately after calling async gRPC flush methods, without waiting for the gRPC calls to complete.This PR:
TaskEventBuffer::Stop()- waits up to 5 seconds (configurable viatask_events_shutdown_flush_timeout_ms) for in-flight gRPC calls to completeStopExportingEvents()method toRayEventRecorderfor graceful shutdownStopExportingEvents()fromGcsServer::Stop()before stopping io_servicetask_events_shutdown_flush_timeout_ms(default 5000ms)Test plan
TestStopFlushesEventsforTaskEventBufferthat verifies events are flushed duringStop()TestStopFlushesEventsforRayEventRecorderthat verifies events are exported duringStopExportingEvents()🤖 Generated with Claude Code