From d2510447fe69e89cb84a1c19377f17292b58f139 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 09:03:47 -0400 Subject: [PATCH 1/2] Retry the sandbox clone when GitHub token replication lags A Daytona-backed run could fail five seconds after start when the sandbox git clone hit a transient GitHub "Repository not found" error. Clone-based providers mint an installation access token and clone with it in the same breath, but GitHub replicates a new token to its edge cache sites asynchronously. A clone that starts within a second of the mint can be rejected before the token is visible to the site serving it, and on a private repo that rejection arrives as "Repository not found" because GitHub answers unauthorized reads with 404. Nothing retried the clone, and the failure classified as `deterministic`, which is the one category `loop_restart` refuses to restart. An identical run relaunched 46 seconds later succeeded with no changes. A successful mint is what makes the 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 repo. Once credentials are in hand, "not found" from the clone itself cannot mean "no access". Add `clone_retry` and use it from both clone-based providers: 3 attempts with 3s then 9s backoff, reusing the same token so replication keeps making progress instead of restarting the clock. Token-replication signatures retry only when credentials are present, so a public clone of a wrong URL still fails fast. Infrastructure failures retry either way. The Docker provider had the identical single-shot clone and is the default runtime provider, so it is covered too. Also fix two nearby issues found while reading the area: - The GitHub-URL-parse path in the Daytona clone skipped `fail_init`, unlike every sibling path, so `InitializeFailed` was never emitted. - The `classify_exec_failure` hint for "repository not found" asserted the App installation may not cover the repo. After a successful scoped mint that diagnosis is impossible, and it sent operators hunting a configuration problem that did not exist. Co-Authored-By: Claude Opus 5 (1M context) --- .../fabro-sandbox/src/clone_retry.rs | 333 ++++++++++++++++++ .../fabro-sandbox/src/daytona/mod.rs | 107 +++++- lib/components/fabro-sandbox/src/docker.rs | 77 +++- lib/components/fabro-sandbox/src/error.rs | 6 +- lib/components/fabro-sandbox/src/lib.rs | 3 + 5 files changed, 500 insertions(+), 26 deletions(-) create mode 100644 lib/components/fabro-sandbox/src/clone_retry.rs 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; From 4727ee8e755cda8cdaa7ba3593012565f7061e1d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 16:26:41 -0400 Subject: [PATCH 2/2] Harden sandbox clone retries --- lib/components/fabro-github/src/lib.rs | 39 +++- .../fabro-sandbox/src/clone_retry.rs | 153 +++++++++---- .../fabro-sandbox/src/clone_source.rs | 54 +++++ .../fabro-sandbox/src/daytona/mod.rs | 171 +++++++------- lib/components/fabro-sandbox/src/docker.rs | 212 +++++++++++------- lib/components/fabro-sandbox/src/error.rs | 6 +- 6 files changed, 417 insertions(+), 218 deletions(-) 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 index d94a52485..f0f3f0572 100644 --- a/lib/components/fabro-sandbox/src/clone_retry.rs +++ b/lib/components/fabro-sandbox/src/clone_retry.rs @@ -1,19 +1,12 @@ //! 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. +//! 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. //! -//! 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. +//! 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 @@ -22,6 +15,7 @@ use std::future::Future; use std::time::Duration; +use fabro_types::SandboxProviderKind; use fabro_util::backoff::BackoffPolicy; use tokio::time; @@ -39,6 +33,23 @@ pub(crate) enum CloneRetryReason { 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. @@ -76,24 +87,35 @@ const TOKEN_REPLICATION_HINTS: &[&str] = &[ /// 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 { +/// `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 Some(CloneRetryReason::TransientInfra); + return CloneMessageClass::Retry(CloneRetryReason::TransientInfra); } - if has_credentials - && TOKEN_REPLICATION_HINTS - .iter() - .any(|hint| lower.contains(hint)) + if TOKEN_REPLICATION_HINTS + .iter() + .any(|hint| lower.contains(hint)) { - return Some(CloneRetryReason::TokenReplication); + return if token_was_freshly_minted { + CloneMessageClass::Retry(CloneRetryReason::TokenReplication) + } else { + CloneMessageClass::Permanent + }; } - None + 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. @@ -112,13 +134,13 @@ fn backoff() -> BackoffPolicy { /// 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. +/// `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: &'static str, + provider: SandboxProviderKind, + deadline: Option, mut attempt: Attempt, classify: Classify, ) -> Result @@ -137,11 +159,16 @@ where 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 = %provider, attempt = attempt_number, max_attempts = MAX_ATTEMPTS, reason = %reason, @@ -184,26 +211,33 @@ mod tests { 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) + CloneMessageClass::Retry(CloneRetryReason::TokenReplication) ); } #[test] - fn not_found_without_credentials_is_a_wrong_url() { + fn not_found_without_a_fresh_token_is_permanent() { assert_eq!( classify_message("repository not found: Repository not found.", false), - None + CloneMessageClass::Permanent ); } #[test] - fn auth_failure_with_credentials_is_a_replication_lag() { + 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 ), - Some(CloneRetryReason::TokenReplication) + CloneMessageClass::Retry(CloneRetryReason::TokenReplication) + ); + assert_eq!( + classify_message( + "fatal: Authentication failed for 'https://github.com/owner/repo'", + false + ), + CloneMessageClass::Permanent ); } @@ -217,7 +251,7 @@ mod tests { ] { assert_eq!( classify_message(message, false), - Some(CloneRetryReason::TransientInfra), + CloneMessageClass::Retry(CloneRetryReason::TransientInfra), "expected {message:?} to be transient" ); } @@ -232,12 +266,20 @@ mod tests { ] { assert_eq!( classify_message(message, true), - None, + 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(); @@ -250,7 +292,8 @@ mod tests { let attempts = Attempts::default(); let result = retry_clone( - "test", + SandboxProviderKind::Docker, + None, |attempt| { attempts.record(attempt); async move { Ok::<_, String>(attempt) } @@ -268,7 +311,8 @@ mod tests { let attempts = Attempts::default(); let result = retry_clone( - "test", + SandboxProviderKind::Docker, + None, |attempt| { attempts.record(attempt); async move { @@ -292,7 +336,8 @@ mod tests { let attempts = Attempts::default(); let result = retry_clone( - "test", + SandboxProviderKind::Docker, + None, |attempt| { attempts.record(attempt); async move { Err::<(), _>(format!("Repository not found. (attempt {attempt})")) } @@ -314,7 +359,8 @@ mod tests { let attempts = Attempts::default(); let result = retry_clone( - "test", + SandboxProviderKind::Docker, + None, |attempt| { attempts.record(attempt); async move { Err::<(), _>("permission denied".to_string()) } @@ -330,4 +376,25 @@ mod tests { "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 39798e54a..6390bb1ef 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -15,7 +15,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; @@ -1049,6 +1049,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(), @@ -1056,40 +1060,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(), - }); - self.fail_init(init_start, 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), }; @@ -1154,13 +1144,11 @@ impl Sandbox for DaytonaSandbox { self.fail_init(init_start, err) })?; - let clone_token = password.clone(); - let has_credentials = password.is_some(); let clone_result = clone_retry::retry_clone( - "daytona", - |attempt| { + SandboxProviderKind::Daytona, + None, + |_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 { @@ -1169,18 +1157,9 @@ impl Sandbox for DaytonaSandbox { password: password.clone(), ..Default::default() }; - 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 - } + async move { git_svc.clone(origin, target, options).await } }, - |err: &DaytonaError| classify_clone_failure(err, has_credentials), + |err: &DaytonaError| classify_clone_failure(err, token_was_freshly_minted), ) .await; @@ -1196,7 +1175,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), @@ -1248,8 +1227,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 {}", @@ -1532,7 +1511,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); } @@ -2492,24 +2471,30 @@ 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, 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); +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; } - clone_retry::classify_message(err.message(), has_credentials) -} -fn daytona_symlink_command(layout: &clone_source::GitHubRepoLayout) -> String { - format!( - "ln -s {} {}", - shell_quote(&layout.primary_repo_path), - shell_quote(&layout.primary_repo_link), - ) + 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. @@ -2879,21 +2864,6 @@ 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(); - - assert_eq!( - daytona_symlink_command(&layout), - "ln -s /home/daytona/repos/fabro-sh/fabro /home/daytona/workspace/fabro" - ); - } - #[test] fn clone_not_found_after_a_successful_mint_is_retried() { // The exact error from run 01KYM99DF27JRRW4XSYZBP27K7: git's stderr, @@ -2914,7 +2884,6 @@ mod tests { #[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, ""), ] { @@ -2926,6 +2895,34 @@ mod tests { } } + #[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 [ diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index c9fd3ce4c..ecb4431ac 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::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}; @@ -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) } @@ -656,40 +662,30 @@ impl DockerSandbox { Ok(()) } - /// Render a failed `git clone` exit into an error, with the auth URL - /// masked. + /// Preserve a failed `git clone` result while masking the auth URL. fn clone_failure_error( &self, - stderr: &str, + result: ExecResult, 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." - ) + 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 { - format!("Failed to clone repo into Docker sandbox: {stderr}") - }) + "Failed to clone repository into Docker sandbox" + }; + crate::Error::context(message, error) } - /// 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" - ); - } + 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( @@ -699,12 +695,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( @@ -725,41 +719,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); - let has_credentials = auth_url.is_some(); + self.emit(SandboxEvent::GitCloneStarted { + url: origin_url.clone(), + branch: branch.clone(), + }); + let clone_start = Instant::now(); + 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( - "docker", - |attempt| { + SandboxProviderKind::Docker, + Some(clone_deadline), + |_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 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(command, 300_000, Some("/"), None, None) - .await?; + .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(()); } - Err(self.clone_failure_error(&result.stderr, auth_url)) + let retry_reason = + classify_docker_clone_result(&result, token_was_freshly_minted); + Err(DockerCloneFailure { + error: self.clone_failure_error(result, auth_url), + retry_reason, + }) } }, - |err: &crate::Error| { - clone_retry::classify_message(&err.display_with_causes(), has_credentials) - }, + |failure: &DockerCloneFailure| failure.retry_reason, ) .await; - if let Err(err) = clone_result { - self.emit(SandboxEvent::GitCloneFailed { - url: origin_url, - error: err.to_string(), - causes: err.causes(), - }); - return Err(err); + 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); @@ -1215,19 +1269,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 { @@ -2031,7 +2083,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); } @@ -2208,20 +2260,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 d872f9ac3..8d6bf0d73 100644 --- a/lib/components/fabro-sandbox/src/error.rs +++ b/lib/components/fabro-sandbox/src/error.rs @@ -143,11 +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 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)", - ) + 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")