diff --git a/docs/installation.md b/docs/installation.md index b4cfc25b39..260067b014 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -57,8 +57,14 @@ Before applying changes, REA checks your current configuration. The plan lists: Malformed or unsafe existing configuration blocks the whole transaction before Hopper installation or any file write. Declining or pressing Ctrl-C makes no changes. Agent configuration writes preserve unrelated entries and comments, -create backups, use atomic replacement, and verify their result. Setup and -uninstall retain an existing `.rea.backup` rather than replacing the first snapshot. +create backups, stage changes in a private directory, and verify their result. +Before publication, they refuse detected changes to the selected configuration. +Replacement uses an explicit destination under the selected canonical parent; +it does not follow a newly inserted destination symlink. First-run setup uses +an exclusive hard link and fails without overwriting a newly created destination; +filesystems without hard-link support cannot publish a first-run configuration. +Setup and uninstall retain an existing `.rea.backup` rather than replacing the +first snapshot. If that snapshot differs from the current configuration, setup and uninstall preserve a verified `.rea.backup..recovery` copy of the current bytes before editing; setup also previews it. Both snapshots remain available. @@ -69,7 +75,8 @@ If setup cannot verify a newly created configuration, it retains the file and reports manual recovery instead of deleting a pathname that may have changed. For an existing configuration, setup and uninstall recovery refuse a detected replacement or edit and name the recovery backup. Quit clients before editing their configuration; -these checks do not lock out another writer modifying the same file in place. +these checks do not lock out another writer modifying the same file in place, +or exclude pathname changes between the final check and publication. Setup also retains valid environment overrides in REA's existing server entry, such as `REA_MCP_INPUT_SCHEMA_PROFILE`, `REA_MCP_OUTPUT_SCHEMA_PROFILE`, and `REA_MCP_MAX_RESPONSE_BYTES`. diff --git a/src/application/ClientConfigurationCleanup.ts b/src/application/ClientConfigurationCleanup.ts new file mode 100644 index 0000000000..bf0e310eab --- /dev/null +++ b/src/application/ClientConfigurationCleanup.ts @@ -0,0 +1,113 @@ +import { + ArtifactResourceRetryFailure, + type ArtifactResourceOwner, + type ArtifactResourceScope, +} from "../artifacts/ArtifactResourceScope.js"; +import type { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; +import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; +import type { AnalysisCleanupObservation } from "../domain/analysisErrorBase.js"; +import type { RegularFileOperationOutcome } from "../filesystem/OwnedRegularFile.js"; +import { + RegularFileCleanupFailure, + retryRegularFileCleanupFailures, +} from "./RegularFileRead.js"; + +/** Completion and exact staging owner retained when publication cleanup fails. */ +export class ClientConfigurationPublicationFailure< + Value = unknown, +> extends ArtifactResourceRetryFailure { + constructor( + readonly path: string, + readonly outcome: RegularFileOperationOutcome, + readonly cleanupCause: unknown, + readonly owner: Extract< + ArtifactResourceOwner, + { readonly kind: "staged-output" } + >, + readonly cleanupLease: AnalysisCleanupLease, + ) { + super( + [{ owner, lease: cleanupLease }], + "Configuration publication cleanup remains incomplete", + { + cause: new AggregateError( + [...(outcome.kind === "failed" ? [outcome.cause] : []), cleanupCause], + "Configuration operation and staging cleanup failures", + ), + cleanup: ArtifactReaderFailure.cleanupObservation( + cleanupCause, + owner.resource, + ), + }, + ); + } +} + +type ClientConfigurationCleanupFailure = + | RegularFileCleanupFailure + | ClientConfigurationPublicationFailure; + +export const isClientConfigurationCleanupFailure = ( + cause: unknown, +): cause is ClientConfigurationCleanupFailure => + cause instanceof RegularFileCleanupFailure || + cause instanceof ClientConfigurationPublicationFailure; + +/** Adopt only capabilities carried by explicit operation outcomes, never opaque causes. */ +export const retryClientConfigurationCleanup = async ( + failure: ClientConfigurationCleanupFailure, + resources: ArtifactResourceScope, +): Promise<{ + readonly primary: unknown; + readonly failures: readonly RegularFileCleanupFailure[]; + readonly cleanup?: AnalysisCleanupObservation; +}> => { + const failures: RegularFileCleanupFailure[] = []; + const observations: AnalysisCleanupObservation[] = []; + let current = failure; + let primary: unknown; + for (;;) { + let nested: unknown; + if (current instanceof RegularFileCleanupFailure) { + const retried = await retryRegularFileCleanupFailures(current, resources); + failures.push(...retried.failures); + if (retried.cleanup !== undefined) observations.push(retried.cleanup); + primary = retried.primary; + const last = retried.failures.at(-1)!; + nested = last.outcome.kind === "failed" ? last.outcome.cause : undefined; + } else { + const released = await resources.release({ + kind: "lease", + resource: current.owner.resource, + lease: current.cleanupLease, + }); + if (released.kind === "failed") + observations.push( + ArtifactReaderFailure.cleanupObservation( + released.cause, + current.owner.resource, + ), + ); + primary = + current.outcome.kind === "failed" + ? current.outcome.cause + : current.cleanupCause; + nested = + current.outcome.kind === "failed" ? current.outcome.cause : undefined; + } + if (!isClientConfigurationCleanupFailure(nested)) break; + current = nested; + } + return { + primary, + failures, + ...(observations.length === 0 + ? {} + : { + cleanup: { + reason: observations.map(({ reason }) => reason).join("; "), + resources: observations.flatMap(({ resources }) => resources), + }, + }), + }; +}; diff --git a/src/application/ClientConfigurationPublication.ts b/src/application/ClientConfigurationPublication.ts index 147de0afb2..4131345b60 100644 --- a/src/application/ClientConfigurationPublication.ts +++ b/src/application/ClientConfigurationPublication.ts @@ -1,13 +1,25 @@ -import { constants } from "node:fs"; -import { lstat, open, writeFile, type FileHandle } from "node:fs/promises"; -import writeFileAtomic from "write-file-atomic"; +import { constants, type Stats } from "node:fs"; +import { + link, + lstat, + mkdtemp, + open, + realpath, + rename, + type FileHandle, +} from "node:fs/promises"; +import { basename, dirname, join, sep } from "node:path"; +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; -import { withOwnedFileHandle } from "../filesystem/OwnedRegularFile.js"; +import { OwnedStagedOutput } from "../filesystem/OwnedStagedOutput.js"; import { RegularFileChangedError, sameRegularFileState, } from "../filesystem/RegularFile.js"; +import type { RegularFileOperationOutcome } from "../filesystem/OwnedRegularFile.js"; +import { readRegularFileTextSnapshot } from "./RegularFileRead.js"; +import { ClientConfigurationPublicationFailure } from "./ClientConfigurationCleanup.js"; /** The exact staged file retained through publication, verification and recovery. */ export class ClientConfigurationPublication { @@ -15,83 +27,86 @@ export class ClientConfigurationPublication { readonly path: string, private readonly contents: string, private readonly owner: OwnedFileHandle, + private readonly staging: OwnedStagedOutput, + private readonly cleanup: AnalysisCleanupLease, ) {} static async write( path: string, contents: string, + previous: string | undefined, mode?: number, ): Promise { - let owner: OwnedFileHandle | undefined; + const parent = await realpath(dirname(path)); + const destination = join(parent, basename(path)); + const input = await selectedConfiguration(destination, previous); + const directory = await mkdtemp(`${parent}${sep}.rea-config-`); + const staging = await OwnedStagedOutput.create(directory); + const cleanup = new AnalysisCleanupLease( + () => staging.close(), + [directory], + ); try { - await writeFileAtomic(path, contents, { - encoding: "utf8", - ...(mode === undefined ? {} : { mode }), - tmpfileCreated: async (stagedPath) => { - const writableFlags = - constants.O_RDWR | + const stagedPath = `${directory}${sep}content`; + const owner = new OwnedFileHandle( + await open( + stagedPath, + constants.O_RDWR | + constants.O_CREAT | + constants.O_EXCL | (constants.O_NONBLOCK ?? 0) | - (constants.O_NOFOLLOW ?? 0); - if (((await lstat(stagedPath)).mode & 0o200) !== 0) { - owner = new OwnedFileHandle(await open(stagedPath, writableFlags)); - } else { - // The atomic writer already owns a writable creation descriptor. - // Retain ours before restoring the staged file's final read-only mode. - const readable = new StagedConfigurationPermissions( - await open( - stagedPath, - constants.O_RDONLY | - (constants.O_NONBLOCK ?? 0) | - (constants.O_NOFOLLOW ?? 0), - ), - ); - await withOwnedFileHandle( - stagedPath, - readable, - {}, - async (handle) => { - const staged = await handle.stat(); - if (!staged.isFile()) - throw new Error("Staged configuration is not a regular file"); - readable.restoreMode(staged.mode & 0o7777); - await handle.chmod((staged.mode & 0o7777) | 0o600); - owner = new OwnedFileHandle( - await open(stagedPath, writableFlags), - ); - const writable = await owner.handle.stat(); - if ( - staged.ino === 0 || - !writable.isFile() || - writable.dev !== staged.dev || - writable.ino !== staged.ino - ) - throw new RegularFileChangedError(stagedPath); - }, - ); - } - if (owner === undefined) - throw new Error("Staged configuration has no file owner"); - if (!(await owner.handle.stat()).isFile()) - throw new Error("Staged configuration is not a regular file"); - }, - }); - if (owner === undefined) - throw new Error("Atomic configuration publication has no file owner"); - return new ClientConfigurationPublication(path, contents, owner); + (constants.O_NOFOLLOW ?? 0), + 0o600, + ), + ); + staging.setFile(owner); + await owner.handle.writeFile(contents, { encoding: "utf8" }); + const created = await owner.handle.stat(); + if ( + input !== undefined && + (input.uid !== created.uid || input.gid !== created.gid) + ) + await owner.handle.chown(input.uid, input.gid); + await owner.handle.chmod(mode ?? (input?.mode ?? 0o600) & 0o7777); + await owner.handle.sync(); + if ((await owner.handle.stat()).ino === 0) + throw new RegularFileChangedError(stagedPath); + await staging.assertReadyForPublication(stagedPath); + await assertSelectedConfiguration(destination, input); + if (input === undefined) { + // An exclusive link cannot overwrite a file created after absence was checked. + await link(stagedPath, destination); + } else { + // The destination is explicit: rename never follows its final symlink. + await rename(stagedPath, destination); + staging.recordPublishedRename(); + } + return new ClientConfigurationPublication( + destination, + contents, + owner, + staging, + cleanup, + ); } catch (cause: unknown) { - if (owner === undefined) throw cause; - return withOwnedFileHandle(path, owner, {}, async () => { - throw cause; - }); + await releaseStaging(path, staging, cleanup, { kind: "failed", cause }); + throw cause; } } - /** Keep the publication descriptor owned until the complete transaction ends. */ - verify(operation: () => Promise): Promise { - return withOwnedFileHandle(this.path, this.owner, {}, operation); + /** Keep all publication resources owned through the complete operation. */ + async verify(operation: () => Promise): Promise { + let outcome: RegularFileOperationOutcome; + try { + outcome = { kind: "completed", value: await operation() }; + } catch (cause: unknown) { + outcome = { kind: "failed", cause }; + } + await releaseStaging(this.path, this.staging, this.cleanup, outcome); + if (outcome.kind === "failed") throw outcome.cause; + return outcome.value; } - /** Readback cannot establish a successful transaction through a replaced name. */ async assertCurrent(): Promise { const owned = await this.owner.handle.stat(); const named = await lstat(this.path); @@ -100,14 +115,10 @@ export class ClientConfigurationPublication { named.isSymbolicLink() || !sameRegularFileState(owned, named) ) - throw new Error( - "Published configuration path no longer names its owned file", - ); + throw new RegularFileChangedError(this.path); } - /** Require observed published bytes and restore through the owned descriptor. - * State checks do not exclude a concurrent writer to the same inode. - */ + /** State checks do not exclude a concurrent writer to the same inode. */ async restore(original: string | undefined): Promise { if (original === undefined) throw new Error( @@ -122,18 +133,25 @@ export class ClientConfigurationPublication { named.isSymbolicLink() || !sameRegularFileState(before, named) || !(await this.#hasPublishedBytes(handle)) || - !sameRegularFileState(before, await handle.stat()) + !sameRegularFileState(before, await handle.stat()) || + !sameRegularFileState(before, await lstat(this.path)) ) - throw new Error( - "Published configuration changed; recovery requires the retained backup", + throw new RegularFileChangedError(this.path); + const bytes = Buffer.from(original, "utf8"); + for (let position = 0; position < bytes.length;) { + const { bytesWritten } = await handle.write( + bytes, + position, + bytes.length - position, + position, ); - // Recovery writes through the retained descriptor, never through the name. - if (!sameRegularFileState(before, await lstat(this.path))) - throw new Error("Published configuration path changed before recovery"); - await writeFile(handle, original, { encoding: "utf8" }); - await handle.truncate(Buffer.byteLength(original, "utf8")); + if (bytesWritten === 0) + throw new Error("Configuration restoration made no write progress"); + position += bytesWritten; + } + await handle.truncate(bytes.length); if (!sameRegularFileState(await handle.stat(), await lstat(this.path))) - throw new Error("Configuration path changed during recovery"); + throw new RegularFileChangedError(this.path); } async #hasPublishedBytes(handle: FileHandle): Promise { @@ -160,27 +178,60 @@ export class ClientConfigurationPublication { } } -/** Restore temporary staging permissions before releasing their exact descriptor. */ -class StagedConfigurationPermissions extends OwnedFileHandle { - #mode: number | undefined; - #restoring: Promise | undefined; - - restoreMode(mode: number): void { - this.#mode = mode; +const selectedConfiguration = async ( + path: string, + previous: string | undefined, +): Promise => { + let selected: + | Awaited> + | undefined; + try { + selected = await readRegularFileTextSnapshot(path, { symlinks: "reject" }); + } catch (cause: unknown) { + if (!(cause instanceof Error && "code" in cause && cause.code === "ENOENT")) + throw cause; } + if (selected?.text !== previous || selected?.state.ino === 0) + throw new RegularFileChangedError(path); + return selected?.state; +}; - override async close(): Promise { - if (this.#mode !== undefined) { - this.#restoring ??= this.handle - .chmod(this.#mode) - .then(() => { - this.#mode = undefined; - }) - .finally(() => { - this.#restoring = undefined; - }); - await this.#restoring; - } - await super.close(); +const assertSelectedConfiguration = async ( + path: string, + selected: Stats | undefined, +): Promise => { + let current: Stats | undefined; + try { + current = await lstat(path); + } catch (cause: unknown) { + if (!(cause instanceof Error && "code" in cause && cause.code === "ENOENT")) + throw cause; } -} + if ( + selected === undefined + ? current !== undefined + : current === undefined || + current.isSymbolicLink() || + !sameRegularFileState(selected, current) + ) + throw new RegularFileChangedError(path); +}; + +const releaseStaging = async ( + path: string, + staging: OwnedStagedOutput, + cleanup: AnalysisCleanupLease, + outcome: RegularFileOperationOutcome, +): Promise => { + try { + await cleanup.release(); + } catch (cause: unknown) { + throw new ClientConfigurationPublicationFailure( + path, + outcome, + cause, + { kind: "staged-output", resource: staging.directory, output: staging }, + cleanup, + ); + } +}; diff --git a/src/application/RegularFileRead.ts b/src/application/RegularFileRead.ts index 199264ec75..6792a3b272 100644 --- a/src/application/RegularFileRead.ts +++ b/src/application/RegularFileRead.ts @@ -188,25 +188,36 @@ export const readRegularFileText = ( path: string, options: RegularFileReadOptions = {}, ): Promise => + readRegularFileContent(path, (bytes) => decodeFileText(path, bytes), options); + +/** Retain the read owner's verified file state together with its decoded text. */ +export const readRegularFileTextSnapshot = ( + path: string, + options: RegularFileReadOptions = {}, +): Promise<{ readonly text: string; readonly state: Stats }> => readRegularFileContent( path, - (bytes) => { - try { - return new TextDecoder("utf-8", { - fatal: true, - ignoreBOM: true, - }).decode(bytes); - } catch (cause: unknown) { - if (!(cause instanceof TypeError)) throw cause; - throw new InvalidUtf8FileError(path, cause); - } - }, + (bytes, state) => ({ + text: decodeFileText(path, bytes), + state, + }), options, ); +const decodeFileText = (path: string, bytes: Buffer): string => { + try { + return new TextDecoder("utf-8", { fatal: true, ignoreBOM: true }).decode( + bytes, + ); + } catch (cause: unknown) { + if (!(cause instanceof TypeError)) throw cause; + throw new InvalidUtf8FileError(path, cause); + } +}; + const readRegularFileContent = ( path: string, - decode: (bytes: Buffer) => Value, + decode: (bytes: Buffer, state: Stats) => Value, options: RegularFileReadOptions, ): Promise => withRegularFile( @@ -219,7 +230,7 @@ const readRegularFileContent = ( ); if (bytes === undefined || bytes.length !== stats.size) throw new RegularFileChangedError(path); - return decode(bytes); + return decode(bytes, stats); }, options, ); diff --git a/src/application/SetupClientConfiguration.ts b/src/application/SetupClientConfiguration.ts index ec5608e522..d0d6c6f242 100644 --- a/src/application/SetupClientConfiguration.ts +++ b/src/application/SetupClientConfiguration.ts @@ -23,6 +23,7 @@ import { import { dshDocument, DshCordisDocument } from "./DshCordisPatch.js"; import { mkdir } from "node:fs/promises"; import { dirname } from "node:path"; +import { isClientConfigurationCleanupFailure } from "./ClientConfigurationCleanup.js"; import { ClientConfigurationPublication } from "./ClientConfigurationPublication.js"; import { clientRegistrationEnvironment, @@ -304,10 +305,11 @@ const persistClientConfiguration = async ({ ...(legacyPath === undefined ? [] : [legacyPath]), ...(enableOmp ? [[OMP_DISABLED_SERVERS_KEY]] : []), ]), + original, 0o600, ); } catch (cause: unknown) { - if (cause instanceof RegularFileCleanupFailure) throw cause; + if (isClientConfigurationCleanupFailure(cause)) throw cause; // Write failure is reported by the status reason. void cause; return { status: "failed", reason: "write" }; @@ -315,7 +317,7 @@ const persistClientConfiguration = async ({ return publication.verify(async () => { try { const readback = parseClientConfiguration( - await readRegularFileText(transactionPath), + await readRegularFileText(publication.path), format, ); await publication.assertCurrent(); diff --git a/src/application/SetupHost.ts b/src/application/SetupHost.ts index 8e4b702f49..8eb27d8df9 100644 --- a/src/application/SetupHost.ts +++ b/src/application/SetupHost.ts @@ -1,3 +1,7 @@ +import { + isClientConfigurationCleanupFailure, + retryClientConfigurationCleanup, +} from "./ClientConfigurationCleanup.js"; import { access } from "node:fs/promises"; import { resolve } from "node:path"; @@ -43,10 +47,7 @@ import { } from "./SetupClientConfiguration.js"; import { failedConfigurationMessage } from "./SetupClients.js"; import { setupInstallFailure } from "./SetupInstallFailure.js"; -import { - RegularFileCleanupFailure, - retryRegularFileCleanupFailures, -} from "./RegularFileRead.js"; +import { RegularFileCleanupFailure } from "./RegularFileRead.js"; import { explicitSetupRegistrationEnvironment, providerRegistrationEnvironment, @@ -403,8 +404,8 @@ const runClientConfigurationOperation = async ( try { return await operation(); } catch (cause: unknown) { - if (!(cause instanceof RegularFileCleanupFailure)) throw cause; - const cleanup = await retryRegularFileCleanupFailures(cause, resources); + if (!isClientConfigurationCleanupFailure(cause)) throw cause; + const cleanup = await retryClientConfigurationCleanup(cause, resources); const rollback = cleanup.failures.find( (failure) => failure instanceof ClientConfigurationCleanupRollbackFailure, diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index e48392540c..5dc44e1533 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -1,3 +1,7 @@ +import { + isClientConfigurationCleanupFailure, + retryClientConfigurationCleanup, +} from "./ClientConfigurationCleanup.js"; import { ClientConfigurationShapeError, parseClientConfiguration, @@ -34,7 +38,6 @@ import { InvalidUtf8FileError, RegularFileCleanupFailure, readRegularFileText, - retryRegularFileCleanupFailures, } from "./RegularFileRead.js"; import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { @@ -66,8 +69,12 @@ export interface UninstallFileSystem { publishConfiguration( path: string, contents: string, + original: string, ): Promise< - Pick + Pick< + ClientConfigurationPublication, + "path" | "verify" | "assertCurrent" | "restore" + > >; stat(path: string): Promise; realpath?(path: string): Promise; @@ -83,8 +90,8 @@ const systemFileSystem = ( readRegularText: (path) => readSkillFile(path, resources), copy: (source, destination) => copyFile(source, destination, fsConstants.COPYFILE_EXCL), - publishConfiguration: (path, contents) => - ClientConfigurationPublication.write(path, contents), + publishConfiguration: (path, contents, original) => + ClientConfigurationPublication.write(path, contents, original), stat: (path) => lstat(path), realpath: (path) => realpath(path), remove: (path, recursive = true) => rm(path, { recursive }), @@ -389,13 +396,15 @@ const readClientConfiguration = async ( try { original = await fileSystem.readText(transactionPath); } catch (cause: unknown) { - if (cause instanceof RegularFileCleanupFailure) { - await retryRegularFileCleanupFailures(cause, resources); + if (isClientConfigurationCleanupFailure(cause)) { + const cleanup = await retryClientConfigurationCleanup(cause, resources); return itemRead( item( client.name, "failed", - "Configuration could not be read or its resources could not be released. Check file permissions, then rerun uninstall.", + cleanup.primary instanceof InvalidUtf8FileError + ? "Configuration is not valid UTF-8 and was not changed; its resources could not be released. Repair it, then rerun uninstall." + : "Configuration could not be read or its resources could not be released. Check file permissions, then rerun uninstall.", ), ); } @@ -570,17 +579,18 @@ const removeClient = async ( original, editedPaths, ), + original, ); } catch (cause: unknown) { - if (cause instanceof RegularFileCleanupFailure) - await retryRegularFileCleanupFailures(cause, resources); + if (isClientConfigurationCleanupFailure(cause)) + await retryClientConfigurationCleanup(cause, resources); return manualRecovery(); } try { return await publication.verify(async () => { try { const readback = parseClientConfiguration( - await fileSystem.readText(transactionPath), + await fileSystem.readText(publication.path), client.format, ); await publication.assertCurrent(); @@ -597,8 +607,8 @@ const removeClient = async ( `Removed registration from ${client.configPath}.`, ); } catch (cause: unknown) { - if (cause instanceof RegularFileCleanupFailure) - await retryRegularFileCleanupFailures(cause, resources); + if (isClientConfigurationCleanupFailure(cause)) + await retryClientConfigurationCleanup(cause, resources); try { await publication.restore(original); } catch { @@ -612,8 +622,8 @@ const removeClient = async ( } }); } catch (cause: unknown) { - if (cause instanceof RegularFileCleanupFailure) - await retryRegularFileCleanupFailures(cause, resources); + if (isClientConfigurationCleanupFailure(cause)) + await retryClientConfigurationCleanup(cause, resources); return item( client.name, "failed", diff --git a/src/filesystem/OwnedStagedOutput.ts b/src/filesystem/OwnedStagedOutput.ts index 9496a7cb9e..af9640c560 100644 --- a/src/filesystem/OwnedStagedOutput.ts +++ b/src/filesystem/OwnedStagedOutput.ts @@ -52,9 +52,11 @@ export class OwnedStagedOutput { this.#file = file; } - /** Verify that the path still names the file its descriptor observed at close. */ + /** Verify the staged name against its descriptor after the last write. */ async assertReadyForPublication(content: string): Promise { await this.#verifiedDirectory(); + if (this.#file !== undefined && !this.#fileClosed) + await this.#observeFileIdentity(); if (this.#fileIdentity === undefined) throw new Error("Cannot verify staged file identity", { cause: this.#identityFailure, @@ -72,6 +74,14 @@ export class OwnedStagedOutput { async closeFile(): Promise { const file = this.#file; if (file === undefined || this.#fileClosed) return; + await this.#observeFileIdentity(); + await file.close(); + this.#fileClosed = true; + } + + async #observeFileIdentity(): Promise { + const file = this.#file; + if (file === undefined) return; // FAT and exFAT assign a file ID when the first write allocates storage, // so only the descriptor's final identity can prove the staged pathname. try { @@ -81,8 +91,6 @@ export class OwnedStagedOutput { this.#fileIdentity = undefined; this.#identityFailure = cause; } - await file.close(); - this.#fileClosed = true; } /** Record that the staged file moved to the published destination by rename. */ diff --git a/tests/boundary/filesystem/clientConfigurationTransactions.test.ts b/tests/boundary/filesystem/clientConfigurationTransactions.test.ts index cb2b51a294..8d8fffa025 100644 --- a/tests/boundary/filesystem/clientConfigurationTransactions.test.ts +++ b/tests/boundary/filesystem/clientConfigurationTransactions.test.ts @@ -32,10 +32,11 @@ const failedVerificationFileSystem = ( readText: (path) => readFile(path, "utf8"), copy: (source, destination) => copyFile(source, destination, fsConstants.COPYFILE_EXCL), - publishConfiguration: async (path, contents) => { + publishConfiguration: async (path, contents, original) => { const publication = await ClientConfigurationPublication.write( path, contents, + original, ); publication.assertCurrent = async () => { throw new Error("verification failed"); diff --git a/tests/boundary/filesystem/clientLifecycle.test.ts b/tests/boundary/filesystem/clientLifecycle.test.ts index 2659bde3ae..19a4b51800 100644 --- a/tests/boundary/filesystem/clientLifecycle.test.ts +++ b/tests/boundary/filesystem/clientLifecycle.test.ts @@ -1902,12 +1902,13 @@ describe("client configuration filesystem failure reporting", () => { failure === "backup" ? Promise.reject(new Error("SECRET backup failure")) : copyFile(source, destination, fsConstants.COPYFILE_EXCL), - publishConfiguration: async (path, contents) => { + publishConfiguration: async (path, contents, original) => { if (failure === "publication") throw new Error("SECRET publication failure"); const publication = await ClientConfigurationPublication.write( path, contents, + original, ); if (failure === "update" || failure === "restore") publication.assertCurrent = async () => { @@ -1939,10 +1940,11 @@ describe("client configuration filesystem failure reporting", () => { await writeFile(`${config}.rea.backup`, initial); const result = await systemUninstallHost(home, { ...testFileSystem, - publishConfiguration: async (path, contents) => { + publishConfiguration: async (path, contents, original) => { const publication = await ClientConfigurationPublication.write( path, contents, + original, ); publication.assertCurrent = async () => { throw new Error("verification failed after writing"); @@ -2054,8 +2056,8 @@ const testFileSystem: UninstallFileSystem = { readText: (path) => readFile(path, "utf8"), copy: (source, destination) => copyFile(source, destination, fsConstants.COPYFILE_EXCL), - publishConfiguration: (path, contents) => - ClientConfigurationPublication.write(path, contents), + publishConfiguration: (path, contents, original) => + ClientConfigurationPublication.write(path, contents, original), stat: (path) => lstat(path), realpath: (path) => realpath(path), remove: (path) => rm(path, { recursive: true }), diff --git a/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts b/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts index 4bd7574bc3..aeeb8a1708 100644 --- a/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts +++ b/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts @@ -70,16 +70,17 @@ const preparedVsCodeSetupHost = async (prefix: string) => { return { home, host, client }; }; -/** Release the held read handle and show host cleanup reuses it as a third attempt. */ +/** Release the held read handle and verify host cleanup retries that same owner. */ const expectCloseReclaimsHandle = async ( host: { close?: () => Promise }, closeAttempts: readonly object[], release: () => void, ) => { + const attemptsBeforeClose = closeAttempts.length; release(); expect(await host.close?.()).toBeUndefined(); - expect(closeAttempts).toHaveLength(3); - expect(closeAttempts[2]).toBe(closeAttempts[0]); + expect(closeAttempts).toHaveLength(attemptsBeforeClose + 1); + expect(closeAttempts.at(-1)).toBe(closeAttempts[0]); }; /** Give up on the assertion path: release the handle and let cleanup close it. */ @@ -113,7 +114,10 @@ it("retains invalid UTF-8 diagnostics and the uninstall read owner when close fa detail: expect.stringContaining("not valid UTF-8"), }); expect(await readFile(path)).toEqual(original); - expect(closeAttempts).toHaveLength(2); + expect(closeAttempts).toHaveLength(3); + expect(closeAttempts.every((handle) => handle === closeAttempts[0])).toBe( + true, + ); await expectCloseReclaimsHandle(host, closeAttempts, release); } finally { await cleanupHeldHandle(host, release); diff --git a/tests/process-global/setupConfigurationRollback.test.ts b/tests/process-global/setupConfigurationRollback.test.ts index 4471e94374..6c3f99641c 100644 --- a/tests/process-global/setupConfigurationRollback.test.ts +++ b/tests/process-global/setupConfigurationRollback.test.ts @@ -13,12 +13,15 @@ const state = vi.hoisted(() => ({ beforePublicationOpen: undefined as | ((path: string) => Promise) | undefined, - stagingPrefix: undefined as string | undefined, - readOnlyStageHandles: [] as FileHandle[], - rejectStageModeRestore: false, + rejectPublicationMode: false, + rejectStageRemoval: false, + beforeFirstPublication: undefined as (() => Promise) | undefined, afterPublication: undefined as ((path: string) => Promise) | undefined, rejectRestore: false, removeCopiedRecovery: false, + afterBackupCopy: undefined as + | ((source: string, destination: string) => Promise) + | undefined, initialCleanupFailure: undefined as unknown, readbackCleanupFailure: undefined as unknown, })); @@ -30,12 +33,13 @@ afterEach(() => { state.publicationCloseFailure = undefined; state.publicationHandles = []; state.beforePublicationOpen = undefined; - state.stagingPrefix = undefined; - state.readOnlyStageHandles = []; - state.rejectStageModeRestore = false; + state.rejectPublicationMode = false; + state.rejectStageRemoval = false; state.afterPublication = undefined; + state.beforeFirstPublication = undefined; state.rejectRestore = false; state.removeCopiedRecovery = false; + state.afterBackupCopy = undefined; state.initialCleanupFailure = undefined; state.readbackCleanupFailure = undefined; }); @@ -50,23 +54,20 @@ vi.mock("node:fs/promises", async (importOriginal) => { if (writable && typeof arguments_[0] === "string") await state.beforePublicationOpen?.(arguments_[0]); const handle = await original.open(...arguments_); - if ( - typeof arguments_[0] === "string" && - state.stagingPrefix !== undefined && - arguments_[0].startsWith(state.stagingPrefix) && - typeof arguments_[1] === "number" && - (arguments_[1] & 3) === 0 - ) { - state.readOnlyStageHandles.push(handle); - const chmod = handle.chmod.bind(handle); - handle.chmod = async (mode) => { - if (state.rejectStageModeRestore && mode === 0o400) - throw new Error("staging mode restoration denied"); - await chmod(mode); - }; - } if (typeof arguments_[1] === "number" && (arguments_[1] & 3) === 2) { state.publicationHandles.push(handle); + const write = handle.write.bind(handle); + handle.write = ((...values: Parameters) => { + if (state.rejectRestore && state.readbackFailed) + return Promise.reject(new Error("injected restore write failure")); + return write(...values); + }) as typeof handle.write; + const chmod = handle.chmod.bind(handle); + handle.chmod = async (mode) => { + if (state.rejectPublicationMode && mode === 0o400) + throw new Error("staging mode preparation denied"); + await chmod(mode); + }; const close = handle.close.bind(handle); handle.close = async () => { if (state.rejectPublicationClose) @@ -79,18 +80,35 @@ vi.mock("node:fs/promises", async (importOriginal) => { } return handle; }, - writeFile: ( - ...arguments_: Parameters - ): ReturnType => { - if (state.rejectRestore && state.readbackFailed) - return Promise.reject(new Error("injected restore write failure")); - return original.writeFile(...arguments_); + rmdir: async (...values: Parameters) => { + if ( + state.rejectStageRemoval && + String(values[0]).includes(".rea-config-") + ) + throw new Error("injected staging directory removal failure"); + return original.rmdir(...values); + }, + rename: async (...values: Parameters) => { + const result = await original.rename(...values); + state.readbackFailed = true; + await state.afterPublication?.(String(values[1])); + return result; + }, + link: async (...values: Parameters) => { + await state.beforeFirstPublication?.(); + const result = await original.link(...values); + state.readbackFailed = true; + await state.afterPublication?.(String(values[1])); + return result; }, copyFile: async ( ...arguments_: Parameters ): Promise => { await original.copyFile(...arguments_); + const source = arguments_[0]; const destination = arguments_[1]; + if (typeof source === "string" && typeof destination === "string") + await state.afterBackupCopy?.(source, destination); if ( state.removeCopiedRecovery && typeof destination === "string" && @@ -101,28 +119,6 @@ vi.mock("node:fs/promises", async (importOriginal) => { }; }); -vi.mock("write-file-atomic", async (importOriginal) => { - const original = await importOriginal<{ - default: typeof import("write-file-atomic"); - }>(); - return { - ...original, - default: async (...arguments_: Parameters) => { - if (state.initialCleanupFailure !== undefined) - throw new Error("unexpected config write before initial read"); - const result = await original.default(...arguments_); - state.readbackFailed = true; - const publishedPath = arguments_[0]; - if ( - state.afterPublication !== undefined && - typeof publishedPath === "string" - ) - await state.afterPublication(publishedPath); - return result; - }, - }; -}); - vi.mock("../../src/application/RegularFileRead.js", async (importOriginal) => { const original = await importOriginal< @@ -143,6 +139,7 @@ vi.mock("../../src/application/RegularFileRead.js", async (importOriginal) => { }; }); +import { ClientConfigurationPublication } from "../../src/application/ClientConfigurationPublication.js"; import { configureClientConfiguration } from "../../src/application/SetupClientConfiguration.js"; import { RegularFileCleanupFailure } from "../../src/application/RegularFileRead.js"; import { OwnedFileHandle } from "../../src/filesystem/OwnedFileHandle.js"; @@ -895,10 +892,10 @@ it("uninstall progresses when a peer leaves a denied-close descriptor pending", }); it.skipIf(process.platform === "win32" || process.getuid?.() === 0)( - "read-only staging never changes permissions on a substituted writable descriptor", + "exclusive staging refuses a preexisting entry without changing its permissions", async () => { const root = await createTestTempDirectory( - "rea-uninstall-staging-handoff-", + "rea-uninstall-staging-exclusive-", ); const path = join(root, "client.json"); const foreign = join(root, "foreign.json"); @@ -912,9 +909,10 @@ it.skipIf(process.platform === "win32" || process.getuid?.() === 0)( await fs.chmod(path, 0o400); await fs.writeFile(foreign, "foreign"); await fs.chmod(foreign, 0o640); + let inserted: string | undefined; state.beforePublicationOpen = async (stagedPath) => { state.beforePublicationOpen = undefined; - await fs.unlink(stagedPath); + inserted = stagedPath; await fs.link(foreign, stagedPath); }; const host = systemUninstallHost(root); @@ -932,16 +930,17 @@ it.skipIf(process.platform === "win32" || process.getuid?.() === 0)( expect((await fs.lstat(foreign)).mode & 0o777).toBe(0o640); expect(await fs.readFile(path, "utf8")).toBe(original); expect((await fs.lstat(path)).mode & 0o777).toBe(0o400); - expect(state.publicationHandles).toHaveLength(1); - expect(state.publicationHandles[0]!.fd).toBe(-1); + expect(state.publicationHandles).toHaveLength(0); + expect(await host.close?.()).toContain("cleanup failed"); } finally { + if (inserted !== undefined) await fs.unlink(inserted); await host.close?.(); } }, ); it.skipIf(process.platform === "win32" || process.getuid?.() === 0)( - "uninstall retains permission restoration ownership until the staged descriptor releases", + "uninstall retains the private stage after mode preparation and descriptor cleanup fail", async () => { const root = await createTestTempDirectory( "rea-uninstall-staging-mode-retry-", @@ -955,8 +954,8 @@ it.skipIf(process.platform === "win32" || process.getuid?.() === 0)( '{"mcpServers":{"rea":{"command":"rea","args":["mcp"]}}}\n'; await fs.writeFile(path, original); await fs.chmod(path, 0o400); - state.stagingPrefix = `${path}.`; - state.rejectStageModeRestore = true; + state.rejectPublicationMode = true; + state.rejectPublicationClose = true; const host = systemUninstallHost(root); try { expect( @@ -968,18 +967,20 @@ it.skipIf(process.platform === "win32" || process.getuid?.() === 0)( }) ).status, ).toBe("failed"); - const pending = state.readOnlyStageHandles.filter(({ fd }) => fd >= 0); - expect(pending).toHaveLength(1); + expect(state.publicationHandles).toHaveLength(1); + expect(state.publicationHandles[0]!.fd).toBeGreaterThanOrEqual(0); expect(await host.close?.()).toContain("cleanup failed"); - state.rejectStageModeRestore = false; + state.rejectPublicationMode = false; + state.rejectPublicationClose = false; expect(await host.close?.()).toBeUndefined(); - expect(pending[0]!.fd).toBe(-1); + expect(state.publicationHandles[0]!.fd).toBe(-1); expect(await fs.readFile(path, "utf8")).toBe(original); expect((await fs.lstat(path)).mode & 0o777).toBe(0o400); } finally { - state.rejectStageModeRestore = false; + state.rejectPublicationMode = false; + state.rejectPublicationClose = false; await host.close?.(); - for (const handle of state.readOnlyStageHandles) + for (const handle of state.publicationHandles) if (handle.fd >= 0) await handle.close(); } }, @@ -1018,3 +1019,348 @@ it.skipIf(process.platform === "win32" || process.getuid?.() === 0)( } }, ); + +it.each(["setup", "uninstall"] as const)( + "%s refuses a configuration symlink inserted after backup without overwriting its target", + async (operation) => { + const root = await createTestTempDirectory( + "rea-config-prepublication-symlink-", + ); + const path = join(root, "client.json"); + const foreign = join(root, "foreign.json"); + const fs = + await vi.importActual( + "node:fs/promises", + ); + const original = + operation === "setup" + ? '{"keep":"original"}\n' + : '{"mcpServers":{"rea":{"command":"rea","args":["mcp"]}},"keep":"original"}\n'; + const unrelated = '{"keep":"foreign"}\n'; + await fs.writeFile(path, original); + await fs.writeFile(foreign, unrelated); + state.failReadback = false; + state.afterBackupCopy = async (source) => { + state.afterBackupCopy = undefined; + await fs.unlink(source); + await fs.symlink(foreign, source); + }; + if (operation === "setup") { + expect( + ( + await configureClientConfiguration( + { name: "cursor", configPath: path, format: "json" }, + {}, + ["rea", "mcp"], + ) + ).status, + ).toBe("failed"); + } else { + const host = systemUninstallHost(root); + try { + expect( + ( + await host.removeClient({ + name: "cursor", + configPath: path, + format: "json", + }) + ).status, + ).toBe("failed"); + } finally { + await host.close?.(); + } + } + expect(await fs.readFile(foreign, "utf8")).toBe(unrelated); + expect((await fs.lstat(path)).isSymbolicLink()).toBe(true); + expect(await fs.readFile(`${path}.rea.backup`, "utf8")).toBe(original); + }, +); + +const runConfigurationChange = async ( + operation: "setup" | "uninstall", + root: string, + path: string, +) => { + if (operation === "setup") + return configureClientConfiguration( + { name: "cursor", configPath: path, format: "json" }, + {}, + ["new-rea", "mcp"], + ); + const host = systemUninstallHost(root); + try { + return await host.removeClient({ + name: "cursor", + configPath: path, + format: "json", + }); + } finally { + await host.close?.(); + } +}; + +it.each([ + ["setup", "edit"], + ["uninstall", "edit"], + ["setup", "delete"], + ["uninstall", "delete"], +] as const)( + "%s preserves a configuration %s after backup", + async (operation, change) => { + const root = await createTestTempDirectory("rea-config-backup-change-"); + const path = join(root, "client.json"); + const fs = + await vi.importActual( + "node:fs/promises", + ); + const original = + '{"mcpServers":{"rea":{"command":"rea","args":["mcp"]}},"keep":true}\n'; + const edited = '{"changed":"by client"}\n'; + await fs.writeFile(path, original); + state.failReadback = false; + state.afterBackupCopy = async (source) => { + state.afterBackupCopy = undefined; + if (change === "edit") await fs.writeFile(source, edited); + else await fs.unlink(source); + }; + expect((await runConfigurationChange(operation, root, path)).status).toBe( + "failed", + ); + if (change === "edit") expect(await fs.readFile(path, "utf8")).toBe(edited); + else await expect(fs.lstat(path)).rejects.toMatchObject({ code: "ENOENT" }); + expect(await fs.readFile(`${path}.rea.backup`, "utf8")).toBe(original); + expect( + (await fs.readdir(root)).filter((name) => + name.startsWith(".rea-config-"), + ), + ).toEqual([]); + }, +); + +it.each(["setup", "uninstall"] as const)( + "%s refuses an input changed during staging", + async (operation) => { + const root = await createTestTempDirectory("rea-config-staging-change-"); + const path = join(root, "client.json"); + const fs = + await vi.importActual( + "node:fs/promises", + ); + const original = + '{"mcpServers":{"rea":{"command":"rea","args":["mcp"]}},"keep":true}\n'; + const edited = '{"changed":"during staging"}\n'; + await fs.writeFile(path, original); + state.failReadback = false; + state.beforePublicationOpen = async () => { + state.beforePublicationOpen = undefined; + await fs.writeFile(path, edited); + }; + expect((await runConfigurationChange(operation, root, path)).status).toBe( + "failed", + ); + expect(await fs.readFile(path, "utf8")).toBe(edited); + expect(await fs.readFile(`${path}.rea.backup`, "utf8")).toBe(original); + expect( + (await fs.readdir(root)).filter((name) => + name.startsWith(".rea-config-"), + ), + ).toEqual([]); + }, +); + +it("first-run setup preserves a destination created during staging", async () => { + const root = await createTestTempDirectory("rea-config-first-conflict-"); + const path = join(root, "client.json"); + const fs = + await vi.importActual( + "node:fs/promises", + ); + const edited = '{"created":"by client"}\n'; + state.failReadback = false; + state.beforePublicationOpen = async () => { + state.beforePublicationOpen = undefined; + await fs.writeFile(path, edited); + }; + expect((await runConfigurationChange("setup", root, path)).status).toBe( + "failed", + ); + expect(await fs.readFile(path, "utf8")).toBe(edited); + expect( + (await fs.readdir(root)).filter((name) => name.startsWith(".rea-config-")), + ).toEqual([]); +}); + +it.each(["setup", "uninstall"] as const)( + "%s retains staging directory cleanup for host retry", + async (operation) => { + const root = await createTestTempDirectory( + "rea-config-staging-directory-retry-", + ); + const path = join(root, "client.json"); + const fs = + await vi.importActual( + "node:fs/promises", + ); + await fs.writeFile( + path, + '{"mcpServers":{"rea":{"command":"rea","args":["mcp"]}},"keep":true}\n', + ); + state.failReadback = false; + state.rejectStageRemoval = true; + const host = + operation === "setup" + ? systemSetupHost( + { platform: "linux", homeDirectory: root } as DoctorHost, + { HOME: root }, + ) + : systemUninstallHost(root); + try { + const client = { + name: "cursor", + configPath: path, + format: "json" as const, + }; + if (operation === "setup") { + await expect( + (host as ReturnType).configureClient( + client, + {}, + ["new-rea", "mcp"], + ), + ).rejects.toThrow(); + } else { + expect( + ( + await (host as ReturnType).removeClient( + client, + ) + ).status, + ).toBe("failed"); + } + expect(await host.close?.()).toContain("cleanup"); + expect( + (await fs.readdir(root)).filter((name) => + name.startsWith(".rea-config-"), + ), + ).toHaveLength(1); + state.rejectStageRemoval = false; + expect(await host.close?.()).toBeUndefined(); + expect( + (await fs.readdir(root)).filter((name) => + name.startsWith(".rea-config-"), + ), + ).toEqual([]); + expect(state.publicationHandles[0]!.fd).toBe(-1); + } finally { + state.rejectStageRemoval = false; + await host.close?.(); + } + }, +); + +it("first-run publication exclusively refuses a destination created after its final check", async () => { + const root = await createTestTempDirectory("rea-config-exclusive-link-"); + const path = join(root, "client.json"); + const fs = + await vi.importActual( + "node:fs/promises", + ); + const edited = '{"created":"after final check"}\n'; + state.beforeFirstPublication = async () => { + await fs.writeFile(path, edited); + }; + await expect( + ClientConfigurationPublication.write(path, "{}", undefined), + ).rejects.toMatchObject({ code: "EEXIST" }); + expect(await fs.readFile(path, "utf8")).toBe(edited); + expect( + (await fs.readdir(root)).filter((name) => name.startsWith(".rea-config-")), + ).toEqual([]); +}); + +it("publication keeps its selected canonical parent when a directory symlink is retargeted", async () => { + const root = await createTestTempDirectory("rea-config-parent-selection-"); + const fs = + await vi.importActual( + "node:fs/promises", + ); + const selected = join(root, "selected"); + const foreign = join(root, "foreign"); + const alias = join(root, "alias"); + await fs.mkdir(selected); + await fs.mkdir(foreign); + await fs.symlink(selected, alias, "dir"); + const original = '{"original":true}\n'; + const unrelated = '{"foreign":true}\n'; + await fs.writeFile(join(selected, "client.json"), original); + await fs.writeFile(join(foreign, "client.json"), unrelated); + state.beforePublicationOpen = async () => { + state.beforePublicationOpen = undefined; + await fs.unlink(alias); + await fs.symlink(foreign, alias, "dir"); + }; + const publication = await ClientConfigurationPublication.write( + join(alias, "client.json"), + "{}", + original, + ); + await publication.verify(() => publication.assertCurrent()); + expect(await fs.readFile(join(selected, "client.json"), "utf8")).toBe("{}"); + expect(await fs.readFile(join(foreign, "client.json"), "utf8")).toBe( + unrelated, + ); + expect( + (await fs.readdir(selected)).filter((name) => + name.startsWith(".rea-config-"), + ), + ).toEqual([]); +}); + +it.each(["setup", "uninstall"] as const)( + "%s reads back the selected canonical parent after its alias is retargeted", + async (operation) => { + const root = await createTestTempDirectory( + "rea-config-adapter-parent-selection-", + ); + const fs = + await vi.importActual( + "node:fs/promises", + ); + const selected = join(root, "selected"); + const foreign = join(root, "foreign"); + const alias = join(root, "alias"); + await fs.mkdir(selected); + await fs.mkdir(foreign); + await fs.symlink(selected, alias, "dir"); + const original = + '{"mcpServers":{"rea":{"command":"rea","args":["mcp"]}},"keep":true}\n'; + const unrelated = '{"foreign":true}\n'; + await fs.writeFile(join(selected, "client.json"), original); + await fs.writeFile(join(foreign, "client.json"), unrelated); + state.failReadback = false; + state.beforePublicationOpen = async () => { + state.beforePublicationOpen = undefined; + await fs.unlink(alias); + await fs.symlink(foreign, alias, "dir"); + }; + expect( + ( + await runConfigurationChange( + operation, + root, + join(alias, "client.json"), + ) + ).status, + ).toBe(operation === "setup" ? "configured" : "removed"); + expect( + JSON.parse(await fs.readFile(join(selected, "client.json"), "utf8")), + ).toMatchObject({ keep: true }); + expect(await fs.readFile(join(foreign, "client.json"), "utf8")).toBe( + unrelated, + ); + expect( + await fs.readFile(join(selected, "client.json.rea.backup"), "utf8"), + ).toBe(original); + }, +);