Skip to content

refactor(core): backup success after I/O-related errors#6856

Merged
romanz merged 1 commit into
mainfrom
romanz/2604/backup-success
May 2, 2026
Merged

refactor(core): backup success after I/O-related errors#6856
romanz merged 1 commit into
mainfrom
romanz/2604/backup-success

Conversation

@romanz

@romanz romanz commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

In case the host is unavailable, #6651 will stop sending ButtonRequests.

This PR makes sure that show_backup_success() won't fail if there was an error during the backup process - so the success screen will be shown to the user.

Currently THP debug builds fail with an assertion if the FW tries to write again before the previous write was ACKed, so this PR makes sure that after an error no ButtonRequest will be sent after #6651 is merged.

Note: writing the secret to storage is not affected by this change, since it doesn't access the active context / button request handler.

In case the host is unavailable, #6651 will stop sending ButtonRequests.

This PR makes sure that `show_backup_success()` won't fail if there was
an error during the backup process - so the success screen will be
shown to the user.

Currently THP debug builds fail with an assertion if the FW tries to
write again before the previous write was ACKed, so this PR makes sure
that after an error no ButtonRequest will be sent after #6651 is merged.

Note: writing the secret to storage is not affected by this change
(since it doesn't access the active context / button request handler).

[no changelog]
@romanz romanz self-assigned this Apr 30, 2026
@coderabbitai

coderabbitai Bot commented Apr 30, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a4a09f05-663a-46ef-aa06-cecda0161c8f

📥 Commits

Reviewing files that changed from the base of the PR and between 993dbad and 45c29c9.

📒 Files selected for processing (1)
  • core/src/apps/management/reset_device/__init__.py

Walkthrough

The change modifies error-handling scope in a backup process by expanding the continue_on_errors("Backup in progress") suppression context. Previously, this error suppression applied only to the backup info generation conditional. The reorganization now places the backup info generation, master secret/storage writes, and backup-success UI all within the same error-suppression context, ensuring I/O-related errors during the backup block don't propagate to subsequent operations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description explains the purpose and context well, but lacks the structured template elements (PR setup, development status markers) required by the repository guidelines. Add structured sections: assign yourself, add to Firmware project with Priority/Team/Sprint, and set appropriate development status (e.g., 'Needs Review').
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: moving backup success handling under error-suppression context to ensure UI displays even after I/O errors.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch romanz/2604/backup-success

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@trezor-bot trezor-bot Bot added this to Firmware Apr 30, 2026
@romanz romanz changed the title refactor(core): show backup success after I/O-related errors refactor(core): backup success after I/O-related errors Apr 30, 2026
@github-project-automation github-project-automation Bot moved this to 🔎 Needs review in Firmware Apr 30, 2026
@romanz romanz added the no-QA On PR-merge, automatically transition status in the "Firmware" project to "Done (no QA)" state. label Apr 30, 2026
@romanz romanz marked this pull request as ready for review April 30, 2026 08:47
@romanz romanz requested a review from obrusvit as a code owner April 30, 2026 08:47
@romanz romanz requested a review from mmilata April 30, 2026 08:47
@github-actions

Copy link
Copy Markdown

en main(all)

model device_test click_test persistence_test
T2T1 test(all) main(all) test(all) main(all) test(all) main(all)
T3B1 test(all) main(all) test(all) main(all) test(all) main(all)
T3T1 test(all) main(all) test(all) main(all) test(all) main(all)
T3W1 test(all) main(all) test(all) main(all) test(all) main(all)

Latest CI run: 25156240778

@obrusvit obrusvit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

utACK

The with continue_on_error(..) context extended to the whole perform_backup + show_backup_success interaction with writing to storage in between.

@romanz romanz merged commit 8d2016c into main May 2, 2026
111 of 157 checks passed
@romanz romanz deleted the romanz/2604/backup-success branch May 2, 2026 09:35
@trezor-bot trezor-bot Bot moved this from 🔎 Needs review to ✅ Done (no QA) in Firmware May 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-QA On PR-merge, automatically transition status in the "Firmware" project to "Done (no QA)" state.

Projects

Status: ✅ Done (no QA)

Development

Successfully merging this pull request may close these issues.

2 participants