fix(inkless:storage): bound bulk-delete failure logging - #742
Draft
jeqo wants to merge 3 commits into
Draft
Conversation
…throttling Under S3 throttling the FileCleaner cycle aborted entirely: S3Storage.delete threw on the first per-key or whole-request error, so FileCleaner never dereferenced the files it had already deleted. The next cycle re-fetched the same set and re-issued DeleteObjects for objects that were already gone, looping without draining and amplifying the throttle. Change the delete contract so partial progress is preserved: - ObjectDeleter.delete(Set) now returns the subset of keys confirmed deleted (idempotent: already-absent keys count as deleted) instead of void. The in-memory and config-test backends return the input set on success. - S3Storage.delete does a single pass per 1000-key batch: it collects the keys S3 confirmed deleted (DeleteObjectsResponse.deleted()) and logs the rest, distinguishing throttling from hard errors for diagnostics only. It no longer throws for partial or whole-request failures (incl. a 503 the SDK's adaptive retry exhausted); undeleted keys stay marked for deletion and are retried on the next cycle. - AzureBlobStorage.delete deletes one blob at a time, so it now accumulates and returns the confirmed-deleted subset instead of throwing on the first failure and abandoning the rest (same monotonic-drain benefit as S3). - GcsStorage.delete stays all-or-nothing: Storage.delete(Iterable) is an atomic batch that raises a genuine failure as a thrown exception rather than a per-blob flag, so a confirmed-deleted subset cannot be extracted; a thrown batch drains nothing and the whole set is retried next cycle (documented inline). - FileCleaner dereferences only the confirmed-deleted subset in the control plane and records that count, so the set shrinks monotonically instead of re-attempting already-deleted keys. Request-rate backoff stays where it already lives: the S3 client's ADAPTIVE_V2 retry strategy within a cycle, and the file-cleaner schedule between cycles. No in-run backoff is added here. No control-plane change is needed: deleteFiles already accepts any subset. Tests: FileCleanerMockedTest gains partial-drain, nothing-deleted, and storage-failure cases, and the existing cases stub the new return value; new S3StorageDeleteTest covers single-pass partial return without retry and a whole-request failure that must not propagate; Azure/GCS are covered by the existing BaseStorageTest.testDeletes integration tests.
Partial delete progress is now the normal case: the delete contract returns the subset of keys the backend confirmed, and a throttled or rejected key stays marked for deletion instead of throwing. Nothing reports that. FileCleanerErrorRate counts thrown exceptions, which the delete path no longer raises for partial failure, and FileCleanerFilesRate counts only what succeeded - so a cycle that drains half its worklist is indistinguishable from one that drains all of it, cycle after cycle. Add FileCleanerFilesFailedRate, the difference between the keys submitted and the keys confirmed. A sustained non-zero value is the signal that keys are being re-attempted every cycle rather than draining, which is what distinguishes throttling (recovers on its own) from a hard per-key failure (does not). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A delete pass that fails for every key it submitted logged one line per key (with a stack trace per key on Azure) and repeated it on every cleanup cycle, since undeleted keys stay marked for deletion. GCS still interpolated the whole key set into the exception message, the bug #733 fixed for S3. Aggregate per-key failures into DeleteErrorSummary: a count by error code, at most 3 sampled hard failures, and the first hard cause. One line per call, bounded by the distinct error codes rather than the key count, at INFO when every failure was a throttle and WARN otherwise. GCS cannot report per-key results, so its message just carries the key count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jeqo
force-pushed
the
jeqo/bound-bulk-delete-logging
branch
from
August 12, 2026 16:11
ffcad22 to
194bab8
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Bounds repeated bulk-delete failure logs across cloud storage backends.
Changes:
- Adds aggregated error counts, samples, and causes.
- Applies bounded logging to S3 and Azure.
- Removes GCS key enumeration from exceptions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
DeleteErrorSummaryTest.java |
Tests summary bounds and formatting. |
DeleteErrorSummary.java |
Implements aggregated failure logging. |
S3Storage.java |
Aggregates per-key S3 failures. |
AzureBlobStorage.java |
Aggregates Azure delete exceptions. |
GcsStorage.java |
Reports only failed key count. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+65
to
+66
| private static final Set<String> THROTTLE_ERROR_CODES = | ||
| Set.of("ServerBusy", "InternalError", "OperationTimedOut"); |
Comment on lines
+77
to
+82
| if (samples.isEmpty()) { | ||
| logger.info(summary); | ||
| } else if (firstHardFailure != null) { | ||
| logger.warn(summary, firstHardFailure); | ||
| } else { | ||
| logger.warn(summary); |
| logDeleteErrors(response.errors()); | ||
| collectDeleteErrors(response.errors(), errors); | ||
| } | ||
| errors.log(LOGGER, objectKeys.size(), deleted.size()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A delete pass that fails for every key it submitted logged one line per key (with a stack trace per key on Azure) and repeated it on every cleanup cycle, since undeleted keys stay marked for deletion. GCS still interpolated the whole key set into the exception message, the bug #733 fixed for S3.
Aggregate per-key failures into DeleteErrorSummary: a count by error code, at most 3 sampled hard failures, and the first hard cause. One line per call, bounded by the distinct error codes rather than the key count, at INFO when every failure was a throttle and WARN otherwise. GCS cannot report per-key results, so its message just carries the key count.