DirectoryTaxonomyReader.getBulkPath overwrites entries of its ordinals argument, although the javadoc says it may only reorder them

TaxonomyReader.getBulkPath(int... ordinals) returns the labels of several taxonomy ordinals in one call, and its javadoc warns: "this method may change (reorder elements) its parameter, you should avoid reusing the parameter after the method is called." From 9.0.0 to 10.4.0 DirectoryTaxonomyReader.getBulkPath sorted the whole array in place, exactly as described.

Since 10.5.0 (#15711, which made the method use the category cache) it looks every ordinal up in the cache first, then copies the uncached ordinals into the front of the caller's array and sorts only that prefix. The slots after the prefix keep their old values, so a cached ordinal that comes before an uncached one is overwritten and the moved value is left twice: with ordinal 2 cached, {2, 1} becomes [1, 1] and {3, 2, 1} becomes [1, 3, 1]. The returned labels are always right.

So this is a documentation/contract defect, priority Low: only a caller that reads the array again after the call, which the javadoc already advises against, gets wrong data, and no in-tree caller does. A two-line swap in the compaction loop makes the javadoc true again without an allocation; a javadoc change is the lowest-risk alternative.

1. The contract. TaxonomyReader.java:241-242: "Note: this method may change (reorder elements) its parameter, you should avoid reusing the parameter after the method is called." The default implementation (:244-250) does not touch the array; the class is @lucene.experimental (:57).
2. The override's own javadoc (DirectoryTaxonomyReader.java:499-508) says nothing about the parameter, so the note is not inherited there. It still describes a StoredFields fallback (:502-505) that the code no longer has.
3. Cache first. :515 getPathFromCache(ordinals) fills the labels of the cached ordinals.
4. Compaction into the caller's array. :519-526: for each ordinal whose label is still missing, ordinals[uncachedLength] = ordinals[i] (:522). The value at uncachedLength is overwritten, not swapped.
5. All cached: early return at :528-530, the array is untouched.
6. Prefix sort only. :533-549 sorts ordinals[0..uncachedLength) with their original positions (.sort(0, uncachedLength), :549); the lookup (:558-578) and the cache update (:580-585) read only that prefix. Nothing restores the rest.
7. What stays right: the returned labels; checkOrdinalBounds still runs first (:512); every write stays inside the caller's array.

Only "a cached ordinal before an uncached one" breaks the contents: an uncached ordinal before a cached one compacts to the identity. The study's randomized check (300 trials, random cached subsets) saw the contents change in exactly those trials, with the labels right in every trial.

Worked example, {2, 1} with 2 cached: slot 0 (ordinal 2) is cached and skipped; slot 1 (ordinal 1) is uncached and copied into slot 0, giving [1, 1]; the prefix of length 1 is already sorted.

• Who. Code that calls DirectoryTaxonomyReader.getBulkPath (directly or through a TaxonomyReader reference) and reads the argument again afterwards.
• Default vs opt-in. The category cache is on by default (LRU, up to 4,000 labels; setCacheSize changes the limit). getPath adds a label on a miss and getBulkPath adds its uncached ordinals, so the mixed cached/uncached state is normal on a long-lived reader (or one from openIfChanged, which shares the caches unless the taxonomy was recreated). The call pattern that sees it goes against the javadoc's advice, in an @lucene.experimental API.
• In-tree callers are not affected. TaxonomyFacets.java:320-326 collects the counts before the call at :328 (the 2024 #14008 fix); :406 passes a fresh ordinals.toArray(); ReindexingEnrichedDirectoryTaxonomyWriter.java:72 uses only the labels; the sandbox TaxonomyOrdLabelBiMap.java:44-45 and TestDirectoryTaxonomyReader.java:521 clone first.
• It has happened before. Until #14008 was fixed in 2024, Lucene's own TaxonomyFacets read the array after getBulkPath (then a pure sort) and reported wrong counts.
• Silent. No exception; the labels are right, so nothing looks wrong unless the array is read again.

• Seeds and assertions. Fails with seeds DEADBEEF and 5EED1234, with assertions on and off.
• Releases. The same file fails unchanged on the 10.5.0 and 10.6.0 release jars and passes on 10.4.0 and 9.12.3.
• As a regression test. With the swap fix or the clone fix the test passes unchanged, so it is the regression test. With the javadoc-only fix it becomes obsolete, because the documented contract then allows the overwrite.

cp $REPRO/TestGetBulkPathArgumentOnlyReordered.java $LUCENE/lucene/facet/src/test/org/apache/lucene/facet/taxonomy/directory/
cd $LUCENE
./gradlew :lucene:facet:test --tests org.apache.lucene.facet.taxonomy.directory.TestGetBulkPathArgumentOnlyReordered -Ptests.seed=DEADBEEF -Ptests.asserts=true
# assertions off: -Ptests.asserts=false (same failure)

# gradle, main 645d4fe14a20, seed DEADBEEF, -Ptests.asserts=true
TestGetBulkPathArgumentOnlyReordered > testArgumentIsOnlyReordered FAILED
    javadoc: getBulkPath may only reorder its parameter [2, 1], but it now holds [1, 1]: arrays first differed at element [1]; expected:<2> but was:<1>
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:AAD32356BCA40DAE]:0)
        at app/junit@4.13.2/org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:78)
        ...
        at app//org.apache.lucene.facet.taxonomy.directory.TestGetBulkPathArgumentOnlyReordered.testArgumentIsOnlyReordered(TestGetBulkPathArgumentOnlyReordered.java:39)
