Work type
Bug / regression
Observed behavior
Under complete-package race stress, TestRunnerWithoutShutdownLeaksDispatcher failed 4 of 5 repetitions after Runner.Shutdown returned successfully. Exact failure at internal/harness/runner_shutdown_test.go:280: poolDispatcher goroutine still present in stack dump after Shutdown; Shutdown did not clean it up.
Exact command: go test -race ./internal/harness -count=5.
The test detects any stack containing poolDispatcher across the whole process. The harness suite runs tests in parallel and other live Runner instances legitimately own dispatcher goroutines, so the current evidence does not identify whether the tested Runner leaked or the global stack assertion observed another test.
Expected behavior
Runner.Shutdown must wait for its own poolDispatcher to exit, and the regression must verify that exact Runner instance without matching unrelated parallel-test goroutines. If a real instance leak exists, the test must deterministically prove it; if not, parallel package load must remain green.
Reproduction
Preconditions: macOS, race detector, full internal/harness package load.
Exact command: go test -race ./internal/harness -count=5.
Actual result: TestRunnerWithoutShutdownLeaksDispatcher failed 4/5 at the post-Shutdown global stack-substring assertion.
Control: the same test generally passes in ordinary single-package runs; failure frequency rises with repeated parallel package load.
Reproduction rate: 4 of 5 in the recorded command.
User and operational impact
A real dispatcher leak would retain goroutines and Runner references across daemon lifecycle, while a false-positive regression blocks every PR full gate and hides real shutdown defects. Either outcome undermines production lifecycle confidence. No security or persisted-data impact is known.
Suspected seam and search evidence
Owning code: internal/harness Runner.poolDispatcher, dispatcherWG, Shutdown, and runner_shutdown_test.go runnerGoroutineStackContains.
Search evidence: Shutdown closes r.done, waits r.inflight and r.dispatcherWG, then returns. poolDispatcher defers dispatcherWG.Done. The failing assertion ignores that instance-owned synchronization and scans all goroutine stacks for the shared function name.
Callers checked: NewRunner starts one dispatcher when WorkerPoolSize is positive; many harness tests run in parallel and create bounded runners.
Current hypothesis: the assertion is process-global and observes another live Runner dispatcher, not the tested instance. The red test must distinguish this from an actual dispatcherWG/lifecycle defect before changing production code.
Adjacent seam ruled out: issue #1067 covers terminal status/event publication, not lifecycle identity. PR #1060 changes remote cron recovery and must not include this fix.
Blast-radius impact map
Callers and data flow: NewRunner -> poolDispatcher goroutine -> Shutdown -> dispatcherWG.
Config/env/defaults: WorkerPoolSize greater than zero activates the surface.
API/CLI/wire formats/tools: None; lifecycle behavior only.
Persistence/schema/cache: None.
Concurrency/lifecycle/cancellation/retries: primary surface; queue drain, inflight accounting, done closure, and dispatcherWG must remain deadlock-free.
Security/auth/permissions/privacy: None.
TUI/web/macOS/other clients: daemon/app shutdown reliability indirectly affected.
Provider/model/tool catalog: None.
Deployment/observability/runbooks: hosted race and full regression stability; goroutine diagnostics.
Compatibility: preserve Shutdown idempotency and existing queue-drain guarantees.
Existing tests/fixtures: runner_shutdown_test.go, runner_worker_pool_test.go, race suite.
Documentation: engineering and observational logs, impact map, active plan, indexes for new docs.
Regression test first
Test file/name: internal/harness/runner_shutdown_test.go, replace or augment TestRunnerWithoutShutdownLeaksDispatcher with an instance-scoped deterministic barrier.
Red command: go test -race ./internal/harness -run TestRunnerDispatcherShutdownIsInstanceScoped -count=100 plus go test -race ./internal/harness -count=5.
Expected failure: before the fix, the instance-scoped signal proves either Shutdown returns before its own dispatcher exits or the existing global detector falsely fails while the instance signal is complete.
Why it proves the bug: it separates runtime lifecycle correctness from cross-test goroutine noise.
False-positive controls: keep a second unrelated bounded Runner alive while shutting down the target; the target assertion must pass while the control dispatcher remains present.
Fix boundaries
In scope: determine runtime leak versus global test false positive; repair the owning Shutdown/dispatcher seam only if the instance signal proves a leak, otherwise make the regression instance-scoped; preserve queue drain and wait-group accounting.
Out of scope: remote cron PR #1060, terminal publication issue #1067, generic goroutine cleanup refactors, worker-pool redesign.
Existing abstraction: r.done plus r.dispatcherWG is the instance-owned lifecycle contract.
No parallel abstraction: tests should observe the existing instance synchronization or a narrow test hook.
Diagnostic and observability evidence
Record goroutine stack output with Runner identity if a real leak exists. Add an instance-scoped dispatcher exit signal in test code or a narrow test hook. Do not rely solely on a shared function-name substring. No prompts or credentials are involved.
Verification plan
- Deterministic two-Runner red/control test.
- Focused normal and race x100.
- Complete internal/harness race x5.
- Affected harness/server normal, race, and vet.
- Unchanged ./scripts/test-regression.sh.
- Hosted test-fast and test-race.
- Verify queue drain, active-run cancellation timeout, and idempotent Shutdown tests remain green.
Rollout and rollback
Ship as a separate small PR before treating the full regression baseline as stable. If production code changes, monitor daemon shutdown latency and goroutine counts; roll back on deadlock, queue accounting regression, or incomplete tool shutdown. If test-only, rollback restores the old detector but also restores the known parallel false positive. No data migration or repair.
Dependencies: independent blocker discovered while validating PR #1060. Must not be combined with #1060 or terminal-order issue #1067.
Documentation and handoff
Update engineering, observational, system, and long-term-thinking logs; add an impact map and update indexes for any new document. Record whether the root cause is a runtime leak or a global assertion false positive and preserve exact red-green commands.
Definition of done
Process acknowledgement
Work type
Bug / regression
Observed behavior
Under complete-package race stress, TestRunnerWithoutShutdownLeaksDispatcher failed 4 of 5 repetitions after Runner.Shutdown returned successfully. Exact failure at internal/harness/runner_shutdown_test.go:280: poolDispatcher goroutine still present in stack dump after Shutdown; Shutdown did not clean it up.
Exact command: go test -race ./internal/harness -count=5.
The test detects any stack containing poolDispatcher across the whole process. The harness suite runs tests in parallel and other live Runner instances legitimately own dispatcher goroutines, so the current evidence does not identify whether the tested Runner leaked or the global stack assertion observed another test.
Expected behavior
Runner.Shutdown must wait for its own poolDispatcher to exit, and the regression must verify that exact Runner instance without matching unrelated parallel-test goroutines. If a real instance leak exists, the test must deterministically prove it; if not, parallel package load must remain green.
Reproduction
Preconditions: macOS, race detector, full internal/harness package load.
Exact command: go test -race ./internal/harness -count=5.
Actual result: TestRunnerWithoutShutdownLeaksDispatcher failed 4/5 at the post-Shutdown global stack-substring assertion.
Control: the same test generally passes in ordinary single-package runs; failure frequency rises with repeated parallel package load.
Reproduction rate: 4 of 5 in the recorded command.
User and operational impact
A real dispatcher leak would retain goroutines and Runner references across daemon lifecycle, while a false-positive regression blocks every PR full gate and hides real shutdown defects. Either outcome undermines production lifecycle confidence. No security or persisted-data impact is known.
Suspected seam and search evidence
Owning code: internal/harness Runner.poolDispatcher, dispatcherWG, Shutdown, and runner_shutdown_test.go runnerGoroutineStackContains.
Search evidence: Shutdown closes r.done, waits r.inflight and r.dispatcherWG, then returns. poolDispatcher defers dispatcherWG.Done. The failing assertion ignores that instance-owned synchronization and scans all goroutine stacks for the shared function name.
Callers checked: NewRunner starts one dispatcher when WorkerPoolSize is positive; many harness tests run in parallel and create bounded runners.
Current hypothesis: the assertion is process-global and observes another live Runner dispatcher, not the tested instance. The red test must distinguish this from an actual dispatcherWG/lifecycle defect before changing production code.
Adjacent seam ruled out: issue #1067 covers terminal status/event publication, not lifecycle identity. PR #1060 changes remote cron recovery and must not include this fix.
Blast-radius impact map
Callers and data flow: NewRunner -> poolDispatcher goroutine -> Shutdown -> dispatcherWG.
Config/env/defaults: WorkerPoolSize greater than zero activates the surface.
API/CLI/wire formats/tools: None; lifecycle behavior only.
Persistence/schema/cache: None.
Concurrency/lifecycle/cancellation/retries: primary surface; queue drain, inflight accounting, done closure, and dispatcherWG must remain deadlock-free.
Security/auth/permissions/privacy: None.
TUI/web/macOS/other clients: daemon/app shutdown reliability indirectly affected.
Provider/model/tool catalog: None.
Deployment/observability/runbooks: hosted race and full regression stability; goroutine diagnostics.
Compatibility: preserve Shutdown idempotency and existing queue-drain guarantees.
Existing tests/fixtures: runner_shutdown_test.go, runner_worker_pool_test.go, race suite.
Documentation: engineering and observational logs, impact map, active plan, indexes for new docs.
Regression test first
Test file/name: internal/harness/runner_shutdown_test.go, replace or augment TestRunnerWithoutShutdownLeaksDispatcher with an instance-scoped deterministic barrier.
Red command: go test -race ./internal/harness -run TestRunnerDispatcherShutdownIsInstanceScoped -count=100 plus go test -race ./internal/harness -count=5.
Expected failure: before the fix, the instance-scoped signal proves either Shutdown returns before its own dispatcher exits or the existing global detector falsely fails while the instance signal is complete.
Why it proves the bug: it separates runtime lifecycle correctness from cross-test goroutine noise.
False-positive controls: keep a second unrelated bounded Runner alive while shutting down the target; the target assertion must pass while the control dispatcher remains present.
Fix boundaries
In scope: determine runtime leak versus global test false positive; repair the owning Shutdown/dispatcher seam only if the instance signal proves a leak, otherwise make the regression instance-scoped; preserve queue drain and wait-group accounting.
Out of scope: remote cron PR #1060, terminal publication issue #1067, generic goroutine cleanup refactors, worker-pool redesign.
Existing abstraction: r.done plus r.dispatcherWG is the instance-owned lifecycle contract.
No parallel abstraction: tests should observe the existing instance synchronization or a narrow test hook.
Diagnostic and observability evidence
Record goroutine stack output with Runner identity if a real leak exists. Add an instance-scoped dispatcher exit signal in test code or a narrow test hook. Do not rely solely on a shared function-name substring. No prompts or credentials are involved.
Verification plan
Rollout and rollback
Ship as a separate small PR before treating the full regression baseline as stable. If production code changes, monitor daemon shutdown latency and goroutine counts; roll back on deadlock, queue accounting regression, or incomplete tool shutdown. If test-only, rollback restores the old detector but also restores the known parallel false positive. No data migration or repair.
Dependencies: independent blocker discovered while validating PR #1060. Must not be combined with #1060 or terminal-order issue #1067.
Documentation and handoff
Update engineering, observational, system, and long-term-thinking logs; add an impact map and update indexes for any new document. Record whether the root cause is a runtime leak or a global assertion false positive and preserve exact red-green commands.
Definition of done
Process acknowledgement