diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index df952bf5250..f9f5bb5e0d9 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -1115,6 +1115,7 @@ Core authorization follows these rules: - A user may set inbox-agent policy to `disabled` or `allowlist`. Policy restrictions override the default-open path, and low-trust agents are denied. - An agent targeting any user other than its resolved responsible user requires either a materialized target-user policy that permits that agent (`open` or matching `allowlist`) or an explicit `inbox:manage` grant. The implicit default-open policy for a missing row remains responsible-user-only, so it never becomes a blanket cross-user grant. Grants may be unscoped or constrained by `scope.userIds` and act as administrative overrides, including over a disabled target-user policy. - Archive and unarchive operations are company-scoped, reversible, and activity logged with actor, agent, run, target user, target-resolution source, and policy mode. +- Concurrent archives keep the newest archive time and its actor attribution. An earlier request must not undo an archive written by human completion while it waited. Unarchive removes that state so a later archive starts fresh. - New qualifying issue activity may invalidate an archive so the item resurfaces; archival is not a substitute for resolving or closing work. - Viewing an issue may update its per-user read receipt, but read receipts alone do not enroll the issue in Mine. Mine participation begins with a user-authored comment, issue creation/assignment, or another audited user mutation; explicit product actions such as manually running a routine may record an audited inbox touch. diff --git a/server/src/__tests__/inbox-archive-routes.test.ts b/server/src/__tests__/inbox-archive-routes.test.ts index b33663e8cac..4b53f1f2736 100644 --- a/server/src/__tests__/inbox-archive-routes.test.ts +++ b/server/src/__tests__/inbox-archive-routes.test.ts @@ -20,6 +20,7 @@ import { import { LOW_TRUST_REVIEW_PRESET } from "@paperclipai/shared"; import { errorHandler } from "../middleware/index.js"; import { issueRoutes } from "../routes/issues.js"; +import { issueService } from "../services/issues.js"; import { subscribeCompanyLiveEvents } from "../services/live-events.js"; import { getEmbeddedPostgresTestSupport, @@ -194,6 +195,168 @@ describeEmbeddedPostgres("inbox archive routes", () => { .expect(({ body }) => expect(body).toEqual({ ok: true, userId: seeded.responsibleUserId })); }); + it.each([false, true])("allows manual archive during human completion (existing archive: %s)", async (alreadyArchived) => { + const seeded = await seed(); + const svc = issueService(db); + if (alreadyArchived) { + await svc.archiveInbox(seeded.companyId, seeded.issueId, seeded.responsibleUserId); + } + const app = appFor({ + type: "board", + source: "session", + userId: seeded.responsibleUserId, + companyIds: [seeded.companyId], + memberships: [{ companyId: seeded.companyId, membershipRole: "operator", status: "active" }], + isInstanceAdmin: false, + }); + let manualArchive: Promise<{ status: number }> | undefined; + let completed: Awaited>; + let manualStatus: number | undefined; + try { + completed = await db.transaction(async (tx) => { + await tx.execute(sql`set local statement_timeout = '5s'`); + // Human completion locks the issue before it writes the archive. + await tx.select({ id: issues.id }).from(issues).where(eq(issues.id, seeded.issueId)).for("update"); + const [{ pid }] = await tx.execute<{ pid: number }>(sql`select pg_backend_pid() as pid`); + manualArchive = request(app).post(`/api/issues/${seeded.issueId}/inbox-archive`).send({}).then((response) => response); + await expect.poll(async () => { + const blocked = await db.execute(sql` + select 1 from pg_stat_activity + where datname = current_database() and ${pid} = any(pg_blocking_pids(pid)) + `); + return blocked.length; + }, { timeout: 3_000 }).toBeGreaterThan(0); + + // The manual request is now waiting on our parent row. Previously it + // already owned the archive insert, so this completion formed a cycle. + return svc.update(seeded.issueId, { + status: "done", + actorUserId: seeded.responsibleUserId, + companyGuard: seeded.companyId, + }, tx, []); + }); + } finally { + // Do not leave an in-flight route writing into the next test's fixture. + if (manualArchive) manualStatus = (await manualArchive).status; + } + expect(manualStatus).toBe(200); + expect(completed!.status).toBe("done"); + const archives = await db.select().from(issueInboxArchives); + expect(archives).toHaveLength(1); + expect(archives[0]).toMatchObject({ + companyId: seeded.companyId, + issueId: seeded.issueId, + userId: seeded.responsibleUserId, + archivedByActorType: "user", + archivedByAgentId: null, + archivedByRunId: null, + }); + const audits = await db.select().from(activityLog).where(eq(activityLog.action, "issue.inbox_archived")); + expect(audits).toHaveLength(2); + expect(audits.some((audit) => audit.details?.source === "issue_status_done")).toBe(true); + expect(archives[0].archivedAt.getTime()).toBeGreaterThanOrEqual(completed!.completedAt!.getTime()); + // The status route records this activity after the completion transaction. + await db.insert(activityLog).values({ + companyId: seeded.companyId, + actorType: "user", + actorId: seeded.responsibleUserId, + action: "issue.updated", + entityType: "issue", + entityId: seeded.issueId, + details: { status: "done", _previous: { status: "todo" } }, + }); + await request(app).get(`/api/companies/${seeded.companyId}/issues`) + .query({ touchedByUserId: seeded.responsibleUserId, inboxArchivedByUserId: seeded.responsibleUserId, status: "done" }) + .expect(200) + .expect(({ body }) => expect(body.map((issue: { id: string }) => issue.id)).not.toContain(seeded.issueId)); + }); + + it("keeps caller rollback atomic without blocking a different user's archive", async () => { + const seeded = await seed(); + const svc = issueService(db); + const rollback = new Error("Roll back the first user's archive"); + await expect(db.transaction(async (tx) => { + await svc.archiveInbox(seeded.companyId, seeded.issueId, seeded.responsibleUserId, new Date(), undefined, tx); + // An exclusive parent lock would unnecessarily serialize these users. + await db.transaction(async (otherTx) => { + await otherTx.execute(sql`set local statement_timeout = '3s'`); + await svc.archiveInbox(seeded.companyId, seeded.issueId, seeded.targetUserId, new Date(), undefined, otherTx); + }); + throw rollback; + })).rejects.toBe(rollback); + const archives = await db.select().from(issueInboxArchives); + expect(archives).toHaveLength(1); + expect(archives[0].userId).toBe(seeded.targetUserId); + }); + + it.each([-1, 0, 1])("keeps the newest archive time and matching attribution (offset: %s)", async (offset) => { + const seeded = await seed(); + const svc = issueService(db); + const firstTime = new Date("2000-01-02T00:00:00.000Z"); + const nextTime = new Date(firstTime.getTime() + offset * 1_000); + const first = await svc.archiveInbox(seeded.companyId, seeded.issueId, seeded.responsibleUserId, firstTime, { + archivedByActorType: "agent", + archivedByAgentId: seeded.agentId, + archivedByRunId: seeded.runId, + }); + const next = await svc.archiveInbox(seeded.companyId, seeded.issueId, seeded.responsibleUserId, nextTime); + expect(next.id).toBe(first.id); + expect(next).toMatchObject(offset < 0 ? first : { + archivedAt: nextTime, + archivedByActorType: "user", + archivedByAgentId: null, + archivedByRunId: null, + }); + expect(await db.select().from(issueInboxArchives)).toEqual([next]); + + await svc.unarchiveInbox(seeded.companyId, seeded.issueId, seeded.responsibleUserId); + const afterUndo = await svc.archiveInbox(seeded.companyId, seeded.issueId, seeded.responsibleUserId, nextTime); + expect(afterUndo).toMatchObject({ archivedAt: nextTime, archivedByActorType: "user" }); + expect(afterUndo.id).not.toBe(first.id); + }); + + it("holds the issue company stable until its archive transaction ends", async () => { + const seeded = await seed(); + const otherCompany = await seed(); + const svc = issueService(db); + let move: Promise | undefined; + try { + await db.transaction(async (tx) => { + await svc.archiveInbox(seeded.companyId, seeded.issueId, seeded.responsibleUserId, new Date(), undefined, tx); + const [{ pid }] = await tx.execute<{ pid: number }>(sql`select pg_backend_pid() as pid`); + move = db.update(issues).set({ companyId: otherCompany.companyId }) + .where(eq(issues.id, seeded.issueId)).then((rows) => rows); + await expect.poll(async () => { + const blocked = await db.execute(sql` + select 1 from pg_stat_activity + where datname = current_database() and ${pid} = any(pg_blocking_pids(pid)) + `); + return blocked.length; + }, { timeout: 3_000 }).toBeGreaterThan(0); + const [stillOwned] = await tx.select().from(issues).where(eq(issues.id, seeded.issueId)); + expect(stillOwned.companyId).toBe(seeded.companyId); + }); + } finally { + await move; + } + const [moved] = await db.select().from(issues).where(eq(issues.id, seeded.issueId)); + expect(moved.companyId).toBe(otherCompany.companyId); + }); + + it("rejects missing and cross-company issues before archiving", async () => { + const seeded = await seed(); + const svc = issueService(db); + const otherCompany = await seed(); + for (const [companyId, issueId] of [ + [otherCompany.companyId, seeded.issueId], + [seeded.companyId, randomUUID()], + ]) { + await expect(svc.archiveInbox(companyId, issueId, seeded.responsibleUserId)) + .rejects.toMatchObject({ status: 404, message: "Issue not found" }); + } + expect(await db.select().from(issueInboxArchives)).toHaveLength(0); + }); + it("silently archives an issue for the board user who moves it to done", async () => { const seeded = await seed(); const app = appFor({ diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index f8c862ad91b..a4ef06d713d 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -25,6 +25,7 @@ import { isNull, like, lt, + lte, ne, notExists, notInArray, @@ -7951,35 +7952,64 @@ export function issueService(db: Db) { }, dbOrTx: any = db, ) { - const now = new Date(); - const [row] = await dbOrTx - .insert(issueInboxArchives) - .values({ - companyId, - issueId, - userId, - archivedByActorType: attribution?.archivedByActorType ?? "user", - archivedByAgentId: attribution?.archivedByAgentId ?? null, - archivedByRunId: attribution?.archivedByRunId ?? null, - archivedAt, - updatedAt: now, - }) - .onConflictDoUpdate({ - target: [ - issueInboxArchives.companyId, - issueInboxArchives.issueId, - issueInboxArchives.userId, - ], - set: { - archivedAt, + const runArchive = async (tx: typeof dbOrTx) => { + // Completion locks the issue before archiving. Take the FK's parent lock + // first too, or an insert can hold the archive key while waiting on that + // issue and deadlock with completion's archive UPSERT. SHARE also holds + // companyId stable; different users can still archive concurrently. + const [issue] = await tx + .select({ id: issues.id }) + .from(issues) + .where(and(eq(issues.companyId, companyId), eq(issues.id, issueId))) + .for("share"); + if (!issue) throw notFound("Issue not found"); + + const now = new Date(); + const [row] = await tx + .insert(issueInboxArchives) + .values({ + companyId, + issueId, + userId, archivedByActorType: attribution?.archivedByActorType ?? "user", archivedByAgentId: attribution?.archivedByAgentId ?? null, archivedByRunId: attribution?.archivedByRunId ?? null, + archivedAt, updatedAt: now, - }, - }) - .returning(); - return row; + }) + .onConflictDoUpdate({ + target: [ + issueInboxArchives.companyId, + issueInboxArchives.issueId, + issueInboxArchives.userId, + ], + set: { + archivedAt, + archivedByActorType: attribution?.archivedByActorType ?? "user", + archivedByAgentId: attribution?.archivedByAgentId ?? null, + archivedByRunId: attribution?.archivedByRunId ?? null, + updatedAt: now, + }, + // A request that waited behind completion must not replace the newer + // archive with its earlier request time and resurface the done task. + setWhere: lte(issueInboxArchives.archivedAt, archivedAt), + }) + .returning(); + if (row) return row; + // ON CONFLICT holds this row lock even when setWhere skips the update. + // Return the newer state, including its matching actor attribution. + const [existing] = await tx + .select() + .from(issueInboxArchives) + .where(and( + eq(issueInboxArchives.companyId, companyId), + eq(issueInboxArchives.issueId, issueId), + eq(issueInboxArchives.userId, userId), + )); + if (!existing) throw new Error("Inbox archive conflict row missing"); + return existing; + }; + return dbOrTx === db ? db.transaction(runArchive) : runArchive(dbOrTx); } const service = {