Feature/mongodb extension - #2698
Conversation
…simplify CAS - Remove duplicate ascending index on createdAt in MongoRemoteSnapshotClient; the TTL index already provides the same sorting capability - Add Objects.requireNonNull guards to MongoBaseStore constructor for database and collectionName parameters - Simplify MongoBaseStore.putIfVersion() return: findOneAndUpdate with version filter already guarantees the result matches expectedVersion + 1
…n MongoAgentStateStore - save() now increments version on each call (Updates.inc) - saveIfVersion(UNVERSIONED) reads version directly from MongoDB document instead of deserializing through State.class interface - saveIfVersion(0) catches MongoCommandException in addition to MongoWriteException for DuplicateKey errors from findOneAndUpdate - TTL index changed from expireAfterSeconds=0 to 30 days to prevent silent data loss by MongoDB TTL monitor - ensureIndexes gracefully handles IndexOptionsConflict (error 85) when upgrading from old TTL index parameters test(extensions-mongodb): add contract tests for BaseStore, AgentStateStore and index lifecycle - MongoBaseStoreContractTest (6 tests): CRUD, CAS, idempotent delete, search - MongoAgentStateStoreContractTest (6 tests): versioning, CAS concurrency - MongoIndexLifecycleContractTest (7 tests): index parameters, TTL values, upgrade from old TTL=0 without IndexOptionsConflict
… safety Assumptions.abort() in @BeforeAll does not prevent @afterall from running. When MongoDB is unreachable, @afterall calls client.getDatabase(dbName).drop() which blocks for 60s then throws MongoTimeoutException — causing CI failure. Fix: add `connected` flag, set only on successful ping. @afterall guards all MongoDB operations behind `if (connected)`. Also move dbName assignment before the try-catch to avoid null in disconnect.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
AgentScopeJavaBot
added
enhancement
|
Hi maintainers, just a gentle ping on this PR – would appreciate a review when you have a moment. 🙏 One thing to note: the CI failure currently showing is not caused by this PR. The CI was all green before I merged the latest PR link for reference: #2698 Thanks for your time! |
|
Solid module overall — well-structured, consistent with the Redis/MySQL extensions, and clearly validated 🔴 Blocker
The full-rewrite and shrink branches both setOnInsert(user_id/session_id) on upsert; the append branch does 🟠 Fix before release
Snapshot TTL is 7 days on createdAt (write-once); session TTL is 30 days on _updated_at (bumped every
Lease TTL and acquisition timeout both derive from lockTimeoutMs (default 30m), with no renewal/heartbeat. 🟡 / 🟢 Minor
Checked and fine MongoLease.close() single-arg eq(lockId) (it's an _id match); saveIfVersion(...,0) create-if-absent; the Bottom line: please fix #1 before merge and prioritize #2/#3; the rest can be scheduled. Nice work overall. |
… blocker, TTL alignment, lease renewal Address all 7 findings from the code review. Blocker: - save(List): add $setOnInsert(user_id/session_id) to the incremental-append branch. A brand-new session whose first write is a non-empty list landed in this branch and was upserted without the identity fields, making it permanently invisible to listSessionIds. Fix before release: - Align snapshot TTL with the session TTL (7d -> 30d) so a live session's snapshot is never reclaimed first; add the error-85 drop/recreate migration so any future TTL adjustment takes effect instead of silently keeping the old index options. - MongoSandboxExecutionGuard: split leaseTtl (lock document lifetime, default 30m) from lockTimeout (acquisition wait), and renew the lease every leaseTtl/3 via a background watchdog while the lease is open, so a long execution no longer loses its own lock to TTL reclamation. Minor: - download(): treat a document without a data field as FileNotFoundException instead of NPE (parity with the Redis implementation). - fromConnectionString: fix javadoc — the store owns and closes the client. - Document that save(List) is intentionally non-atomic (single-writer guarantee comes from SandboxExecutionGuard) and that validateKey covers top-level keys only. Tests: 112 -> 124. New regression coverage: append-branch $setOnInsert (unit + contract), first-save-as-list visibility in listSessionIds (contract), snapshot 30-day TTL and index migration (contract + unit), lease TTL separation, watchdog renewal, close-stops-renewal and lost-lock tolerance (unit). All 124 pass locally; contract tests run against a real MongoDB.
|
Thanks a lot for the thorough review — and especially for re-verifying every point against the latest commits. The blocker is real (and exactly the kind of bug mocks can't catch), and #2/#3 are genuine correctness gaps. All 7 items are addressed and pushed to the same PR branch; details inline below. 🔴 1. Sessions first saved as a non-empty List are invisible to listSessionIds — FIXED Added
🟠 2. Snapshot TTL (7d) < session TTL (30d) — FIXED (TTL alignment) The snapshot TTL is now 30 days, aligned with the session TTL, and the class javadoc states the invariant (a snapshot must not be reclaimed while its session is alive). Two implementation notes:
🟠 3. A long task can lose its own lock → double execution — FIXED
🟡 4. save(List) is non-atomic — confirmed intentional The read-then-write is deliberate: the harness serialises calls per isolation slot via 🟢 5. download() NPE when the data field is missing — FIXED A null 🟢 6. fromConnectionString javadoc — FIXED Removed the incorrect "caller is responsible for closing the client" sentence; the method javadoc now states that the store owns and closes the client, matching the class-level docs and 🟢 7. validateKey only guards the top-level key — documented, behavior unchanged I deliberately don't rewrite nested field names: renaming them on write would break deserialization back into the original Verification
Thanks again for the careful review — please let me know if there's anything else I can adjust before merge. |
There was a problem hiding this comment.
Went back over the 7 issues from the previous review. The high-severity ones are all fixed now (the append
branch adds setOnInsert, the snapshot TTL is bumped to 30 days, long-running tasks get a watchdog to renew
the lease, the download NPE, and the javadoc contradiction). The non-atomic list save and the un-validated
nested keys are now documented as intentional behavior in the javadoc. Compared to the first review, this
version has clearly converged.
This round I found 3 new issues: #1 is worth fixing before merge (the only one that's inconsistent across
backends and touches public-API semantics — the change is tiny); #2 and #3 are optional cleanups. See the
inline comments for details.
larry-zy
left a comment
larry-zy
left a comment
There was a problem hiding this comment.
Took another pass over this after the last round. The high-severity items all look handled now — the append path sets user/session on insert, the snapshot TTL is bumped to 30 days, long-running tasks get a watchdog that renews the lease, the download NPE is guarded, and the javadoc contradiction is fixed. The non-atomic list save and the un-validated nested keys being documented as intentional is fair enough.
Left three notes below. Only the first is one I'd want sorted before merge — it's a small change, but right now Mongo behaves differently from every other store on a public API. The other two are just nice-to-haves, take them or leave them.
| this.renewalExecutor = | ||
| Executors.newSingleThreadScheduledExecutor( | ||
| runnable -> { | ||
| Thread thread = new Thread(runnable, "mongo-sandbox-lease-renewal"); | ||
| thread.setDaemon(true); | ||
| return thread; | ||
| }); |
There was a problem hiding this comment.
Not blocking, just flagging: renewalExecutor is a single-threaded scheduler shared by every lease this guard hands out. If one lease's renew() updateOne hangs on a network hiccup, it blocks the renewals for all the other leases too — and if it stays stuck long enough they can expire and get reclaimed, which is exactly the double-execution this guard is meant to prevent. With the defaults (leaseTtl=30min, renewing every leaseTtl/3 ≈ 10min) it'd need to hang for ~20min before a lock is actually lost, so it's unlikely, but not impossible. A small fixed thread pool, or just keeping the Mongo socket timeout well under leaseTtl/3, would stop one stuck renewal from taking the rest down with it.
Separately, the guard has no close() (the SandboxExecutionGuard interface isn't AutoCloseable), so renewalExecutor never shuts down. The thread is a daemon and there's only one guard per store, so this is basically harmless — just noting it.
…ava/io/agentscope/extensions/mongodb/store/MongoBaseStore.java Co-authored-by: Larry <139796123+larry-zy@users.noreply.github.com>
…ava/io/agentscope/extensions/mongodb/store/MongoBaseStore.java Co-authored-by: Larry <139796123+larry-zy@users.noreply.github.com>
|
All 3 suggestions applied, CI is green. Ready for your final look when you have a moment. Thanks! |
|
LGTM |
3 participants
AgentScope-Java Version
2.0.3-SNAPSHOT
Description
Background
This PR adds the
agentscope-extensions-mongodbmodule, providing a MongoDB-backed distributed storage backend for AgentScope Java.Why MongoDB
Agent 的执行信息（对话历史、思维链、工具调用记录等）通常具有以下特点：
MongoDB 的文档模型天然适合这类场景——单个 Session 对应一个 BSON Document，字段长度不受限制，Schema 灵活可变。作者在实际项目 HiveMind 中已验证了这一方案的可行性，使用 MongoDB 存储 Agent 的 Session 信息与对话历史，运行稳定。
Changes
New files — 6 source + 9 test files:
pom.xmlagentscope-core,agentscope-harness,mongodb-driver-syncMongoDistributedStore.javaDistributedStoreentry point, aggregates all sub-componentsstate/MongoAgentStateStore.javaAgentStateStoreimplementation; single-document model with CAS optimistic lockingstore/MongoBaseStore.javaBaseStoreworkspace KV implementation with namespace compound indexsandbox/MongoSandboxExecutionGuard.javafindOneAndUpdateacquisitionsnapshot/MongoSnapshotSpec.javasnapshot/MongoRemoteSnapshotClient.javaModified files — 3 POM registrations:
agentscope-extensions/pom.xml<module>agentscope-extensions-mongodb</module>agentscope-dependencies-bom/pom.xmlmongodb-driver.versionproperty +dependencyManagemententryagentscope-distribution/agentscope-bom/pom.xmlagentscope-distribution/agentscope-all/pom.xmlDesign Decisions
mongodb-driver-sync)findOneAndUpdate+_version_{key}fieldListHashUtilsampling hash detects changes;pushEachavoids full rewrites^[a-zA-Z_][a-zA-Z0-9_]*$prevents.and$in MongoDB field namesIndexOptionsConflict(error 85), drop old index, recreate with new paramsBug Fixes (discovered during testing)
开发过程中发现并修复了 5 个真实 Bug，均由合约测试覆盖：
save()不递增 versionUpdates.inc(versionField, 1L)缺失，导致乐观锁 CAS 全部失效saveIfVersion(0)异常类型错误findOneAndUpdate抛MongoCommandException而非MongoWriteException，导致并发冲突时直接崩溃saveIfVersion(UNVERSIONED)反序列化失败State是接口无法被 Jackson 实例化，改为只读取 version 字段expireAfterSeconds=0导致 session 数据 60 秒内被 MongoDB TTL 守护进程清除IndexOptionsConflict (error 85)导致应用无法启动How to Test
Testing Summary
Assumptions.abort()自动跳过MongoBaseStoreContractTest— 6 tests, 覆盖 KV 存储读写语义MongoAgentStateStoreContractTest— 6 tests, 覆盖版本控制与并发安全MongoIndexLifecycleContractTest— 7 tests, 覆盖索引参数与升级迁移Checklist
mvn spotless:applymvn test— 112 tests, 0 failures)Related Issue
Closes #2636
References
builder.mongoClient(),builder.databaseName(), etc.)详细测试报告
MongoDB Extension 测试报告与设计解读
1. 测试全景
本模块的测试分为三个层次，各层职责明确、互不重叠：
合计：112 个自动化测试，全部通过。
2. 为什么需要合约测试
2.1 项目已有的合约测试模式
AgentScope 项目对每种存储接口都定义了一套行为合约（Contract），合约测试的核心思想是：
项目中有两份权威合约：
BaseStoreContractTestagentscope-harnessInMemoryStoreAgentStateStoreVersioningContractTestagentscope-coreInMemoryAgentStateStore这些合约测试使用 Java 的 模板方法模式——基类定义测试逻辑，子类通过 override
newStore()注入不同后端。核心模块注释明确要求：2.2 为什么 MongoDB 扩展需要独立编写（而非继承）
我们的 MongoDB 合约测试没有继承基类，而是独立编写。原因是：
BaseStoreContractTest是 package-private class（没有public修饰符），位于io.agentscope.harness.agent.filesystem.remote.store包中，跨包无法继承InMemoryStore.search()使用前缀匹配（search(["a"])返回["a","b"]下的条目），但MongoBaseStore.search()使用精确命名空间匹配。直接继承会导致搜索测试失败独立编写保证了：测试逻辑与核心合约完全对齐，同时适配 MongoDB 的行为差异。
3. 合约测试设计详解
3.1 MongoBaseStoreContractTest — 6 个测试
位置：
src/test/java/.../store/MongoBaseStoreContractTest.java这 6 个测试覆盖了
BaseStore接口的全部核心语义：putGetRoundTrip_versionStartsAtOneput_incrementsVersion$inc原子操作，验证版本自增正确putIfVersion_successAndConflictputIfVersionZero_createIfAbsentMemoryConsolidator中 watermark 写入使用的模式delete_isIdempotentsearch_exactNamespaceMatch第 6 个测试（search）是专门为 MongoDB 编写的，它验证了一个重要行为差异：
这个差异不影响实际使用，因为 AgentScope 的协调命名空间（
["memory", "consolidation"]）没有子命名空间，但必须在测试中明确记录。3.2 MongoAgentStateStoreContractTest — 6 个测试
位置：
src/test/java/.../state/MongoAgentStateStoreContractTest.java这 6 个测试覆盖了
AgentStateStore的版本控制语义——这是 AgentScope 防止并发写入冲突的核心机制：supportsVersioninggetVersioned_absent_returnsVersionZerosaveIfVersion_createIfAbsentsaveIfVersion_unconditionalOverwriteplainSave_bumpsVersionUpdates.inc(versionField, 1L)在 save() 中生效concurrentWriters_onlyOneSucceeds第 6 个测试（并发写入）是最关键的，它用
CountDownLatch精确控制两个线程同时竞争：3.3 MongoIndexLifecycleContractTest — 7 个测试
位置：
src/test/java/.../MongoIndexLifecycleContractTest.java为什么需要单独的索引测试：
MongoDB 的索引参数（TTL 值、sparse、unique）不会体现在读写接口的返回值中，因此前两类合约测试完全无法感知。但索引参数错误会导致：
这类 Bug 是运维层面的致命问题，本地 E2E 测试证明了它们确实会发生，必须有专门的自动化测试覆盖。
agentStateStore_compoundIndex(user_id, session_id)复合索引存在agentStateStore_ttlIndex_30days_updated_atTTL 索引参数是 2592000 秒（30 天）agentStateStore_ttlUpgrade_fromZerobaseStore_namespaceIndexnamespace单字段索引存在baseStore_compoundIndex(namespace, key)复合索引存在sandboxGuard_ttlIndex_immediateexpiresAtTTL=0（锁立即过期）snapshotClient_ttlIndex_7dayscreatedAtTTL=7 天第 3 个测试（升级测试）是最关键的，它分三个阶段模拟真实升级场景：
4. 测试基础设施设计
4.1 MongoDB 连接与跳过机制
每个合约测试类都使用
Assumptions.abort()实现 CI 安全：为什么用
Assumptions.abort()而不是@Disabled：@DisabledAssumptions.abort()JUnit 5 的
Assumptions机制让同一个测试在不同环境下自动适配，无需维护两套测试配置。4.2 数据库隔离策略
每个测试类使用带时间戳的独立数据库名：
为什么不用固定数据库名：
test_db）：多个测试并行或测试失败后残留数据会污染后续运行@AfterAll中db.drop()清理，彻底消除数据残留4.3 测试生命周期
@BeforeEach每次创建新 store 而不是复用，是为了确保每个测试从干净状态开始，避免测试间相互影响。5. 测试过程中发现并修复的 Bug
在编写和执行合约测试的过程中，发现了 5 个真实 Bug，这些 Bug 在纯单元测试（Mockito mock）中无法暴露，只有对真实 MongoDB 执行时才会触发：
Bug 1:
save()不递增 version（P0）现象：
plainSave_bumpsVersion测试失败——期望 version=1，实际 version=0根因：
save()方法的 MongoDB update 操作只更新了数据字段和_updated_at，没有Updates.inc(versionField, 1L)影响： 不修复会导致 plain save 后的 version 始终为 0，下游的乐观锁 CAS 全部失效。
Bug 2:
saveIfVersion(0)的 DuplicateKey 异常类型错误（P0）现象：
saveIfVersion_createIfAbsent测试抛出未捕获的MongoCommandException根因：
findOneAndUpdate在 upsert=true 时遇到 DuplicateKey 冲突，抛出的是MongoCommandException（error code 11000），而不是代码中只捕获的MongoWriteException影响： 不修复会导致并发场景下
saveIfVersion(0)直接抛异常崩溃，而不是返回 UNVERSIONED 表示冲突。Bug 3:
saveIfVersion(UNVERSIONED)反序列化失败（P1）现象：
saveIfVersion_unconditionalOverwrite测试抛出JsonException: Failed to deserialize JSON to State根因：
UNVERSIONED分支调用getVersioned(... State.class)来读取写入后的 version，但State是接口，Jackson 无法实例化影响： 不修复会导致所有使用
UNVERSIONED模式的写入都抛异常。Bug 4: TTL 索引
expireAfterSeconds=0导致数据立即过期（P0 — 严重）现象： 应用运行期间 session 数据正常，重启后
agentscope_sessions文档内容为空根因：
ensureIndexes()中 TTL 索引的expireAfterSeconds设为 0，意味着_updated_at字段一过期文档就立即删除。MongoDB TTL 监控线程每 60 秒扫描一次，会删除所有_updated_at已过期的文档影响： 这是用户在实际使用中发现的 Bug——所有 session 数据在 MongoDB TTL 守护进程运行后（最多 60 秒）就会被清除，导致重启后数据丢失。
Bug 5: 索引参数变更时启动失败（P1）
现象： 修复 Bug 4 后，应用启动报
IndexOptionsConflict (error 85)根因： 数据库中已存在旧的 TTL 索引（
expireAfterSeconds=0），代码尝试用新参数（expireAfterSeconds=2592000）创建同名索引，MongoDB 拒绝创建影响： 不修复会导致从旧版本升级时应用无法启动。
Bug 修复总结
plainSave_bumpsVersionsaveIfVersion_createIfAbsentsaveIfVersion_unconditionalOverwriteagentStateStore_ttlIndex_30daysagentStateStore_ttlUpgrade_fromZero结论： 5 个 Bug 全部能被合约测试覆盖，纯单元测试（Mockito mock）无法发现任何一个。其中 Bug 1-3 由读写语义合约测试直接发现，Bug 4-5 在用户实际使用中首次暴露后，通过新增索引生命周期合约测试补充了自动化回归保护。
6. 测试执行方式
6.1 运行单元测试（CI 自动执行）
输出示例：
6.2 运行合约测试（需要本地 MongoDB）
合约测试包含在上面的命令中。如果本地没有 MongoDB，合约测试会自动跳过（输出中显示
Skipped）：如果本地有 MongoDB（
localhost:27017），19 个合约测试会正常执行。6.3 构建 + 格式检查 + 测试 一体化
7. 实际测试执行结果
7.1 执行环境
7.2 完整测试结果
7.3 合约测试详细结果
BaseStore 合约（6/6 pass）：
AgentStateStore 合约（6/6 pass）：
saveIfVersion_createIfAbsent耗时较长（291ms）是因为它涉及findOneAndUpdate的 upsert 操作，比普通读写多一次 MongoDB 内部的条件检查。7.3.3 Index Lifecycle 合约（7/7 pass）：
baseStore_namespaceIndex耗时较长（367ms）是因为首次创建MongoBaseStore实例时需要建立连接和创建索引。8. CI 策略与 PR 验收
8.1 CI 中的行为
由于项目未使用 Testcontainers，GitHub CI 无法连接 MongoDB：
合约测试的
Assumptions.abort()机制确保了 CI 不会因为 MongoDB 不可用而报错。8.2 PR 验收建议
9. 总结
本次测试工作完成了：
BaseStore和AgentStateStore的读写语义（12 个）+ 全组件索引生命周期（7 个）合约测试的价值在于：它用真实 MongoDB 驱动代码，暴露了 Mockito mock 无法发现的问题。特别是
findOneAndUpdate的异常类型差异（MongoCommandExceptionvsMongoWriteException）和$inc版本递增行为，只有在真实数据库上才能验证。