Skip to content

Let Stop end a running npm install - #274

Merged
juanmaguitar merged 1 commit into
trunkfrom
juanmaguitar/kill-a-running-npm-install
Aug 12, 2026
Merged

Let Stop end a running npm install#274
juanmaguitar merged 1 commit into
trunkfrom
juanmaguitar/kill-a-running-npm-install

Conversation

@juanmaguitar

@juanmaguitar juanmaguitar commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Why

Nothing in the app can stop a running npm install. npm:kill resolves a child out of the script registry only — by runId, or by the directory index npm:run-script maintains — and a running install lives in a different map, runningInstalls, keyed by an installId the preload never hands back. So Stop answers {ok: false, error: 'No running script'} and the install runs to completion.

Nobody has hit it because nothing had asked to stop one: install is reached from the checklist button, and the terminal's Ctrl+C is only wired up during chains that do not include it. #246 changes that — it starts install unattended after the clone, and being able to stop it is the condition that makes running unattended reasonable at all.

What changes

installIdByDirectory, beside the existing runIdByDirectory, and a fallback in npm:kill that consults it once the two script lookups miss. A directory is all a caller can offer for an install, and the two never run for the same directory at once. Everything downstream is already generic: cancelledChildren, killChildTree, the 3-second SIGKILL backstop.

No src/preload.js change — npmKill already forwards { runId, directoryPath }, and during an install currentRunIdRef.current is null, so the directory branch is the one that runs.

Marking the child cancelled matters more for an install than for a script. A stopped install exits non-zero, and on Windows a kill surfaces as a plain exit code rather than a signal, so without it runNpmWithEngineRetry would read the stop as an engine mismatch and respawn the install the contributor just stopped. test/npm-runner.test.cjs already pins that cancelled short-circuits the retry; this makes the install path reach it.

Not in this PR: any caller. The Stop control that uses it ships in the stacked PR for #246. On its own this is reachable only through the terminal's Ctrl+C during a chain that runs an install.

How to test this

Platforms: macOS and Windows both — process-tree killing is the one thing that differs, and this is a kill path.

This has no button of its own until the stacked PR lands, so it is driven from the terminal.

Starting state: a site whose clone has finished and whose dependencies are not installed.

  1. Click Install npm dependencies. Wait for npm to start producing output in the Terminal.
  2. Press Ctrl+C in the Terminal.
  3. The install stops. Check no node/npm child of the app survives — Activity Monitor on macOS, Task Manager on Windows.
  4. The Install npm dependencies step now offers Retry npm install, and the build step stays locked.
  5. Click the retry. It starts a fresh install rather than resuming a dead one.

Before this change, step 2 does nothing at all and the install runs to completion.

What must not have happened:

Covered by npm:kill ends a running install, which only its directory can name in test/ipc-wiring.test.cjs, which fails on the old code (the handler returns {ok: false} and never reaches kill-tree).

Risks and limitations

Small and additive: one map, one fallback branch, no change to any existing lookup. The blast radius is a npm:kill call that used to fail and now succeeds.

The lifetime of installIdByDirectory mirrors runIdByDirectory exactly, including the identity-guarded delete, so a second install for the same directory cannot be made unkillable by the first one's exit.

Not tested by hand on Windows by me — the artifact from this branch is the way to do that.

Related

Groundwork for #246. Does not close anything on its own.

Design decisions and alternatives considered

A separate npm:install-kill channel was the obvious alternative. Rejected: the renderer already calls npmKill({runId, directoryPath}) from one place (killCurrent), and it does not know whether the thing it is stopping is an install or a script — that is precisely the knowledge the main process has. Two channels would have pushed that question back into the renderer for no gain.

Returning the installId to the renderer so an install could be killed by id like a script. Rejected: the preload deliberately keeps installId private, correlating log and done events internally so callers get a plain (onLog, onDone) pair. Widening that to hand out the id would change the shape of the API for every caller to serve one.

Review outcome (required — see AGENTS.md)

1 [fix here] · 0 [follow-up] — fixed.

