fix: enforce classified Git admission paths (#2150) (#2181)

This commit is contained in:
Carlos Florêncio
2026-08-03 11:04:34 +01:00
committed by GitHub
parent 069cc99e10
commit 431e5d40fe
71 changed files with 3130 additions and 357 deletions
+5
View File
@@ -200,6 +200,11 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update
- Provider pattern for DI; stderr for logs, stdout for ACP only.
- Pass context through chains; event bus for cross-component comm.
- Production Git commands must use `subproc.NewGitCommand` with a classified
`subproc.RunGit*` helper, or hold a classified admission slot across
streaming `Start`/`Wait`. Do not construct raw Git commands outside
`internal/common/subproc`; choose `interactive`, `lifecycle`, or `background`
explicitly for each operation.
- **Event-bus wildcard parity:** New NATS wildcard subscriptions must verify equivalent `MemoryEventBus` semantics in `go test ./internal/events/bus`.
- **Repository provider identity:** Provider-backed repositories are keyed by workspace, provider, normalized `provider_host` origin, full owner/namespace, and name. Persist `provider_host` when importing or resolving a remote; do not infer self-managed GitLab rows from owner/name alone. Legacy rows with an empty host have unknown identity and must fail closed for provider write/link operations.
- **Execution access:** Workspace-oriented handlers (files, shell, inference, ports, vscode, LSP) MUST use `GetOrEnsureExecution(ctx, sessionID)` — it recovers from backend restarts by creating executions on-demand. Only use `GetExecutionBySessionID` for operations that require a running agent process (prompt, cancel, mode).
+3 -2
View File
@@ -13,6 +13,7 @@ import (
"runtime"
"strings"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/githubauth"
)
@@ -118,8 +119,8 @@ func resolveGitHubCLIRepository(
if raw = strings.TrimSpace(getenv("GH_REPO")); raw != "" {
return parseGitHubCLIRepository(raw, defaultHost)
}
cmd := exec.CommandContext(ctx, "git", "remote", "get-url", "origin")
output, err := cmd.Output()
cmd := subproc.NewGitCommand(ctx, "remote", "get-url", "origin")
output, err := subproc.RunGitOutputClass(ctx, subproc.GitInteractive, cmd)
if err != nil {
return nil, nil
}
+11 -5
View File
@@ -4,11 +4,12 @@ import (
"context"
"fmt"
"os"
"os/exec"
"strings"
"time"
acp "github.com/coder/acp-go-sdk"
"github.com/kandev/kandev/internal/common/subproc"
)
// Predefined e2e test scenarios with fixed timing for deterministic test assertions.
@@ -482,13 +483,13 @@ func scenarioPushCurrentBranch(e *emitter) {
e.text("push-current-branch: getwd failed: " + err.Error())
return
}
branchNameOut, err := exec.Command("git", "-C", wd, "rev-parse", "--abbrev-ref", "HEAD").Output()
branchNameOut, err := runGitOutput(wd, "rev-parse", "--abbrev-ref", "HEAD")
if err != nil {
e.text("push-current-branch: resolve branch name failed: " + err.Error())
return
}
branchName := strings.TrimSpace(string(branchNameOut))
headSHAOut, err := exec.Command("git", "-C", wd, "rev-parse", "HEAD").Output()
headSHAOut, err := runGitOutput(wd, "rev-parse", "HEAD")
if err != nil {
e.text("push-current-branch: resolve HEAD sha failed: " + err.Error())
return
@@ -1076,6 +1077,11 @@ func scenarioSymlinkFileSetup(e *emitter) {
e.text("symlink-file-setup complete")
}
func runGitOutput(wd string, args ...string) ([]byte, error) {
cmd := subproc.NewGitCommand(context.Background(), append([]string{"-C", wd}, args...)...)
return subproc.RunGitOutputClass(context.Background(), subproc.GitLifecycle, cmd)
}
// makeGitRunner returns a function that runs git commands in the given directory.
func makeGitRunner(wd string) func(args ...string) error {
gitEnv := append(os.Environ(),
@@ -1085,13 +1091,13 @@ func makeGitRunner(wd string) func(args ...string) error {
"GIT_COMMITTER_EMAIL=mock@test.local",
)
return func(args ...string) error {
cmd := exec.Command("git", append([]string{
cmd := subproc.NewGitCommand(context.Background(), append([]string{
"-c", "commit.gpgsign=false",
"-c", "tag.gpgsign=false",
}, args...)...)
cmd.Dir = wd
cmd.Env = gitEnv
out, cmdErr := cmd.CombinedOutput()
out, cmdErr := subproc.RunGitCombinedOutputClass(context.Background(), subproc.GitLifecycle, cmd)
if cmdErr != nil {
_, _ = fmt.Fprintf(logOutput, "mock-agent: git %v failed: %v\nOutput: %s\n", args, cmdErr, out)
}
@@ -12,6 +12,7 @@ import (
"time"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
)
@@ -208,6 +209,32 @@ func (c *ControlClient) Health(ctx context.Context) error {
return nil
}
// SubprocessAdmission returns the agentctl process's current Git admission
// state. It is used by the backend debug export to correlate host and agentctl
// pressure without exposing control-server internals directly.
func (c *ControlClient) SubprocessAdmission(ctx context.Context) (subproc.Snapshot, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/api/v1/debug/subprocess-admission", nil)
if err != nil {
return subproc.Snapshot{}, err
}
resp, err := c.httpClient.Do(req)
if err != nil {
return subproc.Snapshot{}, fmt.Errorf("failed to get subprocess admission: %w", err)
}
defer func() { _ = resp.Body.Close() }()
if resp.StatusCode != http.StatusOK {
return subproc.Snapshot{}, fmt.Errorf("failed to get subprocess admission: status %d", resp.StatusCode)
}
var snapshot subproc.Snapshot
if err := json.NewDecoder(resp.Body).Decode(&snapshot); err != nil {
return subproc.Snapshot{}, fmt.Errorf("failed to decode subprocess admission: %w", err)
}
return snapshot, nil
}
// CreateInstance creates a new agent instance.
func (c *ControlClient) CreateInstance(ctx context.Context, req *CreateInstanceRequest) (*CreateInstanceResponse, error) {
body, err := json.Marshal(req)
@@ -0,0 +1,52 @@
package client
import (
"context"
"encoding/json"
"fmt"
"net"
"net/http"
"net/http/httptest"
"testing"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
)
func TestControlClientSubprocessAdmission(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodGet || r.URL.Path != "/api/v1/debug/subprocess-admission" {
t.Errorf("request = %s %s, want GET /api/v1/debug/subprocess-admission", r.Method, r.URL.Path)
}
if got := r.Header.Get("Authorization"); got != "Bearer admission-secret" {
t.Errorf("authorization = %q, want bearer token", got)
}
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(subproc.Snapshot{
Pool: "git",
Capacity: 4,
Inflight: 1,
Waiters: 2,
Classes: map[string]subproc.ClassSnapshot{"interactive": {Waiters: 2}},
})
}))
t.Cleanup(server.Close)
host, port, err := net.SplitHostPort(server.Listener.Addr().String())
if err != nil {
t.Fatalf("split server address: %v", err)
}
var portNumber int
if _, err := fmt.Sscanf(port, "%d", &portNumber); err != nil {
t.Fatalf("parse server port: %v", err)
}
client := NewControlClient(host, portNumber, logger.Default(), WithControlAuthToken("admission-secret"))
snapshot, err := client.SubprocessAdmission(context.Background())
if err != nil {
t.Fatalf("SubprocessAdmission() error = %v", err)
}
if snapshot.Pool != "git" || snapshot.Capacity != 4 || snapshot.Classes["interactive"].Waiters != 2 {
t.Fatalf("snapshot = %+v, want decoded admission snapshot", snapshot)
}
}
@@ -146,24 +146,24 @@ func (p *LocalPreparer) Prepare(ctx context.Context, req *EnvPrepareRequest, onP
// the workspace_path may be a worktree pointer file or a submodule, both of
// which git resolves correctly while a manual HEAD read would not.
func readCurrentBranchForLocal(workDir string) string {
// `git symbolic-ref` is a cheap local-only call. Acquire the throttle
// slot first (30s budget) and only THEN start the 5s exec timer —
// otherwise queue time eats the exec budget and a stale "" sentinel
// could fall through to a same-branch-checkout that touches the
// working tree.
// `git symbolic-ref` is a cheap local-only call. The after-admission
// helper starts the 5s exec timer only after the lifecycle slot is granted;
// otherwise queue time eats the exec budget and a stale "" sentinel could
// fall through to a same-branch-checkout that touches the working tree.
acquireCtx, cancelAcquire := context.WithTimeout(context.Background(), 30*time.Second)
defer cancelAcquire()
release, err := subproc.Git().Acquire(acquireCtx)
if err != nil {
return ""
}
defer release()
execCtx, cancelExec := context.WithTimeout(context.Background(), 5*time.Second)
defer cancelExec()
cmd := exec.CommandContext(execCtx, "git", "symbolic-ref", "--short", "HEAD")
cmd.Dir = workDir
out, err := cmd.Output()
if err != nil {
out, runErr, execCtxErr := subproc.RunGitOutputAfterAcquireWithExecutionContext(
acquireCtx,
context.Background(),
subproc.GitLifecycle,
5*time.Second,
func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, "symbolic-ref", "--short", "HEAD")
cmd.Dir = workDir
return cmd
},
)
if runErr != nil || execCtxErr != nil {
return ""
}
return strings.TrimSpace(string(out))
@@ -174,13 +174,13 @@ func readCurrentBranchForLocal(workDir string) string {
// the checkout. If the local branch doesn't exist but the remote tracking
// branch does (from the fetch), git creates a local branch tracking it.
func checkoutBranch(ctx context.Context, workDir, branch string, sensitiveValues []string) (string, error) {
fetchCmd := exec.CommandContext(ctx, "git", "fetch", "origin", branch)
fetchCmd := subproc.NewGitCommand(ctx, "fetch", "origin", branch)
fetchCmd.Dir = workDir
fetchOut, fetchErr := subproc.RunGitCombinedOutput(ctx, fetchCmd)
fetchOut, fetchErr := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, fetchCmd)
cmd := exec.CommandContext(ctx, "git", "checkout", branch)
cmd := subproc.NewGitCommand(ctx, "checkout", branch)
cmd.Dir = workDir
out, err := subproc.RunGitCombinedOutput(ctx, cmd)
out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd)
outStr := redactCheckoutOutput(strings.TrimSpace(string(out)), sensitiveValues)
if err != nil {
if fetchErr != nil {
@@ -12,6 +12,7 @@ import (
agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl"
"github.com/kandev/kandev/internal/agentctl/server/process"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
)
// StandaloneExecutor implements Runtime for standalone agentctl execution.
@@ -48,6 +49,12 @@ func (r *StandaloneExecutor) HealthCheck(ctx context.Context) error {
return r.ctl.Health(ctx)
}
// SubprocessAdmission returns the admission snapshot from the host agentctl
// control server for backend diagnostics.
func (r *StandaloneExecutor) SubprocessAdmission(ctx context.Context) (subproc.Snapshot, error) {
return r.ctl.SubprocessAdmission(ctx)
}
func (r *StandaloneExecutor) waitForReady(ctx context.Context) error {
if err := r.ctl.Health(ctx); err == nil {
return nil
@@ -0,0 +1,44 @@
package lifecycle
import (
"context"
"encoding/json"
"fmt"
"net"
"net/http"
"net/http/httptest"
"testing"
agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
)
func TestStandaloneExecutorSubprocessAdmission(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/api/v1/debug/subprocess-admission" {
t.Fatalf("path = %q, want admission route", r.URL.Path)
}
_ = json.NewEncoder(w).Encode(subproc.Snapshot{Pool: "git", Capacity: 6})
}))
t.Cleanup(server.Close)
host, port, err := net.SplitHostPort(server.Listener.Addr().String())
if err != nil {
t.Fatalf("split server address: %v", err)
}
var portNumber int
if _, err := fmt.Sscanf(port, "%d", &portNumber); err != nil {
t.Fatalf("parse server port: %v", err)
}
client := agentctl.NewControlClient(host, portNumber, logger.Default())
executor := NewStandaloneExecutor(client, host, portNumber, logger.Default())
snapshot, err := executor.SubprocessAdmission(context.Background())
if err != nil {
t.Fatalf("SubprocessAdmission() error = %v", err)
}
if snapshot.Pool != "git" || snapshot.Capacity != 6 {
t.Fatalf("snapshot = %+v, want control-client snapshot", snapshot)
}
}
@@ -0,0 +1,36 @@
package lifecycle
import (
"context"
"testing"
"github.com/kandev/kandev/internal/agent/executor"
"github.com/kandev/kandev/internal/common/subproc"
)
type admissionExecutor struct {
MockExecutor
snapshot subproc.Snapshot
}
func (e *admissionExecutor) SubprocessAdmission(context.Context) (subproc.Snapshot, error) {
return e.snapshot, nil
}
func TestManagerSubprocessAdmissionUsesStandaloneProvider(t *testing.T) {
log := newTestRegistryLogger()
registry := NewExecutorRegistry(log)
registry.Register(&admissionExecutor{
MockExecutor: MockExecutor{name: executor.NameStandalone},
snapshot: subproc.Snapshot{Pool: "git", Capacity: 5},
})
manager := NewManager(nil, nil, registry, nil, nil, nil, ExecutorFallbackWarn, "", log)
snapshot, err := manager.SubprocessAdmission(context.Background())
if err != nil {
t.Fatalf("SubprocessAdmission() error = %v", err)
}
if snapshot.Pool != "git" || snapshot.Capacity != 5 {
t.Fatalf("snapshot = %+v, want provider snapshot", snapshot)
}
}
@@ -0,0 +1,31 @@
package lifecycle
import (
"context"
"fmt"
"github.com/kandev/kandev/internal/agent/executor"
"github.com/kandev/kandev/internal/common/subproc"
)
type subprocessAdmissionProvider interface {
SubprocessAdmission(context.Context) (subproc.Snapshot, error)
}
// SubprocessAdmission returns the standalone agentctl admission snapshot when
// the configured default runtime exposes it. Other runtimes remain optional:
// callers can still export the backend-local snapshot when this probe fails.
func (m *Manager) SubprocessAdmission(ctx context.Context) (subproc.Snapshot, error) {
if m.executorRegistry == nil {
return subproc.Snapshot{}, fmt.Errorf("no runtime registry configured")
}
backend, err := m.executorRegistry.GetBackend(executor.NameStandalone)
if err != nil {
return subproc.Snapshot{}, err
}
provider, ok := backend.(subprocessAdmissionProvider)
if !ok {
return subproc.Snapshot{}, fmt.Errorf("standalone runtime does not expose subprocess admission")
}
return provider.SubprocessAdmission(ctx)
}
@@ -5,7 +5,6 @@ import (
"errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"sync"
@@ -18,6 +17,7 @@ import (
"github.com/kandev/kandev/internal/agent/executor"
"github.com/kandev/kandev/internal/agent/runtime/activity"
"github.com/kandev/kandev/internal/agent/settings/cliflags"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/events"
"github.com/kandev/kandev/internal/gitconfigenv"
storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces"
@@ -1512,20 +1512,20 @@ func (m *Manager) initGitRepo(ctx context.Context, workspacePath string) error {
}
// Initialize git repository
cmd := exec.CommandContext(ctx, "git", "init")
cmd := subproc.NewGitCommand(ctx, "init")
cmd.Dir = workspacePath
if output, err := cmd.CombinedOutput(); err != nil {
if output, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil {
return fmt.Errorf("git init failed: %w (output: %s)", err, string(output))
}
// Configure git user (required for initial commit)
configName := exec.CommandContext(ctx, "git", "config", "user.name", "Kandev Quick Chat")
configName := subproc.NewGitCommand(ctx, "config", "user.name", "Kandev Quick Chat")
configName.Dir = workspacePath
_ = configName.Run() // Ignore error - might already be configured globally
_ = subproc.RunGitClass(ctx, subproc.GitLifecycle, configName) // Ignore error - might already be configured globally
configEmail := exec.CommandContext(ctx, "git", "config", "user.email", "quickchat@kandev.local")
configEmail := subproc.NewGitCommand(ctx, "config", "user.email", "quickchat@kandev.local")
configEmail.Dir = workspacePath
_ = configEmail.Run() // Ignore error - might already be configured globally
_ = subproc.RunGitClass(ctx, subproc.GitLifecycle, configEmail) // Ignore error - might already be configured globally
// Create initial commit with empty .gitkeep file
gitkeepPath := filepath.Join(workspacePath, ".gitkeep")
@@ -1533,15 +1533,15 @@ func (m *Manager) initGitRepo(ctx context.Context, workspacePath string) error {
return fmt.Errorf("failed to create .gitkeep: %w", err)
}
addCmd := exec.CommandContext(ctx, "git", "add", ".gitkeep")
addCmd := subproc.NewGitCommand(ctx, "add", ".gitkeep")
addCmd.Dir = workspacePath
if output, err := addCmd.CombinedOutput(); err != nil {
if output, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, addCmd); err != nil {
return fmt.Errorf("git add failed: %w (output: %s)", err, string(output))
}
commitCmd := exec.CommandContext(ctx, "git", "commit", "-m", "Initial commit")
commitCmd := subproc.NewGitCommand(ctx, "commit", "-m", "Initial commit")
commitCmd.Dir = workspacePath
if output, err := commitCmd.CombinedOutput(); err != nil {
if output, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, commitCmd); err != nil {
return fmt.Errorf("git commit failed: %w (output: %s)", err, string(output))
}
@@ -1,9 +1,7 @@
package lifecycle
import (
"bytes"
"context"
"os/exec"
"path/filepath"
"strings"
@@ -11,6 +9,7 @@ import (
"github.com/kandev/kandev/internal/agent/executor"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/scriptengine"
)
@@ -94,13 +93,12 @@ func defaultPreparerSetupScript(req *EnvPrepareRequest) string {
}
func getGitRemoteURL(repoPath string) (string, error) {
cmd := exec.Command("git", "-C", repoPath, "remote", "get-url", "origin")
var out bytes.Buffer
cmd.Stdout = &out
if err := cmd.Run(); err != nil {
cmd := subproc.NewGitCommand(context.Background(), "-C", repoPath, "remote", "get-url", "origin")
out, err := subproc.RunGitOutputClass(context.Background(), subproc.GitLifecycle, cmd)
if err != nil {
return "", err
}
return strings.TrimSpace(out.String()), nil
return strings.TrimSpace(string(out)), nil
}
// runSetupScriptStep executes the resolved setup script as a named prepare step,
@@ -34,6 +34,7 @@ import (
"github.com/kandev/kandev/internal/agentctl/server/config"
"github.com/kandev/kandev/internal/agentctl/server/process"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/pkg/agent"
)
@@ -215,9 +216,9 @@ func setupWorkspace(t *testing.T) string {
}
dir := t.TempDir()
cmd := exec.Command("git", "init")
cmd := subproc.NewGitCommand(context.Background(), "init")
cmd.Dir = dir
if out, err := cmd.CombinedOutput(); err != nil {
if out, err := subproc.RunGitCombinedOutputClass(context.Background(), subproc.GitLifecycle, cmd); err != nil {
t.Fatalf("git init failed in %s: %v\n%s", dir, err, out)
}
@@ -226,17 +227,17 @@ func setupWorkspace(t *testing.T) string {
t.Fatalf("failed to create README.md: %v", err)
}
cmd = exec.Command("git", "add", ".")
cmd = subproc.NewGitCommand(context.Background(), "add", ".")
cmd.Dir = dir
_ = cmd.Run()
_ = subproc.RunGitClass(context.Background(), subproc.GitLifecycle, cmd)
cmd = exec.Command("git", "commit", "-m", "init", "--allow-empty")
cmd = subproc.NewGitCommand(context.Background(), "commit", "-m", "init", "--allow-empty")
cmd.Dir = dir
cmd.Env = append(os.Environ(),
"GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@test.com",
"GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@test.com",
)
_ = cmd.Run()
_ = subproc.RunGitClass(context.Background(), subproc.GitLifecycle, cmd)
return dir
}
@@ -0,0 +1,39 @@
package api
import (
"encoding/json"
"net/http"
"net/http/httptest"
"testing"
"github.com/kandev/kandev/internal/agentctl/server/config"
"github.com/kandev/kandev/internal/agentctl/server/instance"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
)
func TestSubprocessAdmissionRouteRequiresBearerToken(t *testing.T) {
server := NewControlServer(&config.Config{AuthToken: "admission-secret"}, &instance.Manager{}, logger.Default())
unauthenticated := httptest.NewRequest(http.MethodGet, "/api/v1/debug/subprocess-admission", nil)
unauthenticatedResponse := httptest.NewRecorder()
server.Router().ServeHTTP(unauthenticatedResponse, unauthenticated)
if unauthenticatedResponse.Code != http.StatusUnauthorized {
t.Fatalf("unauthenticated status = %d, want %d", unauthenticatedResponse.Code, http.StatusUnauthorized)
}
authenticated := httptest.NewRequest(http.MethodGet, "/api/v1/debug/subprocess-admission", nil)
authenticated.Header.Set("Authorization", "Bearer admission-secret")
authenticatedResponse := httptest.NewRecorder()
server.Router().ServeHTTP(authenticatedResponse, authenticated)
if authenticatedResponse.Code != http.StatusOK {
t.Fatalf("authenticated status = %d, want %d", authenticatedResponse.Code, http.StatusOK)
}
var snapshot subproc.Snapshot
if err := json.Unmarshal(authenticatedResponse.Body.Bytes(), &snapshot); err != nil {
t.Fatalf("decode snapshot: %v", err)
}
if snapshot.Pool != "git" {
t.Fatalf("snapshot pool = %q, want git", snapshot.Pool)
}
}
@@ -11,6 +11,7 @@ import (
"github.com/kandev/kandev/internal/agentctl/server/instance"
"github.com/kandev/kandev/internal/common/httpmw"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
)
@@ -60,6 +61,11 @@ func (m *ControlServer) setupRoutes() {
api.GET("/instances", m.handleListInstances)
api.GET("/instances/:id", m.handleGetInstance)
api.DELETE("/instances/:id", m.handleDeleteInstance)
api.GET("/debug/subprocess-admission", m.handleSubprocessAdmission)
}
func (m *ControlServer) handleSubprocessAdmission(c *gin.Context) {
c.JSON(http.StatusOK, subproc.AdmissionSnapshot())
}
func (m *ControlServer) handleHealth(c *gin.Context) {
@@ -8,11 +8,11 @@ import (
"regexp"
"sort"
"strings"
"sync"
"time"
"github.com/gin-gonic/gin"
"github.com/kandev/kandev/internal/agentctl/server/process"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
"golang.org/x/sync/errgroup"
)
@@ -1099,25 +1099,26 @@ func (s *Server) collectCumulativeDiffForRepo(ctx context.Context, sub string) p
// work, a task with a dozen repositories could exceed the gateway's 60s budget
// and the caller would see a timeout instead of a slow answer.
//
// Concurrency is bounded so a single request cannot occupy the whole shared git
// budget. The process-wide throttle in common/subproc caps subprocesses at 12 by
// default, and it is shared with background workspace polling — an unbounded
// fan-out over a dozen repositories takes all of it, which is what pushed the
// pollers into their own command timeouts.
//
// maxGitFanout is a judgement call rather than a derived number: subproc exposes
// no public accessor for its cap, and the admission design being discussed in
// #2150 is where the real answer belongs. Until then this leaves headroom for
// polling while still parallelising the common case.
const maxGitFanout = 8
// Concurrency is bounded by the process-wide Git admission capacity shared with
// background polling and lifecycle work. The admission controller remains the
// authority on active subprocesses; this limit only bounds goroutines and queued
// repository work created by one request.
func fanOutRepos[T any](ctx context.Context, subpaths []string, collect func(context.Context, string) T) []T {
results := make([]T, len(subpaths))
group, groupCtx := errgroup.WithContext(ctx)
group.SetLimit(maxGitFanout)
var group errgroup.Group
limit := len(subpaths)
if capacity := subproc.GitCapacity(); capacity > 0 && capacity < limit {
limit = capacity
}
group.SetLimit(limit)
for i, sub := range subpaths {
group.Go(func() error {
results[i] = collect(groupCtx, sub)
// collect owns the per-repository result and never returns an
// errgroup error. Preserve the caller's context identity here so
// request cancellation and context-aware singleflight observers see
// the original request boundary rather than an internal wrapper.
results[i] = collect(ctx, sub)
return nil
})
}
@@ -1248,15 +1249,10 @@ func (s *Server) handleGitStatusMulti(c *gin.Context) {
// Parallel fan-out: fresh=true skips the cache, so serial scales linearly and would blow the 2s subscribe timeout for multi-repo workspaces.
result := MultiRepoGitStatusResult{Success: true, Repos: make([]PerRepoGitStatus, len(subpaths))}
ctx := c.Request.Context()
var wg sync.WaitGroup
for i, sub := range subpaths {
wg.Add(1)
go func(i int, sub string) {
defer wg.Done()
result.Repos[i] = s.collectStatusForRepo(ctx, sub, fresh)
}(i, sub)
}
wg.Wait()
outcomes := fanOutRepos(ctx, subpaths, func(ctx context.Context, sub string) PerRepoGitStatus {
return s.collectStatusForRepo(ctx, sub, fresh)
})
copy(result.Repos, outcomes)
c.JSON(http.StatusOK, result)
}
@@ -18,13 +18,14 @@ import (
"github.com/kandev/kandev/internal/agentctl/server/config"
"github.com/kandev/kandev/internal/agentctl/server/process"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
)
// TestFanOutReposRunsConcurrently proves parallelism without measuring how long
// anything takes. Every collect call blocks on a gate that opens only once
// maxGitFanout of them have arrived, so serial execution cannot pass: the first
// would wait on the gate forever and the rest would never start. The timeout is
// a failure detector rather than a threshold, so machine speed does not enter
// anything takes. Every collect call blocks on a gate that opens only after all
// repositories have arrived, so serial execution cannot pass: the first would
// wait on the gate forever and the rest would never start. The timeout is a
// failure detector rather than a threshold, so machine speed does not enter
// into it.
//
// The gate is plain channels rather than the git shim in
@@ -67,12 +68,13 @@ func TestFanOutReposRunsConcurrently(t *testing.T) {
}
}
// TestFanOutReposRespectsLimit pins the bound. With more repositories than
// maxGitFanout, the group must admit exactly maxGitFanout at once — enough to
// parallelise, never enough for one request to take the whole shared git
// budget from the background pollers.
// TestFanOutReposRespectsCapacity pins the bound to the configured process-wide
// Git capacity rather than a private handler constant.
func TestFanOutReposRespectsLimit(t *testing.T) {
subpaths := make([]string, maxGitFanout+4)
const capacity = 3
restore := subproc.Git().SetCapForTest(capacity)
t.Cleanup(restore)
subpaths := make([]string, capacity+4)
for i := range subpaths {
subpaths[i] = fmt.Sprintf("repo%d", i)
}
@@ -98,18 +100,18 @@ func TestFanOutReposRespectsLimit(t *testing.T) {
})
}()
// Reading exactly maxGitFanout admissions blocks until the group is full,
// Reading exactly the configured capacity blocks until the group is full,
// which is the parallelism half. It cannot over-read: the rest are held
// behind SetLimit until we release these.
for i := 0; i < maxGitFanout; i++ {
for i := 0; i < capacity; i++ {
select {
case <-admitted:
case <-time.After(10 * time.Second):
t.Fatalf("only %d of %d workers were admitted; the group is not parallel", i, maxGitFanout)
t.Fatalf("only %d of %d workers were admitted; the group is not parallel", i, capacity)
}
}
if got := atomic.LoadInt64(&peak); got > maxGitFanout {
t.Fatalf("peak concurrency = %d, want at most %d", got, maxGitFanout)
if got := atomic.LoadInt64(&peak); got > capacity {
t.Fatalf("peak concurrency = %d, want at most %d", got, capacity)
}
close(release)
@@ -121,8 +123,8 @@ func TestFanOutReposRespectsLimit(t *testing.T) {
case <-time.After(20 * time.Second):
t.Fatal("fan-out did not drain after the gate opened")
}
if got := atomic.LoadInt64(&peak); got != maxGitFanout {
t.Errorf("peak concurrency = %d, want exactly %d", got, maxGitFanout)
if got := atomic.LoadInt64(&peak); got != capacity {
t.Errorf("peak concurrency = %d, want exactly %d", got, capacity)
}
}
@@ -8,13 +8,13 @@ import (
"net/http"
"net/url"
"os"
"os/exec"
"path/filepath"
"strings"
"unicode"
"github.com/gin-gonic/gin"
"github.com/kandev/kandev/internal/common/securityutil"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
)
@@ -521,9 +521,8 @@ func clearMaterializedQuarantine(root *os.Root) error {
}
func materializeGitOutput(ctx context.Context, args ...string) (string, error) {
// codeql[go/command-injection] git is fixed; HTTP locators and refs are validated before checkout and matching probes.
cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- git is fixed and arguments are validated; no shell.
output, err := cmd.CombinedOutput()
cmd := subproc.NewGitCommand(ctx, args...)
output, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd)
if err != nil {
if ctx.Err() != nil {
return "", ctx.Err()
@@ -7,7 +7,6 @@ import (
"errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"sync"
@@ -167,7 +166,7 @@ func (g *GitOperator) runGitCommand(ctx context.Context, args ...string) (string
// All args validated: flags in securityutil.IsKnownSafeGitFlag whitelist, branch names via securityutil.IsValidBranchName
// regex, commit SHAs via securityutil.LooksLikeCommitSHA pattern, args after "--" separator skipped.
// This defense-in-depth validation prevents injection of arbitrary commands.
cmd := exec.CommandContext(ctx, "git", args...) // lgtm[go/command-injection]
cmd := subproc.NewGitCommand(ctx, args...)
cmd.Dir = g.workDir
cmd.Env = filterGitEnv(g.environmentValues())
@@ -177,7 +176,7 @@ func (g *GitOperator) runGitCommand(ctx context.Context, args ...string) (string
g.logger.Debug("executing git command", zap.Strings("args", args))
err := subproc.RunGit(ctx, cmd)
err := subproc.RunGitClass(ctx, subproc.GitInteractive, cmd)
output := stdout.String()
if stderr.Len() > 0 {
if output != "" {
@@ -416,7 +416,7 @@ func (g *GitOperator) runRepositoryCommand(ctx context.Context, name string, arg
case "gh":
err = subproc.RunGH(ctx, cmd)
case "git":
err = subproc.RunGit(ctx, cmd)
err = subproc.RunGitClass(ctx, subproc.GitInteractive, cmd)
default:
err = cmd.Run()
}
@@ -54,7 +54,7 @@ func isRootOwnershipMarkerPath(path string) bool {
// updateFiles updates the file listing
func (wt *WorkspaceTracker) updateFiles(ctx context.Context) {
files, err := wt.getFileList(ctx)
files, err := wt.getFileListClass(ctx, subproc.GitBackground)
if err != nil {
wt.logger.Debug("failed to get file list", zap.Error(err))
return
@@ -67,6 +67,10 @@ func (wt *WorkspaceTracker) updateFiles(ctx context.Context) {
// getFileList retrieves the list of files in the workspace
func (wt *WorkspaceTracker) getFileList(ctx context.Context) (types.FileListUpdate, error) {
return wt.getFileListClass(ctx, subproc.GitInteractive)
}
func (wt *WorkspaceTracker) getFileListClass(ctx context.Context, class subproc.GitWorkClass) (types.FileListUpdate, error) {
update := types.FileListUpdate{
Timestamp: time.Now(),
RepositoryName: wt.repositoryName,
@@ -77,9 +81,12 @@ func (wt *WorkspaceTracker) getFileList(ctx context.Context) (types.FileListUpda
// --cached: include tracked files
// --others: include untracked files
// --exclude-standard: respect .gitignore
cmd := exec.CommandContext(ctx, "git", "ls-files", "--cached", "--others", "--exclude-standard")
cmd.Dir = wt.workDir
out, err := subproc.RunGitOutput(ctx, cmd)
out, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire(ctx, class, gitCommandTimeout, func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, "ls-files", "--cached", "--others", "--exclude-standard")
cmd.Dir = wt.workDir
return cmd
})
err := gitCommandError(runErr, execCtxErr)
if err != nil {
return update, err
}
@@ -551,10 +558,17 @@ func (wt *WorkspaceTracker) ApplyFileDiff(ctx context.Context, reqPath, unifiedD
}()
// Use git apply to apply the patch directly to the file
cmd := exec.CommandContext(ctx, "git", "apply", "-p0", "--unidiff-zero", "--whitespace=nowarn", patchFile)
cmd.Dir = wt.workDir
output, err := subproc.RunGitCombinedOutput(ctx, cmd)
output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire(
ctx,
subproc.GitInteractive,
gitCommandTimeout,
func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, "apply", "-p0", "--unidiff-zero", "--whitespace=nowarn", patchFile)
cmd.Dir = wt.workDir
return cmd
},
)
err = gitCommandError(runErr, execCtxErr)
if err != nil {
// Treat caller cancellation / deadline as a transient failure and
// propagate rather than overwriting the file from desiredContent —
@@ -905,10 +919,18 @@ func (wt *WorkspaceTracker) GetFileContentAtRef(ctx context.Context, reqPath str
// Preflight: check blob size before materializing content to avoid memory spikes.
// Use CombinedOutput to capture stderr for error detection (git writes errors to stderr).
// Set LC_ALL=C to ensure English error messages for reliable parsing.
sizeCmd := exec.CommandContext(ctx, "git", "cat-file", "-s", gitRef)
sizeCmd.Dir = wt.workDir
sizeCmd.Env = append(os.Environ(), "LC_ALL=C")
sizeOut, err := subproc.RunGitCombinedOutput(ctx, sizeCmd)
sizeOut, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire(
ctx,
subproc.GitInteractive,
gitCommandTimeout,
func(execCtx context.Context) *exec.Cmd {
sizeCmd := subproc.NewGitCommand(execCtx, "cat-file", "-s", gitRef)
sizeCmd.Dir = wt.workDir
sizeCmd.Env = append(os.Environ(), "LC_ALL=C")
return sizeCmd
},
)
err := gitCommandError(runErr, execCtxErr)
if err != nil {
output := string(sizeOut)
if strings.Contains(output, "does not exist") ||
@@ -927,10 +949,17 @@ func (wt *WorkspaceTracker) GetFileContentAtRef(ctx context.Context, reqPath str
return "", blobSize, false, fmt.Errorf("file too large (max 10MB)")
}
cmd := exec.CommandContext(ctx, "git", "show", gitRef)
cmd.Dir = wt.workDir
content, err := subproc.RunGitOutput(ctx, cmd)
content, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire(
ctx,
subproc.GitInteractive,
gitCommandTimeout,
func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, "show", gitRef)
cmd.Dir = wt.workDir
return cmd
},
)
err = gitCommandError(runErr, execCtxErr)
if err != nil {
return "", 0, false, fmt.Errorf("failed to get file at ref: %w", err)
}
@@ -1,13 +1,28 @@
package process
import (
"bytes"
"context"
"os"
"os/exec"
"strings"
"github.com/kandev/kandev/internal/common/subproc"
)
type gitWorkClassContextKey struct{}
func withGitWorkClass(ctx context.Context, class subproc.GitWorkClass) context.Context {
return context.WithValue(ctx, gitWorkClassContextKey{}, class)
}
func gitWorkClass(ctx context.Context) subproc.GitWorkClass {
if class, ok := ctx.Value(gitWorkClassContextKey{}).(subproc.GitWorkClass); ok {
return class
}
return subproc.GitInteractive
}
// gitOptionalLocksOff is the env var git reads to skip "optional" locks, i.e.
// the index refresh lock that `git status` and friends take to update stat
// info. The workspace tracker polls git read-only, but without this flag even
@@ -19,53 +34,115 @@ import (
// See: https://git-scm.com/docs/git#Documentation/git.txt-codeGITOPTIONALLOCKSltbooleangtcode
const gitOptionalLocksOff = "GIT_OPTIONAL_LOCKS=0"
// pollingGitCommand builds an exec.Cmd for a polling git invocation. It sets
// the workspace directory and disables optional locks so the background poll
// loop doesn't contend with user-initiated git operations.
// pollingGitCommand builds an exec.Cmd with optional Git locks disabled. The
// lock policy is independent from the admission class: fresh interactive
// status also needs lockless reads while still using the interactive queue.
func (wt *WorkspaceTracker) pollingGitCommand(ctx context.Context, args ...string) *exec.Cmd {
cmd := exec.CommandContext(ctx, "git", args...)
cmd := subproc.NewGitCommand(ctx, args...)
cmd.Dir = wt.workDir
cmd.Env = append(os.Environ(), gitOptionalLocksOff)
return cmd
}
// gitCmdContext builds a per-command timeout-bounded ctx and an exec.Cmd
// for a git invocation. When polling is true, the cmd is built via
// pollingGitCommand (which sets GIT_OPTIONAL_LOCKS=0) so the background
// poll loop doesn't contend with user-initiated git operations on the
// index lock. The returned cancel must be deferred by the caller to
// release the timer once the subprocess exits.
func (wt *WorkspaceTracker) gitCmdContext(ctx context.Context, polling bool, args ...string) (context.Context, context.CancelFunc, *exec.Cmd) {
cctx, cancel := context.WithTimeout(ctx, gitCommandTimeout)
if polling {
return cctx, cancel, wt.pollingGitCommand(cctx, args...)
// gitCommand builds a git command for the already-admitted execution context.
// The timeout is deliberately owned by subproc.RunGit*AfterAcquire so queue
// time cannot consume the command's execution budget.
func (wt *WorkspaceTracker) gitCommand(ctx context.Context, lockless bool, args ...string) *exec.Cmd {
if lockless {
return wt.pollingGitCommand(ctx, args...)
}
cmd := exec.CommandContext(cctx, "git", args...)
cmd := subproc.NewGitCommand(ctx, args...)
cmd.Dir = wt.workDir
return cctx, cancel, cmd
return cmd
}
// runGitOutput runs a git command with a per-command timeout and returns its
// stdout. The derived ctx ensures cancellation SIGKILLs the subprocess and
// releases its throttle slot even when the outer poll ctx is long-lived.
// runGitOutput runs a class-selected git command with a per-command timeout
// and returns its stdout. Background contexts also disable optional Git locks.
func (wt *WorkspaceTracker) runGitOutput(ctx context.Context, args ...string) ([]byte, error) {
cctx, cancel, cmd := wt.gitCmdContext(ctx, false, args...)
defer cancel()
return subproc.RunGitOutput(cctx, cmd)
class := gitWorkClass(ctx)
return wt.runGitOutputClass(ctx, class, class == subproc.GitBackground, args...)
}
// runGitOutputClass runs a Git command under the supplied admission class and
// independently chooses whether Git's optional locks are disabled. Keeping
// these concerns separate prevents a fresh interactive status from being
// accidentally scheduled on the background FIFO just because it is read-only.
func (wt *WorkspaceTracker) runGitOutputClass(
ctx context.Context,
class subproc.GitWorkClass,
lockless bool,
args ...string,
) ([]byte, error) {
out, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire(
ctx,
class,
gitCommandTimeout,
func(execCtx context.Context) *exec.Cmd {
return wt.gitCommand(execCtx, lockless, args...)
},
)
return out, gitCommandError(runErr, execCtxErr)
}
// runGit is runGitOutput's no-stdout sibling for verify-style probes where
// only the exit code matters.
func (wt *WorkspaceTracker) runGit(ctx context.Context, args ...string) error {
cctx, cancel, cmd := wt.gitCmdContext(ctx, false, args...)
defer cancel()
return subproc.RunGit(cctx, cmd)
class := gitWorkClass(ctx)
runErr, execCtxErr := subproc.RunGitAfterAcquire(
ctx,
class,
gitCommandTimeout,
func(execCtx context.Context) *exec.Cmd {
return wt.gitCommand(execCtx, class == subproc.GitBackground, args...)
},
)
return gitCommandError(runErr, execCtxErr)
}
// runPollingGitOutput is runGitOutput's polling sibling — see
// gitCmdContext for the GIT_OPTIONAL_LOCKS rationale.
// runPollingGitOutput is runGitOutput's polling sibling. It always uses the
// background class and builds the command with GIT_OPTIONAL_LOCKS=0.
func (wt *WorkspaceTracker) runPollingGitOutput(ctx context.Context, args ...string) ([]byte, error) {
cctx, cancel, cmd := wt.gitCmdContext(ctx, true, args...)
defer cancel()
return subproc.RunGitOutput(cctx, cmd)
out, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire(
ctx,
subproc.GitBackground,
gitCommandTimeout,
func(execCtx context.Context) *exec.Cmd {
return wt.gitCommand(execCtx, true, args...)
},
)
return out, gitCommandError(runErr, execCtxErr)
}
func (wt *WorkspaceTracker) runPollingGitOutputWithStderr(ctx context.Context, args ...string) ([]byte, string, error) {
var stderr bytes.Buffer
out, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire(
ctx,
subproc.GitBackground,
gitCommandTimeout,
func(execCtx context.Context) *exec.Cmd {
cmd := wt.gitCommand(execCtx, true, args...)
cmd.Stderr = &stderr
return cmd
},
)
return out, strings.TrimSpace(stderr.String()), gitCommandError(runErr, execCtxErr)
}
func (wt *WorkspaceTracker) runPollingGit(ctx context.Context, args ...string) error {
runErr, execCtxErr := subproc.RunGitAfterAcquire(
ctx,
subproc.GitBackground,
gitCommandTimeout,
func(execCtx context.Context) *exec.Cmd {
return wt.gitCommand(execCtx, true, args...)
},
)
return gitCommandError(runErr, execCtxErr)
}
func gitCommandError(runErr, execCtxErr error) error {
if runErr != nil {
return runErr
}
return execCtxErr
}
@@ -104,6 +104,49 @@ func TestRunGitOutput_PollingVariantReleasesThrottleOnTimeout(t *testing.T) {
}
}
func TestWorkspaceGitAdmissionWaitDoesNotConsumeTimeout(t *testing.T) {
prev := gitCommandTimeout
gitCommandTimeout = 100 * time.Millisecond
t.Cleanup(func() { gitCommandTimeout = prev })
restoreCap := subproc.Git().SetCapForTest(1)
t.Cleanup(restoreCap)
shimDir := installFastGitShim(t)
t.Setenv("PATH", shimDir+string(os.PathListSeparator)+os.Getenv("PATH"))
_, wt := setupTestDir(t)
hold, err := subproc.AcquireGit(context.Background(), subproc.GitInteractive)
if err != nil {
t.Fatalf("hold git slot: %v", err)
}
resultCh := make(chan struct {
out []byte
err error
}, 1)
go func() {
out, runErr := wt.runGitOutput(context.Background(), "status")
resultCh <- struct {
out []byte
err error
}{out: out, err: runErr}
}()
waitForAnyGitWaiter(t, 1)
// Hold the slot longer than the execution timeout. The queued command
// should still receive a fresh 100ms execution budget after release.
timer := time.NewTimer(150 * time.Millisecond)
<-timer.C
hold()
select {
case result := <-resultCh:
if result.err != nil || string(result.out) != "ok" {
t.Fatalf("result = (%q, %v), want fast command after queued admission", result.out, result.err)
}
case <-time.After(time.Second):
t.Fatal("queued git command did not complete")
}
}
// installSleepGitShim writes an executable shell script named 'git' into a
// fresh tmpdir that blocks for an hour, and returns the dir. Callers must
// prepend it to $PATH so exec.Command("git", ...) picks it up before the
@@ -125,3 +168,29 @@ func installSleepGitShim(t *testing.T) string {
}
return dir
}
func installFastGitShim(t *testing.T) string {
t.Helper()
dir := t.TempDir()
shim := filepath.Join(dir, "git")
if err := os.WriteFile(shim, []byte("#!/bin/sh\nprintf ok\n"), 0o755); err != nil {
t.Fatalf("write fast git shim: %v", err)
}
return dir
}
func waitForAnyGitWaiter(t *testing.T, want int) {
t.Helper()
deadline := time.Now().Add(time.Second)
for time.Now().Before(deadline) {
snapshot := subproc.AdmissionSnapshot()
total := 0
for _, class := range snapshot.Classes {
total += int(class.Waiters)
}
if total == want {
return
}
}
t.Fatalf("total git waiters did not reach %d", want)
}
@@ -2,12 +2,10 @@ package process
import (
"context"
"os/exec"
"strings"
"time"
"github.com/kandev/kandev/internal/agentctl/types"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
)
@@ -16,12 +14,12 @@ import (
func (wt *WorkspaceTracker) getCommitsSince(ctx context.Context, baseCommit string) []*types.GitCommitNotification {
// Record separator (\x1e) placed BEFORE fields so --shortstat output stays
// within the same record: \x1e<fields>\n <stat summary>\n\x1e<fields>\n...
cmd := exec.CommandContext(ctx, "git", "log",
out, err := wt.runPollingGitOutput(ctx,
"log",
"--format=\x1e%H\x1f%P\x1f%an\x1f%ae\x1f%s\x1f%aI",
"--shortstat",
baseCommit+"..HEAD")
cmd.Dir = wt.workDir
out, err := subproc.RunGitOutput(ctx, cmd)
baseCommit+"..HEAD",
)
if err != nil {
wt.logger.Debug("failed to get commits since base",
zap.String("base", baseCommit),
@@ -5,7 +5,6 @@ import (
"context"
"io"
"os"
"os/exec"
"path/filepath"
"regexp"
"strconv"
@@ -280,24 +279,20 @@ func carryForwardFileDiff(fi types.FileInfo, filePath string, update *types.GitS
// pipe-based reads. Slot is acquired before Start; if Start fails we
// release immediately, else release runs after Wait.
func capDiffOutput(ctx context.Context, workDir string, args ...string) (string, bool) {
cctx, cancel := context.WithTimeout(ctx, gitCommandTimeout)
release, err := subproc.AcquireGit(ctx, gitWorkClass(ctx))
if err != nil {
return "", false
}
defer release()
execCtx, cancel := context.WithTimeout(ctx, gitCommandTimeout)
defer cancel()
cmd := exec.CommandContext(cctx, "git", args...)
cmd := subproc.NewGitCommand(execCtx, args...)
cmd.Dir = workDir
stdout, err := cmd.StdoutPipe()
if err != nil {
return "", false
}
release, err := subproc.Git().Acquire(cctx)
if err != nil {
_ = stdout.Close()
return "", false
}
// release() is idempotent (sync.Once inside the throttle), so a defer
// covers every return path — including ones added later between Start
// and the explicit end-of-function release.
defer release()
if err := cmd.Start(); err != nil {
_ = stdout.Close()
return "", false
@@ -4,7 +4,7 @@ import (
"context"
"crypto/sha256"
"encoding/hex"
"os/exec"
"errors"
"strings"
"sync/atomic"
"time"
@@ -139,9 +139,23 @@ func (wt *WorkspaceTracker) gitPollTick(ctx context.Context, consecutiveFailures
//
// Returns true if pollGitChanges should exit.
func (wt *WorkspaceTracker) handleGitPollFailure(ctx context.Context, consecutiveFailures *int, cause error) bool {
probeCmd := exec.CommandContext(ctx, "git", "rev-parse", "--git-dir")
probeCmd.Dir = wt.workDir
if probeErr := subproc.RunGit(ctx, probeCmd); probeErr == nil {
if errors.Is(cause, subproc.ErrAdmissionCanceled) {
wt.logger.Debug("git poll admission canceled; preserving tracker liveness",
zap.String("workDir", wt.workDir),
zap.Error(cause))
*consecutiveFailures = 0
return false
}
probeErr := wt.runPollingGit(ctx, "rev-parse", "--git-dir")
if errors.Is(probeErr, subproc.ErrAdmissionCanceled) {
wt.logger.Debug("git poll health probe admission canceled; preserving tracker liveness",
zap.String("workDir", wt.workDir),
zap.Error(probeErr))
*consecutiveFailures = 0
return false
}
if probeErr == nil {
// The repository is fine, so the snapshot read failed for some other
// reason (per-command timeout under load, a concurrent index.lock
// holder). Same as before: only probe failures count against the
@@ -523,14 +537,12 @@ func (wt *WorkspaceTracker) getBaseCommitForBranch(ctx context.Context, branch,
// Try integration branch candidates first (origin/main, origin/master, main, master)
// This ensures we get the branch-off point from the main development line.
// Routed through subproc.RunGit so each rev-parse counts against the global
// Routed through the background admission helpers so each rev-parse counts against the global
// git semaphore — handleBranchSwitch can invoke this on every detected
// branch change, so an unthrottled burst here was the exact pattern the
// throttle is meant to prevent on CrowdStrike-instrumented macOS.
for _, candidate := range []string{"origin/main", "origin/master", "main", "master"} {
checkCmd := exec.CommandContext(ctx, "git", "rev-parse", "--verify", candidate)
checkCmd.Dir = wt.workDir
if err := subproc.RunGit(ctx, checkCmd); err == nil {
if err := wt.runPollingGit(ctx, "rev-parse", "--verify", candidate); err == nil {
baseBranch = candidate
break
}
@@ -538,9 +550,7 @@ func (wt *WorkspaceTracker) getBaseCommitForBranch(ctx context.Context, branch,
// Fall back to upstream tracking branch if no integration branch exists
if baseBranch == "" {
upstreamCmd := exec.CommandContext(ctx, "git", "rev-parse", "--abbrev-ref", branch+"@{upstream}")
upstreamCmd.Dir = wt.workDir
upstreamOut, err := subproc.RunGitOutput(ctx, upstreamCmd)
upstreamOut, err := wt.runPollingGitOutput(ctx, "rev-parse", "--abbrev-ref", branch+"@{upstream}")
if err == nil && len(upstreamOut) > 0 {
baseBranch = strings.TrimSpace(string(upstreamOut))
}
@@ -549,9 +559,7 @@ func (wt *WorkspaceTracker) getBaseCommitForBranch(ctx context.Context, branch,
// Calculate merge-base if we found a base branch
// Use the sampled head SHA instead of live HEAD to avoid race conditions
if baseBranch != "" && head != "" {
mergeBaseCmd := exec.CommandContext(ctx, "git", "merge-base", baseBranch, head)
mergeBaseCmd.Dir = wt.workDir
if mergeBaseOut, err := subproc.RunGitOutput(ctx, mergeBaseCmd); err == nil {
if mergeBaseOut, err := wt.runPollingGitOutput(ctx, "merge-base", baseBranch, head); err == nil {
return strings.TrimSpace(string(mergeBaseOut))
}
}
@@ -561,9 +569,7 @@ func (wt *WorkspaceTracker) getBaseCommitForBranch(ctx context.Context, branch,
// isAncestor checks if commit1 is an ancestor of commit2
func (wt *WorkspaceTracker) isAncestor(ctx context.Context, commit1, commit2 string) bool {
cmd := exec.CommandContext(ctx, "git", "merge-base", "--is-ancestor", commit1, commit2)
cmd.Dir = wt.workDir
err := subproc.RunGit(ctx, cmd)
err := wt.runPollingGit(ctx, "merge-base", "--is-ancestor", commit1, commit2)
// Exit code 0 means commit1 IS an ancestor of commit2
// Exit code 1 means commit1 is NOT an ancestor of commit2
return err == nil
@@ -574,9 +580,7 @@ func (wt *WorkspaceTracker) isAncestor(ctx context.Context, commit1, commit2 str
// as opposed to commits made locally in the session.
func (wt *WorkspaceTracker) isOnRemote(ctx context.Context, commitSHA string) bool {
// Use git branch -r --contains to check if commit is on any remote branch
cmd := exec.CommandContext(ctx, "git", "branch", "-r", "--contains", commitSHA)
cmd.Dir = wt.workDir
out, err := subproc.RunGitOutput(ctx, cmd)
out, err := wt.runPollingGitOutput(ctx, "branch", "-r", "--contains", commitSHA)
if err != nil {
// If the command fails, assume it's not on remote (safer default)
return false
@@ -0,0 +1,61 @@
package process
import (
"context"
"fmt"
"runtime"
"testing"
"time"
"github.com/kandev/kandev/internal/common/subproc"
)
func TestHandleGitPollFailureAdmissionCancellationDoesNotStopTracker(t *testing.T) {
wt := &WorkspaceTracker{logger: newTestLogger(t), workDir: t.TempDir()}
consecutiveFailures := 4
cause := fmt.Errorf("queued poll: %w: %w", subproc.ErrAdmissionCanceled, context.DeadlineExceeded)
if stop := wt.handleGitPollFailure(context.Background(), &consecutiveFailures, cause); stop {
t.Fatal("admission cancellation stopped a healthy tracker")
}
if consecutiveFailures != 0 {
t.Fatalf("consecutiveFailures = %d, want reset to 0 for admission cancellation", consecutiveFailures)
}
}
func TestHandleGitPollFailureProbeAdmissionCancellationDoesNotStopTracker(t *testing.T) {
restore := subproc.Git().SetCapForTest(1)
t.Cleanup(restore)
hold, err := subproc.AcquireGit(context.Background(), subproc.GitInteractive)
if err != nil {
t.Fatalf("hold Git slot: %v", err)
}
wt := &WorkspaceTracker{logger: newTestLogger(t), workDir: t.TempDir()}
consecutiveFailures := 4
cause := fmt.Errorf("snapshot failed")
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
defer cancel()
done := make(chan bool, 1)
go func() {
done <- wt.handleGitPollFailure(ctx, &consecutiveFailures, cause)
}()
deadline := time.Now().Add(time.Second)
for time.Now().Before(deadline) {
if got := subproc.AdmissionSnapshot().Classes[string(subproc.GitBackground)].Waiters; got == 1 {
break
}
runtime.Gosched()
}
if got := subproc.AdmissionSnapshot().Classes[string(subproc.GitBackground)].Waiters; got != 1 {
t.Fatalf("background waiters = %d, want 1", got)
}
stop := <-done
hold()
if stop {
t.Fatal("probe admission cancellation stopped a healthy tracker")
}
if consecutiveFailures != 0 {
t.Fatalf("consecutiveFailures = %d, want reset to 0 for probe admission cancellation", consecutiveFailures)
}
}
@@ -9,6 +9,7 @@ import (
"time"
"github.com/kandev/kandev/internal/agentctl/types"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
)
@@ -28,7 +29,11 @@ var safeBranchRefPattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._/-]*$`)
// cancelled, so callers that must not lose the event they're reacting to
// (see tryUpdateGitStatus) know a retry is needed.
func (wt *WorkspaceTracker) updateGitStatus(ctx context.Context) bool {
status, err := wt.getGitStatus(ctx)
return wt.updateGitStatusClass(ctx, gitWorkClass(ctx))
}
func (wt *WorkspaceTracker) updateGitStatusClass(ctx context.Context, class subproc.GitWorkClass) bool {
status, err := wt.getGitStatusClass(ctx, class)
if err != nil {
wt.logger.Warn("updateGitStatus: getGitStatus failed", zap.Error(err))
return false
@@ -59,7 +64,7 @@ func (wt *WorkspaceTracker) tryUpdateGitStatus(ctx context.Context) bool {
return false
}
defer wt.updateMu.Unlock()
return wt.updateGitStatus(ctx)
return wt.updateGitStatusClass(ctx, subproc.GitBackground)
}
// RefreshGitStatus forces a git status refresh and notifies subscribers.
@@ -68,7 +73,7 @@ func (wt *WorkspaceTracker) tryUpdateGitStatus(ctx context.Context) bool {
func (wt *WorkspaceTracker) RefreshGitStatus(ctx context.Context) {
wt.updateMu.Lock()
defer wt.updateMu.Unlock()
wt.updateGitStatus(ctx)
wt.updateGitStatusClass(ctx, subproc.GitInteractive)
}
// GetCurrentGitStatus returns the current cached git status. If no status has
@@ -84,7 +89,7 @@ func (wt *WorkspaceTracker) GetCurrentGitStatus(ctx context.Context) (types.GitS
// fresh=true.
func (wt *WorkspaceTracker) GetGitStatus(ctx context.Context, fresh bool) (types.GitStatusUpdate, error) {
if fresh {
return wt.getGitStatus(ctx)
return wt.getGitStatusClass(ctx, subproc.GitInteractive)
}
wt.mu.RLock()
@@ -92,7 +97,7 @@ func (wt *WorkspaceTracker) GetGitStatus(ctx context.Context, fresh bool) (types
wt.mu.RUnlock()
if status.Timestamp.IsZero() {
return wt.getGitStatus(ctx)
return wt.getGitStatusClass(ctx, subproc.GitInteractive)
}
return status, nil
@@ -102,11 +107,19 @@ func (wt *WorkspaceTracker) GetGitStatus(ctx context.Context, fresh bool) (types
// computation is owned by the tracker rather than the first caller, while each
// waiter can still return promptly when its own context is canceled.
func (wt *WorkspaceTracker) getGitStatus(ctx context.Context) (types.GitStatusUpdate, error) {
return wt.getGitStatusClass(ctx, gitWorkClass(ctx))
}
func (wt *WorkspaceTracker) getGitStatusClass(ctx context.Context, class subproc.GitWorkClass) (types.GitStatusUpdate, error) {
if err := ctx.Err(); err != nil {
return types.GitStatusUpdate{}, err
}
resultCh := wt.gitStatusGroup.DoChan("live", func() (interface{}, error) {
// Observations are coalesced only within the same admission class. A
// background poll already in flight must not capture a fresh interactive
// request and make its Git commands run on the background queue.
key := "live:" + string(class)
resultCh := wt.gitStatusGroup.DoChan(key, func() (interface{}, error) {
sharedCtx, finish, err := wt.beginGitStatusObservation()
if err != nil {
return types.GitStatusUpdate{}, err
@@ -117,7 +130,7 @@ func (wt *WorkspaceTracker) getGitStatus(ctx context.Context) (types.GitStatusUp
if observer == nil {
observer = wt.computeGitStatus
}
status, err := observer(sharedCtx)
status, err := observer(withGitWorkClass(sharedCtx, class))
if err != nil {
return types.GitStatusUpdate{}, err
}
@@ -561,10 +574,16 @@ func carryRemoteAheadBehind(update *types.GitStatusUpdate, prior types.GitStatus
// parseGitStatusOutput runs git status --porcelain and populates the file lists and map.
func (wt *WorkspaceTracker) parseGitStatusOutput(ctx context.Context, update *types.GitStatusUpdate) error {
// --untracked-files=all shows all files in untracked directories, not just the directory name.
// GIT_OPTIONAL_LOCKS=0 (via runPollingGitOutput) prevents the background poll loop from
// taking .git/index.lock, which would race with concurrent user-initiated git operations.
statusOut, err := wt.runPollingGitOutput(ctx, "status", "--porcelain", "--untracked-files=all")
// --untracked-files=all shows all files in untracked directories, not just
// the directory name. GIT_OPTIONAL_LOCKS=0 prevents the status read from
// taking .git/index.lock, while the carried observation class keeps fresh
// user requests interactive.
statusOut, err := wt.runGitOutputClass(
ctx,
gitWorkClass(ctx),
true,
"status", "--porcelain", "--untracked-files=all",
)
if err != nil {
return err
}
@@ -9,6 +9,7 @@ import (
"time"
"github.com/kandev/kandev/internal/agentctl/types"
"github.com/kandev/kandev/internal/common/subproc"
)
func TestWorkspaceTrackerConcurrentFreshStatusSharesObservation(t *testing.T) {
@@ -133,7 +134,7 @@ func TestWorkspaceTrackerCanceledCallerStartsNoObservation(t *testing.T) {
t.Fatalf("GetGitStatus error = %v, want context.Canceled", err)
}
var probeCalls atomic.Int32
_, _, _ = wt.gitStatusGroup.Do("live", func() (interface{}, error) {
_, _, _ = wt.gitStatusGroup.Do("live:"+string(subproc.GitInteractive), func() (interface{}, error) {
probeCalls.Add(1)
return types.GitStatusUpdate{}, nil
})
@@ -145,6 +146,58 @@ func TestWorkspaceTrackerCanceledCallerStartsNoObservation(t *testing.T) {
}
}
func TestWorkspaceTrackerFreshStatusDoesNotJoinBackgroundObservation(t *testing.T) {
wt := newStatusConcurrencyTracker(t)
started := make(chan subproc.GitWorkClass, 2)
release := make(chan struct{})
var observations atomic.Int32
wt.gitStatusObserver = func(ctx context.Context) (types.GitStatusUpdate, error) {
observations.Add(1)
started <- gitWorkClass(ctx)
<-release
return types.GitStatusUpdate{Timestamp: time.Unix(987, 0)}, nil
}
backgroundDone := make(chan error, 1)
go func() {
_, err := wt.getGitStatusClass(context.Background(), subproc.GitBackground)
backgroundDone <- err
}()
select {
case got := <-started:
if got != subproc.GitBackground {
t.Fatalf("background observation class = %s, want %s", got, subproc.GitBackground)
}
case <-time.After(time.Second):
t.Fatal("background observation did not start")
}
interactiveDone := make(chan error, 1)
go func() {
_, err := wt.GetGitStatus(context.Background(), true)
interactiveDone <- err
}()
select {
case got := <-started:
if got != subproc.GitInteractive {
t.Fatalf("fresh observation class = %s, want %s", got, subproc.GitInteractive)
}
case <-time.After(time.Second):
t.Fatal("fresh interactive observation joined background flight")
}
close(release)
if err := <-backgroundDone; err != nil {
t.Fatalf("background observation failed: %v", err)
}
if err := <-interactiveDone; err != nil {
t.Fatalf("fresh observation failed: %v", err)
}
if got := observations.Load(); got != 2 {
t.Fatalf("observations = %d, want separate background and interactive flights", got)
}
}
func TestWorkspaceTrackerRefreshAndEmptyCacheReadShareObservation(t *testing.T) {
wt := newStatusConcurrencyTracker(t)
started := make(chan struct{})
@@ -7,6 +7,7 @@ import (
"testing"
"github.com/kandev/kandev/internal/agentctl/types"
"github.com/kandev/kandev/internal/common/subproc"
)
func TestUnquoteGitPath(t *testing.T) {
@@ -222,6 +223,29 @@ func TestGetGitStatus_FreshBypassesStaleCache(t *testing.T) {
}
}
func TestGetGitStatusFreshUsesInteractiveAdmissionForEveryCommand(t *testing.T) {
repoDir, cleanup := setupTestRepo(t)
defer cleanup()
wt := NewWorkspaceTracker(repoDir, newTestLogger(t))
t.Cleanup(wt.Stop)
before := subproc.AdmissionSnapshot()
if _, err := wt.GetGitStatus(context.Background(), true); err != nil {
t.Fatalf("fresh GetGitStatus failed: %v", err)
}
after := subproc.AdmissionSnapshot()
interactive := after.Classes[string(subproc.GitInteractive)].AcquireTotal -
before.Classes[string(subproc.GitInteractive)].AcquireTotal
background := after.Classes[string(subproc.GitBackground)].AcquireTotal -
before.Classes[string(subproc.GitBackground)].AcquireTotal
if interactive == 0 {
t.Fatal("fresh status did not admit any interactive Git commands")
}
if background != 0 {
t.Fatalf("fresh status admitted %d background Git commands, want 0", background)
}
}
// mapKeys returns the keys of a map for diagnostic output.
func mapKeys[V any](m map[string]V) []string {
keys := make([]string, 0, len(m))
@@ -1,7 +1,6 @@
package process
import (
"bytes"
"context"
"fmt"
"os"
@@ -80,7 +79,7 @@ func (wt *WorkspaceTracker) monitorLoop(ctx context.Context) {
// stuck waiting for the first tick. Skipped only in fully-paused initial
// state? No: even paused workspaces benefit from a one-time scan so
// transitioning to slow/fast later finds a populated cache.
wt.updateGitStatus(ctx)
wt.updateGitStatusClass(ctx, subproc.GitBackground)
wt.updateFiles(ctx)
// Cache the last known state (ignore error on initial fetch)
@@ -230,28 +229,21 @@ func (wt *WorkspaceTracker) getWorkspaceState(ctx context.Context) (workspaceSta
}
}
// Create a timeout context for git commands to prevent hanging
gitCtx, cancel := context.WithTimeout(ctx, gitCommandTimeout)
defer cancel()
// git diff-files shows changed files with their blob hashes.
// However, the output doesn't change when an already-dirty file is modified
// again because the index blob hash stays constant and the worktree hash
// is always shown as 0000000 (not computed). To detect subsequent changes
// to dirty files, we also include the mtime of each dirty file.
cmd := wt.pollingGitCommand(gitCtx, "diff-files", "--name-only")
var stderrBuf bytes.Buffer
cmd.Stderr = &stderrBuf
out, err := subproc.RunGitOutput(gitCtx, cmd)
out, stderr, err := wt.runPollingGitOutputWithStderr(ctx, "diff-files", "--name-only")
if err != nil {
return state, fmt.Errorf("git diff-files in %s: %w (stderr: %s)",
wt.workDir, err, strings.TrimSpace(stderrBuf.String()))
wt.workDir, err, stderr)
}
state.diffFilesID = wt.buildDirtyFilesID(string(out))
// Check untracked files - git diff-files doesn't include them
// Use git ls-files to get untracked files, then check their mtimes
untrackedID, err := wt.getUntrackedFilesID(gitCtx)
untrackedID, err := wt.getUntrackedFilesID(ctx)
if err != nil {
return state, err
}
@@ -292,13 +284,10 @@ func (wt *WorkspaceTracker) buildDirtyFilesID(diffFilesOutput string) string {
// Returns an error if the git command fails (e.g., timeout, index lock).
func (wt *WorkspaceTracker) getUntrackedFilesID(ctx context.Context) (string, error) {
// Get list of untracked files (excluding ignored)
cmd := wt.pollingGitCommand(ctx, "ls-files", "--others", "--exclude-standard")
var stderrBuf bytes.Buffer
cmd.Stderr = &stderrBuf
out, err := subproc.RunGitOutput(ctx, cmd)
out, stderr, err := wt.runPollingGitOutputWithStderr(ctx, "ls-files", "--others", "--exclude-standard")
if err != nil {
return "", fmt.Errorf("git ls-files in %s: %w (stderr: %s)",
wt.workDir, err, strings.TrimSpace(stderrBuf.String()))
wt.workDir, err, stderr)
}
if len(out) == 0 {
return "", nil // No untracked files is not an error
@@ -4,6 +4,7 @@ import (
"time"
"github.com/kandev/kandev/internal/agentctl/types"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
)
@@ -55,7 +56,7 @@ func (wt *WorkspaceTracker) AttachWorkspaceStreamSubscriber(sub types.WorkspaceS
// Use the tracker's cancellable context (not context.Background) so
// Stop() can kill an in-flight `git status` here without waiting it out.
if wt.updateMu.TryLock() {
if status, err := wt.getGitStatus(wt.cancelCtx); err == nil {
if status, err := wt.getGitStatusClass(wt.cancelCtx, subproc.GitInteractive); err == nil {
wt.mu.Lock()
wt.currentStatus = status
wt.mu.Unlock()
@@ -329,24 +329,23 @@ func resolveGitIndexPath(workDir string) string {
if !workDirHasOwnGitEntry(workDir) {
return ""
}
// One-shot probe at workspace setup. Acquire the throttle slot first
// (30s budget), then start the 5s exec timer — otherwise a busy git
// pool would burn through the exec budget queueing and return "",
// which the caller treats as "not a git repo" and permanently
// disables polling for the workspace.
// One-shot probe at workspace setup. The after-admission helper starts the
// 5s exec timer only after the lifecycle slot is granted; otherwise a busy
// git pool would burn through the exec budget queueing and return "", which
// the caller treats as "not a git repo" and permanently disables polling.
acquireCtx, cancelAcquire := context.WithTimeout(context.Background(), 30*time.Second)
defer cancelAcquire()
release, err := subproc.Git().Acquire(acquireCtx)
if err != nil {
return ""
}
defer release()
execCtx, cancelExec := context.WithTimeout(context.Background(), 5*time.Second)
defer cancelExec()
cmd := exec.CommandContext(execCtx, "git", "rev-parse", "--git-dir")
cmd.Dir = workDir
out, err := cmd.Output()
if err != nil {
out, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire(
acquireCtx,
subproc.GitLifecycle,
5*time.Second,
func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, "rev-parse", "--git-dir")
cmd.Dir = workDir
return cmd
},
)
if runErr != nil || execCtxErr != nil {
return ""
}
gitDir := strings.TrimSpace(string(out))
@@ -66,6 +66,7 @@ func TestRegisterMCPAndDebugRoutesWiresCoordinatorTaskStopper(t *testing.T) {
lifecycleMgr: lifecycleMgr,
eventBus: eventBus,
services: &Services{Workflow: harness.workflowSvc},
devMode: true,
log: log,
addCleanup: func(cleanup func() error) {
registerMCPStopTestCleanup(t, "MCP server", cleanup)
+31 -20
View File
@@ -13,6 +13,7 @@ import (
"github.com/kandev/kandev/internal/common/config"
"github.com/kandev/kandev/internal/common/constants"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/db"
"github.com/kandev/kandev/internal/task/models"
taskservice "github.com/kandev/kandev/internal/task/service"
@@ -158,32 +159,35 @@ func (a *environmentDestroyerAdapter) PushEnvironmentBranch(ctx context.Context,
if env.WorktreePath == "" {
return fmt.Errorf("push-before-reset is not supported for this environment type; please push manually first")
}
pushCtx, cancel := context.WithTimeout(ctx, pushBranchTimeout)
defer cancel()
branch := strings.TrimSpace(env.WorktreeBranch)
var cmd *exec.Cmd
var args []string
if branch == "" {
cmd = exec.CommandContext(pushCtx, "git", "push")
args = []string{"push"}
} else {
// Prefer the branch's configured upstream remote so this works for
// repos whose primary remote isn't called "origin" (e.g. fork
// workflows with "upstream"/"github"). Fall back to "origin" only
// when no upstream is set, matching the historical behaviour.
remote := detectBranchRemote(pushCtx, env.WorktreePath, branch)
cmd = exec.CommandContext(pushCtx, "git", "push", remote, branch)
remote := detectBranchRemote(ctx, env.WorktreePath, branch)
args = []string{"push", remote, branch}
}
cmd.Dir = env.WorktreePath
// Disable interactive credential prompts — without this, a missing
// credential helper can hang waiting on stdin even with the timeout above
// (signal delivery is gated behind the prompt read).
cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0")
out, err := cmd.CombinedOutput()
if err != nil {
if errors.Is(pushCtx.Err(), context.DeadlineExceeded) {
out, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire(ctx, subproc.GitInteractive, pushBranchTimeout, func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, args...)
cmd.Dir = env.WorktreePath
// Disable interactive credential prompts — without this, a missing
// credential helper can hang waiting on stdin even with the timeout
// above (signal delivery is gated behind the prompt read).
cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0")
return cmd
})
if runErr != nil || execCtxErr != nil {
if errors.Is(execCtxErr, context.DeadlineExceeded) {
return fmt.Errorf("git push timed out after %s in %s (branch %q); push manually and retry", pushBranchTimeout, env.WorktreePath, branch)
}
return fmt.Errorf("git push failed: %s: %w", strings.TrimSpace(string(out)), err)
if runErr == nil {
runErr = execCtxErr
}
return fmt.Errorf("git push failed: %s: %w", strings.TrimSpace(string(out)), runErr)
}
return nil
}
@@ -197,10 +201,17 @@ const defaultGitRemote = "origin"
// the worktree's git config. Falls back to defaultGitRemote when no upstream
// is set (matching the historical hard-coded behaviour).
func detectBranchRemote(ctx context.Context, dir, branch string) string {
cmd := exec.CommandContext(ctx, "git", "config", "--get", "branch."+branch+".remote")
cmd.Dir = dir
out, err := cmd.Output()
if err != nil {
out, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire(
ctx,
subproc.GitInteractive,
pushBranchTimeout,
func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, "config", "--get", "branch."+branch+".remote")
cmd.Dir = dir
return cmd
},
)
if runErr != nil || execCtxErr != nil {
return defaultGitRemote
}
remote := strings.TrimSpace(string(out))
@@ -0,0 +1,340 @@
package subproc
import (
"context"
"errors"
"fmt"
"sync"
"time"
)
// GitWorkClass identifies the kind of Git work waiting for a process slot.
type GitWorkClass string
const (
GitInteractive GitWorkClass = "interactive"
GitLifecycle GitWorkClass = "lifecycle"
GitBackground GitWorkClass = "background"
)
var ErrInvalidGitWorkClass = errors.New("invalid git work class")
// ErrGitWorkClassRequired rejects the classless Acquire API on the process-
// wide class-aware Git pool. Git callers must identify their scheduling class
// so interactive work cannot be starved by lifecycle or background traffic.
var ErrGitWorkClassRequired = errors.New("git work class required")
// ErrAdmissionCanceled marks a command that never reached execution because
// its class-aware admission wait was canceled or timed out. Callers can use
// errors.Is with both this sentinel and the underlying context error.
var ErrAdmissionCanceled = errors.New("subprocess admission canceled")
func wrapAdmissionError(err error) error {
return fmt.Errorf("%w: %w", ErrAdmissionCanceled, err)
}
var gitWorkClassOrder = [...]GitWorkClass{
GitInteractive,
GitLifecycle,
GitBackground,
}
type classWaiter struct {
class int
ctx context.Context
ready chan struct{}
enqueuedAt time.Time
queued bool
canceled bool
granted bool
release func()
}
type classAdmission struct {
owner *Throttle
mu sync.Mutex
cap int
inflight int
next int
queues [len(gitWorkClassOrder)][]*classWaiter
}
// NewNamedClassThrottle creates a class-aware throttle. It is intended for
// process-wide pools whose aggregate and per-class metrics should be exposed.
func NewNamedClassThrottle(name string, cap int) *Throttle {
t := &Throttle{name: name}
t.admission = &classAdmission{owner: t, cap: cap}
t.publishCap(cap)
return t
}
func gitClassIndex(class GitWorkClass) (int, bool) {
for i, candidate := range gitWorkClassOrder {
if candidate == class {
return i, true
}
}
return 0, false
}
func (a *classAdmission) acquire(ctx context.Context, class GitWorkClass) (func(), error) {
if err := ctx.Err(); err != nil {
return noopRelease, err
}
idx, ok := gitClassIndex(class)
if !ok {
return noopRelease, fmt.Errorf("%w: %q", ErrInvalidGitWorkClass, class)
}
waiter := &classWaiter{
class: idx,
ctx: ctx,
ready: make(chan struct{}),
enqueuedAt: time.Now(),
}
a.mu.Lock()
if err := ctx.Err(); err != nil {
a.mu.Unlock()
return noopRelease, err
}
if a.cap <= 0 {
a.recordAcquireLocked(class, 0)
a.mu.Unlock()
return noopRelease, nil
}
if a.inflight < a.cap && a.emptyLocked() {
release := a.grantLocked(waiter)
a.mu.Unlock()
return release, nil
}
waiter.queued = true
a.queues[idx] = append(a.queues[idx], waiter)
a.owner.incWaiters(1)
addClassWaiters(a.owner.name, class, 1)
a.dispatchLocked()
a.mu.Unlock()
for {
select {
case <-waiter.ready:
a.mu.Lock()
if waiter.granted {
release := waiter.release
ctxErr := ctx.Err()
a.mu.Unlock()
if ctxErr != nil {
// The dispatcher may have granted the waiter at the same
// instant its context was canceled. Do not admit work after
// cancellation; return the slot before reporting the error.
release()
return noopRelease, ctxErr
}
return release, nil
}
a.mu.Unlock()
case <-ctx.Done():
a.mu.Lock()
if waiter.granted {
release := waiter.release
a.mu.Unlock()
release()
return noopRelease, ctx.Err()
}
if waiter.canceled {
a.mu.Unlock()
return noopRelease, ctx.Err()
}
if a.removeLocked(waiter) {
a.dispatchLocked()
a.mu.Unlock()
return noopRelease, ctx.Err()
}
a.mu.Unlock()
}
}
}
func (a *classAdmission) emptyLocked() bool {
for _, queue := range a.queues {
if len(queue) != 0 {
return false
}
}
return true
}
func (a *classAdmission) dispatchLocked() {
for a.cap > a.inflight {
idx, ok := a.nextQueuedClassLocked()
if !ok {
return
}
waiter := a.queues[idx][0]
a.queues[idx] = a.queues[idx][1:]
a.next = (idx + 1) % len(gitWorkClassOrder)
a.grantLocked(waiter)
}
}
func (a *classAdmission) nextQueuedClassLocked() (int, bool) {
for offset := 0; offset < len(gitWorkClassOrder); offset++ {
idx := (a.next + offset) % len(gitWorkClassOrder)
for len(a.queues[idx]) != 0 {
waiter := a.queues[idx][0]
if !waiter.canceled && waiter.ctx.Err() == nil {
return idx, true
}
// Remove canceled waiters before advancing the round-robin
// cursor. This makes a simultaneous release/cancel boundary
// deterministic: cancellation wins even if its goroutine has
// not reached the ctx.Done select branch yet.
a.queues[idx] = a.queues[idx][1:]
a.discardCanceledLocked(waiter)
}
}
return 0, false
}
func (a *classAdmission) grantLocked(waiter *classWaiter) func() {
waiter.granted = true
a.inflight++
wait := time.Duration(0)
wasQueued := waiter.queued
if wasQueued {
wait = time.Since(waiter.enqueuedAt)
a.owner.incWaiters(-1)
addClassWaiters(a.owner.name, gitWorkClassOrder[waiter.class], -1)
waiter.queued = false
}
a.owner.incInflight(1)
class := gitWorkClassOrder[waiter.class]
addClassInflight(a.owner.name, class, 1)
a.owner.incAcquire(wait)
addClassAcquire(a.owner.name, class, wait)
var once sync.Once
waiter.release = func() {
once.Do(func() { a.release(class) })
}
if wasQueued {
close(waiter.ready)
}
return waiter.release
}
func (a *classAdmission) release(class GitWorkClass) {
a.mu.Lock()
if a.inflight > 0 {
a.inflight--
a.owner.incInflight(-1)
addClassInflight(a.owner.name, class, -1)
}
a.dispatchLocked()
a.mu.Unlock()
}
func (a *classAdmission) removeLocked(waiter *classWaiter) bool {
idx := waiter.class
queue := a.queues[idx]
for i, candidate := range queue {
if candidate != waiter {
continue
}
a.queues[idx] = append(queue[:i], queue[i+1:]...)
a.discardCanceledLocked(waiter)
return true
}
return false
}
func (a *classAdmission) discardCanceledLocked(waiter *classWaiter) {
if !waiter.queued {
return
}
waiter.queued = false
waiter.canceled = true
a.owner.incWaiters(-1)
addClassWaiters(a.owner.name, gitWorkClassOrder[waiter.class], -1)
a.recordAcquireLocked(gitWorkClassOrder[waiter.class], time.Since(waiter.enqueuedAt))
}
func (a *classAdmission) recordAcquireLocked(class GitWorkClass, wait time.Duration) {
a.owner.incAcquire(wait)
addClassAcquire(a.owner.name, class, wait)
}
func (a *classAdmission) setCapForTest(newCap int) func() {
a.mu.Lock()
previous := a.cap
a.cap = newCap
if newCap > 0 {
a.dispatchLocked()
}
a.mu.Unlock()
a.owner.publishCap(newCap)
return func() {
a.mu.Lock()
a.cap = previous
if previous > 0 {
a.dispatchLocked()
}
a.mu.Unlock()
a.owner.publishCap(previous)
}
}
func (a *classAdmission) capacity() int {
a.mu.Lock()
defer a.mu.Unlock()
return a.cap
}
// Snapshot is the structured, process-local view of Git admission state.
type Snapshot struct {
Pool string `json:"pool"`
Capacity int `json:"capacity"`
Inflight int `json:"inflight"`
Waiters int `json:"waiters"`
Classes map[string]ClassSnapshot `json:"classes"`
}
type ClassSnapshot struct {
Inflight int64 `json:"inflight"`
Waiters int64 `json:"waiters"`
AcquireTotal int64 `json:"acquire_total"`
AcquireWaitMillis int64 `json:"acquire_wait_millis_total"`
}
func (a *classAdmission) snapshot() Snapshot {
a.mu.Lock()
defer a.mu.Unlock()
classes := make(map[string]ClassSnapshot, len(gitWorkClassOrder))
for _, class := range gitWorkClassOrder {
classes[string(class)] = classSnapshot(a.owner.name, class)
}
return Snapshot{
Pool: a.owner.name,
Capacity: a.cap,
Inflight: a.inflight,
Waiters: a.waiterCountLocked(),
Classes: classes,
}
}
func (a *classAdmission) waiterCountLocked() int {
total := 0
for _, queue := range a.queues {
total += len(queue)
}
return total
}
// AcquireGit admits a process-wide Git operation in class.
func AcquireGit(ctx context.Context, class GitWorkClass) (func(), error) {
return gitThrottle.AcquireClass(ctx, class)
}
// GitCapacity returns the configured process-wide Git admission capacity.
func GitCapacity() int { return gitThrottle.admission.capacity() }
// AdmissionSnapshot returns the current process-wide Git admission state.
func AdmissionSnapshot() Snapshot { return gitThrottle.admission.snapshot() }
@@ -0,0 +1,481 @@
package subproc
import (
"context"
"errors"
"expvar"
"os/exec"
"runtime"
"strings"
"sync"
"sync/atomic"
"testing"
"time"
)
func TestGitAdmissionPublishesClassMetrics(t *testing.T) {
for _, name := range []string{
"subproc_class_inflight",
"subproc_class_waiters",
"subproc_class_acquire_total",
"subproc_class_acquire_wait_millis_total",
} {
if expvar.Get(name) == nil {
t.Fatalf("expvar %q is not published", name)
}
}
}
func TestClassAdmissionRoundRobin(t *testing.T) {
pool := NewNamedClassThrottle("class-round-robin-"+t.Name(), 1)
hold, err := pool.AcquireClass(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
type waiter struct {
class GitWorkClass
acquired chan struct{}
continueC chan struct{}
err chan error
}
waiters := []waiter{
{class: GitInteractive, acquired: make(chan struct{}), continueC: make(chan struct{}), err: make(chan error, 1)},
{class: GitLifecycle, acquired: make(chan struct{}), continueC: make(chan struct{}), err: make(chan error, 1)},
{class: GitBackground, acquired: make(chan struct{}), continueC: make(chan struct{}), err: make(chan error, 1)},
}
for i := range waiters {
w := &waiters[i]
go func() {
release, acquireErr := pool.AcquireClass(context.Background(), w.class)
w.err <- acquireErr
if acquireErr != nil {
return
}
close(w.acquired)
<-w.continueC
release()
}()
waitForClassWaiters(t, pool, w.class, 1)
}
hold()
for i, want := range []GitWorkClass{GitInteractive, GitLifecycle, GitBackground} {
select {
case <-waiters[i].acquired:
case <-time.After(time.Second):
t.Fatalf("waiter %s was not admitted", want)
}
if err := <-waiters[i].err; err != nil {
t.Fatalf("waiter %s acquire: %v", want, err)
}
close(waiters[i].continueC)
}
}
func TestClassAdmissionFIFOWithinClass(t *testing.T) {
pool := NewNamedClassThrottle("class-fifo-"+t.Name(), 1)
hold, err := pool.AcquireClass(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
firstAcquired := make(chan struct{})
secondAcquired := make(chan struct{})
firstContinue := make(chan struct{})
secondContinue := make(chan struct{})
startWaiter := func(acquired, continueC chan struct{}) {
go func() {
release, acquireErr := pool.AcquireClass(context.Background(), GitInteractive)
if acquireErr != nil {
t.Errorf("acquire: %v", acquireErr)
return
}
close(acquired)
<-continueC
release()
}()
}
startWaiter(firstAcquired, firstContinue)
waitForClassWaiters(t, pool, GitInteractive, 1)
startWaiter(secondAcquired, secondContinue)
waitForClassWaiters(t, pool, GitInteractive, 2)
hold()
select {
case <-firstAcquired:
case <-time.After(time.Second):
t.Fatal("first FIFO waiter was not admitted")
}
select {
case <-secondAcquired:
t.Fatal("second FIFO waiter was admitted before first release")
default:
}
close(firstContinue)
select {
case <-secondAcquired:
case <-time.After(time.Second):
t.Fatal("second FIFO waiter was not admitted")
}
close(secondContinue)
}
func TestClassAdmissionCancellationDoesNotAdvanceQueue(t *testing.T) {
pool := NewNamedClassThrottle("class-cancel-"+t.Name(), 1)
hold, err := pool.AcquireClass(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
cancelCtx, cancel := context.WithCancel(context.Background())
canceled := make(chan error, 1)
go func() {
_, acquireErr := pool.AcquireClass(cancelCtx, GitInteractive)
canceled <- acquireErr
}()
waitForClassWaiters(t, pool, GitInteractive, 1)
cancel()
if err := <-canceled; !errors.Is(err, context.Canceled) {
t.Fatalf("canceled acquire = %v, want context.Canceled", err)
}
waitForClassWaiters(t, pool, GitInteractive, 0)
acquired := make(chan struct{})
continueC := make(chan struct{})
go func() {
release, acquireErr := pool.AcquireClass(context.Background(), GitLifecycle)
if acquireErr != nil {
t.Errorf("lifecycle acquire: %v", acquireErr)
return
}
close(acquired)
<-continueC
release()
}()
waitForClassWaiters(t, pool, GitLifecycle, 1)
hold()
select {
case <-acquired:
case <-time.After(time.Second):
t.Fatal("lifecycle waiter was not admitted after cancellation")
}
close(continueC)
}
func TestClassAdmissionCancellationWinsReleaseBoundary(t *testing.T) {
pool := NewNamedClassThrottle("class-cancel-release-boundary-"+t.Name(), 1)
hold, err := pool.AcquireClass(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
ctx, cancel := context.WithCancel(context.Background())
result := make(chan error, 1)
go func() {
_, acquireErr := pool.AcquireClass(ctx, GitInteractive)
result <- acquireErr
}()
waitForClassWaiters(t, pool, GitInteractive, 1)
// Cancel before releasing without waiting for the waiter goroutine. The
// dispatcher must inspect the canceled queue head itself; otherwise a
// release can grant the slot and the canceled waiter can report success.
cancel()
hold()
select {
case acquireErr := <-result:
if !errors.Is(acquireErr, context.Canceled) {
t.Fatalf("acquire error = %v, want context.Canceled", acquireErr)
}
case <-time.After(time.Second):
t.Fatal("canceled waiter did not return at the release boundary")
}
if snapshot := pool.admission.snapshot(); snapshot.Inflight != 0 || snapshot.Waiters != 0 {
t.Fatalf("admission after canceled release = %+v, want no slot or waiter", snapshot)
}
}
func TestClassAdmissionCancellationCountsAggregateAndClassAttempt(t *testing.T) {
pool := NewNamedClassThrottle("class-cancel-metrics-"+t.Name(), 1)
hold, err := pool.AcquireClass(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
defer hold()
beforeAggregate := metricInt(subprocAcquireTotal, pool.name)
beforeClass := metricInt(subprocClassAcquireTotal, classMetricKey(pool.name, GitLifecycle))
ctx, cancel := context.WithCancel(context.Background())
result := make(chan error, 1)
go func() {
_, acquireErr := pool.AcquireClass(ctx, GitLifecycle)
result <- acquireErr
}()
waitForClassWaiters(t, pool, GitLifecycle, 1)
cancel()
if acquireErr := <-result; !errors.Is(acquireErr, context.Canceled) {
t.Fatalf("canceled acquire = %v, want context.Canceled", acquireErr)
}
if got := metricInt(subprocAcquireTotal, pool.name); got != beforeAggregate+1 {
t.Fatalf("aggregate acquire total = %d, want %d", got, beforeAggregate+1)
}
if got := metricInt(subprocClassAcquireTotal, classMetricKey(pool.name, GitLifecycle)); got != beforeClass+1 {
t.Fatalf("lifecycle acquire total = %d, want %d", got, beforeClass+1)
}
}
func TestClassAdmissionGlobalCapAndWorkConservation(t *testing.T) {
pool := NewNamedClassThrottle("class-cap-"+t.Name(), 2)
var active, peak int64
var wg sync.WaitGroup
for i := 0; i < 32; i++ {
class := gitWorkClassOrder[i%len(gitWorkClassOrder)]
wg.Add(1)
go func() {
defer wg.Done()
release, err := pool.AcquireClass(context.Background(), class)
if err != nil {
t.Errorf("acquire %s: %v", class, err)
return
}
defer release()
current := atomic.AddInt64(&active, 1)
for {
previous := atomic.LoadInt64(&peak)
if current <= previous || atomic.CompareAndSwapInt64(&peak, previous, current) {
break
}
}
time.Sleep(time.Millisecond)
atomic.AddInt64(&active, -1)
}()
}
wg.Wait()
if peak > 2 {
t.Fatalf("peak active = %d, want <= 2", peak)
}
if got := pool.admission.snapshot().Inflight; got != 0 {
t.Fatalf("inflight after drain = %d, want 0", got)
}
holdA, err := pool.AcquireClass(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("interactive hold A: %v", err)
}
holdB, err := pool.AcquireClass(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("interactive hold B: %v", err)
}
queued := make(chan struct{})
continueC := make(chan struct{})
go func() {
release, acquireErr := pool.AcquireClass(context.Background(), GitInteractive)
if acquireErr != nil {
t.Errorf("queued acquire: %v", acquireErr)
return
}
close(queued)
<-continueC
release()
}()
waitForClassWaiters(t, pool, GitInteractive, 1)
holdA()
select {
case <-queued:
case <-time.After(time.Second):
t.Fatal("queued interactive work did not use released capacity")
}
holdB()
close(continueC)
}
func TestClassAdmissionSnapshot(t *testing.T) {
pool := NewNamedClassThrottle("class-snapshot-"+t.Name(), 1)
hold, err := pool.AcquireClass(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
queued := make(chan struct{})
go func() {
release, acquireErr := pool.AcquireClass(context.Background(), GitLifecycle)
if acquireErr != nil {
t.Errorf("queued acquire: %v", acquireErr)
return
}
close(queued)
release()
}()
waitForClassWaiters(t, pool, GitLifecycle, 1)
snapshot := pool.admission.snapshot()
if snapshot.Capacity != 1 || snapshot.Inflight != 1 || snapshot.Waiters != 1 {
t.Fatalf("snapshot = %+v, want cap=1 inflight=1 waiters=1", snapshot)
}
if got := snapshot.Classes[string(GitInteractive)].Inflight; got != 1 {
t.Fatalf("interactive snapshot inflight = %d, want 1", got)
}
if got := snapshot.Classes[string(GitLifecycle)].Waiters; got != 1 {
t.Fatalf("lifecycle snapshot waiters = %d, want 1", got)
}
hold()
select {
case <-queued:
case <-time.After(time.Second):
t.Fatal("queued lifecycle work was not admitted")
}
}
func TestClassAdmissionRejectsUnknownClass(t *testing.T) {
pool := NewNamedClassThrottle("class-invalid-"+t.Name(), 1)
_, err := pool.AcquireClass(context.Background(), GitWorkClass("unknown"))
if !errors.Is(err, ErrInvalidGitWorkClass) {
t.Fatalf("err = %v, want ErrInvalidGitWorkClass", err)
}
}
func TestClassThrottleRejectsClasslessAcquire(t *testing.T) {
pool := NewNamedClassThrottle("class-required-"+t.Name(), 1)
if _, err := pool.Acquire(context.Background()); !errors.Is(err, ErrGitWorkClassRequired) {
t.Fatalf("classless acquire error = %v, want ErrGitWorkClassRequired", err)
}
}
func TestGitCapacityTestSeam(t *testing.T) {
restore := Git().SetCapForTest(3)
defer restore()
if got := GitCapacity(); got != 3 {
t.Fatalf("GitCapacity() = %d, want 3", got)
}
}
func TestRunGitOutputAfterAcquireStartsTimeoutAfterAdmission(t *testing.T) {
restore := Git().SetCapForTest(1)
t.Cleanup(restore)
hold, err := AcquireGit(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
type result struct {
out []byte
runErr error
execErr error
}
resultCh := make(chan result, 1)
started := make(chan struct{})
go func() {
out, runErr, execErr := RunGitOutputAfterAcquire(context.Background(), GitInteractive, 50*time.Millisecond, func(execCtx context.Context) *exec.Cmd {
close(started)
return exec.CommandContext(execCtx, "sh", "-c", "printf ok")
})
resultCh <- result{out: out, runErr: runErr, execErr: execErr}
}()
waitForClassWaiters(t, Git(), GitInteractive, 1)
hold()
select {
case got := <-resultCh:
if got.runErr != nil || got.execErr != nil || strings.TrimSpace(string(got.out)) != "ok" {
t.Fatalf("result = %+v, want successful command after queued admission", got)
}
case <-time.After(time.Second):
t.Fatal("queued command did not complete after admission")
}
select {
case <-started:
default:
t.Fatal("command builder did not run after admission")
}
}
func TestRunGitOutputAfterAcquireWithExecutionContextDetachesAdmissionDeadline(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("uses POSIX `true` binary as a no-op subprocess")
}
restore := Git().SetCapForTest(1)
t.Cleanup(restore)
hold, err := AcquireGit(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
acquireCtx, cancelAcquire := context.WithTimeout(context.Background(), 350*time.Millisecond)
t.Cleanup(cancelAcquire)
var remaining time.Duration
resultCh := make(chan error, 1)
go func() {
_, runErr, execErr := RunGitOutputAfterAcquireWithExecutionContext(
acquireCtx,
context.Background(),
GitInteractive,
100*time.Millisecond,
func(execCtx context.Context) *exec.Cmd {
if deadline, ok := execCtx.Deadline(); ok {
remaining = time.Until(deadline)
}
return exec.CommandContext(execCtx, "true")
},
)
if runErr != nil {
resultCh <- runErr
return
}
resultCh <- execErr
}()
waitForClassWaiters(t, Git(), GitInteractive, 1)
timer := time.NewTimer(300 * time.Millisecond)
<-timer.C
hold()
if err := <-resultCh; err != nil {
t.Fatalf("queued command failed: %v", err)
}
if remaining < 80*time.Millisecond {
t.Fatalf("execution deadline has only %v remaining; admission deadline leaked into execution", remaining)
}
}
func TestRunGitOutputAfterAcquireMarksAdmissionCancellation(t *testing.T) {
restore := Git().SetCapForTest(1)
t.Cleanup(restore)
hold, err := AcquireGit(context.Background(), GitInteractive)
if err != nil {
t.Fatalf("hold slot: %v", err)
}
ctx, cancel := context.WithCancel(context.Background())
resultCh := make(chan error, 1)
go func() {
_, runErr, execErr := RunGitOutputAfterAcquire(ctx, GitInteractive, time.Second, func(execCtx context.Context) *exec.Cmd {
return exec.CommandContext(execCtx, "sh", "-c", "printf unexpected")
})
if execErr != nil {
resultCh <- execErr
return
}
resultCh <- runErr
}()
waitForClassWaiters(t, Git(), GitInteractive, 1)
cancel()
select {
case err := <-resultCh:
if !errors.Is(err, ErrAdmissionCanceled) || !errors.Is(err, context.Canceled) {
t.Fatalf("error = %v, want admission cancellation wrapping context.Canceled", err)
}
case <-time.After(time.Second):
t.Fatal("canceled admission did not return")
}
hold()
}
func waitForClassWaiters(t *testing.T, pool *Throttle, class GitWorkClass, want int64) {
t.Helper()
key := classMetricKey(pool.name, class)
deadline := time.Now().Add(time.Second)
for time.Now().Before(deadline) {
if got := metricInt(subprocClassWaiters, key); got == want {
return
}
runtime.Gosched()
}
t.Fatalf("waiters for %s did not reach %d (got %d)", class, want, metricInt(subprocClassWaiters, key))
}
@@ -0,0 +1,28 @@
package subproc
import (
"context"
"os/exec"
)
// GitExecutablePath resolves the Git binary for helper processes that need to
// preserve a file-descriptor-based working directory. Keeping the lookup in
// the Git seam lets the repository audit cover both command construction and
// executable discovery.
func GitExecutablePath() (string, error) { return exec.LookPath("git") }
// NewGitCommand is the only production Git command-construction seam. Callers
// must pass the returned command to a classified RunGit* helper (or hold a
// classified slot around a streaming Start/Wait lifecycle).
//
// Build the command with only the fixed executable at the exec.CommandContext
// sink, then attach the already-tokenized argv. Git never invokes a shell for
// Cmd.Args, and keeping the user-derived values out of the constructor also
// prevents CodeQL's command-injection query from treating this direct-argv
// seam as shell interpolation. Callers remain responsible for validating
// user-controlled refs, paths, and option values before they reach this seam.
func NewGitCommand(ctx context.Context, args ...string) *exec.Cmd {
cmd := exec.CommandContext(ctx, "git")
cmd.Args = append(cmd.Args, args...)
return cmd
}
@@ -0,0 +1,11 @@
//go:build !linux && !darwin
package subproc
import "errors"
// ExecGit is unavailable on platforms that do not use the inherited-directory
// Git helper. The platform-specific caller does not invoke this stub.
func ExecGit(string, []string, []string) error {
return errors.New("inherited Git execution is unsupported on this platform")
}
@@ -0,0 +1,15 @@
//go:build linux || darwin
package subproc
import "golang.org/x/sys/unix"
// ExecGit replaces the current helper process with Git. Callers must hold a
// classified Git admission slot for the lifetime of that process; this seam
// only owns the platform-specific exec operation.
func ExecGit(path string, args, env []string) error {
argv := make([]string, 0, len(args)+1)
argv = append(argv, "git")
argv = append(argv, args...)
return unix.Exec(path, argv, env)
}
@@ -27,8 +27,49 @@ var (
subprocWaiters = expvar.NewMap("subproc_waiters")
subprocAcquireTotal = expvar.NewMap("subproc_acquire_total")
subprocAcquireWaitMillis = expvar.NewMap("subproc_acquire_wait_millis_total")
subprocClassInflight = expvar.NewMap("subproc_class_inflight")
subprocClassWaiters = expvar.NewMap("subproc_class_waiters")
subprocClassAcquireTotal = expvar.NewMap("subproc_class_acquire_total")
subprocClassWaitMillis = expvar.NewMap("subproc_class_acquire_wait_millis_total")
)
func classMetricKey(pool string, class GitWorkClass) string {
return "pool=" + pool + ";class=" + string(class)
}
func addClassInflight(pool string, class GitWorkClass, delta int64) {
if pool != "" {
subprocClassInflight.Add(classMetricKey(pool, class), delta)
}
}
func addClassWaiters(pool string, class GitWorkClass, delta int64) {
if pool != "" {
subprocClassWaiters.Add(classMetricKey(pool, class), delta)
}
}
func addClassAcquire(pool string, class GitWorkClass, waited time.Duration) {
if pool == "" {
return
}
key := classMetricKey(pool, class)
subprocClassAcquireTotal.Add(key, 1)
if waited > 0 && waited.Milliseconds() > 0 {
subprocClassWaitMillis.Add(key, waited.Milliseconds())
}
}
func classSnapshot(pool string, class GitWorkClass) ClassSnapshot {
key := classMetricKey(pool, class)
return ClassSnapshot{
Inflight: metricInt(subprocClassInflight, key),
Waiters: metricInt(subprocClassWaiters, key),
AcquireTotal: metricInt(subprocClassAcquireTotal, key),
AcquireWaitMillis: metricInt(subprocClassWaitMillis, key),
}
}
// publishCap sets the gauge for the configured cap. Called once at
// NewNamedThrottle and again from SetCapForTest. The cap is published
// even when name == "" is a no-op so test pools don't drop a "0" into
@@ -0,0 +1,126 @@
package subproc
import (
"fmt"
"go/ast"
"go/parser"
"go/token"
"io/fs"
"path/filepath"
"runtime"
"sort"
"strconv"
"strings"
"testing"
)
// TestProductionGitCommandsUseTheAdmissionSeam is a repository guard for the
// global Git cap. Production code may construct Git commands only in the
// subproc seam; every other package must invoke a classified helper instead.
// This catches raw command construction, executable lookup, and direct exec
// additions that a search for the legacy subproc.Git accessor would miss.
func TestProductionGitCommandsUseTheAdmissionSeam(t *testing.T) {
_, sourceFile, _, ok := runtime.Caller(0)
if !ok {
t.Fatal("runtime.Caller failed")
}
backendRoot := filepath.Clean(filepath.Join(filepath.Dir(sourceFile), "../../.."))
var violations []string
err := filepath.WalkDir(backendRoot, func(path string, entry fs.DirEntry, walkErr error) error {
if walkErr != nil {
return walkErr
}
if entry.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") {
return nil
}
rel, err := filepath.Rel(backendRoot, path)
if err != nil {
return err
}
// The common/subproc package is the sole Git execution seam. It may
// contain the platform-specific command/exec primitives, while every
// production caller outside it must use a classified runner.
if !strings.HasPrefix(filepath.ToSlash(rel), "internal/common/subproc/") {
violations = append(violations, scanGoFile(t, path)...)
}
return nil
})
if err != nil {
t.Fatalf("walk production Go files: %v", err)
}
sort.Strings(violations)
if len(violations) != 0 {
t.Fatalf("raw Git execution bypasses the admission seam:\n%s", strings.Join(violations, "\n"))
}
}
func scanGoFile(t *testing.T, path string) []string {
t.Helper()
fileSet := token.NewFileSet()
file, err := parser.ParseFile(fileSet, path, nil, 0)
if err != nil {
t.Fatalf("parse %s: %v", path, err)
}
var violations []string
ast.Inspect(file, func(node ast.Node) bool {
call, ok := node.(*ast.CallExpr)
if !ok || len(call.Args) == 0 {
return true
}
selector, ok := call.Fun.(*ast.SelectorExpr)
if !ok {
return true
}
execPkg, ok := selector.X.(*ast.Ident)
if !ok {
return true
}
if (execPkg.Name == "unix" || execPkg.Name == "syscall") && selector.Sel.Name == "Exec" {
position := fileSet.Position(call.Pos())
violations = append(violations, fmt.Sprintf("%s:%d: direct exec", filepath.ToSlash(path), position.Line))
return true
}
if execPkg.Name != "exec" {
return true
}
if selector.Sel.Name == "LookPath" {
if len(call.Args) == 0 || !isGitString(call.Args[0]) {
return true
}
position := fileSet.Position(call.Pos())
violations = append(violations, fmt.Sprintf("%s:%d: Git lookup", filepath.ToSlash(path), position.Line))
return true
}
if selector.Sel.Name != "Command" && selector.Sel.Name != "CommandContext" {
return true
}
// The command argument is the first argument for Command and the
// second for CommandContext. Inspect both positions explicitly.
commandIndex := 0
if selector.Sel.Name == "CommandContext" {
commandIndex = 1
}
if len(call.Args) <= commandIndex {
return true
}
if !isGitString(call.Args[commandIndex]) {
return true
}
position := fileSet.Position(call.Pos())
violations = append(violations, fmt.Sprintf("%s:%d", filepath.ToSlash(path), position.Line))
return true
})
return violations
}
func isGitString(expr ast.Expr) bool {
// NOTE: this guard intentionally recognizes direct string literals only;
// keep production Git construction on the shared subproc seam rather than
// relying on constant aliases that this lightweight AST walk cannot resolve.
literal, ok := expr.(*ast.BasicLit)
if !ok || literal.Kind != token.STRING {
return false
}
value, err := strconv.Unquote(literal.Value)
return err == nil && value == "git"
}
+87 -13
View File
@@ -45,7 +45,7 @@ var (
// tests that swap the pool via SetCapForTest reuse the same names
// so the published gauges stay coherent across cap changes.
ghThrottle = NewNamedThrottle("gh", resolveCap(ghMaxConcurrentEnv, defaultGHMaxConcurrent))
gitThrottle = NewNamedThrottle("git", resolveCap(gitMaxConcurrentEnv, defaultGitMaxConcurrent))
gitThrottle = NewNamedClassThrottle("git", resolveCap(gitMaxConcurrentEnv, defaultGitMaxConcurrent))
)
// GH returns the process-wide throttle gating gh subprocess execs.
@@ -81,11 +81,10 @@ func resolveCap(env string, def int) int {
func resolveGHMaxConcurrent() int { return resolveCap(ghMaxConcurrentEnv, defaultGHMaxConcurrent) }
func resolveGitMaxConcurrent() int { return resolveCap(gitMaxConcurrentEnv, defaultGitMaxConcurrent) }
// RunGit acquires a git slot, runs cmd, and releases the slot. Use
// this anywhere we exec a git binary — calling cmd.Run() directly
// bypasses the throttle. The caller owns cmd.Stdout/Stderr wiring.
func RunGit(ctx context.Context, cmd *exec.Cmd) error {
release, err := gitThrottle.Acquire(ctx)
// RunGitClass acquires a Git slot in class, runs cmd, and releases the slot.
// The caller owns cmd.Stdout/Stderr wiring.
func RunGitClass(ctx context.Context, class GitWorkClass, cmd *exec.Cmd) error {
release, err := gitThrottle.AcquireClass(ctx, class)
if err != nil {
return err
}
@@ -93,9 +92,9 @@ func RunGit(ctx context.Context, cmd *exec.Cmd) error {
return cmd.Run()
}
// RunGitCombinedOutput is RunGit's CombinedOutput sibling.
func RunGitCombinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
release, err := gitThrottle.Acquire(ctx)
// RunGitCombinedOutputClass is RunGitClass's CombinedOutput sibling.
func RunGitCombinedOutputClass(ctx context.Context, class GitWorkClass, cmd *exec.Cmd) ([]byte, error) {
release, err := gitThrottle.AcquireClass(ctx, class)
if err != nil {
return nil, err
}
@@ -103,10 +102,11 @@ func RunGitCombinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
return cmd.CombinedOutput()
}
// RunGitOutput is RunGit's Output sibling. stderr ends up in
// (*exec.ExitError).Stderr when set by the caller.
func RunGitOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
release, err := gitThrottle.Acquire(ctx)
// RunGitOutputClass is RunGitClass's Output sibling. Stderr is captured in
// (*exec.ExitError).Stderr only when cmd.Stderr is left nil; callers that set
// cmd.Stderr must read it from their provided writer.
func RunGitOutputClass(ctx context.Context, class GitWorkClass, cmd *exec.Cmd) ([]byte, error) {
release, err := gitThrottle.AcquireClass(ctx, class)
if err != nil {
return nil, err
}
@@ -114,6 +114,80 @@ func RunGitOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
return cmd.Output()
}
// RunGitCombinedAfterAcquire starts the execution timeout only after the
// requested class receives a Git slot. The builder runs inside the slot so
// command construction cannot consume the execution budget while queued.
func RunGitCombinedAfterAcquire(
ctx context.Context,
class GitWorkClass,
execTimeout time.Duration,
build func(execCtx context.Context) *exec.Cmd,
) ([]byte, error, error) {
release, err := gitThrottle.AcquireClass(ctx, class)
if err != nil {
return nil, wrapAdmissionError(err), nil
}
defer release()
execCtx, cancel := withExecTimeout(ctx, execTimeout)
defer cancel()
out, runErr := build(execCtx).CombinedOutput()
return out, runErr, execCtx.Err()
}
// RunGitOutputAfterAcquire is RunGitCombinedAfterAcquire's Output sibling.
// The command is built only after admission, and stderr retains exec.Cmd's
// standard Output behavior through *exec.ExitError.Stderr. The acquisition
// context also owns the execution deadline; use
// RunGitOutputAfterAcquireWithExecutionContext when those lifetimes differ.
func RunGitOutputAfterAcquire(
ctx context.Context,
class GitWorkClass,
execTimeout time.Duration,
build func(execCtx context.Context) *exec.Cmd,
) ([]byte, error, error) {
return RunGitOutputAfterAcquireWithExecutionContext(ctx, ctx, class, execTimeout, build)
}
// RunGitOutputAfterAcquireWithExecutionContext admits using acquireCtx, then
// starts the command timeout from execBaseCtx only after a slot is granted.
// This is useful for best-effort probes that need a bounded queue wait but
// must not inherit the queue deadline as their execution deadline.
func RunGitOutputAfterAcquireWithExecutionContext(
acquireCtx context.Context,
execBaseCtx context.Context,
class GitWorkClass,
execTimeout time.Duration,
build func(execCtx context.Context) *exec.Cmd,
) ([]byte, error, error) {
release, err := gitThrottle.AcquireClass(acquireCtx, class)
if err != nil {
return nil, wrapAdmissionError(err), nil
}
defer release()
execCtx, cancel := withExecTimeout(execBaseCtx, execTimeout)
defer cancel()
out, runErr := build(execCtx).Output()
return out, runErr, execCtx.Err()
}
// RunGitAfterAcquire is RunGitCombinedAfterAcquire's plain Run sibling.
func RunGitAfterAcquire(
ctx context.Context,
class GitWorkClass,
execTimeout time.Duration,
build func(execCtx context.Context) *exec.Cmd,
) (error, error) {
release, err := gitThrottle.AcquireClass(ctx, class)
if err != nil {
return wrapAdmissionError(err), nil
}
defer release()
execCtx, cancel := withExecTimeout(ctx, execTimeout)
defer cancel()
runErr := build(execCtx).Run()
return runErr, execCtx.Err()
}
// RunGH / RunGHOutput / RunGHCombinedOutput mirror the git helpers but
// gate on the gh throttle. Keep these in sync with the git triplet —
// if a new exec method is needed (e.g. StdoutPipe streaming), add the
@@ -35,9 +35,10 @@ import (
// reports queue depth and saturation per pool (gh, git, ...). Unnamed
// throttles stay silent — tests and ad-hoc pools don't pollute /debug/vars.
type Throttle struct {
mu sync.RWMutex
sem chan struct{}
name string
mu sync.RWMutex
sem chan struct{}
name string
admission *classAdmission
}
// NewThrottle returns a Throttle with the given cap and no name. cap <= 0
@@ -64,13 +65,15 @@ func NewNamedThrottle(name string, cap int) *Throttle {
return t
}
// Acquire blocks until a slot is available or ctx is cancelled. The
// returned release function is safe to call any number of times — only
// the first invocation returns the slot to the pool — but every successful
// Acquire MUST call release at least once (typically via defer) to avoid
// permanently shrinking the pool. On context error it returns ctx.Err()
// and a no-op release so callers can defer unconditionally without
// leaking slots.
// Acquire blocks until a slot is available or ctx is cancelled. It is the
// classless API for generic throttles such as gh; the process-wide class-aware
// Git throttle rejects it with ErrGitWorkClassRequired. Git callers must use
// AcquireClass (or AcquireGit) so their scheduling class is explicit. The
// returned release function is safe to call any number of times — only the
// first invocation returns the slot to the pool — but every successful Acquire
// MUST call release at least once (typically via defer) to avoid permanently
// shrinking the pool. On context error it returns ctx.Err() and a no-op release
// so callers can defer unconditionally without leaking slots.
//
// Fast path: if ctx is already cancelled at entry, return immediately
// without racing the select against an available slot. Without this,
@@ -78,6 +81,9 @@ func NewNamedThrottle(name string, cap int) *Throttle {
// ready, so a cancelled caller might still acquire and then fail
// downstream — unobservable but non-deterministic.
func (t *Throttle) Acquire(ctx context.Context) (release func(), err error) {
if t.admission != nil {
return noopRelease, ErrGitWorkClassRequired
}
if err := ctx.Err(); err != nil {
return noopRelease, err
}
@@ -136,6 +142,16 @@ func (t *Throttle) Acquire(ctx context.Context) (release func(), err error) {
}
}
// AcquireClass admits one Git operation in the requested work class. It is
// available on Throttle so the existing Git singleton can keep its narrow
// package-level accessor while callers migrate to explicit classification.
func (t *Throttle) AcquireClass(ctx context.Context, class GitWorkClass) (func(), error) {
if t.admission == nil {
return t.Acquire(ctx)
}
return t.admission.acquire(ctx, class)
}
// releaseFunc returns an idempotent release closure that decrements the
// inflight gauge and frees a slot. sync.Once protects against a double
// release (deferred + early-return path in a future refactor) draining a
@@ -172,6 +188,9 @@ func (t *Throttle) currentSem() chan struct{} {
// Restore is idempotent in the sense that the previous pool is captured
// at the call site; nested test setups stack via the returned closure.
func (t *Throttle) SetCapForTest(newCap int) func() {
if t.admission != nil {
return t.admission.setCapForTest(newCap)
}
t.mu.Lock()
prev := t.sem
prevCap := 0
@@ -1,7 +1,6 @@
package configloader
import (
"bytes"
"context"
"fmt"
"os"
@@ -12,6 +11,7 @@ import (
"time"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
)
@@ -262,24 +262,18 @@ func isAllowedRepoURL(u string) bool {
// runGit executes a git command and returns combined stdout. If dir is empty,
// the command runs in the current directory.
func (g *GitManager) runGit(ctx context.Context, dir string, args ...string) (string, error) {
ctx, cancel := context.WithTimeout(ctx, gitCmdTimeout)
defer cancel()
cmd := exec.CommandContext(ctx, "git", args...)
if dir != "" {
cmd.Dir = dir
}
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
if err := cmd.Run(); err != nil {
errMsg := stderr.String()
if errMsg == "" {
errMsg = stdout.String()
output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire(ctx, subproc.GitLifecycle, gitCmdTimeout, func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, args...)
if dir != "" {
cmd.Dir = dir
}
return "", fmt.Errorf("%s: %s", err, strings.TrimSpace(errMsg))
return cmd
})
if runErr != nil || execCtxErr != nil {
if runErr == nil {
runErr = execCtxErr
}
return "", fmt.Errorf("%s: %s", runErr, strings.TrimSpace(string(output)))
}
return stdout.String(), nil
return string(output), nil
}
@@ -7,10 +7,10 @@ import (
"fmt"
"net/url"
"os"
"os/exec"
"path/filepath"
"strings"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/office/models"
)
@@ -176,11 +176,11 @@ func hashLocator(locator string) string {
}
func runGit(dir string, args ...string) error {
cmd := exec.Command("git", args...)
cmd := subproc.NewGitCommand(context.Background(), args...)
if dir != "" {
cmd.Dir = dir
}
out, err := cmd.CombinedOutput()
out, err := subproc.RunGitCombinedOutputClass(context.Background(), subproc.GitLifecycle, cmd)
if err != nil {
return fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(out)))
}
@@ -15,7 +15,6 @@ import (
"net/http"
"net/url"
"os"
"os/exec"
"strings"
"time"
@@ -25,6 +24,7 @@ import (
settingsstore "github.com/kandev/kandev/internal/agent/settings/store"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/events"
"github.com/kandev/kandev/internal/events/bus"
"github.com/kandev/kandev/internal/office/agents"
@@ -142,11 +142,13 @@ func configureGitRemoteHandler(repo *sqliterepo.Repository, log *logger.Logger)
return
}
args := []string{"-C", repository.LocalPath, "remote", "add", "origin", trustedRemoteURL}
if getErr := exec.CommandContext(c.Request.Context(), "git", "-C", repository.LocalPath,
"remote", "get-url", "origin").Run(); getErr == nil {
getCmd := subproc.NewGitCommand(c.Request.Context(), "-C", repository.LocalPath,
"remote", "get-url", "origin")
if getErr := subproc.RunGitClass(c.Request.Context(), subproc.GitLifecycle, getCmd); getErr == nil {
args = []string{"-C", repository.LocalPath, "remote", "set-url", "origin", trustedRemoteURL}
}
if output, runErr := exec.CommandContext(c.Request.Context(), "git", args...).CombinedOutput(); runErr != nil {
setCmd := subproc.NewGitCommand(c.Request.Context(), args...)
if output, runErr := subproc.RunGitCombinedOutputClass(c.Request.Context(), subproc.GitLifecycle, setCmd); runErr != nil {
log.Error("test harness: configure git remote failed", zap.Error(runErr))
errJSON(c, http.StatusInternalServerError, strings.TrimSpace(string(output)))
return
@@ -161,7 +163,8 @@ func configureGitRemoteHandler(repo *sqliterepo.Repository, log *logger.Logger)
}
func removeGitRemote(ctx context.Context, repositoryPath string) {
_ = exec.CommandContext(ctx, "git", "-C", repositoryPath, "remote", "remove", "origin").Run()
cmd := subproc.NewGitCommand(ctx, "-C", repositoryPath, "remote", "remove", "origin")
_ = subproc.RunGitClass(ctx, subproc.GitLifecycle, cmd)
for _, envKey := range []string{"KANDEV_E2E_GITLAB_PUSH_FILE", "KANDEV_E2E_GITLAB_PUSH_RECORD_FILE"} {
if path := os.Getenv(envKey); path != "" {
_ = os.Remove(path)
@@ -5,7 +5,6 @@ import (
"errors"
"fmt"
"maps"
"os/exec"
"regexp"
"strings"
"sync"
@@ -14,6 +13,7 @@ import (
"github.com/google/uuid"
"github.com/kandev/kandev/internal/agent/runtime/lifecycle"
"github.com/kandev/kandev/internal/agent/runtime/routingerr"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/orchestrator/sessionstate"
"github.com/kandev/kandev/internal/repoclone"
"github.com/kandev/kandev/internal/sysprompt"
@@ -583,8 +583,8 @@ func repositoryCloneURL(repo *models.Repository) string {
if repo.LocalPath == "" {
return ""
}
cmd := exec.Command("git", "-C", repo.LocalPath, "remote", "get-url", "origin")
out, err := cmd.Output()
cmd := subproc.NewGitCommand(context.Background(), "-C", repo.LocalPath, "remote", "get-url", "origin")
out, err := subproc.RunGitOutputClass(context.Background(), subproc.GitLifecycle, cmd)
if err != nil {
return ""
}
+16 -16
View File
@@ -347,9 +347,9 @@ func (c *Cloner) SetOriginURL(ctx context.Context, repositoryPath, originURL str
// value. Compare the value stored in the local config instead so a common
// HTTPS-to-SSH rewrite does not turn an already-canonical origin into a
// needless config write on every launch/resume.
cmd := exec.CommandContext(ctx, "git", "-C", repositoryPath, "config", "--local", "--get", "remote.origin.url")
cmd := subproc.NewGitCommand(ctx, "-C", repositoryPath, "config", "--local", "--get", "remote.origin.url")
configureGitCommand(cmd, nil)
currentOutput, err := subproc.RunGitCombinedOutput(ctx, cmd)
currentOutput, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd)
if err != nil {
return fmt.Errorf("inspect repository origin: %w", formatGitOriginError(repositoryPath, currentOutput, err))
}
@@ -357,9 +357,9 @@ func (c *Cloner) SetOriginURL(ctx context.Context, repositoryPath, originURL str
return nil
}
cmd = exec.CommandContext(ctx, "git", "-C", repositoryPath, "remote", "set-url", "origin", "--", originURL)
cmd = subproc.NewGitCommand(ctx, "-C", repositoryPath, "remote", "set-url", "origin", "--", originURL)
configureGitCommand(cmd, nil)
output, err := subproc.RunGitCombinedOutput(ctx, cmd)
output, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd)
if err != nil {
return fmt.Errorf("set repository origin: %w", formatGitOriginError(repositoryPath, output, err))
}
@@ -493,11 +493,11 @@ func (c *Cloner) ensureClonedWithBasicAuth(
func (c *Cloner) fetch(ctx context.Context, repoPath string, auth *cloneAuth) {
c.logger.Debug("repository already cloned, fetching", zap.String("path", repoPath))
cmd := exec.CommandContext(
ctx, "git", "-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags,
cmd := subproc.NewGitCommand(
ctx, "-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags,
)
configureGitCommand(cmd, auth)
if out, err := subproc.RunGitCombinedOutput(ctx, cmd); err != nil {
if out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil {
c.logger.Warn("git fetch failed (non-fatal)",
zap.String("path", repoPath), zap.String("output", redactCloneOutput(string(out), authToken(auth))), zap.Error(err))
}
@@ -508,11 +508,11 @@ func (c *Cloner) clone(ctx context.Context, cloneURL, targetPath string, auth *c
return fmt.Errorf("create parent directory: %w", err)
}
c.logger.Info("cloning repository", zap.String("url", redactCloneURL(cloneURL)), zap.String("target", targetPath))
cmd := exec.CommandContext(
ctx, "git", "clone", "--filter=blob:none", gitNoTags, "--", cloneURL, targetPath,
cmd := subproc.NewGitCommand(
ctx, "clone", "--filter=blob:none", gitNoTags, "--", cloneURL, targetPath,
)
configureGitCommand(cmd, auth)
if out, err := subproc.RunGitCombinedOutput(ctx, cmd); err != nil {
if out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil {
return fmt.Errorf("git clone failed: %s: %w", redactCloneOutput(string(out), authToken(auth)), err)
}
return nil
@@ -520,11 +520,11 @@ func (c *Cloner) clone(ctx context.Context, cloneURL, targetPath string, auth *c
func (c *Cloner) fetchWithHTTPHeader(ctx context.Context, repoPath, authURL, header string) {
c.logger.Debug("repository already cloned, fetching", zap.String("path", repoPath))
cmd := exec.CommandContext(
ctx, "git", "-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags,
cmd := subproc.NewGitCommand(
ctx, "-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags,
)
configureHTTPHeaderCommand(cmd, authURL, header)
if out, err := subproc.RunGitCombinedOutput(ctx, cmd); err != nil {
if out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil {
c.logger.Warn("authenticated git fetch failed (non-fatal)",
zap.String("path", repoPath), zap.String("output", string(out)), zap.Error(err))
}
@@ -535,11 +535,11 @@ func (c *Cloner) cloneWithHTTPHeader(ctx context.Context, cloneURL, targetPath,
return fmt.Errorf("create parent directory: %w", err)
}
c.logger.Info("cloning authenticated repository", zap.String("url", redactCloneURL(cloneURL)), zap.String("target", targetPath))
cmd := exec.CommandContext(
ctx, "git", "clone", "--filter=blob:none", gitNoTags, "--", cloneURL, targetPath,
cmd := subproc.NewGitCommand(
ctx, "clone", "--filter=blob:none", gitNoTags, "--", cloneURL, targetPath,
)
configureHTTPHeaderCommand(cmd, cloneURL, header)
if out, err := subproc.RunGitCombinedOutput(ctx, cmd); err != nil {
if out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil {
return fmt.Errorf("git clone failed: %s: %w", string(out), err)
}
return nil
@@ -9,6 +9,7 @@ import (
"os/exec"
"path/filepath"
"github.com/kandev/kandev/internal/common/subproc"
"golang.org/x/sys/unix"
)
@@ -27,7 +28,7 @@ func init() {
// CommandContext creates a Git initialization command bound to targetDirectory.
func CommandContext(ctx context.Context, _ string, targetDirectory *os.File) (*exec.Cmd, error) {
gitPath, err := exec.LookPath("git")
gitPath, err := subproc.GitExecutablePath()
if err != nil {
return nil, fmt.Errorf("find git: %w", err)
}
@@ -52,9 +53,9 @@ func runInheritedDirectory(gitPath string) int {
fmt.Fprintf(os.Stderr, "git init helper: enter inherited directory: %v\n", err)
return 1
}
if err := unix.Exec(
if err := subproc.ExecGit(
gitPath,
[]string{"git", "init", "--initial-branch=main"},
[]string{"init", "--initial-branch=main"},
withoutHelperEnvironment(os.Environ()),
); err != nil {
fmt.Fprintf(os.Stderr, "git init helper: execute git: %v\n", err)
@@ -7,11 +7,13 @@ import (
"fmt"
"os"
"os/exec"
"github.com/kandev/kandev/internal/common/subproc"
)
// CommandContext creates a Git initialization command for platforms without inherited fd support.
func CommandContext(ctx context.Context, targetPath string, _ *os.File) (*exec.Cmd, error) {
command := exec.CommandContext(ctx, "git", "init", "--initial-branch=main")
command := subproc.NewGitCommand(ctx, "init", "--initial-branch=main")
command.Dir = targetPath
return command, nil
}
@@ -3,7 +3,6 @@ package handlers
import (
"context"
"net/http"
"os/exec"
"strings"
"github.com/gin-gonic/gin"
@@ -13,6 +12,7 @@ import (
"github.com/kandev/kandev/internal/agent/remoteauth"
"github.com/kandev/kandev/internal/agent/runtime/lifecycle"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/task/dto"
"github.com/kandev/kandev/internal/task/models"
"github.com/kandev/kandev/internal/task/service"
@@ -137,8 +137,9 @@ func firstNonEmptyGitConfig(key string) string {
}
func runGitConfig(args ...string) string {
cmd := exec.Command("git", args...)
out, err := cmd.Output()
ctx := context.Background()
cmd := subproc.NewGitCommand(ctx, args...)
out, err := subproc.RunGitOutputClass(ctx, subproc.GitInteractive, cmd)
if err != nil {
return ""
}
@@ -7,6 +7,8 @@ import (
"os/exec"
"regexp"
"strings"
"github.com/kandev/kandev/internal/common/subproc"
)
// gitRefRe accepts the conservative subset of git ref names we expose to user
@@ -147,8 +149,18 @@ func discardLocalChanges(ctx context.Context, repoPath string) error {
}
func runGit(ctx context.Context, dir string, args ...string) (string, error) {
cmd := exec.CommandContext(ctx, "git", args...)
cmd.Dir = dir
out, err := cmd.CombinedOutput()
return strings.TrimSpace(string(out)), err
out, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire(
ctx,
subproc.GitInteractive,
branchFetchTimeout,
func(execCtx context.Context) *exec.Cmd {
cmd := subproc.NewGitCommand(execCtx, args...)
cmd.Dir = dir
return cmd
},
)
if runErr == nil {
runErr = execCtxErr
}
return strings.TrimSpace(string(out)), runErr
}
@@ -13,6 +13,7 @@ import (
"runtime"
"strings"
"github.com/kandev/kandev/internal/common/subproc"
"github.com/kandev/kandev/internal/task/gitinit"
"github.com/kandev/kandev/internal/task/models"
"go.uber.org/zap"
@@ -204,7 +205,7 @@ func initializeGitRepository(ctx context.Context, targetPath string, targetDirec
if err != nil {
return err
}
output, err := command.CombinedOutput()
output, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, command)
if err != nil {
return fmt.Errorf("%w: %s", err, strings.TrimSpace(string(output)))
}
@@ -7,12 +7,12 @@ import (
"io/fs"
"net/url"
"os"
"os/exec"
"path/filepath"
"sort"
"strings"
"github.com/kandev/kandev/internal/common/gitref"
"github.com/kandev/kandev/internal/common/subproc"
)
type RepositoryDiscoveryConfig struct {
@@ -695,9 +695,9 @@ func isHexCommitSHA(s string) bool {
// keep only the target since that's what's currently in the working tree.
// Returns an empty slice for a clean working tree.
func readGitDirtyFiles(ctx context.Context, repoPath string) ([]string, error) {
cmd := exec.CommandContext(ctx, "git", "status", "--porcelain=v1", "-z")
cmd := subproc.NewGitCommand(ctx, "status", "--porcelain=v1", "-z")
cmd.Dir = repoPath
out, err := cmd.Output()
out, err := subproc.RunGitOutputClass(ctx, subproc.GitInteractive, cmd)
if err != nil {
return nil, fmt.Errorf("git status: %w", err)
}
@@ -8,6 +8,7 @@ import (
"sync"
"time"
"github.com/kandev/kandev/internal/common/subproc"
"go.uber.org/zap"
"golang.org/x/sync/singleflight"
)
@@ -91,14 +92,21 @@ func (f *branchFetcher) checkCooldown(repoPath string) (BranchRefreshResult, boo
}
func (f *branchFetcher) runFetch(ctx context.Context, repoPath string) BranchRefreshResult {
fetchCtx, cancel := context.WithTimeout(ctx, branchFetchTimeout)
defer cancel()
cmd := newNonInteractiveGitFetchCmd(fetchCtx, repoPath)
output, err := cmd.CombinedOutput()
output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire(
ctx,
subproc.GitInteractive,
branchFetchTimeout,
func(execCtx context.Context) *exec.Cmd {
return newNonInteractiveGitFetchCmd(execCtx, repoPath)
},
)
res := BranchRefreshResult{FetchedAt: time.Now()}
if err != nil {
res.Err = fmt.Errorf("git fetch: %w", err)
if runErr != nil || execCtxErr != nil {
effectiveErr := runErr
if effectiveErr == nil {
effectiveErr = execCtxErr
}
res.Err = fmt.Errorf("git fetch: %w", effectiveErr)
if f.logger != nil {
// Avoid logging raw git output: it can contain remote URLs with
// embedded credentials or other sensitive data. Surface only the
@@ -106,7 +114,7 @@ func (f *branchFetcher) runFetch(ctx context.Context, repoPath string) BranchRef
f.logger.Warn("branch refresh fetch failed",
zap.String("path", repoPath),
zap.Int("output_bytes", len(output)),
zap.Error(err))
zap.Error(res.Err))
}
}
return res
@@ -117,7 +125,7 @@ func (f *branchFetcher) runFetch(ctx context.Context, repoPath string) BranchRef
// manager. Auth prompts are disabled so missing credentials surface as fast
// failures rather than hanging the dropdown.
func newNonInteractiveGitFetchCmd(ctx context.Context, repoPath string) *exec.Cmd {
cmd := exec.CommandContext(ctx, "git", "-C", repoPath, "fetch", "--all", "--prune", "--no-tags")
cmd := subproc.NewGitCommand(ctx, "-C", repoPath, "fetch", "--all", "--prune", "--no-tags")
cmd.Env = append(os.Environ(),
"GIT_TERMINAL_PROMPT=0",
"GCM_INTERACTIVE=Never",
@@ -3,6 +3,8 @@ package worktree
import (
"context"
"os/exec"
"github.com/kandev/kandev/internal/common/subproc"
)
// newGitCommand enables Git's long-path handling for this process only. Git
@@ -13,5 +15,5 @@ func newGitCommand(ctx context.Context, args ...string) *exec.Cmd {
commandArgs := make([]string, 0, len(args)+2)
commandArgs = append(commandArgs, "-c", "core.longpaths=true")
commandArgs = append(commandArgs, args...)
return exec.CommandContext(ctx, "git", commandArgs...)
return subproc.NewGitCommand(ctx, commandArgs...)
}
@@ -24,21 +24,15 @@ import (
// anywhere we exec a git binary from the worktree package — calling
// cmd.Run() directly bypasses the throttle.
func runGitCmd(ctx context.Context, cmd *exec.Cmd) error {
return subproc.RunGit(ctx, cmd)
return subproc.RunGitClass(ctx, subproc.GitLifecycle, cmd)
}
// runGitCmdCombinedOutput is runGitCmd's CombinedOutput sibling.
func runGitCmdCombinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
return subproc.RunGitCombinedOutput(ctx, cmd)
return subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd)
}
// runGitCmdOutput is runGitCmd's Output sibling.
func runGitCmdOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
return subproc.RunGitOutput(ctx, cmd)
}
// setGitThrottleCapForTest swaps the git throttle pool to the given
// capacity and returns a restore closure. Test-only.
func setGitThrottleCapForTest(cap int) func() {
return subproc.Git().SetCapForTest(cap)
return subproc.RunGitOutputClass(ctx, subproc.GitLifecycle, cmd)
}
@@ -29,13 +29,13 @@ func TestRunGitCmd_SaturatedThrottleBlocks(t *testing.T) {
defer restore()
// Saturate the pool out-of-band so the next runGitCmd is forced to
// wait. Holding the slots via gitThrottle.Acquire (the same pool
// wait. Holding the slots via class-aware AcquireGit (the same pool
// runGitCmd uses) is the cleanest way to set this up without timing
// against a real subprocess. Capture each release so the slots are
// returned to the pool when the test ends — keeps the package-wide
// gitThrottle clean for any subsequent test in the same run.
for i := 0; i < cap; i++ {
release, err := subproc.Git().Acquire(context.Background())
release, err := subproc.AcquireGit(context.Background(), subproc.GitLifecycle)
if err != nil {
t.Fatalf("pre-saturate acquire %d: %v", i, err)
}
@@ -53,6 +53,22 @@ func TestRunGitCmd_SaturatedThrottleBlocks(t *testing.T) {
}
}
func TestRunGitCmd_LifecycleAdmission(t *testing.T) {
restore := setGitThrottleCapForTest(1)
defer restore()
hold, err := subproc.AcquireGit(context.Background(), subproc.GitLifecycle)
if err != nil {
t.Fatalf("hold lifecycle slot: %v", err)
}
defer hold()
ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
defer cancel()
if err := runGitCmd(ctx, exec.Command("sh", "-c", "true")); !errors.Is(err, context.DeadlineExceeded) {
t.Fatalf("runGitCmd error = %v, want context.DeadlineExceeded", err)
}
}
// TestRunGitCmd_RunsToCompletionWhenCapacityAvailable is the positive
// sibling: with capacity free, runGitCmd executes the command and
// returns its result. Guards against a regression where a refactor
@@ -74,6 +90,10 @@ func TestRunGitCmd_RunsToCompletionWhenCapacityAvailable(t *testing.T) {
wg.Wait()
}
func setGitThrottleCapForTest(cap int) func() {
return subproc.Git().SetCapForTest(cap)
}
func TestNewNonInteractiveGitCmd_EnablesLongPathsPerCommand(t *testing.T) {
cmd := (&Manager{}).newNonInteractiveGitCmd(context.Background(), t.TempDir(), "status", "--short")
@@ -45,7 +45,7 @@ func (m *Manager) branchExists(ctx context.Context, repoPath, branch string) (bo
// contention. With this ordering the 10s timer starts the moment
// git is about to run, so we get an accurate "could not tell" only
// when the inspect itself is the slow part.
release, err := subproc.Git().Acquire(ctx)
release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle)
if err != nil {
m.logger.Warn("branchExists bounded by context",
zap.String("repository_path", repoPath),
@@ -120,7 +120,7 @@ func (m *Manager) BranchRecoveryStatus(ctx context.Context, repoPath, branch str
func (m *Manager) currentBranch(ctx context.Context, repoPath string) string {
// Same Acquire-then-build-execCtx ordering as branchExists.
release, err := subproc.Git().Acquire(ctx)
release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle)
if err != nil {
return ""
}
@@ -289,7 +289,7 @@ func (m *Manager) pullCurrentBranchOrFallback(
func (m *Manager) runGitCombinedAfterAcquire(
ctx context.Context, execTimeout time.Duration, repoPath string, args ...string,
) ([]byte, error, error) {
release, err := subproc.Git().Acquire(ctx)
release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle)
if err != nil {
return nil, err, ctx.Err()
}
@@ -0,0 +1,97 @@
# Class-Aware Git Subprocess Admission
- **Status:** Accepted
- **Date:** 2026-08-02
- **Scope:** Backend and agentctl Git subprocess scheduling
- **Related spec:** [Git Subprocess Admission](../specs/platform/git-subprocess-admission.md)
- **Related issue:** [#2150](https://github.com/kdlbs/kandev/issues/2150)
## Context
Kandev has a process-wide semaphore that caps Git subprocess concurrency. All Git
callers enter the same queue, while some API handlers also impose their own
fan-out limit. This bounds peak process creation but does not guarantee progress
for workspace polling or repository lifecycle work during sustained interactive
traffic. It also leaves callers to decide whether admission time consumes a
command timeout.
The admission policy is a cross-cutting platform contract: every backend and
agentctl Git call must use it, future call sites must select the correct behavior,
and operators need to identify which kind of work is creating contention.
## Decision
Kandev will retain one process-local hard cap and make Git admission class-aware.
1. Every production Git operation declares `interactive`, `lifecycle`, or
`background` work when it enters the shared controller.
2. Each class uses a FIFO queue. On each released slot, the controller selects the
next non-empty class by deterministic round robin in this order:
`interactive`, `lifecycle`, `background`.
3. The scheduler is work-conserving. A class may use the entire capacity when no
other class is waiting; there are no reserved slots or independent quotas.
4. The global cap remains the only authority on active Git subprocess count and
continues to use `KANDEV_GIT_MAX_CONCURRENT`.
5. Admission uses the caller lifetime context. Per-command execution timeouts are
created only after admission succeeds.
6. Public Git wrappers require a work class. Classless production helpers are
removed or made inaccessible so new call sites cannot silently join the wrong
queue.
7. Aggregate metrics are preserved and supplemented by per-class admission
metrics and a structured snapshot exposed through authenticated diagnostics.
8. Multi-repository status, log, and diff handlers derive their local worker bound
from the shared Git capacity. Local fan-out controls queued work; it does not
create a second concurrency policy.
## Why deterministic round robin
Round robin gives every continuously queued class a simple bounded-progress rule
without tuning weights that lack workload evidence. It also remains
work-conserving: interactive work keeps full throughput when lifecycle and
background queues are empty. FIFO within a class makes cancellation and ordering
testable and avoids priority inversions inside a class.
## Alternatives considered
### Raise the global limit
Rejected as the scheduling policy. A larger cap may move the saturation point but
does not prevent one workload from excluding another, and it increases peak
process pressure on Windows.
### Keep only local fan-out caps
Rejected because concurrent requests and non-HTTP Git callers still meet at the
global semaphore. Independent local constants cannot enforce system-wide
progress or the total active-process bound.
### Reserve slots per class
Rejected because idle reservations reduce throughput and require choosing
platform-wide quota sizes without sufficient workload evidence.
### Strict interactive priority
Rejected because sustained user traffic could indefinitely delay workspace
monitoring and instance setup, recreating the liveness failure.
### Weighted scheduling
Deferred. It adds policy knobs and more complex guarantees. Deterministic round
robin is sufficient for the observed liveness problem; telemetry can justify a
future weight decision if needed.
## Consequences
- Git call sites become explicit about user-visible, lifecycle, or background
intent, making incorrect classification visible in code review.
- Every continuously queued class makes bounded progress once running commands
release slots, but the scheduler cannot preempt commands that are already
running.
- Admission and execution latency can be measured separately, preventing queue
delay from masquerading as repository failure.
- The controller becomes more complex than a semaphore and needs focused race,
cancellation, fairness, and accounting tests.
- Scheduling state remains process-local. Independent backend and agentctl
processes each enforce their own configured cap; cross-host coordination is not
introduced.
+1
View File
@@ -106,4 +106,5 @@ Read individual ADRs for full context. Create new ones via `/record decision` or
| 2026-08-02-new-workspace-github-access-defaults | [Bootstrap New Workspaces From Host GitHub Access](2026-08-02-new-workspace-github-access-defaults.md) | accepted | backend, frontend, security | 2026-08-02 |
| 2026-08-02-single-owner-agent-task-titles | [Assign Agent Task Titles to One Session](2026-08-02-single-owner-agent-task-titles.md) | accepted | backend, frontend, protocol, workflow | 2026-08-02 |
| 2026-08-02-agent-terminal-diagnostics-over-stderr | [Capture Agent Terminal Diagnostics From Managed Stderr](2026-08-02-agent-terminal-diagnostics-over-stderr.md) | accepted | backend, frontend, protocol, security | 2026-08-02 |
| 2026-08-02-class-aware-git-subprocess-admission | [Class-Aware Git Subprocess Admission](2026-08-02-class-aware-git-subprocess-admission.md) | accepted | backend, agentctl, observability | 2026-08-02 |
| 2026-08-02-isolate-replaceable-session-stream-traffic | [Isolate Replaceable Session Stream Traffic](2026-08-02-isolate-replaceable-session-stream-traffic.md) | accepted | backend, frontend, protocol | 2026-08-02 |
+235
View File
@@ -0,0 +1,235 @@
---
spec: docs/specs/platform/git-subprocess-admission.md
created: 2026-08-02
status: implemented
---
# Implementation Plan: Git Subprocess Admission
## Overview
Replace the undifferentiated Git semaphore with one class-aware,
work-conserving admission controller, then migrate interactive, lifecycle, and
background call sites onto its typed API. Complete the repair by separating
workspace-poll admission from execution timeouts and exposing authenticated
contention diagnostics. Public Git response contracts and the existing process
cap remain unchanged.
Confirmed root cause: all Git work waits on one undifferentiated process-wide
pool, while multi-repository interactive fan-out can fill that pool. Workspace
polling creates its 10-second context before admission and treats the resulting
queue timeout like a Git failure, so repeated contention can stop an otherwise
healthy tracker. The merged fan-out limits reduce pressure but do not provide
cross-class progress or separate queue delay from execution failure.
---
## Backend
### Shared class-aware admission
- **Files:** `apps/backend/internal/common/subproc/throttle.go`,
`apps/backend/internal/common/subproc/shared.go`,
`apps/backend/internal/common/subproc/metrics.go`, and focused sibling tests.
- Add the following public contract (names may vary only to match established Go
naming in the package):
```go
type GitWorkClass string
const (
GitInteractive GitWorkClass = "interactive"
GitLifecycle GitWorkClass = "lifecycle"
GitBackground GitWorkClass = "background"
)
func AcquireGit(ctx context.Context, class GitWorkClass) (release func(), err error)
func GitCapacity() int
func AdmissionSnapshot() Snapshot
```
- Implement one mutex-protected FIFO per class and a round-robin cursor ordered
`interactive`, `lifecycle`, `background`. Skip empty queues and grant
immediately whenever unused capacity exists.
- Remove canceled waiters under the scheduler lock and make release idempotent.
Aggregate and per-class counters must be updated from the same synchronized
state.
- Require `GitWorkClass` on run/output/combined-output helpers and any
after-admission command builder. Keep generic `Throttle` for `gh`; retain old
Git entry points only during migration, then remove or privatize them after all
call sites are classified.
- Preserve `KANDEV_GIT_MAX_CONCURRENT` resolution. Add only a test-scoped capacity
seam; runtime resizing is not part of this repair.
### Interactive operations and repository fan-out
- **Files:** `apps/backend/internal/agentctl/server/api/git.go`,
`apps/backend/internal/agentctl/server/api/git_fanout_test.go`, and agentctl
process Git operators outside the polling path, including `git.go`,
`git_pr_providers.go`, `workspace_files.go`, `workspace_git_cmd.go`,
`workspace_git_commits.go`, and `workspace_git_diff.go`.
- Classify user-request status, log, diff, staging, branch, commit, fetch, pull,
push, file-history, and PR-support commands as `GitInteractive`.
- Replace the unbounded status goroutine burst and `maxGitFanout = 8` with one
helper that creates at most
`min(repository count, subproc.GitCapacity())` workers.
- Preserve indexed result ordering, response shapes, and partial-error behavior.
Admission remains authoritative for active subprocess count across overlapping
requests.
### Backend lifecycle operations
- **Files:** `apps/backend/internal/worktree/git_throttle.go`,
`apps/backend/internal/worktree/manager_git.go`,
`apps/backend/internal/repoclone/clone.go`, and
`apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go`.
- Classify cloning, fetching, branch preparation, worktree creation/removal,
cleanup, rescan, and local runtime preparation as `GitLifecycle`.
- Preserve arguments, credentials, error wrapping, rollback, and cleanup. Use
focused search in the owned backend packages as the migration completeness
check.
### Workspace tracker admission and timeout ownership
- **Files:** `apps/backend/internal/agentctl/server/process/workspace_monitor.go`,
`workspace_git_cmd.go`, `workspace_git_poll.go`, `workspace_tracker.go`, and
focused sibling tests.
- Classify initial index resolution, tracker setup, and explicit rescan as
`GitLifecycle`; classify repeating polls as `GitBackground`.
- Acquire with the tracker lifetime context. Only after admission succeeds,
create the existing 10-second execution context and construct/start Git.
- Return an internal admission-canceled result that does not increment
consecutive Git failures. Preserve the five-failure stop rule for genuine
post-admission failures and the existing missing-workspace behavior.
- After interactive, lifecycle, and poller migrations are complete, remove or
privatize the classless Git entry points and use a repository-wide production
search plus compilation as the final completeness check.
### Authenticated diagnostics
- **Files:** `apps/backend/internal/agentctl/server/api/control_server.go`,
`apps/backend/internal/agent/runtime/agentctl/control.go` with focused tests.
- Preserve aggregate expvar metrics and add class-keyed inflight, waiter,
acquisition, and cumulative admission-wait values plus an immutable snapshot.
- Add authenticated `GET /api/v1/debug/subprocess-admission` to the agentctl
control group and `ControlClient.SubprocessAdmission(ctx)` using its existing
bearer transport.
---
## Tests
The controller fairness test, mixed interactive/background tracker test, and
authenticated diagnostic integration test are written first and must fail on
current behavior for the missing class policy, timeout boundary, and endpoint
respectively before implementation turns them green.
- **What:** hard cap, same-class FIFO, deterministic class rotation,
work-conserving admission, cancellation, idempotent release, env capacity, and
metric/snapshot accounting.
**File:** `apps/backend/internal/common/subproc/*_test.go`.
**How:** table-driven tests with capacity 1–3 and channels controlling grants,
releases, and cancel-versus-release races; repeat concurrency cases under
`go test -race`.
- **What:** capacity-derived fan-out and unchanged status/log/diff order and
partial results.
**File:** `apps/backend/internal/agentctl/server/api/git_fanout_test.go` and
focused handler tests.
**How:** overlapping handler requests against temporary repositories through
the real Git operator; assert peak inflight and `interactive` attribution.
- **What:** backend lifecycle classification and cancellation cleanup.
**File:** focused tests in `internal/worktree`, `internal/repoclone`, and
`internal/agent/runtime/lifecycle`.
**How:** saturate admission, cancel preparation, and assert no command starts and
existing rollback remains intact.
- **What:** interactive saturation cannot turn a queued poll into a Git failure.
**File:** `apps/backend/internal/agentctl/server/process/workspace_*_test.go`.
**How:** use a cross-platform helper process, hold interactive work, enqueue a
poll, release slots, and assert bounded execution, unchanged failure count, and
zero leaked slots/waiters.
- **What:** authenticated diagnostic path.
**File:** control-server and `ControlClient` tests.
**How:** exercise route → bearer-authenticated client and assert authorized,
unauthorized, and snapshot response behavior.
- **What:** before/after Windows evidence required to close issue #2150.
**File:** attach results to the issue or implementation PR; no permanent host
fixture.
**How:** with cap 12, run the same multi-repository workload on current `main`
and the completed branch; record p50/p95/max latency, admission/execution
timeouts, tracker stops, and peak Git process count.
---
## Verification Results
Implementation checks are recorded in the task files. The focused admission,
diagnostics, poller race tests, production classless-call audit, and
`make -C apps/backend build` all pass. The Windows before/after workload
comparison required to close issue #2150 remains an external evidence step and
is not available in this Linux worktree.
The affected backend package sweep also passes: `go test ./internal/common/subproc
./internal/agentctl/server/process ./internal/agentctl/server/api
./internal/worktree ./internal/agent/runtime/lifecycle
./internal/agent/runtime/agentctl ./internal/repoclone ./internal/debug
./internal/backendapp -count=1` — 2,340 tests across 9 packages.
Review-blocker validation also passes: the raw-Git repository guard is green,
the deterministic cancel/release boundary test is green under `go test -race
./internal/common/subproc`, and the fresh-status regression records only
`GitInteractive` admissions for every subprocess in that observation. A
compile-only sweep (`go test -run '^$' ./...`) is green after the final audit.
The repository-wide `go test -tags fts5 ./...` reached 10,254 passing tests and
29 skips but stopped on the pre-existing websocket assertion
`TestTaskEventBroadcaster_NoDuplicateSubscriptions` (62 subscriptions versus
its expected 61); the same isolated test fails without any files in this
change's scope.
---
## Implementation Waves And Parallel Candidates
Wave 1:
- [x] [task-01-class-aware-admission](task-01-class-aware-admission.md) — done
Wave 2 (parallel candidates — user authorization required):
- [x] [task-02-interactive-git-fanout](task-02-interactive-git-fanout.md) — done
- [x] [task-03-lifecycle-git-classification](task-03-lifecycle-git-classification.md) — done
- [x] [task-05-admission-diagnostics](task-05-admission-diagnostics.md) — done
Wave 3:
- [x] [task-04-poller-admission-liveness](task-04-poller-admission-liveness.md) — done
Wave 4:
- [x] [task-06-review-blockers](task-06-review-blockers.md) — done
The default is sequential execution in the primary conversation. Wave 2 is
parallel-safe only because the tasks own disjoint call-site, backend lifecycle,
and diagnostics files; waves do not authorize subagents.
---
## Risks and mitigations
| Risk | Mitigation |
| --- | --- |
| Cancellation races grant work after its caller exits | Queue removal, grant, and counters share one lock; targeted race tests cover cancel-versus-release |
| A forgotten call site remains classless | Remove or privatize classless Git access, audit raw command construction/lookup/exec paths, and enforce the repository guard |
| Round robin reduces single-class throughput | Scheduler skips empty queues and lets the only active class use full capacity |
| Fan-out refactor changes result order or partial errors | Preserve indexed result slots and add handler-level regression tests for all three operations |
| Diagnostics expose process-local state across runtimes | Keep the authenticated agentctl snapshot endpoint process-local and do not aggregate or persist it |
| Fair admission cannot help a permanently hung command | Keep post-admission execution timeouts and release slots on every terminal path |
## Completion criteria
- All task files have recorded passing validation commands.
- Production Git call sites compile only through a declared work class.
- Race tests cover the scheduler and tracker contention paths.
- Documentation links and indexes resolve.
- The Windows before/after evidence required by the spec is attached before
closing issue #2150.
@@ -0,0 +1,79 @@
---
id: "01-class-aware-admission"
title: "Class-aware admission controller"
status: done
wave: 1
plan: "plan.md"
spec: "../../specs/platform/git-subprocess-admission.md"
depends_on: []
---
# Task 01: Class-Aware Admission Controller
## Acceptance
- Active work never exceeds the configured capacity across mixed classes.
- A continuously queued class is selected within three successful releases while
all classes remain queued; FIFO, cancellation, and single-class full throughput
remain deterministic.
- Aggregate `git` metrics remain compatible, class snapshot values match scheduler
state, and the generic `gh` throttle is unchanged.
- The class-aware Git throttle rejects classless `Acquire` calls so production
callers cannot silently default to a scheduling class.
## Verification
```bash
# Direct Go commands are required because the backend Makefile has no focused or
# race-test targets.
cd apps/backend && go test ./internal/common/subproc -run 'Test(Class|GitAdmission|Throttle|Metrics)' -count=1
cd apps/backend && go test -race ./internal/common/subproc -run 'Test(Class|GitAdmission|Throttle|Metrics)' -count=1
```
## Files Likely Touched
- `apps/backend/internal/common/subproc/throttle.go`
- `apps/backend/internal/common/subproc/shared.go`
- `apps/backend/internal/common/subproc/metrics.go`
- `apps/backend/internal/common/subproc/admission.go`
- `apps/backend/internal/common/subproc/admission_test.go`
- Existing sibling throttle, shared, and metrics tests as needed
## Dependencies
None.
## Parallelism
Sequential. Every later task depends on this scheduler and typed API.
## Inputs
- Spec sections: one global hard cap, explicit work classes, fair
work-conserving admission, and diagnostics.
- Plan section: Shared class-aware admission.
- Existing `Throttle`, environment capacity resolver, and expvar compatibility in
`internal/common/subproc`.
- ADR: `docs/decisions/2026-08-02-class-aware-git-subprocess-admission.md`.
## Output Contract
Report red tests, scheduler/API shape, metric compatibility, files changed,
exact command outcomes, race-test evidence, residual risks, and synchronized
task/plan status.
## Results
- RED: `TestGitAdmissionPublishesClassMetrics` failed because the class-level
expvar maps did not exist.
- GREEN: added class-aware FIFO queues, deterministic round-robin admission,
cancellation removal, idempotent release, capacity test seam, typed Git
admission, snapshots, and additive per-class metrics.
- `cd apps/backend && go test ./internal/common/subproc -run 'Test(Class|GitAdmission|Throttle|Metrics)' -count=1` — 17 passed.
- `cd apps/backend && go test -race ./internal/common/subproc -run 'Test(Class|GitAdmission|Throttle|Metrics)' -count=1` — 17 passed.
- `cd apps/backend && go test ./internal/common/subproc -count=1` — 30 passed.
- `cd apps/backend && go test ./internal/common/subproc ./internal/worktree ./internal/agentctl/server/process ./internal/repoclone ./internal/agent/runtime/lifecycle -run '^$'` — all affected packages compiled.
- Classless Git run wrappers were removed after the Task 04 production
call-site audit, and class-aware `Throttle.Acquire` now returns
`ErrGitWorkClassRequired`; generic `Throttle`/`gh` compatibility behavior
remains unchanged.
@@ -0,0 +1,78 @@
---
id: "02-interactive-git-fanout"
title: "Interactive Git fan-out"
status: done
wave: 2
plan: "plan.md"
spec: "../../specs/platform/git-subprocess-admission.md"
depends_on:
- "01-class-aware-admission"
---
# Task 02: Interactive Git Fan-Out
## Acceptance
- Status, log, and diff share one capacity-derived worker policy.
- Multiple simultaneous handler requests never exceed the process-wide Git cap.
- All user-request agentctl Git commands declare `interactive`, with no classless
or hardcoded secondary concurrency path.
- Existing per-repository order, response schemas, and partial results remain
unchanged.
## Verification
```bash
# Direct Go commands are required because the backend Makefile has no focused or
# race-test targets.
cd apps/backend && go test ./internal/agentctl/server/api ./internal/agentctl/server/process -run 'Test(FanOutRepos|Git.*Admission|MultiRepo.*Class)' -count=1
cd apps/backend && go test -race ./internal/agentctl/server/api ./internal/agentctl/server/process -run 'Test(FanOutRepos|Git.*Admission|MultiRepo.*Class)' -count=1
```
## Files Likely Touched
- `apps/backend/internal/agentctl/server/api/git.go`
- `apps/backend/internal/agentctl/server/api/git_fanout_test.go`
- `apps/backend/internal/agentctl/server/process/git.go`
- `apps/backend/internal/agentctl/server/process/git_pr_providers.go`
- `apps/backend/internal/agentctl/server/process/workspace_files.go`
- `apps/backend/internal/agentctl/server/process/workspace_git_cmd.go`
- `apps/backend/internal/agentctl/server/process/workspace_git_commits.go`
- `apps/backend/internal/agentctl/server/process/workspace_git_diff.go`
- Focused sibling tests for migrated operators
## Dependencies
Task 01 (`01-class-aware-admission`).
## Parallelism
Parallel-safe with Tasks 03 and 05 after Task 01. This task owns agentctl Git API
and non-poller process files; Task 03 owns backend lifecycle packages and Task 05
owns control/debug files.
## Inputs
- Spec sections: explicit work classes and consistent multi-repository behavior.
- Plan section: Interactive operations and repository fan-out.
- Existing indexed fan-out behavior in `api/git.go` and typed admission API from
Task 01.
## Output Contract
Report red tests, classification audit, fan-out worker rule, preserved response
semantics, files changed, exact command outcomes, race-test evidence, residual
risks, and synchronized task/plan status.
## Results
- RED: `TestFanOutReposRespectsLimit` failed after deriving its expected bound
from the configured capacity; the old fixed fan-out admitted five workers with
capacity three.
- GREEN: status, log, and cumulative-diff fan-out now derive their worker limit
from `subproc.GitCapacity()` while preserving indexed result order.
- User-triggered Git operator, PR-provider, workspace file, and content-at-ref
operations use `GitInteractive`; tracker-owned background paths were completed
by Task 04 with `GitBackground` admission and post-admission timeouts.
- `cd apps/backend && go test ./internal/agentctl/server/api ./internal/agentctl/server/process -run 'Test(FanOutRepos|Git.*Admission|MultiRepo.*Class)' -count=1` — 3 passed.
- `cd apps/backend && go test -race ./internal/agentctl/server/api ./internal/agentctl/server/process -run 'Test(FanOutRepos|Git.*Admission|MultiRepo.*Class)' -count=1` — 3 passed.
@@ -0,0 +1,74 @@
---
id: "03-lifecycle-git-classification"
title: "Lifecycle Git classification"
status: done
wave: 2
plan: "plan.md"
spec: "../../specs/platform/git-subprocess-admission.md"
depends_on:
- "01-class-aware-admission"
---
# Task 03: Lifecycle Git Classification
## Acceptance
- Every backend lifecycle Git subprocess passes through `GitLifecycle` admission.
- No owned backend lifecycle package uses the old classless shared helper; the
GitHub CLI throttle remains unchanged.
- Lifecycle cancellation starts no subprocess and preserves arguments,
credentials, rollback, error wrapping, and cleanup guarantees.
## Verification
```bash
# Direct Go commands are required because the backend Makefile has no focused
# package target.
cd apps/backend && go test ./internal/worktree ./internal/agent/runtime/lifecycle ./internal/repoclone ./internal/task/service -run 'Test.*Git.*(Admission|Class|Cancel)' -count=1
cd apps/backend && go test ./internal/common/subproc ./internal/worktree ./internal/agent/runtime/lifecycle ./internal/repoclone ./internal/task/service -count=1
cd apps/backend && ! rg -n 'subproc\.Git\(\)' internal/worktree internal/repoclone internal/agent/runtime/lifecycle --glob '*.go' --glob '!*_test.go'
```
Compilation verifies the new required class argument on run helpers; the final
search verifies the owned packages no longer use the classless throttle accessor.
## Files Likely Touched
- `apps/backend/internal/worktree/git_throttle.go`
- `apps/backend/internal/worktree/manager_git.go`
- `apps/backend/internal/repoclone/clone.go`
- `apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go`
- Focused sibling tests for cancellation and classification
## Dependencies
Task 01 (`01-class-aware-admission`).
## Parallelism
Parallel-safe with Tasks 02 and 05 after Task 01. This task owns backend
lifecycle packages; the other tasks own agentctl API/process and diagnostics.
## Inputs
- Spec section: explicit work classes.
- Plan section: Backend lifecycle operations.
- Task 01 typed helpers and the lifecycle-package call-site search recorded in
the design investigation.
## Output Contract
Report the lifecycle call-site classification, red tests, files changed, exact
command outcomes, cleanup evidence, residual risks, and synchronized task/plan
status.
## Results
- Migrated worktree, repository-clone, and local runtime preparation Git calls to
`GitLifecycle`, including explicit acquire-before-execution-timeout paths.
- Moved the worktree capacity seam into its test file so production lifecycle
packages have no classless `subproc.Git()` usage.
- `cd apps/backend && go test ./internal/worktree ./internal/agent/runtime/lifecycle ./internal/repoclone ./internal/task/service -run 'Test.*Git.*(Admission|Class|Cancel)' -count=1` — 1 passed.
- `cd apps/backend && go test ./internal/common/subproc ./internal/worktree ./internal/agent/runtime/lifecycle ./internal/repoclone ./internal/task/service -count=1` — passed.
- `cd apps/backend && ! rg -n 'subproc\.Git\(\)' internal/worktree internal/repoclone internal/agent/runtime/lifecycle --glob '*.go' --glob '!*_test.go'` — no production matches.
- `cd apps/backend && go test -race ./internal/worktree ./internal/agent/runtime/lifecycle ./internal/repoclone ./internal/task/service -run 'Test.*Git.*(Admission|Class|Cancel)' -count=1` — 1 passed.
@@ -0,0 +1,87 @@
---
id: "04-poller-admission-liveness"
title: "Poller admission liveness"
status: done
wave: 3
plan: "plan.md"
spec: "../../specs/platform/git-subprocess-admission.md"
depends_on:
- "01-class-aware-admission"
- "02-interactive-git-fanout"
- "03-lifecycle-git-classification"
---
# Task 04: Poller Admission Liveness
## Acceptance
- Admission wait does not consume the Git execution timeout.
- Admission cancellation does not increment the tracker failure counter or stop a
healthy tracker; genuine post-admission failures retain the five-failure rule.
- Tracker setup uses `lifecycle`, repeating polls use `background`, and a queued
poll progresses within the bounded class rotation after slots release.
- No production package can access the classless Git throttle or run helpers once
all migrations finish.
## Verification
```bash
# Direct Go commands are required because the backend Makefile has no focused or
# race-test targets.
cd apps/backend && go test ./internal/agentctl/server/process -run 'TestWorkspace.*(Admission|Timeout|Monitor)|TestRunPollingGit' -count=1
cd apps/backend && go test -race ./internal/agentctl/server/process -run 'TestWorkspace.*(Admission|Timeout|Monitor)|TestRunPollingGit' -count=1
cd apps/backend && ! rg -n 'subproc\.Git\(\)' internal --glob '*.go' --glob '!*_test.go'
make -C apps/backend build
```
## Files Likely Touched
- `apps/backend/internal/agentctl/server/process/workspace_monitor.go`
- `apps/backend/internal/agentctl/server/process/workspace_git_cmd.go`
- `apps/backend/internal/agentctl/server/process/workspace_git_poll.go`
- `apps/backend/internal/agentctl/server/process/workspace_tracker.go`
- `apps/backend/internal/common/subproc/shared.go`
- Focused `workspace_*_test.go` files and cross-platform helper-process fixture
## Dependencies
Tasks 01 (`01-class-aware-admission`), 02 (`02-interactive-git-fanout`), and 03
(`03-lifecycle-git-classification`).
## Parallelism
Sequential after Wave 2. This completes the agentctl process migration and its
mixed interactive/background tests overlap Task 02's package ownership.
## Inputs
- Spec sections: admission and execution are different phases; interactive load
cannot disable workspace tracking.
- Plan section: Workspace tracker admission and timeout ownership.
- Existing 10-second polling context, five-consecutive-failure behavior, and the
completed production call-site migrations from Tasks 02 and 03.
## Output Contract
Report red contention evidence, admission-versus-execution result semantics,
classless-API removal/search evidence, files changed, exact command outcomes,
race-test and cleanup evidence, preserved failure behavior, residual risks, and
synchronized task/plan status.
## Results
- RED: `TestWorkspaceGitAdmissionWaitDoesNotConsumeTimeout` observed the
queued command inheriting the already-expired 100 ms context and returning
`context deadline exceeded` before the admission/timeout split.
- GREEN: polling and tracker setup now use explicit `background`/`lifecycle`
classes, execution timers start only after admission, and admission
cancellation resets rather than advances the tracker failure counter.
Classless Git run wrappers were removed after the production call-site audit;
the class-aware throttle also rejects classless `Acquire` calls.
- `cd apps/backend && go test ./internal/agentctl/server/process -run 'TestWorkspace.*(Admission|Timeout|Monitor)|TestRunPollingGit' -count=1` — 2 passed.
- `cd apps/backend && go test -race ./internal/agentctl/server/process -run 'TestWorkspace.*(Admission|Timeout|Monitor)|TestRunPollingGit' -count=1` — 2 passed.
- `cd apps/backend && ! rg -n 'subproc\.Git\(\)' internal --glob '*.go' --glob '!*_test.go'` — no production matches.
- `make -C apps/backend build` — passed for agentctl, backend, and companion
binaries (the environment reported only the existing codesign-tool warning).
- Admission helper tests additionally cover timeout freshness and wrapped
cancellation: `cd apps/backend && go test -race ./internal/common/subproc -run 'TestRunGitOutputAfterAcquire|TestClassAdmission' -count=1` — 8 passed.
@@ -0,0 +1,69 @@
---
id: "05-admission-diagnostics"
title: "Admission diagnostics"
status: done
wave: 2
plan: "plan.md"
spec: "../../specs/platform/git-subprocess-admission.md"
depends_on:
- "01-class-aware-admission"
---
# Task 05: Admission Diagnostics
## Acceptance
- The agentctl snapshot is inaccessible without its existing bearer credential.
- The authenticated agentctl endpoint reports the process-local admission
snapshot without exposing or changing the bearer credential.
- Metrics distinguish inflight work, waiters, acquisitions, and wait time per
class while preserving aggregate Git keys and adding no mutable settings or UI.
## Verification
```bash
# Direct Go commands are required because the backend Makefile has no focused or
# race-test targets.
cd apps/backend && go test ./internal/agentctl/server/api ./internal/agent/runtime/agentctl -run 'Test.*SubprocessAdmission' -count=1
cd apps/backend && go test -race ./internal/agentctl/server/api ./internal/agent/runtime/agentctl -run 'Test.*SubprocessAdmission' -count=1
```
## Files Likely Touched
- `apps/backend/internal/agentctl/server/api/control_server.go`
- Focused agentctl control-server tests
- `apps/backend/internal/agent/runtime/agentctl/control.go`
- Focused `ControlClient` tests
## Dependencies
Task 01 (`01-class-aware-admission`).
## Parallelism
Parallel-safe with Tasks 02 and 03 after Task 01. This task owns control-server,
control-client, debug, and backend wiring files, not Git call sites.
## Inputs
- Spec sections: diagnostics, API and permissions, persistence guarantees.
- Plan section: Authenticated diagnostics.
- Existing agentctl bearer middleware and `ControlClient` auth transport.
## Output Contract
Report red authorization tests, response shape, files changed, exact command
outcomes, race-test evidence, security boundary, residual risks, and synchronized
task/plan status.
## Results
- RED: the authenticated control-server test returned 404 for the new
admission route and the client test failed before the route/client contract
existed.
- GREEN: added the bearer-protected agentctl admission snapshot route, the
authenticated `ControlClient` probe, and standalone/lifecycle provider wiring.
- `cd apps/backend && go test ./internal/agentctl/server/api ./internal/agent/runtime/agentctl -run 'Test.*SubprocessAdmission' -count=1` — passed.
- `cd apps/backend && go test -race ./internal/agentctl/server/api ./internal/agent/runtime/agentctl -run 'Test.*SubprocessAdmission' -count=1` — passed.
- The endpoint uses the existing control-server bearer middleware and returns
only the owning agentctl process's in-memory snapshot.
@@ -0,0 +1,90 @@
---
id: "06-review-blockers"
title: "Close admission review blockers"
status: done
wave: 4
plan: "plan.md"
spec: "../../specs/platform/git-subprocess-admission.md"
depends_on:
- "01-class-aware-admission"
- "02-interactive-git-fanout"
- "03-lifecycle-git-classification"
- "04-poller-admission-liveness"
---
# Task 06: Close Admission Review Blockers
## Root Cause
The first migration audited the old `subproc.Git()` accessor but not every raw
`exec.Command*(..., "git", ...)` invocation. The scheduler also treated a
cancel-versus-release interleaving as a successful grant, and fresh status used
the background-only polling helper for its final porcelain command.
## Acceptance
- Every production Git command starts through an explicitly classified shared
helper; a repository guard rejects raw Git execution outside tests and the
approved command-construction seam.
- A waiter canceled at the grant boundary returns `ErrAdmissionCanceled` (and
the context error), starts no command, and leaves the round-robin cursor and
slot accounting correct.
- Every subprocess in a fresh interactive status observation is admitted as
`GitInteractive`; polling commands remain `GitBackground` and retain
`GIT_OPTIONAL_LOCKS=0`.
## Verification
```bash
cd apps/backend && go test ./internal/common/subproc -run 'Test(ClassAdmission|GitAdmission|Throttle|RawGit|FreshStatus)' -count=1
cd apps/backend && go test -race ./internal/common/subproc -run 'Test(ClassAdmission|GitAdmission|Throttle|RawGit|FreshStatus)' -count=1
cd apps/backend && go test ./internal/agentctl/server/process -run 'Test(.*FreshStatus|.*Admission|.*Poll)' -count=1
cd apps/backend && go test -race ./internal/agentctl/server/process -run 'Test(.*FreshStatus|.*Admission|.*Poll)' -count=1
cd apps/backend && go test ./internal/agent/runtime/lifecycle ./internal/backendapp ./internal/agentctl/server/api -run 'Test.*(Git|Admission|Materialize|Worktree)' -count=1
cd apps/backend && rg -n 'exec\.(Command|CommandContext)\([^\n]*"git"|exec\.LookPath\("git"\)|\b(unix|syscall)\.Exec\(' . --glob '*.go' --glob '!*_test.go'
```
## Files
- `apps/backend/internal/common/subproc/admission.go`
- `apps/backend/internal/common/subproc/admission_test.go`
- `apps/backend/internal/common/subproc/raw_git_audit_test.go`
- `apps/backend/internal/agentctl/server/process/workspace_git_cmd.go`
- `apps/backend/internal/agentctl/server/process/workspace_git_status.go`
- `apps/backend/cmd/agentctl/github_cli_shim.go`
- `apps/backend/cmd/mock-agent/scenarios.go`
- direct Git call sites identified by the repository audit
- this task's sibling tests and the shared command helpers
## Results
- Migrated remaining production Git construction paths (task services, local
repository initialization, worktree helpers, agentctl materialization, and
lifecycle helpers) to the shared command seam and classified runners.
- Added a repository AST guard that rejects raw Git command construction,
executable lookup, and direct `unix/syscall.Exec` outside the `subproc` seam.
- Made cancellation win both queue-head dispatch and the post-grant select
boundary; canceled grants release their slot before returning the context
error.
- Split lock suppression from scheduling class so fresh status remains
`GitInteractive` while background polling remains `GitBackground`.
- Added deterministic cancellation/release and fresh-status class-attribution
regression tests.
Validation:
```text
go test ./internal/common/subproc ./internal/task/gitinit ./internal/task/service -run 'TestProductionGitCommandsUseTheAdmissionSeam|TestClassAdmissionCancellationWinsReleaseBoundary|TestClassAdmissionCancellationDoesNotAdvanceQueue|TestPerformFreshBranch|TestInitializeLocalRepository' -count=1 # pass
go test -race ./internal/common/subproc -count=1 # 32 tests pass
go test ./internal/agentctl/server/process ./internal/common/subproc ./internal/task/service ./internal/task/gitinit ./internal/backendapp ./internal/agent/runtime/lifecycle ./internal/agentctl/server/api ./internal/worktree ./internal/repoclone ./internal/office/configloader ./internal/office/skills ./internal/office/testharness ./internal/orchestrator/executor -count=1 # pass
go test ./cmd/agentctl ./cmd/mock-agent -count=1 # 259 tests pass
go test -run '^$' ./... # compile pass
make -C apps/backend build # pass
```
The repository-wide `go test -tags fts5 ./...` reached 10,254 passing tests,
29 skips, and one pre-existing failure in
`TestTaskEventBroadcaster_NoDuplicateSubscriptions` (62 subscriptions versus
61). The isolated websocket test reproduces the same failure outside this
change's scope (`go test -tags fts5 ./internal/gateway/websocket -run
TestTaskEventBroadcaster_NoDuplicateSubscriptions -count=1`).
+1
View File
@@ -44,6 +44,7 @@ Product-wide capabilities that are not tied to a single feature area.
| [plugins — marketplace](plugins/marketplace.md) | building |
| [semantic-notifications](platform/notifications.md) | shipped |
| [workspace-git-status](platform/workspace-git-status.md) | shipped |
| [git-subprocess-admission](platform/git-subprocess-admission.md) | building |
| [bounded-task-status-delivery](platform/bounded-task-status-delivery.md) | approved |
| [diagnostic-logging](platform/diagnostic-logging.md) | approved |
@@ -0,0 +1,210 @@
---
title: Git Subprocess Admission
status: building
created: 2026-08-02
owner: kandev
---
# Git Subprocess Admission
Related issue: [#2150](https://github.com/kdlbs/kandev/issues/2150)
## Why
Kandev limits concurrent Git subprocesses, but every caller currently competes in
one undifferentiated pool. A burst of interactive multi-repository log, diff, or
status work can occupy the pool long enough for workspace monitoring and
instance setup to wait behind it. Workspace polling also starts its execution
timeout before admission, so queue delay can be reported as a Git failure and
eventually stop a healthy tracker.
The existing fan-out caps reduce the frequency of saturation, but they do not
define which work may progress under contention or distinguish admission delay
from command failure. The platform needs one enforceable concurrency boundary
with deterministic progress for every kind of Git work.
## What
### One global hard cap
- All Git subprocesses in a Kandev process MUST pass through one process-wide
admission controller.
- Active Git subprocesses MUST never exceed `KANDEV_GIT_MAX_CONCURRENT`.
- The existing default and environment override remain unchanged by this work.
- A caller that is the only queued work MAY use all available capacity. Separate
per-class pools that can exceed the global cap are not permitted.
### Explicit work classes
Every production Git operation MUST declare one of these classes:
| Class | Work included |
| --- | --- |
| `interactive` | User-observed Git requests, including status, log, diff, staging, commits, and pushes |
| `lifecycle` | Repository, worktree, runtime, tracker setup, cleanup, rescans, and instance construction |
| `background` | Periodic workspace polling and detached refresh work |
Classless production helpers are not part of the supported API. Tests MAY use a
dedicated capacity seam without weakening production classification.
### Fair, work-conserving admission
- Each class has a FIFO wait queue.
- Released slots are assigned in deterministic round-robin order across non-empty
classes: `interactive`, then `lifecycle`, then `background`, then repeat.
- Empty classes are skipped, so available capacity is never reserved for work
that does not exist.
- When all three classes remain queued and admitted commands complete, each class
receives a slot within at most three successful slot releases.
- Already-running commands are not preempted.
- A canceled waiter is removed promptly and MUST NOT consume a slot or start a
subprocess.
### Admission and execution are different phases
- Admission waits under the lifetime context supplied by the caller.
- A command-specific execution timeout starts only after admission succeeds and
before the subprocess starts.
- Admission cancellation or queue delay MUST NOT be counted as a Git command
failure and MUST NOT advance the workspace tracker's permanent-failure counter.
- A Git failure or timeout after admission still follows the caller's existing
error policy, releases its slot, and may advance the tracker failure counter.
- Missing or removed workspaces retain their existing stop behavior.
### Consistent multi-repository behavior
- Multi-repository status, log, and diff requests use one shared fan-out policy.
- Fan-out worker counts are derived from the configured Git capacity rather than
a separate hardcoded concurrency constant.
- The admission controller remains authoritative for active subprocess count;
local fan-out only bounds goroutine and queued-work creation.
- Existing HTTP and WebSocket response shapes, ordering guarantees, and partial
result behavior remain unchanged.
### Diagnostics
- Existing aggregate Git subprocess metrics remain available for compatibility.
- Admission metrics additionally expose, per class: active work, waiters,
admission attempts, and cumulative admission wait time.
- The agentctl control server exposes its process-wide admission snapshot through
an authenticated debug endpoint.
- Admission state is in-memory and resets when its owning process restarts.
## API and permissions
- Existing user-facing Git APIs are unchanged.
- Agentctl adds `GET /api/v1/debug/subprocess-admission` on its control server.
The route requires the same bearer token as instance-management routes and is
not added to the health or handshake authentication exemptions.
- The endpoint permits no changing of capacity, queues, work classes, or running
commands.
## Persistence guarantees
The controller, queues, counters, and structured snapshots are process-local
runtime state. They are not written to SQLite or restored after restart. The
configured capacity continues to come from the existing environment/default
resolution at process startup.
## Failure modes
- If a waiting request is canceled, it returns the context error without starting
Git and without disturbing the round-robin cursor.
- If Git cannot start after admission, the slot is released and the start error is
reported as an execution failure.
- If a running command exceeds its execution timeout, it is canceled, the slot is
released, and the normal post-admission failure policy applies.
- If the agentctl debug endpoint is unavailable, the caller receives the existing
control-client error; no admission state is persisted or inferred remotely.
## Scenarios
### Interactive load cannot disable workspace tracking
1. Interactive multi-repository requests keep their queue continuously populated.
2. A workspace poll enters the `background` queue.
3. The poll receives admission within the bounded round-robin window.
4. Its execution timeout begins after admission.
5. Queueing alone does not increase the tracker failure count or stop monitoring.
### Background work cannot starve instance setup
1. Background polling occupies and replenishes the available capacity.
2. Repository or tracker setup enters the `lifecycle` queue.
3. Lifecycle work receives a slot within the bounded round-robin window.
4. Other classes continue to use remaining slots without exceeding the global cap.
### Single-class throughput remains available
1. Only interactive work is queued.
2. Interactive work consumes every available slot.
3. No capacity remains idle solely to preserve a class reservation.
### Cancellation while queued is safe
1. A request waits while the global cap is full.
2. Its context is canceled before admission.
3. It leaves the queue promptly, starts no subprocess, and does not block the next
eligible waiter.
### Cancellation wins a simultaneous grant race
1. A queued request is canceled at the same scheduling boundary that releases a
slot.
2. The scheduler removes the canceled waiter before advancing the round-robin
cursor, or returns an admission-canceled result if cancellation wins after a
grant notification.
3. The canceled request starts no Git subprocess and does not consume a slot or
count as a post-admission failure.
### Fresh status keeps interactive classification
1. A user requests fresh status while the tracker is also capable of polling.
2. Every Git subprocess spawned for that fresh observation is admitted as
`interactive`, while periodic polling remains `background`.
3. Lock-suppression environment settings may differ by caller, but command
scheduling and per-class metrics reflect the user-visible request.
### Raw Git execution cannot bypass admission
1. A production code path constructs a Git command for lifecycle, interactive,
or background work.
2. It invokes a classified shared helper that acquires the process-wide Git
controller before starting the command.
3. A repository guard fails when production code directly runs a raw Git command
or uses an unclassified execution path.
## Out of scope
- Raising the default Git concurrency cap or introducing platform-specific defaults.
- Removing the process-wide cap or replacing Git subprocesses with a native library.
- Serializing all instance creation independently of Git admission.
- Changing public status, log, or diff payloads.
- Aggregating admission snapshots across Docker or remote agentctl processes.
- Guaranteeing wall-clock completion when Git itself hangs; execution timeouts
continue to bound commands after admission.
## Success criteria
- Deterministic tests prove the hard global cap, same-class FIFO order,
work-conserving behavior, cancellation safety, and bounded progress across all
three classes.
- A saturated interactive workload cannot stop a healthy workspace tracker due
only to admission delay.
- Saturated background work cannot starve interactive or lifecycle operations.
- Status, log, and diff fan-out share the same capacity-derived policy.
- Aggregate and per-class diagnostics attribute contention to admission wait versus
command execution.
- Before closing GitHub issue #2150, current `main` and the completed change are
measured on Windows with the same workload. Record p50, p95, and maximum
latency; timeout and tracker-stop counts; and peak Git process count with the
cap left at 12. Host-dependent values are evidence for closure, not fixed CI
thresholds.
## Decisions
- [Class-aware Git subprocess admission](../../decisions/2026-08-02-class-aware-git-subprocess-admission.md)
## Implementation plan
- [Git subprocess admission plan](../../plans/git-subprocess-admission/plan.md)