diff --git a/lib/components/fabro-github/src/lib.rs b/lib/components/fabro-github/src/lib.rs index bac1f5fbc..bfb62741f 100644 --- a/lib/components/fabro-github/src/lib.rs +++ b/lib/components/fabro-github/src/lib.rs @@ -194,6 +194,12 @@ pub enum GitHubCredentials { } impl GitHubCredentials { + /// Whether resolving these credentials mints a new installation token. + #[must_use] + pub fn mints_installation_token(&self) -> bool { + matches!(self, Self::App(_)) + } + pub fn from_env(app_id: Option<&str>) -> Result, String> { Ok(GitHubAppCredentials::from_env(app_id)?.map(Self::App)) } @@ -372,7 +378,7 @@ pub fn parse_github_owner_repo(url: &str) -> anyhow::Result<(String, String)> { let path = path.trim_end_matches('/'); let path = path.strip_suffix(".git").unwrap_or(path); - let mut parts = path.splitn(3, '/'); + let mut parts = path.split('/'); let owner = parts .next() .filter(|s| !s.is_empty()) @@ -381,6 +387,9 @@ pub fn parse_github_owner_repo(url: &str) -> anyhow::Result<(String, String)> { .next() .filter(|s| !s.is_empty()) .ok_or_else(|| anyhow!("Missing repo in GitHub URL: {display_url}"))?; + if parts.next().is_some() { + bail!("GitHub URL must identify one repository: {display_url}"); + } Ok((owner.to_string(), repo.to_string())) } @@ -1388,6 +1397,16 @@ mod tests { assert_eq!(repo, "repo"); } + #[test] + fn parse_rejects_extra_path_components() { + let error = parse_github_owner_repo("https://github.com/owner/repo/issues").unwrap_err(); + + assert!( + error.to_string().contains("must identify one repository"), + "got: {error}" + ); + } + // ----------------------------------------------------------------------- // ssh_url_to_https // ----------------------------------------------------------------------- @@ -2312,6 +2331,24 @@ mod tests { assert!(!fresh.near_expiry(std::time::Duration::from_mins(15))); } + #[test] + fn only_app_credentials_mint_installation_tokens() { + let app = GitHubCredentials::App(GitHubAppCredentials { + app_id: "test".to_string(), + private_key_pem: test_rsa_key().to_string(), + slug: None, + }); + let pat = GitHubCredentials::Pat("ghp_personal".to_string()); + let installation = GitHubCredentials::Installation(InstallationToken { + token: "ghs_installation".to_string(), + expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), + }); + + assert!(app.mints_installation_token()); + assert!(!pat.mints_installation_token()); + assert!(!installation.mints_installation_token()); + } + #[test] fn validate_static_github_token_rejects_installation_tokens() { validate_static_github_token("ghp_personal").unwrap(); 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/clone_source.rs b/lib/components/fabro-sandbox/src/clone_source.rs index 8988f3d47..1a90c6b21 100644 --- a/lib/components/fabro-sandbox/src/clone_source.rs +++ b/lib/components/fabro-sandbox/src/clone_source.rs @@ -32,6 +32,8 @@ pub(crate) fn github_repo_layout( "Clone-based sandboxes currently support GitHub repository origins only: {err}" )) })?; + validate_path_component("owner", &owner)?; + validate_path_component("repository", &repo)?; let workspace_root = trim_root(workspace_root); let repos_root = trim_root(repos_root); let repos_owner_path = sandbox::join_sandbox_path(repos_root, &owner); @@ -48,6 +50,27 @@ pub(crate) fn github_repo_layout( }) } +fn validate_path_component(label: &str, component: &str) -> crate::Result<()> { + let is_safe = !matches!(component, "." | "..") + && component + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_' | b'.')); + if !is_safe { + return Err(crate::Error::message(format!( + "GitHub {label} is not a safe repository path component" + ))); + } + Ok(()) +} + +pub(crate) fn repo_symlink_command(layout: &GitHubRepoLayout) -> String { + format!( + "ln -s {} {}", + sandbox::shell_quote(&layout.primary_repo_path), + sandbox::shell_quote(&layout.primary_repo_link), + ) +} + fn trim_root(root: &str) -> &str { let trimmed = root.trim_end_matches('/'); if trimmed.is_empty() { "/" } else { trimmed } @@ -204,6 +227,37 @@ mod tests { assert_eq!(layout.execution_directory, "/workspace/fabro"); } + #[test] + fn github_layout_rejects_path_traversal_components() { + for origin in [ + "https://github.com/../widgets", + "https://github.com/acme/..", + "https://github.com/%2e%2e/widgets", + ] { + let error = github_repo_layout(origin, "/workspace", "/repos") + .expect_err("unsafe path component should fail"); + assert!( + error.to_string().contains("safe repository path component"), + "got {error} for {origin}" + ); + } + } + + #[test] + fn repo_symlink_command_quotes_both_paths() { + let layout = github_repo_layout( + "https://github.com/fabro-sh/fabro", + "/work space", + "/repo root", + ) + .unwrap(); + + assert_eq!( + repo_symlink_command(&layout), + "ln -s '/repo root/fabro-sh/fabro' '/work space/fabro'" + ); + } + #[test] fn record_origin_strips_credentials() { assert_eq!( diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 8de651f54..36b94f9ee 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -16,7 +16,7 @@ use daytona_sdk::toolbox_types::Command as SessionCommandResult; use daytona_sdk::{DaytonaError, SessionCommandLogsResult}; use fabro_github::GitHubCredentials; use fabro_static::EnvVars; -use fabro_types::{CommandOutputStream, CommandTermination, RunId}; +use fabro_types::{CommandOutputStream, CommandTermination, RunId, SandboxProviderKind}; use fabro_util::time::elapsed_ms; use rand::Rng; use tokio::runtime::Handle; @@ -25,6 +25,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::{ @@ -1050,6 +1051,10 @@ 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(), @@ -1057,40 +1062,26 @@ impl Sandbox for DaytonaSandbox { let clone_start = Instant::now(); let (username, password) = match &self.github_app { - Some(creds) => { - let (owner, repo) = fabro_github::parse_github_owner_repo(&origin_url) - .map_err(|e| { - let err = crate::Error::message(format!( - "Failed to parse GitHub URL for clone: {e}" - )); - self.emit(SandboxEvent::GitCloneFailed { - url: origin_url.clone(), - error: err.to_string(), - causes: err.causes(), - }); - err - })?; - fabro_github::resolve_clone_credentials( - &fabro_github::GitHubContext::new( - creds, - &fabro_github::github_api_base_url(), - ), - &owner, - &repo, - ) - .await - .map_err(|e| { - let err = crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )); - self.emit(SandboxEvent::GitCloneFailed { - url: origin_url.clone(), - error: err.to_string(), - causes: err.causes(), - }); - self.fail_init(init_start, err) - })? - } + 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}" + )); + self.emit(SandboxEvent::GitCloneFailed { + url: origin_url.clone(), + error: err.to_string(), + causes: err.causes(), + }); + self.fail_init(init_start, err) + })?, None => (None, None), }; @@ -1155,19 +1146,24 @@ impl Sandbox for DaytonaSandbox { self.fail_init(init_start, err) })?; - let clone_token = password.clone(); - let clone_result = git_svc - .clone( - &origin_url, - &layout.primary_repo_path, - daytona_sdk::GitCloneOptions { - branch, - username, - password, + let clone_result = clone_retry::retry_clone( + SandboxProviderKind::Daytona, + None, + |_attempt| { + let git_svc = &git_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 { git_svc.clone(origin, target, options).await } + }, + |err: &DaytonaError| classify_clone_failure(err, token_was_freshly_minted), + ) + .await; match clone_result { Ok(()) => { @@ -1181,7 +1177,7 @@ impl Sandbox for DaytonaSandbox { }); self.fail_init(init_start, err) })?; - let symlink_cmd = daytona_symlink_command(&layout); + let symlink_cmd = clone_source::repo_symlink_command(&layout); let symlink_result = process_svc .execute_command( &wrap_bash_command(&symlink_cmd), @@ -1233,8 +1229,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(token) = clone_token { - match fabro_github::embed_token_in_url(&origin_url, &token) { + 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 {}", @@ -1536,7 +1532,7 @@ impl Sandbox for DaytonaSandbox { // 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 !matches!(creds, GitHubCredentials::App(_)) { + if !creds.mints_installation_token() { return Ok(RefreshOutcome::Skipped); } @@ -2490,12 +2486,36 @@ fn daytona_bash_session_probe_outcome(execution: crate::Result) -> c )) } -fn daytona_symlink_command(layout: &clone_source::GitHubRepoLayout) -> String { - format!( - "ln -s {} {}", - shell_quote(&layout.primary_repo_path), - shell_quote(&layout.primary_repo_link), - ) +/// 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, + 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 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(CloneRetryReason::TransientInfra) + } + DaytonaError::Timeout { .. } + | DaytonaError::Api { .. } + | DaytonaError::NotFound { .. } + | DaytonaError::General(_) => None, + }, + } } /// Wrap Bash source in the canonical non-login Bash transport. @@ -2988,18 +3008,78 @@ mod tests { } #[test] - fn daytona_symlink_command_links_workspace_repo_to_repos_checkout() { - let layout = clone_source::github_repo_layout( - "https://github.com/fabro-sh/fabro", - WORKING_DIRECTORY, - REPOS_ROOT, - ) - .unwrap(); + 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!( - daytona_symlink_command(&layout), - "ln -s /home/daytona/repos/fabro-sh/fabro /home/daytona/workspace/fabro" + 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::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_timeout_is_not_retried_without_remote_termination() { + let err = DaytonaError::timeout("request timed out"); + + 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, 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), true), + None, + "expected {message:?} to take precedence over HTTP 500" + ); + } + } + + #[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] diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index 6236d425d..f6cb40891 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -3,7 +3,7 @@ use std::fmt::Write as _; use std::io::Cursor; use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; -use std::time::{Instant, SystemTime, UNIX_EPOCH}; +use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use async_trait::async_trait; use bollard::Docker; @@ -17,7 +17,7 @@ use bollard::exec::{CreateExecOptions, StartExecOptions, StartExecResults}; use bollard::image::CreateImageOptions; use bollard::models::{ContainerInspectResponse, HostConfig}; use fabro_github::GitHubCredentials; -use fabro_types::{CommandOutputStream, CommandTermination, RunId}; +use fabro_types::{CommandOutputStream, CommandTermination, RunId, SandboxProviderKind}; use fabro_util::time::elapsed_ms; use futures::StreamExt; use tokio::io::{AsyncWriteExt, duplex}; @@ -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 \ @@ -46,6 +46,7 @@ const DOCKER_BASH_REQUIREMENT: &str = "Docker sandboxes require /bin/bash for ev pub(crate) const WORKING_DIRECTORY: &str = "/workspace"; pub(crate) const REPOS_ROOT: &str = "/repos"; const GIT_CLONE_DEPTH: usize = 10; +const GIT_CLONE_TIMEOUT: Duration = Duration::from_mins(5); #[cfg(test)] const EXEC_STOP_POLL_SLEEP_SECONDS: &str = "0.005"; #[cfg(not(test))] @@ -55,6 +56,11 @@ const EXEC_TERM_GRACE_SECONDS: &str = "0.02"; #[cfg(not(test))] const EXEC_TERM_GRACE_SECONDS: &str = "0.2"; +struct DockerCloneFailure { + error: crate::Error, + retry_reason: Option, +} + fn env_entry_name(entry: &str) -> &str { entry.split_once('=').map_or(entry, |(name, _)| name) } @@ -681,6 +687,32 @@ impl DockerSandbox { Ok(()) } + /// Preserve a failed `git clone` result while masking the auth URL. + fn clone_failure_error( + &self, + result: ExecResult, + auth_url: Option<&fabro_redact::DisplaySafeUrl>, + ) -> crate::Error { + let error = result + .into_exec_error_with_redactor("git clone", |output| redact_auth_url(output, auth_url)); + let message = if self.github_app.is_none() { + "Git clone failed. If this is a private repository, configure a GitHub App with \ + `fabro install` and install it for your organization." + } else { + "Failed to clone repository into Docker sandbox" + }; + crate::Error::context(message, error) + } + + fn report_clone_failure(&self, origin_url: &str, err: crate::Error) -> crate::Error { + self.emit(SandboxEvent::GitCloneFailed { + url: origin_url.to_string(), + error: err.to_string(), + causes: err.causes(), + }); + err + } + async fn clone_github_repo( &self, origin_url: String, @@ -688,12 +720,10 @@ impl DockerSandbox { ) -> crate::Result<()> { self.verify_git_available().await?; let layout = clone_source::github_repo_layout(&origin_url, WORKING_DIRECTORY, REPOS_ROOT)?; - - self.emit(SandboxEvent::GitCloneStarted { - url: origin_url.clone(), - branch: branch.clone(), - }); - let clone_start = Instant::now(); + let token_was_freshly_minted = self + .github_app + .as_ref() + .is_some_and(GitHubCredentials::mints_installation_token); let auth_url = match &self.github_app { Some(creds) => Some( @@ -714,26 +744,101 @@ impl DockerSandbox { .as_ref() .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); + self.emit(SandboxEvent::GitCloneStarted { + url: origin_url.clone(), + branch: branch.clone(), + }); + let clone_start = Instant::now(); - 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}") - }); - self.emit(SandboxEvent::GitCloneFailed { - url: origin_url, - error: err.to_string(), - causes: err.causes(), - }); - return Err(err); + let prepare_command = format!( + "mkdir -p {} {}", + shell_quote(WORKING_DIRECTORY), + shell_quote(&layout.repos_owner_path), + ); + match self + .docker_exec_shell(&prepare_command, 10_000, Some("/"), None, None) + .await + { + Ok(result) if result.is_success() => {} + Ok(result) => { + let err = result.into_exec_error("prepare Docker repository checkout"); + return Err(self.report_clone_failure(&origin_url, err)); + } + Err(err) => { + return Err(self.report_clone_failure(&origin_url, err)); + } + } + + 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_result = clone_retry::retry_clone( + SandboxProviderKind::Docker, + Some(clone_deadline), + |_attempt| { + let command = command.as_str(); + let auth_url = auth_url.as_ref(); + async move { + let remaining = clone_deadline.saturating_duration_since(time::Instant::now()); + let timeout_ms = u64::try_from(remaining.as_millis()).unwrap_or(u64::MAX); + if timeout_ms == 0 { + return Err(DockerCloneFailure { + error: crate::Error::message( + "Docker git clone deadline expired before retry", + ), + retry_reason: None, + }); + } + let result = self + .docker_exec_shell_streaming( + command, + Some(timeout_ms), + Some("/"), + None, + None, + None, + ) + .await + .map_err(|error| DockerCloneFailure { + error: crate::Error::context( + "Docker git clone transport failed", + error, + ), + retry_reason: None, + })? + .result; + if result.is_success() { + return Ok(()); + } + let retry_reason = + classify_docker_clone_result(&result, token_was_freshly_minted); + Err(DockerCloneFailure { + error: self.clone_failure_error(result, auth_url), + retry_reason, + }) + } + }, + |failure: &DockerCloneFailure| failure.retry_reason, + ) + .await; + + if let Err(failure) = clone_result { + let err = failure.error; + return Err(self.report_clone_failure(&origin_url, err)); + } + + let symlink_command = clone_source::repo_symlink_command(&layout); + match self + .docker_exec_shell(&symlink_command, 10_000, Some("/"), None, None) + .await + { + Ok(result) if result.is_success() => {} + Ok(result) => { + let err = result.into_exec_error("create Docker workspace repo symlink"); + return Err(self.report_clone_failure(&origin_url, err)); + } + Err(err) => { + return Err(self.report_clone_failure(&origin_url, err)); + } } let _ = self.repo_cloned.set(true); @@ -1252,19 +1357,17 @@ fn git_clone_command(clone_url: &str, branch: Option<&str>, checkout_path: &str) command } -fn git_clone_and_link_command( - clone_url: &str, - branch: Option<&str>, - layout: &clone_source::GitHubRepoLayout, -) -> String { - format!( - "mkdir -p {} {} && {} && ln -s {} {}", - shell_quote(WORKING_DIRECTORY), - shell_quote(&layout.repos_owner_path), - git_clone_command(clone_url, branch, &layout.primary_repo_path), - shell_quote(&layout.primary_repo_path), - shell_quote(&layout.primary_repo_link), - ) +fn classify_docker_clone_result( + result: &ExecResult, + token_was_freshly_minted: bool, +) -> Option { + let stderr = clone_retry::classify_message(&result.stderr, token_was_freshly_minted); + match stderr { + clone_retry::CloneMessageClass::Unknown => { + clone_retry::classify_message(&result.stdout, token_was_freshly_minted).retry_reason() + } + class => class.retry_reason(), + } } fn host_config(config: &DockerSandboxOptions) -> HostConfig { @@ -2079,7 +2182,7 @@ impl Sandbox for DockerSandbox { // 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 !matches!(creds, GitHubCredentials::App(_)) { + if !creds.mints_installation_token() { return Ok(RefreshOutcome::Skipped); } @@ -2307,20 +2410,16 @@ mod tests { } #[test] - fn clone_and_link_command_creates_workspace_symlink_to_repos_checkout() { - let layout = clone_source::github_repo_layout( - "https://github.com/fabro-sh/fabro", - "/workspace", - "/repos", - ) - .unwrap(); - let command = - git_clone_and_link_command("https://github.com/fabro-sh/fabro", Some("main"), &layout); + fn clone_result_uses_stderr_before_stdout() { + let result = ExecResult { + stdout: "Could not resolve host: github.com".to_string(), + stderr: "fatal: destination path 'fabro' already exists".to_string(), + exit_code: Some(128), + termination: CommandTermination::Exited, + duration_ms: 1, + }; - assert_eq!( - command, - "mkdir -p /workspace /repos/fabro-sh && git -c maintenance.auto=0 -c gc.auto=0 clone --branch main --single-branch --depth 10 --no-tags -- https://github.com/fabro-sh/fabro /repos/fabro-sh/fabro && ln -s /repos/fabro-sh/fabro /workspace/fabro" - ); + assert_eq!(classify_docker_clone_result(&result, true), None); } #[test] diff --git a/lib/components/fabro-sandbox/src/error.rs b/lib/components/fabro-sandbox/src/error.rs index 7b2377660..8d6bf0d73 100644 --- a/lib/components/fabro-sandbox/src/error.rs +++ b/lib/components/fabro-sandbox/src/error.rs @@ -143,7 +143,7 @@ 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 - repository is unavailable to the current credentials") } 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;