Skip to content

Add msgvault backup: init, create, list, verify, and restore commands (#339)#428

Merged
wesm merged 5 commits into
mainfrom
backup-restore-design
Jul 5, 2026
Merged

Add msgvault backup: init, create, list, verify, and restore commands (#339)#428
wesm merged 5 commits into
mainfrom
backup-restore-design

Conversation

@wesm

@wesm wesm commented Jul 4, 2026

Copy link
Copy Markdown
Member

Implements the backup and restore side of #339: msgvault backup init / create / list / verify / restore against an app-owned backup repository. Restore is included and proves itself before reporting success — see "Restore" below.

Docs: usage guide · format reference

How this maps to the #339 proposal

Kept as proposed:

  • msgvault owns the correctness boundary; retention, remote storage, and scheduling stay with restic/rclone/cron. The repository is a plain directory of write-once, immutable files — designed to be what those tools sync.
  • --include-config / --include-tokens flags, off by default. Added on top: capturing either into an unencrypted repository requires an explicit --allow-plaintext-secrets, so credentials can't leak by default.
  • Rebuildable derived state (vector db, analytics, caches) is excluded and the exclusion list is recorded in the manifest.
  • The manifest is written last, after all content is durable. A crash at any point leaves either a complete snapshot or no snapshot.
  • A missing or corrupt attachment fails the backup loudly: every attachment is re-read and re-hashed from disk at capture time, read from the storage path the database records for it (importers may namespace paths beyond the loose <aa>/<hash> layout).
  • The manifest records the msgvault version, backup format version, creation timestamp, capture options, attachment counts (content hashes live in dedicated list objects), message/conversation/account/label counts, and the exclusion list.

Where the design goes further than the proposal:

  • Snapshot repository instead of one backup directory. A repository holds many snapshots with content-addressed dedup: the database is captured at 4 KB page granularity (only changed pages stored after the first snapshot), and unchanged attachments cost nothing. Nightly backups scale with churn, not archive size, and you keep history rather than one restore point.
  • Consistency without VACUUM INTO. Instead of a full-copy snapshot, the backup checkpoints the WAL and pins a read transaction on its own connection, coordinating with a running daemon through a short freeze window (loopback API, watchdog-protected). No double-disk requirement, no downtime; the daemon resumes writes while pages are scanned. backup create refuses to run unfrozen against a live daemon.
  • Verification is stronger than existence + hash checks. verify (full mode) re-derives every referenced blob's SHA-256 from its bytes, proves the page map covers the database exactly, and cross-checks manifest counts. Failures name the damaged blob and the pack file holding it. --quick does structural checks; --all covers every snapshot, reading shared content once.
  • Versioning for the long haul. Compatibility is enforced at three levels (repository min_reader_version, per-object magic+version on every binary format, per-snapshot min_reader_version), so an older msgvault refuses a newer repository or snapshot with an explicit upgrade message instead of misreading it. Manifests are deterministic JSON; snapshot IDs are content-derived from the manifest bytes and re-checked on load, so a renamed or forged manifest is rejected.
  • Attachments are packed. Content goes into ~32 MB sealed pack files with per-blob zstd (skipped when content doesn't compress), so a 200k-file attachment tree syncs off-site as a few hundred immutable files. This is backup-repository-internal only — the live archive layout is unchanged.

Restore

backup restore --target DIR [SNAPSHOT] [--overwrite] materializes any snapshot into a usable archive home and does not trust itself:

  • Every database page is checked against the snapshot's page-hash map as it is written; every blob read re-derives its SHA-256 identity.
  • After materialization, the restored database must pass PRAGMA integrity_check and reproduce the manifest's recorded stats through exactly the queries capture ran inside the freeze; any mismatch fails the restore.
  • The end-to-end test asserts the restored database is byte-identical to the live file at capture time, for the latest snapshot and for a parent restored from an incremental chain.
  • Attachments land at the storage paths the restored database records; extras (deletions manifests, config, tokens) land at their captured relative paths and file modes, with traversal-free path validation.
  • A non-empty target is refused without --overwrite; overwrite merges but always removes the database and stale -wal/-shm sidecars first. Restoring into a running daemon's archive home is refused outright. Old backups go through normal schema migration on first open.

Performance

Page scanning hashes on all CPUs while reads stay strictly sequential (safe for NAS/spinning disks with no flag). Verify, restore, attachment capture, and changed-page packing fan out workers with a --jobs flag; --jobs 1 serializes reads for spinning-disk repositories. Progress renders as in-place bars on a TTY and plain lines through pipes, with per-stage elapsed time that keeps counting through long quiet stages such as the restore proof's integrity_check.

Changes

  • New packages: internal/pack (sealed pack container) and internal/backup (repository, locking, incremental page capture, manifests, verify, restore).
  • New CLI: msgvault backup init | create | list | verify | restore; [backup] config section (repo, zstd_level); --jobs on create/verify/restore; --progress rendering.
  • Daemon: loopback freeze endpoints with a 60s watchdog; the CLI proxy admits only backup create from the backup group — verify and restore always run locally.
  • Docs: user guide (docs/usage/backup.md) and on-disk format reference (docs/architecture/backup-format.md), including the roadmap for verify --restore-check, encryption, and retention.
  • TUI: launching against an archive with no usable analytics cache (a freshly restored one, for example) prints a notice and shows it while aggregate views load, instead of a silent slow first screen.

Sequenced later (format hooks in place)

  • Repository encryption (age-wrapped repo key, per-object AEAD) and forget/prune retention. Until prune ships, content purged from the archive persists in earlier snapshots — documented, with the privacy implications (including SQLite free-page remnants), in the usage guide.
  • verify --restore-check: the full restore materialization proof against scratch space without writing a target.

🤖 Generated with Claude Code

Comment thread internal/pack/crypter.go Fixed
@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (89ece82)

Medium confidence: the PR has two medium-severity issues that should be fixed before merge.

Medium

  • internal/backup/manifest.go:123
    LoadManifest trusts the JSON body without recomputing the content-derived snapshot ID or checking it against the requested filename. Corrupted manifest metadata can therefore be accepted by list, latest, and verify.
    Fix: After unmarshal, parse created_at, recompute ComputeSnapshotID, and reject if it differs from the requested ID or embedded snapshot_id.

  • internal/backup/frozen.go:187
    MAX(size) can be NULL because attachments.size is nullable. Scanning it into int64 can make backup creation fail for otherwise valid attachment rows that have content but missing size metadata.
    Fix: Use COALESCE(MAX(size), -1) or scan into sql.NullInt64 and let capture fill the actual size from the file.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 15m33s

@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (d86b7ad)

Medium severity findings:

  • internal/backup/attachments.go:109: Attachment capture reconstructs paths as <hash[:2]>/<hash> instead of using the DB storage_path / thumbnail_path. Existing SyncTech MMS imports store files under paths like synctech-sms/<prefix>/<hash>, so backup create can fail to read otherwise valid attachments.
    Fix: Carry the relative storage path from AttachmentRefs into capture, read that path after validation, and keep the hash-derived path only as a legacy fallback. Add a test with a prefixed storage_path.

  • internal/backup/fsutil.go:49: If the final directory sync fails after os.Rename, the object is left visible even though writeFileAtomic returns an error. Later operations trust visible manifests and indexes, so a retry can build on a file already reported as not durable; a subsequent crash can leave a child manifest or dedup reference pointing at an index/manifest that disappears.
    Fix: On post-rename SyncDir failure, remove the published final file best-effort and sync the directory again, or otherwise mark successfully published objects so later loads ignore files from failed publishes.

  • internal/backup/manifest.go:117: LoadManifest trusts the manifest JSON loaded from snapshots/<id>.mvmanifest without verifying that the file name, snapshot_id, and recomputed content-derived ID still match. A tampered backup repository could edit an existing manifest to point to older valid blobs while preserving the filename and snapshot_id, causing backup verify <snapshot> to verify the attacker-selected graph and report success for a snapshot ID that no longer represents its original contents.
    Fix: On manifest load, validate the requested ID equals m.SnapshotID, recompute ComputeSnapshotID from m.CreatedAt and the loaded manifest, and reject mismatches before verification or parent-chain traversal. Consider checking a longer digest than the current 8 hex chars for stronger tamper resistance.

  • internal/backup/pagehash.go:181: ApplyHashDelta allocates d.PageCount * pageHashSize bytes based on an untrusted page count from repository metadata after only checking integer overflow. A malicious repository can provide a tiny hash-delta object with a huge PageCount and no changed pages, causing backup verify or backup create to allocate enough memory to OOM.
    Fix: Validate delta growth before allocation. Reject deltas where PageCount grows beyond base.PageCount + len(d.Pages) or appended pages are not explicitly present, and impose a sane maximum supported database/hash-map size.


Reviewers: 2 done | Synthesis: codex, 15s | Total: 19m10s

wesm added a commit that referenced this pull request Jul 4, 2026
Two roborev findings on PR #428, both medium:

LoadManifest trusted the JSON body as-is, so corrupted, hand-edited, or
renamed manifest files were accepted by list, latest, and verify. It now
recomputes the content-derived snapshot ID and rejects any manifest whose
computed ID does not match both the requested filename and the embedded
snapshot_id. A side effect is that forged parent cycles can no longer
load at all (a cycle would need SHA-256 fixed-point IDs), so the mapChain
cycle tests are replaced by rejected-at-load coverage and the depth-limit
chain is built with genuine WriteManifest IDs; the verify geometry test
now models a buggy writer with a self-consistent ID, the only way wrong
geometry can still reach full verify.

AttachmentRefs scanned MAX(size) into int64, but attachments.size is
nullable: a content-bearing hash whose rows all lack size metadata made
backup create fail. COALESCE to the same -1 sentinel thumbnails already
use; capture resolves the real size from the file.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (7dc29e4)

Summary: Two medium-severity correctness/concurrency issues need attention; no high or critical findings were reported.

Medium

  • internal/backup/verify.go:336
    Full verification validates the hash-map chain and page-map chain independently, then checks only that page-map blobs exist and page ranges cover the manifest count. It does not validate pm.PageSize against the manifest or confirm that each mapped page’s bytes hash to the materialized page-hash-map entry. A checksum-valid but semantically incorrect page map could pass backup verify while restoring incorrect database bytes.
    Fix: Return the materialized hash map from checkHashMapChain, validate pm.PageSize == m.DB.PageSize, and in full mode walk page-map runs to bounds-check blob offsets and compare each mapped page’s PageHash against the hash map.

  • internal/backup/lock.go:132
    Stale lock reaping performs Stat and later Remove by path without serializing or rechecking ownership. If two processes recover the same stale exclusive.json, one can remove the other’s freshly planted lock between stale check and remove, allowing both operations to proceed as exclusive holders.
    Fix: Serialize lock-directory mutation with a guard/advisory lock, or atomically revalidate the specific lock identity immediately before unlinking and planting the replacement.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 14m29s

@wesm wesm force-pushed the backup-restore-design branch from 7dc29e4 to 620a2e5 Compare July 4, 2026 13:53
@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (620a2e5)

High-level verdict: changes need fixes before merge due to attachment backup correctness and snapshot consistency risks.

High

  • internal/backup/attachments.go:252
    Attachment capture hardcodes <hash[:2]>/<hash> instead of using attachments.storage_path. Existing SyncTech SMS MMS imports store files under synctech-sms/<hash[:2]>/<hash>, so backups for those archives will fail to read valid local attachments.
    Fix: Include storage_path/thumbnail_path in the frozen attachment refs and read from the recorded path, validating the hash from the bytes read.

Medium

  • internal/backup/frozen.go:62
    The daemon operation gate is released immediately after pinning the SQLite snapshot, but attachment files are read much later. A concurrent gated purge such as remove-account can delete files still referenced by the pinned DB snapshot, causing the backup to fail or no longer represent the frozen point.
    Fix: Keep a backup file-read guard active through attachment capture, or make attachment deletion defer/skip while a backup session is active.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 27m42s

@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (e78858e)

High-level verdict: the PR has one High-risk restore correctness issue and three Medium issues around backup consistency, WAL pressure, and extras path safety.

High

  • internal/backup/restore.go:151
    --overwrite accepts a non-empty target without clearing SQLite sidecars, so an existing msgvault.db-wal or msgvault.db-shm can remain next to the restored msgvault.db. The restore proof opens the DB with immutable=1, which ignores those sidecars, so restore can report success for a target that a normal msgvault/SQLite open may read differently.

    Fix: Remove msgvault.db-wal and msgvault.db-shm before proofing/reporting success, or restore into a clean temp directory and replace the target contents.

Medium

  • internal/backup/frozen.go:62
    The daemon operation gate is released immediately after pinning the SQLite snapshot, but attachment files are captured later from refs in that snapshot. A concurrent purge/remove-account can delete a file that is still referenced by the frozen DB image before CaptureAttachments reads it, causing backups of otherwise valid snapshots to fail.

    Fix: Keep a daemon-side guard that prevents attachment deletion until capture finishes, or make purge paths defer/remain blocked while a backup is using frozen attachment refs.

  • internal/backup/create.go:84
    The pinned SQLite read transaction is deferred until the end of Create, so it stays open through attachment capture, extras capture, pack sealing, index writing, and manifest writing. During long backups this prevents WAL checkpointing and can let the live WAL grow until the whole backup completes.

    Fix: Close the frozen session as soon as ScanPages and storePageBlobs finish reading the database bytes; use a separate guard for attachment deletion if needed.

  • internal/backup/restore.go:536
    restoreExtrasEntry allows any local relative extras path, and Restore writes extras after restoring the verified msgvault.db and attachments/ tree. An attacker who can tamper with an off-site/synced backup repository can add a valid manifest/index/pack whose extras tree names msgvault.db or attachments/<hash[:2]>/<hash>. Restore then overwrites previously verified files and can still report success, since the final DB proof only checks SQLite integrity/stats and attachments are not re-hashed after extras are written.

    Fix: Restrict extras restore paths to the capture-produced roots (deletions/, config.toml, tokens/, client_secret*.json) and explicitly reject msgvault.db, attachments/, duplicates, and overlapping paths. Alternatively, restore extras before core files and verify the final completed tree.


Reviewers: 2 done | Synthesis: codex, 15s | Total: 16m54s

@wesm wesm changed the title Add backup repositories: incremental, verifiable snapshots (part 1 of #339) Add backup and restore: incremental snapshots with a self-proving restore (part 1 of #339) Jul 4, 2026
@wesm wesm force-pushed the backup-restore-design branch from 17be3ed to f5473cc Compare July 4, 2026 16:44
@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (060a73c)

Medium-risk issues found; no Critical or High findings reported.

Medium

  • internal/backup/attachments.go:168: Attachment capture defaults to one worker per CPU, and each worker later reads and retains an entire attachment file while hashing/compressing it. A library with several large videos can hold many whole files in memory at once and OOM during a default backup create.

    Fix: Add a byte-weighted memory limit or stream hashing/compression, and consider a much smaller default parallelism for attachment capture.

  • internal/backup/restore.go:594: restoreExtrasEntry only checks that an extras path is local before writing it under the restore target. A tampered backup repository can include extras such as attachments/<aa>/<hash> or msgvault.db and overwrite already-restored archive files, allowing restore to report success while materializing attacker-controlled content.

    Fix: Reject extras paths that overlap managed restore outputs, including msgvault.db, SQLite sidecars, and anything under attachments/, or restore extras before DB/attachments while still reserving those paths explicitly.

  • internal/backup/manifest.go:91: Snapshot IDs use only the first 4 bytes of the manifest SHA-256. A repository writer can feasibly brute-force a 32-bit target, replace a known <snapshot-id>.mvmanifest, and have list, verify, or restore accept the forged manifest under the same ID.

    Fix: Include a substantially longer digest in the snapshot ID, at least 128 bits, or add a signed/MACed manifest if repository tamper resistance is intended.


Reviewers: 2 done | Synthesis: codex, 12s | Total: 17m19s

@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (b8bbeda)

High-level verdict: Changes need attention before merge due to plaintext backup exposure risk.

High

  • internal/backup/create.go:133
    Create builds the pack appender with a nil crypter while repositories are initialized with encryption = "none" and encrypted repos are refused. This means the new backup path writes message database pages and attachments into readable backup packs, while the docs recommend syncing that repository off-site. Anyone with read access to the synced repository, such as a cloud-sync provider or compromised backup destination, could restore or parse the backup and recover archived messages and attachments.

    Suggested remediation: implement and require authenticated repository encryption before recommending untrusted/off-site sync, or clearly gate/document plaintext repositories as trusted-storage-only. At minimum, avoid describing plaintext repositories as safe for off-site sync without a confidentiality warning.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 17m42s

@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (940d5fe)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 20m3s

@wesm wesm changed the title Add backup and restore: incremental snapshots with a self-proving restore (part 1 of #339) Add msgvault backup: init, create, list, verify, and restore commands (part 1 of #339) Jul 4, 2026
@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (f848d0d)

Summary verdict: one medium-severity restore-safety issue remains.

Medium

  • internal/backup/verify.go:620 — Full backup verify only checks that page-map blob IDs exist and can be read; it does not validate each page-map run’s BlobOffset/length bounds or that the mapped page bytes match the materialized page-hash map. A self-consistent bad snapshot can verify cleanly but later fail restore in writeRun.
    • Fix: In full verify, validate page-map runs against the raw page blobs and hash map, reusing or extracting the restore-side bounds and page-hash checks without writing files.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 21m27s

@wesm wesm changed the title Add msgvault backup: init, create, list, verify, and restore commands (part 1 of #339) Add msgvault backup: init, create, list, verify, and restore commands (#339) Jul 4, 2026
@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (8d96010)

Restore has one medium-severity path handling bug that should be fixed before merge.

Medium

  • internal/backup/restore.go:472: Relative restore targets are converted into root-relative SQLite URIs. For --target restore, restoreDBDSN("restore/msgvault.db") becomes file:///restore/msgvault.db?..., so the proof/attachment path query opens the wrong database and the restore fails.
    • Fix: Resolve non-drive-letter relative paths with filepath.Abs before building the file: URI, or make TargetDir absolute at the start of Restore; add a relative-target restore test.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 16m4s

@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (92e30d2)

Medium-risk issue found; no Critical or High findings.

Medium

  • cmd/msgvault/cmd/backup.go:364backup create uses urlFromDaemonRuntime(rt) for the daemon freeze API, but freeze handlers reject non-loopback RemoteAddr. If the daemon is bound to a specific non-loopback address like 192.168.x.x, the subprocess calls the freeze endpoint through that address and receives 404, preventing backups for an otherwise valid remote/NAS bind configuration.
    Suggested fix: Use a reachable loopback listener for freeze control, authenticate/relax the freeze endpoint for same-daemon subprocess calls, or reject and clearly handle specific non-loopback bind addresses for backup create.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 22m33s

@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (68a3c4b)

Medium findings:

  • internal/backup/create.go:122 - Create accepts a context, but large scan, changed-page packing, and attachment capture paths do not observe cancellation. Canceling backup create during these phases can leave work running until completion or daemon termination, keeping the repo lock fresh until stale-lock timeout. Thread ctx through ScanPages, storePageBlobs, and CaptureAttachments; check cancellation before dispatch/read work and return ctx.Err() after draining workers so cleanup defers run.

  • internal/backup/repo.go:156 - CleanStaging trusts <repo>/staging as a real directory. If another principal can replace staging with a symlink, the next cleanup can remove entries in the symlink target as the msgvault user. Validate repo structural directories with os.Lstat before use and refuse or replace symlinks; for staging cleanup, remove and recreate the staging path itself or otherwise operate only on a verified real directory under the repo root.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 25m7s

@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (879aaa9)

Medium-risk issues remain in backup restore safety and local attachment path filtering.

Medium

  • cmd/msgvault/cmd/backup.go:262
    The live-home restore guard only uses findDaemonRuntime, which ignores incompatible-but-running daemons. backup restore --overwrite --target <data_dir> can therefore overwrite a live archive owned by an older/newer daemon.
    Fix: Check for any live daemon runtime record for the data dir, including incompatible runtimes, before allowing restore into the configured archive home.

  • internal/backup/frozen.go:144
    storage_path NOT LIKE 'http%' and the thumbnail equivalent exclude any local attachment path starting with http, so a valid path like http_cache/aa/<hash> is omitted from backup and restore while the proof still passes.
    Fix: Exclude only actual remote URL prefixes, e.g. NOT LIKE 'http://%' AND NOT LIKE 'https://%', for both content and thumbnail paths.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 22m35s

Squashed branch commits:
- fix: verify progress bar tracks blobs, not snapshots
- feat: backup restore with layered self-proof
- fix: address roborev findings on delta safety, storage paths, and progress
- fix: gate namespaced-path snapshots behind manifest reader version 2
- fix: harden capture memory, extras placement, IDs, and map validation
- fix: report and parallelize the changed-page pack phase of backup create
- feat: show per-stage elapsed time and keep the restore proof line alive
- feat: explain slow first TUI views when no analytics cache exists
- fix: close extras traversal gap, stale ticker counts, cache-notice misfires
- docs: correct when the daemon builds the analytics cache in the TUI guide
- fix: report the daemon's analytics engine on /health for the TUI notice
- fix: make backup restore and its tests portable to Windows
- fix: resolve relative restore targets in the proof DSN, finish Windows lane
- fix: allow backup freeze from a daemon bound to a non-loopback address
- fix: honor cancellation in backup capture phases, refuse symlinked staging
- test: skip the staging symlink test where symlink creation is denied

Generated with Codex (GPT-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@wesm wesm force-pushed the backup-restore-design branch from 879aaa9 to 25035c8 Compare July 4, 2026 22:16
…ation

TestRetireGeneration_DisablesStatementTimeout failed on CI: the test
clamps statement_timeout=1ms on the pooled handle to prove the retire
tx overrides it, but the verification COUNT afterwards can be handed
the still-clamped pooled connection, and on a loaded runner even a
COUNT exceeds 1ms (SQLSTATE 57014). Both timeout tests now pin the
pool to one connection (the backfill timeout tests' existing pattern)
and RESET the timeout after the operation under test, so verification
never runs against the clamped session. Verified against a local
pgvector/pgvector:pg16 container.
@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (01999ba)

High-level verdict: changes are mostly sound, but there is one High restore-safety issue and one Medium backup completeness issue to address.

High

  • Location: cmd/msgvault/cmd/backup.go:262
  • Problem: backup restore only blocks restores into the live archive home when findDaemonRuntime finds a compatible daemon. A running incompatible daemon, such as one left over after an upgrade/downgrade, is ignored, so --overwrite can replace msgvault.db while that daemon still owns it.
  • Fix: Block on any live daemon runtime for the data dir, not only compatible ones. Check incompatible runtimes too, or use a helper that accepts any responding runtime.

Medium

  • Location: internal/backup/frozen.go:142
  • Problem: Attachment predicates exclude any local path beginning with http, not just URL-backed attachments. A valid namespaced local path like http-cache/aa/<hash> is omitted from capture and restore, and the matching stats predicate lets the backup still verify successfully while the restored DB points at a missing file.
  • Fix: Exclude only actual URL schemes, such as http://% and https://%, for both content and thumbnail paths, and add a regression test for a local http... namespace.

Reviewers: 2 done | Synthesis: codex, 1m1s | Total: 24m1s

Three review findings on the backup/restore path:

- The restore-into-live-home guard only detected daemons compatible with
  this client, so a daemon left running across a CLI upgrade or
  downgrade — which still holds the archive database open — was
  invisible and --overwrite could clobber its msgvault.db. The guard now
  blocks on any responding daemon via findAnyDaemonRuntime.

- The attachment predicates excluded storage paths with NOT LIKE
  'http%', which also dropped valid local namespaced paths like
  http-cache/aa/<hash> from capture and restore while the stats proof
  still passed (both sides shared the predicate). Only genuine http://
  and https:// URL schemes are excluded now, with predicate-level and
  end-to-end regression coverage.

- Restored files were fsynced but the directories naming them were not
  (only the target root was synced), so a crash shortly after a
  successful restore could lose newly created nested paths. Restore now
  syncs every directory of the restored tree, deepest first, before
  reporting success — one sync per directory instead of per-file parent
  fsyncs in the already fsync-bound attachments phase.
@roborev-ci

roborev-ci Bot commented Jul 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (76896f4)

Summary: One medium-severity issue remains; no critical or high findings were reported.

Medium

  • cmd/msgvault/cmd/backup.go:262: The live-archive restore guard compares filepath.Abs strings, so the same running daemon home can bypass the guard on case-insensitive filesystems when the target path is spelled with different casing.
    • Fix: Resolve both paths to filesystem objects and compare with os.SameFile when they exist, or use platform-aware canonicalization before comparing.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 18m47s

Two review findings on 76896f4:

- The live-archive-home guard compared filepath.Abs strings, so a
  case-variant spelling on a case-insensitive filesystem (the macOS
  default) or a symlinked path to the same home slipped past it and
  could restore over a running daemon's database. When the string
  compare misses, the guard now resolves both paths with os.Stat and
  compares filesystem identity via os.SameFile; a symlink-alias case
  pins the behavior.

- syncRestoredTree walked only under the target, so the target
  directory's own entry — and any missing ancestors os.MkdirAll created
  for it — were never fsynced in their parents; a crash after a
  successful restore into a brand-new path could still lose the whole
  tree. Restore now records the deepest pre-existing ancestor before
  creating the target and the durability pass climbs from the target's
  parent through that ceiling, keeping the deepest-first order.
@roborev-ci

roborev-ci Bot commented Jul 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (597c239)

Medium issue found; no high or critical findings.

Medium

  • internal/backup/frozen.go:90 - SQLite DSN is built via dbPath+"?_busy_timeout=5000", so a valid Unix database path containing ? can be parsed as a DSN query separator. The freeze/checkpoint session may open a different file than the later raw os.Open(opts.DBPath) scan, producing an unfrozen or unrestorable snapshot.
    • Fix: Build a proper file: URI with the path escaped and _busy_timeout=5000 in the query, similar to the restore DSN helper.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 23m45s

openPinnedSession built its SQLite DSN as dbPath+"?_busy_timeout=5000",
so an archive path containing '?' (legal on POSIX filesystems) was cut
at the first '?' and the driver opened — and created — a different,
empty database file. The freeze/checkpoint session would then pin the
wrong file while the raw page scan read opts.DBPath, producing an
unfrozen or unrestorable snapshot.

Extract restoredDBDSN's url.URL-based construction into sqliteURIDSN
(absolute, slash-rooted file: URI immune to '?'/'#' in the path) and
build the pinned-session DSN through it with _busy_timeout carried in
the query. Regression test opens a frozen session on an
"odd? archive#1.db" path and proves the pinned stats come from that
file.
@roborev-ci

roborev-ci Bot commented Jul 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (30da431)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 22m28s

@wesm wesm merged commit 18bede4 into main Jul 5, 2026
14 of 15 checks passed
@wesm wesm deleted the backup-restore-design branch July 5, 2026 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants