Rm and RemoveAllWithPrefix leave revision files behind and take no file lock (erasure gap) #30

New issue

Open

opened 2026-10-03 05:04:30 +00:00 by thklein · 1 comment

thklein commented

2026-10-03 05:04:30 +00:00

Owner

Problem

Deleting a whole workspace, or a prefix inside one, does not reliably delete the file revisions stored for it:

Rm (workspace delete): every provider removes the revision store best effort, with the error discarded:
- pkg/client/s3.go:71: _ = newS.revisionsProvider.RemoveAllWithPrefix(ctx, s.dir)
- pkg/client/directory.go:91, same _ =
- pkg/client/azure.go:125, same _ =
A failure (throttling, a partial DeleteObjects, a network error) leaves every old revision of every file behind, and Rm still reports success. Also check the S3 prefix argument: it passes the factory's s.dir, not the workspace's dir.
RemoveAllWithPrefix(id, prefix) with a non-empty prefix deletes only the content objects under the prefix. The revisions of those files stay in revisions/<dir>/… for good. Compare DeleteFile, which removes a file's revisions too.
Neither path takes the per-file lock that #28 introduced (fileLocks). A concurrent revisioned write to a file under the prefix can recreate a revision or its PendingID marker after the delete has listed the prefix, which leaves an orphan.

Why it matters

This is a data-retention / GDPR Art. 17 erasure gap. Revisions are full prior copies of user files. When a user or project is deleted, bpai deletes the workspace and expects the data gone. On S3, every SaaS tenant, the old versions of every document can survive in the bucket with nothing pointing at them, so no later sweep finds them either.

Fix

Rm: return the revision-store error (or retry, then return it) instead of discarding it. Also verify the S3 prefix.
RemoveAllWithPrefix: also remove revisions/…/<prefix>/.
Take the file locks, or a workspace-level lock, around both.
A one-off sweep for orphaned revisions/<dir>/ prefixes whose workspace no longer exists, run on the SaaS bucket(s).
Tests: delete a workspace or prefix with revisioned files, then assert that no revision objects remain.

Where it was found

Review of #27 / #28. Related to basepeak-ai/bpai#1220. Compare the earlier GDPR erasure sweep in bpai.

## Problem Deleting a whole workspace, or a prefix inside one, does not reliably delete the file revisions stored for it: - **`Rm` (workspace delete):** every provider removes the revision store **best effort, with the error discarded**: - `pkg/client/s3.go:71`: `_ = newS.revisionsProvider.RemoveAllWithPrefix(ctx, s.dir)` - `pkg/client/directory.go:91`, same `_ =` - `pkg/client/azure.go:125`, same `_ =` A failure (throttling, a partial `DeleteObjects`, a network error) leaves every old revision of every file behind, and `Rm` still reports success. Also check the S3 prefix argument: it passes the factory's `s.dir`, not the workspace's `dir`. - **`RemoveAllWithPrefix(id, prefix)`** with a non-empty prefix deletes only the content objects under the prefix. The revisions of those files stay in `revisions/<dir>/…` for good. Compare `DeleteFile`, which removes a file's revisions too. - **Neither path takes the per-file lock** that #28 introduced (`fileLocks`). A concurrent revisioned write to a file under the prefix can recreate a revision or its `PendingID` marker after the delete has listed the prefix, which leaves an orphan. ## Why it matters This is a **data-retention / GDPR Art. 17 erasure gap**. Revisions are full prior copies of user files. When a user or project is deleted, bpai deletes the workspace and expects the data gone. On S3, every SaaS tenant, the old versions of every document can survive in the bucket with nothing pointing at them, so no later sweep finds them either. ## Fix - `Rm`: return the revision-store error (or retry, then return it) instead of discarding it. Also verify the S3 prefix. - `RemoveAllWithPrefix`: also remove `revisions/…/<prefix>/`. - Take the file locks, or a workspace-level lock, around both. - A one-off sweep for orphaned `revisions/<dir>/` prefixes whose workspace no longer exists, run on the SaaS bucket(s). - Tests: delete a workspace or prefix with revisioned files, then assert that no revision objects remain. ## Where it was found Review of #27 / #28. Related to basepeak-ai/bpai#1220. Compare the earlier GDPR erasure sweep in bpai.

thklein added the

bug

label

2026-10-03 05:04:30 +00:00

sherpa commented

2026-10-03 05:35:24 +00:00

Member

Confirmed against the code: Rm discards the revisions-store error via _ = in s3.go:86, directory.go:108, and azure.go:140, so a failed revision cleanup is silently reported as success. RemoveAllWithPrefix (s3.go:384, directory.go:415, azure.go:363) only deletes content objects/files under the given prefix and never touches the parallel revisions/<dir>/... tree, unlike DeleteFile (directory.go:142, azure.go:176, s3.go:154) which explicitly calls getRevisionInfo/deleteAllRevisions (revisions.go:14,134) per file. Neither Rm nor RemoveAllWithPrefix acquire fileLocks (filelock.go) before listing/deleting, so a concurrent revisioned write (revisionwrite.go) can recreate a revision or PendingID marker after the delete has already listed the prefix. One sub-claim needs correction before implementing: the S3 Rm prefix argument s.dir (s3.go:86) is always empty in practice, since Rm is only ever invoked on the top-level factory via Client.Rm/getFactory (client.go:146-157), and RemoveAllWithPrefix's empty-prefix branch already resolves to the correct revisions/<dir>/ path — passing the workspace's dir instead, as the issue suggests, would double-nest the prefix and break it, so the actual S3 fix is 'keep passing empty but document why', not swap the variable. The core fix (propagate the error, extend RemoveAllWithPrefix to also sweep the revisions subtree, add locking, add tests) is clear and well-localized to pkg/client, but two things are genuine product/design decisions the code can't answer.

Questions before this can start:

There is no existing workspace-level lock — fileLocks (filelock.go) is keyed per (workspaceID, fileName). Taking 'the file locks' around Rm/RemoveAllWithPrefix means listing files first and locking each one found, which is still racy against a write to a file created after the listing. Is a new workspace-level lock expected, or is best-effort per-listed-file locking (accepting a narrower race window) sufficient for this fix?
The 'one-off sweep for orphaned revisions// prefixes' run against the SaaS bucket(s) is an operational action against production infrastructure, not a code change to this repo. Should that be a script delivered in this repo (e.g. under cmd/), or tracked/executed separately from this PR?

Confirmed against the code: Rm discards the revisions-store error via `_ =` in s3.go:86, directory.go:108, and azure.go:140, so a failed revision cleanup is silently reported as success. RemoveAllWithPrefix (s3.go:384, directory.go:415, azure.go:363) only deletes content objects/files under the given prefix and never touches the parallel `revisions/<dir>/...` tree, unlike DeleteFile (directory.go:142, azure.go:176, s3.go:154) which explicitly calls getRevisionInfo/deleteAllRevisions (revisions.go:14,134) per file. Neither Rm nor RemoveAllWithPrefix acquire fileLocks (filelock.go) before listing/deleting, so a concurrent revisioned write (revisionwrite.go) can recreate a revision or PendingID marker after the delete has already listed the prefix. One sub-claim needs correction before implementing: the S3 Rm prefix argument `s.dir` (s3.go:86) is always empty in practice, since Rm is only ever invoked on the top-level factory via Client.Rm/getFactory (client.go:146-157), and RemoveAllWithPrefix's empty-prefix branch already resolves to the correct `revisions/<dir>/` path — passing the workspace's `dir` instead, as the issue suggests, would double-nest the prefix and break it, so the actual S3 fix is 'keep passing empty but document why', not swap the variable. The core fix (propagate the error, extend RemoveAllWithPrefix to also sweep the revisions subtree, add locking, add tests) is clear and well-localized to pkg/client, but two things are genuine product/design decisions the code can't answer. **Questions before this can start:** - There is no existing workspace-level lock — fileLocks (filelock.go) is keyed per (workspaceID, fileName). Taking 'the file locks' around Rm/RemoveAllWithPrefix means listing files first and locking each one found, which is still racy against a write to a file created after the listing. Is a new workspace-level lock expected, or is best-effort per-listed-file locking (accepting a narrower race window) sufficient for this fix? - The 'one-off sweep for orphaned revisions/<dir>/ prefixes' run against the SaaS bucket(s) is an operational action against production infrastructure, not a code change to this repo. Should that be a script delivered in this repo (e.g. under cmd/), or tracked/executed separately from this PR?

sherpa added the

labels

2026-10-03 05:35:24 +00:00