[fix] Close goroutine leak in update hint + add exec cobra-level test seam (#301)

* [fix] Close goroutine leak in update hint and add exec cobra-level test seam (#287)

Finding 1: checkUpdateHint previously called u.Check(context.Background()), so
the inner HTTP goroutine kept running for up to the updater's 30s client timeout
after the outer 2s window elapsed. Fix: accept ctx context.Context as the first
param, derive context.WithTimeout(ctx, timeout) for the request, and pass tctx to
u.Check so the HTTP round-trip is bounded by both the caller's context and the
timeout. Also reset rootCmd.ctx in TestExecute's cleanup to prevent the
post-Execute cancelled signal-context from leaking into subsequent tests.

Finding 2: execCmd.RunE had no injectable constructor, so a refactor breaking
the flag-parse→filter→executor wiring could only be caught by the e2e suite.
Extract runExec(cmd, args, r git.Runner, execFn execRunner) and add buildExecCmd
mirroring the buildTrustCmd pattern (trust.go:49). Both production RunE and the
new constructor delegate to runExec, so tests exercise the real wiring. Add four
cobra-level tests: missing-target-flag, executor-invoked, JSON-output,
no-matching-worktrees.

* [fix] Address review feedback: cancel relay goroutine, unified exec flag registration, t.Fatal

- Add defer cancel() to relay goroutine in checkUpdateHint so the context
  timer is released promptly on all exit paths, not just the happy path
- Extract addExecFlags() helper shared by init() and buildExecCmd() so
  both flag-registration sites stay in sync and can't silently diverge
- Change t.Error to t.Fatal in cancelled-context HTTP handler assertion

* [fix] Change t.Error to t.Fatal in TestCheckUpdateHintDevVersion handler

Author: lugassawan

cmd/exec.go

@@ -33,6 +33,9 @@ const (
 	hintConcurrency = "Limit the number of parallel executions"
 )
 
+// execRunner is the injectable executor function type, matching executor.Run.
+type execRunner func(context.Context, executor.Config) []executor.Result
+
 var execCmd = &cobra.Command{
 	Use:   "exec <command>",
 	Short: "Run a shell command across worktrees",
@@ -41,48 +44,78 @@ var execCmd = &cobra.Command{
   rimba exec --type bugfix "npm test"`,
 	Args: cobra.ExactArgs(1),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		opts := execReadFlags(cmd)
-		if err := execValidateFlags(opts); err != nil {
-			return err
-		}
+		return runExec(cmd, args, newRunner(), executor.Run)
+	},
+}
 
-		execShowHints(cmd)
+// runExec is the shared implementation for the exec command, accepting
+// injected runner and executor so both production and tests use the same path.
+func runExec(cmd *cobra.Command, args []string, r git.Runner, execFn execRunner) error {
+	opts := execReadFlags(cmd)
+	if err := execValidateFlags(opts); err != nil {
+		return err
+	}
 
-		r := newRunner()
-		s := spinner.New(spinnerOpts(cmd))
-		defer s.Stop()
-		s.Start("Collecting worktrees...")
+	execShowHints(cmd)
 
-		prefixes := resolver.AllPrefixes()
-		filtered, err := execSelectWorktrees(cmd, r, s, opts, prefixes)
-		if err != nil {
-			return err
-		}
+	s := spinner.New(spinnerOpts(cmd))
+	defer s.Stop()
+	s.Start("Collecting worktrees...")
 
-		if len(filtered) == 0 {
-			s.Stop()
-			fmt.Fprintln(cmd.OutOrStdout(), "No worktrees match the given filters.")
-			return nil
-		}
+	prefixes := resolver.AllPrefixes()
+	filtered, err := execSelectWorktrees(cmd, r, s, opts, prefixes)
+	if err != nil {
+		return err
+	}
+
+	if len(filtered) == 0 {
+		s.Stop()
+		fmt.Fprintln(cmd.OutOrStdout(), "No worktrees match the given filters.")
+		return nil
+	}
 
-		targets := execBuildTargets(filtered, prefixes)
-		s.Update(fmt.Sprintf("Running in %d worktree(s)...", len(targets)))
+	targets := execBuildTargets(filtered, prefixes)
+	s.Update(fmt.Sprintf("Running in %d worktree(s)...", len(targets)))
 
-		results := executor.Run(cmd.Context(), executor.Config{
-			Targets:     targets,
-			Command:     args[0],
-			Concurrency: opts.concurrency,
-			FailFast:    opts.failFast,
-			Runner:      executor.ShellRunner(),
-		})
+	results := execFn(cmd.Context(), executor.Config{
+		Targets:     targets,
+		Command:     args[0],
+		Concurrency: opts.concurrency,
+		FailFast:    opts.failFast,
+		Runner:      executor.ShellRunner(),
+	})
 
-		s.Stop()
+	s.Stop()
 
-		if isJSON(cmd) {
-			return execRenderJSON(cmd, args[0], results)
-		}
-		return execRenderText(cmd, results, prefixes)
-	},
+	if isJSON(cmd) {
+		return execRenderJSON(cmd, args[0], results)
+	}
+	return execRenderText(cmd, results, prefixes)
+}
+
+// addExecFlags registers exec-specific flags on c, shared by execCmd and buildExecCmd.
+func addExecFlags(c *cobra.Command) {
+	c.Flags().Bool(flagAll, false, "run in all eligible worktrees")
+	c.Flags().String(flagType, "", "filter by prefix type (e.g. feature, bugfix)")
+	c.Flags().Bool(flagDirty, false, "run only in dirty worktrees")
+	c.Flags().Bool(flagFailFast, false, "stop after the first failure")
+	c.Flags().Int(flagConcurrency, 0, "max parallel executions (0 = unlimited)")
+}
+
+// buildExecCmd constructs a testable exec command with injected deps.
+// Used by exec_test.go to exercise the full flag-parse → filter → executor pipeline.
+func buildExecCmd(r git.Runner, execFn execRunner) *cobra.Command {
+	c := &cobra.Command{
+		Use:  "exec <command>",
+		Args: cobra.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return runExec(cmd, args, r, execFn)
+		},
+	}
+	addExecFlags(c)
+	c.Flags().Bool(flagJSON, false, "")
+	c.Flags().Bool(flagNoColor, false, "")
+	return c
 }
 
 // execOpts holds parsed exec flags.
@@ -221,14 +254,8 @@ func execRenderText(cmd *cobra.Command, results []executor.Result, prefixes []st
 }
 
 func init() {
-	execCmd.Flags().Bool(flagAll, false, "run in all eligible worktrees")
-	execCmd.Flags().String(flagType, "", "filter by prefix type (e.g. feature, bugfix)")
-	execCmd.Flags().Bool(flagDirty, false, "run only in dirty worktrees")
-	execCmd.Flags().Bool(flagFailFast, false, "stop after the first failure")
-	execCmd.Flags().Int(flagConcurrency, 0, "max parallel executions (0 = unlimited)")
-
+	addExecFlags(execCmd)
 	_ = execCmd.RegisterFlagCompletionFunc(flagType, typeFilterCompletion())
-
 	rootCmd.AddCommand(execCmd)
 }
 

cmd/exec_test.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/lugassawan/rimba/internal/config"
 	"github.com/lugassawan/rimba/internal/executor"
+	"github.com/lugassawan/rimba/internal/git"
 	"github.com/lugassawan/rimba/internal/output"
 	"github.com/lugassawan/rimba/internal/resolver"
 	"github.com/lugassawan/rimba/internal/spinner"
@@ -550,3 +551,150 @@ func TestExecShowHintsJSONEarlyReturn(t *testing.T) {
 		t.Errorf("expected no output in JSON mode, got %q", buf.String())
 	}
 }
+
+// newExecCmd builds a testable exec command with injected runner and executor.
+func newExecCmd(r git.Runner, execFn execRunner) (*cobra.Command, *bytes.Buffer) {
+	buf := &bytes.Buffer{}
+	cfg := &config.Config{DefaultSource: "main"}
+	c := buildExecCmd(r, execFn)
+	c.SetOut(buf)
+	c.SetErr(buf)
+	c.SetContext(config.WithConfig(context.Background(), cfg))
+	return c, buf
+}
+
+func TestExecCmdMissingTargetFlag(t *testing.T) {
+	r := &mockRunner{
+		run:      func(_ ...string) (string, error) { return "", nil },
+		runInDir: noopRunInDir,
+	}
+	cmd, _ := newExecCmd(r, func(_ context.Context, _ executor.Config) []executor.Result { return nil })
+	cmd.SetArgs([]string{"echo hi"})
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("expected error when neither --all nor --type is set")
+	}
+	if !strings.Contains(err.Error(), "provide --all or --type") {
+		t.Errorf("error = %q, want 'provide --all or --type'", err.Error())
+	}
+}
+
+func TestExecCmdInvokesExecutor(t *testing.T) {
+	porcelain := strings.Join([]string{
+		"worktree /repo",
+		"HEAD abc",
+		"branch refs/heads/main",
+		"",
+		"worktree /wt/foo",
+		"HEAD def",
+		"branch refs/heads/feature/foo",
+		"",
+	}, "\n")
+	r := &mockRunner{
+		run:      func(_ ...string) (string, error) { return porcelain, nil },
+		runInDir: noopRunInDir,
+	}
+
+	var capturedCfg executor.Config
+	fakeExec := func(_ context.Context, cfg executor.Config) []executor.Result {
+		capturedCfg = cfg
+		return []executor.Result{
+			{Target: executor.Target{Branch: "feature/foo", Task: "foo", Path: "/wt/foo"}, ExitCode: 0, Stdout: []byte("ok\n")},
+		}
+	}
+
+	cmd, buf := newExecCmd(r, fakeExec)
+	cmd.SetArgs([]string{"--all", "--no-color", "echo ok"})
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if capturedCfg.Command != "echo ok" {
+		t.Errorf("command = %q, want %q", capturedCfg.Command, "echo ok")
+	}
+	if len(capturedCfg.Targets) == 0 {
+		t.Error("expected at least one target")
+	}
+	if !strings.Contains(buf.String(), "foo") {
+		t.Errorf("output missing task name: %q", buf.String())
+	}
+}
+
+func TestExecCmdJSONOutput(t *testing.T) {
+	porcelain := strings.Join([]string{
+		"worktree /repo",
+		"HEAD abc",
+		"branch refs/heads/main",
+		"",
+		"worktree /wt/foo",
+		"HEAD def",
+		"branch refs/heads/feature/foo",
+		"",
+	}, "\n")
+	r := &mockRunner{
+		run:      func(_ ...string) (string, error) { return porcelain, nil },
+		runInDir: noopRunInDir,
+	}
+	fakeExec := func(_ context.Context, _ executor.Config) []executor.Result {
+		return []executor.Result{
+			{Target: executor.Target{Branch: "feature/foo", Task: "foo", Path: "/wt/foo"}, ExitCode: 0},
+		}
+	}
+
+	cmd, buf := newExecCmd(r, fakeExec)
+	cmd.SetArgs([]string{"--all", "--json", "echo hi"})
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	var env output.Envelope
+	if err := json.Unmarshal(buf.Bytes(), &env); err != nil {
+		t.Fatalf("invalid JSON: %v\noutput: %s", err, buf.String())
+	}
+	if env.Command != "exec" {
+		t.Errorf("envelope command = %q, want %q", env.Command, "exec")
+	}
+	dataMap, ok := env.Data.(map[string]any)
+	if !ok {
+		t.Fatalf("data type = %T, want map[string]any", env.Data)
+	}
+	resultsArr, ok := dataMap["results"].([]any)
+	if !ok {
+		t.Fatalf("results type = %T, want []any", dataMap["results"])
+	}
+	if len(resultsArr) != 1 {
+		t.Errorf("results length = %d, want 1", len(resultsArr))
+	}
+}
+
+func TestExecCmdNoMatchingWorktrees(t *testing.T) {
+	// Only main worktree — no non-main worktrees to match.
+	porcelain := strings.Join([]string{
+		"worktree /repo",
+		"HEAD abc",
+		"branch refs/heads/main",
+		"",
+	}, "\n")
+	r := &mockRunner{
+		run:      func(_ ...string) (string, error) { return porcelain, nil },
+		runInDir: noopRunInDir,
+	}
+	called := false
+	fakeExec := func(_ context.Context, _ executor.Config) []executor.Result {
+		called = true
+		return nil
+	}
+
+	cmd, buf := newExecCmd(r, fakeExec)
+	cmd.SetArgs([]string{"--all", "echo hi"})
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if called {
+		t.Error("executor should not be called when no worktrees match")
+	}
+	if !strings.Contains(buf.String(), "No worktrees match") {
+		t.Errorf("expected 'No worktrees match' in output, got: %q", buf.String())
+	}
+}

cmd/root.go

@@ -110,7 +110,13 @@ func init() {
 	rootCmd.SetHelpFunc(func(cmd *cobra.Command, args []string) {
 		var hint <-chan *updater.CheckResult
 		if cmd == rootCmd {
-			hint = checkUpdateHint(version, 2*time.Second)
+			ctx := cmd.Context()
+			if ctx == nil {
+				// cmd.Context() is nil when HelpFunc is called outside of
+				// ExecuteContext (e.g., directly in tests).
+				ctx = context.Background()
+			}
+			hint = checkUpdateHint(ctx, version, 2*time.Second)
 			printBanner(cmd)
 		}
 		originalHelp(cmd, args)

cmd/root_test.go

@@ -218,6 +218,10 @@ func TestExecute(t *testing.T) {
 	t.Cleanup(func() {
 		rootCmd.SetOut(nil)
 		rootCmd.SetErr(nil)
+		// Execute() sets rootCmd.ctx to a signal-notified context that is
+		// cancelled when Execute returns (via defer stop()). Reset it so
+		// subsequent tests that call cmd.Context() get a live context.
+		rootCmd.SetContext(context.Background())
 	})
 
 	if err := Execute(); err != nil {

cmd/update_hint.go

@@ -14,40 +14,47 @@ import (
 // Tests can override this to inject a mock server.
 var newUpdater func(string) *updater.Updater = updater.New
 
-// checkUpdateHint runs a background version check with a timeout.
+// checkUpdateHint runs a background version check bounded by timeout.
 // Returns nil if the version is dev, the check fails, times out, or is up to date.
-func checkUpdateHint(version string, timeout time.Duration) <-chan *updater.CheckResult {
+// The check is cancelled when ctx is done or timeout elapses — whichever comes first.
+func checkUpdateHint(ctx context.Context, version string, timeout time.Duration) <-chan *updater.CheckResult {
 	ch := make(chan *updater.CheckResult, 1)
 
 	if updater.IsDevVersion(version) {
 		close(ch)
 		return ch
 	}
 
+	// Derive a timeout-scoped child so the HTTP request is actually cancelled
+	// when timeout elapses, not just when the caller stops waiting.
+	tctx, cancel := context.WithTimeout(ctx, timeout)
+
 	// Read newUpdater before spawning the goroutine: goroutine launch
 	// establishes a happens-before edge, preventing a data race with
 	// test overrides that restore the variable via t.Cleanup.
 	u := newUpdater(version)
 	go func() {
-		result, err := u.Check(context.Background())
+		defer cancel()
+		result, err := u.Check(tctx)
 		if err != nil || result.UpToDate {
 			close(ch)
 			return
 		}
 		ch <- result
 	}()
 
-	// Return a channel that resolves with the result or nil after timeout
+	// Return a channel that resolves with the result or nil when tctx expires.
 	out := make(chan *updater.CheckResult, 1)
 	go func() {
+		defer cancel()
 		select {
 		case r, ok := <-ch:
 			if ok {
 				out <- r
 			} else {
 				close(out)
 			}
-		case <-time.After(timeout):
+		case <-tctx.Done():
 			close(out)
 		}
 	}()