From b8f64a5f3ecd9d054e1137d2f5bef82695ca5c90 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 13:47:06 -0400 Subject: [PATCH 01/12] fix(store): drop undecodable workspace snapshots instead of bricking startup A snapshot persisted by an older binary (pre-#143, before WorkspaceSnapshot gained tree_sha) fails serde round-trip on load. restore_run_record propagated the error, so load_into aborted and the whole server refused to start. The store is best-effort: log and continue, matching the session-key and broker-message restore paths. --- crates/preloop-runner-server/src/store.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/crates/preloop-runner-server/src/store.rs b/crates/preloop-runner-server/src/store.rs index 56c86110..540fa63c 100644 --- a/crates/preloop-runner-server/src/store.rs +++ b/crates/preloop-runner-server/src/store.rs @@ -555,9 +555,18 @@ pub(crate) fn restore_run_record(cipher: &Envelope, blob: &[u8]) -> anyhow::Resu .unwrap_or_default() .to_owned(); // `run_record_value` always writes the key (null when absent), so a - // JSON null must restore as `None` rather than fail to parse. + // JSON null must restore as `None` rather than fail to parse. A + // snapshot whose shape this binary no longer understands is dropped + // with a warning: the store is best-effort and one stale record must + // not brick startup (see `load_into`). run.workspace_snapshot = match object.get("workspace_snapshot") { - Some(value) if !value.is_null() => Some(serde_json::from_value(value.clone())?), + Some(value) if !value.is_null() => match serde_json::from_value(value.clone()) { + Ok(snapshot) => Some(snapshot), + Err(error) => { + tracing::warn!(run_id = %run.run_id, %error, "dropping undecodable workspace snapshot on load"); + None + } + }, _ => None, }; } From 13b6b6eeeb7e9c58b81e8d7e8b8931801dab45eb Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 15:29:29 -0400 Subject: [PATCH 02/12] fix(orchestrator): re-arm fingerprint-suffixed packed goldens whose checkpoint is spent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit run_slot names per-environment packed goldens '{prefix}-golden-{fp12}' but provision_runner's managed-golden guards only matched the plain '{prefix}-golden' form. A spent per-environment golden (retained RAM checkpoint consumed or lost) therefore looped forever on 'golden is already paused; a valid retained checkpoint is required' — never re-arming, never falling back — starving every queued job. managed_golden now matches both the plain form and the 12-hex-char fingerprint-suffixed form, and deliberately excludes '{prefix}-golden-environment' baked goldens (different runs-on image; falling back would run the job on the wrong OS). This also restores the golden_is_packed branch for fingerprint-suffixed goldens, so forks of a per-environment packed golden no longer attempt per-fork toolchain installs (the DNS-dependent fallback that failed in production). Adds a regression test for the fingerprint-suffixed re-arm path. --- crates/preloop-orchestrator/src/lib.rs | 84 ++++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 5 deletions(-) diff --git a/crates/preloop-orchestrator/src/lib.rs b/crates/preloop-orchestrator/src/lib.rs index 3c4cfd2f..5e65f182 100644 --- a/crates/preloop-orchestrator/src/lib.rs +++ b/crates/preloop-orchestrator/src/lib.rs @@ -3617,6 +3617,31 @@ fn fork_base_unusable(error: &VmError) -> bool { .any(|signature| message.contains(signature)) } +/// Whether `golden` is a fork base this pool baked and manages. +/// +/// The plain form (`{prefix}-golden`) is the single golden baked for the +/// default environment at pool startup; the fingerprint-suffixed form +/// (`{prefix}-golden-{fingerprint12}`) is prepared on demand by `run_slot` +/// when a job asks for a non-default base image. Both carry a packed golden +/// artifact and both can lose their retained fork checkpoint, so the +/// re-arm / independent-create fallback applies to each. +/// +/// A name like `{prefix}-golden-environment` is deliberately NOT matched: +/// that is a baked environment golden serving a different `runs-on` image, +/// and replacing it with the packed artifact would run the job on the wrong +/// operating system. +fn managed_golden(config: &RunnerPoolConfig, golden: &MachineName) -> bool { + let prefix = format!("{}-golden", config.name_prefix); + golden.as_str() == prefix + || golden + .as_str() + .strip_prefix(&prefix) + .is_some_and(|rest| { + rest.strip_prefix('-') + .is_some_and(|fp| fp.len() == 12 && fp.bytes().all(|b| b.is_ascii_hexdigit())) + }) +} + /// Create, boot, and register one ephemeral runner; return its `run` argv. /// /// The caller owns cleanup: on any error the machine may already exist. @@ -3635,7 +3660,7 @@ async fn provision_runner( Ok(()) => Some(golden), Err(error @ VmError::ForkBaseBusy { .. }) if config.use_packed_artifact - && golden.as_str() == format!("{}-golden", config.name_prefix) => + && managed_golden(config, golden) => { // A live plain-fork clone still depends on the golden's frozen // storage. Do not touch the base and do not create another VM @@ -3652,8 +3677,7 @@ async fn provision_runner( None } Err(error) - if config.use_packed_artifact - && golden.as_str() == format!("{}-golden", config.name_prefix) => + if config.use_packed_artifact && managed_golden(config, golden) => { if fork_base_unusable(&error) { // The base is spent. Re-arm it atomically with forking: @@ -3798,8 +3822,8 @@ async fn provision_runner( // so an env-golden fork boots the bare stock base image. Install the // apt baseline and toolchains into the fork itself — it is the job's // single-use machine, so the writes persist for its lifetime. - let golden_is_packed = config.use_packed_artifact - && golden.as_str() == format!("{}-golden", config.name_prefix); + let golden_is_packed = + config.use_packed_artifact && managed_golden(config, golden); if golden_is_packed { // The pack carries the apt baseline, but not necessarily apt's // indices — restore them before any workflow apt-installs. A @@ -5379,6 +5403,56 @@ chmod +x "$destination/bin/node" ); } + /// A fingerprint-suffixed golden (`{prefix}-golden-{fingerprint}`, the + /// name `run_slot` uses for a non-default base image) must be recognized + /// as managed by this pool. The stale guard matched only the plain + /// `{prefix}-golden` form, so a spent per-environment golden looped on + /// "already paused" forever instead of re-arming. + #[tokio::test] + async fn spent_fingerprint_suffixed_fork_base_is_rearmed_and_retried() { + let provider = Arc::new( + TestProvider::new(false, false, false, false, false) + .with_live_forks(false) + .failing_fork_once_spent(), + ); + let config = packed_fork_config(); + let golden = MachineName::new("lifecycle-test-golden-3577f5d5a384").unwrap(); + let name = MachineName::new("lifecycle-test-0-8").unwrap(); + + provision_runner( + &provider, + &config, + &name, + Some(&golden), + &Arc::new(KeyPool::new()), + &test_runner_environment(config.base_image.clone(), Vec::new(), true), + ) + .await + .expect("the re-armed fingerprint-suffixed golden serves the fork"); + + let events = provider.events().await; + let expected = [ + format!("fork:{}:{}", golden.as_str(), name.as_str()), + format!("rearm:{}", golden.as_str()), + format!("delete:{}", name.as_str()), + format!("stop:{}", golden.as_str()), + format!("start:{}", golden.as_str()), + format!("fork:{}:{}", golden.as_str(), name.as_str()), + ]; + let mut cursor = 0; + for event in &expected { + let position = events[cursor..] + .iter() + .position(|seen| seen == event) + .expect("re-arm sequence must include every step"); + cursor += position + 1; + } + assert!( + provider.has_machine(&name).await, + "the retried fork must leave the clone provisioned" + ); + } + /// A spent base that still has live clones must NOT be re-armed: resuming /// it would corrupt the copy-on-write clones. The pool falls back to a /// full create instead. From 74e0cb71947d91c0bbf8bcd24e511d05eb1465e2 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 16:15:40 -0400 Subject: [PATCH 03/12] fix(broker): re-derive a missing dispatch token request at claim A token request registered at build time can be lost when the process dies before the next store snapshot flush: jobs enqueued since the last snapshot restore claim with 'no dispatch token request', the checkout keeps the local runtime JWT, and every git fetch fails on auth. The broker now re-derives the request from the run's submission and the job's declared permissions (the same inputs build_job_artifacts used) whenever the GitHub App is configured, registers it for re-claims, and mints under that policy. This restores the ghs_ installation token for runs that survived an ungraceful restart without their persisted token request. --- crates/preloop-runner-server/src/broker.rs | 111 ++++++++++++++++++++- 1 file changed, 109 insertions(+), 2 deletions(-) diff --git a/crates/preloop-runner-server/src/broker.rs b/crates/preloop-runner-server/src/broker.rs index 65e144cb..2deb26d7 100644 --- a/crates/preloop-runner-server/src/broker.rs +++ b/crates/preloop-runner-server/src/broker.rs @@ -778,10 +778,117 @@ pub(crate) async fn broker_acquire_job( let mut inner = shared.state.inner.lock().await; inner.broker_messages.insert(request_id, message.clone()); } else { - tracing::warn!( + // A token request registered at build time can be lost when the + // process dies before the next store snapshot flush (jobs enqueued + // since the last snapshot restore with `github_token_requests` + // missing). The claim then reaches here with no request to mint + // from, the checkout keeps the local runtime JWT, and every git + // fetch fails on auth. Re-derive the request from the run's + // submission and the job's declared permissions — the same inputs + // `build_job_artifacts` used — and mint under that policy. + let derived = if shared.state.github_app.is_none() { + None + } else { + let inner = shared.state.inner.lock().await; + let record = inner.job_requests.get(&request_id); + let run = record.and_then(|record| inner.runs.get(&record.run_id)); + match (record, run) { + (Some(record), Some(run)) => { + let tier = run + .submission + .trust_tier + .as_deref() + .and_then(|tier| { + serde_json::from_str::(tier) + .ok() + }); + let declared = run + .submission + .payload + .get("workflow_job") + .and_then(|job| job.get("permissions")) + .and_then(serde_json::Value::as_object) + .map(|permissions| { + permissions + .iter() + .map(|(k, v)| { + (k.clone(), v.as_str().unwrap_or("read").to_owned()) + }) + .collect::>() + }); + let policy = crate::events::trust_tier::job_authorization( + tier, + declared.as_ref(), + false, + ); + Some(( + crate::models::GitHubTokenRequest { + repository: run.submission.repository.clone(), + permissions: policy.app_permissions, + declared: declared.is_some(), + untrusted: policy.fork_restricted, + }, + record.request_id, + )) + } + _ => None, + } + }; + if let Some((token_request, derived_request_id)) = derived { + // Register the derived request so a re-claim after a disconnect + // re-mints under the same derived policy, then mint. + { + let mut inner = shared.state.inner.lock().await; + inner + .github_token_requests + .insert(derived_request_id, token_request.clone()); + } + tracing::info!( request_id, - "broker acquire: no dispatch token request for job" + repository = %token_request.repository, + "broker acquire: re-derived missing dispatch token request at claim" ); + let minted = match mint_dispatch_github_token(&shared, &token_request).await { + Ok(minted) => minted, + Err(error) => { + fail_unclaimable_request(&shared, request_id).await; + return Err(error); + } + }; + if let Some(minted) = minted { + let token = minted.token; + tracing::info!( + token_len = token.len(), + "minted re-derived dispatch GitHub token at claim" + ); + message.variables.insert( + "system.github.token".to_owned(), + preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), + ); + message.variables.insert( + "github_token".to_owned(), + preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), + ); + message.variables.insert( + "GITHUB_TOKEN".to_owned(), + preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), + ); + match message.context_data.get_mut("github") { + Some(preloop_gha_protocol::azdo::PipelineContextData::Dict(github)) => { + github.insert( + "token".to_owned(), + preloop_gha_protocol::azdo::PipelineContextData::String(token), + ); + } + other => tracing::warn!( + github_context = %match other { Some(_) => "non-dict", None => "missing" }, + "could not patch github context token for re-derived request" + ), + } + let mut inner = shared.state.inner.lock().await; + inner.broker_messages.insert(request_id, message.clone()); + } + } } // The snapshot checkout token is pinned onto the step at submission, // but a job can sit queued well past its ~50-minute lifetime. The From b76e6f2ca1996b1960c2352a44a7aeb5a9738eb9 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 16:37:58 -0400 Subject: [PATCH 04/12] style: cargo fmt --- crates/preloop-orchestrator/src/lib.rs | 21 ++-- crates/preloop-runner-server/src/broker.rs | 111 ++++++++---------- .../src/results_twirp.rs | 15 ++- crates/preloop-runner-server/src/store.rs | 2 +- 4 files changed, 72 insertions(+), 77 deletions(-) diff --git a/crates/preloop-orchestrator/src/lib.rs b/crates/preloop-orchestrator/src/lib.rs index 5e65f182..61d71c74 100644 --- a/crates/preloop-orchestrator/src/lib.rs +++ b/crates/preloop-orchestrator/src/lib.rs @@ -3633,13 +3633,10 @@ fn fork_base_unusable(error: &VmError) -> bool { fn managed_golden(config: &RunnerPoolConfig, golden: &MachineName) -> bool { let prefix = format!("{}-golden", config.name_prefix); golden.as_str() == prefix - || golden - .as_str() - .strip_prefix(&prefix) - .is_some_and(|rest| { - rest.strip_prefix('-') - .is_some_and(|fp| fp.len() == 12 && fp.bytes().all(|b| b.is_ascii_hexdigit())) - }) + || golden.as_str().strip_prefix(&prefix).is_some_and(|rest| { + rest.strip_prefix('-') + .is_some_and(|fp| fp.len() == 12 && fp.bytes().all(|b| b.is_ascii_hexdigit())) + }) } /// Create, boot, and register one ephemeral runner; return its `run` argv. @@ -3659,8 +3656,7 @@ async fn provision_runner( Some(golden) => match provider.fork(golden, name).await { Ok(()) => Some(golden), Err(error @ VmError::ForkBaseBusy { .. }) - if config.use_packed_artifact - && managed_golden(config, golden) => + if config.use_packed_artifact && managed_golden(config, golden) => { // A live plain-fork clone still depends on the golden's frozen // storage. Do not touch the base and do not create another VM @@ -3676,9 +3672,7 @@ async fn provision_runner( direct_create_from_packed = false; None } - Err(error) - if config.use_packed_artifact && managed_golden(config, golden) => - { + Err(error) if config.use_packed_artifact && managed_golden(config, golden) => { if fork_base_unusable(&error) { // The base is spent. Re-arm it atomically with forking: // partial-clone cleanup, the live-clone check, and the @@ -3822,8 +3816,7 @@ async fn provision_runner( // so an env-golden fork boots the bare stock base image. Install the // apt baseline and toolchains into the fork itself — it is the job's // single-use machine, so the writes persist for its lifetime. - let golden_is_packed = - config.use_packed_artifact && managed_golden(config, golden); + let golden_is_packed = config.use_packed_artifact && managed_golden(config, golden); if golden_is_packed { // The pack carries the apt baseline, but not necessarily apt's // indices — restore them before any workflow apt-installs. A diff --git a/crates/preloop-runner-server/src/broker.rs b/crates/preloop-runner-server/src/broker.rs index 2deb26d7..e4a9d02c 100644 --- a/crates/preloop-runner-server/src/broker.rs +++ b/crates/preloop-runner-server/src/broker.rs @@ -794,14 +794,9 @@ pub(crate) async fn broker_acquire_job( let run = record.and_then(|record| inner.runs.get(&record.run_id)); match (record, run) { (Some(record), Some(run)) => { - let tier = run - .submission - .trust_tier - .as_deref() - .and_then(|tier| { - serde_json::from_str::(tier) - .ok() - }); + let tier = run.submission.trust_tier.as_deref().and_then(|tier| { + serde_json::from_str::(tier).ok() + }); let declared = run .submission .payload @@ -811,9 +806,7 @@ pub(crate) async fn broker_acquire_job( .map(|permissions| { permissions .iter() - .map(|(k, v)| { - (k.clone(), v.as_str().unwrap_or("read").to_owned()) - }) + .map(|(k, v)| (k.clone(), v.as_str().unwrap_or("read").to_owned())) .collect::>() }); let policy = crate::events::trust_tier::job_authorization( @@ -835,61 +828,61 @@ pub(crate) async fn broker_acquire_job( } }; if let Some((token_request, derived_request_id)) = derived { - // Register the derived request so a re-claim after a disconnect - // re-mints under the same derived policy, then mint. - { - let mut inner = shared.state.inner.lock().await; - inner - .github_token_requests - .insert(derived_request_id, token_request.clone()); - } - tracing::info!( - request_id, - repository = %token_request.repository, - "broker acquire: re-derived missing dispatch token request at claim" - ); - let minted = match mint_dispatch_github_token(&shared, &token_request).await { - Ok(minted) => minted, - Err(error) => { - fail_unclaimable_request(&shared, request_id).await; - return Err(error); + // Register the derived request so a re-claim after a disconnect + // re-mints under the same derived policy, then mint. + { + let mut inner = shared.state.inner.lock().await; + inner + .github_token_requests + .insert(derived_request_id, token_request.clone()); } - }; - if let Some(minted) = minted { - let token = minted.token; tracing::info!( - token_len = token.len(), - "minted re-derived dispatch GitHub token at claim" - ); - message.variables.insert( - "system.github.token".to_owned(), - preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), - ); - message.variables.insert( - "github_token".to_owned(), - preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), - ); - message.variables.insert( - "GITHUB_TOKEN".to_owned(), - preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), + request_id, + repository = %token_request.repository, + "broker acquire: re-derived missing dispatch token request at claim" ); - match message.context_data.get_mut("github") { - Some(preloop_gha_protocol::azdo::PipelineContextData::Dict(github)) => { - github.insert( - "token".to_owned(), - preloop_gha_protocol::azdo::PipelineContextData::String(token), - ); + let minted = match mint_dispatch_github_token(&shared, &token_request).await { + Ok(minted) => minted, + Err(error) => { + fail_unclaimable_request(&shared, request_id).await; + return Err(error); } - other => tracing::warn!( - github_context = %match other { Some(_) => "non-dict", None => "missing" }, - "could not patch github context token for re-derived request" - ), + }; + if let Some(minted) = minted { + let token = minted.token; + tracing::info!( + token_len = token.len(), + "minted re-derived dispatch GitHub token at claim" + ); + message.variables.insert( + "system.github.token".to_owned(), + preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), + ); + message.variables.insert( + "github_token".to_owned(), + preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), + ); + message.variables.insert( + "GITHUB_TOKEN".to_owned(), + preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), + ); + match message.context_data.get_mut("github") { + Some(preloop_gha_protocol::azdo::PipelineContextData::Dict(github)) => { + github.insert( + "token".to_owned(), + preloop_gha_protocol::azdo::PipelineContextData::String(token), + ); + } + other => tracing::warn!( + github_context = %match other { Some(_) => "non-dict", None => "missing" }, + "could not patch github context token for re-derived request" + ), + } + let mut inner = shared.state.inner.lock().await; + inner.broker_messages.insert(request_id, message.clone()); } - let mut inner = shared.state.inner.lock().await; - inner.broker_messages.insert(request_id, message.clone()); } } - } // The snapshot checkout token is pinned onto the step at submission, // but a job can sit queued well past its ~50-minute lifetime. The // checkout would then be answered with a git 401 that the step can diff --git a/crates/preloop-runner-server/src/results_twirp.rs b/crates/preloop-runner-server/src/results_twirp.rs index dbcfd84c..7938b99c 100644 --- a/crates/preloop-runner-server/src/results_twirp.rs +++ b/crates/preloop-runner-server/src/results_twirp.rs @@ -996,8 +996,14 @@ mod cache_pb_tests { metadata: Some(PbCacheMetadata { repository_id: 42, scope: vec![ - PbCacheScope { scope: "refs/heads/main".to_string(), permission: 1 }, - PbCacheScope { scope: "refs/heads/feature".to_string(), permission: 2 }, + PbCacheScope { + scope: "refs/heads/main".to_string(), + permission: 1, + }, + PbCacheScope { + scope: "refs/heads/feature".to_string(), + permission: 2, + }, ], }), key: "k".to_string(), @@ -1015,7 +1021,10 @@ mod cache_pb_tests { let fixture = include_bytes!("../../../fixtures/wire/cache-multi-scope.pb"); let (_, _, _, fixture_scopes, _) = pb_cache_request(fixture, CacheRequestKind::GetDownloadUrl).unwrap(); - assert_eq!(fixture_scopes, vec!["refs/heads/main", "refs/heads/feature"]); + assert_eq!( + fixture_scopes, + vec!["refs/heads/main", "refs/heads/feature"] + ); } #[test] diff --git a/crates/preloop-runner-server/src/store.rs b/crates/preloop-runner-server/src/store.rs index 540fa63c..0509e7f6 100644 --- a/crates/preloop-runner-server/src/store.rs +++ b/crates/preloop-runner-server/src/store.rs @@ -18,8 +18,8 @@ use async_trait::async_trait; use preloop_gha_protocol::SessionId; use rusqlite::{params, Connection, OptionalExtension, Transaction}; use sha2::Digest; -use std::sync::Mutex as StdMutex; use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::Mutex as StdMutex; const DATABASE_FILE: &str = "preloop.db"; pub(crate) const SNAPSHOT_FORMAT: u8 = 2; From 09880394d6a6001710566fbf69c068986080b6b7 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 18:17:44 -0400 Subject: [PATCH 05/12] feat(update): resolve same-version installs by build commit, not bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The equal-version branch byte-compared the installed binary against the release asset and reinstalled on any drift. That clobbered a source build from newer main: main reports the same version string as the latest tag (no bump between tag and HEAD), so a build carrying #149/#151/#164 was treated as drift and replaced with the stale release binary every hour. Embed the build commit (build.rs reads git rev-parse HEAD) and expose it in 'preloop version'. The updater now compares commits via the GitHub compare API when versions are equal: - release commit is ahead of installed (installed is an ancestor — the v0.30.2 deaf-runner case) -> reinstall - installed is at or beyond the release -> keep - diverged history (release cut from a dist commit off main, or a local build) or unverifiable (no embedded commit) -> keep; never clobber a real build on an ambiguous comparison Keeps the version-greater upgrade path and the version-less stop unchanged. Drops the byte-compare and its tests; adds decision-mapping tests for ahead/behind/identical/diverged/unknown. --- crates/preloop-cli/build.rs | 23 +++ crates/preloop-cli/src/main.rs | 6 +- crates/preloop-cli/src/update.rs | 237 +++++++++++++++++-------------- 3 files changed, 162 insertions(+), 104 deletions(-) diff --git a/crates/preloop-cli/build.rs b/crates/preloop-cli/build.rs index 561bf1e5..2dccc967 100644 --- a/crates/preloop-cli/build.rs +++ b/crates/preloop-cli/build.rs @@ -11,6 +11,7 @@ use std::env; use std::fs; use std::path::PathBuf; +use std::process::Command; fn main() { let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR")); @@ -43,4 +44,26 @@ fn main() { let out_dir = PathBuf::from(env::var("OUT_DIR").expect("OUT_DIR")); fs::write(out_dir.join("pins.rs"), out).expect("write pins.rs"); println!("cargo:rerun-if-changed=../../versions.toml"); + + // Embed the exact source commit this binary was built from. The updater + // uses it to resolve same-version installs: a source build from newer + // main reports the same version string as the latest release tag, and + // byte-comparing the binaries would clobber the newer build with the + // stale release asset. The commit is the monotonic signal the version + // string cannot carry. Release assets are built in CI from a git + // checkout, so the SHA is always present there; a non-git build (e.g. + // `cargo install` from a crates.io tarball) falls back to a sentinel + // that the updater treats as "cannot verify, keep what is installed". + let commit = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(&manifest_dir) + .output() + .ok() + .filter(|output| output.status.success()) + .and_then(|output| String::from_utf8(output.stdout).ok()) + .map(|sha| sha.trim().to_owned()) + .filter(|sha| !sha.is_empty()) + .unwrap_or_else(|| "unknown".to_owned()); + println!("cargo:rustc-env=PRELOOP_BUILD_COMMIT={commit}"); + println!("cargo:rerun-if-changed=../../.git/HEAD"); } diff --git a/crates/preloop-cli/src/main.rs b/crates/preloop-cli/src/main.rs index 3d00e702..2acf66b0 100644 --- a/crates/preloop-cli/src/main.rs +++ b/crates/preloop-cli/src/main.rs @@ -764,7 +764,11 @@ async fn main() -> anyhow::Result<()> { // one underneath itself. match cli.command { Command::Version => { - println!("preloop {}", env!("CARGO_PKG_VERSION")); + println!( + "preloop {} ({})", + env!("CARGO_PKG_VERSION"), + env!("PRELOOP_BUILD_COMMIT") + ); return Ok(()); } Command::Serve(args) => return cmd_engine(args).await, diff --git a/crates/preloop-cli/src/update.rs b/crates/preloop-cli/src/update.rs index 48078bdd..63fcc0a5 100644 --- a/crates/preloop-cli/src/update.rs +++ b/crates/preloop-cli/src/update.rs @@ -153,20 +153,25 @@ pub(crate) async fn run(args: UpdateArgs) -> anyhow::Result<()> { None => bail!("release {} has no asset for {target}", release.tag_name), }; if remote_version == current_version { - // The version string is self-reported and can lie: a source build or - // a tampered binary claims the release version while its bytes - // differ, and a version-only gate then declares it up to date - // forever (this is how the v0.30.2 deaf-runner fix never reached - // production). Verify the installed binary against the checksummed - // release asset and reinstall on mismatch. - match check_same_version_content(&client, &selected).await { - Ok(ContentCheck::Matches) => { + // The version string is self-reported and can lie: a source build + // claims the release version while it predates the release commit, + // and a version-only gate then declares it up to date forever (this + // is how the v0.30.2 deaf-runner fix never reached production). But + // byte-comparing against the release asset has the opposite failure: + // a source build from *newer* main also reports the same version + // (no bump between the tag and HEAD) and would be clobbered with the + // stale release binary. The commit is the monotonic signal the + // version string cannot carry: reinstall only when the release + // commit is an ancestor of the installed one (the release is + // strictly newer); keep anything newer or unverifiable. + match resolve_same_version(&client, &repository, &release.tag_name, &selected).await { + Ok(SameVersionDecision::UpToDate) => { println!("preloop {} is already up to date", current_version); return Ok(()); } - Ok(ContentCheck::Drift(staged)) => { + Ok(SameVersionDecision::Reinstall(staged)) => { println!( - "preloop {} does not match release {}; {} ({target})", + "preloop {} is older than release {}; {} ({target})", current_version, release.tag_name, if args.check { @@ -190,7 +195,9 @@ pub(crate) async fn run(args: UpdateArgs) -> anyhow::Result<()> { } Err(error) => { // A transient failure to fetch or verify the asset must not - // fail the hourly update timer; the next run retries. + // fail the hourly update timer; the next run retries. Keep + // what is installed: an unverifiable comparison must never + // clobber a possibly-newer build. println!( "warning: could not verify the installed binary against release {}: {error:#}", release.tag_name @@ -722,37 +729,89 @@ async fn stage_release( }) } -enum ContentCheck { - Matches, - Drift(StagedRelease), +enum SameVersionDecision { + /// Installed binary is the same commit as, or newer than, the release. + UpToDate, + /// The release commit is an ancestor of the installed one; reinstall. + Reinstall(StagedRelease), } -/// Compare the installed binary against the checksummed release asset. -async fn check_same_version_content( +/// Resolve an equal-version install by comparing the commit the installed +/// binary was built from against the release tag's commit. +/// +/// The version string is the only thing the two share, so it cannot decide +/// this case. The embedded build commit is monotonic: a source build from +/// newer main is a *descendant* of the release commit (keep it — it carries +/// fixes the release does not), while a source build that predates the +/// release tag is an *ancestor* (reinstall — this is the v0.30.2 deaf-runner +/// case where the fix never reached production). Anything unverifiable +/// (no embedded commit, diverged history, compare API failure) is kept: +/// clobbering a possibly-newer build on a heuristic is worse than one +/// extra hourly poll. +async fn resolve_same_version( client: &Client, + repository: &str, + release_tag: &str, selected: &SelectedAsset<'_>, -) -> anyhow::Result { - let staged = stage_release(client, selected).await?; - - let installed = std::env::current_exe().context("locate running preloop executable")?; - // macOS installs are launched through the `preloop` symlink into - // `/bin/preloop`; canonicalize so a future compare of paths - // (and anyone reading this) sees the real file. - let installed = - fs::canonicalize(&installed).with_context(|| format!("resolve {}", installed.display()))?; - if installed_binary_matches(&installed, &staged.binary_path)? { - Ok(ContentCheck::Matches) - } else { - Ok(ContentCheck::Drift(staged)) +) -> anyhow::Result { + let installed_commit = env!("PRELOOP_BUILD_COMMIT"); + if installed_commit == "unknown" { + println!( + "installed binary has no embedded build commit; keeping it (cannot verify against {} {release_tag})", + env!("CARGO_PKG_VERSION") + ); + return Ok(SameVersionDecision::UpToDate); + } + + let url = format!( + "https://api.github.com/repos/{repository}/compare/{installed_commit}...{release_tag}" + ); + let response = client + .get(&url) + .send() + .await + .with_context(|| format!("poll GitHub compare API: {url}"))?; + let status = response.status(); + if !status.is_success() { + bail!("GitHub compare API returned {status} for {url}"); + } + #[derive(Deserialize)] + struct Compare { + status: String, + } + let compare: Compare = response + .json() + .await + .with_context(|| format!("decode compare response from {url}"))?; + match same_version_decision(compare.status.as_str()) { + SameVersionOutcome::ReleaseNewer => { + let staged = stage_release(client, selected).await?; + Ok(SameVersionDecision::Reinstall(staged)) + } + SameVersionOutcome::KeepInstalled => Ok(SameVersionDecision::UpToDate), } } -/// Content comparison behind the same-version check: `true` only when the -/// installed binary is byte-identical to the release binary. A missing or -/// unreadable file is an `Err`, never a silent `true` — the caller treats -/// "unknown" as "keep what is installed and retry later", not "matches". -fn installed_binary_matches(installed: &Path, release_binary: &Path) -> anyhow::Result { - Ok(sha256_file(installed)? == sha256_file(release_binary)?) +/// Map the GitHub compare API's `status` to a same-version decision. +/// +/// The endpoint reports from the perspective of `head` (the release tag): +/// `ahead` means the release is strictly ahead of the installed commit — +/// reinstall. `behind`/`identical` mean the installed commit is at or +/// beyond the release — keep. Anything else (a `diverged` history, or an +/// unexpected status) keeps what is installed: never clobber a real build +/// over an ambiguous comparison. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum SameVersionOutcome { + ReleaseNewer, + KeepInstalled, +} + +fn same_version_decision(compare_status: &str) -> SameVersionOutcome { + match compare_status { + "ahead" => SameVersionOutcome::ReleaseNewer, + "behind" | "identical" => SameVersionOutcome::KeepInstalled, + _ => SameVersionOutcome::KeepInstalled, + } } fn extract_binary(archive_path: &Path, destination: &Path) -> anyhow::Result<()> { @@ -995,6 +1054,46 @@ mod tests { ))); } + /// The release commit is strictly ahead of the installed commit (a + /// source build older than the tag — the v0.30.2 deaf-runner case): + /// reinstall so the fix that never reached production finally does. + #[test] + fn same_version_release_ahead_reinstalls() { + assert_eq!( + same_version_decision("ahead"), + SameVersionOutcome::ReleaseNewer + ); + } + + /// Installed commit is at or beyond the release (a source build from + /// newer main with no version bump — the production regression this + /// replaces the byte compare for): keep the newer build. + #[test] + fn same_version_installed_at_or_ahead_keeps() { + assert_eq!( + same_version_decision("behind"), + SameVersionOutcome::KeepInstalled + ); + assert_eq!( + same_version_decision("identical"), + SameVersionOutcome::KeepInstalled + ); + } + + /// A diverged history (or any unexpected status) must keep what is + /// installed: clobbering a real build over an ambiguous comparison is + /// the failure mode we are removing. + #[test] + fn same_version_diverged_or_unknown_keeps() { + for status in ["diverged", "unknown", "unexpected"] { + assert_eq!( + same_version_decision(status), + SameVersionOutcome::KeepInstalled, + "status {status:?} must keep installed" + ); + } + } + #[cfg(unix)] #[tokio::test] async fn probe_detects_mount_socket_in_help_text() { @@ -1362,74 +1461,6 @@ mod tests { assert_eq!(std::fs::read(output_path).expect("binary"), contents); } - #[test] - fn content_check_detects_same_version_drift() { - // The v0.30.2 incident: a locally built binary claimed the release - // version string, so the version-only gate declared it up to date - // and the shipped fix never installed. The same-version check must - // compare bytes, not versions: identical content matches, drifted - // content (same claimed version) does not, and an unreadable file is - // an error rather than a silent match. - let temp = tempfile::tempdir().expect("staging directory"); - let installed = temp.path().join("installed"); - let release = temp.path().join("release"); - std::fs::write(&installed, b"installed-build").unwrap(); - std::fs::write(&release, b"installed-build").unwrap(); - assert!( - installed_binary_matches(&installed, &release).expect("both files readable"), - "byte-identical binaries must match" - ); - std::fs::write(&release, b"release-build").unwrap(); - assert!( - !installed_binary_matches(&installed, &release).expect("both files readable"), - "drifted content at the same version must be detected" - ); - assert!( - installed_binary_matches(&installed, &temp.path().join("missing")).is_err(), - "an unreadable binary must not be reported as matching" - ); - } - - #[test] - fn extract_then_content_check_rejects_a_tampered_archive_payload() { - use flate2::write::GzEncoder; - use flate2::Compression; - - let temp = tempfile::tempdir().expect("staging directory"); - let archive_path = temp.path().join("preloop-cli-aarch64-apple-darwin.tar.gz"); - let file = std::fs::File::create(&archive_path).expect("archive"); - let encoder = GzEncoder::new(file, Compression::default()); - let mut builder = tar::Builder::new(encoder); - // Same archive layout, different payload bytes than the "installed" - // binary that claims the same version. - let payload = b"drifted-release-payload"; - let mut header = tar::Header::new_gnu(); - header.set_size(payload.len() as u64); - header.set_mode(0o755); - header.set_cksum(); - builder - .append_data( - &mut header, - "preloop-cli-aarch64-apple-darwin/preloop", - &payload[..], - ) - .expect("binary entry"); - builder - .into_inner() - .expect("gzip stream") - .finish() - .expect("archive"); - - let installed = temp.path().join("installed"); - std::fs::write(&installed, b"local-build-claiming-same-version").unwrap(); - let extracted = temp.path().join(binary_name()); - extract_binary(&archive_path, &extracted).expect("extract binary"); - assert!( - !installed_binary_matches(&installed, &extracted).expect("both files readable"), - "a drifted payload at the same version must trigger reinstall" - ); - } - #[test] fn safe_asset_filename_rejects_traversal_and_absolute_paths() { assert!(safe_asset_filename("preloop-v0.30.2-aarch64-apple-darwin.tar.gz").is_ok()); From a77c1764d5e9e153574e3c5792b654a9f032f6f9 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 19:50:35 -0400 Subject: [PATCH 06/12] fix: address review findings on pool recovery Broker token recovery: - Parse the submission trust tier as a JSON string value. The tier is stored as plain kebab-case ('untrusted-fork-pull-request'); from_str rejected it and job_authorization treated the job as trusted, granting a fork job broader permissions and the PAT fallback. - Derive the job's declared permissions from the persisted message's system.github.token.permissions wire variable instead of the webhook event payload's workflow_job key (absent for push/PR/dispatch), which fell back to the broad default and granted scopes the workflow withheld. - Apply the effective-permissions merge on the recovered mint path so the runner's GITHUB_TOKEN Permissions group never overstates the token. Orchestrator golden provenance: - golden_is_packed now matches only the plain packed golden; fingerprint- suffixed goldens are env-baked (prepare_golden_for_env) and their forks do not inherit the baseline, so they must install it per fork. - The direct-create-from-pack fallback is gated on the plain packed golden; an env-golden fork failure boots the job's own environment, never the default OS. Covers every fallback branch at the create site. Updater: - Same-version reinstall decision no longer stages the release during --check; staging happens after the check guard and lock acquisition. - Compare URL derives from the configured releases API base (GitHub Enterprise / PRELOOP_RELEASES_API) instead of hard-coded api.github.com. Adds tests: env-golden fallback boots the job image, compare-URL derivation from configured base, and the existing decision tests. --- crates/preloop-cli/src/update.rs | 60 ++++++++++++++---- crates/preloop-orchestrator/src/lib.rs | 74 ++++++++++++++++++++-- crates/preloop-runner-server/src/broker.rs | 64 +++++++++++++++---- 3 files changed, 167 insertions(+), 31 deletions(-) diff --git a/crates/preloop-cli/src/update.rs b/crates/preloop-cli/src/update.rs index 63fcc0a5..5176275a 100644 --- a/crates/preloop-cli/src/update.rs +++ b/crates/preloop-cli/src/update.rs @@ -164,12 +164,12 @@ pub(crate) async fn run(args: UpdateArgs) -> anyhow::Result<()> { // version string cannot carry: reinstall only when the release // commit is an ancestor of the installed one (the release is // strictly newer); keep anything newer or unverifiable. - match resolve_same_version(&client, &repository, &release.tag_name, &selected).await { + match resolve_same_version(&client, &api_url, &release.tag_name).await { Ok(SameVersionDecision::UpToDate) => { println!("preloop {} is already up to date", current_version); return Ok(()); } - Ok(SameVersionDecision::Reinstall(staged)) => { + Ok(SameVersionDecision::Reinstall) => { println!( "preloop {} is older than release {}; {} ({target})", current_version, @@ -183,6 +183,7 @@ pub(crate) async fn run(args: UpdateArgs) -> anyhow::Result<()> { if args.check { return Ok(()); } + let staged = stage_release(&client, &selected).await?; let lock_path = update_lock_path()?; let _lock = UpdateLock::acquire(&lock_path)?; let executable = @@ -733,7 +734,9 @@ enum SameVersionDecision { /// Installed binary is the same commit as, or newer than, the release. UpToDate, /// The release commit is an ancestor of the installed one; reinstall. - Reinstall(StagedRelease), + /// The release binary is staged by the caller after the `--check` guard, + /// so check-only runs never download the asset. + Reinstall, } /// Resolve an equal-version install by comparing the commit the installed @@ -750,9 +753,8 @@ enum SameVersionDecision { /// extra hourly poll. async fn resolve_same_version( client: &Client, - repository: &str, + api_url: &str, release_tag: &str, - selected: &SelectedAsset<'_>, ) -> anyhow::Result { let installed_commit = env!("PRELOOP_BUILD_COMMIT"); if installed_commit == "unknown" { @@ -763,9 +765,7 @@ async fn resolve_same_version( return Ok(SameVersionDecision::UpToDate); } - let url = format!( - "https://api.github.com/repos/{repository}/compare/{installed_commit}...{release_tag}" - ); + let url = compare_url(api_url, installed_commit, release_tag); let response = client .get(&url) .send() @@ -784,14 +784,26 @@ async fn resolve_same_version( .await .with_context(|| format!("decode compare response from {url}"))?; match same_version_decision(compare.status.as_str()) { - SameVersionOutcome::ReleaseNewer => { - let staged = stage_release(client, selected).await?; - Ok(SameVersionDecision::Reinstall(staged)) - } + SameVersionOutcome::ReleaseNewer => Ok(SameVersionDecision::Reinstall), SameVersionOutcome::KeepInstalled => Ok(SameVersionDecision::UpToDate), } } +/// Build the compare API URL from the configured releases API base. +/// +/// `/releases` (the endpoint `fetch_release` polls) becomes +/// `/compare/{installed}...{release}`. Deriving from the configured +/// base keeps GitHub Enterprise and `PRELOOP_RELEASES_API` overrides working +/// — a hard-coded api.github.com host would fail the comparison for those +/// setups and never reinstall an older build. +fn compare_url(api_url: &str, installed_commit: &str, release_tag: &str) -> String { + let base = api_url + .trim_end_matches('/') + .strip_suffix("/releases") + .unwrap_or(api_url.trim_end_matches('/')); + format!("{base}/compare/{installed_commit}...{release_tag}") +} + /// Map the GitHub compare API's `status` to a same-version decision. /// /// The endpoint reports from the perspective of `head` (the release tag): @@ -1094,6 +1106,30 @@ mod tests { } } + /// The compare endpoint must derive from the configured releases API + /// base, not a hard-coded github.com host — a GitHub Enterprise or + /// overridden endpoint would otherwise always fail the comparison and + /// never reinstall an older build. + #[test] + fn compare_url_derives_from_the_configured_api_base() { + for (api_url, expected_base) in [ + ( + "https://api.github.com/repos/preloopdev/preloop/releases", + "https://api.github.com/repos/preloopdev/preloop/compare", + ), + ( + "https://github.example.com/api/v3/repos/acme/preloop/releases", + "https://github.example.com/api/v3/repos/acme/preloop/compare", + ), + ] { + assert_eq!( + compare_url(api_url, "deadbeef", "v0.30.10"), + format!("{expected_base}/deadbeef...v0.30.10"), + "compare URL must use the configured API base: {api_url}" + ); + } + } + #[cfg(unix)] #[tokio::test] async fn probe_detects_mount_socket_in_help_text() { diff --git a/crates/preloop-orchestrator/src/lib.rs b/crates/preloop-orchestrator/src/lib.rs index 61d71c74..6f0154db 100644 --- a/crates/preloop-orchestrator/src/lib.rs +++ b/crates/preloop-orchestrator/src/lib.rs @@ -3620,11 +3620,14 @@ fn fork_base_unusable(error: &VmError) -> bool { /// Whether `golden` is a fork base this pool baked and manages. /// /// The plain form (`{prefix}-golden`) is the single golden baked for the -/// default environment at pool startup; the fingerprint-suffixed form -/// (`{prefix}-golden-{fingerprint12}`) is prepared on demand by `run_slot` -/// when a job asks for a non-default base image. Both carry a packed golden -/// artifact and both can lose their retained fork checkpoint, so the -/// re-arm / independent-create fallback applies to each. +/// default environment at pool startup from the packed artifact; the +/// fingerprint-suffixed form (`{prefix}-golden-{fingerprint12}`) is prepared +/// on demand by `run_slot` from the job's OCI image when a job asks for a +/// non-default base. Only the plain form is packed — the suffixed form is an +/// environment golden whose forks do not inherit the baked baseline. Both +/// can lose their retained fork checkpoint, so the re-arm / +/// independent-create fallback applies to each; callers must not conflate +/// "managed" with "packed". /// /// A name like `{prefix}-golden-environment` is deliberately NOT matched: /// that is a baked environment golden serving a different `runs-on` image, @@ -3816,7 +3819,17 @@ async fn provision_runner( // so an env-golden fork boots the bare stock base image. Install the // apt baseline and toolchains into the fork itself — it is the job's // single-use machine, so the writes persist for its lifetime. - let golden_is_packed = config.use_packed_artifact && managed_golden(config, golden); + // Only the plain `{prefix}-golden` fork base is created from the + // packed artifact (`prepare_packed_golden` at pool startup), whose + // rootfs already carries the apt baseline and toolchains that forks + // inherit. Fingerprint-suffixed goldens are baked by + // `prepare_golden_for_env` from the job's OCI image via guest exec, + // and SmolVM's forkable snapshot does NOT carry post-create exec + // writes into clones — so those forks must install the baseline + // themselves. Treating an env golden as packed skipped that install + // and provisioned runners without the curated baseline. + let golden_is_packed = config.use_packed_artifact + && golden.as_str() == format!("{}-golden", config.name_prefix); if golden_is_packed { // The pack carries the apt baseline, but not necessarily apt's // indices — restore them before any workflow apt-installs. A @@ -3875,7 +3888,19 @@ async fn provision_runner( ); } } else { - let uses_packed_artifact = direct_create_from_packed; + // The packed-artifact fallback is valid only for the plain packed + // golden: it boots the *default* OS image. A fingerprint-suffixed + // golden is an environment golden baked from the job's requested + // image, so every fork-failure fallback must boot that job's own + // environment instead — regardless of which branch failed the fork. + // When no golden was attempted (`golden` is None, the create-per- + // runner path), the packed artifact is the pool's normal image and + // stays as-is. + let golden_is_plain_packed = match golden { + Some(golden) => golden.as_str() == format!("{}-golden", config.name_prefix), + None => true, + }; + let uses_packed_artifact = direct_create_from_packed && golden_is_plain_packed; let pack = packed_golden_path(&config.artifact_payload()); let spec = MachineSpec { name: name.clone(), @@ -5446,6 +5471,41 @@ chmod +x "$destination/bin/node" ); } + /// A fingerprint-suffixed golden is an *environment* golden baked from + /// the job's requested image. When its fork fails and the pool falls + /// back to independent creation, the runner must boot that job's + /// environment — not the default packed artifact, which would run a + /// non-default `runs-on` job on the wrong operating system. + #[tokio::test] + async fn fingerprint_golden_fork_failure_falls_back_to_the_job_environment() { + let provider = + Arc::new(TestProvider::new(false, false, false, false, false).failing_fork()); + let config = packed_fork_config(); + let golden = MachineName::new("lifecycle-test-golden-3577f5d5a384").unwrap(); + let name = MachineName::new("lifecycle-test-0-9").unwrap(); + let env = test_runner_environment("mirror.gcr.io/library/ubuntu:22.04", Vec::new(), true); + + provision_runner( + &provider, + &config, + &name, + Some(&golden), + &Arc::new(KeyPool::new()), + &env, + ) + .await + .expect("an env-golden fork failure falls back to the job environment"); + + let created = provider + .created_image(&name) + .await + .expect("the fallback created the runner machine"); + assert_eq!( + created, "mirror.gcr.io/library/ubuntu:22.04", + "an env-golden fallback must boot the job's requested image, not the default pack" + ); + } + /// A spent base that still has live clones must NOT be re-armed: resuming /// it would corrupt the copy-on-write clones. The pool falls back to a /// full create instead. diff --git a/crates/preloop-runner-server/src/broker.rs b/crates/preloop-runner-server/src/broker.rs index e4a9d02c..d215c2a0 100644 --- a/crates/preloop-runner-server/src/broker.rs +++ b/crates/preloop-runner-server/src/broker.rs @@ -794,21 +794,42 @@ pub(crate) async fn broker_acquire_job( let run = record.and_then(|record| inner.runs.get(&record.run_id)); match (record, run) { (Some(record), Some(run)) => { + // The submission stores the tier as a plain kebab-case + // string (e.g. "untrusted-fork-pull-request"), not JSON. + // `from_str` expects JSON and would reject the bare + // string, yielding `None` — which `job_authorization` + // treats as trusted, silently un-restricting a fork + // job's token. Parse via a JSON string value so the + // kebab-case variant decodes. let tier = run.submission.trust_tier.as_deref().and_then(|tier| { - serde_json::from_str::(tier).ok() + serde_json::from_value(serde_json::Value::String(tier.to_owned())).ok() }); - let declared = run - .submission - .payload - .get("workflow_job") - .and_then(|job| job.get("permissions")) - .and_then(serde_json::Value::as_object) - .map(|permissions| { - permissions - .iter() - .map(|(k, v)| (k.clone(), v.as_str().unwrap_or("read").to_owned())) - .collect::>() + // The job's resolved permission set lives in the + // persisted message's `system.github.token.permissions` + // variable (PascalCase wire spelling) — the same + // variable the build path wrote from `JobPlan` + // permissions. The event payload's `workflow_job` key is + // absent for push/PR/dispatch events, so reading it there + // would fall back to the broad default and grant scopes + // the workflow withheld. Recover from the message + // instead, converting the wire spelling back to + // kebab-case for the token request. + let wire_permissions = message + .variables + .get("system.github.token.permissions") + .and_then(|variable| variable.value.as_deref()) + .and_then(|json| { + serde_json::from_str::>(json).ok() }); + // `system.github.token.permissions` carries the effective + // set (defaults substituted when nothing was declared). + // Passing it as the declared set is faithful: for a + // declared job it is exactly the job's set, and for an + // undeclared job `job_authorization` treats a set equal + // to the default identically to `None`. The fork case is + // safe too — the wire variable was restated to the fork + // profile at build, and clamping it again is idempotent. + let declared = wire_permissions.clone(); let policy = crate::events::trust_tier::job_authorization( tier, declared.as_ref(), @@ -866,6 +887,25 @@ pub(crate) async fn broker_acquire_job( "GITHUB_TOKEN".to_owned(), preloop_gha_protocol::azdo::VariableValue::secret(token.clone()), ); + // Restate what the token carries when the installation could + // not grant everything, mirroring the normal mint path: the + // recovered message's wire set is the requested set, and + // leaving it would print authority the token does not have. + if let Some(effective) = minted.effective_permissions { + let merged = merge_narrowed_wire_permissions( + message + .variables + .get("system.github.token.permissions") + .and_then(|variable| variable.value.as_deref()), + &effective, + ); + message.variables.insert( + "system.github.token.permissions".to_owned(), + preloop_gha_protocol::azdo::VariableValue::new( + preloop_gha_parser::job_builder::token_permissions_wire_json(&merged), + ), + ); + } match message.context_data.get_mut("github") { Some(preloop_gha_protocol::azdo::PipelineContextData::Dict(github)) => { github.insert( From 710cc75a30366309cfc1fe43321b17f6af36e156 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 20:22:55 -0400 Subject: [PATCH 07/12] =?UTF-8?q?fix:=20review=20round=202=20=E2=80=94=20w?= =?UTF-8?q?ire-scope=20keys=20in=20recovery,=20ref-tracked=20build=20commi?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - broker: the recovered token request derived declared permissions from the message's system.github.token.permissions wire variable, which spells scopes PascalCase ('PullRequests'); the installation-token mint expects kebab-case ('pull-requests'), so the re-derived request used invalid permission identities and could fail minting or fall back to the broad PAT. Convert every wire key to kebab-case before building the request. - build.rs: Cargo's rerun-if-changed tracked only .git/HEAD, whose contents ('ref: refs/heads/main') do not change when the branch advances, so an incremental rebuild kept embedding the previous commit and the updater could clobber a newer source build. Track the resolved ref (via git rev-parse --symbolic-full-name), packed-refs, and HEAD. --- crates/preloop-cli/build.rs | 28 +++++++++++++++++++++- crates/preloop-runner-server/src/broker.rs | 12 ++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/crates/preloop-cli/build.rs b/crates/preloop-cli/build.rs index 2dccc967..613fc6f4 100644 --- a/crates/preloop-cli/build.rs +++ b/crates/preloop-cli/build.rs @@ -65,5 +65,31 @@ fn main() { .filter(|sha| !sha.is_empty()) .unwrap_or_else(|| "unknown".to_owned()); println!("cargo:rustc-env=PRELOOP_BUILD_COMMIT={commit}"); - println!("cargo:rerun-if-changed=../../.git/HEAD"); + // Make Cargo rebuild this crate when the checked-out commit changes. + // `.git/HEAD` alone is not enough: on a branch checkout it contains the + // symbolic ref (`ref: refs/heads/main`) whose contents do not change + // when the branch advances, so an incremental build would keep embedding + // the previous commit — and the updater could then compare that stale + // commit and clobber a newer source build with the release binary. + // Track HEAD, the ref it points at, and packed-refs (the ref file may + // live there instead of under refs/), all resolved through git itself. + let mut ref_paths = vec![String::from("../../.git/HEAD")]; + if let Ok(output) = Command::new("git") + .args(["rev-parse", "--symbolic-full-name", "HEAD"]) + .current_dir(&manifest_dir) + .output() + { + if output.status.success() { + if let Ok(ref_name) = String::from_utf8(output.stdout) { + let ref_name = ref_name.trim(); + if let Some(short) = ref_name.strip_prefix("refs/") { + ref_paths.push(format!("../../.git/{short}")); + } + } + } + } + ref_paths.push(String::from("../../.git/packed-refs")); + for path in ref_paths { + println!("cargo:rerun-if-changed={path}"); + } } diff --git a/crates/preloop-runner-server/src/broker.rs b/crates/preloop-runner-server/src/broker.rs index d215c2a0..2dc7053e 100644 --- a/crates/preloop-runner-server/src/broker.rs +++ b/crates/preloop-runner-server/src/broker.rs @@ -821,6 +821,18 @@ pub(crate) async fn broker_acquire_job( .and_then(|json| { serde_json::from_str::>(json).ok() }); + // The wire variable spells scopes PascalCase + // ("PullRequests"); the installation-token request and + // `job_authorization` expect the workflow's kebab-case + // identities ("pull-requests"). Minting with PascalCase + // keys fails (or falls back to the broad PAT), so + // convert every key before building the request. + let wire_permissions = wire_permissions.map(|permissions| { + permissions + .into_iter() + .map(|(scope, level)| (wire_scope_to_kebab(&scope), level)) + .collect::>() + }); // `system.github.token.permissions` carries the effective // set (defaults substituted when nothing was declared). // Passing it as the declared set is faithful: for a From 42b770fc5220d0bed923f2435dbefd75188649a3 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 20:55:02 -0400 Subject: [PATCH 08/12] test(push): set git identity in the clone repo so the test is hermetic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit materialize_imports_tree_from_another_repository commits from repo B (the clone), but set the git identity only in repo A. A clone does not inherit the source repo's local config, so the commit failed with 'Author identity unknown' on hosts without a global git identity — the CI fork is one, so the rust job failed. Set the identity in B too; verified passing with GIT_CONFIG_GLOBAL=/dev/null (no host identity leaked in). --- crates/preloop-cli/src/push.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/preloop-cli/src/push.rs b/crates/preloop-cli/src/push.rs index 5f093d94..3127edc0 100644 --- a/crates/preloop-cli/src/push.rs +++ b/crates/preloop-cli/src/push.rs @@ -581,6 +581,12 @@ mod tests { b.path().to_str().unwrap(), ], ); + // The commit happens in B, and a clone does not inherit A's local + // config. Without this, the test only passes on hosts whose global + // git identity leaks into B (the CI fork has none, so it failed with + // "Author identity unknown"). + git(b.path(), &["config", "user.email", "test@example.com"]); + git(b.path(), &["config", "user.name", "Test"]); assert_eq!( git(b.path(), &["rev-parse", "HEAD"]), head, From 551c853aa39bcfe11063bb6a0eb75b6f6699fd51 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 21:13:02 -0400 Subject: [PATCH 09/12] ci: re-trigger with current merge ref From e5ea969327239eb7fb30557bc0e8f65ccf6d1a8a Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 22:27:14 -0400 Subject: [PATCH 10/12] feat(pool): bound on-demand fork concurrency by host memory (OOM guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The on-demand pool sized max_concurrent by CPU alone ((parallelism / cpus) - 1), so on the 6-core / 22 GiB production host with PRELOOP_RUNNER_MEMORY_MIB=8192 enough 8 GiB forks could run to exhaust RAM and OOM the whole control plane — the golden commits 8 GiB, every fork inherits that footprint and grows toward the ceiling while its job runs, and warm mode provisions a successor mid-job (size + 1 live VMs). on_demand_memory_cap: (host_total - golden - 2 GiB reserve) / runner_mib, floored at 1. Applied in both pool modes: - size=0 on-demand: max_concurrent = min(by_cpu, by_memory) - warm mode: warm_size = min(configured size, by_memory), logged when cut Unmeasurable hosts fall back to CPU-only sizing. Tests cover the production 22 GiB / 8 GiB case and the floor. Also adds docs/retries-and-pool-memory.md: the full retry/backoff map across server, pool, smolvm provider, runner, and client, plus the known gaps. --- crates/preloop-orchestrator/src/lib.rs | 128 ++++++++++++++++++++++- docs/retries-and-pool-memory.md | 137 +++++++++++++++++++++++++ 2 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 docs/retries-and-pool-memory.md diff --git a/crates/preloop-orchestrator/src/lib.rs b/crates/preloop-orchestrator/src/lib.rs index 6f0154db..8d70fb9a 100644 --- a/crates/preloop-orchestrator/src/lib.rs +++ b/crates/preloop-orchestrator/src/lib.rs @@ -264,6 +264,62 @@ fn relax_externals_permissions(externals: &Path) { #[cfg(not(unix))] fn relax_externals_permissions(_externals: &Path) {} +/// Total physical memory in MiB, or `None` when it cannot be determined. +/// +/// Used to bound on-demand fork concurrency so the pool never schedules +/// more runner VMs than the host can hold in RAM. `None` (an unreadable +/// `/proc/meminfo`, a non-Linux/non-macOS host) falls back to CPU-only +/// sizing rather than refusing to run. +#[cfg(target_os = "linux")] +fn host_memory_mib() -> Option { + let meminfo = std::fs::read_to_string("/proc/meminfo").ok()?; + let kib: u64 = meminfo.lines().find_map(|line| { + let line = line.trim(); + let rest = line.strip_prefix("MemTotal:")?; + rest.trim().strip_suffix(" kB")?.trim().parse().ok() + })?; + Some(kib / 1024) +} + +/// Total physical memory in MiB, or `None` when it cannot be determined. +#[cfg(target_os = "macos")] +fn host_memory_mib() -> Option { + let output = std::process::Command::new("sysctl") + .args(["-n", "hw.memsize"]) + .output() + .ok()?; + let bytes: u64 = String::from_utf8_lossy(&output.stdout) + .trim() + .parse() + .ok()?; + Some(bytes / (1024 * 1024)) +} + +/// Total physical memory in MiB, or `None` when it cannot be determined. +#[cfg(not(any(target_os = "linux", target_os = "macos")))] +fn host_memory_mib() -> Option { + None +} + +/// On-demand fork concurrency allowed by host memory alone. +/// +/// Every on-demand fork inherits the golden's committed footprint and grows +/// toward `runner_memory_mib` as its guest runs, so the pool must never +/// schedule more concurrent runners than `(host_total - golden - reserve) / +/// runner_ceiling` allows. The 2 GiB reserve keeps the control plane, OS, +/// and page cache alive — without it the host OOMs *after* the forks are up, +/// which is exactly the production failure this guards against. Floors at 1 +/// so a tiny host still runs a single job rather than refusing to work. +fn on_demand_memory_cap(host_total_mib: u64, runner_memory_mib: u64) -> usize { + const HOST_RESERVE_MIB: u64 = 2048; + // Both the golden and each runner use the same ceiling. A degenerate + // zero ceiling means "unbounded" would be unsafe to divide by, so treat + // it as 1; the config layer validates `memory_mib > 0` in practice. + let runner_mib = runner_memory_mib.max(1); + let golden_mib = runner_mib; + (host_total_mib.saturating_sub(golden_mib + HOST_RESERVE_MIB) / runner_mib).max(1) as usize +} + fn default_golden_url(release_version: &str) -> String { format!( "https://github.com/preloopdev/preloop/releases/download/v{release_version}/preloop-ubuntu-24.04-{}", @@ -2300,14 +2356,37 @@ impl RunnerPool

{ let building = Arc::new(AtomicUsize::new(0)); // On-demand mode: size=0 means no warm pool. Fork runners only when - // jobs arrive, capped by the host's CPU budget. + // jobs arrive, capped by the host's CPU and memory budget. if self.config.size == 0 { return self .run_on_demand(shutdown, golden_registry, idle, keys, building) .await; } - for slot in 0..self.config.size { + // Warm mode: cap the configured pool size by host memory as well as + // CPU. Each warm slot forks from the golden and inherits its + // committed footprint (growing toward `memory_mib` while a job + // runs), and a slot provisions its successor mid-job — so on a + // small host the configured size can still exhaust RAM. Sizing down + // at startup is safer than OOMing mid-run; `PRELOOP_RUNNER_POOL_SIZE` + // remains an explicit override that wins. + let warm_size = match host_memory_mib() { + Some(total) => self.config.size.min(on_demand_memory_cap( + total, + u64::from(self.config.memory_mib), + )), + None => self.config.size, + }; + if warm_size < self.config.size { + warn!( + configured = self.config.size, + warm_size, + memory_mib = self.config.memory_mib, + "reduced warm pool size to fit host memory" + ); + } + + for slot in 0..warm_size { let provider = self.provider.clone(); let config = self.config.clone(); let slot_shutdown = shutdown.child_token(); @@ -2380,7 +2459,21 @@ impl RunnerPool

{ // the fork back and spends the golden's retained checkpoint. let parallelism = std::thread::available_parallelism().map_or(2, |value| value.get()); let per_runner = usize::from(self.config.cpus.max(1)); - (parallelism / per_runner).saturating_sub(1).max(1) + let by_cpu = (parallelism / per_runner).saturating_sub(1).max(1); + // memory term: every on-demand fork inherits the golden's + // committed footprint and grows toward `memory_mib` as the guest + // runs. On a small host (the production 6-core/22 GiB machine) + // the CPU term alone allows enough 8 GiB forks to exhaust RAM + // and OOM the whole control plane. Reserve the golden's memory + // plus host headroom, then fit the remainder with per-runner + // ceilings. A host we cannot measure falls back to CPU-only. + match host_memory_mib() { + Some(total) => by_cpu.min(on_demand_memory_cap( + total, + u64::from(self.config.memory_mib), + )), + None => by_cpu, + } }; info!(max_concurrent, "on-demand runner pool (size=0)"); @@ -4302,6 +4395,35 @@ mod lifecycle_tests { use std::process::Command; use tokio::sync::Mutex; + /// The production scenario that OOMed: 22 GiB host, 8 GiB runners. + /// Golden (8 GiB) + 2 GiB reserve leaves 12 GiB → at most 1 concurrent + /// on-demand fork. CPU-only sizing allowed several and the host died. + #[test] + fn on_demand_memory_cap_fits_runners_after_golden_and_reserve() { + assert_eq!(on_demand_memory_cap(22 * 1024, 8 * 1024), 1); + assert_eq!(on_demand_memory_cap(64 * 1024, 8 * 1024), 6); + assert_eq!(on_demand_memory_cap(32 * 1024, 4 * 1024), 6); + } + + /// A host too small for even one runner past the golden still runs one + /// job (floor of 1) rather than refusing to work. + #[test] + fn on_demand_memory_cap_floors_at_one() { + assert_eq!(on_demand_memory_cap(8 * 1024, 8 * 1024), 1); + assert_eq!(on_demand_memory_cap(4 * 1024, 8 * 1024), 1); + } + + /// A zero runner ceiling (should not happen — config validates it) must + /// not divide by zero; it degrades to the 1 MiB floor and lets the host + /// run as many 1 MiB "runners" as fit. + #[test] + fn on_demand_memory_cap_handles_zero_runner_ceiling() { + assert_eq!( + on_demand_memory_cap(32 * 1024, 0), + (32 * 1024 - 2048 - 1) as usize + ); + } + #[derive(Debug)] struct TestProvider { machines: Mutex>, diff --git a/docs/retries-and-pool-memory.md b/docs/retries-and-pool-memory.md new file mode 100644 index 00000000..f24d3ea6 --- /dev/null +++ b/docs/retries-and-pool-memory.md @@ -0,0 +1,137 @@ +# Retry, backoff, and pool memory bounds + +How preloop retries failures, when it backs off, and how the runner pool +keeps itself inside the host's memory budget. Written after the 2026-08-20 +outage, where a 22 GiB production host OOMed because on-demand fork +concurrency was sized by CPU only. + +## Retry philosophy, in one line + +> **Server: retry-once-and-coordinate** — the control plane avoids in-process +> retry loops almost entirely, leaning on GitHub redelivery, runner +> long-polls (bounded by `waitSeconds`), and periodic reaper sweeps. +> **Runner: mirror the official runner** — bounded 3-attempt exponential HTTP +> retries, jittered session backoff (15–60 s) reset on success, bounded lease +> renewal; listener/session loops and the container-health poll retry forever +> by design (the guest is expected to recover). +> **Pool: retry-forever with exponential damping** — slot respawns back off +> 500 ms → 30 s cap with success reset, golden re-arm bounded (12 × 10 s) +> before fallback. + +## Control plane (`preloop-runner-server`) + +| Site | What | Policy | Bounded? | +|---|---|---|---| +| `github.rs` `resolve_check_run_token` | App check-run token mint, transient 422 | 2 attempts, 500 ms fixed | yes | +| `github_app.rs` `mint_for_repository` | Installation token mint, 422 ungranted scope | one re-mint with clamped permissions | yes | +| `store.rs` `checkpoint_wal` | SQLite `wal_checkpoint(TRUNCATE)` blocked | 10 × 10 ms (blocking sleep), then bail | yes (~100 ms) | +| `debug.rs` `pump_axum_ws_to_dap` | DAP bridge WebSocket connect | 50 × 200 ms (≤10 s) | yes | +| `snapshots.rs` `acquire_cache_lock` | Object-cache dir lock | 25 ms poll, stale >60 s force-removed, 10 s deadline | yes | +| `broker.rs` / `distributed_task.rs` | Job-claim long poll | `loop` until `wait` deadline (default 50 s), wakes on notify ≤3 s slices | yes | +| `debug_sessions.rs` `poll_verdict` / `agent_events` | Worker verdict / events long poll | `loop` until 25 s cap (`VERDICT_POLL_MAX`) | yes | +| `bootstrap.rs` `run_background_reaper` | Stalled job/session sweep | 10 s interval, process lifetime | wall-time unbounded, periodic | +| `actions.rs` / `state.rs` | Action ref → SHA resolution | negative cached 60 s, positive 300 s; re-attempt after TTL | TTL-bounded | +| `broker.rs` `broker_acquire_job` | Dispatch token mint refusal (error policy) | **deliberately no retry** — config fault, job failed terminally | n/a | + +GitHub webhooks are **not retried in-process**: the reservation is released +on failure so GitHub's own redelivery is accepted (dedup window 300 s). + +## Pool (`preloop-orchestrator`) + +| Site | What | Policy | Bounded? | +|---|---|---|---| +| `run_on_demand` slot supervisor | Re-spawn failed slots | exp 500 ms → 30 s cap, reset to 0 on success | attempts unbounded, sleep ≤30 s | +| `run_on_demand_slot` provision failure | Fork/create failed | fixed 500 ms + continue; no counter | **unbounded** (tight-ish) | +| `provision_runner` golden re-arm | Spent checkpoint after clone drain | 12 × 10 s (`GOLDEN_DRAIN_PROBE_DELAY`), then direct OCI create | yes (~2 min) | +| `await_guest_ready` | Guest agent readiness probe | 25 ms poll, 30 s deadline | yes | +| golden download | Release/OCI asset | single attempt, 1 h timeout, then local-build fallback | n/a | +| `preload_images` / `docker_start_command` | dockerd readiness / start | shell polls (30 s / 10 s+5 s), start retried once | yes | + +## SmolVM provider (`preloop-vm`) + +| Site | What | Policy | Bounded? | +|---|---|---|---| +| `delete` | `smolvm machine delete` | 3 attempts: "directory not empty" 100 ms, "database is locked" 500 ms; "not found" = success | yes | + +fork / create / exec / start / stop have **no retry** — errors propagate to +the orchestrator, which retries at the slot level. + +## Runner (`preloop-runner`) + +| Site | What | Policy | Bounded? | +|---|---|---|---| +| `client/http.rs` POST/PUT | AzDO/broker JSON, log append | 3 attempts, exp 2 s / 4 s, transient (5xx / network) only | yes | +| `client/http.rs` `SessionBackoff` | Listener reconnect | jittered: ≤5 errors [15,30) s, then [30,60) s; reset on success | unbounded attempts, long sleeps | +| `broker_listener.rs` `run_broker_loop` | Session create + message poll | SessionBackoff; 409 retriable; OAuth linear `min(n*5,60)` s; deprecated → exit | unbounded (until shutdown) | +| `message_listener.rs` | Session create + poll (classic) | conflict: 8 × 30 s (~4 min); transient: 30 s fixed, no cap | conflict yes, transient **unbounded** | +| `container_ops.rs` `wait_for_services_healthy` | Docker service health poll | exp 2 → 32 s (official GetExponentialBackoff) | **unbounded attempts** | +| `container_ops.rs` `docker_registry_login` | `docker login` | 3 attempts, 5 s / 10 s | yes | +| `debug_pause.rs` `await_verdict` | Debug verdict poll | fixed 2 s; **infinite by design** (server suspends timeout) | unbounded by design | +| `live_logs.rs` send / connect | Live-log WebSocket | 3 attempts, random 100–500 ms backoff, 30 s connect timeout | yes | +| `steps_runner.rs` `run_steps` | Debug `:retry` verdicts | capped at `MAX_DEBUG_ATTEMPTS` = 25, then fail | yes | +| `job_runner.rs` `first_renew_gate` | First `renewjob` | 5 retries, random 1–10 s; 404 → Abandoned | yes | +| `job_runner.rs` renew loop | Lease renewal (every 60 s) | random 5–15 s (first 5), 15–30 s after; lease expired `LockedUntil + 5 min`; 401 → re-acquire once; 404 → cancel | yes (lease window) | +| `reporting.rs` `flush_step_updates` | Timeline publish | failed → requeue, retried every 500 ms drain tick | **unbounded attempts** | +| `control_bridge.rs` | TCP splice to upstream | bridge never exits; runner's poll loop retries forever through it | architecture-level | + +## Runner client (`preloop-runner-client`) + +No retry loops — single-shot submits with a reqwest timeout. + +## Gaps worth knowing + +1. **`run_on_demand_slot` provision failure retries at fixed 500 ms, unbounded.** + The slot-supervisor exponential backoff above it does **not** damp this + inner loop — a broken smolvm spins ~2 attempts/s. Not the OOM cause, but + noisy; a per-slot attempt counter with escalation would be an improvement. +2. **`wait_for_services_healthy` polls forever** (exp 2→32 s, no cap). Only + the outer job timeout ends it. Matches the official runner, so changing + it needs a fidelity note. +3. **`flush_step_updates` retries every 500 ms forever** on a permanently + failing publish endpoint. Bounded CPU, but unbounded wall time. +4. **`reporting.rs` requeue** is retry-by-drain-tick; harmless but + unbounded. + +## Pool memory bounds (the OOM guard) + +Before 2026-08-20, on-demand fork concurrency was sized **by CPU only**: + +``` +(available_parallelism / cpus_per_runner) - 1 // floor 1 +``` + +On the 6-core / 22 GiB production host with `PRELOOP_RUNNER_MEMORY_MIB=8192`, +that allowed enough 8 GiB forks to exhaust RAM and OOM the control plane — +the golden alone commits 8 GiB, every fork inherits that footprint and grows +toward the ceiling while its job runs, and warm mode provisions a successor +mid-job (so `size + 1` VMs are live). + +The guard, `on_demand_memory_cap`: + +``` +runner_mib = memory_mib.max(1) +golden_mib = runner_mib +by_memory = (host_total - golden_mib - 2048 MiB reserve) / runner_mib // floor 1 +max_concurrent = min(cpu_term, by_memory) +``` + +- The 2 GiB reserve keeps the control plane, OS, and page cache alive — + without it the host OOMs *after* the forks are up. +- Applied in **both** pool modes: + - size=0 on-demand: `max_concurrent = min(by_cpu, by_memory)` + - warm mode: `warm_size = min(configured size, by_memory)`, logged when + reduced. `PRELOOP_RUNNER_POOL_SIZE` still wins as an explicit override + only up to the memory cap — the cap is a safety floor, not a knob. +- A host whose memory can't be read (`/proc/meminfo` unavailable, non-Unix) + falls back to CPU-only sizing rather than refusing to run. +- Floors at 1 so a tiny host still runs a single job. + +### Test vectors (`on_demand_memory_cap`) + +| Host | Runner ceiling | Cap | +|---|---|---| +| 22 GiB | 8 GiB | 1 (production case) | +| 64 GiB | 8 GiB | 6 | +| 32 GiB | 4 GiB | 6 | +| 8 GiB | 8 GiB | 1 (floor) | +| 4 GiB | 8 GiB | 1 (floor) | From 7df1e53f10cfb9e5055087935a59c8adf9231929 Mon Sep 17 00:00:00 2001 From: Bill Date: Thu, 20 Aug 2026 23:17:52 -0400 Subject: [PATCH 11/12] feat(pool): exponential backoff when draining live clones before golden re-arm A live clone running a long job blocked golden re-arm; the drain loop gave up after a fixed 12 x 10s and every queued job fell back to slow direct creation (~8 min) even though the fork path is ~0.5s once the golden resumes. Probe with exponential backoff instead: 10s -> 20s -> 40s -> 60s cap, with a 5-minute total budget before falling back. A clone that exits mid-drain is now caught and the golden re-arms, keeping the fast path. Adds a paused-clock regression test where the clone drains after 3 probes and the fork is retried successfully. --- crates/preloop-orchestrator/src/lib.rs | 108 ++++++++++++++++++++++--- docs/retries-and-pool-memory.md | 5 +- 2 files changed, 102 insertions(+), 11 deletions(-) diff --git a/crates/preloop-orchestrator/src/lib.rs b/crates/preloop-orchestrator/src/lib.rs index 8d70fb9a..f005a5f9 100644 --- a/crates/preloop-orchestrator/src/lib.rs +++ b/crates/preloop-orchestrator/src/lib.rs @@ -1418,6 +1418,15 @@ const GUEST_READY_TIMEOUT: Duration = Duration::from_secs(30); /// golden fork base. Bounded retries; the probe loop is exercised by tests /// under paused Tokio time, so this is the only knob the delay is tied to. const GOLDEN_DRAIN_PROBE_DELAY: Duration = Duration::from_secs(10); +/// Ceiling for the drain-probe backoff: probes start at +/// `GOLDEN_DRAIN_PROBE_DELAY` and double up to this cap, so a long-running +/// job's clone is re-checked every minute rather than every ten seconds. +const GOLDEN_DRAIN_PROBE_MAX: Duration = Duration::from_secs(60); +/// Total wall-clock budget for waiting on live clones to drain before +/// falling back to independent OCI creation. The fork path is orders of +/// magnitude faster than direct creation, so this is generous; a clone that +/// has not exited within it is unlikely to do so soon. +const GOLDEN_DRAIN_BUDGET: Duration = Duration::from_secs(300); /// Gap between guest readiness probes. const GUEST_READY_POLL: Duration = Duration::from_millis(25); @@ -3804,14 +3813,22 @@ async fn provision_runner( // A live clone (another runner forked from the // golden) blocks the re-freeze; those clones are // ephemeral and exit after their job. Wait for - // them to drain, then retry the re-arm a bounded - // number of times before falling back to direct - // creation (whose socket mount cannot serve the - // control transport, so the fallback usually - // fails registration anyway). + // them to drain, probing with exponential backoff + // so a long-running job does not force the slow + // direct-create path for every queued job in the + // meantime. The fork path is ~0.5 s vs ~8 min for + // independent creation, so waiting is worth it up + // to a generous total budget; only then fall back + // to direct creation (whose socket mount cannot + // serve the control transport, so the fallback + // usually fails registration anyway). let mut rearmed = false; - for attempt in 0..12 { - tokio::time::sleep(GOLDEN_DRAIN_PROBE_DELAY).await; + let mut probe_delay = GOLDEN_DRAIN_PROBE_DELAY; + let drain_deadline = tokio::time::Instant::now() + GOLDEN_DRAIN_BUDGET; + let mut attempt = 0_u32; + while tokio::time::Instant::now() < drain_deadline { + tokio::time::sleep(probe_delay).await; + attempt += 1; match provider.rearm_fork_base(golden, Some(name)).await { Ok(true) => { info!( @@ -3823,8 +3840,12 @@ async fn provision_runner( } Ok(false) => { // Live clones still hold the golden; - // keep probing until the bounded - // retries are exhausted. + // back off and probe again: the next + // probe costs little, and the clone + // may exit before the budget runs out. + probe_delay = probe_delay + .saturating_mul(2) + .min(GOLDEN_DRAIN_PROBE_MAX); } Err(drain_error) => { error!( @@ -4437,6 +4458,9 @@ mod lifecycle_tests { /// Report live clones to `rearm_fork_base`; true by default so a spent /// base with dependents is never re-armed in tests either. live_forks: Mutex, + /// Simulate a clone exiting mid-drain: flip `live_forks` off after + /// this many `rearm_fork_base` calls (0 = never). + drain_live_forks_after: Mutex, fail_start: bool, fail_install: bool, fail_configure: bool, @@ -4469,6 +4493,7 @@ mod lifecycle_tests { fork_base_busy: false, fail_fork_once_spent: Mutex::new(false), live_forks: Mutex::new(true), + drain_live_forks_after: Mutex::new(0), fail_start, fail_install, fail_configure, @@ -4508,6 +4533,13 @@ mod lifecycle_tests { self } + /// Simulate the last live clone exiting after `n` drain probes, so a + /// re-arm that keeps probing eventually succeeds. + fn drain_live_forks_after(mut self, n: u32) -> Self { + *self.drain_live_forks_after.get_mut() = n; + self + } + /// A provider whose guests lack `binary` until an install command for /// it runs — a pack baked without the workspace's toolchain. fn without_binary(binary: &'static str) -> Self { @@ -5184,6 +5216,13 @@ chmod +x "$destination/bin/node" if let Some(partial) = partial { self.delete(partial).await?; } + let mut drain_after = self.drain_live_forks_after.lock().await; + if *drain_after > 0 { + *drain_after -= 1; + if *drain_after == 0 { + *self.live_forks.lock().await = false; + } + } if *self.live_forks.lock().await { return Ok(false); } @@ -5672,6 +5711,57 @@ chmod +x "$destination/bin/node" ); } + /// A live clone that exits mid-drain must be re-armed once it is gone: + /// the drain loop keeps probing with backoff, and the golden resumes + /// serving forks instead of falling back to slow direct creation. + /// Paused time advances the probe sleeps instantly. + #[tokio::test(start_paused = true)] + async fn spent_fork_base_with_clone_that_drains_is_rearmed_and_retried() { + let provider = Arc::new( + TestProvider::new(false, false, false, false, false) + .with_live_forks(true) + .drain_live_forks_after(3) + .failing_fork_once_spent(), + ); + let config = packed_fork_config(); + let golden = MachineName::new("lifecycle-test-golden").unwrap(); + let name = MachineName::new("lifecycle-test-0-10").unwrap(); + + provision_runner( + &provider, + &config, + &name, + Some(&golden), + &Arc::new(KeyPool::new()), + &test_runner_environment(config.base_image.clone(), Vec::new(), true), + ) + .await + .expect("the re-armed golden serves the fork after the clone drains"); + + let events = provider.events().await; + let expected = [ + format!("fork:{}:{}", golden.as_str(), name.as_str()), + format!("rearm:{}", golden.as_str()), + format!("rearm:{}", golden.as_str()), + format!("rearm:{}", golden.as_str()), + format!("stop:{}", golden.as_str()), + format!("start:{}", golden.as_str()), + format!("fork:{}:{}", golden.as_str(), name.as_str()), + ]; + let mut cursor = 0; + for event in &expected { + let position = events[cursor..] + .iter() + .position(|seen| seen == event) + .expect("drain-and-re-arm sequence must include every step"); + cursor += position + 1; + } + assert!( + provider.has_machine(&name).await, + "the retried fork must leave the clone provisioned" + ); + } + /// The provider reports a live clone before invoking SmolVM for another /// plain fork. The orchestrator must neither re-arm the shared golden nor /// instantiate the packed payload beside that clone. diff --git a/docs/retries-and-pool-memory.md b/docs/retries-and-pool-memory.md index f24d3ea6..d81d8c13 100644 --- a/docs/retries-and-pool-memory.md +++ b/docs/retries-and-pool-memory.md @@ -15,7 +15,8 @@ concurrency was sized by CPU only. > renewal; listener/session loops and the container-health poll retry forever > by design (the guest is expected to recover). > **Pool: retry-forever with exponential damping** — slot respawns back off -> 500 ms → 30 s cap with success reset, golden re-arm bounded (12 × 10 s) +> 500 ms → 30 s cap with success reset, golden re-arm exponential (10 s → 60 s, +> ≤ 5 min budget) before fallback > before fallback. ## Control plane (`preloop-runner-server`) @@ -42,7 +43,7 @@ on failure so GitHub's own redelivery is accepted (dedup window 300 s). |---|---|---|---| | `run_on_demand` slot supervisor | Re-spawn failed slots | exp 500 ms → 30 s cap, reset to 0 on success | attempts unbounded, sleep ≤30 s | | `run_on_demand_slot` provision failure | Fork/create failed | fixed 500 ms + continue; no counter | **unbounded** (tight-ish) | -| `provision_runner` golden re-arm | Spent checkpoint after clone drain | 12 × 10 s (`GOLDEN_DRAIN_PROBE_DELAY`), then direct OCI create | yes (~2 min) | +| `provision_runner` golden re-arm | Spent checkpoint after clone drain | exponential: 10 s → 60 s cap, total 300 s budget, then direct OCI create | yes (≤5 min) | | `await_guest_ready` | Guest agent readiness probe | 25 ms poll, 30 s deadline | yes | | golden download | Release/OCI asset | single attempt, 1 h timeout, then local-build fallback | n/a | | `preload_images` / `docker_start_command` | dockerd readiness / start | shell polls (30 s / 10 s+5 s), start retried once | yes | From 2d26eb31273941d09686291c0ac7f4b682caba40 Mon Sep 17 00:00:00 2001 From: "macroscopeapp[bot]" <170038800+macroscopeapp[bot]@users.noreply.github.com> Date: Fri, 21 Aug 2026 08:56:36 -0400 Subject: [PATCH 12/12] Halve memory-derived warm pool size to prevent OOM during successor provisioning (#169) --- crates/preloop-orchestrator/src/lib.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/preloop-orchestrator/src/lib.rs b/crates/preloop-orchestrator/src/lib.rs index f005a5f9..06b5046e 100644 --- a/crates/preloop-orchestrator/src/lib.rs +++ b/crates/preloop-orchestrator/src/lib.rs @@ -2380,10 +2380,11 @@ impl RunnerPool

{ // at startup is safer than OOMing mid-run; `PRELOOP_RUNNER_POOL_SIZE` // remains an explicit override that wins. let warm_size = match host_memory_mib() { - Some(total) => self.config.size.min(on_demand_memory_cap( - total, - u64::from(self.config.memory_mib), - )), + Some(total) => self.config.size.min( + on_demand_memory_cap(total, u64::from(self.config.memory_mib)) + .saturating_div(2) + .max(1), + ), None => self.config.size, }; if warm_size < self.config.size {