Repository navigation
feat: add sandbox creation failure tolerance #1325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,8 +203,9 @@ type Harness struct { | |
| AgentInput string `yaml:"agent_input,omitempty"` | ||
| ValidationLoop *ValidationLoop `yaml:"validation_loop,omitempty"` | ||
| 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"` | ||
| SandboxTimeoutSeconds int `yaml:"sandbox_timeout_seconds,omitempty"` | ||
| Security *SecurityConfig `yaml:"security,omitempty"` | ||
| } | ||
|
|
||
| // Load reads a harness YAML file from path, unmarshals it, and validates it. | ||
|
|
@@ -248,6 +249,9 @@ func (h *Harness) Validate() error { | |
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| return fmt.Errorf("sandbox_timeout_seconds must be between 0 and 600, got %d", h.SandboxTimeoutSeconds) | ||
| } | ||
| for i, hf := range h.HostFiles { | ||
| if hf.Src == "" { | ||
| return fmt.Errorf("host_files[%d]: src is required", i) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,10 +18,15 @@ const ( | |
| // SandboxClaudeConfig is the Claude config directory inside the sandbox. | ||
| SandboxClaudeConfig = "/tmp/claude-config" //nolint:gosec // not a credential | ||
|
|
||
| createTimeout = 65 * time.Second | ||
| readyTimeout = 60 * time.Second | ||
| readyTimeout = 120 * time.Second | ||
| readyPoll = 2 * time.Second | ||
| readyCtxBuffer = 10 * time.Second | ||
| maxReadyTimeout = 600 * time.Second | ||
| transferTimeout = 5 * time.Minute | ||
|
|
||
| DefaultMaxCreateAttempts = 3 | ||
| retryInitialBackoff = 5 * time.Second | ||
| retryMaxBackoff = 15 * time.Second | ||
| ) | ||
|
|
||
| func sanitizeDownload(localDir string) error { | ||
|
|
@@ -149,12 +154,74 @@ func CheckGateway() error { | |
| return nil | ||
| } | ||
|
|
||
| // Create creates a persistent OpenShell sandbox and waits for it to be ready. | ||
| // If providers are given, they are passed as --provider flags. If image is | ||
| // non-empty, it is passed as --from to start the sandbox from a container image. | ||
| // If policy is non-empty, it is applied at creation time via --policy. | ||
| // effectiveReadyTimeout returns the sandbox ready timeout to use. Priority: | ||
| // explicit override (from harness config) > FULLSEND_SANDBOX_READY_TIMEOUT | ||
| // env var > package default. | ||
| func effectiveReadyTimeout(override time.Duration) time.Duration { | ||
| t := readyTimeout | ||
| if override > 0 { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| t = override | ||
| } else if envVal := os.Getenv("FULLSEND_SANDBOX_READY_TIMEOUT"); envVal != "" { | ||
| if d, err := time.ParseDuration(envVal); err == nil && d > 0 { | ||
| t = d | ||
| } | ||
| } | ||
| if t > maxReadyTimeout { | ||
| t = maxReadyTimeout | ||
| } | ||
| return t | ||
| } | ||
|
|
||
| // Create creates a persistent OpenShell sandbox and waits for it to be ready, | ||
| // retrying up to defaultMaxCreateAttempts times with exponential backoff. | ||
| func Create(name string, providers []string, image, policy string) error { | ||
| ctx, cancel := context.WithTimeout(context.Background(), createTimeout) | ||
| return CreateWithRetry(name, providers, image, policy, DefaultMaxCreateAttempts, 0) | ||
| } | ||
|
|
||
| // CreateWithRetry creates a sandbox, retrying up to maxAttempts times with | ||
| // exponential backoff on failure. Between attempts the failed sandbox is | ||
| // deleted to avoid name conflicts. If readyTimeoutOverride is positive, it | ||
| // overrides the default ready timeout. | ||
| func CreateWithRetry(name string, providers []string, image, policy string, maxAttempts int, readyTimeoutOverride time.Duration) error { | ||
| if maxAttempts < 1 { | ||
| return fmt.Errorf("maxAttempts must be >= 1, got %d", maxAttempts) | ||
| } | ||
|
|
||
| timeout := effectiveReadyTimeout(readyTimeoutOverride) | ||
|
|
||
| var lastErr error | ||
| for attempt := 1; attempt <= maxAttempts; attempt++ { | ||
| lastErr = createOnce(name, providers, image, policy, timeout) | ||
| if lastErr == nil { | ||
| return nil | ||
| } | ||
|
|
||
| if delErr := Delete(name); delErr != nil { | ||
| fmt.Fprintf(os.Stderr, " Warning: cleanup of sandbox %s failed: %v\n", name, delErr) | ||
| } | ||
|
|
||
| if attempt < maxAttempts { | ||
| shift := uint(attempt - 1) | ||
| if shift > 30 { | ||
| shift = 30 | ||
| } | ||
| backoff := retryInitialBackoff * time.Duration(1<<shift) | ||
| if backoff > retryMaxBackoff { | ||
| backoff = retryMaxBackoff | ||
| } | ||
| fmt.Fprintf(os.Stderr, " Sandbox creation attempt %d/%d failed (%v), retrying in %s...\n", attempt, maxAttempts, lastErr, backoff) | ||
| time.Sleep(backoff) | ||
| } | ||
| } | ||
| return fmt.Errorf("sandbox creation failed after %d attempts: %w", maxAttempts, lastErr) | ||
| } | ||
|
|
||
| // createOnce creates a persistent OpenShell sandbox and waits for it to be | ||
| // ready. If providers are given, they are passed as --provider flags. If image | ||
| // is non-empty, it is passed as --from to start the sandbox from a container | ||
| // image. If policy is non-empty, it is applied at creation time via --policy. | ||
| func createOnce(name string, providers []string, image, policy string, timeout time.Duration) error { | ||
| ctx, cancel := context.WithTimeout(context.Background(), timeout+readyCtxBuffer) | ||
| defer cancel() | ||
|
|
||
| args := []string{ | ||
|
|
@@ -182,17 +249,17 @@ func Create(name string, providers []string, image, policy string) error { | |
| out, err := cmd.CombinedOutput() | ||
|
|
||
| if err != nil { | ||
| check := exec.Command("openshell", "sandbox", "get", name) | ||
| check := exec.CommandContext(ctx, "openshell", "sandbox", "get", name) | ||
| if checkErr := check.Run(); checkErr != nil { | ||
| return fmt.Errorf("sandbox create failed: %s", string(out)) | ||
| } | ||
| } | ||
|
|
||
| // Wait for sandbox to be fully ready (image pull can take a while). | ||
| deadline := time.Now().Add(readyTimeout) | ||
| deadline := time.Now().Add(timeout) | ||
| var lastOutput, lastStderr string | ||
| for time.Now().Before(deadline) { | ||
| check := exec.Command("openshell", "sandbox", "get", name) | ||
| check := exec.CommandContext(ctx, "openshell", "sandbox", "get", name) | ||
| var stdoutBuf, stderrBuf strings.Builder | ||
| check.Stdout = &stdoutBuf | ||
| check.Stderr = &stderrBuf | ||
|
|
@@ -212,7 +279,7 @@ func Create(name string, providers []string, image, policy string) error { | |
| containerLogs := collectPodmanLogs(name) | ||
|
|
||
| return fmt.Errorf("sandbox %q not ready after %s\nstdout: %s\nstderr: %s\nsupervisor logs: %s\ngateway logs: %s\ncontainer logs: %s", | ||
| name, readyTimeout, lastOutput, lastStderr, supervisorLogs, gatewayLogs, containerLogs) | ||
| name, timeout, lastOutput, lastStderr, supervisorLogs, gatewayLogs, containerLogs) | ||
| } | ||
|
|
||
| // Delete deletes a sandbox, returning any error for the caller to log. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.