Skip to content

Fix: forward async route handler rejections in the dev API server - #719

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/dev-server-unhandled-promise-rejections
Open

Fix: forward async route handler rejections in the dev API server#719
AmaadMartin wants to merge 3 commits into
mainfrom
fix/dev-server-unhandled-promise-rejections

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 6, 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

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

Problem: dev/src/server/adk_api_server.ts registers 15 async express route handlers. Express 4 discards the promise a handler returns, so a rejection raised outside the handler's own try/catch reaches nobody. /run, /run_sse and /api/reasoning_engine all have such a gap, and the raw-body branch of /api/reasoning_engine also passes an async listener to req.on('end'). The request then hangs forever and node raises unhandledRejection, which kills the dev server and every in-memory session with it.

Solution: I added dev/src/utils/express_utils.ts with an asyncHandler wrapper that forwards a rejection to next and a terminal error middleware, and replaced the end listener with text() from node:stream/consumers. Wrapping all 15 handlers, not just the three broken ones, keeps the file consistent and leaves the follow-up type-aware lint change with nothing to mop up. Express 5 awaits handler promises itself, so this also makes that upgrade a no-op instead of a behaviour change.

Notes for the reviewer:

  • Collision check. I listed all 611 open PRs on the fork and diffed the six that touch dev/src/server/adk_api_server.ts. None wraps a handler, adds an error middleware, or removes the end listener. Fix: reject a non-object Reasoning Engine raw body instead of crashing the dev server #621 overlaps: it adds a null/primitive guard to the same raw-body branch. That is validation, a separate concern, so I did not stack on it and I did not add the guard. My test forces the rejection with an injected logger instead of a null body, so it keeps working whichever way Fix: reject a non-object Reasoning Engine raw body instead of crashing the dev server #621 lands. Whoever merges second resolves a small conflict in that branch.
  • The error middleware honours a status the error carries. The spec asked for a flat 500. Body-parser attaches status: 400 to a malformed body and 413 to an oversized one, and a flat 500 would downgrade both. errorStatus keeps a status in the 4xx/5xx range and falls back to 500, which preserves the spec's "outputs unchanged" invariant.
  • Diff size. Roughly 190 of the 411 changed lines in adk_api_server.ts are prettier re-indenting four handler bodies that moved one level deeper. The reviewable content is the new util, the raw-body rewrite, start() and the tests.
  • Why all 15 handlers are wrapped. The other 12 cannot reject today, because each opens try on its first line. That is a property of every handler's current body, not of the route table, and it is exactly the property /run and /run_sse lost when someone hoisted a getSession call above the try. Applying the adapter uniformly makes the invariant structural instead of per-handler, and it is what keeps no-misused-promises at zero for this file when the queued lint change turns the rule on.
  • Known gap. initA2A() mounts the A2A routes after init() has run. Express only searches for an error handler in layers registered after the failing one, so this middleware does not cover the A2A surface. Moving initA2A() is out of scope.
  • I did not touch eslint.config.js. The type-aware block the follow-up task needs does not exist on main yet.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

New dev/test/utils/express_utils_test.ts (12 cases) drives a throwaway express app on a real socket, so no part of express is mocked. New Async handler failures block in dev/test/server/adk_api_server_test.ts (3 cases) plus 2 cases covering lines that moved. All 51 pre-existing cases in that file are unmodified and still pass.

$ npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts dev/test/utils/express_utils_test.ts
 Test Files  2 passed (2)
      Tests  68 passed (68)

Coverage of the new module is 100% statements, branches, functions and lines. Every line the diff adds to adk_api_server.ts is covered. Full gate on the pushed commit: npm run build, npm run lint, npm run format:check, npm run docs:check all pass. One unit:dev case fails, createAgent > Interactive Mode > should handle Vertex AI selection with gcloud defaults; it fails identically on unmodified main and is unrelated to this change.

Proving the tests can fail. Each new test was run against mutated source.

Mutation Test Result
Drop asyncHandler from /run_sse run_sse session lookup rejects Test timed out in 5000ms
Drop asyncHandler from /run run session lookup rejects Test timed out in 5000ms
Restore the req.on('end', async ...) listener reasoning_engine raw body path throws Test timed out in 5000ms
Drop the onListening rejection handler mounting the A2A surface fails Test timed out in 5000ms
Rethrow the raw-body parse error empty raw body that will not parse expected 500 to be 400
Drop .catch(next) from asyncHandler sends a rejection to the error middleware Test timed out in 5000ms
Return the promise from asyncHandler returns undefined expected Promise{…} to be undefined
Drop the headersSent branch delegates the original error expected Error: Cannot set headers after they are sent to be Error: too late
Always return 500 from errorStatus keeps the status middleware carries expected 500 to be 400
Drop the stream error listener rejects when the stream errors Test timed out in 5000ms
Decode readRawBody per chunk multi-byte character split across chunks expected '\ufffd\ufffd' to be 'é'

The hang is the expected failure mode: express never answers, so fetch never resolves.

Complexity review. I took two of the three findings. readRawBody is gone: text() from node:stream/consumers does the same job, and I confirmed on node 22 that it matches the helper on all three edge cases the deleted tests pinned. The onListening() method is gone too, but I did not restore the async listen callback, because that callback is one of the 17 no-misused-promises sites this change exists to clear; instead the A2A mount and the banner moved into start()'s own control flow, which removes a nesting level and the try/reject bridge. I did not drop the 12 no-op handler wraps, for the reason given above.

Empirical lint proof. The two rules at issue are not enabled on main, so I appended the type-aware block to eslint.config.js temporarily and ran npx eslint "dev/src/**/*.ts". adk_api_server.ts goes from 17 no-misused-promises findings to 0, and there are no no-floating-promises findings in it or in the new util. The remaining findings are in other files and belong to the follow-up task: agent_loader.ts (5 no-misused-promises), cli_run.ts (1), cli.ts (1 no-floating-promises). I reverted eslint.config.js before committing; it is not in the diff.

Manual End-to-End (E2E) Tests:

npm run build
node dev/dist/esm/cli_entrypoint.js api_server <agents-dir> --port 8974
curl -X POST localhost:8974/api/reasoning_engine \
  -H 'Content-Type: application/json,application/json' -d 'null'
curl localhost:8974/health

Before this change the server logs Received Reasoning Engine raw body: null and exits. curl reports exit 52 (empty reply), and /health then fails with exit 7 (connection refused). After this change the same request answers HTTP 500 with {"error":"Failed to handle POST /api/reasoning_engine: TypeError: Cannot read properties of null (reading 'input')"}, and /health still answers 200 OK. I also confirmed /run_sse with an unknown session still answers 404 {"error":"Session not found: nope"}, and that a malformed body on /run answers 400 rather than express's HTML page.

Checklist

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

Amaad Martin added 3 commits August 6, 2026 05:31
Express 4 discards the promise an async route handler returns, so a
rejection outside the handler's own try/catch left the request hanging
and raised a process-level unhandledRejection, which kills the dev
server under node's default mode.

Wrap all 15 async handlers in asyncHandler, replace the floating async
'end' listener on /api/reasoning_engine with an awaited readRawBody,
move the app.listen callback body onto a private method, and register a
terminal error middleware that answers with the {error} JSON shape the
routes already use.
Cover asyncHandler, readRawBody, errorStatus and errorHandler in a new
express_utils test that drives a real express app, and add a server
describe proving /run, /run_sse and the Reasoning Engine raw-body path
answer 500 without leaking an unhandled rejection.
Replace the hand-rolled readRawBody with text() from node:stream/consumers,
which node has shipped since 16.7 and which handles the same three edge
cases the helper did.

Move the A2A mount and the startup banner out of the app.listen callback
into start()'s own control flow, so the callback stays void-returning
without needing a separate private method.
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