:lucene:facet:test (FAILURE): 1 test, 1  failure

# the same with -Ptests.asserts=false: the same failure

# standalone, seed DEADBEEF, assertions on
10.6.0  FAIL  ... but it now holds [1, 1]   (also with assertions off)
10.5.0  FAIL  ... but it now holds [1, 1]
10.4.0  PASS
9.12.3  PASS

• The note was exact when written. #12769 (fb5f491643b, 2023-11-09, first in 9.9.0 and 10.0.0) added it when the method sorted the whole array in place; the same PR added the test comment "clone because getBulkPath changes order of param's elements".
• The change was a side effect. #15711 ("let getBulkPath use cache", fixes #15705) makes the method use the cache result it used to compute and throw away. Its description, reviews and linked issue never mention the argument; it did not touch TaxonomyReader.java; CHANGES.txt:990 lists it under Improvements as "DirectoryTaxonomyReader.getBulkPath() now uses getPathFromCache"; MIGRATE.md says nothing about getBulkPath.
• The overwrite is not needed for correctness. Swapping instead of copying keeps the array a permutation and the speed-up, with no allocation (Suggested fix); no benchmark was run.

The strongest case for "leave it": the same sentence says "you should avoid reusing the parameter after the method is called", so a caller that follows it is never affected. In #14008 (2024) a contributor wrote "We're using our API incorrectly by referencing ordinals after getBulkPath", and the fix went into the caller. Both classes are @lucene.experimental, overwriting saves work, and a maintainer can read "(reorder elements)" as an example rather than a limit.

Why it fails: it answers a wrong-results framing, which this report does not make. It does not make "(reorder elements)" accurate, and Lucene's own comments ("shuffles", "changes order") read the note as a permutation; the override's own javadoc says nothing about the parameter at all. Every verifier and reviewer in the study agreed on the mechanism, but the contract question stayed open: the independent reviewer from another model family (OpenAI GPT-5.6), refute lens, rated the "intended" case strong, called the contract question "unresolved" and wrote "A maintainer may still want to clarify that wording even if arbitrary mutation is accepted". Confidence is high that the note is inaccurate, and low to medium that a maintainer will think it worth a change.

Three options; all were applied in a scratch worktree of main 645d4fe14a20 and run with gradle. The swap is suggested because it makes the existing javadoc true again without an allocation (not benchmarked); the javadoc fix is the lowest-risk alternative for a maintainer who prefers no code change. The swap is this brief's suggestion; the clone is the study's.

In the compaction loop of DirectoryTaxonomyReader.getBulkPath, swap the uncached ordinal into the prefix instead of copying it. Before step i, the prefix holds the uncached ordinals seen so far, the slots up to i hold cached ones, and slot i still holds the caller's value; the swap keeps that, so the array stays a permutation, and the later code reads only the prefix.

     for (int i = 0; i < ordinalsLength; i++) {
       if (bulkPath[i] == null) {
+        // swap, do not overwrite: the caller's array may only be reordered
+        int x = ordinals[uncachedLength];
         ordinals[uncachedLength] = ordinals[i];
+        ordinals[i] = x;
         originalPosition[uncachedLength] = i;
         uncachedLength++;
       }

Trade-offs

• Makes the current javadoc true again and keeps #15711's speed-up; no allocation, one extra array read and one extra array write per uncached ordinal (not benchmarked).
• Callers still get a reordered array, as documented, but not the one 10.5.0 to 10.6.0 leave: {2, 1} comes back [1, 2] (executed); the labels do not change.
• A suggestion, not a must: the javadoc fix is the lowest-risk change for an @lucene.experimental API, and a maintainer may prefer it.

What the checks validated

• Gradle, scratch worktree of 645d4fe14a20 (2026-11-01): the minimal test passes with seed DEADBEEF, assertions on and off, and with seed 5EED1234.
• The whole facet module (:lucene:facet:test, the minimal test included) passes: 307 tests, 7 skipped, assertions on.
• :lucene:facet:checkGoogleJavaFormat and :lucene:facet:ecjLintMain pass (eclint is not configured in that build, so no eclint checks ran). Not run: gradlew check, other modules, any benchmark.
• A public-API probe with the swap on main (assertions on and off): {2, 1} with 2 cached comes back [1, 2], {3, 2, 1} [1, 3, 2]; labels right.

Say that the parameter's contents are unspecified after the call, and repeat the note on the DirectoryTaxonomyReader override, whose own javadoc does not inherit it.

--- TaxonomyReader.java
-   * <p>Note: this method may change (reorder elements) its parameter, you should avoid reusing the
-   * parameter after the method is called.
+   * <p>Note: this method may change its parameter (reorder or overwrite its elements), so its
+   * contents are unspecified after the call; you should avoid reusing the parameter after the
+   * method is called.
--- DirectoryTaxonomyReader.java (getBulkPath javadoc, before @param)
+   * <p>Note: this method may reorder and overwrite elements of its parameter, so its contents are
+   * unspecified after the call; you should avoid reusing the parameter after the method is called.
+   *

Trade-offs

• No behaviour change; documents what 10.5.0 to 10.6.0 already do.
• Concedes a weaker contract: a future caller that ignores the note gets a duplicate, not just a different order. The minimal test becomes obsolete.
• Optionally drop the stale StoredFields sentence of the override's javadoc (DirectoryTaxonomyReader.java:502-505) in the same change (not in the diff).

What the checks validated

• Gradle, same scratch worktree: :lucene:facet:checkGoogleJavaFormat and :lucene:facet:renderJavadoc pass, and the rendered TaxonomyReader and DirectoryTaxonomyReader pages show the new notes.
• No test was re-run: the change is inside comments, so it would not change the current test outcome; the minimal test becomes obsolete.

Rename the parameter and work on ordinalsParam.clone(), so the caller's array is never touched.

-  public FacetLabel[] getBulkPath(int... ordinals) throws IOException {
+  public FacetLabel[] getBulkPath(int... ordinalsParam) throws IOException {
     ensureOpen();
-    checkOrdinalBounds(ordinals);
+    checkOrdinalBounds(ordinalsParam);
+    // work on a copy: the uncached ordinals are compacted and sorted below
+    final int[] ordinals = ordinalsParam.clone();

Trade-offs

Stronger than the contract (the array is untouched) at one int[] copy per call; the method already allocates an int[] and a FacetLabel[] of the same length.

What the checks validated

• Gradle, same scratch worktree: the minimal test passes (seed DEADBEEF, assertions on) and :lucene:facet:checkGoogleJavaFormat passes.
• Caution: the study's version of this diff renamed the parameter but not its @param tag and does not compile in the gradle build (doclint: "@param name not found"); the diff used here also renames the tag.

• Context. A hunt unit wrote controlled-concurrency (Fray) and linearizability (Lincheck) harnesses for taxonomy reader cache sharing and commit discovery. Those runs found nothing; while writing them, the harness had to pass a clone to getBulkPath, which led to reading the compaction loop (2026-10-27).
• Witness. A sequential public-API JUnit on the stock main jar: three categories, getPath(2), then getBulkPath(3, 2, 1); the array came back [1, 3, 1], with assertions on and off. It was later reduced to two categories and {2, 1}.
• Then an independent reproducer, read-only cross-model reviews (OpenAI GPT-5.6), an intended-behaviour devil's advocate, an adjudication, a release history of 40 runs, a prior-art search and a credibility audit.