Skip to content

fix(shutdown): show All saved only after a successful backup - #1503

Open
khaira777 wants to merge 2 commits into
mainfrom
fix/shutdown-backup-saved-status
Open

khaira777 wants to merge 2 commits into
mainfrom
fix/shutdown-backup-saved-status

Conversation

@khaira777

Copy link
Copy Markdown
Contributor

Related work

Issue / Discussion: None (follow-up from an audit of recent main commits, finding F-06a/b)

Summary

backupOnShutdown ignored the backupForSession result and the 7s deadline branch, and always ran void showStatus('saved', true). A failed, skipped or timed-out shutdown backup therefore still showed "All saved". The unawaited call also raced holdStatus(SHUTDOWN_MESSAGE_VISIBLE_MS), which measures from messageAt, so after a slow backup the message could flash or never paint before exit.

Now the deadline resolves to false, "All saved" is shown only when the backup succeeded, and that call is awaited so holdStatus keeps it visible. On failure the window keeps "Taking a backup" until exit. The database itself is already durable, and the startup session backup retries on the next launch because isSessionBackupWanted is still true.

Scope

No new status message (adding one would need translations in all 27 status tables). localBackup.stop() ordering is unchanged; it does not affect the shutdown backup, which uses its own AbortController.

Verification

Commands run:

  • npm run lint, npm run build
  • npm run test:status-messages, npm run test:local-backup, npm run test:shutdown-lifecycle
  • oxfmt --check main/index.ts, git diff --check

Results: all passed. Not run: a manual quit of a packaged app with a forced backup failure. backupOnShutdown is private to main/index.ts, so there is no unit test.

Time budget: the 7s backup deadline plus the 450ms MIN_DWELL_MS plus one executeJavaScript stays within the 10s SHUTDOWN_TIMEOUT_MS per-step limit.

Compatibility & data safety

  • Database/data impact: None
  • Migration: None
  • User-visible behavior: "All saved" appears only after a successful shutdown backup, and stays visible about 1.5s
  • Offline/network impact: None

Cross-cutting surfaces

Surface Status
UI affected (shutdown status window)
API not affected
Persistence not affected
Upgrade path not affected
Permissions not affected

Developer documentation

  • No developer documentation change needed
  • Existing developer documentation updated
  • New page justified
  • Not applicable (docs-only or code-only change)

Contributor checklist

  • This is a small isolated fix/doc update OR follows an approved issue/direction.
  • Unrelated cleanups, refactors, and dependency changes were kept out of this PR.
  • Tests were added or updated for any behavior changes. (No automated test; see Verification.)
  • Verification commands were run and documented above.
  • Customer data and upgrade safety were preserved (if touching database/storage).
  • Documentation or translations were updated where relevant.
  • I understand and can explain all submitted changes.

backupOnShutdown ignored the backupForSession result and the deadline branch, so a failed, skipped or timed-out shutdown backup still showed All saved. It now shows the message only when the backup succeeded, and awaits it so holdStatus keeps it on screen before exit.
@coderabbitai

coderabbitai Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Show “All saved” only after a successful shutdown backup

🐞 Bug fix 🕐 Less than 10 minutes

Grey Divider

AI Description

• Prevent failed, skipped, or timed-out shutdown backups from displaying “All saved”.
• Wait for the saved status to render before holding it on screen during shutdown.
Diagram

graph TD
  A["Shutdown coordinator"] --> B["Taking backup"] --> C["Session backup"] --> D{"Succeeded by deadline?"} -->|yes| E["All saved"] --> F["Hold status"]
  D -->|no| F
Loading
High-Level Assessment

Using the existing boolean backup result and status-window behavior is the smallest appropriate fix. A new failure status would require translations without improving backup safety.

Files changed (1) +7 / -4

Bug fix (1) +7 / -4
index.tsGate shutdown success status on the backup result +7/-4

Gate shutdown success status on the backup result

• Makes the seven-second deadline resolve as failure and checks the result of the session backup. Awaits “All saved” only on success, so the subsequent shutdown hold can keep the rendered message visible.

main/index.ts

@qodo-code-review

qodo-code-review Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. A stalled status window aborts shutdown ✓ Resolved

Resolving commit: 53fd368

Resolution reason
The await on showStatus is raced against a timeout (SHUTDOWN_MESSAGE_VISIBLE_MS), so if the status
window stalls, backupOnShutdown proceeds after the timer instead of blocking until the shutdown
deadline.
Description
backupOnShutdown now awaits showStatus('saved', true), whose executeJavaScript call has no
timeout. If the status renderer stalls after a successful backup, the shutdown step reaches its
fatal ten-second limit and exits before the later database-close step runs.

Grey Divider

Context sources
✅ Compliance rules (platform): 5 rules
✅ Cross-repo context — repo relationships
✅ REVIEW.md
Review mode: Auto: ⚖️ Balanced: Small single-hunk change, but touches shutdown backup timing and status logic with no tests.

Grey Divider

Tip of the day
💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread main/index.ts Resolved
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