Skip to content

Reject negative cache item sizes#401

Merged
tkem merged 1 commit into
tkem:developfrom
CodingFeng101:codex/reject-negative-size
Jul 2, 2026
Merged

Reject negative cache item sizes#401
tkem merged 1 commit into
tkem:developfrom
CodingFeng101:codex/reject-negative-size

Conversation

@CodingFeng101

Copy link
Copy Markdown

What changed

Cache.__setitem__() now rejects negative values returned by getsizeof() before mutating cache state.

Why

A custom getsizeof returning a negative number currently lets currsize become negative. That breaks the cache size invariant and can make later capacity checks behave incorrectly.

Validation

  • PYTHONPATH=src python -m unittest tests.test_cache.CacheTest.test_getsizeof_negative
  • PYTHONPATH=src python -m pytest tests -q
  • python -m ruff check src\cachetools\__init__.py tests\__init__.py
  • git diff --check

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (48284d7) to head (fac1154).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #401   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1084      1086    +2     
=========================================
+ Hits          1084      1086    +2     

☔ View full report in Codecov by Harness.
📢 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.

@tkem

tkem commented Jun 30, 2026

Copy link
Copy Markdown
Owner

@CodingFeng101: Thanks for contributing! I never thought that check was actually worth it, because "who would do such a thing?", but you're probably right. If __setitem__ checks against maxsize, it should probably check against zero, too, especially now that due to type hints getsizeof() officially returns a float.
I only wonder if it should actually be size < 0 or maybe size <= 0, since size == 0 would let a cache grow without bounds? I need to think about this, your comments are welcome, of course.
And extra thanks for providing a test for this!

@CodingFeng101

Copy link
Copy Markdown
Author

Thanks for taking a look.

My preference is to keep this PR at size < 0 rather than size <= 0.

The negative case directly breaks the cache accounting invariant because currsize can move below zero after a successful insertion. That makes later capacity checks reason from an impossible state.

size == 0 is different: it can allow unbounded item count if a user chooses that getsizeof, but it does not make currsize internally inconsistent. It may also be intentional for values that should not count against byte/weight capacity. Rejecting zero would therefore be a behavior change for existing custom getsizeof users, while rejecting negatives only rules out values that cannot represent a meaningful size.

So I think < 0 is the narrower compatibility-preserving guard. If you decide the project should also disallow zero sizes, I can adjust the PR, but I would treat that as a separate policy decision rather than the minimal invariant fix.

@tkem tkem changed the base branch from master to develop July 2, 2026 11:14
@tkem tkem merged commit b510c82 into tkem:develop Jul 2, 2026
11 checks passed
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.

2 participants