Skip to content

fix(cache): reuse refreshed database tool entries - #291

Open
Linxiushen wants to merge 1 commit into
TencentCloudADP:mainfrom
Linxiushen:fix/refresh-db-tool-cache
Open

Linxiushen wants to merge 1 commit into
TencentCloudADP:mainfrom
Linxiushen:fix/refresh-db-tool-cache

Conversation

@Linxiushen

Copy link
Copy Markdown

Fixes #290.

After a database tool-cache entry expired, refreshing it inserted another row while the next lookup continued returning the old expired row. This caused every subsequent call to execute the tool again. Refresh the existing entry instead, and prefer the newest matching row so databases with duplicates from earlier runs can reuse their fresh results. Cache bypass still executes the tool and updates the stored result.

Validation:

  • Three new regression cases fail on the base revision and pass with the fix, using a real temporary SQLite database and a controlled clock. They cover expiration and reuse, existing duplicate entries, and bypass followed by reuse; persistence is checked through new database sessions.
  • pytest tests/utils/test_tool_cache.py tests/db/test_db_service.py: 5 passed.
  • Ruff 0.12.11 lint and format checks pass for both changed files. Full-repository checks report the same 3 lint findings and 9 files needing formatting on base c2caa539f4c95ae1c39ed24dc8a99cb3651e1d5d and this branch.

Tested with the locked SQLModel 0.0.24 / SQLAlchemy 2.0.43 dependencies on Python 3.12.14. SQLModel's update helper emits a Pydantic deprecation warning with the locked Pydantic 2.12.4 version. PostgreSQL, live provider calls, and the API-dependent full test suite were not run. This does not add synchronization for simultaneous cache misses or remove historical duplicate rows.

AI assistance was used for implementation and regression tests, followed by an independent agent review of the diff and validation.

@kittimzhe kittimzhe 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.

Verified locally — this is the right fix for the reported behavior, and the tests genuinely pin it.

Local verification (Python 3.12, minimal deps from pyproject, UTU_LLM_TYPE/UTU_LLM_MODEL set since utu/__init__.py asserts them at import)

  • PR head: 3/3 passed
  • Same tests against main's tool_cache.py: 3/3 failed — test_db_cache_reuses_refreshed_result_after_expiration reproduces the exact reported symptom (expired row keeps winning after refresh), and the other two fail on the duplicate/no-update paths. Good red/green separation.

On the fix itself

  • order_by(timestamp.desc(), id.desc()).limit(1) is the deterministic newest-row read the original .first() lacked — the id tiebreaker makes it stable even for equal timestamps.
  • Updating the existing row via sqlmodel_update instead of always inserting both fixes the growth and keeps one row per (function, cache_key).
  • The three tests cover the right matrix: expire→refresh→reuse, newest-duplicate preference, and disabled-cache single-row refresh.

Non-blocking observations

  1. Legacy duplicates are never cleaned. Deployed databases already carry duplicate piles from the old insert-always path. After this fix the orphans are never selected (newest wins) but also never removed. If you want the tables to self-heal, deleting older duplicates for the same (function, cache_key) in the same transaction as the update is a one-statement addition.
  2. USE_CACHE=False now pays the SELECT on every call (previously the read was skipped entirely when disabled). Justified — the update path needs the row reference — and negligible for the cache sizes involved, but it is a small behavior change worth having in the PR description.
  3. First-call race remains (pre-existing): two concurrent calls for a fresh key can both miss cached and both insert. Self-limiting afterwards — the next write updates the newest row — and the dedup-on-update from observation 1 would absorb the loser rows too.

LGTM from my side with those as optional follow-ups.

@Linxiushen

Copy link
Copy Markdown
Author

With USE_CACHE=False, the tool still runs on every call. The SELECT is intentional: it finds the existing row so the write refreshes that row rather than inserting another one.

Legacy-duplicate cleanup and concurrent first-miss handling are outside this fix for #290. The newest-row ordering lets databases with existing duplicates reuse the refreshed result, but it does not remove those duplicates or make concurrent inserts atomic.

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Database tool cache keeps reading the expired row after refresh

2 participants