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 <noreply@anthropic.com>
This commit is contained in:
@@ -371,10 +371,16 @@ different runtime behavior, ownership rule, or out-of-contract handling.
|
|||||||
- **Ink:** `render()` keeps one instance per stdout (`WeakMap<WriteStream, Ink>`); a second
|
- **Ink:** `render()` keeps one instance per stdout (`WeakMap<WriteStream, Ink>`); a second
|
||||||
`render(node, {stdout})` on a stream that already has a live instance warns on stderr but
|
`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.
|
**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
|
- **vue-tui:** a second `mount()` on a still-live stdout warns on stderr, wires no second
|
||||||
**inert handle**. It wires no second renderer and renders nothing; the first app's tree
|
renderer, renders nothing, and returns an empty placeholder object (the real controls —
|
||||||
stays on screen. `unmount()`/`teardown()` on that handle never touch the owner's stream or
|
`unmount()`, `waitUntilExit()` — live on the app, not on `mount()`'s return value). The
|
||||||
registry entry (`unmount()` only settles the inert handle's own exit promise).
|
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
|
- **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;
|
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
|
update reactive state for that). Ink treats it as unsupported and warns too. vue-tui fails
|
||||||
|
|||||||
@@ -8,15 +8,27 @@
|
|||||||
* must work normally.
|
* must work normally.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { defineComponent } from "vue";
|
import { defineComponent, nextTick, shallowRef } from "vue";
|
||||||
import { expect, test, vi, afterEach } from "vite-plus/test";
|
import { expect, test, vi, afterEach } from "vite-plus/test";
|
||||||
import { createApp, Text } from "@vue-tui/runtime";
|
import { createApp, Text } from "@vue-tui/runtime";
|
||||||
import { makeFakeWritable, makeFakeStdin } from "./test-streams.ts";
|
import { captureWrites, makeFakeWritable, makeFakeStdin } from "./test-streams.ts";
|
||||||
|
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
vi.restoreAllMocks();
|
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 () => {
|
test("warn + skip wiring when mount() is called on an already-live stdout", async () => {
|
||||||
const App = defineComponent(() => () => <Text>hello</Text>);
|
const App = defineComponent(() => () => <Text>hello</Text>);
|
||||||
|
|
||||||
@@ -130,3 +142,162 @@ test("unmounting first app allows a subsequent mount on the same stdout (no warn
|
|||||||
await app2.waitUntilRenderFlush();
|
await app2.waitUntilRenderFlush();
|
||||||
app2.unmount();
|
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(() => () => <Text>{msg.value}</Text>);
|
||||||
|
|
||||||
|
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(() => () => <Text>FRESH</Text>));
|
||||||
|
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(() => () => <Text>OWNER-ON-A</Text>));
|
||||||
|
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(() => () => <Text>{msg.value}</Text>));
|
||||||
|
|
||||||
|
// 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(() => () => <Text>FRESH-ON-B</Text>));
|
||||||
|
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(() => () => <Text>{msgA.value}</Text>));
|
||||||
|
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(() => () => <Text>APP2-OWNS-B</Text>));
|
||||||
|
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(() => () => <Text>FRESH-ON-A</Text>));
|
||||||
|
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(() => () => <Text>PROBE</Text>));
|
||||||
|
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();
|
||||||
|
});
|
||||||
|
|||||||
@@ -257,14 +257,15 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp
|
|||||||
let mountedAlternateScreen = false;
|
let mountedAlternateScreen = false;
|
||||||
let mountedClear: (() => void) | null = null;
|
let mountedClear: (() => void) | null = null;
|
||||||
let mountedKittyController: ReturnType<typeof createKittyKeyboardController> | null = null;
|
let mountedKittyController: ReturnType<typeof createKittyKeyboardController> | null = null;
|
||||||
// Tracks whether this app is the owner of the liveInstances entry for its
|
// Tracks whether this app currently owns the liveInstances entry for its
|
||||||
// stdout. A second mount that hits the guard sets this to false so teardown()
|
// stdout — set when a mount() actually wires a renderer, cleared when
|
||||||
// does not evict the first app's entry.
|
// 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;
|
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
|
// 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
|
// 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;
|
let pendingExitResult: unknown = undefined;
|
||||||
|
|
||||||
function resolveExit() {
|
function resolveExit() {
|
||||||
// Skipped mount: no stream was ever wired; resolve the exit promise directly
|
// Nothing wired: this app never mounted a renderer (every mount() either
|
||||||
// without any write-barrier so the owner's stdout is never touched.
|
// never happened or hit the instance-reuse guard, which wires nothing).
|
||||||
if (skippedMount) {
|
// 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)) {
|
if (isErrorInput(pendingExitError)) {
|
||||||
exitReject(pendingExitError);
|
exitReject(pendingExitError);
|
||||||
} else {
|
} 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
|
// synchronously (fs.writeSync) so they reach the fd before signal-exit
|
||||||
// re-raises the signal. The normal unmount()/exit() path keeps async writes.
|
// re-raises the signal. The normal unmount()/exit() path keeps async writes.
|
||||||
function teardown(sync = false) {
|
function teardown(sync = false) {
|
||||||
// Skipped mount: this app never wired a renderer, so teardown is a
|
// Nothing wired: this app never mounted a renderer (never mounted, or
|
||||||
// complete no-op — do not touch any stream or the owner's WeakMap entry.
|
// every mount() hit the instance-reuse guard, which wires nothing), so
|
||||||
if (skippedMount) return;
|
// 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;
|
if (teardownStarted) return;
|
||||||
teardownStarted = true;
|
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.
|
// must unmount() the first app before mounting on the same stream.
|
||||||
// We write the warning directly to native process.stderr so an existing
|
// We write the warning directly to native process.stderr so an existing
|
||||||
// alternate-screen renderer cannot swallow it via patchConsole.
|
// 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)) {
|
if (liveInstances.has(stdout)) {
|
||||||
process.stderr.write(
|
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",
|
"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;
|
return {} as ComponentPublicInstance;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user