mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-03 17:51:58 +01:00
fix(files): preserve saved reads during allocation cleanup (#18701)
## Thinking Path > - Paperclip saves remote agent work in committed workspace snapshots. > - Files can read that save after the writer finishes. > - Compute cleanup can clear the recorded location while a saved preview loads. > - The current fence treats that physical cleanup as a change to the saved bytes. > - Permit only that one-way location removal for an already verified saved copy. > - Keep all logical-authority checks and live-read fences. ## Linked Issues or Issue Description Refs: #18677. This fixes the remaining in-flight cleanup race after saved reads began preferring the current committed version. A Files preview can return `422 workspace_changed` when allocation cleanup clears the location during archive loading. The epoch, committed snapshot, source, clean state, and absent writer can all remain unchanged. The extracted copy remains valid. An original QA denial recorded this error eight milliseconds after location cleanup. Its later successful read does not regrade the failed attempt. The original audit does not reconstruct the full earlier authority record. A controlled production-composed regression isolates the same cleanup transition. ## What Changed - Allow the recorded location to become null during a verified saved read while every other parsed authority field stays identical. - Remember observed removal so a reintroduced or foreign location still rejects the read. - Keep live reads, dirty state, active writers, changed epochs, snapshots, and sources fenced. - Add a real `createApp` content-route regression with PostgreSQL, encrypted archive storage, and private-copy cleanup. - Add negative controls for logical changes, foreign and reintroduced locations, and live location removal. - Update developer guidance and the workspace feature/access maps. ## Verification - Red: the production `createApp` route returned exactly `422 workspace_changed` when normal archive loading overlapped location removal. - Green: all 108 Files route and authorization tests passed. The preview returns the saved bytes, and its private extraction is removed. - All 46 opt-in workspace lifecycle tests passed on one owned local PostgreSQL database with local filesystem transport. The database and server were cleaned up. - `pnpm -r typecheck`, `pnpm build`, `node scripts/check-maps.mjs`, and `git diff --check` passed. - [Current-head CI](https://github.com/paperclipai/paperclip/actions/runs/41550908692) completed successfully on `45517b474b6866de2de6c618b7ff2193c93038ac` (attempt 2). All PR gates are clean: 55 successes, 3 skips, and 1 neutral review result. The full suite ran in CI; it was not duplicated as a long local full-suite invocation. - [Original CI failure](https://github.com/paperclipai/paperclip/actions/runs/41550908692/job/125638034035) is retained: an unchanged mobile frame-sampling check collected 5 frames where more than 5 are required; all sampled geometry was identical. The exact unchanged local Chrome case passed, then one failed-job-only retry passed on the same source. No assertion or time budget changed. - Fresh exact-head Greptile review is 5/5 with no findings, inline comments, or unresolved threads. The separate Paperclip static review is also 5/5 with no actionable findings. - No deployed acceptance is claimed by these local tests. The original failed QA attempt remains failed. ## Risks This permits one specific transition for an immutable saved copy: the original recorded location can disappear. The company/workspace query, epoch, snapshot, source, dirty state, writer, and location-reintroduction checks remain. A live read still rejects physical location changes. The change does not release compute, change snapshot admission, or alter extraction limits. No schema or API-shape change is required. ## Model Used OpenAI GPT-6 through Codex with reasoning and code execution. The runtime does not expose a more specific model identifier or context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
16643a6e03
commit
5955dd4eb4
5 files changed
+127
-5
No files matched your search
+5
-2
@@ -1732,8 +1732,11 @@ Files reads use the current committed saved version when remote authority is
|
||||
clean and has no active writer, even while its old allocation remains recorded.
|
||||
The saved loader verifies the company, workspace, snapshot, and archive before
|
||||
reading. Dirty or writer-owned authority uses the live allocation and cannot
|
||||
substitute an older save if that allocation is unavailable. Reads retain their
|
||||
authority-change fence and remove private saved-file copies after use.
|
||||
substitute an older save if that allocation is unavailable. A saved read permits
|
||||
cleanup to clear its recorded allocation while the epoch, snapshot, source,
|
||||
clean state, and absent writer stay identical. A new or reintroduced allocation
|
||||
still invalidates that read. Live reads retain the full allocation fence, and
|
||||
private saved-file copies are removed after use.
|
||||
Embedders that omit the optional saved-file loader keep valid live reads. They
|
||||
still report unavailability when neither a live location nor a loader exists.
|
||||
|
||||
|
||||
@@ -298,7 +298,9 @@ authorized live allocation or a verified committed saved version. Returned
|
||||
`source.kind` is `live` or `saved`, with `savedAt` for a saved copy. An unavailable
|
||||
clean allocation does not prevent reads of its current committed save once
|
||||
writer authority is clear; saved reads validate the exact company, workspace,
|
||||
and snapshot even while a remote location remains recorded. An unavailable
|
||||
and snapshot even while a remote location remains recorded. Cleanup may clear
|
||||
only that location during a saved read; logical-authority changes and foreign
|
||||
or reintroduced locations still reject it. Live reads retain their full fence. An unavailable
|
||||
dirty allocation cannot silently fall back to stale saved bytes. Remote paths
|
||||
never become controller filesystem authority. Confined path/symlink checks,
|
||||
bounded search/preview, download streaming, and cancellation remain enforced by
|
||||
|
||||
@@ -106,6 +106,10 @@ After a final save clears dirty state and writer authority, Files must read the
|
||||
current committed saved version even while the old allocation is still recorded
|
||||
or its read transport is unavailable. Confirm the original bytes and Saved copy
|
||||
label; a new writer or dirty state must retain the live-read requirement.
|
||||
Clear the recorded allocation while a saved preview is loading: the pinned copy
|
||||
must remain readable. Epoch, snapshot, source, writer, dirty-state, and foreign
|
||||
or reintroduced location changes must still reject the read. Live reads keep
|
||||
their physical-location fence, and all private saved copies must be removed.
|
||||
Close must retain dirty/unfinished work and reject custom cleanup/teardown
|
||||
commands; a clean saved workspace can release compute while retaining files.
|
||||
Manual runtime commands, jobs, repairs, and service starts must reject remote
|
||||
|
||||
@@ -345,6 +345,50 @@ describeDatabase("remote workspace ordinary file routes", () => {
|
||||
return { ...f, storage, saved, input: { companyId: f.ids.company, workspaceId: f.ids.workspace, snapshotId: snapshot.id } };
|
||||
}
|
||||
|
||||
it("keeps a production saved preview available when cleanup clears its location during archive loading", async () => {
|
||||
const f = await savedFileFixture(true);
|
||||
const requestTemp = await fs.mkdtemp(path.join(os.tmpdir(), "saved-file-location-request-"));
|
||||
directories.push(requestTemp);
|
||||
const { createApp } = await import("../app.js");
|
||||
const app = await createApp(db, { uiMode: "none", serverPort: 3100, storageService: f.storage,
|
||||
deploymentMode: "local_trusted", deploymentExposure: "public", bindHost: "127.0.0.1",
|
||||
allowedHostnames: ["127.0.0.1", "localhost"], authReady: false, companyDeletionEnabled: false,
|
||||
managedPluginAutoInstall: [], decisionServiceOptions: { wakeOriginAgent: async () => null } });
|
||||
const originalTmpdir = process.env.TMPDIR;
|
||||
process.env.TMPDIR = requestTemp;
|
||||
let extractedCopy: string | undefined;
|
||||
const getObject = f.storage.getObject.bind(f.storage);
|
||||
const retrieve = vi.spyOn(f.storage, "getObject").mockImplementation(async (...args) => {
|
||||
const object = await getObject(...args);
|
||||
const copies = (await fs.readdir(requestTemp)).filter(name => name.startsWith("pc-workspace-files-"));
|
||||
expect(copies).toHaveLength(1);
|
||||
extractedCopy = path.join(requestTemp, copies[0]!);
|
||||
// The committed bytes and writer epoch remain unchanged. Only the
|
||||
// allocation cleanup recorded by production has finished meanwhile.
|
||||
await db.update(executionWorkspaces).set({ metadata: { remoteWorkspace: {
|
||||
...f.authority, snapshotId: f.saved.id, location: null,
|
||||
} } }).where(eq(executionWorkspaces.id, f.ids.workspace));
|
||||
await db.update(environmentLeases).set({ status: "expired", releasedAt: new Date() })
|
||||
.where(eq(environmentLeases.id, f.ids.lease));
|
||||
return object;
|
||||
});
|
||||
try {
|
||||
await app.locals.bundledPluginsStartup;
|
||||
const response = await request(app).get(`/api/issues/${f.ids.issue}/file-resources/content`)
|
||||
.query({ workspace: "execution", path: "docs/result.md" });
|
||||
expect({ status: response.status, code: response.body.details?.code }).toEqual({ status: 200, code: undefined });
|
||||
expect(response.body).toMatchObject({ resource: { source: { kind: "saved", snapshotId: f.saved.id } },
|
||||
content: { encoding: "utf8", data: "Remote useful result\n" } });
|
||||
expect(retrieve).toHaveBeenCalledTimes(1);
|
||||
expect(extractedCopy).toBeDefined();
|
||||
await expect(fs.stat(extractedCopy!)).rejects.toMatchObject({ code: "ENOENT" });
|
||||
} finally {
|
||||
retrieve.mockRestore();
|
||||
try { await app.locals.paperclipShutdown(); }
|
||||
finally { if (originalTmpdir === undefined) delete process.env.TMPDIR; else process.env.TMPDIR = originalTmpdir; }
|
||||
}
|
||||
}, 60_000);
|
||||
|
||||
it("serves committed saved bytes through ordinary routes while the live allocation is unavailable", async () => {
|
||||
const f = await savedFileFixture(true);
|
||||
f.execute.mockRejectedValue(new Error("synthetic live allocation unavailable"));
|
||||
@@ -447,6 +491,64 @@ describeDatabase("remote workspace ordinary file routes", () => {
|
||||
expect(f.execute).not.toHaveBeenCalled(); expect(f.syncOut).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each(["epoch", "snapshot", "dirty", "writer", "source", "foreign_location"] as const)(
|
||||
"still fences a saved read when %s changes during extraction", async changed => {
|
||||
const f = await savedFileFixture(true), load = createSavedWorkspaceLoader(db, f.storage);
|
||||
let savedRoot: string | undefined;
|
||||
const loadSavedWorkspace = async (input: Parameters<typeof load>[0]) => {
|
||||
const saved = await load(input); savedRoot = saved.root;
|
||||
const next: RemoteWorkspaceAuthority = { ...f.authority, snapshotId: f.saved.id, location: null };
|
||||
if (changed === "epoch") next.epoch++;
|
||||
if (changed === "snapshot") next.snapshotId = randomUUID();
|
||||
if (changed === "dirty") next.dirty = true;
|
||||
if (changed === "writer") next.writerRunId = randomUUID();
|
||||
if (changed === "source") next.source = { kind: "git", repoUrl: "https://example.test/other.git", ref: null };
|
||||
if (changed === "foreign_location") next.location = { ...f.authority.location!, leaseId: randomUUID() };
|
||||
await db.update(executionWorkspaces).set({ metadata: { remoteWorkspace: next } }).where(eq(executionWorkspaces.id, f.ids.workspace));
|
||||
return saved;
|
||||
};
|
||||
const remoteFiles = createRemoteWorkspaceFiles(db, { loadSavedWorkspace, environmentRuntime: { execute: f.execute, syncOut: f.syncOut } });
|
||||
const response = await request(filesApp(f.ids.company, remoteFiles)).get(`/api/issues/${f.ids.issue}/file-resources/content`)
|
||||
.query({ workspace: "execution", path: "docs/result.md" }).expect(422);
|
||||
expect(response.body.details.code).toBe("workspace_changed"); expect(response.body).not.toHaveProperty("content");
|
||||
expect(savedRoot).toBeDefined(); await expect(fs.stat(savedRoot!)).rejects.toMatchObject({ code: "ENOENT" });
|
||||
expect(f.execute).not.toHaveBeenCalled(); expect(f.syncOut).not.toHaveBeenCalled();
|
||||
},
|
||||
);
|
||||
|
||||
it.each([true, false])("rejects a location reappearing on an open saved copy (initial location=%s)", async keepLocation => {
|
||||
const f = await savedFileFixture(keepLocation);
|
||||
const remoteFiles = createRemoteWorkspaceFiles(db, { loadSavedWorkspace: createSavedWorkspaceLoader(db, f.storage) });
|
||||
const access = await remoteFiles.open({ companyId: f.ids.company, workspaceId: f.ids.workspace });
|
||||
expect(access?.source.kind).toBe("saved");
|
||||
try {
|
||||
await db.update(executionWorkspaces).set({ metadata: { remoteWorkspace: { ...f.authority, snapshotId: f.saved.id, location: null } } })
|
||||
.where(eq(executionWorkspaces.id, f.ids.workspace));
|
||||
expect((await access!.stat("docs/result.md")).kind).toBe("file");
|
||||
await db.update(executionWorkspaces).set({ metadata: { remoteWorkspace: { ...f.authority, snapshotId: f.saved.id } } })
|
||||
.where(eq(executionWorkspaces.id, f.ids.workspace));
|
||||
await expect(access!.stat("docs/result.md")).rejects.toMatchObject({ status: 422, details: { code: "workspace_changed" } });
|
||||
} finally { await access?.close(); }
|
||||
});
|
||||
|
||||
it("keeps a live read fenced when its physical location is cleared", async () => {
|
||||
const f = await savedFileFixture(true), live = { ...f.authority, snapshotId: f.saved.id, dirty: true };
|
||||
await db.update(executionWorkspaces).set({ metadata: { remoteWorkspace: live } }).where(eq(executionWorkspaces.id, f.ids.workspace));
|
||||
const execute = f.execute.getMockImplementation()!;
|
||||
f.execute.mockImplementationOnce(async input => {
|
||||
const result = await execute(input);
|
||||
await db.update(executionWorkspaces).set({ metadata: { remoteWorkspace: { ...live, location: null } } })
|
||||
.where(eq(executionWorkspaces.id, f.ids.workspace));
|
||||
return result;
|
||||
});
|
||||
const loadSavedWorkspace = vi.fn(createSavedWorkspaceLoader(db, f.storage));
|
||||
const remoteFiles = createRemoteWorkspaceFiles(db, { loadSavedWorkspace, environmentRuntime: { execute: f.execute, syncOut: f.syncOut } });
|
||||
const response = await request(filesApp(f.ids.company, remoteFiles)).get(`/api/issues/${f.ids.issue}/file-resources/content`)
|
||||
.query({ workspace: "execution", path: "docs/result.md" }).expect(422);
|
||||
expect(response.body.details.code).toBe("workspace_changed"); expect(response.body).not.toHaveProperty("content");
|
||||
expect(f.execute).toHaveBeenCalledTimes(1); expect(loadSavedWorkspace).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("shares fail-fast saved Files admission across loader instances until owned cleanup completes", async () => {
|
||||
const f = await savedFileFixture(), availableBytes = async () => 8n * 1024n ** 3n;
|
||||
const first = createSavedWorkspaceLoader(db, f.storage, { availableBytes });
|
||||
|
||||
@@ -269,16 +269,27 @@ export function createRemoteWorkspaceFiles(db: Db, options: RemoteWorkspaceFileO
|
||||
const authority = readRemoteWorkspaceAuthority(workspace.metadata);
|
||||
if (!authority) return null;
|
||||
const initialAuthority = JSON.stringify(authority);
|
||||
let saved: SavedWorkspaceFiles | undefined;
|
||||
let savedLocationReleased = false;
|
||||
const checkCurrent = async () => {
|
||||
input.signal?.throwIfAborted();
|
||||
const [current] = await db.select({ metadata: executionWorkspaces.metadata }).from(executionWorkspaces).where(and(
|
||||
eq(executionWorkspaces.companyId, input.companyId), eq(executionWorkspaces.id, input.workspaceId),
|
||||
)).limit(1);
|
||||
if (!current || JSON.stringify(readRemoteWorkspaceAuthority(current.metadata)) !== initialAuthority) {
|
||||
const currentAuthority = current ? readRemoteWorkspaceAuthority(current.metadata) : null;
|
||||
// An extracted committed save no longer depends on the allocation.
|
||||
// Allow only its monotonic location removal; every logical field stays
|
||||
// pinned, and a foreign or reintroduced location remains a conflict.
|
||||
if (saved && authority.location && currentAuthority?.location === null
|
||||
&& JSON.stringify({ ...currentAuthority, location: authority.location }) === initialAuthority) {
|
||||
savedLocationReleased = true;
|
||||
return;
|
||||
}
|
||||
if (!currentAuthority || JSON.stringify(currentAuthority) !== initialAuthority
|
||||
|| savedLocationReleased && currentAuthority.location !== null) {
|
||||
throw fileFailure("workspace_changed");
|
||||
}
|
||||
};
|
||||
let saved: SavedWorkspaceFiles | undefined;
|
||||
let environment: Environment | undefined;
|
||||
let lease: EnvironmentLease | undefined;
|
||||
let source: WorkspaceFileSource;
|
||||
|
||||
Reference in new issue
Block a user