Skip to content

[miniflare] Fix resource leaks during config updates#13586

Merged
petebacondarwin merged 1 commit intomainfrom
fix/miniflare-dispose-cleanup-followup
Apr 20, 2026
Merged

[miniflare] Fix resource leaks during config updates#13586
petebacondarwin merged 1 commit intomainfrom
fix/miniflare-dispose-cleanup-followup

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin commented Apr 17, 2026

Fixes #13584, #13585.

Follow-up to #13515 addressing two resource-cleanup issues identified during review:

devRegistryDispatcher unconditionally closed on every config update (#13584):
The #devRegistryDispatcher was torn down and rebuilt on every #assembleAndUpdateConfig() call, regardless of whether the port changed. This is inconsistent with #runtimeDispatcher (which only recreates when the entry URL changes) and could cause brief request failures if #pushRegistryUpdate() is in-flight when the pool is closed. Fixed by tracking the port and only closing/recreating when it actually changes.

Old InspectorProxy instances not disposed during updateConnection (#13585):
InspectorProxyController.updateConnection() replaced this.#proxies with new InspectorProxy instances without first disposing the old ones. This leaked runtime WebSocket connections and their 10-second keepalive interval timers. Fixed by disposing old proxies before replacement.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because: existing miniflare test suite (734 tests) passes; both fixes are straightforward resource-cleanup changes with no behavioral impact beyond preventing leaks
  • Public documentation
    • Documentation not necessary because: internal implementation fix with no user-facing API change

Open with Devin

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 17, 2026

🦋 Changeset detected

Latest commit: dc5a137

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

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Apr 17, 2026
@workers-devprod workers-devprod requested review from a team and jamesopstad and removed request for a team April 17, 2026 15:22
@workers-devprod
Copy link
Copy Markdown
Contributor

workers-devprod commented Apr 17, 2026

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 17, 2026

Changeset Review

File Reviewed

  • .changeset/miniflare-dispose-cleanup-followup.md

Summary

✅ All changesets look good

Details

The changeset for miniflare is well-written:

  • Version Type: patch is correctly chosen for bug fixes addressing resource leaks
  • Description Quality: Clear title and detailed explanation of two specific follow-up fixes
    • Dev-registry dispatcher now only closes when port actually changes
    • InspectorProxy instances are now properly disposed during updates
  • No Headers: Uses plain text and bullet points (no h1/h2/h3)
  • Context: References PR [miniflare] Close all open handles on dispose to prevent process hangs #13515 for additional context
  • Technical Detail: Explains what was broken and how it was fixed without being overly verbose

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 17, 2026

I've now reviewed all the changed files thoroughly. Let me analyze the changes:

Change 1: packages/miniflare/src/index.ts - The #devRegistryDispatcher is now only closed/recreated when devRegistryPort !== this.#devRegistryPort. A new #devRegistryPort field tracks the previous port value. This mirrors how #runtimeDispatcher is handled (only recreated when #runtimeEntryURL changes, line 2411).

Change 2: packages/miniflare/src/plugins/core/inspector-proxy/inspector-proxy-controller.ts - Old InspectorProxy instances are disposed before being replaced in updateConnection(). The dispose() method (line 163-168 in inspector-proxy.ts) clears the keepalive interval and closes both the devtools and runtime WebSocket connections.

Both changes are correct, straightforward, and well-scoped. The changeset is appropriate. I don't see any logic bugs, race conditions, or backward compatibility issues:

  1. For the dispatcher change, the initial call works correctly because #devRegistryPort starts as undefined, and if devRegistryPort from maybeSocketPorts.get(SOCKET_DEV_REGISTRY) is also undefined (no port), the comparison undefined !== undefined is false, so we correctly skip recreation. If it's a real port, realPort !== undefined is true, so we correctly create the dispatcher.

  2. For the inspector proxy change, the dispose() on InspectorProxy is safe to call (just clears an interval and closes WebSockets), and the await ensures cleanup completes before new proxies are created.

LGTM

github run

Copy link
Copy Markdown
Contributor

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

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 3 additional findings.

Open in Devin Review

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 17, 2026

create-cloudflare

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-editor-shared

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

wrangler

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

commit: dc5a137

- Only close/recreate devRegistryDispatcher when the port changes,
  matching the existing runtimeDispatcher pattern (fixes #13584)
- Dispose old InspectorProxy instances before replacing them in
  updateConnection, preventing leaked WebSockets and keepalive
  timers (fixes #13585)
@petebacondarwin petebacondarwin force-pushed the fix/miniflare-dispose-cleanup-followup branch from 0164dfa to dc5a137 Compare April 17, 2026 19:19
Copy link
Copy Markdown
Contributor

@workers-devprod workers-devprod left a comment

Choose a reason for hiding this comment

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

Codeowners reviews satisfied

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Apr 20, 2026
@petebacondarwin petebacondarwin merged commit be5e6a0 into main Apr 20, 2026
63 of 64 checks passed
@petebacondarwin petebacondarwin deleted the fix/miniflare-dispose-cleanup-followup branch April 20, 2026 11:58
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Apr 20, 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.

devRegistryDispatcher is unconditionally closed on every config update, unlike runtimeDispatcher

3 participants