Skip to content

fix: don't crash with EADDRINUSE when the OAuth callback port is already in use - #312

Open
TimChild wants to merge 1 commit into
geelen:mainfrom
TimChild:fix/oauth-callback-eaddrinuse
Open

fix: don't crash with EADDRINUSE when the OAuth callback port is already in use#312
TimChild wants to merge 1 commit into
geelen:mainfrom
TimChild:fix/oauth-callback-eaddrinuse

Conversation

@TimChild

Copy link
Copy Markdown

The bug

setupOAuthCallbackServerWithLongPoll creates the callback server with no error listener:

const server = app.listen(options.port, '127.0.0.1', () => { ... })

A failed bind is therefore an unhandled 'error' event, and the whole process dies:

node:events:485
      throw er; // Unhandled 'error' event
Error: listen EADDRINUSE: address already in use 127.0.0.1:22129

Why EADDRINUSE is expected here, not an edge case

The callback port isn't arbitrary, so instances don't get lucky by landing on different ports:

  • calculateDefaultPort() derives it deterministically from the server URL hash (3335 + parseInt(hash.slice(0, 4), 16) % 45816), so every instance for a given server computes the same port.
  • findExistingClientPort() then reads the port pinned in client_info.json's redirect_uris and uses it unconditionally — the findAvailablePort() result computed alongside it is discarded on that path. So a first run is safe and every subsequent run reuses the fixed port.
  • The lockfile does coordinate instances, but isLockValid() treats a lock older than MAX_LOCK_AGE (30 min) as stale, while real sessions live for hours or days. Once the lock ages out, a later launch deletes it and binds the port itself — which a still-running instance is holding.

Symptom: when the remote server is unauthenticated (or a token refresh fails), the second and later concurrent sessions crash instead of authenticating. Because the throw happens before the code exchange, no token is ever written, so a host that restarts its stdio child relaunches straight into the same crash.

The fix

Attach an error listener that retries once on an OS-assigned port — the same EADDRINUSE→listen(0) pattern findAvailablePort() already uses a few lines below. The startup log moves to the 'listening' event so it reports the port actually bound rather than the requested one. Non-EADDRINUSE errors, and a failure of the retry itself, are logged instead of thrown.

coordinateAuth already reads the bound port back off the server (server.address()) and writes that into the lockfile, so the fallback port propagates to the coordination path with no changes there. Verified: with the port squatted, the lockfile records the fallback port, not the requested one.

Deliberately kept to the crash: no signature changes, no refactor. One behavioural note for reviewers — if the port changes, dynamic client registration re-registers with the new redirect_uri, but a redirect_uri pinned via --static-oauth-client-info will no longer match, so auth fails with a clear server-side error rather than an unhandled crash. Happy to add a strict/opt-out path (fail with an actionable message instead of rebinding when the port was pinned or passed explicitly) if you'd prefer that.

Verification

  • pnpm check (prettier + tsc) — clean.
  • pnpm test:unit — 104 passed, including a regression test that binds the port first and asserts the callback server ends up on a different port. It fails on main with the raw EADDRINUSE and passes with the fix.
  • pnpm build — success.
  • cd test && pnpm test (e2e) — 2 passed, 1 failed: connects to Hugging Face MCP server expects a model_search tool that server no longer lists. Pre-existing — it fails identically on a clean main checkout, unrelated to this change.

Relationship to #262

#262 (fixing #253) targets the same crash and goes further: it threads the actual port back through the coordinator, adds setCallbackPort, and adds a strictPort mode. If you'd rather take that one, this can be closed — no objection, and I'd rather the fix land in any form. Two things led me to open this anyway: it's ~25 lines confined to one function with no signature or call-site changes, and #262 currently fails pnpm check on prettier formatting in src/lib/utils.ts, so CI is red as it stands.

Found while implementing native MCP OAuth in a client, so I'm not blocked on this either way — filing it because the crash is easy to hit and the fix pattern already exists in the file.

The callback server was created with app.listen() and no 'error' listener, so a
failed bind surfaced as an unhandled 'error' event and killed the process.

EADDRINUSE is an expected outcome here rather than an edge case: the callback
port is derived deterministically from the server URL hash and then pinned into
client_info.json's redirect_uris, so every concurrent instance for the same
server computes the same port. The lockfile that coordinates instances treats
locks older than MAX_LOCK_AGE (30 minutes) as stale, while real sessions run for
much longer, so a later launch deletes the lock and tries to bind the port that
the still-running instance holds.

Attach an 'error' listener that retries once on an OS-assigned port, mirroring
what findAvailablePort() already does, and move the startup log to the
'listening' event so it reports the port actually bound.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant