Fix: stop the dev API server disposing the AgentFile it borrows from AgentLoader - #726
Open
AmaadMartin wants to merge 3 commits into
Open
Fix: stop the dev API server disposing the AgentFile it borrows from AgentLoader#726AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 2 commits
August 6, 2026 08:09
…AgentLoader AgentLoader.getAgentFile() lends out the AgentFile it keeps in its own registry. The agent-graph handler and executeAgentRun() bound that handle with `await using`, so the first request for an app deleted the compiled artifact and latched the shared handle as disposed. Both sites now borrow with `const`, and getAgentFile() documents the ownership contract.
The server test harness handed every getAgentFile() call a fresh object, which hid the shared-handle defect. It now lends one stub per test that mirrors the real disposal latch, and three tests assert the handle survives repeated run and agent-graph requests. A loader test pins the premise: getAgentFile() returns the same instance, and disposing it poisons what the loader serves.
The stub threw from getFilePath() exactly when disposeCount was above zero, so the second assertion restated the first. The server never calls getFilePath() on the borrowed handle either. The real AgentFile keeps that behaviour pinned in dev/test/utils/agent_loader_test.ts.
7 tasks
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
Problem:
AgentLoader.getAgentFile()lends out theAgentFileit keeps in its own registry, but the dev API server bound that handle withawait usingin the agent-graph handler and inexecuteAgentRun(). The first graph request or agent run for an app therefore deleted the compiled artifact, removed itsadk_agent_loader-*temp directory, and latched the shared handle as disposed.getFilePath()then throwsAgent is disposed and can not be usedfor the rest of the process, which breakscopyAgentFiles()indev/src/cli/deploy/deploy_utils.ts. The endpoints keep returning 200 only becauseAgentFile.load()memoises the agent.Solution: Both call sites now borrow the handle with
const. Only the loader ends anAgentFile's life, ininvalidateAll()on a watched file change or indisposeAll()at process exit.getAgentFile()gained a doc comment stating that contract, so theawait usingis not reintroduced.cli_run.tsconstructs its ownAgentFileand keepsawait using, sodispose()and[Symbol.asyncDispose]stay on the class.Collision check: I listed all 618 open PRs on the fork and read the diffs of every PR that touches
dev/src/server/adk_api_server.ts(#653, #511, #452, #633, #252, #719, #621, #600). None removes theawait usingat either site. #252 relocates the run-endpoint block and keepsawait using, so it overlaps textually but does not fix this defect; this PR branches frommainbecause a two-line bug fix should not wait on a refactor.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/server/adk_api_server_test.ts dev/test/utils/agent_loader_test.ts— 86 passed.Harness change: the fake loader in
dev/test/server/adk_api_server_test.tsreturned a new object from everygetAgentFile()call, which made the shared-handle defect invisible. It now lends one stub per test that mirrors the real disposal latch. No existing test was deleted or weakened; all 54 tests in that file still pass, including the three that swapgetAgentFilefor a rejecting stub.Each server test asserts that the borrowed handle was never disposed. Asserting only the HTTP status proves nothing here, because the second request returns 200 before the fix too.
Proof the new tests can fail. I restored
await usingat one call site at a time and re-ran the new block:await usingback at the graph site (adk_api_server.ts:339)AssertionError: expected 2 to be +0andexpected 1 to be +0await usingback at the run site (adk_api_server.ts:1039)AssertionError: expected 2 to be +0andexpected 1 to be +0agentsAlreadyPreloaded = trueremoved frompreloadAgents()expected AgentFile{ …(6) } to be AgentFile{ …(6) }disposedguard removed fromgetFilePath()expected [Function] to throw an errorBoth changed production lines execute in the new tests, as the two mutations above show, so new-line coverage is 100%.
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
I ran this against a real server, with no mocks, on both builds.
agents/myagent.tsexporting aBaseAgentsubclass that yields one event, so no model credentials are needed.node dev/dist/esm/cli_entrypoint.js api_server ./agents --port 8791.POST /apps/myagent/users/u1/sessions/s1, thenPOST /runtwice, thenGET /apps/myagent/users/u1/sessions/s1/events/<id>/graphtwice.$TMPDIR/adk_agent_loader-*/myagent.cjs.Results: on the unfixed build the temp directory and the compiled artifact are gone after the first
/run. On this branch both survive all four requests. OnCtrl-Cthe artifact is unlinked; the empty temp directory can survive, which is a separate defect in the asyncexithandler and is out of scope here.Local validation on the pushed commit:
npm run build,npm run lint,npm run format:checkall pass.npx vitest run --project unit:devreports one failure,cli_create_test.ts > should handle Vertex AI selection with gcloud defaults, which also fails on unmodifiedmainon this machine because it reads a local gcloud default.npx tsc --noEmitoutput is byte-identical tomain.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.
CI
All on the head commit
bb70e32c:run-tests (windows-latest)passes with 2790 tests plus eslint, prettier and typedoc;run-tests (macos-latest)passes; the cross-languagerun-testspasses;check-licensepasses.run-tests (ubuntu-latest)never gets a runner. Across five attempts it sits queued and GitHub cancels it with zero steps and no logs. The same zero-step cancellation is hitting unrelated branches on this fork. It passed on the parent commitd6969b30, and the only change since is the deletion of test lines.An earlier
macos-latestattempt failed onapp_loader_test.ts > should discover apps vs agents across directories and standalone files, which times out at 40000ms. That test drivesAgentLoaderdirectly and never starts the API server, and it times out the same way on this branch's base commit81c14222(run 31066708856). It passes now.Local checks on
bb70e32c:npm run build,npm run lint,npm run format:checkand the targeted unit tests (86 passed) are green.