Skip to content

Join errors so they are detectable by errors.Is#4934

Open
casey-tran wants to merge 1 commit intomainfrom
hackathon/context-deadline-make-fatal
Open

Join errors so they are detectable by errors.Is#4934
casey-tran wants to merge 1 commit intomainfrom
hackathon/context-deadline-make-fatal

Conversation

@casey-tran
Copy link
Copy Markdown
Contributor

@casey-tran casey-tran commented Apr 29, 2026

Description:

The purpose of this PR is to have both of the errors emitted by their log lines be joined so that they are detectable by isFatal in `handlers.go.

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

Note

Low Risk
Small change limited to error composition in non-archive chunk processing; main risk is altered error strings/unwrap structure affecting tests or log expectations.

Overview
Updates defaultHandler.handleNonArchiveContent error wrapping to use errors.Join so emitted chunk read/write errors retain ErrProcessingWarning/ErrProcessingFatal in the error chain and can be detected via errors.Is (e.g., by isFatal).

This changes the formatting of the propagated errors to wrap the joined sentinel+underlying error as the %w cause while keeping the log message text stable ("error reading chunk" / "error writing to data channel").

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

@casey-tran casey-tran requested a review from a team April 29, 2026 18:20
@casey-tran casey-tran requested review from a team as code owners April 29, 2026 18:20
@casey-tran
Copy link
Copy Markdown
Contributor Author

A concern is that the error error reading chunk can be caused by a context deadline being exceeded, which we want to treat as a fatal error in isFatal but since the intention of ErrProcessingWarning is to have an error that is able to move on, we seem to have a divergence of intent.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f7771e8. Configure here.

Comment thread pkg/handlers/default.go
if err := chunkResult.Error(); err != nil {
h.metrics.incErrors()
dataOrErr.Err = fmt.Errorf("%w: error reading chunk: %v", ErrProcessingWarning, err)
dataOrErr.Err = fmt.Errorf("error reading chunk: %w", errors.Join(ErrProcessingWarning, err))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Joined warning errors may be misclassified as fatal

Medium Severity

Using errors.Join(ErrProcessingWarning, err) preserves the full error chain of the inner err. If that error wraps context.Canceled or context.DeadlineExceeded (possible when the underlying reader is a network stream or pipe), isFatal will match the first switch case (fatal) before reaching the ErrProcessingWarning case (non-fatal). This causes a supposedly recoverable warning to terminate file processing, contradicting the explicit continue on the next line. The old %v formatting intentionally broke the inner error chain to prevent this.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f7771e8. Configure here.

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