Skip to content

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

Merged
armistcxy merged 5 commits into
pipe-cd:masterfrom
srinivasr:fix/copylock-piped
Aug 26, 2026
Merged

fix: use pointer receivers for piped protobuf models#7174
armistcxy merged 5 commits into
pipe-cd:masterfrom
srinivasr:fix/copylock-piped

Conversation

@srinivasr

Copy link
Copy Markdown
Contributor

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 canceled.

Name Link
🔨 Latest commit 81e5142
🔍 Latest deploy log https://app.netlify.com/projects/pipecd-site/deploys/6a8dd63c4834840009b3bd87

@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
Contributor 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
Contributor 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 🙂

Comment thread pkg/app/piped/livestatestore/kubernetes/store.go

@armistcxy armistcxy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, there one place I think you should add comment on top of that line

@srinivasr
srinivasr requested a review from armistcxy August 25, 2026 17:52
@armistcxy

Copy link
Copy Markdown
Contributor

Special thanks to @areebahmeddd for helping with the review 💯

@armistcxy armistcxy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@armistcxy
armistcxy merged commit 2caadb6 into pipe-cd:master Aug 26, 2026
64 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Thank you for contributing to PipeCD, @srinivasr! The changes in this pull request will be part of the upcoming release!

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.

3 participants