Repository navigation
fix(tui): port upstream failed RPC request reply (anomalyco/opencode#54936) - #1617
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 1 finding(s)
- 1 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/util/rpc.ts (L68-L70)
[🟠 MEDIUM] If JSON.stringify(input) throws an error (e.g., due to circular references in the input argument), the Promise executor will correctly catch it and reject the promise. However, because pending.set was already called, the requestId will remain in the pending map indefinitely. Since the request is never actually sent via postMessage, a response will never arrive to clear it, resulting in a minor memory leak for that map entry.
Consider evaluating JSON.stringify before setting the request in the pending map. If serialization fails, the error will be caught by the Promise executor before the map is mutated, thus preventing the leak:
return new Promise((resolve, reject) => {
const payload = JSON.stringify({ type: "rpc.request", method, input, id: requestId })
pending.set(requestId, { resolve, reject })
target.postMessage(payload)
})| // altimate_change start — upstream_fix: answer a failed RPC request instead of hanging the caller (anomalyco/opencode#54936) | ||
| return new Promise((resolve, reject) => { | ||
| pending.set(requestId, { resolve, reject }) |
There was a problem hiding this comment.
[🟠 MEDIUM] If JSON.stringify(input) throws an error (e.g., due to circular references in the input argument), the Promise executor will correctly catch it and reject the promise. However, because pending.set was already called, the requestId will remain in the pending map indefinitely. Since the request is never actually sent via postMessage, a response will never arrive to clear it, resulting in a minor memory leak for that map entry.
Consider evaluating JSON.stringify before setting the request in the pending map. If serialization fails, the error will be caught by the Promise executor before the map is mutated, thus preventing the leak:
return new Promise((resolve, reject) => {
const payload = JSON.stringify({ type: "rpc.request", method, input, id: requestId })
pending.set(requestId, { resolve, reject })
target.postMessage(payload)
})
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
Issue for this PR
Closes #1616
Type of change
What does this PR do?
The TUI waits on RPC calls to its server worker before it draws anything (e.g.
client.call("server", network)for--port). When a worker handler threw,Rpc.listennever posted a reply andRpc.clienthad no reject path, so the call never settled:altimate-code --port 4096with the port taken is a blank terminal forever.Ported from upstream anomalyco/opencode#54936:
Rpc.listenanswers a failed request with{ type: "rpc.result", error, id }. If the error has no message (Server.listenrejects with an emptyServeErrorthat names the port in its cause), the cause's message is used.Rpc.clienttracks{ resolve, reject }per call and rejects when the result carries anerrorstring, so the failure reaches the TUI's existing error handling.Wrapped in
altimate_changemarkers taggedupstream_fix:. Successful calls are unchanged.How did you verify your code works?
test/util/rpc.test.ts: the client rejects a call whose result carries an error, andlistenanswers a throwing handler (with cause fallback) and keeps answering later requests. Both fail onmainand pass here; the existing 5 RPC tests still pass.bun run typecheckinpackages/opencodeclean;bun run script/upstream/analyze.ts --markers --base origin/main --strictclean.Cost side effects
None. No model calls, queries or resources added.
Screenshots / recordings
N/A, no UI change beyond the existing error message now being reachable.
Checklist
Written by Devin
Summary by cubic
Fixes the TUI hanging forever when an RPC worker handler throws, so
--portwith a port already taken now shows an error instead of a blank terminal.Previously
Rpc.listennever replied to a failed request andRpc.clienthad no reject path, so the call never settled. Nowlistenanswers failures with anerrorfield (falling back to the error'scausemessage for empty errors like the port-in-useServeError), and the client rejects on that error so the TUI's existing error handling runs. Successful calls are unchanged. Closes #1616.Written for commit 8707ec3. Summary will update on new commits.