From 470d2aa7e9e3aff69b8cbee07e2ab12c50c5829f Mon Sep 17 00:00:00 2001 From: Breadway Date: Tue, 25 Aug 2026 16:13:59 +0800 Subject: [PATCH] bread-launcher: log history save failures, clarify LAUNCHER_APP vs event namespace LaunchHistory::save silently swallowed write errors; now shared by two hosts (breadbox + breadbar's capsule), a broken cache dir silently stopped recording launches for both. Log on failure instead. do_launch/emit_launched's app_id parameter is the caller's bread event-namespace id (breadbox passes "box"), not LAUNCHER_APP ("breadbox", scoped to cache/history paths only) - two similarly named but distinct identities. Make the doc comments say so explicitly and add tests pinning the exact namespace-check relationship, since confusing them silently drops the emitted event. --- bread-launcher/src/history.rs | 93 ++++++++++++++++++++++++++++++++--- bread-launcher/src/launch.rs | 84 +++++++++++++++++++++++++++++-- bread-launcher/src/lib.rs | 7 +++ 3 files changed, 174 insertions(+), 10 deletions(-) diff --git a/bread-launcher/src/history.rs b/bread-launcher/src/history.rs index 2d476cf..d642141 100644 --- a/bread-launcher/src/history.rs +++ b/bread-launcher/src/history.rs @@ -25,16 +25,34 @@ impl LaunchHistory { *self.counts.entry(name.to_string()).or_insert(0) += 1; } + /// Writes `counts` to `path` as JSON. Best-effort — a broken cache dir + /// (missing parent, full disk, permissions) must not stop the caller + /// from launching anything, so this never returns an error — but it now + /// logs one on failure rather than swallowing it silently. Shared by two + /// hosts (breadbox's overlay and breadbar's embedded capsule, both keyed + /// under [`crate::LAUNCHER_APP`]), so a save failure here silently stops + /// ranking history for both. pub fn save(&self) { - if let Ok(json) = serde_json::to_string(&self.counts) { - let _ = fs::write(&self.path, json); + match serde_json::to_string(&self.counts) { + Ok(json) => { + if let Err(err) = fs::write(&self.path, json) { + eprintln!( + "bread-launcher: failed to save launch history to {}: {err}", + self.path.display() + ); + } + } + Err(err) => { + eprintln!("bread-launcher: failed to serialize launch history: {err}"); + } } } - /// In-memory history with no backing file — [`save`](Self::save) is a - /// silent no-op (an empty `path`). Lets a test (or a future in-memory - /// host) control counts directly instead of writing through - /// `~/.cache//history.json`. + /// In-memory history with no backing file — [`save`](Self::save) fails + /// (an empty `path` is not writable) and now logs that failure to + /// stderr rather than swallowing it, same as any other broken-path + /// case. Lets a test (or a future in-memory host) control counts + /// directly instead of writing through `~/.cache//history.json`. #[cfg(test)] pub(crate) fn from_counts(counts: HashMap) -> Self { LaunchHistory { @@ -43,3 +61,66 @@ impl LaunchHistory { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn temp_history_path(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "bread-launcher-history-test-{}-{name}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + )); + std::fs::create_dir_all(&dir).unwrap(); + dir.join("history.json") + } + + #[test] + fn save_then_load_round_trips_counts() { + let path = temp_history_path("roundtrip"); + let mut history = LaunchHistory { + counts: HashMap::new(), + path: path.clone(), + }; + history.increment("firefox.desktop"); + history.increment("firefox.desktop"); + history.increment("kitty.desktop"); + history.save(); + + let text = std::fs::read_to_string(&path).expect("save should have written the file"); + let counts: HashMap = serde_json::from_str(&text).unwrap(); + assert_eq!(counts.get("firefox.desktop"), Some(&2)); + assert_eq!(counts.get("kitty.desktop"), Some(&1)); + + // load() from the same path should see the same counts. + let reloaded = LaunchHistory { + counts: serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(), + path: path.clone(), + }; + assert_eq!(reloaded.count("firefox.desktop"), 2); + + let _ = std::fs::remove_dir_all(path.parent().unwrap()); + } + + /// `save()` on an unwritable path (e.g. the parent directory doesn't + /// exist, or `path` is empty) must not panic — it's best-effort, called + /// from a launcher's shutdown path where a hard failure would be worse + /// than a lost history entry. This exercises exactly the failure branch + /// the `eprintln!` above was added for; there is no return value to + /// assert on, so "did not panic" is the contract under test. + #[test] + fn save_to_a_broken_path_does_not_panic() { + let history = LaunchHistory::from_counts(HashMap::from([("x".to_string(), 1)])); + history.save(); + + let history = LaunchHistory { + counts: HashMap::new(), + path: PathBuf::from("/nonexistent-dir/definitely-not-there/history.json"), + }; + history.save(); + } +} diff --git a/bread-launcher/src/launch.rs b/bread-launcher/src/launch.rs index 410ad4c..d035a52 100644 --- a/bread-launcher/src/launch.rs +++ b/bread-launcher/src/launch.rs @@ -25,6 +25,19 @@ fn pick_terminal() -> String { /// Spawns `entry`'s command (through a terminal if `entry.terminal` is set) /// and, on a successful spawn, publishes `event` via [`emit_launched`]. +/// +/// `app_id` here is the caller's **bread event-namespace id** (e.g. +/// `"box"` for breadbox) — NOT [`crate::LAUNCHER_APP`] (`"breadbox"`). +/// Those are two different identities that happen to look similar: +/// `LAUNCHER_APP` only picks the shared cache/history directory (see its own +/// doc comment), while `app_id` here is threaded straight into +/// `BreadClient::connect(app_id)` and must be the caller's own namespace, or +/// `BreadClient::emit`'s `validate_app_namespace` check +/// (`event.starts_with("bread.{app_id}.")`) rejects `event` and drops it +/// with only an eprintln — passing `LAUNCHER_APP` here by mistake is exactly +/// that bug. breadbox passes its own `"box"` (see breadbox's `APP_ID`) so +/// its events publish as `bread.box.*`, matching [`emit_launched`]'s doc +/// example below. pub fn do_launch(entry: &DesktopEntry, app_id: &str, event: &str) { let cmd = entry.exec.trim(); let spawned = if entry.terminal { @@ -48,10 +61,14 @@ pub fn do_launch(entry: &DesktopEntry, app_id: &str, event: &str) { } } -/// Publishes `event` (e.g. `bread.box.launched`) as `app_id` after a -/// successful spawn. Fire-and-forget and non-fatal (`BreadClient::emit` -/// never blocks or errors this caller) — breadd being absent must never -/// affect launching itself. +/// Publishes `event` under `app_id`'s bread namespace after a successful +/// spawn — e.g. breadbox calls this with `app_id = "box"` and +/// `event = "bread.box.launched"`, its own namespace. Fire-and-forget and +/// non-fatal (`BreadClient::emit` never blocks or errors this caller) — +/// breadd being absent must never affect launching itself. `app_id` must be +/// the caller's *own* namespace id, not [`crate::LAUNCHER_APP`] — see +/// [`do_launch`]'s doc comment for why those are different identities and +/// what happens if they're confused. pub fn emit_launched(entry: &DesktopEntry, app_id: &str, event: &str) { let id = if entry.id.is_empty() { entry.exec.as_str() @@ -63,3 +80,62 @@ pub fn emit_launched(entry: &DesktopEntry, app_id: &str, event: &str) { serde_json::json!({ "id": id, "name": entry.name }), ); } + +#[cfg(test)] +mod tests { + use super::*; + + /// Mirrors `bread_shared::apps::validate_app_namespace` exactly + /// (`event.starts_with(&format!("bread.{app}."))`) without pulling in + /// that crate here — this is the one check that decides whether + /// [`emit_launched`]'s event actually gets published. + fn passes_namespace_check(app_id: &str, event: &str) -> bool { + event.starts_with(&format!("bread.{app_id}.")) + } + + #[test] + fn documented_app_id_and_event_pair_passes_the_namespace_check() { + // breadbox's real call site (breadbox/breadbox/src/main.rs): + // APP_ID = "box", LAUNCHED_EVENT = "bread.box.launched". + assert!( + passes_namespace_check("box", "bread.box.launched"), + "do_launch/emit_launched's own doc example must actually pass \ + BreadClient::emit's namespace check" + ); + } + + #[test] + fn launcher_app_is_not_a_valid_app_id_for_the_documented_event() { + // The historical bug this doc fix guards against: passing + // `LAUNCHER_APP` ("breadbox", the cache/history identity) as + // `app_id` instead of the caller's own namespace id ("box") would + // silently drop `bread.box.launched` — event.starts_with( + // "bread.breadbox.") is false for "bread.box.launched". + assert!( + !passes_namespace_check(crate::LAUNCHER_APP, "bread.box.launched"), + "LAUNCHER_APP must NOT satisfy the namespace check for the \ + documented event — if this ever passes, do_launch's doc comment \ + warning about confusing the two identities is wrong" + ); + } + + #[test] + fn emit_launched_does_not_panic_when_breadd_is_unreachable() { + // No daemon is running in a test environment — emit_launched (and + // the BreadClient::emit it wraps) must degrade silently rather than + // panicking or blocking, for both a valid and a namespace-violating + // app_id. + let entry = DesktopEntry { + id: "firefox.desktop".to_string(), + name: "Firefox".to_string(), + exec: "firefox".to_string(), + icon_name: String::new(), + icon_path: None, + categories: vec![], + wm_class: None, + terminal: false, + }; + emit_launched(&entry, "box", "bread.box.launched"); + emit_launched(&entry, crate::LAUNCHER_APP, "bread.box.launched"); + } +} diff --git a/bread-launcher/src/lib.rs b/bread-launcher/src/lib.rs index 1c91f49..4f5952a 100644 --- a/bread-launcher/src/lib.rs +++ b/bread-launcher/src/lib.rs @@ -53,4 +53,11 @@ pub use query::{builtin_commands, eval_calc, filter_commands, parse_query, Comma /// this constant — that reads exactly like an unfixed bug (breadbar naming /// another app's identity) and invites a later "fix" that would quietly /// break the shared history this constant exists to guarantee. +/// +/// **Not** the `app_id` for [`do_launch`]/[`emit_launched`]: those publish +/// bread-bus events, which must be namespaced under the caller's *own* +/// identity (breadbox's is `"box"`, not `"breadbox"`) or +/// `BreadClient::emit`'s namespace check silently drops them. This constant +/// is scoped to the cache/history path family only — see [`do_launch`]'s +/// doc comment for the concrete failure mode if the two get swapped. pub const LAUNCHER_APP: &str = "breadbox";