[wrangler] fix: report non-zero exit code when killed by a signal (e.g. OOM)#14598
Open
matingathani wants to merge 1 commit into
Open
[wrangler] fix: report non-zero exit code when killed by a signal (e.g. OOM)#14598matingathani wants to merge 1 commit into
matingathani wants to merge 1 commit into
Conversation
…g. OOM)
bin/wrangler.js's spawn().on("exit", ...) handler ignored the signal
argument entirely, treating a signal-killed child (code === null) the
same as a clean exit and calling process.exit(0). This made CI pipelines
believe wrangler succeeded when it was actually OOM-killed or otherwise
terminated by a signal. Now the signal is logged and the shim exits 1.
Fixes cloudflare#14584
🦋 Changeset detectedLatest commit: b84853f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
workers-devprod
requested review from
a team and
emily-shen
and removed request for
a team
Contributor
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes Wrangler’s wrapper executable (packages/wrangler/bin/wrangler.js) incorrectly reporting exit code 0 when the spawned CLI process is terminated by a signal (e.g. SIGKILL from OOM), which can cause CI/pipelines to treat failed runs as successful.
Changes:
- Add
resolveExitCode(code, signal)to treat signal-terminated child exits as failures and optionally log the terminating signal. - Update the child process
"exit"handler to useresolveExitCode()instead of treatingnullexit codes as success. - Add a unit test that directly exercises
resolveExitCode()without needing to spawn the full built CLI, plus a changeset entry for the patch release.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/wrangler/bin/wrangler.js | Adds resolveExitCode() and uses it to avoid reporting success when the child is killed by a signal. |
| packages/wrangler/src/tests/bin-wrangler-exit-code.test.ts | Unit tests for resolveExitCode() including signal-kill scenarios. |
| .changeset/fix-wrangler-signal-exit-code.md | Changeset documenting the behavioral fix and release impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@cloudflare/autoconfig
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: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14584
bin/wrangler.js(the actualwranglerexecutable) spawns the real CLI (wrangler-dist/cli.js) as a child process and forwards its exit code:Node's
exitevent fires withcode = nulland a populatedsignalwhen a child is killed by a signal instead of exiting normally. The handler above didn't even capturesignal, and treatedcode === nullas success — so any signal-killedwranglerprocess (OOM/SIGKILL as reported here, but also e.g. SIGTERM from an orchestrator timeout) silently reported exit code 0 to whatever invoked it, making CI pipelines believe the command succeeded. This applies to everywranglercommand, not justpages deploy.Root cause was found by the issue reporter (@QuentinFarizon) in the comments, including a proposed fix pattern.
Fix: capture
signal, and only treat the exit as clean when a real numericcodecame through. Otherwise log the signal to stderr and exit 1.To make this testable without spawning the full built CLI or relying on cross-platform signal delivery (SIGKILL semantics differ on Windows, which this repo's CI matrix covers), the exit-code decision is extracted into a small exported
resolveExitCode(code, signal)function that's unit tested directly. The file's actual CLI-spawning side effect is already guarded behindif (module === require.main), so requiring it from a test doesn't trigger a spawn.