mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
Add bounded evidence for directory lock timeouts (#14787)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agent files use directory locks during collection and cleanup. > - A lock timeout can fail finalization after the model turn completes. > - The timeout currently identifies no owner state or waiting operation. > - This pull request adds bounded evidence to the existing run failure report. > - Operators can distinguish a known local holder from a possible old lock without changing lock safety. ## Linked Issues or Issue Description **What happened?** A directory lock timeout does not distinguish active local work from an owner record left by an earlier process. The stored execution stage can also precede the cleanup operation that failed. **Expected behavior** The failure report should identify the waiting operation and expose bounded ownership clues. It must preserve the timeout and keep unknown ownership protected. **Steps to reproduce** Hold a directory merge lock while a second caller reaches its acquisition deadline. The regression tests exercise a live holder and an older owner record with a live PID. Related: #9667 proposes stale-lock recovery under a single-server assumption. This change only adds evidence and does not adopt that assumption. #14575 and #14665 add other run failure diagnostics. ## What Changed - Record lock owner state, capped age and wait duration, same-process and process-age comparisons, and whether this module holds the lock. - Label agent-directory release, collection, checkpoint, and warm handoff timeouts with a fixed operation code. - Validate each field before the existing event-local Sentry report accepts it. Exclude owner records, PIDs, paths, and absolute timestamps. - Limit the extra diagnostic owner read to 100 ms with best-effort abort; malformed JSON is `invalid` and unreadable owner records remain `unknown`. - Document the diagnostic limits and verify that contenders never reclaim protected locks. ## Verification - Focused lock, diagnostic, real Sentry SDK, and database-backed agent-directory tests: 126 passed, including stalled-read and malformed/missing/unreadable-owner regression coverage. - Final revision `0691613dcc`: all 54 reported checks successful, with two intentionally skipped Storybook checks. Greptile: 5/5, zero unresolved review threads; no merge conflicts. - `pnpm -r typecheck`: passed. - `pnpm build`: passed. - `pnpm test:run`: complete suite coverage ran with the existing repository shard flags: four general-server shards, four serialized shards, two general-workspaces-a shards, and general-workspaces-b. The full run is not green because of the base failures below. - The broad run found 13 failures in the unchanged macOS skill-cache tests. All 13 reproduce on the clean base revision. Open PR #14290 covers that existing failure. - Two unchanged CLI archive tests hit their five-second limits during the broad run; all 17 tests in that file pass on recheck. A CLI auth socket error also cleared on recheck (19 tests), and its full serialized shard passed on rerun. ## Risks This is a diagnostic change, not a stale-lock fix. Owner observations can race with release. Wall-clock shifts can affect the age comparison. A local-holder flag covers only this module instance. None of these fields authorizes reclamation or proves a file save. Lock acquisition, release, retries, task status, and recovery guards retain their current behavior. No schema change or deployment action is required. ## Model Used OpenAI Codex, based on GPT-6, with code execution and repository tools. The exact model build and context window were not exposed to this agent. ## 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 - [ ] I have run tests locally and they pass (focused checks pass; existing base failures are documented above) - [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
842efe0181
commit
4b9a6000f7
7 files changed
+296
-12
No files matched your search
@@ -129,7 +129,11 @@ describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK",
|
||||
code: "ECONNRESET", requestId: "request-123", status: 503,
|
||||
stack: "Error: upstream connection reset\n at socketRead (/app/provider.js:19:7)",
|
||||
response: { body: "private-response" },
|
||||
}) }), { stack: "Error: first run failed\n at originalAdapter (/app/adapter.js:42:7)", request: { headers: "private-headers" } });
|
||||
}) }), { stack: "Error: first run failed\n at originalAdapter (/app/adapter.js:42:7)", request: { headers: "private-headers" },
|
||||
code: "ERR_WORKSPACE_RESTORE_LOCK_TIMEOUT", workspaceRestoreLock: {
|
||||
operation: "agent_directory_release", ownerState: "alive", ownerSameProcess: true, ownerPredatesProcess: true,
|
||||
knownLocalHolder: false, ownerAgeMs: 120_000, waitMs: 30_001, path: "/private-lock-path",
|
||||
} });
|
||||
const diagnostics = sanitizeRunFailureDiagnostics(collectRunFailureDiagnostics({
|
||||
runtimeMode: "legacy", resultJson: { terminalSessionFailure: {
|
||||
category: "service", title: "Provider failed", details: "d".repeat(9000),
|
||||
@@ -175,6 +179,9 @@ describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK",
|
||||
expect(JSON.stringify(exceptions)).not.toContain("captureRunFailure");
|
||||
expect(JSON.stringify(captured(second.errorMessage)?.exception)).not.toContain("captureRunFailure");
|
||||
expect(firstCapture).toMatchObject({ contexts: {
|
||||
run_execution: { restoreLockOperation: "agent_directory_release", restoreLockOwnerState: "alive", restoreLockOwnerSameProcess: true,
|
||||
restoreLockOwnerPredatesProcess: true, restoreLockKnownLocalHolder: false,
|
||||
restoreLockOwnerAgeMs: 120_000, restoreLockWaitMs: 30_001 },
|
||||
provider_failure: { category: "service", details: "d".repeat(9000) },
|
||||
run_exception_1: { code: "ECONNRESET", requestId: "request-123", status: 503 },
|
||||
} });
|
||||
|
||||
@@ -7,6 +7,35 @@ const run = (overrides: Partial<Run> = {}) => ({ resultJson: null, ...overrides
|
||||
const collect = (error: unknown) => collectRunFailureDiagnostics(run(), { error });
|
||||
|
||||
describe("run failure diagnostics", () => {
|
||||
it("selects bounded lock-owner evidence from a caught timeout cause", () => {
|
||||
const error = new Error("outer", { cause: Object.assign(new Error("lock timeout"), {
|
||||
code: "ERR_WORKSPACE_RESTORE_LOCK_TIMEOUT",
|
||||
workspaceRestoreLock: { operation: "agent_directory_release", ownerState: "alive", ownerSameProcess: true,
|
||||
ownerPredatesProcess: true, knownLocalHolder: false, ownerAgeMs: 120_000, waitMs: 30_001,
|
||||
ownerPid: 123, path: "/sentinel-lock-path", owner: { payload: "sentinel-owner-payload" } },
|
||||
}) });
|
||||
const result = sanitizeRunFailureDiagnostics(collect(error));
|
||||
expect(result.execution).toEqual({ restoreLockOperation: "agent_directory_release", restoreLockOwnerState: "alive", restoreLockOwnerSameProcess: true,
|
||||
restoreLockOwnerPredatesProcess: true, restoreLockKnownLocalHolder: false,
|
||||
restoreLockOwnerAgeMs: 120_000, restoreLockWaitMs: 30_001 });
|
||||
expect(JSON.stringify(result)).not.toContain("sentinel-");
|
||||
expect(result.execution).not.toHaveProperty("ownerPid");
|
||||
});
|
||||
|
||||
it.each([null, -1, Infinity, NaN, 1.5, 604_800_001, "private", {}])("omits invalid lock diagnostic values (%j)", value => {
|
||||
const error = Object.assign(new Error("lock timeout"), { code: "ERR_WORKSPACE_RESTORE_LOCK_TIMEOUT",
|
||||
workspaceRestoreLock: { operation: value, ownerState: value, ownerAgeMs: value, waitMs: value,
|
||||
ownerSameProcess: value, ownerPredatesProcess: value, knownLocalHolder: value } });
|
||||
expect(collect(error).execution).toEqual({});
|
||||
});
|
||||
|
||||
it("does not attach lock evidence to unrelated errors or invoke hostile getters", () => {
|
||||
expect(collect({ code: "OTHER", workspaceRestoreLock: { ownerState: "alive" } }).execution).toEqual({});
|
||||
const error = { code: "ERR_WORKSPACE_RESTORE_LOCK_TIMEOUT",
|
||||
workspaceRestoreLock: Object.defineProperty({}, "ownerState", { get() { throw new Error("private"); } }) };
|
||||
expect(collect(error).execution).toEqual({});
|
||||
});
|
||||
|
||||
it.each(["restore_permission_denied", "restore_lock_timeout", "restore_unsafe_archive", "restore_failed"])(
|
||||
"includes the saved %s classification without copying workspace paths or results", (code) => {
|
||||
const result = sanitizeRunFailureDiagnostics(collectRunFailureDiagnostics(run({ resultJson: {
|
||||
|
||||
@@ -4,7 +4,7 @@ import { and, eq } from "drizzle-orm";
|
||||
import { agents, environmentLeases, environments, agentInstructionWorkingCopies as copies, type Db } from "@paperclipai/db";
|
||||
import { syncDirectoryToSsh, restoreWorkspaceFromSshExecution } from "@paperclipai/adapter-utils/ssh";
|
||||
import { prepareAdapterExecutionTargetRuntime, runAdapterExecutionTargetShellCommand, type AdapterExecutionTarget, type PreparedAdapterExecutionTargetRuntime } from "@paperclipai/adapter-utils/execution-target";
|
||||
import { withDirectoryMergeLock, directorySnapshotSha256, parseDirectorySnapshot, serializeDirectorySnapshot } from "@paperclipai/adapter-utils/workspace-restore-merge";
|
||||
import { withDirectoryMergeLock, directorySnapshotSha256, parseDirectorySnapshot, serializeDirectorySnapshot, type DirectoryMergeLockOperation } from "@paperclipai/adapter-utils/workspace-restore-merge";
|
||||
import { AGENT_FILES_CONTRACT, AgentFileLimitError, agentFileStore, agentStorageWarning, inspectAgentDirectory } from "./agent-file-store.js";
|
||||
import { agentInstructionsBundleMode, deriveBundleState, resolveManagedInstructionsRoot } from "./agent-instructions.js";
|
||||
import { instructionGitExcludeProgram } from "./agent-instruction-files.js";
|
||||
@@ -108,7 +108,7 @@ export function agentDirectoryWorkingCopyService(db: Db, get: (companyId: string
|
||||
if (!handedOff) await db.delete(copies).where(and(eq(copies.companyId, input.companyId), eq(copies.runId, input.runId)));
|
||||
throw error;
|
||||
}
|
||||
});
|
||||
}, "agent_directory_handoff");
|
||||
}
|
||||
const localRoot = path.join(path.dirname(root), "file-sync", "runs", input.runId, "live");
|
||||
// Local copies live outside the task cwd. Remote copies use the reserved,
|
||||
@@ -343,18 +343,18 @@ export function agentDirectoryWorkingCopyService(db: Db, get: (companyId: string
|
||||
cwd: remoteCwd, env: {}, timeoutMs: 15_000, bypassSession: true });
|
||||
return result.exitCode === 0 && !result.timedOut;
|
||||
}
|
||||
async function serial<T>(row: Copy, fn: (current: Copy) => Promise<T>): Promise<T> {
|
||||
async function serial<T>(row: Copy, fn: (current: Copy) => Promise<T>, operation: DirectoryMergeLockOperation): Promise<T> {
|
||||
// Duplicate stop callbacks and restart recovery must not race while moving
|
||||
// the stopped working copy. This lock is outside the writable tree.
|
||||
return withDirectoryMergeLock(path.resolve(row.localRoot, "../../.."), async () => {
|
||||
const current = await get(row.companyId, row.runId);
|
||||
if (!current) throw notFound("Agent directory copy not found");
|
||||
return fn(current);
|
||||
});
|
||||
}, process.env, operation);
|
||||
}
|
||||
return { prepare, hasChanges, canReuse,
|
||||
checkpointWarm: (row: Copy, target?: AdapterExecutionTarget | null) => serial(row, current => current.receipt?.warm === true ? checkpoint(current, target) : Promise.resolve(current)),
|
||||
collectStopped: (row: Copy, target?: AdapterExecutionTarget | null) => serial(row, current => collectStopped(current, target)),
|
||||
release: (row: Copy) => serial(row, release),
|
||||
checkpointWarm: (row: Copy, target?: AdapterExecutionTarget | null) => serial(row, current => current.receipt?.warm === true ? checkpoint(current, target) : Promise.resolve(current), "agent_directory_checkpoint"),
|
||||
collectStopped: (row: Copy, target?: AdapterExecutionTarget | null) => serial(row, current => collectStopped(current, target), "agent_directory_collect"),
|
||||
release: (row: Copy) => serial(row, release, "agent_directory_release"),
|
||||
};
|
||||
}
|
||||
@@ -183,6 +183,27 @@ export function collectRunFailureDiagnostics(run: Run, options: RunFailureReport
|
||||
break;
|
||||
}
|
||||
if (typeof error !== "object") break;
|
||||
if (execution.restoreLockOwnerState === undefined && read(error, "code") === "ERR_WORKSPACE_RESTORE_LOCK_TIMEOUT") {
|
||||
const lock = read(error, "workspaceRestoreLock");
|
||||
const operation = read(lock, "operation");
|
||||
if (["agent_directory_release", "agent_directory_collect", "agent_directory_checkpoint", "agent_directory_handoff"].some(value => value === operation)) {
|
||||
execution.restoreLockOperation = operation as string;
|
||||
}
|
||||
const ownerState = read(lock, "ownerState");
|
||||
if (["alive", "dead", "unknown", "missing", "invalid"].some(value => value === ownerState)) {
|
||||
execution.restoreLockOwnerState = ownerState as string;
|
||||
}
|
||||
for (const field of ["knownLocalHolder", "ownerSameProcess", "ownerPredatesProcess"]) {
|
||||
const value = read(lock, field);
|
||||
if (typeof value === "boolean") execution[`restoreLock${field[0]!.toUpperCase()}${field.slice(1)}`] = value;
|
||||
}
|
||||
for (const field of ["ownerAgeMs", "waitMs"]) {
|
||||
const value = read(lock, field);
|
||||
if (typeof value === "number" && Number.isSafeInteger(value) && value >= 0 && value <= 7 * 24 * 60 * 60 * 1000) {
|
||||
execution[`restoreLock${field[0]!.toUpperCase()}${field.slice(1)}`] = value;
|
||||
}
|
||||
}
|
||||
}
|
||||
const entry: RunFailureException = {};
|
||||
for (const field of ["name", "message", "stack", "code"] as const) {
|
||||
const value = read(error, field);
|
||||
|
||||
Reference in new issue
Block a user