From c05b8f6879bad7fa9a49094b15a39a45a0b458d6 Mon Sep 17 00:00:00 2001 From: James Trew <66286082+jamestrew@users.noreply.github.com> Date: Sun, 9 Mar 2025 18:34:49 -0400 Subject: fix: improve broken symlink error handling (#2589) Check atuin setting paths (eg. `db_path`) for broken symlinks on initialization and disable all shell hooks + print error message. sqlite doesn't create db files even with `.create_if_missing` when the db files are a broken symlink. This would cause sqlite to error and atuin to panic on every single keypress. Also improves related error handling when calling atuin client commands directly. --- crates/atuin-client/src/database.rs | 9 ++-- crates/atuin-client/src/record/sqlite_store.rs | 9 +++- crates/atuin-client/src/settings.rs | 32 ++++++++++----- crates/atuin-common/src/utils.rs | 5 +++ crates/atuin/src/command/client.rs | 5 +-- crates/atuin/src/command/client/doctor.rs | 57 ++++++++++++++++++++++---- crates/atuin/src/command/client/init.rs | 5 +++ crates/atuin/src/command/client/search.rs | 6 +-- 8 files changed, 99 insertions(+), 29 deletions(-) diff --git a/crates/atuin-client/src/database.rs b/crates/atuin-client/src/database.rs index 5bdbb75c..b64ff4ce 100644 --- a/crates/atuin-client/src/database.rs +++ b/crates/atuin-client/src/database.rs @@ -130,8 +130,12 @@ impl Sqlite { let path = path.as_ref(); debug!("opening sqlite database at {:?}", path); - let create = !path.exists(); - if create { + if utils::broken_symlink(path) { + eprintln!("Atuin: Sqlite db path ({path:?}) is a broken symlink. Unable to read or create replacement."); + std::process::exit(1); + } + + if !path.exists() { if let Some(dir) = path.parent() { fs::create_dir_all(dir)?; } @@ -150,7 +154,6 @@ impl Sqlite { .await?; Self::setup_db(&pool).await?; - Ok(Self { pool }) } diff --git a/crates/atuin-client/src/record/sqlite_store.rs b/crates/atuin-client/src/record/sqlite_store.rs index 2937dbd7..c42476d4 100644 --- a/crates/atuin-client/src/record/sqlite_store.rs +++ b/crates/atuin-client/src/record/sqlite_store.rs @@ -17,6 +17,7 @@ use sqlx::{ use atuin_common::record::{ EncryptedData, Host, HostId, Record, RecordId, RecordIdx, RecordStatus, }; +use atuin_common::utils; use uuid::Uuid; use super::encryption::PASETO_V4; @@ -33,8 +34,12 @@ impl SqliteStore { debug!("opening sqlite database at {:?}", path); - let create = !path.exists(); - if create { + if utils::broken_symlink(path) { + eprintln!("Atuin: Sqlite db path ({path:?}) is a broken symlink. Unable to read or create replacement."); + std::process::exit(1); + } + + if !path.exists() { if let Some(dir) = path.parent() { fs::create_dir_all(dir)?; } diff --git a/crates/atuin-client/src/settings.rs b/crates/atuin-client/src/settings.rs index c01281c7..a1b0724f 100644 --- a/crates/atuin-client/src/settings.rs +++ b/crates/atuin-client/src/settings.rs @@ -3,6 +3,7 @@ use std::{ }; use atuin_common::record::HostId; +use atuin_common::utils; use clap::ValueEnum; use config::{ builder::DefaultState, Config, ConfigBuilder, Environment, File as ConfigFile, FileFormat, @@ -852,24 +853,33 @@ impl Settings { .map_err(|e| eyre!("failed to deserialize: {}", e))?; // all paths should be expanded - let db_path = settings.db_path; - let db_path = shellexpand::full(&db_path)?; - settings.db_path = db_path.to_string(); - - let key_path = settings.key_path; - let key_path = shellexpand::full(&key_path)?; - settings.key_path = key_path.to_string(); - - let session_path = settings.session_path; - let session_path = shellexpand::full(&session_path)?; - settings.session_path = session_path.to_string(); + settings.db_path = Self::expand_path(settings.db_path)?; + settings.record_store_path = Self::expand_path(settings.record_store_path)?; + settings.key_path = Self::expand_path(settings.key_path)?; + settings.session_path = Self::expand_path(settings.session_path)?; Ok(settings) } + fn expand_path(path: String) -> Result { + shellexpand::full(&path) + .map(|p| p.to_string()) + .map_err(|e| eyre!("failed to expand path: {}", e)) + } + pub fn example_config() -> &'static str { EXAMPLE_CONFIG } + + pub fn paths_ok(&self) -> bool { + let paths = [ + &self.db_path, + &self.record_store_path, + &self.key_path, + &self.session_path, + ]; + paths.iter().all(|p| !utils::broken_symlink(p)) + } } impl Default for Settings { diff --git a/crates/atuin-common/src/utils.rs b/crates/atuin-common/src/utils.rs index fbf2ef82..d495de36 100644 --- a/crates/atuin-common/src/utils.rs +++ b/crates/atuin-common/src/utils.rs @@ -113,6 +113,11 @@ pub fn get_current_dir() -> String { } } +pub fn broken_symlink>(path: P) -> bool { + let path = path.into(); + path.is_symlink() && !path.exists() +} + pub fn is_zsh() -> bool { // only set on zsh env::var("ATUIN_SHELL_ZSH").is_ok() diff --git a/crates/atuin/src/command/client.rs b/crates/atuin/src/command/client.rs index 2637b691..723bb974 100644 --- a/crates/atuin/src/command/client.rs +++ b/crates/atuin/src/command/client.rs @@ -129,6 +129,7 @@ impl Cmd { match self { Self::History(history) => return history.run(&settings).await, Self::Init(init) => return init.run(&settings).await, + Self::Doctor => return doctor::run(&settings).await, _ => {} } @@ -163,8 +164,6 @@ impl Cmd { Ok(()) } - Self::Doctor => doctor::run(&settings).await, - Self::DefaultConfig => { default_config::run(); Ok(()) @@ -175,7 +174,7 @@ impl Cmd { #[cfg(feature = "daemon")] Self::Daemon => daemon::run(settings, sqlite_store, db).await, - _ => unimplemented!(), + Self::History(_) | Self::Init(_) | Self::Doctor => unreachable!(), } } } diff --git a/crates/atuin/src/command/client/doctor.rs b/crates/atuin/src/command/client/doctor.rs index fcd3449e..8203de82 100644 --- a/crates/atuin/src/command/client/doctor.rs +++ b/crates/atuin/src/command/client/doctor.rs @@ -4,13 +4,14 @@ use std::{env, path::PathBuf, str::FromStr}; use atuin_client::database::Sqlite; use atuin_client::settings::Settings; use atuin_common::shell::{shell_name, Shell}; +use atuin_common::utils; use colored::Colorize; use eyre::Result; -use serde::{Deserialize, Serialize}; +use serde::Serialize; use sysinfo::{get_current_pid, Disks, System}; -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Serialize)] struct ShellInfo { pub name: String, @@ -196,13 +197,13 @@ impl ShellInfo { } } -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Serialize)] struct DiskInfo { pub name: String, pub filesystem: String, } -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Serialize)] struct SystemInfo { pub os: String, @@ -233,7 +234,7 @@ impl SystemInfo { } } -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Serialize)] struct SyncInfo { /// Whether the main Atuin sync server is in use /// I'm just calling it Atuin Cloud for lack of a better name atm @@ -255,7 +256,43 @@ impl SyncInfo { } } -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug)] +struct SettingPaths { + db: String, + record_store: String, + key: String, + session: String, +} + +impl SettingPaths { + pub fn new(settings: &Settings) -> Self { + Self { + db: settings.db_path.clone(), + record_store: settings.record_store_path.clone(), + key: settings.key_path.clone(), + session: settings.session_path.clone(), + } + } + + pub fn verify(&self) { + let paths = vec![ + ("ATUIN_DB_PATH", &self.db), + ("ATUIN_RECORD_STORE", &self.record_store), + ("ATUIN_KEY", &self.key), + ("ATUIN_SESSION", &self.session), + ]; + + for (path_env_var, path) in paths { + if utils::broken_symlink(path) { + eprintln!( + "{path} (${path_env_var}) is a broken symlink. This may cause issues with Atuin." + ); + } + } + } +} + +#[derive(Debug, Serialize)] struct AtuinInfo { pub version: String, @@ -264,6 +301,9 @@ struct AtuinInfo { pub sync: Option, pub sqlite_version: String, + + #[serde(skip)] // probably unnecessary to expose this + pub setting_paths: SettingPaths, } impl AtuinInfo { @@ -289,11 +329,12 @@ impl AtuinInfo { version: crate::VERSION.to_string(), sync, sqlite_version, + setting_paths: SettingPaths::new(settings), } } } -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Serialize)] struct DoctorDump { pub atuin: AtuinInfo, pub shell: ShellInfo, @@ -322,6 +363,8 @@ fn checks(info: &DoctorDump) { println!("{zfs_error}"); } + info.atuin.setting_paths.verify(); + // Shell if info.shell.name == "bash" { if !info diff --git a/crates/atuin/src/command/client/init.rs b/crates/atuin/src/command/client/init.rs index 8238a69b..7f8b2d32 100644 --- a/crates/atuin/src/command/client/init.rs +++ b/crates/atuin/src/command/client/init.rs @@ -159,6 +159,11 @@ $env.config = ( } pub async fn run(self, settings: &Settings) -> Result<()> { + if !settings.paths_ok() { + eprintln!("Atuin settings paths are broken. Disabling atuin shell hooks. Run `atuin doctor` to diagnose."); + return Ok(()); + } + if settings.dotfiles.enabled { self.dotfiles_init(settings).await?; } else { diff --git a/crates/atuin/src/command/client/search.rs b/crates/atuin/src/command/client/search.rs index b6504bd9..4c24620b 100644 --- a/crates/atuin/src/command/client/search.rs +++ b/crates/atuin/src/command/client/search.rs @@ -160,17 +160,17 @@ impl Cmd { // when running the equivalent search, but deleting those entries that are // displayed with the search would leave any duplicates of those lines which may // or may not have been intended to be deleted. - println!("\"--limit\" is not compatible with deletion."); + eprintln!("\"--limit\" is not compatible with deletion."); return Ok(()); } if self.delete && query.is_empty() { - println!("Please specify a query to match the items you wish to delete. If you wish to delete all history, pass --delete-it-all"); + eprintln!("Please specify a query to match the items you wish to delete. If you wish to delete all history, pass --delete-it-all"); return Ok(()); } if self.delete_it_all && !query.is_empty() { - println!( + eprintln!( "--delete-it-all will delete ALL of your history! It does not require a query." ); return Ok(()); -- cgit v1.3.1