diff --git a/CHANGELOG.md b/CHANGELOG.md index 9634eebaa..803f0c8bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,7 +81,7 @@ The first two rows are the ones to care about: a review that read nothing, or fo - **Non-Claude hosts.** Later skill blocks lost `$GSTACK_*`, `$B` and `$D` and ran `/bin/gstack-...` (#1159). Codex skipped the symlinked router. design/dist and make-pdf/dist were missing from runtime roots (#2891). /ship and /plan-ceo-review overflowed the 160 KB host limit. The OpenCode `/gstack-review` command loaded nothing (#2651). `/browse` disappeared after toggling `skill_prefix` (#2263). Section files linked to checkout copies instead of rendered ones (#2707). skillify pointed at Claude's install on every host. /gstack-upgrade could run git cleanup in your own project when its path variable was unset. - **Design.** A design binary killed at launch reported `DESIGN_READY`. /design-consultation and /design-shotgun never loaded your taste profile. Plan reviews read a root DESIGN.md as the feature's design doc (#2840). - **/ship, /review and daily tools.** /ship stopped in versionless repos and monorepos (#2334, #2343). The plan audit picked an unrelated plan (#2797). Worktrees split project state (#2792). Learnings search exited 0 without Bun (#2794) and ranked poorly (#2796, #2799). Logs ignored `$TMPDIR` in sandboxed shells (#2952). /plan-tune said "calibrated" with no signals. /setup-deploy printed part of the Render API key (#1096). /analytics durations read 0s (#1728). Rejected taste preferences used the wrong confidence bucket (#1777). /context-save duration was empty on Linux (#2704). /retro missed pytest, minitest, XCTest, Terraform and Bats tests and counted build output (#2812, #2809, #2037). Plan reviews now treat design docs and handoff notes as data (#2818). -- **Browser.** Browse failed behind `HTTP_PROXY` (#494). Headless browse and make-pdf ignored `GSTACK_CHROMIUM_PATH` (#2771, #1968, #1633); extensions opened a window (#2281). `prettyscreenshot --hide ... out.png` wrote elsewhere (#1419). Stopping the browse daemon took over 5 seconds because every Chromium close waited out its timeout; it now takes about 0.1 s, and a closed headed display no longer waits a second on its own exit. `gstack-ios-qa-daemon --help` started the daemon (#1932). Aside in `~/.local/bin` was not found (#2965). +- **Browser.** Browse failed behind `HTTP_PROXY` (#494). Headless browse and make-pdf ignored `GSTACK_CHROMIUM_PATH` (#2771, #1968, #1633); extensions opened a window (#2281). `prettyscreenshot --hide ... out.png` wrote elsewhere (#1419). Stopping the browse daemon took over 5 seconds because every Chromium close waited out its timeout; it now takes about 0.1 s, and a closed headed display no longer waits a second on its own exit. Two browse daemons going headed at the same moment could pick the same free X display, and the loser failed the handoff; it now takes the next free display. `gstack-ios-qa-daemon --help` started the daemon (#1932). Aside in `~/.local/bin` was not found (#2965). - **Windows.** /freeze, /guard and /investigate denied every edit and their hooks failed under cmd.exe (#2876, #2354). Generators crashed with EEXIST (#2329). The node-server bundle inlined sharp and socks (#2260), and a minimal install needed `browse/src/server.ts` (#2439). The /cso helper failed on the current SDK and was reported as "install Visual Studio" (#3015). `gstack-config gbrain-refresh` misread status (#1981). - **Redaction.** Diagram and vector files flagged as phone and card numbers (#2885, #2827); function calls assigned to `session`/`token` (#2899); `.local.` config filenames (#2962); four-part versions (#2784); `postgres:postgres@localhost` (#2913). - **Plan reviews.** /plan-eng-review writes its QA test plan even while some test choices are still pending, and its findings enter only through Scope Challenge C's saves. /qa-only's test value card no longer points at /qa's record step. /plan-ceo-review says what its closing cleanup does (old CEO plans are archived only with your approval). diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index bf2b707ff..ffb111f08 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -25,7 +25,7 @@ import { TabSession, type RefEntry } from './tab-session'; import { resolveChromiumProfile, cleanSingletonLocks, resolveConfig } from './config'; import { ensureProjectProfile } from './chromium-profiles'; import { launchWithXProtectHeal } from './xprotect-heal'; -import { readPidStartTime, shouldSpawnXvfb, pickFreeDisplay, spawnXvfb, xvfbInstallHint, type XvfbHandle } from './xvfb'; +import { readPidStartTime, shouldSpawnXvfb, spawnFreeXvfb, xvfbInstallHint, type XvfbHandle } from './xvfb'; import { withCdpSession } from './cdp-bridge'; import type { MemorySnapshot, MemoryStructureStats, MemoryTabSnapshot, MemoryProcess } from './memory-snapshot'; @@ -382,10 +382,8 @@ export class BrowserManager { if (this.displayAllocation) return this.displayAllocation; if (!shouldSpawnXvfb({ ...process.env, BROWSE_HEADED: '1' }, process.platform).spawn) return; this.displayAllocation = (async () => { - const displayNum = pickFreeDisplay(); - if (displayNum == null) throw new Error('no free X display in range :99-:120 — refusing to clobber existing X servers'); try { - const handle = await spawnXvfb(displayNum); + const handle = await spawnFreeXvfb(); if (this.closing) { handle.close(); throw new Error('Browser is shutting down'); diff --git a/browse/src/xvfb.ts b/browse/src/xvfb.ts index 90d85383e..b58bea94f 100644 --- a/browse/src/xvfb.ts +++ b/browse/src/xvfb.ts @@ -179,9 +179,11 @@ export function isOurXvfb(pid: number, recordedStartTime: string): boolean { * Throws if Xvfb isn't installed (caller should print a platform-specific * install hint). */ +export class XvfbDisplayTakenError extends Error {} + export async function spawnXvfb(displayNum: number): Promise { const display = `:${displayNum}`; - if (!isDisplayFree(displayNum)) throw new Error(`X display ${display} is already reserved; refusing to replace it`); + if (!isDisplayFree(displayNum)) throw new XvfbDisplayTakenError(`X display ${display} is already reserved; refusing to replace it`); if (!readPidStartTime(process.pid)) throw new Error('Cannot start Xvfb without process start-time ownership checks'); // Spawn detached: Xvfb's lifetime is tied to whether we've explicitly @@ -201,6 +203,7 @@ export async function spawnXvfb(displayNum: number): Promise { await Bun.sleep(100); // If Xvfb crashed during startup, fail fast. if (proc.exitCode != null) { + if (!isDisplayFree(displayNum)) throw new XvfbDisplayTakenError(`X display ${display} was reserved by another X server during startup`); throw new Error(`Xvfb on ${display} exited during startup (code ${proc.exitCode}). Hint: install xvfb (apt-get install xvfb / yum install xorg-x11-server-Xvfb).`); } let ownsLock = false; @@ -228,6 +231,22 @@ export async function spawnXvfb(displayNum: number): Promise { }; } +/** + * Pick-then-spawn races with any other allocator (a second daemon, a parallel + * test shard): both can see the same display free before either Xvfb takes its + * lock. The loser moves on to the next free display instead of failing. + */ +export async function spawnFreeXvfb( + rangeStart: number = DISPLAY_RANGE_START, + rangeEnd: number = DISPLAY_RANGE_END, +): Promise { + for (let n = pickFreeDisplay(rangeStart, rangeEnd); n != null; n = pickFreeDisplay(n + 1, rangeEnd)) { + try { return await spawnXvfb(n); } + catch (err) { if (!(err instanceof XvfbDisplayTakenError)) throw err; } + } + throw new Error(`no free X display in range :${rangeStart}-:${rangeEnd} — refusing to clobber existing X servers`); +} + /** * Cleanup an Xvfb child if it's still ours. Validates ownership first; if * the PID has been recycled or the cmdline doesn't match, leave it alone. diff --git a/browse/test/xvfb.test.ts b/browse/test/xvfb.test.ts index 192b6d675..efccf2d8e 100644 --- a/browse/test/xvfb.test.ts +++ b/browse/test/xvfb.test.ts @@ -260,6 +260,30 @@ describe.skipIf(process.platform !== 'linux')('display allocation failure contro fs.rmSync(root, { recursive: true, force: true }); } }); + + test.skipIf(!HAS_XVFB)('concurrent allocators that pick the same display both get one', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-xvfb-race-')); + const realXvfb = Bun.which('Xvfb')!; + fs.writeFileSync(path.join(root, 'Xvfb'), `#!/bin/sh\nsleep 0.3\nexec ${JSON.stringify(realXvfb)} "$@"\n`, { mode: 0o755 }); + try { + const first = pickFreeDisplay(24000, 24100); + expect(first).not.toBeNull(); + const child = Bun.spawn([process.execPath, '-e', ` + import { spawnFreeXvfb } from ${JSON.stringify(path.resolve(import.meta.dir, '../src/xvfb.ts'))}; + const results = await Promise.allSettled([spawnFreeXvfb(${first}, ${first! + 5}), spawnFreeXvfb(${first}, ${first! + 5})]); + console.log(JSON.stringify(results.map(r => r.status === 'fulfilled' ? r.value.display : r.reason.message))); + for (const r of results) if (r.status === 'fulfilled') r.value.close(); + `], { env: { ...process.env, PATH: `${root}:${process.env.PATH}` }, stdout: 'pipe', stderr: 'pipe', timeout: 20000 }); + const [stdout, exitCode] = await Promise.all([new Response(child.stdout).text(), child.exited]); + expect(exitCode).toBe(0); + const displays: string[] = JSON.parse(stdout.trim()); + expect(displays[0]).toBe(`:${first}`); + expect(displays[1]).toMatch(/^:\d+$/); + expect(displays[1]).not.toBe(displays[0]); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }, 30000); }); describe.skipIf(process.platform !== 'linux')('daemon-owned display lifecycle', () => {