fix(core): use index-based tracking in BatchDeleter to fix progress stalls#7401
Open
TennyZhuang wants to merge 3 commits intomainfrom
Open
fix(core): use index-based tracking in BatchDeleter to fix progress stalls#7401TennyZhuang wants to merge 3 commits intomainfrom
TennyZhuang wants to merge 3 commits intomainfrom
Conversation
…talls BatchDeleter previously used a HashSet<(String, OpDelete)> to track pending deletions and relied on OpDelete equality to remove completed items. However, services like S3 and OSS reconstruct OpDelete from response data without preserving all fields (e.g., `recursive`), causing buffer.remove() to silently fail. This left items permanently stuck in the buffer, eventually triggering a "no progress" error. Switch to index-based tracking: BatchDeleteResult now reports success and failure by index into the input batch Vec, and BatchDeleter uses a Vec instead of HashSet. This eliminates the dependency on OpDelete equality entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dosubot
Bot
added
size:L
…entries Address two review findings: 1. flush_buffer() now restores items to self.buffer when delete_batch() returns Err, preventing data loss on transport-level failures. 2. S3 and OSS deleters now use HashMap<key, Vec<usize>> instead of HashMap<key, usize> to correctly handle duplicate entries in the same batch without collapsing their indices. Added a regression test for the transport error buffer restoration path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rror The single-item fast path in flush_buffer() removed the item from the buffer before calling delete_once(). If delete_once() returned an error, the item was permanently lost and could not be retried. Now the item is cloned and only cleared from the buffer after a successful delete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
2 participants
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.
Which issue does this PR close?
N/A (bug found through code analysis and deterministic testing)
Rationale
BatchDeleterpreviously used aHashSet<(String, OpDelete)>to track pending deletions and relied onOpDeleteequality (which derivesHash/Eqover bothversionandrecursivefields) to remove completed items viabuffer.remove().However, services like S3 and OSS reconstruct
OpDeletefrom XML response data without preserving all original fields — for example,recursivealways defaults tofalsein the reconstructedOpDelete. This causesbuffer.remove()to silently fail when the originalOpDeletehadrecursive: true, leaving items permanently stuck in the buffer and eventually triggering the "no progress" error inclose().Changes
BatchDeleteResultfrom(String, OpDelete)tuples to index-based tracking (Vec<usize>for succeeded,Vec<(usize, Error)>for failed), referencing positions in the inputbatchVecBatchDeleter.bufferfromHashSettoVec, removing the dependency onOpDeleteHash/Eqdelete_batchimplementations (S3, OSS, GCS, Azblob, Swift, HF, Cloudflare-KV, object_store) to report results by indexAre there any user-facing changes?
No.
BatchDeleteResultis an internal type inraw::oio— not part of the public user API.This PR was generated with the assistance of an LLM (Claude Opus 4.6) as a coding tool.
🤖 Generated with Claude Code