[RUN-4297] ansible inventory fix (#432)

ronaveva · web-flow · commit 4cdb81dc0bd4 · 2026-04-30T14:30:58.000-04:00
* fix race condition, now each instance uses its own tmp directory

* it keeps previous behavior not validatin tmp folder to exist

* add tests
diff --git a/src/main/groovy/com/rundeck/plugins/ansible/ansible/AnsibleRunnerContextBuilder.java b/src/main/groovy/com/rundeck/plugins/ansible/ansible/AnsibleRunnerContextBuilder.java
@@ -42,6 +42,12 @@ public class AnsibleRunnerContextBuilder {
     private final Map<String, Object> jobConf;
     private final Collection<INodeEntry> nodes;
     private final Collection<File> tempFiles;
+    /**
+     * Per-builder working directory (e.g. {@code ansible-exec-<executionId>-builder-<rand>/})
+     * created atomically with
+     * {@link Files#createTempDirectory(java.nio.file.Path, String, java.nio.file.attribute.FileAttribute[])}.
+     * EXCLUSIVELY owned by this builder; safe to delete recursively.
+     */
     private File executionSpecificDir;
 
     private AnsiblePluginGroup pluginGroup;
@@ -827,20 +833,25 @@ public void cleanupTempFiles() {
         }
         tempFiles.clear();
 
-        // Clean up execution-specific directory (including group_vars when inventory was generated/inline)
-        // Note: group_vars created alongside user-provided inventory files is cleaned up by AnsibleRunner
+        // Clean up the builder-specific working directory (including group_vars when inventory
+        // was generated/inline). Note: group_vars created alongside user-provided inventory files
+        // is cleaned up by AnsibleRunner.
+        // executionSpecificDir is an EXCLUSIVE per-builder directory (ansible-exec-<execId>-builder-<rand>/),
+        // so recursive deletion is always safe — no sibling builder shares files with it.
         if (executionSpecificDir != null && executionSpecificDir.exists()) {
             if (!getDebug()) {
-                log.debug("Cleaning up execution-specific directory: {}", executionSpecificDir.getAbsolutePath());
+                log.debug("Cleaning up builder-specific directory: {}", executionSpecificDir.getAbsolutePath());
                 if (!deleteDirectoryRecursively(executionSpecificDir)) {
-                    log.warn("Failed to completely delete execution-specific directory: {}", executionSpecificDir.getAbsolutePath());
+                    log.warn("Failed to completely delete builder-specific directory: {}", executionSpecificDir.getAbsolutePath());
                 } else {
-                    log.debug("Successfully deleted execution-specific directory: {}", executionSpecificDir.getAbsolutePath());
+                    log.debug("Successfully deleted builder-specific directory: {}", executionSpecificDir.getAbsolutePath());
                 }
             } else {
-                // In debug mode, the execution-specific directory is intentionally preserved for troubleshooting.
-                // Note: These directories will accumulate over time and may require periodic manual cleanup.
-                log.debug("Debug mode enabled; not cleaning up execution-specific directory: {}", executionSpecificDir.getAbsolutePath());
+                // In debug mode the per-builder directory is intentionally preserved for troubleshooting.
+                // Sibling directories of the same Rundeck execution share the ansible-exec-<execId>-* prefix
+                // for easy filtering. These directories will accumulate over time and may require periodic
+                // manual cleanup.
+                log.debug("Debug mode enabled; not cleaning up builder-specific directory: {}", executionSpecificDir.getAbsolutePath());
             }
         }
     }
@@ -1145,16 +1156,25 @@ public Boolean generateInventoryNodesAuth() {
     }
 
     /**
-     * Creates and returns an execution-specific temporary directory path.
-     * This ensures that each execution has its own isolated directory for inventory and group_vars,
-     * preventing conflicts when multiple workflow step executions run in parallel.
+     * Creates and returns a builder-specific temporary directory path.
+     *
+     * <p>Each builder receives a unique flat directory directly under the configured base tmp
+     * dir, named {@code ansible-exec-<executionId>-builder-<rand>/}. The directory is created
+     * atomically with {@link Files#createTempDirectory(java.nio.file.Path, String, java.nio.file.attribute.FileAttribute[])},
+     * which guarantees uniqueness without any coordination between sibling threads sharing the
+     * same Rundeck executionId.</p>
+     *
+     * <p>The {@code ansible-exec-<executionId>} prefix preserves the ability to filter all
+     * directories belonging to a given Rundeck execution (useful in debug mode), while keeping
+     * each directory exclusively owned by a single builder. Recursive cleanup of one builder's
+     * directory cannot affect siblings.</p>
      *
-     * @return The path to the execution-specific directory
+     * @return The absolute path to the builder-specific directory
      */
     String getExecutionSpecificTmpDir() {
         // Return cached directory if already created
         if (executionSpecificDir != null) {
-            log.debug("Using cached execution-specific directory: {}", executionSpecificDir.getAbsolutePath());
+            log.debug("Using cached builder-specific directory: {}", executionSpecificDir.getAbsolutePath());
             return executionSpecificDir.getAbsolutePath();
         }
 
@@ -1169,18 +1189,17 @@ String getExecutionSpecificTmpDir() {
         // Get base tmp directory
         String baseTmpDir = AnsibleUtil.getCustomTmpPathDir(framework);
 
-        // Create execution-specific directory
         if (executionId != null && !executionId.isEmpty()) {
-            executionSpecificDir = new File(baseTmpDir, "ansible-exec-" + executionId);
-            log.debug("Creating execution-specific directory: {}", executionSpecificDir.getAbsolutePath());
+            // Atomic + unique by construction. No coordination needed between sibling threads
+            // sharing the same Rundeck executionId. The executionId prefix lets operators filter
+            // all directories belonging to a given Rundeck execution with a single glob.
             try {
-                // Use Files.createDirectories() which is idempotent (safe to call if directory exists)
-                // and handles race conditions properly. Unlike mkdirs(), it doesn't return false
-                // when the directory already exists - it only throws IOException on actual failure.
-                Files.createDirectories(executionSpecificDir.toPath());
-                log.debug("Successfully ensured execution-specific directory exists: {}", executionSpecificDir.getAbsolutePath());
+                executionSpecificDir = Files
+                        .createTempDirectory(Paths.get(baseTmpDir), "ansible-exec-" + executionId + "-builder-")
+                        .toFile();
+                log.debug("Created builder-specific directory: {}", executionSpecificDir.getAbsolutePath());
             } catch (IOException e) {
-                String errorMsg = "Failed to create execution-specific directory: " + executionSpecificDir.getAbsolutePath();
+                String errorMsg = "Failed to create builder-specific directory under: " + baseTmpDir;
                 log.error(errorMsg, e);
                 throw new IllegalStateException(errorMsg, e);
             }
diff --git a/src/test/groovy/com/rundeck/plugins/ansible/ansible/AnsibleRunnerContextBuilderSpec.groovy b/src/test/groovy/com/rundeck/plugins/ansible/ansible/AnsibleRunnerContextBuilderSpec.groovy
@@ -3,13 +3,26 @@ package com.rundeck.plugins.ansible.ansible
 import com.dtolabs.rundeck.core.common.Framework
 import com.dtolabs.rundeck.core.common.INodeSet
 import com.dtolabs.rundeck.core.execution.ExecutionContext
+import com.dtolabs.rundeck.core.execution.ExecutionLogger
+import com.dtolabs.rundeck.core.utils.PropertyLookup
 import com.dtolabs.rundeck.plugins.step.PluginStepContext
 import com.rundeck.plugins.ansible.plugin.AnsiblePluginGroup
 import spock.lang.Specification
-import com.dtolabs.rundeck.core.execution.ExecutionLogger
+import spock.lang.TempDir
+
+import java.nio.file.Files
+import java.util.concurrent.ConcurrentLinkedQueue
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.CyclicBarrier
+import java.util.concurrent.ExecutorService
+import java.util.concurrent.Executors
+import java.util.concurrent.TimeUnit
 
 class AnsibleRunnerContextBuilderSpec extends Specification {
 
+    @TempDir
+    File baseTmpDir
+
     def "test plugin group"(){
         given:
 
@@ -80,4 +93,184 @@ class AnsibleRunnerContextBuilderSpec extends Specification {
         !contextBuilder.encryptExtraVars()
         contextBuilder.getPlaybookPath() == "path/to/playbook"
     }
+
+    // ----------------------------------------------------------------------------
+    // Race condition fix: getExecutionSpecificTmpDir() / cleanupTempFiles()
+    // See ansible-plugin-race-condition-context.md
+    // ----------------------------------------------------------------------------
+
+    /**
+     * Builds a fresh AnsibleRunnerContextBuilder wired so that
+     * AnsibleUtil.getCustomTmpPathDir(framework) resolves to {@code baseTmpDir} and
+     * the data context exposes the given {@code execId}.
+     */
+    private AnsibleRunnerContextBuilder newBuilder(String execId) {
+        def propertyLookup = Mock(PropertyLookup)
+        propertyLookup.getProperty("framework.tmp.dir") >> baseTmpDir.absolutePath
+
+        def framework = Mock(Framework)
+        framework.getPropertyLookup() >> propertyLookup
+        framework.hasProjectProperty(_, _) >> false
+        framework.hasProperty(_) >> false
+
+        def execContext = Mock(ExecutionContext)
+        execContext.getDataContext() >> ['job': ['execid': execId]]
+        execContext.getFrameworkProject() >> 'test-project'
+
+        def nodes = Mock(INodeSet)
+        nodes.getNodes() >> []
+
+        return new AnsibleRunnerContextBuilder(execContext, framework, nodes, [:])
+    }
+
+    /**
+     * Drops a fake "inventory" file inside the builder's working directory, mirroring
+     * what AnsibleInventoryBuilder.buildInventory() does in production.
+     */
+    private File seedInventoryFile(AnsibleRunnerContextBuilder builder) {
+        File dir = new File(builder.getExecutionSpecificTmpDir())
+        File inventory = File.createTempFile("ansible-inventory", ".json", dir)
+        inventory.text = '{"all":{"hosts":{}}}'
+        return inventory
+    }
+
+    def "two builders with the same executionId get distinct directories with the executionId prefix"() {
+        given:
+        def builderA = newBuilder("4242")
+        def builderB = newBuilder("4242")
+
+        when:
+        File dirA = new File(builderA.getExecutionSpecificTmpDir())
+        File dirB = new File(builderB.getExecutionSpecificTmpDir())
+
+        then: "both directories exist"
+        dirA.exists() && dirA.isDirectory()
+        dirB.exists() && dirB.isDirectory()
+
+        and: "they are different directories"
+        dirA.absolutePath != dirB.absolutePath
+
+        and: "they sit directly under the configured base tmp dir (no shared parent)"
+        dirA.parentFile.absolutePath == baseTmpDir.absolutePath
+        dirB.parentFile.absolutePath == baseTmpDir.absolutePath
+
+        and: "their names start with the ansible-exec-<execId>-builder- prefix for filtering"
+        dirA.name.startsWith("ansible-exec-4242-builder-")
+        dirB.name.startsWith("ansible-exec-4242-builder-")
+    }
+
+    def "directories of distinct executions can coexist under the same base tmp dir"() {
+        given:
+        def builderX = newBuilder("4242")
+        def builderY = newBuilder("9999")
+
+        when:
+        File dirX = new File(builderX.getExecutionSpecificTmpDir())
+        File dirY = new File(builderY.getExecutionSpecificTmpDir())
+
+        then:
+        dirX.parentFile.absolutePath == baseTmpDir.absolutePath
+        dirY.parentFile.absolutePath == baseTmpDir.absolutePath
+        dirX.name.startsWith("ansible-exec-4242-builder-")
+        dirY.name.startsWith("ansible-exec-9999-builder-")
+    }
+
+    def "cleanupTempFiles on one builder must not delete a sibling's inventory"() {
+        given:
+        def builderA = newBuilder("9001")
+        def builderB = newBuilder("9001")
+
+        File invA = seedInventoryFile(builderA)
+        File invB = seedInventoryFile(builderB)
+        File dirA = new File(builderA.getExecutionSpecificTmpDir())
+        File dirB = new File(builderB.getExecutionSpecificTmpDir())
+
+        expect:
+        invA.exists()
+        invB.exists()
+
+        when: "builder A finishes and cleans up"
+        builderA.cleanupTempFiles()
+
+        then: "A's directory and inventory are gone, but B's is intact"
+        !dirA.exists()
+        !invA.exists()
+        dirB.exists()
+        invB.exists()
+
+        when: "builder B finishes and cleans up"
+        builderB.cleanupTempFiles()
+
+        then: "B's directory is also gone"
+        !dirB.exists()
+        !invB.exists()
+    }
+
+    def "parallel builders never lose their inventory files before their own cleanup"() {
+        given:
+        int parallelism = 16
+        ExecutorService pool = Executors.newFixedThreadPool(parallelism)
+        CyclicBarrier barrier = new CyclicBarrier(parallelism)
+        CountDownLatch done = new CountDownLatch(parallelism)
+        ConcurrentLinkedQueue<String> failures = new ConcurrentLinkedQueue<>()
+
+        when:
+        parallelism.times {
+            pool.submit({
+                AnsibleRunnerContextBuilder builder = null
+                try {
+                    builder = newBuilder("55555")
+                    File workDir = new File(builder.getExecutionSpecificTmpDir())
+                    File inventory = File.createTempFile("ansible-inventory", ".json", workDir)
+                    inventory.text = '{"all":{"hosts":{}}}'
+
+                    // All threads have created their own dir+inventory; now hold them open
+                    // simultaneously to maximise the chance of a sibling's cleanup running while
+                    // ours is still alive.
+                    barrier.await(10, TimeUnit.SECONDS)
+
+                    if (!inventory.exists()) {
+                        failures.add("inventory disappeared while builder was alive: ${inventory.absolutePath}")
+                    }
+                    if (!workDir.exists()) {
+                        failures.add("work dir disappeared while builder was alive: ${workDir.absolutePath}")
+                    }
+                } catch (Throwable t) {
+                    failures.add("worker threw: ${t.message}")
+                } finally {
+                    builder?.cleanupTempFiles()
+                    done.countDown()
+                }
+            } as Runnable)
+        }
+
+        boolean finished = done.await(30, TimeUnit.SECONDS)
+        pool.shutdownNow()
+
+        then:
+        finished
+        failures.isEmpty()
+
+        and: "no leftover ansible-exec-55555-builder-* directories remain after cleanup"
+        baseTmpDir.listFiles({ f -> f.name.startsWith("ansible-exec-55555-builder-") } as FileFilter).length == 0
+    }
+
+    def "debug mode preserves the per-builder directory"() {
+        given:
+        def builder = newBuilder("debug-123")
+        // ansible-debug=true must short-circuit cleanup paths.
+        Map<String, Object> jobConf = (Map<String, Object>) builder.getJobConf()
+        jobConf.put("ansible-debug", "true")
+
+        File workDir = new File(builder.getExecutionSpecificTmpDir())
+        File inventory = seedInventoryFile(builder)
+
+        when:
+        builder.cleanupTempFiles()
+
+        then: "nothing is deleted in debug mode, mirroring legacy behaviour"
+        inventory.exists()
+        workDir.exists()
+        workDir.name.startsWith("ansible-exec-debug-123-builder-")
+    }
 }
