mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-08 19:56:31 +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>
78 lines
3.7 KiB
TypeScript
78 lines
3.7 KiB
TypeScript
import { execFile } from "node:child_process";
|
|
import fs from "node:fs/promises";
|
|
import os from "node:os";
|
|
import path from "node:path";
|
|
import { promisify } from "node:util";
|
|
import { afterEach, expect, it } from "vitest";
|
|
import { runWorkspaceGitProcess } from "./workspace-git-stream.js";
|
|
|
|
const exec = promisify(execFile);
|
|
const roots: string[] = [];
|
|
const git = (cwd: string, args: string[]) => exec("git", args, {
|
|
cwd, env: { ...process.env, GIT_OPTIONAL_LOCKS: "1" },
|
|
});
|
|
|
|
afterEach(async () => {
|
|
await Promise.all(roots.splice(0).map((root) => fs.rm(root, { recursive: true, force: true })));
|
|
});
|
|
|
|
async function repository() {
|
|
const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-git-scan-locks-"));
|
|
roots.push(cwd);
|
|
await git(cwd, ["init"]);
|
|
await git(cwd, ["config", "user.name", "Test"]);
|
|
await git(cwd, ["config", "user.email", "test@example.com"]);
|
|
await fs.writeFile(path.join(cwd, "tracked.txt"), "tracked content\n");
|
|
await git(cwd, ["add", "tracked.txt"]);
|
|
await git(cwd, ["commit", "-m", "Initial fixture"]);
|
|
return cwd;
|
|
}
|
|
|
|
function scan(cwd: string, args: string[], env?: NodeJS.ProcessEnv) {
|
|
return runWorkspaceGitProcess({ cwd, args, env, timeoutMs: 5000, maxStdoutBytes: 8192, maxStderrBytes: 8192 });
|
|
}
|
|
|
|
it.each([false, true])("does not refresh a clean index during a background status scan (explicit env: %s)", async (explicitEnv) => {
|
|
const cwd = await repository();
|
|
const index = path.join(cwd, ".git", "index");
|
|
const before = await fs.readFile(index);
|
|
// Force a stale stat cache without changing file content. Ordinary status
|
|
// rewrites the index, while a background scan must only report its result.
|
|
await fs.utimes(path.join(cwd, "tracked.txt"), new Date(1000), new Date(1000));
|
|
const env = explicitEnv ? { ...process.env, GIT_OPTIONAL_LOCKS: "1" } : undefined;
|
|
const result = await scan(cwd, ["status", "--porcelain", "--untracked-files=all"], env);
|
|
expect(result.stdout).toBe("");
|
|
expect(await fs.readFile(index)).toEqual(before);
|
|
await git(cwd, ["status", "--porcelain"]);
|
|
expect(await fs.readFile(index)).not.toEqual(before);
|
|
});
|
|
|
|
it("reports changes beside an existing lock without removing it or bypassing mandatory write locks", async () => {
|
|
const cwd = await repository();
|
|
const lock = path.join(cwd, ".git", "index.lock");
|
|
await fs.writeFile(lock, "another writer owns this lock\n", { flag: "wx" });
|
|
await fs.writeFile(path.join(cwd, "tracked.txt"), "uncommitted work\n");
|
|
await fs.writeFile(path.join(cwd, "untracked.txt"), "scratch\n");
|
|
const result = await scan(cwd, ["status", "--porcelain", "--untracked-files=all"]);
|
|
expect(result.stdout).toContain(" M tracked.txt");
|
|
expect(result.stdout).toContain("?? untracked.txt");
|
|
await expect(scan(cwd, ["reset", "--hard", "HEAD"])).rejects.toMatchObject({
|
|
code: "workspace_git_scan_failed", details: { stderr: expect.stringContaining("index.lock") },
|
|
});
|
|
expect(await fs.readFile(lock, "utf8")).toBe("another writer owns this lock\n");
|
|
expect(await fs.readFile(path.join(cwd, "tracked.txt"), "utf8")).toBe("uncommitted work\n");
|
|
expect(await fs.readFile(path.join(cwd, "untracked.txt"), "utf8")).toBe("scratch\n");
|
|
});
|
|
|
|
it("preserves caller Git configuration without mutating the supplied environment", async () => {
|
|
const cwd = await repository();
|
|
await fs.writeFile(path.join(cwd, "untracked.txt"), "scratch\n");
|
|
const env = {
|
|
...process.env, GIT_OPTIONAL_LOCKS: "1", GIT_CONFIG_COUNT: "1",
|
|
GIT_CONFIG_KEY_0: "status.showUntrackedFiles", GIT_CONFIG_VALUE_0: "no",
|
|
};
|
|
expect((await scan(cwd, ["status", "--porcelain"], env)).stdout).toBe("");
|
|
expect((await git(cwd, ["status", "--porcelain", "--untracked-files=all"])).stdout).toContain("?? untracked.txt");
|
|
expect(env.GIT_OPTIONAL_LOCKS).toBe("1");
|
|
});
|