A negative cache limit makes grouped search silently return groups with no hits

GroupingSearch can cache the hits of its first pass in a CachingCollector and replay them into the second pass instead of running the query again. The caller gives the cache a size limit, and isCached() tells whether the cache kept every hit; if not, the query is re-run. A negative limit, for example setCaching(-1, true) meant as "no limit" or a computed RAM budget that went negative, is accepted without a check, but -1 is also the collector's own internal "purged" marker: no segment gets a caching collector, nothing is cached, and the flag that isCached() returns, which starts true and is cleared only by a caching segment collector, stays true. The second pass replays an empty cache: on 20 matching documents the grouped search returns its 3 groups with 0 hits each and totalHitCount 0, with no error, with assertions on or off. The same goes for setCachingInMB(-1.0, true), CachingCollector.create(.., -1) and the new CachingCollectorManager; through GroupingSearch it takes field or function grouping and a query that finds at least one group. Limits of 0 or more do not trigger it. The defect dates from LUCENE-5527 (5.0.0); until 9.7.0 it threw at replay, and #12380 (9.8.0) made it silent. Rejecting negative limits with an IllegalArgumentException turns the silent loss into an immediate error: this doc's test, the finder's test, the caching tests of the core module and every upstream grouping test pass with it.

• Two-pass grouping: GroupingSearch first finds the top groups (first pass), then collects the top documents inside those groups (second pass). Each pass normally runs the query.
• First-pass cache: with setCaching(maxDocsToCache, cacheScores) or setCachingInMB(maxMB, cacheScores), the first pass goes through a CachingCollector (since #16247 one per slice, through CachingCollectorManager) that records the matching doc ids per segment. If the cache stayed within its limit, isCached() is true and the second pass is fed by replay(); otherwise the cache is purged and the query is run again.

1. The cache flag starts true (CachingCollector.java:337). Only a caching segment collector clears it, when a hit would go over the limit (:178-182, :189-199). Its finish() then sets the collector's remaining limit to -1, the internal "purged" state, so later segments are not cached (:92-96, :214-221).
2. getLeafCollector creates a caching segment collector, and records the segment, only when the limit is >= 0 (:79-90); for -1 it hands back the wrapped collector untouched.
3. Nothing checks the sign of a caller's limit: create(.., int) and create(.., double) (:307-330; -1.0 MB becomes -262144 documents, or -131072 when scores are cached, :312), the CachingCollectorManager constructor (CachingCollectorManager.java:73-75, only "exactly one limit") and the GroupingSearch setters (GroupingSearch.java:250-272).
4. So a caller's -1 starts in the purged state with the flag still true. No segment is cached and nothing can clear the flag: isCached() is true, and replay() (:106-118) loops over an empty segment list and returns normally. GroupingSearch replays whenever isCached() is true (GroupingSearch.java:208-209).

After the first pass, a limit of 0 ends in cached=false maxDocsToCache=-1, a limit of -1 in cached=true maxDocsToCache=-1 with 0 recorded segments. The first pass itself is unaffected (it collected all 20 documents, so the 3 top groups are right); the second pass, fed by the empty replay, finds no documents in them:

CachingCollector.create(collector, false, -1) directly: the first pass collects 20, isCached() is true, replay() delivers 0. totalGroupedHitCount is 0 as well.

• Who is exposed. Code that enables first-pass caching with a negative limit. Caching is off by default (both limits start null, GroupingSearch.java:58-59), and all three classes are @lucene.experimental. Through GroupingSearch only field or function grouping is affected: the cache is built only in groupByFieldOrFunction (:128-137, 167-176); doc-block grouping does not use the cache limits.
• How a negative limit gets there. Most plausibly -1 meant as "no limit": IndexWriterConfig uses -1 (DISABLE_AUTO_FLUSH) as the "off" value of its RAM-MB and document-count flush limits. Or a computed budget (for example "free heap minus a reserve") that went negative. 0, NaN and tiny negative MB values (above about -3.8e-6) truncate to 0 and work. A limit of 0 or more cannot trigger it: the effective document limit then stays >= 0 (CachingCollector.java:312); tested were 0, 1000, Integer.MAX_VALUE, 1.0 MB and 4.0 MB.
• Effect. Every second-pass hit is lost: groups without documents, totalHitCount and totalGroupedHitCount 0. Nothing is added; the top groups themselves are right. No exception since 9.8.0, with assertions on or off. It needs a first pass that finds at least one group: a query that matches nothing returns its (correct) empty result before the replay decision (GroupingSearch.java:193-195).
• Scale. None needed: 20 documents in one segment; an independent repro used 21 documents in 3 segments.
• How likely. Low. A negative limit is a mistake, the documented searches found no report of either form, and Solr's grouping, the one downstream caller the checks read, only creates the cache when its computed limit is > 0.

# Lucene main 645d4fe14a20, assertions on (-ea), seed DEADBEEF: 1 test(s) run, 1 failed
    => java.lang.AssertionError: expected:<1> but was:<0>
       org.junit.Assert.fail(Assert.java:89)
       org.junit.Assert.failNotEquals(Assert.java:835)
       org.junit.Assert.assertEquals(Assert.java:647)
       org.junit.Assert.assertEquals(Assert.java:633)
       org.apache.lucene.search.grouping.TestGroupingNegativeCacheLimit.testNegativeMaxDocsToCache(TestGroupingNegativeCacheLimit.java:45)

• Class contract (CachingCollector.java:25-30): "Once the collection is done, call isCached. If this returns true, you can use replay(Collector) against a new collector. If it returns false, this means too much RAM was required and you must instead re-run the original search." isCached() (:340): "Return true is this collector is able to replay collection." replay() (:349-350): "@throws IllegalStateException if this collector is not cached (i.e., if the RAM limits were too low for the number of documents + scores to cache)."
• Limit contract. create(..) (:304-305, :323-324): "If the collector exceeds the threshold, no documents and scores are cached." GroupingSearch.setCaching / setCachingInMB (GroupingSearch.java:241-244, 258-261): "If the cache grows beyond the specified limit, then the cache is purged and not used in the second pass search." CachingCollectorManager's example re-runs the query when isCached() is false.
• Tests. No test uses a negative limit. TestCachingCollectorManager expects a limit of 0 to give isCached() false and replay to throw IllegalStateException, and already expects IllegalArgumentException for a wrong combination of limits.
• History. The >= 0 gate, the -1 marker and the cached = true start came with LUCENE-5527 (2014), whose discussion never mentions the limits. #12380 (2023) made the failure silent as a side effect of an unrelated refactoring; no review comment mentions CachingCollector. #16247 (2026) added the manager's "exactly one limit" check without the sign.
• Maintainers prefer a loud failure here. In LUCENE-4372 (#5438, open since 2012) the worry was a too-small limit silently dropping hits; it was settled because replay() throws: "I don't have any concerns if you are gonna get an exception." A negative limit gets around exactly that exception. Nearby APIs validate similar values (TopScoreDocCollectorManager: "numHits must be > 0"; PhraseQuery: "Slop must be >= 0").

The strongest case for "works as designed" (the cross-model reviews rated it moderate): a maximum document count or RAM size has an implicit non-negative domain, so -1 is invalid input; the classes are @lucene.experimental; disableCaching() exists for "no cache"; Solr guards its own call with > 0; negative limits never worked (they threw from 5.0.0 to 9.7.0) and no report of it was found.

Why it fails: an implicit domain the API does not enforce is what argument validation is for. Nothing documents a range or a meaning for negative values, the setters accept them silently, and the result breaks the class's own contract (isCached() true means the replay is usable) with silently wrong results instead of the exception the maintainers relied on in #5438. Verdict: not intended; the remaining doubt is about impact and wording. What the case does get right: rejecting negative limits changes behaviour for the few such calls that return correct results today (see the fix trade-offs).

Three shapes were run on main. The recommended one reports the mistake where it is made; the core-only shape is the finder's patch; the alternative keeps today's API and makes a negative limit behave like 0.

The change is +23 lines in 3 files: the two CachingCollector.create factories, the CachingCollectorManager constructor and GroupingSearch.setCaching / setCachingInMB throw IllegalArgumentException("... must be >= 0, got ..."), with @throws lines in their javadoc. fix-reject-negative-tests.diff (+36 lines) adds the checks to TestCachingCollector, TestCachingCollectorManager.testConstructor and TestGroupingSearch.

// CachingCollector.create(Collector, boolean, int); likewise CachingCollectorManager, GroupingSearch.setCaching
if (maxDocsToCache < 0) {
  throw new IllegalArgumentException("maxDocsToCache must be >= 0, got " + maxDocsToCache);
}
// CachingCollector.create(Collector, boolean, double); likewise CachingCollectorManager; setCachingInMB checks maxCacheRAMMB
if (maxRAMMB >= 0 == false) { // also rejects NaN
  throw new IllegalArgumentException("maxRAMMB must be >= 0, got " + maxRAMMB);
}

Trade-offs

• A call with a negative limit now throws: a behaviour change, not only a bug fix. Most such calls give wrong results today (or threw at replay before 9.8.0), but some give correct results and would now fail too: a grouped search whose first pass finds no group returns before the replay (GroupingSearch.java:193-195), and a direct CachingCollector user that never calls isCached() / replay() gets the wrapped collection only.
• NaN, which truncates to 0 and works by accident today, now throws; check maxRAMMB < 0 instead to keep accepting it.
• With the setter checks the error comes from setCaching itself; without them (next option) it comes from GroupingSearch.search. Code that sets -1 and expects only the search to fail sees it earlier: the independent reproducer's 2 GroupingSearch tests call the setter outside their try and so fail with the rejection's IllegalArgumentException, an outcome their oracle accepts only inside the try.
• Argument checks only, outside the collection loop: no performance effect; no benchmark was run. A PR would add a CHANGES entry.

What the checks validated

• Gradle on main, assertions on and off: TestCachingCollector* in core (2 classes, 9 tests) and the whole grouping suite (14 upstream classes, 57 tests) pass, and so do this doc's test (4 of 4), the finder's (5 of 5) and the independent reproducer's manager test (3 of 3). The only 2 failures are that reproducer's 2 GroupingSearch tests, which get the setter's new exception outside their try (see the trade-offs).
• On stock main the same selection fails only in the repro classes (14 of 29 grouping tests), with assertions on and off.
• :lucene:core:check -x test and :lucene:grouping:check -x test pass with the patch, its tests and this doc's test (google-java-format, ecj lint, forbidden APIs).

The same checks without the GroupingSearch setter checks and without the javadoc lines (+10 lines in 2 files). GroupingSearch.setCaching(-1, true) is still accepted, and the next search throws from the manager's constructor.

Trade-offs

Smallest change, all in core; the grouping user sees the error one call later, at search time, which is still before any result is returned.

What the checks validated

Gradle on main, assertions on: core 8 of 8 and the whole grouping suite 75 of 75, the independent reproducer's tests included; earlier the finder ran the 4 upstream caching and grouping suites with it.

The change is +9/-3 lines in CachingCollector.create(Collector, boolean, int): when the limit is negative, set cached = false on the new collector before returning it. A negative limit then behaves like 0: isCached() is false, GroupingSearch re-runs the query, and replay() throws IllegalStateException as documented.

CachingCollector collector =
    cacheScores
        ? new ScoreCachingCollector(other, maxDocsToCache)
        : new NoScoreCachingCollector(other, maxDocsToCache);
if (maxDocsToCache < 0) {
  // a negative limit leaves room for no document: cache nothing and report the cache as purged
  collector.cached = false;
}
return collector;

Trade-offs

No new exception, so it is the gentlest backport. But the mistake stays hidden: a caller who meant -1 as "no limit" silently gets no caching at all and runs every grouped query twice.

What the checks validated

Gradle on main, assertions on: core 8 of 8 and the whole grouping suite 75 of 75; every repro passes because the right result comes back.

Read a negative limit as unbounded, like IndexWriterConfig's -1.

Trade-offs

A new feature rather than a fix, and it removes the RAM guard the limit exists for (4 or 8 bytes per hit, which the class javadoc warns can be "a very substantial amount of RAM"). Not run.

Method: semi-formal (a model's precondition, then code reading and executed tests).

• Hunt (2026-10-28). A hunt on another question (does GroupingSearch apply its cache limit per slice since #16247?) wrote an SMT model of the cache bound in which the limit's domain is D >= 0. Checking that precondition against the code showed that D = -1 is CachingCollector's own "invalidated" state and that no API rejects it.
• The finder then ran a probe and a public-API JUnit test (TestCachingNegativeLimit, 4 of 5 fail on main, assertions on and off; the limit-0 control passes), a version-agnostic main() over 12 releases from 8.0.0 to 10.5.2 (silent from 9.8.0, an exception before), and the reject-negative fix.
• Independent checks followed (see below); the history step extended the sweep to 135 releases from 3.2.0.