Skip to content

Fix: exit non-zero when adk deploy fails - #455

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/deploy-cloud-run-exit-code
Open

Fix: exit non-zero when adk deploy fails#455
AmaadMartin wants to merge 3 commits into
mainfrom
fix/deploy-cloud-run-exit-code

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A — no public issue number exists for this report.
  2. Or, if no issue exists, describe the change:
    Problem: adk deploy exits with status 0 even when the deployment fails, so no
    shell script or CI job can gate on the exit code. There are two independent swallow
    points:
  • deployToCloudRun() (dev/src/cli/deploy/cli_deploy_cloud_run.ts:214) catches, prints
    the red Failed to deploy to Cloud Run: banner, and falls through to finally without
    rethrowing — so the promise resolves on failure. Compare
    deployToAgentEngine() (dev/src/cli/deploy/cli_deploy_agent_engine.ts:209), which is
    structurally identical but ends its catch with throw e;.
  • The CLI action handlers catch, logger.error(...), and return normally
    (dev/src/cli/cli.ts:461 for cloud_run, dev/src/cli/cli.ts:514 inside
    registerAgentEngineCommand). The action's promise resolves and Node exits 0.

agent_engine / reasoning_engine are affected too, not just cloud_run: even though
deployToAgentEngine already rethrows, the rethrow is caught and dropped one frame up in
registerAgentEngineCommand's handler. Fixing only cloud_run would leave the other two
deploy subcommands silently reporting success. All three deploy subcommands are in
scope.

Solution: three source lines.

  1. dev/src/cli/deploy/cli_deploy_cloud_run.ts — append throw e; to the existing catch,
    keeping the banner and the finally block untouched. finally still runs to
    completion (temp folder removed, agentLoader.disposeAll() awaited) before the
    rethrown error propagates, and the original error object is rethrown — not wrapped,
    not re-messaged.
  2. dev/src/cli/cli.tsprocess.exitCode = 1; in the deploy cloud_run action catch.
  3. dev/src/cli/cli.ts — the same one-line addition in the registerAgentEngineCommand
    action catch (covers both agent_engine and reasoning_engine).

Intentional, user-visible behaviour change

adk deploy cloud_run|agent_engine|reasoning_engine changes from "always exit 0" to
"exit 1 on failure". A script that (incorrectly) relied on the deploy command always
succeeding will now fail — that is the point of the fix. No public API changes:
deployToCloudRun is not exported from dev/src/index.ts (whose only exports are
AdkApiClient and AdkApiServer) and its only non-test caller is dev/src/cli/cli.ts,
so no library consumer can observe the rethrow. No signature, option-shape, or dependency
changes.

Why process.exitCode = 1 and not process.exit(1)

web (cli.ts:255) and api_server (cli.ts:300) signal failure with process.exit(1).
This change deliberately does not follow that precedent, and leaves those two handlers
alone
:

  1. process.exit() terminates immediately and truncates pending async writes. Both
    console.error to a pipe (exactly the CI case this bug is about) and the winston
    console transport behind AdkLogger flush asynchronously, so a hard exit risks
    discarding the very error message the user needs.
  2. Nothing keeps the event loop alive after a deploy returns — the temp-folder removal and
    disposeAll() are already awaited in finally — so setting the code and letting Node
    exit naturally is sufficient and strictly safer. web/api_server differ: a
    partially-started server holds handles open, so they need the hard exit.
  3. cli_test.ts drives these action handlers in-process via program.parseAsync, so
    process.exit(1) would kill the vitest worker. Testing it would require stubbing
    process.exit into a no-op, at which point the handler under test no longer behaves
    like production. process.exitCode is directly assertable.

The handler must keep catching rather than simply letting the rejection escape:
dev/src/cli_entrypoint.ts calls the synchronous createProgram().parse(...) inside
a try/catch, which can never observe an async action-handler rejection. Removing the
catch would produce an unhandled promise rejection with a raw stack trace instead of the
human-readable message.

Duplicated error message on the cloud_run failure path

The message is now printed twice on a failed cloud_run deploy: once as the red
Failed to deploy to Cloud Run: banner from deployToCloudRun, and once as
Error deploying agent: <msg> from the CLI handler. This is not new behaviour being
invented — it is exactly what the agent_engine path already does today, and keeping the
banner is a requirement of the fix. Neither message was deleted.

Collision check against sibling PRs

Ran gh pr list --state open --limit 300 and diffed the file lists of every open PR whose
title mentioned deploy or exit. Eight PRs are adjacent (#447, #448, #449, #354, #363,
#307, #304, #286, #279) and four of them touch the same files (dev/src/cli/cli.ts,
dev/src/cli/deploy/cli_deploy_cloud_run.ts, dev/test/cli/cli_test.ts,
dev/test/cli/cli_deploy_cloud_run_test.ts). gh pr diff <n> | grep -E '^\+.*(exitCode|throw e;|process\.exit)' returns nothing for all of them: none changes
the deploy exit-code behaviour, so they overlap textually but not semantically. This PR is
branched from main rather than stacked; expect only trivial merge conflicts in the test
files.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

npx vitest run --project unit:dev dev/test/cli/cli_deploy_cloud_run_test.ts dev/test/cli/cli_test.ts
#  Test Files  2 passed (2)
#       Tests  46 passed (46)

Existing tests whose assertions were flipped (commit
Test: flip the three deployToCloudRun tests that encoded the swallowed error, kept
separate from the source fix). These three encoded the bug — two were already named
should throw error ... while their bodies awaited a resolution — so they had to change:

Test Change
should throw error if package.json has no dependencies await deployToCloudRun(...)await expect(...).rejects.toThrow(/No dependencies found in package.json/); existing consoleErrorSpy banner assertion kept
should throw error if required npm packages are missing in package.json same, with .rejects.toThrow(/Package "@google\/adk" is required but not found/); banner assertion kept
should handle spawn failuresrenamed should reject when the gcloud deploy spawn fails .rejects.toThrow(/Command failed with exit code 1/); banner assertion kept. "handle" was the euphemism for "swallow"

No test was deleted, skipped, or weakened. Every other test in both files is untouched —
in particular the success-path tests and the pre-try throws (Project is not specified, Region is not specified, and the it.each --set-env-vars /
--remove-env-vars conflict cases), whose continued passing is the regression signal that
the rethrow did not change the happy path.

New tests added (commit Test: cover the deploy failure exit code and the failure-path cleanup):

  • dev/test/cli/cli_deploy_cloud_run_test.tsshould still clean up temporary files when the deploy fails: spawn closes with code 1, and the test asserts the call rejects
    and that fs.rm('/tmp/test-deploy', {recursive: true, force: true}) and the
    AgentLoader disposeAll spy were both called. Pins the invariant that the rethrow
    does not skip finally.
  • dev/test/cli/cli_test.tscommand: deploy cloud_run > should set a non-zero exit code when the deploy fails. Reaching the assertion after await parse(...) also proves
    the rejection does not escape the handler as an unhandled promise rejection.
  • dev/test/cli/cli_test.tscommand: deploy cloud_run > should leave the exit code untouched on a successful deploy (asserts against the saved original, not
    toBeUndefined()).
  • dev/test/cli/cli_test.tscommand: deploy agent_engine > should set a non-zero exit code when the deploy fails.

describe('CLI Entrypoint') now saves process.exitCode in beforeEach and restores it
in afterEach. process.exitCode is global process state: a test that leaves it at 1
would make the entire vitest run report failure even though every test passed.

New-line coverage: 100%. Per-line hit counts from
vitest run --coverage --coverage.include='dev/src/cli/cli.ts' --coverage.include='dev/src/cli/deploy/cli_deploy_cloud_run.ts' over the two test files:
cli_deploy_cloud_run.ts:220 (throw e;) 4 hits, cli.ts:463 1 hit, cli.ts:517 1 hit.
No new branches were introduced. The residual file-level gaps (cli.ts 96.15% lines) are
pre-existing lines these two test files never reach.

Mutation testing — every test was proven able to fail. Each source line was reverted
in turn, the targeted suite re-run, and the mutation then reverted:

Mutation Result
Delete throw e; from cli_deploy_cloud_run.ts 4 failed | 15 passed. should throw error if package.json has no dependencies, should throw error if required npm packages are missing in package.json, should reject when the gcloud deploy spawn fails, should still clean up temporary files when the deploy fails — all with AssertionError: promise resolved "undefined" instead of rejecting
Delete process.exitCode = 1; from the cloud_run handler 1 failed | 26 passed. deploy cloud_run > should set a non-zero exit code when the deploy failsAssertionError: expected undefined to be 1 // Object.is equality. The agent_engine test still passed
Delete process.exitCode = 1; from the registerAgentEngineCommand handler 1 failed | 26 passed. deploy agent_engine > should set a non-zero exit code when the deploy failsexpected undefined to be 1. The cloud_run test still passed, proving the two handlers are covered independently rather than one test masking both
Move process.exitCode = 1; out of the cloud_run catch so it runs unconditionally 1 failed | 26 passed. should leave the exit code untouched on a successful deployexpected 1 to be undefined. Proves the negative assertion has teeth

Other local validation on the pushed commit:

npm run lint          # eslint "**/*.ts" — clean
npm run format:check  # prettier --check — "All matched files use Prettier code style!"
npx tsc --noEmit      # 286 errors, all pre-existing; identical count with the four
                      # touched files reverted to main, and none of them are in
                      # dev/src/cli/** or dev/test/cli/**
npm run build --workspace=core --workspace=dev   # both succeed

Integration tests: none added. This defect is entirely in the CLI's error plumbing, and
the integration vitest project does not exercise adk deploy (it would need real GCP
credentials).

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

No cloud access needed — any pre-gcloud failure inside the try reproduces it.

npm run build --workspace=core --workspace=dev
mkdir -p /tmp/nodeps-agent && echo '{"name":"nodeps-agent"}' > /tmp/nodeps-agent/package.json
cd dev && node dist/esm/cli_entrypoint.js deploy cloud_run /tmp/nodeps-agent \
  --project some-project --region us-central1
echo "exit=$?"

Before this change (same command, the two source files reverted to main):

Starting deployment to Cloud Run...
Copying agent source files...
Creating package.json...
Failed to deploy to Cloud Run: No dependencies found in package.json: /tmp/nodeps-agent/package.json
Cleaning up temporary files...
Temporary files cleaned up.
BEFORE-FIX exit=0

After this change:

Starting deployment to Cloud Run...
Copying agent source files...
Creating package.json...
Failed to deploy to Cloud Run: No dependencies found in package.json: /tmp/nodeps-agent/package.json
Cleaning up temporary files...
Temporary files cleaned up.
[ADK CLI] Error deploying agent: No dependencies found in package.json: /tmp/nodeps-agent/package.json
pipeline-exit=1

Message is not truncated when stderr is a pipe (the process.exitCode rationale) —
same command with 2>&1 | cat; both the banner and the Error deploying agent: line are
fully visible and the exit status is still 1:

Starting deployment to Cloud Run...
Copying agent source files...
Creating package.json...
Failed to deploy to Cloud Run: Package "@google/adk" is required but not found in package.json: /tmp/bad-agent/package.json
Cleaning up temporary files...
Temporary files cleaned up.
[ADK CLI] Error deploying agent: Package "@google/adk" is required but not found in package.json: /tmp/bad-agent/package.json
pipeline-exit=1

agent_engine also exits 1:

$ node dist/esm/cli_entrypoint.js deploy agent_engine /tmp/nodeps-agent \
    --project some-project --region us-central1
[ADK CLI] Error deploying agent: Artifact Registry repository is not specified.
Please create a Docker repository in Artifact Registry first and specify it using the --repository flag.
...
pipeline-exit=1

A successful command still exits 0node dist/esm/cli_entrypoint.js --version prints
1.5.0 and exit=0. The Cleaning up temporary files... / Temporary files cleaned up.
pair in every failure transcript above confirms the finally block still runs before the
error propagates, so the temp directory is not leaked.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 1, 2026 05:50
deployToCloudRun printed the red "Failed to deploy to Cloud Run" banner and
then let its promise resolve, and all three deploy action handlers caught the
error, logged it and returned normally. The process therefore exited 0 on a
failed deploy, so CI pipelines and shell scripts that gate on the exit status
could never detect a broken deployment.

Rethrow the original error from deployToCloudRun (mirroring deployToAgentEngine,
whose finally-block cleanup still runs first) and set process.exitCode = 1 in
the cloud_run and agent_engine/reasoning_engine handlers. process.exitCode
rather than process.exit(1) so pending async writes from console.error and the
winston console transport are flushed before Node exits.
…d error

These three tests asserted only that the red banner was printed and let the
resolved promise pass, which is exactly the bug: two of them were already named
"should throw error ..." while their bodies awaited a resolution. They now
assert the rejection alongside the existing banner assertion, and the spawn-
failure test is renamed from "should handle spawn failures" -- "handle" was the
euphemism for "swallow".
Adds a deployToCloudRun test pinning that the rethrow does not skip the finally
block (temp folder removed, agentLoader.disposeAll() awaited), and three CLI
tests asserting process.exitCode is 1 after a failed cloud_run or agent_engine
deploy and untouched after a successful one.

The CLI suite now saves and restores process.exitCode around every test, since
leaking a 1 would fail the entire vitest run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant