mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 11:01:23 +01:00
perf(ci): split the Runner package Vitest lane into three duration-balanced lanes (#19646)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Contributors wait for PR CI before merge, so CI wall-clock time sets the pace of work > - The Runner package Vitest lane is the PR CI critical path. It finishes a median 15.3 minutes after the push > - That lane runs all 311 package test files in one job with 2 workers: 1,381 seconds of serial work and 727 seconds of wall time > - Ten files account for 61% of the time. Vitest's `--shard` splits by file count, not by duration, so it cannot balance this work > - This pull request splits the package files into three lanes balanced by duration, and runs the native server group in its own lane > - The benefit is about 232 seconds of tests per package lane instead of 727 seconds ## Linked Issues or Issue Description **What existing behavior does this improve?** The time that PR CI needs to run the Runner package Vitest tests. **Subsystem affected** CI: the `Verify Paperclip Runner` Vitest lanes in `pr-trusted.yml`, and `packages/paperclip-runner/scripts/run-pr-vitest-lane.mjs`. **Current behavior** The `vitest package` lane takes a median of 13.6 minutes. It is the last job to finish in most PR runs. The lane keeps a 2-worker limit so that Runner and sidecar processes can start within their deadlines. Average CPU use is 29%. **Proposed behavior** Three package lanes each run about one third of the files, balanced by measured duration. Each lane keeps the 2-worker limit. The native server group runs in its own lane, as before. **Reason and benefit** The Runner lane stops being the critical path. More workers per lane would add CPU contention and timeouts, so this change adds lanes instead. The cost is about 3 runner-minutes per run for the extra setup. **Breaking changes** None. The `master` ruleset requires only `ci / verify` and `ci / e2e`, so the new job names do not change required checks. ## What Changed - Add `packages/paperclip-runner/scripts/vitest-package-lanes.mjs`. - It gets the file inventory from `vitest list --filesOnly --json`, so it follows `vitest.config.ts`. - It partitions the files with the `partitionGeneralServerSuites` function from `scripts/general-server-shard.mjs`. - Add `packages/paperclip-runner/scripts/vitest-lane-durations.json`. - It uses the same schema as `scripts/general-server-shard-durations.json`. - It is seeded from CI run 42290920882. It measures 310 of the 311 files. - `run-pr-vitest-lane.mjs` accepts two new arguments: - `--package-lane=K/N` runs one partition. - `--native-server` runs only the native server group. - The `--shard=1/2` and `--shard=2/2` arguments keep their exact behavior for the matrix that master still sends. - `.github/ci-task-matrix.json` and the fallback matrix in `pr-trusted.yml` have three `vitest package (K/3)` lanes and the `vitest native server` lane. The labels keep the `vitest` prefix because `scripts/ci-profile/trace.mjs` matches `Verify Paperclip Runner (vitest`. - Tests: - the lanes cover the inventory exactly once, checked with real `vitest list`; - the old `--shard` commands plan the same commands as before; - the manifest schema is correct; - bad lane arguments are refused. - Update the Runner Vitest lane paragraph in `doc/DEVELOPING.md`. ## Verification - `node --test scripts/__tests__/run-vitest-stable-shard.test.mjs`: 44/44 pass. - `node --test scripts/__tests__/runner-release-crate-cache.test.mjs .github/scripts/tests/ci-plan.test.mjs`, together with the file above: 71/71 pass. - Related workflow, cache, routing and profile tests: 446/446, 153/153 and 14/14 pass. - Local end-to-end run of `GITHUB_WORKFLOW=PR node scripts/run-pr-vitest-lane.mjs --package-lane=3/3` on macOS: - the prep steps ran, and the lane ran 104 of 311 files in 123 seconds; - 2 tests failed: a macOS `kill EPERM` and a 5-second timeout under full local load without the CI worker limit; - both files pass when run alone (35/35 tests). - This PR's own CI runs the matrix from master, so it uses the old `--shard` commands. The new lanes run in CI only after merge. Reviewers can compare the job durations of the first PR runs after the merge. | Lane | Files | Serial estimate | Wall estimate with 2 workers | | --- | --- | --- | --- | | 1/3 | 103 | 464.9 s | ~232 s | | 2/3 | 104 | 465.0 s | ~232 s | | 3/3 | 104 | 465.0 s | ~232 s | ## Risks - The weights come from one CI log and do not include collection or setup time. The balance can be uneven until a refresh measures real lane runs. - Each lane passes its file paths to Vitest as filters, and Vitest matches a filter as a substring of the path. A path that is a substring of another path would run a file in two lanes. No current path does this. If one does, the script prints a warning, and no file is dropped. - Each package lane runs `ensure:eval-build-deps` and `build:rust` again, which adds about 3 runner-minutes per run. - A comment in `scripts/run-vitest-stable.mjs` (about line 110) still says that the last Runner shard runs the native group. A follow-up will correct it. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use and code execution. A Claude sub-agent wrote the change, and the parent session reviewed it and ran the local lane check. ## 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
2e340d6235
commit
a2aa69bc62
10 files changed
+655
-36
No files matched your search
@@ -439,28 +439,34 @@ test("shard flags are rejected for the native-runner group", () => {
|
||||
|
||||
// The PR-side exclusion above is safe only while the wiring it assumes holds:
|
||||
// pr.yml (the caller whose name reusable pr-trusted.yml jobs see as
|
||||
// GITHUB_WORKFLOW) is named PR, the sharded vitest lanes partition cleanly
|
||||
// with exactly one final shard, and that lane's package script routes through
|
||||
// the wrapper that runs the native-runner group.
|
||||
// GITHUB_WORKFLOW) is named PR, the package vitest lanes cover every
|
||||
// partition exactly once, exactly one lane runs the native-runner group, and
|
||||
// those lanes' package script routes through the wrapper.
|
||||
test("the PR workflow wiring for the native-runner lane holds", () => {
|
||||
const prWorkflow = readFileSync(path.join(repoRoot, ".github/workflows/pr.yml"), "utf8");
|
||||
assert.match(prWorkflow, /^name: PR$/m,
|
||||
"renaming pr.yml silently moves the native-runner suite back into the uncached server shards");
|
||||
|
||||
const matrix = JSON.parse(readFileSync(path.join(repoRoot, ".github/ci-task-matrix.json"), "utf8"));
|
||||
const lanes = matrix.verify_paperclip_runner
|
||||
.map((entry) => /test:typescript:vitest --shard=(\d+)\/(\d+)/.exec(entry.command))
|
||||
const vitestLanes = matrix.verify_paperclip_runner.filter((entry) => entry.command.startsWith("test:typescript:vitest"));
|
||||
for (const entry of vitestLanes) {
|
||||
assert.match(entry.command, /^test:typescript:vitest (--package-lane=\d+\/\d+|--native-server)$/, entry.lane_label);
|
||||
assert.equal(entry.task, "ci:runner-vitest", entry.lane_label);
|
||||
}
|
||||
const lanes = vitestLanes
|
||||
.map((entry) => /--package-lane=(\d+)\/(\d+)$/.exec(entry.command))
|
||||
.filter(Boolean).map((match) => [Number(match[1]), Number(match[2])]);
|
||||
assert.ok(lanes.length > 0, "expected sharded test:typescript:vitest lanes in pr-trusted.yml");
|
||||
assert.equal(new Set(lanes.map(([, count]) => count)).size, 1, "vitest lanes must agree on the shard count");
|
||||
const shardCount = lanes[0][1];
|
||||
assert.ok(lanes.length > 0, "expected package-lane test:typescript:vitest lanes in the task matrix");
|
||||
assert.equal(new Set(lanes.map(([, count]) => count)).size, 1, "package lanes must agree on the lane count");
|
||||
const laneCount = lanes[0][1];
|
||||
assert.deepEqual(
|
||||
lanes.map(([index]) => index).sort((left, right) => left - right),
|
||||
Array.from({ length: shardCount }, (_, index) => index + 1),
|
||||
"vitest lanes must cover every shard exactly once",
|
||||
Array.from({ length: laneCount }, (_, index) => index + 1),
|
||||
"package lanes must cover every partition exactly once",
|
||||
);
|
||||
assert.equal(lanes.filter(([index, count]) => index === count).length, 1,
|
||||
"exactly one final vitest shard carries the native-runner group");
|
||||
const native = vitestLanes.filter((entry) => entry.command.endsWith("--native-server"));
|
||||
assert.equal(native.length, 1, "exactly one vitest lane carries the native-runner group");
|
||||
assert.equal(native[0].release_crate, true, "the native-runner lane needs the release crate");
|
||||
|
||||
const runnerPackage = JSON.parse(
|
||||
readFileSync(path.join(repoRoot, "packages/paperclip-runner/package.json"), "utf8"),
|
||||
@@ -508,6 +514,71 @@ test("the PR lanes separate complete package and native workloads without changi
|
||||
assert.notEqual(malformed.status, 0, "a malformed shard flag must fail rather than guess a lane");
|
||||
});
|
||||
|
||||
const runnerRoot = path.join(repoRoot, "packages/paperclip-runner");
|
||||
const prepCommands = [
|
||||
{ command: "pnpm", args: ["run", "ensure:eval-build-deps"], cwd: runnerRoot },
|
||||
{ command: "pnpm", args: ["run", "build:rust"], cwd: runnerRoot },
|
||||
];
|
||||
const nativeCommand = {
|
||||
command: "pnpm",
|
||||
args: ["test:run:general", "--", "--group", "general-server-native-runner"],
|
||||
cwd: repoRoot,
|
||||
};
|
||||
|
||||
// Master's trusted workflow keeps sending the --shard commands until the
|
||||
// package-lane matrix merges, so their exact plans must not move.
|
||||
test("the master-pinned --shard lane commands plan exactly what they did before package lanes", () => {
|
||||
assert.deepEqual(wrapperPlan(["--shard=1/2"], { GITHUB_WORKFLOW: "PR" }).plannedCommands, [
|
||||
...prepCommands,
|
||||
{ command: "pnpm", args: ["exec", "vitest", "run"], cwd: runnerRoot },
|
||||
]);
|
||||
assert.deepEqual(wrapperPlan(["--shard=2/2"], { GITHUB_WORKFLOW: "PR" }).plannedCommands, [...prepCommands, nativeCommand]);
|
||||
for (const plan of [wrapperPlan(["--shard=1/2"], { GITHUB_WORKFLOW: "PR" }), wrapperPlan(["--shard=2/2"], { GITHUB_WORKFLOW: "PR" })]) {
|
||||
assert.equal(plan.packageLane, undefined);
|
||||
}
|
||||
});
|
||||
|
||||
test("--native-server runs only the native group under any workflow", () => {
|
||||
for (const envOverrides of [{ GITHUB_WORKFLOW: "PR" }, {}]) {
|
||||
const plan = wrapperPlan(["--native-server"], envOverrides);
|
||||
assert.deepEqual(plan.plannedCommands, [...prepCommands, nativeCommand]);
|
||||
assert.equal(plan.runPackageTests, false);
|
||||
assert.equal(plan.runNativeRunnerGroup, true);
|
||||
}
|
||||
});
|
||||
|
||||
test("the package lane manifest uses the general server duration schema", () => {
|
||||
const manifest = JSON.parse(readFileSync(path.join(runnerRoot, "scripts/vitest-lane-durations.json"), "utf8"));
|
||||
const general = JSON.parse(readFileSync(durationsManifest, "utf8"));
|
||||
// The refresh script writes exactly these keys; hand-added annotations such
|
||||
// as the server manifest's splitEstimates are optional.
|
||||
assert.deepEqual(Object.keys(manifest), ["$comment", "unit", "sources", "coverage", "durations"]);
|
||||
assert.ok(Object.keys(manifest).every((key) => key in general));
|
||||
assert.equal(manifest.unit, "ms");
|
||||
assert.deepEqual(Object.keys(manifest.coverage), Object.keys(general.coverage));
|
||||
for (const source of manifest.sources) {
|
||||
assert.deepEqual(Object.keys(source), ["run", "attempt", "sha", "url"]);
|
||||
assert.match(source.sha, /^[a-f0-9]{40}$/);
|
||||
}
|
||||
const entries = Object.entries(manifest.durations);
|
||||
assert.ok(entries.length > 100);
|
||||
for (const [file, ms] of entries) {
|
||||
assert.match(file, /^packages\/paperclip-runner\/.+\.test\.[cm]?[jt]sx?$/);
|
||||
assert.ok(Number.isFinite(ms) && ms >= 0, file);
|
||||
}
|
||||
});
|
||||
|
||||
test("package lane arguments fail rather than guess a lane", () => {
|
||||
for (const args of [["--package-lane=0/3"], ["--package-lane=4/3"], ["--package-lane=abc"], ["--package-lane", "1/3"],
|
||||
["--package-lane=1/3", "--package-lane=2/3"], ["--package-lane=1/3", "--shard=1/2"], ["--native-server", "--shard=2/2"],
|
||||
["--native-server", "--package-lane=1/3"], ["--native-server", "src/foo.test.ts"]]) {
|
||||
const result = spawnSync(process.execPath, [laneWrapper, ...args, "--dry-run"], {
|
||||
cwd: repoRoot, encoding: "utf8", env: workflowEnv({ GITHUB_WORKFLOW: "PR" }),
|
||||
});
|
||||
assert.notEqual(result.status, 0, args.join(" "));
|
||||
}
|
||||
});
|
||||
|
||||
const lineShardFile = path.join(repoRoot, "server/src/__tests__/chat-channels.integration.test.ts");
|
||||
const caseAt = (line, name) => ({ name, file: lineShardFile, projectName: "@paperclipai/server", location: { line, column: 3 } });
|
||||
|
||||
|
||||
@@ -361,7 +361,9 @@ test("the Verify Paperclip Runner job restores the crate only in the lanes that
|
||||
assert.deepEqual(lanes, [
|
||||
["static checks", false],
|
||||
["rust", true],
|
||||
["vitest package", false],
|
||||
["vitest package (1/3)", false],
|
||||
["vitest package (2/3)", false],
|
||||
["vitest package (3/3)", false],
|
||||
["vitest native server", true],
|
||||
]);
|
||||
for (const stepName of [KEY_STEP, RESTORE_STEP]) {
|
||||
|
||||
@@ -0,0 +1,74 @@
|
||||
// Runs real Vitest against the Runner package, so it needs installed workspace
|
||||
// dependencies. The `policy` job runs run-vitest-stable-shard.test.mjs without
|
||||
// them; this file runs in `test:release-registry` (Typecheck + Release Registry),
|
||||
// which installs dependencies first.
|
||||
import assert from "node:assert/strict";
|
||||
import { spawnSync } from "node:child_process";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import test from "node:test";
|
||||
|
||||
import { defaultSuiteWeight, loadShardDurations } from "../general-server-shard.mjs";
|
||||
|
||||
const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", "..");
|
||||
const runnerRoot = path.join(repoRoot, "packages/paperclip-runner");
|
||||
const laneWrapper = path.join(runnerRoot, "scripts/run-pr-vitest-lane.mjs");
|
||||
const prepCommands = [
|
||||
{ command: "pnpm", args: ["run", "ensure:eval-build-deps"], cwd: runnerRoot },
|
||||
{ command: "pnpm", args: ["run", "build:rust"], cwd: runnerRoot },
|
||||
];
|
||||
|
||||
function wrapperPlan(args, envOverrides = {}) {
|
||||
const result = spawnSync(process.execPath, [laneWrapper, ...args, "--dry-run"], {
|
||||
cwd: repoRoot,
|
||||
encoding: "utf8",
|
||||
env: { ...process.env, ...envOverrides },
|
||||
});
|
||||
assert.equal(result.status, 0, `expected wrapper dry run to succeed for ${args.join(" ")}: ${result.stderr}`);
|
||||
return JSON.parse(result.stdout);
|
||||
}
|
||||
|
||||
// Asks Vitest itself which files the given CLI filters select, so both the
|
||||
// inventory and the lanes' substring filters follow vitest.config.ts exactly.
|
||||
function vitestListFiles(filters = []) {
|
||||
const listed = spawnSync(process.execPath, [path.join(runnerRoot, "node_modules/vitest/vitest.mjs"), "list", "--filesOnly", ...filters], {
|
||||
cwd: runnerRoot, encoding: "utf8", env: { ...process.env, PAPERCLIP_CI_PROFILE: "" }, maxBuffer: 16 * 1024 * 1024,
|
||||
});
|
||||
assert.equal(listed.status, 0, listed.stderr);
|
||||
return listed.stdout.split("\n").map((line) => line.trim().replaceAll("\\", "/"))
|
||||
.filter((line) => /\.test\.[cm]?[jt]sx?$/.test(line)).sort();
|
||||
}
|
||||
|
||||
test("package lanes form a complete, non-overlapping, balanced cover of the Vitest inventory", () => {
|
||||
const inventory = vitestListFiles();
|
||||
assert.ok(inventory.length > 100, `expected the full Runner package inventory, got ${inventory.length}`);
|
||||
|
||||
const durations = loadShardDurations(path.join(runnerRoot, "scripts/vitest-lane-durations.json"));
|
||||
const weight = (file) => durations[`packages/paperclip-runner/${file}`] ?? defaultSuiteWeight(durations);
|
||||
const laneCount = 3;
|
||||
const lanes = Array.from({ length: laneCount }, (_, index) => {
|
||||
const plan = wrapperPlan([`--package-lane=${index + 1}/${laneCount}`], { GITHUB_WORKFLOW: "PR" });
|
||||
assert.equal(plan.runNativeRunnerGroup, false);
|
||||
assert.deepEqual(plan.plannedCommands.slice(0, 2), prepCommands, "every package lane prepares debug fixtures");
|
||||
assert.ok(!plan.plannedCommands.some((command) => command.args.includes("general-server-native-runner")));
|
||||
assert.equal(plan.packageLane.inventory, inventory.length);
|
||||
const vitest = plan.plannedCommands.filter((command) => command.args[0] === "exec");
|
||||
assert.equal(vitest.length, 1);
|
||||
assert.deepEqual(vitest[0].args, ["exec", "vitest", "run", ...plan.packageLane.files]);
|
||||
assert.ok(plan.packageLane.files.length > 0, "an empty filter list would run the whole inventory");
|
||||
return plan.packageLane;
|
||||
});
|
||||
|
||||
const assigned = lanes.flatMap((lane) => lane.files);
|
||||
assert.equal(new Set(assigned).size, assigned.length, "a file is assigned to more than one lane");
|
||||
assert.deepEqual([...assigned].sort(), inventory, "the lanes must cover the inventory exactly");
|
||||
for (const lane of lanes) {
|
||||
// Vitest filters are substring matches; no lane may also select another lane's files.
|
||||
assert.deepEqual(vitestListFiles(lane.files), [...lane.files].sort(), `lane ${lane.index} filters`);
|
||||
assert.equal(lane.estimatedMs, lane.files.reduce((total, file) => total + weight(file), 0));
|
||||
}
|
||||
// Longest-processing-time bound: no lane exceeds the mean by more than the heaviest file.
|
||||
const total = inventory.reduce((sum, file) => sum + weight(file), 0);
|
||||
const heaviest = Math.max(...inventory.map(weight));
|
||||
for (const lane of lanes) assert.ok(lane.estimatedMs <= total / laneCount + heaviest, `lane ${lane.index} is unbalanced`);
|
||||
});
|
||||
Reference in new issue
Block a user