Skip to content

bugfix: fatal errors do not keep underlying error#5363

Merged
MichaelEischer merged 4 commits into
restic:masterfrom
zmanda:fix-gh-5258-backup-exits-with-wrong-code-on-ctrl-c
Sep 24, 2025
Merged

bugfix: fatal errors do not keep underlying error#5363
MichaelEischer merged 4 commits into
restic:masterfrom
zmanda:fix-gh-5258-backup-exits-with-wrong-code-on-ctrl-c

Conversation

@konidev20

@konidev20 konidev20 commented Apr 19, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR change? What problem does it solve?

  • Modified the fatal errors struct to preserve the underlying error.
  • Added Unwrap method to help errors.Is and errors.As comparisons.
  • Added tests to check if the fatal error preserves the underlying errors

Was the change previously discussed in an issue or on the forum?

Closes #5258

Checklist

  • I have added tests for all code changes.
  • I have added documentation for relevant changes (in the manual).
  • There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • I'm done! This pull request is ready for review.

MichaelEischer
MichaelEischer requested changes Jun 2, 2025
View reviewed changes

@MichaelEischer MichaelEischer left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The change itself looks fine. However, while looking for cases that could regress by merging this PR, I've stumbled on the following pattern: errors.Fatal(err.Error()). This prevents this PR from working in somewhat random places and thus has to be replaced with errors.Fatalf("%s", err). Except for this exact pattern, I haven't found any places that use errors.Fatal in a creative way.

@konidev20 konidev20 force-pushed the fix-gh-5258-backup-exits-with-wrong-code-on-ctrl-c branch from 4ba82c3 to 5b56087 Compare June 3, 2025 09:04
@konidev20 konidev20 requested a review from MichaelEischer June 3, 2025 09:05
@konidev20

konidev20 commented Jun 3, 2025

Copy link
Copy Markdown
Contributor Author

The change itself looks fine. However, while looking for cases that could regress by merging this PR, I've stumbled on the following pattern: errors.Fatal(err.Error()). This prevents this PR from working in somewhat random places and thus has to be replaced with errors.Fatalf("%s", err). Except for this exact pattern, I haven't found any places that use errors.Fatal in a creative way.

I have updated the code changes for the same. Please review at your leisure.

@konidev20

konidev20 commented Sep 5, 2025

Copy link
Copy Markdown
Contributor Author

Hi @MichaelEischer, bubbling this PR up since I see you are reviewing some PRs.

@konidev20
errors: enhance fatalError type to include underlying errors
79c4196
@konidev20
tests: add tests for preserving underlying errors
18b8f88
@konidev20
changelog: add bugfix changelog for issue-5258
576d35b
@konidev20
errors: standardize error wrapping for Fatal errors
ce089f7 
* replace all occurences of  `errors.Fatal(err.Error())` with `errors.Fatalf("%s", err)` so that the error wrapping is correct across the codebase

* updated the review comments
@konidev20 konidev20 force-pushed the fix-gh-5258-backup-exits-with-wrong-code-on-ctrl-c branch from 5b56087 to ce089f7 Compare September 13, 2025 18:02
MichaelEischer
MichaelEischer approved these changes Sep 24, 2025
View reviewed changes

@MichaelEischer MichaelEischer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!

@MichaelEischer MichaelEischer merged commit 4edfd36 into restic:master Sep 24, 2025
20 of 22 checks passed
@MichaelEischer MichaelEischer mentioned this pull request Sep 24, 2025
Merged
4 tasks
@MichaelEischer

MichaelEischer commented Sep 24, 2025

Copy link
Copy Markdown
Member

I found a few more creative usages after merging this PR: #5528

@amrkmn amrkmn mentioned this pull request Jun 9, 2026
Closed
@BrewTestBot BrewTestBot mentioned this pull request Jun 9, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelEischer MichaelEischer MichaelEischer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Wrong error code returned on SIGINT

2 participants

@konidev20 @MichaelEischer