Skip to content

fix: use pointer receivers for piped protobuf models - #7174

Open
srinivasr wants to merge 2 commits into
pipe-cd:masterfrom
srinivasr:fix/copylock-piped
Open

fix: use pointer receivers for piped protobuf models#7174
srinivasr wants to merge 2 commits into
pipe-cd:masterfrom
srinivasr:fix/copylock-piped

Conversation

@srinivasr

Copy link
Copy Markdown

What this PR does:
generated protobuf structs like applicationsyncstate embed a sync.mutex. because their methods were using value receivers, the lock state was being copied every time they were called or passed around (flagged by go vet).

fix:

  • change methods on applicationsyncstate and applicationlivestateversion to use pointer receivers
  • update all caller sites in driftdetector and livestatestore to pass pointers
  • convert range loops in tests to index loops to avoid copying test structs

Why we need it:
passing mutexes by value creates silent data races under concurrent execution because the copies do not block each other.

Which issue(s) this PR fixes:

Fixes # N/A

Does this PR introduce a user-facing change?:

  • How are users affected by this change: no user-facing change.
  • Is this breaking change: no.
  • How to migrate (if breaking change): n/a

@srinivasr
srinivasr requested a review from a team as a code owner August 13, 2026 18:06
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi @srinivasr, welcome to PipeCD and thanks for opening your first pull request!

We’re really happy to have you here

Before your PR gets merged, please check a few important things below.


Helpful resources


DCO Sign-off

All commits must include a Signed-off-by line to comply with the Developer Certificate of Origin (DCO).

In case you forget to sign-off your commit(s), follow these steps:

For the last commit:

git commit --amend --signoff
git push --force-with-lease

For multiple commits:

git rebase --signoff origin/master
git push --force-with-lease

Run checks locally

Before pushing updates, please run:

make check

This runs the same checks as CI and helps catch issues early.


💬 Need help?

If anything is unclear, feel free to ask in this PR or join us on the CNCF Slack in the #pipecd channel.
You can get your Slack invite from: https://communityinviter.com/apps/cloud-native/cncf

Thanks for contributing to PipeCD! ❤️

@netlify

netlify Bot commented Aug 15, 2026

Copy link
Copy Markdown

Deploy Preview for pipecd-site ready!

Name Link
🔨 Latest commit 5d4b105
🔍 Latest deploy log https://app.netlify.com/projects/pipecd-site/deploys/6a8c1d61b61dfe0008b63f77
😎 Deploy Preview https://deploy-preview-7174--pipecd-site.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@areebahmeddd

Copy link
Copy Markdown

i don’t think this branch compiles .. you should run go build ./... and make check locally

Comment thread pkg/model/application.go
Comment thread pkg/app/piped/livestatestore/kubernetes/reflector_test.go Outdated
@srinivasr
srinivasr requested a review from a team as a code owner August 18, 2026 08:51
@srinivasr

Copy link
Copy Markdown
Author

i don’t think this branch compiles .. you should run go build ./... and make check locally

you're right, i missed a caller in builder.go when i was switching the models to use pointer receivers. i just pushed a fix and verified it builds cleanly locally now
image

Copilot stopped reviewing on behalf of srinivasr due to an error August 24, 2026 09:43
Generated protobuf structs such as ApplicationSyncState embed
protoimpl.MessageState, which contains a sync.Mutex. Their methods used
value receivers, so the lock state was copied on every call or pass,
which go vet flags as a copylock violation.

Switch the methods on ApplicationSyncState and
ApplicationLiveStateVersion to pointer receivers and update all caller
sites in driftdetector, livestatestore, livestatereporter, planpreview,
and applicationlivestatestore to pass pointers. Convert range loops in
the affected model tests to index loops so test cases are not copied.

Signed-off-by: srinivasr <sriniv4sreddy@gmail.com>
@srinivasr

Copy link
Copy Markdown
Author

sorry for the delay on this. merged master in (it has #7137 now which touches the same HasDiff func, both changes work together fine) and squashed the branch while I was at it since the history had piled up fixups, so commit dates show today. doc comment is back and the unrelated reflector_test change is reverted as discussed

also pulled the metadatastore race fix out of this PR into its own branch to keep this one focused on what was reviewed.

@srinivasr
srinivasr requested a review from areebahmeddd August 24, 2026 10:30
@areebahmeddd

Copy link
Copy Markdown

looks good. maybe @khanhtc1202 will give a final look and merge 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants