fix(harness): detach memory maintenance from agent call response#2326
Open
Asaduddin18 wants to merge 11 commits into
Open
fix(harness): detach memory maintenance from agent call response#2326Asaduddin18 wants to merge 11 commits into
Asaduddin18 wants to merge 11 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❌ Patch coverage is
📢 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 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
1 participant
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)