Fix runner dispatcher shutdown test isolation

[dennisonbertram]

Closes #1068

Summary

Makes the Runner dispatcher shutdown regression identify the exact Runner under test. A deterministic two-Runner control now proves the target Shutdown cannot return before its own dispatcher exits while an unrelated bounded Runner remains live.

Root cause classification: test false positive, not a production dispatcher leak. The old regression scanned every process goroutine for the shared poolDispatcher function name, so parallel tests' legitimate Runners could be misattributed to the target.

Scope and issue reconciliation

The final diff matches #1068: it adds one narrow internal dispatcher-exit test hook, replaces the process-global target assertion with an instance-scoped two-Runner regression, and records the required plan, impact map, logs, and indexes. Queue draining, inflight accounting, cancellation timeout, idempotent Shutdown, and the existing done/dispatcherWG production contract are preserved.

No changes from PR #1060, PR #1055, or issue #1067 are included. There are no scope deviations.

Impact analysis reconciliation

• Changed runtime symbol: unexported Runner.poolDispatcherExitHook, invoked immediately before the existing dispatcherWG.Done.
• Changed test: TestRunnerDispatcherShutdownIsInstanceScoped in runner_shutdown_test.go.
• Lifecycle/reliability: target shutdown ordering is now deterministically asserted by instance identity.
• Config/API/CLI/wire/persistence/security/clients/providers/tools/deployment: unaffected; no exported contract, data path, credential boundary, or runtime default changed.
• Documentation: issue plan, impact map, active-plan ledger, plans index, and engineering/observational/system/intent logs updated.

Architecture and duplication check

Searched NewRunner, poolDispatcher, poolDispatcherStep, Runner.Shutdown, dispatcherWG, inflight, shutdownOnce, existing worker-pool/shutdown tests, and runnerGoroutineStackContains call sites. The existing per-Runner done plus dispatcherWG remains the only production lifecycle source of truth. The hook is an unexported test seam ordered inside the dispatcher's existing final defer; it does not add a second shutdown primitive or change callers.

Test-first evidence

Red command:

go test -race ./internal/harness -count=5

Observed failure: the original regression failed 4 of 5 repetitions at runner_shutdown_test.go:280, reporting a poolDispatcher somewhere in the process after target Shutdown.

Deterministic red command:

go test -race ./internal/harness -run '^TestRunnerDispatcherShutdownIsInstanceScoped$' -count=1

Observed failure: with an unrelated control Runner intentionally left live, the old global absence assertion failed immediately: process-global detector cannot distinguish the live control Runner from a target leak.

Why it proved the bug: the target Shutdown returned while the control intentionally owned the remaining matching stack frame, demonstrating that a shared function-name substring has no Runner identity.

Green commands:

go test ./internal/harness -run '^TestRunnerDispatcherShutdownIsInstanceScoped$' -count=100
go test -race ./internal/harness -run '^TestRunnerDispatcherShutdownIsInstanceScoped$' -count=100
go test -race ./internal/harness -count=5

All passed. The regression blocks the target's exact exit hook before dispatcherWG.Done, proves Shutdown has not returned, releases that hook, proves Shutdown returns, and confirms the control dispatcher remains live.

Verification evidence

• go test ./internal/harness -run '^TestRunnerDispatcherShutdownIsInstanceScoped$' -count=100 — PASS
• go test -race ./internal/harness -run '^TestRunnerDispatcherShutdownIsInstanceScoped$' -count=100 — PASS
• go test -race ./internal/harness -count=5 — PASS
• go test ./internal/harness ./internal/server — PASS
• go test -race ./internal/harness ./internal/server — PASS
• go vet ./internal/harness ./internal/server — PASS
• unchanged foreground non-TTY ./scripts/test-regression.sh — PASS (go test ./..., go test ./... -race, coverage 85.6%, zero uncovered functions)

No UI or external integration behavior changed, so a mouse/keyboard/API smoke is not applicable.

Review follow-up (verified locally; promotion pending)

Independent review found a second, adjacent lifecycle defect in the test fixtures: five bounded construction sites in runner_worker_pool_test.go create seven Runner instances per package repetition and omitted Shutdown. Because go test -count repeats inside one process, those real fixture dispatchers could survive their tests and contaminate later repetitions even though they did not prove the original target Runner leaked.

The local review fix adds a shared bounded worker-pool test constructor that releases any blocked provider before invoking Runner.Shutdown, routes every affected fixture through it, and adds TestWorkerPoolTestRunnerCleanupStopsDispatcher to prove subtest cleanup fires the exact Runner's dispatcher-exit hook. Other bounded harness fixtures were audited and already own explicit shutdown paths.

Review red:

go test ./internal/harness -run '^TestWorkerPoolTestRunnerCleanupStopsDispatcher$' -count=1

Observed failure: bounded Runner dispatcher survived fixture cleanup because Shutdown was omitted. The red test then explicitly released and shut down its Runner, so the proof itself remained leak-free.

Review-fix verification:

• go test ./internal/harness -run '^TestWorkerPoolTestRunnerCleanupStopsDispatcher$' -count=1 — PASS
• go test -race ./internal/harness -run '^TestWorkerPoolTestRunnerCleanupStopsDispatcher$' -count=1 — PASS
• go test ./internal/harness -run '^TestWorkerPool' -count=100 — PASS
• go test -race ./internal/harness -run '^TestWorkerPool' -count=100 — PASS
• go vet ./internal/harness — PASS
• go test -race ./internal/harness -count=5 — PASS
• unchanged foreground non-TTY ./scripts/test-regression.sh — PASS (go test ./..., go test ./... -race, coverage 85.6%, zero uncovered functions)

At the time of this metadata update, this review fix is intentionally committed only in the local handoff worktree and has not been pushed. PR #1069 therefore remains on a3d8f734728e9362bcdc61c0fef5ec6d0d5419bd, open and unmerged, until the parent promotes the local commit and reruns hosted checks.

Rollout and rollback

No migration, feature flag, persisted data, or deployment ordering is required. Roll back this commit if Runner shutdown deadlocks, latency regresses, queue accounting fails, or hosted normal/race gates regress. Reverting restores the old detector and its known parallel false positive; no data repair is needed.

Documentation

Added the issue plan and impact map; updated docs/plans/INDEX.md, docs/plans/active-plan.md, and engineering, observational, system, and long-term-thinking logs. Public/operator docs and release notes are unaffected because no exported or user-facing behavior changed.

Contract checklist

• Linked issue follows the current structured contract and this PR closes it
• Issue acceptance criteria, impact map, and scope were updated when the design changed
• All callers, consumers, sources of truth, and similar abstractions were searched
• No unrelated cleanup, hidden scope growth, duplicated wiring, or parallel abstraction was introduced
• Tests were written first and the expected red failure was observed, or this is a strictly docs-only minor PR
• Targeted checks and the repository-required full regression are green
• Security, compatibility, lifecycle, deployment, observability, documentation, and rollback were reconciled
• Real mouse/keyboard/API/operator behavior was exercised when the change is interaction- or integration-heavy

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[dennisonbertram]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d1c824230

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Fix the leaked worker-pool runners instead of masking them

Under the documented go test -race ./internal/harness -count=5, this assertion now treats any surviving dispatcher as success. This top-level test is sequential, and each -count iteration waits for its parallel tests before starting the next; bounded tests such as TestWorkerPool_QueuedStatusWhenPoolFull (runner_worker_pool_test.go:110-163) construct a Runner but never call Shutdown, so their dispatchers survive into iterations 2–5—the exact 4/5 failure signature. The change therefore hides and accumulates real test goroutine leaks rather than isolating legitimate concurrent Runners; add shutdown cleanups to the bounded-runner tests while retaining the instance-scoped assertion.

AGENTS.md reference: AGENTS.md:L33-L35

Useful? React with 👍 / 👎.

[dennisonbertram]

Central exact-head verification: a3d8f73 rebased cleanly onto production main d8e7c26. Focused instance-scoped shutdown normal/race passed 100x, complete harness race passed 5x, affected harness/server passed, and foreground ./scripts/test-regression.sh passed at 85.6% with zero uncovered functions. @codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: a3d8f73472

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dennisonbertram]

Exact review correction pushed at e191ff1. The fixture audit found five affected worker-pool construction sites covering seven bounded Runners per repetition; each now releases blocked providers before bounded Shutdown. Cleanup regression normal x50/race x25 and worker-pool lifecycle normal x100/race x50 passed in independent cheap-agent re-review with no findings. Full foreground regression remains PASS at 85.6%, zero uncovered functions. @codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: e191ff16e4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".