mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-19 01:46:15 +02:00
perf(adapter-utils): skip host scans for unchanged restores (#16312)
Skip the host snapshot scan when an unchanged workspace return matches its baseline, while retaining writer locking, both authority hooks, and changed-export conflict handling. Verified with 270 affected tests, final-head CI/E2E, Superagent, and Apex 5/5. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
51b56706fb
commit
82884443be
5 files changed
+150
No files matched your search
@@ -22,6 +22,13 @@ bounded timeout diagnostics. Missing, stale, or incorrect metadata cannot grant
|
||||
or retain ownership. PID reuse, PID namespaces, and wall-clock changes do not
|
||||
decide whether a writer owns the lock.
|
||||
|
||||
An unchanged returned snapshot still acquires the target writer lock and runs
|
||||
both authorization/publication hooks. If every returned path, type, file hash,
|
||||
file mode and symlink target matches the baseline, there is no incoming delta.
|
||||
The merge then skips the host snapshot scan and leaves concurrent host edits
|
||||
untouched. Changed exports retain the full conflict preflight. Checkpoint and
|
||||
archive validation still run before the merge.
|
||||
|
||||
**Never delete, replace, or move a `.lock.sqlite` file while an instance can
|
||||
write to it.** Its stable inode is part of the locking contract. Replacing it
|
||||
could let two writers lock different files for the same target. Files remain
|
||||
|
||||
@@ -75,6 +75,12 @@ then primary source, creation time and ID. A missing or foreign configured sourc
|
||||
does not authorize controller hooks. This precedence neither changes existing
|
||||
creation validation nor rotates bindings or clears retained copy-only markers.
|
||||
Closed-worktree reopening checks current task, agent, organizational and source policies without assuming a future driver. Effective low-trust reopens suppress configured and built-in host scripts. Returned checkpoints must have directory parents for every nested entry before merging, including unchanged entries; invalid output leaves retained host files intact and keeps recovery evidence.
|
||||
|
||||
Skipping the host scan for a fully unchanged returned snapshot grants no new
|
||||
access. The merge still holds its canonical writer lock and runs both authority
|
||||
and publication hooks. Denials still propagate, and checkpoint/archive validation
|
||||
is unchanged. [Regression coverage](../../packages/adapter-utils/src/workspace-restore-unchanged.test.ts)
|
||||
is source-level evidence only.
|
||||
Child creation by an admitted agent checks an explicit prospective-task context before the child is saved. The creator run must have an authoritative saved policy and a verified company/agent/parent lineage. Current parent, prospective child and physical source policies must fit that boundary. This creation path does not relax exact task/run checks for existing-task writes. The [child trust regression](../../server/src/__tests__/task-workspace-child-trust.test.ts) is automated source-level evidence, not live agent qualification.
|
||||
Source review for these additions: PR #15749 and
|
||||
[task workspace routes](../../server/src/routes/task-workspaces.ts),
|
||||
|
||||
@@ -90,6 +90,12 @@ SSH keeps its explicit remote provision-command path; these checks
|
||||
do not claim a new sandbox provision transport.
|
||||
Low-trust closed-worktree reopening, including a board-comment reopen of a projectless task, must also suppress both kinds of host provision script. Returned checkpoints must reject file, symlink or missing parents before host writes, even if a nested file is marked unchanged. The retained host file and recovery evidence must survive rejection. These are automated safety contracts, not new live runtime acceptance.
|
||||
|
||||
An unchanged returned snapshot skips the host scan after acquiring the writer lock
|
||||
and checking authority. Concurrent host edits remain untouched; changed files,
|
||||
modes, symlinks and deletions still take the conflict path. The
|
||||
[unchanged restore regression](../../packages/adapter-utils/src/workspace-restore-unchanged.test.ts)
|
||||
checks both hooks under the lock. This is source-level coverage, not a cloud latency result.
|
||||
|
||||
For a child created by an admitted agent with an explicit existing workspace, verify that a matching sandbox boundary permits creation before the child has an ID. A changed child policy, unrelated parent, or forged creator run must fail. See the [child workspace trust regression](../../server/src/__tests__/task-workspace-child-trust.test.ts). This automated contract does not add live acceptance evidence.
|
||||
|
||||
### `workspace-management`
|
||||
|
||||
@@ -218,6 +218,14 @@ function entriesMatch(left: SnapshotEntry | null | undefined, right: SnapshotEnt
|
||||
return false;
|
||||
}
|
||||
|
||||
function directorySnapshotsMatch(left: DirectorySnapshot, right: DirectorySnapshot): boolean {
|
||||
if (left.entries.size !== right.entries.size) return false;
|
||||
for (const [relative, entry] of right.entries) {
|
||||
if (!entriesMatch(left.entries.get(relative), entry)) return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
const LOCK_WAIT_MS = 30_000;
|
||||
const LOCK_DIAGNOSTIC_READ_TIMEOUT_MS = 100;
|
||||
const activeDirectoryMergeLocks = new Set<string>();
|
||||
@@ -719,6 +727,10 @@ export async function mergeDirectoryWithBaseline(input: {
|
||||
try {
|
||||
await withDirectoryMergeLock(input.targetDir, async (canonicalTargetDir) => {
|
||||
await input.beforeApply?.();
|
||||
if (!input.snapshots?.current && directorySnapshotsMatch(input.baseline, source)) {
|
||||
await input.afterApply?.();
|
||||
return;
|
||||
}
|
||||
// Strict preflight must see excluded children before a directory is
|
||||
// replaced. The merge still applies only the filtered source/baseline.
|
||||
const current = input.snapshots?.current ?? await captureDirectorySnapshot(canonicalTargetDir,
|
||||
|
||||
@@ -0,0 +1,119 @@
|
||||
import { promises as fs } from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, expect, it, vi } from "vitest";
|
||||
import { captureDirectorySnapshot, disposeDirectorySnapshot, mergeDirectoryWithBaseline, withDirectoryMergeLock, WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE } from "./workspace-restore-merge.js";
|
||||
|
||||
const roots: string[] = [];
|
||||
afterEach(async () => {
|
||||
vi.restoreAllMocks();
|
||||
vi.unstubAllEnvs();
|
||||
for (const root of roots.splice(0)) await fs.rm(root, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
async function fixture(diskBacked: boolean) {
|
||||
const root = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-unchanged-restore-"));
|
||||
roots.push(root);
|
||||
vi.stubEnv("PAPERCLIP_HOME", path.join(root, "home"));
|
||||
const target = path.join(root, "target"), source = path.join(root, "source");
|
||||
await fs.mkdir(target); await fs.mkdir(source);
|
||||
await fs.writeFile(path.join(target, "file"), "baseline");
|
||||
await fs.writeFile(path.join(source, "file"), "baseline");
|
||||
const baseline = await captureDirectorySnapshot(target, { diskBacked, exclude: ["cache"] });
|
||||
const incoming = await captureDirectorySnapshot(source, { diskBacked, exclude: ["cache"] });
|
||||
return { root, target, source, baseline, incoming };
|
||||
}
|
||||
|
||||
it.each([
|
||||
{ diskBacked: false, conflictPolicy: undefined }, { diskBacked: true, conflictPolicy: undefined },
|
||||
{ diskBacked: false, conflictPolicy: "reject" as const }, { diskBacked: true, conflictPolicy: "reject" as const },
|
||||
])("does not scan host files for an unchanged $diskBacked snapshot with $conflictPolicy and preserves both fenced hooks", async ({ diskBacked, conflictPolicy }) => {
|
||||
const f = await fixture(diskBacked);
|
||||
await fs.writeFile(path.join(f.target, "file"), "concurrent host edit");
|
||||
await fs.mkdir(path.join(f.target, "cache"));
|
||||
await fs.writeFile(path.join(f.target, "cache", "private"), "host only");
|
||||
const read = vi.spyOn(fs, "opendir");
|
||||
read.mockImplementation(async () => { throw new Error("Unexpected host scan"); });
|
||||
const callbacks: string[] = [];
|
||||
const hook = async (name: string) => {
|
||||
callbacks.push(name);
|
||||
await expect(withDirectoryMergeLock(f.target, async () => { throw new Error("Lock was released"); }, undefined, undefined, 20))
|
||||
.rejects.toMatchObject({ code: WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE });
|
||||
};
|
||||
try {
|
||||
await mergeDirectoryWithBaseline({ baseline: f.baseline, sourceDir: f.source, targetDir: f.target,
|
||||
snapshots: { source: f.incoming }, conflictPolicy, beforeApply: () => hook("before"), afterApply: () => hook("after") });
|
||||
expect(read).not.toHaveBeenCalled();
|
||||
expect(callbacks).toEqual(["before", "after"]);
|
||||
expect(await fs.readFile(path.join(f.target, "file"), "utf8")).toBe("concurrent host edit");
|
||||
expect(await fs.readFile(path.join(f.target, "cache", "private"), "utf8")).toBe("host only");
|
||||
} finally { await disposeDirectorySnapshot(f.incoming); await disposeDirectorySnapshot(f.baseline); }
|
||||
});
|
||||
|
||||
it("skips the target scan when a captured full export is unchanged", async () => {
|
||||
const f = await fixture(false);
|
||||
const read = vi.spyOn(fs, "opendir");
|
||||
await mergeDirectoryWithBaseline({ baseline: f.baseline, sourceDir: f.source, targetDir: f.target, conflictPolicy: "reject" });
|
||||
expect(read.mock.calls.map(call => path.resolve(String(call[0])))).toEqual([path.resolve(f.source)]);
|
||||
});
|
||||
|
||||
it.each(["before", "after"])("propagates %s-hook denial for an unchanged checkpoint", async denied => {
|
||||
const f = await fixture(true);
|
||||
const error = new Error("Authority revoked");
|
||||
const before = vi.fn(async () => { if (denied === "before") throw error; });
|
||||
const after = vi.fn(async () => { if (denied === "after") throw error; });
|
||||
try {
|
||||
await expect(mergeDirectoryWithBaseline({ baseline: f.baseline, sourceDir: f.source, targetDir: f.target,
|
||||
snapshots: { source: f.incoming }, beforeApply: before, afterApply: after })).rejects.toBe(error);
|
||||
expect(before).toHaveBeenCalledOnce();
|
||||
expect(after).toHaveBeenCalledTimes(denied === "after" ? 1 : 0);
|
||||
expect(await fs.readFile(path.join(f.target, "file"), "utf8")).toBe("baseline");
|
||||
await withDirectoryMergeLock(f.target, async () => undefined);
|
||||
} finally { await disposeDirectorySnapshot(f.incoming); await disposeDirectorySnapshot(f.baseline); }
|
||||
});
|
||||
|
||||
async function assertIncomingChange(change: string) {
|
||||
const f = await fixture(true);
|
||||
await disposeDirectorySnapshot(f.incoming);
|
||||
if (change === "hash") await fs.writeFile(path.join(f.source, "file"), "changed");
|
||||
if (change === "mode") await fs.chmod(path.join(f.source, "file"), 0o755);
|
||||
if (change === "kind") { await fs.rm(path.join(f.source, "file")); await fs.mkdir(path.join(f.source, "file")); }
|
||||
if (change === "rename") await fs.rename(path.join(f.source, "file"), path.join(f.source, "renamed"));
|
||||
if (change === "add") await fs.writeFile(path.join(f.source, "added"), "new");
|
||||
if (change === "delete") await fs.rm(path.join(f.source, "file"));
|
||||
const incoming = await captureDirectorySnapshot(f.source, { diskBacked: true, exclude: ["cache"] });
|
||||
const read = vi.spyOn(fs, "opendir");
|
||||
try {
|
||||
await mergeDirectoryWithBaseline({ baseline: f.baseline, sourceDir: f.source, targetDir: f.target,
|
||||
snapshots: { source: incoming }, conflictPolicy: "reject" });
|
||||
expect(read).toHaveBeenCalled();
|
||||
if (change === "hash") expect(await fs.readFile(path.join(f.target, "file"), "utf8")).toBe("changed");
|
||||
if (change === "mode") expect((await fs.stat(path.join(f.target, "file"))).mode & 0o777).toBe(0o755);
|
||||
if (change === "kind") expect((await fs.stat(path.join(f.target, "file"))).isDirectory()).toBe(true);
|
||||
if (change === "rename") expect(await fs.readFile(path.join(f.target, "renamed"), "utf8")).toBe("baseline");
|
||||
if (change === "add") expect(await fs.readFile(path.join(f.target, "added"), "utf8")).toBe("new");
|
||||
if (change === "delete") await expect(fs.stat(path.join(f.target, "file"))).rejects.toMatchObject({ code: "ENOENT" });
|
||||
} finally { await disposeDirectorySnapshot(incoming); await disposeDirectorySnapshot(f.baseline); }
|
||||
}
|
||||
|
||||
it.each(["hash", "kind", "rename", "add", "delete"])("does not skip an incoming %s change", assertIncomingChange);
|
||||
it.skipIf(process.platform === "win32")("does not skip an incoming mode change", () => assertIncomingChange("mode"));
|
||||
|
||||
it.skipIf(process.platform === "win32")("does not skip a changed symlink target", async () => {
|
||||
const f = await fixture(false);
|
||||
await fs.rm(path.join(f.target, "file")); await fs.rm(path.join(f.source, "file"));
|
||||
await fs.symlink("old", path.join(f.target, "file")); await fs.symlink("new", path.join(f.source, "file"));
|
||||
const baseline = await captureDirectorySnapshot(f.target), incoming = await captureDirectorySnapshot(f.source);
|
||||
await mergeDirectoryWithBaseline({ baseline, sourceDir: f.source, targetDir: f.target, snapshots: { source: incoming }, conflictPolicy: "reject" });
|
||||
expect(await fs.readlink(path.join(f.target, "file"))).toBe("new");
|
||||
});
|
||||
|
||||
it("still rejects concurrent edits to a changed incoming file", async () => {
|
||||
const f = await fixture(false);
|
||||
await fs.writeFile(path.join(f.source, "file"), "remote change");
|
||||
await fs.writeFile(path.join(f.target, "file"), "host change");
|
||||
const incoming = await captureDirectorySnapshot(f.source);
|
||||
await expect(mergeDirectoryWithBaseline({ baseline: f.baseline, sourceDir: f.source, targetDir: f.target,
|
||||
snapshots: { source: incoming }, conflictPolicy: "reject" })).rejects.toMatchObject({ code: "DIRECTORY_MERGE_CONFLICT", paths: ["file"] });
|
||||
expect(await fs.readFile(path.join(f.target, "file"), "utf8")).toBe("host change");
|
||||
});
|
||||
Reference in new issue
Block a user