diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index cf22679188..8d130dc57d 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -1031,6 +1031,88 @@ describe.sequential("issue comment reopen routes", () => { )); }); + it("skips the assignee wakeup when the issue is concurrently cancelled while the comment is being written", async () => { + const app = await installActor(createApp()); + // First call is the route's pre-insert fetch; second is the wake-decision + // re-fetch. A stale wake decision would use the first (open, assigned) + // snapshot instead of the second (cancelled) one. + mockIssueService.getById + .mockResolvedValueOnce(makeIssue("in_progress")) + .mockResolvedValueOnce(makeIssue("cancelled")); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-race-cancel", + issueId: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + body: "Still working on this?", + }); + + const res = await request(app) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Still working on this?" }); + + expect(res.status).toBe(201); + expect(mockIssueService.addComment).toHaveBeenCalled(); + // Give any (incorrect) fire-and-forget wakeup a moment to fire before asserting its absence. + await new Promise((resolve) => setTimeout(resolve, 25)); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + + it("wakes the freshly reassigned agent, not the pre-insert snapshot's assignee, when the issue is concurrently reassigned", async () => { + const app = await installActor(createApp()); + const reassignedAgentId = "44444444-4444-4444-8444-444444444444"; + mockIssueService.getById + .mockResolvedValueOnce(makeIssue("in_progress")) + .mockResolvedValueOnce({ ...makeIssue("in_progress"), assigneeAgentId: reassignedAgentId }); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-race-reassign", + issueId: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + body: "Status update", + }); + + const res = await request(app) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Status update" }); + + expect(res.status).toBe(201); + await waitForWakeup(() => expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith( + reassignedAgentId, + expect.objectContaining({ reason: "issue_commented" }), + )); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalledWith( + "22222222-2222-4222-8222-222222222222", + expect.anything(), + ); + }); + + it("keeps the comment write successful and falls back to the in-hand snapshot when the wake re-fetch fails", async () => { + const app = await installActor(createApp()); + // The comment is already committed before the wake-decision re-fetch runs. + // If that best-effort re-fetch throws, the route must still return 201 for + // the persisted comment (a 5xx would invite a retry that duplicates it) and + // fall back to the in-hand snapshot so a legitimate wake isn't dropped. + mockIssueService.getById + .mockResolvedValueOnce(makeIssue("in_progress")) + .mockRejectedValueOnce(new Error("transient read failure")); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-refetch-fail", + issueId: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + body: "Still here?", + }); + + const res = await request(app) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Still here?" }); + + expect(res.status).toBe(201); + expect(mockIssueService.addComment).toHaveBeenCalled(); + await waitForWakeup(() => expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith( + "22222222-2222-4222-8222-222222222222", + expect.objectContaining({ reason: "issue_commented" }), + )); + }); + it("passes validated comment presentation fields to trusted board comment writes", async () => { const app = await installActor(createApp()); mockIssueService.getById.mockResolvedValue(makeIssue("todo")); diff --git a/server/src/__tests__/plugin-orchestration-apis.test.ts b/server/src/__tests__/plugin-orchestration-apis.test.ts index 71f22c181e..b54fe6e31d 100644 --- a/server/src/__tests__/plugin-orchestration-apis.test.ts +++ b/server/src/__tests__/plugin-orchestration-apis.test.ts @@ -903,6 +903,57 @@ describeEmbeddedPostgres("plugin orchestration APIs", () => { expect(run).toMatchObject({ agentId, companyId, status: "queued" }); }); + it("skips the assignee wakeup when the issue is cancelled concurrently with the comment being written", async () => { + const { companyId, agentId } = await seedCompanyAndAgent(); + const humanUserId = randomUUID(); + const issueId = randomUUID(); + await db.insert(companyMemberships).values({ + companyId, + principalType: "user", + principalId: humanUserId, + status: "active", + membershipRole: "owner", + }); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Needs human input", + status: "in_review", + priority: "medium", + assigneeAgentId: agentId, + }); + + const services = buildHostServices(db, "plugin-record-id", "paperclip.gateway", createEventBusStub()); + + // Hold a row lock on the issue so createComment's own `UPDATE issues SET + // updated_at` (inside addComment) blocks until this transaction commits a + // concurrent cancellation. That deterministically reproduces "another + // request closes the issue while the comment is being written" — the + // race Greptile flagged on the pre-insert `issue` snapshot — without + // relying on timing luck for the outcome, only for scheduling. + const lockAndCancelPromise = db.transaction(async (tx) => { + await tx.select().from(issues).where(eq(issues.id, issueId)).for("update"); + await new Promise((resolve) => setTimeout(resolve, 50)); + await tx.update(issues).set({ status: "cancelled" }).where(eq(issues.id, issueId)); + }); + + const commentPromise = services.issues.createComment({ + issueId, + companyId, + body: "Here's my answer", + actorUserId: humanUserId, + }); + + const [comment] = await Promise.all([commentPromise, lockAndCancelPromise]); + + expect(comment).toMatchObject({ + authorType: "user", + authorUserId: humanUserId, + body: "Here's my answer", + }); + await expect(db.select().from(agentWakeupRequests)).resolves.toHaveLength(0); + }); + // --------------------------------------------------------------------------- // LOOA-641 — interactions.respond / approvals.decide impersonation surface. // The host must independently re-verify the paired user's active membership diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 73b6ea87fc..b673573b47 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -12146,13 +12146,27 @@ export function issueRoutes( addWakeup(commentDecisionStageWakeup.agentId, commentDecisionStageWakeup.wakeup); } - const assigneeId = currentIssue.assigneeAgentId; + // Re-fetch immediately before deciding whether to wake anyone: outside + // the reopen/auto-approval branches above, `currentIssue` is still the + // snapshot read before the comment was inserted, so a concurrent + // close/unassign/reassign landing in that window would otherwise wake + // the wrong (or no-longer-relevant) agent off stale state. The comment + // is already committed, so a failed re-fetch is logged and falls back to + // the in-hand snapshot rather than aborting this best-effort wake block. + const wakeIssueSnapshot = (await svc.getById(currentIssue.id).catch((err) => { + logger.warn( + { err, issueId: currentIssue.id }, + "failed to re-fetch issue for comment wake decision; falling back to in-hand snapshot", + ); + return null; + })) ?? currentIssue; + const assigneeId = wakeIssueSnapshot.assigneeAgentId; const actorIsAgent = actor.actorType === "agent"; const selfComment = actorIsAgent && actor.actorId === assigneeId; // Re-derive closed-ness from the post-mutation issue so the auto-approval // transition (in_review -> done) suppresses a stale `issue_commented` wake // to the returnAssignee for an already-completed issue. - const skipWake = selfComment || isClosedIssueStatus(currentIssue.status); + const skipWake = selfComment || isClosedIssueStatus(wakeIssueSnapshot.status); if (assigneeId && (reopened || !skipWake)) { if (reopened) { addWakeup(assigneeId, { diff --git a/server/src/services/plugin-host-services.ts b/server/src/services/plugin-host-services.ts index 78400a43f5..3f975c18ef 100644 --- a/server/src/services/plugin-host-services.ts +++ b/server/src/services/plugin-host-services.ts @@ -2402,36 +2402,55 @@ export function buildHostServices( // handling here, just the core wake. An assignee-less or // closed-status issue is a silent no-op, matching the route's own // guard. - if ( - params.actorUserId - && issue.assigneeAgentId - && issue.status !== "done" - && issue.status !== "cancelled" - ) { - await heartbeat.wakeup(issue.assigneeAgentId, { - source: "automation", - triggerDetail: "system", - reason: "issue_commented", - payload: { + // + // The guard re-fetches the issue instead of trusting the pre-insert + // `issue` snapshot: a concurrent close/unassign/reassign landing + // between the initial fetch and here would otherwise wake the wrong + // (or no-longer-relevant) agent off stale state. + // + // The comment is already committed above, so this best-effort wake + // must never change that outcome: a failed re-fetch is logged and + // falls back to the in-hand snapshot rather than rejecting + // createComment — a rejection would surface to the caller as a failed + // write and invite a retry that inserts a duplicate comment. + if (params.actorUserId) { + const postCommentIssue = (await issues.getById(issue.id).catch((err) => { + logger.warn( + { err, issueId: issue.id, commentId: comment.id }, + "failed to re-fetch issue for plugin-relayed human comment wake; falling back to pre-insert snapshot", + ); + return null; + })) ?? issue; + if ( + postCommentIssue.assigneeAgentId + && postCommentIssue.status !== "done" + && postCommentIssue.status !== "cancelled" + ) { + await heartbeat.wakeup(postCommentIssue.assigneeAgentId, { + source: "automation", + triggerDetail: "system", + reason: "issue_commented", + payload: { + issueId: issue.id, + commentId: comment.id, + mutation: "comment", + }, + requestedByActorType: "user", + requestedByActorId: params.actorUserId, + contextSnapshot: { + issueId: issue.id, + taskId: issue.id, + sourceCommentId: comment.id, + wakeReason: "issue_commented", + source: `plugin:${pluginKey}`, + }, + }).catch((err) => logger.warn({ + err, issueId: issue.id, commentId: comment.id, - mutation: "comment", - }, - requestedByActorType: "user", - requestedByActorId: params.actorUserId, - contextSnapshot: { - issueId: issue.id, - taskId: issue.id, - sourceCommentId: comment.id, - wakeReason: "issue_commented", - source: `plugin:${pluginKey}`, - }, - }).catch((err) => logger.warn({ - err, - issueId: issue.id, - commentId: comment.id, - agentId: issue.assigneeAgentId, - }, "failed to wake assignee on plugin-relayed human comment")); + agentId: postCommentIssue.assigneeAgentId, + }, "failed to wake assignee on plugin-relayed human comment")); + } } return comment;