From 4291c176227a6b7ccb73899354fb24327fd2bc1f Mon Sep 17 00:00:00 2001 From: Michael Hackner Date: Sat, 5 Sep 2026 17:22:19 -0700 Subject: [PATCH] fix(ui): keep suspended jobs alive upon fg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When backgrounding with Ctrl-Z in the shell, fg can kill the process instead of bringing it back to the foreground. Before stopping Hunk, OpenTUI prepares the terminal for the shell: it leaves raw mode, stops reading stdin, and removes its keep-alive timer. The stopped process remains alive, but after fg wakes it, the runtime could exit before Hunk's SIGCONT callback restarted the renderer. The stop actually takes effect before kill returns, since POSIX delivers a signal sent to the caller's own process group before the call completes. So suspend and resume become one straight line, and staying on that call stack is what keeps the job alive: the event loop never gets a chance to go idle and exit. That retires the SIGCONT listener along with the per-suspend state tracking it needed. It also closes a hang. The old fallback only ran when kill threw, so a SIGTSTP that was delivered and discarded — as POSIX requires for an orphaned process group — left the app suspended forever, waiting for a SIGCONT nobody would send. Both cases now reach the same restore. Co-authored-by: Claude Opus 5 --- .changeset/steady-jobs-resume.md | 5 ++ .../hunk/src/core/process/jobControl.test.ts | 87 +++---------------- packages/hunk/src/core/process/jobControl.ts | 44 ++++------ test/pty/lifecycle.test.ts | 60 +++++++++++++ 4 files changed, 94 insertions(+), 102 deletions(-) create mode 100644 .changeset/steady-jobs-resume.md diff --git a/.changeset/steady-jobs-resume.md b/.changeset/steady-jobs-resume.md new file mode 100644 index 000000000..23efd2b4a --- /dev/null +++ b/.changeset/steady-jobs-resume.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Keep suspended Hunk jobs alive so `fg` restores the TUI and its in-progress state. diff --git a/packages/hunk/src/core/process/jobControl.test.ts b/packages/hunk/src/core/process/jobControl.test.ts index 3a0170306..eb12c3a35 100644 --- a/packages/hunk/src/core/process/jobControl.test.ts +++ b/packages/hunk/src/core/process/jobControl.test.ts @@ -57,54 +57,6 @@ function createMockRenderer() { }; } -function createSignalHarness() { - const listeners = new Map void>>(); - const onceWrappers = new Map<() => void, () => void>(); - const removed: NodeJS.Signals[] = []; - - return { - emit(signal: NodeJS.Signals) { - const signalListeners = listeners.get(signal); - if (!signalListeners) { - return; - } - - const snapshot = Array.from(signalListeners); - for (const listener of snapshot) { - listener(); - } - }, - listenerCount(signal: NodeJS.Signals) { - return listeners.get(signal)?.size ?? 0; - }, - off(signal: NodeJS.Signals, listener: () => void) { - removed.push(signal); - listeners.get(signal)?.delete(listener); - const wrapped = onceWrappers.get(listener); - if (wrapped) { - listeners.get(signal)?.delete(wrapped); - onceWrappers.delete(listener); - } - }, - once(signal: NodeJS.Signals, listener: () => void) { - const wrapped = () => { - listeners.get(signal)?.delete(wrapped); - onceWrappers.delete(listener); - listener(); - }; - onceWrappers.set(listener, wrapped); - - let signalListeners = listeners.get(signal); - if (!signalListeners) { - signalListeners = new Set(); - listeners.set(signal, signalListeners); - } - signalListeners.add(wrapped); - }, - removed, - }; -} - describe("installJobControlInterruptSupport", () => { test("routes Ctrl-C through the provided shutdown callback", () => { const renderer = createMockRenderer(); @@ -185,45 +137,40 @@ describe("installJobControlSuspendSupport", () => { expect(sentSignals).toEqual([]); }); - test("suspends the foreground process group on Ctrl-Z and resumes on SIGCONT", () => { + test("suspends the foreground process group on Ctrl-Z and resumes once the job continues", () => { const renderer = createMockRenderer(); - const signals = createSignalHarness(); const sentSignals: Array<{ pid: number; signal: NodeJS.Signals }> = []; installJobControlSuspendSupport(renderer, { - kill: (pid, signal) => sentSignals.push({ pid, signal }), - off: signals.off, - once: signals.once, + kill: (pid, signal) => { + // Stands in for the stopped process: the renderer stays suspended until kill returns. + sentSignals.push({ pid, signal }); + expect(renderer.suspendCalls).toBe(1); + expect(renderer.resumeCalls).toBe(0); + }, platform: "linux", }); const ctrlZ = createTestKey({ ctrl: true, name: "z" }); renderer.emitKeypress(ctrlZ); + expect(ctrlZ.defaultPrevented).toBe(true); expect(ctrlZ.propagationStopped).toBe(true); - expect(renderer.suspendCalls).toBe(1); - expect(signals.listenerCount("SIGCONT")).toBe(1); expect(sentSignals).toEqual([{ pid: 0, signal: "SIGTSTP" }]); - - signals.emit("SIGCONT"); expect(renderer.resumeCalls).toBe(1); - expect(signals.listenerCount("SIGCONT")).toBe(0); }); - test("does not resume a destroyed renderer after SIGCONT", () => { + test("does not resume a destroyed renderer", () => { const renderer = createMockRenderer(); - const signals = createSignalHarness(); installJobControlSuspendSupport(renderer, { - kill: () => undefined, - off: signals.off, - once: signals.once, + kill: () => { + renderer.isDestroyed = true; + }, platform: "linux", }); renderer.emitKeypress(createTestKey({ ctrl: true, name: "z" })); - renderer.isDestroyed = true; - signals.emit("SIGCONT"); expect(renderer.suspendCalls).toBe(1); expect(renderer.resumeCalls).toBe(0); @@ -231,31 +178,24 @@ describe("installJobControlSuspendSupport", () => { test("restores the renderer if SIGTSTP cannot be sent", () => { const renderer = createMockRenderer(); - const signals = createSignalHarness(); installJobControlSuspendSupport(renderer, { kill: () => { throw new Error("unsupported signal"); }, - off: signals.off, - once: signals.once, platform: "linux", }); renderer.emitKeypress(createTestKey({ ctrl: true, name: "z" })); expect(renderer.suspendCalls).toBe(1); expect(renderer.resumeCalls).toBe(1); - expect(signals.listenerCount("SIGCONT")).toBe(0); }); - test("dispose removes the keypress listener and pending SIGCONT listener", () => { + test("dispose removes the keypress listener", () => { const renderer = createMockRenderer(); - const signals = createSignalHarness(); const support = installJobControlSuspendSupport(renderer, { kill: () => undefined, - off: signals.off, - once: signals.once, platform: "linux", }); @@ -263,7 +203,6 @@ describe("installJobControlSuspendSupport", () => { support.dispose(); expect(renderer.keypressListeners.size).toBe(0); - expect(signals.listenerCount("SIGCONT")).toBe(0); renderer.emitKeypress(createTestKey({ ctrl: true, name: "z" })); expect(renderer.suspendCalls).toBe(1); diff --git a/packages/hunk/src/core/process/jobControl.ts b/packages/hunk/src/core/process/jobControl.ts index e513c6463..11f412d50 100644 --- a/packages/hunk/src/core/process/jobControl.ts +++ b/packages/hunk/src/core/process/jobControl.ts @@ -1,6 +1,5 @@ import type { CliRenderer, KeyEvent } from "@opentui/core"; -type SignalListener = () => void; type KeypressListener = (key: KeyEvent) => void; type JobControlRenderer = Pick & { @@ -13,8 +12,6 @@ type JobControlRenderer = Pick unknown; - off?: (signal: NodeJS.Signals, listener: SignalListener) => unknown; - once?: (signal: NodeJS.Signals, listener: SignalListener) => unknown; platform?: NodeJS.Platform | string; /** Signal target passed to process.kill; defaults to 0 for the foreground process group. */ pid?: number; @@ -73,7 +70,16 @@ export function installJobControlInterruptSupport( * OpenTUI receives Ctrl-Z as a parsed keypress instead of letting the terminal driver turn it into * SIGTSTP. Match the common TUI pattern used by apps like opencode: treat Ctrl-Z as an app command, * ask OpenTUI to restore the terminal, then send SIGTSTP to the foreground process group so the - * shell can manage Hunk as a normal suspended job. SIGCONT resumes the renderer after `fg`. + * shell can manage Hunk as a normal suspended job. + * + * The stop takes effect before `kill` returns, because POSIX delivers a signal sent to the caller's + * own process group before the call completes. Suspend and resume are therefore one straight line: + * the statement after `kill` runs only once the shell continues the job with `fg`. Staying on that + * call stack is also what keeps the job alive, since OpenTUI's suspend drops its keep-alive timer + * and stops reading stdin, so a runtime that reached an idle event loop here could exit before + * being continued. A `kill` that returns without stopping — a runtime that refuses SIGTSTP, or an + * orphaned process group that discards it — reaches the same restore instead of waiting for a + * SIGCONT nobody will send. */ export function installJobControlSuspendSupport( renderer: JobControlRenderer, @@ -85,38 +91,21 @@ export function installJobControlSuspendSupport( } const kill = deps.kill ?? process.kill.bind(process); - const off = deps.off ?? process.off.bind(process); - const once = deps.once ?? process.once.bind(process); const pid = deps.pid ?? 0; let disposed = false; - let resumeOnContinue: SignalListener | null = null; - - const clearPendingContinue = () => { - if (resumeOnContinue) { - off("SIGCONT", resumeOnContinue); - resumeOnContinue = null; - } - }; const suspend = () => { - resumeOnContinue = () => { - resumeOnContinue = null; - if (!renderer.isDestroyed) { - renderer.resume(); - } - }; - renderer.suspend(); - once("SIGCONT", resumeOnContinue); try { + // Blocks until the shell continues this job; see the note above. kill(pid, "SIGTSTP"); } catch { - // If the platform/runtime refuses SIGTSTP, leave the app usable instead of half-suspended. - clearPendingContinue(); - if (!renderer.isDestroyed) { - renderer.resume(); - } + // A runtime that refuses SIGTSTP leaves the app usable instead of half-suspended. + } + + if (!renderer.isDestroyed) { + renderer.resume(); } }; @@ -135,7 +124,6 @@ export function installJobControlSuspendSupport( return { dispose: () => { disposed = true; - clearPendingContinue(); renderer.keyInput.off("keypress", keypressListener); }, }; diff --git a/test/pty/lifecycle.test.ts b/test/pty/lifecycle.test.ts index 3eefeff8d..187c5d7e0 100644 --- a/test/pty/lifecycle.test.ts +++ b/test/pty/lifecycle.test.ts @@ -210,6 +210,66 @@ function waitForStreamOutput(stream: NodeJS.ReadableStream, pattern: RegExp, tim } describe("PTY lifecycle", () => { + test.skipIf(process.platform === "win32")( + "resumes a suspended job after fg without losing app state", + async () => { + const fixture = harness.createTabbedFilePair(); + const hunkCommand = harness.buildHunkCommand([ + "diff", + "--files", + fixture.before, + fixture.after, + "--mode", + "stack", + ]); + const session = await harness.launchShellCommand({ + command: "exec /bin/bash --noprofile --norc -i", + cwd: fixture.dir, + }); + + try { + session.writeRaw("PS1='HUNK_SHELL> '\r"); + await session.waitForText(/HUNK_SHELL>/, { timeout: 5_000 }); + session.writeRaw(`${hunkCommand}\r`); + await session.waitForText(/before\.txt.*after\.txt/, { timeout: 15_000 }); + await Bun.sleep(1_000); + await session.press("c"); + await session.waitForText(/Draft note/, { timeout: 5_000 }); + await session.type("Keep this note after resume."); + await session.press(["ctrl", "s"]); + await session.waitForText(/Keep this note after resume\./, { timeout: 5_000 }); + + // OpenTUI parses Ctrl-Z in raw mode, so send its control byte instead of SIGTSTP. + session.writeRaw("\x1a"); + await session.waitForText(/\[\d+\][^\n]*(?:Stopped|suspended)/, { timeout: 5_000 }); + await Bun.sleep(5_000); + session.writeRaw("fg\r"); + await Bun.sleep(1_000); + expect(await session.text({ immediate: true })).not.toContain("HUNK_SHELL>"); + await harness.ensureKeyboardIsLive(session); + const resumed = await session.text({ immediate: true }); + expect(resumed).toMatch(/before\.txt.*after\.txt/); + expect(resumed).toContain("Keep this note after resume."); + + // A resumed job stays suspendable, so Ctrl-Z is not a one-shot escape hatch. An echoed + // shell command proves the stop really happened: the earlier job lines are still on the + // normal screen, so matching them again would pass even if Ctrl-Z did nothing. + session.writeRaw("\x1a"); + await session.waitForText(/\[\d+\][^\n]*(?:Stopped|suspended)/, { timeout: 5_000 }); + session.writeRaw("echo SECOND_SUSPEND_OK\r"); + await session.waitForText(/HUNK_SHELL> echo SECOND_SUSPEND_OK/, { timeout: 5_000 }); + + session.writeRaw("fg\r"); + await Bun.sleep(1_000); + const secondResume = await session.text({ immediate: true }); + expect(secondResume).not.toContain("SECOND_SUSPEND_OK"); + expect(secondResume).toContain("Keep this note after resume."); + } finally { + session.close(); + } + }, + ); + for (const signal of ["SIGHUP", "SIGQUIT", "SIGPIPE"] as const) { test.skipIf(process.platform === "win32")(`exits cleanly on ${signal}`, async () => { const fixture = harness.createLongWrapFilePair();