Skip to content

Commit 393c9ba

Browse files
KirachonCopilot
andcommitted
hardening(openai): honor cancellation and deadlines
Thread AbortSignal and end-to-end deadline metadata through searchAndAsk into the Codex session provider. - terminate spawned Codex subprocesses on request abort - surface provider_aborted consistently for cancelled calls - enforce remaining deadline budget after queue wait for readiness and exec - expose provider health without throwing - add regression coverage for signal forwarding, abort handling, and deadline budgets Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 08486b2 commit 393c9ba

4 files changed

Lines changed: 273 additions & 15 deletions

File tree

src/ai/providers/codexSessionProvider.ts

Lines changed: 122 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -96,13 +96,25 @@ async function runCommand(args: {
9696
cwd: string;
9797
timeoutMs: number;
9898
stdin?: string;
99+
signal?: AbortSignal;
99100
}): Promise<SpawnResult> {
100101
const normalizedCwd =
101102
process.platform === 'win32' && args.cwd.startsWith('\\\\?\\')
102103
? args.cwd.slice(4)
103104
: args.cwd;
104105

105106
return new Promise<SpawnResult>((resolve, reject) => {
107+
if (args.signal?.aborted) {
108+
reject(
109+
new AIProviderError({
110+
code: 'provider_aborted',
111+
provider: 'openai_session',
112+
message: 'Codex session request was aborted before the subprocess started.',
113+
})
114+
);
115+
return;
116+
}
117+
106118
const proc = spawn(args.command, args.commandArgs, {
107119
cwd: normalizedCwd,
108120
shell: false,
@@ -114,9 +126,10 @@ async function runCommand(args: {
114126
let stderr = '';
115127
let settled = false;
116128
let timedOut = false;
129+
let aborted = false;
130+
const abortMessage = 'Codex session request was aborted.';
117131

118-
const timeoutId = setTimeout(() => {
119-
timedOut = true;
132+
const terminateProcess = (): void => {
120133
if (process.platform === 'win32' && typeof proc.pid === 'number') {
121134
const killer = spawn('taskkill', ['/PID', String(proc.pid), '/T', '/F'], {
122135
windowsHide: true,
@@ -129,11 +142,39 @@ async function runCommand(args: {
129142
} else {
130143
proc.kill();
131144
}
145+
};
146+
147+
const settleAbort = (message: string): void => {
148+
if (settled) return;
149+
settled = true;
150+
clearTimeout(timeoutId);
151+
args.signal?.removeEventListener('abort', onAbort);
152+
reject(
153+
new AIProviderError({
154+
code: 'provider_aborted',
155+
provider: 'openai_session',
156+
message,
157+
})
158+
);
159+
};
160+
161+
const onAbort = () => {
162+
aborted = true;
163+
terminateProcess();
164+
setTimeout(() => {
165+
settleAbort(abortMessage);
166+
}, 3_000);
167+
};
168+
169+
const timeoutId = setTimeout(() => {
170+
timedOut = true;
171+
terminateProcess();
132172

133173
// Some Windows process trees survive kill signals. Force-settle so callers don't hang.
134174
setTimeout(() => {
135175
if (settled) return;
136176
settled = true;
177+
args.signal?.removeEventListener('abort', onAbort);
137178
resolve({
138179
exitCode: 1,
139180
stdout,
@@ -153,15 +194,25 @@ async function runCommand(args: {
153194

154195
proc.on('error', (error) => {
155196
if (settled) return;
197+
if (aborted) {
198+
settleAbort(abortMessage);
199+
return;
200+
}
156201
settled = true;
157202
clearTimeout(timeoutId);
203+
args.signal?.removeEventListener('abort', onAbort);
158204
reject(error);
159205
});
160206

161207
proc.on('close', (code) => {
162208
if (settled) return;
209+
if (aborted) {
210+
settleAbort(abortMessage);
211+
return;
212+
}
163213
settled = true;
164214
clearTimeout(timeoutId);
215+
args.signal?.removeEventListener('abort', onAbort);
165216
resolve({
166217
exitCode: typeof code === 'number' ? code : 1,
167218
stdout,
@@ -170,6 +221,14 @@ async function runCommand(args: {
170221
});
171222
});
172223

224+
if (args.signal) {
225+
args.signal.addEventListener('abort', onAbort, { once: true });
226+
if (args.signal.aborted) {
227+
onAbort();
228+
return;
229+
}
230+
}
231+
173232
if (args.stdin !== undefined) {
174233
proc.stdin.write(args.stdin);
175234
}
@@ -189,6 +248,7 @@ export class CodexSessionProvider implements AIProvider {
189248
private readonly identityTtlMs: number;
190249
private identityCache: { checkedAt: number; isLoggedIn: boolean } | null = null;
191250
private selectedCommand: CommandSpec | null = null;
251+
private lastWorkspacePath: string = process.cwd();
192252

193253
constructor() {
194254
this.commandCandidates = this.buildCommandCandidates(process.env.CE_OPENAI_SESSION_CMD);
@@ -274,6 +334,7 @@ export class CodexSessionProvider implements AIProvider {
274334
cwd: string;
275335
timeoutMs: number;
276336
stdin?: string;
337+
signal?: AbortSignal;
277338
}): Promise<SpawnResult> {
278339
const candidates = this.selectedCommand ? [this.selectedCommand] : this.commandCandidates;
279340
let lastNotFoundError: unknown;
@@ -299,6 +360,7 @@ export class CodexSessionProvider implements AIProvider {
299360
cwd: args.cwd,
300361
timeoutMs: args.timeoutMs,
301362
stdin: args.stdin,
363+
signal: args.signal,
302364
});
303365
this.selectedCommand = candidate;
304366
return result;
@@ -314,7 +376,16 @@ export class CodexSessionProvider implements AIProvider {
314376
throw this.missingCommandError(lastNotFoundError);
315377
}
316378

317-
private async ensureSessionReady(workspacePath: string): Promise<void> {
379+
private async ensureSessionReady(workspacePath: string, signal?: AbortSignal): Promise<void> {
380+
const timeoutMs = this.healthcheckTimeoutMs;
381+
return this.ensureSessionReadyWithTimeout(workspacePath, timeoutMs, signal);
382+
}
383+
384+
private async ensureSessionReadyWithTimeout(
385+
workspacePath: string,
386+
timeoutMs: number,
387+
signal?: AbortSignal
388+
): Promise<void> {
318389
const now = Date.now();
319390
if (
320391
this.refreshMode === 'ttl' &&
@@ -335,14 +406,15 @@ export class CodexSessionProvider implements AIProvider {
335406
const statusResult = await this.runWithCommandFallback({
336407
commandArgs: ['login', 'status'],
337408
cwd: workspacePath,
338-
timeoutMs: this.healthcheckTimeoutMs,
409+
timeoutMs,
410+
signal,
339411
});
340412

341413
if (statusResult.timedOut) {
342414
throw new AIProviderError({
343415
code: 'provider_timeout',
344416
provider: this.id,
345-
message: `Codex login status check timed out after ${this.healthcheckTimeoutMs}ms.`,
417+
message: `Codex login status check timed out after ${timeoutMs}ms.`,
346418
retryable: true,
347419
});
348420
}
@@ -366,9 +438,37 @@ export class CodexSessionProvider implements AIProvider {
366438
this.identityCache = { checkedAt: now, isLoggedIn: true };
367439
}
368440

441+
private getRemainingBudgetMs(
442+
request: Pick<AIProviderRequest, 'timeoutMs' | 'deadlineMs'>,
443+
operation: string
444+
): number {
445+
const remainingMs =
446+
typeof request.deadlineMs === 'number'
447+
? request.deadlineMs - Date.now()
448+
: request.timeoutMs;
449+
if (!Number.isFinite(remainingMs) || remainingMs <= 0) {
450+
throw new AIProviderError({
451+
code: 'provider_timeout',
452+
provider: this.id,
453+
message: `Codex session ${operation} timed out before execution started.`,
454+
retryable: true,
455+
});
456+
}
457+
return Math.max(1, Math.floor(remainingMs));
458+
}
459+
369460
async call(request: AIProviderRequest): Promise<AIProviderResponse> {
461+
this.lastWorkspacePath = request.workspacePath;
462+
const readinessTimeoutMs = Math.min(
463+
this.healthcheckTimeoutMs,
464+
this.getRemainingBudgetMs(request, 'request')
465+
);
370466
try {
371-
await this.ensureSessionReady(request.workspacePath);
467+
await this.ensureSessionReadyWithTimeout(
468+
request.workspacePath,
469+
readinessTimeoutMs,
470+
request.signal
471+
);
372472
} catch (error) {
373473
// Some environments report inconsistent `login status` results.
374474
// Defer auth verdict to the actual `exec` call when readiness checks are inconclusive.
@@ -400,19 +500,21 @@ export class CodexSessionProvider implements AIProvider {
400500
'',
401501
request.prompt?.trim() || request.searchQuery,
402502
].join('\n');
503+
const execTimeoutMs = this.getRemainingBudgetMs(request, 'request');
403504

404505
const result = await this.runWithCommandFallback({
405506
commandArgs,
406507
cwd: request.workspacePath,
407-
timeoutMs: request.timeoutMs,
508+
timeoutMs: execTimeoutMs,
408509
stdin: composedPrompt,
510+
signal: request.signal,
409511
});
410512

411513
if (result.timedOut) {
412514
throw new AIProviderError({
413515
code: 'provider_timeout',
414516
provider: this.id,
415-
message: `Codex session request timed out after ${request.timeoutMs}ms.`,
517+
message: `Codex session request timed out after ${execTimeoutMs}ms.`,
416518
retryable: true,
417519
});
418520
}
@@ -458,4 +560,16 @@ export class CodexSessionProvider implements AIProvider {
458560
fs.rmSync(tmpDir, { recursive: true, force: true });
459561
}
460562
}
563+
564+
async health(): Promise<{ ok: boolean; reason?: string }> {
565+
try {
566+
await this.ensureSessionReady(this.lastWorkspacePath);
567+
return { ok: true };
568+
} catch (error) {
569+
return {
570+
ok: false,
571+
reason: error instanceof Error ? error.message : String(error),
572+
};
573+
}
574+
}
461575
}

src/mcp/serviceClient.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3904,6 +3904,7 @@ export class ContextServiceClient {
39043904
const timeoutCandidate = options?.timeoutMs ?? defaultTimeoutMs;
39053905
const requestedTimeoutMs = Number.isFinite(timeoutCandidate) ? timeoutCandidate : defaultTimeoutMs;
39063906
const timeoutMs = Math.max(MIN_API_TIMEOUT_MS, Math.min(MAX_API_TIMEOUT_MS, requestedTimeoutMs));
3907+
const deadlineMs = Date.now() + timeoutMs;
39073908
try {
39083909
const admissionTimeoutError = this.getQueueTimeoutAdmissionError(timeoutMs, priority);
39093910
if (admissionTimeoutError) {
@@ -3934,6 +3935,8 @@ export class ContextServiceClient {
39343935
prompt,
39353936
timeoutMs,
39363937
workspacePath: this.workspacePath,
3938+
signal: options?.signal,
3939+
deadlineMs,
39373940
});
39383941
if (!innerResponse || typeof innerResponse !== 'object' || typeof innerResponse.text !== 'string') {
39393942
throw new Error(

0 commit comments

Comments
 (0)