Skip to content

[wrangler] Fix remote bindings hanging on shutdown#13887

Merged
dario-piotrowicz merged 2 commits into
mainfrom
fix-remote-hang
May 11, 2026
Merged

[wrangler] Fix remote bindings hanging on shutdown#13887
dario-piotrowicz merged 2 commits into
mainfrom
fix-remote-hang

Conversation

@apeacock1991
Copy link
Copy Markdown
Contributor

Fixes #11254

startDev() registers dev hotkeys before authenticating the user. During interactive dev sessions, the auth callback re-registers hotkeys, which updates the local unregisterHotKeys variable to a new cleanup function. However, the unregisterHotKeys value returned to callers was captured as a direct reference to the initial registration, so it would call the stale cleanup function instead of the current one.

This PR fixes the bug by returning a wrapper function () => unregisterHotKeys?.() instead of the variable directly. The wrapper evaluates unregisterHotKeys at call time, ensuring it always invokes the latest cleanup function even after re-registration.

A unit test is added to verify that unregisterHotKeys() correctly calls the latest cleanup after auth triggers a re-registration.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: it's a bug fix

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 11, 2026

🦋 Changeset detected

Latest commit: 5fdb554

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

This PR includes changesets to release 3 packages
Name Type
wrangler Patch
@cloudflare/vite-plugin 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-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 11, 2026

✅ All changesets look good

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 11, 2026

I've now reviewed the entire PR. Let me analyze the changes:

The fix (start-dev.ts:208): Changes unregisterHotKeys to () => unregisterHotKeys?.() — this is correct. The unregisterHotKeys variable is reassigned at line 51 inside the authHook, so the old direct reference in the return value would become stale. The wrapper closure correctly captures the variable by reference, evaluating it at call time.

The test (start-dev.test.ts): The test correctly:

  • Uses expect from test context (line 49)
  • Mocks DevEnv, registerDevHotKeys, isInteractive, and requireAuth
  • Exercises the auth re-registration path by extracting the auth hook from config.set call args and invoking it
  • Asserts that result.unregisterHotKeys() calls the second cleanup function, not the first

The changeset: Correctly scoped to wrangler as a patch.

The code change is a one-line, well-targeted fix for a real closure capture bug. The test adequately covers the scenario. The changeset is appropriate.

LGTM

github run

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 11, 2026

create-cloudflare

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-editor-shared

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

@cloudflare/workers-utils

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

wrangler

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

commit: 5fdb554

@apeacock1991 apeacock1991 marked this pull request as ready for review May 11, 2026 18:01
@workers-devprod workers-devprod requested review from a team and james-elicx and removed request for a team May 11, 2026 18:02
@workers-devprod
Copy link
Copy Markdown
Contributor

workers-devprod commented May 11, 2026

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

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

Open in Devin Review

@james-elicx james-elicx requested review from dario-piotrowicz and removed request for james-elicx May 11, 2026 18:07
Copy link
Copy Markdown
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Looks good to me! (I feel that this bug as actually caused by me! 😅) Thanks @apeacock1991 😄

Could you maybe just expand on the changeset if possible? 🙏

Comment thread .changeset/mean-brooms-hope.md Outdated
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 May 11, 2026
@dario-piotrowicz dario-piotrowicz added the run-remote-tests Run remote/E2E tests that require Cloudflare API credentials label May 11, 2026
@dario-piotrowicz dario-piotrowicz enabled auto-merge (squash) May 11, 2026 19:23
@dario-piotrowicz dario-piotrowicz merged commit d878e13 into main May 11, 2026
88 checks passed
@dario-piotrowicz dario-piotrowicz deleted the fix-remote-hang branch May 11, 2026 19:34
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-remote-tests Run remote/E2E tests that require Cloudflare API credentials

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

"Shutting down remote connection" never completes with a R2 remote binding

3 participants