Skip to content

perf: reduce init-time memory via lazy loading and build tags#20047

Draft
davdhacs wants to merge 1 commit intomasterfrom
davdhacs/pr1f-init-reductions
Draft

perf: reduce init-time memory via lazy loading and build tags#20047
davdhacs wants to merge 1 commit intomasterfrom
davdhacs/pr1f-init-reductions

Conversation

@davdhacs
Copy link
Copy Markdown
Contributor

Description

Reduce init-time allocations by deferring computation to first use and excluding test-only code from production binaries.

Changes

  • pkg/booleanpolicy: lazy regexp compilation via sync.OnceValue, lazy evaluator.Factory
  • pkg/branding: lazy branded logo loading
  • pkg/cloudproviders/aws: lazy root CA pool construction
  • pkg/probeupload: lazy probe name regex
  • pkg/printers/table: moved to sub-package (breaks transitive import chain)
  • pkg/postgres/id: inline CVE ID utilities (avoids heavyweight pkg/cve import)
  • sensor/kubernetes/fake: //go:build fakeworkloads excludes fake workload generator from production binary
  • sensor/common/centralproxy: move test code to _test.go

Dependencies

Depends on #20024 (schema lazy loading — provides pkg/sync.OnceValue wrapper).

User-facing documentation

Testing and quality

  • the change is production ready
  • CI results are inspected

Automated testing

  • modified existing tests

How I validated my change

  • Full go build ./... passes
  • Deployed on live GKE test cluster in earlier combined testing

AI-assisted.

Reduce init-time allocations by deferring computation to first use:
- pkg/booleanpolicy: lazy regexp compilation via sync.OnceValue,
  lazy evaluator.Factory via lazyFactory struct
- pkg/branding: lazy branded logo loading
- pkg/cloudproviders/aws: lazy root CA pool construction
- pkg/probeupload: lazy probe name regex
- pkg/printers/table: moved to sub-package to break transitive
  import chain (roxctl → pkg/printers → tablewriter)
- pkg/postgres/id: inline CVE ID utilities (avoids heavyweight
  pkg/cve import chain)
- sensor/kubernetes/fake: //go:build fakeworkloads tag excludes
  fake workload generator from production binary
- sensor/common/centralproxy: move test code to _test.go

Depends on: #20024 (for pkg/sync.OnceValue wrapper)

AI-assisted.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 16, 2026

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
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 16, 2026

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Optimized resource initialization patterns through lazy loading to improve startup performance.
    • Reorganized internal package structure for table printing functionality.
    • Added conditional build constraints to workload-related features for flexible compilation.
    • Introduced fallback stub implementations for workload management when optional features are disabled.
    • Added composite primary key utility functions for database operations.

Walkthrough

This pull request introduces lazy initialization patterns using sync.OnceValue for regex compilation, certificate parsing, and factory construction across multiple packages. It reorganizes the table printer into its own package, adds build constraints to fake workload files, introduces stub implementations for non-fakeworkloads builds, and adds composite ID utilities and a generic sync helper.

Changes

Cohort / File(s) Summary
Regex Lazy Initialization
pkg/booleanpolicy/value_regex.go, pkg/booleanpolicy/field_metadata.go, pkg/booleanpolicy/validate_test.go, pkg/probeupload/names.go
Converted regex variables from eagerly compiled *regexp.Regexp to functions returning *regexp.Regexp via sync.OnceValue for lazy, cached compilation. Updated callsites to invoke regex as functions. Inlined signatureIntegrationIDPrefix constant.
Lazy Singleton Factories
pkg/booleanpolicy/matcher.go, pkg/cloudproviders/aws/certs.go, pkg/branding/brandedlogo.go
Replaced eager package-level initialization with sync.OnceValue/sync.Once-backed lazy factories for evaluator construction, certificate parsing, and logo base64 encoding. Moved initialization logic from init() or variable declaration into lazy closures.
Certificate Access Updates
pkg/cloudproviders/aws/metadata.go
Updated reference to awsCerts to invoke it as a function call awsCerts().
Table Printer Package Reorganization
pkg/printers/table/table.go, pkg/printers/table/table_test.go, roxctl/common/printer/tabularprinter_factory.go
Moved table printer into dedicated table package. Updated package declarations and adjusted imports in factory to use tabprinters from new location.
New Utilities
pkg/postgres/id/id.go, pkg/sync/common_aliases.go
Added composite primary key utilities (FromPks, ToPks) and generic OnceValue helper alias.
Build Constraints & Workload Stubs
sensor/kubernetes/fake/{common,deployment,fake,flows,labels,namespace,networkpolicy,nodes,openshift,pebble,pools,process,rbac,secret,service,virtualmachines,workload}.go, sensor/kubernetes/fake/{common_test,flows_test,virtualmachines_test}.go, sensor/kubernetes/fake/stub.go
Added //go:build fakeworkloads constraint to existing fake workload files. Introduced stub.go with no-op implementations under !fakeworkloads constraint for WorkloadManager and related types.
Minor Cleanup
sensor/common/centralproxy/testutils_test.go
Removed inline //nolint:wrapcheck suppression comment.

Sequence Diagram(s)

No sequence diagrams generated. The changes consist primarily of lazy initialization refactoring, build constraint additions, and package reorganization without significant new feature flow or multi-component interactions that would benefit from visualization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.19% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'perf: reduce init-time memory via lazy loading and build tags' clearly and concisely summarizes the main changes across the PR, capturing the performance-focused optimization of deferring initialization.
Description check ✅ Passed The PR description provides detailed change summaries for each major area modified, validation evidence (go build, GKE deployment), and acknowledges pending documentation items as unchecked rather than missing.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 davdhacs/pr1f-init-reductions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 2 issues, and left some high level feedback:

  • In sensor/kubernetes/fake/stub.go, returning nil from NewWorkloadManager/ConfigDefaults makes accidental use in non-fakeworkloads builds fail with nil dereferences; consider having these stubs panic (or otherwise clearly fail fast) so misconfiguration is detected immediately and is easier to diagnose.
  • In sensor/common/centralproxy/testutils_test.go, removing the //nolint:wrapcheck without wrapping the error means the linter will likely flag this again; either restore the nolint or wrap the underlying RoundTrip error with additional context.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In sensor/kubernetes/fake/stub.go, returning nil from NewWorkloadManager/ConfigDefaults makes accidental use in non-fakeworkloads builds fail with nil dereferences; consider having these stubs panic (or otherwise clearly fail fast) so misconfiguration is detected immediately and is easier to diagnose.
- In sensor/common/centralproxy/testutils_test.go, removing the `//nolint:wrapcheck` without wrapping the error means the linter will likely flag this again; either restore the nolint or wrap the underlying RoundTrip error with additional context.

## Individual Comments

### Comment 1
<location path="pkg/postgres/id/id.go" line_range="10" />
<code_context>
+import "strings"
+
+// Separator is the separator used in IDs constructed from multiple primary keys.
+const Separator = "#"
+
+// FromPks creates a composite ID from multiple primary key values.
</code_context>
<issue_to_address>
**issue:** Clarify or guard against primary key values that may themselves contain the separator

Since composite IDs rely on `Separator = "#"` with `strings.Join`/`strings.Split`, any PK that can contain `"#"` will break round-tripping and produce ambiguous splits. If that’s possible now or in the future, consider either enforcing that `"#"` is disallowed in PKs used here, or switching to an unambiguous encoding (e.g., length-prefixing or base64-encoding each PK). A brief comment or validation around this invariant would help avoid subtle bugs later.
</issue_to_address>

### Comment 2
<location path="pkg/booleanpolicy/matcher.go" line_range="14" />
<code_context>
 	"github.com/stackrox/rox/pkg/booleanpolicy/query"
 )

+// lazyFactory defers evaluator.Factory creation until first use.
+type lazyFactory struct {
+	once sync.Once
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the custom lazyFactory wrapper with sync.OnceValue-based factory variables to keep lazy initialization while removing extra indirection and dead code.

You can simplify the lazy initialization without the extra `lazyFactory` abstraction and unused `GenerateEvaluator` method by using `sync.OnceValue` directly at the variable level (mirroring the regex pattern).

Instead of:

```go
type lazyFactory struct {
	once sync.Once
	meta *pathutil.AugmentedObjMeta
	val  evaluator.Factory
}

func newLazyFactory(meta *pathutil.AugmentedObjMeta) lazyFactory {
	return lazyFactory{meta: meta}
}

func (f *lazyFactory) get() *evaluator.Factory {
	f.once.Do(func() { f.val = evaluator.MustCreateNewFactory(f.meta) })
	return &f.val
}

func (f *lazyFactory) GenerateEvaluator(q *query.Query) (evaluator.Evaluator, error) {
	return f.get().GenerateEvaluator(q)
}

var (
	deploymentEvalFactory       = newLazyFactory(augmentedobjs.DeploymentMeta)
	deploymentFileAccessFactory = newLazyFactory(augmentedobjs.DeploymentFileAccessMeta)
	// ...
)
```

you can use:

```go
var (
	deploymentEvalFactory = sync.OnceValue(func() *evaluator.Factory {
		return evaluator.MustCreateNewFactory(augmentedobjs.DeploymentMeta)
	})
	deploymentFileAccessFactory = sync.OnceValue(func() *evaluator.Factory {
		return evaluator.MustCreateNewFactory(augmentedobjs.DeploymentFileAccessMeta)
	})
	// ...
)
```

and update call sites from:

```go
sectionsAndEvals, err := getSectionsAndEvals(deploymentEvalFactory.get(), p, storage.LifecycleStage_DEPLOY, options...)
```

to:

```go
sectionsAndEvals, err := getSectionsAndEvals(deploymentEvalFactory(), p, storage.LifecycleStage_DEPLOY, options...)
```

This keeps the lazy behavior and global factories exactly as before, but removes the custom type, the `get()` wrapper, and the now-unused `GenerateEvaluator` method, reducing indirection and making the usage clearer at call sites.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread pkg/postgres/id/id.go
import "strings"

// Separator is the separator used in IDs constructed from multiple primary keys.
const Separator = "#"
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.

issue: Clarify or guard against primary key values that may themselves contain the separator

Since composite IDs rely on Separator = "#" with strings.Join/strings.Split, any PK that can contain "#" will break round-tripping and produce ambiguous splits. If that’s possible now or in the future, consider either enforcing that "#" is disallowed in PKs used here, or switching to an unambiguous encoding (e.g., length-prefixing or base64-encoding each PK). A brief comment or validation around this invariant would help avoid subtle bugs later.

"github.com/stackrox/rox/pkg/booleanpolicy/query"
)

// lazyFactory defers evaluator.Factory creation until first use.
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.

issue (complexity): Consider replacing the custom lazyFactory wrapper with sync.OnceValue-based factory variables to keep lazy initialization while removing extra indirection and dead code.

You can simplify the lazy initialization without the extra lazyFactory abstraction and unused GenerateEvaluator method by using sync.OnceValue directly at the variable level (mirroring the regex pattern).

Instead of:

type lazyFactory struct {
	once sync.Once
	meta *pathutil.AugmentedObjMeta
	val  evaluator.Factory
}

func newLazyFactory(meta *pathutil.AugmentedObjMeta) lazyFactory {
	return lazyFactory{meta: meta}
}

func (f *lazyFactory) get() *evaluator.Factory {
	f.once.Do(func() { f.val = evaluator.MustCreateNewFactory(f.meta) })
	return &f.val
}

func (f *lazyFactory) GenerateEvaluator(q *query.Query) (evaluator.Evaluator, error) {
	return f.get().GenerateEvaluator(q)
}

var (
	deploymentEvalFactory       = newLazyFactory(augmentedobjs.DeploymentMeta)
	deploymentFileAccessFactory = newLazyFactory(augmentedobjs.DeploymentFileAccessMeta)
	// ...
)

you can use:

var (
	deploymentEvalFactory = sync.OnceValue(func() *evaluator.Factory {
		return evaluator.MustCreateNewFactory(augmentedobjs.DeploymentMeta)
	})
	deploymentFileAccessFactory = sync.OnceValue(func() *evaluator.Factory {
		return evaluator.MustCreateNewFactory(augmentedobjs.DeploymentFileAccessMeta)
	})
	// ...
)

and update call sites from:

sectionsAndEvals, err := getSectionsAndEvals(deploymentEvalFactory.get(), p, storage.LifecycleStage_DEPLOY, options...)

to:

sectionsAndEvals, err := getSectionsAndEvals(deploymentEvalFactory(), p, storage.LifecycleStage_DEPLOY, options...)

This keeps the lazy behavior and global factories exactly as before, but removes the custom type, the get() wrapper, and the now-unused GenerateEvaluator method, reducing indirection and making the usage clearer at call sites.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (1)
pkg/booleanpolicy/value_regex.go (1)

15-17: Keep the signature integration ID prefix in one place.

central/signatureintegration/datastore/validate.go still validates IDs against signatures.SignatureIntegrationIDPrefix, so this inline copy can drift and make policy validation disagree with the rest of the system about which IDs are valid. Moving the prefix to a lightweight shared package would keep the init-time win without introducing a second source of truth.

As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/booleanpolicy/value_regex.go` around lines 15 - 17, There is a duplicated
constant: remove the inline const signatureIntegrationIDPrefix from
pkg/booleanpolicy/value_regex.go and centralize the value into a new lightweight
shared package (e.g., a small pkg with exported name
SignatureIntegrationIDPrefix) so both callers use the same symbol; update
references in pkg/booleanpolicy/value_regex.go and
central/signatureintegration/datastore/validate.go to import that lightweight
package and use the exported SignatureIntegrationIDPrefix (ensure the new
package has no heavy init that would reintroduce the cosign/rekor import).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/booleanpolicy/matcher.go`:
- Around line 15-32: Replace the sync.Once + stored evaluator.Factory pattern
with sync.OnceValue to avoid marking done on panic: change lazyFactory to hold
val sync.OnceValue[evaluator.Factory] (remove once and the plain val field),
update get() to call f.val.Do(func() evaluator.Factory { return
evaluator.MustCreateNewFactory(f.meta) }) and return a pointer to the resulting
evaluator.Factory, and ensure GenerateEvaluator uses that get() result; keep
newLazyFactory and lazyFactory.GenerateEvaluator logic but adapt to the
OnceValue API so initialization is retried on panic rather than leaving a
zero-valued Factory.

In `@pkg/postgres/id/id.go`:
- Around line 13-20: FromPks and ToParts currently join/split raw PK strings
with Separator and will corrupt values that contain the separator (e.g., "a#b").
Fix by making FromPks encode each PK before joining and have ToParts decode each
part after splitting; for example, apply a reversible safe encoding such as
URL-safe base64 (base64url without padding) or percent-encoding to each element
in FromPks and decode in ToParts, ensuring round-trip integrity for any input;
alternatively (if you prefer validation) update FromPks to reject/return error
when any PK contains Separator. Update the implementations of FromPks, ToParts
and any callers that assume raw values, keeping Separator usage only as the
join/split delimiter for encoded parts.

In `@sensor/common/centralproxy/testutils_test.go`:
- Line 62: The test removes the wrapcheck suppression while the production
transport still uses it; restore consistency by adding the same
//nolint:wrapcheck comment to the return of m.base.RoundTrip(reqCopy) in
sensor/common/centralproxy/testutils_test.go (the RoundTrip wrapper that calls
m.base.RoundTrip) so the test matches the production transport behavior.

In `@sensor/kubernetes/fake/flows_test.go`:
- Line 1: The test file flows_test.go is gated by the build tag "//go:build
fakeworkloads" so CI never runs it; either add GOTAGS=fakeworkloads to the
unit-tests matrix in .github/workflows/unit-tests.yaml so these tests run in CI,
or remove the "//go:build fakeworkloads" tag (and any matching "+build" lines)
from sensor/kubernetes/fake/flows_test.go to make the tests run under the
existing GOTAGS; update whichever option you choose and ensure the rest of the
fake workload test files use the same approach.

In `@sensor/kubernetes/fake/virtualmachines_test.go`:
- Line 1: The test file virtualmachines_test.go is build-tagged with //go:build
fakeworkloads and is never executed in CI; update CI to run these tests by
either adding a new CI step that executes go test -tags fakeworkloads
./sensor/kubernetes/fake/... or modify the existing go-unit-tests target to
include GOTAGS=fakeworkloads so the sensor/kubernetes/fake package is tested,
alternatively add a CI entry to run the go test for that package path
specifically or document why the fakeworkloads-tagged tests are intentionally
excluded.

---

Nitpick comments:
In `@pkg/booleanpolicy/value_regex.go`:
- Around line 15-17: There is a duplicated constant: remove the inline const
signatureIntegrationIDPrefix from pkg/booleanpolicy/value_regex.go and
centralize the value into a new lightweight shared package (e.g., a small pkg
with exported name SignatureIntegrationIDPrefix) so both callers use the same
symbol; update references in pkg/booleanpolicy/value_regex.go and
central/signatureintegration/datastore/validate.go to import that lightweight
package and use the exported SignatureIntegrationIDPrefix (ensure the new
package has no heavy init that would reintroduce the cosign/rekor import).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: 8bf54ead-ab23-4fb9-8819-30b08739f814

📥 Commits

Reviewing files that changed from the base of the PR and between 237be37 and dc8d15b.

📒 Files selected for processing (35)
  • pkg/booleanpolicy/field_metadata.go
  • pkg/booleanpolicy/matcher.go
  • pkg/booleanpolicy/validate_test.go
  • pkg/booleanpolicy/value_regex.go
  • pkg/branding/brandedlogo.go
  • pkg/cloudproviders/aws/certs.go
  • pkg/cloudproviders/aws/metadata.go
  • pkg/postgres/id/id.go
  • pkg/printers/table/table.go
  • pkg/printers/table/table_test.go
  • pkg/probeupload/names.go
  • pkg/sync/common_aliases.go
  • roxctl/common/printer/tabularprinter_factory.go
  • sensor/common/centralproxy/testutils_test.go
  • sensor/kubernetes/fake/common.go
  • sensor/kubernetes/fake/common_test.go
  • sensor/kubernetes/fake/deployment.go
  • sensor/kubernetes/fake/fake.go
  • sensor/kubernetes/fake/flows.go
  • sensor/kubernetes/fake/flows_test.go
  • sensor/kubernetes/fake/labels.go
  • sensor/kubernetes/fake/namespace.go
  • sensor/kubernetes/fake/networkpolicy.go
  • sensor/kubernetes/fake/nodes.go
  • sensor/kubernetes/fake/openshift.go
  • sensor/kubernetes/fake/pebble.go
  • sensor/kubernetes/fake/pools.go
  • sensor/kubernetes/fake/process.go
  • sensor/kubernetes/fake/rbac.go
  • sensor/kubernetes/fake/secret.go
  • sensor/kubernetes/fake/service.go
  • sensor/kubernetes/fake/stub.go
  • sensor/kubernetes/fake/virtualmachines.go
  • sensor/kubernetes/fake/virtualmachines_test.go
  • sensor/kubernetes/fake/workload.go

Comment on lines +15 to +32
type lazyFactory struct {
once sync.Once
meta *pathutil.AugmentedObjMeta
val evaluator.Factory
}

func newLazyFactory(meta *pathutil.AugmentedObjMeta) lazyFactory {
return lazyFactory{meta: meta}
}

func (f *lazyFactory) get() *evaluator.Factory {
f.once.Do(func() { f.val = evaluator.MustCreateNewFactory(f.meta) })
return &f.val
}

func (f *lazyFactory) GenerateEvaluator(q *query.Query) (evaluator.Evaluator, error) {
return f.get().GenerateEvaluator(q)
}
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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Inspecting evaluator.Factory construction and use..."
rg -n -C3 'type Factory struct|func .*MustCreateNewFactory|func .*CreateNewFactory|func .*GenerateEvaluator' pkg/booleanpolicy/evaluator

echo
echo "Looking for assumptions that zero-value Factory is valid or invalid..."
rg -n -C3 'panic|nil|initialized|zero value|GenerateEvaluator' pkg/booleanpolicy/evaluator

Repository: stackrox/stackrox

Length of output: 50374


🏁 Script executed:

#!/bin/bash

# Check Go version in the repository
go version 2>/dev/null || echo "Go version check not available"

# Look for any error handling around lazyFactory usage
rg -n "lazyFactory" pkg/booleanpolicy/

# Check if there's error handling for Factory creation or use
rg -n -A5 "MustCreateNewFactory" pkg/booleanpolicy/ | head -50

# Check if Factory fields are accessed in a way that requires initialization
rg -n "\.fieldToMetaPaths|\.rootType" pkg/booleanpolicy/evaluator/evaluator.go

Repository: stackrox/stackrox

Length of output: 2295


🏁 Script executed:

#!/bin/bash

# Let's verify Go 1.26's sync.Once behavior with panic
# Check the Go source/documentation or create a small test

cat > /tmp/test_once_panic.go << 'EOF'
package main

import (
	"fmt"
	"sync"
)

var once sync.Once
var counter = 0

func main() {
	// First call - should panic
	fmt.Println("Call 1:")
	func() {
		defer func() {
			if r := recover(); r != nil {
				fmt.Println("  Caught panic:", r)
			}
		}()
		once.Do(func() {
			counter++
			panic("test panic")
		})
	}()

	// Second call - should retry if Once wasn't marked done
	fmt.Println("Call 2:")
	func() {
		defer func() {
			if r := recover(); r != nil {
				fmt.Println("  Caught panic:", r)
			}
		}()
		once.Do(func() {
			counter++
			fmt.Println("  Executing once.Do again, counter:", counter)
		})
	}()

	fmt.Println("Final counter:", counter)
	fmt.Println("If counter is 2, Once was NOT marked done on panic")
	fmt.Println("If counter is 1, Once WAS marked done on panic")
}
EOF

go run /tmp/test_once_panic.go 2>&1

Repository: stackrox/stackrox

Length of output: 218


🏁 Script executed:

#!/bin/bash

# Check what errors NewFactory can return
rg -n -A10 "^func NewFactory" pkg/booleanpolicy/evaluator/evaluator.go

# Check what happens when fieldToMetaPaths is nil
rg -n -B2 -A2 "fieldToMetaPaths\.Get" pkg/booleanpolicy/evaluator/

Repository: stackrox/stackrox

Length of output: 990


🏁 Script executed:

#!/bin/bash

# Check what utils.CrashOnError does
rg -n -A5 "func CrashOnError" 

# Verify that accessing nil.Get panics or errors
cat > /tmp/test_nil_map.go << 'EOF'
package main

import (
	"fmt"
)

type MetaPathMap struct {
	underlying map[string]interface{}
}

func (m *MetaPathMap) Get(key string) (interface{}, bool) {
	if m == nil {
		panic("cannot call Get on nil MetaPathMap")
	}
	val, found := m.underlying[key]
	return val, found
}

func main() {
	var nilMap *MetaPathMap
	defer func() {
		if r := recover(); r != nil {
			fmt.Println("Panic caught:", r)
		}
	}()
	nilMap.Get("test")
}
EOF

go run /tmp/test_nil_map.go 2>&1

Repository: stackrox/stackrox

Length of output: 367


Use sync.OnceValue to preserve fail-fast initialization semantics.

sync.Once has a subtle issue in Go 1.26: when the initializer panics, the Once is still marked done, leaving f.val zero-valued. Subsequent calls then return this uninitialized Factory, which panics later when accessing nil fieldToMetaPaths, masking the original initialization failure. sync.OnceValue[evaluator.Factory] avoids this by not marking done on panic—retrying on each call until successful. This preserves fail-fast behavior and prevents silent initialization failures.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/booleanpolicy/matcher.go` around lines 15 - 32, Replace the sync.Once +
stored evaluator.Factory pattern with sync.OnceValue to avoid marking done on
panic: change lazyFactory to hold val sync.OnceValue[evaluator.Factory] (remove
once and the plain val field), update get() to call f.val.Do(func()
evaluator.Factory { return evaluator.MustCreateNewFactory(f.meta) }) and return
a pointer to the resulting evaluator.Factory, and ensure GenerateEvaluator uses
that get() result; keep newLazyFactory and lazyFactory.GenerateEvaluator logic
but adapt to the OnceValue API so initialization is retried on panic rather than
leaving a zero-valued Factory.

Comment thread pkg/postgres/id/id.go
Comment on lines +13 to +20
func FromPks(pks []string) string {
return strings.Join(pks, Separator)
}

// ToParts splits a composite ID into its primary key parts.
func ToParts(id string) []string {
return strings.Split(id, Separator)
}
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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "1) Locate all call sites of FromPks/ToParts:"
rg -n --type=go '\b(FromPks|ToParts)\s*\('

echo
echo "2) For each call site, inspect nearby PK sources/validators for separator constraints:"
rg -n --type=go -C3 'FromPks\(|ToParts\(|PrimaryKey|primary key|pk|ID|Id'

echo
echo "3) Find any existing invariant checks that disallow '#':"
rg -n --type=go 'Contains\(.+Separator|Contains\(.+"#"'

Repository: stackrox/stackrox

Length of output: 50376


🏁 Script executed:

cat -n pkg/postgres/id/id.go

Repository: stackrox/stackrox

Length of output: 890


🏁 Script executed:

# Search for imports of the id package to confirm no usage
rg -n "pkg/postgres/id|from github.com/stackrox/rox/pkg/postgres/id" --type=go

Repository: stackrox/stackrox

Length of output: 43


🏁 Script executed:

# Search for any imports of the id package more broadly
rg -n 'import.*".*postgres/id"' --type=go

Repository: stackrox/stackrox

Length of output: 43


Composite ID encoding lacks escape handling for PK values containing #.

The FromPks and ToParts functions will silently corrupt IDs if any primary key contains the separator. For example: FromPks(["a#b", "c"]) produces "a#b#c", which ToParts splits back to ["a", "b", "c"] instead of ["a#b", "c"]. This breaks round-trip integrity and record addressing.

Implement a safe encoding strategy (e.g., URL encoding, base64, length-prefixed format) before joining, or validate/reject PKs containing the separator at the creation boundary.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/postgres/id/id.go` around lines 13 - 20, FromPks and ToParts currently
join/split raw PK strings with Separator and will corrupt values that contain
the separator (e.g., "a#b"). Fix by making FromPks encode each PK before joining
and have ToParts decode each part after splitting; for example, apply a
reversible safe encoding such as URL-safe base64 (base64url without padding) or
percent-encoding to each element in FromPks and decode in ToParts, ensuring
round-trip integrity for any input; alternatively (if you prefer validation)
update FromPks to reject/return error when any PK contains Separator. Update the
implementations of FromPks, ToParts and any callers that assume raw values,
keeping Separator usage only as the join/split delimiter for encoded parts.

reqCopy := req.Clone(req.Context())
reqCopy.Header.Set("Authorization", fmt.Sprintf("Bearer %s", m.token))
return m.base.RoundTrip(reqCopy) //nolint:wrapcheck
return m.base.RoundTrip(reqCopy)
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.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check if wrapcheck linter is configured and would flag this line

# Check if golangci-lint config exists and includes wrapcheck
if [ -f .golangci.yml ] || [ -f .golangci.yaml ]; then
  echo "=== Checking for wrapcheck linter configuration ==="
  rg -i "wrapcheck" .golangci.yml .golangci.yaml 2>/dev/null || echo "wrapcheck not found in config"
fi

# Search for other wrapcheck suppressions in the codebase to understand the pattern
echo ""
echo "=== Other wrapcheck suppressions in codebase ==="
rg "nolint:wrapcheck" --type go -C 2

Repository: stackrox/stackrox

Length of output: 5174


Align suppression comment usage with production code.

The //nolint:wrapcheck suppression was removed from this line, but the identical pattern in the production version (sensor/common/centralproxy/transport.go line ~113) still retains it. Since wrapcheck is configured and enabled in .golangci.yml, either the test code should keep the suppression to match production, or the production code should be updated to align if the suppression is no longer needed. Ensure consistency across the transport and test utilities.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@sensor/common/centralproxy/testutils_test.go` at line 62, The test removes
the wrapcheck suppression while the production transport still uses it; restore
consistency by adding the same //nolint:wrapcheck comment to the return of
m.base.RoundTrip(reqCopy) in sensor/common/centralproxy/testutils_test.go (the
RoundTrip wrapper that calls m.base.RoundTrip) so the test matches the
production transport behavior.

@@ -1,3 +1,5 @@
//go:build fakeworkloads
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.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify whether repository automation references fakeworkloads-tagged test execution.
# Expected result: at least one workflow/script/Make target includes go test with fakeworkloads.
rg -n -C2 -P 'fakeworkloads|go\s+test[^\n]*-tags(?:=|\s+)[^\n]*fakeworkloads'

Repository: stackrox/stackrox

Length of output: 4340


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find CI configuration files
echo "=== Looking for CI configuration files ==="
find . -type f \( -name "Makefile" -o -name "*.yml" -o -name "*.yaml" -o -name ".circleci" \) | head -20

# Search GitHub workflows specifically
echo "=== GitHub workflow files ==="
find .github/workflows -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -20

# Look for Makefile targets related to testing
echo "=== Makefile test targets ==="
fd -t f "Makefile" | xargs grep -l "test" 2>/dev/null | head -5

Repository: stackrox/stackrox

Length of output: 983


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search GitHub workflows for fakeworkloads tag in test execution
echo "=== Searching GitHub workflows for fakeworkloads ==="
grep -r "fakeworkloads" .github/workflows/ 2>/dev/null || echo "No matches found in workflows"

# Search for go test commands in workflows
echo -e "\n=== Searching for 'go test' patterns in workflows ==="
grep -r "go.*test" .github/workflows/ -A2 -B2 2>/dev/null | head -100

# Check build.yaml specifically
echo -e "\n=== Checking build.yaml for test targets ==="
cat .github/workflows/build.yaml | grep -A10 "test" | head -50

Repository: stackrox/stackrox

Length of output: 7422


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Get full unit-tests.yaml to see matrix definition
echo "=== Full unit-tests.yaml (first 200 lines) ==="
head -200 .github/workflows/unit-tests.yaml

# Look for Makefile targets related to testing
echo -e "\n=== Makefile go-unit-tests target ==="
grep -A20 "^go-unit-tests" Makefile || echo "Not found in Makefile root"

# Search for Makefile in repository
echo -e "\n=== Finding Makefile files ==="
find . -name "Makefile" -type f | head -5

Repository: stackrox/stackrox

Length of output: 8000


Add fakeworkloads to CI test matrix or remove the build tag.

The //go:build fakeworkloads tag gates flows_test.go and related tests, but the CI matrix in .github/workflows/unit-tests.yaml only runs with GOTAGS="" and GOTAGS=release. Add GOTAGS=fakeworkloads to the matrix to exercise these tests in CI, or remove the build tag if they're meant for local development only.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@sensor/kubernetes/fake/flows_test.go` at line 1, The test file flows_test.go
is gated by the build tag "//go:build fakeworkloads" so CI never runs it; either
add GOTAGS=fakeworkloads to the unit-tests matrix in
.github/workflows/unit-tests.yaml so these tests run in CI, or remove the
"//go:build fakeworkloads" tag (and any matching "+build" lines) from
sensor/kubernetes/fake/flows_test.go to make the tests run under the existing
GOTAGS; update whichever option you choose and ensure the rest of the fake
workload test files use the same approach.

@@ -1,3 +1,5 @@
//go:build fakeworkloads
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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look for fakeworkloads-tagged Go invocations in CI/build config.
rg -n --hidden \
  --glob '.github/workflows/**' \
  --glob 'Makefile*' \
  --glob '**/*.mk' \
  --glob '**/Jenkinsfile*' \
  --glob '**/*.yaml' \
  --glob '**/*.yml' \
  'go (test|build).*(-tags[ =][^\\n]*fakeworkloads)|fakeworkloads'

Repository: stackrox/stackrox

Length of output: 43


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1. Search for any Go test invocations in CI/build files (broader)
echo "=== Go test commands in CI/build config ==="
rg -n --hidden \
  --glob '.github/**' \
  --glob 'Makefile*' \
  --glob '**/*.mk' \
  'go test' \
  | head -20

# 2. Search for any mention of fakeworkloads anywhere in the repo
echo -e "\n=== All mentions of fakeworkloads ==="
rg -n --type go 'fakeworkloads' | head -30

# 3. Check if virtualmachines_test.go file exists and is newly tagged
echo -e "\n=== virtualmachines_test.go file check ==="
find . -name 'virtualmachines_test.go' -type f 2>/dev/null | head -5

# 4. Look for any CI build/test steps that run without tag restrictions
echo -e "\n=== CI files present ==="
find .github/workflows -type f -name '*.yaml' -o -name '*.yml' 2>/dev/null | head -10

Repository: stackrox/stackrox

Length of output: 3714


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1. Search for sensor package test invocations in CI
echo "=== Sensor package tests in CI ==="
rg -n --hidden \
  --glob '.github/**' \
  'sensor' | grep -i 'test\|build' | head -20

# 2. Check git history for this specific file to see if tag is new or pre-existing
echo -e "\n=== Git log for virtualmachines_test.go (limited) ==="
git log --oneline sensor/kubernetes/fake/virtualmachines_test.go 2>/dev/null | head -10 || echo "Git history unavailable or file newly added"

# 3. Check if stub pattern was already present (examine stub.go more closely)
echo -e "\n=== Stub implementation pattern ==="
cat -n sensor/kubernetes/fake/stub.go | head -30

# 4. Look for build constraints in other test files in same package
echo -e "\n=== Build constraints in sensor/kubernetes/fake/ ==="
rg -n '^//' sensor/kubernetes/fake/ | grep -E 'build|go:' | head -20

Repository: stackrox/stackrox

Length of output: 5552


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the sensor-integration-test target in Makefile
echo "=== sensor-integration-test Makefile target ==="
rg -A 10 'sensor-integration-test:' Makefile

# Also check if there are any other references to fakeworkloads in build/test scripts
echo -e "\n=== fakeworkloads in scripts or configs (non-Go files) ==="
rg --type sh --type yaml 'fakeworkloads' 2>/dev/null | head -20

Repository: stackrox/stackrox

Length of output: 919


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check GOTAGS default value
echo "=== GOTAGS definition in Makefile ==="
rg -B 2 -A 2 'GOTAGS\s*=' Makefile | head -30

# Check if any CI command runs sensor/kubernetes/fake tests
echo -e "\n=== CI test paths ==="
rg 'go test.*sensor' .github/workflows/*.yaml | head -20

# Check the actual go-test.sh script to see how it handles tags
echo -e "\n=== go-test.sh script ==="
if [ -f scripts/go-test.sh ]; then
  head -50 scripts/go-test.sh
fi

Repository: stackrox/stackrox

Length of output: 2453


Ensure fakeworkloads-tagged tests run in CI.

This test file is excluded from default go test ./... runs due to the //go:build fakeworkloads constraint. The go-unit-tests target discovers this test file but skips it because GOTAGS does not include fakeworkloads, and the sensor-integration-test runs a different test path (./sensor/tests/...). Without explicit CI coverage, this test codepath remains unexecuted.

Add a CI step or extend an existing one to run tests with -tags fakeworkloads for the sensor/kubernetes/fake/... package, or document why this test is intentionally excluded from coverage.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@sensor/kubernetes/fake/virtualmachines_test.go` at line 1, The test file
virtualmachines_test.go is build-tagged with //go:build fakeworkloads and is
never executed in CI; update CI to run these tests by either adding a new CI
step that executes go test -tags fakeworkloads ./sensor/kubernetes/fake/... or
modify the existing go-unit-tests target to include GOTAGS=fakeworkloads so the
sensor/kubernetes/fake package is tested, alternatively add a CI entry to run
the go test for that package path specifically or document why the
fakeworkloads-tagged tests are intentionally excluded.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 16, 2026

🚀 Build Images Ready

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

export MAIN_IMAGE_TAG=4.11.x-660-gdc8d15b4c0

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 16, 2026

Codecov Report

❌ Patch coverage is 78.98551% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.71%. Comparing base (4881eec) to head (dc8d15b).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
sensor/kubernetes/fake/stub.go 0.00% 13 Missing ⚠️
pkg/booleanpolicy/field_metadata.go 90.12% 8 Missing ⚠️
pkg/cloudproviders/aws/certs.go 0.00% 3 Missing ⚠️
roxctl/common/printer/tabularprinter_factory.go 0.00% 3 Missing ⚠️
pkg/booleanpolicy/matcher.go 93.33% 1 Missing ⚠️
pkg/cloudproviders/aws/metadata.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20047      +/-   ##
==========================================
+ Coverage   49.61%   49.71%   +0.09%     
==========================================
  Files        2765     2749      -16     
  Lines      208643   206632    -2011     
==========================================
- Hits       103520   102727     -793     
+ Misses      97467    96258    -1209     
+ Partials     7656     7647       -9     
Flag Coverage Δ
go-unit-tests 49.71% <78.98%> (+0.09%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

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.

1 participant