Skip to content

fix: persist terminal status on generic SBOM-generation failures to stop reprocessing loop #855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
matthyx merged 4 commits into from
Jul 20, 2026
Merged

fix: persist terminal status on generic SBOM-generation failures to stop reprocessing loop #855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b3170c
fix: persist terminal status on generic SBOM-generation failures to s…
matthyx Jul 20, 2026
db239df
fix: never overwrite existing SBOM content on a failed reprocess
matthyx Jul 20, 2026
d95955c
fix: mark SBOM terminal status via annotation-only patch, not full re…
matthyx Jul 20, 2026
a5c562b
fix: never mark a content-bearing SBOM TooLarge, a storage-layer one-…
matthyx Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 120 additions & 37 deletions pkg/sbommanager/v1/sbom_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"fmt"
"maps"
"net"
"os"
"path/filepath"
Expand All @@ -27,6 +28,7 @@ import (
mapset "github.com/deckarep/golang-set/v2"
"github.com/distribution/distribution/reference"
"github.com/google/go-containerregistry/pkg/name"
"github.com/hashicorp/golang-lru/v2/expirable"
containercollection "github.com/inspektor-gadget/inspektor-gadget/pkg/container-collection"
"github.com/kubescape/go-logger"
"github.com/kubescape/go-logger/helpers"
Expand Down Expand Up @@ -62,6 +64,8 @@ const (
maxScanRetries = 3
scannerReadinessCheckInterval = 5 * time.Second
maxPendingScans = 1000
maxFailureRetryEntries = 1000
failureRetryTTL = 30 * time.Minute
)

// pendingScan holds the data needed to retry a container scan after the sidecar becomes ready.
Expand All @@ -88,11 +92,18 @@ type SbomManager struct {
scannerClient sbomscanner.SBOMScannerClient
scannerMemLimit int64
scanRetries map[string]int // safe without mutex: only accessed from pool workers (pool size 1)
pendingScans map[string]pendingScan
pendingOrder []string
pendingMu sync.Mutex
failureReporter sbommanager.SbomFailureReporter
metrics metricsmanager.MetricsManager
// failureRetries tracks consecutive generic SBOM-generation failures per sbomName. Bounded
// + TTL'd so short-lived images don't leak entries; a side effect is that the count resets
// if two failures for the same image are spaced more than failureRetryTTL apart, so it
// bounds retries for a tight failure cadence (e.g. a crash loop) rather than every possible
// one -- a slow-cadence permanent failure (long-lived pod, infrequent CronJob) can still
// reprocess indefinitely without ever accumulating enough consecutive failures to pin.
failureRetries *expirable.LRU[string, int]

@matthyx matthyx Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The bounded + TTL'd LRU is a good call, and I appreciate that the TTL's downside is documented honestly rather than papered over — a slow-cadence permanent failure (long-lived pod, infrequent CronJob) never accumulating maxScanRetries consecutive failures within 30 minutes is a real hole, and saying so here is much better than silently pretending the loop is bounded in all cases.

Non-blocking thought for later: if that hole ever bites, persisting the attempt count as an annotation (alongside the status) would make the budget survive both the TTL and a node-agent restart, at the cost of one extra patch per failure. Not worth doing now — just noting where the ceiling is.

pendingScans map[string]pendingScan
pendingOrder []string
pendingMu sync.Mutex
failureReporter sbommanager.SbomFailureReporter
metrics metricsmanager.MetricsManager
}

var _ sbommanager.SbomManagerClient = (*SbomManager)(nil)
Expand Down Expand Up @@ -141,6 +152,7 @@ func CreateSbomManager(ctx context.Context, cfg config.Config, socketPath string
scannerClient: scannerClient,
scannerMemLimit: scannerMemLimit,
scanRetries: make(map[string]int),
failureRetries: expirable.NewLRU[string, int](maxFailureRetryEntries, nil, failureRetryTTL),
pendingScans: make(map[string]pendingScan),
failureReporter: failureReporter,
metrics: metrics,
Expand Down Expand Up @@ -283,6 +295,15 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub
},
}
wipSbom, err = s.storageClient.CreateSBOM(wipSbom)
// wipSbomHadContent is true only when we're about to reprocess an SBOM that previously
// completed successfully (the Learning case below). It exists solely to keep a
// content-bearing SBOM from ever being marked TooLarge on the reprocess path: unlike
// Incomplete, TooLarge is a one-way door in the storage layer -- GuaranteedUpdate
// silently drops every future write once status=too-large is set, so persisting it here
// (with the real Spec still attached, since PatchSBOMAnnotations never clears it) would
// leave the SBOM permanently frozen with its old content, unfixable by any later version.
// Incomplete has no such short-circuit and stays safely retryable, so it's used instead.
var wipSbomHadContent bool
switch {
case k8serrors.IsAlreadyExists(err):
// get the existing SBOM metadata and check if it is ready or being processed by another node
Expand Down Expand Up @@ -316,24 +337,19 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub
helpers.String("nodeName", wipSbom.Annotations[NodeNameMetadataKey]))
return
case wipSbom.Annotations[helpersv1.StatusMetadataKey] == helpersv1.Learning:
// only skip if the SBOM was created with the same version of tool
if wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] == s.version {
logger.L().Debug("SbomManager - SBOM is already created, skipping",
helpers.String("namespace", notif.Container.K8s.Namespace),
helpers.String("pod", notif.Container.K8s.PodName),
helpers.String("container", notif.Container.K8s.ContainerName),
helpers.String("sbomName", sbomName))
if !s.shouldRetryAtCurrentVersion(wipSbom, sbomName, notif,
"SBOM is already created, skipping",
"SBOM was created with an different version of tool, recreating it") {
return
}
wipSbomHadContent = true
// continue to create SBOM
case wipSbom.Annotations[helpersv1.StatusMetadataKey] == helpersv1.Incomplete:

@matthyx matthyx Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Blocker (with the comment on markSBOMStatus) — pinning is terminal for far too long, and there's no escape hatch.

The only retry gate here is the tool version. Compare the TooLarge case just above, which has a second escape hatch (ScannerMemoryLimitAnnotation change → retry). Incomplete has none, so an image that failed for a purely environmental reason — registry 503, sidecar restart, temporary MaxImageSize/config value, node pressure — is skipped on every subsequent container start until someone ships a new node-agent version. With the spec-wipe above, that also means no CVE data for that image for the whole window.

The PR description flags this trade-off as acceptable, but I don't think it is at n=1 with no backoff, given the blast radius is "no vulnerability scanning for this image". Options:

  • Reuse the existing scanRetries/maxScanRetries pattern (already in handleScannerCrash) and only pin to Incomplete after N consecutive failures. This is the smallest change that keeps the loop bounded and keeps transient failures self-healing.
  • Or record a retry-after / attempt-count annotation and gate the skip on it, mirroring how ScannerMemoryLimitAnnotation gates the TooLarge skip.

Either way the "stop unbounded reprocessing" goal is met without making one bad minute permanent.

Nits while here:

  • This case is a near-verbatim copy of the Learning case above (~20 lines). Worth collapsing to case status == helpersv1.Learning || status == helpersv1.Incomplete: with the log message varying, or extracting a small shouldRetryAtCurrentVersion helper.
  • The stray // continue to create SBOM comment now sits between the Learning case body and this case line, so it reads as if it belongs to the new case. Move it back inside the Learning case.

@matthyx matthyx Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in db239df2:

  • Bounded retry: added a failureRetries map[string]int counter (separate from scanRetries, which handleScannerCrash already owns for OOM crashes specifically) and a new handleGenericFailure helper. A content-safe (no prior content) generic failure now only gets pinned to Incomplete after maxScanRetries (3) consecutive failures — reused the existing constant/pattern as you suggested, rather than inventing a new one. Failures below that threshold report but don't touch storage, so a transient blip self-heals on the next container start. The counter resets on success and (implicitly) whenever a version bump gives an Incomplete image a fresh budget.
  • Style nits: extracted shouldRetryAtCurrentVersion(wipSbom, sbomName, notif, skipMsg, retryMsg) bool, now shared verbatim by both the Learning and Incomplete cases — eliminates the ~20-line duplication rather than just collapsing the case condition. The stray // continue to create SBOM comment placement is fixed as a side effect of the refactor (each case now owns its own trailing comment unambiguously).

Test coverage for the bounded-retry semantics is in Test_processContainerWithMetadata_IncompleteReprocessing (rewritten to assert maxScanRetries-1 failures don't touch storage, the maxScanRetries-th does, and a version bump grants a fresh budget rather than re-pinning immediately).

if !s.shouldRetryAtCurrentVersion(wipSbom, sbomName, notif,
"SBOM generation previously failed with this tool version, skipping",
"SBOM generation previously failed with a different tool version, retrying") {
return
}
logger.L().Debug("SbomManager - SBOM was created with an different version of tool, recreating it",
helpers.String("namespace", notif.Container.K8s.Namespace),
helpers.String("pod", notif.Container.K8s.PodName),
helpers.String("container", notif.Container.K8s.ContainerName),
helpers.String("sbomName", sbomName),
helpers.String("got version", wipSbom.Annotations[helpersv1.ToolVersionMetadataKey]),
helpers.String("expected version", s.version))
// update the version of the tool
wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] = s.version
// continue to create SBOM
case wipSbom.Annotations[NodeNameMetadataKey] != s.cfg.NodeName:
logger.L().Debug("SbomManager - SBOM is already being processed by another node, skipping",
Expand Down Expand Up @@ -410,7 +426,7 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub
s.metrics.ObserveSBOMScanDuration("oom_killed", scanDuration)
s.metrics.ReportSBOMScannerRestart()
s.metrics.SetSBOMScannerReady(false)
s.handleScannerCrash(sbomName, wipSbom, notif, scanErr, imageTag, imageID)
s.handleScannerCrash(sbomName, notif, scanErr, imageTag, imageID, wipSbomHadContent)
return
}
s.metrics.ReportSBOMScan("error")
Expand All @@ -421,6 +437,7 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub
helpers.String("pod", notif.Container.K8s.PodName),
helpers.String("container", notif.Container.K8s.ContainerName),
helpers.String("sbomName", sbomName))
s.handleGenericFailure(sbomName)
s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonSBOMGenerationFailed, scanErr)
return
}
Expand Down Expand Up @@ -464,15 +481,16 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub
helpers.String("container", notif.Container.K8s.ContainerName),
helpers.String("sbomName", sbomName))
if errors.Is(srcErr, syftutil.ErrImageTooLarge) {
delete(wipSbom.Annotations, NodeNameMetadataKey)
wipSbom.Annotations[helpersv1.StatusMetadataKey] = helpersv1.TooLarge
if _, replaceErr := s.storageClient.ReplaceSBOM(wipSbom); replaceErr != nil {
logger.L().Ctx(s.ctx).Error("SbomManager - failed to persist TooLarge SBOM",
helpers.Error(replaceErr),
helpers.String("sbomName", sbomName))
if wipSbomHadContent {
// don't let a content-bearing SBOM reach the TooLarge one-way door; treat
// it as a generic (retryable, eventually Incomplete) failure instead.
s.handleGenericFailure(sbomName)
} else {
s.markSBOMStatus(sbomName, helpersv1.TooLarge, nil)
}
s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonImageTooLarge, srcErr)
} else {
s.handleGenericFailure(sbomName)
s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonSBOMGenerationFailed, srcErr)
}
return
Expand All @@ -498,6 +516,7 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub
helpers.String("pod", notif.Container.K8s.PodName),
helpers.String("container", notif.Container.K8s.ContainerName),
helpers.String("sbomName", sbomName))
s.handleGenericFailure(sbomName)
s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonSBOMGenerationFailed, syftErr)
return
}
Expand All @@ -506,6 +525,7 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub
}

// prepare the SBOM
s.failureRetries.Remove(sbomName)
delete(wipSbom.Annotations, NodeNameMetadataKey)
wipSbom.Spec.Metadata.Report.CreatedAt = wipSbom.CreationTimestamp
wipSbom.Spec.Metadata.Tool.Name = "syft"
Expand Down Expand Up @@ -557,7 +577,12 @@ func (s *SbomManager) waitForSharedContainerData(containerID string) (*objectcac
}, backoff.WithBackOff(backoff.NewExponentialBackOff()))
}

func (s *SbomManager) handleScannerCrash(sbomName string, wipSbom *v1beta1.SBOMSyft, notif containercollection.PubSubEvent, scanErr error, imageTag, imageID string) {
// handleScannerCrash responds to repeated sidecar OOM crashes while scanning the same image.
// hadContent must be true when the SBOM being reprocessed previously completed successfully
// (see the wipSbomHadContent doc comment in processContainerWithMetadata) -- in that case the
// terminal status is Incomplete rather than TooLarge, since TooLarge is a one-way door in the
// storage layer that would permanently freeze the SBOM's existing content.
func (s *SbomManager) handleScannerCrash(sbomName string, notif containercollection.PubSubEvent, scanErr error, imageTag, imageID string, hadContent bool) {
s.scanRetries[sbomName]++
retryCount := s.scanRetries[sbomName]

Expand All @@ -571,14 +596,12 @@ func (s *SbomManager) handleScannerCrash(sbomName string, wipSbom *v1beta1.SBOMS
helpers.Int("maxRetries", maxScanRetries))

if retryCount >= maxScanRetries {
delete(wipSbom.Annotations, NodeNameMetadataKey)
wipSbom.Annotations[helpersv1.StatusMetadataKey] = helpersv1.TooLarge
wipSbom.Annotations[ScannerMemoryLimitAnnotation] = fmt.Sprintf("%d", s.scannerMemLimit)
wipSbom.Spec = v1beta1.SBOMSyftSpec{}
if _, replaceErr := s.storageClient.ReplaceSBOM(wipSbom); replaceErr != nil {
logger.L().Error("SbomManager - failed to mark SBOM as TooLarge after scanner crashes",
helpers.Error(replaceErr),
helpers.String("sbomName", sbomName))
if hadContent {
s.markSBOMStatus(sbomName, helpersv1.Incomplete, nil)

@matthyx matthyx Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Small behavioural consequence worth knowing about, not a change request: this branch drops ScannerMemoryLimitAnnotation, which the TooLarge branch below still records. So a content-bearing SBOM that OOM-crash-looped is pinned Incomplete, and the Incomplete switch case gates only on tool version — raising the scanner memory limit will no longer make node-agent retry that image; only a version bump will.

That's still strictly better than before: the old TooLarge marking recorded the memory-limit hint, but the storage-layer short-circuit meant that even when the hint did trigger a retry, a successful rescan could never be persisted. So a recoverable-in-principle path became an actually-recoverable one. Just flagging it since "bump the sidecar memory limit to recover OOM-killed images" is presumably the documented operator workflow, and it now silently doesn't apply to this subset.

If you want them symmetric, passing the same map[string]any{ScannerMemoryLimitAnnotation: ...} here plus a memory-limit escape hatch in the Incomplete case (mirroring the one in the TooLarge case) would do it — but that's scope creep on an already-long PR and fine as a follow-up.

} else {
s.markSBOMStatus(sbomName, helpersv1.TooLarge, map[string]any{
ScannerMemoryLimitAnnotation: fmt.Sprintf("%d", s.scannerMemLimit),
})
}
// Report OOM regardless of persist success — the user should know the scan failed
s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonScannerOOMKilled, scanErr)
Expand Down Expand Up @@ -630,6 +653,66 @@ func (s *SbomManager) drainPendingScans() {
}
}

// markSBOMStatus persists the SBOM's terminal status (e.g. TooLarge, Incomplete) so a later
// container start for the same image is handled by the matching case in
// processContainerWithMetadata instead of retrying and failing indefinitely. It also records
// the currently-running tool version alongside the status, since that's what determined the
// outcome -- the Learning/Incomplete cases' version check relies on this being accurate.
func (s *SbomManager) markSBOMStatus(sbomName, status string, extraAnnotations map[string]any) {
annotations := map[string]any{
NodeNameMetadataKey: nil, // no longer owned by this node
helpersv1.StatusMetadataKey: status,
helpersv1.ToolVersionMetadataKey: s.version,
}
maps.Copy(annotations, extraAnnotations)
if _, err := s.storageClient.PatchSBOMAnnotations(sbomName, annotations); err != nil {
logger.L().Ctx(s.ctx).Error("SbomManager - failed to persist SBOM status",
helpers.Error(err),
helpers.String("sbomName", sbomName),
helpers.String("status", status))
}
}

// shouldRetryAtCurrentVersion checks a status-gated SBOM's recorded tool version against the
// running version. If they match, it logs skipMsg and returns false (the caller should skip
// reprocessing). Otherwise it logs retryMsg, updates the tool-version annotation, and returns
// true (the caller should continue to reprocess).
func (s *SbomManager) shouldRetryAtCurrentVersion(wipSbom *v1beta1.SBOMSyft, sbomName string, notif containercollection.PubSubEvent, skipMsg, retryMsg string) bool {
if wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] == s.version {
logger.L().Debug(skipMsg,
helpers.String("namespace", notif.Container.K8s.Namespace),
helpers.String("pod", notif.Container.K8s.PodName),
helpers.String("container", notif.Container.K8s.ContainerName),
helpers.String("sbomName", sbomName))
return false
}
logger.L().Debug(retryMsg,
helpers.String("namespace", notif.Container.K8s.Namespace),
helpers.String("pod", notif.Container.K8s.PodName),
helpers.String("container", notif.Container.K8s.ContainerName),
helpers.String("sbomName", sbomName),
helpers.String("got version", wipSbom.Annotations[helpersv1.ToolVersionMetadataKey]),
helpers.String("expected version", s.version))
wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] = s.version
return true
}

// handleGenericFailure responds to a non-deterministic SBOM-generation failure (source
// construction, syft cataloging, or sidecar scan error). markSBOMStatus only ever patches
// annotations, never Spec, so it's always safe to call regardless of whether the SBOM
// previously had real content -- but the image is only pinned Incomplete after
// maxScanRetries consecutive failures, so a single transient error doesn't lose coverage.
func (s *SbomManager) handleGenericFailure(sbomName string) {
count, _ := s.failureRetries.Get(sbomName)
count++
if count < maxScanRetries {
s.failureRetries.Add(sbomName, count)
return
}
s.failureRetries.Remove(sbomName)
s.markSBOMStatus(sbomName, helpersv1.Incomplete, nil)
}

// reportFailure sends a scan failure report to the backend via the failure reporter.
// Fire-and-forget: errors are logged, never propagated. Safe to call with nil reporter.
func (s *SbomManager) reportFailure(notif containercollection.PubSubEvent, imageTag, imageID, reason string, scanErr error) {
Expand Down
Loading
Loading