[petebacondarwin]

Two instances of the same class of bug found while auditing the codebase after #15206, which fixed a partially-downloaded Chrome install in miniflare. The shared principle: the presence of files is not evidence of a working install, and a delivered signal is not evidence of an exited process.

workers-auth: a half-installed keyring binding was trusted forever

On Windows, opting into keyring-backed credential storage lazily npm installs @napi-rs/keyring. findKeyringBinding() accepted any candidate whose index.js existed:

if (existsSync(path.join(lazy, "index.js"))) {
	cachedBindingPathByDir.set(installDir, lazy);
	return lazy;
}

That index.js requires a platform-specific .node binary which npm delivers as a separate package. An install that is interrupted, runs out of disk, or fails to fetch the platform package leaves index.js on disk with nothing loadable beside it.

The consequences compounded:

1. resolveKeyProvider() saw a non-null result and returned kind: "available", so the needs-install branch was never taken and the install was never retried.
2. Every credential read and write then threw a raw module-resolution error from createRequire(...) in resolveKeyringEntryFactory() — no remediation hint, no fallback.
3. The verdict is memoised in cachedBindingPathByDir, which is only invalidated by a successful install or setNpmRunner(). Since step 1 means no install is ever attempted, the breakage was permanent for that config dir.

A candidate is now only accepted once it has actually been required — precisely what the credential path does with it moments later, so this costs nothing extra (require caches on success) and only ever runs on the Windows opt-in path. A half-installed binding is reported absent, so the resolver reinstalls it and it then works.

Bounding the retry

Reporting the wreck as absent fixes the permanent breakage but, as Devin spotted, leaves the retry unbounded in one case. npm exits 0 when an optional dependency cannot be fetched, and the platform-specific .node binary is exactly that — so on a machine where it can never be fetched (no network route to the platform package, or an npm configured to skip optional dependencies), the install "succeeds" while producing nothing loadable.

handleNeedsInstall() only latches installFailedThisSession when the install throws, so nothing latched, and because the resolver re-resolves on every credential operation it took the needs-install branch again on the very next read or write. The user would sit through a fresh multi-second install before every single credential access, each one still ending in an internal error.

The install now verifies its own work: after npm exits 0 it checks the binding actually loads, and throws a UserError when it does not. That routes the failure through the existing remediation path — one attempt per session, a single explanation, then a fallback to the plaintext credentials file, or a hard error when CLOUDFLARE_AUTH_USE_KEYRING=true demanded the keyring. A transient failure is still retried on the next invocation, since the latch is per-process.

resolveKeyringEntryFactory() also no longer throws a bare Error when the binding is unavailable. With resolution now verifying loadability that is close to an internal invariant violation, but it is still the user's machine that has to be fixed, so it carries the same remediation hint.

mock-npm-registry: stop() resolved before the process had exited

treeKill(server.pid, () => res());

treeKill's callback fires when the signals have been delivered, not when the process tree has finished exiting. Callers removeDir() the registry's storage directory immediately afterwards and may restart on the same port, both of which race a Verdaccio that is still shutting down — ENOTEMPTY/EBUSY on directory removal (notably on Windows, where an open file blocks deletion) and EADDRINUSE on restart.

It now waits for the real exit event, bounded by a 10s timeout so a wedged process cannot hang test teardown. The listener is attached at fork() time rather than inside stop(), so the event cannot be missed. packages/wrangler/e2e/helpers/command.ts already used this pattern correctly; this brings the registry helper in line.

Testing notes

The workers-auth change has six new tests, each mutation-verified — removing the corresponding guard fails exactly the intended test and nothing else:

Test	Fails when
treats a binding whose native binary is missing as absent	the lazy-dir load check is removed
ignores a global binding that cannot be loaded	the global-root load check is removed
recovers when a reinstall repairs a half-installed binding	the lazy-dir load check is removed
throws a UserError when npm exits 0 without leaving a loadable binding	the post-install verification is removed
stops retrying when npm exits 0 but the binding is still unloadable	the post-install verification is removed (expected 3 to be 1 — three credential operations, three installs)
hard-errors when CLOUDFLARE_AUTH_USE_KEYRING=true and the install lands no loadable binding	the post-install verification is removed
throws a UserError with remediation when no binding is available	resolveKeyringEntryFactory() reverts to a bare Error

The fixtures reproduce the real failure exactly, with an index.js containing require("./keyring.win32-x64-msvc.node") and no such binary. Two pre-existing tests whose fake npm runner exited 0 without writing anything now have to land a loadable binding, since the install verifies itself.

