From 76d8bdfe67fbf244970568053f259f169ff5b2d9 Mon Sep 17 00:00:00 2001 From: ihoffmann-dot Date: Tue, 15 Sep 2026 22:43:02 -0300 Subject: [PATCH 1/3] chore(content-drive): start implementation branch for issue #37211 Spec approved and merged in #37525. Planning (/speckit-plan, /speckit-tasks) starts here. From ff86bd17693e0ea2bf169ccfdf6984f3d948717e Mon Sep 17 00:00:00 2001 From: ihoffmann-dot Date: Wed, 16 Sep 2026 20:23:13 -0300 Subject: [PATCH 2/3] fix(content-drive): bound the ES-narrowed scan by time, not row count (#37211) getContentByChunks dropped matches silently when no content-type filter narrowed the candidate set: the row-count cutoff (BROWSER_DB_MAX_SCAN_ROWS) could fire before a match further down DB order was ever sent to ES for text narrowing. Bound the ES-filtered path by elapsed time (BROWSER_DB_MAX_SCAN_TIME_MILLIS) instead, so it keeps scanning while still affordable; the permission-only path keeps its original row-count cutoff. Also resolves spec.md's NEEDS CLARIFICATION on the Search All comparison path (ESContentResourcePortlet). --- .../com/dotcms/browser/BrowserAPIImpl.java | 48 +++- .../com/dotcms/browser/BrowserAPITest.java | 252 ++++++++++++++++++ specs/37418-content-drive-global/spec.md | 17 +- 3 files changed, 306 insertions(+), 11 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java b/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java index b0e900cde2df..f745364b8c9f 100644 --- a/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java @@ -247,6 +247,15 @@ ContentUnderParent getContentUnderParentFromDB(final BrowserQuery browserQuery, * Pagination resumes from {@link BrowserQuery#contentCursor}, which is the DB row offset * returned by the previous page. On the first page it is 0. *

+ *

+ * The scan's cost cap depends on {@code applyESFilter}: when {@code true} (text filtering + * through Elasticsearch), it is bounded by elapsed time + * ({@code BROWSER_DB_MAX_SCAN_TIME_MILLIS}), so an unfiltered global search keeps scanning + * while still affordable instead of giving up at an arbitrary row count and silently + * dropping matches that sit later in DB order (see issue #37211). When {@code false} + * (permission-only filtering), it stays bounded by row count + * ({@code BROWSER_DB_MAX_SCAN_ROWS}), unchanged from before. + *

