SPA Page caching for surviving thundering herds#15040
Conversation
- SPA shells render no user-specific data, so one rendered body serves every client. - cache_no_user_data stores that body in process_cache behind a single-flight election. - Refresh is stale-while-revalidate: requests keep serving the stored copy while the election winner re-renders in a background thread. - Only a cold miss blocks on a render. - The refresh fires at a randomized moment before the deadline, so the entry never hard-expires and refreshes do not fire in lockstep across keys. - UserAuthView drops its server-side authenticated redirect, so it renders no user-specific data. - app.js redirects authenticated users to their landing page client-side instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PeRsMyx4NSFFKrWpD4sZ2o
- Decorating a view with cache_no_user_data registers it for warming. - A startup hook warms each registered view for the device language; other languages warm lazily on the cold-miss path. - The display language is baked into every cached shell, so a post-provision change to it re-warms them via a DeviceSettings post_save signal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PeRsMyx4NSFFKrWpD4sZ2o
github-actions
Bot
added
DEV: backend
🔵 Review postedLast updated: 2026-07-20 16:01 UTC |
rtibbles
changed the title
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15040 — the cache_no_user_data rewrite (stale-while-revalidate, shared dual-encoding bodies, startup warming) is well-structured and unusually well-tested, and the UserAuthView server-redirect → app.js beforeEach move is behavior-preserving. No blocking issues. The findings below are behavior-preservation and lifecycle questions worth a confirmation before merge. CI pending; no UI files changed so manual QA was not required.
- suggestion (pwa/views.py) — manifest + service-worker client cache drops from 7 days to ~15s.
- suggestion (decorators.py) — bodies stored with
timeout=Noneaccumulate forever on a Redis-backedprocess_cache. - suggestion (decorators.py) — cold-miss render is not coalesced (thundering herd for non-device languages).
- suggestion (decorators.py) — refresh runs on a raw daemon thread outside the task framework.
- suggestion (app.js) — redirect guard depends on session state not synchronously populated at first navigation.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| @method_decorator(gzip_page, name="dispatch") | ||
| @method_decorator(cache_page(60 * 60 * 24 * 7), name="dispatch") | ||
| @cache_no_user_data |
There was a problem hiding this comment.
suggestion: Both PWA views previously carried cache_page(60*60*24*7) → Cache-Control: max-age=604800. Under cache_no_user_data, _finalize calls patch_response_headers(response, cache_timeout=BODY_CACHE_REFRESH) (=15s), so clients now get max-age=15 for manifest.webmanifest and sw.js. A short TTL is fine for sw.js, but the manifest going from weekly to 15s client caching is a real behavioral change. Confirm the reduced client TTL here is intended (uniform freshness window) rather than a side effect of consolidating the decorators.
There was a problem hiding this comment.
Thanks for confirming — the uniform freshness window makes sense. Nothing to change here.
| ) | ||
| # timeout=None: the body never hard-expires, so a stale copy is always | ||
| # available to serve while one request refreshes it. | ||
| process_cache.set(body_key, (variants, refresh_at), None) |
There was a problem hiding this comment.
suggestion: timeout=None (intentional, so a stale copy is always servable) means entries never expire. The key embeds kolibri_version and the language-prefixed path, so across version upgrades and each distinct language served, entries accumulate permanently. Fine for a locmem process_cache (clears on restart), but a Redis-backed process_cache without an eviction policy would retain old-version/rare-language bodies indefinitely. Consider a generous hard TTL (hours) while still keying refresh off refresh_at, so orphaned entries age out while live paths stay warm.
| ): | ||
| self._refresh_in_background(request, body_key, lock_key) | ||
| return self._conditional(request, self._pick(request, variants)) | ||
| # Cold miss: render inline, since there is nothing to serve stale. |
There was a problem hiding this comment.
suggestion: The refresh path elects a single refresher via process_cache.add(lock_key, ...), but the cold-miss branch renders inline with no such guard. N concurrent first requests for an unwarmed path (any non-device language, or the window before the startup warm completes) each render the shell synchronously — the thundering-herd case this PR targets. Not a regression (the old decorator had no lock either), but given the cold-start goal, worth gating cold rendering behind the same election or noting it as a follow-up.
| process_cache.set(body_key, (variants, refresh_at), None) | ||
| return self._pick(request, variants) | ||
|
|
||
| def _refresh_in_background(self, request, body_key, lock_key): |
There was a problem hiding this comment.
suggestion: _refresh_in_background renders a full Django view on a bare daemon thread, outside the task framework that AGENTS.md/backend docs steer async work through (observable, retryable, resource-governed). There are reasonable arguments for the escape hatch (a 15s refresh shouldn't queue behind real jobs), and it is bounded — single-flight election, DB connections closed in finally, translation.override, synthetic request. Surfacing as a design-tension: is an in-process thread the intended escape hatch, and any concern about rendering with middleware not run on this path? The del request.session + _build_request seam makes the no-user-data contract explicit, which mitigates most of it.
| // Authenticated users have no business on the auth pages; redirect them | ||
| // to their landing page. | ||
| const { isUserLoggedIn } = useUser(); | ||
| if (get(isUserLoggedIn)) { |
There was a problem hiding this comment.
suggestion: This guard reads isUserLoggedIn on every navigation, including the first one during ready() (registered before super.ready(), runs ahead of initializeFlow). sessionState in useUser initializes anonymous and is populated asynchronously by the heartbeat's first poll, so on a true initial load isUserLoggedIn can still be false for an already-authenticated user and this guard won't fire. That appears fine because the backend view redirects logged-in users server-side before the SPA loads, making this a secondary net for the refocus-another-tab case (where the heartbeat has already populated state). Flagging so the timing dependency is confirmed.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15040 — delta re-review of the same HEAD 4616e647; no new commits since my last pass and none of the five prior suggestions has been addressed. 0 of 5 prior findings resolved; all 5 still open. No new code to evaluate, so nothing new raised. CI is still pending. Verdict stays COMMENT — all five are non-blocking behavior-preservation/lifecycle questions for the author's consideration:
kolibri/plugins/pwa/views.py:15— PWA manifest & service-worker client cache drops from 7 days (cache_page(60*60*24*7)) tomax-age=15; confirm intended.kolibri/core/decorators.py:482—timeout=Nonebodies never expire on a Redis-backedprocess_cache; unbounded growth risk.kolibri/core/decorators.py:416— cold-miss render isn't coalesced; thundering-herd for non-device languages.kolibri/core/decorators.py:485— background refresh runs on a raw daemon thread outside the task framework.kolibri/plugins/user_auth/frontend/app.js:34— redirect guard readsisUserLoggedInon the first navigation, before session state is synchronously populated.
Prior-finding status
UNADDRESSED — kolibri/plugins/pwa/views.py:15 — PWA client cache 7d→15s
UNADDRESSED — kolibri/core/decorators.py:482 — timeout=None bodies never expire
UNADDRESSED — kolibri/core/decorators.py:416 — cold-miss render not coalesced
UNADDRESSED — kolibri/core/decorators.py:485 — refresh on raw daemon thread
UNADDRESSED — kolibri/plugins/user_auth/frontend/app.js:34 — redirect guard vs first-nav session state
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
2 participants
Summary
Our existing caching for SPA pages is incredibly inefficient, with the page getting rerendered on every request, regardless, even if an ETag driven "not modified" is then returned.
This PR changes that around so that we nearly always serve quickly, with cache refreshes happening in two different ways in the background:
This PR also updates the user auth page to leverage the "cache no user data" decorator - previously it did not because it initiated redirects in the backend to sign people into the right page. This is an uncommon occurrence, so instead this is deferred to JS in page load instead. This allows the user auth page to be cached also, which helps with the very start of a thundering herd classroom session.
References
This was one of the more promising avenues for performance improvement while doing load testing
Reviewer guidance
Is the code comprehensible? Is the previously desirable caching behaviour maintained, does the new caching behaviour introduce any unexpected side effects?
I thought about making an explicit Kolibri hook for the cache no user decorator in order to collect the views, but it felt like a lot more overhead - but open to making it explicit if helpful.
Benchmarks
Baseline against
develop, which already caches the ETag but re-renders the shell on every request.All figures are deltas against it.
"Rollover" means that the server would at some point fail under the load.
This change made the difference for both the online server and the raspberry pi (without kolibri server - using as a low end baseline) to actually serve all clients, albeit slowly.
AI usage
I used Claude Code extensively for this work, especially the comparative benchmarking to test different changes in a matrix across the different targets. This particular PR was then worked up from those changes in collaboration with Claude Code.