diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ac30adba..5079d885c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Prevent concurrent browser controllers from mutating the same ask-pro session. - Preserve mutable recovery files when a replacement write fails. - Require state-bearing Temporary Chat evidence, delegate manual-login restart cleanup correctly, and persist captured answers before browser cleanup. diff --git a/src/ask-pro/browserRunner.ts b/src/ask-pro/browserRunner.ts index c2f6ad4ee..00a7ab195 100644 --- a/src/ask-pro/browserRunner.ts +++ b/src/ask-pro/browserRunner.ts @@ -25,6 +25,7 @@ import { writeAskProBrowserMetadata, } from "./session.js"; import { harvestLatestAssistantZip, writeResponseZipManifest } from "./responseZip.js"; +import { withSessionControllerLease } from "./sessionControllerLease.js"; const DEFAULT_TIMEOUT_MS = 180 * 60 * 1000; const MANUAL_LOGIN_WAIT_MS = 10 * 60 * 1000; @@ -44,7 +45,14 @@ export interface RunAskProBrowserSessionOptions { verbose?: boolean; } -export async function runAskProBrowserSession({ +export async function runAskProBrowserSession( + options: RunAskProBrowserSessionOptions, +): Promise { + const { dir } = getAskProSessionPaths(options.cwd, options.sessionId); + return withSessionControllerLease(dir, () => runAskProBrowserSessionWithLease(options)); +} + +async function runAskProBrowserSessionWithLease({ cwd, sessionId, temporary, @@ -239,7 +247,7 @@ export async function runAskProBrowserSession({ chromeMode: "launched", }, }); - return runAskProBrowserSession({ + return runAskProBrowserSessionWithLease({ cwd, sessionId, temporary: false, @@ -299,7 +307,14 @@ export async function runAskProBrowserSession({ } } -export async function resumeAskProBrowserSession({ +export async function resumeAskProBrowserSession( + options: RunAskProBrowserSessionOptions, +): Promise { + const { dir } = getAskProSessionPaths(options.cwd, options.sessionId); + return withSessionControllerLease(dir, () => resumeAskProBrowserSessionWithLease(options)); +} + +async function resumeAskProBrowserSessionWithLease({ cwd, sessionId, temporary, @@ -334,7 +349,7 @@ export async function resumeAskProBrowserSession({ sessionId, "Retrying Temporary Chat session in normal ChatGPT; opening managed browser submission.", ); - await runAskProBrowserSession({ + await runAskProBrowserSessionWithLease({ cwd, sessionId, temporary: false, @@ -359,7 +374,7 @@ export async function resumeAskProBrowserSession({ const shouldPreserveUrl = effectiveTemporary !== undefined || (metadata.url !== undefined && !storedUrlIsDefaultTemporary); - await runAskProBrowserSession({ + await runAskProBrowserSessionWithLease({ cwd, sessionId, temporary: effectiveTemporary, diff --git a/src/ask-pro/sessionControllerLease.ts b/src/ask-pro/sessionControllerLease.ts new file mode 100644 index 000000000..49349d37d --- /dev/null +++ b/src/ask-pro/sessionControllerLease.ts @@ -0,0 +1,68 @@ +import { randomUUID } from "node:crypto"; +import { mkdir, readdir, rename, rm, stat, writeFile } from "node:fs/promises"; +import path from "node:path"; +import { isProcessAlive } from "../browser/profileState.js"; + +const LEASE_FILENAME = ".controller.lease"; +const OWNER_PATTERN = /^(\d+)-(.+)\.owner$/; +const REMOVE_OPTIONS = { recursive: true, force: true, maxRetries: 5, retryDelay: 20 } as const; + +export async function withSessionControllerLease( + sessionDir: string, + action: () => Promise, +): Promise { + const id = randomUUID(); + const leasePath = path.join(sessionDir, LEASE_FILENAME); + const candidatePath = `${leasePath}.${id}.candidate`; + const ownerName = `${process.pid}-${id}.owner`; + await mkdir(candidatePath); + + let acquired = false; + try { + await writeFile(path.join(candidatePath, ownerName), ""); + for (;;) { + try { + await rename(candidatePath, leasePath); + acquired = true; + break; + } catch (error) { + if (!(await stat(leasePath).catch(() => null))) throw error; + } + + const existingOwner = (await readdir(leasePath)).find((entry) => OWNER_PATTERN.test(entry)); + const existingPid = Number(existingOwner?.match(OWNER_PATTERN)?.[1]); + if (!existingOwner || !Number.isInteger(existingPid) || existingPid <= 0) { + throw new Error("ask-pro session controller lease is unreadable"); + } + if (isProcessAlive(existingPid)) { + throw new Error(`ask-pro session controller is already running (pid ${existingPid})`); + } + + try { + await rename(path.join(leasePath, existingOwner), path.join(leasePath, ownerName)); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") continue; + throw error; + } + const stalePath = `${leasePath}.${id}.stale`; + await rename(leasePath, stalePath); + await rm(stalePath, REMOVE_OPTIONS); + } + + try { + return await action(); + } finally { + const ownsLease = (await readdir(leasePath).catch((): string[] => [])).includes(ownerName); + if (ownsLease) { + const retiredPath = `${leasePath}.${id}.retired`; + const retired = await rename(leasePath, retiredPath).then( + () => true, + () => false, + ); + if (retired) await rm(retiredPath, REMOVE_OPTIONS).catch(() => undefined); + } + } + } finally { + if (!acquired) await rm(candidatePath, REMOVE_OPTIONS).catch(() => undefined); + } +} diff --git a/tests/ask-pro/browserRunner.test.ts b/tests/ask-pro/browserRunner.test.ts index 4919eadd9..33553277d 100644 --- a/tests/ask-pro/browserRunner.test.ts +++ b/tests/ask-pro/browserRunner.test.ts @@ -106,6 +106,14 @@ const { AskProNeedsAuthError, resumeAskProBrowserSession, runAskProBrowserSessio const tempDirs: string[] = []; +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} + beforeEach(async () => { managedProfileState.root = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-managed-root-")); managedProfileState.legacyRoot = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-legacy-root-")); @@ -134,6 +142,119 @@ afterEach(async () => { }); describe("ask-pro browser runner", () => { + test("rejects a live same-session controller and releases the lease in finally", async () => { + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-controller-live-")); + tempDirs.push(cwd); + const session = await createAskProSession({ + cwd, + question: "Keep one controller per session.", + filePatterns: [], + dryRun: false, + }); + const entered = deferred(); + const finish = deferred(); + runBrowserModeMock.mockImplementationOnce(async () => { + entered.resolve(); + await finish.promise; + return { + answerText: "agent answer", + answerMarkdown: "# Agent\n", + browserTransport: "launched", + }; + }); + + const first = runAskProBrowserSession({ cwd, sessionId: session.id }); + await entered.promise; + await expect(resumeAskProBrowserSession({ cwd, sessionId: session.id })).rejects.toThrow( + `ask-pro session controller is already running (pid ${process.pid})`, + ); + finish.resolve(); + await first; + + runBrowserModeMock.mockRejectedValueOnce(new Error("browser failed")); + await expect(runAskProBrowserSession({ cwd, sessionId: session.id })).rejects.toThrow( + "browser failed", + ); + await runAskProBrowserSession({ cwd, sessionId: session.id }); + await expect(fs.stat(path.join(session.dir, ".controller.lease"))).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + + test("recovers a stale session-controller owner", async () => { + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-controller-stale-")); + tempDirs.push(cwd); + const session = await createAskProSession({ + cwd, + question: "Recover the stale controller.", + filePatterns: [], + dryRun: false, + }); + const leaseDir = path.join(session.dir, ".controller.lease"); + await fs.mkdir(leaseDir); + await fs.writeFile(path.join(leaseDir, "2147483647-stale.owner"), ""); + const entered = deferred(); + const finish = deferred(); + runBrowserModeMock.mockImplementation(async () => { + entered.resolve(); + await finish.promise; + return { + answerText: "agent answer", + answerMarkdown: "# Agent\n", + browserTransport: "launched", + }; + }); + + const runs = [ + runAskProBrowserSession({ cwd, sessionId: session.id }), + runAskProBrowserSession({ cwd, sessionId: session.id }), + ]; + const rejected = deferred(); + for (const run of runs) void run.catch(() => rejected.resolve()); + const outcomesPromise = Promise.allSettled(runs); + await entered.promise; + await rejected.promise; + finish.resolve(); + const outcomes = await outcomesPromise; + + expect(runBrowserModeMock).toHaveBeenCalledTimes(1); + expect(outcomes.map(({ status }) => status).sort()).toEqual(["fulfilled", "rejected"]); + }); + + test("allows browser controllers for different sessions to run concurrently", async () => { + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-controller-concurrent-")); + tempDirs.push(cwd); + const sessions = await Promise.all( + ["first", "second"].map((name) => + createAskProSession({ + cwd, + question: `${name} concurrent session.`, + filePatterns: [], + dryRun: false, + }), + ), + ); + const bothEntered = deferred(); + const finish = deferred(); + let entered = 0; + runBrowserModeMock.mockImplementation(async () => { + if (++entered === 2) bothEntered.resolve(); + await finish.promise; + return { + answerText: "agent answer", + answerMarkdown: "# Agent\n", + browserTransport: "launched", + }; + }); + + const runs = sessions.map((session) => runAskProBrowserSession({ cwd, sessionId: session.id })); + await bothEntered.promise; + finish.resolve(); + await Promise.all(runs); + + expect(runBrowserModeMock).toHaveBeenCalledTimes(2); + }); + test("runs ask-pro sessions with the explicit agent profile", async () => { const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-run-agent-")); tempDirs.push(cwd);