From 5f463dd625ec280951ad4a9243b0865159482ad5 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Wed, 14 Oct 2026 23:21:16 -0700 Subject: [PATCH] test(server): reuse route modules across test cases (#16396) ## Thinking Path > - Paperclip uses server routes to manage agents and their work. > - Route tests check permissions, login sessions, skills, and file downloads. > - Four suites reload large module graphs before every test. > - Their workers exhaust the heap and block dependency updates. > - This pull request loads each route graph once per suite. > - Each test still gets a fresh app and resets its fixture state. ## Linked Issues or Issue Description Refs #16017. Related: #12987 addresses repeated logger transports through a different production change. **What happened?** CI run 38293835699, attempt 2, aborted four server test workers. The affected suites cover attachments, device login, agent skills, and setup tokens. Three failed log tails show heap exhaustion. The separate `db.transaction` warning came from the document-annotation suite, which passed. **Expected behavior** All route cases complete within the worker heap limit and keep independent test state. **Steps to reproduce** Run the unchanged agent-skills suite from master with `NODE_OPTIONS=--max-old-space-size=1536` and `--logHeapUsage`. The worker aborted after 66 of 90 cases, at a reported heap of 1,532 MB. **Paperclip version** The fix is based on master commit `444d5854b0f53b19d138f060a267cfd10171c7b3`. The original dependency PR head is `7b74c3f248731943154547385b1de5eb6562bf01`. **Deployment mode** Local tests and CI. ## What Changed - Move module resets and mock registration from `beforeEach` to `beforeAll` in four route suites. - Load the agent-skills route modules during suite setup. - Keep each test's app, fixture resets, and login cleanup. - Remove two comments that describe the old reset behavior. ## Verification All 193 tests in the four affected suites pass with a 1.5 GiB heap limit: ```sh NODE_OPTIONS=--max-old-space-size=1536 pnpm exec vitest run --project @paperclipai/server \ server/src/__tests__/agent-skills-routes.test.ts \ server/src/__tests__/agent-device-login-routes.test.ts \ server/src/__tests__/issue-attachment-routes.test.ts \ server/src/routes/setup-token-route.test.ts --reporter=verbose --logHeapUsage ``` - Local runtime: Node 25.9.0, Vitest 5.0.4, Supertest 7.3.1, Express 5.2.1. - Master has an existing manifest/lockfile mismatch. Local installation used `--no-lockfile`. Express was restored to its locked version in an untracked test dependency directory. The tracked lockfile is unchanged. - The agent-skills suite stays below 400 MB of reported heap after the fix. - `git diff --check` passes. - Repository typecheck reaches the Runner Rust check and stops because this environment has no `cargo`. - The repository build and normal server typecheck also stop at the missing Rust toolchain. A direct server `tsc --noEmit` attempt ends with SIGKILL. - [CI run 38686964537](https://github.com/paperclipai/paperclip/actions/runs/38686964537) passes on `c744774bbada4e35ce3acf5991ff19cf50405f2f`, including all general and serialized server shards, typecheck, build, Runner checks, and e2e. - GitHub reports 53 successful check runs, two skipped Storybook checks, and a successful Snyk status. No checks are pending or failing. - [Greptile review](https://github.com/paperclipai/paperclip/pull/16396#issuecomment-6185194537) gives 5/5 on the same commit, with no actionable issues and zero review threads. - The full repository test suite was verified in CI; the local container has a 4 GiB memory limit. ## Risks Module state now lasts for one suite. Each case still constructs its own router and resets its mocks and fixtures. The affected suites pass together locally, and all applicable CI gates pass on the published commit. The change contains four test files. It changes no production code, workflow, manifest, or lockfile. Merge this separate fix before asking Dependabot to rebase #16017. ## Model Used OpenAI GPT-6 through Codex, with reasoning, repository tools, and code execution. The runtime does not expose a more specific model ID or context-window size. ## 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 --- .../agent-device-login-routes.test.ts | 10 ++++----- .../src/__tests__/agent-skills-routes.test.ts | 22 +++++++++---------- .../__tests__/issue-attachment-routes.test.ts | 7 ++++-- server/src/routes/setup-token-route.test.ts | 7 ++++-- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/server/src/__tests__/agent-device-login-routes.test.ts b/server/src/__tests__/agent-device-login-routes.test.ts index c9e1841964f..98128d9438c 100644 --- a/server/src/__tests__/agent-device-login-routes.test.ts +++ b/server/src/__tests__/agent-device-login-routes.test.ts @@ -3,7 +3,7 @@ import request from "supertest"; import { lstat, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { withAccountHomeSecretMutationLock } from "@paperclipai/adapter-codex-local/server"; import { AdapterAuthSessionConflictError } from "../services/device-login-service.js"; import type { @@ -386,8 +386,11 @@ describe("adapter device-login routes", () => { // `afterEach` whether or not the route removed them itself. const accountHomeTestDirs: string[] = []; - beforeEach(() => { + beforeAll(() => { vi.resetModules(); + }); + + beforeEach(() => { vi.clearAllMocks(); mockDeviceLoginPromotion.mockResolvedValue({ outcome: "promoted", @@ -1330,9 +1333,6 @@ describe("adapter device-login routes", () => { accountHomeDir, accountHomeCreated: true, }); - // `vi.resetModules()` in `beforeEach` gives the route module a fresh copy - // of `../errors.js`. Import the conflict helper from that same fresh copy, - // so the route's `err instanceof HttpError` check sees a matching class. const { conflict: freshConflict } = await vi.importActual("../errors.js"); mockSecretService.create.mockRejectedValueOnce( diff --git a/server/src/__tests__/agent-skills-routes.test.ts b/server/src/__tests__/agent-skills-routes.test.ts index 6e4f29ddf10..f49dcd8ea56 100644 --- a/server/src/__tests__/agent-skills-routes.test.ts +++ b/server/src/__tests__/agent-skills-routes.test.ts @@ -1,7 +1,7 @@ import express from "express"; import { readFile } from "node:fs/promises"; import request from "supertest"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; const runnerTarget = vi.hoisted(() => ({ driver: "local", platform: "linux", architecture: "x64" })); vi.mock("../services/agent-runner-selection.js", async (importOriginal) => { @@ -272,14 +272,21 @@ function skillSnapshot(adapterType = "claude_local", mode = "ephemeral") { } describe("agent skill routes", () => { - beforeEach(async () => { - runnerTarget.platform = "linux"; - runnerTarget.architecture = "x64"; + beforeAll(async () => { vi.resetModules(); vi.doUnmock("../routes/agents.js"); vi.doUnmock("../routes/authz.js"); vi.doUnmock("../middleware/index.js"); registerModuleMocks(); + [{ agentRoutes }, { errorHandler }] = await Promise.all([ + vi.importActual("../routes/agents.js"), + vi.importActual("../middleware/index.js"), + ]); + }); + + beforeEach(() => { + runnerTarget.platform = "linux"; + runnerTarget.architecture = "x64"; vi.clearAllMocks(); for (const mock of Object.values(mockAgentService)) mock.mockReset(); for (const mock of Object.values(mockAccessService)) mock.mockReset(); @@ -384,13 +391,6 @@ describe("agent skill routes", () => { mockAccessService.listPrincipalGrants.mockResolvedValue([]); mockAccessService.ensureMembership.mockResolvedValue(undefined); mockAccessService.setPrincipalPermission.mockResolvedValue(undefined); - - // Prepare the module graph inside the setup budget after every reset. Each - // test still constructs its own app after configuring its request-specific mocks. - [{ agentRoutes }, { errorHandler }] = await Promise.all([ - vi.importActual("../routes/agents.js"), - vi.importActual("../middleware/index.js"), - ]); }); it("skips runtime materialization when listing Claude skills", async () => { diff --git a/server/src/__tests__/issue-attachment-routes.test.ts b/server/src/__tests__/issue-attachment-routes.test.ts index 1415722b2fb..383169997da 100644 --- a/server/src/__tests__/issue-attachment-routes.test.ts +++ b/server/src/__tests__/issue-attachment-routes.test.ts @@ -2,7 +2,7 @@ import { Readable } from "node:stream"; import type { IncomingMessage } from "node:http"; import express from "express"; import request from "supertest"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import type { StorageService } from "../storage/types.js"; const mockIssueService = vi.hoisted(() => ({ @@ -224,7 +224,7 @@ describe("MAX_ATTACHMENT_BYTES", () => { }); describe("issue attachment routes", () => { - beforeEach(() => { + beforeAll(() => { vi.resetModules(); vi.doUnmock("@paperclipai/shared/telemetry"); vi.doUnmock("../telemetry.js"); @@ -235,6 +235,9 @@ describe("issue attachment routes", () => { vi.doUnmock("../routes/authz.js"); vi.doUnmock("../middleware/index.js"); registerRouteMocks(); + }); + + beforeEach(() => { vi.clearAllMocks(); mockAccessService.decide.mockResolvedValue({ allowed: true, diff --git a/server/src/routes/setup-token-route.test.ts b/server/src/routes/setup-token-route.test.ts index aaa448f9027..97357ef7966 100644 --- a/server/src/routes/setup-token-route.test.ts +++ b/server/src/routes/setup-token-route.test.ts @@ -19,7 +19,7 @@ import express from "express"; import { Writable } from "node:stream"; import request from "supertest"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { SETUP_TOKEN_SESSION_NOT_FOUND, @@ -465,9 +465,12 @@ async function settle(): Promise { } } -beforeEach(() => { +beforeAll(() => { vi.resetModules(); registerModuleMocks(); +}); + +beforeEach(() => { vi.clearAllMocks(); useOwner(); // The default registry declares the Claude setup-token capability for