Skip to content

perf(ci): stop checking out full history to gate the nodejs suite - #126087

Merged
trunk-io[bot] merged 1 commit into
masterfrom
ci-speed/nightly
Oct 30, 2026
Merged

trunk-io[bot] merged 1 commit into
masterfrom
ci-speed/nightly

Conversation

@fuziontech

@fuziontech fuziontech commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Problem

  • Every pull request that changes Node source waits about 80 seconds before its first Jest shard starts.
  • The Determine need to run Node.js checks gate clones the repository's full history to decide what to run, and every job in the workflow needs that gate. The paths filter it runs reads the change set from the pull-request files API, so most of that clone is spent on history nothing in the job opens.

Changes

  • The gate job now takes 6 seconds instead of 80 (median of 28 runs), so the Node.js suite starts ~74s earlier on every Node-touching pull request.
  • The job checks out the filter action only. Affected-crate detection — the one step that genuinely needs the merge base — gets its own full-history checkout, behind the rust filter output that reaches it, with the same sparse cone ci-rust.yml already gives the same binary.
  • Mechanical: a note in /authoring-ci-workflows recording the two-step pattern, since the section on gating jobs recommends the cheap checkout but says nothing about the step that needs history.

The step graph inside the gate job, before and after:

flowchart LR
  A[changes job] --> B["checkout: whole tree,<br/>full history"]
  B --> C[paths filter]
  C --> D[rust affected crates]
  D --> E[jest selection]
  E --> F[suite jobs]
  classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
  classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
  classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
  class A,F phYellow;
  class B phBlue;
  class C,D,E phGray;
Loading
flowchart LR
  A[changes job] --> B["sparse checkout:<br/>filter action only"]
  B --> C[paths filter]
  C -->|rust changed| R["sparse checkout:<br/>rust/ proto/ .github/"]
  R --> D[rust affected crates]
  C --> E[jest selection]
  D --> E
  E --> F[suite jobs]
  classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
  classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
  classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
  class A,F phYellow;
  class B,R phBlue;
  class C,D,E phGray;
Loading

Measured from the Actions API, on real runs:

