* fix(browser): hot-reload profiles added after gateway start (#4841) * style: format files with oxfmt * Fix hot-reload stale config fields bug in forProfile * Fix test order-dependency in hot-reload profiles test * Fix mock reset order to prevent stale cfgProfiles * Fix config cache blocking hot-reload by clearing cache before loadConfig * test: improve hot-reload test to properly exercise config cache - Add simulated cache behavior in mock - Prime cache before mutating config - Verify stale value without clearConfigCache - Verify fresh value after hot-reload Addresses review comment about test not exercising cache * test: add hot-reload tests for browser profiles in server context. * fix(browser): optimize profile hot-reload to avoid global cache clear * fix(browser): remove unused loadConfig import * fix(test): execute resetModules before test setup * feat: implement browser server context with profile hot-reloading and tab management. * fix(browser): harden profile hot-reload and shutdown cleanup * test(browser): use toSorted in known-profile names test --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
@@ -3,7 +3,11 @@ import { createSubsystemLogger } from "../logging/subsystem.js";
|
||||
import { resolveBrowserConfig, resolveProfile } from "./config.js";
|
||||
import { ensureBrowserControlAuth } from "./control-auth.js";
|
||||
import { ensureChromeExtensionRelayServer } from "./extension-relay.js";
|
||||
import { type BrowserServerState, createBrowserRouteContext } from "./server-context.js";
|
||||
import {
|
||||
type BrowserServerState,
|
||||
createBrowserRouteContext,
|
||||
listKnownProfileNames,
|
||||
} from "./server-context.js";
|
||||
|
||||
let state: BrowserServerState | null = null;
|
||||
const log = createSubsystemLogger("browser");
|
||||
@@ -16,6 +20,7 @@ export function getBrowserControlState(): BrowserServerState | null {
|
||||
export function createBrowserControlContext() {
|
||||
return createBrowserRouteContext({
|
||||
getState: () => state,
|
||||
refreshConfigFromDisk: true,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -71,10 +76,11 @@ export async function stopBrowserControlService(): Promise<void> {
|
||||
|
||||
const ctx = createBrowserRouteContext({
|
||||
getState: () => state,
|
||||
refreshConfigFromDisk: true,
|
||||
});
|
||||
|
||||
try {
|
||||
for (const name of Object.keys(current.resolved.profiles)) {
|
||||
for (const name of listKnownProfileNames(current)) {
|
||||
try {
|
||||
await ctx.forProfile(name).stopRunningBrowser();
|
||||
} catch {
|
||||
|
||||
214
src/browser/server-context.hot-reload-profiles.test.ts
Normal file
214
src/browser/server-context.hot-reload-profiles.test.ts
Normal file
@@ -0,0 +1,214 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
let cfgProfiles: Record<string, { cdpPort?: number; cdpUrl?: string; color?: string }> = {};
|
||||
|
||||
// Simulate module-level cache behavior
|
||||
let cachedConfig: ReturnType<typeof buildConfig> | null = null;
|
||||
|
||||
function buildConfig() {
|
||||
return {
|
||||
browser: {
|
||||
enabled: true,
|
||||
color: "#FF4500",
|
||||
headless: true,
|
||||
defaultProfile: "openclaw",
|
||||
profiles: { ...cfgProfiles },
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
vi.mock("../config/config.js", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("../config/config.js")>();
|
||||
return {
|
||||
...actual,
|
||||
createConfigIO: () => ({
|
||||
loadConfig: () => {
|
||||
// Always return fresh config for createConfigIO to simulate fresh disk read
|
||||
return buildConfig();
|
||||
},
|
||||
}),
|
||||
loadConfig: () => {
|
||||
// simulate stale loadConfig that doesn't see updates unless cache cleared
|
||||
if (!cachedConfig) {
|
||||
cachedConfig = buildConfig();
|
||||
}
|
||||
return cachedConfig;
|
||||
},
|
||||
clearConfigCache: vi.fn(() => {
|
||||
// Clear the simulated cache
|
||||
cachedConfig = null;
|
||||
}),
|
||||
writeConfigFile: vi.fn(async () => {}),
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("./chrome.js", () => ({
|
||||
isChromeCdpReady: vi.fn(async () => false),
|
||||
isChromeReachable: vi.fn(async () => false),
|
||||
launchOpenClawChrome: vi.fn(async () => {
|
||||
throw new Error("launch disabled");
|
||||
}),
|
||||
resolveOpenClawUserDataDir: vi.fn(() => "/tmp/openclaw"),
|
||||
stopOpenClawChrome: vi.fn(async () => {}),
|
||||
}));
|
||||
|
||||
vi.mock("./cdp.js", () => ({
|
||||
createTargetViaCdp: vi.fn(async () => {
|
||||
throw new Error("cdp disabled");
|
||||
}),
|
||||
normalizeCdpWsUrl: vi.fn((wsUrl: string) => wsUrl),
|
||||
snapshotAria: vi.fn(async () => ({ nodes: [] })),
|
||||
getHeadersWithAuth: vi.fn(() => ({})),
|
||||
appendCdpPath: vi.fn((cdpUrl: string, path: string) => `${cdpUrl}${path}`),
|
||||
}));
|
||||
|
||||
vi.mock("./pw-ai.js", () => ({
|
||||
closePlaywrightBrowserConnection: vi.fn(async () => {}),
|
||||
}));
|
||||
|
||||
vi.mock("../media/store.js", () => ({
|
||||
ensureMediaDir: vi.fn(async () => {}),
|
||||
saveMediaBuffer: vi.fn(async () => ({ path: "/tmp/fake.png" })),
|
||||
}));
|
||||
|
||||
describe("server-context hot-reload profiles", () => {
|
||||
beforeEach(() => {
|
||||
vi.resetModules();
|
||||
cfgProfiles = {
|
||||
openclaw: { cdpPort: 18800, color: "#FF4500" },
|
||||
};
|
||||
cachedConfig = null; // Clear simulated cache
|
||||
});
|
||||
|
||||
it("forProfile hot-reloads newly added profiles from config", async () => {
|
||||
// Start with only openclaw profile
|
||||
const { createBrowserRouteContext } = await import("./server-context.js");
|
||||
const { resolveBrowserConfig } = await import("./config.js");
|
||||
const { loadConfig } = await import("../config/config.js");
|
||||
|
||||
// 1. Prime the cache by calling loadConfig() first
|
||||
const cfg = loadConfig();
|
||||
const resolved = resolveBrowserConfig(cfg.browser, cfg);
|
||||
|
||||
// Verify cache is primed (without desktop)
|
||||
expect(cfg.browser.profiles.desktop).toBeUndefined();
|
||||
const state = {
|
||||
server: null,
|
||||
port: 18791,
|
||||
resolved,
|
||||
profiles: new Map(),
|
||||
};
|
||||
|
||||
const ctx = createBrowserRouteContext({
|
||||
getState: () => state,
|
||||
refreshConfigFromDisk: true,
|
||||
});
|
||||
|
||||
// Initially, "desktop" profile should not exist
|
||||
expect(() => ctx.forProfile("desktop")).toThrow(/not found/);
|
||||
|
||||
// 2. Simulate adding a new profile to config (like user editing openclaw.json)
|
||||
cfgProfiles.desktop = { cdpUrl: "http://127.0.0.1:9222", color: "#0066CC" };
|
||||
|
||||
// 3. Verify without clearConfigCache, loadConfig() still returns stale cached value
|
||||
const staleCfg = loadConfig();
|
||||
expect(staleCfg.browser.profiles.desktop).toBeUndefined(); // Cache is stale!
|
||||
|
||||
// 4. Now forProfile should hot-reload (calls createConfigIO().loadConfig() internally)
|
||||
// It should NOT clear the global cache
|
||||
const profileCtx = ctx.forProfile("desktop");
|
||||
expect(profileCtx.profile.name).toBe("desktop");
|
||||
expect(profileCtx.profile.cdpUrl).toBe("http://127.0.0.1:9222");
|
||||
|
||||
// 5. Verify the new profile was merged into the cached state
|
||||
expect(state.resolved.profiles.desktop).toBeDefined();
|
||||
|
||||
// 6. Verify GLOBAL cache was NOT cleared - subsequent simple loadConfig() still sees STALE value
|
||||
// This confirms the fix: we read fresh config for the specific profile lookup without flushing the global cache
|
||||
const stillStaleCfg = loadConfig();
|
||||
expect(stillStaleCfg.browser.profiles.desktop).toBeUndefined();
|
||||
|
||||
// Verify clearConfigCache was not called
|
||||
const { clearConfigCache } = await import("../config/config.js");
|
||||
expect(clearConfigCache).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("forProfile still throws for profiles that don't exist in fresh config", async () => {
|
||||
const { createBrowserRouteContext } = await import("./server-context.js");
|
||||
const { resolveBrowserConfig } = await import("./config.js");
|
||||
const { loadConfig } = await import("../config/config.js");
|
||||
|
||||
const cfg = loadConfig();
|
||||
const resolved = resolveBrowserConfig(cfg.browser, cfg);
|
||||
const state = {
|
||||
server: null,
|
||||
port: 18791,
|
||||
resolved,
|
||||
profiles: new Map(),
|
||||
};
|
||||
|
||||
const ctx = createBrowserRouteContext({
|
||||
getState: () => state,
|
||||
refreshConfigFromDisk: true,
|
||||
});
|
||||
|
||||
// Profile that doesn't exist anywhere should still throw
|
||||
expect(() => ctx.forProfile("nonexistent")).toThrow(/not found/);
|
||||
});
|
||||
|
||||
it("forProfile refreshes existing profile config after loadConfig cache updates", async () => {
|
||||
const { createBrowserRouteContext } = await import("./server-context.js");
|
||||
const { resolveBrowserConfig } = await import("./config.js");
|
||||
const { loadConfig } = await import("../config/config.js");
|
||||
|
||||
const cfg = loadConfig();
|
||||
const resolved = resolveBrowserConfig(cfg.browser, cfg);
|
||||
const state = {
|
||||
server: null,
|
||||
port: 18791,
|
||||
resolved,
|
||||
profiles: new Map(),
|
||||
};
|
||||
|
||||
const ctx = createBrowserRouteContext({
|
||||
getState: () => state,
|
||||
refreshConfigFromDisk: true,
|
||||
});
|
||||
|
||||
const before = ctx.forProfile("openclaw");
|
||||
expect(before.profile.cdpPort).toBe(18800);
|
||||
|
||||
cfgProfiles.openclaw = { cdpPort: 19999, color: "#FF4500" };
|
||||
cachedConfig = null;
|
||||
|
||||
const after = ctx.forProfile("openclaw");
|
||||
expect(after.profile.cdpPort).toBe(19999);
|
||||
expect(state.resolved.profiles.openclaw?.cdpPort).toBe(19999);
|
||||
});
|
||||
|
||||
it("listProfiles refreshes config before enumerating profiles", async () => {
|
||||
const { createBrowserRouteContext } = await import("./server-context.js");
|
||||
const { resolveBrowserConfig } = await import("./config.js");
|
||||
const { loadConfig } = await import("../config/config.js");
|
||||
|
||||
const cfg = loadConfig();
|
||||
const resolved = resolveBrowserConfig(cfg.browser, cfg);
|
||||
const state = {
|
||||
server: null,
|
||||
port: 18791,
|
||||
resolved,
|
||||
profiles: new Map(),
|
||||
};
|
||||
|
||||
const ctx = createBrowserRouteContext({
|
||||
getState: () => state,
|
||||
refreshConfigFromDisk: true,
|
||||
});
|
||||
|
||||
cfgProfiles.desktop = { cdpPort: 19999, color: "#0066CC" };
|
||||
cachedConfig = null;
|
||||
|
||||
const profiles = await ctx.listProfiles();
|
||||
expect(profiles.some((p) => p.name === "desktop")).toBe(true);
|
||||
});
|
||||
});
|
||||
40
src/browser/server-context.list-known-profile-names.test.ts
Normal file
40
src/browser/server-context.list-known-profile-names.test.ts
Normal file
@@ -0,0 +1,40 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import type { BrowserServerState } from "./server-context.js";
|
||||
import { resolveBrowserConfig, resolveProfile } from "./config.js";
|
||||
import { listKnownProfileNames } from "./server-context.js";
|
||||
|
||||
describe("browser server-context listKnownProfileNames", () => {
|
||||
it("includes configured and runtime-only profile names", () => {
|
||||
const resolved = resolveBrowserConfig({
|
||||
defaultProfile: "openclaw",
|
||||
profiles: {
|
||||
openclaw: { cdpPort: 18800, color: "#FF4500" },
|
||||
},
|
||||
});
|
||||
const openclaw = resolveProfile(resolved, "openclaw");
|
||||
if (!openclaw) {
|
||||
throw new Error("expected openclaw profile");
|
||||
}
|
||||
|
||||
const state: BrowserServerState = {
|
||||
server: null as unknown as BrowserServerState["server"],
|
||||
port: 18791,
|
||||
resolved,
|
||||
profiles: new Map([
|
||||
[
|
||||
"stale-removed",
|
||||
{
|
||||
profile: { ...openclaw, name: "stale-removed" },
|
||||
running: null,
|
||||
},
|
||||
],
|
||||
]),
|
||||
};
|
||||
|
||||
expect(listKnownProfileNames(state).toSorted()).toEqual([
|
||||
"chrome",
|
||||
"openclaw",
|
||||
"stale-removed",
|
||||
]);
|
||||
});
|
||||
});
|
||||
@@ -2,6 +2,7 @@ import fs from "node:fs";
|
||||
import type { ResolvedBrowserProfile } from "./config.js";
|
||||
import type { PwAiModule } from "./pw-ai-module.js";
|
||||
import type {
|
||||
BrowserServerState,
|
||||
BrowserRouteContext,
|
||||
BrowserTab,
|
||||
ContextOptions,
|
||||
@@ -9,6 +10,7 @@ import type {
|
||||
ProfileRuntimeState,
|
||||
ProfileStatus,
|
||||
} from "./server-context.types.js";
|
||||
import { createConfigIO, loadConfig } from "../config/config.js";
|
||||
import { appendCdpPath, createTargetViaCdp, getHeadersWithAuth, normalizeCdpWsUrl } from "./cdp.js";
|
||||
import {
|
||||
isChromeCdpReady,
|
||||
@@ -17,7 +19,7 @@ import {
|
||||
resolveOpenClawUserDataDir,
|
||||
stopOpenClawChrome,
|
||||
} from "./chrome.js";
|
||||
import { resolveProfile } from "./config.js";
|
||||
import { resolveBrowserConfig, resolveProfile } from "./config.js";
|
||||
import {
|
||||
ensureChromeExtensionRelayServer,
|
||||
stopChromeExtensionRelayServer,
|
||||
@@ -35,6 +37,14 @@ export type {
|
||||
ProfileStatus,
|
||||
} from "./server-context.types.js";
|
||||
|
||||
export function listKnownProfileNames(state: BrowserServerState): string[] {
|
||||
const names = new Set(Object.keys(state.resolved.profiles));
|
||||
for (const name of state.profiles.keys()) {
|
||||
names.add(name);
|
||||
}
|
||||
return [...names];
|
||||
}
|
||||
|
||||
/**
|
||||
* Normalize a CDP WebSocket URL to use the correct base URL.
|
||||
*/
|
||||
@@ -559,6 +569,8 @@ function createProfileContext(
|
||||
}
|
||||
|
||||
export function createBrowserRouteContext(opts: ContextOptions): BrowserRouteContext {
|
||||
const refreshConfigFromDisk = opts.refreshConfigFromDisk === true;
|
||||
|
||||
const state = () => {
|
||||
const current = opts.getState();
|
||||
if (!current) {
|
||||
@@ -567,10 +579,53 @@ export function createBrowserRouteContext(opts: ContextOptions): BrowserRouteCon
|
||||
return current;
|
||||
};
|
||||
|
||||
const applyResolvedConfig = (
|
||||
current: BrowserServerState,
|
||||
freshResolved: BrowserServerState["resolved"],
|
||||
) => {
|
||||
current.resolved = freshResolved;
|
||||
for (const [name, runtime] of current.profiles) {
|
||||
const nextProfile = resolveProfile(freshResolved, name);
|
||||
if (nextProfile) {
|
||||
runtime.profile = nextProfile;
|
||||
continue;
|
||||
}
|
||||
if (!runtime.running) {
|
||||
current.profiles.delete(name);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
const refreshResolvedConfig = (current: BrowserServerState) => {
|
||||
if (!refreshConfigFromDisk) {
|
||||
return;
|
||||
}
|
||||
const cfg = loadConfig();
|
||||
const freshResolved = resolveBrowserConfig(cfg.browser, cfg);
|
||||
applyResolvedConfig(current, freshResolved);
|
||||
};
|
||||
|
||||
const refreshResolvedConfigFresh = (current: BrowserServerState) => {
|
||||
if (!refreshConfigFromDisk) {
|
||||
return;
|
||||
}
|
||||
const freshCfg = createConfigIO().loadConfig();
|
||||
const freshResolved = resolveBrowserConfig(freshCfg.browser, freshCfg);
|
||||
applyResolvedConfig(current, freshResolved);
|
||||
};
|
||||
|
||||
const forProfile = (profileName?: string): ProfileContext => {
|
||||
const current = state();
|
||||
refreshResolvedConfig(current);
|
||||
const name = profileName ?? current.resolved.defaultProfile;
|
||||
const profile = resolveProfile(current.resolved, name);
|
||||
let profile = resolveProfile(current.resolved, name);
|
||||
|
||||
// Hot-reload: try fresh config if profile not found
|
||||
if (!profile) {
|
||||
refreshResolvedConfigFresh(current);
|
||||
profile = resolveProfile(current.resolved, name);
|
||||
}
|
||||
|
||||
if (!profile) {
|
||||
const available = Object.keys(current.resolved.profiles).join(", ");
|
||||
throw new Error(`Profile "${name}" not found. Available profiles: ${available || "(none)"}`);
|
||||
@@ -580,6 +635,7 @@ export function createBrowserRouteContext(opts: ContextOptions): BrowserRouteCon
|
||||
|
||||
const listProfiles = async (): Promise<ProfileStatus[]> => {
|
||||
const current = state();
|
||||
refreshResolvedConfig(current);
|
||||
const result: ProfileStatus[] = [];
|
||||
|
||||
for (const name of Object.keys(current.resolved.profiles)) {
|
||||
|
||||
@@ -72,4 +72,5 @@ export type ProfileStatus = {
|
||||
export type ContextOptions = {
|
||||
getState: () => BrowserServerState | null;
|
||||
onEnsureAttachTarget?: (profile: ResolvedBrowserProfile) => Promise<void>;
|
||||
refreshConfigFromDisk?: boolean;
|
||||
};
|
||||
|
||||
@@ -9,7 +9,11 @@ import { ensureBrowserControlAuth, resolveBrowserControlAuth } from "./control-a
|
||||
import { ensureChromeExtensionRelayServer } from "./extension-relay.js";
|
||||
import { isPwAiLoaded } from "./pw-ai-state.js";
|
||||
import { registerBrowserRoutes } from "./routes/index.js";
|
||||
import { type BrowserServerState, createBrowserRouteContext } from "./server-context.js";
|
||||
import {
|
||||
type BrowserServerState,
|
||||
createBrowserRouteContext,
|
||||
listKnownProfileNames,
|
||||
} from "./server-context.js";
|
||||
|
||||
let state: BrowserServerState | null = null;
|
||||
const log = createSubsystemLogger("browser");
|
||||
@@ -125,6 +129,7 @@ export async function startBrowserControlServerFromConfig(): Promise<BrowserServ
|
||||
|
||||
const ctx = createBrowserRouteContext({
|
||||
getState: () => state,
|
||||
refreshConfigFromDisk: true,
|
||||
});
|
||||
registerBrowserRoutes(app as unknown as BrowserRouteRegistrar, ctx);
|
||||
|
||||
@@ -173,12 +178,13 @@ export async function stopBrowserControlServer(): Promise<void> {
|
||||
|
||||
const ctx = createBrowserRouteContext({
|
||||
getState: () => state,
|
||||
refreshConfigFromDisk: true,
|
||||
});
|
||||
|
||||
try {
|
||||
const current = state;
|
||||
if (current) {
|
||||
for (const name of Object.keys(current.resolved.profiles)) {
|
||||
for (const name of listKnownProfileNames(current)) {
|
||||
try {
|
||||
await ctx.forProfile(name).stopRunningBrowser();
|
||||
} catch {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
export {
|
||||
clearConfigCache,
|
||||
createConfigIO,
|
||||
loadConfig,
|
||||
parseConfigJson5,
|
||||
|
||||
@@ -820,7 +820,7 @@ function shouldUseConfigCache(env: NodeJS.ProcessEnv): boolean {
|
||||
return resolveConfigCacheMs(env) > 0;
|
||||
}
|
||||
|
||||
function clearConfigCache(): void {
|
||||
export function clearConfigCache(): void {
|
||||
configCache = null;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user