feat(plugins): consolidate lifecycle lock ownership

fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/PluginRegistryService.java

@@ -44,13 +44,7 @@ public class PluginRegistryService {
     /** Built during {@link PostConstruct} and dynamic registration; mutable. */
     private final Map<String, BatchJobPlugin> pluginsByJobName = new LinkedHashMap<>();
 
-    /**
-     * Tracks classloader references for dynamically-registered plugins. Stored for
-     * Phase 4 cleanup (classloader close/reload). Key: job name, Value: classloader
-     * reference.
-     */
-    private final Map<String, Object> classloaderRefs = new HashMap<>();
-
+    /** Creates a registry backed by the shared Spring Batch infrastructure and job definitions. */
     public PluginRegistryService(
             List<BatchJobPlugin> plugins,
             JobRegistry jobRegistry,
@@ -175,15 +169,12 @@ public Set<String> getRegisteredJobNames() {
      * <p>Thread-safe: acquires the {@code pluginsByJobName} monitor before
      * mutation so register-into-map + register-into-JobRegistry are atomic.
      *
-     * @param plugin          the plugin instance to register (must have a unique
-     *                        job name)
-     * @param classLoaderRef  a reference to the classloader that loaded the
-     *                        plugin (stored for Phase 4 cleanup)
+     * @param plugin the plugin instance to register (must have a unique job name)
      * @throws PluginRegistrationException if a plugin with the same job name
      *                                     is already registered, or if
      *                                     {@code configureJob()} fails
      */
-    public void registerDynamicPlugin(BatchJobPlugin plugin, Object classLoaderRef) {
+    public void registerDynamicPlugin(BatchJobPlugin plugin) {
         String name = plugin.getJobName();
 
         synchronized (pluginsByJobName) {
@@ -223,8 +214,6 @@ public void registerDynamicPlugin(BatchJobPlugin plugin, Object classLoaderRef)
             }
 
             pluginsByJobName.put(name, plugin);
-            classloaderRefs.put(name, classLoaderRef);
-
             log.info(
                     "Dynamically registered plugin job '{}' from {}",
                     name,
@@ -236,12 +225,9 @@ public void registerDynamicPlugin(BatchJobPlugin plugin, Object classLoaderRef)
      * Unregisters a dynamically-registered plugin from the internal map and the
      * shared {@link JobRegistry}.
      *
-     * <p>Thread-safe: acquires the {@code pluginsByJobName} monitor. The
-     * classloader reference is <em>retained</em> in {@code classloaderRefs} for
-     * Phase 4 cleanup (graceful drain / close).
-     *
-     * <p>If the job is currently running, a warning is logged but execution is
-     * not interrupted — Phase 4 handles graceful drain.
+     * <p>Thread-safe: acquires the {@code pluginsByJobName} monitor. The map entry
+     * is removed only after {@link JobRegistry#unregister(String)} succeeds, so a
+     * registry failure preserves the plugin's in-memory registration.
      *
      * @param jobName the job name to unregister
      */
@@ -252,22 +238,10 @@ public void unregisterDynamicPlugin(String jobName) {
                 return;
             }
 
+            jobRegistry.unregister(jobName);
             pluginsByJobName.remove(jobName);
 
-            try {
-                jobRegistry.unregister(jobName);
-            } catch (Exception e) {
-                log.warn(
-                        "Failed to unregister job \"{}\" from JobRegistry: {}",
-                        jobName,
-                        e.getMessage());
-            }
-
-            classloaderRefs.remove(jobName);
-
-            log.info(
-                    "Unregistered plugin '{}', classloader reference removed",
-                    jobName);
+            log.info("Unregistered plugin '{}'", jobName);
         }
     }
 

fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoaderService.java

@@ -20,6 +20,7 @@
 import java.util.NoSuchElementException;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Supplier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.batch.core.repository.JobRepository;
@@ -59,7 +60,7 @@
    * winner's entry — leaking the winner's handle. {@link #loadJob(Long)} allocates an entry only
    * after confirming the definition exists, so unknown IDs cannot grow the map without bound.
    */
-  private final Map<Long, ReentrantLock> loadLocks = new ConcurrentHashMap<>();
+  private final Map<Long, LifecycleLockHolder> lifecycleLocks = new ConcurrentHashMap<>();
 
   public DynamicJobLoaderService(
       JobDefinitionRepository jobDefinitionRepository,
@@ -91,215 +92,262 @@
    */
   public LoadResult loadJob(Long definitionId) {
     // Gate lock allocation on existence so unknown IDs (e.g. POST /definitions/{id}/load with a
-    // bogus id) cannot grow loadLocks without bound. The authoritative existence check still runs
+    // bogus id) cannot grow lifecycleLocks without bound. The authoritative existence check still runs
     // inside doLoadJob under the lock, guarding against a definition deleted between the two reads.
     if (jobDefinitionRepository.findById(definitionId).isEmpty()) {
       throw new NoSuchElementException("Job definition not found for id: " + definitionId);
     }
-    ReentrantLock lock = loadLocks.computeIfAbsent(definitionId, k -> new ReentrantLock());
-    lock.lock();
+    return withDefinitionLock(definitionId, () -> doLoadJob(definitionId));
+  }
+
+  <T> T withDefinitionLock(Long definitionId, Supplier<T> action) {
+    LifecycleLockHolder holder =
+        lifecycleLocks.compute(
+            definitionId,
+            (id, current) -> {
+              LifecycleLockHolder selected = current == null ? new LifecycleLockHolder() : current;
+              selected.users++;
+              return selected;
+            });
+    holder.lock.lock();
     try {
-      return doLoadJob(definitionId);
+      return action.get();
     } finally {
-      lock.unlock();
+      holder.lock.unlock();
+      lifecycleLocks.computeIfPresent(
+          definitionId,
+          (id, current) -> --current.users == 0 ? null : current);
     }
   }
 
+  void withDefinitionLock(Long definitionId, Runnable action) {
+    withDefinitionLock(
+        definitionId,
+        () -> {
+          action.run();
+          return null;
+        });
+  }
+
+  private static final class LifecycleLockHolder {
+    private final ReentrantLock lock = new ReentrantLock();
+    private int users;
+  }
+
   /**
    * Performs the actual load. Always invoked while holding the per-definition lock acquired in
    * {@link #loadJob(Long)}, so the already-loaded guard and the classloader/registry writes are
    * atomic with respect to other loads of the same definition.
    */
   private LoadResult doLoadJob(Long definitionId) {
     JobDefinitionEntity entity =
         jobDefinitionRepository
             .findById(definitionId)
             .orElseThrow(
                 () ->
                     new NoSuchElementException(
                         "Job definition not found for id: " + definitionId));
 
     // Pre-condition: must be enabled
     if (!Boolean.TRUE.equals(entity.getEnabled())) {
       throw new IllegalStateException(
           "Job definition \"" + entity.getJobName() + "\" (id=" + definitionId + ") is disabled");
     }
 
     // Approval guard: only APPROVED definitions can be loaded
     if (!"APPROVED".equals(entity.getApprovalStatus())) {
       throw new IllegalStateException(
           "Job definition \""
               + entity.getJobName()
               + "\" (id="
               + definitionId
               + ") must be approved before loading. Current status: "
               + entity.getApprovalStatus());
     }
 
     // Optimistic guard: already loaded in THIS JVM's registry. The persisted
     // loadStatus can be a stale LOADED after a restart (the registry is
     // in-memory and resets), so consult the live registry, not the DB column.
     if (pluginRegistryService.getRegisteredJobNames().contains(entity.getJobName())) {
       throw new IllegalStateException(
           "Job \"" + entity.getJobName() + "\" (id=" + definitionId + ") is already loaded");
     }
 
     // Mark LOADING so concurrent attempts see in-flight state
     entity.setLoadStatus("LOADING");
     entity.setLoadError(null);
     jobDefinitionRepository.save(entity);
 
     long loadStartNanos = System.nanoTime();
     DynamicJobClassLoader classLoader = null;
+    boolean registered = false;
     try {
       // Validate JAR file exists
       File jarFile = new File(entity.getJarFilePath());
       if (!jarFile.exists()) {
         throw new JobLoadException(
             "JAR file not found: " + entity.getJarFilePath());
       }
 
       // Re-verify checksum: detect tampering that occurred after upload
       String storedChecksum = entity.getJarChecksum();
       String actualChecksum = ChecksumUtil.computeSha256(jarFile.toPath());
       if (!actualChecksum.equals(storedChecksum)) {
         entity.setLoadStatus(LoadResult.FAILED);
         entity.setLoadError(
             "Checksum mismatch: stored=" + storedChecksum + " actual=" + actualChecksum);
         jobDefinitionRepository.save(entity);
         throw new JobLoadException(
             "Checksum mismatch for job \""
                 + entity.getJobName()
                 + "\": stored="
                 + storedChecksum
                 + " actual="
                 + actualChecksum);
       }
 
       // Create parent-last classloader
       URL[] jarUrls = new URL[] {jarFile.toURI().toURL()};
       classLoader =
           new DynamicJobClassLoader(jarUrls, getClass().getClassLoader(), entity.getJobName());
 
       // Load the plugin main class
       Class<?> pluginClass = classLoader.loadClass(entity.getMainClassName());
 
       // Validate it implements BatchJobPlugin
       if (!BatchJobPlugin.class.isAssignableFrom(pluginClass)) {
         throw new JobLoadException(
             "Main class "
                 + entity.getMainClassName()
                 + " does not implement BatchJobPlugin");
       }
 
       // Instantiate and verify
       BatchJobPlugin plugin = (BatchJobPlugin) pluginClass.getDeclaredConstructor().newInstance();
 
       if (!plugin.getJobName().equals(entity.getJobName())) {
         throw new JobLoadException(
             "Plugin declares jobName=\""
                 + plugin.getJobName()
                 + "\" but definition expects \""
                 + entity.getJobName()
                 + "\"");
       }
 
       // Register through existing hook — registerDynamicPlugin() internally calls
       // plugin.configureJob(...), validates the Job, and registers in JobRegistry
-      pluginRegistryService.registerDynamicPlugin(plugin, classLoader);
+      pluginRegistryService.registerDynamicPlugin(plugin);
+      registered = true;
 
       // Track classloader for future unload
       classLoaders.put(definitionId, classLoader);
 
       // Persist success
       entity.setLoadStatus(LoadResult.LOADED);
       entity.setLoadError(null);
       jobDefinitionRepository.save(entity);
 
       log.info(
           "Loaded plugin '{}' (id={}) from {}",
           entity.getJobName(),
           definitionId,
           entity.getJarFilePath());
 
       auditService.logEvent(
           new AuditEvent(
               AuditEventType.JOB_LOADED,
               entity.getJobName(),
               AuditService.currentUserId(),
               "Job loaded from " + entity.getJarFilePath(),
               AuditEvent.SUCCESS,
               LocalDateTime.now()));
 
       long durationMs =
           java.util.concurrent.TimeUnit.NANOSECONDS.toMillis(
               System.nanoTime() - loadStartNanos);
       pluginMetrics.incrementLoad();
       pluginMetrics.recordLoadDuration(durationMs);
 
       return new LoadResult(
           entity.getJobName(), LoadResult.LOADED, "Successfully loaded");
 
-    } catch (NoSuchElementException | IllegalStateException e) {
-      // Re-throw without wrapping — these are validation failures, not load errors
-      // Reset status from LOADING back to previous
-      entity.setLoadStatus(getPreviousLoadStatus(entity, definitionId));
-      jobDefinitionRepository.save(entity);
-      auditService.logEvent(
-          new AuditEvent(
-              AuditEventType.JOB_LOADED,
-              entity.getJobName(),
-              AuditService.currentUserId(),
-              "Load rejected: " + e.getMessage(),
-              AuditEvent.FAILURE,
-              LocalDateTime.now()));
-      throw e;
     } catch (Exception e) {
-      // Mark FAILED and persist the error
-      entity.setLoadStatus(LoadResult.FAILED);
-      entity.setLoadError(e.getMessage());
-      jobDefinitionRepository.save(entity);
-
-      // Clean up any partial classloader
-      if (classLoader != null) {
+      boolean canReleaseClassLoader = true;
+      if (registered) {
         try {
-          classLoader.cleanup();
-        } catch (Exception cleanupEx) {
-          log.warn("Failed to clean up classloader for definition {}: {}", definitionId, cleanupEx.getMessage());
+          pluginRegistryService.unregisterDynamicPlugin(entity.getJobName());
+        } catch (Exception rollbackEx) {
+          canReleaseClassLoader = false;
+          e.addSuppressed(rollbackEx);
+          log.error(
+              "Failed to roll back plugin registration for definition {}: {}",
+              definitionId,
+              rollbackEx.getMessage(),
+              rollbackEx);
+        }
+      }
+
+      if (canReleaseClassLoader) {
+        DynamicJobClassLoader ownedClassLoader = classLoaders.remove(definitionId);
+        DynamicJobClassLoader classLoaderToClean =
+            ownedClassLoader != null ? ownedClassLoader : classLoader;
+        if (classLoaderToClean != null) {
+          try {
+            classLoaderToClean.cleanup();
+          } catch (Exception cleanupEx) {
+            e.addSuppressed(cleanupEx);
+            log.warn(
+                "Failed to clean up classloader for definition {}: {}",
+                definitionId,
+                cleanupEx.getMessage(),
+                cleanupEx);
+          }
         }
       }
-      classLoaders.remove(definitionId);
+
+      entity.setLoadStatus(LoadResult.FAILED);
+      entity.setLoadError(e.getMessage());
+      try {
+        jobDefinitionRepository.save(entity);
+      } catch (Exception persistenceEx) {
+        e.addSuppressed(persistenceEx);
+        log.error(
+            "Failed to persist FAILED status for definition {}: {}",
+            definitionId,
+            persistenceEx.getMessage(),
+            persistenceEx);
+      }
 
       log.error("Failed to load plugin for definition {}: {}", definitionId, e.getMessage(), e);
 
       auditService.logEvent(
           new AuditEvent(
               AuditEventType.JOB_LOADED,
               entity.getJobName(),
               AuditService.currentUserId(),
               "Load failed: " + e.getMessage(),
               AuditEvent.FAILURE,
               LocalDateTime.now()));
 
       if (e instanceof JobLoadException) {
         throw (JobLoadException) e;
       }
       throw new JobLoadException(
           "Failed to load job \"" + entity.getJobName() + "\": " + e.getMessage(), e);
     }
   }
 
   /**
    * Unloads a previously-loaded job: unregisters from the registry, closes the classloader, and
    * updates the database status.
    *
    * @param definitionId the database ID of the job definition to unload
    * @param force if {@code true}, skip the running-execution guard
    * @return a {@link LoadResult} describing the outcome
    * @throws NoSuchElementException if the definition does not exist
    * @throws JobUnloadConflictException if the job has running executions and {@code force} is
    *     {@code false}
    */
   public LoadResult unloadJob(Long definitionId, boolean force) {
     long unloadStartNanos = System.nanoTime();
 
@@ -440,16 +488,4 @@
     return results;
   }
 
-  /**
-   * Determines the previous load status for rollback when a pre-condition validation fails after
-   * LOADING was already set.
-   */
-  private String getPreviousLoadStatus(JobDefinitionEntity entity, Long definitionId) {
-    // If we already had a classloader, it means we were LOADED before this attempt
-    if (classLoaders.containsKey(definitionId)) {
-      return LoadResult.LOADED;
-    }
-    // Otherwise revert to UNLOADED (the most common prior state for a fresh load)
-    return LoadResult.UNLOADED;
-  }
 }

fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/PluginRegistryServiceTest.java

@@ -9,6 +9,7 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -120,33 +121,33 @@ void configureJobThrows_wrapsInPluginRegistrationException_withPluginClassAndCau
     void registerDynamicPlugin_addsToMapAndJobRegistry() throws Exception {
         Job job = stubJob("dynamicJob");
         BatchJobPlugin plugin = stubPlugin("dynamicJob", job);
-        Object classLoaderRef = new Object();
 
         PluginRegistryService service = new PluginRegistryService(
                 List.of(), jobRegistry, jobRepository, transactionManager,
                 applicationContext, jobDefinitionRepository);
 
-        service.registerDynamicPlugin(plugin, classLoaderRef);
+        service.registerDynamicPlugin(plugin);
 
         assertTrue(service.getRegisteredJobNames().contains("dynamicJob"));
         assertTrue(service.getPlugin("dynamicJob").isPresent());
         verify(jobRegistry).register(job);
     }
 
     @Test
-    void registerDynamicPlugin_tracksClassloaderReference() throws Exception {
+    void registerDynamicPlugin_doesNotRetainClassloaderOwnership() throws Exception {
         Job job = stubJob("dynamicJob");
         BatchJobPlugin plugin = stubPlugin("dynamicJob", job);
-        Object classLoaderRef = new Object();
 
         PluginRegistryService service = new PluginRegistryService(
                 List.of(), jobRegistry, jobRepository, transactionManager,
                 applicationContext, jobDefinitionRepository);
 
-        service.registerDynamicPlugin(plugin, classLoaderRef);
+        service.registerDynamicPlugin(plugin);
 
-        // getPluginBySource confirms it's registered as classpath
         assertEquals("CLASSPATH", service.getPluginBySource("dynamicJob"));
+        assertThrows(
+                NoSuchFieldException.class,
+                () -> PluginRegistryService.class.getDeclaredField("classloaderRefs"));
     }
 
     @Test
@@ -160,11 +161,11 @@ void registerDynamicPlugin_throwsOnDuplicate() throws Exception {
                 List.of(), jobRegistry, jobRepository, transactionManager,
                 applicationContext, jobDefinitionRepository);
 
-        service.registerDynamicPlugin(plugin1, new Object());
+        service.registerDynamicPlugin(plugin1);
 
         PluginRegistrationException ex = assertThrows(
                 PluginRegistrationException.class,
-                () -> service.registerDynamicPlugin(plugin2, new Object()));
+                () -> service.registerDynamicPlugin(plugin2));
 
         assertTrue(ex.getMessage().contains("dynamicJob"),
                 "message should contain the conflicting job name");
@@ -179,7 +180,7 @@ void unregisterDynamicPlugin_removesFromMapAndJobRegistry() throws Exception {
                 List.of(), jobRegistry, jobRepository, transactionManager,
                 applicationContext, jobDefinitionRepository);
 
-        service.registerDynamicPlugin(plugin, new Object());
+        service.registerDynamicPlugin(plugin);
         service.unregisterDynamicPlugin("dynamicJob");
 
         assertFalse(service.getRegisteredJobNames().contains("dynamicJob"));
@@ -188,20 +189,24 @@ void unregisterDynamicPlugin_removesFromMapAndJobRegistry() throws Exception {
     }
 
     @Test
-    void unregisterDynamicPlugin_retainsClassloaderReference() throws Exception {
+    void unregisterDynamicPlugin_propagatesRegistryFailureAndPreservesPluginMap() throws Exception {
         Job job = stubJob("dynamicJob");
         BatchJobPlugin plugin = stubPlugin("dynamicJob", job);
-        Object classLoaderRef = new Object();
 
         PluginRegistryService service = new PluginRegistryService(
                 List.of(), jobRegistry, jobRepository, transactionManager,
                 applicationContext, jobDefinitionRepository);
 
-        service.registerDynamicPlugin(plugin, classLoaderRef);
-        service.unregisterDynamicPlugin("dynamicJob");
+        service.registerDynamicPlugin(plugin);
+        RuntimeException failure = new RuntimeException("registry unavailable");
+        doThrow(failure).when(jobRegistry).unregister("dynamicJob");
 
-        // Plugin is unregistered from the map but the classloader ref is retained
-        assertFalse(service.getRegisteredJobNames().contains("dynamicJob"));
+        RuntimeException thrown =
+                assertThrows(RuntimeException.class, () -> service.unregisterDynamicPlugin("dynamicJob"));
+
+        assertEquals(failure, thrown);
+        assertTrue(service.getRegisteredJobNames().contains("dynamicJob"));
+        assertTrue(service.getPlugin("dynamicJob").isPresent());
     }
 
     @Test