fix: graceful shutdown and dynamic port resolution

[holiber]

Three bug fixes for createTerminalServer():

1. Port 0 resolution: When called with port=0 (random OS port), the returned port was always 0 instead of the actual assigned port. Now reads the real port from wss.address() after listening.

2. Async listening: Wait for the WebSocketServer 'listening' event before reading wss.address(), ensuring the port is available.

3. Graceful shutdown: Close all WebSocket clients before killing PTY processes. Previously, killing PTYs while clients were connected caused node-pty to throw an uncatchable native C++ exception (libc++abi: terminating due to uncaught Napi::Error), which crashed the host process with SIGABRT.

[holiber]

@cursoragent please review this upcoming PR

[cursor]

Thanks — I reviewed PR #20.

• Main fix looks good: the new two-phase shutdown in src/server/jabtermServer.ts (close/terminate WS clients, brief wait, then kill PTYs) is a sensible mitigation for the node-pty SIGABRT crash during teardown.
• PR description/commit message mismatch: the “port=0 resolution” + “wait for listening before reading address” parts appear to already be in main; this PR’s diff is effectively the shutdown sequencing + some formatting. I’d update the PR description to reflect what’s actually changed.
• A couple of nits/edge cases:
  • close() isn’t fully idempotent under concurrent calls: a second call during the first call’s pre-closePromise window returns Promise.resolve() early. Consider setting closePromise immediately at the top of close() and returning it for all callers.
  • Unrelated to this diff but relevant to the PR text: the CLI currently can’t do --port 0 because it uses args.port || ... (0 gets treated as “unset”). Using args.port ?? ... would fix it.

Overall: LGTM for the shutdown fix; I’d just tweak the PR text and consider the close() concurrency/idempotency improvement (either here or as a follow-up).

  

[holiber]

@cursoragent
Let add your changes that you think are good to add, and make sure ci is green for this PR after your changes

[cursor]

What I changed (pushed to fix/server-shutdown-and-port-resolution)

• Fixed true idempotency for shutdown: createJabtermServer().close() now returns the same closePromise for concurrent callers, avoiding the race where a second caller could get an immediate resolved no-op. (src/server/jabtermServer.ts)
• Fixed CLI --port 0: bin/jabterm-server.mjs now uses args.port ?? envPort, so passing --port 0 correctly results in an ephemeral port instead of being treated as “unset”.

Commit: 4869563 (“fix: idempotent close() and allow --port 0”)

CI / tests

• Ran pnpm run test:smoke successfully: SMOKE: 2/2 passed in 3.8s.
  (Initial failure was just missing Playwright browsers in this fresh environment; after installing browsers + OS deps locally, smoke is green.)

If you want, I can also update the PR description to match the actual diff (since port=0/listening fixes appear to already be in main).

  

[holiber]

@cursoragent
Add tests for the changes

[cursor]

Added tests + small refactors to make them reliable (pushed)

I implemented unit tests for the changes and pushed them to fix/server-shutdown-and-port-resolution:

• New CLI unit tests: added src/server/cli.ts with pure helpers parseCliArgs() + resolveCliPort(), and tests that assert --port 0 is preserved (doesn’t fall back to env/default). (tests/unit/server/cli.test.ts)
• New shutdown unit test: added a test that starts createJabtermServer(), opens a WS connection (with node-pty mocked), calls close() twice concurrently, and verifies it’s idempotent and closes the connection cleanly. (tests/unit/server/close-idempotent.test.ts)
• Small implementation tweak: changed close() to be a non-async function so concurrent callers truly get the same promise instance (no async wrapper promise), strengthening the idempotency guarantee.

Verification

• Ran pnpm run test:unit: all unit tests passed.
• Ran pnpm run test:smoke: green.

Commit pushed: 1e6ac8b (“test: cover shutdown idempotency and --port 0”).

  

[holiber]

@cursoragent nice, merge it and make a new release! Add notes to the PRs in changelog

[cursor]

Done — merged + released with changelog notes

• Merged PR fix: graceful shutdown and dynamic port resolution #20 into main (merge commit e4ed68e).
• Added CHANGELOG.md with release notes referencing PR fix: graceful shutdown and dynamic port resolution #20, and bumped package.json to 0.1.3 (included in the merge).
• Created GitHub Release + tag v0.1.3 targeting main, and fixed the release notes text to match the changelog (no broken formatting).