Repository navigation
feat(recovery): bind database restore to journal - #357
Conversation
Test-First: src-tauri/src/delivery_recovery/restore.rs::foreign_backup_never_replaces_the_journal_database Test-First: src-tauri/src/delivery_recovery/restore.rs::modified_backup_never_replaces_the_journal_database Co-Authored-By: Codex (GPT-5) <noreply@openai.com>
Test-First: src-tauri/src/delivery_recovery/restore.rs::journal_bound_backup_restores_before_write_resume Test-First: src-tauri/src/delivery_recovery/restore.rs::restore_is_refused_after_writes_resumed Co-Authored-By: Codex (GPT-5) <noreply@openai.com>
No-Test: append-only activity handoff Co-Authored-By: Codex (GPT-5) <noreply@openai.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e2252e161
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let bytes = fs::read(&snapshot.path) | ||
| .map_err(|error| format!("failed to read recovery snapshot: {error}"))?; |
There was a problem hiding this comment.
When a legitimate SQLite backup is large relative to available RAM, fs::read allocates the entire snapshot and retains it throughout the subsequent temporary-file copy. The existing restore path streamed from a file handle, so this change can make the recovery helper abort from memory exhaustion even when sufficient disk space is available. Hash while streaming from the already-open snapshot into the temporary file, then atomically replace the destination only after the digest matches.
Useful? React with 👍 / 👎.
| crate::db_restore::restore_verified_snapshot_bytes( | ||
| &snapshot.path, | ||
| &previous.database.path, | ||
| &bytes, | ||
| )?; |
There was a problem hiding this comment.
When validation fails after candidate installation has replaced the executable, this function reports a successful RestorePreviousRuntime after restoring only the database and never verifies or restores previous.binary. That leaves a candidate-binary/old-database combination rather than the journaled runtime pair, so the candidate may immediately remigrate the restored database or fail again. Restore and verify the previous binary before returning success, or narrow the action so a separate mandatory step cannot be skipped.
Useful? React with 👍 / 👎.
| if journal.writes_resumed { | ||
| return Err("refused: writes already resumed".into()); | ||
| } | ||
| if journal.next_action() != *action { | ||
| return Err("refused: restore is not bound to this journal".into()); |
There was a problem hiding this comment.
After a successful restore, the durable journal remains in RecoveryNeeded with writes_resumed == false and the same RestorePreviousRuntime action; there is no UpdateJournal transition that consumes that action or marks the restored runtime as resumed. If the previous runtime accepts new writes and the helper is then retried or restarted, this guard still passes and the adapter restores the old snapshot again, discarding those writes. Persist a restore-completed/resumed transition before writes can resume rather than relying on this unchanged flag.
Useful? React with 👍 / 👎.
| pub fn restore_previous_runtime( | ||
| journal: &UpdateJournal, | ||
| action: &RecoveryAction, | ||
| ) -> Result<(), String> { |
There was a problem hiding this comment.
At this commit, a repository-wide call search finds restore_previous_runtime only in this module's unit tests; RecoveryDriver::step still invokes RecoveryEffects::restore(previous, snapshot), whose signature has neither the journal nor the exact action needed for these checks. Consequently, any restore executed through the recovery driver bypasses the newly added journal binding, write-resumption guard, and snapshot rehash entirely. Wire this adapter into the driver/effect implementation so the protected path is the one production orchestration executes.
Useful? React with 👍 / 👎.
|
Queued — the merge queue status continues in this comment ↓. |
…tion The reader refactor made restore_from_reader borrow the backup file, so the handle stayed open across replace_file. Take the reader by value and drop it before the replace, as the comment above it promises. Test-First: src-tauri/src/db_restore.rs::db_restore::tests::restore_closes_the_source_before_replacing_the_destination Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 28 minutes 1 second in the queue, with no time running CI. ReasonThe pull request conflicts with the base branch The pull request was held in the queue, conflicting with 1 pull request ahead of it. The conflict became a base branch one after #361 merged. HintYou should update or rebase your pull request. If you want to requeue this pull request, you can post a Requeued — the merge queue status continues in this comment ↓. |
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 11 minutes 6 seconds in the queue, with no time running CI. ReasonThe pull request conflicts with the base branch HintYou should update or rebase your pull request. If you want to requeue this pull request, you can post a Requeued — the merge queue status continues in this comment ↓. |
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 8 minutes 55 seconds in the queue, with no time running CI. ReasonThe pull request conflicts with the base branch HintYou should update or rebase your pull request. If you want to requeue this pull request, you can post a Requeued — the merge queue status continues in this comment ↓. |
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 2 minutes 1 second in the queue, with no time running CI. ReasonPull request #357 has been dequeued Queue conditions are not satisfied:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Requeued — the merge queue status continues in this comment ↓. |
No-Test: generated activity record
No-Test: conflict resolution only; existing coverage follows in prepush
|
@Mergifyio queue |
No-Test: generated activity record
Cuarroc
added
the
review-ok
|
@Mergifyio queue |
☑️ Command
|
Merge Queue Status
This pull request spent 15 minutes 10 seconds in the queue, including 11 minutes 34 seconds running CI. Required conditions to merge
|
Der Merge-Konflikt von PR #357 mit dem aktuellen
mainist aufgelöst und der neue Stand ist gepusht.Die Konfliktlösung erhält sowohl die Restore- als auch die Installer-Moduldeklaration; zusätzliche Paketlogik wurde nicht eingeführt.
Der vollständige Linux-Prepush und der Push-Hook sind grün; die Windows-Prüfung bleibt für die Merge-Queue offen.
Report
What changed
origin/mainintocodex/w3-02c-restorewith merge commitba27fcb.src-tauri/src/delivery_recovery.rsby retaining both existing module declarations:installerfrommainandrestorefrom PR feat(recovery): bind database restore to journal #357.ee569df; no unrelated user work remains modified.Evidence
npm run dev:start-check— exit 0; 13.2 GB free RAM and one Cargo build (within the limit of two).bash scripts/ci/secret-scan.sh— exit 0; no leaks found.bash scripts/ci/gates.sh lane precommit— exit 0.bash scripts/ci/gates.sh lane prepush— exit 0; 485 frontend tests passed, 527 HQ tests passed, 1,784 Rust tests passed with 7 skipped, Clippy passed, and architecture drift reported no new occurrences.git push -u origin codex/w3-02c-restore— exit 0; its prepush hook also completed green.git ls-remote origin refs/heads/codex/w3-02c-restore— exit 0; remote SHAba27fcb0bb55f061cd819b060d9040f2fb23bcda.NICHT ABGEDECKT
#[cfg(windows)]tests or the Windows clippy arms (run_in_pty, npm_shim, ConPTY, DPAPI).Architecture drift
Review disposition
Conflict resolution
git merge origin/maininitially stopped because the generated.pa/ACTIVITY.mdentry was uncommitted (exit 2); after recording that entry inee569df, the merge completed with one source conflict.Open points