I was reading through crates/forkd-controller/src/http.rs to understand how the BRANCH endpoint enforces its concurrency limits, and the BranchSlot design (lines 80-140) is genuinely well thought out.
What I like:
-
Both invariants are bundled into one type. The per-tag exclusion set (branch_in_flight: HashSet<String>) and the global semaphore are two pieces of state that have to be acquired and released together — splitting them across the handler body would make it really easy to add an early-return that forgets one half. Wrapping both in BranchSlot and giving it Drop collapses the cleanup into "let it go out of scope".
-
Acquisition order is documented. The comment at lines 122-123 ("Acquire the global permit first") explains exactly why the ordering matters — if you did it the other way, you'd have a HashSet rollback to write on the semaphore-failure path. The comment also tells future-you why you can't just "fix" the order.
-
The error type is just two variants — AlreadyInFlight (→ 409) and CapacityExceeded (→ 503) — which exactly mirror the two failure modes a client cares about. No Box<dyn Error>, no string matching.
-
It's tested in isolation. The three branch_slot_* tests (lines 1600-1670) cover the happy path, the same-tag conflict, the cap, and (the easy-to-forget one) recovery after Drop. The recovery test is exactly the regression I'd worry about if someone refactored the order of operations.
This is the kind of small primitive that pays for itself the first time someone adds a new early-return to branch_sandbox and it just works. Nice work.
I was reading through
crates/forkd-controller/src/http.rsto understand how the BRANCH endpoint enforces its concurrency limits, and theBranchSlotdesign (lines 80-140) is genuinely well thought out.What I like:
Both invariants are bundled into one type. The per-tag exclusion set (
branch_in_flight: HashSet<String>) and the global semaphore are two pieces of state that have to be acquired and released together — splitting them across the handler body would make it really easy to add an early-return that forgets one half. Wrapping both inBranchSlotand giving itDropcollapses the cleanup into "let it go out of scope".Acquisition order is documented. The comment at lines 122-123 ("Acquire the global permit first") explains exactly why the ordering matters — if you did it the other way, you'd have a HashSet rollback to write on the semaphore-failure path. The comment also tells future-you why you can't just "fix" the order.
The error type is just two variants —
AlreadyInFlight(→ 409) andCapacityExceeded(→ 503) — which exactly mirror the two failure modes a client cares about. NoBox<dyn Error>, no string matching.It's tested in isolation. The three
branch_slot_*tests (lines 1600-1670) cover the happy path, the same-tag conflict, the cap, and (the easy-to-forget one) recovery afterDrop. The recovery test is exactly the regression I'd worry about if someone refactored the order of operations.This is the kind of small primitive that pays for itself the first time someone adds a new early-return to
branch_sandboxand it just works. Nice work.