test(harness): drain fire-and-forget memory flush before @TempDir teardown to stop flaky temp-dir deletion - #2935
Merged
Merged
Conversation
…rdown to stop flaky temp-dir deletion
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new quiescence extension currently ignores timeout/interrupt failures from the await methods, which can allow the original teardown flake to persist without a deterministic, actionable failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses intermittent JUnit @TempDir teardown failures in harness tests by ensuring fire-and-forget background writes (session/transcript mirrors and memory background tasks) have quiesced before temporary directories are deleted.
Changes:
- Added a JUnit Jupiter
AfterEachCallbackextension to await harness background task quiescence after each test method. - Added
@HarnessQuiescenceas a composed annotation to apply the extension with a one-liner. - Applied
@HarnessQuiescenceto multiple@TempDir-using harness integration tests that build/transiently runHarnessAgentinstances.
File summaries
| File | Description |
|---|---|
| agentscope-harness/src/test/java/io/agentscope/harness/agent/tools/HarnessAgentToolsConfigTest.java | Applies @HarnessQuiescence to prevent teardown races in this test. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/testing/HarnessQuiescence.java | Introduces composed annotation to register the quiescence extension. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/testing/HarnessBackgroundTaskQuiescenceExtension.java | Adds AfterEachCallback extension that drains mirror + memory background tasks. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/subagent/SubagentIsolationIntegrationTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/PlanModeSubagentPropagationTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/JsonSessionDefaultLocationTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/HarnessAgentTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/HarnessAgentSubagentStreamTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/HarnessAgentSubagentStreamEventsTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/HarnessAgentModelStringTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/HarnessAgentIntegrationExampleTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/HarnessAgentDynamicHookBuilderTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/HarnessAgentDistributedSandboxTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/gateway/SubagentRegistryRecoveryIntegrationTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/example/SandboxFilesystemIsolationScopeExampleTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/example/RemoteFilesystemIsolationScopeExampleTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/example/LocalFilesystemUserIsolationExampleTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/example/LocalFilesystemPersonalAssistantExampleTest.java | Applies @HarnessQuiescence to avoid @TempDir deletion races. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
5 tasks
birdie7761
added a commit
to birdie7761/agentscope-java
that referenced
this pull request
Sep 2, 2026
Since agentscope-ai#2777 the per-call memory flush and maintenance run detached from the agent response, tracked by the JVM-wide MemoryBackgroundTasks counter and drained by HarnessAgent.close() via awaitQuiescence. A hung model call, however, has no bound and no escape: - runFlush's pipeline has no timeout, so a hung flush keeps the conversation's coalescing slot in FLUSH_QUEUES running forever - every later flush of that conversation queues behind the dead slot and memory extraction stops for it; - MemoryBackgroundTasks.inFlight never returns to zero, so every subsequent awaitQuiescence call (e.g. HarnessAgent.close()) waits its full budget, gives up, and leaves the stuck task running against the torn-down workspace - the race agentscope-ai#2935 patched at the test level; - maintenance consolidation ran consolidator.consolidate(rc).block() inside Mono.fromRunnable, so a hung consolidation leaked a boundedElastic worker for the rest of the process: the inner block subscription is unreachable from any outer dispose, and once the model eventually returned, the same worker would keep writing retention/prune results into the torn-down workspace. Fixes: - Both pipelines get a 5-minute timeout. The budget runs from subscription, so it also covers any boundedElastic pickup wait: under scheduler saturation a healthy-but-slow run may be skipped and logged - the safe direction for fire-and-forget work. - doMaintenance is now a reactive composition (expire -> consolidator.consolidate -> prune), so a timeout or quiescence cancel propagates down the chain into the consolidation model stream and the worker is actually freed; consolidation failures log and let the retention steps still run, matching the previous try/catch semantics. - MemoryBackgroundTasks gains register/unregister: each task's subscription is listed under the task-tracking monitor (with a disposed re-check, so a terminated subscription can never linger), and awaitQuiescence disposes still-stuck tasks when its timeout elapses instead of leaving them running. Disposal runs outside the monitor so the cancelled task's cleanup can re-enter it safely. Known bound: the quiescence sweep disposes one task per key; that task's doFinally drains the queue and may start the next queued flush for that key before the sweep finishes - at most one flush per key per close escapes cancellation. New MemoryBackgroundHardeningTest: awaitQuiescence cancels stuck tasks; a hung flush times out, releases the quiescence count and the conversation queue recovers; a hung consolidation times out and the cancellation is asserted to reach the consolidation subscription itself (the old .block() variant released the counter while the worker stayed stuck). The timeout is injectable via package-private test hooks; production uses the 5-minute default.
5 tasks
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.
Problem
Two CI runs fail intermittently with JUnit errors that are not test-logic failures but teardown failures:
Failed to delete temp directory(Windows, PR fix(sandbox): docker sandbox native file transfer + reject truncated downloads (#2618) #2923)Failed to close extension context/Failed to delete temp directory(Ubuntu, PR feat(middleware): add final answer filter for ReAct streams #2926)Both surface only when a test builds a transient
HarnessAgent, drives it to completion via.block()/.stream()...block(), and uses@TempDirfor its workspace/state home.Root cause
This is a long-standing race, not a regression of any single commit. The harness memory flush has always been asynchronous: when an agent stream completes,
MemoryFlushMiddleware#onAgentdispatches the flush onSchedulers.boundedElastic()viasubscribe(...)— i.e. fire-and-forget. The calling test's.block()only waits for the business stream, not for that background flush.So the timeline is:
.block()and returns.@TempDirteardown and deletes the temp directory.boundedElasticis still writing session/transcript mirror files into that same@TempDir(or still holds open file handles).The result: directory/file deletion fails with
IOException→ wrapped asJUnitException. It is timing-dependent (depends on IO speed, scheduler, and how many files are written), which is why it flakes rather than failing deterministically, and why Windows (stricter file locking) fails more readily than Linux.The normal production path avoids this because
HarnessAgent#close()callsSessionTree.awaitMirrorQuiescence(...)+MemoryBackgroundTasks.awaitQuiescence(...). The flaky tests never callclose()on their transient agent.Fix
Add test-side quiescence that mirrors what
HarnessAgent#close()already does, run after each test method but before theTempDirextension deletes the directory:HarnessBackgroundTaskQuiescenceExtension— anAfterEachCallbackthat callsSessionTree.awaitMirrorQuiescence(5s)+MemoryBackgroundTasks.awaitQuiescence(5s). When nothing is in flight both calls return immediately, making it a no-op for tests that never trigger a flush.@HarnessQuiescence— a composed meta-annotation (@ExtendWith(HarnessBackgroundTaskQuiescenceExtension.class)) so at-risk tests only need a one-line annotation.@HarnessQuiescenceto 16 harness test classes that match the at-risk pattern (HarnessAgent.builder()+.call()/.stream()+@TempDir), including the three classes that flaked in CI:JsonSessionDefaultLocationTest,HarnessAgentIntegrationExampleTest,HarnessAgentDynamicHookBuilderTest.Production code is unchanged — the flush remains fire-and-forget so conversation completion is never blocked.