fix: surface the original analysis error instead of context cancellation - #5
Closed
DmitriyLewen wants to merge 2 commits into
Closed
fix: surface the original analysis error instead of context cancellation#5DmitriyLewen wants to merge 2 commits into
DmitriyLewen wants to merge 2 commits into
Conversation
…ncellation `errgroup.WithContext` cancels egCtx as soon as one analysis goroutine returns an error. The synchronous file walk uses the same egCtx for `limit.Acquire`, so after the cancellation the walk fails with `context.Canceled`, and that error was returned before `eg.Wait()`. As a result the real cause - e.g. a remote Maven repository returning 429 Too Many Requests, surfaced as a *types.UserError by aquasecurity#10693 - was masked by a generic "context canceled" on large trees where the walk is still running when the cancellation fires. Wait for the analysis goroutines first so `eg.Wait()` surfaces the original error, and only fall back to the walk error when the goroutines finished without error. Applied to the local (fs), image and vm artifacts, which all share this pattern introduced in aquasecurity#9538.
Verify that Inspect surfaces a fatal analyzer error (*types.UserError, e.g. a remote Maven 429) instead of the context.Canceled the file walk hits after errgroup cancels egCtx. Regression test for aquasecurity#10790.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When an analyzer fails with a fatal error during artifact analysis, the user could see a generic
context canceledinstead of the real cause.Artifact.Inspect/inspectLayer/ VMAnalyzerun file analysis througherrgroup.WithContext. The synchronous file walk uses the group'segCtxforlimit.Acquire. When one analysis goroutine returns an error,errgroupcancelsegCtx; the still-running walk then fails its nextAcquirewithcontext.Canceled, and that walk error was returned beforeeg.Wait()— which is where the original error actually lives.This regression was latent since the
sync.WaitGroup→errgroup.WithContextmigration (aquasecurity#9538) and became observable once aquasecurity#10693 made the Mavenpomanalyzer return a*types.UserErroron a remote429 Too Many Requests. It only reproduces on large/multi-module trees (e.g. Keycloak), where the walk is still in progress when the 429 fires — a single-pom.xmlproject surfaces the 429 correctly because the walk finishes first.The fix: always wait for the analysis goroutines first so
eg.Wait()surfaces the original error, and only fall back to the walk error when the goroutines finished cleanly. Applied consistently to thelocal(fs),imageandvmartifacts.Note: in the masking scenario the user now sees
analyze error: <real cause>instead of... semaphore acquire: context canceled. This is the intended change; no code depends on the old error text.Reproduced by scanning the Keycloak repository (186
pom.xmlfiles) with an empty~/.m2while Maven Central rate-limits the IP.Before
The actual 429 is invisible; the user only sees
context canceled, and the reported file is arbitrary (depends on timing).After
Related issues
semaphore acquire: context canceledinstead of the real error aquasecurity/trivy#10792Related PRs
*types.UserErrorthat exposed this)Checklist