From 543c3dcd43555cd3a71c920efb8dd40734681b0b Mon Sep 17 00:00:00 2001 From: Miya Date: Wed, 12 Aug 2026 13:04:22 +0200 Subject: [PATCH 1/7] fix(cli): report unhandled startup crashes during `node up` instead of dying silently runUpCommand's try/catch only sees rejections it awaits. Anything that rejects off to the side of that chain crashes the process via Node's bare default uncaughtException/unhandledRejection handler instead, which never prints "Failed to start broker: ..." and never records broker_start_failed telemetry. Adds a process-level crash guard, armed for the startup + hold-open lifetime of runUpCommand, that routes that class of crash through the same diagnostic + telemetry + cleanup path as an ordinary caught failure. Also broadens classifyBrokerStartStage's connect-failure regex: it only matched Node's native fetch() message ("fetch failed"), never Bun's ("Unable to connect..."), so every real connect failure on the shipped Bun binary was misclassified as generic stage:'startup'. Co-Authored-By: Claude Sonnet 5 --- packages/cli/src/cli/lib/broker-lifecycle.ts | 102 ++++++++++++++++--- 1 file changed, 88 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/cli/lib/broker-lifecycle.ts b/packages/cli/src/cli/lib/broker-lifecycle.ts index e015f5d4b..199f9d687 100644 --- a/packages/cli/src/cli/lib/broker-lifecycle.ts +++ b/packages/cli/src/cli/lib/broker-lifecycle.ts @@ -332,13 +332,93 @@ export function classifyBrokerStartError(err: unknown): string { /** Exported for testing. */ export function classifyBrokerStartStage(_err: unknown, message: string): string { if (isBrokerAlreadyRunningError(message)) return 'already_running'; - if (/fetch failed/i.test(message)) return 'connect'; + // Node's native fetch() throws "fetch failed"; the CLI's Bun-compiled + // binaries throw Bun's own connect-failure text instead ("Unable to + // connect. Is the computer able to access the url?"). Recognize both so + // the shipped binary doesn't misclassify every connect failure as generic + // 'startup'. + if (/fetch failed/i.test(message) || /unable to connect/i.test(message)) return 'connect'; if (/Broker did not report API port/i.test(message)) return 'spawn'; if (/Broker process exited with code/i.test(message)) return 'spawn'; if (/ENOENT/i.test(message) && /broker/i.test(message)) return 'resolve_binary'; return 'startup'; } +/** + * Render the same "Failed to start broker" diagnostic + telemetry the + * `runUpCommand` catch block has always used. Extracted so the process-level + * crash guard (below) can report an unhandled rejection/exception the exact + * same way as an ordinary caught startup failure. + */ +function reportBrokerStartFailure( + err: unknown, + deps: CoreDependencies, + paths: CoreProjectPaths, + options: UpOptions +): void { + const message = toErrorMessage(err); + const stage = classifyBrokerStartStage(err, message); + track('broker_start_failed', { + stage, + error_class: classifyBrokerStartError(err), + }); + const detailedMessage = describeErrorWithCause(err); + recordBackgroundStartError(detailedMessage, paths.dataDir, options.backgroundChild === true, deps); + if (isBrokerAlreadyRunningError(message)) { + reportAlreadyRunningError(message, paths.dataDir, deps); + } else { + deps.error(`Failed to start broker: ${detailedMessage}`); + } +} + +/** + * `runUpCommand`'s startup try/catch only sees rejections it actually + * `await`s. Anything that rejects off to the side — a fire-and-forget + * background task inside a capability provider, an addon's internal promise + * chain, etc. — crashes the process via Node's bare default + * uncaughtException/unhandledRejection handler instead, which prints no + * "Failed to start broker" line and never records `broker_start_failed` + * telemetry. Observed in the wild as a `node up` that printed "Broker + * started." and then died with nothing further logged. + * + * This guard is armed for the lifetime of the foreground startup + hold-open + * phase so that class of crash gets the same diagnostic + telemetry + cleanup + * treatment as an ordinary caught failure, instead of vanishing into Node's + * default handler. `dispose()` must be called (via `finally`) so the + * listeners don't outlive this command invocation. + */ +function installStartupCrashGuard( + deps: CoreDependencies, + paths: CoreProjectPaths, + options: UpOptions, + shutdownOnce: () => Promise +): { dispose: () => void; markHandled: () => void } { + let handled = false; + const handleCrash = (err: unknown): void => { + if (handled) return; + handled = true; + void (async () => { + await shutdownOnce().catch(() => undefined); + reportBrokerStartFailure(err, deps, paths, options); + deps.exit(1); + })(); + }; + process.on('uncaughtException', handleCrash); + process.on('unhandledRejection', handleCrash); + return { + dispose: () => { + process.off('uncaughtException', handleCrash); + process.off('unhandledRejection', handleCrash); + }, + // Called from the normal catch block so a straggler process-level event + // for the same failure can't fire a second, duplicate report after this + // function has already handled it and moved on. + markHandled: () => { + handled = true; + }, + }; +} + async function resolveApiPortWithFallback( startApiPort: number, maxAttempts: number, @@ -1688,6 +1768,7 @@ export async function runUpCommand(options: UpOptions, deps: CoreDependencies): } await shutdownPromise; }; + const crashGuard = installStartupCrashGuard(deps, paths, options, shutdownOnce); try { if (existingPid !== null) { if (isProcessRunning(existingPid, deps)) { @@ -1849,21 +1930,14 @@ export async function runUpCommand(options: UpOptions, deps: CoreDependencies): await holdOpen; } } catch (err: unknown) { + // A straggler process-level crash event for this same failure must not + // also fire and duplicate the report below. + crashGuard.markHandled(); await shutdownOnce(); - const message = toErrorMessage(err); - const stage = classifyBrokerStartStage(err, message); - track('broker_start_failed', { - stage, - error_class: classifyBrokerStartError(err), - }); - const detailedMessage = describeErrorWithCause(err); - recordBackgroundStartError(detailedMessage, paths.dataDir, options.backgroundChild === true, deps); - if (isBrokerAlreadyRunningError(message)) { - reportAlreadyRunningError(message, paths.dataDir, deps); - } else { - deps.error(`Failed to start broker: ${detailedMessage}`); - } + reportBrokerStartFailure(err, deps, paths, options); deps.exit(1); + } finally { + crashGuard.dispose(); } } From 8e5071b65abcef717286c5b4a30cbd88095950b0 Mon Sep 17 00:00:00 2001 From: Miya Date: Wed, 12 Aug 2026 14:11:24 +0200 Subject: [PATCH 2/7] fix(cli): register node up's SIGINT/SIGTERM handlers before async startup work Live-tested the crash-guard build on sf-mini and reproduced a silent death that isn't a JS exception at all: the process exits with code 143 (SIGTERM), not zero output, not a crash-guard report. Traced it to signal-handler registration timing -- runUpCommand previously wired up its SIGINT/SIGTERM handlers only right before hold-open, after broker spawn, capability providers, and Reflex capture had all completed. A signal arriving anywhere in that earlier window hit Node's bare default disposition (immediate, silent termination) instead of the app's own graceful shutdown path. Moves both handlers to the top of runUpCommand, before any async startup work, alongside the crash guard from the previous commit. A SIGTERM in the startup window now gets the same logged, graceful shutdown as one that arrives during hold-open, regardless of what ultimately sends the signal. Co-Authored-By: Claude Sonnet 5 --- packages/cli/src/cli/lib/broker-lifecycle.ts | 49 +++++++++++--------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/cli/lib/broker-lifecycle.ts b/packages/cli/src/cli/lib/broker-lifecycle.ts index 199f9d687..86577d036 100644 --- a/packages/cli/src/cli/lib/broker-lifecycle.ts +++ b/packages/cli/src/cli/lib/broker-lifecycle.ts @@ -1769,6 +1769,34 @@ export async function runUpCommand(options: UpOptions, deps: CoreDependencies): await shutdownPromise; }; const crashGuard = installStartupCrashGuard(deps, paths, options, shutdownOnce); + // Registered before any async startup work (broker spawn, capability + // providers, Reflex capture, node delivery wait) so a signal arriving + // during that window gets the same graceful, logged shutdown as one that + // arrives later during hold-open. Previously these were registered just + // before hold-open — a SIGTERM in that earlier window hit Node's bare + // default disposition (silent immediate termination) instead, which is + // indistinguishable from a genuine crash when observed from outside. + deps.onSignal('SIGINT', async () => { + sigintCount += 1; + if (shuttingDown) { + if (sigintCount >= 2) { + deps.warn('Force exiting...'); + deps.exit(130); + } + return; + } + deps.log('\nStopping...'); + await shutdownOnce(); + deps.exit(0); + }); + deps.onSignal('SIGTERM', async () => { + if (shuttingDown) { + return; + } + deps.log('\nStopping (SIGTERM)...'); + await shutdownOnce(); + deps.exit(0); + }); try { if (existingPid !== null) { if (isProcessRunning(existingPid, deps)) { @@ -1902,27 +1930,6 @@ export async function runUpCommand(options: UpOptions, deps: CoreDependencies): deps.warn('Warning: --spawn specified but no teams.json found'); } - deps.onSignal('SIGINT', async () => { - sigintCount += 1; - if (shuttingDown) { - if (sigintCount >= 2) { - deps.warn('Force exiting...'); - deps.exit(130); - } - return; - } - deps.log('\nStopping...'); - await shutdownOnce(); - deps.exit(0); - }); - deps.onSignal('SIGTERM', async () => { - if (shuttingDown) { - return; - } - await shutdownOnce(); - deps.exit(0); - }); - const holdOpen = deps.holdOpen(); if (nodeProviders?.done) { await Promise.race([holdOpen, nodeProviders.done]); From 0a52c5f9885925ad60867b507c9ae3ff82bd692c Mon Sep 17 00:00:00 2001 From: Miya Date: Wed, 12 Aug 2026 14:16:51 +0200 Subject: [PATCH 3/7] test(cli): fix SIGINT test's readiness wait after earlier signal registration The 'up force exits on repeated SIGINT during a hung shutdown' test synchronized by polling until deps.onSignal had been called at all, implicitly relying on that only happening after the broker (and `relay`) was fully up. That was true when SIGINT/SIGTERM were registered just before hold-open, but the previous commit moved that registration to the top of runUpCommand, before any async startup work -- so onSignal now fires long before `relay` is assigned, and the test fired SIGINT against a still-null `relay`, so relay.shutdown was never called. Wait for the 'Broker started.' log line instead, which still reflects real startup completion. Co-Authored-By: Claude Sonnet 5 --- packages/cli/src/cli/commands/core.test.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/cli/commands/core.test.ts b/packages/cli/src/cli/commands/core.test.ts index 49e25fb70..3ea49022f 100644 --- a/packages/cli/src/cli/commands/core.test.ts +++ b/packages/cli/src/cli/commands/core.test.ts @@ -1196,11 +1196,13 @@ describe('registerCoreCommands', () => { }); void runCommand(program, ['up']); - for ( - let i = 0; - i < 10 && (deps.onSignal as unknown as { mock: { calls: unknown[][] } }).mock.calls.length === 0; - i += 1 - ) { + // SIGINT/SIGTERM are now registered before the broker starts (so a + // signal arriving during startup is handled gracefully too), so + // registration alone no longer implies `relay` is set. Wait for the + // broker to actually be up before firing the signal. + for (let i = 0; i < 20 && !(deps.log as unknown as { mock: { calls: unknown[][] } }).mock.calls.some( + (call) => call[0] === 'Broker started.' + ); i += 1) { await Promise.resolve(); } From ec078fd696e8abd2b8c90126894f9d3b23a31442 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Wed, 12 Aug 2026 12:18:00 +0000 Subject: [PATCH 4/7] style: auto-format with Prettier --- packages/cli/src/cli/commands/core.test.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/cli/commands/core.test.ts b/packages/cli/src/cli/commands/core.test.ts index 3ea49022f..5c17a4822 100644 --- a/packages/cli/src/cli/commands/core.test.ts +++ b/packages/cli/src/cli/commands/core.test.ts @@ -1200,9 +1200,14 @@ describe('registerCoreCommands', () => { // signal arriving during startup is handled gracefully too), so // registration alone no longer implies `relay` is set. Wait for the // broker to actually be up before firing the signal. - for (let i = 0; i < 20 && !(deps.log as unknown as { mock: { calls: unknown[][] } }).mock.calls.some( - (call) => call[0] === 'Broker started.' - ); i += 1) { + for ( + let i = 0; + i < 20 && + !(deps.log as unknown as { mock: { calls: unknown[][] } }).mock.calls.some( + (call) => call[0] === 'Broker started.' + ); + i += 1 + ) { await Promise.resolve(); } From 163361ede1942c15c43d0edfd5d5e2fae607f788 Mon Sep 17 00:00:00 2001 From: Miya Date: Wed, 12 Aug 2026 20:00:09 +0200 Subject: [PATCH 5/7] fix(cli): retry the post-handshake broker status check on transient connect failures candidate.getStatus() in startBrokerWithPortFallback is the first request made against a broker that just finished a successful handshake -- HarnessDriverClient.spawn()'s own getSession() poll already confirmed the broker was reachable moments earlier. Under load, the broker can be transiently preempted between that handshake and this immediate follow-up request, surfacing as a bare connect failure (Node's "fetch failed" / Bun's "Unable to connect. Is the computer able to access the url?"). Previously this had zero tolerance: one bad request and the whole `up` was reported failed even though the broker was (and remained) healthy -- this is the specific fetch() race behind the "died right after Event stream connected." failure mode traced earlier in this investigation. Adds getBrokerStatusWithRetry(): up to 4 attempts with a fixed 300ms delay between them (under 1s total budget), mirroring the spirit -- not the duration -- of the handshake's own 503-retry loop in HarnessDriverClient.spawn(). That loop waits out a possibly slow cold start; this one only smooths a momentary preemption right after a broker already confirmed up, so the budget is much shorter. Co-Authored-By: Claude Sonnet 5 --- .../cli/src/cli/lib/broker-lifecycle.test.ts | 60 ++++++++++++++++++- packages/cli/src/cli/lib/broker-lifecycle.ts | 52 +++++++++++++++- 2 files changed, 109 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/cli/lib/broker-lifecycle.test.ts b/packages/cli/src/cli/lib/broker-lifecycle.test.ts index 6dbab834b..6926f556d 100644 --- a/packages/cli/src/cli/lib/broker-lifecycle.test.ts +++ b/packages/cli/src/cli/lib/broker-lifecycle.test.ts @@ -7,12 +7,13 @@ import { classifyBrokerStartError, classifyBrokerStartStage, describeErrorWithCause, + getBrokerStatusWithRetry, isBundledBunExecutableEntrypoint, readNodeDeliveryStatus, resolveNodeIdentityFromSession, waitForNodeDelivery, } from './broker-lifecycle.js'; -import type { CoreDependencies } from '../commands/core.js'; +import type { CoreDependencies, CoreRelay } from '../commands/core.js'; describe('isBundledBunExecutableEntrypoint', () => { it.each(['/$bunfs/root/agent-relay', 'B:/~BUN/root/agent-relay.exe', 'B:\\~BUN\\root\\agent-relay.exe'])( @@ -128,6 +129,63 @@ describe('classifyBrokerStartStage', () => { }); }); +describe('getBrokerStatusWithRetry', () => { + function createDeps(sleep = vi.fn(async () => undefined)): CoreDependencies { + return { log: vi.fn(), sleep } as unknown as CoreDependencies; + } + + it('returns the status on the first successful attempt without sleeping', async () => { + const candidate: Pick = { + getStatus: vi.fn(async () => ({ agent_count: 0, pending_delivery_count: 0 })), + }; + const deps = createDeps(); + + const result = await getBrokerStatusWithRetry(candidate, deps); + + expect(result).toEqual({ agent_count: 0, pending_delivery_count: 0 }); + expect(candidate.getStatus).toHaveBeenCalledTimes(1); + expect(deps.sleep).not.toHaveBeenCalled(); + }); + + it('retries a transient connect failure and returns the status once the broker responds', async () => { + let attempt = 0; + const candidate: Pick = { + getStatus: vi.fn(async () => { + attempt += 1; + if (attempt < 3) { + throw new TypeError('Unable to connect. Is the computer able to access the url?'); + } + return { agent_count: 0, pending_delivery_count: 0 }; + }), + }; + const deps = createDeps(); + + const result = await getBrokerStatusWithRetry(candidate, deps, true); + + expect(result).toEqual({ agent_count: 0, pending_delivery_count: 0 }); + expect(candidate.getStatus).toHaveBeenCalledTimes(3); + expect(deps.sleep).toHaveBeenCalledTimes(2); + expect(deps.log).toHaveBeenCalledWith( + expect.stringContaining('Broker status check failed (attempt 1/4), retrying in 300ms...') + ); + }); + + it('exhausts its retry budget and throws the last error when the broker never responds', async () => { + const err = new TypeError('Unable to connect. Is the computer able to access the url?'); + const candidate: Pick = { + getStatus: vi.fn(async () => { + throw err; + }), + }; + const deps = createDeps(); + + await expect(getBrokerStatusWithRetry(candidate, deps)).rejects.toBe(err); + // 4 total attempts: the initial try plus 3 retries. + expect(candidate.getStatus).toHaveBeenCalledTimes(4); + expect(deps.sleep).toHaveBeenCalledTimes(3); + }); +}); + describe('readNodeDeliveryStatus', () => { it('reads the canonical snake_case broker status shape', () => { expect( diff --git a/packages/cli/src/cli/lib/broker-lifecycle.ts b/packages/cli/src/cli/lib/broker-lifecycle.ts index 86577d036..aa6847475 100644 --- a/packages/cli/src/cli/lib/broker-lifecycle.ts +++ b/packages/cli/src/cli/lib/broker-lifecycle.ts @@ -452,6 +452,54 @@ export function resolveBrokerBasePort(deps: Pick): numb return Number.isFinite(raw) && raw >= 0 ? raw : DEFAULT_BROKER_BASE_PORT; } +/** Bounded attempts for {@link getBrokerStatusWithRetry}'s post-handshake status check. */ +const STATUS_CHECK_MAX_ATTEMPTS = 4; +/** Fixed delay between status-check retries, in ms. */ +const STATUS_CHECK_RETRY_DELAY_MS = 300; + +/** + * `candidate.getStatus()` is the first request made against a broker that + * just finished a successful handshake -- `HarnessDriverClient.spawn()`'s own + * `getSession()` poll already confirmed the broker was reachable moments + * earlier. Under load, the broker can be transiently preempted between that + * handshake and this immediate follow-up request, surfacing as a bare + * connect failure (Node's `TypeError: fetch failed` / Bun's "Unable to + * connect. Is the computer able to access the url?"), which previously had + * zero tolerance: one bad request and the whole `up` was reported failed + * even though the broker was (and remained) healthy. + * + * A handful of short, fixed-delay retries absorb that hiccup. This mirrors + * the *spirit* of the 503-retry loop `HarnessDriverClient.spawn()` runs + * during the handshake, not its duration -- that loop waits out a possibly + * slow cold start; this one is only smoothing a momentary preemption right + * after a broker we already know is up, so the total budget is much + * shorter (under 1s across all retries). + * + * Exported for testing. + */ +export async function getBrokerStatusWithRetry( + candidate: Pick, + deps: CoreDependencies, + verbose?: boolean +): Promise { + let lastError: unknown; + for (let attempt = 1; attempt <= STATUS_CHECK_MAX_ATTEMPTS; attempt += 1) { + try { + return await candidate.getStatus(); + } catch (err) { + lastError = err; + if (attempt >= STATUS_CHECK_MAX_ATTEMPTS) break; + vlog( + deps, + verbose, + `Broker status check failed (attempt ${attempt}/${STATUS_CHECK_MAX_ATTEMPTS}), retrying in ${STATUS_CHECK_RETRY_DELAY_MS}ms...` + ); + await deps.sleep(STATUS_CHECK_RETRY_DELAY_MS); + } + } + throw lastError; +} + export async function startBrokerWithPortFallback( paths: CoreProjectPaths, basePort: number, @@ -463,7 +511,7 @@ export async function startBrokerWithPortFallback( vlog(deps, verbose, 'Asking the OS to assign the broker API port...'); const candidate = await deps.createRelay(paths.projectRoot, 0, brokerName, verbose); try { - await candidate.getStatus(); + await getBrokerStatusWithRetry(candidate, deps, verbose); if (!candidate.apiPort) { throw new Error('Broker started without reporting its OS-assigned API port.'); } @@ -496,7 +544,7 @@ export async function startBrokerWithPortFallback( vlog(deps, verbose, 'Broker client created. Checking broker status...'); try { - await candidate.getStatus(); + await getBrokerStatusWithRetry(candidate, deps, verbose); } catch (startupError) { try { await candidate.shutdown(); From d8a5b4f7a3b7a5782fc0a77b832ac3a7bb094f9b Mon Sep 17 00:00:00 2001 From: Miya Date: Fri, 14 Aug 2026 11:00:49 +0200 Subject: [PATCH 6/7] fix(cli): close review gaps in the node up crash guard - installStartupCrashGuard's own deps.exit(1) threw CliExit inside a detached async body with no awaiter; the resulting unhandled rejection hit handleCrash again, saw `handled` already true, and was silently dropped, leaving runUpCommand stuck in holdOpen instead of exiting. Route it through runSignalHandler (the same wrapper deps.onSignal uses) so the throw becomes a real, telemetry-flushed process exit. - getBrokerStatusWithRetry retried every getStatus() error, not just connect-stage ones, adding unnecessary delay and misleading retry logs for permanent failures (auth, protocol). Retry only connect-classified errors. - The catch block's crashGuard.markHandled() ran before await shutdownOnce(), so an unrelated crash during that cleanup window was silently swallowed by the guard. Moved it to run after cleanup, right before reporting, narrowing the suppression window to just the duplicate-report case it's meant for. - A rejecting shutdownOnce() in that same catch block skipped reportBrokerStartFailure/deps.exit(1) entirely, so a cleanup failure silently ate the original startup error's report. Wrapped it in its own try/catch. - SIGTERM/SIGINT arriving between broker-spawn and the status check finding `relay` still null, so shutdownOnce() no-op'd and leaked the broker child. startBrokerWithPortFallback now reports its in-flight candidate as soon as it exists via an optional callback; the failure path clears it back to null since that function already shuts the candidate down internally before rethrowing, avoiding a double shutdown() call on the same handle. Co-Authored-By: Claude Sonnet 5 --- packages/cli/src/cli/commands/core.test.ts | 42 +++++++++++++ .../cli/src/cli/lib/broker-lifecycle.test.ts | 14 +++++ packages/cli/src/cli/lib/broker-lifecycle.ts | 61 ++++++++++++++++--- 3 files changed, 109 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/cli/commands/core.test.ts b/packages/cli/src/cli/commands/core.test.ts index 5c17a4822..37ca18f6b 100644 --- a/packages/cli/src/cli/commands/core.test.ts +++ b/packages/cli/src/cli/commands/core.test.ts @@ -1230,6 +1230,48 @@ describe('registerCoreCommands', () => { expect(logCalls.filter((call) => call[0] === '\nStopping...')).toHaveLength(1); }); + it('up shuts down the in-flight broker candidate when SIGTERM arrives before the status check resolves', async () => { + let resolveStatus: (() => void) | undefined; + const relay = createRelayMock({ + getStatus: vi.fn( + () => + new Promise((resolve) => { + resolveStatus = () => resolve({ agent_count: 0, pending_delivery_count: 0 }); + }) + ), + }); + const { program, deps } = createHarness({ relay }); + void runCommand(program, ['up']); + + // Wait until the status check has actually started. By this point + // `startBrokerWithPortFallback`'s `onCandidateReady` callback has + // already assigned the outer `relay` -- well before the check itself + // resolves. This is exactly the window where `relay` used to still be + // null and a signal would leak the broker child instead of shutting it + // down. + for ( + let i = 0; + i < 20 && + (relay.getStatus as unknown as { mock: { calls: unknown[][] } }).mock.calls.length === 0; + i += 1 + ) { + await Promise.resolve(); + } + expect(relay.getStatus).toHaveBeenCalled(); + expect(relay.shutdown).not.toHaveBeenCalled(); + + const onSignalMock = deps.onSignal as unknown as { mock: { calls: unknown[][] } }; + const sigtermHandler = onSignalMock.mock.calls.find((call) => call[0] === 'SIGTERM')?.[1] as + | (() => Promise) + | undefined; + expect(sigtermHandler).toBeDefined(); + + await expect((sigtermHandler as () => Promise)()).rejects.toMatchObject({ code: 0 }); + + expect(relay.shutdown).toHaveBeenCalledTimes(1); + resolveStatus?.(); + }); + it('down stops broker and cleans stale files', async () => { const connectionPath = '/tmp/project/.agentworkforce/relay/connection.json'; const relaySockPath = '/tmp/project/.agentworkforce/relay/relay.sock'; diff --git a/packages/cli/src/cli/lib/broker-lifecycle.test.ts b/packages/cli/src/cli/lib/broker-lifecycle.test.ts index 6926f556d..a83a1e466 100644 --- a/packages/cli/src/cli/lib/broker-lifecycle.test.ts +++ b/packages/cli/src/cli/lib/broker-lifecycle.test.ts @@ -184,6 +184,20 @@ describe('getBrokerStatusWithRetry', () => { expect(candidate.getStatus).toHaveBeenCalledTimes(4); expect(deps.sleep).toHaveBeenCalledTimes(3); }); + + it('does not retry a non-connect failure -- fails after a single attempt', async () => { + const err = new Error('unauthorized'); + const candidate: Pick = { + getStatus: vi.fn(async () => { + throw err; + }), + }; + const deps = createDeps(); + + await expect(getBrokerStatusWithRetry(candidate, deps)).rejects.toBe(err); + expect(candidate.getStatus).toHaveBeenCalledTimes(1); + expect(deps.sleep).not.toHaveBeenCalled(); + }); }); describe('readNodeDeliveryStatus', () => { diff --git a/packages/cli/src/cli/lib/broker-lifecycle.ts b/packages/cli/src/cli/lib/broker-lifecycle.ts index aa6847475..ae759e735 100644 --- a/packages/cli/src/cli/lib/broker-lifecycle.ts +++ b/packages/cli/src/cli/lib/broker-lifecycle.ts @@ -10,6 +10,7 @@ import type { CoreDependencies, CoreProjectPaths, CoreRelay, SpawnedProcess } fr import { track } from '../telemetry/index.js'; import { buildBundledAgentRelayMcpCommand } from './agent-relay-mcp-command.js'; import { errorClassName } from './telemetry-helpers.js'; +import { runSignalHandler } from './exit.js'; import { createTriggerSyncClient, resolveNodeCapacityHarnesses } from './fleet-sidecar.js'; import { discoverNodeConfigPath, @@ -397,11 +398,18 @@ function installStartupCrashGuard( const handleCrash = (err: unknown): void => { if (handled) return; handled = true; - void (async () => { + // `deps.exit(1)` throws `CliExit` (see `defaultExit`) rather than really + // exiting. Run this body through `runSignalHandler` -- the same wrapper + // `deps.onSignal` uses -- so that throw becomes a real, telemetry-flushed + // `process.exit`. Without it, the throw rejects this detached body with + // no awaiter; the `unhandledRejection` it produces hits `handleCrash` + // again, sees `handled` already true, and is silently dropped, leaving + // `runUpCommand` stuck in `holdOpen` instead of exiting. + runSignalHandler(async () => { await shutdownOnce().catch(() => undefined); reportBrokerStartFailure(err, deps, paths, options); deps.exit(1); - })(); + }); }; process.on('uncaughtException', handleCrash); process.on('unhandledRejection', handleCrash); @@ -488,7 +496,8 @@ export async function getBrokerStatusWithRetry( return await candidate.getStatus(); } catch (err) { lastError = err; - if (attempt >= STATUS_CHECK_MAX_ATTEMPTS) break; + const isConnectFailure = classifyBrokerStartStage(err, toErrorMessage(err)) === 'connect'; + if (!isConnectFailure || attempt >= STATUS_CHECK_MAX_ATTEMPTS) break; vlog( deps, verbose, @@ -505,11 +514,21 @@ export async function startBrokerWithPortFallback( basePort: number, deps: CoreDependencies, brokerName?: string, - verbose?: boolean + verbose?: boolean, + /** + * Invoked as soon as the broker child process has been spawned and its + * client handle exists, well before the handshake/status-check retries + * below resolve. Lets the caller wire up cleanup (e.g. a SIGTERM handler) + * against the real process immediately, instead of only after this whole + * function returns -- a signal arriving during the status check would + * otherwise find no handle to shut down and leak the broker child. + */ + onCandidateReady?: (candidate: CoreRelay) => void ): Promise<{ relay: CoreRelay; apiPort: number }> { if (basePort === 0) { vlog(deps, verbose, 'Asking the OS to assign the broker API port...'); const candidate = await deps.createRelay(paths.projectRoot, 0, brokerName, verbose); + onCandidateReady?.(candidate); try { await getBrokerStatusWithRetry(candidate, deps, verbose); if (!candidate.apiPort) { @@ -541,6 +560,7 @@ export async function startBrokerWithPortFallback( vlog(deps, verbose, 'Creating broker client (spawns broker process, waits for handshake)...'); const candidate = await deps.createRelay(paths.projectRoot, apiPort, brokerName, verbose); + onCandidateReady?.(candidate); vlog(deps, verbose, 'Broker client created. Checking broker status...'); try { @@ -1886,8 +1906,23 @@ export async function runUpCommand(options: UpOptions, deps: CoreDependencies): basePort, deps, options.brokerName, - options.verbose - ); + options.verbose, + // Assign `relay` as soon as the broker child process exists, not only + // once the handshake/status-check retries above also succeed. A + // SIGTERM/SIGINT arriving during that check window otherwise finds + // `relay` still null, so `shutdownOnce()` no-ops and leaks the broker + // child instead of shutting it down. + (candidate) => { + relay = candidate; + } + ).catch((err: unknown) => { + // On failure, `startBrokerWithPortFallback` has already shut down any + // candidate it created before rethrowing. Clear the early handle too + // so the outer catch's `shutdownOnce()` does not call `shutdown()` a + // second time on it. + relay = null; + throw err; + }); relay = started.relay; try { @@ -1985,10 +2020,20 @@ export async function runUpCommand(options: UpOptions, deps: CoreDependencies): await holdOpen; } } catch (err: unknown) { + // A rejection from cleanup must not swallow the original startup + // failure -- without this, `deps.exit(1)` below would never run and the + // startup error would go unreported. + try { + await shutdownOnce(); + } catch (cleanupError) { + deps.warn(`Failed to clean up after broker startup failure: ${describeErrorWithCause(cleanupError)}`); + } // A straggler process-level crash event for this same failure must not - // also fire and duplicate the report below. + // also fire and duplicate the report below. Marked here -- after + // cleanup, right before reporting -- rather than at catch-entry, so an + // unrelated crash during the shutdownOnce() cleanup above is not + // silently swallowed by this guard too. crashGuard.markHandled(); - await shutdownOnce(); reportBrokerStartFailure(err, deps, paths, options); deps.exit(1); } finally { From 1851e4e6d4aff225e5b566418af06f1619242009 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 14 Aug 2026 09:02:56 +0000 Subject: [PATCH 7/7] style: auto-format with Prettier --- packages/cli/src/cli/commands/core.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/cli/src/cli/commands/core.test.ts b/packages/cli/src/cli/commands/core.test.ts index 37ca18f6b..243c809be 100644 --- a/packages/cli/src/cli/commands/core.test.ts +++ b/packages/cli/src/cli/commands/core.test.ts @@ -1251,8 +1251,7 @@ describe('registerCoreCommands', () => { // down. for ( let i = 0; - i < 20 && - (relay.getStatus as unknown as { mock: { calls: unknown[][] } }).mock.calls.length === 0; + i < 20 && (relay.getStatus as unknown as { mock: { calls: unknown[][] } }).mock.calls.length === 0; i += 1 ) { await Promise.resolve();