fix: tear down browser sessions at unit boundaries and in stopAuto (#1733) (#1777)

Auto-mode launches Playwright/Chrome for browser-based verification but
never closes browsers between units or during stopAuto teardown. Over
retries and re-dispatches, Chrome processes accumulate and spike RAM.

Add closeBrowser() calls in two locations:
- stopAuto() finally block: ensures browser cleanup on any exit path
- postUnitPreVerification(): tears down browser between unit completions

Both use a getBrowser() guard to skip the import when no browser is active,
keeping the lazy-load pattern intact.

Closes #1733

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
TÂCHES 2026-03-21 09:46:14 -06:00 committed by GitHub
parent accb327552
commit a95d420972
3 changed files with 154 additions and 0 deletions

View file

@ -249,6 +249,18 @@ export async function postUnitPreVerification(pctx: PostUnitContext, opts?: PreV
debugLog("postUnit", { phase: "prune-bg-shell", error: String(e) });
}
// Tear down browser between units to prevent Chrome process accumulation (#1733)
try {
const { getBrowser } = await import("../browser-tools/state.js");
if (getBrowser()) {
const { closeBrowser } = await import("../browser-tools/lifecycle.js");
await closeBrowser();
debugLog("postUnit", { phase: "browser-teardown", status: "closed" });
}
} catch (e) {
debugLog("postUnit", { phase: "browser-teardown", error: String(e) });
}
// Sync worktree state back to project root (skipped for lightweight sidecars)
if (!opts?.skipWorktreeSync && s.originalBasePath && s.originalBasePath !== s.basePath) {
try {

View file

@ -691,6 +691,15 @@ export async function stopAuto(
}
} finally {
// ── Critical invariants: these MUST execute regardless of errors ──
// Browser teardown — prevent orphaned Chrome processes across retries (#1733)
try {
const { getBrowser } = await import("../browser-tools/state.js");
if (getBrowser()) {
const { closeBrowser } = await import("../browser-tools/lifecycle.js");
await closeBrowser();
}
} catch { /* non-fatal: browser-tools may not be loaded */ }
// External cleanup (not covered by session reset)
clearInFlightTools();
clearSliceProgressCache();

View file

@ -0,0 +1,133 @@
/**
* browser-teardown.test.ts Verifies browser cleanup at unit boundaries (#1733).
*
* Tests that the browser-tools lifecycle module is correctly called to tear
* down Chrome/Playwright processes during stopAuto() and between units.
*/
import test from "node:test";
import assert from "node:assert/strict";
// Direct imports of browser-tools state to verify teardown behavior
import {
getBrowser,
setBrowser,
getContext,
setContext,
resetAllState,
} from "../../browser-tools/state.ts";
import { closeBrowser } from "../../browser-tools/lifecycle.ts";
// ─── closeBrowser clears state ──────────────────────────────────────────────
test("closeBrowser resets browser state even when no browser is running", async () => {
// Ensure clean state
resetAllState();
assert.equal(getBrowser(), null, "browser should be null initially");
assert.equal(getContext(), null, "context should be null initially");
// closeBrowser should be safe to call with no active browser
await closeBrowser();
assert.equal(getBrowser(), null, "browser should remain null after closeBrowser");
assert.equal(getContext(), null, "context should remain null after closeBrowser");
});
test("closeBrowser calls browser.close() and resets all state", async () => {
resetAllState();
let closeCalled = false;
const fakeBrowser = {
close: async () => { closeCalled = true; },
} as any;
setBrowser(fakeBrowser);
setContext({ /* fake context */ } as any);
assert.ok(getBrowser(), "browser should be set before teardown");
assert.ok(getContext(), "context should be set before teardown");
await closeBrowser();
assert.equal(closeCalled, true, "browser.close() should have been called");
assert.equal(getBrowser(), null, "browser should be null after teardown");
assert.equal(getContext(), null, "context should be null after teardown");
});
// ─── getBrowser guard pattern ───────────────────────────────────────────────
test("getBrowser() guard prevents unnecessary closeBrowser calls", async () => {
resetAllState();
// This is the pattern used in stopAuto and postUnitPreVerification:
// if (getBrowser()) { await closeBrowser(); }
// Verify the guard works correctly when no browser is active.
let teardownAttempted = false;
if (getBrowser()) {
await closeBrowser();
teardownAttempted = true;
}
assert.equal(teardownAttempted, false, "should not attempt teardown when no browser is active");
});
test("getBrowser() guard triggers closeBrowser when browser is active", async () => {
resetAllState();
let closeCalled = false;
setBrowser({
close: async () => { closeCalled = true; },
} as any);
let teardownAttempted = false;
if (getBrowser()) {
await closeBrowser();
teardownAttempted = true;
}
assert.equal(teardownAttempted, true, "should attempt teardown when browser is active");
assert.equal(closeCalled, true, "browser.close() should have been called");
assert.equal(getBrowser(), null, "browser should be null after guarded teardown");
});
// ─── Source code verification ───────────────────────────────────────────────
test("stopAuto finally block includes browser teardown", async () => {
// Verify the source code contains the browser teardown call
const { readFileSync } = await import("node:fs");
const { resolve } = await import("node:path");
const autoSource = readFileSync(resolve(import.meta.dirname, "..", "auto.ts"), "utf-8");
assert.ok(
autoSource.includes("closeBrowser"),
"auto.ts should reference closeBrowser for teardown in stopAuto",
);
assert.ok(
autoSource.includes("getBrowser"),
"auto.ts should check getBrowser() before calling closeBrowser",
);
assert.ok(
autoSource.includes("browser-tools/lifecycle"),
"auto.ts should import from browser-tools/lifecycle",
);
});
test("postUnitPreVerification includes browser teardown between units", async () => {
const { readFileSync } = await import("node:fs");
const { resolve } = await import("node:path");
const postUnitSource = readFileSync(resolve(import.meta.dirname, "..", "auto-post-unit.ts"), "utf-8");
assert.ok(
postUnitSource.includes("closeBrowser"),
"auto-post-unit.ts should reference closeBrowser for inter-unit teardown",
);
assert.ok(
postUnitSource.includes("getBrowser"),
"auto-post-unit.ts should check getBrowser() before calling closeBrowser",
);
assert.ok(
postUnitSource.includes("browser-teardown"),
"auto-post-unit.ts should have browser-teardown debug phase",
);
});