docs(content-drive): spec for undersized user cache during folder listings (#37186)

[ihoffmann-dot]

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/search request can execute select * from user_ where userid=? between 363 and 1,025 times. Root cause: cache.userdotcmscache.size is commented out, falling back to cache.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 same modUser/owner resolved through multiple independent code paths).

Two hypotheses investigated and ruled out before this spec was written

• Not a classic per-row N+1 access pattern.
• Not missing negative-caching for nonexistent users — zero were found in the test data.

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

• Tests — land in PR 2, written and confirmed failing first (Constitution Principle V)
• Security Implications Contemplated — this touches user display name resolution only, not access control; permission-filtering behavior must remain unchanged for both an administrator and a permission-restricted user

Related, out of scope for this issue

Two findings surfaced during investigation, not pursued here: BrowserAPIImpl transforms all subfolders before paging (wastes owner resolution on discarded subfolders); two filterCollection calls 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-context consulted — 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

[claude]

Claude finished @ihoffmann-dot's task in 2m 4s —— View job

---

Spec review — revision addressing prior feedback

This is a spec-only PR (spec.md, +280). I re-read the revised spec and verified its code/config claims against origin/main. The revision has adopted essentially all of @fabrizzio-dotCMS's review, and the technical claims now hold up against the code.

Verified against the code

• Config: #cache.userdotcmscache.size=1000 at dotmarketing-config.properties:518, #cache.useremaildotcmscache.size=1000 at :519, cache.default.size=1000 at :438 ✅
• Guarded vs. bare loadUserById: DefaultTransformStrategy.java:161-162 wrap in Try.of(...).getOrNull(); :404 and :417 are bare calls ✅
• Orphan-user availability bug is real: hydrateContentletsInParallel (BrowserAPIImpl.java:1137-1174) runs hydrate→transform inside ~4 parallel chunks (chunkSize = max(1, min(10, total/4)), :1142), and its per-chunk catch is only DotDataException | DotSecurityException (:1152). NoSuchUserException is unchecked, so a bare :404/:417 call on a deleted user escapes, surfaces at future.get() (:1164), and becomes DotRuntimeException("Failed to hydrate contentlets in parallel") (:1170) — failing the whole listing, not one row. FR-004a's "confirmed availability bug" framing is now well-grounded, not speculative ✅
• Thundering-herd mechanism (FR-001): user resolution runs inside the parallel chunks with no per-key lock, so two chunks needing the same uncached id both query — matches the spec's revised root cause ✅

Prior findings — Resolved

• ✅ Mechanism corrected. Root cause rewritten from "8× same-row amplification" to concurrent thundering herd (Scope Boundary, spec.md:29-42); primary fix is now warm-up de-dup (FR-001), capacity demoted to secondary (FR-002).
• ✅ Dead cache region. FR-002 (:181-186) explicitly forbids raising line 519 and explains the UserCacheImpl write-key/read-key mismatch.
• ✅ Orphan-user bug. Now US3 (P2) + FR-004a + Edge Case + SC-002.
• ✅ Negative caching downgraded to a dataset property, not a code claim (:24-27).
• ✅ Residual risk restated as mid-page eviction under concurrent load rather than a gentle slope (:153-159).
• ✅ Memory footprint now has its own criterion (SC-004).
• ✅ Test type named — SC-001 specifies an integration test with a loadUserById DB-call-count assertion (exact location deferred to plan).

New Issues

• 🟡 Medium: spec.md:3 — Feature Branch metadata reads 37148-drive-listing-user-cache, but the actual PR branch is issue-37186-content-drive-user-cache-sizing. Stale header; worth correcting so the spec's own metadata points at the right branch. Fix this →
• 🟡 Medium: spec.md:1 — Title still reads "Bound user-cache eviction cost," which framed the old capacity-first diagnosis. With FR-001 (warm-up de-dup) now the primary fix and eviction demoted to FR-002, the title lags the body. Non-blocking, but a title like "Deduplicate user resolution during folder listings" would match the revised root cause.

Both are cosmetic/doc-consistency only — neither blocks spec approval. No correctness or scoping issues remain in the spec.

One thing to carry into /speckit-plan (not a spec defect)

FR-004a should verify the propagation claim with the Red-phase test before implementation — the "fails the entire request" path only triggers when the orphan id is resolved at the bare sites (:404/:417); the guarded audit sites (:161-162) already swallow it. The failing test must exercise a modUser/locked-by orphan specifically, since that's the only path that reaches the uncaught throw. The spec's Independent Test for US3 already targets this correctly — just flagging that the test must hit the bare sites, not the guarded ones, to actually go Red.

Nice work on the revision — the corrected mechanism and the orphan-user fix are both borne out by the code.
· issue-37186-content-drive-user-cache-sizing

[fabrizzio-dotCMS]

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/owner gets 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 IN clause. VersionableFactoryImpl:463-484 is the house pattern — CHUNK_SIZE = 500, subList, DotConnect.createParametersPlaceholder(chunk.size()) (DotConnect:1087-1094), chunk.forEach(dc::addParam). PermissionBitFactoryImpl:2745 uses 500 too, and PermissionFactory:345 documents 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:465 bypasses the cache entirely — neither reads nor populates. Copied literally, getUsersByIds would 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 :111 already passes to SQL — otherwise two spellings take different stripes and don't collapse. There is a live inconsistency to fix along the way: loadUserById passes the raw userId to userCache.get()/add() while normalizing only the SQL param, and ChainableCacheAdministratorImpl:269,288 lowercases 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-39 supports a named manager with its own stripe count.
• tryLock has a 3-second timeout and then throws DotConcurrentException (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.disable already 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=1000 commented at dotmarketing-config.properties:518, cache.default.size=1000 at :438 ✅
• loadUserById issues exactly select * from user_ where userid=? (:110) ✅
• Redundant same-user resolution per row is real (:161 / :404) ✅ — the redundancy, not the amplification
• UserCacheImpl / UserFactoryImpl.loadUserById are 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.