From 03c5f8c5095dfc2180a5f619753326b49b7e9ac0 Mon Sep 17 00:00:00 2001 From: almog8k Date: Thu, 6 Aug 2026 15:38:02 +0300 Subject: [PATCH 1/6] chore(deps): point raster-shared at local 8.3.0-alpha build Temporary pointer to a locally packed tarball while the reshaped deletion schemas are unreleased. Must be swapped for a published 8.3.0-alpha before merge: the tarball reports the same version as the published alpha, so a plain install on CI would resolve the older package instead. --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index f2577a9..c7b53bd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ "@map-colonies/js-logger": "^5.0.0", "@map-colonies/mc-priority-queue": "^9.1.0", "@map-colonies/mc-utils": "^5.1.0", - "@map-colonies/raster-shared": "^8.3.0-alpha.2", + "@map-colonies/raster-shared": "^9.0.0-alpha.0", "@map-colonies/read-pkg": "^1.0.0", "@map-colonies/schemas": "^1.20.0", "@map-colonies/telemetry": "^10.0.1", @@ -2543,9 +2543,9 @@ "license": "ISC" }, "node_modules/@map-colonies/raster-shared": { - "version": "8.3.0-alpha.2", - "resolved": "https://registry.npmjs.org/@map-colonies/raster-shared/-/raster-shared-8.3.0-alpha.2.tgz", - "integrity": "sha512-P2p0MddLonzxsG/MSkcKopfI05JsaWxe9BpfYPrVllZ7wCbrzyBUnsYsfP3Rl6jHeNlqWWPCJdHkpzvgTi0FOQ==", + "version": "9.0.0-alpha.0", + "resolved": "https://registry.npmjs.org/@map-colonies/raster-shared/-/raster-shared-9.0.0-alpha.0.tgz", + "integrity": "sha512-7NUVOMnlGaeSDYZu84wEXI25SyL/DatD/LHVBiGVP+CYWZ/8KjEe9WMH84EIPZS4VAlNpRbNZ770582AJNRjxQ==", "license": "ISC", "dependencies": { "@map-colonies/mc-priority-queue": "^9.1.0", diff --git a/package.json b/package.json index 03e0d1f..314b7e1 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "@map-colonies/js-logger": "^5.0.0", "@map-colonies/mc-priority-queue": "^9.1.0", "@map-colonies/mc-utils": "^5.1.0", - "@map-colonies/raster-shared": "^8.3.0-alpha.2", + "@map-colonies/raster-shared": "^9.0.0-alpha.0", "@map-colonies/read-pkg": "^1.0.0", "@map-colonies/schemas": "^1.20.0", "@map-colonies/telemetry": "^10.0.1", From a4cf2fb496270bb0f0fa178b54b6ec0ae673251c Mon Sep 17 00:00:00 2001 From: almog8k Date: Thu, 6 Aug 2026 15:39:43 +0300 Subject: [PATCH 2/6] feat(tiles-deletion): resolve S3 bucket and FS subPath from task params (MAPCO-11295) Tiles deletion read its storage locator from strategies.tilesDeletion.{s3Bucket,fsSubPath}, which pinned every task in a deployment to one bucket and one sub path. The reshaped raster-shared deletion schemas carry the locator in the task params, so each task now brings its own. - storageProvider replaces sourceProvider and tilesRelativePath replaces tilesPath, following the shared schemas - an FS storage target is now a sub path of the configured base path. FsStorageProvider joins its own base path and rejects a sub path that falls outside the configured deletion sub paths, so the strategy holds no filesystem knowledge at all and reads only its batching knobs from config - REDIS joined both shared unions but has no provider here yet, so tiles deletion rejects it as unrecoverable (MAPCO-11261) Producers of tiles-deletion and artifacts-deletion tasks must now send the bucket or subPath in the task parameters. --- .../storageProviders/fsStorageProvider.ts | 30 +++-- .../storageProviders/iStorageProvider.ts | 9 +- .../deleteStoredResourcesStrategy.ts | 28 ++-- .../strategies/tilesDeletionStrategy.ts | 66 +++++----- tests/helpers/mocks.ts | 6 - .../fsStorageProvider.spec.ts | 120 +++++++++++------- .../deleteStoredResourcesStrategy.spec.ts | 32 +++-- .../strategies/tilesDeletionStrategy.spec.ts | 111 ++++++++++------ tests/strategyFactory.spec.ts | 4 +- 9 files changed, 243 insertions(+), 163 deletions(-) diff --git a/src/cleaner/storageProviders/fsStorageProvider.ts b/src/cleaner/storageProviders/fsStorageProvider.ts index f91e69b..1e23613 100644 --- a/src/cleaner/storageProviders/fsStorageProvider.ts +++ b/src/cleaner/storageProviders/fsStorageProvider.ts @@ -20,10 +20,19 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { this.logger.debug({ msg: 'Loaded FS storage provider', basePath: this.fsConfig.basePath }); } - public async targetExists(basePath: string, relativePath: string): Promise { - this.logger.debug({ msg: 'Checking if target resource exists', basePath, path: relativePath }); + /** + * @param subPath - Sub path of the configured base path, as supplied by the task + * @param relativePath - Path below `subPath` to check for + */ + public async targetExists(subPath: string, relativePath: string): Promise { + const relativeTargetPath = join(subPath, relativePath); + if (!this.arePathsValid([relativeTargetPath])) + throw new UnrecoverableError(`Cannot act on paths outside the configured sub paths: ${relativeTargetPath}`); + + const targetPath = join(this.fsConfig.basePath, relativeTargetPath); + this.logger.debug({ msg: 'Checking if target resource exists', subPath, path: relativePath, targetPath }); try { - await stat(join(basePath, relativePath)); + await stat(targetPath); return true; } catch (err) { if ((err as NodeJS.ErrnoException).code === 'ENOENT') return false; @@ -31,13 +40,18 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { } } - public async delete(paths: string[], basePath: string): Promise { - this.logger.debug({ msg: 'Deleting files from filesystem', basePath, pathsCount: paths.length }); + /** + * @param paths - Paths relative to `subPath` + * @param subPath - Sub path of the configured base path, as supplied by the task + */ + public async delete(paths: string[], subPath: string): Promise { + this.logger.debug({ msg: 'Deleting files from filesystem', subPath, pathsCount: paths.length }); + const targetPath = join(this.fsConfig.basePath, subPath); let failures: DeleteFailure = new Map(); const results = await Promise.allSettled( paths.map(async (relativePath) => { - await unlink(join(basePath, relativePath)); + await unlink(join(targetPath, relativePath)); }) ); @@ -47,14 +61,14 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { const relativePath = paths[idx]!; const error: unknown = result.reason; const reason = describeError(error); - this.logger.debug({ msg: 'Failed to delete file', path: join(basePath, relativePath), reason, error }); + this.logger.debug({ msg: 'Failed to delete file', path: join(targetPath, relativePath), reason, error }); const chunkFailure = chunkFailures.get(reason); chunkFailures.set(reason, { count: (chunkFailure?.count ?? 0) + 1, sample: chunkFailure?.sample ?? relativePath }); } } failures = mergeFailures({ source: chunkFailures, target: failures }); - await this.cleanupEmptyDirs(paths, basePath); + await this.cleanupEmptyDirs(paths, targetPath); return { failures }; } diff --git a/src/cleaner/storageProviders/iStorageProvider.ts b/src/cleaner/storageProviders/iStorageProvider.ts index 481a529..c841226 100644 --- a/src/cleaner/storageProviders/iStorageProvider.ts +++ b/src/cleaner/storageProviders/iStorageProvider.ts @@ -15,7 +15,8 @@ export interface IStorageProvider { /** * Deletes a batch of relative file paths within the given storage target. * - S3: storageTarget = bucket name; paths are object keys - * - FS: storageTarget = base directory; full path = join(storageTarget, path) + * - FS: storageTarget = sub path of the configured base path; the provider joins its own + * base path and rejects anything falling outside the configured deletion sub paths * * Returns an object including delete failures aggregation with one entry per failed reason. * "Not found" is reported as a failure with additional metadata on failure - count and sample @@ -32,7 +33,8 @@ export interface IStorageProvider { /** * Returns true if relativePath exists within storageTarget and contains data. * - S3: storageTarget = bucket, relativePath = key prefix — lists objects (KeyCount > 0) - * - FS: storageTarget = base directory, relativePath = subdirectory — checks fs.access + * - FS: storageTarget = sub path of the configured base path, relativePath = subdirectory + * below it — checks fs.stat, subject to the same sub path validation as `delete` */ targetExists: (storageTarget: string, relativePath: string) => Promise; } @@ -40,3 +42,6 @@ export interface IStorageProvider { export type StorageProviders = { [T in StorageProvider]?: IStorageProvider; }; + +/** A provider actually registered in the map. */ +export type ResolvedStorageProvider = NonNullable; diff --git a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts index 157b371..7b9b934 100644 --- a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts +++ b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts @@ -1,9 +1,9 @@ import type { Logger } from '@map-colonies/js-logger'; -import { deleteStoredResourcesParamsSchema, type DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; +import { deleteStoredResourcesParamsSchema, DeleteStoredResourcesParams, StorageProvider } from '@map-colonies/raster-shared'; import { inject, injectable } from 'tsyringe'; import type { ConfigType } from '@common/config'; import { SERVICES } from '@common/constants'; -import { summarizeDeleteFailures, type IStorageProvider, type StorageProvider, type StorageProviders } from '@src/cleaner/storageProviders'; +import { summarizeDeleteFailures, type IStorageProvider, type StorageProviders } from '@src/cleaner/storageProviders'; import { RecoverableError, UnrecoverableError } from '../errors'; import { validateSchema } from '../utils'; import type { ITaskStrategy } from './taskStrategy'; @@ -22,16 +22,17 @@ export class DeleteStoredResourcesStrategy implements ITaskStrategy { - const { paths } = params; - const provider = this.resolveStorageProvider(params); + const paths = params.storageProvider === StorageProvider.REDIS ? [] : params.paths; + const provider = this.resolveStorageProvider(params.storageProvider); this.logger.info({ msg: 'Starting deletion', count: paths.length, paths, provider: params.storageProvider, - ...(params.storageProvider === 'S3' && { bucket: params.bucket }), - ...(params.storageProvider === 'FS' && { subPath: params.subPath }), + ...(params.storageProvider === StorageProvider.S3 && { bucket: params.bucket }), + ...(params.storageProvider === StorageProvider.FS && { subPath: params.subPath }), + ...(params.storageProvider === StorageProvider.REDIS && { prefix: params.prefix }), }); const { failures } = await provider.deleteResources(params); @@ -56,14 +57,11 @@ export class DeleteStoredResourcesStrategy implements ITaskStrategy( - params: Extract - ): IStorageProvider { - if (!(params.storageProvider in this.storageProviders)) throw new UnrecoverableError(`Unsupported storage provider ${params.storageProvider}`); - // eslint-disable-next-line @typescript-eslint/naming-convention - const storageProvider = this.storageProviders[params.storageProvider]; - if (storageProvider === undefined) throw new UnrecoverableError(`Unsupported storage provider ${params.storageProvider}`); - this.logger.debug({ msg: `Using ${params.storageProvider} provider` }); - return storageProvider; + private resolveStorageProvider(storageProvider: K): IStorageProvider { + this.logger.debug({ msg: `Resolving storage provider`, provider: storageProvider, providers: Object.keys(this.storageProviders) }); + const provider = this.storageProviders[storageProvider]; + if (provider === undefined) throw new UnrecoverableError(`Unsupported storage provider ${storageProvider}`); + this.logger.debug({ msg: `Using ${storageProvider} provider` }); + return provider; } } diff --git a/src/cleaner/strategies/tilesDeletionStrategy.ts b/src/cleaner/strategies/tilesDeletionStrategy.ts index 792b884..24f94c2 100644 --- a/src/cleaner/strategies/tilesDeletionStrategy.ts +++ b/src/cleaner/strategies/tilesDeletionStrategy.ts @@ -1,34 +1,30 @@ -import { join } from 'node:path'; import { NoSuchKey } from '@aws-sdk/client-s3'; import type { Logger } from '@map-colonies/js-logger'; import type { TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; -import { SourceType, TileRange, TilesDeletionParams, tilesDeletionParamsSchema } from '@map-colonies/raster-shared'; +import { StorageProvider, TileRange, TilesDeletionParams, tilesDeletionParamsSchema } from '@map-colonies/raster-shared'; import { inject, injectable } from 'tsyringe'; import type { ConfigType } from '@common/config'; import { PERCENTAGE_COMPLETE, SERVICES } from '@common/constants'; -import { - mergeFailures, - summarizeDeleteFailures, - type DeleteFailure, - type FsConfig, - type IStorageProvider, - type StorageProvider, - type StorageProviders, -} from '@src/cleaner/storageProviders'; +import { mergeFailures, summarizeDeleteFailures, type DeleteFailure, type StorageProviders } from '@src/cleaner/storageProviders'; import { RecoverableError, UnrecoverableError, describeError } from '../errors'; +import { ResolvedStorageProvider } from '../storageProviders/iStorageProvider'; import { validateSchema } from '../utils'; import type { TaskContext } from './strategyFactory'; import type { ITaskStrategy } from './taskStrategy'; const NOT_FOUND_REASONS = new Set([NoSuchKey.name, 'ENOENT']); +/** + * The params shapes this strategy can act on today. Redis tiles deletion is not implemented + * yet (MAPCO-11261): its params carry a key prefix instead of a tiles path, so there are no + * tile paths to generate. + */ +type SupportedTilesDeletionParams = Exclude; + @injectable() export class TilesDeletionStrategy implements ITaskStrategy { private readonly batchSize: number; private readonly concurrency: number; - private readonly s3Bucket: string; - private readonly fsBasePath: string; - private readonly fsTilesDeletionSubPath: string; public constructor( @inject(SERVICES.LOGGER) private readonly logger: Logger, @@ -39,9 +35,6 @@ export class TilesDeletionStrategy implements ITaskStrategy ) { this.batchSize = config.get('strategies.tilesDeletion.batchSize') as unknown as number; this.concurrency = config.get('strategies.tilesDeletion.concurrency') as unknown as number; - this.s3Bucket = config.get('strategies.tilesDeletion.s3Bucket') as unknown as string; - this.fsBasePath = config.get('storage.fs.basePath') as unknown as FsConfig['basePath']; - this.fsTilesDeletionSubPath = config.get('strategies.tilesDeletion.fsSubPath') as unknown as string; } public validate(params: unknown): TilesDeletionParams { @@ -50,17 +43,21 @@ export class TilesDeletionStrategy implements ITaskStrategy } public async execute(params: TilesDeletionParams): Promise { + if (params.storageProvider === StorageProvider.REDIS) { + throw new UnrecoverableError(`Tiles deletion is not implemented for ${StorageProvider.REDIS} storage`); + } + const { provider, storageTarget } = this.resolveStorageProvider(params); - if (!(await provider.targetExists(storageTarget, params.tilesPath))) { - throw new UnrecoverableError(`${params.sourceProvider} storage target does not exist: ${storageTarget}/${params.tilesPath}`); + if (!(await provider.targetExists(storageTarget, params.tilesRelativePath))) { + throw new UnrecoverableError(`${params.storageProvider} storage target does not exist: ${storageTarget}/${params.tilesRelativePath}`); } const totalTiles = this.countTiles(params); this.logger.info({ msg: 'Starting tiles deletion', - provider: params.sourceProvider, + provider: params.storageProvider, storageTarget, rangeCount: params.ranges.length, totalTiles, @@ -119,22 +116,19 @@ export class TilesDeletionStrategy implements ITaskStrategy this.logger.info({ msg: 'Tiles deletion completed successfully', deletedCount: totalTiles }); } - private resolveStorageProvider( - params: Extract - ): { provider: IStorageProvider; storageTarget: string } { - if (!(params.sourceProvider in this.storageProviders)) throw new UnrecoverableError(`Unsupported storage provider ${params.sourceProvider}`); + private resolveStorageProvider(params: SupportedTilesDeletionParams): { provider: ResolvedStorageProvider; storageTarget: string } { // eslint-disable-next-line @typescript-eslint/naming-convention - const storageProvider = this.storageProviders[params.sourceProvider]; - if (storageProvider === undefined) throw new UnrecoverableError(`Unsupported storage provider ${params.sourceProvider}`); - const storageTarget = params.sourceProvider === SourceType.S3 ? this.s3Bucket : join(this.fsBasePath, this.fsTilesDeletionSubPath); - this.logger.debug({ msg: `Using ${params.sourceProvider} provider` }); + const storageProvider = this.storageProviders[params.storageProvider]; + if (storageProvider === undefined) throw new UnrecoverableError(`Unsupported storage provider ${params.storageProvider}`); + const storageTarget = params.storageProvider === StorageProvider.S3 ? params.bucket : params.subPath; + this.logger.debug({ msg: `Using ${params.storageProvider} provider`, storageTarget }); return { provider: storageProvider, storageTarget }; } private async deleteAllTiles( - provider: IStorageProvider, + provider: ResolvedStorageProvider, storageTarget: string, - params: TilesDeletionParams, + params: SupportedTilesDeletionParams, totalTiles: number ): Promise { const { jobId, taskId } = this.taskContext; @@ -183,7 +177,7 @@ export class TilesDeletionStrategy implements ITaskStrategy * @returns Total tile paths attempted (not necessarily deleted). */ private async flushBatches( - provider: IStorageProvider, + provider: ResolvedStorageProvider, storageTarget: string, pendingBatches: string[][] ): Promise<{ batchFailures: DeleteFailure; processedTilesCount: number }> { @@ -212,20 +206,20 @@ export class TilesDeletionStrategy implements ITaskStrategy * For each range, the tile count is the product of the width (maxX - minX + 1) * and height (maxY - minY + 1) of the range grid. */ - private countTiles(params: TilesDeletionParams): number { + private countTiles(params: SupportedTilesDeletionParams): number { return params.ranges.reduce((sum, r) => sum + (r.maxX - r.minX + 1) * (r.maxY - r.minY + 1), 0); } - private *generateTilePaths(params: TilesDeletionParams): Generator { + private *generateTilePaths(params: SupportedTilesDeletionParams): Generator { for (const range of params.ranges) { - yield* this.generateRangePaths(range, params.tilesPath, params.fileExtension); + yield* this.generateRangePaths(range, params.tilesRelativePath, params.fileExtension); } } - private *generateRangePaths(range: TileRange, tilesPath: string, fileExtension: string): Generator { + private *generateRangePaths(range: TileRange, tilesRelativePath: string, fileExtension: string): Generator { for (let x = range.minX; x <= range.maxX; x++) { for (let y = range.minY; y <= range.maxY; y++) { - yield `${tilesPath}/${range.zoom}/${x}/${y}.${fileExtension}`; + yield `${tilesRelativePath}/${range.zoom}/${x}/${y}.${fileExtension}`; } } } diff --git a/tests/helpers/mocks.ts b/tests/helpers/mocks.ts index 949b565..9f14b46 100644 --- a/tests/helpers/mocks.ts +++ b/tests/helpers/mocks.ts @@ -80,18 +80,12 @@ export function createMockStorageProvider = {}): ConfigType { const values: Record = { 'strategies.tilesDeletion.batchSize': TILES_DELETION_CONFIG_DEFAULTS.batchSize, 'strategies.tilesDeletion.concurrency': TILES_DELETION_CONFIG_DEFAULTS.concurrency, - 'strategies.tilesDeletion.s3Bucket': TILES_DELETION_CONFIG_DEFAULTS.s3Bucket, - 'strategies.tilesDeletion.fsSubPath': TILES_DELETION_CONFIG_DEFAULTS.fsSubPath, - 'storage.fs.basePath': FS_STORAGE_CONFIG_DEFAULTS.basePath, ...overrides, }; return { get: vi.fn().mockImplementation((key: string) => values[key]) } as unknown as ConfigType; diff --git a/tests/storageProviders/fsStorageProvider.spec.ts b/tests/storageProviders/fsStorageProvider.spec.ts index 09603c6..98b4c56 100644 --- a/tests/storageProviders/fsStorageProvider.spec.ts +++ b/tests/storageProviders/fsStorageProvider.spec.ts @@ -15,6 +15,7 @@ vi.mock('node:fs/promises', () => ({ })); const BASE_PATH = FS_VALIDATED_CONFIG_DEFAULTS.basePath; +const SUB_PATH = FS_VALIDATED_CONFIG_DEFAULTS.subPaths[0]!; describe('FsStorageProvider', () => { let provider: FsStorageProvider; @@ -41,18 +42,17 @@ describe('FsStorageProvider', () => { describe('#targetExists', () => { const RELATIVE_PATH = 'layer/v1'; - it('should call stat with full target path', async () => { - const targetPath = join(BASE_PATH, RELATIVE_PATH); - const result = await provider.targetExists(BASE_PATH, RELATIVE_PATH); + it('should call stat with the base path, sub path and relative path joined', async () => { + const result = await provider.targetExists(SUB_PATH, RELATIVE_PATH); expect(result).toBe(true); - expect(stat).toHaveBeenCalledWith(targetPath); + expect(stat).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, RELATIVE_PATH)); }); it('should return false when path does not exist (ENOENT)', async () => { vi.mocked(stat).mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })); - const result = await provider.targetExists(BASE_PATH, RELATIVE_PATH); + const result = await provider.targetExists(SUB_PATH, RELATIVE_PATH); expect(result).toBe(false); }); @@ -60,36 +60,64 @@ describe('FsStorageProvider', () => { it('should throw an errors that are not ENOENT', async () => { vi.mocked(stat).mockRejectedValue(Object.assign(new Error('EACCES'), { code: 'EACCES' })); - await expect(provider.targetExists(BASE_PATH, RELATIVE_PATH)).rejects.toThrow('EACCES'); + await expect(provider.targetExists(SUB_PATH, RELATIVE_PATH)).rejects.toThrow('EACCES'); + }); + + it('should throw UnrecoverableError for a sub path outside the configured sub paths', async () => { + await expect(provider.targetExists('somewhere-else', RELATIVE_PATH)).rejects.toThrow(UnrecoverableError); + expect(stat).not.toHaveBeenCalled(); + }); + + it('should throw UnrecoverableError for a sub path that traverses out of the base path', async () => { + await expect(provider.targetExists(`${SUB_PATH}/../../..`, RELATIVE_PATH)).rejects.toThrow(UnrecoverableError); + expect(stat).not.toHaveBeenCalled(); + }); + + it('should throw UnrecoverableError when the relative path traverses out of the sub path', async () => { + await expect(provider.targetExists(SUB_PATH, '../../..')).rejects.toThrow(UnrecoverableError); + expect(stat).not.toHaveBeenCalled(); + }); + + it('should accept a sub path nested deeper inside a configured sub path', async () => { + const result = await provider.targetExists(`${SUB_PATH}/nested`, RELATIVE_PATH); + + expect(result).toBe(true); + expect(stat).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'nested', RELATIVE_PATH)); }); }); describe('#delete', () => { it('should return empty failures map for empty input', async () => { - const result = await provider.delete([], BASE_PATH); + const result = await provider.delete([], SUB_PATH); expect(result).toEqual({ failures: new Map() }); expect(unlink).not.toHaveBeenCalled(); }); it('should call unlink with joined base path and relative path', async () => { - await provider.delete(['layer/v1/10/0/0.png'], BASE_PATH); + await provider.delete(['layer/v1/10/0/0.png'], SUB_PATH); + + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0/0.png')); + }); + + it('should join a nested sub path onto the base path', async () => { + await provider.delete(['tile/10/0/0.png'], `${SUB_PATH}/nested`); - expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1/10/0/0.png')); + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'nested', 'tile/10/0/0.png')); }); it('should call unlink for every path', async () => { const paths = ['tile/10/0/0.png', 'tile/10/0/1.png', 'tile/10/1/0.png']; - await provider.delete(paths, BASE_PATH); + await provider.delete(paths, SUB_PATH); expect(unlink).toHaveBeenCalledTimes(3); for (const p of paths) { - expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, p)); + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, p)); } }); it('should return empty failures map when all unlinks succeed', async () => { - const result = await provider.delete(['tile/10/0/0.png', 'tile/10/0/1.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png', 'tile/10/0/1.png'], SUB_PATH); expect(result).toEqual({ failures: new Map() }); }); @@ -97,7 +125,7 @@ describe('FsStorageProvider', () => { const enoent = Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); vi.mocked(unlink).mockRejectedValue(enoent); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); expect(result).toEqual({ failures: new Map([['ENOENT', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -106,7 +134,7 @@ describe('FsStorageProvider', () => { const permError = Object.assign(new Error('EACCES'), { code: 'EACCES' }); vi.mocked(unlink).mockRejectedValue(permError); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -114,7 +142,7 @@ describe('FsStorageProvider', () => { it('should fall back to error message when error has no errno code', async () => { vi.mocked(unlink).mockRejectedValue(new Error('disk on fire')); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); expect(result).toEqual({ failures: new Map([['disk on fire', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -122,7 +150,7 @@ describe('FsStorageProvider', () => { it('should fall back to "Unknown" when error has neither errno code nor message', async () => { vi.mocked(unlink).mockRejectedValue(new Error('')); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -130,7 +158,7 @@ describe('FsStorageProvider', () => { it('should tag failures with the stringified value when a non-Error is thrown', async () => { vi.mocked(unlink).mockRejectedValue('raw string failure'); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); expect(result).toEqual({ failures: new Map([['raw string failure', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -138,7 +166,7 @@ describe('FsStorageProvider', () => { it('should fall back to "Unknown" when a non-Error empty value is thrown', async () => { vi.mocked(unlink).mockRejectedValue(''); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -150,7 +178,7 @@ describe('FsStorageProvider', () => { }, }); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); expect(result).toEqual({ failures: new Map([['non-serializable thrown value', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -158,11 +186,11 @@ describe('FsStorageProvider', () => { it('should unlink every path of a large input', async () => { const paths = Array.from({ length: 7 }, (_, i) => `tile/10/0/${i}.png`); - await provider.delete(paths, BASE_PATH); + await provider.delete(paths, SUB_PATH); expect(unlink).toHaveBeenCalledTimes(7); for (const path of paths) { - expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, path)); + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, path)); } }); @@ -171,7 +199,7 @@ describe('FsStorageProvider', () => { vi.mocked(unlink).mockRejectedValue(permError); const paths = Array.from({ length: 7 }, (_, i) => `tile/10/0/${i}.png`); - const result = await provider.delete(paths, BASE_PATH); + const result = await provider.delete(paths, SUB_PATH); expect(result).toEqual({ failures: new Map([['EACCES', { count: 7, sample: 'tile/10/0/0.png' }]]) }); }); @@ -186,7 +214,7 @@ describe('FsStorageProvider', () => { .mockRejectedValueOnce(permError); // real error → failure const paths = ['tile/10/0/0.png', 'tile/10/0/1.png', 'tile/10/0/2.png']; - const result = await provider.delete(paths, BASE_PATH); + const result = await provider.delete(paths, SUB_PATH); expect(result).toEqual({ failures: new Map([ @@ -201,7 +229,7 @@ describe('FsStorageProvider', () => { vi.mocked(unlink).mockRejectedValue(permError); const relativePath = 'layer/v1/10/5/3.png'; - const result = await provider.delete([relativePath], BASE_PATH); + const result = await provider.delete([relativePath], SUB_PATH); expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: relativePath }]]) }); expect(Array.from(result.failures.values())[0]?.sample).not.toMatch(`^${BASE_PATH}*`); @@ -209,27 +237,27 @@ describe('FsStorageProvider', () => { describe('cleanupEmptyDirs', () => { it('should attempt to rmdir the parent directory after deletion', async () => { - await provider.delete(['layer/v1/10/0/0.png'], BASE_PATH); + await provider.delete(['layer/v1/10/0/0.png'], SUB_PATH); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1/10/0')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0')); }); it('should attempt to rmdir all ancestor directories bottom-up', async () => { - await provider.delete(['layer/v1/10/0/0.png'], BASE_PATH); + await provider.delete(['layer/v1/10/0/0.png'], SUB_PATH); // x dir → zoom dir → version dir → layer dir (deepest first) - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1/10/0')); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1/10')); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1')); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer')); }); it('should deduplicate rmdir calls for shared parent directories', async () => { // Both tiles share the same x-dir and zoom dir - await provider.delete(['tile/10/0/0.png', 'tile/10/0/1.png'], BASE_PATH); + await provider.delete(['tile/10/0/0.png', 'tile/10/0/1.png'], SUB_PATH); const rmdirCalls = vi.mocked(rmdir).mock.calls.map(([p]) => p); - const xDirCalls = rmdirCalls.filter((p) => p === join(BASE_PATH, 'tile/10/0')); + const xDirCalls = rmdirCalls.filter((p) => p === join(BASE_PATH, SUB_PATH, 'tile/10/0')); expect(xDirCalls).toHaveLength(1); }); @@ -237,21 +265,21 @@ describe('FsStorageProvider', () => { const enotempty = Object.assign(new Error('ENOTEMPTY'), { code: 'ENOTEMPTY' }); vi.mocked(rmdir).mockRejectedValue(enotempty); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); // Should not throw and should return correct failed paths expect(result).toEqual({ failures: new Map() }); }); it('should not call rmdir when input is empty', async () => { - await provider.delete([], BASE_PATH); + await provider.delete([], SUB_PATH); expect(rmdir).not.toHaveBeenCalled(); }); it('should not call rmdir for a path that has no directory segments', async () => { - await provider.delete(['0.png'], BASE_PATH); + await provider.delete(['0.png'], SUB_PATH); - expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, '0.png')); + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, '0.png')); expect(rmdir).not.toHaveBeenCalled(); }); @@ -259,24 +287,24 @@ describe('FsStorageProvider', () => { // batchSize is 3 → cleanup runs once for all paths, after the last batch const paths = Array.from({ length: 4 }, (_, i) => `tile/10/${i}/0.png`); - await provider.delete(paths, BASE_PATH); + await provider.delete(paths, SUB_PATH); for (let i = 0; i < 4; i++) { - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, `tile/10/${i}`)); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, `tile/10/${i}`)); } - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'tile/10')); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'tile')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'tile/10')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'tile')); }); it('should attempt to rmdir deeper directories before their ancestors', async () => { - await provider.delete(['layer/v1/10/0/0.png'], BASE_PATH); + await provider.delete(['layer/v1/10/0/0.png'], SUB_PATH); const order = vi.mocked(rmdir).mock.calls.map(([path]) => path); expect(order).toEqual([ - join(BASE_PATH, 'layer/v1/10/0'), - join(BASE_PATH, 'layer/v1/10'), - join(BASE_PATH, 'layer/v1'), - join(BASE_PATH, 'layer'), + join(BASE_PATH, SUB_PATH, 'layer/v1/10/0'), + join(BASE_PATH, SUB_PATH, 'layer/v1/10'), + join(BASE_PATH, SUB_PATH, 'layer/v1'), + join(BASE_PATH, SUB_PATH, 'layer'), ]); }); }); diff --git a/tests/strategies/deleteStoredResourcesStrategy.spec.ts b/tests/strategies/deleteStoredResourcesStrategy.spec.ts index 1ff0a63..b9841aa 100644 --- a/tests/strategies/deleteStoredResourcesStrategy.spec.ts +++ b/tests/strategies/deleteStoredResourcesStrategy.spec.ts @@ -1,5 +1,5 @@ import type { Logger } from '@map-colonies/js-logger'; -import { SourceType, type DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; +import { SourceType, type FsDeleteStoredResourcesParams, type S3DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; import { beforeEach, describe, expect, it, type vi } from 'vitest'; import { RecoverableError, UnrecoverableError, ValidationError } from '@src/cleaner/errors'; import type { IStorageProvider, StorageProviders } from '@src/cleaner/storageProviders'; @@ -10,8 +10,8 @@ import { createMockStoredResourcesDeletionStrategyConfig, createMockLogger, crea const S3_BUCKET = 'test-bucket'; const FS_SUB_PATH = 'test/artifacts/tiles'; -const s3Params: DeleteStoredResourcesParams = { storageProvider: SourceType.S3, paths: ['layer1'], bucket: S3_BUCKET }; -const fsParams: DeleteStoredResourcesParams = { storageProvider: SourceType.FS, paths: ['layer2'], subPath: FS_SUB_PATH }; +const s3Params: S3DeleteStoredResourcesParams = { storageProvider: SourceType.S3, paths: ['layer1'], bucket: S3_BUCKET }; +const fsParams: FsDeleteStoredResourcesParams = { storageProvider: SourceType.FS, paths: ['layer2'], subPath: FS_SUB_PATH }; describe('DeleteStoredResourcesStrategy', () => { let strategy: DeleteStoredResourcesStrategy; @@ -57,12 +57,26 @@ describe('DeleteStoredResourcesStrategy', () => { expect(() => strategy.validate({ storageProvider: 'GCS', catalogId: 'layer1' })).toThrow(ValidationError); }); - it('should throw ValidationError when tilesPath is empty string', () => { - expect(() => strategy.validate({ storageProvider: SourceType.S3, catalogId: '' })).toThrow(ValidationError); + it('should validate and return REDIS params, whose prefix is the locator', () => { + const redisParams = { storageProvider: 'REDIS', prefix: 'layer-redis_WorldCRS84' }; + + expect(strategy.validate(redisParams)).toEqual(redisParams); + }); + + it('should throw ValidationError when paths is an empty array', () => { + expect(() => strategy.validate({ storageProvider: SourceType.S3, bucket: S3_BUCKET, paths: [] })).toThrow(ValidationError); + }); + + it('should throw ValidationError when paths is missing', () => { + expect(() => strategy.validate({ storageProvider: SourceType.S3, bucket: S3_BUCKET })).toThrow(ValidationError); + }); + + it('should throw ValidationError when the S3 bucket is missing', () => { + expect(() => strategy.validate({ storageProvider: SourceType.S3, paths: ['layer1'] })).toThrow(ValidationError); }); - it('should throw ValidationError when tilesPath is missing', () => { - expect(() => strategy.validate({ storageProvider: SourceType.S3 })).toThrow(ValidationError); + it('should throw ValidationError when the FS subPath is missing', () => { + expect(() => strategy.validate({ storageProvider: SourceType.FS, paths: ['layer1'] })).toThrow(ValidationError); }); it('should throw ValidationError for null params', () => { @@ -94,7 +108,7 @@ describe('DeleteStoredResourcesStrategy', () => { }); it('should throw UnrecoverableError for unknown provider', async () => { - const unknownParams = { ...s3Params, storageProvider: 'UNKNOWN' } as unknown as DeleteStoredResourcesParams; + const unknownParams = { ...s3Params, storageProvider: 'UNKNOWN' } as unknown as S3DeleteStoredResourcesParams; const result = strategy.execute(unknownParams); @@ -140,7 +154,7 @@ describe('DeleteStoredResourcesStrategy', () => { }); it('should pass every path through to the provider', async () => { - const params: DeleteStoredResourcesParams = { ...s3Params, paths: ['layer1', 'layer2', 'layer3'] }; + const params: S3DeleteStoredResourcesParams = { ...s3Params, paths: ['layer1', 'layer2', 'layer3'] }; await strategy.execute(params); diff --git a/tests/strategies/tilesDeletionStrategy.spec.ts b/tests/strategies/tilesDeletionStrategy.spec.ts index d6dfc10..e80df44 100644 --- a/tests/strategies/tilesDeletionStrategy.spec.ts +++ b/tests/strategies/tilesDeletionStrategy.spec.ts @@ -1,31 +1,38 @@ -import { join } from 'node:path'; import { faker } from '@faker-js/faker'; import type { TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; -import { type TilesDeletionParams, SourceType } from '@map-colonies/raster-shared'; +import { type FsTilesDeletionParams, type S3TilesDeletionParams, SourceType } from '@map-colonies/raster-shared'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { RecoverableError, UnrecoverableError, ValidationError } from '@src/cleaner/errors'; import type { IStorageProvider, StorageProviders } from '@src/cleaner/storageProviders'; import type { TaskContext } from '@src/cleaner/strategies/strategyFactory'; import { TilesDeletionStrategy } from '@src/cleaner/strategies/tilesDeletionStrategy'; -import { createMockLogger, createMockStorageProvider, createMockStrategyConfig, TILES_DELETION_CONFIG_DEFAULTS } from '../helpers/mocks'; +import { createMockLogger, createMockStorageProvider, createMockStrategyConfig } from '../helpers/mocks'; -const { s3Bucket: S3_BUCKET, fsBasePath: FS_BASE_PATH, fsSubPath: FS_SUB_PATH } = TILES_DELETION_CONFIG_DEFAULTS; +const S3_BUCKET = 'test-bucket'; +const FS_SUB_PATH = 'artifacts/tiles'; const JOB_ID = faker.string.uuid(); const TASK_ID = faker.string.uuid(); const TASK_CONTEXT: TaskContext = { jobId: JOB_ID, taskId: TASK_ID, jobType: 'Ingestion_Update', taskType: 'tiles-deletion' }; -const s3Params: TilesDeletionParams = { - sourceProvider: 'S3', - tilesPath: 'layer/v1', +const s3Params: S3TilesDeletionParams = { + storageProvider: 'S3', + bucket: S3_BUCKET, + tilesRelativePath: 'layer/v1', fileExtension: 'png', ranges: [{ zoom: 10, minX: 0, maxX: 1, minY: 0, maxY: 1 }], }; -const fsParams: TilesDeletionParams = { ...s3Params, sourceProvider: 'FS' }; +const fsParams: FsTilesDeletionParams = { + storageProvider: 'FS', + subPath: FS_SUB_PATH, + tilesRelativePath: s3Params.tilesRelativePath, + fileExtension: s3Params.fileExtension, + ranges: s3Params.ranges, +}; -// Builds an expected tile path under the standard s3Params tilesPath/fileExtension. -const tilePath = (z: number, x: number, y: number): string => `${s3Params.tilesPath}/${z}/${x}/${y}.${s3Params.fileExtension}`; +// Builds an expected tile path under the standard tilesRelativePath/fileExtension. +const tilePath = (z: number, x: number, y: number): string => `${s3Params.tilesRelativePath}/${z}/${x}/${y}.${s3Params.fileExtension}`; describe('TilesDeletionStrategy', () => { let strategy: TilesDeletionStrategy; @@ -69,20 +76,28 @@ describe('TilesDeletionStrategy', () => { expect(strategy.validate({ ...s3Params, ranges })).toEqual({ ...s3Params, ranges }); }); - it('should throw ValidationError when sourceProvider is missing', () => { - expect(() => strategy.validate({ ...s3Params, sourceProvider: undefined })).toThrow(ValidationError); + it('should throw ValidationError when storageProvider is missing', () => { + expect(() => strategy.validate({ ...s3Params, storageProvider: undefined })).toThrow(ValidationError); }); - it('should throw ValidationError for unsupported sourceProvider value', () => { - expect(() => strategy.validate({ ...s3Params, sourceProvider: 'GCS' })).toThrow(ValidationError); + it('should throw ValidationError for unsupported storageProvider value', () => { + expect(() => strategy.validate({ ...s3Params, storageProvider: 'GCS' })).toThrow(ValidationError); }); it('should throw ValidationError for empty ranges array', () => { expect(() => strategy.validate({ ...s3Params, ranges: [] })).toThrow(ValidationError); }); - it('should throw ValidationError for empty tilesPath', () => { - expect(() => strategy.validate({ ...s3Params, tilesPath: '' })).toThrow(ValidationError); + it('should throw ValidationError for empty tilesRelativePath', () => { + expect(() => strategy.validate({ ...s3Params, tilesRelativePath: '' })).toThrow(ValidationError); + }); + + it('should throw ValidationError when the S3 bucket is missing', () => { + expect(() => strategy.validate({ ...s3Params, bucket: undefined })).toThrow(ValidationError); + }); + + it('should throw ValidationError when the FS subPath is missing', () => { + expect(() => strategy.validate({ ...fsParams, subPath: undefined })).toThrow(ValidationError); }); it('should throw ValidationError when params is null', () => { @@ -106,16 +121,16 @@ describe('TilesDeletionStrategy', () => { expect(MockFsProvider.delete).not.toHaveBeenCalled(); }); - it('should check targetExists with S3 bucket and tilesPath as relativePath', async () => { + it('should check targetExists with S3 bucket and tilesRelativePath as relativePath', async () => { await strategy.execute(s3Params); - expect(MockS3Provider.targetExists).toHaveBeenCalledWith(S3_BUCKET, s3Params.tilesPath); + expect(MockS3Provider.targetExists).toHaveBeenCalledWith(S3_BUCKET, s3Params.tilesRelativePath); }); - it('should check targetExists with FS base path and tilesPath as relativePath', async () => { + it("should check targetExists with the task's own subPath and tilesRelativePath", async () => { await strategy.execute(fsParams); - expect(MockFsProvider.targetExists).toHaveBeenCalledWith(join(FS_BASE_PATH, FS_SUB_PATH), fsParams.tilesPath); + expect(MockFsProvider.targetExists).toHaveBeenCalledWith(FS_SUB_PATH, fsParams.tilesRelativePath); }); it('should propagate an error thrown by the target existence check', async () => { @@ -128,31 +143,49 @@ describe('TilesDeletionStrategy', () => { }); describe('provider routing', () => { - it('should call S3 provider with s3Bucket as storage target', async () => { - await strategy.execute(s3Params); + it("should call S3 provider with the task's own bucket as storage target", async () => { + const params: S3TilesDeletionParams = { ...s3Params, bucket: 'per-task-bucket' }; + + await strategy.execute(params); - expect(MockS3Provider.delete).toHaveBeenCalledWith(expect.any(Array), S3_BUCKET); + expect(MockS3Provider.delete).toHaveBeenCalledWith(expect.any(Array), 'per-task-bucket'); expect(MockFsProvider.delete).not.toHaveBeenCalled(); }); - it('should call FS provider with fsBasePath as storage target', async () => { + it("should call FS provider with the task's own subPath as storage target", async () => { await strategy.execute(fsParams); - expect(MockFsProvider.delete).toHaveBeenCalledWith(expect.any(Array), join(FS_BASE_PATH, FS_SUB_PATH)); + expect(MockFsProvider.delete).toHaveBeenCalledWith(expect.any(Array), FS_SUB_PATH); + expect(MockS3Provider.delete).not.toHaveBeenCalled(); + }); + + it('should pass the subPath through untouched rather than resolving it', async () => { + const subPath = 'some/other/mount/point'; + + await strategy.execute({ ...fsParams, subPath }); + + expect(MockFsProvider.delete).toHaveBeenCalledWith(expect.any(Array), subPath); + }); + + it('should throw UnrecoverableError for REDIS params, whose tiles are not path addressed', async () => { + const redisParams = { storageProvider: 'REDIS', prefix: 'layer-redis_WorldCRS84', ranges: s3Params.ranges }; + + await expect(strategy.execute(strategy.validate(redisParams))).rejects.toThrow(UnrecoverableError); expect(MockS3Provider.delete).not.toHaveBeenCalled(); + expect(MockFsProvider.delete).not.toHaveBeenCalled(); }); it('should throw UnrecoverableError for unknown provider', async () => { - const unknownParams = { ...s3Params, sourceProvider: 'UNKNOWN' } as unknown as TilesDeletionParams; + const unknownParams = { ...s3Params, storageProvider: 'UNKNOWN' } as unknown as S3TilesDeletionParams; await expect(strategy.execute(unknownParams)).rejects.toThrow(UnrecoverableError); }); it('should throw UnrecoverableError when the provider is registered but resolves to undefined', async () => { - const storageProviders = { + const storageProviders: StorageProviders = { [SourceType.FS]: MockFsProvider, [SourceType.S3]: undefined, - } satisfies StorageProviders; + }; strategy = new TilesDeletionStrategy( createMockLogger(), createMockStrategyConfig(), @@ -179,7 +212,7 @@ describe('TilesDeletionStrategy', () => { }); it('should use the specified file extension', async () => { - const params: TilesDeletionParams = { ...s3Params, fileExtension: 'jpeg' }; + const params: S3TilesDeletionParams = { ...s3Params, fileExtension: 'jpeg' }; await strategy.execute(params); @@ -188,7 +221,7 @@ describe('TilesDeletionStrategy', () => { }); it('should concatenate tiles from multiple ranges', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [ { zoom: 5, minX: 0, maxX: 0, minY: 0, maxY: 0 }, @@ -202,7 +235,7 @@ describe('TilesDeletionStrategy', () => { }); it('should offset x/y correctly when range does not start at 0', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 7, minX: 3, maxX: 4, minY: 8, maxY: 9 }], }; @@ -217,7 +250,7 @@ describe('TilesDeletionStrategy', () => { it('should call updateProgress mid-stream for large tile sets without ever setting 100', async () => { // batchSize=100, concurrency=2 → flush after 200 tiles, then final flush for remainder // 14 * 15 = 210 tiles - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 13, minY: 0, maxY: 14 }], }; @@ -230,7 +263,7 @@ describe('TilesDeletionStrategy', () => { it('should report the percentage of tiles processed so far', async () => { // batchSize=100, concurrency=2 → flush + progress report after the first 200 of 210 tiles - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 13, minY: 0, maxY: 14 }], }; @@ -242,7 +275,7 @@ describe('TilesDeletionStrategy', () => { it('should report progress once per completed concurrency window', async () => { // 420 tiles → two full windows of 200, then a trailing batch of 20 - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 20, minY: 0, maxY: 19 }], }; @@ -262,7 +295,7 @@ describe('TilesDeletionStrategy', () => { it('should not flush an empty trailing batch when the tile count divides evenly', async () => { // 200 tiles = exactly batchSize (100) × concurrency (2) → one window, no remainder - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; @@ -275,7 +308,7 @@ describe('TilesDeletionStrategy', () => { }); it('should pass the correct jobId and taskId on mid-stream updates', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 13, minY: 0, maxY: 14 }], }; @@ -383,7 +416,7 @@ describe('TilesDeletionStrategy', () => { it('should aggregate hard failures of the same reason across concurrent batches', async () => { // 200 tiles → two batches of 100, both rejecting with the same error - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; @@ -396,7 +429,7 @@ describe('TilesDeletionStrategy', () => { }); it('should aggregate hard failures of different reasons across concurrent batches', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; @@ -406,7 +439,7 @@ describe('TilesDeletionStrategy', () => { }); it('should surface both soft failures and hard rejections from the same flush', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; diff --git a/tests/strategyFactory.spec.ts b/tests/strategyFactory.spec.ts index 027a1f1..16c0b3b 100644 --- a/tests/strategyFactory.spec.ts +++ b/tests/strategyFactory.spec.ts @@ -7,7 +7,7 @@ import type { IStorageProvider, StorageProviders } from '@src/cleaner/storagePro import { StrategyNotFoundError } from '../src/cleaner/errors'; import { StrategyFactory, TilesDeletionStrategy, type ITaskStrategy, type TaskContext } from '../src/cleaner/strategies'; import { SERVICES } from '../src/common/constants'; -import { createMockConfig, createMockLogger, createMockQueueClient, createMockStorageProvider } from './helpers/mocks'; +import { createMockLogger, createMockQueueClient, createMockStorageProvider, createMockStrategyConfig } from './helpers/mocks'; class MockStrategy implements ITaskStrategy { public validate(params: unknown): Record { @@ -39,7 +39,7 @@ describe('StrategyFactory', () => { }; container.register(SERVICES.LOGGER, { useValue: mockLogger }); - container.register(SERVICES.CONFIG, { useValue: createMockConfig() }); + container.register(SERVICES.CONFIG, { useValue: createMockStrategyConfig() }); container.register(SERVICES.STORAGE_PROVIDERS, { useValue: storageProviders }); container.register(SERVICES.QUEUE_CLIENT, { useValue: createMockQueueClient() }); From 345cadf8e8ad47f2e650f10bae2e4b9a58da7362 Mon Sep 17 00:00:00 2001 From: almog8k Date: Thu, 6 Aug 2026 15:40:18 +0300 Subject: [PATCH 3/6] refactor(fs): extract sub path validation into a shared util isPathWithinAllowedSubPaths holds the rule that a path must sit strictly under one of the configured sub paths and still resolve inside the base path, which makes it unit testable on its own rather than only through the provider. FsStorageProvider's private check becomes an assertion instead of a boolean, so a caller cannot forget to act on the result, and the thrown message now names the offending paths, the base path and the allowed sub paths. --- .../storageProviders/fsStorageProvider.ts | 46 ++++++------- src/cleaner/utils/index.ts | 2 +- src/cleaner/utils/path.ts | 26 ++++++- tests/utils/path.spec.ts | 68 +++++++++++++++++++ 4 files changed, 116 insertions(+), 26 deletions(-) create mode 100644 tests/utils/path.spec.ts diff --git a/src/cleaner/storageProviders/fsStorageProvider.ts b/src/cleaner/storageProviders/fsStorageProvider.ts index 1e23613..e551c94 100644 --- a/src/cleaner/storageProviders/fsStorageProvider.ts +++ b/src/cleaner/storageProviders/fsStorageProvider.ts @@ -4,7 +4,7 @@ import type { Logger } from '@map-colonies/js-logger'; import type { DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; import { inject, injectable } from 'tsyringe'; import { mergeFailures, type DeleteFailure, type DeleteResult, type IStorageProvider, type StorageProvider } from '@src/cleaner/storageProviders'; -import { getChunk, normalizeFolderPath, resolveAbsolutePath } from '@src/cleaner/utils'; +import { getChunk, isPathWithinAllowedSubPaths } from '@src/cleaner/utils'; import { SERVICES } from '@common/constants'; import { describeError, UnrecoverableError } from '../errors'; import type { FsStorageConfig } from './storageConfig'; @@ -25,11 +25,9 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { * @param relativePath - Path below `subPath` to check for */ public async targetExists(subPath: string, relativePath: string): Promise { - const relativeTargetPath = join(subPath, relativePath); - if (!this.arePathsValid([relativeTargetPath])) - throw new UnrecoverableError(`Cannot act on paths outside the configured sub paths: ${relativeTargetPath}`); + this.assertPathsValid([join(subPath, relativePath)]); - const targetPath = join(this.fsConfig.basePath, relativeTargetPath); + const targetPath = join(this.fsConfig.basePath, subPath, relativePath); this.logger.debug({ msg: 'Checking if target resource exists', subPath, path: relativePath, targetPath }); try { await stat(targetPath); @@ -82,11 +80,7 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { totalFailedPathsCount = 0; const relativePaths = paths.map((path) => join(subPath, path)); - - if (!this.arePathsValid(relativePaths)) - throw new UnrecoverableError( - 'Cannot delete files/folders outside base path or subpath as well as base path or subpath itself. paths must also match a valid configured path.' - ); + this.assertPathsValid(relativePaths); let failures: DeleteFailure = new Map(); @@ -127,22 +121,26 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { } /** - * Preforms several checks on input `paths`. - * Includes a check for path traversal (i.e. accessing folders above root folder) - * @param paths - * @returns boolean whether `paths` are valid and pass all checks + * Gate for every path this provider is asked to touch. + * @param relativePaths - Paths relative to the configured base path, sub path included + * @throws {UnrecoverableError} if any path fails the check; invalid paths are a producer + * bug and will not become valid on retry */ - private arePathsValid(paths: string[]): boolean { + private assertPathsValid(paths: string[]): void { this.logger.debug({ msg: 'Checking paths validity', paths }); - const badPaths = paths.filter((path) => { - const startsWithAllowedSubPath = this.fsConfig.subPaths.some((subPath) => path.startsWith(normalizeFolderPath(subPath))); - const absolutePath = resolveAbsolutePath(join(this.fsConfig.basePath, path)); - const startsWithBasePath = absolutePath.startsWith(normalizeFolderPath(this.fsConfig.basePath)); - return !(startsWithAllowedSubPath && startsWithBasePath); - }); - const areValid = badPaths.length === 0; - this.logger.debug({ msg: `Paths validity check ${areValid ? 'succeeded' : 'failed'}`, ...(!areValid && { badPaths }) }); - return areValid; + const badPaths = paths.filter( + (path) => !isPathWithinAllowedSubPaths({ relativePath: path, basePath: this.fsConfig.basePath, allowedSubPaths: this.fsConfig.subPaths }) + ); + + if (badPaths.length > 0) { + const { basePath, subPaths } = this.fsConfig; + this.logger.error({ msg: 'Paths validity check failed', badPaths, basePath, allowedSubPaths: subPaths }); + throw new UnrecoverableError( + `Cannot delete paths outside the configured sub paths (${subPaths.join(', ')}) of base path '${basePath}', or the sub paths themselves: ${badPaths.join(', ')}` + ); + } + + this.logger.debug({ msg: 'Paths validity check succeeded' }); } // Attempts to remove any directories that became empty after file deletion. diff --git a/src/cleaner/utils/index.ts b/src/cleaner/utils/index.ts index 49d0a4f..a7eead6 100644 --- a/src/cleaner/utils/index.ts +++ b/src/cleaner/utils/index.ts @@ -1,5 +1,5 @@ export { getChunk } from './chunk'; export { assertCanDeleteFromFolder } from './fs'; export { buildPollingPairs } from './pairBuilder'; -export { normalizeFolderPath, resolveAbsolutePath } from './path'; +export { isPathWithinAllowedSubPaths, normalizeFolderPath, resolveAbsolutePath } from './path'; export { validateSchema } from './validationHelper'; diff --git a/src/cleaner/utils/path.ts b/src/cleaner/utils/path.ts index c3049ee..b59df75 100644 --- a/src/cleaner/utils/path.ts +++ b/src/cleaner/utils/path.ts @@ -1,4 +1,4 @@ -import { resolve, sep } from 'node:path/posix'; +import { join, resolve, sep } from 'node:path/posix'; export const normalizeFolderPath = (path: string): string => { return path.endsWith(sep) ? path : `${path}${sep}`; @@ -14,3 +14,27 @@ export const normalizeFolderPath = (path: string): string => { export const resolveAbsolutePath = (path: string): string => { return resolve(`${path.startsWith(sep) ? '' : sep}${path}`); }; + +/** + * Guards a caller-supplied relative path before anything is deleted from the filesystem. + * A path is allowed only when it lives strictly *under* one of the configured sub paths. + * + * @param relativePath - Path relative to `basePath`, including the sub path segment + * @param basePath - Absolute mounted base directory + * @param allowedSubPaths - Sub paths deletion is permitted under, relative to `basePath` + * @returns boolean whether `relativePath` passes both checks + */ +export const isPathWithinAllowedSubPaths = ({ + relativePath, + basePath, + allowedSubPaths, +}: { + relativePath: string; + basePath: string; + allowedSubPaths: string[]; +}): boolean => { + const startsWithAllowedSubPath = allowedSubPaths.some((subPath) => relativePath.startsWith(normalizeFolderPath(subPath))); + const absolutePath = resolveAbsolutePath(join(basePath, relativePath)); + const startsWithBasePath = absolutePath.startsWith(normalizeFolderPath(basePath)); + return startsWithAllowedSubPath && startsWithBasePath; +}; diff --git a/tests/utils/path.spec.ts b/tests/utils/path.spec.ts new file mode 100644 index 0000000..d745914 --- /dev/null +++ b/tests/utils/path.spec.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest'; +import { isPathWithinAllowedSubPaths, normalizeFolderPath, resolveAbsolutePath } from '@src/cleaner/utils/path'; + +const BASE_PATH = '/data'; +const ALLOWED_SUB_PATHS = ['artifacts/tiles', 'artifacts/gpkgs']; + +const isAllowed = (relativePath: string): boolean => + isPathWithinAllowedSubPaths({ relativePath, basePath: BASE_PATH, allowedSubPaths: ALLOWED_SUB_PATHS }); + +describe('path', () => { + describe('#normalizeFolderPath', () => { + it('should append a trailing separator when missing', () => { + expect(normalizeFolderPath('a/b')).toBe('a/b/'); + }); + + it('should leave an already terminated path unchanged', () => { + expect(normalizeFolderPath('a/b/')).toBe('a/b/'); + }); + }); + + describe('#resolveAbsolutePath', () => { + it('should prefix a relative path with the root separator', () => { + expect(resolveAbsolutePath('data/tiles')).toBe('/data/tiles'); + }); + + it('should collapse traversal segments', () => { + expect(resolveAbsolutePath('/data/tiles/../gpkgs')).toBe('/data/gpkgs'); + }); + }); + + describe('#isPathWithinAllowedSubPaths', () => { + it('should allow a path nested under an allowed sub path', () => { + expect(isAllowed('artifacts/tiles/layer/v1')).toBe(true); + }); + + it('should allow a path under any of the allowed sub paths', () => { + expect(isAllowed('artifacts/gpkgs/layer.gpkg')).toBe(true); + }); + + it('should reject an allowed sub path root itself', () => { + expect(isAllowed('artifacts/tiles')).toBe(false); + }); + + it('should reject a path under no allowed sub path', () => { + expect(isAllowed('somewhere-else/layer')).toBe(false); + }); + + it('should reject a sibling that merely shares the sub path prefix', () => { + expect(isAllowed('artifacts/tiles-old/layer')).toBe(false); + }); + + it('should reject traversal that escapes the base path', () => { + expect(isAllowed('artifacts/tiles/../../../etc/passwd')).toBe(false); + }); + + it('should reject traversal that lands back on the base path', () => { + expect(isAllowed('artifacts/tiles/../..')).toBe(false); + }); + + it('should allow traversal that stays under an allowed sub path', () => { + expect(isAllowed('artifacts/tiles/layer/../v2')).toBe(true); + }); + + it('should reject every path when no sub paths are allowed', () => { + expect(isPathWithinAllowedSubPaths({ relativePath: 'artifacts/tiles/layer', basePath: BASE_PATH, allowedSubPaths: [] })).toBe(false); + }); + }); +}); From 76e036f9822970d4d64430138d80e078606c69a7 Mon Sep 17 00:00:00 2001 From: almog8k Date: Thu, 6 Aug 2026 15:40:47 +0300 Subject: [PATCH 4/6] refactor(storage): order provider delete params target-first delete(storageTarget, paths) now reads the same way as targetExists(storageTarget, relativePath). --- .../storageProviders/fsStorageProvider.ts | 4 +- .../storageProviders/iStorageProvider.ts | 2 +- .../storageProviders/s3StorageProvider.ts | 2 +- .../strategies/tilesDeletionStrategy.ts | 2 +- .../fsStorageProvider.spec.ts | 48 +++++++++---------- .../s3StorageProvider.spec.ts | 30 ++++++------ .../strategies/tilesDeletionStrategy.spec.ts | 22 +++++---- 7 files changed, 56 insertions(+), 54 deletions(-) diff --git a/src/cleaner/storageProviders/fsStorageProvider.ts b/src/cleaner/storageProviders/fsStorageProvider.ts index e551c94..ce91394 100644 --- a/src/cleaner/storageProviders/fsStorageProvider.ts +++ b/src/cleaner/storageProviders/fsStorageProvider.ts @@ -39,10 +39,10 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { } /** - * @param paths - Paths relative to `subPath` * @param subPath - Sub path of the configured base path, as supplied by the task + * @param paths - Paths relative to `subPath` */ - public async delete(paths: string[], subPath: string): Promise { + public async delete(subPath: string, paths: string[]): Promise { this.logger.debug({ msg: 'Deleting files from filesystem', subPath, pathsCount: paths.length }); const targetPath = join(this.fsConfig.basePath, subPath); let failures: DeleteFailure = new Map(); diff --git a/src/cleaner/storageProviders/iStorageProvider.ts b/src/cleaner/storageProviders/iStorageProvider.ts index c841226..2afd85c 100644 --- a/src/cleaner/storageProviders/iStorageProvider.ts +++ b/src/cleaner/storageProviders/iStorageProvider.ts @@ -21,7 +21,7 @@ export interface IStorageProvider { * Returns an object including delete failures aggregation with one entry per failed reason. * "Not found" is reported as a failure with additional metadata on failure - count and sample */ - delete: (paths: string[], storageTarget: string) => Promise; + delete: (storageTarget: string, paths: string[]) => Promise; /** * Deletes ALL objects/files under the given paths. diff --git a/src/cleaner/storageProviders/s3StorageProvider.ts b/src/cleaner/storageProviders/s3StorageProvider.ts index 57be934..b1aa1fe 100644 --- a/src/cleaner/storageProviders/s3StorageProvider.ts +++ b/src/cleaner/storageProviders/s3StorageProvider.ts @@ -45,7 +45,7 @@ export class S3StorageProvider implements IStorageProvider { + public async delete(bucket: string, paths: string[]): Promise { this.logger.debug({ msg: 'Deleting objects from S3', bucket, pathsCount: paths.length }); let failures: DeleteFailure = new Map(); diff --git a/src/cleaner/strategies/tilesDeletionStrategy.ts b/src/cleaner/strategies/tilesDeletionStrategy.ts index 24f94c2..5d839d8 100644 --- a/src/cleaner/strategies/tilesDeletionStrategy.ts +++ b/src/cleaner/strategies/tilesDeletionStrategy.ts @@ -184,7 +184,7 @@ export class TilesDeletionStrategy implements ITaskStrategy let failures: DeleteFailure = new Map(); const processedTilesCount = pendingBatches.reduce((sum, b) => sum + b.length, 0); - const results = await Promise.allSettled(pendingBatches.map(async (batch) => provider.delete(batch, storageTarget))); + const results = await Promise.allSettled(pendingBatches.map(async (batch) => provider.delete(storageTarget, batch))); for (const [index, result] of results.entries()) { if (result.status === 'fulfilled') { failures = mergeFailures({ source: result.value.failures, target: failures }); diff --git a/tests/storageProviders/fsStorageProvider.spec.ts b/tests/storageProviders/fsStorageProvider.spec.ts index 98b4c56..f48ac8b 100644 --- a/tests/storageProviders/fsStorageProvider.spec.ts +++ b/tests/storageProviders/fsStorageProvider.spec.ts @@ -88,19 +88,19 @@ describe('FsStorageProvider', () => { describe('#delete', () => { it('should return empty failures map for empty input', async () => { - const result = await provider.delete([], SUB_PATH); + const result = await provider.delete(SUB_PATH, []); expect(result).toEqual({ failures: new Map() }); expect(unlink).not.toHaveBeenCalled(); }); it('should call unlink with joined base path and relative path', async () => { - await provider.delete(['layer/v1/10/0/0.png'], SUB_PATH); + await provider.delete(SUB_PATH, ['layer/v1/10/0/0.png']); expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0/0.png')); }); it('should join a nested sub path onto the base path', async () => { - await provider.delete(['tile/10/0/0.png'], `${SUB_PATH}/nested`); + await provider.delete(`${SUB_PATH}/nested`, ['tile/10/0/0.png']); expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'nested', 'tile/10/0/0.png')); }); @@ -108,7 +108,7 @@ describe('FsStorageProvider', () => { it('should call unlink for every path', async () => { const paths = ['tile/10/0/0.png', 'tile/10/0/1.png', 'tile/10/1/0.png']; - await provider.delete(paths, SUB_PATH); + await provider.delete(SUB_PATH, paths); expect(unlink).toHaveBeenCalledTimes(3); for (const p of paths) { @@ -117,7 +117,7 @@ describe('FsStorageProvider', () => { }); it('should return empty failures map when all unlinks succeed', async () => { - const result = await provider.delete(['tile/10/0/0.png', 'tile/10/0/1.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png', 'tile/10/0/1.png']); expect(result).toEqual({ failures: new Map() }); }); @@ -125,7 +125,7 @@ describe('FsStorageProvider', () => { const enoent = Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); vi.mocked(unlink).mockRejectedValue(enoent); - const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['ENOENT', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -134,7 +134,7 @@ describe('FsStorageProvider', () => { const permError = Object.assign(new Error('EACCES'), { code: 'EACCES' }); vi.mocked(unlink).mockRejectedValue(permError); - const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -142,7 +142,7 @@ describe('FsStorageProvider', () => { it('should fall back to error message when error has no errno code', async () => { vi.mocked(unlink).mockRejectedValue(new Error('disk on fire')); - const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['disk on fire', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -150,7 +150,7 @@ describe('FsStorageProvider', () => { it('should fall back to "Unknown" when error has neither errno code nor message', async () => { vi.mocked(unlink).mockRejectedValue(new Error('')); - const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -158,7 +158,7 @@ describe('FsStorageProvider', () => { it('should tag failures with the stringified value when a non-Error is thrown', async () => { vi.mocked(unlink).mockRejectedValue('raw string failure'); - const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['raw string failure', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -166,7 +166,7 @@ describe('FsStorageProvider', () => { it('should fall back to "Unknown" when a non-Error empty value is thrown', async () => { vi.mocked(unlink).mockRejectedValue(''); - const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -178,7 +178,7 @@ describe('FsStorageProvider', () => { }, }); - const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['non-serializable thrown value', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -186,7 +186,7 @@ describe('FsStorageProvider', () => { it('should unlink every path of a large input', async () => { const paths = Array.from({ length: 7 }, (_, i) => `tile/10/0/${i}.png`); - await provider.delete(paths, SUB_PATH); + await provider.delete(SUB_PATH, paths); expect(unlink).toHaveBeenCalledTimes(7); for (const path of paths) { @@ -199,7 +199,7 @@ describe('FsStorageProvider', () => { vi.mocked(unlink).mockRejectedValue(permError); const paths = Array.from({ length: 7 }, (_, i) => `tile/10/0/${i}.png`); - const result = await provider.delete(paths, SUB_PATH); + const result = await provider.delete(SUB_PATH, paths); expect(result).toEqual({ failures: new Map([['EACCES', { count: 7, sample: 'tile/10/0/0.png' }]]) }); }); @@ -214,7 +214,7 @@ describe('FsStorageProvider', () => { .mockRejectedValueOnce(permError); // real error → failure const paths = ['tile/10/0/0.png', 'tile/10/0/1.png', 'tile/10/0/2.png']; - const result = await provider.delete(paths, SUB_PATH); + const result = await provider.delete(SUB_PATH, paths); expect(result).toEqual({ failures: new Map([ @@ -229,7 +229,7 @@ describe('FsStorageProvider', () => { vi.mocked(unlink).mockRejectedValue(permError); const relativePath = 'layer/v1/10/5/3.png'; - const result = await provider.delete([relativePath], SUB_PATH); + const result = await provider.delete(SUB_PATH, [relativePath]); expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: relativePath }]]) }); expect(Array.from(result.failures.values())[0]?.sample).not.toMatch(`^${BASE_PATH}*`); @@ -237,13 +237,13 @@ describe('FsStorageProvider', () => { describe('cleanupEmptyDirs', () => { it('should attempt to rmdir the parent directory after deletion', async () => { - await provider.delete(['layer/v1/10/0/0.png'], SUB_PATH); + await provider.delete(SUB_PATH, ['layer/v1/10/0/0.png']); expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0')); }); it('should attempt to rmdir all ancestor directories bottom-up', async () => { - await provider.delete(['layer/v1/10/0/0.png'], SUB_PATH); + await provider.delete(SUB_PATH, ['layer/v1/10/0/0.png']); // x dir → zoom dir → version dir → layer dir (deepest first) expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0')); @@ -254,7 +254,7 @@ describe('FsStorageProvider', () => { it('should deduplicate rmdir calls for shared parent directories', async () => { // Both tiles share the same x-dir and zoom dir - await provider.delete(['tile/10/0/0.png', 'tile/10/0/1.png'], SUB_PATH); + await provider.delete(SUB_PATH, ['tile/10/0/0.png', 'tile/10/0/1.png']); const rmdirCalls = vi.mocked(rmdir).mock.calls.map(([p]) => p); const xDirCalls = rmdirCalls.filter((p) => p === join(BASE_PATH, SUB_PATH, 'tile/10/0')); @@ -265,19 +265,19 @@ describe('FsStorageProvider', () => { const enotempty = Object.assign(new Error('ENOTEMPTY'), { code: 'ENOTEMPTY' }); vi.mocked(rmdir).mockRejectedValue(enotempty); - const result = await provider.delete(['tile/10/0/0.png'], SUB_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); // Should not throw and should return correct failed paths expect(result).toEqual({ failures: new Map() }); }); it('should not call rmdir when input is empty', async () => { - await provider.delete([], SUB_PATH); + await provider.delete(SUB_PATH, []); expect(rmdir).not.toHaveBeenCalled(); }); it('should not call rmdir for a path that has no directory segments', async () => { - await provider.delete(['0.png'], SUB_PATH); + await provider.delete(SUB_PATH, ['0.png']); expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, '0.png')); expect(rmdir).not.toHaveBeenCalled(); @@ -287,7 +287,7 @@ describe('FsStorageProvider', () => { // batchSize is 3 → cleanup runs once for all paths, after the last batch const paths = Array.from({ length: 4 }, (_, i) => `tile/10/${i}/0.png`); - await provider.delete(paths, SUB_PATH); + await provider.delete(SUB_PATH, paths); for (let i = 0; i < 4; i++) { expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, `tile/10/${i}`)); @@ -297,7 +297,7 @@ describe('FsStorageProvider', () => { }); it('should attempt to rmdir deeper directories before their ancestors', async () => { - await provider.delete(['layer/v1/10/0/0.png'], SUB_PATH); + await provider.delete(SUB_PATH, ['layer/v1/10/0/0.png']); const order = vi.mocked(rmdir).mock.calls.map(([path]) => path); expect(order).toEqual([ diff --git a/tests/storageProviders/s3StorageProvider.spec.ts b/tests/storageProviders/s3StorageProvider.spec.ts index 50c9cd7..f0a168d 100644 --- a/tests/storageProviders/s3StorageProvider.spec.ts +++ b/tests/storageProviders/s3StorageProvider.spec.ts @@ -77,7 +77,7 @@ describe('S3StorageProvider', () => { }); it('should return empty failures map for empty input', async () => { - const result = await provider.delete([], BUCKET); + const result = await provider.delete(BUCKET, []); expect(result).toEqual({ failures: new Map() }); expect(mockSend).not.toHaveBeenCalled(); }); @@ -85,7 +85,7 @@ describe('S3StorageProvider', () => { it('should send DeleteObjectsCommand with correct keys', async () => { const paths = ['folder/a.txt', 'folder/b.txt']; - await provider.delete(paths, BUCKET); + await provider.delete(BUCKET, paths); expect(DeleteObjectsCommand).toHaveBeenCalledWith({ Bucket: BUCKET, @@ -97,7 +97,7 @@ describe('S3StorageProvider', () => { it('should return empty failures map when all deletes succeed', async () => { const paths = ['a.txt', 'b.txt']; - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map() }); }); @@ -108,7 +108,7 @@ describe('S3StorageProvider', () => { }); const paths = ['a.txt', 'b.txt']; - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'a.txt' }]]) }); }); @@ -119,7 +119,7 @@ describe('S3StorageProvider', () => { }); const paths = ['missing.txt']; - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map([['NoSuchKey', { count: 1, sample: 'missing.txt' }]]) }); }); @@ -129,7 +129,7 @@ describe('S3StorageProvider', () => { Errors: [{ Key: 'a.txt', Message: 'Something bad' }], }); - const result = await provider.delete(['a.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt']); expect(result).toEqual({ failures: new Map([['Something bad', { count: 1, sample: 'a.txt' }]]) }); }); @@ -139,7 +139,7 @@ describe('S3StorageProvider', () => { Errors: [{ Key: 'a.txt' }], }); - const result = await provider.delete(['a.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt']); expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'a.txt' }]]) }); }); @@ -154,7 +154,7 @@ describe('S3StorageProvider', () => { ], }); - const result = await provider.delete(['a.txt', 'b.txt', 'c.txt', 'd.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt', 'b.txt', 'c.txt', 'd.txt']); expect(result).toEqual({ failures: new Map([ @@ -169,7 +169,7 @@ describe('S3StorageProvider', () => { const paths = Array.from({ length: 1500 }, (_, i) => `object-${i}.txt`); provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 1000 }), mockLogger); - await provider.delete(paths, BUCKET); + await provider.delete(BUCKET, paths); expect(mockSend).toHaveBeenCalledTimes(2); const firstCallInput = vi.mocked(DeleteObjectsCommand).mock.calls[0]![0] as { @@ -186,7 +186,7 @@ describe('S3StorageProvider', () => { const paths = Array.from({ length: 2500 }, (_, i) => `object-${i}.txt`); provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 1000 }), mockLogger); - await provider.delete(paths, BUCKET); + await provider.delete(BUCKET, paths); expect(mockSend).toHaveBeenCalledTimes(3); const callInputs = vi.mocked(DeleteObjectsCommand).mock.calls.map((call) => call[0] as { Delete: { Objects: { Key: string }[] } }); @@ -200,7 +200,7 @@ describe('S3StorageProvider', () => { .mockResolvedValueOnce({ Errors: [{ Key: 'object-0.txt', Code: 'AccessDenied' }] }) .mockResolvedValueOnce({ Errors: [{ Key: 'object-1000.txt', Code: 'AccessDenied' }] }); - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 2, sample: 'object-0.txt' }]]) }); }); @@ -210,7 +210,7 @@ describe('S3StorageProvider', () => { Errors: [{ Code: 'InternalError' }, { Key: 'b.txt', Code: 'AccessDenied' }], }); - const result = await provider.delete(['a.txt', 'b.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt', 'b.txt']); expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'b.txt' }]]) }); }); @@ -218,7 +218,7 @@ describe('S3StorageProvider', () => { it('should return an empty failures map when every returned error carries no Key', async () => { mockSend.mockResolvedValue({ Errors: [{ Code: 'InternalError' }] }); - const result = await provider.delete(['a.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt']); expect(result).toEqual({ failures: new Map() }); }); @@ -226,7 +226,7 @@ describe('S3StorageProvider', () => { it('should tag the chunk with the stringified value when send rejects with a non-Error', async () => { mockSend.mockRejectedValue('connection reset'); - const result = await provider.delete(['a.txt', 'b.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt', 'b.txt']); expect(result).toEqual({ failures: new Map([['connection reset', { count: 2, sample: 'a.txt' }]]) }); }); @@ -235,7 +235,7 @@ describe('S3StorageProvider', () => { mockSend.mockRejectedValue(new Error('Network error')); const paths = ['a.txt', 'b.txt']; - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map([['Network error', { count: 2, sample: 'a.txt' }]]) }); }); diff --git a/tests/strategies/tilesDeletionStrategy.spec.ts b/tests/strategies/tilesDeletionStrategy.spec.ts index e80df44..3121265 100644 --- a/tests/strategies/tilesDeletionStrategy.spec.ts +++ b/tests/strategies/tilesDeletionStrategy.spec.ts @@ -148,14 +148,14 @@ describe('TilesDeletionStrategy', () => { await strategy.execute(params); - expect(MockS3Provider.delete).toHaveBeenCalledWith(expect.any(Array), 'per-task-bucket'); + expect(MockS3Provider.delete).toHaveBeenCalledWith('per-task-bucket', expect.any(Array)); expect(MockFsProvider.delete).not.toHaveBeenCalled(); }); it("should call FS provider with the task's own subPath as storage target", async () => { await strategy.execute(fsParams); - expect(MockFsProvider.delete).toHaveBeenCalledWith(expect.any(Array), FS_SUB_PATH); + expect(MockFsProvider.delete).toHaveBeenCalledWith(FS_SUB_PATH, expect.any(Array)); expect(MockS3Provider.delete).not.toHaveBeenCalled(); }); @@ -164,7 +164,7 @@ describe('TilesDeletionStrategy', () => { await strategy.execute({ ...fsParams, subPath }); - expect(MockFsProvider.delete).toHaveBeenCalledWith(expect.any(Array), subPath); + expect(MockFsProvider.delete).toHaveBeenCalledWith(subPath, expect.any(Array)); }); it('should throw UnrecoverableError for REDIS params, whose tiles are not path addressed', async () => { @@ -205,10 +205,12 @@ describe('TilesDeletionStrategy', () => { await strategy.execute(s3Params); // range: minX=0,maxX=1 minY=0,maxY=1 → 4 tiles, x iterates outer - expect(MockS3Provider.delete).toHaveBeenCalledWith( - [tilePath(10, 0, 0), tilePath(10, 0, 1), tilePath(10, 1, 0), tilePath(10, 1, 1)], - S3_BUCKET - ); + expect(MockS3Provider.delete).toHaveBeenCalledWith(S3_BUCKET, [ + tilePath(10, 0, 0), + tilePath(10, 0, 1), + tilePath(10, 1, 0), + tilePath(10, 1, 1), + ]); }); it('should use the specified file extension', async () => { @@ -216,7 +218,7 @@ describe('TilesDeletionStrategy', () => { await strategy.execute(params); - const [paths] = vi.mocked(MockS3Provider.delete).mock.calls[0]!; + const [, paths] = vi.mocked(MockS3Provider.delete).mock.calls[0]!; expect(paths.every((p) => p.endsWith('.jpeg'))).toBe(true); }); @@ -231,7 +233,7 @@ describe('TilesDeletionStrategy', () => { await strategy.execute(params); - expect(MockS3Provider.delete).toHaveBeenCalledWith([tilePath(5, 0, 0), tilePath(6, 0, 0)], S3_BUCKET); + expect(MockS3Provider.delete).toHaveBeenCalledWith(S3_BUCKET, [tilePath(5, 0, 0), tilePath(6, 0, 0)]); }); it('should offset x/y correctly when range does not start at 0', async () => { @@ -242,7 +244,7 @@ describe('TilesDeletionStrategy', () => { await strategy.execute(params); - expect(MockS3Provider.delete).toHaveBeenCalledWith([tilePath(7, 3, 8), tilePath(7, 3, 9), tilePath(7, 4, 8), tilePath(7, 4, 9)], S3_BUCKET); + expect(MockS3Provider.delete).toHaveBeenCalledWith(S3_BUCKET, [tilePath(7, 3, 8), tilePath(7, 3, 9), tilePath(7, 4, 8), tilePath(7, 4, 9)]); }); }); From c1021d40a8137b4269b8c1d1e1fb9a9f8d35c75c Mon Sep 17 00:00:00 2001 From: almog8k Date: Thu, 6 Aug 2026 15:41:02 +0300 Subject: [PATCH 5/6] chore(config): drop dead tiles-deletion locator config strategies.tilesDeletion.s3Bucket and .fsSubPath are no longer read now that the locator travels with the task, so they go along with their configmap entries (TILES_DELETION_S3_BUCKET, TILES_DELETION_FS_SUB_PATH) and the s3.tilesBucket value that fed the former. storage.fs.subPaths stays: it is the allowlist FS deletion targets are checked against. cleanupStorageProviders now defaults to empty so a deployment states its providers explicitly. --- config/custom-environment-variables.json | 2 -- config/default.json | 2 -- helm/templates/configmap.yaml | 3 --- helm/values.yaml | 5 +---- 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/config/custom-environment-variables.json b/config/custom-environment-variables.json index 1ba7871..67aa92f 100644 --- a/config/custom-environment-variables.json +++ b/config/custom-environment-variables.json @@ -114,8 +114,6 @@ }, "strategies": { "tilesDeletion": { - "s3Bucket": "TILES_DELETION_S3_BUCKET", - "fsSubPath": "TILES_DELETION_FS_SUB_PATH", "batchSize": { "__name": "TILES_DELETION_BATCH_SIZE", "__format": "number" diff --git a/config/default.json b/config/default.json index 4d97d6d..fe636a9 100644 --- a/config/default.json +++ b/config/default.json @@ -105,8 +105,6 @@ }, "strategies": { "tilesDeletion": { - "s3Bucket": "", - "fsSubPath": "tiles", "batchSize": 1000, "concurrency": 10 } diff --git a/helm/templates/configmap.yaml b/helm/templates/configmap.yaml index 27a5aff..95bc4c1 100644 --- a/helm/templates/configmap.yaml +++ b/helm/templates/configmap.yaml @@ -5,7 +5,6 @@ {{- $fs := ($storage.fs) | default dict -}} {{- $internalPvc := (($fs).internalPvc) | default dict -}} {{- $fsBasePath := clean (printf "/%s" $internalPvc.mountPath) -}} -{{- $fsTilesDeletionSubPath := clean (printf "%s" ($internalPvc.tilesSubPath | default "tiles")) -}} {{- if .Values.enabled -}} apiVersion: v1 kind: ConfigMap @@ -74,9 +73,7 @@ data: {{- end -}} {{- end }} FS_SUB_PATHS: {{ $subPaths | toJson | quote }} - TILES_DELETION_FS_SUB_PATH: {{ $fsTilesDeletionSubPath | quote }} {{- end }} - TILES_DELETION_S3_BUCKET: {{ $s3.tilesBucket | default "" | quote }} {{- with .Values.env.strategies.tilesDeletion }} TILES_DELETION_BATCH_SIZE: {{ .batchSize | default 1000 | quote }} TILES_DELETION_CONCURRENCY: {{ .concurrency | default 10 | quote }} diff --git a/helm/values.yaml b/helm/values.yaml index fe496fb..376c021 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -13,9 +13,7 @@ serviceUrls: jobTracker: "" storage: - cleanupStorageProviders: - - FS - - S3 + cleanupStorageProviders: {} s3: delete: batchSize: 1000 @@ -24,7 +22,6 @@ storage: secretName: "" sslEnabled: false region: "" - tilesBucket: "" fs: delete: batchSize: 1000 From a7ce00f17b87fb11297614fdc4d0f7a2693860b4 Mon Sep 17 00:00:00 2001 From: almog8k Date: Thu, 6 Aug 2026 16:27:47 +0300 Subject: [PATCH 6/6] fix: add resolveAbsolutePath utility to fsStorageProvider imports --- src/cleaner/storageProviders/fsStorageProvider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cleaner/storageProviders/fsStorageProvider.ts b/src/cleaner/storageProviders/fsStorageProvider.ts index e3968a5..89e3a57 100644 --- a/src/cleaner/storageProviders/fsStorageProvider.ts +++ b/src/cleaner/storageProviders/fsStorageProvider.ts @@ -4,7 +4,7 @@ import type { Logger } from '@map-colonies/js-logger'; import type { DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; import { inject, injectable } from 'tsyringe'; import { mergeFailures, type DeleteFailure, type DeleteResult, type IStorageProvider, type StorageProvider } from '@src/cleaner/storageProviders'; -import { getChunk, isPathWithinAllowedSubPaths } from '@src/cleaner/utils'; +import { getChunk, isPathWithinAllowedSubPaths, resolveAbsolutePath } from '@src/cleaner/utils'; import { SERVICES } from '@common/constants'; import { describeError, UnrecoverableError } from '../errors'; import type { FsStorageConfig } from './storageConfig';