mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-11 14:10:50 +02:00
## Thinking Path Paperclip runs background workspace scans alongside real Git writers. `git status` can refresh the index as an optional side effect, taking a lock that makes another operation fail. Disable optional locking in the shared scan subprocess so background observation does not compete with workspace updates. ## Linked Issues or Issue Description **What existing behavior does this improve?** Workspace Git scans used by changed-file browsing, cleanliness guards, and sandbox snapshots. **Current behavior** The scan process inherits Git's default optional-lock behavior. Even a clean `status` can rewrite stale stat-cache entries in the index and contend with a concurrent writer. **Proposed behavior** Always set `GIT_OPTIONAL_LOCKS=0` for the shared scan subprocess while preserving the selected environment and Git's required write locks. **Reason and benefit** Background reads stop creating avoidable index contention. Git documents this behavior and recommends disabling optional locks for background status: [background refresh](https://git-scm.com/docs/git-status#_background_refresh). **Breaking changes** None to scan results or required write locking. Later scans may repeat stat checks that would otherwise have been cached in the index. Related scan implementation: #11572, #14253. This avoids one known contention source; it does not identify every historical lock owner or repair abandoned locks. ## What Changed - Disable optional locking at the shared scan subprocess boundary, including explicit caller environments. - Test a clean status against a deliberately stale index and prove an ordinary status would rewrite it. - Test tracked/untracked results with an existing index lock, preservation of that lock and working files, and continued rejection of a mandatory-lock write. - Document the scan behavior and performance tradeoff. ## Verification - Focused stream, workspace-sync, and scheduler suites: 63 tests passed. - Full `pnpm -r typecheck` and `pnpm build` passed locally. Final head `effe6420c77bd18d36af6b093db3a564c04b8b38` passed all 53 CI checks, including complete test coverage, typecheck, build, and browser/runner gates; two unrelated checks intentionally skipped. - Full local test attempts initially had missing embedded-Postgres library symlinks; the dependency setup was repaired. Duplicate local full-suite runs were stopped after full CI completed. This PR does not claim a completed full local suite. - Review regression: real Git honors the supplied `GIT_CONFIG_*` setting and the input environment remains unchanged; all four direct subprocess cases passed. - Reviewed the diff for secrets, customer data, and internal references. ## Risks Low risk. Disabling optional index refresh can repeat filesystem stat work on later scans. Required locks remain enforced; no lock is removed, no failed reset is retried, and workspace mutation guards are unchanged. No schema changes. ## Model Used OpenAI GPT-6 via Codex, with repository inspection, code execution, and tests. Exact model build identifier is not exposed by this session. ## Checklist - [x] Thinking path and model are specified - [x] Checked ROADMAP.md; this is a maintenance correction, not planned feature work - [x] Searched for duplicate and related PRs - [x] Described the issue using the enhancement template - [x] No internal issue references, customer data, or private instance links - [x] Descriptive branch name - [x] Focused regression tests pass - [x] Added tests and updated documentation - [x] Risks documented - [x] Required validation and CI gates are green (full suite validated in CI; local scope documented above) - [x] Greptile is 5/5 with no unresolved findings - [x] I will address review comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
96 lines
4.5 KiB
TypeScript
96 lines
4.5 KiB
TypeScript
import { spawn, type ChildProcess } from "node:child_process";
|
|
import { WORKSPACE_STREAM_CHUNK_BYTES } from "./workspace-manifest.js";
|
|
|
|
export interface WorkspaceGitProcessInput {
|
|
cwd: string;
|
|
args: readonly string[];
|
|
env?: NodeJS.ProcessEnv;
|
|
signal?: AbortSignal;
|
|
timeoutMs: number;
|
|
killGraceMs?: number;
|
|
maxStdoutBytes: number;
|
|
maxStderrBytes: number;
|
|
onStdout?: (chunk: Buffer) => Promise<void> | void;
|
|
gitBinary?: string;
|
|
gitArgsPrefix?: readonly string[];
|
|
}
|
|
|
|
function failure(code: string, message: string, details: Record<string, unknown> = {}): Error {
|
|
return Object.assign(new Error(message), { code, details });
|
|
}
|
|
|
|
function signalProcess(child: ChildProcess, signal: NodeJS.Signals): void {
|
|
// The leader may have exited while a descendant still holds its pipes open.
|
|
if (process.platform !== "win32" && child.pid) {
|
|
try { process.kill(-child.pid, signal); return; } catch { /* direct child fallback */ }
|
|
}
|
|
try { child.kill(signal); } catch { /* close owns settlement */ }
|
|
}
|
|
|
|
/** Completion is a barrier for the process, its pipes, and the awaited sink. */
|
|
export async function runWorkspaceGitProcess(input: WorkspaceGitProcessInput): Promise<{ stdout: string; stderr: string }> {
|
|
if (input.signal?.aborted) throw failure("workspace_git_scan_cancelled", "Workspace Git scan was cancelled");
|
|
const child = spawn(input.gitBinary ?? "git", [...(input.gitArgsPrefix ?? []), "-C", input.cwd, ...input.args], {
|
|
// Background scans must not compete with real writers by refreshing the
|
|
// index as a side effect. Mandatory locks for writes remain enforced by Git.
|
|
cwd: input.cwd, env: { ...(input.env ?? process.env), GIT_OPTIONAL_LOCKS: "0" },
|
|
stdio: ["ignore", "pipe", "pipe"], detached: process.platform !== "win32", windowsHide: true,
|
|
});
|
|
let error: Error | null = null;
|
|
let killTimer: NodeJS.Timeout | undefined;
|
|
const terminate = (reason: Error) => {
|
|
if (error) return;
|
|
error = reason;
|
|
signalProcess(child, "SIGTERM");
|
|
killTimer = setTimeout(() => signalProcess(child, "SIGKILL"), input.killGraceMs ?? 250);
|
|
killTimer.unref();
|
|
};
|
|
const onAbort = () => terminate(failure("workspace_git_scan_cancelled", "Workspace Git scan was cancelled"));
|
|
input.signal?.addEventListener("abort", onAbort, { once: true });
|
|
if (input.signal?.aborted) onAbort();
|
|
const timeout = setTimeout(() => terminate(failure("workspace_git_scan_timeout", `Workspace Git scan timed out after ${input.timeoutMs}ms`, { timeoutMs: input.timeoutMs })), input.timeoutMs);
|
|
timeout.unref();
|
|
const closed = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => {
|
|
child.once("error", (cause) => terminate(failure("workspace_git_scan_failed", "Workspace Git scan could not start", { cause: cause.message })));
|
|
child.once("close", (code, signal) => resolve({ code, signal }));
|
|
});
|
|
const collect = async (stream: NonNullable<typeof child.stdout>, limit: number, sink?: WorkspaceGitProcessInput["onStdout"]) => {
|
|
const chunks: Buffer[] = [];
|
|
let bytes = 0;
|
|
try {
|
|
for await (const raw of stream) {
|
|
const buffer = Buffer.isBuffer(raw) ? raw : Buffer.from(raw);
|
|
if (error) continue;
|
|
if (sink) {
|
|
for (let offset = 0; offset < buffer.length; offset += WORKSPACE_STREAM_CHUNK_BYTES) {
|
|
if (error) break;
|
|
await sink(buffer.subarray(offset, offset + WORKSPACE_STREAM_CHUNK_BYTES));
|
|
}
|
|
} else {
|
|
bytes += buffer.length;
|
|
if (bytes > limit) {
|
|
terminate(failure("workspace_git_scan_output_limit", "Workspace Git scan exceeded its output limit"));
|
|
} else chunks.push(buffer);
|
|
}
|
|
}
|
|
} catch (cause) {
|
|
terminate(failure("workspace_git_scan_failed", "Workspace Git scan stream failed", { cause: cause instanceof Error ? cause.message : String(cause) }));
|
|
}
|
|
return Buffer.concat(chunks).toString("utf8");
|
|
};
|
|
try {
|
|
const [outcome, stdout, stderr] = await Promise.all([
|
|
closed, collect(child.stdout!, input.maxStdoutBytes, input.onStdout), collect(child.stderr!, input.maxStderrBytes),
|
|
]);
|
|
if (error) throw error;
|
|
if (outcome.code !== 0) throw failure("workspace_git_scan_failed", "Workspace Git scan failed", {
|
|
exitCode: outcome.code, signal: outcome.signal, stderr: stderr.trim().slice(0, 1000),
|
|
});
|
|
return { stdout, stderr };
|
|
} finally {
|
|
clearTimeout(timeout);
|
|
if (killTimer) clearTimeout(killTimer);
|
|
input.signal?.removeEventListener("abort", onAbort);
|
|
}
|
|
}
|