mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix(browse): terminal-agent allocates from the fixed port scan range, not port:0 (#2314)
The terminal-agent bound `Bun.serve({ port: 0 })` and kept that OS-assigned
port for its whole (weeks-long) lifetime. `port: 0` draws from the OS
EPHEMERAL range (49152-65535 on macOS) — the exact pool every short-lived
`app.listen(0)` test server draws from — so the agent squatted ports that
test suites expected to receive and silently absorbed their traffic as
phantom 404s (two squatting daemons verified in the report).
Fix per decision 8: extract the main server's port allocation into
browse/src/port-allocator.ts (checkPortAvailable / isPortAvailable /
findAvailablePort + the 10000-60000 range constants and the actionable
sandbox-vs-occupied error formatters, all verbatim from server.ts) and make
BOTH long-lived listeners use it — server.ts's findPort is now a thin
findAvailablePort(BROWSE_PORT) wrapper, and terminal-agent's buildServer
takes a pre-allocated port from the same range. No terminal-port consumer
carries a range assumption (they read the port file), verified by grep.
Tests: terminal-agent-port-range (new — allocator stays inside
10000-60000 and below the 49152 ephemeral floor, explicit-port honored,
occupied-explicit throws, static tripwires pin no-port:0 in
terminal-agent.ts and the shared wrapper in server.ts) + findport +
terminal-agent-integration/session-routing/detach-reattach +
dual-listener: 67 pass, 0 fail.
Fixes #2314.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
908c5a6a69
commit
0d4d554e13
@@ -0,0 +1,137 @@
|
||||
/**
|
||||
* Shared loopback port allocation (#2314, decision 8).
|
||||
*
|
||||
* One fixed scan range (10000-60000) for EVERY long-lived gstack listener:
|
||||
* the main browse daemon and the terminal-agent. Binding `port: 0` instead
|
||||
* hands out a port from the OS EPHEMERAL range (49152-65535 on macOS) — the
|
||||
* same pool every short-lived test server draws from — so a daemon that
|
||||
* lives for weeks ends up squatting ports that `app.listen(0)` test servers
|
||||
* expect to receive, silently absorbing their traffic as phantom 404s.
|
||||
*
|
||||
* Extracted from server.ts (which had this logic since #486) so
|
||||
* terminal-agent.ts can reuse it without importing the whole server module.
|
||||
*/
|
||||
|
||||
import * as net from 'net';
|
||||
|
||||
export type PortCheckResult =
|
||||
| { available: true }
|
||||
| { available: false; code?: string; message: string };
|
||||
|
||||
export type FailedPortAttempt = {
|
||||
port: number;
|
||||
result: Extract<PortCheckResult, { available: false }>;
|
||||
};
|
||||
|
||||
export const RANDOM_PORT_MIN = 10000;
|
||||
export const RANDOM_PORT_MAX = 60000;
|
||||
export const RANDOM_PORT_RETRIES = 5;
|
||||
|
||||
export function normalizePortError(err: unknown): Extract<PortCheckResult, { available: false }> {
|
||||
const maybeNodeError = err as NodeJS.ErrnoException | undefined;
|
||||
return {
|
||||
available: false,
|
||||
code: maybeNodeError?.code,
|
||||
message: maybeNodeError?.message || String(err),
|
||||
};
|
||||
}
|
||||
|
||||
export function isOccupiedPort(result: Extract<PortCheckResult, { available: false }>): boolean {
|
||||
return result.code === 'EADDRINUSE';
|
||||
}
|
||||
|
||||
export function formatPortFailureDetail(attempt: FailedPortAttempt): string {
|
||||
const { code, message } = attempt.result;
|
||||
return code ? `${attempt.port} (${code}: ${message})` : `${attempt.port} (${message})`;
|
||||
}
|
||||
|
||||
export function formatExplicitPortUnavailableError(
|
||||
port: number,
|
||||
result: Extract<PortCheckResult, { available: false }>
|
||||
): Error {
|
||||
if (isOccupiedPort(result)) {
|
||||
return new Error(`[browse] Port ${port} (from BROWSE_PORT env) is in use`);
|
||||
}
|
||||
|
||||
const detail = result.code ? `${result.code}: ${result.message}` : result.message;
|
||||
return new Error(
|
||||
`[browse] Cannot bind BROWSE_PORT=${port} on 127.0.0.1 (${detail}). ` +
|
||||
`This usually means localhost port binding is blocked by the current sandbox or OS permissions, ` +
|
||||
`not that the port is occupied. Allow localhost binding, or run browse from an unrestricted terminal.`
|
||||
);
|
||||
}
|
||||
|
||||
export function formatRandomPortUnavailableError(attempts: FailedPortAttempt[]): Error {
|
||||
const blockingAttempts = attempts.filter((attempt) => !isOccupiedPort(attempt.result));
|
||||
|
||||
if (blockingAttempts.length > 0) {
|
||||
const last = blockingAttempts[blockingAttempts.length - 1];
|
||||
return new Error(
|
||||
`[browse] Cannot bind localhost ports after ${attempts.length} attempts in range ` +
|
||||
`${RANDOM_PORT_MIN}-${RANDOM_PORT_MAX}. Last error: ${formatPortFailureDetail(last)}. ` +
|
||||
`This usually means the current sandbox or OS permissions are blocking localhost port binding, ` +
|
||||
`not that every sampled port is occupied. Allow localhost binding, set BROWSE_PORT to an approved ` +
|
||||
`port, or run browse from an unrestricted terminal.`
|
||||
);
|
||||
}
|
||||
|
||||
return new Error(
|
||||
`[browse] No available port after ${RANDOM_PORT_RETRIES} attempts in range ` +
|
||||
`${RANDOM_PORT_MIN}-${RANDOM_PORT_MAX}; every sampled port was already in use`
|
||||
);
|
||||
}
|
||||
|
||||
// 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
|
||||
export function checkPortAvailable(port: number, hostname: string = '127.0.0.1'): Promise<PortCheckResult> {
|
||||
return new Promise((resolve) => {
|
||||
const srv = net.createServer();
|
||||
let settled = false;
|
||||
const finish = (result: PortCheckResult) => {
|
||||
if (settled) return;
|
||||
settled = true;
|
||||
resolve(result);
|
||||
};
|
||||
|
||||
srv.once('error', (err) => finish(normalizePortError(err)));
|
||||
try {
|
||||
srv.listen(port, hostname, () => {
|
||||
srv.close(() => finish({ available: true }));
|
||||
});
|
||||
} catch (err) {
|
||||
finish(normalizePortError(err));
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
export function isPortAvailable(port: number, hostname: string = '127.0.0.1'): Promise<boolean> {
|
||||
return checkPortAvailable(port, hostname).then((result) => result.available);
|
||||
}
|
||||
|
||||
/**
|
||||
* Find a port: the explicit override when given, otherwise a random port in
|
||||
* the fixed 10000-60000 scan range with bounded retries. NEVER `port: 0` —
|
||||
* see the module header for why the ephemeral range is off-limits.
|
||||
*/
|
||||
export async function findAvailablePort(explicitPort?: number | null): Promise<number> {
|
||||
if (explicitPort) {
|
||||
const result = await checkPortAvailable(explicitPort);
|
||||
if (result.available) {
|
||||
return explicitPort;
|
||||
}
|
||||
throw formatExplicitPortUnavailableError(explicitPort, result);
|
||||
}
|
||||
|
||||
const attempts: FailedPortAttempt[] = [];
|
||||
for (let attempt = 0; attempt < RANDOM_PORT_RETRIES; attempt++) {
|
||||
const port = RANDOM_PORT_MIN + Math.floor(Math.random() * (RANDOM_PORT_MAX - RANDOM_PORT_MIN));
|
||||
const result = await checkPortAvailable(port);
|
||||
if (result.available) {
|
||||
return port;
|
||||
}
|
||||
attempts.push({ port, result });
|
||||
}
|
||||
throw formatRandomPortUnavailableError(attempts);
|
||||
}
|
||||
+9
-116
@@ -47,6 +47,9 @@ import { inspectElement, modifyStyle, resetModifications, getModificationHistory
|
||||
// Bun.spawn used instead of child_process.spawn (compiled bun binaries
|
||||
// fail posix_spawn on all executables including /bin/bash)
|
||||
import { safeUnlink, safeUnlinkQuiet, safeKill } from './error-handling';
|
||||
import {
|
||||
findAvailablePort, formatExplicitPortUnavailableError, formatRandomPortUnavailableError,
|
||||
} from './port-allocator';
|
||||
import { readAgentRecord, killAgentByRecord, agentRecordPath, spawnTerminalAgent } from './terminal-agent-control';
|
||||
import { isProcessAlive } from './error-handling';
|
||||
import { sanitizeBody, stripLoneSurrogateEscapes, stripLoneSurrogates, sanitizeReplacer } from './sanitize';
|
||||
@@ -915,124 +918,14 @@ let isShuttingDown = false;
|
||||
// the good final snapshot with a degraded one (zero tabs).
|
||||
let sessionPersistInterval: ReturnType<typeof setInterval> | null = null;
|
||||
|
||||
type PortCheckResult =
|
||||
| { available: true }
|
||||
| { available: false; code?: string; message: string };
|
||||
|
||||
type FailedPortAttempt = {
|
||||
port: number;
|
||||
result: Extract<PortCheckResult, { available: false }>;
|
||||
};
|
||||
|
||||
const RANDOM_PORT_MIN = 10000;
|
||||
const RANDOM_PORT_MAX = 60000;
|
||||
const RANDOM_PORT_RETRIES = 5;
|
||||
|
||||
function normalizePortError(err: unknown): Extract<PortCheckResult, { available: false }> {
|
||||
const maybeNodeError = err as NodeJS.ErrnoException | undefined;
|
||||
return {
|
||||
available: false,
|
||||
code: maybeNodeError?.code,
|
||||
message: maybeNodeError?.message || String(err),
|
||||
};
|
||||
}
|
||||
|
||||
function isOccupiedPort(result: Extract<PortCheckResult, { available: false }>): boolean {
|
||||
return result.code === 'EADDRINUSE';
|
||||
}
|
||||
|
||||
function formatPortFailureDetail(attempt: FailedPortAttempt): string {
|
||||
const { code, message } = attempt.result;
|
||||
return code ? `${attempt.port} (${code}: ${message})` : `${attempt.port} (${message})`;
|
||||
}
|
||||
|
||||
function formatExplicitPortUnavailableError(
|
||||
port: number,
|
||||
result: Extract<PortCheckResult, { available: false }>
|
||||
): Error {
|
||||
if (isOccupiedPort(result)) {
|
||||
return new Error(`[browse] Port ${port} (from BROWSE_PORT env) is in use`);
|
||||
}
|
||||
|
||||
const detail = result.code ? `${result.code}: ${result.message}` : result.message;
|
||||
return new Error(
|
||||
`[browse] Cannot bind BROWSE_PORT=${port} on 127.0.0.1 (${detail}). ` +
|
||||
`This usually means localhost port binding is blocked by the current sandbox or OS permissions, ` +
|
||||
`not that the port is occupied. Allow localhost binding, or run browse from an unrestricted terminal.`
|
||||
);
|
||||
}
|
||||
|
||||
function formatRandomPortUnavailableError(attempts: FailedPortAttempt[]): Error {
|
||||
const blockingAttempts = attempts.filter((attempt) => !isOccupiedPort(attempt.result));
|
||||
|
||||
if (blockingAttempts.length > 0) {
|
||||
const last = blockingAttempts[blockingAttempts.length - 1];
|
||||
return new Error(
|
||||
`[browse] Cannot bind localhost ports after ${attempts.length} attempts in range ` +
|
||||
`${RANDOM_PORT_MIN}-${RANDOM_PORT_MAX}. Last error: ${formatPortFailureDetail(last)}. ` +
|
||||
`This usually means the current sandbox or OS permissions are blocking localhost port binding, ` +
|
||||
`not that every sampled port is occupied. Allow localhost binding, set BROWSE_PORT to an approved ` +
|
||||
`port, or run browse from an unrestricted terminal.`
|
||||
);
|
||||
}
|
||||
|
||||
return new Error(
|
||||
`[browse] No available port after ${RANDOM_PORT_RETRIES} attempts in range ` +
|
||||
`${RANDOM_PORT_MIN}-${RANDOM_PORT_MAX}; every sampled port was already in use`
|
||||
);
|
||||
}
|
||||
|
||||
// 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 checkPortAvailable(port: number, hostname: string = '127.0.0.1'): Promise<PortCheckResult> {
|
||||
return new Promise((resolve) => {
|
||||
const srv = net.createServer();
|
||||
let settled = false;
|
||||
const finish = (result: PortCheckResult) => {
|
||||
if (settled) return;
|
||||
settled = true;
|
||||
resolve(result);
|
||||
};
|
||||
|
||||
srv.once('error', (err) => finish(normalizePortError(err)));
|
||||
try {
|
||||
srv.listen(port, hostname, () => {
|
||||
srv.close(() => finish({ available: true }));
|
||||
});
|
||||
} catch (err) {
|
||||
finish(normalizePortError(err));
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
function isPortAvailable(port: number, hostname: string = '127.0.0.1'): Promise<boolean> {
|
||||
return checkPortAvailable(port, hostname).then((result) => result.available);
|
||||
}
|
||||
// Port allocation lives in port-allocator.ts (#2314, decision 8) so the
|
||||
// terminal-agent shares the SAME fixed 10000-60000 scan range instead of
|
||||
// binding port:0 into the OS ephemeral range. The imports at the top of
|
||||
// this file re-expose the pieces __testInternals__ pins.
|
||||
|
||||
// Find port: explicit BROWSE_PORT, or random in 10000-60000
|
||||
async function findPort(): Promise<number> {
|
||||
// Explicit port override (for debugging)
|
||||
if (BROWSE_PORT) {
|
||||
const result = await checkPortAvailable(BROWSE_PORT);
|
||||
if (result.available) {
|
||||
return BROWSE_PORT;
|
||||
}
|
||||
throw formatExplicitPortUnavailableError(BROWSE_PORT, result);
|
||||
}
|
||||
|
||||
// Random port with retry
|
||||
const attempts: FailedPortAttempt[] = [];
|
||||
for (let attempt = 0; attempt < RANDOM_PORT_RETRIES; attempt++) {
|
||||
const port = RANDOM_PORT_MIN + Math.floor(Math.random() * (RANDOM_PORT_MAX - RANDOM_PORT_MIN));
|
||||
const result = await checkPortAvailable(port);
|
||||
if (result.available) {
|
||||
return port;
|
||||
}
|
||||
attempts.push({ port, result });
|
||||
}
|
||||
throw formatRandomPortUnavailableError(attempts);
|
||||
function findPort(): Promise<number> {
|
||||
return findAvailablePort(BROWSE_PORT);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -27,6 +27,7 @@ import { writeSecureFile, restrictFilePermissions, mkdirSecure } from './file-pe
|
||||
import { atomicWriteSync, atomicWriteQuiet } from '../../lib/fs-atomic';
|
||||
import { safeUnlink } from './error-handling';
|
||||
import { writeAgentRecord, clearAgentRecord } from './terminal-agent-control';
|
||||
import { findAvailablePort } from './port-allocator';
|
||||
import { extractPtyCookie } from './pty-session-cookie';
|
||||
|
||||
const STATE_FILE = process.env.BROWSE_STATE_FILE || path.join(process.env.HOME || '/tmp', '.gstack', 'browse.json');
|
||||
@@ -490,10 +491,15 @@ function maybeSpawnPty(ws: any, session: PtySession): boolean {
|
||||
return true;
|
||||
}
|
||||
|
||||
function buildServer() {
|
||||
function buildServer(port: number) {
|
||||
return Bun.serve({
|
||||
hostname: '127.0.0.1',
|
||||
port: 0,
|
||||
// #2314: allocated from the SAME fixed 10000-60000 scan range the main
|
||||
// server uses (port-allocator.ts, decision 8) — never `port: 0`. Binding
|
||||
// 0 drew from the OS EPHEMERAL range (49152-65535 on macOS), where this
|
||||
// weeks-lived agent squatted ports that short-lived `app.listen(0)` test
|
||||
// servers expected to receive, absorbing their traffic as phantom 404s.
|
||||
port,
|
||||
idleTimeout: 0, // PTY connections are long-lived; default idleTimeout would kill them
|
||||
|
||||
fetch(req, server) {
|
||||
@@ -944,9 +950,12 @@ function readBrowseToken(): string {
|
||||
}
|
||||
|
||||
// Boot.
|
||||
function main() {
|
||||
async function main() {
|
||||
writeClaudeAvailable();
|
||||
const server = buildServer();
|
||||
// #2314: allocate from the shared fixed scan range, then bind. Same
|
||||
// probe-then-bind semantics as the main server's findPort.
|
||||
const allocatedPort = await findAvailablePort();
|
||||
const server = buildServer(allocatedPort);
|
||||
const port = (server as any).port || (server as any).address?.port;
|
||||
if (!port) {
|
||||
console.error('[terminal-agent] failed to bind: no port');
|
||||
@@ -1015,4 +1024,7 @@ try {
|
||||
writeSecureFile(INTERNAL_TOKEN_FILE, INTERNAL_TOKEN);
|
||||
} catch {}
|
||||
|
||||
main();
|
||||
main().catch((err) => {
|
||||
console.error(`[terminal-agent] boot failed: ${err instanceof Error ? err.message : String(err)}`);
|
||||
process.exit(1);
|
||||
});
|
||||
|
||||
@@ -0,0 +1,78 @@
|
||||
/**
|
||||
* #2314: the terminal-agent must allocate its port from the SAME fixed
|
||||
* 10000-60000 scan range the main server uses (port-allocator.ts,
|
||||
* decision 8) — never `port: 0`. Binding 0 drew from the OS ephemeral range
|
||||
* (49152-65535 on macOS), where the weeks-lived agent squatted ports that
|
||||
* short-lived `app.listen(0)` test servers expected to receive, absorbing
|
||||
* their traffic as phantom 404s across every Node test suite on the machine.
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import * as fs from 'fs';
|
||||
import * as net from 'net';
|
||||
import * as path from 'path';
|
||||
import {
|
||||
findAvailablePort,
|
||||
RANDOM_PORT_MIN,
|
||||
RANDOM_PORT_MAX,
|
||||
} from '../src/port-allocator';
|
||||
|
||||
const AGENT_TS = path.resolve(import.meta.dir, '..', 'src', 'terminal-agent.ts');
|
||||
const SERVER_TS = path.resolve(import.meta.dir, '..', 'src', 'server.ts');
|
||||
|
||||
describe('shared port allocator (#2314)', () => {
|
||||
test('allocates inside the fixed scan range, never the ephemeral range', async () => {
|
||||
for (let i = 0; i < 5; i++) {
|
||||
const port = await findAvailablePort();
|
||||
expect(port).toBeGreaterThanOrEqual(RANDOM_PORT_MIN);
|
||||
expect(port).toBeLessThan(RANDOM_PORT_MAX);
|
||||
// The load-bearing property: below the ephemeral floor (49152).
|
||||
expect(RANDOM_PORT_MAX).toBeLessThanOrEqual(60000);
|
||||
expect(RANDOM_PORT_MIN).toBeGreaterThanOrEqual(1024);
|
||||
}
|
||||
});
|
||||
|
||||
test('explicit free port is honored', async () => {
|
||||
// Find a free port by binding 0, then ask the allocator for exactly it.
|
||||
const free = await new Promise<number>((resolve, reject) => {
|
||||
const srv = net.createServer();
|
||||
srv.once('error', reject);
|
||||
srv.listen(0, '127.0.0.1', () => {
|
||||
const p = (srv.address() as net.AddressInfo).port;
|
||||
srv.close(() => resolve(p));
|
||||
});
|
||||
});
|
||||
expect(await findAvailablePort(free)).toBe(free);
|
||||
});
|
||||
|
||||
test('explicit occupied port throws an actionable error', async () => {
|
||||
const srv = net.createServer();
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
srv.once('error', reject);
|
||||
srv.listen(0, '127.0.0.1', () => resolve());
|
||||
});
|
||||
const occupied = (srv.address() as net.AddressInfo).port;
|
||||
try {
|
||||
await expect(findAvailablePort(occupied)).rejects.toThrow(/in use/);
|
||||
} finally {
|
||||
await new Promise<void>((r) => srv.close(() => r()));
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('terminal-agent uses the shared allocator (static tripwire)', () => {
|
||||
test('terminal-agent.ts never binds port: 0', () => {
|
||||
const src = fs.readFileSync(AGENT_TS, 'utf-8');
|
||||
// Strip comments so the explanatory history above the bind doesn't trip.
|
||||
const code = src.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, '');
|
||||
expect(code).not.toMatch(/port:\s*0\b/);
|
||||
expect(src).toContain("from './port-allocator'");
|
||||
expect(src).toContain('findAvailablePort');
|
||||
});
|
||||
|
||||
test('server.ts routes findPort through the same allocator', () => {
|
||||
const src = fs.readFileSync(SERVER_TS, 'utf-8');
|
||||
expect(src).toContain("from './port-allocator'");
|
||||
expect(src).toMatch(/findAvailablePort\(BROWSE_PORT\)/);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user