fix: save Redis state for apps without a partition key - #952
Open
sangkyoonnam wants to merge 1 commit into
Open
sangkyoonnam wants to merge 1 commit into
sangkyoonnam wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This branch has not been deployed
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 Redis persisters now save and load state when the application has no partition key. Today an app built without
with_identifiers(partition_key=...)fails on its first step with either persister.ApplicationBuilderleavespartition_keyasNone, andRedisBasePersister.saveandAsyncRedisBasePersister.saveput it inside thehsetmapping, which redis-py rejects. Without a namespace, redis-py also rejectsNoneas the sorted-set key the reads use. On main, an app withwith_state_persister(persister)and no partition key raisesredis.exceptions.DataError: Invalid input of type: 'NoneType'onstep(), and the same onastep(). Without a namespace,load(None, app_id)andlist_app_ids(None)raise the same error; with a namespace they look up<namespace>:Noneand find nothing. Either wayinitialize_fromcan't resume such an app. The SQLite persisters mapNonetoPARTITION_KEY_DEFAULT("") insave,loadandlist_app_ids, the Postgresloaddoes the same, and #949 (open) adds it to the Postgressave, so this aligns Redis with them.Changes
RedisBasePersisterandAsyncRedisBasePersistergetPARTITION_KEY_DEFAULT = "", matching the SQLite and Postgres persisters.save,loadandlist_app_idson both classes map aNonepartition key toPARTITION_KEY_DEFAULTand annotate it asOptional[str]. With no namespace the partition index becomes the empty-string key and the state hashes:<app_id>:<sequence_id>; with a namespace they becomens:andns::<app_id>:<sequence_id>. Redis accepts both, andloadreturns""as the partition key, the same as SQLite.How I tested this
partition_key=None, load it back withNone, and check thatlist_app_ids(None)lists the app: the sync persister without a namespace, the sync persister with one, and the async persister. The first one also checks that the row reads back withpartition_key == ""and throughload("", app_id). All three fail on main with theDataErrorand pass here.redis:7container withBURR_CI_INTEGRATION_TESTS=true: 17 passed (the 14 on main plus the three new ones).initialize_from(persister, ...),with_state_persister(persister)and no partition key steps twice, rebuilds from the persister at sequence 1 with the saved state, and steps again, on both persisters.pytest tests --ignore=tests/integrations/persisters --ignore=tests/integrations/test_bip0042_bedrock.py: 656 passed, 2 skipped.pre-commit runon the changed files passes.Notes
load(partition_key, app_id=None)still raises the sameDataError. The per-partition sorted set is scored by sequence id, so "latest across app ids" would need a choice the SQL persisters make bycreated_at, andApplicationBuilderalways sets anapp_id, so no app path reaches it. I left it alone.Nonepartition key never got written before. Apps that passedpartition_key=""explicitly now share that key space with apps that passed nothing, which is how the SQLite persisters already behave.partition_key,app_idandsequence_idwith:without escaping, so anapp_idcontaining:can collide across partitions. That predates this change and needs a migration story, so it stays out of scope._partition_key_safe()with"__none__"for the new durable methods on the same classes. If that lands first, one of the two defaults should win. I'd keepPARTITION_KEY_DEFAULT, since SQLite and Postgres already use it.Checklist