fix: unify SBOM scan-retry budgets to bound total attempts - #857
Conversation
scanRetries and failureRetries tracked crash and generic-failure counts independently, both compared against maxScanRetries, so an image alternating between failure categories could take more than maxScanRetries total attempts before ever reaching a terminal status. Unify both onto the existing TTL'd failureRetries LRU via a shared incrementFailureCount helper so the retry budget bounds total attempts per sbomName regardless of failure category. Docs-exempt: internal retry-counter bookkeeping, not previously documented behavior; no API/config/user-facing change Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
added
the
ai-assisted
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesSBOM retry budget
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant processContainerWithMetadata
participant handleScannerCrash
participant failureRetries
participant crashLoopRetries
participant PatchSBOMAnnotations
processContainerWithMetadata->>handleScannerCrash: Handle scanner crash
handleScannerCrash->>failureRetries: Increment shared failure count
handleScannerCrash->>crashLoopRetries: Increment crash-only count
handleScannerCrash->>PatchSBOMAnnotations: Mark Incomplete or contentless TooLarge
Possibly related PRs
🚥 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 |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed the unification itself and it's correct — the shared budget does bound total attempts, and the regression test genuinely fails on main. One blocker before merge, plus one design question and a test gap.
Blocker: sharing the counter also silently shares the classification, so a single sidecar OOM can now pin a contentless SBOM TooLarge — a strictly less recoverable state than the Incomplete it would have gotten before. Details inline; I verified this with a PoC test that passes on this branch and fails on main.
The PR body calls that edge "harmless -- a contentless TooLarge freezes no real content". That framing is about content preservation, but the actual cost of TooLarge here is recoverability, and by that measure it isn't harmless.
| func (s *SbomManager) handleScannerCrash(sbomName string, notif containercollection.PubSubEvent, scanErr error, imageTag, imageID string, hadContent bool) { | ||
| s.scanRetries[sbomName]++ | ||
| retryCount := s.scanRetries[sbomName] | ||
| retryCount := s.incrementFailureCount(sbomName) |
There was a problem hiding this comment.
Blocker — a single OOM can now pin a contentless SBOM TooLarge.
retryCount is now the combined budget, but the classification below it (retryCount >= maxScanRetries → TooLarge when !hadContent) still assumes the count was consumed by scanner crashes. It no longer is.
Concrete sequence on a brand-new image (never succeeded, so wipSbomHadContent == false):
- sidecar scan fails generically (transient) → count 1
- sidecar scan fails generically (transient) → count 2
- sidecar OOMs once →
incrementFailureCountreturns 3 →!hadContent→markSBOMStatus(TooLarge, {scannerMemoryLimit: ...})
PoC (drop into pkg/sbommanager/v1, scanner returns generic/generic/ErrScannerCrashed):
type genericThenCrashClient struct{ calls int }
func (a *genericThenCrashClient) CreateSBOM(_ context.Context, _ sbomscanner.ScanRequest) (*sbomscanner.ScanResult, error) {
a.calls++
if a.calls < 3 {
return nil, errors.New("scan failed")
}
return nil, sbomscanner.ErrScannerCrashed
}
func (a *genericThenCrashClient) Ready() bool { return true }
func (a *genericThenCrashClient) Close() error { return nil }
func Test_POC_SingleOOMAfterGenericFailuresPinsTooLarge(t *testing.T) {
fake := newFakeSbomClient()
mgr := &SbomManager{
cfg: config.Config{NodeName: "node-1"},
ctx: context.Background(),
processing: mapset.NewSet[string](),
storageClient: fake,
scannerClient: &genericThenCrashClient{},
metrics: metricsmanager.NewMetricsNoop(),
version: "v2.0.0",
scannerMemLimit: 1024,
failureRetries: newFailureRetries(),
}
notif, imageStatus, imageTag, imageID := testNotifAndImageStatus()
sbomName, _ := names.ImageInfoToSlug(imageTag, imageID)
for range 3 {
mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID)
}
raw := fake.get(sbomName)
assert.Equal(t, helpersv1.TooLarge, raw.Annotations[helpersv1.StatusMetadataKey])
}Result:
- this branch →
too-large,scannerMemoryLimit: "1024"— passes main(same test, plusscanRetries: make(map[string]int)) →initializing— fails, no terminal status pinned at all
Why this isn't the harmless edge the PR body describes — TooLarge is not merely "a status that freezes no content", it's the least recoverable terminal state in this file:
- the
TooLargecase inprocessContainerWithMetadatareturns early and, unlikeIncomplete, has no tool-version escape hatch —shouldRetryAtCurrentVersionis never consulted. A node-agent/syft upgrade re-attempts anIncompleteSBOM; it will never re-attempt aTooLargeone. - the only way out is
ScannerMemoryLimitAnnotation != scannerMemLimit, i.e. an operator changing the sidecar memory limit — for an image whose real problem was two transient scan errors. - and per the
wipSbomHadContentcomment above,GuaranteedUpdatesilently drops every future write oncestatus=too-large, so even that escape hatch can't rewrite the object.
Net effect: two transient sidecar errors plus one unlucky OOM permanently costs that image its SBOM (and therefore its vulnerability coverage), where before it would have landed on retryable Incomplete. That's a behavior change the issue didn't ask for, and it lands on first-time images — the common case, since hadContent is only true on the Learning reprocess path.
Suggested fix — keep the unified budget (it's the right call), but make TooLarge require crash attribution rather than "the crossing attempt happened to be a crash". Cheapest version: have incrementFailureCount also return whether every counted failure in the current window was a crash (a parallel bool/count in the same LRU value, e.g. a small struct), and only take the TooLarge branch when that holds. Simplest version if you'd rather not carry state: pin Incomplete unconditionally from the shared path and treat TooLarge as reachable only via syftutil.ErrImageTooLarge / the MaxSBOMSize check, which are the two places it's actually true by measurement.
| // handleGenericFailure and handleScannerCrash) and returns the new count. Once the count | ||
| // reaches maxScanRetries the LRU entry is removed, so a later successful attempt or a tool | ||
| // version bump starts with a fresh budget. | ||
| func (s *SbomManager) incrementFailureCount(sbomName string) int { |
There was a problem hiding this comment.
Design question — crash counting silently lost its only unbounded backstop.
The struct comment is upfront that scanner-crash counting now inherits the 30-min TTL reset, and calls it an intentional consistency choice. I'd push back on the choice itself, not the disclosure.
The untimed scanRetries map wasn't an oversight — the failure it bounds is qualitatively different. A generic failure is cheap and image-local. A sidecar OOM kills the shared scanner process (ReportSBOMScannerRestart, SetSBOMScannerReady(false)), which stalls scanning for every other image on the node. The untimed counter guaranteed such an image was eventually pinned and stopped costing the node.
With the 30-min TTL, any image whose container starts are spaced >30 min apart never accumulates to a pin: a CronJob on an hourly schedule, a slow rollout, node churn. Each start OOMs the sidecar again, indefinitely, and the counter is back to zero every time. That's an availability regression for the whole node, hidden behind a per-image consistency argument.
Options, cheapest first: bump failureRetryTTL (30 min is tuned for transient scan errors, not for "is this image fundamentally too big for the sidecar"); or don't refresh the TTL on the crash path; or keep the crash count on a separate longer-lived TTL and only unify the budget check.
Minor, same function: the doc comment says removal at threshold is what lets "a later successful attempt or a tool version bump start with a fresh budget." Neither is accurate — a successful attempt resets via s.failureRetries.Remove(sbomName) on the success path, and a tool-version bump doesn't touch the LRU at all. Worth correcting so the next reader doesn't rely on a mechanism that isn't there.
| scannerClient: &alternatingScannerClient{}, | ||
| metrics: metricsmanager.NewMetricsNoop(), | ||
| version: "v2.0.0", | ||
| failureRetries: newFailureRetries(), |
There was a problem hiding this comment.
Test gap. The new test seeds a content-bearing SBOM, so hadContent is true and the crossing attempt can only ever produce Incomplete. It proves the budget is shared but can't see the classification change the sharing introduces — the contentless path is exactly where the blocker lives, and it's currently uncovered.
Worth adding the mirror case: no seeded SBOM (hadContent == false), generic → generic → crash, asserting the status the fix decides on (Incomplete under either suggested fix), plus that ScannerMemoryLimitAnnotation is absent. That test fails on this branch today, which is the point.
Nit while you're here: the struct-field comment is 12 lines and the handleScannerCrash comment is 16, and both spend most of that pre-arguing with a reviewer ("That is an intentional consistency choice, not an oversight", "The one residual edge is honest to disclose"). Rationale like that belongs in the PR description or the commit message; the surviving code comment should just say what the invariant is. Roughly: "Shared by handleGenericFailure and handleScannerCrash; TTL-reset, so it bounds tight failure cadences rather than every possible one." The rest is recoverable from git blame.
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Address review feedback on PR #857: - Blocker: sharing one counter between generic failures and scanner crashes also silently shared the terminal-status classification, so a contentless image could be pinned TooLarge after just one crash mixed with two unrelated generic failures. TooLarge is now reachable only through a new crashLoopRetries counter that exclusively tracks scanner crashes, so a mixed threshold crossing always resolves to the safely-retryable Incomplete instead. - Design gap: unifying onto failureRetries' 30-min TTL meant scanner-crash counting lost the unbounded-time backstop it had before #857, letting a sparse-cadence crash loop (restarts >30min apart) stall the shared scanner sidecar indefinitely without ever pinning. crashLoopRetries uses a much longer TTL (24h) specifically so it survives the gaps a slow crash loop needs. Corrected an inaccurate doc comment on incrementFailureCount and trimmed the failureRetries struct comment per review feedback. Added regression tests for the mixed-category contentless path, the preserved pure-crash-loop TooLarge path, and the sparse-cadence backstop. Docs-exempt: internal retry-counter bookkeeping, not previously documented behavior; no API/config/user-facing change Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
|
Thanks — the blocker's PoC and the availability point are both fair, and I was wrong to frame the contentless-crash edge as harmless. Pushed a fix in 328e3e8:
|
![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_reprocessing_test.go (1)
471-552: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a
hadContent == truepure-crash-loop test to lock the content-protection branch.The three new tests exercise the contentless paths well, but the
!hadContent && crashLoopTriggeredguard inhandleScannerCrash— the invariant that a content-bearing SBOM must stayIncomplete(never theTooLargeone-way door) even under a pure crash loop — has no direct coverage.Test_..._MixedFailureCategoriesShareBudgetseeds content but uses the alternating client, so the sharedfailureRetriesbudget pinsIncompletebeforecrashLoopTriggeredis ever reached, leaving the crash-loop-with-content branch untested. A mirror ofTest_..._MarksContentlessImageTooLargeOnPureCrashLoopseeding a prior successful (Learning) SBOM and assertingIncomplete(and absence ofScannerMemoryLimitAnnotation) would guard this safety-critical branch against regressions.🤖 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_reprocessing_test.go` around lines 471 - 552, Add a pure-crash-loop regression test mirroring Test_processContainerWithMetadata_MarksContentlessImageTooLargeOnPureCrashLoop, but seed a prior successful Learning SBOM so hadContent is true. Invoke processContainerWithMetadata three times with sbomscanner.ErrScannerCrashed, then assert the result remains Incomplete and ScannerMemoryLimitAnnotation is absent, covering the !hadContent && crashLoopTriggered guard in handleScannerCrash.
🤖 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_reprocessing_test.go`:
- Around line 471-552: Add a pure-crash-loop regression test mirroring
Test_processContainerWithMetadata_MarksContentlessImageTooLargeOnPureCrashLoop,
but seed a prior successful Learning SBOM so hadContent is true. Invoke
processContainerWithMetadata three times with sbomscanner.ErrScannerCrashed,
then assert the result remains Incomplete and ScannerMemoryLimitAnnotation is
absent, covering the !hadContent && crashLoopTriggered guard in
handleScannerCrash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: feb39621-4dcf-4736-8219-b17afb8cfb23
📒 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. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed at 328e3e8. Both findings are resolved — the fix is the right shape, and I verified it independently rather than just reading the diff.
Splitting crashLoopRetries out as a crash-only counter on its own longer TTL addresses both findings with one mechanism, which is neater than either option I suggested: the shared budget keeps bounding total attempts (the original #856 fix), while TooLarge now requires crash-attributed evidence, and the 24h TTL restores the unbounded-time backstop that the 30-min TTL had removed.
Verification — I re-ran my original PoC verbatim against this commit and wrote three more for paths your tests don't cover:
| scenario | result |
|---|---|
| 2 generic + crash, contentless (the original blocker) | incomplete, no memory-limit annotation, exactly 1 patch ✅ |
| crash → generic → crash, contentless (3 combined, 2 crashes) | incomplete ✅ |
| content-bearing + sparse-cadence pure crash loop | incomplete, 2 artifacts intact, no memory-limit annotation ✅ |
go vet + full package, and 15× -race on the sleep-based test |
clean, no flakes observed ✅ |
That third row is the combination your three new tests don't hit — the long-TTL backstop crossing its threshold on an SBOM that does have content. The hadContent gate holds there, so the backstop can't route a content-bearing SBOM into TooLarge's one-way door. Good.
One interaction I chased and satisfied myself is a non-issue, noting it so it doesn't have to be re-derived later: when handleGenericFailure pins Incomplete, crashLoopRetries keeps its stale count for up to 24h, so in principle a later crash could cross the crash threshold on evidence that straddles a pin. It isn't reachable — an Incomplete pin blocks reprocessing at the same tool version, and the only escapes (version bump, memory-limit change) both imply a process restart, which starts with empty LRUs. No action needed.
Also confirmed every SbomManager construction site (1 production + 6 in tests) initializes the new LRU, so there's no nil-crashLoopRetries panic path into handleScannerCrash.
Only a nit left, inline. Nothing blocking — LGTM.
| version: "v2.0.0", | ||
| scannerMemLimit: 4096, | ||
| failureRetries: expirable.NewLRU[string, int](maxFailureRetryEntries, nil, 5*time.Millisecond), | ||
| crashLoopRetries: expirable.NewLRU[string, int](maxFailureRetryEntries, nil, 500*time.Millisecond), |
There was a problem hiding this comment.
Nit (non-blocking): the 500ms crashLoopRetries TTL is a fairly tight margin for a wall-clock-dependent test. The run needs 3 iterations plus 2×20ms sleeps to finish inside 500ms; a GC pause or a loaded CI runner stalling past that budget expires the backstop entry and flips the assertion to Incomplete — a confusing failure, since it looks like the feature regressed rather than the runner hiccuped.
I ran it 15× under -race without a flake, so this is speculative rather than observed. Still, widening to 5*time.Second costs nothing in runtime (the test doesn't wait on this TTL, it only needs it not to expire) and removes the failure mode entirely. The 5ms failureRetries TTL is fine as-is — that one you do wait on, and the 20ms sleep gives it 4× headroom.
There was a problem hiding this comment.
Resolved in 332442e — 5s, and the inline comment now explains why the value is generous (that the test never waits on this TTL) so it doesn't get "optimized" back down later. Stressed the retry/classification tests 20× under -race on the new head: clean, no flakes. Thanks.
…ke risk 500ms was a tight margin: the test needs 3 iterations plus 2x20ms sleeps to finish inside that window, and a GC pause or a loaded CI runner stalling past it would expire the backstop entry and flip the assertion to Incomplete -- a confusing failure that looks like a regression rather than a runner hiccup. The test never waits on this TTL (only failureRetries' 5ms TTL needs to actually expire), so widening it to 5s costs nothing and removes the failure mode. Docs-exempt: test-only change, no behavior or documented API change Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
1 participant
Overview
SbomManagertracked SBOM-generation retry attempts via two independent counters, both compared against the samemaxScanRetries(3):scanRetries(plain map, no TTL) — incremented only byhandleScannerCrashfor consecutive sidecar OOM crashes.failureRetries(TTL'd LRU) — incremented only byhandleGenericFailurefor consecutive generic SBOM-generation failures.Because the counters were independent, an image alternating between failure categories could take more than
maxScanRetriestotal attempts (e.g. 2 generic failures + 3 crashes = 5) before ever reaching a terminal status.This PR unifies both onto the existing TTL'd
failureRetriesLRU via a sharedincrementFailureCounthelper, so the retry budget now bounds total attempts persbomNameregardless of failure category.Two intentional behavior consequences are documented inline rather than left silent:
IncompletevsTooLarge) is now attempt-attributed — whichever handler's call crosses the combined threshold decides, based on that attempt. Safety is preserved by the existinghadContentgate, which is untouched: content-bearing SBOMs are always pinnedIncomplete, neverTooLarge.How to Test
The new
Test_processContainerWithMetadata_MixedFailureCategoriesShareBudgetreproduces the bug scenario from the issue: alternating generic-failure/scanner-crash calls now pin a terminal status at exactly the 3rd combined attempt instead of up to 5-6.Related issues/PRs
failureRetriesalongside the pre-existingscanRetriesChecklist before requesting a review
🤖 Generated with Claude Code
AI-skills: oh-my-claudecode:ralph,oh-my-claudecode:ai-slop-cleaner,oh-my-claudecode:cancel | cmds: /clear,/oh-my-claudecode:ralplan
Summary by CodeRabbit