s3: stop treating a directory marker as a versioned object (#10573)

* s3: delete a directory marker instead of versioning it

The key "dir/" is stored as the filer directory itself, so a delete marker
cannot stand in for it without hiding the children underneath, and its history
has to sit inside the directory it describes, where listings keep meeting it.
Delete it the way an unversioned bucket already does: remove the directory
when nothing is left under it, demote it to a plain directory when children
remain, and drop a history an older build recorded for it.

* s3: stop resolving directory markers through a version history

Nothing records one for them any more, so the lookups that read it are dead
weight - and the one in the listing was a filer round trip per directory
marker returned, which for a bucket that keeps a marker per directory is the
whole listing cost. A listing reads what a directory stands for straight off
the entry it already has; a unit test pins that N markers cost one ListEntries
rather than N+1. The guard that keeps a history left inside a directory by an
older build from surfacing as a key named after it stays.

* s3: do not let deleting "dir/" destroy the object at "dir"

Writing under an existing object turns that object's entry into a directory
while it keeps its data, so the keys "m2" and "m2/" end up sharing one entry.
Stripping the entry to delete "m2/" therefore wiped the object at "m2" - a
different key, and in a versioned bucket one no delete marker records. Leave a
directory holding uploaded data alone; "m2/" does not name it.

* s3: make the directory-marker delete fail closed and take the write lock

The guard that spares a promoted file only fired when the entry read
succeeded, so a transient filer error fell through to the delete and could
destroy the object at "dir" anyway. Fail the request instead, take the object
write lock so the entry cannot change between the check and the delete, and
report a stale history that cannot be removed rather than leaving it to keep
naming the key in ListObjectVersions.

* s3: check If-Match inside the directory-marker delete lock

The lock belongs to the caller: taking it inside the delete nested it under the
batch handler's own lock, and since every lock from a gateway shares one owner
the inner release would have freed it while the outer caller still assumed it
held it. Both callers now own the lock, the single-object path re-checks
If-Match inside it the way the other delete paths do, and a batch delete of a
trailing-slash key in an unversioned bucket goes through the same marker path
instead of the raw delete. A history lookup that fails now fails the delete.
This commit is contained in:
Chris Lu authored and GitHub committed 2026-08-05 00:24:54 -07:00
1 parent 03388c4beb
commit 7d6c83b126
8 files changed
+247 -287

No files matched your search

+6 -25
View File
@@ -649,9 +649,8 @@ func (vc *versionCollector) processVersionsDirectory(entryPath string) error {
return nil
}
// processExplicitDirectory handles an explicit S3 directory object. containerPath is
// the directory's own filer path, which is where the key's version history lives.
func (vc *versionCollector) processExplicitDirectory(entryPath, containerPath string, entry *filer_pb.Entry) error {
// processExplicitDirectory handles an explicit S3 directory object
func (vc *versionCollector) processExplicitDirectory(entryPath string, entry *filer_pb.Entry) {
directoryKey := entryPath
if !strings.HasSuffix(directoryKey, "/") {
directoryKey += "/"
@@ -663,33 +662,18 @@ func (vc *versionCollector) processExplicitDirectory(entryPath, containerPath st
// this mirrors ListObjectsV2 and AWS, and stops clients like Veeam that
// reject unexpected keys in a listing from aborting.
if !strings.HasPrefix(directoryKey, vc.prefix) {
return nil
return
}
// Skip directories at or before keyMarker
if vc.keyMarker != "" && directoryKey <= vc.keyMarker {
return nil
}
// The directory entry is this key's null version. A history under it names the
// current version, so the null is only latest when there is no pointer to follow.
// A history we cannot read leaves that unknown, and claiming latest would hide a
// current delete marker, so the listing fails instead of guessing.
isLatest := true
versionsEntry, err := vc.s3a.getEntry(containerPath, s3_constants.VersionsFolder)
switch {
case err == nil:
if _, hasPointer := versionsEntry.Extended[s3_constants.ExtLatestVersionIdKey]; hasPointer {
isLatest = false
}
case !errors.Is(err, filer_pb.ErrNotFound):
return fmt.Errorf("read version history of %s: %w", containerPath, err)
return
}
versionEntry := &VersionEntry{
Key: directoryKey,
VersionId: "null",
IsLatest: isLatest,
IsLatest: true,
LastModified: time.Unix(entry.Attributes.Mtime, 0),
ETag: "\"d41d8cd98f00b204e9800998ecf8427e\"", // Empty content ETag
Size: 0,
@@ -697,7 +681,6 @@ func (vc *versionCollector) processExplicitDirectory(entryPath, containerPath st
StorageClass: StorageClass(vc.s3a.getStorageClassFromExtended(entry.Extended)),
}
*vc.allVersions = append(*vc.allVersions, versionEntry)
return nil
}
// processRegularFile handles a regular file entry (pre-versioning or suspended-versioning object)
@@ -887,9 +870,7 @@ func (vc *versionCollector) processDirectory(currentPath, entryPath string, entr
// an SDK PutObject of "dir/" carries a default Content-Type, so the two
// listings must agree on what counts as a directory key.
if entry.IsDirectoryKeyObject() {
if err := vc.processExplicitDirectory(entryPath, path.Join(currentPath, entry.Name), entry); err != nil {
return err
}
vc.processExplicitDirectory(entryPath, entry)
}
// Skip entire subdirectory if all keys within it are before the keyMarker.