From 889e0cdb00a181424e5f116ac6af95282b25f5f6 Mon Sep 17 00:00:00 2001 From: Yunfei He Date: Mon, 15 Jun 2026 01:16:51 +0800 Subject: [PATCH] fix(runtime): make error capture first-wins and crash-safe against a racing unmount (#182) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(runtime): make error capture first-wins and crash-safe against a racing unmount Two confirmed bugs in the InternalErrorBoundary's onErrorCaptured: BUG #2 — a component error was silently swallowed when host code threw during an update flush and then synchronously called app.unmount() in the same task. The exit was routed entirely through `void nextTick(() => exitWithError(e))`, so pendingExitError was not recorded until that deferred microtask ran; the racing unmount's resolveExit() read it as undefined and RESOLVED the exit promise clean instead of REJECTING with the error. Fix: record the error SYNCHRONOUSLY via a new recordExitError() bridge (first-wins: only sets pendingExitError if no exit is already decided), while keeping teardown DEFERRED via nextTick. Deferring teardown is load-bearing — teardown()'s final mountedCommit() paints the ErrorOverview frame, and the boundary's errored->true re-render must commit before it; a synchronous exit would drop the overview frame on non-interactive/non-debug mounts. Frame/paint timing is now byte-identical to before in every mode. BUG #5 — two descendants throwing in the same synchronous flush left the displayed overview (caught, last-wins) and the rejected error (pendingExitError, first-wins) disagreeing. Fix: guard the capture body with `if (!errored.value)` so the first thrown error drives both the display and the rejection (e17). Tests: the racing-unmount swallow (interactive/debug AND non-interactive/ non-debug), the two-throw display/reject agreement, and frame-painting guards that pin the overview behavior to main in each mode. Also corrected a stale exit-chain comment in @vue-tui/testing's render(). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(runtime): exit() must not clobber an error already recorded by the boundary (first-wins) Final review found an asymmetry: recordExitError() first-wins-guards its write, but appContext.exit() recorded the error unconditionally. So a captured throw (Error1, shown in the overview, recorded via recordExitError) followed by a racing exit(Error2) before the deferred teardown made waitUntilExit() reject Error2 while the overview displayed Error1 — the BUG #5 display/reject disagreement through a different door. Fix: exit() uses `pendingExitError ??= errorOrResult`, so it keeps a synchronously-recorded error. Identical to `=` in every other case (pendingExitError is undefined on a normal first exit()). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../lifecycle/error-capture-race.test.tsx | 332 ++++++++++++++++++ packages/runtime/src/render.ts | 86 ++++- packages/testing/src/render.ts | 6 +- 3 files changed, 407 insertions(+), 17 deletions(-) create mode 100644 packages/runtime-tests/integration/lifecycle/error-capture-race.test.tsx diff --git a/packages/runtime-tests/integration/lifecycle/error-capture-race.test.tsx b/packages/runtime-tests/integration/lifecycle/error-capture-race.test.tsx new file mode 100644 index 0000000..c56fe28 --- /dev/null +++ b/packages/runtime-tests/integration/lifecycle/error-capture-race.test.tsx @@ -0,0 +1,332 @@ +import { PassThrough } from "node:stream"; +import { defineComponent, h, nextTick, shallowRef } from "vue"; +import { expect, test } from "vite-plus/test"; +import stripAnsi from "strip-ansi"; +import { createApp, Text, useApp } from "@vue-tui/runtime"; +import { + captureWrites, + getContentWrites, + makeFakeStdin, + makeFakeWritable, +} from "./test-streams.ts"; + +// A NON-TTY writable (isTTY=false). makeFakeWritable() forces isTTY=true, which +// makes the app interactive; for the non-interactive non-debug race below we need +// a piped stream so `interactive` derives false (render.ts: +// `options.interactive ?? (!isInCi && Boolean(stdout.isTTY))`). We also pass +// `interactive: false` explicitly so the case is deterministic regardless of CI. +function makeNonTtyWritable(): NodeJS.WriteStream { + const s = new PassThrough() as unknown as NodeJS.WriteStream; + Object.assign(s, { columns: 100, rows: 100, isTTY: false }); + return s; +} + +// These two tests pin down the error-capture/exit-race contract. They mount +// directly (not via @vue-tui/testing's render(), which re-throws and discards +// the painted frame) so we can read the ErrorOverview frame AND inspect what +// waitUntilExit() settles with from a single mount. + +// --- BUG #2: error swallowed when unmount() races the deferred exit --- +// When a render-flush throw queues the boundary's exit and host code calls +// app.unmount() synchronously in the SAME task (before the deferred exit runs), +// the thrown error must still reject waitUntilExit(). Before the fix the +// pendingExitError was recorded only inside the deferred exit, so the racing +// unmount's resolveExit() read undefined and RESOLVED clean — swallowing the +// error. The fix (Option B) records pendingExitError SYNCHRONOUSLY in +// onErrorCaptured (recordExitError) while keeping teardown DEFERRED, so the race +// rejects with the error without disturbing frame/paint timing. +test("update-flush throw then synchronous unmount(): waitUntilExit REJECTS with the thrown error", async () => { + const stdout = makeFakeWritable(); + const stderr = makeFakeWritable(); + const { stream: stdin } = makeFakeStdin(); + + const trigger = shallowRef(false); + const ThrowsOnUpdate = defineComponent(() => { + return () => { + if (trigger.value) { + throw new Error("UPDATE_FLUSH_BOOM"); + } + return h(Text, null, "ok"); + }; + }); + + const app = createApp(ThrowsOnUpdate); + app.mount({ stdout, stdin, stderr, debug: true, exitOnCtrlC: false }); + + type Settled = { kind: "rejected"; message: unknown } | { kind: "resolved"; value: unknown }; + const done: Promise = app.waitUntilExit().then( + (value: unknown): Settled => ({ kind: "resolved", value }), + (e: unknown): Settled => ({ kind: "rejected", message: (e as Error)?.message }), + ); + + // Let the initial mount flush. + await nextTick(); + + // Flip a ref that throws on render. The throw happens INSIDE the flush this + // await waits on, which queues the boundary's deferred exit. + trigger.value = true; + await nextTick(); + + // Race: synchronously unmount in the SAME task, before the queued exit runs. + app.unmount(); + + const settled = await done; + + expect(settled.kind).toBe("rejected"); + if (settled.kind !== "rejected") throw new Error("expected rejection, got resolve"); + expect(settled.message).toBe("UPDATE_FLUSH_BOOM"); +}); + +// --- BUG #5: two throws in one flush — overlay vs reject must AGREE --- +// If two siblings throw in the same synchronous flush, the DISPLAYED overview +// and the REJECTED error must be the SAME (first-thrown) error. Before the fix +// `caught` was last-wins (displayed B) while the exit path was first-wins +// (rejected A) — a display/reject mismatch. +test("two sibling throws in one flush: displayed overview and rejected error AGREE (both first-thrown)", async () => { + const stdout = makeFakeWritable(); + const stderr = makeFakeWritable(); + const { stream: stdin } = makeFakeStdin(); + const writes = captureWrites(stdout); + + const ThrowerA = defineComponent(() => { + return () => { + throw new Error("ERROR_A_FIRST"); + }; + }); + const ThrowerB = defineComponent(() => { + return () => { + throw new Error("ERROR_B_SECOND"); + }; + }); + // Two siblings under one parent: both render (and throw) in the same flush. + const Root = defineComponent(() => { + return () => h(Text, null, [h(ThrowerA), h(ThrowerB)]); + }); + + const app = createApp(Root); + app.mount({ stdout, stdin, stderr, debug: true, exitOnCtrlC: false }); + + type Reject = { kind: "rejected"; message: unknown } | { kind: "resolved" }; + const settled: Promise = app.waitUntilExit().then( + (): Reject => ({ kind: "resolved" }), + (e: unknown): Reject => ({ kind: "rejected", message: (e as Error)?.message }), + ); + + await new Promise((r) => setImmediate(r)); + await new Promise((r) => setImmediate(r)); + const reject = await settled; + + const content = getContentWrites(writes); + const lastContentWrite = content.at(-1); + if (lastContentWrite === undefined) throw new Error("no content write captured"); + const frame = stripAnsi(lastContentWrite); + + // Reject is first-wins. + expect(reject.kind).toBe("rejected"); + if (reject.kind !== "rejected") throw new Error("expected rejection"); + expect(reject.message).toBe("ERROR_A_FIRST"); + + // Display agrees: the overview shows the SAME (first-thrown) error, not B. + expect(frame).toContain("ERROR_A_FIRST"); + expect(frame).not.toContain("ERROR_B_SECOND"); +}); + +// --- BUG #5, through the exit() door: a captured throw then a racing exit(err) --- +// Same display/reject contract as the two-sibling case, but the SECOND error +// arrives via host code calling useApp().exit(Error2) — not a second throw. +// Sequence: a descendant throws Error1 during a flush → the boundary's +// onErrorCaptured shows Error1 in the overview AND calls recordExitError(Error1), +// which sets pendingExitError=Error1 WITHOUT setting exitInitiated, then defers +// nextTick(exitWithError(Error1)). Before that microtask runs, host code calls +// exit(Error2) synchronously in the same task: exitInitiated is still false so +// exit() proceeds. With `pendingExitError = errorOrResult` it CLOBBERS to Error2, +// so the overview shows Error1 while waitUntilExit() rejects Error2 — the exact +// display/reject disagreement BUG #5 forbids. With `pendingExitError ??=` exit() +// keeps the already-recorded Error1, so display and reject AGREE. +test("captured throw then racing exit(err): displayed overview and rejected error AGREE (both Error1)", async () => { + const stdout = makeFakeWritable(); + const stderr = makeFakeWritable(); + const { stream: stdin } = makeFakeStdin(); + const writes = captureWrites(stdout); + + const trigger = shallowRef(false); + // A non-throwing sibling retains exit() (via useApp()) so the host can call it + // synchronously from the test, after the boundary has captured but before the + // deferred exitWithError microtask runs. + // Held in an object so TS doesn't narrow it to `never`: the assignment happens + // inside Retainer's setup closure, which control-flow analysis can't see, so a + // plain `let` read after the await would narrow to its `null` initializer. + const retainer: { exit: ((errorOrResult?: unknown) => void) | null } = { exit: null }; + const Retainer = defineComponent(() => { + retainer.exit = useApp().exit; + return () => h(Text, null, "retainer"); + }); + const ThrowsOnUpdate = defineComponent(() => { + return () => { + if (trigger.value) { + throw new Error("ERROR_1_THROWN"); + } + return h(Text, null, "ok"); + }; + }); + const Root = defineComponent(() => { + return () => h(Text, null, [h(Retainer), h(ThrowsOnUpdate)]); + }); + + const app = createApp(Root); + app.mount({ stdout, stdin, stderr, debug: true, exitOnCtrlC: false }); + + type Settled = { kind: "rejected"; message: unknown } | { kind: "resolved"; value: unknown }; + const done: Promise = app.waitUntilExit().then( + (value: unknown): Settled => ({ kind: "resolved", value }), + (e: unknown): Settled => ({ kind: "rejected", message: (e as Error)?.message }), + ); + + // Let the initial mount flush so the Retainer has captured exit(). + await nextTick(); + const retainedExit = retainer.exit; + if (retainedExit === null) throw new Error("exit() was not retained from useApp()"); + + // Throw Error1 inside the update flush: onErrorCaptured runs synchronously + // during this flush — it shows Error1 in the overview and records it via + // recordExitError(Error1), then queues nextTick(exitWithError(Error1)). + trigger.value = true; + await nextTick(); + + // Race: host calls exit(Error2) in the SAME task, before the deferred + // exitWithError(Error1) microtask runs. With `=` this clobbers pendingExitError + // to Error2; with `??=` it leaves Error1 intact. + retainedExit(new Error("EXIT_ERROR_2")); + + const settled = await done; + + // Reject keeps the first (thrown, displayed) error. + expect(settled.kind).toBe("rejected"); + if (settled.kind !== "rejected") throw new Error("expected rejection, got resolve"); + expect(settled.message).toBe("ERROR_1_THROWN"); + + // Display agrees: the overview painted the SAME first error, never Error2. + const content = getContentWrites(writes); + const lastContentWrite = content.at(-1); + if (lastContentWrite === undefined) throw new Error("no content write captured"); + const frame = stripAnsi(lastContentWrite); + expect(frame).toContain("ERROR_1_THROWN"); + expect(frame).not.toContain("EXIT_ERROR_2"); +}); + +// --- BUG #2, the NON-INTERACTIVE NON-DEBUG case --- +// This is the exact case the discarded synchronous-exit approach broke and the +// case Option B must handle: a piped (non-TTY) stdout with no debug flag. The +// racing app.unmount() runs teardown() + resolveExit() SYNCHRONOUSLY (only the +// boundary's deferred exitWithError teardown is microtask-driven), so resolveExit +// reads pendingExitError in the same task. The synchronous record +// (recordExitError) is what makes the race reject; remove it and this test goes +// RED (resolves clean — the original swallow). +test("non-interactive non-debug: update-flush throw + synchronous unmount() still REJECTS", async () => { + const stdout = makeNonTtyWritable(); + const stderr = makeNonTtyWritable(); + const { stream: stdin } = makeFakeStdin(); + + const trigger = shallowRef(false); + const ThrowsOnUpdate = defineComponent(() => { + return () => { + if (trigger.value) { + throw new Error("NONINTERACTIVE_BOOM"); + } + return h(Text, null, "ok"); + }; + }); + + const app = createApp(ThrowsOnUpdate); + // Non-interactive (piped stdout), non-debug, interactive:false pinned. + app.mount({ stdout, stdin, stderr, interactive: false, exitOnCtrlC: false }); + + type Settled = { kind: "rejected"; message: unknown } | { kind: "resolved"; value: unknown }; + const done: Promise = app.waitUntilExit().then( + (value: unknown): Settled => ({ kind: "resolved", value }), + (e: unknown): Settled => ({ kind: "rejected", message: (e as Error)?.message }), + ); + + await nextTick(); + + // Throw inside the update flush, then race the unmount in the SAME task. + trigger.value = true; + await nextTick(); + app.unmount(); + + const settled = await done; + + expect(settled.kind).toBe("rejected"); + if (settled.kind !== "rejected") throw new Error("expected rejection, got resolve"); + expect(settled.message).toBe("NONINTERACTIVE_BOOM"); +}); + +// --- FRAME-PAINTING regression guard (the synchronous approach would have +// broken the interactive/debug paint by letting teardown run before the +// errored→true re-render committed) --- +// +// Two mode-specific assertions, both pinned to MAIN's empirically-measured +// behavior (Option B keeps teardown timing byte-identical to main): +// - interactive/debug: main DOES paint the ErrorOverview frame (the last +// content write contains the error message). Option B must preserve that. +// - non-interactive non-debug: main does NOT paint ErrorOverview — its only +// content write is a bare trailing "\n" (Ink's `this.lastOutput + '\n'` +// branch, and lastOutput is empty because the dynamic frame was deferred and +// the boundary's error frame is never committed on this path). We assert the +// fix MATCHES main: no error message reaches stdout, just the trailing +// newline. (Verified by probe: main writes ["\n",""]; Option B writes the +// same.) +test("interactive/debug: an error STILL paints the ErrorOverview frame", async () => { + const stdout = makeFakeWritable(); + const stderr = makeFakeWritable(); + const { stream: stdin } = makeFakeStdin(); + const writes = captureWrites(stdout); + + const Throws = defineComponent(() => { + return () => { + throw new Error("PAINTED_BOOM"); + }; + }); + + const app = createApp(Throws); + app.mount({ stdout, stdin, stderr, debug: true, exitOnCtrlC: false }); + app.waitUntilExit().catch(() => {}); + + await new Promise((r) => setImmediate(r)); + await new Promise((r) => setImmediate(r)); + + const content = getContentWrites(writes); + const lastContentWrite = content.at(-1); + if (lastContentWrite === undefined) throw new Error("no content write captured"); + const frame = stripAnsi(lastContentWrite); + expect(frame).toContain("PAINTED_BOOM"); +}); + +test("non-interactive non-debug: error paint MATCHES main (no overview frame, just trailing newline)", async () => { + const stdout = makeNonTtyWritable(); + const stderr = makeNonTtyWritable(); + const { stream: stdin } = makeFakeStdin(); + const writes = captureWrites(stdout); + + const Throws = defineComponent(() => { + return () => { + throw new Error("UNPAINTED_BOOM"); + }; + }); + + const app = createApp(Throws); + app.mount({ stdout, stdin, stderr, interactive: false, exitOnCtrlC: false }); + app.waitUntilExit().catch(() => {}); + + await new Promise((r) => setImmediate(r)); + await new Promise((r) => setImmediate(r)); + await new Promise((r) => setImmediate(r)); + + // Main paints NO ErrorOverview here: the error message never reaches stdout. + const allContent = writes.join(""); + expect(stripAnsi(allContent)).not.toContain("UNPAINTED_BOOM"); + // The only content write is the trailing newline teardown owes (lastFrame is + // empty + "\n"), matching Ink's non-interactive non-debug branch. + const content = getContentWrites(writes); + expect(content).toEqual(["\n"]); +}); diff --git a/packages/runtime/src/render.ts b/packages/runtime/src/render.ts index 1e6cb8a..60500b7 100644 --- a/packages/runtime/src/render.ts +++ b/packages/runtime/src/render.ts @@ -252,6 +252,14 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp // Used by the error boundary to route errors through exit(). let exitWithError: (e: Error) => void = () => {}; + // Record-exit-error bridge, wired alongside exitWithError after mount. The + // error boundary calls this SYNCHRONOUSLY (before the deferred exitWithError) + // to set pendingExitError up front, so a racing unmount() that runs + // resolveExit() before the deferred exit rejects with the thrown error instead + // of resolving clean (BUG #2). Mirrors exitWithError's after-mount indirection + // because pendingExitError/exitInitiated/teardownStarted are all in this scope. + let recordExitError: (e: Error) => void = () => {}; + // First-call-wins guard for exit() (Ink parity G33). Ink's handleAppExit // returns early on `isUnmounted || isUnmounting`, so the FIRST exit() call // captures the value/error and initiates teardown while any subsequent @@ -526,20 +534,48 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp const errored = shallowRef(false); onErrorCaptured((err) => { - // Preserve a genuine Error — including a cross-realm one (fails - // `instanceof Error`, passes the `[object Error]` brand check) — so the - // ORIGINAL thrown error reaches exit()/waitUntilExit() unchanged, - // matching Ink's ErrorBoundary (rejects with the thrown value itself). - // A true non-Error throw (`throw "x"`, `throw 0`, `throw {message:'x'}`) - // is wrapped with the SAME message ErrorOverview displays - // (messageForNonError), so the shown and rejected messages agree (e17). - const e = isErrorInput(err) ? err : new Error(messageForNonError(err)); - caught.value = err; - errored.value = true; - // Flush the ErrorOverview frame, then exit - void nextTick(() => { - exitWithError(e); - }); + // First-wins: only the FIRST captured error is recorded and routed to + // exit(). If two descendants throw in the SAME synchronous flush, the + // displayed `caught` and the rejected exit error must stay the SAME + // error — `caught` is last-wins by assignment, while exit() is + // first-wins, so without this guard the overview would show error #2 + // while waitUntilExit() rejects with error #1 (e17 display/reject + // mismatch). Guarding on `errored` keeps both on the first thrown value. + if (!errored.value) { + // Preserve a genuine Error — including a cross-realm one (fails + // `instanceof Error`, passes the `[object Error]` brand check) — so the + // ORIGINAL thrown error reaches exit()/waitUntilExit() unchanged, + // matching Ink's ErrorBoundary (rejects with the thrown value itself). + // A true non-Error throw (`throw "x"`, `throw 0`, `throw {message:'x'}`) + // is wrapped with the SAME message ErrorOverview displays + // (messageForNonError), so the shown and rejected messages agree (e17). + const e = isErrorInput(err) ? err : new Error(messageForNonError(err)); + caught.value = err; + errored.value = true; + // Record the exit error SYNCHRONOUSLY, but keep the teardown DEFERRED. + // Two distinct concerns, decoupled: + // 1. recordExitError(e) sets pendingExitError NOW (first-wins). A host + // that throws during a flush and then synchronously unmounts in the + // SAME task would otherwise have its racing unmount() run + // resolveExit() while pendingExitError is still undefined — + // resolving CLEAN and swallowing the error (the deferred-exit race, + // BUG #2). Recording it up front makes that resolveExit() reject + // with the thrown error. + // 2. exitWithError(e) stays on nextTick so teardown is DEFERRED until + // AFTER the current flush. teardown() runs the final mountedCommit() + // that paints the ErrorOverview frame (on interactive/debug mounts), + // and the boundary's errored→true re-render must commit BEFORE that + // final commit. A synchronous exit here would let teardown's + // microtask run before the re-render, dropping the overview frame. + // Deferring keeps frame/paint timing byte-identical to main. (In the + // racing-unmount case the unmount sets teardownStarted, so this + // later exit() no-ops via the exitInitiated||teardownStarted guard; + // with no race it proceeds normally.) + recordExitError(e); + void nextTick(() => { + exitWithError(e); + }); + } return false; // stop propagation }); @@ -727,7 +763,17 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp // teardown microtask) so a re-entrant exit() — which is blocked above // anyway — and the eventual resolveExit() always settle on this value. if (isErrorInput(errorOrResult)) { - pendingExitError = errorOrResult; + // Don't clobber an error already recorded synchronously by + // recordExitError() (the boundary captured first): first-wins keeps the + // displayed and rejected error the SAME. pendingExitError is undefined on + // a normal first exit(), so `??=` is identical to `=` in every other case. + // (The race: a descendant throws Error1 → onErrorCaptured shows Error1 and + // recordExitError sets pendingExitError=Error1 WITHOUT setting exitInitiated, + // then app code calls exit(Error2) before the deferred exitWithError(Error1) + // microtask runs — exitInitiated is still false so we reach here. `=` would + // overwrite to Error2, making the overview show Error1 while waitUntilExit() + // rejects Error2.) + pendingExitError ??= errorOrResult; } else { pendingExitResult = errorOrResult; } @@ -1202,6 +1248,16 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp // Wire exit-with-error for the error boundary (must be set before mount). exitWithError = (e: Error) => appContext.exit(e); + recordExitError = (e: Error) => { + // First-wins: don't overwrite an exit already decided (a clean exit() or a + // prior error). Records the error so a racing unmount()'s resolveExit() + // rejects with it instead of resolving clean (BUG #2). Mirrors the + // synchronous record in appContext.exit() — pendingExitError is set here, + // then the deferred exitWithError() drives teardown/resolveExit(). + if (!exitInitiated && !teardownStarted && pendingExitError === undefined) { + pendingExitError = e; + } + }; // Alternate screen: enter BEFORE rendering starts (matching Ink ink.tsx:428). // Requires alternateScreen option + interactive + isTTY. diff --git a/packages/testing/src/render.ts b/packages/testing/src/render.ts index 58072fc..30d97f8 100644 --- a/packages/testing/src/render.ts +++ b/packages/testing/src/render.ts @@ -122,8 +122,10 @@ export async function render( earlyError = e as Error; }); - // Flush the Vue queue. Chain: onErrorCaptured → nextTick → exit → queueMicrotask - // → teardown → resolveExit() → stdout.write("", callback) → reject. + // Flush the Vue queue. Chain: onErrorCaptured (records pendingExitError + // synchronously) → nextTick → exit → queueMicrotask → teardown → resolveExit() + // → stdout.write("", callback) → reject. The error is recorded up front so a + // racing unmount() still rejects; teardown stays deferred so the overview paints. // The stdout write barrier fires via process.nextTick (inside stream internals), // so we need setImmediate (runs after all process.nextTick callbacks), then one // more microtask yield so the .catch() handler on exitPromise can set earlyError.