From 0b266bd084b39a18c9e566fce4690c8c609623cb Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 20 Oct 2026 07:01:17 -0500 Subject: [PATCH] fix(workspaces): keep source preparation locks active (#16956) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Each task prepares its workspace before it starts the provider. > - Source preparation holds a transaction-scoped advisory lock on a dedicated database connection. > - Preparation can use other connections and the filesystem for longer than PostgreSQL allows that lock connection to stay idle. > - PostgreSQL then closes the connection and releases the lock while preparation still runs. > - This pull request reuses the workspace lock keepalive during source preparation. > - Slow preparation retains exclusion and returns its original result or error. ## Linked Issues or Issue Description **What happened?** Tasks failed during workspace preparation with `CONNECTION_CLOSED` at the configured 60-second idle transaction timeout. No provider had started. A live retry showed the source-lock transaction idle while authorization reads ran on another connection. Its advisory-lock key matched the task's source-preparation key. PostgreSQL closed that transaction at the timeout. The same defect remains on current master. A real PostgreSQL regression also shows that another preparation can enter after the timeout while the original callback still runs. **Expected behavior** Source preparation must keep its advisory lock until the callback and any pending keepalive query finish. The ordinary pool must remain usable. The original preparation result or failure must survive. **Steps to reproduce** 1. Set `idle_in_transaction_session_timeout` to one second in a disposable PostgreSQL database. 2. Call `withIsolatedRepositorySourceLock` and keep its callback active for longer than one second without querying the lock connection. 3. Try to acquire the same source lock from another operation. 4. On the old code, the second operation enters and the first transaction fails with `CONNECTION_CLOSED`. **Paperclip version or commit** Reproduced on source at `cde944ed75648adfb569e86133adc8b118b55dd6`. The repair is rebased onto `d855e550ad`. **Additional context** Refs #16340. That change introduced the keepalive for workspace copyback. This PR applies the existing helper to the source-preparation transaction. ## What Changed - Read the configured idle timeout in the same query that acquires the source lock. - Keep that dedicated connection active during source preparation with the existing helper. - Add two real PostgreSQL regressions for exclusion, company independence, a one-slot ordinary pool, nested acquisition, cleanup, and preservation of callback errors. - Update the execution contract and workspace verification maps. ## Verification - Red before the production edit: both new tests failed. A competing operation entered after the lock expired. A callback failure was replaced by `CONNECTION_CLOSED`. - Green after the repair: 86 tests passed across the source lock, source admission, keepalive, and heartbeat workspace preparation suites. - `pnpm -r typecheck`, `pnpm build`, and `node scripts/check-maps.mjs` passed on final commit `2b5000ce25`. - `git diff --check` passed. The lockfile is unchanged. - After rebase, the same 86 focused tests passed. The two source-lock tests passed again after the review cleanup change. - Live acceptance on `7df94d7a06`: a fresh local instance on macOS arm64, Node v26.4.0, native Codex `gpt-6.1-sol`, and permission mode `never`. PostgreSQL had a one-second idle transaction timeout. A temporary Git wrapper delayed repository reads by four seconds. Workspace preparation after environment acquisition took 8,297 ms. The real task completed, wrote and attached the correct line-count/SHA-256 report, reached `done` with native phase `committed`, and cleared its execution lock. The attachment matched the workspace file and survived browser reload. No dedicated lock connections remained. - The live probe initially used `gpt-5.4`, which the ChatGPT account rejected. Selecting the reported task model, `gpt-6.1-sol`, resolved that readiness failure. The local Claude CLI has no login, so no live Claude run is claimed. - Full CI passed on `2b5000ce25`: all general, chat, workspace, serialized-server, Runner, and E2E test shards, plus typecheck, build, and canary packaging. Both Greptile and Paperclip Review rated this commit 5/5 with no open findings. The Paperclip check concluded neutral; two unrelated visual checks were skipped. - The duplicate local `pnpm test:run` serial invocation was stopped with SIGINT after the complete CI test shards passed. It did not finish locally; no local full-suite pass is claimed. Local proof consists of the focused real PostgreSQL tests, typecheck, build, map checks, and the real task above. - The final commit changes test teardown only; production code matches the live acceptance commit. - Initial sandbox runs could not open test database ports or build IPC sockets. The same commands ran with local process access. The frozen install also found an existing manifest/lockfile mismatch; dependency resolution was local and the lockfile was restored. ## Risks - The lock connection sends one small query per keepalive interval. The helper derives that interval from the configured timeout and caps it at 30 seconds. It does no polling when the timeout is disabled. - The helper joins an in-flight query before the transaction ends. A slow database can therefore delay cleanup. - This change does not disable database deadlines, retry preparation, or recover an unexpected connection loss. It preserves existing admission checks and company/source lock keys. - Deployment and recovery of the reported tasks remain separate from code verification. ## Model Used OpenAI GPT-6 through Codex, with repository tools, code execution, and browser verification. The exact served 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 - [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 --- doc/execution-semantics.md | 7 ++ maps/agent-access-map/workspaces.md | 7 ++ maps/feature-map/workspaces.md | 6 ++ ...lated-remote-workspace-source-lock.test.ts | 72 +++++++++++++++++++ .../isolated-remote-workspace-source.ts | 14 ++-- 5 files changed, 101 insertions(+), 5 deletions(-) create mode 100644 server/src/services/isolated-remote-workspace-source-lock.test.ts diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 01c64e634f1..94a47a39638 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -657,6 +657,13 @@ qualify as capacity failures. ### Workspace scan failures before provider startup +Workspace source preparation keeps its dedicated advisory-lock connection active +while authorization and filesystem work use other connections. Keepalive queries +follow the configured idle-transaction timeout without changing that setting. +Preparation retains source exclusion until its work and any in-flight keepalive +query finish. Ordinary pool reads remain available, including with a one-slot pool. +This does not authorize replay after a connection failure or bypass admission gates. + Repository discovery distinguishes an ordinary folder from a failed Git read. A timeout, full scan queue, cancellation, output limit, or Git failure must keep its typed cause through workspace preparation and run persistence. It must not diff --git a/maps/agent-access-map/workspaces.md b/maps/agent-access-map/workspaces.md index 7eeb62a1e3a..d3eb13d3ef4 100644 --- a/maps/agent-access-map/workspaces.md +++ b/maps/agent-access-map/workspaces.md @@ -135,6 +135,13 @@ a running service. ## `provisioning` +Source preparation keeps its dedicated lock connection active during work on +other connections. It preserves company/source exclusion and existing admission +authority; it adds no agent API or permission. The +[source-lock regression](../../server/src/services/isolated-remote-workspace-source-lock.test.ts) +covers PostgreSQL idle timeout, competing preparation, ordinary pool availability, +and failure cleanup. Deployed agent recovery requires separate live evidence. + Review: `baseline`. API: `present` — representative operation IDs: diff --git a/maps/feature-map/workspaces.md b/maps/feature-map/workspaces.md index 5380d9af9a4..95234e045e5 100644 --- a/maps/feature-map/workspaces.md +++ b/maps/feature-map/workspaces.md @@ -55,6 +55,12 @@ Automated: [workspace card](../../ui/src/components/IssueWorkspaceCard.test.tsx) Manual: Run a task that writes a small file, follow the bound workspace, and inspect the file and revision. Reload and verify links still refer to the same workspace, not another checkout or a stale run. +For slow source preparation, configure a short PostgreSQL idle-transaction +timeout in a disposable instance. Hold preparation beyond that interval and +verify it retains its source lock, rejects competing preparation, and leaves +ordinary reads available. The [source-lock regression](../../server/src/services/isolated-remote-workspace-source-lock.test.ts) +uses real PostgreSQL; it does not prove provider startup or deployed recovery. + For fresh standard-trust tasks using an implicit repository default, create a disposable project checkout whose shared Git storage overlaps a private task workspace. Verify that source admission creates a separate checkout from the diff --git a/server/src/services/isolated-remote-workspace-source-lock.test.ts b/server/src/services/isolated-remote-workspace-source-lock.test.ts new file mode 100644 index 00000000000..36f8d460cb1 --- /dev/null +++ b/server/src/services/isolated-remote-workspace-source-lock.test.ts @@ -0,0 +1,72 @@ +import { randomUUID } from "node:crypto"; +import { setTimeout as delay } from "node:timers/promises"; +import { sql } from "drizzle-orm"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { createDb } from "@paperclipai/db"; +import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "../__tests__/helpers/embedded-postgres.js"; +import { IsolatedRepositorySourceBusy, withIsolatedRepositorySourceLock } from "./isolated-remote-workspace-source.js"; + +const support = await getEmbeddedPostgresTestSupport(); +const describePostgres = support.supported ? describe : describe.skip; +if (!support.supported) console.warn(`Workspace source lock tests require Postgres: ${support.reason}`); + +describePostgres("workspace source ownership during preparation", () => { + let database: Awaited>; + let db: ReturnType; + + beforeAll(async () => { + database = await startEmbeddedPostgresTestDatabase("workspace-source-lock-"); + db = createDb(database.connectionString, { maxConnections: 1 }); + const rows = await db.execute(sql`select current_user as name`); + const role = String(rows[0].name); + await db.execute(sql`alter role ${sql.identifier(role)} set idle_in_transaction_session_timeout = '1s'`); + }); + + afterAll(async () => { + try { + await db?.$client.end({ timeout: 1 }); + } finally { + await database?.cleanup(); + } + }); + + it("keeps preparation exclusive beyond the idle timeout while the ordinary pool stays usable", async () => { + const companyId = randomUUID(), issueId = randomUUID(); + let entered!: () => void, release!: () => void; + const started = new Promise(resolve => { entered = resolve; }); + const pending = new Promise(resolve => { release = resolve; }); + const preparation = withIsolatedRepositorySourceLock(db, companyId, issueId, async () => { + await db.execute(sql`select 1`); + entered(); + await pending; + return withIsolatedRepositorySourceLock(db, companyId, issueId, async () => "prepared"); + }).then(value => ({ value }), error => ({ error })); + + try { + await started; + await delay(1400); + await expect(db.execute(sql`select 1 as usable`)).resolves.toMatchObject([{ usable: 1 }]); + await expect(withIsolatedRepositorySourceLock(db, companyId, issueId, async () => "overlap", true)) + .rejects.toBeInstanceOf(IsolatedRepositorySourceBusy); + await expect(withIsolatedRepositorySourceLock(db, randomUUID(), issueId, async () => "independent", true)) + .resolves.toBe("independent"); + } finally { + release(); + await preparation; + } + expect(await preparation).toEqual({ value: "prepared" }); + await expect(withIsolatedRepositorySourceLock(db, companyId, issueId, async () => "next", true)) + .resolves.toBe("next"); + }); + + it("releases a failed preparation and preserves the original failure", async () => { + const companyId = randomUUID(), issueId = randomUUID(); + const failure = new Error("source preparation failed"); + await expect(withIsolatedRepositorySourceLock(db, companyId, issueId, async () => { + await delay(1400); + throw failure; + })).rejects.toBe(failure); + await expect(withIsolatedRepositorySourceLock(db, companyId, issueId, async () => "recovered", true)) + .resolves.toBe("recovered"); + }); +}); diff --git a/server/src/services/isolated-remote-workspace-source.ts b/server/src/services/isolated-remote-workspace-source.ts index db50622f63f..44e7c6ae7db 100644 --- a/server/src/services/isolated-remote-workspace-source.ts +++ b/server/src/services/isolated-remote-workspace-source.ts @@ -11,6 +11,7 @@ import { resolvePaperclipInstanceRoot } from "../home-paths.js"; import { materializeManagedProjectWorkspace } from "./managed-repository-checkout.js"; import { measureRunPhase } from "./execution-preparation-timing.js"; import { isRestrictedWorkspaceSourceDenial } from "./workspace-source-restriction.js"; +import { withWorkspaceLockKeepalive } from "./native-runtime/native-workspace-lock-keepalive.js"; const execFile = promisify(execFileCallback); @@ -24,11 +25,14 @@ export async function withIsolatedRepositorySourceLock(db: Db, companyId: str const held = sourceLocks.getStore() ?? []; if (held.some(lock => lock.db === db && lock.key === key)) return action(); return withDedicatedDbConnection(db, connection => connection.transaction(async tx => { - if (tryOnly) { - const rows = await tx.execute<{ acquired: boolean }>(sql`select pg_try_advisory_xact_lock(hashtextextended(${key}, 0)) as acquired`); - if (!rows[0]?.acquired) throw new IsolatedRepositorySourceBusy(); - } else await tx.execute(sql`select pg_advisory_xact_lock(hashtextextended(${key}, 0))`); - return sourceLocks.run([...held, { db, key }], action); + const acquire = tryOnly + ? sql`pg_try_advisory_xact_lock(hashtextextended(${key}, 0))` + : sql`pg_advisory_xact_lock(hashtextextended(${key}, 0))`; + const rows = await tx.execute(sql`select ${acquire} as acquired, + (select setting::bigint from pg_settings where name = 'idle_in_transaction_session_timeout') as idle_timeout_ms`); + if (tryOnly && !rows[0]?.acquired) throw new IsolatedRepositorySourceBusy(); + return withWorkspaceLockKeepalive(tx, Number(rows[0]?.idle_timeout_ms), + () => sourceLocks.run([...held, { db, key }], action)); })); }