Fix cache poisoning when a singleton write is rolled back - #158
Open
alessio-b2c2 wants to merge 1 commit into
Open
Fix cache poisoning when a singleton write is rolled back#158alessio-b2c2 wants to merge 1 commit into
alessio-b2c2 wants to merge 1 commit into
Conversation
SingletonModel updated the cache synchronously around DB operations. When one ran inside an outer transaction.atomic() that later rolled back, the DB reverted but the cache kept the stale value, disagreeing with the DB until SOLO_CACHE_TIMEOUT or the next write. Three paths were affected: * save() called set_to_cache() right after super().save(). * delete() called clear_cache() before super().delete(). * get_solo() on a cache miss cached the row returned by get_or_create() -- poisoning the cache whether the row was created in the transaction (and then rolled away) or was an uncommitted modification read back from the DB. Defer all three cache updates to transaction.on_commit(), which fires immediately in autocommit mode and is discarded on rollback. This keeps the cache in sync with what the DB actually commits, and is simpler than tracking get_or_create()'s created flag while also closing the read-of-uncommitted-row window that flag-based approach would leave open. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alessio-b2c2
force-pushed
the
fix-cache-poisoning-on-transaction-rollback
branch
from
May 27, 2026 15:01
9dbe975 to
20f7711
Compare
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.
The bug
SingletonModelkeeps the configured cache (SOLO_CACHE) in sync with the DB by updating it synchronously around every DB operation. If an operation runs inside an outertransaction.atomic()that later rolls back, the database reverts but the cache keeps the value it was given. The cache then disagrees with the DB untilSOLO_CACHE_TIMEOUTexpires (5 minutes by default) or the next successful write — soget_solo()serves stale (or non-existent) data in the meantime.There are three poisoning paths in
solo/models.py:save()calledself.set_to_cache()immediately aftersuper().save(). A rollback reverts the row but the cache keeps the rolled-back value. (Model.objects.create()routes throughsave(), so it's covered too.)delete()calledself.clear_cache()beforesuper().delete(). A rollback keeps the row, but the cache was already emptied — so the cache is stale/empty for a row that still exists.get_solo()on a cache miss didget_or_create(pk=...)thenobj.set_to_cache(). This poisons the cache in two sub-cases: the row was created inside the transaction (and then rolled away), or the row existed but had an uncommitted modification earlier in the same transaction thatget_soloread back and cached before the rollback.The fix
Defer every cache update to
transaction.on_commit(...):save()→transaction.on_commit(self.set_to_cache)delete()→transaction.on_commit(self.clear_cache)get_solo()(cache miss) →transaction.on_commit(obj.set_to_cache)on_commitcallbacks fire when the surrounding transaction commits and are discarded on rollback, so the cache only ever reflects what the DB actually committed. In autocommit mode (no surrounding transaction) the callback fires immediately, so non-transactional behaviour is unchanged.For
get_solo()this is both simpler and more correct than capturingget_or_create'screatedflag and skipping the cache for new rows: deferring to commit closes the create-on-miss rollback case and the read-of-uncommitted-modification case, with no flag to track. The only behavioural change is that within a still-open transaction the cache isn't populated until commit (repeated reads hit the DB until then); the common autocommit path is unaffected.Tests
Added a
TransactionRollbackCacheTest(solo/tests/tests.py) with a regression test per path:save()inside anatomic()that rolls back → cache must not hold the rolled-back value.delete()inside anatomic()that rolls back → cache must still hold the (not-actually-deleted) value.get_solo()create-on-miss inside anatomic()that rolls back → cache must not hold an object for the row that no longer exists.get_solo()re-caching an uncommitted modification inside anatomic()that rolls back → cache must not hold the rolled-back value.The tests:
TransactionTestCase(notTestCase) —on_commitcallbacks don't fire inside the wrapping transaction a plainTestCaseuses, which would make the fixes look broken;SOLO_CACHEenabled — the fixes are no-ops when caching is off, sinceset_to_cache/clear_cacheearly-return.I verified each new test fails against the unfixed
models.pyand passes with the fix. The existingtest_delete_if_cache_enabledwas updated to wrapsave()/delete()incaptureOnCommitCallbacks(execute=True), since its cache assertions now depend on the deferredon_commitcallbacks firing.Full suite (
manage.py test solo),ruff format/ruff check, andmypyall pass.🤖 Generated with Claude Code