Skip to content

support benchmark-action/github-action-benchmark#805

Open
paulk-asert wants to merge 1 commit intoapache:mainfrom
paulk-asert:addBenchmarkAction
Open

support benchmark-action/github-action-benchmark#805
paulk-asert wants to merge 1 commit intoapache:mainfrom
paulk-asert:addBenchmarkAction

Conversation

@paulk-asert
Copy link
Copy Markdown

@paulk-asert paulk-asert commented May 5, 2026

Why this action is needed for your project

Groovy wants to create a graphical summary of its jmh tests. We generate a lot of performance stats. It all lives within GitHub action artifacts. It is tedious to determine the overall result of what the performance tests show.

There also seems to be at least one other project that was trying to use this (> 6 months ago):
https://github.com/apache/casbin-ucon/blob/master/.github/workflows/PerformancePush.yml#L24

Result for them:
https://apache.github.io/casbin-ucon/benchmark-monitoring/

Any alternatives you've considered

We looked at what some other projects do for benchmarking

  • Apache Arrow runs its perf infrastructure on Conbench, a custom continuous-benchmarking server the project built and self-hosts. Results live at conbench.ursa.dev, not on GitHub Pages.
  • Apache Lucene uses luceneutil (Mike McCandless's long-running suite, ~15 years of nightly results) published at benchmarks.mikemccandless.com — entirely outside GitHub. The repo issue #1750 ("objective performance test for Lucene") is still open, which suggests they haven't standardised on a CI-driven dashboard at all.
  • Apache Hadoop has built-in benchmarking tools (NNBench, MRBench, TestDFSIO) documented on the project site but, as far as I can see, no automated dashboard wired into GitHub Actions.

These seemed like overkill or not applicable for us.

Any security concerns you've identified

The action writes into the gh-pages section of the repo. We don't use that for anything else.
We do need to give it write permissions to allow it to do its work.

Claude's security assessment of the plugin:

  • No external code is fetched at runtime — only git to GitHub and Octokit calls to api.github.com.
  • No eval/Function/dynamic require; all parsing is JSON.parse or regex.
  • Permissions required: contents: write (for gh-pages auto-push) and issues: write (commit comments)
  • Maintainer concentration is the residual risk — single active maintainer, unsigned release commits. A SHA pin mitigates this until you upgrade.
  1. Pin verification
  • Workflow pin: a60cea5bc7b49e15c1f58f411161f99e0df48372
  • git/refs/tags/v1.22.0 resolves to the same SHA. Pin is genuine.
  • Release commit author/committer: Chris Trzesniewski (GitHub: ktrz). Commit signature: unsigned (verification.verified=false). Not unusual for actions, but means trust is anchored to repo-write access, not a maintainer key.
  1. Project trust surface
Signal Value
Owner benchmark-action org (project was donated by original author rhysd, 297 commits)
Active release maintainer ktrz (68 commits) — sole signer of the last 6 releases
Other contributors small long-tail; mostly Dependabot
Stars / Forks 1.2k / 181
Open issues 125
Published advisories None
License MIT
Last release / push v1.22.0 on 2026-03-31 / repo pushed 2026-04-23

@potiuk
Copy link
Copy Markdown
Member

potiuk commented May 6, 2026

@paulk-asert thanks for the PR. I ran verify-action-build against benchmark-action/github-action-benchmark@a60cea5b… and traced the JS-build-verification failure. It turns out not to be a problem with the action itself — it's a gap in our checker that I've now fixed.

What was happening

The v1.22.0 tag commit (a60cea5b…) is a source-detached orphan tag — its tree contains only dist/, node_modules/, action.yml, package.json, and package-lock.json. The buildable source lives on the master branch at the parallel "release v1.22.0" commit c38cce98215c. This pattern is already supported by verify-action-build (PR #768), but the detector required the orphan tag to have no package.json — and benchmark-action's release workflow ships package.json on the orphan tag (so consumer tooling and node_modules/ resolution work). The fallback therefore never fired, and the rebuild ran against a tree with no source — which is why canonicalizeUnit.js showed up as "only in original".

Fix

PR #808 broadens the orphan-tag detection to handle this shape (drops the not has_pkg requirement, adds a guard for root-source actions). With the fix:

  • Source-detached fallback fires ✓
  • Source commit c38cce98215c is resolved on master
  • All 16 src/*.js files in the published dist/ are byte-identical to a fresh rebuild ✓

One residual finding to flag

After #808 lands, one minor mismatch remains: scripts/ci_validate_modification.js is "only in rebuilt" — the publisher's release workflow excludes CI helper scripts from the published dist/, but tsconfig.build.json's **/*.ts pattern compiles everything. This is publisher curation rather than a security concern. Whether we should be lenient about "extra in rebuilt" outputs (which can never represent malicious additions on the publisher side, only omissions) is a separate design question I'll raise in a follow-up.

Next steps

  • Once verify-action-build: detect orphan tags that ship package.json #808 merges, re-trigger CI on this PR — JS build verification should pass for the 16 src/*.js files.
  • The scripts/ci_validate_modification.js mismatch may still flag depending on how the residual issue lands; if it does, you can either (a) wait for the follow-up, or (b) ask the upstream maintainers to tighten tsconfig.build.json to exclude scripts/.

No action needed on your end right now — the ball is on our side.

@potiuk
Copy link
Copy Markdown
Member

potiuk commented May 6, 2026

Quick follow-up on the residual finding: I've opened benchmark-action/github-action-benchmark#351 asking the upstream maintainers to tighten tsconfig.build.json so scripts/ci_validate_modification.ts isn't compiled into dist/. That's the publisher-side fix that makes their dist reproducible end-to-end. If they merge it on a future release, the "extra in rebuilt" mismatch goes away on its own; if they don't, we'll handle it via the lenience-tweak follow-up I mentioned.

@potiuk
Copy link
Copy Markdown
Member

potiuk commented May 6, 2026

Hey @raboof @dfoulks1 @dave2wave -> can we merge #808 to unblock that one ? Also @paulk-asert -> Author of the action already fixed and released the fix that I asked as a follow-up - so it would be great to swtch to that new version of the action benchmark-action/github-action-benchmark#351 (comment)

It needs to wait for cooldown (if you have it - we have the recommendation to use 7 day cooldown for actions - so either you can wait for it or decrease cooldown for this particular action (once we review it, it should be good to use- we currently have no cooldowns in "infrastructure-actions" - to be able to review them quickly (per @raboof insting :) ) - which I tink this case shows was a good idea :).

@paulk-asert
Copy link
Copy Markdown
Author

@potiuk We have 7 days cooldown for dependabot on the groovy side but not really relevant here since the reference to the plugin is commented out for now - since it breaks the build. But I'll update the PR and re-enable with the correct version once that's passed on the security side.

@paulk-asert paulk-asert force-pushed the addBenchmarkAction branch from 8668ee5 to 0f996dd Compare May 6, 2026 13:48
@paulk-asert
Copy link
Copy Markdown
Author

The PR is updated to use the new version of the action.

@potiuk
Copy link
Copy Markdown
Member

potiuk commented May 7, 2026

@paulk-asert update on the diagnosis: the failure on v1.22.1 (52576c92…) is entirely on our side, not the action's. Two issues found, both case E (verify-script gap):

1. Wrong source commit picked (the cause of all the JS-rebuild "DIFFERENCES DETECTED" output): verify-action-build's source-detached fallback was using a published_at + 1 minute cutoff that excluded benchmark-action's master release v1.22.1 commit by 1 second. Resolver fell back to v1.22.0's release commit, rebuild produced v1.22.0's dist, the diff was actually v1.22.0-vs-v1.22.1 — naturally everything looked different. Fix in flight at #821. With it, all 16 src/*.js files are byte-identical between published and rebuilt; verify exits 0.

2. IDE config files in vendored node_modules (the tunnel/.idea/* warnings): turned out to be a side-effect of #1 — once the right source commit is used, those don't appear anymore (v1.22.1's package-lock.json pins a different tunnel tarball that doesn't carry the IDE files). I've still opened #822 as a defensive filter so this class of drift is ignored if it recurs on a different action.

Once #821 merges and CI re-runs here, this PR should pass cleanly. No action needed on your end.

(Side note: my earlier upstream issue benchmark-action/github-action-benchmark#351 was already addressed by their 496ae8b0184c "fix(build): scope tsconfig.build.json to src/ for reproducibility (#352)". Their scripts/ci_validate_modification.js finding from before is gone in v1.22.1.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants