fix(daemon): retry deps init with backoff to recover from transient failures - #1184
Open
nicacioliveira wants to merge 2 commits into
Open
fix(daemon): retry deps init with backoff to recover from transient failures#1184nicacioliveira wants to merge 2 commits into
nicacioliveira wants to merge 2 commits into
Conversation
…ailures
The deps init middleware caches its first-attempt Promise in `ok` and
serves the same rejection forever once it fails:
const ensureStarted = () => { ok ||= start(); };
In production this turns a single transient failure (GitHub App auth
race during cold boot, ephemeral network hiccup, GitHub API flake) into
a sticky HTTP 424 "Error while starting global deps" that only clears
when the pod is recreated. We've seen it on freshly-warmed sandboxes
where one of the four init steps fails on the first try, then the
exact same operation (with the exact same env) succeeds on every later
attempt.
This change:
1. Wraps `runInitSteps` (the renamed `start`) in `runInitWithRetry`
with exponential-ish backoff (500ms, 1.5s, 5s, 15s, 30s — 6
attempts, ~52s total). The first user request after deploy waits
through retries internally and usually returns 200 within ~500ms
of the first failure.
2. Adds a 60s cooldown after a fully-exhausted retry sequence. The
next request after the cooldown clears `ok` and runs init fresh.
Long-lived transient external outages eventually self-heal without
requiring a pod recreation.
3. Moves `readyResolve()` out of a try/catch so it's only called once
per successful attempt. `readyReject()` is only called after every
retry has failed, preserving the original semantic that `ready`
reflects the first-success / final-failure outcome.
Total retry window is fast enough to recover within a single user
request and slow enough not to hammer GitHub when a permanent failure
is the issue (wrong repo URL, missing GitHub App install, etc.).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 Walkthrough🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@daemon/main.ts`:
- Around line 360-377: The retry loop in runInitWithRetry ignores the site app
abort signal and uses uncancellable sleeps, which can cause init steps to run
after createSiteApp.dispose() has torn down the workspace; update
runInitWithRetry to (1) check the abort signal (ac.signal or the signal passed
to createSiteApp) and immediately return/bail before each iteration and before
calling runInitSteps, and (2) replace the simple setTimeout-based backoff with
an abort-aware delay (e.g., a Promise that races the timeout against
signal.aborted / uses the signal's event listener or AbortSignal.timeout) so the
wait is cancelled when the site is disposed, referencing runInitWithRetry,
RETRY_DELAYS_MS, runInitSteps, and createSiteApp.dispose()/ac.signal to locate
the relevant code.
- Around line 380-403: The current logic permanently rejects the shared deferred
"ready" via readyReject(lastErr) so downstream awaiters never recover; instead,
when you reset after exhaustion (the branch in ensureStarted that checks ok &&
exhaustedAt && Date.now() - exhaustedAt > RESET_AFTER_EXHAUSTION_MS), recreate
the deferred (assign a fresh deps.ready and new readyResolve/readyReject) so
future successful runInitWithRetry/ensureGit can call readyResolve and recover;
locate symbols ensureStarted, ok, exhaustedAt, RESET_AFTER_EXHAUSTION_MS,
runInitWithRetry, ready/readyResolve/readyReject and implement reinitializing
the deferred there (and ensure readyReject(lastErr) is only used for
unrecoverable final failure paths).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="daemon/main.ts">
<violation number="1" location="daemon/main.ts:381">
P1: Rejecting `ready` after retry exhaustion makes recovery one-shot for `gitReady` consumers; later cooldown retries can succeed, but worker/bootstrap paths that awaited `deps.ready` remain permanently failed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
hugo-ccabral
approved these changes
May 5, 2026
Address CodeRabbit + cubic review:
1. **Abort-aware retries** — `runInitWithRetry` now checks `signal.aborted`
before each attempt and sleeps via a new `abortableSleep` helper that
races the timer with `signal.addEventListener("abort")`. Without this,
a `dispose()` call (e.g. sandbox undeploy) during a 30s backoff would
wake up later and run `ensureGit` / manifest generation against a
torn-down sandbox.
2. **Don't permanently reject `ready`** — removed `readyReject(lastErr)`
and the `readyReject` deferred. Promises settle once: rejecting `ready`
on the first exhausted retry sequence permanently strands consumers
(e.g. `createWorker(makeWorkerOptionsFactory(runCmdFactory, deps.ready))`,
`currentSite.gitReady.then(...)`) even when a later cooldown-driven
retry succeeds. Now `ready` stays pending until the first successful
`ensureGit`, and the cooldown path can finally resolve it from a
subsequent attempt.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
daemon/main.ts (1)
356-362: Consider adding jitter to retry delays to reduce herd effects.Line [361] uses fixed delays, which can synchronize retries across pods during shared outages. A small random jitter per delay helps smooth external load without changing behavior materially.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@daemon/main.ts` around lines 356 - 362, RETRY_DELAYS_MS currently contains fixed backoff intervals which can cause synchronized retries; modify the retry loop that consumes RETRY_DELAYS_MS to apply a small random jitter per attempt (e.g., +/- a percentage or a random value up to a max jitter) by computing an actualDelay = baseDelay ± jitter using Math.random() before awaiting/sleeping, and use that actualDelay instead of the raw RETRY_DELAYS_MS value; keep RETRY_DELAYS_MS as the base schedule but ensure the function that performs sleep (the retry loop surrounding RETRY_DELAYS_MS) applies the jitter so each pod gets slightly different wait times.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@daemon/main.ts`:
- Around line 356-362: RETRY_DELAYS_MS currently contains fixed backoff
intervals which can cause synchronized retries; modify the retry loop that
consumes RETRY_DELAYS_MS to apply a small random jitter per attempt (e.g., +/- a
percentage or a random value up to a max jitter) by computing an actualDelay =
baseDelay ± jitter using Math.random() before awaiting/sleeping, and use that
actualDelay instead of the raw RETRY_DELAYS_MS value; keep RETRY_DELAYS_MS as
the base schedule but ensure the function that performs sleep (the retry loop
surrounding RETRY_DELAYS_MS) applies the jitter so each pod gets slightly
different wait times.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
The deps init middleware (daemon/main.ts:355) caches its first-attempt Promise in `ok` and serves the same rejection forever once it fails:
```ts
const ensureStarted = () => { ok ||= start(); };
```
A single transient failure (GitHub App auth race during cold boot, ephemeral network hiccup, GitHub API flake) becomes a sticky HTTP 424 "Error while starting global deps" until the pod is recreated.
Real case observed: On a freshly-warmed sandbox in eks-envs, the first init attempt failed with:
```
fatal: repository 'https://github.com/deco-sites/farmrio.git/' not found
```
The repo IS private, the App IS installed, the env vars ARE set — and a sibling pod with the same image and env (`deco-sandbox-pool-4sxpm`) cloned the same URL successfully ~30min later. Most likely cause: race between when the sandbox handler accepts `/sandbox/deploy` and when `githubApp.ts` finishes computing `GITHUB_APP_CONFIGURED` from the env. Either way, retrying the exact same operation always works.
What this changes
`runInitSteps` (renamed from inner `start`) — unchanged, runs the existing 4 init steps.
`runInitWithRetry` (new) — wraps it with backoff:
`ensureStarted` — adds a 60s cooldown after exhaustion. The next request after the cooldown clears `ok` and runs init fresh. Long-lived transient external outages eventually self-heal without a pod recreation.
`readyResolve()` moved out of the inner try/catch. It's now called inside `runInitSteps` after `ensureGit` succeeds; multiple successful retries are no-ops because the Promise is already settled. `readyReject()` only fires after every retry has failed — preserves the original semantic.
Behavior
The retry only happens inside the lazy init flow triggered by the first request after `/sandbox/deploy`. Once `ok` is resolved, every subsequent request is the existing fast path.
Test plan
🤖 Generated with Claude Code
Summary by cubic
Adds abort-aware backoff retries to daemon deps init to recover from transient failures and avoid sticky 424s. Also adds a 60s cooldown so long outages self-heal without recreating the pod; the happy path is unchanged.
runInitWithRetrywith backoff[500, 1500, 5000, 15000, 30000]ms (6 attempts, ~52s) and abort-aware sleep; logs each attempt.readyresolve-only: callreadyResolveafterensureGitsucceeds; never rejectready, so downstream consumers can recover after the cooldown.ensureStartedto clear cachedokafter 60s when retries were exhausted, then re-run init on the next request. RenamesstarttorunInitSteps(no behavior change).Written for commit 29a64b1. Summary will update on new commits.
Summary by CodeRabbit