fix: persist terminal status on generic SBOM-generation failures to stop reprocessing loop - #855
Conversation
📝 WalkthroughWalkthroughSBOM processing now tracks generic failure retries, gates reprocessing by tool version, preserves existing SBOM content across failures, and uses annotation-only storage patches for terminal status updates. Tests cover generic failures, scanner crashes, oversized images, and fresh reservations. ChangesSBOM reprocessing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ContainerProcessor
participant Scanner
participant FailureHandler
participant SbomClient
ContainerProcessor->>Scanner: Generate SBOM
Scanner-->>ContainerProcessor: Success or failure
ContainerProcessor->>FailureHandler: Handle failure with content state
FailureHandler->>SbomClient: Patch status annotations at retry threshold
FailureHandler-->>ContainerProcessor: Preserve existing Spec during reprocessing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
…top reprocessing loop SBOM-generation failures (sidecar scan error, invalid image source, syft cataloging error) reported via reportFailure but never marked the reserved SBOM object with a terminal status. Since the reprocessing switch only special-cases TooLarge and Learning, a permanently-failing image fell through to the "processing was interrupted, retrying" default case and was silently reprocessed on every subsequent container start for that image, producing repeated identical error logs and backend failure reports forever. Add markSBOMStatus, generalizing the existing TooLarge-marking pattern, and use it to persist an Incomplete status on all three generic-failure call sites. Add a matching Incomplete case to the reprocessing switch, version-gated exactly like the existing Learning case, so a later node-agent build still retries images that previously failed. This is a pre-existing defect independent of #853/#854 (it already affected ordinary syft SBOM-generation failures, unrelated to digests); splitting it into its own change keeps each fix minimal and reviewable. Docs-exempt: pure bug fix, no existing doc describes SBOM reprocessing or terminal-status behavior Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
850ebad to
8b3170c
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed the change end-to-end against main (built, vetted and ran ./pkg/sbommanager/... on both branches). The diagnosis is right — the default: "processing was interrupted, retrying" fall-through really does reprocess permanently-failing images on every container start, and Incomplete is the semantically correct status (kubevuln's core/services/scan.go explicitly skips Incomplete/TooLarge, and scan_failure_reasons.go maps it to ReasonSBOMIncomplete). But treat this as changes requested: as written the fix introduces a worse regression than the bug it fixes.
Blockers
- Silent SBOM data loss + loss of CVE coverage.
markSBOMStatusdoes a fullReplaceSBOMon awipSbomthat, in the reprocessing path, came fromGetSBOMMetaand therefore has no Spec. A previously-good SBOM is overwritten with an empty one and pinned toincomplete, after which kubevuln skips CVE scanning for that image. Reproduced with a test that passes onmainand fails here — details inline onmarkSBOMStatus. - The pin is effectively permanent for transient failures. The only retry gate is a tool-version bump; unlike the
TooLargecase there's no secondary escape hatch. A registry 503 or a sidecar restart costs that image its SBOM until the next node-agent release. Inline suggestion: reuse the existingscanRetries/maxScanRetriespattern so we only pin after N consecutive failures — that still bounds the loop, which is the actual goal.
Non-blocking
- Possible double failure-reporting to the backend (node-agent
ReasonSBOMGenerationFailed+ kubevulnReasonSBOMIncomplete). - Test fake diverges from the real
GetSBOMMetacontract, which is why the regression above isn't caught; plusscanRetriesis left nil innewTestManager. - The new
Incompletecase is a near-copy of theLearningcase, and a// continue to create SBOMcomment got orphaned between the two.
CI: the DCO check is ACTION_REQUIRED — commits need a Signed-off-by line before this can merge.
Happy to re-review quickly once (1) and (2) are addressed — the core insight is solid and I'd like to see it land.
| func (s *SbomManager) markSBOMStatus(wipSbom *v1beta1.SBOMSyft, sbomName, status string) { | ||
| delete(wipSbom.Annotations, NodeNameMetadataKey) | ||
| wipSbom.Annotations[helpersv1.StatusMetadataKey] = status | ||
| if _, err := s.storageClient.ReplaceSBOM(wipSbom); err != nil { |
There was a problem hiding this comment.
Blocker — this wipes a previously-good SBOM and silently kills CVE scanning for that image.
ReplaceSBOM is a full k8s Update, and markSBOMStatus never clears/repopulates wipSbom.Spec. In the IsAlreadyExists reprocessing path, wipSbom came from GetSBOMMeta, which fetches with softwarecomposition.ResourceVersionMetadata (pkg/storage/v1/storage.go:93) and therefore returns the object without its Spec. SbomSyftStrategy.PrepareForUpdate is a no-op, so nothing merges the old spec back.
Failure scenario:
- Image scanned OK at node-agent
v1.0.0→ SBOM stored with fullSpec, statusready. - Node-agent upgraded to
v2.0.0. Container starts →AlreadyExists→GetSBOMMeta(empty Spec) →Learningcase, version mismatch → continue to reprocess. - The scan fails transiently (sidecar gRPC blip, registry hiccup).
- New code:
markSBOMStatus(wipSbom, ..., Incomplete)→Updatewith an empty Spec → the good SBOM's artifacts are destroyed and it is pinned toincompleteatv2.0.0. - kubevuln then skips it forever:
core/services/scan.go→if sbom.Status == helpersv1.Incomplete || ... { skip }(ErrIncompleteSBOM). The image loses vulnerability coverage until the next node-agent version bump.
Verified empirically — this test passes on main and fails on this branch:
// metaOnlyClient mimics the real GetSBOMMeta, which strips Spec.
type metaOnlyClient struct{ *fakeSbomClient }
func (f *metaOnlyClient) GetSBOMMeta(name string) (*v1beta1.SBOMSyft, error) {
f.mu.Lock(); defer f.mu.Unlock()
if s, ok := f.sboms[name]; ok {
c := s.DeepCopy()
c.Spec = v1beta1.SBOMSyftSpec{} // <- what the apiserver actually returns
return c, nil
}
return nil, k8serrors.NewNotFound(schema.GroupResource{Resource: "sbomsyfts"}, name)
}
func Test_goodSBOMWipedByTransientFailure(t *testing.T) {
inner := newFakeSbomClient()
mgr := ... // storageClient: &metaOnlyClient{inner}, version "v2.0.0", scanner always fails
good := &v1beta1.SBOMSyft{}
good.Name = sbomName
good.Annotations = map[string]string{
helpersv1.StatusMetadataKey: helpersv1.Learning,
helpersv1.ToolVersionMetadataKey: "v1.0.0",
}
good.Spec.Syft.Artifacts = make([]v1beta1.SyftPackage, 2)
inner.sboms[sbomName] = good
mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID)
assert.Len(t, inner.sboms[sbomName].Spec.Syft.Artifacts, 2)
}main: status="ready" toolVersion="v1.0.0" artifacts=2 PASS
this branch: status="incomplete" toolVersion="v2.0.0" artifacts=0 FAIL
Pre-PR this path just reportFailure + return, leaving the good SBOM intact — so this is a new regression, not a pre-existing one. (The existing TooLarge branch has the same shape, but it was reachable only via ErrImageTooLarge, which is deterministic per image; generalising it to every scan error is what makes it dangerous. Note handleScannerCrash deliberately does wipSbom.Spec = v1beta1.SBOMSyftSpec{} before its ReplaceSBOM — markSBOMStatus is missing that awareness entirely, but zeroing the spec here would be equally wrong since it would still destroy the good data.)
Suggested fixes, roughly in order of preference:
- Only mark
Incompletewhen we actually own a fresh reservation — i.e. when the pre-scan status wasInitializing. If we're reprocessing an SBOM that already had content, leave the stored object untouched on failure (report and return, as today). - Or update annotations only, via a JSON/merge
Patchon the typed client (GetStorageClient()), so the spec can never be clobbered by a metadata-only object.
There was a problem hiding this comment.
Confirmed — verified GetSBOMMeta's metadata-only fetch directly in pkg/storage/v1/storage.go:93. Fixed in db239df2: markSBOMStatus is no longer called unconditionally. I now track whether the SBOM being reprocessed had prior successful content (wipSbomHadContent, set only when the Learning case takes its version-mismatch retry branch — that's the only path where wipSbom can carry real server-side content it wasn't given) and skip the destructive ReplaceSBOM whenever it did, reporting the failure but otherwise leaving the stored object untouched, per your suggested fix #1.
While auditing every reachable persist path for the same defect, I also found (and fixed) two more sites with the identical unguarded pattern that a second and third review pass caught: handleScannerCrash (clears Spec + ReplaceSBOM after repeated OOM crashes) and the ErrImageTooLarge branch (whose totalSize is computed from the currently-mounted layer paths, not a fixed image property, so it's reachable during a content-bearing reprocess too, contrary to my earlier assumption that it was safely deterministic). All three now share the same guard. Added Test_processContainerWithMetadata_PreservesContentOnReprocessFailure/OnScannerCrash/OnTooLarge, each empirically confirmed to reproduce your exact failure scenario (artifacts wiped, status flipped, ReplaceSBOM called) when the corresponding guard is removed.
One edge case I'm flagging rather than fixing: on the success path, if a fresh reprocess actually runs and produces a real (non-empty) syftDoc that then exceeds MaxSBOMSize, the code still clears Spec and marks TooLarge unconditionally (pre-existing behavior, sbom_manager.go around the size-check after "prepare the SBOM"). That's a different flavor — a real new result being rejected for size, not a stale empty object clobbering a good one — but the net effect (an older, smaller, valid SBOM getting replaced by nothing) is similar. Happy to guard that too if you think it's in scope here, or file it separately.
| // update the version of the tool | ||
| wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] = s.version | ||
| // continue to create SBOM | ||
| case wipSbom.Annotations[helpersv1.StatusMetadataKey] == helpersv1.Incomplete: |
There was a problem hiding this comment.
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/maxScanRetriespattern (already inhandleScannerCrash) and only pin toIncompleteafter 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
ScannerMemoryLimitAnnotationgates theTooLargeskip.
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
Learningcase above (~20 lines). Worth collapsing tocase status == helpersv1.Learning || status == helpersv1.Incomplete:with the log message varying, or extracting a smallshouldRetryAtCurrentVersionhelper. - The stray
// continue to create SBOMcomment now sits between theLearningcase body and thiscaseline, so it reads as if it belongs to the new case. Move it back inside theLearningcase.
There was a problem hiding this comment.
Addressed in db239df2:
- Bounded retry: added a
failureRetries map[string]intcounter (separate fromscanRetries, whichhandleScannerCrashalready owns for OOM crashes specifically) and a newhandleGenericFailurehelper. A content-safe (no prior content) generic failure now only gets pinned toIncompleteaftermaxScanRetries(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 anIncompleteimage a fresh budget. - Style nits: extracted
shouldRetryAtCurrentVersion(wipSbom, sbomName, notif, skipMsg, retryMsg) bool, now shared verbatim by both theLearningandIncompletecases — eliminates the ~20-line duplication rather than just collapsing thecasecondition. The stray// continue to create SBOMcomment 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).
| helpers.String("pod", notif.Container.K8s.PodName), | ||
| helpers.String("container", notif.Container.K8s.ContainerName), | ||
| helpers.String("sbomName", sbomName)) | ||
| s.markSBOMStatus(wipSbom, sbomName, helpersv1.Incomplete) |
There was a problem hiding this comment.
Minor, but worth confirming: this now produces two backend failure reports for one failure. node-agent reports ReasonSBOMGenerationFailed here, and once the object is persisted as incomplete, kubevuln's scan_failure_reasons.go maps helpersv1.Incomplete → ReasonSBOMIncomplete and reports again when it picks the SBOM up. Previously the object stayed initializing and kubevuln never reported. Since reducing duplicate backend reports is one of the stated goals of this PR, it'd be good to check that this doesn't just move the duplication one layer down.
There was a problem hiding this comment.
Fair point, and I don't have visibility to fully resolve it from this repo alone — kubevuln's report-on-Incomplete behavior is outside this PR's scope. What I can say: with the bounded-retry fix above, node-agent's own reporting is no longer unbounded (previously it reported once per attempt, forever; now it's capped at maxScanRetries reports before the image is pinned and stops reprocessing), so the total report volume from node-agent's side is strictly lower than before regardless of what kubevuln does on top. Whether kubevuln's ReasonSBOMIncomplete report is itself redundant with node-agent's ReasonSBOMGenerationFailed is a call for that side to make — flagging it here in case it's useful context if you or someone follows up there.
| return sbom, nil | ||
| } | ||
|
|
||
| func (f *fakeSbomClient) GetSBOMMeta(name string) (*v1beta1.SBOMSyft, error) { |
There was a problem hiding this comment.
Test gap — this fake can't catch the bug in markSBOMStatus.
fakeSbomClient.GetSBOMMeta returns s.DeepCopy() with the full Spec, but the real Storage.GetSBOMMeta fetches with softwarecomposition.ResourceVersionMetadata and returns the object with an empty Spec (pkg/storage/v1/storage.go:93). That divergence is exactly what hides the data-loss regression: the test only ever exercises the freshly-CreateSBOM'd object, never the metadata-only one from the reprocessing path.
Please make the fake mirror the real contract (c.Spec = v1beta1.SBOMSyftSpec{} before returning), and add a case that starts from a pre-existing ready SBOM with a populated Spec.Syft.Artifacts at an older tool version, fails the scan, and asserts the artifacts survive.
Also: newTestManager leaves scanRetries nil. It's unreachable in the current test (the fake scanner never returns ErrScannerCrashed), but handleScannerCrash does s.scanRetries[sbomName]++, which panics on a nil map — easy footgun for whoever extends this table next. Worth initialising it.
There was a problem hiding this comment.
Done in db239df2:
fakeSbomClient.GetSBOMMetanow always stripsSpecbefore returning (meta.Spec = v1beta1.SBOMSyftSpec{}), unconditionally mirroring the realStorage.GetSBOMMetacontract for every test, not just a special-cased wrapper.newTestManagernow initializes bothscanRetriesandfailureRetries(the new counter). Also addednewTestManagerWithScannerErr/newTestManagerInProcessfor the scanner-crash and too-large scenarios, all consistently initializing both maps.- Added the exact case you described: a pre-existing
Learning-status SBOM withSpec.Syft.Artifactspopulated at an older tool version, driven through a failing reprocess, asserting the artifacts survive untouched (plus the scanner-crash and too-large variants of the same scenario, once the audit found those paths had the identical gap).
I verified each of the three new tests is a real regression guard, not a vacuous one — for each, I temporarily removed just its corresponding guard, confirmed the test fails with the exact symptom (artifacts wiped to 0, status flipped, ReplaceSBOM invoked), then restored the fix and reconfirmed the full suite is green.
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Addresses review feedback: markSBOMStatus persisted a wipSbom fetched via GetSBOMMeta, which the storage layer returns without its Spec (metadata- only fetch). Reprocessing a previously-successful, content-bearing SBOM (e.g. after a node-agent version bump) that then fails would silently overwrite its real content with an empty Spec and pin it to a terminal status -- permanently losing vulnerability-scan coverage for that image. Track whether the SBOM being reprocessed had prior successful content (wipSbomHadContent, set only in the Learning-case version-mismatch branch) and skip the destructive persist whenever it did, across every path that can reach it: the generic-failure branches (handleGenericFailure, now with a bounded failureRetries counter so a single transient error doesn't permanently pin an image either), the scanner-crash branch (handleScannerCrash), and the ErrImageTooLarge branch, whose totalSize is computed from the currently-mounted layer paths rather than being a fixed property of the image and so is equally reachable while reprocessing. Also extracts shouldRetryAtCurrentVersion, shared by the Learning and Incomplete switch cases, removing the near-duplicate version-gating logic and the comment that had drifted between them. Docs-exempt: pure bug fix, no existing doc describes SBOM reprocessing or terminal-status behavior Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
|
@matthyx Thanks for the thorough review — the data-loss finding was real and important. Pushed Blocker 1 (data loss): Fixed as you suggested — never persist Blocker 2 (no escape hatch): Added a Non-blocking: replied inline on both — the double-reporting one is now bounded on node-agent's side but the kubevuln half is out of scope here; the test-fake fix and near-duplicate-case extraction are both done. One thing I'm flagging rather than fixing (see inline reply on the All tests + |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed at db239df. Both blockers are fixed — verified independently with my own tests rather than trusting the new ones:
| Check | Result |
|---|---|
Content preserved through 15 consecutive failures on a ready SBOM at a bumped version |
artifacts=2, status=ready, replaceCalls=0 |
| Bounded retry on a fresh SBOM: 20 container starts, always-failing scanner | 3 scan attempts, then pinned incomplete and rescans stop |
go build / go vet / go test ./pkg/sbommanager/... |
pass |
| DCO | now success — both commits signed off |
The wipSbomHadContent approach is sound, and I like that the guard was extended to all three destructive paths rather than just the one I flagged — handleScannerCrash and the ErrImageTooLarge branch were both genuinely reachable while reprocessing, and the reasoning about totalSize being derived from mounted layer paths is correct. I also checked that hadContent is set on the only case that can hold real content: Learning is the sole status node-agent writes with a populated Spec (TooLarge explicitly zeroes it, Initializing/Incomplete never had one), and a completed SBOM has no NodeName annotation so it can't reach the default: branch instead. shouldRetryAtCurrentVersion cleans up the duplication and the orphaned comment nicely, and the test double now mirrors the real metadata-only GetSBOMMeta contract, with scanRetries/failureRetries initialised.
Nothing blocking left. Two non-blocking observations inline:
hadContentreintroduces unbounded reprocessing for previously-successful images that start failing permanently — 10 container starts still produce 10 scans, 10 error logs and 10 backend reports. Acceptable trade vs. data loss, but an annotation-onlyPatchwould give you both properties and lethadContentgo away. At minimum the PR description shouldn't claim the loop is fixed unconditionally.failureRetriesaccumulates stale counters (50 entries after 50 one-shot failures).
Also still open from last round: node-agent's ReasonSBOMGenerationFailed plus kubevuln's ReasonSBOMIncomplete on the persisted object means one failure can still surface as two backend reports — now only after 3 failures instead of 1, so much less noisy, but worth a sanity check on the backend side.
CI: component tests and the benchmark job are still running; everything that has finished is green.
LGTM once you've decided what to do with (1) — happy either way, it just shouldn't be silent.
| // failed), so it's safe to persist -- but only after maxScanRetries consecutive failures, so a | ||
| // single transient error doesn't permanently pin the image to Incomplete. | ||
| func (s *SbomManager) handleGenericFailure(wipSbom *v1beta1.SBOMSyft, sbomName string, hadContent bool) { | ||
| if hadContent { |
There was a problem hiding this comment.
Both of my blockers are resolved here — I re-verified independently rather than relying on the new tests, and the content-preservation guard holds even when hammered well past the retry budget (15 consecutive failures on a ready SBOM at a bumped version: artifacts=2, status=ready, replaceCalls=0).
One residual worth naming explicitly, non-blocking: if hadContent { return } means a previously-successful SBOM that now fails permanently goes back to being reprocessed on every single container start — which is the exact spam this PR set out to kill, just scoped to a narrower set of images:
// content-bearing SBOM at v1.0.0, node-agent now v2.0.0, scanner always fails
for range 10 {
mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID)
}
// scan attempts: 10 (stored tool version still "v1.0.0")Ten container starts → ten full scans, ten SbomManager - sidecar scan failed error logs, ten reportFailure calls to the backend. shouldRetryAtCurrentVersion bumps ToolVersionMetadataKey in memory only, and since nothing is persisted on failure the stored object never advances past v1.0.0, so the next start makes the identical decision. Same shape in handleScannerCrash: with hadContent the OOM is still reported every maxScanRetries crashes, forever.
Trading unbounded logs for not destroying data is clearly the right call, so I'm fine with this shipping. But the cleaner endgame is the annotation-only update I mentioned last round: a JSON/merge Patch on the typed client (GetStorageClient()) touching only metadata.annotations can record the terminal status and the new tool version without ever sending a Spec, so the spec can't be clobbered by a metadata-only object. That gets you both properties at once and lets hadContent disappear entirely, along with the three separate guards it now threads through handleGenericFailure / handleScannerCrash / the ErrImageTooLarge branch.
If you'd rather keep this PR minimal (reasonable — it's already grown), the ask is just to soften the PR description: the reprocessing loop is now bounded for images that never had content, but still unbounded for images that had content and start failing.
There was a problem hiding this comment.
Went with the "cleaner endgame" — pushed d95955c7. Added storage.SbomClient.PatchSBOMAnnotations(name, annotations), a JSON merge patch on metadata.annotations only (implemented via the typed client's .Patch(..., types.MergePatchType, ...)), and switched all SBOM status marking to go through it instead of ReplaceSBOM. Since the patch payload only ever contains {"metadata":{"annotations":{...}}}, it structurally cannot carry spec — there's no path for it to clobber existing content regardless of what the caller does or doesn't know about the object. hadContent and every guard built on it (handleGenericFailure, handleScannerCrash, the ErrImageTooLarge branch) are gone entirely.
Practical effect: retries are now bounded uniformly for every image via the same failureRetries counter, whether or not it previously had content. I independently sanity-tested the real implementation (not just my test double) against a fake k8s clientset — created an SBOM with a node-name annotation, patched with node-name: nil + two new keys, and confirmed the null-valued key was actually deleted and the others set, both in the patch response and a subsequent GetSBOMMeta.
One caveat worth flagging (an independent review pass caught this): the new failureRetries counter uses expirable.LRU for the memory-leak fix below, and its TTL resets on every write — so two failures for the same image spaced more than 30 minutes apart don't accumulate toward the pin threshold. That fully closes the loop for a tight failure cadence (crash loops, the actual reported symptom) but a slow-cadence permanent failure (long-lived pod, infrequent CronJob) could still in principle reprocess indefinitely without ever hitting 3 consecutive failures. Documented on the field; happy to widen the TTL or decouple it from the counting logic if you'd rather close that gap too.
| scannerClient sbomscanner.SBOMScannerClient | ||
| scannerMemLimit int64 | ||
| scanRetries map[string]int // safe without mutex: only accessed from pool workers (pool size 1) | ||
| failureRetries map[string]int // consecutive generic SBOM-generation failures per sbomName; safe without mutex: only accessed from pool workers (pool size 1) |
There was a problem hiding this comment.
Nit: this map only ever shrinks on success or on reaching maxScanRetries, so images that fail once or twice and are never seen again leave their counters behind forever. Easy to observe:
failureRetries entries after 50 distinct one-shot failures: 50
scanRetries has the same shape and predates this PR, so it's not a regression — but failureRetries is reachable from every scan error rather than just sidecar OOM crashes, so it accumulates a lot faster on a long-lived node-agent that sees many short-lived images. Worth capping it the way pendingScans already caps itself with maxPendingScans, or clearing entries when the container-collection drops the image.
There was a problem hiding this comment.
Fixed in d95955c7: failureRetries is now an expirable.LRU[string, int] (github.com/hashicorp/golang-lru/v2, already a repo dependency, used the same way in pkg/hostfimsensor/v1/dedup_cache.go and pkg/rulemanager/rulecooldown) capped at 1000 entries with a 30-minute TTL, instead of a plain unbounded map — so one-shot failures for images never seen again get reclaimed automatically instead of accumulating forever.
Side effect worth calling out (see my reply on the sibling thread): the TTL means the counter also resets between sparse failures, so it bounds retries for a tight failure cadence but not an arbitrarily slow one. Trade-off documented in a comment on the field.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/sbommanager/v1/sbom_manager.go (1)
647-659: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
markSBOMStatusrelies on callers to keepwipSbom.Specempty rather than enforcing it itself.Every current caller of
markSBOMStatus(line 476, and viahandleGenericFailureat line 705) happens to invoke it beforewipSbom.Specis ever populated, so it's currently safe. But the siblingTooLarge-marking code explicitly zeroesSpecbefore persisting (lines 534, 590), whilemarkSBOMStatusdoes not — it's an implicit invariant, not an enforced one. Given the exact failure mode this PR was created to fix was a Spec-wipe caused by a similar implicit assumption, hardening the shared helper removes the risk of a future call site (or reordering) silently reintroducing it.🛡️ Defensive hardening for markSBOMStatus
func (s *SbomManager) markSBOMStatus(wipSbom *v1beta1.SBOMSyft, sbomName, status string) { delete(wipSbom.Annotations, NodeNameMetadataKey) wipSbom.Annotations[helpersv1.StatusMetadataKey] = status + wipSbom.Spec = v1beta1.SBOMSyftSpec{} if _, err := s.storageClient.ReplaceSBOM(wipSbom); err != nil {Also applies to: 685-706
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sbommanager/v1/sbom_manager.go` around lines 647 - 659, Update markSBOMStatus and its callers’ shared status-persistence flow to explicitly preserve the intended empty wipSbom.Spec state before ReplaceSBOM, rather than relying on call order or caller behavior. Align this helper with the existing TooLarge-marking paths that clear Spec, while keeping the annotation updates and error logging unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/sbommanager/v1/sbom_manager.go`:
- Around line 647-659: Update markSBOMStatus and its callers’ shared
status-persistence flow to explicitly preserve the intended empty wipSbom.Spec
state before ReplaceSBOM, rather than relying on call order or caller behavior.
Align this helper with the existing TooLarge-marking paths that clear Spec,
while keeping the annotation updates and error logging unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e548f0a4-03e0-4122-be92-c0f64f7f9afe
📒 Files selected for processing (2)
pkg/sbommanager/v1/sbom_manager.gopkg/sbommanager/v1/sbom_manager_reprocessing_test.go
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
|
Status re-check at Two small things left, neither blocking: 1. The PR description is now stale. The "Known trade-off" paragraph describes the old behaviour and contradicts the shipped code:
That is no longer true —
This is exactly what The description also doesn't mention the content-preservation guard ( 2. I'd take @coderabbitai's Otherwise LGTM. |
|
Tip For best results, initiate chat on the files or code changes.
The PR description should be updated to reflect the bounded generic-failure retry behavior and the I also agree that 🐇 ✅ ✏️ Learnings added
|
…place Addresses non-blocking review feedback: the previous fix's hadContent guard meant a previously-successful SBOM that started failing permanently (not just transiently) was reprocessed on every container start forever, since nothing was ever persisted to stop the loop for that class of image -- the retry-bounding only applied to images that never had content. Add storage.SbomClient.PatchSBOMAnnotations, a JSON merge patch on metadata.annotations only, which never sends spec regardless of what the caller does or doesn't know about the object's content. Route all SBOM status marking (Incomplete, TooLarge) through it instead of a full ReplaceSBOM, so it's always safe to persist a terminal status -- the hadContent tracking, and every guard built on it across handleGenericFailure, handleScannerCrash and the ErrImageTooLarge branch, is removed entirely. Retries are now bounded uniformly for all images via the same failureRetries counter, switched from a plain map to a bounded+TTL'd expirable.LRU so short-lived images don't leak entries either (the second non-blocking item). markSBOMStatus now also records the current tool version alongside the status, since the version check that gates future reprocessing depends on it and the object is no longer implicitly carrying an in-memory version bump the way the old ReplaceSBOM-based code did. Docs-exempt: pure bug fix, no existing doc describes SBOM reprocessing, terminal-status, or storage patch behavior Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
|
@matthyx Pushed
One new trade-off surfaced by an independent review pass and flagged inline: the LRU's TTL resets on every write, so the retry count only accumulates for failures spaced under 30 minutes apart — fully closes the loop for the reported symptom (crash loops) but not for an arbitrarily slow failure cadence. Happy to adjust if you'd like that closed too. All tests green (including a from-scratch sanity check of the real |
|
@matthyx Re: #issuecomment-5022708354 — both addressed:
Let me know if the updated description reads right, or if you'd like the slow-cadence TTL gap closed too. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed at d95955c. The annotation-only patch is the right shape and I verified the mechanism end-to-end rather than assuming it works:
sbomsyfts.NewRESTis a plaingenericregistry.Store(noimmutableStorage, noPatchoverride in theregistry.RESTwrapper), soMergePatchTypeis fully wired.- The registry's
getCurrentStatedoes a plainGet— not theResourceVersionMetadataone — so the merge patch applies to the full stored object, andPrepareForUpdateis a no-op. Spec genuinely survives. nullvalues for annotation deletion are correct merge-patch semantics, and the mock mirrors that.go build ./...,go vet,go test ./pkg/sbommanager/... ./pkg/storage/...all pass.
Removing hadContent entirely, uniform retry bounding for every image, the expirable.LRU (with its TTL caveat documented honestly), and the tests flipping from replaceCalls to patchCalls with an explicit "must never use a full ReplaceSBOM" assertion — all good. This is a genuinely better design than db239df.
One new problem, though, and I think it needs addressing before merge. TooLarge is not just another status: StorageImpl.GuaranteedUpdate (storage@v0.0.290, pkg/registry/file/storage.go:900) short-circuits and silently drops every subsequent write to any object annotated status: too-large. So the two markSBOMStatus(..., TooLarge, ...) call sites — the ErrImageTooLarge branch and handleScannerCrash — can now take a previously-successful, content-bearing SBOM and freeze it permanently: skipped by kubevuln (no CVE coverage), unrepairable by any future node-agent version, and still carrying the full spec that TooLarge exists to shed.
Against main this is still an improvement (there it froze and wiped). Against db239df it's a step back, because the hadContent guard kept those SBOMs ready and scannable. That's on my last review for pushing on the noise without flagging the freeze property — details and three fix options inline; I'd keep the patch for Incomplete and just not let TooLarge land on a content-bearing object.
CI on this head is still running; everything green on the previous head.
| helpers.Error(replaceErr), | ||
| helpers.String("sbomName", sbomName)) | ||
| } | ||
| s.markSBOMStatus(sbomName, helpersv1.TooLarge, nil) |
There was a problem hiding this comment.
New issue introduced by this commit — marking a content-bearing SBOM TooLarge freezes it permanently in storage.
The patch mechanism itself is correct: sbomsyfts.NewREST is a plain genericregistry.Store, so a MergePatchType patch is applied server-side to the full current object (the registry's getCurrentState does a plain Get, not the ResourceVersionMetadata one), and SbomSyftStrategy.PrepareForUpdate is a no-op. So spec really is preserved. That part works.
But TooLarge is special in the storage layer in a way Incomplete is not. StorageImpl.GuaranteedUpdate short-circuits before running tryUpdate:
// storage@v0.0.290 pkg/registry/file/storage.go:900-906
annotations := origState.obj.(metav1.Object).GetAnnotations()
if annotations != nil && annotations[helpersv1.StatusMetadataKey] == helpersv1.TooLarge {
logger.L().Debug("GuaranteedUpdate - already too large object, skipping update", ...)
v.Set(reflect.ValueOf(origState.obj).Elem())
return nil // <- silently drops the write, returns nil error
}Once an object carries status: too-large, no update or patch ever lands on it again — silently, with no error returned to the client.
Combine that with the guard removal here and you get a one-way door on two paths — the ErrImageTooLarge branch above, and handleScannerCrash after maxScanRetries OOMs — both now reachable while reprocessing a previously-successful ready SBOM:
- Image scanned fine at
v1.0.0; SBOM has real artifacts. - Version bump to
v2.0.0→Learningcase reprocesses. ErrImageTooLargefires (your owndb239dfmessage correctly notedtotalSizecomes from the currently-mounted layer paths, so this is reachable), or the sidecar OOMs three times.markSBOMStatus(sbomName, TooLarge, ...)patchesstatus: too-largeand leaves the full spec in place.
The resulting object is now:
- skipped by kubevuln forever —
core/services/scan.goskips onIncomplete || TooLarge, so the image silently loses CVE coverage; - unfixable by any future node-agent version — the
TooLargeswitch case only retries whenScannerMemoryLimitAnnotationchanged, and even if it does retry and succeeds, the finalReplaceSBOMis silently swallowed by the short-circuit above. There is no node-agent code path that can ever repair it; - still carrying the full spec, which defeats the whole point of
TooLarge(storage's ownObjectTooLargeErrorhandler callsclearSpec(ret)with the comment "clear spec to not bloat the storage"), and both otherTooLargewriters (handleScannerCrashpre-this-commit, and thesz > MaxSBOMSizepath at line ~530) zero the Spec explicitly.
Your new Test_..._PreservesContentOnScannerCrash asserts exactly this state (status == TooLarge, artifacts == 2) as the desired outcome — that's the assertion I'd revisit.
Note this is not a regression against main (there, both paths did a spec-clearing ReplaceSBOM, so they froze the object and destroyed the content). It's a regression against db239df, where the hadContent guard left the SBOM ready and scannable, at the cost of the reprocessing noise I flagged. Trading noisy-but-recoverable for silent-and-permanent is the wrong direction, and I don't think I made that trade-off clear enough last round — sorry.
Suggested fix, smallest first:
- Keep the annotation-only patch for
Incomplete, but don't let a content-bearing SBOM reachTooLargeat all on the reprocess path.Incompleteis retryable on a version bump and is not short-circuited by the storage layer, so it's the correct terminal status when we're not deliberately discarding content. - Or keep a narrow
hadContentguard only for the twoTooLargecall sites, and letIncompleteflow freely through the patch — that preserves everything this commit achieves for the common case. - Or, if you do want
TooLargesemantics here, it must clear the spec, which means a fullReplaceSBOM— i.e. deliberately discarding the old SBOM. Defensible for a genuinely-too-large image, much less so for three sidecar OOMs.
I'd go with (1) or (2).
There was a problem hiding this comment.
Went with your fix (1) — pushed a5c562be. Reintroduced wipSbomHadContent, but scoped narrowly to just the two TooLarge write sites this time (not threaded through every generic-failure path like the old hadContent was): the ErrImageTooLarge branch and handleScannerCrash's post-maxScanRetries marking. When a content-bearing SBOM would otherwise hit either, it now falls back to Incomplete (via the same annotation-only patch, retryable, no storage-layer short-circuit) instead of TooLarge. Content-free images still get marked TooLarge immediately on either path — unchanged.
Incomplete keeps flowing entirely through PatchSBOMAnnotations as of d95955c7, untouched by this change — this only affects the two TooLarge writers.
Added a new test, Test_processContainerWithMetadata_MarksFreshImageTooLargeImmediately, since all the prior TooLarge tests happened to be content-bearing scenarios and there was no coverage left for the unchanged "fresh reservation, no content to lose" case. Verified both modified tests (_PreservesContentOnScannerCrash, _PreservesContentOnTooLarge) reproduce your exact one-way-door symptom (status flips to too-large instead of staying incomplete) when the new guard is reverted.
One thing I deliberately left alone, flagging rather than fixing: handleScannerCrash keeps its own independent scanRetries counter rather than sharing failureRetries with the generic-failure path. An image that accumulates some generic failures and then hits 3 OOM crashes gets a fresh crash-specific budget on top, so total attempts before a terminal status can exceed maxScanRetries in a mixed-failure-mode scenario. That's a pre-existing inconsistency (both counters already existed independently before this PR), not something introduced by this fix — happy to unify them as a follow-up if you'd like, but didn't want to fold another counter-architecture change into round 5.
| // 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] |
There was a problem hiding this comment.
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.
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
…way door Addresses review feedback on d95955c: TooLarge is special-cased in the storage layer's GuaranteedUpdate, which silently drops every future write (patch or replace) to an object once its status annotation is TooLarge. Every other TooLarge writer in this codebase explicitly clears Spec first because of this, but PatchSBOMAnnotations never touches Spec at all -- so patching a content-bearing SBOM to TooLarge left its real Spec permanently frozen in storage: unfixable by any future reprocess, and defeating the point of TooLarge in the first place (avoiding a bloated stored object). Reintroduce wipSbomHadContent, scoped narrowly to the two TooLarge write sites (the ErrImageTooLarge branch and handleScannerCrash's post-maxScanRetries marking): a content-bearing SBOM now falls back to the retryable Incomplete path instead, which has no such short-circuit. Incomplete continues to flow entirely through the annotation-only patch introduced in d95955c, unaffected. Docs-exempt: pure bug fix, no existing doc describes SBOM status transitions or storage patch semantics Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/sbommanager/v1/sbom_manager.go (1)
298-353: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
wipSbomHadContentisn't set when reprocessing an already-IncompleteSBOM that still has preserved content — reopens theTooLargeone-way door.
wipSbomHadContentis only settruein theLearningcase (line 345). But anIncomplete-status SBOM can also carry real, preservedSpec.Syft.Artifacts— exactly whatTest_processContainerWithMetadata_PreservesContentOnReprocessFailuredemonstrates: aLearningSBOM that fails repeatedly ends upIncompletewith its 2 artifacts intact, becausePatchSBOMAnnotations/handleGenericFailurenever touchSpec.If that SBOM is reprocessed again at a later tool version (hits the
Incompletecase at line 347, notLearning),wipSbomHadContentstaysfalse. A subsequentErrImageTooLarge(line 484-490) ormaxScanRetriesscanner crashes (line 599-605) then takes the non-hadContentbranch and callsmarkSBOMStatus(..., helpersv1.TooLarge, ...). That's an annotation-only patch — it does not clearSpec— so the real content survives in storage, but the storage layer'sGuaranteedUpdateshort-circuit makesTooLargea one-way door: the object is now permanently frozen with its old content, unfixable by any future version bump. This is the exact bug class already fixed for theLearningcase in this PR, just reachable through a narrower, currently-untested path.Setting
wipSbomHadContent = truein theIncompletecase too closes this gap; the only behavioral cost for a genuinely content-freeIncompleteSBOM is a slightly more conservative (but safe) outcome.🐛 Proposed fix
case wipSbom.Annotations[helpersv1.StatusMetadataKey] == helpersv1.Incomplete: 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 } + // an Incomplete SBOM can already carry preserved content from an earlier + // successful generation (handleGenericFailure/handleScannerCrash never clear + // Spec), so treat it the same as Learning to avoid the TooLarge one-way door. + wipSbomHadContent = true // continue to create SBOM🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sbommanager/v1/sbom_manager.go` around lines 298 - 353, Set wipSbomHadContent to true in the Incomplete branch of the existing-status switch, after shouldRetryAtCurrentVersion permits reprocessing, matching the Learning branch. This ensures reprocessed Incomplete SBOMs preserve content-safe failure handling while leaving the retry and skip behavior unchanged.
🧹 Nitpick comments (1)
pkg/storage/v1/storage.go (1)
107-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGuard against a nil/empty
annotationsmap wiping all annotations.Per RFC 7396 (JSON Merge Patch), a
nullvalue for a field deletes that field from the target.json.Marshalof a nil Go map producesnull, soPatchSBOMAnnotations(name, nil)would send{"metadata":{"annotations":null}}, deleting the object's entire annotations map (status, tool-version, image-tag, etc.) rather than being a no-op.No current caller triggers this (
markSBOMStatusalways builds a non-nil literal map), so this isn't an active bug, but it's a footgun for future callers — andstorage_mock.go's mock implementation silently behaves as a no-op for nil (ranging over a nil map), so tests built on the mock wouldn't catch a real-world regression here.🛡️ Proposed guard
func (sc *Storage) PatchSBOMAnnotations(name string, annotations map[string]any) (*v1beta1.SBOMSyft, error) { + if annotations == nil { + annotations = map[string]any{} + } patch, err := json.Marshal(map[string]any{ "metadata": map[string]any{ "annotations": annotations, }, })Please confirm the RFC 7396 null-deletes-field semantics against your own understanding of the Kubernetes merge-patch behavior before applying, since this affects a sensitive annotation-preservation guarantee this PR relies on.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/storage/v1/storage.go` around lines 107 - 117, Update PatchSBOMAnnotations to return without issuing a patch when annotations is nil or empty, preserving all existing annotations and matching the mock’s no-op behavior. Keep the current merge-patch flow unchanged for non-empty maps, including the existing JSON marshaling and SBOMSyfts patch call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/sbommanager/v1/sbom_manager.go`:
- Around line 298-353: Set wipSbomHadContent to true in the Incomplete branch of
the existing-status switch, after shouldRetryAtCurrentVersion permits
reprocessing, matching the Learning branch. This ensures reprocessed Incomplete
SBOMs preserve content-safe failure handling while leaving the retry and skip
behavior unchanged.
---
Nitpick comments:
In `@pkg/storage/v1/storage.go`:
- Around line 107-117: Update PatchSBOMAnnotations to return without issuing a
patch when annotations is nil or empty, preserving all existing annotations and
matching the mock’s no-op behavior. Keep the current merge-patch flow unchanged
for non-empty maps, including the existing JSON marshaling and SBOMSyfts patch
call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e4a7a44-2c8d-46f8-a502-982d28dbd6fc
📒 Files selected for processing (5)
pkg/sbommanager/v1/sbom_manager.gopkg/sbommanager/v1/sbom_manager_reprocessing_test.gopkg/storage/storage_interface.gopkg/storage/storage_mock.gopkg/storage/v1/storage.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/sbommanager/v1/sbom_manager_reprocessing_test.go
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed at a5c562b. This is the fix I was asking for, and I verified the invariant independently across every failure mode rather than trusting the updated tests.
Hammered a content-bearing SBOM (ready at v1.0.0, 2 artifacts, node-agent at v2.0.0) with maxScanRetries * 4 container starts through each of the three paths that could previously reach TooLarge:
| Failure mode | Final status | Artifacts | patch / replace |
|---|---|---|---|
| generic sidecar error | incomplete |
2 | 1 / 0 |
| scanner OOM crash loop | incomplete |
2 | 1 / 0 |
ErrImageTooLarge on reprocess |
incomplete |
2 | 1 / 0 |
Never TooLarge, content intact, exactly one patch, zero full replaces, then it stops. And the other half still holds — a fresh content-free image hitting ErrImageTooLarge is pinned TooLarge immediately with no retry budget, Spec empty.
I also re-checked the invariant across all three TooLarge writers in the file, since that's what makes the one-way door safe: lines 489 and 602 are now both guarded by hadContent, and the sz > MaxSBOMSize path at 545 explicitly clears the Spec. So every object that can carry status: too-large has an empty Spec again — which is exactly the property the storage-layer short-circuit assumes. Scoping wipSbomHadContent to just the two TooLarge sites, and letting Incomplete keep flowing through the annotation-only patch, is the right split.
go build ./..., go vet, go test ./pkg/sbommanager/... ./pkg/storage/... all pass. CI is green apart from the benchmark job, still running.
LGTM. Two notes inline (a memory-limit-recovery asymmetry, and a pre-existing main issue worth a follow-up issue rather than more churn here), plus one thing to fix in the description before merge:
The rewritten description is much better, but it's now stale by one commit. Under Content preservation it still says:
All SBOM-generation failure paths (generic scan/source/syft errors, repeated sidecar OOM crashes, and
ErrImageTooLarge) now go through this patch-based marking, so retries are bounded uniformly whether or not the image previously had content — no separate case for "already had good data" is needed anymore.
a5c562b reintroduced exactly that separate case, deliberately. Worth replacing that sentence with the one-way-door reasoning — it's now a load-bearing design constraint that the next person to touch markSBOMStatus needs to know, and the commit message explains it well.
Pre-existing, out of scope — recording it so it isn't lost, since it's the last remaining way a good SBOM's content can be destroyed. if a reprocess succeeds but the resulting SBOM exceeds MaxSBOMSize, the sz > MaxSBOMSize path (sbom_manager.go:545) clears the Spec and pins TooLarge via a full ReplaceSBOM. On the reprocess path that overwrites a previously-successful, correctly-sized SBOM with an empty one, and the storage-layer short-circuit then freezes it permanently — the same one-way door a5c562b closes for the failure paths, still open for the success-but-oversized path.
Arguably defensible (we did generate a real SBOM and it genuinely is too large), and unlike the failure paths there's no transient-error component here, so it's much less likely to fire spuriously. Worth a follow-up issue rather than more churn on this PR.
| helpers.Error(replaceErr), | ||
| helpers.String("sbomName", sbomName)) | ||
| if hadContent { | ||
| s.markSBOMStatus(sbomName, helpersv1.Incomplete, nil) |
There was a problem hiding this comment.
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.
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
1 participant
Overview
SBOM-generation failures (sidecar scan error, invalid/unparseable image source, syft cataloging error, oversized image) are reported via
reportFailure, but the reserved SBOM object was never marked with a terminal status. Since the reprocessing switch inprocessContainerWithMetadataonly special-casedTooLargeandLearning, a permanently-failing image fell through to thedefault:"processing was interrupted, retrying" branch and was silently reprocessed on every subsequent container start — repeated identical error logs and repeated backend failure reports, forever.What changed
Incompletecase in the reprocessing switch, version-gated like the existingLearningcase, plus afailureRetriescounter (handleGenericFailure) that only pins an imageIncompleteaftermaxScanRetries(3) consecutive failures — a single transient error (a sidecar blip, a registry hiccup) is self-healing, not permanent.failureRetriesis a bounded, TTL'dexpirable.LRU(1000 entries, 30 min) rather than a plain map, so it doesn't leak entries for images seen once and never again.storage.SbomClient.PatchSBOMAnnotations— a JSON merge patch onmetadata.annotationsonly, which structurally can never sendspec. This matters because the reprocessing path fetches the SBOM viaGetSBOMMeta, which the storage layer returns without its Spec (a metadata-only fetch) — persisting that object via a fullReplaceSBOMwould silently destroy a previously-successful SBOM's real content (its CVE-scan data) the moment a later reprocess attempt failed. All SBOM-generation failure paths (generic scan/source/syft errors, repeated sidecar OOM crashes, andErrImageTooLarge) now go through this patch-based marking, so retries are bounded uniformly whether or not the image previously had content — no separate case for "already had good data" is needed anymore.Residual trade-off (documented, not fixed here)
failureRetries' TTL resets on every write, so the retry count only accumulates for failures spaced under 30 minutes apart. This fully closes the loop for the reported symptom (a tight failure/crash-loop cadence) but a permanent failure recurring more slowly than that (e.g. an infrequently-restarted long-lived pod) could in principle still reprocess indefinitely without ever hitting 3 consecutive failures. Flagged for follow-up if it turns out to matter in practice.Additional Information
This is a pre-existing, independent defect discovered while working on #853/#854 (the digest-panic crash fix) — it already affected ordinary syft SBOM-generation failures, unrelated to digests. It's deliberately split into its own PR to keep each change minimal and independently reviewable, and does not depend on #854.
How to Test
Test_processContainerWithMetadata_IncompleteReprocessingreproduces the bounded-retry behavior:maxScanRetries-1failures don't touch storage, themaxScanRetries-th does, and a version bump grants a fresh retry budget.Test_processContainerWithMetadata_PreservesContentOn{ReprocessFailure,ScannerCrash,TooLarge}seed a content-bearing SBOM and drive each failure path past its retry threshold, asserting the SBOM's real content survives (only its annotations change) — each was verified to actually fail (reproducing the underlying data-loss bug) when its corresponding fix is reverted.Related issues/PRs
Checklist before requesting a review
Summary by CodeRabbit
Bug Fixes
Tests
Summary by CodeRabbit