fix(ci): stop semgrep-general timing out on every PR - #77481
Conversation
semgrep-general was the only semgrep job without a changed-paths gate, so it ran on 100% of PRs while scanning only the directories the other jobs exclude. Its p50 is 12.3 min against a 20 min timeout, so slow runners tripped it and blocked the required Semgrep Checks Pass check. Gate it on the complement of its own --exclude list and raise the timeout to 30, matching semgrep-python. Coverage is unchanged: master pushes and oversized PRs still bypass the filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🚫 This stack was removed from the merge queue because it was canceled by Georges-Antoine Assi (a GitHub user). See more details here.
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Reviews (1): Last reviewed commit: "fix(ci): stop semgrep-general timing out..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR reduces CI tax and merge-queue churn by preventing the semgrep-general shard in the Security workflow from running on PRs that only touch directories it explicitly excludes from scanning, while also increasing its timeout to avoid runner-variance timeouts.
Changes:
- Add a new
generalpaths-filter output and gatesemgrep-generalon it (so it skips on PRs that only change excluded paths). - Increase
semgrep-general’stimeout-minutesfrom 20 to 30 to add runtime headroom.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The 1.163.0 -> 1.167.0 bump in #65921 made semgrep-general 4x slower on linux/amd64: p50 went from 3.1 to 12 min within two minutes of that merge (455 runs before, 143 after). The regression does not reproduce on arm64, where 1.163, 1.167 and 1.172 all run the same scan in ~82s. Bump forward rather than revert, since #65921 moved off 1.163.0 to fix an intermittent hang. Whether this clears the amd64 regression can only be measured in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bumping to 1.172.0 did not clear the amd64 regression: semgrep-general came in at 18.7 min against a 12.3 min p50 on 1.167.0, nowhere near the 3.1 min it ran at before #65921. No benefit to justify moving the pin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Splits semgrep-general's rule packs to test whether p/github-actions carries the 3.9x regression. semgrep-general is the 4-pack control. Not wired into semgrep_checks, so they gate nothing. To be removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 1 cleared p/github-actions (0.7 min) and put the cost on the other three packs (12.2 min). These run each pack alone, plus one job that adds --severity=ERROR to test whether the generic multilang rules drive it. Still not wired into semgrep_checks. To be removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Isolation is done: p/trailofbits accounts for 11.0 of semgrep-general's 12.5 min, and --severity=ERROR across all three packs runs in 1.4 min. Net diff is back to the path gate plus the timeout bump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/trunk merge |
|
This PR is already queued as a stacked merge. Cancel it first to re-submit. |
|
Reviews (2): Last reviewed commit: "chore(ci): remove temporary semgrep diag..." | Re-trigger Greptile |
|
Reviews (3): Last reviewed commit: "Merge branch 'master' into worktree-keen..." | Re-trigger Greptile |
|
Recreated as #78225 on a fresh branch, since this one kept getting stuck in the merge queue. Same diff, same description. |
Problem
semgrep-generaltrips its own timeout often enough to be a real tax on the merge queue. When it does, GitHub reports the job ascancelledrather thanfailed, so it doesn't read like a normal CI failure. Example run: the job ran 20m15s against a 20 min limit and was killed mid-scan, while every other job in that workflow passed.I sampled every
Securityrun from Jul 28 to Aug 4 (692 completedsemgrep-generaljobs):1.4% overall, and in one 2.5 hour window on Aug 4 it was 7 of 258 (2.7%).
Semgrep Checks Passis a required check, so each one blocks a PR or kicks it out of the queue.Root cause: the job got 4x slower on Jun 24
This job used to take 3 minutes. Sampling its duration back through the year:
Not gradual growth from the repo getting bigger. A step change overnight. Pulling all 598
semgrep-generalruns from Jun 24 puts the cutover inside a two minute window:#65921 (
fix(ci): upgrade semgrep 1.163.0 → 1.167.0 to fix intermittent hang) merged at 20:54:35 UTC, eight seconds before the first slow run started. It is the only change toci-security.yamlor.semgrep/that day. Before it, 455 runs at p50 3.1 min. After it, 143 runs at p50 12 min.Warning
The 1.167.0 pin is the actual cause of the slowness, and it is still in place. This PR does not fix that, it just stops it from failing PRs. I tried bumping the version and it did not help, see below.
That regression left a 12 min p50 under a 20 min timeout, about 1.6x headroom, so ordinary runner variance tips it over. #70907 added
--jobs 4three weeks ago to claw the time back and the tail did not move.Second, independent problem:
semgrep-generalwas the only semgrep job without aneeds: changesgate. It scans 1,605 files inpackages/,tools/,.github/,bin/and friends, since everything else is--excluded. Of the last 400 first-parent merges to master, 370 (92.5%) touched nothing it scans and still paid for a 12 min job.Changes
Gate the job on the complement of its own
--excludelist, and raise the timeout to 30 to matchsemgrep-python.Note
Coverage is unchanged. Pushes to master skip the filter entirely (
if: github.event_name != 'push'), so master still runs the full scan, and oversized PRs still force every scan viaforce_all. The aggregator already toleratedskippedfor this job, so no change was needed there.flowchart TD A{{PR}} --> B[changes path filter] B --> C[semgrep-python] B --> D[semgrep-js] B --> E[semgrep-devex] A --> F[semgrep-general<br/>always runs, 12 min] C --> G[Semgrep Checks Pass] D --> G E --> G F --> G classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class A,G phYellow; class B phBlue; class F phRed;flowchart TD A{{PR}} --> B[changes path filter] B --> C[semgrep-python] B --> D[semgrep-js] B --> E[semgrep-devex] B --> F[semgrep-general<br/>skipped on ~92% of PRs] C --> G[Semgrep Checks Pass] D --> G E --> G F --> G classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class A,G phYellow; class B,F phBlue;What I tried and rejected: bumping the version
Pushed 1.172.0 to this PR to see if a newer build cleared the amd64 regression. It did not:
Nowhere near the 3.1 min it ran at before #65921, and if anything worse. Reverted, so this PR leaves the pin at 1.167.0. One sample against a p50 on shared runners, so I would not claim 1.172.0 is worse, only that it plainly does not fix this.
The regression is isolated to one job
Splitting all 597 Jun 24 semgrep jobs on the 20:54:35 boundary:
So it is not a general engine slowdown. Whatever 1.167.0 changed, it only bites
semgrep-general's configuration. Two things are unique to that job: it is the only one using--config p/github-actions, and it is the only one running the full rule set without--severity=ERROR(which is what keepssemgrep-desktopat 1 min despite scanning 4,827 files with the same expensive packs, since it cuts 812 rules to 228 and the generic<multilang>rules from 24 to 3).Which pack: measured, not guessed
I ran two rounds of temporary diagnostic jobs on this PR that split the rule packs, with the 4-pack
semgrep-generalas a same-commit control. My first guess wasp/github-actions, since it is unique to this job and itscurl-eval/gha-curl-pipe-shellrules throw parse errors 24 times per scan. That was wrong.Round 1 cleared it:
diag-only-github-actionsdiag-without-github-actionssemgrep-general(control)Round 2 pinned it:
diag-trailofbitsdiag-owaspdiag-security-auditdiag-severity-error--severity=ERRORsemgrep-general(control)So
p/trailofbitsis essentially the entire cost, 11.0 of 12.5 min, and--severity=ERRORacross all three packs collapses it to 1.4 min.This reconciles with everything else. Local profiling put trailofbits at 69s of 81s, the same shape. If trailofbits is ~11 of today's 12.5 min, it was ~2 of the pre-regression 3.1 min, so Jun 24 was a ~5.5x slowdown localized to that one pack rather than a broad engine change, which is why the other seven jobs stayed flat.
The diagnostic jobs have been removed. They were never wired into
semgrep_checks, so they gated nothing while they existed.Follow-ups (not in this PR)
Decide what to do about
p/trailofbits. Three options, all of them someone else's call since they trade off security coverage:--severity=ERRORtosemgrep-general, assemgrep-desktopalready does. Takes it to ~1.4 min. Costs real coverage: 812 rules drop to 228, and the generic<multilang>rules go from 24 to 3. Not a free win.p/trailofbitsin this job. Other jobs already load it, so the coverage loss is narrower than it looks.linux/amd64between 1.163.0 and 1.167.0, with no change on arm64, looks like a semgrep bug worth filing.curl-evalandgha-curl-pipe-shellare broken. Unrelated to performance, now thatp/github-actionsis cleared. They throwmetavariable-pattern failed when parsing24 times per scan against.github/workflows/**, so those two rules are not doing their job today.Priority note. With the path gate,
semgrep-generalruns on roughly 8% of PRs, so the 12 min is much less costly than it was. Worth fixing, no longer urgent.How did you test this code?
I could not reproduce the slowness locally, and chasing that is what found the root cause. On an arm64 Mac the exact CI command runs in 81 seconds against CI's ~12 minutes, so I A/B'd the pinned versions on identical hardware and flags:
Identical. The regression does not reproduce on arm64, which is why it points at the
linux/amd64build rather than at rule content. I confirmed both image digests are proper multi-arch OCI indexes with a reallinux/amd64entry, so it is not QEMU emulation. Both versions also emit the same 24 internal matching errors and 217 syntax errors, so they are doing the same work.Beyond the local A/B, the pack isolation above was measured in CI on this PR across two rounds of throwaway diagnostic jobs, each with the 4-pack scan as a same-commit control. Those jobs carried
continue-on-error: true, declaredtimeout-minutes, and were deliberately kept out ofsemgrep_checks.needsso they could not gate the required check. All of them have been removed, so the net diff is just the path gate and the timeout.Automated checks I actually ran:
bin/hogli lint:workflows- 7/7 checks pass across 123 workflows, includingWF003-dorny-negation(which exists to catch exactly the negation footgun this filter relies on) andWF007-required-check-gates.hogli ci:preflight- 0 failures.Filterclass over 20 representative paths. Temporary test, not committed.Filter match cases (all pass)
Matches (job runs):
.github/workflows/ci-security.yaml,packages/quill/packages/charts/src/core/canvas-renderer.ts,tools/hogli-commands/hogli_commands/doctor.py,bin/sandbox,terraform/main.tf,uv.lock,.agents/skills/x/SKILL.md,playwright/foo.tsNo match (job skips):
posthog/api/team.py,products/foo/backend/api.py,frontend/src/App.tsx,services/mcp/src/index.ts,.semgrep/rules/security/a.yaml,rust/capture/src/main.rs,docs/internal/x.md,ee/api/x.py,common/x/y.py,nodejs/x.ts,livestream/main.go,cli/src/main.rsThe dotfile cases matter: the action sets
dot: true, so**does cover.github/**and.agents/**.The 92.5% figure comes from replaying
git log --first-parent -400 masterand checking each merge's changed files against the exclude list. Since this PR editsci-security.yaml, the new filter matches andsemgrep-generalruns on this PR, so CI here exercises the gate rather than skipping it.No new tests. The behavior here is a workflow path filter, and
bin/hogli lint:workflowsalready guards the conventions this change touches.Automatic notifications
Docs update
None needed.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
I pointed Claude (Claude Code, Opus 5) at a failing
semgrep-generaljob and asked how often it breaks and whether it could be fixed. No repo skills were invoked.The first useful finding was that the job was not failing on a semgrep rule at all. GitHub surfaces a
timeout-minuteskill ascancelled, which is why this reads as noise rather than a bug. Confirming it meant checking that sibling jobs in the same run had completed successfully, ruling out a whole-run cancellation, then separating the ~20.2 min kills from genuine cancellations by duration across a week of runs.The first pass stopped there and shipped the gate plus the timeout, with a guess that the
p/trailofbitspack was simply expensive. That guess was wrong, and the 81s local vs 12 min CI gap was the thread worth pulling: a 14x difference is too large for hardware. Ruling things out one at a time (semgrep version on arm64, rule count, file set, engine errors, image architecture) left "something environmental changed at a point in time", which turned into plotting the job's duration back through the year and bisecting to a two minute window on Jun 24.Worth flagging for review: the code change here is unchanged from that first pass and is deliberately defensive rather than curative. It stops the bleeding without touching the security scanner pin. Whether to chase the version regression is a separate call, and reverting is specifically not safe given why #65921 happened.