diff --git a/__tests__/cleanup-integration.test.ts b/__tests__/cleanup-integration.test.ts index a7951ebbf..2b462de3f 100644 --- a/__tests__/cleanup-integration.test.ts +++ b/__tests__/cleanup-integration.test.ts @@ -1,10 +1,20 @@ import * as core from '@actions/core' import Docker from 'dockerode' import {ImageService} from '../src/image-service' -import {integration, delay} from './helpers' -import {run, cleanupOldImageVersions} from '../src/cleanup' +import {integration} from './helpers' import {PROXY_IMAGE_NAME, digestName} from '../src/docker-tags' +let run: typeof import('../src/cleanup').run +let cleanupOldImageVersions: typeof import('../src/cleanup').cleanupOldImageVersions + +beforeAll(async () => { + process.env.DEPENDABOT_DISABLE_CLEANUP = '1' + const cleanup = await import('../src/cleanup') + run = cleanup.run + cleanupOldImageVersions = cleanup.cleanupOldImageVersions + delete process.env.DEPENDABOT_DISABLE_CLEANUP +}) + integration('run', () => { beforeEach(async () => { jest.spyOn(core, 'error').mockImplementation(jest.fn()) @@ -65,9 +75,6 @@ integration('cleanupOldImageVersions', () => { expect(initialImages.length).toEqual(2) await cleanupOldImageVersions(docker, currentImage) - // The Docker API seems to ack the removal before it is carried out, so let's wait briefly to ensure - // the verification query doesn't race the deletion - await delay(200) const remainingImages = await docker.listImages(imageOptions) expect(remainingImages.length).toEqual(1) @@ -83,9 +90,6 @@ integration('cleanupOldImageVersions', () => { expect(imageCount).toEqual(2) await run() - // The Docker API seems to ack the removal before it is carried out, so let's wait briefly to ensure - // the verification query doesn't race the deletion - await delay(200) const remainingImages = await docker.listImages(imageOptions) expect(remainingImages.length).toEqual(2) diff --git a/__tests__/cleanup.test.ts b/__tests__/cleanup.test.ts new file mode 100644 index 000000000..3c9fea0cd --- /dev/null +++ b/__tests__/cleanup.test.ts @@ -0,0 +1,190 @@ +import * as core from '@actions/core' +import Docker from 'dockerode' +import { + PROXY_IMAGE_NAME, + repositoryName, + updaterImages +} from '../src/docker-tags' + +const mockPruneNetworks = jest.fn() +const mockPruneContainers = jest.fn() +const mockListImages = jest.fn() +const mockGetImage = jest.fn() + +jest.mock('@actions/core', () => ({ + error: jest.fn(), + info: jest.fn(), + setFailed: jest.fn() +})) +jest.mock('dockerode', () => ({ + __esModule: true, + default: jest.fn().mockImplementation(() => ({ + pruneNetworks: mockPruneNetworks, + pruneContainers: mockPruneContainers, + listImages: mockListImages, + getImage: mockGetImage + })) +})) + +let run: typeof import('../src/cleanup').run +let cleanupOldImageVersions: typeof import('../src/cleanup').cleanupOldImageVersions + +beforeAll(async () => { + process.env.DEPENDABOT_DISABLE_CLEANUP = '1' + const cleanup = await import('../src/cleanup') + run = cleanup.run + cleanupOldImageVersions = cleanup.cleanupOldImageVersions + delete process.env.DEPENDABOT_DISABLE_CLEANUP +}) + +const allImages = [...updaterImages(), PROXY_IMAGE_NAME] +const proxyRepository = repositoryName(PROXY_IMAGE_NAME) + +describe('run', () => { + beforeEach(() => { + delete process.env.DEPENDABOT_DISABLE_CLEANUP + mockPruneNetworks.mockReset().mockResolvedValue(undefined) + mockPruneContainers.mockReset().mockResolvedValue(undefined) + mockListImages.mockReset().mockResolvedValue([]) + mockGetImage.mockReset() + }) + + test('continues cleanup after network pruning fails', async () => { + mockPruneNetworks.mockRejectedValueOnce(new Error('network prune failed')) + + await run() + + expect(core.error).toHaveBeenCalledWith( + 'Error pruning networks: network prune failed' + ) + expect(mockPruneContainers).toHaveBeenCalledTimes(1) + expect(mockListImages).toHaveBeenCalledTimes(allImages.length) + expect(core.setFailed).not.toHaveBeenCalled() + }) + + test('continues image cleanup after container pruning fails', async () => { + mockPruneContainers.mockRejectedValueOnce( + new Error('container prune failed') + ) + + await run() + + expect(mockListImages).toHaveBeenCalledTimes(allImages.length) + expect(core.error).toHaveBeenCalledWith( + 'Error pruning containers: container prune failed' + ) + expect(core.setFailed).not.toHaveBeenCalled() + }) + + test('reports one listing failure and cleans the other repositories', async () => { + const failedRepository = repositoryName(allImages[0]) + mockListImages.mockImplementation( + async (options: {filters: string}): Promise => { + if (options.filters.includes(`"${failedRepository}"`)) { + throw new Error('image listing failed') + } + return [] + } + ) + + await run() + + expect(mockListImages).toHaveBeenCalledTimes(allImages.length) + expect(mockListImages).toHaveBeenCalledWith({ + filters: `{"reference":["${proxyRepository}"]}` + }) + expect(core.error).toHaveBeenCalledWith( + `Error cleaning up images for ${failedRepository}: image listing failed` + ) + expect(core.setFailed).not.toHaveBeenCalled() + }) + + test('waits for every image repository cleanup to settle', async () => { + let finishProxyListing: (() => void) | undefined + const proxyListing = new Promise(resolve => { + finishProxyListing = () => resolve([]) + }) + let markProxyStarted: (() => void) | undefined + const proxyStarted = new Promise(resolve => { + markProxyStarted = resolve + }) + + mockListImages.mockImplementation( + async (options: {filters: string}): Promise => { + if (options.filters.includes(`"${proxyRepository}"`)) { + markProxyStarted?.() + return proxyListing + } + return [] + } + ) + + const cleanup = run() + await proxyStarted + + expect( + await Promise.race([cleanup, Promise.resolve('cleanup pending')]) + ).toBe('cleanup pending') + + finishProxyListing?.() + await cleanup + + expect(mockListImages).toHaveBeenCalledTimes(allImages.length) + expect(core.setFailed).not.toHaveBeenCalled() + }) +}) + +describe('cleanupOldImageVersions', () => { + beforeEach(() => { + mockListImages.mockReset() + mockGetImage.mockReset() + }) + + test('waits for image removal to complete', async () => { + const docker = new Docker() + const oldImage = { + Id: 'old-image', + RepoDigests: ['ghcr.io/dependabot/proxy@sha256:old'] + } as Docker.ImageInfo + let finishRemoval: (() => void) | undefined + const removal = new Promise(resolve => { + finishRemoval = resolve + }) + const remove = jest.fn().mockReturnValue(removal) + + mockListImages.mockResolvedValue([oldImage]) + mockGetImage.mockReturnValue({remove}) + + const cleanup = cleanupOldImageVersions(docker, PROXY_IMAGE_NAME) + + await Promise.resolve() + + expect(remove).toHaveBeenCalledTimes(1) + expect( + await Promise.race([cleanup, Promise.resolve('removal pending')]) + ).toBe('removal pending') + + finishRemoval?.() + await cleanup + }) + + test('reports image removal failures as informational', async () => { + const docker = new Docker() + const oldImage = { + Id: 'old-image', + RepoDigests: ['ghcr.io/dependabot/proxy@sha256:old'] + } as Docker.ImageInfo + const remove = jest.fn().mockRejectedValue(new Error('image in use')) + + mockListImages.mockResolvedValue([oldImage]) + mockGetImage.mockReturnValue({remove}) + + await cleanupOldImageVersions(docker, PROXY_IMAGE_NAME) + + expect(core.info).toHaveBeenCalledWith( + 'Unable to remove old-image -- image in use' + ) + expect(core.error).not.toHaveBeenCalled() + expect(core.setFailed).not.toHaveBeenCalled() + }) +}) diff --git a/dist/cleanup.js b/dist/cleanup.js index a4f9eb067..921a0d389 100644 --- a/dist/cleanup.js +++ b/dist/cleanup.js @@ -74194,19 +74194,36 @@ async function run(cutoff = "24h") { const docker = new import_dockerode.default(); const untilFilter = { until: [cutoff] }; core.info(`Pruning networks older than ${cutoff}`); - await docker.pruneNetworks({ filters: untilFilter }); + await attemptCleanup( + "pruning networks", + async () => docker.pruneNetworks({ filters: untilFilter }) + ); core.info(`Pruning containers older than ${cutoff}`); - await docker.pruneContainers({ filters: untilFilter }); + await attemptCleanup( + "pruning containers", + async () => docker.pruneContainers({ filters: untilFilter }) + ); + const images = [...updaterImages(), PROXY_IMAGE_NAME]; await Promise.all( - updaterImages().map(async (image) => { - return cleanupOldImageVersions(docker, image); + images.map(async (image) => { + const repo = repositoryName(image); + return attemptCleanup( + `cleaning up images for ${repo}`, + async () => cleanupOldImageVersions(docker, image) + ); }) ); - await cleanupOldImageVersions(docker, PROXY_IMAGE_NAME); } catch (error2) { - if (error2 instanceof Error) { - core.error(`Error cleaning up: ${error2.message}`); - } + const message = error2 instanceof Error ? error2.message : String(error2); + core.error(`Error cleaning up: ${message}`); + } +} +async function attemptCleanup(description, cleanup) { + try { + await cleanup(); + } catch (error2) { + const message = error2 instanceof Error ? error2.message : String(error2); + core.error(`Error ${description}: ${message}`); } } async function cleanupOldImageVersions(docker, imageName) { @@ -74215,24 +74232,20 @@ async function cleanupOldImageVersions(docker, imageName) { filters: `{"reference":["${repo}"]}` }; core.info(`Cleaning up images for ${repo}`); - docker.listImages(options, async function(err, imageInfoList) { - if (imageInfoList && imageInfoList.length > 0) { - for (const imageInfo of imageInfoList) { - if (imageMatches(imageInfo, imageName)) { - core.info(`Skipping current image ${imageInfo.Id}`); - continue; - } - core.info(`Removing image ${imageInfo.Id}`); - try { - await docker.getImage(imageInfo.Id).remove(); - } catch (error2) { - if (error2 instanceof Error) { - core.info(`Unable to remove ${imageInfo.Id} -- ${error2.message}`); - } - } - } + const imageInfoList = await docker.listImages(options); + for (const imageInfo of imageInfoList) { + if (imageMatches(imageInfo, imageName)) { + core.info(`Skipping current image ${imageInfo.Id}`); + continue; } - }); + core.info(`Removing image ${imageInfo.Id}`); + try { + await docker.getImage(imageInfo.Id).remove(); + } catch (error2) { + const message = error2 instanceof Error ? error2.message : String(error2); + core.info(`Unable to remove ${imageInfo.Id} -- ${message}`); + } + } } function imageMatches(imageInfo, imageName) { if (hasDigest(imageName)) { diff --git a/src/cleanup.ts b/src/cleanup.ts index 85fe54592..ef0d14770 100644 --- a/src/cleanup.ts +++ b/src/cleanup.ts @@ -21,20 +21,41 @@ export async function run(cutoff = '24h'): Promise { try { const docker = new Docker() const untilFilter = {until: [cutoff]} + core.info(`Pruning networks older than ${cutoff}`) - await docker.pruneNetworks({filters: untilFilter}) + await attemptCleanup('pruning networks', async () => + docker.pruneNetworks({filters: untilFilter}) + ) + core.info(`Pruning containers older than ${cutoff}`) - await docker.pruneContainers({filters: untilFilter}) + await attemptCleanup('pruning containers', async () => + docker.pruneContainers({filters: untilFilter}) + ) + + const images = [...updaterImages(), PROXY_IMAGE_NAME] await Promise.all( - updaterImages().map(async image => { - return cleanupOldImageVersions(docker, image) + images.map(async image => { + const repo = repositoryName(image) + return attemptCleanup(`cleaning up images for ${repo}`, async () => + cleanupOldImageVersions(docker, image) + ) }) ) - await cleanupOldImageVersions(docker, PROXY_IMAGE_NAME) } catch (error: unknown) { - if (error instanceof Error) { - core.error(`Error cleaning up: ${error.message}`) - } + const message = error instanceof Error ? error.message : String(error) + core.error(`Error cleaning up: ${message}`) + } +} + +async function attemptCleanup( + description: string, + cleanup: () => Promise +): Promise { + try { + await cleanup() + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error) + core.error(`Error ${description}: ${message}`) } } @@ -49,34 +70,30 @@ export async function cleanupOldImageVersions( core.info(`Cleaning up images for ${repo}`) - docker.listImages(options, async function (err, imageInfoList) { - if (imageInfoList && imageInfoList.length > 0) { - for (const imageInfo of imageInfoList) { - // The given imageName is expected to be a tag + digest, however to avoid any surprises in future - // we fail over to check for a match on just tags as well. - // - // This means we won't remove any image which matches an imageName of either of these notations: - // - dependabot/image:$TAG@sha256:$REF (current implementation) - // - dependabot/image:v1 - // - // Without checking imageInfo.RepoTags for a match, we would actually remove the latter even if - // this was the active version. - if (imageMatches(imageInfo, imageName)) { - core.info(`Skipping current image ${imageInfo.Id}`) - continue - } + const imageInfoList = await docker.listImages(options) + for (const imageInfo of imageInfoList) { + // The given imageName is expected to be a tag + digest, however to avoid any surprises in future + // we fail over to check for a match on just tags as well. + // + // This means we won't remove any image which matches an imageName of either of these notations: + // - dependabot/image:$TAG@sha256:$REF (current implementation) + // - dependabot/image:v1 + // + // Without checking imageInfo.RepoTags for a match, we would actually remove the latter even if + // this was the active version. + if (imageMatches(imageInfo, imageName)) { + core.info(`Skipping current image ${imageInfo.Id}`) + continue + } - core.info(`Removing image ${imageInfo.Id}`) - try { - await docker.getImage(imageInfo.Id).remove() - } catch (error: unknown) { - if (error instanceof Error) { - core.info(`Unable to remove ${imageInfo.Id} -- ${error.message}`) - } - } - } + core.info(`Removing image ${imageInfo.Id}`) + try { + await docker.getImage(imageInfo.Id).remove() + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error) + core.info(`Unable to remove ${imageInfo.Id} -- ${message}`) } - }) + } } function imageMatches(imageInfo: Docker.ImageInfo, imageName: string): boolean {