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.

Author: pashidlos

src/builds/builds.service.spec.ts

@@ -151,32 +151,137 @@ 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();
+    });
+  });
+
+  describe('deleteOldBuilds', () => {
+    const projectId = 'someProjectId';
+    const build1 = { ...build, id: 'build1', createdAt: new Date(Date.now() - 10000) };
+    const build2 = { ...build, id: 'build2', createdAt: new Date(Date.now() - 5000) };
+    const build3 = { ...build, id: 'build3', createdAt: new Date(Date.now()) };
+
+    it('should delete old builds and keep the specified number', async () => {
+      const buildFindManyMock = jest.fn().mockResolvedValueOnce([build1, build2, build3]);
+      const buildCountMock = jest.fn().mockResolvedValueOnce(3);
+      const removeMock = jest.fn().mockResolvedValue(undefined);
+      const loggerDebugMock = jest.fn();
+
+      service = await initService({ buildFindManyMock, buildCountMock });
+      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, build3]);
+      const buildCountMock = jest.fn().mockResolvedValueOnce(3);
+      const removeMock = jest.fn().mockResolvedValue(undefined);
+      const loggerDebugMock = jest.fn();
+
+      service = await initService({ buildFindManyMock, buildCountMock });
+      service.remove = removeMock;
+      (service as any).logger = { debug: loggerDebugMock };
+
+      const promise1 = service.deleteOldBuilds(projectId, 2);
+      const promise2 = service.deleteOldBuilds(projectId, 2);
+
+      expect(promise1).toBe(promise2); // Should return the same promise
+      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}.`);
+
+      // After completion, a new call should initiate a new deletion
+      const promise3 = service.deleteOldBuilds(projectId, 2);
+      expect(promise3).not.toBe(promise1);
+      await promise3;
+      expect(buildFindManyMock).toHaveBeenCalledTimes(2);
+    });
+
+    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);
+
+      // Check if the promise is removed from the internal map
+      expect((service as any).ongoingDeletions.has(projectId)).toBe(false);
     });
-    expect(testRunDeleteMock).toHaveBeenCalledWith(build.testRuns[0].id);
-    expect(eventBuildDeletedMock).toHaveBeenCalledWith(new BuildDto(build));
-    expect(buildDeleteMock).toHaveBeenCalledWith({
-      where: { id: build.id },
+
+    it('should remove the promise from the map after completion (failure)', async () => {
+      const buildFindManyMock = jest.fn().mockRejectedValueOnce(new Error('DB error'));
+      const buildCountMock = jest.fn().mockResolvedValueOnce(1);
+      const removeMock = jest.fn().mockResolvedValue(undefined);
+
+      service = await initService({ buildFindManyMock, buildCountMock });
+      service.remove = removeMock;
+
+      const projectId = 'testProject';
+      await expect(service.deleteOldBuilds(projectId, 0)).rejects.toThrow('DB error');
+
+      // Check if the promise is removed from the internal map even after an error
+      expect((service as any).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> {