fix: handle cancelled SSE streams - #34
Conversation
|
Also, let me know if you would merge more similar PRs, I've been making fixes to issues I've encountered while using this. I can open more PRs with small evidence based changes. |
ephraimduncan
left a comment
ephraimduncan
left a comment
There was a problem hiding this comment.
The hoisted closed flag correctly prevents enqueue-after-cancel, but the new cancel path needs to tear the bridge down, not just mute the stream.
| } | ||
| }); | ||
| }, | ||
| cancel() { |
There was a problem hiding this comment.
Setting closed = true mutes the SSE output but leaves the bridge fully alive: the heartbeat interval started at line 1563 keeps writing every 5s, and the code's own comment at 1499-1501 notes heartbeats otherwise keep the bridge open forever. bridge.onData also keeps running, so the exec path at 1464-1471 still registers the session in activeBridges even though the disconnected client will never send the follow-up that resumes it, leaving the entry, timer, and subprocess alive until a same-key retry or a config reload. Please mirror the endStream-error teardown at 1507-1509 here: clear heartbeatTimer, delete the activeBridges entry, and call bridge.end().
There was a problem hiding this comment.
Disclaimer: used an llm to research, fix, and write this reply.
You are correct. closed = true does not stop the bridge. I pushed a follow-up commit.
But bridge.end() does not stop the bridge either. It does a half-close. It writes a zero-length message, and this message breaks the stdin loop in h2-bridge.mjs. The loop then calls h2Stream.end() on the request half only. The response half stays open, and h2Stream.on("data") continues to call resetTimeout(). Thus the subprocess stays alive.
bridge.onData also stays installed. The model sends its tool call, and the exec handler writes the activeBridges entry again. The teardown deleted that entry a moment earlier. The map has no TTL, so the new entry stays until a same-key retry or a call to stopProxy().
I did not assume this. I replaced kill() with end() and ran the new test. It failed in the same way as no fix at all.
The commit kills the bridge process on cancellation, clears the heartbeat, and removes the activeBridges entry only if it still belongs to that bridge. This identity check prevents cancellation from deleting a newer same-key bridge. The endStream error path uses the same teardown, changing that path from a half-close to a hard stop as well.
One detail needs a note. cancel() does nothing if the stream is already closed. The tool-call pause path closes its controller on purpose, but it keeps the bridge in activeBridges for the continuation. An unguarded cancel would stop a bridge that OpenCode must resume. The streams specification does not permit this sequence, but the guard costs nothing, and the failure is silent without it.
For the test, I changed the fake H2 backend to hold the Run stream open. A flag controls this behavior, so the non-streaming test still stops. The test aborts the fetch during the stream. It then checks that the upstream Run stream closes. Only the subprocess holds a handle to that stream. Thus a closed stream shows that the process stopped, and not only that it stopped to write.
The last point is optional. I traced the abort path in the Cursor CLI bundle. The client writes a ConversationAction{cancel_action} on the same bidi stream, waits for that write, and only then sends RST_STREAM(CANCEL). It never does a half-close, and it never keeps a cancelled stream for a resume. Thus a hard kill is correct, and cancel_action only lets the server stop and write a final checkpoint. The repo has all the necessary types, but I did not test this against the live server, so I kept it out of this PR.
Muting the controller stopped the crash but left the bridge fully alive: heartbeats kept writing every 5s and bridge.onData kept running, so a disconnected client's exec would still register the session in activeBridges with no follow-up ever coming to resume it. Killing is what actually stops an in-flight run. bridge.end() only half-closes the request side; the bridge keeps reading the response half and the subprocess survives. cancel() is a no-op on an already-closed stream so it cannot tear down a bridge paused for tool-result continuation.
3478ef7 to
67cd2f4
Compare
2 participants
This fixes an issue I had using the plugin, have been getting this error:
I used an LLM to fix the issue. While I am not very familiar with typescript, the fix looks reasonable to me, and the issue did not persist after making the change.