Skip to content

perf(ci): run the semgrep-devex passes as parallel legs - #131450

Open
fuziontech wants to merge 2 commits into
masterfrom
perf/ci-semgrep-devex-parallel-legs
Open

fuziontech wants to merge 2 commits into
masterfrom
perf/ci-semgrep-devex-parallel-legs

Conversation

@fuziontech

@fuziontech fuziontech commented Nov 8, 2026 •

Copy link
Copy Markdown
Member

Problem

Code PRs wait on the semgrep-devex job, and on 19 of the last 40 merged PRs it is the Security job that finishes last, so its wall sets when the PR can merge. Its passes share nothing but ran one after another on one runner: 301s of whole-tree WARNING scan, 265s of ERROR scan, 110s of ratchet and 22s of new-warning diffing, 705s median all told.

Changes

  • semgrep-devex now runs as a three-way matrix (warnings, errors, ratchet), so its wall is the slowest leg instead of the sum: 705s -> 389s.
  • Every command, severity, scan target and if: gate is unchanged. Only the grouping moves, so no finding changes verdict.
  • Semgrep Checks Pass needs no edit: a matrix job reports failure when any leg fails, which is what its needs.semgrep-devex.result read already checks.
  • The legs stay on GitHub-hosted runners. They are unmetered on a public repository, so the two extra runners cost nothing, and the checkout holds PR-controlled code, which must not run on a host with Depot credentials.
flowchart LR
    A[changes]:::phYellow --> B[semgrep-devex]:::phBlue
    B --> B1[Warnings 301s]:::phGray
    B1 --> B2[Errors 265s]:::phGray
    B2 --> B3[Ratchet 132s]:::phGray
    B --> C[Semgrep Checks Pass]:::phRed
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
Loading
flowchart LR
    A[changes]:::phYellow --> B{semgrep-devex matrix}:::phBlue
    B --> B1[Warnings 389s]:::phGray
    B --> B2[Errors 361s]:::phGray
    B --> B3[Ratchet 226s]:::phGray
    B1 --> C[Semgrep Checks Pass]:::phRed
    B2 --> C
    B3 --> C
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
Loading

Before is the median of 11 merged PRs (2026-11-05..07), replayed from each PR's own step durations. After is two production runs of this PR — 42457221509 and 42459349854 — where the three legs ran concurrently and finished at warnings 389s / 371s, errors 361s / 264s, ratchet 226s / 218s, so the wall is 389s and 371s.

before after change
semgrep-devex job wall 705s 371-389s -44.8% to -47.4%
Security lane 12.03 min 6.77 min -43.8%
PR wait, the 19 of 40 PRs whose lane ends on this job 13.20 min 8.62 min -34.7%
PR wait, median of all 40 merged PRs 12.57 min 10.69 min -14.9%

The last row is the whole-PR number and is smaller, because the Security lane is the last check for only half of PRs; the other half lose nothing. Hosted runner-seconds go 705s -> 976s per PR (+38% compute for -45% wall), which is free on a public repository.

Note

The lane has a second long pole, unaffected here: a PR that edits ci-security.yaml makes the changes job force semgrep-python's whole-tree scan. This PR is that case, so its own Security lane ended on semgrep-python, not on the devex legs.

How did you test this code?

Test rationale: no new test. The change is a scheduling edit to one workflow and the existing gates are the test. hogli lint:workflows (15 checks, 144 workflows) and tools/workflow-plan npx vitest run (491 tests) pass. This PR's own Security run is the production check: all three legs ran their own step and nothing else, and all three concluded success.

Release status

  • No feature flag controls this change

Docs update

  • None. docs/internal/ci-things-already-tried.md has no entry for splitting this job; the measurement lives in the workflow comment.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Shelley, DeepSeek V4 Pro (platform API)

Skills: /authoring-ci-workflows, /writing-pr-descriptions

More context for AI agents
  • Session: nightly CI speed run; timings from the GitHub API (actions/runs, commits/<sha>/check-runs).
  • Decisions: split by severity, not by directory: a rule matches within one file, so a per-directory split could change findings.
  • Decisions: the 22s new-warning diff goes with ratchet (226s), not with the 389s WARNING scan, so the slowest leg stays the ERROR one.
  • Read closely: the if: on each pass step. A leg that loses its condition silently runs every pass on every runner.
  • Follow-ups: 127126 collapses the same job into one scan plus a report script; the two designs conflict and a human should pick one.

@fuziontech
fuziontech requested a review from a team as a code owner November 8, 2026 07:03
@fuziontech
fuziontech requested a review from a team November 8, 2026 07:03
@trunk-io

trunk-io Bot commented Nov 8, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

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

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Hey @fuziontech! 👋

It looks like your git author email on this PR isn't your @posthog.com address (fuziontech@gmail.com). Since you're on the PostHog team, it's worth pointing your local git author email at your @posthog.com address. Why it matters:

  • Consistent work identity in git history — internal tooling that attributes commits to team members keys off your @posthog.com address.
  • Keeps team contributions easy to tell apart from external community ones when scanning history.

You can fix it for this repo with:

git config user.email "you@posthog.com"

Or set it globally with git config --global user.email "you@posthog.com". No need to redo this PR — just a nudge for next time. 🙂

@parameterai

parameterai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

Risk: No findings

This increment merges master into the semgrep-devex matrix PR: master's restructuring (diff-scoped blocking Errors scan via the shared semgrep-ci action, PR-only "New sleeps" gate, informational passes moved off pull requests, desktop sparse checkout, the devex-rules full-scan trigger) plus the branch's matrix-leg split. I traced every blocking gate end to end and found no weakening a pull request can exploit: each step's condition only gains a matrix.leg == conjunct, the merge resolution dropped nothing from either parent, and all failure modes of the new diff-scoped Errors leg (oversized PRs, rule edits, odd filenames, git errors) fall back to or fail as a full/blocking scan.

Sentinel reviewed 58c05a4 · Review settings

@posthog-security-review-bot posthog-security-review-bot Bot added the security-review Request a security review label Nov 8, 2026
@posthog-security-review-bot

posthog-security-review-bot Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

✅ Security review complete — 5151bcaf5b7a

No findings above the confidence threshold, in 3 min.

Add the security-review label to run again on the latest commit.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

🚨 Trunk lane — universal lane

This PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong.

@posthog-security-review-bot posthog-security-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent-driven security review - no findings.

The three devex passes scan the same tree with different severities and
share nothing, but they ran one after another on one runner, so the job
wall was their sum. Measured on 11 merged PRs: 301s of whole-tree WARNING
scan, 265s of ERROR scan, 110s of ratchet and 22s of new-warning diffing.

They now run as a three-way matrix, so the wall is the slowest leg rather
than the sum: 705s -> 356s median. The Security lane ends on this job for
19 of 40 merged PRs, so the lane drops 12.0 -> 6.2 min for those.

Every command, severity and scan target is unchanged; only the grouping
moves. The legs stay on GitHub-hosted runners, which are unmetered on a
public repository, so the two extra runners cost nothing, and the checkout
still runs PR-controlled code on a host with no Depot credentials.

'Semgrep Checks Pass' needs no change: a matrix job reports failure when
any leg fails, which is what its needs.semgrep-devex.result check reads.

Co-authored-by: Shelley <shelley@exe.dev>
@fuziontech
fuziontech force-pushed the perf/ci-semgrep-devex-parallel-legs branch from 5151bca to 9de7392 Compare November 8, 2026 07:34
@fuziontech

Copy link
Copy Markdown
Member Author

Nightly CI Speed Review

Reviewed against the production runs and historical baseline data.

1. Verification of Timing & Queue Numbers

