From ed80b03bab95caadd51a4f46896b075538f5686d Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Fri, 15 May 2026 11:01:28 +0200 Subject: [PATCH] Trigger auto-upgrade when dev is quit via q or Ctrl+C The legacy dev UI (Dev.tsx) tore down the process tree with a fixed 2s setTimeout immediately after the user pressed q or hit Ctrl+C. Because oclif only invokes the postrun hook after run() resolves, exiting that early meant the auto-upgrade flow never had a chance to run. Users on the standard `shopify app dev` path therefore stopped receiving CLI auto-upgrades after quitting dev. DevSessionUI.tsx already polled postRunHookHasCompleted() before exiting, but its setInterval was never cleared and the 5s ceiling was too tight for an actual upgrade. This change extracts the shutdown logic into a single helper, waitForPostRunHookAndExit, in cli-kit's postrun module. Both Dev.tsx and DevSessionUI.tsx now call it. The helper: - polls postRunHookHasCompleted() and exits as soon as the hook is done, - caps the wait at a configurable maxWaitMs (default 30s) so a stuck upgrade still terminates the process, - properly clears its interval and guards against double-exit, - delegates to the existing treeKill+process.exit path. --- .../services/dev/ui/components/Dev.test.tsx | 7 ++++ .../cli/services/dev/ui/components/Dev.tsx | 10 +---- .../dev/ui/components/DevSessionUI.test.tsx | 7 ++++ .../dev/ui/components/DevSessionUI.tsx | 17 +------- .../cli-kit/src/public/node/hooks/postrun.ts | 39 ++++++++++++++++++- 5 files changed, 56 insertions(+), 24 deletions(-) diff --git a/packages/app/src/cli/services/dev/ui/components/Dev.test.tsx b/packages/app/src/cli/services/dev/ui/components/Dev.test.tsx index 013035dfe7b..2808a12dbba 100644 --- a/packages/app/src/cli/services/dev/ui/components/Dev.test.tsx +++ b/packages/app/src/cli/services/dev/ui/components/Dev.test.tsx @@ -27,6 +27,13 @@ vi.mock('@shopify/cli-kit/node/system', async () => { vi.mock('../../../context.js') vi.mock('../../fetch.js') vi.mock('../../processes/dev-session.js') +vi.mock('@shopify/cli-kit/node/hooks/postrun', async () => { + const actual: any = await vi.importActual('@shopify/cli-kit/node/hooks/postrun') + return { + ...actual, + waitForPostRunHookAndExit: vi.fn(), + } +}) const developerPlatformClient = testDeveloperPlatformClient() diff --git a/packages/app/src/cli/services/dev/ui/components/Dev.tsx b/packages/app/src/cli/services/dev/ui/components/Dev.tsx index eb93e7f529a..32f7f163666 100644 --- a/packages/app/src/cli/services/dev/ui/components/Dev.tsx +++ b/packages/app/src/cli/services/dev/ui/components/Dev.tsx @@ -10,8 +10,7 @@ import {Box, Text, useInput, useStdin} from '@shopify/cli-kit/node/ink' import {handleCtrlC} from '@shopify/cli-kit/node/ui' import {openURL} from '@shopify/cli-kit/node/system' import figures from '@shopify/cli-kit/node/figures' -import {isUnitTest} from '@shopify/cli-kit/node/context/local' -import {treeKill} from '@shopify/cli-kit/node/tree-kill' +import {waitForPostRunHookAndExit} from '@shopify/cli-kit/node/hooks/postrun' import {Writable} from 'stream' export interface DeveloperPreviewController { @@ -74,12 +73,7 @@ const Dev: FunctionComponent = ({ setIsShuttingDownMessage('Shutting down dev because of an error ...') } else { setIsShuttingDownMessage('Shutting down dev ...') - setTimeout(() => { - if (isUnitTest()) return - treeKill(process.pid, 'SIGINT', false, () => { - process.exit(0) - }) - }, 2000) + waitForPostRunHookAndExit() } clearInterval(pollingInterval.current) await developerPreview.disable() diff --git a/packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx b/packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx index 06c85d56802..bf7ecbf24b2 100644 --- a/packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx +++ b/packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx @@ -24,6 +24,13 @@ vi.mock('@shopify/cli-kit/node/system', async () => { }) vi.mock('@shopify/cli-kit/node/context/local') vi.mock('@shopify/cli-kit/node/tree-kill') +vi.mock('@shopify/cli-kit/node/hooks/postrun', async () => { + const actual: any = await vi.importActual('@shopify/cli-kit/node/hooks/postrun') + return { + ...actual, + waitForPostRunHookAndExit: vi.fn(), + } +}) const mocks = vi.hoisted(() => { return { diff --git a/packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx b/packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx index 6f99d6ed7b4..058d663dace 100644 --- a/packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx +++ b/packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx @@ -17,9 +17,7 @@ import {Box, Text, useInput, useStdin} from '@shopify/cli-kit/node/ink' import {handleCtrlC} from '@shopify/cli-kit/node/ui' import {openURL, terminalSupportsHyperlinks} from '@shopify/cli-kit/node/system' import figures from '@shopify/cli-kit/node/figures' -import {isUnitTest} from '@shopify/cli-kit/node/context/local' -import {treeKill} from '@shopify/cli-kit/node/tree-kill' -import {postRunHookHasCompleted} from '@shopify/cli-kit/node/hooks/postrun' +import {waitForPostRunHookAndExit} from '@shopify/cli-kit/node/hooks/postrun' import {Writable} from 'stream' interface DevStatusShortcut extends TabShortcut { @@ -70,18 +68,7 @@ const DevSessionUI: FunctionComponent = ({ setIsShuttingDownMessage('Shutting down dev ...') await onAbort() } - if (isUnitTest()) return - - // Wait for the post run hook to complete or timeout after 5 seconds. - let totalTime = 0 - setInterval(() => { - if (postRunHookHasCompleted() || totalTime > 5000) { - treeKill(process.pid, 'SIGINT', false, () => { - process.exit(0) - }) - } - totalTime += 100 - }, 100) + waitForPostRunHookAndExit() }) const errorHandledProcesses = useMemo(() => { diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 52d600cdcbc..09146d0cd8c 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -2,6 +2,7 @@ * Postrun hook — uses dynamic imports to avoid loading heavy modules (base-command, analytics) * at module evaluation time. These are only needed after the command has already finished. */ +import {treeKill} from '../tree-kill.js' import {Command, Hook} from '@oclif/core' let postRunHookCompleted = false @@ -15,6 +16,42 @@ export function postRunHookHasCompleted(): boolean { return postRunHookCompleted } +/** + * Wait for the postrun hook to finish (so auto-upgrade has a chance to run) and then + * tree-kill the current process tree before exiting. + * + * Long-running interactive commands like `app dev` need this when the user terminates + * the command via `q` or Ctrl+C. The dev sub-processes such as servers and watchers keep + * the event loop alive, so even after oclif's postrun hook completes the node process + * won't exit on its own and we have to `treeKill` the process tree. We must not do that + * before the postrun hook has actually finished running auto-upgrade, otherwise we would + * kill the upgrade mid-way while `npm install` is still running. + * + * The flag `postRunHookCompleted` is flipped at the very end of the hook after + * `autoUpgradeIfNeeded` resolves, so polling it here is safe. + */ +export function waitForPostRunHookAndExit(): void { + const pollIntervalMs = 100 + // Auto-upgrade can take a while (npm/pnpm/yarn install). Cap the wait generously so + // a stuck upgrade still terminates the process eventually. + const maxWaitMs = 120000 + + let elapsed = 0 + let terminating = false + const handle = setInterval(() => { + if (terminating) return + if (postRunHookHasCompleted() || elapsed >= maxWaitMs) { + terminating = true + clearInterval(handle) + treeKill(process.pid, 'SIGINT', false, () => { + process.exit(0) + }) + return + } + elapsed += pollIntervalMs + }, pollIntervalMs) +} + // This hook is called after each successful command run. More info: https://oclif.io/docs/hooks export const hook: Hook.Postrun = async ({config, Command}) => { await detectStopCommand(Command as unknown as typeof Command) @@ -28,9 +65,9 @@ export const hook: Hook.Postrun = async ({config, Command}) => { const {outputDebug} = await import('../output.js') const command = Command.id.replace(/:/g, ' ') outputDebug(`Completed command ${command}`) - postRunHookCompleted = true if (!command.includes('notifications') && !command.includes('upgrade')) await autoUpgradeIfNeeded() + postRunHookCompleted = true } /**