Improve build removal and add concurrent deletion control

.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> {