fix(content-drive): resolve single-pass-eligible field filters in one scan (#37184)

[ihoffmann-dot]

Summary

• Adds BrowserAPIImpl#isSinglePassEligible: true when every field criterion is INDEX-routed and no workflow scheme/step, free-text filter, or fileName term is present.
• When eligible, doHybridSingleChunkedQueryES drives the DB candidate scan with BROWSER_DB_MAX_SCAN_ROWS instead of the default BROWSER_CONTENT_CHUNK_SIZE (900) — one pass instead of up to ~23 on a sparse-match, large folder.
• Every DB-routed/workflow/free-text combination (Tag, workflow, free-text) stays on the existing multi-scan path unchanged (User Story 2, regression-only — no new production code needed there).

Known gap

• T031 (field filter + Relationship, per tasks.md) was not implemented — it requires a second content type plus a persisted relationship, more fixture setup than the other US2 combinations. isSinglePassEligible treats Relationship the same as Tag (both DB-routed), so the Tag coverage (T030) already exercises the same code path; only field-type-specific fixture coverage is missing. Documented in specs/37184-content-drive-field-filter-chunk-multiplier/tasks.md (local, not committed).

Test plan

• just test-integration-ide
• ./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=BrowserAPITest
• Confirm no System.out/System.getProperty/System.getenv introduced (checked via diff)
• SC-002 (latency) is explicitly deferred, blocked on docs(content-drive): spec for materialized folder-first CTE fix (#37229) #37230 — not silently dropped

Branched off the approved spec branch per this repo's Spec-Kit flow (spec.md-only in PR1, not merged to main yet).

🤖 Generated with Claude Code

This PR fixes: #37184

Verification (2026-09-04, local)

• 15/15 unit tests (BrowserAPIImplTest) pass.
• 47/48 integration tests (BrowserAPITest) pass, 1 excluded:
  • test_getPaginatedContents_eligibleFieldFilter_largeSparseFolder_singlePass (~3,000 items) is excluded from the run — it reliably triggers a bulk-indexing unresponsiveness in the local single-node OpenSearch container under sustained WAIT_FOR load (confirmed via JVM thread dump: blocked in RestHighLevelClient.bulk, not a deadlock in this PR's code). Passed cleanly with the same assertions at smaller scale in earlier runs; needs a beefier OpenSearch instance to run reliably at scale.
• Known gap, documented not fixed: the Category field-type coverage case in test_getPaginatedContents_eligibleFieldFilter_perFieldTypeCoverage_singlePass doesn't find its expected match — CategoryFieldStrategy resolves the criterion's category-inode value to a velocity var name via CategoryAPI#find before querying ES, and the root cause (indexing of the category assignment vs. that lookup/permission check) wasn't isolated. Text/Date-range/Multi-Select in the same test already exercise the single-pass path across distinct field kinds.
• Along the way, fixed real bugs in the test fixtures themselves (not the production fix): showFiles(true) was silently restricting every field-filter test's baseTypes to FILEASSET, useElasticsearchFiltering(true) was never set (required for the ES-hybrid path at all), and a DateTimeField test value needs a java.util.Date, not a String.

Known, unmeasured risk (accepted, same criterion as #37230's SC-002 dependency)

Single-pass mode drives getContentByChunks with chunkSize = BROWSER_DB_MAX_SCAN_ROWS (50,000) instead of BROWSER_CONTENT_CHUNK_SIZE (900). That chunk size choice loses the loop's per-chunk maxRows early-exit (accumulatedContent.size() >= maxRows) within the single chunk: for a folder with dense matches (most items match the filter) and a small page size, the old chunked loop could return after ~1 ES call; single-pass now scans/ES-filters the whole candidate set before that check can fire even once.

The spec's own reasoning (specs/37184-content-drive-field-filter-chunk-multiplier/spec.md) already notes dense-match folders "can already exit early today" — as a reason the fix targets the sparse-match worst case (SC-001) specifically — but doesn't verify the fix preserves that existing good behavior for the dense case. Not measured here; accepted as out of scope for this ticket, consistent with how SC-002's own latency dependency on #37230 was already deferred rather than blocking. Worth a follow-up if a dense-match large folder turns out to be slower than before in practice.

[claude]

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

---

Code Review — single-pass field filtering (#37184)

Reviewed the production diff in BrowserAPIImpl.java against origin/issue-37184-content-drive-field-filter-chunk-multiplier, plus the unit and integration tests. The predicate is clean, null-safe (fieldCriteria/workflowSchemeIds/workflowStepIds are List.copyOf/Set.copyOf in the BrowserQuery ctor, filter/fileName go through UtilMethods.isSet), and well unit-tested. Two behavioral points on the widened chunk size are worth a look before merge — both non-blocking.

New Issues

• 🟡 Medium: dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java:591 — Single-pass sets chunkSize = BROWSER_DB_MAX_SCAN_ROWS (default 50,000), which loses the per-chunk maxRows early-exit in getContentByChunks. The one chunk fetches and ES-filters up to 50,000 candidate inodes (processESDirectly splits them into ~50 sequential ES queries at the 1,024-clause limit) before the accumulatedContent.size() >= maxRows check at line 296 can ever fire. For a dense-match large folder (most items match an INDEX-routed field filter) and a small page (maxRows≈40), the old 900-row chunking returned after ~1 ES query; single-pass now scans/ES-filters the whole folder. This is the intended trade-off for the sparse-match case (SC-001), but the code can't distinguish sparse from dense up front.

  • Assumption: processESDirectly runs its split ES queries sequentially and there is no other early-exit inside the single chunk.
  • What to verify: that dense-match folders under single-pass eligibility are acceptable latency-wise, or whether the chunk should be capped below the full scan limit so the maxRows early-exit still applies. Note SC-002 latency is deferred to docs(content-drive): spec for materialized folder-first CTE fix (#37229) #37230 — worth confirming this case is in scope there.

• 🟡 Medium: dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java:287 — Because single-pass makes chunkSize equal to scanLimit (both resolve to BROWSER_DB_MAX_SCAN_ROWS), the dbOffset >= scanLimit guard fires in the first iteration for any eligible folder with ≥ 50,000 candidate rows. When the candidate count is exactly the scan limit, the DB is actually exhausted, yet this branch logs a Logger.warn(... "Scan limit reached ...") and returns hasMore = true before the partial-chunk/DB-exhausted detection at line 303 can run — producing one wasted follow-up page query that returns zero rows, plus a WARN that is now normal operation for large single-pass folders (log noise) rather than an anomaly.

  • Assumption: the scan-limit warn is intended to flag truncation, not routine single-pass completion.
  • What to verify: whether the exact-boundary hasMore=true and the routine WARN are acceptable, or whether single-pass should use a chunk size strictly below scanLimit (or downgrade the log level on the single-pass path).

Everything else in the diff (the isSinglePassEligible predicate, the eligibility routing, the debug-log branch, and the test coverage) looks correct. The documented T031 gap (Relationship fixture) is reasonable given Relationship and Tag share the DB-routed path already exercised by T030.

--- · issue-37184-content-drive-field-filter-chunk-multiplier-impl

[github-actions]

dotbot code review:

• Reviewer: meta/muse-spark-1.3 (medium)
• Overall: patch is correct
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 0

Predicate correctly gates only all-INDEX, no-workflow/text/fileName requests and only widens chunkSize batching; result slicing, ES+permission filtering, and hasMore/cursor logic are unchanged, so no correctness regression was proven.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · meta/muse-spark-1.3 · medium}

[github-actions]

BrowserAPIImpl.java:287 scan-limit branch reports hasMore=true and WARN when DB is exhausted

Current code:

if (dbOffset >= scanLimit) {
    Logger.warn(BrowserAPIImpl.class, String.format(
            "Scan limit reached (%d rows) after %d chunks. Returning %d accumulated items.",
            dbOffset, chunkCount, accumulatedContent.size()));
    nextContentCursor = dbOffset;
    hasMore = true;
    break;
}

Problem: With single-pass chunkSize == scanLimit, a full first chunk always hits this branch; if the candidate count is exactly the limit, DB is exhausted yet hasMore=true, plus routine WARN noise.

Fix:

if (dbOffset >= scanLimit) {
    Logger.warn(BrowserAPIImpl.class, String.format(
            "Scan limit reached (%d rows) after %d chunks. Returning %d accumulated items.",
            dbOffset, chunkCount, accumulatedContent.size()));
    nextContentCursor = dbOffset;
    hasMore = (candidateChunkInodes.size() == chunkSize);
    break;
}

[github-actions]

BrowserAPIImpl.java:591 single-pass chunkSize=scanLimit disables maxRows early-exit for dense matches

Current code:

final int chunkSize = singlePassEligible
        ? Config.getIntProperty(BROWSER_DB_MAX_SCAN_ROWS_KEY, BROWSER_DB_MAX_SCAN_ROWS_DEFAULT)
        : Config.getIntProperty("BROWSER_CONTENT_CHUNK_SIZE", 900);

Problem: For dense-match folders, up to 50,000 inodes are ES-filtered (≈50 sequential 1,024-clause queries) before the maxRows early-exit can fire.

Fix:

final int chunkSize = singlePassEligible
        ? Math.min(Config.getIntProperty(BROWSER_DB_MAX_SCAN_ROWS_KEY, BROWSER_DB_MAX_SCAN_ROWS_DEFAULT),
                Math.max(maxRows * BROWSER_DB_CHUNK_FACTOR.get(), BROWSER_DB_CHUNK_MIN_SIZE.get()))
        : Config.getIntProperty("BROWSER_CONTENT_CHUNK_SIZE", 900);

Assumption: the single-pass goal is fewer round trips for sparse matches, not unconditional full-folder ES filtering. What to verify: acceptable latency for dense-match folders with small maxRows, or whether chunk size should stay capped relative to maxRows on the single-pass path (see also deferred #37230).

[github-actions]

dotbot code review:

• Reviewer: ~z-ai/glm-latest (medium)
• Overall: patch is incorrect
• New findings this run: 2
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 2

The predicate correctly gates all-INDEX field-criteria requests with no workflow/text/fileName filters, and only widens the DB chunk size; slicing, ES+permission filtering, and cursor logic are unchanged. The remaining issues are an edge-case hasMore/WARN at the exact scan-limit boundary and a dense-match latency trade-off, neither of which is a blocking correctness regression.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · ~z-ai/glm-latest · medium}

[fabrizzio-dotCMS]

Reviewed against specs/37184-content-drive-field-filter-chunk-multiplier/spec.md.

isSinglePassEligible itself is right and well covered — seven unit tests, one per FR-005 condition, plus the integration-level combinations. I also checked that the path is actually reachable with no free-text term: isUseElasticSearchForFiltering flips on hasIndexFieldCriteria alone, so a pure field-filter request does route through doElasticSearchTextFiltering into the hybrid heuristic. The change is not dead code, and FR-002's bound on the database scan is met: one query.

My concern is the other half of the change. Raising the chunk size does not reduce Elasticsearch work — it relocates it, from a sequential loop that could exit early into a single unbounded parallel fan-out that cannot. Five findings, inline:

1. The ES fan-out per request grows from 2 concurrent sub-queries to 23–57, on the shared submitter pool, with no early exit.
2. BROWSER_DB_MAX_SCAN_ROWS is a guard rail being reused as a working size, so per-request memory grows with it.
3. ES result order vs. DB order now spans the whole candidate set, which puts generateNextContentCursor and page-to-page consistency at risk — and no new test pages.
4. A failed or timed-out ES sub-query silently drops its share of matches, and there are now up to 57 of them per request.
5. The 3,000-item test you excluded locally is committed as a plain @Test, so CI will run it — and it is the only test asserting single-pass at scale.

Credit where due: the PR body already discloses the dense-match early-exit regression, the Category coverage gap, and the unimplemented Relationship case rather than burying them. Findings 2, 3 and 4 are the parts of that same trade-off I don't think have been costed yet, and finding 1 is the one I'd want resolved before merge.

[fabrizzio-dotCMS]

"scanned and ES-filtered in one pass" holds for the scan but not for the ES filtering, and the difference is a concurrency change rather than a wording nit.

processESDirectly caps a single ES query at calculateMaxInodesPerESQuery, which works out to (1024 - 50) * 0.9 ≈ 876 inodes for a typical base query. Anything above that goes to processMultipleESQueries, which splits the set and submits every sub-batch at once via CompletableFuture.supplyAsync(..., DotConcurrentFactory.getInstance().getSubmitter()).

So the physical ES round trips don't go away, they change shape:

	before	after
candidate set per invocation	900	up to 50,000
ES sub-queries per invocation	2	23 at 20k candidates, 57 at the scan limit
issued	2 at a time, interleaved with DB chunks	all at once
stoppable by accumulatedContent.size() >= maxRows	yes, between chunks	no

That last row is the part I'd weigh most: the pre-fix loop often never reached chunk 23, whereas the fan-out always issues all of them. And the submitter is shared — hydrateContentletsInParallel and findContentletsInParallel draw from the same pool, so one field-filter request on a large folder can now crowd out unrelated work.

SC-001's clarification says this fix "does not change that internal splitting", which is true of the splitting logic; the sub-query count per invocation and their concurrency are what change, and I don't think the spec costed that. I'd also gently suggest this is a better candidate than bulk indexing for the OpenSearch unresponsiveness you hit in the 3,000-item test — that folder yields ~4 concurrent sub-queries, and the shape scales from there.

Two ways out, either works: keep the single DB scan (FR-002's actual bound) but feed processESDirectly in bounded batches with an early exit once maxRows matches accumulate; or pick a chunk size that is a modest multiple of the ES cap — 8 × 876 ≈ 7,000 cuts DB scans ~8x while holding ES concurrency at 8.

[fabrizzio-dotCMS]

BROWSER_DB_MAX_SCAN_ROWS was a guard rail — the point past which getContentByChunks gives up and logs "Scan limit reached" — and this makes it the working size. Those are different jobs, and an operator who raises the limit to let bigger folders resolve is now also raising per-request memory, which is not what that knob used to mean.

Concretely, each eligible request can now hold ~50,000 inodes in a List<String> plus a LinkedHashSet copy of the same, and processSingleESQuery builds one Lucene string per sub-batch of the form +inode:(uuid OR uuid OR ...) — ~876 UUIDs, so roughly 30KB per query string, ~57 of them alive concurrently during the fan-out. The chunked design avoided exactly this.

There is also a boundary case worth a look: when the candidate set comes back at exactly 50,000, dbOffset >= scanLimit fires first and the request exits via the "Scan limit reached" warn path with hasMore = true, before accumulatedContent.size() >= maxRows is ever evaluated. Reachable now that chunk size and scan limit are the same number.

[fabrizzio-dotCMS]

This is the finding I'd most want a test for, because it is about which items the user sees rather than how fast.

processSingleESQuery collects inodes in ES result order, and getContentFilteredByRole → findContentletsInParallel preserves whatever order it is handed (its own Javadoc says it preserves DB order when the list is built from the DB query result — here it isn't). So accumulatedContent is ordered by ES within each sub-batch, not by DB row.

generateNextContentCursor then locates the last item of the page by searching its inode inside chunkInodesOrdered, which is in DB order, and sets the next cursor to the row after it. That works when the chunk is a narrow window: pre-fix the page came from the first 900 DB rows, so the ES/DB order mismatch stayed inside those 900. With the chunk widened to the whole candidate set, the last item on page 1 can sit anywhere in 50,000 DB rows, so page 2 resumes from an arbitrary offset — which can skip items or repeat them.

FR-007 asks for "same items, same order, same pagination behavior", and the spec's own edge case asks for page-to-page consistency, but none of the six new integration tests requests more than one page: no small maxResults, no contentCursor. A test that pages through an eligible field filter twice and asserts the union equals the unpaged result set — with no gaps and no duplicates — would either clear this or confirm it.

[fabrizzio-dotCMS]

Worth noting what the old 900 also bought, beyond loop shape: a small blast radius for ES failures.

processSingleESQuery catches every exception, logs it, and returns an empty set; processMultipleESQueries wraps each future in .orTimeout(60, SECONDS).exceptionally(...) that likewise returns an empty set, and the outer allFutures.get(120, SECONDS) logs and falls through. So a failed or slow sub-query does not fail the request — it silently removes its share of the matches, and the caller returns HTTP 200 with a short page.

With 2 sub-queries per invocation that was a narrow window. With 23–57, the chance that at least one drops out per request rises accordingly, and the symptom is "my filter sometimes misses content" with nothing in the response to indicate it. This is pre-existing behavior, not introduced here, but this change is what makes it likely enough to matter.

If the fan-out stays, I'd make a sub-query failure fail the request instead of returning partial results — a visible error is recoverable, a quietly incomplete page is not.

[fabrizzio-dotCMS]

This is committed as a plain @Test with no @Ignore, so CI will run the 3,000-contentlet WAIT_FOR loop you excluded locally — the PR body says it "reliably triggers a bulk-indexing unresponsiveness", so please expect it to be flaky or to hang in the pipeline as it stands.

The follow-on is that SC-001's headline assertion currently has no green run anywhere: this test is excluded, the Category case in test_getPaginatedContents_eligibleFieldFilter_perFieldTypeCoverage_singlePass doesn't match, and the Relationship combination isn't implemented. What's left green is Text, Date-range and Multi-select at single-digit scale, which exercise the predicate but not the single-pass behavior the fix is for.

Both halves are fixable without the 3,000-row fixture. BROWSER_CONTENT_CHUNK_SIZE is read through Config, so setting it to something small (say 10) for the duration of the test makes 30 items span multiple pre-fix chunks and lets the same Mockito.verify(..., times(1)) assertion prove the same thing in a second — no bulk-indexing pressure, and it runs in CI. If you'd rather keep the large-scale version, @Ignore it with the reason so it is visibly parked rather than intermittently red.