From 410eef7121f68378dd6b1c09aa1e7f8ce9506597 Mon Sep 17 00:00:00 2001 From: Bill Njoroge Date: Tue, 6 Oct 2026 23:59:05 -0400 Subject: [PATCH 1/4] feat(server): route all GitHub check-run updates through the outbox and one background sender (#365) Every check-run update now flows through the transactional outbox into a durable, coalesced queue (`check_run_updates`, one row per check, strictly newer version wins), and one leased background sender per node talks to GitHub with per-installation rate budgets, Retry-After / secondary-limit handling, jittered backoff, and dedup before re-POST. Previously ~10 call sites awaited GitHub inline: updates were lost on failure or crash, and a push fanning out to many jobs exceeded GitHub's 10 s webhook timeout. Reporters (webhook intake, acquire, dispatch, runs, push, rerun, boot catch-up) are DB-only; the sender is the only Checks API client. Outbox prune never passes the slowest durable consumer's bookmark. Schema: Postgres v5, SQLite v4 (in place; recreate dev databases). --- CHANGELOG.md | 4 + crates/preloop-runner-server/src/bootstrap.rs | 23 +- .../src/check_run_outbox.rs | 68 + .../src/control/backend.rs | 382 +++- .../src/control/lite/check_runs.rs | 175 ++ .../src/control/lite/impls.rs | 91 + .../src/control/lite/lifecycle.rs | 65 +- .../src/control/lite/mod.rs | 3 +- .../src/control/lite/schema.sql | 5 +- .../src/control/pg/check_runs.rs | 251 +++ .../src/control/pg/mod.rs | 3 +- .../src/control/pg/outbox.rs | 12 +- .../src/control/pg/schema.sql | 5 +- .../src/control/pg/trait_impl.rs | 91 + .../src/control/tests.rs | 550 +++++ .../src/control/types.rs | 28 +- .../preloop-runner-server/src/event_feed.rs | 1 + crates/preloop-runner-server/src/github.rs | 1995 ++++++++++++----- .../preloop-runner-server/src/github_app.rs | 15 + .../src/github_breaker.rs | 28 + crates/preloop-runner-server/src/lib.rs | 1 + crates/preloop-runner-server/tests/broker.rs | 3 + docs/architecture.md | 31 + docs/control-schema.sql | 5 +- 24 files changed, 3184 insertions(+), 651 deletions(-) create mode 100644 crates/preloop-runner-server/src/check_run_outbox.rs create mode 100644 crates/preloop-runner-server/src/control/lite/check_runs.rs create mode 100644 crates/preloop-runner-server/src/control/pg/check_runs.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 225d585d6..7d1734d03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,10 @@ Releases before v0.27.0 predate the changelog. limit or a suspension — GitHub does not redeliver, so they are recorded and wait at claim; only a `deleted` namespace refuses them. Namespaces with no limit rows behave as before. +- Check-run updates now use a durable, coalescing transactional-outbox + projector and one leased background sender. GitHub API retries, rate-limit + backoff, stale-id reconciliation and restart recovery happen outside + webhook/request paths on both SQLite and PostgreSQL. ### Changed diff --git a/crates/preloop-runner-server/src/bootstrap.rs b/crates/preloop-runner-server/src/bootstrap.rs index 614b14fe6..f2ea0dfec 100644 --- a/crates/preloop-runner-server/src/bootstrap.rs +++ b/crates/preloop-runner-server/src/bootstrap.rs @@ -511,13 +511,6 @@ pub async fn reap_once(shared: &Arc) { reason: Some("fork-PR approval window expired".to_owned()), }) .await; - crate::github::report_check_run_completed( - shared, - *run_id, - &job_id, - ExecutionStatus::Failure, - ) - .await; } shared .state @@ -541,13 +534,6 @@ pub async fn reap_once(shared: &Arc) { reason: Some(job.reason.clone()), }) .await; - crate::github::report_check_run_completed( - shared, - job.run_id, - &job.job_id, - ExecutionStatus::Failure, - ) - .await; } // Publish each affected run's updated status. A run the sweep just @@ -679,8 +665,9 @@ async fn run_history_archiver(shared: Arc) { } } -/// Outbox rows are only needed for the live fan-out window (no durable -/// consumer reads them yet): older rows are deleted. Env: +/// Outbox rows are pruned once older than the retention window *and* at or +/// below the slowest durable consumer bookmark (the `check-runs` projector); +/// rows a consumer has not read yet are retained however old they are. Env: /// `PRELOOP_OUTBOX_RETENTION_SECONDS` (default 3600). fn outbox_retention() -> Duration { Duration::from_secs( @@ -1694,6 +1681,10 @@ pub async fn serve(config: ServerConfig) -> anyhow::Result<()> { state: state.clone(), shutdown: shutdown.clone(), }); + let check_run_sender_shared = shared.clone(); + tokio::spawn(async move { + crate::github::run_check_run_sender(check_run_sender_shared).await; + }); // 5s sampler — clone needed state under lock, release, then publish. let sampler_shared = shared.clone(); diff --git a/crates/preloop-runner-server/src/check_run_outbox.rs b/crates/preloop-runner-server/src/check_run_outbox.rs new file mode 100644 index 000000000..a7d166b77 --- /dev/null +++ b/crates/preloop-runner-server/src/check_run_outbox.rs @@ -0,0 +1,68 @@ +//! Durable check-run projection and sender support. +//! +//! The projector is deliberately separate from the GitHub client: it only +//! reads committed outbox rows and writes the coalesced desired-state table. + +use std::sync::{Arc, Weak}; +use std::time::Duration; +use tokio::sync::Notify; + +use crate::control::Backend; + +const BATCH: usize = 256; +const LEASE: Duration = Duration::from_secs(30); +const POLL: Duration = Duration::from_millis(250); + +/// Start the durable projector for either backend. PostgreSQL notifications are +/// converted into the same local dirty signal used by SQLite and by writers on +/// this node; polling remains the recovery path for a lost notification. +pub(crate) fn spawn_consumer(backend: &Arc, dirty: Arc) { + if let Some(mut notifications) = backend.subscribe_event_notifications() { + let wake = Arc::clone(&dirty); + tokio::spawn(async move { + loop { + match notifications.recv().await { + Ok(_) => wake.notify_one(), + Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => wake.notify_one(), + Err(tokio::sync::broadcast::error::RecvError::Closed) => break, + } + } + }); + } + tokio::spawn(run_consumer(Arc::downgrade(backend), dirty)); +} + +async fn run_consumer(backend: Weak, dirty: Arc) { + let owner = uuid::Uuid::new_v4().to_string(); + loop { + let Some(backend_ref) = backend.upgrade() else { + return; + }; + let mut processed = 0usize; + loop { + match backend_ref + .consume_check_run_outbox(&owner, LEASE, BATCH) + .await + { + Ok(n) => { + processed += n; + if n < BATCH { + break; + } + } + Err(error) => { + tracing::warn!(?error, "check-run outbox projection failed"); + break; + } + } + } + drop(backend_ref); + if processed > 0 { + continue; + } + tokio::select! { + _ = dirty.notified() => {}, + _ = tokio::time::sleep(POLL) => {}, + } + } +} diff --git a/crates/preloop-runner-server/src/control/backend.rs b/crates/preloop-runner-server/src/control/backend.rs index adfbca130..4f7120b4c 100644 --- a/crates/preloop-runner-server/src/control/backend.rs +++ b/crates/preloop-runner-server/src/control/backend.rs @@ -491,6 +491,96 @@ pub(crate) trait ControlBackend: Send + Sync { limit: usize, ) -> Result; + /// Consume one durable outbox batch for the check-run projector. The + /// bookmark and all desired-state upserts commit atomically while the + /// named consumer lease is held by `owner`. + async fn consume_check_run_outbox( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result; + + /// Lease due desired check-run rows for the single background sender. + async fn lease_check_run_updates( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result, ControlError>; + + /// Save a GitHub check-run id obtained by the sender. A newer desired + /// version may have arrived meanwhile; the conditional update preserves + /// that newer row while still recording the id. + async fn set_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + check_run_id: u64, + ) -> Result<(), ControlError>; + + /// Complete a leased row only when no newer desired version arrived. + async fn finish_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + ) -> Result<(), ControlError>; + + /// Release a failed sender lease with exponential-backoff metadata. + async fn retry_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + permanent: bool, + ) -> Result<(), ControlError>; + + /// Clear a stale GitHub id so the next attempt recreates/reconciles it. + async fn clear_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + expected: u64, + ) -> Result<(), ControlError>; + + async fn enqueue_check_run_update( + &self, + update: CheckRunUpdateInput, + ) -> Result<(), ControlError>; + + /// Release a rate-limited lease without counting a delivery attempt. + async fn defer_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + ) -> Result<(), ControlError>; + + /// Append a durable projection wake after a reporter stamped + /// `reports_check_runs`. + async fn append_check_run_projection( + &self, + run_id: RunId, + job_id: Option<&JobId>, + ) -> Result<(), ControlError>; + + /// Current end of the outbox stream (backend safe point). + async fn outbox_head(&self) -> Result, ControlError>; + + /// Up to `limit` outbox rows after `after`, in backend commit order. + async fn outbox_read( + &self, + after: OutboxBookmark, + limit: usize, + ) -> Result, ControlError>; + /// One reaper tick's inputs, read directly (no working-set load, no lock). async fn reap_inputs(&self) -> Result; @@ -1297,24 +1387,23 @@ impl Backend { } } - /// The outbox position a consumer that starts now reads from. `None` - /// on SQLite. + /// The current end of the outbox stream. SQLite returns event-id order; + /// PostgreSQL returns its `(txid,event_id)` safe point. pub(crate) async fn outbox_head(&self) -> Result, ControlError> { match self { - Self::Sqlite(_) => Ok(None), + Self::Sqlite(b) => b.outbox_head().await.map(Some), Self::Postgres(b) => b.outbox_head().await.map(Some), } } - /// Up to `limit` outbox rows after `after`, from finished transactions, - /// in `(txid, event_id)` order. Empty on SQLite. + /// Up to `limit` rows after `after`, ordered by the backend safe point. pub(crate) async fn outbox_read( &self, after: OutboxBookmark, limit: usize, ) -> Result, ControlError> { match self { - Self::Sqlite(_) => Ok(Vec::new()), + Self::Sqlite(b) => b.outbox_read(after, limit).await, Self::Postgres(b) => b.outbox_read(after, limit).await, } } @@ -1327,6 +1416,144 @@ impl Backend { } } + pub(crate) async fn consume_check_run_outbox( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result { + match self { + Self::Sqlite(b) => b.consume_check_run_outbox(owner, lease_for, limit).await, + Self::Postgres(b) => b.consume_check_run_outbox(owner, lease_for, limit).await, + } + } + + pub(crate) async fn lease_check_run_updates( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result, ControlError> { + match self { + Self::Sqlite(b) => b.lease_check_run_updates(owner, lease_for, limit).await, + Self::Postgres(b) => b.lease_check_run_updates(owner, lease_for, limit).await, + } + } + + pub(crate) async fn set_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + check_run_id: u64, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => { + b.set_check_run_update_id(owner, run_id, job_id, version, check_run_id) + .await + } + Self::Postgres(b) => { + b.set_check_run_update_id(owner, run_id, job_id, version, check_run_id) + .await + } + } + } + + pub(crate) async fn finish_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => { + b.finish_check_run_update(owner, run_id, job_id, version) + .await + } + Self::Postgres(b) => { + b.finish_check_run_update(owner, run_id, job_id, version) + .await + } + } + } + + pub(crate) async fn retry_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + permanent: bool, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => { + b.retry_check_run_update(owner, run_id, job_id, delay, permanent) + .await + } + Self::Postgres(b) => { + b.retry_check_run_update(owner, run_id, job_id, delay, permanent) + .await + } + } + } + + pub(crate) async fn clear_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + expected: u64, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => { + b.clear_check_run_update_id(owner, run_id, job_id, expected) + .await + } + Self::Postgres(b) => { + b.clear_check_run_update_id(owner, run_id, job_id, expected) + .await + } + } + } + + pub(crate) async fn defer_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => b.defer_check_run_update(owner, run_id, job_id, delay).await, + Self::Postgres(b) => b.defer_check_run_update(owner, run_id, job_id, delay).await, + } + } + + /// Append a durable projection wake after a reporter stamped + /// `reports_check_runs`; see `append_check_run_projection` on the + /// backends. + pub(crate) async fn append_check_run_projection( + &self, + run_id: RunId, + job_id: Option<&JobId>, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => b.append_check_run_projection(run_id, job_id).await, + Self::Postgres(b) => b.append_check_run_projection(run_id, job_id).await, + } + } + pub(crate) async fn enqueue_check_run_update( + &self, + update: CheckRunUpdateInput, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => b.enqueue_check_run_update(update).await, + Self::Postgres(b) => b.enqueue_check_run_update(update).await, + } + } + /// `#[cfg(test)]` working-set snapshot for test assertions. Each /// backend rebuilds the old field names from its own tables. #[cfg(any(test, feature = "test-support"))] @@ -1812,6 +2039,149 @@ impl ControlBackend for Backend { Self::Postgres(b) => b.prune_outbox(older_than, limit).await, } } + async fn consume_check_run_outbox( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result { + match self { + Self::Sqlite(b) => b.consume_check_run_outbox(owner, lease_for, limit).await, + Self::Postgres(b) => b.consume_check_run_outbox(owner, lease_for, limit).await, + } + } + async fn lease_check_run_updates( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result, ControlError> { + match self { + Self::Sqlite(b) => b.lease_check_run_updates(owner, lease_for, limit).await, + Self::Postgres(b) => b.lease_check_run_updates(owner, lease_for, limit).await, + } + } + async fn set_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + check_run_id: u64, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => { + b.set_check_run_update_id(owner, run_id, job_id, version, check_run_id) + .await + } + Self::Postgres(b) => { + b.set_check_run_update_id(owner, run_id, job_id, version, check_run_id) + .await + } + } + } + async fn finish_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => { + b.finish_check_run_update(owner, run_id, job_id, version) + .await + } + Self::Postgres(b) => { + b.finish_check_run_update(owner, run_id, job_id, version) + .await + } + } + } + async fn retry_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + permanent: bool, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => { + b.retry_check_run_update(owner, run_id, job_id, delay, permanent) + .await + } + Self::Postgres(b) => { + b.retry_check_run_update(owner, run_id, job_id, delay, permanent) + .await + } + } + } + async fn clear_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + expected: u64, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => { + b.clear_check_run_update_id(owner, run_id, job_id, expected) + .await + } + Self::Postgres(b) => { + b.clear_check_run_update_id(owner, run_id, job_id, expected) + .await + } + } + } + async fn enqueue_check_run_update( + &self, + update: CheckRunUpdateInput, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => b.enqueue_check_run_update(update).await, + Self::Postgres(b) => b.enqueue_check_run_update(update).await, + } + } + async fn defer_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => b.defer_check_run_update(owner, run_id, job_id, delay).await, + Self::Postgres(b) => b.defer_check_run_update(owner, run_id, job_id, delay).await, + } + } + async fn append_check_run_projection( + &self, + run_id: RunId, + job_id: Option<&JobId>, + ) -> Result<(), ControlError> { + match self { + Self::Sqlite(b) => b.append_check_run_projection(run_id, job_id).await, + Self::Postgres(b) => b.append_check_run_projection(run_id, job_id).await, + } + } + async fn outbox_head(&self) -> Result, ControlError> { + match self { + Self::Sqlite(b) => b.outbox_head().await.map(Some), + Self::Postgres(b) => b.outbox_head().await.map(Some), + } + } + async fn outbox_read( + &self, + after: OutboxBookmark, + limit: usize, + ) -> Result, ControlError> { + match self { + Self::Sqlite(b) => b.outbox_read(after, limit).await, + Self::Postgres(b) => b.outbox_read(after, limit).await, + } + } async fn runner_exists(&self, runner_id: i64) -> Result { match self { Self::Sqlite(b) => b.runner_exists(runner_id).await, diff --git a/crates/preloop-runner-server/src/control/lite/check_runs.rs b/crates/preloop-runner-server/src/control/lite/check_runs.rs new file mode 100644 index 000000000..32bab81d4 --- /dev/null +++ b/crates/preloop-runner-server/src/control/lite/check_runs.rs @@ -0,0 +1,175 @@ +use super::{LiteBackend, db}; +use crate::control::types::{CheckRunUpdate, ControlError, OutboxBookmark, OutboxRow}; +use preloop_gha_protocol::{JobId, RunId}; +use rusqlite::{Transaction, params}; +use serde_json::Value; + +const CONSUMER: &str = "check-runs"; +fn relevant(topic: &str) -> bool { + matches!( + topic, + "run.created.v1" + | "run.completed.v1" + | "expansion.queued.v1" + | "job.queued.v1" + | "job.started.v1" + | "job.completed.v1" + | "job_status.v1" + | "run_status.v1" + | "check_run_projection.v1" + ) +} +fn run_text(run: RunId) -> String { + run.0.to_string() +} +fn now_us() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_micros() as i64) + .unwrap_or(0) +} + +impl LiteBackend { + pub(crate) async fn enqueue_check_run_update( + &self, + update: crate::control::types::CheckRunUpdateInput, + ) -> Result<(), ControlError> { + self.write(|tx|{tx.execute("INSERT INTO check_run_updates(run_id,job_id,installation_id,check_run_id,version,payload,not_before,attempts) VALUES(?1,?2,?3,?4,?5,?6,?7,0) ON CONFLICT(run_id,job_id) DO UPDATE SET payload=excluded.payload,version=excluded.version,not_before=excluded.not_before WHERE excluded.version>check_run_updates.version",params![run_text(update.run_id),update.job_id.0,update.installation_id,update.check_run_id.map(|v|v as i64),update.version,update.payload.to_string(),now_us()]).map_err(db)?;Ok(())}) + } + pub(crate) async fn consume_check_run_outbox( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result { + self.write(|tx|{ + let now=now_us(); + tx.execute("INSERT INTO consumer_offsets(consumer_name,last_event_id) VALUES(?1,0) ON CONFLICT(consumer_name) DO NOTHING",params![CONSUMER]).map_err(db)?; + let lease=now+lease_for.as_micros() as i64; + let n=tx.execute("UPDATE consumer_offsets SET lease_owner=?2,lease_until=?3,updated_at=?4 WHERE consumer_name=?1 AND (lease_owner IS NULL OR lease_until?1 ORDER BY event_id LIMIT ?2").map_err(db)?; + let it=st.query_map(params![after,limit as i64],|r|{ + let run_id=match r.get::<_,Option>(1)? { + Some(v)=>Some(v.parse::().map(RunId).map_err(|_|rusqlite::Error::InvalidQuery)?), + None=>None, + }; + Ok(OutboxRow{bookmark:OutboxBookmark{txid:0,event_id:r.get(0)?},run_id,job_id:r.get(2)?,version:r.get(3)?,origin:r.get(4)?,topic:r.get(5)?,payload:serde_json::from_str::(&r.get::<_,String>(6)?).map_err(|_|rusqlite::Error::InvalidQuery)?}) + }).map_err(db)?; + for row in it { rows.push(row.map_err(db)?); } + } + let count=rows.len(); let mut last=after; + for row in rows { last=row.bookmark.event_id; if relevant(&row.topic){project_row(tx,&row)?;} } + if count>0 { tx.execute("UPDATE consumer_offsets SET last_event_id=?2,updated_at=?3 WHERE consumer_name=?1",params![CONSUMER,last,now]).map_err(db)?; } + Ok(count) + }) + } + pub(crate) async fn lease_check_run_updates( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result, ControlError> { + self.write(|tx|{let now=now_us();let until=now+lease_for.as_micros() as i64;let mut st=tx.prepare("SELECT run_id,job_id,installation_id,check_run_id,version,payload,attempts FROM check_run_updates WHERE not_before<=?1 AND (leased_until IS NULL OR leased_until(0)?,r.get::<_,String>(1)?,r.get::<_,i64>(2)?,r.get::<_,Option>(3)?,r.get::<_,i64>(4)?,r.get::<_,String>(5)?,r.get::<_,i32>(6)?))).map_err(db)?;let mut out=Vec::new();for row in rows{let (run,job,inst,id,ver,payload,attempts)=row.map_err(db)?;tx.execute("UPDATE check_run_updates SET lease_owner=?3,leased_until=?4 WHERE run_id=?1 AND job_id=?2",params![run,job,owner,until]).map_err(db)?;out.push(CheckRunUpdate{run_id:run.parse().map(RunId).map_err(|_|ControlError::backend(anyhow::anyhow!("bad run id")))?,job_id:JobId(job),installation_id:inst,check_run_id:id.map(|v|v as u64),version:ver,payload:serde_json::from_str(&payload).map_err(ControlError::backend)?,attempts});}Ok(out)}) + } + pub(crate) async fn set_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + id: u64, + ) -> Result<(), ControlError> { + self.write(|tx|{tx.execute("UPDATE check_run_updates SET check_run_id=?,leased_until=? WHERE lease_owner=? AND run_id=? AND job_id=? AND version=?",params![id as i64,now_us()+300_000_000,owner,run_text(run_id),job_id.0,version]).map_err(db)?;Ok(())}) + } + pub(crate) async fn finish_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + ) -> Result<(), ControlError> { + self.write(|tx|{tx.execute("DELETE FROM check_run_updates WHERE lease_owner=? AND run_id=? AND job_id=? AND version=?",params![owner,run_text(run_id),job_id.0,version]).map_err(db)?;Ok(())}) + } + pub(crate) async fn retry_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + permanent: bool, + ) -> Result<(), ControlError> { + self.write(|tx|{if permanent{tx.execute("DELETE FROM check_run_updates WHERE lease_owner=? AND run_id=? AND job_id=?",params![owner,run_text(run_id),job_id.0]).map_err(db)?;}else{tx.execute("UPDATE check_run_updates SET attempts=attempts+1,not_before=?,leased_until=NULL,lease_owner=NULL WHERE lease_owner=? AND run_id=? AND job_id=?",params![now_us()+delay.as_micros() as i64,owner,run_text(run_id),job_id.0]).map_err(db)?;}Ok(())}) + } + pub(crate) async fn clear_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + expected: u64, + ) -> Result<(), ControlError> { + self.write(|tx|{tx.execute("UPDATE check_run_updates SET check_run_id=NULL WHERE lease_owner=? AND run_id=? AND job_id=? AND check_run_id=?",params![owner,run_text(run_id),job_id.0,expected as i64]).map_err(db)?;Ok(())}) + } + + /// Release a lease without counting an attempt: the sender is waiting out + /// a rate limit, not failing to deliver. + pub(crate) async fn defer_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + ) -> Result<(), ControlError> { + self.write(|tx|{tx.execute("UPDATE check_run_updates SET not_before=?,leased_until=NULL,lease_owner=NULL WHERE lease_owner=? AND run_id=? AND job_id=?",params![now_us()+delay.as_micros() as i64,owner,run_text(run_id),job_id.0]).map_err(db)?;Ok(())}) + } + + /// Append a durable projection wake for one run (or one job) *after* a + /// reporter stamped `reports_check_runs`. + /// + /// Unlike a `JobStatus` event this is never stamped `Stale`, so a job + /// already terminal when the run's reporting flag lands (an `if: false` + /// leg concluded during submit) still reaches the projector. + pub(crate) async fn append_check_run_projection( + &self, + run_id: RunId, + job_id: Option<&JobId>, + ) -> Result<(), ControlError> { + self.write(|tx| { + let payload = serde_json::json!({"job_id": job_id.map(|job| job.0.clone())}); + super::jobs::insert_outbox( + tx, + super::jobs::namespace_of(tx, run_id)?.as_str(), + Some(run_id), + job_id.map(|job| job.0.as_str()), + None, + "check_run_projection.v1", + &payload.to_string(), + ) + }) + } +} +fn project_row(tx: &Transaction<'_>, row: &OutboxRow) -> Result<(), ControlError> { + let Some(run_id) = row.run_id else { + return Ok(()); + }; + let mut st=tx.prepare("SELECT r.reports_check_runs,r.repository,COALESCE(ps.effective_sha,json_extract(s.submission,'$.status_check_sha'),json_extract(s.submission,'$.sha'),r.head_sha),j.job_id,j.status,j.version,COALESCE(js.display_name,j.job_id),j.check_run_id,j.kind FROM runs r JOIN jobs j ON j.run_id=r.run_id LEFT JOIN job_specs js ON js.run_id=j.run_id AND js.job_id=j.job_id LEFT JOIN run_submissions s ON s.run_id=r.run_id LEFT JOIN run_push_states ps ON ps.run_id=r.run_id WHERE r.run_id=?1 AND (?2 IS NULL OR j.job_id=?2)").map_err(db)?; + let mut rows = st + .query(params![run_text(run_id), row.job_id.as_deref()]) + .map_err(db)?; + while let Some(r) = rows.next().map_err(db)? { + let reports: bool = r.get(0).map_err(db)?; + let kind: String = r.get(8).map_err(db)?; + if !reports || kind == "matrix_parent" || kind == "reusable_caller" { + continue; + } + let job: String = r.get(3).map_err(db)?; + let ver: i64 = r.get(5).map_err(db)?; + let payload = serde_json::json!({"repository":r.get::<_,String>(1).map_err(db)?,"sha":r.get::<_,String>(2).map_err(db)?,"job_id":job,"status":r.get::<_,String>(4).map_err(db)?,"name":r.get::<_,String>(6).map_err(db)?}); + let id: Option = r.get(7).map_err(db)?; + tx.execute("INSERT INTO check_run_updates(run_id,job_id,installation_id,check_run_id,version,payload,not_before,attempts,leased_until,lease_owner) VALUES(?1,?2,0,?3,?4,?5,?6,0,NULL,NULL) ON CONFLICT(run_id,job_id) DO UPDATE SET payload=excluded.payload,version=excluded.version,check_run_id=COALESCE(excluded.check_run_id,check_run_updates.check_run_id),not_before=excluded.not_before,leased_until=NULL,lease_owner=NULL,attempts=0 WHERE excluded.version>check_run_updates.version",params![run_text(run_id),job,id,ver,payload.to_string(),now_us()]).map_err(db)?; + } + Ok(()) +} diff --git a/crates/preloop-runner-server/src/control/lite/impls.rs b/crates/preloop-runner-server/src/control/lite/impls.rs index b857bbfb7..e3141e6d3 100644 --- a/crates/preloop-runner-server/src/control/lite/impls.rs +++ b/crates/preloop-runner-server/src/control/lite/impls.rs @@ -233,6 +233,97 @@ impl ControlBackend for LiteBackend { ) -> Result { self.prune_outbox(older_than, limit).await } + async fn consume_check_run_outbox( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result { + self.consume_check_run_outbox(owner, lease_for, limit).await + } + async fn lease_check_run_updates( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result, ControlError> { + self.lease_check_run_updates(owner, lease_for, limit).await + } + async fn set_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + check_run_id: u64, + ) -> Result<(), ControlError> { + self.set_check_run_update_id(owner, run_id, job_id, version, check_run_id) + .await + } + async fn finish_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + ) -> Result<(), ControlError> { + self.finish_check_run_update(owner, run_id, job_id, version) + .await + } + async fn retry_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + permanent: bool, + ) -> Result<(), ControlError> { + self.retry_check_run_update(owner, run_id, job_id, delay, permanent) + .await + } + async fn clear_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + expected: u64, + ) -> Result<(), ControlError> { + self.clear_check_run_update_id(owner, run_id, job_id, expected) + .await + } + async fn enqueue_check_run_update( + &self, + update: CheckRunUpdateInput, + ) -> Result<(), ControlError> { + self.enqueue_check_run_update(update).await + } + async fn defer_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + ) -> Result<(), ControlError> { + self.defer_check_run_update(owner, run_id, job_id, delay) + .await + } + async fn append_check_run_projection( + &self, + run_id: RunId, + job_id: Option<&JobId>, + ) -> Result<(), ControlError> { + self.append_check_run_projection(run_id, job_id).await + } + async fn outbox_head(&self) -> Result, ControlError> { + self.outbox_head().await.map(Some) + } + async fn outbox_read( + &self, + after: OutboxBookmark, + limit: usize, + ) -> Result, ControlError> { + self.outbox_read(after, limit).await + } async fn reap_inputs(&self) -> Result { self.reap_inputs().await } diff --git a/crates/preloop-runner-server/src/control/lite/lifecycle.rs b/crates/preloop-runner-server/src/control/lite/lifecycle.rs index 5c7e91621..44faad691 100644 --- a/crates/preloop-runner-server/src/control/lite/lifecycle.rs +++ b/crates/preloop-runner-server/src/control/lite/lifecycle.rs @@ -1148,8 +1148,11 @@ impl LiteBackend { self.write(|tx| { tx.execute( "DELETE FROM outbox_events WHERE event_id IN ( \ - SELECT event_id FROM outbox_events WHERE created_at < ?1 \ - ORDER BY created_at LIMIT ?2)", + SELECT e.event_id FROM outbox_events e \ + WHERE e.created_at < ?1 AND \ + e.event_id <= COALESCE((SELECT MIN(last_event_id) FROM consumer_offsets), \ + e.event_id) \ + ORDER BY e.created_at LIMIT ?2)", params![cutoff, limit as i64], ) .map(|n| n as u64) @@ -1157,6 +1160,64 @@ impl LiteBackend { }) } + /// The SQLite outbox position at the current end of the single-writer + /// stream. SQLite event ids are commit order, so no txid safe point is + /// needed. + pub(crate) async fn outbox_head(&self) -> Result { + self.read(|tx| { + let event_id: i64 = tx + .query_row( + "SELECT COALESCE(MAX(event_id),0) FROM outbox_events", + [], + |r| r.get(0), + ) + .map_err(db)?; + Ok(OutboxBookmark { txid: 0, event_id }) + }) + } + + pub(crate) async fn outbox_read( + &self, + after: OutboxBookmark, + limit: usize, + ) -> Result, ControlError> { + self.read(|tx| { + let mut stmt = tx + .prepare( + "SELECT event_id,run_id,job_id,version,'' AS origin,topic,payload \ + FROM outbox_events WHERE event_id > ?1 ORDER BY event_id LIMIT ?2", + ) + .map_err(db)?; + let rows = stmt + .query_map(params![after.event_id, limit as i64], |row| { + let run_id = match row.get::<_, Option>(1)? { + Some(value) => Some( + value + .parse() + .map(RunId) + .map_err(|_| rusqlite::Error::InvalidQuery)?, + ), + None => None, + }; + Ok(OutboxRow { + bookmark: OutboxBookmark { + txid: 0, + event_id: row.get(0)?, + }, + run_id, + job_id: row.get(2)?, + version: row.get(3)?, + origin: row.get(4)?, + topic: row.get(5)?, + payload: serde_json::from_str(&row.get::<_, String>(6)?) + .map_err(|_| rusqlite::Error::InvalidQuery)?, + }) + }) + .map_err(db)?; + rows.collect::, _>>().map_err(db) + }) + } + /// `reconcile_on_boot` (pg lifecycle.rs): drop holds/waits of dead or /// terminal runs, requeue claimed jobs whose session is gone, reset /// `expanding` nodes to `pending_expansion`. diff --git a/crates/preloop-runner-server/src/control/lite/mod.rs b/crates/preloop-runner-server/src/control/lite/mod.rs index e192eec6a..e87e82e4a 100644 --- a/crates/preloop-runner-server/src/control/lite/mod.rs +++ b/crates/preloop-runner-server/src/control/lite/mod.rs @@ -13,6 +13,7 @@ //! the SQL mirrors the Postgres backend statement for statement. mod acquire; +mod check_runs; mod codec; mod concurrency; mod dispatch; @@ -48,7 +49,7 @@ use std::time::Duration; /// The schema this build reads and writes. Greenfield: any other stamped /// version is refused at open (no migrations). -pub(crate) const SCHEMA_VERSION: &str = "3"; +pub(crate) const SCHEMA_VERSION: &str = "4"; /// The translated schema (see the file header for the type mapping). const SCHEMA_SQL: &str = include_str!("schema.sql"); diff --git a/crates/preloop-runner-server/src/control/lite/schema.sql b/crates/preloop-runner-server/src/control/lite/schema.sql index d70f35d37..c47e5b7c5 100644 --- a/crates/preloop-runner-server/src/control/lite/schema.sql +++ b/crates/preloop-runner-server/src/control/lite/schema.sql @@ -554,12 +554,13 @@ CREATE TABLE check_run_updates ( job_id TEXT NOT NULL, installation_id INTEGER NOT NULL, check_run_id INTEGER, + version INTEGER NOT NULL DEFAULT 0, payload TEXT NOT NULL, not_before INTEGER NOT NULL DEFAULT (CAST(unixepoch('subsec') * 1000000 AS INTEGER)), attempts INTEGER NOT NULL DEFAULT 0, leased_until INTEGER, - PRIMARY KEY (run_id, job_id), - FOREIGN KEY (run_id, job_id) REFERENCES jobs(run_id, job_id) ON DELETE CASCADE + lease_owner TEXT, + PRIMARY KEY (run_id, job_id) ); CREATE INDEX check_run_updates_queue ON check_run_updates(installation_id, not_before); diff --git a/crates/preloop-runner-server/src/control/pg/check_runs.rs b/crates/preloop-runner-server/src/control/pg/check_runs.rs new file mode 100644 index 000000000..0abd61e33 --- /dev/null +++ b/crates/preloop-runner-server/src/control/pg/check_runs.rs @@ -0,0 +1,251 @@ +use super::{PgBackend, codec, db}; +use crate::control::types::{CheckRunUpdate, ControlError, OutboxBookmark, OutboxRow}; +use preloop_gha_protocol::{JobId, RunId}; +use tokio_postgres::Transaction; + +const CONSUMER: &str = "check-runs"; + +fn relevant(topic: &str) -> bool { + matches!( + topic, + "run.created.v1" + | "run.completed.v1" + | "expansion.queued.v1" + | "job.queued.v1" + | "job.started.v1" + | "job.completed.v1" + | "job_status.v1" + | "run_status.v1" + | "check_run_projection.v1" + ) +} + +fn run_text(run: RunId) -> String { + run.0.to_string() +} + +impl PgBackend { + pub(crate) async fn enqueue_check_run_update( + &self, + update: crate::control::types::CheckRunUpdateInput, + ) -> Result<(), ControlError> { + let client = self.writer().await?; + let payload = serde_json::to_string(&update.payload).map_err(ControlError::backend)?; + client.execute("INSERT INTO check_run_updates(run_id,job_id,installation_id,check_run_id,version,payload,not_before,attempts) VALUES($1::text::uuid,$2,$3,$4,$5,$6::text::jsonb,now(),0) ON CONFLICT(run_id,job_id) DO UPDATE SET payload=EXCLUDED.payload,version=EXCLUDED.version,not_before=now() WHERE EXCLUDED.version>check_run_updates.version", &[&update.run_id.0.to_string(),&update.job_id.0,&update.installation_id,&update.check_run_id.map(|v|v as i64),&update.version,&payload]).await.map(|_|()).map_err(db) + } + pub(crate) async fn consume_check_run_outbox( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result { + let mut client = self.writer().await?; + let tx = client.transaction().await.map_err(db)?; + tx.execute( + "INSERT INTO consumer_offsets (consumer_name,last_txid,last_event_id) VALUES ($1,'0',0) ON CONFLICT DO NOTHING", + &[&CONSUMER], + ).await.map_err(db)?; + let leased = tx.query_opt( + "UPDATE consumer_offsets SET lease_owner=$2, lease_until=now()+make_interval(secs=>$3), updated_at=now() \ + WHERE consumer_name=$1 AND (lease_owner IS NULL OR lease_until < now() OR lease_owner=$2) RETURNING last_txid::text,last_event_id", + &[&CONSUMER, &owner, &lease_for.as_secs_f64()], + ).await.map_err(db)?; + let Some(offset) = leased else { + tx.rollback().await.map_err(db)?; + return Ok(0); + }; + let after = OutboxBookmark { + txid: offset + .get::<_, String>(0) + .parse() + .map_err(ControlError::backend)?, + event_id: offset.get(1), + }; + let rows = read_rows(&tx, after, limit).await?; + let count = rows.len(); + let mut bookmark = after; + for row in rows { + bookmark = row.bookmark; + if relevant(&row.topic) { + project_row(&tx, &row).await?; + } + } + if count != 0 { + tx.execute("UPDATE consumer_offsets SET last_txid=$2::text::xid8,last_event_id=$3,updated_at=now() WHERE consumer_name=$1", &[&CONSUMER, &bookmark.txid.to_string(), &bookmark.event_id]).await.map_err(db)?; + } + tx.commit().await.map_err(db)?; + Ok(count) + } + + pub(crate) async fn lease_check_run_updates( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result, ControlError> { + let mut client = self.writer().await?; + let tx = client.transaction().await.map_err(db)?; + let rows = tx.query( + "WITH picked AS (SELECT run_id,job_id FROM check_run_updates WHERE not_before<=now() AND (leased_until IS NULL OR leased_until$3) FROM picked p WHERE u.run_id=p.run_id AND u.job_id=p.job_id \ + RETURNING u.run_id::text,u.job_id,u.installation_id,u.check_run_id,u.version,u.payload::text,u.attempts", + &[&owner, &(limit as i64), &lease_for.as_secs_f64()]).await.map_err(db)?; + let out = rows + .into_iter() + .map(|r| { + Ok(CheckRunUpdate { + run_id: codec::run_id(r.get(0))?, + job_id: JobId(r.get(1)), + installation_id: r.get(2), + check_run_id: r.get::<_, Option>(3).map(|v| v as u64), + version: r.get(4), + payload: codec::from_json(r.get::<_, String>(5).as_str())?, + attempts: r.get(6), + }) + }) + .collect::, ControlError>>(); + tx.commit().await.map_err(db)?; + out + } + + pub(crate) async fn set_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + id: u64, + ) -> Result<(), ControlError> { + let client = self.writer().await?; + client.execute("UPDATE check_run_updates SET check_run_id=$5,leased_until=now()+interval '5 minutes' WHERE lease_owner=$1 AND run_id=$2::text::uuid AND job_id=$3 AND version=$4", &[&owner,&run_text(run_id),&job_id.0,&version,&(id as i64)]).await.map(|_|()).map_err(db) + } + pub(crate) async fn finish_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + ) -> Result<(), ControlError> { + let client = self.writer().await?; + client.execute("DELETE FROM check_run_updates WHERE lease_owner=$1 AND run_id=$2::text::uuid AND job_id=$3 AND version=$4", &[&owner,&run_text(run_id),&job_id.0,&version]).await.map(|_|()).map_err(db) + } + pub(crate) async fn retry_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + permanent: bool, + ) -> Result<(), ControlError> { + let client = self.writer().await?; + if permanent { + client.execute("DELETE FROM check_run_updates WHERE lease_owner=$1 AND run_id=$2::text::uuid AND job_id=$3", &[&owner,&run_text(run_id),&job_id.0]).await.map_err(db)?; + } else { + client.execute("UPDATE check_run_updates SET attempts=attempts+1,not_before=now()+make_interval(secs=>$4),leased_until=NULL,lease_owner=NULL WHERE lease_owner=$1 AND run_id=$2::text::uuid AND job_id=$3", &[&owner,&run_text(run_id),&job_id.0,&delay.as_secs_f64()]).await.map_err(db)?; + } + Ok(()) + } + pub(crate) async fn clear_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + expected: u64, + ) -> Result<(), ControlError> { + let client = self.writer().await?; + client.execute("UPDATE check_run_updates SET check_run_id=NULL WHERE lease_owner=$1 AND run_id=$2::text::uuid AND job_id=$3 AND check_run_id=$4", &[&owner,&run_text(run_id),&job_id.0,&(expected as i64)]).await.map(|_|()).map_err(db) + } + + /// Release a lease without counting an attempt: the sender is waiting out + /// a rate limit, not failing to deliver. + pub(crate) async fn defer_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + ) -> Result<(), ControlError> { + let client = self.writer().await?; + client.execute("UPDATE check_run_updates SET not_before=now()+make_interval(secs=>$4),leased_until=NULL,lease_owner=NULL WHERE lease_owner=$1 AND run_id=$2::text::uuid AND job_id=$3", &[&owner,&run_text(run_id),&job_id.0,&delay.as_secs_f64()]).await.map(|_|()).map_err(db) + } + + /// Append a durable projection wake for one run (or one job) *after* a + /// reporter stamped `reports_check_runs`. + /// + /// Unlike a `JobStatus` event this is never stamped `Stale`, so a job + /// already terminal when the run's reporting flag lands (an `if: false` + /// leg concluded during submit) still reaches the projector. + pub(crate) async fn append_check_run_projection( + &self, + run_id: RunId, + job_id: Option<&JobId>, + ) -> Result<(), ControlError> { + let mut client = self.writer().await?; + let tx = client.transaction().await.map_err(db)?; + let payload = serde_json::json!({"job_id": job_id.map(|job| job.0.clone())}); + super::dispatch::emit_outbox(&tx, Some(run_id), "check_run_projection.v1", payload).await?; + tx.commit().await.map_err(db)?; + Ok(()) + } +} + +async fn read_rows( + tx: &Transaction<'_>, + after: OutboxBookmark, + limit: usize, +) -> Result, ControlError> { + let rows=tx.query("SELECT txid::text,event_id,run_id::text,job_id,version,origin,topic,payload::text FROM outbox_events WHERE (txid,event_id)>($1::text::xid8,$2) AND txid(0) + .parse() + .map_err(ControlError::backend)?, + event_id: r.get(1), + }, + run_id: r + .get::<_, Option>(2) + .map(|v| codec::run_id(&v)) + .transpose()?, + job_id: r.get(3), + version: r.get(4), + origin: r.get(5), + topic: r.get(6), + payload: codec::from_json(r.get::<_, String>(7).as_str())?, + }) + }) + .collect() +} + +async fn project_row(tx: &Transaction<'_>, row: &OutboxRow) -> Result<(), ControlError> { + let Some(run_id) = row.run_id else { + return Ok(()); + }; + let filter = row.job_id.as_deref(); + let sql = if filter.is_some() { + "SELECT r.reports_check_runs,r.repository,COALESCE(ps.effective_sha,s.submission->>'status_check_sha',s.submission->>'sha',r.head_sha),j.job_id,j.status,j.version,COALESCE(js.display_name,j.job_id),j.check_run_id,j.kind FROM runs r JOIN jobs j ON j.run_id=r.run_id LEFT JOIN job_specs js ON js.run_id=j.run_id AND js.job_id=j.job_id LEFT JOIN run_submissions s ON s.run_id=r.run_id LEFT JOIN run_push_states ps ON ps.run_id=r.run_id WHERE r.run_id=$1::text::uuid AND j.job_id=$2" + } else { + "SELECT r.reports_check_runs,r.repository,COALESCE(ps.effective_sha,s.submission->>'status_check_sha',s.submission->>'sha',r.head_sha),j.job_id,j.status,j.version,COALESCE(js.display_name,j.job_id),j.check_run_id,j.kind FROM runs r JOIN jobs j ON j.run_id=r.run_id LEFT JOIN job_specs js ON js.run_id=j.run_id AND js.job_id=j.job_id LEFT JOIN run_submissions s ON s.run_id=r.run_id LEFT JOIN run_push_states ps ON ps.run_id=r.run_id WHERE r.run_id=$1::text::uuid" + }; + let rows = if let Some(job) = filter { + tx.query(sql, &[&run_text(run_id), &job]).await + } else { + tx.query(sql, &[&run_text(run_id)]).await + } + .map_err(db)?; + for r in rows { + let reports: bool = r.get(0); + let kind: String = r.get(8); + if !reports || kind == "matrix_parent" || kind == "reusable_caller" { + continue; + } + let status: String = r.get(4); + let version: i64 = r.get(5); + let payload = serde_json::json!({"repository":r.get::<_,String>(1),"sha":r.get::<_,String>(2),"job_id":r.get::<_,String>(3),"status":status,"name":r.get::<_,String>(6)}); + let id: r#Option = r.get(7); + tx.execute("INSERT INTO check_run_updates(run_id,job_id,installation_id,check_run_id,version,payload,not_before,attempts,leased_until,lease_owner) VALUES($1::text::uuid,$2,0,$3,$4,$5::text::jsonb,now(),0,NULL,NULL) ON CONFLICT(run_id,job_id) DO UPDATE SET payload=EXCLUDED.payload,version=EXCLUDED.version,check_run_id=COALESCE(EXCLUDED.check_run_id,check_run_updates.check_run_id),not_before=now(),leased_until=NULL,lease_owner=NULL,attempts=0 WHERE EXCLUDED.version>check_run_updates.version", &[&run_text(run_id),&r.get::<_,String>(3),&id,&version,&payload.to_string()]).await.map_err(db)?; + } + Ok(()) +} diff --git a/crates/preloop-runner-server/src/control/pg/mod.rs b/crates/preloop-runner-server/src/control/pg/mod.rs index d8cdf7041..fa820b446 100644 --- a/crates/preloop-runner-server/src/control/pg/mod.rs +++ b/crates/preloop-runner-server/src/control/pg/mod.rs @@ -18,6 +18,7 @@ //! - [`lookups`]: request/callback/check-run/run lookups, push state, //! run numbers, key fingerprint, archival and queue statistics. +mod check_runs; mod codec; mod dispatch; mod graph; @@ -39,7 +40,7 @@ use tokio_postgres::{Client, NoTls}; /// The schema this build creates and accepts. Greenfield v1: there are no /// migrations, a database at any other version is refused. -pub(crate) const SCHEMA_VERSION: &str = "4"; +pub(crate) const SCHEMA_VERSION: &str = "5"; /// The agreed schema plus `schema_meta`. const SCHEMA_SQL: &str = include_str!("schema.sql"); diff --git a/crates/preloop-runner-server/src/control/pg/outbox.rs b/crates/preloop-runner-server/src/control/pg/outbox.rs index c5668a51d..e01a35209 100644 --- a/crates/preloop-runner-server/src/control/pg/outbox.rs +++ b/crates/preloop-runner-server/src/control/pg/outbox.rs @@ -119,10 +119,14 @@ impl PgBackend { let client = self.writer().await?; client .execute( - "DELETE FROM outbox_events WHERE (event_id, created_at) IN \ - (SELECT event_id, created_at FROM outbox_events \ - WHERE created_at < now() - make_interval(secs => $1) \ - ORDER BY created_at LIMIT $2)", + "DELETE FROM outbox_events o WHERE (o.event_id,o.created_at) IN \ + (SELECT e.event_id,e.created_at FROM outbox_events e \ + WHERE e.created_at < now() - make_interval(secs => $1) \ + AND (NOT EXISTS (SELECT 1 FROM consumer_offsets) OR \ + (e.txid,e.event_id) <= \ + (SELECT last_txid,last_event_id FROM consumer_offsets \ + ORDER BY last_txid,last_event_id LIMIT 1)) \ + ORDER BY e.created_at LIMIT $2)", &[&older_than.as_secs_f64(), &(limit as i64)], ) .await diff --git a/crates/preloop-runner-server/src/control/pg/schema.sql b/crates/preloop-runner-server/src/control/pg/schema.sql index 915f4e759..5119a5574 100644 --- a/crates/preloop-runner-server/src/control/pg/schema.sql +++ b/crates/preloop-runner-server/src/control/pg/schema.sql @@ -658,12 +658,13 @@ CREATE TABLE check_run_updates ( job_id text NOT NULL, installation_id bigint NOT NULL, check_run_id bigint, + version bigint NOT NULL DEFAULT 0, payload jsonb NOT NULL, not_before timestamptz NOT NULL DEFAULT now(), attempts integer NOT NULL DEFAULT 0, leased_until timestamptz, - PRIMARY KEY (run_id, job_id), - FOREIGN KEY (run_id, job_id) REFERENCES jobs(run_id, job_id) ON DELETE CASCADE + lease_owner text, + PRIMARY KEY (run_id, job_id) ); CREATE INDEX check_run_updates_queue ON check_run_updates(installation_id, not_before); diff --git a/crates/preloop-runner-server/src/control/pg/trait_impl.rs b/crates/preloop-runner-server/src/control/pg/trait_impl.rs index d4b5198d2..a8bed4307 100644 --- a/crates/preloop-runner-server/src/control/pg/trait_impl.rs +++ b/crates/preloop-runner-server/src/control/pg/trait_impl.rs @@ -235,6 +235,97 @@ impl ControlBackend for PgBackend { ) -> Result { self.prune_outbox(older_than, limit).await } + async fn consume_check_run_outbox( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result { + self.consume_check_run_outbox(owner, lease_for, limit).await + } + async fn lease_check_run_updates( + &self, + owner: &str, + lease_for: std::time::Duration, + limit: usize, + ) -> Result, ControlError> { + self.lease_check_run_updates(owner, lease_for, limit).await + } + async fn set_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + check_run_id: u64, + ) -> Result<(), ControlError> { + self.set_check_run_update_id(owner, run_id, job_id, version, check_run_id) + .await + } + async fn finish_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + version: i64, + ) -> Result<(), ControlError> { + self.finish_check_run_update(owner, run_id, job_id, version) + .await + } + async fn retry_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + permanent: bool, + ) -> Result<(), ControlError> { + self.retry_check_run_update(owner, run_id, job_id, delay, permanent) + .await + } + async fn clear_check_run_update_id( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + expected: u64, + ) -> Result<(), ControlError> { + self.clear_check_run_update_id(owner, run_id, job_id, expected) + .await + } + async fn enqueue_check_run_update( + &self, + update: CheckRunUpdateInput, + ) -> Result<(), ControlError> { + self.enqueue_check_run_update(update).await + } + async fn defer_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + delay: std::time::Duration, + ) -> Result<(), ControlError> { + self.defer_check_run_update(owner, run_id, job_id, delay) + .await + } + async fn append_check_run_projection( + &self, + run_id: RunId, + job_id: Option<&JobId>, + ) -> Result<(), ControlError> { + self.append_check_run_projection(run_id, job_id).await + } + async fn outbox_head(&self) -> Result, ControlError> { + self.outbox_head().await.map(Some) + } + async fn outbox_read( + &self, + after: OutboxBookmark, + limit: usize, + ) -> Result, ControlError> { + self.outbox_read(after, limit).await + } async fn reap_inputs(&self) -> Result { self.reap_inputs().await } diff --git a/crates/preloop-runner-server/src/control/tests.rs b/crates/preloop-runner-server/src/control/tests.rs index 69967b772..352fe8b80 100644 --- a/crates/preloop-runner-server/src/control/tests.rs +++ b/crates/preloop-runner-server/src/control/tests.rs @@ -2663,6 +2663,370 @@ pub(crate) mod suite { ); } + /// Drain the projector until a short batch, returning rows consumed. + async fn project_all_check_run_events(backend: &dyn ControlBackend, owner: &str) -> usize { + let mut total = 0; + for _ in 0..64 { + let n = backend + .consume_check_run_outbox(owner, std::time::Duration::from_secs(30), 256) + .await + .unwrap(); + total += n; + if n < 256 { + break; + } + } + total + } + + async fn lease_check_runs( + backend: &dyn ControlBackend, + owner: &str, + ) -> Vec { + backend + .lease_check_run_updates(owner, std::time::Duration::from_secs(30), 32) + .await + .unwrap() + } + + async fn outbox_len(backend: &dyn ControlBackend) -> usize { + backend + .outbox_read( + OutboxBookmark { + txid: 0, + event_id: 0, + }, + 10_000, + ) + .await + .unwrap() + .len() + } + + /// Consume until the projector has nothing left to read. + /// + /// On a shared Postgres server a concurrent transaction (another test's + /// database, same cluster) holds `pg_snapshot_xmin` back, hiding freshly + /// committed rows for a moment; retry instead of assuming instant + /// visibility. + async fn consume_until_quiescent(backend: &dyn ControlBackend, owner: &str) -> usize { + let mut total = 0; + for _ in 0..600 { + let n = backend + .consume_check_run_outbox(owner, std::time::Duration::from_secs(30), 256) + .await + .unwrap(); + total += n; + if n == 0 && total > 0 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + total + } + + /// Read the outbox length once two consecutive reads agree, so a delayed + /// commit cannot make an assertion count a half-visible stream. + async fn stable_outbox_len(backend: &dyn ControlBackend) -> usize { + let mut last = outbox_len(backend).await; + for _ in 0..200 { + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + let now = outbox_len(backend).await; + if now == last { + return now; + } + last = now; + } + last + } + + /// Wait until the projector has materialized `status` for the job. + async fn wait_for_projected_status( + backend: &dyn ControlBackend, + run_id: RunId, + job_id: &JobId, + status: &str, + ) -> crate::control::types::CheckRunUpdate { + for _ in 0..600 { + let _ = consume_until_quiescent(backend, "suite-projector").await; + let rows = lease_check_runs(backend, "suite-probe").await; + if let Some(row) = rows + .iter() + .find(|row| row.run_id == run_id && row.job_id == *job_id) + && row.payload["status"] == status + { + return row.clone(); + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + panic!("check-run row for {run_id}/{job_id} never reached {status}"); + } + + /// The desired-state row is latest-version-wins: an older projected event + /// (or a direct insert) never overwrites a newer row, so a late `queued` + /// cannot regress a `success` already in the queue. + pub(crate) async fn check_run_projection_never_regresses_a_newer_row( + backend: &dyn ControlBackend, + ) { + let run_id = RunId::new(); + let build = JobId("build".to_owned()); + backend + .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) + .await + .unwrap(); + backend.set_reports_check_runs(run_id, true).await.unwrap(); + + // A newer desired state than any event this run can produce. + backend + .enqueue_check_run_update(CheckRunUpdateInput { + run_id, + job_id: build.clone(), + installation_id: 0, + check_run_id: None, + version: 9_999, + payload: serde_json::json!({ + "repository": "owner/repo", + "sha": "abc123", + "job_id": "build", + "status": "success", + "name": "build", + }), + }) + .await + .unwrap(); + // Direct insert of an older version is a no-op too. + backend + .enqueue_check_run_update(CheckRunUpdateInput { + run_id, + job_id: build.clone(), + installation_id: 0, + check_run_id: None, + version: 3, + payload: serde_json::json!({ + "repository": "owner/repo", + "sha": "abc123", + "job_id": "build", + "status": "queued", + "name": "build", + }), + }) + .await + .unwrap(); + + // A real projection wake for the same job must not regress the row. + backend + .append_check_run_projection(run_id, Some(&build)) + .await + .unwrap(); + project_all_check_run_events(backend, "suite-projector").await; + + let rows = lease_check_runs(backend, "suite-probe").await; + let row = rows + .iter() + .find(|row| row.run_id == run_id && row.job_id == build) + .expect("the queue holds a row for build"); + assert_eq!( + row.version, 9_999, + "an older projected version must not overwrite a newer row" + ); + assert_eq!(row.payload["status"], "success"); + } + + /// The acquire transition must reach GitHub: claiming a job writes + /// `job.started.v1` and moves the row to in_progress, the projector + /// coalesces that into an in_progress queue row, and the sender then + /// PATCHes it (exercised end-to-end by the github.rs sender tests). + pub(crate) async fn acquire_projects_in_progress_check_run(backend: &dyn ControlBackend) { + let run_id = RunId::new(); + let build = JobId("build".to_owned()); + let runner = backend + .register_runner(register_runner("check-run-r1")) + .await + .unwrap(); + let session = backend + .create_session(create_session(runner.runner.id)) + .await + .unwrap(); + backend + .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) + .await + .unwrap(); + backend.set_reports_check_runs(run_id, true).await.unwrap(); + backend + .append_check_run_projection(run_id, Some(&build)) + .await + .unwrap(); + let queued = wait_for_projected_status(backend, run_id, &build, "queued").await; + let queued_version = queued.version; + + let poll = backend + .poll_session(poll(&session.session_id, runner.runner.id)) + .await + .unwrap(); + assert!( + matches!(poll, PollOutcome::Claimed(_)), + "expected a claim, got {poll:?}" + ); + let row = wait_for_projected_status(backend, run_id, &build, "in_progress").await; + assert!( + row.version > queued_version, + "the acquire transition bumps the row version ({} vs {queued_version})", + row.version + ); + } + + /// The consumer bookmark is durable state, not process memory: a second + /// consumer owner (a restarted process) resumes exactly where the first + /// stopped, so events committed while it was down are delivered once. + pub(crate) async fn check_run_bookmark_survives_consumer_restart(backend: &dyn ControlBackend) { + let run_id = RunId::new(); + backend + .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) + .await + .unwrap(); + let total = backend + .outbox_read( + OutboxBookmark { + txid: 0, + event_id: 0, + }, + 10_000, + ) + .await + .unwrap() + .len(); + assert!(total > 1, "a submit emits several outbox rows"); + + // First consumer reads exactly one event and commits the bookmark. + // Its lease is left expired so the restarted consumer can take over + // without waiting out the production lease window. Retry: on a shared + // Postgres cluster a concurrent transaction can hide the row briefly. + let mut first = 0; + for _ in 0..600 { + first = backend + .consume_check_run_outbox( + "suite-consumer-1", + std::time::Duration::from_millis(1), + 1, + ) + .await + .unwrap(); + if first == 1 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + assert_eq!(first, 1, "the first consumer reads one event"); + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + + // Events committed while it was "down" (the second submit) plus the + // remainder are delivered after the restart. Retry until the counts + // settle: a concurrent transaction on a shared Postgres cluster can + // delay visibility of freshly committed rows. + let run_id2 = RunId::new(); + backend + .submit_run(submit_run(run_id2, vec![submit_job(run_id2, "build", 1)])) + .await + .unwrap(); + let mut delivered = 0; + for _ in 0..600 { + delivered += backend + .consume_check_run_outbox( + "suite-consumer-2", + std::time::Duration::from_secs(30), + 256, + ) + .await + .unwrap(); + if delivered > 0 && first + delivered == stable_outbox_len(backend).await { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + assert!( + delivered > 0, + "events committed while the consumer was down are delivered after restart" + ); + assert_eq!( + first + delivered, + stable_outbox_len(backend).await, + "a restarted consumer must resume at the persisted bookmark, not replay the stream" + ); + } + + /// Outbox prune never passes the durable consumer bookmark, and still + /// bounds growth by age when no consumer has registered. + pub(crate) async fn prune_outbox_respects_slowest_consumer(backend: &dyn ControlBackend) { + // No consumer: age alone bounds the stream. + let run_id = RunId::new(); + backend + .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) + .await + .unwrap(); + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + let mut before = 0; + for _ in 0..600 { + before = stable_outbox_len(backend).await; + if before > 0 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + assert!(before > 0, "committed outbox rows become visible"); + let pruned = backend + .prune_outbox(std::time::Duration::ZERO, 10_000) + .await + .unwrap(); + assert!( + pruned >= 1, + "with no durable consumer, age prunes the stream" + ); + assert_eq!( + outbox_len(backend).await, + before - pruned as usize, + "prune removes exactly the rows it counted" + ); + + // A consumer's bookmark: rows at/below it are prunable, rows past it + // are retained however old they are. + let run_id = RunId::new(); + backend + .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) + .await + .unwrap(); + let fast = consume_until_quiescent(backend, "prune-consumer-fast").await; + assert!(fast > 0); + let run_id2 = RunId::new(); + backend + .submit_run(submit_run(run_id2, vec![submit_job(run_id2, "build", 1)])) + .await + .unwrap(); + let mut total = 0; + for _ in 0..600 { + total = stable_outbox_len(backend).await; + if total > 0 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + assert!(total > 0); + let pruned = backend + .prune_outbox(std::time::Duration::ZERO, 10_000) + .await + .unwrap(); + assert!(pruned >= 1, "rows at or below the bookmark are prunable"); + let remaining = outbox_len(backend).await; + assert_eq!( + remaining, + total - pruned as usize, + "prune removes exactly the rows it counted" + ); + assert!( + remaining >= 1, + "rows past the consumer bookmark must survive an age prune" + ); + } + /// `run_dispatch_info` flags the expandable placeholder nodes — a /// deferred-matrix parent and a reusable caller. They never dispatch: /// expansion replaces them with the legs that mint their own checks, so @@ -5051,6 +5415,113 @@ mod pg { suite::prune_outbox_keeps_fresh_rows_and_bounds_a_batch(&backend).await; } + #[tokio::test] + async fn check_run_projection_never_regresses_a_newer_row() { + let Some((_pg, backend)) = backend().await else { + return skip_no_postgres(); + }; + suite::check_run_projection_never_regresses_a_newer_row(&backend).await; + } + + #[tokio::test] + async fn acquire_projects_in_progress_check_run() { + let Some((_pg, backend)) = backend().await else { + return skip_no_postgres(); + }; + suite::acquire_projects_in_progress_check_run(&backend).await; + } + + #[tokio::test] + async fn check_run_bookmark_survives_consumer_restart() { + let Some((_pg, backend)) = backend().await else { + return skip_no_postgres(); + }; + suite::check_run_bookmark_survives_consumer_restart(&backend).await; + } + + #[tokio::test] + async fn prune_outbox_respects_slowest_consumer() { + let Some((_pg, backend)) = backend().await else { + return skip_no_postgres(); + }; + suite::prune_outbox_respects_slowest_consumer(&backend).await; + } + + /// The consumer bookmark is row state in `consumer_offsets`, so a second + /// connection (another node, or the process after a restart) resumes at + /// it instead of replaying the stream. + #[tokio::test] + async fn check_run_bookmark_survives_a_second_connection() { + let Some((_pg, first, second)) = backend_pair().await else { + return skip_no_postgres(); + }; + let run_id = RunId::new(); + first + .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) + .await + .unwrap(); + let mut consumed = 0; + for _ in 0..600 { + consumed = first + .consume_check_run_outbox("pg-consumer-1", std::time::Duration::from_millis(1), 256) + .await + .unwrap(); + if consumed > 0 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + assert!(consumed > 0, "the first connection reads the run's events"); + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + + let run_id2 = RunId::new(); + second + .submit_run(submit_run(run_id2, vec![submit_job(run_id2, "build", 1)])) + .await + .unwrap(); + // Retry until the counts settle: a concurrent transaction on the + // shared test cluster can hold `pg_snapshot_xmin` back. + let mut delivered = 0; + for _ in 0..600 { + delivered += second + .consume_check_run_outbox("pg-consumer-2", std::time::Duration::from_secs(30), 256) + .await + .unwrap(); + let total = second + .outbox_read( + OutboxBookmark { + txid: 0, + event_id: 0, + }, + 10_000, + ) + .await + .unwrap() + .len(); + if delivered > 0 && consumed + delivered == total { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + assert!(delivered > 0, "the down-window events are delivered"); + let total = second + .outbox_read( + OutboxBookmark { + txid: 0, + event_id: 0, + }, + 10_000, + ) + .await + .unwrap() + .len(); + assert_eq!( + consumed + delivered, + total, + "a second connection must resume at the persisted bookmark (no replay, no skip)" + ); + } + #[tokio::test] async fn sessionless_runner_is_not_idle_capacity() { let Some((_pg, backend)) = backend().await else { @@ -6099,6 +6570,85 @@ mod lite { .await; } + #[tokio::test] + async fn check_run_projection_never_regresses_a_newer_row() { + suite::check_run_projection_never_regresses_a_newer_row(&LiteBackend::in_memory().unwrap()) + .await; + } + + #[tokio::test] + async fn acquire_projects_in_progress_check_run() { + suite::acquire_projects_in_progress_check_run(&LiteBackend::in_memory().unwrap()).await; + } + + #[tokio::test] + async fn check_run_bookmark_survives_consumer_restart() { + suite::check_run_bookmark_survives_consumer_restart(&LiteBackend::in_memory().unwrap()) + .await; + } + + #[tokio::test] + async fn prune_outbox_respects_slowest_consumer() { + suite::prune_outbox_respects_slowest_consumer(&LiteBackend::in_memory().unwrap()).await; + } + + /// The bookmark lives in the SQLite file: reopen the database and only the + /// events committed while the consumer was down are delivered. + #[tokio::test] + async fn check_run_bookmark_survives_a_file_reopen() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("preloop.db"); + let run_id = RunId::new(); + let consumed = { + let backend = + LiteBackend::open(&path, false, false, std::time::Duration::from_secs(300)) + .unwrap(); + backend + .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) + .await + .unwrap(); + backend + .consume_check_run_outbox( + "reopen-consumer", + std::time::Duration::from_millis(1), + 256, + ) + .await + .unwrap() + }; + assert!(consumed > 0); + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + + // Down window: a fresh handle appends without consuming. + let reopened = + LiteBackend::open(&path, false, false, std::time::Duration::from_secs(300)).unwrap(); + let run_id2 = RunId::new(); + reopened + .submit_run(submit_run(run_id2, vec![submit_job(run_id2, "build", 1)])) + .await + .unwrap(); + let total = reopened + .outbox_read( + crate::control::types::OutboxBookmark { + txid: 0, + event_id: 0, + }, + 10_000, + ) + .await + .unwrap() + .len(); + let delivered = reopened + .consume_check_run_outbox("reopen-consumer-2", std::time::Duration::from_secs(30), 256) + .await + .unwrap(); + assert_eq!( + delivered, + total - consumed, + "a reopened consumer must resume at the persisted bookmark" + ); + } + #[tokio::test] async fn sessionless_runner_is_not_idle_capacity() { suite::sessionless_runner_is_not_idle_capacity(&LiteBackend::in_memory().unwrap()).await; diff --git a/crates/preloop-runner-server/src/control/types.rs b/crates/preloop-runner-server/src/control/types.rs index c7483ab2a..84d78a050 100644 --- a/crates/preloop-runner-server/src/control/types.rs +++ b/crates/preloop-runner-server/src/control/types.rs @@ -216,14 +216,15 @@ pub(crate) fn status_parse(s: &str) -> ExecutionStatus { } } -/// A position in the outbox. Rows are read in `(txid, event_id)` order. +/// A position in the transactional outbox. PostgreSQL uses `(txid,event_id)`; +/// SQLite leaves `txid` at zero and orders by `event_id`. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub(crate) struct OutboxBookmark { pub(crate) txid: u64, pub(crate) event_id: i64, } -/// One `outbox_events` row, as the stream consumer reads it. +/// One `outbox_events` row, as a stream consumer reads it. #[derive(Debug, Clone)] pub(crate) struct OutboxRow { pub(crate) bookmark: OutboxBookmark, @@ -238,6 +239,29 @@ pub(crate) struct OutboxRow { pub(crate) payload: serde_json::Value, } +/// A coalesced desired check-run state leased by the background sender. +#[derive(Debug, Clone)] +pub(crate) struct CheckRunUpdate { + pub(crate) run_id: RunId, + pub(crate) job_id: JobId, + pub(crate) installation_id: i64, + pub(crate) check_run_id: Option, + pub(crate) version: i64, + pub(crate) payload: serde_json::Value, + pub(crate) attempts: i32, +} + +/// Backend-neutral durable update for one desired check-run state. +#[derive(Debug, Clone)] +pub(crate) struct CheckRunUpdateInput { + pub(crate) run_id: RunId, + pub(crate) job_id: JobId, + pub(crate) installation_id: i64, + pub(crate) check_run_id: Option, + pub(crate) version: i64, + pub(crate) payload: serde_json::Value, +} + /// Everything the submit path pre-built outside the transaction: the run /// record plus one queue entry per non-skipped job and the correlation /// records minted for dispatchable jobs. diff --git a/crates/preloop-runner-server/src/event_feed.rs b/crates/preloop-runner-server/src/event_feed.rs index 9f9fc0df5..afc68c059 100644 --- a/crates/preloop-runner-server/src/event_feed.rs +++ b/crates/preloop-runner-server/src/event_feed.rs @@ -51,6 +51,7 @@ pub(crate) fn spawn( events: broadcast::Sender, dirty: Arc, ) { + crate::check_run_outbox::spawn_consumer(backend, Arc::clone(&dirty)); let Some(origin) = backend.event_origin().map(str::to_owned) else { return; }; diff --git a/crates/preloop-runner-server/src/github.rs b/crates/preloop-runner-server/src/github.rs index 1987e48bf..3e234b002 100644 --- a/crates/preloop-runner-server/src/github.rs +++ b/crates/preloop-runner-server/src/github.rs @@ -226,12 +226,29 @@ async fn send_github_check_request( ) .await?; + // Honour the primary budget advertised on the response: once remaining + // hits zero, wait for the reset instead of spending the next call on a + // guaranteed 403. + breaker.observe_rate_budget(res.headers()); + if !res.status().is_success() { let status = res.status(); + let retry_after = res + .headers() + .get("retry-after") + .and_then(|v| v.to_str().ok()) + .unwrap_or("") + .to_owned(); + let reset = res + .headers() + .get("x-ratelimit-reset") + .and_then(|v| v.to_str().ok()) + .unwrap_or("") + .to_owned(); let err_text = res.text().await.unwrap_or_default(); record_check_reporting(shared, false); return Err(anyhow::anyhow!( - "GitHub Check API failed with status {}: {}", + "GitHub Check API failed with status {}: {}; retry_after={retry_after}; rate_reset={reset}", status, err_text )); @@ -306,77 +323,35 @@ pub fn run_details_url(run_id: RunId) -> Option { /// check runs too". pub async fn report_check_run_queued( shared: &Arc, - repo: &str, - sha: &str, + _repo: &str, + _sha: &str, job_id: &JobId, run_id: RunId, ) -> anyhow::Result> { - // Every intake path that reports checks funnels through here; persist the - // flag so a job materialized later (runtime-expanded leg, reusable callee) - // still mints checks, including after a control-plane restart. shared .state .backend .set_reports_check_runs(run_id, true) .await .map_err(|e| anyhow::anyhow!("{e:?}"))?; - let existing_check_run_id = shared + // The state transition already normally emitted a durable event. Intake + // paths such as rerun/push can report a queued check without changing the + // scheduler row, and a job already terminal when the flag lands would + // stamp a `JobStatus` event `Stale`; append an unconditional projection + // wake instead. + shared .state .backend - .job_check_run_id(run_id, job_id) + .append_check_run_projection(run_id, Some(job_id)) .await .map_err(|e| anyhow::anyhow!("{e:?}"))?; - // The per-run mint lock is still node-local metadata: it only serializes - // concurrent reporters on this node so two of them cannot both observe - // "no mapping" and POST. Fetched before the (slow) token resolution so a - // caller that blocks on it does not re-resolve the token afterwards. - let mint_lock = { - let mut inner = shared.state.inner.lock().await; - inner - .check_run_mint_locks - .entry(run_id) - .or_default() - .clone() - }; - let token = resolve_check_run_token(shared, repo).await; - - if let Some(check_run_id) = existing_check_run_id { - match report_existing_check_run_queued(shared, repo, job_id, run_id, check_run_id).await { - Ok(()) => return Ok(Some(check_run_id)), - Err(error) if !is_check_run_not_found(&error) => return Err(error), - Err(error) => { - warn!( - %run_id, - %job_id, - check_run_id, - %error, - "persisted GitHub check run is stale; reconciling it" - ); - shared - .state - .backend - .clear_job_check_run(run_id, job_id, check_run_id) - .await - .map_err(|e| anyhow::anyhow!("{e:?}"))?; - } - } - } - - // Mint under the per-run lock and re-check the mapping inside it: two - // reporters (e.g. an expansion mint and a claim-time in-progress report) - // can both see "no mapping" and POST, and GitHub accepts duplicate check - // runs for the same name+SHA — the loser would strand a `queued` check. - let _guard = mint_lock.lock().await; - let existing = shared + shared.state.events_dirty.notify_one(); + shared .state .backend .job_check_run_id(run_id, job_id) .await - .map_err(|e| anyhow::anyhow!("{e:?}"))?; - if let Some(check_run_id) = existing { - return Ok(Some(check_run_id)); - } - mint_check_run(shared, repo, sha, job_id, run_id, token.as_deref()).await + .map_err(|e| anyhow::anyhow!("{e:?}")) } /// The repository + SHA check runs for this run attach to, when the run @@ -396,198 +371,69 @@ fn check_run_report_coords(run: &crate::models::RunRecord) -> Option<(String, St Some((run.submission.repository.clone(), sha)) } -/// Return the job's check run id, minting a `queued` check when the job was -/// materialized after intake reported (runtime-expanded matrix legs, -/// reusable callee jobs). `None` when the run does not report checks or the -/// mint fails — callers then behave as before this fix: skip the report. +/// Return an already persisted job→check-run mapping, if any. +/// +/// The sender is solely responsible for creating a missing GitHub check run; +/// request handlers never mint one. Callers that need a check for a +/// late-materialized job call [`report_check_run_queued`], which stamps +/// `reports_check_runs` and appends a durable projection wake. pub async fn ensure_check_run_mapped( shared: &Arc, run_id: RunId, job_id: &JobId, ) -> Option { - let run = shared.state.backend.run_record(run_id).await.ok()?; - if let Some(id) = run.job_check_run_ids.get(job_id).copied() { - return Some(id); - } - let coords = check_run_report_coords(&run)?; - let mint_lock = { - let mut inner = shared.state.inner.lock().await; - inner - .check_run_mint_locks - .entry(run_id) - .or_default() - .clone() - }; - let _guard = mint_lock.lock().await; - let existing = shared + shared .state .backend .job_check_run_id(run_id, job_id) .await .ok() - .flatten(); - if let Some(id) = existing { - return Some(id); - } - let (repo, sha) = coords; - let token = resolve_check_run_token(shared, &repo).await; - match mint_check_run(shared, &repo, &sha, job_id, run_id, token.as_deref()).await { - Ok(id) => id, - Err(error) => { - warn!(%run_id, %job_id, ?error, "failed to mint check run for materialized job"); - None - } - } -} - -/// POST a `queued` check run (or mint a mock id when no token resolves) and -/// record the job→check-run mapping. Caller holds the per-run mint lock. -async fn mint_check_run( - shared: &Arc, - repo: &str, - sha: &str, - job_id: &JobId, - run_id: RunId, - token: Option<&str>, -) -> anyhow::Result> { - let check_run_id = if let Some(token) = token { - let details_url = run_details_url(run_id); - - let job_name = shared - .state - .backend - .job_display_name(run_id, job_id) - .await - .map_err(|e| anyhow::anyhow!("{e:?}"))? - .unwrap_or_else(|| job_id.0.clone()); - let mut body = serde_json::json!({ - "name": job_name, - "head_sha": sha, - "status": "queued", - "output": { - "title": job_name, - "summary": format!("Waiting for a preloop runner.\n\njob_id: `{}`", job_id.0) - } - }); - if let Some(url) = details_url { - body["details_url"] = serde_json::json!(url); - } - - match send_github_check_request( - shared, - &shared.state.github_breaker, - token, - repo, - reqwest::Method::POST, - "check-runs", - &body, - ) - .await - { - Ok(response) => match response.get("id").and_then(Value::as_u64) { - Some(id) => { - info!( - %run_id, - %job_id, - check_run_id = id, - "GitHub check run created successfully" - ); - id - } - None => find_existing_check_run(shared, token, repo, sha, &job_id.to_string()) - .await? - .ok_or_else(|| anyhow::anyhow!("GitHub check-run POST returned no id"))?, - }, - Err(error) => { - // A transport error is ambiguous: GitHub may have - // committed the POST before the connection failed. - match find_existing_check_run(shared, token, repo, sha, &job_id.to_string()).await { - Ok(Some(id)) => id, - Ok(None) => return Err(error), - Err(reconcile_error) => { - return Err(anyhow::anyhow!( - "{error}; check-run reconciliation failed: {reconcile_error}" - )); - } - } - } - } - } else { - info!(%run_id, %job_id, "GitHub token not configured, using mock check run"); - rand::random::() as u64 - }; - - let mapping_changed = shared - .state - .backend - .set_job_check_run(run_id, job_id, check_run_id) - .await - .map_err(|e| anyhow::anyhow!("{e:?}"))?; - if mapping_changed { - // The mapping is meaningful while the run lives, and the next status - // event may be hours away. `set_job_check_run` persisted the event - // inside its own transaction; only the broadcast remains. - shared - .state - .emit_persisted(preloop_gha_protocol::NdjsonEvent::CheckRunCreated { run_id }) - .await; - } - Ok(Some(check_run_id)) + .flatten() } fn is_check_run_not_found(error: &anyhow::Error) -> bool { error.to_string().contains("status 404") } -/// Move an existing GitHub check run back to the queue after a rerequest. +/// Record a rerequest's desired queued state; the sender performs the PATCH. pub async fn report_existing_check_run_queued( shared: &Arc, - repo: &str, + _repo: &str, job_id: &JobId, run_id: RunId, check_run_id: u64, ) -> anyhow::Result<()> { - let token = resolve_check_run_token(shared, repo).await; - if let Some(token) = &token { - let mut body = serde_json::json!({ - "status": "queued", - }); - if let Some(url) = run_details_url(run_id) { - body["details_url"] = serde_json::json!(url); - } - let path = format!("check-runs/{check_run_id}"); - if let Err(error) = send_github_check_request( - shared, - &shared.state.github_breaker, - token, - repo, - reqwest::Method::PATCH, - &path, - &body, - ) + shared + .state + .backend + .set_job_check_run(run_id, job_id, check_run_id) .await - { - warn!( - %run_id, - %job_id, - check_run_id, - %error, - "Failed to requeue GitHub check run" - ); - return Err(error); - } - } else { - info!( - %run_id, - %job_id, - check_run_id, - "Mock requeued GitHub check run" - ); - } + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + shared + .state + .backend + .append_event(&NdjsonEvent::JobStatus { + run_id, + job_id: job_id.clone(), + status: ExecutionStatus::Queued, + reason: None, + }) + .await + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + // The status event above is dropped when the job row settled differently; + // the wake still delivers whatever state the row holds. + shared + .state + .backend + .append_check_run_projection(run_id, Some(job_id)) + .await + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + shared.state.events_dirty.notify_one(); Ok(()) } async fn find_existing_check_run( shared: &Arc, + breaker: &crate::github_breaker::GithubBreaker, token: &str, repo: &str, sha: &str, @@ -595,7 +441,7 @@ async fn find_existing_check_run( ) -> anyhow::Result> { let payload = match send_github_check_request( shared, - &shared.state.github_breaker, + breaker, token, repo, reqwest::Method::GET, @@ -621,20 +467,325 @@ async fn find_existing_check_run( })) } -/// Report a permanent failure check run to GitHub (e.g. invalid workflow YAML, expression failure). +/// Per-installation rate budget key. /// -/// The check name and head SHA are the idempotency key. A retry after a crash -/// first finds the existing check and patches it, avoiding duplicate failed -/// checks in GitHub's UI. -pub async fn report_check_run_permanent_failure( +/// Installation ids come from [`crate::github_app::installation_for_repo`]; +/// the PAT fallback shares one bucket because it is one token. `0` is never a +/// valid GitHub installation id, so it is never used as a key. +#[derive(Clone, PartialEq, Eq, Hash, Debug)] +pub(crate) enum CheckRunRateKey { + Installation(u64), + Pat, +} + +/// Ceiling on how long a rate limit may park a row; a bogus header must not +/// strand a check run for a day. +const MAX_DEFER: std::time::Duration = std::time::Duration::from_secs(3_600); + +/// The single background sender for the durable check-run queue. +/// +/// The serve bootstrap spawns one per node; tests build their own and drive +/// [`CheckRunSender::drain_once`]. Rate budgets are per installation, so a +/// throttled installation parks only its own rows and the rest keep flowing. +pub(crate) struct CheckRunSender { + owner: String, + budget: parking_lot::Mutex< + std::collections::HashMap>, + >, +} + +impl Default for CheckRunSender { + fn default() -> Self { + Self::new() + } +} + +impl CheckRunSender { + pub(crate) fn new() -> Self { + Self { + owner: uuid::Uuid::new_v4().to_string(), + budget: parking_lot::Mutex::new(std::collections::HashMap::new()), + } + } + + fn breaker(&self, key: &CheckRunRateKey) -> Arc { + let mut budget = self.budget.lock(); + budget + .entry(key.clone()) + .or_insert_with(|| Arc::new(crate::github_breaker::GithubBreaker::default())) + .clone() + } + + /// Lease and process one batch. Returns how many rows were leased; the + /// caller keeps looping while batches arrive and sleeps when none do. + pub(crate) async fn drain_once(&self, shared: &Arc) -> usize { + const BATCH: usize = 32; + const LEASE: std::time::Duration = std::time::Duration::from_secs(30); + let updates = match shared + .state + .backend + .lease_check_run_updates(&self.owner, LEASE, BATCH) + .await + { + Ok(updates) => updates, + Err(error) => { + warn!(?error, "check-run sender lease failed"); + return 0; + } + }; + let leased = updates.len(); + for update in updates { + self.send_one(shared, &update).await; + } + leased + } + + async fn send_one( + &self, + shared: &Arc, + update: &crate::control::types::CheckRunUpdate, + ) { + let repo = update + .payload + .get("repository") + .and_then(Value::as_str) + .unwrap_or_default() + .to_owned(); + // Resolve the real installation before budgeting or sending: the + // queue row's `installation_id` is projection metadata (0 = unknown), + // never an authority. + let key = resolve_check_run_rate_key(shared, &repo).await; + let breaker = self.breaker(&key); + if let Some(remaining) = breaker.retry_after() { + // Waiting out a rate limit is not a delivery failure: release the + // row due at GitHub's advertised resume time without burning an + // attempt, so a long throttle cannot exhaust the permanent-drop + // budget. + if let Err(error) = shared + .state + .backend + .defer_check_run_update( + &self.owner, + update.run_id, + &update.job_id, + remaining.min(MAX_DEFER), + ) + .await + { + warn!(run_id=%update.run_id, job_id=%update.job_id, ?error, "failed to defer rate-limited check-run row"); + } + return; + } + let token = resolve_check_run_token(shared, &repo).await; + match send_queued_check_run(shared, &self.owner, &breaker, token.as_deref(), update).await { + Ok(()) => {} + Err(error) if error.to_string().contains("circuit breaker is open") => { + // A breaker that opened between the budget check and the call + // is still a deferral, not a failure. + let remaining = breaker + .retry_after() + .unwrap_or(std::time::Duration::from_secs(30)); + let _ = shared + .state + .backend + .defer_check_run_update( + &self.owner, + update.run_id, + &update.job_id, + remaining.min(MAX_DEFER), + ) + .await; + } + Err(error) => { + let permanent = update.attempts >= 7 || error.to_string().contains("status 422"); + let delay = check_retry_delay(&error, update.attempts); + warn!(run_id=%update.run_id, job_id=%update.job_id, attempts=update.attempts, permanent, ?error, "check-run sender attempt failed"); + if let Err(store_error) = shared + .state + .backend + .retry_check_run_update( + &self.owner, + update.run_id, + &update.job_id, + delay, + permanent, + ) + .await + { + warn!(run_id=%update.run_id, job_id=%update.job_id, ?store_error, "failed to persist check-run retry state"); + } + } + } + } +} + +/// The installation (or PAT) whose budget governs this repository's check +/// runs. Resolution failure falls back to the PAT bucket; it never invents an +/// installation id. +async fn resolve_check_run_rate_key(shared: &Arc, repo: &str) -> CheckRunRateKey { + match crate::github_app::select_app_for_repo(shared, repo).await { + Some(app) => match crate::github_app::installation_for_repo(&app, repo).await { + Ok(installation) if installation != 0 => CheckRunRateKey::Installation(installation), + _ => CheckRunRateKey::Pat, + }, + None => CheckRunRateKey::Pat, + } +} + +/// Drain the durable check-run queue. This is the only path that invokes the +/// GitHub Checks API; request handlers only append desired-state events. +pub(crate) async fn run_check_run_sender(shared: Arc) { + let sender = CheckRunSender::new(); + while !shared.shutdown.is_cancelled() { + let leased = sender.drain_once(&shared).await; + if leased == 0 { + tokio::select! { + _ = shared.shutdown.cancelled() => break, + _ = tokio::time::sleep(std::time::Duration::from_millis(250)) => {}, + } + } + } +} + +/// Test-only single pass of the check-run sender, mirroring the bootstrap +/// loop. Tests that assert a check-run mapping was recorded must drain the +/// sender: the sender is the only path that writes one, so a webhook alone +/// leaves `job_check_run_ids` empty. +#[cfg(any(test, feature = "test-support"))] +pub async fn drain_check_run_sender(shared: &Arc) -> usize { + CheckRunSender::new().drain_once(shared).await +} + +fn check_retry_delay(error: &anyhow::Error, attempts: i32) -> std::time::Duration { + let text = error.to_string(); + if let Some(value) = text + .split("retry_after=") + .nth(1) + .and_then(|v| v.split(';').next()) + && let Ok(seconds) = value.trim().parse::() + && seconds > 0 + { + return std::time::Duration::from_secs(seconds.min(86_400)); + } + // `rate_reset` is `x-ratelimit-reset`: an absolute unix timestamp. + if let Some(value) = text + .split("rate_reset=") + .nth(1) + .and_then(|v| v.split(|c: char| !c.is_ascii_digit()).next()) + && let Ok(reset) = value.parse::() + { + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|since| since.as_secs()) + .unwrap_or(0); + if reset > now { + return std::time::Duration::from_secs((reset - now).min(86_400)); + } + } + if text.contains("secondary rate limit") || text.contains("abuse detection") { + return std::time::Duration::from_secs(60); + } + let base = 250_u64.saturating_mul(1_u64 << (attempts.clamp(0, 8) as u32)); + std::time::Duration::from_millis(base + rand::random::() % 250) +} + +/// Render and deliver one queued update. `breaker` is the resolved +/// installation's rate budget; every request this function makes goes +/// through it. +async fn send_queued_check_run( shared: &Arc, - repo: &str, - sha: &str, - name: &str, - summary: &str, + owner: &str, + breaker: &crate::github_breaker::GithubBreaker, + token: Option<&str>, + update: &crate::control::types::CheckRunUpdate, ) -> anyhow::Result<()> { - let token = resolve_check_run_token(shared, repo).await; - if let Some(token) = &token { + let repo = update + .payload + .get("repository") + .and_then(Value::as_str) + .ok_or_else(|| anyhow::anyhow!("check-run queue row has no repository"))?; + let sha = update + .payload + .get("sha") + .and_then(Value::as_str) + .ok_or_else(|| anyhow::anyhow!("check-run queue row has no head sha"))?; + let name = update + .payload + .get("name") + .and_then(Value::as_str) + .unwrap_or(update.job_id.0.as_str()); + let status = update + .payload + .get("status") + .and_then(Value::as_str) + .unwrap_or("queued"); + let Some(token) = token else { + info!(run_id=%update.run_id, job_id=%update.job_id, status, "GitHub token not configured; check-run update simulated locally"); + // A token-less engine still records a local check-run id. The base + // minted exactly this mock at submit, before the sender took over + // minting; the mapping is what `job_check_run_ids` serves and what + // must survive a restart, so dropping it would silently erase the + // mock path (`check_run_ids_survive_a_restart_before_any_job_event`). + let existing = shared + .state + .backend + .job_check_run_id(update.run_id, &update.job_id) + .await + .ok() + .flatten(); + if existing.is_none() { + let mock = rand::random::() as u64; + let mapping_changed = shared + .state + .backend + .set_job_check_run(update.run_id, &update.job_id, mock) + .await + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + if mapping_changed { + shared + .state + .emit_persisted(preloop_gha_protocol::NdjsonEvent::CheckRunCreated { + run_id: update.run_id, + }) + .await; + } + } + shared + .state + .backend + .finish_check_run_update(owner, update.run_id, &update.job_id, update.version) + .await + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + return Ok(()); + }; + let mut check_run_id = update.check_run_id; + if check_run_id.is_none() { + // The persisted job mapping is the first dedup source: a previous + // attempt may have POSTed and saved the mapping while failing to save + // the queue row. + check_run_id = shared + .state + .backend + .job_check_run_id(update.run_id, &update.job_id) + .await + .ok() + .flatten(); + if let Some(id) = check_run_id { + persist_check_run_id(shared, owner, update, id).await?; + } + } + if check_run_id.is_none() { + // Crash-after-POST reconciliation: never POST a second check run for + // the same commit + name. + check_run_id = find_existing_check_run(shared, breaker, token, repo, sha, name).await?; + if let Some(id) = check_run_id { + persist_check_run_id(shared, owner, update, id).await?; + } + } + + if update.payload.get("kind").and_then(Value::as_str) == Some("workflow_failure") { + // Synthetic workflow-evaluation failures have no run row; keep the + // body byte-identical to the pre-queue reporter. let body = serde_json::json!({ "name": name, "head_sha": sha, @@ -643,27 +794,322 @@ pub async fn report_check_run_permanent_failure( "completed_at": chrono::Utc::now().to_rfc3339(), "output": { "title": "Workflow evaluation failed", - "summary": summary, + "summary": update.payload.get("summary").and_then(Value::as_str).unwrap_or(""), } }); - let existing = find_existing_check_run(shared, token, repo, sha, name).await?; - let (method, path) = match existing { - Some(check_run_id) => (reqwest::Method::PATCH, format!("check-runs/{check_run_id}")), - None => (reqwest::Method::POST, "check-runs".to_owned()), - }; - send_github_check_request( + send_check_run_body( shared, - &shared.state.github_breaker, + owner, + breaker, token, repo, - method, - &path, - &body, + sha, + name, + update, + check_run_id, + body, ) .await?; - } else { - info!(%repo, %name, "GitHub token not configured, mock failure check run recorded"); + finish_check_run_update(shared, owner, update).await?; + return Ok(()); + } + + match status { + // `pending` is a held job (needs/approval); it reports as a queued + // check exactly as intake did. Only terminal statuses conclude one. + "queued" | "pending" | "in_progress" => { + let mut body = if status == "in_progress" { + serde_json::json!({"status":"in_progress","started_at":chrono::Utc::now().to_rfc3339(),"output":{"title":name,"summary":"Running in preloop."}}) + } else { + serde_json::json!({"name":name,"head_sha":sha,"status":"queued","output":{"title":name,"summary":format!("Waiting for a preloop runner.\n\njob_id: `{}`",update.job_id.0)}}) + }; + if let Some(url) = run_details_url(update.run_id) { + body["details_url"] = Value::String(url); + } + send_check_run_body( + shared, + owner, + breaker, + token, + repo, + sha, + name, + update, + check_run_id, + body, + ) + .await?; + } + _ => { + let info = shared + .state + .backend + .run_dispatch_info(update.run_id) + .await + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + let conclusion = match status { + "success" => "success", + "failure" => "failure", + "cancelled" => "cancelled", + "skipped" => "skipped", + _ => "failure", + }; + let (title, summary, started_at, completed_at, annotations) = if let Some(info) = info { + let job = info.jobs.iter().find(|job| job.job_id == update.job_id); + let steps = job.map(|j| j.steps.clone()).unwrap_or_default(); + let (annotations, mut global_issues) = + timeline_annotations(shared, update.run_id, &update.job_id).await; + if let Some(detail) = job.and_then(|j| j.detail.as_ref()) { + for annotation in &detail.annotations { + let message = annotation + .get("message") + .and_then(Value::as_str) + .map(str::to_owned) + .unwrap_or_else(|| annotation.to_string()); + global_issues.push(format!("- {}", markdown_cell(&message))); + } + } + let summary = check_summary(conclusion, &steps, &global_issues, &update.job_id); + let title = job + .and_then(|j| j.display_name.as_deref()) + .unwrap_or(name) + .to_owned(); + let started_at = steps + .iter() + .filter_map(|step| step.started_at) + .min() + .or_else(|| info.started_at.map(chrono::DateTime::::from)); + let completed_at = steps + .iter() + .filter_map(|step| step.finished_at) + .max() + .or_else(|| info.completed_at.map(chrono::DateTime::::from)) + .unwrap_or_else(chrono::Utc::now); + (title, summary, started_at, completed_at, annotations) + } else { + ( + name.to_owned(), + update + .payload + .get("summary") + .and_then(Value::as_str) + .unwrap_or("") + .to_owned(), + None, + chrono::Utc::now(), + Vec::new(), + ) + }; + // Annotations travel 50 per request, exactly as before; only the + // final request concludes the check run. + let chunks: Vec<&[Value]> = if annotations.is_empty() { + vec![&[]] + } else { + annotations.chunks(50).collect() + }; + let mut id = check_run_id; + for (index, chunk) in chunks.iter().enumerate() { + let last = index + 1 == chunks.len(); + let mut body = serde_json::json!({ + "output": {"title": title, "summary": summary, "annotations": chunk} + }); + if last { + body["status"] = Value::String("completed".to_owned()); + body["conclusion"] = Value::String(conclusion.to_owned()); + body["completed_at"] = Value::String(completed_at.to_rfc3339()); + if let Some(started_at) = started_at { + body["started_at"] = Value::String(started_at.to_rfc3339()); + } + if let Some(url) = run_details_url(update.run_id) { + body["details_url"] = Value::String(url); + } + } else if id.is_none() { + // Creating a check run requires a status; annotations + // arrive over the follow-up PATCHes. + body["status"] = Value::String("queued".to_owned()); + } + id = send_check_run_body( + shared, owner, breaker, token, repo, sha, &title, update, id, body, + ) + .await?; + } + } } + finish_check_run_update(shared, owner, update).await?; + Ok(()) +} + +/// The in-memory annotation stream for one job, split into GitHub annotation +/// payloads and free-text failure details (annotations without a file). +async fn timeline_annotations( + shared: &Arc, + run_id: RunId, + job_id: &JobId, +) -> (Vec, Vec) { + let inner = shared.state.inner.lock().await; + let mut annotations = Vec::new(); + let mut global_issues = Vec::new(); + if let Some(events) = inner.timeline_events.get(&run_id) { + for event in events { + if let NdjsonEvent::Annotation { + job_id: event_job_id, + level, + message, + file, + line, + .. + } = event + && event_job_id == job_id + { + let level_str = match level { + AnnotationLevel::Notice => "notice", + AnnotationLevel::Warning => "warning", + AnnotationLevel::Error => "failure", + }; + if let Some(file_path) = file { + let line_num = line.unwrap_or(1); + annotations.push(serde_json::json!({ + "path": file_path, + "start_line": line_num, + "end_line": line_num, + "annotation_level": level_str, + "message": message, + })); + } else { + global_issues.push(format!("**{}**: {}", level_str.to_uppercase(), message)); + } + } + } + } + (annotations, global_issues) +} + +/// Send one Checks API request for `update`, creating the check run on POST. +/// Returns the check-run id (persisted alongside the queue row). +#[allow(clippy::too_many_arguments)] +async fn send_check_run_body( + shared: &Arc, + owner: &str, + breaker: &crate::github_breaker::GithubBreaker, + token: &str, + repo: &str, + sha: &str, + name: &str, + update: &crate::control::types::CheckRunUpdate, + check_run_id: Option, + mut body: Value, +) -> anyhow::Result> { + let (method, path) = match check_run_id { + Some(id) => (reqwest::Method::PATCH, format!("check-runs/{id}")), + None => { + // Creation requires the check name and commit. + body["name"] = Value::String(name.to_owned()); + body["head_sha"] = Value::String(sha.to_owned()); + (reqwest::Method::POST, "check-runs".to_owned()) + } + }; + let response = + match send_github_check_request(shared, breaker, token, repo, method, &path, &body).await { + Ok(response) => response, + Err(error) => { + if let Some(stale) = check_run_id.filter(|_| is_check_run_not_found(&error)) { + let _ = shared + .state + .backend + .clear_check_run_update_id(owner, update.run_id, &update.job_id, stale) + .await; + let _ = shared + .state + .backend + .clear_job_check_run(update.run_id, &update.job_id, stale) + .await; + } + return Err(error); + } + }; + let Some(id) = check_run_id else { + let id = response + .get("id") + .and_then(Value::as_u64) + .or(find_existing_check_run(shared, breaker, token, repo, sha, name).await?) + .ok_or_else(|| anyhow::anyhow!("GitHub check-run POST returned no id"))?; + persist_check_run_id(shared, owner, update, id).await?; + return Ok(Some(id)); + }; + Ok(Some(id)) +} + +/// Record a GitHub check-run id on both the queue row (version-guarded) and +/// the job mapping. +async fn persist_check_run_id( + shared: &Arc, + owner: &str, + update: &crate::control::types::CheckRunUpdate, + id: u64, +) -> anyhow::Result<()> { + shared + .state + .backend + .set_check_run_update_id(owner, update.run_id, &update.job_id, update.version, id) + .await + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + shared + .state + .backend + .set_job_check_run(update.run_id, &update.job_id, id) + .await + .map_err(|e| anyhow::anyhow!("saved check-run id {id} but job mapping failed: {e:?}"))?; + Ok(()) +} + +async fn finish_check_run_update( + shared: &Arc, + owner: &str, + update: &crate::control::types::CheckRunUpdate, +) -> anyhow::Result<()> { + shared + .state + .backend + .finish_check_run_update(owner, update.run_id, &update.job_id, update.version) + .await + .map_err(|e| anyhow::anyhow!("{e:?}")) +} + +/// Report a permanent failure check run to GitHub (e.g. invalid workflow YAML, expression failure). +/// +/// The check name and head SHA are the idempotency key. A retry after a crash +/// first finds the existing check and patches it, avoiding duplicate failed +/// checks in GitHub's UI. +pub async fn report_check_run_permanent_failure( + shared: &Arc, + repo: &str, + sha: &str, + name: &str, + summary: &str, +) -> anyhow::Result<()> { + let run_id = RunId::new(); + let update = crate::control::types::CheckRunUpdateInput { + run_id, + job_id: JobId(format!("__workflow_failure__:{name}")), + installation_id: 0, + check_run_id: None, + version: 0, + payload: serde_json::json!({ + "kind": "workflow_failure", + "repository": repo, + "sha": sha, + "name": name, + "status": "completed", + "summary": summary, + }), + }; + shared + .state + .backend + .enqueue_check_run_update(update) + .await + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + shared.state.events_dirty.notify_one(); Ok(()) } @@ -673,148 +1119,58 @@ pub async fn report_check_runs_for_run( run_id: RunId, reused_check_run: Option<(JobId, u64)>, ) { - let (repository, sha, jobs) = { - // The rerun reports checks even when every job is an expandable - // placeholder — those mint nothing here, but their materialized legs - // report later and need the flag. - if let Err(error) = shared - .state - .backend - .set_reports_check_runs(run_id, true) - .await - { - warn!(%run_id, ?error, "failed to stamp reports_check_runs for rerun"); - } - let outcome = shared + if let Err(error) = shared + .state + .backend + .set_reports_check_runs(run_id, true) + .await + { + warn!(%run_id, ?error, "failed to stamp reports_check_runs for rerun"); + return; + } + if let Some((job_id, check_run_id)) = reused_check_run + && let Err(error) = shared .state .backend - .run_dispatch_info(run_id) + .set_job_check_run(run_id, &job_id, check_run_id) .await - .map_err(crate::ApiError::from) - .ok() - .flatten(); - match outcome { - Some(info) => ( - info.repository, - info.sha, - // Expandable nodes (deferred matrices, reusable callers) are - // placeholders: expansion replaces them, and their - // materialized legs mint their own checks. A `queued` check - // minted here would strand on GitHub (no delete API). - info.jobs - .into_iter() - .filter(|job| !job.placeholder) - .map(|job| (job.job_id, job.status)) - .collect::>(), - ), - None => return, - } - }; - - for (job_id, status) in jobs { - if let Some((reused_job_id, check_run_id)) = &reused_check_run { - if reused_job_id == &job_id { - if let Err(error) = report_existing_check_run_queued( - shared, - &repository, - &job_id, - run_id, - *check_run_id, - ) + { + warn!(%run_id, %job_id, ?error, "failed to persist reused check-run id"); + } + if let Ok(Some(info)) = shared.state.backend.run_dispatch_info(run_id).await { + for job in info.jobs.into_iter().filter(|job| !job.placeholder) { + if let Err(error) = shared + .state + .backend + .append_check_run_projection(run_id, Some(&job.job_id)) .await - { - warn!(%run_id, %job_id, ?error, "failed to requeue GitHub check run"); - } - } else if let Err(error) = - report_check_run_queued(shared, &repository, &sha, &job_id, run_id).await { - warn!(%run_id, %job_id, ?error, "failed to report queued GitHub check run"); + warn!(%run_id, job_id=%job.job_id, ?error, "failed to append rerun check projection wake"); } - } else if let Err(error) = - report_check_run_queued(shared, &repository, &sha, &job_id, run_id).await - { - warn!(%run_id, %job_id, ?error, "failed to report queued GitHub check run"); - } - - if status.is_terminal() { - report_check_run_completed(shared, run_id, &job_id, status).await; } + shared.state.events_dirty.notify_one(); } } /// Report check run status to in_progress on GitHub or simulate it locally. +/// The acquire transition writes `job.started.v1` in its own transaction; the +/// durable projector observes it and the sender performs the PATCH +/// asynchronously. This wake covers a projector that consumed that event +/// before the run's `reports_check_runs` flag landed. pub async fn report_check_run_in_progress( shared: &Arc, run_id: RunId, job_id: &JobId, ) { - let (repo, check_run_id, job_name) = { - let backend = &shared.state.backend; - let Some(check_run_id) = backend - .job_check_run_id(run_id, job_id) - .await - .ok() - .flatten() - else { - return; - }; - let repo = backend - .submission_fields(run_id) - .await - .ok() - .flatten() - .map(|fields| fields.repository); - let Some(repo) = repo else { - return; - }; - let job_name = backend - .job_display_name(run_id, job_id) - .await - .ok() - .flatten() - .unwrap_or_else(|| job_id.0.clone()); - (repo, check_run_id, job_name) - }; - - let token = resolve_check_run_token(shared, &repo).await; - if let Some(token) = &token { - let details_url = run_details_url(run_id); - - let started_at = chrono::Utc::now().to_rfc3339(); - let mut body = serde_json::json!({ - "status": "in_progress", - "started_at": started_at, - "output": { - "title": job_name, - "summary": "Running in preloop." - } - }); - if let Some(url) = details_url { - body["details_url"] = serde_json::json!(url); - } - - let path = format!("check-runs/{}", check_run_id); - if let Err(e) = send_github_check_request( - shared, - &shared.state.github_lifecycle_breaker, - token, - &repo, - reqwest::Method::PATCH, - &path, - &body, - ) + if let Err(error) = shared + .state + .backend + .append_check_run_projection(run_id, Some(job_id)) .await - { - warn!( - %run_id, - %job_id, - check_run_id, - error = %e, - "Failed to update GitHub check run to in_progress" - ); - } + { + warn!(%run_id, %job_id, ?error, "failed to append in-progress check projection wake"); } else { - info!(%run_id, %job_id, check_run_id, "Mock updated check run to in_progress"); + shared.state.events_dirty.notify_one(); } } @@ -851,231 +1207,50 @@ fn check_summary( for step in steps { summary.push_str(&format!( "\n| {} | {} | {} |", - markdown_cell(&step.name), - markdown_cell(&step.conclusion), - duration_text(step.started_at, step.finished_at) - )); - } - if let Some(failed) = steps.iter().find(|step| step.conclusion == "failure") { - summary.push_str(&format!( - "\n\n**Failed step:** `{}`", - markdown_cell(&failed.name) - )); - } - } - if !global_issues.is_empty() { - summary.push_str("\n\n### Failure details\n"); - summary.push_str(&global_issues.join("\n")); - } - // Map the display name back to the workflow job id: required status - // checks match on this exact display string, so a rename that breaks - // the ruleset is diagnosable from the check page itself instead of - // presenting as "Expected" forever with no explanation. - summary.push_str(&format!("\n\njob_id: `{}`", job_id.0)); - summary -} -/// Report check run status to completed on GitHub or simulate it locally. -pub async fn report_check_run_completed( - shared: &Arc, - run_id: RunId, - job_id: &JobId, - status: ExecutionStatus, -) { - // Backend: one indexed dispatch read. Node-local: `timeline_events` for - // annotations. Read each under its own owner. - let (repo, _check_run_id, job_name, steps, started_at, completed_at, detail) = { - let outcome = shared - .state - .backend - .run_dispatch_info(run_id) - .await - .map_err(crate::ApiError::from) - .ok() - .flatten(); - let Some(info) = outcome else { - return; - }; - // A job minted on demand (deferred-matrix leg, reusable callee) may - // have no dispatch row at all — proceed with run-level defaults so - // `ensure_check_run_mapped` below can mint for it. - let job = info.jobs.iter().find(|job| job.job_id == *job_id); - let check_run_id = job.and_then(|job| job.check_run_id); - // `project_run`'s per-job projection, minus the pieces it derived - // from a second pass over the whole run: name, status conclusion and - // the latest attempt's step manifest. - let mut detail = - job.and_then(|job| job.detail.clone()) - .unwrap_or(crate::models::JobDetail { - job_id: job_id.0.clone(), - name: job - .and_then(|job| job.display_name.clone()) - .unwrap_or_else(|| job_id.0.clone()), - conclusion: crate::runtime_scheduling::status_string( - job.map(|job| job.status) - .unwrap_or(ExecutionStatus::Pending), - ), - steps: Vec::new(), - annotations: Vec::new(), - }); - detail.job_id = job_id.0.clone(); - detail.name = job - .and_then(|job| job.display_name.clone()) - .unwrap_or_else(|| job_id.0.clone()); - detail.conclusion = crate::runtime_scheduling::status_string( - job.map(|job| job.status) - .unwrap_or(ExecutionStatus::Pending), - ); - let job_steps: Vec = - job.map(|job| job.steps.clone()).unwrap_or_default(); - if !job_steps.is_empty() { - detail.steps = job_steps.clone(); - } - let job_name = detail.name.clone(); - let started_at = job_steps - .iter() - .filter_map(|step| step.started_at) - .min() - .or_else(|| info.started_at.map(chrono::DateTime::from)); - let completed_at = job_steps - .iter() - .filter_map(|step| step.finished_at) - .max() - .or_else(|| info.completed_at.map(chrono::DateTime::from)) - .unwrap_or_else(chrono::Utc::now); - ( - info.repository, - check_run_id, - job_name, - job_steps, - started_at, - completed_at, - Some(detail), - ) - }; - - let (annotations, global_issues) = { - let inner = shared.state.inner.lock().await; - let mut annotations = Vec::new(); - let mut global_issues = Vec::new(); - if let Some(events) = inner.timeline_events.get(&run_id) { - for event in events { - if let NdjsonEvent::Annotation { - job_id: event_job_id, - level, - message, - file, - line, - .. - } = event - && event_job_id == job_id - { - let level_str = match level { - AnnotationLevel::Notice => "notice", - AnnotationLevel::Warning => "warning", - AnnotationLevel::Error => "failure", - }; - if let Some(file_path) = file { - let line_num = line.unwrap_or(1); - annotations.push(serde_json::json!({ - "path": file_path, - "start_line": line_num, - "end_line": line_num, - "annotation_level": level_str, - "message": message, - })); - } else { - global_issues.push(format!( - "**{}**: {}", - level_str.to_uppercase(), - message - )); - } - } - } - } - if let Some(detail) = &detail { - for annotation in &detail.annotations { - let message = annotation - .get("message") - .and_then(Value::as_str) - .map(str::to_owned) - .unwrap_or_else(|| annotation.to_string()); - global_issues.push(format!("- {}", markdown_cell(&message))); - } - } - (annotations, global_issues) - }; - - // Jobs materialized after intake (runtime matrix fan-out, reusable callee - // jobs) have no check run yet; mint one instead of dropping the report. - // A cancelled expandable placeholder mints a `cancelled` check — GitHub - // has no delete API, so concluding it is better than leaving it queued. - let Some(check_run_id) = ensure_check_run_mapped(shared, run_id, job_id).await else { - return; - }; - - let conclusion = match status { - ExecutionStatus::Success => "success", - ExecutionStatus::Failure => "failure", - ExecutionStatus::Cancelled => "cancelled", - ExecutionStatus::Skipped => "skipped", - _ => "failure", - }; - let summary = check_summary(conclusion, &steps, &global_issues, job_id); - - let token = resolve_check_run_token(shared, &repo).await; - if let Some(token) = &token { - let path = format!("check-runs/{check_run_id}"); - let chunks: Vec<&[Value]> = if annotations.is_empty() { - vec![&[]] - } else { - annotations.chunks(50).collect() - }; - for (index, chunk) in chunks.iter().enumerate() { - let last = index + 1 == chunks.len(); - let mut body = serde_json::json!({ - "output": { - "title": job_name, - "summary": summary, - "annotations": chunk, - } - }); - if last { - body["status"] = serde_json::json!("completed"); - body["conclusion"] = serde_json::json!(conclusion); - body["completed_at"] = serde_json::json!(completed_at.to_rfc3339()); - if let Some(started_at) = started_at { - body["started_at"] = serde_json::json!(started_at.to_rfc3339()); - } - if let Some(url) = run_details_url(run_id) { - body["details_url"] = serde_json::json!(url); - } - } - if let Err(error) = - send_github_check_completion(shared, token, &repo, &path, &body).await - { - warn!( - %run_id, - %job_id, - check_run_id, - annotation_batch = index + 1, - annotation_batches = chunks.len(), - %error, - "Failed to update GitHub check run" - ); - return; - } + markdown_cell(&step.name), + markdown_cell(&step.conclusion), + duration_text(step.started_at, step.finished_at) + )); + } + if let Some(failed) = steps.iter().find(|step| step.conclusion == "failure") { + summary.push_str(&format!( + "\n\n**Failed step:** `{}`", + markdown_cell(&failed.name) + )); } + } + if !global_issues.is_empty() { + summary.push_str("\n\n### Failure details\n"); + summary.push_str(&global_issues.join("\n")); + } + // Map the display name back to the workflow job id: required status + // checks match on this exact display string, so a rename that breaks + // the ruleset is diagnosable from the check page itself instead of + // presenting as "Expected" forever with no explanation. + summary.push_str(&format!("\n\njob_id: `{}`", job_id.0)); + summary +} +/// Report check run status to completed on GitHub or simulate it locally. +/// +/// The terminal transition already wrote a durable event; this wake covers a +/// projector that consumed it before the run's `reports_check_runs` flag +/// landed (a job concluded during submit) — the sender renders the row's +/// final state. +pub async fn report_check_run_completed( + shared: &Arc, + run_id: RunId, + job_id: &JobId, + _status: ExecutionStatus, +) { + if let Err(error) = shared + .state + .backend + .append_check_run_projection(run_id, Some(job_id)) + .await + { + warn!(%run_id, %job_id, ?error, "failed to append completed check projection wake"); } else { - info!( - %run_id, - %job_id, - check_run_id, - conclusion, - annotations_count = annotations.len(), - global_issues_count = global_issues.len(), - "Mock updated check run to completed" - ); + shared.state.events_dirty.notify_one(); } } @@ -3873,6 +4048,9 @@ jobs: #[tokio::test] async fn malformed_pull_request_workflow_reports_head_sha() { + // See `malformed_pull_request_workflow_failure_is_deduplicated`: the + // PR-files lookup must not hit another test's GitHub stub. + let _env = crate::state::GITHUB_ENV_LOCK.lock().await; let temp = tempfile::tempdir().unwrap(); let ws_dir = temp.path().join("ws"); std::fs::create_dir_all(ws_dir.join(".github/workflows")).unwrap(); @@ -3929,6 +4107,10 @@ jobs: #[tokio::test] async fn malformed_pull_request_workflow_failure_is_deduplicated() { + // `PRELOOP_GITHUB_API_URL` is process-global; this scenario expects no + // stub to answer the PR-files lookup, so it must not race a test that + // installs one. + let _env = crate::state::GITHUB_ENV_LOCK.lock().await; let temp = tempfile::tempdir().unwrap(); let ws_dir = temp.path().join("ws"); std::fs::create_dir_all(ws_dir.join(".github/workflows")).unwrap(); @@ -4667,20 +4849,40 @@ jobs: #[tokio::test] async fn materialized_job_mints_check_run_when_intake_reported() { let (_temp, shared, run_id) = mint_fixture(true, None).await; + let job = JobId("build".to_owned()); - // A leg materialized by a deferred matrix has a check run minted - // on demand — no GitHub token resolves in tests, so the mock id is - // fine; the assertion is that the mapping exists at all. - let leg = JobId("build (linux)".to_owned()); - let id = ensure_check_run_mapped(&shared, run_id, &leg).await; - assert!(id.is_some(), "reported runs mint checks for late jobs"); + // Handlers never mint inline any more: there is no GitHub call and no + // persisted id before the sender runs. + assert_eq!( + ensure_check_run_mapped(&shared, run_id, &job).await, + None, + "a request handler must not mint a check run" + ); + + // The durable queued report stamps the run and appends a projection + // wake; the projector materializes the desired row, and the sender + // (exercised by the sender tests) creates the GitHub check from it. + report_check_run_queued( + &shared, + "owner/repo", + "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + &job, + run_id, + ) + .await + .unwrap(); + let row = wait_for_check_run_row(&shared, run_id, &job).await; + assert_eq!(row.payload["status"], "queued"); + assert_eq!(row.payload["name"], "build"); + assert_eq!(row.payload["repository"], "owner/repo"); + assert_eq!( + row.payload["sha"], "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + "the projected row carries the run's check sha" + ); let runs = shared.state.test_tx().await.runs; assert!( - runs.get(&run_id) - .unwrap() - .job_check_run_ids - .contains_key(&leg), - "the minted id must be recorded for lifecycle PATCHes" + runs.get(&run_id).unwrap().reports_check_runs, + "the queued report must stamp the run so later jobs report too" ); } @@ -4701,20 +4903,587 @@ jobs: #[tokio::test] async fn terminal_report_mints_missing_check_run_for_late_job() { let (_temp, shared, run_id) = mint_fixture(true, None).await; - let leg = JobId("build (linux)".to_owned()); + let job = JobId("build".to_owned()); + + // A matrix leg that skips at promotion never dispatches; the only + // report it ever gets is the completion one. It must reach the queue + // even though the job settled before the reporting flag landed (a + // `JobStatus` event would be stamped stale; the projection wake is + // not). + shared + .state + .test_db_mutate(|db| { + db.execute( + "UPDATE jobs SET status = 'skipped' WHERE run_id = ?1 AND job_id = ?2", + rusqlite::params![run_id.to_string(), "build"], + ) + }) + .await + .unwrap(); + report_check_run_completed(&shared, run_id, &job, ExecutionStatus::Skipped).await; - // A matrix leg that skips at promotion never dispatches, so the only - // report it ever gets is the completion one — which must mint the - // check rather than drop the report. - report_check_run_completed(&shared, run_id, &leg, ExecutionStatus::Skipped).await; + let row = wait_for_check_run_row(&shared, run_id, &job).await; + assert_eq!( + row.payload["status"], "skipped", + "a terminal report must queue its final state for the sender" + ); + } - let runs = shared.state.test_tx().await.runs; + /// Wait for the durable projector (which owns the consumer lease in the + /// background) to materialize a queue row for `run_id`/`job_id`. + async fn wait_for_check_run_row( + shared: &std::sync::Arc, + run_id: RunId, + job_id: &JobId, + ) -> crate::control::types::CheckRunUpdate { + for _ in 0..120 { + if let Ok(rows) = shared + .state + .backend + .lease_check_run_updates("test-probe", std::time::Duration::from_millis(1), 100) + .await + && let Some(row) = rows + .into_iter() + .find(|row| row.run_id == run_id && row.job_id == *job_id) + { + return row; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + panic!("check-run row for {run_id}/{job_id} was never projected"); + } + + // ── Sender acceptance tests ──────────────────────────────────────── + + type StubRequests = std::sync::Arc>>; + + /// Fake GitHub API: records every request and answers from + /// `handler(method, path, body)`. Returns the base URL to install as + /// `PRELOOP_GITHUB_API_URL`. + async fn start_github_stub( + handler: impl Fn(&str, &str, &Value) -> (u16, Vec<(&'static str, String)>, Value) + + Send + + Sync + + 'static, + ) -> (String, StubRequests) { + let requests: StubRequests = std::sync::Arc::new(parking_lot::Mutex::new(Vec::new())); + let recorded = requests.clone(); + let handler = std::sync::Arc::new(handler); + let app = axum::Router::new().fallback(move |request: axum::extract::Request| { + let recorded = recorded.clone(); + let handler = handler.clone(); + async move { + let method = request.method().to_string(); + let path = request.uri().path().to_owned(); + let bytes = axum::body::to_bytes(request.into_body(), 1 << 20) + .await + .unwrap_or_default(); + let body: Value = serde_json::from_slice(&bytes).unwrap_or(Value::Null); + recorded + .lock() + .push((method.clone(), path.clone(), body.clone())); + let (status, headers, response_body) = handler(&method, &path, &body); + let mut response = axum::response::Response::new(axum::body::Body::from( + response_body.to_string(), + )); + *response.status_mut() = axum::http::StatusCode::from_u16(status).unwrap(); + for (name, value) in headers { + response.headers_mut().insert( + axum::http::HeaderName::from_bytes(name.as_bytes()).unwrap(), + axum::http::HeaderValue::from_str(&value).unwrap(), + ); + } + response + } + }); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + (format!("http://{addr}"), requests) + } + + fn check_run_writes(requests: &StubRequests) -> Vec<(String, String, Value)> { + requests + .lock() + .iter() + .filter(|(method, _, _)| method == "POST" || method == "PATCH") + .cloned() + .collect() + } + + async fn seed_check_run_update( + shared: &std::sync::Arc, + run_id: RunId, + job_id: &JobId, + version: i64, + status: &str, + check_run_id: Option, + ) { + shared + .state + .backend + .enqueue_check_run_update(crate::control::types::CheckRunUpdateInput { + run_id, + job_id: job_id.clone(), + installation_id: 0, + check_run_id, + version, + payload: serde_json::json!({ + "repository": "owner/repo", + "sha": "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + "job_id": job_id.0, + "status": status, + "name": "build", + }), + }) + .await + .unwrap(); + } + + async fn drain_sender(shared: &std::sync::Arc) -> usize { + CheckRunSender::new().drain_once(shared).await + } + + /// (b) queued → in_progress → completed committed before the sender runs + /// coalesces to ONE Checks API write carrying the final state. + #[tokio::test] + async fn sender_coalesces_versions_into_one_final_request() { + let _env = crate::state::GITHUB_ENV_LOCK.lock().await; + let (api, requests) = start_github_stub(|method, _path, _body| match method { + "GET" => (200, vec![], serde_json::json!({"check_runs": []})), + _ => (201, vec![], serde_json::json!({"id": 4242})), + }) + .await; + let _api = crate::state::TestEnvVar::set("PRELOOP_GITHUB_API_URL", &api); + let _token = crate::state::TestEnvVar::set("PRELOOP_GITHUB_TOKEN", "test-token"); + let (_temp, shared, run_id) = mint_fixture(true, None).await; + let job = JobId("build".to_owned()); + for (version, status) in [(1, "queued"), (2, "in_progress"), (3, "success")] { + seed_check_run_update(&shared, run_id, &job, version, status, None).await; + } + + assert_eq!(drain_sender(&shared).await, 1); + let writes = check_run_writes(&requests); + assert_eq!(writes.len(), 1, "coalesced to one write: {writes:?}"); + assert_eq!(writes[0].0, "POST"); + assert_eq!(writes[0].2["status"], "completed"); + assert_eq!(writes[0].2["conclusion"], "success"); + assert!( + shared + .state + .backend + .lease_check_run_updates("probe", std::time::Duration::from_millis(1), 10) + .await + .unwrap() + .is_empty(), + "the sent row is deleted" + ); + } + + /// (d) a 429 with Retry-After parks the row for the advertised delay and + /// the next attempt succeeds. + #[tokio::test] + async fn sender_retries_after_retry_after_then_succeeds() { + let _env = crate::state::GITHUB_ENV_LOCK.lock().await; + let attempts = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let attempts_for_stub = attempts.clone(); + let (api, requests) = start_github_stub(move |method, _path, _body| { + if method == "GET" { + return (200, vec![], serde_json::json!({"check_runs": []})); + } + if attempts_for_stub.fetch_add(1, std::sync::atomic::Ordering::SeqCst) == 0 { + ( + 429, + vec![("retry-after", "1".to_owned())], + serde_json::json!({"message": "You have exceeded a secondary rate limit"}), + ) + } else { + (201, vec![], serde_json::json!({"id": 777})) + } + }) + .await; + let _api = crate::state::TestEnvVar::set("PRELOOP_GITHUB_API_URL", &api); + let _token = crate::state::TestEnvVar::set("PRELOOP_GITHUB_TOKEN", "test-token"); + let (_temp, shared, run_id) = mint_fixture(true, None).await; + let job = JobId("build".to_owned()); + seed_check_run_update(&shared, run_id, &job, 1, "queued", None).await; + + assert_eq!(drain_sender(&shared).await, 1); + assert_eq!( + check_run_writes(&requests).len(), + 1, + "first attempt is throttled" + ); + assert_eq!( + drain_sender(&shared).await, + 0, + "the row is not due until Retry-After elapses" + ); + assert_eq!(check_run_writes(&requests).len(), 1); + tokio::time::sleep(std::time::Duration::from_millis(1_150)).await; + assert_eq!(drain_sender(&shared).await, 1); + assert_eq!( + check_run_writes(&requests).len(), + 2, + "retried and succeeded" + ); + assert!( + shared + .state + .backend + .lease_check_run_updates("probe", std::time::Duration::from_millis(1), 10) + .await + .unwrap() + .is_empty(), + "the successful retry deletes the row" + ); + } + + /// (e) crash after POST before the id was saved: the next attempt finds + /// the existing check run (or the persisted job mapping) and PATCHes it; + /// it never POSTs a duplicate. + #[tokio::test] + async fn sender_reconciles_instead_of_second_post() { + let _env = crate::state::GITHUB_ENV_LOCK.lock().await; + let (api, requests) = start_github_stub(|method, path, _body| { + if method == "GET" { + let existing = if path.contains("commits/") { + serde_json::json!({"check_runs": [{ + "id": 7001, + "name": "build", + "head_sha": "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + }]}) + } else { + serde_json::json!({}) + }; + return (200, vec![], existing); + } + (200, vec![], serde_json::json!({"id": 7001})) + }) + .await; + let _api = crate::state::TestEnvVar::set("PRELOOP_GITHUB_API_URL", &api); + let _token = crate::state::TestEnvVar::set("PRELOOP_GITHUB_TOKEN", "test-token"); + let (_temp, shared, run_id) = mint_fixture(true, None).await; + let job = JobId("build".to_owned()); + + // The check run already exists on GitHub but the row has no id. + seed_check_run_update(&shared, run_id, &job, 1, "queued", None).await; + assert_eq!(drain_sender(&shared).await, 1); + let writes = check_run_writes(&requests); + assert_eq!(writes.len(), 1, "one write only: {writes:?}"); + assert_eq!(writes[0].0, "PATCH"); + assert!(writes[0].1.ends_with("/check-runs/7001")); + assert_eq!( + shared + .state + .backend + .job_check_run_id(run_id, &job) + .await + .unwrap(), + Some(7001), + "the reconciled id is persisted" + ); + + // A persisted job mapping alone (no GitHub lookup) also PATCHes. + let (_temp2, shared2, run_id2) = mint_fixture(true, None).await; + shared2 + .state + .backend + .set_job_check_run(run_id2, &job, 7002) + .await + .unwrap(); + seed_check_run_update(&shared2, run_id2, &job, 1, "queued", None).await; + let before = requests.lock().len(); + assert_eq!(drain_sender(&shared2).await, 1); + let writes: Vec<_> = requests + .lock() + .iter() + .skip(before) + .filter(|(method, _, _)| method == "POST" || method == "PATCH") + .cloned() + .collect(); + assert_eq!(writes.len(), 1); + assert_eq!(writes[0].0, "PATCH"); + assert!(writes[0].1.ends_with("/check-runs/7002")); + assert!( + !requests + .lock() + .iter() + .skip(before) + .any(|(method, _, _)| method == "POST"), + "no duplicate POST" + ); + } + + /// (h) a throttled installation parks only its own rows: the other + /// installation's check run succeeds in the same batch. + #[tokio::test] + async fn throttled_installation_does_not_block_another() { + let _env = crate::state::GITHUB_ENV_LOCK.lock().await; + let (api, requests) = start_github_stub(|method, path, _body| { + if path.contains("/access_tokens") { + let token = if path.contains("/101/") { + "ghs-a" + } else { + "ghs-b" + }; + return ( + 201, + vec![], + serde_json::json!({"token": token, "expires_at": "2099-01-01T00:00:00Z"}), + ); + } + if method == "GET" && path.starts_with("/app/installations/") { + let login = if path.ends_with("/101") { + "owner-a" + } else { + "owner-b" + }; + return ( + 200, + vec![], + serde_json::json!({"account": {"login": login}}), + ); + } + if method == "GET" { + return (200, vec![], serde_json::json!({"check_runs": []})); + } + if path.starts_with("/repos/owner-a/") { + let reset = (std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() + + 60) + .to_string(); + return ( + 403, + vec![ + ("x-ratelimit-remaining", "0".to_owned()), + ("x-ratelimit-reset", reset), + ], + serde_json::json!({"message": "API rate limit exceeded"}), + ); + } + (201, vec![], serde_json::json!({"id": 900})) + }) + .await; + let _api = crate::state::TestEnvVar::set("PRELOOP_GITHUB_API_URL", &api); + let _token = crate::state::TestEnvVar::set("PRELOOP_GITHUB_TOKEN", "test-token"); + + let key = rsa::RsaPrivateKey::new(&mut rand::thread_rng(), 2048).unwrap(); + let mut app_a = crate::github_app::GitHubAppCredentials::for_tests( + "424", + key.clone(), + crate::github_app::MintFailurePolicy::LocalJwt, + ); + app_a.installation_id = Some(101); + let mut app_b = crate::github_app::GitHubAppCredentials::for_tests( + "425", + key, + crate::github_app::MintFailurePolicy::LocalJwt, + ); + app_b.installation_id = Some(202); + let temp = tempfile::tempdir().unwrap(); + let mut state = AppState::new(temp.path().to_path_buf()).await.unwrap(); + state.github_apps = Some(crate::github_app::GitHubApps { + apps: vec![app_a.clone(), app_b], + default_index: 0, + }); + state.github_app = Some(app_a); + let shared = state.shared(); + let job = JobId("build".to_owned()); + let mut run_ids = Vec::new(); + for repo in ["owner-a/repo", "owner-b/repo"] { + let submission = WorkflowSubmission { + workflow_yaml: "on: push\njobs:\n build:\n runs-on: ubuntu-latest\n steps:\n - run: echo hello\n".to_owned(), + event: "push".to_owned(), + repository: repo.to_owned(), + workflow_path: Some(".github/workflows/ci.yml".to_owned()), + sha: "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2".to_owned(), + resolved_sha: Some("a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2".to_owned()), + ..Default::default() + }; + let accepted = crate::submit_run_inner(&shared, submission).await.unwrap(); + shared + .state + .backend + .set_reports_check_runs(accepted.run_id, true) + .await + .unwrap(); + let payload = serde_json::json!({ + "repository": repo, + "sha": "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + "job_id": "build", + "status": "queued", + "name": "build", + }); + shared + .state + .backend + .enqueue_check_run_update(crate::control::types::CheckRunUpdateInput { + run_id: accepted.run_id, + job_id: job.clone(), + installation_id: 0, + check_run_id: None, + version: 1, + payload, + }) + .await + .unwrap(); + run_ids.push(accepted.run_id); + } + + assert_eq!(drain_sender(&shared).await, 2, "both rows are leased"); + let writes: Vec<_> = check_run_writes(&requests) + .into_iter() + .filter(|(_, path, _)| path.contains("/check-runs")) + .collect(); + assert_eq!( + writes.len(), + 2, + "one write per installation: all={:?}", + check_run_writes(&requests) + ); + let a_writes = writes + .iter() + .filter(|(_, path, _)| path.starts_with("/repos/owner-a/")) + .count(); + let b_writes = writes + .iter() + .filter(|(_, path, _)| path.starts_with("/repos/owner-b/")) + .count(); + assert_eq!((a_writes, b_writes), (1, 1)); + let writes_before = writes.len(); + assert_eq!( + drain_sender(&shared).await, + 0, + "the throttled installation's row is deferred, not retried immediately" + ); + assert_eq!( + check_run_writes(&requests) + .iter() + .filter(|(_, path, _)| path.contains("/check-runs")) + .count(), + writes_before + ); + // B's row is gone; A's is parked until the reset (not due). + assert!( + shared + .state + .backend + .lease_check_run_updates("probe", std::time::Duration::from_millis(1), 10) + .await + .unwrap() + .is_empty(), + "the successful installation's row is deleted and the throttled one is not due" + ); + } + + /// (j) a run that vanished before the sender ran still delivers its final + /// state from the payload; permanent errors drop the row. + #[tokio::test] + async fn sender_delivers_vanished_run_and_drops_permanent_failures() { + let _env = crate::state::GITHUB_ENV_LOCK.lock().await; + let statuses = std::sync::Arc::new(parking_lot::Mutex::new(Vec::::new())); + let statuses_for_stub = statuses.clone(); + let (api, requests) = start_github_stub(move |method, _path, _body| { + if method == "GET" { + return (200, vec![], serde_json::json!({"check_runs": []})); + } + let status = statuses_for_stub.lock().pop().unwrap_or(201); + (status, vec![], serde_json::json!({"id": 555})) + }) + .await; + let _api = crate::state::TestEnvVar::set("PRELOOP_GITHUB_API_URL", &api); + let _token = crate::state::TestEnvVar::set("PRELOOP_GITHUB_TOKEN", "test-token"); + let temp = tempfile::tempdir().unwrap(); + let state = AppState::new(temp.path().to_path_buf()).await.unwrap(); + let shared = state.shared(); + let job = JobId("build".to_owned()); + + // No `runs` row at all: an archived/deleted run, or the synthetic + // workflow-failure shape. + seed_check_run_update(&shared, RunId::new(), &job, 1, "success", None).await; + assert_eq!(drain_sender(&shared).await, 1); + let writes = check_run_writes(&requests); + assert_eq!(writes.len(), 1); + assert_eq!(writes[0].2["status"], "completed"); + assert_eq!(writes[0].2["conclusion"], "success"); + + // A permanent 422 drops the row after one attempt. + statuses.lock().push(422); + seed_check_run_update(&shared, RunId::new(), &job, 1, "queued", None).await; + assert_eq!(drain_sender(&shared).await, 1); assert!( - runs.get(&run_id) + shared + .state + .backend + .lease_check_run_updates("probe", std::time::Duration::from_millis(1), 10) + .await .unwrap() - .job_check_run_ids - .contains_key(&leg), - "a terminal report must mint the check it reports to" + .is_empty(), + "a 422 must drop the row instead of retrying forever" + ); + } + + /// (a) a webhook push fanning out to many jobs returns without any inline + /// GitHub Checks API call; the durable queue carries the reports instead. + #[tokio::test] + async fn webhook_fanout_makes_zero_inline_check_calls() { + let _env = crate::state::GITHUB_ENV_LOCK.lock().await; + let (api, requests) = + start_github_stub(|_method, _path, _body| (200, vec![], serde_json::json!({}))).await; + let _api = crate::state::TestEnvVar::set("PRELOOP_GITHUB_API_URL", &api); + let _token = crate::state::TestEnvVar::set("PRELOOP_GITHUB_TOKEN", "test-token"); + + let temp = tempfile::tempdir().unwrap(); + let ws_dir = temp.path().join("ws"); + std::fs::create_dir_all(ws_dir.join(".github/workflows")).unwrap(); + let mut workflow = "on: push\njobs:\n".to_owned(); + for index in 0..8 { + workflow.push_str(&format!( + " job{index}:\n runs-on: ubuntu-latest\n steps:\n - run: echo {index}\n" + )); + } + std::fs::write(ws_dir.join(".github/workflows/build.yml"), workflow).unwrap(); + let fixture = WebhookFixture::with_workspace(&temp, ws_dir).await; + + assert_eq!( + fixture.post("delivery-check-fanout", Some("push")).await, + StatusCode::ACCEPTED + ); + assert!( + requests + .lock() + .iter() + .all(|(_, path, _)| !path.contains("check-runs")), + "the webhook handler must not call the Checks API inline: {:?}", + requests.lock() + ); + fixture.drain().await; + let record = fixture + .state + .backend + .get_webhook_delivery("delivery-check-fanout") + .await + .unwrap() + .expect("delivery row"); + assert_eq!(record.state, WebhookDeliveryStatus::Done); + let inner = fixture.state.test_tx().await; + assert_eq!(inner.runs.len(), 1); + assert_eq!( + inner.runs.values().next().unwrap().jobs.len(), + 8, + "all eight jobs fan out" + ); + assert!( + requests + .lock() + .iter() + .all(|(_, path, _)| !path.contains("check-runs")), + "delivery must not call the Checks API inline either" ); } diff --git a/crates/preloop-runner-server/src/github_app.rs b/crates/preloop-runner-server/src/github_app.rs index 25030406e..b03f364ca 100644 --- a/crates/preloop-runner-server/src/github_app.rs +++ b/crates/preloop-runner-server/src/github_app.rs @@ -731,6 +731,21 @@ pub async fn select_app_for_repo( .next() } +/// The installation id `app` mints tokens for on `repository`'s owner. +/// +/// Exposed so the check-run sender can key per-installation rate budgets on +/// the *real* installation rather than whatever id a queue row happened to +/// carry: `0` is never a valid GitHub installation, and discovery is cached +/// per App+owner, so steady state costs no extra API call. +pub async fn installation_for_repo( + app: &GitHubAppCredentials, + repository: &str, +) -> anyhow::Result { + let (owner, _) = split_repository(repository)?; + let app_jwt = sign_app_jwt(&app.app_id, &app.private_key)?; + installation_id_for(&api_base(), app, &app_jwt, owner).await +} + /// Mint an installation token for `repository`, scoped to `permissions`. /// /// `repository` is an `owner/repo` slug: the owner selects the installation and diff --git a/crates/preloop-runner-server/src/github_breaker.rs b/crates/preloop-runner-server/src/github_breaker.rs index 2468d52e8..df224c4b6 100644 --- a/crates/preloop-runner-server/src/github_breaker.rs +++ b/crates/preloop-runner-server/src/github_breaker.rs @@ -250,6 +250,34 @@ impl GithubBreaker { } } + /// Honour a *primary* rate limit advertised on an otherwise successful + /// response. + /// + /// GitHub sends `x-ratelimit-remaining: 0` plus `x-ratelimit-reset` on the + /// last allowed response. Recording the advertised window here means the + /// caller parks itself until the reset instead of spending the next + /// request on a guaranteed 403. A response without rate headers, or one + /// that still has budget, changes nothing. + pub fn observe_rate_budget(&self, headers: &HeaderMap) { + let remaining = header_str(headers, "x-ratelimit-remaining") + .and_then(|value| value.trim().parse::().ok()); + if !remaining.is_some_and(|remaining| remaining <= 0) { + return; + } + let Some(wait) = retry_after_from_headers(headers) else { + return; + }; + let now = Instant::now(); + let mut inner = self.inner.lock(); + inner.rate_limited = true; + inner.last_error = Some("GitHub primary rate limit exhausted".to_owned()); + Self::open_for( + &mut inner, + now, + wait.clamp(Duration::from_secs(1), Duration::from_secs(3600)), + ); + } + /// Observe a transport-level failure (DNS, TLS, connect, timeout). pub fn observe_transport_error(&self, error: &reqwest::Error) -> GithubFailureKind { let kind = GithubFailureKind::Unavailable; diff --git a/crates/preloop-runner-server/src/lib.rs b/crates/preloop-runner-server/src/lib.rs index a5bdd6cd8..f196164bd 100644 --- a/crates/preloop-runner-server/src/lib.rs +++ b/crates/preloop-runner-server/src/lib.rs @@ -8,6 +8,7 @@ use std::net::SocketAddr; use std::path::PathBuf; use std::sync::Arc; +mod check_run_outbox; pub mod concurrency; pub mod config; pub mod credential_store; diff --git a/crates/preloop-runner-server/tests/broker.rs b/crates/preloop-runner-server/tests/broker.rs index 3c4b930c4..c0fef9dc4 100644 --- a/crates/preloop-runner-server/tests/broker.rs +++ b/crates/preloop-runner-server/tests/broker.rs @@ -263,6 +263,7 @@ jobs: shutdown: CancellationToken::new(), }); crate::github::drain_webhook_queue(&shared).await.unwrap(); + crate::github::drain_check_run_sender(&shared).await; let inner = state.test_tx().await; assert_eq!(inner.runs.len(), 1); // Queue depth is sampled by the 5s sampler, not updated per operation; @@ -668,6 +669,7 @@ jobs: shutdown: CancellationToken::new(), }); crate::github::drain_webhook_queue(&shared).await.unwrap(); + crate::github::drain_check_run_sender(&shared).await; let inner = state.test_tx().await; let (run_id, run) = inner.runs.iter().next().expect("webhook created a run"); @@ -1182,6 +1184,7 @@ jobs: shutdown: CancellationToken::new(), }); crate::github::drain_webhook_queue(&shared).await.unwrap(); + crate::github::drain_check_run_sender(&shared).await; // Verify triggered run let inner = state.test_tx().await; assert_eq!(inner.runs.len(), 1); diff --git a/docs/architecture.md b/docs/architecture.md index 1e1a724bf..d0337ca69 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -89,6 +89,37 @@ one node per job). Known gaps and their tradeoffs are tracked in the repository issue tracker. +## Transactional event flow + +Every state transition writes its versioned event to `outbox_events` in the +same database transaction as the run/job mutation. The durable `check-runs` +consumer holds a persisted `consumer_offsets` lease, reads from its bookmark +(`(txid,event_id)` on Postgres and `event_id` on SQLite), and atomically +projects the newest desired state for each `(run_id,job_id)` into +`check_run_updates`. Older versions are ignored. + +```text +run/job transaction -> outbox_events + | + check-runs projector + v + check_run_updates (coalesced) + | + one leased sender + v + GitHub Checks API +``` + +The sender is the only check-run HTTP writer. It leases due rows, renders +the existing check name/summary/details/output at send time, chunks +annotations at GitHub's 50-item limit, and owns token caching, circuit +breaking, rate-limit handling, retries and exponential backoff. A webhook +therefore commits durable state and returns without waiting for GitHub. +Postgres `NOTIFY preloop_events`, the local dirty signal, and a short polling +fallback wake the projector. Outbox pruning is bounded by age only when no +durable consumer exists; otherwise rows at or beyond the slowest consumer +bookmark are retained. + ## Secrets Secrets use `SecretString` in `preloop-gha-protocol`. It redacts `Debug`, diff --git a/docs/control-schema.sql b/docs/control-schema.sql index ef5160bf9..ef400c3c5 100644 --- a/docs/control-schema.sql +++ b/docs/control-schema.sql @@ -657,12 +657,13 @@ CREATE TABLE check_run_updates ( job_id text NOT NULL, installation_id bigint NOT NULL, check_run_id bigint, + version bigint NOT NULL DEFAULT 0, payload jsonb NOT NULL, not_before timestamptz NOT NULL DEFAULT now(), attempts integer NOT NULL DEFAULT 0, leased_until timestamptz, - PRIMARY KEY (run_id, job_id), - FOREIGN KEY (run_id, job_id) REFERENCES jobs(run_id, job_id) ON DELETE CASCADE + lease_owner text, + PRIMARY KEY (run_id, job_id) ); CREATE INDEX check_run_updates_queue ON check_run_updates(installation_id, not_before); From 9bdc5d3473b4069d74e10aa6288ac5209fdc4f07 Mon Sep 17 00:00:00 2001 From: Bill Njoroge Date: Tue, 6 Oct 2026 22:20:37 -0400 Subject: [PATCH 2/4] fix(github): harden the check-run sender against duplicates and stuck checks - A PATCH 404 right after creation is GitHub replicating the new check, not a deleted one. The sender cleared the id on the first 404 and POSTed a second check, stranding the original as queued (the bug 7a232785 fixed on main). The id now survives three backoff retries; only a persistent 404 drops it. - Renew the row's lease before every Checks API write and skip the row if another sender re-leased it, so an expired lease cannot lead two senders to POST the same check. Adds renew_check_run_update to both backends. - Stamp external_id = "{run_id}:{job_id}" on creation (workflow failures use their stable job id) and reconcile on it, paging all check runs with filter=all. Name + head_sha matching adopted other workflows' and other apps' same-named checks. - Wait for x-ratelimit-reset only when x-ratelimit-remaining is 0. GitHub sends the reset on every response, so a 404 or 5xx waited out the rate window (up to an hour) instead of the 250 ms backoff. - Drop the eager reconciliation GET after every successful POST, and the dead send_github_check_completion / github_lifecycle_breaker. - CHANGELOG: document the SQLite v4 / Postgres v5 schema bump and the direct-connection (no transaction pooler) requirement. --- CHANGELOG.md | 10 +- .../src/control/backend.rs | 48 +++ .../src/control/lite/check_runs.rs | 27 ++ .../src/control/lite/impls.rs | 10 + .../src/control/pg/check_runs.rs | 27 ++ .../src/control/pg/trait_impl.rs | 10 + crates/preloop-runner-server/src/github.rs | 381 ++++++++++++------ crates/preloop-runner-server/src/state.rs | 9 +- 8 files changed, 395 insertions(+), 127 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d1734d03..527ccb97f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -163,7 +163,15 @@ Releases before v0.27.0 predate the changelog. - Check-run updates now use a durable, coalescing transactional-outbox projector and one leased background sender. GitHub API retries, rate-limit backoff, stale-id reconciliation and restart recovery happen outside - webhook/request paths on both SQLite and PostgreSQL. + webhook/request paths on both SQLite and PostgreSQL. Each check run carries + an `external_id` of `{run_id}:{job_id}`, and crash-after-POST + reconciliation adopts only a check with that id — never another workflow's + or app's same-named check. The control schema moves to **SQLite v4 / + Postgres v5** in place: an existing control database at the old version is + refused and must be recreated. The Postgres backend must **connect + directly, not through a transaction pooler** (e.g. PgBouncer in transaction + mode): it relies on per-connection `search_path` and on a dedicated + `LISTEN` connection, which transaction pooling breaks. ### Changed diff --git a/crates/preloop-runner-server/src/control/backend.rs b/crates/preloop-runner-server/src/control/backend.rs index 4f7120b4c..46d4c812d 100644 --- a/crates/preloop-runner-server/src/control/backend.rs +++ b/crates/preloop-runner-server/src/control/backend.rs @@ -563,6 +563,17 @@ pub(crate) trait ControlBackend: Send + Sync { delay: std::time::Duration, ) -> Result<(), ControlError>; + /// Extend this sender's lease on one row to `now + lease_for`. `false` + /// when the row is no longer leased by `owner` (another sender took it + /// over, or it was settled): the caller must not call GitHub for it. + async fn renew_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + lease_for: std::time::Duration, + ) -> Result; + /// Append a durable projection wake after a reporter stamped /// `reports_check_runs`. async fn append_check_run_projection( @@ -1531,6 +1542,25 @@ impl Backend { } } + pub(crate) async fn renew_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + lease_for: std::time::Duration, + ) -> Result { + match self { + Self::Sqlite(b) => { + b.renew_check_run_update(owner, run_id, job_id, lease_for) + .await + } + Self::Postgres(b) => { + b.renew_check_run_update(owner, run_id, job_id, lease_for) + .await + } + } + } + /// Append a durable projection wake after a reporter stamped /// `reports_check_runs`; see `append_check_run_projection` on the /// backends. @@ -2156,6 +2186,24 @@ impl ControlBackend for Backend { Self::Postgres(b) => b.defer_check_run_update(owner, run_id, job_id, delay).await, } } + async fn renew_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + lease_for: std::time::Duration, + ) -> Result { + match self { + Self::Sqlite(b) => { + b.renew_check_run_update(owner, run_id, job_id, lease_for) + .await + } + Self::Postgres(b) => { + b.renew_check_run_update(owner, run_id, job_id, lease_for) + .await + } + } + } async fn append_check_run_projection( &self, run_id: RunId, diff --git a/crates/preloop-runner-server/src/control/lite/check_runs.rs b/crates/preloop-runner-server/src/control/lite/check_runs.rs index 32bab81d4..bada78bef 100644 --- a/crates/preloop-runner-server/src/control/lite/check_runs.rs +++ b/crates/preloop-runner-server/src/control/lite/check_runs.rs @@ -126,6 +126,33 @@ impl LiteBackend { self.write(|tx|{tx.execute("UPDATE check_run_updates SET not_before=?,leased_until=NULL,lease_owner=NULL WHERE lease_owner=? AND run_id=? AND job_id=?",params![now_us()+delay.as_micros() as i64,owner,run_text(run_id),job_id.0]).map_err(db)?;Ok(())}) } + /// Extend `owner`'s lease on one row. A row another sender re-leased has + /// a different `lease_owner`, so ownership alone decides: `false` means + /// the caller lost the row and must not call GitHub for it. + pub(crate) async fn renew_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + lease_for: std::time::Duration, + ) -> Result { + self.write(|tx| { + let renewed = tx + .execute( + "UPDATE check_run_updates SET leased_until=? \ + WHERE lease_owner=? AND run_id=? AND job_id=?", + params![ + now_us() + lease_for.as_micros() as i64, + owner, + run_text(run_id), + job_id.0 + ], + ) + .map_err(db)?; + Ok(renewed == 1) + }) + } + /// Append a durable projection wake for one run (or one job) *after* a /// reporter stamped `reports_check_runs`. /// diff --git a/crates/preloop-runner-server/src/control/lite/impls.rs b/crates/preloop-runner-server/src/control/lite/impls.rs index e3141e6d3..d1795b1c7 100644 --- a/crates/preloop-runner-server/src/control/lite/impls.rs +++ b/crates/preloop-runner-server/src/control/lite/impls.rs @@ -307,6 +307,16 @@ impl ControlBackend for LiteBackend { self.defer_check_run_update(owner, run_id, job_id, delay) .await } + async fn renew_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + lease_for: std::time::Duration, + ) -> Result { + self.renew_check_run_update(owner, run_id, job_id, lease_for) + .await + } async fn append_check_run_projection( &self, run_id: RunId, diff --git a/crates/preloop-runner-server/src/control/pg/check_runs.rs b/crates/preloop-runner-server/src/control/pg/check_runs.rs index 0abd61e33..af09e7953 100644 --- a/crates/preloop-runner-server/src/control/pg/check_runs.rs +++ b/crates/preloop-runner-server/src/control/pg/check_runs.rs @@ -169,6 +169,33 @@ impl PgBackend { client.execute("UPDATE check_run_updates SET not_before=now()+make_interval(secs=>$4),leased_until=NULL,lease_owner=NULL WHERE lease_owner=$1 AND run_id=$2::text::uuid AND job_id=$3", &[&owner,&run_text(run_id),&job_id.0,&delay.as_secs_f64()]).await.map(|_|()).map_err(db) } + /// Extend `owner`'s lease on one row. A row another sender re-leased has + /// a different `lease_owner`, so ownership alone decides: `false` means + /// the caller lost the row and must not call GitHub for it. + pub(crate) async fn renew_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + lease_for: std::time::Duration, + ) -> Result { + let client = self.writer().await?; + let renewed = client + .execute( + "UPDATE check_run_updates SET leased_until=now()+make_interval(secs=>$4) \ + WHERE lease_owner=$1 AND run_id=$2::text::uuid AND job_id=$3", + &[ + &owner, + &run_text(run_id), + &job_id.0, + &lease_for.as_secs_f64(), + ], + ) + .await + .map_err(db)?; + Ok(renewed == 1) + } + /// Append a durable projection wake for one run (or one job) *after* a /// reporter stamped `reports_check_runs`. /// diff --git a/crates/preloop-runner-server/src/control/pg/trait_impl.rs b/crates/preloop-runner-server/src/control/pg/trait_impl.rs index a8bed4307..792bb098d 100644 --- a/crates/preloop-runner-server/src/control/pg/trait_impl.rs +++ b/crates/preloop-runner-server/src/control/pg/trait_impl.rs @@ -309,6 +309,16 @@ impl ControlBackend for PgBackend { self.defer_check_run_update(owner, run_id, job_id, delay) .await } + async fn renew_check_run_update( + &self, + owner: &str, + run_id: RunId, + job_id: &JobId, + lease_for: std::time::Duration, + ) -> Result { + self.renew_check_run_update(owner, run_id, job_id, lease_for) + .await + } async fn append_check_run_projection( &self, run_id: RunId, diff --git a/crates/preloop-runner-server/src/github.rs b/crates/preloop-runner-server/src/github.rs index 3e234b002..cca612858 100644 --- a/crates/preloop-runner-server/src/github.rs +++ b/crates/preloop-runner-server/src/github.rs @@ -239,16 +239,22 @@ async fn send_github_check_request( .and_then(|v| v.to_str().ok()) .unwrap_or("") .to_owned(); - let reset = res - .headers() - .get("x-ratelimit-reset") - .and_then(|v| v.to_str().ok()) - .unwrap_or("") - .to_owned(); + let header = |name: &str| { + res.headers() + .get(name) + .and_then(|v| v.to_str().ok()) + .unwrap_or("") + .to_owned() + }; + // GitHub sends `x-ratelimit-reset` on every response, so the reset is + // only meaningful together with `remaining`: `check_retry_delay` + // waits for the reset only when the budget is actually spent. + let reset = header("x-ratelimit-reset"); + let remaining = header("x-ratelimit-remaining"); let err_text = res.text().await.unwrap_or_default(); record_check_reporting(shared, false); return Err(anyhow::anyhow!( - "GitHub Check API failed with status {}: {}; retry_after={retry_after}; rate_reset={reset}", + "GitHub Check API failed with status {}: {}; retry_after={retry_after}; rate_remaining={remaining}; rate_reset={reset}", status, err_text )); @@ -259,56 +265,6 @@ async fn send_github_check_request( Ok(val) } -/// GitHub can briefly return 404 when a PATCH races replication of a newly -/// created check run. Retry only that response; every other error is final. -async fn send_github_check_completion( - shared: &Arc, - token: &str, - repo: &str, - path: &str, - body: &Value, -) -> anyhow::Result { - const RETRY_DELAYS: [Duration; 3] = [ - Duration::from_millis(250), - Duration::from_millis(750), - Duration::from_millis(1_500), - ]; - for (attempt, delay) in RETRY_DELAYS.into_iter().enumerate() { - match send_github_check_request( - shared, - &shared.state.github_lifecycle_breaker, - token, - repo, - reqwest::Method::PATCH, - path, - body, - ) - .await - { - Err(error) if is_check_run_not_found(&error) => { - warn!( - attempt = attempt + 1, - delay_ms = delay.as_millis(), - %error, - "new GitHub check run is not visible to PATCH yet; retrying" - ); - tokio::time::sleep(delay).await; - } - result => return result, - } - } - send_github_check_request( - shared, - &shared.state.github_lifecycle_breaker, - token, - repo, - reqwest::Method::PATCH, - path, - body, - ) - .await -} - pub fn run_details_url(run_id: RunId) -> Option { std::env::var("PRELOOP_PUBLIC_URL") .ok() @@ -431,40 +387,74 @@ pub async fn report_existing_check_run_queued( shared.state.events_dirty.notify_one(); Ok(()) } +/// The `external_id` this engine stamps on every check run it creates. It +/// identifies the queue row's own check, so crash-after-POST reconciliation +/// never adopts another workflow's (or another app's) same-named check. +/// +/// Job checks key on `{run_id}:{job_id}`. A workflow-evaluation failure gets a +/// fresh `RunId` on every redelivery but a stable +/// `__workflow_failure__:{name}` job id, so the job id alone is its key. +fn check_run_external_id(update: &crate::control::types::CheckRunUpdate) -> String { + if update.payload.get("kind").and_then(Value::as_str) == Some("workflow_failure") { + update.job_id.0.clone() + } else { + format!("{}:{}", update.run_id, update.job_id) + } +} + +/// Find the check run this queue row already created on `sha`, by its +/// `external_id`. Pages through every check run on the commit (100 per page, +/// bounded) rather than trusting GitHub's default first page of 30, and asks +/// for `filter=all` so a newer same-named check cannot hide ours. async fn find_existing_check_run( shared: &Arc, breaker: &crate::github_breaker::GithubBreaker, token: &str, repo: &str, sha: &str, - name: &str, + external_id: &str, ) -> anyhow::Result> { - let payload = match send_github_check_request( - shared, - breaker, - token, - repo, - reqwest::Method::GET, - &format!("commits/{sha}/check-runs"), - &Value::Null, - ) - .await - { - Ok(payload) => payload, - Err(error) if is_check_run_not_found(&error) => return Ok(None), - Err(error) => return Err(error), - }; - Ok(payload - .get("check_runs") - .and_then(Value::as_array) - .and_then(|check_runs| { - check_runs.iter().find_map(|check_run| { - (check_run.get("name").and_then(Value::as_str) == Some(name) - && check_run.get("head_sha").and_then(Value::as_str) == Some(sha)) - .then(|| check_run.get("id").and_then(Value::as_u64)) - .flatten() - }) - })) + const PER_PAGE: u64 = 100; + const MAX_PAGES: u64 = 10; + for page in 1..=MAX_PAGES { + let payload = match send_github_check_request( + shared, + breaker, + token, + repo, + reqwest::Method::GET, + &format!("commits/{sha}/check-runs?filter=all&per_page={PER_PAGE}&page={page}"), + &Value::Null, + ) + .await + { + Ok(payload) => payload, + Err(error) if is_check_run_not_found(&error) => return Ok(None), + Err(error) => return Err(error), + }; + let check_runs = payload + .get("check_runs") + .and_then(Value::as_array) + .map(Vec::as_slice) + .unwrap_or_default(); + let found = check_runs.iter().find_map(|check_run| { + (check_run.get("external_id").and_then(Value::as_str) == Some(external_id) + && check_run.get("head_sha").and_then(Value::as_str) == Some(sha)) + .then(|| check_run.get("id").and_then(Value::as_u64)) + .flatten() + }); + if found.is_some() { + return Ok(found); + } + let total = payload + .get("total_count") + .and_then(Value::as_u64) + .unwrap_or(0); + if (check_runs.len() as u64) < PER_PAGE || page * PER_PAGE >= total { + return Ok(None); + } + } + Ok(None) } /// Per-installation rate budget key. @@ -482,6 +472,21 @@ pub(crate) enum CheckRunRateKey { /// strand a check run for a day. const MAX_DEFER: std::time::Duration = std::time::Duration::from_secs(3_600); +/// How far each renewal extends a row's lease. Every write to GitHub renews +/// first, and this covers one request at the shared client's 30 s timeout +/// with margin, so a row cannot expire under an in-flight POST and be +/// re-leased by another sender that would POST a second check run. +const CHECK_RUN_LEASE_RENEWAL: std::time::Duration = std::time::Duration::from_secs(60); + +/// Error marker for a row another sender re-leased mid-delivery. +const LOST_CHECK_RUN_LEASE: &str = "check-run lease lost to another sender"; + +/// Attempts during which a PATCH 404 keeps the stored check-run id. GitHub +/// can answer 404 while a just-created check run replicates; only a 404 that +/// outlives these backoff retries means the check is really gone, and only +/// then is the id dropped so the next attempt reconciles or re-creates it. +const CHECK_RUN_NOT_FOUND_RETRIES: i32 = 3; + /// The single background sender for the durable check-run queue. /// /// The serve bootstrap spawns one per node; tests build their own and drive @@ -579,6 +584,11 @@ impl CheckRunSender { let token = resolve_check_run_token(shared, &repo).await; match send_queued_check_run(shared, &self.owner, &breaker, token.as_deref(), update).await { Ok(()) => {} + Err(error) if error.to_string().contains(LOST_CHECK_RUN_LEASE) => { + // Another sender owns the row now; it delivers the update. + // Touching the row here would overwrite its lease state. + debug!(run_id=%update.run_id, job_id=%update.job_id, "check-run lease taken over by another sender; skipping"); + } Err(error) if error.to_string().contains("circuit breaker is open") => { // A breaker that opened between the budget check and the call // is still a deferral, not a failure. @@ -667,11 +677,21 @@ fn check_retry_delay(error: &anyhow::Error, attempts: i32) -> std::time::Duratio { return std::time::Duration::from_secs(seconds.min(86_400)); } - // `rate_reset` is `x-ratelimit-reset`: an absolute unix timestamp. - if let Some(value) = text - .split("rate_reset=") + // `rate_reset` is `x-ratelimit-reset`: an absolute unix timestamp. GitHub + // sends it on every response (404, 422, 5xx included), so it only governs + // the retry when the primary budget is actually spent; otherwise a + // transient failure would wait out the rest of the rate window. + let budget_spent = text + .split("rate_remaining=") .nth(1) .and_then(|v| v.split(|c: char| !c.is_ascii_digit()).next()) + .and_then(|v| v.parse::().ok()) + == Some(0); + if budget_spent + && let Some(value) = text + .split("rate_reset=") + .nth(1) + .and_then(|v| v.split(|c: char| !c.is_ascii_digit()).next()) && let Ok(reset) = value.parse::() { let now = std::time::SystemTime::now() @@ -775,9 +795,12 @@ async fn send_queued_check_run( } } if check_run_id.is_none() { - // Crash-after-POST reconciliation: never POST a second check run for - // the same commit + name. - check_run_id = find_existing_check_run(shared, breaker, token, repo, sha, name).await?; + // Crash-after-POST reconciliation: adopt the check run this row + // already created (matched by its `external_id`), never a same-named + // check from another workflow or app. + let external_id = check_run_external_id(update); + check_run_id = + find_existing_check_run(shared, breaker, token, repo, sha, &external_id).await?; if let Some(id) = check_run_id { persist_check_run_id(shared, owner, update, id).await?; } @@ -1002,17 +1025,41 @@ async fn send_check_run_body( let (method, path) = match check_run_id { Some(id) => (reqwest::Method::PATCH, format!("check-runs/{id}")), None => { - // Creation requires the check name and commit. + // Creation requires the check name and commit; `external_id` is + // what reconciliation matches on after a crash. body["name"] = Value::String(name.to_owned()); body["head_sha"] = Value::String(sha.to_owned()); + body["external_id"] = Value::String(check_run_external_id(update)); (reqwest::Method::POST, "check-runs".to_owned()) } }; + // Hold the row through the request: if it expired and another sender + // re-leased it, that sender owns delivery, and a POST from here would + // create a second check run. + let renewed = shared + .state + .backend + .renew_check_run_update( + owner, + update.run_id, + &update.job_id, + CHECK_RUN_LEASE_RENEWAL, + ) + .await + .map_err(|e| anyhow::anyhow!("{e:?}"))?; + if !renewed { + anyhow::bail!(LOST_CHECK_RUN_LEASE); + } let response = match send_github_check_request(shared, breaker, token, repo, method, &path, &body).await { Ok(response) => response, Err(error) => { - if let Some(stale) = check_run_id.filter(|_| is_check_run_not_found(&error)) { + // An early 404 is usually a just-created check still + // replicating: keep the id and let the backoff retry the + // PATCH. Only a persistent 404 drops it. + if let Some(stale) = check_run_id.filter(|_| { + is_check_run_not_found(&error) && update.attempts >= CHECK_RUN_NOT_FOUND_RETRIES + }) { let _ = shared .state .backend @@ -1028,11 +1075,15 @@ async fn send_check_run_body( } }; let Some(id) = check_run_id else { - let id = response - .get("id") - .and_then(Value::as_u64) - .or(find_existing_check_run(shared, breaker, token, repo, sha, name).await?) - .ok_or_else(|| anyhow::anyhow!("GitHub check-run POST returned no id"))?; + let id = match response.get("id").and_then(Value::as_u64) { + Some(id) => id, + None => { + let external_id = check_run_external_id(update); + find_existing_check_run(shared, breaker, token, repo, sha, &external_id) + .await? + .ok_or_else(|| anyhow::anyhow!("GitHub check-run POST returned no id"))? + } + }; persist_check_run_id(shared, owner, update, id).await?; return Ok(Some(id)); }; @@ -5147,14 +5198,28 @@ jobs: #[tokio::test] async fn sender_reconciles_instead_of_second_post() { let _env = crate::state::GITHUB_ENV_LOCK.lock().await; - let (api, requests) = start_github_stub(|method, path, _body| { + // The row's own check is listed after a same-named check another + // workflow created on the same commit; only the `external_id` match + // may be adopted. The id is filled in once the fixture exists. + let own_external_id = std::sync::Arc::new(parking_lot::Mutex::new(String::new())); + let stub_external_id = own_external_id.clone(); + let (api, requests) = start_github_stub(move |method, path, _body| { if method == "GET" { let existing = if path.contains("commits/") { - serde_json::json!({"check_runs": [{ - "id": 7001, - "name": "build", - "head_sha": "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", - }]}) + serde_json::json!({"total_count": 2, "check_runs": [ + { + "id": 6001, + "name": "build", + "external_id": "another-workflow-run:build", + "head_sha": "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + }, + { + "id": 7001, + "name": "build", + "external_id": *stub_external_id.lock(), + "head_sha": "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + }, + ]}) } else { serde_json::json!({}) }; @@ -5167,6 +5232,7 @@ jobs: let _token = crate::state::TestEnvVar::set("PRELOOP_GITHUB_TOKEN", "test-token"); let (_temp, shared, run_id) = mint_fixture(true, None).await; let job = JobId("build".to_owned()); + *own_external_id.lock() = format!("{run_id}:{}", job.0); // The check run already exists on GitHub but the row has no id. seed_check_run_update(&shared, run_id, &job, 1, "queued", None).await; @@ -5491,27 +5557,43 @@ jobs: async fn terminal_check_retries_not_found_after_creation() { let _env = crate::state::GITHUB_ENV_LOCK.lock().await; let attempts = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); - let mock_app = axum::Router::new().route( - "/repos/owner/repo/check-runs/:id", - axum::routing::patch({ - let attempts = attempts.clone(); - move || { + let creates = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let mock_app = axum::Router::new() + .route( + "/repos/owner/repo/check-runs/:id", + axum::routing::patch({ let attempts = attempts.clone(); - async move { - let attempt = attempts.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - if attempt == 0 { - ( - StatusCode::NOT_FOUND, - Json(serde_json::json!({"message": "Not Found"})), - ) - .into_response() - } else { - Json(serde_json::json!({"id": 7})).into_response() + move || { + let attempts = attempts.clone(); + async move { + let attempt = + attempts.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + if attempt == 0 { + ( + StatusCode::NOT_FOUND, + Json(serde_json::json!({"message": "Not Found"})), + ) + .into_response() + } else { + Json(serde_json::json!({"id": 7})).into_response() + } } } - } - }), - ); + }), + ) + .route( + "/repos/owner/repo/check-runs", + axum::routing::post({ + let creates = creates.clone(); + move || { + let creates = creates.clone(); + async move { + creates.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Json(serde_json::json!({"id": 99})) + } + } + }), + ); let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let port = listener.local_addr().unwrap().port(); let server = tokio::spawn(async move { @@ -5538,15 +5620,72 @@ jobs: let _token = crate::state::TestEnvVar::set("PRELOOP_GITHUB_TOKEN", "check-retry-token"); report_check_run_completed(&shared, run_id, &job_id, ExecutionStatus::Skipped).await; + // The projector queues the update in the background; drain the + // sender until the PATCH is retried past its first 404. + tokio::time::timeout(std::time::Duration::from_secs(10), async { + while attempts.load(std::sync::atomic::Ordering::SeqCst) < 2 { + drain_sender(&shared).await; + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + }) + .await + .expect("the PATCH is retried after an immediate 404"); assert_eq!( attempts.load(std::sync::atomic::Ordering::SeqCst), 2, "an immediate 404 must be retried instead of stranding the check" ); + assert_eq!( + creates.load(std::sync::atomic::Ordering::SeqCst), + 0, + "an immediate 404 must not abandon the check and POST a duplicate" + ); + assert_eq!( + shared + .state + .backend + .job_check_run_id(run_id, &job_id) + .await + .unwrap(), + Some(7), + "the check-run mapping survives the replication-race 404" + ); server.abort(); } + /// GitHub sends `x-ratelimit-reset` on every response, so a failure with + /// budget left keeps the short exponential backoff; only a spent budget + /// waits for the reset. + #[test] + fn check_retry_delay_waits_for_reset_only_when_budget_is_spent() { + let reset = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() + + 1_800; + let failure = |status: u16, remaining: &str| { + anyhow::anyhow!( + "GitHub Check API failed with status {status} Internal Server Error: {{}}; \ + retry_after=; rate_remaining={remaining}; rate_reset={reset}" + ) + }; + + for (status, remaining) in [(500, "4321"), (404, "4999"), (422, "12"), (502, "")] { + let delay = check_retry_delay(&failure(status, remaining), 0); + assert!( + delay < std::time::Duration::from_secs(1), + "status {status} with remaining={remaining:?} must back off briefly, got {delay:?}" + ); + } + + let delay = check_retry_delay(&failure(403, "0"), 0); + assert!( + delay > std::time::Duration::from_secs(1_700), + "a spent budget waits for the advertised reset, got {delay:?}" + ); + } + #[test] fn report_coords_prefer_status_check_sha() { // Late mints must land on the same commit as intake checks: for PR diff --git a/crates/preloop-runner-server/src/state.rs b/crates/preloop-runner-server/src/state.rs index 589711423..ccc29bcbc 100644 --- a/crates/preloop-runner-server/src/state.rs +++ b/crates/preloop-runner-server/src/state.rs @@ -721,11 +721,11 @@ pub struct AppState { /// this (see [`wake_waiters`]); only the sampler listens on it. pub sampler_notify: Arc, pub webhook_queue_notify: Arc, - /// Circuit breaker for durable webhook delivery work. Lifecycle check-run - /// updates use a separate breaker so an ingress outage cannot suppress - /// status transitions for runs already in progress. + /// Circuit breaker for durable webhook delivery work. Check-run updates + /// go through the sender's per-installation budgets instead, so an + /// ingress outage cannot suppress status transitions for runs already in + /// progress. pub github_breaker: Arc, - pub github_lifecycle_breaker: Arc, /// Live status published by the delivery watchdog and App webhook health /// monitor. pub webhook_status: Arc, @@ -1470,7 +1470,6 @@ impl AppState { sampler_notify, webhook_queue_notify: Arc::new(Notify::new()), github_breaker: Arc::new(crate::github_breaker::GithubBreaker::default()), - github_lifecycle_breaker: Arc::new(crate::github_breaker::GithubBreaker::default()), webhook_status: Arc::new(crate::webhook_status::WebhookResilienceStatus::default()), webhook_retry_backoff: crate::github::WEBHOOK_RETRY_BACKOFF.to_vec(), observability: preloop_observability::Observability::noop(), From 80203ec2f5a23970e4e07e5e034e020840757148 Mon Sep 17 00:00:00 2001 From: Bill Njoroge Date: Tue, 6 Oct 2026 22:20:37 -0400 Subject: [PATCH 3/4] test(server): drive the check-run sender in tests written for inline checks Submit, push sync and cancel now only record desired check-run state; the background sender creates and completes checks, and the test harness spawns no sender. The push/check tests added on the base after this branch began waited for an inline POST that never comes. Drain the sender inside their wait loops, as broker.rs already does. Two fixture fixes surfaced once they ran: the dirty-push mock's commits/*ref wildcard also caught the sender's commits/{sha}/check-runs lookup, and the logs tests injected a check-run mapping on a run not stamped reports_check_runs, which the projector rightly skips. --- .../tests/concurrency.rs | 29 ++++++++++++++----- crates/preloop-runner-server/tests/logs.rs | 19 ++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/crates/preloop-runner-server/tests/concurrency.rs b/crates/preloop-runner-server/tests/concurrency.rs index 92a467b39..57320a8e3 100644 --- a/crates/preloop-runner-server/tests/concurrency.rs +++ b/crates/preloop-runner-server/tests/concurrency.rs @@ -2326,10 +2326,12 @@ async fn submit_driven_push_publishes_pr_and_checks_idempotently() { let accepted = submit_push_run(&app, SHA, TREE).await; let run_id = accepted["run_id"].as_str().unwrap().to_owned(); let parsed_run_id: RunId = run_id.parse().unwrap(); - // Check-run creation on submit is detached from the HTTP response, so - // wait for the background reporting task to populate the check run id. + // Submit only records the desired check-run state; the background sender + // creates the check. The harness spawns no sender, so drain it inline + // until the mapping appears. tokio::time::timeout(std::time::Duration::from_secs(5), async { loop { + crate::github::drain_check_run_sender(&state.shared()).await; let ready = { let tx = state.test_tx().await; tx.runs @@ -2673,6 +2675,7 @@ async fn push_intake_reports_no_check_run_for_expandable_placeholders() { // wait for the background reporting task to populate the mapping. tokio::time::timeout(std::time::Duration::from_secs(5), async { loop { + crate::github::drain_check_run_sender(&state.shared()).await; let ready = { let inner = state.test_tx().await; inner @@ -2759,6 +2762,11 @@ async fn dirty_push_sync_verifies_the_branch_head_and_reports_checks_on_the_mate // endpoint matches the whole remaining path. "/repos/owner/repo/commits/*ref", get(|Path(r#ref): Path| async move { + // The wildcard also catches the check-run sender's + // `commits/{sha}/check-runs` dedup lookup: nothing exists yet. + if r#ref.ends_with("/check-runs") { + return Json(json!({"total_count": 0, "check_runs": []})); + } assert_eq!(r#ref, "feat/x", "dirty sync must verify the branch head"); Json(json!({ "sha": MATERIALIZED, @@ -2865,11 +2873,18 @@ async fn dirty_push_sync_verifies_the_branch_head_and_reports_checks_on_the_mate .await; assert_eq!(status, StatusCode::OK, "dirty sync must succeed: {body}"); assert_eq!(pr_creates.load(Ordering::SeqCst), 1, "PR created"); - let check = check_creates - .lock() - .first() - .cloned() - .expect("queued check run"); + // The sync records desired check-run state; the background projector + // queues it and the sender creates the check. The harness spawns no + // sender, so drain it until the POST lands. + tokio::time::timeout(std::time::Duration::from_secs(5), async { + while check_creates.lock().is_empty() { + crate::github::drain_check_run_sender(&state.shared()).await; + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + }) + .await + .expect("queued check run"); + let check = check_creates.lock().first().cloned().unwrap(); assert_eq!( check["head_sha"], MATERIALIZED, "checks attach to the materialized head commit, not the base" diff --git a/crates/preloop-runner-server/tests/logs.rs b/crates/preloop-runner-server/tests/logs.rs index 1173ab726..9fe7a3b0d 100644 --- a/crates/preloop-runner-server/tests/logs.rs +++ b/crates/preloop-runner-server/tests/logs.rs @@ -2648,8 +2648,16 @@ async fn cancel_run_completes_github_checks_and_terminal_metadata() { let run_id: RunId = accepted["run_id"].as_str().unwrap().parse().unwrap(); state .test_db_mutate(|tx| { + // A check-run mapping only exists on a run that reports checks + // (webhook or `--push`); stamp both so the projector delivers + // the completion, as it would for a real reporting run. tx.set_job_check_run(run_id, &JobId("build".into()), CHECK_RUN_ID as i64) .unwrap(); + tx.execute( + "UPDATE runs SET reports_check_runs = 1 WHERE run_id = ?1", + [run_id.to_string()], + ) + .unwrap(); }) .await; @@ -2674,6 +2682,8 @@ async fn cancel_run_completes_github_checks_and_terminal_metadata() { .iter() .any(|(id, body)| *id == CHECK_RUN_ID && body["status"] == "completed") { + // The cancel queues the completion; the sender delivers it. + crate::github::drain_check_run_sender(&state.shared()).await; tokio::time::sleep(std::time::Duration::from_millis(10)).await; } }) @@ -2730,8 +2740,15 @@ async fn completed_check_uploads_every_annotation_in_batches_of_fifty() { let job_id = JobId("build".to_owned()); state .test_db_mutate(|tx| { + // A check-run mapping only exists on a run that reports checks; + // stamp both so the projector delivers the completion. tx.set_job_check_run(run_id, &job_id, CHECK_RUN_ID as i64) .unwrap(); + tx.execute( + "UPDATE runs SET reports_check_runs = 1 WHERE run_id = ?1", + [run_id.to_string()], + ) + .unwrap(); }) .await; { @@ -2768,6 +2785,8 @@ async fn completed_check_uploads_every_annotation_in_batches_of_fifty() { .last() .is_some_and(|body| body["status"] == "completed") { + // The cancel queues the completion; the sender delivers it. + crate::github::drain_check_run_sender(&state.shared()).await; tokio::time::sleep(std::time::Duration::from_millis(10)).await; } }) From 0b1382c95adccb963e6061836653bbc145176a1a Mon Sep 17 00:00:00 2001 From: Bill Njoroge Date: Tue, 6 Oct 2026 22:20:37 -0400 Subject: [PATCH 4/4] test(control): lease-renewal contract; horizon-safe outbox counts on Postgres Postgres reads the outbox only below the cluster-wide pg_snapshot_xmin, so an open transaction in another test's database on the shared server hides fresh rows. check_run_bookmark_survives_consumer_restart counted once and failed on CI; prune_outbox_respects_slowest_consumer could count before the post-bookmark rows were visible and then assert exact equality. Retry the count, wait for the second submit's rows, and assert prune removed no more than it reported (rows can surface late, never vanish). New suite test on both backends: a sender whose lease was taken over learns it lost the row and cannot take it back. --- .../src/control/tests.rs | 133 +++++++++++++++--- 1 file changed, 112 insertions(+), 21 deletions(-) diff --git a/crates/preloop-runner-server/src/control/tests.rs b/crates/preloop-runner-server/src/control/tests.rs index 352fe8b80..0f08806dc 100644 --- a/crates/preloop-runner-server/src/control/tests.rs +++ b/crates/preloop-runner-server/src/control/tests.rs @@ -2884,17 +2884,18 @@ pub(crate) mod suite { .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) .await .unwrap(); - let total = backend - .outbox_read( - OutboxBookmark { - txid: 0, - event_id: 0, - }, - 10_000, - ) - .await - .unwrap() - .len(); + // Postgres reads only below the cluster-wide snapshot `xmin`, so an + // open transaction in another test's database on the shared server + // hides these freshly committed rows for a moment. Retry like the + // consumer loops below instead of counting once. + let mut total = 0; + for _ in 0..600 { + total = outbox_len(backend).await; + if total > 1 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } assert!(total > 1, "a submit emits several outbox rows"); // First consumer reads exactly one event and commits the bookmark. @@ -2954,6 +2955,74 @@ pub(crate) mod suite { ); } + /// Renewal keeps only the current owner's lease: a sender whose lease + /// expired and was re-leased by another sender is told so (and so must + /// not POST), and its renewal attempt does not steal the row back. + pub(crate) async fn check_run_lease_renewal_respects_takeover(backend: &dyn ControlBackend) { + let run_id = RunId::new(); + let build = JobId("build".to_owned()); + backend + .submit_run(submit_run(run_id, vec![submit_job(run_id, "build", 1)])) + .await + .unwrap(); + backend + .enqueue_check_run_update(CheckRunUpdateInput { + run_id, + job_id: build.clone(), + installation_id: 0, + check_run_id: None, + version: 1, + payload: serde_json::json!({ + "repository": "owner/repo", + "sha": "abc123", + "status": "queued", + "name": "build", + }), + }) + .await + .unwrap(); + let holds = |rows: &[crate::control::types::CheckRunUpdate]| { + rows.iter() + .any(|row| row.run_id == run_id && row.job_id == build) + }; + + let first = backend + .lease_check_run_updates("sender-a", std::time::Duration::from_millis(1), 100) + .await + .unwrap(); + assert!(holds(&first), "sender-a leases the row"); + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + let second = backend + .lease_check_run_updates("sender-b", std::time::Duration::from_secs(30), 100) + .await + .unwrap(); + assert!(holds(&second), "sender-b re-leases the expired row"); + + let lease = std::time::Duration::from_secs(60); + assert!( + !backend + .renew_check_run_update("sender-a", run_id, &build, lease) + .await + .unwrap(), + "the superseded sender must learn it lost the row" + ); + assert!( + backend + .renew_check_run_update("sender-b", run_id, &build, lease) + .await + .unwrap(), + "the current owner renews" + ); + let third = backend + .lease_check_run_updates("sender-c", std::time::Duration::from_secs(30), 100) + .await + .unwrap(); + assert!( + !holds(&third), + "a renewed lease is not up for grabs, and the failed renewal took nothing" + ); + } + /// Outbox prune never passes the durable consumer bookmark, and still /// bounds growth by age when no consumer has registered. pub(crate) async fn prune_outbox_respects_slowest_consumer(backend: &dyn ControlBackend) { @@ -2981,10 +3050,13 @@ pub(crate) mod suite { pruned >= 1, "with no durable consumer, age prunes the stream" ); - assert_eq!( - outbox_len(backend).await, - before - pruned as usize, - "prune removes exactly the rows it counted" + // Postgres hides rows above the cluster-wide snapshot `xmin`, so a + // row the count missed can surface after the prune; rows never + // vanish except by pruning. Hence `>=`: prune removed no more than it + // reported. + assert!( + outbox_len(backend).await >= before - pruned as usize, + "prune removes no more rows than it counted" ); // A consumer's bookmark: rows at/below it are prunable, rows past it @@ -2996,6 +3068,10 @@ pub(crate) mod suite { .unwrap(); let fast = consume_until_quiescent(backend, "prune-consumer-fast").await; assert!(fast > 0); + // Rows past the bookmark must be visible before the prune, or the + // "they survive" assertion below is vacuous: wait until the second + // submit's rows are counted, not just the first run's. + let before_second = stable_outbox_len(backend).await; let run_id2 = RunId::new(); backend .submit_run(submit_run(run_id2, vec![submit_job(run_id2, "build", 1)])) @@ -3004,22 +3080,24 @@ pub(crate) mod suite { let mut total = 0; for _ in 0..600 { total = stable_outbox_len(backend).await; - if total > 0 { + if total > before_second { break; } tokio::time::sleep(std::time::Duration::from_millis(50)).await; } - assert!(total > 0); + assert!( + total > before_second, + "the second submit's rows become visible" + ); let pruned = backend .prune_outbox(std::time::Duration::ZERO, 10_000) .await .unwrap(); assert!(pruned >= 1, "rows at or below the bookmark are prunable"); let remaining = outbox_len(backend).await; - assert_eq!( - remaining, - total - pruned as usize, - "prune removes exactly the rows it counted" + assert!( + remaining >= total - pruned as usize, + "prune removes no more rows than it counted" ); assert!( remaining >= 1, @@ -5439,6 +5517,14 @@ mod pg { suite::check_run_bookmark_survives_consumer_restart(&backend).await; } + #[tokio::test] + async fn check_run_lease_renewal_respects_takeover() { + let Some((_pg, backend)) = backend().await else { + return skip_no_postgres(); + }; + suite::check_run_lease_renewal_respects_takeover(&backend).await; + } + #[tokio::test] async fn prune_outbox_respects_slowest_consumer() { let Some((_pg, backend)) = backend().await else { @@ -6592,6 +6678,11 @@ mod lite { suite::prune_outbox_respects_slowest_consumer(&LiteBackend::in_memory().unwrap()).await; } + #[tokio::test] + async fn check_run_lease_renewal_respects_takeover() { + suite::check_run_lease_renewal_respects_takeover(&LiteBackend::in_memory().unwrap()).await; + } + /// The bookmark lives in the SQLite file: reopen the database and only the /// events committed while the consumer was down are delivered. #[tokio::test]