mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-22 00:30:19 +02:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
4460785534
commit
ad0bd26bb7
8 files changed
+186
-22
No files matched your search
@@ -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.
|
||||
|
||||
@@ -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<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | undefined;
|
||||
let db: ReturnType<typeof createDb> | 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<Awaited<ReturnType<typeof service.getById>>>) => {
|
||||
const { runtimeServices: _services, deliveryState: _delivery, ...record } = workspace;
|
||||
return record;
|
||||
};
|
||||
const results = [];
|
||||
let expected: ReturnType<typeof persisted>[] | 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); }
|
||||
}
|
||||
@@ -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")
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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))) : [];
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -64,7 +64,7 @@ export interface HeartbeatEnvironmentSelectionInput {
|
||||
issueAssigneeOverrides: { adapterConfig: Record<string, unknown> | null } | null;
|
||||
};
|
||||
services: {
|
||||
executionWorkspacesSvc: Pick<ReturnType<typeof executionWorkspaceService>, "getById" | "validateSelection">;
|
||||
executionWorkspacesSvc: Pick<ReturnType<typeof executionWorkspaceService>, "getStoredById" | "validateSelection">;
|
||||
environmentsSvc: Pick<ReturnType<typeof environmentService>,
|
||||
"ensureLocalEnvironment" | "findManagedSandboxEnvironment" | "findKubernetesEnvironment" | "ensureKubernetesEnvironment" | "getById">;
|
||||
instanceSettings: Pick<ReturnType<typeof instanceSettingsService>, "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({
|
||||
|
||||
@@ -175,7 +175,7 @@ export interface HeartbeatRuntimeExecutionInput {
|
||||
Pick<Lifecycle, "persistRunProcessMetadata" | "recordCurrentHeartbeatRunRuntimeProgress"> &
|
||||
Pick<ReturnType<typeof createHeartbeatRunControl>, "cancelRunInternal"> &
|
||||
Pick<ReturnType<typeof createHeartbeatQueue>, "enqueueWakeup" | "dispatchPendingNativeStatusWakeups"> & {
|
||||
executionWorkspacesSvc: Pick<ReturnType<typeof executionWorkspaceService>, "getById">;
|
||||
executionWorkspacesSvc: Pick<ReturnType<typeof executionWorkspaceService>, "getStoredById">;
|
||||
instanceSettings: Pick<ReturnType<typeof instanceSettingsService>, "getExperimental">;
|
||||
envOrchestrator: Pick<ReturnType<typeof environmentRunOrchestrator>, "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;
|
||||
|
||||
Reference in new issue
Block a user