Skip to content

Conversation

pablogsal
Copy link
Member

The ObjectTrackingEvent enum values were colliding with AllocatorType
values. This caused a bug where OBJECT_DESTROYED (value 2) was
incorrectly matching PYOBJECT_MALLOC in the deallocation check.

This would incorrectly treat pymalloc allocations as deallocations when
checking stack traces. The bug went undetected because we lacked tests
for Python allocations with trace_python_allocators enabled.

Fixed by changing ObjectTrackingEvent values to 10 and 20 to avoid
collision. Added test coverage for pymalloc allocations with Python
stack traces to prevent regressions.

Issue number of the reported bug or feature request: #

Describe your changes
A clear and concise description of the changes you have made.

Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.

Additional context
Add any other context about your contribution here.

The ObjectTrackingEvent enum values were colliding with AllocatorType
values. This caused a bug where OBJECT_DESTROYED (value 2) was
incorrectly matching PYOBJECT_MALLOC in the deallocation check.

This would incorrectly treat pymalloc allocations as deallocations when
checking stack traces. The bug went undetected because we lacked tests
for Python allocations with trace_python_allocators enabled.

Fixed by changing ObjectTrackingEvent values to 10 and 20 to avoid
collision. Added test coverage for pymalloc allocations with Python
stack traces to prevent regressions.

Signed-off-by: Pablo Galindo Salgado <[email protected]>
See changelog for more details.

Signed-off-by: Pablo Galindo Salgado <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2025

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.35%. Comparing base (6cfb8a4) to head (dc85b8c).

Files with missing lines Patch % Lines
tests/integration/test_tracking.py 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #832      +/-   ##
==========================================
- Coverage   92.43%   92.35%   -0.08%     
==========================================
  Files          99       99              
  Lines       11814    11840      +26     
  Branches      423      425       +2     
==========================================
+ Hits        10920    10935      +15     
- Misses        894      905      +11     
Flag Coverage Δ
cpp 92.35% <96.29%> (-0.08%) ⬇️
python_and_cython 92.35% <96.29%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

I pushed three tiny changes:

  • a fix to the grammar in the news entry
  • a change to the enumerators' values to ensure they don't conflict (the 10 you chose still conflicted; ALIGNED_ALLOC == 10 == OBJECT_CREATED!) - I just picked a different convention; one enum's enumerators count up from 1 and the other's count down from negative 1, and we'll never get conflicts.
  • I added a comment explaining that they need to not conflict.

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.

3 participants