fix(harness): defer self-managed sandbox release until background writes finish

[guslegend0510]

AgentScope-Java Version

2.0.4-SNAPSHOT (main @ 9e43ed8)

Description

Fixes #3415

Background

HarnessAgent.wrappedCall / wrappedStreamEvents release the call's sandbox in the Mono.using / Flux.using cleanup as soon as the call completes. For a self-managed sandbox, release means stop() (snapshot) followed by shutdown() (remove the container). Several writes are still in flight at that point:

• the SessionTree mirror (session-tree-mirror thread) keeps its pinned sandbox and fails with docker tar extract failed ... container is not running;
• memory flush (boundedElastic) runs after the per-call binding is cleared and fails with No active sandbox — sandbox filesystem used outside of a call context;
• memory maintenance has the same race.

So the session log and the extracted memory never reach the snapshot that the next call resumes from. The call's answer is returned normally, so nothing tells the user the data was lost.

Changes

• SandboxBackgroundWrites (new): a process-wide registry of holds, keyed by sandbox identity. releaseWhenIdle(sandbox, teardown) runs the teardown inline when nothing is held, which is the current behaviour. Otherwise it runs the teardown once the last hold closes, on boundedElastic so a slow stop() never stalls the single-threaded mirror executor. A deferred release is forced after agentscope.sandbox.release.maxDeferMillis (default 30s; 0 disables deferral). A registry entry exists only while a hold or teardown is pending.
• SandboxLifecycleMiddleware.releaseForCall: still clears the per-call binding immediately. For self-managed sandboxes it hands release → persist → lease close to releaseWhenIdle. User-managed sandboxes are released immediately, as before.
• SandboxManager: tracks deferred releases per isolation scope. The next acquire for that scope waits for the pending release, so it never resumes state that has not been persisted yet.
• SessionTree: the session and transcript-segment mirrors take a hold when they are scheduled. The sandbox comes from the per-call SandboxAcquireResult on fsRc first ([Bug]: Concurrent calls on one agent corrupt each other's sandbox binding (single-slot SandboxBackedFilesystem / SandboxLifecycleMiddleware) #2490), then from the shared field.
• PinnedSandboxFilesystem: once the deferral budget has released the sandbox, uploads are skipped instead of exec'ing into a stopped container.
• MemoryFlushMiddleware / MemoryMaintenanceMiddleware: pin the call's sandbox at dispatch, while the call still owns it, and write through a RuntimeContext copy that keeps the binding. A coalesced (displaced) flush releases its pin.
• HarnessAgent.close() and the test quiescence extension also wait for pending deferred releases.
• Docs: EN/ZH harness/sandbox.md, section "Cross-call recovery = snapshots".

Relation to existing PRs

This overlaps with #3039 / #3094 (session mirror, #3036) and #3113 (memory flush, #3110). Here a single mechanism (defer the release until in-flight writes finish) covers the mirror, the memory flush and the memory maintenance, so the writes land in the workspace snapshot instead of being dropped or redirected.

How to test

• New tests: SandboxBackgroundWritesTest, SandboxLifecycleMiddlewareBackgroundWriteTest, SessionTreeSandboxReleaseTest, PinnedSandboxFilesystemTest.
• With the SandboxLifecycleMiddleware / SessionTree changes reverted, the three issue-reproduction tests fail: the sandbox is stopped while a write is in flight, and the next call does not wait. With the fix they pass.
• mvn -B -T1 -pl agentscope-harness -am verify passes (core 2,523 / harness 1,100 tests, 0 failures). The new tests were also rerun 5× after the last test additions.
• Not done: an end-to-end run against a real Docker sandbox, and a full-repository mvn clean verify.

Checklist

• Code has been formatted with mvn spotless:apply
• All tests are passing (mvn test)
• Javadoc comments are complete and follow project conventions
• Related documentation has been updated (e.g. links, examples, etc.)
• Code is ready for review

[oss-maintainer]

Summary

Defer self-managed sandbox release until in-flight background writes (session mirror, memory flush/maintenance) drain, keyed per sandbox identity with a 30 s budget, plus a per-isolation-scope hand-off so the next call cannot resume a snapshot that has not landed yet. The design is sound and the failure mode in #3415 is real; my findings are about the paths where the deferral itself can leak or tear a sandbox down in the wrong order.

Findings

• [Warning] SandboxManager.java:269 — awaitDeferredRelease can block the subscribing (possibly event-loop) thread for up to maxDefer + 60 s.
• [Warning] SandboxBackgroundWrites.java:142 — the double-release branch runs the new teardown inline while the stored entry's holds are outstanding, so teardown can run twice and the first run still races the writes.
• [Warning] SessionTree.java:598 — a hold is taken before statements that can throw; a never-closed hold pins a Sandbox in the static IdentityHashMap for the JVM's lifetime.
• [Warning] HarnessAgent.java:466 — the 5 s shutdown wait is shorter than the 30 s deferral budget, so close() can clean the workspace while a forced teardown is still queued.
• [Info] SandboxBackgroundWrites.java:206 — awaitPendingReleases returns true for a failed/cancelled teardown; the snapshot semantics deserve one javadoc sentence.
• [Info] SandboxBackgroundWrites.java:73 — JVM-global property knob and static registry: worth calling out the multi-tenant implications in the docs.

Positives

• clearSandboxIfCurrent / per-call RuntimeContext binding is preserved, so the deferral does not reintroduce the #2490 cross-session clobbering; the new boundSandbox prefers the per-call binding first.
• Skipping (rather than executing) work whose sandbox is already releasing is the right call in both mirrorTarget() and PinnedSandboxFilesystem.uploadFiles, and the skip returns per-file failure responses instead of silently pretending success.
• The teardown is never run on the writer's thread (onHoldClosed dispatches to boundedElastic), which protects the single process-wide mirror executor.
• maxDeferMillis=0 gives a clean kill-switch back to the old behaviour, and both the EN and ZH docs were updated in the same change.
• 788 added lines of tests across 5 new test files (plus 6 in the modified quiescence extension), including a TrackingSandbox double, covering the stuck-writer deadline path and the mid-release hold path.

Verdict

COMMENT — the four [Warning] items are edge paths in new concurrency bookkeeping, not defects in the mainline fix, and CI on 511cbbfd was still in progress at review time. Happy to re-review once the findings are addressed or CI lands.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] correctness/latency — acquire() can block the subscribing thread for up to maxDefer + 60 s (90 s by default).

