Keep OAuth callback sessions on ephemeral ports - #849
Conversation
The callback server already listened on port 0; this makes bind failures actionable and ensures disposal cannot leave an OAuth flow hung. A failed bind now rejects with an error naming the OAuth callback server and hinting at a retry instead of a bare EADDRINUSE, and close() rejects any pending waitForCode so a disposed toolset interrupts interactive auth instead of waiting forever.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Primary · Comment
Ready for human review. The branch stays within the OAuth callback-server boundary and passes the repository pre-PR gate.
No residual findings.
Checks: bun run check (exit 0; 6,286 pass, 0 fail); reviewer-of-record log/stat/message audits; callback-server tests (exit 0).
TheGreatAxios
left a comment
There was a problem hiding this comment.
Critic · Comment
The full origin/main...HEAD behavioral review is clean. Callback sessions use OS-assigned loopback ports, bind failures are actionable, and close rejects pending waits.
No findings.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Critic · Comment
close() rejects pending waitForCode; bind failures get a wrapped error. Listen port is still 0, as on main.
Findings
src/mcp/callback-server.ts:80— every listen error is rewritten as an unexpected ephemeral-port failure that tells the user to retry.EACCES/EADDRNOTAVAILare not retryable, and the wrapper dropscauseandcode. Siblingsrc/auth/oauth/callback-server.ts:96special-casesEADDRINUSEand rethrows other errors as-is.src/mcp/callback-server.test.ts:76— the new port test only checks one server's hostname andport > 0. Changelisten(0, …)to a fixed free port and it still passes, so it does not lock the cross-session invariant the ticket names.
Notes
- Port 0 was already the listen port on main. The hang this branch actually fixes is
waitForCodesurvivingclose().
close() must reject waitForCode so toolset disposal cannot leave interactive authorization hung. Bind failures keep the OS error rather than telling the user to retry an ephemeral port.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Verdict: approve (cannot GitHub-approve own PR).
CL-7529 holds at 2e47f45a. Callback server listens on port 0 only; bind failures keep the OS message with no retry lie; close() rejects pending and post-close waitForCode so disposal cannot hang interactive auth.
No blocking or should-fix findings. Keep the concurrent-port, close-rejects-waiter, and bind-failure tests.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Verdict: approve (GitHub cannot approve own PR).
CL-7529 holds at 2e47f45a. Callback server listens on port 0 only; bind failures keep the OS message with no retry lie; close() rejects pending and post-close waitForCode so disposal cannot hang interactive auth.
No blocking or should-fix findings. Keep the concurrent-port, close-rejects-waiter, and bind-failure tests.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Verdict: approve (GitHub cannot approve own PR).
CL-7529 holds at 2e47f45a. Callback server listens on port 0 only; bind failures keep the OS message; close() rejects pending and post-close waitForCode so disposal cannot hang interactive auth.
CI is green (prettier, eslint, typecheck, build-and-test).
TheGreatAxios
left a comment
There was a problem hiding this comment.
Verdict: approve (GitHub cannot approve own PR).
CL-7529 holds at 2e47f45. Callback server listens on port 0 only; bind failures keep the OS message; close() rejects pending and post-close waitForCode so disposal cannot hang interactive auth.
CI is green (prettier, eslint, typecheck, build-and-test).
Summary
Verification
bun run checkpasses (6,286 tests, 0 failures)Fixes CL-7529