Add comprehensive memory leak testing#19
Open
devin-ai-integration[bot] wants to merge 3 commits intomasterfrom
Open
Add comprehensive memory leak testing#19devin-ai-integration[bot] wants to merge 3 commits intomasterfrom
devin-ai-integration[bot] wants to merge 3 commits intomasterfrom
Conversation
- Add test-memory-leaks.cpp with dedicated leak regression tests - Test model/context/sampler lifecycle patterns - Test error condition cleanup - Test concurrent usage with multiple threads - Test batch operations and KV cache clearing - Add optional Valgrind integration via CMake target Tests cover: - Repeated model load/free cycles (10 iterations) - Context creation/destruction patterns (10 iterations) - Multiple contexts sharing same model (5 contexts) - Sampler lifecycle with chain operations - Backend initialization cleanup - Error path cleanup validation (failed model load) - Concurrent model usage (3 threads) - Batch allocation/deallocation - KV cache memory clearing operations - Model load cancellation (via progress callback) The test follows existing patterns from test-autorelease.cpp and test-thread-safety.cpp, using get-model.cpp helper for model paths and proper cleanup order (sampler → context → model → backend). Run with: cmake -DLLAMA_SANITIZE_ADDRESS=ON && ctest -R test-memory-leaks Optional: make test-valgrind (requires Valgrind installed) Related to disabled test-opt.cpp which has known memory leak at line 300 (ggml_opt_alloc called in loop without cleanup between iterations). Co-Authored-By: Stephen Cornwell <stephen@cognition.ai>
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Update the readme |
- Add comprehensive Testing section with Memory Leak Testing subsection - Document how to run tests with AddressSanitizer - Document Valgrind integration (make test-valgrind) - List all 10 test scenarios covered by test-memory-leaks.cpp - Include build commands, test execution instructions - Document environment variables and CI integration - Reference known issues (test-opt.cpp leak) Addresses GitHub comment from @stephencornwell requesting README update. Co-Authored-By: Stephen Cornwell <stephen@cognition.ai>
Fixes editorconfig check failures: - Remove 67 trailing whitespace errors from test-memory-leaks.cpp - Remove 4 trailing whitespace errors from CMakeLists.txt (lines 228-231) This resolves the editorconfig CI check failure. Co-Authored-By: Stephen Cornwell <stephen@cognition.ai>
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.
Make sure to read the contributing guidelines before submitting a PR
Summary
Adds comprehensive memory leak regression testing to expand beyond the current AddressSanitizer-only approach. This PR introduces a dedicated test suite (
test-memory-leaks.cpp) with 10 focused tests covering various memory lifecycle scenarios, plus optional Valgrind integration.Link to Devin run: https://app.devin.ai/sessions/9c9b287f31684d78a93467d4f27da6fc
Requested by: @stephencornwell
Changes
New Test File:
tests/test-memory-leaks.cppA dedicated memory leak regression test with 10 test functions:
llama_get_memory()+llama_memory_clear()Build System Changes:
tests/CMakeLists.txtllama_build_and_test()macro (LABEL "model")make test-valgrindtarget when Valgrind is installedTesting Approach
The test follows existing patterns from
test-autorelease.cppandtest-thread-safety.cpp:get-model.cpphelper for model paths (skips gracefully if no model provided)LLAMA_SANITIZE_ADDRESS=ONRun with:
Review Checklist
Memory API correctness: Verify
llama_get_memory(ctx)→llama_memory_clear(mem, false)is the correct pattern for KV cache clearing (test_kv_cache_clear_operations)Threading safety: Review
test_threaded_contexts- creates 3 threads sharing one model with separate contexts. Ensure this follows thread-safety best practices for the library.Cleanup order: Confirm sampler → context → model → backend order is correct in all scenarios, especially error paths.
Valgrind CMake syntax: Verify the custom target configuration is correct (untested locally as Valgrind wasn't installed).
CI validation: This will be the first run with actual models and sanitizers - local testing was limited by environment issues and no model file.
Notes
test-opt.cpp remains disabled: While investigating, identified the root cause of its memory leak (line 300:
ggml_opt_alloc()called in loop withoutggml_opt_reset()between iterations), but didn't fix it per task scope. This could be addressed in a follow-up if desired.Local testing limitations: Successfully compiled with AddressSanitizer and test ran via CTest (skipped due to no model), but couldn't validate full test suite locally due to ASan runtime environment issues. CI will provide the first comprehensive validation.
Test philosophy: These tests are designed as regression tests - they pass if no leaks are detected by sanitizers/Valgrind, rather than asserting on memory usage numbers directly.