Metric Before After
Determine need to run Node.js checks, job duration 80s median, 103s max (n=28) 6s (this PR's run)
Checkout step inside it 75s median, 81s max (n=12) 2s
Run created → first Jest shard starts 81s median, 79–89s range (n=3 full-suite runs) 10s
Suite jobs (Jest shards, Rust e2e) — unchanged, so the lane end drops by the gate delta

The lane end is set by the suite, not by the gate, so a run saves the gate delta when its jobs take the same time: 74s of a ~7.5 min lane, about 16%. On this PR's own run the Rust ingestion e2e job happened to take 417s against 326s in the before sample, which hides the 74s in the lane-end number, so the table reports the two figures that isolate the change instead.

How did you test this code?

Test rationale: no new test. A workflow edit is verified by the runs it produces, not by a unit test, and the workflow planner suite already pins which jobs run in each scenario.

  • The gate job's own run on this PR: checkout step 2s, job 6s, suite jobs started 1s later (run 41015849853).
  • PYTHONPATH=tools/hogli-commands:tools/hogli/src python3 -c "..." — all 14 workflow lint checks pass.
  • cd tools/workflow-plan && npx vitest run — 393 tests pass; the ci-nodejs plan output is unchanged in every scenario, since no if: moved.
  • node tools/workflow-plan/src/cli.ts .github/workflows/ci-nodejs.yml — drafts, ready, fork, queue and master rows unchanged.
  • The Rust path is not exercised by this PR's run (the diff touches no rust/ or proto/ file), so its checkout was verified locally instead: a --depth=1 --filter=blob:none clone, unshallowed with --filter=blob:none, switched to the rust/ proto/ .github/ cone, and the base-commit worktree that affected-services builds — the three git operations the step performs — all succeed, and git diff --name-only <base>...HEAD returns the change set the binary reads. It also matches the checkout ci-rust.yml's affected job already runs in production, and a wrong cone fails the step loudly rather than silently skipping the suite.

Release status

  • No feature flag controls this change

Docs update

  • None beyond the /authoring-ci-workflows note in Changes; the existing checkout section already says a gating job should sparse-checkout the filter action.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Shelley (shelley), model deepseek-flash-api-deepseek-com (DeepSeek's own platform API)

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

More context for AI agents

The Node.js changes job checked out the whole repo with fetch-depth: 0 to run
the paths filter, which reads the change set from the pull-request files API
and never touches the tree. The checkout was 75s of an 80s job (median of 12
runs), and every job in the workflow needs that job.

The job now checks out only the filter action, and the affected-crate
detection that does need the merge base gets its own full-history checkout
behind the rust filter that reaches it, with the same sparse cone ci-rust.yml
gives the same binary.

Co-authored-by: Shelley <shelley@exe.dev>
@trunk-io

trunk-io Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@github-actions

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. 🙂

@github-actions

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane

This PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.

@hosthog

hosthog Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

HostHog preview — posthog-desktop-web

The previews for this PR have been torn down and no longer serve.

@fuziontech
fuziontech requested a review from a team October 30, 2026 06:52
@fuziontech
fuziontech marked this pull request as ready for review October 30, 2026 06:53
@parameterai

parameterai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Risk: No findings

A CI-only performance change: the ci-nodejs gating job now sparse-checkouts only the paths-filter action instead of the full-history tree, and adds a conditional second full-history checkout (cone .github/ proto/ rust/) that runs only when the rust filter matches. I traced every step that reads the tree, the conditional gating, the cone coverage against what rust-compute-affected and the affected-services binary need, and the cache/token paths, and found no security-relevant behavior change: failures fail loudly (all suite jobs gate on this job), and no trust boundary, secret, or attacker-controlled input handling changed.

Sentinel reviewed 89ae6d4 · Review settings

@fuziontech

Copy link
Copy Markdown
Member Author

Nightly Follow-Up Review

Reviewed the change against the workflow graph, security requirements, and real GitHub Actions run telemetry.

1. Claimed Performance Win Scrutiny

Recomputed job and queue timing from the GitHub Actions API:

  • Run 41015849853 (PR ci-speed/nightly):
    • Run created_at: 2026-10-30T06:44:28Z
    • Job Determine need to run Node.js checks: created_at: 06:44:29Z, started_at: 06:44:31Z (queue: 2s), completed_at: 06:44:37Z (duration: 6s).
    • Downstream suite jobs (build, lint, tests shards, rust_ingestion_e2e): created and started at 06:44:38Z (10s after run creation).
  • Run 41015862541 (before / master baseline on PR fix(agent): read the claude context window from the cli #126084):
    • Run created_at: 2026-10-30T06:44:36Z
    • Job Determine need to run Node.js checks: created_at: 06:44:37Z, started_at: 06:44:39Z (queue: 2s), completed_at: 06:46:04Z (duration: 85s).
    • Downstream suite jobs: created and started at 06:46:04Z (88s after run creation).

The numbers claimed in the PR description survive scrutiny: gating duration drops from ~85s to 6s, unblocking downstream jobs 78s earlier.

2. Workflow Correctness & Integrity

  • Required checks & dependencies: The needs: graph is unchanged. node_tests (Node.js Tests Pass) remains the gating collate job and correctly aggregates [changes, tests, rust_ingestion_e2e, build, lint].
  • Concurrency & timeouts: Concurrency block untouched; timeout-minutes: 10 easily covers the new gate step (and the conditional Rust fetch).
  • Draft / ready-for-review split: ci-nodejs.yml has no draft/ready split; workflow-plan evaluation across all scenarios (draft, ready, fork, queue, schedule, push) matches master expectations.
  • Workflow verification tools:
    • cmd_lint_workflows: all 14 checks passed across 146 workflows.
    • tools/workflow-plan vitest: 393/393 tests passed.

3. Security & Runner Policies

  • Cache isolation: changes runs on GitHub-hosted ubuntu-24.04 (not Depot), so the executable cache restored in actions/rust-compute-affected stays on GitHub Actions cache with branch isolation.
  • Fork safety: Token minting remains guarded with fallback to github.token.
  • Checkout semantics: sparse-checkout with clean: false preserves .github/actions/paths-filter while allowing the conditional unshallow and sparse cone expansion (rust/, proto/, .github/) if Rust files change.

Verdict

Safe to land. No bugs or regressions found.

@rnegron rnegron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, thanks!

@fuziontech

Copy link
Copy Markdown
Member Author

/trunk merge

@trunk-io
trunk-io Bot merged commit 327ba7b into master Oct 30, 2026
316 checks passed
@trunk-io
trunk-io Bot deleted the ci-speed/nightly branch October 30, 2026 15:59
@deployment-status-posthog

deployment-status-posthog Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-10-30 16:51 UTC Run
prod-us ✅ Deployed 2026-10-30 17:17 UTC Run
prod-eu ✅ Deployed 2026-10-30 17:11 UTC Run

@fuziontech

Copy link
Copy Markdown
Member Author

Effectiveness check: this worked

Verified against the GitHub API on 2026-11-01, a day after this merged.

Claim: the Determine need to run Node.js checks gate goes from ~80s to ~6s, saving ~74s on every Node-touching PR.

Measured:

runs median range
before (merged pre-change) 6 86s 84-99s
after (merged post-change) 5 9s 7-21s

89.5% faster, or ~77s off the gate on every Node-touching PR. The direction and the magnitude both match what the PR promised, and the gate is where the PR said it was: the pre-change job really did spend 84-99s, which is the full-history clone.

How this was measured, since it matters for trusting the number: each sample is a merged PR's head SHA, and the run is picked by its created_at relative to the merge time, not by the PR's merge date. Sampling by merge date silently reclassifies old PRs whose checks were re-run by the merge queue after the change landed, which makes the two sides look identical and reports false "no change". The first attempt here made exactly that mistake and showed 8s vs 7.5s.

Caveats: n is small (6 and 5), and the post-change spread reaches 21s on one run, which is runner variance rather than a regression. No revert needed.

Posted by the nightly CI speed tuner. Method: check-runs API, run-side attribution by created_at.

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