google / google/capsem

[triage:service2-01-named-vm-provision-race-orphan] Concurrent `create -n <name>` race: name guard is a non-atomic pre-check → orphaned VM process + leaked session dir

Open
#144 0 comments 0 reactions 0 assignees View on GitHub
component:service type:bug
Dominant language
Rust
Stars
72
Forks
13
Avg merge
1d 2h
Merged PRs (30d)
5

Description

Imported from Capsem triage report `service2-01-named-vm-provision-race-orphan.md`.

- Severity: `medium`
- Category: `bug`
- Area: `capsem-service`
- Location: - `crates/capsem-service/src/main.rs:3228-3236` (handle_provision early `existing_session_names` 409 check)
- Confidence: `verified`

## Summary
The persistent-VM name uniqueness guard is a check-then-act TOCTOU. Both the handler-level
409 check (`existing_session_names`, main.rs:3228) and the `provision_sandbox`-level check
(`registry.contains(name)`, main.rs:1175) are **read-only pre-checks that do not reserve
the name**, and both release the registry lock before doing any work. The name is only
actually reserved by `registry.register(...)` at main.rs:1497 — which runs *after* the
session directory is created (1251) and the `capsem-process` VM child is spawned (1280).
Two concurrent `capsem create -n foo` requests therefore both pass every pre-check, both
boot a VM, and only the second fails at `register()`. The loser has already spawned an
orphaned VM process and created a session directory that are never cleaned up.

## Evidence
Interleaving of request A and request B, both `create -n foo`, `foo` not yet existing:

1. A and B both pass `handle_provision`'s `existing_session_names` check (main.rs:3228) —
neither is registered, so neither sees the other; no CONFLICT is returned.
2. Inside `provision_sandbox`, A and B both take the registry lock, evaluate
`registry.contains("foo")` → `false`, and **release the lock** (the guard drops at the
end of the `if persistent` block, main.rs:1182). No entry is inserted here.
3. A and B each get a distinct `id` (`new_persistent_vm_id`, main.rs:3237), so the
`instances`/id checks at main.rs:1198-1201 don't collide.
4. A and B each `std::fs::create_dir_all(&session_dir)` (main.rs:1252; dir is keyed by
the unique `id`) and each `spawn()` a `capsem-process` (main.rs:1280). **Two VMs are
now booting under the name `foo`.**
5. A and B each spawn a child-exit reaper task that takes ownership of their `child`
(main.rs:1396) — this reaper only fires cleanup when the child *exits*.
6. A and B reach `registry.register(...)` (main.rs:1497). `register` (registry.rs:120)
re-checks under the lock: the first commits, the **second returns
`Err("... already exists")`** via `?`, so `provision_sandbox` returns early —
**before** the `instances.insert(...)` at main.rs:1538.

The loser (B) is now in a leaked state: its `capsem-process` (and the guest VM) is
running, spawned at 1280 and owned only by the reaper task that is `await`ing an exit that
won't come; it was never inserted into `instances` (1538 not reached) and never registered.
It is invisible to `list`, `stop`, `delete`, and the persistent registry. Its session dir
under `run/persistent//` is left on disk.

## Impact
Two concurrent creates of the same persistent name (a double-submit, a retrying script, a
double-click in the UI) leak a fully-booted, un-tracked VM process — consuming RAM/CPU and
an Apple VZ slot — plus an orphaned session directory, until the service is restarted. The
user also gets an opaque 500 `"... already exists"` from deep in provisioning instead of
the clean 409 the early check was meant to provide, because that early check does not hold
across the provisioning window. This is on the daemon's core lifecycle path.

## Suggested fix
Reserve the name atomically *before* any side effect. Insert a placeholder/"provisioning"
entry into `persistent_registry` under the lock at the top of `provision_sandbox` (using
the atomic `register`, which already rejects duplicates), so a concurrent create fails
immediately with a clean 409 and no dir/process is created; on any later failure, roll the
placeholder back (the retry path already calls `registry.unregister(&name)` at
main.rs:3283, so the rollback primitive exists). Alternatively, serialize provisioning per
name with a keyed async lock. Either way, remove the redundant racy `contains`/`existing_session_names`
pre-checks or make them advisory only. Add a test that fires two `provision` calls for the
same name concurrently and asserts exactly one VM exists and no orphan process/dir remains.

## Triage
Confirmed from the local reviewed report in `/Users/elie/git/capsem/tmp/bugs/service2-01-named-vm-provision-race-orphan.md`. Track implementation in the triage sprint; add regression coverage before fixing.

Contributor guide

Open the contributing guide

Research direction

Start at handle_provision and provision_sandbox in crates/capsem-service/src/main.rs, then read registry.rs and the register/unregister paths. Add the concurrent same-name provision regression test described in the issue and run the relevant service tests. Done means one create succeeds, the other returns a clean 409, and no orphan VM process or session directory remains.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.