From a52b7c63b5da43a77952f7b319974741ea14a3f6 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Mon, 24 Aug 2026 05:29:25 +0200 Subject: [PATCH 1/2] feat(health): publish why a dispatch attempt failed, as a bounded enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `skipReasons` (#358) told us the live container skips 27 candidates every sweep and that 5 of them are `dispatch-failed` — with the control-plane breaker closed, the fleet agent online and `readinessReconcile` healthy. That bucket is a count. It says the sweep got all the way to dispatching and dispatch threw; it does not say what threw, and the message that would say so goes to the daemon's stdout, which does not reach the deployed container's operator. This is the second level of the same breakdown, built the way #358 built the first: - `dispatchFailures` is the total, published as a zero once a sweep completes. `skipReasons` omits zero counts, so on that field alone "every dispatch succeeded" and "this producer has never heard of dispatch failures" are the same absence — and 0.1.72 is in production being exactly the second thing. - `dispatchFailureReasons` splits it by a closed vocabulary. Counts only, keys rebuilt from this side's own list rather than taken from the record, unknown codes folded into `other` so the parts still sum. The code is recorded at the skip site from the thrown value, never parsed back out of the operator-facing `reason` — a reworded message would silently empty a bucket. Classification follows the cause chain, because `contextualError` and the control-plane guard both rethrow wrapped. When nothing named matches, the *phase* that threw is the answer: `unclassified-gate` / `-triage` / `-dispatch`. A unit that failed in triage never reached the fleet, and on a surface carrying no messages the phase is the only thing left that still says who should look. #292's own tests now pin those codes: the `TypeError: fetch failed` case from #291 is `unclassified-triage`, not a fleet fault. `dispatchFailures` is deliberately NOT joined to the all-or-nothing candidates/dispatched/skipped trio. Requiring it would drop a 0.1.72 daemon's whole sweep block — deleting the counters that are currently the only view of the outage. Tests drive the real writer: a live daemon whose real dispatch really throws. Ablated five ways — absent coerced to zero, zero dropped as uninteresting, unknown codes dropped instead of folded, the skip site not classifying, and the incoming string used as an object key — each caught by the assertion that names it. Refs #355, #358 Co-Authored-By: Claude Opus 5 --- src/cli/diagnose.ts | 12 + src/index.ts | 7 + src/orchestrator/dispatch-failure-reason.ts | 136 +++++++ .../dispatch-failure-reasons.test.ts | 361 ++++++++++++++++++ src/orchestrator/factory.test.ts | 12 + src/orchestrator/factory.ts | 112 +++++- src/orchestrator/public-health.test.ts | 144 +++++++ src/orchestrator/public-health.ts | 60 ++- src/types.ts | 42 +- 9 files changed, 878 insertions(+), 8 deletions(-) create mode 100644 src/orchestrator/dispatch-failure-reason.ts create mode 100644 src/orchestrator/dispatch-failure-reasons.test.ts diff --git a/src/cli/diagnose.ts b/src/cli/diagnose.ts index 2a1ed36a..ffe744ae 100644 --- a/src/cli/diagnose.ts +++ b/src/cli/diagnose.ts @@ -506,6 +506,18 @@ export function renderDeployedDiagnosis(diagnosis: DeployedFactoryDiagnosis): st if (readiness.skipReasons) { lines.push(` skip reasons : ${formatSkipReasons(readiness.skipReasons)}`) } + // Rendered whenever the producer reports it, zero included: `skipReasons` + // drops a zero bucket, so without this line "no dispatch failed" and "this + // daemon cannot tell you" look identical to a reader (#355). + if (readiness.dispatchFailures !== undefined) { + lines.push( + ` dispatch failures : ${readiness.dispatchFailures}${ + readiness.dispatchFailureReasons + ? ` (${formatSkipReasons(readiness.dispatchFailureReasons)})` + : '' + }`, + ) + } } const capacity = health.dispatchCapacity if (capacity) { diff --git a/src/index.ts b/src/index.ts index 68816348..7a6c777a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -496,6 +496,13 @@ export { factorySweepSkipReasonCounts, } from './orchestrator/sweep-skip-reason' export type { FactorySweepSkipReasonCode } from './orchestrator/sweep-skip-reason' +export { + FACTORY_DISPATCH_FAILURE_REASON_CODES, + factoryDispatchFailureReasonCode, + factoryDispatchFailureReasonCodeForErrorClass, + factoryDispatchFailureReasonCounts, +} from './orchestrator/dispatch-failure-reason' +export type { FactoryDispatchFailureReasonCode } from './orchestrator/dispatch-failure-reason' export type { FleetControlPlaneState, FleetControlPlaneStatus } from './fleet/control-plane-circuit' export { LOAD_EVIDENCE_CONTRACT, diff --git a/src/orchestrator/dispatch-failure-reason.ts b/src/orchestrator/dispatch-failure-reason.ts new file mode 100644 index 00000000..309c72a8 --- /dev/null +++ b/src/orchestrator/dispatch-failure-reason.ts @@ -0,0 +1,136 @@ +/** + * Why a dispatch *attempt* failed, as a closed set (#355). + * + * `skipReasons` (#358) answers "why did the sweep decline this work unit", and + * on the live container it answers it with `dispatch-failed: 5` — every sweep, + * with the control-plane breaker closed, the fleet agent online and + * `readinessReconcile` healthy. That bucket is a *count*. It says the sweep got + * all the way to dispatching and dispatch threw; it does not say what threw, + * and the message that would say so goes to the daemon's stdout, which does not + * reach the deployed container's operator. + * + * So this is the second level of the same breakdown: `dispatchFailures` is the + * total (the same number `skipReasons['dispatch-failed']` carries), and + * `dispatchFailureReasons` splits it by cause. Same discipline as #358 — + * counts only, keys from this vocabulary rather than from the record, unknown + * codes folded into `other` rather than dropped so the parts still sum. + * + * As in #358 the code is recorded at the skip site alongside the free text, + * never derived from it: `perItemDispatchFailureCode` classifies a thrown value + * by type, and the two named-by-class-name entries below are the explicit + * exception, documented there. + * + * The three `unclassified-*` codes are not a failure of this vocabulary — they + * are its most useful answer when nothing named matches, because the *phase* + * that threw already names the owner. A pass that reaches dispatch and fails is + * a fleet problem; one that fails in triage never got near the fleet; one that + * fails in the gates ahead of triage failed reading durable state. Those are + * three different people's bugs, and separating them costs one variable. + */ +export const FACTORY_DISPATCH_FAILURE_REASON_CODES = [ + /** Relayfile shed an operation this dispatch needed (#297). Clears on its own. */ + 'relayfile-overloaded', + /** Live provider state moved under the dispatch; the unit returns to the queue. */ + 'live-state-changed', + /** The never-placed deadline released the dispatch mid-spawn (#303). Self-healing. */ + 'late-placement-released', + /** The durable lifecycle record is terminal: this unit will not be retried. */ + 'lifecycle-terminal', + /** Another publisher holds the durable lifecycle lease. Clears on release. */ + 'lifecycle-owned-elsewhere', + /** The fleet control-plane circuit refused admission. Dispatch is globally paused. */ + 'control-plane-open', + /** Something in the attempt hit its own deadline — see the note on class names. */ + 'timed-out', + /** The fleet broker never acknowledged the spawn within its ack timeout. */ + 'spawn-ack-timeout', + /** The fleet client holds a read-only identity and cannot spawn at all. */ + 'fleet-identity-read-only', + /** Registering the spawned agent with the broker failed. */ + 'agent-registration-failed', + /** Threw in the gates ahead of triage: scope, orphan recovery, durable reads. */ + 'unclassified-gate', + /** Threw inside triage, before the fleet was ever asked for anything. */ + 'unclassified-triage', + /** Threw inside dispatch itself, and no code above matched it. */ + 'unclassified-dispatch', + /** Recorded by a producer this vocabulary does not know. */ + 'other', +] as const + +export type FactoryDispatchFailureReasonCode = typeof FACTORY_DISPATCH_FAILURE_REASON_CODES[number] + +/** + * Codes recognised by allowlisted error *class name* rather than by `instanceof`. + * + * The rule everywhere else is to classify by type, because a message is one + * rename away from collapsing a bucket. These five are the exception, for one + * reason: their classes live in `src/fleet/relay-fleet-client.ts`, and + * `factory.ts` does not import that module. Adding the edge to get five + * `instanceof` checks would pull the relay SDK into the orchestrator's module + * graph — a real cost — to buy precision over a *class name*, which is + * code-controlled and already the identifier `telemetryErrorClass` publishes + * everywhere else in this codebase. A rename here degrades one bucket to + * `unclassified-dispatch`; it cannot leak and it cannot break the sum. + * + * `TimeoutError` is deliberately generic: `FleetControlPlaneTimeoutError` sets + * that name on purpose, and so do `AbortSignal.timeout` and several SDK paths. + * Mapping it to the control plane specifically would be a guess; mapping it to + * "something in this attempt hit a deadline" is exactly true, and with 34-minute + * sweeps against 60-second attempts it is the answer worth being able to see. + */ +const DISPATCH_FAILURE_ERROR_CLASS_CODES: Readonly> = { + RelaySpawnAckTimeoutError: 'spawn-ack-timeout', + ReadOnlyFleetIdentityError: 'fleet-identity-read-only', + FactoryAgentRegistrationError: 'agent-registration-failed', + TimeoutError: 'timed-out', + AbortError: 'timed-out', +} + +/** The code an allowlisted error class name names, if this vocabulary knows one. */ +export const factoryDispatchFailureReasonCodeForErrorClass = ( + errorClass: string, +): FactoryDispatchFailureReasonCode | undefined => + Object.hasOwn(DISPATCH_FAILURE_ERROR_CLASS_CODES, errorClass) + ? DISPATCH_FAILURE_ERROR_CLASS_CODES[errorClass] + : undefined + +/** + * Coerce an arbitrary value onto the vocabulary. + * + * Unrecognised codes collapse to `other` rather than being dropped, for the + * same reason #358 gives: a failure that vanished from the breakdown would make + * the parts stop summing to `dispatchFailures`, and a reader comparing the two + * would conclude the counter was broken rather than that the producer was newer. + */ +export const factoryDispatchFailureReasonCode = (value: unknown): FactoryDispatchFailureReasonCode => + typeof value === 'string' && + (FACTORY_DISPATCH_FAILURE_REASON_CODES as readonly string[]).includes(value) + ? value as FactoryDispatchFailureReasonCode + : 'other' + +/** + * The per-cause breakdown of one sweep's failed dispatch attempts, counts only. + * + * Counts exactly the entries the sweep recorded as `dispatch-failed`, so the + * parts sum to `dispatchFailures` and to `skipReasons['dispatch-failed']` by + * construction. An entry whose producer recorded no `failureCode` folds into + * `other` for the same reason an unknown one does. + * + * Zero-count codes are omitted — the vocabulary is fixed and published, so an + * absent key reads as zero unambiguously. The total is carried separately by + * `dispatchFailures`, which is written whenever a sweep completes, so "ran and + * every dispatch succeeded" stays distinguishable from "never ran". + */ +export function factoryDispatchFailureReasonCounts( + skipped: Iterable<{ code?: unknown; failureCode?: unknown }> | undefined, +): Partial> { + const counts: Partial> = {} + if (!skipped) return counts + for (const entry of skipped) { + if (entry === null || typeof entry !== 'object' || entry.code !== 'dispatch-failed') continue + const code = factoryDispatchFailureReasonCode(entry.failureCode) + counts[code] = (counts[code] ?? 0) + 1 + } + return counts +} diff --git a/src/orchestrator/dispatch-failure-reasons.test.ts b/src/orchestrator/dispatch-failure-reasons.test.ts new file mode 100644 index 00000000..283e9310 --- /dev/null +++ b/src/orchestrator/dispatch-failure-reasons.test.ts @@ -0,0 +1,361 @@ +import { describe, expect, it } from 'vitest' +import { mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { + FactoryConfigSchema, + createFactory, + publicHealthFromHeartbeat, + type FactoryConfig, + type LinearIssue, + type TriageDecision, + type TriageEngine, +} from '../index' +import { FakeFleetClient, FakeMountClient } from '../testing' +import { RelaySpawnAckTimeoutError } from '../fleet/relay-fleet-client' +import { normalizePublicHealth } from './public-health' +import type { FactoryPublicReadinessReconcileHealth, FactoryReadinessReconcileStatus } from '../types' +import type { SpawnInput, SpawnResult } from '../ports/fleet' + +/** + * Why a dispatch attempt failed, end to end (#355). + * + * The live container publishes `skipReasons: { 'dispatch-failed': 5 }` on every + * sweep, with the control-plane breaker closed, the fleet agent online and + * `readinessReconcile` healthy. Five eligible issues are selected, dispatch is + * attempted, and it fails — and the bucket is a count, so it does not say what + * failed. The daemon knows; it writes the reason to stdout, which does not + * reach the deployed container's operator. + * + * Every assertion below drives the real writer: a live daemon running a real + * sweep whose real dispatch really throws. Hand-setting `failureCode` on a + * report fixture would prove the projection copies a field it was handed, and + * prove nothing about the sweep that has to produce it. + */ + +const ready = '11111111-1111-4111-8111-111111111111' +const implementing = '22222222-2222-4222-8222-222222222222' +const done = '33333333-3333-4333-8333-333333333333' +const planning = '44444444-4444-4444-8444-444444444444' + +const config = (overrides: Record = {}): FactoryConfig => FactoryConfigSchema.parse({ + workspaceId: 'factory-dispatch-failure-reasons', + repos: { + byLabel: { pear: 'AgentWorkforce/pear' }, + clonePaths: { 'AgentWorkforce/pear': '/work/pear' }, + default: 'AgentWorkforce/pear', + }, + triage: { maxImplementers: 4 }, + batchSize: 4, + stateIds: { readyForAgent: ready, agentImplementing: implementing, done, inPlanning: planning }, + verification: { enabled: false }, + ...overrides, +}) + +const issuePath = (n: number) => `/linear/issues/AR-${n}__uuid-${n}.json` + +const issueFile = (n: number) => ({ + provider: 'linear', + objectType: 'issue', + objectId: `uuid-${n}`, + payload: { + id: `uuid-${n}`, + identifier: `AR-${n}`, + title: `[factory-e2e] Fix factory issue ${n}`, + description: 'Implement the requested fix in src/orchestrator/factory.ts and verify it with tests.', + stateId: ready, + url: `https://linear.app/agent-relay/issue/AR-${n}/factory-issue-${n}`, + labels: [{ name: 'pear' }], + labelIds: ['label-id-not-used-by-parser'], + team: { key: 'AR', name: 'Agent Relay' }, + project: { name: 'Factory' }, + state: { id: ready, name: 'Ready for Agent' }, + }, +}) + +class StaticTriage implements TriageEngine { + async triage(issue: LinearIssue): Promise { + const number = issue.key.match(/\d+/)?.[0] ?? '0' + return { + issue: { uuid: issue.uuid, key: issue.key, path: issue.path }, + routes: [{ repo: 'AgentWorkforce/pear', clonePath: '/work/pear', rationale: 'test route' }], + scope: 'single', + implementers: [{ + name: `ar-${number}-impl`, + role: 'implementer', + capability: 'spawn:codex', + model: 'codex', + task: `Implement ${issue.key}`, + repo: 'AgentWorkforce/pear', + clonePath: '/work/pear', + node: 'self', + }], + reviewer: { + name: `ar-${number}-review`, + role: 'reviewer', + capability: 'spawn:claude', + model: 'claude', + task: `Review ${issue.key}`, + repo: 'AgentWorkforce/pear', + clonePath: '/work/pear', + node: 'self', + }, + thin: false, + confidence: 'high', + rationale: 'static test decision', + } + } +} + +/** Triage that throws, so the failure happens before the fleet is ever asked. */ +class ThrowingTriage implements TriageEngine { + async triage(issue: LinearIssue): Promise { + throw new Error(`triage backend refused ${issue.key} at /linear/issues/${issue.key}.json`) + } +} + +/** + * A fleet whose spawn throws for issues this test names. + * + * The failure is injected at the spawn, not at the skip site, precisely because + * the classification has to survive the trip: `#dispatchUnlocked` and + * `contextualError` both rethrow wrapped, so a classifier reading only the + * outermost error would see a plain `Error` and lose every named cause. + */ +class SpawnFailingFleetClient extends FakeFleetClient { + constructor(private readonly failure: (input: SpawnInput) => Error | undefined) { + super() + } + + override async spawn(input: SpawnInput): Promise { + const error = this.failure(input) + if (error) throw error + return super.spawn(input) + } +} + +async function sweepReadiness( + files: Record, + deps: { fleet?: FakeFleetClient; triage?: TriageEngine } = {}, +): Promise { + const root = await mkdtemp(join(tmpdir(), 'factory-dispatch-failure-reasons-')) + const factory = createFactory( + config({ loop: { registryPath: join(root, 'registry.json'), heartbeatPath: join(root, 'heartbeat.json') } }), + { + mount: new FakeMountClient(files), + fleet: deps.fleet ?? new FakeFleetClient(), + triage: deps.triage ?? new StaticTriage(), + logger: {}, + }, + ) + try { + await factory.start({ + mode: 'live', + // The startup backfill is the sweep under test and settles before + // `start()` resolves; a periodic pass landing mid-read would make the + // numbers describe a sweep this test never set up. + liveSubscription: { transport: 'subscribe', reconcileIntervalMs: 600_000 }, + }) + const status = factory.status().readinessReconcile + if (!status) throw new Error('live daemon published no readinessReconcile status') + return status + } finally { + await factory.stop() + await rm(root, { recursive: true, force: true }) + } +} + +/** The public record a container would serve for this readiness status. */ +const published = (status: FactoryReadinessReconcileStatus): FactoryPublicReadinessReconcileHealth => { + const health = publicHealthFromHeartbeat({ + pid: 1, + status: 'running', + iteration: 1, + maxIterations: 1, + updatedAt: new Date(1_700_000_000_000).toISOString(), + updatedAtMs: 1_700_000_000_000, + readinessReconcile: status, + }, { nowMs: 1_700_000_000_000 }) + if (!health.readinessReconcile) throw new Error('public health carried no readinessReconcile block') + return health.readinessReconcile +} + +/** MUST-FIRE: dispatch was attempted, it failed, and the surface names why. */ +const expectNamedDispatchFailures = (readiness: FactoryPublicReadinessReconcileHealth): void => { + expect(readiness.dispatchFailures).toBeGreaterThan(0) + expect(readiness.dispatchFailureReasons).toBeDefined() + expect(Object.values(readiness.dispatchFailureReasons ?? {}).reduce((sum, n) => sum + n, 0)) + .toBe(readiness.dispatchFailures) +} + +/** MUST-NOT-FIRE: dispatch was attempted and nothing failed — a real zero. */ +const expectNoDispatchFailures = (readiness: FactoryPublicReadinessReconcileHealth): void => { + expect(readiness.dispatchFailures).toBe(0) + expect(Object.hasOwn(readiness, 'dispatchFailures')).toBe(true) + expect(Object.hasOwn(readiness, 'dispatchFailureReasons')).toBe(false) +} + +describe('dispatch failure reasons (#355)', () => { + it('names the cause of a failed dispatch, and the parts sum to the bucket that counted it', async () => { + const fleet = new SpawnFailingFleetClient((input) => + input.name.startsWith('ar-951') || input.name.startsWith('ar-952') + ? new RelaySpawnAckTimeoutError('spawn', 60_000) + : undefined) + + const status = await sweepReadiness({ + [issuePath(951)]: issueFile(951), + [issuePath(952)]: issueFile(952), + [issuePath(953)]: issueFile(953), + }, { fleet }) + + expect(status).toMatchObject({ + candidates: 3, + dispatched: 1, + skipped: 2, + skipReasons: { 'dispatch-failed': 2 }, + dispatchFailures: 2, + dispatchFailureReasons: { 'spawn-ack-timeout': 2 }, + }) + + const readiness = published(status) + expectNamedDispatchFailures(readiness) + expect(readiness.dispatchFailureReasons).toEqual({ 'spawn-ack-timeout': 2 }) + // The breakdown refines exactly the bucket it sits under. A reader holding + // both numbers can check that, and it is the only integrity check this + // surface offers. + expect(readiness.dispatchFailures).toBe(readiness.skipReasons?.['dispatch-failed']) + }) + + it('CONTROL: a zero is published as a zero, so "none failed" never reads as "cannot say"', async () => { + // The trap this test exists for. `skipReasons` omits zero counts, so on + // that field alone a sweep in which every dispatch succeeded is the same + // absence as a producer that has never heard of dispatch failures — and + // 0.1.72 is in production being exactly the second thing. If + // `dispatchFailures` were built with a helper that coerced absent to zero, + // or that dropped a zero as uninteresting, this pair would collapse. + const clean = published(await sweepReadiness({ + [issuePath(961)]: issueFile(961), + [issuePath(962)]: issueFile(962), + })) + expect(clean).toMatchObject({ candidates: 2, dispatched: 2, skipped: 0 }) + expectNoDispatchFailures(clean) + // `skipReasons` cannot answer the question, which is why the total exists. + expect(clean.skipReasons).toBeUndefined() + + const failing = published(await sweepReadiness( + { [issuePath(963)]: issueFile(963) }, + { + fleet: new SpawnFailingFleetClient(() => new RelaySpawnAckTimeoutError('spawn', 60_000)), + }, + )) + + // Neither expectation can stand in for the other: swap them and the suite + // fails, so a pass here is evidence about the sweep rather than about a + // constant. + expect(() => expectNoDispatchFailures(failing)).toThrow() + expect(() => expectNamedDispatchFailures(clean)).toThrow() + expect(() => expectNamedDispatchFailures(failing)).not.toThrow() + expect(() => expectNoDispatchFailures(clean)).not.toThrow() + + // And the zero survives the wire, which is where a coercion would show up. + const roundTripped = normalizePublicHealth(JSON.parse(JSON.stringify({ + schemaVersion: 1, + ok: true, + status: 'ok', + stale: false, + degradedSubsystems: [], + readinessReconcile: clean, + }))) + expect(roundTripped?.readinessReconcile?.dispatchFailures).toBe(0) + expect(Object.hasOwn(roundTripped?.readinessReconcile ?? {}, 'dispatchFailures')).toBe(true) + }) + + it('leaves the field absent until a sweep completes, so "never attempted" stays its own reading', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-dispatch-failure-reasons-')) + const factory = createFactory( + config({ loop: { registryPath: join(root, 'registry.json'), heartbeatPath: join(root, 'heartbeat.json') } }), + { mount: new FakeMountClient(), fleet: new FakeFleetClient(), triage: new StaticTriage(), logger: {} }, + ) + try { + const status = factory.status().readinessReconcile + expect(status?.state).toBe('not-running') + expect(status && Object.hasOwn(status, 'dispatchFailures')).toBe(false) + // Three readings, three shapes: absent here, `0` for a clean sweep, a + // positive integer with a breakdown for a failing one. + expect(status && Object.hasOwn(published(status), 'dispatchFailures')).toBe(false) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + + it('separates a failure in triage from one in dispatch, because they are different owners', async () => { + // Nothing named matches either of these, and that is the case the phase + // codes are for: a unit that never reached the fleet is not a fleet bug, + // and on a surface carrying no messages the phase is the only thing left + // that still says who should look. + const triageFailed = await sweepReadiness( + { [issuePath(971)]: issueFile(971) }, + { triage: new ThrowingTriage() }, + ) + expect(triageFailed).toMatchObject({ + candidates: 1, + dispatched: 0, + dispatchFailures: 1, + dispatchFailureReasons: { 'unclassified-triage': 1 }, + }) + + const dispatchFailed = await sweepReadiness( + { [issuePath(972)]: issueFile(972) }, + { fleet: new SpawnFailingFleetClient(() => new Error('broker refused the spawn')) }, + ) + expect(dispatchFailed).toMatchObject({ + candidates: 1, + dispatched: 0, + dispatchFailures: 1, + dispatchFailureReasons: { 'unclassified-dispatch': 1 }, + }) + }) + + it('classifies through the wrapper, so a rethrown cause does not lose its name', async () => { + // `#dispatchUnlocked` and `contextualError` both rethrow wrapped. A + // classifier that read only the outermost error would see a plain `Error` + // here and report `unclassified-dispatch` for a failure that names itself. + const wrapped = new Error('dispatch failed for AR-981') + ;(wrapped as Error & { cause?: unknown }).cause = new RelaySpawnAckTimeoutError('spawn', 60_000) + + const status = await sweepReadiness( + { [issuePath(981)]: issueFile(981) }, + { fleet: new SpawnFailingFleetClient(() => wrapped) }, + ) + expect(status?.dispatchFailureReasons).toEqual({ 'spawn-ack-timeout': 1 }) + }) + + it('publishes counts only: no issue key, path, title or error message crosses', async () => { + const status = await sweepReadiness( + { + [issuePath(991)]: issueFile(991), + [issuePath(992)]: issueFile(992), + }, + { + fleet: new SpawnFailingFleetClient((input) => + input.name.startsWith('ar-991') + ? new Error('spawn failed for AR-991 at /linear/issues/AR-991__uuid-991.json (relay token expired)') + : undefined), + }, + ) + + const serialized = JSON.stringify(published(status)) + expect(serialized).not.toContain('AR-991') + expect(serialized).not.toContain('/linear/issues') + expect(serialized).not.toContain('relay token') + expect(serialized).not.toContain('factory-e2e') + expect(serialized).not.toContain('AgentWorkforce/pear') + + const record = JSON.parse(serialized) as Record + expect(record.dispatchFailures).toBe(1) + for (const count of Object.values(record.dispatchFailureReasons as Record)) { + expect(typeof count).toBe('number') + } + }) +}) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index bdacc1a7..7a7fbc67 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -5485,6 +5485,9 @@ describe('FactoryLoop', () => { issue: { uuid: 'AgentWorkforce/pear#59', key: '59', path: racedPath }, reason: 'live state changed during dispatch', code: 'dispatch-failed', + // The publishable half of the same classification (#355). The bucket + // count says five dispatches failed; this says they raced the provider. + failureCode: 'live-state-changed', }) expect(report.dispatched.map((result) => result.issue.key)).toEqual(['60']) expect(fleet.spawns.map((spawn) => spawn.name)).toEqual([ @@ -5578,6 +5581,7 @@ describe('FactoryLoop', () => { issue: { uuid: 'AgentWorkforce/pear#59', key: '59', path: blockedPath }, reason: 'dispatch lifecycle already terminal', code: 'dispatch-failed', + failureCode: 'lifecycle-terminal', }) expect(report.dispatched.map((result) => result.issue.key)).toEqual(['60']) expect(fleet.spawns.map((spawn) => spawn.name)).toEqual([ @@ -5612,6 +5616,10 @@ describe('FactoryLoop', () => { // carries a classification rather than raw provider text. reason: 'dispatch failed (TypeError)', code: 'dispatch-failed', + // Nothing names a `TypeError: fetch failed`, so the phase does: it + // threw in triage, which is a different owner from a fleet fault even + // though both land in the same `dispatch-failed` bucket (#355). + failureCode: 'unclassified-triage', }) expect(report.dispatched.map((result) => result.issue.key)).toEqual(['60']) expect(factory.status().counters.dispatchItemFailuresSkipped).toBe(1) @@ -5735,6 +5743,10 @@ describe('FactoryLoop', () => { issue: { uuid: 'AgentWorkforce/pear#59', key: '59', path: blockedPath }, reason: 'dispatch failed (Error)', code: 'dispatch-failed', + // The injected fault is the `unrelated per-item fault` above, thrown + // from triage — not the open circuit, which this dry run is exempt + // from. The phase code says so rather than leaving them conflated. + failureCode: 'unclassified-triage', }) expect(report.dispatched.map((result) => result.issue.key)).toEqual(['60']) expect(fleet.spawns).toEqual([]) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 191d4fcd..de5548b7 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -152,6 +152,11 @@ import { } from './public-health' import { factorySweepSkipReasonCounts } from './sweep-skip-reason' import type { FactorySweepSkipReasonCode } from './sweep-skip-reason' +import { + factoryDispatchFailureReasonCodeForErrorClass, + factoryDispatchFailureReasonCounts, +} from './dispatch-failure-reason' +import type { FactoryDispatchFailureReasonCode } from './dispatch-failure-reason' import { boundedRunCostTotal, CostLedger, type RunCostTotal, type UnpricedModelCostRecord } from '../cost/ledger' import { createTicketDispatchDelivery, type TicketDispatchDelivery } from '../delivery/ticket-dispatch' import { @@ -864,6 +869,16 @@ export class FactoryLoop implements Factory { dispatched: number skipped: number skipReasons: Partial> + /** + * The `dispatch-failed` bucket's own total, held even when it is zero. + * + * `skipReasons` omits zero counts, so a sweep in which every dispatch + * succeeded and a daemon that does not report the field are the same + * absence there. Here they are not, and telling them apart is the whole + * point of the breakdown beside it (#355). + */ + dispatchFailures: number + dispatchFailureReasons: Partial> discoveryDeferred?: 'sweep-in-flight' } readonly #liveEventQueue: ChangeEvent[] = [] @@ -1889,6 +1904,8 @@ export class FactoryLoop implements Factory { dispatched: report.dispatched.length, skipped: report.skipped.length, skipReasons: this.#readinessReconcileLastSweep?.skipReasons, + dispatchFailures: this.#readinessReconcileLastSweep?.dispatchFailures, + dispatchFailureReasons: this.#readinessReconcileLastSweep?.dispatchFailureReasons, discoveryDeferred: report.discoveryDeferred, }) } catch (error) { @@ -3093,6 +3110,10 @@ export class FactoryLoop implements Factory { continue } const recoveredIdentity = recoveredOrphan ? githubIssueRefIdentity(issueRef(issue)) : undefined + // Reset per work unit, and advanced by assignment immediately before + // each stage rather than inferred in the catch: the whole value of the + // `unclassified-*` codes is that they name the stage honestly (#355). + let attemptPhase: DispatchAttemptPhase = 'gate' try { if (recoveredOrphan) { const dispatchBlock = await this.#dispatchBlockReason(issue) @@ -3116,8 +3137,10 @@ export class FactoryLoop implements Factory { continue } + attemptPhase = 'triage' const decision = await this.triageIssue(issue) triaged.push(decision) + attemptPhase = 'dispatch' const result = await this.dispatch(decision, { dryRun }) // A completed dispatch — even one that parks or escalates the issue — // proves the pipeline still works, so the fuse below starts over. @@ -3217,6 +3240,11 @@ export class FactoryLoop implements Factory { issue: issueRef(issue), reason: perItemDispatchSkipReason(error), code: 'dispatch-failed', + // The publishable half of the same classification. `reason` is the + // operator's sentence and stays off the health surface; this token + // is what tells a reader watching `dispatch-failed: 5` which of + // five very different bugs they are looking at (#355). + failureCode: perItemDispatchFailureCode(error, attemptPhase), }) continue } finally { @@ -5042,6 +5070,11 @@ export class FactoryLoop implements Factory { dispatched: report.dispatched.length, skipped: report.skipped.length, skipReasons: factorySweepSkipReasonCounts(report.skipped), + // Counted from the same entries `skipReasons` counts, so the parts sum to + // `skipReasons['dispatch-failed']` by construction rather than by a + // second traversal agreeing with the first. + dispatchFailures: report.skipped.filter((entry) => entry.code === 'dispatch-failed').length, + dispatchFailureReasons: factoryDispatchFailureReasonCounts(report.skipped), ...(report.discoveryDeferred ? { discoveryDeferred: report.discoveryDeferred } : {}), } } @@ -5133,6 +5166,13 @@ export class FactoryLoop implements Factory { ...(Object.keys(this.#readinessReconcileLastSweep.skipReasons).length > 0 ? { skipReasons: { ...this.#readinessReconcileLastSweep.skipReasons } } : {}), + // Unconditional, unlike the breakdown below it: a zero here is the + // fact "this sweep attempted dispatches and none of them failed", + // which no other field on this surface can express. + dispatchFailures: this.#readinessReconcileLastSweep.dispatchFailures, + ...(Object.keys(this.#readinessReconcileLastSweep.dispatchFailureReasons).length > 0 + ? { dispatchFailureReasons: { ...this.#readinessReconcileLastSweep.dispatchFailureReasons } } + : {}), ...(this.#readinessReconcileLastSweep.discoveryDeferred ? { discoveryDeferred: this.#readinessReconcileLastSweep.discoveryDeferred } : {}), @@ -20827,16 +20867,21 @@ const PASS_FATAL_CAUSE_DEPTH = 4 * classification has to follow the cause chain rather than trust the outermost * type. */ -const wrapsErrorOfType = ( +const findWrappedErrorOfType = ( error: unknown, - type: abstract new (...args: never[]) => Error, + type: abstract new (...args: never[]) => T, depth = 0, -): boolean => { - if (depth > PASS_FATAL_CAUSE_DEPTH || !(error instanceof Error)) return false - if (error instanceof type) return true - return wrapsErrorOfType((error as { cause?: unknown }).cause, type, depth + 1) +): T | undefined => { + if (depth > PASS_FATAL_CAUSE_DEPTH || !(error instanceof Error)) return undefined + if (error instanceof type) return error + return findWrappedErrorOfType((error as { cause?: unknown }).cause, type, depth + 1) } +const wrapsErrorOfType = ( + error: unknown, + type: abstract new (...args: never[]) => Error, +): boolean => findWrappedErrorOfType(error, type) !== undefined + /** * How many *unclassified* per-item failures without an intervening successful * dispatch end the pass. Named per-item conditions (a lifecycle claim refusal, @@ -20904,6 +20949,61 @@ const perItemDispatchSkipReason = (error: unknown): string => { return `dispatch failed (${telemetryErrorClass(error)})` } +/** + * How far a work unit got before it threw, which is half of its classification. + * + * The per-issue `try` in `#performRunOnce` spans three different jobs: the + * durable/scope gates, triage, then dispatch. They fail for unrelated reasons + * and belong to unrelated owners, and once a thrown value carries no name this + * vocabulary knows, the phase is the only thing left that still says who should + * look. Tracked by assignment rather than inferred, so it cannot drift. + */ +type DispatchAttemptPhase = 'gate' | 'triage' | 'dispatch' + +const UNCLASSIFIED_PHASE_CODES: Readonly> = { + gate: 'unclassified-gate', + triage: 'unclassified-triage', + dispatch: 'unclassified-dispatch', +} + +/** + * Why a dispatch attempt failed, as a code from the published vocabulary (#355). + * + * The sibling of `perItemDispatchSkipReason`: that returns the operator's + * sentence, this returns the one token that may cross onto the unauthenticated + * health surface, and both are recorded at the skip site from the same thrown + * value. The token is never parsed back out of the sentence — a reworded + * message would silently empty a bucket, and this vocabulary is what an + * operator reads when the daemon's stdout does not reach them. + * + * Ordered most specific first. Every branch follows the cause chain, because + * `contextualError` and the control-plane guard both rethrow wrapped. + */ +const perItemDispatchFailureCode = ( + error: unknown, + phase: DispatchAttemptPhase, +): FactoryDispatchFailureReasonCode => { + if (relayfileOverload(error) !== undefined) return 'relayfile-overloaded' + if (wrapsErrorOfType(error, LiveDispatchStateChangedError)) return 'live-state-changed' + if (wrapsErrorOfType(error, LatePlacementReleasedError)) return 'late-placement-released' + const refused = findWrappedErrorOfType(error, DispatchLifecycleClaimRefusedError) + if (refused) return refused.refusal === 'terminal' ? 'lifecycle-terminal' : 'lifecycle-owned-elsewhere' + if (wrapsErrorOfType(error, FleetControlPlaneCircuitOpenError)) return 'control-plane-open' + // The class-name tail of the vocabulary; see its own doc comment for why + // these five are not `instanceof`. Walked down the cause chain like the + // branches above, because `contextualError` wraps in a plain `Error` and + // reading only the outermost name would miss every wrapped spawn failure. + // `telemetryErrorClass` is the same allowlist that guards every other + // identifier leaving this process, so a hostile `name` cannot invent a key + // here either — it collapses to `Error`, which the map does not hold. + for (let cursor: unknown = error, depth = 0; cursor instanceof Error && depth <= PASS_FATAL_CAUSE_DEPTH; depth += 1) { + const named = factoryDispatchFailureReasonCodeForErrorClass(telemetryErrorClass(cursor)) + if (named) return named + cursor = (cursor as { cause?: unknown }).cause + } + return UNCLASSIFIED_PHASE_CODES[phase] +} + const triageEscalationQuestion = (decision: TriageDecision, issue?: { title?: string }): string => { const routedRepos = decision.routes.map((route) => route.repo).filter(Boolean) const subject = issue?.title?.trim() || decision.issue.key diff --git a/src/orchestrator/public-health.test.ts b/src/orchestrator/public-health.test.ts index 26c4221e..0b00501e 100644 --- a/src/orchestrator/public-health.test.ts +++ b/src/orchestrator/public-health.test.ts @@ -879,4 +879,148 @@ describe('sweep counters on the public surface (#355)', () => { expect(JSON.stringify(reread)).not.toContain('workspace-key') expect(reread?.readinessReconcile?.skipReasons).toEqual({ other: 7 }) }) + + // The measurement this whole block exists for: the live container publishes + // `skipReasons: { 'dispatch-failed': 5 }` on every sweep, with the breaker + // closed and readiness healthy, and that bucket is a count with no cause + // attached. The daemon knows the cause; it writes it to stdout, which does + // not reach the deployed operator. + it('breaks the dispatch-failed bucket down by cause, and the parts sum to its total', () => { + const readiness = swept({ + candidates: 27, + dispatched: 0, + skipped: 27, + skipReasons: { 'not-ready': 21, 'parked-dependency': 1, 'dispatch-failed': 5 }, + dispatchFailures: 5, + dispatchFailureReasons: { 'spawn-ack-timeout': 3, 'unclassified-dispatch': 2 }, + }) + + expect(readiness?.dispatchFailures).toBe(5) + expect(readiness?.dispatchFailureReasons) + .toEqual({ 'spawn-ack-timeout': 3, 'unclassified-dispatch': 2 }) + expect(Object.values(readiness?.dispatchFailureReasons ?? {}).reduce((sum, n) => sum + n, 0)) + .toBe(readiness?.dispatchFailures) + // ...and to the bucket it refines, which is the integrity check a reader + // holding both numbers can actually run. + expect(readiness?.dispatchFailures).toBe(readiness?.skipReasons?.['dispatch-failed']) + }) + + // THE CONTROL. `skipReasons` omits zero counts, so on that field alone + // "every dispatch succeeded" is the same absence as "this producer has never + // heard of dispatch failures" — and 0.1.72 is in production right now being + // exactly the second thing. `dispatchFailures` is the field that separates + // them, and it only does that if nothing coerces its absence to a zero or + // its zero to an absence. + it('keeps a zero, an absence and a producer without the field three different readings', () => { + const noneFailed = swept({ + candidates: 4, + dispatched: 4, + skipped: 0, + dispatchFailures: 0, + }) + expect(noneFailed?.dispatchFailures).toBe(0) + expect(Object.hasOwn(noneFailed ?? {}, 'dispatchFailures')).toBe(true) + // No breakdown, because there is nothing to break down — and the total + // still says so out loud. + expect(Object.hasOwn(noneFailed ?? {}, 'dispatchFailureReasons')).toBe(false) + + // A 0.1.72 daemon: the trio it does publish must survive intact, or this + // change would delete the counters that are currently the only view of the + // outage. + const olderProducer = swept({ candidates: 4, dispatched: 4, skipped: 0 }) + expect(olderProducer).toMatchObject({ candidates: 4, dispatched: 4, skipped: 0 }) + expect(Object.hasOwn(olderProducer ?? {}, 'dispatchFailures')).toBe(false) + + // No sweep has completed at all. + const neverRan = swept() + expect(Object.hasOwn(neverRan ?? {}, 'dispatchFailures')).toBe(false) + }) + + it('re-reads its own published zero without turning it back into an absence', () => { + const published = swept({ candidates: 4, dispatched: 4, skipped: 0, dispatchFailures: 0 }) + const reread = normalizePublicHealth({ + schemaVersion: FACTORY_PUBLIC_HEALTH_SCHEMA_VERSION, + ok: true, + status: 'ok', + stale: false, + degradedSubsystems: [], + readinessReconcile: published, + }) + expect(reread?.readinessReconcile?.dispatchFailures).toBe(0) + expect(Object.hasOwn(reread?.readinessReconcile ?? {}, 'dispatchFailures')).toBe(true) + }) + + // MUST-NOT-FIRE, the same leak vector #358 closed one level up: the + // breakdown's *keys* arrive from a remote record, and an object key is as + // publishable as a value. + it('rebuilds the dispatch-failure breakdown from its own vocabulary', () => { + const readiness = swept({ + candidates: 9, + dispatched: 0, + skipped: 9, + dispatchFailures: 9, + dispatchFailureReasons: { + 'spawn-ack-timeout': 4, + // Not in the vocabulary, and carrying exactly what must never publish. + ['AR-350 /linear/issues/AR-350__uuid.json']: 3, + ['Error: connect ECONNREFUSED 10.0.0.4:443']: 2, + } as Record, + }) + + expect(JSON.stringify(readiness)).not.toContain('AR-350') + expect(JSON.stringify(readiness)).not.toContain('/linear/issues') + expect(JSON.stringify(readiness)).not.toContain('ECONNREFUSED') + // Folded into `other`, not dropped, so the parts still sum to the total and + // a reader comparing them detects a newer producer rather than a broken + // counter. + expect(readiness?.dispatchFailureReasons).toEqual({ 'spawn-ack-timeout': 4, other: 5 }) + expect(Object.values(readiness?.dispatchFailureReasons ?? {}).reduce((sum, n) => sum + n, 0)) + .toBe(9) + }) + + it('drops counts a reader cannot use, and a breakdown that has lost its total', () => { + expect(swept({ + candidates: 1, + dispatched: 0, + skipped: 1, + dispatchFailures: 1, + dispatchFailureReasons: { + 'spawn-ack-timeout': Number.NaN, + 'timed-out': -3, + 'live-state-changed': 0, + } as Record, + })?.dispatchFailureReasons).toBeUndefined() + + // A breakdown with no total is an orphan: nothing to check the parts + // against, which is the one integrity check this surface offers. + const orphaned = swept({ + candidates: 1, + dispatched: 0, + skipped: 1, + dispatchFailureReasons: { 'timed-out': 1 } as Record, + }) + expect(Object.hasOwn(orphaned ?? {}, 'dispatchFailureReasons')).toBe(false) + }) + + it('applies the same key rebuild to a record that arrived over the wire', () => { + const reread = normalizePublicHealth({ + schemaVersion: FACTORY_PUBLIC_HEALTH_SCHEMA_VERSION, + ok: true, + status: 'ok', + stale: false, + degradedSubsystems: [], + readinessReconcile: { + state: 'healthy', + consecutiveFailures: 0, + failureThreshold: 3, + candidates: 7, + dispatched: 0, + skipped: 7, + dispatchFailures: 7, + dispatchFailureReasons: { '/srv/agent-workforce/.relay/workspace-key': 7 }, + }, + }) + expect(JSON.stringify(reread)).not.toContain('workspace-key') + expect(reread?.readinessReconcile?.dispatchFailureReasons).toEqual({ other: 7 }) + }) }) diff --git a/src/orchestrator/public-health.ts b/src/orchestrator/public-health.ts index 7942db3d..24106a37 100644 --- a/src/orchestrator/public-health.ts +++ b/src/orchestrator/public-health.ts @@ -7,6 +7,11 @@ import { factorySweepSkipReasonCode, } from './sweep-skip-reason' import type { FactorySweepSkipReasonCode } from './sweep-skip-reason' +import { + FACTORY_DISPATCH_FAILURE_REASON_CODES, + factoryDispatchFailureReasonCode, +} from './dispatch-failure-reason' +import type { FactoryDispatchFailureReasonCode } from './dispatch-failure-reason' import type { FactoryDispatchCapacityStatus, FactoryEventListenerStatus, @@ -202,6 +207,35 @@ const skipReasonCounts = ( return Object.fromEntries(ordered.map((code) => [code, counts[code] as number])) } +/** + * The last sweep's dispatch-failure breakdown, rebuilt key by key (#355). + * + * Identical discipline to `skipReasonCounts` and for the identical reason: the + * keys come from this module's own copy of the vocabulary, never from the + * record, so a producer on another version cannot put an arbitrary string onto + * an unauthenticated surface by using it as an object key. Unknown keys fold + * into `other` rather than being dropped, so the parts still sum to + * `dispatchFailures`. + */ +const dispatchFailureReasonCounts = ( + value: unknown, +): Partial> | undefined => { + const record = plainRecord(value) + if (!record) return undefined + const counts: Partial> = {} + for (const [key, raw] of Object.entries(record)) { + const parsed = finiteNumber(raw) + if (parsed === undefined || parsed < 0) continue + const floored = Math.floor(parsed) + if (floored === 0) continue + const code = factoryDispatchFailureReasonCode(key) + counts[code] = (counts[code] ?? 0) + floored + } + const ordered = FACTORY_DISPATCH_FAILURE_REASON_CODES.filter((code) => counts[code] !== undefined) + if (ordered.length === 0) return undefined + return Object.fromEntries(ordered.map((code) => [code, counts[code] as number])) +} + /** * The last completed sweep's arithmetic, published (#355). * @@ -222,11 +256,19 @@ const sweepOutcome = ( dispatched?: unknown skipped?: unknown skipReasons?: unknown + dispatchFailures?: unknown + dispatchFailureReasons?: unknown discoveryDeferred?: unknown }, ): Partial> => { const candidates = optionalCount('candidates', status.candidates) const dispatched = optionalCount('dispatched', status.dispatched) @@ -240,11 +282,27 @@ const sweepOutcome = ( return {} } const skipReasons = skipReasonCounts(status.skipReasons) + // Deliberately NOT joined to the all-or-nothing trio above. A daemon on + // 0.1.72 publishes the trio and knows nothing about this field, and requiring + // it would drop that producer's whole sweep block — deleting the counters + // that are currently the only view of the outage. Independently optional, and + // `optionalCount` keeps a zero a zero: absent means "no completed sweep, or a + // producer without the field", `0` means "a sweep completed and no dispatch + // it attempted failed". Those are the two facts a bucket count cannot tell + // apart, which is why this number exists next to the breakdown. + const dispatchFailures = optionalCount('dispatchFailures', status.dispatchFailures) + const dispatchFailureReasons = dispatchFailureReasonCounts(status.dispatchFailureReasons) return { ...candidates, ...dispatched, ...skipped, ...(skipReasons ? { skipReasons } : {}), + ...dispatchFailures, + // A breakdown with no total is an orphan: a reader cannot check that the + // parts sum, which is the one integrity check this surface offers. + ...(dispatchFailures.dispatchFailures !== undefined && dispatchFailureReasons + ? { dispatchFailureReasons } + : {}), ...(status.discoveryDeferred === 'sweep-in-flight' ? { discoveryDeferred: 'sweep-in-flight' as const } : {}), diff --git a/src/types.ts b/src/types.ts index f9a1d94f..de2c07dd 100644 --- a/src/types.ts +++ b/src/types.ts @@ -9,6 +9,7 @@ import type { CloseProbePrInput, CloseProbePrResult } from './github/probe-close import type { GhRunner, GithubMergeGate } from './github/merge-gate' import type { AgentProcessFinder, ProcessIdentity } from './orchestrator/process-identity' import type { FactorySweepSkipReasonCode } from './orchestrator/sweep-skip-reason' +import type { FactoryDispatchFailureReasonCode } from './orchestrator/dispatch-failure-reason' import type { DispatchRelayflowOptions, RelayflowPolicyRegistry } from './dispatch/relayflow-registry' import type { VerificationGate } from './environments/verification-pipeline' import type { CostLedger } from './cost/ledger' @@ -253,6 +254,24 @@ export interface FactoryReadinessReconcileStatus { * themselves are a fixed published vocabulary, so an absent key is a zero. */ skipReasons?: Partial> + /** + * Dispatch attempts the last completed sweep made that failed (#355). + * + * The same number `skipReasons['dispatch-failed']` carries, published in its + * own right so it can be a zero. `skipReasons` omits zero-count codes, so + * "every dispatch succeeded" and "this daemon does not report the field" are + * the same absence there; here they are not. + * + * Optional and never defaulted, exactly like `candidates`: absent means no + * sweep has completed *or* the producer predates the field, `0` means a sweep + * completed and nothing it dispatched failed. + */ + dispatchFailures?: number + /** + * `dispatchFailures` split by cause. Zero-count codes are omitted; the codes + * are a fixed published vocabulary, so an absent key is a zero. + */ + dispatchFailureReasons?: Partial> /** * The last completed sweep never enumerated anything: another process held * the discovery lease, so it returned an empty report immediately. @@ -295,6 +314,17 @@ export interface FactoryPublicReadinessReconcileHealth { skipped?: number /** `skipped` split by a closed vocabulary of causes; zero counts omitted. */ skipReasons?: Partial> + /** + * Failed dispatch attempts in the last completed sweep, and why (#355). + * + * `dispatchFailures` is absent until a sweep completes and is a zero + * thereafter, so it separates "never attempted" from "attempted, none + * failed" — which `skipReasons` alone cannot, since it drops zero counts. + * The breakdown is counts only, keyed by a fixed vocabulary, and its parts + * sum to `dispatchFailures`. + */ + dispatchFailures?: number + dispatchFailureReasons?: Partial> /** The last completed sweep deferred to another process's discovery lease. */ discoveryDeferred?: 'sweep-in-flight' lastErrorClass?: string @@ -586,7 +616,17 @@ export interface IterationReport { * `reason` is free text for an operator; `code` is the closed vocabulary * that may cross onto the unauthenticated health surface (#355). */ - skipped: Array<{ issue: IssueRef; reason: string; code?: FactorySweepSkipReasonCode }> + skipped: Array<{ + issue: IssueRef + reason: string + code?: FactorySweepSkipReasonCode + /** + * Why the dispatch *attempt* failed, for `code: 'dispatch-failed'` only + * (#355). Recorded at the skip site by type, never parsed back out of + * `reason`, and absent on every other skip code. + */ + failureCode?: FactoryDispatchFailureReasonCode + }> dryRun: boolean slackDegraded?: boolean /** From d6e0b7fdfd0c3dbafef15ed8ecd1e1f7b5b186e1 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Mon, 24 Aug 2026 05:56:17 +0200 Subject: [PATCH 2/2] fix(factory): classify wrapped relayfile overloads --- .../dispatch-failure-reasons.test.ts | 47 ++++++++++++++++++- src/orchestrator/factory.ts | 32 ++++++++----- 2 files changed, 66 insertions(+), 13 deletions(-) diff --git a/src/orchestrator/dispatch-failure-reasons.test.ts b/src/orchestrator/dispatch-failure-reasons.test.ts index 283e9310..94dbce11 100644 --- a/src/orchestrator/dispatch-failure-reasons.test.ts +++ b/src/orchestrator/dispatch-failure-reasons.test.ts @@ -108,10 +108,19 @@ class StaticTriage implements TriageEngine { } } -/** Triage that throws, so the failure happens before the fleet is ever asked. */ +/** + * Triage that throws, so the failure happens before the fleet is ever asked. + * + * Also the one dispatch-attempt path that does NOT wrap what it throws, which + * makes it the only way to drive an unwrapped error all the way to the skip + * site — see the shed test below. + */ class ThrowingTriage implements TriageEngine { + constructor(private readonly error?: Error) {} + async triage(issue: LinearIssue): Promise { - throw new Error(`triage backend refused ${issue.key} at /linear/issues/${issue.key}.json`) + throw this.error ?? + new Error(`triage backend refused ${issue.key} at /linear/issues/${issue.key}.json`) } } @@ -331,6 +340,40 @@ describe('dispatch failure reasons (#355)', () => { expect(status?.dispatchFailureReasons).toEqual({ 'spawn-ack-timeout': 1 }) }) + // #361 review (P2, codex): overload classification must follow the same + // bounded cause chain as every named error. These two pin the direct and + // wrapped paths to the loop's single shedding predicate. + it('reports an unwrapped relayfile shed as a shed', async () => { + const shed = Object.assign(new Error('workspace durable object is busy'), { + status: 429, + reason: 'inflight_limit', + }) + const status = await sweepReadiness( + { [issuePath(1001)]: issueFile(1001) }, + { triage: new ThrowingTriage(shed) }, + ) + expect(status).toMatchObject({ + dispatchFailures: 1, + dispatchFailureReasons: { 'relayfile-overloaded': 1 }, + }) + }) + + it('treats a relayfile shed wrapped by spawn context as the same shed', async () => { + // `#spawnAgent` wraps this provider failure before the skip site. The + // source overload predicate must still drive the loop's counter/fuse path + // and the published reason; relabelling only the surface would lie. + const wrapped = Object.assign(new Error('workspace durable object is busy'), { + status: 429, + reason: 'inflight_limit', + }) + const status = await sweepReadiness( + { [issuePath(1002)]: issueFile(1002) }, + { fleet: new SpawnFailingFleetClient(() => wrapped) }, + ) + expect(status?.dispatchFailureReasons).toEqual({ 'relayfile-overloaded': 1 }) + expect(status?.dispatchFailures).toBe(1) + }) + it('publishes counts only: no issue key, path, title or error message crosses', async () => { const status = await sweepReadiness( { diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index de5548b7..979d188e 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -20236,20 +20236,27 @@ type RelayfileOverload = { retryAfterSeconds?: number } -const relayfileOverload = (error: unknown): RelayfileOverload | undefined => { - const flat = asRecord(error) ?? {} +/** How far to follow wrapped provider failures without trusting an unbounded chain. */ +const RELAYFILE_OVERLOAD_CAUSE_DEPTH = 4 + +const relayfileOverload = (error: unknown, depth = 0): RelayfileOverload | undefined => { + if (depth > RELAYFILE_OVERLOAD_CAUSE_DEPTH) return undefined + const flat = asRecord(error) + if (!flat) return undefined const response = asRecord(flat.response) ?? {} const data = asRecord(flat.data) ?? asRecord(response.data) ?? {} const details = asRecord(flat.details) ?? asRecord(data.details) ?? {} const statusValue = flat.status ?? flat.statusCode ?? response.status ?? response.statusCode const status = typeof statusValue === 'number' ? statusValue : Number(statusValue) - if (status !== 429) return undefined - const retryValue = flat.retryAfterSeconds ?? details.retryAfterSeconds ?? data.retryAfterSeconds - const parsedRetry = typeof retryValue === 'number' ? retryValue : Number(retryValue) - const retryAfterSeconds = Number.isFinite(parsedRetry) && parsedRetry >= 0 ? parsedRetry : undefined - const reason = stringValue(flat.reason) ?? stringValue(details.reason) ?? stringValue(data.reason) ?? - stringValue(flat.code) ?? stringValue(data.code) ?? 'rate_limited' - return { status, reason, ...(retryAfterSeconds === undefined ? {} : { retryAfterSeconds }) } + if (status === 429) { + const retryValue = flat.retryAfterSeconds ?? details.retryAfterSeconds ?? data.retryAfterSeconds + const parsedRetry = typeof retryValue === 'number' ? retryValue : Number(retryValue) + const retryAfterSeconds = Number.isFinite(parsedRetry) && parsedRetry >= 0 ? parsedRetry : undefined + const reason = stringValue(flat.reason) ?? stringValue(details.reason) ?? stringValue(data.reason) ?? + stringValue(flat.code) ?? stringValue(data.code) ?? 'rate_limited' + return { status, reason, ...(retryAfterSeconds === undefined ? {} : { retryAfterSeconds }) } + } + return relayfileOverload(flat.cause, depth + 1) } /** @@ -20976,8 +20983,11 @@ const UNCLASSIFIED_PHASE_CODES: Readonly