From 05ab9e526c6a0c0e1feffade862d062293487d91 Mon Sep 17 00:00:00 2001 From: Paperclip PlatformSREEngineer Date: Wed, 5 Aug 2026 02:04:53 +0000 Subject: [PATCH 1/2] test(server): stop bounds-cache test from driving 261 sequential HTTP round trips (BLO-21754) `bounds compact issue-list server cache entries` looped ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES + 5 (261) real supertest requests inside vitest's 60s default budget. Under a loaded ARC runner it timed out, and the trailing size assertion then read a partially-filled cache mid-timeout, producing a second, misleading "expected 143 to be 256" error that reads like an LRU regression rather than a slow runner. Add a test-only override for the cache bound (__setIssueListResponseCacheMaxEntriesForTests / __resetIssueListResponseCacheMaxEntriesForTests) so the test exercises the same trimIssueListResponseCache eviction path against a small injected bound (5) instead of the production value (256) -- 10 round trips instead of 261. Also guard the size assertion behind an explicit completedRequests count and give the test its own tight 10s timeout so a future regression reports one clear failure instead of a shadowed pair. Verified: with eviction temporarily disabled the test fails ("expected 10 to be 5"), confirming it still pins the bound rather than passing vacuously. Duration: ~60s timeout -> 407-942ms locally, 14/14 tests in the file pass. --- .../issue-list-assignee-filter-routes.test.ts | 84 +++++++++++-------- server/src/routes/issues.ts | 17 +++- 2 files changed, 66 insertions(+), 35 deletions(-) diff --git a/server/src/__tests__/issue-list-assignee-filter-routes.test.ts b/server/src/__tests__/issue-list-assignee-filter-routes.test.ts index 3c88bd3beb14..e9aa4de008b5 100644 --- a/server/src/__tests__/issue-list-assignee-filter-routes.test.ts +++ b/server/src/__tests__/issue-list-assignee-filter-routes.test.ts @@ -12,7 +12,8 @@ import { errorHandler } from "../middleware/index.js"; import { __clearIssueListResponseCacheForTests, __getIssueListResponseCacheSizeForTests, - ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES, + __resetIssueListResponseCacheMaxEntriesForTests, + __setIssueListResponseCacheMaxEntriesForTests, issueRoutes, } from "../routes/issues.js"; import { issueRecoveryActionService } from "../services/issue-recovery-actions.js"; @@ -550,41 +551,56 @@ describeEmbeddedPostgres("issue list routes assigneeAgentId filter", () => { expect(second.headers["x-paperclip-request-cache"]).toBe("hit"); }); - it("bounds compact issue-list server cache entries", async () => { - const companyId = randomUUID(); - const issueId = randomUUID(); + it( + "bounds compact issue-list server cache entries", + async () => { + const companyId = randomUUID(); + const issueId = randomUUID(); - await db.insert(companies).values({ - id: companyId, - name: "Paperclip", - issuePrefix: uniqueIssuePrefix(), - requireBoardApprovalForNewAgents: false, - }); - await seedCloudTenantMember(companyId); - await db.insert(issues).values({ - id: issueId, - companyId, - title: "Bounded cache issue", - status: "todo", - priority: "medium", - }); - - const app = createApp(companyId); - const fixedNow = Date.now(); - const nowSpy = vi.spyOn(Date, "now").mockReturnValue(fixedNow); - try { - for (let index = 0; index < ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES + 5; index += 1) { - const res = await request(app) - .get(`/api/companies/${companyId}/issues`) - .query({ view: "compact", limit: "20", q: `cache-key-${index}` }); - expect(res.status, JSON.stringify(res.body)).toBe(200); + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: uniqueIssuePrefix(), + requireBoardApprovalForNewAgents: false, + }); + await seedCloudTenantMember(companyId); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Bounded cache issue", + status: "todo", + priority: "medium", + }); + + const app = createApp(companyId); + const fixedNow = Date.now(); + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(fixedNow); + // Small injected bound so this stays a handful of round trips instead of + // scaling with the production cache size (previously 261 sequential + // requests, which blew vitest's 60s default under a loaded runner). + const testMaxEntries = 5; + __setIssueListResponseCacheMaxEntriesForTests(testMaxEntries); + let completedRequests = 0; + try { + for (let index = 0; index < testMaxEntries + 5; index += 1) { + const res = await request(app) + .get(`/api/companies/${companyId}/issues`) + .query({ view: "compact", limit: "20", q: `cache-key-${index}` }); + expect(res.status, JSON.stringify(res.body)).toBe(200); + completedRequests += 1; + } + + // Guard behind loop completion so a hypothetical timeout reports only + // the timeout, never a second, misleading cache-size assertion. + expect(completedRequests).toBe(testMaxEntries + 5); + expect(__getIssueListResponseCacheSizeForTests()).toBe(testMaxEntries); + } finally { + nowSpy.mockRestore(); + __resetIssueListResponseCacheMaxEntriesForTests(); } - - expect(__getIssueListResponseCacheSizeForTests()).toBe(ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES); - } finally { - nowSpy.mockRestore(); - } - }); + }, + 10_000, + ); it("logs request_storm_detected for identical in-flight compact issue-list fanout without query values", async () => { const companyId = randomUUID(); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 63138e4a756c..d7d8c1045a76 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -2393,6 +2393,20 @@ export function __clearIssueListResponseCacheForTests() { issueListResponseCache.clear(); } +let issueListResponseCacheMaxEntriesOverrideForTests: number | null = null; + +export function __setIssueListResponseCacheMaxEntriesForTests(maxEntries: number) { + issueListResponseCacheMaxEntriesOverrideForTests = maxEntries; +} + +export function __resetIssueListResponseCacheMaxEntriesForTests() { + issueListResponseCacheMaxEntriesOverrideForTests = null; +} + +function effectiveIssueListResponseCacheMaxEntries() { + return issueListResponseCacheMaxEntriesOverrideForTests ?? ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES; +} + function shortHash(value: string): string { return createHash("sha256").update(value).digest("base64url").slice(0, 16); } @@ -2519,7 +2533,8 @@ function touchIssueListResponseCacheEntry(key: string, entry: IssueListCacheEntr } function trimIssueListResponseCache() { - while (issueListResponseCache.size > ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES) { + const maxEntries = effectiveIssueListResponseCacheMaxEntries(); + while (issueListResponseCache.size > maxEntries) { const oldestKey = issueListResponseCache.keys().next().value as string | undefined; if (oldestKey === undefined) return; issueListResponseCache.delete(oldestKey); From dfc1cddf7ee73e8ee0c6f22ed7ea9d1723f2767e Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Wed, 5 Aug 2026 22:23:27 +0000 Subject: [PATCH 2/2] fix(server): remove module-global cache-bound override, seed cache entries directly in test (BLO-21754) Ally flagged that the test-only __setIssueListResponseCacheMaxEntriesForTests override is process-global and only reset in a finally block after the async HTTP loop; a vitest timeout doesn't cancel that body, so a slow run could leak the small test bound into a later test after teardown started - reproducing the same shadowed-failure pattern this PR set out to remove. Replace the HTTP-loop-plus-override approach with a synchronous unit test that seeds cache entries directly through the same setIssueListResponseCacheEntry() path production requests use, and drop the override mechanism entirely - the real ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES bound is asserted with no DB, no HTTP, and no awaits, so there's nothing to time out or leak between tests. Co-Authored-By: Paperclip --- .../issue-list-assignee-filter-routes.test.ts | 75 ++++++------------- server/src/routes/issues.ts | 21 ++---- 2 files changed, 27 insertions(+), 69 deletions(-) diff --git a/server/src/__tests__/issue-list-assignee-filter-routes.test.ts b/server/src/__tests__/issue-list-assignee-filter-routes.test.ts index e9aa4de008b5..19cb261e8b0c 100644 --- a/server/src/__tests__/issue-list-assignee-filter-routes.test.ts +++ b/server/src/__tests__/issue-list-assignee-filter-routes.test.ts @@ -2,7 +2,7 @@ import { randomUUID } from "node:crypto"; import express from "express"; import request from "supertest"; import { eq } from "drizzle-orm"; -import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; import { activityLog, agents, companies, companyMemberships, createDb, heartbeatRuns, issues, principalPermissionGrants } from "@paperclipai/db"; import { getEmbeddedPostgresTestSupport, @@ -12,8 +12,8 @@ import { errorHandler } from "../middleware/index.js"; import { __clearIssueListResponseCacheForTests, __getIssueListResponseCacheSizeForTests, - __resetIssueListResponseCacheMaxEntriesForTests, - __setIssueListResponseCacheMaxEntriesForTests, + __setIssueListResponseCacheEntryForTests, + ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES, issueRoutes, } from "../routes/issues.js"; import { issueRecoveryActionService } from "../services/issue-recovery-actions.js"; @@ -551,56 +551,25 @@ describeEmbeddedPostgres("issue list routes assigneeAgentId filter", () => { expect(second.headers["x-paperclip-request-cache"]).toBe("hit"); }); - it( - "bounds compact issue-list server cache entries", - async () => { - const companyId = randomUUID(); - const issueId = randomUUID(); - - await db.insert(companies).values({ - id: companyId, - name: "Paperclip", - issuePrefix: uniqueIssuePrefix(), - requireBoardApprovalForNewAgents: false, - }); - await seedCloudTenantMember(companyId); - await db.insert(issues).values({ - id: issueId, - companyId, - title: "Bounded cache issue", - status: "todo", - priority: "medium", - }); - - const app = createApp(companyId); - const fixedNow = Date.now(); - const nowSpy = vi.spyOn(Date, "now").mockReturnValue(fixedNow); - // Small injected bound so this stays a handful of round trips instead of - // scaling with the production cache size (previously 261 sequential - // requests, which blew vitest's 60s default under a loaded runner). - const testMaxEntries = 5; - __setIssueListResponseCacheMaxEntriesForTests(testMaxEntries); - let completedRequests = 0; - try { - for (let index = 0; index < testMaxEntries + 5; index += 1) { - const res = await request(app) - .get(`/api/companies/${companyId}/issues`) - .query({ view: "compact", limit: "20", q: `cache-key-${index}` }); - expect(res.status, JSON.stringify(res.body)).toBe(200); - completedRequests += 1; - } - - // Guard behind loop completion so a hypothetical timeout reports only - // the timeout, never a second, misleading cache-size assertion. - expect(completedRequests).toBe(testMaxEntries + 5); - expect(__getIssueListResponseCacheSizeForTests()).toBe(testMaxEntries); - } finally { - nowSpy.mockRestore(); - __resetIssueListResponseCacheMaxEntriesForTests(); - } - }, - 10_000, - ); + it("bounds compact issue-list server cache entries", () => { + // Drive the exact same insert-and-trim path a real request takes + // (setIssueListResponseCacheEntry), but with synthetic entries inserted + // synchronously instead of 261 real HTTP round trips against Postgres. + // No DB, no HTTP, no awaits — nothing here can time out under a loaded + // runner, and there's no module-global override left to leak into a + // later test if it did. + const syntheticEntry = { + response: { kind: "compact" as const, body: [], etag: "test-etag", cacheControl: "no-store" }, + expiresAt: Date.now() + 1_000, + staleUntil: Date.now() + 5_000, + }; + + for (let index = 0; index < ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES + 5; index += 1) { + __setIssueListResponseCacheEntryForTests(`cache-key-${index}`, syntheticEntry); + } + + expect(__getIssueListResponseCacheSizeForTests()).toBe(ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES); + }); it("logs request_storm_detected for identical in-flight compact issue-list fanout without query values", async () => { const companyId = randomUUID(); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index d7d8c1045a76..4465ac828e22 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -2393,20 +2393,6 @@ export function __clearIssueListResponseCacheForTests() { issueListResponseCache.clear(); } -let issueListResponseCacheMaxEntriesOverrideForTests: number | null = null; - -export function __setIssueListResponseCacheMaxEntriesForTests(maxEntries: number) { - issueListResponseCacheMaxEntriesOverrideForTests = maxEntries; -} - -export function __resetIssueListResponseCacheMaxEntriesForTests() { - issueListResponseCacheMaxEntriesOverrideForTests = null; -} - -function effectiveIssueListResponseCacheMaxEntries() { - return issueListResponseCacheMaxEntriesOverrideForTests ?? ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES; -} - function shortHash(value: string): string { return createHash("sha256").update(value).digest("base64url").slice(0, 16); } @@ -2533,8 +2519,7 @@ function touchIssueListResponseCacheEntry(key: string, entry: IssueListCacheEntr } function trimIssueListResponseCache() { - const maxEntries = effectiveIssueListResponseCacheMaxEntries(); - while (issueListResponseCache.size > maxEntries) { + while (issueListResponseCache.size > ISSUE_LIST_SERVER_CACHE_MAX_ENTRIES) { const oldestKey = issueListResponseCache.keys().next().value as string | undefined; if (oldestKey === undefined) return; issueListResponseCache.delete(oldestKey); @@ -2546,6 +2531,10 @@ function setIssueListResponseCacheEntry(key: string, entry: IssueListCacheEntry) trimIssueListResponseCache(); } +export function __setIssueListResponseCacheEntryForTests(key: string, entry: IssueListCacheEntry) { + setIssueListResponseCacheEntry(key, entry); +} + function decrementIssueListActorClientInflight(actorClientKey: string) { const next = (issueListActorClientInflight.get(actorClientKey) ?? 1) - 1; if (next <= 0) issueListActorClientInflight.delete(actorClientKey);