Skip to content

Check restore overwrite permission before changing the database - #1449

Merged
WaylandYang merged 5 commits into
deeplethe:devfrom
Floating-Y:fix/cli-restore-preflight
Oct 27, 2026
Merged

WaylandYang merged 5 commits into
deeplethe:devfrom
Floating-Y:fix/cli-restore-preflight

Conversation

@Floating-Y

@Floating-Y Floating-Y commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Why

utopia restore --force was parsed but did not authorize directory replacement, and the non-empty-directory check ran after pg_restore --clean. The documented database overwrite guard was also missing, so an invalid restore target could be discovered after SQL changes.

Closes #1448, which records the dependency rationale. Related: #1444 covers the directory replacement case; this PR also protects database overwrite permission, validates targets before restoration and retains original files on failure.

What changes

  • Validate checksums, manifest versions, required components, target-path types and overwrite permission before changing targets. A non-empty database or data directory being replaced requires --force; --yes only skips confirmation. --dry-run performs the same initial checks without restoring either target.
  • Prepare replacement data on the target filesystem while retaining the old directory. After confirmation and preparation, repeat the preflight immediately before starting pg_restore; reject a changed physical target instead of silently redirecting prepared data.
  • Always pass --single-transaction --exit-on-error. SQL restore errors roll back the database transaction; --clean --if-exists is added only with --force.
  • Install the prepared directory after database success, without merging files. Attempt to restore the old directory if installation fails, and retain/report its location if that rollback also fails. Database-only archives leave existing data untouched.
  • Preserve key handling, including environment-key archives whose manifest records the include-key option without a disk key. Warn from actual restored files rather than keeping an old key.
  • Reuse workspace sqlx, tokio and tempfile, plus test-only utopia-store for the required database guard. No package versions change. The ordinary CI test job installs matching PostgreSQL 16 clients.

Maintainer review follow-up

Implements the four requests in the review:

  1. Run the second preflight after the prompt and replacement-data preparation, immediately before database restoration. New interactive regressions change the database or directory while the CLI waits for confirmation and require refusal with zero restore calls.
  2. Add a single transaction and verify a real SQL failure leaves the target value at 99, rather than restoring the archived 7. Retain the original data directory.
  3. Keep archive-extraction staging in the current working directory with RAII cleanup, as before. The recording double checks this location. Replacement-data staging remains beside the target for installation on the same filesystem.
  4. Use Chinese for comments introduced by this PR, including the tests.

Why the code and tests exceed 500 added lines

Code and tests add 1,162 lines, including 926 lines of integration fixtures and assertions. Database overwrite protection, file replacement and shared preflight must land together to keep the destructive operation consistently guarded. A recording command verifies call order and file retention; actual PostgreSQL restoration verifies transaction rollback. The suite covers the eight requested safety behaviors, confirmation-time changes and concrete key/path regressions. Duplicate missing-key and absent-target scenarios were removed; the short command-failure and file-installation rollback cases cover different failure stages. Confirmation synchronization uses the existing fixture and a bounded wait, without a second test framework.

How it was checked

Revision: 39c5d8ffa3ae4fe773101f7505d016186aafd378, based on dev at c8aedf9.

  • Linux x86_64, Debian bookworm on Docker/WSL2; Rust/Cargo 1.98.1; PostgreSQL clients/server 16.15; Poppler 22.12 with CJK data: cargo test --locked --workspace --no-fail-fast passed: 1,760 passed, 0 failed, 6 existing ignored, across 46 test targets. Both required-test guards were enabled after migrating a dedicated disposable cluster. SHA256 checks matched all four revised files to the tested source snapshot.
  • The workspace run includes 15 CLI unit tests and 17 CLI integration tests, with no CLI failures or ignored cases. The real SQL-error regression verifies transaction rollback and intact old files; interactive regressions verify zero restore calls after confirmation-time changes. Each fixture creates and removes its own database. No business database was used.
  • Before changing production code, both targeted confirmation regressions failed on 343a6e4: a changed directory still reached the restore command, and a changed database was accepted. The expanded 17-case suite at that point had 7 passes and 10 failures, including wrong staging and the observed 7 != 99 SQL rollback assertion.
  • An isolated wrong-prompt diagnostic failed within 32.7 seconds, and confirmed that the CLI child had been reaped. Normal tests synchronize on the actual prompt rather than sleeping.
  • Windows x86_64, Rust/Cargo 1.98.0: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and the committed diff check passed.
  • The six ignored workspace tests are existing live OCR/HTTPS acceptance and opt-in delivery/process-crash experiments; they were not enabled manually. Windows-native real PostgreSQL restoration and power-loss / rollback-failure / disk-full injection were not run.
  • Frontend and bench checks were not repeated locally for this backend change. The latest full PR CI passed at 39c5d8f (backend, test, lint, build, migrations and web).

Database and filesystem restoration are not atomic. SQL execution errors roll back the database transaction, but a successful database commit can be followed by a failed directory operation. Preflight does not lock out concurrent writers. Stop writers, retain a pre-restore backup, and follow the documented retry/manual-recovery steps for interruption or later filesystem failures. In-place --clean replaces archived objects rather than removing unrelated database objects.

Before review

  • Every commit is signed off (git commit -s).
  • Latest format, workspace Clippy and full workspace tests passed locally.
  • Latest full PR CI passed at 39c5d8f.
  • Added database tests use utopia_store::test_db::url(), run in the ordinary workspace suite, and are not ignored.
  • Dependency use is described in Validate CLI restore overwrite permission before changing the database #1448; the PR targets dev.

Signed-off-by: Floating-Y <118035379+Floating-Y@users.noreply.github.com>
Signed-off-by: Floating-Y <118035379+Floating-Y@users.noreply.github.com>
Signed-off-by: Floating-Y <118035379+Floating-Y@users.noreply.github.com>

@WaylandYang WaylandYang 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.

Thanks, this fixes the ordering properly and covers #1444's case too. Small asks before merge: (1) re-run the preflight right before pg_restore (after the prompt), so a target that changed meanwhile is refused before the database is touched; (2) add --single-transaction alongside --exit-on-error, so a SQL failure rolls the database back; (3) keep the staging dir in the current directory as before (or explain the move to the system temp dir); (4) switch the new code comments to Chinese.

Signed-off-by: Floating-Y <118035379+Floating-Y@users.noreply.github.com>

@WaylandYang WaylandYang 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.

Thanks, all four are in: the preflight runs again right before pg_restore and refuses a target that moved, --single-transaction rolls a failed SQL restore back, staging is back in the current directory, and the comments are Chinese. Approving and merging.

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.

2 participants