From ad0bd26bb78e8bb6d4deae826b9bcc9c7e369734 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:53:41 -0500 Subject: [PATCH] perf(heartbeat): avoid Git inspection for stored workspace reads (#16306) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Heartbeat selects and restores a saved execution workspace before dispatch. > - These reads used the workspace detail reader, which also scans Git and resolves delivery state. > - Configuration reads do not use those live display fields. Each extra process can delay other controller work. > - This pull request adds a stored-record reader and uses it in configuration, environment selection, and finalization. > - It also derives ancestry from the existing ahead/behind result and skips delivery HEAD reads for unfinished tasks. > - The benefit is fewer Git processes while live execution and cleanup guards remain in place. ## Linked Issues or Issue Description Related: #16282, #16302, #13619. Those changes reduce background or display scan demand. This change removes live display inspection from execution configuration reads. **What existing behavior does this improve?** Heartbeat workspace configuration reads and workspace close-readiness checks. **Current behavior** Reading saved workspace configuration also loads runtime health, Git status, ancestry, and delivery state. Close readiness walks history twice to determine ancestry. **Proposed behavior** Execution reads stored configuration through a company-checked method. Live branch validation and destructive cleanup still inspect current Git state. Close readiness uses its ahead/behind result to determine ancestry. **Reason and benefit** A synthetic benchmark of the actual service returned identical stored fields for 20 workspaces. Detail reads launched 60 Git commands and took 689 ms. Stored reads launched zero Git commands and took 8 ms. This was a local macOS measurement, not an end-to-end cloud latency claim. **Breaking changes** None to public APIs or saved data. Workspace detail responses keep their live fields. ## What Changed - Add `getStoredById(companyId, id)` and use it in three heartbeat reads. - Preserve the access-denied error for a foreign-company workspace. - Remove the extra `merge-base` process and the unused HEAD read for unfinished tasks. - Add recovery, company-boundary, ancestry, and cleanup regressions. - Add a repeatable database/Git benchmark and document the read boundary. ## Verification - The new environment-selection regression failed before the fix with `Live Git inspection is unavailable` and passes after it. All eight environment-selection tests pass. - On the final rebased commit, workspace service, environment selection, and branch finalization: 89 tests passed. Branch-containment, runtime-execution, and display-cache suites also passed during development. - `pnpm -r typecheck`: passed. - `pnpm build`: passed. - `pnpm test:run`: started locally, then stopped after all full-suite CI test shards passed on the final commit. The complete local run was not finished; CI provides the full-suite result. - Final commit `9a13ef0481b7bf6a60b2b7ac13f35f78a14e232c`: 55 successful checks and 2 expected skips. Greptile: 5/5, no actionable findings. - `pnpm --filter @paperclipai/server exec tsx ../scripts/benchmark-workspace-record-reads.ts`: identical stored fields, 60 to zero Git commands, 689 ms to 8 ms for 20 reads. ## Risks - Execution callers must use saved fields only. Detail readers and live branch and cleanup guards remain separate. - A failed ancestry comparison stays unknown. Automatic cleanup still rejects unknown delivery state and rechecks Git before deletion. - This patch has not been deployed. It does not prove the full slow-run latency is resolved. ## Model Used OpenAI Codex, GPT-6. The exact model identifier and context window are not exposed in this session. Used reasoning, repository inspection, code editing, and local command 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 - [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/DEVELOPING.md | 14 ++++ scripts/benchmark-workspace-record-reads.ts | 81 +++++++++++++++++++ .../execution-workspaces-service.test.ts | 52 ++++++++++++ server/src/services/execution-workspaces.ts | 29 +++---- server/src/services/heartbeat.ts | 2 +- .../heartbeat/environment-selection.test.ts | 21 +++++ .../heartbeat/environment-selection.ts | 4 +- .../services/heartbeat/runtime-execution.ts | 5 +- 8 files changed, 186 insertions(+), 22 deletions(-) create mode 100644 scripts/benchmark-workspace-record-reads.ts diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index f5b7edaa274..7ad912abaf8 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -1065,6 +1065,20 @@ Workspace preparation resolves an existing root symlink before reading the snaps Execution-workspace close-readiness and branch-reconciliation status checks allow up to 32 MiB of buffered output. This accommodates nested task worktrees while preserving exact untracked-file counts. Larger output fails the scan and blocks destructive cleanup. Other buffered scan bounds stay unchanged. +Heartbeat configuration, environment selection, and finalization read persisted +workspace records through the company-scoped `getStoredById` service method. +Those reads do not inspect Git, refresh runtime service health, or resolve PR +delivery. Workspace detail responses still load live display state, and the +execution and cleanup boundaries retain their live branch and cleanliness +guards. Close readiness derives ancestry from its existing ahead/behind +comparison, avoiding a second history walk with `merge-base`. + +Measure the avoided reads on synthetic workspaces with +`pnpm --filter @paperclipai/server exec tsx ../scripts/benchmark-workspace-record-reads.ts`. +The benchmark checks identical persisted fields and reports wall time and actual +Git command counts. Use `--resident-mib=1536` to exercise a large Node process on +Linux. It measures workspace reads, not end-to-end agent latency. + The cache intentionally trades up to a few seconds of changed-file freshness for stable server latency. The file browser retains an explicit refresh action, does not start its query while the panel or browser tab is hidden, and presents overloads as retryable failures rather than an empty workspace. A full queue returns `503` with code `workspace_git_scan_saturated`; a scan exceeding its wall-clock limit returns `504` with code `workspace_git_scan_timeout`. Both responses include `Retry-After: 1`. Sandbox Git sync treats only the selected repository root as a clone source. A selected subfolder uses directory sync within that folder, applies the enclosing repository's ignore rules, and does not transfer parent files or Git history. diff --git a/scripts/benchmark-workspace-record-reads.ts b/scripts/benchmark-workspace-record-reads.ts new file mode 100644 index 00000000000..d92711b1333 --- /dev/null +++ b/scripts/benchmark-workspace-record-reads.ts @@ -0,0 +1,81 @@ +import assert from "node:assert/strict"; +import { execFile } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { promisify } from "node:util"; +import { companies, createDb, executionWorkspaces, startEmbeddedPostgresTestDatabase } from "../packages/db/src/index.js"; +import { executionWorkspaceService } from "../server/src/services/execution-workspaces.js"; + +const run = promisify(execFile); +const root = await fs.mkdtemp(path.join(os.tmpdir(), "workspace-record-benchmark-")); +const repo = path.join(root, "repo"); +const worktree = path.join(root, "worktree"); +const residentMiB = Number(process.argv.find(arg => arg.startsWith("--resident-mib="))?.split("=")[1] ?? 0); +assert.ok(Number.isInteger(residentMiB) && residentMiB >= 0 && residentMiB <= 2048); +const previousTrace = process.env.GIT_TRACE; +const keepAlive = setInterval(() => {}, 1000); +let database: Awaited> | undefined; +let db: ReturnType | undefined; +try { + await fs.mkdir(repo); + const git = (...args: string[]) => run("git", ["-C", repo, ...args]); + await git("init", "-b", "main"); + await git("config", "user.name", "Workspace benchmark"); + await git("config", "user.email", "benchmark@paperclip.local"); + await fs.writeFile(path.join(repo, "README.md"), "Synthetic workspace\n"); + await git("add", "README.md"); + await git("commit", "-m", "Fixture"); + await git("worktree", "add", "-b", "fixture", worktree); + database = await startEmbeddedPostgresTestDatabase("workspace-record-benchmark-"); + db = createDb(database.connectionString); + const companyId = randomUUID(); + await db.insert(companies).values({ id: companyId, name: "Synthetic workspace reads", issuePrefix: "BENCH" }); + const rows = await db.insert(executionWorkspaces).values(Array.from({ length: 20 }, (_, index) => ({ + companyId, name: `Fixture ${index}`, mode: "isolated_workspace", strategyType: "git_worktree", + cwd: worktree, providerRef: worktree, providerType: "git_worktree", baseRef: "main", branchName: "fixture", + }))).returning(); + const service = executionWorkspaceService(db); + const resident = Array.from({ length: residentMiB }, () => Buffer.alloc(1024 * 1024, 1)); + const persisted = (workspace: NonNullable>>) => { + const { runtimeServices: _services, deliveryState: _delivery, ...record } = workspace; + return record; + }; + const results = []; + let expected: ReturnType[] | undefined; + await service.getStoredById(companyId, rows[0].id); + for (const mode of ["detail", "stored"] as const) { + const trace = path.join(root, `${mode}.trace`); + process.env.GIT_TRACE = trace; + const start = performance.now(); + const records = []; + for (const row of rows) { + const workspace = mode === "detail" + ? await service.getById(row.id) + : await service.getStoredById(companyId, row.id); + assert.ok(workspace); + records.push(persisted(workspace)); + } + const wallMs = performance.now() - start; + if (expected) assert.deepEqual(records, expected); + else expected = records; + const traceText = await fs.readFile(trace, "utf8").catch(error => { + if (error.code === "ENOENT") return ""; + throw error; + }); + results.push({ mode, reads: rows.length, wallMs, gitCommands: traceText.split("\n").filter(line => line.includes("built-in: git ")).length }); + } + assert.equal(results[1].gitCommands, 0); + console.log(JSON.stringify({ node: process.version, platform: process.platform, arch: process.arch, + residentMiB: resident.length, rssMiB: Math.round(process.memoryUsage().rss / 1024 / 1024), + equalPersistedRecords: true, results }, null, 2)); +} finally { + if (previousTrace === undefined) delete process.env.GIT_TRACE; + else process.env.GIT_TRACE = previousTrace; + try { + await db?.$client.end({ timeout: 0 }); + await database?.cleanup(); + await fs.rm(root, { recursive: true, force: true }); + } finally { clearInterval(keepAlive); } +} diff --git a/server/src/__tests__/execution-workspaces-service.test.ts b/server/src/__tests__/execution-workspaces-service.test.ts index 6bd89ec434f..e87418af0bb 100644 --- a/server/src/__tests__/execution-workspaces-service.test.ts +++ b/server/src/__tests__/execution-workspaces-service.test.ts @@ -855,6 +855,58 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { expect(workspace).toMatchObject({ status: "archived", cleanupReason: "issue_terminal" }); }, 20_000); + it("reads company-scoped execution configuration without live Git inspection", async () => { + const seed = await seedAncestryTerminalWorkspace(); + const service = executionWorkspaceService(db, { + inspectGitCloseReadiness: async () => { throw new Error("Git inspection unavailable"); }, + }); + const config = { provisionCommand: "echo prepare", environmentId: randomUUID() }; + await db.update(executionWorkspaces).set({ metadata: { config } }) + .where(eq(executionWorkspaces.id, seed.executionWorkspaceId)); + + expect(await service.getStoredById(seed.companyId, seed.executionWorkspaceId)).toMatchObject({ + id: seed.executionWorkspaceId, companyId: seed.companyId, + cwd: seed.worktreePath, baseRef: "main", config, + deliveryState: "unknown", runtimeServices: [], + }); + await expect(service.getStoredById(randomUUID(), seed.executionWorkspaceId)) + .rejects.toThrow("Task workspace access is no longer available"); + expect(await service.getStoredById(seed.companyId, randomUUID())).toBeNull(); + await expect(service.getById(seed.executionWorkspaceId)).rejects.toThrow("Git inspection unavailable"); + }); + + it("derives ancestry from the existing comparison and stays unknown for an invalid base", async () => { + const seed = await seedAncestryTerminalWorkspace(); + const traceDir = await fs.mkdtemp(path.join(os.tmpdir(), "workspace-git-trace-")); + tempDirs.add(traceDir); + const tracePath = path.join(traceDir, "git.trace"); + const previousTrace = process.env.GIT_TRACE; + process.env.GIT_TRACE = tracePath; + try { + const merged = await svc.getCloseReadiness(seed.executionWorkspaceId); + expect(merged?.git).toMatchObject({ aheadCount: 0, behindCount: 0, isMergedIntoBase: true }); + expect(await fs.readFile(tracePath, "utf8")).not.toContain("merge-base"); + + await fs.writeFile(path.join(seed.worktreePath, "new.txt"), "Undelivered work\n"); + await runGit(seed.worktreePath, ["add", "new.txt"]); + await runGit(seed.worktreePath, ["commit", "-m", "Undelivered work"]); + const ahead = await svc.getCloseReadiness(seed.executionWorkspaceId); + expect(ahead?.git).toMatchObject({ aheadCount: 1, behindCount: 0, isMergedIntoBase: false }); + expect(ahead?.deliveryState).toBe("unmerged"); + + await db.update(executionWorkspaces).set({ baseRef: "missing-base" }) + .where(eq(executionWorkspaces.id, seed.executionWorkspaceId)); + const unknown = await svc.getCloseReadiness(seed.executionWorkspaceId); + expect(unknown?.git).toMatchObject({ aheadCount: null, behindCount: null, isMergedIntoBase: null }); + expect(unknown?.deliveryState).toBe("unknown"); + expect(await svc.sweepTerminalWorkspaces()).toMatchObject({ archived: 0, skippedUndelivered: 1 }); + await expect(fs.access(seed.worktreePath)).resolves.toBeUndefined(); + } finally { + if (previousTrace === undefined) delete process.env.GIT_TRACE; + else process.env.GIT_TRACE = previousTrace; + } + }); + it("fails closed before archive when git status inspection is unavailable", async () => { const seeded = await seedAncestryTerminalWorkspace(); const statusSpy = vi.spyOn(workspaceGitOperationScheduler, "run") diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index 05e0bc77a63..6bd5d2762b0 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -963,27 +963,16 @@ async function inspectGitCloseReadiness(workspace: ExecutionWorkspace): Promise< if (repoRoot && baseRef) { try { const counts = (await runGit(["rev-list", "--left-right", "--count", `${baseRef}...HEAD`], workspacePath)).stdout.trim(); - const [behindRaw, aheadRaw] = counts.split(/\s+/); - behindCount = behindRaw ? Number.parseInt(behindRaw, 10) : 0; - aheadCount = aheadRaw ? Number.parseInt(aheadRaw, 10) : 0; + const match = /^(\d+)\s+(\d+)$/.exec(counts); + if (!match) throw new Error("Git returned invalid ahead/behind counts"); + behindCount = Number(match[1]); + aheadCount = Number(match[2]); + isMergedIntoBase = aheadCount === 0; } catch (error) { warnings.push( `Could not compare this workspace against ${baseRef}: ${error instanceof Error ? error.message : String(error)}`, ); } - - try { - await runGit(["merge-base", "--is-ancestor", "HEAD", baseRef], workspacePath); - isMergedIntoBase = true; - } catch (error) { - const code = typeof error === "object" && error && "code" in error ? (error as { code?: unknown }).code : null; - if (code === 1) isMergedIntoBase = false; - else { - warnings.push( - `Could not determine whether this workspace is merged into ${baseRef}: ${error instanceof Error ? error.message : String(error)}`, - ); - } - } } return { @@ -1748,7 +1737,7 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic } let mergedPullRequest = false; let pullRequestStateUnknown = false; - const workspaceHeadSha = git?.repoRoot && git.workspacePath + const workspaceHeadSha = sourceIssueTerminal && git?.repoRoot && git.workspacePath ? await runGit(["rev-parse", "HEAD"], git.workspacePath) .then((result) => result.stdout.trim() || null) .catch(() => null) @@ -2793,6 +2782,12 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic return null; }, + getStoredById: async (companyId: string, id: string) => { + const [row] = await db.select().from(executionWorkspaces).where(eq(executionWorkspaces.id, id)); + if (row && row.companyId !== companyId) throw forbidden("Task workspace access is no longer available"); + return row ? toExecutionWorkspace(row) : null; + }, + getById: async (id: string) => { const row = await db .select() diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index b4c464fe871..1d17cbc8cd8 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -2251,7 +2251,7 @@ export function heartbeatService( ); const contextProjectId = readNonEmptyString(context.projectId); const boundSourceWorkspace = issueContext?.executionWorkspaceId - ? await executionWorkspacesSvc.getById(issueContext.executionWorkspaceId) : null; + ? await executionWorkspacesSvc.getStoredById(agent.companyId, issueContext.executionWorkspaceId) : null; const [selectedWorkspaceSource] = !boundSourceWorkspace && issueContext?.projectWorkspaceId ? await db.select({ projectId: projectWorkspaces.projectId }).from(projectWorkspaces) .where(and(eq(projectWorkspaces.id, issueContext.projectWorkspaceId), eq(projectWorkspaces.companyId, agent.companyId))) : []; diff --git a/server/src/services/heartbeat/environment-selection.test.ts b/server/src/services/heartbeat/environment-selection.test.ts index 5403f8164ef..884ddf1d1dc 100644 --- a/server/src/services/heartbeat/environment-selection.test.ts +++ b/server/src/services/heartbeat/environment-selection.test.ts @@ -89,6 +89,27 @@ describe.skipIf(!support.supported)("heartbeat environment selection boundary", expect(result.persistedNativeExecutionInput?.binding.executionWorkspaceId).toBe(original.id); }); + it("selects the saved execution workspace without inspecting live Git delivery state", async () => { + const { input, workspace, recover } = await fixture(); + const saved = await workspace(); + recover(saved.id); + input.services.executionWorkspacesSvc = executionWorkspaceService(db, { + inspectGitCloseReadiness: async () => { + throw new Error("Live Git inspection is unavailable"); + }, + }); + + const result = await selectHeartbeatEnvironment(db, input); + + expect(result.reusableExistingExecutionWorkspace).toMatchObject({ + id: saved.id, + companyId: input.agent.companyId, + cwd: home, + strategyType: "git_worktree", + }); + expect(result.nativeRecoveryExecutionWorkspaceId).toBe(saved.id); + }); + it("does not treat a synthetic run-id recovery binding as a persisted workspace", async () => { const { input, recover } = await fixture(); recover(input.run.id); diff --git a/server/src/services/heartbeat/environment-selection.ts b/server/src/services/heartbeat/environment-selection.ts index f0436a60727..8a732a90dba 100644 --- a/server/src/services/heartbeat/environment-selection.ts +++ b/server/src/services/heartbeat/environment-selection.ts @@ -64,7 +64,7 @@ export interface HeartbeatEnvironmentSelectionInput { issueAssigneeOverrides: { adapterConfig: Record | null } | null; }; services: { - executionWorkspacesSvc: Pick, "getById" | "validateSelection">; + executionWorkspacesSvc: Pick, "getStoredById" | "validateSelection">; environmentsSvc: Pick, "ensureLocalEnvironment" | "findManagedSandboxEnvironment" | "findKubernetesEnvironment" | "ensureKubernetesEnvironment" | "getById">; instanceSettings: Pick, "get" | "getExperimental">; @@ -105,7 +105,7 @@ export async function selectHeartbeatEnvironment(db: Db, input: HeartbeatEnviron persistedNativeExecutionWorkspaceId ?? readNonEmptyString(issueRef?.executionWorkspaceId); const existingExecutionWorkspace = requestedExecutionWorkspaceId - ? await executionWorkspacesSvc.getById(requestedExecutionWorkspaceId) + ? await executionWorkspacesSvc.getStoredById(agent.companyId, requestedExecutionWorkspaceId) : null; const nativeRecoveryExecutionWorkspaceId = resolveNativeRecoveryExecutionWorkspaceBinding({ diff --git a/server/src/services/heartbeat/runtime-execution.ts b/server/src/services/heartbeat/runtime-execution.ts index bb5f5b99fe2..80e4bf183c5 100644 --- a/server/src/services/heartbeat/runtime-execution.ts +++ b/server/src/services/heartbeat/runtime-execution.ts @@ -175,7 +175,7 @@ export interface HeartbeatRuntimeExecutionInput { Pick & Pick, "cancelRunInternal"> & Pick, "enqueueWakeup" | "dispatchPendingNativeStatusWakeups"> & { - executionWorkspacesSvc: Pick, "getById">; + executionWorkspacesSvc: Pick, "getStoredById">; instanceSettings: Pick, "getExperimental">; envOrchestrator: Pick, "releaseForRun">; }; @@ -266,7 +266,8 @@ export async function executeHeartbeatRuntime(db: Db, input: HeartbeatRuntimeExe let adapterFinalizeOutcome: "succeeded" | "failed" | null = null; const inspectFinalizeWorkspaceBranch = async () => { const workspaceRecord = persistedExecutionWorkspace?.id - ? await executionWorkspacesSvc.getById( + ? await executionWorkspacesSvc.getStoredById( + agent.companyId, persistedExecutionWorkspace.id, ) : persistedExecutionWorkspace;