Skip to content

Commit ea2b558

Browse files
steipetesteipete
authored
fix(browser): prevent stale profile resurrection (#104601)
* fix(browser): serialize profile lifecycle * refactor(browser): tighten lifecycle cleanup * chore(browser): keep release note in PR * test(browser): keep temp fixtures inside plugin * test(browser): use preferred temp root
1 parent 986304e commit ea2b558

79 files changed

Lines changed: 7202 additions & 2069 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎extensions/browser/plugin-registration.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ function createLazyBrowserPluginService(): OpenClawPluginService {
184184
stop: async (ctx) => {
185185
if (!service) {
186186
const { stopBrowserControlService } = await import("./src/control-service.js");
187-
await stopBrowserControlService().catch(() => {});
187+
await stopBrowserControlService();
188188
return;
189189
}
190190
await service.stop?.(ctx);

‎extensions/browser/src/browser-control-state.test.ts‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
// Browser tests cover shared browser-control lifecycle serialization.
2+
import type { Server } from "node:http";
3+
import { beforeEach, describe, expect, it, vi } from "vitest";
4+
import { markBrowserRuntimeStopping } from "./browser/server-context.lifecycle.js";
5+
6+
const runtimeMocks = vi.hoisted(() => ({
7+
stopBrowserRuntime: vi.fn(),
8+
}));
9+
10+
vi.mock("./browser/runtime-lifecycle.js", () => ({
11+
createBrowserRuntimeState: (params: {
12+
server: Server | null;
13+
port: number;
14+
resolved: unknown;
15+
}) => ({
16+
server: params.server,
17+
port: params.port,
18+
resolved: params.resolved,
19+
profiles: new Map(),
20+
}),
21+
stopBrowserRuntime: runtimeMocks.stopBrowserRuntime,
22+
}));
23+
24+
const {
25+
ensureBrowserControlRuntime,
26+
getBrowserControlState,
27+
stopBrowserControlRuntime,
28+
withBrowserControlStart,
29+
} = await import("./browser-control-state.js");
30+
31+
const resolved = { profiles: {}, controlPort: 18_791 } as never;
32+
const onWarn = vi.fn();
33+
34+
function start(owner: "server" | "service", server: Server | null = null) {
35+
return withBrowserControlStart(() =>
36+
ensureBrowserControlRuntime({ server, port: 18_791, resolved, owner, onWarn }),
37+
);
38+
}
39+
40+
function stop(requestedBy: "server" | "service") {
41+
return stopBrowserControlRuntime({ requestedBy, onWarn });
42+
}
43+
44+
beforeEach(() => {
45+
runtimeMocks.stopBrowserRuntime.mockReset().mockImplementation(async (params) => {
46+
params.clearState();
47+
});
48+
});
49+
50+
describe("browser control lifecycle", () => {
51+
it("allows a start queued after a no-state stop", async () => {
52+
const stopping = stop("service");
53+
const starting = start("service");
54+
55+
await expect(stopping).resolves.toBeNull();
56+
await expect(starting).resolves.toBeTruthy();
57+
await stop("service");
58+
});
59+
60+
it("rejects a start requested after stop intent but before stop drains", async () => {
61+
await start("service");
62+
let releaseStop!: () => void;
63+
const stopGate = new Promise<void>((resolve) => {
64+
releaseStop = resolve;
65+
});
66+
runtimeMocks.stopBrowserRuntime.mockImplementationOnce(async (params) => {
67+
await stopGate;
68+
params.clearState();
69+
});
70+
71+
const stopping = stop("service");
72+
const starting = start("service");
73+
releaseStop();
74+
75+
await stopping;
76+
await expect(starting).rejects.toThrow("stopping");
77+
expect(getBrowserControlState()).toBeNull();
78+
});
79+
80+
it("retains a failed stop owner for an exact retry", async () => {
81+
await start("service");
82+
runtimeMocks.stopBrowserRuntime.mockImplementationOnce(async (params) => {
83+
markBrowserRuntimeStopping(params.current);
84+
throw new Error("cleanup failed");
85+
});
86+
87+
await expect(stop("service")).rejects.toThrow("cleanup failed");
88+
expect(getBrowserControlState()).toBeNull();
89+
90+
await expect(stop("service")).resolves.toBeTruthy();
91+
await expect(start("service")).resolves.toBeTruthy();
92+
await stop("service");
93+
});
94+
95+
it("lets a foreground server adopt service state without a second runtime", async () => {
96+
const serviceState = await start("service");
97+
const server = {} as Server;
98+
const serverState = await start("server", server);
99+
100+
expect(serverState).toBe(serviceState);
101+
expect(serverState.server).toBe(server);
102+
await stop("service");
103+
expect(runtimeMocks.stopBrowserRuntime).not.toHaveBeenCalled();
104+
105+
await stop("server");
106+
expect(runtimeMocks.stopBrowserRuntime).toHaveBeenCalledOnce();
107+
});
108+
109+
it("allows a start queued after a foreground-owned service stop", async () => {
110+
await start("service");
111+
await start("server", {} as Server);
112+
const stopping = stop("service");
113+
const starting = start("service");
114+
115+
await expect(stopping).resolves.toBeNull();
116+
await expect(starting).resolves.toBeTruthy();
117+
await stop("server");
118+
});
119+
120+
it("orders a queued stop after an in-progress cold start", async () => {
121+
let releaseStart!: () => void;
122+
const startGate = new Promise<void>((resolve) => {
123+
releaseStart = resolve;
124+
});
125+
const starting = withBrowserControlStart(async () => {
126+
await startGate;
127+
return await ensureBrowserControlRuntime({
128+
server: null,
129+
port: 18_791,
130+
resolved,
131+
owner: "service",
132+
onWarn,
133+
});
134+
});
135+
const stopping = stop("service");
136+
releaseStart();
137+
138+
await starting;
139+
await stopping;
140+
expect(runtimeMocks.stopBrowserRuntime).toHaveBeenCalledOnce();
141+
expect(getBrowserControlState()).toBeNull();
142+
});
143+
});

‎extensions/browser/src/browser-control-state.ts‎

Lines changed: 60 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,44 @@
55
* so local tools can attach to the same browser runtime without racing owners.
66
*/
77
import type { Server } from "node:http";
8+
import { BrowserProfileUnavailableError } from "./browser/errors.js";
89
import { createBrowserRuntimeState, stopBrowserRuntime } from "./browser/runtime-lifecycle.js";
910
import { type BrowserServerState, createBrowserRouteContext } from "./browser/server-context.js";
11+
import { isBrowserRuntimeRunning } from "./browser/server-context.lifecycle.js";
1012

1113
type BrowserControlOwner = "server" | "service";
1214

1315
let state: BrowserServerState | null = null;
1416
let owner: BrowserControlOwner | null = null;
17+
let lifecycleTail = Promise.resolve();
18+
let completedEffectiveStops = 0;
19+
20+
/** Serialize complete Browser runtime start/stop workflows. */
21+
function enqueueBrowserControlLifecycle<T>(run: () => Promise<T>): Promise<T> {
22+
const result = lifecycleTail.then(run, run);
23+
lifecycleTail = result.then(
24+
() => {},
25+
() => {},
26+
);
27+
return result;
28+
}
29+
30+
/** Queue startup, but never turn a request made during shutdown into a post-stop restart. */
31+
export function withBrowserControlStart<T>(run: () => Promise<T>): Promise<T> {
32+
const effectiveStopsAtRequest = completedEffectiveStops;
33+
return enqueueBrowserControlLifecycle(() => {
34+
if (
35+
completedEffectiveStops !== effectiveStopsAtRequest ||
36+
(state ? !isBrowserRuntimeRunning(state) : false)
37+
) {
38+
throw new BrowserProfileUnavailableError("Browser runtime is stopping.");
39+
}
40+
return run();
41+
});
42+
}
1543

1644
export function getBrowserControlState(): BrowserServerState | null {
17-
return state;
45+
return state && isBrowserRuntimeRunning(state) ? state : null;
1846
}
1947

2048
/** Create a route context bound to the current shared browser runtime. */
@@ -25,15 +53,17 @@ export function createBrowserControlContext() {
2553
});
2654
}
2755

28-
/** Start or attach the shared browser runtime for either the server or service owner. */
56+
/**
57+
* Start or attach the shared runtime. Call only from a queued `withBrowserControlStart` entry.
58+
*/
2959
export async function ensureBrowserControlRuntime(params: {
3060
server?: Server | null;
3161
port: number;
3262
resolved: BrowserServerState["resolved"];
3363
owner: BrowserControlOwner;
3464
onWarn: (message: string) => void;
3565
}): Promise<BrowserServerState> {
36-
if (state) {
66+
if (state && isBrowserRuntimeRunning(state)) {
3767
if (params.server) {
3868
// A foreground server takes ownership of the already-started service
3969
// runtime so shutdown and port reporting follow the visible server.
@@ -44,6 +74,9 @@ export async function ensureBrowserControlRuntime(params: {
4474
}
4575
return state;
4676
}
77+
if (state) {
78+
throw new BrowserProfileUnavailableError("Browser runtime cleanup must finish before restart.");
79+
}
4780

4881
state = await createBrowserRuntimeState({
4982
server: params.server ?? null,
@@ -56,28 +89,32 @@ export async function ensureBrowserControlRuntime(params: {
5689
}
5790

5891
/** Stop the shared browser runtime when the requesting owner is allowed to do so. */
59-
export async function stopBrowserControlRuntime(params: {
92+
export function stopBrowserControlRuntime(params: {
6093
requestedBy: BrowserControlOwner;
6194
closeServer?: boolean;
6295
onWarn: (message: string) => void;
63-
}): Promise<void> {
64-
const current = state;
65-
if (!current) {
66-
return;
67-
}
68-
if (params.requestedBy === "service" && current.server && owner === "server") {
69-
// The background service must not close a runtime currently claimed by the
70-
// visible HTTP server; otherwise CLI/browser calls lose their control port.
71-
return;
72-
}
73-
await stopBrowserRuntime({
74-
current,
75-
getState: () => state,
76-
clearState: () => {
77-
state = null;
78-
owner = null;
79-
},
80-
closeServer: params.closeServer,
81-
onWarn: params.onWarn,
96+
}): Promise<BrowserServerState | null> {
97+
return enqueueBrowserControlLifecycle(async () => {
98+
const current = state;
99+
if (!current) {
100+
return null;
101+
}
102+
if (params.requestedBy === "service" && current.server && owner === "server") {
103+
// The background service must not close a runtime currently claimed by the
104+
// visible HTTP server; otherwise CLI/browser calls lose their control port.
105+
return null;
106+
}
107+
await stopBrowserRuntime({
108+
current,
109+
getState: () => state,
110+
clearState: () => {
111+
state = null;
112+
owner = null;
113+
},
114+
closeServer: params.closeServer,
115+
onWarn: params.onWarn,
116+
});
117+
completedEffectiveStops += 1;
118+
return current;
82119
});
83120
}

‎extensions/browser/src/browser/bridge-server.auth.test.ts‎

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,21 @@
11
// Browser tests cover bridge server.auth plugin behavior.
2-
import { afterEach, describe, expect, it } from "vitest";
2+
import { afterEach, describe, expect, it, vi } from "vitest";
3+
import { getBridgeAuthForPort } from "./bridge-auth-registry.js";
34
import { startBrowserBridgeServer, stopBrowserBridgeServer } from "./bridge-server.js";
45
import type { ResolvedBrowserConfig } from "./config.js";
56
import {
67
DEFAULT_OPENCLAW_BROWSER_COLOR,
78
DEFAULT_OPENCLAW_BROWSER_PROFILE_NAME,
89
} from "./constants.js";
10+
import { isBrowserRuntimeRunning } from "./server-context.lifecycle.js";
11+
12+
function deferred() {
13+
let resolve!: () => void;
14+
const promise = new Promise<void>((done) => {
15+
resolve = done;
16+
});
17+
return { promise, resolve };
18+
}
919

1020
function buildResolvedConfig(): ResolvedBrowserConfig {
1121
return {
@@ -86,6 +96,74 @@ describe("startBrowserBridgeServer auth", () => {
8696
).rejects.toThrow(/requires auth/i);
8797
});
8898

99+
it("closes ingress but retains exact bridge cleanup state for retry", async () => {
100+
const bridge = await startBrowserBridgeServer({
101+
resolved: buildResolvedConfig(),
102+
authToken: "secret-token",
103+
skipRouteRegistrationForTest: true,
104+
});
105+
servers.push({ stop: () => stopBrowserBridgeServer(bridge.server) });
106+
const close = vi
107+
.fn<() => Promise<void>>()
108+
.mockRejectedValueOnce(new Error("relay cleanup failed"))
109+
.mockResolvedValue(undefined);
110+
bridge.state.extensionRelays = new Map([
111+
[
112+
"openclaw",
113+
{
114+
port: 18_799,
115+
token: "relay-token",
116+
bridge: {},
117+
close,
118+
} as never,
119+
],
120+
]);
121+
expect(getBridgeAuthForPort(bridge.port)).toEqual({ token: "secret-token" });
122+
123+
const firstStop = stopBrowserBridgeServer(bridge.server);
124+
const concurrentStop = stopBrowserBridgeServer(bridge.server);
125+
expect(concurrentStop).toBe(firstStop);
126+
await expect(firstStop).rejects.toThrow("relay cleanup failed");
127+
await expect(concurrentStop).rejects.toThrow("relay cleanup failed");
128+
expect(bridge.server.listening).toBe(false);
129+
expect(getBridgeAuthForPort(bridge.port)).toBeUndefined();
130+
expect(close).toHaveBeenCalledOnce();
131+
132+
await expect(stopBrowserBridgeServer(bridge.server)).resolves.toBeUndefined();
133+
expect(close).toHaveBeenCalledTimes(2);
134+
});
135+
136+
it("invalidates an active request before waiting for HTTP close", async () => {
137+
const attachStarted = deferred();
138+
const releaseAttach = deferred();
139+
const bridge = await startBrowserBridgeServer({
140+
resolved: buildResolvedConfig(),
141+
authToken: "secret-token",
142+
onEnsureAttachTarget: async () => {
143+
attachStarted.resolve();
144+
await releaseAttach.promise;
145+
},
146+
});
147+
servers.push({ stop: () => stopBrowserBridgeServer(bridge.server) });
148+
149+
const startRequest = fetch(`${bridge.baseUrl}/start`, {
150+
method: "POST",
151+
headers: { Authorization: "Bearer secret-token" },
152+
});
153+
await attachStarted.promise;
154+
155+
const stopping = stopBrowserBridgeServer(bridge.server);
156+
try {
157+
expect(stopBrowserBridgeServer(bridge.server)).toBe(stopping);
158+
expect(bridge.server.listening).toBe(false);
159+
expect(isBrowserRuntimeRunning(bridge.state)).toBe(false);
160+
} finally {
161+
releaseAttach.resolve();
162+
}
163+
164+
await Promise.all([stopping, startRequest.catch(() => undefined)]);
165+
});
166+
89167
it("serves noVNC bootstrap html without leaking password in Location header", async () => {
90168
let resolveCalls = 0;
91169
const bridge = await startBrowserBridgeServer({

0 commit comments

Comments
 (0)