Skip to content

Retry the sandbox clone when GitHub token replication lags - #667

Merged
brynary merged 2 commits into
mainfrom
fix/retry-transient-sandbox-clone
Jul 28, 2026
Merged

Retry the sandbox clone when GitHub token replication lags#667
brynary merged 2 commits into
mainfrom
fix/retry-transient-sandbox-clone

Conversation

@brynary

@brynary brynary commented Jul 28, 2026

Copy link
Copy Markdown
Member

Problem

A Daytona-backed run failed five seconds after start when the sandbox git clone hit a transient GitHub Repository not found error. Run 01KYM99DF27JRRW4XSYZBP27K7:

seq time event
7 11:55:46.189 sandbox.snapshot.ready (31 ms, cache hit)
8 11:55:49.256 sandbox.git.started
9 11:55:51.249 sandbox.git.failed["repository not found: Repository not found."]
10 run.failed, category deterministic

An identical run relaunched 46 seconds later succeeded with no changes, and a different run cloned the same repo successfully 8 minutes earlier. The repo existed and the App installation covered it.

Three things combined:

  1. The token was used within the same second it was minted. GitHub replicates a new installation access token to its edge cache sites asynchronously. A clone that starts before replication finishes can be rejected, and on a private repo that rejection arrives as Repository not found because GitHub answers unauthorized reads with 404 rather than 403.
  2. Nothing retried the clone. git_svc.clone() was single-shot, initialize() is called exactly once, and node-level retry policies never apply because sandbox init happens before any node runs.
  3. The failure classified as deterministic. "repository not found" matches nothing in TRANSIENT_INFRA_HINTS, and loop_restart is hard-blocked for every category except TransientInfra — so the one recovery mechanism that could have absorbed this was disabled for it.

Why a post-mint 404 is provably transient

resolve_clone_credentials already fails loudly on every deterministic explanation for a clone 404:

  • installation lookup 404s → "GitHub App is not installed for {owner}"
  • token creation 422s → "GitHub App does not have access to repository {repo}"

A mint failure surfaces as "Failed to get GitHub App credentials for clone", which is not what the failed run emitted. The mint succeeded, which rules out app-not-installed, repo-not-covered, and repo-does-not-exist. Whatever Repository not found means after a successful scoped mint, it is not deterministic.

The fix

New clone_retry module: 3 attempts, 3s then 9s backoff, reusing the same token. Replication of a given token only makes progress, so each attempt strictly improves the odds — re-minting would restart the clock. This matches GitHub Support's own guidance (see references).

The retry condition splits two ways rather than gating everything on credentials:

  • TransientInfra (network, 5xx, rate limit) retries always.
  • TokenReplication (repository not found, authentication failed) retries only when credentials are present. On a public clone, "not found" means the URL names a repo that is not there, so it still fails fast rather than burning 12s of backoff.

The GitHub 404 does not arrive as an HTTP 404 on the Daytona API 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.

Scope: both clone-based providers

The bug report scoped this to Daytona, but the Docker provider had the identical single-shot clone-after-fresh-mint in clone_github_repo, and defaults.toml makes Docker the default runtime provider. Fixing only Daytona would have left most users exposed. Each provider supplies its own classifier; the retry loop and backoff are shared.

Two additions beyond the report

  1. Cleanup between attempts. A clone killed part-way leaves a partial checkout, and the next git clone refuses to write into a non-empty directory. Without this, the retry would convert a retryable failure into a confusing "destination path already exists". Best-effort, retries only.
  2. Category instead of raw error in the retry log. The report asked for the error text per attempt, but docs/internal/logging-strategy.md prohibits raw command stderr in tracing. Each retry logs reason = token_replication, attempt, and delay_ms; the full error still surfaces if attempts run out.

Secondary issues fixed

  • Missing fail_init: the GitHub-URL-parse path in the Daytona clone returned its error without self.fail_init(...), unlike every sibling path, so SandboxEvent::InitializeFailed was never emitted for that case.
  • Misleading classifier hint: classify_exec_failure mapped "repository not found" to "the App installation may not include this repo". After a successful scoped mint that diagnosis is impossible, and it sent operators hunting a configuration problem that did not exist.

Deliberately not changed

Reclassifying "repository not found" as TransientInfra in classify_failure_reason. A blanket mapping would be wrong — without credentials it is genuinely deterministic. The signal that makes it transient is "a mint succeeded immediately before", which only the clone site knows. With retries in place, the classification of the exhausted case matters much less.

