Repository navigation
Mark a removed subscriptions/listen stream closed - #587
Conversation
ba0ba0c to
1e91d38
Compare
soyeladice-svg
left a comment
There was a problem hiding this comment.
AI-assisted review: this closes the snapshot-before-removal case when the writer is still waiting on write_mutex, but I think a smaller race remains once a writer has already acquired that mutex and passed next if subscription[:closed]. remove_listen_subscription() marks closed under the registry mutex, then callers such as the keepalive ensure and delivery-error path call close_stream_safely() without taking write_mutex. A writer that passed the flag check just before removal can therefore still race the close and fail against a stream being closed—the exact failure mode described in the PR, just in the post-check window. Could removal stay registry-lock-only, but the subsequent close be serialized under subscription[:write_mutex] (after the registry lock is released)? That preserves the lock-order rule while letting an in-flight writer finish before close. A regression that blocks inside send_to_stream after the closed check, removes the entry, and asserts close waits for the writer would pin this down.
## Motivation and Context Removing a listen stream after a failed write or a finished keepalive did not mark its entry closed, as transport close does, so a delivery or keepalive that had taken the entry before the removal still wrote to the stream being closed. That write failed against the closing stream and was reported as a failed delivery. Removal now marks the entry closed, and those writes skip it under the stream's write mutex. The flag is set without taking the write mutex, which must never be taken inside the registry lock; the close that follows a removal takes the write mutex outside the registry lock, so a write already past its check lands before the stream closes, and every later one sees the flag. ## How Has This Been Tested? A new test in `test/mcp/server/transports/streamable_http_transport_test.rb` delivers to an entry taken before its removal and checks that nothing is written. Against the previous library the notification is written to the removed stream. The keepalive failure test also checks that the entry a failed ping removes is marked closed. Another blocks a delivery inside its write, removes the entry from a second thread, and checks that the stream is closed only after the write has completed. ## Breaking Changes None.
1e91d38 to
0cef024
Compare
|
Thanks! That window was real. The close after a removal now takes the stream's write mutex ( |
Motivation and Context
Removing a listen stream after a failed write or a finished keepalive did not mark its entry closed, as transport close does, so a delivery or keepalive that had taken the entry before the removal still wrote to the stream being closed.
That write failed against the closing stream and was reported as a failed delivery.
Removal now marks the entry closed, and those writes skip it under the stream's write mutex. The flag is set without taking the write mutex, which must never be taken inside the registry lock; a write already past its check proceeds and may fail against the closing stream, and every later one sees the flag.
How Has This Been Tested?
A new test in
test/mcp/server/transports/streamable_http_transport_test.rbdelivers to an entry taken before its removal and checks that nothing is written. Against the previous library the notification is written to the removed stream.The keepalive failure test also checks that the entry a failed ping removes is marked closed.
Breaking Changes
None.
Types of changes
Checklist