[workers-utils] Stop the update check from recommending deprecated versions - #15617
[workers-utils] Stop the update check from recommending deprecated versions#15617jpatel3 wants to merge 2 commits into
Conversation
…rsions The "update available" notice always pointed at whichever version was tagged `latest` on npm, even after that version had been deprecated for shipping a bug. The `update-check` package fetched the full packument, which carries the per-version `deprecated` field, but discarded it. Query the npm registry directly instead: if the tagged version has been deprecated, recommend the newest non-deprecated stable release below it, or nothing at all. The on-disk cache location, file format and one-hour refresh interval are unchanged so existing caches keep working. Fixes cloudflare#9154
🦋 Changeset detectedLatest commit: acd4bc1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| latest = await Promise.race([ | ||
| getLatestVersion(name, distTag), | ||
| timersPromises.setTimeout(UPDATE_CHECK_TIMEOUT_MS, TIMED_OUT, { | ||
| ref: false, | ||
| }), |
There was a problem hiding this comment.
🟡 Timed-out checks continue in background
After a slow cache read, Promise.race returns failure while getLatestVersion continues and starts the registry request. Timed-out checks still generate registry traffic and can delay process exit.
Learn more
The timeout promise only stops waiting for getLatestVersion; it does not cancel that operation. A cache read lasting beyond three seconds therefore produces a failed result, but the pending operation resumes when the read completes and calls fetchPackument. That request gets a fresh three-second timeout because its AbortSignal.timeout() starts only when the request begins. The operation can then also write the cache after the public function has returned.
Example: A cache read takes 3.1 seconds. The caller receives failed at 3 seconds. At 3.1 seconds, getLatestVersion starts an npm request that can run until 6.1 seconds, despite the stated three-second overall budget.
Recommended fix: Create one abort signal when fetchLatestNpmVersion starts and pass it through getLatestVersion, readCache, fetchPackument, and cache writes. Use that signal for supported filesystem calls and fetch, and avoid starting later stages once it is aborted.
Was this helpful? React with 👍 or 👎 to provide feedback.
| * - `{ status: "up-to-date" }` if the installed version is already the latest | ||
| * - `{ status: "failed" }` if the check could not be completed (network error, timeout, etc.) | ||
| */ | ||
| export async function fetchLatestNpmVersion( |
There was a problem hiding this comment.
mh... I am wondering if we should rename this function, to something like fetchLatestValidNpmVersion or fetchLatestActiveNpmVersion 🤔
There was a problem hiding this comment.
I'd lean towards keeping it: the export already existed before this PR and is used by wrangler and the vite plugin, and the docblock covers the deprecation behaviour. Not against renaming if you feel strongly about it though.
| /** | ||
| * Pick the version to recommend from a packument. | ||
| * | ||
| * @returns The version behind `distTag`, unless it has been deprecated, in |
There was a problem hiding this comment.
could you also add @params? 🙏
|
|
||
| async function fetchPackument(name: string): Promise<AbbreviatedPackument> { | ||
| const packageUrl = new URL( | ||
| encodeURIComponent(name).replace(/^%40/, "@"), |
There was a problem hiding this comment.
It mirrored how npm itself encodes scoped names in registry URLs (@scope%2Fname), but the registry accepts the fully encoded form too, so it wasn't needed. Dropped it in acd4bc1.
|
Btw, sorry, besides my comments above this looks good to me, thanks for this fix @jpatel3 🙏 🫶 |
4bba64e to
acd4bc1
Compare
3 participants
Fixes #9154.
When a Wrangler release is deprecated on npm (e.g.
4.14.2was deprecated for awrangler devregression), users on an older version still saw(update available 4.14.2)in the banner, nudging them towards a known-bad release.The
update-checkpackage we used fetched the full abbreviated packument, which includes the per-versiondeprecatedfield, but only kept thedist-tagsversion. Fixing this on top of the library would have required a second registry request whenever an update was found, and the banner only waits ~100ms for the (normally cached) result, so that would have hidden the notice for everyone who was behind.Instead
fetchLatestNpmVersionin@cloudflare/workers-utilsnow queries the registry itself (via undici, which the package already depends on) and theupdate-checkdependency is removed:latest, orbetafor0.0.0-*builds) is not deprecated, behaviour is unchanged.latest(e.g. anexttag) are never picked. If nothing suitable exists, the check reports up-to-date.<tmpdir>/update-check/<name>-<tag>.json), file format and one-hour TTL, so caches written by the previous implementation continue to be honoured. Cache write failures no longer turn a successful check into a failure.One deliberate simplification to flag for reviewers:
update-checkresolved the registry URL from.npmrcand retried 4xx responses with an auth token. This implementation always useshttps://registry.npmjs.org/, since both packages that use this check (wranglerand@cloudflare/vite-plugin) are only published there. Users whose network blocks the public registry will get the existing silentfailedresult. Happy to addnpm_config_registrysupport if you'd prefer to keep that behaviour.wrangler's own tests are unaffected because they mockupdateCheckat the wrangler layer.