Skip to content
Closed
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
20 changes: 16 additions & 4 deletions packages/core/src/core/geminiChat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ import {
collectToolCallIdsFromHistory,
normalizeModelToolCallIds,
} from './toolCallIdUtils.js';
import { createStreamIdleWatchdog, linkAbortSignal, type StreamIdleWatchdog, type InvalidStreamErrorType } from './streamIdleWatchdog.js';

const debugLogger = createDebugLogger('QWEN_CODE_CHAT');

Expand Down Expand Up @@ -993,9 +994,9 @@ function stripThoughtPartsFromContent(content: Content): Content | null {
* which should trigger a retry.
*/
export class InvalidStreamError extends Error {
readonly type: 'NO_FINISH_REASON' | 'NO_RESPONSE_TEXT';
readonly type: InvalidStreamErrorType;

constructor(message: string, type: 'NO_FINISH_REASON' | 'NO_RESPONSE_TEXT') {
constructor(message: string, type: InvalidStreamErrorType) {
super(message);
this.name = 'InvalidStreamError';
this.type = type;
Expand Down Expand Up @@ -2605,12 +2606,15 @@ export class GeminiChat {
params: SendMessageParameters,
prompt_id: string,
): Promise<AsyncGenerator<GenerateContentResponse>> {
const streamAbortController = new AbortController();
const cleanupAbortLink = linkAbortSignal(params.config?.abortSignal, streamAbortController);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Abort event listener leak — cleanupAbortLink is never called

linkAbortSignal adds an 'abort' listener to the caller's AbortSignal. The returned cleanup function is stored as cleanupAbortLink but is only called in the dead catch block at line 2671 (see other comment). On the normal completion path, the listener is never removed.

Each stream call in a long-lived session leaks one listener on the parent signal. With enough turns, this triggers MaxListenersExceededWarning and unbounded listener growth.

Fix: Call _cleanupAbortLink?.() from a finally block inside processStreamResponse's generator body, ensuring it runs on all exit paths.

— qwen3.7-max via Qwen Code /review

const streamWatchdog = createStreamIdleWatchdog(model, streamAbortController);
const apiCall = () =>
this.config.getContentGenerator().generateContentStream(
{
model,
contents: requestContents,
config: { ...this.generationConfig, ...params.config },
config: { ...this.generationConfig, ...params.config, abortSignal: streamAbortController.signal },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Existing test failure — config: {} assertion no longer matches

This line injects abortSignal: streamAbortController.signal into the config passed to generateContentStream. The existing test at geminiChat.test.ts:1476 asserts config: {}, which now fails because the actual config is { abortSignal: AbortSignal{...} }.

CI confirms: Test fails on all 3 platforms (macOS, Windows, Linux).

Suggested change
config: { ...this.generationConfig, ...params.config, abortSignal: streamAbortController.signal },
config: { ...this.generationConfig, ...params.config, abortSignal: streamAbortController.signal },

Update the test to use an asymmetric matcher:

config: { abortSignal: expect.any(AbortSignal) },

— qwen3.7-max via Qwen Code /review

},
prompt_id,
);
Expand Down Expand Up @@ -2660,7 +2664,13 @@ export class GeminiChat {
},
});

return this.processStreamResponse(model, streamResponse);
try {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Dead try/catch around an async generator — cleanup is unreachable

processStreamResponse is declared async *, so calling it returns an AsyncGenerator object synchronously without executing any function body. Errors during stream iteration happen in the caller's for await loop, not here. The catch block (which contains the only calls to streamWatchdog.cleanup() and cleanupAbortLink()) can never fire.

This means cleanup of both the watchdog timers and the abort-signal listener never runs on any exit path — not on normal completion, not on error, not on caller abort.

Fix: Remove this try/catch and move cleanup into a try/finally inside processStreamResponse's generator body (wrapping the for await loop). Generator finally blocks run on .return(), .throw(), and normal completion.

— qwen3.7-max via Qwen Code /review

return this.processStreamResponse(model, streamResponse, streamWatchdog, cleanupAbortLink);
} catch (error) {
streamWatchdog?.cleanup();
cleanupAbortLink?.();
throw error;
}
}

/**
Expand Down Expand Up @@ -2964,6 +2974,8 @@ export class GeminiChat {
private async *processStreamResponse(
model: string,
streamResponse: AsyncGenerator<GenerateContentResponse>,
_streamWatchdog?: StreamIdleWatchdog,
_cleanupAbortLink?: () => void,
): AsyncGenerator<GenerateContentResponse> {
// Collect ALL parts from the model response (including thoughts for recording)
const allModelParts: Part[] = [];
Expand Down
79 changes: 79 additions & 0 deletions packages/core/src/core/streamIdleWatchdog.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { createDebugLogger } from '../utils/debugLogger.js';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] No test file for this new module

This module exports 4 functions with non-trivial logic: env var parsing, timer management via Promise.race, abort signal linking (with pre-aborted signal handling), and cleanup. No streamIdleWatchdog.test.ts exists.

Key untested scenarios:

  • isStreamWatchdogDisabled: env var '1', 'true', absent, other values
  • getStreamIdleTimeoutMs: disabled path, invalid values ('abc', '-1', '0')
  • linkAbortSignal: undefined signal, already-aborted signal, cleanup removing listener
  • createStreamIdleWatchdog.next(): timeout fires, promise resolves first, cleanup clears timers

— qwen3.7-max via Qwen Code /review

import { InvalidStreamError } from './geminiChat.js';

const debugLogger = createDebugLogger('QWEN_CODE_WATCHDOG');

export const DEFAULT_STREAM_IDLE_TIMEOUT_MS = 90_000;
const STREAM_IDLE_WARNING_FRACTION = 0.5;

export type InvalidStreamErrorType =
| 'NO_FINISH_REASON'
| 'NO_RESPONSE_TEXT'
| 'STREAM_IDLE_TIMEOUT';

export interface StreamIdleWatchdog {
next<T>(nextPromise: Promise<IteratorResult<T>>): Promise<IteratorResult<T>>;
cleanup(): void;
}

export function isStreamWatchdogDisabled(): boolean {
const value = process.env['QWEN_CODE_DISABLE_STREAM_WATCHDOG'];
return value === '1' || value === 'true';
}

export function getStreamIdleTimeoutMs(): number | undefined {
if (isStreamWatchdogDisabled()) return undefined;
const value = process.env['QWEN_CODE_STREAM_IDLE_TIMEOUT_MS'];
if (!value) return DEFAULT_STREAM_IDLE_TIMEOUT_MS;
const parsed = Number(value);
if (!Number.isFinite(parsed) || parsed <= 0) {
debugLogger.warn('Ignoring invalid STREAM_IDLE_TIMEOUT_MS: ' + value);
return DEFAULT_STREAM_IDLE_TIMEOUT_MS;
}
return parsed;
}

export function linkAbortSignal(
signal: AbortSignal | undefined,
controller: AbortController,
): () => void {
if (!signal) return () => {};
if (signal.aborted) { controller.abort(signal.reason); return () => {}; }
const onAbort = () => controller.abort(signal.reason);
signal.addEventListener('abort', onAbort, { once: true });
return () => signal.removeEventListener('abort', onAbort);
}

export function createStreamIdleWatchdog(
model: string,
abortController: AbortController,
): StreamIdleWatchdog | undefined {
const timeoutMs = getStreamIdleTimeoutMs();
if (timeoutMs === undefined) return undefined;
const warningMs = Math.max(1, Math.floor(timeoutMs * STREAM_IDLE_WARNING_FRACTION));
let timeoutId: ReturnType<typeof setTimeout> | undefined;
let warningId: ReturnType<typeof setTimeout> | undefined;
const clearTimers = () => {
if (timeoutId !== undefined) { clearTimeout(timeoutId); timeoutId = undefined; }
if (warningId !== undefined) { clearTimeout(warningId); warningId = undefined; }
};
return {
next<T>(nextPromise: Promise<IteratorResult<T>>): Promise<IteratorResult<T>> {
const timeoutPromise = new Promise<never>((_, reject) => {
warningId = setTimeout(() => {
debugLogger.warn('Stream idle for ' + warningMs + 'ms from ' + model);
}, warningMs);
timeoutId = setTimeout(() => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] abortController.abort() called without a reason argument

When the watchdog timeout fires, abort() is called with no reason. Downstream consumers that inspect signal.reason (retry predicates, error classifiers, telemetry) see undefined instead of the InvalidStreamError that is simultaneously thrown via reject().

Suggested change
timeoutId = setTimeout(() => {
const err = new InvalidStreamError(
'Stream idle timeout after ' + timeoutMs + 'ms',
'STREAM_IDLE_TIMEOUT',
);
abortController.abort(err);
reject(err);

This ensures signal.reason matches the thrown error and propagates correctly through linkAbortSignal's controller.abort(signal.reason) chain.

— qwen3.7-max via Qwen Code /review

clearTimers();
abortController.abort();
reject(new InvalidStreamError(
'Stream idle timeout after ' + timeoutMs + 'ms',
'STREAM_IDLE_TIMEOUT',
));
}, timeoutMs);
});
return Promise.race([nextPromise, timeoutPromise]).finally(clearTimers);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Unhandled promise rejection when timeout wins the race

When the watchdog timeout fires before nextPromise settles, Promise.race rejects with the timeout error. However, nextPromise (the underlying stream's .next() call) is still pending. The timeout handler calls abortController.abort(), which causes the stream to eventually reject. Since Promise.race already settled, the nextPromise rejection has no handler attached — triggering an unhandled promise rejection warning (or crash in strict mode).

Suggested change
return Promise.race([nextPromise, timeoutPromise]).finally(clearTimers);
return Promise.race([nextPromise, timeoutPromise]).finally(clearTimers);
},

Should be:

    next<T>(nextPromise: Promise<IteratorResult<T>>): Promise<IteratorResult<T>> {
      nextPromise.catch(() => {}); // prevent unhandled rejection if timeout wins
      const timeoutPromise = new Promise<never>((_, reject) => {
        // ...
      });
      return Promise.race([nextPromise, timeoutPromise]).finally(clearTimers);
    },

The .catch(() => {}) doesn't prevent Promise.race from seeing the original rejection — it just silences the unhandled rejection when the losing promise later rejects.

— qwen3.7-max via Qwen Code /review

},
cleanup() { clearTimers(); },
};
}
Loading