From 55244f1e9cf19c382f0a5962cb67a6b1fedd9c92 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 6 Nov 2026 16:02:56 -0800 Subject: [PATCH] test(server): remove two timing flakes in cloud adapter and native cleanup tests (#19668) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Contributors wait for PR CI before merge, and a flaky test forces a re-run of the whole CI run > - Four server tests failed on 2026-11-06 on commits that passed on a later attempt > - Two of them failed because of real slowness inside a fixed time limit on contended 4-vCPU runners > - This pull request removes that slowness from the two tests. It does not raise their time limits > - Of the other two tests, one is already fixed on master, and one is not reproduced (see below) > - The benefit is fewer whole-run re-runs ## Linked Issues or Issue Description **What happened?** These tests failed in PR CI and passed on a re-run of the same commit: - `server/src/adapters/cloud-external-adapters.test.ts` › "Cloud image adapter boundary › cold boots built-ins…" failed with `Test timed out in 15000ms` (3 times). - `server/src/__tests__/retained-native-session-cleanup-work.test.ts` › "drains a backlog across discovery pages…" failed with `Test timed out in 15000ms` (2 times). **Expected behavior** Both tests pass on a loaded CI runner. **Steps to reproduce** 1. Run the test file in a 4-CPU container (`docker run --cpus=4 --memory=16g`) with 4 copies at the same time. 2. Before this change, the adapter case fails in 4 of 4 runs at 15,008 to 15,041 ms. ## What Changed - `cloud-external-adapters.test.ts`: add a `beforeAll` warm-up with a 90-second limit. - The warm-up imports `registry.js`, `plugin-loader.js` and `adapter-plugin-store.js` with an empty temporary home and the managed signal, so it reads and runs no persisted adapter package. - Before this change, the first case paid the one-time transform of the whole built-in adapter graph inside its own 15-second budget. In CI the first case timed out at 15 seconds, and the next case took 1.1 seconds. - Each case still gets fresh module state from the existing `vi.resetModules()`. No assertions change. - `retained-native-session-cleanup-work.test.ts`: - `flush()` takes an optional poll `interval`, which defaults to 50 ms, so other tests do not change. - The backlog test uses 5 ms. The worker admits one cleanup pass per 1-second fake-time step, so 103 entries need 103 polls in a row. - The next wake is scheduled before a pass releases its idle-work hold, so a faster poll cannot see a false quiet state. No assertions change. ## Verification | Test | Before | After | | --- | --- | --- | | Adapter cold boot, container, 4 copies | failed 4/4 (15,008–15,041 ms) | passed 4/4 (537–756 ms per case; the warm-up takes about 23 s) | | Adapter cold boot, host | failed 2/2 | passed 3/3 (0.6–1.6 s) | | Backlog drain, container, 4 copies with load | 6.3–6.6 s | 2.0–2.1 s | | Backlog drain, host, unloaded | 5.1 s | 1.1–1.3 s (5 runs, 29/29 pass) | - Final host run of both files with `--maxWorkers=2`: 33/33 pass. - The backlog test did not reach the 15-second timeout in local runs. The evidence for that fix is the measured poll-bound cost. **Not changed in this PR:** - `connection-management.integration.test.ts` (expected 3 to be 2): - Cause: a GitHub webhook recovery worker from an earlier test fetched through the suite-wide fetch spy 60 seconds later. - Master already fixes this with commit 0b48bc7ac5 (#19541). The failed jobs ran on bases without that commit. - `idle-completed-history.test.ts` (runtime service never ready): - Not reproduced in a 4-CPU container, even with heavy load. CPU load alone does not explain a child process that stays silent for 10 seconds. - A next step is to record spawn-to-listen timing in CI. ## Risks - The adapter warm-up adds about 1 to 25 seconds to the file's `beforeAll`, depending on load, but the test body becomes faster by the same amount. - If the adapter graph import becomes much slower, the 90-second warm-up limit shows that as a clear hook timeout. - Typecheck did not run locally. CI checks it. The edits are a `beforeAll` import and a numeric default parameter. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use and code execution. A Claude sub-agent found the causes and made the fix, and the parent session reviewed it. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- ...tained-native-session-cleanup-work.test.ts | 14 ++++++++--- .../adapters/cloud-external-adapters.test.ts | 25 ++++++++++++++++++- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/server/src/__tests__/retained-native-session-cleanup-work.test.ts b/server/src/__tests__/retained-native-session-cleanup-work.test.ts index 1f698c7b210..f34bba49fdc 100644 --- a/server/src/__tests__/retained-native-session-cleanup-work.test.ts +++ b/server/src/__tests__/retained-native-session-cleanup-work.test.ts @@ -69,12 +69,15 @@ describe("retained native cleanup scheduled lifecycle", () => { worker = retainedNativeSessionCleanupWork(db, cleanup).register(coordinator, () => admitted); await worker.ready; } - async function flush() { + // `interval` is the real-time gap between polls. Each poll advances fake time + // by one second, which admits at most one cleanup pass (batch size 1, one + // second minimum pass interval), so a long backlog drains one entry per poll. + async function flush(interval = 50) { await vi.waitFor(async () => { await vi.advanceTimersByTimeAsync(1000); expect(idleWorkSnapshot().active).toBe(0); expect(coordinator!.nextWakeAt()).toBeNull(); - }, { timeout: 15_000 }); + }, { timeout: 15_000, interval }); } async function quiet(ms = 120_000) { const previous = db.$client.options.debug; @@ -614,7 +617,12 @@ describe("retained native cleanup scheduled lifecycle", () => { it("drains a backlog across discovery pages and becomes quiet with the completed history retained", async () => { const runIds: string[] = []; for (let index = 0; index < 103; index++) runIds.push(await candidate()); - await start(); await flush(); + // 103 serial passes at the default 50ms poll spend ~4.7s of a ~5s local run + // waiting between polls, and contended CI runners pushed it past 15s. A 5ms + // poll drains the same passes in under a second; the wake schedule is set + // before a pass releases its idle-work hold, so polling faster cannot + // observe a false quiet state. + await start(); await flush(5); expect(cleanup.mock.calls.map(([input]) => input.runId).sort()).toEqual(runIds.sort()); expect(await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.companyId, companyId))).toHaveLength(103); await quiet(); diff --git a/server/src/adapters/cloud-external-adapters.test.ts b/server/src/adapters/cloud-external-adapters.test.ts index 21da3bccbeb..637f952c60e 100644 --- a/server/src/adapters/cloud-external-adapters.test.ts +++ b/server/src/adapters/cloud-external-adapters.test.ts @@ -1,7 +1,7 @@ import fs from "node:fs"; 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"; let home: string; let localPackage: string; @@ -35,6 +35,29 @@ function writePackage(packageDir: string, type: string): void { fs.writeFileSync(path.join(packageDir, "ui-parser.mjs"), parserSource); } +// The registry imports every built-in adapter. The first import in a worker +// pays the one-time transform of that whole graph: about 15s on contended CI +// runners (the first case timed out at 15s while the next case took 1.1s) and +// 20-30s on a loaded local machine. Pay it here, not inside the first case's +// budget. Each case still gets fresh module state from vi.resetModules() below, +// and its re-import measured 0.4-1.5s. The warm-up uses an empty home and a +// managed signal, so it reads and runs no persisted adapter package. The 90s +// limit is three times the slowest measured cold import. +beforeAll(async () => { + const warmHome = fs.mkdtempSync(path.join(os.tmpdir(), "paperclip-cloud-adapters-warm-")); + vi.stubEnv("PAPERCLIP_HOME", warmHome); + vi.stubEnv("PAPERCLIP_MANAGED_CONFIG", ""); + try { + const registry = await import("./registry.js"); + await registry.waitForExternalAdapters(); + await import("./plugin-loader.js"); + await import("../services/adapter-plugin-store.js"); + } finally { + vi.unstubAllEnvs(); + fs.rmSync(warmHome, { recursive: true, force: true }); + } +}, 90_000); + beforeEach(() => { vi.resetModules(); vi.stubEnv("PAPERCLIP_CLOUD_TENANT_SERVER_TOKEN", undefined);