From 1953bed4131f0f1fd532e4291d7d4d0a47a7dfd7 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Wed, 5 Aug 2026 23:34:22 -0500 Subject: [PATCH] fix(installer): stop spinner/prompt race during workos install (AUTH-6732) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During npx workos install, a spinner's 80ms redraw interval could keep running while an interactive prompt was waiting for input, scribbling over the question so users never realized they were being asked (reported on a Rails run). Root cause — not clack (already removed in #200, which switched prompts to @inquirer/prompts and made withPrompt pause the *registered* active spinner). The remaining race was spinner lifecycle in the CLI adapter: 1. The agent spinner (started on agent:start) was only stopped by validation:start — which Ruby never emits and JS skips under --no-validate — so it kept animating through the whole post-install phase. 2. Handlers like handleCommitGenerating overwrote this.spinner with a fresh handle without stopping the old one. After the commit prompt resumed the agent spinner, its interval was orphaned: still firing every 80ms but no longer registered, so withPrompt could not pause it and it redrew over the next prompt (e.g. the PR prompt). Fix (minimal — no prompt-library swap needed; the race was lifecycle, not the prompt engine): - ui.spinner(): single-spinner invariant. start() retires the currently active spinner (halt interval + deregister; skips the line erase when the old spinner is paused mid-prompt so it can't wipe the question), and every write is guarded by an active flag so a stale handle goes inert instead of scribbling over its successor. - CLIAdapter: agent:success now finalizes the agent spinner ('Agent completed') so it never outlives the agent phase into the commit/PR prompts; failure paths were already finalized by handleError/handleComplete. - CLIAdapter: all spinner creation goes through startSpinner(), which clears the previous handle first, keeping this.spinner honest. Regression tests: ui.spec.ts covers retire-on-start, inert stale handles, prompt pause/resume, and no-resurrect-after-retire; cli-adapter.spec.ts covers agent:success stopping the spinner and phase-spinner replacement clearing the previous handle. The five lifecycle tests fail against the old code and pass with the fix. Refs AUTH-6732 --- src/lib/adapters/cli-adapter.spec.ts | 64 +++++++++++++++ src/lib/adapters/cli-adapter.ts | 54 ++++++++----- src/utils/ui.spec.ts | 111 +++++++++++++++++++++++++++ src/utils/ui.ts | 63 +++++++++++---- 4 files changed, 260 insertions(+), 32 deletions(-) diff --git a/src/lib/adapters/cli-adapter.spec.ts b/src/lib/adapters/cli-adapter.spec.ts index d8e2438e..4ec0a36d 100644 --- a/src/lib/adapters/cli-adapter.spec.ts +++ b/src/lib/adapters/cli-adapter.spec.ts @@ -364,6 +364,70 @@ describe('CLIAdapter', () => { }); }); + describe('spinner lifecycle (AUTH-6732)', () => { + type SpinnerMock = { + start: ReturnType; + stop: ReturnType; + message: ReturnType; + clear: ReturnType; + }; + const makeSpinnerMock = (): SpinnerMock => ({ + start: vi.fn(), + stop: vi.fn(), + message: vi.fn(), + clear: vi.fn(), + }); + + it('stops the agent spinner on agent:success so it cannot outlive the agent phase', async () => { + await adapter.start(); + const ui = await import('../../utils/ui.js'); + const spinnerMock = makeSpinnerMock(); + vi.mocked(ui.default.spinner).mockImplementation(() => spinnerMock as never); + + emitter.emit('agent:start', {}); + emitter.emit('agent:success', { summary: 'done' }); + + expect(spinnerMock.stop).toHaveBeenCalledWith('Agent completed', 0); + }); + + it('starting a new phase spinner clears the previous handle instead of orphaning it', async () => { + await adapter.start(); + const ui = await import('../../utils/ui.js'); + // A fresh handle per ui.spinner() call, so each phase gets its own. + vi.mocked(ui.default.spinner).mockImplementation(() => makeSpinnerMock() as never); + + emitter.emit('agent:start', {}); + emitter.emit('postinstall:commit:generating', {}); + + const handles = vi.mocked(ui.default.spinner).mock.results.map((r) => r.value as SpinnerMock); + expect(handles).toHaveLength(2); + // The agent spinner was cleared (not just abandoned with a live interval) + // before the commit spinner started. + expect(handles[0].clear).toHaveBeenCalled(); + expect(handles[0].start).not.toHaveBeenCalledWith('Generating commit message...'); + expect(handles[1].start).toHaveBeenCalledWith('Generating commit message...'); + }); + + it('a post-install prompt after agent:success has no stale spinner to resurrect', async () => { + await adapter.start(); + const ui = await import('../../utils/ui.js'); + const spinnerMock = makeSpinnerMock(); + vi.mocked(ui.default.spinner).mockImplementation(() => spinnerMock as never); + vi.mocked(ui.default.confirm).mockResolvedValue(false); + + emitter.emit('agent:start', {}); + emitter.emit('agent:success', { summary: 'done' }); + emitter.emit('postinstall:commit:prompt', {}); + await new Promise((r) => setTimeout(r, 10)); + + // Exactly one stop (agent:success); the prompt did not restart or re-stop + // a stale agent spinner, and no new spinner was created for the prompt. + expect(spinnerMock.stop).toHaveBeenCalledTimes(1); + expect(spinnerMock.start).toHaveBeenCalledTimes(1); + expect(sendEvent).toHaveBeenCalledWith({ type: 'COMMIT_DECLINED' }); + }); + }); + describe('staging success copy', () => { it('device path announces a fresh environment without "retrieved"', async () => { await adapter.start(); diff --git a/src/lib/adapters/cli-adapter.ts b/src/lib/adapters/cli-adapter.ts index b61df430..6e8819a8 100644 --- a/src/lib/adapters/cli-adapter.ts +++ b/src/lib/adapters/cli-adapter.ts @@ -142,6 +142,7 @@ export class CLIAdapter implements InstallerAdapter { this.subscribe('config:complete', this.handleConfigComplete); this.subscribe('agent:start', this.handleAgentStart); this.subscribe('agent:progress', this.handleAgentProgress); + this.subscribe('agent:success', this.handleAgentSuccess); // Persistent, append-only log of file operations + tool calls above the spinner. this.subscribe('file:write', this.handleFileWrite); this.subscribe('file:edit', this.handleFileEdit); @@ -211,6 +212,20 @@ export class CLIAdapter implements InstallerAdapter { } } + /** + * Start a fresh spinner for a new phase, clearing any previous handle first. + * ui.spinner() enforces the same single-spinner invariant globally (start() + * retires the active spinner), but clearing here additionally keeps + * this.spinner honest: it never points at a handle whose phase already ended + * (AUTH-6732 — an overwritten handle left its interval redrawing over the + * next prompt). + */ + private startSpinner(message: string): void { + this.spinner?.clear(); + this.spinner = ui.spinner(); + this.spinner.start(message); + } + /** Debug logging - only outputs when debug mode is enabled */ private debugLog = (message: string): void => { if (this.debug) { @@ -313,8 +328,7 @@ export class CLIAdapter implements InstallerAdapter { console.log(` ${chalk.cyan(verificationUri)}`); console.log(`\nEnter code: ${chalk.bold(userCode)}\n`); - this.spinner = ui.spinner(); - this.spinner.start('Waiting for authentication...'); + this.startSpinner('Waiting for authentication...'); }; private handleDeviceSuccess = (): void => { @@ -322,11 +336,8 @@ export class CLIAdapter implements InstallerAdapter { }; private handleStagingFetching = (): void => { - if (this.spinner) { - this.spinner.stop('Authenticated'); - } - this.spinner = ui.spinner(); - this.spinner.start('Fetching your WorkOS credentials...'); + this.stopSpinner('Authenticated'); + this.startSpinner('Fetching your WorkOS credentials...'); }; private handleStagingSuccess = ({ source }: InstallerEvents['staging:success']): void => { @@ -450,12 +461,22 @@ export class CLIAdapter implements InstallerAdapter { }; private handleAgentStart = (): void => { - this.spinner = ui.spinner(); - this.spinner.start(this.lastAgentMessage); + this.startSpinner(this.lastAgentMessage); // No setInterval: ui animates its own frames, and the old 2s reset // clobbered the current phase text set by handleAgentProgress. }; + /** + * The agent phase is over — finalize its spinner. Integrations that run + * validation emit validation:start (which stops it first); this covers the + * ones that don't (Ruby, or any run with --no-validate), so the spinner + * never outlives its phase into the post-install commit/PR prompts + * (AUTH-6732). Failure paths are already finalized by handleError/handleComplete. + */ + private handleAgentSuccess = (): void => { + this.stopSpinner('Agent completed'); + }; + private handleAgentProgress = ({ step, detail }: InstallerEvents['agent:progress']): void => { const message = detail ? `${step}: ${detail}` : step; this.lastAgentMessage = message; @@ -473,8 +494,7 @@ export class CLIAdapter implements InstallerAdapter { this.spinner = null; render(); if (wasRunning) { - this.spinner = ui.spinner(); - this.spinner.start(this.lastAgentMessage); + this.startSpinner(this.lastAgentMessage); } } @@ -614,8 +634,7 @@ export class CLIAdapter implements InstallerAdapter { private handleScaffoldStart = ({ packageManager }: InstallerEvents['scaffold:start']): void => { this.scaffoldPackageManager = packageManager; - this.spinner = ui.spinner(); - this.spinner.start(`Scaffolding a new Next.js app with ${packageManager} (this can take a minute)...`); + this.startSpinner(`Scaffolding a new Next.js app with ${packageManager} (this can take a minute)...`); }; // create-next-app output is verbose; surface it only under --debug and keep @@ -683,8 +702,7 @@ export class CLIAdapter implements InstallerAdapter { }; private handleCommitGenerating = (): void => { - this.spinner = ui.spinner(); - this.spinner.start('Generating commit message...'); + this.startSpinner('Generating commit message...'); }; private handleCommitSuccess = ({ message }: InstallerEvents['postinstall:commit:success']): void => { @@ -711,16 +729,14 @@ export class CLIAdapter implements InstallerAdapter { }; private handlePrGenerating = (): void => { - this.spinner = ui.spinner(); - this.spinner.start('Generating PR description...'); + this.startSpinner('Generating PR description...'); }; private handlePrPushing = (): void => { if (this.spinner) { this.spinner.message('Pushing to remote...'); } else { - this.spinner = ui.spinner(); - this.spinner.start('Pushing to remote...'); + this.startSpinner('Pushing to remote...'); } }; diff --git a/src/utils/ui.spec.ts b/src/utils/ui.spec.ts index 55575d2e..c0b4aeb5 100644 --- a/src/utils/ui.spec.ts +++ b/src/utils/ui.spec.ts @@ -196,6 +196,117 @@ describe('prompt coordination (withPrompt)', () => { }); }); +describe('spinner coordination (AUTH-6732)', () => { + let stdoutTtyDesc: PropertyDescriptor | undefined; + let writeSpy: ReturnType; + let logSpy: ReturnType; + + const writes = () => writeSpy.mock.calls.map((c) => String(c[0])); + + beforeEach(() => { + vi.useFakeTimers(); + // Spinners only animate on a TTY; stub it so the redraw interval runs. + stdoutTtyDesc = Object.getOwnPropertyDescriptor(process.stdout, 'isTTY'); + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }); + writeSpy = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.clearAllTimers(); + vi.useRealTimers(); + writeSpy.mockRestore(); + logSpy.mockRestore(); + if (stdoutTtyDesc) Object.defineProperty(process.stdout, 'isTTY', stdoutTtyDesc); + else delete (process.stdout as { isTTY?: boolean }).isTTY; + }); + + it('starting a new spinner retires the previous one (no orphaned interval)', () => { + const a = ui.spinner(); + a.start('phase A'); + vi.advanceTimersByTime(240); + expect(writes().some((w) => w.includes('phase A'))).toBe(true); + + writeSpy.mockClear(); + const b = ui.spinner(); + b.start('phase B'); + vi.advanceTimersByTime(240); + + const after = writes(); + expect(after.some((w) => w.includes('phase A'))).toBe(false); + expect(after.some((w) => w.includes('phase B'))).toBe(true); + b.stop('done'); + }); + + it('a retired spinner handle goes inert — stop()/clear() print nothing', () => { + const a = ui.spinner(); + a.start('phase A'); + const b = ui.spinner(); + b.start('phase B'); // retires A + + writeSpy.mockClear(); + logSpy.mockClear(); + a.stop('should not print'); + a.clear(); + a.message('should not render'); + vi.advanceTimersByTime(240); + + expect(writes().every((w) => !w.includes('should not'))).toBe(true); + expect(logSpy).not.toHaveBeenCalled(); + // The live spinner is untouched and keeps animating. + expect(writes().some((w) => w.includes('phase B'))).toBe(true); + b.stop('done'); + }); + + it('a prompt pauses the active spinner and resumes it after the answer', async () => { + const s = ui.spinner(); + s.start('Working'); + vi.advanceTimersByTime(160); + expect(writes().some((w) => w.includes('Working'))).toBe(true); + + let framesDuringPrompt = 0; + writeSpy.mockClear(); + vi.mocked(inquirer.confirm).mockImplementationOnce(async () => { + // While the prompt awaits input, the spinner's 80ms redraw must not fire. + vi.advanceTimersByTime(500); + framesDuringPrompt = writes().filter((w) => w.includes('Working')).length; + return true; + }); + + await ui.confirm({ message: 'ok?' }); + expect(framesDuringPrompt).toBe(0); + + writeSpy.mockClear(); + vi.advanceTimersByTime(240); + expect(writes().some((w) => w.includes('Working'))).toBe(true); + s.stop('done'); + }); + + it('replacing the spinner mid-prompt does not erase the prompt, and the old spinner stays dead', async () => { + const a = ui.spinner(); + a.start('Working'); + + let erasesAtReplace = 0; + vi.mocked(inquirer.confirm).mockImplementationOnce(async () => { + writeSpy.mockClear(); + const b = ui.spinner(); + b.start('Next phase'); // retires the paused "Working" spinner + erasesAtReplace = writes().filter((w) => w.includes('\x1b[2K')).length; + b.stop('done'); + return true; + }); + + await ui.confirm({ message: 'ok?' }); + // A paused spinner owns no line — retiring it must not wipe the prompt's. + expect(erasesAtReplace).toBe(0); + + // The prompt's resume() must not resurrect the retired spinner. + writeSpy.mockClear(); + vi.advanceTimersByTime(500); + expect(writes().every((w) => !w.includes('Working'))).toBe(true); + }); +}); + describe('dashboard mode suppresses output', () => { let logSpy: ReturnType; beforeEach(() => { diff --git a/src/utils/ui.ts b/src/utils/ui.ts index 8d5f7578..35495abf 100644 --- a/src/utils/ui.ts +++ b/src/utils/ui.ts @@ -149,12 +149,14 @@ export interface Spinner { /** * The currently-running spinner, if any. A prompt pauses it before opening so - * the 80ms redraw interval can't overwrite the question (see withPrompt). + * the 80ms redraw interval can't overwrite the question (see withPrompt), and + * start() retires it so two spinner intervals can never run at once. * Internal — not part of the public Spinner surface. */ interface PausableSpinner { pause: () => void; resume: () => void; + retire: () => void; } let activeSpinner: PausableSpinner | null = null; @@ -163,6 +165,14 @@ function spinner(): Spinner { let timer: ReturnType | undefined; let frame = 0; let text = ''; + // True from start() until stop()/clear()/retire(). Guards every write, so a + // stale handle can never scribble over its successor's output (AUTH-6732: + // an overwritten spinner handle used to leave its 80ms interval running, + // redrawing over later prompts). + let active = false; + // True while a prompt has borrowed the terminal (see withPrompt). A paused + // spinner owns no line, so retiring it must not erase the prompt's. + let paused = false; const isTty = Boolean(process.stdout.isTTY) && !dashboardMode; const render = () => { process.stdout.write(`\r${INDENT}${dim(SPINNER_FRAMES[(frame = (frame + 1) % SPINNER_FRAMES.length)])} ${text}`); @@ -176,14 +186,29 @@ function spinner(): Spinner { timer = setInterval(render, 80); } }; + const halt = () => { + if (timer) { + clearInterval(timer); + timer = undefined; + } + }; const handle: Spinner & PausableSpinner = { start(message = '') { text = message; if (dashboardMode) return; if (isTty) { + // Single-spinner invariant: a new spinner retires whatever is on screen + // first, so an orphaned interval can never overwrite later output (or a + // prompt). Retire, not stop — whether the old phase deserves a final ✓ + // line is the caller's decision (stop it explicitly first if so). + const prev = activeSpinner; + if (prev && prev !== handle) prev.retire(); + active = true; + paused = false; tick(); activeSpinner = handle; } else { + active = true; line(`${dim('…')} ${text}`); } }, @@ -191,10 +216,10 @@ function spinner(): Spinner { text = message; }, stop(message?: string, code = 0) { - if (timer) { - clearInterval(timer); - timer = undefined; - } + if (!active) return; + active = false; + paused = false; + halt(); if (activeSpinner === handle) activeSpinner = null; if (dashboardMode) return; clearLine(); @@ -205,10 +230,10 @@ function spinner(): Spinner { // a failed step being cleared before a prompt), and deregister so a prompt // doesn't resume it. clear() { - if (timer) { - clearInterval(timer); - timer = undefined; - } + if (!active) return; + active = false; + paused = false; + halt(); if (activeSpinner === handle) activeSpinner = null; if (dashboardMode) return; clearLine(); @@ -217,17 +242,29 @@ function spinner(): Spinner { // line and halts the redraw interval; resume restarts it. stop() is NOT // called, so activeSpinner stays registered across the prompt. pause() { - if (timer) { - clearInterval(timer); - timer = undefined; - } + if (!active || paused) return; + paused = true; + halt(); clearLine(); }, resume() { + if (!active || !paused) return; + paused = false; // Only resume if this handle is still the active spinner — never resurrect // a spinner that was stopped or cleared while the prompt was open. if (activeSpinner === handle && !dashboardMode) tick(); }, + // Retire on replacement (start() of another spinner): halt + deregister + // without a final line. Like clear(), but skips the erase when paused — + // the prompt owns the line then. + retire() { + if (!active) return; + active = false; + halt(); + if (activeSpinner === handle) activeSpinner = null; + if (!paused) clearLine(); + paused = false; + }, }; return handle; }