Skip to content

Fix: release the start-up watchdog timer and listeners in BaseTestServer.startProcess() - #375

Open
AmaadMartin wants to merge 5 commits into
mainfrom
fix/test-server-start-handshake-cleanup
Open

Fix: release the start-up watchdog timer and listeners in BaseTestServer.startProcess()#375
AmaadMartin wants to merge 5 commits into
mainfrom
fix/test-server-start-handshake-cleanup

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 31, 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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:
    Problem: BaseTestServer.startProcess() (tests/integration/test_case_utils.ts) attaches four things to the freshly spawned child during the start handshake and releases none of them when the handshake settles:
  • A dangling watchdog timer. The setTimeout(..., timeoutMs) handle was never captured and never cleared. timeoutMs is 60000 for AdkTsApiServer (tests/integration/test_api_server.ts) and 30000/60000 for AdkGoServer (tests/cross_language/a2a/ts_go/go_backend/go_server.ts, tests/cross_language/a2a/ts_go/ts_a2a_go_test.ts), so a ref'd handle survived in the vitest worker for up to a full minute after a successful start. Its callback was a dead branch by then (started was true).
  • An 'exit' listener that was never detached. stop() sends SIGINT, so the handler fired on every clean afterAll and printed a crash-looking "<serverName> exited with code ..." line. Reproduced below.
  • An unbounded stdout accumulator. stdoutChunks is only read on the premature-exit path, but the 'data' handler kept pushing to it for the whole lifetime of the server. Worse, the URL regex kept running on post-start output, so any later log line containing a loopback URL silently rewrote this.port / this.url mid-suite.
  • A URL read-back that misses non-localhost hosts. /http:\/\/localhost:([0-9]+)/i cannot match the Go backend's A2A Server started on http://127.0.0.1:<port> banner (tests/cross_language/a2a/ts_go/go_backend/server.go), so the read-back path was dead for AdkGoServer and for any subclass binding 127.0.0.1 or [::1].

This is test-infrastructure only. Nothing under core/src, dev/src or integrations/src is touched, there is no public API change, and both existing subclasses call startProcess with a byte-identical argument shape.

Solution: Give the handshake a single releaseStartHandshake() closure and call it from a finally, so cleanup runs on the success path and on both rejection paths (timeout, premature exit). It clears the watchdog and detaches the stdout 'data' and 'exit' listeners — three lines, nothing else. The URL regex moves to a module-level SERVER_URL_REGEX next to the other regex constants and now accepts localhost, 127.0.0.1 and [::1].

Design notes, and things deliberately not changed:

  • The 'error' listener and the stderr 'data' listener stay attached on purpose. ChildProcess inherits EventEmitter's special handling of 'error': an emitted 'error' with no listener is thrown, which would crash the vitest worker — and a kill() that fails emits one after the handshake. Rejecting an already-settled promise from it is a documented no-op. The stderr listener stays both for diagnostics and to keep stderr draining. Test 2 below pins both retentions so a future cleanup sweep cannot remove them by accident.
  • Detaching the stdout listener does not starve the pipe. Removing the last 'data' listener does not switch a Readable out of flowing mode (only pause() does), so stdout keeps draining and the child never blocks on a full ~64 KB pipe. An earlier revision paired the off() with an explicit stdout.resume() to document that; since the call is provably a no-op in every reachable state, it is now a comment instead. expect(child.stdout.isPaused()).toBe(false) in test 2 passes either way, which is the independent confirmation.
  • stdoutChunks is not explicitly emptied. The unbounded-growth defect is fixed entirely by detaching the 'data' handler: nothing appends to the array after the handshake. It is a local of startProcess referenced only by the two now-detached handlers and the release closure, so it becomes unreachable the moment the method returns — an explicit stdoutChunks.length = 0 would free nothing, and leaving it out keeps the premature-exit diagnostic trivially safe (test 7).
  • The started flag is gone. With onExit detached at settle time it can no longer fire post-start, so the !started guards were dead weight. The finally runs in the microtask following resolve(), strictly before the next I/O event, so no exit can slip through the gap.
  • The bare console.error("<name> exited with code <code>") line is dropped; it was the reported noise. A premature exit is still reported, via the captured-stdout diagnostic plus the rejection.
  • All three error message strings, the successLogMessage line, and the resolved port/url for both existing subclasses are unchanged. stop() is untouched.
  • Out of scope, per the task: stop()'s unconditional 500 ms sleep, getResponse()'s missing timeout, handling a startMessage split across two stdout chunks, and killing the child on the failure path (afterAll still calls stop() when beforeAll throws).

Collision check: ran gh pr list --repo AmaadMartin/adk-js --state open --limit 100 (100 open PRs) and gh pr diff <n> --name-only over every plausibly adjacent test-infrastructure PR (#359, #349, #343, #316, #314, #311, #308, #307, #306, #305, #304, #300, #299, #297, #296, #281, #276, #329). None of them touches tests/integration/test_case_utils.ts, tests/integration/test_api_server.ts, tests/cross_language/** or the Go backend. No overlap, so this branches from main rather than stacking.

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.

New file tests/integration/test_case_utils_test.ts (9 cases, picked up by the existing integration vitest project — no config change). No existing test file was modified or deleted. It drives BaseTestServer through a hermetic, cross-platform node -e child spawned with process.execPath (no shell string, no POSIX-only binary, no literal newlines in the argv element).

$ npx vitest run --project integration tests/integration/test_case_utils_test.ts
 ✓ |integration| tests/integration/test_case_utils_test.ts (9 tests) 6364ms
 Test Files  1 passed (1)
      Tests  9 passed (9)

Proof the tests can fail. Every new case was run against mutated source; the mutation and its failure message:

# Mutation Result
1 Full revert of the startProcess hunk (git checkout main -- tests/integration/test_case_utils.ts) 7 failed / 2 passed. clears the start-up watchdog...expected "clearTimeout" to be called with arguments: [ …(1) ]; detaches the start-up listeners... and rejects and releases the handshake when the server never startsexpected 1 to be +0; does not log an exit message...expected 'Fake exited with code null' not to match /exited with code/; ignores URLs printed after start-upexpected 49999 to be 41111; the 127.0.0.1 and [::1] banner rows → expected 19999 to be 41234. The two that (correctly) still pass are the localhost banner row (unchanged behaviour) and the premature-exit guard.
2 Drop clearTimeout(startTimer) only 1 failed: clears the start-up watchdog once the server reports successexpected "clearTimeout" to be called with arguments: [ …(1) ]
3 Drop the two off(...) calls only 4 failed: detaches the start-up listeners...expected 1 to be +0; does not log an exit message when the server is stopped cleanlyexpected 'Fake Captured stdout before premature…' to be ''; ignores URLs printed after start-upexpected 49999 to be 41111; rejects and releases the handshake when the server never startsexpected 1 to be +0
4 Narrow SERVER_URL_REGEX back to localhost only 2 failed: the http://127.0.0.1:41234 and http://[::1]:41234 rows → expected 19999 to be 41234
5 Empty stdoutChunks at the top of onExit (destroy the diagnostic before it is built) 1 failed: rejects with the captured stdout when the server exits prematurelyexpected 'Fake Captured stdout before premature…' to contain 'boot log line'

Mutation 3 is why the clean-stop case asserts logged is '' and not only that it does not match /exited with code/: dropping the detach moves the noise to the premature-exit diagnostic, which the narrower assertion missed. That strengthening is its own commit.

Coverage. vitest.config.ts only instruments core/src, dev/src and integrations/src, so files under tests/ are not measured and this change cannot move the thresholds (they are untouched). Every new branch is instead covered by a direct behavioural test: the release helper on the success path (cases 2, 3, 4), the timeout path (case 6) and the premature-exit path (case 7); the broadened regex on all three host forms (case 5); the watchdog clear (case 1). Every line of the release closure is mutation-pinned (rows 2 and 3 of the table above).

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

npm install && npm run build
npx vitest run --project integration tests/integration/a2a/basic/a2a_agent_test.ts

This boots the real AdkTsApiServer (the ADK API server CLI) and tears it down in afterAll. Before/after teardown output, same command, same machine:

Before (helper reverted to main) — the run is green but teardown prints a crash-looking line:

stdout | tests/integration/a2a/basic/a2a_agent_test.ts > A2A: Remote Agent Basic
Test ADK API Server started

stderr | tests/integration/a2a/basic/a2a_agent_test.ts > A2A: Remote Agent Basic
CLI exited with code 0

 ✓ |integration| tests/integration/a2a/basic/a2a_agent_test.ts (1 test) 3882ms

After — the CLI exited with code ... line is gone:

stdout | tests/integration/a2a/basic/a2a_agent_test.ts > A2A: Remote Agent Basic
Test ADK API Server started

 ✓ |integration| tests/integration/a2a/basic/a2a_agent_test.ts (1 test) 3910ms

(On this Linux box the CLI installs a SIGINT handler and exits 0, so the observed code is 0 rather than null; the spurious line is the same defect either way.)

Cross-language run with a Go toolchain installed (go1.26.5), exercising AdkGoServer — the subclass the regex fix targets:

$ npx vitest run --project cross-language tests/cross_language/a2a/ts_go/ts_a2a_go_test.ts
Test Go Server started at http://127.0.0.1:44930
 ✓ |cross-language| tests/cross_language/a2a/ts_go/ts_a2a_go_test.ts (2 tests) 15377ms
 Test Files  1 passed (1)
      Tests  2 passed (2)

Green, the resolved URL still points at the port the Go server actually bound, and no Go Server exited with code ... line at teardown.

CI note. The run-tests matrix is green on all three legs of the pushed commit — ubuntu-latest (4m36s), windows-latest (9m38s) and macos-latest (7m35s) — and the new file passes on each (✓ integration tests/integration/test_case_utils_test.ts (9 tests)). An earlier commit on this branch saw macos-latest fail on two tests/integration/app_loader/app_loader_test.ts cases with Test timed out in 40000ms; that file's failing cases exercise AgentLoader.listApps() / AppFile.load() (esbuild compilation) and touch nothing in this diff, the same file fails the same way on macos-latest on an unrelated branch that modifies neither test_case_utils.ts nor app_loader, and that run was simply slow (501s total, 241s of it in transform). It has not recurred.

Static checks on the pushed commit: npm run build, npm run lint and npm run format:check all pass. npm run ts:check is red on main today for unrelated reasons (it is not part of .github/workflows/validation.yaml); neither of the two files in this diff contributes an error to it.

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 5 commits July 31, 2026 01:26
…artProcess

The start handshake attached a watchdog timer, a stdout listener and an
'exit' listener to the spawned child and never released them. The timer
stayed ref'd for up to 60s after a successful start, the stdout listener
kept growing an unread buffer and kept rewriting this.port from any later
loopback URL, and the 'exit' listener printed a crash-looking
"<server> exited with code null" line on every clean SIGINT teardown.

Release all three from a finally block so cleanup also runs on the timeout
and premature-exit paths, keep stdout draining with an explicit resume(),
and broaden the URL read-back to 127.0.0.1 and [::1] so the Go backend's
banner is recognised. The 'error' and stderr listeners stay attached on
purpose: an 'error' event with no listener is thrown by EventEmitter.
Adds a hermetic suite that drives BaseTestServer through a `node -e`
child: it asserts the watchdog handle is passed to clearTimeout, that the
stdout/'exit' listeners are detached (and the 'error'/stderr ones are not)
on the success, timeout and premature-exit paths, that a clean stop() logs
no exit line, that a post-start URL no longer rewrites the port, and that
the banner read-back works for localhost, 127.0.0.1 and [::1].
Dropping the two off() calls left the clean-shutdown case logging the
premature-exit diagnostic instead of the old exit line, which the
/exited with code/ assertion alone did not catch.
The inline start-handshake option type was repeated at two call sites, and
the fake server's constructor port happened to share the watchdog timeout's
literal value, which made the read-back assertions hard to read.
…elease closure

Removing the last 'data' listener does not switch a Readable out of
flowing mode, so the paired resume() could never do anything; a comment
now records why detaching is safe. stdoutChunks is a local referenced
only by the two detached handlers and the release closure, so clearing
it freed nothing. The closure is now the three operations that matter.

Verified unchanged behaviour: all 9 cases in
tests/integration/test_case_utils_test.ts still pass (including the
stdout.isPaused() === false assertion), dropping the two off() calls
still fails 4 of them, and emptying stdoutChunks inside onExit still
fails the premature-exit diagnostic test.
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