ROX-36973: Prevent delegated scan hang when no Scanner V2 by dcaravel · Pull Request #22836 · stackrox/stackrox

dcaravel · 2026-09-14T23:05:08Z

Description

Delegated scan requests will hang for the full timeout (10 mins) when a Secured Cluster only has Scanner V4 installed and Central does not have Scanner V4 installed.

There is historically no Scanner V2 specific capability/message exchanged between Central/Sensor to indicate Scanner V2 is installed.

Now that Scanner V2 is removed, there is no need for Sensor to try to connect to it. This PR removes that path.

Additionally fixes a potential TOCTOU nil pointer panic in Sensor delegated scanning flow that could occur in an (assumingly) extreme edge case (Central's advertised scanner state changes after a delegated scan starts but before the scan request is sent to scanner)

User-facing documentation

CHANGELOG.md is updated OR update is not needed
documentation PR is created and is linked above OR is not needed

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

added unit tests

How I validated my change

CI and manual testing

A perhaps extreme edge case existed where the singleton would return a non-nil client at the scan entrypoint point and then return a nil client later on - causing a nil pointer exception and crash Sensor.

openshift-ci · 2026-09-14T23:05:11Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

coderabbitai · 2026-09-14T23:07:53Z

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: Advanced

Run ID: 7a754766-d461-4255-945e-7d38d3f823b1

📥 Commits

Reviewing files that changed from the base of the PR and between 5521eb0 and d46f942.

📒 Files selected for processing (1)

sensor/common/scan/scan_test.go

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

📝 Summary

Summary by CodeRabbit

Bug Fixes
- Improved scanner client handling to prevent image-analysis attempts when no supported scanner is available.
- Ensured image analysis consistently uses the selected scanner client, preventing intermittent failures.
Changes
- Scanner connectivity now uses Scanner V4 exclusively.
- Removed legacy Scanner V2 connection and image-analysis support.
- Added clearer warnings when Scanner V4 is unavailable or unsupported.

Walkthrough

The change removes Scanner V2 support, restricts singleton creation to Scanner V4, and passes one captured scanner client through image analysis.

Changes

Scanner client flow

Layer / File(s)	Summary
Scanner V4-only client selection `sensor/common/scannerclient/grpc_client.go`, `sensor/common/scannerclient/singleton.go`	Scanner V2 dialing and image analysis methods were removed. The singleton returns nil when Scanner V4 is disabled or unsupported, and otherwise creates a Scanner V4 client.
Image analysis client propagation `sensor/common/scan/scan.go`	Image enrichment stores the singleton result, checks it for nil, and passes the captured client to `fetchImageAnalysis` and `scanImg`.
Regression coverage `sensor/common/scan/scan_test.go`	The test verifies that the singleton is called once and that the captured client is passed to the scan operation.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: alkmim

Merge Risk: ⚪ Minimal · up to d46f9

The captured-client regression test covers the intended nil-client edge case. No merge-blocking issue is identified.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
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 3 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.
Title check	✅ Passed	The title clearly identifies the primary change: preventing delegated scan hangs when Scanner V2 is unavailable.
Description check	✅ Passed	The description explains the problem, the Scanner V2 removal, the nil-pointer fix, testing coverage, and validation steps. The CI inspection checkbox is not selected, although the validation section s…

✨ 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 dc/dele-scan-no-hang

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

codecov · 2026-09-14T23:14:06Z

Codecov Report

❌ Patch coverage is 35.71429% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.80%. Comparing base (e89ea29) to head (d46f942).
⚠️ Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
sensor/common/scannerclient/singleton.go	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #22836      +/-   ##
==========================================
- Coverage   51.83%   51.80%   -0.03%     
==========================================
  Files        2901     2901              
  Lines      182807   182763      -44     
==========================================
- Hits        94749    94681      -68     
- Misses      79770    79773       +3     
- Partials     8288     8309      +21

Flag	Coverage Δ
go-unit-tests	`51.80% <35.71%> (-0.03%)`	⬇️

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 · 2026-09-14T23:28:05Z

/konflux-retest checks

github-actions · 2026-09-14T23:40:30Z

/konflux-retest checks

github-actions · 2026-09-14T23:48:49Z

/konflux-retest checks

github-actions · 2026-09-15T13:37:22Z

🚀 Build Images Ready

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

export MAIN_IMAGE_TAG=5.0.x-298-gd46f9423ad

iamkirkbater · 2026-09-15T15:55:49Z

 	// Check if there is a local Scanner.
 	// No need to continue if there is no local Scanner.
-	if s.scannerClientSingleton() == nil {
+	scannerClient := s.scannerClientSingleton()


Looking at all of these changes in this file - is there actually any change happening here? If I'm reading this all correctly aren't both implementations doing the exact same thing.

I'll admit the new way feels cleaner and more readable (and doesn't rely on implicit global state) but I just want to make sure I'm not missing something :)

Oh, nvm - I see that this is the fix for the TOCTOU mentioned.

s.scannerClientSingleton() could return non-nil on the first invocation and nil on the second invocation in scanImage which would cause a panic. This PR re-uses the returned object to avoid that.

dcaravel added 2 commits September 14, 2026 17:52

remove v2 client that goes nowhere

b2a9a60

propagate scanner client to avoid nil pointer

5521eb0

A perhaps extreme edge case existed where the singleton would return a non-nil client at the scan entrypoint point and then return a nil client later on - causing a nil pointer exception and crash Sensor.

openshift-ci Bot added the do-not-merge/work-in-progress label Sep 14, 2026

github-actions Bot added area/sensor ai-review coderabbit-review labels Sep 14, 2026

add test for TOCTOU nil pointer

d46f942

iamkirkbater reviewed Sep 15, 2026

View reviewed changes

Navigation Menu

Uh oh!

ROX-36973: Prevent delegated scan hang when no Scanner V2 - #22836

ROX-36973: Prevent delegated scan hang when no Scanner V2#22836
dcaravel wants to merge 3 commits into
masterfrom
dc/dele-scan-no-hang

dcaravel commented Sep 14, 2026 •

edited

Loading

Uh oh!

openshift-ci Bot commented Sep 14, 2026

Uh oh!

coderabbitai Bot commented Sep 14, 2026 •

edited

Loading

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

codecov Bot commented Sep 14, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 15, 2026

Uh oh!

iamkirkbater Sep 15, 2026

Uh oh!

iamkirkbater Sep 15, 2026

Uh oh!

dcaravel Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

Description

User-facing documentation

Testing and quality

Automated testing

How I validated my change

Uh oh!

openshift-ci Bot commented Sep 14, 2026

Uh oh!

coderabbitai Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

codecov Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 15, 2026

🚀 Build Images Ready

Uh oh!

iamkirkbater Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

iamkirkbater Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

dcaravel Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dcaravel commented Sep 14, 2026 •

edited

Loading

coderabbitai Bot commented Sep 14, 2026 •

edited

Loading

codecov Bot commented Sep 14, 2026 •

edited

Loading