From c66cddb676d6e0df9cbea6f174697c6f2e49ce12 Mon Sep 17 00:00:00 2001 From: Yunfei He Date: Fri, 12 Jun 2026 02:51:19 +0800 Subject: [PATCH] fix(runtime): derive mount-guard skip from wired state, not a sticky flag (#153) The instance-reuse guard set a per-app skippedMount flag that was never reset, so one guarded mount() call permanently disabled the app's own teardown. Three run-confirmed wedges (audit e18), all absent in Ink: - an owner double-firing mount() on its own live stdout kept painting after unmount() and leaked its registry entry - an app that once hit the guard could never unmount a later legitimate mount on a free stdout - an app live on stream A that merely targeted another app's busy stream B became unkillable on A Delete the flag; teardown()/resolveExit() now consult the actually wired state (mountedAppContext / mountedAsOwner), so a guarded call is inert for that call only. The blessed inert-no-op divergence from Ink's reuse-and-rerender is unchanged; the ledger entry is reworded to the call-scoped semantics. Co-authored-by: Claude Fable 5 --- .agents/docs/ink-divergences.md | 14 +- .../lifecycle/instance-reuse-guard.test.tsx | 175 +++++++++++++++++- packages/runtime/src/render.ts | 47 +++-- 3 files changed, 214 insertions(+), 22 deletions(-) diff --git a/.agents/docs/ink-divergences.md b/.agents/docs/ink-divergences.md index fd9af9b..e997768 100644 --- a/.agents/docs/ink-divergences.md +++ b/.agents/docs/ink-divergences.md @@ -371,10 +371,16 @@ different runtime behavior, ownership rule, or out-of-contract handling. - **Ink:** `render()` keeps one instance per stdout (`WeakMap`); a second `render(node, {stdout})` on a stream that already has a live instance warns on stderr but **reuses** that instance and `rerender`s the new tree into it. -- **vue-tui:** a second `mount()` on a still-live stdout warns on stderr and returns an - **inert handle**. It wires no second renderer and renders nothing; the first app's tree - stays on screen. `unmount()`/`teardown()` on that handle never touch the owner's stream or - registry entry (`unmount()` only settles the inert handle's own exit promise). +- **vue-tui:** a second `mount()` on a still-live stdout warns on stderr, wires no second + renderer, renders nothing, and returns an empty placeholder object (the real controls — + `unmount()`, `waitUntilExit()` — live on the app, not on `mount()`'s return value). The + first app's tree stays on screen. The skip is scoped to that one guarded call — derived + from what the app actually wired, never sticky: a guarded _different_ app's `unmount()` + settles only its own exit promise and never touches the owner's stream or registry entry; + the _owner_ double-firing `mount()` on its own stdout keeps a fully working `unmount()` + (the warning's recovery path); an app that once hit the guard can later mount — and + cleanly unmount — on a free stdout; and a live app that merely targeted another app's + busy stream stays fully killable. - **Why:** a second `mount()` on a live stdout is a misuse (forgot to `unmount()`, a re-render glitch fired `mount()` twice, or expecting `mount()` to re-render — it doesn't; update reactive state for that). Ink treats it as unsupported and warns too. vue-tui fails diff --git a/packages/runtime-tests/integration/lifecycle/instance-reuse-guard.test.tsx b/packages/runtime-tests/integration/lifecycle/instance-reuse-guard.test.tsx index ff8c23d..408c9ff 100644 --- a/packages/runtime-tests/integration/lifecycle/instance-reuse-guard.test.tsx +++ b/packages/runtime-tests/integration/lifecycle/instance-reuse-guard.test.tsx @@ -8,15 +8,27 @@ * must work normally. */ -import { defineComponent } from "vue"; +import { defineComponent, nextTick, shallowRef } from "vue"; import { expect, test, vi, afterEach } from "vite-plus/test"; import { createApp, Text } from "@vue-tui/runtime"; -import { makeFakeWritable, makeFakeStdin } from "./test-streams.ts"; +import { captureWrites, makeFakeWritable, makeFakeStdin } from "./test-streams.ts"; afterEach(() => { vi.restoreAllMocks(); }); +/** Spy on native process.stderr (where the reuse-guard warning is written). */ +function spyOnGuardWarnings(): { warnings: string[]; restore: () => void } { + const warnings: string[] = []; + const spy = vi.spyOn(process.stderr, "write").mockImplementation((chunk) => { + warnings.push(typeof chunk === "string" ? chunk : String(chunk)); + return true; + }); + return { warnings, restore: () => spy.mockRestore() }; +} + +const GUARD_WARNING = "this stdout already has a live app"; + test("warn + skip wiring when mount() is called on an already-live stdout", async () => { const App = defineComponent(() => () => hello); @@ -130,3 +142,162 @@ test("unmounting first app allows a subsequent mount on the same stdout (no warn await app2.waitUntilRenderFlush(); app2.unmount(); }); + +// The three tests below pin the guard's CALL-SCOPED semantics (audit e18): a +// guarded mount() is inert for THAT call only — it must never poison the app's +// ability to tear down the mount it ACTUALLY wired. Each uses debug mode (every +// commit writes the full frame to stdout immediately) so "a frame painted after +// unmount()" is directly observable on the fake stream, mirroring the run-based +// probes (/tmp/ink-audit/e18x-*.mjs) that established the bug. + +test("owner double-firing mount() on its own live stdout keeps a working unmount()", async () => { + // e18x case (a): app1 mounts, then mistakenly calls mount() AGAIN on its own + // live stdout. The second call must warn and stay inert, but the warning's + // own prescribed recovery — unmount() the existing app — must still tear + // down the FIRST (real) mount: no frame after unmount, registry entry freed. + const msg = shallowRef("OWNER-A"); + const App = defineComponent(() => () => {msg.value}); + + const stdout = makeFakeWritable(); + const stderr = makeFakeWritable(); + const { stream: stdin } = makeFakeStdin(); + const writes = captureWrites(stdout); + const { warnings, restore } = spyOnGuardWarnings(); + + const app1 = createApp(App); + app1.mount({ stdout, stdin, stderr, debug: true, exitOnCtrlC: false }); + await nextTick(); + await nextTick(); + expect(writes.join("")).toContain("OWNER-A"); + + // Double-fire: warns, wires nothing. + app1.mount({ stdout, stdin, stderr, debug: true, exitOnCtrlC: false }); + expect(warnings.join("")).toContain(GUARD_WARNING); + + // The recovery path: unmount() must tear down the real first mount. + const exit1 = app1.waitUntilExit(); + app1.unmount(); + await exit1; + + // (1) No frame paints after unmount. + const writesAtUnmount = writes.length; + msg.value = "OWNER-B-AFTER-UNMOUNT"; + await nextTick(); + await nextTick(); + expect(writes.slice(writesAtUnmount).join("")).not.toContain("OWNER-B-AFTER-UNMOUNT"); + + // (2) Registry entry freed: a fresh mount on the same stdout must NOT warn. + warnings.length = 0; + const app2 = createApp(defineComponent(() => () => FRESH)); + app2.mount({ stdout, stdin, stderr, debug: true, exitOnCtrlC: false }); + expect(warnings.join("")).not.toContain(GUARD_WARNING); + restore(); + app2.unmount(); +}); + +test("an app that once hit the guard can later mount AND unmount on a free stdout", async () => { + // e18x case (b): appY first hits the guard on a busy stdout A, then mounts + // legitimately on a FREE stdout B. The earlier guarded call must not poison + // appY: unmount() must tear down the B renderer and free B's registry entry. + const stdoutA = makeFakeWritable(); + const stdoutB = makeFakeWritable(); + const stderr = makeFakeWritable(); + const { stream: stdinA } = makeFakeStdin(); + const { stream: stdinB } = makeFakeStdin(); + const { warnings, restore } = spyOnGuardWarnings(); + + const owner = createApp(defineComponent(() => () => OWNER-ON-A)); + owner.mount({ stdout: stdoutA, stdin: stdinA, stderr, debug: true, exitOnCtrlC: false }); + await nextTick(); + await nextTick(); + + const msg = shallowRef("Y-ON-B-FIRST"); + const appY = createApp(defineComponent(() => () => {msg.value})); + + // Guarded call on busy A: warns, inert. + appY.mount({ stdout: stdoutA, stdin: stdinB, stderr, debug: true, exitOnCtrlC: false }); + expect(warnings.join("")).toContain(GUARD_WARNING); + + // Legitimate mount on free B: renders normally. + const writesB = captureWrites(stdoutB); + appY.mount({ stdout: stdoutB, stdin: stdinB, stderr, debug: true, exitOnCtrlC: false }); + await nextTick(); + await nextTick(); + expect(writesB.join("")).toContain("Y-ON-B-FIRST"); + + // unmount() must tear down the B renderer... + appY.unmount(); + const writesAtUnmount = writesB.length; + msg.value = "Y-ON-B-AFTER-UNMOUNT"; + await nextTick(); + await nextTick(); + expect(writesB.slice(writesAtUnmount).join("")).not.toContain("Y-ON-B-AFTER-UNMOUNT"); + + // ...and free B's registry entry: a fresh mount on B must NOT warn. + warnings.length = 0; + const appZ = createApp(defineComponent(() => () => FRESH-ON-B)); + appZ.mount({ stdout: stdoutB, stdin: stdinB, stderr, debug: true, exitOnCtrlC: false }); + expect(warnings.join("")).not.toContain(GUARD_WARNING); + restore(); + appZ.unmount(); + owner.unmount(); +}); + +test("targeting another app's busy stdout never poisons the caller's own live mount", async () => { + // e18x case (c): app1 is live on stream A; it calls mount() targeting BUSY + // stream B (owned by app2). The guarded call must stay scoped to itself: + // app1's live A mount must remain fully killable, and app2 on B untouched. + const stdoutA = makeFakeWritable(); + const stdoutB = makeFakeWritable(); + const stderr = makeFakeWritable(); + const { stream: stdinA } = makeFakeStdin(); + const { stream: stdinB } = makeFakeStdin(); + const writesA = captureWrites(stdoutA); + const { warnings, restore } = spyOnGuardWarnings(); + + const msgA = shallowRef("APP1-ON-A-FIRST"); + const app1 = createApp(defineComponent(() => () => {msgA.value})); + app1.mount({ stdout: stdoutA, stdin: stdinA, stderr, debug: true, exitOnCtrlC: false }); + await nextTick(); + await nextTick(); + expect(writesA.join("")).toContain("APP1-ON-A-FIRST"); + + const app2 = createApp(defineComponent(() => () => APP2-OWNS-B)); + app2.mount({ stdout: stdoutB, stdin: stdinB, stderr, debug: true, exitOnCtrlC: false }); + await nextTick(); + await nextTick(); + + // app1 (live on A) targets busy B: warns, inert for that call only. + app1.mount({ stdout: stdoutB, stdin: stdinA, stderr, debug: true, exitOnCtrlC: false }); + expect(warnings.join("")).toContain(GUARD_WARNING); + + // app1's REAL mount on A must still be killable. + const exit1 = app1.waitUntilExit(); + app1.unmount(); + await exit1; + const writesAtUnmount = writesA.length; + msgA.value = "APP1-ON-A-AFTER-UNMOUNT"; + await nextTick(); + await nextTick(); + expect(writesA.slice(writesAtUnmount).join("")).not.toContain("APP1-ON-A-AFTER-UNMOUNT"); + + // A's registry entry freed: a fresh mount on A must NOT warn. + warnings.length = 0; + const app3 = createApp(defineComponent(() => () => FRESH-ON-A)); + app3.mount({ stdout: stdoutA, stdin: stdinA, stderr, debug: true, exitOnCtrlC: false }); + expect(warnings.join("")).not.toContain(GUARD_WARNING); + + // Control: app2 still owns B (a mount attempt on B still warns)... + warnings.length = 0; + const probe = createApp(defineComponent(() => () => PROBE)); + probe.mount({ stdout: stdoutB, stdin: stdinB, stderr, debug: true, exitOnCtrlC: false }); + expect(warnings.join("")).toContain(GUARD_WARNING); + restore(); + + // ...and app2 unmounts cleanly. + const exit2 = app2.waitUntilExit(); + app2.unmount(); + await exit2; + probe.unmount(); + app3.unmount(); +}); diff --git a/packages/runtime/src/render.ts b/packages/runtime/src/render.ts index 7865c37..245290b 100644 --- a/packages/runtime/src/render.ts +++ b/packages/runtime/src/render.ts @@ -257,14 +257,15 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp let mountedAlternateScreen = false; let mountedClear: (() => void) | null = null; let mountedKittyController: ReturnType | null = null; - // Tracks whether this app is the owner of the liveInstances entry for its - // stdout. A second mount that hits the guard sets this to false so teardown() - // does not evict the first app's entry. + // Tracks whether this app currently owns the liveInstances entry for its + // stdout — set when a mount() actually wires a renderer, cleared when + // teardown() evicts the entry. A mount() that hits the instance-reuse guard + // wires nothing and leaves this (and all other mounted* state) untouched: + // whether unmount()/teardown() have real work to do is derived from the + // actually-wired state, never from a sticky "was ever guarded" flag (audit + // e18 — a sticky flag let one guarded call disable teardown of a mount the + // app DID wire). let mountedAsOwner = false; - // Set to true when mount() hit the instance-reuse guard and returned early. - // unmount()/teardown()/resolveExit() must be complete no-ops in that case — - // they must not touch the owner's stream or WeakMap entry. - let skippedMount = false; // The renderer's onCommit closure is wired at createApp time but only does // real work after mount swaps in scheduler.schedule. One renderer per app @@ -277,9 +278,13 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp let pendingExitResult: unknown = undefined; function resolveExit() { - // Skipped mount: no stream was ever wired; resolve the exit promise directly - // without any write-barrier so the owner's stdout is never touched. - if (skippedMount) { + // Nothing wired: this app never mounted a renderer (every mount() either + // never happened or hit the instance-reuse guard, which wires nothing). + // Settle the exit promise directly without any write-barrier so no stream + // — in particular a guarded stream's owner — is ever touched. Apps that + // DID wire a renderer always have mountedAppContext set, so a guarded + // call can never reroute their exit settling away from the real stream. + if (!mountedAppContext) { if (isErrorInput(pendingExitError)) { exitReject(pendingExitError); } else { @@ -336,9 +341,15 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp // synchronously (fs.writeSync) so they reach the fd before signal-exit // re-raises the signal. The normal unmount()/exit() path keeps async writes. function teardown(sync = false) { - // Skipped mount: this app never wired a renderer, so teardown is a - // complete no-op — do not touch any stream or the owner's WeakMap entry. - if (skippedMount) return; + // Nothing wired: this app never mounted a renderer (never mounted, or + // every mount() hit the instance-reuse guard, which wires nothing), so + // teardown is a complete no-op — do not touch any stream or another + // app's WeakMap entry. Derived from actual wired state, NOT a sticky + // "was ever guarded" flag: a guarded mount() call is inert for that call + // only and must never disable teardown of a mount this app DID wire + // (double-fire on its own live stdout, a later mount on a free stdout, + // or merely targeting another app's busy stream — audit e18). + if (!mountedAppContext) return; if (teardownStarted) return; teardownStarted = true; @@ -538,13 +549,17 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp // must unmount() the first app before mounting on the same stream. // We write the warning directly to native process.stderr so an existing // alternate-screen renderer cannot swallow it via patchConsole. + // The skip is scoped to THIS call only: it wires nothing, mutates no + // per-app state, and returns an inert handle. unmount()/teardown()/ + // resolveExit() consult the actually-wired state (mountedAppContext / + // mountedAsOwner), so a guarded call never affects the app's ability to + // tear down a mount it really wired (audit e18: a sticky skip flag here + // made the owner's double-fire — and even targeting someone else's busy + // stream — permanently disable the app's own teardown). if (liveInstances.has(stdout)) { process.stderr.write( "Warning: this stdout already has a live app, so this mount() was ignored. To update the current view, change its reactive state instead of remounting; to mount another app, unmount() the existing one first.\n", ); - // Mark this app as skipped so unmount()/teardown()/resolveExit() are - // complete no-ops — they must never touch the owner's stream or WeakMap entry. - skippedMount = true; return {} as ComponentPublicInstance; }