From 280663b37e003fae51c532fcbc392c08a0abf896 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 04:34:35 +0000 Subject: [PATCH 1/2] Fix #84: run shutdown on normal Ctrl+C quit, not just SIGINT/SIGTERM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raw mode (createCliRenderer) disables signal generation, so an interactive Ctrl+C never reaches Node as a real SIGINT — OpenTUI's own exitOnCtrlC handling calls CliRenderer.destroy() directly instead, which never fires SIGINT and therefore never ran wireProcessExit's shutdown(): layout state was never flushed and extensions never deactivated on a normal quit. destroy()'s finalizeDestroy() never calls process.exit (verified against the pinned @opentui/core@0.1.107 bundle) — the process exits only once the event loop drains — so main.ts now wires shutdown() to createCliRenderer's onDestroy config callback (renderShell.tsx's new ShellRenderDeps.onDestroy), chosen over the CliRenderEvents.DESTROY event because onDestroy fires at the very end of teardown and is already guarded by the library's own try/catch, whereas the DESTROY event fires mid-teardown through a plain EventEmitter.emit with no such protection. exitOnCtrlC itself is untouched, so Ctrl+C can never become unquittable. wireProcessExit's shutdown is refactored from a boolean-flag guard to a memoized promise (createShutdown) so a signal racing in while the destroy hook's teardown is still in flight awaits the same real completion instead of resolving early and letting process.exit(0) cut it off. It's now also bounded by a 2s timeout (SHUTDOWN_TIMEOUT_MS) using the same injectable ChordScheduler seam keymap/chords.ts already established, logging rather than hanging if a dispose() ever gets stuck. runTecode's headless-exit path now calls the same shutdown() instead of re-listing every dispose() call a second time. Adds requirements.md Req 12.3 and a design.md §3 shutdown point; tests (packages/cli/src/shutdownOnDestroy.test.ts) exercise the seam rather than a real terminal: a subprocess fixture proves runTecode really wires onDestroy to shutdown() (observed via layoutState's flush landing on disk, synchronized on theme.select's command-registry disposal rather than racing the layout debounce timer), plus direct createShutdown tests for idempotency in both destroy/signal orderings and the timeout path. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- design.md | 1 + packages/cli/src/main.ts | 288 ++++++++++++++++---- packages/cli/src/renderShell.tsx | 41 ++- packages/cli/src/shutdownOnDestroy.test.ts | 297 +++++++++++++++++++++ requirements.md | 1 + 5 files changed, 575 insertions(+), 53 deletions(-) create mode 100644 packages/cli/src/shutdownOnDestroy.test.ts diff --git a/design.md b/design.md index 3108f4c..c0029fb 100644 --- a/design.md +++ b/design.md @@ -72,6 +72,7 @@ Order (*Req 12.1*), with first paint before extension code runs (*Req 12.2*): 1. **Synchronous, before first frame (< 100 ms budget):** parse argv; load user + workspace settings and keybindings (JSONC); detect terminal capabilities (Kitty protocol, color depth); build the theme from the configured theme's cached JSON (themes-default's JSON files are embedded assets, so no extension activation is needed to paint); render the Shell with empty slots and a placeholder editor area. 2. **Deferred (queueMicrotask after first frame):** discover extensions and validate manifests; register declared contributions; fire `onStartup` activations; open the file/directory from argv (which fires `onLanguage:*` activations). 3. Manifest reading executes `manifest.ts` as a module (it is TypeScript), but manifests are constrained by convention and validation to be pure data (`export default {...} satisfies Manifest`); the extension's `index.ts` is not imported until activation (*Req 2.2*). +4. **Shutdown** (*Req 12.3*, Issue #84): `createCliRenderer()` puts stdin in raw mode, which disables signal generation, so an interactive Ctrl+C never reaches Node as a real `SIGINT` — OpenTUI's own `exitOnCtrlC` key handling (default `true`) intercepts the byte itself and calls `CliRenderer.destroy()` directly. `destroy()`'s `finalizeDestroy()` never calls `process.exit` — the process exits only once the event loop drains — so `main.ts` wires its shutdown sequence to `createCliRenderer`'s `onDestroy` config callback (`renderShell.tsx`'s `ShellRenderDeps.onDestroy`), which fires synchronously from inside that teardown with nothing left afterward to catch a throw; `onDestroy` was chosen over subscribing to the `CliRenderEvents.DESTROY` event (which this module already uses for `CAPABILITIES`) because `finalizeDestroy()` emits that event partway through its own teardown, via a plain `EventEmitter.emit` with no try/catch, whereas `onDestroy` fires at the very end and is already wrapped in the library's own try/catch. `SIGINT`/`SIGTERM` (the paths that fire when stdin is NOT in raw mode: `kill`, a supervising shell, headless mode) call the exact same shutdown sequence. The sequence itself (`main.ts`'s `createShutdown`) is a single memoized promise shared by every trigger — so whichever fires first runs the real teardown (flush layout state, dispose every core-owned service, deactivate every extension) and whichever fires afterward, or concurrently, awaits that same settling promise rather than racing a `process.exit(0)` ahead of it — raced against a bounded timeout (a couple of seconds) so a hung disposal degrades to a logged warning instead of an unquittable process. ## 4. Extension Host diff --git a/packages/cli/src/main.ts b/packages/cli/src/main.ts index dbd11af..36e542e 100644 --- a/packages/cli/src/main.ts +++ b/packages/cli/src/main.ts @@ -51,6 +51,7 @@ import { wireEditorLangIdContext, wireThemeConfigSync, type BindingTable, + type ChordScheduler, type ChordStateMachine, type CommandRegistry, type ConfigService, @@ -1107,43 +1108,217 @@ export interface RunTecodeOptions { configDir?: string; } -/** Sets up graceful-shutdown handling (Phase 3's "wire process-exit - * disposeAll"). A synchronous Node/Bun `"exit"` handler cannot await async - * cleanup, so this listens for `SIGINT`/`SIGTERM` instead — the standard - * pattern for a CLI that needs to flush/dispose before actually exiting — - * and calls `process.exit(0)` itself once cleanup settles. Idempotent: a - * second signal while shutdown is already in flight is a no-op. */ -function wireProcessExit(root: AssemblyRoot): void { - let shuttingDown = false; - const shutdown = async (): Promise => { - if (shuttingDown) return; - shuttingDown = true; - await root.layoutState.flush(); - root.config.dispose(); - root.chordPendingIndicator.dispose(); - root.chordMachine.dispose(); - root.findService.dispose(); - root.editorSession.dispose(); - root.editorLangIdSync.dispose(); - root.themeConfigSync.dispose(); - root.themeSelectCommand.dispose(); - root.openFileCommand.dispose(); - root.tabCommands.dispose(); - root.extensionsReloadCommand.dispose(); - root.keybindingsCommands.dispose(); - root.modalCommands.dispose(); - root.modalService.dispose(); - root.windowMessageService.dispose(); - root.hostErrorSink.dispose(); - root.highlightService.dispose(); - root.languageRegistry.dispose(); - await root.hostRef.current?.disposeAll(); +/** The bounded wait {@link createShutdown} allows its teardown sequence + * before giving up on it (Req 12.3, design.md §3's shutdown point): "a + * couple of seconds", per this task's requirement that a hung `dispose()`/ + * `flush()` must never prevent the process from exiting — that would + * reintroduce exactly the "editor cannot quit" risk `onDestroy` (over + * `exitOnCtrlC: false`) was chosen to avoid (this module's `runTecode` + * TSDoc on that seam). Exported so a test can assert against it rather + * than a magic number. */ +export const SHUTDOWN_TIMEOUT_MS = 2_000; + +/** The default {@link ChordScheduler}, backed by the global `setTimeout`/ + * `clearTimeout` — the exact shape `keymap/chords.ts`'s own (unexported) + * `createDefaultScheduler` uses, duplicated here rather than imported + * because that one is private to `chords.ts`; the *type* is still the + * shared, already-proven injectable-timer seam (this module's + * `ShutdownDeps.scheduler` TSDoc). */ +function createDefaultShutdownScheduler(): ChordScheduler { + return { + set(fn, ms) { + return setTimeout(fn, ms); + }, + clear(handle) { + clearTimeout(handle as ReturnType); + }, + }; +} + +/** Exactly what {@link createShutdown}'s teardown sequence calls on each + * {@link AssemblyRoot} field it touches — narrowed per house convention + * ("narrowing, not re-implementing", `ChordStateMachineDeps.table`'s + * `Pick` is the same + * pattern) down to the single method each field is used for, so a test + * can hand-roll a fake root with ~20 one-method objects instead of the + * full {@link AssemblyRoot} (every real `AssemblyRoot` field already + * structurally satisfies this — no cast needed at `wireProcessExit`'s own + * call site). */ +export interface ShutdownRoot { + log: Pick; + layoutState: Pick; + config: Pick; + chordPendingIndicator: Pick; + chordMachine: Pick; + findService: Pick; + editorSession: Pick; + editorLangIdSync: Pick; + themeConfigSync: Pick; + themeSelectCommand: Pick; + openFileCommand: Pick; + tabCommands: Pick; + extensionsReloadCommand: Pick; + keybindingsCommands: Pick; + modalCommands: Pick; + modalService: Pick; + windowMessageService: Pick; + hostErrorSink: Pick; + highlightService: Pick; + languageRegistry: Pick; + hostRef: { current?: Pick }; +} + +/** Dependencies {@link createShutdown} reports through rather than owning + * directly (design.md §5, §14's injected-collaborator pattern, applied + * here exactly as `keymap/chords.ts`'s `ChordStateMachineDeps.scheduler` + * applies it to the chord-pending timeout). */ +export interface ShutdownDeps { + /** Defaults to the global `setTimeout`/`clearTimeout`. Tests inject a + * fake so the {@link SHUTDOWN_TIMEOUT_MS} bound can be proven without an + * actual multi-second wait. */ + scheduler?: ChordScheduler; + /** Defaults to {@link SHUTDOWN_TIMEOUT_MS}. Overridable only for tests — + * production always uses the real budget. */ + timeoutMs?: number; +} + +/** + * Build the single, shared, idempotent, bounded shutdown sequence (Req + * 12.3, design.md §3's shutdown point; Phase 3's original "wire + * process-exit disposeAll"): flush layout state (Req 6.4), dispose every + * startup-owned subscription/service {@link AssemblyRoot}'s own field + * TSDocs point back at this function for, then deactivate every extension + * (`hostRef.current?.disposeAll()`, Req 2.6). + * + * **Why a memoized promise, not a boolean flag**: the previous + * implementation (`shuttingDown` boolean, "second call is a no-op") made a + * SECOND caller's returned promise resolve immediately, even while the + * FIRST call's teardown was still genuinely in flight — safe when the + * only two callers were `SIGINT`/`SIGTERM` racing each other, but wrong + * now that a normal Ctrl+C quit calls this from OpenTUI's `onDestroy` + * (Req 12.3) as well: `SIGTERM` arriving a moment after `onDestroy` fires + * would otherwise `process.exit(0)` before layout state actually + * finished flushing. Returning the SAME in-flight promise to every caller + * — regardless of which of `onDestroy`/`SIGINT`/`SIGTERM` triggered it, + * or in which order — fixes that: every caller's `.finally(() => + * process.exit(0))` now genuinely waits for the one real teardown to + * settle (or time out, below). + * + * **Why a timeout at all**: `onDestroy` is the seam this codebase picked + * specifically because it never risks making the editor unquittable + * (`runTecode`'s TSDoc) — `CliRenderer.finalizeDestroy()` never calls + * `process.exit` itself, so the process exits only once the event loop + * drains, which this function's own pending `flush()`/`dispose()` I/O is + * what keeps it alive long enough to run. A `flush()`/`dispose()` that + * hangs would therefore hang the whole process with no UI left to show + * for it — reintroducing exactly the "cannot quit" risk this design + * otherwise avoids. Racing the real work against {@link SHUTDOWN_TIMEOUT_MS} + * means a hang degrades to "some late writes/disposals may not have + * finished" (logged as a warning) instead of "the process never exits". + * The real work is never cancelled — only no longer awaited — so a + * teardown that finishes just after the timeout still finishes. + * + * Never throws: every `dispose()` above is already documented + * never-throwing (design.md §5, §14's guarded-boundary convention, + * `AssemblyRoot`'s own field TSDocs), but this wraps the whole sequence in + * try/catch anyway per that same convention, logging rather than + * propagating — this is, after all, the function OpenTUI's synchronous, + * un-awaited `onDestroy` callback invokes fire-and-forget, with nothing + * downstream to catch a rejection. + */ +export function createShutdown(root: ShutdownRoot, deps: ShutdownDeps = {}): () => Promise { + const scheduler = deps.scheduler ?? createDefaultShutdownScheduler(); + const timeoutMs = deps.timeoutMs ?? SHUTDOWN_TIMEOUT_MS; + + let shutdownPromise: Promise | undefined; + + async function performShutdown(): Promise { + try { + await root.layoutState.flush(); + root.config.dispose(); + root.chordPendingIndicator.dispose(); + root.chordMachine.dispose(); + root.findService.dispose(); + root.editorSession.dispose(); + root.editorLangIdSync.dispose(); + root.themeConfigSync.dispose(); + root.themeSelectCommand.dispose(); + root.openFileCommand.dispose(); + root.tabCommands.dispose(); + root.extensionsReloadCommand.dispose(); + root.keybindingsCommands.dispose(); + root.modalCommands.dispose(); + root.modalService.dispose(); + root.windowMessageService.dispose(); + root.hostErrorSink.dispose(); + root.highlightService.dispose(); + root.languageRegistry.dispose(); + await root.hostRef.current?.disposeAll(); + } catch (cause) { + root.log.append("error", { + message: `shutdown: teardown threw: ${describeError(cause)}`, + }); + } + } + + return function shutdown(): Promise { + if (shutdownPromise) return shutdownPromise; + const work = performShutdown(); + shutdownPromise = new Promise((resolve) => { + let settled = false; + const timer = scheduler.set(() => { + if (settled) return; + settled = true; + root.log.append("warning", { + message: `shutdown: exceeded ${timeoutMs}ms; continuing to exit without waiting further (teardown keeps running in the background)`, + }); + resolve(); + }, timeoutMs); + void work.then(() => { + if (settled) return; + settled = true; + scheduler.clear(timer); + resolve(); + }); + }); + return shutdownPromise; }; +} + +/** What {@link wireProcessExit} produced. */ +export interface ProcessExitWiring { + /** The shared, idempotent, bounded shutdown sequence (this module's + * {@link createShutdown} TSDoc) — call it from any additional quit path + * a caller wires up (Req 12.3's `onDestroy`, below); `SIGINT`/`SIGTERM` + * already call it internally. */ + shutdown: () => Promise; +} + +/** Sets up graceful-shutdown handling (Phase 3's "wire process-exit + * disposeAll", extended by Req 12.3 for the normal-quit path): builds the + * shared {@link createShutdown} sequence and registers it on `SIGINT`/ + * `SIGTERM` — the only two paths that fire when stdin is NOT in raw mode + * (`kill`, a signal from a supervising shell, headless mode's no-TTY runs) + * — then calls `process.exit(0)` itself once shutdown settles. A + * synchronous Node/Bun `"exit"` handler cannot await async cleanup, hence + * signals rather than that. + * + * Returns the SAME `shutdown` this wires to `SIGINT`/`SIGTERM` (Req 12.3) + * so `runTecode` can *also* hand it to the render seam's `onDestroy` + * callback (`renderShell.tsx`'s `ShellRenderDeps.onDestroy`) — the path + * that actually fires on an interactive Ctrl+C quit, per this module's + * `runTecode` TSDoc on why `SIGINT` never does. Both paths share one + * `createShutdown` instance, so whichever fires first runs the real + * teardown and whichever fires second (or both, racing) gets the exact + * same settled/settling promise back (`createShutdown`'s TSDoc). */ +function wireProcessExit(root: AssemblyRoot, deps: ShutdownDeps = {}): ProcessExitWiring { + const shutdown = createShutdown(root, deps); for (const signal of ["SIGINT", "SIGTERM"] as const) { process.once(signal, () => { void shutdown().finally(() => process.exit(0)); }); } + return { shutdown }; } /** What {@link runTecode} produced, for a non-headless (real) run — a @@ -1254,7 +1429,7 @@ export async function runTecode( // `loadContributions` settles). applyConfiguredTheme(root.config, root.themeService); - wireProcessExit(root); + const { shutdown } = wireProcessExit(root); const renderShell = options.renderShell ?? (headless ? renderShellHeadless : renderShellToTerminal); await renderShell({ @@ -1292,6 +1467,26 @@ export async function runTecode( }); void root.applyKittyKeyboardVerdict(isKittyCapable); }, + // Issue #84 / Req 12.3: `createCliRenderer()`'s raw mode disables + // signal generation, so a normal interactive Ctrl+C quit never fires + // `SIGINT` — OpenTUI's own `exitOnCtrlC` handling calls + // `CliRenderer.destroy()` directly instead, which (per + // `renderShellToTerminal`'s wiring of this callback into + // `createCliRenderer`'s `onDestroy` config) invokes this synchronously + // and never calls `process.exit` itself. `shutdown` is the SAME + // shared, idempotent, timeout-bounded sequence `SIGINT`/`SIGTERM` + // already call (`wireProcessExit`'s TSDoc) — fire-and-forget (`void`) + // is correct and sufficient here: its own pending `flush()`/ + // `dispose()` I/O keeps the event loop alive long enough to finish (or + // hit `SHUTDOWN_TIMEOUT_MS` and give up), and once it settles with + // nothing else scheduled, the process exits on its own with no + // `process.exit` call needed from here (`createShutdown`'s TSDoc). + // Never throws: `shutdown()` itself is guarded (same TSDoc), so this + // callback can't propagate into OpenTUI's `_onDestroy` invocation + // either (which wraps it in try/catch anyway — belt and suspenders). + onDestroy: () => { + void shutdown(); + }, }); const firstFrameMs = performance.now() - startedAt; @@ -1355,26 +1550,15 @@ export async function runTecode( logErrors: postDeferredLogCounts.errors - preDeferredLogCounts.errors, ms: performance.now() - startedAt, }); - await root.layoutState.flush(); - root.config.dispose(); - root.chordPendingIndicator.dispose(); - root.chordMachine.dispose(); - root.findService.dispose(); - root.editorSession.dispose(); - root.editorLangIdSync.dispose(); - root.themeConfigSync.dispose(); - root.themeSelectCommand.dispose(); - root.openFileCommand.dispose(); - root.tabCommands.dispose(); - root.extensionsReloadCommand.dispose(); - root.keybindingsCommands.dispose(); - root.modalCommands.dispose(); - root.modalService.dispose(); - root.windowMessageService.dispose(); - root.hostErrorSink.dispose(); - root.highlightService.dispose(); - root.languageRegistry.dispose(); - await deferred.extensionHost.disposeAll(); + // The exact same shared, idempotent, timeout-bounded sequence + // `wireProcessExit` already wired to `SIGINT`/`SIGTERM` above (and, in + // the non-headless case, to the render seam's `onDestroy` — Req 12.3) + // — reused here rather than re-listing all 15+ dispose calls a second + // time, so this can never silently drift out of sync with `shutdown`'s + // own list (`createShutdown`'s TSDoc). `deferred.extensionHost` and + // `root.hostRef.current` are the SAME object by this point + // (`runDeferredPhase`'s "Fulfills every forward reference" comment). + await shutdown(); process.exit(0); } diff --git a/packages/cli/src/renderShell.tsx b/packages/cli/src/renderShell.tsx index c9a074f..5baafd1 100644 --- a/packages/cli/src/renderShell.tsx +++ b/packages/cli/src/renderShell.tsx @@ -133,6 +133,41 @@ export interface ShellRenderDeps { * TSDoc's "First frame for a headless run"). */ onCapabilitiesResolved?: (capabilitiesValue: unknown) => void; + /** + * Fires once the real `CliRenderer` has been destroyed (Issue #84, Req + * 12.3) — wired straight through to `createCliRenderer`'s own + * `onDestroy` config callback ({@link renderShellToTerminal}, below). + * This is the ONLY reliable "the editor is quitting" signal for an + * interactive Ctrl+C: `createCliRenderer()` puts stdin in raw mode, + * which disables signal generation, so Ctrl+C never reaches Node as a + * real `SIGINT` — OpenTUI's own `exitOnCtrlC` key handling (default + * `true`) intercepts the `\x03` byte itself and calls + * `CliRenderer.destroy()` directly, bypassing `SIGINT` entirely. + * + * `destroy()`'s `finalizeDestroy()` never calls `process.exit` (checked + * against the pinned `@opentui/core@0.1.107` bundle) — the process exits + * naturally once the event loop drains, so `main.ts`'s `runTecode` can + * safely start async cleanup here (`shutdown()`) and let its own + * pending I/O keep the process alive until that cleanup genuinely + * finishes, with no `process.exit` call needed from this callback. + * `onDestroy` (a plain `CliRendererConfig` field) was chosen over + * subscribing to the `CliRenderEvents.DESTROY` event this module + * already uses for `CAPABILITIES` below: `finalizeDestroy()` emits + * `"destroy"` BEFORE it finishes tearing down the renderable tree/ + * console/native renderer, via a plain `EventEmitter.emit` that does + * NOT catch a throwing listener — and by the time it emits, `destroy()` + * has already removed the renderer's own `uncaughtException` handler + * (`cleanupBeforeDestroy()` runs first), so a listener that threw would + * escape uncaught and abort the rest of that teardown. `onDestroy`, by + * contrast, runs at the very END of `finalizeDestroy()` (after the + * renderer has fully torn itself down) and is already wrapped in its + * OWN try/catch internally — a second, library-provided guard on top of + * this callback's own (`runTecode`'s TSDoc on why it never throws + * either). Optional and never required: {@link renderShellHeadless} + * never calls this (no real `CliRenderer` exists to destroy), matching + * every other optional dependency in this module. + */ + onDestroy?: () => void; } /** The render seam's shape: resolves once "first frame" has happened (see @@ -157,7 +192,11 @@ export type RenderShell = (deps: ShellRenderDeps) => Promise; * so `idle()` cannot hang here. */ export const renderShellToTerminal: RenderShell = async (deps) => { - const renderer = await createCliRenderer(); + // `onDestroy: deps.onDestroy` (Issue #84, Req 12.3, this module's + // `ShellRenderDeps.onDestroy` TSDoc): `undefined` when the caller + // doesn't supply one, which `createCliRenderer` treats identically to + // the field being omitted entirely. + const renderer = await createCliRenderer({ onDestroy: deps.onDestroy }); const root = createRoot(renderer); root.render( diff --git a/packages/cli/src/shutdownOnDestroy.test.ts b/packages/cli/src/shutdownOnDestroy.test.ts new file mode 100644 index 0000000..2afc194 --- /dev/null +++ b/packages/cli/src/shutdownOnDestroy.test.ts @@ -0,0 +1,297 @@ +/** + * Shutdown-on-normal-quit tests (Issue #84, Req 12.3, design.md §3's + * shutdown point). + * + * A real interactive Ctrl+C quit cannot be exercised here: `createCliRenderer` + * needs a real TTY, which bun test's sandboxed stdout never provides, and + * `main.ts` forces the no-op `renderShellHeadless` whenever stdout isn't + * one (`renderShell.test.ts`'s own TSDoc makes the identical call for + * `renderShellToTerminal`). So these tests exercise the SEAM instead of + * the terminal: + * + * - The first test spawns a real `bun` subprocess (matching + * `main.integration.test.ts`'s own spawn-and-parse pattern) running a + * small fixture that calls `runTecode` with its `renderShell` seam + * overridden to CAPTURE, rather than open, the real `onDestroy` + * callback, then fires that captured callback directly — proving + * `runTecode` really does wire `onDestroy` to `shutdown()` end to end, + * against a REAL `layoutState.flush()` write to disk. A genuine + * subprocess (not just an env-var mutation of THIS test's own process) + * is required here, not merely for isolation: Bun's `os.homedir()` — + * which `getUserLayoutStatePath()` depends on — is resolved once at + * process start and does not observe a later `process.env.HOME` + * mutation, so only a fresh process actually started with `HOME` + * pointed at a temp directory lets this test observe the write without + * touching the real machine's `~/.config/tecode/state.json`. + * - The remaining tests exercise `createShutdown` — the shared, memoized, + * timeout-bounded sequence `wireProcessExit` builds (`main.ts`'s TSDoc) + * — directly, against a hand-rolled fake `ShutdownRoot`, for the + * idempotency and timeout guarantees that don't need a real terminal, a + * real filesystem, or even a real `AssemblyRoot`. + */ + +import { expect, setDefaultTimeout, test } from "bun:test"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { createHostLog, type ChordScheduler } from "@tecode/core"; +import { createShutdown, SHUTDOWN_TIMEOUT_MS, type ShutdownRoot } from "./main"; + +// Spawning bun as a subprocess (cold module resolution/transpilation) can +// exceed bun:test's 5s default — matches `main.integration.test.ts`'s own +// reasoning for the identical call. +setDefaultTimeout(30_000); + +/** A fake scheduler (matches `keymap/chords.test.ts`'s/ + * `ui/chordPendingIndicator.test.ts`'s own): `fire()` runs every + * still-armed timeout synchronously, so `SHUTDOWN_TIMEOUT_MS`'s real + * multi-second wait never has to elapse in a test. */ +function createFakeScheduler(): ChordScheduler & { fire(): void } { + let nextHandle = 0; + const pending = new Map void>(); + return { + set(fn) { + const handle = nextHandle++; + pending.set(handle, fn); + return handle; + }, + clear(handle) { + pending.delete(handle as number); + }, + fire() { + const callbacks = Array.from(pending.values()); + pending.clear(); + for (const cb of callbacks) cb(); + }, + }; +} + +/** A hand-rolled fake {@link ShutdownRoot}: every disposable just counts + * its own call into `calls`, so a test can invoke the returned + * `shutdown()` any number of times, in any order relative to other + * callers, and assert the real sequence still only ran once. + * `flush` is overridable so the timeout test can hand it a `Promise` that + * never settles, matching a genuinely hung `layoutState.flush()`. */ +function createFakeShutdownRoot(overrides: { flush?: () => Promise } = {}): { + root: ShutdownRoot; + log: ReturnType; + calls: { flush: number; dispose: number; disposeAll: number }; +} { + const calls = { flush: 0, dispose: 0, disposeAll: 0 }; + const log = createHostLog(); + const disposable = (): { dispose: () => void } => ({ + dispose: () => { + calls.dispose++; + }, + }); + const root: ShutdownRoot = { + log, + layoutState: { + flush: + overrides.flush ?? + (async () => { + calls.flush++; + }), + }, + config: disposable(), + chordPendingIndicator: disposable(), + chordMachine: disposable(), + findService: disposable(), + editorSession: disposable(), + editorLangIdSync: disposable(), + themeConfigSync: disposable(), + themeSelectCommand: disposable(), + openFileCommand: disposable(), + tabCommands: disposable(), + extensionsReloadCommand: disposable(), + keybindingsCommands: disposable(), + modalCommands: disposable(), + modalService: disposable(), + windowMessageService: disposable(), + hostErrorSink: disposable(), + highlightService: disposable(), + languageRegistry: disposable(), + hostRef: { + current: { + disposeAll: async () => { + calls.disposeAll++; + }, + }, + }, + }; + return { root, log, calls }; +} + +test("runTecode wires the render seam's onDestroy hook to shutdown(), which flushes layout state to disk", async () => { + const homeDir = await mkdtemp(join(tmpdir(), "tecode-shutdown-home-")); + const workspaceDir = await mkdtemp(join(tmpdir(), "tecode-shutdown-ws-")); + + try { + const mainPath = join(import.meta.dir, "main.ts"); + const statePath = join(homeDir, ".config", "tecode", "state.json"); + const fixturePath = join(workspaceDir, "shutdown-fixture.ts"); + // A real interactive Ctrl+C never reaches this path in a test (this + // file's own TSDoc) — `options.renderShell` is `runTecode`'s existing + // injectable seam this fixture uses to CAPTURE, rather than open, the + // real `onDestroy` callback, then fires it directly to simulate + // OpenTUI's `exitOnCtrlC` path calling `CliRenderer.destroy()` on a + // normal interactive quit (Issue #84) — `destroy()` synchronously + // invokes `onDestroy`, which `runTecode` wires straight to + // `shutdown()` (`main.ts`'s TSDoc). + await writeFile( + fixturePath, + `import { runTecode } from ${JSON.stringify(mainPath)}; + import { readFileSync } from "node:fs"; + + function hasThemeSelect(root) { + return root.commands.list().some((c) => c.id === "theme.select"); + } + + async function main() { + let capturedOnDestroy; + const result = await runTecode([], { + headless: false, + builtins: [], + renderShell: async (deps) => { + capturedOnDestroy = deps.onDestroy; + }, + }); + + if (!hasThemeSelect(result.root)) { + console.log(JSON.stringify({ event: "fixture.badPrecondition" })); + process.exit(1); + } + + // Dirty the layout state (Req 6.4) so flush() has a real pending + // write to perform — flush() is a no-op write-wise when nothing is + // pending (layoutState.ts's own flush/update TSDoc). + result.root.layoutState.update({ sidebarWidth: 987 }); + capturedOnDestroy?.(); + + // performShutdown (main.ts) disposes theme.select's command + // registration STRICTLY AFTER awaiting layoutState.flush() to + // completion — so waiting for theme.select to disappear from the + // registry is a deterministic, race-free signal that flush()'s + // own write has already landed on disk too, without racing + // layoutState's own 250ms debounce timer (which would otherwise + // write the SAME content on its own, defeating this test's whole + // point of proving the DESTROY HOOK caused it). + const start = Date.now(); + while (Date.now() - start < 5000) { + if (!hasThemeSelect(result.root)) { + const text = readFileSync(${JSON.stringify(statePath)}, "utf8"); + console.log(JSON.stringify({ event: "fixture.disposedAndFlushed", text })); + process.exit(text.includes("987") ? 0 : 1); + } + await new Promise((r) => setTimeout(r, 5)); + } + console.log(JSON.stringify({ event: "fixture.timeout" })); + process.exit(1); + } + main(); + `, + "utf8", + ); + + const proc = Bun.spawn({ + cmd: ["bun", "run", fixturePath], + cwd: workspaceDir, + env: { ...process.env, HOME: homeDir, APPDATA: homeDir }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + + expect(exitCode, `fixture stderr:\n${stderr}\nstdout:\n${stdout}`).toBe(0); + expect(stdout).toContain("fixture.disposedAndFlushed"); + + const written = JSON.parse(await readFile(statePath, "utf8")) as { sidebarWidth: number }; + expect(written.sidebarWidth).toBe(987); + } finally { + await rm(homeDir, { recursive: true, force: true }); + await rm(workspaceDir, { recursive: true, force: true }); + } +}); + +test("createShutdown's returned function is idempotent: destroy-then-signal runs the sequence exactly once", async () => { + const { root, calls } = createFakeShutdownRoot(); + const shutdown = createShutdown(root); + + // "destroy" fires first (Issue #84's normal-quit path)... + const destroyCall = shutdown(); + // ...then a SIGTERM/SIGINT races in while the first call is still + // genuinely in flight — this must await the SAME real teardown, not + // resolve early and let a signal handler's `process.exit(0)` cut it off + // (`createShutdown`'s "memoized promise, not a boolean flag" TSDoc). + const signalCall = shutdown(); + + await Promise.all([destroyCall, signalCall]); + + expect(calls.flush).toBe(1); + expect(calls.dispose).toBe(18); // one per disposable field in ShutdownRoot + expect(calls.disposeAll).toBe(1); +}); + +test("createShutdown's returned function is idempotent: signal-then-destroy runs the sequence exactly once", async () => { + const { root, calls } = createFakeShutdownRoot(); + const shutdown = createShutdown(root); + + // Same guarantee, opposite order: a signal fires first, then OpenTUI's + // destroy hook fires while that first call is still in flight. + const signalCall = shutdown(); + const destroyCall = shutdown(); + + await Promise.all([signalCall, destroyCall]); + + expect(calls.flush).toBe(1); + expect(calls.dispose).toBe(18); + expect(calls.disposeAll).toBe(1); + + // A THIRD call, after the sequence has already fully settled, is still + // the same no-further-work no-op (this is what makes it safe to hit + // from as many quit paths as ever call it). + await shutdown(); + expect(calls.flush).toBe(1); + expect(calls.dispose).toBe(18); + expect(calls.disposeAll).toBe(1); +}); + +test("createShutdown bounds the wait: a hung flush() still lets shutdown() settle, and logs a warning", async () => { + const scheduler = createFakeScheduler(); + // A `flush()` that never resolves — matches a genuinely hung dispose in + // production; `createShutdown` must not wait on it forever. + const hungFlush = () => new Promise(() => {}); + const { root, log, calls } = createFakeShutdownRoot({ flush: hungFlush }); + const shutdown = createShutdown(root, { scheduler, timeoutMs: SHUTDOWN_TIMEOUT_MS }); + + let settled = false; + const shutdownPromise = shutdown().then(() => { + settled = true; + }); + + // Nothing has settled yet — the fake flush() never resolves on its own, + // so without the timeout firing, this would hang forever. + expect(settled).toBe(false); + + // Simulate SHUTDOWN_TIMEOUT_MS elapsing, without a real multi-second + // wait (this file's own `createFakeScheduler` TSDoc). + scheduler.fire(); + await shutdownPromise; + + expect(settled).toBe(true); + expect( + log + .entries() + .some((e) => e.level === "warning" && e.error.message.includes(`${SHUTDOWN_TIMEOUT_MS}ms`)), + ).toBe(true); + + // The disposals after the hung flush() genuinely never ran — the + // timeout lets `shutdown()` SETTLE without them, it does not fake + // having run them. + expect(calls.dispose).toBe(0); + expect(calls.disposeAll).toBe(0); +}); diff --git a/requirements.md b/requirements.md index c3fbf93..5c2141c 100644 --- a/requirements.md +++ b/requirements.md @@ -192,6 +192,7 @@ The following points were open in the draft specification and are resolved here 1. WHEN tecode is launched, THE system SHALL proceed in this order: load configuration; discover extensions; register manifest declarations without executing extension code; activate extensions lazily per their activation events; render the UI shell; then open the initial file or directory given on the command line. 2. THE UI shell SHALL render within 100 ms of launch, with extension loading deferred so it does not block first paint. +3. WHEN tecode exits — whether by `SIGINT`/`SIGTERM` or by an interactive Ctrl+C while the terminal is in raw mode (which never delivers `SIGINT`, since raw mode disables signal generation) — THE system SHALL run the same shutdown sequence exactly once regardless of which of these triggers fired first: flush layout state (Requirement 6.4), dispose every core-owned service, and deactivate every extension (Requirement 2.6), all bounded by a timeout so a hung disposal cannot prevent the process from exiting. ### Requirement 13: Non-Functional Requirements From 44e208a4651d1c228e4af86035380d7dfd34be7f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 08:38:26 +0000 Subject: [PATCH 2/2] Fix #84 follow-up: onDestroy must call process.exit(0), not just shutdown() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit (PR #87) caught a real gap: SHUTDOWN_TIMEOUT_MS bounds the shutdown() promise, not the pending flush()/dispose() I/O it raced against. A genuinely hung layoutState.flush() never lets performShutdown() reach its dispose() calls, so whatever real handles those would have closed stay open — and since onDestroy only did `void shutdown()`, nothing ever called process.exit once shutdown() gave up, leaving the process (and the editor) unquittable in exactly the case the timeout exists to guard against. onDestroy now mirrors the SIGINT/SIGTERM path exactly: `void shutdown().finally(() => process.exit(0))`. This costs nothing in the healthy case — shutdown() only resolves early via the timeout branch in the first place, so by the time .finally runs there is nothing further worth blocking exit on. Corrected the onDestroy TSDoc in main.ts and renderShell.tsx, which had asserted the false premise that the process would exit "naturally" once the event loop drains; updated design.md's shutdown point to match. Added a subprocess test with a layoutState.flush() that never resolves (backed by a real, still-armed timer standing in for whatever real handles a completed teardown would have closed) and confirmed it hangs past a bounded wait without the fix, then exits cleanly with it. Also had to rework the existing "wires onDestroy to shutdown()" fixture: once onDestroy calls process.exit(0), it settles within microtask timescale, so any post-hoc polling in the fixture was racing (and losing to) that exit; switched to a synchronous process.on("exit", ...) listener, gated on theme.select's command-registration disposal rather than state.json's content (layoutState.update()'s own real debounce timer can write that file on its own regardless of whether shutdown() ever ran). Re-verified the mutation check: removing the onDestroy wiring still fails the first test. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- design.md | 2 +- packages/cli/src/main.ts | 31 +++- packages/cli/src/renderShell.tsx | 18 +- packages/cli/src/shutdownOnDestroy.test.ts | 204 +++++++++++++++++---- 4 files changed, 206 insertions(+), 49 deletions(-) diff --git a/design.md b/design.md index 8ddd76b..a65ed94 100644 --- a/design.md +++ b/design.md @@ -72,7 +72,7 @@ Order (*Req 12.1*), with first paint before extension code runs (*Req 12.2*): 1. **Synchronous, before first frame (< 100 ms budget):** parse argv; load user + workspace settings and keybindings (JSONC); detect terminal capabilities (Kitty protocol, color depth); build the theme from the configured theme's cached JSON (themes-default's JSON files are embedded assets, so no extension activation is needed to paint); render the Shell with empty slots and a placeholder editor area. 2. **Deferred (queueMicrotask after first frame):** discover extensions and validate manifests; register declared contributions; fire `onStartup` activations; open the file/directory from argv (which fires `onLanguage:*` activations). 3. Manifest reading executes `manifest.ts` as a module (it is TypeScript), but manifests are constrained by convention and validation to be pure data (`export default {...} satisfies Manifest`); the extension's `index.ts` is not imported until activation (*Req 2.2*). -4. **Shutdown** (*Req 12.3*, Issue #84): `createCliRenderer()` puts stdin in raw mode, which disables signal generation, so an interactive Ctrl+C never reaches Node as a real `SIGINT` — OpenTUI's own `exitOnCtrlC` key handling (default `true`) intercepts the byte itself and calls `CliRenderer.destroy()` directly. `destroy()`'s `finalizeDestroy()` never calls `process.exit` — the process exits only once the event loop drains — so `main.ts` wires its shutdown sequence to `createCliRenderer`'s `onDestroy` config callback (`renderShell.tsx`'s `ShellRenderDeps.onDestroy`), which fires synchronously from inside that teardown with nothing left afterward to catch a throw; `onDestroy` was chosen over subscribing to the `CliRenderEvents.DESTROY` event (which this module already uses for `CAPABILITIES`) because `finalizeDestroy()` emits that event partway through its own teardown, via a plain `EventEmitter.emit` with no try/catch, whereas `onDestroy` fires at the very end and is already wrapped in the library's own try/catch. `SIGINT`/`SIGTERM` (the paths that fire when stdin is NOT in raw mode: `kill`, a supervising shell, headless mode) call the exact same shutdown sequence. The sequence itself (`main.ts`'s `createShutdown`) is a single memoized promise shared by every trigger — so whichever fires first runs the real teardown (flush layout state, dispose every core-owned service, deactivate every extension) and whichever fires afterward, or concurrently, awaits that same settling promise rather than racing a `process.exit(0)` ahead of it — raced against a bounded timeout (a couple of seconds) so a hung disposal degrades to a logged warning instead of an unquittable process. +4. **Shutdown** (*Req 12.3*, Issue #84): `createCliRenderer()` puts stdin in raw mode, which disables signal generation, so an interactive Ctrl+C never reaches Node as a real `SIGINT` — OpenTUI's own `exitOnCtrlC` key handling (default `true`) intercepts the byte itself and calls `CliRenderer.destroy()` directly. `destroy()`'s `finalizeDestroy()` never calls `process.exit` itself, so `main.ts` wires its shutdown sequence to `createCliRenderer`'s `onDestroy` config callback (`renderShell.tsx`'s `ShellRenderDeps.onDestroy`), which fires synchronously from inside that teardown with nothing left afterward to catch a throw; `onDestroy` was chosen over subscribing to the `CliRenderEvents.DESTROY` event (which this module already uses for `CAPABILITIES`) because `finalizeDestroy()` emits that event partway through its own teardown, via a plain `EventEmitter.emit` with no try/catch, whereas `onDestroy` fires at the very end and is already wrapped in the library's own try/catch. `SIGINT`/`SIGTERM` (the paths that fire when stdin is NOT in raw mode: `kill`, a supervising shell, headless mode) call the exact same shutdown sequence. The sequence itself (`main.ts`'s `createShutdown`) is a single memoized promise shared by every trigger — so whichever fires first runs the real teardown (flush layout state, dispose every core-owned service, deactivate every extension) and whichever fires afterward, or concurrently, awaits that same settling promise rather than racing a `process.exit(0)` ahead of it — raced against a bounded timeout (a couple of seconds) so a hung disposal degrades to a logged warning rather than hanging forever. That timeout bounds only the `shutdown()` PROMISE, not whatever pending I/O it gave up waiting on — a genuinely hung `flush()`/`dispose()` would otherwise keep the event loop (and the process) alive indefinitely even after `shutdown()` settles, reintroducing the unquittable-editor risk the timeout exists to prevent — so every one of `onDestroy`/`SIGINT`/`SIGTERM` explicitly calls `process.exit(0)` once `shutdown()` settles (`void shutdown().finally(() => process.exit(0))`), rather than relying on the process to exit "naturally" once the loop happens to drain. ## 4. Extension Host diff --git a/packages/cli/src/main.ts b/packages/cli/src/main.ts index cae91aa..941d7f8 100644 --- a/packages/cli/src/main.ts +++ b/packages/cli/src/main.ts @@ -1530,17 +1530,28 @@ export async function runTecode( // `createCliRenderer`'s `onDestroy` config) invokes this synchronously // and never calls `process.exit` itself. `shutdown` is the SAME // shared, idempotent, timeout-bounded sequence `SIGINT`/`SIGTERM` - // already call (`wireProcessExit`'s TSDoc) — fire-and-forget (`void`) - // is correct and sufficient here: its own pending `flush()`/ - // `dispose()` I/O keeps the event loop alive long enough to finish (or - // hit `SHUTDOWN_TIMEOUT_MS` and give up), and once it settles with - // nothing else scheduled, the process exits on its own with no - // `process.exit` call needed from here (`createShutdown`'s TSDoc). - // Never throws: `shutdown()` itself is guarded (same TSDoc), so this - // callback can't propagate into OpenTUI's `_onDestroy` invocation - // either (which wraps it in try/catch anyway — belt and suspenders). + // already call below via `wireProcessExit` — wired here EXACTLY the + // same way (`void shutdown().finally(() => process.exit(0))`), not + // fire-and-forget: `SHUTDOWN_TIMEOUT_MS` bounds the `shutdown()` + // PROMISE, not the pending `flush()`/`dispose()` I/O it raced against + // — a genuinely hung `layoutState.flush()` keeps that I/O outstanding + // even after `shutdown()` gives up and resolves, which would keep the + // event loop (and the process) alive forever with no explicit exit + // call to end it, defeating the entire point of the timeout (an + // unquittable editor is exactly what `onDestroy` — over + // `exitOnCtrlC: false` — was chosen to avoid). Calling + // `process.exit(0)` here costs nothing in the healthy case: `shutdown()` + // only resolves once `performShutdown()` has genuinely finished (the + // timer branch is the only way to resolve early, and that IS the + // give-up path), so by the time this `.finally` runs, either + // everything already completed or the timeout already decided not to + // wait any longer — either way there's nothing further worth blocking + // exit on. Never throws: `shutdown()` itself is guarded (same TSDoc), + // so this callback can't propagate into OpenTUI's `_onDestroy` + // invocation either (which wraps it in try/catch anyway — belt and + // suspenders). onDestroy: () => { - void shutdown(); + void shutdown().finally(() => process.exit(0)); }, }); diff --git a/packages/cli/src/renderShell.tsx b/packages/cli/src/renderShell.tsx index 5baafd1..07ca06f 100644 --- a/packages/cli/src/renderShell.tsx +++ b/packages/cli/src/renderShell.tsx @@ -145,11 +145,19 @@ export interface ShellRenderDeps { * `CliRenderer.destroy()` directly, bypassing `SIGINT` entirely. * * `destroy()`'s `finalizeDestroy()` never calls `process.exit` (checked - * against the pinned `@opentui/core@0.1.107` bundle) — the process exits - * naturally once the event loop drains, so `main.ts`'s `runTecode` can - * safely start async cleanup here (`shutdown()`) and let its own - * pending I/O keep the process alive until that cleanup genuinely - * finishes, with no `process.exit` call needed from this callback. + * against the pinned `@opentui/core@0.1.107` bundle), so `main.ts`'s + * `runTecode` starts async cleanup here (`shutdown()`) and, once it + * settles, explicitly calls `process.exit(0)` itself — mirroring + * exactly what it already does on `SIGINT`/`SIGTERM` + * (`void shutdown().finally(() => process.exit(0))`), rather than + * relying on the process to exit "naturally" once its own pending I/O + * happens to drain the event loop: `shutdown()` is raced against a + * bounded timeout precisely so a hung `flush()`/`dispose()` cannot hang + * the process forever, but that timeout only bounds the `shutdown()` + * PROMISE — it does not cancel the pending I/O behind it — so without + * an explicit exit call here, a genuinely hung disposal would still + * leave the process (and the editor) unquittable even after `shutdown()` + * itself has given up (`createShutdown`'s own TSDoc in `main.ts`). * `onDestroy` (a plain `CliRendererConfig` field) was chosen over * subscribing to the `CliRenderEvents.DESTROY` event this module * already uses for `CAPABILITIES` below: `finalizeDestroy()` emits diff --git a/packages/cli/src/shutdownOnDestroy.test.ts b/packages/cli/src/shutdownOnDestroy.test.ts index f30c98b..1795750 100644 --- a/packages/cli/src/shutdownOnDestroy.test.ts +++ b/packages/cli/src/shutdownOnDestroy.test.ts @@ -31,7 +31,7 @@ */ import { expect, setDefaultTimeout, test } from "bun:test"; -import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { createHostLog, type ChordScheduler } from "@tecode/core"; @@ -139,15 +139,43 @@ test("runTecode wires the render seam's onDestroy hook to shutdown(), which flus // normal interactive quit (Issue #84) — `destroy()` synchronously // invokes `onDestroy`, which `runTecode` wires straight to // `shutdown()` (`main.ts`'s TSDoc). + // + // `onDestroy` now itself calls `process.exit(0)` once `shutdown()` + // settles (`void shutdown().finally(() => process.exit(0))` — see the + // "onDestroy calls process.exit(0)..." test below for why), and that + // settling can happen within a couple of microtask ticks once + // `performShutdown()`'s last `await` resolves — far faster than any + // real timer this fixture could poll on. So rather than racing that + // exit with a `setTimeout`-based poll loop (which lost this exact + // race when first tried — the internal exit consistently won), + // this fixture registers a SYNCHRONOUS `process.on("exit", ...)` + // listener before firing `onDestroy`. Node/Bun always runs "exit" + // listeners synchronously as part of process teardown — whether + // triggered by our own explicit `process.exit(0)` or by the process + // draining naturally — so by the time it runs, `shutdown()` (real + // fix) or nothing at all (mutated-away wiring) has already + // deterministically finished happening; verified empirically that + // Bun reliably flushes a `console.log` made inside this handler to a + // piped stdout in both cases. + // + // `theme.select`'s command registration — disposed synchronously by + // `performShutdown`, with no OTHER path that ever removes it — is the + // decisive signal, not `state.json`'s content: `layoutState.update()` + // below arms a REAL 250ms debounced write regardless of whether + // `shutdown()` ever runs, so if `onDestroy` were entirely missing + // (this file's mutation check), the process would still eventually + // exit once that real timer elapses and fire this SAME "exit" + // listener — with `theme.select` still registered (proving the + // teardown never ran) even though `state.json` might, by then, have + // been written anyway by the unrelated debounce. `hasThemeSelect` + // is asserted false; `state.json`'s content is asserted only as a + // bonus confirmation, meaningful precisely because `hasThemeSelect` + // already established the real teardown ran. await writeFile( fixturePath, `import { runTecode } from ${JSON.stringify(mainPath)}; import { readFileSync } from "node:fs"; - function hasThemeSelect(root) { - return root.commands.list().some((c) => c.id === "theme.select"); - } - async function main() { let capturedOnDestroy; const result = await runTecode([], { @@ -158,36 +186,23 @@ test("runTecode wires the render seam's onDestroy hook to shutdown(), which flus }, }); - if (!hasThemeSelect(result.root)) { - console.log(JSON.stringify({ event: "fixture.badPrecondition" })); - process.exit(1); - } - // Dirty the layout state (Req 6.4) so flush() has a real pending // write to perform — flush() is a no-op write-wise when nothing is // pending (layoutState.ts's own flush/update TSDoc). result.root.layoutState.update({ sidebarWidth: 987 }); - capturedOnDestroy?.(); - // performShutdown (main.ts) disposes theme.select's command - // registration STRICTLY AFTER awaiting layoutState.flush() to - // completion — so waiting for theme.select to disappear from the - // registry is a deterministic, race-free signal that flush()'s - // own write has already landed on disk too, without racing - // layoutState's own 250ms debounce timer (which would otherwise - // write the SAME content on its own, defeating this test's whole - // point of proving the DESTROY HOOK caused it). - const start = Date.now(); - while (Date.now() - start < 5000) { - if (!hasThemeSelect(result.root)) { - const text = readFileSync(${JSON.stringify(statePath)}, "utf8"); - console.log(JSON.stringify({ event: "fixture.disposedAndFlushed", text })); - process.exit(text.includes("987") ? 0 : 1); - } - await new Promise((r) => setTimeout(r, 5)); - } - console.log(JSON.stringify({ event: "fixture.timeout" })); - process.exit(1); + process.on("exit", () => { + const hasThemeSelect = result.root.commands + .list() + .some((c) => c.id === "theme.select"); + let stateContent = null; + try { + stateContent = readFileSync(${JSON.stringify(statePath)}, "utf8"); + } catch {} + console.log(JSON.stringify({ event: "fixture.exit", hasThemeSelect, stateContent })); + }); + + capturedOnDestroy?.(); } main(); `, @@ -208,10 +223,133 @@ test("runTecode wires the render seam's onDestroy hook to shutdown(), which flus ]); expect(exitCode, `fixture stderr:\n${stderr}\nstdout:\n${stdout}`).toBe(0); - expect(stdout).toContain("fixture.disposedAndFlushed"); - const written = JSON.parse(await readFile(statePath, "utf8")) as { sidebarWidth: number }; - expect(written.sidebarWidth).toBe(987); + const exitEvent = stdout + .split("\n") + .map((line) => line.trim()) + .filter(Boolean) + .map((line) => { + try { + return JSON.parse(line) as { event?: string; hasThemeSelect?: boolean; stateContent?: string | null }; + } catch { + return null; + } + }) + .find((line) => line?.event === "fixture.exit"); + + expect(exitEvent, `fixture stderr:\n${stderr}\nstdout:\n${stdout}`).toBeDefined(); + // The decisive assertion (this test's own TSDoc): theme.select's + // command registration is gone ONLY if the real teardown sequence + // (shutdown()) actually ran. + expect(exitEvent?.hasThemeSelect).toBe(false); + + const written = JSON.parse(exitEvent?.stateContent ?? "null") as { sidebarWidth?: number } | null; + expect(written?.sidebarWidth).toBe(987); + } finally { + await rm(homeDir, { recursive: true, force: true }); + await rm(workspaceDir, { recursive: true, force: true }); + } +}); + +test("onDestroy calls process.exit(0) once shutdown() settles, even when layoutState.flush() itself never resolves", async () => { + // This is the gap CodeRabbit found in PR #87: `SHUTDOWN_TIMEOUT_MS` + // bounds the `shutdown()` PROMISE, not the pending I/O it raced + // against. A hung `layoutState.flush()` never lets `performShutdown()` + // reach any of its `dispose()` calls, so whatever real handles those + // would have closed (`ConfigService`'s `fs.watch` when the watched + // files exist, the extension host, etc.) stay open — this fixture + // models that directly with its own live, ref'd `setInterval` (rather + // than depending on, say, `settings.json` happening to exist in this + // hermetic temp `HOME` for a real watcher to attach to) so the failure + // mode is reproduced deterministically. Without an explicit + // `process.exit(0)` once `shutdown()` settles, a still-live handle like + // this would keep the real process running forever — this test proves + // the fix (`onDestroy: () => { void shutdown().finally(() => process.exit(0)); }`) + // exits anyway, and bounds its own wait (killing the child rather than + // hanging this test) so a regression fails cleanly instead of hanging + // the whole suite. + const homeDir = await mkdtemp(join(tmpdir(), "tecode-shutdown-hang-home-")); + const workspaceDir = await mkdtemp(join(tmpdir(), "tecode-shutdown-hang-ws-")); + + try { + const mainPath = join(import.meta.dir, "main.ts"); + const fixturePath = join(workspaceDir, "shutdown-hang-fixture.ts"); + await writeFile( + fixturePath, + `import { runTecode } from ${JSON.stringify(mainPath)}; + + async function main() { + let capturedOnDestroy; + const result = await runTecode([], { + headless: false, + builtins: [], + renderShell: async (deps) => { + capturedOnDestroy = deps.onDestroy; + }, + }); + + // A genuinely hung flush(): a promise that never settles, exactly + // like a stuck real fs write, WITH a real, still-armed, ref'd + // timer behind it — a bare unresolved promise holds no libuv + // handle and costs the event loop nothing on its own, so this + // interval stands in for whatever real resource(s) + // performShutdown()'s later dispose() calls would otherwise have + // closed (ConfigService's fs.watch, the extension host, ...) had + // \`await root.layoutState.flush()\` ever gotten a chance to + // resolve and let them run. + result.root.layoutState.flush = () => + new Promise(() => { + setInterval(() => {}, 1000); + }); + + console.log(JSON.stringify({ event: "fixture.ready" })); + capturedOnDestroy?.(); + } + main(); + `, + "utf8", + ); + + const proc = Bun.spawn({ + cmd: ["bun", "run", fixturePath], + cwd: workspaceDir, + env: { ...process.env, HOME: homeDir, APPDATA: homeDir }, + stdout: "pipe", + stderr: "pipe", + }); + + // SHUTDOWN_TIMEOUT_MS (2s, real/un-mocked here — this is a real + // subprocess) is when `shutdown()` itself is expected to give up; + // this margin is generous headroom above that for the subprocess to + // then actually call `process.exit` and for Bun to report it exited. + const raceTimeoutMs = SHUTDOWN_TIMEOUT_MS + 6_000; + let timedOut = false; + const exitCode = await Promise.race([ + proc.exited, + new Promise((resolve) => { + setTimeout(() => { + timedOut = true; + resolve(-1); + }, raceTimeoutMs); + }), + ]); + if (timedOut) { + // Don't leave a hung child (with its own hung timers/watchers) + // running in the background just because this assertion is about + // to fail. + proc.kill(); + } + + const [stdout, stderr] = await Promise.all([ + new Response(proc.stdout).text().catch(() => "(stdout unavailable)"), + new Response(proc.stderr).text().catch(() => "(stderr unavailable)"), + ]); + + expect( + timedOut, + `fixture did not exit within ${raceTimeoutMs}ms — onDestroy's shutdown() likely settled (after ${SHUTDOWN_TIMEOUT_MS}ms) without ever calling process.exit, leaving the fixture's still-live handle open. stdout:\n${stdout}\nstderr:\n${stderr}`, + ).toBe(false); + expect(exitCode, `stdout:\n${stdout}\nstderr:\n${stderr}`).toBe(0); } finally { await rm(homeDir, { recursive: true, force: true }); await rm(workspaceDir, { recursive: true, force: true });