fix(ci): Cap clang-tidy concurrency in lint:check-cpp-full.#2382
Open
jackluo923 wants to merge 2 commits into
Open
fix(ci): Cap clang-tidy concurrency in lint:check-cpp-full.#2382jackluo923 wants to merge 2 commits into
jackluo923 wants to merge 2 commits into
Conversation
Contributor
WalkthroughThe macOS workflow now limits parallel ChangesmacOS lint workflow
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`lint:check-cpp-full` runs `clang-tidy` over every C++ source file by fanning out one process per file through a Taskfile `deps: for:` loop, which has no built-in concurrency limit. Each `clang-tidy` process holds a full preprocessed translation unit, so spawning ~hundreds at once peaks high in memory. The runners don't have a huge amount of memory, and as the source tree and project complexity grow, that peak grows until it exceeds what the runner has and the OS swaps/thrashes instead of making progress, so the full pass can no longer finish within the runner's timeout. Cap concurrency to the logical-processor count via `task --concurrency` (default `$(getconf _NPROCESSORS_ONLN)`), overridable with `CLP_CPP_LINT_CONCURRENCY` (e.g. "1" for maximum memory safety). Keeping the pass within memory lets it complete, which also lets the `Update lint:check-cpp-static-full cache` step (main only) save a cache so future unchanged-source PRs can skip the step.
jackluo923
force-pushed
the
fix/unbounded-linting-task-execution-parallelism
branch
from
a9a4ced to
5be19cf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/clp-core-build-macos.yaml:
- Around line 117-120: Update the workflow step invoking task with
lint:check-cpp-full to validate CLP_CPP_LINT_CONCURRENCY before passing it as
--concurrency. Reject zero and any non-positive or invalid value, while
preserving the getconf fallback and allowing only positive integers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ae306b1-750a-4c06-942a-9b1db9ae463f
📒 Files selected for processing (1)
.github/workflows/clp-core-build-macos.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
clp-core-build-macos'slint:check-cpp-fullstep runsclang-tidyover every C++ source file by fanning out oneclang-tidyprocess per file through a Taskfiledeps: for:loop (clang-tidy-parallelinyscope-dev-utils). That loop has no built-in concurrency limit, so ~hundreds ofclang-tidyprocesses — each holding a full preprocessed translation unit / AST — are spawned at once.The GitHub runners don't have a huge amount of memory. As the number of source files and the project's complexity grow, the peak memory of this unbounded fan-out grows until it exceeds what the runner has, and the OS likely swaps / thrashes / OOMs instead of making progress — this is hard to confirm directly, but the observed behavior is consistent with it. The full pass then can no longer finish within the runner's timeout and is cancelled.
This has been happening on
main:clp-core-build-macoshasn't succeeded since ~2026-06-30; recent runs were cancelled after 6–9h, e.g.:(In each, the build + unit tests succeed in ~16–22 min; the remaining hours are all spent inside
lint:check-cpp-full.)This also breaks the lint cache (see #1419). The
Update lint:check-cpp-static-full cachestep only runs onmainand only after the full pass completes successfully. Since the passes keep getting cancelled, that save step never fires, so the macOS cache stays a near-empty shell (~37 KB of checksum files, nobuild/lint-clang-tidylogs). With no logs restored, the per-fileclang-tidytasks can't be skipped, so every full pass runs from scratch — a doom-loop that the timeout keeps resetting.The fix
Cap the lint concurrency to the logical-processor count at the call site:
This is a CLI-level cap (
task --concurrency), independent of theyscope-dev-utilssubmodule version, so it needs no submodule change. It keeps the pass within memory so it can complete — which in turn lets the cache save step fire onmain, store real logs, and let future unchanged-source PRs skip the step entirely.The default is
$(getconf _NPROCESSORS_ONLN);CLP_CPP_LINT_CONCURRENCYoverrides it (e.g.1for maximum memory safety) without a workflow edit.This also drops the
CLP_CPP_MAX_PARALLELISM_PER_BUILD_TASK=$(getconf _NPROCESSORS_ONLN)prefix the lint step previously carried. That prefix only looked like it limited lint parallelism: the variable is consumed solely by the cmake/make compilation jobs (deps:core,core-build) viaG_CPP_MAX_PARALLELISM_PER_BUILD_TASK, never byclang-tidyor the lint task chain —clang-tidyis single-threaded per invocation, and its parallelism comes entirely from the Taskfiledeps: for:loop. It was added to the lint step in #966 to limitcore's build parallelism, not the clang-tidy fan-out, so it never addressed the thrash. It was thus a no-op on the lint step, andtask --concurrencyis the actual lever.Checklist
clang-tidyis scheduled, not what is checked or any build output.)Validation performed
Applied the same cap onto
feat/webui-nodejs-24(PR #2373's branch — the worst offender) and ran it ony-scope/clp, and separately ran this PR's own branch through CI:Both capped runs complete in well under an hour on both matrix legs, vs. the uncapped run cancelled at 8h59m. The PR's own run (29474344830) is the most direct evidence — it's this exact branch on
y-scope/clp.With the cap, the log shows
clang-tidyrunning at a steady, controlled cadence (one file every few seconds, a few in parallel) and the full pass completes, vs. the unbounded run whose log goes quiet for hours once all processes are launched and then makes no further measurable progress until cancelled.Concretely, without the cap the log emits a burst of
task: [utils:cpp-lint:clang-tidy:...]start-lines for many files at once — i.e. thedeps: for:loop launches all processes together — after which the run chokes and the log goes quiet. A representative excerpt from an uncapped run (29469389246):The start-lines land together in a burst, and then the log goes quiet for a long time — the processes are launched together and then make no further measurable progress. With the cap, the pass instead proceeds at a steady cadence, with completions appearing roughly one every few seconds.
Note: a non-
mainrun doesn't populate the cache (the save step ismain-only), so the cache-skip benefit will only materialize once this lands onmainand amainrun completes a full pass.