diff --git a/.changeset/1691-alias-lock-timeout.md b/.changeset/1691-alias-lock-timeout.md new file mode 100644 index 000000000..3852535d3 --- /dev/null +++ b/.changeset/1691-alias-lock-timeout.md @@ -0,0 +1,9 @@ +--- +"@donadiosolutions/lcm": patch +--- + +Report a migration `apply` that cannot reconcile the destination project's +aliases because another session holds an uncommitted alias write as a +`migration-target-identity-unresolvable` refusal with evidence +`alias-lock-timeout`, after a 5 second wait, instead of a generic storage +error once the statement timeout cancels the lock request. diff --git a/docs/migration-operations.md b/docs/migration-operations.md index 88cc245aa..07baca5f2 100644 --- a/docs/migration-operations.md +++ b/docs/migration-operations.md @@ -188,6 +188,12 @@ creates identity; run `abort`, then a fresh `plan` and `apply`, which adds the m Rollback leaves the destination project and its alias rows in place and keeps the aliases in the local map. +Adding aliases to an existing destination project waits for other sessions that are writing +project aliases. If that write is still uncommitted after about 5 seconds, `apply` refuses with +`migration-target-identity-unresolvable` (exit 5) and evidence `alias-lock-timeout`, before it +takes the hold and without writing any alias. Run `apply` again once the other session has +finished. + `--dry-run` stops immediately after the confirmation fence, before the hold is taken or anything is captured. The emitted envelope reports the confirmed candidate with `applied: false`. diff --git a/src/migration/target-identity.ts b/src/migration/target-identity.ts index e41b73a84..c8f210122 100644 --- a/src/migration/target-identity.ts +++ b/src/migration/target-identity.ts @@ -1,4 +1,5 @@ import { + PostgreSqlIdentityAliasLockTimeoutError, PostgreSqlIdentityAliasNormalizedPathConflictError, PostgreSqlIdentityAliasPathConflictError, PostgreSqlIdentityConflictError, @@ -152,6 +153,12 @@ const ALIAS_CONFLICT = Object.freeze({ evidence: "alias-conflict", } as const); +const ALIAS_LOCK_TIMEOUT = Object.freeze({ + status: "refuse", + reason: "migration-target-identity-unresolvable", + evidence: "alias-lock-timeout", +} as const); + function isAliasConflict(error: unknown): boolean { return error instanceof PostgreSqlIdentityConflictError || error instanceof PostgreSqlIdentityAliasPathConflictError @@ -208,6 +215,7 @@ export async function resolveMigrationTargetProjectId(input: Readonly<{ try { if (await ensureProjectAliases({ machineId: input.machineId, projectId, aliases: rows }) === null) return ALIAS_CONFLICT; } catch (error) { + if (error instanceof PostgreSqlIdentityAliasLockTimeoutError) return ALIAS_LOCK_TIMEOUT; if (!isAliasConflict(error)) throw error; return ALIAS_CONFLICT; } diff --git a/src/storage/postgresql/identity-repository.ts b/src/storage/postgresql/identity-repository.ts index 6319af407..5e573df2a 100644 --- a/src/storage/postgresql/identity-repository.ts +++ b/src/storage/postgresql/identity-repository.ts @@ -5,7 +5,19 @@ import type { PostgreSqlQueryExecutor, PostgreSqlTransactionOptions, } from "./contracts.js"; -import { PostgreSqlCommitOutcomeUnknownError } from "./errors.js"; +import { + PostgreSqlCommitOutcomeUnknownError, + PostgreSqlStorageOperationError, +} from "./errors.js"; + +/** + * How long the alias reconcile waits for the `lcm.project_aliases` table lock + * before it gives up, so a conflicting uncommitted alias write is reported + * well before the configured statement timeout cancels the statement. + */ +const ALIAS_RECONCILE_LOCK_TIMEOUT_MS = 5_000; +/** lock_not_available (lock_timeout) and query_canceled (statement_timeout). */ +const ALIAS_LOCK_WAIT_SQLSTATES: ReadonlySet = new Set(["55P03", "57014"]); export interface PostgreSqlIdentityExecutor extends PostgreSqlQueryExecutor { transaction( @@ -233,6 +245,26 @@ export class PostgreSqlIdentityRegistrationError extends StorageOperationError { } } +/** + * The alias reconcile could not take the `lcm.project_aliases` table lock + * within its bounded wait because another session holds a conflicting + * uncommitted alias write. Nothing was written; retrying later is safe. + */ +export class PostgreSqlIdentityAliasLockTimeoutError extends StorageOperationError { + constructor(projectId: string) { + super( + "STORAGE_OPERATION_FAILED", + "postgresql", + projectId, + "identity", + "ensureProjectAliases", + ); + this.name = "PostgreSqlIdentityAliasLockTimeoutError"; + this.message = + `timed out waiting for the project alias lock while reconciling project ${projectId}; another session is writing project aliases`; + } +} + export class PostgreSqlIdentityCreateOutcomeUnknownError extends PostgreSqlCommitOutcomeUnknownError { constructor(readonly candidate: RemoteProject) { @@ -588,16 +620,62 @@ export class PostgreSqlIdentityRepository { * TABLE. After writing, the project's rows are read again and anything but * exactly the requested set throws, so the inserts roll back; a `null` * return after a write would commit them. + * + * The lock wait is bounded: the transaction sets a local lock_timeout of + * {@link ALIAS_RECONCILE_LOCK_TIMEOUT_MS} around the LOCK statement only. + * When that statement fails with a lock or statement timeout, because a + * conflicting alias write is still uncommitted, the call rejects with + * {@link PostgreSqlIdentityAliasLockTimeoutError}. The translation happens + * outside the transaction: the runtime rethrows the first failed query's + * error ahead of anything the callback throws. */ async ensureProjectAliases(input: { readonly machineId: string; readonly projectId: string; readonly aliases: readonly RemoteProjectAliasInput[]; }): Promise<{ readonly inserted: number } | null> { + const lock: { failure?: unknown } = {}; + try { + return await this.reconcileProjectAliases(input, lock); + } catch (error) { + if ( + error === lock.failure + && error instanceof PostgreSqlStorageOperationError + && error.sqlState !== null + && ALIAS_LOCK_WAIT_SQLSTATES.has(error.sqlState) + ) throw new PostgreSqlIdentityAliasLockTimeoutError(input.projectId); + throw error; + } + } + + private reconcileProjectAliases(input: { + readonly machineId: string; + readonly projectId: string; + readonly aliases: readonly RemoteProjectAliasInput[]; + }, lock: { failure?: unknown }): Promise<{ readonly inserted: number } | null> { return this.executor.transaction(async (transaction) => { + const options = projectScopeOptions("ensureProjectAliases", [input.projectId]); + const prior = await transaction.query<{ setting: string }>({ + text: "SELECT pg_catalog.current_setting('lock_timeout') AS setting", + }, options); + const previousSetting = prior.rows[0]!.setting; await transaction.query({ - text: "LOCK TABLE lcm.project_aliases IN SHARE ROW EXCLUSIVE MODE", - }, projectScopeOptions("ensureProjectAliases", [input.projectId])); + text: "SELECT pg_catalog.set_config('lock_timeout', $1::pg_catalog.text, true)", + values: [`${ALIAS_RECONCILE_LOCK_TIMEOUT_MS}ms`], + }, options); + try { + await transaction.query({ + text: "LOCK TABLE lcm.project_aliases IN SHARE ROW EXCLUSIVE MODE", + }, options); + } catch (error) { + // Nothing may be queried after this: the transaction is aborted. + lock.failure = error; + throw error; + } + await transaction.query({ + text: "SELECT pg_catalog.set_config('lock_timeout', $1::pg_catalog.text, true)", + values: [previousSetting], + }, options); const current = await transaction.query({ text: `SELECT project_id, machine_id, path, normalized_path, linked_at FROM lcm.project_aliases diff --git a/test/codecov-ownership/1691-alias-lock-timeout.yml b/test/codecov-ownership/1691-alias-lock-timeout.yml new file mode 100644 index 000000000..a966df202 --- /dev/null +++ b/test/codecov-ownership/1691-alias-lock-timeout.yml @@ -0,0 +1,9 @@ +references: ["#1691"] +rationale: >- + #1691 bounds the alias table lock wait in ensureProjectAliases and reports a + timeout as the closed alias-lock-timeout refusal in the migration identity + step. The new typed error lives in the PostgreSQL identity repository and + the mapping in target-identity.ts. No file moves; no topology change. +pins: + - ["src/storage/postgresql/identity-repository.ts", "integration-postgresql-identity"] + - ["src/migration/target-identity.ts", "unit-migration-cutover"] diff --git a/test/migration/target-identity.test.ts b/test/migration/target-identity.test.ts index 877a0fe61..e48fc744b 100644 --- a/test/migration/target-identity.test.ts +++ b/test/migration/target-identity.test.ts @@ -5,6 +5,7 @@ import { join } from "node:path"; import { describe, expect, it, vi } from "vitest"; import { lookupMigrationTargetProjectId, resolveMigrationTargetProjectId } from "../../src/migration/target-identity.js"; import { + PostgreSqlIdentityAliasLockTimeoutError, PostgreSqlIdentityAliasNormalizedPathConflictError, PostgreSqlIdentityAliasPathConflictError, PostgreSqlIdentityConflictError, @@ -174,6 +175,35 @@ describe("resolveMigrationTargetProjectId", () => { expect(result).toEqual(conflict); }); + const lockTimeout = { + status: "refuse", + reason: "migration-target-identity-unresolvable", + evidence: "alias-lock-timeout", + }; + + it("reuse: an alias lock wait that timed out refuses as alias-lock-timeout before any hold (#1691)", async () => { + const result = await resolveMigrationTargetProjectId({ + repository: repositoryStub({ + resolveProject: vi.fn(async () => ({ projectId: "project-1", alias: {} as never })), + ensureProjectAliases: vi.fn(async () => { throw new PostgreSqlIdentityAliasLockTimeoutError("project-1"); }), + }), + ...base, + }); + expect(result).toEqual(lockTimeout); + }); + + it("created: losing the canonical race, then timing out on the winner's alias lock, refuses as alias-lock-timeout (#1691)", async () => { + const result = await resolveMigrationTargetProjectId({ + repository: repositoryStub({ + resolveProject: vi.fn().mockResolvedValueOnce(null).mockResolvedValueOnce({ projectId: "winner", alias: {} as never }), + createProject: vi.fn(async () => { throw new PostgreSqlIdentityConflictError(MACHINE_ID, "/tmp/project", "winner", "loser"); }), + ensureProjectAliases: vi.fn(async () => { throw new PostgreSqlIdentityAliasLockTimeoutError("winner"); }), + }), + ...base, + }); + expect(result).toEqual(lockTimeout); + }); + it.each([ ["a lexical owner", new PostgreSqlIdentityConflictError(MACHINE_ID, "/tmp/p", "other", "project-1")], ["a stored path", new PostgreSqlIdentityAliasPathConflictError(MACHINE_ID, "/tmp/p", "project-1", "/a", "/b")], diff --git a/test/postgresql/identity.integration.ts b/test/postgresql/identity.integration.ts index 50a4e3410..3e6f6b99b 100644 --- a/test/postgresql/identity.integration.ts +++ b/test/postgresql/identity.integration.ts @@ -10,8 +10,10 @@ import { type IdentityRepository, } from "../../src/identity-service.js"; import { recoverMachineIdentity } from "../../src/machine-identity.js"; +import { resolveMigrationTargetProjectId } from "../../src/migration/target-identity.js"; import { clearProjectMapCache, resolveProjectIdentity } from "../../src/project-map.js"; import { + PostgreSqlIdentityAliasLockTimeoutError, PostgreSqlIdentityAliasNormalizedPathConflictError, PostgreSqlIdentityAliasPathConflictError, PostgreSqlIdentityCreateOutcomeUnknownError, @@ -19,8 +21,10 @@ import { PostgreSqlIdentityRepository, } from "../../src/storage/postgresql/identity-repository.js"; import { PostgreSqlCommitOutcomeUnknownError } from "../../src/storage/postgresql/errors.js"; +import { PostgreSqlRuntime } from "../../src/storage/postgresql/runtime.js"; import { assertHarnessReady, + settings, type PostgreSqlTestDatabase, withPostgreSqlTestDatabase, } from "./harness.js"; @@ -1023,4 +1027,77 @@ describe("PostgreSQL 18 machine and project identities", () => { .resolves.toEqual({ inserted: 0 }); }); }); + + it("a migration target alias reconcile blocked by an uncommitted alias writer refuses as alias-lock-timeout and writes nothing (#1691)", async () => { + await withPostgreSqlTestDatabase("identity-alias-lock-timeout", async (database) => { + await grantIdentityRuntimePrivileges(database); + // The harness runtime cancels statements and idle transactions after 5 s, which would race the 5 s lock wait. + const runtime = new PostgreSqlRuntime(settings(database.runtimeUrl, { statementTimeoutMs: 30_000 })); + try { + const repository = new PostgreSqlIdentityRepository(runtime); + const machine = await repository.registerMachine(`machine:${"8".repeat(64)}`, "Machine lock timeout"); + const project = await repository.createProject({ + machineId: machine.machineId, displayName: "Lock timeout target", path: "/work/lock-root", normalizedPath: "/work/lock-root", + }); + const scope = { domain: "identity" as const, operation: "aliasLockTimeoutWriter", projectId: project.projectId, projectIds: [project.projectId] }; + let writerInserted!: () => void; + const writerHasInserted = new Promise((resolve) => { writerInserted = resolve; }); + let releaseWriter!: () => void; + const writerMayRelease = new Promise((resolve) => { releaseWriter = resolve; }); + const writer = runtime.transaction(async (transaction) => { + await transaction.query({ + text: "INSERT INTO lcm.project_aliases (project_id, machine_id, path, normalized_path) VALUES ($1, $2, '/work/lock-stranger', '/work/lock-stranger')", + values: [project.projectId, machine.machineId], + }, scope); + writerInserted(); + await writerMayRelease; + throw new Error("roll the blocking alias write back"); + }, scope).catch(() => undefined); + const wanted = [ + { path: "/work/lock-root", normalizedPath: "/work/lock-root" }, + { path: "/work/lock-moved", normalizedPath: "/work/lock-moved" }, + ]; + let direct: unknown; + let directMs = 0; + let outcome: unknown; + let timer: NodeJS.Timeout | undefined; + try { + await Promise.race([writerHasInserted, writer.then(() => { throw new Error("the writer ended before it inserted"); })]); + const startedAt = Date.now(); + direct = await repository.ensureProjectAliases({ machineId: machine.machineId, projectId: project.projectId, aliases: wanted }) + .catch((error: unknown) => error); + directMs = Date.now() - startedAt; + const resolving = resolveMigrationTargetProjectId({ + repository, machineId: machine.machineId, displayName: "Lock timeout target", + authorityCanonicalPath: "/work/lock-root", aliases: ["/work/lock-moved"], + }).catch((error: unknown) => error); + // The bounded wait ends well inside the 30 s statement timeout; a hang here is the defect. + const deadline = new Promise((resolve) => { timer = setTimeout(() => resolve("still blocked on the alias table lock"), 15_000); }); + outcome = await Promise.race([resolving, deadline]); + } finally { + clearTimeout(timer); + releaseWriter(); + await writer; + } + expect(direct).toBeInstanceOf(PostgreSqlIdentityAliasLockTimeoutError); + expect(directMs).toBeGreaterThanOrEqual(4_000); + expect(directMs).toBeLessThan(15_000); + expect(outcome).toEqual({ + status: "refuse", + reason: "migration-target-identity-unresolvable", + evidence: "alias-lock-timeout", + }); + const rows = await database.migrator.query<{ path: string }>({ + text: "SELECT path FROM lcm.project_aliases WHERE project_id = $1 ORDER BY path", + values: [project.projectId], + }, { domain: "factory", operation: "aliasLockTimeoutRows" }); + expect(rows.rows.map((row) => row.path)).toEqual(["/work/lock-root"]); + // With the blocker gone the same reconcile completes. + await expect(repository.ensureProjectAliases({ machineId: machine.machineId, projectId: project.projectId, aliases: wanted })) + .resolves.toEqual({ inserted: 1 }); + } finally { + await runtime.close(); + } + }); + }, 60_000); }); diff --git a/test/storage/postgresql-identity-repository.test.ts b/test/storage/postgresql-identity-repository.test.ts index fe1d72087..a80fa240e 100644 --- a/test/storage/postgresql-identity-repository.test.ts +++ b/test/storage/postgresql-identity-repository.test.ts @@ -5,6 +5,7 @@ import type { PostgreSqlIdentityExecutor, } from "../../src/storage/postgresql/identity-repository.js"; import { + PostgreSqlIdentityAliasLockTimeoutError, PostgreSqlIdentityAliasNormalizedPathConflictError, PostgreSqlIdentityAliasPathConflictError, PostgreSqlIdentityCreateOutcomeUnknownError, @@ -18,7 +19,7 @@ import { PostgreSqlIdentityUnlinkPathOutcomeUnknownError, PostgreSqlIdentityUnlinkProjectOutcomeUnknownError, } from "../../src/storage/postgresql/identity-repository.js"; -import { PostgreSqlCommitOutcomeUnknownError } from "../../src/storage/postgresql/errors.js"; +import { PostgreSqlCommitOutcomeUnknownError, PostgreSqlStorageOperationError } from "../../src/storage/postgresql/errors.js"; import type { PostgreSqlQueryOptions, } from "../../src/storage/postgresql/contracts.js"; @@ -1654,6 +1655,8 @@ describe("PostgreSQL identity repository", () => { const statements: string[] = []; const db = executor((config) => { statements.push(config.text.trim().split("\n")[0]!); + if (config.text.includes("current_setting")) return result([{ setting: "7s" }]); + if (config.text.includes("set_config")) return result([]); if (config.text.startsWith("LOCK TABLE")) return result([]); if (config.text.includes("SELECT project_id FROM")) return result([{ project_id: projectId }]); if (config.text.includes("INSERT INTO lcm.project_aliases")) { @@ -1673,7 +1676,7 @@ describe("PostgreSQL identity repository", () => { expect(outcome).toEqual({ inserted: 0 }); expect(inserts).toEqual([]); expect(db.transaction).toHaveBeenCalledTimes(1); - expect(db.query.mock.calls[1]![0]).toMatchObject({ + expect(db.query.mock.calls[4]![0]).toMatchObject({ values: [projectId, machineId, [aliasRow.normalized_path, moved.normalizedPath], [aliasRow.path, moved.path]], }); }); @@ -1700,7 +1703,7 @@ describe("PostgreSQL identity repository", () => { it("locks the alias table before it reads and re-reads the whole project after writing", async () => { const { statements } = await ensure([aliasRow]); - expect(statements[0]).toBe("LOCK TABLE lcm.project_aliases IN SHARE ROW EXCLUSIVE MODE"); + expect(statements[2]).toBe("LOCK TABLE lcm.project_aliases IN SHARE ROW EXCLUSIVE MODE"); expect(statements.filter((statement) => statement.startsWith("SELECT project_id, machine_id")).length).toBe(2); const equal = await ensure([aliasRow, movedRow]); expect(equal.statements.filter((statement) => statement.startsWith("SELECT project_id, machine_id")).length).toBe(1); @@ -1720,5 +1723,92 @@ describe("PostgreSQL identity repository", () => { const failure = await ensure([aliasRow], []).catch((error: unknown) => error); expect(failure).toBeInstanceOf(PostgreSqlIdentityNotFoundError); }); + + describe("bounded alias lock wait (#1691)", () => { + const storageFailure = (sqlState: string | null) => new PostgreSqlStorageOperationError( + "STORAGE_OPERATION_FAILED", + { domain: "identity", operation: "ensureProjectAliases", projectId }, + sqlState, + false, + ); + // Models the real runtime: the first failed query is cached and rejects the transaction ahead of any callback error. + const runtimeLike = (failAt: (sql: string) => unknown, lockPolicy: "cached" | "other" = "cached") => { + const statements: string[] = []; + let cached: unknown; + const query = vi.fn(async (config: QueryConfig) => { + statements.push(config.text); + const failure = failAt(config.text); + if (failure !== undefined) { + cached ??= failure; + throw failure; + } + if (config.text.includes("current_setting")) return result([{ setting: "7s" }]); + return result([]); + }); + const transaction = vi.fn(async (callback: (executor: unknown) => Promise) => { + let callbackError: unknown; + try { + await callback({ query }); + } catch (error) { + callbackError = error; + } + if (lockPolicy === "other" && cached !== undefined) throw storageFailure("55P03"); + if (cached !== undefined) throw cached; + throw callbackError; + }); + const db = { query, transaction } as unknown as PostgreSqlIdentityExecutor; + return { repository: new PostgreSqlIdentityRepository(db), statements, query }; + }; + const reconcile = (repository: PostgreSqlIdentityRepository) => repository + .ensureProjectAliases({ machineId, projectId, aliases: wanted }) + .catch((error: unknown) => error); + const isLock = (sql: string) => sql.startsWith("LOCK TABLE"); + + it("bounds the lock wait with a local lock_timeout around the LOCK and restores the prior setting", async () => { + const { statements, db } = await ensure([aliasRow, movedRow]); + expect(statements[0]).toContain("current_setting('lock_timeout')"); + expect(statements[1]).toContain("set_config('lock_timeout'"); + expect(statements[2]).toBe("LOCK TABLE lcm.project_aliases IN SHARE ROW EXCLUSIVE MODE"); + expect(statements[3]).toContain("set_config('lock_timeout'"); + expect(statements[4]).toMatch(/^SELECT project_id, machine_id/u); + expect(db.query.mock.calls[1]![0]).toMatchObject({ values: ["5000ms"] }); + expect(db.query.mock.calls[3]![0]).toMatchObject({ values: ["7s"] }); + }); + + it.each(["55P03", "57014"])("reports %s on the LOCK as a typed alias lock timeout and queries nothing afterwards", async (sqlState) => { + const { repository, statements } = runtimeLike((sql) => isLock(sql) ? storageFailure(sqlState) : undefined); + const failure = await reconcile(repository); + expect(failure).toBeInstanceOf(PostgreSqlIdentityAliasLockTimeoutError); + expect(failure).toMatchObject({ code: "STORAGE_OPERATION_FAILED", domain: "identity", operation: "ensureProjectAliases", projectId, retryable: false }); + expect(failure).not.toBeInstanceOf(PostgreSqlStorageOperationError); + expect(statements).toHaveLength(3); + }); + + it.each([ + ["a later statement timing out", (sql: string) => sql.startsWith("SELECT project_id, machine_id") ? storageFailure("55P03") : undefined, "55P03"], + ["a later statement cancelled", (sql: string) => sql.startsWith("SELECT project_id, machine_id") ? storageFailure("57014") : undefined, "57014"], + ["the timeout setting failing", (sql: string) => sql.includes("set_config") ? storageFailure("55P03") : undefined, "55P03"], + ["the LOCK failing with another SQLSTATE", (sql: string) => isLock(sql) ? storageFailure("40001") : undefined, "40001"], + ["the LOCK failing without a SQLSTATE", (sql: string) => isLock(sql) ? storageFailure(null) : undefined, null], + ])("leaves %s unchanged", async (_label, failAt, sqlState) => { + const { repository } = runtimeLike(failAt); + const failure = await reconcile(repository); + expect(failure).toBeInstanceOf(PostgreSqlStorageOperationError); + expect(failure).toMatchObject({ sqlState }); + }); + + it("leaves a non-storage LOCK failure unchanged", async () => { + const original = new Error("connection reset"); + const { repository } = runtimeLike((sql) => isLock(sql) ? original : undefined); + expect(await reconcile(repository)).toBe(original); + }); + + it("leaves a different rejection of the transaction unchanged even after a LOCK timeout", async () => { + const { repository } = runtimeLike((sql) => isLock(sql) ? storageFailure("55P03") : undefined, "other"); + const failure = await reconcile(repository); + expect(failure).toBeInstanceOf(PostgreSqlStorageOperationError); + expect(failure).toMatchObject({ sqlState: "55P03" }); + }); + }); }); });