From 5efdb31e492f3dd801992d6ac35c08b3ba72d1fc Mon Sep 17 00:00:00 2001 From: Kaushik Samadder Date: Sat, 8 Aug 2026 23:02:31 +0530 Subject: [PATCH] fix(local-cloak): match profile processes on an exact --user-data-dir boundary Background-profile teardown found the Chromium to kill with a bare substring test on the process command line. Because profile ids may share a prefix (`work` / `work-2`, `default` / `default-2`), `--user-data-dir=` matched sibling profiles too, so closing one background context sent SIGTERM to a healthy concurrent profile's browser. The victim surfaced only the opaque "Target page, context or browser has been closed". The same match had no Cloak-binary check, so a user's own Chrome on a matching profile dir was killed as well. Launch recovery in session-manager.ts already did this correctly. Its process matching moves to profile-processes.ts and is reused by terminateProfile, so both paths agree on which processes belong to a profile. The helpers cannot live in either existing module without an import cycle, since session-manager already imports darwin-background-launch. Teardown on the success path is now best-effort as well, so a ps or kill failure no longer replaces the outcome of browser.close(). The launch-failure path was already guarded this way. --- .../local-cloak/darwin-background-launch.ts | 13 ++-- .../local-cloak/profile-processes.test.ts | 63 +++++++++++++++ .../runtime/local-cloak/profile-processes.ts | 76 +++++++++++++++++++ .../runtime/local-cloak/session-manager.ts | 62 +-------------- 4 files changed, 145 insertions(+), 69 deletions(-) create mode 100644 src/browser/runtime/local-cloak/profile-processes.test.ts create mode 100644 src/browser/runtime/local-cloak/profile-processes.ts diff --git a/src/browser/runtime/local-cloak/darwin-background-launch.ts b/src/browser/runtime/local-cloak/darwin-background-launch.ts index 9a9e9576..3bda8288 100644 --- a/src/browser/runtime/local-cloak/darwin-background-launch.ts +++ b/src/browser/runtime/local-cloak/darwin-background-launch.ts @@ -7,6 +7,7 @@ import { buildLaunchOptions, humanizeBrowser } from 'cloakbrowser'; import type { LaunchPersistentContextOptions } from 'cloakbrowser'; import { chromium } from 'playwright-core'; import type { Browser, BrowserContext } from 'playwright-core'; +import { findCloakProfileProcesses, signalPids } from './profile-processes.js'; const execFileAsync = promisify(execFile); @@ -40,13 +41,7 @@ export async function waitForDevToolsPort(portFile: string, timeoutMs = 10_000): } async function terminateProfile(userDataDir: string): Promise { - const { stdout } = await execFileAsync('/bin/ps', ['-axo', 'pid=,command=']); - const needle = `--user-data-dir=${userDataDir}`; - for (const line of stdout.split('\n')) { - if (!line.includes(needle)) continue; - const pid = Number.parseInt(line.trim().split(/\s+/, 1)[0], 10); - if (Number.isInteger(pid) && pid !== process.pid) process.kill(pid, 'SIGTERM'); - } + signalPids(await findCloakProfileProcesses(userDataDir), 'SIGTERM'); } const defaultDependencies: Dependencies = { @@ -109,7 +104,9 @@ export async function launchDarwinBackgroundPersistentContext( await browser!.close(); } finally { contextActivators.delete(context); - await deps.terminateProfile(options.userDataDir); + // Best-effort: a ps/kill failure must not replace the outcome of + // `browser.close()`, which is what the caller awaited. + await deps.terminateProfile(options.userDataDir).catch(() => {}); } }; return context; diff --git a/src/browser/runtime/local-cloak/profile-processes.test.ts b/src/browser/runtime/local-cloak/profile-processes.test.ts new file mode 100644 index 00000000..51b91084 --- /dev/null +++ b/src/browser/runtime/local-cloak/profile-processes.test.ts @@ -0,0 +1,63 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { mockExecFile } = vi.hoisted(() => ({ mockExecFile: vi.fn() })); + +vi.mock('node:child_process', () => ({ execFile: mockExecFile })); + +import { commandUsesProfileDir, findCloakProfileProcesses, isCloakBrowserCommand } from './profile-processes.js'; + +const CLOAK_BINARY = '/home/u/.cloakbrowser/chromium-1234/chrome-linux/chrome'; +const PROFILES = '/home/u/.webcmd/cloak/profiles'; + +// Offsets keep these apart from the running process, whose pid is filtered out. +const OWNER_PID = process.pid + 1; +const SIBLING_PID = process.pid + 2; +const USER_CHROME_PID = process.pid + 3; + +function mockPs(stdout: string): void { + mockExecFile.mockImplementation((_command, _args, _options, callback) => callback(null, stdout)); +} + +beforeEach(() => { + mockExecFile.mockReset(); +}); + +describe('commandUsesProfileDir', () => { + it('does not match a profile dir that is a prefix of another', () => { + const command = `${CLOAK_BINARY} --user-data-dir=${PROFILES}/work-2 about:blank`; + + expect(commandUsesProfileDir(command, [`${PROFILES}/work`])).toBe(false); + expect(commandUsesProfileDir(command, [`${PROFILES}/work-2`])).toBe(true); + }); + + it('matches the flag at the end of the command line', () => { + expect(commandUsesProfileDir(`${CLOAK_BINARY} --user-data-dir=${PROFILES}/work`, [`${PROFILES}/work`])).toBe(true); + }); +}); + +describe('isCloakBrowserCommand', () => { + it('ignores a Chromium that is not the Cloak build', () => { + expect(isCloakBrowserCommand(`/opt/google/chrome/chrome --user-data-dir=${PROFILES}/work`)).toBe(false); + expect(isCloakBrowserCommand(`${CLOAK_BINARY} --user-data-dir=${PROFILES}/work`)).toBe(true); + }); +}); + +describe('findCloakProfileProcesses', () => { + it('returns only the Cloak process owning the exact profile dir', async () => { + mockPs([ + ` ${OWNER_PID} ${CLOAK_BINARY} --user-data-dir=${PROFILES}/work about:blank`, + ` ${SIBLING_PID} ${CLOAK_BINARY} --user-data-dir=${PROFILES}/work-2 about:blank`, + ` ${USER_CHROME_PID} /opt/google/chrome/chrome --user-data-dir=${PROFILES}/work`, + ` ${process.pid} node webcmd daemon`, + '', + ].join('\n')); + + expect(await findCloakProfileProcesses(`${PROFILES}/work`)).toEqual([OWNER_PID]); + }); + + it('reports no processes when ps fails', async () => { + mockExecFile.mockImplementation((_command, _args, _options, callback) => callback(new Error('ps failed'), '')); + + expect(await findCloakProfileProcesses(`${PROFILES}/work`)).toEqual([]); + }); +}); diff --git a/src/browser/runtime/local-cloak/profile-processes.ts b/src/browser/runtime/local-cloak/profile-processes.ts new file mode 100644 index 00000000..77b20123 --- /dev/null +++ b/src/browser/runtime/local-cloak/profile-processes.ts @@ -0,0 +1,76 @@ +import { execFile } from 'node:child_process'; +import fs from 'node:fs'; + +/** + * Discovery and signalling of the Cloak Chromium processes that own a profile + * directory. + * + * Shared by launch recovery (`session-manager.ts`) and background-profile + * teardown (`darwin-background-launch.ts`) so both agree on which processes + * belong to a profile. `session-manager.ts` already imports + * `darwin-background-launch.ts`, so the matching cannot live in either of them + * without a cycle. + */ + +export async function findCloakProfileProcesses(userDataDir: string): Promise { + const profileDirs = profileDirAliases(userDataDir); + const stdout = await psOutput(); + const pids: number[] = []; + for (const line of stdout.split('\n')) { + const match = line.match(/^\s*(\d+)\s+(.+)$/); + if (!match) continue; + const pid = Number(match[1]); + const command = match[2]; + if (!Number.isInteger(pid) || pid === process.pid) continue; + if (!isCloakBrowserCommand(command)) continue; + if (!commandUsesProfileDir(command, profileDirs)) continue; + pids.push(pid); + } + return [...new Set(pids)]; +} + +export function signalPids(pids: number[], signal: NodeJS.Signals): void { + for (const pid of pids) { + try { + process.kill(pid, signal); + } catch { + // Already exited or not signalable; the follow-up poll decides recovery. + } + } +} + +export function commandUsesProfileDir(command: string, profileDirs: string[]): boolean { + for (const dir of profileDirs) { + const marker = `--user-data-dir=${dir}`; + const index = command.indexOf(marker); + if (index < 0) continue; + // The flag value must end here: profile ids may share a prefix + // (`work` / `work-2`), so a substring match would claim a sibling + // profile's Chromium. + const next = command[index + marker.length]; + if (next === undefined || /\s/.test(next)) return true; + } + return false; +} + +export function profileDirAliases(userDataDir: string): string[] { + const aliases = new Set([userDataDir]); + try { + aliases.add(fs.realpathSync.native(userDataDir)); + } catch { + // The launch path is still useful even if realpath cannot resolve it. + } + return [...aliases]; +} + +export function isCloakBrowserCommand(command: string): boolean { + return command.includes('/.cloakbrowser/') || command.includes('\\.cloakbrowser\\'); +} + +function psOutput(): Promise { + return new Promise((resolve) => { + execFile('ps', ['-axo', 'pid=,command='], { encoding: 'utf8', maxBuffer: 10 * 1024 * 1024, timeout: 2000 }, (err, stdout) => { + resolve(err ? '' : String(stdout)); + }); + }); +} diff --git a/src/browser/runtime/local-cloak/session-manager.ts b/src/browser/runtime/local-cloak/session-manager.ts index 644fb679..e15fb884 100644 --- a/src/browser/runtime/local-cloak/session-manager.ts +++ b/src/browser/runtime/local-cloak/session-manager.ts @@ -1,11 +1,11 @@ import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { execFile } from 'node:child_process'; import type { BrowserContext, Page as PlaywrightPage } from 'playwright-core'; import { launchPersistentContext as cloakLaunchPersistentContext } from 'cloakbrowser'; import type { BrowserSurface, BrowserWindowMode, SiteSessionMode } from '../../protocol.js'; import { activateDarwinBackgroundContext, launchDarwinBackgroundPersistentContext } from './darwin-background-launch.js'; +import { findCloakProfileProcesses, signalPids } from './profile-processes.js'; import { normalizeProfileId, resolveCloakProfileDir } from './profiles.js'; import { CloakNetworkCapture } from './network.js'; import { findPackageRoot } from '../../../package-paths.js'; @@ -607,63 +607,3 @@ async function waitForProfileProcessesToExit(userDataDir: string, timeoutMs: num } return (await findCloakProfileProcesses(userDataDir)).length === 0; } - -function signalPids(pids: number[], signal: NodeJS.Signals): void { - for (const pid of pids) { - try { - process.kill(pid, signal); - } catch { - // Already exited or not signalable; the follow-up poll decides recovery. - } - } -} - -async function findCloakProfileProcesses(userDataDir: string): Promise { - const profileDirs = profileDirAliases(userDataDir); - const stdout = await psOutput(); - const pids: number[] = []; - for (const line of stdout.split('\n')) { - const match = line.match(/^\s*(\d+)\s+(.+)$/); - if (!match) continue; - const pid = Number(match[1]); - const command = match[2]; - if (!Number.isInteger(pid) || pid === process.pid) continue; - if (!isCloakBrowserCommand(command)) continue; - if (!commandUsesProfileDir(command, profileDirs)) continue; - pids.push(pid); - } - return [...new Set(pids)]; -} - -function commandUsesProfileDir(command: string, profileDirs: string[]): boolean { - for (const dir of profileDirs) { - const marker = `--user-data-dir=${dir}`; - const index = command.indexOf(marker); - if (index < 0) continue; - const next = command[index + marker.length]; - if (next === undefined || /\s/.test(next)) return true; - } - return false; -} - -function profileDirAliases(userDataDir: string): string[] { - const aliases = new Set([userDataDir]); - try { - aliases.add(fs.realpathSync.native(userDataDir)); - } catch { - // The launch path is still useful even if realpath cannot resolve it. - } - return [...aliases]; -} - -function isCloakBrowserCommand(command: string): boolean { - return command.includes('/.cloakbrowser/') || command.includes('\\.cloakbrowser\\'); -} - -function psOutput(): Promise { - return new Promise((resolve) => { - execFile('ps', ['-axo', 'pid=,command='], { encoding: 'utf8', maxBuffer: 10 * 1024 * 1024, timeout: 2000 }, (err, stdout) => { - resolve(err ? '' : String(stdout)); - }); - }); -}