Add test for ARAL race condition and fix - #22212
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
sequenceDiagram
participant T as Allocator Thread
participant AL as ARAL Logic
participant H as Test Hook (Internal Checks)
participant P as ARAL Page
participant O as Racing Thread
T->>AL: aral_get_first_page_with_a_free_slot()
rect rgb(240,240,240)
Note over AL: NEW: retry_acquisition loop
AL->>AL: aral_acquire_first_page()
AL-->>AL: Target Page Found (Refcount++)
opt NETDATA_INTERNAL_CHECKS enabled
AL->>H: NEW: aral_unittest_wait_for_race_window()
Note over H,O: Allocator thread pauses here
O->>P: Racing thread fills last slot
O->>P: Page moved to 'Full' list
H-->>AL: Release paused allocator
end
AL->>P: aral_page_lock(page)
alt Page has free slots (Happy Path)
AL-->>T: Return Page for allocation
else Page is full (Race Condition detected)
Note right of AL: Detected via !page_lock.free_elements
AL->>P: aral_page_unlock(page)
AL->>P: aral_page_release(page) (Refcount--)
AL->>AL: CHANGED: goto retry_acquisition
end
end
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds a deterministic internal-checks unit test intended to reproduce an ARAL allocator race, and updates ARAL page acquisition logic to safely retry when a page becomes full between “acquire” and “page lock”.
Changes:
- Add NETDATA_INTERNAL_CHECKS-gated race-test hook + unit test (aral_detect_acquire_to_page_lock_race()).
- Fix allocator race by retrying if free_elements == 0 after locking the selected page.
- Tighten aral_page_acquire() by rolling back the refcount increment when acquisition is invalid (rf <= 0).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
…improve readability
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
…and ensure consistent page state handling (unittest)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
…ions and improve page state validation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
…eardown - Add error handling for ARAL allocator thread creation. - Prevent null thread dereference and streamline cleanup with conditional `nd_thread_join`.
thiagoftsm
left a comment
There was a problem hiding this comment.
PR is working as expected locally. LGTM!
Sorry, something went wrong.
* Add stress test and unit test for ARAL allocator race condition fixes * Fix race condition and cleanup tests * Reorder `#ifdef NETDATA_INTERNAL_CHECKS` to fix struct placement and improve readability * Refactor `aral_race_unittest_force_page_full` to return forced entry and ensure consistent page state handling (unittest) * Fix `aral_detect_acquire_to_page_lock_race` to handle retried allocations and improve page state validation * Improve ARAL race unittest to handle thread errors and ensure clean teardown - Add error handling for ARAL allocator thread creation. - Prevent null thread dereference and streamline cleanup with conditional `nd_thread_join`.
* Add stress test and unit test for ARAL allocator race condition fixes * Fix race condition and cleanup tests * Reorder `#ifdef NETDATA_INTERNAL_CHECKS` to fix struct placement and improve readability * Refactor `aral_race_unittest_force_page_full` to return forced entry and ensure consistent page state handling (unittest) * Fix `aral_detect_acquire_to_page_lock_race` to handle retried allocations and improve page state validation * Improve ARAL race unittest to handle thread errors and ensure clean teardown - Add error handling for ARAL allocator thread creation. - Prevent null thread dereference and streamline cleanup with conditional `nd_thread_join`. (cherry picked from commit 9f7887e)
Summary
Summary by cubic
Fixes an ARAL allocator race where a page chosen with a free slot becomes full before it’s locked by adding a lock-time retry and a refcount guard. Adds targeted unit and stress tests to reproduce the race and validate retried allocations and page list transitions.
Bug Fixes
New Features
Written for commit 4aa380e. Summary will update on new commits.