Skip to content

fix(server): retry transient sandbox CAS conflicts - #3501

Open
drew wants to merge 1 commit into
mainfrom
codex/fix-flaky-sandbox-mutations
Open

drew wants to merge 1 commit into
mainfrom
codex/fix-flaky-sandbox-mutations

Conversation

@drew

@drew drew commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Prevent transient sandbox resource-version races from aborting server-managed provider attachment and stopped-sandbox startup. This addresses the Docker provider-readiness and rootless Podman lifecycle flakes observed on #3439.

Related Issue

No issue required: this is a localized reliability fix for observed E2E flakes.

Changes

  • Retry bounded server-owned sandbox CAS mutations while preserving fail-fast behavior for explicit client resource versions
  • Retry runtime identity rotation only when the persisted identity is unchanged, preserving unrelated status updates
  • Add deterministic concurrency tests for retry behavior and identity-rotation fencing

Testing

  • cargo test -p openshell-server
  • cargo clippy -p openshell-server --all-targets -- -D warnings
  • mise run pre-commit
  • The previously failing E2E jobs passed on rerun attempt 2

Checklist

  • Tests added for the fixed race conditions
  • Repository pre-commit checks pass
  • Commit is signed off for DCO
  • No documentation changes required; this does not change user-facing behavior or architecture

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew drew added gator:in-review Gator is reviewing or awaiting PR review feedback test:e2e Requires end-to-end coverage labels Sep 20, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for ea707d2. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@drew drew added gator:blocked Gator is blocked by process or repository gates and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Sep 20, 2026
@drew

drew commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

gator-agent

PR Review Status

This localized sandbox reliability fix is project-valid, and the independent full-diff review found no blocking issues. The required E2E workflow has not started with the new label, so pipeline monitoring cannot begin yet.

Action required: A maintainer must open the existing E2E run for this head and choose Re-run all jobs so the test:e2e suite executes with the label set.

Blocking findings:

  • No blocking findings remain

Carried findings:

  • None

Non-blocking suggestions:

  • None
Gator metadata
  • Validation: Maintainer-authored, localized server reliability fix for observed sandbox lifecycle flakes
  • Docs: Not needed because the patch changes internal retry behavior without changing user-facing behavior
  • Checks: Branch Checks, Helm Lint, and Trivy Changes are green on the current head
  • E2E: test:e2e is applied; the label helper requires Re-run all jobs on the existing run before the suite is dispatched
  • Head SHA: ea707d22973f06cb3eec23386ebf3a02836bb16f
  • Base SHA: 29e89a2f2289ad538c195e136baaaac2a92a3a2e
  • Merge base SHA: 29e89a2f2289ad538c195e136baaaac2a92a3a2e
  • Patch ID: 2e63f6a2c343f28901543396718033eb8e6c442c
  • Gator payload: 9
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:blocked
  • Blocked reason: test_dispatch_required

@elezar

elezar commented Sep 21, 2026

Copy link
Copy Markdown
Member

PR #3485 supersedes the stopped-sandbox runtime-identity portion of this change and should land first. After #3485 merges, the expected next steps here are:

  • Rebase this PR onto main.
  • Retain the bounded server-owned CAS retries for provider attach/detach, including update_sandbox_cas and its concurrency coverage.
  • Drop the mint_next_runtime_authentication / persist_next_runtime_identity changes and the tests tied to that split pre-lock identity flow.
  • If useful, port the unrelated-status-update scenario into a test of the compute-side atomic start transition introduced by fix(server): serialize sandbox restart authentication #3485.

The reason is that this PR fences writers racing from the same persisted identity, but it still leaves identity rotation separate from the lifecycle lock and Starting transition. A later start can rotate the identity again in that window. #3485 closes that window by committing the winning identity and Starting phase atomically, then reusing that identity for idempotent driver retries.

@drew drew added gator:in-review Gator is reviewing or awaiting PR review feedback gator:blocked Gator is blocked by process or repository gates and removed gator:blocked Gator is blocked by process or repository gates gator:in-review Gator is reviewing or awaiting PR review feedback labels Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:blocked Gator is blocked by process or repository gates test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants