From 9aa065a38c71bd6a34d35249b1f1502ba46f5654 Mon Sep 17 00:00:00 2001 From: Dotta Date: Fri, 5 Jun 2026 03:56:39 +0000 Subject: [PATCH] Make deleted-comment cleanup atomic Co-Authored-By: Paperclip --- .../document-annotations-service.test.ts | 44 ++++++++ .../issue-comment-cancel-routes.test.ts | 43 +++++--- server/src/routes/issues.ts | 46 ++++---- server/src/services/document-annotations.ts | 104 ++++++++++-------- server/src/services/issues.ts | 9 +- 5 files changed, 161 insertions(+), 85 deletions(-) diff --git a/server/src/__tests__/document-annotations-service.test.ts b/server/src/__tests__/document-annotations-service.test.ts index 054e57db..14f0612f 100644 --- a/server/src/__tests__/document-annotations-service.test.ts +++ b/server/src/__tests__/document-annotations-service.test.ts @@ -270,4 +270,48 @@ describeEmbeddedPostgres("documentAnnotationService", () => { await expect(db.select().from(documentAnnotationComments)).resolves.toHaveLength(0); }); + + it("does not report already-resolved empty threads as newly resolved during linked comment cleanup", async () => { + const { companyId, issueId, document } = await createIssueWithDocument(); + const [issueComment] = await db + .insert(issueComments) + .values({ + companyId, + issueId, + authorType: "user", + authorUserId: "board-user", + body: "Delete this linked comment from a resolved thread", + }) + .returning(); + + const thread = await annotations.createThread( + issueId, + "plan", + { + baseRevisionId: document.latestRevisionId!, + baseRevisionNumber: document.latestRevisionNumber, + selector: { + quote: { exact: "selected text", prefix: "Alpha ", suffix: " omega" }, + position: { normalizedStart: 6, normalizedEnd: 19, markdownStart: 6, markdownEnd: 19 }, + }, + body: "Linked annotation body", + issueCommentId: issueComment.id, + }, + { actorType: "user", actorId: "board-user", userId: "board-user" }, + ); + + await db + .update(documentAnnotationThreads) + .set({ status: "resolved", resolvedByUserId: "board-user", resolvedAt: new Date("2026-06-05T03:05:00.000Z") }) + .where(eq(documentAnnotationThreads.id, thread.id)); + + const cleanup = await annotations.cleanupForIssueCommentDeletion( + issueId, + issueComment.id, + { actorType: "user", actorId: "board-user", userId: "board-user" }, + ); + + expect(cleanup.deletedCommentIds).toEqual([thread.comments[0]!.id]); + expect(cleanup.resolvedThreadIds).toEqual([]); + }); }); diff --git a/server/src/__tests__/issue-comment-cancel-routes.test.ts b/server/src/__tests__/issue-comment-cancel-routes.test.ts index a50ae488..8c841f08 100644 --- a/server/src/__tests__/issue-comment-cancel-routes.test.ts +++ b/server/src/__tests__/issue-comment-cancel-routes.test.ts @@ -195,15 +195,19 @@ describe.sequential("issue comment cancel routes", () => { mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null }); mockIssueService.getComment.mockResolvedValue(makeComment()); mockIssueService.removeComment.mockResolvedValue(makeComment()); - mockIssueService.tombstoneComment.mockResolvedValue(makeComment({ - body: "", - metadata: null, - deletedAt: new Date("2026-04-11T15:05:00.000Z"), - deletedByType: "user", - deletedByAgentId: null, - deletedByUserId: "local-board", - deletedByRunId: null, - })); + mockIssueService.tombstoneComment.mockImplementation(async (_commentId, _actor, options) => { + const deleted = makeComment({ + body: "", + metadata: null, + deletedAt: new Date("2026-04-11T15:05:00.000Z"), + deletedByType: "user", + deletedByAgentId: null, + deletedByUserId: "local-board", + deletedByRunId: null, + }); + await options?.afterTombstone?.(deleted, "tx"); + return deleted; + }); mockAccessService.canUser.mockResolvedValue(false); mockAccessService.hasPermission.mockResolvedValue(false); mockFeedbackService.listIssueVotesForUser.mockResolvedValue([]); @@ -337,13 +341,17 @@ describe.sequential("issue comment cancel routes", () => { expect(JSON.stringify(res.body)).not.toContain("Sensitive original comment body"); expect(JSON.stringify(res.body)).not.toContain("Sensitive metadata copy"); expect(mockIssueService.removeComment).not.toHaveBeenCalled(); - expect(mockIssueService.tombstoneComment).toHaveBeenCalledWith("comment-1", { - actorType: "user", - agentId: null, - userId: "local-board", - runId: null, - }); - expect(mockIssueReferenceService.syncComment).toHaveBeenCalledWith("comment-1"); + expect(mockIssueService.tombstoneComment).toHaveBeenCalledWith( + "comment-1", + { + actorType: "user", + agentId: null, + userId: "local-board", + runId: null, + }, + expect.objectContaining({ afterTombstone: expect.any(Function) }), + ); + expect(mockIssueReferenceService.syncComment).toHaveBeenCalledWith("comment-1", "tx"); expect(mockDocumentAnnotationService.cleanupForIssueCommentDeletion).toHaveBeenCalledWith( "11111111-1111-4111-8111-111111111111", "comment-1", @@ -351,8 +359,9 @@ describe.sequential("issue comment cancel routes", () => { actorType: "user", userId: "local-board", }), + "tx", ); - expect(mockIssueReferenceService.deleteCommentSource).toHaveBeenCalledWith("annotation-comment-1"); + expect(mockIssueReferenceService.deleteCommentSource).toHaveBeenCalledWith("annotation-comment-1", "tx"); const deletedActivity = mockLogActivity.mock.calls.find((call) => call[1]?.action === "issue.comment_deleted")?.[1]; expect(deletedActivity).toEqual(expect.objectContaining({ action: "issue.comment_deleted", diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index fcf6dbaf..c2945eb1 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -5674,29 +5674,37 @@ export function issueRoutes( return; } - const deleted = await svc.tombstoneComment(commentId, { - actorType: actor.actorType, - agentId: actor.agentId, - userId: actor.actorType === "user" ? actor.actorId : null, - runId: actor.runId, - }); + let annotationCleanup = { deletedCommentIds: [] as string[], resolvedThreadIds: [] as string[] }; + const deleted = await svc.tombstoneComment( + commentId, + { + actorType: actor.actorType, + agentId: actor.agentId, + userId: actor.actorType === "user" ? actor.actorId : null, + runId: actor.runId, + }, + { + afterTombstone: async (deletedComment, tx) => { + await issueReferencesSvc.syncComment(deletedComment.id, tx); + annotationCleanup = await documentAnnotationsSvc.cleanupForIssueCommentDeletion(issue.id, deletedComment.id, { + actorType: actor.actorType, + actorId: actor.actorId, + agentId: actor.agentId, + userId: actor.actorType === "user" ? actor.actorId : null, + runId: actor.runId, + }, tx); + await Promise.all( + annotationCleanup.deletedCommentIds.map((annotationCommentId) => + issueReferencesSvc.deleteCommentSource(annotationCommentId, tx) + ), + ); + }, + }, + ); if (!deleted) { res.status(404).json({ error: "Comment not found" }); return; } - await issueReferencesSvc.syncComment(deleted.id); - const annotationCleanup = await documentAnnotationsSvc.cleanupForIssueCommentDeletion(issue.id, deleted.id, { - actorType: actor.actorType, - actorId: actor.actorId, - agentId: actor.agentId, - userId: actor.actorType === "user" ? actor.actorId : null, - runId: actor.runId, - }); - await Promise.all( - annotationCleanup.deletedCommentIds.map((annotationCommentId) => - issueReferencesSvc.deleteCommentSource(annotationCommentId) - ), - ); await logActivity(db, { companyId: issue.companyId, diff --git a/server/src/services/document-annotations.ts b/server/src/services/document-annotations.ts index 3de9cf02..d9fa9bfe 100644 --- a/server/src/services/document-annotations.ts +++ b/server/src/services/document-annotations.ts @@ -330,58 +330,66 @@ export function documentAnnotationService(db: Db) { issueId: string, issueCommentId: string, actor: ActorInput, - ) => db.transaction(async (tx) => { - const linkedComments: Array> = await tx - .select({ - id: documentAnnotationComments.id, - threadId: documentAnnotationComments.threadId, - }) - .from(documentAnnotationComments) - .where(and( - eq(documentAnnotationComments.issueId, issueId), - eq(documentAnnotationComments.issueCommentId, issueCommentId), - )); - if (linkedComments.length === 0) { - return { deletedCommentIds: [], resolvedThreadIds: [] }; - } - - const deletedCommentIds = linkedComments.map((comment) => comment.id); - const threadIds = [...new Set(linkedComments.map((comment) => comment.threadId))]; - const now = new Date(); - - await tx - .delete(documentAnnotationComments) - .where(inArray(documentAnnotationComments.id, deletedCommentIds)); - - const remainingRows: Array<{ threadId: string; count: number }> = await tx - .select({ - threadId: documentAnnotationComments.threadId, - count: sql`count(*)::int`, - }) - .from(documentAnnotationComments) - .where(inArray(documentAnnotationComments.threadId, threadIds)) - .groupBy(documentAnnotationComments.threadId); - const remainingByThreadId = new Map(remainingRows.map((row) => [row.threadId, Number(row.count)])); - const emptyThreadIds = threadIds.filter((threadId) => (remainingByThreadId.get(threadId) ?? 0) === 0); - - if (emptyThreadIds.length > 0) { - await tx - .update(documentAnnotationThreads) - .set({ - status: "resolved", - resolvedByAgentId: actor.actorType === "agent" ? actor.agentId ?? null : null, - resolvedByUserId: actor.actorType === "user" ? actor.userId ?? null : null, - resolvedAt: now, - updatedAt: now, + dbOrTx: any = db, + ) => { + const runCleanup = async (tx: any) => { + const linkedComments: Array> = await tx + .select({ + id: documentAnnotationComments.id, + threadId: documentAnnotationComments.threadId, }) + .from(documentAnnotationComments) .where(and( - inArray(documentAnnotationThreads.id, emptyThreadIds), - eq(documentAnnotationThreads.status, "open"), + eq(documentAnnotationComments.issueId, issueId), + eq(documentAnnotationComments.issueCommentId, issueCommentId), )); - } + if (linkedComments.length === 0) { + return { deletedCommentIds: [], resolvedThreadIds: [] }; + } - return { deletedCommentIds, resolvedThreadIds: emptyThreadIds }; - }), + const deletedCommentIds = linkedComments.map((comment) => comment.id); + const threadIds = [...new Set(linkedComments.map((comment) => comment.threadId))]; + const now = new Date(); + + await tx + .delete(documentAnnotationComments) + .where(inArray(documentAnnotationComments.id, deletedCommentIds)); + + const remainingRows: Array<{ threadId: string; count: number }> = await tx + .select({ + threadId: documentAnnotationComments.threadId, + count: sql`count(*)::int`, + }) + .from(documentAnnotationComments) + .where(inArray(documentAnnotationComments.threadId, threadIds)) + .groupBy(documentAnnotationComments.threadId); + const remainingByThreadId = new Map(remainingRows.map((row) => [row.threadId, Number(row.count)])); + const emptyThreadIds = threadIds.filter((threadId) => (remainingByThreadId.get(threadId) ?? 0) === 0); + + if (emptyThreadIds.length > 0) { + const resolvedRows: Array<{ id: string }> = await tx + .update(documentAnnotationThreads) + .set({ + status: "resolved", + resolvedByAgentId: actor.actorType === "agent" ? actor.agentId ?? null : null, + resolvedByUserId: actor.actorType === "user" ? actor.userId ?? null : null, + resolvedAt: now, + updatedAt: now, + }) + .where(and( + inArray(documentAnnotationThreads.id, emptyThreadIds), + eq(documentAnnotationThreads.status, "open"), + )) + .returning({ id: documentAnnotationThreads.id }); + + return { deletedCommentIds, resolvedThreadIds: resolvedRows.map((row) => row.id) }; + } + + return { deletedCommentIds, resolvedThreadIds: [] }; + }; + + return dbOrTx === db ? db.transaction(runCleanup) : runCleanup(dbOrTx); + }, updateThread: async ( issueId: string, diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 8af71059..6d2a967a 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -34,6 +34,7 @@ import { } from "@paperclipai/db"; import type { AcceptedPlanDecomposition, + IssueComment, IssueCommentAuthorType, IssueCommentMetadata, IssueCommentPresentation, @@ -5733,6 +5734,9 @@ export function issueService(db: Db) { userId?: string | null; runId?: string | null; }, + options?: { + afterTombstone?: (comment: IssueComment, tx: any) => Promise; + }, ) => { const currentUserRedactionOptions = { enabled: (await instanceSettings.getGeneral()).censorUsernameInLogs, @@ -5763,7 +5767,10 @@ export function issueService(db: Db) { .set({ updatedAt: now }) .where(eq(issues.id, comment.issueId)); - return redactIssueComment(comment, currentUserRedactionOptions.enabled); + const redacted = redactIssueComment(comment, currentUserRedactionOptions.enabled); + await options?.afterTombstone?.(redacted, tx); + + return redacted; }); },