diff --git a/docs/Exit-Codes.md b/docs/Exit-Codes.md index 2fa147ffa..e4c134056 100644 --- a/docs/Exit-Codes.md +++ b/docs/Exit-Codes.md @@ -55,6 +55,7 @@ This document describes the exit codes used by the Ably CLI, particularly in int Exit codes are handled in: - `src/commands/interactive.ts`: Sets exit code 42 for user exit - `src/utils/sigint-exit.ts`: Handles SIGINT behavior and exit code 130 +- `src/utils/sigint-cancellation.ts`: Turns Ctrl+C into exit code 130 for commands that combine spinners and prompts (e.g. `accounts login`) --- diff --git a/docs/Project-Structure.md b/docs/Project-Structure.md index 8f4e72829..a0d1abfe2 100644 --- a/docs/Project-Structure.md +++ b/docs/Project-Structure.md @@ -89,6 +89,7 @@ This document outlines the directory structure of the Ably CLI project. │ ├── prompt-confirmation.ts # Y/N confirmation prompts │ ├── prompt-value.ts # Free-text and masked value prompts │ ├── readline-helper.ts # Readline utilities for interactive mode +│ ├── sigint-cancellation.ts # Ctrl+C cancellation for spinner + prompt flows (e.g. login) │ ├── sigint-exit.ts # SIGINT/Ctrl+C handling (exit code 130) │ ├── string-distance.ts # Levenshtein distance for fuzzy matching │ ├── terminal-diagnostics.ts # Terminal capability detection @@ -132,7 +133,7 @@ This document outlines the directory structure of the Ably CLI project. │ │ └── interactive-mode.test.ts # Interactive REPL integration tests │ ├── tty/ # TTY-dependent tests (node-pty, local only) │ │ ├── tty-test-helper.ts # PTY spawn/wait/write helpers -│ │ └── commands/ # Interactive SIGINT tests (prompt, running command, confirmation) +│ │ └── commands/ # Ctrl+C tests (interactive prompt, running command, confirmation, accounts login) │ ├── e2e/ # End-to-end tests against real Ably │ │ ├── auth/ # Auth E2E tests │ │ ├── bench/ # Benchmark E2E tests diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts index 901950bc6..d6e7dd5d1 100644 --- a/src/commands/accounts/login.ts +++ b/src/commands/accounts/login.ts @@ -1,4 +1,4 @@ -import { Flags } from "@oclif/core"; +import { Flags, Interfaces } from "@oclif/core"; import chalk from "chalk"; import * as readline from "node:readline"; import ora from "ora"; @@ -21,8 +21,15 @@ import openUrl from "../../utils/open-url.js"; import { formatResource } from "../../utils/output.js"; import { promptForConfirmation } from "../../utils/prompt-confirmation.js"; import { promptForValue } from "../../utils/prompt-value.js"; +import { + forwardSigintToProcess, + InterruptError, + withSigintCancellation, +} from "../../utils/sigint-cancellation.js"; import { pickUniqueAlias, slugifyAccountName } from "../../utils/slugify.js"; +type LoginFlags = Interfaces.InferredFlags; + /** Offered as the default when the guided local flow prompts for a URL. */ const DEFAULT_LOCAL_URL = "http://localhost:8081"; @@ -99,6 +106,22 @@ export default class AccountsLogin extends ControlBaseCommand { public async run(): Promise { const { flags } = await this.parse(AccountsLogin); + // Login strings together a spinner and several interactive prompts, and + // the libraries behind them leave Ctrl+C unhandled when combined. Own + // SIGINT for the whole flow and exit from here instead. + try { + await withSigintCancellation((signal) => this.login(flags, signal)); + } catch (error) { + if (error instanceof InterruptError) { + // Match the conventional exit code for SIGINT (128 + signal number). + this.exit(130); + } + + throw error; + } + } + + private async login(flags: LoginFlags, signal: AbortSignal): Promise { // Display ASCII art logo if not in JSON mode and the caller hasn't // opted out (e.g. `ably init` already prints the logo and delegates here). if (!this.shouldOutputJson(flags) && !flags["skip-logo"]) { @@ -114,8 +137,11 @@ export default class AccountsLogin extends ControlBaseCommand { let oauthTokens: OAuthTokens; try { - oauthTokens = await this.oauthLogin(flags); + oauthTokens = await this.oauthLogin(flags, signal); } catch (error) { + // A cancelled login has already been reported by the spinner; `run` + // handles the exit. + if (signal.aborted) throw error; this.fail(error, flags, "accountLogin"); } @@ -286,6 +312,7 @@ export default class AccountsLogin extends ControlBaseCommand { } } } catch (error) { + if (error instanceof InterruptError) throw error; this.logWarning(`Could not fetch apps: ${errorMessage(error)}`, flags); } @@ -413,6 +440,8 @@ export default class AccountsLogin extends ControlBaseCommand { } } } catch (error) { + // Ctrl+C at a confirmation prompt; `run` handles the exit. + if (error instanceof InterruptError) throw error; this.fail(error, flags, "accountLogin"); } } @@ -719,7 +748,10 @@ export default class AccountsLogin extends ControlBaseCommand { } } - private async oauthLogin(flags: BaseFlags): Promise { + private async oauthLogin( + flags: BaseFlags, + signal: AbortSignal, + ): Promise { const oauthClient = new OAuthClient({ oauthHost: flags["oauth-host"], }); @@ -754,38 +786,40 @@ export default class AccountsLogin extends ControlBaseCommand { this.log("Open the URL above in your browser to authorize."); } + // ora discards stdin by default, which puts the terminal in raw mode so + // Ctrl+C arrives as a byte rather than a signal. Its stdin-discarder then + // never reads that byte (it attaches with prependListener, which doesn't + // start a fresh stdin flowing), so Ctrl+C would be silently swallowed. const spinner = this.shouldOutputJson(flags) ? undefined - : ora("Waiting for authorization...").start(); - - // Wire up SIGINT so Ctrl+C during polling aborts the fetch and exits - // cleanly. Without this, the unsettled top-level await in bin/run.js - // triggers a confusing "Detected unsettled top-level await" warning - // from Node when the user hits Ctrl+C. - const abortController = new AbortController(); - const onSigint = () => { - abortController.abort(); - spinner?.fail("Authentication cancelled."); - // Match the conventional exit code for SIGINT (128 + signal number). - this.exit(130); - }; - process.once("SIGINT", onSigint); + : ora({ + discardStdin: false, + text: "Waiting for authorization...", + }).start(); + + // Abort listeners run synchronously on Ctrl+C, so the spinner is stopped + // before the interruption unwinds the command. + const onAbort = () => spinner?.fail("Authentication cancelled."); + signal.addEventListener("abort", onAbort, { once: true }); try { const tokens = await oauthClient.pollForToken( deviceResponse.deviceCode, deviceResponse.interval, deviceResponse.expiresIn, - abortController.signal, + signal, ); spinner?.succeed("Authentication successful."); return tokens; } catch (error) { - spinner?.fail("Authentication failed."); + if (!signal.aborted) { + spinner?.fail("Authentication failed."); + } + throw error; } finally { - process.removeListener("SIGINT", onSigint); + signal.removeEventListener("abort", onAbort); } } @@ -821,6 +855,8 @@ export default class AccountsLogin extends ControlBaseCommand { output: process.stdout, }); + forwardSigintToProcess(rl); + const logFn = this.log.bind(this); return new Promise((resolve) => { @@ -857,6 +893,8 @@ export default class AccountsLogin extends ControlBaseCommand { output: process.stdout, }); + forwardSigintToProcess(rl); + return new Promise((resolve) => { const askForAppName = () => { rl.question("Enter a name for your app: ", (appName) => { diff --git a/src/commands/status.ts b/src/commands/status.ts index 27479c637..e6e47522b 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -37,7 +37,10 @@ export default class StatusCommand extends AblyBaseCommand { const spinner = isInteractive || isJson ? null - : ora("Checking Ably service status...").start(); + : ora({ + discardStdin: false, + text: "Checking Ably service status...", + }).start(); if (isInteractive) { this.logProgress("Checking Ably service status", flags); } diff --git a/src/commands/support/ask.ts b/src/commands/support/ask.ts index 4f07ab574..128d408e6 100644 --- a/src/commands/support/ask.ts +++ b/src/commands/support/ask.ts @@ -38,7 +38,7 @@ export default class AskCommand extends ControlBaseCommand { const spinner = isInteractive || this.shouldOutputJson(flags) ? null - : ora("Thinking...").start(); + : ora({ discardStdin: false, text: "Thinking..." }).start(); if (isInteractive && !this.shouldOutputJson(flags)) { this.logProgress("Thinking", flags); } diff --git a/src/utils/prompt-confirmation.ts b/src/utils/prompt-confirmation.ts index b58e1dd77..3f34646bd 100644 --- a/src/utils/prompt-confirmation.ts +++ b/src/utils/prompt-confirmation.ts @@ -1,12 +1,6 @@ import * as readline from "node:readline"; -/** Rejection reason when the user presses Ctrl+C at a confirmation prompt. */ -export class InterruptError extends Error { - constructor() { - super("Interrupted by user"); - this.name = "InterruptError"; - } -} +import { InterruptError } from "./sigint-cancellation.js"; /** * Prompts the user for confirmation with a yes/no question. diff --git a/src/utils/sigint-cancellation.ts b/src/utils/sigint-cancellation.ts new file mode 100644 index 000000000..ec5db805b --- /dev/null +++ b/src/utils/sigint-cancellation.ts @@ -0,0 +1,84 @@ +// Ctrl+C handling for commands that mix spinners and interactive prompts. +// +// Two behaviours combine to make Ctrl+C unreliable without this: +// +// - Prompts such as legacy inquirer handle Ctrl+C by closing their readline +// and re-raising SIGINT with `process.kill`. Once any JS SIGINT listener +// exists (ora installs one via signal-exit), that signal is only delivered +// on a later event-loop turn. Signal handles don't keep the loop alive, so +// with the prompt closed Node can run out of work first and exit with +// "Detected unsettled top-level await" (exit code 13). +// - A listener cannot exit an oclif command by throwing: `this.exit()` throws +// an ExitError, and from inside an event listener that surfaces as an +// uncaught exception rather than a clean exit. +// +// `withSigintCancellation` owns SIGINT for the duration of a task, keeps the +// event loop alive so a re-raised signal is always delivered, and turns Ctrl+C +// into a rejection the command can handle on its normal async path. + +import type * as readline from "node:readline"; + +/** + * Raised when the user presses Ctrl+C, either while `withSigintCancellation` + * owns SIGINT or at a `promptForConfirmation` prompt. + */ +export class InterruptError extends Error { + constructor() { + super("Interrupted by user"); + this.name = "InterruptError"; + } +} + +/** + * Run `task`, rejecting with an `InterruptError` when the user presses Ctrl+C. + * + * The task receives an `AbortSignal` that is aborted on Ctrl+C, so in-flight + * work (fetches, polling loops, spinners) can stop and clean up. Abort + * listeners run synchronously, before the rejection reaches the caller. + * + * The task's own promise is raced rather than awaited, because some prompts + * (legacy inquirer) never settle once force-closed. + */ +export async function withSigintCancellation( + task: (signal: AbortSignal) => Promise, +): Promise { + const controller = new AbortController(); + const interrupted = new Promise((_, reject) => { + controller.signal.addEventListener( + "abort", + () => reject(new InterruptError()), + { once: true }, + ); + }); + const onSigint = () => controller.abort(); + + process.on("SIGINT", onSigint); + // Keep the event loop alive until the task settles, so a SIGINT re-raised + // by a prompt that has already closed its input is still delivered. Prompts + // only see Ctrl+C as a keypress on a TTY; elsewhere this would turn a prompt + // stuck on closed stdin into a hang. + const keepAlive = process.stdin.isTTY + ? setInterval(() => {}, 1 << 30) + : undefined; + try { + return await Promise.race([task(controller.signal), interrupted]); + } finally { + clearInterval(keepAlive); + process.removeListener("SIGINT", onSigint); + } +} + +/** + * Re-raise Ctrl+C at a readline prompt as a process SIGINT. + * + * Without a SIGINT listener, readline swallows Ctrl+C and only pauses its + * input, leaving the prompt's promise pending forever. Closing the prompt and + * re-raising lets whoever owns SIGINT (`withSigintCancellation`, or Node's + * default handler) end the command. + */ +export function forwardSigintToProcess(rl: readline.Interface): void { + rl.on("SIGINT", () => { + rl.close(); + process.kill(process.pid, "SIGINT"); + }); +} diff --git a/test/tty/commands/accounts-login-sigint.test.ts b/test/tty/commands/accounts-login-sigint.test.ts new file mode 100644 index 000000000..08374f10d --- /dev/null +++ b/test/tty/commands/accounts-login-sigint.test.ts @@ -0,0 +1,206 @@ +/** + * TTY-dependent Ctrl+C tests for `ably accounts login`. + * + * Both failures only reproduce in a real terminal, so they can't be covered by + * unit tests: + * - ora's stdin-discarder puts the TTY in raw mode and then never reads the + * Ctrl+C byte, so Ctrl+C during "Waiting for authorization..." was ignored. + * - After that spinner has run, inquirer's re-raised SIGINT at a select + * prompt was never delivered: the prompt had closed its input, so Node + * ran out of work first and reported "Detected unsettled top-level await". + * + * A local HTTP server stands in for both the OAuth device flow and the + * Control API so the command runs end to end. + * + * Run locally with: pnpm test:tty + * NOT run in CI — GitHub Actions runners have no TTY. + */ +import { describe, it, expect, afterEach, beforeEach } from "vitest"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { + spawnTty, + waitForOutput, + sendCtrlC, + killTty, + type TtyProcess, +} from "../tty-test-helper.js"; + +interface FakeServer { + host: string; + close: () => Promise; +} + +/** + * Serve the OAuth device flow and the Control API endpoints login calls. + * `grantToken` decides whether polling succeeds straight away or keeps + * returning `authorization_pending`; with `noApps`, login offers to create + * one instead of prompting for a key. + */ +async function startFakeServer( + grantToken: boolean, + { noApps = false } = {}, +): Promise { + const routes: Record [number, unknown]> = { + "/oauth/authorize_device": () => [ + 200, + { + device_code: "device-code", + expires_in: 600, + interval: 1, + user_code: "ABCD-EFGH", + verification_uri: "http://localhost/oauth/device", + verification_uri_complete: + "http://localhost/oauth/device?user_code=ABCD-EFGH", + }, + ], + "/oauth/token": () => + grantToken + ? [200, { access_token: "access", refresh_token: "refresh" }] + : [400, { error: "authorization_pending" }], + "/api/v1/me": () => [ + 200, + { + account: { id: "acc-1", name: "Test Account" }, + user: { email: "user@example.com" }, + }, + ], + "/api/v1/me/accounts": () => [200, [{ id: "acc-1", name: "Test Account" }]], + "/api/v1/accounts/acc-1/apps": () => [ + 200, + noApps ? [] : [{ accountId: "acc-1", id: "app-1", name: "Test App" }], + ], + // Two keys, so login has to prompt for one. + "/api/v1/apps/app-1/keys": () => [ + 200, + ["key-1", "key-2"].map((id) => ({ + appId: "app-1", + capability: {}, + created: 0, + id, + key: `app-1.${id}:secret`, + modified: 0, + name: `Key ${id}`, + revocable: true, + status: 0, + })), + ], + }; + + const server = http.createServer((req, res) => { + const route = routes[new URL(req.url ?? "/", "http://x").pathname]; + const [status, body] = route ? route() : [404, { error: "not_found" }]; + res.writeHead(status, { "content-type": "application/json" }); + res.end(JSON.stringify(body)); + }); + + await new Promise((resolve) => server.listen(0, "localhost", resolve)); + const { port } = server.address() as AddressInfo; + + return { + host: `localhost:${port}`, + close: () => + new Promise((resolve) => { + server.closeAllConnections(); + server.close(() => resolve()); + }), + }; +} + +describe("accounts login - Ctrl+C handling (TTY)", () => { + const timeout = 20000; + let proc: TtyProcess | null = null; + let server: FakeServer | null = null; + let configDir: string; + + beforeEach(() => { + configDir = fs.mkdtempSync(path.join(os.tmpdir(), "ably-login-tty-")); + }); + + afterEach(async () => { + if (proc) { + await killTty(proc); + proc = null; + } + await server?.close(); + server = null; + fs.rmSync(configDir, { force: true, recursive: true }); + }); + + function spawnLogin(host: string): TtyProcess { + return spawnTty( + [ + "accounts", + "login", + "--no-browser", + "--skip-logo", + "--oauth-host", + host, + "--control-host", + host, + ], + { + ABLY_ACCESS_TOKEN: undefined, + ABLY_API_KEY: undefined, + ABLY_CLI_CONFIG_DIR: configDir, + // Exercise the real exit path rather than test mode's throwing one. + ABLY_CLI_TEST_MODE: undefined, + ABLY_INTERACTIVE_MODE: "false", + ABLY_TOKEN: undefined, + }, + ); + } + + it( + "exits with 130 when Ctrl+C is pressed while waiting for authorization", + async () => { + server = await startFakeServer(false); + proc = spawnLogin(server.host); + + await waitForOutput(proc, "Waiting for authorization"); + sendCtrlC(proc); + + const { code } = await proc.exitPromise; + expect(code).toBe(130); + expect(proc.output).toContain("Authentication cancelled."); + expect(proc.output).not.toContain("ExitError"); + }, + timeout, + ); + + it( + "exits with 130 when Ctrl+C is pressed at a prompt after authorization", + async () => { + server = await startFakeServer(true); + proc = spawnLogin(server.host); + + await waitForOutput(proc, "Select a key"); + sendCtrlC(proc); + + const { code } = await proc.exitPromise; + expect(code).toBe(130); + expect(proc.output).not.toContain("unsettled top-level await"); + }, + timeout, + ); + + it( + "exits with 130 when Ctrl+C is pressed at a confirmation after authorization", + async () => { + server = await startFakeServer(true, { noApps: true }); + proc = spawnLogin(server.host); + + await waitForOutput(proc, "create your first app"); + sendCtrlC(proc); + + const { code } = await proc.exitPromise; + expect(code).toBe(130); + expect(proc.output).not.toContain("Could not fetch apps"); + }, + timeout, + ); +}); diff --git a/test/tty/tty-test-helper.ts b/test/tty/tty-test-helper.ts index af9ef3960..13d1540da 100644 --- a/test/tty/tty-test-helper.ts +++ b/test/tty/tty-test-helper.ts @@ -43,10 +43,13 @@ export function spawnTty( args: string[] = ["interactive"], env: Record = {}, ): TtyProcess { - const childEnv = { - ...DEFAULT_ENV, - ...env, - }; + // node-pty stringifies values, so drop undefined entries to let callers + // unset inherited variables rather than set them to "undefined". + const childEnv = Object.fromEntries( + Object.entries({ ...DEFAULT_ENV, ...env }).filter( + (entry): entry is [string, string] => entry[1] !== undefined, + ), + ); const pty = nodePty.spawn("node", [BIN_PATH, ...args], { name: "xterm-256color", diff --git a/test/unit/commands/accounts/login.test.ts b/test/unit/commands/accounts/login.test.ts index cf19e4f47..75428b86e 100644 --- a/test/unit/commands/accounts/login.test.ts +++ b/test/unit/commands/accounts/login.test.ts @@ -522,6 +522,38 @@ describe("accounts:login command", () => { expect(authEvent).toHaveProperty("userCode", "TEST-CODE"); expect(authEvent).toHaveProperty("verificationUri"); }); + + it("exits with 130 and no error when interrupted while polling", async () => { + nock("https://ably.com").post("/oauth/authorize_device").reply(200, { + device_code: "dc_test", + expires_in: 300, + interval: 0.01, + user_code: "TEST-CODE", + verification_uri: "https://ably.com/device", + verification_uri_complete: + "https://ably.com/device?user_code=TEST-CODE", + }); + // Interrupt from inside the first poll, so the SIGINT can't arrive + // before the command is listening for it. + nock("https://ably.com") + .post("/oauth/token") + .reply(() => { + setImmediate(() => process.emit("SIGINT")); + return [400, { error: "authorization_pending" }]; + }) + .persist(); + + const { error, stdout } = await runCommand( + ["accounts:login", "--json"], + import.meta.url, + ); + + expect(error?.oclif?.exit).toBe(130); + const errorEvents = parseNdjsonLines(stdout).filter( + (e) => e.type === "error", + ); + expect(errorEvents).toEqual([]); + }); }); describe("local server login", () => { diff --git a/test/unit/utils/prompt-confirmation.test.ts b/test/unit/utils/prompt-confirmation.test.ts index f6c166426..083c57569 100644 --- a/test/unit/utils/prompt-confirmation.test.ts +++ b/test/unit/utils/prompt-confirmation.test.ts @@ -20,10 +20,8 @@ vi.mock("node:readline", () => ({ }, })); -import { - InterruptError, - promptForConfirmation, -} from "../../../src/utils/prompt-confirmation.js"; +import { promptForConfirmation } from "../../../src/utils/prompt-confirmation.js"; +import { InterruptError } from "../../../src/utils/sigint-cancellation.js"; describe("promptForConfirmation", () => { beforeEach(() => { diff --git a/test/unit/utils/sigint-cancellation.test.ts b/test/unit/utils/sigint-cancellation.test.ts new file mode 100644 index 000000000..83e3c8076 --- /dev/null +++ b/test/unit/utils/sigint-cancellation.test.ts @@ -0,0 +1,70 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; +import type * as readline from "node:readline"; + +import { + forwardSigintToProcess, + InterruptError, + withSigintCancellation, +} from "../../../src/utils/sigint-cancellation.js"; + +describe("withSigintCancellation", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("resolves with the task result and removes its SIGINT listener", async () => { + const before = process.listenerCount("SIGINT"); + + const result = await withSigintCancellation(async () => "done"); + + expect(result).toBe("done"); + expect(process.listenerCount("SIGINT")).toBe(before); + }); + + it("propagates task errors", async () => { + await expect( + withSigintCancellation(async () => { + throw new Error("boom"); + }), + ).rejects.toThrow("boom"); + }); + + it("aborts the signal and rejects with InterruptError on SIGINT", async () => { + const before = process.listenerCount("SIGINT"); + let taskSignal: AbortSignal | undefined; + + const pending = withSigintCancellation((signal) => { + taskSignal = signal; + // A task that never settles, like a force-closed inquirer prompt. + return new Promise(() => {}); + }); + + process.emit("SIGINT"); + + await expect(pending).rejects.toBeInstanceOf(InterruptError); + expect(taskSignal?.aborted).toBe(true); + expect(process.listenerCount("SIGINT")).toBe(before); + }); +}); + +describe("forwardSigintToProcess", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("closes the prompt and re-raises SIGINT on the process", () => { + const kill = vi.spyOn(process, "kill").mockImplementation(() => true); + const close = vi.fn(); + const handlers = new Map void>(); + const rl = { + close, + on: (event: string, handler: () => void) => handlers.set(event, handler), + } as unknown as readline.Interface; + + forwardSigintToProcess(rl); + handlers.get("SIGINT")?.(); + + expect(close).toHaveBeenCalledOnce(); + expect(kill).toHaveBeenCalledWith(process.pid, "SIGINT"); + }); +});