Improve build removal and add concurrent deletion control (#333)

* Update builds.service.spec.ts

Improve build removal and add concurrent deletion control

Enhanced the remove method to handle undefined IDs gracefully and log warnings. Refactored deleteOldBuilds to prevent concurrent deletions for the same project by tracking ongoing operations, ensuring only one deletion runs per project at a time, and added comprehensive tests for these behaviors.

* Update workflow.yml

Author: pashidlos

.github/workflows/workflow.yml

@@ -59,7 +59,7 @@ jobs:
         run: docker compose -f docker-compose.yml -f docker-compose.ldap.yml down
 
       - name: SonarCloud Scan
-        uses: SonarSource/sonarqube-scan-action@v5
+        uses: SonarSource/sonarqube-scan-action@v6
         env:
           SONAR_TOKEN: ${{ secrets.SONARCLOUD_TOKEN }}
 

src/builds/builds.service.spec.ts

@@ -151,32 +151,128 @@ describe('BuildsService', () => {
     });
   });
 
-  it('delete', async () => {
-    const buildFindUniqueMock = jest.fn().mockResolvedValueOnce(build);
-    const buildFindManyMock = jest.fn().mockImplementation(() => Promise.resolve(build));
-    const buildDeleteMock = jest.fn().mockImplementation(() => Promise.resolve(build));
-    const testRunDeleteMock = jest.fn();
-    const eventBuildDeletedMock = jest.fn();
-    service = await initService({
-      buildFindUniqueMock,
-      buildDeleteMock,
-      testRunDeleteMock,
-      eventBuildDeletedMock,
-      buildFindManyMock,
+  describe('remove', () => {
+    it('should remove a build successfully', async () => {
+      const buildFindUniqueMock = jest.fn().mockResolvedValueOnce(build);
+      const buildFindManyMock = jest.fn().mockImplementation(() => Promise.resolve(build));
+      const buildDeleteMock = jest.fn().mockImplementation(() => Promise.resolve(build));
+      const testRunDeleteMock = jest.fn();
+      const eventBuildDeletedMock = jest.fn();
+      service = await initService({
+        buildFindUniqueMock,
+        buildDeleteMock,
+        testRunDeleteMock,
+        eventBuildDeletedMock,
+        buildFindManyMock,
+      });
+
+      await service.remove(build.id);
+
+      expect(buildFindUniqueMock).toHaveBeenCalledWith({
+        where: { id: build.id },
+        include: {
+          testRuns: true,
+        },
+      });
+      expect(testRunDeleteMock).toHaveBeenCalledWith(build.testRuns[0].id);
+      expect(eventBuildDeletedMock).toHaveBeenCalledWith(new BuildDto(build));
+      expect(buildDeleteMock).toHaveBeenCalledWith({
+        where: { id: build.id },
+      });
     });
 
-    await service.remove(build.id);
+    it('should handle undefined ID gracefully', async () => {
+      const buildFindUniqueMock = jest.fn();
+      const loggerWarnMock = jest.fn();
+      service = await initService({ buildFindUniqueMock });
+      (service as any).logger = { warn: loggerWarnMock }; // Mock the logger
 
-    expect(buildFindUniqueMock).toHaveBeenCalledWith({
-      where: { id: build.id },
-      include: {
-        testRuns: true,
-      },
+      await service.remove(undefined);
+
+      expect(loggerWarnMock).toHaveBeenCalledWith('Attempted to remove build with undefined ID.');
+      expect(buildFindUniqueMock).not.toHaveBeenCalled();
     });
-    expect(testRunDeleteMock).toHaveBeenCalledWith(build.testRuns[0].id);
-    expect(eventBuildDeletedMock).toHaveBeenCalledWith(new BuildDto(build));
-    expect(buildDeleteMock).toHaveBeenCalledWith({
-      where: { id: build.id },
+  });
+
+  describe('deleteOldBuilds', () => {
+    const projectId = 'someProjectId';
+    const build1 = { ...buildDto, id: 'build1', createdAt: new Date(Date.now() - 10000) };
+    const build2 = { ...buildDto, id: 'build2', createdAt: new Date(Date.now() - 5000) };
+
+    it('should delete old builds and keep the specified number', async () => {
+      const buildFindManyMock = jest.fn().mockResolvedValueOnce([build1, build2]);
+      const buildCountMock = jest.fn().mockResolvedValueOnce(2);
+      const buildFindUniqueMock = jest.fn().mockResolvedValueOnce(build1).mockResolvedValueOnce(build2);
+      mocked(BuildDto)
+        .mockReturnValueOnce(build1 as MockedObject<BuildDto>)
+        .mockReturnValueOnce(build2 as MockedObject<BuildDto>);
+      const testRunFindManyMock = jest.fn().mockResolvedValue([]);
+      const removeMock = jest.fn().mockResolvedValue(undefined);
+      const loggerDebugMock = jest.fn();
+
+      service = await initService({ buildFindManyMock, buildCountMock, buildFindUniqueMock, testRunFindManyMock });
+      service.remove = removeMock;
+      (service as any).logger = { debug: loggerDebugMock };
+
+      await service.deleteOldBuilds(projectId, 2);
+
+      expect(buildFindManyMock).toHaveBeenCalledWith({
+        where: { projectId },
+        take: undefined,
+        skip: 1,
+        orderBy: { createdAt: 'desc' },
+      });
+      expect(removeMock).toHaveBeenCalledTimes(2);
+      expect(removeMock).toHaveBeenCalledWith(build1.id);
+      expect(removeMock).toHaveBeenCalledWith(build2.id);
+      expect(loggerDebugMock).toHaveBeenCalledWith(
+        `Starting to delete old builds for project ${projectId}, keeping 2 builds.`
+      );
+      expect(loggerDebugMock).toHaveBeenCalledWith(`Finished deleting old builds for project ${projectId}.`);
+    });
+
+    it('should handle concurrent calls for the same project by returning the existing promise', async () => {
+      const buildFindManyMock = jest.fn().mockResolvedValueOnce([build1, build2]);
+      const buildCountMock = jest.fn().mockResolvedValueOnce(2);
+      const removeMock = jest.fn().mockResolvedValue(undefined);
+      const testRunFindManyMock = jest.fn().mockResolvedValue([]);
+      const buildFindUniqueMock = jest.fn().mockResolvedValueOnce(build1).mockResolvedValueOnce(build2);
+      mocked(BuildDto)
+        .mockReturnValueOnce(build1 as MockedObject<BuildDto>)
+        .mockReturnValueOnce(build2 as MockedObject<BuildDto>);
+      const loggerDebugMock = jest.fn();
+
+      service = await initService({ buildFindManyMock, buildCountMock, testRunFindManyMock, buildFindUniqueMock });
+      service.remove = removeMock;
+      (service as any).logger = { debug: loggerDebugMock };
+
+      const promise1 = service.deleteOldBuilds(projectId, 2);
+      const promise2 = service.deleteOldBuilds(projectId, 2);
+
+      expect(promise1).toStrictEqual(promise2);
+      expect(loggerDebugMock).toHaveBeenCalledWith(
+        `Deletion for project ${projectId} is already in progress. Returning existing promise.`
+      );
+
+      await promise1; // Wait for the deletion to complete
+
+      expect(buildFindManyMock).toHaveBeenCalledTimes(1); // Only called once
+      expect(removeMock).toHaveBeenCalledTimes(2);
+      expect(loggerDebugMock).toHaveBeenCalledWith(`Finished deleting old builds for project ${projectId}.`);
+    });
+
+    it('should remove the promise from the map after completion (success)', async () => {
+      const buildFindManyMock = jest.fn().mockResolvedValueOnce([build1]);
+      const buildCountMock = jest.fn().mockResolvedValueOnce(1);
+      const removeMock = jest.fn().mockResolvedValue(undefined);
+
+      service = await initService({ buildFindManyMock, buildCountMock });
+      service.remove = removeMock;
+
+      const projectId = 'testProject';
+      await service.deleteOldBuilds(projectId, 0);
+
+      expect(service['ongoingDeletions'].has(projectId)).toBe(false);
     });
   });
 

src/builds/builds.service.ts

@@ -10,6 +10,7 @@ import { ModifyBuildDto } from './dto/build-modify.dto';
 @Injectable()
 export class BuildsService {
   private readonly logger: Logger = new Logger(BuildsService.name);
+  private readonly ongoingDeletions = new Map<string, Promise<void>>();
 
   constructor(
     private prismaService: PrismaService,
@@ -60,6 +61,10 @@ export class BuildsService {
   }
 
   async remove(id: string): Promise<Build> {
+    if (!id) {
+      this.logger.warn(`Attempted to remove build with undefined ID.`);
+      return;
+    }
     this.logger.debug(`Going to remove Build ${id}`);
 
     const build = await this.prismaService.build.findUnique({
@@ -93,13 +98,30 @@ export class BuildsService {
     return build;
   }
 
-  async deleteOldBuilds(projectId: string, keepBuilds: number) {
-    keepBuilds = keepBuilds < 2 ? keepBuilds : keepBuilds - 1;
-    this.findMany(projectId, undefined, keepBuilds).then((buildList) => {
-      buildList.data.forEach((eachBuild) => {
-        this.remove(eachBuild.id);
-      });
+  async deleteOldBuilds(projectId: string, keepBuilds: number): Promise<void> {
+    if (this.ongoingDeletions.has(projectId)) {
+      this.logger.debug(`Deletion for project ${projectId} is already in progress. Returning existing promise.`);
+      return this.ongoingDeletions.get(projectId);
+    }
+
+    const deletionPromise = (async () => {
+      this.logger.debug(`Starting to delete old builds for project ${projectId}, keeping ${keepBuilds} builds.`);
+      keepBuilds = keepBuilds < 2 ? keepBuilds : keepBuilds - 1;
+      const buildList = await this.findMany(projectId, undefined, keepBuilds);
+      for (const eachBuild of buildList.data) {
+        await this.remove(eachBuild.id);
+      }
+      this.logger.debug(`Finished deleting old builds for project ${projectId}.`);
+    })();
+
+    this.ongoingDeletions.set(projectId, deletionPromise);
+
+    deletionPromise.finally(() => {
+      this.ongoingDeletions.delete(projectId);
+      this.logger.debug(`Deletion promise for project ${projectId} resolved/rejected and removed from map.`);
     });
+
+    return deletionPromise;
   }
 
   async approve(id: string, merge: boolean): Promise<void> {