Repository navigation
feat: Add client-side cache buster for expired shapes to prevent 409s - #3103
Conversation
When a shape 409s, the client now stores this information in localStorage and adds a cache_buster parameter to future requests for that shape, preventing redundant 409 responses and reducing app loading latency. - Add localStorage helpers for tracking expired shapes per shape handle - Add SHAPE_CACHE_BUSTER_QUERY_PARAM for expired shape requests - Store expired shape handles when 409 responses occur - Add cache buster to URL construction for known expired shapes Fixes #3100 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3103 +/- ##
==========================================
+ Coverage 77.40% 77.57% +0.17%
==========================================
Files 169 170 +1
Lines 9007 9076 +69
Branches 282 293 +11
==========================================
+ Hits 6972 7041 +69
Misses 2033 2033
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kevin-dp
left a comment
There was a problem hiding this comment.
Looks good, left a few very minor comments.
| #markShapeExpired(shapeHandle: string): void { | ||
| if (typeof localStorage === `undefined`) return | ||
| try { | ||
| localStorage.setItem(this.#getShapeStorageKey(shapeHandle), `true`) |
There was a problem hiding this comment.
nit: we can remove the if test if we do localStorage?.setItem(...)
| export const CHUNK_UP_TO_DATE_HEADER = `electric-up-to-date` | ||
| export const COLUMNS_QUERY_PARAM = `columns` | ||
| export const LIVE_CACHE_BUSTER_QUERY_PARAM = `cursor` | ||
| export const SHAPE_CACHE_BUSTER_QUERY_PARAM = `cache_buster` |
There was a problem hiding this comment.
Do we need to introduce a separate one? Could we not use the cursor also for this one? Since up till now we only use cursor for cache busting live requests, it seems that we can reuse it to bust non-live shape requests too? Just needs a renaming from LIVE_CACHE_BUSTER_QUERY_PARAM to CACHE_BUSTER_QUERY_PARAM.
There was a problem hiding this comment.
The LLM thought the same thing 😆
I suppose in theory we could because this new cache busting is only relevant when we're not live and reading cached values & the live cache cursor is needed for preventing CDNs from returning cached live requests 🤔
But they are different. This PR introduces an actual normal cache buster where the live stuff is a cursor — so cache busting but different flavor as it's helping ensure we move forward through log time.
|
I'm slightly nervous of accumulating these expired shape handles in local storage as individual keys, while it's true we can probably hold 10s k in it, it seems a little messy. Could we:
|
|
@samwillis hmm yeah a little LRU cache would be needed for very large apps w/ lots of churn. Easy enough to add... |
Keeping track of a set of expired shape handles seems like the perfect use case for a bloom filter. It will tell us whether the shape is possibly expired or definitely not expired. If it tells us it is possibly expired, we add a cache buster (no real harm if it would be a false positive). Might be overkill for apps with "few" shapes as we can easily store several thousand shape handles in local storage but might be worth it for apps with huge amounts of shapes. |
Yeah with a ton more shapes that could make sense — I'm more worried though about all the extra code for a bloom filter. I'm thinking of capping the number of shapes at 250 as that's a lot more than we support right now anyways. That'll be cheap still to load (10-15kb) and iterate over. |
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…y so it handles offset=-1 when there is no handle
|
This is ready for another review. |
There was a problem hiding this comment.
I think we are close, and maybe there, but wanted to clarify some thinking.
You are saving one expired handle per url, but as we have seen there can be multiple that cause weird loops. What was your thinking here?
In my mind, as handles are uuids we don't need to bucket them by url, just have a LRU cache of the last n of them. I would keep this as a list with a max length serialised to storage, and as a set in memory for fast lookup.
It may well be that per url is ok, in which case this is good to go I think.
Once thing I think thats worth following up on in a further PR is standardising a key-value metadata interface that ShapeStream takes as a config option. This can default to using a thin wrapper around localStorage in the browser, but provides a route to other storages for React Native / Capacitor / Electron / Node.
| * LRU cache for tracking expired shapes with automatic cleanup | ||
| */ | ||
| export class ExpiredShapesCache { | ||
| private data: Record<string, ExpiredShapeCacheEntry> = {} |
There was a problem hiding this comment.
We are only keeping the last expired handle per url? Is that enough?
There was a problem hiding this comment.
Yeah the idea is that a given shape definition's browser cache might no longer be valid — so the cache key is by shape definition and we only need to store the last-invalidated handle to get out of that handle's cache. We only load one shape / shape definition so no need to store multiple handles or worry about additional URLs from other timelines of the shape that we might have cached in the past.
…roxy support The canonicalShapeKey function now excludes only Electric protocol parameters (live, handle, offset, cursor, expired_handle) while including all other parameters including custom ones used by proxies. This ensures proper cache behavior when proxies use client-provided metadata to construct shape definitions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Changes
SHAPE_CACHE_BUSTER_QUERY_PARAMconstant for expired shape requestsTest plan
Fixes #3100
🤖 Generated with Claude Code