Skip to content

fix(vm): reclaim cancelled image preparation after worker exit - #4039

Merged
johntmyers merged 4 commits into
fix/3950-vm-workload-identity-021400from
fix/3953-image-staging-cleanup-021400
Oct 3, 2026
Merged

fix(vm): reclaim cancelled image preparation after worker exit#4039
johntmyers merged 4 commits into
fix/3950-vm-workload-identity-021400from
fix/3953-image-staging-cleanup-021400

Conversation

@shiju-nv

@shiju-nv shiju-nv commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

A timeout or cancellation can abort a sandbox's async preparation task while blocking filesystem work continues and temporary files remain. Run image and overlay preparation in an owned worker process. On cancellation, stop its process group and wait for cleanup before reclaiming files. Recover inactive attempts after driver restart.

Related Issue

Fixes #3953, accepted by a maintainer on 2026-09-30. Part of #3955. Depends on the workload-identity change in #4036.

Changes

  • Register each worker before yielding. Stop/delete join the cancelled task, terminate its process group, and wait for owned processes to exit. Inherited file locks protect files until descendants stop; uncertain cleanup retains staging and sandbox state.
  • Allow independent preparation and private overlay workers to progress concurrently. Hold the shared cache lock only for the destination readiness check and atomic rename.
  • Serialize shared cache publication and atomically publish complete disks, including writable-overlay retries. Reclaim marked inactive attempts at startup while preserving active attempts, committed caches and unmarked legacy staging.
  • Carry user/group selectors across the worker boundary and validate them against the persisted overlay owner before writes. Preserve successful worker status when stdout closes before exit, including Darwin's exited-process-group EPERM case.

Testing

  • Focused checks appropriate to the changed code pass. Broad lint and test gates run in CI.
  • Regression tests added and exercised against the faulty behavior.

Local verification passed on the combined tree: formatting, diff and license checks, the VM driver package check, focused library regressions, and the actual-worker integration target. Tests prove a second sandbox progresses while another worker is blocked, cache publication preserves the first valid artifact, worker death releases the lock, and cancellation retains its cleanup guarantees. The two concurrency regressions fail with the original outer locks. This head includes the signed identity correction and durable stop/start E2E fixture from #4036. The inherited E2E crate matches its independently reviewed and compiled parent.

Current signed head: ecf8d1973b79. Hosted verification: Branch Checks and Branch E2E Checks. The test:e2e label is enabled for runtime tests.

Hosted VM E2E runs on Linux. Fresh physical-Mac qualification remains pending.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO). The current head has a verified SSH signature.
  • Architecture docs updated (if applicable)

@copy-pr-bot

copy-pr-bot Bot commented Oct 1, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

🌿 Preview your docs: https://nvidia-preview-pr-4039.docs.buildwithfern.com/openshell

@shiju-nv
shiju-nv changed the base branch from main to fix/3950-vm-workload-identity-021400 October 1, 2026 11:40
@shiju-nv
shiju-nv added this pull request to stack #4041 October 1, 2026 11:51
@shiju-nv
shiju-nv marked this pull request as ready for review October 1, 2026 19:20
@shiju-nv
fix(vm): stop image workers before cleaning staging files
1627591 
Run image preparation in an owned worker process, reserve its process
identity until cleanup completes, and protect staging with leases so
cancellation and recovery cannot race with another preparation attempt.

Signed-off-by: Shiju <shiju@nvidia.com>
@shiju-nv
shiju-nv force-pushed the fix/3953-image-staging-cleanup-021400 branch from e6370a5 to 1627591 Compare October 2, 2026 21:06
@shiju-nv
fix(vm): inherit the supervisor startup call-site correction
2757ae7 
Merge the corrected workload-identity parent into the cleanup branch.
Preserve the owned-worker cleanup changes while repairing inherited
supervisor compilation.

Signed-off-by: Shiju <shiju@nvidia.com>
drew
drew reviewed Oct 3, 2026
View reviewed changes

@drew drew 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

This PR is project-valid because it implements the accepted VM cleanup bug in #3953, but the initial review found one blocking concurrency regression. The worker ownership approach is sound in direction; the global lock boundaries need narrowing before runtime E2E dispatch.

Action required: @shiju-nv, limit shared-cache locking to cache miss/build/publication sections so an unrelated slow preparation cannot block cached sandbox starts or private overlay creation.

Blocking findings:

  • GATOR-2757ae7a-01: Full-lifetime global preparation locks serialize independent sandbox preparation.

Carried findings:

  • None

Non-blocking suggestions:

  • None
Gator metadata
  • Validation: Implements accepted issue #3953 and is authored by a verified repository writer.
  • Docs: No Fern update required; this changes internal VM preparation and the crate README documents the operational behavior.
  • Checks: Current branch checks are green; required VM runtime E2E has not yet been dispatched.
  • E2E: test:e2e will be required after review feedback is resolved.
  • Head SHA: 2757ae7a35cdf143bb7d96a2e77f7b526020c605
  • Base SHA: 042bc23704c66cd8ed527ee50c509da3bdb6c531
  • Merge base SHA: 042bc23704c66cd8ed527ee50c509da3bdb6c531
  • Patch ID: ce7c4c19fdc4d3e64a2a7c721df048d80ea54d12
  • Gator payload: 10
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:in-review

Comment thread crates/openshell-driver-vm/src/driver.rs Outdated
@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:in-review Gator is reviewing or awaiting PR review feedback labels Oct 3, 2026
@shiju-nv
fix(vm): limit preparation locking to cache publication
6ed7c1a 
Allow independent image and private overlay workers to progress concurrently.
Serialize only destination checks and atomic cache publication, preserving
worker cancellation ownership. Integrate inactive identity restoration.

Signed-off-by: Shiju <shiju@nvidia.com>
@shiju-nv shiju-nv added the test:e2e Requires end-to-end coverage label Oct 3, 2026
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

Label test:e2e applied for 2757ae7. 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 reviewed Oct 3, 2026
View reviewed changes

@drew drew 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 latest update resolves the prior concurrency blocker by removing the full-worker global locks, limiting serialization to shared-cache publication, and covering independent overlay progress with a regression test. No blocking findings remain, and the required runtime E2E workflow is running.

Blocking findings:

  • No blocking findings remain

Carried findings:

  • GATOR-2757ae7a-01: resolved on this head; the Gator-owned review thread has been closed
Gator metadata
  • Validation: Implements accepted VM cleanup issue #3953 and is authored by a verified repository writer.
  • Docs: No Fern update required; this changes internal VM preparation behavior and updates the VM driver README.
  • Checks: Branch checks, Helm lint, and Trivy are green; required E2E is in progress.
  • E2E: test:e2e is applied and the current-head workflow is running.
  • Head SHA: 6ed7c1a9c60c2bf76c1d56dc2236c55a2bed7cf3
  • Base SHA: fd1a801cfe025bb41c3a493797808baf674cc59e
  • Merge base SHA: fd1a801cfe025bb41c3a493797808baf674cc59e
  • Patch ID: 36de8c962d9c43091542f076f0e17540fa074250
  • Gator payload: 10
  • Review mode: follow_up
  • Previous reviewed SHA: 2757ae7a35cdf143bb7d96a2e77f7b526020c605
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@drew drew added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:blocked Gator is blocked by process or repository gates labels Oct 3, 2026
@shiju-nv
test(vm): inherit durable identity restart fixture
ecf8d19 
Integrate the identity parent correction while preserving cleanup behavior.

Signed-off-by: Shiju <shiju@nvidia.com>
@drew

drew commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

gator-agent

Maintainer Approval Needed

Gator validation and PR monitoring are complete. The current head is patch-equivalent to the reviewed head, the prior concurrency finding remains resolved, and all required checks are green.

Review: No blocking findings remain. The required VM and external-driver E2E jobs passed.

Human maintainer approval is now required.

Gator metadata
  • Validation: Implements accepted VM cleanup issue Reclaim inactive image staging after failed or cancelled MicroVM creation #3953.
  • Docs: No Fern update required; this changes internal VM preparation behavior and updates the VM driver README.
  • Checks: Branch Checks, Helm Lint, Trivy Changes, and required E2E are green.
  • E2E: test:e2e is applied; VM managed and external-driver E2E passed.
  • Head SHA: ecf8d1973b79eb0dcb8c243d8ddadc525ebb4f4f
  • Patch ID: 36de8c962d9c43091542f076f0e17540fa074250
  • Gator payload: 10
  • Review mode: already_reviewed (rebase-equivalent)
  • Next state: gator:approval-needed

@drew drew added gator:approval-needed Gator completed review; maintainer approval needed and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Oct 3, 2026
@johntmyers
johntmyers added this pull request to the merge queue Oct 3, 2026
Merged via the queue into main with commit 4188eab Oct 3, 2026
179 of 182 checks passed
@johntmyers
johntmyers deleted the fix/3953-image-staging-cleanup-021400 branch October 3, 2026 20:14
@drew

drew commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged.

Final status: The reviewed implementation had no remaining blocking findings, required checks and VM E2E passed, and maintainer approval was present before merge.

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

Gator metadata
  • Head SHA: ecf8d1973b79eb0dcb8c243d8ddadc525ebb4f4f
  • Gator payload: 10

@drew drew removed the gator:approval-needed Gator completed review; maintainer approval needed label Oct 3, 2026
@rhods-devops-app rhods-devops-app Bot mentioned this pull request Oct 4, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drew drew drew left review comments
@johntmyers johntmyers johntmyers approved these changes
@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner
@sjenning sjenning Awaiting requested review from sjenning sjenning is a code owner
@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr derekwaynecarr is a code owner

Assignees

No one assigned

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reclaim inactive image staging after failed or cancelled MicroVM creation

3 participants

@shiju-nv @drew @johntmyers