[wrangler] fix: prevent repeated reloads with circular service bindings

[mrbbot]

Fixes #4673.

What this PR solves / how to test:

[email protected] introduced a bug where starting multiple wrangler dev sessions with service bindings to each other resulted in a reload loop. This change ensures Wrangler only reloads when dependent wrangler dev sessions start/stop, by registering workers in the dev registry with the URL of the proxy worker, not the user worker.

Using the user worker URL meant that when a dependent service was started, the original service picked up the change and restarted its user worker getting a new port. The original service registered this new port, which was picked up by the dependent service which itself restarted its user worker on a new port. This created a reload loop.

We're planning to slightly change how dev registry registration works as part of the later WaaL milestones. This should also help prevent these sorts of issues.

To test this, add the following to fixtures/service-bindings-app...

[[services]]
binding = "AYY"
service = 'a'

...then run pnpm dev in fixtures/serivce-bindings-app. Both wrangler dev sessions should start up without entering into a reload loop.

Author has addressed the following:

• Tests
  • Included
  • Not necessary because: we currently skip most of our dev registry tests because of flakiness in CI. Once [WIP] Implement dev registry as background daemon #4241 lands, we should re-enable these tests, including coverage for circular bindings like this.
• Changeset (Changeset guidelines)
  • Included
  • Not necessary because:
• Associated docs
  • Issue(s)/PR(s):
  • Not necessary because: fixing a regression

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

[changeset-bot]

🦋 Changeset detected

Latest commit: af72a72

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

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

[github-actions]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7422613647/npm-package-wrangler-4699

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7422613647/npm-package-wrangler-4699

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7422613647/npm-package-wrangler-4699 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7422613647/npm-package-miniflare-4699

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7422613647/npm-package-cloudflare-pages-shared-4699

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7422613647/npm-package-create-cloudflare-4699 --no-auto-update

Note that these links will no longer work once the GitHub Actions artifact expires.

---

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	workspace:*	3.20231218.1
workerd	1.20231218.0	1.20231218.0
workerd --version	1.20231218.0	2023-12-18

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f4da476) 75.58% compared to head (af72a72) 75.65%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4699      +/-   ##
==========================================
+ Coverage   75.58%   75.65%   +0.06%     
==========================================
  Files         243      243              
  Lines       13096    13096              
  Branches     3375     3376       +1     
==========================================
+ Hits         9899     9908       +9     
+ Misses       3197     3188       -9     

Files	Coverage Δ
packages/wrangler/src/api/startDevWorker/events.ts	15.38% <100.00%> (ø)
packages/wrangler/src/dev/local.tsx	28.39% <100.00%> (+2.46%)	⬆️
packages/wrangler/src/dev/start-server.ts	78.03% <100.00%> (ø)

... and 6 files with indirect coverage changes

[petebacondarwin]

So this is the crux of this fix, right?

[mrbbot]

Kinda, we still register the local worker, just with the URL of the proxy worker, not the user worker. The proxy worker has a stable URL across reloads, breaking the reload loop.

[petebacondarwin]

Nice! This works for me. I pulled it down and also tested locally.

[petebacondarwin]

To test this locally, I followed the procedure laid out in the PR description:

To test this, add the following to fixtures/service-bindings-app...

[[services]]
binding = "AYY"
service = 'a'

...then run pnpm dev in fixtures/serivce-bindings-app. Both wrangler dev sessions should start up without entering into a reload loop.

I was able to see that we got the reloads on main and not after this PR.
I was also able to run the simple worker-app fixture after this PR as normal.

[RamIdeas]

To add some context after discussion with @mrbbot:

This was a good fix to go in now but the real fix is to register the ProxyWorker with the Dev Registry in startDevWorker Phase 2. This fix will be reverted once that is implemented 👍