Skip to content

feat(storage): persist sandbox records in sqlite#329

Draft
kingfs wants to merge 4 commits into
chaitin:mainfrom
kingfs:fix/sandboxes-table
Draft

feat(storage): persist sandbox records in sqlite#329
kingfs wants to merge 4 commits into
chaitin:mainfrom
kingfs:fix/sandboxes-table

Conversation

@kingfs

@kingfs kingfs commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add a SQLite sandboxes table and indexes for persisted sandbox summary records.
  • Record newly created sandboxes and synchronize subsequent status, cell count, and event count updates.
  • Backfill existing filesystem-backed sandboxes into the table during daemon startup using idempotent upserts.
  • Keep sandbox directories and metadata.json as the current source of truth; ListSandboxes is not switched to SQLite in this change.
  • Wire the existing config store into the sandbox store through a focused recording interface.

Testing

  • go test ./pkg/storage/configstore ./pkg/storage/sessionstore ./pkg/agentcompose/app
  • go test -race ./pkg/storage/configstore ./pkg/storage/sessionstore
  • task lint
  • task build
  • task build:agent-compose
  • task test
  • Manual Docker scheduler validation:
    • Started the daemon on 127.0.0.1:17410 with an isolated data root.
    • Applied agents/docker-scheduler-exec-logs.
    • Executed scheduler runs that created, used, and stopped new sandboxes.
    • Verified sandboxes rows matched metadata.json for ID, driver, status, image, runtime reference, cell count, event count, and timestamps.
    • Restarted the daemon and verified existing filesystem records were idempotently backfilled.
    • Confirmed three database rows matched three sandbox metadata files.

Checklist

  • Documentation updated when behavior or configuration changed. No public behavior or configuration changed; no documentation update was required.
  • Tests added or updated for user-visible behavior.
  • No secrets, private endpoints, internal certificates, or local runtime state included.

@monkeyscan

monkeyscan Bot commented Jul 17, 2026

Copy link
Copy Markdown

PR Title: feat(storage): persist sandbox records in sqlite

Commit: f9b39fa

本次PR为沙盒记录引入SQLite持久化层。主要变更:1) 新增 configstore/sandbox_store.go 定义沙盒摘要的表结构、schema初始化与Upsert逻辑;2) 在 core_store.gosandboxStore 嵌入 ConfigStore;3) sessionstore/store.go 引入 SandboxRecorder 接口,在 CreateSandboxUpdateSandboxAddEvent 时回调记录,并在构造阶段调用 MigrateSandboxRecords 扫描已有沙盒元数据;4) app.go 调整DI顺序,将 ConfigStore 同时作为 SandboxRecorder 注入;5) 新增对应单元测试。整体思路是将文件系统沙盒目录与SQLite查询索引分离,属于典型的双写/迁移场景。

发现的主要问题:

  1. UpdateSandbox / AddEvent 在文件系统写入后、SQLite 写入前释放 sandbox 锁,并发场景下可能导致 DB 与文件系统状态持久不一致。
  2. SQLite UPSERT 中 created_at 可被覆盖,破坏创建时间不变性。
  3. tags_json 在 Tags 为 nil 时序列化为 "null",与数组语义及 schema 默认值不一致。
  4. MigrateSandboxRecords 单条 upsert 失败即返回 error,阻断 Store 初始化,存在可用性风险。

Comment thread pkg/storage/configstore/sandbox_store.go
Comment thread pkg/storage/configstore/sandbox_store.go
Comment thread pkg/storage/sessionstore/store.go
Comment thread pkg/storage/sessionstore/store.go
@monkeyscan

monkeyscan Bot commented Jul 17, 2026

Copy link
Copy Markdown

PR Title: feat(storage): persist sandbox records in sqlite

Commit: 2865005

本次 PR 完成了 sandbox 记录在 SQLite 中的生命周期管理。核心变更包括:

  1. sandbox_store.go:将时间戳精度从毫秒提升为纳秒;为 UPSERT 增加乐观并发控制(WHERE excluded.updated_at >= sandboxes.updated_at),防止旧更新覆盖新数据;新增 GetSandboxSummaryListSandboxSummariesDeleteSandbox;修复 Tagsniltags_json 被写入 "null" 的问题;确保 created_at 在更新时不会被覆盖。
  2. sessionstore/store.goSandboxRecorder 接口新增 DeleteSandboxRemoveSandbox 在清理文件系统后同步删除 DB 记录;UpdateSandboxUpdatedAt 赋值移入文件锁内,消除并发下的时序错乱;MigrateSandboxRecords 对单个失败降级为警告并继续,避免坏数据阻塞整体迁移。
  3. model.go:新增 SandboxSummaryListOptionsSandboxSummaryListResult 支持游标分页。
  4. 测试:补充了 stale update、created_at 不变性、RemoveSandbox 联动删除等场景。

整体评估:代码结构清晰,并发控制和不变性修复是亮点,测试覆盖较充分。但 RemoveSandbox 中 DB 删除位于文件系统删除之后,若该步失败将导致 DB 记录孤儿且无法通过重试自愈,存在数据完整性风险。

Comment thread pkg/storage/sessionstore/store.go
@monkeyscan

monkeyscan Bot commented Jul 17, 2026

Copy link
Copy Markdown

PR Title: feat(storage): persist sandbox records in sqlite

Commit: 4fe4c61

该 PR 修复了 RemoveSandbox 在文件系统目录已被删除后无法重试删除数据库记录的缺陷。

变更内容:

  • pkg/storage/sessionstore/store.go:在 RemoveSandbox 开头,当 os.Stat 返回 fs.ErrNotExist 且存在 recorder 时,直接尝试删除数据库中的 sandbox 记录。这解决了此前"先删目录、再删记录"流程中,若记录删除失败,则因目录已不存在导致无法重试的孤儿记录问题。
  • pkg/storage/sessionstore/sandbox_record_test.go:新增了 TestRemoveSandboxRetriesRecordDeletionAfterDirectoryRemoval 测试,验证了:第一次调用时目录被删除但记录删除失败;第二次重试时,即使目录已不存在,也能成功清理残留的数据库记录。

整体评估:
变更逻辑清晰,直接修复了历史遗留的孤儿记录问题。新增测试完整覆盖了修复场景,没有发现引入新的正确性、并发或数据完整性风险。代码风格与现有模式保持一致。

@kingfs
kingfs force-pushed the fix/sandboxes-table branch from 4fe4c61 to d12d2cd Compare July 17, 2026 10:12
@monkeyscan

monkeyscan Bot commented Jul 17, 2026

Copy link
Copy Markdown

MonkeyScan 检测到 rebase / force-push 等历史改写,正在重新评审当前 PR 的全部有效改动,并自动收敛已消失的历史问题;完成后会更新到本评论中,请稍后刷新查看。

@kingfs

kingfs commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@monkeyscan 扫描完了吗

@monkeyscan

monkeyscan Bot commented Jul 17, 2026

Copy link
Copy Markdown

@kingfs wrote:
@monkeyscan 扫描完了吗

无法从当前上下文确认扫描是否完成。review_finding 未提供,无法获取扫描状态。

@kingfs

kingfs commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

MonkeyScan 检测到 rebase / force-push 等历史改写,正在重新评审当前 PR 的全部有效改动,并自动收敛已消失的历史问题;完成后会更新到本评论中,请稍后刷新查看。

@monkeyscan 这个任务执行完成了吗

@monkeyscan

monkeyscan Bot commented Jul 17, 2026

Copy link
Copy Markdown

@kingfs wrote:

MonkeyScan 检测到 rebase / force-push 等历史改写,正在重新评审当前 PR 的全部有效改动,并自动收敛已消失的历史问题;完成后会更新到本评论中,请稍后刷新查看。

@monkeyscan 这个任务执行完成了吗

当前上下文无法确认该任务的执行状态,请等待 MonkeyScan 自动更新结果或查看最新评论。

@kingfs

kingfs commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@monkeyscan 扫描当前pr改动

@monkeyscan

monkeyscan Bot commented Jul 17, 2026

Copy link
Copy Markdown

PR Title: feat(storage): persist sandbox records in sqlite

Commit: d12d2cd

本次 PR 在 SQLite 中新增了 Sandbox 摘要的持久化存储(sandboxStore),并在 sessionstore 中集成了 SandboxRecorder 接口,实现了创建、更新、迁移、删除等生命周期的数据库记录同步。主要变更包括:

  1. pkg/storage/configstore/sandbox_store.go:新增 SQLite 表结构、Upsert/Get/List/Delete 操作,使用 Unix 纳秒时间戳和乐观并发控制(excluded.updated_at >= sandboxes.updated_at)。
  2. pkg/storage/sessionstore/store.go:在创建、更新、添加事件、删除等操作中调用 recorder 同步数据库;新增 MigrateSandboxRecords 在启动时迁移现有文件系统 Sandbox。
  3. pkg/model/model.go:新增 SandboxSummaryListOptions / SandboxSummaryListResult 支持游标分页。
  4. pkg/agentcompose/app/app.go:注册 SandboxRecorder 依赖注入。

整体评估:代码结构清晰,测试覆盖较全面(CRUD、分页、迁移、失败重试)。但发现一处游标分页逻辑缺陷,以及一处创建失败时可能导致孤儿 Sandbox 的问题。

Comment thread pkg/storage/configstore/sandbox_store.go
Comment thread pkg/storage/sessionstore/store.go
@kingfs
kingfs force-pushed the fix/sandboxes-table branch from d12d2cd to 4825865 Compare July 17, 2026 11:30
@kingfs

kingfs commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@monkeyscan 扫描当前pr改动

@monkeyscan

monkeyscan Bot commented Jul 17, 2026

Copy link
Copy Markdown

PR Title: feat(storage): persist sandbox records in sqlite

Commit: 4825865

本次 PR 在现有文件系统存储之外新增了 SQLite 持久化层,用于保存 Sandbox 摘要记录。核心变更包括:

  1. 新增 pkg/storage/configstore/sandbox_store.go,提供 schema 创建、upsert(带乐观时间戳冲突解决)、单条查询、游标分页列表和删除功能。
  2. 修改 pkg/storage/sessionstore/store.go,引入 SandboxRecorder 接口,在沙盒创建、更新、添加事件、删除时将状态同步到数据库,并支持启动时迁移已有沙盒。
  3. pkg/agentcompose/app/app.go 中调整依赖注入顺序,将 ConfigStore 同时注册为 SandboxRecorder
  4. 补充了完整的单元测试,包括游标校验、重试删除、迁移容错等场景。

关键设计决策:

  • 使用 ON CONFLICT ... WHERE excluded.updated_at >= sandboxes.updated_at 实现乐观并发控制,防止乱序更新覆盖新数据。
  • 游标分页基于 (updated_at DESC, id DESC),与索引一致。
  • 删除时先删目录再删记录,并支持在目录已删除后重试补删记录。

整体评价:功能实现完整,测试覆盖良好,但在文件系统锁与数据库 recorder 的协同上存在数据完整性风险,需要关注并发场景下的竞态条件。

return s.saveSandboxPreservingCounts(session)
// Assign the version timestamp while holding the filesystem lock so it
// reflects the serialization order used to persist metadata.json.
session.Summary.UpdatedAt = time.Now().UTC()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RemoveSandbox 与 UpdateSandbox/AddEvent 之间存在竞态,可产生已删除沙盒的僵尸数据库记录

UpdateSandbox 和 AddEvent 在释放 per-sandbox 文件系统锁之后才调用 recorder.UpsertSandbox;RemoveSandbox 同样先释放锁再调用 recorder.DeleteSandbox。这意味着两个并发流程可能交错执行:

  1. UpdateSandbox/AddEvent 完成磁盘写入并释放锁;
  2. RemoveSandbox 获取锁、删除目录、释放锁、删除数据库记录;
  3. UpdateSandbox/AddEvent 随后执行 recorder.UpsertSandbox,将已删除沙盒的记录重新写回数据库。

结果是数据库中存在已删除沙盒的“僵尸记录”,与文件系统状态不一致。

Problem code:

Changed code at pkg/storage/sessionstore/store.go:371-380

Recommendation:
将 recorder 操作纳入文件系统锁的保护范围内,或者在执行 recorder upsert 前检查沙盒目录是否仍然存在;对于删除操作,也可考虑在锁内设置“删除中”标记以阻止并发更新。

}
sandboxToRecord = session
return nil
}(); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddEvent 在 recorder 失败后重试可能导致事件重复追加

AddEvent 在已持有文件系统锁的闭包内先调用 s.appendEvent 将事件追加到 JSONL 文件,并更新 event count 和 metadata,随后释放锁。闭包外部再调用 recorder.UpsertSandbox。如果 recorder 失败,AddEvent 会返回错误。

调用方若基于该错误重试,会再次进入闭包、再次 appendEvent,导致同一条事件被重复追加到 JSONL,且 event count 也会重复累加。这与 CreateSandbox 对 recorder 失败仅记录日志的行为不一致。

Problem code:

Changed code at pkg/storage/sessionstore/store.go:562-566

Recommendation:
使 AddEvent 对 recorder 失败采用与 CreateSandbox 一致的降级策略(记录警告后返回成功),或在 appendEvent 层实现基于事件 ID 的幂等去重,避免重试导致重复事件。

@innomentats
innomentats marked this pull request as draft July 17, 2026 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant