From 3b2aa47e95e277e94a053636f693801c8dc96cf4 Mon Sep 17 00:00:00 2001 From: Hellyson Rodrigo Parteka Date: Tue, 30 Nov 2021 04:39:40 -0300 Subject: [PATCH 01/10] fix(file-cache): try/catch to prevent concurrency issues --- src/services/file-cache.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index fa4f1f7..ddbe190 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -58,10 +58,14 @@ export default class FileCacheProvider { const stats = await fs.stat(tmpPath); if (stats.size !== 0) { - await fs.rename(tmpPath, finalPath); - } + try { + await fs.rename(tmpPath, finalPath); - await FileCache.create({hash, bytes: stats.size, accessedAt: new Date()}); + await FileCache.create({hash, bytes: stats.size, accessedAt: new Date()}); + } catch (e: unknown) { + console.error(e); + } + } await this.evictOldestIfNecessary(); }); From af82be13f9b9a27578c7506c98ef71ccbd84a65c Mon Sep 17 00:00:00 2001 From: Hellyson Rodrigo Parteka Date: Fri, 3 Dec 2021 01:01:35 -0300 Subject: [PATCH 02/10] fix(file-cache): add queue to handle eviction of old files This commit also removes the `await` from every stream creation. The eviction will be handled totally assyncronously. The only drawback is the possibility of exceeding the cache limit for a moment, until the next execution of `evictOldest`. This will only be a problem if the cache is set too close to the remaining disk space, which I wouldn't recomend. I also removed the recursion. --- src/services/file-cache.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index ddbe190..826c26d 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -5,9 +5,11 @@ import sequelize from 'sequelize'; import {FileCache} from '../models/index.js'; import {TYPES} from '../types.js'; import Config from './config.js'; +import PQueue from 'p-queue'; @injectable() export default class FileCacheProvider { + private static readonly evictionQueue = new PQueue({concurrency: 1}); private readonly config: Config; constructor(@inject(TYPES.Config) config: Config) { @@ -67,7 +69,7 @@ export default class FileCacheProvider { } } - await this.evictOldestIfNecessary(); + this.evictOldestIfNecessary(); }); return stream; @@ -80,10 +82,16 @@ export default class FileCacheProvider { */ async cleanup() { await this.removeOrphans(); - await this.evictOldestIfNecessary(); + this.evictOldestIfNecessary(); } - private async evictOldestIfNecessary() { + private evictOldestIfNecessary() { + if (FileCacheProvider.evictionQueue.size === 0 && FileCacheProvider.evictionQueue.pending === 0) { + void FileCacheProvider.evictionQueue.add(this.evictOldest.bind(this)); + } + } + + private async evictOldest() { const [{dataValues: {totalSizeBytes}}] = await FileCache.findAll({ attributes: [ [sequelize.fn('sum', sequelize.col('bytes')), 'totalSizeBytes'], @@ -103,7 +111,7 @@ export default class FileCacheProvider { } // Continue to evict until we're under the limit - await this.evictOldestIfNecessary(); + void FileCacheProvider.evictionQueue.add(this.evictOldest.bind(this)); } } From b52f9253c2a905642a01e510bc91dadeb0db419a Mon Sep 17 00:00:00 2001 From: Hellyson Rodrigo Parteka Date: Fri, 3 Dec 2021 03:26:36 -0300 Subject: [PATCH 03/10] add some debug logs --- src/services/file-cache.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index 826c26d..1df8d71 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -6,6 +6,7 @@ import {FileCache} from '../models/index.js'; import {TYPES} from '../types.js'; import Config from './config.js'; import PQueue from 'p-queue'; +import debug from '../utils/debug.js'; @injectable() export default class FileCacheProvider { @@ -65,7 +66,7 @@ export default class FileCacheProvider { await FileCache.create({hash, bytes: stats.size, accessedAt: new Date()}); } catch (e: unknown) { - console.error(e); + debug(`Caught error trying to move a finished cache file: ${String(e)}`); } } @@ -87,11 +88,13 @@ export default class FileCacheProvider { private evictOldestIfNecessary() { if (FileCacheProvider.evictionQueue.size === 0 && FileCacheProvider.evictionQueue.pending === 0) { + debug('Adding evictOldest task to queue'); void FileCacheProvider.evictionQueue.add(this.evictOldest.bind(this)); } } private async evictOldest() { + debug('Evicting oldest (if found)'); const [{dataValues: {totalSizeBytes}}] = await FileCache.findAll({ attributes: [ [sequelize.fn('sum', sequelize.col('bytes')), 'totalSizeBytes'], @@ -111,8 +114,11 @@ export default class FileCacheProvider { } // Continue to evict until we're under the limit + debug('Scheduling another eviction'); void FileCacheProvider.evictionQueue.add(this.evictOldest.bind(this)); } + + debug('Finished evictOldest'); } private async removeOrphans() { From 70a55e9a2e8df91d2e4dc0792a17d9b48f6f20d0 Mon Sep 17 00:00:00 2001 From: Max Isom Date: Fri, 3 Dec 2021 10:36:06 -0500 Subject: [PATCH 04/10] Disable @typescript-eslint/no-implicit-any-catch (Strict mode in TS 4.4 enables useUnknownInCatchVariables, so this is redundant.) --- package.json | 3 ++- src/services/file-cache.ts | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 79235d3..beedf29 100644 --- a/package.json +++ b/package.json @@ -63,7 +63,8 @@ "new-cap": "off", "@typescript-eslint/no-unused-vars": "off", "@typescript-eslint/no-unused-vars-experimental": "error", - "@typescript-eslint/prefer-readonly-parameter-types": "off" + "@typescript-eslint/prefer-readonly-parameter-types": "off", + "@typescript-eslint/no-implicit-any-catch": "off" } }, "husky": { diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index 1df8d71..b99c8c6 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -65,8 +65,8 @@ export default class FileCacheProvider { await fs.rename(tmpPath, finalPath); await FileCache.create({hash, bytes: stats.size, accessedAt: new Date()}); - } catch (e: unknown) { - debug(`Caught error trying to move a finished cache file: ${String(e)}`); + } catch (error) { + debug('Errored when moving a finished cache file:', error); } } From 3f0f97f762d0cf5c9be6dbc34b0bc081307058ef Mon Sep 17 00:00:00 2001 From: Max Isom Date: Fri, 3 Dec 2021 10:45:09 -0500 Subject: [PATCH 05/10] Return when queue is empty --- src/services/file-cache.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index b99c8c6..e82b705 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -10,7 +10,7 @@ import debug from '../utils/debug.js'; @injectable() export default class FileCacheProvider { - private static readonly evictionQueue = new PQueue({concurrency: 1}); + private readonly evictionQueue = new PQueue({concurrency: 1}); private readonly config: Config; constructor(@inject(TYPES.Config) config: Config) { @@ -70,7 +70,7 @@ export default class FileCacheProvider { } } - this.evictOldestIfNecessary(); + await this.evictOldestIfNecessary(); }); return stream; @@ -83,14 +83,16 @@ export default class FileCacheProvider { */ async cleanup() { await this.removeOrphans(); - this.evictOldestIfNecessary(); + await this.evictOldestIfNecessary(); } - private evictOldestIfNecessary() { - if (FileCacheProvider.evictionQueue.size === 0 && FileCacheProvider.evictionQueue.pending === 0) { + private async evictOldestIfNecessary() { + if (this.evictionQueue.size === 0 && this.evictionQueue.pending === 0) { debug('Adding evictOldest task to queue'); - void FileCacheProvider.evictionQueue.add(this.evictOldest.bind(this)); + void this.evictionQueue.add(this.evictOldest.bind(this)); } + + return this.evictionQueue.onEmpty(); } private async evictOldest() { @@ -115,7 +117,7 @@ export default class FileCacheProvider { // Continue to evict until we're under the limit debug('Scheduling another eviction'); - void FileCacheProvider.evictionQueue.add(this.evictOldest.bind(this)); + void this.evictionQueue.add(this.evictOldest.bind(this)); } debug('Finished evictOldest'); From 4ffd679ddb227ea6e6ac911de382e4d5bec0926d Mon Sep 17 00:00:00 2001 From: Max Isom Date: Fri, 3 Dec 2021 10:52:30 -0500 Subject: [PATCH 06/10] Update debug logging --- src/services/file-cache.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index e82b705..7e7aa6d 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -87,16 +87,13 @@ export default class FileCacheProvider { } private async evictOldestIfNecessary() { - if (this.evictionQueue.size === 0 && this.evictionQueue.pending === 0) { - debug('Adding evictOldest task to queue'); - void this.evictionQueue.add(this.evictOldest.bind(this)); - } + void this.evictionQueue.add(this.evictOldest.bind(this)); return this.evictionQueue.onEmpty(); } private async evictOldest() { - debug('Evicting oldest (if found)'); + debug('Evicting oldest files...'); const [{dataValues: {totalSizeBytes}}] = await FileCache.findAll({ attributes: [ [sequelize.fn('sum', sequelize.col('bytes')), 'totalSizeBytes'], @@ -113,14 +110,14 @@ export default class FileCacheProvider { if (oldest) { await oldest.destroy(); await fs.unlink(path.join(this.config.CACHE_DIR, oldest.hash)); + debug(`${oldest.hash} has been evicted`); } // Continue to evict until we're under the limit - debug('Scheduling another eviction'); void this.evictionQueue.add(this.evictOldest.bind(this)); + } else { + debug(`No files needed to be evicted. Total size of the cache is currently ${totalSizeBytes} bytes, and the cache limit is ${this.config.CACHE_LIMIT_IN_BYTES} bytes.`); } - - debug('Finished evictOldest'); } private async removeOrphans() { From 7ff54b9495545cd23fa87ac13bd654a78c29aa1d Mon Sep 17 00:00:00 2001 From: Max Isom Date: Fri, 3 Dec 2021 11:06:56 -0500 Subject: [PATCH 07/10] Use loop instead of recursion --- src/services/file-cache.ts | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index 7e7aa6d..ac8c815 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -94,13 +94,12 @@ export default class FileCacheProvider { private async evictOldest() { debug('Evicting oldest files...'); - const [{dataValues: {totalSizeBytes}}] = await FileCache.findAll({ - attributes: [ - [sequelize.fn('sum', sequelize.col('bytes')), 'totalSizeBytes'], - ], - }) as unknown as [{dataValues: {totalSizeBytes: number}}]; - if (totalSizeBytes > this.config.CACHE_LIMIT_IN_BYTES) { + let totalSizeBytes = await this.getDiskUsageInBytes(); + let numOfEvictedFiles = 0; + // Continue to evict until we're under the limit + /* eslint-disable no-await-in-loop */ + while (totalSizeBytes > this.config.CACHE_LIMIT_IN_BYTES) { const oldest = await FileCache.findOne({ order: [ ['accessedAt', 'ASC'], @@ -111,10 +110,15 @@ export default class FileCacheProvider { await oldest.destroy(); await fs.unlink(path.join(this.config.CACHE_DIR, oldest.hash)); debug(`${oldest.hash} has been evicted`); + numOfEvictedFiles++; } - // Continue to evict until we're under the limit - void this.evictionQueue.add(this.evictOldest.bind(this)); + totalSizeBytes = await this.getDiskUsageInBytes(); + } + /* eslint-enable no-await-in-loop */ + + if (numOfEvictedFiles > 0) { + debug(`${numOfEvictedFiles} files have been evicted`); } else { debug(`No files needed to be evicted. Total size of the cache is currently ${totalSizeBytes} bytes, and the cache limit is ${this.config.CACHE_LIMIT_IN_BYTES} bytes.`); } @@ -131,4 +135,19 @@ export default class FileCacheProvider { } } } + + /** + * Pulls from the database rather than the filesystem, + * so may be slightly inaccurate. + * @returns the total size of the cache in bytes + */ + private async getDiskUsageInBytes() { + const [{dataValues: {totalSizeBytes}}] = await FileCache.findAll({ + attributes: [ + [sequelize.fn('sum', sequelize.col('bytes')), 'totalSizeBytes'], + ], + }) as unknown as [{dataValues: {totalSizeBytes: number}}]; + + return totalSizeBytes; + } } From 29ec1d0092425c1c3822518b12d62bd4b562704e Mon Sep 17 00:00:00 2001 From: Max Isom Date: Fri, 3 Dec 2021 11:28:50 -0500 Subject: [PATCH 08/10] Revert back to static property --- src/services/file-cache.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index ac8c815..64dcfe1 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -10,7 +10,7 @@ import debug from '../utils/debug.js'; @injectable() export default class FileCacheProvider { - private readonly evictionQueue = new PQueue({concurrency: 1}); + private static readonly evictionQueue = new PQueue({concurrency: 1}); private readonly config: Config; constructor(@inject(TYPES.Config) config: Config) { @@ -87,9 +87,9 @@ export default class FileCacheProvider { } private async evictOldestIfNecessary() { - void this.evictionQueue.add(this.evictOldest.bind(this)); + void FileCacheProvider.evictionQueue.add(this.evictOldest.bind(this)); - return this.evictionQueue.onEmpty(); + return FileCacheProvider.evictionQueue.onEmpty(); } private async evictOldest() { From 0396949b39213104054008ccc4b26c6d5b1dd3e3 Mon Sep 17 00:00:00 2001 From: Max Isom Date: Tue, 7 Dec 2021 20:36:06 -0500 Subject: [PATCH 09/10] Check database direction for orphans --- src/services/file-cache.ts | 66 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/src/services/file-cache.ts b/src/services/file-cache.ts index 64dcfe1..1ffb824 100644 --- a/src/services/file-cache.ts +++ b/src/services/file-cache.ts @@ -125,15 +125,29 @@ export default class FileCacheProvider { } private async removeOrphans() { + // Check filesystem direction (do files exist on the disk but not in the database?) for await (const dirent of await fs.opendir(this.config.CACHE_DIR)) { if (dirent.isFile()) { const model = await FileCache.findByPk(dirent.name); if (!model) { + debug(`${dirent.name} was present on disk but was not in the database. Removing from disk.`); await fs.unlink(path.join(this.config.CACHE_DIR, dirent.name)); } } } + + // Check database direction (do entries exist in the database but not on the disk?) + for await (const model of this.getFindAllIterable()) { + const filePath = path.join(this.config.CACHE_DIR, model.hash); + + try { + await fs.access(filePath); + } catch { + debug(`${model.hash} was present in database but was not on disk. Removing from database.`); + await model.destroy(); + } + } } /** @@ -150,4 +164,56 @@ export default class FileCacheProvider { return totalSizeBytes; } + + /** + * An efficient way to iterate over all rows. + * @returns an iterable for the result of FileCache.findAll() + */ + private getFindAllIterable() { + const limit = 50; + let previousCreatedAt: Date | null = null; + + let models: FileCache[] = []; + + const fetchNextBatch = async () => { + let where = {}; + + if (previousCreatedAt) { + where = { + createdAt: { + [sequelize.Op.gt]: previousCreatedAt, + }, + }; + } + + models = await FileCache.findAll({ + where, + limit, + order: ['createdAt'], + }); + + if (models.length > 0) { + previousCreatedAt = models[models.length - 1].createdAt as Date; + } + }; + + return { + [Symbol.asyncIterator]() { + return { + async next() { + if (models.length === 0) { + await fetchNextBatch(); + } + + if (models.length === 0) { + // Must return value here for types to be inferred correctly + return {done: true, value: null as unknown as FileCache}; + } + + return {value: models.shift()!, done: false}; + }, + }; + }, + }; + } } From 260e8702bf74438fb738d28dd8238576f26ab8a8 Mon Sep 17 00:00:00 2001 From: Max Isom Date: Fri, 17 Dec 2021 18:38:23 -0600 Subject: [PATCH 10/10] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6e9487..353c969 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed +- Fixes a race condition in the file cache service (see #420) ## [0.1.0] ### Added