From 1162dd432b54c450b451ce9df627c38bbc2cccfc Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 10 Nov 2026 20:26:57 -0500 Subject: [PATCH 01/25] Record machine-local GitHub Quota waits shared with the Inbox (#1995) --- GLOSSARY.md | 3 + src/github.rs | 24 +++-- src/github/advisories.rs | 2 +- src/github/execution_tests.rs | 127 +++++++++++++++++++++++++- src/github/quota.rs | 165 +++++++++++++++++++++++++++------- src/github/quota/identity.rs | 128 ++++++++++++++++++++++++++ src/github/quota/store.rs | 163 +++++++++++++++++++++++++++++++++ src/github/quota/tests.rs | 133 +++++++++++++++++++++++++++ src/github/quota/wait.rs | 40 +++++++++ src/logs/github.rs | 49 +++++----- tests/fakes/gh.rs | 2 +- tests/status.rs | 14 +-- 12 files changed, 778 insertions(+), 72 deletions(-) create mode 100644 src/github/quota/identity.rs create mode 100644 src/github/quota/store.rs create mode 100644 src/github/quota/tests.rs create mode 100644 src/github/quota/wait.rs diff --git a/GLOSSARY.md b/GLOSSARY.md index f4993167b..f7788f99a 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -107,6 +107,9 @@ _Avoid_: invocation, job A **Command** that stopped at a plan-limit warning or rejection, with outcome `paused`: an **Experiment** or **Scoring run**. Its **Command record** keeps the stopping Harness and window, utilization and reset time when reported, and the command that resumes it. It waits in **Needs you** as Paused until a later Command resumes the same work or the Day shift makes a **Dismissal**. Resuming continues the same work with finished Trials and Scores kept and nothing finished repeated. This is separate from interruption, is neither success nor failure, and does not resume automatically. _Avoid_: failed Command (for a plan-limit pause) +**Quota wait**: +The time until GitHub's quota for a GitHub user resets, during which **Passes** are skipped and running **Commands** wait instead of failing. + **Ticket run**: One invocation of the factory on a single issue that is not a **Spec**, from launch to cleanup, normally ending in one pull request ready for review, or a **Self-merge** when its command or the **User config** asks for one. A Ticket run on an issue labelled `bug` reproduces the bug before fixing it, in a fresh Ticket run or a **Continuation**, except for **Flaky-test Tickets**, Security fixes and **Base fixes**, which have their own flow. A Continuation whose **Issue branch** has commits beyond the **Base branch** reproduces at their merge base, then checks the fix on the branch; a fix already there still goes on to review and the pull request. If the bug is not reproduced, including when it is absent at that merge base, the Ticket run ends successfully with triage notes and `needs-info` on the issue, without opening or merging a pull request. Started directly on an **Issue URL**, by a **Spec run** for one of its **Tickets**, by an **Architect run** for the Ticket its **Architecture review** published, by a **Pickup run** for the **Ready issue** it took, or by a **Security run** for a **Security finding** it fixes. Every Ticket run carries out a **Process**: the **Built-in process** by default, or a **Custom process** selected by the Process that starts it. That Process owns its sequence of Steps. Dispatch chooses Spec or Ticket and makes any Claim before work starts; child Ticket runs inherit their Command's choices and limits. _Avoid_: Run, Merge run (former names), auto-merge (GitHub's own feature) diff --git a/src/github.rs b/src/github.rs index b39fa8f0b..77c502772 100644 --- a/src/github.rs +++ b/src/github.rs @@ -20,9 +20,9 @@ pub const GENERATED: &str = "This comment was generated by AI."; mod check_read; pub(crate) mod evals; -mod quota; +pub(crate) mod quota; pub(crate) use check_read::is_transient as check_read_is_transient; -pub(crate) use quota::{Quota, Quotas}; +pub(crate) use quota::{Quota, QuotaWait}; mod advisories; mod base_ci; @@ -775,15 +775,12 @@ impl GitHub { } /// What GitHub last showed for one batch of up to a hundred issues and pull requests. - /// The Inbox checks the returned quota before asking for another batch. Each + /// The Inbox checks the shared quota before asking for another batch. Each /// alias asks for state and settlement times (plus issue triage labels), /// and for the query's own `rateLimit`. A subject GitHub can't /// resolve answers null and stays out of the map, without failing the /// rest of the read. Only reads; nothing here writes to GitHub. - pub(crate) fn inbox_states( - &self, - urls: &[String], - ) -> Result<(BTreeMap, Quota)> { + pub(crate) fn inbox_states(&self, urls: &[String]) -> Result> { let mut states = BTreeMap::new(); let mut query = String::from("query {"); let mut aliases = Vec::new(); @@ -841,6 +838,7 @@ impl GitHub { })?; let data = &json["data"]; let quota = Quota::graphql(&data["rateLimit"])?; + self.observe_quota(quota::Resource::Graphql, quota)?; for (alias, url, subject) in aliases { if json["errors"] .as_array() @@ -855,7 +853,7 @@ impl GitHub { } states.insert(url.clone(), subject.read(&url, pull)?); } - Ok((states, quota)) + Ok(states) } /// Open a draft pull request from `head` into `base`, and return its URL. @@ -1278,6 +1276,16 @@ impl GitHub { } fn output_with_input(&self, args: &[&str], input: Option<&[u8]>) -> Result { + let output = self.raw_output_with_input(args, input)?; + if !output.status.success() && self.record_quota_failure(args, &output.stderr).is_err() { + // The original failure remains the caller's error, even when the + // machine cannot persist its wait or identify its signed-in user. + eprintln!("thirdshift: could not record GitHub Quota wait"); + } + Ok(output) + } + + fn raw_output_with_input(&self, args: &[&str], input: Option<&[u8]>) -> Result { process::output( Command::new("gh").args(args), input, diff --git a/src/github/advisories.rs b/src/github/advisories.rs index f0440f079..15226fc36 100644 --- a/src/github/advisories.rs +++ b/src/github/advisories.rs @@ -735,7 +735,7 @@ impl GitHub { pub(crate) fn inbox_findings(&self, repo: &str) -> Result> { self.list_security_records(repo, || { - if let Some(warning) = self.inbox_quotas()?.warning() { + if let Some(warning) = super::QuotaWait::warning(&self.inbox_quotas()?) { bail!(warning); } Ok(()) diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs index d96c158c2..fd405299c 100644 --- a/src/github/execution_tests.rs +++ b/src/github/execution_tests.rs @@ -58,6 +58,125 @@ use std::fs; use std::path::PathBuf; use std::time::{Duration, Instant}; +#[test] +fn primary_quota_failure_records_a_shared_wait() { + with_recorded_signal( + "github::execution_tests::primary_quota_failure_records_a_shared_wait", + |_| { + let fixture = Fixture::new(""); + // SAFETY: this test runs in an isolated child before any workers. + unsafe { std::env::set_var("HOME", fixture.temp.path()) }; + fixture.script( + r#"case "$*" in + 'api user --hostname github.com') echo '{"login":"octocat"}' ;; + 'api rate_limit --hostname github.com') + echo '{"resources":{"core":{"remaining":0,"reset":4102444800}}}' ;; + *) echo 'API rate limit exceeded for user ID 123. (HTTP 403)' >&2; exit 1 ;; +esac"#, + ); + let error = GitHub::new().issue_title(&issue()).unwrap_err(); + assert!(format!("{error:#}").contains("API rate limit exceeded")); + let path = fixture.path(".thirdshift/github-quota.json"); + assert!( + path.exists(), + "primary quota failure left no shared Quota wait" + ); + let record: Value = serde_json::from_slice(&fs::read(path).unwrap()).unwrap(); + assert_eq!(record["waits"][0]["host"], "github.com"); + assert_eq!(record["waits"][0]["login"], "octocat"); + assert_eq!(record["waits"][0]["resource"], "core"); + assert_eq!(record["waits"][0]["reset_at"], "2100-01-01T00:00:00Z"); + assert_eq!(record["waits"][0]["remaining"], 0); + }, + ); +} + +#[test] +fn primary_quota_failure_scopes_graphql_and_defaults_only_unreadable_resets() { + with_recorded_signal( + "github::execution_tests::primary_quota_failure_scopes_graphql_and_defaults_only_unreadable_resets", + |_| { + for (index, response) in [ + "{}", + r#"{"resources":{"graphql":{"reset":"bad"}}}"#, + r#"{"resources":{"graphql":{"reset":4102444800}}}"#, + ] + .into_iter() + .enumerate() + { + let fixture = Fixture::new(""); + // SAFETY: isolated test process, before workers start. + unsafe { + std::env::set_var("GH_HOST", "git.example.com"); + std::env::set_var("GH_ENTERPRISE_TOKEN", "test-secret-never-stored"); + } + fixture.script(&format!( + "case \"$1 $2\" in\n 'api user') echo '{{\"login\":\"enterprise-user\"}}' ;;\n 'api rate_limit') echo '{response}' ;;\n *) echo 'GraphQL: RATE_LIMIT_EXCEEDED (HTTP 429)' >&2; exit 1 ;;\nesac" + )); + let before = Utc::now(); + assert!(GitHub::new().issue_title(&issue()).is_err()); + let after = Utc::now(); + let bytes = fs::read(fixture.path(".thirdshift/github-quota.json")).unwrap(); + assert!(!String::from_utf8_lossy(&bytes).contains("test-secret-never-stored")); + let record: Value = serde_json::from_slice(&bytes).unwrap(); + let wait = &record["waits"][0]; + assert_eq!(wait["host"], "git.example.com"); + assert_eq!(wait["login"], "enterprise-user"); + assert_eq!(wait["resource"], "graphql"); + let reset = DateTime::parse_from_rfc3339(wait["reset_at"].as_str().unwrap()) + .unwrap() + .to_utc(); + if index == 2 { + assert_eq!(reset.timestamp(), 4102444800); + } else { + assert!(reset >= before + chrono::Duration::minutes(15)); + assert!(reset <= after + chrono::Duration::minutes(15)); + } + assert!( + GitHub::new() + .quota_waits_for("github.com") + .unwrap() + .is_empty() + ); + assert_eq!( + GitHub::new() + .quota_waits_for("git.example.com") + .unwrap() + .len(), + 1 + ); + } + }, + ); +} + +#[test] +fn changing_credentials_does_not_reuse_another_users_wait() { + with_recorded_signal( + "github::execution_tests::changing_credentials_does_not_reuse_another_users_wait", + |_| { + let fixture = Fixture::new(""); + fixture.script( + r#"case "$1 $2" in + 'api user') printf '{"login":"%s"}\n' "$GH_TOKEN" ;; + 'api rate_limit') echo '{"resources":{"core":{"remaining":0,"reset":4102444800}}}' ;; + *) echo 'API rate limit exceeded' >&2; exit 1 ;; +esac"#, + ); + // SAFETY: isolated test process, before workers start. + unsafe { std::env::set_var("GH_TOKEN", "alice") }; + assert!(GitHub::new().issue_title(&issue()).is_err()); + assert_eq!(GitHub::new().quota_waits().unwrap().len(), 1); + unsafe { std::env::set_var("GH_TOKEN", "bob") }; + assert!(GitHub::new().quota_waits().unwrap().is_empty()); + assert!(GitHub::new().issue_title(&issue()).is_err()); + assert_eq!(GitHub::new().quota_waits().unwrap().len(), 1); + unsafe { std::env::set_var("GH_TOKEN", "alice") }; + assert_eq!(GitHub::new().quota_waits().unwrap().len(), 1); + }, + ); +} + /// PATH changes happen only in the isolated process. Cleanup uses the /// executable's recorded PID, including after a failed assertion. struct Fixture { @@ -77,7 +196,11 @@ impl Fixture { ); // SAFETY: this isolated process runs one test; no environment readers // or execution workers have started. - unsafe { std::env::set_var("PATH", path) }; + unsafe { + std::env::set_var("PATH", path); + std::env::set_var("HOME", fixture.temp.path()); + std::env::remove_var("GH_HOST"); + } fixture } @@ -424,7 +547,7 @@ fn permanent_and_quota_read_failures_are_not_retried() { "API rate limit exceeded for user ID 123. (HTTP 429)", ] { fixture.script(&format!( - "echo read >> '{}'\ncat >&2 <<'ERROR'\n{stderr}\nERROR\nexit 1", + "case \"$1 $2\" in\n 'api user') echo '{{\"login\":\"runner\"}}'; exit 0 ;;\n 'api rate_limit') echo '{{}}'; exit 0 ;;\nesac\necho read >> '{}'\ncat >&2 <<'ERROR'\n{stderr}\nERROR\nexit 1", fixture.path("reads").display() )); let error = GitHub::new().issue_title(&issue()).unwrap_err(); diff --git a/src/github/quota.rs b/src/github/quota.rs index da759ee96..9cbfcc856 100644 --- a/src/github/quota.rs +++ b/src/github/quota.rs @@ -1,4 +1,5 @@ -//! GitHub's two quota clocks. The Inbox leaves a fixed reserve for the factory. +//! Shared machine-local Quota waits. Observing a wait does not yet change how +//! Passes or running Commands execute. use anyhow::{Context, Result}; use chrono::{DateTime, SecondsFormat, Utc}; use serde::{Deserialize, Serialize}; @@ -6,7 +7,37 @@ use serde_json::Value; use super::GitHub; -#[derive(Clone, Serialize, Deserialize)] +mod identity; +mod store; +#[cfg(test)] +mod tests; +// The wait planner is the seam the following Command-wait Ticket consumes. +#[expect( + dead_code, + reason = "wait planning is connected to Commands by a later Ticket" +)] +pub(crate) mod wait; + +pub(crate) use store::QuotaWait; +use store::Store; + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub(crate) enum Resource { + Core, + Graphql, +} + +impl Resource { + fn name(self) -> &'static str { + match self { + Self::Core => "core", + Self::Graphql => "graphql", + } + } +} + +#[derive(Clone, Debug, Serialize, Deserialize)] pub(crate) struct Quota { remaining: u64, reset_at: DateTime, @@ -39,46 +70,116 @@ impl Quota { }) } - fn low(&self) -> bool { - self.remaining < 500 && self.reset_at > Utc::now() + fn low(&self, now: DateTime) -> bool { + self.remaining < 500 && self.reset_at > now } } -#[derive(Clone, Serialize, Deserialize)] -pub(crate) struct Quotas { - pub graphql: Quota, - rest: Quota, -} - -impl Quotas { - pub fn warning(&self) -> Option { - let reasons: Vec<_> = [ - (&self.graphql, "GraphQL points"), - (&self.rest, "REST requests"), - ] - .into_iter() - .filter(|(quota, _)| quota.low()) - .map(|(quota, name)| { - format!( - "{} {name} remaining (fewer than 500); Inbox reads paused until {}", - quota.remaining, - quota.reset_at.to_rfc3339_opts(SecondsFormat::Secs, true), - ) - }) - .collect(); +impl QuotaWait { + pub fn warning(waits: &[Self]) -> Option { + let reasons: Vec<_> = waits + .iter() + .filter(|wait| wait.quota.reset_at > Utc::now()) + .map(|wait| { + let name = match wait.resource { + Resource::Graphql => "GraphQL points", + Resource::Core => "REST requests", + }; + let reserve = if wait.quota.remaining < 500 { + " (fewer than 500)" + } else { + "" + }; + format!( + "{} {name} remaining{reserve}; Inbox reads paused until {}", + wait.quota.remaining, + wait.quota + .reset_at + .to_rfc3339_opts(SecondsFormat::Secs, true), + ) + }) + .collect(); (!reasons.is_empty()).then(|| reasons.join("; ")) } } impl GitHub { + // Quota probes deliberately bypass failure recording: learning a reset or + // identity must never recursively probe another failure. + fn quota_json(&self, args: &[&str]) -> Result { + let output = self.raw_output_with_input(args, None)?; + anyhow::ensure!(output.status.success(), "GitHub quota probe failed"); + Ok(serde_json::from_slice(&output.stdout)?) + } + + /// Read-only access for the Inbox and later GitHub callers. With a known + /// identity this makes no GitHub calls, even across process restarts. + pub(crate) fn quota_waits(&self) -> Result> { + let host = identity::host(&[]); + self.quota_waits_for(&host) + } + + pub(crate) fn quota_waits_for(&self, host: &str) -> Result> { + let store = Store::machine()?; + let host = host.to_ascii_lowercase(); + match identity::cached(&store, &host)? { + Some(account) => store.waiting(&account, Utc::now()), + None => Ok(Vec::new()), + } + } + + pub(super) fn observe_quota(&self, resource: Resource, quota: Quota) -> Result<()> { + let store = Store::machine()?; + let host = identity::host(&[]); + let account = identity::resolve(self, &store, &host)?; + store.observe(&account, resource, quota, Utc::now()) + } + /// This endpoint reports both quotas and does not spend REST requests: /// https://docs.github.com/en/rest/rate-limit/rate-limit - pub(crate) fn inbox_quotas(&self) -> Result { + pub(crate) fn inbox_quotas(&self) -> Result> { let json = self.gh_json(&["api", "rate_limit"])?; - let resources = &json["resources"]; - Ok(Quotas { - graphql: Quota::rest(&resources["graphql"])?, - rest: Quota::rest(&resources["core"])?, - }) + for resource in [Resource::Graphql, Resource::Core] { + self.observe_quota(resource, Quota::rest(&json["resources"][resource.name()])?)?; + } + self.quota_waits() + } + + pub(super) fn record_quota_failure(&self, args: &[&str], stderr: &[u8]) -> Result<()> { + let diagnostic = String::from_utf8_lossy(stderr).to_ascii_lowercase(); + if diagnostic.contains("secondary rate limit") + || !(diagnostic.contains("api rate limit exceeded") + || diagnostic.contains("api rate limit already exceeded") + || diagnostic.contains("rate_limit_exceeded") + || diagnostic.contains("rate_limited")) + { + return Ok(()); + } + let resource = if diagnostic.contains("graphql") + || args.first() == Some(&"api") && args.contains(&"graphql") + || args.first() != Some(&"api") + && (diagnostic.contains("rate_limit_exceeded") + || diagnostic.contains("rate_limited")) + { + Resource::Graphql + } else { + Resource::Core + }; + let host = identity::host(args); + let store = Store::machine()?; + let account = identity::resolve(self, &store, &host)?; + let now = Utc::now(); + let resource_json = self + .quota_json(&["api", "rate_limit", "--hostname", &host]) + .unwrap_or(Value::Null); + let resource_json = &resource_json["resources"][resource.name()]; + let quota = Quota { + remaining: resource_json["remaining"].as_u64().unwrap_or(0), + reset_at: resource_json["reset"] + .as_i64() + .and_then(|reset| DateTime::from_timestamp(reset, 0)) + .unwrap_or(now + chrono::Duration::minutes(15)), + }; + store.exhausted(&account, resource, quota, now) } } diff --git a/src/github/quota/identity.rs b/src/github/quota/identity.rs new file mode 100644 index 000000000..d35bd7a47 --- /dev/null +++ b/src/github/quota/identity.rs @@ -0,0 +1,128 @@ +//! Cache the authenticated login without keeping credentials. A changed +//! environment token or gh account configuration requires a new observation. +use anyhow::{Context, Result}; +use serde_json::Value; +use sha2::{Digest, Sha256}; + +use super::store::{Account, Store}; +use crate::github::GitHub; + +pub(super) fn host(args: &[&str]) -> String { + for arg in args { + if let Some(host) = arg.strip_prefix("--hostname=") { + return host.to_ascii_lowercase(); + } + if let Some(host) = url_host(arg) { + return host; + } + } + for pair in args.windows(2) { + if matches!(pair[0], "--hostname" | "-h") { + return pair[1].to_ascii_lowercase(); + } + if matches!(pair[0], "--repo" | "-R") + && let [hostname, _, _] = pair[1].split('/').collect::>().as_slice() + { + return hostname.to_ascii_lowercase(); + } + } + std::env::var("GH_HOST") + .ok() + .filter(|host| !host.is_empty()) + .unwrap_or_else(|| "github.com".into()) + .to_ascii_lowercase() +} + +fn url_host(value: &str) -> Option { + let url = reqwest::Url::parse(value).ok()?; + let host = url.host_str()?; + Some(if host == "api.github.com" { + "github.com".into() + } else { + host.to_ascii_lowercase() + }) +} + +fn credential() -> Result { + let mut digest = Sha256::new(); + for name in [ + "GH_TOKEN", + "GITHUB_TOKEN", + "GH_ENTERPRISE_TOKEN", + "GITHUB_ENTERPRISE_TOKEN", + ] { + digest.update(name); + digest.update([0]); + if let Ok(value) = std::env::var(name) { + digest.update(value); + } + digest.update([0]); + } + let dir = std::env::var_os("GH_CONFIG_DIR") + .map(std::path::PathBuf::from) + .or_else(|| { + std::env::var_os("XDG_CONFIG_HOME").map(|dir| std::path::PathBuf::from(dir).join("gh")) + }) + .unwrap_or(crate::config::home()?.join(".config/gh")); + digest.update(dir.as_os_str().as_encoded_bytes()); + match std::fs::read(dir.join("hosts.yml")) { + Ok(bytes) => digest.update(bytes), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => return Err(error.into()), + } + // Only this one-way fingerprint is persisted, never the token or config. + Ok(format!("{:x}", digest.finalize())) +} + +pub(super) fn cached(store: &Store, host: &str) -> Result> { + store.account(host, &credential()?) +} + +pub(super) fn resolve(github: &GitHub, store: &Store, host: &str) -> Result { + let credential = credential()?; + if let Some(account) = store.account(host, &credential)? { + return Ok(account); + } + // Try both quota resources: a depleted REST quota can still leave GraphQL + // available. gh's auth report can also retain a configured active login. + let login = github + .quota_json(&["api", "user", "--hostname", host]) + .ok() + .and_then(|json| json["login"].as_str().map(str::to_owned)) + .or_else(|| { + github + .quota_json(&[ + "api", + "graphql", + "--hostname", + host, + "-f", + "query={viewer{login}}", + ]) + .ok() + .and_then(|json| json["data"]["viewer"]["login"].as_str().map(str::to_owned)) + }) + .or_else(|| { + let json = github + .quota_json(&[ + "auth", + "status", + "--active", + "--hostname", + host, + "--json", + "hosts", + ]) + .ok()?; + json["hosts"][host].as_array()?.iter().find_map(|account| { + (account["active"] == Value::Bool(true)) + .then(|| account["login"].as_str().map(str::to_owned)) + .flatten() + }) + }) + .filter(|login| !login.is_empty()) + .context("could not identify the authenticated GitHub user for Quota wait")?; + let account = Account::new(host, &login); + store.remember(account.clone(), credential)?; + Ok(account) +} diff --git a/src/github/quota/store.rs b/src/github/quota/store.rs new file mode 100644 index 000000000..df23bb010 --- /dev/null +++ b/src/github/quota/store.rs @@ -0,0 +1,163 @@ +//! One machine-local record, with a stable lock across atomic replacements. +use std::fs::{self, OpenOptions}; +use std::io::Write; +use std::path::PathBuf; + +use anyhow::Result; +use chrono::{DateTime, Utc}; +use serde::{Deserialize, Serialize}; + +use super::{Quota, Resource}; + +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +pub(crate) struct Account { + host: String, + login: String, +} + +impl Account { + pub fn new(host: &str, login: &str) -> Self { + Self { + host: host.to_ascii_lowercase(), + login: login.to_ascii_lowercase(), + } + } +} + +#[derive(Clone, Debug, Serialize, Deserialize)] +pub(crate) struct QuotaWait { + #[serde(flatten)] + account: Account, + pub resource: Resource, + #[serde(flatten)] + pub quota: Quota, +} + +#[derive(Serialize, Deserialize)] +struct Identity { + credential: String, + account: Account, +} + +#[derive(Default, Serialize, Deserialize)] +struct Record { + identities: Vec, + waits: Vec, +} + +pub(crate) struct Store { + path: PathBuf, +} + +impl Store { + pub fn new(path: PathBuf) -> Self { + Self { path } + } + + pub fn machine() -> Result { + Ok(Self::new( + crate::config::home()?.join(".thirdshift/github-quota.json"), + )) + } + + pub fn waiting(&self, account: &Account, now: DateTime) -> Result> { + Ok(self + .read()? + .waits + .into_iter() + .filter(|wait| &wait.account == account && wait.quota.reset_at > now) + .collect()) + } + + /// A successful quota observation supersedes the previous one, including + /// clearing a wait when the reserve is available again. + pub fn observe( + &self, + account: &Account, + resource: Resource, + quota: Quota, + now: DateTime, + ) -> Result<()> { + let waiting = quota.low(now); + self.record(account, resource, quota, waiting, now) + } + + pub fn exhausted( + &self, + account: &Account, + resource: Resource, + quota: Quota, + now: DateTime, + ) -> Result<()> { + self.record(account, resource, quota, true, now) + } + + fn record( + &self, + account: &Account, + resource: Resource, + quota: Quota, + waiting: bool, + now: DateTime, + ) -> Result<()> { + self.update(|record| { + record.waits.retain(|wait| { + wait.quota.reset_at > now + && !(&wait.account == account && wait.resource == resource) + }); + if waiting && quota.reset_at > now { + record.waits.push(QuotaWait { + account: account.clone(), + resource, + quota, + }); + } + }) + } + + pub fn account(&self, host: &str, credential: &str) -> Result> { + Ok(self + .read()? + .identities + .into_iter() + .find(|identity| identity.account.host == host && identity.credential == credential) + .map(|identity| identity.account)) + } + + pub fn remember(&self, account: Account, credential: String) -> Result<()> { + self.update(|record| { + record.identities.retain(|identity| { + identity.account.host != account.host || identity.credential != credential + }); + record.identities.push(Identity { + account, + credential, + }); + }) + } + + fn read(&self) -> Result { + match fs::read(&self.path) { + Ok(bytes) => Ok(serde_json::from_slice(&bytes)?), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(Record::default()), + Err(error) => Err(error.into()), + } + } + + fn update(&self, change: impl FnOnce(&mut Record)) -> Result<()> { + let dir = self.path.parent().expect("quota record has a parent"); + fs::create_dir_all(dir)?; + let lock = OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(self.path.with_extension("lock"))?; + lock.lock()?; + let mut record = self.read()?; + change(&mut record); + let mut tmp = tempfile::NamedTempFile::new_in(dir)?; + tmp.write_all(&serde_json::to_vec(&record)?)?; + tmp.persist(&self.path)?; + Ok(()) + } +} diff --git a/src/github/quota/tests.rs b/src/github/quota/tests.rs new file mode 100644 index 000000000..251d1b2e4 --- /dev/null +++ b/src/github/quota/tests.rs @@ -0,0 +1,133 @@ +use super::*; +use std::time::Duration; +use store::Account; +use wait::{WaitBudget, WaitCapExceeded}; + +fn now() -> DateTime { + DateTime::parse_from_rfc3339("2026-11-10T12:00:00Z") + .unwrap() + .to_utc() +} + +fn quota(remaining: u64) -> Quota { + Quota { + remaining, + reset_at: now() + chrono::Duration::hours(1), + } +} + +#[test] +fn waits_are_isolated_by_host_user_and_resource_and_expire_at_reset() { + let dir = tempfile::tempdir().unwrap(); + let store = Store::new(dir.path().join("quota.json")); + let alice = Account::new("github.com", "Alice"); + let bob = Account::new("github.com", "Bob"); + let enterprise = Account::new("git.example.com", "Alice"); + store + .observe(&alice, Resource::Core, quota(499), now()) + .unwrap(); + store + .observe(&bob, Resource::Graphql, quota(123), now()) + .unwrap(); + store + .observe(&enterprise, Resource::Core, quota(42), now()) + .unwrap(); + let waits = store + .waiting(&Account::new("GITHUB.COM", "ALICE"), now()) + .unwrap(); + assert_eq!(waits.len(), 1); + assert_eq!(waits[0].resource, Resource::Core); + assert_eq!(waits[0].quota.remaining, 499); + assert_eq!( + store.waiting(&bob, now()).unwrap()[0].resource, + Resource::Graphql + ); + assert_eq!( + store.waiting(&enterprise, now()).unwrap()[0] + .quota + .remaining, + 42 + ); + assert!( + store + .waiting(&alice, now() + chrono::Duration::hours(1)) + .unwrap() + .is_empty() + ); + store + .observe(&alice, Resource::Core, quota(500), now()) + .unwrap(); + assert!(store.waiting(&alice, now()).unwrap().is_empty()); + assert_eq!(store.waiting(&bob, now()).unwrap().len(), 1); +} + +#[test] +fn concurrent_writers_preserve_each_other_and_readers_only_see_complete_records() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("quota.json"); + let barrier = std::sync::Barrier::new(17); + std::thread::scope(|scope| { + for user in 0..16 { + let path = &path; + let barrier = &barrier; + scope.spawn(move || { + let store = Store::new(path.clone()); + let account = Account::new("github.com", &format!("user{user}")); + barrier.wait(); + for resource in [Resource::Core, Resource::Graphql] { + store + .observe(&account, resource, quota(100 + user), now()) + .unwrap(); + } + }); + } + barrier.wait(); + let store = Store::new(path.clone()); + for _ in 0..100 { + store + .waiting(&Account::new("github.com", "user0"), now()) + .unwrap(); + } + }); + let store = Store::new(path); + for user in 0..16 { + let waits = store + .waiting(&Account::new("github.com", &format!("user{user}")), now()) + .unwrap(); + assert_eq!(waits.len(), 2); + assert!(waits.iter().all(|wait| wait.quota.remaining == 100 + user)); + } +} + +#[test] +fn wait_calculation_uses_injected_clock_and_inclusive_jitter_endpoints() { + let reset = now() + chrono::Duration::minutes(5); + for (jitter, expected) in [(0, 360), (37, 397), (60, 420)] { + let mut budget = WaitBudget::default(); + assert_eq!( + budget.next_with(reset, now, || jitter), + Ok(Duration::from_secs(expected)) + ); + } +} + +#[test] +fn a_commands_wait_total_accepts_two_hours_and_rejects_more_without_spending_it() { + let mut budget = WaitBudget::default(); + let reset = now() + chrono::Duration::seconds(3539); + assert_eq!( + budget.next_with(reset, now, || 0), + Ok(Duration::from_secs(3599)) + ); + assert_eq!( + budget.next_with(reset, now, || 0), + Ok(Duration::from_secs(3599)) + ); + assert_eq!(budget.next_with(now(), now, || 0), Err(WaitCapExceeded)); + let reset = now() - chrono::Duration::seconds(58); + assert_eq!( + budget.next_with(reset, now, || 0), + Ok(Duration::from_secs(2)) + ); + assert_eq!(budget.next_with(reset, now, || 0), Err(WaitCapExceeded)); +} diff --git a/src/github/quota/wait.rs b/src/github/quota/wait.rs new file mode 100644 index 000000000..4cbed3773 --- /dev/null +++ b/src/github/quota/wait.rs @@ -0,0 +1,40 @@ +//! Wait planning only; later Tickets connect this to running Commands. +use std::time::Duration; + +use chrono::{DateTime, Utc}; + +const COMMAND_CAP: Duration = Duration::from_secs(2 * 60 * 60); + +#[derive(Default)] +pub(crate) struct WaitBudget { + total: Duration, +} + +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct WaitCapExceeded; + +impl WaitBudget { + pub fn next(&mut self, reset: DateTime) -> Result { + self.next_with(reset, Utc::now, || rand::random_range(0..=60)) + } + + /// Reserve this wait in the Command's running total. Rejected waits leave + /// the total unchanged; the caller never sleeps beyond the two-hour cap. + pub fn next_with( + &mut self, + reset: DateTime, + clock: impl FnOnce() -> DateTime, + jitter_seconds: impl FnOnce() -> u64, + ) -> Result { + let jitter = jitter_seconds(); + assert!(jitter <= 60, "Quota wait jitter must be in 0..=60 seconds"); + let until = reset + chrono::Duration::seconds(60 + jitter as i64); + let wait = (until - clock()).to_std().unwrap_or(Duration::ZERO); + let total = self.total.checked_add(wait).ok_or(WaitCapExceeded)?; + if total > COMMAND_CAP { + return Err(WaitCapExceeded); + } + self.total = total; + Ok(wait) + } +} diff --git a/src/logs/github.rs b/src/logs/github.rs index da122f790..6a363c4d2 100644 --- a/src/logs/github.rs +++ b/src/logs/github.rs @@ -16,7 +16,7 @@ use serde::{Deserialize, Serialize}; use super::reading::Snapshot; use crate::config::UserConfig; -use crate::github::{GitHub, InboxState, IssueState, PullRequestState, Quotas}; +use crate::github::{GitHub, InboxState, IssueState, PullRequestState, QuotaWait}; /// How long `status` reuses the kept answer before reading GitHub again. pub const FRESH_FOR: Duration = Duration::from_secs(5 * 60); @@ -60,8 +60,6 @@ struct File { answered_at: BTreeMap, #[serde(default)] failure: Option, - #[serde(default)] - quotas: Option, /// The warning at the last refresh, so crossing the age threshold /// wakes pages even when the facts and failure are unchanged. #[serde(default)] @@ -81,7 +79,7 @@ pub struct Kept { pub age: Option, last_answered: Option>, pub failure: Option, - quotas: Option, + waits: Vec, } /// The exact kept facts for a row, including dispatched work and private @@ -187,7 +185,7 @@ impl Kept { pub fn warning(&self) -> Option { let mut reasons = Vec::new(); - if let Some(reason) = self.quotas.as_ref().and_then(Quotas::warning) { + if let Some(reason) = QuotaWait::warning(&self.waits) { reasons.push(reason); } if self.age.is_none_or(|age| age > FRESH_FOR) @@ -306,10 +304,13 @@ pub fn load(logs_dir: &Path) -> Kept { age: modified.and_then(|at| at.elapsed().ok()), last_answered: modified.map(DateTime::from), failure: file.failure, - quotas: file.quotas, + waits: GitHub::new().quota_waits().unwrap_or_default(), } } - None => Kept::default(), + None => Kept { + waits: GitHub::new().quota_waits().unwrap_or_default(), + ..Kept::default() + }, } } @@ -360,16 +361,20 @@ pub(super) fn refresh( } let subjects = &work.subjects; let findings = &work.findings; - if kept.age.is_some_and(|age| age < fresh_for) - || !work.has_remote_reads() - || kept.quotas.as_ref().and_then(Quotas::warning).is_some() - { + if QuotaWait::warning(&kept.waits).is_some() { + // A factory call may have learned this wait outside the Inbox. Keep + // the warning marker in sync so open pages learn about it too. + save(&config.logs_dir, &kept, false)?; + return Ok(kept); + } + if kept.age.is_some_and(|age| age < fresh_for) || !work.has_remote_reads() { return Ok(kept); } let github = GitHub::new(); match github.inbox_quotas() { - Ok(quotas) => kept.quotas = Some(quotas), + Ok(waits) => kept.waits = waits, Err(error) => { + kept.waits = github.quota_waits().unwrap_or_default(); kept.failure = Some(format!("{error:#}")); save(&config.logs_dir, &kept, false)?; return Ok(kept); @@ -378,12 +383,12 @@ pub(super) fn refresh( let mut answered = false; let mut read_failed = false; for chunk in subjects.chunks(100) { - if kept.quotas.as_ref().and_then(Quotas::warning).is_some() { + if QuotaWait::warning(&kept.waits).is_some() { break; } let urls: Vec<_> = chunk.iter().map(|subject| subject.url.clone()).collect(); match github.inbox_states(&urls) { - Ok((states, quota)) => { + Ok(states) => { let answered_at = Utc::now().to_rfc3339(); for subject in chunk { let url = &subject.url; @@ -406,13 +411,11 @@ pub(super) fn refresh( kept.states.insert(url.clone(), state); kept.answered_at.insert(url.clone(), answered_at.clone()); } - kept.quotas - .as_mut() - .expect("quotas read before rows") - .graphql = quota; + kept.waits = github.quota_waits()?; answered = true; } Err(error) => { + kept.waits = github.quota_waits().unwrap_or_default(); kept.failure = Some(format!("{error:#}")); read_failed = true; break; @@ -427,7 +430,7 @@ pub(super) fn refresh( .push(subject); } for (repository, subjects) in repositories { - if kept.quotas.as_ref().and_then(Quotas::warning).is_some() { + if QuotaWait::warning(&kept.waits).is_some() { break; } // One record must not choose the request path or fail every other read. @@ -472,8 +475,8 @@ pub(super) fn refresh( Err(error) => { kept.failure = Some(format!("{error:#}")); read_failed = true; - if let Ok(quotas) = github.inbox_quotas() { - kept.quotas = Some(quotas); + if let Ok(waits) = github.inbox_quotas() { + kept.waits = waits; } break; } @@ -481,8 +484,9 @@ pub(super) fn refresh( // Includes every paginated request and private-storage discovery, so // another repository cannot start a list on an exhausted reserve. match github.inbox_quotas() { - Ok(quotas) => kept.quotas = Some(quotas), + Ok(waits) => kept.waits = waits, Err(error) => { + kept.waits = github.quota_waits().unwrap_or_default(); kept.failure = Some(format!("{error:#}")); read_failed = true; break; @@ -510,7 +514,6 @@ fn save(logs_dir: &Path, kept: &Kept, answered: bool) -> Result<()> { findings: kept.findings.clone(), answered_at: kept.answered_at.clone(), failure: kept.failure.clone(), - quotas: kept.quotas.clone(), warning: kept.warning(), }; let bytes = serde_json::to_vec(&file)?; diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 84399eea2..bafaf0458 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -1566,7 +1566,7 @@ fn api(state: &mut Json, positional: &[String], flags: &Flags) { advisory_api(state, positional, flags); return; } - if path == "user" && flags.is_empty() { + if path == "user" && flags.keys().all(|flag| flag == "hostname") { let email = state.get("user_email").cloned().unwrap_or(Null); println!( "{}", diff --git a/tests/status.rs b/tests/status.rs index 536219931..0a5021bbb 100644 --- a/tests/status.rs +++ b/tests/status.rs @@ -3451,9 +3451,7 @@ fn github_failures_warn_with_last_answer_and_clear_after_recovery() { #[test] fn status_reserves_both_quotas_until_their_resets() { - for (resource, kept_name, description) in - [("graphql", "graphql", "GraphQL"), ("core", "rest", "REST")] - { + for (resource, description) in [("graphql", "GraphQL"), ("core", "REST")] { let scenario = Scenario::new(); ready_record_with_pr(&scenario, 7, &hours_ago(48), Some(7)); github_pr_is(&scenario, 7, "OPEN", None, None); @@ -3504,9 +3502,15 @@ fn status_reserves_both_quotas_until_their_resets() { "read while {description} quota was low" ); // Move the recorded reset into the past at the fixture's time seam. - let path = scenario.path(GITHUB_CACHE); + let path = scenario.path("home/.thirdshift/github-quota.json"); let mut kept: Value = serde_json::from_slice(&fs::read(&path).unwrap()).unwrap(); - kept["quotas"][kept_name]["reset_at"] = hours_ago(1).into(); + let wait = kept["waits"] + .as_array_mut() + .unwrap() + .iter_mut() + .find(|wait| wait["resource"] == resource) + .unwrap(); + wait["reset_at"] = hours_ago(1).into(); fs::write(path, serde_json::to_vec(&kept).unwrap()).unwrap(); gh["rate_limits"][resource]["remaining"] = 5000.into(); scenario.write_gh_state(&gh); From 558c7be084a3887b6c555cd96d1e94217c20340a Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 10 Nov 2026 20:34:46 -0500 Subject: [PATCH 02/25] Fix quota host attribution and retain Inbox read-only checks --- src/github/execution_tests.rs | 30 ++++++++++++++++++++++++++ src/github/quota.rs | 18 ++++++++++------ src/github/quota/identity.rs | 40 +++++++++++++++++++++++------------ src/github/quota/tests.rs | 3 ++- tests/status.rs | 26 ++++++++++++++--------- 5 files changed, 87 insertions(+), 30 deletions(-) diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs index fd405299c..d707cb002 100644 --- a/src/github/execution_tests.rs +++ b/src/github/execution_tests.rs @@ -177,6 +177,36 @@ esac"#, ); } +#[test] +fn quota_failure_uses_repository_host_instead_of_comment_url() { + with_recorded_signal( + "github::execution_tests::quota_failure_uses_repository_host_instead_of_comment_url", + |_| { + let fixture = Fixture::new(""); + fixture.script( + r#"case "$1 $2" in + 'api user') echo '{"login":"octocat"}' ;; + 'api rate_limit') + echo '{"resources":{"core":{"remaining":0,"reset":4102444800}}}' ;; + *) echo 'API rate limit exceeded for user ID 123. (HTTP 403)' >&2; exit 1 ;; +esac"#, + ); + assert!( + GitHub::new() + .comment_issue(&issue(), "https://docs.example.net/") + .is_err() + ); + let record: Value = serde_json::from_slice( + &fs::read(fixture.path(".thirdshift/github-quota.json")).unwrap(), + ) + .unwrap(); + assert_eq!(record["waits"][0]["host"], "github.com"); + assert_eq!(record["waits"][0]["login"], "octocat"); + assert_eq!(record["waits"][0]["resource"], "core"); + }, + ); +} + /// PATH changes happen only in the isolated process. Cleanup uses the /// executable's recorded PID, including after a failed assertion. struct Fixture { diff --git a/src/github/quota.rs b/src/github/quota.rs index 9cbfcc856..336832053 100644 --- a/src/github/quota.rs +++ b/src/github/quota.rs @@ -76,26 +76,32 @@ impl Quota { } impl QuotaWait { + pub fn reset_at(&self) -> DateTime { + self.quota.reset_at + } + + pub fn remaining(&self) -> u64 { + self.quota.remaining + } + pub fn warning(waits: &[Self]) -> Option { let reasons: Vec<_> = waits .iter() - .filter(|wait| wait.quota.reset_at > Utc::now()) + .filter(|wait| wait.reset_at() > Utc::now()) .map(|wait| { let name = match wait.resource { Resource::Graphql => "GraphQL points", Resource::Core => "REST requests", }; - let reserve = if wait.quota.remaining < 500 { + let reserve = if wait.remaining() < 500 { " (fewer than 500)" } else { "" }; format!( "{} {name} remaining{reserve}; Inbox reads paused until {}", - wait.quota.remaining, - wait.quota - .reset_at - .to_rfc3339_opts(SecondsFormat::Secs, true), + wait.remaining(), + wait.reset_at().to_rfc3339_opts(SecondsFormat::Secs, true), ) }) .collect(); diff --git a/src/github/quota/identity.rs b/src/github/quota/identity.rs index d35bd7a47..3226b8191 100644 --- a/src/github/quota/identity.rs +++ b/src/github/quota/identity.rs @@ -8,22 +8,36 @@ use super::store::{Account, Store}; use crate::github::GitHub; pub(super) fn host(args: &[&str]) -> String { - for arg in args { - if let Some(host) = arg.strip_prefix("--hostname=") { - return host.to_ascii_lowercase(); - } - if let Some(host) = url_host(arg) { - return host; + let mut hostname = None; + let mut repository = None; + let mut args = args.iter().copied(); + while let Some(arg) = args.next() { + match arg { + "--hostname" | "-h" => hostname = args.next(), + "--repo" | "-R" => repository = args.next(), + // Text and API field values are payload, even when they look + // like URLs or selector flags. + "--body" | "--title" | "-b" | "-t" | "-f" | "-F" => { + args.next(); + } + _ => { + if let Some(value) = arg.strip_prefix("--hostname=") { + hostname = Some(value); + } else if let Some(value) = arg.strip_prefix("--repo=") { + repository = Some(value); + } + } } } - for pair in args.windows(2) { - if matches!(pair[0], "--hostname" | "-h") { - return pair[1].to_ascii_lowercase(); + if let Some(hostname) = hostname { + return hostname.to_ascii_lowercase(); + } + if let Some(repository) = repository { + if let Some(host) = url_host(repository) { + return host; } - if matches!(pair[0], "--repo" | "-R") - && let [hostname, _, _] = pair[1].split('/').collect::>().as_slice() - { - return hostname.to_ascii_lowercase(); + if let [host, _, _] = repository.split('/').collect::>().as_slice() { + return host.to_ascii_lowercase(); } } std::env::var("GH_HOST") diff --git a/src/github/quota/tests.rs b/src/github/quota/tests.rs index 251d1b2e4..66d050e85 100644 --- a/src/github/quota/tests.rs +++ b/src/github/quota/tests.rs @@ -37,7 +37,8 @@ fn waits_are_isolated_by_host_user_and_resource_and_expire_at_reset() { .unwrap(); assert_eq!(waits.len(), 1); assert_eq!(waits[0].resource, Resource::Core); - assert_eq!(waits[0].quota.remaining, 499); + assert_eq!(waits[0].remaining(), 499); + assert_eq!(waits[0].reset_at(), now() + chrono::Duration::hours(1)); assert_eq!( store.waiting(&bob, now()).unwrap()[0].resource, Resource::Graphql diff --git a/tests/status.rs b/tests/status.rs index 0a5021bbb..e0758a69f 100644 --- a/tests/status.rs +++ b/tests/status.rs @@ -624,12 +624,14 @@ fn inbox_clears_ended_problems_on_a_later_local_start_or_the_configured_age() { assert_eq!(commands[0]["issue"]["number"], 12); assert_eq!(commands[0]["group"], "needs you"); assert_eq!(graphql_calls(&scenario).len(), 1); - assert!( - scenario - .gh_calls() - .iter() - .all(|call| call[0] == "api" && matches!(call[1].as_str(), "graphql" | "rate_limit")) - ); + assert!(scenario.gh_calls().iter().all(|call| { + call[0] == "api" + && (matches!(call[1].as_str(), "graphql" | "rate_limit") + || call + .iter() + .map(String::as_str) + .eq(["api", "user", "--hostname", "github.com"])) + })); } #[test] @@ -2931,10 +2933,14 @@ fn merged_pull_request_takes_its_ready_row_into_resolved_saying_so() { "{calls:?}" ); assert!( - scenario - .gh_calls() - .iter() - .all(|call| call[0] == "api" && matches!(call[1].as_str(), "graphql" | "rate_limit")), + scenario.gh_calls().iter().all(|call| call[0] == "api" + && (matches!(call[1].as_str(), "graphql" | "rate_limit") + || call.iter().map(String::as_str).eq([ + "api", + "user", + "--hostname", + "github.com" + ]))), "{:?}", scenario.gh_calls() ); From 008714be37a1ad2dabf7ad6aaea56123f8ac4adb Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 10 Nov 2026 20:38:11 -0500 Subject: [PATCH 03/25] Exercise Inbox recovery after the shared quota reset --- tests/status.rs | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/tests/status.rs b/tests/status.rs index e0758a69f..fd67528bf 100644 --- a/tests/status.rs +++ b/tests/status.rs @@ -2075,6 +2075,19 @@ fn graphql_calls(scenario: &Scenario) -> Vec> { scenario.gh_calls_of("api", "graphql") } +fn expire_quota_wait(scenario: &Scenario, resource: &str) { + let path = scenario.path("home/.thirdshift/github-quota.json"); + let mut record: Value = serde_json::from_slice(&fs::read(&path).unwrap()).unwrap(); + let wait = record["waits"] + .as_array_mut() + .unwrap() + .iter_mut() + .find(|wait| wait["resource"] == resource) + .unwrap(); + wait["reset_at"] = hours_ago(1).into(); + fs::write(path, serde_json::to_vec(&record).unwrap()).unwrap(); +} + fn age_cache_file(scenario: &Scenario, seconds: u64) { let file = fs::File::options() .write(true) @@ -3434,6 +3447,15 @@ fn github_failures_warn_with_last_answer_and_clear_after_recovery() { assert!(text.stdout.contains(diagnostic), "{}", text.stdout); gh["failing"] = serde_json::json!([]); scenario.write_gh_state(&gh); + if diagnostic.contains("API rate limit exceeded") { + let before = scenario.gh_calls(); + let paused = scenario.run(&["status"]); + assert!(paused.stdout.contains("Inbox reads paused until")); + assert_eq!(scenario.gh_calls(), before, "read before quota reset"); + // Removing the diagnostic does not replenish GitHub's quota. + // Advance the recorded reset at the fixture's time seam first. + expire_quota_wait(&scenario, "graphql"); + } let output = scenario.run(&["status", "--json"]); let status: Value = serde_json::from_str(&output.stdout).unwrap(); assert_eq!(status["github_failure"], Value::Null, "{status}"); @@ -3508,16 +3530,7 @@ fn status_reserves_both_quotas_until_their_resets() { "read while {description} quota was low" ); // Move the recorded reset into the past at the fixture's time seam. - let path = scenario.path("home/.thirdshift/github-quota.json"); - let mut kept: Value = serde_json::from_slice(&fs::read(&path).unwrap()).unwrap(); - let wait = kept["waits"] - .as_array_mut() - .unwrap() - .iter_mut() - .find(|wait| wait["resource"] == resource) - .unwrap(); - wait["reset_at"] = hours_ago(1).into(); - fs::write(path, serde_json::to_vec(&kept).unwrap()).unwrap(); + expire_quota_wait(&scenario, resource); gh["rate_limits"][resource]["remaining"] = 5000.into(); scenario.write_gh_state(&gh); let output = scenario.run(&["status", "--json"]); From a40e6f5cbc39e5d3b6b832498e639563685854ea Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 10 Nov 2026 20:43:42 -0500 Subject: [PATCH 04/25] Classify quota resources without matching repository names --- src/github/execution_tests.rs | 22 ++++++++++++++++++++++ src/github/quota.rs | 4 ++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs index d707cb002..7dc0f14d0 100644 --- a/src/github/execution_tests.rs +++ b/src/github/execution_tests.rs @@ -150,6 +150,28 @@ fn primary_quota_failure_scopes_graphql_and_defaults_only_unreadable_resets() { ); } +#[test] +fn a_rest_quota_failure_in_a_repository_named_graphql_keeps_the_core_resource() { + with_recorded_signal( + "github::execution_tests::a_rest_quota_failure_in_a_repository_named_graphql_keeps_the_core_resource", + |_| { + let fixture = Fixture::new(""); + fixture.script( + r#"case "$1 $2" in + 'api user') echo '{"login":"octocat"}' ;; + 'api rate_limit') echo '{"resources":{"core":{"remaining":0,"reset":4102444800}}}' ;; + *) echo 'HTTP 403: API rate limit exceeded (https://api.github.com/repos/acme/graphql/issues/7/comments)' >&2; exit 1 ;; +esac"#, + ); + let issue = IssueUrl::parse("https://github.com/acme/graphql/issues/7").unwrap(); + assert!(GitHub::new().comment_issue(&issue, "hello").is_err()); + let waits = GitHub::new().quota_waits().unwrap(); + assert_eq!(waits.len(), 1); + assert!(matches!(waits[0].resource, quota::Resource::Core)); + }, + ); +} + #[test] fn changing_credentials_does_not_reuse_another_users_wait() { with_recorded_signal( diff --git a/src/github/quota.rs b/src/github/quota.rs index 336832053..87437e8dc 100644 --- a/src/github/quota.rs +++ b/src/github/quota.rs @@ -161,8 +161,8 @@ impl GitHub { { return Ok(()); } - let resource = if diagnostic.contains("graphql") - || args.first() == Some(&"api") && args.contains(&"graphql") + let resource = if diagnostic.contains("graphql:") + || args.starts_with(&["api", "graphql"]) || args.first() != Some(&"api") && (diagnostic.contains("rate_limit_exceeded") || diagnostic.contains("rate_limited")) From b8334db8842e92e8660fe2fba70619930ecee182 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 10 Nov 2026 21:10:09 -0500 Subject: [PATCH 05/25] Keep Close comments out of quota host selection --- src/github/execution_tests.rs | 32 ++++++++++++++++++++++++++++++++ src/github/quota/identity.rs | 3 ++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs index 7dc0f14d0..3e65acc76 100644 --- a/src/github/execution_tests.rs +++ b/src/github/execution_tests.rs @@ -199,6 +199,38 @@ esac"#, ); } +#[test] +fn quota_failure_does_not_treat_a_close_comment_as_a_host_selector() { + with_recorded_signal( + "github::execution_tests::quota_failure_does_not_treat_a_close_comment_as_a_host_selector", + |_| { + let fixture = Fixture::new(""); + fixture.script( + r#"case "$1 $2" in + 'api user') echo '{"login":"octocat"}' ;; + 'api rate_limit') echo '{"resources":{"core":{"remaining":0,"reset":4102444800}}}' ;; + *) echo 'API rate limit exceeded' >&2; exit 1 ;; +esac"#, + ); + assert!( + GitHub::new() + .close_issue(&issue(), "--hostname=docs.example.net") + .is_err() + ); + assert_eq!( + GitHub::new().quota_waits_for("github.com").unwrap().len(), + 1 + ); + assert!( + GitHub::new() + .quota_waits_for("docs.example.net") + .unwrap() + .is_empty() + ); + }, + ); +} + #[test] fn quota_failure_uses_repository_host_instead_of_comment_url() { with_recorded_signal( diff --git a/src/github/quota/identity.rs b/src/github/quota/identity.rs index 3226b8191..99a100676 100644 --- a/src/github/quota/identity.rs +++ b/src/github/quota/identity.rs @@ -17,7 +17,8 @@ pub(super) fn host(args: &[&str]) -> String { "--repo" | "-R" => repository = args.next(), // Text and API field values are payload, even when they look // like URLs or selector flags. - "--body" | "--title" | "-b" | "-t" | "-f" | "-F" => { + "--body" | "--title" | "--comment" | "--label" | "--search" | "-b" | "-t" | "-f" + | "-F" => { args.next(); } _ => { From f5625cfb1cd09b3b63d8f2346c7e5440c24b341b Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 10 Nov 2026 22:43:42 -0500 Subject: [PATCH 06/25] Skip every Pass during a shared GitHub Quota wait (#1996) --- docs/guide.md | 6 +- site/architecture/index.html | 1 + site/docs/index.html | 6 +- site/how-it-works/index.html | 1 + site/index.html | 1 + src/architect.rs | 15 ++- src/command_limit.rs | 29 ++++- src/console/changes.rs | 3 + src/console/inbox.html | 1 + src/fix.rs | 2 +- src/github/quota.rs | 29 ++++- src/launch.rs | 14 +- src/logs/github.rs | 23 +++- src/logs/status.rs | 5 + src/logs/status/inbox.rs | 3 + src/pickup.rs | 13 +- src/processes.rs | 15 ++- src/processes/review.rs | 5 + src/security.rs | 10 +- src/triage/pass.rs | 2 +- tests/console/github.rs | 55 ++++++++ tests/pass_quota.rs | 242 +++++++++++++++++++++++++++++++++++ 22 files changed, 437 insertions(+), 44 deletions(-) create mode 100644 tests/pass_quota.rs diff --git a/docs/guide.md b/docs/guide.md index e3824b73d..5ff75f83c 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -164,7 +164,7 @@ For each repository and kind of Pass whose latest Activity log line is a skip, s Architect run idle on acme/widgets: Ready issue #7 "Make widgets" goes first: https://github.com/acme/widgets/issues/7 ``` -`status` and the Console Inbox also show `Pickup last fired ago on /` (and the equivalent for Architect and Security), even when repeated Passes skip for the same reason. Every Pass saves its start once its repository is known, before launch checks and the Command limit, in `///.last-fired`. This replaces the previous time without adding Activity log lines. A repository with no saved start has no last-fired entry; older Activity log times are not used as evidence that the scheduler recently fired. +`status` and the Console Inbox also show `Pickup last fired ago on /` (and the equivalent for Architect and Security), even when repeated Passes skip for the same reason. Beside those lines, `GitHub Quota wait until