Skip to content

Fix: consolidate the three AgentLoader process-handler PRs onto one branch - #861

Open
AmaadMartin wants to merge 7 commits into
mainfrom
fix/agent-loader-process-handler-consolidation
Open

Fix: consolidate the three AgentLoader process-handler PRs onto one branch#861
AmaadMartin wants to merge 7 commits into
mainfrom
fix/agent-loader-process-handler-consolidation

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):

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

Problem: Three open PRs rewrite the same process-handler block in dev/src/utils/agent_loader.ts, so whichever lands first makes the other two unmergeable. Each one alone leaves a real defect: #511 fixes the listener leak and the silent uncaughtException exit, #703 fixes the exit status, #653 fixes the temp-directory leak. On main the constructor installs five listeners that nothing removes, Ctrl-C on adk web exits 0, and every run leaves an adk_agent_loader-* directory in the OS temp directory.

Solution: This branch carries all three fixes on one trunk. #511 is the trunk because opt-in registration is the only one of the three that changes the shape of the block, so the other two reduce to a small delta on top of it. Commits 1-5 are #511 verbatim, rebased on main with no conflicts. Commit 6 folds in #703's exit status and commit 7 folds in #653's synchronous cleanup.

Collision check: gh pr list plus a diff grep for process.on|process.exit|installProcessHandlers|disposeAllSync|uncaughtException over every open PR that touches agent_loader.ts (#830, #814, #793, #755, #731, #674, #662, #854). Only #511, #703 and #653 touch this block, so nothing else collides.

Behaviour changes (all intended, all argued in the source PRs):

  1. A crash now exits 1 with a stack, and Ctrl-C exits 130. Both were 0.
  2. Handler registration is opt-in. All five construction sites in this repo are updated; AgentLoader is not exported from core, so the blast radius is the dev package.
  3. AdkApiServer.stop() disposes the loader it built itself. A caller who injected a loader is unaffected.

Conflict resolutions worth review:

Known residual leak, not fixed here: adk web can still leave one scratch directory when the debug UI compiles an agent, the server disposes that borrowed AgentFile through await using, and a later request recompiles it. disposeSync() skips a file already marked disposed. The root cause is the borrowed-handle lifetime, which #830 fixes.

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/utils/agent_loader_test.ts     # 54 passed
npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts  # 58 passed
npx vitest run --project unit:dev                     # 276 passed, 1 failed
npm run build && npx eslint <the four changed files> && npx prettier "dev/**/*.ts" --check

The one unit:dev failure is pre-existing and unrelated: dev/test/cli/cli_create_test.ts > should handle Vertex AI selection with gcloud defaults reads the developer's real gcloud config. It fails identically with dev/ checked out from main.

npx tsc --noEmit reports no error in any file this PR touches. The errors it does report are pre-existing, in core/test/ and tests/integration/.

Mutation proofs. Every new test was run once against the unfixed line.

Mutation Test that failed Message
process.exit(128 + os.constants.signals[signal]) -> process.exit() exits SIGINT/SIGUSR1/SIGUSR2 with 128 plus the signal number expected "spy" to be called with arguments: [ 130 ]
removeListener -> removeAllListeners keeps another loader listeners when disposeAll runs twice expected [ 0, 0, 0, 0, 1 ] to deeply equal [ 1, 1, 1, 1, 1 ]
onExit -> () => void this.disposeAll() cleans up synchronously from the exit listener, the exit listener disposes cached agents expected true to be false, expected "disposeAllSync" to be called at least once
drop removeProcessHandlers from disposeAllSync() removes installed listeners on disposeAllSync expected [ 1, 1, 1, 1, 1 ] to deeply equal [ 0, 0, 0, 0, 1 ]
drop fs.rmSync from disposeSync() 5 tests, incl. removes the compiled output directory synchronously on disposeSync expected true to be false
ownsAgentLoader = false should dispose the agent loader it built itself when stopped expected [ 'agent-0' ] to deeply equal []
stop() finally -> straight-line should release its own agent loader even when closing the server fails expected "disposeAll" to be called 2 times, but got 1 times

Existing tests this PR edits. Both edits sit in the commit that changes the behaviour they pin.

  • the exit listener disposes cached agents now spies on disposeAllSync, because that is what the listener calls. The behaviour it pins is unchanged.
  • The server suite's file_utils stub moves from getTempDir to createTempDir. main removed getTempDir, so the old stub was inert and the ownership test failed with expected [] to have a length of 1.

Tests from the superseded PRs that are not carried over. Each is a duplicate; the surviving test is named.

Manual End-to-End (E2E) Tests:

npm run build
mkdir /tmp/agents && cat > /tmp/agents/temp_agent.js <<'EOF'
export const rootAgent = {name: 'tempAgent', [Symbol.for('google.adk.baseAgent')]: true};
EOF
npx adk web /tmp/agents --port 8080     # then GET /list-apps, then Ctrl-C
echo $?                                 # 130
ls -d $TMPDIR/adk_agent_loader-*        # the directory this run created is gone

Measured on this branch, for both adk web and adk api_server: exit 130, scratch directory removed. Measured on main with the same script: exit 0, scratch directory left behind.

tests/integration/ needs no edit. The integration project does not run in my sandbox: its fixtures run npm install, which cannot reach the registry here. It fails the same way on main.

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 7 commits August 9, 2026 04:21
The AgentLoader constructor unconditionally registered five process
listeners, so merely constructing a loader (directly or via
AdkApiServer) mutated global process state. The uncaughtException
listener converted every crash in the process into a silent exit 0
with no stack trace, and none of the listeners were ever removed, so
each loader leaked one listener per event for the process lifetime.

Registration now happens only through an explicit
installProcessHandlers(), called from the CLI entrypoints that own the
process, and disposeAll() removes what it installed. No
uncaughtException listener is installed at all, so Node's default
crash reporting is restored.
Adds a process-handlers suite to the agent loader tests that pins the
constructor registering nothing, the opt-in install adding exactly one
listener per event, the absence of any uncaughtException listener,
idempotency, removal on disposeAll and reinstall afterwards. Server,
CLI and deploy tests cover the opt-in flag and the CLI call sites.

The loader tests capture the listener the loader installed by diffing
process.listeners() and invoke it directly, rather than emitting the
event, so Vitest's and Tinypool's own listeners are left alone.
…ader

Taking the baseline after construction meant the test only pinned the
install path; a constructor that registered an uncaughtException
listener still passed. Snapshotting first pins the invariant across the
loader's whole lifecycle.
vi.mocked keeps the mock metadata typed, so the new assertions do not
need an `as unknown as Mock` escape hatch to reach .mock.calls.
The ProcessHandler record type and the array of them were a generic
listener registry serving two listener shapes, with the bookkeeping
spread over five sites. Closing over the two listeners and storing one
removal closure drops the interface, the private removal method and the
per-signal closure allocation, and makes the installed/not-installed
state the presence of that closure. Behaviour is unchanged: install is
still idempotent, disposeAll still removes exactly this instance's
listeners, and reinstall still works.
…lers

The signal listeners called a bare `process.exit()`, so Ctrl-C on `adk web`
reported success. A shell reports a process killed by signal `N` as `128 + N`,
so `SIGINT` must exit `130`. Node passes the signal name to the listener, so
one closure reads it and looks the number up in `os.constants.signals`, which
differs per platform.

This folds PR #703 onto the opt-in registration lifecycle of PR #511. #703 also
removed the `uncaughtException` listener and added a removal closure; #511
already does both, so only the exit status is new here.

The two new tests ask for the handlers explicitly, because the constructor no
longer installs them.
…s exit

A process 'exit' listener must be fully synchronous: Node terminates as soon as
the last listener returns and drops any pending promise or queued I/O. The
listener awaited disposeAll(), so every `adk web` and `adk api_server` run left
its compiled-agent directory behind in the OS temp directory.

AgentFile.disposeSync() and AgentLoader.disposeAllSync() do the same work with
`fs.rmSync`, and the exit listener calls the sync path. disposeAllSync() also
removes the process handlers, so it stays symmetric with disposeAll() and is
safe for ordinary teardown; removing a listener during 'exit' emission is
allowed. AdkApiServer.stop() now disposes the loader it built for itself; a
loader supplied through ServerOptions still belongs to the caller.

This folds PR #653 onto PR #511's opt-in registration lifecycle, so the
constructor still registers nothing. Two existing tests change with it: `the
exit listener disposes cached agents` now spies on disposeAllSync, because that
is what the listener calls, and the file_utils stub in the server suite moves
from the removed getTempDir to createTempDir.
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