From 4e737667324cbf05166f2ab182b179a994f1d45f Mon Sep 17 00:00:00 2001 From: Yoshihiro OKUMURA Date: Mon, 20 Apr 2026 16:51:26 +0900 Subject: [PATCH] perf(cache): reuse auth state in memory Cache parsed auth state per remote and validate it with on-disk file metadata so repeated authenticated API calls can skip redundant open/read/JSON parse work within one process. Centralize cache load, persist, and removal helpers in the cache module, reuse them from login, logout, and whoami, and update the refresh path to persist structured cache data directly. Add targeted cache tests for memory reuse, invalidation after external writes, persist updates, and cache removal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cache/mod.rs | 385 ++++++++++++++++++++++++++++++++++------- src/cache/types.rs | 14 +- src/commands/login.rs | 81 ++------- src/commands/logout.rs | 9 +- src/commands/whoami.rs | 24 +-- 5 files changed, 353 insertions(+), 160 deletions(-) diff --git a/src/cache/mod.rs b/src/cache/mod.rs index f4d7b32..55d34fe 100644 --- a/src/cache/mod.rs +++ b/src/cache/mod.rs @@ -2,13 +2,17 @@ pub mod digest; pub mod types; pub use digest::compute_digest; -pub use types::{Cache, CacheLaboratory, CacheLabsWrapper, CacheUser}; +pub use types::{Cache, CacheLaboratory, CacheLabsWrapper, CacheToken, CacheUser}; use crate::connection::MDRSConnection; use anyhow::{anyhow, bail}; use std::collections::HashMap; use std::fs; +#[cfg(unix)] +use std::os::unix::fs::{MetadataExt, PermissionsExt}; +use std::path::{Path, PathBuf}; use std::sync::{Arc, LazyLock, Mutex}; +use std::time::UNIX_EPOCH; // --------------------------------------------------------------------------- // Per-remote async mutex map (in-process serialization) @@ -17,6 +21,29 @@ use std::sync::{Arc, LazyLock, Mutex}; static REMOTE_LOCKS: LazyLock>>>> = LazyLock::new(|| Mutex::new(HashMap::new())); +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +struct CacheStoreKey { + config_dir: PathBuf, + remote: String, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +struct CacheFileSnapshot { + len: u64, + modified_nanos: u128, + #[cfg(unix)] + inode: u64, +} + +#[derive(Clone)] +struct MemoryCacheEntry { + snapshot: CacheFileSnapshot, + cache: Cache, +} + +static MEMORY_CACHE: LazyLock>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + fn get_remote_lock(remote: &str) -> Arc> { let mut map = REMOTE_LOCKS.lock().unwrap(); map.entry(remote.to_string()) @@ -28,11 +55,161 @@ fn get_remote_lock(remote: &str) -> Arc> { // Cache file path helpers // --------------------------------------------------------------------------- -fn cache_file_path(remote: &str) -> std::path::PathBuf { - crate::settings::SETTINGS - .config_dirname - .join("cache") - .join(format!("{}.json", remote)) +fn cache_store_key(config_dir: &Path, remote: &str) -> CacheStoreKey { + CacheStoreKey { + config_dir: config_dir.to_path_buf(), + remote: remote.to_string(), + } +} + +fn cache_dir_path(config_dir: &Path) -> PathBuf { + config_dir.join("cache") +} + +fn cache_file_path_in(config_dir: &Path, remote: &str) -> PathBuf { + cache_dir_path(config_dir).join(format!("{}.json", remote)) +} + +fn cache_file_path(remote: &str) -> PathBuf { + cache_file_path_in(&crate::settings::SETTINGS.config_dirname, remote) +} + +fn cache_snapshot(metadata: &fs::Metadata) -> CacheFileSnapshot { + let modified_nanos = metadata + .modified() + .ok() + .and_then(|time| time.duration_since(UNIX_EPOCH).ok()) + .map(|duration| duration.as_nanos()) + .unwrap_or_default(); + + CacheFileSnapshot { + len: metadata.len(), + modified_nanos, + #[cfg(unix)] + inode: metadata.ino(), + } +} + +fn read_cache_snapshot(cache_path: &Path) -> Result { + fs::metadata(cache_path).map(|metadata| cache_snapshot(&metadata)) +} + +fn cached_entry(config_dir: &Path, remote: &str, snapshot: &CacheFileSnapshot) -> Option { + let key = cache_store_key(config_dir, remote); + let map = MEMORY_CACHE.lock().unwrap(); + map.get(&key) + .filter(|entry| entry.snapshot == *snapshot) + .map(|entry| entry.cache.clone()) +} + +fn update_cached_entry(config_dir: &Path, remote: &str, snapshot: CacheFileSnapshot, cache: Cache) { + let key = cache_store_key(config_dir, remote); + let mut map = MEMORY_CACHE.lock().unwrap(); + map.insert(key, MemoryCacheEntry { snapshot, cache }); +} + +fn invalidate_cached_entry(config_dir: &Path, remote: &str) { + let key = cache_store_key(config_dir, remote); + let mut map = MEMORY_CACHE.lock().unwrap(); + map.remove(&key); +} + +fn ensure_cache_dir(cache_dir: &Path) -> Result<(), anyhow::Error> { + fs::create_dir_all(cache_dir)?; + #[cfg(unix)] + { + let mut perms = fs::metadata(cache_dir)?.permissions(); + perms.set_mode(0o700); + fs::set_permissions(cache_dir, perms)?; + } + Ok(()) +} + +fn write_cache_file(cache_path: &Path, cache: &Cache) -> Result<(), anyhow::Error> { + let tmp_path = cache_path.with_extension("tmp"); + fs::write(&tmp_path, serde_json::to_vec_pretty(cache)?)?; + #[cfg(unix)] + { + let mut perms = fs::metadata(&tmp_path)?.permissions(); + perms.set_mode(0o600); + fs::set_permissions(&tmp_path, perms)?; + } + fs::rename(&tmp_path, cache_path)?; + Ok(()) +} + +fn parse_cache(remote: &str, data: &str) -> Result { + serde_json::from_str::(data).map_err(|e| { + anyhow!( + "Cache for `{}` is invalid or outdated ({}). Run `mdrs login {}` to refresh it.", + remote, + e, + remote + ) + }) +} + +fn load_cache_from_dir(remote: &str, config_dir: &Path) -> Result { + let cache_path = cache_file_path_in(config_dir, remote); + let snapshot = match read_cache_snapshot(&cache_path) { + Ok(snapshot) => snapshot, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + invalidate_cached_entry(config_dir, remote); + bail!( + "Not logged in to `{}`. Run `mdrs login {}` first.", + remote, + remote + ); + } + Err(e) => return Err(e.into()), + }; + + if let Some(cache) = cached_entry(config_dir, remote, &snapshot) { + return Ok(cache); + } + + let data = match fs::read_to_string(&cache_path) { + Ok(data) => data, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + invalidate_cached_entry(config_dir, remote); + bail!( + "Not logged in to `{}`. Run `mdrs login {}` first.", + remote, + remote + ); + } + Err(e) => return Err(e.into()), + }; + let cache = parse_cache(remote, &data)?; + update_cached_entry(config_dir, remote, snapshot, cache.clone()); + Ok(cache) +} + +fn persist_cache_in_dir( + remote: &str, + config_dir: &Path, + cache: &Cache, +) -> Result<(), anyhow::Error> { + let cache_dir = cache_dir_path(config_dir); + ensure_cache_dir(&cache_dir)?; + + let cache_path = cache_file_path_in(config_dir, remote); + write_cache_file(&cache_path, cache)?; + + let snapshot = read_cache_snapshot(&cache_path)?; + update_cached_entry(config_dir, remote, snapshot, cache.clone()); + Ok(()) +} + +fn remove_cache_in_dir(remote: &str, config_dir: &Path) -> Result<(), anyhow::Error> { + let cache_path = cache_file_path_in(config_dir, remote); + match fs::remove_file(&cache_path) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(e.into()), + } + invalidate_cached_entry(config_dir, remote); + Ok(()) } // --------------------------------------------------------------------------- @@ -41,23 +218,17 @@ fn cache_file_path(remote: &str) -> std::path::PathBuf { /// Load token and laboratories from the login cache file (no token refresh check). pub fn load_cache(remote: &str) -> Result { - let cache_path = cache_file_path(remote); - if !cache_path.exists() { - bail!( - "Not logged in to `{}`. Run `mdrs login {}` first.", - remote, - remote - ); - } - let data = fs::read_to_string(&cache_path)?; - serde_json::from_str::(&data).map_err(|e| { - anyhow!( - "Cache for `{}` is invalid or outdated ({}). Run `mdrs login {}` to refresh it.", - remote, - e, - remote - ) - }) + load_cache_from_dir(remote, &crate::settings::SETTINGS.config_dirname) +} + +/// Persist a cache entry and refresh the in-memory fast path. +pub fn persist_cache(remote: &str, cache: &Cache) -> Result<(), anyhow::Error> { + persist_cache_in_dir(remote, &crate::settings::SETTINGS.config_dirname, cache) +} + +/// Remove a cache entry from disk and memory. +pub fn remove_cache(remote: &str) -> Result<(), anyhow::Error> { + remove_cache_in_dir(remote, &crate::settings::SETTINGS.config_dirname) } // --------------------------------------------------------------------------- @@ -98,8 +269,7 @@ pub async fn load_cache_with_token_refresh(remote: &str) -> Result Result Result { +async fn refresh_and_persist(remote: &str, cache: &Cache) -> Result { let url = crate::commands::config::get_remote_url(remote)? .ok_or_else(|| anyhow!("Remote `{}` is not configured.", remote))?; let conn = MDRSConnection::new(&url); let new_access = conn.token_refresh(&cache.token.refresh).await?; - let new_digest = compute_digest( - cache.user.as_ref(), - &new_access, - &cache.token.refresh, - &cache.laboratories, + let mut updated_cache = cache.clone(); + updated_cache.token.access = new_access; + updated_cache.digest = compute_digest( + updated_cache.user.as_ref(), + &updated_cache.token.access, + &updated_cache.token.refresh, + &updated_cache.laboratories, ); - let cache_path = cache_file_path(remote); - let raw = fs::read_to_string(&cache_path)?; - let mut obj: serde_json::Value = serde_json::from_str(&raw)?; + persist_cache(remote, &updated_cache)?; - obj["token"]["access"] = serde_json::Value::String(new_access.clone()); - obj["digest"] = serde_json::Value::String(new_digest); - - // Write atomically: write to .tmp then rename. - let tmp_path = cache_path.with_extension("tmp"); - { - use std::io::Write; - let mut tmp_file = fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .open(&tmp_path)?; - tmp_file.write_all(serde_json::to_string(&obj)?.as_bytes())?; - tmp_file.flush()?; - } - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let mut perms = fs::metadata(&tmp_path)?.permissions(); - perms.set_mode(0o600); - fs::set_permissions(&tmp_path, perms)?; - } - fs::rename(&tmp_path, &cache_path)?; - - Ok(new_access) + Ok(updated_cache) } // --------------------------------------------------------------------------- @@ -172,3 +318,124 @@ pub fn create_authenticated_conn( .with_remote(remote) .with_token(cache.token.access.clone())) } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + fn sample_cache(username: &str) -> Cache { + Cache { + user: Some(CacheUser { + id: 1, + username: username.to_string(), + laboratory_ids: vec![10, 20], + is_reviewer: false, + }), + token: types::CacheToken { + access: format!("access-{username}"), + refresh: format!("refresh-{username}"), + }, + laboratories: CacheLabsWrapper { + items: vec![CacheLaboratory { + id: 10, + name: "lab".to_string(), + pi_name: String::new(), + full_name: "Laboratory".to_string(), + }], + }, + digest: format!("digest-{username}"), + } + } + + fn remote_name(prefix: &str, config_dir: &Path) -> String { + format!( + "{prefix}-{}", + config_dir + .file_name() + .unwrap_or_default() + .to_string_lossy() + .replace('.', "_") + ) + } + + #[cfg(unix)] + #[test] + fn load_cache_uses_memory_fast_path_when_snapshot_matches() { + let dir = tempdir().unwrap(); + let remote = remote_name("fast-path", dir.path()); + let cache = sample_cache("alice"); + + persist_cache_in_dir(&remote, dir.path(), &cache).unwrap(); + let first = load_cache_from_dir(&remote, dir.path()).unwrap(); + assert_eq!(first.user.unwrap().username, "alice"); + + let cache_path = cache_file_path_in(dir.path(), &remote); + let mut perms = fs::metadata(&cache_path).unwrap().permissions(); + perms.set_mode(0o000); + fs::set_permissions(&cache_path, perms).unwrap(); + + let second = load_cache_from_dir(&remote, dir.path()).unwrap(); + assert_eq!(second.user.unwrap().username, "alice"); + } + + #[test] + fn load_cache_reloads_when_external_writer_changes_file() { + let dir = tempdir().unwrap(); + let remote = remote_name("reload", dir.path()); + let original = sample_cache("alice"); + let updated = sample_cache("bob"); + + persist_cache_in_dir(&remote, dir.path(), &original).unwrap(); + let first = load_cache_from_dir(&remote, dir.path()).unwrap(); + assert_eq!(first.user.unwrap().username, "alice"); + + let cache_dir = cache_dir_path(dir.path()); + ensure_cache_dir(&cache_dir).unwrap(); + let cache_path = cache_file_path_in(dir.path(), &remote); + write_cache_file(&cache_path, &updated).unwrap(); + + let second = load_cache_from_dir(&remote, dir.path()).unwrap(); + assert_eq!(second.user.unwrap().username, "bob"); + } + + #[cfg(unix)] + #[test] + fn persist_cache_refreshes_memory_entry() { + let dir = tempdir().unwrap(); + let remote = remote_name("persist", dir.path()); + let original = sample_cache("alice"); + let updated = sample_cache("bob"); + + persist_cache_in_dir(&remote, dir.path(), &original).unwrap(); + let _ = load_cache_from_dir(&remote, dir.path()).unwrap(); + + persist_cache_in_dir(&remote, dir.path(), &updated).unwrap(); + + let cache_path = cache_file_path_in(dir.path(), &remote); + let mut perms = fs::metadata(&cache_path).unwrap().permissions(); + perms.set_mode(0o000); + fs::set_permissions(&cache_path, perms).unwrap(); + + let loaded = load_cache_from_dir(&remote, dir.path()).unwrap(); + assert_eq!(loaded.user.unwrap().username, "bob"); + } + + #[test] + fn remove_cache_invalidates_memory_entry() { + let dir = tempdir().unwrap(); + let remote = remote_name("remove", dir.path()); + let cache = sample_cache("alice"); + + persist_cache_in_dir(&remote, dir.path(), &cache).unwrap(); + let _ = load_cache_from_dir(&remote, dir.path()).unwrap(); + + remove_cache_in_dir(&remote, dir.path()).unwrap(); + + let err = load_cache_from_dir(&remote, dir.path()).unwrap_err(); + assert!( + err.to_string() + .contains(&format!("Not logged in to `{remote}`")) + ); + } +} diff --git a/src/cache/types.rs b/src/cache/types.rs index d72f640..0990cf4 100644 --- a/src/cache/types.rs +++ b/src/cache/types.rs @@ -1,14 +1,14 @@ -use serde::Deserialize; +use serde::{Deserialize, Serialize}; /// Access and refresh token pair stored in the login cache file. -#[derive(Deserialize, Clone)] +#[derive(Deserialize, Serialize, Clone, Debug, PartialEq, Eq)] pub struct CacheToken { pub access: String, pub refresh: String, } /// Minimal user fields stored in the cache, matching Python's `User` dataclass. -#[derive(Deserialize, Clone)] +#[derive(Deserialize, Serialize, Clone, Debug, PartialEq, Eq)] pub struct CacheUser { pub id: u32, pub username: String, @@ -17,7 +17,7 @@ pub struct CacheUser { } /// All four laboratory fields needed for digest computation. -#[derive(Deserialize, Clone)] +#[derive(Deserialize, Serialize, Clone, Debug, PartialEq, Eq)] pub struct CacheLaboratory { pub id: u32, pub name: String, @@ -28,15 +28,17 @@ pub struct CacheLaboratory { } /// Wrapper matching Python's `Laboratories` serialization: `{"items": [...]}`. -#[derive(Deserialize, Clone, Default)] +#[derive(Deserialize, Serialize, Clone, Debug, Default, PartialEq, Eq)] pub struct CacheLabsWrapper { pub items: Vec, } /// Full login cache, corresponding to the `.json` file written by `login`. -#[derive(Deserialize, Clone)] +#[derive(Deserialize, Serialize, Clone, Debug, PartialEq, Eq)] pub struct Cache { pub user: Option, pub token: CacheToken, pub laboratories: CacheLabsWrapper, + #[serde(default)] + pub digest: String, } diff --git a/src/commands/login.rs b/src/commands/login.rs index 0d73944..ad002a5 100644 --- a/src/commands/login.rs +++ b/src/commands/login.rs @@ -1,14 +1,10 @@ -use crate::cache::{CacheLabsWrapper, CacheUser, compute_digest}; +use crate::cache::{Cache, CacheLabsWrapper, CacheToken, CacheUser, compute_digest, persist_cache}; use crate::connection::MDRSConnection; use crate::models::laboratory::Laboratories; use crate::models::user::User; use anyhow::{anyhow, bail}; use reqwest::Client; use serde::Deserialize; -use serde_json::{Value, json}; -use std::fs; -#[cfg(unix)] -use std::os::unix::fs::PermissionsExt; /// Prompt for credentials if not supplied and then perform login. /// This is the entry point called from `main`. @@ -72,66 +68,23 @@ pub async fn login(username: &str, password: &str, remote: &str) -> Result<(), a let cache_labs: CacheLabsWrapper = (&labs).into(); // compute Python-compatible digest - let digest = compute_digest( - cache_user_opt.as_ref(), - &token.access, - &token.refresh, - &cache_labs, - ); - - // build the cache JSON — field order matches Python's dataclass layout: - // user (id, username, laboratory_ids, is_reviewer) - // token (access, refresh) - // laboratories (items) - // digest - let user_val: Value = match &cache_user_opt { - Some(u) => json!({ - "id": u.id, - "username": u.username, - "laboratory_ids": u.laboratory_ids, - "is_reviewer": u.is_reviewer - }), - None => Value::Null, + let cache = Cache { + user: cache_user_opt, + token: CacheToken { + access: token.access, + refresh: token.refresh, + }, + laboratories: cache_labs, + digest: String::new(), }; - let labs_items: Vec = cache_labs - .items - .iter() - .map(|l| { - json!({ - "id": l.id, - "name": l.name, - "pi_name": l.pi_name, - "full_name": l.full_name - }) - }) - .collect(); - let obj = json!({ - "user": user_val, - "token": {"access": token.access, "refresh": token.refresh}, - "laboratories": {"items": labs_items}, - "digest": digest - }); - - // write cache file: {config_dirname}/cache/.json - let cache_dir = crate::settings::SETTINGS.config_dirname.join("cache"); - fs::create_dir_all(&cache_dir)?; - #[cfg(unix)] - { - let mut perms = fs::metadata(&cache_dir)?.permissions(); - perms.set_mode(0o700); - fs::set_permissions(&cache_dir, perms)?; - } - let cache_file = cache_dir.join(format!("{}.json", remote)); - let tmp = cache_file.with_extension("tmp"); - - fs::write(&tmp, serde_json::to_vec_pretty(&obj)?)?; - #[cfg(unix)] - { - let mut perms = fs::metadata(&tmp)?.permissions(); - perms.set_mode(0o600); - fs::set_permissions(&tmp, perms)?; - } - fs::rename(&tmp, &cache_file)?; + let mut cache = cache; + cache.digest = compute_digest( + cache.user.as_ref(), + &cache.token.access, + &cache.token.refresh, + &cache.laboratories, + ); + persist_cache(remote, &cache)?; println!("Login Successful"); Ok(()) diff --git a/src/commands/logout.rs b/src/commands/logout.rs index 3e4b63a..93a6648 100644 --- a/src/commands/logout.rs +++ b/src/commands/logout.rs @@ -1,10 +1,3 @@ pub fn logout(remote: &str) -> Result<(), anyhow::Error> { - let cache_path = crate::settings::SETTINGS - .config_dirname - .join("cache") - .join(format!("{}.json", remote)); - if cache_path.exists() { - std::fs::remove_file(&cache_path)?; - } - Ok(()) + crate::cache::remove_cache(remote) } diff --git a/src/commands/whoami.rs b/src/commands/whoami.rs index 91278c9..5d0be21 100644 --- a/src/commands/whoami.rs +++ b/src/commands/whoami.rs @@ -1,27 +1,5 @@ -use serde::Deserialize; -use std::fs; - -#[derive(Deserialize)] -struct CacheUser { - username: String, -} - -#[derive(Deserialize)] -struct WhoamiCache { - user: Option, -} - pub async fn whoami(remote: &str) -> Result<(), anyhow::Error> { - let cache_path = crate::settings::SETTINGS - .config_dirname - .join("cache") - .join(format!("{}.json", remote)); - if !cache_path.exists() { - println!("(Anonymous)"); - return Ok(()); - } - let data = fs::read_to_string(&cache_path)?; - match serde_json::from_str::(&data) { + match crate::cache::load_cache(remote) { Ok(cache) => match cache.user { Some(user) => println!("{}", user.username), None => println!("(Anonymous)"),