From 3a440d28c4e78ee56ecd52b19f14a38373b3516a Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Wed, 14 Oct 2026 20:01:34 -0700 Subject: [PATCH] fix(server): read existing grants before seeding role defaults (#16365) `insertMissingPrincipalGrants` now reads the existing permission keys first and inserts only missing grants. In the steady state no INSERT runs, so cloud-tenant auth resync no longer waits on row locks held by concurrent grant edits until the statement timeout (Sentry PAPERCLIP-BACKEND-3S). Co-Authored-By: Paperclip Co-Authored-By: Claude Opus 5.5 --- .../principal-access-compatibility.test.ts | 158 ++++++++++++++++++ .../src/middleware/cloud-tenant-actor.test.ts | 15 +- .../principal-access-compatibility.ts | 27 ++- 3 files changed, 196 insertions(+), 4 deletions(-) create mode 100644 server/src/__tests__/principal-access-compatibility.test.ts diff --git a/server/src/__tests__/principal-access-compatibility.test.ts b/server/src/__tests__/principal-access-compatibility.test.ts new file mode 100644 index 00000000000..88336433550 --- /dev/null +++ b/server/src/__tests__/principal-access-compatibility.test.ts @@ -0,0 +1,158 @@ +import { randomUUID } from "node:crypto"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { and, eq } from "drizzle-orm"; +import { companies, createDb, principalPermissionGrants } from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { grantsForHumanRole } from "../services/company-member-roles.js"; +import { + ensureHumanRoleDefaultGrants, + insertMissingPrincipalGrants, +} from "../services/principal-access-compatibility.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres principal grant tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`, + ); +} + +describeEmbeddedPostgres("insertMissingPrincipalGrants", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-principal-grants-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(principalPermissionGrants); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + async function createCompany() { + return db + .insert(companies) + .values({ + name: `Grant Seeding ${randomUUID()}`, + issuePrefix: `GS${randomUUID().slice(0, 6).toUpperCase()}`, + }) + .returning() + .then((rows) => rows[0]!); + } + + async function grantKeys(companyId: string, principalId: string) { + const rows = await db + .select({ permissionKey: principalPermissionGrants.permissionKey }) + .from(principalPermissionGrants) + .where( + and( + eq(principalPermissionGrants.companyId, companyId), + eq(principalPermissionGrants.principalType, "user"), + eq(principalPermissionGrants.principalId, principalId), + ), + ); + return rows.map((row) => row.permissionKey).sort(); + } + + it("inserts only the grants that are missing", async () => { + const company = await createCompany(); + const userId = `user-${randomUUID()}`; + const ownerGrants = grantsForHumanRole("owner"); + await insertMissingPrincipalGrants(db, { + companyId: company.id, + principalType: "user", + principalId: userId, + grants: ownerGrants.slice(0, 2), + grantedByUserId: null, + }); + + const inserted = await ensureHumanRoleDefaultGrants(db, { + companyId: company.id, + principalId: userId, + membershipRole: "owner", + grantedByUserId: null, + }); + + expect(inserted).toBe(ownerGrants.length - 2); + expect(await grantKeys(company.id, userId)).toEqual( + ownerGrants.map((grant) => grant.permissionKey).sort(), + ); + await expect( + ensureHumanRoleDefaultGrants(db, { + companyId: company.id, + principalId: userId, + membershipRole: "owner", + grantedByUserId: null, + }), + ).resolves.toBe(0); + }); + + it("does not wait on a transaction that is deleting grants that already exist", async () => { + const company = await createCompany(); + const userId = `user-${randomUUID()}`; + await ensureHumanRoleDefaultGrants(db, { + companyId: company.id, + principalId: userId, + membershipRole: "owner", + grantedByUserId: null, + }); + + let releaseLock!: () => void; + const lockReleased = new Promise((resolve) => { + releaseLock = resolve; + }); + let signalLockHeld!: () => void; + const lockHeld = new Promise((resolve) => { + signalLockHeld = resolve; + }); + const rollback = new Error("rollback"); + // A permission edit deletes and re-inserts a member's grants inside one + // transaction. Until it commits, an INSERT ... ON CONFLICT on the same keys + // must wait for it. + const holder = db + .transaction(async (tx) => { + await tx + .delete(principalPermissionGrants) + .where( + and( + eq(principalPermissionGrants.companyId, company.id), + eq(principalPermissionGrants.principalId, userId), + ), + ); + signalLockHeld(); + await lockReleased; + throw rollback; + }) + .catch((error) => { + if (error !== rollback) throw error; + }); + + try { + await lockHeld; + const seeding = ensureHumanRoleDefaultGrants(db, { + companyId: company.id, + principalId: userId, + membershipRole: "owner", + grantedByUserId: null, + }); + const outcome = await Promise.race([ + seeding, + new Promise<"blocked">((resolve) => setTimeout(() => resolve("blocked"), 2_000)), + ]); + expect(outcome).toBe(0); + } finally { + releaseLock(); + await holder; + } + }); +}); diff --git a/server/src/middleware/cloud-tenant-actor.test.ts b/server/src/middleware/cloud-tenant-actor.test.ts index 1e090fc5e33..6bf5f55ccd9 100644 --- a/server/src/middleware/cloud-tenant-actor.test.ts +++ b/server/src/middleware/cloud-tenant-actor.test.ts @@ -3,7 +3,14 @@ import express, { type Request } from "express"; import request from "supertest"; import { and, eq } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; -import { authUsers, companies, companyMemberships, instanceSettings, instanceUserRoles } from "@paperclipai/db"; +import { + authUsers, + companies, + companyMemberships, + instanceSettings, + instanceUserRoles, + principalPermissionGrants, +} from "@paperclipai/db"; import { actorMiddleware, cloudActorHeaderSourceFromHeaders, resolveCloudTenantActor } from "./auth.js"; import { idleAdmissionMiddleware, trackIdleRequestHandlers } from "./idle-admission.js"; import { idleWorkSnapshot, startTaskDrain, stopTaskDrain } from "../services/task-admission.js"; @@ -59,10 +66,14 @@ function createFakeDb(options: { return chain; }, select: () => { - if (options.selectThrows) throw new Error("select unavailable"); return { from: (table: unknown) => ({ where: (condition: unknown) => { + // Role-default grant seeding reads existing grants first; keep that + // read working so selectThrows exercises only the degradable reads. + if (options.selectThrows && table !== principalPermissionGrants) { + throw new Error("select unavailable"); + } selectWheres.push({ table, condition }); const rows = table === instanceSettings && settingsRow diff --git a/server/src/services/principal-access-compatibility.ts b/server/src/services/principal-access-compatibility.ts index cb1e86be82f..ca372d98372 100644 --- a/server/src/services/principal-access-compatibility.ts +++ b/server/src/services/principal-access-compatibility.ts @@ -1,4 +1,4 @@ -import { and, eq, notInArray } from "drizzle-orm"; +import { and, eq, inArray, notInArray } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { agents, companyMemberships, principalPermissionGrants } from "@paperclipai/db"; import type { PermissionKey, PrincipalType } from "@paperclipai/shared"; @@ -26,11 +26,34 @@ export async function insertMissingPrincipalGrants( ): Promise { if (input.grants.length === 0) return 0; + // Read before writing. Cloud tenant authentication re-seeds role defaults + // on every sync, and the grants almost always exist already. An INSERT ... + // ON CONFLICT still waits on another transaction that is deleting or + // re-inserting a conflicting row, which stalled authentication until the + // statement timeout. A plain read never waits on row locks. + const existing = await db + .select({ permissionKey: principalPermissionGrants.permissionKey }) + .from(principalPermissionGrants) + .where( + and( + eq(principalPermissionGrants.companyId, input.companyId), + eq(principalPermissionGrants.principalType, input.principalType), + eq(principalPermissionGrants.principalId, input.principalId), + inArray( + principalPermissionGrants.permissionKey, + input.grants.map((grant) => grant.permissionKey), + ), + ), + ); + const existingKeys = new Set(existing.map((row) => row.permissionKey)); + const missing = input.grants.filter((grant) => !existingKeys.has(grant.permissionKey)); + if (missing.length === 0) return 0; + const now = new Date(); const inserted = await db .insert(principalPermissionGrants) .values( - input.grants.map((grant) => ({ + missing.map((grant) => ({ companyId: input.companyId, principalType: input.principalType, principalId: input.principalId,