From 3b8b6b4d5249f7a98656ee53844f2fc412e2ea5c Mon Sep 17 00:00:00 2001 From: protagonista-design <64110002+usefulish@users.noreply.github.com> Date: Tue, 14 Jul 2026 15:06:50 -0400 Subject: [PATCH] fix: log hook and dispatch errors instead of exiting Any hook run error (typo in command path, resource exhaustion...) or failure to canonicalize an event's folder path propagated to main and killed the daemon on the first triggered event. Log these errors and keep the event loop running. Event dispatch is extracted into a function so the behavior is testable. Co-Authored-By: Claude Fable 5 --- src/main.rs | 190 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 136 insertions(+), 54 deletions(-) diff --git a/src/main.rs b/src/main.rs index d3461e5..8493777 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,6 +6,8 @@ use std::{ hash_map::{Entry, HashMap}, }, io, + path::Path, + process::Child, rc::Rc, sync::{Arc, LazyLock, Mutex, mpsc}, thread, @@ -31,7 +33,76 @@ static CONFLICT_MATCHER: LazyLock = LazyLock::new(|| { .compile_matcher() }); -#[expect(clippy::too_many_lines)] +/// Map from event kind + folder to the hooks to run +type HookMap = HashMap<(config::FolderEvent, Rc), Vec>; + +/// Run a hook, logging errors instead of propagating them, so that a failing hook +/// (typo in command path, resource exhaustion...) can not crash the daemon +fn run_hook( + hook: &config::FolderHook, + path: Option<&Path>, + folder: &NormalizedPath, + reaper_tx: &mpsc::Sender<(hook::FolderHookId, Child)>, + running_hooks: &Arc>>, +) { + if let Err(err) = hook::run(hook, path, folder, reaper_tx, running_hooks) { + log::error!( + "Failed to run hook command {command:?}: {err:#}", + command = hook.command + ); + } +} + +/// Dispatch a single event to its matching hooks, individual hook run errors are +/// logged and do not interrupt dispatching +fn dispatch_event( + event: &syncthing::Event, + hooks_map: &HookMap, + reaper_tx: &mpsc::Sender<(hook::FolderHookId, Child)>, + running_hooks: &Arc>>, +) -> anyhow::Result<()> { + match event { + syncthing::Event::FileDownSyncDone { path, folder } => { + let folder: Rc = Rc::new(folder.as_path().try_into()?); + for hook in hooks_map + .get(&(config::FolderEvent::FileDownSyncDone, Rc::clone(&folder))) + .unwrap_or(&vec![]) + { + if hook.filter.as_ref().is_none_or(|g| g.is_match(path)) { + run_hook(hook, Some(path), &folder, reaper_tx, running_hooks); + } + } + for hook in hooks_map + .get(&(config::FolderEvent::RemoteFileConflict, Rc::clone(&folder))) + .unwrap_or(&vec![]) + { + if CONFLICT_MATCHER.is_match(path) { + run_hook(hook, Some(path), &folder, reaper_tx, running_hooks); + } + } + } + syncthing::Event::FolderDownSyncDone { folder } => { + let folder: Rc = Rc::new(folder.as_path().try_into()?); + for hook in hooks_map + .get(&(config::FolderEvent::FolderDownSyncDone, Rc::clone(&folder))) + .unwrap_or(&vec![]) + { + run_hook(hook, None, &folder, reaper_tx, running_hooks); + } + } + syncthing::Event::FileConflict { path, folder } => { + let folder: Rc = Rc::new(folder.as_path().try_into()?); + for hook in hooks_map + .get(&(config::FolderEvent::FileConflict, Rc::clone(&folder))) + .unwrap_or(&vec![]) + { + run_hook(hook, Some(path), &folder, reaper_tx, running_hooks); + } + } + } + Ok(()) +} + fn main() -> anyhow::Result<()> { // Init logger simple_logger::SimpleLogger::new() @@ -43,8 +114,7 @@ fn main() -> anyhow::Result<()> { let (cfg, hooks) = config::parse().context("Failed to read local config")?; // Build hook map for fast matching - let mut hooks_map: HashMap<(config::FolderEvent, Rc), Vec> = - HashMap::new(); + let mut hooks_map: HookMap = HashMap::new(); for hook in &hooks.hooks { match hooks_map.entry((hook.event.clone(), Rc::new(hook.folder.clone()))) { Entry::Occupied(mut e) => { @@ -102,57 +172,11 @@ fn main() -> anyhow::Result<()> { }; log::info!("New event: {event:?}"); - // Dispatch event - match event { - syncthing::Event::FileDownSyncDone { path, folder } => { - let folder: Rc = Rc::new(folder.as_path().try_into()?); - for hook in hooks_map - .get(&(config::FolderEvent::FileDownSyncDone, Rc::clone(&folder))) - .unwrap_or(&vec![]) - { - if hook.filter.as_ref().is_none_or(|g| g.is_match(path)) { - hook::run( - hook, - Some(path), - &folder, - &reaper_tx, - &running_hooks, - )?; - } - } - for hook in hooks_map - .get(&(config::FolderEvent::RemoteFileConflict, Rc::clone(&folder))) - .unwrap_or(&vec![]) - { - if CONFLICT_MATCHER.is_match(path) { - hook::run( - hook, - Some(path), - &folder, - &reaper_tx, - &running_hooks, - )?; - } - } - } - syncthing::Event::FolderDownSyncDone { folder } => { - let folder: Rc = Rc::new(folder.as_path().try_into()?); - for hook in hooks_map - .get(&(config::FolderEvent::FolderDownSyncDone, Rc::clone(&folder))) - .unwrap_or(&vec![]) - { - hook::run(hook, None, &folder, &reaper_tx, &running_hooks)?; - } - } - syncthing::Event::FileConflict { path, folder } => { - let folder: Rc = Rc::new(folder.as_path().try_into()?); - for hook in hooks_map - .get(&(config::FolderEvent::FileConflict, Rc::clone(&folder))) - .unwrap_or(&vec![]) - { - hook::run(hook, Some(path), &folder, &reaper_tx, &running_hooks)?; - } - } + // Dispatch event, errors (unresolvable folder path, failing hook...) + // are logged and do not stop the event loop + if let Err(err) = dispatch_event(event, &hooks_map, &reaper_tx, &running_hooks) + { + log::error!("Failed to dispatch event {event:?}: {err:#}"); } } cursor = events.cursor(); @@ -177,6 +201,8 @@ fn main() -> anyhow::Result<()> { #[cfg(test)] mod tests { + use std::path::PathBuf; + use super::*; /// Conflict file names as created by Syncthing must match, at any folder depth @@ -187,4 +213,60 @@ mod tests { assert!(!CONFLICT_MATCHER.is_match("doc.txt")); assert!(!CONFLICT_MATCHER.is_match("sync-conflict.txt")); } + + /// Hook running `command` for `folder` on file down sync done + fn test_hook(folder: &NormalizedPath, command: &[&str]) -> config::FolderHook { + config::FolderHook { + folder: folder.clone(), + event: config::FolderEvent::FileDownSyncDone, + filter: None, + command: command.iter().map(|a| (*a).to_owned()).collect(), + allow_concurrent: None, + } + } + + /// A hook that fails to run must not interrupt dispatching: following hooks still run + #[test] + fn failing_hook_does_not_interrupt_dispatch() { + let dir = tempfile::tempdir().unwrap(); + let folder: NormalizedPath = dir.path().try_into().unwrap(); + let marker = dir.path().join("marker"); + let hooks = vec![ + test_hook(&folder, &["/nonexistent/hook/command"]), + test_hook(&folder, &["touch", marker.to_str().unwrap()]), + ]; + let mut hooks_map = HookMap::new(); + hooks_map.insert( + (config::FolderEvent::FileDownSyncDone, Rc::new(folder)), + hooks, + ); + let running_hooks = Arc::new(Mutex::new(HashSet::new())); + let (reaper_tx, reaper_rx) = mpsc::channel(); + let event = syncthing::Event::FileDownSyncDone { + path: PathBuf::from("file.txt"), + folder: dir.path().to_owned(), + }; + + dispatch_event(&event, &hooks_map, &reaper_tx, &running_hooks).unwrap(); + + // Only the second hook spawned a process + let (_hook_id, mut child) = reaper_rx.try_recv().unwrap(); + assert!(child.wait().unwrap().success()); + assert!(reaper_rx.try_recv().is_err()); + assert!(marker.is_file()); + } + + /// An event folder that does not resolve locally is an error, not an exit or panic + #[test] + fn unresolvable_folder_path_is_an_error() { + let hooks_map = HookMap::new(); + let running_hooks = Arc::new(Mutex::new(HashSet::new())); + let (reaper_tx, _reaper_rx) = mpsc::channel(); + let event = syncthing::Event::FileDownSyncDone { + path: PathBuf::from("file.txt"), + folder: PathBuf::from("/nonexistent/stfed/folder"), + }; + + assert!(dispatch_event(&event, &hooks_map, &reaper_tx, &running_hooks).is_err()); + } }