Devin FoleyandPaperclip
dd9983b894
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agent runs restore workspace files and collect instruction-file changes. > - Writers to the same target directory must wait for each other. > - The current lock records a PID, which a new process can reuse after a crash. > - A reused PID can keep an orphaned lock alive and make each later run fail. > - This pull request makes a SQLite file lock decide ownership. The OS releases it when the process exits. > - Later runs can proceed after a crash, and concurrent live writers remain protected. ## Linked Issues or Issue Description Refs #10914. This addresses crash recovery. It does not cancel a stalled operation in a process that is still alive. Related work: #9667, #14787, and #12187. The earlier attempt in #9667 assumes one live server per lock root. This implementation uses an OS-backed lock to support concurrent writers without treating a different process token or an old timestamp as proof of a dead owner. It retains the private lock root and bounded timeout diagnostics from the merged changes. After a process dies while holding a restore lock, a replacement process can reuse its PID. The existing `process.kill(pid, 0)` check then reports a live owner forever. Later runs can complete their model turn but fail during file collection or restore. ## What Changed - Hold a SQLite `BEGIN IMMEDIATE` transaction for each directory write. Use the existing built-in `node:sqlite` dependency. - Keep each lock database on a stable inode. Keep PID and time metadata only for diagnostics. - Retain the 30-second asynchronous wait and existing timeout error code and diagnostic fields. - Fail closed when an old directory lock exists. Document a stopped-writer upgrade and rollback procedure. - Add real child-process tests for crashes, PID reuse, live owners, and connection cleanup. Cover callback failures, independent targets, stable inodes, invalid lock files, and ambiguous legacy records. ## Verification - Before the fix, the crash/PID-reuse test and the live-owner test both failed. Both pass with this change. - `pnpm exec vitest run packages/adapter-utils/src/directory-merge-lock.test.ts packages/adapter-utils/src/workspace-restore-merge.test.ts`: 56 tests passed. - Restore and agent-file working-copy integration tests: 118 tests passed before the additional connection-cleanup test. - `pnpm -r typecheck`: passed. - `pnpm build`: passed. - Full GitHub CI: all checks passed, including Linux workspace tests, server test shards, build, typecheck, and browser tests. - Greptile: 5/5, with no review threads or unresolved comments. - `pnpm test:run`: started locally, then stopped with SIGINT (exit 130) after full CI passed. The local serial run did not complete and is not counted as a full local pass. The completed CI shards provide the full-suite result. ## Risks - **Upgrade and rollback require a drain.** Stop every old writer that shares an instance root before switching protocols. Old and new versions must not write concurrently. - Existing legacy `.lock/` directories remain blocking. After all writers stop, preserve run evidence and move those directories to an operator scratch directory. The new code does not infer that they are abandoned from PID or age. - Never delete or replace a `.lock.sqlite` file while writers can run. These small files remain after release. - The shared filesystem must support reliable SQLite locking. Broken network-filesystem locking is unsupported. - This change prevents new orphaned ownership. It does not recover file changes lost during earlier failed collections, or interrupt a live operation that stalls. - No application database migration or new native dependency is required. See `doc/workspace-restore-locks.md` for the procedure. ## Model Used OpenAI Codex based on GPT-6, with code execution and repository tools. The exact model variant and context window are not exposed in this session. ## 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 (focused regression and integration suites; see the full-suite note 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>
3.2 KiB
Workspace restore locks
Workspace restore and agent-file collection serialize writes to each canonical
target directory. Their lock files live in
<instance root>/locks/directory-merge, outside the writable target. All writers
must use the same instance root and a filesystem with reliable SQLite file
locking. Network filesystems that do not provide that locking are not supported.
Each target has a permanent <hash>.lock.sqlite file. An open SQLite
BEGIN IMMEDIATE transaction holds its reserved file lock for the entire write
operation. Contenders retry without blocking the Node event loop, up to the
existing 30-second limit. Closing the connection releases the lock; the operating
system also releases it when the process exits or crashes. Independent targets
use different files and can proceed concurrently.
This uses the same built-in node:sqlite dependency as workspace manifests.
See SQLite file locking for the reserved
lock contract. No application database or schema migration is involved.
The adjacent <hash>.lock.owner.json records a PID and creation time only for
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.
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
after release, including for targets that no longer exist. Their diagnostic
owner sidecars are normally removed after release.
Upgrade from directory locks
Older versions created <hash>.lock/owner.json and checked only whether the
recorded PID existed. A server restart could reuse that PID and leave an orphaned
lock permanently protected. Those records cannot establish a process lifetime or
PID namespace, so the new implementation never guesses that a legacy holder is
dead. An existing legacy directory continues to block admission.
Do not run old and new lock protocols concurrently against the same instance root. An old process does not participate in the SQLite lock protocol.
- Drain and stop all old server and worker processes that can write to the instance root, including processes on other hosts or in other containers.
- Preserve any unfinished-run evidence needed for recovery. After all writers
are stopped, move leftover legacy
<hash>.lock/directories to an operator scratch directory outside the lock root. Do not remove.lock.sqlitefiles. - Start all writers on the new version. Verify that a run completes both the provider turn and file collection/restore. A successful model response alone does not prove that its local file changes were saved.
The same drain requirement applies to rollback. Existing permanent SQLite files can remain on disk; older versions ignore them. Do not infer that a legacy lock is safe to remove from its age, an absent PID, or a successful task response.
This change prevents new orphaned ownership. It cannot recover file changes that an earlier failed collection discarded.