fix(natsrouter,testutil): close Add+Wait race; tighten panic-backstop test fidelity; document best-effort cleanup

CodeRabbit review on PR #157 surfaced three items on the squashed
branch:

(MAJOR) pkg/natsrouter/router.go — Shutdown's wg.Wait() phase could
race with Add(1) calls from still-draining subscriptions when the
closeLoop broke early on ctx.Done(). Per sync.WaitGroup docs, Add
concurrent with Wait is undefined when the counter is zero (panic
risk). Fix: track allClosed; only enter the wg.Wait block when every
subscription confirmed close. If ctx expired before all subs closed,
surface the error and let the caller's deadline take precedence --
remaining handler goroutines continue in the background until process
exit. Comment block rewritten to explain the invariant.

(NIT) pkg/natsrouter/integration_test.go — TestIntegration_SpawnSite-
PanicBackstop used WithMaxConcurrency(2). With cap=2, a leaked
semaphore slot would still leave capacity for the follow-up "ok"
request, masking a cleanup regression. Switched to cap=1 so a leaked
slot blocks the second request and the test actually observes slot
release. Added a comment explaining why cap=1 is load-bearing.

(NIT) pkg/testutil/minio.go — replaced two `_ = container.Terminate(ctx)`
calls with explicit "best-effort cleanup" comments per CLAUDE.md
"never ignore errors silently — comment if intentionally discarded".
The discarded errors are intentional (init already failed; Docker
reaps the container on test-process exit either way), now documented.

Author: claude

pkg/natsrouter/integration_test.go

@@ -281,7 +281,13 @@ func TestIntegration_SpawnSitePanicBackstop(t *testing.T) {
 	nc := setupNATS(t)
 	// Note: NO Recovery middleware installed. We're testing the spawn-site
 	// backstop, not the middleware path.
-	r := natsrouter.New(nc, "integration-panic-backstop", natsrouter.WithMaxConcurrency(2))
+	//
+	// MaxConcurrency=1 is load-bearing: with cap=1, a leaked semaphore slot
+	// would block every subsequent request. cap=2 (or higher) would let
+	// the follow-up "ok" request acquire a slot even if cleanup were
+	// broken, masking the regression. cap=1 forces the test to actually
+	// observe slot release.
+	r := natsrouter.New(nc, "integration-panic-backstop", natsrouter.WithMaxConcurrency(1))
 
 	natsrouter.Register(r, "boom.{id}",
 		func(c *natsrouter.Context, req echoReq) (*echoResp, error) {

pkg/natsrouter/router.go

@@ -256,32 +256,40 @@ func (r *Router) Shutdown(ctx context.Context) error {
 	}
 
 	// Wait for each subscription's dispatcher to finish. On ctx expiry,
-	// record the error and stop waiting on remaining subscriptions — but
-	// DO NOT return early. We must fall through to the WaitGroup wait
-	// below so in-flight handler goroutines are not abandoned. The
-	// WaitGroup wait itself also respects ctx and will short-circuit.
+	// record the error and stop waiting. allClosed tracks whether every
+	// subscription confirmed close; if not, we MUST NOT enter r.wg.Wait()
+	// below -- a still-draining subscription can fire natsHandler, which
+	// calls r.wg.Add(1) concurrently with our Wait(). Per sync.WaitGroup
+	// docs that's an Add+Wait race (panic if counter was 0 when Wait
+	// started). When ctx expires we instead surface the error and let the
+	// caller's deadline take precedence: any remaining handler goroutines
+	// continue in the background until process exit.
+	allClosed := true
 closeLoop:
 	for i, ch := range closed {
 		select {
 		case <-ch:
 		case <-ctx.Done():
 			errs = append(errs, fmt.Errorf("waiting for %q close: %w", subs[i].Subject, ctx.Err()))
+			allClosed = false
 			break closeLoop
 		}
 	}
 
-	// Subscriptions are drained: no new natsHandler callbacks will fire.
-	// Wait for any in-flight handler goroutines that were already spawned
-	// before drain completed. Use a channel so we can select on ctx.Done().
-	wgDone := make(chan struct{})
-	go func() {
-		r.wg.Wait()
-		close(wgDone)
-	}()
-	select {
-	case <-wgDone:
-	case <-ctx.Done():
-		errs = append(errs, fmt.Errorf("waiting for in-flight handlers: %w", ctx.Err()))
+	// Subscriptions are drained: no new natsHandler callbacks will fire,
+	// so r.wg counter is now stable and Wait() is race-free. Skip this
+	// block when allClosed is false (see comment above).
+	if allClosed {
+		wgDone := make(chan struct{})
+		go func() {
+			r.wg.Wait()
+			close(wgDone)
+		}()
+		select {
+		case <-wgDone:
+		case <-ctx.Done():
+			errs = append(errs, fmt.Errorf("waiting for in-flight handlers: %w", ctx.Err()))
+		}
 	}
 
 	return errors.Join(errs...)

pkg/testutil/minio.go

@@ -36,6 +36,9 @@ func ensureMinIOClient() (*minio.Client, error) {
 		// already (no scheme). No TrimPrefix needed.
 		endpoint, err := container.ConnectionString(ctx)
 		if err != nil {
+			// Best-effort: the primary error is what callers need to see;
+			// a Terminate failure during init-failure cleanup is noise.
+			// Docker will reap the container on test-process exit either way.
 			_ = container.Terminate(ctx)
 			minioInitErr = fmt.Errorf("get minio endpoint: %w", err)
 			return
@@ -45,6 +48,7 @@ func ensureMinIOClient() (*minio.Client, error) {
 			Secure: false,
 		})
 		if err != nil {
+			// Best-effort cleanup; see comment above.
 			_ = container.Terminate(ctx)
 			minioInitErr = fmt.Errorf("connect minio: %w", err)
 			return