[backend] fix(manager-factory): full initialization at construction and disallow concurrent calls (#4680) #4696

RomuDeuxfois · 2026-01-08T13:12:36Z

Proposed changes

Testing Instructions

Step-by-step how to test
Environment or config notes

Related issues

Closes [bug] Built-in integrations manager first initialisation may be too late #4680

Checklist

I consider the submitted work as finished
I tested the code for its functionality
I wrote test cases for the relevant uses case
I added/update the relevant documentation (either on github or on notion)
Where necessary I refactored code to improve the overall quality
For bug fix -> I implemented a test that covers the bug

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

…nd disallow concurrent calls

codecov · 2026-01-08T13:27:26Z

Codecov Report

❌ Patch coverage is 52.94118% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.08%. Comparing base (5772942) to head (3494674).
⚠️ Report is 41 commits behind head on release/current.

Files with missing lines	Patch %	Lines
...in/java/io/openaev/integration/ManagerFactory.java	0.00%	7 Missing ⚠️
...aev/scheduler/jobs/ManagerIntegrationsSyncJob.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                  @@
##             release/current    #4696      +/-   ##
=====================================================
+ Coverage              52.74%   53.08%   +0.34%     
- Complexity              4119     4199      +80     
=====================================================
  Files                    963      972       +9     
  Lines                  28942    29374     +432     
  Branches                2152     2216      +64     
=====================================================
+ Hits                   15265    15594     +329     
- Misses                 12779    12877      +98     
- Partials                 898      903       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR addresses issue #4680 by ensuring the Manager class is fully initialized at construction time and preventing concurrent calls to critical sections. The key changes involve calling monitorIntegrations() immediately after Manager instantiation and adding a lock to prevent race conditions.

Key changes:

Manager initialization now includes monitorIntegrations() call in ManagerFactory
Added @Lock annotation to monitorIntegrations() method to prevent concurrent execution
Refactored LockResourceType enum to support configurable stripe counts per lock type

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
openaev-api/src/main/java/io/openaev/integration/ManagerFactory.java	Adds `monitorIntegrations()` call during Manager initialization to ensure full setup
openaev-api/src/main/java/io/openaev/integration/Manager.java	Adds `@Lock` annotation to `monitorIntegrations()` to prevent concurrent access
openaev-api/src/main/java/io/openaev/aop/lock/LockResourceType.java	Refactors enum to include stripe counts, adds MANAGER_FACTORY with single stripe for exclusive access
openaev-api/src/main/java/io/openaev/aop/lock/LockAspect.java	Updates initialization to use stripe counts from enum instead of hardcoded values
openaev-api/src/main/java/io/openaev/scheduler/jobs/EngineSyncExecutionJob.java	Removes unused `@LogExecutionTime` import

Copilot · 2026-01-09T11:58:59Z

openaev-api/src/main/java/io/openaev/integration/ManagerFactory.java

  public Manager getManager() throws Exception {
    if (this.manager == null) {
      this.manager = new Manager(factories);
+      this.manager.monitorIntegrations();
    }
    return this.manager;


The lazy initialization pattern with null check is not thread-safe. Multiple threads could pass the null check simultaneously, creating multiple Manager instances and calling monitorIntegrations() multiple times. Consider using double-checked locking with volatile, or making this method synchronized, or using a safer initialization pattern.

Copilot · 2026-01-09T11:58:59Z

openaev-api/src/main/java/io/openaev/aop/lock/LockResourceType.java

-  INJECT,
-  SECURITY_COVERAGE
+  INJECT(4096),
+  SECURITY_COVERAGE(4096),


The choice of a single stripe (1) for MANAGER_FACTORY should be documented. While this effectively creates an exclusive lock across all keys of this type, it's not immediately obvious why this differs from the 4096 stripes used for other lock types. Consider adding a comment explaining that this is intentional to ensure exclusive access during manager initialization.

Suggested change

SECURITY_COVERAGE(4096),

SECURITY_COVERAGE(4096),

// Use a single stripe to enforce an exclusive lock across all manager factory keys,

// ensuring only one manager initialization can proceed at a time.

antoinemzs

Approving to expedite, but solving the race condition on getManager() is a must do. Thanks!

antoinemzs · 2026-01-09T15:54:55Z

openaev-api/src/main/java/io/openaev/integration/ManagerFactory.java


  public Manager getManager() throws Exception {
    if (this.manager == null) {
      this.manager = new Manager(factories);
+      this.manager.monitorIntegrations();
    }
    return this.manager;


Possible race condition on getManager(), so there should be a lock here too like we discussed.

Yes, after thinking it through and looking at how the Manager is accessed, it felt odd to duplicate logic instead of clearly separating responsibilities.

I’ve updated the design and introduced a clearer approach, let me know what you think

The manager field is initialized only via initializeAndSync(), which is protected by a distributed lock.
The getManager() method is read-only and never performs initialization.

monitorIntegrations() is intended to be called exclusively by ManagerFactory.

…nd disallow concurrent calls (#4680)

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Copilot · 2026-01-10T10:37:52Z

openaev-api/src/main/java/io/openaev/integration/ManagerFactory.java

+  public Manager getManager() {
+    Manager local = manager;
+    if (local == null) {
+      throw new IllegalStateException("Manager not initialized yet");
+    }
+    return local;
+  }


The getManager() method now throws an IllegalStateException if called before initialization, but there's no mechanism to ensure initialization happens at application startup. This can lead to the same race condition the PR intends to fix, where ExecutionExecutorService calls getManager() before the scheduled job has run initializeAndSync(). Consider adding an @eventlistener(ApplicationReadyEvent.class) method to guarantee initialization happens early in the application lifecycle.

Copilot · 2026-01-10T10:37:52Z

openaev-api/src/main/java/io/openaev/scheduler/jobs/EngineSyncExecutionJob.java

  @Override
  @Transactional(rollbackFor = Exception.class)
-  @LogExecutionTime
  public void execute(JobExecutionContext jobExecutionContext) throws JobExecutionException {


The removal of @LogExecutionTime from EngineSyncExecutionJob appears unrelated to the stated purpose of fixing the manager initialization race condition. This should either be explained in the PR description or removed from this PR to keep changes focused.

Copilot · 2026-01-10T10:37:52Z

openaev-api/src/main/java/io/openaev/integration/ManagerFactory.java

+  @Transactional
+  @Lock(type = MANAGER_FACTORY, key = "manager-factory")
+  public void initializeAndSync() throws Exception {
+    if (manager == null) {
+      manager = new Manager(factories);
    }
-    return this.manager;
+    manager.monitorIntegrations();
  }


The new initializeAndSync() method lacks test coverage. Given that this method addresses a critical race condition bug, it should have comprehensive tests covering: 1) successful initialization on first call, 2) thread safety when called concurrently, 3) proper synchronization behavior with the Lock annotation, and 4) that subsequent calls don't re-initialize but do call monitorIntegrations().

Copilot · 2026-01-10T10:37:53Z

openaev-api/src/main/java/io/openaev/aop/lock/LockResourceType.java

-  SECURITY_COVERAGE
+  INJECT(4096),
+  SECURITY_COVERAGE(4096),
+  MANAGER_FACTORY(1);


Setting MANAGER_FACTORY to use only 1 stripe effectively serializes all calls to initializeAndSync(), which may be overly conservative. While this is acceptable for initialization, consider whether this lock configuration will cause unnecessary blocking in future use cases. Document why 1 stripe was chosen versus the 4096 used for other resource types.

Copilot · 2026-01-10T10:37:53Z

openaev-api/src/main/java/io/openaev/integration/ManagerFactory.java

+  public Manager getManager() {
+    Manager local = manager;
+    if (local == null) {
+      throw new IllegalStateException("Manager not initialized yet");


The error message "Manager not initialized yet" should be more informative to help developers diagnose the issue. Consider a message like "Manager has not been initialized. Ensure initializeAndSync() is called during application startup or by the scheduled ManagerIntegrationsSyncJob before accessing the manager."

Suggested change

throw new IllegalStateException("Manager not initialized yet");

throw new IllegalStateException(

"Manager has not been initialized. Ensure initializeAndSync() is called during "

+ "application startup or by the scheduled ManagerIntegrationsSyncJob before "

+ "accessing the manager.");

Copilot · 2026-01-10T10:37:53Z

openaev-api/src/main/java/io/openaev/aop/lock/LockResourceType.java

+
+  LockResourceType(int stripes) {
+    this.stripes = stripes;
+  }
+


The stripes() method and the enum constructor lack documentation. Add Javadoc explaining what "stripes" means in this context (the number of locks in the striped lock implementation) and why different resource types require different stripe counts.

Suggested change

LockResourceType(int stripes) {

this.stripes = stripes;

}

/**

* Creates a new {@link LockResourceType} with the given number of stripes.

*

* <p>In this context, a <em>stripe</em> represents one lock in a striped lock implementation.

* Each resource type can be protected by multiple underlying locks; the {@code stripes} value

* defines how many distinct locks are available. Using more stripes spreads lock acquisition

* across more lock instances, which can reduce contention and increase throughput for

* high-concurrency resources, at the cost of additional lock objects and slightly higher

* management overhead.

*

* @param stripes the number of locks (stripes) to use when guarding this resource type

*/

LockResourceType(int stripes) {

this.stripes = stripes;

}

/**

* Returns the number of stripes configured for this resource type.

*

* <p>The stripe count is the number of underlying locks used by the striped lock implementation

* when synchronizing access to resources of this type. Resource types that are accessed

* frequently or by many concurrent requests typically use a higher stripe count to reduce lock

* contention, while resource types with low contention can use fewer stripes to minimize

* overhead.

*

* @return the number of locks (stripes) associated with this resource type

*/

Copilot · 2026-01-10T10:37:53Z

openaev-api/src/main/java/io/openaev/integration/ManagerFactory.java

-@Transactional
 public class ManagerFactory {
  private final List<IntegrationFactory> factories;
  private Manager manager = null;


The manager field should be declared as volatile to ensure visibility guarantees when accessed from multiple threads. The current implementation uses a local variable copy in getManager() which suggests an attempt at implementing double-checked locking, but without volatile, this pattern is broken and can lead to returning a partially constructed Manager object or null even after initialization.

Suggested change

private Manager manager = null;

private volatile Manager manager = null;

RomuDeuxfois changed the base branch from master to release/current January 8, 2026 13:13

RomuDeuxfois changed the title ~~issue/4680~~ fix(manager-factory): full initialization at construction and disallow concurrent calls Jan 8, 2026

RomuDeuxfois linked an issue Jan 8, 2026 that may be closed by this pull request

[bug] Built-in integrations manager first initialisation may be too late #4680

Open

[backend] fix(manager-factory): full initialization at construction a…

4c0f3b1

…nd disallow concurrent calls

RomuDeuxfois force-pushed the issue/4680 branch from 1e80762 to 4c0f3b1 Compare January 8, 2026 13:14

RomuDeuxfois changed the title ~~fix(manager-factory): full initialization at construction and disallow concurrent calls~~ [backend] fix(manager-factory): full initialization at construction and disallow concurrent calls Jan 8, 2026

SamuelHassine requested a review from Copilot January 9, 2026 11:58

Copilot AI reviewed Jan 9, 2026

View reviewed changes

antoinemzs approved these changes Jan 9, 2026

View reviewed changes

[backend] fix(manager-factory): full initialization at construction a…

3494674

…nd disallow concurrent calls (#4680)

RomuDeuxfois force-pushed the issue/4680 branch from 5f034d7 to 3494674 Compare January 9, 2026 18:22

RomuDeuxfois changed the title ~~[backend] fix(manager-factory): full initialization at construction and disallow concurrent calls~~ [backend] fix(manager-factory): full initialization at construction and disallow concurrent calls (#4680) Jan 9, 2026

SamuelHassine requested a review from Copilot January 10, 2026 10:32

Copilot started reviewing on behalf of SamuelHassine January 10, 2026 10:33 View session

Copilot AI reviewed Jan 10, 2026

View reviewed changes

-      throw new IllegalStateException("Manager not initialized yet");
+      throw new IllegalStateException(
+          "Manager has not been initialized. Ensure initializeAndSync() is called during "
+              + "application startup or by the scheduled ManagerIntegrationsSyncJob before "
+              + "accessing the manager.");

-  LockResourceType(int stripes) {
-    this.stripes = stripes;
-  }
+  /**
+   * Creates a new {@link LockResourceType} with the given number of stripes.
+   *
+   * <p>In this context, a <em>stripe</em> represents one lock in a striped lock implementation.
+   * Each resource type can be protected by multiple underlying locks; the {@code stripes} value
+   * defines how many distinct locks are available. Using more stripes spreads lock acquisition
+   * across more lock instances, which can reduce contention and increase throughput for
+   * high-concurrency resources, at the cost of additional lock objects and slightly higher
+   * management overhead.
+   *
+   * @param stripes the number of locks (stripes) to use when guarding this resource type
+   */
+  LockResourceType(int stripes) {
+    this.stripes = stripes;
+  }
+  /**
+   * Returns the number of stripes configured for this resource type.
+   *
+   * <p>The stripe count is the number of underlying locks used by the striped lock implementation
+   * when synchronizing access to resources of this type. Resource types that are accessed
+   * frequently or by many concurrent requests typically use a higher stripe count to reduce lock
+   * contention, while resource types with low contention can use fewer stripes to minimize
+   * overhead.
+   *
+   * @return the number of locks (stripes) associated with this resource type
+   */

	private Manager manager = null;
	private volatile Manager manager = null;

[backend] fix(manager-factory): full initialization at construction and disallow concurrent calls (#4680) #4696

Are you sure you want to change the base?

[backend] fix(manager-factory): full initialization at construction and disallow concurrent calls (#4680) #4696

Uh oh!

Conversation

RomuDeuxfois commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Testing Instructions

Related issues

Checklist

Further comments

Uh oh!

codecov bot commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

antoinemzs left a comment

Choose a reason for hiding this comment

Uh oh!

antoinemzs Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

RomuDeuxfois Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RomuDeuxfois commented Jan 8, 2026 •

edited

Loading

codecov bot commented Jan 8, 2026 •

edited

Loading