vnc: reclaim the port before starting, and do not trust a listener we did not spawn
Closes the TODO item about orphaned/duplicated VNC servers across sidecar restarts. The diagnosis there was correct and outlived the move off x11vnc, because the shape of the bug is in the lifecycle, not the server: the running desktop lives in module state, a sidecar restart forgets it while the process keeps running, and waitForPort accepted ANY listener on 5900 as proof of a healthy start. The next start would then spawn a server that could not bind the port, see the ORPHAN listening, and report success — leaving the platform convinced it had started a desktop the browser was not looking at. Two changes. reclaimPort frees the port before spawning: TERM whatever holds it, KILL after two seconds. waitForPort now also fails when the process we spawned has exited, so a foreign listener cannot be mistaken for our own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -136,10 +136,32 @@ async function isPortOpen(port: number): Promise<boolean> {
|
||||
}
|
||||
}
|
||||
|
||||
// Xvnc stays in the foreground, so readiness is the listening port rather than exit code.
|
||||
async function waitForPort(port: number): Promise<boolean> {
|
||||
// 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<void> {
|
||||
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<boolean> {
|
||||
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:
|
||||
|
||||
Reference in New Issue
Block a user