Fix: stop the app_loader discovery test billing its fixture setup to the first it() (Part 1/2)

[AmaadMartin]

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):
   No public issue tracks this. The failure is observed on the macos-latest and windows-latest legs of the validation workflow, on branches unrelated to the test file — so it predates and is independent of any one change.
2. Or, if no issue exists, describe the change:

Problem: tests/integration/app_loader/app_loader_test.ts intermittently fails with Test timed out in 40000ms, and always on the same two cases:

• should discover apps vs agents across directories and standalone files
• should load App from directory entrypoint and expose App and rootAgent

while the third case in the same suite, should synthesize App when loadApp() is called on BaseAgent file, passes.

There are three separate defects behind that. This PR (Part 1/2) fixes the two in the test file; the third — the CI matrix cancelling its sibling legs — is #652, stacked on this branch, because it is a repo-wide workflow policy change and does not belong in a test-timeout fix.

(a) The test body did all the work; the assertions were free. beforeAll only ran npm install and constructed the loader; AgentLoader does everything else lazily, so the first test body (loader.listApps()) is what drove preloadAgents(). preloadAgents() calls AgentFile.load() for each of the four discovered entrypoints (dev/src/utils/agent_loader.ts:514, :536), and each load() is a full esbuild.build() with bundle: true, minify: true — so every fixture inlines the whole @google/adk dependency graph. Measured on an idle Linux workstation: 20511 ms in the first test, 1 ms each in the other two. 100% of the suite's cost was billed to one it(), against a 40000 ms budget.

The cascade to the second case has a specific cause: preloadAgents() guards re-entry with a plain boolean that is assigned only after its await Promise.all(...) resolves (dev/src/utils/agent_loader.ts:502), with no in-flight promise memo. When case 1 times out, Vitest fails the test but the pending preload keeps running with the flag still false, so case 2 starts a second complete set of four bundles competing with the first and times out too. By case 3 the first preload has settled and everything is a cache hit — hence 1 ms and a pass.

(b) The file-local 40 s constant was passed to the hooks, capping them below the project budget. vitest.config.ts:79 sets hookTimeout: 120000 on the integration project precisely because these hooks are install-heavy. Passing TEST_EXECUTION_TIMEOUT (40000) as the second argument to beforeAll/afterAll overrode that downwards. The three app_* hooks are the acute case: they genuinely need their fixture install, and measured cold on this workstation that install takes ~70 s — 1.75× the cap that was attached to it. That is not a flake, it is a hook that cannot pass from a clean checkout (see the mutation proof below, which reproduces Hook timed out in 40000ms on demand).

(c) One slow runner cancelled the other two. .github/workflows/validation.yaml runs its three-OS matrix without fail-fast: false, so a single timing-out leg cancels its siblings and the run produces no usable signal from the OSes that were fine. Fixed in the stacked Part 2/2, not here — it is a one-line repo-wide policy change with its own cost tradeoff (a genuinely broken PR now burns all three runners), and it deserves to be accepted or rejected on its own merits rather than riding along with a test fix.

Solution: move the one-time cost off the per-test budget and stop the file overriding the project's hook budget — rather than inflating any budget. vitest.config.ts is not touched and no global testTimeout is raised.

• beforeAll now warms the loader with a single await loader.preloadAgents();, so every listApps() / listAgents() / getAppFile() in the bodies below is a cache hit.
• One call, sequentially — deliberately not Promise.all([listApps(), listAgents()]), which would duplicate all four bundles for exactly the re-entrancy reason above.
• All four hooks in the file drop their explicit TEST_EXECUTION_TIMEOUT argument so they inherit the integration project's hookTimeout. The bug was this file overriding the project hook budget down to 40 s; the fix is to stop overriding it, not to override it with a different number (see the deviation note below).
• The loader is constructed as the first statement of beforeAll. That is what makes the old teardown cascade unreachable: previously npm install ran ahead of the assignment, so a slow install left loader undefined and afterAll died with TypeError: Cannot read properties of undefined (reading 'disposeAll'), burying the real hook error. The constructor only registers process.on handlers and cannot reject, so loader is now always assigned by the time afterAll runs.
• The three it() bodies and their assertions are unchanged, and TEST_EXECUTION_TIMEOUT stays 40000 on them. Warmed, the discovery bodies run in ~1 ms, so the budget still catches a future change that makes discovery expensive again.

