From 823521085a28f825e7fee1929d852d420042d1f7 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 18 Oct 2026 07:22:14 +0800 Subject: [PATCH] fix(setup): transfer unscoped registration cleanup ownership Preserve original RegularFileCleanupFailure capabilities when registration inspection or maintenance planning has no caller-supplied resource scope. Keep scoped diagnostics with the scope that already retained the descriptor. Cover configuration and policy sources, completed and malformed-UTF8 reads, all four exported reader/planner boundaries, exact descriptor retry after pathname replacement, no subsequent execution, and scoped cleanup retry. Validation: 79 tests across four focused files; the historical baseline fails all 16 new ownership-transfer controls with eight existing/scoped controls passing. check:fast, format:check, test discovery and six isolated completed public doctor comparisons passed. Scoped source review found no blocker. Real temporary descriptors and injected close rejection on macOS arm64 / Node24.18.0; no native Windows/Linux close faults or provider verification. Fixes #4910 --- src/application/ClientRegistrationStatus.ts | 20 ++- src/application/UpdateMaintenance.ts | 4 + .../setupClientConfigCleanupOwnership.test.ts | 165 +++++++++++++++++- 3 files changed, 186 insertions(+), 3 deletions(-) diff --git a/src/application/ClientRegistrationStatus.ts b/src/application/ClientRegistrationStatus.ts index 5c9e4e218f..9d6acea53b 100644 --- a/src/application/ClientRegistrationStatus.ts +++ b/src/application/ClientRegistrationStatus.ts @@ -13,6 +13,7 @@ import { isAbsolute } from "node:path"; import { z } from "zod"; import { valid } from "semver"; +import { RegularFileCleanupFailure } from "./RegularFileRead.js"; import { PRODUCT_IDENTITY } from "../identity.js"; import { CURRENT_ENTRYPOINT_PATH } from "../process/CurrentEntrypointPath.js"; import { @@ -119,7 +120,9 @@ export const readClientRegistrationStatuses = async ( await readClientRegistrationInspections(home, currentCommandPath, options) ).map(({ status }) => status); -/** Parse effective registrations and their client-specific activation once. */ +/** Parse effective registrations and their client-specific activation once. + * A supplied scope retains failed cleanup; otherwise transfer its exact failure. + */ export const readClientRegistrationInspections = async ( home: string, currentCommandPath: string | undefined = CURRENT_ENTRYPOINT_PATH, @@ -178,6 +181,11 @@ export const readClientRegistrationInspections = async ( options.resources, ); if (!configuration.ok) { + if ( + options.resources === undefined && + configuration.error.cleanupFailure !== undefined + ) + throw configuration.error.cleanupFailure; record({ client: client.name, config_path: configuration.error.path, @@ -200,6 +208,11 @@ export const readClientRegistrationInspections = async ( options.resources, ); if (!policy.ok) { + if ( + options.resources === undefined && + policy.error.cleanupFailure !== undefined + ) + throw policy.error.cleanupFailure; record({ client: client.name, config_path: configPath, @@ -313,6 +326,11 @@ export const readClientRegistrationInspections = async ( activation, ); } catch (cause: unknown) { + if ( + options.resources === undefined && + cause instanceof RegularFileCleanupFailure + ) + throw cause; record( unavailableStatus( client, diff --git a/src/application/UpdateMaintenance.ts b/src/application/UpdateMaintenance.ts index 6c8b7fbafd..e8a206da30 100644 --- a/src/application/UpdateMaintenance.ts +++ b/src/application/UpdateMaintenance.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { RegularFileCleanupFailure } from "./RegularFileRead.js"; import { classifyClientRegistrationCommand } from "./ClientRegistrationIdentity.js"; import { readClientRegistrationInspections } from "./ClientRegistrationStatus.js"; @@ -150,6 +151,9 @@ export const planIntegrationMaintenance = async ( ), }; } catch (cause: unknown) { + // Without a supplied scope, the typed failure is the caller's cleanup capability. + if (resources === undefined && cause instanceof RegularFileCleanupFailure) + throw cause; return { status: "unavailable", remediation: cause instanceof Error ? cause.message : String(cause), diff --git a/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts b/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts index aeeb8a1708..fd63214fb5 100644 --- a/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts +++ b/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts @@ -1,8 +1,25 @@ -import { mkdir, open as fsOpen, readFile, writeFile } from "node:fs/promises"; +import { + mkdir, + open as fsOpen, + readFile, + rename, + writeFile, + type FileHandle, +} from "node:fs/promises"; import { dirname, join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + readClientRegistrationInspections, + readClientRegistrationStatuses, +} from "../../../src/application/ClientRegistrationStatus.js"; +import { + existingMaintenanceScope, + planIntegrationMaintenance, +} from "../../../src/application/UpdateMaintenance.js"; +import { ArtifactResourceScope } from "../../../src/artifacts/ArtifactResourceScope.js"; +import { ok } from "../../../src/domain/result.js"; import type { DoctorHost } from "../../../src/application/Doctor.js"; import { ConfigurationError } from "../../../src/domain/configurationErrors.js"; import { @@ -34,7 +51,7 @@ beforeEach(async () => { afterEach(() => openMock.mockReset()); const holdReadHandle = (path: string, reason: string) => { - const closeAttempts: object[] = []; + const closeAttempts: FileHandle[] = []; let released = false; openMock.mockImplementation(async (...args) => { const handle = await actualFs.open(...args); @@ -332,3 +349,147 @@ describe("setup client configuration cleanup ownership", () => { } }); }); + +const unscopedRegistrationReaders: readonly { + readonly name: string; + readonly read: ( + home: string, + environment: NodeJS.ProcessEnv, + execute: Parameters[3], + ) => Promise; +}[] = [ + { + name: "inspections", + read: (home, environment) => + readClientRegistrationInspections(home, "/current/rea", { environment }), + }, + { + name: "statuses", + read: (home, environment) => + readClientRegistrationStatuses(home, "/current/rea", { environment }), + }, + { + name: "maintenance scope", + read: (home, environment) => + existingMaintenanceScope(home, "/current/rea", environment), + }, + { + name: "maintenance plan", + read: (home, environment, execute) => + planIntegrationMaintenance(home, "/current/rea", environment, execute), + }, +]; + +it.each( + unscopedRegistrationReaders.flatMap((reader) => + ["configuration", "policy"].flatMap((source) => + ["completed", "failed"].map((readOutcome) => ({ + ...reader, + source, + readOutcome, + })), + ), + ), +)( + "$name transfers the exact $source owner after a $readOutcome read without a supplied scope", + async ({ read, source, readOutcome }) => { + const home = await createTestTempDirectory( + "rea-registration-unscoped-close-", + ); + const config = join( + home, + source === "policy" ? ".gemini/settings.json" : ".cursor/mcp.json", + ); + await mkdir(dirname(config), { recursive: true }); + const registration = + '{"mcpServers":{"rea":{"command":"rea","args":["mcp"]}}}\n'; + await writeFile(config, registration); + const heldPath = source === "policy" ? join(home, "system.json") : config; + const heldText = source === "policy" ? "{}\n" : registration; + await writeFile( + heldPath, + readOutcome === "failed" ? Buffer.from([0xff]) : heldText, + ); + const environment = { + HOME: home, + GEMINI_CLI_SYSTEM_SETTINGS_PATH: join(home, "system.json"), + GEMINI_CLI_SYSTEM_DEFAULTS_PATH: join(home, "absent-defaults.json"), + }; + const held = holdReadHandle( + heldPath, + "held standalone registration descriptor", + ); + const execute = vi.fn(async () => + ok('{"status":"planned","plannedActions":[]}'), + ); + try { + const outcome = await read(home, environment, execute).then( + (value) => ({ kind: "returned" as const, value }), + (cause: unknown) => ({ kind: "threw" as const, cause }), + ); + expect(outcome).toMatchObject({ + kind: "threw", + cause: expect.any(RegularFileCleanupFailure), + }); + if ( + outcome.kind !== "threw" || + !(outcome.cause instanceof RegularFileCleanupFailure) + ) + throw new Error("Read lost its failed cleanup owner"); + const failure = outcome.cause; + expect(failure.path).toBe(heldPath); + if (readOutcome === "completed") + expect(failure.outcome).toEqual({ kind: "completed", value: heldText }); + else + expect(failure.outcome).toMatchObject({ + kind: "failed", + cause: { _tag: "InvalidUtf8FileError", path: heldPath }, + }); + expect(failure.owner.handle).toBe(held.closeAttempts[0]); + await expect(failure.owner.close()).rejects.toThrow("held standalone"); + await rename(heldPath, `${heldPath}.retired`); + await writeFile(heldPath, "replacement"); + held.release(); + await failure.owner.close(); + expect(failure.owner.handle.fd).toBe(-1); + expect(new Set(held.closeAttempts).size).toBe(1); + expect(await readFile(heldPath, "utf8")).toBe("replacement"); + expect(execute).not.toHaveBeenCalled(); + } finally { + held.release(); + for (const handle of new Set(held.closeAttempts)) await handle.close(); + } + }, +); + +it("keeps a supplied registration scope responsible for diagnostics and cleanup retry", async () => { + const home = await createTestTempDirectory("rea-registration-scoped-close-"); + const config = join(home, ".cursor/mcp.json"); + await mkdir(dirname(config), { recursive: true }); + await writeFile( + config, + '{"mcpServers":{"rea":{"command":"rea","args":["mcp"]}}}', + ); + const held = holdReadHandle(config, "held scoped registration descriptor"); + const resources = new ArtifactResourceScope(); + try { + expect( + ( + await readClientRegistrationStatuses(home, "/current/rea", { + environment: { HOME: home }, + resources, + }) + ).find((status) => status.client === "cursor"), + ).toMatchObject({ state: "invalid", command: [] }); + expect(resources.hasPending).toBe(true); + await expect(resources.close()).rejects.toThrow(); + held.release(); + await resources.close(); + expect(resources.hasPending).toBe(false); + expect(new Set(held.closeAttempts).size).toBe(1); + expect(held.closeAttempts[0]!.fd).toBe(-1); + } finally { + held.release(); + await resources.close(); + } +});