fix: list each app once in SQL persister list_app_ids, newest save first - #942
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 Opus 5.5 <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.
list_app_idson 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. Savinga-appseq 0,z-appseq 0,a-appseq 1,a-appseq 2 in separate transactions returns['a-app', 'a-app', 'z-app', 'a-app']on main, for both psycopg2 and asyncpg. The SQLite query isSELECT 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 withpersister.list_app_ids("")[0], so on SQLite that can resume the wrong app. This came up while reviewing #920.Changes
GROUP BY app_id ORDER BY MAX(created_at) DESC.How I tested this
created_atvalues 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.postgres:15container withBURR_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 runon the changed files passes.Notes
sequence_idrather than save time, so ordering still differs across persisters.created_athas 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 existingcreated_atindex. 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 (19 ms afterANALYZE). A(partition_key, app_id, created_at)index could help large tables, but I haven't measured it and left the schema alone.Checklist