The discovery fixture npm install is removed (and with it the matching node_modules / package-lock.json teardown, and the fixture's now-dead devDependencies). Three things worth stating explicitly, since removing an install inside a timeout fix looks unrelated:

1. Why it is safe to delete. discovery/package.json declares no scripts. Unlike its app_ts / app_js / app_default siblings — whose "start": "npx @google/adk-devtools run app.ts" genuinely needs a local node_modules/.bin, which is why those three installs stay — nothing in this fixture ever spawns a subprocess; it is only read in-process by AgentLoader, whose esbuild pass resolves @google/adk by walking up to the workspace-root node_modules/@google/adk -> core symlink. CI populates core/dist with npm run build before npm run test:coverage (.github/workflows/validation.yaml). The install was copy-paste from the sibling describe, and it was not free: it materialised a tree for preloadAgents() to readdir + stat as a candidate agent directory and then recursively delete.
2. The fixture's devDependencies go with it. The two file: links to core/ and dev/ were only ever consumed by that install. Left behind they are dead metadata that implies an install which no longer happens — and there is no teardown left to clean up after anyone who runs one by hand.
3. What it does change. linkProjectNodeModules() (dev/src/utils/agent_loader.ts:616-641) now takes its undefined early return for this fixture. getProjectNodeModulesDir() looks for node_modules beside the nearest package.json only — that is discovery/package.json, which no longer has one — so the temp bundle directory gets no node_modules sibling symlinked in. This is harmless under the default bundle: true (everything the entrypoints need is already inlined), and all six cases pass on all three CI legs, but it is a real change in which branch the integration test exercises and should be stated rather than discovered.

Verification gate result: the removal is kept — with the fixture in fresh-checkout state (no node_modules, no package-lock.json) all six cases pass.

discovery/package.json itself is deliberately kept. It is load-bearing beyond dependency declaration: getTypeFromPackageJson() (dev/src/utils/agent_loader.ts:591-614) walks up from the entry file and stops at the first package.json. Deleting it would reach the repo-root package.json with "type": "module", flipping the fixtures from CJS to ESM output and silently changing what the test exercises.

Deviation from the plan, stated explicitly

The design called for a named HOOK_TIMEOUT = 120000 constant passed to the hooks, mirroring tests/integration/build_setup/build_setup_test.ts:15-24. I removed the hook arguments instead of replacing them, and this is the only substantive place I departed from the design. The design's postcondition — "no beforeAll/afterAll in the file is capped below the project's hookTimeout of 120 000 ms" — is met either way; the difference is where the number lives.

The design's own justification for the value is that 120000 "matches INTEGRATION_HOOK_TIMEOUT_MS in vitest.config.ts" — i.e. the number the project already applies to this file (vitest.config.ts puts tests/integration/**/*_test.ts in the integration project and sets hookTimeout: INTEGRATION_HOOK_TIMEOUT_MS). Passing it explicitly is a no-op that hardcodes a second copy of the number, free to drift from the config — which is the same failure shape as the bug being fixed, just pointed the other way. Dropping the argument gets the identical budget with one source of truth, and keeps all four hooks in the file consistent instead of leaving the discovery pair inheriting while the app_* pair restates.

The build_setup_test.ts precedent is weaker than it looks: it landed in google#549 at 2026-07-30 20:01:22, thirty seconds before google#548 added the project-wide hookTimeout at 20:01:52. The two were independent PRs merged back to back, so the duplication is an artifact of that collision rather than a considered idiom.

A comment on TEST_EXECUTION_TIMEOUT now says what the constant is for and why the hooks do not take it, so the 40000 does not get re-attached to a hook by a future edit.

Verified empirically rather than assumed, in both directions: inserting a temporary 45 s sleep into the discovery hook (on top of the ~20 s warm-up, so ~65 s total) passes, where the same hook on unmodified main reports Hook timed out in 40000ms; and the app_* install hook completes a ~70 s cold install that fails at 40 s with the argument restored (mutation C below). The override is genuinely gone and the larger project budget applies.

Two smaller departures, both narrowing the diff: the design suggested warming via listAgents(), but preloadAgents() is public and is literally the method that sets agentsAlreadyPreloaded, so calling it directly removes a layer of indirection the comment would otherwise have to explain; and the design's optional loader?.disposeAll() is not included, because moving the constructor to the first statement of beforeAll (above) makes the undefined-loader case unreachable, and loader is typed non-nullable.

Collision check (open PRs on this fork)

Recorded as required; gh pr list --limit 1000 plus gh pr diff --name-only on everything adjacent (--limit 100 silently truncates on this fork). Twelve open PRs touch this file. The closest:

• Fix: attribute cold AgentLoader discovery cost to beforeAll to stop macOS CI flake #260 fix/app-loader-discovery-timeout-flake and Fix: bill app_loader discovery fixture compilation to beforeAll instead of the first test #560 fix/app-loader-integration-test-timeout both add the same warm-up to beforeAll. This PR is the narrower form of the same fix: Fix: attribute cold AgentLoader discovery cost to beforeAll to stop macOS CI flake #260 also raises all four hooks to a local FIXTURE_SETUP_TIMEOUT = 180000 (a third distinct copy of a budget the project config already sets), and Fix: bill app_loader discovery fixture compilation to beforeAll instead of the first test #560 additionally reflows all six it()s and both CLI hooks. Either can supersede this one — the fix is the same idea and I make no claim to priority.
• Test: drop the npm install from the AgentLoader discovery integration fixture #276 feat/trim-integration-test-npm-installs drops the same fixture install, but also deletes discovery/package.json, which flips the fixtures from CJS to ESM; it compensates with a new expect(path.extname(...)).toBe('.mjs') assertion. This PR keeps the package.json and leaves what the suite exercises unchanged. These two conflict and only one should land.
• Fix: stop the app_loader discovery test billing its setup to a test, and make a premature test-server exit self-diagnosing #545 does the same billing fix as part of a much larger seven-file Windows/CLI-diagnostics change.
• Fix: raise app_loader integration-test timeout to 60s to stop CI flake #256 raises the budget to 60 s instead of moving the cost — the explicit non-goal here.
• Fix: bound AgentLoader bundle imports so app_loader discovery test stops timing out on Windows/macOS CI #247 / Perf: scope the agent-loader esbuild plugin filter and skip node_modules during discovery #264 / Fix: decouple minification from --bundle in the dev agent loader #328 / Fix: stop embedding the ADK runtime in every compiled agent file #480 / Perf: externalize third-party packages when compiling agent files (~54x faster AgentFile.load) #285 attack the underlying bundling cost in dev/src/ instead of the test. They are complementary; if one lands the warm-up simply gets cheaper.
• Adjacent budget PRs Fix: align integration install hooks on the project-wide hook timeout #405 / Fix: let integration install/server-start hooks inherit the project hook timeout #554 / Fix: set project-wide integration hookTimeout/testTimeout in vitest.config.ts #118 rework integration timeouts across several files and will conflict textually.

Note on this branch's own history: an earlier revision of this PR additionally changed the discovery loader to {compile: true, bundle: false} and cherry-picked the dev/src/utils/agent_loader.ts fix that bundle: false depends on (owned by #275). That has been dropped. The production change belongs to #275, and touching dev/src/ widens the blast radius of what should be a CI-stability fix — the missing in-flight dedup in preloadAgents() is real but is tracked separately. This PR touches no production source: 2 files, +9/-19 across the test and its fixture manifest. The workflow line lives in the stacked Part 2/2.

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.

This change adds no production code — no line in core/src, dev/src, or integrations/src is added or modified — so there is no new code to cover and no new unit test to write. The changed file is the test. The existing unit coverage of the code paths involved must keep passing untouched, and does:

npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts
  -> Test Files 1 passed (1) | Tests 30 passed (30)

npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts --reporter=verbose
  -> Test Files 1 passed (1) | Tests 6 passed (6)     Duration 287.48s
     ✓ App entrypoint with app_ts      > should run app ...     5776ms
     ✓ App entrypoint with app_js      > should run app ...     6301ms
     ✓ App entrypoint with app_default > should run app ...     5440ms
     ✓ should discover apps vs agents across directories ...       4ms
     ✓ should load App from directory entrypoint ...               1ms
     ✓ should synthesize App when loadApp() ...                    1ms

Per-test durations showing the cost has left the test bodies:

case	before	after
should discover apps vs agents across directories and standalone files	20511 ms	3–4 ms
should load App from directory entrypoint and expose App and rootAgent	1 ms	1 ms
should synthesize App when loadApp() is called on BaseAgent file	1 ms	1 ms

Proof the change does something. This is a timing bug, so the mutations are the budgets themselves. Each was applied to the fixed tree, run, and reverted.

Mutation A — the warm-up. Note the design's suggested recipe (set TEST_EXECUTION_TIMEOUT to 2000 on unfixed code) cannot produce the CI signature, because that same constant also budgeted the npm install hooks, which fail first. Reproducing it needs a budget below the ~20 s preload but above zero. At 10000, with only the await loader.preloadAgents(); line removed and everything else in place, the suite fails:

× ... should discover apps vs agents across directories and standalone files 10005ms
  -> Test timed out in 10000ms.
× ... should load App from directory entrypoint and expose App and rootAgent 10001ms
  -> Test timed out in 10000ms.
× ... should synthesize App when loadApp() is called on BaseAgent file 13922ms
  -> Test timed out in 10000ms.

Tests  3 failed | 3 skipped (6)

Putting that single line back, with the budget still at 10000, turns it green:

✓ ... should discover apps vs agents across directories and standalone files 5ms
✓ ... should load App from directory entrypoint and expose App and rootAgent 1ms
✓ ... should synthesize App when loadApp() is called on BaseAgent file 1ms

Tests  3 passed | 3 skipped (6)

An earlier run of the same mutation reproduced the reported CI pattern exactly — two failures and a pass, with the third case surviving on a cache hit:

× ... should discover apps vs agents across directories and standalone files 10007ms
  -> Test timed out in 10000ms.
× ... should load App from directory entrypoint and expose App and rootAgent 11672ms
  -> Test timed out in 10000ms.
✓ ... should synthesize App when loadApp() is called on BaseAgent file 1ms

Reported honestly: whether that third case survives is machine-load dependent, which is the nature of the flake being fixed. What is stable across runs is that the mutation always fails and the fix always passes. The second case overrunning its own budget (11672 ms against 10000 ms) is the cascade itself — it re-entered preloadAgents() and started a duplicate set of bundles.

_Mutation C — the app\__hook budget.* This one needs no invented number: restore the removed}, TEST_EXECUTION_TIMEOUT);on the install hook and run one case from a cold fixture (nonode_modules, no package-lock.json):

npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts -t "app_ts"

 FAIL  ... > App loader CLI integration > App entrypoint with app_ts
Error: Hook timed out in 40000ms.
 ❯ tests/integration/app_loader/app_loader_test.ts:34:7
     35|         await execAsync('npm install', {cwd: projectPath});
     36|       }, TEST_EXECUTION_TIMEOUT);

 Test Files  1 failed (1)      Tests  6 skipped (6)      Duration 47.86s

With the argument dropped again and the fixture reset to the same cold state, the identical command passes:

 ✓ ... > App entrypoint with app_ts > should run app via package.json start script ... 5536ms
 Test Files  1 passed (1)      Tests  1 passed | 5 skipped (6)      Duration 83.73s

83.73 s total minus the 5.5 s test body puts the cold npm install at ~70 s — which is why the 40 s cap failed outright rather than intermittently, and why the fix is to let the hook use the 120 s budget vitest.config.ts already defines for exactly this. Both hooks were left in the reverted (fixed) state; git diff confirms it.

Change (c), the workflow. A matrix cancellation cannot be reproduced locally, so it is verified structurally — the file parses and the strategy resolves to {"fail-fast":false,"matrix":{"os":["ubuntu-latest","windows-latest","macos-latest"]}}, with the OS list and the env block byte-identical to before. Note npm run format:check globs **/*.ts only and this workflow file is not Prettier-formatted on main; its existing quoting is therefore left exactly as-is rather than reflowed into the diff.

Failure path exercised. On a genuinely cold checkout the unmodified file fails deterministically here, which is what motivated moving the loader construction to the first statement of the hook:

Error: Hook timed out in 40000ms.       <- discovery beforeAll, npm install
TypeError: Cannot read properties of undefined (reading 'disposeAll')
  131|     await loader.disposeAll();   <- teardown masks the real hook error
Tests  6 skipped (6)

Neighbour suite. tests/integration/agent_loader/agent_dirname_test.ts is unmodified and was checked as a guard against a fixture-resolution regression. It fails on this workstation with Hook timed out in 40000ms on its own fixture install — verified pre-existing: git stash-ing this change and re-running reproduces the identical failure on an unmodified tree. That file uses the same 40000-everywhere pattern and is tracked separately; it is deliberately not touched here.

npx eslint and npx prettier --check are clean on the changed test file. npx tsc --noEmit reports 281 errors both with and without this change (all in core/test/**, none mentioning any file in this diff) — a pre-existing local state, confirmed by stashing the change and re-counting.

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

# Fresh-checkout state: no fixture may carry an install of its own.
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 six cases pass; the three discovery cases report single-digit milliseconds, the one-time bundling cost is visible in the discovery beforeAll, and the three app_* installs now complete inside the project hook budget instead of being cut off at 40 s. Afterwards, git status --porcelain tests/integration/app_loader is empty apart from the intended edit — the suite no longer creates or deletes discovery/node_modules.

To see the attribution fix directly, compare those per-test durations against the same command 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.