fix(http): make SSE close and response delivery deterministic#209
Open
federicociner wants to merge 1 commit into
Open
fix(http): make SSE close and response delivery deterministic#209federicociner wants to merge 1 commit into
federicociner wants to merge 1 commit into
Conversation
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.
Contributor
Author
|
Fixing this in #198 since it is related to the backend SPI feature. |
Member
|
I am happy to land it independently if you want |
Contributor
Author
|
Thanks @benbrandt I've reopened the PR. |
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.
Summary
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 connectionDELETErequest 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
DELETEwhile 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
AbortControllerbefore awaitingDELETE, so active SSE streams and in-flight requests are signalled to stop immediately.DELETEthen 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 awaitedsubscription.stream.read(), it checkeddesiredSizeagain. If demand dropped to zero in that small window, the already-read message was saved inpendingMessageandpull()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: