mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
fix(daemon): stop misclassifying signals/interrupts as retryable timeout (#3980)
`classifyRunFailure` mapped every `AGENT_SIGNAL_*` errorCode to category=timeout / stage=first_token_wait / retryable=true. That laundered OOM kills (SIGKILL), process crashes (SIGSEGV/SIGABRT/SIGILL/SIGTRAP/SIGBUS), daemon shutdowns/cancels (SIGTERM), and interruptions (SIGINT / exit 130) into a transient timeout. The safe-retry policy then auto-respawned the run from scratch — and an OOM/crash near-certainly reproduces, so we burned a full retry for nothing. Exit code 130 (128+SIGINT, used by AMR/vela to mean "request cancelled / upstream stream aborted") also fell into the generic `exit_code` bucket, mixed with arbitrary non-zero exits. - contracts: add failure details `signal_killed`, `process_crashed`, `interrupted`. - classifier: drop `AGENT_SIGNAL_*` from the timeout condition (genuine inactivity timeouts still match via isTimeoutText / TIMEOUT) and add an explicit signal/interrupt branch before the generic AGENT_EXIT bucket: SIGKILL -> signal_killed (non-retryable); crash signals -> process_crashed (non-retryable); SIGINT / exit 130 -> stream_disconnected when the text names a disconnect, else interrupted (non-retryable); SIGTERM -> timeout only when the text is inactivity (handled by the earlier timeout branch), else terminated_unknown (non-retryable). All land as category=process_exit, which is already outside the retry policy's transient set, so none are auto-retried. Red-then-green: the new signal/interrupt specs in run-failure-classification.test.ts go red on origin/main (SIGKILL -> timeout retryable=true, exit 130 -> exit_code, plain SIGTERM -> timeout) and green on this branch. Co-authored-by: audit <a@b.c>
This commit is contained in:
@@ -194,6 +194,64 @@ function upstreamDetail(text: string): TrackingRunFailureDetail {
|
||||
return 'network_error';
|
||||
}
|
||||
|
||||
// Signals that mean the agent process aborted abnormally (segfault, abort,
|
||||
// illegal instruction, trap, bus error). Distinct from SIGKILL (OOM / forced
|
||||
// kill) and SIGTERM (graceful shutdown / cancel). None of these are timeouts.
|
||||
const PROCESS_CRASH_SIGNALS = new Set([
|
||||
'SIGSEGV',
|
||||
'SIGABRT',
|
||||
'SIGILL',
|
||||
'SIGTRAP',
|
||||
'SIGBUS',
|
||||
]);
|
||||
|
||||
// Classifies a run that died from an OS signal or an interrupt exit code
|
||||
// (130 = 128 + SIGINT). Returns null when the failure is not signal/interrupt
|
||||
// shaped so the caller can fall through to the generic exit-code bucket.
|
||||
//
|
||||
// Earlier classifier branches already claim the cases where the failure text
|
||||
// carries richer meaning than the bare signal: an inactivity-driven SIGTERM is
|
||||
// caught by the timeout branch above, and a SIGINT/exit-130 whose text names a
|
||||
// stream disconnect is caught by the upstream branch. By the time control
|
||||
// reaches here a signal is the strongest evidence we have, so map it to a
|
||||
// non-retryable process_exit instead of laundering it into a retryable timeout.
|
||||
function signalInterruptClassification(
|
||||
errorCode: string,
|
||||
text: string,
|
||||
retryableHint: boolean | undefined,
|
||||
): RunFailureClassification | null {
|
||||
const isInterruptExit = errorCode === 'AGENT_EXIT_130';
|
||||
const signal = errorCode.startsWith('AGENT_SIGNAL_')
|
||||
? errorCode.slice('AGENT_SIGNAL_'.length)
|
||||
: '';
|
||||
if (!signal && !isInterruptExit) return null;
|
||||
|
||||
if (signal === 'SIGKILL') {
|
||||
return classification('process_exit', 'signal_killed', 'child_close', false, 'none');
|
||||
}
|
||||
if (PROCESS_CRASH_SIGNALS.has(signal)) {
|
||||
return classification('process_exit', 'process_crashed', 'child_close', false, 'none');
|
||||
}
|
||||
if (signal === 'SIGINT' || isInterruptExit) {
|
||||
// Defensive: the upstream branch above already claims disconnect text, but
|
||||
// re-check so a reordering can never silently bury a cancelled stream.
|
||||
if (isUpstreamDetailText(text)) {
|
||||
return classification(
|
||||
'upstream_unavailable',
|
||||
upstreamDetail(text),
|
||||
'first_token_wait',
|
||||
retryableHint ?? true,
|
||||
'retry',
|
||||
);
|
||||
}
|
||||
return classification('process_exit', 'interrupted', 'child_close', false, 'none');
|
||||
}
|
||||
// SIGTERM (graceful shutdown / cancel) and any other signal. Inactivity-driven
|
||||
// SIGTERMs were already claimed by the timeout branch above, so reaching here
|
||||
// means there is no timeout evidence: treat as a non-retryable termination.
|
||||
return classification('process_exit', 'terminated_unknown', 'child_close', false, 'none');
|
||||
}
|
||||
|
||||
function processExitDetail(
|
||||
errorCode: string,
|
||||
text: string,
|
||||
@@ -354,11 +412,7 @@ export function classifyRunFailure(
|
||||
);
|
||||
}
|
||||
|
||||
if (
|
||||
isTimeoutText(text) ||
|
||||
errorCode === 'TIMEOUT' ||
|
||||
errorCode.startsWith('AGENT_SIGNAL_')
|
||||
) {
|
||||
if (isTimeoutText(text) || errorCode === 'TIMEOUT') {
|
||||
const retryable = retryableHint ?? true;
|
||||
return classification(
|
||||
'timeout',
|
||||
@@ -392,6 +446,9 @@ export function classifyRunFailure(
|
||||
);
|
||||
}
|
||||
|
||||
const signalInterrupt = signalInterruptClassification(errorCode, text, retryableHint);
|
||||
if (signalInterrupt) return signalInterrupt;
|
||||
|
||||
if (
|
||||
errorCode.startsWith('AGENT_EXIT_') ||
|
||||
errorCode === 'AGENT_TERMINATED_UNKNOWN' ||
|
||||
|
||||
@@ -468,3 +468,131 @@ describe('classifyRunFailure', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// Regression coverage for the signal/interrupt mis-attribution bug: process
|
||||
// crashes (SIGKILL/SIGSEGV/...), daemon shutdowns (SIGTERM), and cancellations
|
||||
// (SIGINT / exit 130) must NOT be laundered into a retryable `timeout`. Each
|
||||
// has a non-retryable home so the safe-retry policy never re-spawns an
|
||||
// OOM/crash that is near-certain to reproduce.
|
||||
describe('classifyRunFailure — signal and interrupt attribution', () => {
|
||||
function classifySignal(
|
||||
signal: string,
|
||||
message = '',
|
||||
exitCode: number | null = null,
|
||||
) {
|
||||
const errorCode = `AGENT_SIGNAL_${signal}`;
|
||||
return classifyRunFailure({
|
||||
result: 'failed',
|
||||
status: {
|
||||
status: 'failed',
|
||||
error: message || null,
|
||||
signal,
|
||||
exitCode,
|
||||
errorCode: null,
|
||||
},
|
||||
errorCode,
|
||||
agentId: 'claude',
|
||||
events: [],
|
||||
});
|
||||
}
|
||||
|
||||
function classifyExit(
|
||||
exitCode: number,
|
||||
message = '',
|
||||
) {
|
||||
const errorCode = `AGENT_EXIT_${exitCode}`;
|
||||
return classifyRunFailure({
|
||||
result: 'failed',
|
||||
status: {
|
||||
status: 'failed',
|
||||
error: message || null,
|
||||
signal: null,
|
||||
exitCode,
|
||||
errorCode: null,
|
||||
},
|
||||
errorCode,
|
||||
agentId: 'claude',
|
||||
events: [],
|
||||
});
|
||||
}
|
||||
|
||||
it('classifies SIGKILL as a non-retryable process kill, not a timeout', () => {
|
||||
expect(classifySignal('SIGKILL', 'Killed')).toMatchObject({
|
||||
failure_category: 'process_exit',
|
||||
failure_detail: 'signal_killed',
|
||||
retryable: false,
|
||||
user_action: 'none',
|
||||
});
|
||||
});
|
||||
|
||||
it('classifies hard crash signals as non-retryable process crashes', () => {
|
||||
for (const signal of ['SIGSEGV', 'SIGABRT', 'SIGILL', 'SIGTRAP', 'SIGBUS']) {
|
||||
expect(classifySignal(signal, 'core dumped')).toMatchObject({
|
||||
failure_category: 'process_exit',
|
||||
failure_detail: 'process_crashed',
|
||||
retryable: false,
|
||||
user_action: 'none',
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
it('classifies SIGINT as a non-retryable interruption', () => {
|
||||
expect(classifySignal('SIGINT', 'Interrupted')).toMatchObject({
|
||||
failure_category: 'process_exit',
|
||||
failure_detail: 'interrupted',
|
||||
retryable: false,
|
||||
user_action: 'none',
|
||||
});
|
||||
});
|
||||
|
||||
it('classifies exit code 130 (128+SIGINT) as a non-retryable interruption', () => {
|
||||
expect(classifyExit(130, 'Request was cancelled')).toMatchObject({
|
||||
failure_category: 'process_exit',
|
||||
failure_detail: 'interrupted',
|
||||
retryable: false,
|
||||
user_action: 'none',
|
||||
});
|
||||
});
|
||||
|
||||
it('routes an interrupted run whose text names a stream disconnect to upstream', () => {
|
||||
expect(
|
||||
classifyExit(130, 'stream disconnected before completion'),
|
||||
).toMatchObject({
|
||||
failure_category: 'upstream_unavailable',
|
||||
failure_detail: 'stream_disconnected',
|
||||
retryable: true,
|
||||
user_action: 'retry',
|
||||
});
|
||||
expect(
|
||||
classifySignal('SIGINT', 'Upstream request failed; aborting stream'),
|
||||
).toMatchObject({
|
||||
failure_category: 'upstream_unavailable',
|
||||
retryable: true,
|
||||
user_action: 'retry',
|
||||
});
|
||||
});
|
||||
|
||||
it('classifies a plain SIGTERM (not inactivity) as a non-retryable termination', () => {
|
||||
expect(classifySignal('SIGTERM', 'Terminated')).toMatchObject({
|
||||
failure_category: 'process_exit',
|
||||
failure_detail: 'terminated_unknown',
|
||||
retryable: false,
|
||||
user_action: 'none',
|
||||
});
|
||||
});
|
||||
|
||||
it('still classifies an inactivity-driven SIGTERM as a retryable timeout', () => {
|
||||
expect(
|
||||
classifySignal(
|
||||
'SIGTERM',
|
||||
'Agent stalled without emitting any new output for 120s.',
|
||||
),
|
||||
).toMatchObject({
|
||||
failure_category: 'timeout',
|
||||
failure_detail: 'inactivity_timeout',
|
||||
failure_stage: 'first_token_wait',
|
||||
retryable: true,
|
||||
user_action: 'retry',
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -187,4 +187,23 @@ describe('decideSafeRunRetry', () => {
|
||||
retrySuppressedReason: 'cancel_requested',
|
||||
});
|
||||
});
|
||||
|
||||
it('never auto-retries process kills, crashes, or interruptions', () => {
|
||||
for (const failure_detail of [
|
||||
'signal_killed',
|
||||
'process_crashed',
|
||||
'interrupted',
|
||||
] as const) {
|
||||
expect(
|
||||
decide({
|
||||
failure: {
|
||||
failure_category: 'process_exit',
|
||||
failure_detail,
|
||||
failure_stage: 'child_close',
|
||||
retryable: false,
|
||||
},
|
||||
}).shouldRetry,
|
||||
).toBe(false);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -264,6 +264,9 @@ export type TrackingRunFailureDetail =
|
||||
| 'agent_protocol_error'
|
||||
| 'permission_request_not_found'
|
||||
| 'qoder_stop_sequence'
|
||||
| 'signal_killed'
|
||||
| 'process_crashed'
|
||||
| 'interrupted'
|
||||
| 'exit_code'
|
||||
| 'terminated_unknown'
|
||||
| 'execution_failed'
|
||||
|
||||
Reference in New Issue
Block a user