Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apps/desktop/src/main/ai/agent/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,9 @@ async function runSingleSession(
modelId: phaseModelId,
})
: 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,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Comment on lines +344 to +346
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

};

let sessionResult: SessionResult;
Expand Down Expand Up @@ -517,6 +520,7 @@ async function runDefaultSession(
modelId: session.modelId,
})
: undefined,
streamInactivityTimeoutMs: session.provider === 'ollama' ? 300_000 : undefined,
}, {
contextWindowLimit,
apiKey: session.apiKey,
Expand Down Expand Up @@ -1113,6 +1117,7 @@ async function runAgenticSpecOrchestrator(
modelId: session.modelId,
})
: undefined,
streamInactivityTimeoutMs: session.provider === 'ollama' ? 300_000 : undefined,
}, {
contextWindowLimit,
apiKey: session.apiKey,
Expand Down
18 changes: 13 additions & 5 deletions apps/desktop/src/main/ai/session/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,13 @@ export interface RunnerOptions {
onAccountSwitch?: (failedAccountId: string, error: SessionError) => Promise<QueueResolvedAuth | null>;
/** Current account ID from the priority queue (needed for account-switch retry) */
currentAccountId?: string;
/**
* Stream inactivity timeout in milliseconds. If no stream parts arrive within
* this period, the stream is aborted. Defaults to 60 seconds. Local providers
* (e.g., Ollama) may need a higher value since model inference can take much
* longer than cloud providers.
*/
streamInactivityTimeoutMs?: number;
}

// =============================================================================
Expand All @@ -149,7 +156,7 @@ export async function runAgentSession(
config: SessionConfig,
options: RunnerOptions = {},
): Promise<SessionResult> {
const { onEvent, onAuthRefresh, onModelRefresh, tools, memoryContext, onAccountSwitch, currentAccountId } = options;
const { onEvent, onAuthRefresh, onModelRefresh, tools, memoryContext, onAccountSwitch, currentAccountId, streamInactivityTimeoutMs } = options;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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;

const startTime = Date.now();

let authRetries = 0;
Expand All @@ -159,7 +166,7 @@ export async function runAgentSession(
// Retry loop for auth refresh and account switching
while (authRetries <= MAX_AUTH_RETRIES) {
try {
const result = await executeStream(activeConfig, tools, onEvent, memoryContext);
const result = await executeStream(activeConfig, tools, onEvent, memoryContext, streamInactivityTimeoutMs);
return {
...result,
durationMs: Date.now() - startTime,
Expand Down Expand Up @@ -265,6 +272,7 @@ async function executeStream(
tools: Record<string, AITool> | undefined,
onEvent: SessionEventCallback | undefined,
memoryContext: MemorySessionContext | undefined,
inactivityTimeoutMs: number = STREAM_INACTIVITY_TIMEOUT_MS,
): Promise<Omit<SessionResult, 'durationMs'>> {
const baseMaxSteps = config.maxSteps ?? DEFAULT_MAX_STEPS;

Expand Down Expand Up @@ -472,14 +480,14 @@ async function executeStream(
});

// Consume the full stream with inactivity timeout protection.
// The timer fires if no stream parts arrive within STREAM_INACTIVITY_TIMEOUT_MS,
// The timer fires if no stream parts arrive within inactivityTimeoutMs,
// aborting the stream and preventing indefinite worker hangs.
let streamInactivityTimer: ReturnType<typeof setTimeout> | null = null;
const resetStreamInactivityTimer = () => {
if (streamInactivityTimer) clearTimeout(streamInactivityTimer);
streamInactivityTimer = setTimeout(() => {
streamInactivityController.abort(STREAM_INACTIVITY_REASON);
}, STREAM_INACTIVITY_TIMEOUT_MS);
}, inactivityTimeoutMs);
};

resetStreamInactivityTimer(); // Arm for initial response
Expand All @@ -503,7 +511,7 @@ async function executeStream(
usage: summary.usage,
error: {
code: 'stream_timeout',
message: `Stream inactivity timeout — no data received from provider for ${STREAM_INACTIVITY_TIMEOUT_MS / 1000}s`,
message: `Stream inactivity timeout — no data received from provider for ${inactivityTimeoutMs / 1000}s`,
retryable: true,
},
messages,
Expand Down