awaitDeferredRelease is called from SandboxManager.acquire(), which SandboxLifecycleMiddleware.acquireForCall() invokes inside Mono.using's resource supplier — i.e. on whatever thread subscribes (for the gateway / WebSocket and distribution paths that can be an event-loop thread). A single wedged stop() therefore stalls a reactive pipeline for up to 90 s rather than the 30 s the deferral advertises.

Suggestion:

• keep the total wait close to the advertised budget (e.g. maxDeferMillis() plus a small fixed grace, and make the grace configurable through the same property namespace), and/or
• document that acquire() is a blocking call and make sure every call site subscribes on a bounded-elastic worker instead of a netty/event-loop thread.

[oss-maintainer]

[Warning] correctness — the double-release branch can tear the same sandbox down twice, in the racing order this PR is trying to remove.

When a release is already pending for this instance (entry.teardown != null) the local entry is set to null, so after the synchronized block the new teardown runs inline while entry.holds of the stored entry are still outstanding. Later, when those holds drain, onHoldClosed dispatches the stored teardown as well. Result: stop()/shutdown() execute twice, and the first execution happens exactly while background writes are in flight — the failure mode from #3415. The comment says a second release is a caller bug, so this path is unlikely, but the fallback should be safe rather than worse than the old behaviour.

Suggested fix: keep the entry and attach the newest teardown to it so that teardown still runs exactly once —

if (entry != null && entry.teardown != null) {
    entry.teardown = teardown;   // last release wins, still runs once
    return entry.done;
}

(or, if running twice really is preferred for a caller bug, at least mark entry.releasing = true and log at warn so the second dispatch in onHoldClosed is suppressed).

[oss-maintainer]

[Warning] correctness/leak — the hold is taken before code that can still throw, and a hold that is never closed pins a Sandbox forever.

Between mirrorTarget() (which does tryHold) and submitMirror(target, …) there are calls that may throw — here resolveRelativePath(contextFile) / resolveRelativePath(logFile), and in scheduleSegmentMirror the payload build (sb.append(entry.toString()), getBytes). If any of them throws, target.done() is never reached, so entry.holds stays > 0 and removeIfIdle can never evict the entry: ENTRIES is an IdentityHashMap with a strong key, so the Sandbox (and its state/filesystem graph) is retained for the lifetime of the JVM, and every subsequent release for that instance is deferred until the deadline forces it.

Cheapest fix: make the window exception-free by taking the hold last —

final String contextRel = resolveRelativePath(contextFile);
final String logRel = resolveRelativePath(logFile);
final MirrorTarget target = mirrorTarget();
if (target == null) { return; }
submitMirror(target, () -> { … });

or wrap the interval in try { … } catch (RuntimeException e) { target.done(); throw e; }. The same pattern is worth a look in MemoryFlushMiddleware.scheduleFlush (pin taken before compositeTimerKey / ConversationKey construction).

[oss-maintainer]

[Warning] correctness — a 5 s wait is shorter than the deferral budget this feature introduces, so close() can return before the teardown runs and the temp workspace is cleaned up under it.

The default maxDeferMillis is 30 s, and the deferred teardown itself is scheduled on boundedElastic. awaitPendingReleases(5, SECONDS) gives up well before a legitimately deferred release (session mirror uploads run on a single process-wide executor, so queueing is normal), then close() proceeds to shutdownTaskRepository() and the workspace/container cleanup below. When the forced release fires afterwards it writes the snapshot / persists state against a workspace that may already be gone — a shutdown-path variant of the same #3415 class of error, now harder to reproduce.

Suggest deriving the wait from the budget the release is allowed to take, e.g.

long budget = SandboxBackgroundWrites.maxDeferMillis();
SandboxBackgroundWrites.awaitPendingReleases(budget + 5_000L, TimeUnit.MILLISECONDS);

and, when it returns false, log at warn with the number of releases still pending so shutdown leaks are visible in the logs.

[oss-maintainer]

[Info] awaitPendingReleases reports success for a failed teardown.

catch (Exception e) { return true; } maps ExecutionException/CancellationException to true, which the caller reads as "everything drained". runTeardown completes done normally today, so this is unreachable in practice, but the contract is misleading and would hide a future regression; a bare catch (Exception) here also swallows CancellationException from allOf. Returning false plus a log.warn (or catching only ExecutionException) keeps the boolean honest. Related: the pending list is a snapshot taken under the lock, so a release registered after it started is not waited for — worth one sentence in the javadoc so callers of close() do not read true as "no release will ever run again".

[oss-maintainer]

[Info] process-wide static state + global system property — worth stating explicitly in the public API docs.

The javadoc already says the registry is process-wide and keyed by sandbox identity, which matches MemoryBackgroundTasks / the mirror executor, so the design is consistent. Two consequences are easy to miss for adopters though: (a) agentscope.sandbox.release.maxDeferMillis is a JVM-global knob, so in a multi-tenant distribution one tenant's setting changes every tenant's release latency (a per-agent/per-manager option, with the property as fallback, would fit the rest of the configuration model better); (b) because the map is static and holds strong keys, a hold that is never closed is a permanent leak rather than one that disappears with the agent instance (see the SessionTree comment). A short "scope of this setting" note in docs/v2/*/harness/sandbox.md next to the new paragraph would cover both.

[oss-maintainer]

Summary

Re-review of 182c1d14 only (previous review was anchored on 511cbbfd). All four [Warning] findings and both [Info] notes from the first pass are addressed, each in the way the code needed rather than the way the comment suggested:

Prior finding	Resolution
awaitDeferredRelease could block up to maxDefer + 60 s	grace is now 5 s, and acquire() documents that it blocks and must stay off event-loop threads
double release ran the new teardown inline while the stored entry's holds were outstanding	the extra teardown is now chained — either appended to entry.teardown before dispatch, or thenRun after the in-flight one; covered by two new tests (secondReleaseWhileDeferredRunsOnceAfterTheFirst, secondReleaseDuringATeardownWaitsForIt) that assert both the no-overlap and exactly-once properties, including the forced-release case
hold taken in SessionTree before statements that can throw	holds are now taken last in both scheduleMirror and scheduleSegmentMirror, transcriptStoreForMirror is wrapped with target.done(), and mirrorTarget() closes the hold if PinnedSandboxFilesystem construction throws
close()'s 5 s wait was shorter than the 30 s deferral budget	wait is now derived from maxDeferMillis() + 5 s, and awaitPendingReleases logs the pending count on timeout
awaitPendingReleases reported success for a failed teardown	catches ExecutionException explicitly and returns false; the snapshot semantics are stated in the javadoc
JVM-wide knob / static-registry implications	one sentence added to both EN and ZH harness/sandbox.md, plus a class-javadoc note that an unclosed Hold is a permanent retain

The catch (RuntimeException e) { hold.close(); throw e; } in mirrorTarget() is the correct shape, and MemoryMaintenanceMiddleware's begin-then-pin pair inside doOnComplete is consistent with it.

Remaining findings

Four [Info] items, all narrow and none blocking — a hold that can still leak inside pinCallSandbox (line 122, same class as the one just fixed), the thenRun-chained release not being visible to awaitPendingReleases (line 190, the per-scope hand-off is correct), the shutdown ceiling that close() now inherits from the budget (HarnessAgent line 470), and the fact that the new "off event-loop threads" contract on SandboxManager.acquire() is unenforced by the Mono.using resource suppliers in HarnessAgent (line 87, worth a separate issue rather than a change here).

CI / merge state

• validate, Check License, Check Module Sync — all pass.
• build (ubuntu-latest) is red only at the "Upload coverage reports to Codecov" step; the Build and Test with Coverage [Linux] step itself is green (the same Codecov TLS/infra failure has now been seen on #3130, where a re-run confirmed it is not PR-related).
• build (windows-latest) was cancelled inside Build and Test with Coverage [Windows], so the Windows leg has no verdict on 182c1d14 — worth a re-run of the workflow before merge, not for this review's sake but so the matrix is actually covered.
• mergeStateStatus=BLOCKED with MERGEABLE: branch protection awaiting a human approval, not a conflict.

Verdict

APPROVE. The deferral can no longer tear a sandbox down in the wrong order or leak a registry entry on the paths I checked, the failure modes are now visible in logs instead of silent, and the tests pin the ordering rather than just the outcome. The four [Info] notes are documentation-or-one-line hardening; take them or leave them, no re-review needed for any of them.

---

Automated review by github-manager-bot

[oss-maintainer]

[Info] the hold-leak class you just closed in SessionTree is still open at this line. tryHold has already incremented entry.holds when the context copy is made, and RuntimeContext.builder(rc).build() is not a total function — Builder.from(source) does new ConcurrentHashMap<>(source.stringAttributes) / new HashMap<>(entry.getValue()) and the copy constructor then putAlls them. If anything there throws, the Hold never reaches a Pin, so nobody can close it: removeIfIdle can never evict the entry, ENTRIES (strong-keyed IdentityHashMap) keeps that Sandbox reachable for the life of the JVM, and every later release of that instance is deferred until the deadline forces it (maxDeferMillis=0 avoids the wait but leaves the entry pinned, since removeIfIdle requires holds == 0).

The window is narrow, but the guard is one line and it is exactly the shape mirrorTarget() adopted in this commit:

Hold hold = tryHold(sandbox);
if (hold == null) {
    return new Pin(rc, null);
}
try {
    return new Pin(RuntimeContext.builder(rc).build(), hold);
} catch (RuntimeException e) {
    hold.close();
    throw e;
}

Related, one level up: in MemoryFlushMiddleware.scheduleFlush the MemoryBackgroundTasks.begin() (line 175) is outside any guard, so a throw here would also leave the in-flight counter permanently incremented and make every awaitQuiescence(...) — in close() and in HarnessBackgroundTaskQuiescenceExtension — burn its full timeout from then on. MemoryMaintenanceMiddleware (lines 162-164) has the same begin()-then-pin pair inside doOnComplete. Non-blocking.

[oss-maintainer]

[Info] Chaining instead of running inline is the right fix — the double stop()/shutdown() race is gone, and secondReleaseWhileDeferredRunsOnceAfterTheFirst / secondReleaseDuringATeardownWaitsForIt / secondReleaseAfterAForcedReleaseRunsOnce cover the three orders that matter (deferred-not-yet-dispatched, mid-teardown, post-forced-release). entry.finished = true + removeIfIdle in runTeardown's finally before done.complete(...) is also what makes the third case behave.

One asymmetry worth a sentence rather than a restructure: the teardown attached here is not registered in ENTRIES, so awaitPendingReleases() cannot see it. Because runTeardown drops the entry under the lock before completing done, a global await that starts after that point sees an empty pending list and returns true while this chained release is still running. The per-scope hand-off is correct — releaseForCall passes this composed future to SandboxManager.trackDeferredRelease, so the next acquire() in the same isolation scope waits for both — so only HarnessAgent.close() in the caller-bug case can slip past. Suggest stating it in the awaitPendingReleases javadoc next to the existing snapshot caveat ("a second release of an already-releasing sandbox is returned to its caller but not tracked here") and leaving the code alone.

[oss-maintainer]

[Info] Deriving the wait from the deferral budget closes the ordering hole I raised — close() no longer cleans the workspace under a legitimately deferred release, and the timeout now logs what is still pending instead of returning silently.

Two notes on the resulting shape, neither blocking:

1. The ceiling moved rather than shrank: worst case is ~45 s inside close() (5 s mirror quiescence + 5 s memory quiescence + maxDeferMillis + 5 s), paid exactly when a writer is wedged — which is also the situation where an adopter has put close() behind a JVM shutdown hook or @PreDestroy and the process gets killed before the teardown lands, losing the very snapshot we are waiting for. Cheap options: cap it (Math.min(maxDeferMillis() + 5_000L, CLOSE_MAX_WAIT_MILLIS), warn when the cap bites), or align the docs, which currently promise only "capped at 30 seconds" for the release and say nothing about close().
2. The boolean return is discarded here. Now that it tells the truth, if (!awaited) log.warn(...) at this call site makes a shutdown leak visible to someone reading close() rather than only to whoever knows the helper logs it internally.

Both the quiescence waits above and this one are point-in-time/bounded, so the finally block below (workspace index, MCP clients, delegate.close()) is still not strictly ordered against a release that fires late. That trade is documented and conscious — flagging it only because close() is where a lost snapshot is hardest to notice.

[oss-maintainer]

[Info] Grace from 60 s to 5 s plus this javadoc resolves the concern for this PR: the deferral adds at most maxDefer + 5 s to an acquire() that was already doing blocking remote I/O (client.resume() / create), so it bounds an existing stall instead of introducing one. trackDeferredRelease + the whenComplete((v,e) -> deferredReleases.remove(key, tracked)) CAS are clean, and awaitDeferredRelease no longer swallows the timeout silently.

One correction to make the contract accurate rather than aspirational: the gateway path already honours it — HarnessGateway.withGatedTurn / withGatedStream end with .subscribeOn(Schedulers.boundedElastic()) (lines 719 and 747), so a wedged release stalls a bounded-elastic worker there, not an event loop. What is not enforced is the direct case: HarnessAgent.wrappedCall / wrappedStreamEvents call acquireForCall from the Mono.using/Flux.using resource supplier, which runs on whatever thread the application subscribes on, and there is no scheduler hop anywhere in the harness call path (HarnessAgent doesn't reference Schedulers at all). So for a WebFlux/SSE controller calling agent.call(...)/agent.stream(...) directly on netty, "reactive callers must invoke it off event-loop threads" is a rule the framework does not implement for them. Two ways to finish the thought in a follow-up issue: hop inside the resource supplier (Mono.fromCallable(() -> { acquireForCall(effective); return effective; }).subscribeOn(Schedulers.boundedElastic())), or restate the constraint on the public call/stream javadoc where applications will actually read it. No change requested here.

[oss-maintainer]

Summary

Re-review of the delta pushed after my approval on 182c1d14: 366b16d5 + 407577bd touch only .github/workflows/maven-ci.yml (codecov-action@v4 → v6.0.1 with use_pypi: true + version: 11.3.1, and fail-fast: true → false). The harness code under review — SandboxBackgroundWrites, the deferred release in SandboxLifecycleMiddleware, the per-isolation-scope hand-off in SandboxManager, and the CRLF/maven-ci interaction — is byte-identical to the head I approved, so that approval still stands; my findings here are only about the CI delta.

The verdict stays COMMENT rather than a second APPROVE because the CI change does not achieve its goal: the coverage-upload step still fails on 407577bd (run 37271237149), with codecov-cli 11.3.1 giving up on https://ingest.codecov.io/... after retries, and the same step is red right now on unrelated heads (83486d1f in #3420, 9ee0e2eff in #2697). That is repo-wide infra, not a defect in this PR.

Findings

• [Warning] .github/workflows/maven-ci.yml:122 — CI-infra change mixed into a feature PR and does not fix the failure (run-by-run evidence above); split it out.
• [Warning] .github/workflows/maven-ci.yml:127 — fail_ci_if_error: true keeps turning every PR's build job red on a Codecov outage; suggest continue-on-error / fail_ci_if_error: false, or fix egress to ingest.codecov.io.
• [Info] .github/workflows/maven-ci.yml:47 — fail-fast: false improves diagnosability at the cost of runner minutes on red runs.
• [Info] .github/workflows/maven-ci.yml:124 — use_pypi: true adds an uncached per-run pip install and a PyPI dependency to the merge path; consider a SHA pin for the action.

No change requested on the fix itself

The deferred-release design and the tests added in the earlier rounds are unaffected by this delta; build (windows-latest) is still in_progress on the current head, so there is nothing to attribute to your Java changes. Once the workflow edit is split into its own PR, this one is good to merge from a code standpoint.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] This CI change is unrelated to the PR's stated fix (#3415 deferred sandbox release) and it does not fix the failure it targets, so coupling it to the feature PR makes both harder to land and to revert.

Same step (Upload coverage reports to Codecov, build (ubuntu-latest)) on this branch:

head	run	outcome
182c1d14 (codecov-action@v4)	37261666748	Error: write EPROTO ... ssl/tls alert handshake failure
366b16d5 (@v6.0.1)	37269719429	still failure at the same step
407577bd (@v6.0.1 + use_pypi, current head)	37271237149	Exception: Request failed after too many retries. URL: https://ingest.codecov.io/upload/github/agentscope-ai::::agentscope-java/upload-coverage

The v6 + PyPI-installed CLI reaches the same unreachable ingest endpoint, so the blocker is the upload network path, not the action's Node runtime. Suggest keeping this PR scoped to the harness fix and moving the workflow change into its own PR, where the CI experiment can be observed (and reverted) independently.

[oss-maintainer]

[Warning] fail_ci_if_error: true is kept, so a Codecov outage fails every PR's build job repo-wide, not only this one. The same step is currently red on unrelated heads: 83486d1f (#3420) and 9ee0e2eff (#2697) both fail at Upload coverage reports to Codecov.

Coverage upload is advisory for correctness — consider decoupling it from the merge gate:

- name: Upload coverage reports to Codecov
  if: runner.os == 'Linux'
  uses: codecov/codecov-action@v6.0.1
  continue-on-error: true          # or: fail_ci_if_error: false
  with:
    use_pypi: true
    version: 11.3.1
    token: ${{ secrets.CODECOV_TOKEN }}
    verbose: true

(For reference, in v6.0.1 fail_ci_if_error/verbose map to codecov-cli --fail-on-error --verbose, which is what the run log shows, so the CLI still logs the failure either way.) If the team does want coverage upload to remain a hard gate, the fix has to be on the infra side (egress to ingest.codecov.io) — bumping the action alone will not turn CI green.

[oss-maintainer]

[Info] fail-fast: false is a good change for diagnosability — each matrix leg now reports its own verdict instead of the Windows job being cancelled as a dependent, which is exactly what made runs like 37261666748 ambiguous. Trade-off worth noting: since coverage upload is Linux-only, a Linux failure now also spends a full Windows build's runner minutes on every red run.

[oss-maintainer]

[Info] Pinning the action (v6.0.1) and the CLI (version: 11.3.1) is the right instinct for reproducibility. Two notes: use_pypi: true installs into /home/runner/.local on every run (≈2 s in run 37271237149, no cache), which adds a PyPI availability dependency to the merge path; and pinning the action to a full commit SHA rather than the v6.0.1 tag would remove the tag-mutability question for a workflow that gates merges.