From 39686fc0adc658d98bffce555df9d464e3ac62da Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Thu, 23 Jul 2026 11:55:58 +0200 Subject: [PATCH 1/2] refactor(logger): Restore cursor on ui5 serve termination log-update hides the terminal cursor on every render and only shows it again in its own done(), which InteractiveConsole calls from disable(). On CTRL+C the process is signal-terminated without disable() ever running, so the cursor stays hidden in the user's shell. The restore-cursor safety net (a bare "\x1b[?25h" with no newline) is also swallowed: the stderr write interceptor buffers non-newline-terminated writes into `partial` and only flushes them in disable(). Register a handler for SIGINT/SIGTERM/SIGHUP/SIGBREAK (and an `exit` catch-all) in enable(), removed in disable(). Each signal handler tears the live region down via disable(), then re-raises the signal with process.kill(process.pid, signal) rather than calling process.exit(). Re-raising lets the cooperating handlers in profile.js and ProjectBuilder still run their async cleanup, and lets the default disposition set the 128+n exit code. disable() removes the handlers before re-raise so the re-sent signal reaches the default disposition instead of looping. disable() now also writes the show-cursor escape explicitly to its own stream, after uninstalling the interceptor. log-update's done() targets the real process.stderr and is gated on isTTY, so it can be a no-op; the explicit write restores the cursor regardless and is observable in tests. --- .../logger/lib/writers/InteractiveConsole.js | 55 ++++++++++++ .../test/lib/writers/InteractiveConsole.js | 90 +++++++++++++++++++ 2 files changed, 145 insertions(+) diff --git a/packages/logger/lib/writers/InteractiveConsole.js b/packages/logger/lib/writers/InteractiveConsole.js index 55e78334f99..308eb2dc0d8 100644 --- a/packages/logger/lib/writers/InteractiveConsole.js +++ b/packages/logger/lib/writers/InteractiveConsole.js @@ -23,6 +23,14 @@ const BUILDING_TICK_MS = 120; // errors from these modules, and all other levels, still pass through. const BANNER_REDUNDANT_INFO_MODULES = new Set(["ProjectBuilder"]); +// DECTCEM "show cursor". log-update hides the cursor on each render; disable() +// emits this to restore it. +const CURSOR_SHOW = "[?25h"; + +// Signals that terminate the process while the live region is on-screen. +// SIGBREAK exists only on Windows; on other platforms the listener is inert. +const TERMINATION_SIGNALS = ["SIGHUP", "SIGINT", "SIGTERM", "SIGBREAK"]; + // Decode the tail of `stream.write(chunk[, encoding][, callback])`. `encoding` // falls back to "utf8" (Node's default) when not supplied. function parseWriteArgs(encodingOrCallback, maybeCallback) { @@ -97,6 +105,10 @@ class InteractiveConsole { #onStopConsole; #onResize; + // Termination-signal handlers (plus an `exit` catch-all), keyed by event + // name, so disable() detaches exactly what enable() registered. + #signalHandlers = new Map(); + constructor({stderr = process.stderr} = {}) { this.#stderr = stderr; this.#headerState = createHeaderState(); @@ -117,6 +129,7 @@ class InteractiveConsole { process.emit("ui5.log.stop-console"); this.#stopped = false; this.#attachListeners(); + this.#registerSignalHandlers(); if (!this.#logUpdate) { this.#logUpdate = createLogUpdate(this.#stderr); } @@ -138,6 +151,9 @@ class InteractiveConsole { this.#stopped = true; this.#clearTick(); this.#detachListeners(); + // Detach before the terminal teardown so a re-raised signal (see + // #handleTerminationSignal) doesn't re-enter our handler. + this.#deregisterSignalHandlers(); // Flush any partial (non-newline-terminated) buffered writes before // we tear down the live region, then restore the process.* originals. // Flushing first while the live region is still on-screen keeps the @@ -148,9 +164,48 @@ class InteractiveConsole { // and resets state. End on a clean newline so the prompt lands // below the final frame. this.#withRenderingGuard(() => this.#logUpdate?.done()); + // done() is a no-op on a non-TTY stderr, so restore the cursor + // explicitly. This runs after #uninstallWriteInterceptors so the escape + // reaches the stream instead of being buffered as a partial write. + this.#stderr.write(CURSOR_SHOW); this.#stderr.write("\n"); } + // ---- Termination-signal teardown ----------------------------------------- + // On CTRL+C the process dies without disable() ever running, leaving the + // cursor hidden. Tear the live region down on the terminating signals, then + // re-raise so cooperating handlers (profile.js, ProjectBuilder) still run. + + #registerSignalHandlers() { + for (const signal of TERMINATION_SIGNALS) { + const handler = () => this.#handleTerminationSignal(signal); + this.#signalHandlers.set(signal, handler); + process.on(signal, handler); + } + // `exit` covers cooperating handlers that call process.exit(); it never + // fires on a bare signal kill, hence the signal handlers above. + const onExit = () => this.disable(); + this.#signalHandlers.set("exit", onExit); + process.on("exit", onExit); + } + + #deregisterSignalHandlers() { + for (const [event, handler] of this.#signalHandlers) { + process.off(event, handler); + } + this.#signalHandlers.clear(); + } + + #handleTerminationSignal(signal) { + // Re-raise only if this call did the teardown, so a stale handler + // invoked after disable() can't kill an already-stopped process. + const wasStopped = this.#stopped; + this.disable(); + if (!wasStopped) { + process.kill(process.pid, signal); + } + } + // Compose the full live region as a single string. One block per region, // separated by their own leading blank line. `log-update` handles wrapping // and erase of the previous frame, so we just hand it the full text. diff --git a/packages/logger/test/lib/writers/InteractiveConsole.js b/packages/logger/test/lib/writers/InteractiveConsole.js index 936a9dcbfa3..c16c709f3d1 100644 --- a/packages/logger/test/lib/writers/InteractiveConsole.js +++ b/packages/logger/test/lib/writers/InteractiveConsole.js @@ -986,3 +986,93 @@ test.serial("ui5.log.stop-console handler calls disable() on the active writer", const output = stripAnsi(stderr.writes.join("")); t.notRegex(output, /post-stop warn/, "listeners are detached after the stop-console handler"); }); + +// ---- Termination-signal teardown -------------------------------------------- +// enable() registers a handler per terminating signal; on CTRL+C it tears the +// region down (restoring the cursor) and re-raises. The tests capture the +// handler via process.listeners(signal) and stub process.kill, so invoking it +// directly exercises the teardown without terminating the AVA worker. + +const CURSOR_SHOW = "[?25h"; + +test.serial("enable() registers a handler for each termination signal", (t) => { + const {writer} = createWriter(); + // Capture the writer's own handler per signal by identity, so the assertions + // hold regardless of other listeners present on the shared process object. + const registered = {}; + for (const signal of ["SIGHUP", "SIGINT", "SIGTERM", "SIGBREAK"]) { + const handler = process.listeners(signal).at(-1); + t.is(typeof handler, "function", `${signal} handler registered on enable`); + registered[signal] = handler; + } + writer.disable(); + // disable() removes exactly what enable() added. + for (const signal of ["SIGHUP", "SIGINT", "SIGTERM", "SIGBREAK"]) { + t.false(process.listeners(signal).includes(registered[signal]), + `${signal} handler removed on disable`); + } +}); + +test.serial("SIGINT handler restores the cursor and re-raises the signal", (t) => { + const killStub = sinon.stub(process, "kill"); + t.teardown(() => killStub.restore()); + + const {stderr} = createWriter(); + const handler = process.listeners("SIGINT").at(-1); + t.is(typeof handler, "function", "SIGINT handler captured"); + + stderr.writes.length = 0; + handler(); // invoke directly: process.kill is stubbed, so the worker survives + + const raw = stderr.writes.join(""); + t.true(raw.includes(CURSOR_SHOW), "show-cursor escape written on SIGINT teardown"); + t.true(raw.endsWith("\n"), "trailing newline written after teardown"); + t.true(killStub.calledOnceWithExactly(process.pid, "SIGINT"), + "signal re-raised to self rather than process.exit()"); + + // Own listener must be gone before the re-raise, else it loops. + t.false(process.listeners("SIGINT").includes(handler), + "own SIGINT listener removed before re-raise"); +}); + +test.serial("signal teardown detaches ui5 event listeners", (t) => { + const killStub = sinon.stub(process, "kill"); + t.teardown(() => killStub.restore()); + + const {stderr} = createWriter(); + process.listeners("SIGINT").at(-1)(); + + stderr.writes.length = 0; + process.emit("ui5.log", {level: "warn", message: "post-signal warn"}); + const output = stripAnsi(stderr.writes.join("")); + t.notRegex(output, /post-signal warn/, "ui5.* listeners detached after signal teardown"); +}); + +test.serial("a stale signal handler invoked after disable() does not re-raise", (t) => { + const killStub = sinon.stub(process, "kill"); + t.teardown(() => killStub.restore()); + + const {writer} = createWriter(); + const handler = process.listeners("SIGINT").at(-1); + writer.disable(); // first teardown; removes the real listener + + killStub.resetHistory(); + // A stale handler invoked after disable() must be a no-op: disable() + // short-circuits on #stopped and the guarded re-raise never fires. + t.notThrows(() => handler()); + t.true(killStub.notCalled, "no re-raise after the writer was already stopped"); +}); + +test.serial("process 'exit' handler tears down as a best-effort catch-all", (t) => { + const {writer, stderr} = createWriter(); + const exitHandler = process.listeners("exit").at(-1); + t.is(typeof exitHandler, "function", "exit handler registered on enable"); + + stderr.writes.length = 0; + exitHandler(); + + const raw = stderr.writes.join(""); + t.true(raw.includes(CURSOR_SHOW), "cursor restored via the exit catch-all"); + // Idempotent with a later explicit disable(). + t.notThrows(() => writer.disable()); +}); From 55f55465aea7c8ba0734181a0e6f66ac0c527419 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Fri, 24 Jul 2026 11:01:24 +0200 Subject: [PATCH 2/2] refactor(logger): Make signal handler registration idempotent A second enable() without an intervening disable() overwrote every #signalHandlers map entry with fresh closures, stranding the first set of handlers on process. #deregisterSignalHandlers() iterates the map, so it could only detach the latest set, leaking the originals. Guard #registerSignalHandlers() on the map already being populated and return early, mirroring how disable() guards on #stopped. Registration and deregistration happen as a unit, so a single check covers all signals. --- .../logger/lib/writers/InteractiveConsole.js | 5 ++ .../test/lib/writers/InteractiveConsole.js | 59 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/packages/logger/lib/writers/InteractiveConsole.js b/packages/logger/lib/writers/InteractiveConsole.js index 308eb2dc0d8..3eec54e212e 100644 --- a/packages/logger/lib/writers/InteractiveConsole.js +++ b/packages/logger/lib/writers/InteractiveConsole.js @@ -177,6 +177,11 @@ class InteractiveConsole { // re-raise so cooperating handlers (profile.js, ProjectBuilder) still run. #registerSignalHandlers() { + // Re-registering would strand the previous handlers on `process`: + // #deregisterSignalHandlers() can only detach what the map tracks. + if (this.#signalHandlers.size > 0) { + return; + } for (const signal of TERMINATION_SIGNALS) { const handler = () => this.#handleTerminationSignal(signal); this.#signalHandlers.set(signal, handler); diff --git a/packages/logger/test/lib/writers/InteractiveConsole.js b/packages/logger/test/lib/writers/InteractiveConsole.js index c16c709f3d1..09bfcbef0c0 100644 --- a/packages/logger/test/lib/writers/InteractiveConsole.js +++ b/packages/logger/test/lib/writers/InteractiveConsole.js @@ -947,6 +947,65 @@ test.serial("enable() reuses an existing log-update instance across disable+enab writer.disable(); }); +test.serial("a second enable() does not leak signal handlers", (t) => { + // A re-enable self-disables via ui5.log.stop-console, so the SIGINT handler + // count stays at one and returns to baseline after a single disable(). + const stderr = createStubStderr(); + const baseline = process.listenerCount("SIGINT"); + const writer = new InteractiveConsole({stderr}); + + writer.enable(); + const afterEnable = process.listenerCount("SIGINT"); + t.is(afterEnable, baseline + 1, "enable() registers exactly one SIGINT handler"); + + writer.enable(); + t.is(process.listenerCount("SIGINT"), afterEnable, + "second enable() does not add another SIGINT handler"); + + writer.disable(); + t.is(process.listenerCount("SIGINT"), baseline, + "a single disable() detaches the handler, leaving no leak"); +}); + +test.serial("#registerSignalHandlers() is a no-op when the map is already populated", (t) => { + // Reach the size guard: drop the writer's ui5.log.stop-console listener so + // the second enable() skips its self-disable and registration runs with the + // map still populated. #attachListeners() runs again unguarded, so restore + // process's listeners to baseline on teardown. + const events = [ + "ui5.log", "ui5.build-metadata", "ui5.build-status", "ui5.project-build-status", + "ui5.serve-status", "ui5.tool-info", "ui5.tool-mode", "ui5.project-resolved", + "ui5.server-listening", "ui5.log.stop-console", + "SIGHUP", "SIGINT", "SIGTERM", "SIGBREAK", "exit", + ]; + const snapshot = new Map(events.map((e) => [e, new Set(process.listeners(e))])); + t.teardown(() => { + for (const [event, original] of snapshot) { + for (const listener of process.listeners(event)) { + if (!original.has(listener)) { + process.off(event, listener); + } + } + } + }); + + const stderr = createStubStderr(); + const writer = new InteractiveConsole({stderr}); + writer.enable(); + const firstHandler = process.listeners("SIGINT").at(-1); + + // Drop the stop-console listener enable() attached, so the next emit skips + // the self-disable. + process.off("ui5.log.stop-console", process.listeners("ui5.log.stop-console").at(-1)); + + writer.enable(); + + // Guard returned early: the first SIGINT handler is still live, not + // overwritten by a second registration. + t.is(process.listeners("SIGINT").at(-1), firstHandler, + "map-populated guard preserves the originally-registered handler"); +}); + test.serial("spinner tick timer without unref support is left as-is", (t) => { // Node returns Timeout objects with an unref() method; some polyfills and // runtimes don't. The writer must survive that shape — it doesn't rely on