From a54a10f649d52a308dcf44199bc56d23edc3c703 Mon Sep 17 00:00:00 2001 From: Jeff Widman Date: Tue, 18 Aug 2026 11:33:19 -0700 Subject: [PATCH] Attempt and await all Docker cleanup without failing Dependabot jobs (#1753) * Await Docker image cleanup * Drop mocked cleanup unit test The test mocked Dockerode end to end, so it only verified that cleanup awaited a promise supplied by the mock. It did not exercise Dockerode's promise implementation or prove that an image was removed. The Docker-backed integration test already verifies cleanup through the real client and daemon, including the resulting image state immediately after cleanup resolves. Keep that as the authoritative coverage instead. * Make Docker cleanup failures independent and non-fatal Attempt network pruning, container pruning, and each updater or proxy image cleanup independently so one Docker error cannot suppress the remaining housekeeping work. Await every repository cleanup and report operation-level failures with contextual error annotations without calling core.setFailed. Keep individual image-removal failures informational because images may still be referenced or encounter expected Docker races. Add deterministic orchestration coverage, prevent duplicate import-time cleanup in the integration test, and rebuild the checked-in cleanup bundle. --- __tests__/cleanup-integration.test.ts | 20 +-- __tests__/cleanup.test.ts | 190 ++++++++++++++++++++++++++ dist/cleanup.js | 63 +++++---- src/cleanup.ts | 85 +++++++----- 4 files changed, 291 insertions(+), 67 deletions(-) create mode 100644 __tests__/cleanup.test.ts 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 {