From 821d3c6c93ed2baaff986f5b7e80148e30c0ccb2 Mon Sep 17 00:00:00 2001 From: Aman Varshney Date: Fri, 7 Aug 2026 17:01:27 +0530 Subject: [PATCH 1/2] fix(cli): make dev shutdown reliable Wait for file watchers to close before reporting shutdown complete, and let a second signal force termination when graceful cleanup stalls. Cover the shutdown controller with regression tests. Signed-off-by: Aman Varshney --- .../cli/src/dev/__tests__/run-dev.test.ts | 55 ++++++++- .../cli/src/dev/__tests__/watch.test.ts | 6 +- .../3-tooling/cli/src/dev/run-dev.ts | 106 +++++++++++++----- .../3-tooling/cli/src/dev/watch.ts | 7 +- 4 files changed, 139 insertions(+), 35 deletions(-) diff --git a/packages/0-framework/3-tooling/cli/src/dev/__tests__/run-dev.test.ts b/packages/0-framework/3-tooling/cli/src/dev/__tests__/run-dev.test.ts index 9c514c22c..82f2719d8 100644 --- a/packages/0-framework/3-tooling/cli/src/dev/__tests__/run-dev.test.ts +++ b/packages/0-framework/3-tooling/cli/src/dev/__tests__/run-dev.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test'; -import { renderFrontDoor } from '../run-dev.ts'; +import { createDevShutdownController, renderFrontDoor } from '../run-dev.ts'; describe('renderFrontDoor()', () => { test('starts with "[dev] ready:", then orders by address depth (fewest dots first), then lexicographic', () => { @@ -23,3 +23,56 @@ describe('renderFrontDoor()', () => { expect(renderFrontDoor([])).toEqual(['[dev] ready:']); }); }); + +describe('createDevShutdownController()', () => { + test('waits for graceful cleanup and ignores signals after it completes', async () => { + const cleanup = Promise.withResolvers(); + let cleanupCalls = 0; + const forcedExitCodes: number[] = []; + const shutdown = createDevShutdownController( + () => { + cleanupCalls += 1; + return cleanup.promise; + }, + (code) => forcedExitCodes.push(code), + ); + + let stopped = false; + void shutdown.done.then(() => { + stopped = true; + }); + + shutdown.handle('SIGINT'); + await Promise.resolve(); + expect(cleanupCalls).toBe(1); + expect(stopped).toBe(false); + + cleanup.resolve(); + await shutdown.done; + shutdown.handle('SIGINT'); + + expect(stopped).toBe(true); + expect(forcedExitCodes).toEqual([]); + }); + + test('a second signal during cleanup forces the conventional signal exit code', async () => { + for (const [signal, exitCode] of [ + ['SIGINT', 130], + ['SIGTERM', 143], + ] as const) { + const cleanup = Promise.withResolvers(); + const forcedExitCodes: number[] = []; + const shutdown = createDevShutdownController( + () => cleanup.promise, + (code) => forcedExitCodes.push(code), + ); + + shutdown.handle(signal); + shutdown.handle(signal); + + expect(forcedExitCodes).toEqual([exitCode]); + cleanup.resolve(); + await shutdown.done; + } + }); +}); diff --git a/packages/0-framework/3-tooling/cli/src/dev/__tests__/watch.test.ts b/packages/0-framework/3-tooling/cli/src/dev/__tests__/watch.test.ts index da9756027..4ce027337 100644 --- a/packages/0-framework/3-tooling/cli/src/dev/__tests__/watch.test.ts +++ b/packages/0-framework/3-tooling/cli/src/dev/__tests__/watch.test.ts @@ -64,7 +64,7 @@ describe('startWatch()', () => { await until(() => calls === 1, 2000); expect(calls).toBe(1); } finally { - watch.stop(); + await watch.stop(); fs.rmSync(dir, { recursive: true, force: true }); } }, 10_000); @@ -78,7 +78,7 @@ describe('startWatch()', () => { const watch = startWatch([{ address: 'a', paths: [file] }], () => { calls += 1; }); - watch.stop(); + await watch.stop(); fs.writeFileSync(file, 'a2'); await sleep(500); @@ -123,7 +123,7 @@ describe('startWatch()', () => { await until(() => calls === 2, 3000); expect(calls).toBe(2); } finally { - watch.stop(); + await watch.stop(); fs.rmSync(dir, { recursive: true, force: true }); } }, 10_000); diff --git a/packages/0-framework/3-tooling/cli/src/dev/run-dev.ts b/packages/0-framework/3-tooling/cli/src/dev/run-dev.ts index e8432aa76..e9b28cc57 100644 --- a/packages/0-framework/3-tooling/cli/src/dev/run-dev.ts +++ b/packages/0-framework/3-tooling/cli/src/dev/run-dev.ts @@ -37,6 +37,51 @@ function toCliError(error: unknown): CliError { : new CliError(error instanceof Error ? error.message : String(error)); } +type DevShutdownSignal = 'SIGINT' | 'SIGTERM'; + +export interface DevShutdownController { + /** Resolves after the graceful cleanup started by the first signal completes. */ + readonly done: Promise; + /** Starts graceful cleanup, or forces termination when cleanup is already in progress. */ + handle(signal: DevShutdownSignal): void; +} + +/** + * Makes the first signal graceful and the second decisive. A stuck watcher or + * attachment must never leave a dev process that swallows every later Ctrl-C. + */ +export function createDevShutdownController( + cleanup: () => Promise, + forceExit: (code: number) => void = (code) => process.exit(code), +): DevShutdownController { + let state: 'idle' | 'stopping' | 'stopped' = 'idle'; + let handle: (signal: DevShutdownSignal) => void = () => {}; + + const done = new Promise((resolve, reject) => { + handle = (signal) => { + if (state === 'stopped') return; + if (state === 'stopping') { + forceExit(signal === 'SIGINT' ? 130 : 143); + return; + } + + state = 'stopping'; + void cleanup().then( + () => { + state = 'stopped'; + resolve(); + }, + (error: unknown) => { + state = 'stopped'; + reject(error); + }, + ); + }; + }); + + return { done, handle }; +} + /** `[dev] ready:` then one line per endpoint, ordered by address depth (fewest dots first) then lexicographic. Exported for tests. */ export function renderFrontDoor( endpoints: readonly { readonly address: string; readonly url: string }[], @@ -255,38 +300,43 @@ export async function runDev(args: DevArgs, deps: DevRunDeps = {}): Promise((resolve) => { - let stopping = false; - - const finish = (): void => { - if (stopping) return; - stopping = true; - console.log("[dev] stopping — the app's services are stopping; emulators and data stay up."); - watch.stop(); - void (async () => { + const shutdown = createDevShutdownController(async () => { + console.log("[dev] stopping — the app's services are stopping; emulators and data stay up."); + await Promise.all([ + watch.stop(), + (async () => { for (const attachment of attachments) { await attachment.stopServices().catch(() => undefined); } - console.log('[dev] stopped.'); - resolve(); - })(); - }; - - // alchemy's own library code (imported transitively while loading the - // app's config/providers) registers its own process-level SIGINT/SIGTERM - // listeners for ITS OWN in-process resource bookkeeping — irrelevant - // here, since the actual converge runs in a separate spawned `alchemy` - // child process (run-alchemy.ts), never in this one. Left in place, - // whichever of its listeners runs first can call process.exit() - // synchronously and tear this process down before the watch loop's own - // async cleanup (stopping the app's services) ever gets a turn. This is - // this process's OWN signal handling from here on: strip whatever else - // is registered and become the only listener. - process.removeAllListeners('SIGINT'); - process.removeAllListeners('SIGTERM'); - process.on('SIGINT', finish); - process.on('SIGTERM', finish); + })(), + ]); + console.log('[dev] stopped.'); }); + const onSigint = (): void => shutdown.handle('SIGINT'); + const onSigterm = (): void => shutdown.handle('SIGTERM'); + + // alchemy's own library code (imported transitively while loading the + // app's config/providers) registers its own process-level SIGINT/SIGTERM + // listeners for ITS OWN in-process resource bookkeeping — irrelevant + // here, since the actual converge runs in a separate spawned `alchemy` + // child process (run-alchemy.ts), never in this one. Left in place, + // whichever of its listeners runs first can call process.exit() + // synchronously and tear this process down before the watch loop's own + // async cleanup (stopping the app's services) ever gets a turn. This is + // this process's OWN signal handling from here on: strip whatever else + // is registered and become the only listener. + process.removeAllListeners('SIGINT'); + process.removeAllListeners('SIGTERM'); + process.on('SIGINT', onSigint); + process.on('SIGTERM', onSigterm); + + try { + await shutdown.done; + } finally { + process.off('SIGINT', onSigint); + process.off('SIGTERM', onSigterm); + } + return 0; } diff --git a/packages/0-framework/3-tooling/cli/src/dev/watch.ts b/packages/0-framework/3-tooling/cli/src/dev/watch.ts index b97e9b0a5..40c66ad0a 100644 --- a/packages/0-framework/3-tooling/cli/src/dev/watch.ts +++ b/packages/0-framework/3-tooling/cli/src/dev/watch.ts @@ -49,7 +49,8 @@ export function watchTargetsFrom(bundles: Readonly>): { export interface WatchHandle { /** Resolves once chokidar's OS-level watches are attached — a change made before this can be missed entirely. Also resolves on `stop()` so an awaiting caller can never hang. */ readonly ready: Promise; - stop(): void; + /** Stops every OS-level watcher and resolves only after chokidar has released them. */ + stop(): Promise; } /** @@ -129,10 +130,10 @@ export function startWatch(targets: readonly WatchTarget[], onChange: () => void return { ready, - stop: () => { + stop: async () => { if (timer !== undefined) clearTimeout(timer); markReady(); - for (const watcher of watchers) void watcher.close(); + await Promise.all(watchers.map((watcher) => watcher.close())); }, }; } From 6606459e3d94b92384d7c9bd90f19168300f6d37 Mon Sep 17 00:00:00 2001 From: Aman Varshney Date: Tue, 15 Sep 2026 00:52:07 +0530 Subject: [PATCH 2/2] fix: align dev cleanup reporting and verify startup rollback Signed-off-by: Aman Varshney --- .../3-tooling/cli/src/dev/watch.ts | 2 +- .../3-tooling/cli/src/family/commands/dev.ts | 2 +- .../operations/__tests__/operations.test.ts | 85 +++++++++++-------- .../3-tooling/cli/src/operations/dev.ts | 8 +- .../cli/src/operations/execute-dev.ts | 5 +- 5 files changed, 55 insertions(+), 47 deletions(-) diff --git a/packages/0-framework/3-tooling/cli/src/dev/watch.ts b/packages/0-framework/3-tooling/cli/src/dev/watch.ts index e410f5b50..3209716a7 100644 --- a/packages/0-framework/3-tooling/cli/src/dev/watch.ts +++ b/packages/0-framework/3-tooling/cli/src/dev/watch.ts @@ -49,7 +49,7 @@ export function watchTargetsFrom(bundles: Readonly>): { export interface WatchHandle { /** Resolves once chokidar's OS-level watches are attached — a change made before this can be missed entirely. Also resolves on `stop()` so an awaiting caller can never hang. */ readonly ready: Promise; - /** Stops every OS-level watcher and resolves only after chokidar has released them. */ + /** Awaits every watcher close; rejects with an AggregateError if any close fails. */ stop(): Promise; } diff --git a/packages/0-framework/3-tooling/cli/src/family/commands/dev.ts b/packages/0-framework/3-tooling/cli/src/family/commands/dev.ts index d3cbc6e20..ed750bc8a 100644 --- a/packages/0-framework/3-tooling/cli/src/family/commands/dev.ts +++ b/packages/0-framework/3-tooling/cli/src/family/commands/dev.ts @@ -152,7 +152,7 @@ function reportDevEvent( report({ kind: 'message', severity: 'warn', - text: `A service refused to stop: ${event.message}`, + text: `Dev cleanup failed: ${event.message}`, }); return; diff --git a/packages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.ts b/packages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.ts index ab6d8f244..127efc768 100644 --- a/packages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.ts +++ b/packages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.ts @@ -22,6 +22,7 @@ import type { import type { LocalTargetAttachment, LocalTargetDescriptor } from '@internal/core/local-target'; import * as Layer from 'effect/Layer'; import { DEPLOYMENT_RESULT_FILE_ENV, type DeploymentSummary } from '../../deployment-summary.ts'; +import * as Watch from '../../dev/watch.ts'; import type { AppIdentity } from '../../pipeline.ts'; import type { AlchemyInvocation } from '../../run-alchemy.ts'; import { deployWithDeps } from '../deploy.ts'; @@ -909,42 +910,48 @@ describe.skipIf(process.platform === 'win32')('dev()', () => { expect(stops).toBe(1); }, 15_000); - test('a startServices that throws mid-start is rolled back: the partially-started attachment is stopped again', async () => { - const app = makeAppDir('hello-dev'); - let stops = 0; - const attachment: LocalTargetAttachment = { - // Models a partial start: some services came up before the throw, so - // the rollback must stop this attachment even though startServices - // never returned. - startServices: () => Promise.reject(new Error('service two failed to bind its port')), - stopServices: () => { - stops += 1; - return Promise.resolve(); - }, - endpoints: () => Promise.resolve([]), - logs: async function* () {}, - }; - - const result = await silently(() => - devWithDeps( - { - entry: app.entryPath, - cwd: app.dir, - }, - { - config: devConfigWith(attachment), - runAssembler: fakeAssembler, - alchemy: async () => ({ exitCode: 0, signal: null }), + test.each(['success', 'sync failure', 'async failure'] as const)( + 'a partial start is rolled back and preserves its failure after cleanup %s', + async (cleanup) => { + const app = makeAppDir('hello-dev'); + let stops = 0; + const attachment: LocalTargetAttachment = { + // Models a partial start: some services came up before the throw, so + // the rollback must stop this attachment even though startServices + // never returned. + startServices: () => Promise.reject(new Error('service two failed to bind its port')), + stopServices: () => { + stops += 1; + if (cleanup === 'sync failure') throw new Error('cleanup failed'); + if (cleanup === 'async failure') return Promise.reject(new Error('cleanup failed')); + return Promise.resolve(); }, - ), - ); + endpoints: () => Promise.resolve([]), + logs: async function* () {}, + }; - expect(result.ok).toBe(false); - if (result.ok) throw new Error('unreachable'); - expect(result.failure.code).toBe('DEV.SERVICE_START_FAILED'); - expect(result.failure.message).toBe('service two failed to bind its port'); - expect(stops).toBe(1); - }, 15_000); + const result = await silently(() => + devWithDeps( + { + entry: app.entryPath, + cwd: app.dir, + }, + { + config: devConfigWith(attachment), + runAssembler: fakeAssembler, + alchemy: async () => ({ exitCode: 0, signal: null }), + }, + ), + ); + + expect(result.ok).toBe(false); + if (result.ok) throw new Error('unreachable'); + expect(result.failure.code).toBe('DEV.SERVICE_START_FAILED'); + expect(result.failure.message).toBe('service two failed to bind its port'); + expect(stops).toBe(1); + }, + 15_000, + ); test.each(['synchronous', 'asynchronous'] as const)( 'stop() surfaces a %s cleanup failure and still finishes', @@ -992,7 +999,11 @@ describe.skipIf(process.platform === 'win32')('dev()', () => { async (phase) => { const app = makeAppDir('shutdown-race'); const watched = path.join(app.dir, 'output.txt'); - fs.writeFileSync(watched, 'before'); + let triggerChange = () => {}; + const watch = spyOn(Watch, 'startWatch').mockImplementation((_targets, onChange) => { + triggerChange = onChange; + return { ready: Promise.resolve(), stop: async () => {} }; + }); let resume = () => {}; const blockedWork = new Promise((resolve) => { resume = resolve; @@ -1039,10 +1050,10 @@ describe.skipIf(process.platform === 'win32')('dev()', () => { return { exitCode: 0, signal: null }; }, }, - ); + ).finally(() => watch.mockRestore()); if (!start.ok) throw new Error('expected a started session'); try { - fs.writeFileSync(watched, 'after'); + triggerChange(); const deadline = Date.now() + 5000; while (!entered && rebuildFailure === undefined && Date.now() < deadline) { await new Promise((resolve) => setTimeout(resolve, 25)); diff --git a/packages/0-framework/3-tooling/cli/src/operations/dev.ts b/packages/0-framework/3-tooling/cli/src/operations/dev.ts index 2be74c445..eb68e1345 100644 --- a/packages/0-framework/3-tooling/cli/src/operations/dev.ts +++ b/packages/0-framework/3-tooling/cli/src/operations/dev.ts @@ -2,7 +2,7 @@ * The programmatic `dev` operation (`@prisma/composer/control`): typed input, * events out through `onEvent`, lifetime owned by the returned DevSession — * no argv, no console, no process.exit, and NEVER any process signal - * handling (the host owns signals; the CLI adapter dev/run-dev.ts shows the + * handling (the host owns signals; the CLI adapter family/commands/dev.ts shows the * pattern). The executor loads lazily, so importing this module executes * nothing; an executor that fails to load comes back as a structured * failure, never a throw out of the host. @@ -26,7 +26,7 @@ export type DevEvent = readonly cwd: string; } | { readonly kind: 'stopping' } - /** One service refused to stop during stop(); teardown continues and `stopped` still follows. */ + /** Watcher or service cleanup failed during stop(); teardown continues and `stopped` still follows. */ | { readonly kind: 'stop-error'; readonly message: string } | { readonly kind: 'stopped' }; @@ -38,9 +38,7 @@ export interface DevInput { readonly onEvent?: ((event: DevEvent) => void) | undefined; } -/** A running dev session. The operation NEVER touches process signal handlers — - * the host owns signals (and must evict alchemy's import-time SIGINT/SIGTERM - * listeners before installing its own; see run-dev.ts). */ +/** A running dev session. The host owns process signals and calls stop() to shut down. */ export interface DevSession { /** The initial front door, already merged across attachments. */ readonly endpoints: readonly ServiceEndpoint[]; diff --git a/packages/0-framework/3-tooling/cli/src/operations/execute-dev.ts b/packages/0-framework/3-tooling/cli/src/operations/execute-dev.ts index 312b299e6..5da572268 100644 --- a/packages/0-framework/3-tooling/cli/src/operations/execute-dev.ts +++ b/packages/0-framework/3-tooling/cli/src/operations/execute-dev.ts @@ -1,8 +1,7 @@ /** - * The dev executor — run-dev.ts's pipeline (local-dev spec § 6) with console - * and signal handling removed: events out through `onEvent`, lifetime owned by + * The dev executor (local-dev spec § 6): events out through `onEvent`, lifetime owned by * the returned DevSession. The operation NEVER touches process signal - * handlers — the host does (see run-dev.ts). Reached only by lazy import + * handlers — the host does (see family/commands/dev.ts). Reached only by lazy import * from dev.ts — this module's static graph transitively loads alchemy's * provider tree, so the control entry must never import it statically. */