fix(folder): compile recursive blob cleanup - #190
PeterGuy326 wants to merge 5 commits into
Conversation
Independent automated preflight — PASS, bounded #177 cleanup scopeCandidate: Independently inspected the production wiring and transaction boundary: storage keys come from the owner-scoped Independently inspected the new live gate and test: Rechecked The shared Web lockfile is identical to #192. This PR is still based on #192: land that baseline through its independent review gate first, then retarget to main and verify the final candidate. No approval bypass, production deletion, or release is claimed. |
Review summary (verified independently)
Gap found: this test requires Same base/retarget blocker as #189 — this PR bases off #192's branch, not |
Review follow-up — correcting the MinIO evidence@Bindy-lbb The base/ancestry concern is confirmed; this PR is now Draft until #192 lands in main and the retarget/integration/fresh-review sequence is completed. There is one CI distinction to correct: the ordinary Go unit job in I directly checked job 102747493733, run The structured log events above are condensed to their relevant fields. The old PR-body V5 was a historical local skip, not a description of this current-head lifecycle run; the body now separates these facts and links the job. Thus “always skipped in CI” does not apply to this head. A separate automated checker also extracted the actual shell/JSON gate from script blob Author-posted preflight comments are supplemental automated evidence only, never a substitute for a non-author APPROVE. Contributor credit for @sun-970 / Li Yuanyang remains in the body. |
|
Thanks for the detailed PR description and thorough validation ledger. Here's my review: Overall: The approach is sound — collecting Issues found:
Minor nits:
|
waterbro-8
left a comment
waterbro-8
left a comment
There was a problem hiding this comment.
REQUEST-CHANGES — the guard looks at the wrong column, and this PR makes the miss unrecoverable
First, what is clearly good and I want on the record: replacing the shared p + ".tmp" staging
path with os.CreateTemp fixes a real collision bug; rows.Err() and rows.Close() are both
handled; a nil store is a clean way to keep unit tests out of object storage; and the doc
comment honestly states the crash window instead of claiming atomicity.
S2-1 — containsMemoriesTx checks where memories live, not what they cite
server/internal/folder/folder.go:691 — the only thing standing between
DELETE ?recursive=true and physical destruction is:
AND (m.path = $2 OR left(m.path, length($2) + 1) = $2 || '/')That is a prefix match on memories.path. It never consults memories.source_file_id, which
is declared in 0008_agent_memories.sql:45 as uuid REFERENCES files(id) ON DELETE SET NULL.
Those two things are not the same relation: a memory at /Work/task can cite a file at
/Photos/2012, and the guard will not see it.
So for that file: the row is deleted (FK silently NULLs source_file_id), the blob is destroyed,
the memory row survives as active, and source_file_sha256 — text NOT NULL DEFAULT '' with
only a format CHECK, no FK, nothing that re-validates it — keeps naming an object that no
longer exists.
I checked whether this predates your PR, and partly it does. On main,
server/internal/file/file.go:675 already destroys the blob on single-file delete, with no
provenance guard at all and the error swallowed silently at :676-680 (_ = derr, not even a
log). I am not asking you to fix file.Delete here. What this PR changes is the blast radius
and the recoverability: ?recursive=true takes a whole subtree in one call, and where main's
DB-rows-only behaviour left the bytes in the bucket so an operator could still prove or rebuild
the link, after this PR there is no recovery path. Precedent is not a defence when the
operation is the bulk form of it.
Any one of these resolves it:
- make the guard referential, or
- restrict cleanup to unreferenced files, e.g. in the
DELETE … RETURNING storage_keyadd
AND NOT EXISTS (SELECT 1 FROM memories m WHERE m.source_file_id = f.id AND m.lifecycle_status IN ('active','archived')), or - record explicitly in the changelog that provenance loss on cross-path citations is accepted.
S2-2 — the DEPLOYMENT.md justification is not supported by the schema
New text: orphans are "unreferenced — no live database row points at them — and are safe to
leave in place." After the FK NULLs the pointer, that is literally true of the pointer columns
and false of the situation: the live memory row still carries source_file_sha256, which is
exactly what cmd/mem/cmds_memory.go:476-480 prints to users as provenance. Please reword to
say what survives and what is destroyed — right now the doc launders an unstated data-loss
tradeoff into a reassurance.
S2-3 — post-commit cleanup runs on the request context
if txErr != nil { return txErr }
if s.store != nil {
for _, key := range orphanKeys {
if derr := s.store.Delete(ctx, key); derr != nil {
s.log.Warn("folder.blob_delete_failed", …)ctx here is the handler's context, and the DB transaction has already committed. A client
disconnect or an HTTP server timeout mid-delete fails every remaining key with
context canceled, each producing one Warn, with no counter, no summary, and nothing that
would retry — so the failure mode is a silent, unbounded pile of orphaned blobs precisely in
the case where there are the most of them. Subtree deletes are the slow ones, which is the
worst pairing. You already log a Warn (better than file.go, which logs nothing); what's
missing is a detached context and an aggregate. There is an in-repo precedent for the
detachment: indexgeneration/store.go uses defer tx.Rollback(context.WithoutCancel(ctx)).
S2-4 — no test observes the guard/cleanup interaction
TestRecursiveDeleteCleansBlobs creates zero memory rows, so it exercises the happy path
only. The one test that does create memories, TestMemoryPathLifecycleIntegration
(folder_test.go:228), is switched to New(pool, nil, nil) — store == nil, so cleanup is
skipped there by construction. Net effect: no test in the PR would fail if containsMemoriesTx
were deleted outright.
Please add the case the design actually needs: an active memory whose path is outside the
deleted subtree but whose source_file_id points at a file inside it — assert whatever you
decide S2-1 resolves to, so the invariant is pinned either way. Also worth checking as negative
controls: with AND user_id = $1 dropped from the DELETE, and with the
left(path, length($3) + 1) = $3 || '/' arm dropped, does anything fail? I did not run them;
if not, the query's two most safety-relevant predicates are unasserted.
Not blocking, for context
Your acceptance wiring does land: TestRecursiveDeleteCleansBlobs runs under
memory-validation.yml:191 → scripts/acceptance_agent_memory.sh, which exports
MEM_TEST_S3_ENDPOINT itself, and it is green at a9f7238e8 (6 runs, 0 failures). Heads-up
that any rebase onto current main will turn that job red for a reason that is not yours:
pull access denied for minio/minio, same as #198/#199/#203/#205. Also
mergeable_state=behind.
Provenance
Static read of head a9f7238e8 diff plus folder.go, file.go, 0008_agent_memories.sql and
cmds_memory.go on main 3c13f04. I ran no Go tests and no MinIO/PostgreSQL instance, so
the negative-control questions above are questions, not findings. If you disagree with my read
of S2-1 I would want the reachable-scenario check to come from a test rather than from me.
Review: the guard checks the wrong column, and this PR makes the miss unrecoverableFirst, what is right here, because it is most of the PR. The problem is one query that this PR did not touch, and the way this PR changes its blast radius. The guard asks where a memory lives, not what it cites
SELECT EXISTS (
SELECT 1
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 || '/')
)
source_file_id uuid REFERENCES files(id) ON DELETE SET NULL,So the sequence is:
The comment right above the guard states the intent: // A folder operation must never become an implicit memory
// deletion. The caller has to use the memory lifecycle's
// explicit forget operation first.The guard does not enforce that intent. It enforces "no memory is stored in this subtree", which is a different and much weaker claim. Why this PR is the reason it needs fixing nowBefore this change, recursive delete removed DB rows and left the objects in the bucket. That was wrong, but it was recoverable: an operator could reconcile orphan objects against After this change the objects are actually deleted. So this PR converts a silent provenance loss into an irreversible silent provenance loss. That is the specific reason I am blocking on this line rather than filing it as a follow-up. What I am asking forExtend the recursive check to also cover citation, e.g.: SELECT EXISTS (
SELECT 1
FROM memories AS m
JOIN workspaces AS w ON w.id = m.workspace_id
JOIN files AS f ON f.id = m.source_file_id
WHERE w.resource_owner_user_id = $1
AND m.lifecycle_status IN ('active', 'archived')
AND (f.folder_id = $2 OR f.path = $3
OR left(f.path, length($3) + 1) = $3 || '/')
)with And a test that fails without the fix: put a file in the deleted subtree, put a memory outside that subtree whose Two smaller things
|
关闭：两份并行实现收敛到 #210按清理指令处理 #177 的重复实现：这一条关闭，#210 保留。先把你这边做对的地方记在案上，再讲为什么收敛到 #210。 #190 里值得保留的判断
这些设计判断 #210 全部继承了（ 为什么保留 #210 而不是 #190实测两份 head 的代码，三处硬差异：
需要说明的是：我没能把两份都跑起来对比（本机跑不了 CI 那条 关闭 #190 不撤掉我在这条 PR 上给的评审意见
后果：记忆挂在 B 目录、引用的是 A 目录的文件时，递归删 A 会把 blob 物理删除、 这一条我已经挂在 #210 上作为合并前事项，不会随本 PR 一起消失。 顺带记录两件与代码质量无关、但当时确实挡着这台车的事
（本次关闭不构成代码评审投票，不代表合并或验收任何东西。） |
4 participants
Requirement and behavior
Refs #177. This canonical PR carries the recursive object-cleanup implementation from #186 and corrects its three invalid assignments from pgx.Rows.Close(), which returns no value.
Recursive folder deletion obtains owner-scoped storage keys from DELETE ... RETURNING inside the database transaction, removes objects only after commit, and logs the key when object deletion fails. Explicit memory-forget and task-state guards remain in place. The documented best-effort crash window remains accepted; this is not guaranteed physical erasure or a crash-recovery queue.
Reviewed follow-up:
a9f7238e8fcfbb031c84912a2923a6e6af0a6578, parente76d4b03a4d3681db868bcd2587e54a56cbceda1. It consolidates the duplicate Unreleased Fixed subsection, adds a bounded duplicate-subsection regression to the existing release guard, and restores the memory-lifecycle, folder_id SET NULL and subfolder ON DELETE CASCADE rationale. Production code differs from the parent only in comments; queries, API, dependencies and the lifecycle CI implementation are unchanged.Validation ledger
ERROR: duplicate changelog subsection at line 52: ### Fixed; corrected candidate preserves every bullet and one Fixed subsectiongit diff --check HEAD^ HEADThe new changelog check permits the same subsection in different releases and rejects repeated subsections within one release, including duplicates separated by another category. Sequential changelog integration with the other feature PRs remains a coordinator-owned step; no other feature's entry is copied into this PR.
Reproduce the complete release guards
From this candidate's repository root, with a Linux Docker engine able to bind-mount the checkout:
docker run --rm --network none --read-only \ --tmpfs /tmp:rw,exec,size=128m \ --volume "$PWD:/src:ro" \ debian@sha256:88200866dfff7ea7f5cbcb6ec7c8a701889efe6fe859fe64d6990e4b07ea4171 \ bash /src/scripts/test_release_guards.shExpected and observed: exit 0 and
PASS: release source, notes, asset-set and checksum guards fail closed.No extra environment variables, real Git refs or Git-directory mount are required. The script creates its own temporary fake git and supplies its PATH/FAKE_* environment for Git-object guard cases. The tmpfs must allow execution: a default noexec tmpfs prevented that fixture from executing in the first independent attempt and surfaced as a misleading missing-tag error. With exec enabled the unchanged script passed. The earlier native macOS attempt also stopped at BSD find's unsupported -printf; the pinned Linux image provides the expected GNU utilities. Containers and their temporary files are removed on exit. No newly built macOS Go executable was run.
Real object-store evidence boundary
HTTP, CLI and MCP lifecycle job 102807313835 belongs to successful run 34457532100 on exact published head
a9f7238e8fcfbb031c84912a2923a6e6af0a6578. It starts real isolated PostgreSQL and MinIO, then emits named JSON RUN and PASS (0.04s), with no skip, for TestRecursiveDeleteCleansBlobs, followed by lifecycle PASS. The earlier job 102747493733 remains historical evidence for parente76d4b03a4d3681db868bcd2587e54a56cbceda1only.This proof comes from the required HTTP, CLI and MCP lifecycle job in memory-validation.yml, not the PostgreSQL integration job or the ordinary ci.yml Go unit job. The latter may skip the S3 test when its endpoint is absent. scripts/acceptance_agent_memory.sh lines 894–907 invoke the exact test with go test -json and configured PostgreSQL/MinIO values; the following jq predicate requires Action=pass for that exact name, rejecting skips, while pipefail preserves producer errors.
On a Linux host with Go, Docker Compose, curl and jq,
./scripts/acceptance_agent_memory.shreproduces the real-service scenario and owns its disposable _test database, MinIO stack and cleanup. The historical local skipped invocation remains NOT VERIFIED and is not counted as this CI result. The earlier independent 16-case synthetic gate verification is supplemental shell/JSON evidence, not another real-storage run.Integration and attribution
The PR remains Draft against main. #192 has merged as
87db0dfe0507be2190fe2fdcce0e267be8224f4d; this exact follow-up does not merge that main commit into feature history. The coordinator retains final integration, ancestry and merge decisions. Automated source review and test replay are not a formal human APPROVE.Original implementation: #186 by @sun-970 (Li Yuanyang; commits attributed to
liyuanyang). This PR retains that contribution with maintainer compile corrections and bounded follow-up cleanup. Contributor credit must be retained in any eventual squash commit; no history or approval is rewritten here.Risk and rollback
The follow-up does not change recursive deletion behavior. Reverting it restores the prior changelog layout, comments and guard coverage. The underlying object cleanup remains best-effort after commit: failure does not undo the database deletion, and a process crash can leave objects requiring operator follow-up, as documented in docs/DEPLOYMENT.md. No production erasure or issue-closure claim is made.