From a492aaf381e608872d3ff1cbcac603e1d9364ea7 Mon Sep 17 00:00:00 2001 From: Ronaldo Martins Date: Mon, 10 Aug 2026 17:50:26 -0300 Subject: [PATCH 1/2] perf(api): convert sync I/O to async in diff-validator; cache version (ENG-1667) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - diff-validator.ts: mkdtemp/writeFile/unlink/rm/mkdir via fs.promises - applyFileChanges now async with parallel writes (Promise.all) — previously writeFileSync inside a loop blocked the event loop - cleanupTempDir async (awaited in validateDiff finally) - version.ts: in-memory cache, reads package.json at most once per process - process-exit cleanup handler intentionally kept sync (exit handlers cannot await) --- packages/api/src/core/diff-validator.ts | 75 ++++++++++++++----------- packages/api/src/lib/version.ts | 31 +++++++--- 2 files changed, 66 insertions(+), 40 deletions(-) diff --git a/packages/api/src/core/diff-validator.ts b/packages/api/src/core/diff-validator.ts index d379fdc..274beed 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,42 @@ 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 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. */ -function applyFileChanges(tempDir: string, files: DiffFile[]): void { - for (const file of files) { - const fullPath = path.join(tempDir, file.path); - const dir = path.dirname(fullPath); - - if (file.deleted) { - if (fs.existsSync(fullPath)) { - fs.unlinkSync(fullPath); +async function applyFileChanges( + tempDir: string, + files: DiffFile[], +): Promise { + await Promise.all( + files.map(async (file) => { + 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; } - continue; - } - // Ensure directory exists - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true }); - } - - fs.writeFileSync(fullPath, file.content, "utf-8"); - } + // 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"); + }), + ); } /** @@ -623,8 +632,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 +648,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; +} From 92669615c0539f38db37e06939ecff6b35535902 Mon Sep 17 00:00:00 2001 From: Ronaldo Martins Date: Mon, 10 Aug 2026 21:44:14 -0300 Subject: [PATCH 2/2] fix(api): serialize path-conflicting file ops in applyFileChanges (ENG-1667) applyFileChanges parallelized all file writes/deletes via Promise.all with no ordering guarantee between operations on conflicting paths. Deleting a file while adding a file under a directory of the same name (or the inverse) could race mkdir/writeFile against rm and throw EEXIST/EISDIR/ENOTDIR depending on scheduling. Group file operations by path-prefix conflict (union-find over normalized paths where one path is an ancestor of another), run conflicting groups sequentially with deletes before creates, and keep independent groups running in parallel via Promise.all. Also guard the write path against a stale directory left behind at the exact write target after its last child was deleted. Export applyFileChanges for direct unit testing; add regression tests for file->dir and dir->file replacement in the same changeset, plus coverage for delete-before-create ordering regardless of input order and for independent paths remaining parallel. --- packages/api/src/core/diff-validator.test.ts | 182 +++++++++++++++++++ packages/api/src/core/diff-validator.ts | 122 +++++++++++-- 2 files changed, 292 insertions(+), 12 deletions(-) create mode 100644 packages/api/src/core/diff-validator.test.ts 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 274beed..14102a2 100644 --- a/packages/api/src/core/diff-validator.ts +++ b/packages/api/src/core/diff-validator.ts @@ -238,29 +238,127 @@ async function cleanupTempDir(tempDir: string): Promise { } } +/** + * Apply a single file change (write or delete) to the temp directory. + */ +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 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; + } + + 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); + } + } + } + + 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. */ -async function applyFileChanges( +export async function applyFileChanges( tempDir: string, files: DiffFile[], ): Promise { - await Promise.all( - files.map(async (file) => { - const fullPath = path.join(tempDir, file.path); - const dir = path.dirname(fullPath); + const groups = groupConflictingFiles(files); - if (file.deleted) { - // rm with force ignores missing files (no existsSync needed) - await fs.promises.rm(fullPath, { force: true }); + await Promise.all( + groups.map(async (group) => { + if (group.length === 1) { + await applyOneFileChange(tempDir, group[0] as DiffFile); return; } - - // 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"); + // 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); + } }), ); }