From 1cadb986fc82e1079d9514fb4dee1cdce9c939ee Mon Sep 17 00:00:00 2001 From: YellowSnnowmann <167776381+YellowSnnowmann@users.noreply.github.com> Date: Wed, 20 May 2026 02:46:54 +0530 Subject: [PATCH] fix(credentials): diagnose + recover from H8 auth-profile-lock create failures (#2180) --- src/openhuman/credentials/profiles.rs | 26 +++++++++- src/openhuman/credentials/profiles_tests.rs | 53 +++++++++++++++++++++ src/openhuman/util.rs | 23 ++++++++- 3 files changed, 100 insertions(+), 2 deletions(-) diff --git a/src/openhuman/credentials/profiles.rs b/src/openhuman/credentials/profiles.rs index 2c94b9d1a..75d97b51b 100644 --- a/src/openhuman/credentials/profiles.rs +++ b/src/openhuman/credentials/profiles.rs @@ -593,7 +593,16 @@ impl AuthProfilesStore { thread::sleep(Duration::from_millis(LOCK_WAIT_MS)); waited = waited.saturating_add(LOCK_WAIT_MS); } else { - return Err(e).context("Failed to create auth profile lock"); + // Sentry OPENHUMAN-TAURI-H8 collapses every + // non-AlreadyExists, non-transient `create_new` + // failure into a single fingerprint with no + // breadcrumb of which OS code actually fired. + // `annotate_lock_create_failure` embeds the + // underlying `io::ErrorKind` + `raw_os_error()` so + // future events split by root cause and we can + // widen `is_transient_fs_error` (or fix the + // underlying condition) for whichever code is hot. + return Err(annotate_lock_create_failure(e)); } } } @@ -664,6 +673,21 @@ impl AuthProfilesStore { /// means we keep waiting on a lock that was actually already gone, which /// is the safe direction. Backed by sysinfo so we don't grow a new libc / /// windows-sys dependency for one syscall. +/// Wrap a non-`AlreadyExists` `create_new` failure with a context line that +/// embeds the underlying `io::ErrorKind` and `raw_os_error()`. Pulled out +/// of [`AuthProfilesStore::acquire_lock`] so unit tests can drive the +/// formatting directly without depending on filesystem permissions (CI runs +/// as root and bypasses `chmod 0500`). +fn annotate_lock_create_failure(err: anyhow::Error) -> anyhow::Error { + let io = err.chain().find_map(|c| c.downcast_ref::()); + let kind = io.map(|ioe| ioe.kind()); + let os_code = io.and_then(|ioe| ioe.raw_os_error()); + err.context(format!( + "Failed to create auth profile lock (kind={:?}, os_code={:?})", + kind, os_code + )) +} + fn is_pid_alive(pid: u32) -> bool { use sysinfo::{Pid, ProcessRefreshKind, ProcessesToUpdate, System}; let target = Pid::from_u32(pid); diff --git a/src/openhuman/credentials/profiles_tests.rs b/src/openhuman/credentials/profiles_tests.rs index a17688542..68056e9f2 100644 --- a/src/openhuman/credentials/profiles_tests.rs +++ b/src/openhuman/credentials/profiles_tests.rs @@ -442,6 +442,59 @@ fn acquire_lock_writes_pid_so_future_callers_can_recover() { assert!(!lock_path.exists(), "guard must remove lock on drop"); } +/// Sentry OPENHUMAN-TAURI-H8: when `OpenOptions::create_new` fails with +/// anything other than `AlreadyExists`, the error surfaced to Sentry +/// must embed the underlying `io::ErrorKind` and `raw_os_error()` so we +/// can tell which OS code is firing. Drive the wrapping helper directly +/// with a synthetic `io::Error` so the test is platform-independent and +/// doesn't depend on filesystem permissions (CI runs as root and bypasses +/// `chmod`). +#[test] +fn annotate_lock_create_failure_embeds_io_kind_and_os_code() { + // Use each platform's native permission-denied code so the test exercises + // the OS error that real production failures would carry. Rust does map + // `from_raw_os_error(13)` to `PermissionDenied` on Windows too, but real + // Windows `create_new` failures surface code 5 (ERROR_ACCESS_DENIED), and + // running against the native code catches regressions in + // `annotate_lock_create_failure`'s handling of the platform-specific + // value. + #[cfg(windows)] + let raw_code = 5; // ERROR_ACCESS_DENIED + #[cfg(not(windows))] + let raw_code = 13; // EACCES + + let io_err = std::io::Error::from_raw_os_error(raw_code); + let wrapped = annotate_lock_create_failure(anyhow::Error::new(io_err)); + let msg = format!("{wrapped:?}"); + + assert!( + msg.contains("Failed to create auth profile lock"), + "stable top-level message missing: {msg}" + ); + assert!( + msg.contains("kind=Some(PermissionDenied)"), + "context must include io::ErrorKind for Sentry diagnosis: {msg}" + ); + assert!( + msg.contains(&format!("os_code=Some({raw_code})")), + "context must include raw OS code for Sentry diagnosis: {msg}" + ); +} + +/// If somehow the chained error is not an `io::Error`, the wrapper must +/// still emit the stable top-level message with explicit `None` markers so +/// the Sentry fingerprint still splits cleanly (and we know to look +/// upstream for an io::Error that got dropped). +#[test] +fn annotate_lock_create_failure_handles_missing_io_error() { + let wrapped = annotate_lock_create_failure(anyhow::anyhow!("synthetic")); + let msg = format!("{wrapped:?}"); + + assert!(msg.contains("Failed to create auth profile lock"), "{msg}"); + assert!(msg.contains("kind=None"), "{msg}"); + assert!(msg.contains("os_code=None"), "{msg}"); +} + #[test] fn auth_profile_kind_serde_roundtrip() { let json = serde_json::to_string(&AuthProfileKind::OAuth).unwrap(); diff --git a/src/openhuman/util.rs b/src/openhuman/util.rs index b179a1419..b6886b53e 100644 --- a/src/openhuman/util.rs +++ b/src/openhuman/util.rs @@ -440,6 +440,17 @@ mod tests { ); } + #[cfg(windows)] + #[test] + fn is_transient_fs_error_classifies_windows_delete_pending() { + let io_err = std::io::Error::from_raw_os_error(303); + let err = anyhow::Error::new(io_err); + assert!( + is_transient_fs_error(&err), + "ERROR_DELETE_PENDING (303) must be transient on Windows" + ); + } + /// A chained io::Error with `ErrorKind::NotFound` is not a transient /// locking error — we should not retry it. #[test] @@ -617,8 +628,18 @@ pub fn is_transient_fs_error(err: &anyhow::Error) -> bool { // 5: ERROR_ACCESS_DENIED // 32: ERROR_SHARING_VIOLATION // 33: ERROR_LOCK_VIOLATION + // 303: ERROR_DELETE_PENDING — the previous owner's + // `Drop::drop` issued `fs::remove_file` and Windows + // acknowledged it, but the file is still in the + // "delete pending" limbo because AV/indexer holds a + // handle. A retry-with-backoff resolves it as soon as + // the holder closes its handle. Sentry OPENHUMAN-TAURI-H8 + // bails at `elapsed_ms ≈ 2` against + // `openhuman.team_get_usage` because this code was not + // previously classified as transient and `create_new` + // returned a `kind = Other` io::Error on the first try. // 1224: ERROR_USER_MAPPED_FILE - return code == 5 || code == 32 || code == 33 || code == 1224; + return code == 5 || code == 32 || code == 33 || code == 303 || code == 1224; } } #[cfg(not(windows))]