Skip to content

fix(http): make SSE close and response delivery deterministic#209

Open
federicociner wants to merge 1 commit into
agentclientprotocol:mainfrom
federicociner:fix/http-sse-lifecycle
Open

fix(http): make SSE close and response delivery deterministic#209
federicociner wants to merge 1 commit into
agentclientprotocol:mainfrom
federicociner:fix/http-sse-lifecycle

Conversation

@federicociner

@federicociner federicociner commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Abort active HTTP SSE requests before awaiting connection DELETE during transport close
  • Move SSE body creation into an internal helper and enqueue backend messages immediately after they are read
  • Add regression coverage for close abort ordering and post-read SSE message delivery when demand changes

Rationale

These changes address two HTTP/SSE lifecycle races that can cause ACP clients to hang across HTTP transport implementations.

1. Close needed to abort SSE before DELETE

HttpStreamTransport.close() previously awaited the connection DELETE request before aborting long-lived SSE requests. That ordering is risky because server-side cleanup may depend on active streams being torn down first.

If a client waits for DELETE while SSE GETs are still alive, shutdown can stall: the transport is trying to delete the connection while the long-lived streams for that same connection remain active. Depending on the server, runtime, or Web Streams adapter, that can keep cleanup work open longer than intended or cause close to hang.

The fix aborts the transport’s shared AbortController before awaiting DELETE, so active SSE streams and in-flight requests are signalled to stop immediately. DELETE then runs while local stream teardown is already in progress instead of waiting behind long-lived reads.

2. SSE body could strand a message after reading it

The server SSE body only enters pull() when there is stream demand. However, after it awaited subscription.stream.read(), it checked desiredSize again. If demand dropped to zero in that small window, the already-read message was saved in pendingMessage and pull() returned.

That is problematic because some Web Streams implementations do not promptly call pull() again after demand changes in this situation. The message has already been removed from the backend stream, but it is not written to the HTTP SSE response, so the client can wait forever for a JSON-RPC response that the server has technically already consumed.

The fix is to enqueue the message immediately once it has been read from the backend stream. Backpressure is still respected before starting the read, but once a message has been consumed, delivery becomes deterministic.

Impact

Together, these fixes prevent hangs where:

  • Transport shutdown waits on cleanup while long-lived SSE GETs remain active;
  • A client request waits indefinitely for a JSON-RPC response that was read internally but never emitted over SSE;
  • Behavior depends on timing-sensitive interactions between Web Streams demand, backend reads, and HTTP response delivery.

Abort active HTTP SSE requests before attempting connection DELETE so transport close does not wait on long-lived GET streams before it can tear them down.

Also avoid parking a subscription message after it has already been read from the backend stream. The SSE body pull is entered when there is demand; re-checking desiredSize after awaiting the backend read can strand the message with no prompt follow-up pull under some Web Streams adapters.

This fixes cases where session/load responses or errors are read from the backend stream but the client request or transport cleanup hangs.
@federicociner

Copy link
Copy Markdown
Contributor Author

Fixing this in #198 since it is related to the backend SPI feature.

@benbrandt

Copy link
Copy Markdown
Member

I am happy to land it independently if you want

@federicociner federicociner reopened this Jul 6, 2026
@federicociner

Copy link
Copy Markdown
Contributor Author

Thanks @benbrandt I've reopened the PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants