s3: map routed write/delete errors and fall back on directory markers

Address review feedback on the routed fast paths:

- PutObject: map the response's machine-readable FilerError code to the same S3
  error the lock path produces (key-too-long, parent/existing-is-file,
  existing-is-directory, precondition), instead of re-wrapping resp.Error in a
  fresh error that filerErrorToS3Error can no longer match by sentinel. Unknown
  codes and in-band errors fall back to the lock path for exact semantics.

- DeleteObject: on any non-precondition routed error, fall back to the lock
  path instead of returning InternalError. This restores directory-marker
  handling (demote a marker that still has children rather than failing on a
  non-empty folder), which the raw DeleteEntry call lacks.
This commit is contained in:
Chris Lu
2026-05-22 23:07:26 -07:00
parent 21af729cb4
commit 20934cc854
3 changed files with 33 additions and 6 deletions
+4 -2
View File
@@ -231,8 +231,10 @@ func (s3a *S3ApiServer) DeleteObjectHandler(w http.ResponseWriter, r *http.Reque
case resp.ErrorCode == filer_pb.FilerError_PRECONDITION_FAILED:
deleteCode, deleteHandled = s3err.ErrPreconditionFailed, true
case resp.Error != "":
glog.Errorf("DeleteObjectHandler: routed delete failed for %s/%s: %s", bucket, object, resp.Error)
deleteCode, deleteHandled = s3err.ErrInternalError, true
// Fall back to the lock path, which carries extra handling the
// raw DeleteEntry lacks (e.g. demoting a directory marker that
// still has children instead of failing on a non-empty folder).
glog.Warningf("DeleteObjectHandler: routed delete to %s returned %q for %s/%s, falling back to lock", owner, resp.Error, bucket, object)
default:
deleteCode, deleteHandled = s3err.ErrNone, true
}
+10 -4
View File
@@ -850,11 +850,17 @@ func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader
switch {
case err != nil:
glog.Warningf("putToFiler: routed create to %s failed for %s, falling back to lock: %v", owner, filePath, err)
case resp.ErrorCode == filer_pb.FilerError_PRECONDITION_FAILED:
createCode, routed = s3err.ErrPreconditionFailed, true
case resp.ErrorCode != filer_pb.FilerError_OK:
// Map known filer error codes to the same S3 errors the lock path
// would produce; fall back for any code this does not recognize.
if code, mapped := filerErrorCodeToS3Error(resp.ErrorCode); mapped {
createCode, routed = code, true
} else {
glog.Warningf("putToFiler: routed create to %s returned code %v for %s, falling back to lock", owner, resp.ErrorCode, filePath)
}
case resp.Error != "":
createErr = fmt.Errorf("%s", resp.Error)
createCode, routed = filerErrorToS3Error(createErr), true
// In-band error without a code: fall back so the lock path maps it.
glog.Warningf("putToFiler: routed create to %s returned %q for %s, falling back to lock", owner, resp.Error, filePath)
default:
entryCreated, createCode, routed = true, s3err.ErrNone, true
}
+19
View File
@@ -128,6 +128,25 @@ func (s3a *S3ApiServer) createEntryOnFiler(owner pb.ServerAddress, req *filer_pb
return resp, err
}
// filerErrorCodeToS3Error maps a routed response's machine-readable FilerError
// to the same S3 error the lock path produces via filerErrorToS3Error, so the
// fast path keeps identical semantics. ok is false for codes it does not map,
// signalling the caller to fall back to the lock path for exact behavior.
func filerErrorCodeToS3Error(code filer_pb.FilerError) (s3err.ErrorCode, bool) {
switch code {
case filer_pb.FilerError_PRECONDITION_FAILED:
return s3err.ErrPreconditionFailed, true
case filer_pb.FilerError_ENTRY_NAME_TOO_LONG:
return s3err.ErrKeyTooLongError, true
case filer_pb.FilerError_PARENT_IS_FILE, filer_pb.FilerError_EXISTING_IS_FILE:
return s3err.ErrExistingObjectIsFile, true
case filer_pb.FilerError_EXISTING_IS_DIRECTORY:
return s3err.ErrExistingObjectIsDirectory, true
default:
return s3err.ErrNone, false
}
}
// buildDeleteCondition reduces a DeleteObject's If-Match header to a primitive.
// DeleteObject only honors If-Match (matching checkDeleteIfMatch), so other
// conditional headers are ignored here as they are on the existing path.