fix(execution): reserve process capacity before launching

[PierrunoYT]

What changed

Fixes #1085 (approved for community implementation).

Reserve capacity under the manager lock before starting the transport. In-flight launches count toward MaxProcesses; launch failures release their reservation and admission failures clean up prepared resources. At capacity, prune completed history only and return ErrProcessCapacity if every slot is live or reserved.

This intentionally replaces live eviction with rejection, including for limits below eight. Admission no longer selects a live victim or treats a kill attempt as recovered capacity, eliminating the shared-victim race and the capacity consequences of ignored kill errors. Stop/StopAll signatures are unchanged. Remove cannot forget a live process and bypass accounting. Zero still means the default limit (64), and negative limits remain unlimited.

Regression evidence

Ran the new tests against the original implementation using a Go source overlay. They failed for the intended reasons:

TestProcessManagerReservesCapacityBeforeTransport:
  additional transport launched while the only slot was reserved
TestProcessManagerCapacityDoesNotEvictLiveProcesses/{1,9}:
  full manager returned unexpected launch, want capacity error before launch
TestProcessManagerCapacityWithRunningProcess:
  second start = <nil>, process=...; want rejection before OS launch

Coverage includes 16 simultaneous starts behind a blocked transport, reservation release after transport failure, prepared-resource cleanup, failed termination, attempted removal of live identity, completed-history pruning, and a real child process at MaxProcesses=1 followed by slot reuse after completion. New tests use portable Go child processes/fake transports rather than POSIX shell commands.

Verification

Executed on Linux amd64 with the Go version in go.mod (1.26.6):

• make fmt-check — passed
• go vet ./... — passed
• go test ./... — passed
• go test -race ./internal/execution -count=1 — passed
• go test -race ./internal/execution -run 'TestProcessManager(ReservesCapacity|CapacityDoes|CapacityWith)' -count=30 — passed
• go run ./cmd/zero-release build — passed
• go run ./cmd/zero-release smoke — zero smoke check passed (0.9.0)
• make vulncheck — No vulnerabilities found.
• git diff HEAD --check — passed before commit
• make lint-static — four pre-existing, unrelated advisory staticcheck findings: QF1001 in internal/installtest/workflow_permissions_test.go:20; QF1008 in internal/proxydial/proxydial.go:67,72 and internal/tools/web_fetch.go:315. No findings in changed files.

Native macOS and Windows execution was not available in this Linux orb. Upstream main was fetched and confirmed unchanged before committing.

Summary by CodeRabbit

• Bug Fixes
  • Improved process-limit handling: when all available slots are occupied by active processes, attempts to start another process are rejected instead of interrupting a running process. Completed processes free capacity for new starts.
  • Failed starts no longer leave capacity unavailable, so later attempts can proceed when a slot is free.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: Gitlawb/zero/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 4831b034-748a-4a5e-9615-0332949a51d0

📥 Commits

Reviewing files that changed from the base of the PR and between 99721c7 and 79ff80e.

📒 Files selected for processing (2)

• internal/execution/process_manager.go
• internal/execution/process_manager_test.go

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

---

Walkthrough

ProcessManager now reserves capacity before starting a process. When a positive limit is full, it prunes a completed process or returns ErrProcessCapacity. Live processes are not evicted or removed to bypass the limit. Tests cover concurrent starts, failed launches, and capacity release after completion.

Changes

Process Capacity Enforcement

Layer / File(s)	Summary
Reserve capacity before launch internal/execution/process_manager.go, internal/execution/process_manager_test.go	Start reserves capacity before transport startup and releases the reservation if startup fails. Admission failure cleans prepared resources. Pruning no longer selects a live process. Tests check concurrent admission and cleanup.
Preserve live entries and release completed capacity internal/execution/process_manager.go, internal/execution/process_manager_test.go	Remove only deletes completed processes. Tests check capacity rejection, release after completion, and behavior with a running child process.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Start as ProcessManager.Start
  participant ProcessManager
  participant Transport
  participant ChildProcess
  Start->>ProcessManager: request process start
  ProcessManager->>ProcessManager: reserve capacity
  ProcessManager->>Transport: start process
  Transport->>ChildProcess: launch OS process
  Transport-->>ProcessManager: return launch result
  ProcessManager->>ProcessManager: store process and release reservation

Loading

Suggested reviewers: anandh8x

Merge Risk: ⚪ Minimal · up to 79ff8

Process capacity is reserved before launch, and completed processes can free slots for later starts. No identified issue prevents merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 79ff8

The new admission rule prevents launches from exceeding the configured limit and avoids evicting running processes. A stalled launch could occupy capacity without being reachable by the normal stop operation; whether this can occur in production remains uncertain.

Retained concerns

• Low · reliability · inferred: An in-flight launch holds a capacity reservation before it has a process identity. If transport startup stalls, cancellation or StopAll cannot reach that reservation or its prepared-resource cleanup, and subsequent starts can be rejected until startup returns.

Security review details

Security Blast Radius

• inferred — The demonstrated availability effect is bounded to starts using the same ProcessManager and its configured capacity. The evidence does not establish a cross-tenant or service-wide exposure.

Trust Boundaries and Controls

• observed — The inspected command-tool caller selects the sandbox engine and prepares and validates the request before Start; the changed admission path does not itself grant additional process-launch authority.

Resilience and Maintainability Implications

• inferred — A startup that never returns differs from a returned startup failure: its reservation remains counted while StopAll has no process entry to terminate. Whether production startup can remain blocked long enough to affect availability is unresolved.

Hardening Proposals

• proposed — Consider an owned, cancellable in-flight launch state or a bounded startup path so stopping a manager can account for pending launches without releasing a slot while an OS launch might still succeed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: reserving process capacity before launching a process.
Linked Issues check	✅ Passed	The PR meets the coding requirements in [#1085]. reserve runs under manager.mu before startTransport, and starting counts in-flight launches. A positive limit rejects admission when all slots …
Out of Scope Changes check	✅ Passed	The changes are limited to internal/execution/process_manager.go and its tests. The implementation changes capacity accounting, launch cleanup, live-process retention, and completed-history pruning.…

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[Vasanthdev2004]

Reviewed at 79ff80ed. Rejecting at capacity instead of evicting a live process is one of the options #1085 offered, and it makes the limit strict, so the eight-process floor I said could stay goes with the eviction. That's the right trade: a rejection tells the caller to stop something, where an eviction killed a process somebody may still have wanted.

The reservation holds on every path. Each start between reserve and store either stores the process or gives the slot back, and no wording about eviction is left for the model to read. Each piece is load-bearing:

• Dropping the capacity check fails TestProcessManagerReservesCapacityBeforeTransport and both legs of TestProcessManagerCapacityDoesNotEvictLiveProcesses.
• Keeping the reservation after a failed launch fails with failed launch did not release reservation.
• Letting Remove forget a live process frees a slot the capacity test then catches.
• Keeping the reservation in store stops the slot being reused after completion in TestProcessManagerCapacityWithRunningProcess.

internal/execution passes natively on Windows, and under -race for the manager tests. CI is 9 of 9 at head. Approving.

[anandh8x]

LGTM. Verified the reservation is taken before transport start and released on every path, and the new tests fail on main for the intended reasons.

[euxaristia]

The reserve-before-launch race is closed: the starting counter lives under the same mutex as the map, completed-only pruning means no live eviction, both failure paths release, and the test drives the race with a real blocked transport and a live process.