Skip to content

ROX-36975: Fix race - #22827

Merged
mtodor merged 1 commit into
masterfrom
mtodor/ROX-36975-avoid-race
Sep 15, 2026
Merged

ROX-36975: Fix race#22827
mtodor merged 1 commit into
masterfrom
mtodor/ROX-36975-avoid-race

Conversation

@mtodor

@mtodor mtodor commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

The problem we are facing is that we expect that Pull after Push in main test routine will be executed, but we have another routine (started consumer) waiting on signal and there can be a race where consumer picks item before main routine.

Simple way to reproduce:

		q.Push(99)
		time.Sleep(1 * time.Second)
		pulled := q.Pull()

The purpose of this test is to ensure that empty queue with triggered nonEmpty signal will not cause spin-loop.

Suggested fix
Instead of triggering nonEmpty signal with push->pull - we will trigger it directly.

Related to: #22456

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • modified existing tests

How I validated my change

  • reproduced locally with sleep
  • executed unit tests locally

@mtodor
mtodor requested review from janisz and lvalerom September 14, 2026 12:49
@coderabbitai

coderabbitai Bot commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: e0b3948f-30b1-459d-a9cc-636adcb05503

📥 Commits

Reviewing files that changed from the base of the PR and between a6e66cd and e5b52e8.

📒 Files selected for processing (1)
  • pkg/queue/queue_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Tests
    • Updated queue signaling coverage to verify stale-signal behavior and confirm that items pushed afterward are delivered correctly.

Walkthrough

The queue test directly signals notEmptySignal to create a stale empty-queue wakeup before it verifies later item delivery.

Changes

Queue signaling validation

Layer / File(s) Summary
Stale signal test setup
pkg/queue/queue_test.go		 The test replaces the temporary Push(99) and Pull() sequence with a direct notEmptySignal.Signal() call.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~2 minutes

Change: Bug fix

Suggested reviewers: janisz

Merge Risk: ⚪ Minimal · up to b103d

The test isolates stale wakeups and verifies subsequent item delivery, so the change is mergeable.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the issue and the main change: fixing a race. It is concise and relevant to the pull request.
Description check ✅ Passed The description explains the race, the test change, the related pull request, documentation status, and local validation. The CI inspection checkbox is not checked, but the description is otherwise su…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mtodor/ROX-36975-avoid-race

Comment @coderabbitai help to get the list of available commands.

@janisz
janisz requested a review from vikin91 September 14, 2026 12:50
@codecov

codecov Bot commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.79%. Comparing base (4ce5f0f) to head (b103d8b).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #22827      +/-   ##
==========================================
- Coverage   51.82%   51.79%   -0.04%     
==========================================
  Files        2901     2901              
  Lines      182819   182819              
==========================================
- Hits        94755    94691      -64     
- Misses      79777    79822      +45     
- Partials     8287     8306      +19
Flag Coverage Δ
go-unit-tests 51.79% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🚀 Build Images Ready

Images are ready for commit 51600ba. To use with deploy scripts:

export MAIN_IMAGE_TAG=5.0.x-302-g51600ba89f

janisz
janisz reviewed Sep 14, 2026
View reviewed changes
Comment thread pkg/queue/queue_test.go
@mtodor
mtodor force-pushed the mtodor/ROX-36975-avoid-race branch from a6e66cd to e5b52e8 Compare September 14, 2026 13:45
@mtodor
mtodor force-pushed the mtodor/ROX-36975-avoid-race branch from e5b52e8 to b103d8b Compare September 15, 2026 09:37
@openshift-ci

openshift-ci Bot commented Sep 15, 2026

Copy link
Copy Markdown

@mtodor: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/gke-qa-e2e-tests e5b52e8 link false /test gke-qa-e2e-tests
ci/prow/ocp-4-22-operator-e2e-tests e5b52e8 link false /test ocp-4-22-operator-e2e-tests
ci/prow/ocp-4-12-qa-e2e-tests e5b52e8 link false /test ocp-4-12-qa-e2e-tests
ci/prow/ocp-4-22-nongroovy-e2e-tests e5b52e8 link false /test ocp-4-22-nongroovy-e2e-tests
ci/prow/ocp-4-22-qa-e2e-tests e5b52e8 link false /test ocp-4-22-qa-e2e-tests
ci/prow/ocp-4-12-nongroovy-e2e-tests e5b52e8 link false /test ocp-4-12-nongroovy-e2e-tests
ci/prow/gke-operator-e2e-tests b103d8b link false /test gke-operator-e2e-tests
ci/prow/ocp-4-12-operator-e2e-tests b103d8b link false /test ocp-4-12-operator-e2e-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@mtodor

mtodor commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Failed e2e tests are not related to: pkg/queue/queue_test.go - changed in this PR.

@mtodor
mtodor merged commit 51600ba into master Sep 15, 2026
103 of 112 checks passed
@mtodor
mtodor deleted the mtodor/ROX-36975-avoid-race branch September 15, 2026 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@janisz janisz janisz approved these changes
@lvalerom lvalerom Awaiting requested review from lvalerom
@vikin91 vikin91 Awaiting requested review from vikin91

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mtodor @janisz