🐛 priorityqueue: fix data race on []*item in logState by lanfix · Pull Request #3588 · kubernetes-sigs/controller-runtime

lanfix · 2026-09-08T12:50:22Z

What this does

logState collected the queue's items into a []*item[T] under w.lock,
released the lock, and then passed those pointers to the logger for
serialization. The pointed-to item[T] values stay live in w.items /
w.waiting / w.ready and keep being mutated by the other queue goroutines
while the logger walks them, so the snapshot is a data race.

This PR takes a by-value deep copy of every item (including the *time.Time
behind ReadyAt) under w.lock and hands that copy to the logger. The lock is
held only for the cheap copy, not for serialization, and the logged payload is
fully detached from the live queue state. Log output is unchanged ([]item[T]
and []*item[T] marshal identically; readyAt,omitempty still works).

Why we need it

With a logger that has V(5) enabled, logState runs every 10s. The race is
observable via go test -race, and in production it intermittently crashes the
controller process:

linux-foundation-easycla · 2026-09-08T12:50:29Z

The committers listed above are authorized under a signed CLA.

✅ login: lanfix / name: lanfix (796e145)

kubernetes-prow · 2026-09-08T12:50:32Z

Welcome @lanfix!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

kubernetes-prow · 2026-09-08T12:50:33Z

Hi @lanfix. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alvaroaleman

/ok-to-test

alvaroaleman · 2026-09-09T14:44:21Z

+// data race. The race can crash the process from within encoding/json when a
+// ReadyAt pointer is observed non-nil and then dereferenced after it went nil:
+// "value method time.Time.MarshalJSON called using nil *Time pointer".
+func (w *priorityqueue[T]) cloneItems() []item[T] {


Please add a test for this that validates it copies all fields now and in the future, maybe using randfill or such

Done in 2ba4b6f

alvaroaleman · 2026-09-11T00:27:17Z

+		})
+	}
+
+	time.Sleep(500 * time.Millisecond)


Use synctest instead of sleeps, please

Done. The test now runs inside synctest.Test and advances fake time with time.Sleep(10 * time.Second) so that logState's 10s tick fires.

alvaroaleman · 2026-09-11T00:28:33Z

+// TestPriorityQueueCloneItemsCopiesAllFields checks that cloneItems returns items that are
+// equal to the queued ones and share no memory with them. Filling the items with random
+// values keeps both checks valid for fields that are added to item in the future.
+func TestPriorityQueueCloneItemsCopiesAllFields(t *testing.T) {


Please factor the code such that this can test only the cloning, not a bunch of unrelated things. Also, the testname is self-explanatory and doesn't require a comment

Done. Extracted the per-item copy into item.clone() and cloneItems just calls it, so the tests only exercise clone() on a randfill-ed item and don't build a queue anymore. Dropped the comment. The randfill setup lives in newItemFiller, as randfill can't fill time.Time on its own (all its fields are unexported).

alvaroaleman · 2026-09-11T00:30:41Z

+		})
+	}
+
+	// Readers: do what logState does - snapshot the queue and serialize it.


Can we simply pass in a logger at v5 instead to test what actually does happen rather than imitate it? The race detector will go off if a race could happen, it doesn't require the race to actually happen

Done. newQueue now accepts Opts and the test passes a funcr logger with Verbosity: 5, so the real logState does the reading. I verified that the race detector reports the old logState implementation with this test (read in logState vs. write of item.ReadyAt in lockedAddWithOpts), and that it is clean with the current one.

alvaroaleman · 2026-09-11T00:33:46Z

+	// Writers: churn items through waiting -> ready (handleWaitingItems clears
+	// ReadyAt) and through the update path (lockedAddWithOpts reassigns ReadyAt).
+	for range 8 {
+		wg.Go(func() {


I don't think using randomized keys adds anything here, the race happens if the after for the same key gets changed which only happens if its lower, so a simpler version of this would be to just add the same key in a loop with a lowering after, maybe by substracting the loop index from a start value or some such

Done. The test now re-adds the same key in a loop with a lowering After (time.Hour minus i minutes) and sleeps between the adds so that logState logs the item in between the ReadyAt updates.

alvaroaleman · 2026-09-11T00:35:52Z

+			t.Fatalf("cloned items don't match the queued ones: %s", diff)
+		}
+
+		// Equal fields are not enough, a clone must not reference the item it was cloned from.


If anything, make this its own test please

Done, split into TestItemCloneCopiesAllFields (cmp.Diff against the original) and TestItemCloneDoesNotShareMemoryWithOriginal (no pointer/map/slice field of the clone may point into the original).

Note that copying the time.Time behind ReadyAt isn't needed for the race itself (the pointee is never mutated in place), it only guards against a future in-place update.

alvaroaleman · 2026-09-13T16:27:25Z

/cherrypick release-0.25

k8s-infra-cherrypick-robot · 2026-09-13T16:27:29Z

@alvaroaleman: once the present PR merges, I will cherry-pick it on top of release-0.25 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-0.25

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kubernetes-prow · 2026-09-13T16:27:46Z

LGTM label has been added.

Details

Git tree hash: 06001486f87eec7d7a9ad5f59d0bc456660a6c3e

kubernetes-prow · 2026-09-13T16:27:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, lanfix

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-infra-cherrypick-robot · 2026-09-13T16:30:02Z

@alvaroaleman: new pull request created: #3593

Details

In response to this:

/cherrypick release-0.25

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sbueringer · 2026-09-23T10:18:14Z

Thx! Nice fix!

/cherry-pick release-0.24

k8s-infra-cherrypick-robot · 2026-09-23T10:18:53Z

@sbueringer: new pull request created: #3602

Details

In response to this:

Thx! Nice fix!

/cherry-pick release-0.24

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

🐛 priorityqueue: Fix the data race with ReadAt field;

796e145

kubernetes-prow Bot requested review from alvaroaleman and sbueringer September 8, 2026 12:50

kubernetes-prow Bot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 8, 2026

kubernetes-prow Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 8, 2026

kubernetes-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 8, 2026

alvaroaleman reviewed Sep 9, 2026

View reviewed changes

kubernetes-prow Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 9, 2026

🐛 priorityqueue: Changes after review;

2ba4b6f

lanfix requested a review from alvaroaleman September 10, 2026 11:59

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 11, 2026

alvaroaleman reviewed Sep 11, 2026

View reviewed changes

🐛 priorityqueue: Changes after review, part 2;

c8b5719

lanfix requested a review from alvaroaleman September 11, 2026 08:18

lanfix changed the title ~~🐛 priorityqueue: fix data race on item.ReadyAt in logState~~ 🐛 priorityqueue: fix data race on []*items in logState Sep 11, 2026

lanfix changed the title ~~🐛 priorityqueue: fix data race on []*items in logState~~ 🐛 priorityqueue: fix data race on []*item in logState Sep 11, 2026

alvaroaleman approved these changes Sep 13, 2026

View reviewed changes

kubernetes-prow Bot assigned alvaroaleman Sep 13, 2026

kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2026

kubernetes-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2026

kubernetes-prow Bot merged commit a1be2bf into kubernetes-sigs:main Sep 13, 2026
18 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Sep 13, 2026

[release-0.25] 🐛 priorityqueue: fix data race on []*item in logState #3593

Merged

sbueringer mentioned this pull request Sep 23, 2026

Feature: Priority Queue #2374

Open

35 tasks done

k8s-infra-cherrypick-robot mentioned this pull request Sep 23, 2026

[release-0.24] 🐛 priorityqueue: fix data race on []*item in logState #3602

Merged

roguepikachu mentioned this pull request Oct 12, 2026

[Enhancement] Move controller-runtime from the main-branch pin to the v0.26.0 release kubevela/kubevela#7451

Open

1 task done

Conversation

lanfix commented Sep 8, 2026

What this does

Why we need it

Uh oh!

linux-foundation-easycla Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubernetes-prow Bot commented Sep 8, 2026

Uh oh!

kubernetes-prow Bot commented Sep 8, 2026

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanfix Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvaroaleman commented Sep 13, 2026

Uh oh!

k8s-infra-cherrypick-robot commented Sep 13, 2026

Uh oh!

kubernetes-prow Bot commented Sep 13, 2026

Uh oh!

kubernetes-prow Bot commented Sep 13, 2026

Uh oh!

Uh oh!

k8s-infra-cherrypick-robot commented Sep 13, 2026

Uh oh!

sbueringer commented Sep 23, 2026

Uh oh!

k8s-infra-cherrypick-robot commented Sep 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linux-foundation-easycla Bot commented Sep 8, 2026 •

edited

Loading

lanfix Sep 11, 2026 •

edited

Loading