Skip to content

[Server] Stop overwriting client responses while polling for them - #545

Closed
chr-hertel wants to merge 1 commit into
mainfrom
claude/session-lost-update
Closed

chr-hertel wants to merge 1 commit into
mainfrom
claude/session-lost-update

Conversation

@chr-hertel

Copy link
Copy Markdown
Member

A request from the server to the client (sampling, elicitation, roots) intermittently timed out over Streamable HTTP when the server runs in more than one process. In CI this showed up as rare 30–90s hangs in the multi-worker php -S tests, so far in DualEraEndpointTest::testAsksForInput (#540) and in HttpClientCommunicationTest on #541.

Cause

While such a request is pending, the process holding the SSE stream polls the session every 100 ms. Each poll calls Protocol::consumeOutgoingMessages(), which:

  1. reads the session,
  2. empties the outgoing queue,
  3. saves the whole session back, even when the queue was already empty.

The client's answer arrives as a separate POST, usually handled by another process, which stores it in the same session. If that store lands between steps 1 and 3, step 3 writes the old copy back, and the answer is lost. The waiting request never sees it and times out. FileSessionStore writes atomically, but nothing protects a read-modify-write, so the session's last writer wins.

Fix

consumeOutgoingMessages() leaves the session untouched when there is nothing to dequeue. While it waits, the polling loop then only reads, until the answer is there.

Test

ProtocolSessionRaceTest interleaves the two processes deterministically. A store decorator runs the answering worker's processInput() right after the waiting worker's read.

  • On main, the answer is gone (checkResponse() returns null).
  • With this change, it is found.

Existing unit tests, including the ones counting save() calls with messages queued, are unchanged and green.

Not covered

Writes that do have something to persist are still unprotected read-modify-writes of the whole session. Two processes changing a session at the same moment can still lose one change. Closing that fully needs locking or a compare-and-swap in SessionStoreInterface, which is a larger change. This PR removes the write that happened on every poll, which is the one that made this race likely.

🤖 Generated with Claude Code

https://claude.ai/code/session_014RjzbQfz2DVm9xHmGsqM7d


Generated by Claude Code

Comment thread src/Server/Protocol.php Outdated
@chr-hertel
chr-hertel force-pushed the claude/session-lost-update branch from f43de77 to c25ce09 Compare October 7, 2026 20:11
@chr-hertel chr-hertel added bug Something isn't working Server Issues & PRs related to the Server component labels Oct 7, 2026
While a request to the client is pending over HTTP, the process holding
the SSE stream polls the session every 100ms, and every poll emptied the
outgoing queue and saved the whole session, even with nothing queued. The
client's answer arrives as a separate POST, usually in another process,
which stores it in the same session. A poll that read the session just
before that store and saved just after wrote the old copy back, the answer
was lost, and the request timed out.

Leave the session untouched when there is nothing to dequeue, so the
waiting loop only reads until the answer is there. This showed up as
intermittent timeouts of the multi-worker php -S tests in CI.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RjzbQfz2DVm9xHmGsqM7d
@chr-hertel
chr-hertel force-pushed the claude/session-lost-update branch from c25ce09 to 21429a5 Compare October 7, 2026 20:28
@chr-hertel

Copy link
Copy Markdown
Member Author

Side effect of introducing client tests - will be closed in favor of #535

@chr-hertel chr-hertel added the on hold Blocked on external dependency (SEP, other PR, decision) label Oct 7, 2026
@chr-hertel chr-hertel closed this Oct 8, 2026
guillaume-sainthillier added a commit to guillaume-sainthillier/php-sdk that referenced this pull request Oct 8, 2026
Ported from modelcontextprotocol#545: one worker polls the session for a client's answer while another stores it. Saving the session on every poll used to overwrite that answer.

The store fixture moves to Session/Fixture and gains a hook after the next read.

Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working on hold Blocked on external dependency (SEP, other PR, decision) Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants