Skip to content

Fix: let AgentLoader own the AgentFile lifetime the dev API server borrows - #830

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/agent-loader-owns-agent-file-lifetime
Open

Fix: let AgentLoader own the AgentFile lifetime the dev API server borrows#830
AmaadMartin wants to merge 3 commits into
mainfrom
fix/agent-loader-owns-agent-file-lifetime

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 8, 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):
  2. Or, if no issue exists, describe the change:

Problem: AgentLoader.getAgentFile() lends the AgentFile the loader keeps in its cache, but the dev API server bound that shared handle with await using in the agent-graph handler and in executeAgentRun(). Finishing the first /run therefore unlinked the compiled bundle and removed its temp directory, which also removes the node_modules symlink that every external package resolves through at runtime. AgentLoader.disposeAll() had the same ownership confusion: it disposed every cached entry but kept it, so a later getAgentFile() returned a disposed handle. Both defaults make this the normal path, because --compile and --bundle are .default(true) in dev/src/cli/cli.ts.

Solution: Both call sites now borrow the handle with const. Only the loader ends an AgentFile's life, and disposeAll() now drops the cache entries the way invalidateAll() already did, so a reused loader re-scans instead of lending a corpse. Both dispose paths drain the cache through one new private helper, takeAgentFiles(), so they cannot drift. dev/src/cli/cli_run.ts constructs its own AgentFile, so its await using is correct and stays, and AgentFile.dispose() is unchanged.

This supersedes #726, which fixes the two call sites but leaves disposeAll() alone and adds a loader test that pins the disposed-handle behaviour. Please close #726 in favour of this PR.

Collision check: I listed all open PRs on the fork and read the diff of every PR that touches dev/src/utils/agent_loader.ts or dev/src/server/adk_api_server.ts (#726, #727, #731, #755, #778, #793, #814, #823). Only #726 overlaps. #823 relocates the run block into a shared helper and keeps await using, so it conflicts textually but does not fix the defect. #793 replaces the preload flag with a generation counter and is a rename-level conflict inside disposeAll().

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:

  • I have added or updated unit tests for my change.
  • 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 — 89 passed.

Three tests in a new agent file lifecycle block assert the run and graph endpoints never dispose the borrowed handle. Two loader tests assert getAgentFile() lends one live handle, and lends a fresh usable one after disposeAll(). The shared fake in the server suite returns a new object per call, which is why the suite could not see this defect; I left it alone and swapped getAgentFile in place inside the new block, as three existing tests already do.

No existing test is edited, deleted or skipped.

Every line I added to dev/src/ runs under those two test files. I read the v8 JSON report to confirm it: adk_api_server.ts:349 and :1049, every statement in takeAgentFiles(), and both of its callers have a non-zero hit count.

Proof each new test can fail. I mutated one line at a time and re-ran the new tests:

Mutation Result
await using back at the graph site graph test and run+graph test fail: expected 2 to be +0 and expected 1 to be +0
await using back at the run site run test and run+graph test fail: expected 2 to be +0 and expected 1 to be +0
disposeAll() body reverted to main re-scans and lends a fresh AgentFile after disposeAll fails: expected AgentFile{ …(6) } not to be AgentFile{ …(6) }
agentsAlreadyPreloaded = true removed from preloadAgents() lends the same live AgentFile to repeated getAgentFile calls fails: expected AgentFile{ …(6) } to be AgentFile{ …(6) }

Manual End-to-End (E2E) Tests:

Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

I ran a real adk api_server with no mocks, on this branch and on the unfixed build.

  1. Write agents/myagent.ts exporting a BaseAgent subclass that yields one event, so no model credentials are needed.
  2. TMPDIR=/tmp/probe node dev/dist/esm/cli_entrypoint.js api_server ./agents --port 8791.
  3. POST /apps/myagent/users/u1/sessions/s1, then POST /run twice, then GET /apps/myagent/users/u1/sessions/s1/events/<id>/graph twice.
  4. find "$TMPDIR" -name '*.cjs' after each step.

Unfixed build: the bundle and its adk_agent_loader-* directory are gone after the first /run. This branch: both survive all four requests, all four return 200, and Ctrl-C removes the bundle.

CI

All five checks pass on head 83ee5d3d, including run-tests on ubuntu-latest, macos-latest and windows-latest.

The first windows-latest attempt failed. Two tests in tests/integration/app_loader/app_loader_test.ts hit the 40000ms per-test budget, in a run whose whole suite took 551s. Those tests call listApps(), listAgents() and getAppFile(), none of which my change touches; disposeAll() runs only in afterAll and did not report an error. The same job passed on the previous head of this branch, which had identical disposeAll() behaviour. I re-ran the job with no code change and it passed in 10m31s, so this is a slow-runner timeout and not a regression.

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 8, 2026 15:53
…orrows

The dev API server bound the loader-owned AgentFile with `await using` in
the agent-graph handler and in executeAgentRun(). Finishing the first
request therefore unlinked the compiled bundle and removed its temp
directory, and left the cached handle disposed for the rest of the
process.

Both call sites now borrow the handle with `const`. disposeAll() also
empties the cache and clears the preloaded flag, so a loader used after
disposeAll() re-scans instead of lending a disposed handle.
The API server fake returned a fresh AgentFile per getAgentFile() call, so
the suite could not see a borrower disposing the shared handle. The new
lifecycle block lends one CountingAgentFile and asserts the run and graph
endpoints never dispose it. Two loader tests assert getAgentFile() lends a
live handle and re-scans after disposeAll().
invalidateAll() and disposeAll() both empty preloadedAgents and clear
agentsAlreadyPreloaded before ending the handles' lives. takeAgentFiles()
does that once, so the two lifetime-ending paths cannot drift.
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