mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 02:32:07 +01:00
fix(server): finish workspace operations when the log write fails (#17415)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Before a run starts, Paperclip prepares the execution workspace (for example `git worktree add`). Each step is a workspace operation. The workspace operation recorder writes it as a row and as a log file on disk. > - When an operation fails, the recorder writes the error to the log file, then sets the row to `failed`. > - If that log write also fails (for example ENOSPC when the disk is full), the recorder throws before it updates the row. > - The row stays `running` with no `finishedAt`. The log write error also replaces the real error of the operation. > - This pull request makes the failure-path log write best-effort, so the row always reaches `failed` and callers get the correct error. > - The benefit: on a full disk, the operator sees a terminal operation with the real failure reason. Operations do not stay "running" for ever. ## Linked Issues or Issue Description No public issue exists. Bug description: **What happened?** A run failed during workspace setup on a host with a full disk (`ENOSPC: no space left on device, write`). The stack shows the error in the workspace operation log append inside `recordOperation`. In the failure path, the recorder calls the log append before it updates the row. Because the append threw again, the row stayed `running` with no `finishedAt`. When the operation itself failed (for example a git error), callers got the ENOSPC log error and not the git error. **Expected behavior** A failed workspace operation always reaches `failed` with `finishedAt` set. Callers get the operation's own error. A log write failure must not hide that error. **Steps to reproduce** 1. Make the operation log append throw ENOSPC (the new tests stub `fs.appendFile` for the log root). 2. Record an operation whose `run` throws, or whose output append throws. 3. Before this fix: the row stays `running`, and the caller gets the ENOSPC error and not the operation's error. **Paperclip version or commit** `ee29e28` (current `master`). **Agent adapter(s) involved** Not adapter-specific (core bug). Related open PRs with disk-full handling in other places (no overlap with this code path): #5882 (run-log rotation), #16773 (backup disk use). ## What Changed - `server/src/services/workspace-operations.ts`: in the `recordOperation` failure path, the stderr log append is now best-effort (`.catch(() => undefined)`). The stderr excerpt still records the message, because the excerpt updates before the file write. `logStore.finalize` was already best-effort in this path. - `server/src/__tests__/workspace-operations-reconciliation.test.ts`: two regression tests with log appends that fail with ENOSPC: - The output append fails after a successful run. The row reaches `failed` with `finishedAt` and the ENOSPC message in the stderr excerpt. The caller gets the ENOSPC error. - The operation itself throws. The row reaches `failed` and the caller gets the operation's own error, not the log write error. ## Verification - Red before the fix: both new tests failed on `master`. The row stayed `running`, and the caller got the ENOSPC error and not the git error. - Green after the fix: `pnpm exec vitest run src/__tests__/workspace-operations-reconciliation.test.ts src/__tests__/workspace-runtime-control-recovery.test.ts` passes (17 tests). - Sibling suites that use the recorder (26 files, 945 tests): all pass except 4 tests in `workspace-runtime.test.ts`. Those 4 tests also fail on unchanged `master` in the same sandbox, because they need the local pnpm and worktree CLI. This change does not cause them. - Typecheck: `tsc --noEmit` on `workspace-operations.ts` passes. The full server `tsc` was not run locally because the 4 GB sandbox ran out of memory. CI runs the full typecheck. ## Risks - Low risk. The change affects only the failure path, and only the log file write. The success path does not change: if a log write fails after a successful run, the operation still fails with the write error. - The log file for a failed operation can lose its last stderr line on a full disk. The row's stderr excerpt still has the message. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude Code with tool use (shell, file edit, test execution). ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Devin Foley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
2 files changed
+90
-2
No files matched your search
@@ -2,7 +2,7 @@ import { randomUUID } from "node:crypto";
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
companies,
|
||||
createDb,
|
||||
@@ -271,4 +271,89 @@ describeEmbeddedPostgres("workspace operation reconciliation", () => {
|
||||
"Workspace repair target_backup: succeeded.",
|
||||
);
|
||||
});
|
||||
|
||||
describe("when the operation log file cannot be written", () => {
|
||||
// The log file is created, then every append hits a full disk (PAPERCLIP-BACKEND-4H).
|
||||
function failLogAppends() {
|
||||
const enospc = Object.assign(new Error("ENOSPC: no space left on device, write"), {
|
||||
code: "ENOSPC",
|
||||
errno: -28,
|
||||
syscall: "write",
|
||||
});
|
||||
const appendFile = fs.appendFile;
|
||||
const spy = vi.spyOn(fs, "appendFile").mockImplementation(async (file, ...rest) => {
|
||||
if (typeof file === "string" && file.startsWith(logRoot)) throw enospc;
|
||||
return appendFile(file, ...rest);
|
||||
});
|
||||
return { enospc, spy };
|
||||
}
|
||||
|
||||
async function seedCompany() {
|
||||
const companyId = randomUUID();
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: "Workspace operation full disk",
|
||||
issuePrefix: `F${companyId.replace(/-/g, "").slice(0, 7).toUpperCase()}`,
|
||||
requireBoardApprovalForNewAgents: false,
|
||||
});
|
||||
return companyId;
|
||||
}
|
||||
|
||||
async function operationRows(companyId: string) {
|
||||
return db.select().from(workspaceOperations).where(eq(workspaceOperations.companyId, companyId));
|
||||
}
|
||||
|
||||
it("terminalizes the operation and surfaces the write error", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const { enospc, spy } = failLogAppends();
|
||||
try {
|
||||
await expect(
|
||||
workspaceOperationService(db)
|
||||
.createRecorder({ companyId })
|
||||
.recordOperation({
|
||||
phase: "worktree_prepare",
|
||||
command: "git worktree add",
|
||||
cwd: "/tmp/full-disk-workspace",
|
||||
run: async () => ({ status: "succeeded", exitCode: 0, stdout: "Preparing worktree\n" }),
|
||||
}),
|
||||
).rejects.toBe(enospc);
|
||||
} finally {
|
||||
spy.mockRestore();
|
||||
}
|
||||
|
||||
const rows = await operationRows(companyId);
|
||||
expect(rows).toHaveLength(1);
|
||||
expect(rows[0]).toMatchObject({ status: "failed", phase: "worktree_prepare" });
|
||||
expect(rows[0]?.finishedAt).toBeInstanceOf(Date);
|
||||
expect(rows[0]?.stderrExcerpt).toContain("ENOSPC: no space left on device, write");
|
||||
});
|
||||
|
||||
it("keeps the operation's own error instead of the log write error", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const { spy } = failLogAppends();
|
||||
const gitError = new Error("fatal: could not create work tree dir");
|
||||
try {
|
||||
await expect(
|
||||
workspaceOperationService(db)
|
||||
.createRecorder({ companyId })
|
||||
.recordOperation({
|
||||
phase: "worktree_prepare",
|
||||
command: "git worktree add",
|
||||
cwd: "/tmp/full-disk-workspace",
|
||||
run: async () => {
|
||||
throw gitError;
|
||||
},
|
||||
}),
|
||||
).rejects.toBe(gitError);
|
||||
} finally {
|
||||
spy.mockRestore();
|
||||
}
|
||||
|
||||
const rows = await operationRows(companyId);
|
||||
expect(rows).toHaveLength(1);
|
||||
expect(rows[0]).toMatchObject({ status: "failed" });
|
||||
expect(rows[0]?.finishedAt).toBeInstanceOf(Date);
|
||||
expect(rows[0]?.stderrExcerpt).toContain("fatal: could not create work tree dir");
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -654,7 +654,10 @@ export function workspaceOperationService(db: Db) {
|
||||
if (!row) throw notFound("Workspace operation not found");
|
||||
return toWorkspaceOperation(row);
|
||||
} catch (error) {
|
||||
await append("stderr", error instanceof Error ? error.message : String(error));
|
||||
// The log file is best-effort once the operation has failed: a log write error (for
|
||||
// example ENOSPC on a full disk) must neither strand this row in `running` nor replace
|
||||
// the operation's own error. The excerpt still records the message.
|
||||
await append("stderr", error instanceof Error ? error.message : String(error)).catch(() => undefined);
|
||||
const finalized = await logStore.finalize(handle).catch(() => null);
|
||||
const finishedAt = new Date();
|
||||
await db
|
||||
|
||||
Reference in new issue
Block a user