Skip to content

Commit 6d663a2

Browse files
authored
[fix] Bound git status queries with a per-item timeout (#288) (#302)
* [refactor] Thread context.Context through parallel.Collect (#288) Extend the Collect signature to accept a ctx and fn(ctx, i) so callers can enforce per-item deadlines without reaching into captured variables. All 7 production callers and the test suite updated; the two long-running sites (deps install, mcp sync) pass ctx through without a deadline. * [fix] Add git.WithItemTimeout to bound per-item git queries (#288) Introduces a 10s per-item deadline helper that callers can apply to each fan-out slot so a stalled worktree (e.g. NFS mount) cannot occupy a parallel.Collect worker slot indefinitely. * [fix] Add Unknown sentinel to WorktreeStatus for timed-out git queries (#288) A worktree whose git queries time out (or fail due to a stalled NFS mount) previously rendered as clean (✓) — indistinguishable from a healthy worktree. Now CollectWorktreeStatus sets Unknown=true on any error, FormatStatus returns "unknown", and colorStatus renders it gray. AheadBehind is updated to propagate context.Canceled/DeadlineExceeded instead of silently swallowing it. * [fix] Make fsutil.DirSize cancellable via context (#288) DirSize now accepts a context.Context and runs the WalkDir in a goroutine so a stalled syscall (NFS mount) cannot block past the deadline. Callers in status_dashboard.go updated to thread ctx through; all existing tests updated for the new signature, plus a cancellation path test. * [fix] Apply per-item 10s timeout to all git status fan-out queries (#288) Five parallel.Collect sites that query git per worktree now derive a bounded context via git.WithItemTimeout before any git call. A stalled worktree (NFS mount, hung git process) times out in ≤10s and renders as "unknown" instead of hanging the fan-out indefinitely. The two long-running sites (deps install, mcp sync) pass ctx through unchanged — they must not get a 10s cap. * [fix] Set context.Background() on test cobra commands (#288) git.WithItemTimeout panics when passed a nil parent context. Test commands created by newTestCmd() had no context set, so cmd.Context() returned nil. * [fix] Address review feedback: clarify queryPRInfo ctx + test MCP unknown (#288) Add comment explaining that queryPRInfo receives outer ctx (not itemCtx) because it is a gh network call with its own prQueryTimeout, not a git query. Add TestStatusToolUnknownWorktree to verify that a stalled NFS mount (all runInDir calls failing) results in Unknown=true in the MCP status response and is not miscounted as Dirty or Behind. * [chore] Simplify and fix code comments from review feedback - dirsize.go: clarify goroutine continues until OS returns (not cancelled) - branch.go: condense AheadBehind context-propagation note to one line - timeout.go: shorten itemQueryTimeout and WithItemTimeout comments - status.go: shorten CollectWorktreeStatus doc to one line - list_worktrees.go: condense queryPRInfo ctx comment to one line * [fix] Address review feedback: ctx-aware semaphore, errors.Is, mcp filterDirty timeout - parallel: make semaphore acquire ctx-aware (select on sem/ctx.Done) so goroutines waiting on a full semaphore exit on cancellation rather than blocking forever; add TestCollectCancelledContextDoesNotHang - git/branch: switch AheadBehind ctx check to errors.Is(err, Deadline/Canceled) so wrapped context errors are correctly propagated - mcp/tool_exec: migrate filterDirty to parallel.Collect + git.WithItemTimeout, closing the sixth unguarded parallel git fan-out site (#288) * [fix] Address follow-up feedback: Unknown pass-through + Collect doc contract - FilterDetailsByStatus: keep Unknown-status rows regardless of --dirty/--behind filter flags so stalled worktrees are never silently excluded from list output; add TestFilterDetailsByStatusUnknownPassthrough - parallel.Collect: document that goroutines cancelled at the semaphore leave the zero value of T at their result index * [fix] Use ctx.Err() for timeout detection in AheadBehind (#288) errors.Is(err, context.DeadlineExceeded/Canceled) never fires when exec.CommandContext kills the process: CombinedOutput returns *exec.ExitError (signal killed), not the context sentinel. Check ctx.Err() directly instead. Also fix TestDirSizeCancelledContext (racy select assertion) and add omitempty to Dirty/Ahead/Behind for consistent WorktreeStatus JSON schema.
1 parent fa2e69b commit 6d663a2

23 files changed

Lines changed: 350 additions & 80 deletions

cmd/colors.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@ func typeColor(t string) termcolor.Color {
3030

3131
// colorStatus applies colors to each component of a formatted status string.
3232
func colorStatus(p *termcolor.Painter, s resolver.WorktreeStatus) string {
33+
if s.Unknown {
34+
return p.Paint("unknown", termcolor.Gray)
35+
}
36+
3337
formatted := resolver.FormatStatus(s)
3438

3539
if !s.Dirty && s.Ahead == 0 && s.Behind == 0 {

cmd/colors_test.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,11 @@ func TestColorStatus(t *testing.T) {
6565
status: resolver.WorktreeStatus{Dirty: true, Ahead: 2, Behind: 1},
6666
want: "[dirty] ↑2 ↓1",
6767
},
68+
{
69+
name: "unknown",
70+
status: resolver.WorktreeStatus{Unknown: true},
71+
want: "unknown",
72+
},
6873
}
6974

7075
for _, tt := range tests {

cmd/exec.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,9 +266,11 @@ func filterDirtyWorktrees(ctx context.Context, cmd *cobra.Command, r git.Runner,
266266
n := len(worktrees)
267267
var done atomic.Int32
268268

269-
results := parallel.Collect[dirtyResult](n, 8, func(i int) dirtyResult {
269+
results := parallel.Collect[dirtyResult](ctx, n, 8, func(ctx context.Context, i int) dirtyResult {
270+
itemCtx, cancel := git.WithItemTimeout(ctx)
271+
defer cancel()
270272
path := worktrees[i].Path
271-
dirty, err := git.IsDirty(ctx, r, path)
273+
dirty, err := git.IsDirty(itemCtx, r, path)
272274
count := done.Add(1)
273275
s.Update(fmt.Sprintf("Checking dirty status... (%d/%d)", count, n))
274276
if err != nil {

cmd/log.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,12 +125,14 @@ func init() {
125125
func collectLogEntries(ctx context.Context, r git.Runner, candidates []git.WorktreeEntry, s *spinner.Spinner) []logEntry {
126126
prefixes := resolver.AllPrefixes()
127127
s.Update("Collecting commit info...")
128-
results := parallel.Collect(len(candidates), 8, func(i int) logEntry {
128+
results := parallel.Collect(ctx, len(candidates), 8, func(ctx context.Context, i int) logEntry {
129+
itemCtx, cancel := git.WithItemTimeout(ctx)
130+
defer cancel()
129131
e := candidates[i]
130132
svc, task, matchedPrefix := resolver.ServiceFromBranch(e.Branch, prefixes)
131133
typeName := strings.TrimSuffix(matchedPrefix, "/")
132134

133-
ct, subject, err := git.LastCommitInfo(ctx, r, e.Branch)
135+
ct, subject, err := git.LastCommitInfo(itemCtx, r, e.Branch)
134136
if err != nil {
135137
return logEntry{branch: e.Branch, task: task, service: svc, typeName: typeName, path: e.Path}
136138
}

cmd/mock_runner_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ func newTestCmd() (*cobra.Command, *bytes.Buffer) {
108108
cmd.Flags().Bool(flagJSON, false, "")
109109
cmd.SetOut(buf)
110110
cmd.SetErr(buf)
111+
cmd.SetContext(context.Background())
111112
return cmd, buf
112113
}
113114

internal/deps/manager.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ func (m *Manager) install(ctx context.Context, worktreePath, sourceWT string, mo
105105
var done atomic.Int32
106106
total := len(hashed)
107107

108-
results = parallel.Collect(total, concurrency, func(i int) InstallResult {
108+
// No per-item timeout here — dependency installation is long-running by design.
109+
results = parallel.Collect(ctx, total, concurrency, func(ctx context.Context, i int) InstallResult {
109110
res := m.installModule(ctx, worktreePath, hashed[i], existingPaths)
110111
completed := done.Add(1)
111112
progress.Notifyf(onProgress, "%d/%d complete", completed, total)

internal/fsutil/dirsize.go

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,37 @@
22
package fsutil
33

44
import (
5+
"context"
56
"io/fs"
67
"path/filepath"
78
)
89

10+
type dirSizeResult struct {
11+
size int64
12+
err error
13+
}
14+
915
// DirSize returns the total size of regular files under path.
10-
// Symlinks are not followed. On partial failure (permission denied,
11-
// races) it returns the best-effort total and the first error seen.
12-
func DirSize(path string) (int64, error) {
16+
// Symlinks are not followed; partial failures return a best-effort total.
17+
//
18+
// Returns (0, ctx.Err()) immediately on cancellation. The WalkDir goroutine
19+
// continues until the OS returns — it cannot be interrupted mid-syscall.
20+
func DirSize(ctx context.Context, path string) (int64, error) {
21+
ch := make(chan dirSizeResult, 1)
22+
go func() {
23+
size, err := walkDirSize(path)
24+
ch <- dirSizeResult{size: size, err: err}
25+
}()
26+
27+
select {
28+
case <-ctx.Done():
29+
return 0, ctx.Err()
30+
case r := <-ch:
31+
return r.size, r.err
32+
}
33+
}
34+
35+
func walkDirSize(path string) (int64, error) {
1336
var total int64
1437
var firstErr error
1538

internal/fsutil/dirsize_test.go

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package fsutil_test
22

33
import (
4+
"context"
45
"os"
56
"path/filepath"
67
"runtime"
@@ -16,7 +17,7 @@ func TestDirSizeSumsRegularFiles(t *testing.T) {
1617
writeFile(t, filepath.Join(root, "nested", "b.txt"), 250)
1718
writeFile(t, filepath.Join(root, "nested", "deeper", "c.txt"), 50)
1819

19-
got, err := fsutil.DirSize(root)
20+
got, err := fsutil.DirSize(context.Background(), root)
2021
if err != nil {
2122
t.Fatalf("DirSize returned error: %v", err)
2223
}
@@ -39,7 +40,7 @@ func TestDirSizeDoesNotFollowSymlinks(t *testing.T) {
3940
t.Fatalf("symlink: %v", err)
4041
}
4142

42-
got, err := fsutil.DirSize(root)
43+
got, err := fsutil.DirSize(context.Background(), root)
4344
if err != nil {
4445
t.Fatalf("DirSize returned error: %v", err)
4546
}
@@ -53,7 +54,7 @@ func TestDirSizeDoesNotFollowSymlinks(t *testing.T) {
5354

5455
func TestDirSizeEmptyDir(t *testing.T) {
5556
root := t.TempDir()
56-
got, err := fsutil.DirSize(root)
57+
got, err := fsutil.DirSize(context.Background(), root)
5758
if err != nil {
5859
t.Fatalf("DirSize returned error: %v", err)
5960
}
@@ -63,7 +64,7 @@ func TestDirSizeEmptyDir(t *testing.T) {
6364
}
6465

6566
func TestDirSizeMissingPath(t *testing.T) {
66-
_, err := fsutil.DirSize(filepath.Join(t.TempDir(), "does-not-exist"))
67+
_, err := fsutil.DirSize(context.Background(), filepath.Join(t.TempDir(), "does-not-exist"))
6768
if err == nil {
6869
t.Fatal("DirSize on missing path returned nil error, want non-nil")
6970
}
@@ -90,7 +91,7 @@ func TestDirSizeBestEffortOnPartialError(t *testing.T) {
9091
}
9192
t.Cleanup(func() { _ = os.Chmod(denied, 0o755) })
9293

93-
got, err := fsutil.DirSize(root)
94+
got, err := fsutil.DirSize(context.Background(), root)
9495
if err == nil {
9596
t.Fatal("DirSize returned nil error despite permission-denied subdir")
9697
}
@@ -102,6 +103,21 @@ func TestDirSizeBestEffortOnPartialError(t *testing.T) {
102103
}
103104
}
104105

106+
func TestDirSizeCancelledContext(t *testing.T) {
107+
root := t.TempDir()
108+
writeFile(t, filepath.Join(root, "a.txt"), 100)
109+
110+
ctx, cancel := context.WithCancel(context.Background())
111+
cancel() // pre-cancel
112+
113+
// When both ctx.Done() and the walk goroutine are ready simultaneously,
114+
// Go's select picks randomly — assert err != nil rather than a specific sentinel.
115+
_, err := fsutil.DirSize(ctx, root)
116+
if err == nil {
117+
t.Error("expected non-nil error on pre-cancelled context, got nil")
118+
}
119+
}
120+
105121
func writeFile(t *testing.T, path string, size int) {
106122
t.Helper()
107123
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {

internal/git/branch.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,17 @@ func IsDirty(ctx context.Context, r Runner, dir string) (bool, error) {
6969

7070
// AheadBehind returns the ahead/behind counts of the current branch vs its upstream.
7171
// Returns (0, 0, nil) if there's no upstream configured.
72+
// Returns ctx.Err() on context cancellation so callers can distinguish a
73+
// timed-out query from a branch with no upstream.
7274
func AheadBehind(ctx context.Context, r Runner, dir string) (ahead, behind int, _ error) {
7375
out, err := r.RunInDir(ctx, dir, "rev-list", "--left-right", "--count", "@{upstream}...HEAD")
7476
if err != nil {
75-
// No upstream or other error — treat as 0/0
77+
// exec.CommandContext kills via SIGKILL; CombinedOutput returns *exec.ExitError,
78+
// not the context sentinel — check ctx.Err() directly.
79+
if ctx.Err() != nil {
80+
return 0, 0, ctx.Err()
81+
}
82+
// No upstream configured or other non-fatal error — treat as 0/0.
7683
return 0, 0, nil //nolint:nilerr // intentional: missing upstream is not an error
7784
}
7885

internal/git/timeout.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
package git
2+
3+
import (
4+
"context"
5+
"time"
6+
)
7+
8+
// itemQueryTimeout bounds a single git query; prevents a stalled worktree from blocking the fan-out.
9+
const itemQueryTimeout = 10 * time.Second
10+
11+
// WithItemTimeout returns a child context bounded by itemQueryTimeout. Caller must defer cancel().
12+
func WithItemTimeout(ctx context.Context) (context.Context, context.CancelFunc) {
13+
return context.WithTimeout(ctx, itemQueryTimeout)
14+
}

0 commit comments

Comments
 (0)