fix: recover stalled and wedged multi-runs - #230

internal/core/run/executor/runner_stall_test.go (1)

213-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap the flat top-level tests in Should… subtests.

Several new tests here (e.g. TestJobRunnerStallRetryIsIndependentOfMaxRetries, ...SecondStallParksWithPopulatedPayload, ...StallRetryRunsFromACleanWorktree, ...ZeroStallBudgetParksOnFirstStall, ...StalledJobDoesNotAffectSiblings) run assertions directly at the top level rather than inside a t.Run("Should…") subtest, unlike the ones that already do. Please align them with the convention. As per path instructions: "MUST use t.Run("Should...") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/executor/runner_stall_test.go` at line 213, The new runner
stall tests are asserting at the top level instead of following the required
Should-prefixed subtest convention. Update the affected test functions in
runner_stall_test.go, including TestJobRunnerStallRetryIsIndependentOfMaxRetries
and the related stall retry cases, to wrap their assertions inside
t.Run("Should...", ...) subtests so they match the existing pattern used by the
other tests and the path requirement.

Source: Path instructions

internal/core/run/executor/parked_alert_test.go (1)

38-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the t.Run("Should...") subtest pattern.

The table cases here are named "nil config is silent", "sound disabled...", etc., and TestNotifyParkedAlertIsANoOpWhenSoundIsDisabled / TestNotifyParkedAlertOnNilExecutionContextDoesNotPanic are flat. Rename case names to the Should… form (and wrap the flat tests in a Should… subtest) for consistency with the repo test convention. As per path instructions: "MUST use t.Run("Should...") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/executor/parked_alert_test.go` around lines 38 - 110,
Update the parked alert tests to follow the repo’s required t.Run("Should...")
convention. In TestSoundConfigForGatesOnTheSoundFeatureFlag, rename each table
case to a Should... subtest name, and in
TestNotifyParkedAlertIsANoOpWhenSoundIsDisabled and
TestNotifyParkedAlertOnNilExecutionContextDoesNotPanic wrap the assertions in a
single t.Run("Should...") subtest. Keep the existing behavior and assertions
intact while making the names consistent with the test pattern used around
soundConfigFor and notifyParkedAlert.

Source: Path instructions

internal/core/workspace/config_test.go (1)

487-561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap standalone stall tests in t.Run("Should...") subtests.

TestLoadConfigParsesStallSection and TestLoadConfigMergesStallWorkspaceOverGlobalConfig don't use the t.Run("Should...") pattern. As per path instructions, all test cases must use this pattern.

