DeclarativeRecipe ThreadLocal accumulator causes NPE for ScanningRecipe preconditions in multi-threaded execution

[timtebeek]

What happened

When a ScanningRecipe is used as a precondition in a DeclarativeRecipe (YAML recipe), its getVisitor(T acc) is called with null for the accumulator. This causes an NPE in any ScanningRecipe that uses the accumulator in getVisitor().

This affects RepositoryContainsFile in rewrite-core and RepositoryHasDependency / ModuleHasDependency in rewrite-java-dependencies.

Stack trace (from Moderne production)

java.lang.NullPointerException: Cannot invoke "java.util.concurrent.atomic.AtomicBoolean.get()" because "acc" is null
  org.openrewrite.java.dependencies.search.RepositoryHasDependency.getVisitor(RepositoryHasDependency.java:100)
  org.openrewrite.java.dependencies.search.RepositoryHasDependency.getVisitor(RepositoryHasDependency.java:28)
  org.openrewrite.config.DeclarativeRecipe.orVisitors(DeclarativeRecipe.java:517)
  org.openrewrite.config.DeclarativeRecipe.lambda$getRecipeList$1(DeclarativeRecipe.java:500)
  org.openrewrite.Preconditions.lambda$and$0(Preconditions.java:111)
  org.openrewrite.config.DeclarativeRecipe$PreconditionBellwether$1.<init>(DeclarativeRecipe.java:233)
  org.openrewrite.config.DeclarativeRecipe$PreconditionBellwether.getVisitor(DeclarativeRecipe.java:232)
  org.openrewrite.config.DeclarativeRecipe$BellwetherDecoratedRecipe.getVisitor(DeclarativeRecipe.java:284)
  org.openrewrite.scheduling.RecipeRunCycle.lambda$editSource$10(RecipeRunCycle.java:223)
  org.openrewrite.scheduling.RecipeStack.reduce(RecipeStack.java:60)
  org.openrewrite.scheduling.RecipeRunCycle.editSource(RecipeRunCycle.java:200)
  org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$8(RecipeRunCycle.java:194)
  io.moderne.worker.execution.CountingModerneLargeSourceSet.lambda$edit$4(CountingModerneLargeSourceSet.java:132)
  io.moderne.serialization.ModerneLargeSourceSet.edit(ModerneLargeSourceSet.java:450)

Root cause

DeclarativeRecipe stores its Accumulator in two places:

1. On the root cursor — via ScanningRecipe.getAccumulator() which calls rootCursor.computeMessageIfAbsent(key, ...). This survives across RecipeRunCycle boundaries and threads. However, the cursor is not passed through the impacted code path (RecipeStack.reduce() → getRecipeList() → orVisitors()), so it cannot be used to recover the accumulator at that point.
2. In a ThreadLocal<Accumulator> (line 165) — used by orVisitors() to look up precondition accumulators. This is thread-local and not shared.

The impacted code path is: both the scan and edit phases use RecipeStack.reduce(). This calls RecipeStack.recurseRecipeList() → DeclarativeRecipe.getRecipeList() → which creates suppliers that call orVisitors(). When a new thread takes or receives the getRecipeList() call, the ThreadLocal accumulator resolves to null, causing the error.

Here's what happens step by step:

1. Scan phase (Thread A): getInitialValue() creates the Accumulator, populates each precondition ScanningRecipe's sub-accumulator, sets the ThreadLocal on Thread A, and returns the accumulator which gets stored on the root cursor by ScanningRecipe.getAccumulator().

2. Worker yields: At any point during execution, the platform may yield the worker. A new RecipeRunCycle is created which contains a new RecipeStack (with an empty cache), but the same rootCursor is passed through.

3. New cycle calls reduce(): Both scanSources() and editSources() go through RecipeStack.reduce() → init() → recurseRecipeList() → DeclarativeRecipe.getRecipeList(). Since this is a fresh RecipeStack, the cache is empty, so getRecipeList() is called again. This creates new supplier objects and a new PreconditionBellwether.

4. Bellwether evaluates the supplier: PreconditionBellwether.getVisitor() triggers the supplier → calls orVisitors() → reads this.accumulator.get() (the ThreadLocal).

