From 087c4bf46cb1c233c85da5c6a27e0a6fde98ffa2 Mon Sep 17 00:00:00 2001 From: Arno V Date: Tue, 4 Aug 2026 15:53:02 -0400 Subject: [PATCH 1/2] fix(timer): stop crashing when the timer expires on Apple Silicon The timer crashed with `spawn Unknown system error -86` every time it expired on an Apple Silicon Mac without Rosetta 2 installed. errno 86 is EBADARCH, "Bad CPU type in executable". Since notifications default to on, this made the tool unusable on those machines. node-notifier displays macOS notifications by spawning a vendored terminal-notifier binary that is x86_64-only, so it cannot run natively on arm64. The spawn failure was thrown synchronously through the unguarded notify() call inside the expiry callback, where nothing caught it, turning a cosmetic failure into an uncaught exception that killed the process. Notifications now use the notifier each OS already ships: osascript on macOS, notify-send on Linux, and a PowerShell toast on Windows. Nothing is vendored, so nothing is coupled to a CPU architecture. A notification that cannot be displayed resolves to false instead of throwing, so a failed notification can never take the timer down again. Consumer impact: - node-notifier is no longer a dependency. - `wait: true` is gone, so the process no longer stays alive until the notification is dismissed; it exits once the notification is delivered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PmVBQszqooNXgLX5x6VTth --- packages/timer/package.json | 3 +- packages/timer/src/__tests__/notify.test.ts | 152 ++++++++++++++++++++ packages/timer/src/notify.ts | 111 ++++++++++++++ packages/timer/src/utilities.ts | 18 ++- pnpm-lock.yaml | 47 ------ 5 files changed, 275 insertions(+), 56 deletions(-) create mode 100644 packages/timer/src/__tests__/notify.test.ts create mode 100644 packages/timer/src/notify.ts diff --git a/packages/timer/package.json b/packages/timer/package.json index b8ac8d4b..9e5bca19 100644 --- a/packages/timer/package.json +++ b/packages/timer/package.json @@ -37,8 +37,7 @@ "dependencies": { "@node-cli/logger": "workspace:*", "@node-cli/parser": "workspace:*", - "moment": "2.30.1", - "node-notifier": "10.0.1" + "moment": "2.30.1" }, "publishConfig": { "access": "public" diff --git a/packages/timer/src/__tests__/notify.test.ts b/packages/timer/src/__tests__/notify.test.ts new file mode 100644 index 00000000..f42ae191 --- /dev/null +++ b/packages/timer/src/__tests__/notify.test.ts @@ -0,0 +1,152 @@ +import { execFile } from "node:child_process"; +import { vi } from "vitest"; +import { buildNotifyCommand, notify } from "../notify.js"; + +vi.mock("node:child_process", () => ({ + execFile: vi.fn(), +})); + +const mockedExecFile = vi.mocked(execFile); + +/** + * Mimics a notifier that spawns successfully and exits cleanly. + */ +const spawnSucceeds = () => { + mockedExecFile.mockImplementation((( + _command: string, + _args: string[], + callback: (error: null, stdout: string, stderr: string) => void, + ) => { + callback(null, "", ""); + }) as unknown as typeof execFile); +}; + +/** + * Mimics `child_process.spawn` failing synchronously, which is how a notifier + * binary that cannot run on the host CPU surfaces: EBADARCH (errno -86, "Bad + * CPU type in executable") is thrown, not passed to the callback. + */ +const spawnThrowsBadArch = () => { + mockedExecFile.mockImplementation((() => { + const error: NodeJS.ErrnoException = new Error( + "spawn Unknown system error -86", + ); + error.errno = -86; + error.code = "Unknown system error -86"; + error.syscall = "spawn"; + throw error; + }) as unknown as typeof execFile); +}; + +/** + * Mimics a notifier that is simply not installed on the host. + */ +const spawnFailsNotFound = () => { + mockedExecFile.mockImplementation((( + _command: string, + _args: string[], + callback: (error: NodeJS.ErrnoException) => void, + ) => { + const error: NodeJS.ErrnoException = new Error("spawn ENOENT"); + error.code = "ENOENT"; + callback(error); + }) as unknown as typeof execFile); +}; + +const notification = { + message: "Time's up!", + sound: "Funk", + title: "Timer Notification", +}; + +describe("when building the platform-native notification command", () => { + it("should use osascript on macOS", async () => { + expect(buildNotifyCommand("darwin", notification)).toStrictEqual({ + command: "osascript", + args: [ + "-e", + 'display notification "Time\'s up!" with title "Timer Notification" sound name "Funk"', + ], + }); + }); + + it("should omit the sound on macOS when none is requested", async () => { + expect( + buildNotifyCommand("darwin", { message: "a", title: "b" }), + ).toStrictEqual({ + command: "osascript", + args: ["-e", 'display notification "a" with title "b"'], + }); + }); + + it("should escape quotes and backslashes in the AppleScript literal", async () => { + const { args } = buildNotifyCommand("darwin", { + message: 'say "hi" \\ bye', + title: 'a "quoted" title', + }); + expect(args[1]).toBe( + 'display notification "say \\"hi\\" \\\\ bye" with title "a \\"quoted\\" title"', + ); + }); + + it("should use notify-send on Linux", async () => { + expect(buildNotifyCommand("linux", notification)).toStrictEqual({ + command: "notify-send", + args: ["Timer Notification", "Time's up!"], + }); + }); + + it("should use a PowerShell toast on Windows", async () => { + const result = buildNotifyCommand("win32", notification); + expect(result.command).toBe("powershell"); + expect(result.args[0]).toBe("-NoProfile"); + expect(result.args[2]).toContain("ToastNotificationManager"); + expect(result.args[2]).toContain("Timer Notification"); + }); + + it("should double single quotes in the PowerShell literal", async () => { + const { args } = buildNotifyCommand("win32", { + message: "Time's up!", + title: "it's fine", + }); + expect(args[2]).toContain("'Time''s up!'"); + expect(args[2]).toContain("'it''s fine'"); + }); + + it("should return null on an unsupported platform", async () => { + expect(buildNotifyCommand("aix", notification)).toBeNull(); + }); +}); + +describe("when displaying a notification", () => { + beforeEach(() => { + mockedExecFile.mockReset(); + }); + + it("should resolve to true when the notifier runs", async () => { + spawnSucceeds(); + await expect(notify(notification, "darwin")).resolves.toBe(true); + expect(mockedExecFile).toHaveBeenCalledTimes(1); + }); + + /** + * Regression: node-notifier spawned a vendored x86_64-only terminal-notifier + * binary, so on an Apple Silicon host without Rosetta the spawn threw EBADARCH + * straight through the timer callback and crashed the process. A failed + * notification must degrade, not throw. + */ + it("should resolve to false when the notifier cannot run on this CPU", async () => { + spawnThrowsBadArch(); + await expect(notify(notification, "darwin")).resolves.toBe(false); + }); + + it("should resolve to false when the notifier is not installed", async () => { + spawnFailsNotFound(); + await expect(notify(notification, "linux")).resolves.toBe(false); + }); + + it("should resolve to false without spawning on an unsupported platform", async () => { + await expect(notify(notification, "aix")).resolves.toBe(false); + expect(mockedExecFile).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/timer/src/notify.ts b/packages/timer/src/notify.ts new file mode 100644 index 00000000..3c0b7045 --- /dev/null +++ b/packages/timer/src/notify.ts @@ -0,0 +1,111 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +const execFileAsync = promisify(execFile); + +export type Notification = { + message: string; + sound?: string; + title: string; +}; + +export type NotifyCommand = { + args: string[]; + command: string; +}; + +/** + * Escapes a value so it can be embedded in an AppleScript double-quoted string + * literal, where only backslashes and double quotes are special. + */ +const escapeAppleScript = (value: string): string => + value.replaceAll("\\", "\\\\").replaceAll('"', '\\"'); + +/** + * Escapes a value so it can be embedded in a PowerShell single-quoted string + * literal, where a single quote is escaped by doubling it. + */ +const escapePowerShell = (value: string): string => value.replaceAll("'", "''"); + +const macCommand = ({ message, sound, title }: Notification): NotifyCommand => { + const script = [ + `display notification "${escapeAppleScript(message)}"`, + `with title "${escapeAppleScript(title)}"`, + sound ? `sound name "${escapeAppleScript(sound)}"` : "", + ] + .filter(Boolean) + .join(" "); + + return { args: ["-e", script], command: "osascript" }; +}; + +const linuxCommand = ({ message, title }: Notification): NotifyCommand => ({ + args: [title, message], + command: "notify-send", +}); + +const windowsCommand = ({ message, title }: Notification): NotifyCommand => { + const script = [ + "[Windows.UI.Notifications.ToastNotificationManager, Windows.UI.Notifications, ContentType = WindowsRuntime] > $null", + "$xml = [Windows.UI.Notifications.ToastNotificationManager]::GetTemplateContent([Windows.UI.Notifications.ToastTemplateType]::ToastText02)", + `$xml.GetElementsByTagName('text')[0].AppendChild($xml.CreateTextNode('${escapePowerShell(title)}')) > $null`, + `$xml.GetElementsByTagName('text')[1].AppendChild($xml.CreateTextNode('${escapePowerShell(message)}')) > $null`, + "[Windows.UI.Notifications.ToastNotificationManager]::CreateToastNotifier('@node-cli/timer').Show([Windows.UI.Notifications.ToastNotification]::new($xml))", + ].join("; "); + + return { args: ["-NoProfile", "-Command", script], command: "powershell" }; +}; + +/** + * Builds the platform-native command used to display a desktop notification. + * Returns null on platforms we do not know how to notify on. + * + * Every supported platform ships its notifier as part of the OS, so nothing is + * vendored and nothing has to match the host CPU architecture. + * + */ +export const buildNotifyCommand = ( + platform: string, + notification: Notification, +): NotifyCommand | null => { + switch (platform) { + case "darwin": { + return macCommand(notification); + } + case "linux": { + return linuxCommand(notification); + } + case "win32": { + return windowsCommand(notification); + } + default: { + return null; + } + } +}; + +/** + * Displays a desktop notification, resolving to true when it was shown. + * + * A notification is a courtesy, never a reason to fail: an unsupported + * platform, a missing notifier, or a notifier that refuses to run all resolve + * to false instead of throwing. + * + */ +export const notify = async ( + notification: Notification, + platform: string = process.platform, +): Promise => { + const notifyCommand = buildNotifyCommand(platform, notification); + + if (notifyCommand === null) { + return false; + } + + try { + await execFileAsync(notifyCommand.command, notifyCommand.args); + return true; + } catch { + return false; + } +}; diff --git a/packages/timer/src/utilities.ts b/packages/timer/src/utilities.ts index ce1e6be8..d8521b83 100644 --- a/packages/timer/src/utilities.ts +++ b/packages/timer/src/utilities.ts @@ -1,7 +1,7 @@ import { Logger, Spinner } from "@node-cli/logger"; import moment, { Duration } from "moment"; -import notifier from "node-notifier"; +import { notify } from "./notify.js"; import { Configuration } from "./parse.js"; const logger = new Logger(); @@ -80,19 +80,23 @@ export class Timer { const timer = setInterval(this.printRemainingTime, 1000); // kill the spinner when the timer is done. - setTimeout(() => { + setTimeout(async () => { const message = "Time's up!"; + clearInterval(timer); + this.printRemainingTime(message); + spinner.stop(); + + /** + * notifying last, and never fatally: a desktop notification that cannot be + * displayed must not take the timer down with it. + */ if (this.config.flags.notification) { - notifier.notify({ + await notify({ message, sound: "Funk", title: "Timer Notification", - wait: true, }); } - clearInterval(timer); - this.printRemainingTime(message); - spinner.stop(); }, this.timerDurationMilliSeconds); } }; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3ca45b1f..1feccf4a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -354,9 +354,6 @@ importers: moment: specifier: 2.30.1 version: 2.30.1 - node-notifier: - specifier: 10.0.1 - version: 10.0.1 devDependencies: '@vitest/coverage-v8': specifier: 4.1.10 @@ -2688,9 +2685,6 @@ packages: graceful-fs@4.2.11: resolution: {integrity: sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ==} - growly@1.3.0: - resolution: {integrity: sha512-+xGQY0YyAWCnqy7Cd++hc2JqMYzlm0dG30Jd0beaA64sROr8C4nt8Yc9V5Ro3avlSUDTN0ulqP/VBKi1/lLygw==} - handlebars@4.7.9: resolution: {integrity: sha512-4E71E0rpOaQuJR2A3xDZ+GM1HyWYv1clR58tC8emQNeQe3RH7MAzSbat+V0wG78LQBo6m6bzSG/L4pBuCsgnUQ==} engines: {node: '>=0.4.7'} @@ -2868,11 +2862,6 @@ packages: is-arrayish@0.2.1: resolution: {integrity: sha512-zz06S8t0ozoDXMG+ube26zeCTNXcKIPJZJi8hBrF4idCLms4CG9QtK7qBl1boi5ODzFpjswb5JPmHCbMpjaYzg==} - is-docker@2.2.1: - resolution: {integrity: sha512-F+i2BKsFrH66iaUFc0woD8sLy8getkwTwtOBjvs56Cx4CgJDeKQeqfz8wAYiSb8JOprWhHH5p77PbmYCvvUuXQ==} - engines: {node: '>=8'} - hasBin: true - is-docker@3.0.0: resolution: {integrity: sha512-eljcgEDlEns/7AXFosB5K/2nCM4P7FQPkGc/DWLy5rmFEWvZayGrik1d9/QIY5nJ4f9YsVvBkA6kJpHn9rISdQ==} engines: {node: ^12.20.0 || ^14.13.1 || >=16.0.0} @@ -2937,10 +2926,6 @@ packages: resolution: {integrity: sha512-mE00Gnza5EEB3Ds0HfMyllZzbBrmLOX3vfWoj9A9PEnTfratQ/BcaJOuMhnkhjXvb2+FkY3VuHqtAGpTPmglFQ==} engines: {node: '>=18'} - is-wsl@2.2.0: - resolution: {integrity: sha512-fKzAra0rGJUUBwGBgNkHZuToZcn+TtXHpeCgmkMJMMYx1sQDYaCSyjJBSCa2nH1DGm7s3n1oBnohoVTBaN7Lww==} - engines: {node: '>=8'} - is-wsl@3.1.0: resolution: {integrity: sha512-UcVfVfaK4Sc4m7X3dUSoHoozQGBEFeDC+zVo06t98xe8CzHSZZBekNXH+tu0NalHolcJ/QAGqS46Hef7QXBIMw==} engines: {node: '>=16'} @@ -3375,9 +3360,6 @@ packages: engines: {node: ^20.17.0 || >=22.9.0} hasBin: true - node-notifier@10.0.1: - resolution: {integrity: sha512-YX7TSyDukOZ0g+gmzjB6abKu+hTGvO8+8+gIFDsRCU2t8fLV/P2unmt+LGFaIa4y64aX98Qksa97rgz4vMNeLQ==} - nopt@8.1.0: resolution: {integrity: sha512-ieGu42u/Qsa4TFktmaKEwM6MQH0pOWnaB3htzh0JRtx84+Mebc0cbZYN5bC+6WTZ4+77xrL9Pn5m7CV6VIkV7A==} engines: {node: ^18.17.0 || >=20.5.0} @@ -3955,9 +3937,6 @@ packages: resolution: {integrity: sha512-Iov+JwFv/2HcTpcwNMKd8+IWNb8tboQJNQTkAY/LLVK7gGH9jy+LGkVqPxfekHl+yMmiqXszdGWXgkfml7hjqA==} engines: {node: '>= 0.4'} - shellwords@0.1.1: - resolution: {integrity: sha512-vFwSUfQvqybiICwZY5+DAWIPLKsWO31Q91JSKl3UYv+K5c2QRPzn0qzec6QPu1Qc9eHYItiP3NdJqNVqetYAww==} - siginfo@2.0.0: resolution: {integrity: sha512-ybx0WO1/8bSBLEWXZvEd7gMW3Sn3JFlW3TvX1nREbDLRNQNaeNN8WK0meBwPdAaOI7TtRRRJn/Es1zhrrCHu7g==} @@ -4298,11 +4277,6 @@ packages: resolution: {integrity: sha512-6ZxzVpzDXDa3bJWaHilVayA+BH/1zmxCJoVgvmqJnid/gPoKHxUrS/aC/T6LGQtNHT+XHG9fXPJB4d+IrU30Ew==} hasBin: true - uuid@8.3.2: - resolution: {integrity: sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==} - deprecated: uuid@10 and below is no longer supported. For ESM codebases, update to uuid@latest. For CommonJS codebases, use uuid@11 (but be aware this version will likely be deprecated in 2028). - hasBin: true - validate-npm-package-license@3.0.4: resolution: {integrity: sha512-DpKm2Ui/xN7/HQKCtpZxoRWBhZ9Z0kqtygG8XCgNQ8ZlDnxuQmWhj566j8fN4Cu3/JmbhsDo7fcAJq4s9h27Ew==} @@ -6862,8 +6836,6 @@ snapshots: graceful-fs@4.2.11: {} - growly@1.3.0: {} - handlebars@4.7.9: dependencies: minimist: 1.2.8 @@ -7048,8 +7020,6 @@ snapshots: is-arrayish@0.2.1: {} - is-docker@2.2.1: {} - is-docker@3.0.0: {} is-fullwidth-code-point@3.0.0: {} @@ -7087,10 +7057,6 @@ snapshots: is-unicode-supported@2.1.0: {} - is-wsl@2.2.0: - dependencies: - is-docker: 2.2.1 - is-wsl@3.1.0: dependencies: is-inside-container: 1.0.0 @@ -7574,15 +7540,6 @@ snapshots: undici: 6.27.0 which: 6.0.1 - node-notifier@10.0.1: - dependencies: - growly: 1.3.0 - is-wsl: 2.2.0 - semver: 7.8.5 - shellwords: 0.1.1 - uuid: 8.3.2 - which: 2.0.2 - nopt@8.1.0: dependencies: abbrev: 3.0.1 @@ -8311,8 +8268,6 @@ snapshots: shell-quote@1.9.0: {} - shellwords@0.1.1: {} - siginfo@2.0.0: {} signal-exit@3.0.7: {} @@ -8646,8 +8601,6 @@ snapshots: uuid@14.0.1: {} - uuid@8.3.2: {} - validate-npm-package-license@3.0.4: dependencies: spdx-correct: 3.2.0 From 5eb45f83f803801ac22ab859cac2e15808e4f791 Mon Sep 17 00:00:00 2001 From: Arno V Date: Tue, 4 Aug 2026 16:32:37 -0400 Subject: [PATCH 2/2] fix(timer): bound the notifier spawn and harden the no-crash guarantee Follow-up from pre-landing review. The notification path could still take the CLI down, just by hanging instead of crashing. - notify.ts now passes { timeout: 5000, killSignal: SIGKILL } to execFile. A notifier that spawns but never exits (notify-send on a dead D-Bus session, a stalled osascript) was the last thing holding the event loop open once the spinner stopped, so the CLI hung forever after printing "Time's up!". SIGKILL rather than the default SIGTERM, since a wedged child can ignore SIGTERM and hang anyway. - buildNotifyCommand() moved inside the try block. It ran before it, so a malformed notification made notify() reject with a TypeError instead of resolving false, contradicting the "never throws" contract documented on the function. - The expiry callback in utilities.ts wraps its await in try/catch. An async setTimeout callback has nothing to attach a rejection handler to, so an unhandled rejection there is fatal, which is the original crash shape one frame higher up. - The EBADARCH regression test was not testing EBADARCH. Node's real execFile defines promisify.custom and calls execFile outside a promise executor, which is what lets a spawn failure throw synchronously; a bare vi.fn() mock has no such property, so promisify wrapped the call in `new Promise` and quietly turned the synchronous throw into a rejection. The mock now mirrors Node's shape, and the test fails if the await is ever moved out of the try block. - Added guards for the spawn timeout and for malformed input, and switched the Timer class from `/* v8 ignore next 60 */` to the start/stop form used elsewhere in the package, since the line count went stale when the class grew. 15 tests pass, coverage 100%, full suite green across 12 projects. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PmVBQszqooNXgLX5x6VTth --- packages/timer/src/__tests__/notify.test.ts | 90 +++++++++++++++++---- packages/timer/src/notify.ts | 22 +++-- packages/timer/src/utilities.ts | 21 +++-- 3 files changed, 105 insertions(+), 28 deletions(-) diff --git a/packages/timer/src/__tests__/notify.test.ts b/packages/timer/src/__tests__/notify.test.ts index f42ae191..21f89cfb 100644 --- a/packages/timer/src/__tests__/notify.test.ts +++ b/packages/timer/src/__tests__/notify.test.ts @@ -1,23 +1,59 @@ import { execFile } from "node:child_process"; import { vi } from "vitest"; -import { buildNotifyCommand, notify } from "../notify.js"; +import { buildNotifyCommand, type Notification, notify } from "../notify.js"; -vi.mock("node:child_process", () => ({ - execFile: vi.fn(), -})); +/** + * Node's own execFile defines promisify.custom, and that implementation calls + * execFile outside a promise executor, which is what lets a spawn failure throw + * synchronously. A bare mock has no such property, so promisify would fall back + * to wrapping the call in `new Promise`, quietly turning every synchronous + * throw into a rejection and hiding the very failure mode these tests exist to + * pin down. + */ +vi.mock("node:child_process", () => { + const execFile = vi.fn(); + /** + * vi.mock factories are hoisted, so the symbol is resolved inline rather than + * through a top-level `promisify` import. + */ + execFile[Symbol.for("nodejs.util.promisify.custom")] = ( + ...args: unknown[] + ) => { + let resolve: (value: unknown) => void; + let reject: (reason: Error) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + /** + * deliberately outside the executor above, mirroring Node, so that a + * synchronous throw stays synchronous instead of becoming a rejection. + */ + execFile(...args, (error: Error | null) => + error ? reject(error) : resolve({ stderr: "", stdout: "" }), + ); + return promise; + }; + return { execFile }; +}); const mockedExecFile = vi.mocked(execFile); +type ExecFileCallback = (error: NodeJS.ErrnoException | null) => void; + +/** + * execFile is called with an optional options argument, so the callback is + * whatever lands last rather than a fixed position. + */ +const lastArgument = (args: unknown[]): ExecFileCallback => + args.at(-1) as ExecFileCallback; + /** * Mimics a notifier that spawns successfully and exits cleanly. */ const spawnSucceeds = () => { - mockedExecFile.mockImplementation((( - _command: string, - _args: string[], - callback: (error: null, stdout: string, stderr: string) => void, - ) => { - callback(null, "", ""); + mockedExecFile.mockImplementation(((...args: unknown[]) => { + lastArgument(args)(null); }) as unknown as typeof execFile); }; @@ -42,14 +78,10 @@ const spawnThrowsBadArch = () => { * Mimics a notifier that is simply not installed on the host. */ const spawnFailsNotFound = () => { - mockedExecFile.mockImplementation((( - _command: string, - _args: string[], - callback: (error: NodeJS.ErrnoException) => void, - ) => { + mockedExecFile.mockImplementation(((...args: unknown[]) => { const error: NodeJS.ErrnoException = new Error("spawn ENOENT"); error.code = "ENOENT"; - callback(error); + lastArgument(args)(error); }) as unknown as typeof execFile); }; @@ -145,6 +177,32 @@ describe("when displaying a notification", () => { await expect(notify(notification, "linux")).resolves.toBe(false); }); + /** + * A notifier that spawns but never exits would otherwise keep the CLI alive + * indefinitely, since the pending child is the last thing holding the event + * loop open once the spinner has stopped. + */ + it("should bound the spawn so a hung notifier cannot keep the CLI alive", async () => { + spawnSucceeds(); + await notify(notification, "darwin"); + expect(mockedExecFile).toHaveBeenCalledWith( + "osascript", + expect.any(Array), + expect.objectContaining({ killSignal: "SIGKILL", timeout: 5000 }), + expect.any(Function), + ); + }); + + it("should resolve to false when a malformed notification is passed", async () => { + await expect( + notify( + { message: undefined, title: "t" } as unknown as Notification, + "darwin", + ), + ).resolves.toBe(false); + expect(mockedExecFile).not.toHaveBeenCalled(); + }); + it("should resolve to false without spawning on an unsupported platform", async () => { await expect(notify(notification, "aix")).resolves.toBe(false); expect(mockedExecFile).not.toHaveBeenCalled(); diff --git a/packages/timer/src/notify.ts b/packages/timer/src/notify.ts index 3c0b7045..53cc22eb 100644 --- a/packages/timer/src/notify.ts +++ b/packages/timer/src/notify.ts @@ -3,6 +3,13 @@ import { promisify } from "node:util"; const execFileAsync = promisify(execFile); +/** + * A notifier that spawns but never exits would keep the CLI alive forever, so + * it is killed outright rather than merely asked to stop: a wedged child can + * ignore SIGTERM. + */ +const NOTIFIER_TIMEOUT_MS = 5000; + export type Notification = { message: string; sound?: string; @@ -96,14 +103,17 @@ export const notify = async ( notification: Notification, platform: string = process.platform, ): Promise => { - const notifyCommand = buildNotifyCommand(platform, notification); + try { + const notifyCommand = buildNotifyCommand(platform, notification); - if (notifyCommand === null) { - return false; - } + if (notifyCommand === null) { + return false; + } - try { - await execFileAsync(notifyCommand.command, notifyCommand.args); + await execFileAsync(notifyCommand.command, notifyCommand.args, { + killSignal: "SIGKILL", + timeout: NOTIFIER_TIMEOUT_MS, + }); return true; } catch { return false; diff --git a/packages/timer/src/utilities.ts b/packages/timer/src/utilities.ts index d8521b83..486d305e 100644 --- a/packages/timer/src/utilities.ts +++ b/packages/timer/src/utilities.ts @@ -48,7 +48,7 @@ export const extractDuration = (config: Configuration): Duration => { } }; -/* v8 ignore next 60 */ +/* v8 ignore start */ export class Timer { config: Configuration; startTime: number; @@ -91,11 +91,19 @@ export class Timer { * displayed must not take the timer down with it. */ if (this.config.flags.notification) { - await notify({ - message, - sound: "Funk", - title: "Timer Notification", - }); + try { + await notify({ + message, + sound: "Funk", + title: "Timer Notification", + }); + } catch { + /** + * notify is already total, so this only ever catches a future regression + * in it. Rejecting here would be fatal: an async setTimeout callback has + * nothing to attach a handler to. + */ + } } }, this.timerDurationMilliSeconds); } @@ -113,3 +121,4 @@ export class Timer { } }; } +/* v8 ignore stop */