From b17a59620c8b32663e3cf7f3c0c76c2a97c95ed2 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 3 Jun 2025 15:23:41 -0400 Subject: [PATCH 1/4] Avoid hanging test process by looping interval --- src/builder.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/builder.ts b/src/builder.ts index d900e70..4e0ed75 100644 --- a/src/builder.ts +++ b/src/builder.ts @@ -286,8 +286,10 @@ function reportQueueDepth() { gauge( 'build_queue', buildQueue.length ); } -loop( warnOnQueueBuildup, ONE_MINUTE ); -loop( buildFromQueue, ONE_SECOND ); +if ( process.env.NODE_ENV !== 'test' ) { + loop( warnOnQueueBuildup, ONE_MINUTE ); + loop( buildFromQueue, ONE_SECOND ); -// report the queue depth every five seconds to keep statsd aggregations happy -loop( reportQueueDepth, ONE_SECOND * 5 ); + // report the queue depth every five seconds to keep statsd aggregations happy + loop( reportQueueDepth, ONE_SECOND * 5 ); +} From 86463ab35982792716ade2202f28491f08b77e27 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 3 Jun 2025 15:23:54 -0400 Subject: [PATCH 2/4] Avoid isolated test run with `test.only` --- test/api.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/api.test.ts b/test/api.test.ts index f98d924..c39ad40 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -37,7 +37,7 @@ describe( 'api', () => { expect( getExpiredContainers() ).toEqual( images ); } ); - test.only( 'returns empty list if everything was accessed before expiry', () => { + test( 'returns empty list if everything was accessed before expiry', () => { state.accesses.set( 'foo', GOOD_TIME ); state.accesses.set( 'bar', GOOD_TIME ); From 8bfde769587dc2c71aaf429f1bd691fcea0746b3 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 3 Jun 2025 15:24:08 -0400 Subject: [PATCH 3/4] Fix TypeScript error with nullable set getter --- test/api.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/api.test.ts b/test/api.test.ts index c39ad40..e790604 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -51,7 +51,7 @@ describe( 'api', () => { } ); test( 'young images are not returned, regardless of access time', () => { - state.containers.get( getImageName( '1' ) ).Created = Date.now() / 1000; + state.containers.get( getImageName( '1' ) )!.Created = Date.now() / 1000; expect( getExpiredContainers() ).toEqual( [ state.containers.get( getImageName( '2' ) ) ] ); } ); From 2dd176a2673267c5b0907257182f6d03de3555b0 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 3 Jun 2025 15:24:33 -0400 Subject: [PATCH 4/4] Reset shared image cache on docker pull failure --- src/api.ts | 1 + test/api.test.ts | 103 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 2 deletions(-) diff --git a/src/api.ts b/src/api.ts index f90c1e3..15eceb9 100644 --- a/src/api.ts +++ b/src/api.ts @@ -598,6 +598,7 @@ export async function pullImage( imageName: ImageName, onProgress: ( data: any ) onProgress ); } catch( err ) { + state.pullingImages.delete( imageName ); reject(err); } } ); diff --git a/test/api.test.ts b/test/api.test.ts index e790604..b5ec17a 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -1,8 +1,107 @@ -import { getExpiredContainers, getImageName, state } from '../src/api'; - +import { getExpiredContainers, getImageName, state, docker, pullImage } from '../src/api'; import { CONTAINER_EXPIRY_TIME } from '../src/constants'; +jest.mock( 'dockerode', () => { + return jest.fn().mockImplementation( () => ( { + pull: jest.fn(), + modem: { + followProgress: jest.fn(), + }, + } ) ); +} ); + describe( 'api', () => { + describe( 'pullImage', () => { + const mockImageName = 'test-image:latest'; + const mockOnProgress = jest.fn(); + + beforeEach( () => { + state.pullingImages = new Map(); + jest.clearAllMocks(); + mockOnProgress.mockClear(); + } ); + + test( 'should store promise in state.pullingImages and clean up on success', async () => { + ( docker.pull as jest.Mock ).mockResolvedValue( 'mock-stream' ); + + ( docker.modem.followProgress as jest.Mock ).mockImplementation( ( _stream, callback ) => { + callback( null ); + } ); + + expect( state.pullingImages.has( mockImageName ) ).toBe( false ); + + const pullPromise = pullImage( mockImageName, mockOnProgress ); + + expect( state.pullingImages.has( mockImageName ) ).toBe( true ); + + // Wait for the promise to resolve + await pullPromise; + + // Should clean up after successful completion + expect( state.pullingImages.has( mockImageName ) ).toBe( false ); + } ); + + test( 'should reuse existing promise for concurrent requests', async () => { + ( docker.pull as jest.Mock ).mockResolvedValue( 'mock-stream' ); + + ( docker.modem.followProgress as jest.Mock ).mockImplementation( ( _stream, callback ) => { + callback( null ); + } ); + + // Start first request + const firstRequest = pullImage( mockImageName, mockOnProgress ); + expect( state.pullingImages.has( mockImageName ) ).toBe( true ); + expect( docker.pull ).toHaveBeenCalledTimes( 1 ); + + // Start second request while first is still in progress (should reuse the same promise) + const secondRequest = pullImage( mockImageName, mockOnProgress ); + expect( state.pullingImages.has( mockImageName ) ).toBe( true ); + // docker.pull should not be called again + expect( docker.pull ).toHaveBeenCalledTimes( 1 ); + + await Promise.all( [ firstRequest, secondRequest ] ); + expect( state.pullingImages.has( mockImageName ) ).toBe( false ); + } ); + + test( 'should clean up state.pullingImages when followProgress callback receives error', async () => { + ( docker.pull as jest.Mock ).mockResolvedValue( 'mock-stream' ); + ( docker.modem.followProgress as jest.Mock ).mockImplementation( ( _stream, callback ) => { + callback( new Error( 'Follow progress error' ) ); + } ); + + expect( state.pullingImages.has( mockImageName ) ).toBe( false ); + + const pullPromise = pullImage( mockImageName, mockOnProgress ); + expect( state.pullingImages.has( mockImageName ) ).toBe( true ); + + await expect( pullPromise ).rejects.toThrow( 'Follow progress error' ); + + expect( state.pullingImages.has( mockImageName ) ).toBe( false ); + } ); + + test( 'should allow retry after docker.pull error', async () => { + ( docker.pull as jest.Mock ).mockRejectedValue( new Error( 'Image not found' ) ); + + // First call fails + await expect( pullImage( mockImageName, mockOnProgress ) ).rejects.toThrow( + 'Image not found' + ); + expect( docker.pull ).toHaveBeenCalledTimes( 1 ); + expect( state.pullingImages.has( mockImageName ) ).toBe( false ); + + // Mock a successful pull for retry + ( docker.pull as jest.Mock ).mockResolvedValue( 'mock-stream' ); + ( docker.modem.followProgress as jest.Mock ).mockImplementation( ( _stream, callback ) => { + callback( null ); + } ); + + // Second call should work and call docker.pull again + await pullImage( mockImageName, mockOnProgress ); + expect( docker.pull ).toHaveBeenCalledTimes( 2 ); + expect( state.pullingImages.has( mockImageName ) ).toBe( false ); + } ); + } ); + describe( 'getExpiredContainers', () => { const RealNow = Date.now; const fakeNow = RealNow() + 24 * 60 * 1000;