fix: propagate a raising key __eq__ instead of masking it as SystemError (#36)#79
Merged
Conversation
CacheKey::eq and BorrowedArgs::equivalent compared `PyObject_RichCompareBool(...) == 1`. RichCompareBool returns -1 with a Python exception set when a key's __eq__ raises; `== 1` maps that to "not equal" and the exception is left pending on the thread. The lookup reports a (spurious) miss, the call path recomputes / returns Ok, and PyO3 then surfaces a confusing `SystemError: ... returned a result with an exception set`, masking the user's real error (e.g. RuntimeError). Route both comparisons through a shared `rich_compare_eq` helper that: - returns false (not equal) for both 0 and -1, but on -1 leaves the exception set for the caller to fetch; and - bails out early (returns false) if an exception is already pending, so a multi-key probe never re-enters Python with a live exception and clobbers the original error. Memory-backend lookup sites then call `PyErr::take(py)` after the lookup and return the error: `__call__` (read path, before recomputing; and the write-path double-check, before inserting), `get`, and `_probe`. evict_one and the write-path remove/insert compare a key against itself (identity), so RichCompareBool short-circuits to 1 and never runs __eq__ — no check needed. Add tests (tests/test_raising_eq.py): a key whose __eq__ raises now propagates the original exception through __call__, get, and _probe; the cache stays usable afterward; and hash-colliding keys that compare cleanly still cache independently. Verified fail-before (SystemError) -> pass-after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Closes #36
What & why
CacheKey::eqandBorrowedArgs::equivalentcomparedPyObject_RichCompareBool(...) == 1.RichCompareBoolreturns -1 with a Python exception set when a key's__eq__raises — and== 1maps that to "not equal", leaving the exception pending on the thread. The lookup reports a spurious miss; the call path recomputes / returnsOkwhile an exception is set; PyO3 then raises a confusingSystemError: ... returned a result with an exception set, masking the user's real error (e.g.RuntimeError).functools.lru_cachechecks the -1 sentinel; we didn't.Reachable purely from safe Python: cache a key whose
__hash__collides with a stored key and whose__eq__raises (e.g.f(Bad(42)); f(Bad(42))).The fix
Both comparisons now go through a shared
key.rs::rich_compare_eqhelper:false("not equal") for both0and-1, but on-1leaves the exception set for the caller to fetch;false) if an exception is already pending, so a multi-key probe never re-enters the interpreter with a live exception and clobbers the original error.Each memory-backend lookup site then calls
PyErr::take(py)after the lookup and returns the error instead of proceeding:__call__— read path (before recomputing) and the write-path double-check (before inserting),get,_probe.evict_oneand the write-pathremove/insertcompare a key against itself (samePyObject*), soRichCompareBoolshort-circuits to1via the identity fast path and never runs__eq__— no check needed there.Test (
tests/test_raising_eq.py)A key whose
__eq__raises now propagates the original exception through__call__,get, and_probe; the cache stays usable afterward; and hash-colliding keys that compare cleanly still cache independently (guards against over-eager error detection).Verified fail-before → pass-after by stashing just the fix: the three read-path tests + the resilience test raised
SystemError: ... returned a result with an exception set; with the fix all five pass.Gates run (risky change —
src/key.rskey equality / FFI boundary)make fmt/make lint(ruff, ty, clippy-D warnings) ✓make test—cargo test(11) + pytest (98, +5 new) ✓make test-matrix— Python 3.10–3.13 ✓, new tests pass on each (3.14 skipped locally via the documenteduv-resolves-stale-alpha guard; CI covers 3.14 final)make bench— no regression: the added per-comparisonPyErr_Occurred()is a TLS read; memory backend ~18.7M ops/s, single-thread 13.6–22.2M, within run-to-run variance🤖 Generated with Claude Code