Skip to content

Fix: attribute cold AgentLoader discovery cost to beforeAll to stop macOS CI flake - #260

Closed
AmaadMartin wants to merge 2 commits into
mainfrom
fix/app-loader-discovery-timeout-flake
Closed

Fix: attribute cold AgentLoader discovery cost to beforeAll to stop macOS CI flake#260
AmaadMartin wants to merge 2 commits into
mainfrom
fix/app-loader-discovery-timeout-flake

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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:
    Problem: tests/integration/app_loader/app_loader_test.ts'should discover apps vs agents across directories and standalone files' intermittently fails on the run-tests (macos-latest) leg of .github/workflows/validation.yaml with Error: Test timed out in 40000ms., and passes on an immediate re-run of the same commit.

This is a cost mis-attribution, not a slow assertion. AgentLoader discovery is lazy: the constructor (dev/src/utils/agent_loader.ts:365) only assigns fields and registers exit handlers, so the beforeAll that builds the loader does almost nothing. The first thing the test body does is await loader.listApps(), and listApps() begins with await this.preloadAgents() — so the test pays for the entire cold discovery pass: four AgentFile.load() calls that each run a full esbuild.build() with bundle: true and minify: true (agent_loader.ts:181, :188-189), inlining the whole @google/adk dependency graph, followed by a dynamic import() of each emitted artifact.

That is why this test flakes and the two after it never do: preloadAgents() short-circuits on agentsAlreadyPreloaded and AgentFile.load() short-circuits on its cached result, so the following tests inherit a warm loader and run in ~1 ms. All the cost is concentrated in whichever test runs first.

Solution: Move the one-time cost to where it belongs — warm the loader with await loader.preloadAgents() in the discovery beforeAll — and give the install/IO-bound fixture hooks their own FIXTURE_SETUP_TIMEOUT budget, sized for the work they actually perform.

Every it(...) deliberately keeps the existing 40 s TEST_EXECUTION_TIMEOUT. After this change the discovery tests are pure assertions against a warm loader, so if one ever needs more than 40 s again that is a real regression and should still fail loudly. The alternative — simply raising the test timeout until the failure stops — would have hidden that signal.

Measured on one machine, same command, same load, before vs. after (npx vitest run --project integration --reporter=verbose tests/integration/app_loader/app_loader_test.ts):

Test Before After
should discover apps vs agents across directories and standalone files 39587 ms 3 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

The baseline run reproduces the flake exactly: the test passed with only 413 ms of headroom under its 40 000 ms budget. Any slower runner — which macos-latest is — crosses the line.

Note this does not make CI faster, and is not intended to: the same work happens, just in the setup phase instead of inside an assertion. Total wall-clock time for the file was unchanged (60.7 s before vs. 65.0 s after for the discovery block in isolation). One intentional side effect: a genuinely broken fixture now surfaces as a beforeAll failure with its original stack trace, rather than as a confusing assertion failure in whichever test ran first.

This is a test-only change. No file under core/src, dev/src, or integrations/src is touched, no assertion is added, removed, or weakened, no test is skipped or retried, and no any / @ts-ignore / @ts-expect-error / eslint-disable is introduced (preloadAgents() is a public method returning Promise<void>).

It is also independent of the open PR that sets a project-wide hookTimeout/testTimeout in vitest.config.ts. A project-level timeout is only a default; an explicit third argument to it(...) / second argument to beforeAll(...) overrides it, and every test and hook in this file already passes one explicitly. The two changes touch disjoint files and cannot conflict.

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.

No new unit test: this change adds no production code, only re-attributes existing work between a test and its setup hook, so there is no new line or branch to cover. Asserting on a test-harness timeout constant would have no value. The existing unit suite for the code under test was run unchanged and stays green:

$ npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts
 ✓ |unit:dev| dev/test/utils/agent_loader_test.ts (30 tests) 1445ms
   Test Files  1 passed (1)
        Tests  30 passed (30)

[x] All unit tests pass locally.

Manual End-to-End (E2E) Tests:
Setup: npm install && npm run build at the repository root. The build is mandatory — the fixtures resolve @google/adk and @google/adk-devtools through file: links into core/ and dev/, which serve from dist/.

  1. Full affected file, all six tests, with per-test durations:

    $ npx vitest run --project integration --reporter=verbose \
        tests/integration/app_loader/app_loader_test.ts
    
     ✓ App entrypoint with app_ts > should run app via package.json start script and get responses 6647ms
     ✓ App entrypoint with app_js > should run app via package.json start script and get responses 6843ms
     ✓ App entrypoint with app_default > should run app via package.json start script and get responses 5693ms
     ✓ AgentLoader discovery ... > should discover apps vs agents across directories and standalone files 3ms
     ✓ AgentLoader discovery ... > should load App from directory entrypoint and expose App and rootAgent 1ms
     ✓ AgentLoader discovery ... > should synthesize App when loadApp() is called on BaseAgent file 1ms
    
     Test Files  1 passed (1)
          Tests  6 passed (6)
    
  2. Baseline comparison — reverting only this commit's test file and re-running the identical command reproduces the flake's cause, with the first discovery test at 39587 ms (413 ms under its 40 s budget) while the two after it report 1 ms each. That asymmetry is the bug; it disappears after the change.

  3. Timeout-wiring sanity check — temporarily setting FIXTURE_SETUP_TIMEOUT to 1 makes the run fail with Error: Hook timed out in 1ms. attributed to the discovery beforeAll, confirming the budget is wired to the hook and not to the test. (Probe reverted; not part of the diff.)

  4. Repository gates that CI enforces, run on the exact pushed commit:

    $ npx prettier "tests/integration/app_loader/app_loader_test.ts" --check
    All matched files use Prettier code style!
    $ npx eslint "tests/integration/app_loader/app_loader_test.ts"
    (clean)
    
  5. git status --porcelain is empty after the run — no fixture node_modules/, package-lock.json, or dist/ artifacts leaked into the tree.

One unrelated pre-existing issue was found while verifying and deliberately left out of this diff to keep it reviewable: tests/integration/agent_loader/agent_dirname_test.ts:29 runs npm install for each of its three fixtures inside a beforeAll budgeted at the same 40 s, and fails with Hook timed out in 40000ms on a sufficiently loaded machine. It is the identical anti-pattern in a file this PR does not touch, and is tracked separately.

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 July 29, 2026 11:11
…eAll

The discovery test in app_loader_test.ts intermittently timed out at 40s on
the macos-latest CI leg. AgentLoader discovery is lazy: the constructor does
no work, so the first listApps() call in the test body paid for four
concurrent esbuild bundle-and-minify passes over the whole ADK dependency
graph. The cost belonged to setup, not to an assertion.

Warm the loader with preloadAgents() in the discovery beforeAll, and give the
four install/IO-bound fixture hooks their own FIXTURE_SETUP_TIMEOUT budget.
Every it() keeps the existing 40s TEST_EXECUTION_TIMEOUT, so a discovery
assertion that ever needs more than that still fails loudly.
Address simplicity review: the rationale for FIXTURE_SETUP_TIMEOUT was 8
lines of prose for a one-line constant, and it described the loader warm-up
that only one of the five hooks performs. Keep that detail at the call site
where it applies and state the budget's rationale in one sentence.
This was referenced Jul 29, 2026
@AmaadMartin

Copy link
Copy Markdown
Owner Author

Close as superseded by #506. Same warm-up in beforeAll, but this adds a third local copy of a hook budget that vitest.config.ts already sets to 120000 ms. That duplicated constant is the drift risk #506 removes.

@AmaadMartin AmaadMartin closed this Aug 6, 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