Skip to content

Commit e30de89

Browse files
claudeclaude
committed
fix(test): make the web-shutdown lane's idle-timer proof and cleanup real
Two problems in the shutdown lane added for #1868: 1. createWebSmokeContext() unconditionally set AGENT_BROWSER_IDLE_TIMEOUT_MS to 30s for every caller, including the new shutdown scenario, whose own poll window is 45s. A reverted fix (no active browser close on shutdown) could still pass: agent-browser's own 30s idle timer would reap the fleet on its own well inside the 45s window, independent of whatever the daemon's teardown did or didn't do. The comment claiming production 5-minute behavior was simply false. Parameterize the context so only the functional smoke test opts into the shortened idle timeout; the shutdown lane now omits the override entirely, leaving the real 5-minute default in place (pinned by the existing `resolveAgentBrowserIdleTimeoutMs({})` case in agent-browser-lifecycle.test.ts) — comfortably past the 45s poll, so a pass can only mean the daemon actively closed the browser. 2. The scenario's `finally` only closed the fixture HTTP server. A failed assertion (including the exact failure mode this test exists to catch) left the daemon process and any still-running Chrome processes on the runner with nothing cleaning them up. Move cleanup authority fully into `finally`: stopProcessForTakeover on the daemon pid (a no-op if it already exited) and the same orphan sweep daemon startup runs for any leftover managed-browser processes, both best-effort and independent of how far the try block got, mirroring cleanupWebSmoke's AggregateError shape so a cleanup failure never swallows the assertion failure it ran alongside. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_011ot5wg8YUEyWRrphs8bMSJ
1 parent ce3adf4 commit e30de89

1 file changed

Lines changed: 67 additions & 11 deletions

File tree

‎test/integration/smoke-web-platform.test.ts‎

Lines changed: 67 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,22 +8,30 @@ import { assertPngDimensions, assertPngFile } from './provider-scenarios/asserti
88
import { runCleanupWithCoverageReport } from './web-e2e/coverage-report.ts';
99
import { assertNoDaemonLeaks } from './support/daemon-leak-oracle.ts';
1010
import {
11+
cleanupManagedAgentBrowserOrphans,
1112
inspectManagedAgentBrowserProcesses,
1213
type AgentBrowserProcessSummary,
1314
} from '../../src/platforms/web/agent-browser-lifecycle.ts';
14-
import { getManagedAgentBrowserStatus } from '../../src/platforms/web/agent-browser-tool.ts';
15+
import {
16+
getManagedAgentBrowserStatus,
17+
type AgentBrowserToolStatus,
18+
} from '../../src/platforms/web/agent-browser-tool.ts';
19+
import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts';
1520
import { isProcessAlive } from '../../src/utils/host-process.ts';
1621

1722
const TEST_NAME = 'live web platform e2e smoke';
1823
const SHUTDOWN_TEST_NAME = 'live web platform e2e daemon-shutdown browser cleanup';
1924
const WEB_E2E_ENABLED = process.env.AGENT_DEVICE_WEB_E2E === '1';
2025
// #1868: SIGTERM must close the managed Chrome fleet well before agent-browser's own idle timer
21-
// (DEFAULT_AGENT_BROWSER_IDLE_TIMEOUT_MS, 5 minutes) would have. Left at its production default
22-
// here (unlike the functional smoke test's shortened override below) so a pass can only mean the
23-
// daemon's shutdown teardown actively closed the browser, not that the idle timer coincidentally
24-
// beat this test's own poll deadline.
26+
// would have. This lane's context omits the AGENT_BROWSER_IDLE_TIMEOUT_MS override the functional
27+
// smoke test above uses, so the idle timer that could otherwise reap the fleet on its own stays at
28+
// its production default — DEFAULT_AGENT_BROWSER_IDLE_TIMEOUT_MS, 5 minutes, pinned by
29+
// `resolveAgentBrowserIdleTimeoutMs({})` in agent-browser-lifecycle.test.ts — comfortably past this
30+
// poll deadline. A pass can then only mean the daemon's shutdown teardown actively closed the
31+
// browser, not that the idle timer coincidentally beat this test's own wait.
2532
const WEB_SHUTDOWN_SETTLE_TIMEOUT_MS = 45_000;
2633
const WEB_SHUTDOWN_SETTLE_POLL_MS = 500;
34+
const WEB_SHUTDOWN_CLEANUP_TIMEOUT_MS = 5_000;
2735

2836
type StepRecord = {
2937
step: string;
@@ -53,7 +61,9 @@ test(
5361
: 'Set AGENT_DEVICE_WEB_E2E=1 to run the managed web backend smoke.',
5462
},
5563
async () => {
56-
await runWebSmoke(await createWebSmokeContext());
64+
// Shortens agent-browser's own idle-reap window so a leaked fleet from this test doesn't
65+
// linger on the runner; the shutdown lane below deliberately omits this override instead.
66+
await runWebSmoke(await createWebSmokeContext({ agentBrowserIdleTimeoutMs: '30000' }));
5767
},
5868
);
5969

@@ -77,21 +87,26 @@ test(
7787
);
7888

7989
async function runWebShutdownSmoke(context: WebSmokeContext): Promise<void> {
90+
// Cleanup authority lives entirely in `finally`, driven by these two, so a failed assertion
91+
// above (including the very failure this test exists to catch: processes still alive when the
92+
// fix regresses) can never leave a daemon or a Chrome fleet running on the host afterward.
93+
let daemonPid: number | undefined;
94+
let status: AgentBrowserToolStatus | undefined;
8095
try {
8196
await runStep(context, 'set up managed web backend', ['web', 'setup', '--json']);
8297
await runStep(context, 'open local fixture', ['open', context.url, ...context.common]);
8398

8499
const stateDir = context.env.AGENT_DEVICE_STATE_DIR;
85100
assert.ok(stateDir, 'expected the smoke context to configure a state dir');
86-
const status = getManagedAgentBrowserStatus({ stateDir });
101+
status = getManagedAgentBrowserStatus({ stateDir });
87102

88103
const before = await inspectManagedAgentBrowserProcesses(status);
89104
assert.ok(
90105
before.count > 0,
91106
`expected the managed browser fleet to be running after open, found none: ${formatProcessSummary(before)}`,
92107
);
93108

94-
const daemonPid = readDaemonPid(stateDir);
109+
daemonPid = readDaemonPid(stateDir);
95110
assert.equal(isProcessAlive(daemonPid), true, 'expected a live daemon before SIGTERM');
96111

97112
// The scenario #1868 is about: SIGTERM the daemon directly, the way an operator or an
@@ -114,8 +129,45 @@ async function runWebShutdownSmoke(context: WebSmokeContext): Promise<void> {
114129
// state-dir residue either, on top of the process-level proof above.
115130
await assertNoDaemonLeaks({ stateDir, daemonPids: [daemonPid], phase: 'after-shutdown' });
116131
} finally {
132+
await cleanupWebShutdownSmoke(context, daemonPid, status);
133+
}
134+
}
135+
136+
// Best-effort and independent of how far the `try` block got: a daemon that survived SIGTERM
137+
// (stopProcessForTakeover escalates to SIGKILL) and a Chrome fleet that outlived it (the same
138+
// orphan sweep daemon startup runs) are reaped here regardless of which assertion above failed,
139+
// or whether none did. Mirrors cleanupWebSmoke's AggregateError shape so a cleanup failure never
140+
// silently swallows the original assertion failure it ran alongside.
141+
async function cleanupWebShutdownSmoke(
142+
context: WebSmokeContext,
143+
daemonPid: number | undefined,
144+
status: AgentBrowserToolStatus | undefined,
145+
): Promise<void> {
146+
const errors: unknown[] = [];
147+
if (daemonPid !== undefined) {
148+
try {
149+
await stopProcessForTakeover(daemonPid, {
150+
termTimeoutMs: WEB_SHUTDOWN_CLEANUP_TIMEOUT_MS,
151+
killTimeoutMs: WEB_SHUTDOWN_CLEANUP_TIMEOUT_MS,
152+
});
153+
} catch (error) {
154+
errors.push(error);
155+
}
156+
}
157+
if (status) {
158+
try {
159+
await cleanupManagedAgentBrowserOrphans(status, 'daemon-startup', {});
160+
} catch (error) {
161+
errors.push(error);
162+
}
163+
}
164+
try {
117165
await closeServer(context.server);
166+
} catch (error) {
167+
errors.push(error);
118168
}
169+
if (errors.length === 1) throw errors[0];
170+
if (errors.length > 1) throw new AggregateError(errors, 'web shutdown smoke cleanup failed');
119171
}
120172

121173
function readDaemonPid(stateDir: string): number {
@@ -127,7 +179,7 @@ function readDaemonPid(stateDir: string): number {
127179
}
128180

129181
async function settleManagedBrowserProcesses(
130-
status: Parameters<typeof inspectManagedAgentBrowserProcesses>[0],
182+
status: AgentBrowserToolStatus,
131183
): Promise<AgentBrowserProcessSummary> {
132184
const deadline = Date.now() + WEB_SHUTDOWN_SETTLE_TIMEOUT_MS;
133185
let summary = await inspectManagedAgentBrowserProcesses(status);
@@ -174,7 +226,9 @@ async function assertWebViewport(context: WebSmokeContext): Promise<void> {
174226
});
175227
}
176228

177-
async function createWebSmokeContext(): Promise<WebSmokeContext> {
229+
async function createWebSmokeContext(
230+
options: { agentBrowserIdleTimeoutMs?: string } = {},
231+
): Promise<WebSmokeContext> {
178232
const artifactDir = createArtifactDir();
179233
const stateDir = path.join(artifactDir, 'agent-device-state');
180234
const agentBrowserConfigPath = path.join(artifactDir, 'agent-browser.json');
@@ -185,7 +239,9 @@ async function createWebSmokeContext(): Promise<WebSmokeContext> {
185239
AGENT_DEVICE_STATE_DIR: stateDir,
186240
AGENT_BROWSER_CONFIG: agentBrowserConfigPath,
187241
AGENT_BROWSER_HEADED: 'false',
188-
AGENT_BROWSER_IDLE_TIMEOUT_MS: '30000',
242+
...(options.agentBrowserIdleTimeoutMs === undefined
243+
? {}
244+
: { AGENT_BROWSER_IDLE_TIMEOUT_MS: options.agentBrowserIdleTimeoutMs }),
189245
};
190246

191247
mkdirSync(stateDir, { recursive: true });

0 commit comments

Comments
 (0)