Skip to content

fix(server): serialize sandbox restart authentication - #3485

Merged
elezar merged 1 commit into
mainfrom
codex/fix-sandbox-start-auth-race/drew
Sep 21, 2026
Merged

elezar merged 1 commit into
mainfrom
codex/fix-sandbox-start-auth-race/drew

Conversation

@drew

@drew drew commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

A stopped sandbox could rotate its runtime identity before acquiring the compute lifecycle locks, allowing reconciliation or another start to invalidate the CAS or launch with credentials that no longer matched persisted state. Commit the identity rotation and Starting transition in one resource-version update, retry conflicts, and make idempotent starts reuse the winning identity.

Related Issue

No issue required: localized concurrency bug confirmed across the reviewed lifecycle failures and requested directly for implementation.

Changes

  • Move restart authentication preparation inside the sandbox lifecycle and global state guards.
  • Atomically persist the new authorization identity with the Starting phase.
  • Retry CAS conflicts and reuse the persisted identity when another start wins.
  • Add a regression test covering the single-write transition, matching launch credentials, and idempotent retry.
  • Document the cross-replica lifecycle invariant.

Testing

  • mise run pre-commit passes
  • mise run test passes
  • Focused authenticated restart regression test passes
  • Unit tests added/updated
  • E2E tests added/updated (not applicable)

Checklist

@drew
drew added this pull request to stack #3486 September 20, 2026 01:19
@drew
drew requested review from a team and derekwaynecarr as code owners September 20, 2026 01:19
@drew
drew force-pushed the codex/update-rustls/drew branch from 48e2473 to 6215d97 Compare September 20, 2026 01:55
@drew
drew force-pushed the codex/fix-sandbox-start-auth-race/drew branch from b403498 to bea0fdf Compare September 20, 2026 01:55
@drew
drew removed this pull request from stack #3486 September 20, 2026 01:58
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew
drew force-pushed the codex/update-rustls/drew branch from 6215d97 to 8353f52 Compare September 20, 2026 01:58
@drew
drew force-pushed the codex/fix-sandbox-start-auth-race/drew branch from bea0fdf to 924f043 Compare September 20, 2026 01:58
@drew
drew changed the base branch from codex/update-rustls/drew to main September 20, 2026 01:58
@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 924f043. 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

The initial code review found no validated blocking findings. The sandbox lifecycle change requires the standard E2E suite, but applying test:e2e did not dispatch it automatically.

Action required: A maintainer must open Branch E2E run 35482747929 and choose Re-run all jobs so the suite executes with test:e2e applied.

Blocking findings:

  • No blocking findings remain

Carried findings:

  • None

Non-blocking suggestions:

  • None
Gator metadata
  • Validation: Project-valid localized concurrency fix authored by a verified repository admin
  • Docs: Fern docs are not needed because this does not change a direct user-facing interface; the lifecycle architecture note was updated
  • Checks: Branch Checks, Helm Lint, and Trivy Changes passed on the current head; the required-gates publisher is refreshing after the label change
  • E2E: test:e2e is applied, but the existing Branch E2E run must be rerun before testing is dispatched
  • Head SHA: 924f04336de15432fe6ef3496048c378f87b4b37
  • Base SHA: d6f3e1f5f2fcb38d8feb5ad90574a057ae805486
  • Merge base SHA: d6f3e1f5f2fcb38d8feb5ad90574a057ae805486
  • Patch ID: 2fac353ec3c84e803f3b27576f4be9c920a7d2d7
  • 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

@drew drew added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed and removed gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status labels Sep 21, 2026

@elezar elezar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed the lifecycle and authentication changes. Moving restart identity preparation under lifecycle serialization and committing the identity with the Starting transition in one CAS closes the stale-credential window; idempotent retries correctly reuse the persisted winning identity. All required checks, including Core E2E, are green. Approved.

@elezar
elezar added this pull request to the merge queue Sep 21, 2026
@drew drew added gator:merge-ready and removed gator:approval-needed Gator completed review; maintainer approval needed labels Sep 21, 2026
Merged via the queue into main with commit 484f076 Sep 21, 2026
128 of 133 checks passed
@elezar
elezar deleted the codex/fix-sandbox-start-auth-race/drew branch September 21, 2026 10:54
@drew

drew commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged.

Final status: the current head passed the required branch, Helm, Trivy, and Core E2E checks, received maintainer approval, and reached gator:merge-ready before merge.

I removed the active gator:* label because there is nothing left for gator to monitor on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants