Skip to content

fix: save Redis state for apps without a partition key - #952

Open
sangkyoonnam wants to merge 1 commit into
apache:mainfrom
sangkyoonnam:fix/redis-default-partition-key
Open

sangkyoonnam wants to merge 1 commit into
apache:mainfrom
sangkyoonnam:fix/redis-default-partition-key

Conversation

@sangkyoonnam

Copy link
Copy Markdown

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.

ApplicationBuilder leaves partition_key as None, and RedisBasePersister.save and AsyncRedisBasePersister.save put it inside the hset mapping, which redis-py rejects. Without a namespace, redis-py also rejects None as the sorted-set key the reads use. On main, an app with with_state_persister(persister) and no partition key raises redis.exceptions.DataError: Invalid input of type: 'NoneType' on step(), and the same on astep(). Without a namespace, load(None, app_id) and list_app_ids(None) raise the same error; with a namespace they look up <namespace>:None and find nothing. Either way initialize_from can't resume such an app. The SQLite persisters map None to PARTITION_KEY_DEFAULT ("") in save, load and list_app_ids, the Postgres load does the same, and #949 (open) adds it to the Postgres save, so this aligns Redis with them.

Changes

  • RedisBasePersister and AsyncRedisBasePersister get PARTITION_KEY_DEFAULT = "", matching the SQLite and Postgres persisters.
  • save, load and list_app_ids on both classes map a None partition key to PARTITION_KEY_DEFAULT and annotate it as Optional[str]. With no namespace the partition index becomes the empty-string key and the state hashes :<app_id>:<sequence_id>; with a namespace they become ns: and ns::<app_id>:<sequence_id>. Redis accepts both, and load returns "" as the partition key, the same as SQLite.

How I tested this

  • Three new tests save with partition_key=None, load it back with None, and check that list_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 with partition_key == "" and through load("", app_id). All three fail on main with the DataError and pass here.
  • The Redis tests ran against a local redis:7 container with BURR_CI_INTEGRATION_TESTS=true: 17 passed (the 14 on main plus the three new ones).
  • An app with 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 run on the changed files passes.

Notes

  • load(partition_key, app_id=None) still raises the same DataError. The per-partition sorted set is scored by sequence id, so "latest across app ids" would need a choice the SQL persisters make by created_at, and ApplicationBuilder always sets an app_id, so no app path reaches it. I left it alone.
  • Existing data isn't affected. A None partition key never got written before. Apps that passed partition_key="" explicitly now share that key space with apps that passed nothing, which is how the SQLite persisters already behave.
  • The key layout joins partition_key, app_id and sequence_id with : without escaping, so an app_id containing : can collide across partitions. That predates this change and needs a migration story, so it stays out of scope.
  • feat: durable execution (suspend/resume + ctx.durable journal) #786 adds _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 keep PARTITION_KEY_DEFAULT, since SQLite and Postgres already use it.
  • The existing Redis tests always pass a partition key, which is why this didn't show up in CI.
  • I wrote this with Claude Code and reviewed and ran it myself.

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added area/storage Persisters, state storage area/integrations External integrations (LLMs, frameworks) labels Oct 6, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/integrations External integrations (LLMs, frameworks) area/storage Persisters, state storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant