BpVectorReorderer never calls the Executor it is given and forks its work onto ForkJoinPool.commonPool()

BpVectorReorderer (lucene/misc, main and branch_11x only) renumbers documents by a float vector field, through BPReorderingMergePolicy, computeDocMap(reader, tempDir, executor) or its command line. The merge API describes the executor as the means of parallelism, and IndexWriter always passes the merge scheduler's intra-merge executor, which by default runs everything on the calling thread. The reorderer wraps that executor in a TaskExecutor and carries it through every task, but never calls it: its tasks are ForkJoin RecursiveActions forked with ForkJoinTask.invokeAll from a thread outside any pool, so a non-null executor only switches forking on and each forked half runs on ForkJoinPool.commonPool(). The command line's --thread-count pool never receives a task and is never shut down. The order is correct (each thread reads its own copy of the vectors); the reorder's CPU work just runs on common-pool threads (by default up to cores - 1) outside the merge scheduler's thread budget. The sibling BPIndexReorderer was moved to the executor in #13289, four months before this class was added with the old pattern. Main and branch_11x only; no release has the class.

1. The executor is wrapped and carried, never called. computeDocMap builds new TaskExecutor(executor) when the executor is non-null (BpVectorReorderer.java:580-585) and passes it through computeValueMap (:591, :596-607) to the root ReorderTask (:627). Every task stores it (:151-157) and hands it to its children (:254-257, :526-547). Nothing calls invokeAll, submit or execute on it.
2. It is only a switch. shouldFork returns false when the executor is null (:160-162); otherwise it consults the ForkJoin-only getSurplusQueuedTaskCount() (:163-167) and forks at the root and above FORK_THRESHOLD = 8192 (:78, :168-173).
3. Forking goes through ForkJoin. BaseRecursiveAction extends RecursiveAction (:149); ReorderTask (:259-260) and ComputeBiasTask (:523-547) call ForkJoinTask.invokeAll, which forks one half and runs the other on the current thread.
4. The caller is outside any pool. The root runs with .compute() on the calling thread (:627). fork() from a thread that is not in a ForkJoinPool uses ForkJoinPool.commonPool(), so a common-pool worker takes the forked half while the caller works on the other; its own forks stay in the common pool.
5. The command line's pool is dead code. main() builds new ForkJoinPool(threadCount, ...) when --thread-count is not 1 (default: the core count, :660, :673-678) and passes it as the executor (:679-681); nothing submits to it and it is never shut down.
6. Results stay correct. Each thread gets its own vectors.copy() (:108-110, :619-625), and tasks work on disjoint slices of the shared arrays (:252-253).

The sibling for comparison: BPIndexReorderer.java:158-191 has a task base class and shouldFork of the same shape (its shouldFork has no queue-depth check) but implements Callable<Void> and forks with executor.invokeAll(...); TaskExecutor submits all but one task to the executor and runs the rest on the calling thread (TaskExecutor.java:37-45).

• Minimal test: 4,096 two-dimensional vectors, executor Runnable::run: readers are the test thread and ForkJoinPool.commonPool-worker-1 (gradle -ea and -da; standalone runner 1/1 -ea, 10/10 iterations -da; also with -Djava.util.concurrent.ForkJoinPool.common.parallelism=0).
• Model leg (executor null, caller-runs, fixed 2-thread pool, 2-thread ForkJoinPool x 64 and 4,096 vectors x EUCLIDEAN and DOT_PRODUCT): null stays on the caller; all 12 non-null cells read on the common pool with 0 tasks handed to the executor. With the fix no cell touches the common pool and the executor receives 3 to 5 tasks.
• Probe: a 2-thread ForkJoinPool (the --thread-count 2 shape) and 65,536 vectors: 0 execute calls, the pool never started a thread, readers were the caller and 9 common-pool workers (common-pool parallelism 31 on that host). With the fix: 206 execute calls; the readers were the caller and the pool's 2 workers, no common-pool thread.

• Who. Users of main or branch_11x (would ship in 11.0.0) who opt in: BPReorderingMergePolicy with a BpVectorReorderer, on every reordering merge with any merge scheduler, since IndexWriter always passes getIntraMergeExecutor(merge) (IndexWriter.java:3558-3562 for addIndexes(CodecReader...), :5542-5558 for merges; BPReorderingMergePolicy.java:133-138); a direct computeDocMap call with a non-null executor; the command line with --thread-count other than 1, which includes the default (the core count) on any multi-core host.
• What goes wrong. The reorder's CPU work runs on common-pool threads (with the JVM's default common-pool size, up to cores - 1) instead of the merge thread (SerialMergeScheduler, "using the current thread"; small ConcurrentMergeScheduler merges, "don't do multithreaded merges for small merges"), instead of ConcurrentMergeScheduler's intra-merge executor, which "provides a limited number of threads" tied to maxThreadCount, or instead of the application's own pool. It competes with everything else in the JVM that uses the common pool (parallel streams, default async CompletableFuture methods). Among valid (positive) values, --thread-count T only matters as 1 versus not 1; 0 or a negative value makes the ForkJoinPool constructor throw IllegalArgumentException.
• Not hit. The document order is correct; no exception, hang or corruption; a null executor stays on the caller.
• Default vs opt-in, scale. Opt-in class in lucene/misc, used by no default codec or merge policy. Any input big enough to be split (at least 2 x minPartitionSize, 64 vectors by default): the root split then always forks when an executor is given, and 64 vectors already reach the common pool. Smaller inputs are not split, so nothing reaches the common pool (63 vectors: no reader in 20 of 20 iterations). Same with assertions on and off.
• How likely to be noticed. Silent: only applications that budget merge threads or share the common pool feel it, and no public report was found. No benchmark of the harm was run, so it stands as a consistency fix.

• Same-package test. It calls the package-private computeValueMap(vectors, similarity, TaskExecutor), which computeDocMap calls with new TaskExecutor(executor) and which TestBpVectorReorderer also uses. The cross-model reviewer reproduced the same through public computeDocMap on an IndexWriter-written 8,192-document index.
• How stable. invokeAll forks one half and computes the other on the caller, which takes long enough that a common-pool worker picks up the forked half. That is scheduling in principle, but it failed in every run observed: gradle 2/2 (the test JVM reported cpus=1), standalone runner 1/1 -ea and 10/10 iterations -da, 10/10 separate -da JVMs, an independent test 10/10 and with -XX:ActiveProcessorCount=1, and with common-pool parallelism 0 (JDK 25 still starts worker-1).
• Fully deterministic variant: wrap Runnable::run in an Executor that counts calls and also assert the count is above 0, in this same test. It is 0 on main for every non-null executor and 3 to 5 with the fix; on its own it only shows that the executor is unused.
• Format. The study's test, re-wrapped by applyGoogleJavaFormat with its comment split in two; it passes checkGoogleJavaFormat and ecjLintTest. As a regression test, use this same test.

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

# stock main 645d4fe14a20, gradle, seed DEADBEEF, assertions on (off, seed 5EED1070: same failure)
TestBpVectorReordererExecutor > testSameThreadExecutorKeepsWorkOnCaller FAILED
    java.lang.AssertionError: expected:<[Thread[#20,TEST-TestBpVectorReordererExecutor.testSameThreadExecutorKeepsWorkOnCaller-seed#[DEADBEEF],5,TGRP-TestBpVectorReordererExecutor]]> but was:<[Thread[#21,ForkJoinPool.commonPool-worker-1,5,InnocuousForkJoinWorkerThreadGroup], Thread[#20,TEST-TestBpVectorReordererExecutor.testSameThreadExecutorKeepsWorkOnCaller-seed#[DEADBEEF],5,TGRP-TestBpVectorReordererExecutor]]>
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:3B24BB7FAA917674]:0)
        at junit@4.13.2/org.junit.Assert.fail(Assert.java:89)
        at junit@4.13.2/org.junit.Assert.failNotEquals(Assert.java:835)
        at junit@4.13.2/org.junit.Assert.assertEquals(Assert.java:120)
        at junit@4.13.2/org.junit.Assert.assertEquals(Assert.java:146)
        at org.apache.lucene.misc.index.TestBpVectorReordererExecutor.testSameThreadExecutorKeepsWorkOnCaller(TestBpVectorReordererExecutor.java:58)
:lucene:misc:test (FAILURE): 1 test, 1  failure

# with the recommended fix: the repro + TestBpVectorReorderer + TestBPIndexReorderer + TestBPReorderingMergePolicy
:lucene:misc:test (SUCCESS): 20 tests, 2 skipped
# the 2 skipped are @Nightly; with -Ptests.nightly=true (TestBPReorderingMergePolicy + TestBpVectorReorderer)
:lucene:misc:test (SUCCESS): 11 tests
# all of lucene/misc
:lucene:misc:test (SUCCESS): 317 tests, 9 skipped

• IndexReorderer#computeDocMap: "@param executor may be used to parallelize reordering work." (IndexReorderer.java:32)
• OneMerge#reorder: "An executor that can be used to parallelize the reordering logic. May be {@code null} if no concurrency is supported." (MergePolicy.java:321-322)
• MergeScheduler#getIntraMergeExecutor: "By default, the method returns a SameThreadExecutorService where all intra-merge actions occur in their calling thread." (MergeScheduler.java:60-66). SerialMergeScheduler does each merge "using the current thread"; ConcurrentMergeScheduler returns the same-thread executor for small merges on purpose.
• #13289 (Adrien Grand, 2024-09-12): "Index reordering can benefit greatly from parallelism, so it should try to use the intra-merge executor when possible." It moved BPIndexReorderer from RecursiveAction to Callable tasks forked through TaskExecutor.invokeAll, whose javadoc says "A value of null indicates that reordering should be performed in the current thread" (BPIndexReorderer.java:831-832). BpVectorReorderer (#14097, 2025-01-07) has the old RecursiveAction code with the new TaskExecutor field.
• The class expects it too. It carries the executor through every task and builds a --thread-count pool for the command line; neither is used.
• Tests. TestBpVectorReorderer builds its own ForkJoinPool "to avoid tests failing with FS-based directories" and passes it (:132-144); on main it runs nothing. No test asserts where the work runs.
• The class author and other contributors assume it is used. Open #16729, from another contributor, describes the class as using executor capacity at each recursion node; on open PR #15430 the class author credits a merge-time speedup to "passing the mergestate intra-merge executor in", which with this code comes from the common pool.

The case for intended: read literally, "may be used" and "May be null if no concurrency is supported" make a non-null executor a licence to parallelize; no javadoc names the pool the work must run on; results are correct and OneMerge#reorder is experimental. The cross-model reviewer (OpenAI GPT-5.6, refute lens) rated this a moderate argument.

Why it fails. (1) Lucene's own schedulers pass a non-null same-thread executor precisely to mean "no concurrency"; under the licence reading every reordering SerialMergeScheduler merge would spread over the common pool, contradicting "using the current thread". (2) "May be used" permits not parallelizing, not parallelizing on a different, JVM-wide pool. (3) The carried-but-unused executor and the dead command-line pool show the work was meant to run there. (4) #13289 fixed the same pattern in the sibling for this reason. The reviewer, the intended-behaviour devil's advocate and the adjudication all called it real.

Option A was applied in a scratch worktree of main 645d4fe14a20 and run with gradle and the standalone JUnit runner; B and C are alternatives from reading the code, not run. Workaround today (from the code, not run as such; the null-executor controls stay on the caller): wrap the reorderer so it gets a null executor, for example new BPReorderingMergePolicy(mp, (reader, dir, executor) -> bpv.computeDocMap(reader, dir, null)); the reorder then stays on the merge thread, serially (shouldFork returns false for null).

BaseRecursiveAction implements Callable<Void> with an abstract compute(); call() runs it, and a local invokeAll(BaseRecursiveAction...) forks through executor.invokeAll(Arrays.asList(actions)) (as in BPIndexReorderer.java:183-190). Drop the ForkJoin-only getSurplusQueuedTaskCount() check. Shut down the pool that main() creates: without that, the command-line pool now starts threads that are never stopped, and the upstream suite fails its thread-leak check.

diff --git a/lucene/misc/src/java/org/apache/lucene/misc/index/BpVectorReorderer.java b/lucene/misc/src/java/org/apache/lucene/misc/index/BpVectorReorderer.java
index f2b987b3a51..9c490d9c387 100644
--- a/lucene/misc/src/java/org/apache/lucene/misc/index/BpVectorReorderer.java
+++ b/lucene/misc/src/java/org/apache/lucene/misc/index/BpVectorReorderer.java
@@ -22,10 +22,10 @@ import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.file.Path;
 import java.util.Arrays;
+import java.util.concurrent.Callable;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ForkJoinPool;
 import java.util.concurrent.ForkJoinWorkerThread;
-import java.util.concurrent.RecursiveAction;
 import org.apache.lucene.index.CodecReader;
 import org.apache.lucene.index.DirectoryReader;
 import org.apache.lucene.index.FieldInfo;
