From b847e8b6f6d2b3fe2d81882c1c8c2050a5293e1f Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:39:23 -0400 Subject: [PATCH] perf(server): reduce issue detail request overhead (#10414) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source control plane people use to coordinate AI-agent work > - Opening an issue fans out into several authenticated issue-detail reads, so repeated work on that path directly affects perceived latency > - Those reads repeated issue and authorization lookups, returned full private JSON even when unchanged, and performed non-critical bookkeeping writes on the request path > - Interaction reads also performed lifecycle writes even though `GET` must be read-only > - This pull request adds request-scoped reuse, private conditional responses, read-only interaction access, and bounded write debouncing without crossing actor, request, or company boundaries > - The result is less database, serialization, logging, and response-body work while preserving authorization and interaction lifecycle invariants ## Linked Issues or Issue Description This is the server-only latency phase. Related work is tracked separately in #10415 (aggregate view), #10416 (warm navigation, merged into the base), and #10463 (bundle split). This pull request intentionally excludes those scopes. **What happened?** Opening an issue detail view caused avoidable server costs: repeated issue and authorization reads within one request, full private JSON responses when a representation was unchanged, writes during interaction-list reads, production debug transport setup, and immediate bookkeeping writes for cloud tenant activity and board-key usage. **Expected behavior** All successful JSON `GET /api/issues/:id/*` responses should support strong private ETags and `304 Not Modified`. Repeated work may be reused only within the current request. `GET /interactions` must not modify stored interactions. Non-critical activity timestamps may be debounced without weakening authentication or stale instance-admin cleanup. **Steps to reproduce** 1. Start Paperclip in local development or self-hosted server mode. 2. Open one issue and request its detail subresources with the same authenticated actor. 3. Repeat a successful JSON request with its `ETag` in `If-None-Match`. 4. Observe `304 Not Modified`, no interaction writes from `GET /interactions`, and unchanged authorization boundaries. **Deployment mode / installation** - Local development or self-hosted server - Built from source - Core server behavior; not adapter-specific ## What Changed - Added strong ETags and `Cache-Control: private, must-revalidate` to successful JSON reads under `/api/issues/:id/*`, including standards-compliant `If-None-Match` handling. - Added request-scoped promise memoization for issue and authorization lookups; no authorization result survives the request. - Made `GET /interactions` read-only, moved supersession and terminal-state handling to mutation paths, and prevented plugin callers from accepting or rejecting interactions after an issue closes. - Removed the production debug-file logger transport while preserving development formatting. - Debounced cloud-tenant activity and board-key `lastUsedAt` persistence, while keeping stale instance-admin deletion unconditional and authentication checks per request. - Added focused tests for ETags, request isolation, authorization lifecycle behavior, interaction invariants, logger configuration, and retry-safe debounce behavior. ## Verification - `pnpm exec vitest run server/src/__tests__/private-json-etag.test.ts server/src/__tests__/issue-thread-interaction-routes.test.ts` — 2 files, 23 tests passed. - Focused Vitest run covering request memoization, authorization, interactions, plugin orchestration, logger, cloud tenant, board auth, and issue services — 9 files, 264 tests passed. - `pnpm --filter @paperclipai/server typecheck` — passed. - `git diff --check origin/master...HEAD` — passed. - Scope guardrails: 21 changed files under `server/src`; no lockfile, workflow, migration, UI, aggregate-view, or bundle-split changes. ## Risks - Strong ETags hash each successful serialized JSON response. This adds a small CPU cost but avoids transferring unchanged bodies. - Debounced bookkeeping timestamps can lag by the bounded debounce interval. They are non-critical usage metadata; authentication still runs per request, and stale instance-admin deletion remains unconditional. - Legacy pending interactions on terminal issues are projected as expired by reads and are finalized only by mutation paths. The stored record remains unchanged on `GET` by design. - No database schema or migration changes are included. > This is a focused performance correction and does not duplicate a planned core feature in `ROADMAP.md`. ## Model Used OpenAI Codex using `gpt-5.3-codex` for the initial implementation and `gpt-5.6-sol` for isolation, verification, and PR preparation, with reasoning, repository tool use, code execution, and GitHub CLI access. The runtimes did not expose authoritative context-window sizes. ## 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 (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [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 - [x] All Paperclip CI gates are green - [x] 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 Co-authored-by: Dev Agent --- .../__tests__/authorization-service.test.ts | 61 ++++++++++ ...ue-interactions-read-only-contract.test.ts | 18 +++ .../issue-thread-interaction-routes.test.ts | 73 +----------- .../issue-thread-interactions-service.test.ts | 37 ++++++ server/src/__tests__/issues-service.test.ts | 106 ++++++++++++++++++ server/src/__tests__/logger-tz.test.ts | 21 +++- .../plugin-orchestration-apis.test.ts | 39 +++++++ .../src/__tests__/private-json-etag.test.ts | 26 +++++ server/src/lib/request-promise-memo.test.ts | 101 +++++++++++++++++ server/src/lib/request-promise-memo.ts | 33 ++++++ server/src/middleware/auth.ts | 63 +++++++++-- .../src/middleware/cloud-tenant-actor.test.ts | 13 +++ server/src/middleware/logger.ts | 39 +------ server/src/middleware/private-json-etag.ts | 40 +++++++ server/src/routes/issues.ts | 103 +++++++++-------- server/src/services/authorization.ts | 33 +----- server/src/services/board-auth.test.ts | 40 +++++++ server/src/services/board-auth.ts | 39 ++++++- .../src/services/issue-thread-interactions.ts | 61 +++++++--- server/src/services/issues.ts | 34 ++++++ server/src/services/plugin-host-services.ts | 10 +- 21 files changed, 774 insertions(+), 216 deletions(-) create mode 100644 server/src/__tests__/issue-interactions-read-only-contract.test.ts create mode 100644 server/src/__tests__/private-json-etag.test.ts create mode 100644 server/src/lib/request-promise-memo.test.ts create mode 100644 server/src/lib/request-promise-memo.ts create mode 100644 server/src/middleware/private-json-etag.ts create mode 100644 server/src/services/board-auth.test.ts diff --git a/server/src/__tests__/authorization-service.test.ts b/server/src/__tests__/authorization-service.test.ts index 821eae7d46..407d1bdf61 100644 --- a/server/src/__tests__/authorization-service.test.ts +++ b/server/src/__tests__/authorization-service.test.ts @@ -1,5 +1,6 @@ import { randomUUID } from "node:crypto"; import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { and, eq } from "drizzle-orm"; import { agents, authUsers, @@ -776,6 +777,66 @@ describeEmbeddedPostgres("authorization service", () => { }); }); + it("rechecks responsible-user membership on a new request actor after revocation", async () => { + const company = await createCompany(db, "ResponsibleUserRevocation"); + const actorAgent = await createAgent(db, company.id, { role: "engineer" }); + const issue = await createIssue(db, company.id, { + title: "Assigned issue mutation after revocation", + assigneeAgentId: actorAgent.id, + }); + const responsibleUserId = await createUser(db); + await db.insert(companyMemberships).values({ + companyId: company.id, + principalType: "user", + principalId: responsibleUserId, + status: "active", + membershipRole: "operator", + }); + const authz = authorizationService(db); + const resource = { + type: "issue" as const, + companyId: company.id, + issueId: issue.id, + assigneeAgentId: actorAgent.id, + }; + + await expect(authz.decide({ + actor: { + type: "agent", + agentId: actorAgent.id, + companyId: company.id, + onBehalfOfUserId: responsibleUserId, + source: "agent_jwt", + }, + action: "issue:mutate", + resource, + })).resolves.toMatchObject({ allowed: true }); + + await db + .update(companyMemberships) + .set({ status: "suspended" }) + .where(and( + eq(companyMemberships.companyId, company.id), + eq(companyMemberships.principalType, "user"), + eq(companyMemberships.principalId, responsibleUserId), + )); + + await expect(authz.decide({ + actor: { + type: "agent", + agentId: actorAgent.id, + companyId: company.id, + onBehalfOfUserId: responsibleUserId, + source: "agent_jwt", + }, + action: "issue:mutate", + resource, + })).resolves.toMatchObject({ + allowed: false, + code: "RESPONSIBLE_USER_UNAVAILABLE", + }); + }); + it("keeps responsible-user issue mutations denied for viewer memberships", async () => { const company = await createCompany(db, "ResponsibleUserIssueViewerDenied"); const actorAgent = await createAgent(db, company.id, { role: "engineer" }); diff --git a/server/src/__tests__/issue-interactions-read-only-contract.test.ts b/server/src/__tests__/issue-interactions-read-only-contract.test.ts new file mode 100644 index 0000000000..720c988d82 --- /dev/null +++ b/server/src/__tests__/issue-interactions-read-only-contract.test.ts @@ -0,0 +1,18 @@ +import fs from "node:fs"; +import { describe, expect, it } from "vitest"; + +describe("issue interactions GET contract", () => { + it("does not perform expiry sweeps or activity writes", () => { + const source = fs.readFileSync(new URL("../routes/issues.ts", import.meta.url), "utf8"); + const start = source.indexOf('router.get("/issues/:id/interactions"'); + const end = source.indexOf('router.post("/issues/:id/interactions"', start); + const handler = source.slice(start, end); + + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + expect(handler).toContain("listForIssue"); + expect(handler).not.toContain("expireRequestConfirmations"); + expect(handler).not.toContain("expirePendingInteractions"); + expect(handler).not.toContain("logActivity"); + }); +}); diff --git a/server/src/__tests__/issue-thread-interaction-routes.test.ts b/server/src/__tests__/issue-thread-interaction-routes.test.ts index ed26e33bee..e984b3d4f4 100644 --- a/server/src/__tests__/issue-thread-interaction-routes.test.ts +++ b/server/src/__tests__/issue-thread-interaction-routes.test.ts @@ -419,50 +419,9 @@ describe.sequential("issue thread interaction routes", () => { mockReviewTransition.value = null; }); - it("lists and creates board-authored interactions", async () => { - mockInteractionService.expireRequestConfirmationsSupersededByHistoricalComments.mockResolvedValueOnce([ - { - id: "interaction-expired", - kind: "ask_user_questions", - status: "expired", - result: { - version: 1, - answers: [], - expirationReason: "superseded_by_comment", - commentId: "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb", - summaryMarkdown: null, - }, - }, - ]); - mockInteractionService.listForIssue.mockResolvedValue([ - { id: "interaction-1", kind: "suggest_tasks", status: "pending" }, - ]); + it("creates board-authored interactions", async () => { const app = await createApp(); - const listRes = await request(app).get("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions"); - expect(listRes.status).toBe(200); - expect(listRes.body).toEqual([ - { id: "interaction-1", kind: "suggest_tasks", status: "pending" }, - ]); - expect(mockInteractionService.expireRequestConfirmationsSupersededByHistoricalComments).toHaveBeenCalledWith( - expect.objectContaining({ id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" }), - ); - expect(mockLogActivity).toHaveBeenCalledWith( - expect.anything(), - expect.objectContaining({ - action: "issue.thread_interaction_expired", - details: expect.objectContaining({ - interactionId: "interaction-expired", - interactionKind: "ask_user_questions", - source: "issue.interactions.catchup_superseded_by_comment", - result: expect.objectContaining({ - expirationReason: "superseded_by_comment", - commentId: "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb", - }), - }), - }), - ); - const createRes = await request(app) .post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions") .send({ @@ -485,43 +444,21 @@ describe.sequential("issue thread interaction routes", () => { }), }), ); - }); + }, 10_000); - it("queues one bounded recovery when historical-comment catch-up expires the final review interactions", async () => { + it("does not run historical-comment catch-up or queue recovery from the interaction read path", async () => { mockIssueService.getById.mockResolvedValue(createIssue({ status: "in_review", assigneeAgentId: ASSIGNEE_AGENT_ID, })); - mockInteractionService.expireRequestConfirmationsSupersededByHistoricalComments.mockResolvedValueOnce([ - { id: "interaction-z", kind: "request_confirmation", status: "expired" }, - { id: "interaction-a", kind: "request_item_verdicts", status: "expired" }, - ]); - mockIssueService.listReviewAttention.mockResolvedValueOnce(new Map([[ - "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", - { state: "stalled", paths: [], reason: "Historical comments consumed the final paths" }, - ]])); mockInteractionService.listForIssue.mockResolvedValue([]); - mockHeartbeatService.wakeup.mockResolvedValueOnce({ id: "catchup-recovery-run" }); await request(await createApp()) .get("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions") .expect(200); - expect(mockHeartbeatService.wakeup).toHaveBeenCalledTimes(1); - expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith(ASSIGNEE_AGENT_ID, expect.objectContaining({ - reason: "issue_review_path_lost", - idempotencyKey: expect.stringMatching( - /^issue_review_path_lost:aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa:/, - ), - payload: expect.objectContaining({ - reviewPathConsumedRef: "interactions:interaction-a,interaction-z", - reviewPathRecoveryAttempt: 1, - }), - contextSnapshot: expect.objectContaining({ - source: "issue.interactions.catchup_superseded_by_comment", - wakeReason: "issue_review_path_lost", - }), - })); + expect(mockInteractionService.expireRequestConfirmationsSupersededByHistoricalComments).not.toHaveBeenCalled(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); }); it("wakes the addressed agent when an interaction is created", async () => { diff --git a/server/src/__tests__/issue-thread-interactions-service.test.ts b/server/src/__tests__/issue-thread-interactions-service.test.ts index 3358935d1d..f01204f5da 100644 --- a/server/src/__tests__/issue-thread-interactions-service.test.ts +++ b/server/src/__tests__/issue-thread-interactions-service.test.ts @@ -2477,6 +2477,43 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { expect(listed[0]?.status).toBe("cancelled"); }); + it("derives legacy pending interactions as expired on closed issues without mutating the GET", async () => { + const { companyId, issueId } = await seedConfirmationIssue("Legacy pending interaction on closed issue"); + const created = await interactionsSvc.create({ id: issueId, companyId }, { + kind: "request_confirmation", + payload: { version: 1, prompt: "Proceed?" }, + }, { userId: "local-board" }); + + await db.update(issues).set({ status: "done" }).where(eq(issues.id, issueId)); + + const listed = await interactionsSvc.listForIssue(issueId); + expect(listed[0]).toMatchObject({ + id: created.id, + status: "expired", + result: { version: 1, outcome: "issue_closed" }, + }); + + const stored = await db + .select({ status: issueThreadInteractions.status }) + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, created.id)) + .then((rows) => rows[0]); + expect(stored?.status).toBe("pending"); + + await expect(interactionsSvc.acceptInteraction({ + id: issueId, + companyId, + projectId: null, + goalId: null, + status: "done", + }, created.id, {}, { userId: "local-board" })).rejects.toThrow( + "Interaction is no longer actionable because the issue is closed", + ); + await expect(interactionsSvc.withdrawInteraction({ id: issueId, companyId, status: "done" }, created.id, {}, { + userId: "local-board", + })).rejects.toThrow("Interaction is no longer actionable because the issue is closed"); + }); + it("does not supersede request confirmations for agent, system, or older user comments", async () => { const { companyId, issueId } = await seedConfirmationIssue("Comment supersede exclusions"); diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index 6c4342ff00..825e0048fd 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -588,6 +588,112 @@ describeEmbeddedPostgres("issueService.list participantAgentId", () => { }); }); + it("expires superseded interactions when human comments are added through the service", async () => { + const companyId = await seedAssignableAgentCompany(); + const issue = await svc.create(companyId, { + title: "Answer with a comment", + description: null, + status: "in_review", + priority: "medium", + }); + const interactionId = randomUUID(); + await db.insert(issueThreadInteractions).values({ + id: interactionId, + companyId, + issueId: issue.id, + kind: "ask_user_questions", + status: "pending", + continuationPolicy: "wake_assignee", + payload: { + version: 1, + supersedeOnUserComment: true, + questions: [{ + id: "scope", + prompt: "Pick one", + selectionMode: "single", + options: [{ id: "a", label: "A" }], + }], + } as never, + }); + + const comment = await svc.addComment(issue.id, "Use option A", { userId: "local-board" }); + + const interaction = await db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, interactionId)) + .then((rows) => rows[0] ?? null); + expect(interaction).toMatchObject({ + status: "expired", + resolvedByUserId: "local-board", + }); + expect(interaction?.result).toMatchObject({ + expirationReason: "superseded_by_comment", + commentId: comment.id, + }); + + const logged = await db + .select() + .from(activityLog) + .where(eq(activityLog.action, "issue.thread_interaction_expired")); + expect(logged).toHaveLength(1); + expect(logged[0]?.details).toMatchObject({ + interactionId, + interactionKind: "ask_user_questions", + interactionStatus: "expired", + }); + }); + + it("keeps interactions pending when the board concierge adds a comment", async () => { + const companyId = await seedAssignableAgentCompany(); + const issue = await svc.create(companyId, { + title: "Concierge reply", + description: null, + status: "in_review", + priority: "medium", + }); + const interactionId = randomUUID(); + await db.insert(issueThreadInteractions).values({ + id: interactionId, + companyId, + issueId: issue.id, + kind: "ask_user_questions", + status: "pending", + continuationPolicy: "wake_assignee", + payload: { + version: 1, + supersedeOnUserComment: true, + questions: [{ + id: "scope", + prompt: "Pick one", + selectionMode: "single", + options: [{ id: "a", label: "A" }], + }], + } as never, + }); + + await svc.addComment(issue.id, "Automated concierge reply", { + userId: "board-concierge", + }); + + const interaction = await db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, interactionId)) + .then((rows) => rows[0] ?? null); + expect(interaction).toMatchObject({ + status: "pending", + resolvedByUserId: null, + result: null, + }); + + const logged = await db + .select() + .from(activityLog) + .where(eq(activityLog.action, "issue.thread_interaction_expired")); + expect(logged).toHaveLength(0); + }); + it("rejects moving an existing terminated assignment into progress without clearing it", async () => { const companyId = await seedAssignableAgentCompany(); const assigneeAgentId = randomUUID(); diff --git a/server/src/__tests__/logger-tz.test.ts b/server/src/__tests__/logger-tz.test.ts index 60340cd71d..9e81d9b322 100644 --- a/server/src/__tests__/logger-tz.test.ts +++ b/server/src/__tests__/logger-tz.test.ts @@ -50,19 +50,30 @@ vi.mock("../home-paths.js", () => ({ describe("logger translateTime respects TZ environment variable", () => { beforeEach(() => { + vi.resetModules(); + vi.unstubAllEnvs(); vi.clearAllMocks(); }); it("configures pino-pretty with SYS:HH:MM:ss so timestamps honour the TZ env var", async () => { + vi.stubEnv("NODE_ENV", "development"); await import("../middleware/logger.js"); expect(mockTransport).toHaveBeenCalledOnce(); - const { targets } = mockTransport.mock.calls[0][0] as { - targets: Array<{ options: Record }>; + const transport = mockTransport.mock.calls[0][0] as { + target: string; + options: Record; }; - for (const target of targets) { - expect(target.options.translateTime).toBe("SYS:HH:MM:ss"); - } + expect(transport.target).toBe("pino-pretty"); + expect(transport.options.translateTime).toBe("SYS:HH:MM:ss"); + }); + + it("does not construct a pretty transport in production", async () => { + vi.stubEnv("NODE_ENV", "production"); + await import("../middleware/logger.js"); + + expect(mockTransport).not.toHaveBeenCalled(); + expect(mockPino).toHaveBeenCalledWith(expect.objectContaining({ level: "info" })); }); it("SYS: prefix produces timezone-sensitive output: UTC epoch formats differently under UTC vs UTC+8", () => { diff --git a/server/src/__tests__/plugin-orchestration-apis.test.ts b/server/src/__tests__/plugin-orchestration-apis.test.ts index e7226267b1..9409ac0284 100644 --- a/server/src/__tests__/plugin-orchestration-apis.test.ts +++ b/server/src/__tests__/plugin-orchestration-apis.test.ts @@ -1001,6 +1001,45 @@ describeEmbeddedPostgres("plugin orchestration APIs", () => { expect(row?.status).toBe("accepted"); }); + it.each(["accept", "reject"] as const)( + "respondInteraction rejects %s after the issue closes", + async (action) => { + const { companyId } = await seedCompanyAndAgent(); + const operatorUserId = randomUUID(); + await db.insert(companyMemberships).values({ + companyId, + principalType: "user", + principalId: operatorUserId, + status: "active", + membershipRole: "operator", + }); + const issueId = randomUUID(); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Closed decision", + status: "done", + priority: "medium", + }); + const interactionId = await seedInteraction(companyId, issueId); + const services = buildHostServices(db, "plugin-record-id", "paperclip.gateway", createEventBusStub()); + + await expect(services.issues.respondInteraction({ + issueId, + interactionId, + companyId, + action, + actorUserId: operatorUserId, + })).rejects.toThrow("Interaction is no longer actionable because the issue is closed"); + + const [row] = await db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, interactionId)); + expect(row?.status).toBe("pending"); + }, + ); + it("respondInteraction converges (applied:false) when the interaction is already resolved", async () => { const { companyId, agentId } = await seedCompanyAndAgent(); const humanUserId = randomUUID(); diff --git a/server/src/__tests__/private-json-etag.test.ts b/server/src/__tests__/private-json-etag.test.ts new file mode 100644 index 0000000000..3d55c4a917 --- /dev/null +++ b/server/src/__tests__/private-json-etag.test.ts @@ -0,0 +1,26 @@ +import express from "express"; +import request from "supertest"; +import { describe, expect, it } from "vitest"; +import { privateJsonEtag } from "../middleware/private-json-etag.js"; + +describe("privateJsonEtag", () => { + it("returns 304 for unchanged private JSON and changes when the payload changes", async () => { + let revision = 1; + const app = express(); + app.get("/issue", privateJsonEtag(), (_req, res) => res.json({ revision })); + + const first = await request(app).get("/issue"); + expect(first.status).toBe(200); + expect(first.headers["cache-control"]).toBe("private, must-revalidate"); + expect(first.headers.etag).toBeTruthy(); + + const unchanged = await request(app).get("/issue").set("If-None-Match", first.headers.etag); + expect(unchanged.status).toBe(304); + expect(unchanged.text).toBe(""); + + revision = 2; + const changed = await request(app).get("/issue").set("If-None-Match", first.headers.etag); + expect(changed.status).toBe(200); + expect(changed.headers.etag).not.toBe(first.headers.etag); + }); +}); diff --git a/server/src/lib/request-promise-memo.test.ts b/server/src/lib/request-promise-memo.test.ts new file mode 100644 index 0000000000..bd6c6756f5 --- /dev/null +++ b/server/src/lib/request-promise-memo.test.ts @@ -0,0 +1,101 @@ +import { describe, expect, it, vi } from "vitest"; +import { createRequestPromiseMemo } from "./request-promise-memo.js"; + +type TestRequest = { + actor: { + userId: string | null; + companyIds: string[]; + memberships: Array<{ companyId: string; membershipRole: string; status: string }>; + isInstanceAdmin: boolean; + keyScope: string | null; + responsibleUserId: string | null; + }; +}; + +describe("createRequestPromiseMemo", () => { + it("deduplicates repeated loads only within the same request", async () => { + const memoize = createRequestPromiseMemo(); + const load = vi.fn(async () => "allowed"); + const request = requestFor(); + + await Promise.all([ + memoize(request, "issue-1", load), + memoize(request, "issue-1", load), + ]); + + expect(load).toHaveBeenCalledTimes(1); + }); + + it.each([ + ["actor", { userId: "user-2" }], + ["company", { companyIds: ["company-2"] }], + ["membership role", { memberships: [{ companyId: "company-1", membershipRole: "member", status: "active" }] }], + ["membership status", { memberships: [{ companyId: "company-1", membershipRole: "owner", status: "suspended" }] }], + ["instance admin", { isInstanceAdmin: true }], + ["agent key scope", { keyScope: "read" }], + ["responsible user", { responsibleUserId: "user-2" }], + ])("never reuses a decision across requests after a %s change", async (_label, actorPatch) => { + const memoize = createRequestPromiseMemo(); + const load = vi.fn(async () => "allowed"); + const firstRequest = requestFor(); + const secondRequest = requestFor(actorPatch); + + await memoize(firstRequest, "issue-1", load); + await memoize(secondRequest, "issue-1", load); + + expect(load).toHaveBeenCalledTimes(2); + }); + + it("retries a rejected load within the same request", async () => { + const memoize = createRequestPromiseMemo(); + const load = vi.fn() + .mockRejectedValueOnce(new Error("transient")) + .mockResolvedValueOnce("allowed"); + const request = requestFor(); + + await expect(memoize(request, "issue-1", load)).rejects.toThrow("transient"); + await expect(memoize(request, "issue-1", load)).resolves.toBe("allowed"); + expect(load).toHaveBeenCalledTimes(2); + }); + + it("does not cache resolved values rejected by the cache policy", async () => { + const memoize = createRequestPromiseMemo({ + shouldCache: (value) => value !== null, + }); + const load = vi.fn(async () => null); + const request = requestFor(); + + await expect(memoize(request, "missing-issue", load)).resolves.toBeNull(); + await expect(memoize(request, "missing-issue", load)).resolves.toBeNull(); + + expect(load).toHaveBeenCalledTimes(2); + }); + + it("does not reuse loads across many unique requests and keys", async () => { + const memoize = createRequestPromiseMemo({ + shouldCache: (value) => value !== null, + }); + const load = vi.fn(async () => null); + const requestCount = 1_000; + + await Promise.all(Array.from({ length: requestCount }, (_, index) => ( + memoize(requestFor(), `missing-issue-${index}`, load) + ))); + + expect(load).toHaveBeenCalledTimes(requestCount); + }); +}); + +function requestFor(actorPatch: Partial = {}): TestRequest { + return { + actor: { + userId: "user-1", + companyIds: ["company-1"], + memberships: [{ companyId: "company-1", membershipRole: "owner", status: "active" }], + isInstanceAdmin: false, + keyScope: null, + responsibleUserId: "user-1", + ...actorPatch, + }, + }; +} diff --git a/server/src/lib/request-promise-memo.ts b/server/src/lib/request-promise-memo.ts new file mode 100644 index 0000000000..3d07604d25 --- /dev/null +++ b/server/src/lib/request-promise-memo.ts @@ -0,0 +1,33 @@ +type RequestPromiseMemoOptions = { + shouldCache?: (value: TValue) => boolean; +}; + +export function createRequestPromiseMemo( + options: RequestPromiseMemoOptions = {}, +) { + const requests = new WeakMap>>(); + const shouldCache = options.shouldCache ?? (() => true); + + return function memoize(request: TRequest, key: string, load: () => Promise) { + let memo = requests.get(request); + if (!memo) { + memo = new Map(); + requests.set(request, memo); + } + + const cached = memo.get(key); + if (cached) return cached; + + const value = load(); + memo.set(key, value); + void value.then( + (resolved) => { + if (!shouldCache(resolved) && memo.get(key) === value) memo.delete(key); + }, + () => { + if (memo.get(key) === value) memo.delete(key); + }, + ); + return value; + }; +} diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index b8a0ee9b75..13c928961b 100644 --- a/server/src/middleware/auth.ts +++ b/server/src/middleware/auth.ts @@ -17,6 +17,33 @@ import { isUuidLike, normalizeAgentApiKeyScope, type DeploymentMode } from "@pap import type { BetterAuthSessionResult } from "../auth/better-auth.js"; import { logger } from "./logger.js"; import { boardAuthService } from "../services/board-auth.js"; + +const CLOUD_TENANT_WRITE_DEBOUNCE_MS = 5_000; +const CLOUD_TENANT_WRITE_DEBOUNCE_MAX = 1_000; +const cloudTenantWriteDebounces = new WeakMap>(); + +function cloudTenantWriteDebounceFor(db: Db) { + let debounce = cloudTenantWriteDebounces.get(db); + if (!debounce) { + debounce = new Map(); + cloudTenantWriteDebounces.set(db, debounce); + } + return debounce; +} + +function pruneCloudTenantWriteDebounce( + debounce: Map, + nowMs: number, +) { + for (const [subject, entry] of debounce) { + if (entry.syncedAt <= nowMs - CLOUD_TENANT_WRITE_DEBOUNCE_MS) debounce.delete(subject); + } + while (debounce.size > CLOUD_TENANT_WRITE_DEBOUNCE_MAX) { + const oldestSubject = debounce.keys().next().value; + if (!oldestSubject) break; + debounce.delete(oldestSubject); + } +} import { instanceSettingsService } from "../services/instance-settings.js"; import { ensureHumanRoleDefaultGrants } from "../services/principal-access-compatibility.js"; import { forbidden, unprocessable } from "../errors.js"; @@ -431,8 +458,20 @@ export async function resolveCloudTenantActor(db: Db, req: Request): Promise { expect(insertedTables).toContain(companyMemberships); }); + it("resyncs an A to B to A context transition inside the debounce window", async () => { + const { db, insertedTables } = createFakeDb(); + const contextA = VALID_HEADERS; + const contextB = { ...VALID_HEADERS, "x-paperclip-cloud-stack-role": "member" }; + + await resolveCloudTenantActor(db, fakeReq(contextA)); + await resolveCloudTenantActor(db, fakeReq(contextB)); + await resolveCloudTenantActor(db, fakeReq(contextA)); + + expect(insertedTables.filter((table) => table === authUsers)).toHaveLength(3); + expect(insertedTables.filter((table) => table === companyMemberships)).toHaveLength(3); + }); + it("returns null when the server token is unset", async () => { delete process.env.PAPERCLIP_CLOUD_TENANT_SERVER_TOKEN; const { db } = createFakeDb(); diff --git a/server/src/middleware/logger.ts b/server/src/middleware/logger.ts index 2ce76d7250..d46f9adff9 100644 --- a/server/src/middleware/logger.ts +++ b/server/src/middleware/logger.ts @@ -1,51 +1,22 @@ -import path from "node:path"; -import fs from "node:fs"; import pino from "pino"; import { pinoHttp } from "pino-http"; -import { readConfigFile } from "../config-file.js"; -import { resolveDefaultLogsDir, resolveHomeAwarePath } from "../home-paths.js"; import { HTTP_LOG_REDACT_PATHS } from "./http-log-redaction.js"; import { shouldSilenceHttpSuccessLog } from "./http-log-policy.js"; import { redactSensitive } from "./redact-sensitive.js"; -function resolveServerLogDir(): string { - const envOverride = process.env.PAPERCLIP_LOG_DIR?.trim(); - if (envOverride) return resolveHomeAwarePath(envOverride); - - const fileLogDir = readConfigFile()?.logging.logDir?.trim(); - if (fileLogDir) return resolveHomeAwarePath(fileLogDir); - - return resolveDefaultLogsDir(); -} - -const logDir = resolveServerLogDir(); -fs.mkdirSync(logDir, { recursive: true }); - -const logFile = path.join(logDir, "server.log"); - const sharedOpts = { translateTime: "SYS:HH:MM:ss", ignore: "pid,hostname", singleLine: true, }; -export const logger = pino({ - level: "debug", - redact: [...HTTP_LOG_REDACT_PATHS], -}, pino.transport({ - targets: [ - { +const isProduction = process.env.NODE_ENV === "production"; +export const logger = isProduction + ? pino({ level: process.env.PAPERCLIP_LOG_LEVEL?.trim() || "info", redact: [...HTTP_LOG_REDACT_PATHS] }) + : pino({ level: process.env.PAPERCLIP_LOG_LEVEL?.trim() || "debug", redact: [...HTTP_LOG_REDACT_PATHS] }, pino.transport({ target: "pino-pretty", options: { ...sharedOpts, ignore: "pid,hostname,req,res,responseTime", colorize: true, destination: 1 }, - level: "info", - }, - { - target: "pino-pretty", - options: { ...sharedOpts, colorize: false, destination: logFile, mkdir: true }, - level: "debug", - }, - ], -})); + })); export const httpLogger = pinoHttp({ logger, diff --git a/server/src/middleware/private-json-etag.ts b/server/src/middleware/private-json-etag.ts new file mode 100644 index 0000000000..ac89074b50 --- /dev/null +++ b/server/src/middleware/private-json-etag.ts @@ -0,0 +1,40 @@ +import { createHash } from "node:crypto"; +import type { RequestHandler } from "express"; + +function matchesEtag(header: string | undefined, etag: string) { + if (!header) return false; + return header.split(",").some((candidate) => { + const normalized = candidate.trim(); + return normalized === "*" || normalized === etag || normalized === `W/${etag}`; + }); +} + +export function privateJsonEtag(): RequestHandler { + return (req, res, next) => { + if (req.method !== "GET") { + next(); + return; + } + + const originalSend = res.send.bind(res); + res.send = ((body: unknown) => { + const contentType = res.getHeader("Content-Type"); + if ( + res.statusCode < 200 + || res.statusCode >= 300 + || typeof contentType !== "string" + || !contentType.includes("application/json") + ) return originalSend(body); + const serialized = typeof body === "string" ? body : JSON.stringify(body); + const etag = `"${createHash("sha256").update(serialized).digest("base64url")}"`; + res.setHeader("Cache-Control", "private, must-revalidate"); + res.setHeader("ETag", etag); + if (matchesEtag(req.header("if-none-match"), etag)) { + res.status(304).end(); + return res; + } + return originalSend(body); + }) as typeof res.send; + next(); + }; +} diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index ac605bd6a5..a7c9d23d8f 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -152,6 +152,8 @@ import { import type { TaskWatchdogServiceDeps, taskWatchdogService } from "../services/task-watchdogs.js"; import { logger } from "../middleware/logger.js"; import { badRequest, conflict, forbidden, HttpError, notFound, unauthorized, unprocessable } from "../errors.js"; +import { privateJsonEtag } from "../middleware/private-json-etag.js"; +import { createRequestPromiseMemo } from "../lib/request-promise-memo.js"; import { assertBoard, assertCompanyAccess, getAccessibleResource, getActorInfo } from "./authz.js"; import { assertNoAgentHostWorkspaceCommandMutation, @@ -2743,6 +2745,25 @@ export function issueRoutes( const decisionTrainingSvc = decisionTrainingService(db); const issueReferencesSvc = issueReferenceService(db); const issueThreadInteractionsSvc = issueThreadInteractionService(db); + const memoizeIssueRead = createRequestPromiseMemo>>({ + shouldCache: (issue) => issue !== null, + }); + const memoizeIssueReadDecision = createRequestPromiseMemo>>(); + + function getIssueById(req: Request, id: string) { + if (req.method !== "GET") return svc.getById(id); + return memoizeIssueRead(req, id, () => svc.getById(id)); + } + + const issueDetailEtag = privateJsonEtag(); + router.use((req, res, next) => { + if (/^\/issues\/[^/]+(?:\/|$)/.test(req.path)) { + issueDetailEtag(req, res, next); + return; + } + next(); + }); + const taskWatchdogFactory: TaskWatchdogServiceFactory | undefined = Object.prototype.hasOwnProperty.call( serviceIndex, "taskWatchdogService", @@ -3778,7 +3799,9 @@ export function issueRoutes( } async function assertIssueReadAllowed(req: Request, res: Response, issue: Parameters[1]) { - const decision = await decideIssueAccess(req, issue, "issue:read"); + const key = `${issue.id}:${issue.companyId}:${issue.projectId ?? ""}:${issue.parentId ?? ""}:${issue.assigneeAgentId ?? ""}:${issue.assigneeUserId ?? ""}:${issue.status}`; + const value = memoizeIssueReadDecision(req, key, () => decideIssueAccess(req, issue, "issue:read")); + const decision = await value; if (decision.allowed) return true; res.status(403).json({ error: "Issue is outside this actor's authorization boundary" }); return false; @@ -5724,7 +5747,7 @@ export function issueRoutes( router.get("/issues/:id/heartbeat-context", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; @@ -5878,7 +5901,7 @@ export function issueRoutes( router.get("/issues/:id/diagnostics/blockers", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; @@ -5909,7 +5932,7 @@ export function issueRoutes( router.get("/issues/:id/diagnostics/wakes", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; @@ -5954,7 +5977,7 @@ export function issueRoutes( router.get("/issues/:id/diagnostics/subtree", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; @@ -6004,7 +6027,7 @@ export function issueRoutes( router.get("/issues/:id", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const inboxArchiveFieldsPromise = req.actor.type === "board" && req.actor.userId @@ -6090,7 +6113,7 @@ export function issueRoutes( router.get("/issues/:id/watchdog", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; res.json(await taskWatchdogsSvc.getActiveForIssue(issue.companyId, issue.id)); @@ -6098,7 +6121,7 @@ export function issueRoutes( router.put("/issues/:id/watchdog", validate(upsertIssueWatchdogSchema), async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; @@ -6139,7 +6162,7 @@ export function issueRoutes( router.delete("/issues/:id/watchdog", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; @@ -6176,7 +6199,7 @@ export function issueRoutes( router.get("/issues/:id/recovery-actions", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const active = await revalidateActiveSourceRecoveryForRead({ @@ -6377,7 +6400,7 @@ export function issueRoutes( router.get("/issues/:id/work-products", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const workProducts = await workProductsSvc.listForIssue(issue.id); @@ -6386,7 +6409,7 @@ export function issueRoutes( router.get("/issues/:id/external-objects", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const objects = await externalObjectsSvc.listForIssue(issue.id); @@ -6395,7 +6418,7 @@ export function issueRoutes( router.get("/issues/:id/external-object-summary", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const summary = await externalObjectsSvc.getIssueSummary(issue.id); @@ -6456,7 +6479,7 @@ export function issueRoutes( router.get("/issues/:id/documents", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const docs = await documentsSvc.listIssueDocuments(issue.id, { @@ -6467,7 +6490,7 @@ export function issueRoutes( router.get("/issues/:id/documents/:key", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const keyParsed = issueDocumentKeySchema.safeParse(String(req.params.key ?? "").trim().toLowerCase()); @@ -6493,7 +6516,7 @@ export function issueRoutes( router.get("/issues/:id/documents/:key/annotations", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const keyParsed = issueDocumentKeySchema.safeParse(String(req.params.key ?? "").trim().toLowerCase()); @@ -6563,7 +6586,7 @@ export function issueRoutes( router.get("/issues/:id/documents/:key/annotations/:threadId", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const keyParsed = issueDocumentKeySchema.safeParse(String(req.params.key ?? "").trim().toLowerCase()); @@ -6902,7 +6925,7 @@ export function issueRoutes( router.get("/issues/:id/documents/:key/revisions", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const keyParsed = issueDocumentKeySchema.safeParse(String(req.params.key ?? "").trim().toLowerCase()); @@ -7565,7 +7588,7 @@ export function issueRoutes( router.get("/issues/:id/approvals", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (await assertLowTrustControlPlaneDenied(req, res, issue.companyId, issue)) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; @@ -8145,7 +8168,7 @@ export function issueRoutes( router.get("/issues/:id/accepted-plan-decompositions", async (req, res) => { const sourceIssueId = req.params.id as string; - const sourceIssue = await getAccessibleResource(req, res, svc.getById(sourceIssueId), "Issue not found"); + const sourceIssue = await getAccessibleResource(req, res, getIssueById(req, sourceIssueId), "Issue not found"); if (!sourceIssue) return; const decompositions = await svc.listAcceptedPlanDecompositions(sourceIssue.id); res.json(decompositions); @@ -10219,7 +10242,7 @@ export function issueRoutes( router.get("/issues/:id/comments", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const afterCommentId = @@ -10250,36 +10273,10 @@ export function issueRoutes( router.get("/issues/:id/interactions", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; - const actor = getActorInfo(req); - const interactionSvc = issueThreadInteractionService(db); - const supersededInteractions = await interactionSvc.expireRequestConfirmationsSupersededByHistoricalComments(issue); - await logExpiredRequestConfirmations({ - issue, - interactions: supersededInteractions, - actor, - source: "issue.interactions.catchup_superseded_by_comment", - }); - await queueExpiredInteractionReviewPathRecovery({ - issue, - interactions: supersededInteractions, - actor, - source: "issue.interactions.catchup_superseded_by_comment", - }); - const closedIssueInteractions = await interactionSvc.expirePendingInteractionsForTerminalIssue(issue, { - agentId: actor.agentId, - userId: actor.actorType === "user" ? actor.actorId : null, - }); - await logExpiredRequestConfirmations({ - issue, - interactions: closedIssueInteractions, - actor, - source: "issue.interactions.catchup_issue_closed", - }); - - const interactions = await interactionSvc.listForIssue(id); + const interactions = await issueThreadInteractionService(db).listForIssue(id); res.json(interactions); }); @@ -10841,7 +10838,7 @@ export function issueRoutes( router.get("/issues/:id/comments/:commentId", async (req, res) => { const id = req.params.id as string; const commentId = req.params.commentId as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const comment = await svc.getComment(commentId); @@ -11002,7 +10999,7 @@ export function issueRoutes( router.get("/issues/:id/feedback-votes", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (req.actor.type !== "board") { res.status(403).json({ error: "Only board users can view feedback votes" }); @@ -11015,7 +11012,7 @@ export function issueRoutes( router.get("/issues/:id/feedback-traces", async (req, res) => { const id = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, id), "Issue not found"); if (!issue) return; if (req.actor.type !== "board") { res.status(403).json({ error: "Only board users can view feedback traces" }); @@ -11870,7 +11867,7 @@ export function issueRoutes( router.get("/issues/:id/attachments", async (req, res) => { const issueId = req.params.id as string; - const issue = await getAccessibleResource(req, res, svc.getById(issueId), "Issue not found"); + const issue = await getAccessibleResource(req, res, getIssueById(req, issueId), "Issue not found"); if (!issue) return; if (!(await assertIssueReadAllowed(req, res, issue))) return; const attachments = await svc.listAttachments(issueId); diff --git a/server/src/services/authorization.ts b/server/src/services/authorization.ts index 807cde2f47..fe461488df 100644 --- a/server/src/services/authorization.ts +++ b/server/src/services/authorization.ts @@ -468,18 +468,6 @@ type ResponsibleUserActorWithMemo = AuthorizationActor & { __responsibleUserSnapshotMemo?: Map>; }; -const responsibleUserSnapshotCache = new Map< - string, - { expiresAt: number; promise: Promise } ->(); - -function responsibleUserSnapshotTtlMs() { - const raw = process.env.PAPERCLIP_RESPONSIBLE_USER_AUTHZ_CACHE_TTL_MS?.trim(); - if (!raw) return 5_000; - const parsed = Number(raw); - return Number.isFinite(parsed) && parsed >= 0 ? parsed : 5_000; -} - export function responsibleUserAuthzShadowMode() { const mode = process.env.PAPERCLIP_RESPONSIBLE_USER_AUTHZ_MODE?.trim().toLowerCase(); const shadow = process.env.PAPERCLIP_RESPONSIBLE_USER_AUTHZ_SHADOW?.trim().toLowerCase(); @@ -627,24 +615,13 @@ export function authorizationService(db: Db) { return promise; } - const now = Date.now(); - const cached = responsibleUserSnapshotCache.get(key); - if (cached && cached.expiresAt > now) { - actorWithMemo.__responsibleUserSnapshotMemo.set(key, cached.promise); - return cached.promise; - } - - const ttlMs = responsibleUserSnapshotTtlMs(); const promise = loadResponsibleUserSnapshot(input.companyId, input.userId); - if (ttlMs > 0) { - responsibleUserSnapshotCache.set(key, { expiresAt: now + ttlMs, promise }); - promise.catch(() => { - if (responsibleUserSnapshotCache.get(key)?.promise === promise) { - responsibleUserSnapshotCache.delete(key); - } - }); - } actorWithMemo.__responsibleUserSnapshotMemo.set(key, promise); + void promise.catch(() => { + if (actorWithMemo.__responsibleUserSnapshotMemo?.get(key) === promise) { + actorWithMemo.__responsibleUserSnapshotMemo.delete(key); + } + }); return promise; } diff --git a/server/src/services/board-auth.test.ts b/server/src/services/board-auth.test.ts new file mode 100644 index 0000000000..f5849e57bb --- /dev/null +++ b/server/src/services/board-auth.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it, vi } from "vitest"; +import type { Db } from "@paperclipai/db"; +import { boardAuthService } from "./board-auth.js"; + +describe("boardAuthService touchBoardApiKey", () => { + it("retries the audit write after a transient failure", async () => { + const writes = [Promise.reject(new Error("transient")), Promise.resolve([])]; + const update = vi.fn(() => ({ + set: () => ({ + where: () => writes.shift(), + }), + })); + const service = boardAuthService({ update } as unknown as Db); + + await expect(service.touchBoardApiKey("key-1")).rejects.toThrow("transient"); + await expect(service.touchBoardApiKey("key-1")).resolves.toBeUndefined(); + + expect(update).toHaveBeenCalledTimes(2); + }); + + it("shares one in-flight audit write across concurrent touches", async () => { + let releaseWrite: (() => void) | undefined; + const write = new Promise((resolve) => { + releaseWrite = resolve; + }); + const update = vi.fn(() => ({ + set: () => ({ + where: () => write, + }), + })); + const service = boardAuthService({ update } as unknown as Db); + + const first = service.touchBoardApiKey("key-1"); + const second = service.touchBoardApiKey("key-1"); + releaseWrite?.(); + await Promise.all([first, second]); + + expect(update).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/src/services/board-auth.ts b/server/src/services/board-auth.ts index 35ed3eef80..7478ae1af8 100644 --- a/server/src/services/board-auth.ts +++ b/server/src/services/board-auth.ts @@ -13,6 +13,8 @@ import { conflict, forbidden, notFound } from "../errors.js"; export const BOARD_API_KEY_TTL_MS = 30 * 24 * 60 * 60 * 1000; export const CLI_AUTH_CHALLENGE_TTL_MS = 10 * 60 * 1000; +const BOARD_API_KEY_TOUCH_DEBOUNCE_MS = 60_000; +const BOARD_API_KEY_TOUCH_CACHE_MAX = 1_000; export type CliAuthChallengeStatus = "pending" | "approved" | "cancelled" | "expired"; @@ -50,6 +52,20 @@ function challengeStatusForRow(row: typeof cliAuthChallenges.$inferSelect): CliA } export function boardAuthService(db: Db) { + const touchedBoardApiKeys = new Map | null }>(); + + function pruneTouchedBoardApiKeys(nowMs: number) { + for (const [id, entry] of touchedBoardApiKeys) { + if (!entry.inFlight && entry.completedAt !== null && entry.completedAt <= nowMs - BOARD_API_KEY_TOUCH_DEBOUNCE_MS) { + touchedBoardApiKeys.delete(id); + } + } + while (touchedBoardApiKeys.size > BOARD_API_KEY_TOUCH_CACHE_MAX) { + const oldestId = touchedBoardApiKeys.keys().next().value; + if (!oldestId) break; + touchedBoardApiKeys.delete(oldestId); + } + } async function resolveBoardAccess(userId: string) { const [user, memberships, adminRole] = await Promise.all([ db @@ -147,7 +163,28 @@ export function boardAuthService(db: Db) { } async function touchBoardApiKey(id: string) { - await db.update(boardApiKeys).set({ lastUsedAt: new Date() }).where(eq(boardApiKeys.id, id)); + const nowMs = Date.now(); + pruneTouchedBoardApiKeys(nowMs); + const cached = touchedBoardApiKeys.get(id); + if (cached?.inFlight) return cached.inFlight; + if (cached?.completedAt !== null && cached?.completedAt !== undefined + && cached.completedAt > nowMs - BOARD_API_KEY_TOUCH_DEBOUNCE_MS) return; + + const inFlight = db + .update(boardApiKeys) + .set({ lastUsedAt: new Date() }) + .where(eq(boardApiKeys.id, id)) + .then(() => { + touchedBoardApiKeys.delete(id); + touchedBoardApiKeys.set(id, { completedAt: Date.now(), inFlight: null }); + pruneTouchedBoardApiKeys(Date.now()); + }) + .catch((error) => { + if (touchedBoardApiKeys.get(id)?.inFlight === inFlight) touchedBoardApiKeys.delete(id); + throw error; + }); + touchedBoardApiKeys.set(id, { completedAt: null, inFlight }); + return inFlight; } async function revokeBoardApiKey(id: string) { diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index 57171354a9..11ffee195e 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -1285,7 +1285,7 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti } async function getPendingInteractionForResolution(args: { - issue: { id: string; companyId: string }; + issue: { id: string; companyId: string; status?: string }; interactionId: string; }) { const current = await db @@ -1298,12 +1298,21 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti if (current.companyId !== args.issue.companyId || current.issueId !== args.issue.id) { throw notFound("Interaction not found"); } + if (args.issue.status && isTerminalIssueStatus(args.issue.status)) { + throw conflict("Interaction is no longer actionable because the issue is closed"); + } if (current.status !== "pending") { throw conflict("Interaction has already been resolved"); } return current; } + function assertIssueOpenForInteractionResolution(issue: { id: string; companyId: string; status?: string }) { + if (issue.status && isTerminalIssueStatus(issue.status)) { + throw conflict("Interaction is no longer actionable because the issue is closed"); + } + } + async function acceptRequestConfirmation(args: { issue: { id: string; companyId: string }; current: IssueThreadInteractionRow; @@ -1614,13 +1623,29 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti return { checked: rows.length, candidates: eligible.length, accepted, woken }; }, listForIssue: async (issueId: string) => { - const rows = await db - .select() - .from(issueThreadInteractions) - .where(eq(issueThreadInteractions.issueId, issueId)) - .orderBy(asc(issueThreadInteractions.createdAt), asc(issueThreadInteractions.id)); + const [rows, issueStatus] = await Promise.all([ + db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.issueId, issueId)) + .orderBy(asc(issueThreadInteractions.createdAt), asc(issueThreadInteractions.id)), + db + .select({ status: issues.status }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((issueRows) => issueRows[0]?.status ?? null), + ]); - return rows.map((row) => hydrateInteraction(row)); + return rows.map((row) => hydrateInteraction( + issueStatus && isTerminalIssueStatus(issueStatus) && row.status === "pending" + ? { + ...row, + status: "expired", + result: buildAdministrativeOutcomeResult(row, "issue_closed"), + resolvedAt: row.updatedAt, + } + : row, + )); }, getById: async (interactionId: string) => { @@ -1976,7 +2001,7 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti }, acceptInteraction: async ( - issue: { id: string; companyId: string; projectId: string | null; goalId: string | null }, + issue: { id: string; companyId: string; projectId: string | null; goalId: string | null; status?: string }, interactionId: string, input: AcceptIssueThreadInteraction, actor: InteractionActor, @@ -2024,11 +2049,12 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti }, acceptSuggestedTasks: async ( - issue: { id: string; companyId: string; projectId: string | null; goalId: string | null }, + issue: { id: string; companyId: string; projectId: string | null; goalId: string | null; status?: string }, interactionId: string, input: AcceptIssueThreadInteraction, actor: InteractionActor, ) => { + assertIssueOpenForInteractionResolution(issue); const current = await db .select() .from(issueThreadInteractions) @@ -2177,7 +2203,7 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti }, rejectInteraction: async ( - issue: { id: string; companyId: string }, + issue: { id: string; companyId: string; status?: string }, interactionId: string, input: RejectIssueThreadInteraction, actor: InteractionActor, @@ -2202,11 +2228,12 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti }, submitItemVerdicts: async ( - issue: { id: string; companyId: string }, + issue: { id: string; companyId: string; status?: string }, interactionId: string, input: SubmitIssueThreadInteractionVerdicts, actor: InteractionActor, ): Promise<{ interaction: IssueThreadInteraction; newlyResolvedItemIds: string[] }> => { + assertIssueOpenForInteractionResolution(issue); const data = submitIssueThreadInteractionVerdictsSchema.parse(input); const submission = await db.transaction(async (tx) => { const current = await tx @@ -2303,12 +2330,13 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti }, rejectSuggestedTasks: async ( - issue: { id: string; companyId: string }, + issue: { id: string; companyId: string; status?: string }, interactionId: string, input: RejectIssueThreadInteraction, actor: InteractionActor, current: IssueThreadInteractionRow, ) => { + assertIssueOpenForInteractionResolution(issue); if (current.companyId !== issue.companyId || current.issueId !== issue.id) { throw notFound("Interaction not found"); } @@ -2350,7 +2378,7 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti }, expireRequestConfirmationsSupersededByComment: async ( - issue: { id: string; companyId: string }, + issue: { id: string; companyId: string; status?: string }, comment: { id: string; createdAt: Date | string; authorUserId?: string | null; createdByRunId?: string | null }, actor: InteractionActor, ) => { @@ -2694,6 +2722,7 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti input: WithdrawIssueThreadInteraction, actor: InteractionActor, ) => { + assertIssueOpenForInteractionResolution(issue); const data = withdrawIssueThreadInteractionSchema.parse(input); const current = await db .select() @@ -2761,11 +2790,12 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti }, answerQuestions: async ( - issue: { id: string; companyId: string }, + issue: { id: string; companyId: string; status?: string }, interactionId: string, input: RespondIssueThreadInteraction, actor: InteractionActor, ) => { + assertIssueOpenForInteractionResolution(issue); const current = await db .select() .from(issueThreadInteractions) @@ -2822,11 +2852,12 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti }, cancelQuestions: async ( - issue: { id: string; companyId: string }, + issue: { id: string; companyId: string; status?: string }, interactionId: string, input: CancelIssueThreadInteraction, actor: InteractionActor, ) => { + assertIssueOpenForInteractionResolution(issue); const data = cancelIssueThreadInteractionSchema.parse(input); const current = await db .select() diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index b87156d913..75f1f4ee92 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -8692,6 +8692,40 @@ export function issueService(db: Db) { .set({ updatedAt: new Date() }) .where(eq(issues.id, issueId)); + if ( + authorType === "user" && + actor.userId && + actor.userId !== "board-concierge" && + !createdByRunId + ) { + const { issueThreadInteractionService } = await import("./issue-thread-interactions.js"); + const expiredInteractions = await issueThreadInteractionService(dbOrTx) + .expireRequestConfirmationsSupersededByComment( + { id: issueId, companyId: issue.companyId }, + comment, + { agentId: actor.agentId, userId: actor.userId }, + ); + for (const interaction of expiredInteractions) { + await logActivity(dbOrTx, { + companyId: issue.companyId, + actorType: "user", + actorId: actor.userId, + agentId: actor.agentId ?? null, + runId: createdByRunId, + action: "issue.thread_interaction_expired", + entityType: "issue", + entityId: issueId, + details: { + interactionId: interaction.id, + interactionKind: interaction.kind, + interactionStatus: interaction.status, + source: "issue.comment.service", + result: interaction.result ?? null, + }, + }); + } + } + return redactIssueComment(comment, currentUserRedactionOptions.enabled); }, diff --git a/server/src/services/plugin-host-services.ts b/server/src/services/plugin-host-services.ts index 1428f42fd4..09f40e250b 100644 --- a/server/src/services/plugin-host-services.ts +++ b/server/src/services/plugin-host-services.ts @@ -2492,7 +2492,13 @@ export function buildHostServices( }; if (params.action === "accept") { const result = await interactions.acceptInteraction( - { id: issue.id, companyId, projectId: issue.projectId ?? null, goalId: issue.goalId ?? null }, + { + id: issue.id, + companyId, + projectId: issue.projectId ?? null, + goalId: issue.goalId ?? null, + status: issue.status, + }, params.interactionId, {}, actor, @@ -2507,7 +2513,7 @@ export function buildHostServices( } } else { resolved = (await interactions.rejectInteraction( - { id: issue.id, companyId }, + { id: issue.id, companyId, status: issue.status }, params.interactionId, { reason: params.reason ?? undefined }, actor,