Recomputed queue times (started_at - created_at) and job durations from the GitHub API across both cited production runs:

  • Run 42457221509:
    • semgrep-devex (warnings): queue 2s, duration 389s
    • semgrep-devex (errors): queue 2s, duration 361s
    • semgrep-devex (ratchet): queue 2s, duration 226s
    • Parallel wall clock: 389s
  • Run 42459349854:
    • semgrep-devex (warnings): queue 2s, duration 371s
    • semgrep-devex (errors): queue 2s, duration 264s
    • semgrep-devex (ratchet): queue 2s, duration 218s
    • Parallel wall clock: 371s
  • Pre-change baseline check: Sampled merged PRs (e.g. PR fix(persons): keep query url updates on the persons list #131431, run 42424670705), where sequential execution took 881s (warnings 350s + auth migration 55s + errors 319s + ratchet 94s + new warnings 23s + ~40s overhead).
  • Verdict on numbers: The claimed ~45% to ~47% reduction in semgrep-devex wall clock (705s median down to 371–389s) holds up. GitHub-hosted runners queued in ~2s for all three parallel legs.

2. Required Check & Graph Integrity

  • The required status check is Semgrep Checks Pass (semgrep_checks job).
  • It depends on semgrep-devex via needs: [..., semgrep-devex, ...]. In GitHub Actions, when semgrep-devex is a matrix job, ${{ needs.semgrep-devex.result }} resolves to "success" if and only if all matrix legs succeed, and "failure" if any leg fails. fail-fast: false ensures all legs report their findings.
  • Note on run 42457221509: Semgrep Checks Pass shows cancelled in run 42457221509 because the commit amend at 07:34:24Z triggered concurrency cancellation right during the job's final cleanup step; the job step Check matrix outcome itself completed with All checks passed. before cancellation.
  • timeout-minutes: 20 comfortably covers the ~4–6 minute legs.

3. Security & Side Effects

  • Uses ubuntu-24.04 (GitHub-hosted), keeping PR-controlled code off Depot runners with persistent cache credentials.
  • fetch-depth: 2 preserves HEAD^1 access for the PR baseline diff on the ratchet leg.
  • persist-credentials: false is maintained.
  • No stale telemetry or external tool dependencies on the split job names (preflight_checks.py only uses semgrep-devex as an internal key).

4. Local Test Results

  • Workflow linter: PYTHONPATH=tools/hogli-commands:tools/hogli/src python3 -c "from click.testing import CliRunner; from hogli_commands.workflow_lint.cli import cmd_lint_workflows; r=CliRunner().invoke(cmd_lint_workflows, []); print(r.output[-200:], r.exit_code)" -> 15/15 checks passed across 144 workflows (exit 0).
  • Workflow plan tests: cd tools/workflow-plan && npx vitest run -> 491/491 passed.

Conclusion

The change is sound, safe to land, and delivers the claimed speedup. No fix commit required.

@fuziontech

Copy link
Copy Markdown
Member Author

Lane replay, to size what this is worth. Not a review, just a number a reviewer can use.

I replayed 45 recently merged pull requests through the needs: graph with this change's shape applied (the devex job becomes a three-way matrix whose wall is its warnings leg, 334s scan + 35s checkout, and the Semgrep Checks Pass fan-in pulls forward to the end of that leg). Everything else is each PR's own measured job times.

PR lane median p90
master today 13.2 min 20.2 min
with this PR applied 7.1 min 20.2 min

That is 46% off the median pull request, because Semgrep Checks Pass is the last thing to finish for 28 of the 45 sampled PRs. This is the single largest CI wall-clock lever I found, which is why it is worth the extra runners.

The p90 does not move, which is the honest limit of the number: it is set by the pull requests where something other than Security runs long (MCP Integration Tests at 630s, Playwright E2E tests at 674s, the Node.js serial shards at ~330s). Those are untouched here and only matter on a minority of PRs.

Two notes on the replay, since it is a model rather than a production A/B:

Master moved the devex job twice while this branch split it into matrix
legs: the two informational passes now run on pushes and the hourly
schedule only, and the blocking ERROR pass now goes through the shared
`semgrep-ci` action. Both changes are orthogonal to the legs, so the
resolution keeps both sides.

- The informational steps keep their leg guard and gain master's event
  guard, so they run on the `warnings`/`ratchet` leg of a push only.
- The blocking ERROR step keeps `matrix.leg == 'errors'` and takes
  master's action and `--include` arguments.
- "New sleeps in Python tests" arrived from master without a leg guard.
  With three legs it would have scanned the same changed files three
  times, so it joins the ratchet leg with the other diff-based passes.

On a pull request the warnings leg now has no steps left to run; the
split there is errors against ratchet.

Co-authored-by: Shelley <shelley@exe.dev>
@veria-ai

veria-ai Bot commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Reviews

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants