diff --git a/doc/company-chat-deletion.md b/doc/company-chat-deletion.md new file mode 100644 index 00000000000..71eb88c9cf8 --- /dev/null +++ b/doc/company-chat-deletion.md @@ -0,0 +1,60 @@ +# Company deletion with chat endpoints + +Company deletion must remove chat endpoints before deleting their referenced issues or assigned agents. +The endpoint-to-agent foreign key uses RESTRICT, so waiting for the final company +cascade fails while deleting agents. This was reproduced by the Discord scheduling +producer test and by the owning company-removal suite. + +The chat owner participates in the existing company-deletion transaction. It +removes only the requested company's endpoint rows before issue deletion and before the agent lifecycle +participant runs. Existing endpoint foreign keys remove their dependent records. +The lifecycle owner still requires completed agent termination. The transaction +still rolls back all SQL cleanup if a later participant fails. No external calls, +new scheduler, schema change, or relaxed constraint is introduced. + +Discord command ownership tombstones have no cascading foreign keys and remain +unchanged. Deletion does not authorize a new endpoint to adopt an old application. +Other retained-history constraints, including voice-session restrictions, still +apply; this fix does not bypass them or change external provider teardown policy. + +Both new PostgreSQL regressions failed before the change. All six cleanup-removal +cases pass afterward, including deletion ordering, another company's untouched +endpoint, preserved namespace ownership, and rollback after a later failure. +The suite accepts a validated dedicated external PostgreSQL test database because +embedded PostgreSQL cannot start on this host. Server typecheck/build and remote +CI/review are tracked separately; this is not evidence of deployed host sleep. + + +Review expanded the fixture to include a conversation and publication linked to +an issue. Both regressions then failed because cleanup ran after issue deletion. +The chat participant now runs immediately after the company row is locked, before +any referenced issue or agent is removed. The same transaction preserves history +on rollback. This supplements the original endpoint-only proof; it does not claim +to remove existing voice-session restrictions. + +## Current-parent acceptance, October 27 + +This slice normally integrates health parent #17554 at +`8c1d293d1ae2b7db753ab20f5a2a2edec1fb0a62`. Only the four company-cleanup +files differ from that parent; no database, workflow, or provider change is added. +The parent must land before this prerequisite can merge into master. + +The remaining Ponytail finding reproduced on an initially empty dedicated +PostgreSQL 17 database: the real endpoint-deletion case and its cleanup failed +with missing-table errors. The external fixture now applies pending migrations +before creating its client and uses the shared 90-second embedded-bootstrap setup budget. The six complete +cleanup-removal cases pass on that same initially empty database. Test teardown +also deletes the ownership tombstones it creates; an independent query after +the complete suite finds zero rows. This fixture cleanup does not change the +production rule that company deletion preserves those tombstones. + +The failed fresh-database run remains part of the evidence. Server typecheck and +build pass with this repair. Fresh current-head CI/reviews remain separate gates. No full local +repository-suite or deployed suspension claim is made. + +The final review identified that the previously suggested 30-second setup limit +was below the repository's documented bootstrap budget under runner contention. +The fixture now imports and uses EMBEDDED_POSTGRES_TEST_TIMEOUT_MS. It changes no +assertion or production behavior. The earlier fresh-database failure and six-case +acceptance remain recorded; the shared timeout addresses documented setup cost, +not a reproduced production defect or a claim of measured CI contention here. diff --git a/server/src/__tests__/cleanup-removal-service.test.ts b/server/src/__tests__/cleanup-removal-service.test.ts index 6c7761e2b56..2bf989846d2 100644 --- a/server/src/__tests__/cleanup-removal-service.test.ts +++ b/server/src/__tests__/cleanup-removal-service.test.ts @@ -1,8 +1,18 @@ +import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; +import { agentLifecycleCompanyDeletion } from "../modules/agent-lifecycle/company-deletion.js"; import { createAgentLifecycle, configureAgentLifecycle } from "../services/agent-lifecycle.js"; import { randomUUID } from "node:crypto"; import { eq } from "drizzle-orm"; -import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { + applyPendingMigrations, + EMBEDDED_POSTGRES_TEST_TIMEOUT_MS, + chatEndpoints, + chatConversations, + chatPublications, + chatDiscordCommandOwners, + toolApplications, + toolConnections, activityLog, agents, companies, @@ -26,7 +36,8 @@ import { import { agentService } from "../services/agents.ts"; import { companyService } from "../services/companies.ts"; -const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const external = resolveExternalTestDatabaseUrl("PAPERCLIP_CLEANUP_TEST_DATABASE_URL"); +const embeddedPostgresSupport = external ? { supported: true, reason: undefined } : await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; if (!embeddedPostgresSupport.supported) { @@ -40,12 +51,18 @@ describeEmbeddedPostgres("cleanup removal services", () => { let tempDb: Awaited> | null = null; beforeAll(async () => { - tempDb = await startEmbeddedPostgresTestDatabase("paperclip-cleanup-removal-"); - db = createDb(tempDb.connectionString); + if (!external) tempDb = await startEmbeddedPostgresTestDatabase("paperclip-cleanup-removal-"); + if (external) await applyPendingMigrations(external); + db = createDb(external ?? tempDb!.connectionString); configureAgentLifecycle(db, { requiredPluginIds: async () => [], runPlugin: async () => "complete", runHost: async () => "complete" }); - }, 20_000); + }, EMBEDDED_POSTGRES_TEST_TIMEOUT_MS); afterEach(async () => { + vi.restoreAllMocks(); + await db.delete(chatEndpoints); + await db.delete(chatDiscordCommandOwners); + await db.delete(toolConnections); + await db.delete(toolApplications); await db.delete(heartbeatRunEvents); await db.delete(activityLog); await db.delete(issueReadStates); @@ -62,6 +79,7 @@ describeEmbeddedPostgres("cleanup removal services", () => { }); afterAll(async () => { + await db?.$client.end({ timeout: 1 }); await tempDb?.cleanup(); }); @@ -282,4 +300,55 @@ describeEmbeddedPostgres("cleanup removal services", () => { await expect(db.select().from(agents).where(eq(agents.id, agentId))).resolves.toHaveLength(0); await expect(db.select().from(companies).where(eq(companies.id, companyId))).resolves.toHaveLength(0); }); + async function seedEndpoint(companyId: string, agentId: string, issueId: string) { + const [app] = await db.insert(toolApplications).values({ companyId, name: "Discord", type: "chat" }).returning(); + const [connection] = await db.insert(toolConnections).values({ + companyId, applicationId: app.id, uid: randomUUID(), name: "Discord", transport: "chat_sdk", connectionPurpose: "channel", + }).returning(); + const [endpoint] = await db.insert(chatEndpoints).values({ + companyId, connectionId: connection.id, provider: "discord", publicId: randomUUID(), assignedAgentId: agentId, + }).returning(); + const [conversation] = await db.insert(chatConversations).values({ + companyId, endpointId: endpoint.id, issueId, externalConversationId: randomUUID(), externalLabel: "Saved chat", + }).returning(); + await db.insert(chatPublications).values({ + companyId, endpointId: endpoint.id, conversationId: conversation.id, issueId, + idempotencyKey: randomUUID(), payload: { text: "Saved response" }, state: "published", + }); + return endpoint; + } + + it("deletes company endpoints before their agents while preserving other companies and ownership tombstones", async () => { + const first = await seedFixture(), second = await seedFixture(); + const endpoint = await seedEndpoint(first.companyId, first.agentId, first.issueId); + const other = await seedEndpoint(second.companyId, second.agentId, second.issueId); + const [owner] = await db.insert(chatDiscordCommandOwners).values({ + applicationId: String(1_400_000_000_000_000_000n + BigInt("0x" + randomUUID().replaceAll("-", "").slice(0, 12))), + companyId: first.companyId, endpointId: endpoint.id, actionId: randomUUID(), + }).returning(); + await companyService(db).remove(first.companyId); + expect(await db.select().from(chatEndpoints).where(eq(chatEndpoints.id, endpoint.id))).toEqual([]); + expect(await db.select().from(chatConversations).where(eq(chatConversations.companyId, first.companyId))).toEqual([]); + expect(await db.select().from(chatPublications).where(eq(chatPublications.companyId, first.companyId))).toEqual([]); + expect(await db.select().from(agents).where(eq(agents.companyId, first.companyId))).toEqual([]); + expect(await db.select().from(companies).where(eq(companies.id, first.companyId))).toEqual([]); + expect(await db.select().from(chatEndpoints).where(eq(chatEndpoints.id, other.id))).toEqual([other]); + expect(await db.select().from(chatDiscordCommandOwners).where(eq(chatDiscordCommandOwners.applicationId, owner.applicationId))).toEqual([owner]); + }); + + it("rolls back endpoint cleanup when the later agent deletion fails", async () => { + const f = await seedFixture(); + const endpoint = await seedEndpoint(f.companyId, f.agentId, f.issueId); + vi.spyOn(agentLifecycleCompanyDeletion, "deleteCompanyData").mockImplementationOnce(async tx => { + expect(await tx.select().from(chatEndpoints).where(eq(chatEndpoints.id, endpoint.id))).toEqual([]); + throw new Error("agent deletion failed"); + }); + await expect(companyService(db).remove(f.companyId)).rejects.toThrow("agent deletion failed"); + expect(await db.select().from(chatEndpoints).where(eq(chatEndpoints.id, endpoint.id))).toEqual([endpoint]); + expect(await db.select().from(chatConversations).where(eq(chatConversations.endpointId, endpoint.id))).toHaveLength(1); + expect(await db.select().from(chatPublications).where(eq(chatPublications.endpointId, endpoint.id))).toHaveLength(1); + expect(await db.select().from(companies).where(eq(companies.id, f.companyId))).toHaveLength(1); + expect(await db.select().from(agents).where(eq(agents.id, f.agentId))).toHaveLength(1); + }); + }); diff --git a/server/src/services/chat-channels/company-deletion.ts b/server/src/services/chat-channels/company-deletion.ts new file mode 100644 index 00000000000..a9a3575a6da --- /dev/null +++ b/server/src/services/chat-channels/company-deletion.ts @@ -0,0 +1,9 @@ +import { chatEndpoints } from "@paperclipai/db"; +import { eq } from "drizzle-orm"; +import type { CompanyDeletionParticipant } from "../../lib/company-deletion.js"; + +export const chatCompanyDeletion: CompanyDeletionParticipant = { + async deleteCompanyData(tx, companyId) { + await tx.delete(chatEndpoints).where(eq(chatEndpoints.companyId, companyId)); + }, +}; diff --git a/server/src/services/company-deletion.ts b/server/src/services/company-deletion.ts index 5a78ef30208..ca79c6a4271 100644 --- a/server/src/services/company-deletion.ts +++ b/server/src/services/company-deletion.ts @@ -1,3 +1,4 @@ +import { chatCompanyDeletion } from "./chat-channels/company-deletion.js"; import { notifyToolConnectionHealth } from "./tool-connection-health-notifications.js"; import { notifyCompanyRetentionWork } from "./retention-work-notifications.js"; import { notifyToolReviewChange } from "./tool-review-work-notifications.js"; @@ -63,6 +64,7 @@ export async function deleteCompany(db: Db, id: string) { const [existing] = await tx.select({ id: companies.id }).from(companies) .where(eq(companies.id, id)).for("no key update"); if (!existing) return null; + await chatCompanyDeletion.deleteCompanyData(tx, id); await notifyDeliveryWork(tx, DELIVERY_QUEUES.skillSourceSync); await notifyDeliveryWork(tx, DELIVERY_QUEUES.secretProposalExpiry, id); await notifyToolReviewChange(tx, id);