Trigger auto-upgrade when dev is quit via q or Ctrl+C#7554
Conversation
c0444fc to
dacb80b
Compare
|
/snapit |
1 similar comment
|
/snapit |
|
🫰✨ Thanks @alfonso-noriega! Your snapshot has been published to npm. Test the snapshot by installing your package globally: pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260515095956Caution After installing, validate the version by running |
8165d1f to
629f2fe
Compare
|
/snapit |
|
🫰✨ Thanks @alfonso-noriega! Your snapshot has been published to npm. Test the snapshot by installing your package globally: pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260515114340Caution After installing, validate the version by running |
|
|
||
| // Run auto-upgrade *before* flipping the postRunHookCompleted flag so consumers that | ||
| // poll it (e.g. `waitForPostRunHookAndExit` from `app dev`) don't `treeKill` the | ||
| // process mid-upgrade. |
There was a problem hiding this comment.
No need for this comment, it just the logical thing to have postRunHookCompleted = true at the end of the hook
| // Wait for the oclif postrun hook (which triggers auto-upgrade) to finish before | ||
| // tree-killing the process tree, otherwise quitting via `q` or Ctrl+C skips | ||
| // auto-upgrade. | ||
| waitForPostRunHookAndExit() |
There was a problem hiding this comment.
no need for the extra comments, seems like we are explaining the same thing in 3 different places.
waitForPostRunHookAndExit already has a description, anyone would understand what it does.
let's remove this comment and the one in Dev.tsx
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.
629f2fe to
ed80b03
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/hooks/postrun.d.ts@@ -1,7 +1,3 @@
-/**
- * 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 { Hook } from '@oclif/core';
/**
* Check if post run hook has completed.
@@ -9,6 +5,21 @@ import { Hook } from '@oclif/core';
* @returns Whether post run hook has completed.
*/
export declare function postRunHookHasCompleted(): boolean;
+/**
+ * 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 need this when the user terminates
+ * the command via 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 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 is still running.
+ *
+ * The flag is flipped at the very end of the hook after
+ * resolves, so polling it here is safe.
+ */
+export declare function waitForPostRunHookAndExit(): void;
export declare const hook: Hook.Postrun;
/**
* Auto-upgrades the CLI after a command completes, if a newer version is available.
|

WHY are these changes introduced?
When a developer runs
shopify app devand quits the command viaqorCtrl+C, the CLI's auto-upgrade either never started or — more confusingly — started and got killed mid-stream. There were two distinct bugs combining to produce this:Dev.tsx(the legacy dev UI) hard-killed the process tree with a fixed 2 ssetTimeoutimmediately after the abort fired. Once the postrun hook entered the auto-upgrade flow, thetreeKillwould race against it; ifnpm/pnpm/yarn installtook longer than 2 s (almost always), the kill won and the upgrade was lost.DevSessionUI.tsx(the dev-session UI) tried to do better by pollingpostRunHookHasCompleted(). But the flag was being set in the postrun hook beforeautoUpgradeIfNeeded()was awaited — so as soon as the hook entered auto-upgrade the flag flipped totrue, the poll fired, andtreeKillran while the upgrade was still in progress. (This explains the inconsistent reports of "auto-upgrade started — sometimes finished — sometimes didn't".)Once the user aborts a dev command,
useAbortSignaldoes eventually callcomplete()→ Ink'sexit()→waitUntilExit()resolves → the devrun()returns → oclif fires the postrun hook →autoUpgradeIfNeeded()runs. The dev sub-processes (servers, watchers, …) keep the event loop alive after that, so the node process won't exit on its own — we still need totreeKill. We just need to do so after auto-upgrade actually finishes.WHAT is this pull request doing?
Two changes in
packages/cli-kit/src/public/node/hooks/postrun.ts:postRunHookCompleted = trueto afterautoUpgradeIfNeeded()resolves, so the flag accurately reflects "the postrun hook (including auto-upgrade) is done".waitForPostRunHookAndExit()that pollspostRunHookHasCompleted()every 100 ms and, once true,treeKills the process tree and exits. A 120 s safety cap guarantees a stuck upgrade can't keep the dev process alive forever.Both
Dev.tsxandDevSessionUI.tsxnow callwaitForPostRunHookAndExit()from their abort handler instead of doing their ownsetTimeout/ leaky polling. The corresponding unit tests mockwaitForPostRunHookAndExitso the abort-path scenarios don't trigger real intervals ortreeKill/process.exit.Files changed:
packages/cli-kit/src/public/node/hooks/postrun.ts— flag-ordering fix + newwaitForPostRunHookAndExit()helper.packages/app/src/cli/services/dev/ui/components/Dev.tsx— use the helper, drop the 2 ssetTimeoutand theisUnitTest/treeKill/context/localimports.packages/app/src/cli/services/dev/ui/components/DevSessionUI.tsx— use the helper, drop the leaky inline polling and theisUnitTest/treeKill/context/localimports.packages/app/src/cli/services/dev/ui/components/Dev.test.tsx— mock the helper.packages/app/src/cli/services/dev/ui/components/DevSessionUI.test.tsx— mock the helper.How to test your changes?
SHOPIFY_CLI_FORCE_AUTO_UPGRADE=1and ensureversionToAutoUpgrade()would return a newer version (e.g. by pointing the CLI at a stale version).shopify app devagainst a sample app.q(orCtrl+C) to quit.Shutting down dev …, the auto-upgrade output appears and completes, and only then does the process exit. Onmaintoday the process exits before the upgrade can run, or the upgrade gets killed mid-install.DevSessionUI) — same expected behavior.Post-release steps
N/A.
Checklist
treeKillpath is reused.patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add