[workers-auth] Spawn cloudflared asynchronously so ctrl+c can interrupt Access authorization - #14838
Conversation
…pt Access authorization
🦋 Changeset detectedLatest commit: 40df592 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
edmundhung
left a comment
edmundhung
left a comment
There was a problem hiding this comment.
Thanks for the fix!
I’m concerned that relying on the exit hook may be too late, since the running cloudflared process could itself prevent Wrangler from exiting. Could we pass the abort signal from the runtime controller through getAccessHeaders() to spawn() instead?
Relying on the process exit hook alone can be too late, since a running cloudflared child can itself keep wrangler from exiting. Thread an optional AbortSignal from RemoteRuntimeController through getAccessHeaders() into spawn(), so tearing down the remote session kills a still-pending authorization immediately and surfaces an AbortError, which the controller already ignores. Both the child's error and close events settle the promise as aborted, since their order after an abort kill is not guaranteed. The exit hook remains as last-resort cleanup for callers without a signal.
|
Good point, done in 6e47e5a. getAccessHeaders() now takes an optional signal and RemoteRuntimeController passes its own abort signal through to spawn(), so tearing down the session kills a pending cloudflared immediately instead of waiting for process exit. The abort surfaces as an AbortError, which the controller's existing catch already ignores. Two details from implementing it: the promise treats both the child's error and close events as abort-aware, because after a signal kill their order isn't guaranteed and the close path would otherwise resolve with empty stdout. And I kept the exit hook as a last resort for call paths that don't have a signal, like the direct preview flow. Happy to drop it if you'd rather rely on the signal alone. Tests cover the abort (child killed, AbortError propagated) and the workers-auth suite plus wrangler typecheck are green. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@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: |
3 participants
Fixes #12900.
When a domain is behind Cloudflare Access (e.g. during remote bindings startup), wrangler invokes
cloudflared access login <domain>, which only exits once the user completes the authorization flow in the browser.getAccessHeaders()ran this viaspawnSync, blocking Node's event loop for the entire wait. While blocked, no SIGINT listener and no raw-mode keypress handler can run, so ctrl+c does nothing — abandoning or accidentally closing the browser tab leaves a hung wrangler that must be killed from another terminal.This PR spawns
cloudflaredasynchronously (preserving the stdout token-parsing contract and the spawn-failure → "please installcloudflared" error), keeping wrangler interruptible during the wait. Aprocessexit hook kills a still-pendingcloudflaredso it doesn't outlive wrangler.Notes:
getAccessHeaders()calls for the same domain could already race past the header cache and invokecloudflaredmore than once (the awaited Access-detection fetch yields between the cache check and the spawn). This PR doesn't change that; in-flight promise dedup would be a natural follow-up if it bites in practice.cloudflaredinstalled; the tests modelcloudflaredwith a fake child process, including a regression test that fails if the event loop is ever blocked again mid-authorization (it asserts a timer can fire while the login is pending).