Skip to content

fix(docker): reclaim sandbox token files on out-of-band removal - #3220

Open
letv1nnn wants to merge 1 commit into
NVIDIA:mainfrom
letv1nnn:3041-token-file-not-cleaned-up/letv1nnn
Open

fix(docker): reclaim sandbox token files on out-of-band removal#3220
letv1nnn wants to merge 1 commit into
NVIDIA:mainfrom
letv1nnn:3041-token-file-not-cleaned-up/letv1nnn

Conversation

@letv1nnn

@letv1nnn letv1nnn commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

delete_sandbox_inner skipped per-sandbox token file cleanup on one exit path — Docker reports no container and no in-memory pending record survives — leaving the sandbox's gateway JWT on disk indefinitely. Every other exit path in that function already cleans up. This adds the missing call plus regression coverage.

Related Issue

Fixes #3041

Changes

  • crates/openshell-driver-docker/src/lib.rs: call cleanup_sandbox_token_file_for_delete in the "container gone, no pending record" branch of delete_sandbox_inner.
    • Uses _for_delete rather than _by_id because delete accepts a name with no id (require_sandbox_identifier requires only one of the two). With an empty id, sandbox_token_path resolves to <namespace>/sandbox.jwt, whose parent is the shared namespace directory. _for_delete guards on !sandbox_id.is_empty().
    • Keeps the Ok(false) return: nothing was removed from Docker, so no deletion is claimed and no Deleted watch event fires.
  • crates/openshell-driver-docker/src/tests.rs: two regression tests, plus a loopback stub that answers Docker's GET /containers/json with [] so the branch is reachable without a daemon.
  • crates/openshell-driver-docker/Cargo.toml / Cargo.lock: enable temp-env's async_closure feature. The sync with_vars cannot wrap an async test body, and the tests need XDG_STATE_HOME scoped to a tempdir.

Left alone deliberately: the adjacent summary_container_targetNone branch returns Ok(pending.is_some()) without removing the container. That one is a live container being silently abandoned, not a token leak — adding cleanup there would revoke a running sandbox's credential. Worth its own issue; stop_sandbox_inner handles the identical case with Err(Status::not_found).

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 8, 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.

@letv1nnn
letv1nnn marked this pull request as ready for review September 8, 2026 16:51
@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
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/3220 does not exist yet. A maintainer needs to comment /ok to test b0118ac2927bd0dc459a417318febb41e1dad8fa to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test b0118ac

@johntmyers johntmyers 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 9, 2026
@johntmyers

johntmyers commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

This focused Docker lifecycle fix is project-valid through linked bug #3041. The initial independent code review found no blocking issues, and no Fern docs update is needed because the change restores internal token-file cleanup without changing a published user interface.

Action required: A maintainer must dispatch Trivy Changes for base 2ad86c1b2e3e81a26e0fd38ec860398acb31f2fc and head b0118ac2927bd0dc459a417318febb41e1dad8fa. This required workflow was added to main after the PR head diverged, so no current-head Trivy run or required status exists.

Blocking findings:

  • No blocking code findings remain

Carried findings:

  • None

Non-blocking suggestions:

  • None
Gator metadata
  • Validation: Linked bug bug(driver-docker): token file not cleaned up when container is already gone with no pending record #3041 provides a concrete Docker-driver reproduction and acceptance criteria, and this four-file patch is narrowly scoped to that defect and regression coverage.
  • Docs: Not needed; this is an internal cleanup correctness fix with no direct CLI, API, configuration, policy, or workflow UX change.
  • Checks: DCO and vouch are green. The current-head mirror exists; Branch Checks and E2E are running, Helm Lint is green after change detection, and a current-head Trivy Changes run must be manually dispatched.
  • E2E: test:e2e applied; /ok to test b0118ac2927bd0dc459a417318febb41e1dad8fa posted; mirror head verified; Branch E2E Checks run 34397594779 queued.
  • Head SHA: b0118ac2927bd0dc459a417318febb41e1dad8fa
  • Base SHA: 2ad86c1b2e3e81a26e0fd38ec860398acb31f2fc
  • Merge base SHA: 320d4ef79dd572c642133f175f12bafc20d89fd9
  • Patch ID: 75d5c6386c9d5657a4292aa914694a9de61e0d9a
  • Gator payload: 8
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:blocked
  • Blocked reason: trivy_workflow_dispatch_required

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:blocked Gator is blocked by process or repository gates and removed gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status labels Sep 9, 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.

bug(driver-docker): token file not cleaned up when container is already gone with no pending record

2 participants