TEMP: validate the deferred-registration fix on Windows (do not merge) - #15025
TEMP: validate the deferred-registration fix on Windows (do not merge)#15025petebacondarwin wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: c5edab7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
|
✅ All changesets look good |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This is a throwaway Windows validation harness for #15024 that also carries the real deferred-dev-registry-registration fix (miniflare + vite-plugin). The production fix itself looks logically sound: #registerWorkers() is mutex-protected and guarded by isEnabled(), the hold is correctly re-armed on setOptions(), and reads of the registry are left untouched so peer resolution still works. My only concerns are around merge-safety and one imprecise doc comment.
Ranked:
-
(medium) Merge-safety: this PR is labelled "do not merge", but it contains a real, non-temporary changeset that bumps
miniflareto a minor release. If this branch is ever merged (or the changeset is copied into another PR), it will trigger a real package release together with the temporary CI scaffolding (--force, the isolated@fixture/dev-registryfilter, the 60-minute timeout, theround 1..4loop, and theCRASH-DETECTEDlogging). Consider gating the release behind the actual fix PR rather than this harness, or renaming the changeset so it can't accidentally publish from here. -
(low)
unsafeRegisterInDevRegistry()JSDoc says it is "a no-op unlessunsafeDeferDevRegistryRegistrationis set", but the implementation unconditionally sets#devRegistryRegistrationReleased = trueand calls#registerWorkers()regardless of the option. Harmless (idempotent), but the comment is inaccurate.
| * put in place by `unsafeDeferDevRegistryRegistration`. | ||
| * | ||
| * Call this once the runtime is the one peers should actually connect to. It | ||
| * is idempotent, and a no-op unless `unsafeDeferDevRegistryRegistration` is |
There was a problem hiding this comment.
This says the method is "a no-op unless unsafeDeferDevRegistryRegistration is set", but the body below always sets #devRegistryRegistrationReleased = true and always calls #registerWorkers() (which itself only checks #devRegistry.isEnabled()), independent of the defer option. It's idempotent so no functional harm, but the comment is misleading.
| * is idempotent, and a no-op unless `unsafeDeferDevRegistryRegistration` is | |
| * Call this once the runtime is the one peers should actually connect to. It | |
| * is idempotent. If the dev registry is enabled it advertises this instance's | |
| * Workers even when `unsafeDeferDevRegistryRegistration` was not set (in which | |
| * case they will already have been advertised). Registration does not restart | |
| * `workerd`, so this is cheap. |
|
Review posted successfully to PR #15025. Summary of what I did:
The core production fix logic itself is sound (mutex-protected registration, |
The plugin starts workerd twice: once to discover each Worker's exports by running it, then again with a config built from what it found. The first runtime was published to the dev registry and then torn down, leaving peers holding a debug port that no longer existed. On Windows a peer with a tail_consumers edge to it aborts its own workerd with std::terminate. Miniflare gains unsafeDeferDevRegistryRegistration to hold back self-advertisement, and unsafeRegisterInDevRegistry() to release it once the runtime is final. Reading the registry is unaffected, so a starting session still resolves Workers from sessions already running.
55041e7 to
4ea6155
Compare
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
4ea6155 to
c5edab7
Compare
2 participants
Throwaway validation for #15024. Repeats the
vite dev <-> vite devsuite 4x on Windows with explicit crash reporting, and isolates the dev-registry fixture so an unrelated fixture flake cannot abort the job before it runs.Expectation: no
CRASH-DETECTEDlines, and the target test well under its 50s budget.Note
This is a contribution from an AI agent: OpenCode, claude-opus-5.