Skip to content

Fix call tracking for all acquisition methods#15

Merged
man8 merged 3 commits intomainfrom
codex/locate-and-fix-important-codebase-bug
Nov 7, 2025
Merged

Fix call tracking for all acquisition methods#15
man8 merged 3 commits intomainfrom
codex/locate-and-fix-important-codebase-bug

Conversation

@man8
Copy link
Owner

@man8 man8 commented Sep 23, 2025

Summary

  • ensure RateLimiter records tracked calls for sync acquire/try paths and reuse that logic in async helpers
  • remove redundant tracking from context managers now that acquisition routines account for delays themselves
  • add regression tests covering direct sync and async acquisition with tracking enabled

Testing

  • PYTHONPATH=src pytest -q

https://chatgpt.com/codex/tasks/task_e_68d220efa95c832c97c9bd9d65c443b0


Note

Ensure all acquisition paths (sync/async acquire and try) increment tracked call counts with refactored internal tracking under a single lock.

  • Fixes:
    • Consistent call tracking when track_calls=True for acquire(), try_acquire(), async_acquire(), and async_try_acquire().
    • Eliminated double-locking by making _record_call() require the caller to hold self.lock.
  • Refactor:
    • Implemented tracking at internal methods (_try_consume_one_token_sync, _try_acquire_sync); context managers now delegate to these.
  • Tests:
    • Added regression tests verifying call counting for direct sync/async acquisition and success-only increments for try methods; preserved mixed sync/async behavior.
  • Versioning & Docs:
    • Bumped __version__ to 0.3.4 and updated CHANGELOG.md.

Written by Cursor Bugbot for commit 2215b0e. This will update automatically on new commits. Configure here.

man8 added 3 commits November 7, 2025 18:51
Fixes call tracking bugs where try_acquire(), async_try_acquire(), and
acquire() did not increment call_count when track_calls=True.

Changes:
- Move call tracking to internal helper methods (_try_consume_one_token_sync,
  _try_acquire_sync) for cleaner architecture
- Remove duplicate tracking from async_acquire (was causing double-counting
  after rebase with v0.3.2)
- Context managers now delegate tracking to underlying acquisition methods
- All acquisition methods now consistently track calls

Fixes #15
The _record_call() method was acquiring self.lock even though all
callers already held the lock. Since self.lock is an RLock (reentrant),
this did not cause deadlocks but was inefficient and architecturally
unclear.

Changes:
- Remove lock acquisition from _record_call()
- Update docstring to document that caller must hold lock
- Add Args documentation for clarity

All 148 tests still pass.
@man8 man8 force-pushed the codex/locate-and-fix-important-codebase-bug branch from 461e2ca to 2215b0e Compare November 7, 2025 17:04
@man8
Copy link
Owner Author

man8 commented Nov 7, 2025

Rebase and Code Review Complete

This PR has been rebased on current main (incorporating v0.3.2 and v0.3.3 changes) and reviewed. All 148 tests pass.

Changes Made During Rebase

  1. Fixed double-counting bug - After rebasing, async_acquire() was tracking calls twice (once in the internal helper from this PR, once from v0.3.2's fix). Removed the duplicate tracking from async_acquire().

  2. Fixed double-locking issue - Code review caught that _record_call() was acquiring self.lock even though all callers already held it. Removed the redundant lock acquisition and updated docstring.

  3. Updated to v0.3.4 - Bumped version and added CHANGELOG entry.

Commits

  • 8d20b78 - Original PR: Fix call tracking for all acquisition methods
  • 8e452f4 - Fix: ensure all acquisition methods track calls consistently (version bump, rebase fixes)
  • 2215b0e - Refactor: remove inefficient double-locking from _record_call

Test Results

All 148 tests pass:

  • Original call tracking tests (sync and async)
  • New regression tests for direct acquire methods
  • All existing functionality tests

Ready to Merge

This PR is now ready to:

  • Mark as ready for review (remove DRAFT status)
  • Merge to main
  • Tag and release as v0.3.4

@man8 man8 marked this pull request as ready for review November 7, 2025 17:05
@man8 man8 self-assigned this Nov 7, 2025
@man8 man8 merged commit a71359c into main Nov 7, 2025
12 checks passed
@man8 man8 deleted the codex/locate-and-fix-important-codebase-bug branch November 7, 2025 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant