Skip to content

Commit a6749eb

Browse files
author
Eva
committed
fix(workspaces): preserve gallery reservation ownership
1 parent e33b5eb commit a6749eb

6 files changed

Lines changed: 169 additions & 290 deletions

File tree

‎docs/.generated/plugin-sdk-api-baseline.sha256‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
8b66b318a1c92ddca44cc95063acc233293e9d9b17342af5d5a366630e9266b7 plugin-sdk-api-baseline.json
2-
0a0f6c3e0c3b32ee04a70a62131eab6ea9ad03f4a6614903ba96a959e2e77f76 plugin-sdk-api-baseline.jsonl
1+
f39468d6dba6c9e748ae212720545717267fe164fcea76b8fea418081913f016 plugin-sdk-api-baseline.json
2+
e5c1abdd6c9ec3588dc904d87b7035ba3eba46b2b0957727c8fb1d0f044a4409 plugin-sdk-api-baseline.jsonl

‎extensions/workspaces/src/gallery.test.ts‎

Lines changed: 120 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import fs from "node:fs/promises";
22
import os from "node:os";
33
import path from "node:path";
4+
import { __setFsSafeTestHooksForTest } from "@openclaw/fs-safe/test-hooks";
45
import {
56
resolvePinnedHostnameWithPolicy,
67
resolveSsrFPolicyForUrl,
78
type LookupFn,
89
} from "openclaw/plugin-sdk/ssrf-runtime";
9-
import { describe, expect, it, vi } from "vitest";
10+
import { afterEach, describe, expect, it, vi } from "vitest";
1011
import {
1112
fetchWorkspaceGallery,
1213
installWorkspaceGalleryWidget,
@@ -73,6 +74,10 @@ async function withTempStateDir<T>(run: (stateDir: string) => Promise<T>): Promi
7374
}
7475
}
7576

77+
afterEach(() => {
78+
__setFsSafeTestHooksForTest(undefined);
79+
});
80+
7681
describe("parseWorkspaceGalleryConfig", () => {
7782
it("accepts only explicit HTTPS origins", () => {
7883
expect(
@@ -281,7 +286,7 @@ describe("installWorkspaceGalleryWidget", () => {
281286
}
282287
});
283288

284-
it("reserves the final directory exclusively before exposing the complete tree", async () => {
289+
it("keeps partial files out of the registry until the complete tree is present", async () => {
285290
await withTempStateDir(async (stateDir) => {
286291
const store = new WorkspaceStore({ stateDir });
287292
const files = Object.fromEntries(
@@ -296,8 +301,7 @@ describe("installWorkspaceGalleryWidget", () => {
296301
);
297302
const widgetDir = resolveWidgetDir("weather", stateDir);
298303
let settled = false;
299-
let observedReservation = false;
300-
let observedPartial = false;
304+
let observedUnregistered = false;
301305

302306
const install = installWorkspaceGalleryWidget(
303307
"https://gallery.example/widgets/weather.json",
@@ -315,32 +319,94 @@ describe("installWorkspaceGalleryWidget", () => {
315319
if (settled) {
316320
break;
317321
}
318-
try {
322+
const registryEntry = store.read().widgetsRegistry.weather;
323+
if (registryEntry === undefined) {
324+
observedUnregistered = true;
325+
} else {
319326
const observed = await fs.readdir(widgetDir, { recursive: true });
320-
if (observed.length === 0) {
321-
observedReservation = true;
322-
await expect(fs.mkdir(widgetDir)).rejects.toMatchObject({ code: "EEXIST" });
323-
} else if (!expectedFiles.every((file) => observed.includes(file))) {
324-
observedPartial = true;
325-
break;
326-
}
327-
} catch (error) {
328-
if ((error as NodeJS.ErrnoException).code !== "ENOENT") {
329-
throw error;
330-
}
327+
expect(expectedFiles.every((file) => observed.includes(file))).toBe(true);
331328
}
332329
await new Promise<void>((resolve) => {
333330
setImmediate(resolve);
334331
});
335332
}
336333
await install;
337334

338-
expect(observedReservation).toBe(true);
339-
expect(observedPartial).toBe(false);
335+
expect(observedUnregistered).toBe(true);
340336
await expect(fs.readdir(widgetDir)).resolves.toEqual(expect.arrayContaining(expectedFiles));
341337
});
342338
});
343339

340+
it("does not replace a target directory created during reservation", async () => {
341+
await withTempStateDir(async (stateDir) => {
342+
const store = new WorkspaceStore({ stateDir });
343+
const widgetDir = resolveWidgetDir("weather", stateDir);
344+
let competitorInode: number | bigint | undefined;
345+
__setFsSafeTestHooksForTest({
346+
beforeOpen: async (targetPath) => {
347+
if (
348+
competitorInode !== undefined ||
349+
!path.basename(targetPath).startsWith(".openclaw-gallery-reservation-")
350+
) {
351+
return;
352+
}
353+
await fs.mkdir(widgetDir);
354+
competitorInode = (await fs.stat(widgetDir, { bigint: true })).ino;
355+
},
356+
});
357+
const fetchGuard = vi.fn<WorkspaceGalleryFetch>(async (options) =>
358+
guardedResponse(jsonResponse(bundle()), options.url),
359+
);
360+
361+
await installWorkspaceGalleryWidget("https://gallery.example/widgets/weather.json", {
362+
allowedOrigins: ["https://gallery.example"],
363+
fetchGuard,
364+
stateDir,
365+
store,
366+
actor: "user",
367+
});
368+
369+
expect(competitorInode).toBeDefined();
370+
expect((await fs.stat(widgetDir, { bigint: true })).ino).toBe(competitorInode);
371+
expect(store.read().widgetsRegistry.weather?.status).toBe("pending");
372+
});
373+
});
374+
375+
it("never cleans up a competitor file that wins create-new", async () => {
376+
await withTempStateDir(async (stateDir) => {
377+
const store = new WorkspaceStore({ stateDir });
378+
const widgetDir = resolveWidgetDir("weather", stateDir);
379+
const competitorPath = path.join(widgetDir, "index.html");
380+
let competitorCreated = false;
381+
__setFsSafeTestHooksForTest({
382+
beforeOpen: async (targetPath) => {
383+
if (competitorCreated || path.basename(targetPath) !== "index.html") {
384+
return;
385+
}
386+
competitorCreated = true;
387+
await fs.writeFile(competitorPath, "competitor", { flag: "wx" });
388+
},
389+
});
390+
const fetchGuard = vi.fn<WorkspaceGalleryFetch>(async (options) =>
391+
guardedResponse(jsonResponse(bundle()), options.url),
392+
);
393+
394+
await expect(
395+
installWorkspaceGalleryWidget("https://gallery.example/widgets/weather.json", {
396+
allowedOrigins: ["https://gallery.example"],
397+
fetchGuard,
398+
stateDir,
399+
store,
400+
actor: "user",
401+
}),
402+
).rejects.toThrow(/exist/i);
403+
404+
expect(competitorCreated).toBe(true);
405+
await expect(fs.readFile(competitorPath, "utf8")).resolves.toBe("competitor");
406+
expect(store.read().widgetsRegistry.weather).toBeUndefined();
407+
});
408+
});
409+
344410
it("serializes gallery installation with scaffolding for the same widget name", async () => {
345411
await withTempStateDir(async (stateDir) => {
346412
const store = new WorkspaceStore({ stateDir });
@@ -398,14 +464,42 @@ describe("installWorkspaceGalleryWidget", () => {
398464
expect(results.filter((result) => result.status === "fulfilled")).toHaveLength(1);
399465
expect(results.filter((result) => result.status === "rejected")).toHaveLength(1);
400466
const files = (await fs.readdir(resolveWidgetDir("weather", stateDir))).toSorted();
467+
const reservationFiles = files.filter((file) =>
468+
file.startsWith(".openclaw-gallery-reservation-"),
469+
);
401470
expect([
402471
["index.html", "widget.json"],
403472
["README.md", "index.html", "widget.json"],
404-
]).toContainEqual(files);
473+
]).toContainEqual(files.filter((file) => !file.startsWith(".openclaw-gallery-reservation-")));
474+
expect(reservationFiles).toHaveLength(results[0]?.status === "fulfilled" ? 1 : 0);
405475
});
406476
});
407477

408-
it("removes staging directories after a nested file write fails", async () => {
478+
it("retains a successful reservation marker instead of deleting an ambiguous pathname", async () => {
479+
await withTempStateDir(async (stateDir) => {
480+
const store = new WorkspaceStore({ stateDir });
481+
const fetchGuard = vi.fn<WorkspaceGalleryFetch>(async (options) =>
482+
guardedResponse(jsonResponse(bundle()), options.url),
483+
);
484+
485+
await expect(
486+
installWorkspaceGalleryWidget("https://gallery.example/widgets/weather.json", {
487+
allowedOrigins: ["https://gallery.example"],
488+
fetchGuard,
489+
stateDir,
490+
store,
491+
actor: "user",
492+
}),
493+
).resolves.toMatchObject({ registry: { status: "pending" } });
494+
495+
const widgetEntries = await fs.readdir(resolveWidgetDir("weather", stateDir));
496+
expect(
497+
widgetEntries.filter((entry) => entry.startsWith(".openclaw-gallery-reservation-")),
498+
).toHaveLength(1);
499+
});
500+
});
501+
502+
it("leaves an unregistered reservation after an ambiguous partial write failure", async () => {
409503
await withTempStateDir(async (stateDir) => {
410504
const store = new WorkspaceStore({ stateDir });
411505
const tooLongName = `${"x".repeat(256)}.js`;
@@ -432,7 +526,12 @@ describe("installWorkspaceGalleryWidget", () => {
432526
}),
433527
).rejects.toThrow();
434528

435-
await expect(fs.readdir(path.join(stateDir, "workspaces", "widgets"))).resolves.toEqual([]);
529+
const widgetEntries = await fs.readdir(resolveWidgetDir("weather", stateDir));
530+
expect(
531+
widgetEntries.some((entry) => entry.startsWith(".openclaw-gallery-reservation-")),
532+
).toBe(true);
533+
expect(widgetEntries).toContain("index.html");
534+
expect(widgetEntries).not.toContain("widget.json");
436535
expect(store.read().widgetsRegistry.weather).toBeUndefined();
437536
});
438537
});

‎extensions/workspaces/src/gallery.ts‎

Lines changed: 42 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import {
99
resolveWidgetDir,
1010
validateWidgetManifest,
1111
WIDGET_CONTENT_TYPES,
12-
type WidgetManifest,
1312
} from "./manifest.js";
1413
import type { WorkspaceActor, WorkspaceWidgetRegistryEntry } from "./schema.js";
1514
import type { WorkspaceStore } from "./store.js";
@@ -283,7 +282,11 @@ export async function fetchWorkspaceGallery(
283282
);
284283
}
285284

286-
type NormalizedBundle = { name: string; manifest: WidgetManifest; files: Map<string, string> };
285+
type NormalizedBundle = {
286+
name: string;
287+
manifest: ReturnType<typeof validateWidgetManifest>;
288+
files: Map<string, string>;
289+
};
287290

288291
function validateBundle(value: unknown): NormalizedBundle {
289292
if (!isRecord(value)) {
@@ -352,88 +355,57 @@ async function installBundle(
352355
const root = await fsRoot(stateDir, { mkdir: true, mode: 0o600, symlinks: "reject" });
353356
const widgetsRoot = path.posix.join("workspaces", "widgets");
354357
const targetDir = path.posix.join(widgetsRoot, bundle.name);
355-
const stagedDir = path.posix.join(widgetsRoot, `.gallery-install-${randomUUID()}`);
358+
const reservationPath = path.posix.join(
359+
targetDir,
360+
`.openclaw-gallery-reservation-${randomUUID()}`,
361+
);
356362
await root.mkdir(widgetsRoot);
357363
const files = new Map(bundle.files);
358364
files.set("widget.json", `${JSON.stringify(bundle.manifest, null, 2)}\n`);
359-
const installedPaths = [...files.keys()].map((logicalPath) =>
360-
path.posix.join(stagedDir, logicalPath),
361-
);
362365
const result = await withWidgetInstallLock(bundle.name, stateDir, async () => {
363-
let cleanupDir: string | null = null;
364-
let reservationOwned = false;
365-
try {
366-
try {
367-
await root.mkdirExclusive(targetDir, { mode: 0o700 });
368-
reservationOwned = true;
369-
} catch (error) {
370-
if (["already-exists", "EEXIST"].includes((error as NodeJS.ErrnoException).code ?? "")) {
371-
throw new Error(`workspace widget already exists: ${bundle.name}`, { cause: error });
372-
}
373-
throw error;
374-
}
375-
376-
// Reserve the final name before staging. Peers see EEXIST while the
377-
// completed tree remains hidden under its random staging name.
378-
await root.mkdir(stagedDir);
379-
cleanupDir = stagedDir;
380-
for (const [logicalPath, content] of files) {
381-
const relativePath = path.posix.join(stagedDir, logicalPath);
382-
await root.create(relativePath, content, { mkdir: true, mode: 0o600 });
383-
}
384-
await root.move(stagedDir, targetDir, { overwrite: true });
385-
reservationOwned = false;
386-
cleanupDir = targetDir;
366+
await assertWidgetTargetMissing(root, targetDir, bundle.name);
387367

388-
const mutation = options.store.mutate(
389-
(draft) => {
390-
if (draft.widgetsRegistry[bundle.name]) {
391-
throw new Error(`workspace widget already exists: ${bundle.name}`);
392-
}
393-
draft.widgetsRegistry[bundle.name] = { status: "pending", createdBy: options.actor };
394-
},
395-
{ actor: options.actor },
396-
);
397-
return mutation.doc.widgetsRegistry[bundle.name]!;
398-
} catch (error) {
399-
if (cleanupDir) {
400-
await removeInstalledTree(root, cleanupDir, installedPaths, stagedDir).catch(() => {});
401-
}
402-
if (reservationOwned) {
403-
await root.remove(targetDir).catch(() => {});
404-
}
405-
throw error;
368+
// Gallery installs and scaffolds share a cross-process per-widget lock.
369+
// The create-new marker owns the target without replacing a directory that
370+
// appears after the absence check. The registry remains unchanged until
371+
// every create-new bundle file is present, so partial trees cannot render.
372+
// Keep the marker permanently as a non-servable install receipt. Deleting
373+
// its pathname after creation would be unsafe: a direct-filesystem writer
374+
// could replace it first, causing Gallery to delete content it does not own.
375+
// Failed installs likewise remain unregistered with their marker in place.
376+
await root.create(reservationPath, bundle.name, { mkdir: true, mode: 0o600 });
377+
for (const [logicalPath, content] of files) {
378+
const relativePath = path.posix.join(targetDir, logicalPath);
379+
await root.create(relativePath, content, { mkdir: true, mode: 0o600 });
406380
}
381+
const mutation = options.store.mutate(
382+
(draft) => {
383+
if (draft.widgetsRegistry[bundle.name]) {
384+
throw new Error(`workspace widget already exists: ${bundle.name}`);
385+
}
386+
draft.widgetsRegistry[bundle.name] = { status: "pending", createdBy: options.actor };
387+
},
388+
{ actor: options.actor },
389+
);
390+
return mutation.doc.widgetsRegistry[bundle.name]!;
407391
});
408392
return result;
409393
}
410394

411-
async function removeInstalledTree(
395+
async function assertWidgetTargetMissing(
412396
root: GalleryRoot,
413-
cleanupDir: string,
414-
writtenPaths: readonly string[],
415-
stagedDir: string,
397+
targetDir: string,
398+
name: string,
416399
): Promise<void> {
417-
const paths = writtenPaths.map((entry) =>
418-
cleanupDir === stagedDir
419-
? entry
420-
: path.posix.join(cleanupDir, path.posix.relative(stagedDir, entry)),
421-
);
422-
const directories = new Set<string>();
423-
for (const filePath of paths) {
424-
await root.remove(filePath).catch(() => {});
425-
for (
426-
let parent = path.posix.dirname(filePath);
427-
parent !== cleanupDir;
428-
parent = path.posix.dirname(parent)
429-
) {
430-
directories.add(parent);
400+
try {
401+
await root.stat(targetDir);
402+
} catch (error) {
403+
if (["not-found", "ENOENT"].includes((error as NodeJS.ErrnoException).code ?? "")) {
404+
return;
431405
}
406+
throw error;
432407
}
433-
for (const directory of [...directories].toSorted((left, right) => right.length - left.length)) {
434-
await root.remove(directory).catch(() => {});
435-
}
436-
await root.remove(cleanupDir);
408+
throw new Error(`workspace widget already exists: ${name}`);
437409
}
438410

439411
export async function installWorkspaceGalleryWidget(

0 commit comments

Comments
 (0)