perf(auth): cache account pool store scans#163
Conversation
Avoid repeated credential-file reads while the account store directory remains unchanged, while preserving mtime-based no-restart discovery. Refs #151
There was a problem hiding this comment.
Code Review
This pull request introduces an in-memory caching mechanism for account directory scans in src/auth/shared.rs using the directory's modification time (mtime) to optimize steady-state request performance by avoiding redundant file I/O. It includes the implementation of scan_cached, a process-wide cache, and corresponding unit tests. The review feedback suggests refactoring scan_cached to be asynchronous and wrapping the synchronous directory scanning in tokio::task::spawn_blocking to prevent blocking the async runtime worker threads.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review of the mtime-keyed account-pool scan cache surfaced a latent cross-provider hazard and several inaccurate comments: - Key the shared cache by (provider, dir), not dir alone, so two stores pointed at the same directory (SHUNT_CLAUDE_ACCOUNTS_DIR == SHUNT_CODEX_ACCOUNTS_DIR) can never serve one provider's scan (e.g. Codex's UUID-less entries) to the other. - Scope the zero-read claim to account-list discovery (a full request still reads the selected account file), reword the stale per-request scan line, and frame directory mtime as a best-effort invalidation signal (coarse-resolution filesystems can defer a re-scan one tick). - Soften the concurrent-miss comment: an entry is only served while its stored mtime matches, so a stale write is re-scanned away next request. - Strengthen the invalidation test to prove the refreshed account set replaces the cached one (not just that it re-scans) and to assert a second provider keeps its own entry at the same path. Refs #151
…tion test Second review pass on the cache comments: - Note that provider_label now also partitions the scan cache, not just the error text. - Correct the coarse-mtime caveat: a change sharing the cached scan's timestamp goes unnoticed until a later change advances the mtime (not merely "within one clock tick"). - State the concurrent-miss guarantee precisely: snapshots may differ and the last write wins, but an entry is served only while its stored mtime equals the mtime the request sampled, so a stale write is re-scanned away once a request observes a different mtime. - Strengthen the test with a second same-provider call proving the entry is retained (cache hit), not just isolated from the other provider. Refs #151
Greptile SummaryThis PR adds a process-wide mtime-keyed cache for account-pool store scans in
Confidence Score: 5/5Safe to merge; the change is a pure performance optimization with no observable behavior change. The caching logic is correct: mtime is sampled before the scan, the lock is dropped during I/O to avoid blocking concurrent hits, and the no-mtime bypass ensures correctness on every platform. Concurrency trade-offs are well-documented in code comments. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant R as Pooled Request
participant B as spawn_blocking
participant FS as Filesystem
participant C as scan_cache (Mutex)
R->>B: resolve_pool_accounts(provider, dir, scan)
B->>FS: fs::metadata(dir)
alt dir absent (NotFound)
FS-->>B: Err(NotFound)
B-->>R: Ok([])
else stat succeeds
FS-->>B: Ok(meta) → mtime
B->>C: lock → get((provider, dir))
alt "cache hit (stored mtime == current mtime)"
C-->>B: accounts.clone()
B-->>R: Ok(cached accounts)
else cache miss
C-->>B: None or stale mtime
Note over B: drop lock before I/O
B->>FS: scan() → read_dir + per-file reads
FS-->>B: accounts
B->>C: "lock → insert((provider, dir), {mtime, accounts})"
B-->>R: Ok(fresh accounts)
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant R as Pooled Request
participant B as spawn_blocking
participant FS as Filesystem
participant C as scan_cache (Mutex)
R->>B: resolve_pool_accounts(provider, dir, scan)
B->>FS: fs::metadata(dir)
alt dir absent (NotFound)
FS-->>B: Err(NotFound)
B-->>R: Ok([])
else stat succeeds
FS-->>B: Ok(meta) → mtime
B->>C: lock → get((provider, dir))
alt "cache hit (stored mtime == current mtime)"
C-->>B: accounts.clone()
B-->>R: Ok(cached accounts)
else cache miss
C-->>B: None or stale mtime
Note over B: drop lock before I/O
B->>FS: scan() → read_dir + per-file reads
FS-->>B: accounts
B->>C: "lock → insert((provider, dir), {mtime, accounts})"
B-->>R: Ok(fresh accounts)
end
end
Reviews (2): Last reviewed commit: "docs(auth): reflow account scan cache co..." | Re-trigger Greptile |
|



Summary
Cache account-pool store scans by the store directory's modification time. Previously, every pooled request traversed
resolve_pool_accounts→scan_accounts→read_account_uuid, performing one directory read plus one credential-file read and JSON parse per account. The shared process-wideauth::shared::scan_cachedhelper now serves the previous scan when the directory mtime is unchanged, reducing steady-state discovery to one metadata stat and zero credential-file reads.The existing metadata lookup from #118 supplies the cache signal, so this adds no extra stat. Directory changes still invalidate the cache for no-restart account discovery, scans remain on
spawn_blocking, absent stores retain the cheapNotFoundshort-circuit, and platforms without an mtime bypass the cache. Claude and Codex use separate store paths and can both benefit, while the primary improvement is avoiding Claude UUID reads.Closes #151
Part of #149 (Tier 1).
Milestone / spec
Performance follow-up tracked by #149 (Tier 1) and #151. This internal refactor does not change observable behavior, configuration, endpoints, CLI behavior, or provider/model semantics, so no
README.md,docs/, orsite/update is needed.Checklist
cargo buildpassescargo testpasses (new behavior is covered; tests run without network/loopback where possible)cargo clippy --all-targets -- -D warningscleancargo fmt --all --checkcleandocs/updated if this change deviates from itREADME.md/site/as applicable (wiki/is generated; don't hand-edit)Notes for reviewers
scan_cached_serves_cache_until_mtime_changes_and_bypasses_without_mtimeinjects mtimes directly to verify cache hits, invalidation, and no-mtime bypass behavior.resolve_pool_accounts_caches_scan_across_unchanged_requestsverifies against the real filesystem that an unchanged second request does not re-scan.cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings, andcargo test --all-features --workspace(529 library tests plus all integration tests, includingmulti_account).auth::sharedclosely.Summary by cubic
Caches pooled-account store scans by store-directory mtime to skip repeat credential-file reads during discovery; biggest win is avoiding Claude UUID reads. The cache is keyed by provider and directory; unchanged stores now cost one stat and zero credential-file reads, with behavior unchanged.
(provider, dir)and invalidated by the store’s mtime.spawn_blocking.Written for commit 299f489. Summary will update on new commits.