From 5f34d74cd4d71317e170862b7dca9be6a3a479b5 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Wed, 16 Sep 2020 17:30:05 +0200
Subject: [PATCH] Schedule session index cleanup task only once. (#75661)

---
 .../session_management_service.test.ts        | 23 ++++++++-----------
 .../session_management_service.ts             | 18 +++++++++++++--
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/x-pack/plugins/security/server/session_management/session_management_service.test.ts b/x-pack/plugins/security/server/session_management/session_management_service.test.ts
index df528e3f97cb4..0328455fc8379 100644
--- a/x-pack/plugins/security/server/session_management/session_management_service.test.ts
+++ b/x-pack/plugins/security/server/session_management/session_management_service.test.ts
@@ -147,7 +147,9 @@ describe('SessionManagementService', () => {
       mockStatusSubject.next({ scheduleRetry: mockScheduleRetry });
       await nextTick();
       expect(mockSessionIndexInitialize).toHaveBeenCalledTimes(2);
-      expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(2);
+
+      // Session index task shouldn't be scheduled twice due to TM issue.
+      expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
 
       expect(mockScheduleRetry).not.toHaveBeenCalled();
     });
@@ -199,16 +201,8 @@ describe('SessionManagementService', () => {
       expect(mockTaskManager.get).toHaveBeenCalledWith(SESSION_INDEX_CLEANUP_TASK_NAME);
 
       expect(mockTaskManager.remove).not.toHaveBeenCalled();
-
-      expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
-      expect(mockTaskManager.ensureScheduled).toHaveBeenCalledWith({
-        id: SESSION_INDEX_CLEANUP_TASK_NAME,
-        taskType: SESSION_INDEX_CLEANUP_TASK_NAME,
-        scope: ['security'],
-        schedule: { interval: '3600s' },
-        params: {},
-        state: {},
-      });
+      // No need to schedule a task if Task Manager says it's already scheduled.
+      expect(mockTaskManager.ensureScheduled).not.toHaveBeenCalled();
     });
 
     it('schedules retry if index initialization fails', async () => {
@@ -224,11 +218,12 @@ describe('SessionManagementService', () => {
       expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
       expect(mockScheduleRetry).toHaveBeenCalledTimes(1);
 
-      // Still fails.
+      // Still fails, but cleanup task is scheduled already
+      mockTaskManager.get.mockResolvedValue({ schedule: { interval: '3600s' } } as any);
       mockStatusSubject.next({ scheduleRetry: mockScheduleRetry });
       await nextTick();
       expect(mockSessionIndexInitialize).toHaveBeenCalledTimes(2);
-      expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(2);
+      expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
       expect(mockScheduleRetry).toHaveBeenCalledTimes(2);
 
       // And finally succeeds, retry is not scheduled.
@@ -237,7 +232,7 @@ describe('SessionManagementService', () => {
       mockStatusSubject.next({ scheduleRetry: mockScheduleRetry });
       await nextTick();
       expect(mockSessionIndexInitialize).toHaveBeenCalledTimes(3);
-      expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(3);
+      expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
       expect(mockScheduleRetry).toHaveBeenCalledTimes(2);
     });
 
diff --git a/x-pack/plugins/security/server/session_management/session_management_service.ts b/x-pack/plugins/security/server/session_management/session_management_service.ts
index 6691b47638e27..60c0f7c23e959 100644
--- a/x-pack/plugins/security/server/session_management/session_management_service.ts
+++ b/x-pack/plugins/security/server/session_management/session_management_service.ts
@@ -47,6 +47,7 @@ export class SessionManagementService {
   private statusSubscription?: Subscription;
   private sessionIndex!: SessionIndex;
   private config!: ConfigType;
+  private isCleanupTaskScheduled = false;
 
   constructor(private readonly logger: Logger) {}
 
@@ -124,7 +125,12 @@ export class SessionManagementService {
 
     // Check if currently scheduled task is scheduled with the correct interval.
     const cleanupInterval = `${this.config.session.cleanupInterval.asSeconds()}s`;
-    if (currentTask && currentTask.schedule?.interval !== cleanupInterval) {
+    if (currentTask) {
+      if (currentTask.schedule?.interval === cleanupInterval) {
+        this.logger.debug('Session index cleanup task is already scheduled.');
+        return;
+      }
+
       this.logger.debug(
         'Session index cleanup interval has changed, the cleanup task will be rescheduled.'
       );
@@ -139,6 +145,13 @@ export class SessionManagementService {
           throw err;
         }
       }
+    } else if (this.isCleanupTaskScheduled) {
+      // WORKAROUND: This is a workaround for the Task Manager issue: https://github.com/elastic/kibana/issues/75501
+      // and should be removed as soon as this issue is resolved.
+      this.logger.error(
+        'Session index cleanup task has been already scheduled, but is missing in the task list for some reason. Please restart Kibana to automatically reschedule this task.'
+      );
+      return;
     }
 
     try {
@@ -151,10 +164,11 @@ export class SessionManagementService {
         state: {},
       });
     } catch (err) {
-      this.logger.error(`Failed to register session index cleanup task: ${err.message}`);
+      this.logger.error(`Failed to schedule session index cleanup task: ${err.message}`);
       throw err;
     }
 
+    this.isCleanupTaskScheduled = true;
     this.logger.debug('Successfully scheduled session index cleanup task.');
   }
 }