diff --git a/breadpad-shared/src/ai.rs b/breadpad-shared/src/ai.rs index 7bb69ad..b6afa17 100644 --- a/breadpad-shared/src/ai.rs +++ b/breadpad-shared/src/ai.rs @@ -61,8 +61,14 @@ impl OllamaClient { "stream": false }); + // ureq 2's default agent has no overall request timeout, so a hung + // local Ollama endpoint would otherwise stall this call forever — + // and since classification now runs from an idle callback after the + // capture window has already closed (see `main.rs`), a hang here is + // invisible to the user, not just slow. let response = ureq::post(&url) .set("Content-Type", "application/json") + .timeout(std::time::Duration::from_secs(10)) .send_json(payload) .map_err(|e| anyhow::anyhow!("Ollama HTTP error: {}", e))?; diff --git a/breadpad-shared/src/calendar.rs b/breadpad-shared/src/calendar.rs index 479ad05..99df780 100644 --- a/breadpad-shared/src/calendar.rs +++ b/breadpad-shared/src/calendar.rs @@ -18,7 +18,13 @@ impl CalDavClient { pub fn new(config: CalendarConfig) -> Self { // `reqwest::Client::builder().build()` can only fail if the TLS backend can't be // initialised; fall back to `Client::new()` semantics rather than panicking. + // + // A request-wide timeout is set here (rather than on `Client::new()`'s + // untimed defaults) so a hung/unreachable CalDAV server can't hang + // whatever's making the request indefinitely — `reqwest::Client::new()` + // has no timeout of its own. let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(15)) .build() .unwrap_or_else(|e| { tracing::warn!("falling back to default HTTP client: {}", e); diff --git a/breadpad-shared/src/config.rs b/breadpad-shared/src/config.rs index 9d81e5e..2eeb3d5 100644 --- a/breadpad-shared/src/config.rs +++ b/breadpad-shared/src/config.rs @@ -193,6 +193,20 @@ impl Config { } let text = toml::to_string_pretty(self)?; fs::write(&path, text)?; + + // This file can hold the CalDAV password in plaintext (see + // `CalendarConfig`'s own doc comment) — `fs::write` creates it with + // the process's default umask, which on most setups means + // world-readable. Lock it down to owner-only rather than just + // telling the user to do it themselves. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if let Err(e) = fs::set_permissions(&path, fs::Permissions::from_mode(0o600)) { + tracing::warn!("failed to restrict permissions on {}: {}", path.display(), e); + } + } + Ok(()) } } diff --git a/breadpad-shared/src/scheduler.rs b/breadpad-shared/src/scheduler.rs index ff5ff26..98d9f2b 100644 --- a/breadpad-shared/src/scheduler.rs +++ b/breadpad-shared/src/scheduler.rs @@ -206,6 +206,42 @@ pub(crate) fn parse_next_from_rrule(rrule_str: &str, default_morning: &str) -> O (now.date_naive() + chrono::Duration::days(days_ahead)).and_time(fire_time); return Some(local_naive_to_utc(target_date)); } + "MONTHLY" => { + use chrono::Datelike; + // BYMONTHDAY isn't guaranteed to be present — breadman's note + // editor lets a user type an arbitrary RRULE by hand, and + // "FREQ=MONTHLY" alone is a perfectly valid (if under-specified) + // one. Fall back to today's day-of-month, mirroring how WEEKLY + // above defaults BYDAY to "MO" when absent. + let day: u32 = parts + .get("BYMONTHDAY") + .and_then(|v| v.parse().ok()) + .filter(|d: &u32| (1..=31).contains(d)) + .unwrap_or_else(|| now.day()); + + let mut year = now.year(); + let mut month = now.month(); + + // Walk forward month by month for the next calendar date that + // (a) actually has this day-of-month (a 31st skips e.g. April) + // and (b) is still in the future. Bounded to 24 months as a + // defensive cap — every valid day (1-31) recurs well within a + // year, so this should never come close to firing. + for _ in 0..24 { + if let Some(date) = chrono::NaiveDate::from_ymd_opt(year, month, day) { + let candidate = date.and_time(fire_time); + if now.naive_local() < candidate { + return Some(local_naive_to_utc(candidate)); + } + } + month += 1; + if month > 12 { + month = 1; + year += 1; + } + } + None + } _ => None, } } @@ -404,7 +440,40 @@ mod tests { #[test] fn unknown_freq_returns_none() { - assert!(parse_next_from_rrule("RRULE:FREQ=MONTHLY;BYHOUR=9;BYMINUTE=0", "08:00").is_none()); + assert!(parse_next_from_rrule("RRULE:FREQ=YEARLY;BYHOUR=9;BYMINUTE=0", "08:00").is_none()); + } + + #[test] + fn monthly_reschedules_instead_of_returning_none() { + // This used to be the exact bug: MONTHLY fell into the `_ => None` + // arm, so a monthly reminder fired once and never rescheduled. + let t = parse_next_from_rrule("RRULE:FREQ=MONTHLY;BYMONTHDAY=15;BYHOUR=9;BYMINUTE=0", "08:00"); + assert!(t.is_some(), "MONTHLY must produce a next occurrence, not None"); + let local: chrono::DateTime = t.unwrap().into(); + assert_eq!(local.day(), 15); + assert_eq!(local.hour(), 9); + assert_eq!(local.minute(), 0); + assert!(local > Local::now()); + } + + #[test] + fn monthly_without_bymonthday_uses_todays_day_of_month() { + let t = parse_next_from_rrule("RRULE:FREQ=MONTHLY;BYHOUR=23;BYMINUTE=59", "08:00").unwrap(); + let local: chrono::DateTime = t.into(); + assert_eq!(local.day(), Local::now().day()); + } + + #[test] + fn monthly_on_the_31st_skips_shorter_months() { + // Every candidate month/day combination this walks must actually + // exist (from_ymd_opt returning None for e.g. April 31 is skipped + // internally) — this mostly guards against a panic/infinite loop + // regression rather than a specific date, since "next Feb 31" et al + // must fall through to a month that really has a 31st. + let t = parse_next_from_rrule("RRULE:FREQ=MONTHLY;BYMONTHDAY=31;BYHOUR=9;BYMINUTE=0", "08:00"); + assert!(t.is_some()); + let local: chrono::DateTime = t.unwrap().into(); + assert_eq!(local.day(), 31); } #[test] diff --git a/breadpad-shared/src/store.rs b/breadpad-shared/src/store.rs index bc8b86a..653eff8 100644 --- a/breadpad-shared/src/store.rs +++ b/breadpad-shared/src/store.rs @@ -44,11 +44,43 @@ impl Store { } } + /// Path to the sidecar lock file guarding the whole note store — a + /// single lock covers both `notes.jsonl` and `archive.jsonl` since + /// `rotate_archive` moves notes between them in one logical operation. + fn lock_path(&self) -> PathBuf { + self.notes_path.with_extension("lock") + } + + /// Blocks until an exclusive advisory lock (`flock`) is held on the + /// sidecar lock file, and holds it for as long as the returned `File` + /// stays alive (the lock is released automatically when it's dropped, + /// same as it would be on process exit/crash). + /// + /// breadpad (capture/fire/snooze), breadman (edit), and multiple + /// concurrent reminder-fire processes all read and rewrite the same + /// JSONL file with no coordination otherwise: two concurrent + /// load-modify-rewrite cycles racing `write_all`'s rename can silently + /// lose one side's change. This is intentionally one lock for the + /// entire store rather than per-note or per-file locking — contention + /// is expected to be rare (a handful of short-lived CLI-ish processes, + /// not a server), so simplicity wins over fine-grained locking here. + fn acquire_lock(&self) -> Result { + let file = OpenOptions::new() + .create(true) + .write(true) + .open(self.lock_path()) + .context("failed to open notes store lock file")?; + file.lock().context("failed to acquire notes store lock")?; + Ok(file) + } + pub fn load_all(&self) -> Result> { + let _lock = self.acquire_lock()?; self.load_from(&self.notes_path) } pub fn load_archive(&self) -> Result> { + let _lock = self.acquire_lock()?; self.load_from(&self.archive_path) } @@ -74,12 +106,15 @@ impl Store { } pub fn save_note(&self, note: &Note) -> Result<()> { - let mut file = OpenOptions::new() - .create(true) - .append(true) - .open(&self.notes_path)?; - let line = serde_json::to_string(note)?; - writeln!(file, "{}", line)?; + { + let _lock = self.acquire_lock()?; + let mut file = OpenOptions::new() + .create(true) + .append(true) + .open(&self.notes_path)?; + let line = serde_json::to_string(note)?; + writeln!(file, "{}", line)?; + } if let Some(cal_cfg) = &self.calendar { if cal_cfg.enabled && (note.time.is_some() || note.rrule.is_some()) { @@ -103,13 +138,18 @@ impl Store { } pub fn delete_note(&self, id: &str) -> Result<()> { - let all = self.load_all()?; - let (to_delete, keep): (Vec, Vec) = all.into_iter().partition(|n| n.id == id); - self.write_all(&self.notes_path, &keep)?; + let to_delete_note = { + let _lock = self.acquire_lock()?; + let all = self.load_from(&self.notes_path)?; + let (to_delete, keep): (Vec, Vec) = + all.into_iter().partition(|n| n.id == id); + self.write_all(&self.notes_path, &keep)?; + to_delete.into_iter().next() + }; if let Some(cal_cfg) = &self.calendar { if cal_cfg.enabled { - if let Some(note) = to_delete.into_iter().next() { + if let Some(note) = to_delete_note { spawn_caldav_delete(caldav_uid(¬e), cal_cfg.clone()); } } @@ -118,11 +158,17 @@ impl Store { Ok(()) } + /// Holds the store lock across the whole load-modify-write span (not + /// just the write) — `load_from` is used directly here rather than the + /// public, self-locking `load_all`, since re-acquiring the same + /// process-wide advisory lock while already holding it would block + /// forever (`flock` isn't re-entrant across separate file descriptors). fn rewrite_notes(&self, mut f: F) -> Result<()> where F: FnMut(Note) -> Note, { - let notes: Vec = self.load_all()?.into_iter().map(|n| f(n)).collect(); + let _lock = self.acquire_lock()?; + let notes: Vec = self.load_from(&self.notes_path)?.into_iter().map(|n| f(n)).collect(); self.write_all(&self.notes_path, ¬es) } @@ -141,8 +187,9 @@ impl Store { } pub fn rotate_archive(&self, archive_after_days: i64) -> Result { + let _lock = self.acquire_lock()?; let cutoff = Utc::now() - Duration::days(archive_after_days); - let notes = self.load_all()?; + let notes = self.load_from(&self.notes_path)?; let (to_archive, keep): (Vec, Vec) = notes .into_iter() .partition(|n| n.done && n.completed.map_or(false, |c| c < cutoff)); diff --git a/breadpad/src/main.rs b/breadpad/src/main.rs index 033a114..e33087c 100644 --- a/breadpad/src/main.rs +++ b/breadpad/src/main.rs @@ -694,6 +694,7 @@ fn build_window( let selected_type_clone = selected_type.clone(); let cfg_clone = cfg.clone(); let workspace_clone = workspace.clone(); + let app_clone = app.clone(); let save_and_close = { let win = win_clone.clone(); @@ -701,6 +702,7 @@ fn build_window( let selected_type = selected_type_clone.clone(); let cfg = cfg_clone.clone(); let workspace = workspace_clone.clone(); + let app = app_clone.clone(); move || { let text = entry.text().to_string(); @@ -711,10 +713,20 @@ fn build_window( let note_type = selected_type.borrow().clone(); let cfg_c = cfg.clone(); let ws_c = workspace.clone(); - // Close first so the popup disappears immediately, then save. + // Close first so the popup disappears immediately, then save — + // but `hold()` the application across the gap first. Without + // this, closing the only open window can let the whole process + // (and with it, the `idle_add_local_once` callback below that + // actually writes the note) exit before that callback ever + // runs, silently losing the note the user just typed. `hold()` + // returns an RAII guard that keeps the app alive with zero + // windows open until it's dropped, right after the save + // finishes below. + let hold_guard = app.hold(); win.close(); glib::idle_add_local_once(move || { save_note_classified(&text, note_type, no_classify, cfg_c, ws_c); + drop(hold_guard); }); } };