feat: add sandbox creation failure tolerance - #1325
Conversation
Sandbox creation can fail due to transient infrastructure issues like slow image pulls exceeding the ready timeout. Add multi-layer failure tolerance to prevent single transient failures from killing agent runs. - Pre-pull sandbox image in action.yml before fullsend run - Add CreateWithRetry with exponential backoff (3 attempts, 5-15s) - Increase default readyTimeout from 60s to 120s - Add sandbox_timeout_seconds harness config field - Add FULLSEND_SANDBOX_READY_TIMEOUT env var override Signed-off-by: Wayne Sun <gsun@redhat.com>
Site previewPreview: https://fd4047ae-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsLow
Previous runReviewFindingsLow
Info
Previous run (2)ReviewFindingsLow
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| RunnerEnv map[string]string `yaml:"runner_env,omitempty"` | ||
| TimeoutMinutes int `yaml:"timeout_minutes,omitempty"` | ||
| Security *SecurityConfig `yaml:"security,omitempty"` | ||
| TimeoutMinutes int `yaml:"timeout_minutes,omitempty"` |
There was a problem hiding this comment.
[low] style
The new SandboxTimeoutSeconds field introduces column-aligned struct tags, changing whitespace on existing TimeoutMinutes and Security lines. The rest of the struct uses single-space separation.
Suggested fix: Use single-space separation between field type and tag, consistent with the rest of the struct.
fullsend-ai-review
Bot
added
the
ready-for-merge
- Guard maxAttempts < 1 in CreateWithRetry (returns error instead of nil) - Cap effectiveReadyTimeout at 600s to prevent extreme values - Add upper bound (600) on sandbox_timeout_seconds harness validation - Use exec.CommandContext for readiness poll to respect timeout context - Add timeout 300s and -- separator to podman pull in action.yml - Wrap final error with attempt count for better diagnostics - Include error reason in retry stderr message - Log Delete failures between retries instead of silencing - Use exported DefaultMaxCreateAttempts constant in run.go - Replace magic 10s context buffer with named readyCtxBuffer constant - Remove unused createTimeout constant - Cap backoff shift at 30 to prevent overflow Signed-off-by: Wayne Sun <gsun@redhat.com>
| // env var > package default. | ||
| func effectiveReadyTimeout(override time.Duration) time.Duration { | ||
| t := readyTimeout | ||
| if override > 0 { |
There was a problem hiding this comment.
[low] correctness
The backward-compatible Create() wrapper now silently retries 3 times via CreateWithRetry, changing observable behavior (longer wall-clock time on failure, sandbox deletion side effects between attempts) for any future caller who expects single-attempt semantics.
Suggested fix: Update the Create() godoc to explicitly note that it retries up to DefaultMaxCreateAttempts times with exponential backoff.
| if h.TimeoutMinutes < 0 { | ||
| return fmt.Errorf("timeout_minutes must be non-negative, got %d", h.TimeoutMinutes) | ||
| } | ||
| if h.SandboxTimeoutSeconds < 0 || h.SandboxTimeoutSeconds > 600 { |
There was a problem hiding this comment.
[info] correctness
sandbox_timeout_seconds validation accepts values as low as 1, which would be too short for any practical sandbox creation. Consider adding a minimum (e.g., 30s) or documenting that very low values will cause retries and eventual failure.
fullsend-ai-review
Bot
added
ready-for-merge
- Enforce minimum sandbox_timeout_seconds of 30 (values 1-29 are too short for any practical sandbox creation) - Fix struct tag alignment to use consistent single-space separation - Update Create() godoc to document retry and cleanup behavior Signed-off-by: Wayne Sun <gsun@redhat.com>
fullsend-ai-review
Bot
added
ready-for-merge
1 participant
Summary
action.ymlbeforefullsend run— moves image pull time out of the sandbox ready timeout windowCreateWithRetrywith exponential backoff (3 attempts, 5-15s delay) and cleanup between attempts — a single transient failure no longer kills the runreadyTimeoutfrom 60s to 120s — gives cold image pulls more headroomsandbox_timeout_secondsharness config field — per-agent tuning without env varsFULLSEND_SANDBOX_READY_TIMEOUTenv var override — operator tuning without code changesFixes #1298
Test plan
effectiveReadyTimeoutunit tests: default, override, env var, precedence, invalid/negative env, capped at maxCreateWithRetryerror path tests (openshell not in PATH, zero/negative maxAttempts)SandboxTimeoutSecondsharness validation tests: negative rejected, >600 rejected, zero/positive/600 acceptedSandboxTimeoutSecondsYAML load round-trip testmake go-testpassesmake go-vetpassesmake lintpasses