fix(tui): resolve streaming freeze from unhandled finish event, missing timeouts, and unguarded effects#4
fix(tui): resolve streaming freeze from unhandled finish event, missing timeouts, and unguarded effects#4coleleavitt wants to merge 1 commit intodevfrom
Conversation
…ng timeouts, and unguarded effects - Fix stream termination in processor.ts: for-await loop now breaks on finish event - Increase SDK event batch window from 16ms to 50ms to reduce render churn - Add withTimeout to MCP client.listTools() to prevent indefinite hangs - Handle fire-and-forget effect Promise rejections in db.ts
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's stability and performance by addressing several critical issues that could lead to hangs, unresponsiveness, or silent failures. The changes ensure proper termination of streaming processes, optimize event handling for better UI responsiveness, prevent indefinite waits on external service calls, and improve error resilience for asynchronous operations. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThese changes adjust event batching thresholds to reduce immediate renders, add per-client timeout configuration for MCP tool discovery, introduce stream completion signaling in session processing, and wrap side-effect invocations with Promise-based error handling for improved fault tolerance. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Review Summary by QodoFix TUI streaming freeze from unhandled finish event and missing timeouts
WalkthroughsDescription• Fix stream termination in processor.ts where finish event now properly breaks the for-await loop • Add timeout handling to MCP client.listTools() to prevent indefinite hangs • Increase SDK event batch window from 16ms to 50ms reducing render frequency • Add error handling for fire-and-forget effect Promises in db.ts Diagramflowchart LR
A["Stream Processing"] -->|finish event| B["Break Loop"]
C["MCP Client"] -->|withTimeout| D["Prevent Hangs"]
E["SDK Events"] -->|batch 50ms| F["Reduce Renders"]
G["DB Effects"] -->|error handling| H["Prevent Crashes"]
File Changes1. packages/opencode/src/session/processor.ts
|
Code Review by Qodo
1. catch uses implicit any
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request delivers several crucial fixes to address application freezes and hangs. The changes include properly terminating a stream processing loop, adding a necessary timeout to a client call, and safeguarding against unhandled promise rejections from side effects. A performance enhancement to batch UI events is also included. The fixes are well-implemented. I have one suggestion to improve code maintainability by using a constant for a hardcoded value.
| if (elapsed < 50) { | ||
| timer = setTimeout(flush, 50) |
There was a problem hiding this comment.
The batch window time 50 is used as a magic number in both the condition and the setTimeout call. To improve readability and maintainability, consider defining it as a constant, for example const BATCH_WINDOW_MS = 50;, at a higher scope (e.g., at the top of the init function) and using it here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/src/storage/db.ts (1)
125-125: Extract shared effect-drain logic to avoid drift.Line 125 and Line 149 duplicate the same fire-and-forget error-handling pattern. A small helper keeps both paths consistent.
Proposed refactor
+ function runEffects(effects: (() => void | Promise<void>)[]) { + for (const effect of effects) { + Promise.resolve(effect()).catch((e) => log.error("effect failed", { error: e })) + } + } + export function use<T>(callback: (trx: TxOrDb) => T): T { @@ - for (const effect of effects) Promise.resolve(effect()).catch((e) => log.error("effect failed", { error: e })) + runEffects(effects) return result @@ - for (const effect of effects) Promise.resolve(effect()).catch((e) => log.error("effect failed", { error: e })) + runEffects(effects) return resultAlso applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/storage/db.ts` at line 125, Two duplicated fire-and-forget loops call Promise.resolve(effect()).catch(...) — extract that logic into a small helper (e.g., drainEffects or runFireAndForget) and use it in both places to keep behavior consistent; the helper should accept an iterable of effect functions (or promises), call Promise.resolve(effect()) for each, and log errors via log.error("effect failed", { error: e }) on rejection so both the block that iterates over effects and the other identical call reuse the same implementation.packages/opencode/src/cli/cmd/tui/context/sdk.tsx (1)
57-58: Cap the delay to the remaining batch window.At Line 57–Line 58, scheduling a full
50mseven whenelapsedis already close to50mscan stretch effective flush latency close to ~100ms. Use the remaining window instead.Proposed refactor
- if (elapsed < 50) { - timer = setTimeout(flush, 50) + if (elapsed < 50) { + timer = setTimeout(flush, Math.max(0, 50 - elapsed)) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/cli/cmd/tui/context/sdk.tsx` around lines 57 - 58, The scheduling uses a fixed 50ms even when part of the batch window has already elapsed; change the timeout to the remaining window so you don't push flush out to ~100ms. In the block that checks elapsed and sets timer (variables: elapsed, timer, flush), compute the remaining delay (e.g., 50 - elapsed), clamp it to a non-negative value, and pass that remaining delay into setTimeout instead of the constant 50 to ensure the flush runs at the end of the intended 50ms window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/src/cli/cmd/tui/context/sdk.tsx`:
- Around line 57-58: The scheduling uses a fixed 50ms even when part of the
batch window has already elapsed; change the timeout to the remaining window so
you don't push flush out to ~100ms. In the block that checks elapsed and sets
timer (variables: elapsed, timer, flush), compute the remaining delay (e.g., 50
- elapsed), clamp it to a non-negative value, and pass that remaining delay into
setTimeout instead of the constant 50 to ensure the flush runs at the end of the
intended 50ms window.
In `@packages/opencode/src/storage/db.ts`:
- Line 125: Two duplicated fire-and-forget loops call
Promise.resolve(effect()).catch(...) — extract that logic into a small helper
(e.g., drainEffects or runFireAndForget) and use it in both places to keep
behavior consistent; the helper should accept an iterable of effect functions
(or promises), call Promise.resolve(effect()) for each, and log errors via
log.error("effect failed", { error: e }) on rejection so both the block that
iterates over effects and the other identical call reuse the same
implementation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/opencode/src/cli/cmd/tui/context/sdk.tsxpackages/opencode/src/mcp/index.tspackages/opencode/src/session/processor.tspackages/opencode/src/storage/db.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/storage/db.ts">
<violation number="1" location="packages/opencode/src/storage/db.ts:125">
P1: `Promise.resolve(effect())` does not catch synchronous throws from `effect()`. If the effect function throws synchronously, the exception escapes before `Promise.resolve` wraps it, crashing the caller and skipping remaining effects. Use `Promise.resolve().then(() => effect())` to defer execution into the microtask queue, which catches both sync throws and async rejections.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const effects: (() => void | Promise<void>)[] = [] | ||
| const result = ctx.provide({ effects, tx: Client() }, () => callback(Client())) | ||
| for (const effect of effects) effect() | ||
| for (const effect of effects) Promise.resolve(effect()).catch((e) => log.error("effect failed", { error: e })) |
There was a problem hiding this comment.
P1: Promise.resolve(effect()) does not catch synchronous throws from effect(). If the effect function throws synchronously, the exception escapes before Promise.resolve wraps it, crashing the caller and skipping remaining effects. Use Promise.resolve().then(() => effect()) to defer execution into the microtask queue, which catches both sync throws and async rejections.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/storage/db.ts, line 125:
<comment>`Promise.resolve(effect())` does not catch synchronous throws from `effect()`. If the effect function throws synchronously, the exception escapes before `Promise.resolve` wraps it, crashing the caller and skipping remaining effects. Use `Promise.resolve().then(() => effect())` to defer execution into the microtask queue, which catches both sync throws and async rejections.</comment>
<file context>
@@ -122,7 +122,7 @@ export namespace Database {
const effects: (() => void | Promise<void>)[] = []
const result = ctx.provide({ effects, tx: Client() }, () => callback(Client()))
- for (const effect of effects) effect()
+ for (const effect of effects) Promise.resolve(effect()).catch((e) => log.error("effect failed", { error: e }))
return result
}
</file context>
| for (const effect of effects) Promise.resolve(effect()).catch((e) => log.error("effect failed", { error: e })) | |
| for (const effect of effects) Promise.resolve().then(() => effect()).catch((e) => log.error("effect failed", { error: e })) |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| const effects: (() => void | Promise<void>)[] = [] | ||
| const result = ctx.provide({ effects, tx: Client() }, () => callback(Client())) | ||
| for (const effect of effects) effect() | ||
| for (const effect of effects) Promise.resolve(effect()).catch((e) => log.error("effect failed", { error: e })) |
There was a problem hiding this comment.
1. catch uses implicit any 📘 Rule violation ✓ Correctness
The new Promise rejection handlers introduce e as an implicit any (from Promise.catch), which weakens type safety. This can hide mistakes when logging/inspecting errors and violates the no-any rule.
Agent Prompt
## Issue description
New `.catch((e) => ...)` handlers introduce an implicit `any` rejection reason, violating the requirement to avoid `any`.
## Issue Context
In TypeScript, `Promise.catch` typically types the rejection reason as `any`; explicitly annotating as `unknown` and narrowing preserves type safety.
## Fix Focus Areas
- packages/opencode/src/storage/db.ts[125-125]
- packages/opencode/src/storage/db.ts[149-149]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const mcpEntry = config[clientName] | ||
| const timeout = (isMcpConfigured(mcpEntry) ? mcpEntry.timeout : undefined) ?? defaultTimeout ?? DEFAULT_TIMEOUT | ||
| const toolsResult = await withTimeout(client.listTools(), timeout).catch((e) => { | ||
| log.error("failed to get tools", { clientName, error: e.message }) | ||
| const failedStatus = { | ||
| status: "failed" as const, |
There was a problem hiding this comment.
2. Mcp timeout leaks clients 🐞 Bug ⛯ Reliability
On listTools() timeout/failure in MCP.tools(), the client is deleted from state without calling client.close(), potentially leaking transports/sockets. This is especially risky now that withTimeout() will trigger failures that previously would hang indefinitely.
Agent Prompt
### Issue description
When `listTools()` times out/fails in `MCP.tools()`, the client is removed from state but never closed, potentially leaking resources.
### Issue Context
Other code paths (e.g., `disconnect()` and `create()` when initial `listTools()` fails) close clients, suggesting this is expected cleanup behavior.
### Fix Focus Areas
- packages/opencode/src/mcp/index.ts[578-590]
### Suggested changes
1) Before `delete s.clients[clientName]`, do:
- `await client.close().catch((error) => log.error("Failed to close MCP client", { clientName, error }))`
2) Make the error logging safe:
- Replace `error: e.message` with `error: e instanceof Error ? e.message : String(e)` (to avoid issues if `e` is `null`/non-object).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR fixes a critical streaming hang bug that left sessions stuck after completion and adds timeouts to prevent indefinite waits on MCP servers, alongside defensive error handling and performance tuning.
🌟 Strengths
- Effectively addresses root causes of observed freezes with targeted fixes.
- Enhances system robustness through timeouts and error logging.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | src/session/processor.ts | Bug | Fixes hang bug leaving sessions stuck after streaming | |
| P1 | src/mcp/index.ts | Architecture | Prevents indefinite hangs from unresponsive MCP servers | |
| P2 | src/storage/db.ts | Maintainability | Logs errors to prevent silent crashes from background effects | |
| P2 | src/cli/cmd/tui/context/sdk.tsx | Performance | Increases batching window; may reduce responsiveness but cut render churn | path:src/cli/cmd/tui/context/sync.tsx |
🔍 Notable Themes
- Timeout and Error Handling: Multiple changes introduce timeouts and error logging to prevent hangs and silent failures, improving overall system resilience.
- Performance vs. Responsiveness Trade-off: The increased batching window aims to reduce render churn but requires monitoring for potential latency impacts on UI responsiveness.
📈 Risk Diagram
This diagram illustrates the fix for the streaming hang bug where the loop now breaks on the finish event.
sequenceDiagram
participant SP as Session Processor
participant LS as LLM Stream
SP->>LS: Start streaming
loop Stream Events
LS->>SP: Emit event (e.g., text, reasoning)
alt Event is "finish"
Note over SP: R1(P1): Sets finished flag and breaks loop to prevent hang
SP->>SP: Break loop
end
end
SP->>SP: Exit loop and continue
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: src/session/processor.ts
The introduction of the finished flag and the break condition directly addresses a critical hang bug where the for await (const value of stream.fullStream) loop would never terminate after receiving a "finish" event, leaving the session processor stuck at 0% CPU. The change is deterministic and corrects an observable failure mode (the hang). The fix is localized to the streaming logic.
Related Code:
case "finish":
log.info("stream finish event received")
finished = true
break📁 File: src/mcp/index.ts
Adding a timeout to client.listTools() is an important robustness fix to prevent the entire tools-fetching operation from hanging indefinitely on unresponsive MCP servers. This prevents a caller (like the TUI) from being blocked. The timeout logic correctly respects a hierarchy of configurations (client-specific, experimental global, default), aligning with the existing architectural pattern for MCP timeouts.
Related Code:
const timeout = (isMcpConfigured(mcpEntry) ? mcpEntry.timeout : undefined) ?? defaultTimeout ?? DEFAULT_TIMEOUT
const toolsResult = await withTimeout(client.listTools(), timeout).catch((e) => {📁 File: src/storage/db.ts
The change wraps fire-and-forget effect promises with a .catch() handler to log errors. Previously, an unhandled rejection in any of these effects would crash the Node.js process or bubble up silently. This is a defensive programming improvement that increases operational visibility and prevents silent crashes from background tasks. It correctly uses Promise.resolve() to handle both synchronous and asynchronous effects.
Related Code:
for (const effect of effects) Promise.resolve(effect()).catch((e) => log.error("effect failed", { error: e }))📁 File: src/cli/cmd/tui/context/sdk.tsx
Speculative: Increasing the event batching window from 16ms to 50ms aims to reduce render churn during high-throughput streaming (like LLM token deltas). This change should be evaluated in the context of the related PR #3, which refactors sync.tsx to use more efficient setStore updates. The risk is that a 50ms delay could make the TUI feel less responsive for non-streaming UI events. The impact depends on the event volume and the efficiency gains from PR #3.
Related Code:
// If we just flushed recently (within 50ms), batch this with future events
// Otherwise, process immediately to avoid latency
if (elapsed < 50) {
timer = setTimeout(flush, 50)
return
}
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
1 participant
Summary
processor.tswherefor await (stream.fullStream)never exited onfinishevent, causing 0% CPU hangs after streaming completessdk.tsxto reduce render frequency during high-throughput streamingclient.listTools()inmcp/index.tsto prevent indefinite hangs on unresponsive MCP serversdb.tsto prevent silent crash propagationContext
These are the non-reactive-store fixes from the original freeze investigation. The reactive store fix (sync.tsx delta coalescing) is in PR #3 as a separate concern.
Files Changed
src/session/processor.tsfor-awaitloop onfinisheventsrc/cli/cmd/tui/context/sdk.tsxsrc/mcp/index.tswithTimeout()toclient.listTools()src/storage/db.tsTesting
turbo typecheckpasses across all 18 packagesturbo buildsucceedsSummary by cubic
Fixes a streaming freeze that left sessions stuck after completion and reduces TUI render churn during high-throughput updates. Adds timeouts and guards to prevent hangs and silent errors with MCP and database effects.
for-awaitloop onfinishinsession/processor.tsto stop post-stream hangs.cli/cmd/tui/context/sdk.tsxto cut render frequency.withTimeout()aroundclient.listTools()inmcp/index.tsto avoid indefinite waits.storage/db.tswith rejection handling and logging.Written for commit 3cd4444. Summary will update on new commits.
Summary by CodeRabbit