Skip to content

Conversation

@eileenaaa
Copy link
Contributor

Description

This PR addresses a stability issue in mutation handling:

  • Replaced shallow copy of committedUids with deep copy to prevent external
    modifications from corrupting committed state.

These changes make committedUids safer and preserve correctness during refresh.

Checklist

  • Code compiles correctly and linting passes locally
  • For all code changes, an entry added to the CHANGELOG.md file describing and linking to
    this PR
  • Tests added for new functionality, or regression tests for bug fixes added as applicable

@eileenaaa eileenaaa requested a review from a team August 19, 2025 10:04
@ghost
Copy link

ghost commented Aug 19, 2025

Any reason for this? From what I understand, the underlying structures wont be changed. Did you notice any issue or just because of linters / safety?

@eileenaaa
Copy link
Contributor Author

Any reason for this? From what I understand, the underlying structures wont be changed. Did you notice any issue or just because of linters / safety?

The change to proto.Clone(post).(*pb.Posting) was made because PR 9480 aims to fix a race condition in the mutation map.
Simply doing newMap[uid] = post only copies the pointer, meaning multiple goroutines could still share and mutate the same underlying object.
By using proto.Clone, each entry becomes an independent copy, which is necessary to fully prevent the race condition.

@ghost
Copy link

ghost commented Aug 20, 2025

@eileenaaa The reason I didn't do this in that PR was, even though map might get updated, the underlying values don't get updated. This is already committed data which is not going to get updated. Make a proto clone there only adds more memory without really giving us any benefit. Hence I asked did you see any error, which would mean that my theory was wrong.

@eileenaaa
Copy link
Contributor Author

eileenaaa commented Aug 22, 2025

@harshil-goel I checked the subsequent code and confirmed that committedUids data is indeed not modified afterwards. I also ran tests with concurrent data, and it works fine, so making a deep copy here is not necessary.
Sorry for the confusion — I initially misunderstood the scope of PR 9480 and thought it was addressing the map race itself, which is why I raised the concern about not doing a deep copy here. Since this part is fine, I’ll go ahead and close the PR.

@eileenaaa eileenaaa closed this Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant