diff --git a/packages/api/src/core/diff-validator.test.ts b/packages/api/src/core/diff-validator.test.ts new file mode 100644 index 0000000..43a45b6 --- /dev/null +++ b/packages/api/src/core/diff-validator.test.ts @@ -0,0 +1,182 @@ +import { describe, it, expect, afterEach } from "bun:test"; +import * as fs from "fs"; +import * as path from "path"; +import * as os from "os"; +import { applyFileChanges, type DiffFile } from "./diff-validator"; + +const tempDirs: string[] = []; + +function makeTempDir(): string { + const dir = fs.mkdtempSync( + path.join(os.tmpdir(), "diff-validator-test-"), + ); + tempDirs.push(dir); + return dir; +} + +afterEach(() => { + while (tempDirs.length > 0) { + const dir = tempDirs.pop(); + if (dir) { + fs.rmSync(dir, { recursive: true, force: true }); + } + } +}); + +describe("applyFileChanges", () => { + it("writes new files and creates missing directories", async () => { + const tempDir = makeTempDir(); + const files: DiffFile[] = [ + { path: "src/index.ts", content: "export {};", deleted: false }, + { path: "src/nested/util.ts", content: "export const x = 1;", deleted: false }, + ]; + + await applyFileChanges(tempDir, files); + + expect(fs.readFileSync(path.join(tempDir, "src/index.ts"), "utf-8")).toBe( + "export {};", + ); + expect( + fs.readFileSync(path.join(tempDir, "src/nested/util.ts"), "utf-8"), + ).toBe("export const x = 1;"); + }); + + it("deletes files that exist", async () => { + const tempDir = makeTempDir(); + fs.mkdirSync(path.join(tempDir, "src"), { recursive: true }); + fs.writeFileSync(path.join(tempDir, "src/old.ts"), "old content"); + + const files: DiffFile[] = [ + { path: "src/old.ts", content: "", deleted: true }, + ]; + + await applyFileChanges(tempDir, files); + + expect(fs.existsSync(path.join(tempDir, "src/old.ts"))).toBe(false); + }); + + it("deleting a missing file does not throw (force rm)", async () => { + const tempDir = makeTempDir(); + const files: DiffFile[] = [ + { path: "does/not/exist.ts", content: "", deleted: true }, + ]; + + await expect(applyFileChanges(tempDir, files)).resolves.toBeUndefined(); + }); + + it("applies independent (non-conflicting) file changes in parallel without error", async () => { + const tempDir = makeTempDir(); + const files: DiffFile[] = Array.from({ length: 20 }, (_, i) => ({ + path: `pkg-${i}/file.ts`, + content: `export const n = ${i};`, + deleted: false, + })); + + await applyFileChanges(tempDir, files); + + for (let i = 0; i < 20; i++) { + expect( + fs.readFileSync(path.join(tempDir, `pkg-${i}/file.ts`), "utf-8"), + ).toBe(`export const n = ${i};`); + } + }); + + // Regression: ENG-1667 review finding — Promise.all gave no ordering + // guarantee between operations on conflicting paths, so replacing a file + // with a directory of the same name (or vice versa) could race + // mkdir/writeFile against rm and throw EEXIST/EISDIR/ENOTDIR. + describe("path-conflict ordering (ENG-1667 regression)", () => { + it("file -> dir: deleting a file while adding a file under a directory of the same name", async () => { + const tempDir = makeTempDir(); + // Pre-existing tracked file at "foo" + fs.writeFileSync(path.join(tempDir, "foo"), "old file content"); + + const files: DiffFile[] = [ + { path: "foo", content: "", deleted: true }, + { path: "foo/bar.ts", content: "export const bar = 1;", deleted: false }, + ]; + + await applyFileChanges(tempDir, files); + + expect(fs.existsSync(path.join(tempDir, "foo"))).toBe(true); + expect(fs.statSync(path.join(tempDir, "foo")).isDirectory()).toBe(true); + expect( + fs.readFileSync(path.join(tempDir, "foo/bar.ts"), "utf-8"), + ).toBe("export const bar = 1;"); + }); + + it("dir -> file: deleting all files under a directory while adding a file at the directory's path", async () => { + const tempDir = makeTempDir(); + // Pre-existing tracked directory "foo/" containing a file + fs.mkdirSync(path.join(tempDir, "foo"), { recursive: true }); + fs.writeFileSync(path.join(tempDir, "foo/bar.ts"), "export const bar = 1;"); + + const files: DiffFile[] = [ + { path: "foo/bar.ts", content: "", deleted: true }, + { path: "foo", content: "export const foo = 2;", deleted: false }, + ]; + + await applyFileChanges(tempDir, files); + + expect(fs.statSync(path.join(tempDir, "foo")).isFile()).toBe(true); + expect(fs.readFileSync(path.join(tempDir, "foo"), "utf-8")).toBe( + "export const foo = 2;", + ); + }); + + it("orders deletes before creates within a conflicting group regardless of input order", async () => { + const tempDir = makeTempDir(); + fs.writeFileSync(path.join(tempDir, "shared"), "old file content"); + + // create listed before delete in the input array + const files: DiffFile[] = [ + { + path: "shared/child.ts", + content: "export const child = true;", + deleted: false, + }, + { path: "shared", content: "", deleted: true }, + ]; + + await applyFileChanges(tempDir, files); + + expect(fs.statSync(path.join(tempDir, "shared")).isDirectory()).toBe( + true, + ); + expect( + fs.readFileSync(path.join(tempDir, "shared/child.ts"), "utf-8"), + ).toBe("export const child = true;"); + }); + + it("does not serialize unrelated path groups (independent paths remain unaffected by a conflicting group)", async () => { + const tempDir = makeTempDir(); + fs.writeFileSync(path.join(tempDir, "conflict"), "old"); + + const files: DiffFile[] = [ + { path: "conflict", content: "", deleted: true }, + { + path: "conflict/child.ts", + content: "export const child = 1;", + deleted: false, + }, + { path: "independent/a.ts", content: "export const a = 1;", deleted: false }, + { path: "independent/b.ts", content: "export const b = 2;", deleted: false }, + ]; + + await applyFileChanges(tempDir, files); + + expect(fs.statSync(path.join(tempDir, "conflict")).isDirectory()).toBe( + true, + ); + expect( + fs.readFileSync(path.join(tempDir, "conflict/child.ts"), "utf-8"), + ).toBe("export const child = 1;"); + expect( + fs.readFileSync(path.join(tempDir, "independent/a.ts"), "utf-8"), + ).toBe("export const a = 1;"); + expect( + fs.readFileSync(path.join(tempDir, "independent/b.ts"), "utf-8"), + ).toBe("export const b = 2;"); + }); + }); +}); diff --git a/packages/api/src/core/diff-validator.ts b/packages/api/src/core/diff-validator.ts index d379fdc..14102a2 100644 --- a/packages/api/src/core/diff-validator.ts +++ b/packages/api/src/core/diff-validator.ts @@ -124,7 +124,9 @@ async function cloneRepo( ): Promise { registerCleanupHandlers(); - const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "diff-validate-")); + const tempDir = await fs.promises.mkdtemp( + path.join(os.tmpdir(), "diff-validate-"), + ); tempDirs.add(tempDir); const token = process.env.GITHUB_TOKEN; @@ -138,9 +140,11 @@ async function cloneRepo( // Write credentials to temp file that git will use const credentialFile = path.join(tempDir, ".git-credentials"); - fs.writeFileSync(credentialFile, `https://oauth2:${token}@github.com\n`, { - mode: 0o600, - }); + await fs.promises.writeFile( + credentialFile, + `https://oauth2:${token}@github.com\n`, + { mode: 0o600 }, + ); const envWithCredentials = { GIT_ASKPASS: "echo", @@ -170,7 +174,7 @@ async function cloneRepo( } finally { // Clean up credentials file immediately try { - fs.unlinkSync(credentialFile); + await fs.promises.unlink(credentialFile); } catch { // Ignore } @@ -179,9 +183,11 @@ async function cloneRepo( if (cloneResult.exitCode !== 0) { // Try cloning main/master if branch doesn't exist yet const credentialFile2 = path.join(tempDir, ".git-credentials"); - fs.writeFileSync(credentialFile2, `https://oauth2:${token}@github.com\n`, { - mode: 0o600, - }); + await fs.promises.writeFile( + credentialFile2, + `https://oauth2:${token}@github.com\n`, + { mode: 0o600 }, + ); let mainResult: { exitCode: number; stdout: string; stderr: string }; try { @@ -202,7 +208,7 @@ async function cloneRepo( ); } finally { try { - fs.unlinkSync(credentialFile2); + await fs.promises.unlink(credentialFile2); } catch { // Ignore } @@ -221,39 +227,140 @@ async function cloneRepo( } /** - * Remove temp directory and untrack it + * Remove temp directory and untrack it (async — ENG-1667) */ -function cleanupTempDir(tempDir: string): void { +async function cleanupTempDir(tempDir: string): Promise { tempDirs.delete(tempDir); try { - fs.rmSync(tempDir, { recursive: true, force: true }); + await fs.promises.rm(tempDir, { recursive: true, force: true }); } catch { // Ignore cleanup errors } } /** - * Apply file changes to the temp directory + * Apply a single file change (write or delete) to the temp directory. */ -function applyFileChanges(tempDir: string, files: DiffFile[]): void { - for (const file of files) { - const fullPath = path.join(tempDir, file.path); - const dir = path.dirname(fullPath); +async function applyOneFileChange( + tempDir: string, + file: DiffFile, +): Promise { + const fullPath = path.join(tempDir, file.path); + const dir = path.dirname(fullPath); + + if (file.deleted) { + // rm with force ignores missing files (no existsSync needed) + await fs.promises.rm(fullPath, { force: true }); + return; + } - if (file.deleted) { - if (fs.existsSync(fullPath)) { - fs.unlinkSync(fullPath); - } - continue; + // If a conflicting group left a stale directory at this exact path (e.g. + // the last file under it was just deleted, but the now-empty directory + // remains), remove it before writing — writeFile fails with EISDIR + // otherwise. Sibling deletes always run before this create within the + // same group, but an emptied directory itself isn't cleaned up by rm-ing + // its children. + const existingStat = await fs.promises.stat(fullPath).catch(() => null); + if (existingStat?.isDirectory()) { + await fs.promises.rm(fullPath, { recursive: true, force: true }); + } + + // Ensure directory exists (recursive mkdir is a no-op if present) + await fs.promises.mkdir(dir, { recursive: true }); + await fs.promises.writeFile(fullPath, file.content, "utf-8"); +} + +/** + * True if `a` and `b` are the same path, or one is an ancestor directory of + * the other (e.g. "foo" and "foo/bar.ts"). Such pairs are path-dependent: + * replacing a tracked file with a directory of the same name (or vice + * versa) requires the delete to complete before the create runs, or a + * parallel mkdir/rm race can throw EEXIST/EISDIR/ENOTDIR. + */ +function pathsConflict(a: string, b: string): boolean { + if (a === b) return true; + const aWithSep = a.endsWith(path.sep) ? a : a + path.sep; + const bWithSep = b.endsWith(path.sep) ? b : b + path.sep; + return aWithSep.startsWith(bWithSep) || bWithSep.startsWith(aWithSep); +} + +/** + * Partition files into disjoint groups such that any two files with a + * path-dependent relationship (see pathsConflict) end up in the same group. + * Uses union-find over normalized relative paths. + */ +function groupConflictingFiles(files: DiffFile[]): DiffFile[][] { + const normalized = files.map((f) => path.normalize(f.path)); + const parent = files.map((_, i) => i); + + function find(i: number): number { + while (parent[i] !== i) { + parent[i] = parent[parent[i] as number] as number; + i = parent[i] as number; } + return i; + } + + function union(i: number, j: number): void { + const ri = find(i); + const rj = find(j); + if (ri !== rj) parent[ri] = rj; + } - // Ensure directory exists - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true }); + for (let i = 0; i < files.length; i++) { + for (let j = i + 1; j < files.length; j++) { + if (pathsConflict(normalized[i] as string, normalized[j] as string)) { + union(i, j); + } } + } - fs.writeFileSync(fullPath, file.content, "utf-8"); + const groups = new Map(); + for (let i = 0; i < files.length; i++) { + const root = find(i); + const group = groups.get(root); + if (group) { + group.push(files[i] as DiffFile); + } else { + groups.set(root, [files[i] as DiffFile]); + } } + return Array.from(groups.values()); +} + +/** + * Apply file changes to the temp directory. + * Async with parallel writes via Promise.all (ENG-1667) — previously used + * writeFileSync inside a loop, blocking the event loop per modified file. + * + * Path-dependent changes (e.g. deleting `foo` while adding `foo/bar.ts`) + * are grouped together and applied sequentially within the group, deletes + * before creates, so a directory-for-file (or file-for-directory) + * replacement never races an mkdir/writeFile against a still-present + * conflicting entry (EEXIST/EISDIR/ENOTDIR). Groups with no path overlap + * still run fully in parallel. + */ +export async function applyFileChanges( + tempDir: string, + files: DiffFile[], +): Promise { + const groups = groupConflictingFiles(files); + + await Promise.all( + groups.map(async (group) => { + if (group.length === 1) { + await applyOneFileChange(tempDir, group[0] as DiffFile); + return; + } + // Deletes first, then creates/updates, both in original diff order, + // so e.g. delete `foo` fully completes before create `foo/bar.ts`. + const deletes = group.filter((f) => f.deleted); + const creates = group.filter((f) => !f.deleted); + for (const file of [...deletes, ...creates]) { + await applyOneFileChange(tempDir, file); + } + }), + ); } /** @@ -623,8 +730,8 @@ export async function validateDiff( try { tempDir = await cloneRepo(repoFullName, branch); - // Apply the file changes - applyFileChanges(tempDir, files); + // Apply the file changes (parallel async writes) + await applyFileChanges(tempDir, files); // Run typecheck const typecheckResult = await runTypecheck(tempDir); @@ -639,7 +746,7 @@ export async function validateDiff( } finally { // Cleanup temp directory if (tempDir) { - cleanupTempDir(tempDir); + await cleanupTempDir(tempDir); } } diff --git a/packages/api/src/lib/version.ts b/packages/api/src/lib/version.ts index 5add977..1de7765 100644 --- a/packages/api/src/lib/version.ts +++ b/packages/api/src/lib/version.ts @@ -1,25 +1,42 @@ import * as fs from 'fs'; import * as path from 'path'; +// In-memory cache (ENG-1667): package.json is immutable at runtime, so we +// read it from disk at most once per process instead of on every call. +let cachedVersion: string | null = null; + /** * Reads and returns the version string from package.json. - * + * The value is cached in memory after the first read. + * * @returns The version string from the project's package.json * @throws Error if package.json cannot be read or version field is missing */ export function getVersion(): string { + if (cachedVersion !== null) { + return cachedVersion; + } + const packageJsonPath = path.resolve(__dirname, '../../package.json'); - + if (!fs.existsSync(packageJsonPath)) { throw new Error(`package.json not found at ${packageJsonPath}`); } - + const packageJsonContent = fs.readFileSync(packageJsonPath, 'utf-8'); const packageJson = JSON.parse(packageJsonContent) as { version?: string }; - + if (!packageJson.version) { throw new Error('version field not found in package.json'); } - - return packageJson.version; -} \ No newline at end of file + + cachedVersion = packageJson.version; + return cachedVersion; +} + +/** + * Test-only helper to reset the in-memory cache. + */ +export function __resetVersionCacheForTests(): void { + cachedVersion = null; +}