diff --git a/doc/plans/2026-10-30-github-connection-sleep.md b/doc/plans/2026-10-30-github-connection-sleep.md index 9369d76cf4b..3e6de1148bb 100644 --- a/doc/plans/2026-10-30-github-connection-sleep.md +++ b/doc/plans/2026-10-30-github-connection-sleep.md @@ -51,7 +51,11 @@ Binding-only retries use the existing credential refresh lease and current secret resolver without repeating account/repository discovery. The initial OAuth callback uses the same saved debt and settlement path after connection activation. -Cloud binding calls retain the grant, its connection, and a company share lock. +Cloud binding calls acquire the company share lock before locking the grant and +its connection, matching company deletion's parent-before-child order. A deletion +that wins admission leaves no registration to apply; a handoff already admitted +keeps its fence through completion. The grant, connection, company status and +binding revision are re-read under those locks before calling Cloud. That admission fence prevents a concurrent disable, revoke, or archive from finishing before an already-admitted registration call. The API has no remote revision fence that could safely replace those locks with two short transactions. diff --git a/server/src/__tests__/github-connection-work.test.ts b/server/src/__tests__/github-connection-work.test.ts index af19526d232..5ee7db0df5e 100644 --- a/server/src/__tests__/github-connection-work.test.ts +++ b/server/src/__tests__/github-connection-work.test.ts @@ -1,4 +1,5 @@ import { notifyChatEndpointConfiguration } from "../services/chat-channels/configuration-work-notifications.js"; +import { deleteCompany } from "../services/company-deletion.js"; import { toolAccessService } from "../services/tool-access.js"; import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; import { randomUUID, generateKeyPairSync, verify, createHash, diffieHellman, hkdfSync, randomBytes, createCipheriv } from "node:crypto"; @@ -376,6 +377,146 @@ describeDatabase("GitHub coordinator with PostgreSQL and controlled Cloud HTTP ( } finally { heldBindingCompany = undefined; release(); await outcome; bindingGate = undefined; bindingArrived = undefined; bindingClosed = undefined; } }); + it("serializes binding handoff behind canonical company deletion without a child lock cycle", async () => { + const f = await fixture([]); + const [pending] = await db.update(connectionGrants).set({ providerTenant: { + ...f.grant.providerTenant, github: { ...f.grant.providerTenant!.github!, + bindingSync: { id: randomUUID(), removeInstallationIds: ["101"] } }, + } }).where(eq(connectionGrants.id, f.grant.id)).returning(); + let release!: () => void, entered!: (pid: number) => void; + const gate = new Promise(resolve => { release = resolve; }); + const companyLocked = new Promise(resolve => { entered = resolve; }); + const transaction = db.transaction.bind(db); + // Pause after the canonical deletion's actual company lock, before its child writes. + const spy = vi.spyOn(db, "transaction").mockImplementation(((callback: (tx: unknown) => Promise) => + transaction(async tx => { + const select = tx.select.bind(tx); + tx.select = ((...args: Parameters) => { + const query = select(...args), from = query.from.bind(query); + query.from = ((table: unknown) => { + const builder = from(table as typeof companies); + if (table === companies) { + const lock = builder.for.bind(builder); + builder.for = ((strength: Parameters[0]) => { + const locked = lock(strength); + if (strength === "no key update") { + const then = locked.then.bind(locked); + locked.then = ((resolve: (rows: unknown) => unknown, reject: (error: unknown) => unknown) => then(async rows => { + const [owner] = await tx.execute(sql`select pg_backend_pid() as pid`); + entered(Number(owner!.pid)); await gate; return resolve(rows); + }, reject)) as typeof locked.then; + } + return locked; + }) as typeof builder.for; + } + return builder; + }) as typeof query.from; + return query; + }) as typeof tx.select; + return callback(tx); + })) as typeof db.transaction); + const deletion = deleteCompany(db, f.company.id); + const deleted = Promise.allSettled([deletion]); + let handoff: Promise | undefined; + try { + const deletionPid = await companyLocked; + handoff = f.service.syncGitHubGrantBindings(f.connection, pending!); + const completed = Promise.allSettled([deletion, handoff]); + await vi.waitFor(async () => { + const waiting = await db.execute(sql`select pid from pg_stat_activity + where datname = current_database() and query like '%"companies"%' and query like '%for share%' + and ${deletionPid} = any(pg_blocking_pids(pid))`); + expect(waiting).toHaveLength(1); + }, { timeout: 5_000, interval: 20 }); + release(); + const results = await completed; + expect(results.map(row => row.status), JSON.stringify(results)).toEqual(["fulfilled", "fulfilled"]); + expect(await db.select().from(companies).where(eq(companies.id, f.company.id))).toHaveLength(0); + expect(await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id))).toHaveLength(0); + expect(bindings).toHaveLength(0); + } finally { release(); await deleted; await handoff?.catch(() => {}); spy.mockRestore(); } + }); + + it("lets an admitted binding finish before canonical company deletion", async () => { + const f = await fixture([]); + const [pending] = await db.update(connectionGrants).set({ providerTenant: { + ...f.grant.providerTenant, github: { ...f.grant.providerTenant!.github!, + bindingSync: { id: randomUUID(), removeInstallationIds: ["101"] } }, + } }).where(eq(connectionGrants.id, f.grant.id)).returning(); + heldBindingCompany = f.company.id; + let release!: () => void; + bindingGate = new Promise(resolve => { release = resolve; }); + const arrived = new Promise(resolve => { bindingArrived = resolve; }); + const handoff = f.service.syncGitHubGrantBindings(f.connection, pending!); + const handedOff = Promise.allSettled([handoff]); + let deletion: Promise | undefined; + try { + await arrived; + deletion = deleteCompany(db, f.company.id); + const deleted = Promise.allSettled([deletion]); + await vi.waitFor(async () => { + const waiting = await db.execute(sql`select pid from pg_stat_activity + where datname = current_database() and query like '%"companies"%' and query like '%for no key update%' + and cardinality(pg_blocking_pids(pid)) > 0`); + expect(waiting).toHaveLength(1); + }, { timeout: 5_000, interval: 20 }); + release(); + expect((await handedOff)[0]!.status).toBe("fulfilled"); + expect((await deleted)[0]!.status).toBe("fulfilled"); + expect(bindings).toEqual([expect.objectContaining({ grantId: f.grant.id, connectionId: f.connection.id, installationId: "101", active: false })]); + expect(await db.select().from(companies).where(eq(companies.id, f.company.id))).toHaveLength(0); + expect(await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id))).toHaveLength(0); + } finally { release(); await handedOff; await deletion?.catch(() => {}); heldBindingCompany = undefined; + bindingGate = undefined; bindingArrived = undefined; } + }); + + it.each(["revocation", "revision", "rollback"] as const)("revalidates a late %s after waiting for company admission", async change => { + const f = await fixture([]), revision = randomUUID(); + const [pending] = await db.update(connectionGrants).set({ providerTenant: { + ...f.grant.providerTenant, github: { ...f.grant.providerTenant!.github!, + bindingSync: { id: randomUUID(), removeInstallationIds: ["101"] } }, + } }).where(eq(connectionGrants.id, f.grant.id)).returning(); + let entered!: (pid: number) => void, release!: () => void; + const locked = new Promise(resolve => { entered = resolve; }); + const gate = new Promise(resolve => { release = resolve; }); + const mutation = db.transaction(async tx => { + await tx.select().from(companies).where(eq(companies.id, f.company.id)).for("no key update"); + const [owner] = await tx.execute(sql`select pg_backend_pid() as pid`); + entered(Number(owner!.pid)); await gate; + if (change === "revision") await tx.update(connectionGrants).set({ providerTenant: { + ...pending!.providerTenant, github: { ...pending!.providerTenant!.github!, + bindingSync: { id: revision, removeInstallationIds: ["202"] } }, + } }).where(eq(connectionGrants.id, f.grant.id)); + else await tx.update(connectionGrants).set({ status: "revoked" }).where(eq(connectionGrants.id, f.grant.id)); + if (change === "rollback") throw new Error("rollback late revocation"); + }); + const mutated = Promise.allSettled([mutation]); + let handoff: Promise | undefined; + try { + const ownerPid = await locked; + handoff = f.service.syncGitHubGrantBindings(f.connection, pending!); + const handedOff = Promise.allSettled([handoff]); + await vi.waitFor(async () => { + const waiting = await db.execute(sql`select pid from pg_stat_activity + where datname = current_database() and query like '%"companies"%' and query like '%for share%' + and ${ownerPid} = any(pg_blocking_pids(pid))`); + expect(waiting).toHaveLength(1); + }, { timeout: 5_000, interval: 20 }); + release(); + expect((await mutated)[0]!.status).toBe(change === "rollback" ? "rejected" : "fulfilled"); + expect((await handedOff)[0]!.status).toBe("fulfilled"); + const [saved] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id)); + if (change === "rollback") { + expect(bindings).toEqual([expect.objectContaining({ grantId: f.grant.id, installationId: "101", active: false })]); + expect(saved!.status).toBe("active"); expect(saved!.providerTenant!.github!.bindingSync).toBeUndefined(); + } else { + expect(bindings).toHaveLength(0); + if (change === "revocation") expect(saved!.status).toBe("revoked"); + else expect(saved!.providerTenant!.github!.bindingSync).toEqual({ id: revision, removeInstallationIds: ["202"] }); + } + } finally { release(); await mutated; await handoff?.catch(() => {}); } + }); + it("renews an installation created without metadata using the saved connection expiry", async () => { const f = await fixture(); vi.stubEnv("PAPERCLIP_SECRETS_MASTER_KEY", "01234567890123456789012345678901"); diff --git a/server/src/services/tool-access/github-grant-continuity.ts b/server/src/services/tool-access/github-grant-continuity.ts index c0f56c96f3c..13aaec986e7 100644 --- a/server/src/services/tool-access/github-grant-continuity.ts +++ b/server/src/services/tool-access/github-grant-continuity.ts @@ -234,6 +234,11 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector // Serialize external binding changes with grant/connection lifecycle writes. // Requests occur only for saved debt; no idle lock or connection is retained. return db.transaction(async tx => { + // Company deletion locks its parent before cascading to grants/connections. + // Keep that order while retaining admission through the external handoff. + const [company] = await tx.select({ status: companies.status }).from(companies) + .where(eq(companies.id, connection.companyId)).for("share").limit(1); + if (!company) return grant; const [current] = await tx.select().from(connectionGrants).where(and( eq(connectionGrants.id, grant.id), eq(connectionGrants.companyId, connection.companyId), eq(connectionGrants.connectionId, connection.id), @@ -243,9 +248,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector eq(toolConnections.id, connection.id), eq(toolConnections.companyId, connection.companyId), )).for("update").limit(1); if (!liveConnection || liveConnection.status !== "active" || !liveConnection.enabled) return current; - const [company] = await tx.select({ status: companies.status }).from(companies) - .where(eq(companies.id, connection.companyId)).for("share").limit(1); - if (company?.status !== "active") return current; + if (company.status !== "active") return current; const github = current.providerTenant?.github; if (!github?.bindingSync || github.bindingSync.id !== grant.providerTenant?.github?.bindingSync?.id) return current; const subject = current.kind === "agent" && current.subjectAgentId ? `agent:${current.subjectAgentId}`