diff --git a/.changeset/suppress-signal-termination-crash-prompt.md b/.changeset/suppress-signal-termination-crash-prompt.md new file mode 100644 index 00000000..45afcbd8 --- /dev/null +++ b/.changeset/suppress-signal-termination-crash-prompt.md @@ -0,0 +1,5 @@ +--- +"@inkeep/open-knowledge": patch +--- + +The desktop app no longer invites you to file a bug report after it was asked to quit by a termination signal (a macOS logout, `killall`, Activity Monitor's "Quit", or a parent process stopping it). These signals are an orderly request to stop, not an app crash, but the main process previously exited without running its clean-quit path — so the dirty-shutdown sentinel was left behind and the next launch misread the session as a crash. The main process now handles SIGTERM/SIGINT/SIGHUP by clearing the sentinel and quitting cleanly. Genuine app crashes still prompt exactly as before, and a crash that produced a crash dump is still detected on the next boot. diff --git a/packages/desktop/src/main/index.ts b/packages/desktop/src/main/index.ts index dfebdb5a..f03a0e2b 100644 --- a/packages/desktop/src/main/index.ts +++ b/packages/desktop/src/main/index.ts @@ -316,6 +316,7 @@ import { startFirstRunHandshake } from './share-handoff.ts'; import { checkOutboundUrl, handleShellOpenExternal } from './shell-allowlist.ts'; import { applyHarvestedAuthSock, harvestShellAuthSock } from './shell-env.ts'; import { createShowGateRegistry, type ShowGateRegistry } from './show-gate.ts'; +import { installSignalCleanQuit } from './signal-clean-quit.ts'; import { reclaimProjectSkillsOnProjectOpen, reclaimUserSkillsOnLaunch } from './skill-reclaim.ts'; import { attachSpellcheckContextMenu } from './spellcheck-context-menu.ts'; import { popSpellcheckMenu } from './spellcheck-menu.ts'; @@ -5436,6 +5437,17 @@ function bootPrimaryInstance(): void { powerMonitor.on('shutdown', () => crashDetection?.noteOsShutdown()); powerMonitor.on('suspend', () => crashDetection?.noteSuspend()); powerMonitor.on('resume', () => crashDetection?.noteResume()); + // Clean-quit on catchable termination signals (SIGTERM/SIGINT/SIGHUP) so an + // orderly stop (logout, `killall`, Activity Monitor's "Quit") isn't misread + // as a crash next boot. Installed after crashDetection is wired so + // `markCleanQuit` is live. Full rationale + SIGKILL-race handling live in + // `signal-clean-quit.ts`. + installSignalCleanQuit({ + process, + markCleanQuit: () => crashDetection?.markCleanQuit(), + quit: () => app.quit(), + logger: getLogger('signal-clean-quit'), + }); app.on('child-process-gone', (_event, details) => { // Feed the server-exit recorder every Utility death (not just the crash // reasons the invitation pipeline acts on) so the bundle can distinguish a diff --git a/packages/desktop/src/main/signal-clean-quit.test.ts b/packages/desktop/src/main/signal-clean-quit.test.ts new file mode 100644 index 00000000..ceafb731 --- /dev/null +++ b/packages/desktop/src/main/signal-clean-quit.test.ts @@ -0,0 +1,106 @@ +/** + * Signal-clean-quit tests: a fake process emitter records the per-signal + * handlers the installer registers, and fake `markCleanQuit` / `quit` hooks + * record call order. No Electron, no real signals — the whole pipeline is + * exercised by emitting into the fake emitter. + */ + +import { describe, expect, test } from 'vitest'; +import { CLEAN_QUIT_SIGNALS, installSignalCleanQuit } from './signal-clean-quit.ts'; + +interface Rig { + emit(signal: NodeJS.Signals): void; + registered: NodeJS.Signals[]; + calls: string[]; + install(overrides?: { markCleanQuit?: () => void }): void; +} + +function makeRig(): Rig { + const handlers = new Map void>>(); + const registered: NodeJS.Signals[] = []; + const calls: string[] = []; + const rig: Rig = { + registered, + calls, + emit(signal) { + for (const handler of handlers.get(signal) ?? []) handler(); + }, + install(overrides) { + installSignalCleanQuit({ + process: { + on(signal, listener) { + registered.push(signal); + const list = handlers.get(signal) ?? []; + list.push(listener); + handlers.set(signal, list); + return this; + }, + }, + markCleanQuit: + overrides?.markCleanQuit ?? + (() => { + calls.push('markCleanQuit'); + }), + quit: () => { + calls.push('quit'); + }, + logger: { info: () => {} }, + }); + }, + }; + return rig; +} + +describe('installSignalCleanQuit', () => { + test('registers a handler for each of SIGTERM/SIGINT/SIGHUP by default', () => { + const rig = makeRig(); + rig.install(); + expect(rig.registered).toEqual([...CLEAN_QUIT_SIGNALS]); + expect([...CLEAN_QUIT_SIGNALS]).toEqual(['SIGTERM', 'SIGINT', 'SIGHUP']); + }); + + test('clears the sentinel before quitting so it is durable against a SIGKILL race', () => { + const rig = makeRig(); + rig.install(); + + rig.emit('SIGTERM'); + + // markCleanQuit MUST precede quit: app.quit() teardown is async and may be + // cut short by a SIGKILL escalation, so the synchronous sentinel clear has + // to have already happened. + expect(rig.calls).toEqual(['markCleanQuit', 'quit']); + }); + + test('each catchable signal drives a clean quit', () => { + for (const signal of CLEAN_QUIT_SIGNALS) { + const rig = makeRig(); + rig.install(); + rig.emit(signal); + expect(rig.calls).toEqual(['markCleanQuit', 'quit']); + } + }); + + test('fires once — a second signal during teardown is a no-op', () => { + const rig = makeRig(); + rig.install(); + + rig.emit('SIGTERM'); + rig.emit('SIGINT'); + rig.emit('SIGTERM'); + + expect(rig.calls).toEqual(['markCleanQuit', 'quit']); + }); + + test('a throwing markCleanQuit still lets the quit proceed', () => { + const rig = makeRig(); + rig.install({ + markCleanQuit: () => { + rig.calls.push('markCleanQuit'); + throw new Error('sentinel unwritable'); + }, + }); + + expect(() => rig.emit('SIGTERM')).not.toThrow(); + expect(rig.calls).toEqual(['markCleanQuit', 'quit']); + }); +}); diff --git a/packages/desktop/src/main/signal-clean-quit.ts b/packages/desktop/src/main/signal-clean-quit.ts new file mode 100644 index 00000000..66006b8b --- /dev/null +++ b/packages/desktop/src/main/signal-clean-quit.ts @@ -0,0 +1,95 @@ +/** + * Signal-driven clean-quit for the Electron main process. + * + * A `SIGTERM` / `SIGINT` / `SIGHUP` is an *orderly* request to stop — the OS + * asking apps to quit for a logout, a parent process, `killall`, or Activity + * Monitor's "Quit" (as opposed to "Force Quit", which is an uncatchable + * `SIGKILL`). Node's default disposition terminates the process WITHOUT running + * Electron's `before-quit` -> `will-quit` sequence, so the dirty-shutdown + * sentinel is never cleared and the NEXT boot misreports the session as a crash + * ("previous session ended without a clean quit"). That is the same + * "the environment ended the session, not the app" class the reboot / + * OS-shutdown / suspend suppression already covers — a termination signal is + * just the one that arrives in-band at teardown time rather than as an + * announced power marker. + * + * These handlers close that gap. On the first such signal we (1) clear the + * sentinel synchronously so the record is durable even if a `SIGKILL` + * escalation lands mid-teardown (the OS gives a signalled app a short grace + * before force-killing it), then (2) drive Electron's normal `app.quit()` so + * the full teardown (`will-quit` sentinel clear, PTY reap, logger flush, + * owned-server drain on the update path) still runs. The handler fires once — + * a second signal arriving while the quit sequence is already in flight is a + * no-op rather than a second `app.quit()`. + * + * A genuine main-process crash is unaffected: it never delivers a signal here, + * and the boot-time minidump scan remains the authoritative native-crash signal + * (clearing the sentinel does not touch minidumps, so a fresh dump still arms a + * dump-driven invitation on the next boot). + * + * This is deliberately NOT a `process.on('uncaughtException')` handler — see + * `process-safety-net.ts` for why one must never be added. A signal handler is + * a distinct disposition and does not change Electron's main-process + * crash-dialog semantics. It mirrors the established teardown-signal wiring in + * the sibling utility processes: `pty-host.ts` `installHostReaping` handles all + * three of these signals, and `server-entry.ts` handles SIGTERM and SIGINT. + * + * Electron-free by construction (the process emitter, clean-quit hook, quit + * driver, and logger are all injected) so it is unit-testable without a live + * app. + */ + +/** Catchable teardown signals that read as an orderly stop, not an app crash. */ +export const CLEAN_QUIT_SIGNALS: readonly NodeJS.Signals[] = ['SIGTERM', 'SIGINT', 'SIGHUP']; + +interface SignalCleanQuitLogger { + info(payload: Record, msg: string): void; +} + +/** Minimal surface this installer needs — `process` satisfies it; tests pass an emitter. */ +interface ProcessSignalEmitter { + on(signal: NodeJS.Signals, listener: () => void): unknown; +} + +export interface InstallSignalCleanQuitOpts { + /** The process to subscribe on — `process` in production, a fake emitter in tests. */ + process: ProcessSignalEmitter; + /** + * Clear the dirty-shutdown sentinel so the next boot doesn't read this + * session as a crash. Runs synchronously before `quit()` so it is durable + * against a `SIGKILL` escalation racing the async quit teardown. + */ + markCleanQuit: () => void; + /** Drive Electron's orderly quit (`before-quit` -> `will-quit` -> exit). */ + quit: () => void; + logger: SignalCleanQuitLogger; +} + +/** + * Register clean-quit handlers for the catchable teardown signals. Call once, + * during main-process boot, after crash detection is wired (so `markCleanQuit` + * is live). The first signal wins; later signals during teardown are no-ops. + */ +export function installSignalCleanQuit(opts: InstallSignalCleanQuitOpts): void { + let handled = false; + const handle = (signal: NodeJS.Signals): void => { + if (handled) return; + handled = true; + opts.logger.info( + { event: 'desktop.signal-clean-quit', signal }, + 'received termination signal — quitting cleanly', + ); + // Clear the sentinel first: durable even if the OS escalates to SIGKILL + // before `app.quit()`'s async teardown reaches `will-quit`. `markCleanQuit` + // is idempotent, so the `will-quit` handler calling it again is a no-op. + // A throw here must not skip `quit()` — the signal is a request to stop and + // we honor it regardless of sentinel-clear outcome. + try { + opts.markCleanQuit(); + } catch {} + opts.quit(); + }; + for (const signal of CLEAN_QUIT_SIGNALS) { + opts.process.on(signal, () => handle(signal)); + } +}