mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-27 19:35:13 +01:00
fix: Serialize ACPX session record saves per record to stop overlapping renames (EPERM on Windows) (#16925)
> [!NOTE] > Opened by Prospector's automated issue-fix pipeline. A maintainer reviews and decides; nothing here merges on its own. ## Thinking Path - Issue #15346 reports a defect in paperclipai/paperclip. - Root cause, as the fixing agent read it: ACPX saves one session record from several writers (live checkpoint timer, turn end, finalize, close) with no coordination; each save renames a temp file onto the same record file, and on Windows overlapping renames onto one file fail with EPERM. All of these saves go through Paperclip's session-store wrapper, which forwarded them concurrently. - This pull request: Serialize ACPX session record saves per record to stop overlapping renames (EPERM on Windows) ## What Changed - `packages/adapter-utils/src/acpx-engine/ephemeral-session-environment.ts`: Every ACPX save in Paperclip passes through this wrapper, so it now puts all saves of one record id in a single queue (a promise chain shared by all wrappers in the process). Saves of one record never overlap and keep their order; a failed save still returns its own error and does not block the next one; saves of different records still run in parallel. ## Verification - Reproducing test(s), added in this PR: `packages/adapter-utils/src/acpx-engine/ephemeral-session-environment.test.ts` - They fail on the base commit and pass with the fix, each twice in a fresh sandbox. - 3 independent attempts wrote 3 reproductions; this fix passes every one of them. - The repository's compile command passes with the fix applied. - 3 existing test file(s) near the change pass. - The full test suite passes except for the 246 file(s) that already fail on the base commit. ## Risks - Written by an automated pipeline, not a person: review it as you would an outside contribution. - Risk tier of the touched paths: 2. - The scope reviewer found it also changes, beyond the report: save() called when no earlier save of that record is pending - The scope reviewer found it also changes, beyond the report: persistedStore.save throwing synchronously - The scope reviewer found it also changes, beyond the report: Records with missing/undefined acpxRecordId - The scope reviewer found it also changes, beyond the report: save() for different record ids ## Model Used - opus, sonnet (Anthropic Claude, via Claude Code), writing and checking the change in an isolated sandbox. ## Linked Issues Fixes #15346 Other pull requests that name #15346: - #15370 (closed): fix(acpx): serialize session file saves and clean failed temporary writes ## 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) - [ ] 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) - [ ] 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 - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge <!-- prospector:issue-fix v1 issue=15346 base=1c07b5903b1b report=4fe3fefc8864e00f --> Co-authored-by: commitperclip[bot] <284812584+commitperclip[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
585d37d4f2
commit
2d72f2c4c0
2 files changed
+59
-1
No files matched your search
@@ -162,4 +162,45 @@ describe("ephemeral ACPX session environment", () => {
|
||||
const input = freeze(record()); await expect(wrapper.save(input)).rejects.toBe(failure);
|
||||
expect(input.acpx?.session_options?.env).toEqual(currentEnvironment);
|
||||
});
|
||||
|
||||
it("never overlaps two saves of one record, even across wrappers, and keeps their order", async () => {
|
||||
let active = 0, maxActive = 0;
|
||||
const order: string[] = [];
|
||||
const failure = new Error("fixture rename failure");
|
||||
const backing = stubStore(undefined);
|
||||
backing.save.mockImplementation(async (saved) => {
|
||||
active += 1; maxActive = Math.max(maxActive, active);
|
||||
await new Promise(resolve => setTimeout(resolve, 5));
|
||||
order.push(saved.lastUsedAt);
|
||||
active -= 1;
|
||||
if (saved.lastUsedAt === "checkpoint") throw failure;
|
||||
});
|
||||
const turnStore = createEphemeralSessionEnvironmentStore(backing, currentEnvironment);
|
||||
const closeStore = createEphemeralSessionEnvironmentStore(backing, currentEnvironment);
|
||||
const results = await Promise.allSettled([
|
||||
turnStore.save({ ...record(), lastUsedAt: "checkpoint" }),
|
||||
turnStore.save({ ...record(), lastUsedAt: "turn-end" }),
|
||||
closeStore.save({ ...record(), lastUsedAt: "close" }),
|
||||
]);
|
||||
expect(maxActive).toBe(1);
|
||||
expect(order).toEqual(["checkpoint", "turn-end", "close"]);
|
||||
expect(results.map(result => result.status)).toEqual(["rejected", "fulfilled", "fulfilled"]);
|
||||
expect((results[0] as PromiseRejectedResult).reason).toBe(failure);
|
||||
});
|
||||
|
||||
it("does not serialize saves of different records", async () => {
|
||||
let active = 0, maxActive = 0;
|
||||
const backing = stubStore(undefined);
|
||||
backing.save.mockImplementation(async () => {
|
||||
active += 1; maxActive = Math.max(maxActive, active);
|
||||
await new Promise(resolve => setTimeout(resolve, 5));
|
||||
active -= 1;
|
||||
});
|
||||
const wrapper = createEphemeralSessionEnvironmentStore(backing, currentEnvironment);
|
||||
await Promise.all([
|
||||
wrapper.save({ ...record(), acpxRecordId: "fixture-record-a" }),
|
||||
wrapper.save({ ...record(), acpxRecordId: "fixture-record-b" }),
|
||||
]);
|
||||
expect(maxActive).toBe(2);
|
||||
});
|
||||
});
|
||||
@@ -1,5 +1,22 @@
|
||||
import type { AcpSessionStore } from "acpx/runtime";
|
||||
|
||||
// ACPX saves one record from several writers (live checkpoint, turn end,
|
||||
// finalize, close) with no lock. Each save renames a temp file onto the record
|
||||
// file, and on Windows two overlapping renames onto one file fail with EPERM.
|
||||
// Chain every save of one record so they never overlap.
|
||||
const pendingSaves = new Map<string, Promise<void>>();
|
||||
|
||||
function saveInOrder(recordId: string, save: () => Promise<void>): Promise<void> {
|
||||
const previous = pendingSaves.get(recordId) ?? Promise.resolve();
|
||||
const current = previous.catch(() => undefined).then(save);
|
||||
const settled = current.catch(() => undefined);
|
||||
pendingSaves.set(recordId, settled);
|
||||
void settled.then(() => {
|
||||
if (pendingSaves.get(recordId) === settled) pendingSaves.delete(recordId);
|
||||
});
|
||||
return current;
|
||||
}
|
||||
|
||||
/** Keep the current run's environment in memory, outside persisted conversation state. */
|
||||
export function createEphemeralSessionEnvironmentStore(
|
||||
persistedStore: AcpSessionStore,
|
||||
@@ -26,7 +43,7 @@ export function createEphemeralSessionEnvironmentStore(
|
||||
const { env: _environment, ...sessionOptions } = record.acpx.session_options;
|
||||
persisted.acpx = { ...record.acpx, session_options: sessionOptions };
|
||||
}
|
||||
return persistedStore.save(persisted);
|
||||
return saveInOrder(record.acpxRecordId, () => persistedStore.save(persisted));
|
||||
},
|
||||
};
|
||||
}
|
||||
Reference in new issue
Block a user