Skip to content

ci: split valgrind Lua tests into blocking job#112

Merged
membphis merged 2 commits into
mainfrom
codex/110-ci-valgrind
May 31, 2026
Merged

ci: split valgrind Lua tests into blocking job#112
membphis merged 2 commits into
mainfrom
codex/110-ci-valgrind

Conversation

@membphis
Copy link
Copy Markdown
Collaborator

@membphis membphis commented May 31, 2026

Summary

  • Split upstream LuaJIT valgrind memcheck out of the lua matrix job into a dedicated lua-valgrind job.
  • Keep the normal Lua integration job topology intact while letting valgrind start independently of the Rust matrix gate.
  • Make valgrind blocking by removing the old continue-on-error path and preserving the existing full tests/lua memcheck parameters.

Test Plan

  • ruby -ryaml -e 'ci = YAML.load_file(".github/workflows/ci.yml"); jobs = ci.fetch("jobs"); vg = jobs.fetch("lua-valgrind"); raise "lua-valgrind must not need rust" if Array(vg["needs"]).include?("rust"); raise "lua-valgrind must block on failure" if vg["continue-on-error"]; text = File.read(".github/workflows/ci.yml"); raise "lua matrix still has valgrind flag" if text.include?("matrix.runtime.valgrind") || text.include?("valgrind: true") || text.include?("valgrind: false"); puts "ci valgrind topology invariant OK"'
  • go run github.com/rhysd/actionlint/cmd/actionlint@latest .github/workflows/ci.yml
  • PATH="$HOME/.luarocks/bin:$PATH" make test LUAJIT=/opt/homebrew/bin/luajit
  • cargo test --release --no-default-features
  • cargo clippy --release --all-targets -- -D warnings
  • cargo test --features test-panic --release
  • cargo test --no-default-features

Notes

Summary by CodeRabbit

  • Chores
    • Reorganized CI workflow to separate valgrind memory checks into a dedicated job for improved test clarity and maintainability.
    • Added valgrind suppressions for known false positives to reduce test output noise.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 31, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR decouples valgrind memcheck testing from the serial lua job into an independent parallel CI job. The main lua job's OpenResty LuaJIT matrix entry is simplified by removing valgrind configuration; valgrind-specific build steps and tests are removed from the lua job and consolidated into a new standalone lua-valgrind job that runs independently. CI-specific valgrind suppressions are added to handle false positives from stripped LuaJIT binaries.

Changes

Valgrind Decoupling for Parallel CI Execution

Layer / File(s) Summary
Remove valgrind from lua job matrix and steps
.github/workflows/ci.yml
Removed the explicit valgrind: false field from the OpenResty LuaJIT matrix entry; deleted the conditional valgrind installation step and the busted test execution under valgrind from the lua job.
Create standalone lua-valgrind job
.github/workflows/ci.yml
Added a new independent lua-valgrind job that performs the complete upstream LuaJIT valgrind memcheck workflow: checks out submodules, installs Rust/LuaJIT/LuaRocks/valgrind tooling, builds the Rust cdylib, installs Lua test dependencies, resolves the LuaJIT executable, and runs busted under valgrind.
Add Valgrind suppressions for CI
valgrind.supp
Added Memcheck suppression blocks targeting CI-specific Cond reports from stripped LuaJIT binaries, restricting suppressions to LuaJIT and Lua-cjson frame stacks while leaving Rust/qjson frames unsuppressed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #110 — This PR directly implements the core requirement to decouple valgrind from the serial lua job into an independent parallel job (lua-valgrind), reducing the CI critical path from ~12.5min to ~9min by moving valgrind out of the sequential chain while preserving full memcheck coverage on every PR.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'ci: split valgrind Lua tests into blocking job' accurately and concisely describes the main change: extracting valgrind memcheck from the lua matrix job into a separate blocking job.
Linked Issues check ✅ Passed The PR fulfills all key coding requirements from issue #110: valgrind extracted into independent parallel job without rust dependency, continue-on-error removed, existing test parameters preserved, and lua matrix simplified by removing valgrind flags.
Out of Scope Changes check ✅ Passed All changes directly support the objective of decoupling valgrind into a blocking job: CI workflow restructuring and valgrind suppression file additions are both scoped to this goal.
E2e Test Quality Review ✅ Passed This is CI workflow infrastructure only; the E2E Test Quality Review check is not applicable as no test code exists to review, only GitHub Actions configuration.
Security Check ✅ Passed No vulnerabilities found in security categories 1-7. PR contains only CI/CD config changes with no hardcoded secrets, unencrypted credentials, authorization bypasses, or data exposure issues.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/110-ci-valgrind

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 @.github/workflows/ci.yml:
- Around line 296-299: The CI job "lua-valgrind" currently inherits default
GITHUB_TOKEN scopes; update the workflow job definition for lua-valgrind to add
a minimal permissions block (e.g., permissions: contents: read) under the job
declaration so the job uses least-privilege access; edit the "lua-valgrind" job
in .github/workflows/ci.yml to insert the permissions mapping directly beneath
the job name/runner configuration.
🪄 Autofix (Beta)

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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bba88d35-6bdb-4724-bd46-4b7e3d2add43

📥 Commits

Reviewing files that changed from the base of the PR and between 90fd9d1 and cad1f23.

📒 Files selected for processing (2)
  • .github/workflows/ci.yml
  • valgrind.supp

Comment thread .github/workflows/ci.yml
Comment on lines +296 to +299
lua-valgrind:
name: Lua valgrind memcheck (upstream LuaJIT)
runs-on: ubuntu-latest
steps:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add least-privilege permissions to the new lua-valgrind job.

This job currently inherits default token scopes. Please set explicit minimal permissions (for this workflow, contents: read is likely sufficient), to avoid broader-than-needed GITHUB_TOKEN access.

Suggested patch
   lua-valgrind:
     name: Lua valgrind memcheck (upstream LuaJIT)
     runs-on: ubuntu-latest
+    permissions:
+      contents: read
     steps:
       - uses: actions/checkout@v4
📝 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.

Suggested change
lua-valgrind:
name: Lua valgrind memcheck (upstream LuaJIT)
runs-on: ubuntu-latest
steps:
lua-valgrind:
name: Lua valgrind memcheck (upstream LuaJIT)
runs-on: ubuntu-latest
permissions:
contents: read
steps:
🧰 Tools
🪛 zizmor (1.25.2)

[warning] 296-377: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block

(excessive-permissions)

🤖 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/ci.yml around lines 296 - 299, The CI job "lua-valgrind"
currently inherits default GITHUB_TOKEN scopes; update the workflow job
definition for lua-valgrind to add a minimal permissions block (e.g.,
permissions: contents: read) under the job declaration so the job uses
least-privilege access; edit the "lua-valgrind" job in .github/workflows/ci.yml
to insert the permissions mapping directly beneath the job name/runner
configuration.

@membphis membphis merged commit 37d60b2 into main May 31, 2026
16 checks passed
@membphis membphis deleted the codex/110-ci-valgrind branch May 31, 2026 06:46
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.

ci: 缩短 CI 关键路径执行时长(实测 12.5min → ~9min)

1 participant