Skip to content

[codex] Retry empty AI stream responses #576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
buger merged 2 commits into from
Jun 12, 2026
+322 −139
Merged

[codex] Retry empty AI stream responses #576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb527db
Retry empty AI stream responses
buger Jun 11, 2026
a3596c3
Address empty stream retry review
buger Jun 11, 2026
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
204 changes: 117 additions & 87 deletions npm/src/agent/ProbeAgent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1453,30 +1453,45 @@
);
}

/**
* Create a RetryManager from the agent's retry configuration.
* @param {Object} [overrides={}] - Retry option overrides
* @returns {RetryManager}
* @private
*/
_createRetryManager(overrides = {}) {
return new RetryManager({

Check warning on line 1463 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The _createRetryManager method is introduced but then duplicated inline in _executeWithVercelProvider. The method at line 1463 creates a RetryManager with the same configuration pattern as lines 1466-1472, violating DRY principles. The factory method should be used consistently.
Raw output 
Replace the inline RetryManager creation at lines 1466-1472 with a call to this._createRetryManager(). The same applies to the fallback path RetryManager creation.

Check warning on line 1463 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The _createRetryManager factory method is incomplete - it doesn't handle the fallback path's need to override maxRetries per provider. The abstraction leaks configuration details into _executeWithVercelProvider.
Raw output 
Extend _createRetryManager to handle the fallback override case: _createRetryManager({ maxRetries: config.maxRetries ?? this.retryConfig.maxRetries }) or make the method handle both cases cleanly.

Check warning on line 1463 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: performance

performance Issue 

New _createRetryManager method creates additional RetryManager instances without cleanup mechanism. The method is called in _executeWithVercelProvider (line 1467) and for fallback providers (line 1513), creating multiple instances that persist for the lifetime of the ProbeAgent. The existing cleanup() method does not clear retryManager or fallbackManager instances, causing potential memory leaks in long-running processes.
Raw output 
Add cleanup of retryManager and fallbackManager instances in the cleanup() method, or implement a dispose pattern. Consider using weak references or clearing instances when no longer needed.
maxRetries: overrides.maxRetries ?? this.retryConfig.maxRetries ?? 3,
initialDelay: overrides.initialDelay ?? this.retryConfig.initialDelay ?? 1000,
maxDelay: overrides.maxDelay ?? this.retryConfig.maxDelay ?? 30000,
backoffFactor: overrides.backoffFactor ?? this.retryConfig.backoffFactor ?? 2,
retryableErrors: overrides.retryableErrors ?? this.retryConfig.retryableErrors,
jitter: overrides.jitter ?? this.retryConfig.jitter,
debug: overrides.debug ?? this.debug
});
}

/**
* Execute streamText with Vercel AI SDK using retry/fallback logic
* @param {Object} options - streamText options
* @param {AbortController} controller - Abort controller for the operation
* @param {Function} [consumeResult] - Optional callback to consume the stream result inside the same retry budget

Check warning on line 1478 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: performance

performance Issue 

New consumeResult callback parameter adds an extra async function wrapper around the entire post-stream consumption path. This wraps result.steps, result.text, and usage resolution in an additional async layer, increasing call stack depth and adding overhead to every AI request. The callback is invoked even when not needed (consumeResult ? await consumeResult(result) : result).
Raw output 
Consider inlining the post-stream logic or using a more efficient pattern that avoids the extra async wrapper when consumeResult is not provided.
* @returns {Promise<Object>} - Stream result
* @private
*/
async _executeWithVercelProvider(options, controller) {
async _executeWithVercelProvider(options, controller, consumeResult) {
// Initialize retry manager if not already created
if (!this.retryManager) {
this.retryManager = new RetryManager({
maxRetries: this.retryConfig.maxRetries ?? 3,
initialDelay: this.retryConfig.initialDelay ?? 1000,
maxDelay: this.retryConfig.maxDelay ?? 30000,
backoffFactor: this.retryConfig.backoffFactor ?? 2,
retryableErrors: this.retryConfig.retryableErrors,
debug: this.debug
});
this.retryManager = this._createRetryManager();
}

// If no fallback manager, just use retry with current provider
if (!this.fallbackManager) {
return await this.retryManager.executeWithRetry(
() => streamText({ ...options, abortSignal: controller.signal }),
async () => {
const result = streamText({ ...options, abortSignal: controller.signal });
return consumeResult ? await consumeResult(result) : result;
},
{
provider: this.apiType,
model: this.model,
Expand All @@ -1495,7 +1510,7 @@
if (this.concurrencyLimiter) {
fallbackModel = ProbeAgent._wrapModelWithLimiter(fallbackModel, this.concurrencyLimiter, this.debug);
}
const fallbackOptions = {

Check warning on line 1513 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The fallback path RetryManager creation duplicates the configuration logic instead of reusing _createRetryManager. This creates maintenance burden and risks configuration drift.
Raw output 
Replace inline RetryManager creation with: const providerRetryManager = this._createRetryManager({ maxRetries: config.maxRetries ?? this.retryConfig.maxRetries });

Check notice on line 1513 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: performance

performance Issue 

New RetryManager instance created for each fallback provider in executeWithFallback. With multiple fallback providers and retries, this can create many RetryManager instances that accumulate statistics objects and configuration copies.
Raw output 
Consider reusing a single RetryManager instance with updated configuration, or implement a pooling mechanism for RetryManager instances.
...options,
model: fallbackModel,
abortSignal: controller.signal
Expand All @@ -1510,17 +1525,15 @@
}
}

const providerRetryManager = new RetryManager({
maxRetries: config.maxRetries ?? this.retryConfig.maxRetries ?? 3,
initialDelay: this.retryConfig.initialDelay ?? 1000,
maxDelay: this.retryConfig.maxDelay ?? 30000,
backoffFactor: this.retryConfig.backoffFactor ?? 2,
retryableErrors: this.retryConfig.retryableErrors,
debug: this.debug
const providerRetryManager = this._createRetryManager({
maxRetries: config.maxRetries ?? this.retryConfig.maxRetries
});

return await providerRetryManager.executeWithRetry(
() => streamText(fallbackOptions),
async () => {
const result = streamText(fallbackOptions);
return consumeResult ? await consumeResult(result) : result;
},
{
provider: config.provider,
model: model,
Expand Down Expand Up @@ -1671,10 +1684,11 @@
/**
* Execute streamText with retry and fallback support
* @param {Object} options - streamText options
* @returns {Promise<Object>} - streamText result
* @param {Function} [consumeResult] - Optional callback to consume the result inside the same retry attempt
* @returns {Promise<Object>} - streamText result or consumer result
* @private
*/
async streamTextWithRetryAndFallback(options) {
async streamTextWithRetryAndFallback(options, consumeResult) {
// Wrap the model with per-call concurrency gating if limiter is configured.
// This acquires/releases the slot around each individual LLM API call (doStream/doGenerate)
// instead of holding it for the entire multi-step agent session.
Expand Down Expand Up @@ -1727,6 +1741,7 @@
const useCodex = this.clientApiProvider === 'codex' || process.env.USE_CODEX === 'true';

let result;
let usedVercelProvider = false;
if (useClaudeCode || useCodex) {
try {
result = await this._tryEngineStreamPath(options, controller, timeoutState);
Expand All @@ -1747,7 +1762,12 @@

if (!result) {
// Use Vercel AI SDK with retry/fallback
result = await this._executeWithVercelProvider(options, controller);
usedVercelProvider = true;
result = await this._executeWithVercelProvider(options, controller, consumeResult);
}

if (!usedVercelProvider && result && consumeResult) {
return await consumeResult(result);
}

return result;
Expand Down Expand Up @@ -3763,8 +3783,6 @@
activeTools.delete(key);
}
};
this.events.on('toolCall', onToolCall);

// Timeout observer: separate LLM call that decides whether to extend.
// Runs independently of the main agent loop — works even when blocked by delegates.
const runTimeoutObserver = async () => {
Expand Down Expand Up @@ -4423,83 +4441,95 @@
const providerOpts = this._buildThinkingProviderOptions(maxResponseTokens);
if (providerOpts) {
streamOptions.providerOptions = providerOpts;
}

Check warning on line 4444 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The consumeResult callback wraps 80+ lines of complex logic including timeout setup, error handling, resource cleanup, and event listener management. This creates a deeply nested callback structure that's hard to reason about and debug.
Raw output 
Extract the callback body into a separate method like _consumeStreamResult(result, timeoutState, gracefulTimeoutState, negotiatedTimeoutState, onToolCall) to improve readability and testability.

Check warning on line 4444 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The toolCall event listener is registered inside the retry callback but removed in the finally block. If retry occurs, the listener could be registered multiple times before cleanup, creating potential memory leaks or duplicate event handling.
Raw output 
Move event listener registration outside the retry boundary or ensure proper cleanup between retry attempts. Consider using a singleton pattern for the listener registration.

Check warning on line 4444 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

Timeout timers (gracefulTimeoutId, hardAbortTimeoutId, negotiatedTimeoutState.softTimeoutId) are created inside the retry callback. If retry occurs, previous timers may not be properly cleaned up before new ones are created, causing resource leaks.
Raw output 
Ensure timer cleanup happens before retry attempts or move timer setup outside the retry boundary. The finally block should clear all timers even when retrying.

Check notice on line 4444 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: performance

performance Issue 

The entire executeAIRequest function is now wrapped in a retry loop via the consumeResult callback. This means all post-stream processing (timeout setup, result.steps await, result.text await, usage recording, cleanup) is executed on every retry attempt. With default maxRetries=3, this code path can execute up to 4 times (initial + 3 retries) for empty-stream failures.
Raw output 
Consider moving the timeout setup and cleanup outside the retry loop to avoid redundant operations. Only the streamText call and empty-output detection should be retried.

const executeAIRequest = async () => {
const result = await this.streamTextWithRetryAndFallback(streamOptions);

// Set up timeout timer now that streamText is running.
// streamText() returns immediately — the actual tool loop runs asynchronously
// and completes when we await result.steps/result.text below.
let gracefulTimeoutId = null;
let hardAbortTimeoutId = null;
if (this.timeoutBehavior === 'graceful' && gracefulTimeoutState && this.maxOperationTimeout > 0) {
gracefulTimeoutId = setTimeout(() => {
gracefulTimeoutState.triggered = true;
if (this.debug) {
console.log(`[DEBUG] Soft timeout after ${this.maxOperationTimeout}ms — entering wind-down mode (${gracefulTimeoutState.bonusStepsMax} bonus steps)`);
}
// Safety net: hard abort after 60s if wind-down doesn't complete
hardAbortTimeoutId = setTimeout(() => {
if (this._abortController) {
this._abortController.abort();
return await this.streamTextWithRetryAndFallback(streamOptions, async (result) => {
this.events.on('toolCall', onToolCall);

// Set up timeout timer now that streamText is running.
// streamText() returns immediately — the actual tool loop runs asynchronously
// and completes when we await result.steps/result.text below.
let gracefulTimeoutId = null;
let hardAbortTimeoutId = null;
if (this.timeoutBehavior === 'graceful' && gracefulTimeoutState && this.maxOperationTimeout > 0) {
gracefulTimeoutId = setTimeout(() => {
gracefulTimeoutState.triggered = true;
if (this.debug) {
console.log(`[DEBUG] Soft timeout after ${this.maxOperationTimeout}ms — entering wind-down mode (${gracefulTimeoutState.bonusStepsMax} bonus steps)`);
}
// Safety net: hard abort after 60s if wind-down doesn't complete
hardAbortTimeoutId = setTimeout(() => {
if (this._abortController) {
this._abortController.abort();
}
if (this.debug) {
console.log(`[DEBUG] Hard abort — wind-down safety net expired after 60s`);
}
}, 60000);
}, this.maxOperationTimeout);
}

// Negotiated timeout: run the timeout observer (separate LLM call)
if (this.timeoutBehavior === 'negotiated' && this.maxOperationTimeout > 0) {
negotiatedTimeoutState.softTimeoutId = setTimeout(() => {
if (this.debug) {
console.log(`[DEBUG] Hard abort — wind-down safety net expired after 60s`);
console.log(`[DEBUG] Soft timeout after ${this.maxOperationTimeout}ms — invoking timeout observer`);

Check notice on line 4477 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

Empty output detection for non-schema responses is a special case hardcoded in the consumption logic. This creates a fragile pattern where schema vs non-schema handling is scattered across multiple locations.
Raw output 
Consider centralizing empty output detection in a dedicated validation method that can be reused across different code paths, making the logic more explicit and testable.

Check notice on line 4477 in npm/src/agent/ProbeAgent.js

View check run for this annotation

probelabs / Visor: performance

performance Issue 

Empty-output detection check (!options.schema && (!steps || steps.length === 0) && (!finalText || !finalText.trim())) is performed AFTER awaiting result.steps and result.text. This means the async operations complete even when the result will be discarded and retried. With multiple retries, this wastes time awaiting promises that will be thrown away.
Raw output 
Consider early detection of empty streams before awaiting the full result chain, or add a timeout to the await operations to fail fast on empty responses.
}
}, 60000);
}, this.maxOperationTimeout);
}
runTimeoutObserver();
}, this.maxOperationTimeout);
}

// Negotiated timeout: run the timeout observer (separate LLM call)
if (this.timeoutBehavior === 'negotiated' && this.maxOperationTimeout > 0) {
negotiatedTimeoutState.softTimeoutId = setTimeout(() => {
if (this.debug) {
console.log(`[DEBUG] Soft timeout after ${this.maxOperationTimeout}ms — invoking timeout observer`);
try {
// Use only the last step's text as the final answer.
// result.text concatenates ALL steps (including intermediate planning text),
// but the user should only see the final answer from the last step.
const steps = await result.steps;
let finalText;
if (steps && steps.length > 1) {
// Multi-step: use last step's text (the actual answer after tool calls)
const lastStepText = steps[steps.length - 1].text;
finalText = lastStepText || await result.text;
} else {
finalText = await result.text;
}
runTimeoutObserver();
}, this.maxOperationTimeout);
}

try {
// Use only the last step's text as the final answer.
// result.text concatenates ALL steps (including intermediate planning text),
// but the user should only see the final answer from the last step.
const steps = await result.steps;
let finalText;
if (steps && steps.length > 1) {
// Multi-step: use last step's text (the actual answer after tool calls)
const lastStepText = steps[steps.length - 1].text;
finalText = lastStepText || await result.text;
} else {
finalText = await result.text;
}
// Native schema responses can legitimately carry their payload in result.output
// while text and steps stay empty. Plain text answers with no steps and no
// text are empty streams and should be retried by the active retry manager.
if (!options.schema && (!steps || steps.length === 0) && (!finalText || !finalText.trim())) {
throw Object.assign(
new Error('No output generated. Check the stream for errors.'),
{ name: 'AI_NoOutputGeneratedError' }
);
}

if (this.debug) {
console.log(`[DEBUG] streamText completed: ${steps?.length || 0} steps, finalText=${finalText?.length || 0} chars`);
}
if (this.debug) {
console.log(`[DEBUG] streamText completed: ${steps?.length || 0} steps, finalText=${finalText?.length || 0} chars`);
}

// Record final token usage
const usage = await result.usage;
if (usage) {
this.tokenCounter.recordUsage(usage, result.experimental_providerMetadata);
}
// Record final token usage
const usage = await result.usage;
if (usage) {
this.tokenCounter.recordUsage(usage, result.experimental_providerMetadata);
}

return { finalText, result };
} finally {
// Clean up graceful timeout timers
if (gracefulTimeoutId) clearTimeout(gracefulTimeoutId);
if (hardAbortTimeoutId) clearTimeout(hardAbortTimeoutId);
// Clean up negotiated timeout timer
if (negotiatedTimeoutState.softTimeoutId) clearTimeout(negotiatedTimeoutState.softTimeoutId);
// Clean up graceful stop hard abort timer
if (this._gracefulStopHardAbortId) {
clearTimeout(this._gracefulStopHardAbortId);
this._gracefulStopHardAbortId = null;
return { finalText, result };
} finally {
// Clean up graceful timeout timers
if (gracefulTimeoutId) clearTimeout(gracefulTimeoutId);
if (hardAbortTimeoutId) clearTimeout(hardAbortTimeoutId);
// Clean up negotiated timeout timer
if (negotiatedTimeoutState.softTimeoutId) clearTimeout(negotiatedTimeoutState.softTimeoutId);
// Clean up graceful stop hard abort timer
if (this._gracefulStopHardAbortId) {
clearTimeout(this._gracefulStopHardAbortId);
this._gracefulStopHardAbortId = null;
}
// Remove in-flight tool tracker
this.events.removeListener('toolCall', onToolCall);
}
// Remove in-flight tool tracker
this.events.removeListener('toolCall', onToolCall);
}
});
};

let aiResult;
Expand Down
8 changes: 6 additions & 2 deletions npm/src/agent/RetryManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
'ECONNRESET',
'ETIMEDOUT',
'ENOTFOUND',
'api_error'
'api_error',
'No output generated',
'AI_NoOutputGeneratedError'
];

/**
Expand All @@ -39,6 +41,7 @@

const errorString = error.toString().toLowerCase();
const errorMessage = (error.message || '').toLowerCase();
const errorName = (error.name || error.constructor?.name || '').toLowerCase();
const errorCode = (error.code || '').toLowerCase();
const errorType = (error.type || '').toLowerCase();
const statusCode = error.statusCode || error.status;
Expand All @@ -49,6 +52,7 @@

if (errorString.includes(lowerPattern) ||
errorMessage.includes(lowerPattern) ||
errorName.includes(lowerPattern) ||
errorCode.includes(lowerPattern) ||
errorType.includes(lowerPattern) ||
statusCode?.toString() === pattern) {
Expand Down Expand Up @@ -106,7 +110,7 @@
this.backoffFactor = this._validateNumber(options.backoffFactor, 2, 'backoffFactor', 1, 10);
this.retryableErrors = options.retryableErrors || DEFAULT_RETRYABLE_ERRORS;
this.debug = options.debug ?? false;
this.jitter = options.jitter ?? true; // Add random jitter by default
this.jitter = typeof options.jitter === 'boolean' ? options.jitter : true; // Add random jitter by default

Check notice on line 113 in npm/src/agent/RetryManager.js

View check run for this annotation

probelabs / Visor: performance

performance Issue 

Jitter normalization uses typeof options.jitter === 'boolean' which is less efficient than direct boolean coercion. The jitter option is checked on every retry attempt, adding unnecessary type checking overhead.
Raw output 
Use options.jitter === false for more efficient boolean coercion, or normalize once in constructor instead of checking on every retry.

// Validate that maxDelay >= initialDelay
if (this.maxDelay < this.initialDelay) {
Expand Down
Loading
Loading