From 5998891c8f402e98a66d50bc3c8fe9a4b211a7fe Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 19:32:40 -0400 Subject: [PATCH] Limit git exec spans PR to logging changes --- Cargo.lock | 2 - lib/components/fabro-agent/src/lib.rs | 5 +- lib/components/fabro-agent/src/sandbox.rs | 7 +- lib/components/fabro-github/Cargo.toml | 2 - lib/components/fabro-github/src/lib.rs | 2 - .../fabro-github/src/token_source.rs | 607 ------------ lib/components/fabro-sandbox/Cargo.toml | 6 +- .../fabro-sandbox/src/clone_retry.rs | 400 ++++++++ .../fabro-sandbox/src/daytona/mod.rs | 203 ++-- lib/components/fabro-sandbox/src/docker.rs | 172 ++-- lib/components/fabro-sandbox/src/git_retry.rs | 743 -------------- lib/components/fabro-sandbox/src/lib.rs | 18 +- lib/components/fabro-sandbox/src/local.rs | 23 +- .../fabro-sandbox/src/push_credentials.rs | 599 ------------ lib/components/fabro-sandbox/src/sandbox.rs | 925 +----------------- lib/components/fabro-workflow/src/error.rs | 42 +- lib/components/fabro-workflow/src/event.rs | 2 +- .../fabro-workflow/src/event/convert.rs | 161 --- .../fabro-workflow/src/event/events.rs | 8 +- .../fabro-workflow/src/handler/llm/acp.rs | 442 +-------- .../fabro-workflow/src/lifecycle/event.rs | 1 - .../fabro-workflow/src/lifecycle/git.rs | 85 +- .../fabro-workflow/src/pipeline/finalize.rs | 40 +- .../fabro-workflow/src/pipeline/initialize.rs | 27 +- .../fabro-workflow/src/pipeline/publish.rs | 231 +---- .../fabro-workflow/src/run_metadata.rs | 202 ++-- .../fabro-workflow/src/sandbox_git_runtime.rs | 23 +- .../fabro-types/src/run_event/misc.rs | 44 - .../fabro-types/src/run_event/mod.rs | 2 - 29 files changed, 774 insertions(+), 4250 deletions(-) delete mode 100644 lib/components/fabro-github/src/token_source.rs create mode 100644 lib/components/fabro-sandbox/src/clone_retry.rs delete mode 100644 lib/components/fabro-sandbox/src/git_retry.rs delete mode 100644 lib/components/fabro-sandbox/src/push_credentials.rs diff --git a/Cargo.lock b/Cargo.lock index 15bb36035..5d47c63c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2654,7 +2654,6 @@ name = "fabro-github" version = "0.331.0-nightly.0" dependencies = [ "anyhow", - "async-trait", "base64", "chrono", "fabro-http", @@ -2666,7 +2665,6 @@ dependencies = [ "jsonwebtoken", "serde", "serde_json", - "strum 0.28.0", "thiserror 2.0.18", "tokio", "tracing", diff --git a/lib/components/fabro-agent/src/lib.rs b/lib/components/fabro-agent/src/lib.rs index e9549a716..c9d7f2e3d 100644 --- a/lib/components/fabro-agent/src/lib.rs +++ b/lib/components/fabro-agent/src/lib.rs @@ -59,9 +59,8 @@ pub use question_tools::{ }; pub use sandbox::{ CommandOutputCallback, DirEntry, ExecResult, ExecStreamingRequest, ExecStreamingResult, - GrepOptions, RefreshOutcome, RemoteCredentialAction, Sandbox, SandboxEvent, - SandboxEventCallback, StderrCollector, StdioProcess, StdioProcessHandle, TokenProvenance, - TokenSnapshot, format_lines_numbered, shell_quote, + GrepOptions, RefreshOutcome, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, + StdioProcess, StdioProcessHandle, format_lines_numbered, shell_quote, }; pub use session::{ CompletionCoordinator, Session, SessionControlHandle, SessionInputTiming, diff --git a/lib/components/fabro-agent/src/sandbox.rs b/lib/components/fabro-agent/src/sandbox.rs index 2f1ade3a4..47fbf894c 100644 --- a/lib/components/fabro-agent/src/sandbox.rs +++ b/lib/components/fabro-agent/src/sandbox.rs @@ -3,8 +3,7 @@ // `crate::delegate_sandbox!` invocations continue to work. pub use fabro_sandbox::{ CommandOutputCallback, DirEntry, ExecResult, ExecStreamingRequest, ExecStreamingResult, - GrepOptions, RefreshOutcome, RemoteCredentialAction, Sandbox, SandboxEvent, - SandboxEventCallback, SandboxFile, StderrCollector, StdioProcess, StdioProcessHandle, - StdioProcessTermination, TokenProvenance, TokenSnapshot, WalkOptions, delegate_sandbox, - format_lines_numbered, shell_quote, + GrepOptions, RefreshOutcome, Sandbox, SandboxEvent, SandboxEventCallback, SandboxFile, + StderrCollector, StdioProcess, StdioProcessHandle, StdioProcessTermination, WalkOptions, + delegate_sandbox, format_lines_numbered, shell_quote, }; diff --git a/lib/components/fabro-github/Cargo.toml b/lib/components/fabro-github/Cargo.toml index 788c6efe1..9db73130e 100644 --- a/lib/components/fabro-github/Cargo.toml +++ b/lib/components/fabro-github/Cargo.toml @@ -14,9 +14,7 @@ workspace = true [dependencies] anyhow.workspace = true -async-trait.workspace = true serde.workspace = true -strum.workspace = true serde_json.workspace = true fabro-http.workspace = true fabro-redact.workspace = true diff --git a/lib/components/fabro-github/src/lib.rs b/lib/components/fabro-github/src/lib.rs index 9973c4377..fd4b98936 100644 --- a/lib/components/fabro-github/src/lib.rs +++ b/lib/components/fabro-github/src/lib.rs @@ -9,8 +9,6 @@ use fabro_types::settings::run::MergeStrategy; use serde::Deserialize; use tokio::process::Command; -pub mod token_source; - pub const GITHUB_API_BASE_URL: &str = "https://api.github.com"; /// Returns the GitHub API base URL, allowing override via `GITHUB_BASE_URL` env diff --git a/lib/components/fabro-github/src/token_source.rs b/lib/components/fabro-github/src/token_source.rs deleted file mode 100644 index 11743e077..000000000 --- a/lib/components/fabro-github/src/token_source.rs +++ /dev/null @@ -1,607 +0,0 @@ -//! Cached GitHub installation-token source. -//! -//! One [`InstallationTokenSource`] serves every GitHub-token consumer for an -//! origin repository — the clone-based sandbox providers and the run-metadata -//! writer share a single source, so "reuse a token until near expiry" is the -//! default behavior instead of a per-call-site special case. Reusing mature -//! tokens keeps consumers out of GitHub's token-replication lag window, where -//! a token minted milliseconds earlier is rejected with 404 "Repository not -//! found" or an authentication failure. -//! -//! The source also reports *provenance*: when it minted the token it returned, -//! and which mint generation it belongs to. Retry classification, logging, and -//! failure reports all read that one fact instead of threading booleans -//! through call stacks. - -use std::fmt; -use std::sync::Arc; -use std::time::Duration; - -use anyhow::Context as _; -use chrono::{DateTime, Utc}; -use tokio::sync::Mutex; - -use crate::{GitHubAppCredentials, GitHubCredentials, InstallationToken}; - -/// How long before expiry a cached installation token stops being reused. -/// -/// Must comfortably exceed the longest git operation that pins a resolved -/// token, so a token handed out just above the margin still outlives the -/// operation. GitHub App installation tokens live 60 minutes. -pub const REFRESH_MARGIN: Duration = Duration::from_mins(10); - -/// Where the token a resolve returned came from. -/// -/// Time metadata exists only for tokens this source minted. Static -/// credentials (a PAT, or a pre-minted installation token) carry no -/// `minted_at`, so token age is undefined for them and they are never -/// treated as freshly minted. -#[derive(Debug, Clone, Copy, PartialEq, Eq, strum::Display)] -#[strum(serialize_all = "snake_case")] -pub enum TokenProvenance { - /// This resolve minted the token. - Minted { - minted_at: DateTime, - expires_at: DateTime, - }, - /// This resolve returned a token minted by an earlier resolve. - Reused { - minted_at: DateTime, - expires_at: DateTime, - }, - /// A fixed credential the source cannot re-mint. - Static, -} - -/// Non-secret description of the token a resolve returned. Shared by the -/// source, refresh outcomes, logs, and events. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct TokenSnapshot { - /// Increments per mint; 0 for `Static`. - pub generation: u64, - pub provenance: TokenProvenance, -} - -impl TokenSnapshot { - #[must_use] - pub fn minted_at(&self) -> Option> { - match self.provenance { - TokenProvenance::Minted { minted_at, .. } - | TokenProvenance::Reused { minted_at, .. } => Some(minted_at), - TokenProvenance::Static => None, - } - } - - #[must_use] - pub fn expires_at(&self) -> Option> { - match self.provenance { - TokenProvenance::Minted { expires_at, .. } - | TokenProvenance::Reused { expires_at, .. } => Some(expires_at), - TokenProvenance::Static => None, - } - } - - /// Age of the token at `now`. `None` for static credentials, whose age is - /// undefined. - #[must_use] - pub fn age_at(&self, now: DateTime) -> Option { - let minted_at = self.minted_at()?; - Some((now - minted_at).to_std().unwrap_or(Duration::ZERO)) - } - - /// Age of the token in milliseconds, measured now. - #[must_use] - pub fn age_ms(&self) -> Option { - self.age_at(Utc::now()) - .map(|age| u64::try_from(age.as_millis()).unwrap_or(u64::MAX)) - } - - #[must_use] - pub fn is_static(&self) -> bool { - matches!(self.provenance, TokenProvenance::Static) - } -} - -/// A token secret that never appears in `Debug` output. Call -/// [`SecretString::expose`] at the point of use (URL embedding, git -/// credentials) — never in a log line. -#[derive(Clone)] -pub struct SecretString(String); - -impl SecretString { - #[must_use] - pub fn new(secret: String) -> Self { - Self(secret) - } - - #[must_use] - pub fn expose(&self) -> &str { - &self.0 - } -} - -impl fmt::Debug for SecretString { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str("SecretString()") - } -} - -/// A token handed out by [`InstallationTokenSource::resolve`]: the secret plus -/// its non-secret snapshot. Only the snapshot may cross logging or event -/// boundaries. -#[derive(Debug, Clone)] -pub struct ResolvedToken { - pub token: SecretString, - pub snapshot: TokenSnapshot, -} - -/// Mints installation tokens for [`InstallationTokenSource`]. Abstracted so -/// tests can script mint results without HTTP. -#[async_trait::async_trait] -pub trait InstallationTokenMinter: Send + Sync { - async fn mint(&self) -> anyhow::Result; -} - -/// Real minter backed by GitHub App credentials. -struct AppTokenMinter { - creds: GitHubAppCredentials, - http: fabro_http::HttpClient, - owner: String, - repo: String, - base_url: String, - permissions: serde_json::Value, -} - -#[async_trait::async_trait] -impl InstallationTokenMinter for AppTokenMinter { - async fn mint(&self) -> anyhow::Result { - self.creds - .mint_installation_token( - &self.http, - &self.owner, - &self.repo, - &self.base_url, - self.permissions.clone(), - None, - ) - .await - } -} - -/// A minted token plus the metadata the cache tracks for it. -struct CachedToken { - token: InstallationToken, - minted_at: DateTime, - generation: u64, -} - -impl CachedToken { - fn resolved(&self, provenance: TokenProvenance) -> ResolvedToken { - ResolvedToken { - token: SecretString::new(self.token.token.clone()), - snapshot: TokenSnapshot { - generation: self.generation, - provenance, - }, - } - } -} - -enum SourceState { - /// A fixed personal access token — no expiry metadata. - Pat(SecretString), - /// A pre-minted installation token — fixed, rejected client-side once - /// expired. - Installation(InstallationToken), - /// GitHub App credentials that mint installation tokens on demand. - /// - /// The async lock is held across the mint, making `resolve()` - /// single-flight: concurrent near-expiry callers wait and receive the - /// same generation instead of racing to mint. - App { - minter: Box, - cache: Mutex>, - }, -} - -/// Cached installation-token source for one origin repository. -/// -/// Static credentials pass through unchanged. App credentials mint through -/// the shared cache: a resolve reuses the cached token until it is within -/// [`REFRESH_MARGIN`] of expiry, then mints a new generation. -pub struct InstallationTokenSource { - /// `owner/repo`, for logs only. - repo: String, - state: SourceState, -} - -impl InstallationTokenSource { - /// Build a source for `creds` against the repository in `origin_url`. - /// - /// `permissions` scopes minted installation tokens; static credentials - /// pass through and ignore it. - pub fn for_origin( - creds: &GitHubCredentials, - origin_url: &str, - permissions: serde_json::Value, - ) -> anyhow::Result> { - let normalized = crate::normalize_repo_origin_url(origin_url); - let (owner, repo) = crate::parse_github_owner_repo(&normalized) - .context("parsing GitHub origin for token source")?; - let repo_display = format!("{owner}/{repo}"); - let state = match creds { - GitHubCredentials::Pat(token) => SourceState::Pat(SecretString::new(token.clone())), - GitHubCredentials::Installation(token) => SourceState::Installation(token.clone()), - GitHubCredentials::App(app) => { - let http = fabro_http::http_client() - .map_err(anyhow::Error::new) - .context("building HTTP client for token source")?; - SourceState::App { - minter: Box::new(AppTokenMinter { - creds: app.clone(), - http, - owner, - repo, - base_url: crate::github_api_base_url(), - permissions, - }), - cache: Mutex::new(None), - } - } - }; - Ok(Arc::new(Self { - repo: repo_display, - state, - })) - } - - /// Build a minting source over a custom minter. For tests. - #[must_use] - pub fn with_minter(repo: String, minter: Box) -> Arc { - Arc::new(Self { - repo, - state: SourceState::App { - minter, - cache: Mutex::new(None), - }, - }) - } - - /// Whether this source can mint new tokens (GitHub App credentials). - #[must_use] - pub fn mints_installation_tokens(&self) -> bool { - matches!(self.state, SourceState::App { .. }) - } - - /// Resolve a token, reusing the cached one until it nears expiry. - pub async fn resolve(&self) -> anyhow::Result { - match &self.state { - SourceState::Pat(_) | SourceState::Installation(_) => self.resolve_static(), - SourceState::App { minter, cache } => { - let mut cache = cache.lock().await; - // Re-check under the lock: a waiter queued behind a minter - // finds the fresh token here instead of minting again. - if let Some(cached) = cache.as_ref() { - if !cached.token.near_expiry(REFRESH_MARGIN) { - let resolved = cached.resolved(TokenProvenance::Reused { - minted_at: cached.minted_at, - expires_at: cached.token.expires_at, - }); - tracing::debug!( - repo = %self.repo, - generation = cached.generation, - expires_at = %cached.token.expires_at, - "Reusing cached GitHub installation token" - ); - return Ok(resolved); - } - } - self.mint_locked(minter.as_ref(), &mut cache).await - } - } - } - - /// Mint a fresh token for the first repository clone and seed the cache - /// with it. - /// - /// The clone deliberately never reuses a warm cache: retrying a clone with - /// the token minted for it is the established replication-lag recovery, - /// and reuse of older tokens for clones is a separate follow-up. Seeding - /// makes the clone token generation 1, so later refreshes reuse it until - /// it nears expiry. - pub async fn mint_for_clone(&self) -> anyhow::Result { - match &self.state { - SourceState::Pat(_) | SourceState::Installation(_) => self.resolve_static(), - SourceState::App { minter, cache } => { - let mut cache = cache.lock().await; - self.mint_locked(minter.as_ref(), &mut cache).await - } - } - } - - fn resolve_static(&self) -> anyhow::Result { - let secret = match &self.state { - SourceState::Pat(token) => token.clone(), - SourceState::Installation(token) => SecretString::new(token.valid_token()?.to_owned()), - SourceState::App { .. } => unreachable!("resolve_static called for App credentials"), - }; - Ok(ResolvedToken { - token: secret, - snapshot: TokenSnapshot { - generation: 0, - provenance: TokenProvenance::Static, - }, - }) - } - - async fn mint_locked( - &self, - minter: &dyn InstallationTokenMinter, - cache: &mut Option, - ) -> anyhow::Result { - let token = minter - .mint() - .await - .context("minting GitHub installation access token")?; - let generation = cache.as_ref().map_or(0, |cached| cached.generation) + 1; - let minted_at = Utc::now(); - tracing::info!( - repo = %self.repo, - generation, - expires_at = %token.expires_at, - "Minted GitHub installation token" - ); - let cached = CachedToken { - token, - minted_at, - generation, - }; - let resolved = cached.resolved(TokenProvenance::Minted { - minted_at, - expires_at: cached.token.expires_at, - }); - *cache = Some(cached); - Ok(resolved) - } -} - -#[cfg(test)] -mod tests { - use std::collections::VecDeque; - use std::sync::atomic::{AtomicUsize, Ordering}; - - use anyhow::anyhow; - - use super::*; - - enum MintAction { - Token(&'static str, DateTime), - Error(&'static str), - } - - struct MockMinter { - calls: AtomicUsize, - script: Mutex>, - } - - impl MockMinter { - fn new(script: Vec) -> Self { - Self { - calls: AtomicUsize::new(0), - script: Mutex::new(script.into()), - } - } - - fn calls(&self) -> usize { - self.calls.load(Ordering::SeqCst) - } - } - - #[async_trait::async_trait] - impl InstallationTokenMinter for MockMinter { - async fn mint(&self) -> anyhow::Result { - self.calls.fetch_add(1, Ordering::SeqCst); - match self.script.lock().await.pop_front().expect("mint script") { - MintAction::Token(token, expires_at) => Ok(InstallationToken { - token: token.to_string(), - expires_at, - }), - MintAction::Error(message) => Err(anyhow!(message)), - } - } - } - - struct SharedMinter(Arc); - - #[async_trait::async_trait] - impl InstallationTokenMinter for SharedMinter { - async fn mint(&self) -> anyhow::Result { - self.0.mint().await - } - } - - fn mintable(script: Vec) -> (Arc, Arc) { - let minter = Arc::new(MockMinter::new(script)); - let source = InstallationTokenSource::with_minter( - "owner/repo".to_string(), - Box::new(SharedMinter(Arc::clone(&minter))), - ); - (source, minter) - } - - #[tokio::test] - async fn pat_resolves_as_static_generation_zero() { - let source = InstallationTokenSource::for_origin( - &GitHubCredentials::Pat("ghp_pat".to_string()), - "https://github.com/owner/repo.git", - serde_json::json!({ "contents": "write" }), - ) - .unwrap(); - - let resolved = source.resolve().await.unwrap(); - assert_eq!(resolved.token.expose(), "ghp_pat"); - assert_eq!(resolved.snapshot.generation, 0); - assert!(resolved.snapshot.is_static()); - assert!(!source.mints_installation_tokens()); - } - - #[tokio::test] - async fn static_installation_token_resolves_until_expiry() { - let valid = InstallationTokenSource::for_origin( - &GitHubCredentials::Installation(InstallationToken { - token: "ghs_static".to_string(), - expires_at: Utc::now() + chrono::Duration::minutes(30), - }), - "https://github.com/owner/repo.git", - serde_json::json!({}), - ) - .unwrap(); - let resolved = valid.resolve().await.unwrap(); - assert_eq!(resolved.token.expose(), "ghs_static"); - assert!(resolved.snapshot.is_static()); - - let expired = InstallationTokenSource::for_origin( - &GitHubCredentials::Installation(InstallationToken { - token: "ghs_expired".to_string(), - expires_at: Utc::now() - chrono::Duration::seconds(1), - }), - "https://github.com/owner/repo.git", - serde_json::json!({}), - ) - .unwrap(); - assert!(expired.resolve().await.is_err()); - } - - #[tokio::test] - async fn resolve_reuses_cached_token_before_the_margin() { - let (source, minter) = mintable(vec![MintAction::Token( - "ghs_gen1", - Utc::now() + chrono::Duration::minutes(30), - )]); - - let first = source.resolve().await.unwrap(); - let second = source.resolve().await.unwrap(); - - assert_eq!(minter.calls(), 1); - assert_eq!(first.snapshot.generation, 1); - assert_eq!(second.snapshot.generation, 1); - assert!(matches!( - first.snapshot.provenance, - TokenProvenance::Minted { .. } - )); - assert!(matches!( - second.snapshot.provenance, - TokenProvenance::Reused { .. } - )); - assert_eq!(second.token.expose(), "ghs_gen1"); - } - - #[tokio::test] - async fn resolve_mints_a_new_generation_inside_the_margin() { - let (source, minter) = mintable(vec![ - // Expires inside REFRESH_MARGIN, so the second resolve re-mints. - MintAction::Token("ghs_gen1", Utc::now() + chrono::Duration::minutes(5)), - MintAction::Token("ghs_gen2", Utc::now() + chrono::Duration::minutes(60)), - ]); - - let first = source.resolve().await.unwrap(); - let second = source.resolve().await.unwrap(); - - assert_eq!(minter.calls(), 2); - assert_eq!(first.snapshot.generation, 1); - assert_eq!(second.snapshot.generation, 2); - assert!(matches!( - second.snapshot.provenance, - TokenProvenance::Minted { .. } - )); - assert_eq!(second.token.expose(), "ghs_gen2"); - } - - #[tokio::test] - async fn concurrent_resolves_share_one_generation() { - // Single mint in the script: a second mint would panic on an empty - // script, so success proves single-flight. - let (source, minter) = mintable(vec![MintAction::Token( - "ghs_gen1", - Utc::now() + chrono::Duration::minutes(60), - )]); - - let handles: Vec<_> = (0..8) - .map(|_| { - let source = Arc::clone(&source); - tokio::spawn(async move { source.resolve().await }) - }) - .collect(); - - for handle in handles { - let resolved = handle.await.unwrap().unwrap(); - assert_eq!(resolved.snapshot.generation, 1); - assert_eq!(resolved.token.expose(), "ghs_gen1"); - } - assert_eq!(minter.calls(), 1); - } - - #[tokio::test] - async fn mint_for_clone_always_mints_and_seeds_the_cache() { - let (source, minter) = mintable(vec![MintAction::Token( - "ghs_clone", - Utc::now() + chrono::Duration::minutes(60), - )]); - - let clone_token = source.mint_for_clone().await.unwrap(); - assert_eq!(clone_token.snapshot.generation, 1); - assert!(matches!( - clone_token.snapshot.provenance, - TokenProvenance::Minted { .. } - )); - - // A later resolve reuses the clone token instead of minting again. - let refreshed = source.resolve().await.unwrap(); - assert_eq!(refreshed.snapshot.generation, 1); - assert_eq!(refreshed.token.expose(), "ghs_clone"); - assert!(matches!( - refreshed.snapshot.provenance, - TokenProvenance::Reused { .. } - )); - assert_eq!(minter.calls(), 1); - } - - #[tokio::test] - async fn mint_failure_surfaces_with_context() { - let (source, _minter) = mintable(vec![MintAction::Error("mint failed")]); - - let err = format!("{:#}", source.resolve().await.unwrap_err()); - assert!(err.contains("mint failed"), "got: {err}"); - assert!( - err.contains("minting GitHub installation access token"), - "got: {err}" - ); - } - - #[test] - fn secret_string_debug_never_prints_the_secret() { - let secret = SecretString::new("ghs_super_secret".to_string()); - let rendered = format!("{secret:?}"); - assert!(!rendered.contains("ghs_super_secret"), "{rendered}"); - } - - #[test] - fn snapshot_age_is_defined_only_for_minted_tokens() { - let now = Utc::now(); - let minted = TokenSnapshot { - generation: 3, - provenance: TokenProvenance::Minted { - minted_at: now - chrono::Duration::seconds(42), - expires_at: now + chrono::Duration::minutes(60), - }, - }; - assert_eq!(minted.age_at(now), Some(Duration::from_secs(42))); - - let fixed = TokenSnapshot { - generation: 0, - provenance: TokenProvenance::Static, - }; - assert_eq!(fixed.age_at(now), None); - assert_eq!(fixed.expires_at(), None); - } -} diff --git a/lib/components/fabro-sandbox/Cargo.toml b/lib/components/fabro-sandbox/Cargo.toml index 7153e3ea1..908d010e6 100644 --- a/lib/components/fabro-sandbox/Cargo.toml +++ b/lib/components/fabro-sandbox/Cargo.toml @@ -9,8 +9,8 @@ description = "Sandbox trait and implementations for Fabro agent execution envir [features] default = ["local"] local = [] -docker = ["dep:bollard", "dep:tar"] -daytona = ["dep:daytona-sdk", "dep:daytona-api-client", "dep:git2", "dep:fabro-config", "dep:fabro-http", "dep:reqwest-middleware", "dep:rand", "dep:tokio-tungstenite", "dep:futures-util", "dep:rustls"] +docker = ["dep:bollard", "dep:tar", "dep:fabro-github"] +daytona = ["dep:daytona-sdk", "dep:daytona-api-client", "dep:git2", "dep:fabro-github", "dep:fabro-config", "dep:fabro-http", "dep:reqwest-middleware", "dep:rand", "dep:tokio-tungstenite", "dep:futures-util", "dep:rustls"] test-support = [] [lib] @@ -47,7 +47,7 @@ tar = { workspace = true, optional = true } # daytona fabro-config = { path = "../../foundation/fabro-config", optional = true } -fabro-github = { path = "../fabro-github" } +fabro-github = { path = "../fabro-github", optional = true } fabro-types = { path = "../../foundation/fabro-types" } chrono = { workspace = true } diff --git a/lib/components/fabro-sandbox/src/clone_retry.rs b/lib/components/fabro-sandbox/src/clone_retry.rs new file mode 100644 index 000000000..f0f3f0572 --- /dev/null +++ b/lib/components/fabro-sandbox/src/clone_retry.rs @@ -0,0 +1,400 @@ +//! Retry for the first repository clone in a clone-based sandbox. +//! +//! Clone-based providers can mint a GitHub App installation token and clone +//! with it immediately. GitHub can reject that first clone before the token is +//! available to the git endpoint. On a private repository, the rejection can +//! arrive as `Repository not found.` or an authentication failure. +//! +//! Only a token minted during the current clone operation makes those messages +//! safe to retry. Static PATs and pre-minted installation tokens fail fast. +//! +//! Retries reuse the same token on purpose. Replication of a given token only +//! makes progress, so each attempt strictly improves the odds, while re-minting +//! would restart the replication clock. + +use std::future::Future; +use std::time::Duration; + +use fabro_types::SandboxProviderKind; +use fabro_util::backoff::BackoffPolicy; +use tokio::time; + +/// Total clone attempts, including the first. +const MAX_ATTEMPTS: u32 = 3; + +/// Why a failed clone attempt is worth repeating. +#[derive(Clone, Copy, Debug, PartialEq, Eq, strum::Display)] +#[strum(serialize_all = "snake_case")] +pub(crate) enum CloneRetryReason { + /// A freshly minted installation token has not reached the GitHub edge + /// cache site serving this clone yet. + TokenReplication, + /// The clone failed on infrastructure, unrelated to credentials. + TransientInfra, +} + +/// What a clone failure message tells us about retry safety. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum CloneMessageClass { + Retry(CloneRetryReason), + Permanent, + Unknown, +} + +impl CloneMessageClass { + pub(crate) fn retry_reason(self) -> Option { + match self { + Self::Retry(reason) => Some(reason), + Self::Permanent | Self::Unknown => None, + } + } +} + +/// Message fragments that mean the clone failed on infrastructure. +/// +/// These are safe to retry whether or not the clone was authenticated. +const TRANSIENT_HINTS: &[&str] = &[ + "could not resolve host", + "temporary failure in name resolution", + "connection refused", + "connection reset", + "connection timed out", + "timed out", + "network is unreachable", + "no route to host", + "tls handshake", + "early eof", + "rpc failed", + "unexpected disconnect", + "the remote end hung up unexpectedly", + "index-pack failed", + "service unavailable", + "gateway timeout", + "too many requests", + "rate limit", +]; + +/// Message fragments GitHub uses when a token is not yet visible. +/// +/// Only meaningful when the clone carried credentials. The same lag surfaces as +/// 404 or as an auth failure depending on which endpoint answers first. +const TOKEN_REPLICATION_HINTS: &[&str] = &[ + "repository not found", + "authentication failed", + "invalid username or password", + "bad credentials", +]; + +/// Classify a failed clone by its rendered message. +/// +/// `token_was_freshly_minted` gates the token-replication reading. A static +/// credential cannot become valid during backoff, so auth failures for it are +/// permanent. +pub(crate) fn classify_message(message: &str, token_was_freshly_minted: bool) -> CloneMessageClass { + let lower = message.to_ascii_lowercase(); + + if TRANSIENT_HINTS.iter().any(|hint| lower.contains(hint)) { + return CloneMessageClass::Retry(CloneRetryReason::TransientInfra); + } + if TOKEN_REPLICATION_HINTS + .iter() + .any(|hint| lower.contains(hint)) + { + return if token_was_freshly_minted { + CloneMessageClass::Retry(CloneRetryReason::TokenReplication) + } else { + CloneMessageClass::Permanent + }; + } + let permanent = lower.contains("could not read username") + || lower.contains("terminal prompts disabled") + || lower.contains("permission denied") + || (lower.contains("permission to") && lower.contains("denied")) + || (lower.contains("destination path") && lower.contains("already exists")) + || (lower.contains("remote branch") && lower.contains("not found")); + if permanent { + return CloneMessageClass::Permanent; + } + CloneMessageClass::Unknown +} + +/// Backoff between clone attempts: 3s, then 9s. +/// +/// GitHub's guidance for token replication is to wait a few seconds and retry +/// with the same token. Sub-second delays land inside the same replication +/// window and spend an attempt for nothing. +fn backoff() -> BackoffPolicy { + BackoffPolicy { + initial_delay: Duration::from_secs(3), + factor: 3.0, + max_delay: Duration::from_secs(10), + jitter: false, + } +} + +/// Run a clone, repeating it while the failure looks transient. +/// +/// `attempt` receives the 1-based attempt number. `classify` decides whether an +/// error is worth repeating; `None` returns it to the caller untouched. When a +/// deadline is present, a retry starts only when its backoff fits before that +/// deadline. The final error is returned as-is. +pub(crate) async fn retry_clone( + provider: SandboxProviderKind, + deadline: Option, + mut attempt: Attempt, + classify: Classify, +) -> Result +where + Attempt: FnMut(u32) -> Fut, + Fut: Future>, + Classify: Fn(&E) -> Option, +{ + let backoff = backoff(); + + for attempt_number in 1..MAX_ATTEMPTS { + match attempt(attempt_number).await { + Ok(value) => return Ok(value), + Err(err) => { + let Some(reason) = classify(&err) else { + return Err(err); + }; + let delay = backoff.delay_for_attempt(attempt_number); + if deadline.is_some_and(|deadline| { + delay >= deadline.saturating_duration_since(time::Instant::now()) + }) { + return Err(err); + } + // The failure text can carry git stderr, so log the category + // rather than the message. The caller still reports the full + // error if the attempts run out. + tracing::warn!( + provider = %provider, + attempt = attempt_number, + max_attempts = MAX_ATTEMPTS, + reason = %reason, + delay_ms = u64::try_from(delay.as_millis()).unwrap_or(u64::MAX), + "Git clone failed, retrying" + ); + time::sleep(delay).await; + } + } + } + + attempt(MAX_ATTEMPTS).await +} + +#[cfg(test)] +mod tests { + use std::sync::Mutex; + + use super::*; + + /// Records the attempt numbers a closure was called with. + #[derive(Default)] + struct Attempts(Mutex>); + + impl Attempts { + fn record(&self, attempt: u32) { + self.0.lock().expect("attempt log mutex").push(attempt); + } + + fn recorded(&self) -> Vec { + self.0.lock().expect("attempt log mutex").clone() + } + } + + /// A classifier that treats every failure as worth repeating. + const ALWAYS_RETRY: fn(&String) -> Option = + |_| Some(CloneRetryReason::TokenReplication); + + #[test] + fn private_repo_not_found_after_a_successful_mint_is_a_replication_lag() { + assert_eq!( + classify_message("repository not found: Repository not found.", true), + CloneMessageClass::Retry(CloneRetryReason::TokenReplication) + ); + } + + #[test] + fn not_found_without_a_fresh_token_is_permanent() { + assert_eq!( + classify_message("repository not found: Repository not found.", false), + CloneMessageClass::Permanent + ); + } + + #[test] + fn auth_failure_with_a_fresh_token_is_a_replication_lag() { + assert_eq!( + classify_message( + "fatal: Authentication failed for 'https://github.com/owner/repo'", + true + ), + CloneMessageClass::Retry(CloneRetryReason::TokenReplication) + ); + assert_eq!( + classify_message( + "fatal: Authentication failed for 'https://github.com/owner/repo'", + false + ), + CloneMessageClass::Permanent + ); + } + + #[test] + fn infra_failures_retry_without_credentials() { + for message in [ + "fatal: unable to access: Could not resolve host: github.com", + "error: RPC failed; curl 56 recv failure", + "fatal: early EOF", + "Operation timed out", + ] { + assert_eq!( + classify_message(message, false), + CloneMessageClass::Retry(CloneRetryReason::TransientInfra), + "expected {message:?} to be transient" + ); + } + } + + #[test] + fn genuine_failures_are_not_retried() { + for message in [ + "fatal: could not read Username for 'https://github.com'", + "remote: Permission to owner/repo.git denied", + "fatal: destination path 'repo' already exists", + ] { + assert_eq!( + classify_message(message, true), + CloneMessageClass::Permanent, + "expected {message:?} to fail fast" + ); + } + } + + #[test] + fn unrecognized_failures_remain_unknown() { + assert_eq!( + classify_message("git clone stopped for an unexpected reason", true), + CloneMessageClass::Unknown + ); + } + + #[test] + fn backoff_waits_seconds_not_milliseconds() { + let backoff = backoff(); + assert_eq!(backoff.delay_for_attempt(1), Duration::from_secs(3)); + assert_eq!(backoff.delay_for_attempt(2), Duration::from_secs(9)); + } + + #[tokio::test(start_paused = true)] + async fn first_success_runs_one_attempt() { + let attempts = Attempts::default(); + + let result = retry_clone( + SandboxProviderKind::Docker, + None, + |attempt| { + attempts.record(attempt); + async move { Ok::<_, String>(attempt) } + }, + ALWAYS_RETRY, + ) + .await; + + assert_eq!(result, Ok(1)); + assert_eq!(attempts.recorded(), vec![1]); + } + + #[tokio::test(start_paused = true)] + async fn retries_until_a_later_attempt_succeeds() { + let attempts = Attempts::default(); + + let result = retry_clone( + SandboxProviderKind::Docker, + None, + |attempt| { + attempts.record(attempt); + async move { + if attempt < 3 { + Err("Repository not found.".to_string()) + } else { + Ok(attempt) + } + } + }, + ALWAYS_RETRY, + ) + .await; + + assert_eq!(result, Ok(3)); + assert_eq!(attempts.recorded(), vec![1, 2, 3]); + } + + #[tokio::test(start_paused = true)] + async fn exhausted_attempts_return_the_final_error() { + let attempts = Attempts::default(); + + let result = retry_clone( + SandboxProviderKind::Docker, + None, + |attempt| { + attempts.record(attempt); + async move { Err::<(), _>(format!("Repository not found. (attempt {attempt})")) } + }, + ALWAYS_RETRY, + ) + .await; + + assert_eq!( + result, + Err("Repository not found. (attempt 3)".to_string()), + "the caller should see the last failure, not the first" + ); + assert_eq!(attempts.recorded(), vec![1, 2, 3]); + } + + #[tokio::test(start_paused = true)] + async fn unretryable_failure_stops_immediately() { + let attempts = Attempts::default(); + + let result = retry_clone( + SandboxProviderKind::Docker, + None, + |attempt| { + attempts.record(attempt); + async move { Err::<(), _>("permission denied".to_string()) } + }, + |_: &String| None, + ) + .await; + + assert_eq!(result, Err("permission denied".to_string())); + assert_eq!( + attempts.recorded(), + vec![1], + "a deterministic failure should not wait out the backoff" + ); + } + + #[tokio::test(start_paused = true)] + async fn deadline_stops_retry_when_backoff_does_not_fit() { + let attempts = Attempts::default(); + let deadline = time::Instant::now() + Duration::from_secs(2); + + let result = retry_clone( + SandboxProviderKind::Docker, + Some(deadline), + |attempt| { + attempts.record(attempt); + async move { Err::<(), _>("temporary failure".to_string()) } + }, + ALWAYS_RETRY, + ) + .await; + + assert_eq!(result, Err("temporary failure".to_string())); + assert_eq!(attempts.recorded(), vec![1]); + assert_eq!(time::Instant::now() + Duration::from_secs(2), deadline); + } +} diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index b35a3b368..5a6da1d6c 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -15,7 +15,6 @@ use daytona_sdk::api_types::SignedPortPreviewUrl; use daytona_sdk::toolbox_types::Command as SessionCommandResult; use daytona_sdk::{DaytonaError, SessionCommandLogsResult}; use fabro_github::GitHubCredentials; -use fabro_github::token_source::InstallationTokenSource; use fabro_static::EnvVars; use fabro_types::{CommandOutputStream, CommandTermination, RunId, SandboxProviderKind}; use fabro_util::time::elapsed_ms; @@ -26,9 +25,8 @@ use tokio::task::JoinHandle; use tokio::{fs, time}; use tokio_util::sync::CancellationToken; +use crate::clone_retry::{self, CloneRetryReason}; use crate::clone_source::{self, CloneDecision, EmptyWorkspaceReason}; -use crate::git_retry::{self, CredentialContext, GitRetryReason}; -use crate::push_credentials::{self, PushCredentialState}; use crate::redact::redact_auth_url; use crate::sandbox::{ self, BASH_ENV_VAR, BASH_PROBE_MARKER, BASH_PROBE_SCRIPT, BASH_PROBE_TIMEOUT_MS, REMOTE_BASH, @@ -343,7 +341,6 @@ pub struct DaytonaSandbox { client: daytona_sdk::Client, api_key: Option, github_app: Option, - push_credentials: PushCredentialState, sandbox: OnceCell, snapshot_name: OnceCell, rg_available: OnceCell, @@ -377,16 +374,11 @@ impl DaytonaSandbox { let client = build_daytona_client(api_key.clone()) .await .map_err(|e| crate::Error::context("Failed to create Daytona client", e))?; - let push_credentials = PushCredentialState::new(push_credentials::build_token_source( - github_app.as_ref(), - clone_origin_url.as_deref(), - )?); Ok(Self { config, client, api_key, github_app, - push_credentials, sandbox: OnceCell::new(), snapshot_name: OnceCell::new(), rg_available: OnceCell::const_new(), @@ -439,7 +431,6 @@ impl DaytonaSandbox { client, api_key, github_app: None, - push_credentials: PushCredentialState::new(None), sandbox: sandbox_cell, snapshot_name: OnceCell::new(), rg_available: OnceCell::const_new(), @@ -1083,18 +1074,27 @@ impl Sandbox for DaytonaSandbox { let layout = clone_source::github_repo_layout(&origin_url, WORKING_DIRECTORY, REPOS_ROOT) .map_err(|err| self.fail_init(init_start, err))?; + let token_was_freshly_minted = self + .github_app + .as_ref() + .is_some_and(GitHubCredentials::mints_installation_token); self.emit(SandboxEvent::GitCloneStarted { url: origin_url.clone(), branch: branch.clone(), }); let clone_start = Instant::now(); - // The clone mints its own token (never a warm-cache reuse) and - // seeds the shared source, so the first refresh compares - // against the clone token instead of believing nothing was - // ever embedded. - let resolved_token = match self.push_credentials.source() { - Some(source) => Some(source.mint_for_clone().await.map_err(|e| { + let (username, password) = match &self.github_app { + Some(creds) => fabro_github::resolve_clone_credentials( + &fabro_github::GitHubContext::new( + creds, + &fabro_github::github_api_base_url(), + ), + &layout.owner, + &layout.repo, + ) + .await + .map_err(|e| { let err = crate::Error::message(format!( "Failed to get GitHub App credentials for clone: {e}" )); @@ -1104,23 +1104,7 @@ impl Sandbox for DaytonaSandbox { causes: err.causes(), }); self.fail_init(init_start, err) - })?), - None => None, - }; - // The clone call site maps its mint knowledge onto the - // credential context: a token minted for this clone is - // FreshApp; a static credential cannot become valid by - // waiting. - let clone_credential_context = match &resolved_token { - Some(token) if !token.snapshot.is_static() => CredentialContext::FreshApp, - Some(_) => CredentialContext::Static, - None => CredentialContext::None, - }; - let (username, password) = match &resolved_token { - Some(token) => ( - Some("x-access-token".to_string()), - Some(token.token.expose().to_string()), - ), + })?, None => (None, None), }; @@ -1185,11 +1169,9 @@ impl Sandbox for DaytonaSandbox { self.fail_init(init_start, err) })?; - let clone_plan = git_retry::RetryPlan::clone_default(None); - let clone_result = git_retry::retry_git( + let clone_result = clone_retry::retry_clone( SandboxProviderKind::Daytona, - "clone", - &clone_plan, + None, |_attempt| { let git_svc = &git_svc; let origin = origin_url.as_str(); @@ -1202,7 +1184,7 @@ impl Sandbox for DaytonaSandbox { }; async move { git_svc.clone(origin, target, options).await } }, - |err: &DaytonaError| classify_clone_failure(err, clone_credential_context), + |err: &DaytonaError| classify_clone_failure(err, token_was_freshly_minted), ) .await; @@ -1270,11 +1252,8 @@ impl Sandbox for DaytonaSandbox { let _ = self.origin_url.set(origin_url.clone()); self.set_working_directory(layout.execution_directory.clone()) .map_err(|err| self.fail_init(init_start, err))?; - if let Some(resolved) = resolved_token { - match fabro_github::embed_token_in_url( - &origin_url, - resolved.token.expose(), - ) { + if let Some(token) = password.as_deref() { + match fabro_github::embed_token_in_url(&origin_url, token) { Ok(auth_url) => { let cmd = format!( "git -c maintenance.auto=0 remote set-url origin {}", @@ -1307,12 +1286,7 @@ impl Sandbox for DaytonaSandbox { sandbox will fail" ); } - Ok(_) => { - // Origin now carries this token; - // record it so refreshes compare - // against the clone generation. - self.push_credentials.record_embedded(resolved).await; - } + Ok(_) => {} Err(_) => { tracing::warn!( error_class = "daytona_set_url_exec_failed", @@ -1531,19 +1505,11 @@ impl Sandbox for DaytonaSandbox { )] } - async fn git_push_ref( - &self, - refspec: &str, - plan: &crate::RetryPlan, - ) -> Result { + async fn git_push_ref(&self, refspec: &str) -> crate::Result<()> { if !self.repo_cloned() { - return Ok(crate::PushReport::default()); + return Ok(()); } - let credentials = self - .origin_url - .get() - .map(|origin_url| (&self.push_credentials, origin_url.as_str())); - sandbox::git_push_via_exec(self, credentials, refspec, plan).await + crate::git_push_via_exec(self, refspec).await } async fn ssh_access_command(&self) -> crate::Result> { @@ -1579,38 +1545,50 @@ impl Sandbox for DaytonaSandbox { #[tracing::instrument(name = "git_op", skip_all, fields(op = "refresh-credentials"))] async fn refresh_push_credentials(&self) -> crate::Result { if !self.repo_cloned() { - return Ok(RefreshOutcome::none()); + return Ok(RefreshOutcome::Skipped); } let Some(origin_url) = self.origin_url.get() else { - return Ok(RefreshOutcome::none()); // no authenticated origin — nothing to refresh + return Ok(RefreshOutcome::Skipped); // no authenticated origin — nothing to refresh }; - self.push_credentials - .refresh(origin_url, |auth_url| async move { - let cmd = format!( - "git -c maintenance.auto=0 remote set-url origin {}", - shell_quote(auth_url.as_raw_url().as_str()), - ); - let result = self - .exec_command(&cmd, 10_000, None, None, None) - .await - .map_err(|_| { - crate::Error::message( - "Failed to refresh push credentials: set_url_exec_failed", - ) - })?; - if !result.is_success() { - return Err(result.into_exec_error_with_redactor( - "git remote set-url origin (refresh push credentials)", - |s| redact_auth_url(s, Some(&auth_url)), - )); - } - Ok(()) - }) - .await - } + let Some(creds) = &self.github_app else { + return Ok(RefreshOutcome::Skipped); + }; + // Only a GitHub App installation token can be re-minted; a static PAT or + // a pre-minted Installation token is fixed, so re-embedding it changes + // nothing. Short-circuit to Skipped before the resolve + set-url exec. + if !creds.mints_installation_token() { + return Ok(RefreshOutcome::Skipped); + } - fn push_token_source(&self) -> Option> { - self.push_credentials.source().cloned() + let auth_url = fabro_github::resolve_authenticated_url( + &fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), + origin_url, + ) + .await + .map_err(|_| { + crate::Error::message("Failed to refresh push credentials: token_mint_failed") + })?; + + let cmd = format!( + "git -c maintenance.auto=0 remote set-url origin {}", + shell_quote(auth_url.as_raw_url().as_str()), + ); + let result = self + .exec_command(&cmd, 10_000, None, None, None) + .await + .map_err(|_| { + crate::Error::message("Failed to refresh push credentials: set_url_exec_failed") + })?; + if !result.is_success() { + return Err(result.into_exec_error_with_redactor( + "git remote set-url origin (refresh push credentials)", + |s| redact_auth_url(s, Some(&auth_url)), + )); + } + + // Static creds were short-circuited to Skipped above; reaching here means + // a GitHub App installation token was freshly minted. + Ok(RefreshOutcome::Refreshed) } async fn set_autostop_interval(&self, minutes: i32) -> crate::Result<()> { @@ -2646,20 +2624,23 @@ fn daytona_bash_session_probe_outcome(execution: crate::Result) -> c /// inside the sandbox, so its stderr comes back through the toolbox as the /// error message — the credential race has to be matched on text. Daytona's own /// transport failures are visible structurally. -fn classify_clone_failure(err: &DaytonaError, cred: CredentialContext) -> Option { +fn classify_clone_failure( + err: &DaytonaError, + token_was_freshly_minted: bool, +) -> Option { // A Daytona request timeout does not prove that the remote clone stopped. // Retrying could overlap the still-running first request. if matches!(err, DaytonaError::Timeout { .. }) { return None; } - match git_retry::classify_message(err.message(), cred) { - git_retry::GitMessageClass::Retry(reason) => Some(reason), - git_retry::GitMessageClass::Permanent => None, - git_retry::GitMessageClass::Unknown => match err { - DaytonaError::RateLimit { .. } => Some(GitRetryReason::TransientInfra), + match clone_retry::classify_message(err.message(), token_was_freshly_minted) { + clone_retry::CloneMessageClass::Retry(reason) => Some(reason), + clone_retry::CloneMessageClass::Permanent => None, + clone_retry::CloneMessageClass::Unknown => match err { + DaytonaError::RateLimit { .. } => Some(CloneRetryReason::TransientInfra), DaytonaError::Api { status_code, .. } if (500..600).contains(status_code) => { - Some(GitRetryReason::TransientInfra) + Some(CloneRetryReason::TransientInfra) } DaytonaError::Timeout { .. } | DaytonaError::Api { .. } @@ -2794,7 +2775,6 @@ mod tests { client, api_key: Some(api_key.to_string()), github_app: None, - push_credentials: PushCredentialState::new(None), sandbox: OnceCell::new(), snapshot_name: OnceCell::new(), rg_available: OnceCell::const_new(), @@ -3195,11 +3175,11 @@ mod tests { let err = DaytonaError::general("repository not found: Repository not found."); assert_eq!( - classify_clone_failure(&err, CredentialContext::FreshApp), - Some(GitRetryReason::TokenReplication) + classify_clone_failure(&err, true), + Some(CloneRetryReason::TokenReplication) ); assert_eq!( - classify_clone_failure(&err, CredentialContext::None), + classify_clone_failure(&err, false), None, "without credentials there is no token to replicate" ); @@ -3212,8 +3192,8 @@ mod tests { DaytonaError::api(503, ""), ] { assert_eq!( - classify_clone_failure(&err, CredentialContext::None), - Some(GitRetryReason::TransientInfra), + classify_clone_failure(&err, false), + Some(CloneRetryReason::TransientInfra), "expected {err:?} to be transient" ); } @@ -3223,33 +3203,24 @@ mod tests { fn clone_timeout_is_not_retried_without_remote_termination() { let err = DaytonaError::timeout("request timed out"); - assert_eq!( - classify_clone_failure(&err, CredentialContext::FreshApp), - None - ); + assert_eq!(classify_clone_failure(&err, true), None); } #[test] fn clone_api_failure_message_takes_precedence_over_status() { let not_found = DaytonaError::api(500, "repository not found: Repository not found."); assert_eq!( - classify_clone_failure(¬_found, CredentialContext::FreshApp), - Some(GitRetryReason::TokenReplication) - ); - assert_eq!( - classify_clone_failure(¬_found, CredentialContext::Static), - None + classify_clone_failure(¬_found, true), + Some(CloneRetryReason::TokenReplication) ); + assert_eq!(classify_clone_failure(¬_found, false), None); for message in [ "fatal: destination path 'fabro' already exists", "remote: Permission to fabro-sh/fabro.git denied", ] { assert_eq!( - classify_clone_failure( - &DaytonaError::api(500, message), - CredentialContext::FreshApp - ), + classify_clone_failure(&DaytonaError::api(500, message), true), None, "expected {message:?} to take precedence over HTTP 500" ); @@ -3265,7 +3236,7 @@ mod tests { DaytonaError::general("fatal: could not read Username for 'https://github.com'"), ] { assert_eq!( - classify_clone_failure(&err, CredentialContext::FreshApp), + classify_clone_failure(&err, true), None, "expected {err:?} to fail fast" ); diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index e8440e66b..21530cc94 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -17,7 +17,6 @@ use bollard::exec::{CreateExecOptions, StartExecOptions, StartExecResults}; use bollard::image::CreateImageOptions; use bollard::models::{ContainerInspectResponse, HostConfig}; use fabro_github::GitHubCredentials; -use fabro_github::token_source::InstallationTokenSource; use fabro_types::{CommandOutputStream, CommandTermination, RunId, SandboxProviderKind}; use fabro_util::time::elapsed_ms; use futures::StreamExt; @@ -27,9 +26,7 @@ use tokio::{fs, time}; use tokio_util::sync::CancellationToken; use crate::clone_source::{self, CloneDecision, EmptyWorkspaceReason}; -use crate::git_retry::{self, CredentialContext}; use crate::managed_labels::{self, MANAGED_LABEL, RUN_ID_LABEL}; -use crate::push_credentials::{self, PushCredentialState}; use crate::redact::redact_auth_url; use crate::sandbox::{ self, BASH_ENV_VAR, BASH_PROBE_SCRIPT, BASH_PROBE_TIMEOUT_MS, REMOTE_BASH, @@ -40,7 +37,7 @@ use crate::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingRequest, ExecStreamingResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, SandboxFile, StderrCollector, StdioProcess, StdioProcessHandle, - StdioProcessTermination, WalkOptions, format_lines_numbered, shell_quote, + StdioProcessTermination, WalkOptions, clone_retry, format_lines_numbered, shell_quote, }; const DOCKER_BASH_REQUIREMENT: &str = "Docker sandboxes require /bin/bash for every command, with no `sh` fallback; use an \ @@ -61,7 +58,7 @@ const EXEC_TERM_GRACE_SECONDS: &str = "0.2"; struct DockerCloneFailure { error: crate::Error, - retry_reason: Option, + retry_reason: Option, } fn env_entry_name(entry: &str) -> &str { @@ -135,7 +132,6 @@ pub struct DockerSandbox { docker: Docker, config: DockerSandboxOptions, github_app: Option, - push_credentials: PushCredentialState, run_id: Option, clone_origin_url: Option, clone_branch: Option, @@ -171,14 +167,14 @@ impl DockerSandbox { clone_branch: Option, ) -> crate::Result { let docker = Docker::connect_with_local_defaults().map_err(crate::Error::docker_connect)?; - Self::with_docker_client( + Ok(Self::with_docker_client( docker, config, github_app, run_id, clone_origin_url, clone_branch, - ) + )) } fn with_docker_client( @@ -188,16 +184,11 @@ impl DockerSandbox { run_id: Option, clone_origin_url: Option, clone_branch: Option, - ) -> crate::Result { - let push_credentials = PushCredentialState::new(push_credentials::build_token_source( - github_app.as_ref(), - clone_origin_url.as_deref(), - )?); - Ok(Self { + ) -> Self { + Self { docker, config, github_app, - push_credentials, run_id, clone_origin_url, clone_branch, @@ -209,7 +200,7 @@ impl DockerSandbox { cached_os_version: std::sync::OnceLock::new(), rg_available: OnceCell::const_new(), event_callback: None, - }) + } } pub async fn reconnect( @@ -750,35 +741,23 @@ impl DockerSandbox { ) -> crate::Result<()> { self.verify_git_available().await?; let layout = clone_source::github_repo_layout(&origin_url, WORKING_DIRECTORY, REPOS_ROOT)?; - // The clone mints its own token (never a warm-cache reuse) and seeds - // the shared source, so the first refresh compares against the clone - // token instead of believing nothing was ever embedded. - let resolved_token = match self.push_credentials.source() { - Some(source) => Some(source.mint_for_clone().await.map_err(|e| { - crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )) - })?), - None => None, - }; - // The clone call site maps its mint knowledge onto the credential - // context: a token minted for this clone is FreshApp; a static - // credential cannot become valid by waiting. - let clone_credential_context = match &resolved_token { - Some(token) if !token.snapshot.is_static() => CredentialContext::FreshApp, - Some(_) => CredentialContext::Static, - None => CredentialContext::None, - }; + let token_was_freshly_minted = self + .github_app + .as_ref() + .is_some_and(GitHubCredentials::mints_installation_token); - let auth_url = match &resolved_token { - Some(token) => Some( - fabro_github::embed_token_in_url(&origin_url, token.token.expose()).map_err( - |e| { - crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )) - }, - )?, + let auth_url = match &self.github_app { + Some(creds) => Some( + fabro_github::resolve_authenticated_url( + &fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), + &origin_url, + ) + .await + .map_err(|e| { + crate::Error::message(format!( + "Failed to get GitHub App credentials for clone: {e}" + )) + })?, ), None => None, }; @@ -813,11 +792,9 @@ impl DockerSandbox { let command = git_clone_command(clone_url, branch.as_deref(), &layout.primary_repo_path); let clone_deadline = time::Instant::now() + GIT_CLONE_TIMEOUT; - let clone_plan = git_retry::RetryPlan::clone_default(Some(clone_deadline)); - let clone_result = git_retry::retry_git( + let clone_result = clone_retry::retry_clone( SandboxProviderKind::Docker, - "clone", - &clone_plan, + Some(clone_deadline), |_attempt| { let command = command.as_str(); let auth_url = auth_url.as_ref(); @@ -851,7 +828,7 @@ impl DockerSandbox { return Ok(()); } let retry_reason = - classify_docker_clone_result(&result, clone_credential_context); + classify_docker_clone_result(&result, token_was_freshly_minted); Err(DockerCloneFailure { error: self.clone_failure_error(result, auth_url), retry_reason, @@ -885,11 +862,6 @@ impl DockerSandbox { let _ = self.repo_cloned.set(true); let _ = self.origin_url.set(origin_url.clone()); self.set_working_directory(layout.execution_directory.clone())?; - if let Some(token) = resolved_token { - // The clone URL embedded this token in `origin`; record it so - // refreshes compare against the clone generation. - self.push_credentials.record_embedded(token).await; - } if let Some(auth_url) = auth_url.as_ref() { let command = format!( @@ -1405,12 +1377,12 @@ fn git_clone_command(clone_url: &str, branch: Option<&str>, checkout_path: &str) fn classify_docker_clone_result( result: &ExecResult, - cred: CredentialContext, -) -> Option { - let stderr = git_retry::classify_message(&result.stderr, cred); + token_was_freshly_minted: bool, +) -> Option { + let stderr = clone_retry::classify_message(&result.stderr, token_was_freshly_minted); match stderr { - git_retry::GitMessageClass::Unknown => { - git_retry::classify_message(&result.stdout, cred).retry_reason() + clone_retry::CloneMessageClass::Unknown => { + clone_retry::classify_message(&result.stdout, token_was_freshly_minted).retry_reason() } class => class.retry_reason(), } @@ -2194,19 +2166,11 @@ impl Sandbox for DockerSandbox { )] } - async fn git_push_ref( - &self, - refspec: &str, - plan: &crate::RetryPlan, - ) -> Result { + async fn git_push_ref(&self, refspec: &str) -> crate::Result<()> { if !self.repo_cloned() { - return Ok(crate::PushReport::default()); + return Ok(()); } - let credentials = self - .origin_url - .get() - .map(|origin_url| (&self.push_credentials, origin_url.as_str())); - sandbox::git_push_via_exec(self, credentials, refspec, plan).await + crate::git_push_via_exec(self, refspec).await } fn origin_url(&self) -> Option<&str> { @@ -2219,33 +2183,47 @@ impl Sandbox for DockerSandbox { #[tracing::instrument(name = "git_op", skip_all, fields(op = "refresh-credentials"))] async fn refresh_push_credentials(&self) -> crate::Result { if !self.repo_cloned() { - return Ok(RefreshOutcome::none()); + return Ok(RefreshOutcome::Skipped); } let Some(origin_url) = self.origin_url.get() else { - return Ok(RefreshOutcome::none()); + return Ok(RefreshOutcome::Skipped); }; - self.push_credentials - .refresh(origin_url, |auth_url| async move { - let command = format!( - "git -c maintenance.auto=0 remote set-url origin {}", - shell_quote(auth_url.as_raw_url().as_str()) - ); - let result = self - .docker_exec_shell(&command, 10_000, Some(self.working_directory()), None, None) - .await?; - if !result.is_success() { - return Err(result.into_exec_error_with_redactor( - "git remote set-url origin (refresh push credentials)", - |s| redact_auth_url(s, Some(&auth_url)), - )); - } - Ok(()) - }) - .await - } + let Some(creds) = &self.github_app else { + return Ok(RefreshOutcome::Skipped); + }; + // Only a GitHub App installation token can be re-minted; a static PAT or + // a pre-minted Installation token is fixed, so re-embedding it changes + // nothing. Short-circuit to Skipped before the resolve + set-url exec. + if !creds.mints_installation_token() { + return Ok(RefreshOutcome::Skipped); + } - fn push_token_source(&self) -> Option> { - self.push_credentials.source().cloned() + let auth_url = fabro_github::resolve_authenticated_url( + &fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), + origin_url, + ) + .await + .map_err(|_| { + crate::Error::message("Failed to refresh push credentials: token_mint_failed") + })?; + + let command = format!( + "git -c maintenance.auto=0 remote set-url origin {}", + shell_quote(auth_url.as_raw_url().as_str()) + ); + let result = self + .docker_exec_shell(&command, 10_000, Some(self.working_directory()), None, None) + .await?; + if !result.is_success() { + return Err(result.into_exec_error_with_redactor( + "git remote set-url origin (refresh push credentials)", + |s| redact_auth_url(s, Some(&auth_url)), + )); + } + + // Static creds were short-circuited to Skipped above; reaching here means + // a GitHub App installation token was freshly minted. + Ok(RefreshOutcome::Refreshed) } } @@ -2453,10 +2431,7 @@ mod tests { duration_ms: 1, }; - assert_eq!( - classify_docker_clone_result(&result, CredentialContext::FreshApp), - None - ); + assert_eq!(classify_docker_clone_result(&result, true), None); } #[test] @@ -2742,8 +2717,7 @@ mod tests { None, None, None, - ) - .expect("test sandbox should build"); + ); sandbox .container_id .set(container_id.to_string()) diff --git a/lib/components/fabro-sandbox/src/git_retry.rs b/lib/components/fabro-sandbox/src/git_retry.rs deleted file mode 100644 index 5e767abdb..000000000 --- a/lib/components/fabro-sandbox/src/git_retry.rs +++ /dev/null @@ -1,743 +0,0 @@ -//! Retry for git operations against GitHub from clone-based sandboxes. -//! -//! Clone-based providers can mint a GitHub App installation token and use it -//! immediately. GitHub can reject that first operation before the token is -//! available to the git endpoint. On a private repository, the rejection can -//! arrive as `Repository not found.` or an authentication failure. -//! -//! Only a token minted recently makes those messages safe to retry. Static -//! PATs and pre-minted installation tokens fail fast; a mature App token can -//! still hit a service-side blip that presents the same surface, so it -//! retries as transient infrastructure. -//! -//! Retries reuse the same token on purpose. Replication of a given token only -//! makes progress, so each attempt strictly improves the odds, while -//! re-minting would restart the replication clock. - -use std::future::Future; -use std::time::Duration; - -use chrono::Utc; -use fabro_github::token_source::TokenSnapshot; -#[cfg(test)] -use fabro_github::token_source::{REFRESH_MARGIN, TokenProvenance}; -use fabro_types::SandboxProviderKind; -use fabro_util::backoff::BackoffPolicy; -use tokio::time; - -/// How long after its mint a token is presumed to still be replicating to -/// GitHub's git endpoints. Matches the observed scale of the lag (seconds, -/// occasionally tens of seconds). -pub(crate) const REPLICATION_HORIZON: Duration = Duration::from_mins(1); - -/// Why a failed git attempt is worth repeating. -#[derive(Clone, Copy, Debug, PartialEq, Eq, strum::Display)] -#[strum(serialize_all = "snake_case")] -pub enum GitRetryReason { - /// A recently minted installation token has not reached the GitHub edge - /// cache site serving this operation yet. - TokenReplication, - /// The operation failed on infrastructure, unrelated to credentials. - TransientInfra, -} - -/// What a git failure message tells us about retry safety. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) enum GitMessageClass { - Retry(GitRetryReason), - Permanent, - Unknown, -} - -impl GitMessageClass { - pub(crate) fn retry_reason(self) -> Option { - match self { - Self::Retry(reason) => Some(reason), - Self::Permanent | Self::Unknown => None, - } - } -} - -/// What the operation's credentials say about retrying auth-shaped failures. -/// -/// Derived from the [`TokenSnapshot`] of the token embedded for the attempt, -/// so classification reads provenance as data instead of threading booleans -/// through call stacks. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum CredentialContext { - /// An installation token younger than [`REPLICATION_HORIZON`] — a 404 or - /// auth failure is likely replication lag; retry with the same token. - FreshApp, - /// An installation token older than the horizon. A 404 with it is - /// indistinguishable from a service-side blip at this layer, so it stays - /// transient rather than proving access loss. - MatureApp, - /// A PAT or pre-minted token — it cannot become valid by waiting. - Static, - /// No credentials at all. - None, -} - -impl CredentialContext { - #[must_use] - pub fn from_snapshot(snapshot: Option<&TokenSnapshot>) -> Self { - match snapshot { - None => Self::None, - Some(snapshot) => match snapshot.age_at(Utc::now()) { - None => Self::Static, - Some(age) if age < REPLICATION_HORIZON => Self::FreshApp, - Some(_) => Self::MatureApp, - }, - } - } -} - -/// Message fragments that mean the operation failed on infrastructure. -/// -/// These are safe to retry whether or not the operation was authenticated. -const TRANSIENT_HINTS: &[&str] = &[ - "could not resolve host", - "temporary failure in name resolution", - "connection refused", - "connection reset", - "connection timed out", - "timed out", - "network is unreachable", - "no route to host", - "tls handshake", - "early eof", - "rpc failed", - "unexpected disconnect", - "the remote end hung up unexpectedly", - "index-pack failed", - "service unavailable", - "gateway timeout", - "too many requests", - "rate limit", -]; - -/// Message fragments GitHub uses when a token is not yet visible. -/// -/// Only meaningful when the operation carried credentials. The same lag -/// surfaces as 404 or as an auth failure depending on which endpoint answers -/// first. -const TOKEN_REPLICATION_HINTS: &[&str] = &[ - "repository not found", - "authentication failed", - "invalid username or password", - "bad credentials", - // git CLI over HTTP. - "the requested url returned error: 401", - "the requested url returned error: 403", - "the requested url returned error: 404", - // libgit2 (the run-metadata writer pushes through git2). - "unexpected http status code: 401", - "unexpected http status code: 403", - "unexpected http status code: 404", -]; - -/// Whether a failure message has the 404/auth-failure shape GitHub produces -/// for both token-replication lag and a drifted or missing embedded token. -pub(crate) fn matches_auth_failure_hints(message: &str) -> bool { - let lower = message.to_ascii_lowercase(); - TOKEN_REPLICATION_HINTS - .iter() - .any(|hint| lower.contains(hint)) -} - -/// Classify a failed git operation by its rendered message. -/// -/// `cred` gates the reading of 404/auth-failure messages: a fresh App token -/// retries as replication lag, a mature one as transient infrastructure, and -/// a static credential (or none) fails fast because waiting cannot make it -/// valid. -pub(crate) fn classify_message(message: &str, cred: CredentialContext) -> GitMessageClass { - let lower = message.to_ascii_lowercase(); - - if TRANSIENT_HINTS.iter().any(|hint| lower.contains(hint)) { - return GitMessageClass::Retry(GitRetryReason::TransientInfra); - } - if TOKEN_REPLICATION_HINTS - .iter() - .any(|hint| lower.contains(hint)) - { - return match cred { - CredentialContext::FreshApp => GitMessageClass::Retry(GitRetryReason::TokenReplication), - CredentialContext::MatureApp => GitMessageClass::Retry(GitRetryReason::TransientInfra), - CredentialContext::Static | CredentialContext::None => GitMessageClass::Permanent, - }; - } - let permanent = lower.contains("could not read username") - || lower.contains("terminal prompts disabled") - || lower.contains("permission denied") - || (lower.contains("permission to") && lower.contains("denied")) - || (lower.contains("destination path") && lower.contains("already exists")) - || (lower.contains("remote branch") && lower.contains("not found")); - if permanent { - return GitMessageClass::Permanent; - } - GitMessageClass::Unknown -} - -/// Classify a rendered git failure message, returning the retry reason when -/// the failure is transient for these credentials. `None` means the failure -/// is permanent or unrecognized. -#[must_use] -pub fn classify_failure(message: &str, cred: CredentialContext) -> Option { - classify_message(message, cred).retry_reason() -} - -/// Backoff between attempts: 3s, then 9s. -/// -/// GitHub's guidance for token replication is to wait a few seconds and retry -/// with the same token. Sub-second delays land inside the same replication -/// window and spend an attempt for nothing. -fn clone_backoff() -> BackoffPolicy { - BackoffPolicy { - initial_delay: Duration::from_secs(3), - factor: 3.0, - max_delay: Duration::from_secs(10), - jitter: false, - } -} - -/// Attempt and time bounds for one retried git operation. -/// -/// All bounds are optional so existing behaviors are expressible unchanged. -/// The effective deadline is the minimum of the bounds that are present -/// (`start + max_elapsed`, `outer_deadline`); each attempt runs with -/// `min(per_attempt_timeout, remaining)` over the caps that are present, and -/// no attempt or backoff starts past the effective deadline. -#[derive(Debug, Clone)] -pub struct RetryPlan { - /// Total attempts, including the first. - pub max_attempts: u32, - pub backoff: BackoffPolicy, - /// Wall clock for this whole operation. - pub max_elapsed: Option, - /// Cap for any single attempt. - pub per_attempt_timeout: Option, - /// Caller-supplied absolute bound. - pub outer_deadline: Option, -} - -impl RetryPlan { - /// The clone policy both providers already trust: 3 attempts, 3s/9s - /// backoff, no plan-level bounds. Docker supplies its existing absolute - /// five-minute deadline through `outer_deadline`; Daytona supplies none. - #[must_use] - pub fn clone_default(outer_deadline: Option) -> Self { - Self { - max_attempts: 3, - backoff: clone_backoff(), - max_elapsed: None, - per_attempt_timeout: None, - outer_deadline, - } - } - - /// Checkpoint pushes stay cheap: the next checkpoint re-pushes the same - /// branch anyway. Worst case ~90 seconds of wall clock. - #[must_use] - pub fn checkpoint_push() -> Self { - Self { - max_attempts: 3, - backoff: clone_backoff(), - max_elapsed: Some(Duration::from_secs(90)), - per_attempt_timeout: Some(Duration::from_mins(1)), - outer_deadline: None, - } - } - - /// The terminal publish push guards the whole run's value, so it gets a - /// real budget: 5 attempts with growing backoff (~3s/10s/33s/60s), - /// bounded at 4 minutes of wall clock. The 4-minute bound must stay - /// under the token source's `REFRESH_MARGIN` (see the margin-invariant - /// test) so a pinned token always outlives the operation. - #[must_use] - pub fn publish_push() -> Self { - Self { - max_attempts: 5, - backoff: BackoffPolicy { - initial_delay: Duration::from_secs(3), - factor: 10.0 / 3.0, - max_delay: Duration::from_mins(1), - jitter: false, - }, - max_elapsed: Some(Duration::from_mins(4)), - per_attempt_timeout: Some(Duration::from_mins(1)), - outer_deadline: None, - } - } - - /// The absolute deadline this operation must finish by, if any bound is - /// present. - pub(crate) fn effective_deadline(&self, start: time::Instant) -> Option { - let elapsed_deadline = self.max_elapsed.map(|max| start + max); - match (elapsed_deadline, self.outer_deadline) { - (Some(a), Some(b)) => Some(a.min(b)), - (Some(a), None) => Some(a), - (None, Some(b)) => Some(b), - (None, None) => None, - } - } - - /// Time cap for an attempt starting now: the per-attempt cap bounded by - /// the time remaining before the effective deadline. - pub(crate) fn attempt_timeout(&self, deadline: Option) -> Option { - let remaining = deadline.map(|d| d.saturating_duration_since(time::Instant::now())); - match (self.per_attempt_timeout, remaining) { - (Some(cap), Some(remaining)) => Some(cap.min(remaining)), - (Some(cap), None) => Some(cap), - (None, remaining) => remaining, - } - } -} - -/// Run a git operation, repeating it while the failure looks transient. -/// -/// `attempt` receives the 1-based attempt number. `classify` decides whether -/// an error is worth repeating; `None` returns it to the caller untouched. -/// A retry starts only when its backoff fits before the plan's effective -/// deadline. The final error is returned as-is. -pub(crate) async fn retry_git( - provider: SandboxProviderKind, - op: &str, - plan: &RetryPlan, - mut attempt: Attempt, - classify: Classify, -) -> Result -where - Attempt: FnMut(u32) -> Fut, - Fut: Future>, - Classify: Fn(&E) -> Option, -{ - let deadline = plan.effective_deadline(time::Instant::now()); - - for attempt_number in 1..plan.max_attempts.max(1) { - match attempt(attempt_number).await { - Ok(value) => return Ok(value), - Err(err) => { - let Some(reason) = classify(&err) else { - return Err(err); - }; - let delay = plan.backoff.delay_for_attempt(attempt_number); - if deadline.is_some_and(|deadline| { - delay >= deadline.saturating_duration_since(time::Instant::now()) - }) { - return Err(err); - } - // The failure text can carry git stderr, so log the category - // rather than the message. The caller still reports the full - // error if the attempts run out. - tracing::warn!( - provider = %provider, - op, - attempt = attempt_number, - max_attempts = plan.max_attempts, - reason = %reason, - delay_ms = u64::try_from(delay.as_millis()).unwrap_or(u64::MAX), - "Git operation failed, retrying" - ); - time::sleep(delay).await; - } - } - } - - attempt(plan.max_attempts.max(1)).await -} - -#[cfg(test)] -mod tests { - use std::sync::Mutex; - - use super::*; - - /// Records the attempt numbers a closure was called with. - #[derive(Default)] - struct Attempts(Mutex>); - - impl Attempts { - fn record(&self, attempt: u32) { - self.0.lock().expect("attempt log mutex").push(attempt); - } - - fn recorded(&self) -> Vec { - self.0.lock().expect("attempt log mutex").clone() - } - } - - /// A classifier that treats every failure as worth repeating. - const ALWAYS_RETRY: fn(&String) -> Option = - |_| Some(GitRetryReason::TokenReplication); - - fn fresh_snapshot(age: Duration, ttl: Duration) -> TokenSnapshot { - let now = Utc::now(); - TokenSnapshot { - generation: 1, - provenance: TokenProvenance::Minted { - minted_at: now - chrono::Duration::from_std(age).unwrap(), - expires_at: now + chrono::Duration::from_std(ttl).unwrap(), - }, - } - } - - #[test] - fn credential_context_reads_token_age_from_provenance() { - assert_eq!( - CredentialContext::from_snapshot(None), - CredentialContext::None - ); - assert_eq!( - CredentialContext::from_snapshot(Some(&TokenSnapshot { - generation: 0, - provenance: TokenProvenance::Static, - })), - CredentialContext::Static - ); - assert_eq!( - CredentialContext::from_snapshot(Some(&fresh_snapshot( - Duration::from_secs(5), - Duration::from_hours(1) - ))), - CredentialContext::FreshApp - ); - assert_eq!( - CredentialContext::from_snapshot(Some(&fresh_snapshot( - Duration::from_mins(2), - Duration::from_hours(1) - ))), - CredentialContext::MatureApp - ); - } - - #[test] - fn private_repo_not_found_with_a_fresh_token_is_a_replication_lag() { - assert_eq!( - classify_message( - "repository not found: Repository not found.", - CredentialContext::FreshApp - ), - GitMessageClass::Retry(GitRetryReason::TokenReplication) - ); - } - - #[test] - fn not_found_with_a_mature_token_is_transient_not_permanent() { - // A service-side blip is indistinguishable from access loss at this - // layer, so a mature-App 404 stays retryable. - assert_eq!( - classify_message( - "repository not found: Repository not found.", - CredentialContext::MatureApp - ), - GitMessageClass::Retry(GitRetryReason::TransientInfra) - ); - } - - #[test] - fn not_found_with_static_or_no_credentials_is_permanent() { - for cred in [CredentialContext::Static, CredentialContext::None] { - assert_eq!( - classify_message("repository not found: Repository not found.", cred), - GitMessageClass::Permanent, - "{cred:?} cannot become valid by waiting" - ); - } - } - - #[test] - fn auth_failure_classification_follows_the_credential_context() { - let message = "fatal: Authentication failed for 'https://github.com/owner/repo'"; - assert_eq!( - classify_message(message, CredentialContext::FreshApp), - GitMessageClass::Retry(GitRetryReason::TokenReplication) - ); - assert_eq!( - classify_message(message, CredentialContext::MatureApp), - GitMessageClass::Retry(GitRetryReason::TransientInfra) - ); - assert_eq!( - classify_message(message, CredentialContext::Static), - GitMessageClass::Permanent - ); - } - - #[test] - fn infra_failures_retry_without_credentials() { - for message in [ - "fatal: unable to access: Could not resolve host: github.com", - "error: RPC failed; curl 56 recv failure", - "fatal: early EOF", - "Operation timed out", - ] { - assert_eq!( - classify_message(message, CredentialContext::None), - GitMessageClass::Retry(GitRetryReason::TransientInfra), - "expected {message:?} to be transient" - ); - } - } - - #[test] - fn genuine_failures_are_not_retried() { - for message in [ - "fatal: could not read Username for 'https://github.com'", - "remote: Permission to owner/repo.git denied", - "fatal: destination path 'repo' already exists", - ] { - assert_eq!( - classify_message(message, CredentialContext::FreshApp), - GitMessageClass::Permanent, - "expected {message:?} to fail fast" - ); - } - } - - #[test] - fn unrecognized_failures_remain_unknown() { - assert_eq!( - classify_message( - "git operation stopped for an unexpected reason", - CredentialContext::FreshApp - ), - GitMessageClass::Unknown - ); - } - - #[test] - fn backoff_waits_seconds_not_milliseconds() { - let plan = RetryPlan::clone_default(None); - assert_eq!(plan.backoff.delay_for_attempt(1), Duration::from_secs(3)); - assert_eq!(plan.backoff.delay_for_attempt(2), Duration::from_secs(9)); - } - - #[test] - fn publish_backoff_grows_toward_a_one_minute_cap() { - let plan = RetryPlan::publish_push(); - assert_eq!(plan.backoff.delay_for_attempt(1), Duration::from_secs(3)); - assert_eq!(plan.backoff.delay_for_attempt(2), Duration::from_secs(10)); - assert!(plan.backoff.delay_for_attempt(3) < Duration::from_secs(35)); - assert_eq!(plan.backoff.delay_for_attempt(4), Duration::from_mins(1)); - } - - /// `REFRESH_MARGIN` must exceed every push plan's `max_elapsed`: a push - /// pins the token of its single successful resolve, and any token the - /// source returns has at least the margin of validity left, so the pinned - /// token must outlive the whole operation. - #[test] - fn refresh_margin_exceeds_every_push_plan_elapsed_bound() { - for plan in [RetryPlan::checkpoint_push(), RetryPlan::publish_push()] { - let max_elapsed = plan.max_elapsed.expect("push plans bound elapsed time"); - assert!( - REFRESH_MARGIN > max_elapsed, - "margin invariant violated: {max_elapsed:?}" - ); - } - } - - #[test] - fn effective_deadline_takes_the_minimum_of_present_bounds() { - let start = time::Instant::now(); - let outer = start + Duration::from_secs(30); - - let unbounded = RetryPlan::clone_default(None); - assert_eq!(unbounded.effective_deadline(start), None); - - let outer_only = RetryPlan::clone_default(Some(outer)); - assert_eq!(outer_only.effective_deadline(start), Some(outer)); - - let mut both = RetryPlan::checkpoint_push(); - both.outer_deadline = Some(outer); - assert_eq!(both.effective_deadline(start), Some(outer)); - - both.outer_deadline = Some(start + Duration::from_mins(10)); - assert_eq!( - both.effective_deadline(start), - Some(start + Duration::from_secs(90)) - ); - } - - #[tokio::test(start_paused = true)] - async fn attempt_timeout_is_capped_by_the_remaining_deadline() { - let plan = RetryPlan::checkpoint_push(); - let deadline = Some(time::Instant::now() + Duration::from_secs(20)); - assert_eq!( - plan.attempt_timeout(deadline), - Some(Duration::from_secs(20)) - ); - assert_eq!(plan.attempt_timeout(None), Some(Duration::from_mins(1))); - - let unbounded = RetryPlan::clone_default(None); - assert_eq!(unbounded.attempt_timeout(None), None); - } - - #[tokio::test(start_paused = true)] - async fn first_success_runs_one_attempt() { - let attempts = Attempts::default(); - - let result = retry_git( - SandboxProviderKind::Docker, - "clone", - &RetryPlan::clone_default(None), - |attempt| { - attempts.record(attempt); - async move { Ok::<_, String>(attempt) } - }, - ALWAYS_RETRY, - ) - .await; - - assert_eq!(result, Ok(1)); - assert_eq!(attempts.recorded(), vec![1]); - } - - #[tokio::test(start_paused = true)] - async fn retries_until_a_later_attempt_succeeds() { - let attempts = Attempts::default(); - - let result = retry_git( - SandboxProviderKind::Docker, - "clone", - &RetryPlan::clone_default(None), - |attempt| { - attempts.record(attempt); - async move { - if attempt < 3 { - Err("Repository not found.".to_string()) - } else { - Ok(attempt) - } - } - }, - ALWAYS_RETRY, - ) - .await; - - assert_eq!(result, Ok(3)); - assert_eq!(attempts.recorded(), vec![1, 2, 3]); - } - - #[tokio::test(start_paused = true)] - async fn exhausted_attempts_return_the_final_error() { - let attempts = Attempts::default(); - - let result = retry_git( - SandboxProviderKind::Docker, - "clone", - &RetryPlan::clone_default(None), - |attempt| { - attempts.record(attempt); - async move { Err::<(), _>(format!("Repository not found. (attempt {attempt})")) } - }, - ALWAYS_RETRY, - ) - .await; - - assert_eq!( - result, - Err("Repository not found. (attempt 3)".to_string()), - "the caller should see the last failure, not the first" - ); - assert_eq!(attempts.recorded(), vec![1, 2, 3]); - } - - #[tokio::test(start_paused = true)] - async fn unretryable_failure_stops_immediately() { - let attempts = Attempts::default(); - - let result = retry_git( - SandboxProviderKind::Docker, - "clone", - &RetryPlan::clone_default(None), - |attempt| { - attempts.record(attempt); - async move { Err::<(), _>("permission denied".to_string()) } - }, - |_: &String| None, - ) - .await; - - assert_eq!(result, Err("permission denied".to_string())); - assert_eq!( - attempts.recorded(), - vec![1], - "a deterministic failure should not wait out the backoff" - ); - } - - /// Docker clone parity: the caller's absolute deadline stops retries when - /// the backoff no longer fits before it. - #[tokio::test(start_paused = true)] - async fn outer_deadline_stops_retry_when_backoff_does_not_fit() { - let attempts = Attempts::default(); - let deadline = time::Instant::now() + Duration::from_secs(2); - - let result = retry_git( - SandboxProviderKind::Docker, - "clone", - &RetryPlan::clone_default(Some(deadline)), - |attempt| { - attempts.record(attempt); - async move { Err::<(), _>("temporary failure".to_string()) } - }, - ALWAYS_RETRY, - ) - .await; - - assert_eq!(result, Err("temporary failure".to_string())); - assert_eq!(attempts.recorded(), vec![1]); - assert_eq!(time::Instant::now() + Duration::from_secs(2), deadline); - } - - /// Daytona clone parity: with no bounds at all, attempts are limited only - /// by `max_attempts` and backoff. - #[tokio::test(start_paused = true)] - async fn unbounded_plan_runs_all_attempts() { - let attempts = Attempts::default(); - - let result = retry_git( - SandboxProviderKind::Daytona, - "clone", - &RetryPlan::clone_default(None), - |attempt| { - attempts.record(attempt); - async move { Err::<(), _>("temporary failure".to_string()) } - }, - |_: &String| Some(GitRetryReason::TransientInfra), - ) - .await; - - assert!(result.is_err()); - assert_eq!(attempts.recorded(), vec![1, 2, 3]); - } - - #[tokio::test(start_paused = true)] - async fn max_elapsed_stops_retry_when_backoff_does_not_fit() { - let attempts = Attempts::default(); - let plan = RetryPlan { - max_attempts: 5, - backoff: clone_backoff(), - max_elapsed: Some(Duration::from_secs(4)), - per_attempt_timeout: None, - outer_deadline: None, - }; - - let result = retry_git( - SandboxProviderKind::Docker, - "push", - &plan, - |attempt| { - attempts.record(attempt); - async move { Err::<(), _>("temporary failure".to_string()) } - }, - ALWAYS_RETRY, - ) - .await; - - assert!(result.is_err()); - // Attempt 1 fails instantly, 3s backoff fits inside 4s, attempt 2 - // fails, and the 9s backoff no longer fits. - assert_eq!(attempts.recorded(), vec![1, 2]); - } -} diff --git a/lib/components/fabro-sandbox/src/lib.rs b/lib/components/fabro-sandbox/src/lib.rs index 5777beccf..3a5e91ab2 100644 --- a/lib/components/fabro-sandbox/src/lib.rs +++ b/lib/components/fabro-sandbox/src/lib.rs @@ -9,13 +9,13 @@ pub mod sandbox_spec; #[cfg(any(feature = "docker", feature = "daytona"))] mod clone_source; -pub mod git_retry; +#[cfg(any(feature = "docker", feature = "daytona", test))] +mod clone_retry; #[cfg(any(feature = "docker", feature = "daytona", test))] mod managed_labels; -mod push_credentials; - +#[cfg(any(feature = "docker", feature = "daytona", test))] pub mod redact; pub mod details; @@ -39,11 +39,7 @@ pub use details::sandbox_details; #[cfg(feature = "docker")] pub use docker::{DockerSandbox, DockerSandboxOptions}; pub use error::{Error, Result, default_redacted_output_tail, display_for_log}; -pub use fabro_github::token_source::{ - InstallationTokenSource, ResolvedToken, TokenProvenance, TokenSnapshot, -}; pub use fabro_types::{RunSandboxInstance, SandboxProviderKind}; -pub use git_retry::{CredentialContext, GitRetryReason, RetryPlan}; pub use local::LocalSandbox; #[cfg(feature = "daytona")] pub use provider::daytona::DaytonaSandboxProvider; @@ -53,15 +49,13 @@ pub use provider::{ LocalSandboxProvider, SandboxCreateSpec, SandboxLookupError, SandboxProvider, SandboxProviderRegistry, }; -pub use push_credentials::RefreshErrorKind; pub use reconnect::{reconnect, reconnect_for_run, reconnect_for_run_with_callback}; pub use sandbox::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingRequest, ExecStreamingResult, GitRunInfo, GitSetupIntent, GrepOptions, - PushAttempt, PushError, PushReport, RefreshOutcome, RemoteCredentialAction, Sandbox, - SandboxEvent, SandboxEventCallback, SandboxFile, StderrCollector, StdioProcess, - StdioProcessHandle, StdioProcessTermination, WalkOptions, format_lines_numbered, - redacted_output_tail, setup_git_via_exec, shell_quote, + RefreshOutcome, Sandbox, SandboxEvent, SandboxEventCallback, SandboxFile, StderrCollector, + StdioProcess, StdioProcessHandle, StdioProcessTermination, WalkOptions, format_lines_numbered, + git_push_via_exec, redacted_output_tail, setup_git_via_exec, shell_quote, }; pub use sandbox_spec::SandboxSpec; pub use terminal::{TerminalSession, TerminalSize, open_terminal_for_run}; diff --git a/lib/components/fabro-sandbox/src/local.rs b/lib/components/fabro-sandbox/src/local.rs index 650d57c47..49737780d 100644 --- a/lib/components/fabro-sandbox/src/local.rs +++ b/lib/components/fabro-sandbox/src/local.rs @@ -13,8 +13,8 @@ use tokio::{fs, time}; use tokio_util::sync::CancellationToken; use crate::sandbox::{ - self, BASH_ENV_VAR, BASH_PROBE_SCRIPT, BASH_PROBE_TIMEOUT_MS, StdioProcessControl, - optional_timeout, validate_bash_probe, write_process_stdin, + BASH_ENV_VAR, BASH_PROBE_SCRIPT, BASH_PROBE_TIMEOUT_MS, StdioProcessControl, optional_timeout, + validate_bash_probe, write_process_stdin, }; use crate::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, @@ -878,31 +878,20 @@ impl Sandbox for LocalSandbox { Ok(()) } - async fn git_push_ref( - &self, - refspec: &str, - plan: &crate::RetryPlan, - ) -> Result { + async fn git_push_ref(&self, refspec: &str) -> crate::Result<()> { let has_origin = match self .exec_command("git remote get-url origin", 10_000, None, None, None) .await { Ok(result) if result.is_success() => true, Ok(_) => false, - Err(err) => { - return Err(crate::PushError { - report: crate::PushReport::default(), - error: crate::Error::context("git remote get-url origin", err), - }); - } + Err(err) => return Err(crate::Error::context("git remote get-url origin", err)), }; if !has_origin { - return Ok(crate::PushReport::default()); + return Ok(()); } - // Local pushes use whatever credentials the host repository already - // carries; there is no managed credential state to lease. - sandbox::git_push_via_exec(self, None, refspec, plan).await + crate::git_push_via_exec(self, refspec).await } async fn cleanup(&self) -> crate::Result<()> { diff --git a/lib/components/fabro-sandbox/src/push_credentials.rs b/lib/components/fabro-sandbox/src/push_credentials.rs deleted file mode 100644 index 5ec3cc48b..000000000 --- a/lib/components/fabro-sandbox/src/push_credentials.rs +++ /dev/null @@ -1,599 +0,0 @@ -//! Shared push-credential state for clone-based sandbox providers. -//! -//! Docker and Daytona embed GitHub credentials into the cloned repository's -//! `origin` remote and refresh them before pushes. Both providers hold this -//! state so the compare → `set-url` → record sequence, the generation -//! tracking, and the refresh-error logging behave identically across -//! providers. The token cache itself sits below the providers, in -//! [`fabro_github::token_source::InstallationTokenSource`]. - -use std::future::Future; -use std::sync::Arc; - -use fabro_github::GitHubCredentials; -use fabro_github::token_source::{InstallationTokenSource, ResolvedToken, TokenSnapshot}; -use fabro_redact::DisplaySafeUrl; -use tokio::sync::{Mutex, MutexGuard}; - -use crate::redact; -use crate::sandbox::{RefreshOutcome, RemoteCredentialAction}; - -/// Build the shared installation-token source for a clone-based sandbox. -/// -/// Returns `None` when there are no managed credentials or no GitHub origin -/// to scope them to. Minted tokens carry the same `contents: write` -/// permission the clone token uses. -pub(crate) fn build_token_source( - github_app: Option<&GitHubCredentials>, - clone_origin_url: Option<&str>, -) -> crate::Result>> { - let Some(creds) = github_app else { - return Ok(None); - }; - let Some(origin_url) = clone_origin_url.filter(|url| !url.trim().is_empty()) else { - return Ok(None); - }; - let normalized = fabro_github::normalize_repo_origin_url(origin_url); - if fabro_github::parse_github_owner_repo(&normalized).is_err() { - // Non-GitHub origins never clone in these providers, so there is no - // remote to keep credentials fresh for. - return Ok(None); - } - InstallationTokenSource::for_origin( - creds, - &normalized, - serde_json::json!({ "contents": "write" }), - ) - .map(Some) - .map_err(|err| crate::Error::message(format!("Failed to build GitHub token source: {err:#}"))) -} - -/// Push-credential state one provider instance tracks for its `origin` -/// remote. -pub(crate) struct PushCredentialState { - source: Option>, - /// Serializes compare → `set-url` → record. The token source's - /// single-flight ends before the sandbox exec, so without this lock a - /// refresh-ahead tick and a push could both see the old embedded - /// generation and race on `.git/config.lock`. Holds the last - /// successfully embedded token: its secret is already in the remote URL - /// inside the sandbox, so retaining it adds no exposure, and it is what - /// a push falls back to when a refresh fails. The tracked value is local - /// belief, not ground truth — agent code inside the sandbox can rewrite - /// `origin`. - embedded: Mutex>, -} - -impl PushCredentialState { - pub(crate) fn new(source: Option>) -> Self { - Self { - source, - embedded: Mutex::new(None), - } - } - - pub(crate) fn source(&self) -> Option<&Arc> { - self.source.as_ref() - } - - /// Record the token embedded in `origin` outside the refresh path — the - /// clone is the first operation to embed a token, and it seeds this - /// state so the first refresh compares against the clone token instead - /// of believing nothing was ever embedded. - pub(crate) async fn record_embedded(&self, token: ResolvedToken) { - *self.embedded.lock().await = Some(token); - } - - /// Refresh the credentials embedded in `origin`. - /// - /// Resolves through the shared source, skips the `set-url` exec when the - /// resolved generation is already embedded, and records the new - /// generation only after `set_url` succeeds. `set_url` receives the - /// authenticated URL to embed and runs under the embed lock. - pub(crate) async fn refresh( - &self, - origin_url: &str, - set_url: F, - ) -> crate::Result - where - F: FnOnce(DisplaySafeUrl) -> Fut, - Fut: Future>, - { - let Some(source) = &self.source else { - return Ok(RefreshOutcome::none()); - }; - let mut embedded = self.embedded.lock().await; - let resolved = match source.resolve().await { - Ok(resolved) => resolved, - Err(err) => { - // The refresh-error path is defined, not incidental: the push - // proceeds with the last embedded token, so log which one - // that is instead of losing the credential state. - if let Some(prev) = embedded.as_ref() { - tracing::warn!( - error = %format!("{err:#}"), - generation = prev.snapshot.generation, - provenance = %prev.snapshot.provenance, - token_age_ms = prev.snapshot.age_ms(), - "GitHub token refresh failed; origin keeps the last embedded credentials" - ); - } else { - tracing::warn!( - error = %format!("{err:#}"), - "GitHub token refresh failed and no credentials were ever embedded" - ); - } - return Err(crate::Error::message( - "Failed to refresh push credentials: token_mint_failed", - )); - } - }; - if embedded - .as_ref() - .is_some_and(|prev| prev.snapshot.generation == resolved.snapshot.generation) - { - return Ok(RefreshOutcome { - action: RemoteCredentialAction::Unchanged, - token: Some(resolved.snapshot), - }); - } - let auth_url = fabro_github::embed_token_in_url(origin_url, resolved.token.expose()) - .map_err(|err| { - crate::Error::message(format!("Failed to build authenticated origin URL: {err:#}")) - })?; - set_url(auth_url).await?; - let snapshot = resolved.snapshot; - *embedded = Some(resolved); - Ok(RefreshOutcome { - action: RemoteCredentialAction::Embedded, - token: Some(snapshot), - }) - } -} - -/// Which refresh step failed while a push held the credential lease. -#[derive(Clone, Copy, Debug, PartialEq, Eq, strum::Display)] -#[strum(serialize_all = "snake_case")] -pub enum RefreshErrorKind { - /// Minting a replacement token failed; the push proceeded with the last - /// embedded token. - Mint, - /// Rewriting `origin` with the resolved token failed; the push proceeded - /// with the last embedded token. - SetUrl, -} - -/// What [`CredentialLease::ensure_embedded`] did for one push attempt. -#[derive(Debug, Clone, Copy)] -pub(crate) struct EnsureOutcome { - pub action: RemoteCredentialAction, - /// The token embedded in the remote right now — never an unembedded mint. - pub token: Option, - pub refresh_error: Option, -} - -/// Scoped pin of push credentials for one push operation. -/// -/// Holds the provider's embed mutex until dropped, so no other refresh can -/// re-embed mid-operation — a refresh-ahead tick crossing the cache margin -/// during a retrying push waits here instead of swapping the remote out from -/// under the pin. Internally retains up to two secrets: the last successfully -/// embedded token (the fallback) and the operation's resolved target, so both -/// drift re-embedding and the refresh-error fallback work. Only non-secret -/// snapshots leave the lease. -/// -/// A successful resolve happens at most once per operation and is never -/// replaced; the pin transitions to the target only through a successful -/// embed. The token source's refresh margin exceeds every push plan's elapsed -/// bound, so the pinned token always outlives the operation. -pub(crate) struct CredentialLease<'a> { - source: Option<&'a InstallationTokenSource>, - /// Embed-mutex guard: the last successfully embedded token. - embedded: MutexGuard<'a, Option>, - /// The operation's single successful resolve. - target: Option, -} - -impl PushCredentialState { - /// Acquire the push-credential lease for one push operation. - /// - /// Resolves the operation's target token up front, pinning one token - /// generation for every attempt. A failed resolve still acquires the - /// lease when an earlier operation embedded a token (the push falls back - /// to it and [`CredentialLease::ensure_embedded`] retries the resolve on - /// later attempts); with managed credentials but nothing ever embedded, - /// acquisition fails — there is nothing to push with. - pub(crate) async fn lease(&self) -> crate::Result> { - let embedded = self.embedded.lock().await; - let Some(source) = self.source.as_deref() else { - return Ok(CredentialLease { - source: None, - embedded, - target: None, - }); - }; - match source.resolve().await { - Ok(resolved) => Ok(CredentialLease { - source: Some(source), - embedded, - target: Some(resolved), - }), - Err(err) => { - if let Some(prev) = embedded.as_ref() { - tracing::warn!( - error = %format!("{err:#}"), - generation = prev.snapshot.generation, - provenance = %prev.snapshot.provenance, - token_age_ms = prev.snapshot.age_ms(), - "token resolve failed; push pins the last embedded credentials" - ); - Ok(CredentialLease { - source: Some(source), - embedded, - target: None, - }) - } else { - tracing::warn!( - error = %format!("{err:#}"), - "token resolve failed and no credentials were ever embedded" - ); - Err(crate::Error::message( - "Failed to refresh push credentials: token_mint_failed", - )) - } - } - } - } -} - -impl CredentialLease<'_> { - /// Non-secret description of the token embedded in the remote right now. - pub(crate) fn snapshot(&self) -> Option { - self.embedded.as_ref().map(|token| token.snapshot) - } - - /// Embed the pinned generation if the remote does not carry it. - /// - /// One call covers the initial embed, a deferred embed after an earlier - /// failure, and drift repair (`force` re-embeds even when the tracked - /// generation matches, for remotes rewritten inside the sandbox). While - /// the lease has no target, this retries the failed `resolve()` first — - /// retrying a failed resolve discards no fresh token, so it cannot - /// restart any replication clock. Refresh failures are recorded, never - /// propagated: the push proceeds with the last embedded token. - pub(crate) async fn ensure_embedded( - &mut self, - sandbox: &dyn crate::Sandbox, - origin_url: &str, - force: bool, - ) -> EnsureOutcome { - let Some(source) = self.source else { - return EnsureOutcome { - action: RemoteCredentialAction::None, - token: None, - refresh_error: None, - }; - }; - let mut refresh_error = None; - if self.target.is_none() { - match source.resolve().await { - Ok(resolved) => self.target = Some(resolved), - Err(err) => { - tracing::warn!( - error = %format!("{err:#}"), - "token resolve retry failed; pushing with the last embedded token" - ); - refresh_error = Some(RefreshErrorKind::Mint); - } - } - } - let Some(desired) = self.target.as_ref().or(self.embedded.as_ref()).cloned() else { - // Managed credentials with nothing resolved or embedded: - // acquisition fails before any attempt runs, so pushes never see - // this state. - return EnsureOutcome { - action: RemoteCredentialAction::None, - token: None, - refresh_error, - }; - }; - let embedded_generation = self - .embedded - .as_ref() - .map(|token| token.snapshot.generation); - if !force && embedded_generation == Some(desired.snapshot.generation) { - return EnsureOutcome { - action: RemoteCredentialAction::Unchanged, - token: Some(desired.snapshot), - refresh_error, - }; - } - match set_url_via_exec(sandbox, origin_url, &desired).await { - Ok(()) => { - let snapshot = desired.snapshot; - *self.embedded = Some(desired); - EnsureOutcome { - action: RemoteCredentialAction::Embedded, - token: Some(snapshot), - refresh_error, - } - } - Err(err) => { - tracing::warn!( - error = %crate::display_for_log(&err), - "embedding push credentials in origin failed; pushing with the last embedded token" - ); - EnsureOutcome { - action: RemoteCredentialAction::Unchanged, - token: self.snapshot(), - refresh_error: Some(RefreshErrorKind::SetUrl), - } - } - } - } -} - -/// Rewrite `origin` with the token embedded, through the sandbox's uniform -/// exec surface. -async fn set_url_via_exec( - sandbox: &dyn crate::Sandbox, - origin_url: &str, - token: &ResolvedToken, -) -> crate::Result<()> { - let auth_url = - fabro_github::embed_token_in_url(origin_url, token.token.expose()).map_err(|err| { - crate::Error::message(format!("Failed to build authenticated origin URL: {err:#}")) - })?; - let command = format!( - "git -c maintenance.auto=0 remote set-url origin {}", - crate::shell_quote(auth_url.as_raw_url().as_str()) - ); - let result = sandbox - .exec_command(&command, 10_000, None, None, None) - .await - .map_err(|_| { - crate::Error::message("Failed to refresh push credentials: set_url_exec_failed") - })?; - if !result.is_success() { - return Err(result.into_exec_error_with_redactor( - "git remote set-url origin (push credential lease)", - |s| redact::redact_auth_url(s, Some(&auth_url)), - )); - } - Ok(()) -} - -#[cfg(test)] -mod tests { - use std::sync::atomic::{AtomicUsize, Ordering}; - - use chrono::Utc; - use fabro_github::InstallationToken; - use fabro_github::token_source::InstallationTokenMinter; - use tokio::time::sleep; - - use super::*; - - struct FixedMinter { - calls: AtomicUsize, - ttl: chrono::Duration, - } - - #[async_trait::async_trait] - impl InstallationTokenMinter for FixedMinter { - async fn mint(&self) -> anyhow::Result { - let call = self.calls.fetch_add(1, Ordering::SeqCst) + 1; - Ok(InstallationToken { - token: format!("ghs_gen{call}"), - expires_at: Utc::now() + self.ttl, - }) - } - } - - struct FailingMinter; - - #[async_trait::async_trait] - impl InstallationTokenMinter for FailingMinter { - async fn mint(&self) -> anyhow::Result { - Err(anyhow::anyhow!("mint failed")) - } - } - - fn minting_state(ttl: chrono::Duration) -> PushCredentialState { - PushCredentialState::new(Some(InstallationTokenSource::with_minter( - "owner/repo".to_string(), - Box::new(FixedMinter { - calls: AtomicUsize::new(0), - ttl, - }), - ))) - } - - const ORIGIN: &str = "https://github.com/owner/repo"; - /// Long enough for a blocked task to be observably pending on paused time. - const SHORT_WAIT: std::time::Duration = std::time::Duration::from_secs(5); - - /// A refresh-ahead tick crossing the cache margin during a push waits on - /// the embed mutex until the operation releases the lease, so the remote - /// can never be swapped out from under the pinned generation. - #[tokio::test(start_paused = true)] - async fn refresh_waits_for_the_lease_to_release() { - let state = std::sync::Arc::new(minting_state(chrono::Duration::minutes(60))); - - let lease = state.lease().await.expect("lease acquires"); - - let refresh_task = { - let state = std::sync::Arc::clone(&state); - tokio::spawn(async move { - state - .refresh(ORIGIN, |_| async { Ok(()) }) - .await - .expect("refresh succeeds after the lease releases") - }) - }; - - // The refresh must be blocked while the lease holds the embed mutex. - sleep(SHORT_WAIT).await; - assert!( - !refresh_task.is_finished(), - "refresh must wait on the embed mutex" - ); - - drop(lease); - let outcome = refresh_task.await.expect("refresh task completes"); - // The lease's resolve minted generation 1; the deferred refresh - // reuses it (the operation never embedded, so the refresh embeds). - assert_eq!(outcome.token.unwrap().generation, 1); - } - - #[tokio::test] - async fn refresh_without_managed_credentials_reports_none() { - let state = PushCredentialState::new(None); - let outcome = state - .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) - .await - .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::None); - assert_eq!(outcome.token, None); - } - - #[tokio::test] - async fn refresh_embeds_a_new_generation_and_skips_matching_ones() { - let state = minting_state(chrono::Duration::minutes(60)); - let set_url_calls = AtomicUsize::new(0); - - let first = state - .refresh(ORIGIN, |auth_url| { - set_url_calls.fetch_add(1, Ordering::SeqCst); - assert!(auth_url.as_raw_url().as_str().contains("ghs_gen1")); - async { Ok(()) } - }) - .await - .unwrap(); - assert_eq!(first.action, RemoteCredentialAction::Embedded); - assert_eq!(first.token.unwrap().generation, 1); - - // The cached token is fresh, so the second refresh must skip set-url. - let second = state - .refresh(ORIGIN, |_| { - set_url_calls.fetch_add(1, Ordering::SeqCst); - async { Ok(()) } - }) - .await - .unwrap(); - assert_eq!(second.action, RemoteCredentialAction::Unchanged); - assert_eq!(second.token.unwrap().generation, 1); - assert_eq!(set_url_calls.load(Ordering::SeqCst), 1); - } - - #[tokio::test] - async fn refresh_embeds_again_when_the_source_mints_a_new_generation() { - // Tokens expire inside the margin, so every resolve re-mints. - let state = minting_state(chrono::Duration::minutes(5)); - let set_url_calls = AtomicUsize::new(0); - - let first = state - .refresh(ORIGIN, |_| { - set_url_calls.fetch_add(1, Ordering::SeqCst); - async { Ok(()) } - }) - .await - .unwrap(); - let second = state - .refresh(ORIGIN, |_| { - set_url_calls.fetch_add(1, Ordering::SeqCst); - async { Ok(()) } - }) - .await - .unwrap(); - - assert_eq!(first.token.unwrap().generation, 1); - assert_eq!(second.action, RemoteCredentialAction::Embedded); - assert_eq!(second.token.unwrap().generation, 2); - assert_eq!(set_url_calls.load(Ordering::SeqCst), 2); - } - - #[tokio::test] - async fn clone_seed_makes_the_first_refresh_a_no_op() { - let state = minting_state(chrono::Duration::minutes(60)); - let clone_token = state.source().unwrap().mint_for_clone().await.unwrap(); - state.record_embedded(clone_token).await; - - let outcome = state - .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) - .await - .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::Unchanged); - assert_eq!(outcome.token.unwrap().generation, 1); - } - - #[tokio::test] - async fn failed_set_url_does_not_record_the_new_generation() { - let state = minting_state(chrono::Duration::minutes(60)); - - let err = state - .refresh(ORIGIN, |_| async { - Err(crate::Error::message("set-url failed")) - }) - .await - .unwrap_err(); - assert!(err.to_string().contains("set-url failed")); - - // The generation was not recorded, so the retry embeds again instead - // of wrongly skipping. - let retried = state.refresh(ORIGIN, |_| async { Ok(()) }).await.unwrap(); - assert_eq!(retried.action, RemoteCredentialAction::Embedded); - assert_eq!(retried.token.unwrap().generation, 1); - } - - #[tokio::test] - async fn static_credentials_seeded_at_clone_skip_set_url() { - let source = InstallationTokenSource::for_origin( - &GitHubCredentials::Pat("ghp_pat".to_string()), - ORIGIN, - serde_json::json!({ "contents": "write" }), - ) - .unwrap(); - let state = PushCredentialState::new(Some(source)); - let clone_token = state.source().unwrap().mint_for_clone().await.unwrap(); - state.record_embedded(clone_token).await; - - let outcome = state - .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) - .await - .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::Unchanged); - assert!(outcome.token.unwrap().is_static()); - } - - #[tokio::test] - async fn mint_failure_maps_to_the_token_mint_failed_error() { - let state = PushCredentialState::new(Some(InstallationTokenSource::with_minter( - "owner/repo".to_string(), - Box::new(FailingMinter), - ))); - - let err = state - .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) - .await - .unwrap_err(); - assert!(err.to_string().contains("token_mint_failed"), "{err}"); - } - - #[test] - fn token_source_requires_managed_credentials_and_a_github_origin() { - assert!(build_token_source(None, Some(ORIGIN)).unwrap().is_none()); - let pat = GitHubCredentials::Pat("ghp_pat".to_string()); - assert!(build_token_source(Some(&pat), None).unwrap().is_none()); - assert!( - build_token_source(Some(&pat), Some("https://gitlab.com/owner/repo")) - .unwrap() - .is_none() - ); - assert!( - build_token_source(Some(&pat), Some(ORIGIN)) - .unwrap() - .is_some() - ); - } -} diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index dc6401723..f904ceec2 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -7,7 +7,6 @@ use std::sync::Arc; use std::time::Duration; use async_trait::async_trait; -use fabro_github::token_source::{InstallationTokenSource, TokenSnapshot}; use fabro_types::{CommandOutputStream, CommandTermination}; use fabro_util::shell; use fabro_util::workspace_glob::WorkspaceGlob; @@ -18,9 +17,6 @@ use tokio::task::JoinHandle; use tokio::time; use tokio_util::sync::CancellationToken; -use crate::git_retry::{self, CredentialContext, GitMessageClass, GitRetryReason, RetryPlan}; -use crate::push_credentials::{CredentialLease, PushCredentialState, RefreshErrorKind}; - /// Git command prefix that disables background maintenance. const GIT: &str = "git -c maintenance.auto=0 -c gc.auto=0"; @@ -284,12 +280,6 @@ macro_rules! delegate_sandbox { self.$field.refresh_push_credentials().await } - fn push_token_source( - &self, - ) -> Option> { - self.$field.push_token_source() - } - async fn set_autostop_interval(&self, minutes: i32) -> $crate::Result<()> { self.$field.set_autostop_interval(minutes).await } @@ -302,12 +292,8 @@ macro_rules! delegate_sandbox { self.$field.resume_setup_commands(run_branch) } - async fn git_push_ref( - &self, - refspec: &str, - plan: &$crate::RetryPlan, - ) -> Result<$crate::PushReport, $crate::PushError> { - self.$field.git_push_ref(refspec, plan).await + async fn git_push_ref(&self, refspec: &str) -> $crate::Result<()> { + self.$field.git_push_ref(refspec).await } async fn ssh_access_command(&self) -> $crate::Result> { @@ -1027,42 +1013,16 @@ pub struct GrepOptions { pub max_results: Option, } -/// What [`Sandbox::refresh_push_credentials`] did to the origin remote. -/// -/// Distinct from what the token *is* — the two are independent facts. A token -/// minted by another consumer and embedded here for the first time is an -/// `Embedded` action carrying a `Reused` provenance. -#[derive(Debug, Clone, Copy, PartialEq, Eq, strum::Display)] -#[strum(serialize_all = "snake_case")] -pub enum RemoteCredentialAction { - /// `set-url` ran with a different generation than last embedded. - Embedded, - /// The resolved generation matched the last embedded one; `set-url` was - /// skipped. - Unchanged, - /// No managed credentials to embed (no clone, no authenticated origin, or - /// no GitHub credentials). - None, -} - -/// Outcome of [`Sandbox::refresh_push_credentials`]: what this call did to the -/// remote, and the non-secret description of the token embedded in it. -/// `token` is `None` only when `action` is [`RemoteCredentialAction::None`]. +/// Outcome of [`Sandbox::refresh_push_credentials`]: whether a fresh token was +/// actually minted and applied to the origin remote, or the call was a no-op +/// (no clone, no authenticated origin, or no GitHub App credentials to rotate). +/// Lets callers log accurately instead of assuming every `Ok` re-minted. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct RefreshOutcome { - pub action: RemoteCredentialAction, - pub token: Option, -} - -impl RefreshOutcome { - /// No managed credentials to refresh. - #[must_use] - pub fn none() -> Self { - Self { - action: RemoteCredentialAction::None, - token: None, - } - } +pub enum RefreshOutcome { + /// A fresh token was minted and the origin remote URL was updated. + Refreshed, + /// Nothing to refresh (no clone / no origin / no managed credentials). + Skipped, } #[async_trait] @@ -1282,22 +1242,11 @@ pub trait Sandbox: Send + Sync { } /// Refresh git push credentials (e.g. rotate an expiring GitHub App token). - /// Default is a no-op; Docker/Daytona override to resolve a token through - /// the shared source and update the remote URL when the embedded - /// generation is stale. Returns [`RefreshOutcome`] so callers can tell - /// what happened to the remote and which token it carries. + /// Default is a no-op; Docker/Daytona override to update the remote URL + /// with a fresh token. Returns [`RefreshOutcome`] so callers can tell + /// an actual re-mint from a skipped no-op. async fn refresh_push_credentials(&self) -> crate::Result { - Ok(RefreshOutcome::none()) - } - - /// The shared installation-token source feeding this sandbox's push - /// credentials, when the provider manages GitHub credentials. - /// - /// Consumers outside the sandbox (e.g. the run-metadata writer) share - /// this source so every GitHub-token consumer for the origin repository - /// reuses one cached token instead of minting its own. - fn push_token_source(&self) -> Option> { - None + Ok(RefreshOutcome::Skipped) } /// Set the auto-stop interval in minutes (0 to disable). @@ -1319,18 +1268,11 @@ pub trait Sandbox: Send + Sync { Vec::new() } - /// Push a full refspec to origin from inside the sandbox, retrying per - /// `plan` with a pinned credential generation. Failures keep their - /// attempt history in the returned [`PushError`]. - async fn git_push_ref( - &self, - _refspec: &str, - _plan: &RetryPlan, - ) -> Result { - Err(PushError { - report: PushReport::default(), - error: crate::Error::message("git_push_ref not implemented for this sandbox"), - }) + /// Push a full refspec to origin from inside the sandbox. + async fn git_push_ref(&self, _refspec: &str) -> crate::Result<()> { + Err(crate::Error::message( + "git_push_ref not implemented for this sandbox", + )) } /// Return an SSH command string for connecting to this sandbox, if @@ -1570,827 +1512,30 @@ pub(crate) async fn fetch_source_run_ref( Err(crate::Error::message(last_error)) } -/// One push attempt inside a retried push operation. Runtime detail only — -/// the durable serialized shape lives in `fabro-types` and the workflow layer -/// owns the conversion. -#[derive(Debug, Clone)] -pub struct PushAttempt { - /// 1-based attempt number within this operation. - pub attempt: u32, - pub started_at: chrono::DateTime, - pub success: bool, - /// The classifier's verdict for a failed attempt — recorded on the - /// terminal attempt too; whether a retry actually followed is positional - /// (every entry except the last). - pub retry_reason: Option, - /// Redacted, bounded output tail; failed attempts only. - pub exec_output_tail: Option, - /// The token embedded in the remote during this attempt. - pub token: Option, - /// What `ensure_embedded` did to the remote this attempt. - pub credential_action: Option, - /// A mint or `set-url` failure this attempt pushed through. - pub refresh_error: Option, -} - -/// The attempt history of one push operation. -#[derive(Debug, Clone, Default)] -pub struct PushReport { - pub attempts: Vec, -} - -/// A failed push operation: the final typed error plus the attempt history. -/// The error type stays the safety boundary for output tails. -#[derive(Debug)] -pub struct PushError { - pub report: PushReport, - pub error: crate::Error, -} - -impl std::fmt::Display for PushError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.error.fmt(f) - } -} - -impl std::error::Error for PushError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - Some(&self.error) - } -} - -/// Classify a failed push attempt by the failure's rendered output. -fn classify_push_error(error: &crate::Error, cred: CredentialContext) -> Option { - let class = match error { - crate::Error::Exec { result, .. } => { - let by_stderr = git_retry::classify_message(&result.stderr, cred); - if by_stderr == GitMessageClass::Unknown { - git_retry::classify_message(&result.stdout, cred) - } else { - by_stderr - } - } - other => git_retry::classify_message(&crate::display_for_log(other), cred), - }; - class.retry_reason() -} - -/// Whether a failed push attempt has the 404/auth-failure shape that a -/// drifted or missing embedded token also produces. -fn push_failure_looks_auth_shaped(error: &crate::Error) -> bool { - match error { - crate::Error::Exec { result, .. } => { - git_retry::matches_auth_failure_hints(&result.stderr) - || git_retry::matches_auth_failure_hints(&result.stdout) - } - other => git_retry::matches_auth_failure_hints(&crate::display_for_log(other)), - } -} - /// Helper for sandbox implementations that manage git internally. -/// -/// Pushes a refspec to origin via `exec_command` inside the sandbox, -/// retrying per `plan` with one pinned credential generation for the whole -/// operation. `credentials` is the provider's push-credential state plus the -/// origin URL; `None` pushes with whatever the remote already carries (the -/// local sandbox, or a workspace without managed credentials). +/// Pushes a refspec to origin via exec_command inside the sandbox. // Async-safe by construction: the span is attached to the future, so it // follows the task across worker threads, and the providers' `exec_command: // entered` lines inherit it — the log renders as // `git_op{op=push}: exec_command: entered ...`. #[tracing::instrument(name = "git_op", skip_all, fields(op = "push"))] -pub(crate) async fn git_push_via_exec( - sandbox: &dyn Sandbox, - credentials: Option<(&PushCredentialState, &str)>, - refspec: &str, - plan: &RetryPlan, -) -> Result { - use CredentialContext; - use CredentialLease; - - // The lease pins one token generation and owns the embed mutex for the - // whole operation; no concurrent refresh can re-embed mid-operation, and - // no attempt can cross the refresh margin and restart the replication - // clock. - let mut lease: Option<(CredentialLease<'_>, &str)> = match credentials { - Some((state, origin_url)) => match state.lease().await { - Ok(lease) => Some((lease, origin_url)), - Err(error) => { - return Err(PushError { - report: PushReport::default(), - error, - }); - } - }, - None => None, - }; - - let start = time::Instant::now(); - let deadline = plan.effective_deadline(start); - let mut attempts: Vec = Vec::new(); - let mut force_reembed = false; - let mut drift_repaired = false; +pub async fn git_push_via_exec(sandbox: &dyn Sandbox, refspec: &str) -> crate::Result<()> { + if let Err(e) = sandbox.refresh_push_credentials().await { + tracing::warn!( + refspec = %refspec, + error = %crate::display_for_log(&e), + "Failed to refresh push credentials before git push" + ); + } let cmd = format!("{GIT} push origin {}", shell_quote(refspec)); let label = format!("git push origin {refspec}"); - - loop { - let attempt_number = u32::try_from(attempts.len()).unwrap_or(u32::MAX) + 1; - let started_at = chrono::Utc::now(); - let (token, credential_action, refresh_error) = match lease.as_mut() { - Some((lease, origin_url)) => { - let ensured = lease - .ensure_embedded(sandbox, origin_url, force_reembed) - .await; - force_reembed = false; - (ensured.token, Some(ensured.action), ensured.refresh_error) - } - None => (None, None, None), - }; - - let timeout = plan - .attempt_timeout(deadline) - .unwrap_or(Duration::from_mins(1)); - let timeout_ms = u64::try_from(timeout.as_millis()).unwrap_or(u64::MAX); - let push_result = match sandbox - .exec_command(&cmd, timeout_ms, None, None, None) - .await - { - Ok(result) => result.into_result(&label).map(|_| ()), - Err(err) => Err(crate::Error::context(label.clone(), err)), - }; - - match push_result { - Ok(()) => { - attempts.push(PushAttempt { - attempt: attempt_number, - started_at, - success: true, - retry_reason: None, - exec_output_tail: None, - token, - credential_action, - refresh_error, - }); - tracing::info!( - refspec = %refspec, - attempt = attempt_number, - token_generation = token.map(|token| token.generation), - token_age_ms = token.and_then(|token| token.age_ms()), - "Pushed git ref to origin" - ); - return Ok(PushReport { attempts }); - } - Err(error) => { - // Drift recovery: the tracked generation is local belief, and - // agent code inside the sandbox can rewrite `origin`. The - // first auth/not-found failure earns one forced re-embed of - // the pinned token, inside the same retry budget. - if !drift_repaired && lease.is_some() && push_failure_looks_auth_shaped(&error) { - drift_repaired = true; - force_reembed = true; - } - let cred = CredentialContext::from_snapshot(token.as_ref()); - let retry_reason = classify_push_error(&error, cred); - attempts.push(PushAttempt { - attempt: attempt_number, - started_at, - success: false, - retry_reason, - exec_output_tail: error.default_redacted_output_tail(), - token, - credential_action, - refresh_error, - }); - - let exhausted = attempt_number >= plan.max_attempts.max(1); - let Some(reason) = retry_reason.filter(|_| !exhausted) else { - return Err(PushError { - report: PushReport { attempts }, - error, - }); - }; - let delay = plan.backoff.delay_for_attempt(attempt_number); - if deadline.is_some_and(|deadline| { - delay >= deadline.saturating_duration_since(time::Instant::now()) - }) { - return Err(PushError { - report: PushReport { attempts }, - error, - }); - } - // The failure text can carry git stderr, so log the category - // rather than the message. - tracing::warn!( - refspec = %refspec, - attempt = attempt_number, - max_attempts = plan.max_attempts, - reason = %reason, - token_generation = token.map(|token| token.generation), - token_age_ms = token.and_then(|token| token.age_ms()), - delay_ms = u64::try_from(delay.as_millis()).unwrap_or(u64::MAX), - "Git push failed, retrying with the same token" - ); - time::sleep(delay).await; - } - } - } -} - -#[cfg(test)] -mod push_tests { - use std::collections::VecDeque; - use std::sync::Mutex; - use std::sync::atomic::{AtomicUsize, Ordering}; - - use chrono::Utc; - use fabro_github::InstallationToken; - use fabro_github::token_source::{ - InstallationTokenMinter, InstallationTokenSource, REFRESH_MARGIN, - }; - use tokio::sync::Mutex as AsyncMutex; - - use super::*; - use crate::git_retry::{GitRetryReason, RetryPlan}; - use crate::push_credentials::{PushCredentialState, RefreshErrorKind}; - - const ORIGIN: &str = "https://github.com/fabro-testing/repo"; - const REFSPEC: &str = "refs/heads/fabro/run/01M0DH033P2XSTHAGVBHG6922F"; - - fn ok_exec() -> ExecResult { - ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 5, - } - } - - fn failed_exec(stderr: &str) -> ExecResult { - ExecResult { - stdout: String::new(), - stderr: stderr.to_string(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 5, - } - } - - /// Sandbox stub that scripts `git push` results and records the exec - /// commands the push driver runs. `git remote set-url` execs succeed - /// unless scripted otherwise. - struct ScriptedGitSandbox { - push_results: Mutex>, - set_url_results: Mutex>, - push_commands: Mutex>, - set_url_commands: Mutex>, - } - - impl ScriptedGitSandbox { - fn new(push_results: Vec) -> Self { - Self { - push_results: Mutex::new(push_results.into()), - set_url_results: Mutex::new(VecDeque::new()), - push_commands: Mutex::new(Vec::new()), - set_url_commands: Mutex::new(Vec::new()), - } - } - - fn with_set_url_results(self, results: Vec) -> Self { - *self.set_url_results.lock().unwrap() = results.into(); - self - } - - fn push_count(&self) -> usize { - self.push_commands.lock().unwrap().len() - } - - fn set_url_commands(&self) -> Vec { - self.set_url_commands.lock().unwrap().clone() - } - } - - #[async_trait] - impl Sandbox for ScriptedGitSandbox { - async fn exec_command( - &self, - command: &str, - _timeout_ms: u64, - _working_dir: Option<&str>, - _env_vars: Option<&HashMap>, - _cancel_token: Option, - ) -> crate::Result { - if command.contains("remote set-url") { - self.set_url_commands - .lock() - .unwrap() - .push(command.to_string()); - return Ok(self - .set_url_results - .lock() - .unwrap() - .pop_front() - .unwrap_or_else(ok_exec)); - } - assert!( - command.contains("push origin"), - "unexpected exec: {command}" - ); - self.push_commands.lock().unwrap().push(command.to_string()); - Ok(self - .push_results - .lock() - .unwrap() - .pop_front() - .expect("push script exhausted")) - } - - async fn read_file_bytes(&self, _path: &str) -> crate::Result> { - unimplemented!() - } - - async fn write_file(&self, _path: &str, _content: &str) -> crate::Result<()> { - unimplemented!() - } - - async fn delete_file(&self, _path: &str) -> crate::Result<()> { - unimplemented!() - } - - async fn file_exists(&self, _path: &str) -> crate::Result { - unimplemented!() - } - - async fn list_directory( - &self, - _path: &str, - _depth: Option, - ) -> crate::Result> { - unimplemented!() - } - - async fn grep( - &self, - _pattern: &str, - _path: &str, - _options: &GrepOptions, - ) -> crate::Result> { - unimplemented!() - } - - async fn download_file_to_local( - &self, - _remote_path: &str, - _local_path: &Path, - ) -> crate::Result<()> { - unimplemented!() - } - - async fn upload_file_from_local( - &self, - _local_path: &Path, - _remote_path: &str, - ) -> crate::Result<()> { - unimplemented!() - } - - async fn initialize(&self) -> crate::Result<()> { - Ok(()) - } - - async fn cleanup(&self) -> crate::Result<()> { - Ok(()) - } - - fn working_directory(&self) -> &'static str { - "/workspace" - } - - fn platform(&self) -> &'static str { - "linux" - } - - fn os_version(&self) -> String { - "linux".to_string() - } - } - - enum MintAction { - Token(&'static str, chrono::Duration), - Error(&'static str), - } - - struct ScriptedMinter { - calls: AtomicUsize, - script: AsyncMutex>, - } - - impl ScriptedMinter { - fn new(script: Vec) -> std::sync::Arc { - std::sync::Arc::new(Self { - calls: AtomicUsize::new(0), - script: AsyncMutex::new(script.into()), - }) - } - - fn calls(&self) -> usize { - self.calls.load(Ordering::SeqCst) - } - } - - struct SharedMinter(std::sync::Arc); - - #[async_trait] - impl InstallationTokenMinter for SharedMinter { - async fn mint(&self) -> anyhow::Result { - self.0.calls.fetch_add(1, Ordering::SeqCst); - match self.0.script.lock().await.pop_front().expect("mint script") { - MintAction::Token(token, ttl) => Ok(InstallationToken { - token: token.to_string(), - expires_at: Utc::now() + ttl, - }), - MintAction::Error(message) => Err(anyhow::anyhow!(message)), - } - } - } - - fn minting_state( - script: Vec, - ) -> (PushCredentialState, std::sync::Arc) { - let minter = ScriptedMinter::new(script); - let source = InstallationTokenSource::with_minter( - "fabro-testing/repo".to_string(), - Box::new(SharedMinter(std::sync::Arc::clone(&minter))), - ); - (PushCredentialState::new(Some(source)), minter) - } - - async fn seed_clone_token(state: &PushCredentialState) { - let clone_token = state - .source() - .expect("state has a source") - .mint_for_clone() - .await - .expect("clone mint succeeds"); - state.record_embedded(clone_token).await; - } - - /// Regression for run `01M0DH033P2XSTHAGVBHG6922F` (the push variant of - /// `clone_not_found_after_a_successful_mint_is_retried`): GitHub rejected - /// pushes with 404 "Repository not found" milliseconds after a token - /// mint. The retry must reuse the same token — replication of a given - /// token only makes progress — and recover inside the plan's budget. - #[tokio::test(start_paused = true)] - async fn push_not_found_after_a_successful_mint_is_retried_with_the_same_token() { - let (state, minter) = minting_state(vec![MintAction::Token( - "ghs_gen1", - chrono::Duration::minutes(60), - )]); - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - ok_exec(), - ]); - - let report = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) + sandbox + .exec_command(&cmd, 60_000, None, None, None) .await - .expect("push should recover within the checkpoint plan"); - - assert_eq!(report.attempts.len(), 3); - assert_eq!(minter.calls(), 1, "retries must not re-mint"); - for attempt in &report.attempts { - assert_eq!(attempt.token.expect("token recorded").generation, 1); - } - assert_eq!( - report.attempts[0].retry_reason, - Some(GitRetryReason::TokenReplication) - ); - assert!(report.attempts[0].exec_output_tail.is_some()); - assert_eq!( - report.attempts[0].credential_action, - Some(RemoteCredentialAction::Embedded), - "first attempt embeds the resolved token" - ); - assert!(report.attempts[2].success); - assert!(report.attempts[2].exec_output_tail.is_none()); - assert_eq!(sandbox.push_count(), 3); - } - - /// The publish plan gives the terminal push a real budget: four - /// replication-lag failures still recover on the fifth attempt. - #[tokio::test(start_paused = true)] - async fn publish_plan_survives_four_not_found_failures() { - let (state, minter) = minting_state(vec![MintAction::Token( - "ghs_gen1", - chrono::Duration::minutes(60), - )]); - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - ok_exec(), - ]); - - let report = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::publish_push(), - ) - .await - .expect("push should recover within the publish plan"); - - assert_eq!(report.attempts.len(), 5); - assert_eq!(minter.calls(), 1); - assert!(report.attempts[4].success); - } - - /// Margin-boundary pinning: a token resolved just above the refresh - /// margin stays pinned through a full retry sequence — the operation - /// never re-resolves mid-flight, so no fresh mint can restart the - /// replication clock. - #[tokio::test(start_paused = true)] - async fn token_resolved_just_above_the_margin_stays_pinned_through_retries() { - let ttl = REFRESH_MARGIN + Duration::from_secs(5); - let (state, minter) = minting_state(vec![MintAction::Token( - "ghs_gen1", - chrono::Duration::from_std(ttl).unwrap(), - )]); - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - ok_exec(), - ]); - - let report = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect("push should recover"); - - assert_eq!(minter.calls(), 1, "no mid-operation mint"); - let generations: Vec = report - .attempts - .iter() - .map(|attempt| attempt.token.expect("token recorded").generation) - .collect(); - assert_eq!(generations, vec![1, 1, 1]); - } - - #[tokio::test(start_paused = true)] - async fn static_credential_auth_failure_fails_fast() { - let source = InstallationTokenSource::for_origin( - &fabro_github::GitHubCredentials::Pat("ghp_pat".to_string()), - ORIGIN, - serde_json::json!({ "contents": "write" }), - ) - .unwrap(); - let state = PushCredentialState::new(Some(source)); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![failed_exec( - "fatal: Authentication failed for 'https://github.com/fabro-testing/repo'", - )]); - - let push_error = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::publish_push(), - ) - .await - .expect_err("static credentials cannot become valid by waiting"); - - assert_eq!(push_error.report.attempts.len(), 1); - assert_eq!(push_error.report.attempts[0].retry_reason, None); - assert!(push_error.report.attempts[0].token.unwrap().is_static()); - } - - /// Clone seeding closes the "nothing was ever embedded" hole: when the - /// first refresh mint fails, the push falls back to the clone token - /// recorded as last-embedded instead of aborting. - #[tokio::test(start_paused = true)] - async fn mint_failure_falls_back_to_the_clone_token() { - let (state, minter) = minting_state(vec![ - MintAction::Token("ghs_clone", chrono::Duration::minutes(5)), - // The clone token is inside the margin, so the lease acquisition - // re-mints — and fails. So does the attempt-level retry. - MintAction::Error("mint failed"), - MintAction::Error("mint failed"), - ]); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); - - let report = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect("push proceeds with the still-valid clone token"); - - assert_eq!(minter.calls(), 3); - let attempt = &report.attempts[0]; - assert!(attempt.success); - assert_eq!(attempt.refresh_error, Some(RefreshErrorKind::Mint)); - assert_eq!( - attempt.token.expect("fallback token recorded").generation, - 1, - "attempts classify against the embedded clone token, never None" - ); - assert_eq!( - attempt.credential_action, - Some(RemoteCredentialAction::Unchanged) - ); - } - - #[tokio::test(start_paused = true)] - async fn acquisition_fails_when_mint_fails_and_nothing_was_embedded() { - let (state, _minter) = minting_state(vec![MintAction::Error("mint failed")]); - let sandbox = ScriptedGitSandbox::new(vec![]); - - let push_error = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect_err("there is nothing to push with"); - - assert!(push_error.report.attempts.is_empty()); - assert!(push_error.error.to_string().contains("token_mint_failed")); - assert_eq!(sandbox.push_count(), 0); - } - - /// Late-mint recovery: the fallback push fails on the expired-ish old - /// token, a later attempt's resolve retry succeeds, the target embeds, - /// and the push recovers — all inside one operation's budget. - #[tokio::test(start_paused = true)] - async fn late_mint_recovery_lands_the_target_inside_the_operation() { - let (state, minter) = minting_state(vec![ - MintAction::Token("ghs_gen1", chrono::Duration::minutes(5)), - MintAction::Error("mint failed"), - MintAction::Error("mint failed"), - MintAction::Token("ghs_gen2", chrono::Duration::minutes(60)), - ]); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("fatal: Authentication failed for 'https://github.com'"), - ok_exec(), - ]); - - let report = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect("late mint should recover the push"); - - assert_eq!(minter.calls(), 4); - let first = &report.attempts[0]; - assert_eq!(first.refresh_error, Some(RefreshErrorKind::Mint)); - assert_eq!(first.token.unwrap().generation, 1); - let second = &report.attempts[1]; - assert!(second.success); - assert_eq!(second.refresh_error, None); - assert_eq!(second.token.unwrap().generation, 2); - assert_eq!( - second.credential_action, - Some(RemoteCredentialAction::Embedded), - "the report shows the single generation transition" - ); - } - - /// A failed `set-url` defers the embed: attempt 1 records the old - /// generation with the refresh error, attempt 2 lands the target, and the - /// report shows the one generation transition via `credential_action`. - #[tokio::test(start_paused = true)] - async fn set_url_failure_defers_the_embed_until_the_next_attempt() { - let (state, minter) = minting_state(vec![ - MintAction::Token("ghs_gen1", chrono::Duration::minutes(5)), - MintAction::Token("ghs_gen2", chrono::Duration::minutes(60)), - ]); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("error: RPC failed; connection reset by peer"), - ok_exec(), - ]) - .with_set_url_results(vec![failed_exec("error: could not lock config file")]); - - let report = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect("deferred embed should land on the retry"); - - assert_eq!( - minter.calls(), - 2, - "the successful resolve is never repeated" - ); - let first = &report.attempts[0]; - assert_eq!(first.refresh_error, Some(RefreshErrorKind::SetUrl)); - assert_eq!( - first.token.unwrap().generation, - 1, - "pin stays on the old token" - ); - assert_eq!( - first.credential_action, - Some(RemoteCredentialAction::Unchanged) - ); - let second = &report.attempts[1]; - assert_eq!(second.token.unwrap().generation, 2); - assert_eq!( - second.credential_action, - Some(RemoteCredentialAction::Embedded) - ); - assert!(second.success); - } - - /// Remote drift: agent code rewrote `origin`, so the push fails on auth - /// even though the tracked generation looks current. The first - /// auth-shaped failure earns one forced re-embed of the pinned token. - #[tokio::test(start_paused = true)] - async fn remote_drift_gets_one_forced_reembed_of_the_pinned_token() { - let (state, minter) = minting_state(vec![MintAction::Token( - "ghs_gen1", - chrono::Duration::minutes(60), - )]); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec( - "fatal: could not read Username for 'https://github.com': No such device or address\nremote: Repository not found.", - ), - ok_exec(), - ]); - - let report = git_push_via_exec( - &sandbox, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect("drift repair should restore the pinned credentials"); - - assert_eq!(minter.calls(), 1, "drift repair re-embeds, never re-mints"); - assert_eq!( - report.attempts[0].credential_action, - Some(RemoteCredentialAction::Unchanged), - "before the failure the tracked generation matched" - ); - assert_eq!( - report.attempts[1].credential_action, - Some(RemoteCredentialAction::Embedded), - "the retry force-re-embeds the pinned token" - ); - let set_urls = sandbox.set_url_commands(); - assert_eq!(set_urls.len(), 1); - assert!(set_urls[0].contains("ghs_gen1")); - } - - #[tokio::test(start_paused = true)] - async fn push_without_managed_credentials_reports_no_token() { - let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); - - let report = git_push_via_exec(&sandbox, None, REFSPEC, &RetryPlan::checkpoint_push()) - .await - .expect("push succeeds"); - - assert_eq!(report.attempts.len(), 1); - assert_eq!(report.attempts[0].token, None); - assert_eq!(report.attempts[0].credential_action, None); - } - - #[tokio::test(start_paused = true)] - async fn unauthenticated_auth_failure_is_permanent() { - let sandbox = ScriptedGitSandbox::new(vec![failed_exec( - "fatal: Authentication failed for 'https://github.com/fabro-testing/repo'", - )]); - - let push_error = git_push_via_exec(&sandbox, None, REFSPEC, &RetryPlan::publish_push()) - .await - .expect_err("no credentials to wait on"); - - assert_eq!(push_error.report.attempts.len(), 1); - assert_eq!(push_error.report.attempts[0].retry_reason, None); - } + .map_err(|e| crate::Error::context(label.clone(), e))? + .into_result(&label)?; + tracing::info!(refspec = %refspec, "Pushed git ref to origin"); + Ok(()) } #[cfg(test)] diff --git a/lib/components/fabro-workflow/src/error.rs b/lib/components/fabro-workflow/src/error.rs index 3171b5008..21833a8df 100644 --- a/lib/components/fabro-workflow/src/error.rs +++ b/lib/components/fabro-workflow/src/error.rs @@ -309,10 +309,6 @@ pub enum Error { message: String, failure_class: FailureCategory, exec_output_tail: Option, - /// Structured context lines appended after the source chain in - /// `causes()` — e.g. one line per push attempt on a publish push - /// failure. - extra_causes: Vec, #[source] source: Option, }, @@ -361,7 +357,6 @@ impl Error { message, failure_class, exec_output_tail, - extra_causes: Vec::new(), source: None, } } @@ -383,7 +378,6 @@ impl Error { message, failure_class, exec_output_tail, - extra_causes: Vec::new(), source: Some(source), } } @@ -460,42 +454,12 @@ impl Error { Self::stage_with_source(ErrorStage::Publish, message, source, exec_output_tail) } - /// Build a publish error with an explicitly determined failure category, - /// for callers that know more than message sniffing can recover — e.g. - /// exhausted push retries whose attempts all classified as transient. - /// `extra_causes` lines land after the source chain in the failure - /// detail (one line per push attempt). - pub fn publish_with_source_and_class( - message: impl Into, - source: impl Into, - failure_class: FailureCategory, - exec_output_tail: Option, - extra_causes: Vec, - ) -> Self { - Self::Stage { - stage: ErrorStage::Publish, - message: message.into(), - failure_class, - exec_output_tail, - extra_causes, - source: Some(SharedError::new(source.into())), - } - } - #[must_use] pub fn causes(&self) -> Vec { match self { - Self::Stage { - source, - extra_causes, - .. - } => { - let mut causes = source - .as_ref() - .map_or_else(Vec::new, |source| collect_chain(source)); - causes.extend(extra_causes.iter().cloned()); - causes - } + Self::Stage { source, .. } => source + .as_ref() + .map_or_else(Vec::new, |source| collect_chain(source)), Self::Template { source, .. } => collect_chain(source), Self::ScriptInterpolation { source, .. } => collect_chain(source), Self::Llm(err) => collect_causes(err), diff --git a/lib/components/fabro-workflow/src/event.rs b/lib/components/fabro-workflow/src/event.rs index 19c61c43d..a5c1f583e 100644 --- a/lib/components/fabro-workflow/src/event.rs +++ b/lib/components/fabro-workflow/src/event.rs @@ -10,7 +10,7 @@ mod test_support; pub use fabro_types::{EventBody, RunNoticeCode, RunNoticeLevel}; -pub use self::convert::{git_push_attempt_props, to_run_event, to_run_event_at}; +pub use self::convert::{to_run_event, to_run_event_at}; pub use self::emitter::Emitter; pub use self::events::Event; pub use self::names::event_name; diff --git a/lib/components/fabro-workflow/src/event/convert.rs b/lib/components/fabro-workflow/src/event/convert.rs index ded494571..6403d8933 100644 --- a/lib/components/fabro-workflow/src/event/convert.rs +++ b/lib/components/fabro-workflow/src/event/convert.rs @@ -23,36 +23,6 @@ fn stage_status_from_string(status: &str) -> StageOutcome { }) } -/// Project the sandbox layer's runtime push attempts into the durable -/// `git.push` attempt shape. -/// -/// This is the only place the runtime attempt record crosses into stored -/// events: the token snapshot flattens into the three flat `token_*` fields -/// (a nested provenance enum never appears in stored events), and the retry -/// classifier's verdict becomes `classified_reason`. -pub fn git_push_attempt_props( - attempts: &[fabro_sandbox::PushAttempt], -) -> Vec { - attempts - .iter() - .map(|attempt| fabro_types::GitPushAttemptProps { - attempt: attempt.attempt, - started_at: attempt.started_at, - success: attempt.success, - classified_reason: attempt.retry_reason.map(|reason| reason.to_string()), - exec_output_tail: attempt.exec_output_tail.clone(), - token_generation: attempt.token.map(|token| token.generation), - token_provenance: attempt.token.map(|token| token.provenance.to_string()), - token_age_ms: attempt - .token - .and_then(|token| token.age_at(attempt.started_at)) - .map(|age| u64::try_from(age.as_millis()).unwrap_or(u64::MAX)), - credential_action: attempt.credential_action.map(|action| action.to_string()), - refresh_error: attempt.refresh_error.map(|kind| kind.to_string()), - }) - .collect() -} - fn event_body_from_event(event: &Event) -> EventBody { match event { Event::RunCreated { @@ -550,12 +520,10 @@ fn event_body_from_event(event: &Event) -> EventBody { branch, success, exec_output_tail, - attempts, } => EventBody::GitPush(fabro_types::GitPushProps { branch: branch.clone(), success: *success, exec_output_tail: exec_output_tail.clone(), - attempts: attempts.clone(), }), Event::GitFetch { branch, success } => EventBody::GitFetch(fabro_types::GitFetchProps { branch: branch.clone(), @@ -2202,141 +2170,12 @@ mod tests { } } - /// The `git.push` attempts contract: every runtime attempt fact - /// round-trips through `GitPushAttemptProps`, the token snapshot is - /// flattened to the three flat token fields (a nested provenance enum - /// never appears in stored events), and optional failure fields are - /// omitted when absent. - #[test] - fn git_push_attempts_round_trip_through_the_durable_shape() { - let started_at = Utc::now(); - let minted_at = started_at - chrono::Duration::milliseconds(180); - let expires_at = started_at + chrono::Duration::minutes(60); - let attempts = git_push_attempt_props(&[ - fabro_sandbox::PushAttempt { - attempt: 1, - started_at, - success: false, - retry_reason: Some(fabro_sandbox::GitRetryReason::TokenReplication), - exec_output_tail: Some(exec_tail()), - token: Some(fabro_sandbox::TokenSnapshot { - generation: 14, - provenance: fabro_sandbox::TokenProvenance::Minted { - minted_at, - expires_at, - }, - }), - credential_action: Some(fabro_sandbox::RemoteCredentialAction::Embedded), - refresh_error: None, - }, - // Terminal classified failure with a refresh error: the last - // attempt carries its classification too. - fabro_sandbox::PushAttempt { - attempt: 2, - started_at: started_at + chrono::Duration::seconds(3), - success: false, - retry_reason: Some(fabro_sandbox::GitRetryReason::TransientInfra), - exec_output_tail: Some(exec_tail()), - token: Some(fabro_sandbox::TokenSnapshot { - generation: 14, - provenance: fabro_sandbox::TokenProvenance::Reused { - minted_at, - expires_at, - }, - }), - credential_action: Some(fabro_sandbox::RemoteCredentialAction::Unchanged), - refresh_error: Some(fabro_sandbox::RefreshErrorKind::SetUrl), - }, - ]); - - let stored = to_run_event(&fixtures::RUN_1, &Event::GitPush { - branch: "fabro/run/01M0DH033P2XSTHAGVBHG6922F".to_string(), - success: false, - exec_output_tail: Some(exec_tail()), - attempts: attempts.clone(), - }); - - let json = serde_json::to_value(&stored).unwrap(); - let serialized = &json["properties"]["attempts"]; - assert_eq!(serialized[0]["attempt"], 1); - assert_eq!(serialized[0]["classified_reason"], "token_replication"); - assert_eq!(serialized[0]["token_generation"], 14); - assert_eq!(serialized[0]["token_provenance"], "minted"); - assert_eq!(serialized[0]["token_age_ms"], 180); - assert_eq!(serialized[0]["credential_action"], "embedded"); - assert!(serialized[0].get("refresh_error").is_none()); - assert_eq!(serialized[1]["classified_reason"], "transient_infra"); - assert_eq!(serialized[1]["token_provenance"], "reused"); - assert_eq!(serialized[1]["refresh_error"], "set_url"); - // The provenance enum never nests in stored events. - assert!(serialized[0].get("token").is_none()); - - let round_tripped: ::fabro_types::RunEvent = serde_json::from_value(json).unwrap(); - match round_tripped.body { - EventBody::GitPush(props) => { - assert!(!props.success); - assert_eq!(props.attempts, attempts); - } - other => panic!("expected GitPush body, got {other:?}"), - } - } - - #[test] - fn successful_single_attempt_push_omits_failure_fields() { - let attempts = git_push_attempt_props(&[fabro_sandbox::PushAttempt { - attempt: 1, - started_at: Utc::now(), - success: true, - retry_reason: None, - exec_output_tail: None, - token: Some(fabro_sandbox::TokenSnapshot { - generation: 0, - provenance: fabro_sandbox::TokenProvenance::Static, - }), - credential_action: Some(fabro_sandbox::RemoteCredentialAction::Unchanged), - refresh_error: None, - }]); - let stored = to_run_event(&fixtures::RUN_1, &Event::GitPush { - branch: "fabro/run/run-1".to_string(), - success: true, - exec_output_tail: None, - attempts, - }); - - let json = serde_json::to_value(&stored).unwrap(); - let attempt = &json["properties"]["attempts"][0]; - assert_eq!(attempt["success"], true); - assert_eq!(attempt["token_provenance"], "static"); - for absent in [ - "classified_reason", - "exec_output_tail", - "token_age_ms", - "refresh_error", - ] { - assert!(attempt.get(absent).is_none(), "{absent} should be omitted"); - } - } - - /// Events stored before attempts were recorded deserialize with the field - /// absent; the pre-existing three fields are untouched. - #[test] - fn stored_git_push_without_attempts_still_deserializes() { - let json = serde_json::json!({ - "branch": "fabro/run/old", - "success": true - }); - let props: fabro_types::GitPushProps = serde_json::from_value(json).unwrap(); - assert!(props.attempts.is_empty()); - assert!(props.exec_output_tail.is_none()); - } - #[test] fn git_push_maps_exec_output_tail_to_props() { let stored = to_run_event(&fixtures::RUN_1, &Event::GitPush { branch: "refs/heads/run:refs/heads/run".to_string(), success: false, exec_output_tail: Some(exec_tail()), - attempts: Vec::new(), }); match stored.body { diff --git a/lib/components/fabro-workflow/src/event/events.rs b/lib/components/fabro-workflow/src/event/events.rs index 7d18ec911..a38cefbd0 100644 --- a/lib/components/fabro-workflow/src/event/events.rs +++ b/lib/components/fabro-workflow/src/event/events.rs @@ -432,10 +432,6 @@ pub enum Event { success: bool, #[serde(default, skip_serializing_if = "Option::is_none")] exec_output_tail: Option, - /// Per-attempt history of the push operation, already projected to - /// the durable shape by the emit site. - #[serde(default, skip_serializing_if = "Vec::is_empty")] - attempts: Vec, }, GitFetch { branch: String, @@ -1234,16 +1230,14 @@ impl Event { branch, success, exec_output_tail, - attempts, } => { if *success { - debug!(branch, attempts = attempts.len(), "Git push succeeded"); + debug!(branch, "Git push succeeded"); } else { let tail = fabro_types::ExecOutputTail::trace_summary(exec_output_tail.as_ref()); warn!( branch, - attempts = attempts.len(), exec_output_tail_present = tail.present, exec_stdout_tail_bytes = tail.stdout_bytes, exec_stderr_tail_bytes = tail.stderr_bytes, diff --git a/lib/components/fabro-workflow/src/handler/llm/acp.rs b/lib/components/fabro-workflow/src/handler/llm/acp.rs index 4edcf3e81..10837272a 100644 --- a/lib/components/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/components/fabro-workflow/src/handler/llm/acp.rs @@ -11,10 +11,8 @@ use fabro_acp::{ render_stop_reason, }; use fabro_agent::{ - AgentEvent, RefreshOutcome, RemoteCredentialAction, Sandbox, StaticEnvProvider, SteeringItem, - ToolEnvProvider, + AgentEvent, RefreshOutcome, Sandbox, StaticEnvProvider, SteeringItem, ToolEnvProvider, }; -use fabro_github::token_source::REFRESH_MARGIN; use fabro_graphviz::graph::Node; use fabro_static::EnvVars; use fabro_types::{ @@ -34,13 +32,8 @@ use crate::handler::NodeTimeoutPolicy; use crate::steering_hub::{ActiveControlHandle, SteeringHub}; /// Default refresh-ahead interval — comfortably under the ~60-min GitHub App -/// installation-token TTL. Used as the loop cadence when a tick reports no -/// managed credentials; ticks that see a real token reschedule from its -/// expiry instead. +/// installation-token TTL. const REFRESH_INTERVAL_DEFAULT: Duration = Duration::from_mins(45); -/// Floor for expiry-driven rescheduling, so a token already inside the cache -/// margin cannot pin the loop in a hot cycle. -const REFRESH_RESCHEDULE_FLOOR: Duration = Duration::from_secs(30); /// Upper bound on a single push-credential refresh (token mint + `git remote /// set-url` exec). The turn-entry refresh runs before the ACP process spawns /// and the ACP node uses `NodeTimeoutPolicy::HandlerManaged`, so without this @@ -107,35 +100,12 @@ fn push_cred_refresh_interval() -> Option { ) } -/// Delay until the next refresh-ahead tick after a successful refresh. -/// -/// With a cached token source, a fixed interval is unsafe: a tick landing -/// just outside the cache margin returns a reused token, and a fixed -/// 45-minute sleep would leave the embedded token expired until the next -/// tick. Schedule from the token's own `expires_at` instead: wake when the -/// cache margin opens, so that tick re-mints. `None` disables the loop — -/// static credentials cannot be re-minted by waiting. -fn next_refresh_delay(outcome: &RefreshOutcome, fallback: Duration) -> Option { - let Some(token) = outcome.token else { - // No managed credentials to watch; keep the configured cadence in - // case a later tick sees them (e.g. after a reconnect). - return Some(fallback); - }; - let expires_at = token.expires_at()?; - let margin = chrono::Duration::from_std(REFRESH_MARGIN).unwrap_or(chrono::Duration::MAX); - let until_margin = ((expires_at - margin) - chrono::Utc::now()) - .to_std() - .unwrap_or(Duration::ZERO); - Some(until_margin.max(REFRESH_RESCHEDULE_FLOOR)) -} - -/// Background loop that keeps the sandbox's push credentials fresh for the -/// duration of one ACP turn, so a single turn that outlives the -/// installation-token TTL still pushes with a fresh token. Bounded by -/// `cancel` (the drop-guard cancels it at turn end). Each successful tick -/// reschedules from the embedded token's expiry ([`next_refresh_delay`]); a -/// failed or timed-out tick retries after a shorter delay so a transient -/// error does not leave a longer-than-interval window with an expired token. +/// Background loop that re-mints the sandbox's push credentials every +/// `interval` for the duration of one ACP turn, so a single turn that outlives +/// the installation-token TTL still pushes with a fresh token. Bounded by +/// `cancel` (the drop-guard cancels it at turn end). A failed or timed-out tick +/// retries after a shorter delay so a transient error does not leave a +/// longer-than-interval window with an expired token. async fn refresh_ahead_loop( sandbox: Arc, cancel: CancellationToken, @@ -150,34 +120,19 @@ async fn refresh_ahead_loop( match timeout(REFRESH_MINT_TIMEOUT, sandbox.refresh_push_credentials()) .await { - Ok(Ok(outcome)) => { - match outcome.action { - RemoteCredentialAction::Embedded => { - tracing::info!( - generation = outcome.token.map(|token| token.generation), - "refresh-ahead re-embedded push credentials mid-turn" - ); - } - RemoteCredentialAction::Unchanged => { - tracing::debug!( - generation = outcome.token.map(|token| token.generation), - "refresh-ahead tick: embedded push credentials still fresh" - ); - } - RemoteCredentialAction::None => { - tracing::debug!( - "refresh-ahead tick: no managed push credentials to refresh" - ); - } - } - if let Some(next) = next_refresh_delay(&outcome, interval) { - delay = next; - } else { - tracing::debug!( - "refresh-ahead loop stopped: static credentials cannot be re-minted" - ); - break; - } + Ok(Ok(RefreshOutcome::Refreshed)) => { + tracing::info!( + interval_secs = interval.as_secs(), + "refresh-ahead re-minted push credentials mid-turn" + ); + delay = interval; + } + Ok(Ok(RefreshOutcome::Skipped)) => { + tracing::debug!( + interval_secs = interval.as_secs(), + "refresh-ahead tick: no managed push credentials to refresh" + ); + delay = interval; } Ok(Err(e)) => { tracing::warn!( @@ -316,51 +271,38 @@ impl AgentAcpBackend { // turn so the agent's own `git push` uses a live token instead of the one // baked into the clone at run start. // - // Part 2 (turn-entry): resolve through the cached token source and - // rewrite the origin URL before the ACP process spawns, covering a push - // early in the turn. A fresh cached token makes this a no-op exec-wise. - // Non-fatal and timeout-bounded — a stalled mint must neither fail nor - // hang node entry. Part 3 (loop): a background task keeps the embedded - // token fresh so a single turn that outlives the ~60-min - // installation-token TTL still pushes with a fresh token; ticks - // reschedule from the embedded token's expiry, so a normal short turn - // never ticks (the drop-guard aborts the task at turn end). + // Part 2 (turn-entry): re-mint + rewrite the origin URL before the ACP + // process spawns, covering a push early in the turn. Non-fatal and + // timeout-bounded — a stalled mint must neither fail nor hang node entry. + // Part 3 (loop): a background task re-mints every ~45 min so a single turn + // that itself outlives the ~60-min installation-token TTL still pushes + // with a fresh token; a normal sub-interval turn never ticks (the + // drop-guard aborts the task at turn end before the first tick). // // FABRO_PUSH_CRED_REFRESH_AHEAD=0 (or false/off/no/empty, case- - // insensitive) disables the WHOLE feature — turn-entry refresh AND loop — + // insensitive) disables the WHOLE feature — turn-entry re-mint AND loop — // so an operator who manages `origin` themselves can opt out of all // fabro-side origin rewriting. FABRO_PUSH_CRED_REFRESH_INTERVAL_SECONDS - // overrides the loop cadence for ticks without token expiry info; 0 - // disables just the loop. + // overrides the loop interval; 0 disables just the loop. // // Known limitations tracked as follow-ups (not addressed here): (a) // resumed/parked runs reconnect the sandbox with no GitHub App creds, so // refresh no-ops until those creds are threaded through the reconnect - // path; (b) the background `git remote set-url` can contend with the - // agent's own git on `.git/config.lock` (skipped entirely while the - // cached generation is already embedded); (c) parallel ACP branches each - // run their own loop; (d) this refresh lives in the ACP handler only, - // though the stale-origin problem is stage-type-agnostic (native/command - // stages that push are not covered); (e) refresh failures are logged via - // tracing but not surfaced as a RunNotice event on the run stream. + // path; (b) the turn-entry re-mint has no freshness check, so it mints + // once per node entry even when the current token is still fresh; (c) the + // background `git remote set-url` can contend with the agent's own git on + // `.git/config.lock`; (d) parallel ACP branches each run their own loop; + // (e) this refresh lives in the ACP handler only, though the stale-origin + // problem is stage-type-agnostic (native/command stages that push are not + // covered); (f) refresh failures are logged via tracing but not surfaced + // as a RunNotice event on the run stream. let refresh_enabled = push_cred_refresh_enabled(); if refresh_enabled { match timeout(REFRESH_MINT_TIMEOUT, sandbox.refresh_push_credentials()).await { - Ok(Ok(outcome)) => match outcome.action { - RemoteCredentialAction::Embedded => { - tracing::debug!( - generation = outcome.token.map(|token| token.generation), - "refreshed sandbox push credentials at ACP turn entry" - ); - } - RemoteCredentialAction::Unchanged => { - tracing::debug!( - generation = outcome.token.map(|token| token.generation), - "sandbox push credentials already fresh at ACP turn entry" - ); - } - RemoteCredentialAction::None => {} - }, + Ok(Ok(RefreshOutcome::Refreshed)) => { + tracing::debug!("refreshed sandbox push credentials at ACP turn entry"); + } + Ok(Ok(RefreshOutcome::Skipped)) => {} Ok(Err(e)) => { tracing::warn!( error = %fabro_sandbox::display_for_log(&e), @@ -669,18 +611,14 @@ mod tests { use fabro_acp::test_support::fake_acp_agent_script; use fabro_acp::{AcpError, AcpProcessExit}; - use fabro_agent::{ - LocalSandbox, RefreshOutcome, RemoteCredentialAction, Sandbox, TokenProvenance, - TokenSnapshot, shell_quote, - }; + use fabro_agent::{LocalSandbox, RefreshOutcome, Sandbox, shell_quote}; use fabro_graphviz::graph::{AttrValue, Node}; use fabro_sandbox::test_support::MockSandbox; use fabro_types::{CommandTermination, EventBody, ExecOutputTail}; use tokio_util::sync::CancellationToken; use super::{ - AgentAcpBackend, REFRESH_RESCHEDULE_FLOOR, acp_error_to_workflow, next_refresh_delay, - parse_refresh_enabled, parse_refresh_interval, refresh_ahead_loop, + AgentAcpBackend, acp_error_to_workflow, parse_refresh_enabled, parse_refresh_interval, }; use crate::context::Context; use crate::event::Emitter; @@ -733,299 +671,17 @@ mod tests { } #[tokio::test] - async fn refresh_reports_no_action_without_managed_credentials() { - // MockSandbox uses the trait default (no GitHub App creds), so refresh - // is a no-op that must report no remote action and no token — the - // signal the refresh-ahead loop relies on to log at debug rather than - // falsely claim a re-embed. + async fn refresh_reports_skipped_without_managed_credentials() { + // MockSandbox uses the trait default (no GitHub App creds), so refresh is + // a no-op that must report Skipped — the signal the refresh-ahead loop + // relies on to log at debug rather than falsely claim a re-mint. let sandbox = MockSandbox::linux(); assert_eq!( sandbox.refresh_push_credentials().await.unwrap(), - RefreshOutcome::none() + RefreshOutcome::Skipped ); } - fn minted_outcome( - action: RemoteCredentialAction, - generation: u64, - minted_ago: chrono::Duration, - expires_in: chrono::Duration, - reused: bool, - ) -> RefreshOutcome { - let now = chrono::Utc::now(); - let minted_at = now - minted_ago; - let expires_at = now + expires_in; - let provenance = if reused { - TokenProvenance::Reused { - minted_at, - expires_at, - } - } else { - TokenProvenance::Minted { - minted_at, - expires_at, - } - }; - RefreshOutcome { - action, - token: Some(TokenSnapshot { - generation, - provenance, - }), - } - } - - fn static_outcome() -> RefreshOutcome { - RefreshOutcome { - action: RemoteCredentialAction::Unchanged, - token: Some(TokenSnapshot { - generation: 0, - provenance: TokenProvenance::Static, - }), - } - } - - #[test] - fn next_refresh_delay_schedules_from_token_expiry_minus_margin() { - let outcome = minted_outcome( - RemoteCredentialAction::Embedded, - 1, - chrono::Duration::zero(), - chrono::Duration::minutes(60), - false, - ); - let delay = next_refresh_delay(&outcome, Duration::from_mins(45)).unwrap(); - // Expiry minus the 10-minute refresh margin: ~50 minutes out. - assert!(delay > Duration::from_mins(49), "{delay:?}"); - assert!(delay <= Duration::from_mins(50), "{delay:?}"); - } - - #[test] - fn next_refresh_delay_floors_when_the_margin_is_already_open() { - let outcome = minted_outcome( - RemoteCredentialAction::Unchanged, - 1, - chrono::Duration::minutes(55), - chrono::Duration::minutes(5), - true, - ); - assert_eq!( - next_refresh_delay(&outcome, Duration::from_mins(45)), - Some(REFRESH_RESCHEDULE_FLOOR) - ); - } - - #[test] - fn next_refresh_delay_disables_the_loop_for_static_credentials() { - assert_eq!( - next_refresh_delay(&static_outcome(), Duration::from_mins(45)), - None - ); - } - - #[test] - fn next_refresh_delay_keeps_the_cadence_without_managed_credentials() { - assert_eq!( - next_refresh_delay(&RefreshOutcome::none(), Duration::from_mins(45)), - Some(Duration::from_mins(45)) - ); - } - - /// Sandbox stub whose refresh outcomes are scripted, recording when each - /// refresh tick lands on the (paused) tokio clock. - struct ScriptedRefreshSandbox { - script: Mutex>, - ticks: Mutex>, - } - - impl ScriptedRefreshSandbox { - fn new(script: Vec) -> Arc { - Arc::new(Self { - script: Mutex::new(script.into()), - ticks: Mutex::new(Vec::new()), - }) - } - - fn ticks(&self) -> Vec { - self.ticks.lock().expect("ticks lock").clone() - } - } - - #[async_trait::async_trait] - impl Sandbox for ScriptedRefreshSandbox { - async fn refresh_push_credentials(&self) -> fabro_sandbox::Result { - self.ticks - .lock() - .expect("ticks lock") - .push(tokio::time::Instant::now()); - Ok(self - .script - .lock() - .expect("script lock") - .pop_front() - .expect("refresh script exhausted")) - } - - async fn read_file_bytes(&self, _path: &str) -> fabro_sandbox::Result> { - unimplemented!("refresh loop only calls refresh_push_credentials") - } - - async fn write_file(&self, _path: &str, _content: &str) -> fabro_sandbox::Result<()> { - unimplemented!() - } - - async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { - unimplemented!() - } - - async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { - unimplemented!() - } - - async fn list_directory( - &self, - _path: &str, - _depth: Option, - ) -> fabro_sandbox::Result> { - unimplemented!() - } - - async fn exec_command( - &self, - _command: &str, - _timeout_ms: u64, - _working_dir: Option<&str>, - _env_vars: Option<&HashMap>, - _cancel_token: Option, - ) -> fabro_sandbox::Result { - unimplemented!() - } - - async fn grep( - &self, - _pattern: &str, - _path: &str, - _options: &fabro_sandbox::GrepOptions, - ) -> fabro_sandbox::Result> { - unimplemented!() - } - - async fn download_file_to_local( - &self, - _remote_path: &str, - _local_path: &std::path::Path, - ) -> fabro_sandbox::Result<()> { - unimplemented!() - } - - async fn upload_file_from_local( - &self, - _local_path: &std::path::Path, - _remote_path: &str, - ) -> fabro_sandbox::Result<()> { - unimplemented!() - } - - async fn initialize(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - - async fn cleanup(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - - fn working_directory(&self) -> &str { - "/workspace" - } - - fn platform(&self) -> &str { - "linux" - } - - fn os_version(&self) -> String { - "linux".to_string() - } - } - - /// Long-turn timeline: the clone/turn-entry mint happened at minute 0 with - /// a 60-minute TTL. The loop's first tick at minute 45 sees the cached - /// token reused with ~15 minutes left and must NOT sleep another fixed 45 - /// minutes (that would cross expiry at minute 60) — it reschedules for the - /// margin opening (~5 minutes out). That margin-crossing tick re-mints and - /// reschedules from the fresh token's expiry (~50 minutes out). - #[tokio::test(start_paused = true)] - async fn refresh_ahead_reschedules_from_token_expiry_across_a_long_turn() { - let interval = Duration::from_mins(45); - let sandbox = ScriptedRefreshSandbox::new(vec![ - // Minute 45: cache still fresh (expires minute 60, margin opens - // minute 50). - minted_outcome( - RemoteCredentialAction::Unchanged, - 1, - chrono::Duration::minutes(45), - chrono::Duration::minutes(15), - true, - ), - // Minute ~50: margin open → the source minted generation 2. - minted_outcome( - RemoteCredentialAction::Embedded, - 2, - chrono::Duration::zero(), - chrono::Duration::minutes(60), - false, - ), - // Minute ~100: generation 2 still fresh. - minted_outcome( - RemoteCredentialAction::Unchanged, - 2, - chrono::Duration::minutes(50), - chrono::Duration::minutes(10), - true, - ), - ]); - let cancel = CancellationToken::new(); - let start = tokio::time::Instant::now(); - let loop_task = tokio::spawn(refresh_ahead_loop( - Arc::clone(&sandbox) as Arc, - cancel.clone(), - interval, - )); - - while sandbox.ticks().len() < 3 { - tokio::time::sleep(Duration::from_secs(1)).await; - } - cancel.cancel(); - loop_task.await.expect("refresh loop should exit cleanly"); - - let ticks = sandbox.ticks(); - assert_eq!(ticks[0] - start, interval, "first tick uses the interval"); - // Reused token expiring in 15 minutes → next tick when the 10-minute - // margin opens, ~5 minutes later (never another fixed 45 minutes). - let second_gap = ticks[1] - ticks[0]; - assert!(second_gap <= Duration::from_mins(5), "{second_gap:?}"); - assert!(second_gap > Duration::from_mins(4), "{second_gap:?}"); - // Fresh 60-minute token → next tick ~50 minutes out. - let third_gap = ticks[2] - ticks[1]; - assert!(third_gap <= Duration::from_mins(50), "{third_gap:?}"); - assert!(third_gap > Duration::from_mins(49), "{third_gap:?}"); - } - - #[tokio::test(start_paused = true)] - async fn refresh_ahead_stops_by_itself_for_static_credentials() { - let sandbox = ScriptedRefreshSandbox::new(vec![static_outcome()]); - let cancel = CancellationToken::new(); - let loop_task = tokio::spawn(refresh_ahead_loop( - Arc::clone(&sandbox) as Arc, - cancel.clone(), - Duration::from_mins(45), - )); - - // The loop exits after the first tick without being cancelled: static - // credentials cannot be re-minted, so there is nothing to keep fresh. - loop_task.await.expect("refresh loop should stop by itself"); - assert_eq!(sandbox.ticks().len(), 1); - } - #[tokio::test] async fn acp_backend_run_sends_prompt_and_returns_text() { let tempdir = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-workflow/src/lifecycle/event.rs b/lib/components/fabro-workflow/src/lifecycle/event.rs index 6303ea1f3..8922708e0 100644 --- a/lib/components/fabro-workflow/src/lifecycle/event.rs +++ b/lib/components/fabro-workflow/src/lifecycle/event.rs @@ -493,7 +493,6 @@ impl RunLifecycle for EventLifecycle { branch: push.branch.clone(), success: push.success, exec_output_tail: push.exec_output_tail.clone(), - attempts: push.attempts.clone(), }); } } diff --git a/lib/components/fabro-workflow/src/lifecycle/git.rs b/lib/components/fabro-workflow/src/lifecycle/git.rs index a744d0ba0..18f4139bf 100644 --- a/lib/components/fabro-workflow/src/lifecycle/git.rs +++ b/lib/components/fabro-workflow/src/lifecycle/git.rs @@ -8,18 +8,13 @@ use fabro_core::lifecycle::RunLifecycle; use fabro_core::outcome::NodeResult; use fabro_core::state::ExecutionState; use fabro_dump::RunDump; -use fabro_sandbox::git_retry; -use fabro_types::run_event::{ - GitPushAttemptProps, MetadataSnapshotFailureKind, MetadataSnapshotPhase, -}; +use fabro_types::run_event::{MetadataSnapshotFailureKind, MetadataSnapshotPhase}; use fabro_types::{CheckpointRecord, DiffSummary, RunDiff, RunId}; use fabro_util::error::collect_causes; use fabro_util::time::elapsed_ms; use crate::artifact; -use crate::event::{ - Emitter, Event, RunNoticeCode, RunNoticeLevel, StageScope, git_push_attempt_props, -}; +use crate::event::{Emitter, Event, RunNoticeCode, RunNoticeLevel, StageScope}; use crate::graph::{WorkflowGraph, WorkflowNode}; use crate::lifecycle::event::stage_scope_for; use crate::outcome::BilledModelUsage; @@ -77,38 +72,21 @@ pub(crate) struct PushResult { pub branch: String, pub success: bool, pub exec_output_tail: Option, - /// Per-attempt history, already projected to the durable event shape. - pub attempts: Vec, } /// Push a run branch to its remote counterpart. /// /// Owns the refspec convention so the checkpoint push and the terminal publish -/// push cannot drift apart. The caller picks the retry budget: cheap for -/// checkpoint pushes (the next checkpoint re-pushes the same branch anyway), -/// generous for the terminal publish push. +/// push cannot drift apart. pub(crate) async fn push_run_branch( sandbox: &dyn fabro_sandbox::Sandbox, branch: &str, - plan: &fabro_sandbox::RetryPlan, -) -> Result { +) -> fabro_sandbox::Result<()> { sandbox - .git_push_ref(&format!("refs/heads/{branch}:refs/heads/{branch}"), plan) + .git_push_ref(&format!("refs/heads/{branch}:refs/heads/{branch}")) .await } -/// Whether a metadata push failure leaves the writer eligible for re-probing -/// at later checkpoints. Only push failures with retryable classifications -/// (replication lag on a fresh token, transient infrastructure) qualify; -/// everything else keeps the permanent latch. -fn metadata_push_failure_is_transient( - detail: &str, - token: Option<&fabro_sandbox::TokenSnapshot>, -) -> bool { - let cred = fabro_sandbox::CredentialContext::from_snapshot(token); - git_retry::classify_failure(detail, cred).is_some() -} - /// Sub-lifecycle responsible for git operations (checkpoint commits, pushes, /// diffs). pub(crate) struct GitLifecycle { @@ -139,7 +117,7 @@ impl RunLifecycle for GitLifecycle { "git lifecycle mutex should not be poisoned: no code panics while holding this lock", ) = None; if let Some(meta_branch) = self.metadata_branch().map(str::to_string) { - if self.metadata_writer.is_none() || self.metadata_runtime.metadata_suspended() { + if self.metadata_writer.is_none() || self.metadata_runtime.metadata_degraded() { return Ok(()); } let phase = MetadataSnapshotPhase::Init; @@ -176,7 +154,6 @@ impl RunLifecycle for GitLifecycle { self.emit_metadata_warning( RunNoticeCode::CheckpointMetadataWriteFailed, message, - false, ); } }, @@ -197,7 +174,6 @@ impl RunLifecycle for GitLifecycle { self.emit_metadata_warning( RunNoticeCode::CheckpointMetadataWriteFailed, message, - false, ); } } @@ -232,7 +208,7 @@ impl RunLifecycle for GitLifecycle { None, ); let shadow_sha = if let Some(meta_branch) = self.metadata_branch().map(str::to_string) { - if self.metadata_writer.is_none() || self.metadata_runtime.metadata_suspended() { + if self.metadata_writer.is_none() || self.metadata_runtime.metadata_degraded() { None } else { let phase = MetadataSnapshotPhase::Checkpoint; @@ -277,7 +253,6 @@ impl RunLifecycle for GitLifecycle { self.emit_metadata_warning( RunNoticeCode::CheckpointMetadataWriteFailed, message, - false, ); None } @@ -301,7 +276,6 @@ impl RunLifecycle for GitLifecycle { self.emit_metadata_warning( RunNoticeCode::CheckpointMetadataWriteFailed, message, - false, ); None } @@ -346,41 +320,30 @@ impl RunLifecycle for GitLifecycle { .as_ref() .and_then(|g| g.run_branch.as_ref()) { - let plan = fabro_sandbox::RetryPlan::checkpoint_push(); - let (push_ok, exec_output_tail, attempts) = - match push_run_branch(self.sandbox.as_ref(), branch, &plan).await { - Ok(report) => { - self.sandbox_git.record_successful_push(); - (true, None, report.attempts) - } - Err(push_error) => { + let (push_ok, exec_output_tail) = + match push_run_branch(self.sandbox.as_ref(), branch).await { + Ok(()) => (true, None), + Err(err) => { let exec_output_tail = - fabro_sandbox::default_redacted_output_tail( - &push_error.error, - ); + fabro_sandbox::default_redacted_output_tail(&err); tracing::warn!( branch = %branch, - attempts = push_error.report.attempts.len(), - error = %fabro_sandbox::display_for_log(&push_error.error), + error = %fabro_sandbox::display_for_log(&err), "git push from run lifecycle failed" ); self.emitter.notice_with_tail( RunNoticeLevel::Warn, RunNoticeCode::GitPushFailed, - format!( - "Failed to push run branch {branch}: {}", - push_error.error - ), + format!("Failed to push run branch {branch}: {err}"), exec_output_tail.clone(), ); - (false, exec_output_tail, push_error.report.attempts) + (false, exec_output_tail) } }; git_result.push_results.push(PushResult { branch: branch.clone(), success: push_ok, exec_output_tail, - attempts: git_push_attempt_props(&attempts), }); } } @@ -489,7 +452,7 @@ impl GitLifecycle { message: &str, scope: Option<&StageScope>, ) -> Option { - if self.metadata_runtime.metadata_suspended() { + if self.metadata_runtime.metadata_degraded() { return None; } let writer = self.metadata_writer.as_ref()?; @@ -514,12 +477,8 @@ impl GitLifecycle { self.emit_metadata_warning( RunNoticeCode::CheckpointMetadataPushFailed, message, - metadata_push_failure_is_transient(detail, snapshot.token.as_ref()), ); } else { - // One good snapshot ends the degradation; a later - // independent failure warns again. - self.metadata_runtime.clear_metadata_degraded(); self.emit_metadata_snapshot_completed( phase, meta_branch, @@ -544,11 +503,7 @@ impl GitLifecycle { None, scope, ); - self.emit_metadata_warning( - RunNoticeCode::CheckpointMetadataWriteFailed, - message, - false, - ); + self.emit_metadata_warning(RunNoticeCode::CheckpointMetadataWriteFailed, message); None } } @@ -633,8 +588,8 @@ impl GitLifecycle { } } - fn emit_metadata_warning(&self, code: RunNoticeCode, message: String, transient: bool) { - if self.metadata_runtime.mark_metadata_degraded(transient) { + fn emit_metadata_warning(&self, code: RunNoticeCode, message: String) { + if self.metadata_runtime.mark_metadata_degraded() { self.emitter.notice(RunNoticeLevel::Warn, code, message); } } @@ -1247,7 +1202,7 @@ mod tests { let repo_dir = tempfile::tempdir().unwrap(); init_git_repo(repo_dir.path()); let runtime = Arc::new(RunMetadataRuntime::new()); - runtime.mark_metadata_degraded(false); + runtime.mark_metadata_degraded(); let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); let events = record_events(&emitter); let lifecycle = git_lifecycle( diff --git a/lib/components/fabro-workflow/src/pipeline/finalize.rs b/lib/components/fabro-workflow/src/pipeline/finalize.rs index f30bbc27c..b1e7847ee 100644 --- a/lib/components/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/components/fabro-workflow/src/pipeline/finalize.rs @@ -4,7 +4,6 @@ use std::time::Instant; use fabro_dump::RunDump; use fabro_hooks::{HookContext, HookEvent}; -use fabro_sandbox::git_retry; use fabro_types::run_event::{MetadataSnapshotFailureKind, MetadataSnapshotPhase}; use fabro_types::{BilledTokenCounts, DiffSummary, EventBody, RunFailure, RunProjection}; use fabro_util::error::collect_causes; @@ -203,7 +202,7 @@ pub async fn write_finalize_commit( services: &RunServices, conclusion: &Conclusion, ) { - if services.metadata_runtime.metadata_suspended() { + if services.metadata_runtime.metadata_degraded() { return; } let Some(writer) = services.metadata_writer.as_ref() else { @@ -241,7 +240,6 @@ pub async fn write_finalize_commit( services, RunNoticeCode::CheckpointMetadataWriteFailed, message, - false, ); return; } @@ -267,7 +265,6 @@ pub async fn write_finalize_commit( services, RunNoticeCode::CheckpointMetadataWriteFailed, message, - false, ); return; } @@ -293,10 +290,8 @@ pub async fn write_finalize_commit( services, RunNoticeCode::CheckpointMetadataPushFailed, message, - metadata_push_failure_is_transient(detail, snapshot.token.as_ref()), ); } else { - services.metadata_runtime.clear_metadata_degraded(); emit_metadata_snapshot_completed(services, phase, meta_branch, started, &snapshot); } } @@ -318,7 +313,6 @@ pub async fn write_finalize_commit( services, RunNoticeCode::CheckpointMetadataWriteFailed, message, - false, ); } } @@ -382,23 +376,8 @@ fn emit_metadata_snapshot_failed( }); } -/// Whether a metadata push failure leaves the writer eligible for re-probing. -/// See `lifecycle::git`: only retryable push classifications qualify. -fn metadata_push_failure_is_transient( - detail: &str, - token: Option<&fabro_sandbox::TokenSnapshot>, -) -> bool { - let cred = fabro_sandbox::CredentialContext::from_snapshot(token); - git_retry::classify_failure(detail, cred).is_some() -} - -fn emit_metadata_warning( - services: &RunServices, - code: RunNoticeCode, - message: String, - transient: bool, -) { - if services.metadata_runtime.mark_metadata_degraded(transient) { +fn emit_metadata_warning(services: &RunServices, code: RunNoticeCode, message: String) { + if services.metadata_runtime.mark_metadata_degraded() { services.emitter.notice(RunNoticeLevel::Warn, code, message); } } @@ -1278,7 +1257,7 @@ mod tests { let emitter = Arc::new(Emitter::new(test_run_id())); let events = record_events(&emitter); let runtime = Arc::new(RunMetadataRuntime::new()); - runtime.mark_metadata_degraded(false); + runtime.mark_metadata_degraded(); let services = test_services( RunStoreHandle::local(run_store), emitter, @@ -1482,18 +1461,7 @@ mod tests { ); let events = events.lock().unwrap(); let names = events.iter().map(RunEvent::event_name).collect::>(); - // Exactly one durable git.push event per high-level push — retries - // nest inside it as attempts, never as extra events. assert_eq!(names, vec!["git.push", "run.failed"]); - match &events.first().unwrap().body { - EventBody::GitPush(props) => { - assert!(!props.success); - // MockSandbox's default git_push_ref fails before any attempt - // runs, so the nested history is empty here. - assert!(props.attempts.is_empty()); - } - other => panic!("expected git.push, got {other:?}"), - } match &events.last().unwrap().body { EventBody::RunFailed(props) => { assert_eq!(props.failure.reason, FailureReason::PublishFailed); diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index da80f5085..5a7dd6069 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -600,21 +600,20 @@ pub async fn initialize( }); } - let metadata_writer = - match build_metadata_writer(&options.run_options, sandbox.push_token_source()) { - Ok(writer) => writer, - Err(err) => { - let message = format!("failed to initialize checkpoint metadata writer: {err}"); - if metadata_runtime.mark_metadata_degraded(false) { - options.emitter.notice( - RunNoticeLevel::Warn, - RunNoticeCode::CheckpointMetadataWriteFailed, - message, - ); - } - None + let metadata_writer = match build_metadata_writer(&options.run_options) { + Ok(writer) => writer, + Err(err) => { + let message = format!("failed to initialize checkpoint metadata writer: {err}"); + if metadata_runtime.mark_metadata_degraded() { + options.emitter.notice( + RunNoticeLevel::Warn, + RunNoticeCode::CheckpointMetadataWriteFailed, + message, + ); } - }; + None + } + }; let run_services = RunServices::new( options.run_store.clone(), diff --git a/lib/components/fabro-workflow/src/pipeline/publish.rs b/lib/components/fabro-workflow/src/pipeline/publish.rs index 2191f7a7e..30fe5b055 100644 --- a/lib/components/fabro-workflow/src/pipeline/publish.rs +++ b/lib/components/fabro-workflow/src/pipeline/publish.rs @@ -1,13 +1,9 @@ -use std::fmt::Write as _; use std::sync::Arc; -use fabro_types::ExecOutputTail; -use fabro_types::run_event::GitPushAttemptProps; - use super::pull_request::{AutoMergeOptions, OpenPullRequestRequest, open_pull_request}; use super::types::{Concluded, PublishOptions, PublishOutcome, Published}; -use crate::error::{Error, FailureCategory, classify_failure_reason}; -use crate::event::{Event, git_push_attempt_props}; +use crate::error::Error; +use crate::event::Event; use crate::lifecycle::git::push_run_branch; /// PUBLISH phase: push the final run commit and, when configured, open a pull @@ -39,76 +35,6 @@ pub async fn publish(concluded: Concluded, options: &PublishOptions) -> Publishe } } -/// Build the terminal publish error from a failed push operation. -/// -/// Retries exhausted on transient classifications stay `TransientInfra`: a -/// mature-token 404 is not proof of permanent access loss — a service-side -/// failure presents the same surface — so `Deterministic` would need -/// independent evidence this path does not gather. Each attempt becomes one -/// bounded cause line in the failure detail; git output stays inside the -/// exec output tail. -fn publish_push_error( - run_branch: &str, - push_error: fabro_sandbox::PushError, - exec_output_tail: Option, - attempts: &[GitPushAttemptProps], - last_successful_push_at: Option>, -) -> Error { - let message = match last_successful_push_at { - Some(at) => format!( - "failed to push run branch '{run_branch}' (last successful push at {})", - at.to_rfc3339_opts(chrono::SecondsFormat::Millis, true) - ), - None => format!("failed to push run branch '{run_branch}'"), - }; - let failure_class = match push_error - .report - .attempts - .last() - .and_then(|attempt| attempt.retry_reason) - { - Some(_) => FailureCategory::TransientInfra, - None => classify_failure_reason(&format!( - "{message}: {}", - fabro_sandbox::display_for_log(&push_error.error) - )), - }; - let causes = attempts.iter().map(push_attempt_cause).collect(); - Error::publish_with_source_and_class( - message, - push_error, - failure_class, - exec_output_tail, - causes, - ) -} - -/// One bounded line per push attempt for the failure detail. -fn push_attempt_cause(attempt: &GitPushAttemptProps) -> String { - let outcome = if attempt.success { - "succeeded" - } else { - attempt - .classified_reason - .as_deref() - .unwrap_or("unclassified") - }; - let mut line = format!( - "push attempt {} at {}: {outcome}", - attempt.attempt, - attempt - .started_at - .to_rfc3339_opts(chrono::SecondsFormat::Millis, true) - ); - if let Some(age_ms) = attempt.token_age_ms { - let _ = write!(line, " (token age {age_ms}ms)"); - } - if let Some(refresh_error) = &attempt.refresh_error { - let _ = write!(line, ", refresh error: {refresh_error}"); - } - line -} - impl Concluded { /// Run the publish steps, recording each one into `outcome` as it lands. /// @@ -233,36 +159,26 @@ impl Concluded { } async fn push_final_commit(&self, run_branch: &str) -> Result<(), Error> { - // The terminal push guards the whole run's value, so it gets a real - // retry budget; attempts are nearly free at this point. - let plan = fabro_sandbox::RetryPlan::publish_push(); - match push_run_branch(self.services.sandbox.as_ref(), run_branch, &plan).await { - Ok(report) => { - self.services.sandbox_git.record_successful_push(); + match push_run_branch(self.services.sandbox.as_ref(), run_branch).await { + Ok(()) => { self.services.emitter.emit(&Event::GitPush { branch: run_branch.to_string(), success: true, exec_output_tail: None, - attempts: git_push_attempt_props(&report.attempts), }); Ok(()) } - Err(push_error) => { - let exec_output_tail = - fabro_sandbox::default_redacted_output_tail(&push_error.error); - let attempts = git_push_attempt_props(&push_error.report.attempts); + Err(error) => { + let exec_output_tail = fabro_sandbox::default_redacted_output_tail(&error); self.services.emitter.emit(&Event::GitPush { branch: run_branch.to_string(), success: false, exec_output_tail: exec_output_tail.clone(), - attempts: attempts.clone(), }); - Err(publish_push_error( - run_branch, - push_error, + Err(Error::publish_with_source_and_exec_output_tail( + format!("failed to push run branch '{run_branch}'"), + error, exec_output_tail, - &attempts, - self.services.sandbox_git.last_successful_push_at(), )) } } @@ -276,132 +192,3 @@ impl Concluded { Error::publish(message) } } - -#[cfg(test)] -mod tests { - use chrono::Utc; - use fabro_types::run_event::GitPushAttemptProps; - - use super::*; - use crate::error::FailureCategory; - - fn attempt_props( - attempt: u32, - classified_reason: Option<&str>, - token_age_ms: Option, - refresh_error: Option<&str>, - ) -> GitPushAttemptProps { - GitPushAttemptProps { - attempt, - started_at: Utc::now(), - success: false, - classified_reason: classified_reason.map(str::to_string), - exec_output_tail: None, - token_generation: Some(14), - token_provenance: Some("minted".to_string()), - token_age_ms, - credential_action: Some("unchanged".to_string()), - refresh_error: refresh_error.map(str::to_string), - } - } - - fn push_error_with_reasons( - reasons: &[Option], - ) -> fabro_sandbox::PushError { - let attempts = reasons - .iter() - .enumerate() - .map(|(index, reason)| fabro_sandbox::PushAttempt { - attempt: u32::try_from(index).unwrap() + 1, - started_at: Utc::now(), - success: false, - retry_reason: *reason, - exec_output_tail: None, - token: None, - credential_action: None, - refresh_error: None, - }) - .collect(); - fabro_sandbox::PushError { - report: fabro_sandbox::PushReport { attempts }, - error: fabro_sandbox::Error::message("remote: Repository not found."), - } - } - - /// Exhausted retries on a retryable classification are transient - /// infrastructure, not deterministic: the same push succeeded manually an - /// hour after run 01M0DH033P2XSTHAGVBHG6922F failed, with no - /// configuration change. - #[test] - fn exhausted_transient_retries_classify_as_transient_infra() { - let error = publish_push_error( - "fabro/run/test", - push_error_with_reasons(&[ - Some(fabro_sandbox::GitRetryReason::TokenReplication), - Some(fabro_sandbox::GitRetryReason::TokenReplication), - ]), - None, - &[], - None, - ); - assert_eq!(error.failure_category(), FailureCategory::TransientInfra); - } - - #[test] - fn permanently_classified_push_falls_back_to_message_sniffing() { - let error = publish_push_error( - "fabro/run/test", - push_error_with_reasons(&[None]), - None, - &[], - None, - ); - // "Repository not found." carries no transient hint for the - // heuristic, so the fallback stays deterministic. - assert_eq!(error.failure_category(), FailureCategory::Deterministic); - } - - #[test] - fn failure_detail_renders_one_cause_line_per_attempt() { - let attempts = vec![ - attempt_props(1, Some("token_replication"), Some(180), None), - attempt_props(2, Some("token_replication"), Some(3320), Some("set_url")), - ]; - let last_push = Utc::now() - chrono::Duration::seconds(67); - let error = publish_push_error( - "fabro/run/test", - push_error_with_reasons(&[ - Some(fabro_sandbox::GitRetryReason::TokenReplication), - Some(fabro_sandbox::GitRetryReason::TokenReplication), - ]), - None, - &attempts, - Some(last_push), - ); - - let detail = error.to_failure_detail(); - assert!( - detail.message.contains("last successful push at"), - "{}", - detail.message - ); - let attempt_lines: Vec<&String> = detail - .causes - .iter() - .filter(|cause| cause.starts_with("push attempt")) - .collect(); - assert_eq!(attempt_lines.len(), 2); - assert!( - attempt_lines[0].contains("token_replication"), - "{attempt_lines:?}" - ); - assert!( - attempt_lines[0].contains("(token age 180ms)"), - "{attempt_lines:?}" - ); - assert!( - attempt_lines[1].contains("refresh error: set_url"), - "{attempt_lines:?}" - ); - } -} diff --git a/lib/components/fabro-workflow/src/run_metadata.rs b/lib/components/fabro-workflow/src/run_metadata.rs index e609fc760..39d686857 100644 --- a/lib/components/fabro-workflow/src/run_metadata.rs +++ b/lib/components/fabro-workflow/src/run_metadata.rs @@ -1,11 +1,12 @@ +use std::collections::HashMap; use std::path::Path; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, LazyLock, Mutex}; +use anyhow::Context as _; use async_trait::async_trait; use fabro_checkpoint::git::{FileMode, Store, TreeEntries}; use fabro_dump::RunDump; -use fabro_github::token_source::{InstallationTokenSource, TokenSnapshot}; use git2::{ Cred, Direction, ErrorClass, ErrorCode, FetchOptions, Oid, PushOptions, RemoteCallbacks, Repository, Signature, @@ -15,6 +16,13 @@ use tokio::task::{self, JoinError}; use crate::git::{GitAuthor, META_BRANCH_PREFIX}; use crate::run_options::RunOptions; +static METADATA_PERMISSIONS: LazyLock> = LazyLock::new(|| { + [("contents", "write")] + .into_iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect() +}); + pub(crate) fn metadata_branch_name(run_id: &str) -> String { format!("{META_BRANCH_PREFIX}{run_id}") } @@ -47,63 +55,26 @@ pub(crate) struct MetadataSnapshot { pub push_error: Option, pub entry_count: usize, pub bytes: u64, - /// The token the push authenticated with, for classifying a push - /// failure against the credential context. - pub token: Option, } pub(crate) struct RunMetadataRuntime { - degraded: AtomicBool, - /// A transiently degraded writer stays eligible for one snapshot attempt - /// at each subsequent checkpoint; a permanent failure latches snapshots - /// off for the rest of the run. - reprobe_eligible: AtomicBool, - warning_emitted: AtomicBool, + degraded: AtomicBool, + warning_emitted: AtomicBool, } impl RunMetadataRuntime { pub(crate) fn new() -> Self { Self { - degraded: AtomicBool::new(false), - reprobe_eligible: AtomicBool::new(false), - warning_emitted: AtomicBool::new(false), + degraded: AtomicBool::new(false), + warning_emitted: AtomicBool::new(false), } } - /// Record a metadata failure. `transient` failures (push failures with - /// retryable classifications) leave the writer eligible for re-probing; - /// initialization, discovery, serialization, and permanent-authentication - /// failures latch it off — repeating known-failing work at every - /// checkpoint is noise, not resilience. A permanent latch is never - /// upgraded by a later transient failure. Returns whether the caller - /// should emit the degradation warning (once per degradation). - pub(crate) fn mark_metadata_degraded(&self, transient: bool) -> bool { - let was_degraded = self.degraded.swap(true, Ordering::SeqCst); - if was_degraded { - if !transient { - self.reprobe_eligible.store(false, Ordering::SeqCst); - } - } else { - self.reprobe_eligible.store(transient, Ordering::SeqCst); - } + pub(crate) fn mark_metadata_degraded(&self) -> bool { + self.degraded.store(true, Ordering::SeqCst); !self.warning_emitted.swap(true, Ordering::SeqCst) } - /// A successful snapshot clears the degraded state and re-arms the - /// warning, so a later independent failure warns again instead of - /// failing silently. - pub(crate) fn clear_metadata_degraded(&self) { - self.degraded.store(false, Ordering::SeqCst); - self.reprobe_eligible.store(false, Ordering::SeqCst); - self.warning_emitted.store(false, Ordering::SeqCst); - } - - /// Whether snapshot writes should be skipped: degraded with no re-probe - /// eligibility. - pub(crate) fn metadata_suspended(&self) -> bool { - self.degraded.load(Ordering::SeqCst) && !self.reprobe_eligible.load(Ordering::SeqCst) - } - pub(crate) fn metadata_degraded(&self) -> bool { self.degraded.load(Ordering::SeqCst) } @@ -115,40 +86,28 @@ impl Default for RunMetadataRuntime { } } -/// A resolved metadata push token: the secret plus the non-secret snapshot -/// used to classify push failures against the credential context. -pub(crate) struct MetadataToken { - pub secret: String, - pub snapshot: Option, -} - #[async_trait] pub(crate) trait AuthProvider: Send + Sync { - async fn token(&self) -> Result, RunMetadataError>; + async fn token(&self) -> Result, RunMetadataError>; } struct GitHubAuthProvider { - source: Arc, + creds: fabro_github::GitHubCredentials, + origin_url: String, } impl GitHubAuthProvider { - fn new(source: Arc) -> Self { - Self { source } + fn new(creds: fabro_github::GitHubCredentials, origin_url: String) -> Self { + Self { creds, origin_url } } } #[async_trait] impl AuthProvider for GitHubAuthProvider { - async fn token(&self) -> Result, RunMetadataError> { - self.source - .resolve() + async fn token(&self) -> Result, RunMetadataError> { + mint_token(&self.creds, &self.origin_url, &METADATA_PERMISSIONS) .await - .map(|resolved| { - Some(MetadataToken { - secret: resolved.token.expose().to_owned(), - snapshot: Some(resolved.snapshot), - }) - }) + .map(Some) .map_err(RunMetadataError::TokenMint) } } @@ -159,7 +118,7 @@ struct NoAuth; #[cfg(test)] #[async_trait] impl AuthProvider for NoAuth { - async fn token(&self) -> Result, RunMetadataError> { + async fn token(&self) -> Result, RunMetadataError> { Ok(None) } } @@ -232,8 +191,6 @@ impl RunMetadataWriterHandle { message: &str, ) -> Result { let token = self.auth.token().await?; - let token_snapshot = token.as_ref().and_then(|token| token.snapshot); - let secret = token.map(|token| token.secret); let entries = dump .git_entries() .map_err(RunMetadataError::DumpSerialize)?; @@ -242,20 +199,15 @@ impl RunMetadataWriterHandle { task::spawn_blocking(move || { let mut guard = writer.lock().expect("metadata writer mutex poisoned"); - guard.write_snapshot_blocking(&entries, &message, secret.as_deref()) + guard.write_snapshot_blocking(&entries, &message, token.as_deref()) }) .await .map_err(RunMetadataError::Join)? - .map(|mut snapshot| { - snapshot.token = token_snapshot; - snapshot - }) } } pub(crate) fn build_metadata_writer( run_options: &RunOptions, - token_source: Option>, ) -> Result, RunMetadataError> { if !run_options.settings.run.meta_branch.enabled { return Ok(None); @@ -282,20 +234,10 @@ pub(crate) fn build_metadata_writer( return Ok(None); } - // Share the sandbox's token source so the metadata writer reuses the - // same cached token as every other consumer for this origin. Resumed - // runs reconnect the sandbox without one; they build their own cached - // source from the run's credentials. - let source = match token_source { - Some(source) => source, - None => InstallationTokenSource::for_origin( - creds, - &normalized_url, - serde_json::json!({ "contents": "write" }), - ) - .map_err(RunMetadataError::TokenMint)?, - }; - let auth = Arc::new(GitHubAuthProvider::new(source)); + let auth = Arc::new(GitHubAuthProvider::new( + creds.clone(), + normalized_url.clone(), + )); let writer = RunMetadataWriter::new( normalized_url, meta_branch.clone(), @@ -306,6 +248,28 @@ pub(crate) fn build_metadata_writer( Ok(Some(RunMetadataWriterHandle::new(writer, auth))) } +pub(crate) async fn mint_token( + creds: &fabro_github::GitHubCredentials, + origin_url: &str, + permissions: &HashMap, +) -> anyhow::Result { + let normalized_url = fabro_github::normalize_repo_origin_url(origin_url); + let (owner, repo) = + fabro_github::parse_github_owner_repo(&normalized_url).context("parsing GitHub origin")?; + let client = fabro_http::http_client().map_err(anyhow::Error::new)?; + let permissions = + serde_json::to_value(permissions).context("serializing GitHub permissions")?; + creds + .resolve_bearer_token( + &client, + &owner, + &repo, + &fabro_github::github_api_base_url(), + permissions, + ) + .await +} + pub(crate) struct RunMetadataWriter { store: Store, tempdir: tempfile::TempDir, @@ -395,7 +359,6 @@ impl RunMetadataWriter { push_error, entry_count, bytes, - token: None, }) } @@ -1048,19 +1011,16 @@ mod tests { for (origin, expected) in cases { let options = run_options_for_origin(origin); - let handle = build_metadata_writer(&options, None).unwrap().unwrap(); + let handle = build_metadata_writer(&options).unwrap().unwrap(); assert_eq!(handle.remote_url_for_test(), expected); assert!(!handle.remote_url_for_test().contains("ghs_aaaaaa")); assert!(!handle.remote_url_for_test().contains('@')); } assert!( - build_metadata_writer( - &run_options_for_origin("https://gitlab.com/owner/repo.git"), - None - ) - .unwrap() - .is_none() + build_metadata_writer(&run_options_for_origin("https://gitlab.com/owner/repo.git")) + .unwrap() + .is_none() ); } @@ -1069,54 +1029,6 @@ mod tests { let mut options = run_options_for_origin("https://github.com/owner/repo.git"); options.settings.run.meta_branch.enabled = false; - assert!(build_metadata_writer(&options, None).unwrap().is_none()); - } - #[test] - fn transient_degradation_stays_eligible_for_reprobe() { - let runtime = RunMetadataRuntime::new(); - assert!(runtime.mark_metadata_degraded(true), "first failure warns"); - assert!(runtime.metadata_degraded()); - assert!( - !runtime.metadata_suspended(), - "a transiently degraded writer re-probes at later checkpoints" - ); - assert!( - !runtime.mark_metadata_degraded(true), - "repeat failures do not warn again" - ); - } - - #[test] - fn permanent_degradation_latches_snapshots_off() { - let runtime = RunMetadataRuntime::new(); - runtime.mark_metadata_degraded(false); - assert!(runtime.metadata_suspended()); - - // A later transient failure never upgrades a permanent latch. - runtime.mark_metadata_degraded(true); - assert!(runtime.metadata_suspended()); - } - - #[test] - fn permanent_failure_latches_a_transiently_degraded_writer() { - let runtime = RunMetadataRuntime::new(); - runtime.mark_metadata_degraded(true); - assert!(!runtime.metadata_suspended()); - runtime.mark_metadata_degraded(false); - assert!(runtime.metadata_suspended()); - } - - #[test] - fn successful_snapshot_clears_degradation_and_rearms_the_warning() { - let runtime = RunMetadataRuntime::new(); - assert!(runtime.mark_metadata_degraded(true)); - runtime.clear_metadata_degraded(); - - assert!(!runtime.metadata_degraded()); - assert!(!runtime.metadata_suspended()); - assert!( - runtime.mark_metadata_degraded(false), - "a later independent failure warns again instead of failing silently" - ); + assert!(build_metadata_writer(&options).unwrap().is_none()); } } diff --git a/lib/components/fabro-workflow/src/sandbox_git_runtime.rs b/lib/components/fabro-workflow/src/sandbox_git_runtime.rs index 1178d5c69..2bd246e13 100644 --- a/lib/components/fabro-workflow/src/sandbox_git_runtime.rs +++ b/lib/components/fabro-workflow/src/sandbox_git_runtime.rs @@ -6,35 +6,16 @@ use tokio::sync::OnceCell; use crate::sandbox_git::{GIT_REMOTE, exec_err}; pub(crate) struct SandboxGitRuntime { - probe: OnceCell>, - /// When the run last pushed its branch successfully (checkpoint or - /// publish). Read by the publish failure report so "last success 67s - /// before the failure" is visible from the run conclusion. - last_successful_push_at: std::sync::Mutex>>, + probe: OnceCell>, } impl SandboxGitRuntime { pub(crate) fn new() -> Self { Self { - probe: OnceCell::new(), - last_successful_push_at: std::sync::Mutex::new(None), + probe: OnceCell::new(), } } - pub(crate) fn record_successful_push(&self) { - *self - .last_successful_push_at - .lock() - .expect("last push timestamp mutex poisoned") = Some(chrono::Utc::now()); - } - - pub(crate) fn last_successful_push_at(&self) -> Option> { - *self - .last_successful_push_at - .lock() - .expect("last push timestamp mutex poisoned") - } - pub(crate) async fn ensure_git_available( &self, sandbox: &dyn Sandbox, diff --git a/lib/foundation/fabro-types/src/run_event/misc.rs b/lib/foundation/fabro-types/src/run_event/misc.rs index 9643f9fb1..cde08f65f 100644 --- a/lib/foundation/fabro-types/src/run_event/misc.rs +++ b/lib/foundation/fabro-types/src/run_event/misc.rs @@ -112,56 +112,12 @@ pub struct GitCommitProps { pub sha: String, } -/// One attempt of a retried git push, nested inside [`GitPushProps`]. -/// -/// The durable projection of the sandbox layer's runtime attempt record. -/// Token identity is flattened into the three `token_*` fields — a nested -/// provenance enum never appears in stored events. `classified_reason` is the -/// retry classifier's verdict for a failed attempt (the terminal attempt -/// carries its classification too); whether an attempt was actually retried -/// is positional — every entry except the last. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct GitPushAttemptProps { - /// 1-based attempt number within this push operation. - pub attempt: u32, - pub started_at: chrono::DateTime, - pub success: bool, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub classified_reason: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub exec_output_tail: Option, - /// Generation of the token embedded during this attempt (0 for static - /// credentials). - #[serde(default, skip_serializing_if = "Option::is_none")] - pub token_generation: Option, - /// `minted`, `reused`, or `static`. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub token_provenance: Option, - /// Token age at the attempt; absent for static credentials. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub token_age_ms: Option, - /// What the credential refresh did to the remote this attempt: - /// `embedded`, `unchanged`, or `none`. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub credential_action: Option, - /// A credential `mint` or `set_url` failure this attempt pushed through. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub refresh_error: Option, -} - #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct GitPushProps { pub branch: String, - /// Final outcome of the whole push operation — one `git.push` event per - /// high-level push, so finality is unambiguous. pub success: bool, - /// The final attempt's output tail, unchanged for existing consumers. #[serde(default, skip_serializing_if = "Option::is_none")] pub exec_output_tail: Option, - /// Per-attempt history. Absent on events stored before attempts were - /// recorded. - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub attempts: Vec, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/lib/foundation/fabro-types/src/run_event/mod.rs b/lib/foundation/fabro-types/src/run_event/mod.rs index 6323e5071..99c9c4aa5 100644 --- a/lib/foundation/fabro-types/src/run_event/mod.rs +++ b/lib/foundation/fabro-types/src/run_event/mod.rs @@ -1954,7 +1954,6 @@ mod tests { branch: "refs/heads/run:refs/heads/run".to_string(), success: false, exec_output_tail: Some(tail.clone()), - attempts: Vec::new(), }), ] { let value = serde_json::to_value(&body).unwrap(); @@ -1986,7 +1985,6 @@ mod tests { branch: "refs/heads/run:refs/heads/run".to_string(), success: false, exec_output_tail: None, - attempts: Vec::new(), }), ] { let value = serde_json::to_value(&body).unwrap();