Remove the dev server's listeners when its start throws

[zaerl]

Why

window.api.startServer subscribes to playground:log, playground:url and playground:stopped before it invokes playground:start, and removes them only when the server announces it has stopped. A start that throws (rather than answering { ok: false }) has no server, so nothing ever announces a stop and the three listeners stay for the life of the window. The site's next server would then open the site in the browser once more for each of them, and print its log twice (#604).

Today a contributor cannot reach that next server: after a thrown start the button refuses every further click until the app restarts. #603 fixes that dead button, and once it lands this leak becomes visible. This PR is the half that makes the retry clean, and does not depend on #603.

What changes

• startServer in src/preload.js takes the shape runPullRequest in the same file already has: one cleanup, called by the stop handler as before, and now also when the invoke throws, before the error is re-thrown.
• startPhpServer's catch in use-dev-server.jsx also drops the mail subscription, as stopDevServer does on every other way out. That subscription is guarded against stacking (use-site-mail.jsx), so this is tidying rather than a second leak: before, one subscription just stayed open while no server ran.

Root cause: the only removal path ran on an event that a start which throws before spawning can never produce. The { ok: false } failures (a server that exits before reporting an address, the 120 s timeout) were never affected: each has a child whose close sends playground:stopped.

Deliberately not here: the dead button after a thrown start. That is #603, by another author.

How to test this

Platforms: any. Nothing here touches paths, spawning or line endings.

Covered by the unit suite, which runs on both platforms on every PR:

node --test tests/unit/preload-listeners.test.cjs

Run from the repository root. startServer leaves no listener behind when the start throws, and the next server is heard once fails on trunk (all three listener counts stay at 1, and the failed start's callbacks receive the next server's log line and address) and passes here. A second test pins the existing removal on playground:stopped, including that another site's stop does not remove this site's listeners.

By hand, only once #603 is also applied, since without it there is no second start to watch. Build from a branch with both, or from source:

Starting state: a built site, server stopped. In src/main.js, make the playground:start handler throw once (the issue's step 1: let once = true; above it, and if (once) { once = false; throw new Error('forced failure'); } as its first line), then npm start from the repository root.

1. Click Start development server. The Server log says Failed to start PHP server: … forced failure, and the button goes back to Start development server.
2. Click Start development server again. The site opens in the browser once.

What must not have happened: the site opening in two browser tabs at step 2, or each line of the Server log at step 2 appearing twice. Revert the main.js edit afterwards.

Risks and limitations

• Low. The change only runs on a path that ends in a thrown playground:start, which needs readSiteMeta, the store, readSettings or the spawn itself to throw. The normal start and stop paths are unchanged, and the existing removal on playground:stopped is now pinned by a test.
• The hook change has no test. It is one unconditional call inside a .jsx hook, which the suite cannot load (TESTING.md, layer 1).
• Review: 0 [fix here] · 1 [follow-up], the follow-up being Fix: Fixes dev server retry after start failure #603's fix.

Related

Fixes #604. Independent of #603, and either can merge first. They touch the same catch in use-dev-server.jsx on neighbouring lines, so the second to merge may need a trivial rebase.

---

Design decisions and alternatives considered

• Copy runPullRequest rather than invent a new shape. It is the bridge in the same file that already solved this, and the issue suggested it.
• Not a journey. The leak lives entirely in the preload closure, which tests/unit/preload-listeners.test.cjs already loads against a fake ipcRenderer for exactly this class of bug (debug.log lines duplicate after each dev-server restart (renderer listener leak) #86, Preload's per-run IPC listener plumbing is untested #149, Open a pull request from the app, without a push credential on disk #167). That is the highest layer that sees it. A journey would also need Fix: Fixes dev server retry after start failure #603 to reach a second start.
• Not stopDevServer() in the hook's catch. That would also reset devServerActiveRef and fix the dead button, which is Fix: Fixes dev server retry after start failure #603's change, by its author.

Review outcome (required — see AGENTS.md)

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

• 🟡 [follow-up], architecture (failure paths), use-dev-server.jsx startPhpServer catch: devServerActiveRef stays true after a thrown start, so the Start button is a silent no-op until restart. It predates this change and is exactly what Fix: Fixes dev server retry after start failure #603 fixes, so it is deferred to Fix: Fixes dev server retry after start failure #603 rather than taken over here. The new test's header comment was reworded after the review to say that the leak's effect is only reachable once a retry is possible.

Lint and tests: npm test passes 2095, 0 fail. ESLint on the changed files is clean. Repo-wide npm run lint fails locally only inside gitignored .claude/worktrees copies, not in tracked files.

• Review: completed. Separate agent context (Claude Code Explore subagent) against .github/instructions/code-review.instructions.md, step 3. Covered the uncommitted change on base f5b3b81; outcome as above.
• Since review: committed as 77246ea. The only difference from the reviewed diff is the test header comment reworded as described above; no code changed. Trunk has since moved to b17ca54 (Skip the unit and E2E suites on pull requests that change only Markdown #660, CI workflow paths only), which does not touch these files.

Screenshots or recording

Nothing on screen changes.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8ca1827f-518c-4349-9d7b-9f52b090f11c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}