Deduplicate call-log prompt text into a hash-keyed prompts table - #22
Merged
Conversation
xagent re-routes the same prompt many times within one task (a real log had 1088 calls over only 161 distinct prompts, one prompt repeated 72x), so identical prompt text was stored once per call. - New prompts table (sha256 unique + text); calls.prompt becomes a prompt_id FK. recent() joins the text back, so the history API shape is unchanged. - record() upserts the prompt by content hash (savepoint-protected against concurrent inserts of the same hash). - delete() garbage-collects the prompt row once no call references it, so user prompt text never outlives its last call. - Migration 0004 backfills existing databases (dedup + column swap) and supports downgrade; legacy pre-Alembic databases stamp through the existing checkpoint mechanism.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new prompts table to deduplicate prompt texts from the calls table, including an Alembic migration to backfill existing data, updated SQLAlchemy models, and garbage collection of orphaned prompts upon call deletion. The review feedback highlights critical concurrency and database compatibility issues: a potential migration failure on case-insensitive databases due to subquery cardinality, and race conditions between concurrent prompt deletions and insertions. To address these, the reviewer suggests using MIN(p.id) in the migration subquery and implementing with_for_update locks during prompt retrieval and garbage collection.
qinxuye
commented
Jul 8, 2026
An unspaced 60-char token (e.g. a URL prompt) has no break opportunity, so it overflowed the 260px prompt cell. Apply word-break: break-all to the short view, matching the expanded .full-prompt detail view.
…enforcement - delete() locks the prompt row (FOR UPDATE) and uses a locking existence read before GC, so two transactions deleting the last two calls of one prompt cannot both skip GC and leak the text, and a concurrent record() cannot lose its prompt row mid-transaction. - _get_or_create_prompt() locks the looked-up row for the same reason and so the post-conflict re-read sees the winner's row under REPEATABLE READ. - On SQLite FOR UPDATE is a no-op: enable PRAGMA foreign_keys so a dangling prompt_id raises instead of being accepted, and retry record() once when a concurrent GC invalidates the prompt between lookup and insert. - Migration backfill now dedupes by exact content hash in Python instead of SQL DISTINCT / text-equality joins, which under case-insensitive collations (MySQL default) would merge prompts differing only in casing and rewrite the stored text of some calls. - Regression tests: concurrent-delete GC (row-locking backends) and case-variant backfill.
Collaborator
Author
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
qinxuye
commented
Jul 9, 2026
delete() now takes the prompt row lock BEFORE deleting the call row, so every delete()/record() for one prompt serializes on a single lock before acquiring per-call row locks. With the previous order (call row first, prompt lock second), two transactions deleting the last two calls of one prompt deadlocked: T1 held the prompt lock while its locking reference check waited on T2's deleted-but-uncommitted call row, and T2 waited on the prompt lock. The regression test now runs the real reference check concurrently with the queued delete — verified against Postgres 16: the old order fails with a detected deadlock, the new order passes.
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
1 participant
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.
Why
xagent re-requests routing for the same prompt many times within one task — a real deployment log had 1088 calls over only 161 distinct prompts (one prompt repeated 72×, at 4–10s intervals). Each call stored the full prompt text again.
What
promptstable:sha256(unique, indexed) +text.calls.promptis replaced by aprompt_idFK (indexed).record()looks the prompt up by content hash and inserts only if new; the insert is savepoint-protected so a concurrent writer inserting the same hash cannot fail the transaction.recent()joins the text back (lazy="joined"), so the history API/UI response shape is unchanged.delete()garbage-collects the prompt row once no call references it — the log holds user prompts, so orphaned text must not outlive its last call.0004backfills existing DBs (dedup existing text, swap the column, batch mode for SQLite) with a full downgrade path. Legacy pre-Alembic DBs stamp correctly via the existing_SCHEMA_CHECKPOINTSmechanism (prompt_id→ 0004).Verified
integrity_checkok; then applied for real via server restart (auto-migrate) and the live/api/historyreturns full text.WHERE sha256=?confirmed to use the unique index (covering-index search).CallStore); its 23 tests pass against this change.Note for operators
Old code cannot read a migrated DB (the
promptcolumn is gone) — rolling back the package requiresalembic downgradeor a backup. Multi-replica Postgres/MySQL deployments should migrate on a single instance first (run_migrationstakes no cross-process lock).