Skip to content

Revert "Revert "[wrangler] fix: prevent remote binding sessions expiring during long dev sessions""#13470

Merged
penalosa merged 4 commits intomainfrom
revert-13464-revert-13016-penalosa/remote-bindings-timeout
Apr 16, 2026
Merged

Revert "Revert "[wrangler] fix: prevent remote binding sessions expiring during long dev sessions""#13470
penalosa merged 4 commits intomainfrom
revert-13464-revert-13016-penalosa/remote-bindings-timeout

Conversation

@penalosa
Copy link
Copy Markdown
Contributor

@penalosa penalosa commented Apr 14, 2026

Reverts #13464


Open with Devin

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 14, 2026

🦋 Changeset detected

Latest commit: e750b57

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 14, 2026
@workers-devprod workers-devprod requested review from a team and NuroDev and removed request for a team April 14, 2026 16:28
@workers-devprod
Copy link
Copy Markdown
Contributor

workers-devprod commented Apr 14, 2026

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

I've now thoroughly reviewed all the changes in this PR. The code is well-structured, the proactive refresh logic is correct, the timer cleanup is properly handled in all lifecycle events (bundleStart, teardown), the error handling changes are sound, and the lazy auth resolution is a good improvement. The tests are comprehensive and cover the key scenarios (proactive refresh, timer cancellation on bundle start, timer cancellation on teardown).

LGTM

github run

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 14, 2026

✅ All changesets look good

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 14, 2026

create-cloudflare

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-editor-shared

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

wrangler

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

commit: e750b57

@penalosa
Copy link
Copy Markdown
Contributor Author

/bonk can you fix the E2E test that is failing on this pr?

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 found 1 potential issue.

View 6 additional findings in Devin Review.

Open in Devin Review

Comment thread packages/wrangler/src/api/startDevWorker/RemoteRuntimeController.ts
The PR changed LocalRuntimeController to pass the auth hook lazily to
maybeStartOrUpdateRemoteProxySession. This caused the user's auth
callback to be called twice during initial startup: once during the
nested startWorker's resolveDevConfig zone validation (line 121), and
again in RemoteRuntimeController.#onBundleComplete when unwrapping
config.dev.auth.

Revert to eagerly resolving auth in LocalRuntimeController before
passing it, so the resolved CfAccount object flows through without
triggering additional auth calls in the nested startWorker chain.
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

E2E Test Fix

The failing E2E test e2e/start-worker-auth-opts.test.ts expected the user's auth callback (validAuth) to be called exactly once, but it was being called twice.

Root Cause

The PR changed LocalRuntimeController to pass data.config.dev.auth (the auth hook function) lazily to maybeStartOrUpdateRemoteProxySession, instead of eagerly resolving it with await unwrapHook(data.config.dev.auth).

This caused the user's auth callback to be invoked twice during initial startup of the remote proxy session:

  1. First call: In the nested startWorker (created by startRemoteProxySession with dev.remote: "minimal"), resolveDevConfig eagerly calls auth() at line 121 for zone validation when input.dev?.remote is truthy. This unwinds through the wrapper chain to call the user's validAuth.

  2. Second call: RemoteRuntimeController.#onBundleComplete calls unwrapHook(config.dev.auth), which again unwinds through the wrapper chain to call validAuth.

Previously, when the resolved CfAccount object was passed (not the function), unwrapHook on a plain object simply returned it without calling the user's auth function again.

Fix

Reverted the LocalRuntimeController change to eagerly resolve auth before passing it to maybeStartOrUpdateRemoteProxySession. This restores the original behavior where the resolved CfAccount object flows through the nested startWorker chain without triggering additional auth calls. All other improvements from the PR (proactive refresh timer, timer cleanup, error handling) are preserved.

Other Failing Tests

  • Tests (Linux, fixtures) - worker-basic ECONNREFUSED: flaky infrastructure test, unrelated to this PR
  • Check - PR description validation: missing tests/docs justification, unrelated to code

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

The fix has been committed and pushed. Here's a summary:

Problem: The E2E test e2e/start-worker-auth-opts.test.ts was failing with "expected vi.fn() to be called once, but got 2 times" because the user's auth callback was being invoked twice during initial startup.

Root cause: The PR changed LocalRuntimeController to pass the auth hook function lazily (instead of the eagerly-resolved CfAccount object) to maybeStartOrUpdateRemoteProxySession. When this hook function reached the nested startWorker created by startRemoteProxySession, it was unwrapped twice: once during resolveDevConfig zone validation and once in RemoteRuntimeController.#onBundleComplete.

Fix: Restored eager auth resolution in LocalRuntimeController.#onBundleComplete (packages/wrangler/src/api/startDevWorker/LocalRuntimeController.ts), so the resolved CfAccount object is passed through the chain without triggering additional auth calls. All other PR improvements (proactive refresh timer, timer cleanup, error handling changes) are preserved.

github run

@penalosa
Copy link
Copy Markdown
Contributor Author

/bonk that's absolutely not the right fix. eagerly resolving auth is a bug, not a solution. resolving auth twice is fine, since the second time it'll be cached and the lookup will be fast. update the e2e test assertion instead

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 found 1 new potential issue.

View 7 additional findings in Devin Review.

Open in Devin Review

Comment thread packages/wrangler/src/api/startDevWorker/RemoteRuntimeController.ts
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

UnknownError: ProviderInitError

github run

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

@penalosa Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@penalosa
Copy link
Copy Markdown
Contributor Author

/bonk retry the previous task

ask-bonk bot added 2 commits April 14, 2026 17:43
The lazy auth passing causes auth to be resolved twice during startup
(once in resolveDevConfig for zone validation, once in
RemoteRuntimeController#onBundleComplete). The second call is cached
and fast, so this is expected behavior. Update assertions from
toHaveBeenCalledOnce() to toHaveBeenCalled().
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

Done. Here's what I did:

  1. Reverted the previous wrong fix (commit 587471f) that eagerly resolved auth in LocalRuntimeController - as @penalosa noted, eager resolution is a bug, not a solution.

  2. Updated the E2E test assertions in packages/wrangler/e2e/start-worker-auth-opts.test.ts - changed 4 instances of toHaveBeenCalledOnce() to toHaveBeenCalled(). With lazy auth passing, the auth callback may be called twice during startup (once in resolveDevConfig for zone validation, once in RemoteRuntimeController#onBundleComplete), but the second call is cached and fast, so this is expected and correct behavior. The not.toHaveBeenCalled() assertion for the no-remote-bindings test was left unchanged.

github run

@penalosa penalosa added the skip-pr-description-validation Skip validation of the required PR description format label Apr 16, 2026
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 16, 2026
@penalosa penalosa merged commit 4fda685 into main Apr 16, 2026
64 of 65 checks passed
@penalosa penalosa deleted the revert-13464-revert-13016-penalosa/remote-bindings-timeout branch April 16, 2026 15:09
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-remote-tests skip-pr-description-validation Skip validation of the required PR description format

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants