Fix: mark AgentFile disposed even when nothing was compiled - #725
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: mark AgentFile disposed even when nothing was compiled#725AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
AgentFile.dispose() set the disposed flag inside the cleanupFilePath
branch. cleanupFilePath is only assigned when load() compiles, so an
AgentFile built with {compile: false, bundle: false} was never marked
disposed. getFilePath() kept returning a path after dispose(), and an
"await using" scope left a live object behind.
Hoist the flag out of the branch. The unlink and removeFolder cleanup
stays guarded by cleanupFilePath, so an uncompiled instance still does
no filesystem work and the user's original agent file is untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem:
AgentFile.dispose()sets thedisposedflag inside theif (this.cleanupFilePath)branch.load()only assignscleanupFilePathwhen it compiles, so anAgentFilebuilt with{compile: false, bundle: false}is never marked disposed.getFilePath()then keeps returning a path afterdispose(), and anawait usingscope leaves a live object behind. The CLI reaches this through--compile false --bundle false, andAdkApiServerOptions.agentFileLoadOptionsreaches it as well.Solution: I hoisted
this.disposed = trueout of the branch, sodispose()marks the instance disposed on its first call. TheunlinkandremoveFoldercleanup stays guarded bycleanupFilePath, so an uncompiled instance still does no filesystem work and the user's original agent file is untouched. The flag is set before theawait, so a failedunlinkcannot leave a usable instance.The production change is one moved line:
if (this.disposed) { return; } + this.disposed = true; if (this.cleanupFilePath) { - this.disposed = true; await fsPromises.unlink(this.cleanupFilePath);Caller audit:
getFilePath()has one production caller,copyAgentFilesindev/src/cli/deploy/deploy_utils.ts. Both deploy flows call it inside thetryand callagentLoader.disposeAll()in thefinally, so everygetFilePath()call precedes anydispose().cli_run.tsandadk_api_server.tsuseawait usingbut never callgetFilePath(). No production caller changes behaviour.Overlap check: I scanned the open PRs for work on
AgentFile. #653 is the nearest neighbour; it adds a newdisposeSync()method and does not touchdispose(), so the two do not collide. Note thatdisposeSync()in #653 keeps an uncompiled instance usable, which will differ fromdispose()after this change; whichever lands second should align the two.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.
Two tests in
dev/test/utils/agent_loader_test.ts, next to the existing'returns original file path if not compiled':'throws when getting file path if an uncompiled agent is disposed'— the regression test.'disposes an uncompiled agent file without touching the filesystem'— pins that cleanup stays conditional, and that a seconddispose()does not reject.Proof each test can fail. I ran both against mutated source:
this.disposed = trueback inside theif (this.cleanupFilePath)branchexpected [Function] to throw an error. Test 2 still passes.await fsPromises.unlink(this.cleanupFilePath ?? this.filePath)promise rejected "Error: ENOENT: no such file or directory" instead of resolving. Test 1 still passes.Each test catches a defect the other misses.
Coverage.
dispose()is fully covered by the file's suite: 12 of 12 statements and 4 of 4 branch arms, measured with@vitest/coverage-v8. Both arms of thecleanupFilePathconditional run, and theif (this.disposed) returnfast path runs twice.Commands run, on the pushed commit:
Two failures are pre-existing and unrelated to this change. I confirmed both by stashing the change and re-running:
dev/test/cli/cli_create_test.ts > 'should handle Vertex AI selection with gcloud defaults'fails identically on a clean tree. It reads local gcloud defaults.npm run ts:checkreports the same 280 errors with and without this change, all incore/test/**and none indev/.CI.
run-testspasses on ubuntu, macos and windows. The first attempt needed re-runs for three flakes, all in files this PR does not touch:tests/integration/app_loader/app_loader_test.tstimed out at 40000ms. It passed on ubuntu in the same attempt and passes on macos after a re-run.core/test/code_executors/unsafe_local_code_executor_test.ts > 'should execute shell code and return stdout'timed out at 5000ms, and the spawned test server exited early attests/integration/test_case_utils.ts:341.fail, but the matrix cancelled it when macos failed, so that result was inconclusive.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Before this change the last line returns the path. After it, the line throws
Agent is disposed and can not be used, which matches what a compiled agent file already does.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.