Fix: force established sockets shut in AdkApiServer.stop() so teardown always settles - #452
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: force established sockets shut in AdkApiServer.stop() so teardown always settles#452AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
http.Server#close() stops the listener but then waits for every established connection to drain, so a client parking a socket -- a browser tab on the dev UI, or an in-flight /run_sse stream -- left stop() unsettled forever. Call closeAllConnections() after close(), per the Node guidance, so teardown is bounded regardless of attached clients. Adds a regression test that parks a raw connected socket across stop(), plus a no-connections test pinning the ordinary path.
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
No existing issue.
Problem:
AdkApiServer.stop()(dev/src/server/adk_api_server.ts) is a bare wrapper aroundhttp.Server#close. Per the Node.js docs,close()"stops the server from accepting new connections and closes all connections connected to this server which are not sending a request or waiting for a response", and its callback only fires once every remaining connection has drained. It never forces an established socket shut, so a single client parking a connection wedges teardown andawait server.stop()never settles. This is not theoretical for this server: the SSE route setsConnection: keep-alive, callsres.flushHeaders(), and streams for as long as the agent runs, so one attached/run_sseclient — or one browser tab on the dev UI mid-connect — is enough to hang shutdown and hold the port.Measured on Node v22.22.2 (
node -vin this workspace), with a plainhttp.Serverand no mocks:close()is calledclose()close()+closeAllConnections()Solution: call
this.server!.closeAllConnections()inside the same promise executor, immediately afterthis.server!.close(...). Two statements plus a comment; no new file, type, option, export, or dependency.Design notes:
closeAllConnections()and not manual socket tracking. The alternative — aSet<net.Socket>, aconnectionlistener, a per-socketcloselistener to evict entries, and teardown on the error path — is a leak-prone accumulator reimplementing what the runtime already maintains correctly.close(). The Node docs say explicitly: "Whenever using this in conjunction withserver.close, calling this afterserver.closeis recommended as to avoid race conditions where new connections are created between a call to this and a call toserver.close." Both calls stay synchronous within the executor, so nothing can be accepted in between.closeIdleConnections(). It only reaps sockets that are neither sending a request nor awaiting a response — precisely the setclose()already reaps on Node >= 19. Verified: an in-flight SSE stream still hangs withclose()+closeIdleConnections().closeAllConnections()is@since v18.2.0per the declaration in the repo's own pinned@types/node(dev/package.jsonpins^20.12.7;node_modules/@types/node/http.d.ts:444).README.md:55states the only declared requirement — "ADK for TypeScript requires a current Node.js LTS release" — and every supported LTS line clears 18.2.0.core/test/a2a/agent_to_a2a_body_parsing_test.tsalready callscloseAllConnections()for this exact reason, so the API is established precedent here. This PR deliberately does not add anenginesfield; that gap is separate and unrelated to the hang.close(err => ...)callback still rejects verbatim, soERR_SERVER_NOT_RUNNINGon a doublestop()or a failedlisten()is unchanged —dev/test/server/adk_api_server_test.ts:1213relies on that via.catch(() => {})and still passes.closeAllConnections()is a synchronous void method that does not throw on a never-listened, listen-failed, already-closed, or listening server, so it is not wrapped intry/catch, and no?.()guard is added (this.serveris already narrowed and the method is non-optional in@types/node).Behavioural change, stated plainly:
stop()no longer waits for in-flight requests to complete — established connections are destroyed, and clients see an abrupt close (ECONNRESET/TypeError: terminatedfromfetch). A caller relying onstop()as a graceful drain would notice. This is intended and assessed as acceptable: this is the local development / test API server,stop()has no production call site indev/src(grepped — the only callers are tests; the CLI has noSIGINThandler that invokes it), and the previous "graceful" behaviour was in practice an unbounded hang rather than a bounded drain. A desirable side effect: destroying an SSE socket fires the existingreq.on('close')handler, which callsabortController.abort(), so shutdown cancels in-flight agent runs through the cancellation path that already exists instead of orphaning them.Collision check (required before implementation):
gh pr list --repo AmaadMartin/adk-js --state open --limit 400returned 353 open PRs; I filtered titles/branches forstop|close|connection|socket|server|shutdown|listen|hang|teardown|drain|destroy|settleand diffed every plausibly adjacent PR. Four open PRs touchdev/src/server/adk_api_server.ts(#252, #199, #300) or mentionstop()(#359), and none of them addscloseAllConnections()or otherwise changesstop(): #252 consolidates the run handlers, #199 adds a cross-origin guard, #300 fixes A2A agent-card URLs, #359 only names hook timeouts inwebui_test.ts. No collision, so this is branched frommainrather than stacked. (#359's PR body assertsstop()is "http.Server.close()" when sizing itsafterAllbudget; that description becomes stale with this change, but the two diffs do not overlap.)Out of scope, untouched:
tests/integration/adk_web/webui_test.ts,core/test/a2a/agent_to_a2a_body_parsing_test.ts, anyenginesdeclaration,stop()idempotence (a second call still rejects withERR_SERVER_NOT_RUNNING), and any new configuration knob — a grace-period/timeout option would be write-only config with no reader.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.
Two tests added to
dev/test/server/adk_api_server_test.tsas a newdescribe('Shutdown', ...)block. No existing test was edited, skipped, weakened, or deleted.should resolve stop() while a client holds a connection open— the regression test. Starts a dedicatedAdkApiServer, opens a rawnet.connectsocket and awaits itsconnectevent, then assertsstop()resolves. The socket is deliberately connected but has sent no request bytes: an idle keep-alive socket would be reaped byclose()on Node >= 19 and the test would pass with and without the fix. Anerrorlistener is attached before stopping because the server-side destroy surfaces asECONNRESETand an unhandlednet.Socketerror crashes the worker; the socket is destroyed in afinally. The host is pinned to127.0.0.1because the defaultlocalhostresolves to::1on some machines (verified locally), which would make the client connect fail rather than exercise the bug. Bound is Vitest's default 5 s timeout — the gap being measured is ~1 ms vs. infinite, so noPromise.raceagainst a hand-rolled timer was added.should resolve stop() with no connections attached— pins that the forceful destroy did not break the ordinary path, and guards theclose()-then-closeAllConnections()ordering.Proof the tests can fail (mutation). Deleted exactly the added line
this.server!.closeAllConnections();fromstop(), leaving everything else in place, and re-ran the file:Test 1 hangs and fails on the 5 s timeout without the fix; Test 2 still passes, confirming it pins the ordinary path rather than the bug. The line was then restored and the full file re-run green.
Coverage. The new production statement is on the already-exercised
stop()happy path.npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts --coverage.include='dev/src/server/adk_api_server.ts'reports the file at 84.06% lines / 68.65% branches overall (pre-existing, unrelated routes), and the added line is not in the uncovered list — 100% of the new code is covered. The one uncovered line insidestop()is the pre-existingreturn Promise.resolve()early return for a never-started server (991-992), whose behaviour this change does not touch; per the plan no test was added for it.Regression suites that already exercise real
start()/stop()over real sockets (unedited, still green):CI gates, run locally on the pushed commit:
npm run ts:checkreports pre-existing errors in 41 unrelated files onmain; neitherdev/src/server/adk_api_server.tsnordev/test/server/adk_api_server_test.tsappears in that output.CI:
run-testsgreen on ubuntu-latest, macos-latest, and windows-latest. The first windows-latest attempt failed oncore/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout("Test timed out in 5000ms") — an unrelated pre-existing Windows shell flake in a file this PR does not touch (already being addressed by #254 and #246); it passed on re-run with no code change.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Note that the CLI installs no
SIGINThandler that callsstop()(greppeddev/src), so a Ctrl-C onadk webdoes not route through this code path. The equivalent real e2e drives the built package over real sockets with no mocks:npm install && npm run build.@google/adk-devtools(adjust the import path to your checkout):Before the fix:
stop()never settles. After the fix (measured):stop(): settled after 1ms; port 43873 rebindable: true. 3. SSE path, verified separately against a plainhttp.Serverreproducing the/run_sseshape (Connection: keep-alive+res.flushHeaders()+ a written chunk + a response that never ends), with the client holding the stream open:close()alone was still hung at the 2000 ms cutoff, whileclose()+closeAllConnections()settled in 1 ms and fired the server-sidereq.on('close')handler — the same handler the dev server uses to callabortController.abort()and log "HTTP connection closed. Aborting agent SSE execution for session …".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.