fix(rm): support documented destructive removal flags#289
Conversation
overtrue
left a comment
There was a problem hiding this comment.
Self-review note (needs a fix before merge)
The bypass-governance plumbing and the version-listing for delete-marker-only keys are good, with solid unit/header-level tests. But the headline rm --versions path, used recursively without --purge, does not actually purge — I verified this against the server as well.
Findings
[MAJOR] crates/cli/src/commands/rm.rs:285 (+ :315, :389) — rm <prefix>/ --recursive --versions (without --purge) does not purge versions; it silently creates delete markers and reports success
- Why:
--versionslists versions (:257) but, becauseargs.purgeis false, execution falls through to the batchdelete_objects_with_optionspath (:315). On the server,x-rustfs-force-deleteis honored only on the single-object DELETE path (rustfs/src/app/object_usecase.rs:6639readsopts.delete_prefix); the multi-object handler buildsObjectOptions { versioned, version_suspended, object_lock_delete: Some(bypass_governance), ..Default::default() }atobject_usecase.rs:6432— it never setsdelete_prefixand never reads the force-delete header — so the header is a no-op on the batch path. Version IDs are also dropped client-side (list_version_keysinserts onlyitem.key; the batch call sends noVersionId). On a versioned bucket each key therefore just gets a new delete marker while all prior versions remain. Only--purge(per-key single DELETE, :285) actually purges;rb --forceis safe because it setspurge: true. (Bypass-governance itself is plumbed correctly on both paths — that part works.) - Fix: Route force-delete through the per-key single-DELETE path, e.g.
if args.purge || args.versionsat rm.rs:285 (delete_request_optionsalready setsforce_deletefor versions), and add an integration test for the recursive--versionspath.
[MINOR] docs/reference/rc/rb.md — the documented --dangerous flag is now hidden and non-functional
- Why:
rb.mdstill lists--dangerous("Remove the bucket even when incomplete multipart uploads exist"), but this PR hides it from help (rb.rs:25) and keeps rejecting it as unimplemented — a user following the docs hitsUnsupportedFeature. - Fix: Update
rb.mdto drop/mark--dangerousas unsupported so the documented contract matches the CLI.
[MINOR] crates/cli/src/commands/rm.rs:389 — wiring --versions to force_delete extends prefix-delete over-reach to single objects
- Why:
rm bucket/key --versions(non-recursive) now sends force-delete, which the server treats as a recursive prefix delete ofkey/; a sibling object likekey/childwould also be destroyed. Previously this footgun existed only for--purge. - Fix: Document that
--versions/--purgeremove everything under the key path, or scope single-object force-delete to the exact object.
Test coverage
The --purge per-key path is well covered (rm_purge.rs), including an assertion that it avoids the batch path. Gaps: no integration test for the recursive --versions (non-purge) path (would have caught the MAJOR); --bypass and rb --force are only exercised at unit/header level, not end-to-end with exit codes.
Related Issues
Refs rustfs/rustfs#5104.
Background
rc rm --versions,rc rm --bypass, andrc rb --forcewere shown in help but failed during execution. Versioned buckets could be left with non-current versions or delete markers thatrccould not purge.Changes
rm --versionsuse the existing RustFS force-delete path and list object versions for recursive deletes, so delete-marker-only keys are included.rm --bypass.rb --forcepurge bucket contents before deleting the bucket.--incomplete/--dangerousfrom help while keeping explicit runtime rejection if passed.Validation
make pre-commit