fix(browse): apply codex adversarial findings on the new lifecycle

Codex outside-voice review caught five real production-failure modes in
the v1.28.0.0 proxy/headed lifecycle. Fixed:

1) `browse disconnect` skip-graceful for proxy-only daemons
   (browse/src/cli.ts). The graceful /command POST went out with stray
   `domains,` shorthand and (even fixed) the server's disconnect handler
   only tears down headed mode — proxy-only daemons returned 200 "Not
   in headed mode" while leaving the bridge running. Now disconnect
   short-circuits to force-cleanup for non-headed daemons, which kicks
   process.on('exit') in server.ts to close the bridge + Xvfb.

2) sendCommand crash retry preserves --proxy / --headed
   (browse/src/cli.ts). The ECONNRESET retry path called startServer()
   with no extraEnv, silently dropping the proxied flags. A daemon that
   died mid-command would silently restart in default direct/headless
   mode and bypass the SOCKS bridge. Now reapplies BROWSE_PROXY_URL,
   BROWSE_HEADED, and BROWSE_CONFIG_HASH from the resolved global flags.

3) `connect` honors --proxy (browse/src/cli.ts). The headed-mode
   `connect` command built its own serverEnv that didn't include
   BROWSE_PROXY_URL, so `browse --proxy <url> connect` launched headed
   Chromium without the proxy. Now threads proxyUrl + configHash into
   the connect serverEnv.

4) SOCKS5 bridge handles fragmented TCP frames
   (browse/src/socks-bridge.ts). Previously used once('data') and
   parsed each chunk as a complete SOCKS5 frame — TCP doesn't preserve
   message boundaries and split greetings/CONNECT requests caused
   intermittent handshake failures. Replaced with a single state
   machine that buffers chunks and uses size predicates on the SOCKS5
   header to know when a complete frame has arrived. Pauses the client
   socket during upstream connect and replays any remainder bytes
   into the upstream on success.

5) Xvfb cleanup-then-state-delete ordering
   (browse/src/server.ts). emergencyCleanup() previously deleted the
   state file BEFORE any Xvfb cleanup could read it, orphaning Xvfb
   on uncaughtException / unhandledRejection. Now reads the state
   file first, calls cleanupXvfb() (which validates cmdline +
   start-time before kill), then deletes the state file.

Adds a regression test for #4: writes the SOCKS5 greeting + CONNECT
one byte at a time with 5ms ticks, asserts a clean round trip after
the fragmented handshake.

Codex's sixth finding (bridge advertises NO_AUTH on 127.0.0.1, so any
co-located process can use the authenticated upstream) is documented
as a known limitation — gstack's threat model assumes single-user
hosts. Adding bridge-side auth is a separate change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-05-07 14:54:15 -07:00
parent 412a996f1b
commit 50d07eb234
4 changed files with 244 additions and 61 deletions

View File

@@ -996,6 +996,31 @@ if (process.platform === 'win32') {
function emergencyCleanup() {
if (isShuttingDown) return;
isShuttingDown = true;
// Xvfb cleanup MUST happen before state-file deletion. spawnXvfb detaches
// the child, so without this, an uncaught exception leaves the Xvfb
// running with no PID record — orphan accumulates and eventually
// exhausts the :99-:120 display range. Read the state file FIRST,
// call cleanupXvfb (validates cmdline + start-time before kill), THEN
// delete the state file.
try {
if (fs.existsSync(config.stateFile)) {
const raw = fs.readFileSync(config.stateFile, 'utf-8');
const state = JSON.parse(raw);
if (state.xvfbPid && state.xvfbStartTime) {
// Lazy import — emergencyCleanup may run on platforms where
// ./xvfb's Linux-specific helpers fail to load. Best effort.
try {
const { cleanupXvfb } = require('./xvfb');
cleanupXvfb({
pid: state.xvfbPid,
startTime: state.xvfbStartTime,
display: state.xvfbDisplay || ':99',
});
} catch { /* best effort */ }
}
}
} catch { /* state file unparseable — fall through to lock + state cleanup */ }
// Clean Chromium profile locks
const profileDir = path.join(process.env.HOME || '/tmp', '.gstack', 'chromium-profile');
for (const lockFile of ['SingletonLock', 'SingletonSocket', 'SingletonCookie']) {