test(session): add multi-backend replay consistency harness#2268
test(session): add multi-backend replay consistency harness#2268wei-yan1 wants to merge 5 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughEnglishChange overview
Compatibility and behavioral risks
Recommended validation
中文变更概览
兼容性与行为风险
建议验证步骤
WalkthroughAdds a reusable cross-backend session replay framework with deterministic operations, normalization, semantic diffing, fault injection, report generation, backend validation, integration tests, and bilingual documentation. ChangesReplay consistency framework
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
session/replaytest/compare.go (1)
123-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
breakdiverges from the other allowed-rule loops. Unlike the loops at Lines 61-67 and 202-208 (whichbreakon first match), this loop keeps iterating, so a last-matching rule'sReasonwins. Exact-match validation makes this only reachable via duplicate rules, so it's cosmetic today, but aligning the three loops avoids surprise later.中文
此处缺少
break，与其余两处允许规则循环不一致。 相较 Lines 61-67 和 202-208（首次匹配即break），此循环会继续遍历，导致最后一个匹配规则的Reason生效。由于校验禁止通配符，仅在重复规则时才可触达，目前仅属外观问题，但统一三处逻辑可避免后续踩坑。♻️ Suggested change
for _, rule := range allowed { if allowedDiffMatches(rule, diff) { diff.AllowedDiff = true diff.Explanation = rule.Reason + break } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/compare.go` around lines 123 - 128, Add a break to the allowed-rule loop containing allowedDiffMatches so it stops after the first matching rule, preserving the first match’s Reason and aligning it with the other allowed-rule loops.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@session/replaytest/compare.go`:
- Around line 123-128: Add a break to the allowed-rule loop containing
allowedDiffMatches so it stops after the first matching rule, preserving the
first match’s Reason and aligning it with the other allowed-rule loops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 145d5d87-f329-4d6c-aa6e-206351796ad4
⛔ Files ignored due to path filters (2)
docs/mkdocs/images/issue-2001-replay-acceptance.pngis excluded by!**/*.png,!**/*.pngtest/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
docs/mkdocs.ymldocs/mkdocs/en/session/replay_consistency.mddocs/mkdocs/zh/session/replay_consistency.mdsession/replaytest/cases.gosession/replaytest/compare.gosession/replaytest/core_test.gosession/replaytest/doc.gosession/replaytest/fault.gosession/replaytest/harness.gosession/replaytest/ledger.gosession/replaytest/normalize.gosession/replaytest/operation.gosession/replaytest/report.gosession/replaytest/types.gotest/go.modtest/replay_consistency_optional_test.gotest/replay_consistency_test.gotest/testdata/session_memory_summary_track_diff_report.json
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2268 +/- ##
===================================================
- Coverage 89.85102% 89.80597% -0.04506%
===================================================
Files 1149 1159 +10
Lines 203321 205640 +2319
===================================================
+ Hits 182686 184677 +1991
- Misses 12918 13100 +182
- Partials 7717 7863 +146
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
session/replaytest/operations_test.go (1)
219-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the exact stored-event count.
require.GreaterOrEqual(t, len(stored.Events), 7)is satisfied even if a fault mode silently stops appending its event (as long as ≥7 remain), so a real regression in one mode could go undetected. Since the appended sequence here is fully deterministic, asserting the exact count would tighten the guarantee.中文
require.GreaterOrEqual(t, len(stored.Events), 7)属于弱断言：即使某个 fault 模式悄悄不再追加事件（只要剩余 ≥7），断言仍会通过，可能漏掉真实回归。此处追加序列是完全确定的，建议改为断言精确数量以增强保证。As per path instructions: "Whether assertions are strong enough to ensure the intended behavior is actually verified."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/operations_test.go` around lines 219 - 223, In the session replay test’s final stored-session assertions, replace the lower-bound check on len(stored.Events) with an exact-count assertion for the deterministic sequence, while leaving the state assertion unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@session/replaytest/operations_test.go`:
- Around line 219-223: In the session replay test’s final stored-session
assertions, replace the lower-bound check on len(stored.Events) with an
exact-count assertion for the deterministic sequence, while leaving the state
assertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42b0c854-e96b-483a-952a-cd60d7bfe7bf
📒 Files selected for processing (3)
session/replaytest/compare.gosession/replaytest/core_test.gosession/replaytest/operations_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- session/replaytest/compare.go
Rememorio
left a comment
Rememorio
left a comment
There was a problem hiding this comment.
I reviewed the changed lines and left 7 inline comments below. The comments focus on issues worth addressing before merge.
Fresh review found three correctness/reliability issues in the replay framework and four important test/lifecycle gaps. The main risks are destructive report replacement, false positives for unordered duplicate memories, and capability declarations that do not actually prevent unsupported state APIs from being called. Targeted validation should cover destination-directory safety, reversed equal-memory ordering, unsupported state services, nested parallel operations, real timeouts, external-backend cleanup, and pre-normalization fault injection.
中文
我看过这次变更的相关代码，在下面留下 7 条行内评论。评论聚焦在合并前值得处理的问题。
本次审查发现 3 个 replay 框架正确性/可靠性问题，以及 4 个重要的测试与生命周期缺口。主要风险包括：报告替换路径可能产生破坏性删除、无序重复 Memory 可能误报差异，以及 capability 声明无法阻止框架调用不受支持的状态接口。建议补充针对目标目录安全、等价 Memory 逆序、状态能力缺失、嵌套并发操作、真实超时、外部后端清理和归一化前故障注入的验证。
| if err != nil { | ||
| return CaptureInput{}, fmt.Errorf("load session from %s: %w", r.Backend.Name, err) | ||
| } | ||
| appState, err := r.Backend.Session.ListAppStates(ctx, r.Backend.SessionKey.AppName) |
There was a problem hiding this comment.
P1: Honor unsupported state capabilities during capture
The default loader invokes ListAppStates and ListUserStates unconditionally. A backend that supports the event case but correctly declares app or user state unsupported will therefore abort during capture instead of producing the documented unsupported result; SessionWindowCheckpointOperation repeats the same unconditional reads.
Gate these calls on CapabilityAppState and CapabilityUserState, as the memory read already does, and leave the corresponding capture section empty for normalization. A fake service whose unsupported methods return an error would close this compatibility gap.
中文
状态捕获应遵守 unsupported capability
默认 loader 无条件调用 ListAppStates 和 ListUserStates。因此，一个能够执行 Event case、但正确声明不支持 App/User State 的后端，会在捕获阶段直接报错，而不是按设计输出 unsupported 结果；SessionWindowCheckpointOperation 也存在同样的无条件读取。
建议像 Memory 读取一样，分别根据 CapabilityAppState 和 CapabilityUserState 决定是否调用，并在不支持时为对应捕获区段保留空值。可增加一个 fake service，让不支持的方法返回错误，以覆盖这一兼容性场景。
| for i := range values { | ||
| values[i].item.Rank = -1 | ||
| } | ||
| sort.SliceStable(values, func(i, j int) bool { |
There was a problem hiding this comment.
P1: Canonicalize equal unordered memories by logical identity
MemoryOrderUnordered still retains backend input order when two records have the same semantic fingerprint and rounded score: this stable comparator returns neither record as smaller, and logical IDs are assigned only after sorting. If two backends return equal-content logical memories in opposite orders, their snapshots become [memory:a, memory:b] and [memory:b, memory:a], producing a blocking false positive even though multiplicity and contents match.
Resolve the logical identity before sorting and use it as the final deterministic tie-breaker, or compare an actual multiset. A regression test should register two equal-content/equal-score memories and normalize reversed backend result orders.
中文
无序 Memory 相等时应按逻辑身份规范排序
当两条记录的语义指纹和舍入后 score 都相同时，MemoryOrderUnordered 仍会保留后端原始顺序，因为当前稳定排序无法区分两者，而逻辑 ID 又是在排序后才写入。若两个后端以相反顺序返回内容相同但逻辑身份不同的 Memory，快照会分别成为 [memory:a, memory:b] 和 [memory:b, memory:a]，从而产生 blocking 误报。
建议在排序前解析逻辑身份，并把它作为最终确定性排序键，或者直接进行多重集合比较。回归测试应注册两条内容、score 均相同的 Memory，并让两个后端按相反顺序返回。
| } | ||
| if err := os.Rename(temporaryPath, path); err != nil { | ||
| // Windows cannot replace an existing destination with os.Rename. | ||
| if removeErr := os.Remove(path); removeErr != nil && !os.IsNotExist(removeErr) { |
There was a problem hiding this comment.
P1: Do not delete the destination after an arbitrary rename failure
os.Rename can fail for reasons other than Windows refusing to replace an existing file. This fallback removes path before establishing that the failure is the expected file-collision case. For example, if path is an empty directory, the first rename fails, os.Remove deletes the directory, and the retry replaces it with the report file; an existing valid report can likewise be lost if the second rename fails.
Only use a replacement fallback after identifying the platform-specific destination-file collision, and preserve the old destination until replacement is guaranteed. Please add a regression test showing that an existing directory or an unrelated rename error leaves the destination untouched.
中文
不要在任意 rename 失败后删除目标路径
os.Rename 失败并不只代表 Windows 无法覆盖已有文件。当前逻辑没有确认错误类型就先删除 path：如果目标恰好是空目录，第一次 rename 会失败，随后目录会被删除并被报告文件替代；如果目标是已有的有效报告，而第二次 rename 仍失败，也会丢失旧报告。
建议仅在确认属于特定平台的“目标文件已存在”冲突时执行替换逻辑，并保证替换成功前旧目标保持不变。同时增加回归测试，验证目标为已有目录或出现其他 rename 错误时不会修改目标。
| for _, operation := range o.Operations { | ||
| operation := operation | ||
| group.Add(1) | ||
| go func() { defer group.Done(); <-start; errorsCh <- operation.Execute(ctx, runtime) }() |
There was a problem hiding this comment.
P2: Validate child operations before starting parallel execution
validateReplayCase validates only top-level operations, so an exported ParallelOperation containing a nil child passes validation and panics in this goroutine when Execute is called. An empty child list also silently turns a concurrency case into a no-op.
Recursively validate parallel children, rejecting empty lists, nil entries, missing IDs, and conflicting IDs before any goroutine starts. Add malformed nested-program tests alongside the existing top-level validation cases.
中文
并发执行前应校验子 Operation
validateReplayCase 目前只校验顶层 Operation。公开的 ParallelOperation 如果包含 nil 子项，仍能通过校验，并在该 goroutine 调用 Execute 时触发 panic；空的子操作列表则会悄悄把并发 case 变成无操作。
建议在启动 goroutine 前递归校验并发子项，拒绝空列表、nil 项、缺失 ID 和冲突 ID，并在现有顶层非法程序测试旁增加嵌套场景。
| Capabilities: capabilities.Clone(), | ||
| } | ||
| cleanup := func() error { | ||
| return errors.Join(memoryService.Close(), sessionService.Close()) |
There was a problem hiding this comment.
P2: Clean persistent external-backend data, not just clients
This cleanup only closes the services. Each optional run creates a never-reused table/key prefix, configures session/app/user TTLs as zero, and writes sessions plus scoped state, so Redis keys and SQL/ClickHouse rows and tables remain after the test. Repeated opt-in or CI runs will accumulate a fresh persistent namespace every time.
Delete the case session and scoped state and drop or reuse the test tables before closing, or require and manage a disposable schema/database. Redis should also use a bounded TTL or explicit prefix cleanup. Document the cleanup contract for users supplying these DSNs.
中文
外部后端清理不能只关闭客户端
这里的 cleanup 只关闭服务。每次可选集成测试都会生成一个不再复用的新 table/key prefix，同时把 Session/App/User TTL 配置为 0，并写入会话及分层状态。因此测试结束后，Redis key 以及 SQL/ClickHouse 的数据和表都会继续存在；反复执行会不断累积新的持久命名空间。
建议在关闭服务前删除 case 会话和分层状态，并删除或复用测试表；也可以明确要求并管理一次性 schema/database。Redis 还应设置有限 TTL 或显式清理整个 prefix，并在文档中说明调用方提供 DSN 时的清理契约。
| cases := replaytest.PublicCases() | ||
| require.GreaterOrEqual(t, len(cases), 10) | ||
| report, err := replaytest.RunSuite( | ||
| context.Background(), |
There was a problem hiding this comment.
P2: Enforce the advertised replay deadline with context cancellation
The 30-second assertion runs only after RunSuite returns, while the suite receives context.Background(). A blocked session, memory, summary, or database call can therefore hang CI indefinitely and never reach the asserted upper bound; the optional two-minute check has the same issue.
Create a context with the advertised deadline and pass it to RunSuite in both tests. The resulting cancellation error should also be asserted so a timeout is reported as a bounded failure rather than a stuck job.
中文
通过 Context 真正执行 replay 超时限制
当前 30 秒断言只有在 RunSuite 返回后才会执行，而 suite 使用的是 context.Background()。如果 Session、Memory、Summary 或数据库调用卡住，CI 会无限等待，根本到不了耗时断言；可选后端的两分钟检查也有同样问题。
建议为两组测试创建带对应 deadline 的 Context 并传给 RunSuite，同时校验取消后的错误，使超时表现为有界失败，而不是卡死任务。
|
|
||
| baseline := result.Traces["inmemory"] | ||
| source := result.Traces["sqlite"] | ||
| faulty, err := fault.Inject(source) |
There was a problem hiding this comment.
P2: Inject required faults before capture and normalization
All 15 acceptance faults mutate a Trace after the backend state has already passed through Runtime.Capture and Normalizer. The metric therefore proves that CompareTraces notices edits to its own normalized DTO, but a regression that erases or aliases the backend discrepancy during capture still reports 15/15. For example, summary_wrong_session writes SummarySnapshot.SessionID directly even though normalization synthesizes that field from the enclosing session.
Keep these mutations as comparator tests, but exercise the required summary ownership/filter and boundary faults in a raw CaptureInput or service fake before calling Runtime.Capture. Assert that the resulting normalized traces still contain a blocking, precisely located diff.
中文
必要故障应在捕获和归一化之前注入
这 15 个验收故障都是在后端状态已经经过 Runtime.Capture 和 Normalizer 后，直接修改 Trace。因此该指标只能证明 CompareTraces 能发现对归一化 DTO 的修改；如果捕获或归一化阶段错误地丢弃、覆盖了后端差异，测试仍会得到 15/15。例如 summary_wrong_session 直接修改 SummarySnapshot.SessionID，但真实归一化逻辑会从外层 Session 合成这个字段。
可以保留这些 mutation 作为比较器单元测试，但必要的 Summary 归属、filter 和 boundary 故障应通过原始 CaptureInput 或 service fake 在 Runtime.Capture 前注入，并断言归一化后的 trace 仍产生可精确定位的 blocking diff。
2 participants
What changed
session/replaytest.allowed_diffhandling and explicit unsupported capability reporting.test/testdata/session_memory_summary_track_diff_report.json.Why
Session and Memory backends may produce different replay-visible behavior for the same agent trajectory. These differences can involve event ordering, state transitions, memory contents, Summary ownership, truncation boundaries, Track data, retries, or duplicate writes.
Without a reusable replay harness, backend migrations and storage changes can introduce subtle inconsistencies that are difficult to detect with isolated unit tests.
This change provides a deterministic cross-backend validation layer without modifying existing Session or Memory runtime APIs. It distinguishes meaningful mismatches from backend-generated noise and reports unsupported capabilities explicitly instead of silently ignoring them.
Fixes #2001
Testing
go build ./...go vet ./session/replaytestcd test && go vet .golangci-lint run --timeout=10m ./session/replaytest/...cd test && golangci-lint run --timeout=10m replay_consistency_test.go replay_consistency_optional_test.gogo test ./session/replaytest -count=20go test -race ./session/replaytest -count=1cd test && go test . -run ReplayConsistency -count=10cd test && go test -race . -run ReplayConsistency -count=1cd test && go test ./... -count=1cd session/sqlite && go test ./... -count=1cd memory/sqlite && go test ./... -count=1Acceptance results:
The repository-wide root test suite remains environment-sensitive on Windows because of unrelated existing tests requiring
sh, WSL commands, and strict timestamp-resolution behavior. The affected replay, Session SQLite, Memory SQLite, andtestmodules pass independently.Notes for reviewers
allowed_diffand unsupported-capability semantics.github.com/mattn/go-sqlite3.Acceptance test screenshot
The screenshot below shows the local InMemory/SQLite acceptance run, fault detection, race detection, 20-run stability check, build verification, and golden report hash.