diff --git a/src/core/account_runtime.rs b/src/core/account_runtime.rs index bcb949c..427248b 100644 --- a/src/core/account_runtime.rs +++ b/src/core/account_runtime.rs @@ -384,6 +384,9 @@ impl FolderRuntime { })) .with_health_probe(Arc::new(|account, _password| { crate::nextcloud::sync_engine::ProductionHealthProbe::run(account) + })) + .with_etag_probe(Arc::new(|account, folder, password| { + crate::nextcloud::sync_engine::ProductionEtagProbe::run(account, folder, password) })); (Box::new(engine), rx) } diff --git a/src/nextcloud/api.rs b/src/nextcloud/api.rs index 6cf26ed..af786e5 100644 --- a/src/nextcloud/api.rs +++ b/src/nextcloud/api.rs @@ -72,6 +72,12 @@ use roxmltree::{Document, Node}; /// bodies are ever downloaded). const PROPFIND_BODY: &[u8] = b""; +/// PROPFIND body requesting only ``, used for the cheap root ETag +/// poll that gates the periodic remote reconciliation (issue #189). Unlike +/// [`PROPFIND_BODY`] it asks for a single property so the response is small +/// and the comparison is just the folder's own ETag. +const PROPFIND_ETAG_BODY: &[u8] = b""; + /// PROPFIND body for the trashbin listing (issue #38): the Nextcloud /// trash properties plus the resource type. const TRASH_PROPFIND_BODY: &[u8] = b""; @@ -458,6 +464,42 @@ impl NextcloudApi { Ok(children > 0) } + /// Return the WebDAV ETag of a folder root, used to gate a periodic + /// reconciliation (issue #189). + /// + /// A single `PROPFIND` with `Depth: 0` asking for `` returns the + /// folder's own ETag cheaply (mirrors the official client's `RequestEtagJob`). + /// The app compares this against the last value it recorded: when it is + /// unchanged, the folder's remote tree has not changed, so the full + /// `nextcloudcmd` reconciliation can be skipped. Returned `trimmed`; an + /// absent/empty `` yields `Ok(None)`. Transport/auth errors keep + /// their [`ApiError`] so the caller can decide (e.g. treat an unreachable + /// server as "changed" to avoid skipping a real change). + pub fn root_etag( + &self, + server: &str, + username: &str, + password: &str, + remote_path: &str, + ) -> Result, ApiError> { + let base = dav_base(server, username); + let folder = format!( + "{base}{}/", + percent_encode_path(remote_path.trim_end_matches('/')) + ); + let authorization = basic_authorization(username, password); + let headers = [ + ("Depth", "0"), + ("Content-Type", "application/xml; charset=utf-8"), + ("Authorization", authorization.as_str()), + ]; + let response = + self.http + .request("PROPFIND", &folder, &headers, Some(PROPFIND_ETAG_BODY))?; + map_status(response.status)?; + Ok(parse_root_etag(&response.body)) + } + /// Estimate the total size in bytes of a remote folder (issue #36). /// /// A single `PROPFIND` with `Depth: infinity` asks the server to walk @@ -1100,6 +1142,20 @@ fn href_path_of(value: &str) -> &str { path.trim_end_matches('/') } +/// Parse the `` of a root PROPFIND (issue #189). +/// +/// Takes the first `` in the response and returns its trimmed text. +/// An absent or empty value yields `None` (the folder reports no ETag). +fn parse_root_etag(body: &[u8]) -> Option { + let text = std::str::from_utf8(body).ok()?; + let doc = Document::parse(text).ok()?; + doc.descendants() + .find(|node| node.has_tag_name((DAV_NS, "getetag"))) + .and_then(|node| node.text()) + .map(|text| text.trim().to_string()) + .filter(|text| !text.is_empty()) +} + /// Parse a PROPFIND multistatus body into normalized entries. fn parse_multistatus(body: &[u8]) -> Result, ApiError> { let text = std::str::from_utf8(body).map_err(|_| ApiError::InvalidResponse)?; @@ -1252,6 +1308,17 @@ mod tests { "#; + const ROOT_ETAG_PROPFIND: &[u8] = br#" + + + /remote.php/dav/files/alice/ + + "cafebabe-0123456789" + HTTP/1.1 200 OK + + +"#; + const POPULATED_PROPFIND: &[u8] = br#" @@ -1644,6 +1711,42 @@ mod tests { assert!(request.url.ends_with("/Documents/")); } + #[test] + fn root_etag_reads_the_etag_with_depth_zero() { + let http = FakeHttp::new(207, ROOT_ETAG_PROPFIND); + let requests = http.requests.clone(); + let api = NextcloudApi::with_http(Box::new(http)); + let etag = api + .root_etag("https://cloud.example.com", "alice", "secret", "/") + .unwrap(); + assert_eq!(etag.as_deref(), Some("\"cafebabe-0123456789\"")); + let request = &requests.borrow()[0]; + assert_eq!(request.method, "PROPFIND"); + assert_eq!(header_value(request, "Depth"), Some("0")); + assert!(request.body.is_some()); + } + + #[test] + fn root_etag_absent_body_yields_none() { + let http = FakeHttp::new(207, EMPTY_PROPFIND); + let api = NextcloudApi::with_http(Box::new(http)); + assert_eq!( + api.root_etag("https://cloud.example.com", "alice", "secret", "/") + .unwrap(), + None + ); + } + + #[test] + fn parse_root_etag_returns_trimmed_text() { + let body = br#" + + "abc" +"#; + assert_eq!(parse_root_etag(body).as_deref(), Some("\"abc\"")); + assert_eq!(parse_root_etag(br#""#), None); + } + #[test] fn probe_http_error_surfaces() { let http = FakeHttp::new(500, b""); diff --git a/src/nextcloud/sync_engine.rs b/src/nextcloud/sync_engine.rs index e3e906e..a5c06b0 100644 --- a/src/nextcloud/sync_engine.rs +++ b/src/nextcloud/sync_engine.rs @@ -150,6 +150,38 @@ pub type RemoteEnsurer = /// instead of retrying a dead server forever. pub type HealthProbe = Arc Result<(), ApiError> + Send + Sync>; +/// Reads the root ETag of a folder to decide whether a periodic interval +/// reconciliation can be skipped (issue #189). +/// +/// Returns `Ok(Some(etag))` when the server reported the folder's ETag, +/// `Ok(None)` when it is absent, and `Err` on transport/auth problems. A +/// mismatch against the recorded value means the remote tree changed; an +/// error means "do not skip" (a real change might go unseen). +pub type EtagProbe = Arc< + dyn Fn(&AccountConfig, &FolderConfig, &str) -> Result, ApiError> + Send + Sync, +>; + +/// Production [`EtagProbe`]: a `PROPFIND Depth:0` for `` on the +/// folder root (mirrors the official client's `RequestEtagJob`). +#[derive(Default)] +pub struct ProductionEtagProbe; + +impl ProductionEtagProbe { + /// Read the folder root ETag for one folder pair. + pub fn run( + account: &AccountConfig, + folder: &FolderConfig, + password: &str, + ) -> Result, ApiError> { + NextcloudApi::new().root_etag( + &account.server_url, + &account.login_name, + password, + &folder.remote_path, + ) + } +} + /// Production [`HealthProbe`]: a short GET to the server's status endpoint. /// /// Nextcloud answers `/status.php`; OpenCloud answers `/` with a status @@ -224,6 +256,13 @@ pub struct SyncEngine { process: Arc>>, remote_ensurer: Option, health_probe: Option, + /// Issue #189: last observed root ETag of this folder, shared between the + /// main thread (captured when a run starts) and the worker. Comparing it + /// against a fresh `PROPFIND` lets the periodic remote-interval skip a + /// full `nextcloudcmd` reconciliation when nothing changed. + etag_slot: Arc>>, + /// Issue #189: reads the folder root ETag before a periodic interval run. + etag_probe: Option, } impl SyncEngine { @@ -251,6 +290,8 @@ impl SyncEngine { process: Arc::new(Mutex::new(None)), remote_ensurer: None, health_probe: None, + etag_slot: Arc::new(Mutex::new(None)), + etag_probe: None, } } @@ -273,6 +314,13 @@ impl SyncEngine { self } + /// Install the folder ETag probe (issue #189). Without it the periodic + /// remote-interval always reconciles (no skip). + pub fn with_etag_probe(mut self, probe: EtagProbe) -> Self { + self.etag_probe = Some(probe); + self + } + /// Whether a reconciliation is currently running. pub fn is_running(&self) -> bool { self.process @@ -283,7 +331,7 @@ impl SyncEngine { } impl SyncRunner for SyncEngine { - fn start(&mut self, _reasons: &[Trigger], on_finished: Box) { + fn start(&mut self, reasons: &[Trigger], on_finished: Box) { let account_id = self.account.id.clone(); let account = self.account.clone(); let folder = self.folder.clone(); @@ -295,6 +343,8 @@ impl SyncRunner for SyncEngine { let process = Arc::clone(&self.process); let remote_ensurer = self.remote_ensurer.clone(); let health_probe = self.health_probe.clone(); + let etag_slot = Arc::clone(&self.etag_slot); + let etag_probe = self.etag_probe.clone(); let inputs = EngineInputs { account, folder, @@ -303,6 +353,9 @@ impl SyncRunner for SyncEngine { executable, remote_ensurer, health_probe, + reasons: reasons.to_vec(), + etag_slot, + etag_probe, }; glib::spawn_future_local(async move { let run = @@ -365,6 +418,13 @@ struct EngineInputs { executable: Option, remote_ensurer: Option, health_probe: Option, + /// Issue #189: the triggers that requested this run (used to decide whether + /// the ETag gate applies, i.e. a periodic interval that may be skipped). + reasons: Vec, + /// Issue #189: shared last-observed root ETag (see [`SyncEngine::etag_slot`]). + etag_slot: Arc>>, + /// Issue #189: reads the folder root ETag before a periodic interval run. + etag_probe: Option, } /// Run the whole reconciliation on the blocking thread pool. @@ -418,6 +478,32 @@ fn engine_thread( Err(_) => {} } } + // Issue #189: for a pure periodic remote-interval run, a cheap root-ETag + // check tells us whether the remote tree changed at all. If it has not, + // skip the whole `nextcloudcmd` reconciliation (it scans the trees and + // emits a huge number of progress events for nothing). This is exactly + // what the official client does with `RequestEtagJob`/`Folder::etagRetrieved`: + // only a changed ETag triggers a full sync. Non-interval runs (manual, + // inotify, startup, remote push, network-restored, resume, retry, local + // recovery) always reconcile. + if etag_gate_applies(&inputs.reasons) { + if let Some(probe) = inputs.etag_probe.as_ref() { + let fresh = probe(&inputs.account, &inputs.folder, &password); + if let Ok(Some(fresh_etag)) = fresh { + let previous = inputs.etag_slot.lock().unwrap().clone(); + if previous.as_deref() == Some(fresh_etag.as_str()) { + // Nothing changed remotely: report a clean success and keep + // the recorded ETag so the next interval also skips. + return EngineRun::Direct(SyncOutcome::Success); + } + // Changed (or first run): record the new ETag and reconcile. + *inputs.etag_slot.lock().unwrap() = Some(fresh_etag); + } + // Ok(None) or Err(_): the server did not answer or the ETag is + // unavailable - do NOT skip the reconciliation (a real change + // might be missed). + } + } let driver = driver_for(inputs.account.provider); let ctx = DriverContext::from_folder( &inputs.account, @@ -511,6 +597,14 @@ fn engine_thread( EngineRun::Result(result) } +/// Whether the request reasons consist solely of the periodic remote interval +/// (issue #189). Only then is the root-ETag gate applied; user-triggered and +/// change-driven runs (inotify, startup, remote push, manual, recovery) must +/// always reconcile, otherwise a real change could be missed. +fn etag_gate_applies(reasons: &[Trigger]) -> bool { + matches!(reasons, [Trigger::RemoteInterval]) +} + /// Drain one process stream: redact, retain the tail and emit parsed /// progress. Both streams carry progress under `QT_FORCE_STDERR_LOGGING` /// (transfers on stderr, summaries on stdout), so both parse; the @@ -672,6 +766,30 @@ mod tests { panic!("timed out waiting for the engine outcome"); } + /// Run an engine with a periodic remote-interval reason (the only trigger + /// the ETag gate applies to), returning the outcome. A skipped run still + /// resolves (success) without ever spawning `nextcloudcmd`. + fn run_engine_interval( + mut engine: SyncEngine, + progress_rx: &async_channel::Receiver, + ) -> SyncOutcome { + let context = glib::MainContext::new(); + let (outcome_tx, outcome_rx) = std::sync::mpsc::channel(); + let outcome = context + .with_thread_default(|| { + engine.start( + &[Trigger::RemoteInterval], + Box::new(move |outcome| { + let _ = outcome_tx.send(outcome); + }), + ); + pump_outcome(&context, &outcome_rx) + }) + .expect("the test main context is available"); + while progress_rx.try_recv().is_ok() {} + outcome + } + #[test] fn emits_progress_and_reports_success() { let dir = tempfile::tempdir().unwrap(); @@ -1155,6 +1273,106 @@ mod tests { assert_eq!(outcome, SyncOutcome::Success); } + /// Issue #189: an unchanged root ETag on a periodic interval skips the + /// reconciliation - `nextcloudcmd` is never spawned and the run reports a + /// clean success (the folder is already up to date). + #[test] + fn unchanged_etag_skips_the_interval_run() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("engine-ran"); + let script = write_script( + dir.path(), + "fake-engine-etag-skip", + &format!("#!/bin/sh\ntouch {}\nexit 0\n", marker.display()), + ); + let (etag_tx, _etag_rx) = async_channel::unbounded(); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + Some(script), + etag_tx, + ) + .with_credentials(Arc::new(FakeCredentials(CredentialLookup::Found( + "secret".to_string(), + )))) + .with_etag_probe(Arc::new(|_account, _folder, _password| { + Ok(Some("\"abc\"".to_string())) + })); + // Seed the slot with the same ETag the probe returns (a prior run + // recorded it), then a pure interval must not reconcile. + *engine.etag_slot.lock().unwrap() = Some("\"abc\"".to_string()); + let outcome = run_engine_interval(engine, &async_channel::unbounded().1); + assert_eq!(outcome, SyncOutcome::Success); + assert!(!marker.exists(), "nextcloudcmd must not be spawned"); + } + + /// Issue #189: a changed root ETag (or no recorded ETag yet, e.g. first + /// run) must reconcile. + #[test] + fn changed_etag_reconciles_the_interval_run() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("engine-ran"); + let script = write_script( + dir.path(), + "fake-engine-etag-changed", + &format!("#!/bin/sh\ntouch {}\nexit 0\n", marker.display()), + ); + let (etag_tx, _etag_rx) = async_channel::unbounded(); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + Some(script), + etag_tx, + ) + .with_credentials(Arc::new(FakeCredentials(CredentialLookup::Found( + "secret".to_string(), + )))) + .with_etag_probe(Arc::new(|_account, _folder, _password| { + Ok(Some("\"new-etag\"".to_string())) + })); + // Previous recorded ETag differs from the fresh one -> must sync. + *engine.etag_slot.lock().unwrap() = Some("\"old-etag\"".to_string()); + let outcome = run_engine_interval(engine, &async_channel::unbounded().1); + assert_eq!(outcome, SyncOutcome::Success); + assert!(marker.exists(), "nextcloudcmd must be spawned"); + } + + /// Issue #189: the gate only applies to a pure remote-interval run; a + /// manual run must always reconcile even if the ETag is unchanged. + #[test] + fn manual_run_ignores_the_etag_gate() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("engine-ran"); + let script = write_script( + dir.path(), + "fake-engine-etag-manual", + &format!("#!/bin/sh\ntouch {}\nexit 0\n", marker.display()), + ); + let (etag_tx, _etag_rx) = async_channel::unbounded(); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + Some(script), + etag_tx, + ) + .with_credentials(Arc::new(FakeCredentials(CredentialLookup::Found( + "secret".to_string(), + )))) + .with_etag_probe(Arc::new(|_account, _folder, _password| { + Ok(Some("\"abc\"".to_string())) + })); + *engine.etag_slot.lock().unwrap() = Some("\"abc\"".to_string()); + let (outcome, _) = run_engine(engine, &async_channel::unbounded().1); // Manual + assert_eq!(outcome, SyncOutcome::Success); + assert!(marker.exists(), "manual run must reconcile"); + } + /// Issue #179: a run that ends Failed while the server does not answer a /// health probe must be reported as NetworkError (the account is /// unreachable, not the folder broken).