From 2e55c1e703cb88ba9881b138ec785831601b9431 Mon Sep 17 00:00:00 2001 From: Lionel Besson Date: Wed, 22 Feb 2023 09:49:13 +0100 Subject: [PATCH] Grace period for modern thumbnails on publish (#227) --- src/config.ts | 4 +- src/koa/routes.ts | 55 +++++++++++++++++++++++----- src/metrics/prometheus.ts | 8 +++- test/environment/env-vars.options.js | 1 + test/unit/options/webp.avif.test.ts | 39 +++++++++++++++++--- 5 files changed, 90 insertions(+), 17 deletions(-) diff --git a/src/config.ts b/src/config.ts index 3b8eee38..9799893d 100644 --- a/src/config.ts +++ b/src/config.ts @@ -37,8 +37,8 @@ export const AUTO_ORIENT_ORIGINAL = yn(process.env['AUTO_ORIENT_ORIGINAL'], { de export const GENERATE_AVIF = yn(process.env['GENERATE_AVIF'], { default: false }); export const GENERATE_WEBP = yn(process.env['GENERATE_WEBP'], { default: false }); -export const S3_EXPIRE_HOURS = 2; - export const ALLOWED_ORIGINS = (process.env['ALLOWED_ORIGINS'] ?? '*').split(','); export const PREFER_IDENTIFY_OVER_FILE = yn(process.env['PREFER_IDENTIFY_OVER_FILE'], { default: false }); +export const THUMBNAILS_PUBLISH_DELAY = Number.parseInt(process.env['THUMBNAILS_PUBLISH_DELAY'] ?? '', 10) || 20000; +export const S3_EXPIRE_HOURS = 2; diff --git a/src/koa/routes.ts b/src/koa/routes.ts index 2e7ea187..9eb3e957 100644 --- a/src/koa/routes.ts +++ b/src/koa/routes.ts @@ -2,7 +2,7 @@ import Router from '@koa/router'; import { koaBody } from 'koa-body'; import fs from 'node:fs'; import path from 'node:path'; -import { AUTO_ORIENT_ORIGINAL } from '../config.js'; +import { AUTO_ORIENT_ORIGINAL, THUMBNAILS_PUBLISH_DELAY } from '../config.js'; import { autoOrient } from '../image/autoorient.js'; import { getFileFormat } from '../image/filetype.js'; import { rotateImages } from '../image/rotate.js'; @@ -10,7 +10,9 @@ import { allThumbnailKeys, baseThumbnailKeys, createThumbnails, modernThumbnailK import { log } from '../log.js'; import { promDeletedImagesCounter, + promErrorsCounter, promPublishedImagesCounter, + promPublishedThumbnailsErrorCounter, promRotatedImagesCounter, promUploadedImagesCounter } from '../metrics/prometheus.js'; @@ -62,7 +64,12 @@ router.post('/upload', bodyParser, async ctx => { log.debug(`${keyPrefix} - uploading original file`); await uploadOriginalAndBaseThumbnails(originalKey); - allRendered.then(async () => uploadModernThumbnails(originalKey)).catch(error => log.error(error)); + allRendered + .then(async () => uploadModernThumbnails(originalKey)) + .catch(error => { + log.error(error); + promErrorsCounter.inc(); + }); log.debug(`${keyPrefix} - returning response`); @@ -86,13 +93,43 @@ router.post('/publish', bodyParser, apiOnly, async ctx => { } if (!published) { - await Promise.all( - allThumbnailKeys(key).map(async resizedKey => { - if (await incomingStorage.exists(resizedKey)) { - return incomingStorage.move(resizedKey, activeStorage); - } - }) - ); + const publishThumbnails = async (thumbnailKeys: string[]): Promise => { + const unrenderedThumbnailKeys: string[] = []; + await Promise.all( + thumbnailKeys.map(async resizedKey => { + if (await incomingStorage.exists(resizedKey)) { + return incomingStorage.move(resizedKey, activeStorage); + } else { + unrenderedThumbnailKeys.push(resizedKey); + } + }) + ); + return unrenderedThumbnailKeys; + }; + + const missingThumbnailKeys = await publishThumbnails(allThumbnailKeys(key)); + + // In some cases, it could be that all "modern" thumbnails are not yet generated (they are generated asyncrhonously) + // In that case, we give it some more time to fullfill. This will be done asynchronously, and will not prevent the response to be sent. + if (missingThumbnailKeys.length) { + setTimeout( + async () => + publishThumbnails(missingThumbnailKeys) + .then(({ length }) => { + if (!length) { + return; + } + // some thumbnails were not generated + log.error(`${length} thumbnails could not be published for image ${key}`); + promPublishedThumbnailsErrorCounter.labels({ key }).inc(length); + }) + .catch(error => { + log.error(error); + promErrorsCounter.inc(); + }), + THUMBNAILS_PUBLISH_DELAY + ); + } await incomingStorage.move(key, activeStorage); diff --git a/src/metrics/prometheus.ts b/src/metrics/prometheus.ts index 865d28be..acac461c 100644 --- a/src/metrics/prometheus.ts +++ b/src/metrics/prometheus.ts @@ -49,7 +49,13 @@ export const promUploadedImagesCounter = new Counter({ // published images export const promPublishedImagesCounter = new Counter({ name: 'published_images', - help: 'Counter of images published to the active bucket' + help: 'Counter of images published to the active bucket', + labelNames: ['key'] as const +}); + +export const promPublishedThumbnailsErrorCounter = new Counter({ + name: 'published_thumbnails_error', + help: 'Counter of thumbnails that could not be published to the active bucket' }); // deleted images diff --git a/test/environment/env-vars.options.js b/test/environment/env-vars.options.js index 5b730967..1d63ddab 100644 --- a/test/environment/env-vars.options.js +++ b/test/environment/env-vars.options.js @@ -5,3 +5,4 @@ process.env.GENERATE_AVIF = '1'; process.env.GENERATE_WEBP = '1'; process.env.AUTO_ORIENT_ORIGINAL = '1'; process.env.ALLOWED_ORIGINS = '*'; +process.env.THUMBNAILS_PUBLISH_DELAY = '1000'; diff --git a/test/unit/options/webp.avif.test.ts b/test/unit/options/webp.avif.test.ts index cc5beea0..4cf0ec11 100644 --- a/test/unit/options/webp.avif.test.ts +++ b/test/unit/options/webp.avif.test.ts @@ -1,11 +1,12 @@ import request from 'supertest'; +import { THUMBNAILS_PUBLISH_DELAY } from '../../../src/config.js'; import { isAvifWriteSupported, isWebpWriteSupported } from '../../../src/exec/imagemagick.js'; import { baseThumbnailKeys, modernThumbnailKeys } from '../../../src/image/thumbnails.js'; import { koa } from '../../../src/koa/app.js'; import { generateUniqueKeyPrefix } from '../../../src/koa/utils.js'; import { activeStorage, incomingStorage } from '../../../src/storage/storage.js'; -const MODERN_GENERATION_TIME = 10000; +const MAX_MODERN_GENERATION_TIME = 10000; describe('Asynchronous generation of Webp and Avif enabled', () => { test( @@ -29,12 +30,12 @@ describe('Asynchronous generation of Webp and Avif enabled', () => { // modules with jest and --experimental-vm-modules is not top // for the moment. // We expect the generations of modern thumbnails to take less than 10s - await new Promise(resolve => setTimeout(resolve, MODERN_GENERATION_TIME)); + await new Promise(resolve => setTimeout(resolve, MAX_MODERN_GENERATION_TIME)); for (const resizedKey of modernThumbnailKeys(key)) { expect(await incomingStorage.exists(resizedKey)).toBe(true); } }, - MODERN_GENERATION_TIME * 2 + MAX_MODERN_GENERATION_TIME * 2 ); test( @@ -65,11 +66,39 @@ describe('Asynchronous generation of Webp and Avif enabled', () => { // modules with jest and --experimental-vm-modules is not top // for the moment. // We expect the generations of modern thumbnails to take less than 10s - await new Promise(resolve => setTimeout(resolve, MODERN_GENERATION_TIME)); + await new Promise(resolve => setTimeout(resolve, MAX_MODERN_GENERATION_TIME)); for (const fileKey of modernThumbnailKeys(filename)) { expect(await activeStorage.exists(fileKey)).toBe(true); } }, - MODERN_GENERATION_TIME * 2 + MAX_MODERN_GENERATION_TIME * 2 ); + + test('POST /publish will give a grace period for modern thumbnails to be generated after upload', async () => { + const key = generateUniqueKeyPrefix(); + // make it so that BI webp is not available in incoming folder + await incomingStorage.put(`${key}.png`, 'test/data/piano.png'); + await incomingStorage.put(`${key}BI.png`, 'test/data/piano.png'); + await incomingStorage.put(`${key}BI.avif`, 'test/data/piano.png'); // we don't really care about the actual format + + const response = await request(koa.callback()) + .post('/publish') + .send({ secret: 'my secret', filename: `${key}.png` }); + + expect(JSON.parse(response.text)).toEqual({ success: true }); + expect(response.status).toBe(200); + expect(await incomingStorage.exists(`${key}.png`)).toBe(false); + expect(await incomingStorage.exists(`${key}BI.png`)).toBe(false); + expect(await incomingStorage.exists(`${key}BI.avif`)).toBe(false); + expect(await activeStorage.exists(`${key}.png`)).toBe(true); + expect(await activeStorage.exists(`${key}BI.png`)).toBe(true); + expect(await activeStorage.exists(`${key}BI.avif`)).toBe(true); + expect(await activeStorage.exists(`${key}MI.avif`)).toBe(false); // could not be published, doesn't exist in incoming + + await incomingStorage.put(`${key}MI.avif`, 'test/data/piano.png'); + + await new Promise(resolve => setTimeout(resolve, THUMBNAILS_PUBLISH_DELAY + 1000)); + + expect(await activeStorage.exists(`${key}MI.avif`)).toBe(true); // should now be published + }); });