Skip to content

Revert "Add Valgrind memcheck (ruby_memcheck) for the C extension" - #150

Merged
SpringMT merged 1 commit into
mainfrom
revert-147-feature/valgrind-memcheck
Aug 9, 2026
Merged

Revert "Add Valgrind memcheck (ruby_memcheck) for the C extension"#150
SpringMT merged 1 commit into
mainfrom
revert-147-feature/valgrind-memcheck

Conversation

@SpringMT

@SpringMT SpringMT commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Reverts #147

@SpringMT
SpringMT merged commit 456ca48 into main Aug 9, 2026
11 checks passed
@SpringMT
SpringMT deleted the revert-147-feature/valgrind-memcheck branch August 9, 2026 13:28
Watson1978 added a commit to Watson1978/zstd-ruby that referenced this pull request Aug 9, 2026
Running the suite under Valgrind on a Ruby C extension is normally impractical:
the interpreter itself produces a large volume of reports that have nothing to
do with the extension. ruby_memcheck wraps Valgrind and only surfaces errors
whose stack trace passes through the extension's .so, which makes the output
usable. It is the same tool Shopify runs on nokogiri and liquid-c.

    rake spec:valgrind

This reworks SpringMT#147, which was reverted in SpringMT#150 because it broke CI: the Gemfile
guarded the dependency on Linux but not on the Ruby version, and ruby_memcheck
3.x requires Ruby >= 3.0, so `bundle install` failed on the 2.7 entry of the
matrix. The guard now covers both. Verified on Ruby 2.7.8 locally: bundle
install, rake compile and rspec all pass, and the Rakefile simply does not
define the task there.

The memcheck run gets its own workflow rather than a job inside Ruby.yml, so a
report from it cannot turn the main test matrix red. It pins one Ruby, installs
Valgrind, and carries a 30 minute job timeout; a cold run -- extension build
plus the suite under memcheck -- takes about 1m45s locally.

The suite reports nothing on the current tree, so no suppression file is
needed. The task does have teeth: run against the use-after-free of a stream's
borrowed CDict/DDict it reports 113 Invalid read records, and against the
ZSTD_DCtx leak it reports the context as definitely lost, both attributed
inside zstdruby.so.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant