diff --git a/src/cmd/auth.rs b/src/cmd/auth.rs index 011d73d..bd3c172 100644 --- a/src/cmd/auth.rs +++ b/src/cmd/auth.rs @@ -858,23 +858,23 @@ async fn status(json: bool) -> Result<()> { // the DID documents rather than trusted from cache, and the cache is // repaired on the way past. A DID doc lookup needs no token, so this // still works for an account whose session has run out. + // + // What is printed comes from the DID document either way, so the repair + // is a side errand and `refresh_cached_handle` treats it as one: it + // skips a registry another atgc holds the lock on, skips one it could + // not read rather than overwriting it, and skips the write entirely + // while the cache already agrees. This is the command somebody runs + // *because* something about their accounts is wrong, and the loop is + // once per account, so on a machine full of agents it was the likeliest + // command in the tool to lose that race — first fatally, then loudly. + // The warning went with the fatality: contention is the ordinary case + // here, and a per-account warning about a cache nobody reads is noise + // over a condition the reader cannot act on. It goes to the debug log. let mut rows = Vec::new(); for account in &known { let handle = match handle_from_did_doc(&account.did).await { Some(handle) => { - // Best-effort, and `status` is the command that most has to - // survive it failing: repairing the cache is a side errand, - // and this is what somebody runs *because* something about - // their accounts is wrong. A registry another atgc holds the - // lock on, or one this refuses to overwrite because it could - // not be read, must not take the answer down with it. - if let Err(e) = crate::config::account::remember(&account.did, Some(&handle)).await - { - crate::term::say::warning!( - Config, - "could not record {handle}'s handle in the registry: {e}" - ); - } + crate::config::account::refresh_cached_handle(&account.did, &handle).await; Some(handle) } None => account.handle.clone(), diff --git a/src/config/account/mod.rs b/src/config/account/mod.rs index 20cec1f..f9bd416 100644 --- a/src/config/account/mod.rs +++ b/src/config/account/mod.rs @@ -53,7 +53,8 @@ pub(crate) mod registry; pub(crate) mod selection; pub use registry::{ - cached_handle, client_id, forget, known, remember, remember_client_id, set_active, + cached_handle, client_id, forget, known, refresh_cached_handle, remember, remember_client_id, + set_active, }; pub use selection::{ Selection, Standing, actor_did, advise, display_account, init, lookup, owner, repo_did, select, diff --git a/src/config/account/registry.rs b/src/config/account/registry.rs index 3df5b25..a3f0886 100644 --- a/src/config/account/registry.rs +++ b/src/config/account/registry.rs @@ -180,20 +180,38 @@ fn save(_guard: &crate::config::lock::Guard, registry: &Registry) -> Result<()> /// other. async fn update(change: impl FnOnce(&mut Registry)) -> Result<()> { let guard = crate::config::lock::take_async(crate::logging::oauth::Purpose::Registry).await?; + edit(&guard, change) +} + +/// The read-change-write half of [`update`], once the lock is held. +/// +/// Split out so that [`refresh_cached_handle`], which takes the lock in a way +/// [`update`] does not, cannot reach the file without going past the refusal +/// below. It reached `load` and `save` directly for one revision of this work, +/// which is precisely the bug the refusal exists to stop. +fn edit(guard: &crate::config::lock::Guard, change: impl FnOnce(&mut Registry)) -> Result<()> { + let mut registry = readable()?; + change(&mut registry); + save(guard, ®istry) +} + +/// The registry as a starting point for a rewrite, or a refusal. +/// +/// **A read-modify-write may not start from a file it could not read.** +/// [`load`] answers an unparseable file with an empty registry, which is the +/// right answer to "what do we know" and the wrong one to start a rewrite +/// from: the write would replace every other account's entry with nothing, +/// and one of those fields cannot be rebuilt. `client_id` is the loopback +/// redirect URI of the login that produced the grant, ephemeral port and all, +/// so losing it does not cost a cached handle — it costs every one of those +/// accounts a fresh login, silently, on whichever command happened to record +/// a handle. +/// +/// Refusing leaves the file exactly as it is, which is what makes it +/// recoverable: the bytes are still there to look at, and `auth login` +/// rewrites the entry for one account without this path. +fn readable() -> Result { let reading = read(); - // **A read-modify-write may not start from a file it could not read.** - // [`load`] answers an unparseable file with an empty registry, which is - // the right answer to "what do we know" and the wrong one to start a - // rewrite from: the write would replace every other account's entry with - // nothing, and one of those fields cannot be rebuilt. `client_id` is the - // loopback redirect URI of the login that produced the grant, ephemeral - // port and all, so losing it does not cost a cached handle — it costs - // every one of those accounts a fresh login, silently, on whichever - // command happened to record a handle. - // - // Refusing leaves the file exactly as it is, which is what makes it - // recoverable: the bytes are still there to look at, and `auth login` - // rewrites the entry for one account without this path. if let Some(error) = reading.unreadable { let path = registry_path()?; anyhow::bail!( @@ -206,9 +224,7 @@ async fn update(change: impl FnOnce(&mut Registry)) -> Result<()> { path.display(), ); } - let mut registry = reading.registry; - change(&mut registry); - save(&guard, ®istry) + Ok(reading.registry) } /// An account atgc knows about, with whatever session state goes with it. @@ -246,25 +262,121 @@ pub fn known() -> Result> { .collect()) } -/// Record an account, or update its cached handle. Called after a login and -/// after any successful handle resolution. +/// How long a cached handle is trusted before [`refresh_cached_handle`] +/// bothers to re-stamp it. +/// +/// Twenty-four hours, chosen the way a cache TTL is chosen rather than the +/// way a correctness deadline is. What this field is *for* is labelling an +/// account in a listing or an error, and the offline fallback in +/// [`super::selection`] when the resolver cannot be reached. Every path that +/// acts as an account resolves the handle live first and only then reaches +/// here, so a stamp a few hours behind changes nothing anyone observes; a +/// handle that has actually moved is caught by the comparison above this +/// one, not by the age. +/// +/// What re-stamping costs is the process-global lock over +/// `~/.config/atgc`, on the hot path of every `--account @handle` command. +/// With a wave of agents against one `$HOME` that is one serialized lock +/// acquisition per invocation, for a value nobody needs to be current. A day +/// makes it one write between all of them instead. +const HANDLE_CACHE_TTL: chrono::TimeDelta = chrono::TimeDelta::days(1); + +/// Whether the registry already says what a handle resolution just learned, +/// recently enough that rewriting it would buy nothing. +/// +/// Pure, and takes `now` and the entry rather than reading either, so the +/// decision is testable — the write it guards is not, since it goes through +/// `$HOME` (see [`crate::docs::testing`]). +/// +/// Anything unexpected answers "not current", which spends a lock and is +/// therefore the safe direction to be wrong in: a missing entry, a different +/// handle, a missing or unparseable stamp — and a stamp in the *future*, +/// which means a clock moved and the age computed from it means nothing. +fn cached_handle_is_current( + entry: Option<&Account>, + handle: &str, + now: chrono::DateTime, +) -> bool { + let Some(entry) = entry else { return false }; + if entry.handle.as_deref() != Some(handle) { + return false; + } + let Some(stamp) = entry.handle_checked_at.as_deref() else { + return false; + }; + let Ok(checked) = chrono::DateTime::parse_from_rfc3339(stamp) else { + return false; + }; + let age = now.signed_duration_since(checked.with_timezone(&chrono::Utc)); + age >= chrono::TimeDelta::zero() && age < HANDLE_CACHE_TTL +} + +/// Repair the cached handle for a DID after resolving it, at most once a +/// [`HANDLE_CACHE_TTL`] and never fatally. +/// +/// The two callers — `selection`'s `--account @handle` path and `auth +/// status` — are repairing a cache on the way past doing something else. +/// Both used to call [`remember`] with a `?`, which is two bugs at once: +/// `remember` always re-stamps `handle_checked_at`, so the write was +/// unconditional and every handle-named command queued for the global lock; +/// and a lock lost to an unrelated slow process then failed the command the +/// user actually asked for. Neither the write nor its failure is worth +/// anything to the caller, so this skips the first when the cache already +/// agrees and swallows the second either way. +pub async fn refresh_cached_handle(did: &str, handle: &str) { + if cached_handle_is_current(load().accounts.get(did), handle, chrono::Utc::now()) { + return; + } + let what = format!("the cached handle for {did}"); + let taken = crate::config::lock::take_async(crate::logging::oauth::Purpose::Registry).await; + // `or_skip` rather than a bare `?`: the work must not happen unlocked — + // an unlocked read-modify-write here can drop another process's entry — + // but not happening at all is free. + let Some(guard) = crate::config::lock::or_skip(taken, &format!("updating {what}")) else { + return; + }; + // Through `edit` rather than `load` and `save`, so that a registry that + // is there and does not parse refuses here exactly as it does under + // `update`: a best-effort cache repair is the last thing that should be + // allowed to overwrite a file holding `client_id`s it could not read. + if let Err(e) = edit(&guard, |registry| stamp_handle(registry, did, handle)) { + crate::logging::debug::log(format!("could not update {what}: {e}")); + } +} + +/// Write a resolved handle and the moment it was resolved into an entry, +/// creating the entry if this is the first sighting of the DID. +fn stamp_handle(registry: &mut Registry, did: &str, handle: &str) { + let entry = registry.accounts.entry(did.to_string()).or_default(); + let now = || { + jacquard::types::string::Datetime::now() + .as_str() + .to_string() + }; + if entry.added_at.is_none() { + entry.added_at = Some(now()); + } + entry.handle = Some(handle.to_string()); + entry.handle_checked_at = Some(now()); +} + +/// Record an account, or update its cached handle. Called after a login. +/// +/// Unconditional and fatal, unlike [`refresh_cached_handle`]: a login that +/// cannot record the account it just created has not finished, and the entry +/// it writes is what `remember_client_id` fills in next. pub async fn remember(did: &str, handle: Option<&str>) -> Result<()> { - update(|registry| { - let entry = registry.accounts.entry(did.to_string()).or_default(); - if entry.added_at.is_none() { - entry.added_at = Some( - jacquard::types::string::Datetime::now() - .as_str() - .to_string(), - ); - } - if let Some(handle) = handle { - entry.handle = Some(handle.to_string()); - entry.handle_checked_at = Some( - jacquard::types::string::Datetime::now() - .as_str() - .to_string(), - ); + update(|registry| match handle { + Some(handle) => stamp_handle(registry, did, handle), + None => { + let entry = registry.accounts.entry(did.to_string()).or_default(); + if entry.added_at.is_none() { + entry.added_at = Some( + jacquard::types::string::Datetime::now() + .as_str() + .to_string(), + ); + } } }) .await @@ -315,3 +427,140 @@ pub async fn forget(did: &str) -> Result<()> { }) .await } + +#[cfg(test)] +mod tests { + use super::*; + + fn at(stamp: &str) -> chrono::DateTime { + chrono::DateTime::parse_from_rfc3339(stamp) + .expect("a test stamp parses") + .with_timezone(&chrono::Utc) + } + + fn entry(handle: &str, checked_at: &str) -> Account { + Account { + handle: Some(handle.to_string()), + handle_checked_at: Some(checked_at.to_string()), + added_at: Some(checked_at.to_string()), + client_id: None, + } + } + + /// The defect this guard exists for: a cache that already agrees must + /// not be rewritten, because rewriting it takes the process-global lock + /// on the hot path of every `--account @handle` command. Before the TTL + /// the answer here was always "write" — `remember` re-stamped + /// `handle_checked_at`, so the contents always differed and the write + /// always really happened. + #[test] + fn a_handle_the_registry_already_holds_is_not_rewritten() { + let held = entry("alice.example", "2026-08-26T09:00:00Z"); + assert!(cached_handle_is_current( + Some(&held), + "alice.example", + at("2026-08-26T09:00:01Z") + )); + } + + /// The cache still has to be repaired eventually, or a stamp written + /// once would be trusted forever and `handle_checked_at` would stop + /// meaning what it says. A day is the line; either side of it is + /// asserted so that a TTL edited to zero fails this rather than merely + /// making the tool slower. + #[test] + fn a_stamp_older_than_a_day_is_re_checked_and_a_younger_one_is_not() { + let held = entry("alice.example", "2026-08-25T09:00:00Z"); + assert!( + cached_handle_is_current(Some(&held), "alice.example", at("2026-08-26T08:59:59Z")), + "a stamp not yet a day old was re-written" + ); + assert!( + !cached_handle_is_current(Some(&held), "alice.example", at("2026-08-26T09:00:01Z")), + "a stamp over a day old was trusted" + ); + } + + /// A handle that has actually moved must be written through however + /// fresh the stamp is: the age is about the cache going stale, and this + /// is the cache being wrong, which the comparison catches directly. + #[test] + fn a_renamed_handle_is_written_however_recently_it_was_checked() { + let held = entry("old.example", "2026-08-26T09:00:00Z"); + assert!(!cached_handle_is_current( + Some(&held), + "new.example", + at("2026-08-26T09:00:01Z") + )); + } + + /// Every shape the registry can be in that is not "a handle checked + /// recently" has to answer "write", including the ones that are not + /// errors so much as absences: a DID recorded by `remember_client_id` + /// before any handle was resolved, and an entry from a version that + /// cached a handle without stamping it. + #[test] + fn an_entry_with_nothing_to_compare_against_is_written() { + assert!( + !cached_handle_is_current(None, "alice.example", at("2026-08-26T09:00:00Z")), + "a DID with no entry at all" + ); + + let no_handle = Account { + client_id: Some("http://localhost/".to_string()), + ..Account::default() + }; + assert!( + !cached_handle_is_current(Some(&no_handle), "alice.example", at("2026-08-26T09:00:00Z")), + "an entry that carries only a client_id" + ); + + let unstamped = Account { + handle: Some("alice.example".to_string()), + ..Account::default() + }; + assert!( + !cached_handle_is_current(Some(&unstamped), "alice.example", at("2026-08-26T09:00:00Z")), + "a cached handle with no stamp beside it" + ); + + let unparseable = entry("alice.example", "last tuesday"); + assert!( + !cached_handle_is_current( + Some(&unparseable), + "alice.example", + at("2026-08-26T09:00:00Z") + ), + "a stamp that is not a datetime" + ); + } + + /// A stamp in the future means a clock moved — the machine's own, or + /// that of the process that wrote it — and an age computed from it is + /// meaningless. Treating it as fresh would pin the cache until the + /// future caught up, which for a badly set clock can be years. + #[test] + fn a_stamp_from_the_future_is_not_trusted() { + let held = entry("alice.example", "2026-08-27T09:00:00Z"); + assert!(!cached_handle_is_current( + Some(&held), + "alice.example", + at("2026-08-26T09:00:00Z") + )); + } + + /// The write path and the guard must agree about what "current" means, + /// or every command would write a stamp the next one immediately + /// rejects. Driven through the function `remember` and + /// `refresh_cached_handle` share, against the registry it produced. + #[test] + fn what_the_write_path_stamps_reads_back_as_current() { + let mut registry = Registry::default(); + stamp_handle(&mut registry, "did:plc:example", "alice.example"); + assert!(cached_handle_is_current( + registry.accounts.get("did:plc:example"), + "alice.example", + chrono::Utc::now() + )); + } +} diff --git a/src/config/account/selection.rs b/src/config/account/selection.rs index 5fd317b..0831b46 100644 --- a/src/config/account/selection.rs +++ b/src/config/account/selection.rs @@ -16,7 +16,7 @@ use anyhow::{Context, Result, bail}; use std::sync::OnceLock; -use super::registry::{Known, known, load, remember}; +use super::registry::{Known, known, load, refresh_cached_handle}; /// `--account` as passed on the command line. A process-wide slot, like /// `debug::init`, because the selector applies to every command and @@ -737,7 +737,7 @@ async fn resolve_spec(spec: &str, known: &[Known], source: Source) -> Result