Skip to content

Conversation

@brian-hussey
Copy link
Member

… to be more rigourous.

🐛 Bug-fix PR

Before opening this PR please:

  1. make lint - passes ruff, mypy, pylint
  2. make test - all unit + integration tests green
  3. make coverage - ≥ 90 %
  4. make docker docker-run-ssl or make podman podman-run-ssl
  5. Update relevant documentation.
  6. Tested with sqlite and postgres + redis.
  7. Manual regression no longer fails. Ensure the UI and /version work correctly.

📌 Summary

This change addresses #1614. It works in the suggested change, ensures tests work as expected and make the tests more robust for the new model.
It removes the need for the "last_accessed" property in the CacheEntry class/objects.

🔁 Reproduction Steps

Link the issue and minimal steps to reproduce the bug.

🐞 Root Cause

Implementation of the cache could be improved.

💡 Fix Description

Using the suggested OrderedDict approach and testing around that.

🧪 Verification

Check Command Status
Lint suite make lint
Unit tests make test
Coverage ≥ 90 % make coverage
Manual regression no longer fails steps / screenshots

📐 MCP Compliance (if relevant)

  • Matches current MCP spec
  • No breaking change to MCP clients

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • No secrets/credentials committed

@brian-hussey brian-hussey marked this pull request as ready for review December 16, 2025 15:08
@brian-hussey brian-hussey marked this pull request as draft December 16, 2025 15:27
@brian-hussey
Copy link
Member Author

One question on this, should the expires_at be updated now that we're not using last_accessed for this or should the expiry time still be based on when it was first cached?

@kevalmahajan
Copy link
Member

kevalmahajan commented Dec 17, 2025

Hi @brian-hussey,

I think the expires_at value should remain fixed from the point of insertion, rather than being refreshed upon each access. Refreshing the TTL on every get() could result in frequently accessed keys remaining in cache indefinitely, which would most probably undermine the intended purpose of the TTL.

For example, if the TTL is set to 3600 seconds and an item is accessed every 30 minutes, it could effectively never expire, even if the data becomes stale.

So, if we only update LRU position and keep TTL fixed, it will ensure hot items stay in cache longer than cold items (LRU working correctly) and all items still expire after their TTL (data freshness guaranteed)

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.

3 participants