fix: release pi review ports on session shutdown

[SyahrulBhudiF]

Summary

Fixes Pi browser-session cleanup so fixed ports (notably remote default 19432) are released when a session ends or its tool call is cancelled.

• Force-close Node HTTP connections before server teardown in annotate, plan review, and code review servers.
• Stop all active browser decision sessions on Pi session_shutdown.
• Wire tool cancellation (AbortSignal) into plan-review browser sessions.
• Add lifecycle coverage for shutdown/cancellation and idempotent cleanup.

Root cause

server.close() stopped accepting new connections but could leave active browser HTTP connections open. In Pi’s long-lived Node process, this
could retain the fixed listener and cause the next annotation/review session to fail with EADDRINUSE.

Test plan

• bun test apps/pi-extension/plannotator-browser.test.ts apps/pi-extension/server/port-startup-compat.test.ts apps/pi-extension/stale-ctx.test.ts --verbose
• bunx tsc --noEmit -p apps/pi-extension/tsconfig.json

Fixes #1159

[backnotprop]

Thanks for the fast PR, and for the thorough issue. Your three fixes are all real and they stay: session cleanup on session_shutdown (covers in-process session replacement), AbortSignal wiring for tool cancellation, and the connection drain on stop. Rather than a review round trip we pushed a fix-up commit e6fa7fb directly to your branch (maintainer edits are enabled) adding the one missing piece, plus two small adjustments. Please pull before further changes.

One correction to the root cause, from empirical testing. We reproduced the exact keep-alive scenario with live Node 24 servers on both macOS and Linux: server.close() alone frees the port for rebinding even while a browser holds open keep-alive sockets, so closeAllConnections() is good hygiene (it matches Bun's stop() semantics and kills lingering sockets immediately) but it is not what un-sticks the port. The actual mechanism behind your repro is that an abandoned session (tab closed, no decision) never calls stop() at all: the server is still deliberately listening when the next command tries to bind the same fixed port. That case fires neither session_shutdown nor an abort, which is why the PR as-is did not cover its own repro steps.

What the fix-up adds: self-preemption on bind contention. When a server start fails with the port-in-use (or port-range-exhausted) error and this pi process has live tracked sessions, we stop them through your stopAllBrowserDecisionSessions() registry, wait 150ms, and retry exactly once. A fresh command is the user asking for a new surface, so the stale session loses. Local random-port sessions never collide, so concurrent local sessions are untouched, and a port held by a different process still fails cleanly after the retry. Your Set-based registry made this clean to build on, nicer than the single-slot tracker we had prototyped.

The two adjustments: closeAllConnections is now optional-called (closeAllConnections?.()) for resilience on older runtimes, and a superseded session now notifies as info ("Previous Plannotator browser session was closed.") instead of surfacing as an error, since supersession and cancellation are expected outcomes.

Also added: six unit tests for the preemption logic through an injectable seam (so they never race your registry tests) and three source-scan tests pinning the drain calls. Full pi suite: 171 pass, typecheck clean.

The /api/exit cross-process probe from your issue is deliberately left out of this PR: same-process contention is the reported case, and cross-process preemption has trust implications worth their own discussion. Feel free to open a follow-up issue if you hit that variant for real.

[backnotprop]

An independent adversarial review ran against e6fa7fb (covering both your work and our fix-up equally) and found real issues, now fixed in b7dd33b, pushed to your branch. Please pull before further changes. Summary:

In the fix-up code (ours): the preemption sweep could stop a concurrent command's freshly bound session (two overlapping commands, second one's sweep kills the first's new server). Now age-gated: only sessions registered before the failing start began are sweep targets.

In the session_shutdown sweep (removed, with reasoning in-code): pi fires session_shutdown on in-process session replacement (/new, resume), and Plannotator deliberately lets browser tabs outlive replacement so feedback from a pre-replacement tab reaches the new session (withCurrentPiSessionFallbackHeader). The sweep killed those tabs, and on real process teardown the OS frees ports anyway, so its only effective case was the harmful one. The registry itself stays and now powers the preemption.

Other fixes in b7dd33b: try/finally in all three server stop() bodies so a throwing dispose can never leave the listener bound (that path would have reproduced the exact bug being fixed); the sweep loop is per-stop exception-safe; archive sessions are now registered (they could preempt others but not be preempted); the stopped error is typed (browser-session-error.ts) instead of substring-matched; a stopped plan review now returns an honest tool result instead of a false "Failed to start" error; your test file no longer launches the real system browser (it was spawning 5 windows per bun test run on a bare machine and reading the developer's ~/.plannotator/config.json - remote mode plus BROWSER clearing suppresses it, restored in afterAll); and new tests pin the age gate, the call-site wrappings, and the registry count.

Full pi suite on b7dd33b: 174 pass, 0 fail; typecheck clean. With these, the PR covers the reported repro (self-preemption), tool cancellation (your signal wiring), and teardown robustness. Thanks for kicking this off and for the registry design, which everything else builds on.