Skip to content

Fix: stop embedding the ADK runtime in every compiled agent file - #480

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/agent-loader-external-adk-runtime
Open

Fix: stop embedding the ADK runtime in every compiled agent file#480
AmaadMartin wants to merge 2 commits into
mainfrom
fix/agent-loader-external-adk-runtime

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 1, 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:

The AgentLoader discovery integration test intermittently fails with
Test timed out in 40000ms on the macos-latest and windows-latest legs of
the validation workflow. It is not branch specific — it has reddened push runs
against main with the ubuntu leg green.

The root cause is measured, not inferred. The failing test's first statement is
await loader.listApps(), which is the first call into preloadAgents() and so
calls AgentFile.load() on every discovered candidate. AgentFile.load() runs
esbuild.build() with packages: 'bundle', so each four-line fixture is
compiled into a self-contained minified copy of the entire ADK runtime
(@google/adk pulls in @google-cloud/storage, ten @opentelemetry/*
packages, @mikro-orm/*, express, winston, @google/genai, zod, …). That
artifact is then await import()ed into the test process. Four fixtures means
four independent multi-megabyte ADK evaluations.

Measured on this branch, on a fast multi-core Linux workstation with warm
caches, no coverage instrumentation and no competing test files:

Phase Before After
Compiled discovery/service_alpha/app.ts artifact 5,902,465 bytes 981 bytes
listApps() over the 4-fixture discovery tree 17,852 ms 1,941 ms
should discover apps vs agents… (in suite) 17,862 ms 1,992 ms

17.9 s of a 40 s budget was being consumed on the fastest platform under ideal
conditions. CI runs npm run test:coverage (V8 instrumentation) on 2–4 core
macOS/Windows runners with ~65 other integration/e2e files scheduled across
parallel forks, where a 2–3× slowdown is unremarkable. Nothing about the test is
non-deterministic except how much CPU it gets — hence the intermittency.

Separately, every beforeAll/afterAll in the file passed
TEST_EXECUTION_TIMEOUT (40000) as its third argument, which lowers the hook
budget below the integration project's deliberate hookTimeout: 120000 — a
value chosen (vitest.config.ts:11-15) precisely because a cold, network-bound
fixture install has been measured at ~70 s. So the install-heavy hooks were
capped at 40 s for no reason.

Solution:

Remove the work rather than widen the budget. No per-test timeout is raised
anywhere.

  1. dev/src/utils/agent_loader.ts — add @google/adk and
    @google/adk-devtools to the external array already passed to
    esbuild.build(). Runtime resolution is served by the node_modules symlink
    the loader already creates next to the output (linkProjectNodeModules) —
    that helper exists precisely so bare specifiers resolve from the compiled
    artifact's location. This is the entire production diff (5 lines, 2 of them
    the literals). Beyond CI, a directory of N agents now evaluates one shared
    ADK runtime instead of N private copies, so adk web / adk run startup
    time and resident memory stop scaling linearly with agent count.
  2. tests/integration/app_loader/app_loader_test.ts — drop the hard-coded
    40 s third argument from the four hooks that run npm install or tear down
    its node_modules tree, so they inherit the project's deliberate 120 s
    hookTimeout. Every it()'s TEST_EXECUTION_TIMEOUT is untouched, and
    TEST_EXECUTION_TIMEOUT is still referenced by the five per-test budgets.

Why not the alternatives (all measured):

Alternative Why rejected
Raise TEST_EXECUTION_TIMEOUT Treats the symptom; the test would still burn ~18 s on the fastest platform. After this change it runs in 1,992 ms against the unchanged 40 s budget.
Drop minify A red herring: turning it off moves the esbuild step from 495 ms to 397 ms, i.e. ~100 ms of a ~6,000 ms per-fixture cost.
packages: 'external' wholesale Too broad: it would also externalise a user agent's own third-party deps, which adk deploy ships as a single copied file. Externalising only the two ADK packages is the minimal change that removes the duplication.
Make preloadAgents() lazy Does not fix this test (listApps() must load to tell an App from a BaseAgent), changes /list-apps semantics, and breaks copyAgentFiles(), which calls getFilePath() right after listAgents().

Deliberately NOT in this PR. Both were in the original plan for this fix and
were cut during review. Each is a real change, but neither is needed to fix the
flake:

  • Deleting tests/integration/app_loader/discovery/package.json and its
    npm install.
    That fixture's install is slow (606 packages / ~423 MB /
    31,072 files, ~72 s cold) and the fixture never spawns a child process, so on
    paper it is removable. But it is also the only fixture in this suite with a
    project-local node_modules holding a real installed @google/adk — which
    is exactly the resolution path this PR's external marking now depends on,
    and the only realistic user layout (getProjectNodeModulesDir looks beside
    the nearest package.json; without it the walk reaches the monorepo root,
    which no user project has). Deleting it would also flip the fixture from CJS
    to ESM via the root "type": "module", losing CJS integration coverage in the
    same PR that changes how the artifact resolves its imports. Keeping it means
    the new assertion below runs against a project-local install producing a
    .cjs artifact. The 72 s install is covered by the 120 s hook budget;
    removing it, with its CJS→ESM coverage trade-off, belongs in its own PR
    (Test: drop the npm install from the AgentLoader discovery integration fixture #276). Consequence, stated plainly: discovery/node_modules is still
    enumerated by getDirFiles() on every discovery pass, and that is the ~1.9 s
    which remains of the original 17.9 s.
  • fail-fast: false on the CI OS matrix. The matrix does default to
    fail-fast, so one flaking leg still cancels the other two, but that is a
    repo-wide CI policy affecting every job on every PR and nothing about marking
    @google/adk external needs it. Fix: stabilize app_loader integration test timeouts and stop matrix fail-fast #235 already carries it as its own change.

Breaking change analysis: no exported signature, option or return type
changes. listApps() still returns ["service_alpha","standalone_app"] and
listAgents() still returns the same four names (verified below).
isApp()/isBaseAgent() are brand checks on Symbol.for(...), which uses the
cross-realm global registry, so identification is unaffected by which copy
constructed the object — and this was already the situation, since today's
bundle embeds its own ADK copy distinct from the caller's.

adk deploy cloud_run stays safe: the deployed artifact now imports
@google/adk rather than embedding it, and the generated image provides it
three ways over — createPackageJson() hard-fails unless the project declares
REQUIRED_NPM_PACKAGES = ['@google/adk']; the generated Dockerfile does
COPY node_modules /app/node_modules then RUN npm install --production; and
the artifact lands in /app/agents/<appName>/, so Node's upward walk reaches
/app/node_modules.

Residual risk (low, disclosed): getProjectNodeModulesDir() picks the
node_modules beside the nearest ancestor package.json, whereas Node's own
resolution walks every ancestor node_modules. A layout that hoists
@google/adk above the nearest package.json and has no local node_modules
would now fail at import time instead of at build time. That surfaces as an
immediate, legible ERR_MODULE_NOT_FOUND at the existing await import() site
(already inside the try that raises AgentFileLoadingError) rather than
silent misbehaviour, and no fixture or supported layout in this repo exhibits
it.

Collision check (run before any code was written; gh pr list --repo AmaadMartin/adk-js --state open --limit 1000, 378 open PRs, then gh pr diff
on every plausibly adjacent one). Three open PRs overlap and a maintainer should
pick between them rather than merge blindly:

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 tests (both added alongside the existing ones; no existing test was edited,
skipped, weakened or deleted, and no fixture was changed):

  • dev/test/utils/agent_loader_test.ts"marks the ADK packages external so
    each agent does not embed a copy"
    . Follows the sibling test's pattern of
    inspecting (esbuild.build as Mock).mock.calls[0][0].
    npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts
    31 passed (30 before, all unmodified).
  • tests/integration/app_loader/app_loader_test.ts"compiles an agent
    without embedding the ADK runtime"
    . Pins the fix behaviourally rather than by
    timing (a duration assertion would itself be flaky): the compiled artifact
    must be under 64 KB. It runs against the unchanged discovery fixture, i.e. a
    real project-local npm install of @google/adk producing a .cjs
    artifact — the layout and module format a user project actually has. The
    margin is three orders of magnitude — 981 bytes with the fix, 5,902,465 bytes
    without — so the threshold is not fragile.

Mutation proof — each new test was run against the unfixed code and confirmed
to FAIL.
The mutation was deleting the two external entries from
dev/src/utils/agent_loader.ts:

  • unit test →
    AssertionError: expected [ 'sqlite3', 'better-sqlite3', …(11) ] to deeply equal ArrayContaining{…}
  • integration test → AssertionError: expected 5902465 to be less than 65536

The production diff appends two literals to an existing array and introduces no
new statement or branch, so new-line/branch coverage is satisfied by
construction and the repo-wide coverage.thresholds cannot move.

Full-file regression run (the gate that would catch a bad externalisation —
it exercises adk run in a real child process, which executes dev/dist, so
npm run build is mandatory after the source edit):

npm run build
rm -rf tests/integration/app_loader/*/node_modules \
       tests/integration/app_loader/*/package-lock.json
npx vitest run --project integration \
  tests/integration/app_loader/app_loader_test.ts --reporter=verbose

All 7 pass, from clean fixtures:

Test Result
app_ts / app_js / app_default CLI (real child process) pass, 4.72 s / 5.14 s / 4.89 s
should discover apps vs agents… pass, 1,992 ms (17,862 ms before)
should load App from directory entrypoint… pass, 1 ms
should synthesize App when loadApp()… pass, 1 ms
compiles an agent without embedding the ADK runtime (new) pass, 3 ms

Also run: npm run build (OK), npm run lint (clean), npm run format:check
(clean).

Two honest notes about neighbouring checks:

  • npx vitest run --project integration tests/integration/agent_loader/agent_dirname_test.ts
    fails with Hook timed out in 40000msand fails identically on the
    unmodified base commit
    (verified by stashing this diff and re-running). It
    is the same hard-coded 40 s cap on an npm install hook that this PR removes
    from app_loader_test.ts; that file is left alone here to keep the diff
    focused, and Fix: align integration install hooks on the project-wide hook timeout #405 already covers it. Nothing in this diff can affect an
    npm install.
  • npm run ts:check reports 281 pre-existing tsc errors — exactly 281 on
    the unmodified base too
    (verified the same way). ts:check is not part of
    the validation workflow, which runs build, test:coverage and lint.

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
  1. Discovery over the real loader, no mocks, against the discovery fixture
    with its own npm install in place — listApps() returned
    ["service_alpha","standalone_app"] and listAgents() returned
    ["service_alpha","service_beta","standalone_agent","standalone_app"],
    identical to before the change.
  2. Inspect the compiled artifact under the OS temp dir
    (adk_agent_loader/<uuid>/). It is app.cjs, 981 bytes, its directory
    listing is [ 'app.cjs', 'node_modules' ] with node_modules a symlink to
    the fixture's own installed tree, and the body contains a literal
    require("@google/adk") — i.e. it imports the runtime from the project's
    real install instead of inlining it.
  3. adk webnode dev/dist/esm/cli_entrypoint.js web tests/integration/app_loader/discovery --port 8791, then
    curl 'http://[::1]:8791/list-apps' returned
    ["service_alpha","service_beta","standalone_agent","standalone_app"] in
    105 ms.
  4. adk run child-process path — covered by the three CLI fixtures above;
    additionally npx @google/adk-devtools run agent.ts in
    tests/integration/agent_loader/import_meta_url answered
    I'm stubby model response!, confirming __dirname/import.meta.url
    rewriting still works with the ADK externalised.
  5. npm run lint && npm run format:check — both clean.

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 2 commits August 1, 2026 16:03
AgentFile.load() compiles each agent with esbuild packages: 'bundle', so a
four-line agent becomes a self-contained 5.9MB copy of the whole ADK runtime
that Node then has to evaluate. Loading a directory of N agents therefore
evaluates N private ADK copies: the AgentLoader discovery fixture measured
17.9s for listApps() on a fast Linux box, which is what makes the app_loader
integration test time out on the slower macOS and Windows CI legs.

Mark @google/adk and @google/adk-devtools external so the artifact imports the
runtime from the node_modules symlink the loader already creates beside it.
The compiled discovery artifact drops from 5,878,817 to 574 bytes and
listApps() from 17.9s to 69ms.
Every beforeAll/afterAll in this file passed TEST_EXECUTION_TIMEOUT (40s) as its
third argument, which lowers the budget below the integration project's
deliberate hookTimeout of 120s -- chosen because a cold, network-bound fixture
install has been measured at ~70s. Drop the argument from the four hooks that
run npm install or tear down its node_modules tree so they inherit 120s; every
it()'s own TEST_EXECUTION_TIMEOUT is untouched.

Also pin the externalization behaviourally rather than by timing: the compiled
discovery artifact must stay under 64KB. It is 981 bytes with the ADK left
external and 5,902,465 bytes without, so the threshold has three orders of
magnitude of margin and needs no flaky duration assertion.
@AmaadMartin
AmaadMartin force-pushed the fix/agent-loader-external-adk-runtime branch from 795a1e0 to c9b10db Compare August 1, 2026 23:53
@AmaadMartin AmaadMartin changed the title Fix: stop embedding the ADK runtime in every compiled agent file (17.9s -> 67ms discovery) Fix: stop embedding the ADK runtime in every compiled agent file Aug 1, 2026
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