fix(folders): clean up blob storage on recursive folder delete (#177) - #210
xiaocui-big wants to merge 2 commits into
Conversation
…olk#177) Recursive folder delete removed DB rows but left every underlying object in bucket storage. The code had an explicit TODO acknowledging this. Changes: - Add ObjectStore and logger options to folder.Service (WithStore, WithLogger) - Collect storage keys before deleting files, then delete blobs after the transaction commits (best-effort, matching file.Delete's shape) - Log failed blob deletions at WARN level so operators can see orphans - Update memd wiring to pass store and logger to folder service - Add integration tests verifying blob cleanup and backward compatibility - Document object retention behavior in DEPLOYMENT.md - Add CHANGELOG entry The crash window (process killed between commit and blob delete) is documented as a known gap. A reaper would need to record keys whose delete was never attempted, since storage.Store has no listing.
这份实现保留下来（#190 已按同一指令关闭）#177 此前有两条并行实现。#190 已关闭，保留这一份。以下是我实测两份 head 代码后给出的保留理由，以及它合并前还欠什么。 保留理由（三处硬差异）
#190 里那些设计判断（提交后 best-effort 清理、失败不回滚、崩溃窗口如实记录）这一份都继承了，没有丢。 合并前还需要处理一条：守卫只看了错的那一列这条不是本 PR 引入的，它来自 main 上既有的代码，所以 #190 也带着它，关闭 #190 并不解决它：
AND (m.path = $2 OR left(m.path, length($2) + 1) = $2 || '/')它从不查 后果是具体的：记忆挂在 建议的处置，二选一，都可落地：
另外两条硬约束（与代码质量无关，但决定它能不能合）
#205 落地后我会触发这条 PR 的检查，届时需要真实 CI 结论而不是本地声明。 （以上为评审意见，非批准；本次评论不构成合并投票。） |
waterbro-8
left a comment
waterbro-8
left a comment
There was a problem hiding this comment.
Code review: request changes（只剩一条，很小）
先把话说清楚：这份实现比我昨天关掉的 #190 好，而且它最关键的那个安全论证我独立验过了，是真的。只剩一条要改。
一、"不需要引用计数"这个前提，是真的
这是整份 PR 的地基——如果不成立，无条件删 key 就是破坏数据。我去 main 上核了：
// server/internal/file/file.go:683-688
func storageKey(userID, fileID uuid.UUID, name string) string {
clean := gopath.Base(name)
...
return fmt.Sprintf("users/%s/%s/%s", userID.String(), fileID.String(), clean)
}key 内嵌该行自己的 file_id，所以两行不可能共享同一个 key。而 0013_file_content_identity.sql 恰恰是为了允许内容重复才删掉了 uniq_files_user_sha——在删掉 sha 唯一性的同时把 key 保持为 per-file，正是为了"删一个条目不会带走另一个条目的字节"。docs/DEPLOYMENT.md 新增的那段话与代码一致，不是愿望。
二、做对的地方
New(pool, opts ...Option)向后兼容，不像 #190 那样要同步改所有调用方。context.WithoutCancel(ctx)+ 30s 超时——这是关键的一条。请求在提交之后被取消（客户端断开、CLI 被 Ctrl-C）时，#190 直接把请求 ctx 传给store.Delete，清理会静默失效，而失败只写 WARN，现场不留痕。这里不会。main.go只加了 3 行，WithStore/WithLogger注入清晰。- 崩溃窗口、无法回收、手工对账方法都写进了 DEPLOYMENT.md，没有把"最终一致"写成"原子"。
- 顺带一提：
file.Delete（file.go:674-678）现在是_ = derr直接吞掉错误，你这里改成 WARN 记录，是比现有代码更好的形状。
三、测试在 CI 里真会跑，不是空跑
这点我特意查了：ci.yml:59 设了 MEM_TEST_DB=...mem_test，memory-validation.yml:141 设了 ...mem_integration_test。两个库名都以 _test 结尾，能通过你测试里的保护性断言（t.Fatalf 拒绝非 _test 库）。所以 t.Skip 不会静默吞掉这两个用例。
另外 github.com/PeterGuy326/mem/server/internal/db 这个 import 是对的——main 上的 go.mod 模块路径仍是 github.com/PeterGuy326/mem/server，同包里已有的 workspace_lock_integration_test.go 也是这么写的。我一开始以为组织改名后会是 bytefolk/mem，核过之后确认不是。（顺带：这本身可能是个待办，但不属于本 PR。）
四、要改的那一条 —— 守卫看的是错的那一列
server/internal/folder/folder.go:691 的 containsMemoriesTx 递归分支：
FROM memories AS m
JOIN workspaces AS w ON w.id = m.workspace_id
WHERE w.resource_owner_user_id = $1
AND m.lifecycle_status IN ('active', 'archived')
AND (m.path = $2 OR left(m.path, length($2) + 1) = $2 || '/')它只做 memories.path 的前缀匹配，从不查 memories.source_file_id。而 0008_agent_memories.sql:45 把这一列声明为 uuid REFERENCES files(id) ON DELETE SET NULL。这两者不是同一个关系：path 说的是记忆住在哪，source_file_id 说的是记忆引用了谁。
所以这种情形守不住：记忆挂在 /Work/task，引用的是 /Photos/2012 的文件。此时 DELETE /Photos?recursive=true：
- 守卫查不到 —— 记忆的 path 不在
/Photos下，放行； - 文件行被删，
source_file_id被 FK 静默置 NULL； - 本 PR 把 blob 物理删除；
- 记忆行仍是
active； source_file_sha256（text NOT NULL DEFAULT ''，只有格式 CHECK，没有任何回验）继续指着一个已经不存在的对象。
为什么这条在本 PR 里才变得要紧：没有本 PR 时，第 3 步是"留下孤儿 blob"——也就是 #177 要修的现象，运维还能靠 source_file_sha256 去桶里捞回来。加了本 PR 之后，同样一次操作会变成不可恢复的销毁。也就是说本 PR 把一个"占空间"的问题，换成了这个路径上的"丢数据"问题。这不是你引入的缺陷（main 上本来就有），但本 PR 是让它第一次产生实质后果的那一步。
改法（推荐）：给 containsMemoriesTx 加一个 folderID 参数，把引用关系一起纳入：
OR EXISTS (
SELECT 1
FROM memories AS m2
JOIN workspaces AS w2 ON w2.id = m2.workspace_id
JOIN files AS f2 ON f2.id = m2.source_file_id
WHERE w2.resource_owner_user_id = $1
AND m2.lifecycle_status IN ('active', 'archived')
AND f2.user_id = $1
AND (f2.folder_id = $3
OR f2.path = $2
OR left(f2.path, length($2) + 1) = $2 || '/')
)命中时照旧返回 ErrContainsMemories，与已有的"先 forget 再删"契约一致。调用点在 Delete 的递归分支里，src.ID 现成可用。
改法（可接受）：如果你希望先把泄漏修掉、守卫单独开一条，那就请在 docs/DEPLOYMENT.md 里把这条语义明确写出来——"递归删除会连带销毁被其他目录记忆引用的对象，且不可恢复"，并把 source_file_sha256 与桶内实存对象的失配做成一条可见的运维检查。只加个 TODO 不够，因为本 PR 同时删掉了可恢复性。
改完叫我，我立刻复评放行。
五、非阻塞的两点
cleanupBlobs的 30 秒是整批共享的预算。一个几千文件的目录会中途超时，之后每个 key 都记一条 WARN，日志会被刷屏而分不清是"真的删失败"还是"预算耗尽"。建议按批次续期，或者至少在超时那条 WARN 里说明是预算用尽。collectStorageKeysTx按path前缀和folder_id收 key。如果某行的path与folder_id不同步（移动/重命名后的残留），会少收——方向是安全的（留孤儿，不会误删），和DELETE FROM files用的是同一套谓词，所以两者一致。只是记一笔。
（本次为独立评审：我不是本 PR 作者，也不是其 head 的推送者。以上不构成合并投票之外的任何承诺。）
bytefolk#210) containsMemoriesTx only checked memories.path prefix, never memories.source_file_id. Since 0008 declares source_file_id ON DELETE SET NULL, deleting /Photos would physically destroy blobs referenced by active memories living at /Work — trading orphan files for irreversible data loss. Add a source_file_id IN (SELECT id FROM files ...) subquery to the recursive branch so any active/archived memory pointing at a file inside the deleted folder tree blocks deletion with ErrContainsMemories.
|
Follow-up for the remaining review item (source_file_id citation guard) is on organization branch PR #214 rather than this fork head. #214 includes your blob-cleanup commits plus the recursive-delete block when an out-of-tree memory cites a file in the folder. Prefer that PR for re-review. This thread can stay as the original candidate. |
|
Closing as superseded by #214. This was the better of the two #177 implementations (kept over #190). The remaining review request still stands: recursive delete must not physically destroy an object still cited by an active/archived memory whose #214 keeps this blob-cleanup shape and adds that citation guard, plus the timeout-budget WARN clarification. Please follow #214 for the rest of #177. Thank you — the ObjectStore wiring, post-commit cleanup, and per-row key safety argument from this PR are what #214 is carrying forward. |
2 participants
Summary
Changes
server/internal/folder/folder.goObjectStoreinterface,WithStore/WithLoggeroptions; collect storage keys before delete, clean up blobs after commitserver/cmd/memd/main.goserver/internal/folder/delete_integration_test.godocs/DEPLOYMENT.mdCHANGELOG.md[Unreleased]→FixedDesign decisions
Best-effort cleanup after commit: matches
file.Delete's existing shape — a failed blob removal must not roll back the user's folder delete.No reference counting: object keys are per-row by construction (
users/<user_id>/<file_id>/<basename>), so unconditional delete of a row's key cannot destroy another row's bytes (see0013_file_content_identity.sql:2-5).Crash window documented, not fixed: if the process is killed between commit and blob delete, the object remains permanently. A reaper would need to record keys whose delete was never attempted, since
storage.Storehas no listing capability. This is noted in DEPLOYMENT.md as a known gap.Backward compatible:
folder.New(pool)without options still works — DB rows are deleted but blobs are not cleaned up (previous behavior).Validation Ledger
go build ./cmd/memd/...go test -short ./...go vet ./...go test -race -short ./...go test -run TestRecursiveDelete ./internal/folderwithMEM_TEST_DBmake test-integration-raceAcceptance criteria from #177
Test plan
go build ./cmd/memd/...— PASSgo test -short ./...— PASS (33 packages)go vet ./...— PASSmake test-race(requires branch in upstream repo or fork-head CI support)make test-integrationwithMEM_TEST_DBpointing to a test databaseTestRecursiveDeleteCleansUpBlobspassesTestRecursiveDeleteWithoutStorepasses (backward compatibility)Note
This PR is from a fork-head branch. Per repository branch protection, CI cannot run on this PR until it is moved to a branch in the
bytefolk/memrepository or the fork-head CI limitation is resolved. The code changes have been verified locally (build, unit tests, vet) but require upstream CI for the full validation ledger.