Skip to content

Conversation

zscgeek
Copy link

@zscgeek zscgeek commented Jul 16, 2025

feat: add return_stale_on_timeout parameter for stale-while-revalidate caching

Adds new return_stale_on_timeout parameter that enables returning stale
cached values when wait_for_calc_timeout expires, instead of triggering
a new calculation. This implements a stale-while-revalidate pattern that
keeps applications responsive while ensuring background cache refresh.

Key changes:

  • Add return_stale_on_timeout parameter to all cache backends
  • Modify core logic to return stale values on RecalculationNeeded exception
  • Fix memory core timeout handling to properly check wait_for_calc_timeout
  • Add comprehensive test suite with 7 test cases
  • Update documentation and maintain backward compatibility

When enabled, the behavior follows this pattern:

  1. Fresh values (≤ stale_after) return immediately
  2. Stale values trigger background refresh
  3. Caller waits up to wait_for_calc_timeout for refresh to complete
  4. If timeout expires, return stale value instead of blocking
  5. Background refresh continues for next request

zscgeek added 2 commits July 16, 2025 13:16
…e caching

Adds new `return_stale_on_timeout` parameter that enables returning stale
cached values when `wait_for_calc_timeout` expires, instead of triggering
a new calculation. This implements a stale-while-revalidate pattern that
keeps applications responsive while ensuring background cache refresh.

Key changes:
- Add return_stale_on_timeout parameter to all cache backends
- Modify core logic to return stale values on RecalculationNeeded exception
- Fix memory core timeout handling to properly check wait_for_calc_timeout
- Add comprehensive test suite with 7 test cases
- Update documentation and maintain backward compatibility

When enabled, the behavior follows this pattern:
1. Fresh values (≤ stale_after) return immediately
2. Stale values trigger background refresh
3. Caller waits up to wait_for_calc_timeout for refresh to complete
4. If timeout expires, return stale value instead of blocking
5. Background refresh continues for next request

Closes: Implements stale-while-revalidate caching pattern
@zscgeek zscgeek requested a review from shaypal5 as a code owner July 16, 2025 13:06
@zscgeek
Copy link
Author

zscgeek commented Jul 16, 2025

bugbot run

Copy link

codecov bot commented Jul 16, 2025

Codecov Report

❌ Patch coverage is 26.08696% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.39%. Comparing base (2a4d1c9) to head (f1c214a).
⚠️ Report is 95 commits behind head on master.

Files with missing lines Patch % Lines
src/cachier/cores/memory.py 15.38% 10 Missing and 1 partial ⚠️
src/cachier/core.py 14.28% 4 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (2a4d1c9) and HEAD (f1c214a). Click for more details.

HEAD has 30 uploads less than BASE
Flag BASE (2a4d1c9) HEAD (f1c214a)
30 0
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #297       +/-   ##
===========================================
- Coverage   97.84%   63.39%   -34.46%     
===========================================
  Files           8       12        +4     
  Lines         511      948      +437     
  Branches       88      108       +20     
===========================================
+ Hits          500      601      +101     
- Misses         10      329      +319     
- Partials        1       18       +17     
Flag Coverage Δ
mongodb 48.73% <26.08%> (?)
postgres 48.20% <21.73%> (?)
redis 45.88% <21.73%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/cachier/config.py 81.01% <100.00%> (ø)
src/cachier/cores/base.py 96.22% <100.00%> (ø)
src/cachier/cores/mongo.py 97.01% <ø> (ø)
src/cachier/cores/pickle.py 22.94% <100.00%> (ø)
src/cachier/cores/redis.py 65.89% <ø> (ø)
src/cachier/cores/sql.py 94.33% <ø> (ø)
src/cachier/core.py 78.97% <14.28%> (ø)
src/cachier/cores/memory.py 27.77% <15.38%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d02182...f1c214a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shaypal5
Copy link
Member

Please rebase over master before continuing to work - a lot of new code was added recently. @zscgeek

@zscgeek
Copy link
Author

zscgeek commented Jul 21, 2025

Please rebase over master before continuing to work - a lot of new code was added recently. @zscgeek

Hi! I will do in the next day or so.

@Borda Borda changed the title add return_stale_on_timeout parameter for stale-while-revalidate caching add return_stale_on_timeout parameter for stale-while-revalidate caching Oct 2, 2025
@Borda Borda requested a review from Copilot October 2, 2025 10:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new return_stale_on_timeout parameter that implements stale-while-revalidate caching behavior. When enabled, the feature returns cached stale values if wait_for_calc_timeout expires, instead of triggering a new calculation, helping keep applications responsive during cache refreshes.

Key Changes:

  • Add return_stale_on_timeout parameter to all cache backends and core logic
  • Fix memory core timeout handling to properly respect wait_for_calc_timeout
  • Implement comprehensive test coverage with 7 test scenarios

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/cachier/core.py Main decorator updated with new parameter and stale-return logic
src/cachier/cores/base.py Base class updated to accept new parameter
src/cachier/cores/memory.py Memory backend enhanced with proper timeout handling
src/cachier/cores/pickle.py Pickle backend updated with new parameter
src/cachier/cores/mongo.py MongoDB backend updated with new parameter
src/cachier/cores/sql.py SQL backend updated with new parameter
src/cachier/cores/redis.py Redis backend updated with new parameter
src/cachier/config.py Global configuration updated with new parameter
tests/test_return_stale_on_timeout.py Comprehensive test suite for the new feature
demo_return_stale_on_timeout.py Demo script showcasing the feature

# If we weren't signaled and the entry is still processing, check timeout
if not signaled:
time_spent += 1
self.check_calc_timeout(time_spent)
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable hash_key is used but not defined in this method. It should be key (which is the parameter) or self._hash_func_key(key) to get the correct hash key.

Suggested change
self.check_calc_timeout(time_spent)
self.check_calc_timeout(time_spent, hash_key)

Copilot uses AI. Check for mistakes.

@shaypal5 shaypal5 removed their request for review October 2, 2025 13:15
@Borda
Copy link
Contributor

Borda commented Oct 3, 2025

@zscgeek I have resolved conflicts, but I am unable to push the update to your PR :(

@shaypal5
Copy link
Member

shaypal5 commented Oct 3, 2025

@zscgeek I have resolved conflicts, but I am unable to push the update to your PR :(

I'll pull the PR into a new dev branch on the main repo, which you can then use to work on as you see fit. :)

@Borda

@shaypal5 shaypal5 changed the base branch from master to return_stale_on_timeout October 3, 2025 20:43
@shaypal5 shaypal5 deleted the branch python-cachier:master October 3, 2025 20:48
@shaypal5 shaypal5 closed this Oct 3, 2025
@shaypal5 shaypal5 reopened this Oct 3, 2025
@shaypal5 shaypal5 changed the base branch from return_stale_on_timeout to master October 3, 2025 20:49
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is being reviewed by Cursor Bugbot

Details

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

except RecalculationNeeded:
if _return_stale_on_timeout and entry and entry.value is not None:
_print("Timeout reached, returning stale value.")
return entry.value
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Cache Stale Value Handling Issues

The return_stale_on_timeout logic at lines 378 and 399 has two issues. It prevents returning stale None values when allow_none=True, inconsistent with other None handling. Additionally, returning a stale value on timeout leaves the cache entry marked as _processing=True, causing an inconsistent state for subsequent lookups.

Additional Locations (1)

Fix in Cursor Fix in Web

# If we weren't signaled and the entry is still processing, check timeout
if not signaled:
time_spent += 1
self.check_calc_timeout(time_spent)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Race Condition and Unnecessary Timeout Check

The wait_on_entry_calc method has two issues. A race condition can cause an AttributeError if entry._condition becomes None between releasing and re-acquiring the lock in the waiting loop. Additionally, the timeout check may be called unnecessarily, potentially raising RecalculationNeeded even if the calculation completes successfully, if it finishes right after a wait timeout.

Fix in Cursor Fix in Web

@shaypal5
Copy link
Member

shaypal5 commented Oct 3, 2025

Closed as @zscgeek seems unavailable at the moment to continue work on the PR.
Moved to #308 for @Borda to work on.
@zscgeek should feel free to reopen this. :)

@shaypal5 shaypal5 closed this Oct 3, 2025
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