Testing

  • 13 new tests, including one built from the exact error string in run 01KYM99DF27JRRW4XSYZBP27K7.
  • Retry-loop tests use #[tokio::test(start_paused = true)], so 12s of backoff costs no wall-clock.
  • cargo nextest run --workspace — 7413 passed.
  • Workspace clippy clean, fmt --check clean, and cargo build -p fabro-sandbox with default features only confirms the new module's feature gating.

Not verified against a live replication lag. The failure is timing-dependent and cannot be reproduced on demand, so the classifier is covered by unit tests against the recorded error string rather than by racing GitHub.

Nothing wraps sandbox.initialize() in a timeout, so the added ~12s worst case cannot blow an init deadline.

References

  • aws-amplify/amplify-hosting#4080 — same pattern. GitHub Support, quoted in the thread: "When you create a token, this needs to be replicated to all of our edge cache sites to be usable on them." Recommended waiting a few seconds and retrying with the same token.
  • akuity/kargo#5610 — same symptom, different cause (their token cache re-served expired tokens). A caution that the symptom alone does not prove the mechanism; Fabro's mint-per-clone path has no cache, so that mode is excluded here.

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 13:04
@brynary

brynary commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

Notes for whoever picks this up

Draft because the core failure mode is not reproducible on demand — it depends on GitHub's internal replication timing. Everything here is reasoned from the event stream of one incident plus GitHub's own statements. Read the argument before trusting the fix.

The load-bearing claim

Everything rests on this: a successful mint rules out every deterministic reason for a clone 404. If that is wrong, this PR retries failures that will never succeed and adds 12s to every one of them.

The argument is in clone_retry.rs's module doc. Worth checking yourself against fabro-github/src/lib.rs — specifically that mint_installation_token_with_jwt really does 404 on installation lookup and 422 on token creation, and that neither can silently pass through to a clone. If you find a fourth way the mint can succeed while access is genuinely absent, this needs rethinking.

Places I made a judgment call you may want to revisit

Backoff of 3s → 9s (12s worst case). From GitHub Support's guidance in the Amplify thread (~3s, then 10s). No measurement behind it — we have exactly one data point, where a retry 46s later worked. If clone failures show up in telemetry still exhausting all three attempts, the window is too short and the right move is more attempts, not longer ones (replication only makes progress).

Retrying authentication failed, not just repository not found. Reasoning: the same lag can surface as 401/403 depending on which endpoint answers first. This one is not backed by the incident — the incident only ever produced repository not found. It is the most speculative hint in TOKEN_REPLICATION_HINTS. If it causes real auth misconfigurations to take 12s to fail, drop it; the 404 case is the one with evidence.

rm -rf between attempts. New destructive command in the init path. The path is <repos_root>/<owner>/<repo> from github_repo_layout, shell-quoted, and parse_github_owner_repo guarantees both segments are non-empty. I convinced myself this is safe, but it is the change I would most want a second pair of eyes on. It follows the existing rm -rf {temp_q} idiom in sandbox_git_runtime.rs:42.

Strictly speaking this cleanup guards a different failure (clone killed mid-flight) than the one we are fixing — for the auth-race case git cleans up after itself. I included it because without it the retry can turn a retryable error into a confusing "destination path already exists", which would make the fix look broken. Reasonable to argue it belongs in its own PR.

What I could not verify

  • No live test of the actual race. The classifier is tested against the recorded error string from run 01KYM99DF27JRRW4XSYZBP27K7; the retry loop is tested with a fake clone. Neither proves the real Daytona toolbox surfaces the message in the shape I matched on. If you can, run the Daytona E2E suite (cargo nextest run -p fabro-workflow --profile e2e --run-ignored only) — I did not, since it needs live credentials.
  • The exact DaytonaError variant carrying the message. classify_clone_failure handles it either way (NotFound and General both fall through to the text match), so this does not change behavior — but it means the structural half of that function is less exercised than it looks.

Deliberately left alone

classify_failure_reason still classifies an exhausted clone as deterministic, so loop_restart will still refuse it. That is intentional: a blanket "repository not found"TransientInfra mapping would be wrong for the no-credentials case, and the signal that makes it transient ("a mint succeeded immediately before") is only visible at the clone site. If we want defense in depth here, the clean version is a typed error from the clone path carrying its own category rather than another string hint — bigger change than this fix warranted.

Reviewing efficiently

Read in this order:

  1. clone_retry.rs module doc — the whole argument.
  2. classify_message + its tests — where a wrong call costs 12s or a missed retry.
  3. The two provider call sites — mechanical, but check that neither re-mints between attempts. That is the property that makes retrying worthwhile, and it is easy to break later by "helpfully" refreshing credentials in the retry loop. Worth a comment or a test if you think it is at risk.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a shared retry loop for the initial repository clone performed during sandbox initialization, addressing transient “repository not found” failures caused by GitHub installation-token replication lag (observed in Daytona, and applicable to Docker as well).

Changes:

  • Introduces a shared clone_retry module that retries clone operations (3 attempts with 3s/9s backoff) based on structured retry reasons (TransientInfra vs TokenReplication).
  • Updates Docker and Daytona clone paths to use the shared retry loop and to clean up partial checkouts between attempts.
  • Improves clone error classification and fixes a Daytona init failure path to emit InitializeFailed via fail_init.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/components/fabro-sandbox/src/lib.rs Wires in the new clone_retry module behind feature/test cfgs.
lib/components/fabro-sandbox/src/error.rs Expands the “repository not found” exec hint to include token replication lag as a possible cause.
lib/components/fabro-sandbox/src/docker.rs Wraps Docker clone execution in the shared retry loop and clears partial clones between attempts.
lib/components/fabro-sandbox/src/daytona/mod.rs Wraps Daytona clone in the shared retry loop, adds retry classification, and fixes missing fail_init on a clone-credential error path.
lib/components/fabro-sandbox/src/clone_retry.rs New shared retry/backoff + message-based classification with unit tests.
Comments suppressed due to low confidence (1)

lib/components/fabro-sandbox/src/docker.rs:761

  • GitCloneFailed now emits err.to_string() and err.causes() directly. If the underlying Docker exec error (or its cause chain) includes the authenticated clone URL, this can leak the freshly minted GitHub installation token into sandbox events/logs. The git stderr path is redacted, but this error path is not.
            self.emit(SandboxEvent::GitCloneFailed {
                url:    origin_url,
                error:  err.to_string(),
                causes: err.causes(),
            });

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings July 28, 2026 20:26
@brynary

brynary commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

Pushed review cleanup in 4727ee8.

Changes:

  • Retry auth and repository-not-found failures only after Fabro mints a fresh GitHub App token. Static PAT and installation-token failures now fail fast.
  • Give Daytona message classification precedence over HTTP status and avoid retrying ambiguous remote timeouts.
  • Run Docker clones through controlled exec and keep all attempts within the original five-minute deadline.
  • Clone first and create the workspace symlink once after success.
  • Remove recursive retry cleanup and validate repository path components to prevent checkout paths from escaping their roots.
  • Reuse the shared credential-kind and symlink helpers, typed provider values, and structured exec errors.

Local verification passed:

  • cargo +nightly-2026-04-14 fmt --check --all
  • cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings
  • cargo build --workspace
  • cargo nextest run --workspace (7,420 passed; 200 skipped)
  • cargo nextest run -p fabro-github (55 passed; 6 skipped)
  • cargo nextest run -p fabro-sandbox --all-features (218 passed; 9 skipped)

@brynary
brynary marked this pull request as ready for review July 28, 2026 20:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

lib/components/fabro-sandbox/src/docker.rs:766

  • retry_clone retries the same git clone command but doesn’t clean layout.primary_repo_path between attempts. If the first attempt creates a partial checkout, the next attempt will likely fail with "destination path ... already exists" (classified as permanent), making the retry loop ineffective for mid-clone transient failures.
        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),

lib/components/fabro-sandbox/src/daytona/mod.rs:1151

  • The Daytona clone retry loop doesn’t appear to clean layout.primary_repo_path between attempts. If an attempt fails after creating a partial checkout, later attempts can fail immediately with a non-empty destination (e.g. "destination path ... already exists"), which would undermine the intended retry behavior for transient mid-clone failures.
                let clone_result = clone_retry::retry_clone(
                    SandboxProviderKind::Daytona,
                    None,
                    |_attempt| {
                        let git_svc = &git_svc;

@brynary
brynary merged commit 9f32e14 into main Jul 28, 2026
14 checks passed
@brynary
brynary deleted the fix/retry-transient-sandbox-clone branch July 28, 2026 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants