Skip to content

Guard sentinel/cluster err.txt valgrind scan with $::valgrind#3982

Merged
enjoy-binbin merged 1 commit into
valkey-io:unstablefrom
skourta:fix-sentinel-cluster-valgrind-scan-guard
Jun 15, 2026
Merged

Guard sentinel/cluster err.txt valgrind scan with $::valgrind#3982
enjoy-binbin merged 1 commit into
valkey-io:unstablefrom
skourta:fix-sentinel-cluster-valgrind-scan-guard

Conversation

@skourta

@skourta skourta commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Issue Description

log_crashes in tests/instances.tcl — the multi-instance harness used by both make test-sentinel and make test-cluster — scans every instance's err.txt with find_valgrind_errors unconditionally:

set logs [glob */err.txt]
foreach log $logs {
    set res [find_valgrind_errors $log true]
    if {$res != ""} {
        puts $res
        incr ::failed
    }
}

The main test framework runs the identical scan only behind a valgrind guard — in tests/support/server.tcl, check_valgrind_errors is called from kill_server inside if {$::valgrind}.

find_valgrind_errors' fallback (in tests/support/util.tcl) treats any stderr that lacks a valgrind leak-free summary as an error:

# Look for the absence of a leak free summary
# (happens when the server isn't terminated properly).
if {(![regexp -- {definitely lost: 0 bytes} $buf] &&
     ![regexp -- {no leaks are possible} $buf])} {
    return $buf
}

When the suite is not running under valgrind, that summary is never present, so any non-empty err.txt is flagged as a failure.

Concrete trigger

On hosts whose CPU does not expose the constant_tsc flag (common inside VMs / LXD), every valkey-server and valkey-sentinel prints to stderr at startup (src/monotonic.c):

monotonic: x86 linux, 'constant_tsc' flag not present

The sentinel suite then reports exactly 10 failures (5 sentinel + 5 valkey instances) on every run — even though every individual test passes and no assertion fails. The cluster suite shares this harness and is affected identically. On hosts that expose constant_tsc the message is never printed, so the bug is invisible — which is why it only surfaces in certain CI/VM environments.

Fix

Wrap the valgrind err.txt scan in if {$::valgrind}, mirroring server.tcl. The sanitizer scan immediately below is left unguarded, matching check_sanitizer_errors, which always runs. No test coverage is lost.

if {$::valgrind} {
    set logs [glob */err.txt]
    foreach log $logs {
        set res [find_valgrind_errors $log true]
        if {$res != ""} {
            puts $res
            incr ::failed
        }
    }
}

Signed-off-by: Smail Kourta <smail.kourta@canonical.com>
@skourta skourta force-pushed the fix-sentinel-cluster-valgrind-scan-guard branch from 2ee614e to 0b03e0d Compare June 12, 2026 13:21
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds a conditional guard to the log_crashes test utility procedure, scoping Valgrind error file scanning to runs where Valgrind is enabled. Previously, the error log scanning logic ran unconditionally; now it executes only when ::valgrind is true.

Changes

Valgrind error scan conditional guard

Layer / File(s) Summary
Conditional Valgrind error scanning
tests/instances.tcl
The log_crashes procedure wraps Valgrind-specific error file globbing and detection in an if {$::valgrind} guard, preventing unnecessary error log processing when Valgrind is not enabled.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Suggested reviewers

  • hwware
  • lucasyonge
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: guarding the Valgrind error scan for err.txt files with the $::valgrind flag, which is the core fix in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description provides a detailed explanation of the problem, the concrete trigger, and the fix, all directly related to the changeset in tests/instances.tcl.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@enjoy-binbin enjoy-binbin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Make sense

@enjoy-binbin enjoy-binbin added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jun 13, 2026
@codecov

codecov Bot commented Jun 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.70%. Comparing base (86acf8d) to head (0b03e0d).
⚠️ Report is 3 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3982      +/-   ##
============================================
+ Coverage     76.54%   76.70%   +0.15%     
============================================
  Files           162      162              
  Lines         80775    80777       +2     
============================================
+ Hits          61833    61962     +129     
+ Misses        18942    18815     -127     

see 20 files with indirect coverage changes

🚀 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.

@enjoy-binbin enjoy-binbin merged commit fa23923 into valkey-io:unstable Jun 15, 2026
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants