Skip to content

Fix: report the reason and exit cleanly when adk web / api_server fail to start - #904

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/cli-server-startup-exit-code
Open

Fix: report the reason and exit cleanly when adk web / api_server fail to start#904
AmaadMartin wants to merge 4 commits into
mainfrom
fix/cli-server-startup-exit-code

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 11, 2026

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

  2. Or, if no issue exists, describe the change:

Problem: adk web and adk api_server log a start-up failure and then call process.exit(1) in the same tick. The winston Console transport writes on a later tick, so the reason is dropped whenever output already sits queued in the pipe. The catch block also casts with (error as Error).message, so a non-Error rejection prints undefined.

Solution: The failure path now sets process.exitCode = 1 and lets the process end on its own, which lets the pending write drain. It also calls AdkApiServer.stop(), because start() can reject after listen() has bound the socket and the listening handle would otherwise keep the process alive forever. stop() now disposes the agent loader too, so --reload_agents cannot strand its file watcher; it disposes only a loader the server built for itself, because an injected loader belongs to the caller.

Collision check: gh pr list --repo AmaadMartin/adk-js --state open --limit 100, then gh pr diff --name-only on every open PR that touches dev/src/cli/cli.ts (#900, #870, #839, #816, #803). None touches the web / api_server catch blocks. The nearest neighbour is #839, which sets process.exitCode in the integration conformance action and adds an exit-code save/restore to the shared beforeEach; this PR keeps its save/restore local to its own describe block, so the two only conflict textually. This PR branches from main.

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_test.ts — 32 passed.
npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts — 56 passed.
npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts — 36 passed.

Six new cases in dev/test/cli/cli_test.ts cover both commands, the swallowed stop() rejection from a server that never bound, a plain-string rejection, a constructor failure with no server to stop, and the untouched success path. Three new cases in dev/test/server/adk_api_server_test.ts cover the loader disposal, the untouched injected loader, and a stop() that never reached listen(). No existing test was edited.

Coverage of the new lines is 100% statements and branches (read from the v8 coverage-final.json). Whole-file cli.ts moves from 95.29% / 73.17% to 96.63% / 82.00%; the remainder is pre-existing.

Proof the tests can fail. Each mutation was applied to dev/src/cli/cli.ts alone and reverted after the run:

Mutation Result
process.exit(1) back in place of process.exitCode = 1 5 failed — process.exit was called, and process.exit unexpectedly called with "1"
await server?.stop() deleted 2 failed — expected "spy" to be called 1 times, but got 0 times
try/catch around stop() removed 1 failed — promise rejected "Error: Server is not running." instead of resolving
the error instanceof Error test back to (error as Error).message 1 failed — expected "error" to be called with arguments: [ 'Error starting web server:', …(1) ]
disposeAll() removed from stop() 1 failed — expected "disposeAll" to be called 1 times, but got 0 times
the ownsAgentLoader guard removed 24 failed — this.agentLoader.disposeAll is not a function

One as unknown as Mock cast is added, on the module mock of AdkApiServer. It is the pattern the rest of this file already uses, and it is needed because the test double is a partial instance. The three uses share one cast.

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

A. The reason survives a pipe. With a 1 MB write already queued on stdout, process.exit(1) loses the record and process.exitCode keeps it:

$ node /tmp/trunc.mjs exit | (sleep 2; grep -c "Port 8000 is already in use")
0
$ node /tmp/trunc.mjs code | (sleep 2; grep -c "Port 8000 is already in use")
1

With an empty pipe buffer the small write reaches the reader either way, so this defect needs pending output to show itself.

B. A bind failure reports and exits.

npm run build -w core && npm run build -w dev
node -e 'require("http").createServer().listen(8000,"127.0.0.1")' &
node dev/dist/esm/cli_entrypoint.js api_server --host 127.0.0.1 ./tests/integration/a2a/basic/remote_a2a | cat
echo "exit=${PIPESTATUS[0]}"
[ADK CLI] Error starting API server: Port 8000 is already in use
exit=1

C. No hang once the socket is bound. With throw new Error("forced a2a failure") inserted at the top of initA2A(), adk web --a2a ends in about two seconds:

[ADK CLI] Error starting web server: forced a2a failure
| ADK API Server stopped                                                      |
exit=1

real	0m1.974s

Dropping only the stop() call from the same build reproduces the hang this PR avoids: the command printed the same message and was still running when a 30 second timeout killed it (exit=124, real 0m30.017s).

D. No hang with --reload_agents. The same forced failure, raised after initA2A() has listed the agents and so started the watcher, with adk web --a2a --reload_agents. Without the loader disposal the command outlived a 20 second timeout (exit=124, real 0m20.060s). With it:

[ADK CLI] Error starting web server: forced a2a failure
| ADK API Server stopped                                                      |
exit=1

real	0m3.046s

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.

Review round 1

A complexity review asked for three changes, all applied. stop() now releases the agent loader, which removes the --reload_agents hang the first revision documented as a follow-up. The try/catch around stop() became .catch(() => {}), and describeError was inlined at its single call site.

CI

run-tests (unit) passes, and dev/test/cli/cli_test.ts reports 32 passed on every runner. The run-tests (ubuntu-latest) job fails on 7 pre-existing tests/integration/workflows/* sample tests. The same 7 failures appear on unrelated PR #903, so this branch does not cause them. The macOS and Windows jobs were cancelled by the matrix fail-fast, not by a failure of their own.

Amaad Martin added 4 commits August 10, 2026 19:35
adk web and adk api_server called process.exit(1) after logging the
failure, which terminates before the winston Console transport drains a
queued write, so the operator loses the reason on a pipe. Setting
process.exitCode instead needs the listening socket released, because
start() can reject after listen() has bound it.

Also stringify a non-Error rejection, which previously rendered as
undefined through an unchecked cast.
Six cases pin the log text, the stop() teardown, the swallowed stop()
rejection, a non-Error rejection, a constructor failure and the
untouched success path.
start() can leave AgentLoader watching the agents directory, and stop()
closed only the HTTP handle, so 'adk web --a2a --reload_agents' stayed
alive after a start-up failure printed its reason. stop() now disposes
the loader, but only one the server built for itself: an injected loader
belongs to the caller.
… each

Both had a single caller. stop() always returns a promise, so the
rejection needs .catch(), not a try/catch block.
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