fix(state): return unconditional write versions atomically

[pengmoubuaixuexi]

AgentScope-Java Version

2.0.3-SNAPSHOT, based on main at 2b378e158ce1e555cd98c67d114d43cbc316af73.

Description

Fixes #3076.

Concurrent unconditional InMemoryAgentStateStore.saveIfVersion calls can both return the later write's version: the method saves first, then reads the version separately. A caller whose state was overwritten can consequently use that newer version to pass a subsequent CAS incorrectly.

Route unconditional writes through the existing synchronized CAS helper, skipping the comparison for UNVERSIONED and returning the version assigned inside the same critical section. Plain save() delegates to the same helper with UNVERSIONED, keeping version increment and write logic in one synchronized operation. Conditional writes retain their existing behavior.

The Javadoc on both entry points explicitly documents that saveIfVersion, including unconditional writes, does not invoke the public save method. Subclasses that intercept single-state writes need to override both entry points.

The regression test forces the old write/read interleaving with a barrier after super.save() in a test-only subclass. It checks distinct versions, their association with the persisted state, and rejection of the earlier writer's stale version. The test fails on the unmodified implementation and passes with this patch. An additional local stress reproduction using the ordinary store produced 2,563 duplicate versions in 8,000 writes before the fix, and zero after it.

A second committed test uses an ordinary store with no method overrides: four workers each perform 200 unconditional writes, rendezvousing before every round. It checks all 800 returned versions are unique and contiguous, the final state belongs to the final returned version, and a stale-version write is rejected. This contention test complements the targeted old-interleaving regression; it does not rely on the public save() hook or guarantee a particular thread schedule.

Validation

• Baseline existing versioning contract: 6 tests passed.
• Both committed concurrency tests fail against unmodified current main c5db8f72dbea5c2c70de96885e4167ce5b1b24b0 in an isolated checkout: the targeted regression returns version 2 twice, and the 800-write test detects a duplicate returned version (2 failures, 0 errors).
• After the fix: 43 tests passed, zero failures/errors/skips:

mvn -B -pl agentscope-core -am '-Dtest=*StateStore*Test,ReActAgentPerSessionStateTest' '-Dsurefire.failIfNoSpecifiedTests=false' '-Dspotless.skip=true' test

• Spotless apply/check passed for both changed files:

mvn -B -pl agentscope-core '-DspotlessFiles=.*InMemoryAgentStateStore.java,.*InMemoryAgentStateStoreConcurrencyTest.java' spotless:apply spotless:check

• git diff --cached --check passed.
• Environment: Windows 11, Temurin 17.0.15, Maven 3.9.12. The full multi-module test suite was not run locally.

Checklist

• Changed code has been formatted with Spotless.
• Relevant state-store and per-session regression tests pass.
• Full repository mvn test (not run locally).
• Public method signatures and version semantics are preserved; the change to public save() delegation is documented for subclasses.
• Code is ready for review.

AI assistance was used to find, implement, and test this change. Validation results above are from commands executed against the local checkout.

[CLAassistant]

All committers have signed the CLA.

[Copilot]

🟢 Approval recommended

The change directly addresses the demonstrated race and includes a focused regression test covering the previously broken interleaving and expected versioning semantics.

Pull request overview

Fixes a concurrency race in InMemoryAgentStateStore.saveIfVersion(..., UNVERSIONED) where two concurrent unconditional writers could both return the later writer’s version, breaking the versioning/CAS contract described in #3076.

Changes:

• Route unconditional writes through the synchronized CAS path so the write and returned version are produced within the same critical section.
• Update CAS helper to treat UNVERSIONED as “skip compare” and to increment from the current stored version.
• Add a deterministic concurrency regression test that fails on the pre-fix interleaving and validates correct version/state association and stale-version rejection.

File summaries

File	Description
agentscope-core/src/main/java/io/agentscope/core/state/InMemoryAgentStateStore.java	Makes unconditional saveIfVersion return the version assigned to its own write atomically via the per-session synchronized CAS helper.
agentscope-core/src/test/java/io/agentscope/core/state/InMemoryAgentStateStoreConcurrencyTest.java	Adds a regression test that forces the problematic interleaving and verifies distinct returned versions and correct CAS rejection behavior.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pengmoubuaixuexi]

Addressed in 693cf18. setSingleState now delegates to casSingleState(key, value, UNVERSIONED), so plain saves and unconditional versioned saves share the version increment/write logic. Synchronization stays in casSingleState; the delegating method no longer needs a second synchronized entry. Re-ran the 42 state-store/per-session tests and Spotless apply/check for the changed files: all passed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[dailingtao]

LGTM

[oss-maintainer]

Summary

Real race: saveIfVersion(..., UNVERSIONED) wrote and then re-read the version outside the critical section, so two concurrent unconditional writers could both return the later version and poison a subsequent CAS. Funneling unconditional writes through casSingleState and deriving next from current is the right fix, and the stale-version rejection is asserted. Two notes: the public save() hook is now bypassed on that path, and the regression test's barrier only engages the buggy shape.

---

Automated review by github-manager-bot

[oss-maintainer]

Routing unconditional writes through casSingleState fixes the write-then-read race correctly, but it also means save() is no longer on the saveIfVersion(..., UNVERSIONED) path. save is public and overridable (the repo's own SessionSandboxStateStoreTest.NoDeleteSession subclasses this store), so a subclass hook that used to fire on an unconditional versioned write now silently does not. Worth either documenting the new delegation on save(), or keeping save() as the single funnel.

[pengmoubuaixuexi]

Addressed in 2f73def by taking the documentation option: the Javadoc on both save() and saveIfVersion() now explicitly states that versioned writes, including UNVERSIONED, go directly through the internal atomic operation and do not invoke the public save() override. It also tells subclasses that intercept single-state writes to override both entry points.

This retains the atomic write-and-return path without expanding the change to a new subclass hook API. I also checked NoDeleteSession: it overrides delete(), so that particular subclass does not depend on the old save() delegation. The relevant state-store/per-session suite passes all 43 tests, and Spotless passes for both changed files.

[oss-maintainer]

The barrier only trips on the buggy baseline (after the fix save() is never called from this path), so on main the test asserts distinct versions with effectively no concurrency stress — it guards the exact regression but is weak as a general stress test. The 8k-write stress reproduction you ran locally is the stronger signal; consider keeping a small deterministic loop here (e.g. 4 threads x 200 unconditional writes asserting all returned versions are distinct) so the guard survives refactors of the hook itself.

[pengmoubuaixuexi]

Addressed in 2f73def. I kept the targeted old-interleaving regression and added concurrentUnconditionalWritesReturnUniqueVersions using a plain InMemoryAgentStateStore, with no overridden hooks. Four workers each perform 200 writes, rendezvousing at a barrier before each round. The test checks all 800 returned versions are unique and contiguous, the final persisted state matches the write that returned the final version, and a stale-version write is rejected without changing the state.

I ran both tests against unmodified current main (c5db8f72dbea5c2c70de96885e4167ce5b1b24b0) in an isolated checkout: both failed on duplicate returned versions (2 failures, 0 errors). On the PR branch, all 43 relevant state-store/per-session tests pass, and Spotless passes. The new test adds contention on the real write path; the original test remains the deterministic guard for the specific old interleaving, since a contention loop cannot guarantee a particular thread schedule.

[oss-maintainer]

Summary

Documents and tests the invariant that unconditional writes through saveIfVersion(..., UNVERSIONED) bypass the save(...) entry point and return a unique, monotonically increasing version per write, determined in the same critical section as the state write itself.

The delta since my previous review is javadoc plus a concurrency test — no production code change. The doc additions are the valuable part: they make explicit that a subclass intercepting single-state writes by overriding save(...) will not observe unconditional saveIfVersion(...) calls, which is a genuine footgun for the AgentStateStore SPI and exactly the kind of thing that otherwise gets discovered as a production bug in a custom store implementation.

The test is well constructed for its purpose:

• 4 writers × 200 writes contending on the same (user, session, key) through a CyclicBarrier releases them per round, so the versions really are produced under contention rather than sequentially;
• asserting assertNull(writesByVersion.put(version, value)) on every collected write catches a duplicate version without needing to know which writer produced it, and the subsequent 1..800 contiguity check plus latest.version() == 800 rules out both duplication and gaps;
• HashMap is used only on the main thread after Future#get(), so there is no unsynchronised access in the test itself;
• the final block pins the other half of the contract — a stale expectedVersion returns UNVERSIONED and leaves the stored state untouched, verified by re-reading.

Findings

No defects found in this delta. One suggestion below.

Suggestions

The barrier is awaited inside the per-iteration loop before each write, so it synchronises the start of each write rather than batching writes into rounds. That is fine for the property under test (each write still lands on whatever state the previous writes produced), but it means the assertion assertEquals(totalWrites, writesByVersion.size()) depends on all 800 writes succeeding, and a CyclicBarrier timeout here would surface as a BrokenBarrierException inside Future#get() rather than as a clear test failure. Under a heavily loaded CI runner with @Timeout(15) this is the most plausible source of future flakiness; if it ever trips, the barrier is the first place to look rather than the store.

Note

InMemoryAgentStateStore.java:58 and the second javadoc block are documentation-only, so no behavioural risk. CLA is signed and CI is green (MERGEABLE/BLOCKED — the BLOCKED state is branch protection awaiting a maintainer approval, not a failure).

---

Automated review by github-manager-bot