From 9e1694211d0cf561920f82605a9a7c237d9dcd9f Mon Sep 17 00:00:00 2001 From: Dotta Date: Fri, 30 Oct 2026 21:22:18 -0500 Subject: [PATCH] fix(github): fence retry attempts and restored binding intent Keep provider backoff tied to the credential revision actually attempted. Rebuild restoration debt and exclude active installations from retirement commands. Co-Authored-By: Paperclip --- .../2026-10-30-github-connection-sleep.md | 6 +- .../__tests__/github-connection-work.test.ts | 116 +++++++++++++++++- .../src/services/github-connection-events.ts | 4 +- .../tool-access/github-grant-continuity.ts | 33 +++-- 4 files changed, 144 insertions(+), 15 deletions(-) diff --git a/doc/plans/2026-10-30-github-connection-sleep.md b/doc/plans/2026-10-30-github-connection-sleep.md index 6d381373b19..9369d76cf4b 100644 --- a/doc/plans/2026-10-30-github-connection-sleep.md +++ b/doc/plans/2026-10-30-github-connection-sleep.md @@ -43,7 +43,11 @@ A grant stores a unique `bindingSync` revision and removed installation IDs befo Cloud registration calls. Successful delivery clears only the locked current revision. Failure retains the saved intent and an exponential retry deadline, including provider Retry-After advice. A restart loads that deadline from the -grant. Binding-only retries use the existing credential refresh lease and current +grant. A retry write compares the attempted credential/installation revision under +the grant lock and cannot delay work committed by a newer revision. Restoration +creates a new binding revision and removes active installations from the pending +retirement list; binding commands also reject legacy conflicting retirements. +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. diff --git a/server/src/__tests__/github-connection-work.test.ts b/server/src/__tests__/github-connection-work.test.ts index 1a268d57605..783ed45de43 100644 --- a/server/src/__tests__/github-connection-work.test.ts +++ b/server/src/__tests__/github-connection-work.test.ts @@ -6,9 +6,9 @@ import { createServer, type Server } from "node:http"; import { once } from "node:events"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { eq, sql } from "drizzle-orm"; -import { applyPendingMigrations, companies, activityLog, companySecrets, connectionGrants, createDb, EMBEDDED_POSTGRES_TEST_TIMEOUT_MS, toolApplications, toolConnections } from "@paperclipai/db"; +import { applyPendingMigrations, companies, activityLog, companySecrets, connectionEventDeliveries, connectionGrants, createDb, EMBEDDED_POSTGRES_TEST_TIMEOUT_MS, toolApplications, toolConnections } from "@paperclipai/db"; import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; -import { createGitHubGrantContinuity, githubGrantContinuityDeadline } from "../services/tool-access/github-grant-continuity.js"; +import { createGitHubGrantContinuity, githubGrantContinuityDeadline, githubContinuityRevision } from "../services/tool-access/github-grant-continuity.js"; import { createPaperclipCloudConnector } from "../services/paperclip-cloud-connector.js"; import { githubConnectionEventService } from "../services/github-connection-events.js"; import { createDeliveryWorkCoordinator } from "../services/delivery-work-coordinator.js"; @@ -36,6 +36,7 @@ describeDatabase("GitHub coordinator with PostgreSQL and controlled Cloud HTTP ( const signing = generateKeyPairSync("ed25519"), sealing = generateKeyPairSync("x25519"); let releaseRefresh: (() => void) | undefined, refreshArrived: (() => void) | undefined; let refreshGate: Promise | undefined; + let discoveryFailureGate: Promise | undefined, discoveryArrived: (() => void) | undefined; let heldBindingCompany: string | undefined, bindingGate: Promise | undefined; let bindingArrived: (() => void) | undefined, bindingClosed: (() => void) | undefined; const realFetch = globalThis.fetch; @@ -86,6 +87,10 @@ describeDatabase("GitHub coordinator with PostgreSQL and controlled Cloud HTTP ( res.writeHead(failBindings ? 503 : 200, { "content-type": "application/json" }).end("{}"); return; } githubRequests++; + if (req.url === "/user" && discoveryFailureGate) { + discoveryArrived?.(); await discoveryFailureGate; + res.writeHead(503, { "content-type": "application/json", "retry-after": "86400" }).end("{}"); return; + } const body = req.url === "/user" ? { id: 42, login: "octocat" } : req.url!.startsWith("/user/installations?") ? { installations: [{ id: 101, repository_selection: "all", account: { login: "octocat" } }] } : { total_count: 1, repositories: [{ id: 999, full_name: "octocat/repo" }] }; @@ -187,6 +192,108 @@ describeDatabase("GitHub coordinator with PostgreSQL and controlled Cloud HTTP ( expect(await restarted.nextGitHubConnectionContinuityAt()).toBeNull(); }); + it.each(["credential", "installation"])("does not attach a withheld provider failure to newer %s work", async change => { + const f = await fixture(["101"]); + vi.spyOn(globalThis, "fetch").mockImplementation((url, init) => String(url).startsWith("https://api.github.com") + ? realFetch(`${origin}${new URL(String(url)).pathname}${new URL(String(url)).search}`, init) : realFetch(url, init)); + await db.update(connectionGrants).set({ providerTenant: { ...f.grant.providerTenant, + oauth: { ...f.grant.providerTenant!.oauth, accessTokenExpiresAt: new Date(Date.now() + 30 * 60_000).toISOString() } } }).where(eq(connectionGrants.id, f.grant.id)); + let release!: () => void; + const arrived = new Promise(resolve => { discoveryArrived = resolve; }); + discoveryFailureGate = new Promise(resolve => { release = resolve; }); + const pending = f.service.sweepGitHubConnectionContinuity(f.company.id); + const finished = Promise.allSettled([pending]); + try { + await Promise.race([arrived, pending.then(() => { throw new Error("Expected held provider request"); })]); + const revision = randomUUID(); + const expiry = new Date(Date.now() + 10 * 60_000).toISOString(); + await db.transaction(async tx => { + await notifyGitHubCredentialChange(tx, f.connection); + await tx.update(connectionGrants).set({ providerTenant: change === "credential" + ? sql`jsonb_set(${connectionGrants.providerTenant}, '{oauth,accessTokenExpiresAt}', ${JSON.stringify(expiry)}::jsonb)` + : sql`jsonb_set(${connectionGrants.providerTenant}, '{github,bindingSync}', ${JSON.stringify({ id: revision, removeInstallationIds: ["202"] })}::jsonb)`, + updatedAt: new Date() }).where(eq(connectionGrants.id, f.grant.id)); + }); + release(); + expect(await pending).toMatchObject({ due: 1, failed: 1 }); + const [saved] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id)); + expect(saved!.providerTenant!.github!.continuityRetry).toBeUndefined(); + if (change === "credential") expect(saved!.providerTenant!.oauth!.accessTokenExpiresAt).toBe(expiry); + else expect(saved!.providerTenant!.github!.bindingSync!.id).toBe(revision); + expect(githubGrantContinuityDeadline(f.connection, saved!)).toBeLessThanOrEqual(Date.now()); + discoveryFailureGate = undefined; + const restarted = createGitHubGrantContinuity(db, f.dependencies); + expect(await restarted.sweepGitHubConnectionContinuity(f.company.id)).toMatchObject({ due: 1, refreshed: 1, failed: 0 }); + expect(bindings).toContainEqual(expect.objectContaining({ installationId: "101", active: true })); + if (change === "installation") expect(bindings).toContainEqual(expect.objectContaining({ installationId: "202", active: false })); + const [settled] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id)); + expect(settled!.providerTenant!.github!.bindingSync).toBeUndefined(); + const next = await restarted.nextGitHubConnectionContinuityAt(f.company.id); + expect(next).toBeLessThan(Date.now() + 24 * 60 * 60_000); + } finally { release(); await finished; discoveryFailureGate = undefined; discoveryArrived = undefined; } + }); + + it("retains provider backoff for the actual attempted revision after its own credential rotation", async () => { + const f = await fixture(["101"]); + await db.update(connectionGrants).set({ providerTenant: { ...f.grant.providerTenant, + oauth: { ...f.grant.providerTenant!.oauth, accessTokenExpiresAt: new Date(Date.now() + 30 * 60_000).toISOString() } } }).where(eq(connectionGrants.id, f.grant.id)); + vi.spyOn(globalThis, "fetch").mockImplementation((url, init) => String(url).startsWith("https://api.github.com") + ? realFetch(`${origin}${new URL(String(url)).pathname}${new URL(String(url)).search}`, init) : realFetch(url, init)); + const owner = createGitHubGrantContinuity(db, { ...f.dependencies, refreshOAuthGrantCredentials: async () => { + const [rotated] = await db.update(connectionGrants).set({ providerTenant: sql`jsonb_set(${connectionGrants.providerTenant}, '{oauth}', ${JSON.stringify({ + accessTokenExpiresAt: new Date(Date.now() + 10 * 60_000).toISOString(), refreshedAt: new Date().toISOString(), + })}::jsonb)` }).where(eq(connectionGrants.id, f.grant.id)).returning(); + return rotated!; + } }); + discoveryFailureGate = Promise.resolve(); + try { + expect(await owner.sweepGitHubConnectionContinuity(f.company.id)).toMatchObject({ due: 1, failed: 1 }); + const [saved] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id)); + expect(saved!.providerTenant!.github!.continuityRetry!.revision).toBe(githubContinuityRevision(saved!)); + expect(await owner.nextGitHubConnectionContinuityAt(f.company.id)).toBeGreaterThan(Date.now() + 23 * 60 * 60_000); + const calls = githubRequests; + expect(await createGitHubGrantContinuity(db, f.dependencies).sweepGitHubConnectionContinuity(f.company.id)).toMatchObject({ due: 0 }); + expect(githubRequests).toBe(calls); + } finally { discoveryFailureGate = undefined; } + }); + + it.each([["suspend", "unsuspend"], ["unsuspend", "suspend"]])("reconciles same-batch installation order %s then %s and duplicate replay", async (first, last) => { + const f = await fixture(["101"]); + await db.update(connectionGrants).set({ providerTenant: { ...f.grant.providerTenant, + github: { ...f.grant.providerTenant!.github!, bindingSync: { id: randomUUID(), removeInstallationIds: ["202"] } } } }).where(eq(connectionGrants.id, f.grant.id)); + const events = [first, last].map(action => ({ id: randomUUID(), provider: "github" as const, event: "installation", action, + installationId: "101", repositoryId: null, createdAt: new Date(Date.now() + 1000).toISOString(), bindingIds: [`${f.grant.id}_101`], payload: { installationId: "101" } })); + const acknowledge = vi.fn(async () => events.length); + const ingress = githubConnectionEventService(db, { connector: { ...f.connector, leaseEvents: async () => ({ leaseId: "installation-batch", events }), acknowledgeEvents: acknowledge } }); + expect(await ingress.pollOnce({ forDelivery: true })).toMatchObject({ processed: 2, failed: 0 }); + const [saved] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id)); + const active = last === "unsuspend"; + expect(saved!.providerTenant!.github!.installationIds).toEqual(active ? ["101"] : []); + expect(saved!.providerTenant!.github!.bindingSync!.removeInstallationIds.sort()).toEqual(active ? ["202"] : ["101", "202"]); + expect(githubGrantContinuityDeadline(f.connection, saved!)).toBe(0); + await f.service.syncGitHubGrantBindings(f.connection, saved!); + expect(bindings.filter(binding => binding.installationId === "101")).toEqual([expect.objectContaining({ active })]); + expect(bindings.filter(binding => binding.installationId === "202")).toEqual([expect.objectContaining({ active: false })]); + const [settled] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id)); + expect(settled!.providerTenant!.github!.bindingSync).toBeUndefined(); + // Batch duplicates must retain terminal receipts and emit no registration changes. + expect(await ingress.pollOnce({ forDelivery: true })).toMatchObject({ processed: 0, duplicate: active ? 2 : 0, ignored: active ? 0 : 2, failed: 0 }); + await f.service.syncGitHubGrantBindings(f.connection, saved!); + expect(bindings).toHaveLength(2); + expect((await db.select().from(connectionGrants).where(eq(connectionGrants.id, f.grant.id)))[0]!.providerTenant).toEqual(settled!.providerTenant); + expect((await db.select().from(connectionEventDeliveries).where(eq(connectionEventDeliveries.companyId, f.company.id))).map(receipt => receipt.status)).toEqual(["processed", "processed"]); + expect(acknowledge).toHaveBeenCalledTimes(2); + }); + + it("does not retire active installations from a legacy conflicting binding intent", async () => { + const f = await fixture(["101"]); + const [saved] = await db.update(connectionGrants).set({ providerTenant: { ...f.grant.providerTenant, + github: { ...f.grant.providerTenant!.github!, bindingSync: { id: randomUUID(), removeInstallationIds: ["101", "202"] } } } }).where(eq(connectionGrants.id, f.grant.id)).returning(); + await f.service.syncGitHubGrantBindings(f.connection, saved!); + expect(bindings.filter(binding => binding.installationId === "101")).toEqual([expect.objectContaining({ active: true })]); + expect(bindings.filter(binding => binding.installationId === "202")).toEqual([expect.objectContaining({ active: false })]); + }); + it("preserves webhook removal debt across an actual signed, held Cloud token rotation and lease cleanup", async () => { const f = await fixture(["101"]); vi.stubEnv("PAPERCLIP_SECRETS_MASTER_KEY", "01234567890123456789012345678901"); @@ -327,6 +434,11 @@ describeDatabase("GitHub coordinator with PostgreSQL and controlled Cloud HTTP ( expect(leases).toBeLessThanOrEqual(3); expect(load.mock.calls.length).toBeLessThanOrEqual(2); if (loadTimes.length === 2) expect(loadTimes[1]! - loadTimes[0]!).toBeGreaterThanOrEqual(5_000); if (leaseTimes.length === 3) expect(leaseTimes[2]! - leaseTimes[1]!).toBeGreaterThanOrEqual(5_000); + // A hint arriving during the first SQL pass retains one spaced generation. + // Start the empty-window assertion after that concrete work has settled. + await vi.advanceTimersByTimeAsync(31_000); await settled(coordinator); + expect(leases).toBeLessThanOrEqual(3); expect(load.mock.calls.length).toBeLessThanOrEqual(2); + if (loadTimes.length === 2) expect(loadTimes[1]! - loadTimes[0]!).toBeGreaterThanOrEqual(5_000); const quiet = { leases, loads: load.mock.calls.length }; await vi.advanceTimersByTimeAsync(60_000); expect({ leases, loads: load.mock.calls.length }).toEqual(quiet); diff --git a/server/src/services/github-connection-events.ts b/server/src/services/github-connection-events.ts index 2e08d887dee..a70213dc44b 100644 --- a/server/src/services/github-connection-events.ts +++ b/server/src/services/github-connection-events.ts @@ -330,9 +330,9 @@ export function githubConnectionEventService( // Lifecycle webhooks carry IDs, not the user token’s complete repository view. // Discard the snapshot until Refresh access verifies it again. accessRevision: randomUUID(), - ...(unavailable ? { bindingSync: { + ...(unavailable || github.bindingSync || !github.installationIds.includes(binding.installationId) ? { bindingSync: { id: randomUUID(), removeInstallationIds: [...new Set([ - ...(github.bindingSync?.removeInstallationIds ?? []), binding.installationId, + ...(github.bindingSync?.removeInstallationIds ?? []), ...(unavailable ? [binding.installationId] : []), ])].filter(id => !installationIds.includes(id)), } } : {}), repositories: undefined, diff --git a/server/src/services/tool-access/github-grant-continuity.ts b/server/src/services/tool-access/github-grant-continuity.ts index e3095597d4b..c0f56c96f3c 100644 --- a/server/src/services/tool-access/github-grant-continuity.ts +++ b/server/src/services/tool-access/github-grant-continuity.ts @@ -18,6 +18,8 @@ function asRecord(value: unknown): Record { return {}; } +type RecordGitHubAttempt = (grant: typeof connectionGrants.$inferSelect) => void; + type GitHubContinuityDependencies = { now: () => Date; currentCloudConnector: () => PaperclipCloudConnector | null; @@ -30,6 +32,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector connection: typeof toolConnections.$inferSelect, initialGrant: typeof connectionGrants.$inferSelect, actor?: ActorInfo, + recordAttempt?: RecordGitHubAttempt, ) { await actor?.managementContext?.reauthorize(); return measureRunPhase("connectors.github.refresh", async () => { @@ -38,6 +41,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector connection, initialGrant, actor, + recordAttempt, ); } catch (error) { if ( @@ -56,7 +60,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector ), )); if (!current || current.status !== "active") throw error; - return refreshManagedGitHubGrantAccessOnce(connection, current, actor); + return refreshManagedGitHubGrantAccessOnce(connection, current, actor, recordAttempt); } }); } @@ -65,6 +69,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector connection: typeof toolConnections.$inferSelect, initialGrant: typeof connectionGrants.$inferSelect, actor?: ActorInfo, + recordAttempt?: RecordGitHubAttempt, ) { await actor?.managementContext?.reauthorize(); let grant = await measureRunPhase("connectors.github.credentials", () => refreshOAuthGrantCredentials({ @@ -73,6 +78,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector grantId: initialGrant.id, actor, })); + recordAttempt?.(grant); const resolveAccessToken = async () => measureRunPhase("connectors.github.credentials", async () => { const accessRef = grant.credentialSecretRefs.find( (ref) => ref.configPath === "oauth.access_token", @@ -113,6 +119,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector forceRefresh: true, actor, })); + recordAttempt?.(grant); accessToken = await resolveAccessToken(); try { metadata = await measureRunPhase("connectors.github.repositories", () => loadGitHubGrantMetadata( @@ -204,7 +211,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector return { updated, previousGitHub }; }); - return measureRunPhase("connectors.github.webhooks", () => syncGitHubGrantBindings(connection, updated, actor, accessToken)); + return measureRunPhase("connectors.github.webhooks", () => syncGitHubGrantBindings(connection, updated, actor, accessToken, recordAttempt)); } async function syncGitHubGrantBindings( @@ -212,6 +219,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector initialGrant: typeof connectionGrants.$inferSelect, actor?: ActorInfo, accessToken?: string, + recordAttempt?: RecordGitHubAttempt, ) { await actor?.managementContext?.reauthorize(); const cloudConnector = currentCloudConnector(); @@ -243,9 +251,11 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector const subject = current.kind === "agent" && current.subjectAgentId ? `agent:${current.subjectAgentId}` : current.kind === "user" ? current.subjectUserId : null; if (!subject) throw unprocessable("GitHub webhook subject is unavailable"); + recordAttempt?.(grant); const changes = [ ...github.installationIds.map(installationId => ({ installationId, active: true as const })), - ...github.bindingSync.removeInstallationIds.map(installationId => ({ installationId, active: false as const })), + ...github.bindingSync.removeInstallationIds.filter(id => !github.installationIds.includes(id)) + .map(installationId => ({ installationId, active: false as const })), ]; for (let offset = 0; offset < changes.length; offset += 4) { const outcomes = await Promise.allSettled(changes.slice(offset, offset + 4).map(change => cloudConnector.setWebhookBinding({ @@ -265,13 +275,14 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector }); } - async function retainRetry(connection: typeof toolConnections.$inferSelect, grantId: string, error?: unknown, minimumMs = 30_000) { + async function retainRetry(connection: typeof toolConnections.$inferSelect, grantId: string, attemptedRevision: string, error?: unknown, minimumMs = 30_000) { await db.transaction(async tx => { const [current] = await tx.select().from(connectionGrants).where(and( eq(connectionGrants.id, grantId), eq(connectionGrants.companyId, connection.companyId), eq(connectionGrants.connectionId, connection.id), eq(connectionGrants.status, "active"), )).for("update").limit(1); - if (!current?.providerTenant?.github) return; + // Backoff belongs to the attempted credential/installation revision. + if (!current?.providerTenant?.github || githubContinuityRevision(current) !== attemptedRevision) return; const attempts = Math.min(16, (current.providerTenant.github.continuityRetry?.attempts ?? 0) + 1); const advised = error instanceof PaperclipCloudConnectorError ? error.retryAt : error instanceof HttpError ? asRecord(error.details).retryAt : undefined; @@ -280,7 +291,7 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector await notifyGitHubCredentialChange(tx, connection); await tx.update(connectionGrants).set({ providerTenant: sql`jsonb_set(${connectionGrants.providerTenant}, '{github,continuityRetry}', ${JSON.stringify({ - at, attempts, revision: githubContinuityRevision(current), + at, attempts, revision: attemptedRevision, })}::jsonb)`, updatedAt: now(), }).where(and(eq(connectionGrants.id, current.id), eq(connectionGrants.companyId, connection.companyId))); @@ -324,18 +335,20 @@ export function createGitHubGrantContinuity(db: Db, { now, currentCloudConnector let refreshed = 0; let failed = 0; for (const row of due) { + let attemptedRevision = githubContinuityRevision(row.grant); + const recordAttempt: RecordGitHubAttempt = grant => { attemptedRevision = githubContinuityRevision(grant); }; try { const actor: ActorInfo = { actorType: "system", actorId: "github-connection-continuity" }; const updated = row.grant.providerTenant?.github?.bindingSync - ? await syncGitHubGrantBindings(row.connection, row.grant, actor) - : await refreshManagedGitHubGrantAccess(row.connection, row.grant, actor); + ? await syncGitHubGrantBindings(row.connection, row.grant, actor, undefined, recordAttempt) + : await refreshManagedGitHubGrantAccess(row.connection, row.grant, actor, recordAttempt); // A successful provider response can still leave an unchanged/short-lived // credential due. Retain that obligation with bounded durable backoff. const next = githubGrantContinuityDeadline(row.connection, updated); - if (next !== null && next <= now().getTime()) await retainRetry(row.connection, updated.id, undefined, 60 * 60_000); + if (next !== null && next <= now().getTime()) await retainRetry(row.connection, updated.id, githubContinuityRevision(updated), undefined, 60 * 60_000); refreshed += 1; } catch (error) { - await retainRetry(row.connection, row.grant.id, error); + await retainRetry(row.connection, row.grant.id, attemptedRevision, error); failed += 1; } }