Skip to content

fix(driver-docker): scope pending sandbox matching by id and workspace - #3240

Open
letv1nnn wants to merge 1 commit into
NVIDIA:mainfrom
letv1nnn:3234-deleting-same-named-sandbox/letv1nnn
Open

fix(driver-docker): scope pending sandbox matching by id and workspace#3240
letv1nnn wants to merge 1 commit into
NVIDIA:mainfrom
letv1nnn:3234-deleting-same-named-sandbox/letv1nnn

Conversation

@letv1nnn

@letv1nnn letv1nnn commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Pending-sandbox lookups in the Docker compute driver matched on sandbox_id OR sandbox_name. Sandbox names are unique per workspace, not globally, so a lifecycle request carrying a correct id could resolve to a different sandbox that happened to share a name in another workspace — evicting its pending record and aborting its in-flight provisioning task. This makes sandbox_id authoritative, rejects ambiguous name-only lookups instead of letting HashMap iteration order pick a victim, and scopes the reserve-time name-conflict check by workspace.

Related Issue

Fixes #3234

Changes

  • Replace pending_sandbox_matches with resolve_pending_id, which resolves a lifecycle request to at most one pending sandbox id:
    • sandbox_id is authoritative — when supplied, the name is never consulted as an alternative. Previously an id that was absent from the pending map fell through to a name match.
    • The name fallback now applies only when no id is supplied, and returns FailedPrecondition when more than one pending sandbox shares the name. The driver request carries no workspace, so the match is genuinely ambiguous and the previous behavior depended on HashMap iteration order.
    • The id path is a keyed lookup rather than a linear scan; the map is already keyed by sandbox.id.
  • Thread the resulting Result through pending_snapshot, remove_pending_sandbox, and their callers in get_sandbox_snapshot, delete_sandbox_inner, and stop_sandbox_inner.
  • Scope the reserve_pending_sandbox name-conflict check by workspace. Creating a sandbox named demo in workspace beta while a demo in workspace alpha was still provisioning previously failed with AlreadyExists. Duplicate ids are still rejected unconditionally. This uses DriverSandbox.workspace, which the driver already receives — no proto change.

Behavior change

An ambiguous name-only DeleteSandbox/StopSandbox/GetSandbox now returns FailedPrecondition rather than acting on an arbitrary match. The gateway always sends a non-empty sandbox_id (crates/openshell-server/src/compute/mod.rs), so this path is reachable only by direct driver RPCs.

Out of scope

  • Adding workspace to the driver-facing DeleteSandboxRequest/StopSandboxRequest/StartSandboxRequest. It would let the name fallback disambiguate instead of erroring, but it touches five driver crates and the driver proto contract, so it belongs in its own change. The public DeleteSandboxRequest in proto/openshell.proto already carries workspace; the driver-facing message in proto/compute_driver.proto does not.
  • crates/openshell-driver-vm (driver.rs:1391, :1471, :1536, :1617) and crates/openshell-driver-mxc (driver.rs:345) resolve in-memory records by name with the same weakness. Podman and Kubernetes are unaffected — they resolve against platform labels. I have reviewed the matching shape in those drivers but not traced their full call paths; filing separately rather than expanding this PR.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

The complete Docker-driver patch is project-valid and the initial code review found no blocking issues. The authoritative-id lookup, ambiguous name-only rejection, workspace-scoped reservation, and regression coverage form a proportionate fix for the linked bug.

Blocking findings:

  • No blocking findings remain

Carried findings:

  • None

Non-blocking suggestions:

  • None
Gator metadata
  • Validation: Localized Docker-driver correctness fix linked to issue #3234 with a concrete user path and regression coverage
  • Docs: Not needed because this corrects internal driver identity matching without changing a supported user-facing workflow
  • Checks: DCO and vouch pass; current-head branch checks await authorized test dispatch
  • E2E: Required for Docker sandbox lifecycle behavior; dispatch pending
  • Head SHA: 6e63b1a0060249c30bb85935dd622509367e05fb
  • Base SHA: 8af79a7f4b68abf09299371f20987fde90667139
  • Merge base SHA: 118b250f015af84ce4a225cd45d1ce7b64bb54cb
  • Patch ID: ce6704dce29ac63e7974a5d7d3e31b9ecd874c35
  • Gator payload: 8
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:in-review

@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback test:e2e Requires end-to-end coverage labels Sep 9, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 6e63b1a

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Label test:e2e applied for 6e63b1a. Open Branch E2E Checks, find the run for commit 6e63b1a, 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 and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed labels Sep 9, 2026
@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates gator:approval-needed Gator completed review; maintainer approval needed and removed gator:blocked Gator is blocked by process or repository gates gator:approval-needed Gator completed review; maintainer approval needed labels Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:approval-needed Gator completed review; maintainer approval needed test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(driver-docker): deleting a sandbox can destroy a same-named sandbox in another workspace

2 participants