From 5fe76e46a3ca6e443dc2e6b293820d531f69a687 Mon Sep 17 00:00:00 2001 From: Taylor Eernisse Date: Mon, 26 Jan 2026 22:46:08 -0500 Subject: [PATCH] fix(core): Add structured error handling and responsive lock release Improves core infrastructure with robot-friendly error output and faster lock release for better sync behavior. Error handling improvements (error.rs): - ErrorCode::exit_code(): Unique exit codes per error type (1-13) for programmatic error handling in scripts/agents - GiError::suggestion(): Helpful hints for common error recovery - GiError::to_robot_error(): Structured JSON error conversion - RobotError/RobotErrorOutput: Serializable error types with code, message, and optional suggestion fields Lock improvements (lock.rs): - Heartbeat thread now polls every 100ms for release flag, only updating database heartbeat at full interval (5s default) - Eliminates 5-10s delay after sync completion when waiting for heartbeat thread to notice release - Reduces lock hold time after operation completes Database (db.rs): - Bump expected schema version to 6 for MR migration The exit code mapping enables shell scripts and CI/CD pipelines to distinguish between configuration errors (2-4), GitLab API errors (5-8), and database errors (9-11) for appropriate retry/alert logic. Co-Authored-By: Claude Opus 4.5 --- src/core/db.rs | 14 +++++++-- src/core/error.rs | 79 +++++++++++++++++++++++++++++++++++++++++++++++ src/core/lock.rs | 25 ++++++++++----- 3 files changed, 109 insertions(+), 9 deletions(-) diff --git a/src/core/db.rs b/src/core/db.rs index 876853a..7b8ca51 100644 --- a/src/core/db.rs +++ b/src/core/db.rs @@ -15,8 +15,18 @@ const MIGRATIONS: &[(&str, &str)] = &[ ("001", include_str!("../../migrations/001_initial.sql")), ("002", include_str!("../../migrations/002_issues.sql")), ("003", include_str!("../../migrations/003_indexes.sql")), - ("004", include_str!("../../migrations/004_discussions_payload.sql")), - ("005", include_str!("../../migrations/005_assignees_milestone_duedate.sql")), + ( + "004", + include_str!("../../migrations/004_discussions_payload.sql"), + ), + ( + "005", + include_str!("../../migrations/005_assignees_milestone_duedate.sql"), + ), + ( + "006", + include_str!("../../migrations/006_merge_requests.sql"), + ), ]; /// Create a database connection with production-grade pragmas. diff --git a/src/core/error.rs b/src/core/error.rs index 41ffa36..6133523 100644 --- a/src/core/error.rs +++ b/src/core/error.rs @@ -2,6 +2,7 @@ //! //! Uses thiserror for ergonomic error definitions with structured error codes. +use serde::Serialize; use thiserror::Error; /// Error codes for programmatic error handling. @@ -43,6 +44,27 @@ impl std::fmt::Display for ErrorCode { } } +impl ErrorCode { + /// Get the exit code for this error (for robot mode). + pub fn exit_code(&self) -> i32 { + match self { + Self::InternalError => 1, + Self::ConfigNotFound => 2, + Self::ConfigInvalid => 3, + Self::TokenNotSet => 4, + Self::GitLabAuthFailed => 5, + Self::GitLabNotFound => 6, + Self::GitLabRateLimited => 7, + Self::GitLabNetworkError => 8, + Self::DatabaseLocked => 9, + Self::DatabaseError => 10, + Self::MigrationFailed => 11, + Self::IoError => 12, + Self::TransformError => 13, + } + } +} + /// Main error type for gitlab-inbox. #[derive(Error, Debug)] pub enum GiError { @@ -132,6 +154,63 @@ impl GiError { Self::Other(_) => ErrorCode::InternalError, } } + + /// Get a suggestion for how to fix this error. + pub fn suggestion(&self) -> Option<&'static str> { + match self { + Self::ConfigNotFound { .. } => Some("Run 'gi init' to create configuration"), + Self::ConfigInvalid { .. } => Some("Check config file syntax or run 'gi init' to recreate"), + Self::GitLabAuthFailed => Some("Verify token has read_api scope and is not expired"), + Self::GitLabNotFound { .. } => Some("Check the resource path exists and you have access"), + Self::GitLabRateLimited { .. } => Some("Wait and retry, or reduce request frequency"), + Self::GitLabNetworkError { .. } => Some("Check network connection and GitLab URL"), + Self::DatabaseLocked { .. } => Some("Wait for other sync to complete or use --force"), + Self::MigrationFailed { .. } => Some("Check database file permissions or reset with 'gi reset'"), + Self::TokenNotSet { .. } => Some("Export the token environment variable"), + Self::Database(_) => Some("Check database file permissions or reset with 'gi reset'"), + Self::Http(_) => Some("Check network connection"), + Self::NotFound(_) => Some("Verify the entity exists using 'gi list'"), + Self::Ambiguous(_) => Some("Use --project flag to disambiguate"), + _ => None, + } + } + + /// Get the exit code for this error. + pub fn exit_code(&self) -> i32 { + self.code().exit_code() + } + + /// Convert to robot-mode JSON error output. + pub fn to_robot_error(&self) -> RobotError { + RobotError { + code: self.code().to_string(), + message: self.to_string(), + suggestion: self.suggestion().map(String::from), + } + } +} + +/// Structured error for robot mode JSON output. +#[derive(Debug, Serialize)] +pub struct RobotError { + pub code: String, + pub message: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub suggestion: Option, +} + +/// Wrapper for robot mode error output. +#[derive(Debug, Serialize)] +pub struct RobotErrorOutput { + pub error: RobotError, +} + +impl From<&GiError> for RobotErrorOutput { + fn from(e: &GiError) -> Self { + Self { + error: e.to_robot_error(), + } + } } pub type Result = std::result::Result; diff --git a/src/core/lock.rs b/src/core/lock.rs index c310275..25d81e3 100644 --- a/src/core/lock.rs +++ b/src/core/lock.rs @@ -42,10 +42,7 @@ pub struct AppLock { impl AppLock { /// Create a new app lock instance. pub fn new(conn: Connection, options: LockOptions) -> Self { - let db_path = conn - .path() - .map(PathBuf::from) - .unwrap_or_default(); + let db_path = conn.path().map(PathBuf::from).unwrap_or_default(); Self { conn, @@ -73,7 +70,9 @@ impl AppLock { let now = now_ms(); // Use IMMEDIATE transaction to prevent race conditions - let tx = self.conn.transaction_with_behavior(TransactionBehavior::Immediate)?; + let tx = self + .conn + .transaction_with_behavior(TransactionBehavior::Immediate)?; // Check for existing lock within the transaction let existing: Option<(String, i64, i64)> = tx @@ -176,9 +175,21 @@ impl AppLock { } }; - loop { - thread::sleep(interval); + // Poll frequently for early exit, but only update heartbeat at full interval + const POLL_INTERVAL: Duration = Duration::from_millis(100); + loop { + // Sleep in small increments, checking released flag frequently + let mut elapsed = Duration::ZERO; + while elapsed < interval { + thread::sleep(POLL_INTERVAL); + elapsed += POLL_INTERVAL; + if released.load(Ordering::SeqCst) { + return; + } + } + + // Check once more after full interval elapsed if released.load(Ordering::SeqCst) { break; }