Await Docker image cleanup to ensure GHES stale image cleanup succeeds

[jeffwidman]

Why this is needed

The cleanup post-action previously started Docker image discovery through Dockerode’s callback API, but cleanupOldImageVersions returned before that callback and the image removals completed. This allowed the post-action to report success and the Node process to exit while cleanup was still in flight.

In production, that means stale Dependabot updater and proxy images may be left behind. This is especially important for persistent self-hosted and GHES runners, where Docker state survives between jobs: old image versions can accumulate over time, consume disk space, and eventually interfere with later Dependabot jobs. Awaiting both image listing and every removal makes completion of the post-action mean that cleanup has actually finished.

The change also speeds up the integration tests by removing two 200 ms timing delays. That test improvement is secondary; the main benefit is making production image cleanup reliable rather than best-effort timing-dependent work.

Summary

• use Dockerode’s promise API when listing images so cleanup waits for listing and removals to finish
• replace timing-based integration assertions with immediate state checks
• add deterministic coverage proving cleanupOldImageVersions remains pending until image removal completes
• rebuild the checked-in cleanup action bundle

Testing

• npx eslint src/cleanup.ts __tests__/cleanup.test.ts __tests__/cleanup-integration.test.ts
• npx tsc --noEmit
• SKIP_INTEGRATION_TESTS=true npx jest __tests__/cleanup.test.ts --runInBand
• npx jest __tests__/cleanup-integration.test.ts --runInBand
• npm run package

The full local non-integration suite still hits the existing five-second timeout in two container-service.test.ts cases; the cleanup unit and integration suites pass.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[jeffwidman]

I'm a little torn on this mock... yes, this test ensures the function was called as expected, but due to the mocks it doesn't ensure it behaves under the covers as expected...

Ultimately I decided to drop this test, it doesn't do much and the integration test does enough (especially since we removed the artificial sleep on it... it should now fail if we're not properly awaiting)

[ruben-pachecocaldera]

Copilot flagged this:

Now that we await docker.listImages(options) directly, a rejection here propagates up and rejects cleanupOldImageVersions. In run(), since the updater images are cleaned via Promise.all(...), a listImages failure for one repo will reject the whole batch and skip cleanup of the remaining images (and the subsequent proxy-image cleanup) — landing in the outer try/catch.

The old callback-based code silently ignored the err argument, so a listing failure for one repo didn't stop the others. To preserve that isolation, consider scoping the listing failure per repo, e.g.:

let imageInfoList: Docker.ImageInfo[]
try {
  imageInfoList = await docker.listImages(options)
} catch (error: unknown) {
  if (error instanceof Error) {
    core.info(`Unable to list images for ${repo} -- ${error.message}`)
  }
  return
}

That keeps one repo's listing failure from short-circuiting cleanup of the others, while still awaiting removals as intended. Alternatively, Promise.allSettled in run() would achieve similar isolation across images.

[jeffwidman]

I'd spotted that error propagation change originally and saw it as a feature not a bug! Since it'll stop swallowing errors...

But I hadn't considered there might be a way to both report the errors and continue for non-error ones. I'll poke at this.