diff --git a/browse/src/server.ts b/browse/src/server.ts index 8d5a49e0..00e4393a 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -26,6 +26,7 @@ import { emitActivity, subscribe, getActivityAfter, getActivityHistory, getSubsc // Bun.spawn used instead of child_process.spawn (compiled bun binaries // fail posix_spawn on all executables including /bin/bash) import * as fs from 'fs'; +import * as net from 'net'; import * as path from 'path'; import * as crypto from 'crypto'; @@ -547,17 +548,28 @@ export { READ_COMMANDS, WRITE_COMMANDS, META_COMMANDS }; const browserManager = new BrowserManager(); let isShuttingDown = false; +// Test if a port is available by binding and immediately releasing. +// Uses net.createServer instead of Bun.serve to avoid a race condition +// in the Node.js polyfill where listen/close are async but the caller +// expects synchronous bind semantics. See: #486 +function isPortAvailable(port: number, hostname: string = '127.0.0.1'): Promise { + return new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, hostname, () => { + srv.close(() => resolve(true)); + }); + }); +} + // Find port: explicit BROWSE_PORT, or random in 10000-60000 async function findPort(): Promise { // Explicit port override (for debugging) if (BROWSE_PORT) { - try { - const testServer = Bun.serve({ port: BROWSE_PORT, fetch: () => new Response('ok') }); - testServer.stop(); + if (await isPortAvailable(BROWSE_PORT)) { return BROWSE_PORT; - } catch { - throw new Error(`[browse] Port ${BROWSE_PORT} (from BROWSE_PORT env) is in use`); } + throw new Error(`[browse] Port ${BROWSE_PORT} (from BROWSE_PORT env) is in use`); } // Random port with retry @@ -566,12 +578,8 @@ async function findPort(): Promise { const MAX_RETRIES = 5; for (let attempt = 0; attempt < MAX_RETRIES; attempt++) { const port = MIN_PORT + Math.floor(Math.random() * (MAX_PORT - MIN_PORT)); - try { - const testServer = Bun.serve({ port, fetch: () => new Response('ok') }); - testServer.stop(); + if (await isPortAvailable(port)) { return port; - } catch { - continue; } } throw new Error(`[browse] No available port after ${MAX_RETRIES} attempts in range ${MIN_PORT}-${MAX_PORT}`); diff --git a/browse/test/findport.test.ts b/browse/test/findport.test.ts new file mode 100644 index 00000000..fb3a9cb0 --- /dev/null +++ b/browse/test/findport.test.ts @@ -0,0 +1,191 @@ +import { describe, test, expect } from 'bun:test'; +import * as net from 'net'; +import * as path from 'path'; + +const polyfillPath = path.resolve(import.meta.dir, '../src/bun-polyfill.cjs'); + +// Helper: bind a port and hold it open, returning a cleanup function +function occupyPort(port: number): Promise<() => Promise> { + return new Promise((resolve, reject) => { + const srv = net.createServer(); + srv.once('error', reject); + srv.listen(port, '127.0.0.1', () => { + resolve(() => new Promise((r) => srv.close(() => r()))); + }); + }); +} + +// Helper: find a known-free port by binding to 0 +function getFreePort(): Promise { + return new Promise((resolve, reject) => { + const srv = net.createServer(); + srv.once('error', reject); + srv.listen(0, '127.0.0.1', () => { + const port = (srv.address() as net.AddressInfo).port; + srv.close(() => resolve(port)); + }); + }); +} + +describe('findPort / isPortAvailable', () => { + + test('isPortAvailable returns true for a free port', async () => { + // Use the same isPortAvailable logic from server.ts + const port = await getFreePort(); + + const available = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + expect(available).toBe(true); + }); + + test('isPortAvailable returns false for an occupied port', async () => { + const port = await getFreePort(); + const release = await occupyPort(port); + + try { + const available = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + expect(available).toBe(false); + } finally { + await release(); + } + }); + + test('port is actually free after isPortAvailable returns true', async () => { + // This is the core race condition test: after isPortAvailable says + // a port is free, can we IMMEDIATELY bind to it? + const port = await getFreePort(); + + // Simulate isPortAvailable + const isFree = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + expect(isFree).toBe(true); + + // Now immediately try to bind — this would fail with the old + // Bun.serve() polyfill approach because the test server's + // listen() would still be pending + const canBind = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + expect(canBind).toBe(true); + }); + + test('polyfill Bun.serve stop() is fire-and-forget (async)', async () => { + // Verify that the polyfill's stop() does NOT wait for the socket + // to actually close — this is the root cause of the race condition. + // On macOS/Linux the OS reclaims the port fast enough that the race + // rarely manifests, but on Windows TIME_WAIT makes it 100% repro. + const result = Bun.spawnSync(['node', '-e', ` + require('${polyfillPath}'); + const net = require('net'); + + async function test() { + const port = 10000 + Math.floor(Math.random() * 50000); + + const testServer = Bun.serve({ + port, + hostname: '127.0.0.1', + fetch: () => new Response('ok'), + }); + + // stop() returns undefined — it does NOT return a Promise, + // so callers cannot await socket teardown + const retval = testServer.stop(); + console.log(typeof retval === 'undefined' ? 'FIRE_AND_FORGET' : 'AWAITABLE'); + } + + test(); + `], { stdout: 'pipe', stderr: 'pipe' }); + + const output = result.stdout.toString().trim(); + // Confirms the polyfill's stop() is fire-and-forget — callers + // cannot wait for the port to be released, hence the race + expect(output).toBe('FIRE_AND_FORGET'); + }); + + test('net.createServer approach does not have the race condition', async () => { + // Prove the fix: net.createServer with proper async bind/close + // releases the port cleanly + const result = Bun.spawnSync(['node', '-e', ` + const net = require('net'); + + async function testFix() { + const port = 10000 + Math.floor(Math.random() * 50000); + + // Simulate the NEW isPortAvailable: proper async bind/close + const isFree = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + if (!isFree) { + console.log('PORT_BUSY'); + return; + } + + // Immediately try to bind — should succeed because close() + // completed before the Promise resolved + const canBind = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + console.log(canBind ? 'FIX_WORKS' : 'FIX_BROKEN'); + } + + testFix(); + `], { stdout: 'pipe', stderr: 'pipe' }); + + const output = result.stdout.toString().trim(); + expect(output).toBe('FIX_WORKS'); + }); + + test('isPortAvailable handles rapid sequential checks', async () => { + // Stress test: check the same port multiple times in sequence + const port = await getFreePort(); + const results: boolean[] = []; + + for (let i = 0; i < 5; i++) { + const available = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + results.push(available); + } + + // All 5 checks should succeed — no leaked sockets + expect(results).toEqual([true, true, true, true, true]); + }); +});