tests 🟡 [fix here] — the new test passed under Electron's Node but failed under npm test on .nvmrc's Node: emitting close reaches npm:install's onDone, which calls getStore(), which pulled in the real electron-store and through it the real electron package, tripping the harness's own guard. Fixed by adding fakeSettingsStore() to the stubs, the same way the quit-sweep test does for the same reason. This is exactly the "green on one of the two runtimes" shape the review standard names; both runtimes are green now (840 tests each).

The judgement pass was run in this session rather than dispatched to a subagent — the tooling in use blocked spawning one. Flagging it because the standard prefers fresh context for that pass, and a self-review is the weaker version.

Screenshots or recording

Nothing on screen changes. The behaviour is in the main process; the control that exposes it ships in the stacked PR.

@juanmaguitar juanmaguitar mentioned this pull request Aug 11, 2026
Merged
@juanmaguitar @claude
Let Stop end a running npm install
6c51f64 
`npm:kill` resolved a child out of the script registry alone — by `runId`, or
by the directory index `npm:run-script` maintains. A running install lives in
`runningInstalls`, keyed by an `installId` the preload never hands back, and no
directory index pointed at it. So Stop answered "No running script" and the
install ran to completion; nothing in the app had asked to stop one before.

Add `installIdByDirectory` beside `runIdByDirectory`, and let `npm:kill` fall
back to it once the script lookups miss. A directory is all a caller can offer
for an install, and the two never run for the same directory at once.

Marking the child cancelled matters more here than for a script: a stopped
install exits non-zero, and on Windows a kill surfaces as a plain code rather
than a signal, so without it `runNpmWithEngineRetry` would read the stop as an
engine mismatch and respawn the install the contributor just stopped.

A cancelled install still records `installFailed: true`, which is the wanted
outcome — a half-written node_modules must not read as a completed setup step
(#42) — and it leaves the retry available.

Groundwork for #246, which auto-starts install and build after the clone and
can only do that if the contributor can stop it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@juanmaguitar
juanmaguitar force-pushed the juanmaguitar/kill-a-running-npm-install branch from d4c3565 to 6c51f64 Compare August 12, 2026 05:32
@juanmaguitar
juanmaguitar merged commit c528efb into trunk Aug 12, 2026
3 checks passed
juanmaguitar added a commit that referenced this pull request Aug 12, 2026
@juanmaguitar @claude
Run the initial setup steps as one chain (#246) (#276)
f63b559 
> **Stacked on #274.** Review that one first — it is the kill path this
PR's Stop button depends on. The diff below is this branch against #274,
not against trunk.

## Why

After the clone finishes, the setup checklist stops and waits. **Install
npm dependencies**, then **Run full build** — two clicks, with no
decision to make between them. The contributor's only job is to notice a
step ended and press the next button.

That is the wrong shape for a wizard. Someone who walks away during the
clone — which is what you do during a ten-minute clone at a Contributor
Day — comes back to a checklist waiting on them rather than to an
environment they can work in. #246 makes the case at length.

## What changes

Setup becomes the third chain of the shape the app already runs twice:
`planUpdateSteps` (fetch → install → build) for a trunk update,
`planApplySteps` (apply → install → build) for a patch, both sharing
`updateStepStatuses` and `updateOutcome`. `planSetupSteps` joins them,
and **the chain starts on its own when the clone finishes** — no button.

What makes running unattended reasonable rather than presumptuous, and
each of these is the reason the issue gives:

- **It is visible.** A banner names the running step and counts it
(`step 2 of 3`), and every line of npm output still goes to the Terminal
(#41).
- **It stops.** **Stop setup**, and Ctrl+C in the Terminal, end the
child — install included, which is what #274 is for.
- **It stops at the first failure.** A build on a half-installed tree
cannot work, and its failure would bury the one that mattered (#42).
- **It starts once, on an edge.** The clone going from running to
finished, once per row. A site already cloned when its row appears never
triggers, so reopening the app on a half-finished site lands on the
manual checklist rather than launching a half-hour build nobody asked
for.

**Deliberately not in it:** starting the dev server. #246 lists it as
the fourth link, and I left it out — that step also calls
`markSkipWizard()` and hands the contributor to a WordPress setup wizard
in a browser, so running it unattended would end the checklist on their
behalf and leave a server listening that nobody asked for. Also not in
it: resuming the chain after a stop. Retry is a plain button on the
failed step; a cancel followed by a retry must not silently restart a
half-hour build.

**It also closes the open half of #44.** The checklist ladder moves out
of `index.jsx` into `setupStepStatuses` and gains a `failed` state, so a
step whose last attempt lost says **Failed**, keeps its retry, and does
not hand `current` to the next step. #258 fixed the label half ("Ready"
until an action runs); the explicit failed/retryable state was the
acceptance criterion still open, and a chain that can stop is what made
it necessary rather than nice.

## How to test this

Platforms: **any** for 1 and 4–6; do **2 and 3 on both macOS and
Windows** — process-tree killing is where they differ.

**Starting state:** the app, with no site for the WordPress repo you are
about to create. This is a real clone plus a real install plus a real
build, so budget for it — the point of steps 2 and 3 is that you do not
have to sit through the build.

1. **Create a WordPress Core site.** Watch the *Download WordPress
development version* step. It should say the install and build start on
their own when it finishes.
2. **When the clone finishes, click nothing.** The blue banner appears,
*Install npm dependencies* turns **In progress**, and npm output streams
into the Terminal. When install ends, the build follows on its own and
the banner reads *step 3 of 3*.
- *Start dev server & finish wizard* must still read **Ready** and must
**not** have started.
3. **Press Stop setup** while npm install is running. The install
actually dies — check no `node`/`npm` child of the app survives. The
build never starts, the banner is replaced by *Setup stopped*, and the
install step offers **Retry npm install**.
4. **Press Stop setup during the build** instead (create a second site,
let install finish). The whole Grunt tree dies, not just the runner
(#83, #146).
5. **A failing install.** Easiest is to pull the network mid-install.
The step reads **Failed** in red with its retry live, the build stays
**Locked**, and the chain does not advance.
6. **Reopen mid-setup.** Quit during the build, relaunch, open the site.
The checklist shows the real state from disk and the chain does **not**
restart by itself.
7. **Skip initialization wizard** during a running chain. The checklist
goes away, the chain keeps running, the Terminal still shows it.

**What must not have happened:**

- **The dev server must not have started, and the wizard must not be
marked skipped.** The chain ends at the build. If you land on a running
server or a post-init view without clicking, that is the regression.
- **No orphaned `npm`/`node` process after a Stop**, on either platform.
- **A stop must not be reported as a failure.** A killed npm exits
non-zero — on Windows without even a signal — so the step must read
*Setup stopped*, not *npm install failed*.
- **A stop must not respawn itself.** Watch for a second install
starting a moment after you stopped the first (that is #274's
`cancelled` flag doing its job).
- **Reopening the app must not start anything.** Step 6 is the one that
would be easy not to notice: it only misbehaves on a site you left
half-finished, and the symptom is your laptop quietly building for half
an hour.

## Risks and limitations

**The end-to-end chain has not been driven by hand.** I could not: this
Electron build does not expose its accessibility tree to the automation
available here, and creating a site needs a native folder picker. What I
did verify is that the renderer bundles and mounts with these changes,
lint is clean, and both Node runtimes are green (840 tests). **Steps 1–7
above are unverified and want a human**, and step 3 in particular is the
one that justifies the whole feature.

`buildFailed` is session-local. Only the install outcome is persisted
(`src/main.js` records it on the site's meta), so after a restart a
failed build reads **Ready** again rather than **Failed**. That is the
honest fallback — the app still knows there is no build on disk, just
not that the last attempt lost — and persisting it is a separate change.

Auto-start is once per row **mount**, not once per site. Switching away
from a site and back during a clone remounts the row; if the clone
finishes after that remount the chain still fires, which is the wanted
behaviour, but it does mean the guard is memory, not state on disk. The
`hasNodeModules` check is what stops that mattering.

The banner says the build can take up to half an hour on Windows (#72).
It runs a production build where a dev build would do — that is #92,
untouched here, and auto-starting makes it more worth fixing, not less.

## Related

Closes #246. Closes the remaining acceptance criterion of #44. Depends
on #274. Makes #57 (the redundant nested `npm install` between the two
steps) easier to fix now the two are one run.

---

<details>
<summary>Design decisions and alternatives considered</summary>

**The checklist rows are not driven by `updateStepStatuses`.** #246
suggests they could be — "it already returns the
complete/current/pending/skipped states the checklist needs". They
cannot, and this is the one place I departed from the issue. Those
states are derived from disk (`hasNodeModules`, `hasBuilt`,
`installFailed`) precisely so a site reopened days later, with nothing
running, still shows the truth; driving them from chain position would
show every step `pending` on a reopened half-finished site. So the reuse
is real but narrower: `planSetupSteps` + `updateStepStatuses` +
`setupOutcome` drive the **banner** — progress counter, Stop, how it
ended — exactly as they drive the update panel, and the rows keep their
own ladder.

**`planSetupSteps` lives in `update-plan.cjs`** rather than in
`setup-steps.cjs`, following the `planApplySteps` precedent: the module
is where the chains and the machinery they share live, and splitting the
third one off would have meant importing `updateStepStatuses` across
modules to save a rename. Its docblock now says it owns three chains.

**`setupOutcome` separates `stopped` from
`failed-install`/`failed-build`** even though the exit codes are
identical. Telling a contributor their install "failed" when they
pressed Stop is how a tool loses their trust, and the exit code
genuinely cannot tell the two apart — so `stopped` comes from the fact
that we asked for the kill, not from what the process did.

**Auto-start is a tri-state decision, not a chain of `if`s in an
effect.** `setupAutoStartDecision` returns `skip` / `probe` / `start`,
because the decision is taken in two halves: is this the clone-finished
edge (so reading status off disk is worth it), and then does that status
say this is a fresh clone. Being able to test that is worth more than
the shape being slightly unusual — it is the riskiest behaviour in the
PR.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

2 [fix here] · 0 [follow-up] — both fixed.

**architecture 🟡 [fix here]** — the auto-start decision lived inline in
a `useEffect` in `index.jsx`, which the suite cannot reach. Per §1, the
finding is the missing module, not the missing test — and this was the
riskiest decision in the change (too eager and it launches a half-hour
build unasked, too shy and the wizard never finishes itself). Extracted
to `setupAutoStartDecision` with nine tests covering the edge, the
arming, the two refusals and a failed status probe.

**architecture 🟡 [fix here]** — four branchy user-facing strings
(install and build labels and descriptions) were derived inline in
`index.jsx`. §1 names "a string the user reads" explicitly. Extracted to
`setupStepCopy`, which takes the same flags as `computeSetupStepState`
so the words and the button state cannot disagree; four tests, including
one pinning that pairing for the #42 case.

The judgement pass was run in this session rather than dispatched to a
subagent — the tooling in use blocked spawning one. Flagging it because
the standard prefers fresh context for that pass, and self-review is the
weaker version. Worth a second pair of eyes on the effect in `index.jsx`
in particular.

</details>

<details>
<summary>Implementation notes</summary>

The chain reuses the wizard's own `runInstall` / `runScript` wrappers,
the same way `runUpdateInstallAndBuild` does, so exit codes, retries,
terminal streaming and the post-run status reload all behave identically
to every other path. Nothing new crosses IPC.

`runScript` gained one line: it clears `buildFailed` when a build starts
and records it on exit. Clearing on start rather than on the next exit
is what stops a step reading **Failed** while its own retry is streaming
output — there is a test for that shape on the install side.

`deriveNextAction` is unchanged. The call site now passes a failed step
as the current one, since retrying it is what the contributor should do
next and a `failed` row consumes no `current` — without that, a stopped
chain would leave the view with no cue at all.

</details>

<details>
<summary>Screenshots or recording</summary>

None — see the limitation above: I could not drive the app's UI from
this environment, so there is no shot of the banner or the failed row.
This is the part of the PR that most wants a human with the Buildkite
artifact.

</details>

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@juanmaguitar