diff --git a/TODO.md b/TODO.md index 30107ccc..8f67cde0 100644 --- a/TODO.md +++ b/TODO.md @@ -105,15 +105,13 @@ it exists in the reference app, so none of it was in scope for parity. paths and an outdated package name, so `installPi` always fires and floods the logs with `EEXIST` noise. Should detect `@earendil-works/pi-coding-agent` at the real npm prefix. -- [ ] **VNC mirror can orphan/duplicate x11vnc across sidecar restarts.** `vnc-manager.startSession` - tracks the running mirror in module-level state and skips spawning only if that pid is alive. - On a vnc-sidecar restart the state resets to null while the old x11vnc keeps running orphaned; - the next `startSession` spawns a second one. Worse, `waitForPort` treats *any* listener on 5900 - as success, so the platform can believe it started a mirror that is actually a stale process — - desyncing into multiple contending x11vnc instances on `:0` (suspected cause of click-lag on - 2026-07-16, cleared by killing all but one). Fix: before spawning, kill any existing - `x11vnc -display :0` / free port 5900 (an `ExecStartPre`-style cleanup, like the old - officer-vnc.service unit did with `vncserver -kill`). +- [x] **VNC mirror can orphan/duplicate x11vnc across sidecar restarts.** FIXED 2026-08-01 in the + Xvnc rewrite. The diagnosis was right and survived the move off mirroring: the running desktop is + tracked in module-level state, so a sidecar restart forgot it while the server kept running + orphaned, and `waitForPort` treated ANY listener on 5900 as success — so the next start reported + success while the browser talked to the stale process. Now `reclaimPort` frees 5900 (TERM, then + KILL after 2s) before spawning, and `waitForPort` also fails if the process we spawned has + exited, so a listener that is not ours can no longer be mistaken for a healthy start. ## Infra (alpha) diff --git a/src/servers/sidecar/vnc/vnc-manager.ts b/src/servers/sidecar/vnc/vnc-manager.ts index 4abffd32..a0e68d63 100644 --- a/src/servers/sidecar/vnc/vnc-manager.ts +++ b/src/servers/sidecar/vnc/vnc-manager.ts @@ -136,10 +136,32 @@ async function isPortOpen(port: number): Promise { } } -// Xvnc stays in the foreground, so readiness is the listening port rather than exit code. -async function waitForPort(port: number): Promise { +// A listener on the port is NOT proof our server started. The running desktop is tracked in module +// state, so a sidecar restart forgets it while the server keeps running orphaned — then the next start +// spawns a second one, which cannot bind the port, while this check sees the ORPHAN listening and +// reports success. The platform then believes it started a desktop the browser is not looking at. +// So: reclaim the port before spawning, and treat our own process dying as failure. +async function reclaimPort(port: number): Promise { + if (!(await isPortOpen(port))) return; + + console.log(`[vnc] port ${port} already held — killing the orphan before starting`); + Bun.spawnSync({ cmd: ['fuser', '-k', '-TERM', `${port}/tcp`], stdout: 'ignore', stderr: 'ignore' }); + + for (let i = 0; i < 20; i++) { + await Bun.sleep(100); + if (!(await isPortOpen(port))) return; + } + + Bun.spawnSync({ cmd: ['fuser', '-k', '-KILL', `${port}/tcp`], stdout: 'ignore', stderr: 'ignore' }); + await Bun.sleep(300); +} + +// Xvnc stays in the foreground, so readiness is the listening port rather than exit code — but only +// once we know the process we spawned is the one still alive to own it. +async function waitForPort(port: number, proc: { exitCode: number | null }): Promise { const deadline = Date.now() + READY_TIMEOUT_MS; while (Date.now() < deadline) { + if (proc.exitCode !== null) return false; if (await isPortOpen(port)) return true; await Bun.sleep(100); } @@ -155,6 +177,10 @@ export async function startSession(params: VncStartParams): Promise<{ port: numb const homeDir = getOwnerHomeDir(params.email); const { passwdFile } = await ensureVncPassword(homeDir); + // Before choosing a display: an orphan from a previous sidecar life would still hold the port, and + // findFreeDisplay would then hand us a number whose server can never bind it. + await reclaimPort(VNC_PORT); + const display = findFreeDisplay(); const authFile = ensureXauthority(join(homeDir, '.vnc'), display); @@ -203,13 +229,13 @@ export async function startSession(params: VncStartParams): Promise<{ port: numb } })(); - if (!(await waitForPort(VNC_PORT))) { + if (!(await waitForPort(VNC_PORT, proc))) { try { proc.kill(); } catch { // already dead } - throw new Error(`Xvnc failed to listen on ${VNC_PORT}: ${stderrTail.trim() || 'timed out'}`); + throw new Error(`Xvnc failed to listen on ${VNC_PORT}: ${stderrTail.trim() || 'exited or timed out'}`); } // The desktop itself, once the server is accepting. dbus-run-session gives this session its own bus: