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 4 commits into
Conversation
|
Claude finished @ihoffmann-dot's task in 1m 17s —— View job Review — spec update (
|
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.
|
@fabrizzio-dotCMS good catch on the mechanism. Rewrote the root cause: it's concurrent thundering herd in |
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Round 2 — re-review of cfc81566
Re-reviewed against main @ 340c703feb.
This is the strongest of the four round-2 responses. The root cause was genuinely rewritten rather than patched — all six of my items are answered: the thundering-herd mechanism replaced the "8x amplification" story, US2 dropped to P3, the cache bump went from primary fix to secondary hygiene, the dead region is handled, B4 became US3 + FR-004a, "ruled out" was softened, memory became SC-004, and a test got named.
What's left is one scope gap the rewrite created, one feasibility wrinkle, and a test with no infrastructure to run on.
Re-verified on current main
Every prior code claim still reproduces (BrowserAPIImpl moved ~+373 lines since the pin):
hydrateContentletsInParallelat:1137-1174;chunkSize = Math.max(1, Math.min(10, total/4))at:1142✅- It catches only
DotDataException | DotSecurityException(:1152), so an uncheckedNoSuchUserExceptionescapes, resurfaces atfuture.get(30s)(:1164), and becomesDotRuntimeException("Failed to hydrate contentlets in parallel")(:1170) ✅ DefaultTransformStrategy:161/:162guarded withTry.of(...).getOrNull();:404and:417bare ✅ — and:405'snull != modUser ? … : NOT_APPLICABLEstill can't fire- New check:
:404sits inaddVersionProperties, gated byoptions.contains(VERSION_INFO)— andVERSION_INFOis indefaultOptions, whichdotContentMapuses. So the unguarded call runs on every Content Drive listing row. FR-004a is squarely on the hot path. ✅
🔴 The rewrite fixed the intra-request herd and dropped the multi-user case
FR-001's warm-up resolves each distinct id once within one request. Two users opening the same cold folder at the same moment still both run warm-up, both miss, and both query — the same herd, one level up.
The spec used to acknowledge this. Round 1's Assumptions said the item's value "is in avoiding proportionally larger cost under concurrent multi-user load, which has not been measured." Round 2 replaced that sentence with "this item's win scales with the number of distinct authors/owners in a folder and with concurrent chunk contention."
So the stated value proposition moved from multi-user to intra-request chunk contention, and FR-001 only addresses the second. I searched the current spec for "concurrent request", "across request", "multi-user", "striped", "cluster", "flushGroup", "cold start", "follow-up": zero hits.
My prior review recommended warm-up as this item's fix and a striped lock as a durable follow-up in its own issue, precisely because warm-up can't cover: concurrent requests, cold start or post-flushGroup/cluster invalidation, and the callers that resolve one user at a time with no id list (permissions, workflow, content editor, VTL). The warm-up was taken; the follow-up vanished without a mention.
Choosing warm-up alone is a legitimate scope call — but the spec has to say it. As written, a reader concludes the herd is solved. One sentence in Assumptions plus a named follow-up issue closes this; without the issue, it repeats exactly what happened to #37148's correctness findings.
🟠 FR-001 can't warm locked-by as cheaply as it implies
FR-001 requires collecting "the distinct set of modUser/owner (and, where resolved, locked-by) user ids the page needs."
modUser and owner are fields on Contentlets already materialized before hydrateContentletsInParallel (called with fromDB.contentlets, :1761) — collecting them is free. lockedBy is not. DefaultTransformStrategy:414 obtains it via toolBox.versionableAPI.getLockedBy(contentlet), a per-contentlet call. Warming it means running N getLockedBy calls in the new sequential phase — new serial work on the critical path, which is what the parallel phase existed to avoid.
Either drop locked-by from the warm-up set (it only applies to locked content, so the herd exposure is small) or account for the cost. As written it reads as free.
🟠 SC-001's verification method has no existing infrastructure
SC-001 asks for "an integration test asserting the DB call count for loadUserById, e.g. a DotConnect/query-count assertion in BrowserAPITest". I searched dotcms-integration/src/test for a query-counting precedent and found nothing usable — no harness that counts DotConnect executions per request.
Under Principle V the Red test must exist and fail before implementation, so building that harness is itself a task in PR 2, not a parenthetical. Either name the mechanism (a DotConnect counter, a JDBC proxy, a UserFactory spy) or state that SC-001 is validated by measurement rather than by an automated test.
🟡 FR-002 answered half of the dead-region point
Correctly says not to raise line 519 because the region is unreadable — that's the important half. But UserCacheImpl.add() still writes to it on every miss (:36, keyed by raw email, while get() at :67 uses the prefixed key). So every warm-up resolution pays a wasted cache.put into a dead region — N of them per cold page, right on the path FR-001 exists to optimize. Worth a line, since the spec now names that path as the fix.
🟡 "Confirmed" availability bug is confirmed by reading, not by running
US3 and FR-004a call the orphan-user failure a "confirmed availability bug". I re-verified the whole chain above and the inference is solid — but nothing in the spec or the reply says it was reproduced. US3's own Independent Test is the five-minute repro (delete a user with content, list their folder). Worth running before PR 2, and worth marking as inferred until then, since US3's acceptance scenarios are written as though the behaviour is observed.
Verified — no action needed
- Root cause rewrite is accurate and the numbers hang together: per-row arithmetic gives ~120–160 for 40 rows against the observed 363–1,025, and the spec now says so ✅
- US2 → P3 with the right reasoning (a same-row repeat is a cache hit, not a round trip) ✅
- SC-004 (memory footprint) added ✅
- Negative caching downgraded from "ruled out" to "not present in this dataset" ✅
- Sequencing note on #37230 added to Legacy Considerations ✅
- FR-001's blast-radius claim is fair: the warm-up is localized to
BrowserAPIImpland changes noUserCacheImpl/UserFactoryImplsemantics, and the spec correctly separates that from FR-002 and FR-004a, which do touch shared code ✅
Gate status
All six prior items answered, and the one blocker left is a say-it fix rather than a redesign — one sentence plus a follow-up issue for the cross-request herd. The other two are honest sizing corrections the plan phase can absorb once they're written down.
Of the four specs in this set, this is the one I'd approve first, and the only one where the next push could plausibly be the approvable one.
Comment rather than a change request, same as the others.
|
@ihoffmann-dot Approved — the spec phase is cleared. Nice rewrite: the root cause is now correct rather than patched, and every item from the first round is answered. I'm approving because everything still open is write it down, not figure it out — the mechanism is diagnosed and verified against the code. Two things I'd like carried into the plan phase rather than another review round: 1. Open the follow-up issue for the cross-request herd before I'm asking for the issue specifically because this recommendation already vanished once: it was in my first review alongside the warm-up, and only the warm-up survived into 2. Name SC-001's verification mechanism before Non-blocking, from the review above: @ihoffmann-dot could you take these two on before moving to |
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