mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
test(server): remove two timing flakes in cloud adapter and native cleanup tests (#19668)
## 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
f4b19e4231
commit
55244f1e9c
2 files changed
+35
-4
No files matched your search
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in new issue
Block a user