-
Notifications
You must be signed in to change notification settings - Fork 4
Implement stale while revalidate (SWR) cache strategy #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
kxzk
merged 24 commits into
simplepractice:main
from
drborges:feature/stale-while-revalidate
Jan 22, 2026
Merged
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
593f0e8
feat(deps): Add concurrent-ruby gem
drborges d2a9e13
feat(config): add stale-while-revalidate configuration options
drborges 098c59b
refactor(cache): update CacheEntry to support fresh/stale/expired states
drborges f761e8f
feat(cache): implement stale-while-revalidate in RailsCacheAdapter
drborges 1c184ef
feat(client): wire SWR configuration to RailsCacheAdapter
drborges d0d4772
refactor(api_client): add intelligent caching strategy selection
drborges 26ce6d6
fix(rubocop): disable RSpec/MultipleMemoizedHelpers for a few specs
drborges d5b915d
feat(prompt_cache): Add SWR to PromptCache
drborges e8ae59d
feat(config): Allow configuring SWR caches that never expire
drborges 687e1e0
refactor(config): Apply better defaults for stale_ttl
drborges ab7628d
docs(swr): Add documentation on stale while revalidate behavior
drborges b9abbd8
fix(swr): Consider SWR enabled as long as stale_ttl is positive
drborges dd24ee8
style(lint): Disable ClassLength for ApiClient
drborges 316706a
refactor(swr): raises error when attempting to use SWR and invalid st…
drborges 16e7618
style(comments): Remove emoji from comments
drborges e18b701
refactor(cache): Unify locking API across cache implementations
drborges 30b72d3
refactor(cache): Move fetch_with_lock to RailsCacheAdapter
drborges 45425f2
refactor(cache): Remove expires_in parameter from set/cache_set methods
drborges 737539f
feat(shutdown): Add cache shutdown to client cleanup
drborges f7e9fdc
refactor(config): Use :indefinite symbol instead of Float::INFINITY f…
drborges c2b2b4b
feat(config): Add helpful validation for SWR with nil cache_stale_ttl
drborges df3ace7
refactor(swr): Set min_threads to 0 for on-demand thread creation
drborges f76f579
docs(swr): Document memory leak risk with in-memory locks in PromptCache
drborges 9f26069
Merge branch 'main' of github.com:simplepractice/langfuse-rb into fea…
drborges File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Configuration Complexity
Concern: This PR adds 3 new configuration options (
cache_stale_while_revalidate,cache_stale_ttl,cache_refresh_threads). I'd push back on exposing all of these to users.Principle: Every configuration option is a decision the user has to make. Good defaults mean fewer decisions.
Suggestion: Consider collapsing to a single option:
Instead of:
Just:
Reasoning:
cache_stale_ttldefaulting tocache_ttlis already the right choice 99% of the time - why expose it?cache_refresh_threadsat 5 is fine for virtually all deployments - this is an implementation detail, not a user concernThe bar for adding config options should be: "Will a significant number of users need to change this from the default?" If not, hardcode it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We absolutely need to set the TTL to infinite. Not exposing the TTL setting won't work for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what the use case for a non-infinite TTL?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it certainly makes sense to hide
cache_refresh_threads. Exposingcache_stale_ttlwould be more of a flexibility in my opinion in case clients have very specific needs.With that said, if I choose to enable SWR, I would more often than not want an infinite grace period, e.g.
cache_stale_ttl = :indefinitefor high availability reasons, then usecache_ttlto control how often cache is refreshed.Based on that, only exposing
config.cache_stale_while_revalidatewould make sense to me if that defaultscache_stale_ttlto:indefiniteas per the proposed implementation.Does that make sense?