Skip to content

fix(api-server): preserve interim run event ordering across executor completion - #89748

Open
RoySRose wants to merge 1 commit into
NousResearch:mainfrom
RoySRose:fix/runs-interim-event-ordering
Open

RoySRose wants to merge 1 commit into
NousResearch:mainfrom
RoySRose:fix/runs-interim-event-ordering

Conversation

@RoySRose

Copy link
Copy Markdown

Interim run events (tool progress, reasoning previews, message deltas) are
emitted from the executor thread running agent.run_conversation() via
loop.call_soon_threadsafe(q.put_nowait, event). That only schedules the
put; it doesn't wait for the event loop to apply it. Once
run_in_executor() returns, the run coroutine resumes on the loop thread
and enqueues run.completed/run.failed and the closing None sentinel
directly via q.put_nowait() on that same thread. A still-pending
scheduled put for an interim event can lose that race and land after
run.completed, or after the SSE consumer has already seen the sentinel
and stopped reading -- silently dropping the event from the
/v1/runs/{run_id}/events stream. Observed on Python 3.13.

Add _enqueue_run_event(), a small ack-wait wrapper around
call_soon_threadsafe: when called off the loop thread it blocks (bounded
by a 2s timeout so loop shutdown can't strand the executor thread) until
the loop has actually applied the put, so the caller cannot proceed toward
run completion until its event is durably queued ahead of anything the loop
thread does afterward. Wired into both interim event paths that cross the
executor thread/event loop boundary: the tool_progress_callback and the
stream_delta_callback.

Testing

  • New unit tests directly exercise the ordering guarantee: blocks until
    applied when called off the loop thread, synchronous passthrough when
    already on the loop thread, and FIFO ordering preserved against a
    directly-enqueued completion event. Confirmed they fail against the
    pre-fix code and pass after.
  • New SSE-level test confirms an interim tool event fired just before
    run_conversation() returns still appears ahead of run.completed in
    the stream.
  • Full tests/gateway/test_api_server_runs.py suite passes (26/26), plus
    the broader test_api_server* suite (267/267), no regressions.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 19, 2026
…completion

Interim run events (tool progress, message deltas) are emitted from the
executor thread running agent.run_conversation() via
loop.call_soon_threadsafe(q.put_nowait, event). That only schedules the
put; it doesn't wait for the event loop to apply it. Once
run_in_executor() returns, the run coroutine resumes on the loop thread
and enqueues run.completed/run.failed and the closing None sentinel
directly on that same thread. A still-pending scheduled put for an
interim event can lose that race and land after run.completed, or after
the SSE consumer has already seen the sentinel and stopped reading --
silently dropping the event from the /v1/runs/{run_id}/events stream.
Observed on Python 3.13.

Add _enqueue_run_event(), a small ack-wait wrapper around
call_soon_threadsafe: when called off the loop thread it blocks (bounded
by a 2s timeout so loop shutdown can't strand the executor thread) until
the loop has actually run the given put, so the caller cannot proceed
toward run completion until its event is durably queued ahead of
anything the loop thread does afterward. Wire it into both interim event
paths that cross the executor thread/event loop boundary: the
tool_progress callback (_push) and the stream_delta_callback (_text_cb).
_text_cb routes through the existing _put_event_if_active staleness
guard rather than a bare queue put, so _enqueue_run_event takes a zero-arg
callable instead of a fixed (queue, event) pair to accommodate both call
sites without bypassing that guard.

Add regression coverage: unit tests for _enqueue_run_event itself
(blocks until applied off-loop, synchronous on-loop, FIFO against a
racing direct put, and respects the _put_event_if_active staleness
guard) plus an SSE-level test that an interim tool event fired just
before run_conversation() returns still shows up ahead of run.completed
in the stream. Tests: tests/gateway/test_api_server_runs.py.
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Reviewed by reviewer-e (AI automated review).

Correct race analysis and fix: call_soon_threadsafe only schedules, so a pending interim put could lose to run.completed/sentinel enqueued directly on the loop thread the moment run_in_executor resolved — blocking the executor thread until the loop has actually applied each put restores the happens-before ordering, the loop-thread passthrough avoids self-deadlock, and the 2s bound keeps a dying loop from stranding the worker. The tests are exactly what this needs: an end-to-end SSE ordering regression plus unit coverage of the happens-before guarantee, same-thread passthrough, and multi-event submission order. Findings below are minor:

  1. gateway/platforms/api_server.py:_enqueue_run_event — when applied.wait(timeout=2.0) times out, the interim event is silently dropped (identical to the pre-fix behavior, but now after deliberately stalling the executor thread for 2s). A single logger.debug/warning naming the run and event type on that branch would make a stalled-loop incident diagnosable instead of looking like flaky SSE gaps.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants