From 7851d8b9bdfaf8f2c4ca62d3a91f391ff2fce818 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 10 Nov 2026 16:33:46 -0500 Subject: [PATCH 1/9] Keep scheduled Passes clear of live Experiments (#1494) --- GLOSSARY.md | 4 +- console/tests/experiments.test.cjs | 26 +++ docs/experiment-results.md | 21 +- docs/guide.md | 27 ++- site/architecture/index.html | 1 + site/docs/index.html | 24 ++- site/how-it-works/index.html | 3 +- site/index.html | 1 + src/command_limit.rs | 31 ++- src/console/experiments/render.rs | 22 +- src/console/openapi.rs | 4 +- src/console/start/experiment.rs | 4 +- src/evals/experiment.rs | 8 +- src/evals/experiment/coordination.rs | 51 +++++ src/evals/experiment/options.rs | 11 +- src/evals/experiment/reading.rs | 2 +- src/evals/experiment/store.rs | 27 +++ src/host_work.rs | 33 +++ src/logs.rs | 14 ++ src/logs/command_record.rs | 4 + src/logs/live.rs | 40 ++++ src/logs/reading.rs | 1 + src/logs/reading/inbox.rs | 2 +- src/logs/recording.rs | 11 + src/logs/status.rs | 8 +- src/main.rs | 4 + tests/console/start.rs | 2 +- tests/experiments.rs | 6 + tests/experiments/host.rs | 293 +++++++++++++++++++++++++++ tests/experiments/result_format.rs | 4 +- tests/support/mod.rs | 12 ++ 31 files changed, 675 insertions(+), 26 deletions(-) create mode 100644 src/evals/experiment/coordination.rs create mode 100644 src/host_work.rs create mode 100644 src/logs/live.rs create mode 100644 tests/experiments/host.rs diff --git a/GLOSSARY.md b/GLOSSARY.md index c68d7d86b..ff909571c 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -49,6 +49,7 @@ _Avoid_: Claim limit (former name), WIP limit, concurrency (it counts issues wai **Command limit**: The number of live **Commands** on one repository on one machine at which a **Pass** there is skipped, set in the **User config**, one by default. Every live Command counts, a Ticket run started by hand on an **Issue URL** included, together with the Spec run or Ticket run each Pass dispatched, but only a Pass is ever skipped for it: a Command a person types always runs. Passes of different kinds may run side by side up to the limit, but never two **Architect runs** or two **Security runs** on one repository at once. It counts no other repository; load across a machine is the **User config**'s memory and load waits. +An Experiment independently holds back every Pass of its OS account across the machine's repositories, including while it waits for live Runs to end. _Avoid_: Pass lock (former, one-wide form), concurrency limit, parallel **Sweep**: @@ -145,6 +146,7 @@ _Avoid_: intake **Pass**: An **Architect run**, a **Pickup run**, a **Security run**, a **Review run** started with `--author`, a **Fix run** started with no pull request, a **Triage run** started with neither a **Report** nor an **Issue URL**, or a **Custom process** started with no issue and neither an Issue-branch nor a Spec-branch worktree: a **Command** started with no **Issue URL**, which decides before any work whether it is skipped. Such a Custom process is skipped when the repository is at its **Command limit** on the machine, and it does not take an Architect run's Ready-issue or Architect-idea gates; a file declaring either branch worktree needs an issue, and a Custom process started with an issue is not a Pass. +Every Pass is skipped while an Experiment is live on its machine for the same OS account, including one still waiting for live Runs to end. A Command started on an Issue URL still runs, with a warning that it competes with the Experiment's Trials. _Avoid_: tick, cycle **Process**: @@ -422,7 +424,7 @@ What a **Grader** decides about one **Trial** against its **Eval task**'s succes _Avoid_: grade, rating, feedback **Experiment**: -The **Command** that compares a **Baseline arm** with a **Candidate arm** on the **Eval suites** a change touches, against a **Bar** fixed before it starts, and ends in a **Verdict**. It shows whether the change makes the agents better or worse before it reaches production. +The **Command** that compares a **Baseline arm** with a **Candidate arm** on the **Eval suites** a change touches, against a **Bar** fixed before it starts, and ends in a **Verdict**. It waits for the same OS account's other live Commands on its machine before starting Trials, including on resume; the Day shift can bypass the wait, with the number of competing Commands recorded beside the Verdict. _Avoid_: replay, eval run, benchmark run, A/B test **Bar**: diff --git a/console/tests/experiments.test.cjs b/console/tests/experiments.test.cjs index 0f4cca443..472ad7ee6 100644 --- a/console/tests/experiments.test.cjs +++ b/console/tests/experiments.test.cjs @@ -9,6 +9,32 @@ const { createInterface } = require('node:readline'); const { chromium } = require('./playwright.cjs'); const fixture = require('./fixtures/experiments.cjs'); +test('host waits and bypassed Command counts appear on Experiment pages and lists', async t => { + t.after(() => fixture.save(home, fixture.record())); + for (const viewport of [{ width: 1440, height: 900 }, { width: 390, height: 844 }]) { + const context = await browser.newContext({ viewport }); + try { + const page = await context.newPage(); + const waiting = fixture.record(); + waiting.waiting_for = ['Ticket run acme/widgets#7 \n`); + owner = createServer((request, response) => { + const value = request.url.endsWith('/whoami') + ? { version: 1, api_version: 'v1', hostname: 'owner', thirdshift_version: 'fixture', scopes: ['read'] } + : request.url.endsWith('/cronjobs') + ? { jobs: [{ repository: 'acme/widgets', command_kind: 'Security run', schedule: '@daily', command: remoteCommand }], pass_starts: [] } + : { version: 2, hostname: 'owner', active_runs: 0, commands: [], pass_starts: [], machines: [] }; + response.setHeader('Content-Type', 'application/json'); + response.end(JSON.stringify(value)); + }); + await new Promise(resolve => owner.listen(0, '127.0.0.1', resolve)); + const pairing = spawn(process.env.THIRDSHIFT_BINARY ?? resolve(__dirname, '../../target/debug/thirdshift'), ['console', 'fleet', 'add', remoteName, `http://127.0.0.1:${owner.address().port}`], { + env: { ...process.env, HOME: home }, stdio: ['pipe', 'ignore', 'inherit'] + }); + const paired = once(pairing, 'exit'); + pairing.stdin.end('fixture-token\n'); + assert.equal((await paired)[0], 0); server = spawn(process.env.THIRDSHIFT_BINARY ?? resolve(__dirname, '../../target/debug/thirdshift'), ['console', '--no-sign-in'], { env: { ...process.env, HOME: home, PATH: `${join(home, 'bin')}:${process.env.PATH}` }, stdio: ['ignore', 'pipe', 'inherit'] @@ -38,9 +57,42 @@ after(async () => { await browser?.close(); if (server && server.exitCode === null) server.kill(); await exited; + if (owner) await new Promise(resolve => owner.close(resolve)); if (home) await rm(home, { recursive: true, force: true }); }); +test('Fleet schedules show repository, machine and Command kind on desktop and phone', async t => { + for (const javaScriptEnabled of [true, false]) { + for (const width of [1440, 390]) { + await t.test(`${width}px, JavaScript ${javaScriptEnabled}`, async t => { + const context = await browser.newContext({ javaScriptEnabled, viewport: { width, height: width === 390 ? 844 : 900 } }); + t.after(() => context.close()); + const page = await context.newPage(); + await page.goto(`${origin}/cronjobs`); + await page.getByRole('link', { name: 'Fleet schedules', exact: true }).click(); + await page.getByRole('heading', { name: 'Fleet schedules', exact: true }).waitFor(); + assert.deepEqual(await page.locator('main h2').allTextContents(), ['Machines', 'acme/tools', 'acme/widgets', 'Other']); + const widgets = page.getByRole('region', { name: 'acme/widgets Fleet schedules', exact: true }); + assert.equal(await widgets.locator('h3').count(), 2); + await widgets.getByRole('heading', { name: remoteName, exact: true }).waitFor(); + assert.match(await widgets.textContent(), /Pickup run/); + const commands = await widgets.locator('.cron-command').allTextContents(); + assert.ok(commands.includes(widgetsCommand)); + assert.ok(commands.includes(remoteCommand)); + const other = page.getByRole('region', { name: 'Other Fleet schedules', exact: true }); + assert.match(await other.textContent(), /Security run/); + assert.match(await other.textContent(), / {% include "console/header.html" %}{% include "console/nav.html" %}

Cron jobs

thirdshift cronjobs --json
+{% if page.can_read() %}

This machine's Cron jobs · Fleet schedules

{% endif %} {% include "console/machine_controls.html" %} {% if !machine.is_empty() %}

Machine: {{ machine }}

{% endif %}

thirdshift entries in this account's crontab (crontab -l). Schedules use cron's time zone. Reload to see changes.

diff --git a/src/console/cronjobs.rs b/src/console/cronjobs.rs index 7ac91a0ec..5229634d2 100644 --- a/src/console/cronjobs.rs +++ b/src/console/cronjobs.rs @@ -1,5 +1,6 @@ //! Cron job inventory with confirmed starts through the Console API. use askama::Template; +use axum::http::StatusCode; use axum::response::Response; use super::filters; @@ -21,15 +22,47 @@ struct CronJobsPage { targets: Vec, } +#[derive(Template)] +#[template(path = "console/cronjobs_fleet.html")] +struct FleetPage { + page: super::PageContext, + inventory: cronjobs::fleet::Inventory, +} + pub(super) async fn show( axum::Extension(viewer): axum::Extension, axum::Extension(format): axum::Extension, axum::extract::RawQuery(query): axum::extract::RawQuery, ) -> Response { + let fleet = super::query_value(&query, "fleet"); + if format == super::api::Format::Json && !fleet.is_empty() { + return super::api::error( + StatusCode::BAD_REQUEST, + "Fleet schedules are an explicit client view; call each owning machine's API directly", + ); + } + if fleet == "true" && !viewer.scopes.contains(&super::Scope::Read) { + return super::api::error(StatusCode::FORBIDDEN, "Fleet schedules need read scope"); + } super::api::read(format, move || { let selection = super::fleet_control::Selection::query(&query); let config = UserConfig::load()?; + if fleet == "true" { + let inventory = cronjobs::fleet::load(&config, viewer.fleet_person()); + return format.render(inventory, |inventory| { + Ok(FleetPage { + page: super::PageContext::new(&config, &viewer.scopes), + inventory, + } + .render()?) + }); + } let remote = selection.remote(&config, &viewer, format)?; + let captured = if remote.is_none() { + Some(crate::launch_directories::Captured::capture(&config)?) + } else { + None + }; let mut remote_passes = None; // Run lists startable Passes. The crontab and last-fired times stay behind read. let (inventory, show_jobs) = if let Some(remote) = &remote { @@ -53,7 +86,7 @@ pub(super) async fn show( ) } } else { - (cronjobs::load()?, true) + (captured.as_ref().unwrap().cronjobs()?.clone(), true) }; format.render(inventory, |inventory| { let can_run = remote_passes.is_some() @@ -61,7 +94,7 @@ pub(super) async fn show( let (passes, start_error) = if let Some(passes) = remote_passes { (passes, None) } else if remote.is_none() && can_run { - match super::start_controls::passes(&config, &inventory.jobs) { + match super::start_controls::cron_passes(&config, captured.as_ref().unwrap()) { Ok(passes) => (passes, None), Err(error) => (Vec::new(), Some(error.to_string())), } diff --git a/src/console/cronjobs_fleet.html b/src/console/cronjobs_fleet.html new file mode 100644 index 000000000..49417add0 --- /dev/null +++ b/src/console/cronjobs_fleet.html @@ -0,0 +1,18 @@ + + +Fleet schedules · {{ page.hostname }} · thirdshift +{% include "console/header.html" %}{% include "console/nav.html" %} +

Fleet schedules

thirdshift cronjobs --fleet --json
+

This machine's Cron jobs · Fleet schedules

+

Configured thirdshift entries in the serving account's crontab on each Fleet machine. Schedules keep each machine's cron time zone; no conversion is applied. Reload to see changes.

+

Other accounts, system crontabs and other schedulers are outside this view. Use each machine's Cron jobs for Run now and observed Pass starts.

+

Machines

+
    {% for machine in inventory.machines %}
  • {{ machine.name }}: {{ machine.status }}{% if let Some(address) = machine.address %} · Open Console{% endif %}{% if machine.empty %} · No thirdshift cron jobs in this account's crontab.{% endif %}{% if let Some(error) = machine.error %}

    {{ error }}

    {% endif %}
    • {% endfor %}
+
+{% for repository in inventory.repositories %}
+

{{ repository.heading() }}

+{% for machine in repository.machines %}

{{ machine.name }}

+{% for job in machine.jobs %}

{% if let Some(kind) = job.command_kind %}{{ kind }}{% else %}Unknown Command kind{% endif %}

+
{{ job.schedule }}  {{ job.command|one_line }}
+{% endfor %}{% endfor %}
{% endfor %} +
{% include "console/chrome.html" %} diff --git a/src/console/fleet_control.rs b/src/console/fleet_control.rs index 49abafb6b..155f6bc2f 100644 --- a/src/console/fleet_control.rs +++ b/src/console/fleet_control.rs @@ -59,18 +59,12 @@ fn local_machine(config: &UserConfig, name: &str) -> bool { && !config.console.fleet.iter().any(|entry| entry.name == name)) } -pub(super) fn local_name(config: &UserConfig) -> String { - let hostname = super::hostname(); - if config - .console - .fleet - .iter() - .any(|entry| entry.name == hostname) - { - format!("{hostname} (local)") - } else { - hostname +pub(crate) fn local_name(config: &UserConfig) -> String { + let mut name = super::hostname(); + while config.console.fleet.iter().any(|entry| entry.name == name) { + name.push_str(" (local)"); } + name } pub(super) struct Target { diff --git a/src/console/inbox.html b/src/console/inbox.html index d7812cb22..bca1cc2ef 100644 --- a/src/console/inbox.html +++ b/src/console/inbox.html @@ -63,6 +63,7 @@

{% if machines.len() > 1 %}No Commands are running in this Fleet.{% else %}N {% endif %} {% endif %} + {% match inbox.github_quota_wait %}{% when Some with (wait) %}

{{ wait }}

{% when None %}{% endmatch %} {% for section in inbox.sections %}

{{ section.title }} {{ section.rows.len() }}

 diff --git a/src/console/openapi.rs b/src/console/openapi.rs index 157433fff..055786521 100644 --- a/src/console/openapi.rs +++ b/src/console/openapi.rs @@ -234,7 +234,8 @@ fn operation(route: &PageRoute) -> (Vec, Value) { })), View::CronJobs => object(json!({ "jobs": array(object(json!({ - "schedule": text, "command": text, "repository": {"type": ["string", "null"]} + "schedule": text, "command": text, "repository": {"type": ["string", "null"]}, + "command_kind": {"type": ["string", "null"], "description": "Conservative owning-machine Command recognition; null when unknown"} }))), "pass_starts": array(record) })), View::LaunchDirectories => object(json!({ diff --git a/src/console/parity.rs b/src/console/parity.rs index 65277a9ee..5ed34fc2d 100644 --- a/src/console/parity.rs +++ b/src/console/parity.rs @@ -97,6 +97,7 @@ const FLEET: &[(&str, &str)] = &[ // Distinct actions represented by fields of a CLI command, rather than enums. const ACTIONS: &[&str] = &[ + "cronjobs --fleet", "update --check", "config spec.parallel", "config spec.parallel 3", @@ -118,6 +119,10 @@ const ACTIONS: &[&str] = &[ // Keep these reasons and ADR 0026's CLI-only list together. A prefix is never // an exclusion: each entry is parsed and covers only its command/action. const EXCLUSIONS: &[(&str, &str)] = &[ + ( + "cronjobs --fleet", + "Fleet schedule aggregation is an explicit client of owning-machine inventories; the API stays local and cannot lend saved Fleet credentials.", + ), ( "console token list", "Token management stays in the shell so the API cannot widen its own access.", @@ -212,6 +217,7 @@ fn identity(words: &str) -> (Discriminant, String) { Command::ConsoleFleet(action) => format!("{:?}", discriminant(action)), Command::Update { check, .. } => check.to_string(), Command::Config { key, value, .. } => format!("{}:{}", key.is_some(), value.is_some()), + Command::CronJobs { fleet, .. } => fleet.to_string(), Command::ConsoleOtherAccounts { value, .. } => value.is_some().to_string(), Command::ConsoleTheme { name, .. } => name.is_some().to_string(), Command::ConsoleSections { hide, show, .. } => { diff --git a/src/console/row.html b/src/console/row.html index b68799597..1da3bcc59 100644 --- a/src/console/row.html +++ b/src/console/row.html @@ -1,6 +1,10 @@
  • {% if !row.machine.is_empty() %}{% if row.console.is_empty() %}{{ row.machine }}{% else %}{{ row.machine }}{% endif %}{% endif %} -
    {% if row.live %}{% endif %}{{ row.state }} {% match row.account %}{% when Some with (account) %} {% when None %}{% endmatch %}{% match row.subject_url %}{% when Some with (url) %}{% match row.subject.rsplit_once('#') %}{% when Some with ((repository, number)) %}{{ repository }}#{{ number }}{% when None %}{{ row.subject }}{% endmatch %}{% when None %}{% match row.subject.rsplit_once(" on ") %}{% when Some with ((command, repository)) %}{{ repository }} {{ command }}{% when None %}{{ row.subject }}{% endmatch %}{% endmatch %} {{ row.title }}{{ self.gist()|readable }}{% match row.timing.finish %}{% when Some with (finish) %} · {% when None %}{% endmatch %}{{ row.timing.age }} +
    {% if row.live %}{% endif %}{{ row.state }} {% match row.account %}{% when Some with (account) %} {% when None %}{% endmatch %}{% match row.subject_url %}{% when Some with (url) %}{% match row.subject.rsplit_once('#') %}{% when Some with ((repository, number)) %}{{ repository }}#{{ number }}{% when None %}{{ row.subject }}{% endmatch %}{% when None %}{% match row.subject.rsplit_once(" on ") %}{% when Some with ((command, repository)) %}{{ repository }} {{ command }}{% when None %}{{ row.subject }}{% endmatch %}{% endmatch %} + {%- match row.base_branch %}{% when Some with (base) %}Base branch {{ base }}{% when None %}{% endmatch %} + {%- match row.target_pr %}{% when Some with (pr) %}PR #{{ pr.issue.number }}{% when None %}{% endmatch -%} + {{ row.title }}{{ self.gist()|readable }}{% match row.timing.finish %}{% when Some with (finish) %} · {% when None %}{% endmatch %}{{ row.timing.age }} + {% match row.ticket_progress %}{% when Some with (progress) %}{{ progress.complete }} / {{ progress.total }} Tickets complete{% when None %}{% endmatch %} {% match row.ci %}{% when Some with (ci) %}{{ ci.label() }}{% if row.merge_conflict %}Merge conflict{% endif %}{% when None %}{% endmatch %} {% if can_copy || (can_act && (row.can_dismiss || row.remote_stop || row.remote_undo)) %}
    {% if can_copy %} {% endif %} {% if can_act && !row.console.is_empty() && row.remote_stop %}
    {% endif %} diff --git a/src/console/start_controls.rs b/src/console/start_controls.rs index 72ca1cf5b..6aef56e0e 100644 --- a/src/console/start_controls.rs +++ b/src/console/start_controls.rs @@ -55,6 +55,16 @@ impl Form { } pub fn load(config: &UserConfig) -> Self { + match crate::launch_directories::Captured::capture(config) { + Ok(captured) => Self::from_directories(config, captured.directories()), + Err(error) => Self::from_directories(config, Err(error)), + } + } + + fn from_directories( + config: &UserConfig, + directories: Result<&crate::launch_directories::Inventory>, + ) -> Self { let mut form = Self { repositories: Vec::new(), harness: config @@ -69,12 +79,12 @@ impl Form { .collect(), error: None, }; - match crate::launch_directories::load() { + match directories { Ok(inventory) => { let mut repositories: Vec<_> = inventory .directories - .into_iter() - .map(|d| d.repository) + .iter() + .map(|d| d.repository.clone()) .collect(); repositories.sort(); repositories.dedup(); @@ -149,20 +159,10 @@ pub(super) struct Options { impl Options { pub fn load(config: &UserConfig) -> Result { - let jobs = match crate::cronjobs::jobs() { - Ok(jobs) => jobs, - Err(error) - if error - .downcast_ref::() - .is_some_and(|error| error.kind() == std::io::ErrorKind::NotFound) => - { - Vec::new() - } - Err(error) => return Err(error), - }; + let captured = crate::launch_directories::Captured::capture(config)?; Ok(Self { - form: Form::load(config), - passes: passes(config, &jobs)?, + form: Form::from_directories(config, captured.directories()), + passes: passes(config, &captured)?, }) } } @@ -176,18 +176,41 @@ pub(super) async fn options() -> axum::response::Response { } /// Preserve the cron Pass's CLI options, while the API still decides admission. -pub(super) fn passes(config: &UserConfig, jobs: &[crate::cronjobs::CronJob]) -> Result> { - let directories = crate::launch_directories::load()?; +pub(super) fn passes( + config: &UserConfig, + captured: &crate::launch_directories::Captured, +) -> Result> { + Ok(pass_choices(config, captured)? + .into_iter() + .map(|(_, pass)| pass) + .collect()) +} + +/// The local Cron jobs page has always presented controls in repository order. +pub(super) fn cron_passes( + config: &UserConfig, + captured: &crate::launch_directories::Captured, +) -> Result> { + let mut choices = pass_choices(config, captured)?; + choices.sort_by_key(|(repository, _)| *repository); + Ok(choices.into_iter().map(|(_, pass)| pass).collect()) +} + +fn pass_choices<'a>( + config: &UserConfig, + captured: &'a crate::launch_directories::Captured, +) -> Result> { + captured.directories()?; let mut passes = Vec::new(); - for job in jobs { - let Some((path, arguments)) = crate::launch_directories::cron_arguments(&job.command) - else { + for entry in captured.entries()? { + let Some(directory) = &entry.directory else { continue; }; - let Some(directory) = directories.directories.iter().find(|d| d.path == path) else { - continue; - }; - let Ok(parsed) = args::parse(&arguments) else { + let Some(parsed) = entry + .launch + .as_ref() + .and_then(|launch| launch.command.as_ref()) + else { continue; }; let (mut request, flags, security) = match parsed { @@ -196,36 +219,36 @@ pub(super) fn passes(config: &UserConfig, jobs: &[crate::cronjobs::CronJob]) -> crate::ready::PickupOrder::LowestFirst => "lowest-first", crate::ready::PickupOrder::HighestFirst => "highest-first", })}), - pass.flags, + pass.flags.clone(), false, ), args::Command::Secure(pass) => ( json!({"command":"secure", "base":pass.base}), - pass.flags, + pass.flags.clone(), true, ), args::Command::Architect(pass) => ( json!({"command":"architect", "base":pass.base, "focus":pass.focus, "plan_only":pass.plan_only}), - pass.flags, + pass.flags.clone(), false, ), args::Command::Review(review) => { - let crate::review::Target::Author(author) = review.target else { + let crate::review::Target::Author(author) = &review.target else { continue; }; ( json!({"command":"review", "author":author, "recommend_only":review.mode == crate::review::Mode::RecommendOnly}), - review.flags, + review.flags.clone(), false, ) } args::Command::Fix(fix) => { - let crate::fix::Target::Pass { base, author } = fix.target else { + let crate::fix::Target::Pass { base, author } = &fix.target else { continue; }; ( json!({"command":"fix", "base":base, "author":author}), - fix.flags, + fix.flags.clone(), false, ) } @@ -236,8 +259,8 @@ pub(super) fn passes(config: &UserConfig, jobs: &[crate::cronjobs::CronJob]) -> json!({"command":"process", "name":process.name, "plan_only":process.plan_only}), Flags { goal: process.goal, - email: process.email, - harness: process.asked, + email: process.email.clone(), + harness: process.asked.clone(), security_review: process.security_review, security_fix: process.security_fix, security_close: process.security_close, @@ -311,11 +334,14 @@ pub(super) fn passes(config: &UserConfig, jobs: &[crate::cronjobs::CronJob]) -> } else { flags.harness(config) }; - passes.push(Pass { - command: job.command.clone(), - request: serde_json::to_string(&request)?, - harness: harness.harness.name().into(), - }); + passes.push(( + directory.repository.as_str(), + Pass { + command: entry.job.command.clone(), + request: serde_json::to_string(&request)?, + harness: harness.harness.name().into(), + }, + )); } Ok(passes) } diff --git a/src/cronjobs.rs b/src/cronjobs.rs index 0f9ffcced..7e51c3af6 100644 --- a/src/cronjobs.rs +++ b/src/cronjobs.rs @@ -7,7 +7,11 @@ use serde::{Deserialize, Serialize}; use crate::config::UserConfig; use crate::logs::pass_starts::{self, PassStart}; -#[derive(Serialize, Deserialize)] +mod command; +pub(crate) mod fleet; +pub(crate) use command::{Interpretation, LaunchFacts}; + +#[derive(Clone, Serialize, Deserialize)] pub(crate) struct CronJobs { pub jobs: Vec, pub pass_starts: Vec, @@ -31,7 +35,7 @@ impl CronJobs { } } -#[derive(Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct CronJob { pub schedule: String, pub command: String, @@ -40,6 +44,65 @@ pub struct CronJob { /// heading would add a line to the schedule listing. #[serde(default, deserialize_with = "deserialize_repository")] pub repository: Option, + /// Owning-machine recognition only; absent on older inventories. + #[serde(default, deserialize_with = "deserialize_command_kind")] + pub command_kind: Option, +} + +#[derive(Clone, Copy, Serialize, Deserialize)] +pub enum CommandKind { + #[serde(rename = "Pickup run")] + Pickup, + #[serde(rename = "Security run")] + Security, + #[serde(rename = "Architect run")] + Architect, + #[serde(rename = "Triage run")] + Triage, + #[serde(rename = "Fix run")] + Fix, + #[serde(rename = "Review run")] + Review, + #[serde(rename = "Custom process")] + Process, +} + +impl std::fmt::Display for CommandKind { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let name = match self { + Self::Pickup => "Pickup run", + Self::Security => "Security run", + Self::Architect => "Architect run", + Self::Triage => "Triage run", + Self::Fix => "Fix run", + Self::Review => "Review run", + Self::Process => "Custom process", + }; + f.write_str(name) + } +} + +fn deserialize_command_kind<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + // A future owner's Command kind must not hide an otherwise readable entry. + let value = serde_json::Value::deserialize(deserializer)?; + Ok(serde_json::from_value(value).unwrap_or(None)) +} + +/// One Fleet report field. A control character or Unicode line separator would +/// start another line or a terminal control sequence, so one machine's schedule +/// or error could look like another machine's entry. JSON keeps the original text. +pub(crate) fn report_line(text: &str) -> String { + text.chars() + .map(|character| match character { + '\n' | '\u{2028}' | '\u{2029}' => '\u{240A}', + '\r' => '\u{240D}', + character if character.is_control() => '\u{FFFD}', + character => character, + }) + .collect() } fn repository_label(repository: Option) -> Option { @@ -61,13 +124,14 @@ where pub(crate) fn load() -> Result { let config = UserConfig::load()?; - let home = crate::config::home()?; - let mut jobs = jobs()?; + Ok(crate::launch_directories::Captured::capture(&config)? + .cronjobs()? + .clone()) +} + +pub(crate) fn inventory(mut jobs: Vec, logs_dir: &std::path::Path) -> Result { for job in &mut jobs { - job.repository = repository_label( - crate::launch_directories::from_cron(&job.command, &home) - .map(|directory| directory.repository), - ); + job.repository = repository_label(job.repository.take()); } // Stable sorting retains crontab order and duplicates within each repository. jobs.sort_by(|a, b| { @@ -75,11 +139,16 @@ pub(crate) fn load() -> Result { }); Ok(CronJobs { jobs, - pass_starts: pass_starts::load(&config.logs_dir)?, + pass_starts: pass_starts::load(logs_dir)?, }) } -pub(crate) fn jobs() -> Result> { +pub(crate) struct InterpretedEntry { + pub job: CronJob, + pub interpretation: Interpretation, +} + +pub(crate) fn entries() -> Result> { let output = Command::new("crontab") .arg("-l") .env("LC_ALL", "C") @@ -101,7 +170,11 @@ pub(crate) fn jobs() -> Result> { ); } let crontab = String::from_utf8(output.stdout).context("crontab is not UTF-8")?; - Ok(crontab.lines().filter_map(entry).collect()) + Ok(parse(&crontab)) +} + +pub(crate) fn parse(crontab: &str) -> Vec { + crontab.lines().filter_map(entry).collect() } pub fn show(json: bool) -> Result { @@ -129,7 +202,7 @@ pub fn show(json: bool) -> Result { Ok(lines.join("\n")) } -fn entry(line: &str) -> Option { +fn entry(line: &str) -> Option { let mut command = line.trim(); // User crontabs have five schedule fields, or one @-schedule field. // Requiring a schedule excludes comments and environment assignments. @@ -146,31 +219,18 @@ fn entry(line: &str) -> Option { schedule.push(&command[..end]); command = command[end..].trim_start(); } - // Keep stdin text for display, but don't match it as a command. - let mentions_thirdshift = shell_command(command) - .split(|c: char| c.is_whitespace() || "'\";&|()<>".contains(c)) - .any(|word| word.rsplit('/').next() == Some("thirdshift")); - mentions_thirdshift.then(|| CronJob { - schedule: schedule.join(" "), - command: command.into(), - repository: None, + let interpreted = Interpretation::new(command); + interpreted.recognized().then(|| InterpretedEntry { + job: CronJob { + schedule: schedule.join(" "), + command: command.into(), + repository: None, + command_kind: interpreted.command_kind(), + }, + interpretation: interpreted, }) } -/// Cron sends everything after its first unescaped % to stdin, even in quotes. -pub(crate) fn shell_command(command: &str) -> &str { - let mut escaped = false; - let command_end = command - .char_indices() - .find_map(|(index, character)| { - let separator = character == '%' && !escaped; - escaped = character == '\\' && !escaped; - separator.then_some(index) - }) - .unwrap_or(command.len()); - &command[..command_end] -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/cronjobs/command.rs b/src/cronjobs/command.rs new file mode 100644 index 000000000..c0a1d795f --- /dev/null +++ b/src/cronjobs/command.rs @@ -0,0 +1,533 @@ +//! Literal Cron job interpretation, without filesystem reads or shell execution. +use std::path::{Path, PathBuf}; + +use crate::{args, config}; + +use super::CommandKind; + +/// Independent inventory and Launch projections of one command, tokenized once. +pub(crate) struct Interpretation { + recognized: bool, + kind: Option, + launch: Option, +} + +struct LiteralLaunch { + directory: String, + output_log: Option, + command: Option, +} + +pub(crate) struct LaunchFacts { + pub directory: PathBuf, + pub output_log: Option, + /// A parsed candidate; Console still decides which Commands it can start. + pub command: Option, +} + +impl Interpretation { + pub(crate) fn new(command: &str) -> Self { + let command = shell_command(command); + // Inclusion is lexical, so failed shell/CLI parsing cannot hide an entry. + let recognized = command + .split(|c: char| c.is_whitespace() || "'\";&|()<>".contains(c)) + .any(|word| word.rsplit('/').next() == Some("thirdshift")); + let Some(words) = shell_words(command) else { + return Self { + recognized, + kind: None, + launch: None, + }; + }; + let initial = initial_directory(&words); + let invocation = initial.map_or(words.as_slice(), |(_, rest)| rest); + let launch = initial + .and_then(|(path, rest)| launch_invocation(rest).map(|selected| (path, selected))); + let selected = launch.map_or(invocation, |(_, selected)| selected); + let parsed = selected + .split_first() + .and_then(|(_, arguments)| literal_cli_arguments(arguments)) + .and_then(|arguments| args::parse(&arguments).ok()); + let kind = if selected == invocation && is_plain_invocation(invocation) { + parsed.as_ref().and_then(command_kind) + } else { + None + }; + let launch = launch.map(|(directory, selected)| LiteralLaunch { + directory: directory.into(), + output_log: selected + .windows(2) + .find(|pair| matches!(pair[0].as_str(), ">>" | ">" | "1>" | "1>>" | "&>" | "&>>")) + .map(|pair| pair[1].clone()) + .filter(|log| !log.contains(['$', '`'])), + command: parsed, + }); + Self { + recognized, + kind, + launch, + } + } + + pub(crate) fn recognized(&self) -> bool { + self.recognized + } + + pub(crate) fn command_kind(&self) -> Option { + self.kind + } + + /// Expand only literal paths, using the requesting machine's supplied home. + pub(crate) fn into_launch(self, home: &Path) -> Option { + let launch = self.launch?; + let directory = config::expand_home(&launch.directory, home)?; + let output_log = launch + .output_log + .map(|log| config::expand_home(&log, home).unwrap_or_else(|| directory.join(log))); + Some(LaunchFacts { + directory, + output_log, + command: launch.command, + }) + } +} + +fn initial_directory(words: &[String]) -> Option<(&str, &[String])> { + let [cd, path, and, rest @ ..] = words else { + return None; + }; + (cd == "cd" && and == "&&" && !path.contains(['$', '`'])).then_some((path.as_str(), rest)) +} + +fn is_plain_invocation(words: &[String]) -> bool { + let Some((executable, arguments)) = words.split_first() else { + return false; + }; + !executable.contains(['$', '`']) + && Path::new(executable) + .file_name() + .is_some_and(|name| name == "thirdshift") + && !arguments + .iter() + .any(|word| matches!(word.as_str(), "&&" | "||" | ";" | "|" | "&")) +} + +#[expect( + clippy::wildcard_enum_match_arm, + reason = "inventory recognizes work Commands only; inspection and administration commands have unknown kind" +)] +fn command_kind(command: &args::Command) -> Option { + use args::Command as Parsed; + Some(match command { + Parsed::Pickup(_) => CommandKind::Pickup, + Parsed::Secure(_) => CommandKind::Security, + Parsed::Architect(_) => CommandKind::Architect, + Parsed::Triage(_) => CommandKind::Triage, + Parsed::Fix(_) => CommandKind::Fix, + Parsed::Review(_) => CommandKind::Review, + Parsed::Process(args::ProcessAction::Run(_)) => CommandKind::Process, + // An Issue URL can select a Spec run or Ticket run; labels are not read here. + _ => return None, + }) +} + +fn launch_invocation(rest: &[String]) -> Option<&[String]> { + rest.split(|word| matches!(word.as_str(), "&&" | ";" | "||")) + // A later directory change makes the initial cd unreliable. Leave + // these compounds display-only rather than infer the wrong checkout. + .take_while(|part| { + let executable = part.iter().map(String::as_str).find(|word| { + !word.contains('=') && !matches!(*word, "command" | "builtin" | "--" | "-p" | "!") + }); + !matches!( + executable, + Some("cd" | "pushd" | "popd" | "eval" | "source" | ".") + ) + }) + .find(|part| { + let executable = if part.first().is_some_and(|word| { + Path::new(word) + .file_name() + .is_some_and(|name| name == "env") + }) { + // Retain launcher words in the candidate: starting it must not + // silently discard the environment used by the Cron job. + part.iter().skip(1).find(|word| !word.contains('=')) + } else { + part.first() + }; + executable.is_some_and(|word| { + Path::new(word) + .file_name() + .is_some_and(|name| name == "thirdshift") + }) + }) +} + +/// Cron sends everything after its first unescaped % to stdin, even in quotes. +fn shell_command(command: &str) -> &str { + let mut escaped = false; + let command_end = command + .char_indices() + .find_map(|(index, character)| { + let separator = character == '%' && !escaped; + escaped = character == '\\' && !escaped; + separator.then_some(index) + }) + .unwrap_or(command.len()); + &command[..command_end] +} + +// Keep literal CLI words before shell redirects; expansions remain unknown. +fn literal_cli_arguments(words: &[String]) -> Option> { + let arguments: Vec<_> = words + .iter() + .take_while(|word| { + let operator = word.trim_start_matches(|c: char| c.is_ascii_digit()); + !matches!( + operator, + ">" | ">>" | "&>" | "&>>" | "<" | ">&" | "<&" | "#" + ) + }) + .cloned() + .collect(); + if arguments.iter().any(|word| word.contains(['$', '`'])) { + return None; + } + Some(arguments) +} + +// shlex splits shell words, but leaves operators attached to those words. +// Separate unquoted operators first, preserving quotes and escaped characters. +fn shell_words(command: &str) -> Option> { + let mut spaced = String::new(); + let mut quote = None; + let mut escaped = false; + let mut chars = command.chars().peekable(); + while let Some(character) = chars.next() { + if escaped { + spaced.push(character); + escaped = false; + continue; + } + if character == '\\' && quote != Some('\'') { + spaced.push(character); + escaped = true; + continue; + } + if quote == Some(character) { + quote = None; + } else if quote.is_none() && matches!(character, '\'' | '"') { + quote = Some(character); + } else if quote.is_none() && "&|;<>".contains(character) { + // An attached IO number belongs to its redirect; standalone + // numbers remain CLI arguments (parallel 2 >> log). + let word_start = spaced.rfind(char::is_whitespace).map_or(0, |at| at + 1); + let word = &spaced[word_start..]; + let io_number = matches!(character, '<' | '>') + && !word.is_empty() + && word.bytes().all(|byte| byte.is_ascii_digit()); + if !io_number { + spaced.push(' '); + } + spaced.push(character); + if chars.peek() == Some(&character) + || matches!(character, '>' | '<') && chars.peek() == Some(&'&') + { + spaced.push(chars.next()?); + } else if character == '&' && chars.peek() == Some(&'>') { + spaced.push(chars.next()?); + if chars.peek() == Some(&'>') { + spaced.push(chars.next()?); + } + } + spaced.push(' '); + continue; + } + spaced.push(character); + } + shlex::split(&spaced) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn directory_changing_launch_forms_leave_later_commands_unresolved() { + for change in [ + "cd /other", + "pushd /other", + "popd", + "command cd /other", + "command -p -- cd /other", + "builtin cd /other", + "CDPATH=.. cd other", + "! cd /other", + "eval 'cd /other'", + "source /change-directory.sh", + ". /change-directory.sh", + ] { + let text = format!("cd /repo && {change} && thirdshift pickup"); + let interpreted = Interpretation::new(&text); + assert!(interpreted.recognized(), "{text}"); + assert!(interpreted.command_kind().is_none(), "{text}"); + assert!( + interpreted.into_launch(Path::new("/home/test")).is_none(), + "{text}" + ); + } + } + + #[test] + fn literal_launch_facts_preserve_cron_separators_paths_and_stdout_logs() { + let home = Path::new("/home/test"); + for (text, directory, log, kind, candidate) in [ + ( + "cd /repo&&thirdshift pickup parallel 2>>cron.log 2>&1", + Some("/repo"), + None, + None, + false, + ), + ( + "cd /repo&&thirdshift pickup parallel 2 >>cron.log 2>&1", + Some("/repo"), + Some("/repo/cron.log"), + Some("Pickup run"), + true, + ), + ( + "cd /repo && thirdshift pickup parallel 2 2>>errors.log", + Some("/repo"), + None, + Some("Pickup run"), + true, + ), + ( + "cd /repo && thirdshift pickup parallel 2 2>&1", + Some("/repo"), + None, + Some("Pickup run"), + true, + ), + ( + "cd /repo && thirdshift pickup parallel 2 &>>~/cron.log", + Some("/repo"), + Some("/home/test/cron.log"), + Some("Pickup run"), + true, + ), + ( + "cd /repo && thirdshift pickup parallel 2 1>/tmp/cron.log", + Some("/repo"), + Some("/tmp/cron.log"), + Some("Pickup run"), + true, + ), + ( + "cd /repo && thirdshift pickup parallel 2 >'$LOG'", + Some("/repo"), + None, + Some("Pickup run"), + true, + ), + ( + "cd /repo && thirdshift pickup parallel 2 >'`log`'", + Some("/repo"), + None, + Some("Pickup run"), + true, + ), + ( + "cd relative && thirdshift pickup", + None, + None, + Some("Pickup run"), + false, + ), + ( + "cd ~someone/repo && thirdshift pickup", + None, + None, + Some("Pickup run"), + false, + ), + ( + "cd '$HOME/repo' && thirdshift pickup", + None, + None, + None, + false, + ), + ( + "cd /repo && thirdshift pickup%stdin && cd elsewhere", + Some("/repo"), + None, + Some("Pickup run"), + true, + ), + ( + "cd /repo && thirdshift pickup >'quoted%log'", + None, + None, + None, + false, + ), + ( + r"cd /repo && thirdshift pickup >escaped\%log", + Some("/repo"), + Some("/repo/escaped%log"), + Some("Pickup run"), + true, + ), + ( + r"cd /repo && thirdshift pickup >escaped\\%stdin", + Some("/repo"), + Some("/repo/escaped\\"), + Some("Pickup run"), + true, + ), + ] { + let interpreted = Interpretation::new(text); + assert!(interpreted.recognized(), "{text}"); + assert_eq!( + interpreted + .command_kind() + .map(|kind| kind.to_string()) + .as_deref(), + kind, + "{text}" + ); + let facts = interpreted.into_launch(home); + assert_eq!( + facts.as_ref().map(|facts| facts.directory.as_path()), + directory.map(Path::new), + "{text}" + ); + assert_eq!( + facts.as_ref().and_then(|facts| facts.output_log.as_deref()), + log.map(Path::new), + "{text}" + ); + let parsed = facts.and_then(|facts| facts.command); + if candidate { + let Some(args::Command::Pickup(pass)) = parsed else { + panic!("expected Pickup: {text}"); + }; + if text.contains("parallel 2") { + assert_eq!(pass.flags.parallel.map(|n| n.get()), Some(2), "{text}"); + } + } else { + assert!(parsed.is_none(), "{text}"); + } + } + } + + #[test] + fn interpretation_preserves_independent_cron_job_facts() { + let home = std::path::Path::new("/home/test"); + // Command kind and a parsed start candidate are independent projections. + for (text, shown, kind, directory, candidate) in [ + ("thirdshift pickup", true, Some("Pickup run"), None, false), + ( + "cd ~/repo && thirdshift pickup", + true, + Some("Pickup run"), + Some("/home/test/repo"), + true, + ), + ( + "cd ~/repo && env X=1 thirdshift pickup", + true, + None, + Some("/home/test/repo"), + false, + ), + ( + "cd ~/repo && thirdshift status --json", + true, + None, + Some("/home/test/repo"), + true, + ), + ( + "cd ~/repo && thirdshift pickup --unknown", + true, + None, + Some("/home/test/repo"), + false, + ), + ( + "cd ~/repo && thirdshift pickup base '$BASE'", + true, + None, + Some("/home/test/repo"), + false, + ), + ( + "cd ~/repo && cd /elsewhere && thirdshift pickup", + true, + None, + None, + false, + ), + ( + "cd ~/repo && thirdshift pickup && echo done", + true, + None, + Some("/home/test/repo"), + true, + ), + ( + "cd ~/repo && thirdshift fix https://github.com/acme/widgets/pull/7", + true, + Some("Fix run"), + Some("/home/test/repo"), + true, + ), + ( + "cd ~/repo && thirdshift review https://github.com/acme/widgets/pull/7", + true, + Some("Review run"), + Some("/home/test/repo"), + true, + ), + ( + "cd ~/repo && thirdshift triage", + true, + Some("Triage run"), + Some("/home/test/repo"), + true, + ), + ("cd '~/repo && thirdshift pickup", true, None, None, false), + ("backup%thirdshift pickup", false, None, None, false), + ] { + let interpreted = Interpretation::new(text); + assert_eq!(interpreted.recognized(), shown, "{text}"); + assert_eq!( + interpreted + .command_kind() + .map(|kind| kind.to_string()) + .as_deref(), + kind, + "{text}" + ); + let facts = interpreted.into_launch(home); + assert_eq!( + facts.as_ref().map(|facts| facts.directory.as_path()), + directory.map(std::path::Path::new), + "{text}" + ); + assert_eq!( + facts.as_ref().is_some_and(|facts| facts.command.is_some()), + candidate, + "{text}" + ); + assert!( + facts.is_none_or(|facts| facts.output_log.is_none()), + "{text}" + ); + } + } +} diff --git a/src/cronjobs/fleet.rs b/src/cronjobs/fleet.rs new file mode 100644 index 000000000..3567b4d44 --- /dev/null +++ b/src/cronjobs/fleet.rs @@ -0,0 +1,272 @@ +//! Explicit Fleet schedule client. Owning-machine APIs always remain local. +use std::collections::BTreeMap; + +use anyhow::{Context, Result}; +use serde::Serialize; + +use super::{CronJob, CronJobs}; +use crate::config::{UserConfig, console::fleet::Entry}; +use crate::console::fleet::read::{Refusal, Remote}; +use crate::resend_key::Credentials; + +#[derive(Serialize)] +pub(crate) struct Inventory { + pub repositories: Vec, + pub machines: Vec, +} + +#[derive(Serialize)] +pub(crate) struct Repository { + pub repository: Option, + pub machines: Vec, +} + +impl Repository { + pub fn heading(&self) -> &str { + self.repository.as_deref().unwrap_or("Other") + } +} + +#[derive(Serialize)] +pub(crate) struct Schedules { + pub name: String, + pub jobs: Vec, +} + +#[derive(Serialize)] +pub(crate) struct Machine { + pub name: String, + pub address: Option, + pub status: Status, + pub error: Option, + pub empty: bool, +} + +#[derive(Serialize)] +#[serde(rename_all = "kebab-case")] +pub(crate) enum Status { + Ok, + Unavailable, + Refused, + Incompatible, + LinkOnly, +} + +impl std::fmt::Display for Status { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Self::Ok => "ok", + Self::Unavailable => "unavailable", + Self::Refused => "refused", + Self::Incompatible => "incompatible", + Self::LinkOnly => "link-only", + }) + } +} + +impl Machine { + fn capture(mut self, result: Result) -> (Self, Vec) { + match result { + Ok(inventory) => { + self.empty = inventory.jobs.is_empty(); + (self, inventory.jobs) + } + Err(error) => { + self.status = if let Some(refusal) = error.downcast_ref::() { + match refusal.status.as_u16() { + 401 | 403 => Status::Refused, + 404 => Status::Incompatible, + _ if refusal.message.contains("incompatible") => Status::Incompatible, + _ => Status::Unavailable, + } + } else if error.to_string().contains("incompatible") { + Status::Incompatible + } else { + Status::Unavailable + }; + self.error = Some(format!("{error:#}")); + (self, Vec::new()) + } + } + } +} + +fn remote(config: &UserConfig, entry: &Entry, person: Option<&str>) -> (Machine, Vec) { + let mut machine = Machine { + name: entry.name.clone(), + address: Some(entry.address.clone()), + status: Status::Ok, + error: None, + empty: false, + }; + let read = || -> Result> { + // A link never sends even an identity probe. The transport revalidates + // paired destinations before attaching any saved token. + if Credentials::read()?.fleet_token(&entry.name).is_none() { + return Ok(None); + } + let remote = Remote::select(config, &entry.name, person.map(str::to_owned))? + .context("expected a Fleet machine")?; + let inventory = serde_json::from_value(remote.get("/cronjobs")?) + .context("incompatible cron inventory")?; + Ok(Some(inventory)) + }; + match read() { + Ok(None) => { + machine.status = Status::LinkOnly; + (machine, Vec::new()) + } + Ok(Some(inventory)) => machine.capture(Ok(inventory)), + Err(error) => machine.capture(Err(error)), + } +} + +pub(crate) fn load(config: &UserConfig, person: Option<&str>) -> Inventory { + let local = Machine { + name: crate::console::fleet_control::local_name(config), + address: None, + status: Status::Ok, + error: None, + empty: false, + } + .capture(super::load()); + let mut snapshots = vec![local]; + // Independent requests share no cache and each uses the Fleet transport's + // three-second bound and no-redirect policy. + snapshots.extend(std::thread::scope(|threads| { + let handles: Vec<_> = config + .console + .fleet + .iter() + .map(|entry| threads.spawn(move || remote(config, entry, person))) + .collect(); + handles + .into_iter() + .map(|handle| handle.join().expect("Fleet schedule reader panicked")) + .collect::>() + })); + let mut grouped: BTreeMap<(bool, Option), BTreeMap>> = + BTreeMap::new(); + let mut machines = Vec::new(); + for (machine, jobs) in snapshots { + for job in jobs { + grouped + .entry((job.repository.is_none(), job.repository.clone())) + .or_default() + .entry(machine.name.clone()) + .or_default() + .push(job); + } + machines.push(machine); + } + Inventory { + repositories: grouped + .into_iter() + .map(|((_, repository), machines)| Repository { + repository, + machines: machines + .into_iter() + .map(|(name, jobs)| Schedules { name, jobs }) + .collect(), + }) + .collect(), + machines, + } +} + +pub(crate) fn show(json: bool) -> Result { + let inventory = load(&UserConfig::load()?, None); + if json { + return Ok(serde_json::to_string_pretty(&inventory)?); + } + Ok(text(&inventory)) +} + +fn text(inventory: &Inventory) -> String { + let mut lines = Vec::new(); + for machine in &inventory.machines { + let mut line = format!("{}: {}", super::report_line(&machine.name), machine.status); + if let Some(address) = &machine.address { + line.push_str(&format!(" ({})", super::report_line(address))); + } + if let Some(error) = &machine.error { + line.push_str(&format!(" — {}", super::report_line(error))); + } else if machine.empty { + line.push_str(" — No thirdshift cron jobs in this account's crontab."); + } + lines.push(line); + } + for repository in &inventory.repositories { + lines.push(super::report_line(repository.heading())); + for machine in &repository.machines { + lines.push(format!(" {}", super::report_line(&machine.name))); + for job in &machine.jobs { + let kind = job + .command_kind + .map(|kind| kind.to_string()) + .unwrap_or_else(|| "Unknown Command kind".into()); + lines.push(format!( + " {kind}: {} {}", + super::report_line(&job.schedule), + super::report_line(&job.command) + )); + } + } + } + lines.join("\n") +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::cronjobs::{CommandKind, CronJob}; + + #[test] + fn a_fleet_report_cannot_forge_another_machines_line() { + let schedule = "* * * * *\n west\u{2028}forged"; + let command = "thirdshift pickup\r\u{1b}[1K\u{8}"; + let error = "denied\nwest: ok"; + let inventory = Inventory { + machines: vec![Machine { + name: "east".into(), + address: Some("https://east.example".into()), + status: Status::Refused, + error: Some(error.into()), + empty: false, + }], + repositories: vec![Repository { + repository: Some("acme/widgets".into()), + machines: vec![Schedules { + name: "east".into(), + jobs: vec![CronJob { + schedule: schedule.into(), + command: command.into(), + repository: Some("acme/widgets".into()), + command_kind: Some(CommandKind::Pickup), + }], + }], + }], + }; + // Structured JSON keeps the owning machine's original field text. + let json = serde_json::to_string(&inventory).unwrap(); + assert!(json.contains(r"\n west"), "{json}"); + assert!(json.contains(r"\u001b"), "{json}"); + assert!(json.contains('\u{2028}'), "{json}"); + let report = text(&inventory); + let lines: Vec<_> = report.lines().collect(); + assert!( + lines.iter().all(|line| !line.chars().any(|character| { + character.is_control() || matches!(character, '\u{2028}' | '\u{2029}') + })), + "{report:?}" + ); + assert_eq!(lines.len(), 4, "{report:?}"); + assert!(lines[0].starts_with("east: refused"), "{report:?}"); + assert!(lines[0].contains("west: ok"), "{report:?}"); + assert_eq!(lines[1], "acme/widgets"); + assert_eq!(lines[2], " east"); + assert!(lines[3].starts_with(" Pickup run:"), "{report:?}"); + assert!(lines[3].contains("thirdshift pickup"), "{report:?}"); + assert!(lines[3].contains("* * * * *"), "{report:?}"); + } +} diff --git a/src/evals/experiment/execution.rs b/src/evals/experiment/execution.rs index b85ffea4c..1a9e2e786 100644 --- a/src/evals/experiment/execution.rs +++ b/src/evals/experiment/execution.rs @@ -205,6 +205,7 @@ impl<'a> Execution<'a> { &prepared_repo.repository, &setup.choice(), )); + let waiting = RefCell::new(crate::poll::WaitingLine::already_reported(())); let started = Instant::now(); // Machine caps are read live per attempt, not fixed with the Bar: an // operator edit between attempts applies to the next one. @@ -223,7 +224,16 @@ impl<'a> Execution<'a> { stop: &|child| process::stop(child, &[libc::SIGTERM, libc::SIGKILL]), }, Instant::now() + self.timeout, - &|| monitor.borrow_mut().check(self.signal), + &|| { + monitor.borrow_mut().check(self.signal)?; + // Trial work writes nothing to the Command log until its + // outcome. Repeat a line so a long Trial is not taken as + // Silent; supervision ends it, however the Trial ends. + if waiting.borrow_mut().should_report(()) { + progress::step(format!("Trial {key} still running")); + } + Ok(()) + }, ); let stopped_at = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH)? diff --git a/src/fix.rs b/src/fix.rs index d024f31ef..4232c0deb 100644 --- a/src/fix.rs +++ b/src/fix.rs @@ -44,16 +44,22 @@ pub(crate) fn run( crate::command_limit::Sharing::UpToTheLimit, )? { crate::launch::Start::Clear(launch) => launch, - crate::launch::Start::AlreadyRunning(running) => { + crate::launch::Start::NotAdmitted(running) => { return Ok(skipped(&running.repo, running.to_string())); } }; let github = GitHub::new(); let base = launch.base.name(); - if author.is_none() - && let Some(ending) = base_ci::run(&launch, command, config, harness)? - { - return Ok(ending); + if author.is_none() { + match base_ci::run(&launch, command, config, harness)? { + base_ci::Outcome::Clear => {} + base_ci::Outcome::Skipped(reason) => { + logs::base_ci_skipped(&launch.repo, &reason); + } + base_ci::Outcome::Run(ended) => { + return Ok(crate::command::Ending::Run(ended)); + } + } } for pr in github.fix_candidates(&launch.repo, base, author.as_deref())? { if !github.needs_fix(&pr, base, author.as_deref(), false)? { diff --git a/src/fix/base_ci.rs b/src/fix/base_ci.rs index eac9e93c2..021700f3e 100644 --- a/src/fix/base_ci.rs +++ b/src/fix/base_ci.rs @@ -8,22 +8,27 @@ use crate::github::TipCi; use crate::issue::IssueUrl; use crate::run::{Goal, Resolution}; +pub(super) enum Outcome { + Clear, + Skipped(String), + Run(Ended), +} + pub(super) fn run( launch: &crate::launch::Launch, command: &FixArgs, config: &UserConfig, harness: &mut Choice, -) -> Result> { +) -> Result { let repo = &launch.repo; let base = launch.base.name(); let tip = launch.base.origin_commit(); // Share Pickup's CI reservation so overlapping hosts cannot re-run or // publish the same Base branch repair concurrently. let Some(_held) = crate::reservation::Reservation::ci_red(launch.directory.git())? else { - return Ok(Some(skipped( - repo, + return Ok(Outcome::Skipped( "Base branch CI is reserved on origin".into(), - ))); + )); }; let github = GitHub::new(); // Delivery's repository-scoped adapters take an IssueUrl. No issue exists @@ -36,26 +41,25 @@ pub(super) fn run( }; let checks = match github.fix_base_tip(&repository, base, tip)? { TipCi::Waiting => { - return Ok(Some(skipped( - repo, - format!("CI on {base} is pending or re-running"), + return Ok(Outcome::Skipped(format!( + "CI on {base} is pending or re-running" ))); } TipCi::Checks(checks) => checks, }; if !ci::needs_repair(&checks) { - return Ok(None); + return Ok(Outcome::Clear); } let failed = ci::base_failures(checks); let branch = format!("fix-{base}-{}", ci::short(tip)); if github.base_fix_pr_exists(repo, &branch)? || launch.directory.git().on_origin(&branch)? { - return Ok(Some(skipped( - repo, - format!("a repair branch or pull request from {branch} already exists"), + return Ok(Outcome::Skipped(format!( + "a repair branch or pull request from {branch} already exists" ))); } harness.check()?; logs::started(Work::FixRun(repo), harness); + logs::base_branch(base); logs::issue_branch(Some(&branch)); let title = format!("Fix CI on {base}: {}", ci::title_check_names(&failed)); logs::issue_title(Some(title.clone())); @@ -120,9 +124,9 @@ pub(super) fn run( format_args!("could not close CI-red issues on {base}"), ); } - Ok(Some(crate::command::Ending::Run(Ended { + Ok(Outcome::Run(Ended { outcome, base_fix: None, advice: Vec::new(), - }))) + })) } diff --git a/src/github.rs b/src/github.rs index b39fa8f0b..c10093a4f 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; @@ -98,18 +98,21 @@ pub(crate) fn operational_cause(error: &anyhow::Error) -> Option<&'static str> { /// independently without changing the recorded request or other views. pub struct GitHub { interruption: Interruption, + wait_for_quota: bool, } impl GitHub { pub fn new() -> Self { Self { interruption: Interruption::Ordinary, + wait_for_quota: true, } } pub fn completion(&self) -> Self { Self { interruption: Interruption::Completion, + wait_for_quota: self.wait_for_quota, } } @@ -775,15 +778,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 +841,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 +856,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 +1279,36 @@ impl GitHub { } fn output_with_input(&self, args: &[&str], input: Option<&[u8]>) -> Result { + let mut secondary_retries = 0; + loop { + let output = self.raw_output_with_input(args, input)?; + if output.status.success() { + return Ok(output); + } + match quota::Limit::of(args, &output.stderr) { + Some(quota::Limit::Primary(resource)) => { + let reset = self.record_quota_failure(args, resource); + if !self.wait_for_quota + || !quota::command::wait(reset, resource, self.interruption)? + { + return Ok(output); + } + } + Some(quota::Limit::Secondary) + if self.wait_for_quota && quota::command::active() && secondary_retries < 5 => + { + secondary_retries += 1; + crate::progress::step(format!( + "GitHub secondary limit; retry {secondary_retries}/5 in 60s" + )); + self.interruption.pause(quota::wait::minute())?; + } + _ => return 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/check_read.rs b/src/github/check_read.rs index 460750c02..cf0e4086a 100644 --- a/src/github/check_read.rs +++ b/src/github/check_read.rs @@ -45,10 +45,7 @@ pub(crate) fn is_transient(error: &anyhow::Error) -> bool { let stderr = failure.stderr.trim().to_ascii_lowercase(); // Quota exhaustion belongs to quota-aware deferral, not a short retry. // gh can report GraphQL quota errors with or without an HTTP status. - if stderr.contains("api rate limit") - || stderr.contains("rate_limited") - || stderr.contains("rate_limit_exceeded") - { + if super::quota::Limit::of(&[], stderr.as_bytes()).is_some() { return false; } // gh reports HTTP status in stderr. Prefer it to incidental prose, so diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs index d96c158c2..bac7b8936 100644 --- a/src/github/execution_tests.rs +++ b/src/github/execution_tests.rs @@ -58,6 +58,209 @@ 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 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( + "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); + }, + ); +} + +#[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( + "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 { @@ -77,7 +280,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 } @@ -407,9 +614,9 @@ printf '%s' '{{"title":"Recovered"}}' } #[test] -fn permanent_and_quota_read_failures_are_not_retried() { +fn permanent_reads_and_quota_reads_outside_a_command_are_not_retried() { with_recorded_signal( - "github::execution_tests::permanent_and_quota_read_failures_are_not_retried", + "github::execution_tests::permanent_reads_and_quota_reads_outside_a_command_are_not_retried", |_| { let fixture = Fixture::new(""); for stderr in [ @@ -424,7 +631,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(); @@ -1413,7 +1620,7 @@ fn check_reads_classify_transport_and_api_failures_without_hiding_diagnostics() ("read tcp: connection reset by peer", true), ("net/http: TLS handshake timeout", true), ("gh: too many requests (HTTP 429)", true), - ("GraphQL: secondary rate limit exceeded (HTTP 429)", true), + ("GraphQL: secondary rate limit exceeded (HTTP 429)", false), ("gh: Bad Gateway (HTTP 502)", true), ("gh: service unavailable (HTTP 503)", true), ("gh: Gateway Timeout (HTTP 504)", true), diff --git a/src/github/quota.rs b/src/github/quota.rs index da759ee96..db99c6d64 100644 --- a/src/github/quota.rs +++ b/src/github/quota.rs @@ -1,4 +1,4 @@ -//! GitHub's two quota clocks. The Inbox leaves a fixed reserve for the factory. +//! Shared machine-local Quota waits, Pass admission and running Command recovery. use anyhow::{Context, Result}; use chrono::{DateTime, SecondsFormat, Utc}; use serde::{Deserialize, Serialize}; @@ -6,7 +6,33 @@ use serde_json::Value; use super::GitHub; -#[derive(Clone, Serialize, Deserialize)] +pub(crate) mod command; +mod identity; +mod store; +#[cfg(test)] +mod tests; +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 +65,199 @@ 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, + fn pass_reason(&self, resource: Resource) -> String { + format!( + "GitHub Quota wait until {} ({}: {} left)", + self.reset_at.to_rfc3339_opts(SecondsFormat::Secs, true), + resource.name(), + self.remaining, + ) + } } -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 { + /// One stable reason, held until the last depleted resource resets. + pub fn pass_reason(waits: &[Self]) -> Option { + let wait = waits + .iter() + .filter(|wait| wait.reset_at() > Utc::now()) + .max_by_key(|wait| wait.reset_at())?; + Some(wait.quota.pass_reason(wait.resource)) + } + + 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.reset_at() > Utc::now()) + .map(|wait| { + let name = match wait.resource { + Resource::Graphql => "GraphQL points", + Resource::Core => "REST requests", + }; + let reserve = if wait.remaining() < 500 { + " (fewer than 500)" + } else { + "" + }; + format!( + "{} {name} remaining{reserve}; Inbox reads paused until {}", + wait.remaining(), + wait.reset_at().to_rfc3339_opts(SecondsFormat::Secs, true), + ) + }) + .collect(); (!reasons.is_empty()).then(|| reasons.join("; ")) } } impl GitHub { + /// A known wait needs no probe. Non-quota probe failures fail open; + /// exhaustion recorded by a failed probe still skips this Pass. + pub(crate) fn pass_quota_wait(&self) -> Option { + if let Some(reason) = QuotaWait::pass_reason(&self.quota_waits().unwrap_or_default()) { + return Some(reason); + } + // Admission records exhaustion and skips; only admitted Commands + // wait and retry. Keep the probe's ordinary read-failure handling. + let probe = Self { + interruption: self.interruption, + wait_for_quota: false, + }; + if let Ok(quotas) = probe.current_quotas() { + // Admission depends on GitHub's observation even when an + // exhausted account cannot identify itself for shared storage. + let reason = quotas + .iter() + .filter(|(_, quota)| quota.low(Utc::now())) + .max_by_key(|(_, quota)| quota.reset_at) + .map(|(resource, quota)| quota.pass_reason(*resource)); + for (resource, quota) in quotas { + let _ = probe.observe_quota(resource, quota); + } + return reason; + } + QuotaWait::pass_reason(&self.quota_waits().unwrap_or_default()) + } + + // 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 { + fn current_quotas(&self) -> Result<[(Resource, Quota); 2]> { let json = self.gh_json(&["api", "rate_limit"])?; - let resources = &json["resources"]; - Ok(Quotas { - graphql: Quota::rest(&resources["graphql"])?, - rest: Quota::rest(&resources["core"])?, - }) + Ok([ + ( + Resource::Graphql, + Quota::rest(&json["resources"]["graphql"])?, + ), + (Resource::Core, Quota::rest(&json["resources"]["core"])?), + ]) + } + + pub(crate) fn inbox_quotas(&self) -> Result> { + for (resource, quota) in self.current_quotas()? { + self.observe_quota(resource, quota)?; + } + self.quota_waits() + } + + pub(super) fn record_quota_failure(&self, args: &[&str], resource: Resource) -> DateTime { + let host = identity::host(args); + 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)), + }; + let reset = quota.reset_at; + let record = || -> Result<()> { + let store = Store::machine()?; + let account = identity::resolve(self, &store, &host)?; + store.exhausted(&account, resource, quota, now) + }; + if record().is_err() { + // Recording failure must not prevent waiting or hide the original + // GitHub diagnostic if the Command reaches its wait cap. + crate::progress::step("warning: could not record GitHub Quota wait"); + } + reset + } +} + +pub(super) enum Limit { + Primary(Resource), + Secondary, +} + +impl Limit { + pub(super) fn of(args: &[&str], stderr: &[u8]) -> Option { + let diagnostic = String::from_utf8_lossy(stderr).to_ascii_lowercase(); + if diagnostic.contains("secondary rate limit") || diagnostic.contains("abuse detection") { + return Some(Self::Secondary); + } + if !(diagnostic.contains("api rate limit exceeded") + || diagnostic.contains("api rate limit already exceeded") + || diagnostic.contains("rate_limit_exceeded") + || diagnostic.contains("rate_limited")) + { + return None; + } + let graphql = diagnostic.contains("graphql:") + || args.starts_with(&["api", "graphql"]) + || args.first() != Some(&"api") + && (diagnostic.contains("rate_limit_exceeded") + || diagnostic.contains("rate_limited")); + Some(Self::Primary(if graphql { + Resource::Graphql + } else { + Resource::Core + })) } } diff --git a/src/github/quota/command.rs b/src/github/quota/command.rs new file mode 100644 index 000000000..f0da97b6c --- /dev/null +++ b/src/github/quota/command.rs @@ -0,0 +1,180 @@ +//! One budget and notification total per owning Command, including subprocess +//! Ticket runs. The Command log owner supplies their shared sidecar path. +use std::collections::BTreeSet; +use std::fs; +use std::io::Write; +use std::path::PathBuf; +use std::sync::Mutex; +use std::time::{Duration, Instant}; + +use anyhow::Result; +use chrono::{DateTime, SecondsFormat, Utc}; +use serde::{Deserialize, Serialize}; + +use super::{Resource, wait::WaitBudget}; +use crate::{logs, process::Interruption, progress}; + +#[derive(Clone, Default, Serialize, Deserialize)] +struct Totals { + budget: WaitBudget, + waited: Duration, + resources: BTreeSet, +} + +#[derive(Default)] +struct Command { + totals: Totals, + path: Option, +} + +static COMMAND: Mutex> = Mutex::new(None); + +pub(crate) fn begin() { + COMMAND.lock().unwrap().get_or_insert_with(Command::default); +} + +pub(in crate::github) fn active() -> bool { + COMMAND.lock().unwrap().is_some() +} + +/// Called outside the recording lock, once its owner is known. A child uses +/// the inherited outermost owner, never its own child record's path. +pub(crate) fn attach(path: PathBuf) -> Result<()> { + let mut command = COMMAND.lock().unwrap(); + let Some(command) = command.as_mut() else { + return Ok(()); + }; + if command.path.is_none() { + let totals = std::mem::take(&mut command.totals); + command.path = Some(path.with_extension("quota-wait.json")); + // Before log allocation, only the parent can have waited. Commands + // that never wait need no quota sidecar. + if !totals.budget.is_empty() { + command.update(|stored| *stored = totals)?; + } + } + Ok(()) +} + +impl Command { + fn read(&self) -> Result { + let Some(path) = &self.path else { + return Ok(self.totals.clone()); + }; + match fs::read(path) { + Ok(bytes) => Ok(serde_json::from_slice(&bytes)?), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(Totals::default()), + Err(error) => Err(error.into()), + } + } + + fn update(&mut self, change: impl FnOnce(&mut Totals) -> T) -> Result { + let Some(path) = &self.path else { + return Ok(change(&mut self.totals)); + }; + let _lock = super::store::lock_exclusive(&path.with_extension("lock"))?; + let mut totals = self.read()?; + let value = change(&mut totals); + let mut tmp = tempfile::NamedTempFile::new_in(path.parent().unwrap())?; + tmp.write_all(&serde_json::to_vec(&totals)?)?; + tmp.persist(path)?; + Ok(value) + } +} + +/// The ordinary sleep and the completion sleep share accounting but preserve +/// their caller's interruption policy. A rejected reservation leaves the +/// caller's original GitHub failure intact. +pub(in crate::github) fn wait( + reset: DateTime, + resource: Resource, + interruption: Interruption, +) -> Result { + let delay = { + let mut command = COMMAND.lock().unwrap(); + let Some(command) = command.as_mut() else { + return Ok(false); + }; + command.update(|totals| totals.budget.next(reset.max(Utc::now())))? + }; + let Ok(delay) = delay else { return Ok(false) }; + let until = (Utc::now() + chrono::Duration::from_std(delay)?) + .to_rfc3339_opts(SecondsFormat::Secs, true); + logs::quota_wait(Some(&until)); + let started = Instant::now(); + let mut left = delay; + let result = loop { + progress::step(format!("Quota wait until {until} ({})", resource.name())); + if let Err(error) = interruption.pause(left.min(Duration::from_secs(30))) { + break Err(error); + } + left = delay.saturating_sub(started.elapsed()); + if left.is_zero() { + break Ok(()); + } + }; + logs::quota_wait(None); + let elapsed = started.elapsed().min(delay); + if let Some(command) = COMMAND.lock().unwrap().as_mut() { + command.update(|totals| { + totals.budget.refund(delay.saturating_sub(elapsed)); + totals.waited += elapsed; + if !elapsed.is_zero() { + totals.resources.insert(resource.name().into()); + } + })?; + } + result.map(|()| true) +} + +pub(crate) fn notification_line() -> Result> { + let command = COMMAND.lock().unwrap(); + let Some(command) = command.as_ref() else { + return Ok(None); + }; + let totals = command.read()?; + Ok((!totals.waited.is_zero()).then(|| { + format!( + "Waited {} for the GitHub quota ({})", + progress::duration(totals.waited), + totals + .resources + .iter() + .cloned() + .collect::>() + .join(", ") + ) + })) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use std::os::unix::fs::PermissionsExt; + use std::time::Duration; + + #[test] + fn quota_wait_sidecar_lock_stays_private_to_the_owner() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("run.quota-wait.json"); + let lock_path = path.with_extension("lock"); + fs::write(&lock_path, []).unwrap(); + fs::set_permissions(&lock_path, fs::Permissions::from_mode(0o644)).unwrap(); + let mut command = Command { + totals: Totals::default(), + path: Some(path), + }; + command + .update(|totals| { + totals.waited = Duration::from_secs(1); + }) + .unwrap(); + let mode = fs::metadata(&lock_path).unwrap().permissions().mode(); + assert_eq!( + mode & 0o077, + 0, + "sidecar lock mode {mode:o} lets another local user block the owner" + ); + } +} diff --git a/src/github/quota/identity.rs b/src/github/quota/identity.rs new file mode 100644 index 000000000..99a100676 --- /dev/null +++ b/src/github/quota/identity.rs @@ -0,0 +1,143 @@ +//! 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 { + 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" | "--comment" | "--label" | "--search" | "-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); + } + } + } + } + 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 let [host, _, _] = repository.split('/').collect::>().as_slice() { + return host.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..9c504ea35 --- /dev/null +++ b/src/github/quota/store.rs @@ -0,0 +1,181 @@ +//! One machine-local record, with a stable lock across atomic replacements. +use std::fs::{self, File, OpenOptions}; +use std::io::Write; +use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; +use std::path::{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 = lock_exclusive(&self.path.with_extension("lock"))?; + let mut record = self.read()?; + let previous = serde_json::to_vec(&record)?; + change(&mut record); + let bytes = serde_json::to_vec(&record)?; + // The Console watches this record: unchanged quota observations + // must not wake open Inbox pages. + if bytes == previous { + return Ok(()); + } + let mut tmp = tempfile::NamedTempFile::new_in(dir)?; + tmp.write_all(&bytes)?; + tmp.persist(&self.path)?; + Ok(()) + } +} + +/// Another local user can take an exclusive flock on a lock they can open, +/// even with a read-only descriptor, and stall every Quota update. Creation +/// mode is masked by umask, so an existing lock is tightened explicitly. +pub(super) fn lock_exclusive(path: &Path) -> Result { + let lock = OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .mode(0o600) + .open(path)?; + lock.set_permissions(fs::Permissions::from_mode(0o600))?; + lock.lock()?; + Ok(lock) +} diff --git a/src/github/quota/tests.rs b/src/github/quota/tests.rs new file mode 100644 index 000000000..c66feadc1 --- /dev/null +++ b/src/github/quota/tests.rs @@ -0,0 +1,160 @@ +use super::*; +use std::os::unix::fs::PermissionsExt; +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].remaining(), 499); + assert_eq!(waits[0].reset_at(), now() + chrono::Duration::hours(1)); + 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 quota_record_lock_stays_private_to_the_owner() { + let dir = tempfile::tempdir().unwrap(); + let lock_path = dir.path().join("quota.lock"); + // A lock created under the usual umask is group- and world-readable. + // Opening it read-only is enough to take an exclusive flock. + std::fs::write(&lock_path, []).unwrap(); + std::fs::set_permissions(&lock_path, std::fs::Permissions::from_mode(0o644)).unwrap(); + let store = Store::new(dir.path().join("quota.json")); + store + .observe( + &Account::new("github.com", "octocat"), + Resource::Core, + quota(100), + now(), + ) + .unwrap(); + let mode = std::fs::metadata(&lock_path).unwrap().permissions().mode(); + assert_eq!( + mode & 0o077, + 0, + "quota lock mode {mode:o} lets another local user block the owner" + ); +} + +#[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..343bc4cd7 --- /dev/null +++ b/src/github/quota/wait.rs @@ -0,0 +1,61 @@ +//! A running Command's primary-quota wait budget, shared by its child Runs. +use std::time::Duration; + +use chrono::{DateTime, Utc}; + +const COMMAND_CAP: Duration = Duration::from_secs(2 * 60 * 60); + +#[derive(Clone, Default, serde::Serialize, serde::Deserialize)] +pub(crate) struct WaitBudget { + total: Duration, +} + +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct WaitCapExceeded; + +impl WaitBudget { + pub(super) fn is_empty(&self) -> bool { + self.total.is_zero() + } + 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"); + // The override shortens the safety minute and jitter for binary tests; + // the reset itself remains a real wall-clock deadline. + let minute = minute(); + let margin = minute + minute.mul_f64(jitter as f64 / 60.0); + let until = reset + chrono::Duration::from_std(margin).unwrap(); + 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) + } + + pub(super) fn refund(&mut self, unused: Duration) { + self.total = self.total.saturating_sub(unused); + } +} + +/// Shorten a quota safety minute for binary tests, as with CI's grace period. +pub(in crate::github) fn minute() -> Duration { + std::env::var("THIRDSHIFT_QUOTA_MINUTE_MS") + .ok() + .and_then(|ms| ms.parse().ok()) + .filter(|ms| *ms > 0) + .map(Duration::from_millis) + .unwrap_or(Duration::from_secs(60)) +} diff --git a/src/launch.rs b/src/launch.rs index a3b53de9e..0a9066e47 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -19,7 +19,7 @@ use std::path::PathBuf; use anyhow::{Context, Result, bail}; use crate::command_limit; -use crate::command_limit::{AlreadyRunning, Sharing}; +use crate::command_limit::{NotAdmitted, Sharing}; use crate::git::Git; use crate::github::GitHub; use crate::issue::{IssueInput, IssueUrl, Repo}; @@ -338,7 +338,7 @@ pub enum Start { /// Its checks passed and it holds a slot on its repository. Clear(Launch), /// It is skipped, with nothing done. - AlreadyRunning(AlreadyRunning), + NotAdmitted(NotAdmitted), } /// Start a run with no Issue URL from the Launch directory, the current @@ -346,8 +346,9 @@ pub enum Start { /// whatever the Launch directory has checked out, or without one the branch /// checked out there, so a detached HEAD with neither fails it. /// -/// Once the preflight checks pass, the run is [`AlreadyRunning`], with -/// nothing done, if its repository is at its Command `limit` on this +/// A Quota wait skips the Pass before remote preparation. Once the preflight +/// checks pass, the run is also [`NotAdmitted`], with nothing done, if its +/// repository is at its Command `limit` on this /// machine, or, as `sharing` says, another Pass of its kind runs there. /// Otherwise this process holds a slot there, and the lock of its kind if it /// has one, until it exits, through whatever it dispatches. @@ -355,10 +356,13 @@ pub fn start(base: Option<&str>, limit: NonZeroUsize, sharing: Sharing) -> Resul let directory = LaunchDirectory::open(current_dir()?, Opening::Pass)?; let repo = directory.repo()?; crate::logs::pass_fired(&repo); + if let Some(skipped) = command_limit::quota_wait(&repo, limit) { + return Ok(Start::NotAdmitted(skipped)); + } directory.check_identity()?; let base = directory.prepare_base_branch(base)?; if let Some(running) = command_limit::hold_pass(&repo, limit, sharing)? { - return Ok(Start::AlreadyRunning(running)); + return Ok(Start::NotAdmitted(running)); } Ok(Start::Clear(Launch { directory, diff --git a/src/launch_directories.rs b/src/launch_directories.rs index b219eef95..ab0342dce 100644 --- a/src/launch_directories.rs +++ b/src/launch_directories.rs @@ -1,4 +1,5 @@ //! Known Launch directories, shared by CLI discovery and Console starts. +use std::cell::{OnceCell, RefCell}; use std::collections::BTreeMap; use std::path::{Path, PathBuf}; @@ -7,7 +8,10 @@ use serde::Serialize; use crate::{config, cronjobs, git::Git, issue::Repo}; -#[derive(Serialize)] +#[cfg(test)] +mod tests; + +#[derive(Clone, Serialize)] pub(crate) struct Directory { pub repository: String, pub path: PathBuf, @@ -15,199 +19,165 @@ pub(crate) struct Directory { pub output_log: Option, } -#[derive(Serialize)] +#[derive(Clone, Serialize)] pub(crate) struct Inventory { pub directories: Vec, } pub(crate) fn load() -> Result { - let home = config::home()?; let config = config::UserConfig::load()?; - let jobs = match cronjobs::jobs() { - Ok(jobs) => jobs, - Err(error) - if error - .downcast_ref::() - .is_some_and(|error| error.kind() == std::io::ErrorKind::NotFound) => - { - Vec::new() - } - Err(error) => return Err(error), - }; - let mut directories = BTreeMap::new(); - for job in jobs { - if let Some(directory) = from_cron(&job.command, &home) { - directories - .entry(directory.path.clone()) - .or_insert(directory); - } - } - for path in config.launch_directories { - let directory = resolve(&path) - .with_context(|| format!("invalid configured Launch directory {}", path.display()))?; - directories - .entry(directory.path.clone()) - .or_insert(directory); - } - Ok(Inventory { - directories: directories.into_values().collect(), - }) + Ok(Captured::capture(&config)?.directories()?.clone()) } -fn resolve(path: &Path) -> Result { - let path = path.canonicalize()?; - let origin = Git::new(&path).run(&["config", "--get", "remote.origin.url"])?; - let repo = Repo::of_origin(&origin).context("Launch directory origin is not on GitHub")?; - Ok(Directory { - repository: repo.slug(), - path, - output_log: None, - }) +/// Associated facts for one entry, independent of directory deduplication. +pub(crate) struct Entry { + pub job: cronjobs::CronJob, + pub launch: Option, + pub directory: Option, } -// Parse literal shell words only. Never execute cron text to discover a path. -pub(crate) fn from_cron(command: &str, home: &Path) -> Option { - let invocation = cron_invocation(command, home)?; - let mut directory = resolve(&invocation.directory).ok()?; - directory.output_log = invocation.output_log; - Some(directory) +/// One owning-machine operation. Failed observations live as long as successes; +/// configured directories and Pass history are acquired only by their projections. +pub(crate) struct Captured { + entries: Result>, + configured: Vec, + logs_dir: PathBuf, + checkouts: RefCell, + directories: OnceCell>, + cronjobs: OnceCell>, } -struct CronInvocation { - directory: PathBuf, - words: Vec, - output_log: Option, -} - -fn cron_invocation(command: &str, home: &Path) -> Option { - let words = shell_words(cronjobs::shell_command(command))?; - let [cd, path, and, rest @ ..] = words.as_slice() else { - return None; - }; - if cd != "cd" || and != "&&" || path.contains(['$', '`']) { - return None; +impl Captured { + pub(crate) fn capture(config: &config::UserConfig) -> Result { + let home = config::home()?; + Ok(Self::new(config, &home, cronjobs::entries())) } - let thirdshift = rest - .split(|word| matches!(word.as_str(), "&&" | ";" | "||")) - // A later directory change makes the initial cd unreliable. Leave - // these compounds display-only rather than infer the wrong checkout. - .take_while(|part| { - let executable = part.iter().map(String::as_str).find(|word| { - !word.contains('=') && !matches!(*word, "command" | "builtin" | "--" | "-p" | "!") - }); - !matches!( - executable, - Some("cd" | "pushd" | "popd" | "eval" | "source" | ".") - ) - }) - .find(|part| { - let executable = if part.first().is_some_and(|word| { - Path::new(word) - .file_name() - .is_some_and(|name| name == "env") - }) { - // A literal env wrapper still uses this cd directory. Keep the - // wrapper words below so Run now cannot discard its environment. - part.iter().skip(1).find(|word| !word.contains('=')) - } else { - part.first() - }; - executable.is_some_and(|word| { - Path::new(word) - .file_name() - .is_some_and(|name| name == "thirdshift") - }) - })?; - let path = config::expand_home(path, home)?; - let output_log = thirdshift - .windows(2) - .find(|pair| matches!(pair[0].as_str(), ">>" | ">" | "1>" | "1>>" | "&>" | "&>>")) - .and_then(|pair| { - let log = &pair[1]; - if log.contains(['$', '`']) { - return None; - } - config::expand_home(log, home).or_else(|| Some(path.join(log))) - }); - Some(CronInvocation { - directory: path, - words: thirdshift.iter().skip(1).cloned().collect(), - output_log, - }) -} -/// A literal cron invocation that can also be represented by the start API. -/// Shell expressions remain display-only; no cron text is ever executed here. -pub(crate) fn cron_arguments(command: &str) -> Option<(PathBuf, Vec)> { - let home = config::home().ok()?; - let invocation = cron_invocation(command, &home)?; - let arguments: Vec<_> = invocation - .words - .into_iter() - .take_while(|word| { - let operator = word.trim_start_matches(|c: char| c.is_ascii_digit()); - !matches!( - operator, - ">" | ">>" | "&>" | "&>>" | "<" | ">&" | "<&" | "#" - ) - }) - .collect(); - if arguments.iter().any(|word| word.contains(['$', '`'])) { - return None; + fn new( + config: &config::UserConfig, + home: &Path, + input: Result>, + ) -> Self { + let mut checkouts = Checkouts::default(); + let entries = input.map(|entries| { + entries + .into_iter() + .map(|entry| { + let launch = entry.interpretation.into_launch(home); + let directory = launch.as_ref().and_then(|facts| { + let mut directory = checkouts.resolve(&facts.directory).ok()?; + directory.output_log = facts.output_log.clone(); + Some(directory) + }); + Entry { + job: entry.job, + launch, + directory, + } + }) + .collect() + }); + Self { + entries, + configured: config.launch_directories.clone(), + logs_dir: config.logs_dir.clone(), + checkouts: RefCell::new(checkouts), + directories: OnceCell::new(), + cronjobs: OnceCell::new(), + } } - Some((invocation.directory.canonicalize().ok()?, arguments)) -} -// shlex splits shell words, but leaves operators attached to those words. -// Separate unquoted operators first, preserving quotes and escaped characters. -fn shell_words(command: &str) -> Option> { - let mut spaced = String::new(); - let mut quote = None; - let mut escaped = false; - let mut chars = command.chars().peekable(); - while let Some(character) = chars.next() { - if escaped { - spaced.push(character); - escaped = false; - continue; - } - if character == '\\' && quote != Some('\'') { - spaced.push(character); - escaped = true; - continue; - } - if quote == Some(character) { - quote = None; - } else if quote.is_none() && matches!(character, '\'' | '"') { - quote = Some(character); - } else if quote.is_none() && "&|;<>".contains(character) { - // A contiguous IO number belongs to the redirect (2>&1); a - // standalone number remains a CLI argument (parallel 2 >> log). - let word_start = spaced.rfind(char::is_whitespace).map_or(0, |at| at + 1); - let word = &spaced[word_start..]; - let io_number = matches!(character, '<' | '>') - && !word.is_empty() - && word.bytes().all(|byte| byte.is_ascii_digit()); - if !io_number { - spaced.push(' '); - } - spaced.push(character); - if chars.peek() == Some(&character) - || matches!(character, '>' | '<') && chars.peek() == Some(&'&') + /// Start/discovery projections tolerate only an absent crontab executable. + pub(crate) fn entries(&self) -> Result<&[Entry]> { + match &self.entries { + Err(error) + if error + .downcast_ref::() + .is_some_and(|error| error.kind() == std::io::ErrorKind::NotFound) => { - spaced.push(chars.next()?); - } else if character == '&' && chars.peek() == Some(&'>') { - spaced.push(chars.next()?); - if chars.peek() == Some(&'>') { - spaced.push(chars.next()?); - } + Ok(&[]) } - spaced.push(' '); - continue; + result => Ok(observed(result)?.as_slice()), } - spaced.push(character); } - shlex::split(&spaced) + + pub(crate) fn directories(&self) -> Result<&Inventory> { + observed(self.directories.get_or_init(|| { + let mut directories = BTreeMap::new(); + for directory in self + .entries()? + .iter() + .filter_map(|entry| entry.directory.as_ref()) + { + directories + .entry(directory.path.clone()) + .or_insert_with(|| directory.clone()); + } + for path in &self.configured { + // Validate aliases too; a Cron entry never authorizes a configured path. + let directory = self.checkouts.borrow_mut().resolve(path).with_context(|| { + format!("invalid configured Launch directory {}", path.display()) + })?; + directories + .entry(directory.path.clone()) + .or_insert(directory); + } + Ok(Inventory { + directories: directories.into_values().collect(), + }) + })) + } + + /// Strict inventory, without configured-directory validation or start data. + pub(crate) fn cronjobs(&self) -> Result<&cronjobs::CronJobs> { + observed(self.cronjobs.get_or_init(|| { + let jobs = observed(&self.entries)? + .iter() + .map(|entry| { + let mut job = entry.job.clone(); + job.repository = entry + .directory + .as_ref() + .map(|directory| directory.repository.clone()); + job + }) + .collect(); + cronjobs::inventory(jobs, &self.logs_dir) + })) + } +} + +fn observed(result: &Result) -> Result<&T> { + result + .as_ref() + .map_err(|error| anyhow::anyhow!("{error:#}")) +} + +#[derive(Default)] +struct Checkouts { + canonical: BTreeMap>, + origins: BTreeMap>, +} + +impl Checkouts { + fn resolve(&mut self, literal: &Path) -> Result { + let path = observed( + self.canonical + .entry(literal.to_owned()) + .or_insert_with(|| Ok(literal.canonicalize()?)), + )?; + let repository = observed(self.origins.entry(path.clone()).or_insert_with(|| { + let origin = Git::new(path).run(&["config", "--get", "remote.origin.url"])?; + Ok(Repo::of_origin(&origin) + .context("Launch directory origin is not on GitHub")? + .slug()) + }))?; + Ok(Directory { + repository: repository.clone(), + path: path.clone(), + output_log: None, + }) + } } pub fn show(json: bool) -> Result { diff --git a/src/launch_directories/tests.rs b/src/launch_directories/tests.rs new file mode 100644 index 000000000..322e5b638 --- /dev/null +++ b/src/launch_directories/tests.rs @@ -0,0 +1,166 @@ +use super::*; +use std::fs; +use std::os::unix::fs::symlink; + +fn checkout(home: &Path, name: &str, origin: &str) -> PathBuf { + let path = home.join(name); + fs::create_dir_all(&path).unwrap(); + let git = Git::new(&path); + git.run(&["init", "-q"]).unwrap(); + git.run(&["remote", "add", "origin", origin]).unwrap(); + path.canonicalize().unwrap() +} + +fn config(home: &Path) -> config::UserConfig { + config::UserConfig::parse("", &home.join("config.toml"), home).unwrap() +} + +#[test] +fn captured_projections_preserve_entry_associations_order_and_literal_logs() { + let home = tempfile::tempdir().unwrap(); + let a = checkout(home.path(), "a", "https://github.com/acme/z.git"); + let b = checkout(home.path(), "b", "https://github.com/acme/a.git"); + let alias = home.path().join("alias"); + symlink(&a, &alias).unwrap(); + let mut config = config(home.path()); + config.launch_directories = vec![alias.clone(), a.clone()]; + let text = "@daily cd ~/alias && thirdshift pickup\n@hourly cd ~/a && thirdshift secure >> later.log\n@daily cd ~/b && thirdshift architect > first.log\n@daily cd ~/b && thirdshift architect > first.log\n"; + let captured = Captured::new(&config, home.path(), Ok(cronjobs::parse(text))); + let entries = captured.entries().unwrap(); + assert_eq!(entries.len(), 4); + assert_eq!(entries[0].directory.as_ref().unwrap().path, a); + assert!(entries[0].launch.as_ref().unwrap().command.is_some()); + assert_eq!( + entries[1].launch.as_ref().unwrap().output_log, + Some(a.join("later.log")) + ); + let directories = &captured.directories().unwrap().directories; + assert_eq!( + directories.iter().map(|d| &d.path).collect::>(), + [&a, &b] + ); + assert_eq!(directories[0].output_log, None); + assert_eq!(directories[1].output_log, Some(b.join("first.log"))); + let jobs = &captured.cronjobs().unwrap().jobs; + assert_eq!( + jobs.iter() + .map(|job| job.schedule.as_str()) + .collect::>(), + ["@daily", "@daily", "@daily", "@hourly"] + ); + assert_eq!(jobs[0].repository.as_deref(), Some("acme/a")); + assert_eq!(jobs[2].repository.as_deref(), Some("acme/z")); + + let text = "@daily cd ~/alias && thirdshift pickup >> output.log\n@daily cd ~/a && thirdshift pickup\n"; + let captured = Captured::new(&config, home.path(), Ok(cronjobs::parse(text))); + assert_eq!( + captured.directories().unwrap().directories[0].output_log, + Some(alias.join("output.log")) + ); + // A captured checkout remains an observation; the next operation sees edits. + Git::new(&a) + .run(&[ + "remote", + "set-url", + "origin", + "https://github.com/acme/changed.git", + ]) + .unwrap(); + assert_eq!( + captured.cronjobs().unwrap().jobs[0].repository.as_deref(), + Some("acme/z") + ); + let fresh = Captured::new(&config, home.path(), Ok(cronjobs::parse(text))); + assert_eq!( + fresh.cronjobs().unwrap().jobs[0].repository.as_deref(), + Some("acme/changed") + ); +} + +#[test] +fn unavailable_checkout_observations_are_shared_with_configured_validation() { + let home = tempfile::tempdir().unwrap(); + let invalid = checkout( + home.path(), + "invalid", + "https://gitlab.com/acme/widgets.git", + ); + let missing = home.path().join("missing"); + symlink(&invalid, home.path().join("alias")).unwrap(); + let mut config = config(home.path()); + let text = + "@daily cd ~/missing && thirdshift pickup\n@daily cd ~/invalid && thirdshift pickup\n"; + config.launch_directories = vec![missing.clone()]; + let captured = Captured::new(&config, home.path(), Ok(cronjobs::parse(text))); + checkout( + home.path(), + "missing", + "https://github.com/acme/widgets.git", + ); + assert!( + captured + .cronjobs() + .unwrap() + .jobs + .iter() + .all(|job| job.repository.is_none()) + ); + assert!( + captured + .directories() + .err() + .unwrap() + .to_string() + .contains("invalid configured Launch directory") + ); + + config.launch_directories = vec![home.path().join("alias")]; + let captured = Captured::new(&config, home.path(), Ok(cronjobs::parse(text))); + Git::new(&invalid) + .run(&[ + "remote", + "set-url", + "origin", + "https://github.com/acme/widgets.git", + ]) + .unwrap(); + assert!( + captured + .directories() + .err() + .unwrap() + .to_string() + .contains("not on GitHub") + ); + assert_eq!(captured.cronjobs().unwrap().jobs.len(), 2); + let fresh = Captured::new(&config, home.path(), Ok(cronjobs::parse(text))); + assert_eq!(fresh.directories().unwrap().directories.len(), 2); +} + +#[test] +fn projection_errors_preserve_strict_cron_and_missing_tool_discovery() { + let home = tempfile::tempdir().unwrap(); + let path = checkout(home.path(), "repo", "https://github.com/acme/widgets.git"); + let mut config = config(home.path()); + config.launch_directories = vec![path]; + for kind in [ + std::io::ErrorKind::NotFound, + std::io::ErrorKind::PermissionDenied, + ] { + let error = + Err(std::io::Error::new(kind, "fixture crontab error")).context("can't read cron jobs"); + let captured = Captured::new(&config, home.path(), error); + assert!(captured.cronjobs().is_err()); + assert_eq!( + captured.directories().is_ok(), + kind == std::io::ErrorKind::NotFound + ); + if kind == std::io::ErrorKind::NotFound { + assert!(captured.entries().unwrap().is_empty()); + assert_eq!(captured.directories().unwrap().directories.len(), 1); + } + } + let captured = Captured::new(&config, home.path(), Ok(Vec::new())); + assert!(captured.cronjobs().unwrap().jobs.is_empty()); + assert_eq!(captured.directories().unwrap().directories.len(), 1); +} diff --git a/src/logs.rs b/src/logs.rs index 062cea47f..1705d21ca 100644 --- a/src/logs.rs +++ b/src/logs.rs @@ -197,6 +197,12 @@ pub enum Begin<'a> { /// The work a command starts. #[derive(Clone, Copy)] pub enum Work<'a> { + /// A Command still in preflight, retained only because it hit a Quota wait. + Starting { + repo: &'a Repo, + command: CommandKind, + issue: Option<&'a IssueUrl>, + }, /// A Run on its issue. Run(&'a IssueUrl), /// A Base fix, a child Run into the Run's Base branch. @@ -290,6 +296,7 @@ const PARENT_DISPATCH: &str = "THIRDSHIFT_PARENT_DISPATCH"; /// shows them, so a skipped pass may print nothing. A child Run prints no /// such line, and takes the stamp it was given. pub fn begin(begin: Begin) { + crate::github::quota::command::begin(); let mut record = record(); if matches!(begin, Begin::ChildRun(..)) { record.inherit_owner(owner::Owner::inherited()); @@ -297,6 +304,19 @@ pub fn begin(begin: Begin) { record.child_dispatch(std::env::var(PARENT_DISPATCH).ok()); } record.begin(begin); + drop(record); + attach_quota_budget(); +} + +fn attach_quota_budget() { + if let Ok(owner) = captured_owner() + && let Some(path) = owner.log_path() + && let Err(error) = crate::github::quota::command::attach(path.into()) + { + crate::progress::step(format!( + "warning: could not attach Command quota budget: {error:#}" + )); + } } /// Give a child Run its exact parent, independently of second-resolution stamps. @@ -322,11 +342,17 @@ pub(crate) fn tickets(tickets: Vec) { /// `activity.quiet_skips` is set, show the lines a pass held from the /// terminal and every line from here on. With it set, a pass's lines stay /// held until it starts work, ends or fails, so a skipped pass prints -/// nothing. +/// nothing. Capture the Command's effective waiting-line interval before +/// any waiting stages begin. pub fn configured(config: &UserConfig) { + crate::poll::configure_waiting_lines(config.effective_waiting_line_interval()); record().configured(config); } +pub(crate) fn preflight_issue(issue: &IssueUrl) { + record().preflight_issue(issue.clone()); +} + /// Keep the Pass's latest start once its repository is known, before gates. pub(crate) fn pass_fired(repo: &Repo) { record().pass_fired(repo); @@ -341,6 +367,7 @@ pub(crate) fn pass_fired(repo: &Repo) { /// immediate parent, with no Command log of its own. pub fn started(work: Work, harness: &Choice) { record().started(work, harness); + attach_quota_budget(); } /// Capture the Custom process's loaded source before recording its start or skip. @@ -366,9 +393,15 @@ pub fn skipped(pass: Pass, repo: &Repo, reason: impl Display) { record().skipped(pass, repo, reason); } +/// Record a skipped Base branch CI step before the Fix Pass starts a Run. +pub(crate) fn base_ci_skipped(repo: &Repo, reason: &str) { + record().base_ci_skipped(repo, reason); +} + /// Record that the Custom process `name`, started with no issue, was skipped /// on `repo` for `reason`. The Activity log keeps `Process skipped: -/// ` unless that is already its last Process line. Unlike the other +/// ` unless that is already its last Process line (its own named +/// Process's last skip during a Quota wait). Unlike the other /// Passes, it also keeps a Command record whose outcome is `skipped`: the /// Process's record shows that ending. `harness` is the choice it would have /// run on; the skip does not check that it can run. @@ -376,9 +409,9 @@ pub fn skipped_process( repo: &Repo, name: &str, harness: &crate::harness::Choice, - reason: impl Display, + reason: &crate::command_limit::NotAdmitted, ) { - record().skipped_process(repo, name, harness, &reason.to_string()); + record().skipped_process(repo, name, harness, reason); } /// Add an observation to the running command's repository Activity log. @@ -408,6 +441,18 @@ pub(crate) fn waiting_for(commands: &[String]) { record().waiting_for(commands); } +/// Temporarily overlay the active stage without resetting its timing. +pub(crate) fn quota_wait(until: Option<&str>) { + if until.is_some() && record().needs_quota_repo() { + let repo = crate::launch::repo().ok(); + record().preflight_repo(repo); + } + record().quota_wait(until); + if until.is_some() { + attach_quota_budget(); + } +} + /// Keep the current Session trace, including a Resume's own log, as /// `session` enters its stage. pub fn session(session: Logged, path: &Path) { @@ -424,6 +469,11 @@ pub(crate) fn pull_request(url: &str) { record().pull_request(url); } +/// The selected Base branch of a Fix Pass's CI repair, before a PR exists. +pub(crate) fn base_branch(branch: &str) { + record().base_branch(branch); +} + /// Retain Delivery's final counts for the active Run's completion metrics. pub(crate) fn unaddressed_findings(counts: Option) { record().unaddressed_findings(counts); @@ -591,6 +641,7 @@ impl<'a> Work<'a> { fn process(self) -> crate::processes::Identity { use crate::processes::Identity; let file = match self { + Self::Starting { .. } => return Identity::default(), Self::Run(_) | Self::BaseFix(_) => "ticket-run", Self::SpecRun(_) => "spec-run", Self::ArchitectRun(_) => "architect-run", @@ -608,6 +659,7 @@ impl<'a> Work<'a> { fn initial_stage(self) -> Option { match self { + Work::Starting { .. } => None, Work::Run(_) | Work::BaseFix(_) => Some(Stage::Implement), Work::SpecRun(_) => Some(Stage::Tickets), Work::ArchitectRun(_) => Some(Stage::ArchitectureReview), @@ -623,6 +675,7 @@ impl<'a> Work<'a> { /// The repository whose work it records. fn repo(self) -> Repo { match self { + Work::Starting { repo, .. } => repo.clone(), Work::Run(issue) | Work::BaseFix(issue) | Work::SpecRun(issue) => issue.repo(), Work::PickupRun(listed) => listed.issue.repo(), Work::ArchitectRun(repo) @@ -639,6 +692,18 @@ impl<'a> Work<'a> { /// Its kind, in the Activity log. fn kind(self) -> Kind { match self { + Work::Starting { command, .. } => match command { + CommandKind::Issue => Kind::Run, + CommandKind::Architect => Kind::Pass(Pass::Architect), + CommandKind::Pickup => Kind::Pass(Pass::Pickup), + CommandKind::Secure => Kind::Pass(Pass::Security), + CommandKind::Scoring => Kind::ScoringRun, + CommandKind::Review => Kind::ReviewRun, + CommandKind::Fix => Kind::FixRun, + CommandKind::Triage => Kind::TriageRun, + CommandKind::Experiment => Kind::Experiment, + CommandKind::Process => Kind::Process, + }, Work::Run(_) | Work::BaseFix(_) => Kind::Run, Work::SpecRun(_) => Kind::SpecRun, Work::PickupRun(_) => Kind::Pass(Pass::Pickup), @@ -656,6 +721,7 @@ impl<'a> Work<'a> { /// Its issue, if it has one. pub(crate) fn issue_url(self) -> Option<&'a IssueUrl> { match self { + Work::Starting { issue, .. } => issue, Work::Run(issue) | Work::BaseFix(issue) | Work::SpecRun(issue) => Some(issue), Work::PickupRun(listed) => Some(&listed.issue), Work::ArchitectRun(_) diff --git a/src/logs/activity.rs b/src/logs/activity.rs index 6bdb6c857..88ac853b7 100644 --- a/src/logs/activity.rs +++ b/src/logs/activity.rs @@ -23,6 +23,7 @@ pub(super) enum Kind { ScoringRun, ReviewRun, FixRun, + BaseCi, TriageRun, Experiment, Process, @@ -39,6 +40,7 @@ impl Display for Kind { Kind::ScoringRun => f.write_str("Scoring run"), Kind::ReviewRun => f.write_str("Review run"), Kind::FixRun => f.write_str("Fix run"), + Kind::BaseCi => f.write_str("Base branch CI step"), Kind::TriageRun => f.write_str("Triage run"), Kind::Experiment => f.write_str("Experiment"), Kind::Process => f.write_str("Process"), @@ -76,10 +78,26 @@ pub(super) fn tail(path: &Path, limit: u64) -> Result { /// The time and unstamped text of the last line of `kind` in the tail. pub(super) fn last_of(tail: &str, kind: Kind) -> Option<(&str, &str)> { let kind = kind.to_string(); - tail.lines().rev().find_map(|line| { - let unstamped = line.get(TIME_FORMAT_LENGTH + 1..)?; - let rest = unstamped.strip_prefix(&kind)?; - (rest.starts_with(' ') || rest.starts_with(':')) - .then_some((line.get(..TIME_FORMAT_LENGTH)?, unstamped)) + lines(tail).rev().find(|(_, text)| { + text.strip_prefix(&kind) + .is_some_and(|rest| rest.starts_with(' ') || rest.starts_with(':')) + }) +} + +/// Compare the complete Process name: names can contain spaces and colons. +pub(super) fn last_process_skip<'a>(tail: &'a str, name: &str) -> Option<(&'a str, &'a str)> { + lines(tail).rev().find(|(_, text)| { + text.rsplit_once(" skipped: ") + .and_then(|(subject, _)| subject.strip_prefix("Process ")) + == Some(name) + }) +} + +fn lines(tail: &str) -> impl DoubleEndedIterator { + tail.lines().filter_map(|line| { + Some(( + line.get(..TIME_FORMAT_LENGTH)?, + line.get(TIME_FORMAT_LENGTH + 1..)?, + )) }) } diff --git a/src/logs/command_record.rs b/src/logs/command_record.rs index a8a8e2404..4605de6d4 100644 --- a/src/logs/command_record.rs +++ b/src/logs/command_record.rs @@ -104,6 +104,8 @@ pub(super) struct CommandRecord { #[serde(skip)] pub end_time_estimated: bool, pub stages: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub quota_wait_until: Option, pub session_log: Option, pub outcome: Option, #[serde(default)] @@ -116,6 +118,9 @@ pub(super) struct CommandRecord { #[serde(default, skip_serializing_if = "Option::is_none")] pub resume_command: Option, pub issue_branch: Option, + /// The selected Base branch for a Fix Pass repairing its CI, not its repair branch. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub base_branch: Option, pub pull_request: Option, #[serde(default)] pub parent_record: Option, @@ -259,6 +264,7 @@ impl StageName { /// The stage a pass enters while `work`, the Command it dispatched, runs. pub(super) fn dispatched(work: Work<'_>) -> Self { match work { + Work::Starting { .. } => Self::ProcessStep, Work::SpecRun(_) => Self::DispatchedSpecRun, Work::Run(_) | Work::BaseFix(_) @@ -365,11 +371,15 @@ impl CommandRecord { ) { self.ended_at = Some(outside.now().to_rfc3339()); self.outcome = Some(account.outcome.into()); - self.cause = account - .ended - .as_ref() - .err() - .map(|cause| cause.full().into()); + // A Quota wait may already have opened this record. Pre-audit GitHub + // diagnostics stay on the terminal; the record keeps the fixed cause. + self.cause = account.pre_audit_failure.map(str::to_owned).or_else(|| { + account + .ended + .as_ref() + .err() + .map(|cause| cause.full().to_owned()) + }); self.pull_request = account.pr_url.map(String::from); self.security_findings_waiting = account.security_findings_waiting; self.security_findings = account.security_findings.unwrap_or_default().to_vec(); @@ -396,7 +406,8 @@ impl CommandRecord { // The Ready issue listing named a Pickup run's issue before it started. title: match work { Work::PickupRun(listed) => Some(listed.title.clone()), - Work::Run(_) + Work::Starting { .. } + | Work::Run(_) | Work::BaseFix(_) | Work::SpecRun(_) | Work::ArchitectRun(_) @@ -410,6 +421,10 @@ impl CommandRecord { }, }); let name = match work { + Work::Starting { command, issue, .. } => match issue { + Some(issue) => format!("{}#{}", issue.repo_slug(), issue.number), + None => format!("{}-{stamp}", command.folder()), + }, Work::ArchitectRun(_) => format!("architect-{stamp}"), Work::SecurityRun(_) => format!("secure-{stamp}"), Work::ScoringRun(_) => format!("scoring-{stamp}"), @@ -444,6 +459,7 @@ impl CommandRecord { ended_at: None, end_time_estimated: false, stages: Vec::new(), + quota_wait_until: None, session_log: None, outcome: None, run_metrics: None, @@ -452,6 +468,7 @@ impl CommandRecord { limit: None, resume_command: None, issue_branch: None, + base_branch: None, pull_request: None, parent_record: None, parent_dispatch: None, diff --git a/src/logs/github.rs b/src/logs/github.rs index da122f790..6a28cd38b 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 @@ -185,9 +183,13 @@ impl Kept { .map(|at| at.to_rfc3339_opts(SecondsFormat::Secs, true)) } + pub fn quota_wait(&self) -> Option { + QuotaWait::pass_reason(&self.waits) + } + 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) @@ -208,10 +210,23 @@ impl Kept { }) } - /// An open Inbox must also learn when a failed answer becomes stale. + /// An open Inbox must learn when an answer becomes stale or a Quota wait resets. pub fn next_warning_change(&self) -> Option { - let age = self.age?; - (age <= FRESH_FOR).then(|| FRESH_FOR - age + Duration::from_millis(1)) + let freshness = self + .age + .and_then(|age| (age <= FRESH_FOR).then(|| FRESH_FOR - age + Duration::from_millis(1))); + let now = Utc::now(); + let reset = self + .waits + .iter() + .filter_map(|wait| { + (wait.reset_at() - now) + .to_std() + .ok() + .filter(|delay| !delay.is_zero()) + }) + .min(); + freshness.into_iter().chain(reset).min() } fn state(&self, row: &str, url: &str) -> Option<&InboxState> { @@ -306,10 +321,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 +378,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 +400,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 +428,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 +447,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 +492,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 +501,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 +531,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/src/logs/owner.rs b/src/logs/owner.rs index ee17ede39..4786e8689 100644 --- a/src/logs/owner.rs +++ b/src/logs/owner.rs @@ -136,6 +136,12 @@ pub(super) fn allocate( stamp: &str, ) -> Result> { let (kind, subject) = match work { + Work::Starting { command, issue, .. } => ( + command, + issue + .map(|issue| format!("{}-", issue.number)) + .unwrap_or_default(), + ), Work::Run(issue) | Work::BaseFix(issue) | Work::SpecRun(issue) => { (CommandKind::Issue, format!("{}-", issue.number)) } diff --git a/src/logs/reading.rs b/src/logs/reading.rs index 4128003fe..1557e9281 100644 --- a/src/logs/reading.rs +++ b/src/logs/reading.rs @@ -444,13 +444,26 @@ impl Command { } else { "died" }; - let stage = record.current_stage().map(|stage| CurrentStage { - name: stage.name.clone(), - began_at: stage.began_at.clone(), - age_seconds: DateTime::parse_from_rfc3339(&stage.began_at) - .map(|began| now.signed_duration_since(began).num_seconds().max(0)) - .unwrap_or(0), - }); + let stage = record + .current_stage() + .map(|stage| CurrentStage { + name: record + .quota_wait_until + .as_ref() + .map(|until| format!("Quota wait until {until}")) + .unwrap_or_else(|| stage.name.clone()), + began_at: stage.began_at.clone(), + age_seconds: DateTime::parse_from_rfc3339(&stage.began_at) + .map(|began| now.signed_duration_since(began).num_seconds().max(0)) + .unwrap_or(0), + }) + .or_else(|| { + record.quota_wait_until.as_ref().map(|until| CurrentStage { + name: format!("Quota wait until {until}"), + began_at: record.started_at.clone(), + age_seconds: 0, + }) + }); Self { account: None, activity: captured_activity::Activity::new(log, record.session_log.as_deref(), now), diff --git a/src/logs/reading/health.rs b/src/logs/reading/health.rs index ebbfa869f..e9c62c4ce 100644 --- a/src/logs/reading/health.rs +++ b/src/logs/reading/health.rs @@ -80,7 +80,11 @@ pub(super) fn observe( if let Some(stage) = &command.stage && !StageName::is_dispatch(&stage.name) { - if let Some(range) = ranges.get(&command.record.repository, &stage.name) { + if let Some(range) = command + .record + .current_stage() + .and_then(|underlying| ranges.get(&command.record.repository, &underlying.name)) + { // The first Slow step starts strictly above the upper range; // the second starts at twice that range, on sampled seconds. for threshold in [ diff --git a/src/logs/reading/tests.rs b/src/logs/reading/tests.rs index 96cf5224c..7d53b338e 100644 --- a/src/logs/reading/tests.rs +++ b/src/logs/reading/tests.rs @@ -66,6 +66,39 @@ fn retained_pull_request_lookup_orders_instants_across_offsets() { ); } +#[test] +fn quota_wait_keeps_slow_timing_on_the_underlying_stage() { + let dir = tempfile::tempdir().unwrap(); + for number in 1..=8 { + let mut past = record(number); + past["outcome"] = "ready for review".into(); + past["ended_at"] = "2026-10-19T11:01:00-04:00".into(); + past["stages"] = json!([{"name":"CI watch", "began_at":"2026-10-19T11:00:00-04:00", "session_log":null}]); + write(dir.path(), &format!("{number}-past.command.json"), &past); + } + let mut live = record(9); + live["stages"] = + json!([{"name":"CI watch", "began_at":"2026-10-19T11:30:00-04:00", "session_log":null}]); + live["quota_wait_until"] = "2026-10-19T12:01:37-04:00".into(); + write(dir.path(), "9-live.command.json", &live); + fs::write( + dir.path().join("acme/widgets/commands/issue/9-live.log"), + "still waiting\n", + ) + .unwrap(); + let snapshot = Snapshot::load(dir.path(), now()).unwrap(); + let selected = snapshot.resolve("widgets#9").unwrap(); + let Observation::Record(command) = selected.observation else { + panic!("record-backed Command") + }; + assert_eq!( + command.stage.as_ref().unwrap().name, + "Quota wait until 2026-10-19T12:01:37-04:00" + ); + assert_eq!(command.stage.as_ref().unwrap().age_seconds, 1800); + assert!(matches!(command.health, Some(health::Health::Slow { .. }))); +} + #[test] fn direct_run_selection_captures_its_owner_and_reloads_retained_work() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/logs/recording.rs b/src/logs/recording.rs index be15ffca8..f13deeb22 100644 --- a/src/logs/recording.rs +++ b/src/logs/recording.rs @@ -37,6 +37,9 @@ pub(super) struct Record { /// repository has them. config_warnings: Vec, command_record: Option<(PathBuf, CommandTree)>, + preflight_repo: Option, + preflight_issue: Option, + quota_record: bool, custom_process: Option, parent_record: Option, parent_dispatch: Option, @@ -69,6 +72,11 @@ struct Started { process: Option, } +enum History<'a> { + Kind(Kind), + ProcessSkip(&'a str), +} + impl Started { fn subject(&self) -> String { if let Some(name) = &self.process { @@ -98,6 +106,9 @@ impl Record { activity_warned: false, config_warnings: Vec::new(), command_record: None, + preflight_repo: None, + preflight_issue: None, + quota_record: false, custom_process: None, parent_record: None, parent_dispatch: None, @@ -107,6 +118,9 @@ impl Record { } pub(super) fn begin(&mut self, begin: Begin) { + if let Begin::Run(issue) = &begin { + self.preflight_issue((*issue).clone()); + } self.command = match &begin { Begin::Run(_) => CommandKind::Issue, Begin::ChildRun(_, command) => *command, @@ -166,6 +180,21 @@ impl Record { self.logs_dir.clone() } + pub(super) fn preflight_repo(&mut self, repo: Option) { + if self.preflight_repo.is_none() { + self.preflight_repo = repo; + } + } + + pub(super) fn needs_quota_repo(&self) -> bool { + self.command_record.is_none() && self.preflight_repo.is_none() + } + + pub(super) fn preflight_issue(&mut self, issue: crate::issue::IssueUrl) { + self.preflight_repo = Some(issue.repo()); + self.preflight_issue = Some(issue); + } + /// Hand on the progress line `message`, to the terminal even while a /// quiet pass holds its other lines, and to the Command log as any /// other. @@ -259,7 +288,7 @@ impl Record { let stamp = self.stamp(); let began = matches!(self.command_log, Log::Held { .. }); self.keep(&root, work, &stamp); - if began { + if began || self.quota_record { self.start_record(work, harness); } self.step(format_args!("sessions run on {harness}")); @@ -277,7 +306,8 @@ impl Record { issue: work.issue(), process: match work { Work::Process { name, .. } => Some((*name).to_string()), - Work::Run(_) + Work::Starting { .. } + | Work::Run(_) | Work::BaseFix(_) | Work::SpecRun(_) | Work::PickupRun(_) @@ -339,6 +369,12 @@ impl Record { fn start_record(&mut self, work: Work, harness: &Choice) { let stamp = self.stamp(); let mut facts = CommandRecord::new(work, &stamp, harness, self.outside.now()); + if self.quota_record + && let Some((_, tree)) = &self.command_record + { + facts.started_at = tree.command.started_at.clone(); + } + self.quota_record = false; if matches!(work, Work::Process { .. }) && let Some(identity) = &self.custom_process { @@ -388,6 +424,9 @@ impl Record { } pub(super) fn skipped(&mut self, pass: Pass, repo: &Repo, reason: impl Display) { + if self.quota_record { + self.outcome("skipped", Some(&reason.to_string())); + } let kind = Kind::Pass(pass); self.activity_write( &self.root(repo), @@ -396,6 +435,13 @@ impl Record { ); } + pub(super) fn base_ci_skipped(&mut self, repo: &Repo, reason: &str) { + let kind = Kind::BaseCi; + let message = format!("{kind} skipped: {reason}"); + self.step(&message); + self.activity_write(&self.root(repo), &message, Some(kind)); + } + /// A skipped Custom process keeps the Activity log line and a Command /// record. The record is what shows the skipped ending; the other Passes /// drop the Command log they never kept. @@ -404,21 +450,28 @@ impl Record { repo: &Repo, name: &str, harness: &Choice, - reason: &str, + reason: &crate::command_limit::NotAdmitted, ) { let root = self.root(repo); self.record_config_warnings(&root); - self.activity_write( + // Quota waits compare this named Process's last reason even when + // another Custom process fires between its Passes. + let history = if reason.is_quota_wait() { + History::ProcessSkip(name) + } else { + History::Kind(Kind::Process) + }; + self.activity_write_with_history( &root, &format!("Process {name} skipped: {reason}"), - Some(Kind::Process), + Some(history), ); let stamp = self.stamp(); // Leave held lines unshown, so a quiet skip prints nothing. They are // still the Command log. self.retain(&root, Work::Process { repo, name }, &stamp, false); self.start_record(Work::Process { repo, name }, harness); - self.outcome("skipped", Some(reason)); + self.outcome("skipped", Some(&reason.to_string())); } pub(super) fn activity(&mut self, message: impl Display) { @@ -457,6 +510,13 @@ impl Record { } } + pub(super) fn base_branch(&mut self, branch: &str) { + if let Some((_, tree)) = &mut self.command_record { + tree.active().base_branch = Some(branch.into()); + self.write_record(); + } + } + pub(super) fn pull_request(&mut self, url: &str) { if let Some((_, tree)) = &mut self.command_record { tree.active().pull_request = Some(url.into()); @@ -485,6 +545,36 @@ impl Record { self.write_record(); } + pub(super) fn quota_wait(&mut self, until: Option<&str>) { + if until.is_some() + && self.command_record.is_none() + && let Some(repo) = self.preflight_repo.clone() + && self.logs_dir.is_some() + && (!self.child || self.preflight_issue.is_some()) + { + let issue = self.preflight_issue.clone(); + let work = Work::Starting { + repo: &repo, + command: self.command, + issue: issue.as_ref(), + }; + if !self.child { + let stamp = self.stamp(); + self.keep(&self.root(&repo), work, &stamp); + } + self.start_record(work, &Choice::default()); + if let Some((_, tree)) = &mut self.command_record { + // No Harness has been selected or launched during preflight. + tree.command.harness = "not started".into(); + } + self.quota_record = true; + } + if let Some((_, tree)) = &mut self.command_record { + tree.active().quota_wait_until = until.map(str::to_owned); + self.write_record(); + } + } + pub(super) fn unaddressed_findings( &mut self, counts: Option, @@ -822,6 +912,15 @@ impl Record { } fn activity_write(&mut self, root: &Path, line: &str, unless_last_of: Option) { + self.activity_write_with_history(root, line, unless_last_of.map(History::Kind)); + } + + fn activity_write_with_history( + &mut self, + root: &Path, + line: &str, + unless_last_of: Option>, + ) { let appended = self.activity_append(&root.join(activity::FILE), line, unless_last_of); if let Err(error) = appended && !self.activity_warned @@ -838,9 +937,9 @@ impl Record { &mut self, path: &Path, line: &str, - unless_last_of: Option, + unless_last_of: Option>, ) -> Result<()> { - if let Some(kind) = unless_last_of { + if let Some(history) = unless_last_of { let tail = self.outside.activity_tail(path, activity::TAIL)?; let text = String::from_utf8_lossy(&tail.bytes); let whole = if tail.from > 0 { @@ -848,7 +947,11 @@ impl Record { } else { &text }; - if activity::last_of(whole, kind).map(|(_, text)| text) == Some(line) { + let last = match history { + History::Kind(kind) => activity::last_of(whole, kind), + History::ProcessSkip(name) => activity::last_process_skip(whole, name), + }; + if last.map(|(_, text)| text) == Some(line) { return Ok(()); } } diff --git a/src/logs/recording/tests.rs b/src/logs/recording/tests.rs index d2540c56d..959f626b5 100644 --- a/src/logs/recording/tests.rs +++ b/src/logs/recording/tests.rs @@ -292,6 +292,36 @@ fn a_stop_request_in_the_file_adapter_is_logged_before_the_ending() { assert!(stopped < activity.find("Run #7 ended: interrupted").unwrap()); } +#[test] +fn quota_wait_restores_the_previous_stage_without_restarting_its_clock() { + let memory = Scripted::default(); + let mut record = Record::new(memory.clone()); + record.begin(Begin::Run(&issue())); + record.configured(&config(false)); + record.started(Work::Run(&issue()), &harness()); + record.stage(StageName::CiWatch, None); + let stages_before = memory.command_record().stages.len(); + let began = memory + .command_record() + .current_stage() + .unwrap() + .began_at + .clone(); + record.quota_wait(Some("2026-11-11T12:01:37Z")); + let waiting = memory.command_record(); + assert_eq!( + waiting.quota_wait_until.as_deref(), + Some("2026-11-11T12:01:37Z") + ); + assert_eq!(waiting.current_stage().unwrap().began_at, began); + record.quota_wait(None); + let resumed = memory.command_record(); + assert!(resumed.quota_wait_until.is_none()); + assert_eq!(resumed.current_stage().unwrap().name, "CI watch"); + assert_eq!(resumed.current_stage().unwrap().began_at, began); + assert_eq!(resumed.stages.len(), stages_before); +} + #[test] fn a_command_record_write_failure_warns_once_and_keeps_the_outcome() { let memory = Scripted::default(); diff --git a/src/logs/status.rs b/src/logs/status.rs index 06f3f4174..3acfa0a38 100644 --- a/src/logs/status.rs +++ b/src/logs/status.rs @@ -29,6 +29,7 @@ struct Status<'a> { commands: Vec>, idle_passes: &'a [IdlePass], pass_starts: &'a [PassStart], + github_quota_wait: Option, /// How old the Inbox's kept GitHub answer is, in seconds. `None` where /// no answer is kept: with no ready pull requests to read, or before /// the first read. @@ -71,6 +72,7 @@ impl Read { .into_rows(all), idle_passes: &self.snapshot.idle_passes, pass_starts: &self.snapshot.pass_starts, + github_quota_wait: self.kept.quota_wait(), github_answer_age_seconds: self.kept.age.map(|age| age.as_secs()), github_last_answered_at: self.kept.last_answered_at(), github_failure: self.kept.failure.as_deref(), @@ -261,6 +263,9 @@ pub fn show(json: bool, all: bool, machine: Option<&str>, local_only: bool) -> R for pass in status.pass_starts.iter().filter(|_| include_local) { println!("{}", pass.line()); } + if include_local && let Some(wait) = status.github_quota_wait { + println!("{wait}"); + } } Ok(()) } diff --git a/src/logs/status/inbox.rs b/src/logs/status/inbox.rs index d42cedaf7..d53f8a298 100644 --- a/src/logs/status/inbox.rs +++ b/src/logs/status/inbox.rs @@ -16,6 +16,7 @@ pub(crate) struct Inbox { pub sections: Vec
    , pub idle_passes: Vec, pub pass_starts: Vec, + pub github_quota_wait: Option, pub warning: Option<&'static str>, pub github_warning: Option, } @@ -47,6 +48,8 @@ pub(crate) struct Row { pub subject: String, /// The pull request or issue's `https://` address, which the subject links to. pub subject_url: Option, + pub target_pr: Option, + pub base_branch: Option, pub title: String, pub state: String, pub now_line: String, @@ -62,12 +65,19 @@ pub(crate) struct Row { pub pull_request: String, pub ci: Option, pub merge_conflict: bool, + pub ticket_progress: Option, pub tickets: Vec, pub children: Vec, pub findings: Vec, pub sessions: Vec, } +#[derive(Clone, Copy)] +pub(crate) struct TicketProgress { + pub complete: usize, + pub total: usize, +} + pub(crate) struct Timing { pub finish: Option, pub age: String, @@ -321,6 +331,7 @@ fn present(value: &serde_json::Value, now: DateTime) -> Result { command_log: Option, cause: Option, pull_request: Option, + base_branch: Option, harness: Option, model: Option, effort: Option, @@ -374,7 +385,10 @@ fn present(value: &serde_json::Value, now: DateTime) -> Result { .unwrap_or_default(); let process = (facts.kind == "Process").then(|| facts.process_name.as_deref().unwrap_or(&facts.name)); - let name = if value["source"] == "command_record" { + let live_target = live && matches!(facts.kind.as_str(), "Fix run" | "Review run"); + let name = if live_target { + &facts.kind + } else if value["source"] == "command_record" { &facts.name } else { &facts.kind @@ -436,6 +450,13 @@ fn present(value: &serde_json::Value, now: DateTime) -> Result { logs_url: String::new(), subject, subject_url, + target_pr: facts + .pull_request + .as_deref() + .filter(|_| live_target) + .and_then(|url| crate::review::PullRequestUrl::parse(url).ok()) + .filter(|pr| pr.issue.repo_slug().eq_ignore_ascii_case(&facts.repository)), + base_branch: facts.base_branch.filter(|_| live_target), title, state, now_line: super::stage_details( @@ -469,6 +490,19 @@ fn present(value: &serde_json::Value, now: DateTime) -> Result { pull_request: facts.pull_request.unwrap_or_default(), ci: None, merge_conflict: false, + ticket_progress: (!facts.tickets.is_empty()).then(|| TicketProgress { + complete: facts + .tickets + .iter() + .filter(|ticket| { + matches!( + ticket.state, + crate::logs::command_record::TicketState::Merged + ) + }) + .count(), + total: facts.tickets.len(), + }), tickets: facts .tickets .iter() @@ -483,6 +517,14 @@ fn present(value: &serde_json::Value, now: DateTime) -> Result { findings: facts.security_findings, sessions: Vec::new(), }; + // Compact Inbox rows hide nested runs. Carry the dispatched Spec's + // recorded accounting onto its Pass without counting child attempts. + if row.ticket_progress.is_none() { + row.ticket_progress = row.children[..facts.dispatched.len()] + .iter() + .rev() + .find_map(|child| child.ticket_progress); + } if ready { row.present_ready(ready_title, facts.github_ci, facts.github_mergeable); } @@ -623,6 +665,8 @@ fn entry_row(entry: Observation<'_>, now: DateTime, root: &std::path::Pat .unwrap_or_else(|| "/?page=logs".into()), subject, subject_url: issue_url(command.issue.as_ref()), + target_pr: None, + base_branch: None, title, state: command.state.into(), now_line: command @@ -639,6 +683,7 @@ fn entry_row(entry: Observation<'_>, now: DateTime, root: &std::path::Pat pull_request: String::new(), ci: None, merge_conflict: false, + ticket_progress: None, tickets: Vec::new(), children: Vec::new(), findings: Vec::new(), @@ -739,6 +784,7 @@ pub(super) fn from_read( github_warning: kept.warning(), idle_passes: snapshot.idle_passes.clone(), pass_starts: snapshot.pass_starts.clone(), + github_quota_wait: kept.quota_wait(), warning: (config.effective_waiting_line_interval() != config.waiting_line_interval).then_some( "logs.waiting_line_minutes must be less than inbox.silent_minutes; using half the silent time for waiting lines"), }) @@ -1031,6 +1077,7 @@ mod tests { ], idle_passes: Vec::new(), pass_starts: Vec::new(), + github_quota_wait: None, warning: None, github_warning: None, }; diff --git a/src/main.rs b/src/main.rs index 5cdeeb471..78c76fce2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -201,7 +201,7 @@ usage: thirdshift Run the factory on the iss thirdshift experiments [] [--json] List Experiments or show one; --name renames, --record-difficulty records A/A Difficulty, --leak decides Session evidence hits, --pair reads a pair blind and records your calls thirdshift recheck |--pending|--batch ... Review or record Score rechecks; see the manual thirdshift status [--all] [--json] [--machine | --local] List Fleet or local Commands and named Experiment waits, also shown in the Console - thirdshift cronjobs | launch-directories [--json] List cron jobs or known Launch directories + thirdshift cronjobs [--fleet] [--json] | launch-directories [--json] List schedules or Launch directories thirdshift resources [--json] Show host memory, CPU load and session limits thirdshift start [options] [--machine ] Start detached work on a known repository thirdshift config [ []] [--json] [--machine ] Read or change User config @@ -288,7 +288,13 @@ fn main() -> ExitCode { } Ok(Command::Experiments(args)) => return printed(evals::experiment::show(&args)), Ok(Command::Recheck(args)) => return printed(evals::experiment::recheck(&args)), - Ok(Command::CronJobs { json }) => return printed(cronjobs::show(json)), + Ok(Command::CronJobs { json, fleet }) => { + return printed(if fleet { + cronjobs::fleet::show(json) + } else { + cronjobs::show(json) + }); + } Ok(Command::LaunchDirectories { json }) => return printed(launch_directories::show(json)), Ok(Command::Resources { json }) => return printed(resources::show(json)), Ok(Command::Config { key, value, json }) => { diff --git a/src/notification.rs b/src/notification.rs index b7d7296f5..f7672ab30 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -2,6 +2,7 @@ //! run, a Pass or a Process ends, unless the Pass or Process was skipped, //! and optionally emailed through the same checks and the same send as `email-test`. +use std::collections::HashSet; use std::path::Path; use std::time::{Duration, Instant}; @@ -235,6 +236,13 @@ impl RunNotification { host.as_deref().unwrap_or("unknown host"), self.started.elapsed(), ); + match crate::github::quota::command::notification_line() { + Ok(Some(line)) => text += &format!("\n{line}\n"), + Ok(None) => {} + Err(error) => progress::step(format!( + "warning: could not read Command quota waits: {error:#}" + )), + } if account.pre_audit_failure.is_some() { text += &format!( "Started (UTC): {}\nDiagnostics: Check this command's stderr on {} at that time (or the scheduler's captured stderr, if scheduled).\n", @@ -349,9 +357,10 @@ fn body( } if let Some(recommendation) = account.triage { text += &recommendation.notification; - } - for line in account.triage_publication_lines() { - text += &format!("{line}\n"); + } else { + for line in account.triage_publication_lines() { + text += &format!("{line}\n"); + } } if let Err(cause) = &account.ended && let Some(notification) = cause.private_triage_notification() @@ -479,7 +488,11 @@ fn body( if let Some(log) = account.log { text += &format!("Session trace: {}\n", log.display()); } - if let Some(log) = command_log { + // A pre-audit notification uses the fixed cause. The Command log path + // stays on this machine: the email goes through a third party. + if account.pre_audit_failure.is_none() + && let Some(log) = command_log + { text += &format!("Command log: {}\n", log.display()); } if let Some(configured) = configured { @@ -503,7 +516,19 @@ fn body( text += &format!("{line}\n"); } } - text + // Questionnaires and retained failure analyses can repeat publication + // records. Keep the first record for each published issue in this body. + let published = account.triage_publication_lines().collect::>(); + if published.is_empty() { + return text; + } + let mut shown = HashSet::new(); + text.split_inclusive('\n') + .filter(|line| { + let record = line.trim_end_matches('\n'); + !published.contains(record) || shown.insert(record) + }) + .collect() } /// Promote usage exhaustion out of a Spec's Ticket details without replacing diff --git a/src/pickup.rs b/src/pickup.rs index 1d15a1c11..f8d0e2f06 100644 --- a/src/pickup.rs +++ b/src/pickup.rs @@ -19,7 +19,7 @@ use anyhow::Result; use crate::asks::Flags; use crate::claim; -use crate::command_limit::{AlreadyRunning, Sharing}; +use crate::command_limit::{NotAdmitted, Sharing}; use crate::config::UserConfig; use crate::github::{GitHub, ListedIssue}; use crate::harness::Choice; @@ -54,9 +54,10 @@ pub struct Took { /// Why a Pickup run was skipped. Its `Display` is the reason, as the skipped /// run's one line gives it. pub enum Skipped { - /// The repository is at its Command limit on this machine, each Pass's + /// GitHub is in a Quota wait, or the repository is at its Command limit + /// on this machine, each Pass's /// dispatched Spec run or Run counted as part of it. - AlreadyRunning(AlreadyRunning), + NotAdmitted(NotAdmitted), /// The repository is at its Failure limit: `failed` lists the open /// issues labelled `agent-failed` that still wait for the Day shift, /// lowest number first, and `limit` or more stop a Pickup run from @@ -77,7 +78,7 @@ pub enum Skipped { impl fmt::Display for Skipped { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::AlreadyRunning(running) => running.fmt(f), + Self::NotAdmitted(running) => running.fmt(f), Self::AtFailureLimit { repo, failed, @@ -172,9 +173,9 @@ pub fn run( base, } = match launch::start(base, config.command_limit, Sharing::UpToTheLimit)? { Start::Clear(launch) => launch, - Start::AlreadyRunning(running) => { + Start::NotAdmitted(running) => { logs::skipped(Pass::Pickup, &running.repo, &running); - return Ok(Outcome::Skipped(Skipped::AlreadyRunning(running))); + return Ok(Outcome::Skipped(Skipped::NotAdmitted(running))); } }; // Only Pickup handles Base branch CI reporting. A CI or publication failure diff --git a/src/poll.rs b/src/poll.rs index 722d2c558..49cdf90f8 100644 --- a/src/poll.rs +++ b/src/poll.rs @@ -41,6 +41,15 @@ impl WaitingLine { } } + /// A waiting line for a state whose first line was already written: + /// it repeats only after the interval. + pub fn already_reported(state: T) -> Self { + Self { + reported: Some(state), + ..Self::new() + } + } + pub fn should_report(&mut self, state: T) -> bool { if self.reported.as_ref() != Some(&state) || self.last_line.elapsed() >= self.interval { self.reported = Some(state); diff --git a/src/processes.rs b/src/processes.rs index 57d0f4716..10e120078 100644 --- a/src/processes.rs +++ b/src/processes.rs @@ -636,6 +636,14 @@ fn run(command: &ProcessRun) -> Result { Ok(issue) => issue, Err(error) => return Err(failed_before_steps(notification, error)), }; + if let Some(issue) = &issue { + // Issue validation, branch selection and Claim creation all use + // GitHub before the first Step. They share the Command's wait budget + // and must already have a visible, stoppable record when they wait. + logs::begin(Begin::Process { name, pass: false }); + logs::configured(&config); + logs::preflight_issue(issue); + } if let Err(error) = require_issue(&path, &file, issue.as_ref()) { return Err(failed_before_steps(notification, error)); } @@ -648,25 +656,30 @@ fn run(command: &ProcessRun) -> Result { if issue.is_none() { logs::begin(Begin::Process { name, pass: true }); logs::configured(&config); - let base = match launch::checked_out_base() { - Ok(base) => base, - Err(error) => return Err(failed_before_steps(notification, error)), - }; let repo = match launch::repo() { Ok(repo) => repo, Err(error) => return Err(failed_before_steps(notification, error)), }; + let skip = |reason: &command_limit::NotAdmitted| { + let choice = Choice::of(&command.asked, &config.harness); + progress::step(reason); + logs::skipped_process(&repo, name, &choice, reason); + String::new() + }; + if let Some(skipped) = command_limit::quota_wait(&repo, config.command_limit) { + return Ok(skip(&skipped)); + } + let base = match launch::checked_out_base() { + Ok(base) => base, + Err(error) => return Err(failed_before_steps(notification, error)), + }; match command_limit::hold_pass( &repo, config.command_limit, command_limit::Sharing::UpToTheLimit, ) { Ok(Some(running)) => { - let reason = running.to_string(); - let choice = Choice::of(&command.asked, &config.harness); - progress::step(&reason); - logs::skipped_process(&repo, name, &choice, &reason); - return Ok(String::new()); + return Ok(skip(&running)); } Ok(None) => {} Err(error) => return Err(failed_before_steps(notification, error)), @@ -738,17 +751,18 @@ fn run(command: &ProcessRun) -> Result { Ok(repo) => repo, Err(error) => return Err(failed_before_steps(notification, error)), }; - logs::begin(Begin::Process { name, pass: false }); - logs::configured(&config); finish_run(name, &repo, prepared, notification) } /// A stop after the email check and before any Step is still a failed Process. fn failed_before_steps(notification: RunNotification, error: anyhow::Error) -> anyhow::Error { let cause = format!("{error:#}"); + let outcome = failure_outcome(); + logs::ended(outcome); + logs::complete(outcome, Some(&cause)); notify( notification, - "failed", + outcome, Some(&cause), &CompletedRuns::default(), ); diff --git a/src/processes/review.rs b/src/processes/review.rs index 1272e6963..44cd55331 100644 --- a/src/processes/review.rs +++ b/src/processes/review.rs @@ -25,14 +25,20 @@ pub(crate) fn run(command: ReviewArgs) -> Result { crate::issue::Repo::of_origin(&launch.run(&["config", "remote.origin.url"])?) .context("review --author needs a GitHub origin")?; logs::pass_fired(&repo); + let skip = |reason: &dyn std::fmt::Display| { + logs::skipped(logs::Pass::Review, &repo, reason); + progress::step(reason); + String::new() + }; + if let Some(skipped) = command_limit::quota_wait(&repo, config.command_limit) { + return Ok(skip(&skipped)); + } if let Some(running) = command_limit::hold_pass( &repo, config.command_limit, command_limit::Sharing::UpToTheLimit, )? { - logs::skipped(logs::Pass::Review, &repo, &running); - progress::step(&running); - return Ok(String::new()); + return Ok(skip(&running)); } let github = GitHub::new(); let mut selected = None; @@ -49,9 +55,7 @@ pub(crate) fn run(command: ReviewArgs) -> Result { } let Some(selected) = selected else { let reason = format!("no waiting pull request by {author} on {}", repo.slug()); - logs::skipped(logs::Pass::Review, &repo, &reason); - progress::step(reason); - return Ok(String::new()); + return Ok(skip(&reason)); }; selected } diff --git a/src/processes/source.rs b/src/processes/source.rs index 0ed74b4b4..e1e60dae8 100644 --- a/src/processes/source.rs +++ b/src/processes/source.rs @@ -912,11 +912,8 @@ pub(super) fn run_issue_branch( } }; let mut held = Some(checkout); - // Session traces ask for the log root, which exists once this command is - // configured. The Command record stays the Process; the traces are named - // for the issue. - logs::begin(Begin::Process { name, pass: false }); - logs::configured(config); + // Recording began before preflight GitHub reads. The Command record + // stays the Process; the Session traces are named for the issue. if let Some(made) = &mut claim.claim { made.capture_private_reports(); } diff --git a/src/ready.rs b/src/ready.rs index 85d7a1983..555981397 100644 --- a/src/ready.rs +++ b/src/ready.rs @@ -79,7 +79,7 @@ pub struct ReadyIssue { pub enum PriorityIssue { Ready(ReadyIssue), /// Every other Ready issue condition holds, including a Spec having a - /// Startable Ticket. Pickup still cannot take it. + /// Startable Ticket or an all-closed continuation. Pickup still waits. Settling(ListedIssue), } @@ -191,6 +191,8 @@ trait Reads { fn candidate(&self, issue: &IssueUrl) -> Result; /// What shows `issue` was ever started, if it was. fn started(&self, issue: &IssueUrl) -> Result>; + /// Whether the latest Issue branch can be continued under ADR-0002. + fn can_continue(&self, issue: &IssueUrl) -> Result; /// The Tickets of `spec`, an issue with sub-issues, as a Spec run reads /// them. fn tickets(&self, spec: &IssueUrl) -> Result>; @@ -216,6 +218,10 @@ impl Reads for GitHubAndOrigin<'_> { branch::started(self.launch, &self.github, issue) } + fn can_continue(&self, issue: &IssueUrl) -> Result { + branch::can_continue(self.launch, &self.github, issue) + } + fn tickets(&self, spec: &IssueUrl) -> Result> { self.github.tickets(spec) } @@ -495,10 +501,10 @@ fn why_none_can_start(spec: &IssueUrl, tickets: &[Ticket]) -> String { /// Trusted accounts, it has no label that makes an Unready Ticket and no /// Claim, is not a sub-issue, has no `base-fix` or `security-fix` label and no /// open blocker, was never started or has a recorded ready-for-agent -/// hand-back after its last Claim ended, is not a Spec whose Tickets are all -/// closed, is settled: `wait` has passed since it was last labelled -/// `ready-for-agent` and since a sub-issue or a "blocked by" link of its was -/// last added or removed, and, if it is a Spec, has a startable Ticket. Its +/// hand-back after its last Claim ended, is settled: `wait` has passed since +/// it was last labelled `ready-for-agent` and since a sub-issue or a "blocked by" link of its was +/// last added or removed, and, if it is a Spec, has a Startable Ticket or +/// every Ticket closed with a Spec branch that can be continued. Its /// Tickets are read only for that last check. fn standing_of( reads: &impl Reads, @@ -538,9 +544,14 @@ fn standing_of( if read.claimed { return passed_over(Reason::Claimed); } - if spec_run::all_closed(read.sub_issue_is_open.iter().copied()) { - return passed_over(Reason::TicketsClosed); - } + let continuing_closed_spec = if spec_run::all_closed(read.sub_issue_is_open.iter().copied()) { + if !reads.can_continue(&candidate.issue)? { + return passed_over(Reason::TicketsClosed); + } + true + } else { + false + }; let unsettled = read .last_shaped .filter(|shaped| wait > TimeDelta::zero() && now - shaped.at < wait); @@ -557,9 +568,12 @@ fn standing_of( let tickets = reads.tickets(&candidate.issue)?; // Read after the candidate: its Tickets may have closed since. if !tickets.iter().any(|ticket| ticket.is_open) { - return passed_over(Reason::TicketsClosed); - } - if tickets + if tickets.is_empty() + || !(continuing_closed_spec || reads.can_continue(&candidate.issue)?) + { + return passed_over(Reason::TicketsClosed); + } + } else if tickets .iter() .all(|ticket| spec_run::unstartable(&candidate.issue, ticket).is_some()) { @@ -664,6 +678,10 @@ mod tests { Ok(self.facts(issue).and_then(|facts| facts.started.clone())) } + fn can_continue(&self, _issue: &IssueUrl) -> Result { + Ok(false) + } + fn tickets(&self, spec: &IssueUrl) -> Result> { self.seen.borrow_mut().push(Seen::TicketsRead(spec.number)); let facts = self.facts(spec).expect("only a Spec's Tickets are read"); diff --git a/src/run.rs b/src/run.rs index e5251bd72..9cd5b2051 100644 --- a/src/run.rs +++ b/src/run.rs @@ -777,7 +777,8 @@ mod tests { let spec_run = match work { Work::Run(_) | Work::BaseFix(_) => false, Work::SpecRun(_) => true, - Work::PickupRun(_) + Work::Starting { .. } + | Work::PickupRun(_) | Work::ArchitectRun(_) | Work::SecurityRun(_) | Work::ScoringRun(_) diff --git a/src/run_ending.rs b/src/run_ending.rs index 59d2e21a3..176117385 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -586,10 +586,15 @@ impl Shown { match &account.ended { // Also on stderr, so the outcome shows even when stdout is captured. Ok(line) => steps.push(line.clone()), - Err(cause) => steps.push(cause.full().to_string()), + Err(cause) => steps.push( + account + .pre_audit_failure + .unwrap_or_else(|| cause.full()) + .to_string(), + ), } steps.extend(account.advice.iter().map(Advice::to_string)); - if account.ended.is_err() { + if account.ended.is_err() && account.pre_audit_failure.is_none() { if let Some(log) = account.log { steps.push(format!("{SESSION_LOG}{}", log.display())); } diff --git a/src/security.rs b/src/security.rs index b8d321bb0..a7ec97d1f 100644 --- a/src/security.rs +++ b/src/security.rs @@ -7,7 +7,7 @@ use anyhow::Result; use crate::asks::Flags; use crate::base_fix::Advice; -use crate::command_limit::{AlreadyRunning, PassKind, Sharing}; +use crate::command_limit::{NotAdmitted, PassKind, Sharing}; use crate::config::UserConfig; use crate::failed_run::FailedRun; use crate::github::ListedIssue; @@ -95,7 +95,7 @@ impl From for Ended { pub use crate::github::RecordedFinding; pub enum Skipped { - AlreadyRunning(AlreadyRunning), + NotAdmitted(NotAdmitted), ReadyIssue(ListedIssue), FailedFix(crate::issue::IssueUrl), UnchangedBase(String), @@ -104,7 +104,7 @@ pub enum Skipped { impl fmt::Display for Skipped { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::AlreadyRunning(running) => running.fmt(f), + Self::NotAdmitted(running) => running.fmt(f), Self::ReadyIssue(listed) => write!( f, "Ready issue #{} \"{}\" goes first: {}", @@ -157,9 +157,9 @@ pub fn run( Sharing::OneOfAKind(PassKind::Security), ) { Ok(Start::Clear(launch)) => launch, - Ok(Start::AlreadyRunning(running)) => { + Ok(Start::NotAdmitted(running)) => { logs::skipped(Pass::Security, &running.repo, &running); - return Outcome::Skipped(Skipped::AlreadyRunning(running)); + return Outcome::Skipped(Skipped::NotAdmitted(running)); } Err(error) => return Outcome::Audited(error.into()), }; diff --git a/src/triage/pass.rs b/src/triage/pass.rs index c489ff638..0879c820f 100644 --- a/src/triage/pass.rs +++ b/src/triage/pass.rs @@ -44,7 +44,7 @@ pub fn run( base, } = match launch::start(base, config.command_limit, Sharing::UpToTheLimit)? { Start::Clear(launch) => launch, - Start::AlreadyRunning(running) => return Ok(skipped(&running.repo, &running)), + Start::NotAdmitted(running) => return Ok(skipped(&running.repo, &running)), }; let failed = crate::pickup::failures(&mut LaunchAndGitHub { launch: directory.git(), diff --git a/src/worktree.rs b/src/worktree.rs index 9649e282b..66253c669 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -29,6 +29,8 @@ mod preservation_tests; #[cfg(test)] mod review_recovery_tests; #[cfg(test)] +mod skill_tests; +#[cfg(test)] mod synchronization_tests; #[derive(Clone, Copy)] diff --git a/src/worktree/skill.rs b/src/worktree/skill.rs index 2cb26ff52..168055221 100644 --- a/src/worktree/skill.rs +++ b/src/worktree/skill.rs @@ -1,5 +1,6 @@ //! Standalone skill checkouts start at the Launch directory's committed HEAD. +use std::os::unix::fs::MetadataExt; use std::path::Path; use anyhow::Result; @@ -44,13 +45,9 @@ impl SkillWorktree { let recovered = retained.is_some(); let checkout = match retained { Some(checkout) => checkout, - None => { - let path = tempfile::Builder::new() - .prefix("thirdshift-review-") - .tempdir()? - .keep(); - acquisition::add_review(launch, branch, &path, commit)? - } + None => acquire_in_temp("thirdshift-review-", |path| { + acquisition::add_review(launch, branch, path, commit) + })?, }; Ok(Self { launch: Git::new(root), @@ -62,16 +59,13 @@ impl SkillWorktree { fn checkout(launch: &Git, commit: &str) -> Result { let root = launch.run(&["rev-parse", "--show-toplevel"])?; - // Cleanup belongs to the captured checkout, never TempDir's recursive Drop. - let path = tempfile::Builder::new() - .prefix("thirdshift-skill-") - .tempdir()? - .keep(); - progress::step(format_args!( - "creating skill worktree {} at {commit}", - path.display() - )); - let checkout = acquisition::add(launch, None, &path, commit)?; + let checkout = acquire_in_temp("thirdshift-skill-", |path| { + progress::step(format_args!( + "creating skill worktree {} at {commit}", + path.display() + )); + acquisition::add(launch, None, path, commit) + })?; Ok(Self { launch: Git::new(root), checkout, @@ -130,6 +124,26 @@ impl SkillWorktree { } } +/// Cleanup belongs to the captured checkout, never TempDir's recursive Drop: +/// a failed acquisition can retain work there. Failure removes only the +/// allocated directory itself, still empty, and keeps acquisition's error. +fn acquire_in_temp( + prefix: &str, + acquire: impl FnOnce(&Path) -> Result, +) -> Result { + let directory = tempfile::Builder::new().prefix(prefix).tempdir()?; + let allocated = std::fs::symlink_metadata(directory.path())?; + let path = directory.keep(); + acquire(&path).inspect_err(|_| { + // remove_dir refuses a directory with contents, such as a retained checkout. + if std::fs::symlink_metadata(&path).is_ok_and(|current| { + current.is_dir() && current.dev() == allocated.dev() && current.ino() == allocated.ino() + }) { + let _ = std::fs::remove_dir(&path); + } + }) +} + impl Drop for SkillWorktree { fn drop(&mut self) { self.checkout diff --git a/src/worktree/skill_tests.rs b/src/worktree/skill_tests.rs new file mode 100644 index 000000000..db2989f4a --- /dev/null +++ b/src/worktree/skill_tests.rs @@ -0,0 +1,194 @@ +//! Skill and Review checkout allocation, each run in a child with its own temp root. + +use std::fs; +use std::path::Path; + +use super::*; + +const CHILD: &str = "THIRDSHIFT_SKILL_ALLOCATION_TEST"; +const MISSING_COMMIT: &str = "1234567890123456789012345678901234567890"; + +/// Run the calling test again in a child whose temp root holds only its +/// fixtures, so allocation leftovers can be counted. +fn ran_in_isolated_child(configure: impl FnOnce(&Path, &mut std::process::Command)) -> bool { + if std::env::var_os(CHILD).is_some() { + return false; + } + let root = tempfile::tempdir().unwrap(); + let name = std::thread::current().name().unwrap().to_string(); + // The launch fixture's empty clone warns, so judge only the child's status. + let mut command = std::process::Command::new(std::env::current_exe().unwrap()); + command + .args(["--exact", &name, "--nocapture"]) + .env(CHILD, "1") + .env("TMPDIR", root.path()); + configure(root.path(), &mut command); + let output = command.output().unwrap(); + assert!( + output.status.success(), + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + true +} + +fn allocations() -> Vec { + fs::read_dir(std::env::temp_dir()) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| { + name.starts_with("thirdshift-review-") || name.starts_with("thirdshift-skill-") + }) + .collect() +} + +#[test] +fn a_failed_skill_checkout_leaves_no_allocation_behind() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + + assert!(SkillWorktree::at(&launch, MISSING_COMMIT).is_err()); + + assert_eq!(allocations(), Vec::::new()); +} + +#[test] +fn a_failed_review_checkout_leaves_no_allocation_behind() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + + assert!(SkillWorktree::on_pull_request(&launch, "feature", MISSING_COMMIT).is_err()); + + assert_eq!(allocations(), Vec::::new()); + assert_eq!(local_head(&launch, "feature").unwrap(), None); +} + +#[test] +fn a_branch_occupied_after_review_preflight_keeps_the_occupier_and_no_allocation() { + if ran_in_isolated_child(|root, command| { + // Occupy the branch once, between preflight and Git's add. + let real = String::from_utf8( + std::process::Command::new("sh") + .args(["-c", "command -v git"]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + let bin = root.join("bin"); + fs::create_dir(&bin).unwrap(); + crate::test_support::write_executable( + &bin.join("git"), + &format!( + "#!/bin/sh\nif [ \"$1 $2 $3\" = 'worktree add -b' ] && [ ! -e '{root}/occupier' ]; then\n '{real}' worktree add -q -b \"$4\" '{root}/occupier' \"$6\" || exit 97\nfi\nexec '{real}' \"$@\"\n", + root = root.display(), + real = real.trim(), + ), + ); + let path = std::env::join_paths( + std::iter::once(bin).chain(std::env::split_paths(&std::env::var_os("PATH").unwrap())), + ) + .unwrap(); + command.env("PATH", path); + }) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + + let Err(error) = SkillWorktree::on_pull_request(&launch, "feature", &start) else { + panic!("a branch occupied after preflight was acquired"); + }; + + assert!(format!("{error:#}").contains("feature"), "{error:#}"); + assert_eq!(allocations(), Vec::::new()); + let occupier = std::env::temp_dir().join("occupier"); + assert_eq!( + Git::new(&occupier).run(&["rev-parse", "HEAD"]).unwrap(), + start + ); + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); +} + +#[test] +fn a_branch_occupied_before_review_is_refused_before_allocation() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + let occupier = std::env::temp_dir().join("occupier"); + launch + .run(&[ + "worktree", + "add", + "-q", + "-b", + "feature", + occupier.to_str().unwrap(), + ]) + .unwrap(); + + let Err(error) = SkillWorktree::on_pull_request(&launch, "feature", &start) else { + panic!("an occupied branch was acquired"); + }; + + assert!( + format!("{error:#}").contains("Occupied checkout"), + "{error:#}" + ); + assert_eq!(allocations(), Vec::::new()); + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); +} + +#[test] +fn hook_work_from_a_failed_skill_or_review_checkout_survives() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + crate::test_support::write_executable( + &temp.path().join("hooks/post-checkout"), + "#!/bin/sh\necho hook work > hook.txt\nexit 1\n", + ); + + assert!(SkillWorktree::at(&launch, &start).is_err()); + assert!(SkillWorktree::on_pull_request(&launch, "feature", &start).is_err()); + + let mut retained = allocations(); + retained.sort_by_key(|name| name.starts_with("thirdshift-review-")); + assert!( + retained.len() == 2 + && retained[0].starts_with("thirdshift-skill-") + && retained[1].starts_with("thirdshift-review-"), + "{retained:?}" + ); + for name in retained { + let path = std::env::temp_dir().join(name); + assert_eq!( + fs::read_to_string(path.join("hook.txt")).unwrap(), + "hook work\n" + ); + } + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); +} + +#[test] +fn successful_skill_and_review_checkouts_are_cleaned_up() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + + drop(SkillWorktree::at(&launch, &start).unwrap()); + drop(SkillWorktree::on_pull_request(&launch, "feature", &start).unwrap()); + + assert_eq!(allocations(), Vec::::new()); +} diff --git a/tests/architect_run.rs b/tests/architect_run.rs index c8e2e4fcf..beade1736 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -2055,7 +2055,10 @@ fn a_second_architect_run_on_the_repository_is_skipped_while_the_first_is_mid_re ); assert_eq!(scenario.entries("work"), [REPO, "widgets-architect"]); assert_eq!(scenario.gh_state(), github); - assert_eq!(scenario.gh_calls(), gh_calls); + assert_eq!( + scenario.gh_calls(), + [gh_calls, vec![vec!["api".into(), "rate_limit".into()]]].concat() + ); release(&scenario); let first = first.finish(); @@ -2696,6 +2699,61 @@ fn the_lowest_numbered_ready_or_settling_issue_goes_first() { } } +#[test] +fn an_all_closed_spec_has_architect_priority_only_with_a_handed_back_continuation() { + for (branch, handed_back, ready_ago) in [ + (true, true, 60), + (true, true, 4), + (false, true, 60), + (false, true, 4), + (true, false, 60), + ] { + let scenario = scenario(); + ready_issue(&scenario, 7, "Finish the Spec review", &[]); + scenario.spec_has_tickets(7, &[(9, &[])]); + scenario.issue_is(9, "CLOSED"); + if branch { + scenario.origin_has_branch("issue-7", "main", &["Landed Tickets"]); + } + let mut timeline = Vec::new(); + if handed_back { + timeline.extend([ + (TimelineEvent::Labelled(IN_PROGRESS), 90), + (TimelineEvent::Unlabelled(IN_PROGRESS), 61), + ]); + } + timeline.push((TimelineEvent::Labelled("ready-for-agent"), ready_ago)); + scenario.issue_timeline(7, &timeline); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + let github = scenario.gh_state(); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + if branch && handed_back { + assert_eq!(result.stdout, format!("{}\n", scenario.issue_url(7))); + let reason = if ready_ago < 5 { + format!( + "Ready issue #7 \"Finish the Spec review\" goes first once it settles: {}", + scenario.issue_url(7) + ) + } else { + goes_first(&scenario, 7, "Finish the Spec review") + }; + assert_eq!( + after_start(&result.stderr), + format!("thirdshift: {reason}\n") + ); + assert!(scenario.claude_calls().is_empty()); + assert_eq!(scenario.gh_state(), github); + assert_nothing_left_behind(&scenario); + } else { + assert_eq!(result.stdout, format!("{}\n", scenario.issue_url(10))); + assert_eq!(scenario.claude_calls().len(), 1); + } + } +} + #[test] fn a_settling_spec_goes_first_only_with_a_startable_ticket() { for startable in [false, true] { diff --git a/tests/ci.rs b/tests/ci.rs index b7cd02372..bcdbf8593 100644 --- a/tests/ci.rs +++ b/tests/ci.rs @@ -237,28 +237,10 @@ fn unchanged_running_checks_keep_writing_waiting_lines_to_the_command_log() { args.push("merge".to_string()); } - let result = scenario.run_with_env( + assert_ci_waiting_lines( + &scenario, &args.iter().map(String::as_str).collect::>(), - &[("THIRDSHIFT_WAITING_MINUTE_MS", "150")], - ); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - let waiting = "1 still running out of 1 checks listed so far"; - assert!( - result.stderr.matches(waiting).count() >= 3, - "{}", - result.stderr - ); - let logs = scenario.path("home/.thirdshift/logs/acme/widgets/commands/issue"); - let log = std::fs::read_dir(logs) - .unwrap() - .map(|entry| entry.unwrap().path()) - .find(|path| path.extension().is_some_and(|ext| ext == "log")) - .unwrap(); - let log = std::fs::read_to_string(log).unwrap(); - assert_eq!( - log.matches(waiting).count(), - result.stderr.matches(waiting).count() + "issue", ); if merge { assert_eq!(scenario.gh_state()["prs"][0]["state"], "MERGED"); @@ -266,6 +248,85 @@ fn unchanged_running_checks_keep_writing_waiting_lines_to_the_command_log() { } } +#[test] +fn custom_process_ci_waiting_lines_use_the_configured_interval() { + for config in [ + "[logs]\nwaiting_line_minutes = 1\n", + "[logs]\nwaiting_line_minutes = 20\n[inbox]\nsilent_minutes = 4\n", + ] { + custom_process_ci_waiting_lines(config); + } +} + +fn custom_process_ci_waiting_lines(config: &str) { + let scenario = Scenario::new(); + scenario.user_config_is(config); + let processes = scenario.path("home/.thirdshift/processes"); + std::fs::create_dir_all(&processes).unwrap(); + std::fs::write( + processes.join("ship.toml"), + include_str!("../processes/ticket-run.toml"), + ) + .unwrap(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}", + checks_on_head(r#"[{"name":"test","conclusion":"success","pending_polls":14}]"#) + )); + + assert_ci_waiting_lines(&scenario, &["process", "ship", "issue", "7"], "process"); +} + +#[test] +fn review_ci_waiting_lines_use_the_configured_interval() { + for author in [false, true] { + for config in [ + "[logs]\nwaiting_line_minutes = 1\n", + "[logs]\nwaiting_line_minutes = 20\n[inbox]\nsilent_minutes = 4\n", + ] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + scenario.origin_has_branch("colleague", "main", &["Colleague feature"]); + let url = scenario.github_has_pr("colleague", "main", "OPEN"); + scenario.agent_does(&format!( + "{}printf 'No findings.' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n", + checks_on_head(r#"[{"name":"test","conclusion":"success","pending_polls":14}]"#) + )); + let args = if author { + vec!["review", "--author", "colleague"] + } else { + vec!["review", &url] + }; + + assert_ci_waiting_lines(&scenario, &args, "review"); + } + } +} + +fn assert_ci_waiting_lines(scenario: &Scenario, args: &[&str], command: &str) { + let result = scenario.run_with_env(args, &[("THIRDSHIFT_WAITING_MINUTE_MS", "150")]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + let waiting = "1 still running out of 1 checks listed so far"; + assert!( + result.stderr.matches(waiting).count() >= 3, + "expected repeated CI waiting lines, got {}:\n{}", + result.stderr.matches(waiting).count(), + result.stderr + ); + let log = std::fs::read_dir(scenario.path(&format!( + "home/.thirdshift/logs/acme/widgets/commands/{command}" + ))) + .unwrap() + .map(|entry| entry.unwrap().path()) + .find(|path| path.extension().is_some_and(|ext| ext == "log")) + .unwrap(); + let log = std::fs::read_to_string(log).unwrap(); + assert_eq!( + log.matches(waiting).count(), + result.stderr.matches(waiting).count() + ); +} + #[test] fn ci_watch_uses_half_the_silent_time_when_the_waiting_interval_is_too_long() { let scenario = Scenario::new(); diff --git a/tests/command_logs.rs b/tests/command_logs.rs index 3e6dacd45..68a5bdb48 100644 --- a/tests/command_logs.rs +++ b/tests/command_logs.rs @@ -91,6 +91,63 @@ fn the_one_command_log(scenario: &Scenario, folder: &str, prefix: &str) -> (Path (path, format!("{stamp}~{token}")) } +#[test] +fn process_pass_and_issue_waiting_lines_reach_the_command_log() { + for issue in [false, true] { + for config in [ + "[logs]\nwaiting_line_minutes = 1\n", + "[logs]\nwaiting_line_minutes = 20\n[inbox]\nsilent_minutes = 4\n", + ] { + let scenario = Scenario::new(); + scenario.user_config_is(&format!( + "{config}[resources]\nmin_available_memory_mb = 9007199254740991\n" + )); + let processes = scenario.path("home/.thirdshift/processes"); + fs::create_dir_all(&processes).unwrap(); + fs::write( + processes.join("notes.toml"), + "version = 1\ndescription = 'Take notes'\n[[steps]]\nname = 'notes'\nskill = 'code-review'\n", + ) + .unwrap(); + scenario.agent_does("printf 'Done.' > \"$FAKE_CLAUDE_FINAL_MESSAGE\""); + let blocked = |_: u32| assert!(scenario.claude_calls().is_empty()); + let release = |pid: u32| { + blocked(pid); + scenario.user_config_is(""); + }; + let waiting = "waiting for host resources:"; + let mut args = vec!["process", "notes"]; + if issue { + args.extend(["issue", "7"]); + } + + let result = scenario.run_terminal( + &args, + &[ZONE, ("THIRDSHIFT_WAITING_MINUTE_MS", "150")], + &[ + support::TerminalStep::inspect(waiting, &blocked), + support::TerminalStep::inspect(waiting, &blocked), + support::TerminalStep::inspect(waiting, &release), + ], + ); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(scenario.claude_calls().len(), 1); + assert!( + result.stderr.matches(waiting).count() >= 3, + "{}", + result.stderr + ); + let (path, _) = the_one_command_log(&scenario, "process", ""); + let log = fs::read_to_string(path).unwrap(); + assert_eq!( + log.matches(waiting).count(), + result.stderr.matches(waiting).count() + ); + } + } +} + #[test] fn self_merge_keeps_writing_waiting_lines_while_github_calculates_mergeability() { let scenario = Scenario::new(); diff --git a/tests/console.rs b/tests/console.rs index 315470c6c..ff83961aa 100644 --- a/tests/console.rs +++ b/tests/console.rs @@ -297,6 +297,30 @@ fn console_pages_show_the_running_thirdshift_version() { } } +#[test] +fn quota_wait_stage_is_shared_by_status_show_and_console_and_resumes() { + let home = tempfile::tempdir().unwrap(); + configure(home.path()); + let mut value = record(home.path(), 7, None, 0); + value["quota_wait_until"] = "2026-11-11T12:01:37Z".into(); + write_record(home.path(), 7, &value); + let binary = Path::new(env!("CARGO_BIN_EXE_thirdshift")); + let console = Console::start(binary, home.path()); + let name = "Quota wait until"; + let mut url = reqwest::Url::parse("http://localhost/peek").unwrap(); + url.query_pairs_mut().append_pair("command", "widgets#7"); + let peek_path = format!("/peek?{}", url.query().unwrap()); + for path in ["/", &peek_path] { + let text = console.get(path).text().unwrap(); + assert!(text.contains(name), "{path}: {text}"); + } + value["quota_wait_until"] = Value::Null; + write_record(home.path(), 7, &value); + let text = console.get(&peek_path).text().unwrap(); + assert!(!text.contains(name), "{text}"); + assert!(text.contains("implement"), "{text}"); +} + #[test] fn command_changes_push_events_over_http() { let home = tempfile::tempdir().unwrap(); diff --git a/tests/console/address_scopes.rs b/tests/console/address_scopes.rs index bfa549305..83ff8e092 100644 --- a/tests/console/address_scopes.rs +++ b/tests/console/address_scopes.rs @@ -1,5 +1,101 @@ use super::*; +#[test] +fn fleet_schedules_forward_the_person_and_require_read_on_each_owner() { + let owner_home = configured(); + let caller_home = configured(); + let bin = owner_home.path().join("bin"); + fs::create_dir(&bin).unwrap(); + support::write_executable( + &bin.join("crontab"), + "#!/bin/sh\nprintf '%s\\n' '@daily thirdshift secure base main # owner-schedule-marker'\n", + ); + let mut command = Command::new(env!("CARGO_BIN_EXE_thirdshift")); + command + .args(["console", "--no-sign-in"]) + .env("HOME", owner_home.path()) + .env( + "PATH", + format!("{}:{}", bin.display(), std::env::var("PATH").unwrap()), + ); + let owner = Console::start_command(command); + let token = tokens::create(owner_home.path(), "schedule-reader", &["--scopes", "read"]); + assert!( + crate::fleet::add(caller_home.path(), "owner", &owner.address, &token) + .status + .success() + ); + assert!( + password(caller_home.path(), None, PASSWORD, PASSWORD) + .0 + .success() + ); + api::cli( + caller_home.path(), + &["console", "scopes", "--scopes", "read", "--json"], + ); + let caller = Console::start_mode( + Path::new(env!("CARGO_BIN_EXE_thirdshift")), + caller_home.path(), + false, + ); + let session = cookie(&sign_in(&caller, EMAIL, PASSWORD)); + let page = || { + get(&caller, "/cronjobs?fleet=true", &session) + .text() + .unwrap() + }; + let allowed = page(); + assert!(allowed.contains("owner-schedule-marker"), "{allowed}"); + assert!(!allowed.contains("Run Passes now")); + api::cli( + owner_home.path(), + &[ + "console", "scopes", "--email", EMAIL, "--scopes", "run", "--json", + ], + ); + let refused = page(); + assert!(refused.contains("refused"), "{refused}"); + assert!(!refused.contains("owner-schedule-marker")); + // A CLI Fleet read acts as its token holder, independently of this person's scopes. + let output = cli(caller_home.path(), &["cronjobs", "--fleet", "--json"]); + assert!(output.status.success(), "{output:?}"); + assert!( + String::from_utf8(output.stdout) + .unwrap() + .contains("owner-schedule-marker") + ); + allow(owner_home.path(), false); + let refused = page(); + assert!(refused.contains("refused"), "{refused}"); + assert!(!refused.contains("owner-schedule-marker")); + // A run-only token also cannot read inventory, even with no forwarded person. + assert!( + cli( + owner_home.path(), + &["console", "token", "revoke", "schedule-reader"] + ) + .status + .success() + ); + let runner = tokens::create(owner_home.path(), "schedule-runner", &["--scopes", "run"]); + assert!( + crate::fleet::add(caller_home.path(), "owner", &owner.address, &runner) + .status + .success() + ); + let output = cli(caller_home.path(), &["cronjobs", "--fleet", "--json"]); + assert!(output.status.success(), "{output:?}"); + let value: Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(value["machines"][1]["status"], "refused"); + assert!(!value.to_string().contains("owner-schedule-marker")); + api::cli( + caller_home.path(), + &["console", "scopes", "--scopes", "run", "--json"], + ); + assert_eq!(get(&caller, "/cronjobs?fleet=true", &session).status(), 403); +} + #[test] fn remote_details_name_the_signed_in_person_and_honor_the_owning_allowlist_and_scopes() { let caller_home = configured(); diff --git a/tests/console/cronjobs.rs b/tests/console/cronjobs.rs index 86e05972c..b1157006d 100644 --- a/tests/console/cronjobs.rs +++ b/tests/console/cronjobs.rs @@ -39,6 +39,466 @@ fn checkout(home: &Path, name: &str, origin: &str) -> std::path::PathBuf { path } +fn changing_crontab(home: &Path) { + checkout(home, "first", "https://github.com/acme/first.git"); + checkout(home, "second", "https://github.com/acme/second.git"); + support::write_executable( + &home.join("bin/crontab"), + "#!/bin/sh\nn=0\n[ ! -f \"$HOME/reads\" ] || n=$(cat \"$HOME/reads\")\nn=$((n+1))\necho $n > \"$HOME/reads\"\nif [ $n = 1 ]; then repo=first; else repo=second; fi\necho \"@daily cd ~/$repo && thirdshift pickup\"\n", + ); +} + +#[test] +fn start_options_share_one_crontab_capture_and_refresh_on_the_next_request() { + let home = fixture(); + changing_crontab(home.path()); + let mut start = command(home.path()); + start.args(["console", "--no-sign-in"]); + let console = Console::start_command(start); + for (read, repo) in [(1, "first"), (2, "second")] { + let options = api::json(console.get("/api/v1/start-options")); + assert_eq!( + fs::read_to_string(home.path().join("reads")) + .unwrap() + .trim(), + read.to_string() + ); + assert_eq!( + options["form"]["repositories"][0]["repository"], + format!("acme/{repo}") + ); + let request: Value = + serde_json::from_str(options["passes"][0]["request"].as_str().unwrap()).unwrap(); + assert_eq!(request["repository"], format!("acme/{repo}")); + } +} + +#[test] +fn local_cron_page_shares_its_capture_with_run_now() { + let home = fixture(); + changing_crontab(home.path()); + let mut start = command(home.path()); + start.args(["console", "--no-sign-in"]); + let console = Console::start_command(start); + for (read, repo) in [(1, "first"), (2, "second")] { + let html = console.get("/cronjobs").text().unwrap(); + let document = scraper::Html::parse_document(&html); + let commands = scraper::Selector::parse(".cron-command").unwrap(); + let buttons = scraper::Selector::parse("[data-start-request]").unwrap(); + assert_eq!( + document + .select(&commands) + .next() + .unwrap() + .text() + .collect::(), + format!("cd ~/{repo} && thirdshift pickup") + ); + let request: Value = serde_json::from_str( + document + .select(&buttons) + .next() + .unwrap() + .attr("data-start-request") + .unwrap(), + ) + .unwrap(); + assert_eq!(request["repository"], format!("acme/{repo}")); + assert_eq!( + fs::read_to_string(home.path().join("reads")) + .unwrap() + .trim(), + read.to_string() + ); + } +} + +#[test] +fn cron_page_keeps_repository_order_while_start_options_keep_crontab_order() { + let home = fixture(); + checkout(home.path(), "a", "https://github.com/acme/a.git"); + checkout(home.path(), "b", "https://github.com/acme/b.git"); + fs::write(home.path().join("crontab"), "@daily cd ~/b && thirdshift pickup\n@hourly cd ~/a && thirdshift architect\n@daily cd ~/b && thirdshift secure\n").unwrap(); + let mut start = command(home.path()); + start.args(["console", "--no-sign-in"]); + let console = Console::start_command(start); + let options = api::json(console.get("/api/v1/start-options")); + assert_eq!( + options["passes"] + .as_array() + .unwrap() + .iter() + .map(|pass| pass["command"].as_str().unwrap()) + .collect::>(), + [ + "cd ~/b && thirdshift pickup", + "cd ~/a && thirdshift architect", + "cd ~/b && thirdshift secure" + ] + ); + let html = console.get("/cronjobs").text().unwrap(); + let document = scraper::Html::parse_document(&html); + let buttons = scraper::Selector::parse("[data-start-request]").unwrap(); + let commands: Vec<_> = document + .select(&buttons) + .map(|button| { + let request: Value = + serde_json::from_str(button.attr("data-start-request").unwrap()).unwrap(); + request["command"].as_str().unwrap().to_owned() + }) + .collect(); + assert_eq!(commands, ["architect", "pickup", "secure"]); +} + +#[test] +fn aliases_and_duplicate_entries_share_successful_and_failed_origin_observations() { + let home = fixture(); + let valid = checkout(home.path(), "valid", "https://github.com/acme/widgets.git"); + let invalid = checkout( + home.path(), + "invalid", + "https://gitlab.com/acme/widgets.git", + ); + std::os::unix::fs::symlink(&valid, home.path().join("alias")).unwrap(); + std::os::unix::fs::symlink(&invalid, home.path().join("bad-alias")).unwrap(); + fs::write( + home.path().join(".thirdshift/config.toml"), + "launch_directories = ['~/alias', '~/valid']\n[console]\nport = 0\n", + ) + .unwrap(); + fs::write(home.path().join("crontab"), "@daily cd ~/alias && thirdshift pickup\n@daily cd ~/valid && thirdshift pickup\n@hourly cd ~/alias && thirdshift secure\n@daily cd ~/invalid && thirdshift pickup\n@daily cd ~/bad-alias && thirdshift pickup\n").unwrap(); + support::write_executable( + &home.path().join("bin/git"), + "#!/bin/sh\nif [ \"$1 $2 $3\" = 'config --get remote.origin.url' ]; then echo \"$PWD\" >> \"$HOME/origin-reads\"; fi\nexec /usr/bin/git \"$@\"\n", + ); + let mut start = command(home.path()); + start.args(["console", "--no-sign-in"]); + let console = Console::start_command(start); + let options = api::json(console.get("/api/v1/start-options")); + assert_eq!(options["form"]["repositories"].as_array().unwrap().len(), 1); + assert_eq!(options["passes"].as_array().unwrap().len(), 3); + let reads = fs::read_to_string(home.path().join("origin-reads")).unwrap(); + assert_eq!( + reads.lines().collect::>(), + [valid.to_str().unwrap(), invalid.to_str().unwrap()] + ); +} + +#[test] +fn invalid_configured_directory_keeps_cron_inventory_and_reports_start_error() { + let home = fixture(); + checkout(home.path(), "repo", "https://github.com/acme/widgets.git"); + fs::write( + home.path().join("crontab"), + "@daily cd ~/repo && thirdshift pickup\n", + ) + .unwrap(); + fs::write( + home.path().join(".thirdshift/config.toml"), + "launch_directories = ['~/missing']\n[console]\nport = 0\n", + ) + .unwrap(); + let mut start = command(home.path()); + start.args(["console", "--no-sign-in"]); + let console = Console::start_command(start); + assert_eq!( + api::json(console.get("/api/v1/cronjobs"))["jobs"][0]["repository"], + "acme/widgets" + ); + assert_eq!(console.get("/api/v1/launch-directories").status(), 422); + assert_eq!(console.get("/api/v1/start-options").status(), 422); + let page = console.get("/cronjobs"); + assert_eq!(page.status(), 200); + let html = page.text().unwrap(); + assert!(html.contains("acme/widgets"), "{html}"); + assert!( + html.contains("Could not load startable Passes: invalid configured Launch directory"), + "{html}" + ); + assert!( + console + .get("/") + .text() + .unwrap() + .contains("invalid configured Launch directory") + ); +} + +#[test] +fn owning_machine_projections_keep_crontab_error_rules() { + let home = fixture(); + checkout(home.path(), "repo", "https://github.com/acme/widgets.git"); + fs::write( + home.path().join(".thirdshift/config.toml"), + "launch_directories = ['~/repo']\n[console]\nport = 0\n", + ) + .unwrap(); + std::os::unix::fs::symlink("/usr/bin/git", home.path().join("bin/git")).unwrap(); + let mut start = command(home.path()); + start + .env("PATH", home.path().join("bin")) + .args(["console", "--no-sign-in"]); + let console = Console::start_command(start); + for (script, strict, discovery) in [ + (Some("#!/bin/sh\nexit 0\n"), 200, 200), + ( + Some("#!/bin/sh\necho 'crontab: no crontab for owner' >&2\nexit 1\n"), + 200, + 200, + ), + (None, 422, 200), + ( + Some("#!/bin/sh\necho 'crontab: permission denied' >&2\nexit 1\n"), + 422, + 422, + ), + (Some("#!/bin/sh\nprintf '\\377'\n"), 422, 422), + ] { + let executable = home.path().join("bin/crontab"); + if let Some(script) = script { + support::write_executable(&executable, script); + } else { + fs::remove_file(&executable).unwrap(); + } + for path in ["/api/v1/cronjobs", "/cronjobs"] { + let status = if path == "/cronjobs" && strict == 422 { + 500 + } else { + strict + }; + assert_eq!(console.get(path).status(), status, "{script:?}: {path}"); + } + for path in ["/api/v1/launch-directories", "/api/v1/start-options"] { + let response = console.get(path); + assert_eq!(response.status(), discovery, "{script:?}: {path}"); + if discovery == 200 { + let value = api::json(response); + if path.ends_with("start-options") { + assert_eq!( + value["form"]["repositories"][0]["repository"], + "acme/widgets" + ); + assert!(value["passes"].as_array().unwrap().is_empty()); + } else { + assert_eq!(value["directories"][0]["repository"], "acme/widgets"); + } + } + } + for (args, expected) in [ + (vec!["cronjobs", "--json"], strict), + (vec!["launch-directories", "--json"], discovery), + ] { + let output = command(home.path()) + .env("PATH", home.path().join("bin")) + .args(args) + .output() + .unwrap(); + assert_eq!( + output.status.success(), + expected == 200, + "{script:?}: {output:?}" + ); + } + } +} + +#[test] +fn cron_job_projections_agree_without_conflating_kind_directory_and_start() { + let home = fixture(); + let repo = checkout(home.path(), "repo", "https://github.com/acme/widgets.git"); + checkout( + home.path(), + "elsewhere", + "https://gitlab.com/acme/widgets.git", + ); + fs::write(home.path().join("crontab"), "").unwrap(); + let mut start = command(home.path()); + start.args(["console", "--no-sign-in"]); + let console = Console::start_command(start); + for (text, kind, resolved, offered) in [ + ("thirdshift pickup", Some("Pickup run"), false, false), + ( + "cd ~/repo && thirdshift pickup", + Some("Pickup run"), + true, + true, + ), + ("cd ~/repo && env X=1 thirdshift pickup", None, true, false), + ( + "cd ~/repo && /usr/bin/env X=1 thirdshift pickup", + None, + true, + false, + ), + ("cd ~/repo && thirdshift status --json", None, true, false), + ( + "cd ~/repo && thirdshift pickup --unknown", + None, + true, + false, + ), + ( + "cd ~/repo && thirdshift pickup base '$BASE'", + None, + true, + false, + ), + ( + "cd ~/repo && thirdshift pickup base '`branch`'", + None, + true, + false, + ), + ( + "cd ~/missing && thirdshift pickup", + Some("Pickup run"), + false, + false, + ), + ( + "cd ~/elsewhere && thirdshift pickup", + Some("Pickup run"), + false, + false, + ), + ( + "cd ~/repo && command -p -- cd /elsewhere && thirdshift pickup", + None, + false, + false, + ), + ( + "cd ~/repo && echo ready && thirdshift pickup && echo done", + None, + true, + true, + ), + ( + "cd ~/repo && thirdshift pickup && echo done", + None, + true, + true, + ), + ( + "cd ~/repo && thirdshift fix https://github.com/acme/widgets/pull/7", + Some("Fix run"), + true, + false, + ), + ( + "cd ~/repo && thirdshift review https://github.com/acme/widgets/pull/7", + Some("Review run"), + true, + false, + ), + ( + "cd ~/repo && thirdshift triage", + Some("Triage run"), + true, + false, + ), + ("cd '~/repo && thirdshift pickup", None, false, false), + ] { + fs::write(home.path().join("crontab"), format!("@daily {text}\n")).unwrap(); + let inventory = command(home.path()) + .args(["cronjobs", "--json"]) + .output() + .unwrap(); + assert!(inventory.status.success(), "{inventory:?}"); + let inventory: Value = serde_json::from_slice(&inventory.stdout).unwrap(); + assert_eq!( + inventory["jobs"], + json!([{ + "schedule": "@daily", "command": text, "command_kind": kind, + "repository": resolved.then_some("acme/widgets"), + }]), + "{text}" + ); + assert_eq!( + api::json(console.get("/api/v1/cronjobs")), + inventory, + "{text}" + ); + let directories = command(home.path()) + .args(["launch-directories", "--json"]) + .output() + .unwrap(); + assert!(directories.status.success(), "{directories:?}"); + let directories: Value = serde_json::from_slice(&directories.stdout).unwrap(); + let expected = if resolved { + json!({"directories": [{"repository": "acme/widgets", "path": repo}]}) + } else { + json!({"directories": []}) + }; + assert_eq!(directories, expected, "{text}"); + assert_eq!( + api::json(console.get("/api/v1/launch-directories")), + expected, + "{text}" + ); + let options = api::json(console.get("/api/v1/start-options")); + let passes = options["passes"].as_array().unwrap(); + assert_eq!(passes.len(), usize::from(offered), "{text}: {options}"); + if offered { + assert_eq!(passes[0]["command"], text); + let request: Value = + serde_json::from_str(passes[0]["request"].as_str().unwrap()).unwrap(); + assert_eq!(request["command"], "pickup"); + assert_eq!(request["repository"], "acme/widgets"); + } + } +} + +#[test] +fn cronjobs_classifies_literal_commands_without_executing_them() { + let home = fixture(); + let repo = checkout( + home.path(), + "widgets", + "https://github.com/acme/widgets.git", + ); + let examples = [ + ("pickup base main", Some("Pickup run")), + ("secure base main", Some("Security run")), + ("architect base main", Some("Architect run")), + ("triage", Some("Triage run")), + ("fix", Some("Fix run")), + ("review --author acme", Some("Review run")), + ("process inspect", Some("Custom process")), + ("https://github.com/acme/widgets/issues/7", None), + ("pickup > cron.log && thirdshift secure", None), + ("status --json", None), + ("pickup base \"$BASE\"", None), + ("unknown-command", None), + ]; + fs::write( + home.path().join("crontab"), + examples + .iter() + .map(|(args, _)| { + format!( + "@daily cd '{}' && thirdshift {args} >> cron.log 2>&1\n", + repo.display() + ) + }) + .collect::(), + ) + .unwrap(); + let output = command(home.path()) + .args(["cronjobs", "--json"]) + .output() + .unwrap(); + assert!(output.status.success(), "{output:?}"); + let inventory: Value = serde_json::from_slice(&output.stdout).unwrap(); + for (job, (_, expected)) in inventory["jobs"].as_array().unwrap().iter().zip(examples) { + assert_eq!(job.get("command_kind"), Some(&json!(expected)), "{job}"); + } + let mut start = command(home.path()); + start.args(["console", "--no-sign-in"]); + let console = Console::start_command(start); + assert_eq!(api::json(console.get("/api/v1/cronjobs")), inventory); +} + #[test] fn cronjobs_groups_repositories_stably_in_cli_and_api() { let home = fixture(); @@ -68,11 +528,11 @@ fn cronjobs_groups_repositories_stably_in_cli_and_api() { assert_eq!( inventory, json!({"jobs": [ - {"repository": "acme/widgets", "schedule": "0 2 * * *", "command": alpha_command}, - {"repository": "acme/widgets", "schedule": "*/10 * * * *", "command": another_alpha}, - {"repository": "zeta/widgets", "schedule": "*/5 * * * *", "command": zulu_command}, - {"repository": "zeta/widgets", "schedule": "*/5 * * * *", "command": zulu_command}, - {"repository": null, "schedule": "@reboot", "command": "thirdshift secure base main"} + {"repository": "acme/widgets", "command_kind":"Architect run", "schedule": "0 2 * * *", "command": alpha_command}, + {"repository": "acme/widgets", "command_kind":null, "schedule": "*/10 * * * *", "command": another_alpha}, + {"repository": "zeta/widgets", "command_kind":"Pickup run", "schedule": "*/5 * * * *", "command": zulu_command}, + {"repository": "zeta/widgets", "command_kind":"Pickup run", "schedule": "*/5 * * * *", "command": zulu_command}, + {"repository": null, "command_kind":"Security run", "schedule": "@reboot", "command": "thirdshift secure base main"} ], "pass_starts": []}) ); let text = command(home.path()).arg("cronjobs").output().unwrap(); @@ -104,6 +564,21 @@ fn cronjobs_remote_and_legacy_inventories_use_the_owning_machines_metadata() { assert!(output.status.success(), "{output:?}"); } +#[test] +fn cronjobs_fleet_combines_inventories_and_isolates_failures() { + let output = Command::new("python3") + .args([ + concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/console/cronjobs_fleet.py" + ), + env!("CARGO_BIN_EXE_thirdshift"), + ]) + .output() + .unwrap(); + assert!(output.status.success(), "{output:?}"); +} + #[test] fn cronjobs_keeps_directory_changing_compounds_under_other() { let home = fixture(); @@ -150,7 +625,7 @@ fn cronjobs_keeps_directory_changing_compounds_under_other() { for (job, expected) in jobs.iter().zip(commands) { assert_eq!( job, - &json!({"repository": null, "schedule": "@daily", "command": expected}) + &json!({"repository": null, "command_kind":null, "schedule": "@daily", "command": expected}) ); } let mut start = command(home.path()); @@ -211,16 +686,24 @@ fn cronjobs_keeps_unresolved_entries_and_never_evaluates_shell_text() { assert_eq!(jobs.len(), 8, "{inventory}"); assert_eq!( jobs[0], - json!({"repository":"acme/widgets", "schedule":"@daily", "command":literal}) + json!({"repository":"acme/widgets", "command_kind":null, "schedule":"@daily", "command":literal}) ); assert_eq!( jobs[1], - json!({"repository":"acme/widgets", "schedule":"@hourly", "command":display_only}) + json!({"repository":"acme/widgets", "command_kind":null, "schedule":"@hourly", "command":display_only}) ); - for (job, expected) in jobs[2..].iter().zip(unresolved) { + let kinds = [ + Some("Pickup run"), + Some("Pickup run"), + Some("Pickup run"), + None, + None, + None, + ]; + for ((job, expected), kind) in jobs[2..].iter().zip(unresolved).zip(kinds) { assert_eq!( job, - &json!({"repository":null, "schedule":"@reboot", "command":expected}) + &json!({"repository":null, "command_kind":kind, "schedule":"@reboot", "command":expected}) ); } let mut start = command(home.path()); @@ -288,8 +771,8 @@ fn cronjobs_pairs_cli_and_console_without_running_scheduled_commands() { assert_eq!( jobs, json!({"jobs": [ - {"repository": null, "schedule": "*/5 * * * *", "command": "cd /srv/repo && /home/me/.cargo/bin/thirdshift pickup base main >> /tmp/cron.log 2>&1"}, - {"repository": null, "schedule": "@reboot", "command": "thirdshift secure base main # "} + {"repository": null, "command_kind":"Pickup run", "schedule": "*/5 * * * *", "command": "cd /srv/repo && /home/me/.cargo/bin/thirdshift pickup base main >> /tmp/cron.log 2>&1"}, + {"repository": null, "command_kind":"Security run", "schedule": "@reboot", "command": "thirdshift secure base main # "} ], "pass_starts": []}) ); let mut start = command(home.path()); @@ -456,7 +939,7 @@ fn cronjobs_respects_crons_command_and_stdin_separator() { assert_eq!( serde_json::from_slice::(&output.stdout).unwrap(), json!({"jobs": [ - {"repository": null, "schedule": "* * * * *", "command": "/usr/local/bin/thirdshift%stdin"} + {"repository": null, "command_kind":null, "schedule": "* * * * *", "command": "/usr/local/bin/thirdshift%stdin"} ], "pass_starts": []}) ); } diff --git a/tests/console/cronjobs_fleet.py b/tests/console/cronjobs_fleet.py new file mode 100644 index 000000000..2fbd457d3 --- /dev/null +++ b/tests/console/cronjobs_fleet.py @@ -0,0 +1,153 @@ +"""Fleet schedules share CLI/page data and isolate failures at owning APIs.""" +import http.server +from html import unescape +import json +import os +from pathlib import Path +import select +import socket +import subprocess +import sys +import tempfile +import threading +import time +import urllib.error +import urllib.request + + +binary = sys.argv[1] +with tempfile.TemporaryDirectory(prefix="cronjobs-fleet-") as temp: + home = Path(temp) + root = home / ".thirdshift" + root.mkdir() + (home / "bin").mkdir() + crontab = home / "bin/crontab" + crontab.write_text('#!/bin/sh\ncat "$HOME/crontab"\n') + crontab.chmod(0o755) + (home / "crontab").write_text('@daily thirdshift "$UNKNOWN"\n') + (root / "config.toml").write_text("[console]\nport = 0\n") + calls = [] + active = False + job = {"repository": "acme/widgets", "command_kind": "Pickup run", + "schedule": "*/5 * * * *", "command": "cd /remote/only && thirdshift pickup"} + inventories = { + "east": {"jobs": [job, dict(job, schedule="@daily")], "pass_starts": []}, + "west": {"jobs": [dict(job, command_kind="Security run"), + dict(job, repository="acme/tools"), + {"schedule": "@reboot", "command": 'thirdshift "