blob: fix bulk delete breaking on more than 1000 objects - #531
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #531 +/- ##
=========================================
Coverage 82.39% 82.40%
Complexity 662 662
=========================================
Files 210 210
Lines 14334 14342 +8
Branches 1932 1932
=========================================
+ Hits 11811 11819 +8
Misses 1696 1696
Partials 827 827
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| protected void doDelete(Collection<BlobIdentifier> objects) { | ||
| s3Client.deleteObjects(transformer.toDeleteRequests(objects)); | ||
| for (DeleteObjectsRequest request : transformer.toDeleteRequests(objects)) { | ||
| s3Client.deleteObjects(request); |
There was a problem hiding this comment.
DeleteObjectsResponse is discarded, so per-key delete failures are silently reported as success.
S3 DeleteObjects returns HTTP 200 even when individual keys fail — the failures come back in the response's Errors array (AccessDenied on an object-lock/retention-protected or cross-account key, InternalError, etc.), and the SDK does not throw for them. Both this loop and AwsAsyncBlobStore.doDelete (which drops each response into CompletableFuture.allOf) throw the response away; .errors() is referenced nowhere in the blob module. That contradicts BucketClient.delete(Collection)'s documented contract: "@throws SubstrateSdkException Thrown if the operation fails."
This predates the PR, but chunking is what makes it dangerous. Previously a >1000-key delete failed loudly, so this could only ever hide failures within one request. Now a 50,000-object delete issues 50 requests and returns success even if thousands of keys were rejected — the caller has no way to learn which objects still exist. On a retention/erasure path that is a silent compliance failure.
Suggested fix: collect response.errors() across chunks and, if non-empty, throw a SubstrateSdkException naming the failed keys and their error codes.
| CompletableFuture<?>[] deletions = | ||
| transformer.toDeleteRequests(objects).stream() | ||
| .map(request -> client.deleteObjects(request)) | ||
| .toArray(CompletableFuture[]::new); | ||
| return CompletableFuture.allOf(deletions); |
There was a problem hiding this comment.
The async path submits every chunk at once, so in-flight request count is driven directly by caller input with no cap.
toDeleteRequests(objects).stream().map(client::deleteObjects) eagerly starts ceil(n/1000) requests before allOf is built. A 5M-object delete fires 5,000 concurrent deleteObjects calls. NettyNioAsyncHttpClient defaults to maxConcurrency 50 with a 10s connectionAcquisitionTimeout and a 10,000-entry pending-acquire queue, so the tail of that burst fails with connection-acquisition timeouts rather than being throttled gracefully, and S3 will answer the burst with 503 SlowDown. Because allOf fails as soon as any chunk fails while the rest keep running, the caller gets one exception and no record of which chunks landed.
The sync path is deliberately sequential here, so the two implementations of the same driver method have very different blast radii under load.
Suggested fix: bound the fan-out — e.g. chain chunks with thenCompose in groups of a fixed width (8–16), or walk the chunk list with a small fixed-size window — so concurrency is a constant rather than a function of the caller's collection size.
| .bucket(getBucket()) | ||
| .delete(Delete.builder().objects(objectIds).build()) | ||
| .build(); | ||
| return Lists.partition(objectIds, MAX_DELETE_OBJECTS_PER_REQUEST).stream() |
There was a problem hiding this comment.
This duplicates a chunking mechanism that already exists in this module, leaving two constants for the same S3 limit and a now-redundant double partition.
AwsTransformer already has partitionList(List<BlobInfo>, int) (line 924), and AwsAsyncBlobStore already declares private static final int MAX_OBJECTS_PER_DELETE = 1000 (line 95) and uses it in doDeleteDirectory to pre-chunk batches at exactly this limit before calling doDelete:
List<List<BlobInfo>> partitionedBlobLists =
transformer.partitionList(batch.getBlobs(), MAX_OBJECTS_PER_DELETE);
for (List<BlobInfo> blobList : partitionedBlobLists) {
futures.add(doDelete(transformer.toBlobIdentifiers(blobList)));
}After this change doDeleteDirectory partitions at 1000 and then toDeleteRequests partitions the result at 1000 again — harmless, but the outer pre-chunking is now dead weight, and the S3 key limit is encoded in two separate constants in the same module.
Suggested fix: keep MAX_DELETE_OBJECTS_PER_REQUEST as the single source of truth, delete AwsAsyncBlobStore.MAX_OBJECTS_PER_DELETE, and drop the now-unnecessary partitionList pre-chunking in doDeleteDirectory so bulk delete is batched in exactly one place.
4 participants
Summary
Right now deleting more than 1000 objects at once just fails. S3 caps DeleteObjects at 1000 keys per request and we were sending them all in one shot. So any large cleanup blows up.
This splits the delete into batches under the limit (one after another for the sync client, in parallel for async), so deletes of any size just work now. Added tests for the >1000 case and the empty case.
Ref: https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObjects.html