Fix silent data loss in DMap.Delete with multi-key/multi-owner deletes - #287
Open
fcraviolatti wants to merge 1 commit into
Open
Fix silent data loss in DMap.Delete with multi-key/multi-owner deletes#287fcraviolatti wants to merge 1 commit into
fcraviolatti wants to merge 1 commit into
Conversation
deleteKeys groups the requested keys by partition owner, then loops over that map to delete each group. The remote-owner branch of the loop returned unconditionally after handling the first remote owner (both on success and on error), so any additional remote owners in the map were never processed. With a multi-node cluster and enough keys to span more than one remote owner, Delete(ctx, keys...) silently skipped deleting the keys owned by every remote owner after the first, returning no error. Fix the fan-out to use errgroup, matching the pattern already used by deleteBackupOnCluster and destroyOnCluster: every owner (local and remote) is processed, and the first error (if any) is returned only after all owners have been attempted. The returned count now reflects the keys actually processed across all owners. Added TestDMap_Delete_MultiKeyDifferentRemoteOwners, which reproduces the bug deterministically on a 3-node cluster by asserting the test's own key set hashes to at least two different remote owners before calling Delete.
There was a problem hiding this comment.
Pull request overview
Fixes a silent data loss bug in DMap.Delete where multi-key deletes spanning multiple remote partition owners would stop after the first remote owner, leaving remaining keys undeleted without returning an error.
Changes:
- Refactors
deleteKeysto fan out deletes to all owners usingerrgroup.Groupand aggregate the processed-key count. - Adds a regression test ensuring multi-key deletes across multiple remote owners actually delete all keys.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/dmap/delete.go | Fans out delete requests to all owners concurrently and waits for completion instead of returning after the first remote owner. |
| internal/dmap/delete_test.go | Adds a regression test covering multi-key deletes across multiple remote owners. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+151
to
+153
| // Fan out to every owner instead of stopping at the first remote one - | ||
| // see the errgroup pattern used by deleteBackupOnCluster/destroyOnCluster. | ||
| var count int64 |
| require.NoError(t, err) | ||
|
|
||
| ctx := context.Background() | ||
| const keyCount = 30 |
Merged
3 tasks
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.
Fixes #286
Summary
deleteKeysreturned unconditionally after the first remote owner(success or error), skipping remaining remote owners in multi-key
deletes and silently leaving their keys undeleted.
errgroup.Group, matching the pattern already usedby
deleteBackupOnCluster/destroyOnClusterin the same file: everyowner is attempted, first error is returned only after all owners
have completed. Returned count reflects keys actually processed.
Test plan
TestDMap_Delete_MultiKeyDifferentRemoteOwners, confirmedit fails on master and passes with this fix
go test ./internal/dmap/... -run TestDMap_Delete(8/8 pass)go test ./...(539/539 pass, 35 packages)go test ./internal/dmap/... -race(97/97 pass, no races)