diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 646197aa6d..778b3b73c6 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -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). diff --git a/apps/backend/cmd/agentctl/github_cli_shim.go b/apps/backend/cmd/agentctl/github_cli_shim.go index 1862bf4771..a59b66ace6 100644 --- a/apps/backend/cmd/agentctl/github_cli_shim.go +++ b/apps/backend/cmd/agentctl/github_cli_shim.go @@ -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 } diff --git a/apps/backend/cmd/mock-agent/scenarios.go b/apps/backend/cmd/mock-agent/scenarios.go index 072695da24..20443362b8 100644 --- a/apps/backend/cmd/mock-agent/scenarios.go +++ b/apps/backend/cmd/mock-agent/scenarios.go @@ -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) } diff --git a/apps/backend/internal/agent/runtime/agentctl/control.go b/apps/backend/internal/agent/runtime/agentctl/control.go index 86241d1f5a..0fc790bb1a 100644 --- a/apps/backend/internal/agent/runtime/agentctl/control.go +++ b/apps/backend/internal/agent/runtime/agentctl/control.go @@ -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) diff --git a/apps/backend/internal/agent/runtime/agentctl/control_admission_test.go b/apps/backend/internal/agent/runtime/agentctl/control_admission_test.go new file mode 100644 index 0000000000..87f2767e8d --- /dev/null +++ b/apps/backend/internal/agent/runtime/agentctl/control_admission_test.go @@ -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) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go index 3e26279d9b..f27f335dfe 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go +++ b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go @@ -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 { diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_standalone.go b/apps/backend/internal/agent/runtime/lifecycle/executor_standalone.go index 3aeec4fb58..f54c7a1901 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_standalone.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_standalone.go @@ -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 diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_standalone_admission_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_standalone_admission_test.go new file mode 100644 index 0000000000..3b4c3da50d --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_standalone_admission_test.go @@ -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) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_admission_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_admission_test.go new file mode 100644 index 0000000000..160e88ec58 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_admission_test.go @@ -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) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_diagnostics.go b/apps/backend/internal/agent/runtime/lifecycle/manager_diagnostics.go new file mode 100644 index 0000000000..16077ee00f --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_diagnostics.go @@ -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) +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go index e05f2e20a1..8d15e5238c 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go @@ -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)) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/preparer_script.go b/apps/backend/internal/agent/runtime/lifecycle/preparer_script.go index db1f71269f..bce90b4753 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/preparer_script.go +++ b/apps/backend/internal/agent/runtime/lifecycle/preparer_script.go @@ -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, diff --git a/apps/backend/internal/agentctl/server/adapter/e2e/harness.go b/apps/backend/internal/agentctl/server/adapter/e2e/harness.go index 86218e270d..4736013ab9 100644 --- a/apps/backend/internal/agentctl/server/adapter/e2e/harness.go +++ b/apps/backend/internal/agentctl/server/adapter/e2e/harness.go @@ -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 } diff --git a/apps/backend/internal/agentctl/server/api/control_admission_test.go b/apps/backend/internal/agentctl/server/api/control_admission_test.go new file mode 100644 index 0000000000..c9e16d0a12 --- /dev/null +++ b/apps/backend/internal/agentctl/server/api/control_admission_test.go @@ -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) + } +} diff --git a/apps/backend/internal/agentctl/server/api/control_server.go b/apps/backend/internal/agentctl/server/api/control_server.go index cfebb9db9d..a2096758e4 100644 --- a/apps/backend/internal/agentctl/server/api/control_server.go +++ b/apps/backend/internal/agentctl/server/api/control_server.go @@ -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) { diff --git a/apps/backend/internal/agentctl/server/api/git.go b/apps/backend/internal/agentctl/server/api/git.go index aa54dc7a93..b76eaf02b8 100644 --- a/apps/backend/internal/agentctl/server/api/git.go +++ b/apps/backend/internal/agentctl/server/api/git.go @@ -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) } diff --git a/apps/backend/internal/agentctl/server/api/git_fanout_test.go b/apps/backend/internal/agentctl/server/api/git_fanout_test.go index e1c5ed8a4a..da628615d4 100644 --- a/apps/backend/internal/agentctl/server/api/git_fanout_test.go +++ b/apps/backend/internal/agentctl/server/api/git_fanout_test.go @@ -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) } } diff --git a/apps/backend/internal/agentctl/server/api/workspace_materialize.go b/apps/backend/internal/agentctl/server/api/workspace_materialize.go index 772a26e93a..d1abb43e0a 100644 --- a/apps/backend/internal/agentctl/server/api/workspace_materialize.go +++ b/apps/backend/internal/agentctl/server/api/workspace_materialize.go @@ -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() diff --git a/apps/backend/internal/agentctl/server/process/git.go b/apps/backend/internal/agentctl/server/process/git.go index 436b0d6a8c..047db0574a 100644 --- a/apps/backend/internal/agentctl/server/process/git.go +++ b/apps/backend/internal/agentctl/server/process/git.go @@ -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 != "" { diff --git a/apps/backend/internal/agentctl/server/process/git_pr_providers.go b/apps/backend/internal/agentctl/server/process/git_pr_providers.go index 99875fa533..d12b2afa3d 100644 --- a/apps/backend/internal/agentctl/server/process/git_pr_providers.go +++ b/apps/backend/internal/agentctl/server/process/git_pr_providers.go @@ -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() } diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index 006e3f5f23..079a2c8a3a 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -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) } diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_cmd.go b/apps/backend/internal/agentctl/server/process/workspace_git_cmd.go index f1ae245da7..81eace6a36 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_cmd.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_cmd.go @@ -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 } diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_cmd_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_cmd_test.go index 1d68a6ce26..394e7f6d6d 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_cmd_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_cmd_test.go @@ -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) +} diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_commits.go b/apps/backend/internal/agentctl/server/process/workspace_git_commits.go index ea8a393da4..8bac5f7d50 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_commits.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_commits.go @@ -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\n \n\x1e\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), diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_diff.go b/apps/backend/internal/agentctl/server/process/workspace_git_diff.go index f3a7261023..75533d9666 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_diff.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_diff.go @@ -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 diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_poll.go b/apps/backend/internal/agentctl/server/process/workspace_git_poll.go index f853838b87..bdffa5ddef 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_poll.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_poll.go @@ -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 diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_poll_admission_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_poll_admission_test.go new file mode 100644 index 0000000000..0f166a4ca0 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/workspace_git_poll_admission_test.go @@ -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) + } +} diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status.go b/apps/backend/internal/agentctl/server/process/workspace_git_status.go index 446eb35378..978b60e2fc 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status.go @@ -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 } diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status_concurrency_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_status_concurrency_test.go index 1b3a0e09ed..f9c63c9639 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status_concurrency_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status_concurrency_test.go @@ -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{}) diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go index 169f5c1692..62856bbb31 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go @@ -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)) diff --git a/apps/backend/internal/agentctl/server/process/workspace_monitor.go b/apps/backend/internal/agentctl/server/process/workspace_monitor.go index 087fdabfbf..cb191b0813 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_monitor.go +++ b/apps/backend/internal/agentctl/server/process/workspace_monitor.go @@ -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 diff --git a/apps/backend/internal/agentctl/server/process/workspace_stream.go b/apps/backend/internal/agentctl/server/process/workspace_stream.go index 491885ed6d..3bcf85a006 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_stream.go +++ b/apps/backend/internal/agentctl/server/process/workspace_stream.go @@ -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() diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker.go b/apps/backend/internal/agentctl/server/process/workspace_tracker.go index b715a26250..18b6aaa0c3 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker.go @@ -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)) diff --git a/apps/backend/internal/backendapp/mcp_stop_composition_test.go b/apps/backend/internal/backendapp/mcp_stop_composition_test.go index 2ba6ec4b37..183a6695dc 100644 --- a/apps/backend/internal/backendapp/mcp_stop_composition_test.go +++ b/apps/backend/internal/backendapp/mcp_stop_composition_test.go @@ -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) diff --git a/apps/backend/internal/backendapp/worktree.go b/apps/backend/internal/backendapp/worktree.go index 1773500ccb..57b868b433 100644 --- a/apps/backend/internal/backendapp/worktree.go +++ b/apps/backend/internal/backendapp/worktree.go @@ -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)) diff --git a/apps/backend/internal/common/subproc/admission.go b/apps/backend/internal/common/subproc/admission.go new file mode 100644 index 0000000000..8f0eb2811c --- /dev/null +++ b/apps/backend/internal/common/subproc/admission.go @@ -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() } diff --git a/apps/backend/internal/common/subproc/admission_test.go b/apps/backend/internal/common/subproc/admission_test.go new file mode 100644 index 0000000000..621a0ce2b4 --- /dev/null +++ b/apps/backend/internal/common/subproc/admission_test.go @@ -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)) +} diff --git a/apps/backend/internal/common/subproc/git_command.go b/apps/backend/internal/common/subproc/git_command.go new file mode 100644 index 0000000000..c510d641ac --- /dev/null +++ b/apps/backend/internal/common/subproc/git_command.go @@ -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 +} diff --git a/apps/backend/internal/common/subproc/git_exec_other.go b/apps/backend/internal/common/subproc/git_exec_other.go new file mode 100644 index 0000000000..aa46106015 --- /dev/null +++ b/apps/backend/internal/common/subproc/git_exec_other.go @@ -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") +} diff --git a/apps/backend/internal/common/subproc/git_exec_unix.go b/apps/backend/internal/common/subproc/git_exec_unix.go new file mode 100644 index 0000000000..14a6e0b04c --- /dev/null +++ b/apps/backend/internal/common/subproc/git_exec_unix.go @@ -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) +} diff --git a/apps/backend/internal/common/subproc/metrics.go b/apps/backend/internal/common/subproc/metrics.go index 61af9cf992..fed85cebff 100644 --- a/apps/backend/internal/common/subproc/metrics.go +++ b/apps/backend/internal/common/subproc/metrics.go @@ -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 diff --git a/apps/backend/internal/common/subproc/raw_git_audit_test.go b/apps/backend/internal/common/subproc/raw_git_audit_test.go new file mode 100644 index 0000000000..4aeaf82969 --- /dev/null +++ b/apps/backend/internal/common/subproc/raw_git_audit_test.go @@ -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" +} diff --git a/apps/backend/internal/common/subproc/shared.go b/apps/backend/internal/common/subproc/shared.go index 8b2fe89adc..94f8d8c2bf 100644 --- a/apps/backend/internal/common/subproc/shared.go +++ b/apps/backend/internal/common/subproc/shared.go @@ -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 diff --git a/apps/backend/internal/common/subproc/throttle.go b/apps/backend/internal/common/subproc/throttle.go index e544db3610..b0dcaf770b 100644 --- a/apps/backend/internal/common/subproc/throttle.go +++ b/apps/backend/internal/common/subproc/throttle.go @@ -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 diff --git a/apps/backend/internal/office/configloader/git.go b/apps/backend/internal/office/configloader/git.go index 0420a7234e..a41f79ff7f 100644 --- a/apps/backend/internal/office/configloader/git.go +++ b/apps/backend/internal/office/configloader/git.go @@ -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 } diff --git a/apps/backend/internal/office/skills/materialization.go b/apps/backend/internal/office/skills/materialization.go index 94b9313baa..097548fd48 100644 --- a/apps/backend/internal/office/skills/materialization.go +++ b/apps/backend/internal/office/skills/materialization.go @@ -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))) } diff --git a/apps/backend/internal/office/testharness/routes.go b/apps/backend/internal/office/testharness/routes.go index 62b4907792..3dc06453c9 100644 --- a/apps/backend/internal/office/testharness/routes.go +++ b/apps/backend/internal/office/testharness/routes.go @@ -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) diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index ca54207c7e..f3298b45d0 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -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 "" } diff --git a/apps/backend/internal/repoclone/clone.go b/apps/backend/internal/repoclone/clone.go index 030fac359f..da9f3005e2 100644 --- a/apps/backend/internal/repoclone/clone.go +++ b/apps/backend/internal/repoclone/clone.go @@ -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 diff --git a/apps/backend/internal/task/gitinit/command_descriptor.go b/apps/backend/internal/task/gitinit/command_descriptor.go index 75d09daf0b..eb6a0712c7 100644 --- a/apps/backend/internal/task/gitinit/command_descriptor.go +++ b/apps/backend/internal/task/gitinit/command_descriptor.go @@ -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) diff --git a/apps/backend/internal/task/gitinit/command_path.go b/apps/backend/internal/task/gitinit/command_path.go index 0c5eb99289..ea924b1c38 100644 --- a/apps/backend/internal/task/gitinit/command_path.go +++ b/apps/backend/internal/task/gitinit/command_path.go @@ -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 } diff --git a/apps/backend/internal/task/handlers/executor_profile_handlers.go b/apps/backend/internal/task/handlers/executor_profile_handlers.go index 77e3231df0..f91f7e2c2d 100644 --- a/apps/backend/internal/task/handlers/executor_profile_handlers.go +++ b/apps/backend/internal/task/handlers/executor_profile_handlers.go @@ -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 "" } diff --git a/apps/backend/internal/task/service/fresh_branch.go b/apps/backend/internal/task/service/fresh_branch.go index f96c0f88ec..92854cdb8c 100644 --- a/apps/backend/internal/task/service/fresh_branch.go +++ b/apps/backend/internal/task/service/fresh_branch.go @@ -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 } diff --git a/apps/backend/internal/task/service/local_repository_initialization.go b/apps/backend/internal/task/service/local_repository_initialization.go index 2c110af06a..375ab2b190 100644 --- a/apps/backend/internal/task/service/local_repository_initialization.go +++ b/apps/backend/internal/task/service/local_repository_initialization.go @@ -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))) } diff --git a/apps/backend/internal/task/service/repository_discovery.go b/apps/backend/internal/task/service/repository_discovery.go index 7891472dba..f1f1f1fe6b 100644 --- a/apps/backend/internal/task/service/repository_discovery.go +++ b/apps/backend/internal/task/service/repository_discovery.go @@ -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) } diff --git a/apps/backend/internal/task/service/repository_fetch.go b/apps/backend/internal/task/service/repository_fetch.go index 0f6c56c954..25d3c0e80c 100644 --- a/apps/backend/internal/task/service/repository_fetch.go +++ b/apps/backend/internal/task/service/repository_fetch.go @@ -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", diff --git a/apps/backend/internal/worktree/git_command.go b/apps/backend/internal/worktree/git_command.go index 0e712655b5..b90dc8f360 100644 --- a/apps/backend/internal/worktree/git_command.go +++ b/apps/backend/internal/worktree/git_command.go @@ -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...) } diff --git a/apps/backend/internal/worktree/git_throttle.go b/apps/backend/internal/worktree/git_throttle.go index ef147e9ecd..26395a72c4 100644 --- a/apps/backend/internal/worktree/git_throttle.go +++ b/apps/backend/internal/worktree/git_throttle.go @@ -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) } diff --git a/apps/backend/internal/worktree/git_throttle_test.go b/apps/backend/internal/worktree/git_throttle_test.go index b686e5ed46..9aeb96449d 100644 --- a/apps/backend/internal/worktree/git_throttle_test.go +++ b/apps/backend/internal/worktree/git_throttle_test.go @@ -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") diff --git a/apps/backend/internal/worktree/manager_git.go b/apps/backend/internal/worktree/manager_git.go index 05d90a19a1..91a1ef4e58 100644 --- a/apps/backend/internal/worktree/manager_git.go +++ b/apps/backend/internal/worktree/manager_git.go @@ -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() } diff --git a/docs/decisions/2026-08-02-class-aware-git-subprocess-admission.md b/docs/decisions/2026-08-02-class-aware-git-subprocess-admission.md new file mode 100644 index 0000000000..a67ce7166b --- /dev/null +++ b/docs/decisions/2026-08-02-class-aware-git-subprocess-admission.md @@ -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. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 428cd488d2..0d48cf612b 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -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 | diff --git a/docs/plans/git-subprocess-admission/plan.md b/docs/plans/git-subprocess-admission/plan.md new file mode 100644 index 0000000000..77e4dd45c4 --- /dev/null +++ b/docs/plans/git-subprocess-admission/plan.md @@ -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. diff --git a/docs/plans/git-subprocess-admission/task-01-class-aware-admission.md b/docs/plans/git-subprocess-admission/task-01-class-aware-admission.md new file mode 100644 index 0000000000..29631d8e06 --- /dev/null +++ b/docs/plans/git-subprocess-admission/task-01-class-aware-admission.md @@ -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. diff --git a/docs/plans/git-subprocess-admission/task-02-interactive-git-fanout.md b/docs/plans/git-subprocess-admission/task-02-interactive-git-fanout.md new file mode 100644 index 0000000000..cc95580330 --- /dev/null +++ b/docs/plans/git-subprocess-admission/task-02-interactive-git-fanout.md @@ -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. diff --git a/docs/plans/git-subprocess-admission/task-03-lifecycle-git-classification.md b/docs/plans/git-subprocess-admission/task-03-lifecycle-git-classification.md new file mode 100644 index 0000000000..a18b5c0b26 --- /dev/null +++ b/docs/plans/git-subprocess-admission/task-03-lifecycle-git-classification.md @@ -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. diff --git a/docs/plans/git-subprocess-admission/task-04-poller-admission-liveness.md b/docs/plans/git-subprocess-admission/task-04-poller-admission-liveness.md new file mode 100644 index 0000000000..4263c7bef2 --- /dev/null +++ b/docs/plans/git-subprocess-admission/task-04-poller-admission-liveness.md @@ -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. diff --git a/docs/plans/git-subprocess-admission/task-05-admission-diagnostics.md b/docs/plans/git-subprocess-admission/task-05-admission-diagnostics.md new file mode 100644 index 0000000000..8e42a2093a --- /dev/null +++ b/docs/plans/git-subprocess-admission/task-05-admission-diagnostics.md @@ -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. diff --git a/docs/plans/git-subprocess-admission/task-06-review-blockers.md b/docs/plans/git-subprocess-admission/task-06-review-blockers.md new file mode 100644 index 0000000000..450d4356e5 --- /dev/null +++ b/docs/plans/git-subprocess-admission/task-06-review-blockers.md @@ -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`). diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index b865d55084..89d7dda76d 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -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 | diff --git a/docs/specs/platform/git-subprocess-admission.md b/docs/specs/platform/git-subprocess-admission.md new file mode 100644 index 0000000000..73fecf92d0 --- /dev/null +++ b/docs/specs/platform/git-subprocess-admission.md @@ -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)