5. ThreadLocal is null: On a different thread (Thread B), the ThreadLocal was never set. Even on Thread A, a new RecipeRunCycle may not have triggered getInitialValue() again, so the ThreadLocal could be stale or cleared.

6. NPE: orVisitors() passes null to scanning.getVisitor(null), and the precondition recipe calls a method on the null accumulator.

The disconnect: the accumulator is safely stored on the root cursor (step 1), but orVisitors() only reads from the ThreadLocal (step 4). The cursor is not available in the getRecipeList() → orVisitors() code path, so it cannot fall back to the cursor there.

Reproducing test

This test simulates multi-threaded execution by scanning on the main thread and editing on a separate thread with a new RecipeRunCycle:

@Test
void scanningPreconditionAccumulatorSurvivesAcrossThreads() throws Exception {
    var recipe = new DeclarativeRecipe(
      "test.ScanningPrecondition", "Scanning precondition test", "Test.",
      emptySet(), null, URI.create("test"), true, emptyList()
    );
    recipe.addPrecondition(new RepositoryContainsFile("*.txt"));
    recipe.addUninitialized(new ChangeText("changed"));
    recipe.initialize(List.of());

    List<SourceFile> sources = List.of(
      PlainText.builder().id(Tree.randomId()).sourcePath(Paths.get("file1.txt")).text("hello").build()
    );

    var rootCursor = new Cursor(null, Cursor.ROOT_VALUE);
    var ctx = new WatchableExecutionContext(new InMemoryExecutionContext(error -> {
        throw new RuntimeException(error);
    }));

    // Scan on main thread
    var scanCycle = new RecipeRunCycle<LargeSourceSet>(
      recipe, 1, rootCursor, ctx,
      new RecipeRunStats(Recipe.noop()), new SearchResults(Recipe.noop()),
      new SourcesFileResults(Recipe.noop()), new SourcesFileErrors(Recipe.noop()),
      LargeSourceSet::edit
    );
    ctx.putCycle(scanCycle);
    scanCycle.scanSources(new InMemoryLargeSourceSet(sources));

    // Edit on a different thread to simulate platform cross-thread scenario.
    // The new RecipeRunCycle has a fresh RecipeStack, so getRecipeList() is called again,
    // creating new suppliers. The ThreadLocal is null on this thread, so the accumulator
    // must be resolved from the cursor instead.
    AtomicReference<Throwable> threadError = new AtomicReference<>();
    Thread editThread = new Thread(() -> {
        var editCycle = new RecipeRunCycle<LargeSourceSet>(
          recipe, 1, rootCursor, ctx,
          new RecipeRunStats(Recipe.noop()), new SearchResults(Recipe.noop()),
          new SourcesFileResults(Recipe.noop()), new SourcesFileErrors(Recipe.noop()),
          LargeSourceSet::edit
        );
        ctx.putCycle(editCycle);
        try {
            editCycle.editSources(new InMemoryLargeSourceSet(sources));
        } catch (RuntimeException e) {
            threadError.set(e);
        }
    });
    editThread.start();
    editThread.join();

    if (threadError.get() != null) {
        throw new AssertionError(
          "Edit phase which ran in different thread should not throw an exception.",
          threadError.get()
        );
    }
}

[Jenson3210]

Downstream recipes can add if (acc == null) guards in their getVisitor(T acc) override, which is what we did in openrewrite/rewrite-java-dependencies#174. But the framework-level fix should make this unnecessary.

Can they though... 🤔
This commit is actually wrong imo. When the threads lose the acc, we might suddenly not pass the precondition anymore even if the accumulator would mark the file in edit.

[timtebeek]

Agree! Best reverted; had only added that following this PR and the comments on it, while still figuring out the actual cause with this issue (and mostly being out yesterday)

• Fix NPE with RepositoryHasDependency as precondition rewrite-java-dependencies#174

[Jenson3210]

Edited PR description with a unit test that really has the error shown.

[timtebeek]

After some investigation we've concluded this is likely caused by yielding, and as such not something we have to fix anymore.