diff --git a/plan/credential-store.md b/plan/credential-store.md index 6dd4626..33f7938 100644 --- a/plan/credential-store.md +++ b/plan/credential-store.md @@ -77,6 +77,63 @@ re-proposed: the OS keyring, and encrypting the DPoP key at rest. ## Done +- [x] **The lock's two ends were unrelated numbers.** `WAIT` was 30 seconds + and its comment claimed a caller reaching it "is not queued behind + honest work" — false in both halves. The critical section covered + *two* uncached network round trips, the + `/.well-known/oauth-authorization-server` fetch and the token POST, and + the client bounds around them do not sum to a total: 5s connect, a + *per-read* 30s stall allowance, and `MAX_RETRY_AFTER` permitting 30s of + backoff on top. So one slow authorization server failed every other + atgc on the machine with `Exit::Conflict` while it waited on entirely + honest work, and the lock being directory-global meant N processes + refreshing N *different* accounts serialized N full refreshes. Worth + fixing now because of how atgc is actually run: many agent processes + spawning dynamically on one shared, often ephemeral machine, where many + concurrent invocations sharing one `$HOME` is the normal case rather + than two terminals colliding. Now `HOLD_BUDGET` (25s) caps the refresh + section and is *enforced* — a `tokio::time::timeout` around `restore`, + which drops the guard along with the future, so it is a bound on the + lock and not merely on the command — and `WAIT` is derived from it + (`QUEUE_DEPTH` whole holds plus `SLACK`, 60s) so the two cannot cross; + a `const` assertion fails the build if they ever do. A timeout is + `Unreachable` rather than `Conflict`, because nothing is contended and + the answer is to retry unchanged +- [x] The metadata round trip is gone from inside the lock. + `SessionRegistry::get_refreshed` builds an `OAuthMetadata` before it can + POST, and building one fetched the well-known document afresh every + time — the same bytes for every account on a PDS, on the order of never + changing, fetched under a global lock. `oauth::metadata::CachingMetadata` + is one more `HttpClient` wrapper, outermost so a hit is not written to + either transport log as if it were a request, caching only 200-answered + `GET`s of that one path for 15 minutes in + `~/.config/atgc/authserver-metadata.json` (0600, `write_atomic`). On + atgc's side rather than in the vendored crate on purpose: the three + local patches there are bugs to report upstream and drop, and a cache is + a policy decision about this machine. The TTL is short and the + document's own `Cache-Control` is deliberately *not* read — servers + advertise hours or days, and honouring one would let a rotated + `token_endpoint` break refreshes here for that long with an unknown + cache file as the cause. The file is the one place an unlocked + read-modify-write is right: losing an update to it costs one HTTP GET, + and locking it would put a third thing inside the section this exists to + shorten +- [x] `try_lock` in a poll loop is not a queue: no ordering, no fairness, only + whoever calls at the moment the lock is free. Processes that start + polling together stayed phase-aligned and kept colliding, and the same + one could keep losing — a starvation tail bounded by nothing but `WAIT`. + Each sleep is now spread +/-40% around the 25ms interval, drawn from a + per-process xorshift seeded from `RandomState` so two invocations + starting together do not draw the same sequence. The arithmetic is a + pure function and is tested on its decision rather than on a stopwatch +- [x] Whether `Exit::Conflict` on contention is right for an agent fleet was + reconsidered and deliberately left alone, with the reasoning now in the + module docs rather than only here. A bounded retry with backoff in the + caller would be indistinguishable from a longer `WAIT` — the wait + already *is* a retry loop — while hiding how long a command really took + and stacking more pollers onto a lock with no queue. The bound belongs + in one place + - [x] The session store's own read-modify-write was not under the lock — only its write was. `SessionStore::put` and `remove` read the whole map, compared, and *then* called a `write` that took the lock, which is one diff --git a/plan/http-bounds.md b/plan/http-bounds.md index 9f3ee51..7752709 100644 --- a/plan/http-bounds.md +++ b/plan/http-bounds.md @@ -42,6 +42,21 @@ jacquard's transport is buffered whole before atgc sees it. ## Done +- [x] The three bounds here are each a bound on a *part* of an exchange, and + nothing in the tree turned them into a bound on a whole one: 5s connect + plus a per-read 30s stall allowance plus up to 30s of `MAX_RETRY_AFTER` + backoff is not a total, and no number here says how long one request may + take end to end. That is the right shape for this module — a total + timeout is a hidden ceiling on payload size, which is why `READ_TIMEOUT` + is per-read — but it left the one caller that needs a total without one. + A token refresh holds the process-global config lock across the whole + exchange (`plan/credential-store.md`), so its length is what every other + atgc on the machine waits out. `config::lock::HOLD_BUDGET` supplies that + total, at the caller and not here: a `tokio::time::timeout` around the + refresh, sized under the waiters' own deadline. Recorded in both files + because the number is only correct as long as both halves are read + together + - [x] One HTTP client constructor (src/clients/http.rs) with a 5s connect timeout and a 30s read timeout, behind every request atgc makes. `reqwest`'s defaults have no deadline at any layer, so a host that drops packets diff --git a/src/config/lock.rs b/src/config/lock.rs index 7f1e0bd..2fa713f 100644 --- a/src/config/lock.rs +++ b/src/config/lock.rs @@ -696,7 +696,7 @@ mod tests { let releaser = tokio::spawn(async move { for _ in 0..3 { counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - tokio::time::sleep(POLL).await; + tokio::time::sleep(poll_delay()).await; } held.unlock().unwrap(); }); @@ -710,6 +710,7 @@ mod tests { "the waiter blocked its thread: nothing else on this runtime ran" ); drop(guard); + } /// **The defect this change exists for**, as an assertion on the two /// numbers rather than on a stopwatch: a waiter must outlast the longest