Merge pull request #7554 from Shopify/autopgrade-fails-in-app-dev

Trigger auto-upgrade when dev is quit via q or Ctrl+C

Author: alfonso-noriega

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()
 

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<DevProps> = ({
       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()

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 {

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<DevSesionUIProps> = ({
       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(() => {

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
 }
 
 /**