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..d94a52485 --- /dev/null +++ b/lib/components/fabro-sandbox/src/clone_retry.rs @@ -0,0 +1,333 @@ +//! Retry for the first repository clone in a clone-based sandbox. +//! +//! Clone-based providers mint a GitHub installation access token and clone with +//! it in the same breath. GitHub replicates a new token to its edge cache sites +//! asynchronously, so a clone that starts within a second of the mint can be +//! rejected before the token is visible to the site serving it. On a private +//! repository that rejection arrives as `Repository not found.`, because GitHub +//! answers unauthorized reads with 404 rather than 403. +//! +//! A successful mint is what makes that message safe to retry. +//! `resolve_clone_credentials` already fails loudly on every deterministic +//! explanation for a clone 404: the installation lookup 404s when the App is +//! not installed for the owner, and token creation 422s when the installation +//! does not cover the repository. So once credentials are in hand, `not found` +//! from the clone itself cannot mean "no access" — the repository exists and +//! the token covers it. +//! +//! 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_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, +} + +/// 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. +/// +/// `has_credentials` gates the token-replication reading. Without credentials +/// there is no token to replicate, so `not found` on a public clone means the +/// URL names a repository that is not there — that should fail immediately +/// rather than burn 12 seconds of backoff. +pub(crate) fn classify_message(message: &str, has_credentials: bool) -> Option { + let lower = message.to_ascii_lowercase(); + + if TRANSIENT_HINTS.iter().any(|hint| lower.contains(hint)) { + return Some(CloneRetryReason::TransientInfra); + } + if has_credentials + && TOKEN_REPLICATION_HINTS + .iter() + .any(|hint| lower.contains(hint)) + { + return Some(CloneRetryReason::TokenReplication); + } + None +} + +/// 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 so the caller can clear +/// leftovers from the previous try before cloning again. `classify` decides +/// whether an error is worth repeating; `None` returns it to the caller +/// untouched. The error from the final attempt is returned as-is, so callers +/// keep the cause chain they would have had without retries. +pub(crate) async fn retry_clone( + provider: &'static str, + 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); + // 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, + 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), + Some(CloneRetryReason::TokenReplication) + ); + } + + #[test] + fn not_found_without_credentials_is_a_wrong_url() { + assert_eq!( + classify_message("repository not found: Repository not found.", false), + None + ); + } + + #[test] + fn auth_failure_with_credentials_is_a_replication_lag() { + assert_eq!( + classify_message( + "fatal: Authentication failed for 'https://github.com/owner/repo'", + true + ), + Some(CloneRetryReason::TokenReplication) + ); + } + + #[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), + Some(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), + None, + "expected {message:?} to fail fast" + ); + } + } + + #[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( + "test", + |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( + "test", + |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( + "test", + |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( + "test", + |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" + ); + } +} diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 292cc3ebe..39798e54a 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -24,6 +24,7 @@ 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::redact::redact_auth_url; use crate::sandbox::{ @@ -1066,7 +1067,7 @@ impl Sandbox for DaytonaSandbox { error: err.to_string(), causes: err.causes(), }); - err + self.fail_init(init_start, err) })?; fabro_github::resolve_clone_credentials( &fabro_github::GitHubContext::new( @@ -1154,18 +1155,34 @@ impl Sandbox for DaytonaSandbox { })?; let clone_token = password.clone(); - let clone_result = git_svc - .clone( - &origin_url, - &layout.primary_repo_path, - daytona_sdk::GitCloneOptions { - branch, - username, - password, + let has_credentials = password.is_some(); + let clone_result = clone_retry::retry_clone( + "daytona", + |attempt| { + let git_svc = &git_svc; + let fs_svc = &fs_svc; + let origin = origin_url.as_str(); + let target = layout.primary_repo_path.as_str(); + let options = daytona_sdk::GitCloneOptions { + branch: branch.clone(), + username: username.clone(), + password: password.clone(), ..Default::default() - }, - ) - .await; + }; + async move { + if attempt > 1 { + // git removes the directory it created when it + // exits on its own, but a clone killed part-way + // leaves a partial checkout that the next + // `git clone` would refuse to write into. + let _ = fs_svc.delete_file(target, true).await; + } + git_svc.clone(origin, target, options).await + } + }, + |err: &DaytonaError| classify_clone_failure(err, has_credentials), + ) + .await; match clone_result { Ok(()) => { @@ -2469,6 +2486,24 @@ fn daytona_bash_session_probe_outcome(execution: crate::Result) -> c )) } +/// Classify a failed Daytona clone for retry. +/// +/// The GitHub 404 does not arrive as an HTTP 404 on the Daytona call. git runs +/// 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, has_credentials: bool) -> Option { + let transient_transport = match err { + DaytonaError::Timeout { .. } | DaytonaError::RateLimit { .. } => true, + DaytonaError::Api { status_code, .. } => (500..600).contains(status_code), + DaytonaError::NotFound { .. } | DaytonaError::General(_) => false, + }; + if transient_transport { + return Some(CloneRetryReason::TransientInfra); + } + clone_retry::classify_message(err.message(), has_credentials) +} + fn daytona_symlink_command(layout: &clone_source::GitHubRepoLayout) -> String { format!( "ln -s {} {}", @@ -2859,6 +2894,54 @@ mod tests { ); } + #[test] + fn clone_not_found_after_a_successful_mint_is_retried() { + // The exact error from run 01KYM99DF27JRRW4XSYZBP27K7: git's stderr, + // relayed through the toolbox, five seconds after a token was minted. + let err = DaytonaError::general("repository not found: Repository not found."); + + assert_eq!( + classify_clone_failure(&err, true), + Some(CloneRetryReason::TokenReplication) + ); + assert_eq!( + classify_clone_failure(&err, false), + None, + "without credentials there is no token to replicate" + ); + } + + #[test] + fn clone_transient_transport_failures_are_retried() { + for err in [ + DaytonaError::timeout("request timed out"), + DaytonaError::rate_limit("too many requests"), + DaytonaError::api(503, ""), + ] { + assert_eq!( + classify_clone_failure(&err, false), + Some(CloneRetryReason::TransientInfra), + "expected {err:?} to be transient" + ); + } + } + + #[test] + fn clone_client_errors_are_not_retried() { + for err in [ + DaytonaError::api(400, "bad request"), + DaytonaError::api(403, "forbidden"), + DaytonaError::not_found("Sandbox not found"), + DaytonaError::general("fatal: could not read Username for 'https://github.com'"), + ] { + assert_eq!( + classify_clone_failure(&err, true), + None, + "expected {err:?} to fail fast" + ); + } + } + #[test] fn wrap_fs_error_classifies_http_400_and_403() { let err_400 = wrap_fs_error( diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index fb9b7df9c..c9fd3ce4c 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -37,7 +37,7 @@ use crate::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, SandboxFile, StderrCollector, StdioProcess, StdioProcessHandle, StdioProcessTermination, WalkOptions, - format_lines_numbered, shell_quote, + 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 \ @@ -656,6 +656,42 @@ impl DockerSandbox { Ok(()) } + /// Render a failed `git clone` exit into an error, with the auth URL + /// masked. + fn clone_failure_error( + &self, + stderr: &str, + auth_url: Option<&fabro_redact::DisplaySafeUrl>, + ) -> crate::Error { + let stderr = redact_auth_url(stderr, auth_url); + crate::Error::message(if self.github_app.is_none() { + format!( + "Git clone failed: {stderr}. If this is a private repository, configure a GitHub App with `fabro install` and install it for your organization." + ) + } else { + format!("Failed to clone repo into Docker sandbox: {stderr}") + }) + } + + /// Clear a checkout left behind by a failed clone, before cloning again. + /// + /// git removes the directory it created when it exits on its own, but a + /// clone killed by the exec timeout leaves a partial checkout that the next + /// `git clone` would refuse to write into. Best-effort: if the removal + /// fails, the retry surfaces the real error. + async fn clear_partial_clone(&self, repo_path: &str) { + let command = format!("rm -rf {}", shell_quote(repo_path)); + if let Err(err) = self + .docker_exec_shell(&command, 30_000, Some("/"), None, None) + .await + { + tracing::debug!( + error = %crate::display_for_log(&err), + "Failed to clear partial clone before retry" + ); + } + } + async fn clone_github_repo( &self, origin_url: String, @@ -690,19 +726,34 @@ impl DockerSandbox { .map_or(origin_url.as_str(), |url| url.as_raw_url().as_str()); let command = git_clone_and_link_command(clone_url, branch.as_deref(), &layout); + let has_credentials = auth_url.is_some(); - let result = self - .docker_exec_shell(&command, 300_000, Some("/"), None, None) - .await?; - if !result.is_success() { - let stderr = redact_auth_url(&result.stderr, auth_url.as_ref()); - let err = crate::Error::message(if self.github_app.is_none() { - format!( - "Git clone failed: {stderr}. If this is a private repository, configure a GitHub App with `fabro install` and install it for your organization." - ) - } else { - format!("Failed to clone repo into Docker sandbox: {stderr}") - }); + let clone_result = clone_retry::retry_clone( + "docker", + |attempt| { + let command = command.as_str(); + let auth_url = auth_url.as_ref(); + let repo_path = layout.primary_repo_path.as_str(); + async move { + if attempt > 1 { + self.clear_partial_clone(repo_path).await; + } + let result = self + .docker_exec_shell(command, 300_000, Some("/"), None, None) + .await?; + if result.is_success() { + return Ok(()); + } + Err(self.clone_failure_error(&result.stderr, auth_url)) + } + }, + |err: &crate::Error| { + clone_retry::classify_message(&err.display_with_causes(), has_credentials) + }, + ) + .await; + + if let Err(err) = clone_result { self.emit(SandboxEvent::GitCloneFailed { url: origin_url, error: err.to_string(), diff --git a/lib/components/fabro-sandbox/src/error.rs b/lib/components/fabro-sandbox/src/error.rs index 7b2377660..d872f9ac3 100644 --- a/lib/components/fabro-sandbox/src/error.rs +++ b/lib/components/fabro-sandbox/src/error.rs @@ -143,7 +143,11 @@ pub(crate) fn classify_exec_failure(stderr: &str) -> Option<&'static str> { } else if lower.contains("could not resolve host") || lower.contains("network is unreachable") { Some("network failure inside sandbox - check DNS / egress from the run container") } else if lower.contains("repository not found") { - Some("github 404 - the App installation may not include this repo") + Some( + "github 404 - the repo does not exist, the App installation does not \ + include it, or a freshly minted installation token has not replicated \ + to GitHub's edge cache yet (github answers unauthorized reads with 404)", + ) } else if lower.contains("no such remote") && lower.contains("origin") { Some("origin remote missing - push credentials could not be installed") } else if lower.contains("not a git repository") diff --git a/lib/components/fabro-sandbox/src/lib.rs b/lib/components/fabro-sandbox/src/lib.rs index edf567ada..06285ae37 100644 --- a/lib/components/fabro-sandbox/src/lib.rs +++ b/lib/components/fabro-sandbox/src/lib.rs @@ -9,6 +9,9 @@ pub mod sandbox_spec; #[cfg(any(feature = "docker", feature = "daytona"))] mod clone_source; +#[cfg(any(feature = "docker", feature = "daytona", test))] +mod clone_retry; + #[cfg(any(feature = "docker", feature = "daytona", test))] mod managed_labels;