Skip to content

fix(server): add connection_closed shared flag for HTTP streaming stop detection#23837

Closed
kripper wants to merge 1 commit into
ggml-org:masterfrom
kripper:fix-timeout
Closed

fix(server): add connection_closed shared flag for HTTP streaming stop detection#23837
kripper wants to merge 1 commit into
ggml-org:masterfrom
kripper:fix-timeout

Conversation

@kripper

@kripper kripper commented May 28, 2026

Copy link
Copy Markdown

Overview

Replace should_stop reference with a shared_ptr<std::atomic<bool>> that is signaled by process_handler_response() on connection close, allowing pending streaming requests to detect client disconnection and stop gracefully.

Additional information

Fixes:

Also mentioned here:

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: Yes, I used AI to analyze the code. The commit was reviewed and tested by me. I will continue testing.

…p detection

Replace should_stop reference with a shared_ptr<std::atomic<bool>> that is
signaled by process_handler_response() on connection close, allowing pending
streaming requests to detect client disconnection and stop gracefully.
@kripper
kripper requested a review from a team as a code owner May 28, 2026 19:53
@ngxson

ngxson commented May 28, 2026

Copy link
Copy Markdown
Collaborator

I don't see how this solve the problem. A disconnected request now will keep going which defeats the whole point of allowing client to stop by disconnecting.

The root cause comes from 2 different problem: timeout not handled properly and task cancel doesn't produce checkpoint (edit: it does, but you need to set --checkpoint-min-step and batch size to a small value), these will have different fixes

@ngxson ngxson closed this May 28, 2026
@kripper

kripper commented May 29, 2026

Copy link
Copy Markdown
Author

Thanks for the review.

  1. Now I noticed multiple requests being deferred, which means the client is effectively retrying the request, so it must indeed be a client-side timeout.

  2. Regarding the correct solution: Can we take a checkpoint when the request is cancelled? This way we could resume exactly where we left.
    Isn't it more efficient also (only 1 checkpoint at the end)?

@kripper

kripper commented May 29, 2026

Copy link
Copy Markdown
Author

There is something strange going on, and I'm still not sure whether this is a client-side timeout:

  • With this PR, when I run a single agent (at most one request), if the client times out, I would expect to observe connection retries being deferred. However, that's not the case. The deferred counter always stays at 0. It seems there is no timeout in this scenario and everything works fine.

  • But when I run more than one agent, every so often a new connection is started and deferred (the deferred counter keeps increasing constantly).

My interpretation is:

  • the request is cancelled when a second request is deferred, waits, and then gets cancelled due to a timeout. In other words, the timeout does not happen while streaming, but while waiting for the initial response. That would explain why everything works fine with a single agent.

  • when there is a single agent, without my PR the stop condition is met after a reproducible amount of time. However, with my PR, the single agent does not time out because I'm no longer directly checking req.is_connection_closed() on the server side, and because there appears to be no client-side timeout (otherwise I would expect to observe a retry and a new deferred request).

Thus, it seems the timeout is occurring:

  • on the server side while streaming
  • and on the client side while the request is deferred and waiting for the initial response

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants