fix(tokens): prune pricing cache files after each refresh + drop deprecated utcnow - #5517
fix(tokens): prune pricing cache files after each refresh + drop deprecated utcnow#5517ksk2023 wants to merge 2 commits into
Conversation
_fetch_and_cache_pricing_data writes a new timestamped pricing_YYYYMMDD_HHMMSS.json on every refresh, but clean_old_caches() was never invoked anywhere — cache files accumulated indefinitely in ~/.cache/browser_use/token_cost for long-running sessions. Call clean_old_caches() after a successful cache write so only the most recent files (keep_count=3) survive. Also replace deprecated datetime.utcnow() in DBStyleEntry with datetime.now(timezone.utc) — utcnow() is deprecated since Python 3.12 and returns a naive datetime, inconsistent with the timezone-aware usage elsewhere in the codebase. Adds regression tests covering the pruning behavior and the post-write cleanup invocation.
|
|
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Review follow-ups: - clean_old_caches() now delegates to a synchronous _clean_old_caches_sync helper executed via anyio.to_thread.run_sync, so reading and parsing every cached JSON file no longer blocks the event loop when the cache directory has a large backlog. The post-refresh call in _fetch_and_cache_pricing_data uses the same path. - The prune test now decouples filename order from age order (oldest file has the lexicographically largest name), so it fails if pruning ever sorts by filename instead of st_mtime. - The fetch test now serves the pricing payload from pytest-httpserver with the real httpx client and the real clean_old_caches, asserting the final file list end to end instead of stubbing the cleanup call.
|
Addressed the review feedback in f08a0f9:
Both tests pass locally ( |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/ci/test_pricing_cache_cleanup.py">
<violation number="1" location="tests/ci/test_pricing_cache_cleanup.py:45">
P2: The test cannot detect filename-based pruning: ages rise monotonically with the filename (`i` defaults give 000000→0h … 000002→2h), so the three newest-by-mtime files are exactly the three lexicographically smallest names, and a buggy prune that keeps the first N names returns the same `remaining` list the assertion expects. The docstring claims a filename-based sort would keep the wrong files, but that requires the age order to be non-monotonic with name order. The previous version of this test (ages `6-i`, expecting 000003–000005) actually caught that case; this version does not. Assign ages so the mtime-newest set is neither the smallest nor the largest three names (e.g. ages `000000→48, 000001→1, 000002→24, 000003→3, 000004→4, 000005→5`), and assert the resulting non-contiguous file set.</violation>
</file>
<file name="browser_use/tokens/service.py">
<violation number="1" location="browser_use/tokens/service.py:177">
P2: When a `TokenCost` subclass overrides `clean_old_caches()`, a refresh no longer invokes that method because `_fetch_and_cache_pricing_data()` calls the private helper directly. Call `await self.clean_old_caches()` here; its implementation already offloads `_clean_old_caches_sync` to a worker.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
|
||
| # Ages are deliberately opposite to filename order: pricing_000005 is the | ||
| # OLDEST file (48h) and pricing_000000 is the NEWEST (0h). | ||
| ages = {'pricing_20260101_000005.json': 48, 'pricing_20260101_000004.json': 24, 'pricing_20260101_000003.json': 5} |
There was a problem hiding this comment.
P2: The test cannot detect filename-based pruning: ages rise monotonically with the filename (i defaults give 000000→0h … 000002→2h), so the three newest-by-mtime files are exactly the three lexicographically smallest names, and a buggy prune that keeps the first N names returns the same remaining list the assertion expects. The docstring claims a filename-based sort would keep the wrong files, but that requires the age order to be non-monotonic with name order. The previous version of this test (ages 6-i, expecting 000003–000005) actually caught that case; this version does not. Assign ages so the mtime-newest set is neither the smallest nor the largest three names (e.g. ages 000000→48, 000001→1, 000002→24, 000003→3, 000004→4, 000005→5), and assert the resulting non-contiguous file set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/ci/test_pricing_cache_cleanup.py, line 45:
<comment>The test cannot detect filename-based pruning: ages rise monotonically with the filename (`i` defaults give 000000→0h … 000002→2h), so the three newest-by-mtime files are exactly the three lexicographically smallest names, and a buggy prune that keeps the first N names returns the same `remaining` list the assertion expects. The docstring claims a filename-based sort would keep the wrong files, but that requires the age order to be non-monotonic with name order. The previous version of this test (ages `6-i`, expecting 000003–000005) actually caught that case; this version does not. Assign ages so the mtime-newest set is neither the smallest nor the largest three names (e.g. ages `000000→48, 000001→1, 000002→24, 000003→3, 000004→4, 000005→5`), and assert the resulting non-contiguous file set.</comment>
<file context>
@@ -10,81 +10,83 @@
+ # Ages are deliberately opposite to filename order: pricing_000005 is the
+ # OLDEST file (48h) and pricing_000000 is the NEWEST (0h).
+ ages = {'pricing_20260101_000005.json': 48, 'pricing_20260101_000004.json': 24, 'pricing_20260101_000003.json': 5}
+ DEFAULT_SOURCE = 'https://raw.githubusercontent.com/BerriAI/litellm/main/model_prices_and_context_window.json'
+
</file context>
| # The cleanup reads and parses every cached JSON file, so run it in a | ||
| # worker thread to keep the event loop responsive when the cache | ||
| # directory has a large backlog. | ||
| await anyio.to_thread.run_sync(self._clean_old_caches_sync) |
There was a problem hiding this comment.
P2: When a TokenCost subclass overrides clean_old_caches(), a refresh no longer invokes that method because _fetch_and_cache_pricing_data() calls the private helper directly. Call await self.clean_old_caches() here; its implementation already offloads _clean_old_caches_sync to a worker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At browser_use/tokens/service.py, line 177:
<comment>When a `TokenCost` subclass overrides `clean_old_caches()`, a refresh no longer invokes that method because `_fetch_and_cache_pricing_data()` calls the private helper directly. Call `await self.clean_old_caches()` here; its implementation already offloads `_clean_old_caches_sync` to a worker.</comment>
<file context>
@@ -171,7 +171,10 @@ async def _fetch_and_cache_pricing_data(self) -> None:
+ # The cleanup reads and parses every cached JSON file, so run it in a
+ # worker thread to keep the event loop responsive when the cache
+ # directory has a large backlog.
+ await anyio.to_thread.run_sync(self._clean_old_caches_sync)
except Exception as e:
logger.debug(f'Error fetching pricing data: {e}')
</file context>
| await anyio.to_thread.run_sync(self._clean_old_caches_sync) | |
| \t\t\tawait self.clean_old_caches() |
Description
TokenCost._fetch_and_cache_pricing_datawrites a new timestamped filepricing_YYYYMMDD_HHMMSS.jsonon every refresh, butclean_old_caches()had no call sites anywhere in the codebase — cache files accumulated indefinitely in~/.cache/browser_use/token_cost(one new file per refresh, ≥1/day for long-running services).clean_old_caches()after each successful cache write so only the most recent files (keep_count=3) survive.datetime.utcnow()inDBStyleEntry.created_atwithdatetime.now(timezone.utc)—utcnow()is deprecated since Python 3.12 and returns a naive datetime, inconsistent with the timezone-aware usage elsewhere in the codebase. Output now correctly carries+00:00.Testing
tests/ci/test_pricing_cache_cleanup.py:test_clean_old_caches_prunes_to_keep_count— 6 cache files → pruning keeps the 3 newesttest_fetch_and_cache_invokes_cleanup— network mocked; verifiesclean_old_cachesis invoked exactly once after a successful write and the pricing payload is cachedtest_fetch_and_cache_invokes_cleanupfails without the fix.Summary by cubic
Prunes pricing cache files after each refresh and runs cleanup off the event loop to prevent I/O stalls. Also replaces deprecated
utcnowwith a timezone-aware timestamp._fetch_and_cache_pricing_data()calls_clean_old_caches_syncviaanyio.to_thread.run_sync;clean_old_caches()delegates to the same helper. Keeps only the 3 newest files by mtime; older files are removed on the next refresh. No config changes.DBStyleEntry.created_atnow usesdatetime.now(timezone.utc)so timestamps include+00:00.httpxwithpytest-httpserverthat confirms post-write cleanup and correct caching.Written for commit f08a0f9. Summary will update on new commits.