fix(sandbox): serialize same-key concurrent calls to prevent persisted state races

[larry-zy]

Closes #2845

Background

On a single HarnessAgent, concurrent calls that resolve to the same SandboxIsolationKey read and write the same persisted sandbox state record. The sandbox acquire -> resume -> persist -> release window runs outside the delegate serialization gate, so parallel same-key calls could start independent containers from the same state and silently overwrite each other on completion.

This is separate from #2490: that fix isolates live bindings for different-key calls, but it does not protect same-key persisted state.

Changes

• Add InProcessSandboxExecutionGuard, using a fair per-key semaphore to serialize the complete sandbox lifecycle within one JVM.
• Change the default guard from noop() to inProcess(). An explicit SandboxExecutionGuard.noop() remains available as an opt-out.
• Give the in-process guard a 30-minute default wait bound so wedged holders cannot occupy the fixed acquisition scheduler indefinitely. inProcess(Duration) can tune the bound, while the explicit duration constructor still accepts null for a deliberate unbounded wait.
• Run blocking acquisition on a dedicated fixed scheduler and cleanup on boundedElastic, including cancellation compensation, so event-loop and cancelling threads do not perform blocking sandbox work.
• Preserve typed failures: SandboxExecutionTimeoutException propagates unchanged, and interruption becomes SandboxExecutionInterruptedException after restoring the interrupt flag.
• Resolve DistributedStore.sandboxExecutionGuard() once before installing it.
• Guarantee lease cleanup in finally, including persist or release failures.
• Update English and Chinese sandbox/filesystem documentation and release notes with the new default, latency impact, distributed deployment requirement, and opt-out path.

AgentRun lifecycle contract

Sandbox acquisition is intentionally asynchronous. After subscription, an AgentRun remains QUEUED while the sandbox slot is being acquired and changes to RUNNING when the delegate lifecycle is admitted. Tests now assert this transition explicitly instead of assuming that subscription synchronously binds the sandbox.

Migration and deployment notes

Same-slot calls now queue instead of running separate containers and racing with last-write-wins persistence. For SESSION, USER, or AGENT scopes, wall-clock latency can therefore grow with the number of queued peers. Tune the wait with SandboxExecutionGuard.inProcess(Duration) when calls can legitimately exceed 30 minutes.

The default guard coordinates only within one JVM. If the same slot can be reached by multiple application instances, configure the distributed guard supplied by a DistributedStore or set one on SandboxFilesystemSpec. Use SandboxExecutionGuard.noop() only when external serialization or last-writer-wins behavior is explicitly intended.

Verification

• Regression coverage for queued-to-running status, cancellation compensation, distributed guard accessor identity, typed timeout/interruption propagation, default timeout, and same-key end-to-end serialization.
• mvn -B -T1 clean verify passes locally across all 96 reactor modules.

[codecov]

Codecov Report

❌ Patch coverage is 75.19380% with 32 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...s/agent/middleware/SandboxLifecycleMiddleware.java	47.82%	11 Missing and 1 partial ⚠️
.../agent/sandbox/InProcessSandboxExecutionGuard.java	85.18%	5 Missing and 3 partials ⚠️
...entscope/harness/agent/sandbox/SandboxManager.java	71.42%	0 Missing and 6 partials ⚠️
.../io/agentscope/harness/agent/DistributedStore.java	20.00%	3 Missing and 1 partial ⚠️
...e/harness/agent/sandbox/SandboxExecutionGuard.java	60.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[muranchenhui]

LGTM

[muranchenhui]

LGTM

[guslegend0510]

LGTM

[oss-maintainer]

Summary

Fixes a real and well-diagnosed race: with the default noop() guard, two concurrent calls resolving to the same SandboxIsolationKey both resumed from the same persisted state and last-writer-wins on persistState. The InProcessSandboxExecutionGuard design is careful — refcounted slot recycling under compute, a fair Semaphore(1) rather than a ReentrantLock because release lands on a different thread, and an idempotent lease — and the 700 lines of added tests across three files target exactly the window being closed. The lease-close-in-finally pair in SandboxLifecycleMiddleware is the right shape: a permit that leaks on a rollback path wedges the slot permanently.

Two things block approval. First, CI is red on this head and the failure is caused by the change, not by a stale branch: moving the acquire to subscribeOn(...) makes the status flip asynchronous and HarnessAgentTest asserts it synchronously (detailed at HarnessAgent.java:1034). That is also a user-visible contract question, so it needs a deliberate answer rather than a rebase to see. Second, the guard accessor is called twice in build(), which matters for implementations that construct a new guard per call.

The rest is about failure modes and reachability: an indefinite default wait on a bounded pool turns one wedged holder into a JVM-wide acquire stall, and wrapping everything in RuntimeException(e) hides the SandboxExecutionTimeoutException that the same PR introduces for precisely that diagnosis. Flipping noop() → inProcess() is also a behaviour change for anyone fanning out concurrent calls on one key, so it deserves a migration note next to the javadoc.

Findings

• [Critical] HarnessAgent.java:1034 — off-thread acquire breaks the synchronous RUNNING-after-subscribe guarantee; HarnessAgentTest:667 fails on this head while main is green.
• [Warning] HarnessAgent.java:2502 — sandboxExecutionGuard() invoked twice; null-checked instance need not be the installed instance (Redis builds one per call).
• [Warning] HarnessAgent.java:956 — indefinite default wait parks the fixed-size acquire pool; one wedged holder stalls unrelated keys.
• [Warning] SandboxLifecycleMiddleware.java:152 — RuntimeException(e) wrapping erases SandboxExecutionTimeoutException / interrupt typing.
• [Info] IsolationScope.java:50 — default flip is a behaviour change; add a changelog + doc migration note.
• [Info] HarnessAgent.java:1040 — cancel-path release runs inline on the cancelling thread while the normal path releases off-thread.

---

Automated review by github-manager-bot

[larry-zy]

@guslegend0510 @muranchenhui All review findings have been addressed, all 9 review threads are resolved, the latest main has been merged without remaining conflicts, and the full GitHub CI matrix is green on head 8fc48d15 (Ubuntu, Windows, docs validation, license, module sync, Codecov, and CLA). Could you please re-review the latest head when convenient?

[oss-maintainer]

Summary

Re-reviewed on 8fc48d15. Everything I flagged on b50e0dbf is addressed and verified: CI is green on this head (Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest), codecov/patch, validate), sandboxExecutionGuard() is resolved once before installation, SandboxExecutionTimeoutException and the interrupt flag now survive acquireForCall as typed unchecked exceptions, the cancel-path release moved off the cancelling thread, the lease close is finally-guarded on both the rollback and the normal release path, and the default flip has both a javadoc treatment and an Unreleased → Changed release-notes entry. The refcounted-slot recycling under compute and the 2-bit acquire/cancel reconciliation hold up on a re-read.

Not approving, for one substantive reason and one contract reason. The substantive one is scope: the inProcess() default keeps its state per guard instance and the guard is built per HarnessAgent, so the JVM-wide serialization the class javadoc, the interface javadoc and both docs pages promise is not what ships — for SESSION scope the persisted slot path is sandbox/session/<sessionId> with no agentId in it, so two agents serving one session share the state the guard is meant to protect while waiting on unrelated semaphores. The contract one is DistributedStore.sandboxExecutionGuard(), which moved from a documented non-null return to null-means-default; that is a source break for out-of-tree implementations and leaves CompositeDistributedStore carrying a possibly-null value with no @Nullable anywhere in the file. The remaining notes are the still-open acquire-pool bound from my previous review, the wait-timeout backstop, and three test gaps that map onto exactly these cases.

Findings

• [Warning] SandboxExecutionGuard.java:101 — inProcess() default is instance-local state, but the slot it protects is derived from the isolation key (SESSION slots carry no agentId), so the advertised JVM-wide serialization does not hold across agent instances.
• [Warning] HarnessAgent.java:1103 — the permit now spans the entire delegate execution rather than the acquire/persist window, which opens a re-entrancy deadlock shape (USER key vs. slotKey(userId, sessionId) gate) and widens same-user cross-session serialization beyond what the release notes describe.
• [Warning] DistributedStore.java:115 — non-null documented contract changed to null-means-default; needs Optional or an explicit @Nullable and a dereference audit.
• [Warning] HarnessAgent.java:974 — carried over from b50e0dbf: fixed-capacity acquire pool plus a 30-minute park, with no interrupt-on-cancel on this newBoundedElastic overload, still lets one hot key delay unrelated keys.
• [Info] InProcessSandboxExecutionGuard.java:62 — the bound doubles as the maximum queue wait; put the tuning knob and the waiter count in the failure path.
• [Info] SandboxSameSessionSerializationTest.java:73 — coverage is good for the window that was closed; the shared-slot-across-instances, re-entrant, and recycle-under-contended-registration cases are untested.

Verified good

• Both prior blockers are genuinely fixed rather than papered over: git log shows a follow-up test(harness): await async sandbox cancellation cleanup on top of 3467eda1, and the checks list on this head has no failures.
• releaseForCall and the acquireForCall rollback both close the lease in a finally, and SlotLease.close() is idempotent through an AtomicBoolean, so a double-close from the compensation path plus usingWhen cleanup cannot release two permits.
• InterruptedException handling is correct for a blocking tryAcquire: the flag is restored and the type is preserved, so upstream callers can branch on either.
• tryEnter registers interest inside slots.compute, so a slot cannot be recycled between a waiter deciding to wait and that waiter taking the permit; the timeout and interrupt paths both drop the reference they added.
• The DistributedStore builder no longer coerces an unset guard to noop(), and HarnessAgent.build() only adopts a store guard when the store actually supplies one — the null-is-opt-out distinction is consistent in both places and matches the new javadoc.

---

Automated review by github-manager-bot

[oss-maintainer]

Summary

The three new commits (881e246, 5ec1f1c, c58042f) make the built-in guard a shared JVM-wide default via defaultInProcess() (SandboxExecutionGuard.java:105-110), bound same-key contention to MAX_WAITERS_PER_KEY = 8 with a fail-fast SandboxExecutionQueueFullException (InProcessSandboxExecutionGuard.java:65,111-112), and document the shared default while preserving explicit noop() behavior. Verdict: previous-round issues #1 and #5 are resolved in the guard sources, and the new exception types land as unchecked — but SandboxReentrantExecutionException has no throw site in the guard, so that item stays partial.

Previous-round status

• [#1 inProcess() instance-local state vs. advertised JVM-wide serialization] resolved — the state is per-instance (InProcessSandboxExecutionGuard.java:67), the inProcess() javadoc now discloses "Each call returns an independent guard" and steers shared coordination to defaultInProcess() (SandboxExecutionGuard.java:93-94), and defaultInProcess() returns a single static DefaultHolder.INSTANCE (SandboxExecutionGuard.java:105-110) so harness agents genuinely share one guard per JVM.
• [#5 queue bound / waiter observability] resolved — MAX_WAITERS_PER_KEY = 8 (InProcessSandboxExecutionGuard.java:65) is enforced atomically in slots.compute by throwing SandboxExecutionQueueFullException when refs >= maxWaitersPerKey + 1 (InProcessSandboxExecutionGuard.java:111-112), and observability hooks exist via trackedSlots() (InProcessSandboxExecutionGuard.java:150-152), waitingCalls(key) (InProcessSandboxExecutionGuard.java:155-158), and waitTimeout() (InProcessSandboxExecutionGuard.java:161-163).
• [new exception types] partial — both extend RuntimeException (SandboxExecutionQueueFullException.java:19, SandboxReentrantExecutionException.java:19); the queue-full type is thrown from the guard (InProcessSandboxExecutionGuard.java:112), but the reentrant type is never thrown in these four files — a nested same-key call parks on the semaphore until the 30-minute DEFAULT_WAIT_TIMEOUT (InProcessSandboxExecutionGuard.java:62,118-125) instead of failing fast.

Findings

• [Warning] SandboxReentrantExecutionException.java:19 — declared for nested re-entry but never thrown by InProcessSandboxExecutionGuard.tryEnter (InProcessSandboxExecutionGuard.java:118-125); a nested same-key call blocks up to DEFAULT_WAIT_TIMEOUT (30 min, InProcessSandboxExecutionGuard.java:62) or indefinitely when waitTimeout is null. Throw it from tryEnter on detected re-entrancy, or document the actual throw site.

CI / tests

CI on head 9e7009796c is green (Check License, Check Module Sync, validate, build (ubuntu-latest), build (windows-latest), codecov/patch pass) and license/cla is signed. Tests were not executed in this round; review was limited to the four guard sources above, so findings #2/#4 from the previous round (permit scope in HarnessAgent, interrupt-on-cancel on the acquire pool) were not re-verified.

---

Automated review by github-manager-bot

[oss-maintainer]

Summary

Fourth-round re-review at head 9e7009796c, prompted by @larry-zy's answer to my open finding. The last outstanding item (SandboxReentrantExecutionException having no throw site) is resolved: the throw is in SandboxManager.acquire() at SandboxManager.java:101, guarded by the HeldKeys inherited through RuntimeContext and taken before executionGuard.tryEnter() at :104, with SandboxSameSessionSerializationTest.nestedCallOnSameUserSlotFailsPromptlyInsteadOfWaitingForItsParent pinning the behaviour. Approving.

Findings from previous rounds — status at this head

• [Critical] HarnessAgent off-thread acquire vs. the synchronous QUEUED -> RUNNING contract — resolved. The acquire is pinned to a dedicated SANDBOX_ACQUIRE_SCHEDULER (HarnessAgent.java:1063) instead of Schedulers.boundedElastic(), the RUNNING transition is documented at HarnessAgent.java:857,869, and HarnessAgentTest (+58) plus the new HarnessAgentSandboxOffThreadTest (+235) cover it. CI is green on this head, which was the original symptom.
• [Warning] sandboxExecutionGuard() called twice in build() — resolved: one accessor call remains in the path (HarnessAgent.java:2526), so the instance that is checked is the instance that is installed.
• [Warning] unbounded wait parking the acquire pool — resolved. defaultInProcess() is the harness default with DEFAULT_WAIT_TIMEOUT as the bound, inProcess(Duration) exposes tuning, MAX_WAITERS_PER_KEY fail-fasts via SandboxExecutionQueueFullException, and the shared-default vs. instance-local distinction is documented in SandboxExecutionGuard.java:25-26,34,89-94.
• [Warning] RuntimeException(e) erasing diagnostic types — resolved at the layer that matters: InterruptedException restores the interrupt flag and is re-thrown as SandboxExecutionInterruptedException, and checked RuntimeExceptions pass through unwrapped.
• [Info] default flip noop() -> inProcess() needs a migration note — resolved: EN/ZH docs/harness/sandbox.md, docs/harness/filesystem.md and docs/others/release-notes.md all updated in this PR.
• [Warning] re-entry detection in tryEnter (this thread) — resolved as described in my thread reply; manager-layer detection is the correct location for this guard design.

Gate checks

• CLA: license/cla = success ("Contributor License Agreement is signed") on head 9e7009796c.
• Checks on head: validate, build (ubuntu-latest), build (windows-latest), Check License, Check Module Sync, codecov/patch all success.
• Mergeability: mergeable=MERGEABLE, no conflict with main; mergeStateStatus=BLOCKED only because an approving review was missing — that is what this review supplies. Merge itself remains a maintainer action.

Notes for the maintainer

• The default guard change is user-visible for anyone fanning out concurrent same-key calls on one agent; the release-notes entry is the communication surface for that.
• Tests were not executed locally in this round (review was source- and CI-based), but the PR carries ~970 added test lines across the four sandbox test classes and the full matrix is green on the reviewed head.

---

Automated review by github-manager-bot