From b7fd6c59b6e0e8dc54c7ac0da63a62db4eb01ee0 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Thu, 27 Aug 2026 18:37:30 -0700 Subject: [PATCH] fix(server): load the costs-service route module graph once per file (#12375) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The server test suite checks budget and cost routes > - The costs-service test rebuilt the full route module graph before every test > - Synchronous graph rebuilds caused long stalls under CPU load > - This pull request loads the mocked graph once for each describe block and keeps per-test mock setup > - The benefit is a stable 17-test file without production code changes ## Linked Issues or Issue Description **What happened?** The costs-service route test rebuilt its full mocked module graph before every test. Under CPU load, a rebuild sometimes stalled a test past the 15-second timeout. **Expected behavior** The test file should load its mocked route graph once for each describe block while each test keeps isolated mock behavior. **Steps to reproduce** 1. Run the costs-service route test under synthetic CPU load. 2. Repeat the file test 30 times. 3. Observe intermittent test timeouts before this change. **Paperclip version or commit** The test used the current master branch at the time of this change. **Deployment mode** Built from source. ## What Changed - Add `hoistModuleGraph` to load the mocked route graph once for each describe block. - Keep per-test mock setup in `beforeEach` so test isolation stays unchanged. - Keep all 17 tests and their assertions. - Remove the module graph rebuild from the per-test path. ## Verification - Run `npx vitest run src/__tests__/costs-service.test.ts` from `server/`. - Confirm that the file reports 17 tests and zero skipped tests. - Confirm that 30 runs under the same synthetic CPU load report 0.0% failure after the change, compared with 10.0% before the change. - Confirm that mutation checks still fail when each authorization guard is broken. ## Risks This change affects test setup only. The main risk is weaker test isolation if a mock keeps state between tests. Each test still re-arms its mock behavior in `beforeEach`, and the full assertion set remains. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. This bug fix does not add a core feature. ## Model Used OpenAI GPT-5. The model used tool calls and code execution. The exact context window and reasoning configuration were not exposed by the runtime. ## 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 (the submitting engineer ran the file before handoff) - [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 --- server/src/__tests__/costs-service.test.ts | 191 +++++++++--------- .../__tests__/helpers/hoist-module-graph.ts | 40 ++++ 2 files changed, 134 insertions(+), 97 deletions(-) create mode 100644 server/src/__tests__/helpers/hoist-module-graph.ts diff --git a/server/src/__tests__/costs-service.test.ts b/server/src/__tests__/costs-service.test.ts index 25cccb8094..a8ea5fa299 100644 --- a/server/src/__tests__/costs-service.test.ts +++ b/server/src/__tests__/costs-service.test.ts @@ -21,6 +21,7 @@ import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase, } from "./helpers/embedded-postgres.js"; +import { hoistModuleGraph } from "./helpers/hoist-module-graph.js"; function makeDb(overrides: Record = {}) { const selectChain = { @@ -127,94 +128,90 @@ function registerModuleMocks() { })); } -async function createApp() { - const [{ costRoutes }, { errorHandler }] = await Promise.all([ - vi.importActual("../routes/costs.js"), - vi.importActual("../middleware/index.js"), - ]); - const app = express(); - app.use(express.json()); - app.use((req, _res, next) => { - req.actor = { type: "board", userId: "board-user", source: "local_implicit" }; - next(); - }); - app.use("/api", costRoutes(makeDb() as any)); - app.use(errorHandler); - return app; -} - -async function createAppWithActor(actor: any) { - const [{ costRoutes }, { errorHandler }] = await Promise.all([ - vi.importActual("../routes/costs.js"), - vi.importActual("../middleware/index.js"), - ]); - const app = express(); - app.use(express.json()); - app.use((req, _res, next) => { - req.actor = actor; - next(); - }); - app.use("/api", costRoutes(makeDb() as any)); - app.use(errorHandler); - return app; -} - -async function loadCostParsers() { - const { parseCostDateRange, parseCostLimit } = await import("../routes/costs.js"); - return { parseCostDateRange, parseCostLimit }; -} - -beforeEach(() => { - vi.resetModules(); - vi.doUnmock("../services/index.js"); - vi.doUnmock("../services/quota-windows.js"); - vi.doUnmock("../routes/costs.js"); - vi.doUnmock("../middleware/index.js"); - registerModuleMocks(); - vi.clearAllMocks(); - mockAccessService.decide.mockReset(); - mockAccessService.decide.mockResolvedValue({ - allowed: true, - action: "company_scope:read", - reason: "allow_test", - explanation: "Allowed by test mock.", - }); - mockCompanyService.update.mockResolvedValue({ - id: "company-1", - name: "Paperclip", - budgetMonthlyCents: 100, - spentMonthlyCents: 0, - }); - mockAgentService.getById.mockResolvedValue({ - id: "agent-1", - companyId: "company-1", - name: "Budget Agent", - budgetMonthlyCents: 100, - spentMonthlyCents: 0, - }); - mockAgentService.update.mockResolvedValue({ - id: "agent-1", - companyId: "company-1", - name: "Budget Agent", - budgetMonthlyCents: 100, - spentMonthlyCents: 0, - }); - mockIssueService.getById.mockResolvedValue({ - id: "issue-1", - companyId: "company-1", - identifier: "PC1A2-1", - }); - mockIssueService.getByIdentifier.mockResolvedValue({ - id: "issue-1", - companyId: "company-1", - identifier: "PC1A2-1", - }); - mockBudgetService.upsertPolicy.mockResolvedValue(undefined); -}); - describe("cost routes", () => { + const routeModules = hoistModuleGraph(registerModuleMocks, async () => { + const [costsRouteModule, middlewareModule] = await Promise.all([ + vi.importActual("../routes/costs.js"), + vi.importActual("../middleware/index.js"), + ]); + return { ...costsRouteModule, errorHandler: middlewareModule.errorHandler }; + }); + + function createApp() { + const { costRoutes, errorHandler } = routeModules.value; + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.actor = { type: "board", userId: "board-user", source: "local_implicit" }; + next(); + }); + app.use("/api", costRoutes(makeDb() as any)); + app.use(errorHandler); + return app; + } + + function createAppWithActor(actor: any) { + const { costRoutes, errorHandler } = routeModules.value; + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.actor = actor; + next(); + }); + app.use("/api", costRoutes(makeDb() as any)); + app.use(errorHandler); + return app; + } + + function loadCostParsers() { + const { parseCostDateRange, parseCostLimit } = routeModules.value; + return { parseCostDateRange, parseCostLimit }; + } + + beforeEach(() => { + vi.clearAllMocks(); + mockAccessService.decide.mockReset(); + mockAccessService.decide.mockResolvedValue({ + allowed: true, + action: "company_scope:read", + reason: "allow_test", + explanation: "Allowed by test mock.", + }); + mockCompanyService.update.mockResolvedValue({ + id: "company-1", + name: "Paperclip", + budgetMonthlyCents: 100, + spentMonthlyCents: 0, + }); + mockAgentService.getById.mockResolvedValue({ + id: "agent-1", + companyId: "company-1", + name: "Budget Agent", + budgetMonthlyCents: 100, + spentMonthlyCents: 0, + }); + mockAgentService.update.mockResolvedValue({ + id: "agent-1", + companyId: "company-1", + name: "Budget Agent", + budgetMonthlyCents: 100, + spentMonthlyCents: 0, + }); + mockIssueService.getById.mockResolvedValue({ + id: "issue-1", + companyId: "company-1", + identifier: "PC1A2-1", + }); + mockIssueService.getByIdentifier.mockResolvedValue({ + id: "issue-1", + companyId: "company-1", + identifier: "PC1A2-1", + }); + mockBudgetService.upsertPolicy.mockResolvedValue(undefined); + }); + it("accepts valid ISO date strings", async () => { - const { parseCostDateRange } = await loadCostParsers(); + const { parseCostDateRange } = loadCostParsers(); expect(parseCostDateRange({ from: "2026-01-01T00:00:00.000Z", to: "2026-01-31T23:59:59.999Z", @@ -225,17 +222,17 @@ describe("cost routes", () => { }); it("returns 400 for an invalid 'from' date string", async () => { - const { parseCostDateRange } = await loadCostParsers(); + const { parseCostDateRange } = loadCostParsers(); expect(() => parseCostDateRange({ from: "not-a-date" })).toThrow(/invalid 'from' date/i); }); it("returns 400 for an invalid 'to' date string", async () => { - const { parseCostDateRange } = await loadCostParsers(); + const { parseCostDateRange } = loadCostParsers(); expect(() => parseCostDateRange({ to: "banana" })).toThrow(/invalid 'to' date/i); }); it("returns finance summary rows for valid requests", async () => { - const app = await createApp(); + const app = createApp(); const res = await request(app) .get("/api/companies/company-1/costs/finance-summary") .query({ from: "2026-02-01T00:00:00.000Z", to: "2026-02-28T23:59:59.999Z" }); @@ -250,7 +247,7 @@ describe("cost routes", () => { }); it("returns issue subtree cost summaries for issue refs", async () => { - const app = await createApp(); + const app = createApp(); const res = await request(app).get("/api/issues/pc1a2-1/cost-summary"); expect(res.status).toBe(200); @@ -272,17 +269,17 @@ describe("cost routes", () => { }); it("returns 400 for invalid finance event list limits", async () => { - const { parseCostLimit } = await loadCostParsers(); + const { parseCostLimit } = loadCostParsers(); expect(() => parseCostLimit({ limit: "0" })).toThrow(/invalid 'limit'/i); }); it("accepts valid finance event list limits", async () => { - const { parseCostLimit } = await loadCostParsers(); + const { parseCostLimit } = loadCostParsers(); expect(parseCostLimit({ limit: "25" })).toBe(25); }); it("rejects company budget updates for board users outside the company", async () => { - const app = await createAppWithActor({ + const app = createAppWithActor({ type: "board", userId: "board-user", source: "session", @@ -299,7 +296,7 @@ describe("cost routes", () => { }); it("rejects agent budget updates for board users outside the agent company", async () => { - const app = await createAppWithActor({ + const app = createAppWithActor({ type: "board", userId: "board-user", source: "session", @@ -317,7 +314,7 @@ describe("cost routes", () => { }); it("rejects agent budget updates from the target agent without changing the budget policy", async () => { - const app = await createAppWithActor({ + const app = createAppWithActor({ type: "agent", agentId: "agent-1", companyId: "company-1", @@ -336,7 +333,7 @@ describe("cost routes", () => { }); it("rejects agent budget updates from another same-company agent without changing the budget policy", async () => { - const app = await createAppWithActor({ + const app = createAppWithActor({ type: "agent", agentId: "agent-2", companyId: "company-1", @@ -362,7 +359,7 @@ describe("cost routes", () => { budgetMonthlyCents: 2500, spentMonthlyCents: 0, }); - const app = await createAppWithActor({ + const app = createAppWithActor({ type: "board", userId: "board-user", source: "session", diff --git a/server/src/__tests__/helpers/hoist-module-graph.ts b/server/src/__tests__/helpers/hoist-module-graph.ts new file mode 100644 index 0000000000..2ab237d329 --- /dev/null +++ b/server/src/__tests__/helpers/hoist-module-graph.ts @@ -0,0 +1,40 @@ +import { beforeAll } from "vitest"; + +/** + * Loads a mocked module graph one time for the enclosing `describe` block, + * not one time for each test. + * + * A per-test `vi.resetModules()` plus a per-test `vi.importActual` (or a + * dynamic `import()`) forces Node to transform and evaluate the module + * graph again on every test. This work is synchronous and CPU-bound. Under + * CPU load, one re-import can take seconds instead of milliseconds and + * stall a test past its timeout. + * + * A route module graph does not need a fresh import for test isolation. + * `vi.hoisted` mocks keep a stable identity across tests. Each test can + * still re-arm mock behavior in its own `beforeEach`. Register the module + * mocks one time. Import the graph one time in `beforeAll`. Reuse the + * result for every test in the block. + * + * Read `.value` only inside a test body. It throws before `beforeAll` runs. + */ +export function hoistModuleGraph( + registerMocks: () => void, + loadGraph: () => Promise, +): { readonly value: T } { + let graph: T | undefined; + + beforeAll(async () => { + registerMocks(); + graph = await loadGraph(); + }); + + return { + get value(): T { + if (graph === undefined) { + throw new Error("module graph is not loaded yet — read .value inside a test"); + } + return graph; + }, + }; +}