fix: clone map race condition in UpdateIfNotExists#4328
Open
Amogh-Bharadwaj wants to merge 1 commit into
Open
Conversation
Amogh-Bharadwaj
changed the title
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
❌ 2 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Contributor
❌ Test FailureAnalysis: Test_Soft_Delete_IUD_Same_Batch fails deterministically across all 3 matrix jobs with near-identical ~197s timeouts on "normalize tx", suggesting the fix-clone-map-race PR introduced a hang in the normalize path rather than a flaky failure. |
serprex
reviewed
May 20, 2026
| for col, val := range input.ColToVal { | ||
| // clone input map to avoid concurrent map access, since input may reference | ||
| // a record that was already sent to the CDCStream channel for the sync goroutine | ||
| inputColToVal := maps.Clone(input.ColToVal) |
serprex
reviewed
May 20, 2026
| inputColToVal := maps.Clone(input.ColToVal) | ||
| updatedCols := make([]string, 0, len(inputColToVal)) | ||
| for col, val := range inputColToVal { | ||
| if _, ok := r.ColToVal[col]; !ok { |
Member
There was a problem hiding this comment.
You're still mutating original. There's no lock. What's correct? You want read side to have updated version? Then need to block. Mutex only good if you don't care. You should care
Member
There was a problem hiding this comment.
iow I think update should be moved to before record sent over channel; after sending it should be considered moved, like in rust where after moving object is no longer accessible
Member
There was a problem hiding this comment.
Unless update is because of future record, in which case seems like bug having future record mutated past record
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.
The problem
The pull and sync goroutines share record objects — addRecordWithKey stores a record in CDCStore and sends the same pointer to the CDCStream channel. Since RecordItems/PgItems are value types wrapping a map, copies share the underlying ColToVal map. When a later update/delete arrives for the same primary key, the pull goroutine retrieves the stored record and iterates its ColToVal via UpdateIfNotExists, while the sync goroutine concurrently iterates the same map for JSON serialization or numeric truncation.
The fix
Clone the input's ColToVal map at the start of UpdateIfNotExists in both RecordItems and PgItems, so the pull goroutine iterates a private copy. Additionally, the delete backfill path now uses UpdateIfNotExists (which clones internally) instead of directly assigning r.Items = latestRecord.GetItems(), which would have aliased the shared map.