fix(harness): recover from context overflow on the streaming path

[KANLON]

Summary

HarnessAgent.streamEvents() previously had no context overflow recovery. When the model returned a 400 / context overflow error on the streaming path, the caller saw the raw provider error directly, for example:

• input token limit is 1048576

Before the fix, the call returned the error directly.

The error message looked like this:

This PR was written with AI assistance, has been code-reviewed, and has been manually verified by running it.

This PR adds the emergency recovery mechanism to the streaming path so it aligns with call().

Changes

• streamEvents() now passes the original input messages to the wrapper so they can be replayed after an overflow.
• Registers onErrorResume inside the Flux.using sandbox resource scope, ensuring the sandbox stays bound during compaction and retry.
• Adds recoverFromOverflowStream(): first performs emergency compaction, then replays the full event stream once. Errors thrown by the retry do not re-enter the handler, so there is no retry loop.
• Extracts the shared forceCompactContext(), so both the blocking call() and streamEvents() reuse the same compaction logic.
• Emergency compaction uses triggerMessages=1 / keepMessages=0 / keepTokens=0, so the full persisted context is compacted and oversized recent messages are not kept verbatim in the retried request.
• Applies the compacted result with AgentState.replaceContextPreservingAppends(), so messages appended concurrently during compaction survive; if the context prefix changed during compaction, recovery is aborted and the original overflow remains the primary cause.
• Calls saveAgentState() after compaction (off the reactive worker thread) so the persisted state is updated before the retry reloads it.
• Wraps recovery in ReActAgent.serializeForSession() so compaction and retry are serialized against other calls for the same session.
• Preserves the original provider error as the cause and attaches compaction/save failures as suppressed exceptions.
• Adds AgentState.replaceContext() / replaceContextPreservingAppends() and migrates full-context replacement call sites (StateBackedMemory, SubAgentTool, ReActAgent, AgentStateRestorer, SessionOperations, CompactionMiddleware) to the atomic APIs.
• Adds an OverflowRecoveryGate so streaming replay only happens before the first semantic event is emitted.

After the fix, the error reaches this recovery path:

Related Issues

• This PR fixes the issue where streamEvents() does not recover after a context overflow.

Related bug:

#3171

Scope / Known Limitations

• Overflow recovery on streamEvents() is gated to the window between the root AgentStart / ModelCallStart events and the first semantic event. If any text delta, tool call, or other non-opening event has already been emitted, the retry is skipped and the original provider error propagates, so tokens already shown to the user are never replayed.
• For the blocking call() path, recovery still retries the whole invocation when an overflow is detected, so an overflow after tool execution can repeat tool side effects. Per-model-call recovery for that path is left as a follow-up.
• When recovery runs, the stream emits a complete second lifecycle (AgentStart -> ... -> AgentEnd) for the retry. Downstream consumers that correlate events by lifecycle should expect one failed lifecycle followed by a successful one.
• Emergency compaction runs over the full persisted context with triggerMessages=1 / keepMessages=0 / keepTokens=0, writes the compacted state back before replaying, and preserves messages appended concurrently during compaction. If the context prefix changes during compaction, recovery is aborted and the original overflow remains the primary cause.
• Overflow detection is currently substring matching on the exception message: context_length_exceeded, context length, maximum context, token limit, too many tokens, exceeds the model's maximum, and reduce the length. New provider formats will require adding keywords or switching to structured error detection.

Tests

• HarnessAgentOverflowRecoveryTest covers real provider payload shapes, the recovery gate, no duplicate content, concurrent appends during compaction, and preservation of the original overflow as the cause.
• AgentStateTest covers replaceContext() and replaceContextPreservingAppends().

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

❌ Patch coverage is 70.00000% with 24 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...a/io/agentscope/core/memory/StateBackedMemory.java	0.00%	7 Missing ⚠️
...e/src/main/java/io/agentscope/core/ReActAgent.java	40.00%	5 Missing and 1 partial ⚠️
...main/java/io/agentscope/core/state/AgentState.java	87.87%	1 Missing and 3 partials ⚠️
...harness/agent/middleware/CompactionMiddleware.java	77.77%	2 Missing and 2 partials ⚠️
...e/spring/boot/admin/service/SessionOperations.java	80.00%	1 Missing and 1 partial ⚠️
...io/agentscope/core/tool/subagent/SubAgentTool.java	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Adds context-overflow recovery to streamEvents() by extracting the shared forceCompactContext() helper and replaying the event stream once after an emergency compaction. Direction looks right and aligns the streaming path with call(), but the replay currently duplicates already-emitted tokens (the streamed.length() > 0 skip mentioned in the description is not in the diff) and the new state mutation/persist has thread-safety and error-diagnostics gaps, plus this path has no test coverage.

Findings

• [Critical] HarnessAgent.java:1009 — replay after partial emission duplicates output; guard described in the PR body is missing
• [Warning] HarnessAgent.java:1144 — blocking saveAgentState inside a reactive flatMap
• [Warning] HarnessAgent.java:1141 — non-atomic contextMutable() clear/addAll on shared agent state
• [Warning] HarnessAgent.java:1010 — original provider error is dropped when recovery fails
• [Info] HarnessAgent.java:1005 — sandbox scope asymmetry vs. the blocking wrappedCall path
• [Warning] No test for the new streaming recovery path. A unit test with a model that fails once with a token-limit error and succeeds on retry would pin the three behaviours this change introduces: exactly one replay, no duplicated content events, and persisted compacted state.

Suggestions

1. Introduce the partial-emission guard you describe (or a reset marker event) before the replay.
2. Keep the original exception as the cause of any recovery failure.
3. Move saveAgentState off the reactive worker thread and make the context swap atomic.

---

Automated review by github-manager-bot

[oss-maintainer]

[Critical] Duplicate output on replay. onErrorResume here resubscribes with delegate.streamEvents(msgs, ...), but every AgentEvent already emitted by the failed attempt has already reached the subscriber, so a client that received partial text before the 400 will receive the whole answer twice. The PR body describes a streamed.length() > 0 skip, but that guard is not present in the diff. Could you either (a) track whether any content event was emitted and skip the replay in that case, or (b) emit an explicit reset/marker event before replaying so consumers can discard the partial output?

[oss-maintainer]

[Warning] Blocking call inside a reactive chain. ReActAgent#saveAgentState(RuntimeContext) is void and performs store I/O, so invoking it from this flatMap blocks whatever thread the compaction result is delivered on (potentially a Netty event loop for streaming model clients). Consider Mono.fromRunnable(() -> delegate.saveAgentState(effective)).subscribeOn(Schedulers.boundedElastic()), or an async state-store API if one exists.

[oss-maintainer]

[Warning] Shared-state mutation race. state.contextMutable().clear() followed by addAll(...) replaces the context of a live AgentState that other calls on the same (userId, sessionId) slot may be reading concurrently, and the intermediate empty state is observable. A copy-on-write swap (build the new list, then replace under the same lock the state store uses) would make the emergency compaction atomic for concurrent sessions.

[oss-maintainer]

[Warning] The original provider error is dropped. If forceCompactContext fails, the subscriber only sees Context overflow: emergency compaction yielded no result / no compaction configured, and the request id / provider message that made isContextOverflowError match is lost. Passing the caught exception as the cause (new RuntimeException(msg, e)) would keep the diagnostics that operators need.

[oss-maintainer]

[Info] Sandbox scope is now asymmetric between the two paths: here the retry runs inside Flux.using, so sandboxLifecycleMw.releaseForCall happens after the replay, whereas wrappedCall applies onErrorResume outside Mono.using, so the blocking recovery runs after the sandbox was already released. Both can be justified, but please confirm the intent — and if streaming-inside-scope is the correct semantics, it would be worth extracting one helper so the two paths cannot drift apart.

[oss-maintainer]

Summary

Re-review after the new commits since the previous pass (1900b58b → 09938206). Extending overflow recovery to the streaming path is a real gap in 2.0 and the design here is careful: the OverflowRecoveryGate limits replay to the pre-first-model-call window so tool executions and hooks can never be duplicated, the retry's duplicate opening events are filtered, cause chains are preserved via addSuppressed, and the whole-context compaction (keepMessages(0)) matches the intent. replaceContext preserving the live handle identity is the right shape. Two things hold this back from approval: the new synchronization contract is not enforced at existing call sites, and the snapshot→replace window can drop concurrent appends.

Findings

• [Warning] AgentState.java:190 — new "synchronize compound operations" invariant is violated by unmigrated callers (StateBackedMemory:64/76-77, SubAgentTool:288-289, ReActAgent:4384)
• [Warning] HarnessAgent.java:1163 — compacted snapshot is taken before the async compaction and applied after, so messages appended meanwhile are cleared, not merged
• [Info] HarnessAgent.java:1227 — opening-event suppression relies on a strict AgentStartEvent → ModelCallStartEvent prefix
• [Info] HarnessAgent.java:1129 — the effective behaviour comes from keepMessages(0), not keepTokens(0); worth a note plus a non-empty-result test for tool-pair tails
• [Info] HarnessAgent.java:1190 — prose substring matching can trigger a destructive full compaction from a non-overflow 400

Test coverage

HarnessAgentOverflowRecoveryTest (+337 lines) covers the real provider payload shapes, the cause chain, and the gate behaviour — good. Please add the concurrent-append case described above once the locking question is settled.

---

Automated review by github-manager-bot

[oss-maintainer]

The new javadoc makes "callers that perform a compound operation must synchronize on the returned list" a documented invariant, but the existing compound writers were not migrated, so they now violate it: StateBackedMemory.java:64/76-77 and SubAgentTool.java:288-289 still do contextMutable().clear(); contextMutable().addAll(...), and ReActAgent.java:4384 still clears the handle. A concurrent getContext()/replaceContext() can observe the cleared window or throw ConcurrentModificationException. Since replaceContext(List) is exactly what those sites need, please migrate them in this PR (or at least the two clear()+addAll() pairs) instead of leaving the new contract unenforced — otherwise this is a thread-safety regression risk introduced by the synchronizedList switch without the corresponding call-site fix.

[oss-maintainer]

The compacted replacement is built from the snapshot taken in forceCompactContext via state.getContext() (a defensive copy), and only applied here, after the async compaction call completes. Any message appended by another writer during that window (sub-agent persistence, memory flush middleware, or the contextMutable().addAll(msgs) at ReActAgent.java:2291) is dropped by replaceContext, because it clears rather than merges. Consider either performing the snapshot+replace under synchronized (state.contextMutable()), or merging: replaceContext(compact(old) ++ newSinceSnapshot). A comment stating that recovery is only safe because the failed call holds the sandbox/per-session lock would also be enough to make this reviewable.

[oss-maintainer]

suppressRetryOpening assumes the retry stream begins strictly with the root AgentStartEvent and then the root ModelCallStartEvent. The filter also runs its state machine on every non-matching event, so if the delegate emits anything before AgentStartEvent (or if a future version drops ModelCallStartEvent for a cached/prehandled call), the original opening events are forwarded and the client sees a duplicated agent_start/model_call_start pair. Since this is cosmetic but hard to debug, consider takeWhile-style explicit prefix skipping, or asserting the invariant with a log.warn when the first event is not a root AgentStartEvent.

[oss-maintainer]

Worth spelling out the CompactionConfig semantics this relies on, because they are not obvious: per CompactionConfig.getKeepTokens() javadoc, keepTokens == 0 means "use keepMessages" (not "keep zero tokens") and -1 is the dynamic mode, so the effective behaviour here comes from keepMessages(0) → findMessageBasedCutoff returning messages.size(), i.e. summarise everything. findSafeCutoffPoint can then move that cutoff backwards when the tail starts with TOOL messages. Please add a note (and a test asserting the compacted result still contains the summary block for a context ending in a tool-call/tool-result pair) so the emergency path cannot regress into producing an empty context that fails the very next model call.

[oss-maintainer]

The two new patterns are loose text matches on a toLowerCase() blob. exceeds the available context is specific enough for the provider messages in HarnessAgentOverflowRecoveryTest, but isContextOverflowError is also reached for non-model failures; a generic error mentioning "context length" in an unrelated validation message would now trigger an emergency full-context compaction — a destructive operation. Consider gating on the provider error code/status (e.g. HTTP 400 plus exceed_context_size_error / context_length_exceeded) instead of free-form prose, or requiring the failure to originate from a model call.

[oss-maintainer]

Summary

Delta re-review after 09938206 → 73e39e25 (make whole-context replacement atomic and preserve concurrent appends). Both blocking points from my previous pass are addressed, and the way they were fixed is better than what I suggested. Every compound contextMutable() reader/writer I listed is migrated to getContext() + replaceContext(...) (StateBackedMemory, SubAgentTool.applyLoadedState, AgentStateRestorer.restore, ReActAgent.clearContext / compressStructuredOutputContext / prepareSummaryMessages, CompactionMiddleware.applyToContext, the aistio SessionCompactor javadoc), AgentState now documents the identity-vs-CAS split, and replaceContextPreservingAppends closes the snapshot→apply window with a real CAS that keeps post-snapshot appends instead of dropping them. forceCompactContext moving under serializeForSession is the correct fix for the interleaving concern, and streamEvents_preservesMessagesAppendedDuringCompaction covers the new guarantee. CI is green on this head (Check License, Check Module Sync; license/cla=success on 73e39e25), CLA signed.

What I am not yet comfortable approving is the blast radius of the new serialization plus the fact that the same race survives on the routine compaction path:

Findings

• [Warning] CompactionMiddleware.java:225 — applyToContext still applies with the non-preserving replaceContext after an async model round trip, so the everyday (non-overflow) compaction path keeps the exact drop-concurrent-appends window this commit closed for overflow. Arguably the higher-traffic of the two.
• [Warning] HarnessAgent.java:1177 — a lost CAS now fails the user's whole turn with Context overflow: context changed during emergency compaction, and the writers that can move the prefix (admin SessionOperations.compact, SubAgentTool.applyLoadedState, AgentStateRestorer.restore, direct contextMutable() appends) all bypass the session gate. A single re-read/re-CAS retry, or a degrade-with-WARN, would be a friendlier failure mode.
• [Warning] ReActAgent.java:745 — serializeForSession holds a non-reentrant gate with no timeout across a full compaction LLM call: a stalled provider now head-of-line-blocks every later same-session call (before this commit the shrink ran outside the gate). Suggest bounding the action with a timeout, and stating the non-reentrancy contract in the javadoc now that AgentBase.serializeOnKey is protected final.
• [Info] AgentState.java:231 — the prefix CAS is identity-based (Msg does not override equals), so a snapshot taken before a store reload always rejects, indistinguishable in logs from a genuine conflicting write. Worth one javadoc sentence plus a debug log on the reject branch.
• [Info] HarnessAgent.java:1013 — dropping suppressRetryOpening makes the retry emit a second complete AgentStartEvent/ModelCallStartEvent lifecycle (test now asserts 2, previously 1). That is the more honest event model and I am not arguing to restore the fragile prefix filter, but it is consumer-visible for console/AG-UI transcript rendering and per-turn accounting, so it should be called out in the PR description and cross-checked against #3186/#3188.

Verdict

COMMENT — no request-changes; the correctness of the overflow path itself improved materially in this commit and the remaining items are about extending the same guarantee to CompactionMiddleware and bounding the new gate. The CompactionMiddleware item is the one I would most like settled before merge.

---

Automated review by github-manager-bot

[oss-maintainer]

The routine compaction path still has the exact race this commit closes on the overflow path.

conversation is snapshotted from input.messages() at the top of onReasoning, handed to compactIfNeeded(...) (an async model call), and the result is then applied with the non-preserving replaceContext — so any message appended to the live context while the compaction model call was in flight is cleared rather than merged. That is the same window replaceContextPreservingAppends was introduced for in HarnessAgent.forceCompactContext.

The overflow path is now protected but the everyday path is not, and the everyday path runs on every turn that crosses the trigger threshold, so it is the one more likely to be hit in practice. Suggest threading the snapshot through and using the CAS variant:

AgentState state = RuntimeContext.resolveAgentState(rc, reActAgent);
List<Msg> snapshot = state.getContext();
// ... compactIfNeeded(snapshot) ...
if (!state.replaceContextPreservingAppends(snapshot, compacted)) {
    log.warn("Context changed during compaction, skipping application ({} -> {})",
            snapshot.size(), compacted.size());
}

Note this needs the live snapshot as the CAS base, not conversation, otherwise the identity check can never match once a turn has appended its own messages. If there is a reason the middleware path is exempt (e.g. the reasoning input is always the authoritative context), a one-line javadoc note here would save the next reader the same question.

[oss-maintainer]

Failing the user's whole turn is a hard response to a lost CAS — and the CAS is now losable by design.

With serializeForSession the recovery holds the session gate for its full duration, so a same-session agent call can no longer move the prefix. What can still move it is every writer that does not go through the gate: the admin SessionOperations.compact, SubAgentTool.applyLoadedState, AgentStateRestorer.restore, and any contextMutable() direct append (the new test drives the rejection path exactly that way). Those are all legitimate concurrent writers, and today the user just gets RuntimeException("Context overflow: context changed during emergency compaction") on top of the original overflow — i.e. the one mechanism meant to save an overflown session silently becomes a new failure mode.

Since the whole point of replaceContextPreservingAppends is that appends after the snapshot are compatible with the replacement, a cheap retry looks safe here: re-read state.getContext(), re-run the shrink against the new base once, and only then give up:

if (!state.replaceContextPreservingAppends(allMsgs, opt.get())) {
    List<Msg> reread = state.getContext();
    List<Msg> retry = shrinkKeepingTail(reread, opt.get());   // or compactIfNeeded(reread, ...)
    if (!state.replaceContextPreservingAppends(reread, retry)) {
        return Mono.error(new RuntimeException(
                "Context overflow: context changed during emergency compaction", overflowError));
    }
}

Even without the retry, please consider whether REQUEST-level failure is the intent — a WARN + "compaction skipped, retrying without shrink" would keep the previous behaviour for the contended case.

[oss-maintainer]

serializeForSession holds a non-reentrant, non-timeout gate across a full model call.

The action passed here is forceCompactContext(...), whose chain includes ConversationCompactor.compactIfNeeded — i.e. a blocking-on-the-session LLM round trip. Two consequences worth a deliberate decision:

1. No bound. serializeOnKey has no timeout, so if the compaction model call hangs (provider stall, no timeout() on that path), every subsequent same-session call queues behind it indefinitely. Before this commit the shrink ran outside the gate, so a stalled compaction cost latency but never head-of-line-blocked the session. A .timeout(compactionTimeout) (or reuse of the model's request timeout) on the action inside forceCompactContext would bound the blast radius.
2. Re-entrancy. The gate self-queues: a caller that invokes serializeForSession from inside an already-gated section waits on its own release. AgentBase.serializeOnKey is now protected final, so subclasses can reach it directly and the trap is not visible from the signature. A javadoc line on serializeForSession ("must not be called from within a gated call lifecycle; the gate is not reentrant") would make the contract explicit — the current overflow-recovery call site is safe only because the enclosing call has already terminated with the overflow error by the time onErrorResume runs.

[oss-maintainer]

The prefix CAS is identity-based — worth stating in the javadoc.

Msg does not override equals, so context.subList(0, expectedSize).equals(expected) compares message instances, not content. For the in-JVM getContext() → mutate → CAS flow this is exactly right (and stricter is better). But it also means a state that was reloaded from the AgentStateStore in between (new Msg instances, same content) always fails the CAS, and the failure is indistinguishable from a real conflicting write in the logs.

One sentence in the javadoc — "comparison is by message identity; a snapshot taken before a state reload will not match" — plus a debug log on the reject branch would make the behaviour obvious to the next caller of this method (and to whoever reads the Context overflow: context changed ... error).

[oss-maintainer]

Dropping suppressRetryOpening is a consumer-visible contract change — flagging so it lands consciously.

The recovery stream now emits its own root AgentStartEvent / ModelCallStartEvent pair, and the updated test asserts 2 of each where it previously asserted 1. That is the simpler and more honest event model (my earlier round noted the suppression relied on a fragile AgentStart → ModelCallStart prefix assumption, so I'm not arguing for bringing it back), but any downstream that treats one user request as exactly one agent lifecycle — console transcript rendering, AG-UI replay, per-turn usage accounting — will now see two lifecycles for a single request on overflow only, which is precisely the case that is hard to reproduce in review.

If the agentscope-agui / console side of this repo already keys transcripts off replyId rather than event counts, a note in the PR description (or a line in the recovery javadoc saying "the retry emits a complete second lifecycle") is all this needs. #3186/#3188 touch adjacent channel paths, so worth a cross-check there too.

[oss-maintainer]

Follow-up on the review above (posted a few minutes ago): the head moved between my triage scan and the review, so for the record I reviewed the equivalent of 09938206 → 61e5c814 (73e39e25 and 61e5c814 are the same change set, 61e5c814 rebased). The review's inline anchors all resolved against 61e5c814, and nothing in the four findings changes.

One thing in 61e5c814 worth calling out positively, because it sharpens my remaining point: SessionOperations.compact now handles a lost CAS correctly — it restores previousSummary, logs a WARN and returns a CompactResponse describing the unchanged session instead of claiming a compaction that did not happen. That is exactly the degrade-not-fail shape I asked for at HarnessAgent.java:1177, where an equivalent lost CAS still aborts the user's turn with Context overflow: context changed during emergency compaction. Aligning the two paths (or at least retrying once with a re-read snapshot on the recovery side) would close the last substantive item from my side.

Automated review follow-up by github-manager-bot

[oss-maintainer]

Summary

Nice follow-up — 7d679d1 closes most of the concurrency findings from the previous review: the routine compaction path now goes through the same atomic apply as the overflow path, emergency compaction is serialized against the session gate instead of racing it, the whole-context clear()/addAll() swap is gone, suppressRetryOpening and the two loose overflow-text patterns were dropped, and the new replaceContextPreservingAppends has direct unit coverage including the appended-during-compaction case on the streaming path. What remains is the shape of the new CAS itself: it is identity-based and its failure is swallowed differently on each of the two paths.

Findings

• [Warning] agentscope-core/.../state/AgentState.java:218 — the prefix check compares by object identity (Msg has no equals), so a benign reload/rewrite of the prefix is indistinguishable from a real concurrent change; both recovery paths then fail as if there were a conflict.
• [Warning] agentscope-harness/.../middleware/CompactionMiddleware.java:228 — on CAS failure the routine path only logs, so compaction can quietly become a permanent no-op (over-budget context plus one paid-for compaction model call every turn) while the overflow path fails the turn loudly.

Resolved since last review

HarnessAgent.java:1141 (shared-state mutation race), CompactionMiddleware.java:225 (same race on the routine path), ReActAgent.java:745 (serializeForSession now guards the recovery action), and HarnessAgent.java:1013 (opening-event contract restored by removing the filter) — all addressed, with streamEvents_preservesMessagesAppendedDuringCompaction covering the streaming case.

Overall: the direction is right; the two warnings are about making the new guard fail in a way you can notice.

---

Automated review by github-manager-bot

[oss-maintainer]

The prefix guard is identity-based, and Msg does not override equals, so context.subList(0, expectedSize).equals(expected) only succeeds when every prefix element is the same instance. That makes the method reject two situations it cannot distinguish: a genuine concurrent rewrite of the prefix, and a benign re-materialisation of it (a session reload, compressStructuredOutputContext filtering the list, replaceContext(List.of()) followed by a re-append). In those cases overflow recovery reports "context changed during emergency compaction" and the routine path logs and skips, even though nothing was lost. Since this is now the single mechanism protecting both compaction paths, it is worth either stating the identity-prefix requirement in the javadoc above this signature, or comparing by Msg identity/id so that a reload does not look like a conflict — and giving the two callers distinct failure modes so a permanently failing CAS is not mistaken for normal contention.

[oss-maintainer]

With the false branch reduced to a log.warn, the routine compaction path can fail its CAS forever without anything surfacing: the model call proceeds with the still-over-budget context, the compaction's model call has already been paid for, and the next turn repeats both. Combined with the identity-based prefix check above, a session that is reloaded between snapshot and apply (or whose memory offload removed a mid-context message) ends up in exactly that state — compaction becomes a silent no-op that costs one extra model call per turn. The overflow path at least fails the turn loudly. A counter plus a one-time warn (or applying the replacement to the prefix that is still present rather than skipping entirely) would keep the fix from degrading invisibly.

[oss-maintainer]

Summary

Re-reviewed the new commit 53b84c5b. Both findings from my previous round are fixed, and fixed in the way I would have chosen: the prefix guard now compares by Msg id + role instead of object identity (so a store round trip or snapshot restore is no longer mistaken for a conflict, while an id-less prefix stays closed), and applyToContext returns a boolean so a rejected replacement on the routine path is reported with the snapshot/current/compacted sizes instead of looking like a successful compaction. The migration of the compound writers (StateBackedMemory, SubAgentTool, AgentStateRestorer, compressStructuredOutputContext, admin compact) to atomic replaceContext calls removes the observable empty window, and the new AgentStateTest / CompactionMiddlewareTest / SessionOperationsTest cases pin the accept-on-rebuild and reject-on-rewrite semantics in both directions. CI is green on this head (build ubuntu/windows, License, Module Sync).

One item left, then this is good to go from my side.

Findings

• [Warning] agentscope-core/.../ReActAgent.java:1453 — compressStructuredOutputContext is the one migrated call site that uses the non-preserving replaceContext: it filters a getContext() snapshot and overwrites the live list, so a concurrent append between the read and the write is dropped silently. Every other compaction/state-swap site in this PR moved to the preserving variant; this one kept the lossy shape.
• [Info] agentscope-core/.../state/AgentState.java:281 — id + role is a deliberately loose equality; worth a test asserting the intended outcome for a message that keeps its id while its content blocks are mutated in place (streamed assistant/tool-accumulation messages).

Resolved since last review

• AgentState.java:218 (identity-based prefix check rejecting benign rehydrates) — replaced by prefixMatchesSnapshot / representsSameMessage, documented, and covered by replaceContextPreservingAppendsAcceptsPrefixRehydratedFromState plus the two reject cases.
• CompactionMiddleware.java:228 (routine path swallowing a lost CAS) — now a WARN with sizes and a boolean result, and the javadoc states plainly that the persisted context stays over budget and compaction re-runs next turn.
• serializeForSession on both recovery paths closes the non-gated-writer race I raised earlier, and dropping suppressRetryOpening + the two loose overflow-text patterns simplified the matching surface as suggested.

Only the first bullet is actionable; it is a one-line change and does not need another round of design discussion.

---

Automated review by github-manager-bot

[oss-maintainer]

This call site was migrated to the non-preserving variant, so it can still drop a concurrent append. original is a snapshot taken by getContext(), retained is a filter of it, and replaceContext(retained) then overwrites the live context unconditionally — any message appended between the read and the write (the same non-gated writers this commit enumerates elsewhere: SessionOperations.compact, AgentStateRestorer.restore, SubAgentTool.applyLoadedState) disappears with no log line.

That is the exact failure mode replaceContextPreservingAppends was introduced for, and the two compaction paths now use it. Since this method already has the snapshot in hand, the fix is one word:

if (!agentState.replaceContextPreservingAppends(original, retained)) {
    log.warn("Structured-output compression skipped: context changed while it was running");
}

If the intent is that structured-output compression must win even over a concurrent append, worth a one-line comment saying so — right now it reads like the plain-replace was chosen for convenience rather than semantics.

[oss-maintainer]

representsSameMessage treats id + role as sufficient, so a message that keeps its id while its content changes passes the guard. Ids are random UUIDs assigned once, so for persisted messages that is unreachable in practice — but the class of mutation that does happen in this codebase is a streamed assistant message whose content blocks are filled in place (tool-use accumulation on the accumulator path). Worth one test pinning the assumption you are relying on, e.g. snapshot a context containing a partially built assistant message, mutate its content blocks while keeping the id, then assert whether the CAS accepting it is the intended outcome. If "same id, different content" should be rejected, a cheap discriminator (block count, or a text-length sum) would catch it without comparing full content.

[oss-maintainer]

Summary

Re-reviewed the amended head 2a1a56e8. The only difference against 53b84c5b is ReActAgent.compressStructuredOutputContext (verified by a file-level blob comparison across all 14 files in the PR — every other blob is identical), and it is exactly the fix asked for in my previous round: the structured-output compaction now applies its replacement with replaceContextPreservingAppends(original, retained) and reports a rejection at WARN with the snapshot/retained sizes instead of silently overwriting.

That closes the last blocking point. All three compaction/rewrite paths — routine (CompactionMiddleware), overflow (HarnessAgent) and structured-output (ReActAgent) — now share one CAS discipline, so no in-flight append can be dropped by any of them, and none of them can fail invisibly. The javadoc added on the method also documents the reject semantics (scaffolding stays in the context, conversation still usable), which is the behaviour I would want an operator to be able to reason about from the log line alone.

Remaining non-blocking item, left open deliberately:

• [Info] agentscope-core/src/main/java/io/agentscope/core/state/AgentState.java:281 — representsSameMessage accepts id + role as sufficient. Not a real risk today (ids are UUIDs assigned once, and content-derived for generated messages), but a test pinning "same id, mutated content blocks" would document the contract for the accumulator path. Your call — it does not gate this PR.

CI at the time of review: build (ubuntu-latest) pass, Check License pass, Check Module Sync pass, codecov/patch pass, license/cla signed. build (windows-latest) was still queued/running — noting it explicitly, this approval is of the code, not a claim that the whole suite is green.

LGTM, thanks for the quick turnaround on this one.

---

Automated review by github-manager-bot

[oss-maintainer]

Follow-up on my approval: build (windows-latest) has since completed and is pass (26m15s) on 2a1a56e8, so every required check on this head is now green (ubuntu + windows build, Check License, Check Module Sync, codecov/patch, license/cla). The PR is MERGEABLE; it still shows BLOCKED only because it needs a maintainer review/approval to satisfy branch protection — merging is not something I perform.

---

Automated note by github-manager-bot

[oss-maintainer]

Correction to my note above, posted a minute later: my APPROVED review has now registered, so this head is MERGEABLE / CLEAN with reviewDecision = APPROVED — it is no longer blocked on review. Ready for a maintainer to merge (I do not merge).

---

Automated note by github-manager-bot