@@ -146,7 +146,7 @@ public class BpVectorReorderer extends AbstractBPReorderer {
     }
   }
 
-  private abstract class BaseRecursiveAction extends RecursiveAction {
+  private abstract class BaseRecursiveAction implements Callable<Void> {
 
     protected final TaskExecutor executor;
     protected final int depth;
@@ -156,15 +156,27 @@ public class BpVectorReorderer extends AbstractBPReorderer {
       this.depth = depth;
     }
 
+    protected abstract void compute();
+
+    @Override
+    public Void call() {
+      compute();
+      return null;
+    }
+
+    protected final void invokeAll(BaseRecursiveAction... actions) {
+      assert executor != null : "Only call invokeAll if shouldFork returned true";
+      try {
+        executor.invokeAll(Arrays.asList(actions));
+      } catch (IOException e) {
+        throw new UncheckedIOException(e);
+      }
+    }
+
     protected final boolean shouldFork(int problemSize, int totalProblemSize) {
       if (executor == null) {
         return false;
       }
-      if (getSurplusQueuedTaskCount() > 3) {
-        // Fork tasks if this worker doesn't have more queued work than other workers
-        // See javadocs of #getSurplusQueuedTaskCount for more details
-        return false;
-      }
       if (problemSize == totalProblemSize) {
         // Sometimes fork regardless of the problem size to make sure that unit tests also exercise
         // forking
@@ -678,6 +690,10 @@ public class BpVectorReorderer extends AbstractBPReorderer {
     }
     try (Directory dir = FSDirectory.open(Path.of(directory))) {
       reorderer.reorderIndexDirectory(dir, executor);
+    } finally {
+      if (executor instanceof ForkJoinPool pool) {
+        pool.shutdown();
+      }
     }
   }
 

Trade-offs

Parallelism is then bounded by the given executor: a same-thread executor (serial merges, small ConcurrentMergeScheduler merges) gives a serial reorder, so a reorder that today silently gets common-pool parallelism may take longer, and speedups measured with the current code (such as the one reported on #15430) may change. Forking is decided by size alone, as in BPIndexReorderer. No benchmark was run. Open PR #15430 moves the class, so the fix site may move.

What the checks validated

Gradle, scratch worktree of 645d4fe14a20, JDK 25 (2026-11-02): the minimal test passes with assertions on (DEADBEEF) and off (5EED1070); TestBpVectorReorderer 8/8, TestBPIndexReorderer 8/8, TestBPReorderingMergePolicy 3/3 with -Ptests.nightly=true; all of :lucene:misc:test (317 tests, 9 skipped, 0 failures); checkGoogleJavaFormat, ecjLintMain, ecjLintTest. Standalone runner on all cores: the upstream suite passes 8/8 with the fix and fails its thread-leak check (ForkJoinPool-1-worker-1 leaked) without the main() shutdown. The study: no common-pool reads in any model cell, 3 to 5 tasks reach the executor; with a 2-thread pool and 65,536 vectors, 206 execute calls, and the readers were the caller and the pool's 2 workers (no common-pool thread).

What BPIndexReorderer did before #13289 (it submitted its root task to the given pool); do not fork for any other executor. Keeps RecursiveAction.

Trade-offs

The merge schedulers' executors are not ForkJoinPools (MergeScheduler.java:38, ConcurrentMergeScheduler.java:945), so every merge would reorder serially, the opposite of #13289's goal. Not run.

Say that the executor only switches forking on and the common pool does the work.

Trade-offs

Contradicts OneMerge#reorder, SerialMergeScheduler and the sibling's contract, and leaves --thread-count meaningless. Not recommended.

• Lead. A unit on the misc reorder and split tools (2026-10-26) ran a determinism differential (null, ForkJoinPool, direct and rejecting executors gave identical doc maps), then read the reorderers' executor plumbing and witnessed the defect with a JUnit that records which threads read the vectors.
• Adjudication unit. Re-ran the lead, reduced it to one JUnit and ran a bounded differential over executor kind {null, caller-runs, fixed pool, ForkJoinPool} x size {64, 4096} x similarity {EUCLIDEAN, DOT_PRODUCT} on stock main and with the fix, counting tasks that reach the executor.
• Verification. An independent reproducer, an executing cross-model review (OpenAI GPT-5.6, refute lens), an intended-behaviour devil's advocate, an adjudication and a credibility audit.