fix(ci): cut cache-quota waste, add a self-serve build-health dashboard - #656
fix(ci): cut cache-quota waste, add a self-serve build-health dashboard#656balajinvda wants to merge 5 commits into
Conversation
…tool The repository was at 9.42 GB of its 10 GB Actions cache quota across 16 entries, so every new entry was evicting one another job needed. Nothing reports this: jobs still pass, they are just colder. Two causes. dependency-docs-bazel was 4.19 GB, 42% of the whole budget, in three copies. It caches a Bazel build of five Java runtime inventories, and every branch wrote its own full-size copy. It now restores everywhere and saves only from main, so one authoritative entry serves every branch. Merge-queue refs were duplicating the largest keys. gh-readonly-queue/** branches are deleted when the queue drains, so an entry saved there can never be restored, but it still counts against quota until evicted. 1.90 GB was sat in exactly that state, from the pr-593 and pr-595 merges. The bazel cache is now split into restore plus a save that skips those refs. Adds tools/ci/ci-health so this is answerable without hand-written gh api calls: cache totals against quota, largest families with their share, duplicate keys across refs with the merge-queue waste called out, workflow duration percentiles by day, and PR open-to-merge latency. It also records the trap in reading those numbers: a low median duration usually means change detection skipped rows, not that builds got faster, so the report prints run counts and p90 alongside. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR separates CI cache restore and save operations, increases Bazel concurrency, and replaces the CI-health script with a Go CLI. The CLI collects GitHub Actions data, analyzes timing and cache metrics, and produces text or self-contained HTML reports with tests. ChangesCI health and cache management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CIHealth
participant GitHub
participant Dashboard
Operator->>CIHealth: Select report mode and options
CIHealth->>GitHub: Fetch runs, jobs, caches, and pull requests
GitHub-->>CIHealth: Return API data
CIHealth->>CIHealth: Analyze timing, diagnostics, and cache state
CIHealth->>Dashboard: Render self-contained report
Dashboard-->>Operator: Display CI health results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tools/ci/ci-health`:
- Around line 174-191: Add focused tests in tools/ci/test-ci-health covering
mocked gh responses, pagination behavior, percentile edge cases, and main()
command dispatch for workflow, merge-time, cache, and --all options. Keep the
tests targeted to the existing cache_report, workflow_report, merge_time_report,
and main symbols without changing tool behavior.
- Line 81: Update the GitHub API retrieval used by the cache, workflow-run, and
pull-request metrics to follow pagination and aggregate every page needed for
the report window. In particular, replace the single-page call around gh with a
pagination-aware fetch, then apply args.prs only as a client-side limit after
all relevant pull requests are collected, preserving existing metric
calculations.
- Around line 118-123: Update percentile to use a documented percentile
calculation method that interpolates between adjacent sorted values, ensuring
percentile([1, 2], 0.5) returns 1.5 and p90 for ten ascending values reflects
the 90th percentile rather than automatically selecting the maximum. Preserve
the existing empty-input return of 0.0.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee52de18-52b9-4966-9a97-e99e95c715b7
📒 Files selected for processing (3)
.github/workflows/bazel.yml.github/workflows/license-dependencies.ymltools/ci/ci-health
The duration report pulled `actions/runs?per_page=100` and filtered by workflow name in Python, so it sampled the last 100 runs across all workflows and kept whichever happened to be bazel. In practice that was 14 runs out of 1275, and it under-reported worse as the repo got busier. It now reads the per-workflow endpoint and pages through it. That truncation also produced a false conclusion: with only two days of runs visible there appeared to be no history to trend against. There are three full weeks. Adds `--dashboard`, which writes a self-contained HTML report and opens it. No CDN and no JavaScript dependency; the charts are inline SVG, so the file works offline and adds nothing to the dependency surface. The report separates queue wait from execution time, because the Actions UI shows only their sum. On current data that distinction matters: several matrix rows spend more wall clock waiting for a runner than building. Also drops the oldest week from the trend when the history window was truncated. Only the tail of that week is held, so its median came from an arbitrary slice and plotted as a misleading near-zero point. Adds focused tests for the pure analysis functions, one per defect the tool has actually shipped with. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
A 25-row merge-queue run needs three waves at max-parallel 8 and two at 12, and full-matrix runs are what set the p90 (20.1 min against a 6.8 min median). The cap exists to keep simultaneous actions/checkout downloads under GitHub's rate limit, which previously failed runs at "Set up job" with HTTP 429. That constraint is unchanged, so this is a measured step rather than a jump to 16: combined with the bazel-docker matrix the checkout burst goes from 12 to 16. Walk this back first if 429s reappear. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
balajinvda
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bazel.yml (1)
741-754: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winSave the caches after the test step, not between build and test.
The previous combined
actions/cache@v4step saved in the post-job phase, so the entry contained everything the build and the test step fetched.actions/cache/save@v4saves at the point where the step runs.bazel test //...can fetch further external repositories into~/.cache/bazel/_bazel_*/cache(test-only dependencies and toolchains reached only by the test configuration). Those fetches now land after the save, so the next run restores an incomplete repository cache and re-downloads them.Move this step below
bazel test //...(after line 876). The conditions themselves are correct.♻️ Proposed move
- - name: Save Bazel repository + disk caches - # Never from a merge-queue ref: gh-readonly-queue/** is deleted when the - # queue drains, so the entry is unrestorable but still occupies quota. - if: >- - steps.precheck.outputs.skip == 'false' - && steps.bazel_cache.outputs.cache-hit != 'true' - && !startsWith(github.ref, 'refs/heads/gh-readonly-queue/') - uses: actions/cache/save@v4 - with: - path: | - ~/.cache/bazel/_bazel_${{ env.USER || 'root' }}/install - ~/.cache/bazel/_bazel_${{ env.USER || 'root' }}/cache - key: bazel-${{ matrix.subtree.workdir == '.' && 'rootmodule' || matrix.subtree.id }}-${{ hashFiles(format('{0}/MODULE.bazel.lock', matrix.subtree.workdir), format('{0}/.bazelversion', matrix.subtree.workdir)) }} - - name: bazel test //...Then add the same step immediately after the
bazel test //...step:- name: Save Bazel repository + disk caches # Runs after build and test so the entry holds every fetched external # repository. Never from a merge-queue ref: gh-readonly-queue/** is # deleted when the queue drains, so the entry is unrestorable but still # occupies quota. if: >- always() && steps.precheck.outputs.skip == 'false' && steps.bazel_cache.outputs.cache-hit != 'true' && !startsWith(github.ref, 'refs/heads/gh-readonly-queue/') uses: actions/cache/save@v4 with: path: | ~/.cache/bazel/_bazel_${{ env.USER || 'root' }}/install ~/.cache/bazel/_bazel_${{ env.USER || 'root' }}/cache key: bazel-${{ matrix.subtree.workdir == '.' && 'rootmodule' || matrix.subtree.id }}-${{ hashFiles(format('{0}/MODULE.bazel.lock', matrix.subtree.workdir), format('{0}/.bazelversion', matrix.subtree.workdir)) }}Drop
always()if you want a failed test to skip the save.🤖 Prompt for 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. In @.github/workflows/bazel.yml around lines 741 - 754, Move the “Save Bazel repository + disk caches” step to immediately after the `bazel test //...` step so test-only downloads are included. Preserve its existing conditions, or add `always()` if failed tests should still save the cache; do not leave the original pre-test save step in place.
🧹 Nitpick comments (2)
tools/ci/test-ci-health (1)
59-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the API-shaped helpers.
The pure analysis functions are well covered.
gh_pagedandmerge_time_reportare not, and that is where thekey="items"defect ontools/ci/ci-healthline 568 sits. Add a case that stubsci.gh_jsonwith a bare list and a 100-item page, then asserts thatgh_pagedandmerge_time_reportaggregate correctly.As per coding guidelines, "For changed tool behavior, add or update focused tests."
Also applies to: 219-235
🤖 Prompt for 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. In `@tools/ci/test-ci-health` around lines 59 - 70, Add focused tests for the API-shaped helpers `gh_paged` and `merge_time_report` in the existing test suite. Stub `ci.gh_json` to return a bare list containing a full 100-item page, then assert both helpers aggregate the results correctly and preserve the expected behavior without assuming an `items` wrapper.Source: Coding guidelines
tools/ci/ci-health (1)
65-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the
ghcall.
subprocess.runhas no timeout.fetch_jobsissues these calls from eight threads, so one stalled request blocks the tool with no output. Add a timeout and convert the expiry into the existingRuntimeErrorpath, whichmainalready reports throughsys.exit.♻️ Proposed change
def gh_json(path, repo): cmd = ["gh", "api", f"repos/{repo}/{path}"] - out = subprocess.run(cmd, capture_output=True, text=True) + try: + out = subprocess.run(cmd, capture_output=True, text=True, timeout=60) + except subprocess.TimeoutExpired: + raise RuntimeError(f"gh api {path} timed out") if out.returncode != 0: raise RuntimeError(f"gh api {path} failed: {out.stderr.strip()}") return json.loads(out.stdout)🤖 Prompt for 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. In `@tools/ci/ci-health` around lines 65 - 70, Update gh_json to pass a finite timeout to subprocess.run and catch subprocess.TimeoutExpired, converting it into the existing RuntimeError path with a clear failure message. Preserve the current nonzero-return handling and JSON parsing behavior for completed requests.
🤖 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 `@tools/ci/ci-health`:
- Around line 567-575: Update merge_time_report so every --prs value uses
gh_paged with key=None for the pulls endpoint, including limits at or below 100,
and rely on its client-side limit. Remove the conditional gh_json path while
preserving the existing pull request filtering and latency calculation.
In `@tools/ci/test-ci-health`:
- Around line 16-23: Add tools/ci/test-ci-health to the appropriate CI job in
.github/workflows/bazel.yml, following the existing invocation pattern used for
tools/ci/test-bazel-cache-upload-mode. Preserve the executable entrypoint and
ensure the test runs as part of the workflow.
- Around line 25-36: Add an explicit import for importlib.machinery alongside
the existing imports in the test module so the SourceFileLoader reference used
during ci module loading is available reliably; leave the existing _spec
initialization and execution flow unchanged.
---
Outside diff comments:
In @.github/workflows/bazel.yml:
- Around line 741-754: Move the “Save Bazel repository + disk caches” step to
immediately after the `bazel test //...` step so test-only downloads are
included. Preserve its existing conditions, or add `always()` if failed tests
should still save the cache; do not leave the original pre-test save step in
place.
---
Nitpick comments:
In `@tools/ci/ci-health`:
- Around line 65-70: Update gh_json to pass a finite timeout to subprocess.run
and catch subprocess.TimeoutExpired, converting it into the existing
RuntimeError path with a clear failure message. Preserve the current
nonzero-return handling and JSON parsing behavior for completed requests.
In `@tools/ci/test-ci-health`:
- Around line 59-70: Add focused tests for the API-shaped helpers `gh_paged` and
`merge_time_report` in the existing test suite. Stub `ci.gh_json` to return a
bare list containing a full 100-item page, then assert both helpers aggregate
the results correctly and preserve the expected behavior without assuming an
`items` wrapper.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6f3bcbbc-02cf-45bc-840a-967d7306d398
📒 Files selected for processing (3)
.github/workflows/bazel.ymltools/ci/ci-healthtools/ci/test-ci-health
| """Tests for tools/ci/ci-health. | ||
|
|
||
| Every case here corresponds to a defect the tool actually shipped with at | ||
| some point. The analysis functions are pure, so none of this touches the | ||
| network. | ||
|
|
||
| Run: tools/ci/test-ci-health | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the executable bit and find the CI job that runs the tools tests.
set -euo pipefail
echo "== mode bits =="
fd -t f 'test-ci-health|test-bazel-cache-upload-mode' tools --exec-batch ls -l
echo "== workflow references to tools/ci tests =="
rg -n 'test-ci-health|test-bazel-cache-upload-mode|tools/ci' .github/workflowsRepository: NVIDIA/nvcf
Length of output: 3523
Add tools/ci/test-ci-health to CI workflow.
The file has the executable bit set. However, it is not invoked in any CI workflow. Follow the pattern of tools/ci/test-bazel-cache-upload-mode in .github/workflows/bazel.yml (line 89) to add an invocation. Per coding guidelines, update CI jobs when adding or changing a tool entrypoint.
🤖 Prompt for 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.
In `@tools/ci/test-ci-health` around lines 16 - 23, Add tools/ci/test-ci-health to
the appropriate CI job in .github/workflows/bazel.yml, following the existing
invocation pattern used for tools/ci/test-bazel-cache-upload-mode. Preserve the
executable entrypoint and ensure the test runs as part of the workflow.
Source: Coding guidelines
| import datetime as dt | ||
| import importlib.util | ||
| import os | ||
| import unittest | ||
|
|
||
| _HERE = os.path.dirname(os.path.abspath(__file__)) | ||
| _spec = importlib.util.spec_from_loader( | ||
| "ci_health", | ||
| importlib.machinery.SourceFileLoader("ci_health", os.path.join(_HERE, "ci-health")), | ||
| ) | ||
| ci = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(ci) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does importlib.util import importlib.machinery as a side effect in Python 3.13
💡 Result:
No, importlib.util does not import importlib.machinery as a mandatory side effect in Python 3.13 [1][2]. While importing modules in Python can sometimes trigger side effects if other packages or standard library modules (such as pkgutil or inspect) are involved [3][4], the source code for importlib.util in Python 3.13 does not contain an import statement for importlib.machinery [1]. Users should not rely on any automatic availability of importlib.machinery via importlib.util [3][5]. If your code requires functionality from importlib.machinery, you should explicitly import it [6].
Citations:
- 1: https://github.com/python/cpython/blob/3.13/Lib/importlib/util.py
- 2: https://docs.python.org/3/library/importlib.html
- 3: https://discuss.python.org/t/python3-11-importlib-no-longer-exposes-util/25641
- 4: https://github.com/python/cpython/blob/main/Lib/pkgutil.py
- 5: Missing
importlibre-exported top-level modules python/typeshed#10746 - 6: Improve the docs regarding the migration from imp to importlib python/cpython#104212
🌐 Web query:
Python 3.13 importlib.util SourceFileLoader AttributeError
💡 Result:
The AttributeError related to importlib.util.SourceFileLoader in Python 3.13 typically arises in two distinct contexts: the deprecation and removal of older methods, or issues with outdated packaging dependencies. 1. Deprecation of load_module: The most common cause of AttributeError when working with SourceFileLoader is attempting to call.load_module. This method has been deprecated since Python 3.6 and is slated for removal. In later Python versions, objects created via SourceFileLoader do not possess a.load_module attribute [1][2]. To fix this, you should transition to the modern importlib API using exec_module [1]. The recommended pattern for loading a module from a file path is as follows [3][4][5]: import importlib.util import importlib.machinery import sys def load_source(module_name, file_path): loader = importlib.machinery.SourceFileLoader(module_name, file_path) spec = importlib.util.spec_from_file_location(module_name, file_path, loader=loader) module = importlib.util.module_from_spec(spec) sys.modules[module_name] = module spec.loader.exec_module(module) return module 2. Packaging and Environment Issues (importlib._bootstrap): If you encounter an error stating: AttributeError: module 'importlib._bootstrap' has no attribute 'SourceFileLoader', this is almost always caused by an outdated or broken setuptools or distribute installation [6][7]. This often happens when installing old, unmaintained packages that force a downgrade of setuptools [8]. To resolve this, upgrade your packaging tools using: python -m pip install --upgrade setuptools pip If the issue persists in a virtual environment, ensure you are not using a broken environment by recreating it and avoiding the installation of legacy, unmaintained packages [8].
Citations:
- 1: https://docs.python.org/3/library/importlib.html
- 2: https://bugzilla.redhat.com/show_bug.cgi?id=2424309
- 3: Improve the docs regarding the migration from imp to importlib python/cpython#104212
- 4: gh-121607: Edited source file import recipe to make it more clear python/cpython#121519
- 5: Improve/clarify importlib recipe for loading code from a source file path python/cpython#121607
- 6: https://stackoverflow.com/questions/78857313/cannot-install-python3-distribute-package-no-attribute-sourcefileloader
- 7: https://stackoverflow.com/questions/44761958/using-pip3-module-importlib-bootstrap-has-no-attribute-sourcefileloader
- 8: https://stackoverflow.com/questions/69343581/every-action-shows-attributeerror-module-importlib-bootstrap-has-no-attribu
🏁 Script executed:
# Check if the file snippet shows actual code that would fail without the import
cat -n <<'EOF'
import datetime as dt
import importlib.util
import os
import unittest
_HERE = os.path.dirname(os.path.abspath(__file__))
_spec = importlib.util.spec_from_loader(
"ci_health",
importlib.machinery.SourceFileLoader("ci_health", os.path.join(_HERE, "ci-health")),
)
ci = importlib.util.module_from_spec(_spec)
_spec.loader.exec_module(ci)
EOFRepository: NVIDIA/nvcf
Length of output: 577
Add explicit import for importlib.machinery.
The code on line 9 uses importlib.machinery.SourceFileLoader, but only importlib.util is imported. importlib.machinery must be imported explicitly; it is not guaranteed to be available as a side effect of importing importlib.util. In Python 3.13 and later, the code will raise AttributeError on access without the explicit import.
🛡️ Proposed fix
import datetime as dt
+import importlib.machinery
import importlib.util
import os
import unittest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import datetime as dt | |
| import importlib.util | |
| import os | |
| import unittest | |
| _HERE = os.path.dirname(os.path.abspath(__file__)) | |
| _spec = importlib.util.spec_from_loader( | |
| "ci_health", | |
| importlib.machinery.SourceFileLoader("ci_health", os.path.join(_HERE, "ci-health")), | |
| ) | |
| ci = importlib.util.module_from_spec(_spec) | |
| _spec.loader.exec_module(ci) | |
| import datetime as dt | |
| import importlib.machinery | |
| import importlib.util | |
| import os | |
| import unittest | |
| _HERE = os.path.dirname(os.path.abspath(__file__)) | |
| _spec = importlib.util.spec_from_loader( | |
| "ci_health", | |
| importlib.machinery.SourceFileLoader("ci_health", os.path.join(_HERE, "ci-health")), | |
| ) | |
| ci = importlib.util.module_from_spec(_spec) | |
| _spec.loader.exec_module(ci) |
🤖 Prompt for 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.
In `@tools/ci/test-ci-health` around lines 25 - 36, Add an explicit import for
importlib.machinery alongside the existing imports in the test module so the
SourceFileLoader reference used during ci module loading is available reliably;
leave the existing _spec initialization and execution flow unchanged.
| @@ -0,0 +1,653 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Can you rewrite this in bash or go
There was a problem hiding this comment.
Done, rewritten in Go: tools/ci-health/ as its own module, with tools/ci/ci-health kept as the stable entrypoint per the tools/AGENTS.md layout guidance.
You were right that it shouldn't have been Python. tools/AGENTS.md calls for Go on non-trivial tooling and warns that some CI environments here don't guarantee a Python interpreter, and this is structured API parsing plus concurrent fetches, which is squarely that.
Behaviour is unchanged, verified against live data. No third-party dependencies; the dashboard is inline SVG rather than a charting library, so it stays self-contained and offline.
The port also picked up two real bugs from the Python version, both now covered by tests:
- The
pullsendpoint returns a bare array, so paging it with a wrapper key crashed once--prswent past one page. - The cache listing read a single page, silently capping the report at 100 entries.
go test ./tools/ci-health/... is 37 tests.
Review feedback: tools/AGENTS.md prefers Go for non-trivial repo tooling and says to avoid new Python, partly because some CI environments here do not guarantee a Python interpreter. This tool is squarely in that category: structured API parsing, concurrent fetches, and logic that benefits from unit tests. It should not have been Python to begin with. Behaviour is unchanged. The Go build produces the same ranked causes, the same weekly trend, and a visually identical dashboard against live data. Layout follows the existing convention: the tool lives in its own module at tools/ci-health/, with tools/ci/ci-health kept as the stable entrypoint. Two fixes carried over from review of the Python version: - The pulls endpoint returns a bare array, so paging it with a wrapper key crashed once --prs exceeded one page. The Go paging helper takes an empty key for bare-array endpoints, and a test covers both response shapes. - The cache listing read a single page, silently capping the report at 100 entries. It now pages. 37 tests, no third-party dependencies. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@tools/ci-health/github.go`:
- Around line 170-195: Update fetchJobs to paginate each run’s jobs endpoint
until all pages are retrieved, preserving the worker limit and aggregating every
page’s Jobs into that run’s result. Use the API’s pagination metadata or
next-page indicator and stop when no further page exists. Add a focused test
covering a run whose jobs span multiple pages and verifying all jobs are
returned.
In `@tools/ci-health/main.go`:
- Around line 156-162: Maintain a separate run slice matching the --runs
job-timing window, and pass that slice to diagnose instead of runsList. Update
printWhy and the dashboard diagnostic summary to use the same job-sample slice,
while retaining runsList exclusively for trend-chart data.
- Around line 143-154: Update the command flow around the usage fetch,
fetchCaches, and summariseCaches calls to compute a needCache condition and
perform those requests only for the default, --why, --all, and dashboard modes.
Ensure --durations and --merge-times bypass all cache access while preserving
the existing cache summary behavior for cache-dependent modes.
- Around line 89-93: Validate the parsed count options before dispatch so
--runs, --history, --weeks, and --prs reject negative values instead of reaching
slicing logic. Update the flag-handling flow around the IntVar bindings and
return a clear validation error or usage failure for any negative value, while
preserving valid zero and positive inputs.
In `@tools/ci-health/render.go`:
- Around line 183-196: The quotaBar function currently caps the calculated used
percentage before displaying it. Preserve the raw percentage for threshold
selection and the quotatxt output, while introducing a separately capped value
only for the CSS width attribute. Add a regression test covering utilization
above quota, such as 12 GB, verifying the text reports 120% while the bar width
remains 100%.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 79e3a8e1-7fba-4df1-a6ff-3ec263ba77c8
📒 Files selected for processing (9)
tools/ci-health/.gitignoretools/ci-health/analysis.gotools/ci-health/analysis_test.gotools/ci-health/github.gotools/ci-health/github_test.gotools/ci-health/go.modtools/ci-health/main.gotools/ci-health/render.gotools/ci/ci-health
| flag.IntVar(&o.runs, "runs", 60, "runs to pull per-job detail for") | ||
| flag.IntVar(&o.history, "history", 1000, "runs to trend over") | ||
| flag.IntVar(&o.weeks, "weeks", 12, "weeks of trend to print") | ||
| flag.IntVar(&o.prs, "prs", 100, "merged PRs to sample") | ||
| flag.BoolVar(&o.noOpen, "no-open", false, "do not launch a browser") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject negative count flags before slicing.
flag.IntVar accepts negative values. --runs=-1 panics at Line 138. --weeks=-1 panics at Line 281. Reject negative values for --runs, --history, --weeks, and --prs before dispatch.
Proposed fix
func run() error {
o := parseFlags()
+ if o.runs < 0 || o.history < 0 || o.weeks < 0 || o.prs < 0 {
+ return fmt.Errorf("count flags must be non-negative")
+ }🤖 Prompt for 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.
In `@tools/ci-health/main.go` around lines 89 - 93, Validate the parsed count
options before dispatch so --runs, --history, --weeks, and --prs reject negative
values instead of reaching slicing logic. Update the flag-handling flow around
the IntVar bindings and return a clear validation error or usage failure for any
negative value, while preserving valid zero and positive inputs.
Four issues from review, all real, all verified rather than taken on trust. fetchJobs requested per_page=100 but never asked for page 2, so any run with more than 100 jobs reported partial timings. Wide matrix runs are exactly the ones whose numbers matter, so this silently understated the busiest runs. It now pages until total_count is reached. Negative counts panicked instead of erroring: --runs, --history, --weeks and --prs all reach slice bounds, and `--runs=-1` died with "slice bounds out of range [:-1]". Reproduced before fixing. They are now rejected with a message naming the flag. --durations and --merge-times read no cache data but still fetched it, so they failed whenever cache access failed, for reports that never used the result. The cache calls are now made only for the modes that read them. diagnose divided job medians drawn from the --runs sample by a wall-clock median drawn from the full --history window. Mixing two windows can misstate each cause's share and reorder them. Diagnosis now runs on the sampled window; the full history still feeds the trend charts, where it belongs. Tests cover pagination across two pages, the single-page stop, and rejection of every negative count flag. 40 tests pass. Verified live afterwards: --runs=-1 now prints a clean error, --durations completes without touching the cache, and --why reports over the window it actually sampled. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
3 participants
Why
Two questions came up that nobody could answer without hand-rolling
gh apicalls: are the caches healthy, and why is the build slow.
The caches were not healthy. The repo sat at 9.42 GB of its 10 GB quota (94%,
and 97% by the time this was written), which means GitHub is evicting
least-recently-used entries continuously. That failure mode is invisible in a
green pipeline: jobs still pass, they just stop being fast. Two specific causes:
dependency-docs-bazelheld 4.19 GB across three copies, 42% of the entirequota, because every branch saved its own full-size copy.
gh-readonly-queue/**refs. Those branches are deleted whenthe merge queue drains, so the entries can never be restored, but they still
count against quota until evicted.
Answering the second question needed a tool, and the first version of that tool
was wrong in a way worth calling out: it sampled
actions/runs?per_page=100andfiltered by workflow name client-side, so it saw 14 of 1275 bazel runs. That led
me to state there was no history to trend against. There are three full weeks.
What changed
Cache:
actions/cache/restore+ guardedactions/cache/save. Dependency-docs saves only frommain; bazel skipsmerge-queue refs. Restores are unchanged everywhere, so no job gets colder.
tools/ci-health(Go, withtools/ci/ci-healthas the stable entrypoint):--dashboardwrites a self-contained HTML report and opens it. Inline SVG,no CDN and no JavaScript dependency, so it works offline and adds nothing to
the dependency surface.
and the distinction turns out to matter:
stargatespends roughly 6.1 minwaiting for a runner against roughly 3.4 min building.
only the tail of that week is held, so its median plotted as a misleading
near-zero point.
misread as a speedup.
Matrix:
max-parallel8 to 12 on the bazel matrix.Customer Release Notes
Not customer visible.
Plan Summary
Not applicable.
Usage
Testing
go test ./tools/ci-health/...: 37 tests, no third-party dependencies. Eachcase corresponds to a defect this tool actually shipped with, including the two
found in review: bare-array pagination and single-page cache listing. Coverage
includes percentile interpolation, the partial-week drop, skipped-matrix
classification, the queue/execution split, long-pole attribution excluding gate
jobs, tolerance of failed job fetches, and that the dashboard stays
self-contained.
Every subcommand was run against live data, including error paths. The dashboard
was rendered headless and inspected, not just parsed.
The full 25-row matrix passed on this branch with no HTTP 429s at the new cap.
Notes
On the
max-parallelchange, the measured result was weaker than predicted.Comparing two 25-row
pull_requestruns, same branch and same runner class:Wall clock improved 9%, but per-row time rose 22%. Both runs used GitHub-hosted
runners (separate VMs, so not CPU contention between rows); the shared remote
cache endpoint is the likeliest explanation. This is n=1 on each side, so it may
be noise. Worth a week of samples via
tools/ci/ci-health --durationsbeforeconcluding either way, and easy to revert if not.
The cap also exists to keep simultaneous
actions/checkoutdownloads underGitHub's rate limit, which previously failed runs at "Set up job" with HTTP 429.
That is why this is 12 and not 16: combined with the
bazel-dockermatrix thecheckout burst goes from 12 to 16. If 429s reappear, walk this back first.
This fix stops the bleed but does not reclaim space. Deleting the two stranded
merge-queue entries frees 1.90 GB immediately.
GitHub Pages is not enabled on this repo, and enabling it would publish the
dashboard publicly. A scheduled workflow uploading the HTML as an artifact is
the better route if we want it without running the tool locally.
References
None
Related Merge Requests/Pull Requests
None
Dependencies
None. The dashboard is inline SVG specifically to avoid adding a charting
library, and the Go module has no requires.
Github commit:
fix(ci): cut cache-quota waste, add a self-serve build-health dashboard
Co-authored-by: Balaji Ganesan bganesan@nvidia.com
Summary by CodeRabbit
New Features
Chores