fix(framework): Avoid race condition in agent publishing - #8000
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the shutdown semantics of RuntimeAgentEvents (the background Agent event publisher) to prevent premature worker termination and adds a unit test to validate that queued events are published on close.
Changes:
- Switch the worker loop from
_closed-gated termination to a stop-sentinel–driven loop (while True+_EVENT_PUBLISH_STOP). - Change
close()to enqueue the stop sentinel using a blockingQueue.put()instead ofput_nowait(). - Add a unit test asserting that
close()drains/publishes queued events before the worker stops.
Critical issues
close(timeout=...)can still hang indefinitely becauseQueue.put()is unbounded (no timeout) and occurs beforejoin(), so the provided timeout may be ignored.- A concurrent/in-flight
emit()can enqueue an event after the stop sentinel (it may pass the_closedcheck beforeclose()flips the flag), causing the worker to exit on the sentinel and leave a late event unpublished.
Simplicity/readability suggestions
- None.
Consistency concerns
- None.
Whether the PR should be split
- No.
Overall verdict
- Not ready as-is due to shutdown/timeout edge cases that can lead to hangs or dropped events.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| framework/py/flwr/supercore/task_process/agent/session.py | Changes worker shutdown behavior to use a stop sentinel rather than _closed loop termination. |
| framework/py/flwr/supercore/task_process/agent/session_test.py | Adds a test to verify queued events are published before shutdown completes. |
Suppressed comments (1)
framework/py/flwr/supercore/task_process/agent/session.py:131
- With the stop-sentinel approach, an
emit()that is already in progress can enqueue aTaskEventafter_EVENT_PUBLISH_STOP(it can pass the_closedcheck beforeclose()flips the flag). In that case, the worker will return as soon as it reads the stop sentinel and the late event remains unpublished in the queue. Consider switching to a two-phase shutdown: once the stop sentinel is observed, drain any remaining queued events (for a short grace period) before exiting.
while True:
item = self._queue.get()
if item is _EVENT_PUBLISH_STOP:
return
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
framework/py/flwr/supercore/task_process/agent/session.py:110
timeoutno longer boundsclose(): this blockingput()happens beforejoin(timeout). If all 256 slots are occupied while the publisher is stuck inPushTaskEvents, theclose(1)used by the signal-exit path can hang indefinitely and never reach the timed join. Apply one deadline to both enqueueing the stop marker and joining the worker (using a timedput, translatingFulltoTimeoutError, then joining with the remaining time).
self._queue.put(_EVENT_PUBLISH_STOP)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a74bf0e48f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| except Full: | ||
| pass # The worker will still stop due to the `_closed` flag. | ||
| self._closed = True | ||
| self._queue.put(_EVENT_PUBLISH_STOP) |
There was a problem hiding this comment.
Honor the close timeout while enqueueing the stop marker
When the 256-entry queue is full because PushTaskEvents is slow or stalled, this blocking put occurs before join(timeout), so the timeout does not bound close at all. In particular, run_agentapp.py calls agent_events.close(1) during exit, but shutdown can instead wait for the Runtime HTTP request timeout (or indefinitely if the worker has terminated unexpectedly). Apply the same deadline to inserting the stop marker, or otherwise avoid blocking before the timed join.
Useful? React with 👍 / 👎.
Issue
Description
Related issues/PRs
Proposal
Explanation
Checklist
#contributions)Any other comments?