fix: make stream inactivity timeout configurable, increase for Ollama#1993
fix: make stream inactivity timeout configurable, increase for Ollama#1993octo-patch wants to merge 1 commit intoAndyMik90:developfrom
Conversation
…fixes AndyMik90#1984) Local providers like Ollama on slower hardware can take more than 60 seconds to generate the first response token. The hardcoded 60s inactivity timeout was causing tasks to silently fail and transfer to Human Review without any visible error message in the frontend logs. - Add streamInactivityTimeoutMs option to RunnerOptions so callers can override the inactivity timeout per-session - Use the configurable value in executeStream() (defaulting to 60s as before) - Set a 5-minute (300s) timeout for Ollama sessions in all three worker session runners (runSingleSession, runDefaultSession, runAgenticSpecOrchestrator) to accommodate slow inference on local hardware Co-Authored-By: Octopus <liyuan851277048@icloud.com>
|
|
📝 WalkthroughWalkthroughAdded configurable stream inactivity timeout support to the AI agent runner. Introduced Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 introduces a configurable streamInactivityTimeoutMs for AI sessions, specifically setting a five-minute timeout for the Ollama provider to accommodate slower local hardware. The review feedback suggests refactoring repeated magic numbers and hardcoded strings into constants or enums to improve maintainability, and recommends breaking a long destructuring statement in runner.ts into multiple lines for better readability.
| : undefined, | ||
| // Local providers like Ollama can take much longer than 60s to generate a | ||
| // response on slower hardware. Use a 5-minute timeout instead of the default. | ||
| streamInactivityTimeoutMs: baseSession.provider === 'ollama' ? 300_000 : undefined, |
There was a problem hiding this comment.
The magic number 300_000 (representing a 5-minute timeout) and the hardcoded string 'ollama' are repeated in three different locations within this file (lines 346, 523, and 1120). This duplication makes the code harder to maintain and prone to inconsistency if the timeout needs to be adjusted in the future. Consider defining a constant for the Ollama inactivity timeout and using the SupportedProvider.Ollama enum instead of a string literal.
| options: RunnerOptions = {}, | ||
| ): Promise<SessionResult> { | ||
| const { onEvent, onAuthRefresh, onModelRefresh, tools, memoryContext, onAccountSwitch, currentAccountId } = options; | ||
| const { onEvent, onAuthRefresh, onModelRefresh, tools, memoryContext, onAccountSwitch, currentAccountId, streamInactivityTimeoutMs } = options; |
There was a problem hiding this comment.
This line has become excessively long and difficult to read due to the addition of streamInactivityTimeoutMs. Breaking the destructuring of options into multiple lines would improve maintainability and follow common TypeScript style conventions.
const {
onEvent,
onAuthRefresh,
onModelRefresh,
tools,
memoryContext,
onAccountSwitch,
currentAccountId,
streamInactivityTimeoutMs,
} = options;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/ai/session/runner.ts (1)
159-169:⚠️ Potential issue | 🟠 MajorRecompute inactivity timeout after provider account-switch retries.
streamInactivityTimeoutMsis captured once from initial options and reused on retries. IfonAccountSwitchchanges provider (for example to Ollama),executeStream()still uses the old timeout, which can reintroduce premature stream timeouts.💡 Proposed fix
export async function runAgentSession( config: SessionConfig, options: RunnerOptions = {}, ): Promise<SessionResult> { const { onEvent, onAuthRefresh, onModelRefresh, tools, memoryContext, onAccountSwitch, currentAccountId, streamInactivityTimeoutMs } = options; const startTime = Date.now(); let authRetries = 0; let activeConfig = config; let activeAccountId = currentAccountId; + let activeStreamInactivityTimeoutMs = streamInactivityTimeoutMs; // Retry loop for auth refresh and account switching while (authRetries <= MAX_AUTH_RETRIES) { try { - const result = await executeStream(activeConfig, tools, onEvent, memoryContext, streamInactivityTimeoutMs); + const result = await executeStream(activeConfig, tools, onEvent, memoryContext, activeStreamInactivityTimeoutMs); return { ...result, durationMs: Date.now() - startTime, }; } catch (error: unknown) { @@ if (newAuth) { @@ activeAccountId = newAuth.accountId; + if (streamInactivityTimeoutMs == null) { + activeStreamInactivityTimeoutMs = + newAuth.resolvedProvider === 'ollama' ? 300_000 : undefined; + } continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ai/session/runner.ts` around lines 159 - 169, The loop captures streamInactivityTimeoutMs once from options causing executeStream to keep using the stale timeout across account-switch retries; before each call to executeStream (inside the auth retry loop) re-read/recompute the timeout from the current options/provider (i.e., get options.streamInactivityTimeoutMs or derive it from the updated provider after onAccountSwitch) and pass that fresh value into executeStream so timeouts reflect any provider/account change (symbols to update: streamInactivityTimeoutMs, options, executeStream, onAccountSwitch, activeAccountId, activeConfig, authRetries).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ai/agent/worker.ts`:
- Around line 344-346: Refactor the duplicated Ollama timeout check by
extracting a single constant and/or helper (e.g., OLLAMA_STREAM_TIMEOUT_MS =
300_000 and a small helper getProviderStreamInactivityTimeout(provider) or
isOllamaProvider(provider)) and replace each inline occurrence of
baseSession.provider === 'ollama' ? 300_000 : undefined (including places that
set streamInactivityTimeoutMs) with a call to that helper or the constant
lookup; update the three duplicated sites so they all reference the new symbol
(streamInactivityTimeoutMs assignments should use the helper/constant) to
centralize future policy changes.
---
Outside diff comments:
In `@apps/desktop/src/main/ai/session/runner.ts`:
- Around line 159-169: The loop captures streamInactivityTimeoutMs once from
options causing executeStream to keep using the stale timeout across
account-switch retries; before each call to executeStream (inside the auth retry
loop) re-read/recompute the timeout from the current options/provider (i.e., get
options.streamInactivityTimeoutMs or derive it from the updated provider after
onAccountSwitch) and pass that fresh value into executeStream so timeouts
reflect any provider/account change (symbols to update:
streamInactivityTimeoutMs, options, executeStream, onAccountSwitch,
activeAccountId, activeConfig, authRetries).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3edb350a-d380-44f9-b9cb-0e1c4f5f991c
📒 Files selected for processing (2)
apps/desktop/src/main/ai/agent/worker.tsapps/desktop/src/main/ai/session/runner.ts
| // Local providers like Ollama can take much longer than 60s to generate a | ||
| // response on slower hardware. Use a 5-minute timeout instead of the default. | ||
| streamInactivityTimeoutMs: baseSession.provider === 'ollama' ? 300_000 : undefined, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Centralize the Ollama timeout policy to avoid divergence across callsites.
The same provider check and 300_000 literal are duplicated in three places. Extract a shared constant/helper so future timeout policy changes are one-line updates.
♻️ Proposed refactor
+const OLLAMA_STREAM_INACTIVITY_TIMEOUT_MS = 300_000;
+
+function getStreamInactivityTimeoutMs(provider: string): number | undefined {
+ return provider === 'ollama' ? OLLAMA_STREAM_INACTIVITY_TIMEOUT_MS : undefined;
+}
@@
- streamInactivityTimeoutMs: baseSession.provider === 'ollama' ? 300_000 : undefined,
+ streamInactivityTimeoutMs: getStreamInactivityTimeoutMs(baseSession.provider),
@@
- streamInactivityTimeoutMs: session.provider === 'ollama' ? 300_000 : undefined,
+ streamInactivityTimeoutMs: getStreamInactivityTimeoutMs(session.provider),
@@
- streamInactivityTimeoutMs: session.provider === 'ollama' ? 300_000 : undefined,
+ streamInactivityTimeoutMs: getStreamInactivityTimeoutMs(session.provider),Also applies to: 523-523, 1120-1120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/ai/agent/worker.ts` around lines 344 - 346, Refactor
the duplicated Ollama timeout check by extracting a single constant and/or
helper (e.g., OLLAMA_STREAM_TIMEOUT_MS = 300_000 and a small helper
getProviderStreamInactivityTimeout(provider) or isOllamaProvider(provider)) and
replace each inline occurrence of baseSession.provider === 'ollama' ? 300_000 :
undefined (including places that set streamInactivityTimeoutMs) with a call to
that helper or the constant lookup; update the three duplicated sites so they
all reference the new symbol (streamInactivityTimeoutMs assignments should use
the helper/constant) to centralize future policy changes.
2 participants
Fixes #1984
Problem
When using Auto Claude with local Ollama models on slower hardware, tasks silently fail and transfer to Human Review without any error message shown in the frontend logs. This happens because:
STREAM_INACTIVITY_TIMEOUT_MSwas hardcoded at 60 seconds inrunner.tsSolution
streamInactivityTimeoutMsfield toRunnerOptionsso callers can override the inactivity timeout per-session (defaults to the existing 60s for backward compatibility)executeStream()instead of using the hardcoded constantrunSingleSession,runDefaultSession,runAgenticSpecOrchestrator), detect the Ollama provider and pass a 5-minute (300s) timeout to accommodate slow local inferenceThe 60s default is preserved for cloud providers where 60s without a response almost certainly indicates a real problem. The 5-minute timeout for Ollama gives local inference time to complete on slower hardware without hanging indefinitely.
Testing
streamInactivityTimeoutMsparameter flows correctly:RunnerOptions→runAgentSession()→executeStream()with proper default fallbackSummary by CodeRabbit
Release Notes