Skip to content

Fix: de-duplicate concurrent AgentLoader discovery scans - #664

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/app-loader-discovery-macos-timeout
Open

Fix: de-duplicate concurrent AgentLoader discovery scans#664
AmaadMartin wants to merge 2 commits into
mainfrom
fix/app-loader-discovery-macos-timeout

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 5, 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):
    N/A — no public issue tracks this flake.
  2. Or, if no issue exists, describe the change:
    Problem: AgentLoader.preloadAgents() has no in-flight guard. It sets
    agentsAlreadyPreloaded = true only after its Promise.all resolves, so every
    caller that arrives while a scan is running starts a competing full scan.

A discovery pass is expensive: for each candidate it constructs an AgentFile
and calls load(), which runs a full esbuild.build() (bundle: true,
minify: true, packages: 'bundle') and then dynamically import()s the
result. A duplicate scan therefore re-bundles and re-imports every entrypoint.
Worse, the second scan's AgentFile instances overwrite the first scan's in
preloadedAgents, so the displaced ones are never disposed and their temp
directories leak — disposeAll() can only reach the survivors.

Two callers hit this in practice:

  • dev/src/server/adk_api_server.tslistAgents()/getAgentFile() are
    reached from request handlers, so two requests arriving before the first scan
    finishes each trigger a full rebuild of every agent.
  • tests/integration/app_loader/app_loader_test.ts on the macos-latest CI
    leg — the first discovery test pays the whole cold-discovery cost; when it is
    killed by the per-test timeout the scan it started is still running and the
    flag is still false, so the next test starts a second concurrent scan.
    That is the mechanism behind "1 test timed out on one run, 2 on the immediate
    rerun of the same commit". For public repositories macos-latest is a 3-vCPU
    / 7 GB M1 runner against 4 vCPU / 16 GB for the other two legs
    (GitHub-hosted runners reference),
    which is why only that leg loses the race.

Solution: memoize the running scan so concurrent callers join it instead of
starting a competing one.

this.preloadInFlight ??= this.scanAgents().catch((e: unknown) => {
  this.preloadInFlight = undefined;
  throw e;
});

return this.preloadInFlight;

The scan body moved verbatim into a private scanAgents(); preloadAgents()
keeps its public Promise<void> signature and agentsAlreadyPreloaded keeps
its exact meaning (set only on successful completion). Two details matter:

  • A rejected scan is discarded, not cached, so a later call retries from
    scratch rather than replaying a stale rejection forever. Every joined caller
    still receives the rejection.
  • invalidateAll() drops the in-flight scan, so a file-change reload during
    a scan is not silently swallowed by a caller joining pre-change results.

I chose .catch over .finally deliberately: on success the slot is not
cleared, which means a scan that completes after an invalidateAll() cannot
clobber the replacement scan a later caller already started. agentsAlreadyPreloaded
short-circuits the field before it can be read again, and invalidateAll()
clears it.

Collision check (562 open PRs on the fork)

The elaborated task had three parts. I checked every open PR on this fork by
touched file (GraphQL pullRequests(states: OPEN) { files }) before writing any
code, and shipped only the part that is not already open:

Task part Status
dev/src/utils/agent_loader.ts concurrent-scan de-duplication Shipped here. 25 open PRs touch this file; none changes preloadAgents concurrency. The nearest is #633, which memoizes discovery as part of a wholesale lazy-discovery restructure (it deletes preloadAgents and makes listAgents() lazy) — a different, much larger design.
Move the discovery cost into beforeAll + a justified HOOK_TIMEOUT in app_loader_test.ts Dropped — already open as #506 (Part 1/2), and again in #560, #260, #545 and #235. #506 makes exactly the change the task described, including the await loader.preloadAgents() warm-up.
fail-fast: false in .github/workflows/validation.yaml Dropped — already open as #652 (Part 2/2 of the #506 stack), and again in #235.

Filing a sixth copy of the test/CI changes would have been pure duplicate work,
so this PR is scoped to the production defect. It is complementary to #506/#652
and touches no file they touch, so it can merge in any order relative to them.
Branched from main rather than stacked, for that reason.

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.

Three new cases in dev/test/utils/agent_loader_test.ts. No existing test was
modified, deleted, skipped, or weakened
— the file goes from 30 to 33 tests,
and the two tests that read agentsAlreadyPreloaded directly still pass
unchanged.

npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts
  ✓ 33 passed (33)

npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts \
  dev/test/cli/cli_run_test.ts dev/test/cli/cli_deploy_cloud_run_test.ts \
  dev/test/cli/cli_deploy_agent_engine_test.ts \
  dev/test/conformance/yaml_agent_loader_test.ts
  ✓ 98 passed (98)      # the callers of preloadAgents()

Proof each new test can fail. Every new line of production code was mutated
separately and each mutation killed exactly one test — a clean 1:1 mapping, no
test passing on a bug:

Mutation Failing test Message
Drop the memo (return this.scanAgents();) runs a single discovery pass for concurrent preloadAgents() calls expected 6 to be 3 (each entrypoint compiled twice)
Drop the .catch clear (??= this.scanAgents();) re-scans after a failed discovery pass instead of replaying its rejection promise rejected "Error: compile failed" instead of resolving
Drop preloadInFlight = undefined from invalidateAll() starts a fresh scan when invalidateAll is called during a scan expected 3 to be 6 (the replacement joined the discarded scan)

Coverage of the changed lines (v8, --coverage.include='dev/src/utils/agent_loader.ts'):
every new statement and branch is executed — the ??= both-ways, the .catch
body, and the invalidateAll() reset. No coverage suppression was added and no
structure was changed to chase a number.

The assertions deliberately compare compiled entrypoints against their distinct
set rather than hardcoding "3 builds", so they stay correct if the unit fixture
gains or loses an entrypoint.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

1. Real, unmocked proof of the fix (the unit tests mock esbuild; this does
not). From a built tree, against the real tests/integration/app_loader/discovery
fixture — two concurrent listApps() calls, counting the compiled-output
directories the loader actually creates under os.tmpdir()/adk_agent_loader:

cd tests/integration/app_loader/discovery && npm install && cd -
npm run build
node -e "…new AgentLoader(discoveryFixture); Promise.all([l.listApps(), l.listApps()])…"
before (main) after
compiled output dirs created 8 (two scans × four entrypoints) 4
dirs still on disk after disposeAll() 4 leaked 0
wall clock for the two concurrent calls 5870 ms / 5728 ms 2941 ms / 2960 ms

Both calls return the same correct result in both cases
(apps = [service_alpha, standalone_app],
agents = [service_alpha, service_beta, standalone_agent, standalone_app]) — the
old code was correct, just doing the work twice and orphaning half of it.

2. Integration suites (unchanged by this PR, run to prove no collateral
damage):

npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts
  ✓ 6 passed — discovery test 18931ms, then 1ms and 6ms
npx vitest run --project integration tests/integration/agent_loader/agent_dirname_test.ts
  ✓ 3 passed

Note the shape of the first run: the first discovery test still absorbs the
whole cold cost (18.9 s here) because moving that cost into beforeAll is
#506's change, not this one. This PR removes the second scan that the timeout
cascade triggers, which is what turns one slow test into two failing tests.

3. Static checks on the pushed commit:

npm run lint          # clean
npm run format:check  # clean
npm run build         # clean
npm run ts:check      # 281 errors — all pre-existing

ts:check is already red on main: 281 errors with my change, 281 with the
branch stashed, none in either file I touched. Several open PRs are fixing that
separately; it is not this change.

4. CI on the pushed commit — the validation workflow ran the full
run-tests job (install, secretlint, build, test:coverage, lint, format,
docs) on all three matrix legs and all three passed, including the
macos-latest leg this change targets:

run-tests (ubuntu-latest)   pass  5m47s
run-tests (macos-latest)    pass  5m40s
run-tests (windows-latest)  pass  9m12s

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.

Reviewer notes

  • One as unknown as in the new tests, to call the private
    invalidateAll(): (loader as unknown as {invalidateAll: () => void}). This
    follows the in-file precedent (resets preload cache when invalidateAll is called already does exactly this) and there is no public route to the case
    under test — invalidateAll() is only reachable from the fs watcher, and
    startWatching() runs at the end of a scan, so a watcher-driven
    invalidation can never land mid-scan. I removed the second private access the
    test originally had (agentsAlreadyPreloaded) and assert the same property
    through the public API instead: a following listAgents() must not trigger a
    third compile pass.
  • No any, no @ts-expect-error, no eslint-disable, no coverage suppression
    anywhere in the diff.
  • Adding a third AgentLoader-constructing test tips this file past Node's
    default listener limit
    , so the run now prints
    MaxListenersExceededWarning: … 11 exit listeners added to [process]. That is
    a pre-existing defect — AgentLoader's constructor registers five
    process.on(...) handlers per instance with no removal path — surfaced, not
    caused, by this PR; Fix: make AgentLoader process exit/signal handlers opt-in and removable #511 fixes the root cause. Nothing fails.
  • Out of scope on purpose: making listAgents() lazy (it deliberately returns
    only entries that load successfully, which handles AgentFileLoadingError in directory loading pins) and switching packages: 'bundle' to 'external'
    (changes runtime module resolution for every adk web / adk run user).

Amaad Martin added 2 commits August 4, 2026 17:52
preloadAgents() only set agentsAlreadyPreloaded after Promise.all resolved
and had no in-flight guard, so every caller arriving during a scan started a
competing one. Each extra scan re-bundles and re-imports every entrypoint,
and its AgentFile instances overwrite the first scan's in preloadedAgents,
so the displaced ones are never disposed and their temp directories leak.

Memoize the running scan so concurrent callers join it. A rejected scan is
discarded so a later call retries, and invalidateAll() drops it so a
file-change reload is not swallowed.
Replaces the private agentsAlreadyPreloaded read with a listAgents() call
that must not trigger a third compile pass, so the case is pinned without
reaching into the loader's internals.
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