fix(cache): clamp the background drain budget to the cache limit#443
Draft
sonhmai wants to merge 2 commits into
Draft
fix(cache): clamp the background drain budget to the cache limit#443sonhmai wants to merge 2 commits into
sonhmai wants to merge 2 commits into
Conversation
max_drain_bytes=None previously meant an unbounded it.drain(): a partially consumed stream (head of a multi-GB object) kept downloading and buffering the whole remainder in process memory, then the oversized add evicted every other entry and finally itself, wiping the cache. Draining past cache_limit can never produce a cache hit, so the budget now resolves via FileCacheMixin.drain_budget (TS drainBudget): min(max_drain_bytes or cache_limit, cache_limit). None keeps meaning "default", now bounded; the unbounded path is gone. Snapshot state still round-trips the raw configured value.
Review findings on the drain clamp: drain_bounded returned early without closing the underlying iterator, so streaming backends kept their HTTP response open until GC, and this path is now the default. drain_bounded (TS drainBounded) closes the source on budget exceed; CachableAsyncIterator grows an aclose() delegating to the source. Also drop the console.info on the TS skip path: budget exceed is a normal event now and core has no logger, so it polluted stdout. The Python side keeps logger.info, which is silent unless configured.
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.
Problem
max_drain_bytesdefaults toNone, which the background drain treated as unbounded. A partially consumed streamed read (head -c 100of a multi-GB S3 object) spawned_background_drain, which kept downloading and buffering the entire remainder in process memory. The oversizedaddthen evicted every other entry and finally the new entry itself, so the full download bought nothing and wiped the cache.Draining more than
cache_limitbytes can never produce a cache hit: eviction runs right after the add and an entry larger than the limit is always dropped. Unbounded draining is therefore pure waste and there is no reason to keep it as an option.Change
FileCacheMixin.drain_budget(TSdrainBudget(cache)) resolves the effective budget:min(max_drain_bytes or cache_limit, cache_limit)._background_drain/backgroundDrainalways usedrain_bounded; the unboundeddrain()branch is gone. Exceeding the budget logs and skips the fill, and the next read fetches fresh from the resource.Nonenow means "default to the cache limit" instead of "unbounded"; values above the limit are clamped.max_drain_bytes, so old snapshots withNoneload with the new bounded semantics.Tests
drain_budgetdefault/keep/clamp cases; default budget stops a stream larger thancache_limitand still caches smaller ones; clamp applies whenmax_drain_bytesexceeds the limit.drainBudgetcases plus the mirrored drain behavior tests.packages/core/src/cache/file), prettier and eslint on the touched files, and the Python pre-commit hooks.