Skip to content

DO NOT MERGE: validate dev registry registration fix on Windows - #15041

Closed
edmundhung wants to merge 2 commits into
mainfrom
edmundhung/validate-dev-registry-registration
Closed

DO NOT MERGE: validate dev registry registration fix on Windows#15041
edmundhung wants to merge 2 commits into
mainfrom
edmundhung/validate-dev-registry-registration

Conversation

@edmundhung

@edmundhung edmundhung commented Aug 5, 2026

Copy link
Copy Markdown
Member

Temporary validation harness for #15040, addressing #15035. Not for merge.

This reuses #15038's harness so the result is directly comparable with Pete's validation of #15037. It lifts #15018's Windows skip on fixtures/dev-registry, repeats the vite dev <-> vite dev suite four times, dumps every session's timestamped output, reports CRASH-DETECTED per session, isolates the fixture in the workflow, and raises the job timeout.

This PR should be closed once the Windows result has been measured and recorded on #15040.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because: this branch is a throwaway measurement harness and will never be merged.
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: throwaway validation branch.

Open in Devin Review

@changeset-bot

changeset-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 544d014

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

This PR includes changesets to release 8 packages
Name Type
miniflare Minor
wrangler Patch
@cloudflare/vite-plugin Patch
@cloudflare/deploy-helpers Patch
@cloudflare/pages-shared Patch
@cloudflare/remote-bindings Patch
@cloudflare/runtime-types Patch
@cloudflare/vitest-pool-workers 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-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Aug 5, 2026
@workers-devprod
workers-devprod requested review from a team and NuroDev and removed request for a team August 5, 2026 14:58
@workers-devprod

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • @cloudflare/wrangler
Show detailed file reviewers
  • .changeset/quiet-workers-register.md: [@cloudflare/wrangler]
  • .github/workflows/test-and-check.yml: [@cloudflare/wrangler]
  • fixtures/dev-registry/tests/dev-registry.test.ts: [@cloudflare/wrangler]
  • fixtures/entrypoints-rpc-tests/tests/entrypoints.spec.ts: [@cloudflare/wrangler]
  • packages/miniflare/README.md: [@cloudflare/wrangler]
  • packages/miniflare/src/index.ts: [@cloudflare/wrangler]
  • packages/miniflare/src/plugins/core/index.ts: [@cloudflare/wrangler]
  • packages/miniflare/src/shared/DEV_REGISTRY.md: [@cloudflare/wrangler]
  • packages/miniflare/src/shared/dev-registry.ts: [@cloudflare/wrangler]
  • packages/miniflare/test/dev-registry.spec.ts: [@cloudflare/wrangler]
  • packages/miniflare/test/plugins/local-explorer/aggregation.spec.ts: [@cloudflare/wrangler]
  • packages/miniflare/test/plugins/local-explorer/index.spec.ts: [@cloudflare/wrangler]
  • packages/miniflare/test/plugins/queues/cross-process.spec.ts: [@cloudflare/wrangler]
  • packages/vite-plugin-cloudflare/src/miniflare-options.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/api/integrations/platform/index.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/dev/miniflare/index.ts: [@cloudflare/wrangler]

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

✅ All changesets look good

@ask-bonk

ask-bonk Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

vitest-pool-workers doesn't use the dev registry, so no change needed there. All internal consumers that need registration have been updated (wrangler dev, wrangler platform integration, vite dev, vite preview).

I've completed a thorough review. Summary of findings:

  • The core functional change (unsafeRegisterWorker opt-in) is correct, well-tested, documented, and all internal call sites are updated.
  • The register() early-return on empty entries is safe (unregister is handled separately in updateRegistryPath).
  • The workflow/test changes are explicitly temporary validation scaffolding, which the author has clearly marked and does not intend to merge.
  • No logic bugs, security issues, or backward-compat violations in the shippable code (unsafe* API, correctly classified as minor).

The only "issues" are the intentional DO-NOT-MERGE scaffolding (60min timeout, isolated fixture filter, 4x loop, lifted Windows skip), which the author has explicitly flagged as temporary and states will be closed rather than merged. Flagging these would be redundant with the author's own clearly-stated intent, and there are no concrete fixes to suggest for a deliberate throwaway harness.

No actionable code issues.

LGTM

github run

@pkg-pr-new

pkg-pr-new Bot commented Aug 5, 2026

Copy link
Copy Markdown
@cloudflare/autoconfig

npm i https://pkg.pr.new/@cloudflare/autoconfig@15041

@cloudflare/build-output-utils

npm i https://pkg.pr.new/@cloudflare/build-output-utils@15041

@cloudflare/config

npm i https://pkg.pr.new/@cloudflare/config@15041

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@15041

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@15041

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@15041

miniflare

npm i https://pkg.pr.new/miniflare@15041

@cloudflare/pages-functions

npm i https://pkg.pr.new/@cloudflare/pages-functions@15041

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@15041

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@15041

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@15041

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@15041

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@15041

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@15041

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@15041

wrangler

npm i https://pkg.pr.new/wrangler@15041

commit: 544d014

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

@edmundhung
edmundhung marked this pull request as draft August 5, 2026 15:04
@edmundhung

Copy link
Copy Markdown
Member Author

Validation result: this does not fix the Windows flake, so #15040 should be judged as the internal-worker collision/churn fix for #15035 only.

I reused #15038's harness unchanged in #15041 and ran the vite dev <-> vite dev suite four times on Windows. The result was:

  • 19 std::terminate aborts (versus 23 in DO NOT MERGE: validate dev-registry reconcile fix on Windows #15038)
  • 11 sessions marked CRASH-DETECTED
  • 3 failed tests out of 42:
    • round 1 supports exported handler fetch over service binding returned Vite HTML instead of the Worker response
    • round 2 of the same test timed out
    • round 2 supports tail handler timed out in waitFor

Job: https://github.com/cloudflare/workers-sdk/actions/runs/31017919389/job/92346768939

The difference in raw abort count is too small and noisy to indicate an improvement; the abort remains readily reproducible after the internal registry entries are removed. #15018's Windows skip needs to stay. This rules out the fixed-name internal-worker collisions as the sole trigger, while #15040 still removes those accidental entries and the cross-session registry traffic they cause.

@edmundhung edmundhung closed this Aug 5, 2026
@github-project-automation github-project-automation Bot moved this from Untriaged to Done in workers-sdk Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants