Memoize DeviceSettings reads in process memory#14894
Conversation
github-actions
Bot
added
DEV: backend
Build Artifacts
Smoke test screenshot |
4df3e73 to
2b5baf4
Compare
🔵 Review postedLast updated: 2026-07-20 04:06 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14894 adds a 1s in-process memo over process_cache for DeviceSettings reads. Invalidation on save/delete, the up-front clear before full_clean(), not caching misses, and the test-suite reset hooks (conftest + auth helpers) are all sound. CI passing; no UI files changed, so UI/manual-QA verification is not applicable.
No blocking issues. Findings:
- suggestion (models.py:108) — the memo now hands the same mutable instance to every caller within the TTL, a behavior change from the copy-returning cache; interacts with the get→mutate→save pattern. See inline.
- suggestion (test:46) — the save-invalidation test aliases the memoized instance, so it can't fail if invalidation is removed.
- nitpick (test:38) — comment says "sqlite-backed" but the test cache is LocMem.
@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
| model = None | ||
| model = _device_settings_memo.get() | ||
| if isinstance(model, DeviceSettings): | ||
| return model |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The memo returns the same DeviceSettings object to every get() caller in the process for the TTL window, whereas process_cache (DiskCache) previously deserialized a fresh copy per call. This collides with the get→mutate-in-place→save pattern in device/utils.py (set_device_settings/provision_device): thread A mutates the shared instance via setattr before calling save() (which is where the memo clears), so a concurrent thread B's get_device_setting(...) can read uncommitted values that full_clean() may still reject. Low-frequency, and all current call sites are read-only or mutate-then-save, so it's safe today — but the invariant ("returned instance is shared; don't mutate without an immediate save()") is non-obvious. Either copy.copy(model) on set/get (preserves the read-savings goal with per-caller isolation) or document it on get().
There was a problem hiding this comment.
Fixed — the memo now copy()s on both set and get, so each caller gets its own instance as DiskCache deserialization did. Not just theoretical: DeviceSettingsView.patch binds a DRF serializer to the returned instance and DeviceNameView.patch setattrs before save().
| # Populate the memo, then change the setting through a fresh save. | ||
| DeviceSettings.objects.get() | ||
| ds = DeviceSettings.objects.get() | ||
| ds.language_id = "es" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: ds here is the same object the memo holds (get() returns the memoized instance), so mutating ds.language_id mutates the memoized instance in place. With the _device_settings_memo.clear() removed from save() the memo still holds this now-"es" object, so line 48 passes anyway — the test can't fail on the invalidation it names. Change the value through a non-aliasing path, e.g. DeviceSettings(language_id="es").save() on a fresh instance, so a missing clear leaves the stale "en" and the test fails. (test_delete_invalidates_in_process_memo already fails correctly.)
| DeviceSettings.objects.get() | ||
| DeviceSettings.objects.get() | ||
| # First read populates the in-process memo; the rest are served from it | ||
| # without touching the (sqlite-backed) process_cache. |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: Under test settings process_cache is a LocMemCache, not sqlite/DiskCache — the "(sqlite-backed)" parenthetical could mislead someone debugging this test. Drop it or say "the process_cache backend".
2b5baf4 to
64cb1cc
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14894 — all three prior findings resolved; one minor forward-looking suggestion below.
The memo now copies on both set() and get(), the save-invalidation test fetches via a non-aliasing filter(pk=1).first(), and the misleading "(sqlite-backed)" comment is gone. Clearing the memo before full_clean() in save() is a nice defensive touch — a ValidationError can't leave a stale entry.
- suggestion (models.py) —
copy()is shallow; noted inline as a future-safety guard for the sharedextra_settingsdict.
Prior-finding status
RESOLVED — models.py:111 — memo handed same mutable instance to callers (now copies on set/get)
RESOLVED — test_device_settings.py:47 — save-invalidation test aliased memoized instance (now non-aliasing filter().first())
RESOLVED — test_device_settings.py:38 — misleading "(sqlite-backed)" comment (removed)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
|
|
||
| class _DeviceSettingsMemo(object): | ||
| # Copies in and out so each caller gets its own instance, as deserializing |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: copy() is a shallow copy, so the JSONField extra_settings dict is shared between the memo's stored instance and every returned copy. Fine today — all callers reassign whole fields via setattr then save() (which clears the memo). But a future caller that mutates instance.extra_settings[...] = x in place without saving would silently corrupt the memoized value (the old DiskCache path deep-copied via pickle, so it was immune). Worth a one-line note here, or copy.deepcopy, so the invariant survives a future edit.
The DeviceSettings singleton is read many times per request (e.g. by the locale middleware), and each read of the sqlite-backed process_cache is a measurable cost. Hold the loaded instance in process memory for a short TTL on top of process_cache; local writes clear the memo immediately and the TTL bounds staleness from writes in other processes. Also collapse the previous `key in cache` plus `cache.get` pair into a single get, halving the diskcache ops on the memo-miss path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
64cb1cc to
2b67d56
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14894 — delta re-review. All 4 prior findings resolved; the memo now deepcopys in and out so each caller's extra_settings is isolated, and the new tests lock in the isolation and save-invalidation behavior. One soft note on the post-save window (inline, no change required). CI pending; no UI files, so Phase 3 N/A.
Prior-finding status
RESOLVED — models.py:112 — memo handed same mutable instance to callers
RESOLVED — models.py — copy() shallow, shared extra_settings dict (now deepcopy)
RESOLVED — test_device_settings.py:57 — save-invalidation test aliased memoized instance
RESOLVED — test_device_settings.py:38 — misleading "(sqlite-backed)" comment
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| ) | ||
|
|
||
| def save(self, *args, **kwargs): | ||
| # Clear up front so a failed full_clean leaves no stale memo behind. |
There was a problem hiding this comment.
suggestion: save() clears the memo here but only re-seats process_cache at the end. In a threaded worker, a concurrent get() landing in that window reads the pre-save value from process_cache and re-memoizes it with a fresh 1s TTL — so an already-committed write can be served stale from this process for up to the TTL, slightly stronger than the cross-process framing in the PR description. Bounded to 1s and always a valid DeviceSettings, so consistent with the accepted staleness tradeoff; no change required if 1s is acceptable (it almost certainly is). Sync workers are unaffected.
There was a problem hiding this comment.
I think if we're not OK with 1s staleness, we shouldn't be doing this, so I think this is fine.
There was a problem hiding this comment.
Agreed — that framing is exactly right. The window I described is just another facet of the same 1s staleness budget the design already accepts, so it stands or falls with that same decision. No change needed.
2 participants
Summary
Memoizes device settings in process memory for 1s.
Nearly every request reads device settings, so even cached it means a disk read each time.
In-process memo means that under heavy load Kolibri does one disk read per second for it, not hundreds.
References
None - testing a range of performance optimizations showed this as a clear and easy winner.
Reviewer guidance
Load testing results (learner focused flow):
Online pod (Postgres, 200 users, 3 min):
developthe pod reliably fails: ~14k requests, p99 69s - cascading login failure results in requests never happening.Invalidation and test isolation:
save()clears the memo up front, so a failedfull_clean()leaves nothing stale.delete()clears it too, on both the model and the queryset.conftest.py's autouse fixture clears the memo alongsideprocess_cache, to reset per test.AI usage
Implemented with Claude Code from an approach I specified; I reviewed the code and tests and measured the results myself.