refactor: replace Bun.serve with Node http.createServer in OAuth handlers#18327
refactor: replace Bun.serve with Node http.createServer in OAuth handlers#18327
Conversation
…lers Use Node's createServer for MCP OAuth callback and Codex plugin OAuth servers instead of Bun.serve, making them work under Node.js.
Greptile SummaryThis PR refactors both OAuth servers — in Key observations:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant Server as http.createServer
participant Handler as Request Handler
participant Pending as pendingOAuth/pendingAuths
App->>Server: createServer(handler) + listen(PORT)
Note over Server: Listening on PORT
App->>Pending: waitForOAuthCallback(state)
Note over App: Browser redirected to localhost
Server->>Handler: GET /auth/callback?code=X&state=Y
Handler->>Pending: resolve(code or tokens)
Handler->>Server: res.writeHead(200) + res.end(HTML_SUCCESS)
alt codex.ts stopOAuthServer
App->>Server: server.close(cb) — async
App->>App: oauthServer = undefined — SYNC (race!)
Note over Server: Port may still be bound when next startOAuthServer runs
else oauth-callback.ts stop
App->>Server: await server.close(cb)
Note over Server: Port released before returning
App->>App: server = undefined
end
Last reviewed commit: "refactor: replace Bu..." |
| function stopOAuthServer() { | ||
| if (oauthServer) { | ||
| oauthServer.stop() | ||
| oauthServer.close(() => { | ||
| log.info("codex oauth server stopped") | ||
| }) | ||
| oauthServer = undefined | ||
| log.info("codex oauth server stopped") | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition: port still in use after
stopOAuthServer returns
oauthServer is set to undefined synchronously, but server.close() in Node.js is asynchronous — the underlying port is only released once the close callback fires. If startOAuthServer() is called again before that callback fires (e.g. the user immediately re-initiates auth), oauthServer is undefined so the early-return guard is bypassed, and the new listen() call will fail with EADDRINUSE.
By contrast, Bun.serve.stop() released the port synchronously. The sibling McpOAuthCallback.stop() correctly awaits the close before clearing the reference:
await new Promise<void>((resolve) => server!.close(() => resolve()))
server = undefinedstopOAuthServer should follow the same pattern (make it async and await the close) or at minimum only clear oauthServer inside the close callback:
function stopOAuthServer() {
if (oauthServer) {
const closing = oauthServer
oauthServer = undefined
closing.close(() => {
log.info("codex oauth server stopped")
})
}
}Note: even the snippet above doesn't fully close the port before returning. If true synchronous-style safety is needed, the function should be made async and awaited at the call site.
|
|
||
| export async function ensureRunning(): Promise<void> { | ||
| if (server) return | ||
| function handleRequest(req: import("http").IncomingMessage, res: import("http").ServerResponse) { |
There was a problem hiding this comment.
Inline dynamic import types are unnecessary
import("http").IncomingMessage and import("http").ServerResponse are used as inline type expressions, but http is already imported at line 2. The types can be imported alongside createServer to keep the signature readable.
| function handleRequest(req: import("http").IncomingMessage, res: import("http").ServerResponse) { | |
| function handleRequest(req: import("http").IncomingMessage, res: import("http").ServerResponse) { |
Change the top-level import to:
import { createServer, type IncomingMessage, type ServerResponse } from "http"Then the function signature becomes:
function handleRequest(req: IncomingMessage, res: ServerResponse) {Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
…ver in OAuth handlers
Summary
Bun.servewithhttp.createServerin MCP OAuth callback serverBun.servewithhttp.createServerin Codex plugin OAuth serverres.writeHead/res.end