fix(harness): detach memory maintenance from agent call response - #2326
Open
Asaduddin18 wants to merge 15 commits into
Open
fix(harness): detach memory maintenance from agent call response#2326Asaduddin18 wants to merge 15 commits into
Asaduddin18 wants to merge 15 commits into
Conversation
MemoryMaintenanceMiddleware concatenated the maintenance Mono onto the returned Flux via concatWith, so callers that consume the response to completion (blockLast(), takeLast(1)) waited for the full LLM-based consolidation call to finish -- inflating agent call latency by up to 30s and contradicting documentation that describes this as a background/fire-and-forget step. Detach maintenance via doOnComplete(...).subscribe() instead, so the returned Flux completes as soon as the underlying agent call does, independent of maintenance duration. Also add a timeout to the consolidation LLM call so a hung provider can't tie up a shared boundedElastic thread indefinitely. Fixes agentscope-ai#2225
Detaching MemoryMaintenanceMiddleware's fire-and-forget maintenance run (agentscope-ai#2225's fix) left it fully untracked: HarnessAgent.close() returned immediately even while a background maintenance write to the workspace was still in flight. In tests using JUnit @tempdir this raced against directory cleanup and intermittently failed with DirectoryNotEmptyException; in production it could tear down a workspace/sandbox out from under an in-progress write. Track each scheduled maintenance run's Disposable and add MemoryMaintenanceMiddleware.close(), called first from HarnessAgent.close(), which stops scheduling new runs and waits (bounded, 5s) for outstanding ones to finish before disposing stragglers.
…ce guard Adds coverage for the branches introduced by the previous commit that jacoco flagged as untested: scheduleMaintenance() no-op'ing after close(), and close()'s interrupted-wait / dispose-stragglers paths. Also drops the extra "dispose immediately if closed flipped true right after pending.add(d)" guard in scheduleMaintenance(). It only mattered for a TOCTOU race too narrow to hit deterministically in a test, and close()'s own wait-then-dispose-all loop already reclaims that disposable in every realistic timing.
5 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
HarnessAgent.close()'s `if (memoryMaintenanceMw != null)` guard only had its true branch exercised (every existing test that calls close() builds an agent with memory hooks enabled), leaving the disabled-hooks path an untested branch.
This was referenced Jul 24, 2026
oss-maintainer
suggested changes
Jul 28, 2026
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Review: PR #2326 — fix(harness): detach memory maintenance from agent call response
Verdict: Changes Requested — CI is failing
Concept: Approved ✅
The approach is correct and addresses issue #2276. Moving memory maintenance (consolidate + flush) out of the concatWith chain so it runs asynchronously is the right fix. The close() method properly cleans up the middleware.
Issue: Build failures on both Linux and Windows
The agentscope-harness module has test failures:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.5:test
on project agentscope-harness: There are test failures.
Please check the surefire reports and fix the failing tests. Common causes:
- Timing-sensitive async tests — the new async behavior may cause race conditions in tests that assert on timing
- Constructor signature changes — the new
MemoryMaintenanceMiddlewareparameter in theHarnessAgentconstructor may break existing test setups that use the old constructor - Mock setup — tests that mock the middleware chain may need updating for the new detached behavior
Code observations:
- The nested try-finally in
close()is correct for resource cleanup ordering - The
doFinallyfor cleanup is a good pattern - Consider adding a timeout to the async maintenance to prevent resource leaks on shutdown
…ntion localFilesystem_filesPersistAcrossCalls wrote its persistence-demo content to MEMORY.md, which the harness's own memory-maintenance subsystem also owns (on by default whenever a model is configured). Detaching maintenance (this branch's own fix) means background consolidation is no longer serialized with the test's manual write, so it could race in and overwrite MEMORY.md with the stub model's output before the test's second assertion ran -- observed as a flaky CI failure on both Linux and Windows. Move the demo content to notes.md, a path the harness has no opinion about, so the test only exercises the local-filesystem persistence behavior it's actually named for.
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
2 participants
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.
AgentScope-Java Version
2.0.1-SNAPSHOT
Description
Background:
MemoryMaintenanceMiddleware.onAgentconcatenated the memory-maintenanceMonoonto the returnedFluxvia.concatWith(...). BecauseReActAgent.callInternalends its call chain with.takeLast(1)(which can't emit until the upstream Flux signalsonComplete), any caller consuming the agent response to completion —blockLast(),takeLast(1), or a WebFlux controller awaiting theMono<Msg>— ended up waiting for the full LLM-based consolidation call insideconsolidateMemory()(consolidator.consolidate(rc).block()), inflating agent-call latency by up to ~30s in the reported case. This also contradicted the harness memory documentation, which describes this step as running in the "background" in a "fire-and-forget" fashion.Root cause:
doOnComplete(...)(no scheduler switch) was the original implementation and matches what the docs still describe. A later, unrelated commit swapped it for.concatWith(Mono.fromRunnable(...).subscribeOn(Schedulers.boundedElastic())...), which moved the blocking work onto a different thread pool but did not actually detach it from the response —concatWithkeeps the maintenance Mono part of the same Flux the caller is awaiting.Changes:
MemoryMaintenanceMiddleware.onAgent: replaced.concatWith(...)with.doOnComplete(() -> ...).subscribe()so the maintenance Mono is genuinely detached — subscribed independently of the returned Flux, matching the fire-and-forget pattern already used elsewhere in this package (AsyncToolMiddleware,SubagentsMiddleware). The returned Flux now completes as soon as the underlying agent call does, regardless of how long maintenance takes. Error-handling semantics (onErrorResume+log.warn) are unchanged, only relocated.MemoryConsolidator.consolidate(): added a.timeout(CONSOLIDATION_TIMEOUT)on the model call (reusingExecutionConfig.MODEL_DEFAULTS's 5-minute timeout) so a hung model provider can no longer tie up a sharedboundedElasticworker thread indefinitely — this pool is shared process-wide with 40+ other call sites.MemoryMaintenanceMiddlewareto describe the actual (now-correct) detached behavior instead of the staleconcatWithreference.Why no doc changes: This fix restores the exact behavior the existing harness docs (
docs/v2/*/docs/harness/memory.md,compaction.md,workspace.md) already describe (background/fire-and-forget maintenance) — so those docs are accurate again as a result of this fix, rather than needing separate correction.How to test:
mvn test -pl agentscope-harness -am -Dtest=MemoryMaintenanceMiddlewareAsyncBehaviorTest,MemoryConsolidatorTimeoutTestMemoryMaintenanceMiddlewareAsyncBehaviorTest.onAgent_completesBeforeSlowConsolidationFinishes: mocks a slowMemoryConsolidator.consolidate()call and assertsonAgent(...)'s returned Flux completes in well under the consolidation duration — this test fails against the pre-fixconcatWithcode (verified locally: takes 3s instead of completing immediately).MemoryMaintenanceMiddlewareAsyncBehaviorTest.onAgent_onErrorResume_handlesExceptionFromDetachedMaintenance: verifies the detached maintenance error path is still handled byonErrorResume(usingHooks.onErrorDroppedto detect if it would otherwise be silently dropped).MemoryConsolidatorTimeoutTest.consolidate_timesOutInsteadOfHangingForever: usesStepVerifier.withVirtualTimeto confirm a model that never responds causesconsolidate()to error with aTimeoutExceptioninstead of hanging — verified this test fails to even compile without the fix (confirming it's tied to the change).Full harness module suite: 637/637 tests passing, 0 failures/errors. Every changed/added line has full line and branch coverage (verified via JaCoCo — no partial coverage on any touched line).
Fixes #2225
Checklist
mvn spotless:applymvn test)