I did not add a test for the mock-npm-registry change, and want to be upfront about why: I wrote one, and it passed against the unfixed code too. Verdaccio exits fast enough after being signalled on macOS that the port is already free by the time the assertion runs, so the test proved nothing. The race only bites where an exiting process still holds its files or the port is reclaimed before the listener is released, which I cannot reproduce on this platform. Rather than commit a test that would give false confidence, I've left it out. That code path is exercised (though not asserted on) by the create-cloudflare, vitest-pool-workers, and vite-plugin-cloudflare e2e global setups.

pnpm check passes: 0 lint errors, all typecheck tasks green, workers-auth 16 test files / 173 tests passing, plus wrangler's 93 user.test.ts tests.

---

• 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: both changes fix internal failure-recovery behaviour with no change to any documented interface. The keyring install remains transparent to the user; it now repairs itself, and when it cannot, it degrades gracefully instead of failing permanently.

Note

This is a contribution from an AI agent: OpenCode, claude-opus-5.

[changeset-bot]

🦋 Changeset detected

Latest commit: 7f09943

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

This PR includes changesets to release 2 packages

Name	Type
@cloudflare/workers-auth	Patch
@cloudflare/remote-bindings	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

[workers-devprod]

Codeowners approval required for this PR:

• ✅ @cloudflare/wrangler

Show detailed file reviewers

[github-actions]

✅ All changesets look good

[ask-bonk]

Now I have a solid understanding. Let me consider one important concern about require caching and the recovery test.

Critical concern — require cache poisoning across recovery:

In canLoadKeyringBinding, when a half-installed binding fails to load, require throws. But when the repaired binding is later loaded, require should succeed. The recovery test writes a valid module and expects it to work.

But there's a subtle real-world risk that the test doesn't catch: the createRequire(candidate)(candidate) call. When the FIRST load fails (missing .node), Node's module cache does NOT cache failed requires — so retry after reinstall works. The test confirms this. Good.

However — one genuinely important issue: the recovery test rewrites index.js in place to a valid module (module.exports = {}), same path. In the real recovery flow, installKeyringBindingSync clears the cache entry (line 258), and a fresh findKeyringBinding re-probes. But Node's require cache is keyed by resolved filename. If the first (failed) probe had partially cached anything... it doesn't, since it threw. So recovery works. The test validates this. Fine.

Let me verify there isn't an issue with the memoization interaction — specifically whether a successful canLoadKeyringBinding in the global-root branch could be reached before checking the lazy branch's existsSync short-circuit. The logic is sound.

Let me double-check one thing: the error/disconnect reject during shutdown in mock-npm-registry — whether disconnect firing during a normal stop could cause an unhandled rejection. The listeners were removed at line 312-313 when the server started successfully, so they won't fire during stop(). Correct.

Both changes are well-reasoned, correct, and thoroughly documented. The tests are appropriate. Let me verify there are no edge cases with the treeKill on an already-exited pid or the timer.

The stop() function: treeKill callback resolves regardless of error (intended). Then races exited against a 10s timeout. clearTimeout in finally. All correct. If exited already resolved (process died before stop called), the race resolves immediately. Good.

I've reviewed both changes carefully:

1. workers-auth canLoadKeyringBinding: Correctly probes with a real require, consistent with resolveKeyringEntryFactory. Failed requires aren't cached by Node, so recovery works. Cache invalidation on install is correct. The three tests are mutation-verified.

2. mock-npm-registry stop(): The exited promise is attached at fork time (can't miss the event), the error/disconnect reject listeners are removed on successful start so they can't fire during shutdown, the timeout is unref'd and cleared, and the race is bounded. All sound.

3. Changeset: Correct package/patch level.

No logic bugs, security issues, backward-compat violations, or incorrect API behavior found. The author was appropriately transparent about the untested mock-npm-registry path, and the reasoning is valid.

LGTM

github run

[pkg-pr-new]

@cloudflare/autoconfig

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

@cloudflare/build-output-utils

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

@cloudflare/config

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

create-cloudflare

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

@cloudflare/deploy-helpers

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-functions

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-auth

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

@cloudflare/workers-editor-shared

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

@cloudflare/workers-utils

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

wrangler

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

commit: 7f09943

[NuroDev]

Apart from a few comments from Devin that need looking at this looks good to me

[workers-devprod]

Codeowners reviews satisfied