♻️ Proposed refactor for `TestLoadConfigParsesStallSection`

 func TestLoadConfigParsesStallSection(t *testing.T) {
-	isolateWorkspaceConfigHome(t)
-	root := t.TempDir()
-	writeWorkspaceConfig(t, root, `
+	t.Run("Should parse stall section fields and sound.on_parked", func(t *testing.T) {
+		isolateWorkspaceConfigHome(t)
+		root := t.TempDir()
+		writeWorkspaceConfig(t, root, `
 	[defaults.stall]
 	enabled = false
 	timeout = "2m"
 	child_timeout = "8m"
 	terminal_command_timeout = "30m"
 	retries = 2

 	[sound]
 	on_parked = "ping"
 	`)
-
-	cfg, _, err := LoadConfig(context.Background(), root)
-	if err != nil {
-		t.Fatalf("load config: %v", err)
-	}
-	stall := cfg.Defaults.Stall
-	if stall.Enabled == nil || *stall.Enabled {
-		t.Fatalf("unexpected stall.enabled: %#v", stall.Enabled)
-	}
-	if stall.Timeout == nil || *stall.Timeout != "2m" {
-		t.Fatalf("unexpected stall.timeout: %#v", stall.Timeout)
-	}
-	if stall.ChildTimeout == nil || *stall.ChildTimeout != "8m" {
-		t.Fatalf("unexpected stall.child_timeout: %#v", stall.ChildTimeout)
-	}
-	if stall.TerminalCommandTimeout == nil || *stall.TerminalCommandTimeout != "30m" {
-		t.Fatalf("unexpected stall.terminal_command_timeout: %#v", stall.TerminalCommandTimeout)
-	}
-	if stall.Retries == nil || *stall.Retries != 2 {
-		t.Fatalf("unexpected stall.retries: %#v", stall.Retries)
-	}
-	if cfg.Sound.OnParked == nil || *cfg.Sound.OnParked != "ping" {
-		t.Fatalf("unexpected sound.on_parked: %#v", cfg.Sound.OnParked)
-	}
+
+		cfg, _, err := LoadConfig(context.Background(), root)
+		if err != nil {
+			t.Fatalf("load config: %v", err)
+		}
+		stall := cfg.Defaults.Stall
+		if stall.Enabled == nil || *stall.Enabled {
+			t.Fatalf("unexpected stall.enabled: %#v", stall.Enabled)
+		}
+		if stall.Timeout == nil || *stall.Timeout != "2m" {
+			t.Fatalf("unexpected stall.timeout: %#v", stall.Timeout)
+		}
+		if stall.ChildTimeout == nil || *stall.ChildTimeout != "8m" {
+			t.Fatalf("unexpected stall.child_timeout: %#v", stall.ChildTimeout)
+		}
+		if stall.TerminalCommandTimeout == nil || *stall.TerminalCommandTimeout != "30m" {
+			t.Fatalf("unexpected stall.terminal_command_timeout: %#v", stall.TerminalCommandTimeout)
+		}
+		if stall.Retries == nil || *stall.Retries != 2 {
+			t.Fatalf("unexpected stall.retries: %#v", stall.Retries)
+		}
+		if cfg.Sound.OnParked == nil || *cfg.Sound.OnParked != "ping" {
+			t.Fatalf("unexpected sound.on_parked: %#v", cfg.Sound.OnParked)
+		}
+	})
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/workspace/config_test.go` around lines 487 - 561, Wrap each
standalone stall test in a t.Run subtest using the required Should naming
pattern. Update TestLoadConfigParsesStallSection and
TestLoadConfigMergesStallWorkspaceOverGlobalConfig to place their existing
assertions inside t.Run("Should...", ...) blocks, preserving the current test
logic and helper usage like isolateWorkspaceConfigHome, writeWorkspaceConfig,
writeGlobalConfig, and LoadConfig.

Source: Path instructions

internal/core/run/ui/stall_states_test.go (1)

127-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap single-scenario tests in t.Run("Should...") subtests.

TestParkedJobRendersTerminalParkedState and TestZeroStallRunLeavesPerJobLayoutUnchanged don't use the t.Run("Should...") pattern, unlike the other tests in this file which correctly follow it.

As per path instructions: "MUST use t.Run("Should...") pattern for ALL test cases".

♻️ Suggested wrapping

 func TestParkedJobRendersTerminalParkedState(t *testing.T) {
 	t.Parallel()
+	t.Run("Should render terminal parked state after stall and retry", func(t *testing.T) {
+		t.Parallel()
 
-	m, translator := newStallTestModel(1)
+		m, translator := newStallTestModel(1)
 
-	applyEventToModel(t, m, translator, jobStalledEvent(t, 0, 1))
-	applyEventToModel(t, m, translator, jobRetryEvent(t, 0, 2))
-	applyEventToModel(t, m, translator, jobParkedEvent(t, 0))
+		applyEventToModel(t, m, translator, jobStalledEvent(t, 0, 1))
+		applyEventToModel(t, m, translator, jobRetryEvent(t, 0, 2))
+		applyEventToModel(t, m, translator, jobParkedEvent(t, 0))
 
-	job := &m.jobs[0]
+		job := &m.jobs[0]
 
-	if job.state != jobParked {
-		t.Fatalf("expected jobParked, got %v", job.state)
-	}
+		if job.state != jobParked {
+			t.Fatalf("expected jobParked, got %v", job.state)
+		}
 
-	if got, want := m.jobStateIcon(job.state), jobIconParked; got != want {
-		t.Fatalf("expected icon %q, got %q", want, got)
-	}
+		if got, want := m.jobStateIcon(job.state), jobIconParked; got != want {
+			t.Fatalf("expected icon %q, got %q", want, got)
+		}
 
-	if got := m.sidebarTimeString(job); got != "parked" {
-		t.Fatalf("expected sidebar time string %q, got %q", "parked", got)
-	}
+		if got := m.sidebarTimeString(job); got != "parked" {
+			t.Fatalf("expected sidebar time string %q, got %q", "parked", got)
+		}
 
-	if got, want := job.worktreePath, "/tmp/wt/task_02"; got != want {
-		t.Fatalf("expected preserved worktree %q, got %q", want, got)
-	}
+		if got, want := job.worktreePath, "/tmp/wt/task_02"; got != want {
+			t.Fatalf("expected preserved worktree %q, got %q", want, got)
+		}
 
-	if m.parked != 1 {
-		t.Fatalf("expected parked=1, got %d", m.parked)
-	}
+		if m.parked != 1 {
+			t.Fatalf("expected parked=1, got %d", m.parked)
+		}
 
-	if m.completed != 0 || m.failed != 0 {
-		t.Fatalf("parked must not count as completed or failed, got completed=%d failed=%d", m.completed, m.failed)
-	}
+		if m.completed != 0 || m.failed != 0 {
+			t.Fatalf("parked must not count as completed or failed, got completed=%d failed=%d", m.completed, m.failed)
+		}
 
-	if !m.isRunComplete() {
-		t.Fatal("expected a parked job to settle the run")
-	}
+		if !m.isRunComplete() {
+			t.Fatal("expected a parked job to settle the run")
+		}
 
-	if m.hasActiveJobs() {
-		t.Fatal("expected no active jobs after a park")
-	}
+		if m.hasActiveJobs() {
+			t.Fatal("expected no active jobs after a park")
+		}
 
-	if got := m.composerDisabledLabel(job); got != "Task parked - needs attention" {
-		t.Fatalf("unexpected composer label %q", got)
-	}
+		if got := m.composerDisabledLabel(job); got != "Task parked - needs attention" {
+			t.Fatalf("unexpected composer label %q", got)
+		}
+	})
 }

Also applies to: 271-286

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/ui/stall_states_test.go` around lines 127 - 163, Wrap the
single-scenario tests in this file, especially
TestParkedJobRendersTerminalParkedState and
TestZeroStallRunLeavesPerJobLayoutUnchanged, in t.Run("Should...") subtests to
match the required test pattern used by the other stall state tests. Keep the
existing assertions and setup inside the new subtest blocks, and ensure the
top-level test functions only orchestrate the subtest structure.

Source: Path instructions

internal/core/run/ui/parked_detail_test.go (1)

23-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align subtest names with the "Should..." pattern.

Several tests in this file don't follow the t.Run("Should...") naming convention required by the project's test path instructions. TestParkedJobCarriesFullTriageDetail uses bare names like "reason" and "last tool call", and TestSummaryParkedBoxRendersActionableDetail, TestSummaryViewOmitsParkedBoxWithoutParkedJobs, TestParkedDetailLinesSkipEmptyFields, and TestParkedProgressValue lack subtests entirely.

As per path instructions: "MUST use t.Run("Should...") pattern for ALL test cases".

♻️ Suggested naming adjustments

 func TestParkedJobCarriesFullTriageDetail(t *testing.T) {
 	t.Parallel()
 
 	job := &parkedDetailModel(t).jobs[0]
 	cases := []struct {
 		name string
 		got  string
 		want string
 	}{
-		{"reason", job.stallReason, "no output for 3m0s"},
-		{"last tool call", job.stallLastToolCall, "Bash go test ./..."},
-		{"worktree", job.worktreePath, "/tmp/wt/task_02"},
-		{"log", job.parkLogPath, "/tmp/logs/task_02.out.log"},
+		{"Should carry the stall reason", job.stallReason, "no output for 3m0s"},
+		{"Should carry the last tool call", job.stallLastToolCall, "Bash go test ./..."},
+		{"Should carry the preserved worktree path", job.worktreePath, "/tmp/wt/task_02"},
+		{"Should carry the preserved log path", job.parkLogPath, "/tmp/logs/task_02.out.log"},
 	}

For single-scenario tests, wrap the body in a t.Run:

 func TestSummaryParkedBoxRendersActionableDetail(t *testing.T) {
 	t.Parallel()
+	t.Run("Should render all parked triage fields in the summary box", func(t *testing.T) {
+		t.Parallel()
 
-	m := parkedDetailModel(t)
+		m := parkedDetailModel(t)
 
-	parked := m.parkedJobs()
-	if len(parked) != 1 {
-		t.Fatalf("expected exactly one parked job, got %d", len(parked))
-	}
+		parked := m.parkedJobs()
+		if len(parked) != 1 {
+			t.Fatalf("expected exactly one parked job, got %d", len(parked))
+		}
 
-	box := xansi.Strip(m.renderSummaryParkedBox(80, parked))
-	for _, want := range []string{
-		"RUN.PARKED",
-		statusLabelParked,
-		"task_02",
-		"no output for 3m0s",
-		"Bash go test ./...",
-		"seq 42",
-		"/tmp/wt/task_02",
-		"/tmp/logs/task_02.out.log",
-	} {
-		if !strings.Contains(box, want) {
-			t.Fatalf("parked box missing %q:\n%s", want, box)
-		}
-	}
+		box := xansi.Strip(m.renderSummaryParkedBox(80, parked))
+		for _, want := range []string{
+			"RUN.PARKED",
+			statusLabelParked,
+			"task_02",
+			"no output for 3m0s",
+			"Bash go test ./...",
+			"seq 42",
+			"/tmp/wt/task_02",
+			"/tmp/logs/task_02.out.log",
+		} {
+			if !strings.Contains(box, want) {
+				t.Fatalf("parked box missing %q:\n%s", want, box)
+			}
+		}
+	})
 }

Also applies to: 50-74, 78-90, 94-110, 112-121

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/ui/parked_detail_test.go` around lines 23 - 48, Update the
parked detail tests to follow the required t.Run("Should...") convention. In
TestParkedJobCarriesFullTriageDetail, rename each subtest from generic labels
like "reason" and "last tool call" to descriptive "Should..." names, and wrap
the other single-scenario tests (TestSummaryParkedBoxRendersActionableDetail,
TestSummaryViewOmitsParkedBoxWithoutParkedJobs,
TestParkedDetailLinesSkipEmptyFields, and TestParkedProgressValue) in t.Run
calls as well. Keep the existing assertions and behavior in
parked_detail_test.go unchanged, only adjust the test case structure and names.

Source: Path instructions

internal/core/run/internal/acpshared/diagnostics_test.go (1)

43-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Subtest names should follow the required Should... pattern.

The TestLogProgressSignalDiagnostics subtests (e.g. "logs non-zero drop and backpressure counters when drops occurred") do not use the mandated t.Run("Should...") form. Please rename accordingly.

As per path instructions: "MUST use t.Run("Should...") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/internal/acpshared/diagnostics_test.go` around lines 43 -
104, The subtest names in TestLogProgressSignalDiagnostics do not follow the
required Should... convention. Rename each t.Run case in
logProgressSignalDiagnostics-related tests to start with "Should..." so they
comply with the test naming rule, and update the descriptive strings for the
scenarios covering drops, clean runs, non-journal submitters, and nil sessions.

Source: Path instructions

internal/core/agent/session_test.go (1)

193-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test-case names should follow the required Should... pattern.

The new subtests use descriptive names (e.g. "critical completed update is delivered under saturated buffer", "tool call started is critical") rather than the mandated t.Run("Should...") form. Please rename to the Should... convention.

As per path instructions: "MUST use t.Run("Should...") pattern for ALL test cases".

Also applies to: 356-409

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/agent/session_test.go` around lines 193 - 245, The new subtests
in session_test.go do not follow the required Should... naming convention.
Rename each t.Run case in the affected tests, including the ones around the
critical publish/backpressure scenarios and the related cases later in the file,
so every test name starts with Should... and remains descriptive. Use the
existing subtest blocks and identifiers like t.Run, session.publish, and the
critical/non-critical update scenarios to update the names consistently across
all affected test cases.

Source: Path instructions

internal/core/run/internal/acpshared/watchdog_test.go (1)

122-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefix table-driven subtest names with "Should" to match the required pattern.

The subtest names in TestWatchdogIdleTimeoutArmsIndependentlyOfTimeout are descriptive but don't follow the t.Run("Should...") pattern required for all test cases. Renaming them is straightforward and improves test output consistency.

♻️ Suggested subtest name updates

 		{
-			name: "armed with Timeout=0 when stall enabled",
+			name: "Should arm with Timeout=0 when stall is enabled",
 			cfg:  &config{Timeout: 0, Stall: model.StallPolicy{Enabled: true, IdleTimeout: 3 * time.Minute}},
 			want: 3 * time.Minute,
 		},
 		{
-			name: "disabled stall never arms even with a positive Timeout",
+			name: "Should not arm when stall is disabled even with a positive Timeout",
 			cfg: &config{
 				Timeout: 10 * time.Minute,
 				Stall:   model.StallPolicy{Enabled: false, IdleTimeout: 3 * time.Minute},
 			},
 			want: 0,
 		},
 		{
-			name: "zero idle window never arms",
+			name: "Should not arm when idle window is zero",
 			cfg:  &config{Stall: model.StallPolicy{Enabled: true, IdleTimeout: 0}},
 			want: 0,
 		},
 		{
-			name: "nil config never arms",
+			name: "Should not arm when config is nil",
 			cfg:  nil,
 			want: 0,
 		},

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/internal/acpshared/watchdog_test.go` around lines 122 -
161, The table-driven subtests in
TestWatchdogIdleTimeoutArmsIndependentlyOfTimeout do not follow the required
naming pattern; update each t.Run case name to start with “Should” while keeping
the same scenarios. Make this change in the subtest name strings inside the loop
so the test output is consistent with the expected convention.

Source: Path instructions

internal/core/agent/terminal_lifecycle_test.go (1)

144-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap standalone test cases in t.Run("Should...") subtests.

TestResolveTerminalCapFallsBackToStallDefault, TestTerminalBaseContextPrefersSessionRunContext, and TestClientRequestPermissionReturnsWithinDeadline exercise meaningful business logic but skip the t.Run("Should...") pattern required for all test cases. Wrapping them improves test reporting and complies with the path instruction.

♻️ Suggested refactor for t.Run pattern compliance

 func TestResolveTerminalCapFallsBackToStallDefault(t *testing.T) {
-	t.Parallel()
-
-	client := &clientImpl{}
-	if got := client.resolveTerminalCap(); got != model.DefaultStallTerminalCap {
-		t.Fatalf("resolveTerminalCap() = %s, want default %s", got, model.DefaultStallTerminalCap)
-	}
-	client.terminalCommandTimeout = 5 * time.Minute
-	if got := client.resolveTerminalCap(); got != 5*time.Minute {
-		t.Fatalf("resolveTerminalCap() = %s, want 5m", got)
-	}
+	t.Parallel()
+	t.Run("Should fall back to the stall default when terminalCommandTimeout is unset", func(t *testing.T) {
+		t.Parallel()
+		client := &clientImpl{}
+		if got := client.resolveTerminalCap(); got != model.DefaultStallTerminalCap {
+			t.Fatalf("resolveTerminalCap() = %s, want default %s", got, model.DefaultStallTerminalCap)
+		}
+	})
+	t.Run("Should return the configured timeout when terminalCommandTimeout is set", func(t *testing.T) {
+		t.Parallel()
+		client := &clientImpl{terminalCommandTimeout: 5 * time.Minute}
+		if got := client.resolveTerminalCap(); got != 5*time.Minute {
+			t.Fatalf("resolveTerminalCap() = %s, want 5m", got)
+		}
+	})
 }

Also applies to: 286-308

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/agent/terminal_lifecycle_test.go` around lines 144 - 179, Wrap
the standalone tests in this file, including
TestResolveTerminalCapFallsBackToStallDefault and
TestTerminalBaseContextPrefersSessionRunContext, in t.Run("Should...") subtests
to match the required test naming pattern. Keep the existing assertions and
setup intact, but move each case into a descriptive Should-style subtest so
reporting and compliance are consistent; apply the same refactor to the related
TestClientRequestPermissionReturnsWithinDeadline case as indicated.

Source: Path instructions

internal/core/run/internal/acpshared/session_exec.go (1)

228-278: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider adding a sync.WaitGroup for the watchdog goroutine.

The runActivityWatchdog goroutine is started in StartACPActivityWatchdog and exits via stopCh or cfg.Ctx.Done(), but the stop function returns without waiting for the goroutine to actually exit. This means the goroutine may briefly outlive the caller. While the practical risk is low (the goroutine only reads a ticker and calls idempotent operations), the coding guidelines require tracking all goroutines with sync.WaitGroup or equivalent.

As per coding guidelines: "No fire-and-forget goroutines; track all goroutines with sync.WaitGroup or equivalent."

♻️ Proposed WaitGroup integration

 func StartACPActivityWatchdog(cfg ActivityWatchdogConfig) func() {
 	if cfg.Monitor == nil || cfg.IdleTimeout <= 0 || cfg.Cancel == nil {
 		return func() {}
 	}
 	if cfg.Ctx == nil {
 		cfg.Ctx = context.Background()
 	}
 	if cfg.Clock == nil {
 		cfg.Clock = runshared.RealClock{}
 	}
 	if cfg.Logger == nil {
 		cfg.Logger = silentLogger()
 	}
 	stopCh := make(chan struct{})
 	var stopOnce sync.Once
+	var wg sync.WaitGroup
+	wg.Add(1)
-	go runActivityWatchdog(cfg, watchdogInterval(cfg.IdleTimeout), stopCh)
+	go func() {
+		defer wg.Done()
+		runActivityWatchdog(cfg, watchdogInterval(cfg.IdleTimeout), stopCh)
+	}()
 	return func() {
 		stopOnce.Do(func() {
 			close(stopCh)
 		})
+		wg.Wait()
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/internal/acpshared/session_exec.go` around lines 228 - 278,
StartACPActivityWatchdog launches runActivityWatchdog as a fire-and-forget
goroutine, but the returned stop function only closes stopCh and does not wait
for shutdown. Add a sync.WaitGroup (or equivalent) in StartACPActivityWatchdog
to track the watchdog goroutine, have the goroutine signal completion when
runActivityWatchdog exits, and make the stop closure wait for that completion
before returning. Keep the change localized around StartACPActivityWatchdog,
runActivityWatchdog, and the stopCh/stopOnce flow.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes