Skip to content

Commit 547d85a

Browse files
mattrbeckatscott
mattrbeck
authored and
atscott
committed
ci: run benchmark comparison in isolated worktree and harden security
- Run comparison benchmark in an isolated git worktree to prevent workspace pollution and local branch conflicts. - Harden security by passing benchmark target and SHA as environment variables to prevent shell injection, and adding '--' to bazel query and git rev-parse. - Optimize workflow by removing pnpm caching to mitigate cache poisoning risks. - Improve robustness of benchmark log parsing, supporting both ZIP outputs and raw directories, and safely checking for JSON reports. - Centralize git command execution on the dev-infra GitClient for consistency. - Add tslib to benchpress dependencies to prevent module resolution failures.
1 parent cd771d3 commit 547d85a

7 files changed

Lines changed: 201 additions & 94 deletions

File tree

‎.github/workflows/benchmark-compare.yml‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,17 +39,19 @@ jobs:
3939
# We cannot use `angular/dev-infra/github-actions/npm/checkout-and-setup-node` here
4040
# because it does not support checking out from a fork (as it lacks a `repository` input).
4141
# Thus, we checkout and setup Node/pnpm manually.
42-
- name: Install pnpm
43-
uses: pnpm/action-setup@0e279bb959325dab635dd2c09392533439d90093 # v6.0.8
44-
4542
- name: Setup Node.js
4643
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
4744
with:
4845
node-version-file: '.nvmrc'
49-
cache: 'pnpm'
46+
47+
- name: Install pnpm
48+
uses: pnpm/action-setup@0e279bb959325dab635dd2c09392533439d90093 # v6.0.8
5049

5150
- run: pnpm install --frozen-lockfile
5251

52+
- name: Setup Bazel
53+
uses: angular/dev-infra/github-actions/bazel/setup@442c2fcbf06a321b5196b4c5fc70e78a49242958
54+
5355
- uses: angular/dev-infra/github-actions/bazel/configure-remote@442c2fcbf06a321b5196b4c5fc70e78a49242958
5456
with:
5557
bazelrc: ./.bazelrc.user
@@ -61,10 +63,12 @@ jobs:
6163
COMMENT_BODY: ${{ github.event.comment.body }}
6264
run: pnpm benchmarks prepare-for-github-action "$COMMENT_BODY"
6365

64-
- run: pnpm benchmarks run-compare ${{steps.info.outputs.compareSha}} "${{steps.info.outputs.benchmarkTarget}}"
65-
66+
- run: pnpm benchmarks run-compare "$COMPARE_SHA" "$BENCHMARK_TARGET"
6667
id: benchmark
6768
name: Running benchmark
69+
env:
70+
BENCHMARK_TARGET: ${{steps.info.outputs.benchmarkTarget}}
71+
COMPARE_SHA: ${{steps.info.outputs.compareSha}}
6872

6973
- uses: peter-evans/create-or-update-comment@e8674b075228eee787fea43ef493e45ece1004c9 # v5
7074
with:

‎packages/benchpress/package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44
"description": "Benchpress - a framework for e2e performance tests",
55
"dependencies": {
66
"@angular/core": "^22.0.0-next",
7-
"reflect-metadata": "^0.2.0"
7+
"reflect-metadata": "^0.2.0",
8+
"tslib": "^2.3.0"
89
},
910
"repository": {
1011
"type": "git",

‎pnpm-lock.yaml‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎scripts/benchmarks/index.mts‎

Lines changed: 91 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import fs from 'fs';
10+
import path from 'path';
911
import {setOutput} from '@actions/core';
1012
import {GitClient, Log, bold, green, yellow} from '@angular/ng-dev';
1113
import {select} from '@inquirer/prompts';
@@ -17,7 +19,7 @@ import {
1719
getTestlogPath,
1820
resolveTarget,
1921
} from './targets.mts';
20-
import {exec} from './utils.mts';
22+
import {exec, projectDir} from './utils.mts';
2123

2224
const benchmarkTestFlags = [
2325
'--cache_test_results=no',
@@ -27,6 +29,9 @@ const benchmarkTestFlags = [
2729
// reduce fluctuation. Output streamed ensures that deps can build with RBE, but
2830
// tests run locally while also providing useful output for debugging.
2931
'--test_output=streamed',
32+
// In the comparison run, we create a hybrid workspace (main files + PR scripts/lockfiles).
33+
// This causes a lockfile mismatch, so we must allow Bazel to update the lockfile in memory.
34+
'--lockfile_mode=update',
3035
];
3136

3237
await yargs(process.argv.slice(2))
@@ -98,11 +103,11 @@ async function prepareForGitHubAction(commentBody: string): Promise<void> {
98103
// Attempt to find the compare SHA. The commit may be either part of the
99104
// pull request, or might be a commit unrelated to the PR- but part of the
100105
// upstream repository. We attempt to fetch/resolve the SHA in both remotes.
101-
const compareRefResolve = git.runGraceful(['rev-parse', compareRefRaw]);
106+
const compareRefResolve = git.runGraceful(['rev-parse', '--', compareRefRaw]);
102107
let compareRefSha = compareRefResolve.stdout.trim();
103108
if (compareRefSha === '' || compareRefResolve.status !== 0) {
104109
git.run(['fetch', '--depth=1', git.getRepoGitUrl(), compareRefRaw]);
105-
compareRefSha = git.run(['rev-parse', 'FETCH_HEAD']).stdout.trim();
110+
compareRefSha = git.run(['rev-parse', '--', 'FETCH_HEAD']).stdout.trim();
106111
}
107112

108113
setOutput('compareSha', compareRefSha);
@@ -126,8 +131,8 @@ async function runBenchmarkCmd(bazelTargetRaw: string | undefined): Promise<void
126131
}
127132

128133
/** Runs a benchmark Bazel target. */
129-
async function runBenchmarkTarget(bazelTarget: ResolvedTarget): Promise<void> {
130-
await exec('bazel', ['test', bazelTarget, ...benchmarkTestFlags]);
134+
async function runBenchmarkTarget(bazelTarget: ResolvedTarget, cwd?: string): Promise<void> {
135+
await exec('pnpm', ['bazel', 'test', bazelTarget, ...benchmarkTestFlags], cwd);
131136
}
132137

133138
/**
@@ -138,13 +143,6 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string
138143
const git = await GitClient.get();
139144
const currentRef = git.getCurrentBranchOrRevision();
140145

141-
if (git.hasUncommittedChanges()) {
142-
Log.warn(bold('You have uncommitted changes.'));
143-
Log.warn('The script will stash your changes and re-apply them so that');
144-
Log.warn('the comparison ref can be checked out.');
145-
Log.warn('');
146-
}
147-
148146
if (bazelTargetRaw === undefined) {
149147
bazelTargetRaw = await promptForBenchmarkTarget();
150148
}
@@ -159,29 +157,92 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string
159157

160158
const workingDirResults = await collectBenchmarkResults(testlogPath);
161159

162-
// Stash working directory as we might be in the middle of developing
163-
// and we wouldn't want to discard changes when checking out the compare SHA.
164-
git.run(['stash']);
160+
// Define isolated temporary workspace inside `dist/` so it is ignored by git.
161+
const tempDir = path.join(projectDir, 'dist/benchmark-compare-temp');
162+
let comparisonResults: any = null;
165163

166164
try {
167-
Log.log(green('Fetching comparison revision.'));
168-
// Note: Not using a shallow fetch here as that would convert the local
169-
// user repository into an incomplete repository.
170-
git.run(['fetch', git.getRepoGitUrl(), compareRef]);
171-
Log.log(green('Checking out comparison revision.'));
172-
git.run(['checkout', 'FETCH_HEAD']);
173-
174-
await exec('pnpm', ['install', '--frozen-lockfile']);
175-
await runBenchmarkTarget(bazelTarget);
165+
Log.log(green(`Creating isolated workspace in ${tempDir}`));
166+
try {
167+
git.run(['worktree', 'remove', '--force', tempDir]);
168+
} catch (e) {
169+
if (fs.existsSync(tempDir)) {
170+
fs.rmSync(tempDir, {recursive: true, force: true});
171+
}
172+
try {
173+
git.run(['worktree', 'prune']);
174+
} catch (pruneError) {
175+
// Ignore prune errors
176+
}
177+
}
178+
179+
// Ensure the comparison ref is fetched on the main repository if not already present.
180+
const hasCommit = git.runGraceful(['cat-file', '-e', `${compareRef}^{commit}`]).status === 0;
181+
if (!hasCommit) {
182+
Log.log(green(`Fetching comparison revision ${compareRef}...`));
183+
git.run(['fetch', git.getRepoGitUrl(), compareRef]);
184+
} else {
185+
Log.log(
186+
green(`Comparison revision ${compareRef} is already available locally. Skipping fetch.`),
187+
);
188+
}
189+
190+
// Create isolated workspace instantly using native git worktree.
191+
Log.log(green(`Creating isolated worktree for ${compareRef} in ${tempDir}`));
192+
git.run(['worktree', 'add', '--detach', tempDir, compareRef]);
193+
194+
// Copy the current PR's benchmark scripts and packages into the isolated workspace.
195+
// Explicitly exclude node_modules to avoid copying broken relative symlinks.
196+
Log.log(green('Copying PR benchmark scripts and packages into isolated workspace...'));
197+
const dirsToCopy = ['scripts/benchmarks', 'packages/benchpress'];
198+
for (const relDir of dirsToCopy) {
199+
const src = path.join(projectDir, relDir);
200+
const dest = path.join(tempDir, relDir);
201+
fs.rmSync(dest, {recursive: true, force: true});
202+
fs.cpSync(src, dest, {
203+
recursive: true,
204+
filter: (srcPath) => !srcPath.split(path.sep).includes('node_modules'),
205+
});
206+
}
207+
208+
// Copy `.bazelrc.user` if it exists, otherwise create it.
209+
const bazelrcUser = path.join(projectDir, '.bazelrc.user');
210+
const tempBazelrcUser = path.join(tempDir, '.bazelrc.user');
211+
if (fs.existsSync(bazelrcUser)) {
212+
fs.copyFileSync(bazelrcUser, tempBazelrcUser);
213+
} else {
214+
fs.writeFileSync(tempBazelrcUser, '');
215+
}
216+
217+
// Run pnpm install inside the isolated workspace.
218+
Log.log(green('Installing dependencies in isolated workspace...'));
219+
await exec('pnpm', ['install', '--no-frozen-lockfile', '--prefer-offline'], tempDir);
220+
221+
// Run the benchmark on the comparison workspace.
222+
Log.log(green('Running benchmark in isolated workspace...'));
223+
await runBenchmarkTarget(bazelTarget, tempDir);
224+
225+
// Resolve testlog path and collect results from the isolated workspace.
226+
Log.log(green('Collecting comparison results...'));
227+
const tempTestlogPath = await getTestlogPath(bazelTarget, tempDir);
228+
comparisonResults = await collectBenchmarkResults(tempTestlogPath);
176229
} finally {
177-
restoreWorkingStage(git, currentRef);
230+
Log.log(green('Cleaning up isolated workspace...'));
231+
try {
232+
git.run(['worktree', 'remove', '--force', tempDir]);
233+
} catch (e) {
234+
Log.warn(`Failed to clean up isolated worktree: ${e}`);
235+
if (fs.existsSync(tempDir)) {
236+
fs.rmSync(tempDir, {recursive: true, force: true});
237+
}
238+
try {
239+
git.run(['worktree', 'prune']);
240+
} catch (pruneError) {
241+
// Ignore prune errors
242+
}
243+
}
178244
}
179245

180-
// Re-install dependencies for `HEAD`.
181-
await exec('pnpm', ['install', '--frozen-lockfile']);
182-
183-
const comparisonResults = await collectBenchmarkResults(testlogPath);
184-
185246
// If we are running in a GitHub action, expose the benchmark text
186247
// results as outputs. Useful if those are exposed as a GitHub comment then.
187248
if (process.env.GITHUB_ACTION !== undefined) {
@@ -198,11 +259,3 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string
198259
Log.info(bold(yellow(`Working stage (${currentRef}) results:`)), '\n');
199260
Log.info(workingDirResults.summaryConsoleText);
200261
}
201-
202-
function restoreWorkingStage(git: GitClient, initialRef: string) {
203-
Log.log(green('Restoring working stage'));
204-
git.run(['checkout', '-f', initialRef]);
205-
206-
// Stash apply could fail if there were not changes in the working stage.
207-
git.runGraceful(['stash', 'apply']);
208-
}

‎scripts/benchmarks/results.mts‎

Lines changed: 68 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import fs from 'fs';
910
import path from 'path';
1011
import Zip from 'adm-zip';
1112

@@ -16,6 +17,7 @@ interface JsonReport {
1617
metricsText: string;
1718
statsText: string;
1819
validSampleTexts: string[];
20+
completeSample?: any;
1921
}
2022

2123
/** Results of an individual benchmark scenario. */
@@ -38,31 +40,80 @@ export interface OverallResult {
3840

3941
/** Collects and parses the benchmark results of the given Bazel target testlog directory. */
4042
export function collectBenchmarkResults(testlogDir: string): OverallResult {
41-
const z = new Zip(path.join(testlogDir, 'test.outputs/outputs.zip'));
4243
const scenarioResults: ScenarioResult[] = [];
44+
const zipPath = path.join(testlogDir, 'test.outputs/outputs.zip');
4345

44-
for (const e of z.getEntries()) {
45-
if (path.extname(e.entryName) !== '.json') {
46-
continue;
46+
if (fs.existsSync(zipPath)) {
47+
const z = new Zip(zipPath);
48+
for (const e of z.getEntries()) {
49+
if (path.extname(e.entryName) !== '.json') {
50+
continue;
51+
}
52+
53+
try {
54+
const data = JSON.parse(z.readAsText(e.entryName));
55+
if (isJsonReport(data)) {
56+
addScenarioResult(data, scenarioResults);
57+
}
58+
} catch (err) {
59+
// Skip files that fail to parse
60+
}
4761
}
62+
} else {
63+
const outputsDir = path.join(testlogDir, 'test.outputs');
64+
if (fs.existsSync(outputsDir)) {
65+
for (const file of fs.readdirSync(outputsDir)) {
66+
if (path.extname(file) !== '.json') {
67+
continue;
68+
}
69+
70+
const filePath = path.join(outputsDir, file);
71+
if (!fs.statSync(filePath).isFile()) {
72+
continue;
73+
}
74+
75+
let data;
76+
try {
77+
data = JSON.parse(fs.readFileSync(filePath, 'utf-8'));
78+
} catch (e) {
79+
continue;
80+
}
4881

49-
const data = JSON.parse(z.readAsText(e.entryName));
82+
if (!isJsonReport(data)) {
83+
continue;
84+
}
5085

51-
// Skip files that do not look like benchpress reports.
52-
if (!isJsonReport(data)) {
53-
continue;
86+
addScenarioResult(data, scenarioResults);
87+
}
5488
}
89+
}
90+
91+
if (scenarioResults.length === 0) {
92+
throw new Error(`No valid benchpress benchmark reports found in "${testlogDir}".`);
93+
}
94+
95+
return {
96+
scenarios: scenarioResults,
97+
summaryConsoleText: scenarioResults
98+
.map((s) => `${bold(s.id)}\n\n${s.summaryConsoleText}`)
99+
.join('\n\n'),
100+
summaryMarkdownText: scenarioResults
101+
.map((s) => `### ${s.id}\n\n${s.summaryMarkdownText}`)
102+
.join('\n\n'),
103+
};
104+
}
55105

56-
scenarioResults.push({
57-
id: data.description.id,
58-
data,
59-
// Output used for console output when running locally/CI.
60-
summaryConsoleText: `\
106+
function addScenarioResult(data: JsonReport, scenarioResults: ScenarioResult[]) {
107+
scenarioResults.push({
108+
id: data.description.id,
109+
data,
110+
// Output used for console output when running locally/CI.
111+
summaryConsoleText: `\
61112
${data.metricsText}
62113
${data.validSampleTexts.join('\n')}
63114
${data.statsText}`,
64-
// Output used for e.g. GitHub actions.
65-
summaryMarkdownText: `\
115+
// Output used for e.g. GitHub actions.
116+
summaryMarkdownText: `\
66117
<details><summary>Full example results</summary>
67118
68119
\`\`\`
@@ -77,21 +128,10 @@ ${data.statsText}
77128
${data.metricsText}
78129
${data.statsText}
79130
\`\`\``,
80-
});
81-
}
82-
83-
return {
84-
scenarios: scenarioResults,
85-
summaryConsoleText: scenarioResults
86-
.map((s) => `${bold(s.id)}\n\n${s.summaryConsoleText}`)
87-
.join('`\n'),
88-
summaryMarkdownText: scenarioResults
89-
.map((s) => `### ${s.id}\n\n${s.summaryMarkdownText}`)
90-
.join('`\n'),
91-
};
131+
});
92132
}
93133

94134
/** Whether the object corresponds to a benchpress JSON report. */
95135
function isJsonReport(data: any): data is JsonReport {
96-
return data['completeSample'] !== undefined;
136+
return data?.completeSample !== undefined;
97137
}

0 commit comments

Comments
 (0)