diff --git a/apps/backend/internal/agent/handlers/git_handlers.go b/apps/backend/internal/agent/handlers/git_handlers.go index 490a874b10..e9f7a7a8f3 100644 --- a/apps/backend/internal/agent/handlers/git_handlers.go +++ b/apps/backend/internal/agent/handlers/git_handlers.go @@ -5,16 +5,28 @@ import ( "context" "errors" "fmt" + "strconv" "strings" "time" + "go.uber.org/zap" + "golang.org/x/sync/singleflight" + "github.com/kandev/kandev/internal/agent/runtime/agentctl" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/common/logger" ws "github.com/kandev/kandev/pkg/websocket" - "go.uber.org/zap" ) +// singleflightComputeTimeout bounds the detached context used for the +// shared body of session.git.commits / session.cumulative_diff. The body +// runs in a goroutine spawned by singleflight that is NOT tied to any +// single waiter's request ctx — see wsGitCommits — so it needs its own +// timeout floor. Generous enough to survive a slow agentctl round-trip +// under git-pool contention, short enough that a wedged execution can't +// pin the singleflight slot indefinitely. +const singleflightComputeTimeout = 60 * time.Second + // PRCreatedCallback is called after a PR is successfully created. `repo` is // the multi-repo subpath (e.g. "kandev"); empty for single-repo workspaces. // The callback uses it to scope the resulting TaskPR / PRWatch rows to the @@ -50,12 +62,20 @@ type SessionReader interface { // GitHandlers provides WebSocket handlers for git worktree operations. // Operations are executed via agentctl which runs in the worktree context. +// +// commitsGroup / diffGroup deduplicate concurrent identical requests +// (same session, same params) so a frontend re-render storm doesn't fan +// out into N agentctl HTTP calls that each spawn a git subprocess under +// the agentctl-side throttle. Reproduced in PR #1216 trace: ~10 +// session.git.commits arrived for the same sessionID inside 30ms. type GitHandlers struct { lifecycleMgr ExecutionLookup sessionReader SessionReader logger *logger.Logger onPRCreated PRCreatedCallback onGitOperationFailed GitOperationFailedCallback + commitsGroup singleflight.Group + diffGroup singleflight.Group } // NewGitHandlers creates a new GitHandlers instance. @@ -686,51 +706,68 @@ type GitCommitsRequest struct { // The base commit SHA is always looked up from the session metadata in the database. // This ensures commits are filtered to only those made during the session. // When a target branch is available, we use dynamic merge-base calculation for accuracy. +// +// Concurrent identical requests (same sessionID + limit) collapse onto a +// single in-flight computeGitCommits call via singleflight; each waiter +// still wraps the shared payload in its own ws.Response using its own +// msg.ID so the WS layer sees N replies for N requests. +// +// The shared body uses a detached, bounded context — NOT the per-request +// ctx — so a single waiter's WS disconnect or timeout doesn't cancel the +// in-flight computation for the other waiters. Each caller still selects +// on its own ctx via DoChan so an individual waiter can give up without +// blocking on the shared result. func (h *GitHandlers) wsGitCommits(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req GitCommitsRequest if err := msg.ParsePayload(&req); err != nil { return nil, fmt.Errorf("invalid payload: %w", err) } - if req.SessionID == "" { return nil, fmt.Errorf("session_id is required") } + key := req.SessionID + ":" + strconv.Itoa(req.Limit) + ch := h.commitsGroup.DoChan(key, func() (interface{}, error) { + detached, cancel := context.WithTimeout(context.Background(), singleflightComputeTimeout) + defer cancel() + return h.computeGitCommits(detached, &req) + }) + select { + case r := <-ch: + if r.Err != nil { + return nil, r.Err + } + return ws.NewResponse(msg.ID, msg.Action, r.Val) + case <-ctx.Done(): + return nil, ctx.Err() + } +} - // Use GetOrEnsureExecution to recover workspace after backend restarts. - // This is a workspace-oriented operation that doesn't require a running agent process. +// computeGitCommits is the singleflight body for wsGitCommits. Returns +// a JSON-ready payload — the not-ready / agent-client-nil cases return +// the same `{commits:[], ready:false}` envelope as before, just packaged +// as a value so all waiters can share it. +func (h *GitHandlers) computeGitCommits(ctx context.Context, req *GitCommitsRequest) (interface{}, error) { execution, err := h.lifecycleMgr.GetOrEnsureExecution(ctx, req.SessionID) if err != nil { - // Check for specific "not ready" errors that indicate the session workspace - // is still being prepared. For these, return ready:false so the client can retry. + // "Not ready" errors map to ready:false so the client retries; any + // other error (DB failure, etc.) propagates as a real error. if errors.Is(err, lifecycle.ErrSessionWorkspaceNotReady) || isSessionNotReadyError(err) { - return ws.NewResponse(msg.ID, msg.Action, map[string]any{ - "commits": []any{}, - "ready": false, - }) + return map[string]any{"commits": []any{}, "ready": false}, nil } - // For unexpected errors (database failures, etc.), return the error - // so the client can display an appropriate error message. return nil, fmt.Errorf("failed to get execution for session %s: %w", req.SessionID, err) } - agentClient := execution.GetAgentCtlClient() if agentClient == nil { - return ws.NewResponse(msg.ID, msg.Action, map[string]any{ - "commits": []any{}, - "ready": false, - }) + return map[string]any{"commits": []any{}, "ready": false}, nil } - - // Look up base commit SHA and target branch from the session metadata var baseCommit, targetBranch string if h.sessionReader != nil { baseCommit = h.sessionReader.GetSessionBaseCommit(ctx, req.SessionID) targetBranch = h.sessionReader.GetSessionBaseBranch(ctx, req.SessionID) } - - // Fallback: if base_commit_sha is not stored in session, use git merge-base - // from git status. This happens for sessions created before the base commit - // capture feature or if the capture failed. + // Fallback: if base_commit_sha is not stored in the session, use the + // merge-base from git status. Applies to sessions created before the + // base-commit capture feature, or when that capture failed. if baseCommit == "" { status, statusErr := agentClient.GetGitStatus(ctx) if statusErr == nil && status != nil && status.BaseCommit != "" { @@ -740,15 +777,11 @@ func (h *GitHandlers) wsGitCommits(ctx context.Context, msg *ws.Message) (*ws.Me zap.String("base_commit", baseCommit)) } } - - // Use target branch for dynamic merge-base calculation. - // This ensures accurate commit filtering even after rebases. result, err := agentClient.GitLog(ctx, baseCommit, req.Limit, targetBranch, "") if err != nil { return nil, fmt.Errorf("git log failed: %w", err) } - - return ws.NewResponse(msg.ID, msg.Action, result) + return result, nil } // CumulativeDiffRequest for session.cumulative_diff action @@ -756,51 +789,54 @@ type CumulativeDiffRequest struct { SessionID string `json:"session_id"` } -// wsCumulativeDiff handles session.cumulative_diff action +// wsCumulativeDiff handles session.cumulative_diff action. // The base commit SHA is always looked up from the session metadata in the database. +// Concurrent identical requests collapse via singleflight — see wsGitCommits +// for the detached-ctx / DoChan rationale. func (h *GitHandlers) wsCumulativeDiff(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req CumulativeDiffRequest if err := msg.ParsePayload(&req); err != nil { return nil, fmt.Errorf("invalid payload: %w", err) } - if req.SessionID == "" { return nil, fmt.Errorf("session_id is required") } + ch := h.diffGroup.DoChan(req.SessionID, func() (interface{}, error) { + detached, cancel := context.WithTimeout(context.Background(), singleflightComputeTimeout) + defer cancel() + return h.computeCumulativeDiff(detached, &req) + }) + select { + case r := <-ch: + if r.Err != nil { + return nil, r.Err + } + return ws.NewResponse(msg.ID, msg.Action, r.Val) + case <-ctx.Done(): + return nil, ctx.Err() + } +} - // Use GetOrEnsureExecution to recover workspace after backend restarts. - // This is a workspace-oriented operation that doesn't require a running agent process. +// computeCumulativeDiff is the singleflight body for wsCumulativeDiff. +// Returns a JSON-ready payload — not-ready / no-base-commit cases keep +// the same response envelope shape as before. +func (h *GitHandlers) computeCumulativeDiff(ctx context.Context, req *CumulativeDiffRequest) (interface{}, error) { execution, err := h.lifecycleMgr.GetOrEnsureExecution(ctx, req.SessionID) if err != nil { if errors.Is(err, lifecycle.ErrSessionWorkspaceNotReady) || isSessionNotReadyError(err) { - return ws.NewResponse(msg.ID, msg.Action, map[string]any{ - "cumulative_diff": nil, - "ready": false, - }) + return map[string]any{"cumulative_diff": nil, "ready": false}, nil } return nil, fmt.Errorf("failed to get execution for session %s: %w", req.SessionID, err) } - agentClient := execution.GetAgentCtlClient() if agentClient == nil { - return ws.NewResponse(msg.ID, msg.Action, map[string]any{ - "cumulative_diff": nil, - "ready": false, - }) + return map[string]any{"cumulative_diff": nil, "ready": false}, nil } - - // Look up base commit SHA and target branch from the session metadata. - // targetBranch lets agentctl recompute the base via merge-base against - // origin/ for live divergence — same anchoring as the COMMITS - // panel, so the file diff doesn't include changes that came in via merges - // from main after the session was started. var baseCommit, targetBranch string if h.sessionReader != nil { baseCommit = h.sessionReader.GetSessionBaseCommit(ctx, req.SessionID) targetBranch = h.sessionReader.GetSessionBaseBranch(ctx, req.SessionID) } - - // Fallback: if base_commit_sha is not stored, use git merge-base from status if baseCommit == "" { status, statusErr := agentClient.GetGitStatus(ctx) if statusErr == nil && status != nil && status.BaseCommit != "" { @@ -809,22 +845,15 @@ func (h *GitHandlers) wsCumulativeDiff(ctx context.Context, msg *ws.Message) (*w zap.String("session_id", req.SessionID), zap.String("base_commit", baseCommit)) } else { - // Repo-less tasks (no git workspace) have no base commit — return an - // empty diff instead of erroring so the frontend's polling doesn't - // spam the logs. - return ws.NewResponse(msg.ID, msg.Action, map[string]any{ - "cumulative_diff": nil, - }) + // Repo-less tasks (no git workspace) have no base commit — return + // an empty diff instead of erroring so the frontend's polling + // doesn't spam the logs. + return map[string]any{"cumulative_diff": nil}, nil } } - result, err := agentClient.GetCumulativeDiff(ctx, baseCommit, targetBranch) if err != nil { return nil, fmt.Errorf("cumulative diff failed: %w", err) } - - // Wrap in cumulative_diff key as expected by frontend - return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{ - "cumulative_diff": result, - }) + return map[string]interface{}{"cumulative_diff": result}, nil } diff --git a/apps/backend/internal/agent/handlers/git_handlers_test.go b/apps/backend/internal/agent/handlers/git_handlers_test.go index c34cbf519e..3add1f15b6 100644 --- a/apps/backend/internal/agent/handlers/git_handlers_test.go +++ b/apps/backend/internal/agent/handlers/git_handlers_test.go @@ -7,6 +7,7 @@ import ( "fmt" "sync" "testing" + "testing/synctest" "time" "github.com/kandev/kandev/internal/agent/runtime/agentctl" @@ -21,6 +22,46 @@ type mockExecutionLookup struct { ensureErr error } +// blockingExecutionLookup is a counted, blocking ExecutionLookup used by the +// singleflight dedup tests. Each GetOrEnsureExecution call bumps a counter, +// signals on `entered` (non-blocking), then blocks on `release` so the test +// can fire N concurrent requests and assert exactly one ever runs the inner +// work. Returns `err` once released — set to ErrSessionWorkspaceNotReady so +// the handler returns the graceful not-ready envelope rather than failing. +type blockingExecutionLookup struct { + mu sync.Mutex + calls int + entered chan struct{} + release chan struct{} + err error +} + +func (b *blockingExecutionLookup) GetExecutionBySessionID(_ string) (*lifecycle.AgentExecution, bool) { + return nil, false +} + +func (b *blockingExecutionLookup) GetOrEnsureExecution(ctx context.Context, _ string) (*lifecycle.AgentExecution, error) { + b.mu.Lock() + b.calls++ + b.mu.Unlock() + select { + case b.entered <- struct{}{}: + default: + } + select { + case <-b.release: + return nil, b.err + case <-ctx.Done(): + return nil, ctx.Err() + } +} + +func (b *blockingExecutionLookup) callCount() int { + b.mu.Lock() + defer b.mu.Unlock() + return b.calls +} + func (m *mockExecutionLookup) GetExecutionBySessionID(sessionID string) (*lifecycle.AgentExecution, bool) { if m.executions == nil { return nil, false @@ -613,3 +654,200 @@ func TestWsGitCommits_WrappedWorkspaceNotReadyError(t *testing.T) { t.Errorf("expected ready=false, got %v", payload["ready"]) } } + +// TestWsGitCommits_SingleflightDedupesConcurrentRequests verifies that N +// concurrent identical (sessionID, limit) requests collapse onto a single +// computeGitCommits invocation. Without dedup, a frontend re-render storm +// fans out into N agentctl HTTP calls under the per-process throttle. +// +// Runs inside synctest so the assertion fires only after all N waiters +// have called DoChan and the singleflight body has parked — eliminating +// the wall-clock window where a late waiter could miss the dedup and +// trigger a second body invocation. +func TestWsGitCommits_SingleflightDedupesConcurrentRequests(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + log := newTestLogger() + lookup := &blockingExecutionLookup{ + entered: make(chan struct{}, 1), + release: make(chan struct{}), + err: lifecycle.ErrSessionWorkspaceNotReady, + } + h := NewGitHandlers(lookup, nil, log) + + const N = 8 + type result struct { + resp *ws.Message + err error + } + results := make(chan result, N) + for i := 0; i < N; i++ { + go func() { + msg, _ := ws.NewRequest("test", ws.ActionSessionGitCommits, GitCommitsRequest{SessionID: "session-1", Limit: 50}) + r, err := h.wsGitCommits(context.Background(), msg) + results <- result{r, err} + }() + } + + // Wait until every waiter has parked on its singleflight ch and + // the body has parked on release. After this, callCount must be + // exactly 1: any value > 1 means a waiter raced past the dedup. + synctest.Wait() + if got := lookup.callCount(); got != 1 { + t.Fatalf("expected exactly 1 compute call before release, got %d — singleflight dedup broken", got) + } + close(lookup.release) + synctest.Wait() + + for i := 0; i < N; i++ { + r := <-results + if r.err != nil { + t.Fatalf("waiter %d got error: %v", i, r.err) + } + if r.resp == nil { + t.Fatalf("waiter %d got nil response", i) + } + } + + if got := lookup.callCount(); got != 1 { + t.Fatalf("expected exactly 1 compute call, got %d — singleflight dedup broken", got) + } + }) +} + +// TestWsGitCommits_SingleflightWaiterCancellationDoesNotPoisonOthers verifies +// that cancelling one waiter's request ctx does NOT cancel the shared +// computation or affect other waiters. The shared work runs on a detached +// 60s ctx; individual waiters select on their own ctx via DoChan. +// +// Runs inside synctest so the cancel happens only after every waiter has +// joined the same singleflight call. Without that guarantee, a late waiter +// could race past dedup on slow CI and spawn a second body invocation. +func TestWsGitCommits_SingleflightWaiterCancellationDoesNotPoisonOthers(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + log := newTestLogger() + lookup := &blockingExecutionLookup{ + entered: make(chan struct{}, 1), + release: make(chan struct{}), + err: lifecycle.ErrSessionWorkspaceNotReady, + } + h := NewGitHandlers(lookup, nil, log) + + const N = 4 + type result struct { + resp *ws.Message + err error + } + results := make([]chan result, N) + ctxs := make([]context.Context, N) + cancels := make([]context.CancelFunc, N) + for i := 0; i < N; i++ { + results[i] = make(chan result, 1) + ctxs[i], cancels[i] = context.WithCancel(context.Background()) + } + defer func() { + for _, c := range cancels { + c() + } + }() + + for i := 0; i < N; i++ { + go func(idx int) { + msg, _ := ws.NewRequest("test", ws.ActionSessionGitCommits, GitCommitsRequest{SessionID: "session-1", Limit: 50}) + r, err := h.wsGitCommits(ctxs[idx], msg) + results[idx] <- result{r, err} + }(i) + } + + // Block until every waiter has parked on its singleflight ch and + // the shared body has parked on release. After this returns, all + // N waiters share the same in-flight call. + synctest.Wait() + if got := lookup.callCount(); got != 1 { + t.Fatalf("expected exactly 1 compute call before cancellation, got %d", got) + } + + // Cancel waiter 0 while the shared work is still in flight. + cancels[0]() + synctest.Wait() + select { + case r := <-results[0]: + if !errors.Is(r.err, context.Canceled) { + t.Fatalf("waiter 0 expected context.Canceled, got: %v", r.err) + } + default: + t.Fatalf("waiter 0 never observed its ctx cancellation") + } + + // Release the shared computation — the remaining N-1 waiters must + // still receive a non-error response. + close(lookup.release) + synctest.Wait() + for i := 1; i < N; i++ { + select { + case r := <-results[i]: + if r.err != nil { + t.Fatalf("waiter %d got unexpected error after peer cancelled: %v", i, r.err) + } + if r.resp == nil { + t.Fatalf("waiter %d got nil response", i) + } + default: + t.Fatalf("waiter %d never received result", i) + } + } + + if got := lookup.callCount(); got != 1 { + t.Fatalf("expected exactly 1 compute call despite cancellation, got %d", got) + } + }) +} + +// TestWsCumulativeDiff_SingleflightDedupesConcurrentRequests mirrors the +// commits test for the cumulative-diff handler, which uses the same DoChan +// + detached-ctx pattern on diffGroup. +func TestWsCumulativeDiff_SingleflightDedupesConcurrentRequests(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + log := newTestLogger() + lookup := &blockingExecutionLookup{ + entered: make(chan struct{}, 1), + release: make(chan struct{}), + err: lifecycle.ErrSessionWorkspaceNotReady, + } + h := NewGitHandlers(lookup, nil, log) + + const N = 6 + type result struct { + resp *ws.Message + err error + } + results := make(chan result, N) + for i := 0; i < N; i++ { + go func() { + msg, _ := ws.NewRequest("test", ws.ActionSessionCumulativeDiff, CumulativeDiffRequest{SessionID: "session-1"}) + r, err := h.wsCumulativeDiff(context.Background(), msg) + results <- result{r, err} + }() + } + + synctest.Wait() + if got := lookup.callCount(); got != 1 { + t.Fatalf("expected exactly 1 compute call before release, got %d — singleflight dedup broken on diffGroup", got) + } + close(lookup.release) + synctest.Wait() + + for i := 0; i < N; i++ { + r := <-results + if r.err != nil { + t.Fatalf("waiter %d got error: %v", i, r.err) + } + if r.resp == nil { + t.Fatalf("waiter %d got nil response", i) + } + } + + if got := lookup.callCount(); got != 1 { + t.Fatalf("expected exactly 1 compute call, got %d — singleflight dedup broken on diffGroup", got) + } + }) +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/credential_uploader.go b/apps/backend/internal/agent/runtime/lifecycle/credential_uploader.go index 6ae8c2480a..d8303fc7c1 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/credential_uploader.go +++ b/apps/backend/internal/agent/runtime/lifecycle/credential_uploader.go @@ -13,6 +13,7 @@ import ( "github.com/kandev/kandev/internal/agent/remoteauth" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/subproc" ) // FileUploader abstracts writing files to a remote environment. Used by @@ -68,10 +69,22 @@ func UploadCredentialFiles( } // DetectGHToken runs `gh auth token` locally and returns the GitHub OAuth token. +// +// Splits the throttle wait (30s) from the exec budget (5s) so a busy gh pool +// can't silently disable token injection. Critically the exec context is +// built AFTER Acquire returns — otherwise its 5s deadline would already be +// ticking down while we waited in the throttle queue. func DetectGHToken() (string, error) { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - cmd := exec.CommandContext(ctx, "gh", "auth", "token") + acquireCtx, cancelAcquire := context.WithTimeout(context.Background(), 30*time.Second) + defer cancelAcquire() + release, err := subproc.GH().Acquire(acquireCtx) + if err != nil { + return "", fmt.Errorf("gh throttle acquire: %w", err) + } + defer release() + execCtx, cancelExec := context.WithTimeout(context.Background(), 5*time.Second) + defer cancelExec() + cmd := exec.CommandContext(execCtx, "gh", "auth", "token") out, err := cmd.Output() if err != nil { return "", fmt.Errorf("gh auth token failed: %w", err) 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 062c3113fe..033ad95e60 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go +++ b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go @@ -14,6 +14,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/common/shellexec" + "github.com/kandev/kandev/internal/common/subproc" "github.com/kandev/kandev/internal/worktree" ) @@ -143,7 +144,21 @@ 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 { - cmd := exec.Command("git", "symbolic-ref", "--short", "HEAD") + // `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. + 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 { @@ -159,11 +174,11 @@ func readCurrentBranchForLocal(workDir string) string { func checkoutBranch(ctx context.Context, workDir, branch string) (string, error) { fetchCmd := exec.CommandContext(ctx, "git", "fetch", "origin", branch) fetchCmd.Dir = workDir - fetchCmd.Run() //nolint:errcheck // fetch failure is non-fatal, we fall back to local + _ = subproc.RunGit(ctx, fetchCmd) // fetch failure is non-fatal, fall back to local cmd := exec.CommandContext(ctx, "git", "checkout", branch) cmd.Dir = workDir - out, err := cmd.CombinedOutput() + out, err := subproc.RunGitCombinedOutput(ctx, cmd) outStr := strings.TrimSpace(string(out)) if err != nil { return outStr, worktree.ClassifyGitError(outStr, err) diff --git a/apps/backend/internal/agentctl/server/api/workspace.go b/apps/backend/internal/agentctl/server/api/workspace.go index 1e23807a11..49507c58ce 100644 --- a/apps/backend/internal/agentctl/server/api/workspace.go +++ b/apps/backend/internal/agentctl/server/api/workspace.go @@ -198,7 +198,7 @@ func (s *Server) handleFileUpdate(c *gin.Context) { } // Apply the diff - newHash, resolution, err := s.procMgr.GetWorkspaceTracker().ApplyFileDiff(scopedPath, req.Diff, req.OriginalHash, req.DesiredContent) + newHash, resolution, err := s.procMgr.GetWorkspaceTracker().ApplyFileDiff(c.Request.Context(), scopedPath, req.Diff, req.OriginalHash, req.DesiredContent) if err != nil { c.JSON(400, streams.FileUpdateResponse{ Path: req.Path, diff --git a/apps/backend/internal/agentctl/server/process/git.go b/apps/backend/internal/agentctl/server/process/git.go index d51c89c34a..16f39e7c6f 100644 --- a/apps/backend/internal/agentctl/server/process/git.go +++ b/apps/backend/internal/agentctl/server/process/git.go @@ -16,6 +16,7 @@ import ( "github.com/kandev/kandev/internal/agentctl/types/streams" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/common/securityutil" + "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -149,7 +150,7 @@ func (g *GitOperator) runGitCommand(ctx context.Context, args ...string) (string g.logger.Debug("executing git command", zap.Strings("args", args)) - err := cmd.Run() + err := subproc.RunGit(ctx, 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 47dff7d9ea..980442a953 100644 --- a/apps/backend/internal/agentctl/server/process/git_pr_providers.go +++ b/apps/backend/internal/agentctl/server/process/git_pr_providers.go @@ -13,6 +13,8 @@ import ( "strings" "go.uber.org/zap" + + "github.com/kandev/kandev/internal/common/subproc" ) type prProvider string @@ -355,7 +357,19 @@ func (g *GitOperator) runRepositoryCommand(ctx context.Context, name string, arg zap.Strings("args", sanitizeRepositoryArgs(args)), zap.String("workDir", g.workDir)) - err := cmd.Run() + // Route through the matching subproc throttle so PR-creation execs + // (gh / git) count against the same process-wide cap as the rest of + // the codebase. Unknown binaries (e.g. az) skip throttling — those + // aren't part of the fork-storm pattern. + var err error + switch name { + case "gh": + err = subproc.RunGH(ctx, cmd) + case "git": + err = subproc.RunGit(ctx, cmd) + default: + err = cmd.Run() + } stdoutOutput := strings.TrimSpace(stdout.String()) stderrOutput := strings.TrimSpace(stderr.String()) if err != nil { diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index dab2327207..3437ed50c8 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -19,6 +19,7 @@ import ( "unicode/utf8" "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -58,7 +59,7 @@ func (wt *WorkspaceTracker) getFileList(ctx context.Context) (types.FileListUpda // --exclude-standard: respect .gitignore cmd := exec.CommandContext(ctx, "git", "ls-files", "--cached", "--others", "--exclude-standard") cmd.Dir = wt.workDir - out, err := cmd.Output() + out, err := subproc.RunGitOutput(ctx, cmd) if err != nil { return update, err } @@ -338,7 +339,7 @@ func (wt *WorkspaceTracker) resolveSymlinkRelPath(reqPath string) string { // When desiredContent is provided and the diff cannot be applied (hash conflict), // the file is overwritten with the desired content as a fallback. // Returns the new hash and a resolution string ("applied" or "overwritten"). -func (wt *WorkspaceTracker) ApplyFileDiff(reqPath, unifiedDiff, originalHash string, desiredContent *string) (string, string, error) { +func (wt *WorkspaceTracker) ApplyFileDiff(ctx context.Context, reqPath, unifiedDiff, originalHash string, desiredContent *string) (string, string, error) { safePath, err := wt.resolveSafePath(reqPath) if err != nil { return "", "", err @@ -376,11 +377,17 @@ func (wt *WorkspaceTracker) ApplyFileDiff(reqPath, unifiedDiff, originalHash str }() // Use git apply to apply the patch directly to the file - cmd := exec.Command("git", "apply", "-p0", "--unidiff-zero", "--whitespace=nowarn", patchFile) + cmd := exec.CommandContext(ctx, "git", "apply", "-p0", "--unidiff-zero", "--whitespace=nowarn", patchFile) cmd.Dir = wt.workDir - output, err := cmd.CombinedOutput() + output, err := subproc.RunGitCombinedOutput(ctx, cmd) if err != nil { + // Treat caller cancellation / deadline as a transient failure and + // propagate rather than overwriting the file from desiredContent — + // the user pressing cancel must NOT silently clobber what's on disk. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return "", "", fmt.Errorf("git apply cancelled: %w", err) + } if desiredContent != nil { return wt.writeDesiredContent(safePath, cleanWorkDir, reqPath, *desiredContent, currentHash) } @@ -671,7 +678,7 @@ func (wt *WorkspaceTracker) GetFileContentAtRef(ctx context.Context, reqPath str sizeCmd := exec.CommandContext(ctx, "git", "cat-file", "-s", gitRef) sizeCmd.Dir = wt.workDir sizeCmd.Env = append(os.Environ(), "LC_ALL=C") - sizeOut, err := sizeCmd.CombinedOutput() + sizeOut, err := subproc.RunGitCombinedOutput(ctx, sizeCmd) if err != nil { output := string(sizeOut) if strings.Contains(output, "does not exist") || @@ -693,7 +700,7 @@ func (wt *WorkspaceTracker) GetFileContentAtRef(ctx context.Context, reqPath str cmd := exec.CommandContext(ctx, "git", "show", gitRef) cmd.Dir = wt.workDir - content, err := cmd.Output() + content, err := subproc.RunGitOutput(ctx, cmd) 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_commits.go b/apps/backend/internal/agentctl/server/process/workspace_git_commits.go index 0121748df4..ea8a393da4 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_commits.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_commits.go @@ -7,6 +7,7 @@ import ( "time" "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -20,7 +21,7 @@ func (wt *WorkspaceTracker) getCommitsSince(ctx context.Context, baseCommit stri "--shortstat", baseCommit+"..HEAD") cmd.Dir = wt.workDir - out, err := cmd.Output() + out, err := subproc.RunGitOutput(ctx, cmd) 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 19c39ee13a..0cae50cd85 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_diff.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_diff.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -58,7 +59,7 @@ func (wt *WorkspaceTracker) enrichWithBranchDiff(ctx context.Context, update *ty // git diff --numstat covers committed + staged + unstaged changes. numstatCmd := exec.CommandContext(ctx, "git", "diff", "--numstat", update.BaseCommit) numstatCmd.Dir = wt.workDir - numstatOut, err := numstatCmd.Output() + numstatOut, err := subproc.RunGitOutput(ctx, numstatCmd) if err != nil { wt.logger.Debug("enrichWithBranchDiff: numstat failed", zap.Error(err)) return @@ -102,6 +103,12 @@ func totalDiffBytes(update *types.GitStatusUpdate) int64 { // capDiffOutput runs a git diff command and returns at most maxDiffOutputSize bytes. // Returns the output string and whether it was truncated. +// +// Holds a git throttle slot for the full Start → Wait lifetime so streaming +// diffs count against the same global cap as Output/CombinedOutput callers +// — otherwise the throttle could be silently bypassed by switching to +// 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) { cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = workDir @@ -110,6 +117,15 @@ func capDiffOutput(ctx context.Context, workDir string, args ...string) (string, if err != nil { return "", false } + release, err := subproc.Git().Acquire(ctx) + 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 @@ -157,7 +173,7 @@ func resolveNumstatPath(numstatPath string) string { func (wt *WorkspaceTracker) enrichWithUnstagedDiff(ctx context.Context, update *types.GitStatusUpdate, baseRef string) { numstatCmd := exec.CommandContext(ctx, "git", "diff", "--numstat", baseRef) numstatCmd.Dir = wt.workDir - numstatOut, err := numstatCmd.Output() + numstatOut, err := subproc.RunGitOutput(ctx, numstatCmd) if err != nil { wt.logger.Debug("failed to get numstat", zap.Error(err)) return @@ -219,7 +235,7 @@ func (wt *WorkspaceTracker) enrichWithStagedDiff(ctx context.Context, update *ty // and has no additional unstaged changes, its diff won't appear there. stagedCmd := exec.CommandContext(ctx, "git", "diff", "--cached", "--numstat", baseRef) stagedCmd.Dir = wt.workDir - stagedOut, err := stagedCmd.Output() + stagedOut, err := subproc.RunGitOutput(ctx, stagedCmd) if err != nil { return } 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 0dfa874725..5e7e43e9ad 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_poll.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_poll.go @@ -10,6 +10,7 @@ import ( "time" "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -120,7 +121,7 @@ func (wt *WorkspaceTracker) gitPollTick(ctx context.Context, consecutiveFailures // stop after maxConsecutiveGitFailures to avoid wasting CPU. probeCmd := exec.CommandContext(ctx, "git", "rev-parse", "--git-dir") probeCmd.Dir = wt.workDir - if err := probeCmd.Run(); err != nil { + if err := subproc.RunGit(ctx, probeCmd); err != nil { *consecutiveFailures++ if *consecutiveFailures >= maxConsecutiveGitFailures { wt.logger.Error("git not functional, stopping git polling", @@ -141,7 +142,7 @@ func (wt *WorkspaceTracker) gitPollTick(ctx context.Context, consecutiveFailures func (wt *WorkspaceTracker) getHeadSHA(ctx context.Context) string { cmd := exec.CommandContext(ctx, "git", "rev-parse", "HEAD") cmd.Dir = wt.workDir - out, err := cmd.Output() + out, err := subproc.RunGitOutput(ctx, cmd) if err != nil { return "" } @@ -153,7 +154,7 @@ func (wt *WorkspaceTracker) getHeadSHA(ctx context.Context) string { func (wt *WorkspaceTracker) getCurrentBranchName(ctx context.Context) string { cmd := exec.CommandContext(ctx, "git", "symbolic-ref", "--short", "-q", "HEAD") cmd.Dir = wt.workDir - out, err := cmd.Output() + out, err := subproc.RunGitOutput(ctx, cmd) if err != nil { return "" } @@ -167,7 +168,7 @@ func (wt *WorkspaceTracker) getCurrentBranchName(ctx context.Context) string { // directory traversal that --untracked-files=all performs on large repos. func (wt *WorkspaceTracker) getGitStatusHash(ctx context.Context) string { cmd := wt.pollingGitCommand(ctx, "status", "--porcelain", "--untracked-files=no") - out, err := cmd.Output() + out, err := subproc.RunGitOutput(ctx, cmd) if err != nil { return "" } @@ -347,11 +348,15 @@ func (wt *WorkspaceTracker) getBaseCommitForBranch(ctx context.Context, branch, var baseBranch string // Try integration branch candidates first (origin/main, origin/master, main, master) - // This ensures we get the branch-off point from the main development line + // 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 + // 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 := checkCmd.Run(); err == nil { + if err := subproc.RunGit(ctx, checkCmd); err == nil { baseBranch = candidate break } @@ -361,7 +366,7 @@ func (wt *WorkspaceTracker) getBaseCommitForBranch(ctx context.Context, branch, if baseBranch == "" { upstreamCmd := exec.CommandContext(ctx, "git", "rev-parse", "--abbrev-ref", branch+"@{upstream}") upstreamCmd.Dir = wt.workDir - upstreamOut, err := upstreamCmd.Output() + upstreamOut, err := subproc.RunGitOutput(ctx, upstreamCmd) if err == nil && len(upstreamOut) > 0 { baseBranch = strings.TrimSpace(string(upstreamOut)) } @@ -372,7 +377,7 @@ func (wt *WorkspaceTracker) getBaseCommitForBranch(ctx context.Context, branch, if baseBranch != "" && head != "" { mergeBaseCmd := exec.CommandContext(ctx, "git", "merge-base", baseBranch, head) mergeBaseCmd.Dir = wt.workDir - if mergeBaseOut, err := mergeBaseCmd.Output(); err == nil { + if mergeBaseOut, err := subproc.RunGitOutput(ctx, mergeBaseCmd); err == nil { return strings.TrimSpace(string(mergeBaseOut)) } } @@ -384,7 +389,7 @@ func (wt *WorkspaceTracker) getBaseCommitForBranch(ctx context.Context, branch, 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 := cmd.Run() + err := subproc.RunGit(ctx, cmd) // Exit code 0 means commit1 IS an ancestor of commit2 // Exit code 1 means commit1 is NOT an ancestor of commit2 return err == nil @@ -397,7 +402,7 @@ func (wt *WorkspaceTracker) isOnRemote(ctx context.Context, commitSHA string) bo // 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 := cmd.Output() + out, err := subproc.RunGitOutput(ctx, cmd) 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_status.go b/apps/backend/internal/agentctl/server/process/workspace_git_status.go index e2438af702..6c946fa7f3 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status.go @@ -8,6 +8,7 @@ import ( "time" "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -122,7 +123,7 @@ func (wt *WorkspaceTracker) getGitBranchInfo(ctx context.Context, update *types. // Get current branch branchCmd := exec.CommandContext(ctx, "git", "rev-parse", "--abbrev-ref", "HEAD") branchCmd.Dir = wt.workDir - branchOut, err := branchCmd.Output() + branchOut, err := subproc.RunGitOutput(ctx, branchCmd) if err != nil { return err } @@ -131,14 +132,14 @@ func (wt *WorkspaceTracker) getGitBranchInfo(ctx context.Context, update *types. // Get remote branch remoteCmd := exec.CommandContext(ctx, "git", "rev-parse", "--abbrev-ref", "@{upstream}") remoteCmd.Dir = wt.workDir - if remoteOut, err := remoteCmd.Output(); err == nil { + if remoteOut, err := subproc.RunGitOutput(ctx, remoteCmd); err == nil { update.RemoteBranch = strings.TrimSpace(string(remoteOut)) } // Get current HEAD commit SHA headCmd := exec.CommandContext(ctx, "git", "rev-parse", "HEAD") headCmd.Dir = wt.workDir - if headOut, err := headCmd.Output(); err == nil { + if headOut, err := subproc.RunGitOutput(ctx, headCmd); err == nil { update.HeadCommit = strings.TrimSpace(string(headOut)) } @@ -149,7 +150,7 @@ func (wt *WorkspaceTracker) getGitBranchInfo(ctx context.Context, update *types. 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 := checkCmd.Run(); err == nil { + if err := subproc.RunGit(ctx, checkCmd); err == nil { baseBranch = candidate break } @@ -159,7 +160,7 @@ func (wt *WorkspaceTracker) getGitBranchInfo(ctx context.Context, update *types. // This is correct even when the branch has diverged from main. mergeBaseCmd := exec.CommandContext(ctx, "git", "merge-base", baseBranch, "HEAD") mergeBaseCmd.Dir = wt.workDir - if mergeBaseOut, err := mergeBaseCmd.Output(); err == nil { + if mergeBaseOut, err := subproc.RunGitOutput(ctx, mergeBaseCmd); err == nil { update.BaseCommit = strings.TrimSpace(string(mergeBaseOut)) } } @@ -179,7 +180,7 @@ func (wt *WorkspaceTracker) getAheadBehindCounts(ctx context.Context, update *ty for _, candidate := range []string{"origin/main", "origin/master"} { checkCmd := exec.CommandContext(ctx, "git", "rev-parse", "--verify", candidate) checkCmd.Dir = wt.workDir - if err := checkCmd.Run(); err == nil { + if err := subproc.RunGit(ctx, checkCmd); err == nil { compareRef = candidate break } @@ -189,7 +190,7 @@ func (wt *WorkspaceTracker) getAheadBehindCounts(ctx context.Context, update *ty } countCmd := exec.CommandContext(ctx, "git", "rev-list", "--left-right", "--count", update.Branch+"..."+compareRef) countCmd.Dir = wt.workDir - if countOut, err := countCmd.Output(); err == nil { + if countOut, err := subproc.RunGitOutput(ctx, countCmd); err == nil { parts := strings.Fields(string(countOut)) if len(parts) == 2 { update.Ahead, _ = strconv.Atoi(parts[0]) @@ -204,7 +205,7 @@ func (wt *WorkspaceTracker) parseGitStatusOutput(ctx context.Context, update *ty // GIT_OPTIONAL_LOCKS=0 (via pollingGitCommand) prevents the background poll loop from taking // .git/index.lock, which would race with concurrent user-initiated git operations. statusCmd := wt.pollingGitCommand(ctx, "status", "--porcelain", "--untracked-files=all") - statusOut, err := statusCmd.Output() + statusOut, err := subproc.RunGitOutput(ctx, statusCmd) if err != nil { return err } diff --git a/apps/backend/internal/agentctl/server/process/workspace_monitor.go b/apps/backend/internal/agentctl/server/process/workspace_monitor.go index ef427cc51a..9fed11af14 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_monitor.go +++ b/apps/backend/internal/agentctl/server/process/workspace_monitor.go @@ -11,6 +11,7 @@ import ( "time" "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -231,7 +232,7 @@ func (wt *WorkspaceTracker) getWorkspaceState(ctx context.Context) (workspaceSta cmd := wt.pollingGitCommand(gitCtx, "diff-files", "--name-only") var stderrBuf bytes.Buffer cmd.Stderr = &stderrBuf - out, err := cmd.Output() + out, err := subproc.RunGitOutput(gitCtx, cmd) if err != nil { return state, fmt.Errorf("git diff-files in %s: %w (stderr: %s)", wt.workDir, err, strings.TrimSpace(stderrBuf.String())) @@ -284,7 +285,7 @@ func (wt *WorkspaceTracker) getUntrackedFilesID(ctx context.Context) (string, er cmd := wt.pollingGitCommand(ctx, "ls-files", "--others", "--exclude-standard") var stderrBuf bytes.Buffer cmd.Stderr = &stderrBuf - out, err := cmd.Output() + out, err := subproc.RunGitOutput(ctx, cmd) if err != nil { return "", fmt.Errorf("git ls-files in %s: %w (stderr: %s)", wt.workDir, err, strings.TrimSpace(stderrBuf.String())) diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker.go b/apps/backend/internal/agentctl/server/process/workspace_tracker.go index c4cafbe01b..cd0cef5915 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker.go @@ -11,6 +11,7 @@ import ( "github.com/kandev/kandev/internal/agentctl/types" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -211,7 +212,21 @@ func resolveGitIndexPath(workDir string) string { if !workDirHasOwnGitEntry(workDir) { return "" } - cmd := exec.Command("git", "rev-parse", "--git-dir") + // 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. + 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 { diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go index 092099394a..63acf560cf 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go @@ -743,7 +743,7 @@ func TestApplyFileDiff_RegularFile(t *testing.T) { // Build a unified diff that changes line2 -> modified diff := "--- test.txt\n+++ test.txt\n@@ -1,3 +1,3 @@\n line1\n-line2\n+modified\n line3\n" - hash, resolution, err := wt.ApplyFileDiff("test.txt", diff, "", nil) + hash, resolution, err := wt.ApplyFileDiff(context.Background(), "test.txt", diff, "", nil) if err != nil { t.Fatalf("ApplyFileDiff failed: %v", err) } @@ -798,7 +798,7 @@ func TestApplyFileDiff_Symlink(t *testing.T) { // Build a diff targeting the symlink path diff := "--- LINK.md\n+++ LINK.md\n@@ -1,3 +1,3 @@\n line1\n-line2\n+patched\n line3\n" - hash, resolution, err := wt.ApplyFileDiff("LINK.md", diff, "", nil) + hash, resolution, err := wt.ApplyFileDiff(context.Background(), "LINK.md", diff, "", nil) if err != nil { t.Fatalf("ApplyFileDiff through symlink failed: %v", err) } @@ -859,7 +859,7 @@ func TestApplyFileDiff_ConflictDetection(t *testing.T) { diff := "--- conflict.txt\n+++ conflict.txt\n@@ -1 +1 @@\n-original\n+patched\n" - _, _, err := wt.ApplyFileDiff("conflict.txt", diff, origHash, nil) + _, _, err := wt.ApplyFileDiff(context.Background(), "conflict.txt", diff, origHash, nil) if err == nil { t.Fatal("expected conflict error, got nil") } @@ -929,7 +929,7 @@ func TestApplyFileDiff_ConflictWithDesiredContent(t *testing.T) { diff := "--- file.txt\n+++ file.txt\n@@ -1 +1 @@\n-original\n+user-version\n" desiredContent := "user-desired-content\n" - newHash, resolution, err := wt.ApplyFileDiff("file.txt", diff, origHash, &desiredContent) + newHash, resolution, err := wt.ApplyFileDiff(context.Background(), "file.txt", diff, origHash, &desiredContent) if err != nil { t.Fatalf("ApplyFileDiff with desiredContent should not fail: %v", err) } @@ -968,7 +968,7 @@ func TestApplyFileDiff_ConflictWithoutDesiredContent(t *testing.T) { diff := "--- file.txt\n+++ file.txt\n@@ -1 +1 @@\n-original\n+user-version\n" // Without desiredContent, conflict should still fail - _, _, err := wt.ApplyFileDiff("file.txt", diff, origHash, nil) + _, _, err := wt.ApplyFileDiff(context.Background(), "file.txt", diff, origHash, nil) if err == nil { t.Fatal("expected conflict error, got nil") } @@ -1008,7 +1008,7 @@ func TestApplyFileDiff_SymlinkConflictWithDesiredContent(t *testing.T) { diff := "--- LINK.md\n+++ LINK.md\n@@ -1 +1 @@\n-original\n+user-version\n" desiredContent := "user-content\n" - newHash, resolution, err := wt.ApplyFileDiff("LINK.md", diff, origHash, &desiredContent) + newHash, resolution, err := wt.ApplyFileDiff(context.Background(), "LINK.md", diff, origHash, &desiredContent) if err != nil { t.Fatalf("ApplyFileDiff with desiredContent through symlink should not fail: %v", err) } diff --git a/apps/backend/internal/common/subproc/metrics.go b/apps/backend/internal/common/subproc/metrics.go new file mode 100644 index 0000000000..61af9cf992 --- /dev/null +++ b/apps/backend/internal/common/subproc/metrics.go @@ -0,0 +1,89 @@ +package subproc + +import ( + "expvar" + "strconv" + "time" +) + +// expvar maps published at package init, exposed via stdlib's +// /debug/vars handler. Each map is keyed by Throttle.name ("gh", "git", +// ...) so a single backend process surfaces both pools side-by-side +// without an extra metrics pipeline. Unnamed throttles (tests, ad-hoc +// pools) bypass these maps entirely so /debug/vars stays clean. +// +// Counters and gauges only; the wait-millis sum lets you compute a +// rolling average via two scrapes (delta(sum)/delta(acquires)). p95 / +// histograms would need a more invasive change — kept out for now since +// the host-freeze investigation only needs "is there a queue, and how +// long is the wait?" +// +// Naming follows the existing routing_* / watcher_dispatch_* precedent +// in apps/backend/internal/office/scheduler/metrics_vars.go and +// internal/orchestrator/watcher_throttle.go. +var ( + subprocCap = expvar.NewMap("subproc_cap") + subprocInflight = expvar.NewMap("subproc_inflight") + subprocWaiters = expvar.NewMap("subproc_waiters") + subprocAcquireTotal = expvar.NewMap("subproc_acquire_total") + subprocAcquireWaitMillis = expvar.NewMap("subproc_acquire_wait_millis_total") +) + +// 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 +// the production map. +func (t *Throttle) publishCap(c int) { + if t.name == "" { + return + } + v := new(expvar.Int) + v.Set(int64(c)) + subprocCap.Set(t.name, v) +} + +func (t *Throttle) incInflight(delta int64) { + if t.name == "" { + return + } + subprocInflight.Add(t.name, delta) +} + +func (t *Throttle) incWaiters(delta int64) { + if t.name == "" { + return + } + subprocWaiters.Add(t.name, delta) +} + +// incAcquire bumps the acquire counter and, when waited > 0, the +// cumulative wait gauge. Wait time is recorded in whole milliseconds — +// the host-freeze investigation cared about second-level deltas, not +// sub-ms precision, and integer math keeps the expvar.Map type stable. +func (t *Throttle) incAcquire(waited time.Duration) { + if t.name == "" { + return + } + subprocAcquireTotal.Add(t.name, 1) + if waited > 0 { + ms := waited.Milliseconds() + if ms > 0 { + subprocAcquireWaitMillis.Add(t.name, ms) + } + } +} + +// metricInt reads a Throttle metric for tests. The expvar.Map exposes +// Get(string) but returns expvar.Var; this unwraps to int64 with a +// sane default for missing keys. +func metricInt(m *expvar.Map, key string) int64 { + v := m.Get(key) + if v == nil { + return 0 + } + parsed, err := strconv.ParseInt(v.String(), 10, 64) + if err != nil { + return 0 + } + return parsed +} diff --git a/apps/backend/internal/common/subproc/metrics_test.go b/apps/backend/internal/common/subproc/metrics_test.go new file mode 100644 index 0000000000..394f9c556d --- /dev/null +++ b/apps/backend/internal/common/subproc/metrics_test.go @@ -0,0 +1,112 @@ +package subproc + +import ( + "context" + "sync" + "testing" + "testing/synctest" +) + +// TestThrottle_MetricsPublishedForNamedPool covers the happy path where +// /debug/vars should now show inflight + acquire counts climbing as +// callers grab slots. Without these metrics the host-freeze +// investigation has no way to tell whether a stall is throttle queue +// time or something else. +func TestThrottle_MetricsPublishedForNamedPool(t *testing.T) { + name := "test-pool-named-" + t.Name() + tt := NewNamedThrottle(name, 2) + if got := metricInt(subprocCap, name); got != 2 { + t.Fatalf("cap gauge = %d, want 2", got) + } + + r1, err := tt.Acquire(context.Background()) + if err != nil { + t.Fatalf("acquire 1: %v", err) + } + if got := metricInt(subprocInflight, name); got != 1 { + t.Fatalf("inflight after 1 acquire = %d, want 1", got) + } + if got := metricInt(subprocAcquireTotal, name); got != 1 { + t.Fatalf("acquire_total after 1 acquire = %d, want 1", got) + } + + r2, err := tt.Acquire(context.Background()) + if err != nil { + t.Fatalf("acquire 2: %v", err) + } + if got := metricInt(subprocInflight, name); got != 2 { + t.Fatalf("inflight after 2 acquires = %d, want 2", got) + } + r1() + if got := metricInt(subprocInflight, name); got != 1 { + t.Fatalf("inflight after 1 release = %d, want 1", got) + } + r2() + if got := metricInt(subprocInflight, name); got != 0 { + t.Fatalf("inflight after both released = %d, want 0", got) + } +} + +// TestThrottle_WaitersGaugeReflectsQueueDepth is the regression test for +// the failure mode that triggered this metric: under saturation we want +// to see waiters > 0 so an operator inspecting /debug/vars knows the +// queue, not the operation itself, is the bottleneck. +func TestThrottle_WaitersGaugeReflectsQueueDepth(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + name := "test-pool-waiters-" + t.Name() + tt := NewNamedThrottle(name, 1) + + hold, err := tt.Acquire(context.Background()) + if err != nil { + t.Fatalf("initial acquire: %v", err) + } + defer hold() + + var wg sync.WaitGroup + wg.Add(3) + releases := make(chan func(), 3) + for i := 0; i < 3; i++ { + go func() { + defer wg.Done() + r, _ := tt.Acquire(context.Background()) + releases <- r + }() + } + // All three goroutines registered as waiters once they're + // durably blocked on the semaphore send. + synctest.Wait() + if got := metricInt(subprocWaiters, name); got != 3 { + t.Fatalf("waiters gauge = %d, want 3", got) + } + + hold() + r1 := <-releases + r1() + r2 := <-releases + r2() + r3 := <-releases + r3() + wg.Wait() + if got := metricInt(subprocWaiters, name); got != 0 { + t.Fatalf("waiters after drain = %d, want 0", got) + } + }) +} + +// TestThrottle_UnnamedThrottleSkipsMetrics keeps the metrics surface +// clean for tests and ad-hoc pools — the global gh/git keys would +// otherwise see counter pollution from every test file that builds a +// NewThrottle(N) sample pool. +func TestThrottle_UnnamedThrottleSkipsMetrics(t *testing.T) { + tt := NewThrottle(1) + before := metricInt(subprocAcquireTotal, "") + r, err := tt.Acquire(context.Background()) + if err != nil { + t.Fatalf("acquire: %v", err) + } + r() + after := metricInt(subprocAcquireTotal, "") + if after != before { + t.Fatalf("acquire_total bumped on unnamed throttle (before=%d after=%d)", before, after) + } +} diff --git a/apps/backend/internal/common/subproc/shared.go b/apps/backend/internal/common/subproc/shared.go new file mode 100644 index 0000000000..0ee6eee90d --- /dev/null +++ b/apps/backend/internal/common/subproc/shared.go @@ -0,0 +1,138 @@ +package subproc + +import ( + "context" + "os" + "os/exec" + "strconv" +) + +// Shared throttle singletons. +// +// Both gh and git are spawned from many packages across the backend +// (PR poller, worktree manager, agentctl process group, agent lifecycle +// credential uploader, repoclone, ...). To make the cap actually global +// across the process, the singleton lives here — at the lowest layer +// any of those callers already depend on — instead of inside one of the +// higher-level packages. That way no caller has to import a sibling +// package solely to share its semaphore. + +const ( + // defaultGHMaxConcurrent and defaultGitMaxConcurrent are sized to + // stay below the spawn rate at which macOS code-signing + EDR + // latency (CrowdStrike Falcon + syspolicyd) starts to back up and + // freeze the host. Git's cap is higher than gh's because typical + // git work is local-only and drains the queue faster. + defaultGHMaxConcurrent = 8 + defaultGitMaxConcurrent = 12 + + ghMaxConcurrentEnv = "KANDEV_GH_MAX_CONCURRENT" + gitMaxConcurrentEnv = "KANDEV_GIT_MAX_CONCURRENT" +) + +var ( + // Names ("gh", "git") double as the expvar.Map keys under + // /debug/vars (subproc_cap, subproc_inflight, subproc_waiters, + // subproc_acquire_total, subproc_acquire_wait_millis_total). Unit + // 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)) +) + +// GH returns the process-wide throttle gating gh subprocess execs. +// All gh callers across the codebase share this single semaphore so the +// total host fork pressure stays bounded regardless of caller count. +func GH() *Throttle { return ghThrottle } + +// Git returns the process-wide throttle gating git subprocess execs. +// Shared by the worktree manager, agentctl process group, agent runtime +// env preparers, and any other call site that shells out to git. +func Git() *Throttle { return gitThrottle } + +// resolveCap reads env for an integer cap, falling back to def for +// missing/invalid/non-positive values. cap parsing is intentionally +// permissive in only the failure direction — typos and clears revert +// to the safe default rather than disabling the throttle silently. +func resolveCap(env string, def int) int { + raw := os.Getenv(env) + if raw == "" { + return def + } + n, err := strconv.Atoi(raw) + if err != nil || n <= 0 { + return def + } + return n +} + +// resolveGHMaxConcurrent and resolveGitMaxConcurrent are kept as +// package-private accessors so the unit tests can verify env parsing +// without exporting the parser itself. Production code constructs the +// singleton at init time and never re-reads the env. +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) + if err != nil { + return err + } + defer release() + return cmd.Run() +} + +// RunGitCombinedOutput is RunGit's CombinedOutput sibling. +func RunGitCombinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + release, err := gitThrottle.Acquire(ctx) + if err != nil { + return nil, err + } + defer release() + 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) + if err != nil { + return nil, err + } + defer release() + return cmd.Output() +} + +// 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 +// matching helper to both rather than open-coding Acquire/release. +func RunGH(ctx context.Context, cmd *exec.Cmd) error { + release, err := ghThrottle.Acquire(ctx) + if err != nil { + return err + } + defer release() + return cmd.Run() +} + +func RunGHOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + release, err := ghThrottle.Acquire(ctx) + if err != nil { + return nil, err + } + defer release() + return cmd.Output() +} + +func RunGHCombinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + release, err := ghThrottle.Acquire(ctx) + if err != nil { + return nil, err + } + defer release() + return cmd.CombinedOutput() +} diff --git a/apps/backend/internal/common/subproc/shared_test.go b/apps/backend/internal/common/subproc/shared_test.go new file mode 100644 index 0000000000..7ab4401e1e --- /dev/null +++ b/apps/backend/internal/common/subproc/shared_test.go @@ -0,0 +1,71 @@ +package subproc + +import "testing" + +func TestResolveCap(t *testing.T) { + const env = "KANDEV_SUBPROC_CAP_TEST" + const def = 7 + + cases := []struct { + name string + val string + want int + }{ + {"empty", "", def}, + {"valid", "3", 3}, + {"zero falls back", "0", def}, + {"negative falls back", "-1", def}, + {"garbage falls back", "abc", def}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(env, tc.val) + if got := resolveCap(env, def); got != tc.want { + t.Errorf("resolveCap(%q, %d) = %d, want %d", tc.val, def, got, tc.want) + } + }) + } +} + +// TestResolveGHMaxConcurrent and TestResolveGitMaxConcurrent guard the +// process-wide defaults: a typo'd env or a fall-through must land on +// the safe default rather than disable the throttle. +func TestResolveGHMaxConcurrent(t *testing.T) { + t.Setenv(ghMaxConcurrentEnv, "") + if got := resolveGHMaxConcurrent(); got != defaultGHMaxConcurrent { + t.Errorf("resolveGHMaxConcurrent() = %d, want %d", got, defaultGHMaxConcurrent) + } + t.Setenv(ghMaxConcurrentEnv, "garbage") + if got := resolveGHMaxConcurrent(); got != defaultGHMaxConcurrent { + t.Errorf("garbage env: got %d, want %d", got, defaultGHMaxConcurrent) + } + t.Setenv(ghMaxConcurrentEnv, "5") + if got := resolveGHMaxConcurrent(); got != 5 { + t.Errorf("valid env: got %d, want 5", got) + } +} + +func TestResolveGitMaxConcurrent(t *testing.T) { + t.Setenv(gitMaxConcurrentEnv, "") + if got := resolveGitMaxConcurrent(); got != defaultGitMaxConcurrent { + t.Errorf("resolveGitMaxConcurrent() = %d, want %d", got, defaultGitMaxConcurrent) + } + t.Setenv(gitMaxConcurrentEnv, "0") + if got := resolveGitMaxConcurrent(); got != defaultGitMaxConcurrent { + t.Errorf("zero env: got %d, want %d", got, defaultGitMaxConcurrent) + } + t.Setenv(gitMaxConcurrentEnv, "20") + if got := resolveGitMaxConcurrent(); got != 20 { + t.Errorf("valid env: got %d, want 20", got) + } +} + +// TestGHGitAreDistinctThrottles verifies the two singletons aren't +// accidentally aliased. A regression where both helpers return the same +// pool would let a gh storm starve git ops (or vice-versa) — exactly +// the cross-contention the per-binary split is meant to prevent. +func TestGHGitAreDistinctThrottles(t *testing.T) { + if GH() == Git() { + t.Fatal("GH() and Git() returned the same throttle instance") + } +} diff --git a/apps/backend/internal/common/subproc/throttle.go b/apps/backend/internal/common/subproc/throttle.go new file mode 100644 index 0000000000..e544db3610 --- /dev/null +++ b/apps/backend/internal/common/subproc/throttle.go @@ -0,0 +1,194 @@ +// Package subproc provides a cross-package primitive for capping concurrent +// external-binary execs (gh, git, ...). +// +// Why this exists: +// +// On managed corporate macOS hosts (CrowdStrike Falcon + syspolicyd code +// signing), every fork/exec is intercepted, serialized, and validated. +// kandev workloads that fan out subprocesses (PR poller, worktree creation, +// agent lifecycle) can fire bursts of 40+ external execs in a few seconds, +// turning that EDR queue into a multi-second backlog. Other processes on the +// host queue behind us — including kandev's own retries — and the result is +// a system-wide UI freeze plus a wave of 30s-timeout kills inside kandev. +// +// A Throttle caps concurrent execs for one binary. Callers that arrive past +// the cap wait in line, context-aware: a cancelled/deadlined caller exits +// without spawning anything. Each binary owns its own Throttle so a busy +// gh stream cannot starve a worktree-creation git burst (or vice-versa). +package subproc + +import ( + "context" + "sync" + "time" +) + +// Throttle caps concurrent users of a resource via a fixed-size slot pool. +// Zero value is unusable — construct via NewThrottle or NewNamedThrottle. +// +// Throttle is safe for concurrent use. The slot pool can be hot-swapped via +// SetCapForTest so unit tests can drive the semaphore at small caps without +// affecting other tests in the same binary. +// +// When name != "" the Throttle publishes inflight / waiters / acquire / wait +// counters into the package-level expvar maps (see metrics.go) so /debug/vars +// 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 +} + +// NewThrottle returns a Throttle with the given cap and no name. cap <= 0 +// disables throttling (Acquire returns a no-op release immediately). +// +// Production code that wants the throttle's metrics published under +// /debug/vars should use NewNamedThrottle. NewThrottle stays in place for +// tests and for any caller that intentionally wants an unobservable pool. +func NewThrottle(cap int) *Throttle { + return NewNamedThrottle("", cap) +} + +// NewNamedThrottle returns a Throttle that publishes inflight / waiters / +// acquire-count / acquire-wait-millis under the package-level expvar maps +// using name as the label key. Use this for process-wide singletons (gh, +// git) so saturation is observable from /debug/vars without a separate +// metrics pipeline. +func NewNamedThrottle(name string, cap int) *Throttle { + t := &Throttle{name: name} + if cap > 0 { + t.sem = make(chan struct{}, cap) + } + t.publishCap(cap) + 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. +// +// Fast path: if ctx is already cancelled at entry, return immediately +// without racing the select against an available slot. Without this, +// Go's select picks randomly between sem<- and ctx.Done() when both are +// 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 err := ctx.Err(); err != nil { + return noopRelease, err + } + sem := t.currentSem() + if sem == nil { + // Throttle disabled (cap<=0) or swapped out by a test teardown. + // Still bump the acquire counter for visibility but don't record + // a wait — there's nothing to queue behind. + t.incAcquire(0) + return noopRelease, nil + } + // Fast path: try to grab a slot without registering as a waiter. + // Most acquires under normal load hit this branch and add zero metric + // overhead. Only the contended path below has to bump the waiter gauge. + select { + case sem <- struct{}{}: + // Re-check ctx after the send: between the pre-check above and + // here, ctx may have been cancelled by another goroutine. Without + // this, a cancelled caller would silently acquire a slot and the + // downstream exec.CommandContext would fail late while holding + // it. Bounce the slot back so the next waiter can use it. Record + // the acquire with 0 wait so cancellation accounting matches the + // slow-path cancel branch below. + if err := ctx.Err(); err != nil { + <-sem + t.incAcquire(0) + return noopRelease, err + } + t.incAcquire(0) + t.incInflight(1) + return t.releaseFunc(sem), nil + default: + } + // Slow path: register as a waiter so /debug/vars reflects current + // queue depth, then block on the select. The deferred dec ensures we + // drop the waiter count whether we acquire, get cancelled, or panic. + t.incWaiters(1) + start := time.Now() + defer t.incWaiters(-1) + select { + case sem <- struct{}{}: + // Same race as the fast path: when both sem<- and ctx.Done() are + // ready, Go's select picks randomly, so a cancelled caller can + // still land in this branch. Re-check and bounce on cancellation. + if err := ctx.Err(); err != nil { + <-sem + t.incAcquire(time.Since(start)) + return noopRelease, err + } + t.incAcquire(time.Since(start)) + t.incInflight(1) + return t.releaseFunc(sem), nil + case <-ctx.Done(): + t.incAcquire(time.Since(start)) + return noopRelease, ctx.Err() + } +} + +// 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 +// slot belonging to another in-flight caller — without it the pool's +// effective cap would slowly leak to zero. +func (t *Throttle) releaseFunc(sem chan struct{}) func() { + var once sync.Once + return func() { + once.Do(func() { + select { + case <-sem: + t.incInflight(-1) + default: + // Pool was swapped out from under us (test teardown). + // Releasing a slot we never held is a no-op. + } + }) + } +} + +func noopRelease() {} + +// currentSem returns the active semaphore under a read lock. +func (t *Throttle) currentSem() chan struct{} { + t.mu.RLock() + defer t.mu.RUnlock() + return t.sem +} + +// SetCapForTest replaces the slot pool with one of the given capacity and +// returns a restore function. Test-only — production code MUST NOT call +// this. Use cap=0 to disable throttling for a test. +// +// 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() { + t.mu.Lock() + prev := t.sem + prevCap := 0 + if prev != nil { + prevCap = cap(prev) + } + if newCap <= 0 { + t.sem = nil + } else { + t.sem = make(chan struct{}, newCap) + } + t.mu.Unlock() + t.publishCap(newCap) + return func() { + t.mu.Lock() + t.sem = prev + t.mu.Unlock() + t.publishCap(prevCap) + } +} diff --git a/apps/backend/internal/common/subproc/throttle_test.go b/apps/backend/internal/common/subproc/throttle_test.go new file mode 100644 index 0000000000..113eb5fc09 --- /dev/null +++ b/apps/backend/internal/common/subproc/throttle_test.go @@ -0,0 +1,157 @@ +package subproc + +import ( + "context" + "errors" + "sync" + "sync/atomic" + "testing" + "testing/synctest" + "time" +) + +func TestThrottle_BlocksPastCap(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + tt := NewThrottle(2) + ctx := context.Background() + + rA, err := tt.Acquire(ctx) + if err != nil { + t.Fatalf("acquire A: %v", err) + } + rB, err := tt.Acquire(ctx) + if err != nil { + t.Fatalf("acquire B: %v", err) + } + + blocked := make(chan struct{}) + go func() { + r, _ := tt.Acquire(ctx) + defer r() + close(blocked) + }() + // Wait until every goroutine in the bubble is durably blocked. + // If the third Acquire ran (cap not honoured) `blocked` would be + // closed by now. + synctest.Wait() + select { + case <-blocked: + t.Errorf("third acquire returned without waiting — throttle did not block") + default: + } + rA() + <-blocked + rB() + }) +} + +func TestThrottle_ContextCancelWhileQueued(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + tt := NewThrottle(1) + ctx := context.Background() + r, err := tt.Acquire(ctx) + if err != nil { + t.Fatalf("acquire: %v", err) + } + defer r() + + cctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { + _, err := tt.Acquire(cctx) + done <- err + }() + // Wait for the queued Acquire to register as a waiter. + synctest.Wait() + cancel() + got := <-done + if !errors.Is(got, context.Canceled) { + t.Errorf("err = %v, want context.Canceled", got) + } + }) +} + +func TestThrottle_DoubleReleaseIsNoOp(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + tt := NewThrottle(2) + ctx := context.Background() + rA, _ := tt.Acquire(ctx) + rB, _ := tt.Acquire(ctx) + + rA() + rA() // double release must be a no-op + + rC, err := tt.Acquire(ctx) + if err != nil { + t.Fatalf("acquire after double release: %v", err) + } + defer rC() + + blocked := make(chan struct{}) + go func() { + r, _ := tt.Acquire(ctx) + defer r() + close(blocked) + }() + synctest.Wait() + select { + case <-blocked: + t.Errorf("4th acquire ran — double release leaked a slot") + default: + } + rB() + <-blocked + }) +} + +func TestThrottle_PeakConcurrencyHonoured(t *testing.T) { + tt := NewThrottle(4) + var active, peak int64 + var wg sync.WaitGroup + for i := 0; i < 32; i++ { + wg.Add(1) + go func() { + defer wg.Done() + r, err := tt.Acquire(context.Background()) + if err != nil { + return + } + defer r() + cur := atomic.AddInt64(&active, 1) + for { + p := atomic.LoadInt64(&peak) + if cur <= p || atomic.CompareAndSwapInt64(&peak, p, cur) { + break + } + } + time.Sleep(2 * time.Millisecond) + atomic.AddInt64(&active, -1) + }() + } + wg.Wait() + if peak > 4 { + t.Errorf("peak concurrency = %d, want <= 4", peak) + } +} + +func TestThrottle_DisabledWhenCapZero(t *testing.T) { + tt := NewThrottle(0) + r, err := tt.Acquire(context.Background()) + if err != nil { + t.Fatalf("acquire on disabled throttle: %v", err) + } + r() +} + +func TestThrottle_PreCancelledContextReturnsImmediately(t *testing.T) { + tt := NewThrottle(8) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + for i := 0; i < 50; i++ { + release, err := tt.Acquire(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("iter %d: err = %v, want context.Canceled", i, err) + } + release() + } +} diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index 0bf682ee7b..dd1edfe16a 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -126,6 +126,15 @@ func (c *GHClient) IsAuthenticated(ctx context.Context) (bool, error) { func (c *GHClient) RunAuthDiagnostics(ctx context.Context) *AuthDiagnostics { ctx, cancel := withDefaultGHTimeout(ctx) defer cancel() + release, err := acquireGHSlot(ctx) + if err != nil { + return &AuthDiagnostics{ + Command: "gh auth status --hostname github.com", + Output: err.Error(), + ExitCode: -1, + } + } + defer release() cmd := exec.CommandContext(ctx, "gh", "auth", "status", "--hostname", "github.com") var stdout, stderr bytes.Buffer cmd.Stdout = &stdout @@ -851,6 +860,11 @@ func withDefaultGHTimeout(ctx context.Context) (context.Context, context.CancelF func (c *GHClient) run(ctx context.Context, args ...string) (string, error) { ctx, cancel := withDefaultGHTimeout(ctx) defer cancel() + release, err := acquireGHSlot(ctx) + if err != nil { + return "", fmt.Errorf("gh %s: %w", firstArg(args), err) + } + defer release() cmd := exec.CommandContext(ctx, "gh", args...) var stdout, stderr bytes.Buffer cmd.Stdout = &stdout @@ -867,6 +881,11 @@ func (c *GHClient) run(ctx context.Context, args ...string) (string, error) { func (c *GHClient) runWithStdin(ctx context.Context, stdin []byte, args ...string) (string, error) { ctx, cancel := withDefaultGHTimeout(ctx) defer cancel() + release, err := acquireGHSlot(ctx) + if err != nil { + return "", fmt.Errorf("gh %s: %w", firstArg(args), err) + } + defer release() cmd := exec.CommandContext(ctx, "gh", args...) cmd.Stdin = bytes.NewReader(stdin) var stdout, stderr bytes.Buffer @@ -879,6 +898,15 @@ func (c *GHClient) runWithStdin(ctx context.Context, stdin []byte, args ...strin return stdout.String(), nil } +// firstArg returns args[0] when present, else "". Used for error +// messages when the semaphore acquisition fails before we even exec gh. +func firstArg(args []string) string { + if len(args) == 0 { + return "" + } + return args[0] +} + // ghRateLimitResponse mirrors the GET /rate_limit JSON shape so we can seed // the tracker on startup and after CLI failures without parsing prose. type ghRateLimitResponse struct { diff --git a/apps/backend/internal/github/gh_throttle.go b/apps/backend/internal/github/gh_throttle.go new file mode 100644 index 0000000000..20ebcd30a3 --- /dev/null +++ b/apps/backend/internal/github/gh_throttle.go @@ -0,0 +1,37 @@ +package github + +import ( + "context" + + "github.com/kandev/kandev/internal/common/subproc" +) + +// Why this throttle exists: +// +// Before this, every code path that wanted PR status spawned its own gh +// subprocess. With ~40 active PR watches and a 30s frontend refresh, the +// background sync could fire 40-70 gh invocations within a few seconds. +// On managed corporate macOS hosts (CrowdStrike Falcon + syspolicyd code +// signing) every fork/exec is intercepted, serialized, and validated, +// which turned the burst into a multi-second exec queue. Other processes +// waiting on the same queue (new terminal shells, kandev's own gh probes) +// then hit their 30s ctx deadline and were killed, causing a feedback +// loop that effectively froze the host. +// +// The cap, env var (KANDEV_GH_MAX_CONCURRENT), and singleton live in +// internal/common/subproc so all gh callers across the backend share +// one pool. This file only wraps the package-local accessors so existing +// call sites keep their short names and so the test seam stays here. + +// acquireGHSlot blocks until a gh subprocess slot is available or ctx is +// cancelled. See subproc.Throttle.Acquire for release semantics. +func acquireGHSlot(ctx context.Context) (release func(), err error) { + return subproc.GH().Acquire(ctx) +} + +// setGHSemaphoreForTest replaces the gh throttle's slot pool with one of +// the given capacity and returns a restore function. Test-only — production +// code MUST NOT call this. Use cap=0 to disable throttling for a test. +func setGHSemaphoreForTest(cap int) func() { + return subproc.GH().SetCapForTest(cap) +} diff --git a/apps/backend/internal/github/gh_throttle_test.go b/apps/backend/internal/github/gh_throttle_test.go new file mode 100644 index 0000000000..c85b6ba5f8 --- /dev/null +++ b/apps/backend/internal/github/gh_throttle_test.go @@ -0,0 +1,191 @@ +package github + +import ( + "context" + "errors" + "sync" + "sync/atomic" + "testing" + "time" +) + +func TestAcquireGHSlot_BlocksWhenSaturated(t *testing.T) { + restore := setGHSemaphoreForTest(2) + defer restore() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Hold both slots. + releaseA, errA := acquireGHSlot(ctx) + if errA != nil { + t.Fatalf("acquire A: %v", errA) + } + releaseB, errB := acquireGHSlot(ctx) + if errB != nil { + t.Fatalf("acquire B: %v", errB) + } + + // Third caller must block until a slot frees. + acquired := make(chan struct{}) + var releaseC func() + go func() { + var err error + releaseC, err = acquireGHSlot(ctx) + if err != nil { + t.Errorf("acquire C: unexpected err %v", err) + return + } + close(acquired) + }() + + select { + case <-acquired: + t.Fatalf("third acquire returned without waiting; pool size 2 not enforced") + case <-time.After(50 * time.Millisecond): + } + + releaseA() + select { + case <-acquired: + case <-time.After(time.Second): + t.Fatalf("third acquire did not return after slot freed") + } + releaseB() + releaseC() +} + +func TestAcquireGHSlot_ContextCancelledWhileQueued(t *testing.T) { + restore := setGHSemaphoreForTest(1) + defer restore() + + // Saturate the single slot. + holdCtx, holdCancel := context.WithCancel(context.Background()) + defer holdCancel() + release, err := acquireGHSlot(holdCtx) + if err != nil { + t.Fatalf("acquire hold: %v", err) + } + defer release() + + // Queue a second caller with a short-lived context. + waitCtx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) + defer cancel() + relQueued, err := acquireGHSlot(waitCtx) + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("expected context.DeadlineExceeded, got %v", err) + } + // Release fn must be safe to call even on error so callers can `defer` it + // unconditionally without leaking the held slot. + relQueued() +} + +func TestAcquireGHSlot_ConcurrentCapEnforced(t *testing.T) { + const cap = 4 + const callers = 16 + restore := setGHSemaphoreForTest(cap) + defer restore() + + var inFlight, peak int32 + var wg sync.WaitGroup + for i := 0; i < callers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + release, err := acquireGHSlot(context.Background()) + if err != nil { + t.Errorf("acquire: %v", err) + return + } + defer release() + cur := atomic.AddInt32(&inFlight, 1) + // Track peak concurrency. + for { + old := atomic.LoadInt32(&peak) + if cur <= old || atomic.CompareAndSwapInt32(&peak, old, cur) { + break + } + } + time.Sleep(5 * time.Millisecond) + atomic.AddInt32(&inFlight, -1) + }() + } + wg.Wait() + if got := atomic.LoadInt32(&peak); got > int32(cap) { + t.Errorf("peak concurrency %d exceeded cap %d", got, cap) + } +} + +// TestAcquireGHSlot_DoubleReleaseIsNoOp regression-tests that a release +// closure called twice does not transfer a slot to another in-flight +// holder. Without sync.Once around the release, a double-defer (or a +// refactor that calls release in both an error path and via defer) +// would slowly drain the effective cap to zero. +func TestAcquireGHSlot_DoubleReleaseIsNoOp(t *testing.T) { + restore := setGHSemaphoreForTest(2) + defer restore() + + ctx := context.Background() + releaseA, err := acquireGHSlot(ctx) + if err != nil { + t.Fatalf("acquire A: %v", err) + } + releaseB, err := acquireGHSlot(ctx) + if err != nil { + t.Fatalf("acquire B: %v", err) + } + + // Caller A releases twice — buggy code path. With sync.Once the second + // call is a no-op; without it the second call would steal B's slot. + releaseA() + releaseA() + + // Pool should still have exactly one slot held (by B). A new acquire + // must succeed (slot vacated by A) and a second concurrent acquire + // must block (B still holds the other slot). + releaseC, err := acquireGHSlot(ctx) + if err != nil { + t.Fatalf("acquire C after double release: %v", err) + } + defer releaseC() + + blocked := make(chan struct{}) + go func() { + release, err := acquireGHSlot(ctx) + if err == nil { + release() + } + close(blocked) + }() + select { + case <-blocked: + t.Errorf("third acquire returned without waiting — double release leaked a slot") + case <-time.After(50 * time.Millisecond): + } + releaseB() + <-blocked +} + +// TestAcquireGHSlot_PreCancelledContextReturnsImmediately asserts the +// fast path: when ctx is already done at entry, we don't race the +// select against a free slot. Without the precheck the select would +// pick randomly between sem<- and ctx.Done(), letting a cancelled +// caller still acquire then fail downstream. +func TestAcquireGHSlot_PreCancelledContextReturnsImmediately(t *testing.T) { + restore := setGHSemaphoreForTest(8) + defer restore() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + for i := 0; i < 100; i++ { + release, err := acquireGHSlot(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("iter %d: err = %v, want context.Canceled", i, err) + } + release() // must be safe even on the error path + } +} + +// Cap parsing tests live in internal/common/subproc/shared_test.go now +// that the env var, default, and resolver are owned by that package. diff --git a/apps/backend/internal/github/poller.go b/apps/backend/internal/github/poller.go index eed9f1f88f..c8f931b0db 100644 --- a/apps/backend/internal/github/poller.go +++ b/apps/backend/internal/github/poller.go @@ -187,89 +187,50 @@ func (p *Poller) checkPRWatches(ctx context.Context) { } // tryBatchedPRWatchCheck runs the batched GraphQL flow for the supplied -// watches. Returns true when the path succeeded so the caller can skip the -// per-watch fallback. +// watches via the shared Service.SyncWatchesBatched seam. Returns true +// when the path succeeded so the caller can skip the per-watch fallback. +// The poller's only extra responsibility on top of the service apply is +// publishing PRFeedback events for status changes and merge/close events +// (the on-demand sync path intentionally doesn't publish these). func (p *Poller) tryBatchedPRWatchCheck(ctx context.Context, watches []*PRWatch) bool { - exec, err := graphQLExecutorFor(p.service.client) + results, err := p.service.SyncWatchesBatched(ctx, watches) if err != nil { + p.logger.Debug("batched PR watch check failed", zap.Error(err)) return false } - numbered, searching := splitPRWatches(watches) - statusByKey, ok := p.fetchBatchedStatuses(ctx, exec, numbered, searching) - if !ok { - return false - } - for _, w := range numbered { - key := prStatusCacheKey(w.Owner, w.Repo, w.PRNumber) - status, ok := statusByKey[key] - if !ok { - // Alias missing — PR may have been deleted; fall through to the - // per-watch path which already handles the not-found case. - p.checkSinglePRWatch(ctx, w) + for _, r := range results { + if !r.Found || r.Status == nil { continue } - p.applyPRStatus(ctx, w, status) - } - for _, w := range searching { - key := graphqlBranchKey(w.Owner, w.Repo, w.Branch) - status, ok := statusByKey[key] - if !ok || status == nil || status.PR == nil { - // No PR yet for this branch — record the timestamp like the - // per-watch path so liveness updates. - now := time.Now().UTC() - _ = p.service.store.UpdatePRWatchTimestamps(ctx, w.ID, now, nil, "", "") + // Skip publish when the underlying SyncTaskPR failed — the task_pr + // row is still stale, so emitting a "PR merged" / "checks changed" + // event would put the frontend ahead of the DB until the next poll. + if r.SyncFailed { continue } - p.applyDetectedPR(ctx, w, status.PR) + // Publish PRFeedback event when the watch was previously numbered and + // either the PR transitioned to merged/closed or check/review state + // changed. Searching watches (PRNumber==0) that just got promoted + // don't fire a feedback event — they get an associate-event via + // SyncTaskPR / AssociatePRWithTask inside the service apply. + if r.Watch.PRNumber == 0 { + continue + } + pr := r.Status.PR + if pr != nil && (pr.State == prStateMerged || pr.State == prStateClosed) { + p.publishPRStatusEvent(ctx, r.Watch, r.Status) + continue + } + if r.Changed { + p.publishPRStatusEvent(ctx, r.Watch, r.Status) + } } return true } -// fetchBatchedStatuses runs both the numbered-PR query and the branch query -// against GraphQL. The combined map keys numbered watches by -// prStatusCacheKey and searching watches by graphqlBranchKey, so callers can -// look up either kind in one place. Returns (nil, false) when any required -// query failed so the caller falls back to per-watch checks rather than -// silently absorbing the failure as "no result". -func (p *Poller) fetchBatchedStatuses( - ctx context.Context, exec GraphQLExecutor, numbered, searching []*PRWatch, -) (map[string]*PRStatus, bool) { - combined := make(map[string]*PRStatus) - - if len(numbered) > 0 { - refs := make([]graphQLPRRef, 0, len(numbered)) - for _, w := range numbered { - refs = append(refs, graphQLPRRef{Owner: w.Owner, Repo: w.Repo, Number: w.PRNumber}) - } - out, err := runBatchedPRQuery(ctx, exec, refs) - if err != nil { - p.logger.Debug("batched PR status query failed", zap.Error(err)) - return nil, false - } - for k, v := range out { - combined[k] = v - } - } - if len(searching) > 0 { - refs := make([]graphQLBranchRef, 0, len(searching)) - for _, w := range searching { - refs = append(refs, graphQLBranchRef{Owner: w.Owner, Repo: w.Repo, Branch: w.Branch}) - } - out, err := runBatchedBranchQuery(ctx, exec, refs) - if err != nil { - p.logger.Debug("batched branch query failed", zap.Error(err)) - return nil, false - } - for k, v := range out { - combined[k] = v - } - } - return combined, true -} - // splitPRWatches partitions watches into "we know the PR number" and "still // searching for one on this branch" buckets so each gets its own batched -// query shape. +// query shape. Used by Service.SyncWatchesBatched. func splitPRWatches(watches []*PRWatch) (numbered, searching []*PRWatch) { for _, w := range watches { if w.PRNumber == 0 { @@ -281,57 +242,6 @@ func splitPRWatches(watches []*PRWatch) (numbered, searching []*PRWatch) { return numbered, searching } -// applyPRStatus is the post-fetch processing extracted from -// checkSinglePRWatch so the batched path can reuse it. -func (p *Poller) applyPRStatus(ctx context.Context, watch *PRWatch, status *PRStatus) { - if status == nil { - return - } - hasNew := status.ChecksState != watch.LastCheckStatus || status.ReviewState != watch.LastReviewState - now := time.Now().UTC() - if err := p.service.store.UpdatePRWatchTimestamps(ctx, watch.ID, now, nil, status.ChecksState, status.ReviewState); err != nil { - p.logger.Error("failed to update PR watch timestamps", zap.String("id", watch.ID), zap.Error(err)) - } - if syncErr := p.service.SyncTaskPR(ctx, watch.TaskID, status); syncErr != nil { - p.logger.Error("failed to sync task PR", - zap.String("task_id", watch.TaskID), zap.Error(syncErr)) - return - } - if status.PR != nil && (status.PR.State == prStateMerged || status.PR.State == prStateClosed) { - p.publishPRStatusEvent(ctx, watch, status) - if resetErr := p.service.store.UpdatePRWatchPRNumber(ctx, watch.ID, 0); resetErr != nil { - p.logger.Error("failed to reset completed PR watch", - zap.String("id", watch.ID), zap.Error(resetErr)) - } - return - } - if hasNew { - p.publishPRStatusEvent(ctx, watch, status) - } -} - -// applyDetectedPR is the searching-watch counterpart to applyPRStatus — -// promotes a PRWatch from "searching" to a known PR number and records the -// task association. -func (p *Poller) applyDetectedPR(ctx context.Context, watch *PRWatch, pr *PR) { - now := time.Now().UTC() - _ = p.service.store.UpdatePRWatchTimestamps(ctx, watch.ID, now, nil, "", "") - if err := p.service.store.UpdatePRWatchPRNumber(ctx, watch.ID, pr.Number); err != nil { - p.logger.Error("failed to update PR watch with detected PR", - zap.String("watch_id", watch.ID), zap.Int("pr_number", pr.Number), zap.Error(err)) - return - } - if _, err := p.service.AssociatePRWithTask(ctx, watch.TaskID, watch.RepositoryID, pr); err != nil { - p.logger.Error("failed to associate detected PR with task", - zap.String("task_id", watch.TaskID), zap.Int("pr_number", pr.Number), zap.Error(err)) - return - } - p.logger.Info("detected PR for session branch (batched)", - zap.String("watch_id", watch.ID), - zap.String("branch", watch.Branch), - zap.Int("pr_number", pr.Number)) -} - func (p *Poller) checkSinglePRWatch(ctx context.Context, watch *PRWatch) { // PRWatch with pr_number=0 means we're still searching for a PR on this branch. if watch.PRNumber == 0 { diff --git a/apps/backend/internal/github/poller_test.go b/apps/backend/internal/github/poller_test.go index f3d23be2e4..ecd411e6ac 100644 --- a/apps/backend/internal/github/poller_test.go +++ b/apps/backend/internal/github/poller_test.go @@ -7,6 +7,7 @@ import ( "errors" "strings" "testing" + "testing/synctest" "time" "github.com/jmoiron/sqlx" @@ -547,9 +548,16 @@ type graphQLMockClient struct { branchErr error // returned for the next "Branches" call prQueries []string branchQueries []string + // onExecute, if set, is called at the top of every ExecuteGraphQL before + // canned responses are consumed. Used by the singleflight coalescing + // test to block the first in-flight call while a second one races. + onExecute func() } func (m *graphQLMockClient) ExecuteGraphQL(_ context.Context, query string, _ map[string]any, out any) error { + if m.onExecute != nil { + m.onExecute() + } if strings.Contains(query, "query Branches") { m.branchQueries = append(m.branchQueries, query) if m.branchErr != nil { @@ -692,38 +700,94 @@ func TestTryBatchedPRWatchCheck_FallsBackOnUnsupportedClient(t *testing.T) { } } -func TestFetchBatchedStatuses_NumberedQueryError_ReturnsNilFalse(t *testing.T) { - poller, _, gh, _ := setupBatchedPollerTest(t) +// TestSyncWatchesBatched_ManyWatches_TwoGraphQLCalls is the regression test +// for the OS-freeze caused by per-watch gh subprocess fan-out. A workspace +// with N numbered + M searching watches must fire exactly TWO GraphQL +// queries (one numbered, one searching), regardless of N or M — that's +// what keeps the gh subprocess burst bounded. +func TestSyncWatchesBatched_ManyWatches_TwoGraphQLCalls(t *testing.T) { + _, svc, gh, store := setupBatchedPollerTest(t) + ctx := context.Background() + + // Canned responses: one PR alias per numbered watch, one branch alias per + // searching watch. Hand-rolled minimal payloads so the assertion stays on + // "did we batch?" rather than on the field decoder. + gh.prResponses = []string{`{"data":{ + "repo0":{"pr0":{"state":"OPEN","title":"a","url":"x","headRefName":"a","baseRefName":"main","headRefOid":"1","author":{"login":"x"},"createdAt":"2026-01-01T00:00:00Z","updatedAt":"2026-01-01T00:00:00Z","reviews":{"nodes":[]},"reviewRequests":{"totalCount":0},"commits":{"nodes":[]}}}, + "repo1":{"pr0":{"state":"OPEN","title":"b","url":"x","headRefName":"b","baseRefName":"main","headRefOid":"2","author":{"login":"x"},"createdAt":"2026-01-01T00:00:00Z","updatedAt":"2026-01-01T00:00:00Z","reviews":{"nodes":[]},"reviewRequests":{"totalCount":0},"commits":{"nodes":[]}}}, + "repo2":{"pr0":{"state":"OPEN","title":"c","url":"x","headRefName":"c","baseRefName":"main","headRefOid":"3","author":{"login":"x"},"createdAt":"2026-01-01T00:00:00Z","updatedAt":"2026-01-01T00:00:00Z","reviews":{"nodes":[]},"reviewRequests":{"totalCount":0},"commits":{"nodes":[]}}} + }}`} + gh.branchResponses = []string{`{"data":{ + "b0":{"pullRequests":{"nodes":[]}}, + "b1":{"pullRequests":{"nodes":[]}}, + "b2":{"pullRequests":{"nodes":[]}}, + "b3":{"pullRequests":{"nodes":[]}} + }}`} + + var watches []*PRWatch + for i := 0; i < 3; i++ { + w := &PRWatch{SessionID: "n" + string(rune('a'+i)), TaskID: "tn" + string(rune('a'+i)), + Owner: "o", Repo: "r", PRNumber: i + 1, Branch: "n"} + if err := store.CreatePRWatch(ctx, w); err != nil { + t.Fatalf("create numbered: %v", err) + } + watches = append(watches, w) + } + for i := 0; i < 4; i++ { + w := &PRWatch{SessionID: "s" + string(rune('a'+i)), TaskID: "ts" + string(rune('a'+i)), + Owner: "o", Repo: "r", PRNumber: 0, Branch: "s" + string(rune('a'+i))} + if err := store.CreatePRWatch(ctx, w); err != nil { + t.Fatalf("create searching: %v", err) + } + watches = append(watches, w) + } + + if _, err := svc.SyncWatchesBatched(ctx, watches); err != nil { + t.Fatalf("SyncWatchesBatched: %v", err) + } + + // THE assertion: 7 watches → exactly 1 numbered query + 1 branch query. + // Pre-fix this would have been 7 separate gh subprocess invocations. + if got := len(gh.prQueries); got != 1 { + t.Errorf("numbered GraphQL calls = %d, want 1 (all numbered watches must batch)", got) + } + if got := len(gh.branchQueries); got != 1 { + t.Errorf("branch GraphQL calls = %d, want 1 (all searching watches must batch)", got) + } +} + +func TestSyncWatchesBatched_NumberedQueryError_ReturnsError(t *testing.T) { + _, svc, gh, _ := setupBatchedPollerTest(t) ctx := context.Background() gh.prErr = errors.New("graphql 500") numbered := []*PRWatch{{Owner: "o", Repo: "r", PRNumber: 1}} - got, ok := poller.fetchBatchedStatuses(ctx, gh, numbered, nil) - if ok || got != nil { - t.Errorf("expected (nil, false) on numbered query error, got (%v, %v)", got, ok) + got, err := svc.SyncWatchesBatched(ctx, numbered) + if err == nil || got != nil { + t.Errorf("expected (nil, err) on numbered query error, got (%v, %v)", got, err) } } -func TestFetchBatchedStatuses_BranchQueryError_TriggersFallback(t *testing.T) { - // Greptile P2: when only branch query fails (no numbered watches), the - // previous return of an empty non-nil map silently absorbed the failure. - // The fix returns (nil, false) so the caller falls back per-watch. - poller, _, gh, _ := setupBatchedPollerTest(t) +func TestSyncWatchesBatched_BranchQueryError_ReturnsError(t *testing.T) { + // Greptile P2 (preserved from previous fetchBatchedStatuses test): when only + // the branch query fails (no numbered watches), an empty non-nil map + // silently absorbed the failure. The fix returns a non-nil error so the + // caller falls back per-watch. + _, svc, gh, _ := setupBatchedPollerTest(t) ctx := context.Background() gh.branchErr = errors.New("graphql 500") searching := []*PRWatch{{Owner: "o", Repo: "r", PRNumber: 0, Branch: "feat"}} - got, ok := poller.fetchBatchedStatuses(ctx, gh, nil, searching) - if ok || got != nil { - t.Errorf("expected (nil, false) on branch query error, got (%v, %v)", got, ok) + got, err := svc.SyncWatchesBatched(ctx, searching) + if err == nil || got != nil { + t.Errorf("expected (nil, err) on branch query error, got (%v, %v)", got, err) } } -func TestApplyPRStatus_MergedPR_ResetsWatch(t *testing.T) { - poller, _, _, store := setupPollerTest(t) +func TestSyncWatchesBatched_MergedPR_ResetsWatch(t *testing.T) { + _, svc, gh, store := setupBatchedPollerTest(t) ctx := context.Background() - mergedAt := time.Now().UTC().Add(-time.Hour) watch := &PRWatch{ SessionID: "s1", TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 99, Branch: "feat", } @@ -738,13 +802,30 @@ func TestApplyPRStatus_MergedPR_ResetsWatch(t *testing.T) { t.Fatalf("create task PR: %v", err) } - status := &PRStatus{ - PR: &PR{ - Number: 99, State: prStateMerged, RepoOwner: "o", RepoName: "r", - HeadBranch: "feat", BaseBranch: "main", MergedAt: &mergedAt, URL: "https://x/99", Title: "Merged PR", - }, + // Canned merged-PR response — convertBatchedPRResult promotes state to + // "merged" whenever mergedAt is non-empty, which is what triggers the + // watch reset. + gh.prResponses = []string{`{ + "data": { + "repo0": { + "pr0": { + "state": "MERGED", "title": "Merged PR", "url": "https://x/99", + "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", + "headRefName": "feat", "baseRefName": "main", "headRefOid": "abc", + "author": {"login": "alice"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-02T00:00:00Z", + "mergedAt": "2026-01-02T00:00:00Z", + "reviews": {"nodes": []}, + "reviewRequests": {"totalCount": 0}, + "commits": {"nodes": [{"commit": {"statusCheckRollup": {"state": "SUCCESS"}}}]} + } + } + } + }`} + + if _, err := svc.SyncWatchesBatched(ctx, []*PRWatch{watch}); err != nil { + t.Fatalf("SyncWatchesBatched: %v", err) } - poller.applyPRStatus(ctx, watch, status) updated, err := store.GetPRWatchBySession(ctx, "s1") if err != nil || updated == nil { @@ -755,6 +836,156 @@ func TestApplyPRStatus_MergedPR_ResetsWatch(t *testing.T) { } } +// TestRefreshStaleWorkspaceWatches_CoalescesConcurrentCalls regression-tests +// the singleflight guard on per-workspace background refresh. Without it, +// the frontend's repeated WS polls (every ~5s) stack background goroutines +// that each fire a batched GraphQL request, defeating the per-process +// throttle once the cap is small enough to queue. +func TestRefreshStaleWorkspaceWatches_CoalescesConcurrentCalls(t *testing.T) { + _, svc, gh, store := setupBatchedPollerTest(t) + ctx := context.Background() + + // Seed two tasks with one searching watch each. Searching (PRNumber=0) + // means refreshStaleWorkspaceWatches drives them through the branch + // query path, which is what onExecute will block on. + for _, id := range []string{"t1", "t2"} { + w := &PRWatch{SessionID: "sess-" + id, TaskID: id, Owner: "o", Repo: "r", PRNumber: 0, Branch: "br-" + id} + if err := store.CreatePRWatch(ctx, w); err != nil { + t.Fatalf("create watch: %v", err) + } + } + + // Two canned branch responses so neither the first NOR a hypothetical + // duplicate refresh starves on missing data — we want the assertion to + // be "duplicate never ran", not "duplicate failed". + gh.branchResponses = []string{ + `{"data":{"b0":{"pullRequests":{"nodes":[]}},"b1":{"pullRequests":{"nodes":[]}}}}`, + `{"data":{"b0":{"pullRequests":{"nodes":[]}},"b1":{"pullRequests":{"nodes":[]}}}}`, + } + + started := make(chan struct{}, 4) + release := make(chan struct{}) + gh.onExecute = func() { + started <- struct{}{} + <-release + } + + staleTasks := map[string]struct{}{"t1": {}, "t2": {}} + + // Kick off the first refresh and wait until it's actually inside ExecuteGraphQL. + svc.refreshStaleWorkspaceWatches("ws1", staleTasks) + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatalf("first refresh never entered ExecuteGraphQL") + } + + // Fire a second refresh for the SAME workspace while the first is blocked. + // The singleflight guard must drop it on the floor — no new goroutine, no + // new GraphQL call. A small grace period lets a buggy unguarded goroutine + // reach ExecuteGraphQL if it were going to. + svc.refreshStaleWorkspaceWatches("ws1", staleTasks) + select { + case <-started: + t.Fatalf("second refresh entered ExecuteGraphQL while first was in flight — coalescing broken") + case <-time.After(100 * time.Millisecond): + } + + // Release the first and confirm post-completion refreshes work again. + close(release) + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if _, busy := svc.inflightWorkspaceRefreshes.Load("ws1"); !busy { + break + } + time.Sleep(5 * time.Millisecond) + } + if _, busy := svc.inflightWorkspaceRefreshes.Load("ws1"); busy { + t.Fatalf("inflight key for ws1 never cleared after refresh completion") + } + + // A fresh refresh must start (key was cleared in the defer). + release2 := make(chan struct{}) + gh.onExecute = func() { + started <- struct{}{} + <-release2 + } + svc.refreshStaleWorkspaceWatches("ws1", staleTasks) + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatalf("post-completion refresh never started — inflight key leaked") + } + close(release2) +} + +// TestServiceStop_DrainsRefreshGoroutine verifies that Service.Stop() blocks +// until an in-flight refreshStaleWorkspaceWatches goroutine completes and +// cancels its syncCtx so it stops doing work. Without this, process +// shutdown could race a goroutine that is mid-DB-write or mid-gh-spawn. +// +// Runs inside synctest so "Stop is parked on bgWG.Wait" is a structural +// guarantee (synctest.Wait returns once every bubble goroutine is durably +// blocked) rather than a wall-clock window that could yield false negatives +// on a slow CI runner where Stop hasn't entered yet. +func TestServiceStop_DrainsRefreshGoroutine(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + _, svc, gh, store := setupBatchedPollerTest(t) + ctx := context.Background() + + w := &PRWatch{SessionID: "s1", TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 0, Branch: "br-1"} + if err := store.CreatePRWatch(ctx, w); err != nil { + t.Fatalf("create watch: %v", err) + } + + gh.branchResponses = []string{`{"data":{"b0":{"pullRequests":{"nodes":[]}}}}`} + + started := make(chan struct{}, 1) + release := make(chan struct{}) + gh.onExecute = func() { + select { + case started <- struct{}{}: + default: + } + <-release + } + + svc.refreshStaleWorkspaceWatches("ws1", map[string]struct{}{"t1": {}}) + // Wait until the refresh goroutine has parked on <-release. + synctest.Wait() + select { + case <-started: + default: + t.Fatalf("refresh goroutine never entered ExecuteGraphQL") + } + + stopped := make(chan struct{}) + go func() { + svc.Stop() + close(stopped) + }() + // Stop must park on bgWG.Wait() — the refresh goroutine still + // holds the only outstanding WaitGroup counter. + synctest.Wait() + select { + case <-stopped: + t.Fatalf("Stop() returned while refresh goroutine was still blocked — drain skipped") + default: + } + + close(release) + synctest.Wait() + select { + case <-stopped: + default: + t.Fatalf("Stop() did not return after refresh goroutine released") + } + + // Second Stop is a no-op. + svc.Stop() + }) +} + func TestWaitForRateLimit_SkipsWhenHealthy(t *testing.T) { poller, _, _, _ := setupPollerTest(t) ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) diff --git a/apps/backend/internal/github/provider.go b/apps/backend/internal/github/provider.go index 7b5e653d25..b90c69739c 100644 --- a/apps/backend/internal/github/provider.go +++ b/apps/backend/internal/github/provider.go @@ -57,6 +57,10 @@ func Provide( svc.subscribeTaskEvents() cleanup := func() error { + // Stop background goroutines (currently the per-workspace + // stale-PR refreshes) before tearing down event subs so an + // in-flight refresh's DB writes don't race with shutdown. + svc.Stop() svc.unsubscribeTaskEvents() return nil } diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index 2569d1305b..8a3662a62e 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -94,10 +94,32 @@ type Service struct { // in different goroutines, and the map is shared between them. cleanupFailureMu sync.Mutex cleanupFailureCounts map[string]int + + // inflightWorkspaceRefreshes tracks workspaces whose stale-PR background + // refresh is currently running, so overlapping ListWorkspaceTaskPRs polls + // from the frontend coalesce instead of stacking goroutines (each of + // which fires its own batched GraphQL request). Keys are workspaceID; + // presence means "a refresh is in flight". sync.Map is the right shape + // here because keys are unbounded and short-lived and the hot path is a + // LoadOrStore guard, not iteration. + inflightWorkspaceRefreshes sync.Map + + // stopCtx / stopCancel / bgWG own the lifecycle of background goroutines + // the service spawns lazily (currently refreshStaleWorkspaceWatches). + // Stop() cancels stopCtx and waits for bgWG to drain, satisfying the + // "long-running goroutine has a single owner with explicit start/stop" + // convention in apps/backend/AGENTS.md. stopOnce makes Stop idempotent + // so the Provide cleanup func can be invoked from any number of + // shutdown paths. + stopCtx context.Context + stopCancel context.CancelFunc + bgWG sync.WaitGroup + stopOnce sync.Once } // NewService creates a new GitHub service. func NewService(client Client, authMethod string, secrets SecretProvider, store *Store, eventBus bus.EventBus, log *logger.Logger) *Service { + stopCtx, stopCancel := context.WithCancel(context.Background()) return &Service{ client: client, authMethod: authMethod, @@ -112,9 +134,25 @@ func NewService(client Client, authMethod string, secrets SecretProvider, store protectionCache: newBranchProtectionCache(), rateTracker: NewRateTracker(eventBus, log), cleanupFailureCounts: make(map[string]int), + stopCtx: stopCtx, + stopCancel: stopCancel, } } +// Stop cancels in-flight background goroutines (currently the +// per-workspace stale-PR refreshes spawned by refreshStaleWorkspaceWatches) +// and waits for them to drain. Idempotent: safe to call from the Provide +// cleanup func and any other shutdown path. +func (s *Service) Stop() { + if s == nil { + return + } + s.stopOnce.Do(func() { + s.stopCancel() + s.bgWG.Wait() + }) +} + // RateTracker exposes the service's rate-limit tracker so factory callers // can wire it into individual clients. func (s *Service) RateTracker() *RateTracker { diff --git a/apps/backend/internal/github/service_pr_watch.go b/apps/backend/internal/github/service_pr_watch.go index c8c6de2a48..b6c4e4e6c1 100644 --- a/apps/backend/internal/github/service_pr_watch.go +++ b/apps/backend/internal/github/service_pr_watch.go @@ -5,6 +5,7 @@ import ( "fmt" "strconv" "strings" + "sync" "time" "go.uber.org/zap" @@ -353,8 +354,7 @@ func (s *Service) ListWorkspaceTaskPRs(ctx context.Context, workspaceID string) } // Collect stale task IDs for background refresh. A task is considered stale - // if any of its PRs are stale; the sync is per-task so we only need to - // queue each task once. + // if any of its PRs are stale. staleTasks := make(map[string]struct{}) for taskID, prs := range result { for _, tp := range prs { @@ -364,32 +364,97 @@ func (s *Service) ListWorkspaceTaskPRs(ctx context.Context, workspaceID string) } } } - staleTaskIDs := make([]string, 0, len(staleTasks)) - for id := range staleTasks { - staleTaskIDs = append(staleTaskIDs, id) + if len(staleTasks) > 0 { + s.refreshStaleWorkspaceWatches(workspaceID, staleTasks) } - - // Background refresh with bounded concurrency - if len(staleTaskIDs) > 0 { - go func() { - sem := make(chan struct{}, 5) - for _, taskID := range staleTaskIDs { - sem <- struct{}{} - go func(id string) { - defer func() { <-sem }() - syncCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - if _, syncErr := s.TriggerPRSyncAll(syncCtx, id); syncErr != nil { - s.logger.Debug("background PR sync failed", zap.String("task_id", id), zap.Error(syncErr)) - } - }(taskID) - } - }() - } - return result, nil } +// refreshStaleWorkspaceWatches fans in all stale watches across the stale +// tasks and runs ONE batched GraphQL sync, so a 40-watch workspace fires +// ~2 gh subprocess calls instead of 40 (one per_task * five concurrent). +// Best-effort: errors are logged at Debug and the cached result is still +// returned to the caller. Background goroutine so the WS handler returns +// immediately. +// +// Coalesces overlapping refreshes for the same workspace: if a refresh is +// already in flight, subsequent calls drop their stale set on the floor +// and let the running goroutine finish. Without this, the frontend's 5s +// poll (or a burst of workspace events) would stack goroutines that each +// fire a batched GraphQL request, defeating the per-process throttle +// once the cap is small enough to start queueing. +func (s *Service) refreshStaleWorkspaceWatches(workspaceID string, staleTasks map[string]struct{}) { + if _, inflight := s.inflightWorkspaceRefreshes.LoadOrStore(workspaceID, struct{}{}); inflight { + return + } + // Track the goroutine on the service WaitGroup so Stop() drains it, + // and derive syncCtx from s.stopCtx so shutdown cancels the in-flight + // sync instead of letting it run past process teardown. See + // apps/backend/AGENTS.md "Goroutine ownership and leak testing". + s.bgWG.Add(1) + go func() { + defer s.bgWG.Done() + defer s.inflightWorkspaceRefreshes.Delete(workspaceID) + syncCtx, cancel := context.WithTimeout(s.stopCtx, 60*time.Second) + defer cancel() + var allWatches []*PRWatch + for taskID := range staleTasks { + watches, err := s.store.ListPRWatchesByTask(syncCtx, taskID) + if err != nil { + s.logger.Debug("list PR watches for refresh failed", + zap.String("task_id", taskID), zap.Error(err)) + continue + } + allWatches = append(allWatches, watches...) + } + if len(allWatches) == 0 { + return + } + if _, err := s.SyncWatchesBatched(syncCtx, allWatches); err != nil { + // Batched fetch failed (noop client, auth blip, GraphQL error). + // Fall back to per-task sync with bounded concurrency so we + // don't spawn one gh per watch in lockstep. + s.logger.Debug("batched workspace PR sync failed; falling back per-task", + zap.Int("watches", len(allWatches)), zap.Error(err)) + s.refreshStaleTasksPerTask(syncCtx, staleTasks) + } + }() +} + +// refreshStaleTasksPerTask is the legacy bounded-concurrency fallback for +// when SyncWatchesBatched can't be used (e.g. NoopClient). Kept small — +// the global gh subprocess semaphore in gh_throttle.go is what actually +// caps fan-out now, the worker pool here just bounds goroutine count. +// +// Blocks until every spawned goroutine returns. The caller relies on this +// to keep `inflightWorkspaceRefreshes` from being cleared while +// TriggerPRSyncAll is still writing PR-watch state — otherwise a follow-up +// refresh for the same workspace could race the still-running fallback. +func (s *Service) refreshStaleTasksPerTask(ctx context.Context, staleTasks map[string]struct{}) { + sem := make(chan struct{}, 5) + var wg sync.WaitGroup + for taskID := range staleTasks { + // Honor ctx so shutdown / parent-timeout stops queuing new tasks + // instead of waiting on a busy semaphore slot. + select { + case sem <- struct{}{}: + case <-ctx.Done(): + wg.Wait() + return + } + wg.Add(1) + go func(id string) { + defer wg.Done() + defer func() { <-sem }() + if _, syncErr := s.TriggerPRSyncAll(ctx, id); syncErr != nil { + s.logger.Debug("background PR sync failed", + zap.String("task_id", id), zap.Error(syncErr)) + } + }(taskID) + } + wg.Wait() +} + // findTaskPRForStatus locates the TaskPR row matching the (task, owner, repo, // pr_number) tuple from a poll result. Multi-repo tasks can have multiple // rows for the same task — narrowing by (owner, repo, pr_number) ensures the @@ -547,6 +612,12 @@ func (s *Service) TriggerPRSync(ctx context.Context, taskID string) (*TaskPR, er // touches the most recently updated watch and silently leaves the other // repos' PRs stale. Returns an empty slice (not nil) when the task has no // watches. +// +// When the client supports GraphQL (the production path) all of the task's +// watches are fetched in 1-2 batched gh subprocess calls. The per-watch +// fallback below is reached only for the NoopClient (auth disabled) or +// when the batched fetch itself fails; in those cases we fan out one +// subprocess per watch as before. func (s *Service) TriggerPRSyncAll(ctx context.Context, taskID string) ([]*TaskPR, error) { watches, err := s.store.ListPRWatchesByTask(ctx, taskID) if err != nil { @@ -562,6 +633,21 @@ func (s *Service) TriggerPRSyncAll(ctx context.Context, taskID string) ([]*TaskP } return existing, nil } + if _, batchErr := s.SyncWatchesBatched(ctx, watches); batchErr != nil { + s.logger.Debug("batched PR sync failed; falling back to per-watch", + zap.String("task_id", taskID), zap.Error(batchErr)) + return s.triggerPRSyncAllPerWatch(ctx, taskID, watches) + } + // Batched path applied all DB updates inline; reload so the WS caller + // sees the freshest TaskPR rows. + return s.store.ListTaskPRsByTask(ctx, taskID) +} + +// triggerPRSyncAllPerWatch is the legacy fan-out path: one gh subprocess +// per watch. Kept as a fallback for the NoopClient and for the rare case +// where the batched GraphQL call fails (auth glitch, network blip) so a +// single bad cycle doesn't leave the UI staring at stale data. +func (s *Service) triggerPRSyncAllPerWatch(ctx context.Context, taskID string, watches []*PRWatch) ([]*TaskPR, error) { results := make([]*TaskPR, 0, len(watches)) for _, w := range watches { var tp *TaskPR diff --git a/apps/backend/internal/github/service_pr_watch_batched.go b/apps/backend/internal/github/service_pr_watch_batched.go new file mode 100644 index 0000000000..d3b099134f --- /dev/null +++ b/apps/backend/internal/github/service_pr_watch_batched.go @@ -0,0 +1,155 @@ +package github + +import ( + "context" + "fmt" + "time" + + "go.uber.org/zap" +) + +// PRWatchSyncResult is the per-watch outcome from SyncWatchesBatched. +// Callers (poller, on-demand sync) can post-process — e.g. publish +// PRFeedback events — without re-fetching from GitHub. +type PRWatchSyncResult struct { + Watch *PRWatch + Status *PRStatus // nil when no PR was found for a searching watch, or alias missing + Found bool // true when status applied (PR data exists) + Changed bool // numbered watches only: true if checks/review state moved + SyncFailed bool // true when SyncTaskPR returned an error — callers must NOT publish events +} + +// SyncWatchesBatched runs the batched GraphQL queries for the supplied +// watches and applies the resulting DB updates: timestamps, task PR sync, +// watch PR-number promotion on detection, watch reset on merge/close. +// Returns per-watch results so callers can post-process (event publishing). +// +// Returns an error when the batched fetch itself fails — the caller should +// fall back to per-watch checks rather than silently dropping a poll cycle. +// Errors from the per-watch DB applies are logged but do not abort the +// loop, matching the per-watch path's best-effort semantics. +// +// This is the single seam both the 1-minute poller and the on-demand +// TriggerPRSyncAll / ListWorkspaceTaskPRs background refresh share, so a +// 40-watch workspace fans out to ~2 gh subprocess calls instead of 40. +func (s *Service) SyncWatchesBatched(ctx context.Context, watches []*PRWatch) ([]PRWatchSyncResult, error) { + if len(watches) == 0 { + return nil, nil + } + exec, err := graphQLExecutorFor(s.client) + if err != nil { + return nil, err + } + numbered, searching := splitPRWatches(watches) + statusByKey, err := s.fetchBatchedWatchStatuses(ctx, exec, numbered, searching) + if err != nil { + return nil, err + } + + results := make([]PRWatchSyncResult, 0, len(watches)) + now := time.Now().UTC() + for _, w := range numbered { + results = append(results, s.applyBatchedNumberedWatch(ctx, w, statusByKey, now)) + } + for _, w := range searching { + results = append(results, s.applyBatchedSearchingWatch(ctx, w, statusByKey, now)) + } + return results, nil +} + +// fetchBatchedWatchStatuses runs the numbered- and branch-keyed GraphQL +// queries and merges their results. Returns an error when either query +// fails so the caller can fall back to per-watch checks. +func (s *Service) fetchBatchedWatchStatuses( + ctx context.Context, exec GraphQLExecutor, numbered, searching []*PRWatch, +) (map[string]*PRStatus, error) { + combined := make(map[string]*PRStatus, len(numbered)+len(searching)) + if len(numbered) > 0 { + refs := make([]graphQLPRRef, 0, len(numbered)) + for _, w := range numbered { + refs = append(refs, graphQLPRRef{Owner: w.Owner, Repo: w.Repo, Number: w.PRNumber}) + } + out, err := runBatchedPRQuery(ctx, exec, refs) + if err != nil { + return nil, fmt.Errorf("batched PR query: %w", err) + } + for k, v := range out { + combined[k] = v + } + } + if len(searching) > 0 { + refs := make([]graphQLBranchRef, 0, len(searching)) + for _, w := range searching { + refs = append(refs, graphQLBranchRef{Owner: w.Owner, Repo: w.Repo, Branch: w.Branch}) + } + out, err := runBatchedBranchQuery(ctx, exec, refs) + if err != nil { + return nil, fmt.Errorf("batched branch query: %w", err) + } + for k, v := range out { + combined[k] = v + } + } + return combined, nil +} + +// applyBatchedNumberedWatch mirrors Poller.applyPRStatus on the service +// side so on-demand callers reuse the same DB-write sequence. +func (s *Service) applyBatchedNumberedWatch( + ctx context.Context, w *PRWatch, statusByKey map[string]*PRStatus, now time.Time, +) PRWatchSyncResult { + status, ok := statusByKey[prStatusCacheKey(w.Owner, w.Repo, w.PRNumber)] + if !ok || status == nil { + // Alias missing — best-effort liveness bump so we don't immediately re-probe. + _ = s.store.UpdatePRWatchTimestamps(ctx, w.ID, now, nil, "", "") + return PRWatchSyncResult{Watch: w} + } + changed := status.ChecksState != w.LastCheckStatus || status.ReviewState != w.LastReviewState + if err := s.store.UpdatePRWatchTimestamps(ctx, w.ID, now, nil, status.ChecksState, status.ReviewState); err != nil { + s.logger.Error("failed to update PR watch timestamps", zap.String("id", w.ID), zap.Error(err)) + } + if syncErr := s.SyncTaskPR(ctx, w.TaskID, status); syncErr != nil { + s.logger.Error("failed to sync task PR", zap.String("task_id", w.TaskID), zap.Error(syncErr)) + // SyncFailed=true so poller skips publishing PR feedback while the + // task_pr row is still stale — old applyPRStatus path early-returned + // on this error for the same reason. + return PRWatchSyncResult{Watch: w, Status: status, Found: true, SyncFailed: true} + } + // Reset to "searching" when the PR is merged/closed so a follow-up PR on the + // same branch can be detected without manual intervention. + if status.PR != nil && (status.PR.State == prStateMerged || status.PR.State == prStateClosed) { + if resetErr := s.store.UpdatePRWatchPRNumber(ctx, w.ID, 0); resetErr != nil { + s.logger.Error("failed to reset completed PR watch", zap.String("id", w.ID), zap.Error(resetErr)) + } + } + return PRWatchSyncResult{Watch: w, Status: status, Found: true, Changed: changed} +} + +// applyBatchedSearchingWatch mirrors Poller.applyDetectedPR on the service +// side. A searching watch (pr_number=0) is promoted to a known PR when the +// branch lookup returns one; otherwise we just bump last_checked_at. +func (s *Service) applyBatchedSearchingWatch( + ctx context.Context, w *PRWatch, statusByKey map[string]*PRStatus, now time.Time, +) PRWatchSyncResult { + // Timestamps get bumped on both the "no PR found" and "PR detected" + // paths, so hoist the single call above the branch to make that + // invariant obvious. + _ = s.store.UpdatePRWatchTimestamps(ctx, w.ID, now, nil, "", "") + status, ok := statusByKey[graphqlBranchKey(w.Owner, w.Repo, w.Branch)] + if !ok || status == nil || status.PR == nil { + return PRWatchSyncResult{Watch: w} + } + if err := s.store.UpdatePRWatchPRNumber(ctx, w.ID, status.PR.Number); err != nil { + s.logger.Error("failed to update PR watch with detected PR", + zap.String("watch_id", w.ID), zap.Int("pr_number", status.PR.Number), zap.Error(err)) + return PRWatchSyncResult{Watch: w, Status: status, Found: true} + } + if _, err := s.AssociatePRWithTask(ctx, w.TaskID, w.RepositoryID, status.PR); err != nil { + s.logger.Error("failed to associate detected PR with task", + zap.String("task_id", w.TaskID), zap.Int("pr_number", status.PR.Number), zap.Error(err)) + return PRWatchSyncResult{Watch: w, Status: status, Found: true} + } + s.logger.Info("detected PR for session branch (batched)", + zap.String("watch_id", w.ID), zap.String("branch", w.Branch), zap.Int("pr_number", status.PR.Number)) + return PRWatchSyncResult{Watch: w, Status: status, Found: true} +} diff --git a/apps/backend/internal/improvekandev/github.go b/apps/backend/internal/improvekandev/github.go index 4947666e1a..a07c55a1f5 100644 --- a/apps/backend/internal/improvekandev/github.go +++ b/apps/backend/internal/improvekandev/github.go @@ -7,6 +7,8 @@ import ( "os/exec" "strings" "time" + + "github.com/kandev/kandev/internal/common/subproc" ) // GitHubInfo exposes the authenticated user's login, write-access status, @@ -77,7 +79,7 @@ func runGH(ctx context.Context, args ...string) (string, error) { var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { + if err := subproc.RunGH(ctx, cmd); err != nil { return stdout.String(), fmt.Errorf("gh %s: %w: %s", args[0], err, stderr.String()) } return stdout.String(), nil diff --git a/apps/backend/internal/orchestrator/executor/executor_credentials.go b/apps/backend/internal/orchestrator/executor/executor_credentials.go index c26e354970..e5872c4e30 100644 --- a/apps/backend/internal/orchestrator/executor/executor_credentials.go +++ b/apps/backend/internal/orchestrator/executor/executor_credentials.go @@ -9,6 +9,8 @@ import ( "time" "go.uber.org/zap" + + "github.com/kandev/kandev/internal/common/subproc" ) const ( @@ -217,11 +219,23 @@ func (e *Executor) resolveGHCLIToken(req *LaunchAgentRequest, metadata map[strin } // detectGHToken runs `gh auth token` to extract the GitHub token from the local gh CLI. +// +// Throttle Acquire (30s budget) runs first; the 5s exec ctx is constructed +// AFTER the slot is held so its deadline starts from when gh actually +// begins running. Building it earlier would let throttle queue time eat +// into the exec budget and silently skip GITHUB_TOKEN injection. func detectGHToken() (string, error) { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() + acquireCtx, cancelAcquire := context.WithTimeout(context.Background(), 30*time.Second) + defer cancelAcquire() + release, err := subproc.GH().Acquire(acquireCtx) + if err != nil { + return "", fmt.Errorf("gh throttle acquire: %w", err) + } + defer release() + execCtx, cancelExec := context.WithTimeout(context.Background(), 5*time.Second) + defer cancelExec() - cmd := exec.CommandContext(ctx, "gh", "auth", "token") + cmd := exec.CommandContext(execCtx, "gh", "auth", "token") out, err := cmd.Output() if err != nil { return "", err diff --git a/apps/backend/internal/repoclone/clone.go b/apps/backend/internal/repoclone/clone.go index 2c8fef5770..a29637eb01 100644 --- a/apps/backend/internal/repoclone/clone.go +++ b/apps/backend/internal/repoclone/clone.go @@ -13,6 +13,7 @@ import ( "go.uber.org/zap" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/subproc" ) // ghCredentialHelper is the git credential helper command that delegates to gh CLI. @@ -126,7 +127,7 @@ func (c *Cloner) gitCmd(ctx context.Context, args ...string) *exec.Cmd { func (c *Cloner) fetch(ctx context.Context, repoPath string) { c.logger.Debug("repository already cloned, fetching", zap.String("path", repoPath)) cmd := c.gitCmd(ctx, "-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags) - if out, err := cmd.CombinedOutput(); err != nil { + if out, err := subproc.RunGitCombinedOutput(ctx, cmd); err != nil { c.logger.Warn("git fetch failed (non-fatal)", zap.String("path", repoPath), zap.String("output", string(out)), @@ -157,7 +158,7 @@ func (c *Cloner) clone(ctx context.Context, cloneURL, targetPath, owner, name st // Fallback: git clone with credential helper. cmd := c.gitCmd(ctx, "clone", "--filter=blob:none", gitNoTags, cloneURL, targetPath) - if out, err := cmd.CombinedOutput(); err != nil { + if out, err := subproc.RunGitCombinedOutput(ctx, cmd); err != nil { return fmt.Errorf("git clone failed: %s: %w", string(out), err) } return nil @@ -169,7 +170,7 @@ func (c *Cloner) ghClone(ctx context.Context, owner, name, targetPath string) er nwo := owner + "/" + name cmd := exec.CommandContext(ctx, "gh", "repo", "clone", nwo, targetPath, "--", "--filter=blob:none", gitNoTags) cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") - out, err := cmd.CombinedOutput() + out, err := subproc.RunGHCombinedOutput(ctx, cmd) if err != nil { c.logger.Debug("gh repo clone failed, falling back to git clone", zap.String("repo", nwo), diff --git a/apps/backend/internal/repoclone/protocol.go b/apps/backend/internal/repoclone/protocol.go index 9cceeec130..59e645014f 100644 --- a/apps/backend/internal/repoclone/protocol.go +++ b/apps/backend/internal/repoclone/protocol.go @@ -6,6 +6,8 @@ import ( "os/exec" "strings" "time" + + "github.com/kandev/kandev/internal/common/subproc" ) // ProtocolSSH is the SSH git protocol. @@ -20,7 +22,7 @@ const ProtocolHTTPS = "https" func DetectGitProtocol() string { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - out, err := exec.CommandContext(ctx, "gh", "config", "get", "git_protocol").Output() + out, err := subproc.RunGHOutput(ctx, exec.CommandContext(ctx, "gh", "config", "get", "git_protocol")) if err == nil { if strings.TrimSpace(string(out)) == ProtocolHTTPS { return ProtocolHTTPS diff --git a/apps/backend/internal/worktree/git_throttle.go b/apps/backend/internal/worktree/git_throttle.go new file mode 100644 index 0000000000..ef147e9ecd --- /dev/null +++ b/apps/backend/internal/worktree/git_throttle.go @@ -0,0 +1,44 @@ +package worktree + +import ( + "context" + "os/exec" + + "github.com/kandev/kandev/internal/common/subproc" +) + +// Why this throttle exists: +// +// Worktree creation, cleanup, and submodule init all shell out to git in +// quick succession (worktree add, branch -D, worktree prune, submodule +// update --init --recursive, ...). When multiple agents start at once the +// number of concurrent git execs can climb past 30, and on managed +// corporate macOS hosts (CrowdStrike Falcon + syspolicyd) every fork/exec +// is intercepted, serialized, and validated. The resulting backlog has +// been observed alongside the gh-poller storm — see internal/common/ +// subproc for the full rationale. This file just provides short +// package-local wrappers so callers don't have to reach into subproc +// for every git exec. + +// runGitCmd acquires a git slot, runs cmd, and releases. Use this +// 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) +} + +// runGitCmdCombinedOutput is runGitCmd's CombinedOutput sibling. +func runGitCmdCombinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + return subproc.RunGitCombinedOutput(ctx, 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) +} diff --git a/apps/backend/internal/worktree/git_throttle_test.go b/apps/backend/internal/worktree/git_throttle_test.go new file mode 100644 index 0000000000..fd8f16897f --- /dev/null +++ b/apps/backend/internal/worktree/git_throttle_test.go @@ -0,0 +1,77 @@ +package worktree + +import ( + "context" + "errors" + "os/exec" + "sync" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/subproc" +) + +// TestRunGitCmd_SaturatedThrottleBlocks verifies that runGitCmd routes +// through the package-wide gitThrottle: once the cap is saturated, the +// next call must block until a holder releases. This is the host-freeze +// mitigation in action — without the throttle, lifecycle bursts +// (worktree add + branch -D + worktree prune + submodule update) for +// several agents starting in parallel could spawn 20+ simultaneous git +// processes, saturating the macOS code-signing fork queue. +// +// Uses gitThrottle.Acquire directly to saturate the pool (no real +// subprocess needed for the saturation half), then `sh -c true` to +// confirm runGitCmd itself goes through the same gate. +func TestRunGitCmd_SaturatedThrottleBlocks(t *testing.T) { + const cap = 2 + restore := setGitThrottleCapForTest(cap) + 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 + // 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()) + if err != nil { + t.Fatalf("pre-saturate acquire %d: %v", i, err) + } + defer release() + } + + // A runGitCmd call with a short-deadline ctx must surface the + // throttle's ctx.Err — proving it tried to acquire and got blocked + // rather than exec'ing the subprocess and racing past the throttle. + cctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + err := runGitCmd(cctx, exec.Command("sh", "-c", "true")) + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("expected DeadlineExceeded once cap=%d saturated, got %v", cap, 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 +// would acquire and immediately error out without running the command. +func TestRunGitCmd_RunsToCompletionWhenCapacityAvailable(t *testing.T) { + restore := setGitThrottleCapForTest(4) + defer restore() + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if err := runGitCmd(context.Background(), exec.Command("sh", "-c", "true")); err != nil { + t.Errorf("runGitCmd unexpected err: %v", err) + } + }() + } + wg.Wait() +} + +// Cap parsing tests live in internal/common/subproc/shared_test.go now +// that the env var, default, and resolver are owned by that package. diff --git a/apps/backend/internal/worktree/gitcrypt.go b/apps/backend/internal/worktree/gitcrypt.go index 53aa37d1e5..7194f55388 100644 --- a/apps/backend/internal/worktree/gitcrypt.go +++ b/apps/backend/internal/worktree/gitcrypt.go @@ -120,7 +120,7 @@ func (m *Manager) unlockGitCryptAndCheckout(ctx context.Context, worktreePath st checkoutCmd := exec.CommandContext(ctx, "git", checkoutArgs...) checkoutCmd.Dir = worktreePath - if output, err := checkoutCmd.CombinedOutput(); err != nil { + if output, err := runGitCmdCombinedOutput(ctx, checkoutCmd); err != nil { m.logger.Error("git checkout failed after git-crypt setup", zap.String("worktree_path", worktreePath), zap.String("output", string(output)), @@ -138,7 +138,7 @@ func (m *Manager) unlockGitCryptAndCheckout(ctx context.Context, worktreePath st for _, sp := range submodulePaths { resetCmd := exec.CommandContext(ctx, "git", "reset", "HEAD", "--", sp) resetCmd.Dir = worktreePath - if output, err := resetCmd.CombinedOutput(); err != nil { + if output, err := runGitCmdCombinedOutput(ctx, resetCmd); err != nil { m.logger.Debug("failed to reset submodule index entry", zap.String("path", sp), zap.String("output", string(output)), @@ -164,7 +164,7 @@ func (m *Manager) unlockGitCryptAndCheckout(ctx context.Context, worktreePath st func resolveGitDir(ctx context.Context, worktreePath, flag string) (string, error) { cmd := exec.CommandContext(ctx, "git", "rev-parse", flag) cmd.Dir = worktreePath - out, err := cmd.Output() + out, err := runGitCmdOutput(ctx, cmd) if err != nil { return "", fmt.Errorf("git rev-parse %s: %w", flag, err) } @@ -206,7 +206,7 @@ func isGitCryptUnlocked(gitCryptDir string) bool { func enableWorktreeConfig(ctx context.Context, worktreePath string) error { cmd := exec.CommandContext(ctx, "git", "config", "extensions.worktreeConfig", "true") cmd.Dir = worktreePath - if out, err := cmd.CombinedOutput(); err != nil { + if out, err := runGitCmdCombinedOutput(ctx, cmd); err != nil { return fmt.Errorf("git config extensions.worktreeConfig: %s: %w", string(out), err) } return nil @@ -228,7 +228,7 @@ func configureGitCryptFilters(ctx context.Context, worktreePath string) error { for _, kv := range configs { cmd := exec.CommandContext(ctx, "git", "config", "--worktree", kv[0], kv[1]) cmd.Dir = worktreePath - if out, err := cmd.CombinedOutput(); err != nil { + if out, err := runGitCmdCombinedOutput(ctx, cmd); err != nil { return fmt.Errorf("git config %s: %s: %w", kv[0], string(out), err) } } @@ -262,7 +262,7 @@ func disableGitCryptFilters(ctx context.Context, worktreePath string) error { for _, kv := range configs { cmd := exec.CommandContext(ctx, "git", "config", kv[0], kv[1]) cmd.Dir = worktreePath - if out, err := cmd.CombinedOutput(); err != nil { + if out, err := runGitCmdCombinedOutput(ctx, cmd); err != nil { return fmt.Errorf("git config %s: %s: %w", kv[0], string(out), err) } } diff --git a/apps/backend/internal/worktree/manager.go b/apps/backend/internal/worktree/manager.go index cdb9115760..a825d6ea0d 100644 --- a/apps/backend/internal/worktree/manager.go +++ b/apps/backend/internal/worktree/manager.go @@ -13,9 +13,16 @@ import ( ) const ( - defaultGitFetchTimeout = 90 * time.Second - defaultGitPullTimeout = 60 * time.Second - defaultGitInspectTimeout = 10 * time.Second + defaultGitFetchTimeout = 90 * time.Second + defaultGitPullTimeout = 60 * time.Second + // defaultGitInspectTimeout bounds cheap local git ref-inspection + // commands (rev-parse --verify, rev-parse --abbrev-ref HEAD). + // Normally <100ms, but on CrowdStrike-instrumented macOS every + // fork+exec is intercepted by syspolicyd so a single spawn can take + // 1–3s under load. 30s gives ~100x headroom over normal operation + // while still surfacing true hangs (credential prompts, stuck + // filters, filesystem stalls) in a reasonable window. + defaultGitInspectTimeout = 30 * time.Second gitNoTags = "--no-tags" ) diff --git a/apps/backend/internal/worktree/manager_cleanup.go b/apps/backend/internal/worktree/manager_cleanup.go index 6413a2c766..1e4a864a60 100644 --- a/apps/backend/internal/worktree/manager_cleanup.go +++ b/apps/backend/internal/worktree/manager_cleanup.go @@ -48,7 +48,7 @@ func (m *Manager) removeWorktree(ctx context.Context, wt *Worktree, removeBranch cmd := exec.CommandContext(ctx, "git", "branch", "-D", wt.Branch) cmd.Dir = wt.RepositoryPath - if output, err := cmd.CombinedOutput(); err != nil { + if output, err := runGitCmdCombinedOutput(ctx, cmd); err != nil { m.logger.Warn("failed to delete branch from main repository", zap.String("branch", wt.Branch), zap.String("repository_path", wt.RepositoryPath), @@ -228,7 +228,7 @@ func (m *Manager) removeWorktreeDir(ctx context.Context, worktreePath, repoPath // First try git worktree remove cmd := exec.CommandContext(ctx, "git", "worktree", "remove", "--force", worktreePath) cmd.Dir = repoPath - if output, err := cmd.CombinedOutput(); err != nil { + if output, err := runGitCmdCombinedOutput(ctx, cmd); err != nil { m.logger.Debug("git worktree remove failed, falling back to rm", zap.String("output", string(output)), zap.Error(err)) @@ -240,7 +240,7 @@ func (m *Manager) removeWorktreeDir(ctx context.Context, worktreePath, repoPath // Prune stale worktree entries pruneCmd := exec.CommandContext(ctx, "git", "worktree", "prune") pruneCmd.Dir = repoPath - if err := pruneCmd.Run(); err != nil { + if err := runGitCmd(ctx, pruneCmd); err != nil { m.logger.Debug("git worktree prune failed", zap.Error(err)) } } diff --git a/apps/backend/internal/worktree/manager_git.go b/apps/backend/internal/worktree/manager_git.go index c17569d889..0f90cb76a5 100644 --- a/apps/backend/internal/worktree/manager_git.go +++ b/apps/backend/internal/worktree/manager_git.go @@ -11,6 +11,8 @@ import ( "time" "go.uber.org/zap" + + "github.com/kandev/kandev/internal/common/subproc" ) // isGitRepo checks if a path is a Git repository. @@ -36,6 +38,22 @@ func (m *Manager) isGitRepo(path string) bool { // "missing branch" from a "could not tell" and avoid surfacing a // misleading ErrInvalidBaseBranch. func (m *Manager) branchExists(ctx context.Context, repoPath, branch string) (bool, error) { + // Acquire the throttle slot FIRST, then start the inspectTimeout + // timer. Building inspectCtx before Acquire (as we did originally) + // let throttle queue time eat through the 10s budget under load, + // producing 70s-lock-held / signal:killed cascades under git-pool + // 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) + if err != nil { + m.logger.Warn("branchExists bounded by context", + zap.String("repository_path", repoPath), + zap.String("branch", branch), + zap.Error(err)) + return false, fmt.Errorf("branch check timed out for %q before throttle acquire: %w", branch, err) + } + defer release() inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout) defer cancel() cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "rev-parse", "--verify", branch) @@ -74,11 +92,17 @@ func (m *Manager) checkoutBranchExistsAnywhere(ctx context.Context, repoPath, br } func (m *Manager) currentBranch(ctx context.Context, repoPath string) string { + // Same Acquire-then-build-execCtx ordering as branchExists. + release, err := subproc.Git().Acquire(ctx) + if err != nil { + return "" + } + defer release() inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout) defer cancel() cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "rev-parse", "--abbrev-ref", "HEAD") - output, err := cmd.Output() - if err != nil { + output, runErr := cmd.Output() + if runErr != nil { if ctxErr := inspectCtx.Err(); ctxErr != nil { m.logger.Warn("currentBranch bounded by context", zap.String("repository_path", repoPath), @@ -137,17 +161,18 @@ func (m *Manager) pullBaseBranch(ctx context.Context, repoPath, baseBranch strin Output: fmt.Sprintf("Fetching latest changes for %s", baseBranch), }) - // Fetch branch from origin in non-interactive mode. - fetchCtx, cancelFetch := context.WithTimeout(ctx, m.fetchTimeout) - defer cancelFetch() - + // Acquire the git throttle slot first, then start the fetch timer. + // Order matters: the previous "build fetchCtx, then runGitCmd" shape + // let throttle queue time burn the fetch budget while we waited for a + // slot, and the cmd was killed with `signal: killed` the moment it + // got one (70s-lock-held trace under contention). fetchArgs := []string{"fetch", gitNoTags, "origin"} if localBranch != "" { fetchArgs = append(fetchArgs, localBranch) } - fetchCmd := m.newNonInteractiveGitCmd(fetchCtx, repoPath, fetchArgs...) - if output, err := fetchCmd.CombinedOutput(); err != nil { - return m.handleFetchFallback(baseBranch, stepName, onProgress, fetchCtx.Err(), output, err) + output, err, execCtxErr := m.runGitCombinedAfterAcquire(ctx, m.fetchTimeout, repoPath, fetchArgs...) + if err != nil { + return m.handleFetchFallback(baseBranch, stepName, onProgress, execCtxErr, output, err) } if isRemoteRef { @@ -208,12 +233,10 @@ func (m *Manager) pullCurrentBranchOrFallback( ctx context.Context, repoPath, baseBranch, remoteRef, stepName string, onProgress SyncProgressCallback, ) string { - pullCtx, cancelPull := context.WithTimeout(ctx, m.pullTimeout) - defer cancelPull() - - pullCmd := m.newNonInteractiveGitCmd(pullCtx, repoPath, "pull", "--ff-only", "origin", baseBranch) - if output, err := pullCmd.CombinedOutput(); err != nil { - reason := classifyGitFallbackReason(err, string(output), pullCtx.Err()) + // Same Acquire-then-build-execCtx ordering as the fetch path. + output, err, execCtxErr := m.runGitCombinedAfterAcquire(ctx, m.pullTimeout, repoPath, "pull", "--ff-only", "origin", baseBranch) + if err != nil { + reason := classifyGitFallbackReason(err, string(output), execCtxErr) m.logger.Warn("git pull failed before worktree creation; continuing with remote ref", zap.String("branch", baseBranch), zap.String("reason", reason), @@ -226,3 +249,27 @@ func (m *Manager) pullCurrentBranchOrFallback( m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Synced and using %s", baseBranch), "") return baseBranch } + +// runGitCombinedAfterAcquire acquires the backend git throttle slot, +// then constructs a child context with execTimeout and runs the +// non-interactive git command with CombinedOutput. The exec timer +// starts only AFTER Acquire returns so throttle queue time cannot +// burn the budget (otherwise the cmd gets killed with `signal: killed` +// the moment it acquires a slot under contention). +// Returns (combined output, run error, exec-ctx error). The exec-ctx +// error lets callers tell a context-driven kill (timeout) from a +// regular git failure when classifying fallbacks. +func (m *Manager) runGitCombinedAfterAcquire( + ctx context.Context, execTimeout time.Duration, repoPath string, args ...string, +) ([]byte, error, error) { + release, err := subproc.Git().Acquire(ctx) + if err != nil { + return nil, err, ctx.Err() + } + defer release() + execCtx, cancel := context.WithTimeout(ctx, execTimeout) + defer cancel() + cmd := m.newNonInteractiveGitCmd(execCtx, repoPath, args...) + out, runErr := cmd.CombinedOutput() + return out, runErr, execCtx.Err() +} diff --git a/apps/backend/internal/worktree/manager_lifecycle.go b/apps/backend/internal/worktree/manager_lifecycle.go index 3e8610622f..6a6d47df33 100644 --- a/apps/backend/internal/worktree/manager_lifecycle.go +++ b/apps/backend/internal/worktree/manager_lifecycle.go @@ -341,11 +341,11 @@ func (m *Manager) setUpstreamIfExists(ctx context.Context, worktreePath, localBr // Verify the remote-tracking ref exists. Use the non-interactive helper so // this cannot hang on a credential prompt while Create holds repoLock. verifyCmd := m.newNonInteractiveGitCmd(ctx, worktreePath, "rev-parse", "--verify", upstream) - if err := verifyCmd.Run(); err != nil { + if err := runGitCmd(ctx, verifyCmd); err != nil { return } cmd := m.newNonInteractiveGitCmd(ctx, worktreePath, "branch", "--set-upstream-to="+upstream, localBranch) - if out, err := cmd.CombinedOutput(); err != nil { + if out, err := runGitCmdCombinedOutput(ctx, cmd); err != nil { m.logger.Debug("failed to set upstream (non-fatal)", zap.String("branch", localBranch), zap.String("upstream", upstream), @@ -377,16 +377,18 @@ func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch strin zap.Int("pr_number", prNumber), zap.String("repo_path", repoPath)) - // Try to fetch from origin to get the latest version. - fetchCtx, cancelFetch := context.WithTimeout(ctx, m.fetchTimeout) - defer cancelFetch() - + // Acquire the throttle slot FIRST, then build fetchCtx with the + // full m.fetchTimeout budget. The previous shape (build fetchCtx, + // then call runGitCmdCombinedOutput which Acquires internally) let + // throttle queue time burn the fetch budget — same failure mode as + // the manager_git.go probes fixed in PR #1216 (70s lock-held trace, + // signal:killed). With this ordering the budget only counts actual + // git execution time. refspec := branch + ":" + branch if prNumber > 0 { refspec = fmt.Sprintf("pull/%d/head:%s", prNumber, branch) } - fetchCmd := m.newNonInteractiveGitCmd(fetchCtx, repoPath, "fetch", gitNoTags, "origin", refspec) - output, err := fetchCmd.CombinedOutput() + output, err, fetchCtxErr := m.runGitCombinedAfterAcquire(ctx, m.fetchTimeout, repoPath, "fetch", gitNoTags, "origin", refspec) if err == nil { return &FetchBranchResult{}, nil } @@ -396,7 +398,7 @@ func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch strin // the local ref. Retry by fetching only the remote-tracking ref (origin/branch), // which is always safe regardless of worktree state. if isFetchRefusedCheckedOut(outputStr) { - if result := m.retryFetchAsRemoteTrackingRef(fetchCtx, repoPath, branch, prNumber); result != nil { + if result := m.retryFetchAsRemoteTrackingRef(ctx, repoPath, branch, prNumber); result != nil { return result, nil } } @@ -415,7 +417,7 @@ func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch strin return nil, fmt.Errorf("branch %q not found locally or on remote: %s", branch, outputStr) } - reason := classifyGitFallbackReason(err, outputStr, fetchCtx.Err()) + reason := classifyGitFallbackReason(err, outputStr, fetchCtxErr) warning := fmt.Sprintf("Could not fetch latest from origin (%s). Using local version of branch %q which may be outdated.", reason, branch) m.logger.Info("using local branch (fetch failed)", zap.String("branch", branch), @@ -431,13 +433,16 @@ func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch strin // fails when that branch is already checked out in another worktree. Returns // nil if the retry also failed and the caller should fall back to the local // branch path. -func (m *Manager) retryFetchAsRemoteTrackingRef(fetchCtx context.Context, repoPath, branch string, prNumber int) *FetchBranchResult { +// +// Uses ctx (not the original fetchCtx) and a fresh m.fetchTimeout budget via +// runGitCombinedAfterAcquire — the parent's fetchCtx is already consumed by +// the first attempt, so reusing it would leave no room for the retry. +func (m *Manager) retryFetchAsRemoteTrackingRef(ctx context.Context, repoPath, branch string, prNumber int) *FetchBranchResult { retryRef := branch if prNumber > 0 { retryRef = fmt.Sprintf("pull/%d/head", prNumber) } - retryCmd := m.newNonInteractiveGitCmd(fetchCtx, repoPath, "fetch", gitNoTags, "origin", retryRef) - if _, retryErr := retryCmd.CombinedOutput(); retryErr != nil { + if _, retryErr, _ := m.runGitCombinedAfterAcquire(ctx, m.fetchTimeout, repoPath, "fetch", gitNoTags, "origin", retryRef); retryErr != nil { return nil } m.logger.Info("fetched via remote-tracking ref (branch checked out elsewhere)", @@ -470,7 +475,7 @@ func (m *Manager) gitAddWorktreeExisting(ctx context.Context, repoPath, branchNa cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = repoPath - output, err := cmd.CombinedOutput() + output, err := runGitCmdCombinedOutput(ctx, cmd) if err == nil { if usesGitCrypt { if unlockErr := m.unlockGitCryptAndCheckout(ctx, worktreePath); unlockErr != nil { @@ -520,7 +525,7 @@ func (m *Manager) retryWorktreeExisting(ctx context.Context, repoPath, branchNam retryCmd := exec.CommandContext(ctx, "git", args...) retryCmd.Dir = repoPath - retryOutput, retryErr := retryCmd.CombinedOutput() + retryOutput, retryErr := runGitCmdCombinedOutput(ctx, retryCmd) if retryErr != nil { retryOutStr := string(retryOutput) if isBranchCheckedOutError(retryOutStr) { @@ -551,7 +556,7 @@ func (m *Manager) gitAddWorktreeExistingWithGitCrypt(ctx context.Context, repoPa cmd := exec.CommandContext(ctx, "git", "worktree", "add", "--no-checkout", worktreePath, branchName) cmd.Dir = repoPath - output, err := cmd.CombinedOutput() + output, err := runGitCmdCombinedOutput(ctx, cmd) if err != nil { outStr := string(output) if isBranchCheckedOutError(outStr) { @@ -595,7 +600,7 @@ func (m *Manager) tryRecoverCheckedOutBranch(ctx context.Context, repoPath, bran pruneCmd := exec.CommandContext(ctx, "git", "worktree", "prune") pruneCmd.Dir = repoPath - if output, err := pruneCmd.CombinedOutput(); err != nil { + if output, err := runGitCmdCombinedOutput(ctx, pruneCmd); err != nil { m.logger.Error("git worktree prune failed", zap.String("output", string(output)), zap.Error(err)) @@ -657,7 +662,7 @@ func (m *Manager) gitAddWorktree(ctx context.Context, repoPath, branchName, work cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = repoPath - output, err := cmd.CombinedOutput() + output, err := runGitCmdCombinedOutput(ctx, cmd) if err != nil { outStr := string(output) // Check if this is a git-crypt error we didn't anticipate @@ -703,7 +708,7 @@ func (m *Manager) gitAddWorktreeWithGitCrypt(ctx context.Context, repoPath, bran worktreePath, baseRef) cmd.Dir = repoPath - output, err := cmd.CombinedOutput() + output, err := runGitCmdCombinedOutput(ctx, cmd) if err != nil { m.logger.Error("git worktree add (--no-checkout) failed", zap.String("output", string(output)), @@ -734,7 +739,7 @@ func (m *Manager) gitAddWorktreeForRecreate(ctx context.Context, repoPath, branc cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = repoPath - output, err := cmd.CombinedOutput() + output, err := runGitCmdCombinedOutput(ctx, cmd) if err == nil { return usesGitCrypt, nil } @@ -744,7 +749,7 @@ func (m *Manager) gitAddWorktreeForRecreate(ctx context.Context, repoPath, branc zap.String("output", outStr)) retryCmd := exec.CommandContext(ctx, "git", "worktree", "add", "--no-checkout", worktreePath, branch) retryCmd.Dir = repoPath - if retryOutput, retryErr := retryCmd.CombinedOutput(); retryErr != nil { + if retryOutput, retryErr := runGitCmdCombinedOutput(ctx, retryCmd); retryErr != nil { m.logger.Error("failed to recreate worktree (--no-checkout)", zap.String("output", string(retryOutput)), zap.Error(retryErr)) @@ -810,7 +815,7 @@ func (m *Manager) recreate(ctx context.Context, existing *Worktree, req CreateRe // Remove from git worktree list cmd := exec.CommandContext(ctx, "git", "worktree", "prune") cmd.Dir = req.RepositoryPath - if err := cmd.Run(); err != nil { + if err := runGitCmd(ctx, cmd); err != nil { m.logger.Debug("git worktree prune failed", zap.Error(err)) } diff --git a/apps/backend/internal/worktree/submodule.go b/apps/backend/internal/worktree/submodule.go index 273525fb96..3dae88b85c 100644 --- a/apps/backend/internal/worktree/submodule.go +++ b/apps/backend/internal/worktree/submodule.go @@ -14,7 +14,7 @@ import ( func getSubmodulePaths(ctx context.Context, dir string) ([]string, error) { cmd := exec.CommandContext(ctx, "git", "ls-tree", "-r", "HEAD") cmd.Dir = dir - output, err := cmd.Output() + output, err := runGitCmdOutput(ctx, cmd) if err != nil { return nil, err } @@ -41,7 +41,7 @@ func getSubmodulePaths(ctx context.Context, dir string) ([]string, error) { func (m *Manager) initSubmodules(ctx context.Context, dir string) { cmd := exec.CommandContext(ctx, "git", "submodule", "update", "--init", "--recursive") cmd.Dir = dir - output, err := cmd.CombinedOutput() + output, err := runGitCmdCombinedOutput(ctx, cmd) if err != nil { m.logger.Warn("git submodule update --init failed (non-fatal)", zap.String("dir", dir),