Cut PR feedback time from 56 to about 35 minutes - #590
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes GitHub Actions workflow performance and cache utilization to reduce end-to-end PR feedback time, primarily by freeing Actions cache quota, improving sanitizer build cache effectiveness, parallelizing coverage capture, and removing non-gating work from PR-triggered runs.
Changes:
- Reduce GitHub Actions cache pressure (Docker layer cache
mode=min, larger sanitizer ccache limits, sanitizer-g1to shrink DWARF-heavy artifacts). - Speed up CI execution (merge Jazzy build+test into one job; run
lcov --capturewith--parallel). - Limit long-running, non-gating jobs to
main/scheduled contexts (coverage job onmainonly; Pixi moved off PRs + nightly schedule).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/ros2_medkit_cmake/cmake/ROS2MedkitSanitizers.cmake |
Adds -g1 to sanitizer builds to reduce debug-info bloat and improve cache behavior. |
QUALITY_DECLARATION.md |
Updates CI policy documentation to reflect coverage running on main only and PR gating via static coverage-scope checks. |
CONTRIBUTING.md |
Updates contributor-facing CI/CD description to match new CI job structure and responsibilities. |
.github/workflows/quality.yml |
Increases sanitizer job ccache sizes to avoid self-eviction during builds. |
.github/workflows/pixi.yml |
Moves Pixi workflow off PRs; adds nightly schedule as a backstop. |
.github/workflows/docker-publish.yml |
Switches Docker Buildx cache export to mode=min to reduce Actions cache usage. |
.github/workflows/ci.yml |
Parallelizes lcov --capture, merges Jazzy build+test into a single job, and restricts coverage job to main pushes while fixing missing test-failure gating. |
mfaferek93
reviewed
Aug 5, 2026
Moving the coverage job to pushes on main took the last pull request build that compiled without -DNDEBUG, so every assert() would have been compiled out of pull request builds: the remaining jobs are Release or RelWithDebInfo. ROS2MedkitSanitizers.cmake now passes -UNDEBUG, which puts them back in the two sanitizer jobs at no extra CI time, since those already run on every pull request over a superset of the test selection the coverage job ran. The flag is directory-scoped, so it also enables the assertions in headers compiled into a participating package - nlohmann/json routes JSON_ASSERT to assert, and the vendored cpp-httplib and dynmsg carry their own. That is the intent: those checks are what a sanitizer build should be running. The cache sizes in the sanitizer jobs rest on ccache no longer evicting during its own build, and nothing read the numbers that would show it. scripts/ccache_report.sh parses ccache --print-stats after every ccache-backed build, writes hit rate, size and cleanups to the job summary, and raises a workflow warning for the two states that make a cache useless: cleanups above zero, where the ceiling is too small or the Actions quota is evicting the entry, and a hit rate under 50% with no cleanups, where the cache was never restored and the key or its restore-keys prefix is wrong. It does not fail the step, because a cold cache is legitimate. The CI documentation was inaccurate in several places and is corrected: the quality declaration listed a coverage job that no longer runs on pull requests, called the build "full" when all three distro jobs skip ros2_medkit_opcua, described clang-tidy as a full compilation-database sweep when it is incremental on a pull request, omitted the Sphinx and OPC-UA workflows and two source gates that do gate pull requests, overstated sanitizer instrumentation to packages that do not include the module, and asserted a merge gate that no required status check enforces. CONTRIBUTING claimed every CI job uses ccache. The docker-publish comment claimed mode=min loses nothing, when it drops the builder stage; what a source-touching push actually loses is the build-deps apt layer, since the COPY src/ layers already invalidated the build layer under mode=max. The three ~27 GB figures in quality.yml are marked as measured before -g1.
The first run of the report showed the threshold was wrong. Three jobs reported cleanups: jazzy-test at 98% of its 500M ceiling with 1672 of them and 16% hits, jazzy-tsan at 47% of 1.5G with 186, and humble at 15% of 500M with 4. Only the first is the state the sizes were raised to avoid. ccache trims a subdirectory when it passes max_size/16, so a nearly empty cache reports a few cleanups and it means nothing, and a warning that fires there teaches people to ignore the warning. The eviction warning now needs the cache to be at least 90% full as well, and the low-hit-rate warning becomes the else branch, so exactly one of the two can fire. Its text now covers the second legitimate cause: a branch that changed a compiler flag invalidates the objects and the next run recovers on its own, which is what jazzy-tsan is showing. scripts/test_ccache_report.sh pins this against the numbers the six jobs actually reported, plus a full-but-clean cache, a step that compiled nothing, and one pinned cache that is also cold. It stubs ccache on PATH so the script runs unmodified. Against the previous threshold it fails on the humble and jazzy-tsan rows, which is the point. It runs in format-lint next to the other source gates.
ccache keeps its counters inside the cache directory and actions/cache restores that directory whole, counters included. Read without preparation they are the running total over every job that has fed the entry. The report added earlier read them that way, so its hit rates and cleanup counts described a cache lineage rather than a build, which is not the quantity either warning talks about. Verified directly: one extra compilation against an existing cache moved cache_miss from 375 to 376 rather than starting from zero. Every build step that reports now runs ccache -z first, so the numbers describe that build alone. format-lint gains a report as well, since it also builds the workspace through ccache and had no reason to be the one build nobody measured. Cache size and max size are current state rather than counters, so the fill percentage the eviction warning keys on was unaffected.
Its key was ccache-jazzy-<sha> with a restore-keys prefix of ccache-jazzy-, which also matches ccache-jazzy-asan-, -tsan-, -lint- and -tidy-. restore-keys takes the most recently created match, so the job loaded whichever other Jazzy cache finished last. Measured on a run: "Cache restored from key: ccache-jazzy-asan-818af7de", 2 GB of instrumented objects this job cannot use, 42 hits out of 411 calls, and 219 cleanups as its own Release objects fought for the 500M ceiling on top of them. It was the critical path at 41m7s while the ASan job it had robbed finished in 36m55s at 99% hits. The prefix is now ccache-jazzy-test-, which matches nothing else. The report gains the shape that describes this, because "raise CCACHE_MAXSIZE" was the wrong advice for it: a cache both full and useless was filled by something other than this build, and the warning now says to check the restore-keys prefix. A cache that evicts while still serving most of its calls keeps the old message. It also gains a fallback for the max cache size. The Jammy image ships ccache 4.5, which omits max_cache_size_kibibyte from --print-stats, so the fill read 0 on the Humble job and no eviction warning could ever fire there. The configured value is parsed instead, accepting both "500.0M" and "500.0 MB", in powers of ten as ccache counts them. Both are pinned in the test, which now carries the per-build numbers of all six reporting jobs. Removing the fallback fails the humble-old row; removing the new branch fails the foreign row.
mfaferek93
approved these changes
Aug 6, 2026
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
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.
Summary
Targets the 56-minute pull request feedback time measured in the issue, with an expected
result of 34 to 37 minutes. No test and no distribution is dropped from pull requests. Two
things do change and are called out below: the coverage job moves to
main, and the imagepublish job gives up its builder layer cache.
Measured on this branch. Each run changed something the caches depended on, so read them in
order:
sanitizer-asanjazzy-testjazzy-testwas restoring the ASan cache, see change 10jazzy-testThe sanitizer jobs reach the expected figure from run 2 onwards, which is what the issue
identified as 64% of the critical path.
jazzy-testhas not yet had a run with a warm cacheof its own; the next push is its first.
Ten changes, in the order they depend on each other:
docker-publish.yml:cache-togoes frommode=maxtomode=min. It was holding about3.1 GB of intermediate layers in a repository cache that is already over its 10 GB limit,
for a job that only runs on pushes to
main. Nothing else can be fixed until this freesspace. This costs build time in that job. The
Dockerfileis two-stage andmode=minexports only the final image, so builder-stage layers are not restored at all. In
practice the loss is smaller than it sounds: the
COPY src/layers sit above therosdep+colcon buildlayer, so any push touchingsrc/already invalidated thatlayer under
mode=maxtoo. What such a push genuinely loses is the build-depsapt-getlayer. The job is not on the pull request path, and the quota it was holding is what
every pull request needs.
ROS2MedkitSanitizers.cmake: adds-g1to sanitizer builds. The module alreadyoverrode the build type's optimisation level but never its debug level, so
RelWithDebInfokept full debug info and the instrumented tree grew to about 27 GB,mostly DWARF. On one translation unit the object shrinks by 45% and its debug info by
72%.
quality.yml:CCACHE_MAXSIZEgoes to 2G for ASan and 1.5G for TSan. At 500M both jobsevicted hundreds of objects during their own build and never got above 18% hits.
ci.yml:lcov --capturegets--parallel. It was more than 99% of the 12.7-minutecoverage report step and ran on one core.
ci.yml:jazzy-buildis merged intojazzy-test. The build artifact had exactly oneconsumer, so the split paid about 4.7 minutes of repeated container and ROS setup for no
parallel work. The job id stays
jazzy-test, sonotify-demosneeds no change.ci.yml: thecoveragejob runs on pushes tomainonly, and gains--return-code-on-test-failure. It was missing that flag while the two other test jobshave it, so a failing test there could not fail CI. The job read as a gate without being
one.
ROS2MedkitSanitizers.cmake: adds-UNDEBUG. This is the compensation for change 6.The coverage job was the last pull request build compiling without
-DNDEBUG, so on itsown that change would have taken every
assert()out of pull request builds, since theremaining jobs are
ReleaseorRelWithDebInfo. The two sanitizer jobs already run onevery pull request, over a superset of the test selection the coverage job used, so
putting the asserts there costs no CI time. Note the reach: the flag is directory-scoped,
so it also enables assertions in headers compiled into a participating package.
nlohmann/jsonroutesJSON_ASSERTtoassert, and the vendoredcpp-httplibanddynmsgcarry their own. That is the intent, but it does mean a latent bug in a headernow shows up as an abort in the sanitizer jobs and nowhere else.
pixi.yml: runs on pushes tomainplus a nightly cron instead of on every pullrequest. It is
continue-on-error, so it never gated a pull request, and its cache isonly written on
main, so every pull request paid a 30.8-minute cold build.scripts/ccache_report.sh: reports what ccache actually did. Changes 1 and 3 rest on theclaim that ccache stops evicting during its own build, and nothing read the numbers that
would show whether it worked. Every ccache-backed build step now runs
ccache -zbeforecompiling and writes hit rate, cache size, fill level and cleanup count to the job
summary afterwards. The zeroing matters: ccache keeps its counters inside the cache
directory and
actions/cacherestores that directory whole, so read as they come thenumbers are the running total over every job that fed the entry rather than what this
build did.
It raises a workflow warning in two states. A cache at 90% of its ceiling or more that is
still running cleanups is discarding objects the same build produced, so the ceiling is
too small or the Actions quota is evicting the entry. A hit rate under 50% without that
ceiling pressure means the cache was not restored, or the branch changed a compiler flag
and invalidated the objects. Cleanups on their own prove nothing, because ccache trims a
subdirectory when it passes
max_size/16and a nearly empty cache reports a few. Thescript never fails the step, because a cold cache is a legitimate state.
ci.yml:jazzy-testgets its own ccache key prefix. It usedccache-jazzy-, whichalso matches
ccache-jazzy-asan-,-tsan-,-lint-and-tidy-, andrestore-keystakes the most recently created match. Run 2 shows it loading
ccache-jazzy-asan-818af7de: 2 GB of instrumented objects it cannot use, 42 hits out of411 calls, then 219 cleanups as its own
Releaseobjects fought for the 500M ceiling ontop of them. It was the critical path at 41m7s while the ASan job whose cache it had
taken finished in 36m55s at 99% hits. The prefix is now
ccache-jazzy-test-. This isolder than this branch; it only became visible once change 9 made the numbers per-build.
Pull requests keep the static coverage scope check, which is in the Quality workflow and
fails if a package compiles production C++ without coverage instrumentation.
QUALITY_DECLARATION.mdandCONTRIBUTING.mdare corrected against what the workflowsactually do, including checks that were already inaccurate before this branch: the build is
not "full" (all three distro jobs skip
ros2_medkit_opcua), clang-tidy is incremental on apull request, the Sphinx and OPC-UA workflows and two source gates also gate pull requests,
sanitizer instrumentation does not reach packages that omit the module, and no required
status check enforces any of it.
Issue
Type
Testing
lcov --parallelproduces the same file as the serial run. Same md5, all 163 source filerecords equal, same line and function totals. The whole downstream chain of filters and
genhtmlruns on the parallel output.-g1keeps what a sanitizer report needs. An ASan stack buffer overflow built with the newflags still names file and line in every frame and still names the overflowed variable,
because that name comes from the frame descriptor the sanitizer embeds, not from DWARF.
What is lost is inspecting local variables in a debugger on a core file.
-UNDEBUGwas verified by running the suite with the asserts live. The workspace was builtRelWithDebInfowith the config flags overridden to-O1 -g1, which is what the sanitizerjobs compile with minus the instrumentation, and with no
-DNDEBUGin any compile command.The coverage job's test selection then ran over 18 packages: 4571 tests, and no assertion
fired anywhere. No
Assertion ... failed, noSIGABRT, no core dump. One integration testfailed,
test_external_app_fault_rollup, and it is not related: it fails at the same ratewith the asserts compiled out (2 of 10 passing either way, run alternately to keep machine
load equal), its log carries the
Buffer is empty, cannot create bag filewarning, and abranch that fixes that empty-buffer path passes it 10 times out of 10 on the same machine.
ccache_report.shhas its thresholds pinned byscripts/test_ccache_report.sh, which runsin
format-lint. It stubs ccache onPATHso the script runs unmodified, and its rows arethe numbers six jobs reported on the first run of this branch plus three constructed cases:
a full cache that is not evicting, a step that compiled nothing, and a cache that is both
pinned and cold. Against the earlier threshold, which warned on any cleanup at all, it
fails on two of those rows, which is what makes it a test rather than a constant.
Both the threshold and the zeroing came out of that first run rather than from reading the
code. It
reported cleanups in three jobs, one of them a cache at 15% of its ceiling, which is ccache
trimming a subdirectory and not a ceiling worth raising. Chasing that down showed the
counters were cumulative, which is what change 9 now handles.
The speedup itself cannot be measured locally and is not claimed here. The
ccacherows inthe job summaries are the thing to read, and from the second run onwards they describe a
single build: cleanups should be zero or the cache well below its ceiling, and the hit rate
well above the 50% floor. For scale, a cold full
Releasebuild of the workspace storesabout 101 MiB across 376 entries locally, so the 500M ceilings hold several builds' worth
of objects.
Every workflow file still parses, and every
needs:edge andneeds.<job>.*expression inall seven workflows was resolved against the declared job list, with no broken references.
Checklist