Question: design rationale for tag-scoped vs sandbox-scoped branch concurrency

[bouncia]

In crates/forkd-controller/src/http.rs, try_acquire_branch_slot (lines 117-139) keys the in-flight set on the target snapshot tag of the branch operation, not the source sandbox id:

pub fn try_acquire_branch_slot(self: &Arc<Self>, tag: &str) -> Result<BranchSlot, ...> {
    ...
    if !in_flight.insert(tag.to_string()) { return Err(AlreadyInFlight); }
    ...
}

The comment on branch_in_flight (lines 53-58) explains why: "two concurrent POST /branch calls targeting the same tag from racing to clobber memory.bin". That's clearly correct — two clients picking the same explicit tag shouldn't be allowed through.

But the default tag is auto-generated as branch-<sandbox-id>-<unix-now-secs> (line 531). So the realistic collision case is two clients branching the same sandbox at the same second — which gets rejected with 409 "branch for tag 'branch-…' is already in progress". From the client's POV that's surprising: they didn't pick the same tag, the daemon did, and the daemon then refused them.

Two design questions:

1. Would per-sandbox-id exclusion be more useful? A given source sandbox can only have one in-flight pause/snapshot/resume cycle anyway (pause is exclusive — line 599 takes the Vm out of live_vms), so keying on sandbox id rather than tag gives the same correctness with a clearer error ("sandbox X is already being branched"), and the auto-tag collision goes away.

2. Should the auto-tag include sub-second precision? Even with tag-scoped locking, generating tags that include unix_now() in seconds means two branches of the same sandbox in the same second collide. A nanosecond suffix or a short random would prevent that without changing the semantics.

I have my own preference (sandbox-id exclusion + sub-second auto-tag) but don't want to send a PR without knowing the maintainers' rationale for the current shape. Happy to write either version.

[WaylandYang]

Thanks for the careful writeup — verified all of it against current main. Your reasoning is sound; the line numbers drifted (http.rs moved a lot since you filed). Re-anchoring, then a disposition.

Citations — four of five correct, just mis-numbered: (A) the in-flight set is keyed on the target tag, http.rs:155; (B) the "race to clobber memory.bin" comment, http.rs:54; (C) auto-tag branch-{id}-{unix_now()}, http.rs:1195; (D) unix_now() is seconds, http.rs:1830. The fifth is off in mechanism, not just line: single-branch-per-sandbox isn't from pause being exclusive — it's from the source id being removed from live_vms for the whole cycle (http.rs:1316; a second branch on the same id then hits None => not_found, and the VM is reinserted at :1629). pause() is later, inside the blocking task (:1463).

Proposal #1 (re-key the lock on sandbox-id): declining — not correctness-equivalent. Re-keying reopens a real memory.bin clobber: two different sandboxes branching to the same explicit tag concurrently — POST /sandboxes/A/branch {tag:"out"} and .../B/branch {tag:"out"}. The destination is keyed purely on the tag (snap_dir = snapshot_root.join(&tag), :1254 → memory.bin at :1378); nothing rejects two sandboxes picking the same tag (is_safe_tag, :689, has no notion of the source); the semaphore is global, not per-tag (lib.rs:134). Today both the concurrent path (tag-keyed branch_in_flight, :155) and the sequential path (the vmstate-exists 409, :1255) gate this case — and both gate on the tag. Re-key to the id and the concurrent gate is gone: A locks "A", B locks "B", both pass the TOCTOU .exists(), both write out/memory.bin. So the tag is the right key — it names the resource being written. You're right the lock is load-bearing, just for the cross-sandbox case rather than the one in the title; for the same sandbox it indeed buys nothing the live_vms removal doesn't already give.

Proposal #2 (entropy on the auto-tag): accept — please send it. The same-second collision is real and the auto path shouldn't surface it to the caller. I'd take a short monotonic suffix over nanoseconds (ns can still tie on fast hardware and bloats the tag):

// http.rs:1195
.unwrap_or_else(|| format!("branch-{}-{}-{:04x}", id, unix_now(), branch_suffix()));

fn branch_suffix() -> u16 {  // beside unix_now()
    use std::sync::atomic::{AtomicU16, Ordering};
    static SEQ: AtomicU16 = AtomicU16::new(0);
    SEQ.fetch_add(1, Ordering::Relaxed)
}

+5 chars, [0-9a-f], well under the 64-char cap, and nothing parses the tag back into parts (:1195 is its only producer). That one-liner + the helper is the whole PR — I'll review fast.

One doc fix that's on me, not you: docs/API.md documented a tag regex (^[A-Za-z0-9_][A-Za-z0-9._-]{0,63}$) that doesn't match is_safe_tag ([A-Za-z0-9_-], no leading-char restriction, no .). Corrected it just now. Thanks for the sharp question.