fix: Concurrent modification issue with pending migrations feature flags (#27926)

## Description
In this code change, a separate loop has been introduced to handle the
removal of specific flags from a map to avoid encountering a
`ConcurrentModificationException` that can occur when attempting to
remove an entry from a map while iterating over it directly. This new
loop iterates through a collection of `featureFlagsToBeRemoved`,
removing each corresponding flag from the mentioned maps, ensuring safe
removal without disrupting the ongoing iteration process.

#### PR fixes following issue(s)
Fixes https://github.com/appsmithorg/appsmith-ee/issues/2608

#### Type of change
- Bug fix (non-breaking change which fixes an issue)

## Testing
>
#### How Has This Been Tested?
- [x] Manual
- [x] JUnit
- [ ] Jest
- [ ] Cypress
>
>

## Checklist:
#### Dev activity
- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] PR is being merged under a feature flag


#### QA activity:
- [ ] [Speedbreak
features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-)
have been covered
- [ ] Test plan covers all impacted features and [areas of
interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-)
- [ ] Test plan has been peer reviewed by project stakeholders and other
QA members
- [ ] Manually tested functionality on DP
- [ ] We had an implementation alignment call with stakeholders post QA
Round 2
- [ ] Cypress test cases have been added and approved by SDET/manual QA
- [ ] Added `Test Plan Approved` label after Cypress tests were reviewed
- [ ] Added `Test Plan Approved` label after JUnit tests were reviewed

app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java

@@ -14,7 +14,9 @@ import reactor.core.publisher.Mono;
 
 import java.time.Instant;
 import java.time.temporal.ChronoUnit;
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import static com.appsmith.server.constants.FeatureMigrationType.DISABLE;
@@ -136,6 +138,9 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel
                 } catch (Exception e) {
                     // Ignore IllegalArgumentException as all the feature flags are not added on
                     // server side
+                    if (!(e instanceof IllegalArgumentException)) {
+                        log.error("Error while parsing the feature flag {} with value {}", key, value, e);
+                    }
                 }
             }
         });
@@ -157,13 +162,13 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel
         // featuresWithPendingMigrationDB       => {feature1 : enable, feature2 : disable}
         // latestFeatureDiffsWithMigrationType  => {feature1 : enable, feature2 : enable, feature3 : disable}
         // updatedFlagsForMigrations            => {feature1 : enable, feature3 : disable}
-
+        List<FeatureFlagEnum> featureFlagsToBeRemoved = new ArrayList<>();
         updatedFlagsForMigrations.forEach((featureFlagEnum, featureMigrationType) -> {
             if (latestFeatureDiffsWithMigrationType.containsKey(featureFlagEnum)
                     && !featureMigrationType.equals(latestFeatureDiffsWithMigrationType.get(featureFlagEnum))) {
                 /*
                 Scenario when the migrations will be blocked on user input and may end up in a case where we just have
-                to remove the entry as migration it's no longer needed:
+                to remove the entry as migration is no longer needed:
                     Step 1: Feature gets enabled by adding a valid licence and enable migration gets registered
                     Step 2: License expires which results in feature getting disabled so migration entry gets registered
                             with disable type (This will happen via cron to check the license status)
@@ -172,10 +177,16 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel
                     Step 4: User adds the valid key or renews the subscription again which results in enabling the
                             feature and ends up in nullifying the effect for step 2
                  */
-                updatedFlagsForMigrations.remove(featureFlagEnum);
-                latestFeatureDiffsWithMigrationType.remove(featureFlagEnum);
+                featureFlagsToBeRemoved.add(featureFlagEnum);
             }
         });
+
+        // Added a separate loop to remove the flags as we cannot remove the entry while iterating over the map to
+        // avoid the ConcurrentModificationException
+        featureFlagsToBeRemoved.forEach(featureFlagEnum -> {
+            updatedFlagsForMigrations.remove(featureFlagEnum);
+            latestFeatureDiffsWithMigrationType.remove(featureFlagEnum);
+        });
         // Add the latest flags which were not part of earlier check.
         updatedFlagsForMigrations.putAll(latestFeatureDiffsWithMigrationType);
         return updatedFlagsForMigrations;

app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/FeatureFlagMigrationHelperTest.java

@@ -226,4 +226,50 @@ class FeatureFlagMigrationHelperTest {
                 })
                 .verifyComplete();
     }
+
+    @Test
+    void
+            getUpdatedFlagsWithPendingMigration_diffForExistingAndLatestFlag_sameFlagIsFlippedAsPerDBState_flagGetsRemovedFromPendingMigrationList() {
+
+        // Mock DB state to have the feature flag in pending migration list with DISABLE status which means the feature
+        // flag flipped from true to false
+        Tenant defaultTenant = new Tenant();
+        defaultTenant.setId(UUID.randomUUID().toString());
+        TenantConfiguration tenantConfiguration = new TenantConfiguration();
+        tenantConfiguration.setFeaturesWithPendingMigration(Map.of(TENANT_TEST_FEATURE, DISABLE));
+        defaultTenant.setTenantConfiguration(tenantConfiguration);
+
+        // Mock CS calls to fetch the feature flags to have the feature flag in pending migration list with ENABLE
+        // status
+        // This means the feature flag flipped from false to true again with latest check
+        CachedFeatures existingCachedFeatures = new CachedFeatures();
+        Map<String, Boolean> featureMap = new HashMap<>();
+        featureMap.put(TENANT_TEST_FEATURE.name(), false);
+        existingCachedFeatures.setFeatures(featureMap);
+        existingCachedFeatures.setRefreshedAt(Instant.now().minus(1, ChronoUnit.DAYS));
+
+        CachedFeatures latestCachedFeatures = new CachedFeatures();
+        Map<String, Boolean> latestFeatureMap = new HashMap<>();
+        latestFeatureMap.put(TENANT_TEST_FEATURE.name(), true);
+        latestCachedFeatures.setFeatures(latestFeatureMap);
+        latestCachedFeatures.setRefreshedAt(Instant.now());
+
+        Mockito.when(cacheableFeatureFlagHelper.fetchCachedTenantFeatures(any()))
+                .thenReturn(Mono.just(existingCachedFeatures))
+                .thenReturn(Mono.just(latestCachedFeatures));
+
+        Mockito.when(cacheableFeatureFlagHelper.evictCachedTenantFeatures(any()))
+                .thenReturn(Mono.empty());
+
+        Mono<Map<FeatureFlagEnum, FeatureMigrationType>> getUpdatedFlagsWithPendingMigration =
+                featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(defaultTenant);
+
+        StepVerifier.create(getUpdatedFlagsWithPendingMigration)
+                .assertNext(featureFlagEnumFeatureMigrationTypeMap -> {
+                    // As the feature flag is flipped back to true, the feature flag should be removed from the pending
+                    // migration entries as the migration is no longer required
+                    assertThat(featureFlagEnumFeatureMigrationTypeMap).isEmpty();
+                })
+                .verifyComplete();
+    }
 }