[wrangler] serialize custom build watch events to avoid concurrent runs#12704
[wrangler] serialize custom build watch events to avoid concurrent runs#12704dhruv7539 wants to merge 3 commits intocloudflare:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 9e7dab9 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Addressed both actionable comments in the latest push (8ae0df1):\n- fixed wiring in custom-build path to use \n- added a Wrangler patch changeset () |
|
Addressed both actionable comments in the latest push (
|
8ae0df1 to
90cb61d
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
| ); | ||
| }); | ||
|
|
||
| test("custom build watcher changes are processed without concurrent custom builds", async () => { |
There was a problem hiding this comment.
🟡 New test uses global expect instead of test context ({ expect }), violating AGENTS.md rule
The new test at line 258 uses async () => { without destructuring expect from the test context, then calls expect(customBuildErrors).toHaveLength(0) at line 313 using the global expect. This violates the explicit rule in packages/wrangler/AGENTS.md: "Never import expect from vitest — use test context ({ expect }) => {}". Every other test in this file (lines 72, 132, 185, 318, 371, 493, 603, 644) correctly uses the async ({ expect }) => { pattern.
| test("custom build watcher changes are processed without concurrent custom builds", async () => { | |
| test("custom build watcher changes are processed without concurrent custom builds", async ({ expect }) => { |
Was this helpful? React with 👍 or 👎 to provide feedback.
90cb61d to
c5495bd
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
| while (this.#pendingCustomBuild) { | ||
| const nextBuild = this.#pendingCustomBuild; | ||
| this.#pendingCustomBuild = undefined; | ||
| await this.#runCustomBuild( | ||
| nextBuild.config, | ||
| nextBuild.filePath, | ||
| nextBuild.buildAborter | ||
| ); |
There was a problem hiding this comment.
🟡 Stale pending custom build executes its external command when aborter is already aborted
When a config update (#startCustomBuild) occurs while a custom build is pending in the queue, #startCustomBuild at BundlerController.ts:230 aborts this.#customBuildAborter (which is the same AbortController stored in #pendingCustomBuild.buildAborter). However, #runPendingCustomBuilds at line 67-74 does not check buildAborter.signal.aborted before dequeuing and running the stale pending build, and #runCustomBuild at line 88-89 emits a bundleStart event and executes the external build command before checking the abort signal at line 101. This means a build known to be cancelled still runs its external command, emits a spurious bundleStart (without a matching bundleComplete), and if the external command fails, emits a confusing error event to downstream consumers (ProxyController, RuntimeControllers per DevEnv.ts:101-104). Adding an early abort guard would avoid this unnecessary work.
| while (this.#pendingCustomBuild) { | |
| const nextBuild = this.#pendingCustomBuild; | |
| this.#pendingCustomBuild = undefined; | |
| await this.#runCustomBuild( | |
| nextBuild.config, | |
| nextBuild.filePath, | |
| nextBuild.buildAborter | |
| ); | |
| while (this.#pendingCustomBuild) { | |
| const nextBuild = this.#pendingCustomBuild; | |
| this.#pendingCustomBuild = undefined; | |
| if (nextBuild.buildAborter.signal.aborted) { | |
| continue; | |
| } | |
| await this.#runCustomBuild( | |
| nextBuild.config, | |
| nextBuild.filePath, | |
| nextBuild.buildAborter | |
| ); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
BundlerControllerWhy
Fixes the behavior in #10944 where rapid multi-file changes can start multiple custom builds at once.
Testing
pnpm -C packages/wrangler test src/__tests__/api/startDevWorker/BundleController.test.tspnpm -C packages/wrangler exec eslint src/api/startDevWorker/BundlerController.ts src/__tests__/api/startDevWorker/BundleController.test.tsAdded regression test
custom build watcher changes are processed without concurrent custom buildsCustom build failedevents are emitted