feat(db): 一貫したスナップショットを書き出す backup_to を追加 - #44
Conversation
WAL モードのため単純なファイルコピーでは未反映のトランザクションが 取り残され、古い、または壊れた複製ができる。VACUUM INTO で writer 接続 から単一ファイルのコピーを作る API を公開する。 キーチェーンが永続しない環境では accounts.token に API トークンが平文で 残るため、複製側からトークンを落とすオプションも持たせる。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds ChangesDatabase backup export
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/db.rs (1)
1476-1478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that both WAL sidecars are absent.
The test checks
backup.db-walbut notbackup.db-shm, although the method documentation promises neither artifact is produced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 1476 - 1478, Extend the VACUUM INTO test assertions to also verify that the destination database’s `backup.db-shm` sidecar is absent, alongside the existing `backup.db-wal` check, preserving the documented guarantee that neither WAL artifact is produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db.rs`:
- Line 730: Update the backup flow around restrict_permissions so permission
enforcement returns and propagates set_permissions failures instead of being
ignored. Make restrict_permissions fallible, have its caller fail the backup
when owner-only permissions cannot be applied, and clean up the destination if
required by the existing backup error path.
- Around line 732-735: Update the strip_tokens branch in the backup flow to
write to a temporary destination, enable SQLite secure deletion and vacuum after
clearing account tokens, then atomically rename the sanitized temporary file to
dest. Ensure temporary files are removed on any failure so an unsanitized or
partial backup is never published.
- Around line 717-724: In the backup flow around dest.exists(), compare dest
with the current database filename before removing it and reject the operation
when they refer to the same path, preserving the live database. Add a regression
test covering a same-path VACUUM INTO attempt and verify the database file is
not deleted or replaced.
- Around line 717-724: Update the backup overwrite handling around the
dest.exists() check to remove SQLite’s associated sidecar files, including
dest-wal and dest-shm, before VACUUM INTO writes the replacement. Preserve the
existing error mapping and ensure stale WAL/SHM contents cannot remain alongside
the overwritten backup.
- Line 728: Update the backup logic around the VACUUM INTO execution to reject
destinations whose OsStr cannot be represented as UTF-8 before invoking SQLite,
rather than using dest.to_string_lossy(). Preserve the existing path used for
subsequent hardening and reopening, and return an appropriate error for
non-UTF-8 backup paths.
---
Nitpick comments:
In `@src/db.rs`:
- Around line 1476-1478: Extend the VACUUM INTO test assertions to also verify
that the destination database’s `backup.db-shm` sidecar is absent, alongside the
existing `backup.db-wal` check, preserving the documented guarantee that neither
WAL artifact is produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if dest.exists() { | ||
| // VACUUM INTO は既存ファイルへの書き出しを拒否する | ||
| std::fs::remove_file(dest).map_err(|e| { | ||
| NoteDeckError::Database(rusqlite::Error::SqliteFailure( | ||
| rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_CANTOPEN), | ||
| Some(format!("failed to replace existing backup: {e}")), | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Locate the backup implementation and relevant tests.
fd -a 'db.rs' .
echo '---'
ast-grep outline src/db.rs --view expanded || true
echo '---'
rg -n "backup_to|VACUUM INTO|remove_file|dest.exists|same.*path|file identity|metadata" src/db.rs
echo '---'
sed -n '660,780p' src/db.rsRepository: notedeck-dev/notecli
Length of output: 13512
🏁 Script executed:
set -euo pipefail
# Inspect the backup tests and surrounding setup to see whether same-path behavior is covered.
sed -n '1330,1518p' src/db.rsRepository: notedeck-dev/notecli
Length of output: 6083
🏁 Script executed:
set -euo pipefail
# Check whether Database retains the source path and whether any same-file guard exists.
sed -n '95,190p' src/db.rs
echo '---'
rg -n "same.*file|canonical|metadata\\(|dev\\(|ino\\(|path:" src/db.rsRepository: notedeck-dev/notecli
Length of output: 4278
Reject the live database path here. If dest points to the open database file, remove_file(dest) unlinks the live DB before VACUUM INTO, so existing connections keep using the unlinked inode while the path is recreated as the backup. Compare dest against the current database filename before deletion and add a same-path regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db.rs` around lines 717 - 724, In the backup flow around dest.exists(),
compare dest with the current database filename before removing it and reject
the operation when they refer to the same path, preserving the live database.
Add a regression test covering a same-path VACUUM INTO attempt and verify the
database file is not deleted or replaced.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first
ast-grep outline src/db.rs --view expanded
# Read the relevant section around the reported lines
sed -n '680,780p' src/db.rs
# Look for other backup / VACUUM INTO / strip_tokens handling
rg -n "VACUUM INTO|strip_tokens|backup|remove_file\\(|-wal|-shm|journal" src/db.rsRepository: notedeck-dev/notecli
Length of output: 12915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '160,190p' src/db.rs
sed -n '1460,1515p' src/db.rs
# Search for any existing cleanup of SQLite sidecars around backup / temp DB handling
rg -n "suffix in \\[\"\", \"-wal\", \"-shm\"\\]|remove_file\\(.*-wal|remove_file\\(.*-shm|backup_to_overwrites_an_existing_file|backup_to_can_strip_tokens_from_the_copy" src/db.rsRepository: notedeck-dev/notecli
Length of output: 3286
🌐 Web query:
SQLite VACUUM INTO existing destination WAL SHM sidecar files strip_tokens backup overwrite
💡 Result:
The SQLite VACUUM INTO command requires that the destination file does not exist [1][2]. If the destination file already exists, the command will fail with an error [1][2]. It cannot be used to overwrite an existing database file directly. Regarding WAL (Write-Ahead Logging) and SHM (shared-memory) sidecar files: 1. VACUUM INTO creates a new, compacted database file [1][2]. It does not create WAL or SHM files for the destination at the moment of creation [3][4]. 2. When restoring a database, existing WAL (-wal) and SHM (-shm) sidecar files associated with the destination must be manually deleted before the new database file is put in place [5]. These sidecar files are intrinsically linked to a specific database file state and are not compatible with a different or restored version of the database [5][6]. 3. If the destination file exists (and for some reason you are replacing it), you must remove the old database file along with its associated *-wal and *-shm sidecar files to prevent corruption or invalid state [5]. In summary, VACUUM INTO is a safe, non-destructive operation because it enforces the absence of the destination file [1]. If you intend to use it as part of an update or restoration process, you must manually clear the destination directory of the old database and its sidecar files before executing the command [5].
Citations:
- 1: https://sqlite.org/lang_vacuum.html
- 2: https://www2.sqlite.org/lang_vacuum.html
- 3: https://www.sqlite.org/walformat.html
- 4: https://www.sqlite.org/tempfiles.html
- 5: https://agenticdevelopercookbook.com/guidelines/implementing/data/backup-and-recovery
- 6: https://www.productionhardening.org/backup-recovery-data-integrity/
🌐 Web query:
SQLite stale WAL file replaced database file ignored salt checksum same path
💡 Result:
When you replace an SQLite database file while leaving an old Write-Ahead Log (-wal) and shared memory (-shm) file in place, SQLite will attempt to process the stale files during the next connection [1][2][3]. Because the WAL file contains metadata including "salt" values that must match the current database file header, a mismatch will occur when they do not align [4][5][6]. Here is how SQLite handles this situation: 1. Salt Mismatch: The WAL header contains two "salt" values [7][5]. When SQLite opens a database, it checks if the salt values in the -wal file match the database file [4][6]. If you have replaced the database file, the old WAL file will contain salts from the previous database [1][3]. SQLite sees this mismatch and correctly identifies the WAL as "stale" or belonging to a different database instance [3]. 2. Checksum Verification: Beyond salt checks, SQLite verifies the integrity of each "frame" (transaction) in the WAL file by computing a checksum that includes the header and frame data [8][4][6]. If the WAL is stale, or if there is a mismatch during the recovery/checkpointing process, the checksum verification will fail [8][4][6]. When a frame fails checksum validation, SQLite stops processing the WAL at that point [8][9][10]. 3. Outcome: If the WAL and SHM files do not match the new database file, the database can appear "corrupt" or "malformed," potentially leading to error messages such as "database disk image is malformed" [1][3]. SQLite does not automatically "ignore" these files in a way that preserves data; instead, it may attempt to roll back or checkpoint them, which can lead to data loss or corruption of the newly placed database file [3][9][11]. To safely replace a database file in WAL mode, you must delete the accompanying -wal and -shm files alongside the main database file [1][2][3]. The only safe way to remove these files without manual deletion is to ensure all connections are closed properly, allowing SQLite to clean them up, or by opening the database and closing it immediately via the SQLite API [11][10]. Manually replacing only the.db file while leaving stale auxiliary files in the same path is a frequent cause of database corruption [1][3].
Citations:
- 1: https://exchangetuts.com/sqlite-database-disk-image-malformed-on-iphone-sdk-1640416563800255
- 2: https://stackoverflow.com/questions/54051322/database-import-and-export-not-working-in-android-pie
- 3: https://stackoverflow.com/questions/72420883/sqlite-database-gets-overwritten-when-manually-placed-onto-device
- 4: https://github.com/endlesssoftware/sqlite3/blob/master/wal.c
- 5: https://chromium.googlesource.com/chromium/src/+/c368e30ae55600a1c3c9cb1710a54f9c55de786e/third_party/sqlite/src/src/wal.c
- 6: https://github.com/sqlite/sqlite/blob/master/src/wal.c
- 7: https://docs.rs/fsqlite-wal/latest/fsqlite_wal/checksum/struct.WalHeader.html
- 8: https://avi.im/blag/2025/sqlite-wal-checksum/
- 9: https://sqlite.org/forum/info/100daaad30d247ce9b324010721e20f2f43dd60b38ea66214cd3f568f293c8bf
- 10: https://www.sqlite.org/walformat.html
- 11: https://www2.sqlite.org/wal.html
Remove SQLite sidecars when overwriting backups. Deleting only dest leaves any existing dest-wal/dest-shm files behind, so an overwritten backup can still expose old WAL contents and break the strip-tokens guarantee. Remove the full SQLite file set or write to a fresh temp path and rename it into place.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db.rs` around lines 717 - 724, Update the backup overwrite handling
around the dest.exists() check to remove SQLite’s associated sidecar files,
including dest-wal and dest-shm, before VACUUM INTO writes the replacement.
Preserve the existing error mapping and ensure stale WAL/SHM contents cannot
remain alongside the overwritten backup.
| } | ||
| { | ||
| let conn = self.lock_write()?; | ||
| conn.execute("VACUUM INTO ?1", params![dest.to_string_lossy()])?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure and inspect the relevant area in src/db.rs
git ls-files 'src/db.rs' 'src/*.rs' | sed -n '1,20p'
wc -l src/db.rs
sed -n '680,760p' src/db.rsRepository: notedeck-dev/notecli
Length of output: 3457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact function around the VACUUM INTO call and any path validation nearby
sed -n '700,750p' src/db.rs
printf '\n---\n'
rg -n "to_string_lossy|VACUUM INTO|PathBuf|canonicalize|metadata|permissions" src/db.rsRepository: notedeck-dev/notecli
Length of output: 2905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect permission-hardening logic and any path validation around backup callers
sed -n '120,210p' src/db.rs
printf '\n--- callers ---\n'
rg -n "backup_to\(" -S .
printf '\n--- path validation ---\n'
rg -n "to_str\(|to_string_lossy\(|is_utf8|utf-8|UTF-8|non-UTF|Path::new|OsStr|OsString" src/db.rs src/*.rsRepository: notedeck-dev/notecli
Length of output: 4491
Reject non-UTF-8 backup paths. to_string_lossy() can change the filename passed to VACUUM INTO, so the backup may be written under a different path than the one later hardened and reopened. Reject non-UTF-8 paths or use an OS-string-preserving binding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db.rs` at line 728, Update the backup logic around the VACUUM INTO
execution to reject destinations whose OsStr cannot be represented as UTF-8
before invoking SQLite, rather than using dest.to_string_lossy(). Preserve the
existing path used for subsequent hardening and reopening, and return an
appropriate error for non-UTF-8 backup paths.
| let conn = self.lock_write()?; | ||
| conn.execute("VACUUM INTO ?1", params![dest.to_string_lossy()])?; | ||
| } | ||
| Self::restrict_permissions(dest); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Propagate permission-hardening failures for backups.
restrict_permissions ignores set_permissions errors, so this method can return success with a group/world-readable backup containing API tokens. Make permission enforcement fallible for backups and fail or clean up when owner-only permissions cannot be applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db.rs` at line 730, Update the backup flow around restrict_permissions so
permission enforcement returns and propagates set_permissions failures instead
of being ignored. Make restrict_permissions fallible, have its caller fail the
backup when owner-only permissions cannot be applied, and clean up the
destination if required by the existing backup error path.
| if strip_tokens { | ||
| let copy = Connection::open(dest)?; | ||
| copy.execute("UPDATE accounts SET token = ''", [])?; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant area around the reported lines.
ast-grep outline src/db.rs --view expanded > /tmp/db_outline.txt
sed -n '700,780p' src/db.rs | cat -n
# Find the strip_tokens implementation and any related backup/publish logic.
rg -n "strip_tokens|VACUUM|secure_delete|Connection::open\\(dest\\)|UPDATE accounts SET token" src/db.rsRepository: notedeck-dev/notecli
Length of output: 5002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect DB initialization pragmas and any backup-related tests/docs.
sed -n '1,260p' src/db.rs | cat -n
printf '\n--- backup tests ---\n'
sed -n '1460,1525p' src/db.rs | cat -n
printf '\n--- search for secure_delete / temp backup handling ---\n'
rg -n "secure_delete|temp file|tempfile|rename\\(|VACUUM INTO|backup_to\\(" srcRepository: notedeck-dev/notecli
Length of output: 14773
Make strip_tokens sanitize the backup before publishing it.
UPDATE accounts SET token = '' only clears the logical value; the original bytes can still remain in SQLite pages unless secure_delete/VACUUM is used. Because this writes straight to dest, a failed strip also leaves an unsanitized backup behind. Stage it in a temp path, scrub there, then rename into place and clean up on error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db.rs` around lines 732 - 735, Update the strip_tokens branch in the
backup flow to write to a temporary destination, enable SQLite secure deletion
and vacuum after clearing account tokens, then atomically rename the sanitized
temporary file to dest. Ensure temporary files are removed on any failure so an
unsanitized or partial backup is never published.
1 participant
なぜ
DB は WAL モードで動いているため、
notecli.dbを単純にファイルコピーしても WAL に未反映のトランザクションが取り残され、古い、または壊れた複製ができる。NoteDeck 側の「DB をエクスポート」は現状まさにその単純コピーで、この不整合を踏みうる。また定期バックアップ (notedeck-dev/notedeck#816) の土台としても、整合性のあるスナップショットを作る手段が要る。
Databaseは生のConnectionを公開していないため、呼び出し側からはVACUUM INTOも backup API も叩けない。ここに口を開ける。変更
Database::backup_to(dest, strip_tokens)を追加VACUUM INTOを実行し、単一ファイルで完結したコピーを作る (-wal/-shmを伴わない)strip_tokensを立てると複製側のaccounts.tokenを空にする。キーチェーンが永続しない環境では DB に API トークンが平文で残るため、持ち出す成果物からは落とせるようにするVACUUM INTOは既存ファイルへの書き出しを拒否するので、宛先があれば先に消すテスト
3 件追加 (コピーが読めてデータが入ること / トークン除去が複製にだけ効き元 DB を触らないこと / 既存ファイルを上書きできること)。既存 193 件と合わせて通過。
Summary by CodeRabbit
New Features
Bug Fixes