diff --git a/aw-datastore/src/datastore.rs b/aw-datastore/src/datastore.rs index bbe12a64..aa414036 100644 --- a/aw-datastore/src/datastore.rs +++ b/aw-datastore/src/datastore.rs @@ -32,7 +32,7 @@ fn _get_db_version(conn: &Connection) -> i32 { * 5: Replaced single-column events indexes with a composite index * 6: Added an endtime-first index for recent interval reads */ -static NEWEST_DB_VERSION: i32 = 6; +pub const NEWEST_DB_VERSION: i32 = 6; fn _create_tables(conn: &Connection, version: i32) -> bool { let mut first_init = false; diff --git a/aw-datastore/src/lib.rs b/aw-datastore/src/lib.rs index a93bf896..1dd60961 100644 --- a/aw-datastore/src/lib.rs +++ b/aw-datastore/src/lib.rs @@ -24,12 +24,16 @@ mod privacy_filter; mod worker; pub use self::datastore::DatastoreInstance; +pub use self::datastore::NEWEST_DB_VERSION; pub use self::worker::Datastore; #[derive(Clone)] pub enum DatastoreMethod { Memory(), File(String), + /// Existing file, opened `mode=ro&immutable=1`. Never migrates, never + /// creates `-wal`/`-shm`. Used by aw-sync when reading a peer's db. + FileReadOnly(String), /// Encrypted SQLite file using SQLCipher. Only available with the /// `encryption` or `encryption-vendored` feature flags. #[cfg(any(feature = "encryption", feature = "encryption-vendored"))] @@ -41,6 +45,7 @@ impl fmt::Debug for DatastoreMethod { match self { DatastoreMethod::Memory() => write!(f, "Memory()"), DatastoreMethod::File(p) => write!(f, "File({p:?})"), + DatastoreMethod::FileReadOnly(p) => write!(f, "FileReadOnly({p:?})"), #[cfg(any(feature = "encryption", feature = "encryption-vendored"))] DatastoreMethod::FileEncrypted(p, _) => write!(f, "FileEncrypted({p:?}, )"), } diff --git a/aw-datastore/src/worker.rs b/aw-datastore/src/worker.rs index 0a3a4b1a..d0c0959f 100644 --- a/aw-datastore/src/worker.rs +++ b/aw-datastore/src/worker.rs @@ -12,6 +12,7 @@ use chrono::Utc; use rusqlite::Connection; use rusqlite::DropBehavior; +use rusqlite::OpenFlags; use rusqlite::Transaction; use rusqlite::TransactionBehavior; @@ -26,6 +27,66 @@ use crate::DatastoreMethod; type RequestSender = mpsc_requests::RequestSender>; type RequestReceiver = mpsc_requests::RequestReceiver>; +/// SQLite URI for a side-effect-free open: no `-wal`/`-shm`, no locks that +/// fight a file syncer. `?`/`#`/`%` in the path are encoded so they cannot +/// be parsed as the query string. +/// +/// `immutable=1` is load-bearing. It tells SQLite the file cannot change, so +/// the open never creates `-wal`/`-shm` and never takes a lock — which is +/// why a pull can read a peer file in a directory this device does not own. +/// Do not drop it to "see the WAL": that reintroduces sidecars in peers' +/// folders, and a plain `mode=ro` connection then rejects `BEGIN IMMEDIATE`. +/// See [`Datastore::open_read_only`]. +/// +/// The connection can live across a multi-page pull. Syncthing/Dropbox write +/// a temp file and rename, so an open handle keeps the old inode on POSIX +/// (a consistent snapshot) and blocks the rename on Windows (the syncer +/// retries). Only in-place rewriting (`rsync --inplace`, a naive `cp` over +/// the file) defeats it. Copy-then-open is the belt-and-braces option if +/// that ever bites; not needed now. +/// +/// Windows path normalisation (backslash → slash, drive-letter, UNC) is +/// gated on `cfg!(windows)`, not path shape. A backslash is a legal POSIX +/// filename character; rewriting it on Linux/macOS would make the probe +/// target a different file. +fn sqlite_readonly_uri(path: &str) -> String { + let encoded = path + .replace('%', "%25") + .replace('?', "%3F") + .replace('#', "%23"); + if cfg!(windows) { + let encoded = encoded.replace('\\', "/"); + let has_drive_letter = encoded.len() >= 2 && encoded.as_bytes().get(1) == Some(&b':'); + let is_unc = encoded.starts_with("//"); + if has_drive_letter { + return format!("file:///{encoded}?mode=ro&immutable=1"); + } + if is_unc { + // SQLite UNC form is file:////server/share/file.db (four slashes). + return format!("file://{encoded}?mode=ro&immutable=1"); + } + } + format!("file:{encoded}?mode=ro&immutable=1") +} + +fn open_readonly_connection(path: &str) -> rusqlite::Result { + Connection::open_with_flags( + sqlite_readonly_uri(path), + OpenFlags::SQLITE_OPEN_READ_ONLY | OpenFlags::SQLITE_OPEN_URI, + ) +} + +/// Read `user_version` without mutating the file. +fn probe_user_version(path: &str) -> Result { + let conn = open_readonly_connection(path).map_err(|e| { + DatastoreError::InternalError(format!("read-only open failed for {path}: {e}")) + })?; + conn.pragma_query_value(None, "user_version", |row| row.get(0)) + .map_err(|e| { + DatastoreError::InternalError(format!("user_version read failed for {path}: {e}")) + }) +} + #[derive(Clone)] pub struct Datastore { requester: RequestSender, @@ -148,6 +209,8 @@ impl DatastoreWorker { } fn work_loop(&mut self, method: DatastoreMethod) { + let read_only = matches!(&method, DatastoreMethod::FileReadOnly(_)); + // Open SQLite connection let mut conn = match &method { DatastoreMethod::Memory() => { @@ -156,6 +219,9 @@ impl DatastoreWorker { DatastoreMethod::File(path) => { Connection::open(path).expect("Failed to create datastore") } + DatastoreMethod::FileReadOnly(path) => { + open_readonly_connection(path).expect("Failed to open datastore read-only") + } #[cfg(any(feature = "encryption", feature = "encryption-vendored"))] DatastoreMethod::FileEncrypted(path, key) => { let conn = Connection::open(path).expect("Failed to create encrypted datastore"); @@ -176,24 +242,22 @@ impl DatastoreWorker { conn.busy_timeout(std::time::Duration::from_secs(5)) .expect("Failed to set busy timeout"); - // WAL turns each commit into a single sequential WAL append+fsync where - // delete mode paid two fsyncs plus journal-file churn, and lets future - // reader connections proceed while a commit is in flight. - // synchronous=FULL is set explicitly (rather than relying on the - // default) so a commit remains durable on disk the moment it returns; - // with NORMAL the WAL is only synced at checkpoints, which would - // silently widen the loss window on power failure. - // In-memory databases ignore the request (journal_mode stays "memory"). - let journal_mode: String = conn - .pragma_update_and_check(None, "journal_mode", "WAL", |row| row.get(0)) - .expect("Failed to query journal_mode"); - if !matches!(&method, DatastoreMethod::Memory()) && journal_mode != "wal" { - warn!("Failed to enable WAL (journal_mode={journal_mode}), continuing without it"); + // WAL / synchronous=FULL are writes. Skip them on a peer file: a pull + // must not create `-wal`/`-shm` in a directory this device does not + // own (ActivityWatch/aw-server-rust#693). In-memory databases ignore + // the request (journal_mode stays "memory"). + if !read_only { + let journal_mode: String = conn + .pragma_update_and_check(None, "journal_mode", "WAL", |row| row.get(0)) + .expect("Failed to query journal_mode"); + if !matches!(&method, DatastoreMethod::Memory()) && journal_mode != "wal" { + warn!("Failed to enable WAL (journal_mode={journal_mode}), continuing without it"); + } + conn.pragma_update(None, "synchronous", "FULL") + .expect("Failed to set synchronous=FULL"); } - conn.pragma_update(None, "synchronous", "FULL") - .expect("Failed to set synchronous=FULL"); - let mut ds = DatastoreInstance::new(&conn, true).unwrap(); + let mut ds = DatastoreInstance::new(&conn, !read_only).unwrap(); // Load persisted privacy filters before serving inserts. The engine // starts empty; without this, rules saved in a previous process sit @@ -222,19 +286,28 @@ impl DatastoreWorker { } } + // BEGIN IMMEDIATE takes a reserved (write) lock. On a read-only + // connection that is either SQLITE_READONLY (the worker retried + // forever) or a no-op depending on SQLite version — Deferred is the + // correct read-only behavior either way. + let tx_behavior = if read_only { + TransactionBehavior::Deferred + } else { + TransactionBehavior::Immediate + }; + // Start handling and respond to requests loop { let last_commit_time: DateTime = Utc::now(); - let mut tx: Transaction = - match conn.transaction_with_behavior(TransactionBehavior::Immediate) { - Ok(tx) => tx, - Err(err) => { - error!("Unable to start transaction! {:?}", err); - // Wait 1s before retrying - std::thread::sleep(std::time::Duration::from_millis(1000)); - continue; - } - }; + let mut tx: Transaction = match conn.transaction_with_behavior(tx_behavior) { + Ok(tx) => tx, + Err(err) => { + error!("Unable to start transaction! {:?}", err); + // Wait 1s before retrying + std::thread::sleep(std::time::Duration::from_millis(1000)); + continue; + } + }; // Snapshot BEFORE the request loop. SetKeyValue/DeleteKeyValue // reload the engine from the still-open transaction so a later // insert in the same batch is filtered. If commit fails we restore @@ -508,6 +581,34 @@ impl Datastore { Datastore::_new_internal(method, legacy_import) } + /// Open an existing database without writing to it. + /// + /// Uses `file:…?mode=ro&immutable=1`, never runs migrations, never sets + /// `journal_mode`/`synchronous`. Returns `OldDbVersion` when + /// `user_version != NEWEST_DB_VERSION` so a caller can skip that peer + /// (ActivityWatch/aw-server-rust#693). + /// + /// `immutable=1` means SQLite will not look at a peer's `-wal`/`-shm`. + /// Committed-but-not-yet-checkpointed frames in that WAL are therefore + /// invisible. aw-sync checkpoints on clean close, so the steady-state + /// file is self-contained; this only bites mid-push. A stale-but- + /// consistent snapshot is strictly better than a torn one, and dropping + /// `immutable` would reintroduce `-shm` files in a foreign directory. + pub fn open_read_only(dbpath: String) -> Result { + let version = probe_user_version(&dbpath)?; + if version != crate::NEWEST_DB_VERSION { + return Err(DatastoreError::OldDbVersion(format!( + "Tried to open a database with an incompatible database version! \ + Database has version {version} while the supported version is {}", + crate::NEWEST_DB_VERSION + ))); + } + Ok(Datastore::_new_internal( + DatastoreMethod::FileReadOnly(dbpath), + false, + )) + } + pub fn new_in_memory(legacy_import: bool) -> Self { let method = DatastoreMethod::Memory(); Datastore::_new_internal(method, legacy_import) @@ -766,3 +867,46 @@ impl Datastore { } } } + +#[cfg(test)] +mod sqlite_readonly_uri_tests { + use super::sqlite_readonly_uri; + + #[test] + fn posix_absolute() { + assert_eq!( + sqlite_readonly_uri("/var/lib/activitywatch/peer.db"), + "file:/var/lib/activitywatch/peer.db?mode=ro&immutable=1" + ); + } + + #[test] + #[cfg(not(windows))] + fn posix_path_with_literal_backslash_is_not_rewritten() { + // A backslash is a valid POSIX filename character. Windows + // normalisation is cfg!(windows)-gated so this path is passed + // through untouched on Linux/macOS. + assert_eq!( + sqlite_readonly_uri(r"/home/erik/we\ird.db"), + r"file:/home/erik/we\ird.db?mode=ro&immutable=1" + ); + } + + #[test] + #[cfg(windows)] + fn windows_drive_letter() { + assert_eq!( + sqlite_readonly_uri(r"C:\Users\bob\peer.db"), + "file:///C:/Users/bob/peer.db?mode=ro&immutable=1" + ); + } + + #[test] + #[cfg(windows)] + fn windows_unc() { + assert_eq!( + sqlite_readonly_uri(r"\\server\share\peer.db"), + "file:////server/share/peer.db?mode=ro&immutable=1" + ); + } +} diff --git a/aw-datastore/tests/datastore.rs b/aw-datastore/tests/datastore.rs index f4d836cb..85e6fc4b 100644 --- a/aw-datastore/tests/datastore.rs +++ b/aw-datastore/tests/datastore.rs @@ -14,7 +14,7 @@ mod datastore_tests { use chrono::Utc; use serde_json::json; - use aw_datastore::Datastore; + use aw_datastore::{Datastore, DatastoreError}; use aw_models::Bucket; use aw_models::BucketMetadata; @@ -1124,4 +1124,80 @@ mod datastore_tests { let _ = std::fs::remove_file(db_path.with_extension("db-wal")); let _ = std::fs::remove_file(db_path.with_extension("db-shm")); } + + /// ActivityWatch/aw-server-rust#693: a pull must not create `-wal`/`-shm` + /// beside a file this device does not own. + #[test] + fn test_read_only_open_does_not_create_wal_sidecars() { + let test_dir = tempfile::tempdir().unwrap(); + let db_path = test_dir.path().join("peer-readonly.db"); + let db_path_str = db_path.to_str().unwrap().to_string(); + + { + let ds = Datastore::new(db_path_str.clone(), false); + create_test_bucket(&ds); + ds.insert_events("testid", &[test_event(Utc::now(), Duration::seconds(1))]) + .unwrap(); + ds.force_commit().unwrap(); + ds.close(); + } + + // Checkpoint so we can delete WAL sidecars without losing the row. + { + let conn = rusqlite::Connection::open(&db_path).unwrap(); + let _ = conn.execute_batch("PRAGMA wal_checkpoint(TRUNCATE);"); + } + + let wal = db_path.with_extension("db-wal"); + let shm = db_path.with_extension("db-shm"); + let _ = std::fs::remove_file(&wal); + let _ = std::fs::remove_file(&shm); + + { + let ds = Datastore::open_read_only(db_path_str).unwrap(); + let buckets = ds.get_buckets().unwrap(); + assert!( + buckets.contains_key("testid"), + "read-only open must still see existing buckets, got {buckets:?}" + ); + let events = ds.get_events("testid", None, None, None).unwrap(); + assert_eq!(events.len(), 1); + ds.close(); + } + + assert!( + !wal.exists(), + "read-only open must not create {}", + wal.display() + ); + assert!( + !shm.exists(), + "read-only open must not create {}", + shm.display() + ); + } + + #[test] + fn test_read_only_open_skips_old_user_version() { + let test_dir = tempfile::tempdir().unwrap(); + let db_path = test_dir.path().join("peer-v4.db"); + { + let conn = rusqlite::Connection::open(&db_path).unwrap(); + conn.pragma_update(None, "user_version", 4).unwrap(); + } + + let wal = db_path.with_extension("db-wal"); + let shm = db_path.with_extension("db-shm"); + let _ = std::fs::remove_file(&wal); + let _ = std::fs::remove_file(&shm); + + match Datastore::open_read_only(db_path.to_str().unwrap().to_string()) { + Err(DatastoreError::OldDbVersion(msg)) => { + assert!(msg.contains("version 4"), "got {msg}"); + } + other => panic!("expected OldDbVersion, got {other:?}"), + } + assert!(!wal.exists(), "version probe must not create a WAL sidecar"); + assert!(!shm.exists(), "version probe must not create a SHM sidecar"); + } } diff --git a/aw-sync/src/sync.rs b/aw-sync/src/sync.rs index 31b7e9d0..4e2af511 100644 --- a/aw-sync/src/sync.rs +++ b/aw-sync/src/sync.rs @@ -101,11 +101,13 @@ pub fn sync_run( } } - // TODO: Check for compatible remote db version before opening + // Peer files are opened read-only: never migrate, never flip WAL + // (ActivityWatch/aw-server-rust#693). Version mismatch is skipped, not fatal. let mut ds_remotes = Vec::new(); for path in &remote_dbfiles { - match create_datastore(path) { - Ok(ds) => ds_remotes.push(ds), + match open_peer_datastore(path) { + Ok(Some(ds)) => ds_remotes.push(ds), + Ok(None) => {} Err(e) => { warn!("Failed to open remote db {}: {e}", path.display()); return Err(e.into()); @@ -166,12 +168,12 @@ pub fn list_buckets(client: &AwClient) -> Result<(), Box> { let remote_dbfiles = crate::util::find_remotes_nonlocal(sync_directory, device_id, None)?; info!("Found remotes: {:?}", remote_dbfiles); - // TODO: Check for compatible remote db version before opening - let ds_remotes: Vec = remote_dbfiles - .iter() - .map(|p| p.as_path()) - .map(create_datastore) - .collect::, _>>()?; + let mut ds_remotes = Vec::new(); + for path in &remote_dbfiles { + if let Some(ds) = open_peer_datastore(path)? { + ds_remotes.push(ds); + } + } log_buckets(client)?; log_buckets(&ds_localremote)?; @@ -208,12 +210,34 @@ fn setup_local_remote(path: &Path, device_id: &str) -> Result Result { - let pathstr = path - .to_str() - .ok_or_else(|| format!("Sync database path is not valid UTF-8: {}", path.display()))?; + let pathstr = utf8_db_path(path)?; Ok(Datastore::new(pathstr.to_string(), false)) } +/// Open a *peer* database for pull: read-only, no migration, no WAL sidecars. +/// +/// Returns `Ok(None)` when `user_version` does not match this binary so the +/// caller can skip that peer and keep walking (ActivityWatch/aw-server-rust#693). +fn open_peer_datastore(path: &Path) -> Result, String> { + let pathstr = utf8_db_path(path)?; + match Datastore::open_read_only(pathstr.to_string()) { + Ok(ds) => Ok(Some(ds)), + Err(DatastoreError::OldDbVersion(msg)) => { + warn!("Skipping peer db {}: {msg}", path.display()); + Ok(None) + } + Err(e) => Err(format!( + "Failed to open remote db {}: {e:?}", + path.display() + )), + } +} + +fn utf8_db_path(path: &Path) -> Result<&str, String> { + path.to_str() + .ok_or_else(|| format!("Sync database path is not valid UTF-8: {}", path.display())) +} + /// Returns the sync-destination bucket for a given bucket, creates it if it doesn't exist. /// /// Returns an error rather than panicking on a datastore failure or on bucket