fix: use single connection for upsert_by_logical_key#135
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Refactor upsert_by_logical_key to perform the INSERT attempt, existing-row fetch, and conditional UPDATE within a single pooled connection and transaction. Previously acquired up to 3 separate connections (_query_entries + update_entry each took their own), causing unnecessary pool contention under concurrency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
206dd80 to
99e754b
Compare
cmeans
reviewed
Mar 31, 2026
Owner
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #135: Single-connection upsert_by_logical_key (MEDIUM #2)
Reviewer: QA Agent | Date: 2026-03-31
Code Review: PASS
The entire upsert flow (INSERT attempt, existing-row fetch, diff computation, conditional UPDATE) now runs within one with self._pool.connection() as conn, conn.transaction(), conn.cursor() as cur: block. Previously acquired up to 3 connections via _query_entries and update_entry.
Key implementation details:
- Uses
_load_sql("query_entries").format(...)directly instead of calling_query_entries— avoids the separate connection that method acquires. - Update logic inlined (changelog computation,
update_entrySQL) — mirrorsupdate_entrybehavior but within the existing cursor. - Early return for no-diff case (
if not updates: return (old, False)) — skips unnecessary UPDATE. _cleanup_expired()only called on insert path (correct — updates don't create new entries).
Tests: 554/554 PASS
New test_upsert_by_logical_key_creates_then_updates covers create→update→changelog→single-entry verification. Existing concurrent upsert test still passes.
CI: All green
Findings: Zero
Owner
QA Audit — PR #135CI: All green | Tests: 554/554 pass
|
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.
Summary
upsert_by_logical_keyinpostgres_store.pyto perform the INSERT attempt, existing-row fetch, and conditional UPDATE within a single pooled connection and transaction_query_entriesandupdate_entryeach took their own), causing unnecessary pool contention under concurrencyQA
Prerequisites
pip install -e ".[dev]"AWARENESS_PORT=8421)Manual tests (via MCP tools)
Expected: entry created successfully, returns the new entry
Expected: existing entry updated (not duplicated), description and tags reflect new values, changelog records the change
Expected: exactly one entry with logical_key="upsert-qa-key", showing updated description and tags
🤖 Generated with Claude Code