From 2853a9ae69dd932e0b48a96436da5ff78edb2f95 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 19 Jun 2026 19:22:17 -0700 Subject: [PATCH] perf(ci): shard the general-server test lane across 3 runners (#8360) 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 > - Every PR runs the `PR` GitHub Actions workflow, whose `verify` gate fans out into parallel test lanes (general tests, serialized server route suites, build, typecheck) > - The `General tests (server)` lane had grown into the run's critical path: it executed all ~213 non-route server suites serially in a single job (~7.2m of test time), more than 2x any other job > - It runs serially because `server/vitest.config.ts` pins `maxWorkers: 1`, so server suites cannot parallelize within a single runner — the only lever is spreading them across runners > - This pull request shards that lane into 3 even partitions that run on separate runners, mirroring the 4-way sharding already used for the serialized route suites > - The benefit is the lane drops from ~7.7m to ~2.4m/shard, cutting overall PR wall time roughly in half (~8.5m → ~4.2m) ## Linked Issues or Issue Description No public GitHub issue exists for this work, so the underlying issue is described inline following the feature-request template. ### Problem or motivation PR CI wall time had crept back up to ~8.5m. On a recent fully-green run, the `General tests (server)` job took 7.72m — more than double any other job and the clear critical path. Of that, 7.23m was pure test execution (dependency install was a cached 0.27m). The job ran all server suites that are not route/authz tests (213 files) one after another, because the server vitest project pins `maxWorkers: 1`, making these suites inherently serial within a single runner. ### Proposed solution Shard the general-server lane across 3 parallel runners — the same technique the route/authz suites already use — so the suite set is split into even, deterministic partitions that run concurrently. Add a regression test that proves the shards always cover the full suite set with no gaps or overlap. ### Alternatives considered - **Raise `maxWorkers` for the server project** to parallelize within one runner — rejected: the server suites share process-level state (DB/port), which is exactly why `maxWorkers: 1` is pinned. - **Two shards instead of three** — would leave the lane at ~3.6m, still above the next bottleneck (Canary Dry Run, ~4.1m wouldn't be the gate). Three lands the lane comfortably below it. - **Do nothing / accept the slow lane** — rejected: it gates every PR. ### Roadmap alignment Developer-experience / CI tooling. Not core product roadmap work; does not overlap with planned features in `ROADMAP.md`. ## What Changed - `scripts/run-vitest-stable.mjs`: the `general-server` general-test group now accepts `--shard-index` / `--shard-count`. It enumerates the full server test set (the whole `server/src` tree, minus the route/authz suites that already run in their own serialized shards) and splits it deterministically by modulo. The non-sharded local invocation (`pnpm test:run:general --group general-server`) is unchanged. - `.github/workflows/pr.yml`: the `general_tests` matrix runs `general-server` as 3 parallel shards (1/3, 2/3, 3/3). Workspace groups are unchanged. The `verify` gate already aggregates the whole matrix result, so the required check name is unaffected. - `scripts/__tests__/run-vitest-stable-shard.test.mjs`: a `node:test` suite asserting the 3 shards form a complete, non-overlapping partition of the general-server set, that no route/authz suite leaks into it, and that shard flags are rejected for the parallel workspace groups. Wired into the `policy` job. ## Verification - New partition test passes locally: `node --test ./scripts/__tests__/run-vitest-stable-shard.test.mjs` (3/3). - Confirmed the 3 shards form a complete, non-overlapping partition of all 213 files (71/71/71). - Ran a live thin shard (3 real server suites, including one outside `__tests__`) — 23 tests passed, confirming positional-include execution works end to end. - This PR's own CI is the authoritative check: all three `General tests (server (n/3))` jobs went green on the prior run, collectively covering every suite the old single job ran. ## Risks - Low risk. No product code changes — only test orchestration and CI matrix. Shard partitioning is deterministic and is now covered by an automated test that fails if the partition ever develops a gap or overlap. Modulo-on-sorted-filenames balances duration reasonably, matching the approach already proven by the serialized route shards. ## Model Used - Claude (Anthropic), `claude-opus-4-8`, extended thinking + tool use (agentic coding via Paperclip). ## 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 - [ ] If this change affects the UI, I have included before/after screenshots (N/A — no UI change) - [x] I have updated relevant documentation to reflect my changes (inline comments explain the sharding rationale) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (pending this PR's run) - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending review) - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- .github/workflows/pr.yml | 26 ++++++- .../run-vitest-stable-shard.test.mjs | 63 ++++++++++++++++ scripts/run-vitest-stable.mjs | 75 +++++++++++++++---- 3 files changed, 149 insertions(+), 15 deletions(-) create mode 100644 scripts/__tests__/run-vitest-stable-shard.test.mjs diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 8b74430b..f157e12c 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -55,6 +55,9 @@ jobs: - name: Test no-git-push check run: node --test ./scripts/check-no-git-push.test.mjs + - name: Test general-server shard partition + run: node --test ./scripts/__tests__/run-vitest-stable-shard.test.mjs + - name: Validate release package manifest run: node ./scripts/release-package-map.mjs check @@ -135,8 +138,21 @@ jobs: fail-fast: false matrix: include: + # The server suite is pinned to maxWorkers=1 (server/vitest.config.ts), + # so it can only be parallelized across runners. Shard it to keep this + # lane off the PR critical path. - group: general-server - group_label: server + group_label: server (1/3) + shard_index: 0 + shard_count: 3 + - group: general-server + group_label: server (2/3) + shard_index: 1 + shard_count: 3 + - group: general-server + group_label: server (3/3) + shard_index: 2 + shard_count: 3 - group: general-workspaces-a group_label: workspaces-a - group: general-workspaces-b @@ -168,7 +184,13 @@ jobs: run: pnpm install --frozen-lockfile - name: Run grouped general test suites - run: pnpm test:run:general -- --group '${{ matrix.group }}' + run: | + if [ -n "${{ matrix.shard_count }}" ]; then + pnpm test:run:general -- --group '${{ matrix.group }}' \ + --shard-index ${{ matrix.shard_index }} --shard-count ${{ matrix.shard_count }} + else + pnpm test:run:general -- --group '${{ matrix.group }}' + fi verify: # Preserve the legacy required-check name while the underlying work runs in parallel. diff --git a/scripts/__tests__/run-vitest-stable-shard.test.mjs b/scripts/__tests__/run-vitest-stable-shard.test.mjs new file mode 100644 index 00000000..889c5fb0 --- /dev/null +++ b/scripts/__tests__/run-vitest-stable-shard.test.mjs @@ -0,0 +1,63 @@ +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"; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", ".."); +const script = path.join(repoRoot, "scripts", "run-vitest-stable.mjs"); + +function dryRun(args) { + const result = spawnSync(process.execPath, [script, ...args, "--dry-run"], { + cwd: repoRoot, + encoding: "utf8", + }); + return result; +} + +function dryRunJson(args) { + const result = dryRun(args); + assert.equal(result.status, 0, `expected success for ${args.join(" ")}: ${result.stderr}`); + return JSON.parse(result.stdout); +} + +const SHARD_COUNT = 3; + +test("the general-server shards form a complete, non-overlapping partition", () => { + const shards = Array.from({ length: SHARD_COUNT }, (_, index) => + dryRunJson(["--mode", "general", "--group", "general-server", "--shard-index", String(index), "--shard-count", String(SHARD_COUNT)]), + ); + + const total = shards[0].generalServerSuiteCount; + assert.ok(total > 0, "expected a non-empty general-server suite set"); + + const seen = new Set(); + let selectedTotal = 0; + for (const shard of shards) { + assert.equal(shard.generalServerSuiteCount, total, "suite count must be stable across shards"); + for (const file of shard.selectedGeneralServerSuites) { + assert.ok(!seen.has(file), `suite assigned to more than one shard: ${file}`); + seen.add(file); + selectedTotal += 1; + } + } + + // Every suite runs exactly once: union covers the whole set with no overlap. + assert.equal(selectedTotal, total, "every suite must be selected exactly once"); + assert.equal(seen.size, total, "union of shards must cover the whole suite set"); +}); + +test("a route/authz suite never leaks into the general-server shards", () => { + const shard = dryRunJson(["--mode", "general", "--group", "general-server", "--shard-index", "0", "--shard-count", SHARD_COUNT.toString()]); + for (const file of shard.selectedGeneralServerSuites) { + assert.ok( + !/[^/]*(?:route|routes|authz)[^/]*\.test\.ts$/.test(file), + `route/authz suite must stay in the serialized lane, not general-server: ${file}`, + ); + } +}); + +test("shard flags are rejected for the parallel workspace groups", () => { + const result = dryRun(["--mode", "general", "--group", "general-workspaces-a", "--shard-index", "0", "--shard-count", "3"]); + assert.notEqual(result.status, 0, "workspace groups must not accept shard flags"); +}); diff --git a/scripts/run-vitest-stable.mjs b/scripts/run-vitest-stable.mjs index 117f5995..28a6888f 100644 --- a/scripts/run-vitest-stable.mjs +++ b/scripts/run-vitest-stable.mjs @@ -6,6 +6,7 @@ import path from "node:path"; const repoRoot = process.cwd(); const serverRoot = path.join(repoRoot, "server"); +const serverSrcDir = path.join(repoRoot, "server", "src"); const serverTestsDir = path.join(repoRoot, "server", "src", "__tests__"); const nonServerProjects = [ "@paperclipai/shared", @@ -198,8 +199,13 @@ function parseCliOptions(argv) { fail("--shard-index and --shard-count must be provided together."); } - if (mode !== serializedModeName && shardIndex !== null) { - fail("--shard-index/--shard-count are only valid with --mode serialized."); + const shardAllowed = + mode === serializedModeName || + (mode === generalModeName && group === generalServerGroupName); + if (!shardAllowed && shardIndex !== null) { + fail( + "--shard-index/--shard-count are only valid with --mode serialized or --mode general --group general-server.", + ); } if (group !== null && mode !== generalModeName) { @@ -210,17 +216,17 @@ function parseCliOptions(argv) { fail(`Unknown group "${group}". Expected one of: ${generalGroupNames.join(", ")}.`); } - if (mode === serializedModeName) { - const resolvedShardCount = shardCount ?? 1; - const resolvedShardIndex = shardIndex ?? 0; - if (resolvedShardIndex >= resolvedShardCount) { - fail(`--shard-index must be less than --shard-count. Received ${resolvedShardIndex} of ${resolvedShardCount}.`); + if (shardIndex !== null) { + if (shardIndex >= shardCount) { + fail(`--shard-index must be less than --shard-count. Received ${shardIndex} of ${shardCount}.`); } + } + if (mode === serializedModeName) { return { mode, - shardIndex: resolvedShardIndex, - shardCount: resolvedShardCount, + shardIndex: shardIndex ?? 0, + shardCount: shardCount ?? 1, group: null, dryRun, }; @@ -228,8 +234,8 @@ function parseCliOptions(argv) { return { mode, - shardIndex: null, - shardCount: null, + shardIndex, + shardCount, group, dryRun, }; @@ -280,8 +286,31 @@ function runProjectGroup(projects, groupName) { } } -function runGeneralGroup(routeTests, groupName) { +function runGeneralGroup(routeTests, groupName, shardIndex = null, shardCount = null) { if (groupName === generalServerGroupName) { + if (shardCount !== null && shardCount > 1) { + const shardFiles = generalServerTestFiles.filter( + (_, index) => index % shardCount === shardIndex, + ); + console.log( + `\n[test:run] general-server shard ${shardIndex + 1}/${shardCount} running ${shardFiles.length} of ${generalServerTestFiles.length} suites`, + ); + if (shardFiles.length === 0) { + return; + } + + runVitest( + [ + "--project", + "@paperclipai/server", + ...serializedServerVitestArgs, + ...shardFiles, + ], + `${groupName} shard ${shardIndex + 1}/${shardCount}`, + ); + return; + } + const excludeRouteArgs = routeTests.flatMap((file) => ["--exclude", file.serverPath]); runVitest( [ @@ -336,6 +365,17 @@ const routeTests = walk(serverTestsDir) })) .sort((a, b) => a.repoPath.localeCompare(b.repoPath)); +// Every server test file that the general-server group is responsible for, +// i.e. the whole server project minus the route/authz suites that run in the +// dedicated serialized shards. Sharding this list across runners is what keeps +// the general-server lane from becoming the PR critical path: the server vitest +// config pins maxWorkers to 1, so the only way to parallelize is across jobs. +const generalServerTestFiles = walk(serverSrcDir) + .map((file) => toRepoPath(file)) + .filter((repoPath) => repoPath.endsWith(".test.ts")) + .filter((repoPath) => !isRouteOrAuthzTest(repoPath)) + .sort((a, b) => a.localeCompare(b)); + const options = parseCliOptions(process.argv.slice(2)); if (options.dryRun) { const serializedSuites = @@ -352,6 +392,15 @@ if (options.dryRun) { availableGeneralGroups: generalGroupNames, serializedSuiteCount: routeTests.length, selectedSerializedSuites: serializedSuites.map((routeTest) => routeTest.repoPath), + generalServerSuiteCount: generalServerTestFiles.length, + selectedGeneralServerSuites: + options.mode === generalModeName && + options.group === generalServerGroupName && + options.shardCount !== null + ? generalServerTestFiles.filter( + (_, index) => index % options.shardCount === options.shardIndex, + ) + : null, }, null, 2, @@ -362,7 +411,7 @@ if (options.dryRun) { if (options.mode === generalModeName || options.mode === allModeName) { if (options.group) { - runGeneralGroup(routeTests, options.group); + runGeneralGroup(routeTests, options.group, options.shardIndex, options.shardCount); } else { runGeneralSuites(routeTests); }