Skip to content

fix(vmm): offload recursive directory deletion - #1456

Draft
Leechael wants to merge 2 commits into
nextfrom
fix/vmm-offload-directory-deletion
Draft

Leechael wants to merge 2 commits into
nextfrom
fix/vmm-offload-directory-deletion

Conversation

@Leechael

@Leechael Leechael commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Why

RemoveVm returns quickly, but its background task calls synchronous filesystem deletion on a Tokio worker. tokio::spawn does not move that work off the async executor. Unlinking a fully allocated qcow2 disk can spend seconds freeing filesystem extents.

Production sampling caught a Rocket worker spending about 3.7 seconds in kernel CPU during one removal. Waiting Status requests were dispatched immediately after the workdir deletion finished. The same timing pattern appeared across multiple removals.

Change

Run workdir deletion in spawn_blocking, after the VM exits and netd confirms interface release.

The blocking task also owns the per-VM lock, the existing removal guard, and final state/CID cleanup. This matters because cancelling the async waiter does not cancel an already-running blocking task. Offloading only the filesystem call would let cancellation release the guards while deletion continues, or leave a deleted VM occupying its CID.

Apply the same offload to failed-create cleanup and image deletion. The existing removing state and .removing recovery mechanism are unchanged; this PR adds no VM state or RPC changes.

Tests

  • cargo test --locked -p dstack-vmm: 224 passed, 0 failed, 3 ignored. The ignored tests require root and a network namespace.
  • Formatting and strict Clippy checks pass.
  • A single-worker regression test pauses deletion and checks that Status can still complete. It fails against the old implementation and passes with this fix. Cancellation, netd ordering, orphan retention, and marker ordering also have regression checks.

The async-progress test covers VM removal. The two other deletion paths have result/error tests, not async-progress tests.

This removes a confirmed source of executor blocking. It does not establish why one blocked worker delayed multiple requests on the production 128-worker runtime, or explain stalls without a removal.

Signed-off-by: Leechael Yim <yanleech@gmail.com>
@Leechael
Leechael force-pushed the fix/vmm-offload-directory-deletion branch from 4e73819 to 109599d Compare October 17, 2026 10:13
Signed-off-by: Leechael Yim <yanleech@gmail.com>

@kvinwang kvinwang 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.

The core fix is right: tokio::spawn doesn't move the synchronous remove_all() off the executor, and unlinking a preallocated qcow2 can take seconds in the kernel. But most of the +445 lines are tests that don't test this change, and the production stall isn't fully explained yet. I'd like to see this trimmed before merging.

1. The cancellation scenario doesn't happen in production

finish_remove_vm is only reached through tokio::spawn (remove_vm, spawn_finish_remove). No request future awaits it, so the only way it gets cancelled is runtime shutdown. start_vm is different: its future belongs to the RPC request, which is why the launch lock has to travel into config_qemu's blocking task. Here, the minimal fix is enough:

if let Err(err) = tokio::task::spawn_blocking(move || vm_path.remove_all()).await? { ... }

and the rest of the function stays as it is. If you want to keep the guards in the closure defensively, the comment should say that, not describe a race that can't occur. cancelling_vm_deletion_keeps_guards_and_finishes_cleanup tests that non-existent scenario.

2. Test-only hook on App

The #[cfg(test)] before_remove_workdir: Option<Arc<dyn Fn() -> Result<()> + Send + Sync>> field and the remove_vm_workdir wrapper exist only for injection. I'd prefer not to put test seams into the production struct for this.

3. Test scope

Only slow_vm_deletion_does_not_block_status tests this change, and even it could lose about half of its assertions. The others cover behavior this PR doesn't change: unmarked orphan retention, failed-deletion marker recovery, the malformed .removing marker, delete_image name and not-found validation, and the create_vm load-failure cleanup. Some also assert internals: launch_lock_handle().try_lock(), cid_pool.occupy(), and the panic/JoinError path. That makes them brittle under later refactors. Please drop them or move them to a separate PR.

4. create_vm failure-path offload is unnecessary

When load_vm fails, the workdir only holds the manifest and shared config. The disk is created later, in config_qemu. That remove_dir_all is effectively instant, so the spawn_blocking plus the three-arm match only adds code. The delete_image offload makes sense, since image dirs can be large.

5. The production stall is still unexplained

As the description says, one blocked worker out of 128 shouldn't delay several Status requests if work-stealing were in play. One plausible explanation (unverified): tokio's per-worker LIFO slot can't be stolen (tokio-rs/tokio#4941). A task woken on the blocked worker, such as a hyper connection task, waits until that worker returns. That matches "Status requests were dispatched immediately after the deletion finished". If that's the cause, any synchronous fs work on the async path in VMM can reproduce the stall, not just the deletion. That deserves a follow-up audit, or a look at disable_lifo_slot (still unstable), more than additional tests here.

Suggested shape: offload only remove_all (and delete_image), drop the create_vm change and the test hook, keep at most one regression test, and track the LIFO-slot question separately.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants