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 013035dfe7..2808a12dbb 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 eb93e7f529..32f7f16366 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 06c85d5680..bf7ecbf24b 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 6f99d6ed7b..058d663dac 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 52d600cdcb..09146d0cd8 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 } /**