fix(cursor): close HTTP/2 after turnEnded so a held-open response cannot stall the turn - #2321
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Cursor live transport now waits 500 ms after ChangesCursor transport closure
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
intake: hygiene-blocked
|
⏳ DRAFT
What to do
Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f2a37af17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.turnEndedCloseTimer) return; | ||
| this.turnEndedCloseTimer = setTimeout(() => { | ||
| this.turnEndedCloseTimer = undefined; | ||
| if (this.expectedClose || this.emittedTerminal) return; |
There was a problem hiding this comment.
When a turnEnded frame is decoded, mapCursorProtobufServerMessage() synchronously returns either done or the fail-closed error, and the local push() sets this.emittedTerminal = true before this 500 ms callback can run. This guard therefore always returns for the frame that armed the timer, so an upstream that holds HTTP/2 or HTTP/1.1 open continues to pin run() until the external stall timeout—the exact hang this change is intended to fix. Gate only an already-requested close (or track this timer with a dedicated flag), and cover the held-open-server case with a focused transport test.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
리뷰 · 우선순위 61 / 80지금 현재 막힘. 타이머 가드가 더. (1)
해결방안: 드래프트 유지. 이 댓글은 grok-bot이 작성했습니다 |
4f2a37a to
2828efc
Compare
…not stall the turn T03 (senpi #1062): after the server sends turnEnded, the application turn is complete. A server that keeps the HTTP/2 stream open past this point cannot hold the turn hostage until a 300s bridge idle timeout. Close our side after a 500ms grace so trailing frames (late usage, checkpoint) still land before we release the socket. The grace timer only checks expectedClose (client-tool suspend cancel); emittedTerminal is deliberately not checked because finalizeTurnEvents sets it synchronously during turnEnded mapping, before the timer fires. The client-tool suspend path (live-transport.ts:203-206) intentionally ends without waiting for turnEnded and is not affected by this change.
2828efc to
90dd314
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90dd314798
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.http1Connection.close(); | ||
| } else { | ||
| try { | ||
| this.stream?.close(); |
There was a problem hiding this comment.
Although the previous emittedTerminal guard is now removed, a held-open HTTP/2 response can still hang under Bun: in the available Bun runtime, ClientHttp2Stream.close() marks the stream closed but does not emit the end or error event that open() relies on to invoke its settler, so run() remains blocked after this timer fires. Explicitly settle after queued frame work drains, or force destroy() and cover this with an H2 fixture that writes turnEnded without calling stream.end().
AGENTS.md reference: src/AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
1 participant
Summary
turnEnded, the application turn is complete. A server that keeps the HTTP/2 stream open past this point cannot hold the turn hostage until a 300s bridge idle timeout.turnEndedand is not affected.Stacked on #2320 (T01 error classification).
Verification
bun test tests/cursor-live-transport.test.ts tests/cursor-protobuf-events.test.ts tests/cursor-errors.test.ts— 78 pass / 0 failbun run typecheck— exit 0Checklist
Summary by CodeRabbit