Skip to content

fix: list each app once in SQL persister list_app_ids, newest save first - #942

Open
sangkyoonnam wants to merge 2 commits into
apache:mainfrom
sangkyoonnam:fix/list-app-ids-distinct-latest
Open

sangkyoonnam wants to merge 2 commits into
apache:mainfrom
sangkyoonnam:fix/list-app-ids-distinct-latest

Conversation

@sangkyoonnam

@sangkyoonnam sangkyoonnam commented Oct 4, 2026 •

Copy link
Copy Markdown

list_app_ids on the SQL persisters now returns each app once, ordered by that app's most recent save. Today the Postgres persisters can return the same app once per save, and the SQLite persisters can put an older app first.

The Postgres query is SELECT DISTINCT app_id, created_at ... ORDER BY created_at DESC, so it keeps one row per distinct (app_id, created_at) pair. Saving a-app seq 0, z-app seq 0, a-app seq 1, a-app seq 2 in separate transactions returns ['a-app', 'a-app', 'z-app', 'a-app'] on main, for both psycopg2 and asyncpg. The SQLite query is SELECT DISTINCT app_id ... ORDER BY created_at DESC, which sorts by whichever row SQLite keeps for each app. Saving a, z, a at distinct timestamps returns ['z-app', 'a-app']. The youtube-to-social-media-post notebook picks the app to resume with persister.list_app_ids("")[0], so on SQLite that can resume the wrong app. This came up while reviewing #920.

Changes

  • The SQLite, aiosqlite, psycopg2 and asyncpg persisters use GROUP BY app_id ORDER BY MAX(created_at) DESC.
  • Their list_app_ids docstrings now say each app is listed once, most recently saved first.

How I tested this

  • One new test per persister saves a, z, a with fixed created_at values and asserts ['a-app', 'z-app']. All four fail on main and pass here. The Postgres tests use their own partition key, so rows from the existing tests don't leak in.
  • The Postgres tests ran against a local postgres:15 container with BURR_CI_INTEGRATION_TESTS=true: 13 passed.
  • pytest tests --ignore=tests/integrations/persisters/test_postgresql.py --ignore=tests/integrations/test_bip0042_bedrock.py: 682 passed, 4 skipped.
  • pre-commit run on the changed files passes.

Notes

  • Redis and MongoDB already return unique IDs and aren't changed here. Redis orders by sequence_id rather than save time, so ordering still differs across persisters.
  • SQLite created_at has one-second resolution, so apps saved within the same second can still come back in either order.
  • MAX(created_at) per app isn't served by the existing created_at index. On an in-memory SQLite table with 100,000 saves across 100 apps, the new query takes about 21 ms against 3 ms for the old one. After ANALYZE the planner moves the old query onto the created_at index and both land at roughly 20 ms. A (partition_key, app_id, created_at) index could help large tables, but I haven't measured it and left the schema alone.
  • 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 Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added area/core Application, State, Graph, Actions area/storage Persisters, state storage area/integrations External integrations (LLMs, frameworks) labels Oct 4, 2026

@CanReader CanReader left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I pulled the branch and ran the sqlite and aiosqlite tests, the new ones fail on main and pass here. GROUP BY with MAX(created_at) is the right fix, the old DISTINCT app_id, created_at in the postgres ones was clearly returning the same app more than once.

Small things, none of them blocking:

  • Since "newest first" is now kind of a promise for the SQL persisters, maybe worth adding it to the list_app_ids docstring so people know they can rely on list_app_ids(...)[0] like the notebook does. Redis still orders differently, so it's good to be clear about which ones do this.
  • Agree with leaving the index out of this PR. If someone hits slow queries on a big table, a (partition_key, app_id, created_at) index can be its own PR.

I didn't run the postgres tests locally, only the sqlite ones.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sangkyoonnam

Copy link
Copy Markdown
Author

Thanks for checking it. I added the ordering note to the list_app_ids docstrings of the four SQL persisters in 7031e00, and agreed on leaving the index for its own PR.

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/core Application, State, Graph, Actions area/integrations External integrations (LLMs, frameworks) area/storage Persisters, state storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants