From 6296fdf560f1ac4aa55985558edaa98300aacdbc Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 19 Oct 2026 08:36:53 +0800 Subject: [PATCH] fix(reference)!: preserve symlink races and root failure facts Classify replacement EINVAL at the readlink syscall so the reader and importer retain sibling observations. Give an admitted root's enumeration failure its own typed field, use it for unresolved symlink coverage, and project it as a graph limitation instead of an invalid dot-path child entry. Keep admission, cancellation and cleanup failures distinct. Closes #5135 Closes #5136 BREAKING CHANGE: Partial reference-source reader results report failed root enumeration in rootFailure instead of a failed directory child at path ".". Historical source graphs preserve the root failure as a top-level limitation. --- docs/cli.md | 6 + .../ReferenceSourceImportEntries.ts | 4 + src/reference/ReferenceSourceReader.test.ts | 27 +++ src/reference/ReferenceSourceReader.ts | 21 +- src/reference/ReferenceSourceReaderEntries.ts | 70 +++--- src/reference/ReferenceSourceReaderTypes.ts | 8 + .../referenceSourceCleanupOwnership.test.ts | 5 +- .../referenceSourceSymlinkRace.test.ts | 213 ++++++++++++++++++ 8 files changed, 312 insertions(+), 42 deletions(-) create mode 100644 tests/process-global/referenceSourceSymlinkRace.test.ts diff --git a/docs/cli.md b/docs/cli.md index c0f8eefff4..cad2255d91 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -286,6 +286,12 @@ reported separately as configured-secret exclusions. Re-import saved graphs whose pattern-based exclusions lack `pattern` before using them in source-to-bundle comparisons. +Entries that change during an import retain an unavailable-entry diagnostic +alongside readable siblings. If enumeration of an already admitted root fails, +the graph keeps its observed children and records the root failure as a +top-level limitation. That partial inventory does not establish absence among +unexamined children. + JavaScript and TypeScript import parsing requires valid UTF-8. Malformed source bytes retain their original hashes and sizes with a decoding diagnostic; REA does not infer module targets from replacement characters. diff --git a/src/application/ReferenceSourceImportEntries.ts b/src/application/ReferenceSourceImportEntries.ts index 84885a215e..d603201823 100644 --- a/src/application/ReferenceSourceImportEntries.ts +++ b/src/application/ReferenceSourceImportEntries.ts @@ -419,6 +419,10 @@ export const parseReferenceSourceEntries = ( parseFailures: [], limitations: [...read.limitations], }; + if (read.rootFailure !== undefined) + output.limitations.push( + `Reference root enumeration is incomplete: ${read.rootFailure.message}`, + ); const packageMainInventory = readPackageMains(read); const context: SpecifierContext = { lookupPath: createReferenceSourcePathLookup(filePaths), diff --git a/src/reference/ReferenceSourceReader.test.ts b/src/reference/ReferenceSourceReader.test.ts index 2eef9ec319..8cb73cae9b 100644 --- a/src/reference/ReferenceSourceReader.test.ts +++ b/src/reference/ReferenceSourceReader.test.ts @@ -30,6 +30,33 @@ const exclusions = ( ): TraversalState["exclusions"] => items; describe("unobserved internal symlink reconciliation", () => { + it("uses the explicit root failure only when no closer completed directory or exclusion covers the target", () => { + const entries: ReferenceSourceEntry[] = [ + internalLink("unobserved", "unseen/target.txt"), + internalLink("complete", "known/target.txt"), + internalLink("excluded", "omitted/target.txt"), + { status: "read", kind: "directory", path: "known" }, + ]; + + reconcileUnobservedSymlinkTargets( + entries, + exclusions({ path: "omitted", kind: "directory" }), + undefined, + { code: "changed", message: "Reference root changed" }, + ); + + expect(entries[0]).toMatchObject({ + status: "failed", + kind: "symlink", + code: "changed", + message: expect.stringContaining("Reference root changed"), + }); + expect(entries.slice(1, 3)).toEqual([ + internalLink("complete", "known/target.txt"), + internalLink("excluded", "omitted/target.txt"), + ]); + }); + it("retains unreadable targets below the nearest failed directory", () => { const entries: ReferenceSourceEntry[] = [ internalLink("alias.txt", "hidden/target.txt"), diff --git a/src/reference/ReferenceSourceReader.ts b/src/reference/ReferenceSourceReader.ts index 88553bee2b..4af7da9de2 100644 --- a/src/reference/ReferenceSourceReader.ts +++ b/src/reference/ReferenceSourceReader.ts @@ -9,6 +9,7 @@ import { traverseDirectory } from "./ReferenceSourceReaderEntries.js"; import type { ReferenceSourceReaderOptions, ReferenceSourceEntry, + ReferenceSourceRootFailure, ReferenceSourceRead, ReferenceSourceResult, TraversalState, @@ -78,12 +79,16 @@ const readSource = async ( state.entries, state.exclusions, state.signal, + state.rootFailure, ); state.entries.sort((left, right) => compareUnicodeCodePoints(left.path, right.path), ); const read: ReferenceSourceRead = { root: canonicalRoot, + ...(state.rootFailure === undefined + ? {} + : { rootFailure: state.rootFailure }), entries: state.entries, bytesRead: state.bytesRead, limitations: [PATH_RACE_LIMITATION], @@ -101,6 +106,7 @@ export const reconcileUnobservedSymlinkTargets = ( entries: ReferenceSourceEntry[], exclusions: TraversalState["exclusions"] = [], signal?: AbortSignal, + rootFailure?: ReferenceSourceRootFailure, ): void => { const failedDirectories = new Map( entries.flatMap((entry) => @@ -111,7 +117,11 @@ export const reconcileUnobservedSymlinkTargets = ( : [], ), ); - if (failedDirectories.size === 0 || isAborted(signal)) return; + if ( + (failedDirectories.size === 0 && rootFailure === undefined) || + isAborted(signal) + ) + return; const directoryEntries = entries.filter( (entry) => entry.kind === "directory", ); @@ -140,6 +150,7 @@ export const reconcileUnobservedSymlinkTargets = ( failedDirectories, excludedPaths, signal, + rootFailure, ); if (ancestor === undefined) continue; entries[index] = { @@ -161,7 +172,8 @@ const nearestFailedAncestor = ( >, excludedPaths: ReturnType, signal: AbortSignal | undefined, -): Extract | undefined => { + rootFailure: ReferenceSourceRootFailure | undefined, +): (ReferenceSourceRootFailure & { readonly path: string }) | undefined => { for ( let candidate = posix.dirname(target); candidate !== "." && candidate !== "/"; @@ -175,10 +187,7 @@ const nearestFailedAncestor = ( } if (isAborted(signal)) return undefined; if (excludedPaths.resolve(".") !== undefined) return undefined; - const observedRoot = directoryPaths.resolve("."); - return observedRoot === undefined - ? undefined - : failedDirectories.get(observedRoot); + return rootFailure === undefined ? undefined : { path: ".", ...rootFailure }; }; const traverse = async ( diff --git a/src/reference/ReferenceSourceReaderEntries.ts b/src/reference/ReferenceSourceReaderEntries.ts index 1ca0ce3e2e..c7f634168e 100644 --- a/src/reference/ReferenceSourceReaderEntries.ts +++ b/src/reference/ReferenceSourceReaderEntries.ts @@ -26,10 +26,29 @@ import type { PendingDirectory, ReferenceSourceEntry, ReferenceSourceExclusionKind, + ReferenceSourceFailureCode, ReferenceSourceResult, TraversalState, } from "./ReferenceSourceReaderTypes.js"; +const recordDirectoryFailure = ( + state: TraversalState, + absolute: string, + code: ReferenceSourceFailureCode, + message: string, +): void => { + if (absolute === state.root) state.rootFailure = { code, message }; + else + state.entries.push( + entryFailure( + pathFromRoot(state.root, absolute), + "directory", + code, + message, + ), + ); +}; + export const traverseDirectory = async ( state: TraversalState, current: PendingDirectory, @@ -42,14 +61,7 @@ export const traverseDirectory = async ( ); if (isAborted(state.signal)) return { ok: false, error: cancelled() }; if (!before.ok) { - state.entries.push( - entryFailure( - pathFromRoot(state.root, current.path), - "directory", - before.code, - before.message, - ), - ); + recordDirectoryFailure(state, current.path, before.code, before.message); return { ok: true, value: undefined }; } const directories: PendingDirectory[] = []; @@ -86,14 +98,7 @@ export const traverseDirectory = async ( } if (cleanup.kind === "failed") { if (result.ok && !isAborted(state.signal) && readFailure !== undefined) - state.entries.push( - entryFailure( - pathFromRoot(state.root, current.path), - "directory", - "io", - readFailure, - ), - ); + recordDirectoryFailure(state, current.path, "io", readFailure); const observation = ArtifactReaderFailure.cleanupObservation( cleanup.cause, current.path, @@ -126,14 +131,7 @@ export const traverseDirectory = async ( } if (isAborted(state.signal)) return { ok: false, error: cancelled() }; if (readFailure !== undefined) { - state.entries.push( - entryFailure( - pathFromRoot(state.root, current.path), - "directory", - "io", - readFailure, - ), - ); + recordDirectoryFailure(state, current.path, "io", readFailure); scheduleObservedDirectories(state, directories); return { ok: true, value: undefined }; } @@ -145,13 +143,11 @@ export const traverseDirectory = async ( ); if (isAborted(state.signal)) return { ok: false, error: cancelled() }; if (!after.ok || !sameFile(before.stats, after.stats)) { - state.entries.push( - entryFailure( - pathFromRoot(state.root, current.path), - "directory", - "changed", - "Directory changed while it was read", - ), + recordDirectoryFailure( + state, + current.path, + after.ok ? "changed" : after.code, + after.ok ? "Directory changed while it was read" : after.message, ); scheduleObservedDirectories(state, directories); return { ok: true, value: undefined }; @@ -356,7 +352,17 @@ const readSymlinkTarget = async ( absolute: string, path: string, ): Promise => { - const rawTarget = await readlink(absolute); + let rawTarget: string; + try { + rawTarget = await readlink(absolute); + } catch (cause: unknown) { + // POSIX readlink reports EINVAL when the observed link became a non-link. + // Keep this classification at the syscall, rather than hiding EINVAL from + // unrelated metadata or target-resolution operations. + if (cause instanceof Error && Reflect.get(cause, "code") === "EINVAL") + return changedSymlink(path); + throw cause; + } const lexicalTarget = resolve(dirname(absolute), rawTarget); if (!isPathWithinRoot(root, lexicalTarget)) return { diff --git a/src/reference/ReferenceSourceReaderTypes.ts b/src/reference/ReferenceSourceReaderTypes.ts index 85d6d76029..a9e3f470bf 100644 --- a/src/reference/ReferenceSourceReaderTypes.ts +++ b/src/reference/ReferenceSourceReaderTypes.ts @@ -61,8 +61,15 @@ export type ReferenceSourceEntry = readonly size?: number; }; +/** A failure enumerating an already admitted root, separate from its children. */ +export interface ReferenceSourceRootFailure { + readonly code: ReferenceSourceFailureCode; + readonly message: string; +} + export interface ReferenceSourceRead { readonly root: string; + readonly rootFailure?: ReferenceSourceRootFailure; readonly entries: readonly ReferenceSourceEntry[]; readonly bytesRead: number; /** Node exposes no portable openat traversal; pathname identity is checked around each operation. */ @@ -98,6 +105,7 @@ export type TraversalState = { readonly kind: ReferenceSourceExclusionKind; }>; readonly pending: PendingDirectory[]; + rootFailure?: ReferenceSourceRootFailure; bytesRead: number; }; diff --git a/tests/boundary/filesystem/referenceSourceCleanupOwnership.test.ts b/tests/boundary/filesystem/referenceSourceCleanupOwnership.test.ts index 3ad4665bf4..e471bceafe 100644 --- a/tests/boundary/filesystem/referenceSourceCleanupOwnership.test.ts +++ b/tests/boundary/filesystem/referenceSourceCleanupOwnership.test.ts @@ -253,10 +253,7 @@ it("preserves a directory read failure when closing that directory also fails", bytes: Buffer.from(bytes), }), ); - expect(result.error.partial?.entries).toContainEqual({ - status: "failed", - kind: "directory", - path: ".", + expect(result.error.partial?.rootFailure).toEqual({ code: "io", message: expect.stringContaining(readFailure.message), }); diff --git a/tests/process-global/referenceSourceSymlinkRace.test.ts b/tests/process-global/referenceSourceSymlinkRace.test.ts new file mode 100644 index 0000000000..6229ab873d --- /dev/null +++ b/tests/process-global/referenceSourceSymlinkRace.test.ts @@ -0,0 +1,213 @@ +import { expect, it, vi } from "vitest"; +import type { PathLike } from "node:fs"; + +const race = vi.hoisted(() => ({ + path: "", + beforeReadlink: undefined as (() => Promise) | undefined, + actualErrno: undefined as string | undefined, + directory: "", + afterDirectoryClose: undefined as (() => Promise) | undefined, +})); + +vi.mock("node:fs/promises", async (importOriginal) => { + const filesystem = await importOriginal(); + return { + ...filesystem, + opendir: async (...args: Parameters) => { + const directory = await filesystem.opendir(...args); + if (String(args[0]) === race.directory) { + const close = directory.close.bind(directory); + vi.spyOn(directory, "close").mockImplementation(async () => { + await close(); + const hook = race.afterDirectoryClose; + race.afterDirectoryClose = undefined; + await hook?.(); + }); + } + return directory; + }, + readlink: async ( + path: PathLike, + ...args: Parameters extends [ + PathLike, + ...infer R, + ] + ? R + : never + ) => { + if (String(path) === race.path && race.beforeReadlink !== undefined) { + const beforeReadlink = race.beforeReadlink; + race.beforeReadlink = undefined; + await beforeReadlink(); + } + try { + return await filesystem.readlink(path, ...args); + } catch (cause: unknown) { + if (String(path) === race.path && cause instanceof Error) + race.actualErrno = Reflect.get(cause, "code"); + throw cause; + } + }, + }; +}); + +import { + mkdir, + readFile, + rename, + unlink, + writeFile, + symlink, +} from "node:fs/promises"; +import { join } from "node:path"; +import { createTestTempDirectory } from "../fixtures/temporaryDirectory.js"; +import { ArtifactResourceScope } from "../../src/artifacts/ArtifactResourceScope.js"; +import { importReferenceSource } from "../../src/application/ReferenceSourceImport.js"; +import { readReferenceSource } from "../../src/reference/ReferenceSourceReader.js"; + +it.skipIf(process.platform === "win32").each(["reader", "import"] as const)( + "keeps sibling observations through %s when a symlink becomes a regular file before readlink", + async (operation) => { + const root = await createTestTempDirectory("rea-reference-symlink-race-"); + await mkdir(join(root, "nested")); + const linkPath = join(root, "nested", "link"); + const siblingPath = join(root, "sibling.txt"); + await symlink("missing-target", linkPath); + await writeFile(siblingPath, "sibling remains observable"); + race.path = linkPath; + race.actualErrno = undefined; + race.beforeReadlink = async () => { + await unlink(linkPath); + await writeFile(linkPath, "replacement regular file"); + }; + + const resources = new ArtifactResourceScope(); + let read: Awaited> | undefined; + let imported: Awaited> | undefined; + let thrown: unknown; + try { + if (operation === "reader") + read = await readReferenceSource(root, resources); + else + imported = await importReferenceSource( + { + root, + caller: "symlink-replacement-test", + policy: { secretPatterns: [] }, + }, + resources, + ); + } catch (cause: unknown) { + thrown = cause; + } finally { + race.path = ""; + race.beforeReadlink = undefined; + await resources.close(); + } + + // The hook establishes scheduling; this assertion verifies the host's real + // readlink syscall reports the replacement as EINVAL. + expect(race.actualErrno).toBe("EINVAL"); + expect(thrown).toBeUndefined(); + expect(read?.ok ?? imported?.ok, JSON.stringify(imported)).toBe(true); + if (imported?.ok) { + expect(imported.value.inventory_state).toBe("partial"); + expect(imported.value.entries).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + path: "nested/link", + kind: "symlink", + target: null, + target_state: "unreadable", + }), + expect.objectContaining({ + path: "sibling.txt", + kind: "file", + content_state: "hashed", + sha256: expect.any(String), + }), + ]), + ); + expect( + imported.value.entries + .find(({ path }) => path === "nested/link") + ?.limitations.join(" "), + ).toContain("Symbolic link changed"); + } + if (!read?.ok) return; + expect(read.value.rootFailure).toBeUndefined(); + expect(read.value.entries.some(({ path }) => path === ".")).toBe(false); + expect(read.value.entries).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + status: "failed", + kind: "symlink", + path: "nested/link", + code: "changed", + }), + expect.objectContaining({ + status: "read", + kind: "file", + path: "sibling.txt", + }), + ]), + ); + }, +); + +it.skipIf(process.platform === "win32").each(["contents", "root"] as const)( + "retains a changed %s as a graph limitation after its directory handle closes", + async (changed) => { + const parent = await createTestTempDirectory("rea-reference-root-race-"); + const root = join(parent, "observed"); + await mkdir(root); + const source = join(root, "source.txt"); + await writeFile(source, "already observed source"); + race.directory = root; + race.afterDirectoryClose = () => + changed === "contents" + ? rename(source, join(root, "renamed.txt")) + : rename(root, join(parent, "moved")); + const resources = new ArtifactResourceScope(); + let imported: Awaited>; + try { + imported = await importReferenceSource( + { + root, + caller: "root-revalidation-test", + policy: { secretPatterns: [] }, + }, + resources, + ); + } finally { + race.directory = ""; + race.afterDirectoryClose = undefined; + await resources.close(); + } + const retainedPath = + changed === "contents" + ? join(root, "renamed.txt") + : join(parent, "moved", "source.txt"); + expect(await readFile(retainedPath, "utf8")).toBe( + "already observed source", + ); + await expect(readFile(source)).rejects.toMatchObject({ code: "ENOENT" }); + expect(imported.ok, JSON.stringify(imported)).toBe(true); + if (!imported.ok) return; + expect(imported.value.inventory_state).toBe("partial"); + expect(imported.value.limitations).toContainEqual( + expect.stringContaining( + changed === "contents" + ? "Reference root enumeration is incomplete: Reference root changed" + : "Reference root enumeration is incomplete: Directory identity could not be verified: ENOENT", + ), + ); + expect(imported.value.entries).toEqual([ + expect.objectContaining({ + path: "source.txt", + kind: "file", + content_state: "hashed", + }), + ]); + }, +);