fix(agentserver-responses): keep background+stream handler alive on client disconnect when keep-alive is enabled#47833
Open
antriksh30 wants to merge 3 commits into
Open
fix(agentserver-responses): keep background+stream handler alive on client disconnect when keep-alive is enabled#47833antriksh30 wants to merge 3 commits into
antriksh30 wants to merge 3 commits into
Conversation
…lient disconnect when keep-alive is enabled
The FR-012/FR-013 "background+stream survives client disconnect" behavior was only
implemented on the no-keep-alive fast path. When SSE keep-alive is enabled
(sse_keep_alive_interval_seconds set) — which is how the hosted platform runs the SDK —
streaming took the keep-alive merge path, whose finally cancels the handler task on
client disconnect. That killed in-flight background runs, so a later
GET /responses/{id}?stream=true&starting_after=N reconnected to a dead run and returned
200 with no new events.
Route background+store streams to the shielded, independent-task path regardless of
keep-alive, and emit keep-alive comments from that path (fed to the same queue, stopped
from both the producer and consumer finally so it can never outlive the run). The
keep-alive merge path now only handles non-background streams, where cancelling on
disconnect is correct.
Add regression tests that run the disconnect scenario with keep-alive enabled.
antriksh30
requested review from
RaviPidaparthi,
ankitbko and
vangarp
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes azure-ai-agentserver-responses SSE streaming so that background+stream+store response runs keep executing after the client disconnects even when SSE keep-alive is enabled (hosted-platform configuration), enabling later reconnection via GET /responses/{id}?stream=true&starting_after=N.
Changes:
- Route
background+storestreaming requests through the shielded independent-task path regardless of keep-alive, preventing handler cancellation on disconnect. - Add keep-alive comment emission to the independent background-stream path with cancellation from both producer/consumer finalizers.
- Add contract regression tests covering disconnect survival with keep-alive enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sdk/agentserver/azure-ai-agentserver-responses/azure/ai/agentserver/responses/hosting/_orchestrator.py | Changes _live_stream routing and adds keep-alive emission for background+store streaming without cancelling the handler on disconnect. |
| sdk/agentserver/azure-ai-agentserver-responses/tests/contract/test_bg_stream_disconnect.py | Adds keep-alive-enabled disconnect regression tests and a helper client builder for keep-alive configuration. |
- orchestrator: capture keep-alive interval in a local (drops type: ignore[arg-type]); narrow queue item with isinstance(str) instead of type: ignore[misc] - tests: correct keep-alive docstrings (background+store no longer uses the merge path); cancel/gather the event-wait tasks to avoid leaked pending tasks
| break | ||
| yield item # type: ignore[misc] | ||
| if isinstance(item, str): | ||
| yield item |
Member
There was a problem hiding this comment.
Q: can the instance be not _SENTINEL_BG and not of type str ?
Author
There was a problem hiding this comment.
The producer only enqueues encode_sse_any_event() and encode_keep_alive_comment() - both return str - plus the _SENTINEL_BG sentinel. So, after the sentinel check, the item is always a str .
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
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.
Issue
For a
background=true, stream=true, store=trueResponses request, the background run should keep running after the client disconnects the streamingPOSTso a laterGET /responses/{id}?stream=true&starting_after=Ncan reconnect and resume. It didn't: the run died on disconnect and reconnect returned200with no new events.The disconnect-survival behavior (FR-012/FR-013) was only implemented on the no-keep-alive fast path of
_live_stream. When SSE keep-alive is enabled (sse_keep_alive_interval_secondsset) — the hosted-platform configuration — streaming took the keep-alive merge path, whosefinallycancels the handler task on client disconnect, killing the in-flight background run.Fix
background+storestreams to the shielded, independent-task path regardless of keep-alive, so they no longer hit the path that cancels the handler on disconnect.finallyso the task can never outlive the run), preserving keep-alive for background streams.