Skip to content

ASan+UBSan workflow lane Pt2 #1728

Merged
elalish merged 18 commits into
elalish:masterfrom
AnshulPatil2005:ci/sanitizer-pt2
Jun 10, 2026
Merged

ASan+UBSan workflow lane Pt2 #1728
elalish merged 18 commits into
elalish:masterfrom
AnshulPatil2005:ci/sanitizer-pt2

Conversation

@AnshulPatil2005

Copy link
Copy Markdown
Contributor

Summary

some improvements on #1666

Improvements

  1. Move gtest filter to env var
    Use SANITIZER_GTEST_FILTER in workflow so test-set edits don’t require command-line rewrites.

  2. Extract filter to script
    Store sanitizer test sets in scripts/sanitizer_cases.sh and source it from workflow for easier tuning.

  3. Add explicit sanitizer options
    Use stable defaults such as:

  • ASAN_OPTIONS=detect_container_overflow=0:strict_init_order=1
  • UBSAN_OPTIONS=print_stacktrace=1
  1. Add clear failure diagnostics
    On failure, print a short local reproduction block directly in job logs.

  2. Add runtime guardrails
    Track runtime and keep the lane within budget; make test-set trimming/expansion data-driven.

  3. Split core and extended subsets
    Run a small core subset on PRs; keep broader subset for scheduled/manual runs.

  4. Pin compiler/setup consistency
    Keep clang setup explicit to reduce drift and cross-runner variability.

  5. (Optional) Separate build/test stages with artifact reuse
    Improve retry speed and log clarity by decoupling build from execution.

Why this is needed

The sanitizer lane is now part of routine CI, so the next step is to make it:

  • simpler to update,
  • less noisy/flaky,
  • easier to debug when failures happen,
  • predictable in runtime.

Scope

  • CI/workflow + helper-script refactor only
  • no product/runtime behavior changes in manifold code

@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.90%. Comparing base (8cab4b6) to head (b1fb88e).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1728      +/-   ##
==========================================
- Coverage   95.13%   94.90%   -0.24%     
==========================================
  Files          37       37              
  Lines        8291     8406     +115     
==========================================
+ Hits         7888     7978      +90     
- Misses        403      428      +25     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pca006132

Copy link
Copy Markdown
Collaborator

do you want to cache the build when src and bindings/c is not changed? otherwise I don't see how this can be useful

@AnshulPatil2005 AnshulPatil2005 marked this pull request as ready for review May 22, 2026 07:40
@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

Local sanitizer timing looks good: core subset runs in ~56 ms (6 tests) and extended subset in ~119 ms (7 tests), so the core/extended split keeps PR checks fast

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

Sanitizer build (ASan+UBSan) builds the sanitizer binaries (reusing cache/artifacts when available), and Sanitizer (ASan+UBSan) runs the sanitizer test subset against that build.

@elalish elalish left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'll defer to @pca006132 on the details of this one.

Comment thread .github/workflows/manifold.yml Outdated
@pca006132

Copy link
Copy Markdown
Collaborator
  1. How much time does it take to build the sanitized version of the library? If it is in a couple minutes, I don't think it is worth making it cached. Caching stuff feels quite fragile to me.
  2. I doubt if those additional data you print to the output can aid error discovery. I think we should just keep it simple, maybe add some information to documentation instead of making this shell script.

@AnshulPatil2005

AnshulPatil2005 commented May 23, 2026

Copy link
Copy Markdown
Contributor Author
  1. How much time does it take to build the sanitized version of the library? If it is in a couple minutes, I don't think it is worth making it cached. Caching stuff feels quite fragile to me.

Adding cache changed the time from around 5 minutes to some 19-20 seconds.
I can add a cache toggle so caching can be turned off for debugging when needed ? it will add some workflow complexity though

  1. I doubt if those additional data you print to the output can aid error discovery. I think we should just keep it simple, maybe add some information to documentation instead of making this shell script.

Yes, The extra output is not actually providing enough value as i expected earlier before execution. I’ll remove that diagnostic output, and add any useful guidance into documentation.

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

@pca006132 @elalish
Should I add anything else in this PR, or is the current sanitizer CI scope good for now?

@elalish elalish left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think the scope is fine for now as long as we're planning to expand it in the next PR.

Comment thread README.md Outdated
Comment thread scripts/ci/sanitizer_test.sh Outdated
@pca006132

Copy link
Copy Markdown
Collaborator

not really required, but I think it will be nice if you can try the build on docker, look at the rpath issue. maybe it can be fixed by cmake args.

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

not really required, but I think it will be nice if you can try the build on docker, look at the rpath issue. maybe it can be fixed by cmake args.

I'll do it, and report you the findings or fixes before implementing

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

not really required, but I think it will be nice if you can try the build on docker, look at the rpath issue. maybe it can be fixed by cmake args.

I checked this in Docker and the issue was the runtime path after the artifact gets downloaded to a different location. The sanitizer build was embedding absolute build-time library paths, so manifold_test could not find libsamples.so.3 without LD_LIBRARY_PATH.

So I changed the build to use -DCMAKE_BUILD_RPATH_USE_ORIGIN=ON, which makes the binaries use $ORIGIN-relative paths instead. Since that passed in CI, I then removed the LD_LIBRARY_PATH workaround from the test script and now it just relies on the embedded runtime path.

@pca006132

Copy link
Copy Markdown
Collaborator

So I think this is good to go? Do you have anything else?

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

So I think this is good to go? Do you have anything else?

Its done for this pr, we can make a follow up later if needed.

@elalish elalish merged commit 324bec7 into elalish:master Jun 10, 2026
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants