diff --git a/.claude/agent-memory/review-review-comment-analyzer/MEMORY.md b/.claude/agent-memory/review-review-comment-analyzer/MEMORY.md index 73e1e6f1..dc0b5940 100644 --- a/.claude/agent-memory/review-review-comment-analyzer/MEMORY.md +++ b/.claude/agent-memory/review-review-comment-analyzer/MEMORY.md @@ -3,3 +3,4 @@ - [shunt otel privacy-claim rot](shunt-otel-privacy-claim-rot.md) — `include_session_id` in `src/config.rs`/`src/telemetry.rs` was documented but never wired into `src/proxy.rs`; always grep a privacy/gating config field's name across the whole `src/` tree before trusting doc-comment claims about it. - [shunt "verbatim" convention](shunt-verbatim-terminology-convention.md) — shunt uses "verbatim" strictly for byte-identical passthrough; PR #114 had one loose use of it for a re-shaped error envelope. - [responses adapter stream/JSON doc generalization](shunt-responses-adapter-stream-json-doc-generalization.md) — RESOLVED by PR #120 (issue #113): JSON paths now surface backend-sent error events as 502s via `AnthropicSseMachine::backend_error()`; recurring rot hotspot, recheck stream-vs-JSON parity language whenever these paths are touched again. +- [shunt account scan-cache comment rot](shunt-account-scan-cache-comment-rot.md) — Recheck lexical path collisions, discovery-only I/O claims, concurrent misses, and mtime invalidation language. diff --git a/.claude/agent-memory/review-review-comment-analyzer/shunt-account-scan-cache-comment-rot.md b/.claude/agent-memory/review-review-comment-analyzer/shunt-account-scan-cache-comment-rot.md new file mode 100644 index 00000000..956fa0b4 --- /dev/null +++ b/.claude/agent-memory/review-review-comment-analyzer/shunt-account-scan-cache-comment-rot.md @@ -0,0 +1,12 @@ +--- +name: shunt-account-scan-cache-comment-rot +description: Comment-rot checks for the mtime-keyed Claude/Codex account-store scan cache. +metadata: + type: project +--- + +The account-store scan cache is a documentation hotspot: distinguish a per-request directory metadata check from an actual `read_dir` scan, and scope “zero credential-file reads” to discovery because selected-account authentication still reads its credential file. + +**Why:** The cache is keyed by `(provider label, lexical store directory path)`, so Claude and Codex remain separate even when overrides point to the same path. Comments saying `provider_label` affects error text only are therefore stale. Concurrent cache misses can also observe different snapshots if a store mutation overlaps their scans, so equal pre-scan mtimes do not guarantee identical results. + +**How to apply:** Whenever this cache or account-store write paths change, re-check comments about per-request scans, total I/O, provider-label use, cross-store key separation, concurrent misses, and categorical mtime invalidation. Internal Claude writes use same-directory temp-file creation plus rename and removals use `remove_file`, but equal observed mtimes remain possible on coarse filesystems and a directory can change after a request samples its metadata. diff --git a/.claude/agent-memory/review-review-pr-test-analyzer/MEMORY.md b/.claude/agent-memory/review-review-pr-test-analyzer/MEMORY.md index 664a1040..21bdff68 100644 --- a/.claude/agent-memory/review-review-pr-test-analyzer/MEMORY.md +++ b/.claude/agent-memory/review-review-pr-test-analyzer/MEMORY.md @@ -8,3 +8,4 @@ - [shunt inbound auth multi-slot coverage](shunt-inbound-auth-multislot-coverage.md) — PR #133 tests/inbound_auth.rs gap analysis; gate-token now accepted via header/Bearer/x-api-key; priority/attribution test drops the NoHeader leak assertions the single-slot test uses — recurring "priority test forgets the leak assertion" pattern. - [shunt pool burn-rate coverage](shunt-pool-burn-rate-coverage.md) — PR #136 src/accounts.rs+config.rs gap analysis; unusually thorough unit tests, but headroom ordering among still-available (not just near-quota) accounts is untested, plus all-accounts-disabled and multi-window aggregation gaps. - [shunt Codex admin coverage](shunt-codex-admin-coverage.md) — PR #144 Codex admin OAuth happy path/storage/list/pool/delete are strong; gaps remain for state/account-id rejection, Codex-route CSRF wiring, and pending-kind isolation. +- [shunt account scan cache coverage](shunt-account-scan-cache-coverage.md) — PR #163 covers hit/invalidation triggers without race risk, but never asserts changed scan output replaces the cached account list. diff --git a/.claude/agent-memory/review-review-pr-test-analyzer/shunt-account-scan-cache-coverage.md b/.claude/agent-memory/review-review-pr-test-analyzer/shunt-account-scan-cache-coverage.md new file mode 100644 index 00000000..3a200ce4 --- /dev/null +++ b/.claude/agent-memory/review-review-pr-test-analyzer/shunt-account-scan-cache-coverage.md @@ -0,0 +1,12 @@ +--- +name: shunt-account-scan-cache-coverage +description: PR #163 mtime-keyed account-scan cache coverage; hit/invalidation triggers covered, but changed scan output is never asserted. +metadata: + type: project +--- + +PR #163's deterministic `scan_cached` test covers first miss, same-mtime hit, changed-mtime re-scan, and no-mtime bypass; its real-filesystem resolver test covers unchanged-request reuse. Unique pid+nanos paths isolate the process-wide cache, and the sole static counting callback is used by only one awaited test, so there is no concrete parallel-test race. + +**Why:** Both scans return the same `one_account()` value, so call-count assertions prove re-scanning but cannot catch a regression that invokes the scanner after invalidation while continuing to return/cache the old account list. That would break the hard no-restart account-discovery requirement. + +**How to apply:** For cache invalidation tests, make the scan result change across mtimes and assert both the immediate refreshed result and the subsequent hit. Do not require a timing-sensitive real-filesystem mtime test when the injected-mtime seam plus resolver hit test already covers composition. diff --git a/src/auth/shared.rs b/src/auth/shared.rs index 25e5ac93..7f75b91e 100644 --- a/src/auth/shared.rs +++ b/src/auth/shared.rs @@ -7,11 +7,12 @@ //! into a sibling provider's module. use std::{ + collections::HashMap, env, fs, io, path::{Path, PathBuf}, sync::{ atomic::{AtomicUsize, Ordering}, - OnceLock, + Mutex, OnceLock, }, time::{Duration, SystemTime, UNIX_EPOCH}, }; @@ -275,11 +276,12 @@ pub fn scan_account_dir( } /// Resolve a pooled provider's effective account list: its configured -/// `[[accounts]]` when non-empty, otherwise a per-request scan of the store -/// directory (which enables no-restart account discovery, mirroring the -/// Anthropic pool). The existence probe and the scan both do synchronous -/// filesystem I/O, so they run together on the blocking pool — never a stat or -/// `read_dir` on a runtime worker thread. When `accounts_dir` is genuinely absent +/// `[[accounts]]` when non-empty, otherwise a scan of the store directory +/// (cached by its mtime — see [`scan_cached`]) that enables no-restart account +/// discovery, mirroring the Anthropic pool. The existence probe and the scan +/// both do synchronous filesystem I/O, so they run together on the blocking +/// pool — never a stat or `read_dir` on a runtime worker thread. When +/// `accounts_dir` is genuinely absent /// (a `NotFound` stat) — the backward-compat deployment that sets /// `auth = "..._oauth"` but never runs `shunt login` — the scan is short-circuited /// right after that cheap stat (no `read_dir`, no per-file reads), preserving the @@ -288,9 +290,18 @@ pub fn scan_account_dir( /// masked as "no accounts", mirroring `scan_account_dir`'s own `NotFound`-only /// handling and preserving the pre-#118 guarantee that a broken store is /// diagnosable. The check runs on every request, so once an account is added -/// (which creates the directory) scanning resumes with no restart. -/// `provider_label` shapes the error text only ("codex" / "Claude"); the error is -/// returned preformatted so each pool wraps it in its own gateway error type. +/// (which creates the directory) scanning resumes with no restart. The +/// `read_dir` + per-account UUID reads are cached by the store directory's mtime +/// (see [`scan_cached`]): an unchanged store re-serves the last scan, so +/// steady-state account-list discovery costs one stat and zero credential-file +/// reads, while adding or removing an account changes the directory mtime and so +/// re-scans on the next request — preserving no-restart discovery. (Directory +/// mtime is the invalidation signal, so on a filesystem with coarse mtime +/// resolution a change that shares the cached scan's timestamp goes unnoticed +/// until a later change advances the mtime.) +/// `provider_label` shapes the error text ("codex" / "Claude") and partitions the +/// scan cache (see [`scan_cache`]); the error is returned preformatted so each +/// pool wraps it in its own gateway error type. pub(crate) async fn resolve_pool_accounts( provider_label: &str, configured: &[AccountConfig], @@ -305,13 +316,18 @@ pub(crate) async fn resolve_pool_accounts( // short-circuits on genuine absence (a cheap stat, no `read_dir`); any other // stat error is surfaced, not masked as "no accounts". let scan_dir = accounts_dir.clone(); + let provider = provider_label.to_string(); let scanned = tokio::task::spawn_blocking(move || { - match fs::metadata(&scan_dir) { - Ok(_) => {} + // One stat serves two purposes: the #118 `NotFound` short-circuit (no + // `read_dir` when the store is genuinely absent) and the cache signal + // (the store directory's mtime). Reusing it keeps steady-state discovery + // at one stat and zero credential-file reads. + let modified = match fs::metadata(&scan_dir) { + Ok(meta) => meta.modified().ok(), Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(Vec::new()), Err(error) => return Err(error), - } - scan() + }; + scan_cached(&provider, &scan_dir, modified, scan) }) .await .map_err(|error| format!("{provider_label} account store scan task failed: {error}"))?; @@ -323,6 +339,71 @@ pub(crate) async fn resolve_pool_accounts( }) } +/// One cached scan result: the accounts a scan produced and the store directory +/// mtime it was produced against. A later request whose stat reports the same +/// mtime reuses `accounts` verbatim. +struct CachedScan { + modified: SystemTime, + accounts: Vec, +} + +/// Process-wide cache of [`scan_account_dir`] results, keyed by +/// `(provider, store directory path)`. Both stores share the map but never read +/// each other's entries: the provider label is part of the key, so even two +/// stores pointed at the same directory (their UUID semantics differ) stay +/// separate. The cache primarily spares the Claude store its per-account UUID +/// reads — a cache hit does zero credential-file reads. +fn scan_cache() -> &'static Mutex> { + static CACHE: OnceLock>> = OnceLock::new(); + CACHE.get_or_init(|| Mutex::new(HashMap::new())) +} + +/// Return the cached scan for `dir` when its stored mtime still matches +/// `modified`; otherwise run `scan`, cache the result against `modified`, and +/// return it. `modified` is the store directory's mtime, sampled *before* the +/// scan so a change racing the scan is caught by the next request rather than +/// masked. When `modified` is `None` (a platform/filesystem that reports no +/// mtime) the cache is bypassed and every call scans, so correctness never +/// depends on a signal we could not read. +fn scan_cached( + provider: &str, + dir: &Path, + modified: Option, + scan: impl Fn() -> io::Result>, +) -> io::Result> { + let Some(modified) = modified else { + return scan(); + }; + let key = (provider.to_string(), dir.to_path_buf()); + { + let cache = scan_cache().lock().expect("scan cache mutex poisoned"); + if let Some(entry) = cache.get(&key) { + if entry.modified == modified { + return Ok(entry.accounts.clone()); + } + } + } + // Cache miss (first sight, or the store changed): scan without holding the + // lock so concurrent hits are never blocked behind this file I/O, then + // record the result. Concurrent misses each scan and race to insert; the + // last write wins, and their snapshots may differ if the store changed while + // they overlapped. That is safe: an entry is served only while its stored + // mtime equals the mtime this request sampled, so a stale write is re-scanned + // away as soon as a request observes a different directory mtime. + let accounts = scan()?; + scan_cache() + .lock() + .expect("scan cache mutex poisoned") + .insert( + key, + CachedScan { + modified, + accounts: accounts.clone(), + }, + ); + Ok(accounts) +} + /// Write an account file born-private: create its parent directory `0700` on Unix /// (no chmod-after-create window on a multi-user host), then atomically write /// `value` via [`write_auth_file_atomic`]. Both stores import credentials this way. @@ -539,7 +620,7 @@ mod tests { #[tokio::test] async fn resolve_pool_accounts_scans_when_store_dir_exists() { // Once the store directory exists (an operator added an account), the - // scan runs on every request — no-restart discovery is preserved. + // first request scans and caches it — no-restart discovery is preserved. let dir = temp_dir("pool-present"); fs::create_dir_all(&dir).unwrap(); let accounts = resolve_pool_accounts("codex", &[], dir.clone(), one_account) @@ -585,6 +666,124 @@ mod tests { let _ = fs::remove_dir_all(dir); } + fn account_names(accounts: &[AccountConfig]) -> Vec<&str> { + accounts + .iter() + .map(|account| account.name.as_str()) + .collect() + } + + #[test] + fn scan_cached_serves_cache_until_mtime_changes_and_bypasses_without_mtime() { + // Drive the cache with explicit mtimes (no reliance on filesystem mtime + // resolution), checking both how often the underlying scan runs and which + // account set is served. The directory path is unique per test, so the + // process-wide cache map cannot collide with a sibling test. + let dir = temp_dir("scan-cached"); + let calls = AtomicUsize::new(0); + // The scan's result grows with the store: the first mtime yields one + // account, a later mtime yields two — so an invalidation that re-scans + // but keeps the stale list is distinguishable from one that refreshes it. + let scan = || { + let mut accounts = vec![AccountConfig { + name: "primary".to_string(), + ..Default::default() + }]; + if calls.fetch_add(1, Ordering::Relaxed) >= 1 { + accounts.push(AccountConfig { + name: "secondary".to_string(), + ..Default::default() + }); + } + Ok(accounts) + }; + let t1 = UNIX_EPOCH + Duration::from_secs(1_000); + + // First sight: cache miss, scans once, returns the one-account set. + assert_eq!( + account_names(&scan_cached("codex", &dir, Some(t1), scan).unwrap()), + ["primary"] + ); + assert_eq!(calls.load(Ordering::Relaxed), 1); + + // Unchanged mtime: cache hit — no scan, same set (0 credential-file reads). + assert_eq!( + account_names(&scan_cached("codex", &dir, Some(t1), scan).unwrap()), + ["primary"] + ); + assert_eq!(calls.load(Ordering::Relaxed), 1); + + // Store changed (mtime advanced): invalidate, re-scan, and the REFRESHED + // set replaces the stale one — proving invalidation updates the cached + // value, not merely that it re-scans. + let t2 = t1 + Duration::from_secs(1); + assert_eq!( + account_names(&scan_cached("codex", &dir, Some(t2), scan).unwrap()), + ["primary", "secondary"] + ); + assert_eq!(calls.load(Ordering::Relaxed), 2); + + // The refreshed set is now what a hit serves (still no further scan). + assert_eq!( + account_names(&scan_cached("codex", &dir, Some(t2), scan).unwrap()), + ["primary", "secondary"] + ); + assert_eq!(calls.load(Ordering::Relaxed), 2); + + // A different provider at the same path scans its own entry rather than + // borrowing Codex's — the shared map never serves one provider's scan to + // another. + assert_eq!( + account_names(&scan_cached("Claude", &dir, Some(t2), scan).unwrap()), + ["primary", "secondary"] + ); + assert_eq!(calls.load(Ordering::Relaxed), 3); + + // A second Claude call at the same mtime hits Claude's own entry: no + // re-scan, so the count holds — proving the entry is retained, not shared. + assert_eq!( + account_names(&scan_cached("Claude", &dir, Some(t2), scan).unwrap()), + ["primary", "secondary"] + ); + assert_eq!(calls.load(Ordering::Relaxed), 3); + + // No mtime signal: the cache is bypassed and every call scans. + scan_cached("codex", &dir, None, scan).unwrap(); + scan_cached("codex", &dir, None, scan).unwrap(); + assert_eq!(calls.load(Ordering::Relaxed), 5); + } + + static POOL_CACHE_SCAN_CALLS: AtomicUsize = AtomicUsize::new(0); + + fn counting_scan() -> io::Result> { + POOL_CACHE_SCAN_CALLS.fetch_add(1, Ordering::Relaxed); + one_account() + } + + #[tokio::test] + async fn resolve_pool_accounts_caches_scan_across_unchanged_requests() { + // With an unchanged store, a second pooled request re-serves the cached + // scan: the underlying scan runs once, so steady-state discovery does no + // repeat `read_dir` or per-account reads. + let dir = temp_dir("pool-cache-hit"); + fs::create_dir_all(&dir).unwrap(); + POOL_CACHE_SCAN_CALLS.store(0, Ordering::Relaxed); + + let first = resolve_pool_accounts("codex", &[], dir.clone(), counting_scan) + .await + .unwrap(); + assert_eq!(first.len(), 1); + assert_eq!(POOL_CACHE_SCAN_CALLS.load(Ordering::Relaxed), 1); + + let second = resolve_pool_accounts("codex", &[], dir.clone(), counting_scan) + .await + .unwrap(); + assert_eq!(second.len(), 1); + assert_eq!(POOL_CACHE_SCAN_CALLS.load(Ordering::Relaxed), 1); + + let _ = fs::remove_dir_all(dir); + } + #[test] fn write_account_file_creates_born_private_and_round_trips() { let dir = temp_dir("write");