Skip to content

Fix: make the AgentLoader preload memo the only record of a completed scan (stacked on #664) - #674

Open
AmaadMartin wants to merge 3 commits into
fix/app-loader-discovery-macos-timeoutfrom
fix/agent-loader-concurrent-preload-dedup
Open

Fix: make the AgentLoader preload memo the only record of a completed scan (stacked on #664)#674
AmaadMartin wants to merge 3 commits into
fix/app-loader-discovery-macos-timeoutfrom
fix/agent-loader-concurrent-preload-dedup

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

  2. Or, if no issue exists, describe the change:

Stacked on #664. Collision check: I ran gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 and diffed every adjacent PR. #664 already de-duplicates concurrent preloadAgents() callers, so I did not reimplement it — this PR is the remaining delta on top of it. #633 restructures the same method further; whichever lands first, the other rebases.

Problem: #664 keeps the agentsAlreadyPreloaded flag next to its new promise memo. A scan sets that flag after invalidateAll() cleared it, so an invalidation that lands during a scan is swallowed and the next request is served from the pre-change scan. #664 also clears the memo from any rejected scan, so a scan that fails after being superseded discards the results of the replacement scan that a later caller already started.

Solution: I deleted the flag. The settled preloadInFlight promise already records that a scan completed, and invalidateAll() clears it, so one field is the only source of truth. A rejected scan clears the memo only while it is still the current one.

Two disclosures.

The tests reach invalidateAll through one as unknown as {invalidateAll: () => void} cast, hoisted into a single invalidateAll(loader) helper. The method is private and only the fs.watch callback calls it, and driving a real watcher event would be timing-dependent across the CI matrix. I did not widen the method to public.

This PR does not close the leak of superseded AgentFile objects. A scan that invalidateAll() supersedes keeps writing into preloadedAgents after the record was emptied, and the replacement scan overwrites those entries without disposing them, so their temp directories survive until the next invalidation. Cancelling a superseded scan needs a generation guard, which is a separate change.

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.

Command: npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts — 35 passed. I ran it 8 times to check for flakes.

Two new tests:

  • does not reuse a scan that was invalidated while it was in flight
  • keeps the replacement memo when a superseded scan fails

Every changed line and branch in preloadAgents() and invalidateAll() is covered (v8 JSON report, no uncovered entry in the changed region).

Mutation results. Each mutation was applied alone and the suite re-run:

  1. Restore the source to Fix: de-duplicate concurrent AgentLoader discovery scans #664 exactly: does not reuse a scan that was invalidated while it was in flight fails with AssertionError: expected 3 to be greater than 3.
  2. Drop the this.preloadInFlight === scan guard: keeps the replacement memo when a superseded scan fails fails with AssertionError: expected 9 to be 6.
  3. Stop clearing preloadInFlight in invalidateAll(): four tests fail, including the updated resets preload cache when invalidateAll is called.

One existing test was rewritten, not added to: resets preload cache when invalidateAll is called (simulates file-change reload). I rewrote it because it asserted the removed agentsAlreadyPreloaded flag, so it cannot compile against this change. It now pins the same behaviour through the public listAgents() path, by asserting that the next request recompiles. Its name and intent are unchanged, and mutation 3 above shows it still fails when the fix is removed. No test was deleted, skipped or weakened.

Manual End-to-End (E2E) Tests:

  1. Put two or three agents in a directory and run adk web <agents-dir> --reload_agents.
  2. Issue overlapping requests before the first one settles: for i in 1 2 3 4; do curl -s localhost:8000/list-apps & done; wait. The responses match and each agent is compiled once.
  3. Edit an agent file while a request is in flight. The next request serves the edit.

CI does not run on this PR: .github/workflows/validation.yaml triggers on pull_request: branches: [main] and this PR targets #664's branch. Local validation on commit be3200ab: npm run build passed, npm run lint passed, npm run format:check passed, and the targeted suite passed. dev/test/cli/cli_create_test.ts has one failure that also reproduces without this change; it is unrelated.

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 3 commits August 5, 2026 03:27
The `agentsAlreadyPreloaded` flag survives an invalidation that lands
while a scan is in flight: the scan sets the flag after `invalidateAll()`
cleared it, so the next request is served from the pre-change scan.

Delete the flag. The settled `preloadInFlight` promise already records
that a scan completed, and `invalidateAll()` clears it. A rejected scan
now clears the memo only while it is still the current one, so a scan
superseded by an invalidation cannot discard its replacement's results.
`mockRejectedValueOnce` failed whichever scan issued the first compile,
so the replacement scan failed instead of the superseded one on about
one run in three. The test now controls the first compile with a
deferred, and fails it only after the replacement owns the memo.
Three tests repeated the same cast to reach the private invalidateAll,
so one typed helper now holds it. The helper that counts compiles moves
above its first caller, and the memo comment drops the claim about a
leak this change does not close.
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