From 05b84bf7963ed695875f0b655c07aa1e409c37d3 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Fri, 16 Oct 2026 14:48:32 +0200 Subject: [PATCH] fix(fuse,core): keep a mirrored folder safe when another computer syncs it A mirrored folder assumed it was the only client writing to its folder on Drive. With a second computer syncing the same folder, edits could be lost: - The first sync took a same-size file at the same path for the same file. It now records the baseline only if the local file's SHA-1 matches Drive's digest, and otherwise keeps a conflict copy (B248). - An upload replaced a revision another client had committed since the pass looked. The upload now checks Drive's active revision first, and an edit that read back with someone else's digest is left for the next pass to keep as a conflict copy (B249). - A download recorded a revision committed while it ran as synced, so that edit never arrived. The baseline now records the revision the pass planned to download (B250). - Conflict copies now carry the computer's hostname, so two computers do not make the same name on Drive (B251). The baseline gains the revision id it was synced at (schema 44), and a new revision counts as a remote change even at the same mtime and size. --- crates/pdfs-core/src/db/migrations.rs | 33 +- crates/pdfs-core/src/db/sync.rs | 35 +- crates/pdfs-core/src/db/tests.rs | 70 ++++ crates/pdfs-fuse/src/lib.rs | 104 +++++- crates/pdfs-fuse/src/sim/daemon.rs | 210 ++++++++++++ crates/pdfs-fuse/src/sweep.rs | 42 ++- crates/pdfs-fuse/src/sync.rs | 439 ++++++++++++++++++++------ crates/pdfs-fuse/src/sync/model.rs | 80 ++++- crates/pdfs-fuse/src/sync/planner.rs | 151 +++++++-- crates/pdfs-fuse/src/takeout.rs | 2 +- 10 files changed, 998 insertions(+), 168 deletions(-) diff --git a/crates/pdfs-core/src/db/migrations.rs b/crates/pdfs-core/src/db/migrations.rs index 1689224..08b946b 100644 --- a/crates/pdfs-core/src/db/migrations.rs +++ b/crates/pdfs-core/src/db/migrations.rs @@ -11,7 +11,7 @@ use super::Db; use crate::Result; /// Current schema version. Bump on every forward migration added below. -pub(super) const SCHEMA_VERSION: i64 = 43; +pub(super) const SCHEMA_VERSION: i64 = 44; impl Db { pub(super) fn migrate(&self) -> Result<()> { @@ -450,6 +450,23 @@ impl Db { tx.execute_batch(MIGRATION_V43)?; } } + if current < 44 { + // Same guards as V26-V42. + let has_column: bool = tx.query_row( + "SELECT COUNT(*) FROM pragma_table_info('sync_entry') \ + WHERE name = 'remote_revision_id'", + [], + |row| row.get::<_, i64>(0), + )? > 0; + let has_entries: bool = tx.query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'sync_entry'", + [], + |row| row.get::<_, i64>(0), + )? > 0; + if has_entries && !has_column { + tx.execute_batch(MIGRATION_V44)?; + } + } tx.execute( "INSERT INTO sync_state (key, value) VALUES ('schema_version', ?1) ON CONFLICT(key) DO UPDATE SET value = excluded.value", @@ -1351,3 +1368,17 @@ ALTER TABLE pending_op ADD COLUMN sent_to TEXT; const MIGRATION_V43: &str = " CREATE INDEX IF NOT EXISTS pending_op_kind_name ON pending_op(kind, name); "; + +/// Schema v44: a synced file's baseline remembers the revision it was synced +/// at. +/// +/// A mirrored file edited here is uploaded as a new revision of whatever is +/// current on Drive, and another computer syncing the same folder may have +/// committed one since this pass looked. The revision id is what the upload +/// checks first, so that edit becomes a conflict copy instead of being +/// replaced (B249). `NULL` means "this row predates the column", and the check +/// falls back to the modification time and size; each row gains a real value +/// the next time its path is synced. +const MIGRATION_V44: &str = " +ALTER TABLE sync_entry ADD COLUMN remote_revision_id TEXT; +"; diff --git a/crates/pdfs-core/src/db/sync.rs b/crates/pdfs-core/src/db/sync.rs index af93b07..960c00a 100644 --- a/crates/pdfs-core/src/db/sync.rs +++ b/crates/pdfs-core/src/db/sync.rs @@ -45,6 +45,12 @@ pub struct StoredSyncEntry { pub local_mtime_ns: Option, pub remote_rev: Option, pub remote_hash: Option, + /// The id of the remote revision this row was synced at. A mirror upload + /// checks it before it supersedes what Drive holds, because another client + /// may have committed a revision since (`docs/BUGS.md` B249). `None` on a + /// row written before schema 44, or by a pass that did not learn it; the + /// check then falls back to `remote_rev` and `remote_hash`. + pub remote_revision_id: Option, } impl Db { @@ -188,7 +194,7 @@ impl Db { let conn = self.read(); let mut stmt = conn.prepare( "SELECT rel_path, remote_uid, local_mtime, local_size, remote_rev, remote_hash, - local_mtime_ns + local_mtime_ns, remote_revision_id FROM sync_entry WHERE folder_id = ?1", )?; let rows = stmt.query_map(params![folder_id], |r| { @@ -200,6 +206,7 @@ impl Db { remote_rev: r.get(4)?, remote_hash: r.get(5)?, local_mtime_ns: r.get(6)?, + remote_revision_id: r.get(7)?, }) })?; let mut map = HashMap::new(); @@ -238,15 +245,16 @@ impl Db { conn.execute( "INSERT INTO sync_entry (folder_id, rel_path, remote_uid, local_mtime, local_size, remote_rev, - remote_hash, local_mtime_ns) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8) + remote_hash, local_mtime_ns, remote_revision_id) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9) ON CONFLICT(folder_id, rel_path) DO UPDATE SET - remote_uid = excluded.remote_uid, - local_mtime = excluded.local_mtime, - local_size = excluded.local_size, - remote_rev = excluded.remote_rev, - remote_hash = excluded.remote_hash, - local_mtime_ns = excluded.local_mtime_ns", + remote_uid = excluded.remote_uid, + local_mtime = excluded.local_mtime, + local_size = excluded.local_size, + remote_rev = excluded.remote_rev, + remote_hash = excluded.remote_hash, + local_mtime_ns = excluded.local_mtime_ns, + remote_revision_id = excluded.remote_revision_id", params![ folder_id, e.rel_path, @@ -256,6 +264,7 @@ impl Db { e.remote_rev, e.remote_hash, e.local_mtime_ns, + e.remote_revision_id, ], )?; Ok(()) @@ -306,7 +315,7 @@ impl Db { { let mut select = tx.prepare( "SELECT rel_path, remote_uid, local_mtime, local_size, remote_rev, remote_hash, - local_mtime_ns + local_mtime_ns, remote_revision_id FROM sync_entry WHERE folder_id = ?1 AND (rel_path = ?2 OR substr(rel_path, 1, length(?2) + 1) = ?2 || '/')", @@ -321,6 +330,7 @@ impl Db { remote_rev: r.get(4)?, remote_hash: r.get(5)?, local_mtime_ns: r.get(6)?, + remote_revision_id: r.get(7)?, }) })? .collect::>>()?; @@ -332,8 +342,8 @@ impl Db { let mut insert = tx.prepare( "INSERT OR REPLACE INTO sync_entry (folder_id, rel_path, remote_uid, local_mtime, local_size, remote_rev, - remote_hash, local_mtime_ns) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)", + remote_hash, local_mtime_ns, remote_revision_id) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)", )?; for row in rows { let rel = format!("{to_rel}{}", &row.rel_path[from_rel.len()..]); @@ -346,6 +356,7 @@ impl Db { row.remote_rev, row.remote_hash, row.local_mtime_ns, + row.remote_revision_id, ])?; } } diff --git a/crates/pdfs-core/src/db/tests.rs b/crates/pdfs-core/src/db/tests.rs index 63d4c1c..549eb8c 100644 --- a/crates/pdfs-core/src/db/tests.rs +++ b/crates/pdfs-core/src/db/tests.rs @@ -1327,6 +1327,7 @@ fn a_rebuild_keeps_the_queue_baselines_and_pins_of_the_damaged_file() { local_size: 6, remote_rev: Some("7".into()), remote_hash: Some("8".into()), + remote_revision_id: None, }, ) .unwrap(); @@ -5343,6 +5344,7 @@ fn baseline_row(rel: &str) -> StoredSyncEntry { local_size: 7, remote_rev: Some("1700000000".into()), remote_hash: Some("7".into()), + remote_revision_id: None, } } @@ -5437,6 +5439,7 @@ fn mirror_uid_lookup_covers_the_root_and_synced_descendants_only() { local_size: 2, remote_rev: Some("3".into()), remote_hash: Some("2".into()), + remote_revision_id: None, local_mtime_ns: None, }, ) @@ -5454,6 +5457,7 @@ fn mirror_uid_lookup_covers_the_root_and_synced_descendants_only() { local_size: 2, remote_rev: Some("3".into()), remote_hash: Some("2".into()), + remote_revision_id: None, local_mtime_ns: None, }, ) @@ -6631,6 +6635,71 @@ fn migration_v27_adds_the_access_deferral_column_to_a_v26_queue() { let _ = std::fs::remove_file(format!("{}.lock", path.display())); } +/// V44 adds the remote revision a baseline row was synced at. A row carried +/// over from V43 has none, and the mirror then compares the modification time +/// and size as before, so upgrading changes nothing about what is in step +/// (`docs/BUGS.md` B249). +#[test] +fn migration_v44_adds_the_synced_revision_without_disturbing_a_v43_baseline() { + let path = std::env::temp_dir().join(format!( + "pdfs-db-v43-fixture-{}-{}.db", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let row = StoredSyncEntry { + rel_path: "notes.txt".into(), + remote_uid: Some("vol~notes".into()), + local_mtime: 1_700_000_000, + local_mtime_ns: Some(1_700_000_000_123_456_789), + local_size: 42, + remote_rev: Some("1700000000".into()), + remote_hash: Some("42".into()), + remote_revision_id: Some("rev-1".into()), + }; + { + let db = Db::open(&path).unwrap(); + let folder = db + .sync_folder_add("/home/me/Mirror", "vol~root", "share") + .unwrap(); + db.sync_entry_upsert(folder, &row).unwrap(); + } + { + // Put the file back in the state a released V43 database was in. + let conn = rusqlite::Connection::open(&path).unwrap(); + conn.execute_batch( + "ALTER TABLE sync_entry DROP COLUMN remote_revision_id; + UPDATE sync_state SET value = '43' WHERE key = 'schema_version';", + ) + .unwrap(); + } + + let db = Db::open(&path).unwrap(); + let entries = db.sync_entries(1).unwrap(); + let entry = entries.get("notes.txt").expect("the baseline row survives"); + assert_eq!( + entry, + &StoredSyncEntry { + remote_revision_id: None, + ..row.clone() + } + ); + + // And the column is live, including through a move of the row. + db.sync_entry_upsert(1, &row).unwrap(); + db.sync_entries_move(1, "notes.txt", 1, "moved.txt") + .unwrap(); + let entries = db.sync_entries(1).unwrap(); + assert_eq!( + entries["moved.txt"].remote_revision_id.as_deref(), + Some("rev-1") + ); + drop(db); + let _ = std::fs::remove_file(&path); +} + /// A mirror baseline carried over from V27 keeps its rows, and each one reads /// back with no nanosecond time. That `None` is the whole point: it makes the /// comparison fall back to whole seconds for rows written before the column @@ -6661,6 +6730,7 @@ fn migration_v28_adds_sub_second_local_times_without_disturbing_a_v27_baseline() local_size: 42, remote_rev: Some("1700000000".into()), remote_hash: Some("42".into()), + remote_revision_id: None, }, ) .unwrap(); diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 229bc66..8d02340 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -6165,11 +6165,61 @@ fn is_gone(e: &(dyn std::error::Error + 'static)) -> bool { /// (`sync.rs`, `conflict_path`), so the two halves of the product name the same /// situation the same way and a user only has to learn it once. fn conflict_name(name: &str, stamp: i64) -> String { + conflict_name_as(name, conflict_label(), stamp) +} + +/// [`conflict_name`] with the machine's label given. +fn conflict_name_as(name: &str, label: &str, stamp: i64) -> String { let path = Path::new(name); let stem = path.file_stem().and_then(|s| s.to_str()).unwrap_or(name); + let tag = conflict_tag(label, stamp, 0); match path.extension().and_then(|s| s.to_str()) { - Some(ext) => format!("{stem} (sync-conflict {stamp}).{ext}"), - None => format!("{stem} (sync-conflict {stamp})"), + Some(ext) => format!("{stem} (sync-conflict {tag}).{ext}"), + None => format!("{stem} (sync-conflict {tag})"), + } +} + +/// This machine's name as its conflict copies carry it (`docs/BUGS.md` B251). +/// +/// Two computers syncing one folder that each keep a copy of the same file in +/// the same second would otherwise make the same name on Drive, and nobody +/// could tell which computer a copy came from. Read once, from the hostname. +fn conflict_label() -> &'static str { + static LABEL: OnceLock = OnceLock::new(); + LABEL.get_or_init(|| conflict_label_of(&this_hostname())) +} + +/// `host` as one token: anything but ASCII letters, digits, `-`, `_` and `.` +/// becomes `-`, so the label holds no space for `conflict_base_name` to trip +/// on and no `/` or `)` to break the name. +fn conflict_label_of(host: &str) -> String { + let label: String = host + .chars() + .take(32) + .map(|c| { + if c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.') { + c + } else { + '-' + } + }) + .collect(); + label.trim_matches('-').to_string() +} + +/// The inside of a conflict copy's marker: `