diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 642e2b41..0140c962 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -157,6 +157,7 @@ import { factoryDispatchFailureReasonCounts, } from './dispatch-failure-reason' import type { FactoryDispatchFailureReasonCode } from './dispatch-failure-reason' +import { isAgentAlreadyGoneOnRelease } from './release-error' import { boundedRunCostTotal, CostLedger, type RunCostTotal, type UnpricedModelCostRecord } from '../cost/ledger' import { createTicketDispatchDelivery, type TicketDispatchDelivery } from '../delivery/ticket-dispatch' import { @@ -8618,17 +8619,41 @@ export class FactoryLoop implements Factory { } if (record) await this.#reportAgent(record, tracked, 'agent.released', { releaseReason: reason }) } catch (error) { - failed.push(agentName) - this.#logger.warn?.(`[factory] failed to release ${agentName} during ${context}`, error) - if (record) { - const lifecycle = await this.#state - .getDispatchLifecycle(this.#workspaceId, dispatchLifecycleKey(record.issue)) - .catch(() => undefined) - if (lifecycle) { - await this.#reportLifecycle(lifecycle, 'factory.failure', { - level: 'error', - errorCode: 'release_failed', - }) + if (isAgentAlreadyGoneOnRelease(error)) { + // Do the same bookkeeping the success path does — the agent IS + // gone. Skipping this would leave `batch.recordRelease` unset and + // the next retry attempt would think the invocation is still + // dispatchable. + this.#increment('releaseAgentAlreadyGone') + this.#logger.info?.( + `[factory] release skipped: agent already gone during ${context}`, + { agentName, reason }, + ) + if (record && batch && context !== 'stop') { + const releasedInvocationId = batch.recordRelease(record, agentName, this.#clock.now()) + if (releasedInvocationId) { + this.#logger.debug?.('[factory] released agent invocation is no longer dispatchable', { + issue: record.issue.key, + agentName, + reason, + invocationId: releasedInvocationId, + }) + } + } + if (record) await this.#reportAgent(record, tracked, 'agent.released', { releaseReason: reason }) + } else { + failed.push(agentName) + this.#logger.warn?.(`[factory] failed to release ${agentName} during ${context}`, error) + if (record) { + const lifecycle = await this.#state + .getDispatchLifecycle(this.#workspaceId, dispatchLifecycleKey(record.issue)) + .catch(() => undefined) + if (lifecycle) { + await this.#reportLifecycle(lifecycle, 'factory.failure', { + level: 'error', + errorCode: 'release_failed', + }) + } } } } @@ -20580,6 +20605,7 @@ const isRegistrationLagInjectionError = (error: unknown): boolean => { .test(errorMessage) } + const isDispatchDeliveryError = (error: unknown): boolean => { if (isRegistrationLagInjectionError(error)) return true const { errorMessage } = describeError(error) diff --git a/src/orchestrator/release-error.test.ts b/src/orchestrator/release-error.test.ts new file mode 100644 index 00000000..4b133228 --- /dev/null +++ b/src/orchestrator/release-error.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest' + +import { isAgentAlreadyGoneOnRelease } from './release-error' + +describe('isAgentAlreadyGoneOnRelease', () => { + it('recognises the canonical Relay 404 agent_not_found shape', () => { + const relayError = { + name: 'RelayError', + message: 'Agent "ar-9-impl-sandbox" not found', + code: 'not_found', + retryable: false, + statusCode: 404, + rawCode: 'agent_not_found', + status: 404, + } + expect(isAgentAlreadyGoneOnRelease(relayError)).toBe(true) + }) + + it('accepts the fallback shape (404 + code=not_found without rawCode)', () => { + // A middleware that re-throws with the HTTP status preserved but drops + // the SDK-specific rawCode must still classify — otherwise the fix + // depends on which layer surfaces the failure. + const rethrown = { statusCode: 404, code: 'not_found', message: 'not found' } + expect(isAgentAlreadyGoneOnRelease(rethrown)).toBe(true) + }) + + it('rejects a 404 on a different code (e.g. workspace_not_found)', () => { + // 404 alone is not sufficient — a not-found response for a different + // resource must not silently succeed a release. + const workspaceMissing = { + statusCode: 404, + code: 'workspace_not_found', + rawCode: 'workspace_not_found', + } + expect(isAgentAlreadyGoneOnRelease(workspaceMissing)).toBe(false) + }) + + it('rejects a 503 host-unavailable (retryable path)', () => { + // The ar-1540-babysit-relay case: retryable transport failure. Must NOT + // classify as gone — the caller should keep retrying with backoff. + const hostUnavailable = { + name: 'RelayError', + code: 'transport_error', + retryable: true, + statusCode: 503, + rawCode: 'agent_host_unavailable', + } + expect(isAgentAlreadyGoneOnRelease(hostUnavailable)).toBe(false) + }) + + it('rejects a generic 500 without the not_found code', () => { + expect( + isAgentAlreadyGoneOnRelease({ statusCode: 500, code: 'internal_error' }), + ).toBe(false) + }) + + it('rejects null / undefined / primitive errors safely', () => { + expect(isAgentAlreadyGoneOnRelease(null)).toBe(false) + expect(isAgentAlreadyGoneOnRelease(undefined)).toBe(false) + expect(isAgentAlreadyGoneOnRelease('string error')).toBe(false) + expect(isAgentAlreadyGoneOnRelease(0)).toBe(false) + expect(isAgentAlreadyGoneOnRelease(new Error('bare error'))).toBe(false) + }) + + it('rejects an object without any of the discriminator fields', () => { + expect(isAgentAlreadyGoneOnRelease({ message: 'oops' })).toBe(false) + expect(isAgentAlreadyGoneOnRelease({ statusCode: 404 })).toBe(false) + expect(isAgentAlreadyGoneOnRelease({ code: 'not_found' })).toBe(false) + }) +}) diff --git a/src/orchestrator/release-error.ts b/src/orchestrator/release-error.ts new file mode 100644 index 00000000..bc2e8f04 --- /dev/null +++ b/src/orchestrator/release-error.ts @@ -0,0 +1,34 @@ +/** + * Classify errors returned by `FleetClient.release()`. + * + * A release that comes back as a Relay 404 `agent_not_found` IS the successful + * end state release aims for: the agent is already gone from Relay's view. + * Treating it as a failure re-arms `#abandonStuckDispatch` → + * `#releaseAndTerminateAgents` at ~1 Hz forever, which is what wedged cloud + * dispatch: every sweep tick reprinted the same three ghost agents and + * starved discovery entirely. + * + * Two shapes have to satisfy the check because the SDK's `RelayError` and a + * plain-Error re-throw both reach the caller: + * + * 1. `rawCode === 'agent_not_found'` — the canonical field the Relay SDK + * stamps onto the error before rethrowing. + * 2. `statusCode === 404 && code === 'not_found'` — a defensive fallback for + * a middleware that re-threw with the HTTP status preserved but dropped + * the SDK-specific `rawCode`. Neither field on its own is enough (a 404 + * on a different route would misclassify). + * + * Anything else is a real release failure — a 503 host-unavailable is + * retryable and must keep going through the failure path; a 5xx from the + * broker itself is a real fault and must not silently succeed. + */ +export const isAgentAlreadyGoneOnRelease = (error: unknown): boolean => { + const errAny = error as { + rawCode?: unknown + statusCode?: unknown + code?: unknown + } | null | undefined + if (errAny?.rawCode === 'agent_not_found') return true + if (errAny?.statusCode === 404 && errAny?.code === 'not_found') return true + return false +}