fix(setup): retain client configuration cleanup owners

This commit is contained in:
morluto
2026-10-11 06:30:31 +08:00
parent 279c942fe1
commit 67966c69a5
7 changed files with 487 additions and 34 deletions
+8 -1
View File
@@ -1,5 +1,8 @@
import { err, ok, type Result } from "../domain/result.js";
import { readRegularFileText } from "./RegularFileRead.js";
import {
RegularFileCleanupFailure,
readRegularFileText,
} from "./RegularFileRead.js";
import {
effectiveClientServer,
parseClientConfiguration,
@@ -13,6 +16,7 @@ export interface ClientConfigurationFileError {
readonly kind: "unreadable" | "malformed";
readonly path: string;
readonly detail: string;
readonly cleanupFailure?: RegularFileCleanupFailure;
}
/** Preserve named read failures and distinguish a missing source from an empty file. */
@@ -28,6 +32,9 @@ export const readConfigurationText = async (
kind: "unreadable",
path,
detail: `Configuration ${path} could not be read: ${cause instanceof Error ? cause.message : String(cause)}`,
...(cause instanceof RegularFileCleanupFailure
? { cleanupFailure: cause }
: {}),
});
}
};
+73
View File
@@ -1,5 +1,8 @@
import { describe, expect, it } from "vitest";
import { AnalysisError } from "../domain/analysisErrorBase.js";
import { projectAnalysisError } from "../domain/analysisErrorProjection.js";
import { ConfigurationError } from "../domain/configurationErrors.js";
import { SUPPORTED_NODE_VERSION_PROSE } from "../domain/runtimeVersion.js";
import { FakeSetupHost, options } from "./Setup.fixture.js";
import { runSetup } from "./Setup.js";
@@ -107,6 +110,76 @@ describe("setup workflow", () => {
});
});
describe("setup failure cleanup projection", () => {
it("projects a configuration failure together with failed host cleanup", async () => {
const configPath = "/fixture/home/vscode/settings.json";
const primary = new ConfigurationError("Client config could not be read", {
settings: [
{
setting: configPath,
constraint: "The selected config could not be read.",
},
],
cleanup: {
reason: "The admitted config handle remains open.",
resources: [configPath],
},
});
class SetupHostWithCleanupFailure extends FakeSetupHost {
close = async (): Promise<string> =>
"Setup could not release its client configuration resources.";
}
const host = new SetupHostWithCleanupFailure();
host.clients = [{ name: "codex", configPath }];
host.inspectClientConfiguration = async () => {
throw primary;
};
const failure = await runSetup(
{ ...options(true), clientIds: ["codex"] },
host,
).then(
() => undefined,
(cause: unknown) => cause,
);
expect(failure).toBeInstanceOf(AnalysisError);
if (!(failure instanceof AnalysisError))
throw new Error("Expected setup to preserve a typed analysis failure");
const projected = projectAnalysisError(failure);
expect(projected).toMatchObject({
code: "cleanup_incomplete",
details: {
cleanup: "incomplete",
resources: [configPath],
diagnostics: {
primary_error: {
code: "cleanup_incomplete",
details: { resources: [configPath] },
},
cleanup_error: {
code: "cleanup_incomplete",
details: { resources: [configPath] },
},
},
},
});
expect(projected.details).toMatchObject({
diagnostics: {
primary_error: { message: expect.stringContaining("selected config") },
cleanup_error: {
details: {
diagnostics: {
reason:
"Setup could not release its client configuration resources.",
},
},
},
},
});
});
});
describe("setup scoped readiness", () => {
it("limits Hopper host checks to workflows that request Hopper", async () => {
const platformHost = new FakeSetupHost("win32");
+14
View File
@@ -23,6 +23,9 @@ import {
hostRemediation,
systemSetupHost,
} from "./SetupHost.js";
import { AnalysisError } from "../domain/analysisErrorBase.js";
import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js";
import { ProviderCleanupError } from "../domain/providerCleanupError.js";
/** Discover, approve, and apply setup actions idempotently. */
export const runSetup = async (
@@ -48,6 +51,17 @@ export const runSetup = async (
}
if (threw) {
if (cleanupFailure !== undefined && primaryFailure instanceof AnalysisError)
throw analysisErrorWithCleanupFailure(
primaryFailure,
new ProviderCleanupError(
"setup",
primaryFailure.cleanupResources,
{ reason: cleanupFailure },
{ operation: "setup" },
),
"setup",
);
if (cleanupFailure !== undefined)
throw new AggregateError(
[primaryFailure, new Error(cleanupFailure)],
+91 -17
View File
@@ -22,7 +22,11 @@ import { PRODUCT_IDENTITY } from "../identity.js";
import { npxRegistrationCommand } from "./ClientRegistrationIdentity.js";
import { MCP_STARTUP_POLICY } from "../mcpStartupPolicy.js";
import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js";
import { readRegularFileText } from "./RegularFileRead.js";
import {
readRegularFileText,
RegularFileCleanupFailure,
} from "./RegularFileRead.js";
import type { ClientConfigurationFileError } from "./ClientConfigurationFiles.js";
import type {
ClientConfigurationInspection,
ClientConfigurationResult,
@@ -38,19 +42,22 @@ export const configureClientConfiguration = (
): Promise<ClientConfigurationResult> => {
if (client.configPathError !== undefined)
return Promise.resolve({ status: "failed", reason: "path" });
if (client.format === undefined || client.format === "unsupported")
return Promise.resolve({ status: "failed", reason: "readback" });
return configureClientDocument(client, environment, command, client.format);
return configureClientDocument(client, environment, command);
};
const configureClientDocument = async (
client: SetupClient,
environment: SetupProviderEnvironment,
command: readonly string[],
format: NonNullable<SetupClient["format"]>,
): Promise<ClientConfigurationResult> => {
const format = client.format;
if (format === undefined || format === "unsupported")
return { status: "failed", reason: "readback" };
const files = await readClientConfigurationFiles(client);
if (!files.ok) return { status: "failed", reason: "readback" };
if (!files.ok) {
throwCleanupFailure(files.error);
return { status: "failed", reason: "readback" };
}
const policy = await readClientPolicyBlock(
client,
undefined,
@@ -61,7 +68,11 @@ const configureClientDocument = async (
files.value.at(-1),
),
);
if (!policy.ok || policy.value !== undefined)
if (!policy.ok) {
throwCleanupFailure(policy.error);
return { status: "failed", reason: "readback" };
}
if (policy.value !== undefined)
return { status: "failed", reason: "readback" };
const transactionPath = await resolveClientConfigTransactionPath(
client.configPath,
@@ -72,6 +83,7 @@ const configureClientDocument = async (
try {
original = await readRegularFileText(transactionPath);
} catch (cause: unknown) {
if (cause instanceof RegularFileCleanupFailure) throw cause;
if (!isMissing(cause)) return { status: "failed", reason: "readback" };
}
let parsed: ClientConfigurationDocument;
@@ -85,9 +97,14 @@ const configureClientDocument = async (
parsed,
clientConfigurationDesired(client, environment, command, parsed),
);
if (!policy.ok || policy.value !== undefined)
if (!policy.ok) {
throwCleanupFailure(policy.error);
return { status: "failed", reason: "readback" };
}
if (policy.value !== undefined)
return { status: "failed", reason: "readback" };
} catch (cause: unknown) {
if (cause instanceof RegularFileCleanupFailure) throw cause;
// Malformed existing configuration fails the readback gate.
void cause;
return { status: "failed", reason: "readback" };
@@ -99,6 +116,33 @@ const configureClientDocument = async (
parsed,
);
if (registrationCurrent(parsed, desired)) return { status: "unchanged" };
return persistClientConfiguration({
client,
transactionPath,
format,
original,
parsed,
desired,
});
};
interface ClientConfigurationUpdate {
readonly client: SetupClient;
readonly transactionPath: string;
readonly format: NonNullable<SetupClient["format"]>;
readonly original: string | undefined;
readonly parsed: ClientConfigurationDocument;
readonly desired: unknown;
}
const persistClientConfiguration = async ({
client,
transactionPath,
format,
original,
parsed,
desired,
}: ClientConfigurationUpdate): Promise<ClientConfigurationResult> => {
const backupPath =
original === undefined ? undefined : `${client.configPath}.rea.backup`;
if (
@@ -163,6 +207,7 @@ const configureClientDocument = async (
return { status: "failed", reason: "readback" };
}
} catch (cause: unknown) {
if (cause instanceof RegularFileCleanupFailure) throw cause;
// Readback failure restores the transaction before reporting.
void cause;
await restoreConfig(transactionPath, original);
@@ -183,7 +228,10 @@ export const clientConfigurationAligned = async (
if (client.configPathError !== undefined) return false;
try {
const files = await readClientConfigurationFiles(client);
if (!files.ok) return false;
if (!files.ok) {
throwCleanupFailure(files.error);
return false;
}
const original = await readRegularFileText(client.configPath);
const parsed = parseClientConfiguration(original, client.format);
const policy = await readClientPolicyBlock(
@@ -191,8 +239,11 @@ export const clientConfigurationAligned = async (
parsed,
clientConfigurationDesired(client, providerEnvironment, command, parsed),
);
if (!policy.ok) {
throwCleanupFailure(policy.error);
return false;
}
return (
policy.ok &&
policy.value === undefined &&
registrationCurrent(
parsed,
@@ -205,6 +256,7 @@ export const clientConfigurationAligned = async (
)
);
} catch (cause: unknown) {
if (cause instanceof RegularFileCleanupFailure) throw cause;
// Unreadable configuration is treated as not aligned so setup repairs it.
void cause;
return false;
@@ -221,7 +273,13 @@ export const inspectClientConfiguration = async (
return { status: "invalid", remediation: client.configPathError };
if (client.format === "unsupported") return { status: "already_current" };
const files = await readClientConfigurationFiles(client);
if (!files.ok) return { status: "invalid", remediation: files.error.detail };
if (!files.ok) {
throwCleanupFailure(files.error);
return {
status: "invalid",
remediation: files.error.detail,
};
}
const policy = await readClientPolicyBlock(
client,
undefined,
@@ -232,8 +290,13 @@ export const inspectClientConfiguration = async (
files.value.at(-1),
),
);
if (!policy.ok)
return { status: "invalid", remediation: policy.error.detail };
if (!policy.ok) {
throwCleanupFailure(policy.error);
return {
status: "invalid",
remediation: policy.error.detail,
};
}
if (policy.value !== undefined)
return { status: "invalid", remediation: policy.value };
const transactionPath = await resolveClientConfigTransactionPath(
@@ -249,11 +312,11 @@ export const inspectClientConfiguration = async (
try {
original = await readRegularFileText(transactionPath);
} catch (cause: unknown) {
if (cause instanceof RegularFileCleanupFailure) throw cause;
if (isMissing(cause)) return { status: "create" };
return {
status: "invalid",
remediation:
"The configuration file could not be read. Check its permissions before rerunning setup.",
remediation: `The configuration file could not be read. Check its permissions before rerunning setup. ${cause instanceof Error ? cause.message : String(cause)}`,
};
}
try {
@@ -263,8 +326,13 @@ export const inspectClientConfiguration = async (
parsed,
clientConfigurationDesired(client, providerEnvironment, command, parsed),
);
if (!policy.ok)
return { status: "invalid", remediation: policy.error.detail };
if (!policy.ok) {
throwCleanupFailure(policy.error);
return {
status: "invalid",
remediation: policy.error.detail,
};
}
if (policy.value !== undefined)
return { status: "invalid", remediation: policy.value };
const desired = clientConfigurationDesired(
@@ -276,6 +344,7 @@ export const inspectClientConfiguration = async (
if (registrationCurrent(parsed, desired))
return { status: "already_current" };
} catch (cause: unknown) {
if (cause instanceof RegularFileCleanupFailure) throw cause;
// Malformed configuration is reported with the invalid remediation.
void cause;
return {
@@ -292,6 +361,11 @@ export const inspectClientConfiguration = async (
const isMissing = (cause: unknown): boolean =>
cause instanceof Error && "code" in cause && cause.code === "ENOENT";
const throwCleanupFailure = (error: ClientConfigurationFileError): void => {
if (error.cleanupFailure !== undefined) throw error.cleanupFailure;
};
const preserveConfigBackup = async (
source: string,
destination: string,
+110 -12
View File
@@ -1,6 +1,9 @@
import { access } from "node:fs/promises";
import { resolve } from "node:path";
import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js";
import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js";
import { ConfigurationError } from "../domain/configurationErrors.js";
import { PRODUCT_IDENTITY } from "../identity.js";
import { npxRegistrationCommand } from "./ClientRegistrationIdentity.js";
import {
@@ -30,6 +33,10 @@ import {
inspectClientConfiguration,
} from "./SetupClientConfiguration.js";
import { setupInstallFailure } from "./SetupInstallFailure.js";
import {
RegularFileCleanupFailure,
retryRegularFileCleanup,
} from "./RegularFileRead.js";
import { providerRegistrationEnvironment } from "./SetupRegistrationEnvironment.js";
import type {
SetupHost,
@@ -98,6 +105,7 @@ export const systemSetupHost = (
platform === "darwin"
? systemMacHopperInstallHost(homeDirectory, environment)
: undefined;
const resources = new ArtifactResourceScope();
return {
platform,
homeDirectory,
@@ -110,7 +118,7 @@ export const systemSetupHost = (
nodeVersion: process.versions.node,
macosVersion: () => doctorHost.macosVersion(),
linuxDistribution: readLinuxDistribution,
close: () => macHopperHost?.close() ?? Promise.resolve(undefined),
close: () => closeSetupResources(resources, macHopperHost),
initialSetupState: async (
scope?: DoctorScope,
): Promise<SetupInitialState> => {
@@ -162,17 +170,7 @@ export const systemSetupHost = (
detectedClients: () => detectClients(homeDirectory, platform, environment),
supportedClients: () =>
Promise.resolve(supportedClients(homeDirectory, platform, environment)),
configureClient: (client, providerEnvironment, command) =>
client.format === "unsupported"
? Promise.resolve({ status: "skipped" })
: configureClientConfiguration(client, providerEnvironment, command),
clientNeedsConfigure: (client, providerEnvironment, command) =>
client.format === "unsupported"
? Promise.resolve(false)
: clientConfigurationAligned(client, providerEnvironment, command).then(
(aligned) => !aligned,
),
inspectClientConfiguration: inspectClientConfiguration,
...clientConfigurationOperations(resources),
skillNeedsInstall: (clientIds) =>
canonicalSkillNeedsInstall(
homeDirectory,
@@ -186,6 +184,106 @@ export const systemSetupHost = (
};
};
const clientConfigurationOperations = (resources: ArtifactResourceScope) => ({
configureClient: (
client: SetupClient,
providerEnvironment: SetupProviderEnvironment,
command: readonly string[],
) =>
runClientConfigurationOperation(resources, async () =>
client.format === "unsupported"
? { status: "skipped" as const }
: configureClientConfiguration(client, providerEnvironment, command),
),
clientNeedsConfigure: (
client: SetupClient,
providerEnvironment: SetupProviderEnvironment,
command: readonly string[],
) =>
runClientConfigurationOperation(resources, async () =>
client.format === "unsupported"
? false
: !(await clientConfigurationAligned(
client,
providerEnvironment,
command,
)),
),
inspectClientConfiguration: (
client: SetupClient,
providerEnvironment: SetupProviderEnvironment,
command: readonly string[],
) =>
runClientConfigurationOperation(resources, () =>
inspectClientConfiguration(client, providerEnvironment, command),
),
});
const closeSetupResources = async (
resources: ArtifactResourceScope,
macHopperHost: ReturnType<typeof systemMacHopperInstallHost> | undefined,
): Promise<string | undefined> => {
const [resourceCleanup, hopperCleanup] = await Promise.allSettled([
resources.close(),
Promise.resolve().then(() => macHopperHost?.close()),
]);
const failures: string[] = [];
if (hopperCleanup.status === "rejected")
failures.push(
`Hopper host cleanup failed: ${errorMessage(hopperCleanup.reason)}`,
);
else if (hopperCleanup.value !== undefined)
failures.push(hopperCleanup.value);
if (resourceCleanup.status === "rejected")
failures.push(errorMessage(resourceCleanup.reason));
return failures.length === 0 ? undefined : failures.join("; ");
};
const runClientConfigurationOperation = async <Value>(
resources: ArtifactResourceScope,
operation: () => Promise<Value>,
): Promise<Value> => {
try {
return await resources.run(async () => {
try {
return await operation();
} catch (cause: unknown) {
if (!(cause instanceof RegularFileCleanupFailure)) throw cause;
const cleanup = await retryRegularFileCleanup(cause, resources);
const primary =
cleanup.outcome.kind === "failed"
? cleanup.outcome.cause
: cause.cleanupCause;
throw configurationCleanupError(
`Could not safely access client configuration at ${cause.path}: ${errorMessage(primary)}`,
cause,
cleanup.cleanup,
);
}
});
} catch (cause: unknown) {
if (cause instanceof ArtifactReaderFailure)
throw configurationCleanupError(cause.message, cause, cause.cleanup);
throw cause;
}
};
const configurationCleanupError = (
message: string,
cause: unknown,
cleanup:
| { readonly reason: string; readonly resources: readonly string[] }
| undefined,
): ConfigurationError =>
new ConfigurationError(message, {
cause,
settings: [{ setting: "client configuration", constraint: message }],
...(cleanup === undefined ? {} : { cleanup }),
});
const errorMessage = (cause: unknown): string =>
cause instanceof Error ? cause.message : String(cause);
/** Detect supported agents from their config files or stable installation markers. */
export const detectClients = async (
home: string,
@@ -0,0 +1,187 @@
import { mkdir, open as fsOpen, readFile, writeFile } from "node:fs/promises";
import { dirname, join } from "node:path";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import type { DoctorHost } from "../../../src/application/Doctor.js";
import { ConfigurationError } from "../../../src/domain/configurationErrors.js";
import {
filterClientsNeedingConfigure,
systemSetupHost,
} from "../../../src/application/SetupHost.js";
import { RegularFileCleanupFailure } from "../../../src/application/RegularFileRead.js";
import { configureDetectedClients } from "../../../src/application/SetupClients.js";
import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js";
vi.mock("node:fs/promises", async (importOriginal) => {
const actual = await importOriginal<typeof import("node:fs/promises")>();
return { ...actual, open: vi.fn(actual.open) };
});
const openMock = vi.mocked(fsOpen);
let actualFs: typeof import("node:fs/promises");
beforeEach(async () => {
actualFs =
await vi.importActual<typeof import("node:fs/promises")>(
"node:fs/promises",
);
openMock.mockImplementation((...args) => actualFs.open(...args));
});
afterEach(() => openMock.mockReset());
describe("setup client configuration cleanup ownership", () => {
it("stops client configuration and retries the same alignment handle from host close", async () => {
const home = await createTestTempDirectory("rea-setup-config-close-");
const environment = {
HOME: home,
XDG_CONFIG_HOME: join(home, "xdg"),
};
const host = systemSetupHost(
{ platform: "linux", homeDirectory: home } as DoctorHost,
environment,
);
const client = (await host.supportedClients()).find(
({ name }) => name === "vscode",
);
if (client === undefined) throw new Error("VS Code setup client missing");
await mkdir(dirname(client.configPath), { recursive: true });
const original = Buffer.from('{"servers": {"other": {}}}\n');
await writeFile(client.configPath, original);
const closeAttempts: object[] = [];
let allowClose = false;
openMock.mockImplementation(async (...args) => {
const handle = await actualFs.open(...args);
if (String(args[0]) !== client.configPath) return handle;
const nativeClose = handle.close.bind(handle);
vi.spyOn(handle, "close").mockImplementation(async () => {
closeAttempts.push(handle);
if (!allowClose) throw new Error("test holds the config handle open");
await nativeClose();
});
return handle;
});
const configure = vi.spyOn(host, "configureClient");
try {
const failure = await filterClientsNeedingConfigure(host, [client], {}, [
"rea",
"mcp",
])
.then((detectedClients) =>
configureDetectedClients({
host,
detectedClients,
providerEnvironment: {},
command: ["rea", "mcp"],
clients: {},
appliedActions: [],
}),
)
.then(
() => undefined,
(cause: unknown) => cause,
);
expect(failure).toBeInstanceOf(ConfigurationError);
expect(failure).toMatchObject({
_tag: "ConfigurationError",
cause: expect.any(RegularFileCleanupFailure),
cleanup: {
resources: [client.configPath],
reason: expect.stringContaining("test holds the config handle open"),
},
});
expect(configure).not.toHaveBeenCalled();
expect(closeAttempts).toHaveLength(2);
expect(closeAttempts[0]).toBe(closeAttempts[1]);
expect(await readFile(client.configPath)).toEqual(original);
allowClose = true;
const close = host.close;
if (close === undefined) throw new Error("Setup host close is missing");
expect(await close()).toBeUndefined();
expect(closeAttempts).toHaveLength(3);
expect(closeAttempts[2]).toBe(closeAttempts[0]);
} finally {
allowClose = true;
await host.close?.();
}
});
it("waits for an admitted configuration read and rejects later admissions", async () => {
const home = await createTestTempDirectory("rea-setup-config-inflight-");
const environment = {
HOME: home,
XDG_CONFIG_HOME: join(home, "xdg"),
};
const host = systemSetupHost(
{ platform: "linux", homeDirectory: home } as DoctorHost,
environment,
);
const client = (await host.supportedClients()).find(
({ name }) => name === "vscode",
);
if (client === undefined) throw new Error("VS Code setup client missing");
await mkdir(dirname(client.configPath), { recursive: true });
await writeFile(client.configPath, '{"servers": {}}\n');
let beginRead!: () => void;
const readStarted = new Promise<void>((resolve) => {
beginRead = resolve;
});
let unblockRead!: () => void;
const readGate = new Promise<void>((resolve) => {
unblockRead = resolve;
});
let heldRead = false;
openMock.mockImplementation(async (...args) => {
const handle = await actualFs.open(...args);
if (String(args[0]) !== client.configPath || heldRead) return handle;
heldRead = true;
const nativeRead = handle.read.bind(handle);
vi.spyOn(handle, "read").mockImplementation(async (...readArgs) => {
beginRead();
await readGate;
return nativeRead(...readArgs);
});
return handle;
});
let hostCloseFinished = false;
let alignment: Promise<boolean> | undefined;
try {
alignment = host.clientNeedsConfigure(client, {}, ["rea", "mcp"]);
await readStarted;
const close = host.close;
if (close === undefined) throw new Error("Setup host close is missing");
const closeResult = close().then((result) => {
hostCloseFinished = true;
return result;
});
const lateAdmission = await host
.clientNeedsConfigure(client, {}, ["rea", "mcp"])
.then(
() => undefined,
(cause: unknown) => cause,
);
expect(lateAdmission).toBeInstanceOf(ConfigurationError);
expect(lateAdmission).toMatchObject({
message: expect.stringContaining("Artifact resource scope is closed"),
});
expect(hostCloseFinished).toBe(false);
unblockRead();
await expect(alignment).resolves.toBe(true);
expect(await closeResult).toBeUndefined();
expect(hostCloseFinished).toBe(true);
} finally {
unblockRead();
await alignment?.catch(() => undefined);
await host.close?.();
}
});
});
@@ -58,11 +58,11 @@ describe("setup with configured but unavailable providers", () => {
},
]),
});
const host = {
const createHost = () => ({
...systemSetupHost(doctor),
detectedClients: () => Promise.resolve([client]),
supportedClients: () => Promise.resolve([client]),
};
});
const selected = {
...options(true),
clientIds: ["cursor"],
@@ -71,7 +71,7 @@ describe("setup with configured but unavailable providers", () => {
// This lane checks registration effects; no deep provider is required.
readinessScope: { clients: [], providers: [], skill: false },
};
const first = await runSetup(selected, host);
const first = await runSetup(selected, createHost());
expect(first.status).toBe("ready");
expect(first.clients.cursor).toMatchObject({ status: "configured" });
const configured = await readFile(client.configPath, "utf8");
@@ -83,7 +83,7 @@ describe("setup with configured but unavailable providers", () => {
readFile(`${client.configPath}.rea.backup`, "utf8"),
).resolves.toBe(original);
const second = await runSetup(selected, host);
const second = await runSetup(selected, createHost());
expect(second.status).toBe("ready");
expect(second.plannedActions).toEqual([]);
expect(second.appliedActions).toEqual([]);