* * @param browserQuery query containing search criteria, user context, and the current cursor * @param maxRows maximum number of permission-visible items to return @@ -263,7 +272,15 @@ private ContentUnderParent getContentByChunks(final BrowserQuery browserQuery, final int maxRows, final SelectQuery sqlQuery, final int chunkSize, final boolean applyESFilter) throws DotDataException, DotSecurityException { - final int scanLimit = Config.getIntProperty(BROWSER_DB_MAX_SCAN_ROWS_KEY, BROWSER_DB_MAX_SCAN_ROWS_DEFAULT); + final int scanRowLimit = Config.getIntProperty(BROWSER_DB_MAX_SCAN_ROWS_KEY, BROWSER_DB_MAX_SCAN_ROWS_DEFAULT); + // The ES-narrowed scan (text filter) bounds cost by elapsed time instead of row count -- + // see BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY. The permission-only scan keeps the original + // row-count cutoff; it has no completeness gap to fix (every candidate row already + // matches the query's own SQL criteria). + final long scanTimeBudgetMillis = applyESFilter + ? Config.getLongProperty(BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY, BROWSER_DB_MAX_SCAN_TIME_MILLIS_DEFAULT) + : -1L; + final long scanStartNanos = System.nanoTime(); final List accumulatedContent = new ArrayList<>(); List candidateChunkInodes; @@ -296,10 +313,20 @@ private ContentUnderParent getContentByChunks(final BrowserQuery browserQuery, dbOffset += candidateChunkInodes.size(); - if (dbOffset >= scanLimit) { + // Row-count-only cutoff dropped ES-narrowed matches that sit past it in DB order + // without ever sending them to ES (#37211) -- an unfiltered global search's SQL + // candidate set is essentially the whole site, so DB position says nothing about + // whether a match exists. Bound that scan by elapsed time instead, so it keeps + // going while still affordable rather than giving up at an arbitrary row count. + final boolean scanBudgetExhausted = applyESFilter + ? TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - scanStartNanos) >= scanTimeBudgetMillis + : dbOffset >= scanRowLimit; + + if (scanBudgetExhausted) { Logger.warn(BrowserAPIImpl.class, String.format( - "Scan limit reached (%d rows) after %d chunks. Returning %d accumulated items.", - dbOffset, chunkCount, accumulatedContent.size())); + "Scan budget reached (%s) after %d chunks, %d rows scanned. Returning %d accumulated items.", + applyESFilter ? scanTimeBudgetMillis + "ms" : scanRowLimit + " rows", + chunkCount, dbOffset, accumulatedContent.size())); nextContentCursor = dbOffset; hasMore = true; break; @@ -748,9 +775,22 @@ private String buildPureESQuery(final BrowserQuery browserQuery) { // Maximum total DB rows to scan per request across all chunks. Acts as a safety cap to prevent // runaway queries when a restricted user has access to a small fraction of site content. // Default of 50,000 covers a worst-case ~5% permission pass rate for a full page of 300 items. + // Used as-is (row-count cutoff) for the permission-only scan (applyESFilter=false); see + // BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY for the ES-narrowed (text-filter) scan's cost bound. static final String BROWSER_DB_MAX_SCAN_ROWS_KEY = "BROWSER_DB_MAX_SCAN_ROWS"; static final int BROWSER_DB_MAX_SCAN_ROWS_DEFAULT = 50_000; + // Maximum wall-clock time to spend scanning DB chunks when text-filtering through ES + // (applyESFilter=true). A row-count cutoff here silently drops matches that fall later in + // DB order than the cutoff, even though they were never actually sent to ES for narrowing + // (issue #37211) -- a global search with no content-type filter has a broad, effectively + // unbounded-by-type candidate set, so match position in DB order says nothing about whether + // the match exists. Bounding by elapsed time instead lets the scan keep going as long as it + // is still affordable, rather than giving up at an arbitrary row count regardless of + // coverage. + static final String BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY = "BROWSER_DB_MAX_SCAN_TIME_MILLIS"; + static final long BROWSER_DB_MAX_SCAN_TIME_MILLIS_DEFAULT = 10_000L; + /** * Represents content items under a specific parent along with the total count. * This class is immutable and holds a list of content items and their total results count. diff --git a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java index f543fa6e8b50..f885dfca1760 100644 --- a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java @@ -2228,6 +2228,258 @@ public void test_getPaginatedContents_scanLimitStopsLoop() throws Exception { } } + /** + *
    + *
  • Method to Test: {@link BrowserAPI#getPaginatedContents(BrowserQuery)}
    • + *
  • Given Scenario: A folder holds enough content to exceed + * {@code BROWSER_DB_MAX_SCAN_ROWS} (intentionally lowered for the test, along with + * {@code BROWSER_CONTENT_CHUNK_SIZE} so the fixture actually spans multiple chunks -- + * otherwise a small fixture fits in the default 900-row chunk and the whole thing gets + * ES-filtered before the scan-limit check ever runs), with no content-type filter + * applied. The one item whose title matches the free-text filter is the newest of the + * batch, so under the default ascending {@code mod_date} scan order it is scanned + * last -- in a chunk past the lowered scan limit.
    • + *
  • Expected Result: The unfiltered global search must still return the + * matching item. Before the fix for + * #37211, the row-count-only + * scan cutoff drops it silently.
    • + *
+ */ + @Test + public void test_getPaginatedContents_unfilteredTextSearch_findsMatchPastScanLimit() throws Exception { + final int chunkSize = 5; + final int scanLimit = 15; + final int fillerCount = 20; + + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_KEY, scanLimit); + Config.setProperty("BROWSER_CONTENT_CHUNK_SIZE", chunkSize); + try { + final Host host = new SiteDataGen().nextPersisted(); + final Folder folder = new FolderDataGen().site(host).nextPersisted(); + final var contentType = new ContentTypeDataGen() + .host(host) + .folder(folder) + .field(new FieldDataGen().name("title").velocityVarName("title").next()) + .nextPersisted(); + + // Filler items created first -- oldest mod_date, scanned first under the default + // ascending order, all safely inside the scan limit. + for (int i = 0; i < fillerCount; i++) { + new ContentletDataGen(contentType) + .setProperty("title", "Filler " + i) + .host(host) + .folder(folder) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + } + + // The matching item is created LAST -- newest mod_date, scanned last, past the + // lowered scan limit -- reproducing "a just-uploaded item is missing from search". + final Contentlet capybara = new ContentletDataGen(contentType) + .setProperty("title", "Capybara Image") + .host(host) + .folder(folder) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + + final BrowserQuery query = BrowserQuery.builder() + .withHostOrFolderId(folder.getIdentifier()) + .withFilter("Capybara") + .useElasticsearchFiltering(true) // Content Drive always sets this (ContentDriveHelper) -- + // the bug only reproduces on the ES-routed path, never + // the plain DB ILIKE fallback used when this is false. + .showContent(true) + .showFiles(false) + .showFolders(false) + .showLinks(false) + .showDotAssets(false) + .showWorking(true) + .showArchived(false) + .maxResults(100) + .contentCursor(0) + .build(); + + final PaginatedContents result = browserAPI.getPaginatedContents(query); + + assertNotNull("Result must not be null", result); + final Set foundInodes = result.list.stream() + .map(item -> (String) item.get("inode")) + .collect(Collectors.toSet()); + assertTrue("Unfiltered global search must find a match that exists past the scan " + + "limit (issue #37211) -- found: " + foundInodes, + foundInodes.contains(capybara.getInode())); + } finally { + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_KEY, + BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_DEFAULT); + Config.setProperty("BROWSER_CONTENT_CHUNK_SIZE", 900); + } + } + + /** + *
    + *
  • Method to Test: {@link BrowserAPI#getPaginatedContents(BrowserQuery)}
    • + *
  • Given Scenario: Two content types each hold one item matching the same + * free-text filter. An unfiltered global search is compared against the same search + * narrowed to a single content type.
    • + *
  • Expected Result: The content-type-filtered result is a subset of the + * unfiltered result -- it drops the other type's match but introduces nothing the + * unfiltered search didn't already find (AC-002 of + * #37211).
    • + *
+ */ + @Test + public void test_getPaginatedContents_contentTypeFilter_onlyNarrowsUnfilteredMatches() throws Exception { + final Host host = new SiteDataGen().nextPersisted(); + final Folder folder = new FolderDataGen().site(host).nextPersisted(); + + final var typeA = new ContentTypeDataGen() + .host(host) + .folder(folder) + .field(new FieldDataGen().name("title").velocityVarName("title").next()) + .nextPersisted(); + final var typeB = new ContentTypeDataGen() + .host(host) + .folder(folder) + .field(new FieldDataGen().name("title").velocityVarName("title").next()) + .nextPersisted(); + + final Contentlet matchA = new ContentletDataGen(typeA) + .setProperty("title", "Capybara from Type A") + .host(host) + .folder(folder) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + final Contentlet matchB = new ContentletDataGen(typeB) + .setProperty("title", "Capybara from Type B") + .host(host) + .folder(folder) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + + final BrowserQuery unfilteredQuery = BrowserQuery.builder() + .withHostOrFolderId(folder.getIdentifier()) + .withFilter("Capybara") + .useElasticsearchFiltering(true) + .showContent(true) + .showFiles(false) + .showFolders(false) + .showLinks(false) + .showDotAssets(false) + .showWorking(true) + .showArchived(false) + .maxResults(100) + .contentCursor(0) + .build(); + + final PaginatedContents unfiltered = browserAPI.getPaginatedContents(unfilteredQuery); + final Set unfilteredInodes = unfiltered.list.stream() + .map(item -> (String) item.get("inode")) + .collect(Collectors.toSet()); + + assertTrue("Unfiltered search must find the Type A match", unfilteredInodes.contains(matchA.getInode())); + assertTrue("Unfiltered search must find the Type B match", unfilteredInodes.contains(matchB.getInode())); + + final BrowserQuery typeAFilteredQuery = BrowserQuery.builder() + .withHostOrFolderId(folder.getIdentifier()) + .withFilter("Capybara") + .withContentTypes(Set.of(typeA.id())) + .useElasticsearchFiltering(true) + .showContent(true) + .showFiles(false) + .showFolders(false) + .showLinks(false) + .showDotAssets(false) + .showWorking(true) + .showArchived(false) + .maxResults(100) + .contentCursor(0) + .build(); + + final PaginatedContents typeAFiltered = browserAPI.getPaginatedContents(typeAFilteredQuery); + final Set filteredInodes = typeAFiltered.list.stream() + .map(item -> (String) item.get("inode")) + .collect(Collectors.toSet()); + + assertTrue("Type-A-filtered search must still find the Type A match", + filteredInodes.contains(matchA.getInode())); + assertFalse("Type-A-filtered search must drop the Type B match", + filteredInodes.contains(matchB.getInode())); + assertTrue("Filtered result must be a subset of the unfiltered result -- it must not " + + "introduce items the unfiltered search didn't find", + unfilteredInodes.containsAll(filteredInodes)); + } + + /** + *
    + *
  • Method to Test: {@link BrowserAPI#getPaginatedContents(BrowserQuery)}
    • + *
  • Given Scenario: A folder holds more content than the (lowered) scan limit, + * none of it matching the free-text filter at all.
    • + *
  • Expected Result: The search still completes within a bounded time and + * returns no matches -- fixing the silent-drop defect + * (#37211) must not regress + * into an unconditional full-table scan (see PR #37395 and siblings for the original + * scan-cost concern the row cap was introduced to address).
    • + *
+ */ + @Test + public void test_getPaginatedContents_unfilteredTextSearch_noMatchStaysBounded() throws Exception { + final int chunkSize = 5; + final int scanLimit = 15; + final int fillerCount = 20; + final long boundedMillis = 30_000L; + + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_KEY, scanLimit); + Config.setProperty("BROWSER_CONTENT_CHUNK_SIZE", chunkSize); + try { + final Host host = new SiteDataGen().nextPersisted(); + final Folder folder = new FolderDataGen().site(host).nextPersisted(); + final var contentType = new ContentTypeDataGen() + .host(host) + .folder(folder) + .field(new FieldDataGen().name("title").velocityVarName("title").next()) + .nextPersisted(); + + for (int i = 0; i < fillerCount; i++) { + new ContentletDataGen(contentType) + .setProperty("title", "Filler " + i) + .host(host) + .folder(folder) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + } + + final BrowserQuery query = BrowserQuery.builder() + .withHostOrFolderId(folder.getIdentifier()) + .withFilter("NoSuchTermAnywhere") + .useElasticsearchFiltering(true) + .showContent(true) + .showFiles(false) + .showFolders(false) + .showLinks(false) + .showDotAssets(false) + .showWorking(true) + .showArchived(false) + .maxResults(100) + .contentCursor(0) + .build(); + + final long start = System.currentTimeMillis(); + final PaginatedContents result = browserAPI.getPaginatedContents(query); + final long elapsed = System.currentTimeMillis() - start; + + assertNotNull("Result must not be null", result); + assertEquals("No item matches the filter term", 0, result.contentCount); + assertTrue("A non-matching scan must still complete within a bounded time (" + + elapsed + "ms) -- fixing #37211 must not reintroduce an " + + "unconditional full-table scan", + elapsed < boundedMillis); + } finally { + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_KEY, + BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_DEFAULT); + Config.setProperty("BROWSER_CONTENT_CHUNK_SIZE", 900); + } + } + /** *
    *
  • Method to test: {@link BrowserAPI#getFolderContentList(BrowserQuery)}
    • diff --git a/specs/37418-content-drive-global/spec.md b/specs/37418-content-drive-global/spec.md index 1aa749f40158..a3852490ed68 100644 --- a/specs/37418-content-drive-global/spec.md +++ b/specs/37418-content-drive-global/spec.md @@ -127,13 +127,16 @@ Two adjacent theories were checked and **refuted**, ruling out alternative root ever reaches ES. The legacy Search All portlet does not exhibit this bug because it does not go through this -DB-prescan/chunk mechanism — it queries ES directly. (Architecturally confirmed; the exact -backing class/line was not pinned down in this pass — candidates are -`ESSearchAPIImpl`/`ESContentResourcePortlet` under `dotCMS/src/main/java/com/dotcms/**` — -and should be confirmed during planning.) - -[NEEDS CLARIFICATION: Confirm the exact Search All REST resource/service class and line -reference, to formally document the working comparison path in the plan.] +DB-prescan/chunk mechanism — it queries ES directly. **Confirmed during planning**: the search +handler is `com.dotcms.rest.elasticsearch.ESContentResourcePortlet` +(`dotCMS/src/main/java/com/dotcms/rest/elasticsearch/ESContentResourcePortlet.java`) — +`search`/`searchPost` build a raw ES/OS Lucene query and execute it via +`ContentletAPI.search(...)` (`esapi.search(...)`, lines 133-135 and its `searchPost` +counterpart), with `esapi = APILocator.getContentletAPI()` (line 60); `searchRaw` +(line 293) does the same via `esapi.searchRaw(...)`. None of these paths build DB-order +paged candidate chunks first — the query goes straight to ES/OS, which matches and ranks +over the whole index in one shot, so a match is never dropped by a DB-page cutoff before ES +gets to see it. ## Fix Scope & Non-Goals *(mandatory)* From 1ba054eec788bc76554122cf017a17d1e7afbc8b Mon Sep 17 00:00:00 2001 From: ihoffmann-dot Date: Wed, 16 Sep 2026 21:50:29 -0300 Subject: [PATCH 3/3] fix(content-drive): add row hard cap and cutoff test coverage per review (#37211) Addresses PR bot review feedback on the ES-narrowed scan time-budget fix: - Add BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP as a co-bound alongside the time budget, so a fast DB/ES pair (or many concurrent unfiltered searches) can't turn the time budget into unbounded per-request work. - Add an integration test that forces the time-cutoff branch itself (not just DB exhaustion) and confirms the returned cursor resumes correctly. - Add an integration test that forces the new row hard cap. - Document that completeness here is best-effort (load-dependent), and that BROWSER_DB_MAX_SCAN_ROWS still clamps the ES path's chunk size even though it no longer bounds the ES scan's total rows. Co-Authored-By: Claude Sonnet 5 --- .../com/dotcms/browser/BrowserAPIImpl.java | 67 ++++++- .../com/dotcms/browser/BrowserAPITest.java | 186 ++++++++++++++++++ 2 files changed, 244 insertions(+), 9 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java b/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java index 05fe0cc20fbe..7d63136e47ca 100644 --- a/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java @@ -258,9 +258,21 @@ ContentUnderParent getContentUnderParentFromDB(final BrowserQuery browserQuery, * through Elasticsearch), it is bounded by elapsed time * ({@code BROWSER_DB_MAX_SCAN_TIME_MILLIS}), so an unfiltered global search keeps scanning * while still affordable instead of giving up at an arbitrary row count and silently - * dropping matches that sit later in DB order (see issue #37211). When {@code false} - * (permission-only filtering), it stays bounded by row count - * ({@code BROWSER_DB_MAX_SCAN_ROWS}), unchanged from before. + * dropping matches that sit later in DB order (see issue #37211), plus a much higher hard + * row ceiling ({@code BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP}) as a co-bound so a fast DB/ES + * pair -- or many concurrent unfiltered searches -- cannot turn the time budget into + * unbounded per-request work (found in review). When {@code false} (permission-only + * filtering), it stays bounded by row count ({@code BROWSER_DB_MAX_SCAN_ROWS}), unchanged + * from before. + *

    + *

    + * Completeness here is best-effort, not a hard guarantee. Because the ES-narrowed + * cutoff depends on wall-clock time, a slower or more heavily loaded node can exhaust the + * budget before reaching a match that an idle node would find in the same request, so two + * otherwise-identical requests against the same data can return different result sets + * depending on load (found in review). This trades the old cutoff's determinism for + * completeness under normal conditions; it does not eliminate the possibility of a dropped + * match under sustained load, only make it far less likely and no longer position-dependent. *

    * * @param browserQuery query containing search criteria, user context, and the current cursor @@ -284,6 +296,14 @@ private ContentUnderParent getContentByChunks(final BrowserQuery browserQuery, // scan limit below the caller's chunk size (e.g. below BROWSER_SINGLE_PASS_CHUNK_SIZE's // 7,000 default) -- the dbOffset >= scanRowLimit check only runs after a chunk is fetched, so // nothing upstream of it would have caught that (found in review, issue #37184). + // NOTE: on the ES-narrowed path (applyESFilter=true), BROWSER_DB_MAX_SCAN_ROWS no longer + // bounds the *total* rows the scan may read -- BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP does + // that instead (see below). This clamp still shapes the ES path's working chunk size for + // the same reason it does on the permission-only path (bounding the SQL page size), so it + // intentionally stays in effect for both; lowering BROWSER_DB_MAX_SCAN_ROWS therefore + // still shrinks ES chunk size (more round trips) without limiting the ES scan itself + // (found in review, issue #37211) -- surprising if read as "the" scan limit, so calling + // it out explicitly here. final int effectiveChunkSize = Math.min(chunkSize, scanRowLimit); // The ES-narrowed scan (text filter) bounds cost by elapsed time instead of row count -- // see BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY. The permission-only scan keeps the original @@ -292,6 +312,15 @@ private ContentUnderParent getContentByChunks(final BrowserQuery browserQuery, final long scanTimeBudgetMillis = applyESFilter ? Config.getLongProperty(BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY, BROWSER_DB_MAX_SCAN_TIME_MILLIS_DEFAULT) : -1L; + // Co-bound alongside the time budget: without a row ceiling, a fast DB/ES pair (or many + // concurrent unfiltered searches sharing the DotSubmitter pool) could scan far more rows + // in scanTimeBudgetMillis than the old row-count cutoff ever allowed, multiplying real + // cost under load (found in review, issue #37211). Set high enough to comfortably cover + // the real-world scale this fix targets (~718,174 contentlets, per the issue) so it does + // not reintroduce the original completeness bug at that scale. + final int esRowHardCap = applyESFilter + ? Config.getIntProperty(BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP_KEY, BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP_DEFAULT) + : -1; final long scanStartNanos = System.nanoTime(); final List accumulatedContent = new ArrayList<>(); @@ -356,18 +385,26 @@ private ContentUnderParent getContentByChunks(final BrowserQuery browserQuery, // without ever sending them to ES (#37211) -- an unfiltered global search's SQL // candidate set is essentially the whole site, so DB position says nothing about // whether a match exists. Bound that scan by elapsed time instead, so it keeps - // going while still affordable rather than giving up at an arbitrary row count. The - // permission-only scan keeps the original row-count cutoff -- it has no completeness - // gap to fix (every candidate row already matches the query's own SQL criteria). + // going while still affordable rather than giving up at an arbitrary row count -- + // plus a much higher row hard cap as a co-bound, so a fast DB/ES pair (or many + // concurrent unfiltered searches) cannot turn the time budget into unbounded + // per-request work (found in review). The permission-only scan keeps the original + // row-count cutoff -- it has no completeness gap to fix (every candidate row + // already matches the query's own SQL criteria). + final boolean esTimeBudgetExhausted = applyESFilter + && TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - scanStartNanos) >= scanTimeBudgetMillis; + final boolean esRowHardCapExceeded = applyESFilter && dbOffset >= esRowHardCap; final boolean scanBudgetExhausted = applyESFilter - ? TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - scanStartNanos) >= scanTimeBudgetMillis + ? (esTimeBudgetExhausted || esRowHardCapExceeded) : dbOffset >= scanRowLimit; if (scanBudgetExhausted) { + final String exhaustedBoundDescription = !applyESFilter + ? scanRowLimit + " rows" + : (esRowHardCapExceeded ? esRowHardCap + " rows (hard cap)" : scanTimeBudgetMillis + "ms"); Logger.warn(BrowserAPIImpl.class, String.format( "Scan budget reached (%s) after %d chunks, %d rows scanned. Returning %d accumulated items.", - applyESFilter ? scanTimeBudgetMillis + "ms" : scanRowLimit + " rows", - chunkCount, dbOffset, accumulatedContent.size())); + exhaustedBoundDescription, chunkCount, dbOffset, accumulatedContent.size())); nextContentCursor = dbOffset; hasMore = true; break; @@ -878,6 +915,18 @@ private String buildPureESQuery(final BrowserQuery browserQuery) { static final String BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY = "BROWSER_DB_MAX_SCAN_TIME_MILLIS"; static final long BROWSER_DB_MAX_SCAN_TIME_MILLIS_DEFAULT = 10_000L; + // Co-bound alongside BROWSER_DB_MAX_SCAN_TIME_MILLIS for the ES-narrowed scan + // (applyESFilter=true): a hard ceiling on total rows read, independent of elapsed time. The + // time budget alone does not cap *work* -- a fast DB/ES pair could scan far more rows in + // BROWSER_DB_MAX_SCAN_TIME_MILLIS than the old row-count cutoff ever allowed, which under + // concurrent unfiltered searches multiplies real cost (found in review, issue #37211). Set + // well above the real-world scale this fix targets (~718,174 contentlets, per the issue) so + // it does not reintroduce the original completeness bug at that scale; it exists to cap the + // pathological case (a much larger site, or many concurrent requests) that the time budget + // alone cannot. + static final String BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP_KEY = "BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP"; + static final int BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP_DEFAULT = 1_000_000; + // Default DB chunk size for the hybrid DB+ES text-filtering loop. static final String BROWSER_CONTENT_CHUNK_SIZE_KEY = "BROWSER_CONTENT_CHUNK_SIZE"; static final int BROWSER_CONTENT_CHUNK_SIZE_DEFAULT = 900; diff --git a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java index 9c4ba59b4eac..f3a83cfb52a0 100644 --- a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java @@ -2490,6 +2490,192 @@ public void test_getPaginatedContents_unfilteredTextSearch_noMatchStaysBounded() } } + /** + *
      + *
    • Method to Test: {@link BrowserAPI#getPaginatedContents(BrowserQuery)}
      • + *
    • Given Scenario: {@code BROWSER_DB_MAX_SCAN_TIME_MILLIS} is set to an + * effectively-zero budget (1 ms) so the ES-narrowed scan's time cutoff -- not + * DB exhaustion, not the row-count guard rail -- fires after the very first chunk, with + * a matching item still unscanned several chunks later. This exercises the mechanism + * itself (found in review: the earlier scan-limit tests only ever exhaust the DB + * naturally, so none of them actually drive {@code scanBudgetExhausted} to + * {@code true} via elapsed time).
      • + *
    • Expected Result: Page 1 stops after one chunk with {@code hasMoreContent} + * true and a partial-progress cursor; the match is not yet in that page. Resuming from + * that cursor with a normal time budget reaches the match -- proving the time-cutoff + * path produces a valid, resumable cursor rather than silently losing coverage.
      • + *
    + */ + @Test + public void test_getPaginatedContents_timeBudgetExhausted_resumesFromCursor() throws Exception { + final int chunkSize = 5; + final int fillerCount = 15; + final long tinyTimeBudgetMillis = 1L; + + Config.setProperty("BROWSER_CONTENT_CHUNK_SIZE", chunkSize); + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY, tinyTimeBudgetMillis); + try { + final Host host = new SiteDataGen().nextPersisted(); + final Folder folder = new FolderDataGen().site(host).nextPersisted(); + final var contentType = new ContentTypeDataGen() + .host(host) + .folder(folder) + .field(new FieldDataGen().name("title").velocityVarName("title").next()) + .nextPersisted(); + + for (int i = 0; i < fillerCount; i++) { + new ContentletDataGen(contentType) + .setProperty("title", "Filler " + i) + .host(host) + .folder(folder) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + } + final Contentlet match = new ContentletDataGen(contentType) + .setProperty("title", "Wombat Image") + .host(host) + .folder(folder) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + + final BrowserQuery firstQuery = BrowserQuery.builder() + .withHostOrFolderId(folder.getIdentifier()) + .withFilter("Wombat") + .useElasticsearchFiltering(true) + .showContent(true) + .showFiles(false) + .showFolders(false) + .showLinks(false) + .showDotAssets(false) + .showWorking(true) + .showArchived(false) + .maxResults(100) + .contentCursor(0) + .build(); + + final PaginatedContents firstPage = browserAPI.getPaginatedContents(firstQuery); + + assertNotNull("First page must not be null", firstPage); + assertTrue("The 1ms time budget must cut the scan short before the match's chunk " + + "is reached", firstPage.hasMoreContent); + assertTrue("nextContentCursor must reflect partial progress (> 0 and <= filler count, " + + "not the full dataset) -- was: " + firstPage.nextContentCursor, + firstPage.nextContentCursor > 0 && firstPage.nextContentCursor <= fillerCount); + final Set firstPageInodes = firstPage.list.stream() + .map(item -> (String) item.get("inode")) + .collect(Collectors.toSet()); + assertFalse("The match must not appear in the time-cutoff page", + firstPageInodes.contains(match.getInode())); + + // Resume from the returned cursor with a normal time budget so the scan can finish. + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY, + BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_DEFAULT); + final BrowserQuery secondQuery = BrowserQuery.builder() + .withHostOrFolderId(folder.getIdentifier()) + .withFilter("Wombat") + .useElasticsearchFiltering(true) + .showContent(true) + .showFiles(false) + .showFolders(false) + .showLinks(false) + .showDotAssets(false) + .showWorking(true) + .showArchived(false) + .maxResults(100) + .contentCursor(firstPage.nextContentCursor) + .build(); + + final PaginatedContents secondPage = browserAPI.getPaginatedContents(secondQuery); + final Set secondPageInodes = secondPage.list.stream() + .map(item -> (String) item.get("inode")) + .collect(Collectors.toSet()); + + assertTrue("Resuming from the time-cutoff cursor must reach the match in a later " + + "chunk", secondPageInodes.contains(match.getInode())); + } finally { + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY, + BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_DEFAULT); + Config.setProperty("BROWSER_CONTENT_CHUNK_SIZE", 900); + } + } + + /** + *
      + *
    • Method to Test: {@link BrowserAPI#getPaginatedContents(BrowserQuery)}
      • + *
    • Given Scenario: Every item in the fixture matches the free-text filter, and + * {@code BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP} is lowered well below the fixture's row + * count while {@code BROWSER_DB_MAX_SCAN_TIME_MILLIS} is left generous, so only the row + * hard cap -- not the time budget, not running out of matches -- can terminate the + * ES-narrowed scan (found in review: the co-bound added alongside the time budget had no + * test of its own).
      • + *
    • Expected Result: The scan stops at the hard cap with {@code hasMoreContent} + * true, proving the ES path is not left with an effectively unbounded row ceiling once + * the row-count-based cutoff was replaced by a time budget.
      • + *
    + */ + @Test + public void test_getPaginatedContents_esRowHardCap_stopsRunawayScanUnderGenerousTimeBudget() + throws Exception { + final int chunkSize = 5; + final int hardCap = 15; + final int fillerCount = 20; + + Config.setProperty("BROWSER_CONTENT_CHUNK_SIZE", chunkSize); + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP_KEY, hardCap); + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY, + BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_DEFAULT); + try { + final Host host = new SiteDataGen().nextPersisted(); + final Folder folder = new FolderDataGen().site(host).nextPersisted(); + final var contentType = new ContentTypeDataGen() + .host(host) + .folder(folder) + .field(new FieldDataGen().name("title").velocityVarName("title").next()) + .nextPersisted(); + + for (int i = 0; i < fillerCount; i++) { + new ContentletDataGen(contentType) + .setProperty("title", "AllMatchTerm " + i) + .host(host) + .folder(folder) + .setPolicy(IndexPolicy.WAIT_FOR) + .nextPersisted(); + } + + final BrowserQuery query = BrowserQuery.builder() + .withHostOrFolderId(folder.getIdentifier()) + .withFilter("AllMatchTerm") + .withContentTypes(Set.of(contentType.id())) + .useElasticsearchFiltering(true) + .showContent(true) + .showFiles(false) + .showFolders(false) + .showLinks(false) + .showDotAssets(false) + .showWorking(true) + .showArchived(false) + .maxResults(100) + .contentCursor(0) + .build(); + + final PaginatedContents result = browserAPI.getPaginatedContents(query); + + assertNotNull("Result must not be null", result); + assertTrue("The row hard cap must stop the scan before it reaches the end of the " + + "fixture (20 items) -- hasMoreContent should be true", + result.hasMoreContent); + assertTrue("nextContentCursor must reflect the hard cap having fired (>= hard cap, " + + "< full fixture size) -- was: " + result.nextContentCursor, + result.nextContentCursor >= hardCap && result.nextContentCursor < fillerCount); + } finally { + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP_KEY, + BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_ES_HARD_CAP_DEFAULT); + Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_KEY, + BrowserAPIImpl.BROWSER_DB_MAX_SCAN_TIME_MILLIS_DEFAULT); + Config.setProperty("BROWSER_CONTENT_CHUNK_SIZE", 900); + } + } + /** *
      *
    • Method to Test: {@link BrowserAPI#getPaginatedContents(BrowserQuery)}