Skip to content

fix AR mime-type reader error wrap to preserve errors.Is identity#4932

Open
johnelliott wants to merge 1 commit intomainfrom
polecat/nitro-mok93hrf
Open

fix AR mime-type reader error wrap to preserve errors.Is identity#4932
johnelliott wants to merge 1 commit intomainfrom
polecat/nitro-mok93hrf

Conversation

@johnelliott
Copy link
Copy Markdown
Contributor

@johnelliott johnelliott commented Apr 29, 2026

Summary

Same-shape follow-up to PR #4929. Switch the inner %v to %w at pkg/handlers/ar.go:101 so the error returned from newMimeTypeReader stays inspectable via errors.Is after being wrapped with ErrProcessingWarning.

Why

Reachability re-audit confirms the path:

bufReader.Peek -> io.SectionReader over the deb library reader
              -> BufferedReadSeeker.ReadAt
              -> the original io.Reader passed to HandleFile

For source readers backed by HTTP/GCS-style transports, parent-context cancellation surfaces here as a Read error whose chain satisfies errors.Is(err, context.DeadlineExceeded) (or context.Canceled). The %v wrap strips that inner identity, so isFatal at pkg/handlers/handlers.go:482 falls into the ErrProcessingWarning branch and classifies the cancelled/deadline-exceeded read as a non-fatal warning.

Blast radius is lower than the sibling ar.go:109 fix because processARFiles checks ctx.Done() at the top of each loop iteration, so the next iteration catches the cancellation anyway. This change is a symmetry/correctness fix that lets the classifier act on the actual cause rather than the outer warning sentinel.

What changed

  • pkg/handlers/ar.go:101: %v -> %w on the inner error.
  • pkg/handlers/ar_test.go: regression test TestARHandler_MimeTypeReaderErrPreservesIdentity that drives processARFiles directly with a crafted *deb.Ar whose first entry's data section returns the target error from ReadAt. Table-driven over context.DeadlineExceeded, context.Canceled, and io.ErrUnexpectedEOF (io.EOF is filtered inside newMimeTypeReader and never reaches the wrap site). Each case asserts the outer ErrProcessingWarning wrap is preserved, the inner cause is inspectable via errors.Is, and isFatal classifies based on the inner cause.

Test plan

  • go test ./pkg/handlers/ -count=1 passes
  • Reverting the fix to %v makes the new test fail on errors.Is(warnErr, tc.innerErr) and isFatal for the fatal cases
  • golangci-lint clean on the changed package

Note

Low Risk
Small error-wrapping change plus targeted tests; behavior change is limited to how AR entry read failures are classified via errors.Is.

Overview
Adjusts AR archive handling to wrap newMimeTypeReader failures with %w (instead of %v), preserving the underlying error in the errors.Is chain when emitting ErrProcessingWarning.

Adds a regression test that drives processARFiles with a crafted deb.Ar reader that injects specific ReadAt errors, asserting the inner error remains detectable via errors.Is and that isFatal classifies context cancellation/deadlines as fatal while treating io.ErrUnexpectedEOF as non-fatal.

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

The wrap at pkg/handlers/ar.go:101 used %v on the inner error returned
from newMimeTypeReader, dropping its identity from the errors.Is chain.
The reachability path is bufReader.Peek -> io.SectionReader over the
deb library reader -> BufferedReadSeeker.ReadAt -> the original io.Reader
passed to HandleFile. For HTTP/GCS-backed source readers, parent-context
cancellation surfaces as a Read error whose chain satisfies
errors.Is(err, context.DeadlineExceeded). The %v strips that inner
identity, so isFatal at handlers.go:482 misclassifies the wrapped error
as a non-fatal warning.

Switch to %w to match the same-shape fix already applied at
pkg/handlers/default.go:137 and pkg/handlers/ar.go:109. Blast radius is
lower than ar.go:109 because processARFiles checks ctx.Done() at the top
of each loop iteration, so the next iteration backstops cancellation.
The fix is a symmetry/correctness change that lets isFatal classify the
wrapped error against its actual cause.

Add a regression test that drives processARFiles directly with a
crafted *deb.Ar whose first entry's data section returns the target
error from ReadAt. The table covers context.DeadlineExceeded,
context.Canceled, and io.ErrUnexpectedEOF (io.EOF is filtered inside
newMimeTypeReader and never reaches the wrap site). Each case asserts
the outer ErrProcessingWarning wrap is preserved, the inner cause is
inspectable via errors.Is, and isFatal classifies based on the inner
cause.
@johnelliott johnelliott force-pushed the polecat/nitro-mok93hrf branch from 76f94e8 to 816029e 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
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.

1 participant