Skip to content

fix nested archive handler error wraps to preserve errors.Is identity#4931

Open
johnelliott wants to merge 3 commits into
mainfrom
hackathon/handlers-ar-rpm-errwrap
Open

fix nested archive handler error wraps to preserve errors.Is identity#4931
johnelliott wants to merge 3 commits into
mainfrom
hackathon/handlers-ar-rpm-errwrap

Conversation

@johnelliott
Copy link
Copy Markdown
Contributor

@johnelliott johnelliott commented Apr 29, 2026

Summary

Two same-shape %v error wraps in nested archive handlers drop the inner
error's identity from the errors.Is chain, causing the
isFatal classifier in pkg/handlers/handlers.go to misclassify
cancellation and deadline-exceeded conditions as non-fatal warnings.
This is the identical bug fixed for the canonical chunk-reader wrap in
PR #4929.

Affected sites:

  • pkg/handlers/ar.go:109
  • pkg/handlers/rpm.go:120

Both wrap the return value of handleNonArchiveContent, which can be
ctx.Err() (context.Canceled / context.DeadlineExceeded) when its
internal CancellableWrite fails. The wrapped error feeds
isFatal at pkg/handlers/handlers.go:425. With %v, neither
errors.Is(err, context.Canceled) nor
errors.Is(err, context.DeadlineExceeded) matches, so the classifier
falls through to the ErrProcessingWarning case and processing
continues despite a fatal cause.

Fix

Change the inner %v to %w in both wraps, matching the shape used by
PR #4929. Multi-%w in a single fmt.Errorf is supported on Go
1.20+; this repo is on Go 1.24.

Behavior change

Cancelled or deadline-exceeded processing inside an AR or RPM nested
archive now correctly terminates instead of being logged as a warning
and continuing.

Tests

Added a regression test for each handler:

  • TestARHandler_NonArchiveErrPreservesIdentity in ar_test.go
  • TestRPMHandler_NonArchiveErrPreservesIdentity in rpm_test.go

Each test injects a chunk reader that cancels the parent context before
delivering its chunk, forcing handleNonArchiveContent's
CancellableWrite to return context.Canceled, which the surrounding
handler wraps. The tests assert the outer ErrProcessingWarning wrap is
preserved, the inner cancellation cause is inspectable via
errors.Is, and isFatal classifies the wrapped error as fatal.

Verified the tests catch the regression by reverting just the wrap
fix: both tests fail on errors.Is(err, context.Canceled) and
isFatal(err), then pass again once the fix is reapplied.

Test plan

  • go test ./pkg/handlers/ -count=1 passes
  • go vet ./pkg/handlers/ clean
  • Catches-the-bug verification: revert %w -> %v -> tests fail; reapply -> tests pass

Note

Low Risk
Small, targeted change to error wrapping plus new tests; behavior only changes for cancellation/deadline paths in AR/RPM processing and is unlikely to impact normal success flows.

Overview
Fixes AR and RPM nested archive handlers to wrap handleNonArchiveContent failures with %w (instead of %v) while still tagging them with ErrProcessingWarning, preserving errors.Is identity for underlying causes like context.Canceled/context.DeadlineExceeded.

Adds regression tests (TestARHandler_NonArchiveErrPreservesIdentity, TestRPMHandler_NonArchiveErrPreservesIdentity) that inject a cancelling chunk reader and assert both the outer warning wrap and the inner cancellation cause remain detectable and are classified as fatal by isFatal.

Reviewed by Cursor Bugbot for commit 7b66333. Bugbot is set up for automated code reviews on this repo. Configure here.

@johnelliott
Copy link
Copy Markdown
Contributor Author

johnelliott commented Apr 29, 2026

Question on the test assertions: any reason these use assert.True(t, errors.Is(warnErr, target), "...") rather than assert.ErrorIs(t, warnErr, target, "...")?

The testify helper produces a better diagnostic on failure (it prints the actual error chain), and we already import github.com/stretchr/testify/assert. Same applies to the matching test in rpm_test.go. Happy to leave as-is if there's a reason; just wanted to ask.

The non-archive content paths in pkg/handlers/ar.go:109 and
pkg/handlers/rpm.go:120 wrapped the inner error returned from
handleNonArchiveContent with %v, dropping its identity from the
errors.Is chain. Because handleNonArchiveContent can return ctx.Err()
(context.Canceled or context.DeadlineExceeded) when its CancellableWrite
fails, isFatal at handlers.go:425 then misclassified those cancelled
or deadline-exceeded conditions as non-fatal warnings and continued
processing.

Switch both wraps to %w so the inner cause is preserved alongside
ErrProcessingWarning, matching the same-shape fix already applied to
the canonical wrap at default.go:137.

Add regression tests for both handlers that exercise the production
wrap path via an injected chunk reader that cancels the parent context
before its chunk is written, which forces handleNonArchiveContent to
return context.Canceled and processARFiles / processRPMFiles to wrap
it. The tests assert the outer ErrProcessingWarning wrap is preserved,
the inner cancellation cause is inspectable via errors.Is, and isFatal
classifies the result as fatal.
@johnelliott johnelliott force-pushed the hackathon/handlers-ar-rpm-errwrap branch from f822d37 to e7d7f6f Compare April 29, 2026 17:14
@johnelliott johnelliott marked this pull request as ready for review April 29, 2026 17:27
@johnelliott johnelliott requested a review from a team April 29, 2026 17:27
@johnelliott johnelliott requested review from a team as code owners April 29, 2026 17:27
Copy link
Copy Markdown
Contributor

@rosecodym rosecodym left a comment

Choose a reason for hiding this comment

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

left some style notes

Comment thread pkg/handlers/ar_test.go Outdated
Comment thread pkg/handlers/rpm_test.go Outdated
- Drop stdctx alias on the stdlib context import; alias trufflehog's
  pkg/context as trContext to match the package idiom.
- Replace assert.True(t, errors.Is(...)) with assert.ErrorIs to get
  better diagnostics on failure.
@johnelliott johnelliott requested a review from rosecodym April 29, 2026 18:12
Comment thread pkg/handlers/ar_test.go Outdated
Address review nit on PR #4931: line numbers in regression-test
docstrings will go stale as the file evolves. Reference the symbolic
shape instead (dataOrErrChan ErrProcessingWarning send in
processARFiles/processRPMFiles' handleNonArchiveContent error path).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants