docs(content-drive): spec for undersized user cache during folder listings (#37186) - #37190
docs(content-drive): spec for undersized user cache during folder listings (#37186)#37190ihoffmann-dot wants to merge 1 commit into
Conversation
|
Claude finished @ihoffmann-dot's task in 51s —— View job Spec Review —
|
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Reviewed against main at 88af0bad55.
Structure is complete and the PR description matches what the spec actually contains — worth saying, since that isn't true of every spec in this series. The two ruled-out hypotheses are real investigative work.
My concern is that the mechanism in the diagnosis doesn't hold, and since the fix follows from the mechanism, the fix changes too.
The stated amplification cannot produce extra DB round trips
each eviction is amplified up to 8x, because the same
modUser/ownergets resolved multiple times per row through different code paths
The redundancy is real — I can see it:
| Site | Call |
|---|---|
DefaultTransformStrategy.java:161 |
loadUserById(contentlet.getModUser()) — audit properties |
:162 |
loadUserById(contentlet.getOwner()) — audit properties |
:404 |
loadUserById(contentlet.getModUser()) — again, version properties |
:417 |
loadUserById(lockedBy) — only when locked |
But resolving the same id twice in one row cannot cost two queries. loadUserById populates the cache on the miss path — UserFactoryImpl.java:117, userCache.add(userId, user), immediately after the query. By the time :404 runs, :161 has already put modUser in the cache. The duplicate costs a cache.get, not a round trip. For the second call to hit the DB the entry would have to be evicted between two lines of the same row's transform.
So "3–4 lookups per row, 2 of them the same user" is right, but it isn't an amplifier, and it isn't 8.
What does produce repeated queries for the same id is concurrency. hydrateContentletsInParallel (BrowserAPIImpl.java:1132-1153) splits rows into chunks — chunkSize = max(1, min(10, total/4)), so 4 parallel chunks for a 40-row page — and runs them on the submitter. When several rows share an author (normal: one editor loaded half the folder), each thread does its own get, misses, and issues its own SELECT before any of them reaches add(). There is no per-key lock and no loading cache. That is a thundering herd on the user id, and it multiplies exactly where the spec attributes per-row amplification.
It also fits the numbers better. Per-row arithmetic gives ~120–160 calls for a 40-row page, not the reported 363–1,025 — a gap the spec doesn't decompose. And it explains the jump to 1–2 after raising the size: once warm there are no misses, so no herd. With a small cache, every navigation to a new folder reopens the window.
This matters because the two mechanisms have different fixes. Capacity is not what is failing inside a single request — a request's working set is ~40 distinct ids, which fits in 1,000 easily.
UserCacheImpl has two regions, and one of them is unreachable
The spec proposes raising cache.userdotcmscache.size without opening UserCacheImpl. There are two regions:
17: private String primaryGroup = "UserDotCMSCache";
18: private String emailGroup = "UserEmailDotCMSCache";and add() writes to both on every miss (:34, :36). Both sizing properties are commented out, on adjacent lines:
438: cache.default.size=1000
518: #cache.userdotcmscache.size=1000
519: #cache.useremaildotcmscache.size=1000
FR-001/FR-002 name only line 518. But line 519 shouldn't simply follow it, because:
UserEmailDotCMSCache is write-only. add() keys it by the raw email address (:36); get() reads it with the prefixed key (:58 sets key = primaryGroup + key, :67 reads emailGroup with that). Those never match. And UserFactoryImpl#loadByUserEmail (:125-146) goes straight to DotConnect without consulting the cache at all. remove() has the same prefix mismatch (:84, :87), so entries only leave via flushGroup.
So every miss on the hot path this item is optimizing pays two cache.puts, one into a region nothing can read, plus a full region of heap holding unreachable User objects. Raising line 519 to 4,000 would 4× the memory of a dead region.
I'd split this out as its own bug rather than attach it here — but the spec should decide explicitly which region(s) FR-001 resizes.
Possible availability bug on orphaned user ids (please verify — I read this, I did not run it)
The four resolution sites are not guarded consistently: :161 and :162 wrap in Try.of(...).getOrNull(); :404 and :417 are bare calls.
com.dotmarketing.business.NoSuchUserException extends DotStateException extends DotRuntimeException — unchecked. hydrateContentletsInParallel catches only DotDataException | DotSecurityException (BrowserAPIImpl.java:1147), so it escapes the CompletableFuture, surfaces at future.get() as ExecutionException (:1159), and becomes DotRuntimeException("Failed to hydrate contentlets in parallel").
If that reads right, a deleted user who still owns content doesn't cost an extra query — it fails the whole listing request. Supporting detail: :405 does null != modUser ? modUser.getFullName() : NOT_APPLICABLE, a null-guard that can never fire, because :404 throws rather than returning null.
This is directly relevant to this spec: Edge Cases and FR-003 require preserving the "N/A"/"unknown" fallback for genuinely missing users — but that fallback only exists at the two guarded sites. And the negative-caching hypothesis was ruled out because "zero nonexistent users were found in the test dataset", so this path was never exercised.
Falsifiable in five minutes: delete a user who has content, list their folder. If it reproduces, it deserves its own issue and is probably more urgent than item 4.
"Ruled out: negative caching" is a property of the dataset, not the code
loadUserById (:104-121) throws NoSuchUserException on an empty result without caching anything (:113-114). Negative caching genuinely is absent. Ruling it out because the reference instance had no orphaned ids is fine as an observation about that instance, but on a long-lived install with deleted users, every row with an orphaned author is a guaranteed round trip forever — immune to cache size, since nothing is ever stored. Suggest downgrading to "not present in the reference dataset; separate exposure where orphaned ids exist".
The residual risk is a cliff, not a slope
1,828 users against a 1,000-entry cache under random access would give roughly 55% hits. Going from ~688 to 1–2 by raising to 4,000 is the signature of LRU thrashing on a cyclic working set: near-0% below capacity, near-100% above. So the Edge Case wording — "reproduce today's problem at a new threshold" — understates it. An instance one user over the new default is back to today's behaviour, not slightly worse. Worth stating plainly in FR-001's rationale, and it argues for an eviction/hit-rate signal an operator can see rather than only a larger constant.
Two gaps in the acceptance surface
- No memory analysis. Legacy Considerations correctly calls this "a shared, low-level cache … wider blast radius than the two listing endpoints", but there is no per-entry footprint, no heap delta, and no success criterion on memory. For a change whose entire content is a number, the number's cost belongs in the spec.
- No test type is named anywhere. SC-001 is measured against "the pre-fix baseline measured for this item" — an average from one instance, not reproducible in CI. Constitution Principle V wants tests written and confirmed failing first, so "what test proves SC-001, and can it run without the benchmark dataset" needs an answer before
/speckit-plan, not in PR 2.
Suggested options
Four distinct failure modes: (1) capacity, (2) concurrent stampede, (3) orphaned ids, (4) the dead region.
| Option | Fixes | Cost | Scope | |
|---|---|---|---|---|
| A | Raise cache.userdotcmscache.size to 4000 |
1 | a number | config |
| B | Warm-up: resolve distinct ids before hydrateContentletsInParallel |
2 | ~15 lines in BrowserAPIImpl |
listing |
| C | Striped lock in loadUserById + negative caching with TTL |
2, 3 | ~20 lines in UserFactoryImpl |
product-wide |
| D | Drop the duplicate loadUserById at DefaultTransformStrategy:404 |
noise, not queries | one line | product-wide |
| E | Fix or remove UserEmailDotCMSCache |
4 | ~10 lines in UserCacheImpl |
product-wide |
B is the fix for this item. It is where the symptom was measured, it is local, and it is independent of cache size — a single request's working set is ~40 ids, which already fits in 1,000. The parallel phase becomes all hits. Open design point: a new getUsersByIds(List<String>) versus N sequential loadUserById calls. The latter needs no new API and already removes the herd.
If you do add the batch method, two things it must get right:
- Chunk the
INclause.VersionableFactoryImpl:463-484is the house pattern —CHUNK_SIZE = 500,subList,DotConnect.createParametersPlaceholder(chunk.size())(DotConnect:1087-1094),chunk.forEach(dc::addParam).PermissionBitFactoryImpl:2745uses 500 too, andPermissionFactory:345documents why. A 40-row page never reaches 500, but a public API will be called with 10,000. - Read the cache first and query only the missing ids. Note the existing batch precedent at
VersionableFactoryImpl:465bypasses the cache entirely — neither reads nor populates. Copied literally,getUsersByIdswould warm nothing.
The batch should be lock-free: it runs once, single-threaded, before the parallel phase, so there is no concurrency to collapse, and taking 40 striped locks would serialize it for nothing.
C is the durable follow-up, in its own issue. The pattern already exists in-house — DotConcurrentFactory.getInstance().getIdentifierStripedLock(), used by PermissionBitFactoryImpl:99 and VersionableFactoryImpl:82, with the canonical double-checked shape at VersionableFactoryImpl:486-511. Where it applies is precisely where warm-up cannot: across concurrent requests, on cold start or after a flushGroup/cluster invalidation, and for the many callers that resolve one user at a time and have no id list to pre-resolve (permissions, workflow, content editor, VTL). Notes if it is pursued:
- Key on the normalized id,
userId.trim().toLowerCase(), matching what:111already passes to SQL — otherwise two spellings take different stripes and don't collapse. There is a live inconsistency to fix along the way:loadUserByIdpasses the rawuserIdtouserCache.get()/add()while normalizing only the SQL param, andChainableCacheAdministratorImpl:269,288lowercases but does not trim. - Namespace the lock key (
"user:" + id):getIdentifierStripedLock()returns one shared instance (DotConcurrentFactory:136) already used for identifiers. Striped.lazyWeakLock(64)by default (StripedLockImpl:22,45). Adding product-wide user resolution to the pool identifiers and permissions already share raises false contention;DotKeyLockManagerBuilder:35-39supports a named manager with its own stripe count.tryLockhas a 3-second timeout and then throwsDotConcurrentException(StripedLockImpl:93-97) rather than falling through to the DB. Enormous headroom for a ~0.007 ms lookup, but it is a behaviour change.- The lock must cover the negative outcome too, or N threads on an orphaned id all miss, all query, and all throw — the herd intact exactly where nothing is ever cached.
dotcms.concurrent.locks.disablealready exists as a global escape hatch.
D can land on its own today, though it may be a reorder rather than a delete — :405 writes modUserName from that lookup.
A demoted from "the fix" to "a sane default." 1,000 entries for the product-wide user cache on an 1,828-user instance is genuinely small, so raising it isn't wrong — but with B or C in place the number stops being the dominant factor, and as hygiene it still needs the memory analysis and a decision on line 519.
E as its own bug, not attached to item 4.
Verified, no action needed
#cache.userdotcmscache.size=1000commented atdotmarketing-config.properties:518,cache.default.size=1000at:438✅loadUserByIdissues exactlyselect * from user_ where userid=?(:110) ✅- Redundant same-user resolution per row is real (
:161/:404) ✅ — the redundancy, not the amplification UserCacheImpl/UserFactoryImpl.loadUserByIdare the right classes to name in Legacy Considerations ✅- All four mandatory sections of the dotCMS override template present, plus Assumptions ✅
- FR-005 (a display-name fix must not blur into access control) is a good instinct — keep it ✅
Adjacent, found while checking the above: loadByUserEmail (UserFactoryImpl.java:144-145) calls dotConnect.loadObjectResults() twice — once in the isSet guard, once in the transformer — so it runs its query twice per call.
Sequencing
#37148 is closed and its guidance was to land item 1 first, then re-evaluate whether items 2–4 are still needed. Item 1's spec (#37230) is still open. This item's own Assumptions concede the single-user win is "a few milliseconds" and that the concurrent case "has not been measured" — so the case for building it now rests on an unmeasured benefit, which is also the case option C would strengthen.
2 participants
Spec-Kit PR 1 of 2. Carries the spec alone. Needs a developer approval (not a merge) before /speckit-plan runs.
Resolves the spec phase of #37186.
Proposed Changes
spec.md— 2 prioritized user stories, 5 functional requirements (1 resolved decision recorded inline, no open clarifications remaining), 3 success criteria, edge cases, and the dotCMS Legacy Considerations section.Summary
A single
POST /api/v1/drive/searchrequest can executeselect * from user_ where userid=?between 363 and 1,025 times. Root cause:cache.userdotcmscache.sizeis commented out, falling back tocache.default.size=1000; a reference instance with 1,828 active users exceeds that, so the cache evicts continuously during listing navigation — and each eviction is amplified up to 8x by redundant same-user resolution within a single row (the samemodUser/ownerresolved through multiple independent code paths).Two hypotheses investigated and ruled out before this spec was written
Confirmed empirically: raising the cache size to 4000 dropped lookups per request from an average of ~688 to 1-2.
Decision recorded (technical call, no product sign-off needed)
Ship a larger static default (~4000, the tested value with margin), not auto-computed sizing. Auto-sizing was rejected as disproportionate engineering effort (startup-time counting, recalculation-on-growth, failure handling) for a problem a documented, generous default already resolves in practice. Accepted residual risk: an instance whose user count eventually exceeds the new default reproduces the problem at a new threshold — same manual-override path as today.
Checklist
Related, out of scope for this issue
Two findings surfaced during investigation, not pursued here:
BrowserAPIImpltransforms all subfolders before paging (wastes owner resolution on discarded subfolders); twofilterCollectioncalls use the non-batch overload where the codebase's own standard prefers batch.Additional Info
Parent epic #36814. Originally investigated as item 4 of #37148 (umbrella investigation issue) — its original diagnosis in that issue (a per-row N+1) was superseded by this corrected root cause.
/speckit-adr-contextconsulted — no ADR identified as binding; this is a cache-sizing change with no schema, index, or REST-contract impact. No new ADR proposed.🤖 Generated with Claude Code
This PR fixes: #37186