mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 20:21:50 +01:00
fix(server): remove chat endpoints before company agents (#17561)
## Thinking Path > - Paperclip manages agent companies and their connected chat channels. > - A company deletion must remove dependent data in one transaction. > - Chat endpoints reference assigned agents with a restrictive foreign key. > - The current deletion order purges agents before those endpoints. > - This pull request removes company endpoint rows and chat history before issue and agent deletion. > - The existing lifecycle checks, company isolation, and rollback remain intact. ## Linked Issues or Issue Description **What happened?** Deleting a company with a chat endpoint fails on the endpoint's assigned-agent foreign key. The failure was found while verifying the company-deletion wake in #17557. **Expected behavior** Company deletion removes endpoint dependencies before their referenced issues and agents. It preserves other companies and global Discord ownership tombstones. A later failure rolls back the cleanup. **Steps to reproduce** Create a company, an agent, and a Discord endpoint assigned to that agent. Complete agent termination, then delete the company. The old order fails with the restrictive endpoint-to-agent foreign key. **Paperclip version or commit** This prerequisite includes merged #17554 at `94a780286f` and supports #17557. Only four cleanup-owned paths differ from that landed parent. Related #16465 covers the deletion feature flag, not dependency order. ## What Changed - Add a chat-owned company-deletion participant using the existing transaction contract. - Remove only the requested company's endpoints before issue deletion and the agent lifecycle participant. - Preserve namespace tombstones and existing restrictive foreign keys. - Add real PostgreSQL regressions for ordering, isolation, and rollback. - Migrate a validated disposable external database before fixture use, use the shared PostgreSQL bootstrap budget for setup, and remove fixture-owned Discord tombstones during test teardown. ## Verification Published head: `9766cc61c42fe0f73e241bbe99f9d9f145f4b11c`. Parent #17554 is merged. This branch reconciles the actual landed health squash `94a780286f` and targets master. Fresh reviews and CI are required for this exact head. - The latest review requested the repository's shared EMBEDDED_POSTGRES_TEST_TIMEOUT_MS (90 seconds) for bootstrap. This replaces the earlier suggested 30-second limit. All six existing cases pass on another initially empty database with that constant; no test assertion or production code changes. The documented contention budget is repository evidence, not a locally reproduced 40-second bootstrap failure. - All six complete cleanup-removal service cases pass on an initially empty dedicated PostgreSQL 17 database with the updated health parent. - The pre-fix real endpoint-deletion case failed with missing-table errors because external fixture setup skipped migrations. Applying pending migrations before client use fixes that failure; no assertion is removed or weakened. - An independent query after the complete suite finds zero fixture ownership tombstones. Production company deletion still preserves those records. - Cases cover endpoint removal before issue and agent references, other-company isolation, namespace ownership preservation, and rollback after a later failure. Both product regressions failed before the original ordering fix. - Server typecheck and build pass with the updated parent and fixture repair at `21be75d2b0`. The landed-parent reconciliation changes only the lockfile; all source and test bytes remain identical. All six cases pass again with the new installed dependency patch. The initial scope-proof command raced a shared `origin/master` update; a new proof pins the actual landed parent and confirms the four-file scope. A later native-checkpoint merge `09ad9adce2` has no overlap with these four files. - [Owning acceptance document](https://github.com/paperclipai/paperclip/blob/9766cc61c42fe0f73e241bbe99f9d9f145f4b11c/doc/company-chat-deletion.md#current-parent-acceptance-october-27) retains the failing baseline, parent scope, and limitations. No full local repository-suite or deployed suspension claim is made. Older inherited gateway/combined-stack evidence is separate from this four-file fix. ## Risks - The endpoint cleanup shares the caller's transaction. A later deletion failure must restore the endpoint; the regression verifies that boundary. - Existing retained voice-history restrictions and external provider teardown policy remain unchanged. This fix does not bypass restrictive foreign keys. - No migrations, lockfile changes, or workflow edits. ## Model Used OpenAI Codex, GPT-6 family, with repository inspection, code edits, and tool-assisted tests. The exact runtime variant and context-window size are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either linked existing issues or described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal Paperclip ticket id - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
70c8646337
commit
f8563d2507
4 files changed
+145
-5
No files matched your search
@@ -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.
|
||||
@@ -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<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | 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);
|
||||
});
|
||||
|
||||
});
|
||||
@@ -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));
|
||||
},
|
||||
};
|
||||
@@ -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);
|
||||
|
||||
Reference in new issue
Block a user