From e021082ecf651a679bc364d8a13eb4abf675b739 Mon Sep 17 00:00:00 2001 From: almog8k Date: Mon, 7 Sep 2026 15:27:19 +0300 Subject: [PATCH 01/27] feat: add redis storage configuration (MAPCO-11263) Adds the ioredis dependency and the storage.redis config block, following the existing buildFsStorageConfig / buildS3StorageConfig pattern: a single place where the untyped config.get() cast happens, the delete.batchSize is hoisted flat, and bad values fail at boot rather than on the first task. scanCount is the SCAN COUNT hint. It is validated as > 0 because Redis rejects COUNT 0 with 'ERR syntax error' rather than falling back to a default, which would otherwise fail every prefix wipe at task time. Credentials are logged field by field rather than spread, so a configured password never reaches the logs. REDIS is deliberately left out of the default cleanupStorageProviders: startup is fail-fast, so enabling it by default would impose a hard Redis dependency before any task can dispatch to it (CL-3). --- config/custom-environment-variables.json | 27 ++++++++ config/default.json | 9 +++ package-lock.json | 61 +++++++++++++++++++ package.json | 1 + src/cleaner/storageProviders/index.ts | 3 + src/cleaner/storageProviders/storageConfig.ts | 53 ++++++++++++++++ tests/helpers/mocks.ts | 41 ++++++++++++- tests/storageProviders/storageConfig.spec.ts | 42 ++++++++++++- 8 files changed, 235 insertions(+), 2 deletions(-) diff --git a/config/custom-environment-variables.json b/config/custom-environment-variables.json index 67aa92f..779709b 100644 --- a/config/custom-environment-variables.json +++ b/config/custom-environment-variables.json @@ -110,6 +110,33 @@ "__name": "FS_SUB_PATHS", "__format": "json" } + }, + "redis": { + "delete": { + "batchSize": { + "__name": "REDIS_DELETE_BATCH_SIZE", + "__format": "number" + } + }, + "host": "REDIS_HOST", + "port": { + "__name": "REDIS_PORT", + "__format": "number" + }, + "db": { + "__name": "REDIS_DB", + "__format": "number" + }, + "scanCount": { + "__name": "REDIS_SCAN_COUNT", + "__format": "number" + }, + "username": "REDIS_USERNAME", + "password": "REDIS_PASSWORD", + "tlsEnabled": { + "__name": "REDIS_TLS_ENABLED", + "__format": "boolean" + } } }, "strategies": { diff --git a/config/default.json b/config/default.json index 96a6b10..482974b 100644 --- a/config/default.json +++ b/config/default.json @@ -97,6 +97,15 @@ "subPaths": { "tilesSubPath": "tiles" } + }, + "redis": { + "delete": { + "batchSize": 1000 + }, + "host": "localhost", + "port": 6379, + "db": 0, + "scanCount": 1000 } }, "strategies": { diff --git a/package-lock.json b/package-lock.json index d97d035..d3d5f50 100644 --- a/package-lock.json +++ b/package-lock.json @@ -23,6 +23,7 @@ "@opentelemetry/api": "^1.9.0", "compression": "^1.8.0", "express": "^4.21.2", + "ioredis": "^6.0.0", "prom-client": "^15.1.3", "reflect-metadata": "^0.2.2", "tsyringe": "^4.8.0", @@ -2080,6 +2081,12 @@ "url": "https://github.com/sponsors/nzakas" } }, + "node_modules/@ioredis/commands": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@ioredis/commands/-/commands-2.0.0.tgz", + "integrity": "sha512-vrx0AE/T0h7cRZwfo1M39Cr+ZhZrkf0V8mQN75wucKCxCLD9l/VX6no3gFvrLqD1IlG/1LtzWovqEw3t0Vr9zg==", + "license": "MIT" + }, "node_modules/@isaacs/balanced-match": { "version": "4.0.1", "resolved": "https://registry.npmjs.org/@isaacs/balanced-match/-/balanced-match-4.0.1.tgz", @@ -9051,6 +9058,15 @@ "node": ">=12" } }, + "node_modules/cluster-key-slot": { + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/cluster-key-slot/-/cluster-key-slot-1.1.1.tgz", + "integrity": "sha512-rwHwUfXL40Chm1r08yrhU3qpUvdVlgkKNeyeGPOxnW8/SyVDvgRaed/Uz54AqWNaTCAThlj6QAs3TZcKI0xDEw==", + "license": "Apache-2.0", + "engines": { + "node": ">=0.10.0" + } + }, "node_modules/code-block-writer": { "version": "12.0.0", "resolved": "https://registry.npmjs.org/code-block-writer/-/code-block-writer-12.0.0.tgz", @@ -9709,6 +9725,15 @@ "node": ">=0.4.0" } }, + "node_modules/denque": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/denque/-/denque-2.1.0.tgz", + "integrity": "sha512-HVQE3AAb/pxF8fQAoiqpvg9i3evqug3hoiwakOyZAwJm+6vZehbkYXZ0l4JxS+I3QxM97v5aaRNhj8v5oBhekw==", + "license": "Apache-2.0", + "engines": { + "node": ">=0.10" + } + }, "node_modules/density-clustering": { "version": "1.3.0", "resolved": "https://registry.npmjs.org/density-clustering/-/density-clustering-1.3.0.tgz", @@ -11592,6 +11617,27 @@ "node": "^14.17.0 || ^16.13.0 || >=18.0.0" } }, + "node_modules/ioredis": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/ioredis/-/ioredis-6.0.0.tgz", + "integrity": "sha512-f+Dtubxfpf6KYFq7WVXJoOLn0bk4TJrMrN9SzeE+jrWrCWj7XX3fA6vkryafhADX+GMymRxgDJDOI33COkJc0w==", + "license": "MIT", + "dependencies": { + "@ioredis/commands": "2.0.0", + "cluster-key-slot": "1.1.1", + "debug": "4.4.3", + "denque": "2.1.0", + "redis-errors": "1.2.0", + "standard-as-callback": "2.1.0" + }, + "engines": { + "node": ">=20.0.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/ioredis" + } + }, "node_modules/ipaddr.js": { "version": "1.9.1", "resolved": "https://registry.npmjs.org/ipaddr.js/-/ipaddr.js-1.9.1.tgz", @@ -13851,6 +13897,15 @@ "node": ">= 12.13.0" } }, + "node_modules/redis-errors": { + "version": "1.2.0", + "resolved": "https://registry.npmjs.org/redis-errors/-/redis-errors-1.2.0.tgz", + "integrity": "sha512-1qny3OExCf0UvUV/5wpYKf2YwPcOqXzkwKKSmKHiE6ZMQs5heeE/c8eXK+PNllPvmjgAbfnsbpkGZWy8cBpn9w==", + "license": "MIT", + "engines": { + "node": ">=4" + } + }, "node_modules/reflect-metadata": { "version": "0.2.2", "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.2.2.tgz", @@ -14537,6 +14592,12 @@ "dev": true, "license": "MIT" }, + "node_modules/standard-as-callback": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/standard-as-callback/-/standard-as-callback-2.1.0.tgz", + "integrity": "sha512-qoRRSyROncaz1z0mvYqIE4lCd9p2R90i6GxW3uZv5ucSu8tU7B5HXUP1gG8pVZsYNVaXjk8ClXHPttLyxAL48A==", + "license": "MIT" + }, "node_modules/statuses": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/statuses/-/statuses-2.0.1.tgz", diff --git a/package.json b/package.json index 61e1672..1c68732 100644 --- a/package.json +++ b/package.json @@ -46,6 +46,7 @@ "@opentelemetry/api": "^1.9.0", "compression": "^1.8.0", "express": "^4.21.2", + "ioredis": "^6.0.0", "prom-client": "^15.1.3", "reflect-metadata": "^0.2.2", "tsyringe": "^4.8.0", diff --git a/src/cleaner/storageProviders/index.ts b/src/cleaner/storageProviders/index.ts index 43f390e..42e40c2 100644 --- a/src/cleaner/storageProviders/index.ts +++ b/src/cleaner/storageProviders/index.ts @@ -4,9 +4,12 @@ export type { DeleteFailure, DeleteResult, IStorageProvider, StorageProvider, St export { S3StorageProvider } from './s3StorageProvider'; export { buildFsStorageConfig, + buildRedisStorageConfig, buildS3StorageConfig, type FsConfig, type FsStorageConfig, + type RedisConfig, + type RedisStorageConfig, type S3Config, type S3StorageConfig, } from './storageConfig'; diff --git a/src/cleaner/storageProviders/storageConfig.ts b/src/cleaner/storageProviders/storageConfig.ts index 8bd2f8b..2906491 100644 --- a/src/cleaner/storageProviders/storageConfig.ts +++ b/src/cleaner/storageProviders/storageConfig.ts @@ -42,6 +42,30 @@ export interface S3StorageConfig { batchSize: number; } +export interface RedisConfig { + delete: { + batchSize: number; + }; + host: string; + port: number; + db: number; + scanCount: number; + username?: string; + password?: string; + tlsEnabled?: boolean; +} + +export interface RedisStorageConfig { + host: string; + port: number; + db: number; + scanCount: number; + username?: string; + password?: string; + tlsEnabled?: boolean; + batchSize: number; +} + /** * Reads and validates `storage.fs`. * @returns {FsStorageConfig} FS configuration for FsStorageProvider @@ -90,3 +114,32 @@ export function buildS3StorageConfig(config: ConfigType, logger: Logger): S3Stor batchSize, }; } + +/** + * Reads and validates `storage.redis`. + * @returns {RedisStorageConfig} Redis configuration for RedisStorageProvider + * @throws {ConfigurationError} if the config is unusable + */ +export function buildRedisStorageConfig(config: ConfigType, logger: Logger): RedisStorageConfig { + //TODO: when we create a worker config schema the shape checks below can be dropped along with the cast + const redisConfig = config.get('storage.redis') as unknown as RedisConfig; + + const { + delete: { batchSize: deleteBatchSize }, + ...redisStorageConfig + } = redisConfig; + + if (deleteBatchSize <= 0) throw new ConfigurationError('Deletion batch size must be greater than 0'); + if (redisStorageConfig.scanCount <= 0) throw new ConfigurationError('Redis scan count must be greater than 0'); + + // Logged field by field rather than spread, so credentials never reach the logs. + logger.info({ + msg: 'Validated Redis storage config', + host: redisStorageConfig.host, + port: redisStorageConfig.port, + db: redisStorageConfig.db, + scanCount: redisStorageConfig.scanCount, + batchSize: deleteBatchSize, + }); + return { ...redisStorageConfig, batchSize: deleteBatchSize }; +} diff --git a/tests/helpers/mocks.ts b/tests/helpers/mocks.ts index 9f14b46..daf0390 100644 --- a/tests/helpers/mocks.ts +++ b/tests/helpers/mocks.ts @@ -2,7 +2,14 @@ import type { Logger } from '@map-colonies/js-logger'; import type { TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; import { vi } from 'vitest'; import type { IStorageProvider, StorageProvider } from '@src/cleaner/storageProviders/iStorageProvider'; -import type { FsConfig, FsStorageConfig, S3Config, S3StorageConfig } from '@src/cleaner/storageProviders/storageConfig'; +import type { + FsConfig, + FsStorageConfig, + RedisConfig, + RedisStorageConfig, + S3Config, + S3StorageConfig, +} from '@src/cleaner/storageProviders/storageConfig'; import type { ErrorHandler } from '../../src/cleaner/errors'; import type { JobTrackerClient } from '../../src/cleaner/httpClients'; import type { ITaskStrategy, StrategyFactory } from '../../src/cleaner/strategies'; @@ -168,6 +175,38 @@ export function createFsStorageConfig(overrides: Partial = {}): return { ...FS_VALIDATED_CONFIG_DEFAULTS, ...overrides }; } +// ─── Redis Storage Config (RedisStorageProvider) ───────────────────────────── + +export const REDIS_STORAGE_CONFIG_DEFAULTS = { + delete: { + batchSize: 3, + }, + host: 'localhost', + port: 6379, + db: 0, + scanCount: 10, +} as const satisfies RedisConfig; + +export function createMockRedisConfig(overrides: Record = {}): ConfigType { + return { + get: vi.fn().mockReturnValue({ ...REDIS_STORAGE_CONFIG_DEFAULTS, ...overrides }), + } as unknown as ConfigType; +} + +// ─── Redis Validated Storage Config ────────────────────────────────────────── + +export const REDIS_VALIDATED_CONFIG_DEFAULTS = { + host: REDIS_STORAGE_CONFIG_DEFAULTS.host, + port: REDIS_STORAGE_CONFIG_DEFAULTS.port, + db: REDIS_STORAGE_CONFIG_DEFAULTS.db, + scanCount: REDIS_STORAGE_CONFIG_DEFAULTS.scanCount, + batchSize: REDIS_STORAGE_CONFIG_DEFAULTS.delete.batchSize, +} as const satisfies RedisStorageConfig; + +export function createRedisStorageConfig(overrides: Partial = {}): RedisStorageConfig { + return { ...REDIS_VALIDATED_CONFIG_DEFAULTS, ...overrides }; +} + // ─── JobTrackerClient ───────────────────────────────────────────────────────── export function createMockJobTrackerClient(): JobTrackerClient { diff --git a/tests/storageProviders/storageConfig.spec.ts b/tests/storageProviders/storageConfig.spec.ts index a427b7e..3eff463 100644 --- a/tests/storageProviders/storageConfig.spec.ts +++ b/tests/storageProviders/storageConfig.spec.ts @@ -2,14 +2,16 @@ import type { Logger } from '@map-colonies/js-logger'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { faker } from '@faker-js/faker'; import { ConfigurationError } from '@src/cleaner/errors'; -import { buildFsStorageConfig, buildS3StorageConfig, type FsConfig } from '@src/cleaner/storageProviders/storageConfig'; +import { buildFsStorageConfig, buildRedisStorageConfig, buildS3StorageConfig, type FsConfig } from '@src/cleaner/storageProviders/storageConfig'; import { assertCanDeleteFromFolder } from '@src/cleaner/utils/fs'; import type { ConfigType } from '@src/common/config'; import { createFsStorageConfig, createMockFsConfig, createMockLogger, + createMockRedisConfig, createMockS3Config, + createRedisStorageConfig, createS3StorageConfig, FS_STORAGE_CONFIG_DEFAULTS, } from '../helpers/mocks'; @@ -133,4 +135,42 @@ describe('storageConfig', () => { expect(() => buildS3StorageConfig(config, mockLogger)).toThrow(ConfigurationError); }); }); + + describe('#buildRedisStorageConfig', () => { + it('should return the validated config with the delete batch size hoisted', () => { + const config = createMockRedisConfig(); + + const result = buildRedisStorageConfig(config, mockLogger); + + expect(result).toStrictEqual(createRedisStorageConfig()); + }); + + it('should carry optional credentials and tls through', () => { + const config = createMockRedisConfig({ username: 'user', password: 'secret', tlsEnabled: true }); + + const result = buildRedisStorageConfig(config, mockLogger); + + expect(result).toMatchObject({ username: 'user', password: 'secret', tlsEnabled: true }); + }); + + it('should throw ConfigurationError when batchSize is less than or equal to 0', () => { + const config = createMockRedisConfig({ delete: { batchSize: faker.number.int({ max: 0, min: -Number.MAX_SAFE_INTEGER }) } }); + + expect(() => buildRedisStorageConfig(config, mockLogger)).toThrow(ConfigurationError); + }); + + it('should throw ConfigurationError when scanCount is less than or equal to 0', () => { + const config = createMockRedisConfig({ scanCount: faker.number.int({ max: 0, min: -Number.MAX_SAFE_INTEGER }) }); + + expect(() => buildRedisStorageConfig(config, mockLogger)).toThrow(ConfigurationError); + }); + + it('should not log the password, so a secret never reaches the logs', () => { + const config = createMockRedisConfig({ password: 'secret' }); + + buildRedisStorageConfig(config, mockLogger); + + expect(JSON.stringify(vi.mocked(mockLogger.info).mock.calls)).not.toContain('secret'); + }); + }); }); From c161e4f4d4834735995da7b82d389e2133eec0f0 Mon Sep 17 00:00:00 2001 From: almog8k Date: Mon, 14 Sep 2026 00:48:14 +0300 Subject: [PATCH 02/27] feat: add redis connection (MAPCO-11263) Opens the Redis connection at startup and returns ioredis's client directly, the way S3StorageProvider uses the SDK's own S3Client. Unlike the S3 and FS clients, which are stateless and built inside their providers, this one is a long-lived socket. It is created outside the provider so a bad host or credential fails the service at boot alongside the other storage config, and so the onSignal hook has something to close. Standalone only, by design. There is no topology probe: a directly sharded cluster cannot be populated by the serving side in the first place, so a sweep there finds nothing rather than under-deleting something, and a proxy-fronted shard reports cluster_enabled:0 and would evade a probe anyway. The general detector for 'we deleted nothing' is the deleted-count signal, which lands with the provider. --- src/cleaner/storageProviders/index.ts | 1 + src/cleaner/storageProviders/redisClient.ts | 29 +++++++ tests/storageProviders/redisClient.spec.ts | 91 +++++++++++++++++++++ 3 files changed, 121 insertions(+) create mode 100644 src/cleaner/storageProviders/redisClient.ts create mode 100644 tests/storageProviders/redisClient.spec.ts diff --git a/src/cleaner/storageProviders/index.ts b/src/cleaner/storageProviders/index.ts index 42e40c2..ac5c175 100644 --- a/src/cleaner/storageProviders/index.ts +++ b/src/cleaner/storageProviders/index.ts @@ -1,6 +1,7 @@ export { mergeFailures, summarizeDeleteFailures, type DeleteFailureSummary } from './failuresHandling'; export { FsStorageProvider } from './fsStorageProvider'; export type { DeleteFailure, DeleteResult, IStorageProvider, StorageProvider, StorageProviders } from './iStorageProvider'; +export { createRedisConnection } from './redisClient'; export { S3StorageProvider } from './s3StorageProvider'; export { buildFsStorageConfig, diff --git a/src/cleaner/storageProviders/redisClient.ts b/src/cleaner/storageProviders/redisClient.ts new file mode 100644 index 0000000..62e0e75 --- /dev/null +++ b/src/cleaner/storageProviders/redisClient.ts @@ -0,0 +1,29 @@ +import type { Logger } from '@map-colonies/js-logger'; +// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class +import Redis from 'ioredis'; +import type { RedisStorageConfig } from './storageConfig'; + +/** + * Connects to a standalone Redis. + * + * The Redis connection is a long-lived socket: it is opened here at startup so a bad host or + * credential fails the service at boot rather than on the first task, and closed by the + * `onSignal` shutdown hook. + */ +export async function createRedisConnection(config: RedisStorageConfig, logger: Logger): Promise { + const client = new Redis({ + host: config.host, + port: config.port, + db: config.db, + username: config.username, + password: config.password, + lazyConnect: true, + ...(config.tlsEnabled === true && { tls: {} }), + }); + + await client.connect(); + + logger.info({ msg: 'Connected to Redis', host: config.host, port: config.port, db: config.db }); + + return client; +} diff --git a/tests/storageProviders/redisClient.spec.ts b/tests/storageProviders/redisClient.spec.ts new file mode 100644 index 0000000..a3f26e4 --- /dev/null +++ b/tests/storageProviders/redisClient.spec.ts @@ -0,0 +1,91 @@ +import type { Logger } from '@map-colonies/js-logger'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { createRedisConnection } from '@src/cleaner/storageProviders/redisClient'; +import { createMockLogger, createRedisStorageConfig } from '../helpers/mocks'; + +const { redisConstructor, connect, quit, scan, unlink } = vi.hoisted(() => ({ + redisConstructor: vi.fn(), + connect: vi.fn(), + quit: vi.fn(), + scan: vi.fn(), + unlink: vi.fn(), +})); + +vi.mock('ioredis', () => ({ + default: class { + public connect = connect; + public quit = quit; + public scan = scan; + public unlink = unlink; + public constructor(options: unknown) { + redisConstructor(options); + } + }, +})); + +describe('createRedisConnection', () => { + let mockLogger: Logger; + + beforeEach(() => { + vi.clearAllMocks(); + mockLogger = createMockLogger(); + connect.mockResolvedValue(undefined); + quit.mockResolvedValue('OK'); + }); + + describe('connecting', () => { + it('should return the connected client itself, not a wrapper', async () => { + scan.mockResolvedValue(['0', []]); + unlink.mockResolvedValue(1); + + const client = await createRedisConnection(createRedisStorageConfig(), mockLogger); + await client.scan('0', 'MATCH', 'p-*', 'COUNT', 10); + await client.unlink('k'); + + expect(connect).toHaveBeenCalledTimes(1); + expect(scan).toHaveBeenCalledWith('0', 'MATCH', 'p-*', 'COUNT', 10); + expect(unlink).toHaveBeenCalledWith('k'); + }); + + it('should build the client lazily with the configured connection details', async () => { + await createRedisConnection(createRedisStorageConfig({ host: 'redis.local', port: 6380, db: 2 }), mockLogger); + + expect(redisConstructor).toHaveBeenCalledWith(expect.objectContaining({ host: 'redis.local', port: 6380, db: 2, lazyConnect: true })); + }); + + it('should pass credentials through to the client', async () => { + await createRedisConnection(createRedisStorageConfig({ username: 'user', password: 'secret' }), mockLogger); + + expect(redisConstructor).toHaveBeenCalledWith(expect.objectContaining({ username: 'user', password: 'secret' })); + }); + + it('should enable tls when configured', async () => { + await createRedisConnection(createRedisStorageConfig({ tlsEnabled: true }), mockLogger); + + expect(redisConstructor).toHaveBeenCalledWith(expect.objectContaining({ tls: {} })); + }); + + it('should hand back a client the shutdown hook can quit', async () => { + const client = await createRedisConnection(createRedisStorageConfig(), mockLogger); + await client.quit(); + + expect(quit).toHaveBeenCalledTimes(1); + }); + }); + + describe('connection failure', () => { + it('should propagate the error so startup fails fast rather than on the first task', async () => { + connect.mockRejectedValue(new Error('ECONNREFUSED')); + + await expect(createRedisConnection(createRedisStorageConfig(), mockLogger)).rejects.toThrow('ECONNREFUSED'); + }); + + it('should not hand back a connection when connect failed', async () => { + connect.mockRejectedValue(new Error('ECONNREFUSED')); + + await expect(createRedisConnection(createRedisStorageConfig(), mockLogger)).rejects.toThrow(); + + expect(quit).not.toHaveBeenCalled(); + }); + }); +}); From 40ce69d16a6d2939b7342f10f01a729f4d9f7deb Mon Sep 17 00:00:00 2001 From: almog8k Date: Mon, 14 Sep 2026 01:20:33 +0300 Subject: [PATCH 03/27] feat: add redis storage provider key deletion (MAPCO-11263) Implements delete() for REDIS: keys are chunked by the configured batch size and each chunk goes out as a single multi-key UNLINK, summing the integer replies. deleteResources() and targetExists() are stubbed for the next change. DeleteResult gains an optional deletedCount. S3 raises NoSuchKey and FS raises ENOENT on a miss, so their callers can infer what was removed from the failures. UNLINK on an absent key returns 0 without raising, so without an observed count a completely wrong prefix would infer full success. A chunk that throws is recorded whole against its reason, with the first key as the sample: the command is atomic, so nothing in it was removed, and Redis gives no per-key attribution to do better. --- .../storageProviders/iStorageProvider.ts | 1 + src/cleaner/storageProviders/index.ts | 1 + .../storageProviders/redisStorageProvider.ts | 69 ++++++++++++ src/common/constants.ts | 2 + .../redisStorageProvider.spec.ts | 100 ++++++++++++++++++ 5 files changed, 173 insertions(+) create mode 100644 src/cleaner/storageProviders/redisStorageProvider.ts create mode 100644 tests/storageProviders/redisStorageProvider.spec.ts diff --git a/src/cleaner/storageProviders/iStorageProvider.ts b/src/cleaner/storageProviders/iStorageProvider.ts index 2afd85c..7968453 100644 --- a/src/cleaner/storageProviders/iStorageProvider.ts +++ b/src/cleaner/storageProviders/iStorageProvider.ts @@ -7,6 +7,7 @@ export type DeleteFailure = Map; export interface DeleteResult { failures: DeleteFailure; + deletedCount?: number; } export type StorageProvider = Storage['storageProvider']; diff --git a/src/cleaner/storageProviders/index.ts b/src/cleaner/storageProviders/index.ts index ac5c175..6c715c1 100644 --- a/src/cleaner/storageProviders/index.ts +++ b/src/cleaner/storageProviders/index.ts @@ -2,6 +2,7 @@ export { mergeFailures, summarizeDeleteFailures, type DeleteFailureSummary } fro export { FsStorageProvider } from './fsStorageProvider'; export type { DeleteFailure, DeleteResult, IStorageProvider, StorageProvider, StorageProviders } from './iStorageProvider'; export { createRedisConnection } from './redisClient'; +export { RedisStorageProvider } from './redisStorageProvider'; export { S3StorageProvider } from './s3StorageProvider'; export { buildFsStorageConfig, diff --git a/src/cleaner/storageProviders/redisStorageProvider.ts b/src/cleaner/storageProviders/redisStorageProvider.ts new file mode 100644 index 0000000..406fcee --- /dev/null +++ b/src/cleaner/storageProviders/redisStorageProvider.ts @@ -0,0 +1,69 @@ +import type { Logger } from '@map-colonies/js-logger'; +import type { DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; +// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class +import type Redis from 'ioredis'; +import { inject, injectable } from 'tsyringe'; +import { SERVICES } from '@common/constants'; +import { getChunk } from '@src/cleaner/utils'; +import { describeError } from '../errors'; +import type { DeleteFailure, DeleteResult, IStorageProvider, StorageProvider } from './iStorageProvider'; +import type { RedisStorageConfig } from './storageConfig'; + +type RedisStorageProviderType = Extract; + +@injectable() +export class RedisStorageProvider implements IStorageProvider { + public constructor( + @inject(SERVICES.REDIS_STORAGE_CONFIG) private readonly redisConfig: RedisStorageConfig, + @inject(SERVICES.REDIS_CONNECTION) private readonly client: Redis, + @inject(SERVICES.LOGGER) private readonly logger: Logger + ) { + this.logger.debug({ + msg: 'Loaded Redis storage provider', + host: redisConfig.host, + port: redisConfig.port, + batchSize: redisConfig.batchSize, + }); + } + + public async delete(prefix: string, keys: string[]): Promise { + if (keys.length === 0) { + return { failures: new Map(), deletedCount: 0 }; + } + + this.logger.info({ msg: 'Deleting keys from Redis', prefix, keysCount: keys.length }); + return this.unlinkInBatches(keys); + } + + public async deleteResources(params: Extract): Promise { + return Promise.reject(new Error(`Not implemented: deleteResources for ${params.prefix}`)); + } + + public async targetExists(prefix: string, relativePath: string): Promise { + return Promise.reject(new Error(`Not implemented: targetExists for ${prefix}${relativePath}`)); + } + + /** + * One multi-key `UNLINK` per chunk, summing what Redis reports it removed. + * + * A chunk that throws is recorded whole against its reason: the command is atomic, so + * nothing in it was removed, and Redis gives no per-key attribution to do better. + */ + private async unlinkInBatches(keys: string[]): Promise { + const failures: DeleteFailure = new Map(); + let deletedCount = 0; + + for (const chunk of getChunk(keys, this.redisConfig.batchSize)) { + try { + deletedCount += await this.client.unlink(...chunk); + } catch (error) { + const reason = describeError(error); + this.logger.error({ msg: 'Unlink chunk failed', reason, keysCount: chunk.length, err: error }); + const failure = failures.get(reason); + failures.set(reason, { count: (failure?.count ?? 0) + chunk.length, sample: failure?.sample ?? chunk[0]! }); + } + } + + return { failures, deletedCount }; + } +} diff --git a/src/common/constants.ts b/src/common/constants.ts index a9be61f..3214b87 100644 --- a/src/common/constants.ts +++ b/src/common/constants.ts @@ -21,6 +21,8 @@ export const SERVICES = { STORAGE_PROVIDERS: Symbol('StorageProviders'), FS_STORAGE_CONFIG: Symbol('FsStorageConfig'), S3_STORAGE_CONFIG: Symbol('S3StorageConfig'), + REDIS_STORAGE_CONFIG: Symbol('RedisStorageConfig'), + REDIS_CONNECTION: Symbol('RedisConnection'), TASK_CONTEXT: Symbol('TaskContext'), JOB_TRACKER_CLIENT: Symbol('JobTrackerClient'), // ============================================================================= diff --git a/tests/storageProviders/redisStorageProvider.spec.ts b/tests/storageProviders/redisStorageProvider.spec.ts new file mode 100644 index 0000000..88bd288 --- /dev/null +++ b/tests/storageProviders/redisStorageProvider.spec.ts @@ -0,0 +1,100 @@ +// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class +import type Redis from 'ioredis'; +import { beforeEach, describe, expect, it, vi, type Mock } from 'vitest'; +import { RedisStorageProvider } from '@src/cleaner/storageProviders/redisStorageProvider'; +import { createMockLogger, createRedisStorageConfig } from '../helpers/mocks'; + +/** + * A fake Redis recording every key it was asked to unlink. Cast at the boundary like every + * other mock in `tests/helpers/mocks.ts` — the provider only ever touches `scan` and `unlink`. + */ +interface FakeRedis { + unlinked: string[]; + scan: Mock; + unlink: Mock; +} + +function createFakeClient(): FakeRedis { + const unlinked: string[] = []; + return { + unlinked, + scan: vi.fn().mockResolvedValue(['0', []]), + unlink: vi.fn().mockImplementation(async (...keys: string[]) => { + unlinked.push(...keys); + return Promise.resolve(keys.length); + }), + }; +} + +function asRedis(fake: FakeRedis): Redis { + return fake as unknown as Redis; +} + +describe('RedisStorageProvider', () => { + const config = createRedisStorageConfig({ batchSize: 3, scanCount: 2 }); + let client: FakeRedis; + let provider: RedisStorageProvider; + + beforeEach(() => { + client = createFakeClient(); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + }); + + describe('#delete', () => { + it('should unlink every key and report how many were removed', async () => { + const keys = ['p-1-1-1', 'p-1-1-2', 'p-1-2-1']; + + const result = await provider.delete('p', keys); + + expect(client.unlinked).toEqual(keys); + expect(result.deletedCount).toBe(3); + expect(result.failures.size).toBe(0); + }); + + it('should send one unlink per batchSize chunk', async () => { + await provider.delete('p', ['a', 'b', 'c', 'd', 'e']); + + // batchSize is 3, so 5 keys means two commands. + expect(client.unlink).toHaveBeenCalledTimes(2); + expect(client.unlink).toHaveBeenNthCalledWith(1, 'a', 'b', 'c'); + expect(client.unlink).toHaveBeenNthCalledWith(2, 'd', 'e'); + }); + + it('should report deletedCount 0 when the keys were already gone, rather than a failure', async () => { + client.unlink = vi.fn().mockResolvedValue(0); + + const result = await provider.delete('p', ['missing-1-1-1']); + + expect(result.deletedCount).toBe(0); + expect(result.failures.size).toBe(0); + }); + + it('should record a failed chunk against its reason without losing the rest', async () => { + client.unlink = vi + .fn() + .mockRejectedValueOnce(new Error('READONLY')) + .mockImplementation(async (...keys: string[]) => Promise.resolve(keys.length)); + + const result = await provider.delete('p', ['a', 'b', 'c', 'd']); + + expect(result.failures.get('READONLY')).toEqual({ count: 3, sample: 'a' }); + expect(result.deletedCount).toBe(1); + }); + + it('should keep the first sample when two chunks fail for the same reason', async () => { + client.unlink = vi.fn().mockRejectedValue(new Error('READONLY')); + + const result = await provider.delete('p', ['a', 'b', 'c', 'd']); + + expect(result.failures.get('READONLY')).toEqual({ count: 4, sample: 'a' }); + expect(result.deletedCount).toBe(0); + }); + + it('should return an empty result without touching Redis when given no keys', async () => { + const result = await provider.delete('p', []); + + expect(client.unlink).not.toHaveBeenCalled(); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); + }); + }); +}); From ac6e46741d69c2e13c53aef0dc780a9a57e047fd Mon Sep 17 00:00:00 2001 From: almog8k Date: Mon, 14 Sep 2026 01:38:43 +0300 Subject: [PATCH 04/27] feat: add redis prefix wipe and target existence check (MAPCO-11263) Implements deleteResources() as a prefix wipe: scanKeys pages through SCAN and hands each non-empty page to unlinkInBatches, merging failures and summing counts across pages. scanKeys ends on the cursor returning to '0', never on an empty page. MATCH filters after the elements are retrieved, so a page can come back empty while keys still remain further along the keyspace. targetExists reports whether any key lives under the prefix, short-circuiting on the first non-empty page. It ignores relativePath, since a Redis prefix has no sub paths. Note it has no caller once the strategy is unblocked for REDIS: an empty prefix there is a legitimate cold cache rather than a missing target, so the strategy skips the precondition. unlinkInBatches now returns a required deletedCount rather than a DeleteResult, which drops an unreachable '?? 0' fallback in deleteResources. --- .../storageProviders/redisStorageProvider.ts | 48 +++++++- .../redisStorageProvider.spec.ts | 116 ++++++++++++++++++ 2 files changed, 161 insertions(+), 3 deletions(-) diff --git a/src/cleaner/storageProviders/redisStorageProvider.ts b/src/cleaner/storageProviders/redisStorageProvider.ts index 406fcee..8eb43b7 100644 --- a/src/cleaner/storageProviders/redisStorageProvider.ts +++ b/src/cleaner/storageProviders/redisStorageProvider.ts @@ -6,6 +6,7 @@ import { inject, injectable } from 'tsyringe'; import { SERVICES } from '@common/constants'; import { getChunk } from '@src/cleaner/utils'; import { describeError } from '../errors'; +import { mergeFailures } from './failuresHandling'; import type { DeleteFailure, DeleteResult, IStorageProvider, StorageProvider } from './iStorageProvider'; import type { RedisStorageConfig } from './storageConfig'; @@ -36,11 +37,52 @@ export class RedisStorageProvider implements IStorageProvider): Promise { - return Promise.reject(new Error(`Not implemented: deleteResources for ${params.prefix}`)); + const pattern = this.matchPattern(params.prefix); + this.logger.info({ msg: 'Starting Redis prefix wipe', prefix: params.prefix, pattern }); + + let failures: DeleteFailure = new Map(); + let deletedCount = 0; + + for await (const keys of this.scanKeys(pattern)) { + if (keys.length === 0) { + continue; + } + const result = await this.unlinkInBatches(keys); + failures = mergeFailures({ source: result.failures, target: failures }); + deletedCount += result.deletedCount; + } + + this.logger.info({ msg: 'Completed Redis prefix wipe', prefix: params.prefix, deletedCount, failedReasons: failures.size }); + return { failures, deletedCount }; } public async targetExists(prefix: string, relativePath: string): Promise { - return Promise.reject(new Error(`Not implemented: targetExists for ${prefix}${relativePath}`)); + for await (const keys of this.scanKeys(this.matchPattern(prefix))) { + if (keys.length > 0) { + return true; + } + } + + return false; + } + + private matchPattern(prefix: string): string { + return `${prefix}-*`; + } + + /** + * Walks the keyspace a page at a time, so nothing is buffered whole. + * + * `MATCH` filters after retrieval, so a page can come back empty while keys still remain. + * The loop therefore ends on the cursor returning to '0', never on an empty page. + */ + private async *scanKeys(pattern: string): AsyncGenerator { + let cursor = '0'; + do { + const [nextCursor, keys] = await this.client.scan(cursor, 'MATCH', pattern, 'COUNT', this.redisConfig.scanCount); + cursor = nextCursor; + yield keys; + } while (cursor !== '0'); } /** @@ -49,7 +91,7 @@ export class RedisStorageProvider implements IStorageProvider { + private async unlinkInBatches(keys: string[]): Promise> { const failures: DeleteFailure = new Map(); let deletedCount = 0; diff --git a/tests/storageProviders/redisStorageProvider.spec.ts b/tests/storageProviders/redisStorageProvider.spec.ts index 88bd288..d3febb4 100644 --- a/tests/storageProviders/redisStorageProvider.spec.ts +++ b/tests/storageProviders/redisStorageProvider.spec.ts @@ -30,6 +30,19 @@ function asRedis(fake: FakeRedis): Redis { return fake as unknown as Redis; } +/** A client whose SCAN walks `pages` in order, returning to cursor '0' only on the last one. */ +function createScanningClient(pages: string[][]): FakeRedis { + const client = createFakeClient(); + let call = 0; + client.scan = vi.fn().mockImplementation(async () => { + const page = pages[call] ?? []; + call += 1; + const cursor = call >= pages.length ? '0' : String(call); + return Promise.resolve([cursor, page] as [string, string[]]); + }); + return client; +} + describe('RedisStorageProvider', () => { const config = createRedisStorageConfig({ batchSize: 3, scanCount: 2 }); let client: FakeRedis; @@ -97,4 +110,107 @@ describe('RedisStorageProvider', () => { expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); }); }); + + describe('#deleteResources', () => { + it('should scan the prefix and unlink everything it finds', async () => { + client = createScanningClient([['p-1-1-1', 'p-1-1-2'], ['p-2-1-1']]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); + + expect([...client.unlinked].sort()).toEqual(['p-1-1-1', 'p-1-1-2', 'p-2-1-1']); + expect(result.deletedCount).toBe(3); + expect(result.failures.size).toBe(0); + }); + + it('should follow the cursor until it returns to 0', async () => { + client = createScanningClient([['a'], ['b'], ['c']]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); + + expect(client.scan).toHaveBeenCalledTimes(3); + }); + + it('should keep scanning past an empty page, because MATCH filters after retrieval', async () => { + client = createScanningClient([[], ['found']]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); + + expect(client.unlinked).toEqual(['found']); + expect(result.deletedCount).toBe(1); + }); + + it('should not unlink at all for an empty page', async () => { + client = createScanningClient([[], []]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); + + expect(client.unlink).not.toHaveBeenCalled(); + }); + + it('should scan with the prefix and a trailing dash wildcard', async () => { + client = createScanningClient([[]]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'layer-redis_WorldCRS84' }); + + expect(client.scan).toHaveBeenCalledWith('0', 'MATCH', 'layer-redis_WorldCRS84-*', 'COUNT', config.scanCount); + }); + + it('should report deletedCount 0 for a cold cache rather than failing', async () => { + client = createScanningClient([[]]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); + + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); + }); + + it('should accumulate failures across pages without losing the count', async () => { + client = createScanningClient([['a'], ['b']]); + client.unlink = vi.fn().mockRejectedValue(new Error('READONLY')); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); + + expect(result.failures.get('READONLY')).toEqual({ count: 2, sample: 'a' }); + expect(result.deletedCount).toBe(0); + }); + }); + + describe('#targetExists', () => { + it('should return true as soon as one key is found', async () => { + client = createScanningClient([['p-1-1-1']]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + await expect(provider.targetExists('p', 'ignored')).resolves.toBe(true); + }); + + it('should return false when the prefix holds nothing', async () => { + client = createScanningClient([[]]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + await expect(provider.targetExists('p', 'ignored')).resolves.toBe(false); + }); + + it('should stop scanning once a key is found rather than walking the whole keyspace', async () => { + client = createScanningClient([['found'], ['more']]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + await provider.targetExists('p', 'ignored'); + + expect(client.scan).toHaveBeenCalledTimes(1); + }); + + it('should keep looking past an empty page before concluding the prefix is empty', async () => { + client = createScanningClient([[], ['found']]); + provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); + + await expect(provider.targetExists('p', 'ignored')).resolves.toBe(true); + expect(client.scan).toHaveBeenCalledTimes(2); + }); + }); }); From a9f72f54bc17c072a65fad209ba9d3f3a8c3d5de Mon Sep 17 00:00:00 2001 From: almog8k Date: Mon, 14 Sep 2026 08:19:39 +0300 Subject: [PATCH 05/27] feat: report observed deleted count from storage providers (MAPCO-11263) Make deletedCount required on DeleteResult, propagate it from the FS and S3 providers through the tiles-deletion strategy, and log it in the outcome. Extract countFailures and trim comments. --- .../storageProviders/failuresHandling.ts | 22 ++-- .../storageProviders/fsStorageProvider.ts | 16 ++- .../storageProviders/iStorageProvider.ts | 5 +- src/cleaner/storageProviders/index.ts | 2 +- .../storageProviders/s3StorageProvider.ts | 28 ++-- .../strategies/tilesDeletionStrategy.ts | 52 ++++---- tests/helpers/mocks.ts | 4 +- .../fsStorageProvider.spec.ts | 39 +++--- .../s3StorageProvider.spec.ts | 61 ++++----- .../strategies/tilesDeletionStrategy.spec.ts | 123 ++++++++++++++++-- 10 files changed, 237 insertions(+), 115 deletions(-) diff --git a/src/cleaner/storageProviders/failuresHandling.ts b/src/cleaner/storageProviders/failuresHandling.ts index 19c1d1d..1dc0cdf 100644 --- a/src/cleaner/storageProviders/failuresHandling.ts +++ b/src/cleaner/storageProviders/failuresHandling.ts @@ -13,6 +13,14 @@ export interface DeleteFailureSummary { samples: string[]; } +export const countFailures = (failures: DeleteFailure): number => { + let count = 0; + for (const { count: reasonCount } of failures.values()) { + count += reasonCount; + } + return count; +}; + export const mergeFailures = ({ source, target }: { source: DeleteFailure; target: DeleteFailure }): DeleteFailure => { const failures: DeleteFailure = structuredClone(target); @@ -24,17 +32,9 @@ export const mergeFailures = ({ source, target }: { source: DeleteFailure; targe return failures; }; -/** - * Reduces a list of provider delete failures into a compact, log-friendly - * shape. Lives alongside `IStorageProvider` because it operates purely on - * `DeleteResult` — any caller of `provider.delete()` can use it, regardless - * of which storage backend produced the failures. - */ -export function summarizeDeleteFailures({ failures }: DeleteResult): DeleteFailureSummary { - let failuresCount = 0; - for (const { count } of failures.values()) { - failuresCount += count; - } +/** Reduces delete failures into a compact, log-friendly shape, independent of the storage backend. */ +export function summarizeDeleteFailures({ failures }: Pick): DeleteFailureSummary { + const failuresCount = countFailures(failures); const sortedFailures = Array.from(failures.entries()).sort(([, { count: a }], [, { count: b }]) => b - a); const summary = sortedFailures.map(([reason, { count }]) => `${reason}=${count}`).join(', '); diff --git a/src/cleaner/storageProviders/fsStorageProvider.ts b/src/cleaner/storageProviders/fsStorageProvider.ts index d6667de..d46ec20 100644 --- a/src/cleaner/storageProviders/fsStorageProvider.ts +++ b/src/cleaner/storageProviders/fsStorageProvider.ts @@ -3,7 +3,14 @@ import { join } from 'node:path'; 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 { + countFailures, + mergeFailures, + type DeleteFailure, + type DeleteResult, + type IStorageProvider, + type StorageProvider, +} from '@src/cleaner/storageProviders'; import { getChunk, isPathWithinAllowedSubPaths, resolveAbsolutePath } from '@src/cleaner/utils'; import { SERVICES } from '@common/constants'; import { describeError, UnrecoverableError } from '../errors'; @@ -68,7 +75,7 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { await this.cleanupEmptyDirs(paths, targetPath); - return { failures }; + return { failures, deletedCount: paths.length - countFailures(failures) }; } public async deleteResources({ @@ -103,8 +110,7 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { } failures = mergeFailures({ source: chunkFailures, target: failures }); - let failedPathsCount = 0; - chunkFailures.forEach((chunkFailure) => (failedPathsCount += chunkFailure.count)); + const failedPathsCount = countFailures(chunkFailures); const deletedPathsCount = relativePathsChunk.length - failedPathsCount; totalDeletedPathsCount += deletedPathsCount; totalFailedPathsCount += failedPathsCount; @@ -119,7 +125,7 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { await this.cleanupEmptyDirs(paths, resolveAbsolutePath(join(this.fsConfig.basePath, subPath))); - return { failures }; + return { failures, deletedCount: totalDeletedPathsCount }; } /** diff --git a/src/cleaner/storageProviders/iStorageProvider.ts b/src/cleaner/storageProviders/iStorageProvider.ts index 7968453..058c985 100644 --- a/src/cleaner/storageProviders/iStorageProvider.ts +++ b/src/cleaner/storageProviders/iStorageProvider.ts @@ -7,7 +7,7 @@ export type DeleteFailure = Map; export interface DeleteResult { failures: DeleteFailure; - deletedCount?: number; + deletedCount: number; } export type StorageProvider = Storage['storageProvider']; @@ -18,9 +18,6 @@ export interface IStorageProvider { * - S3: storageTarget = bucket name; paths are object keys * - 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 */ delete: (storageTarget: string, paths: string[]) => Promise; diff --git a/src/cleaner/storageProviders/index.ts b/src/cleaner/storageProviders/index.ts index 6c715c1..af81e29 100644 --- a/src/cleaner/storageProviders/index.ts +++ b/src/cleaner/storageProviders/index.ts @@ -1,4 +1,4 @@ -export { mergeFailures, summarizeDeleteFailures, type DeleteFailureSummary } from './failuresHandling'; +export { countFailures, mergeFailures, summarizeDeleteFailures, type DeleteFailureSummary } from './failuresHandling'; export { FsStorageProvider } from './fsStorageProvider'; export type { DeleteFailure, DeleteResult, IStorageProvider, StorageProvider, StorageProviders } from './iStorageProvider'; export { createRedisConnection } from './redisClient'; diff --git a/src/cleaner/storageProviders/s3StorageProvider.ts b/src/cleaner/storageProviders/s3StorageProvider.ts index b1aa1fe..56db343 100644 --- a/src/cleaner/storageProviders/s3StorageProvider.ts +++ b/src/cleaner/storageProviders/s3StorageProvider.ts @@ -16,7 +16,14 @@ import type { Logger } from '@map-colonies/js-logger'; import type { DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; import { inject, injectable } from 'tsyringe'; import { SERVICES } from '@common/constants'; -import { mergeFailures, type DeleteFailure, type DeleteResult, type IStorageProvider, type StorageProvider } from '@src/cleaner/storageProviders'; +import { + countFailures, + mergeFailures, + type DeleteFailure, + type DeleteResult, + type IStorageProvider, + type StorageProvider, +} from '@src/cleaner/storageProviders'; import { getChunk, normalizeFolderPath } from '@src/cleaner/utils'; import { describeError, UnrecoverableError } from '../errors'; import type { S3StorageConfig } from './storageConfig'; @@ -54,7 +61,7 @@ export class S3StorageProvider implements IStorageProvider): Promise { this.logger.debug({ msg: `Starting S3 resources deletion`, bucket, pathsCount: paths.length }); let failures: DeleteFailure = new Map(); + let deletedCount = 0; - if (paths.length === 0) return { failures }; + if (paths.length === 0) return { failures, deletedCount }; if (paths.some((path) => path.length === 0)) throw new UnrecoverableError('Cannot delete resources directly under root path of the bucket'); // Prevent root deletion const exists = await this.bucketExists(bucket); @@ -73,11 +81,12 @@ export class S3StorageProvider implements IStorageProvider { @@ -146,7 +155,7 @@ export class S3StorageProvider implements IStorageProvider { + private async deleteResource({ bucket, path }: { bucket: string; path: string }): Promise { this.logger.debug({ msg: 'Deleting a resource', bucket, path }); let failures: DeleteFailure = new Map(); let totalDeletedObjectsCount = 0, @@ -177,8 +186,7 @@ export class S3StorageProvider implements IStorageProvider (failedObjectsCount += chunkFailure.count)); + const failedObjectsCount = countFailures(chunkFailures); const deletedObjectsCount = keys.length - failedObjectsCount; totalDeletedObjectsCount += deletedObjectsCount; totalFailedObjectsCount += failedObjectsCount; @@ -192,7 +200,7 @@ export class S3StorageProvider implements IStorageProvider totalTiles, }); - const failures = await this.deleteTiles(provider, storageTarget, params, totalTiles); - this.reportOutcome(failures, totalTiles); + const { failures, deletedCount } = await this.deleteTiles(provider, storageTarget, params, totalTiles); + this.reportOutcome(failures, totalTiles, deletedCount); } /** @@ -70,18 +70,13 @@ export class TilesDeletionStrategy implements ITaskStrategy * matches the desired end-state) but counted separately for visibility. * Terminal progress to 100% is handled by the queue's task-ack — no explicit call needed here. */ - private reportOutcome(failures: DeleteFailure, totalTiles: number): void { + private reportOutcome(failures: DeleteFailure, totalTiles: number, deletedCount: number): void { const retryable: DeleteFailure = new Map(); const notFound: DeleteFailure = new Map(); - for (const failure of failures) { - (NOT_FOUND_REASONS.has(failure[0]) ? notFound : retryable).set(failure[0], failure[1]); + for (const [reason, failure] of failures) { + (NOT_FOUND_REASONS.has(reason) ? notFound : retryable).set(reason, failure); } - - let retryableCount = 0; - retryable.forEach((retryableFailure) => (retryableCount += retryableFailure.count)); - let notFoundCount = 0; - notFound.forEach((notFoundFailure) => (notFoundCount += notFoundFailure.count)); - const deletedCount = totalTiles - retryableCount - notFoundCount; + const notFoundCount = countFailures(notFound); if (retryable.size > 0) { const { failuresCount, samples, summary } = summarizeDeleteFailures({ failures: retryable }); @@ -102,14 +97,14 @@ export class TilesDeletionStrategy implements ITaskStrategy this.logger.warn({ msg: 'Tiles deletion completed with missing tiles', totalTiles, - notFoundCount: notFound.size, + notFoundCount, deletedCount, - allTilesMissing: notFound.size === totalTiles, + allTilesMissing: notFoundCount === totalTiles, }); return; } - this.logger.info({ msg: 'Tiles deletion completed successfully', deletedCount: totalTiles }); + this.logger.info({ msg: 'Tiles deletion completed successfully', deletedCount }); } private resolveStorageProvider(params: SupportedTilesDeletionParams): { provider: ResolvedStorageProvider; storageTarget: string } { @@ -126,12 +121,13 @@ export class TilesDeletionStrategy implements ITaskStrategy storageTarget: string, params: SupportedTilesDeletionParams, totalTiles: number - ): Promise { + ): Promise<{ failures: DeleteFailure; deletedCount: number }> { const { jobId, taskId } = this.taskContext; let failures: DeleteFailure = new Map(); const pendingBatches: string[][] = []; let batch: string[] = []; let processedTiles = 0; + let deletedCount = 0; for (const tileKey of this.generateTileKeys(params)) { batch.push(tileKey); @@ -139,9 +135,10 @@ export class TilesDeletionStrategy implements ITaskStrategy pendingBatches.push(batch); batch = []; if (pendingBatches.length === this.concurrency) { - const { batchFailures, processedTilesCount } = await this.flushBatches(provider, storageTarget, pendingBatches); - processedTiles += processedTilesCount; - failures = mergeFailures({ source: batchFailures, target: failures }); + const flushed = await this.flushBatches(provider, storageTarget, pendingBatches); + processedTiles += flushed.processedTilesCount; + deletedCount += flushed.deletedCount; + failures = mergeFailures({ source: flushed.batchFailures, target: failures }); const percentage = Math.round((processedTiles / totalTiles) * PERCENTAGE_COMPLETE); await this.queueClient.updateProgress(jobId, taskId, percentage); this.logger.info({ msg: 'Tiles deletion progress', deletionProgress: `${processedTiles}/${totalTiles}`, failedTiles: failures.size }); @@ -153,13 +150,14 @@ export class TilesDeletionStrategy implements ITaskStrategy pendingBatches.push(batch); } if (pendingBatches.length > 0) { - const { batchFailures, processedTilesCount } = await this.flushBatches(provider, storageTarget, pendingBatches); - processedTiles += processedTilesCount; - failures = mergeFailures({ source: batchFailures, target: failures }); + const flushed = await this.flushBatches(provider, storageTarget, pendingBatches); + processedTiles += flushed.processedTilesCount; + deletedCount += flushed.deletedCount; + failures = mergeFailures({ source: flushed.batchFailures, target: failures }); this.logger.info({ msg: 'Tiles deletion progress', deletionProgress: `${processedTiles}/${totalTiles}`, failedTiles: failures.size }); } - return failures; + return { failures, deletedCount }; } /** @@ -169,21 +167,21 @@ export class TilesDeletionStrategy implements ITaskStrategy * the reason) are both collected so nothing is silently lost and every failed path * carries a cause the caller can surface in the task rejection reason. * pendingBatches is cleared in-place for reuse. - * - * @returns Total tile paths attempted (not necessarily deleted). */ private async flushBatches( provider: ResolvedStorageProvider, storageTarget: string, pendingBatches: string[][] - ): Promise<{ batchFailures: DeleteFailure; processedTilesCount: number }> { + ): Promise<{ batchFailures: DeleteFailure; processedTilesCount: number; deletedCount: number }> { let failures: DeleteFailure = new Map(); + let deletedCount = 0; const processedTilesCount = pendingBatches.reduce((sum, b) => sum + b.length, 0); 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 }); + deletedCount += result.value.deletedCount; } else { const error: unknown = result.reason; const reason = describeError(error); @@ -194,7 +192,7 @@ export class TilesDeletionStrategy implements ITaskStrategy } } pendingBatches.length = 0; - return { batchFailures: failures, processedTilesCount }; + return { batchFailures: failures, processedTilesCount, deletedCount }; } /** diff --git a/tests/helpers/mocks.ts b/tests/helpers/mocks.ts index daf0390..edc18aa 100644 --- a/tests/helpers/mocks.ts +++ b/tests/helpers/mocks.ts @@ -76,8 +76,8 @@ export function createMockErrorHandler(defaultDecision: ErrorDecision = { should export function createMockStorageProvider(): IStorageProvider { return { - delete: vi.fn().mockResolvedValue({ failures: new Map() }), - deleteResources: vi.fn().mockResolvedValue({ failures: new Map() }), + delete: vi.fn().mockResolvedValue({ failures: new Map(), deletedCount: 0 }), + deleteResources: vi.fn().mockResolvedValue({ failures: new Map(), deletedCount: 0 }), targetExists: vi.fn().mockResolvedValue(true), }; } diff --git a/tests/storageProviders/fsStorageProvider.spec.ts b/tests/storageProviders/fsStorageProvider.spec.ts index ab4db25..51a7814 100644 --- a/tests/storageProviders/fsStorageProvider.spec.ts +++ b/tests/storageProviders/fsStorageProvider.spec.ts @@ -89,7 +89,7 @@ describe('FsStorageProvider', () => { describe('#delete', () => { it('should return empty failures map for empty input', async () => { const result = await provider.delete(SUB_PATH, []); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); expect(unlink).not.toHaveBeenCalled(); }); @@ -118,7 +118,7 @@ describe('FsStorageProvider', () => { it('should return empty failures map when all unlinks succeed', async () => { const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png', 'tile/10/0/1.png']); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); }); it('should treat ENOENT as a failed deletion tagged with ENOENT reason', async () => { @@ -127,7 +127,7 @@ describe('FsStorageProvider', () => { 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' }]]) }); + expect(result).toEqual({ failures: new Map([['ENOENT', { count: 1, sample: 'tile/10/0/0.png' }]]), deletedCount: 0 }); }); it('should return failed path with reason for non-ENOENT errors', async () => { @@ -136,7 +136,7 @@ describe('FsStorageProvider', () => { 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' }]]) }); + expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: 'tile/10/0/0.png' }]]), deletedCount: 0 }); }); it('should fall back to error message when error has no errno code', async () => { @@ -144,7 +144,7 @@ describe('FsStorageProvider', () => { 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' }]]) }); + expect(result).toEqual({ failures: new Map([['disk on fire', { count: 1, sample: 'tile/10/0/0.png' }]]), deletedCount: 0 }); }); it('should fall back to "Unknown" when error has neither errno code nor message', async () => { @@ -152,7 +152,7 @@ describe('FsStorageProvider', () => { 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' }]]) }); + expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'tile/10/0/0.png' }]]), deletedCount: 0 }); }); it('should tag failures with the stringified value when a non-Error is thrown', async () => { @@ -160,7 +160,7 @@ describe('FsStorageProvider', () => { 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' }]]) }); + expect(result).toEqual({ failures: new Map([['raw string failure', { count: 1, sample: 'tile/10/0/0.png' }]]), deletedCount: 0 }); }); it('should fall back to "Unknown" when a non-Error empty value is thrown', async () => { @@ -168,7 +168,7 @@ describe('FsStorageProvider', () => { 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' }]]) }); + expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'tile/10/0/0.png' }]]), deletedCount: 0 }); }); it('should fall back to a generic reason when the thrown value cannot be stringified', async () => { @@ -180,7 +180,7 @@ describe('FsStorageProvider', () => { 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' }]]) }); + expect(result).toEqual({ failures: new Map([['non-serializable thrown value', { count: 1, sample: 'tile/10/0/0.png' }]]), deletedCount: 0 }); }); it('should unlink every path of a large input', async () => { @@ -201,7 +201,7 @@ describe('FsStorageProvider', () => { const result = await provider.delete(SUB_PATH, paths); - expect(result).toEqual({ failures: new Map([['EACCES', { count: 7, sample: 'tile/10/0/0.png' }]]) }); + expect(result).toEqual({ failures: new Map([['EACCES', { count: 7, sample: 'tile/10/0/0.png' }]]), deletedCount: 0 }); }); it('should handle mixed success, ENOENT and real errors', async () => { @@ -221,6 +221,7 @@ describe('FsStorageProvider', () => { ['ENOENT', { count: 1, sample: 'tile/10/0/1.png' }], ['EACCES', { count: 1, sample: 'tile/10/0/2.png' }], ]), + deletedCount: 1, }); }); @@ -231,7 +232,7 @@ describe('FsStorageProvider', () => { const relativePath = 'layer/v1/10/5/3.png'; const result = await provider.delete(SUB_PATH, [relativePath]); - expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: relativePath }]]) }); + expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: relativePath }]]), deletedCount: 0 }); expect(Array.from(result.failures.values())[0]?.sample).not.toMatch(`^${BASE_PATH}*`); }); @@ -268,7 +269,7 @@ describe('FsStorageProvider', () => { 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() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 1 }); }); it('should not call rmdir when input is empty', async () => { @@ -318,7 +319,7 @@ describe('FsStorageProvider', () => { it('should successfully return without failures for empty paths', async () => { const result = await provider.deleteResources({ paths: [], subPath: FS_SUB_PATH, storageProvider: 'FS' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); expect(rm).not.toHaveBeenCalled(); expect(rmdir).not.toHaveBeenCalled(); }); @@ -326,7 +327,7 @@ describe('FsStorageProvider', () => { it('should successfully call delete all files and return without failures', async () => { const result = await provider.deleteResources({ paths: [RELATIVE_PATH], subPath: FS_SUB_PATH, storageProvider: 'FS' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 1 }); expect(rm).toHaveBeenCalledWith(join(BASE_PATH, FS_SUB_PATH, RELATIVE_PATH), { recursive: true, force: true }); expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, FS_SUB_PATH, 'layer')); }); @@ -334,7 +335,7 @@ describe('FsStorageProvider', () => { it('should successfully call delete all files and return without failures for multiple paths', async () => { const result = await provider.deleteResources({ paths: [RELATIVE_PATH, RELATIVE_PATH], subPath: FS_SUB_PATH, storageProvider: 'FS' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); expect(rm).toHaveBeenCalledWith(join(BASE_PATH, FS_SUB_PATH, RELATIVE_PATH), { recursive: true, force: true }); expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, FS_SUB_PATH, 'layer')); }); @@ -342,7 +343,7 @@ describe('FsStorageProvider', () => { it('should successfully call delete all files and return without failures for nested paths', async () => { const result = await provider.deleteResources({ paths: [RELATIVE_PATH, `${RELATIVE_PATH}/old`], subPath: FS_SUB_PATH, storageProvider: 'FS' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); expect(rm).toHaveBeenCalledWith(join(BASE_PATH, FS_SUB_PATH, RELATIVE_PATH), { recursive: true, force: true }); expect(rmdir).toHaveBeenNthCalledWith(1, join(BASE_PATH, FS_SUB_PATH, 'layer')); expect(rmdir).toHaveBeenNthCalledWith(2, join(BASE_PATH, FS_SUB_PATH, RELATIVE_PATH)); @@ -387,6 +388,7 @@ describe('FsStorageProvider', () => { expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: join(BASE_PATH, FS_SUB_PATH, RELATIVE_PATH) }]]), + deletedCount: 0, }); }); @@ -418,6 +420,7 @@ describe('FsStorageProvider', () => { expect(result).toEqual({ failures: new Map([['EACCES', { count: 7, sample: join(BASE_PATH, FS_SUB_PATH, 'layer/v0') }]]), + deletedCount: 0, }); }); @@ -437,6 +440,7 @@ describe('FsStorageProvider', () => { ['EACCES', { count: 2, sample: join(BASE_PATH, FS_SUB_PATH, 'layer/v0') }], ['EBUSY', { count: 1, sample: join(BASE_PATH, FS_SUB_PATH, 'layer/v2') }], ]), + deletedCount: 1, }); }); @@ -521,7 +525,7 @@ describe('FsStorageProvider', () => { const result = await provider.deleteResources({ paths: ['layer/v1/old'], subPath: FS_SUB_PATH, storageProvider: 'FS' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 1 }); }); it('should still attempt cleanup when rm rejects, without adding cleanup errors to the failures', async () => { @@ -533,6 +537,7 @@ describe('FsStorageProvider', () => { expect(rmdir).toHaveBeenCalledWith(join(SUB_PATH_ROOT, 'layer/v1')); expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: join(SUB_PATH_ROOT, 'layer/v1/old') }]]), + deletedCount: 0, }); }); }); diff --git a/tests/storageProviders/s3StorageProvider.spec.ts b/tests/storageProviders/s3StorageProvider.spec.ts index f0a168d..2082241 100644 --- a/tests/storageProviders/s3StorageProvider.spec.ts +++ b/tests/storageProviders/s3StorageProvider.spec.ts @@ -78,7 +78,7 @@ describe('S3StorageProvider', () => { it('should return empty failures map for empty input', async () => { const result = await provider.delete(BUCKET, []); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); expect(mockSend).not.toHaveBeenCalled(); }); @@ -99,7 +99,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, paths); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); }); it('should return failed paths tagged with the S3 error Code', async () => { @@ -110,7 +110,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, paths); - expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'a.txt' }]]) }); + expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'a.txt' }]]), deletedCount: 1 }); }); it('should treat NoSuchKey as a failed deletion tagged with NoSuchKey reason', async () => { @@ -121,7 +121,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, paths); - expect(result).toEqual({ failures: new Map([['NoSuchKey', { count: 1, sample: 'missing.txt' }]]) }); + expect(result).toEqual({ failures: new Map([['NoSuchKey', { count: 1, sample: 'missing.txt' }]]), deletedCount: 0 }); }); it('should fall back to Message when error has no Code', async () => { @@ -131,7 +131,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, ['a.txt']); - expect(result).toEqual({ failures: new Map([['Something bad', { count: 1, sample: 'a.txt' }]]) }); + expect(result).toEqual({ failures: new Map([['Something bad', { count: 1, sample: 'a.txt' }]]), deletedCount: 0 }); }); it('should fall back to "Unknown" when error has neither Code nor Message', async () => { @@ -141,7 +141,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, ['a.txt']); - expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'a.txt' }]]) }); + expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'a.txt' }]]), deletedCount: 0 }); }); it('should return all errors including NoSuchKey with their codes', async () => { @@ -162,6 +162,7 @@ describe('S3StorageProvider', () => { ['AccessDenied', { count: 1, sample: 'b.txt' }], ['InternalError', { count: 1, sample: 'd.txt' }], ]), + deletedCount: 0, }); }); @@ -202,7 +203,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, paths); - expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 2, sample: 'object-0.txt' }]]) }); + expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 2, sample: 'object-0.txt' }]]), deletedCount: 1498 }); }); it('should ignore returned errors that carry no Key', async () => { @@ -212,7 +213,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, ['a.txt', 'b.txt']); - expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'b.txt' }]]) }); + expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'b.txt' }]]), deletedCount: 1 }); }); it('should return an empty failures map when every returned error carries no Key', async () => { @@ -220,7 +221,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, ['a.txt']); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 1 }); }); it('should tag the chunk with the stringified value when send rejects with a non-Error', async () => { @@ -228,7 +229,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, ['a.txt', 'b.txt']); - expect(result).toEqual({ failures: new Map([['connection reset', { count: 2, sample: 'a.txt' }]]) }); + expect(result).toEqual({ failures: new Map([['connection reset', { count: 2, sample: 'a.txt' }]]), deletedCount: 0 }); }); it('should add entire chunk to failures tagged with the thrown error when send rejects', async () => { @@ -237,7 +238,7 @@ describe('S3StorageProvider', () => { const result = await provider.delete(BUCKET, paths); - expect(result).toEqual({ failures: new Map([['Network error', { count: 2, sample: 'a.txt' }]]) }); + expect(result).toEqual({ failures: new Map([['Network error', { count: 2, sample: 'a.txt' }]]), deletedCount: 0 }); }); }); @@ -303,7 +304,7 @@ describe('S3StorageProvider', () => { it('should return empty result when input paths is empty', async () => { const result = await provider.deleteResources({ paths: [], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -318,7 +319,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(1); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -349,7 +350,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [path], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(1); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -368,7 +369,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(2); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -387,7 +388,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(2); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -409,7 +410,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(3); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -430,7 +431,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 1 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(3); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -452,7 +453,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [objectKey], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 1 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(3); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -484,7 +485,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH1, PATH2], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 4 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(6); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -504,7 +505,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 3 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(2); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -519,7 +520,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(2); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -538,7 +539,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); expect(DeleteObjectsCommand).toHaveBeenCalledWith({ Bucket: BUCKET, Delete: { Objects: keys.map((Key) => ({ Key })) } }); expect(mockSend).toHaveBeenCalledTimes(3); }); @@ -553,7 +554,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); expect(DeleteObjectsCommand).not.toHaveBeenCalled(); expect(mockSend).toHaveBeenCalledTimes(2); }); @@ -571,7 +572,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 2, sample: 'layer/v1/0/0.png' }]]) }); + expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 2, sample: 'layer/v1/0/0.png' }]]), deletedCount: 0 }); }); it('should accumulate failures of the same reason across paths keeping the first sample', async () => { @@ -589,7 +590,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: ['layer/v1', 'layer/v2'], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 2, sample: 'layer/v1/0/0.png' }]]) }); + expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 2, sample: 'layer/v1/0/0.png' }]]), deletedCount: 0 }); }); it('should request pages sized by the configured batch size', async () => { @@ -616,7 +617,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(2); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -635,7 +636,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map() }); + expect(result).toEqual({ failures: new Map(), deletedCount: 2 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(2); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -706,7 +707,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'layer/v1/0/0.png' }]]) }); + expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'layer/v1/0/0.png' }]]), deletedCount: 1 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(3); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); @@ -728,7 +729,7 @@ describe('S3StorageProvider', () => { const result = await provider.deleteResources({ paths: [PATH], bucket: BUCKET, storageProvider: 'S3' }); - expect(result).toEqual({ failures: new Map([['NetworkError', { count: 1, sample: 'layer/v1/0/0.png' }]]) }); + expect(result).toEqual({ failures: new Map([['NetworkError', { count: 1, sample: 'layer/v1/0/0.png' }]]), deletedCount: 1 }); expect(mockPaginateListObjectsV2Next).toHaveBeenCalledTimes(3); expect(mockPaginateListObjectsV2Return).toHaveBeenCalledTimes(0); expect(mockPaginateListObjectsV2Throw).toHaveBeenCalledTimes(0); diff --git a/tests/strategies/tilesDeletionStrategy.spec.ts b/tests/strategies/tilesDeletionStrategy.spec.ts index 5d1f0e8..ed17e60 100644 --- a/tests/strategies/tilesDeletionStrategy.spec.ts +++ b/tests/strategies/tilesDeletionStrategy.spec.ts @@ -311,7 +311,10 @@ describe('TilesDeletionStrategy', () => { }); it('should not call updateProgress when retryable failures occur', async () => { - vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map([['AccessDenied', { count: 1, sample: tilePath(10, 0, 0) }]]) }); + vi.mocked(MockS3Provider.delete).mockResolvedValue({ + failures: new Map([['AccessDenied', { count: 1, sample: tilePath(10, 0, 0) }]]), + deletedCount: 0, + }); await expect(strategy.execute(s3Params)).rejects.toThrow(RecoverableError); @@ -319,7 +322,10 @@ describe('TilesDeletionStrategy', () => { }); it('should not call updateProgress when only not-found failures occur', async () => { - vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map([['NoSuchKey', { count: 1, sample: tilePath(10, 0, 0) }]]) }); + vi.mocked(MockS3Provider.delete).mockResolvedValue({ + failures: new Map([['NoSuchKey', { count: 1, sample: tilePath(10, 0, 0) }]]), + deletedCount: 0, + }); await expect(strategy.execute(s3Params)).resolves.toBeUndefined(); expect(mockUpdateProgress).not.toHaveBeenCalled(); @@ -328,13 +334,19 @@ describe('TilesDeletionStrategy', () => { describe('failure handling', () => { it('should throw RecoverableError when provider returns fatal failed paths', async () => { - vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map([['AccessDenied', { count: 1, sample: tilePath(10, 0, 0) }]]) }); + vi.mocked(MockS3Provider.delete).mockResolvedValue({ + failures: new Map([['AccessDenied', { count: 1, sample: tilePath(10, 0, 0) }]]), + deletedCount: 0, + }); await expect(strategy.execute(s3Params)).rejects.toThrow(RecoverableError); }); it('should include fatal failed count in RecoverableError message', async () => { - vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map([['AccessDenied', { count: 2, sample: tilePath(10, 0, 0) }]]) }); + vi.mocked(MockS3Provider.delete).mockResolvedValue({ + failures: new Map([['AccessDenied', { count: 2, sample: tilePath(10, 0, 0) }]]), + deletedCount: 0, + }); await expect(strategy.execute(s3Params)).rejects.toThrow(/Failed to delete 2/); }); @@ -345,6 +357,7 @@ describe('TilesDeletionStrategy', () => { ['EACCES', { count: 2, sample: tilePath(10, 0, 0) }], ['AccessDenied', { count: 1, sample: tilePath(10, 1, 0) }], ]), + deletedCount: 0, }); await expect(strategy.execute(s3Params)).rejects.toThrow(/Reasons: EACCES=2, AccessDenied=1/); @@ -356,13 +369,17 @@ describe('TilesDeletionStrategy', () => { ['NoSuchKey', { count: 1, sample: tilePath(10, 0, 0) }], ['AccessDenied', { count: 1, sample: tilePath(10, 0, 1) }], ]), + deletedCount: 0, }); await expect(strategy.execute(s3Params)).rejects.toThrow(/Failed to delete 1.*Reasons: AccessDenied=1/); }); it('should include path and reason in the failure sample', async () => { - vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map([['AccessDenied', { count: 1, sample: tilePath(10, 0, 0) }]]) }); + vi.mocked(MockS3Provider.delete).mockResolvedValue({ + failures: new Map([['AccessDenied', { count: 1, sample: tilePath(10, 0, 0) }]]), + deletedCount: 0, + }); await expect(strategy.execute(s3Params)).rejects.toThrow(/layer\/v1\/10\/0\/0\.png \(AccessDenied\)/); }); @@ -370,19 +387,23 @@ describe('TilesDeletionStrategy', () => { it('should resolve successfully when all failures are not-found (ENOENT)', async () => { vi.mocked(MockFsProvider.delete).mockResolvedValue({ failures: new Map([['ENOENT', { count: 1, sample: tilePath(10, 0, 0) }]]), + deletedCount: 3, }); await expect(strategy.execute(fsParams)).resolves.toBeUndefined(); }); it('should resolve successfully when all failures are not-found (NoSuchKey)', async () => { - vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map([['NoSuchKey', { count: 1, sample: tilePath(10, 0, 0) }]]) }); + vi.mocked(MockS3Provider.delete).mockResolvedValue({ + failures: new Map([['NoSuchKey', { count: 1, sample: tilePath(10, 0, 0) }]]), + deletedCount: 0, + }); await expect(strategy.execute(s3Params)).resolves.toBeUndefined(); }); it('should resolve successfully when provider returns no failed paths', async () => { - vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map() }); + vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map(), deletedCount: 4 }); await expect(strategy.execute(s3Params)).resolves.toBeUndefined(); }); @@ -436,7 +457,7 @@ describe('TilesDeletionStrategy', () => { ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; vi.mocked(MockS3Provider.delete) - .mockResolvedValueOnce({ failures: new Map([['AccessDenied', { count: 3, sample: tilePath(5, 0, 0) }]]) }) + .mockResolvedValueOnce({ failures: new Map([['AccessDenied', { count: 3, sample: tilePath(5, 0, 0) }]]), deletedCount: 0 }) .mockRejectedValueOnce(new Error('S3 connection lost')); // reasons are ordered by descending count @@ -455,5 +476,91 @@ describe('TilesDeletionStrategy', () => { await expect(strategy.execute(fsParams)).resolves.toBeUndefined(); }); }); + + describe('deleted count reporting', () => { + let mockLogger: ReturnType; + + function buildStrategyWithLogger(batchSize = 100): TilesDeletionStrategy { + mockLogger = createMockLogger(); + const storageProviders: StorageProviders = { [SourceType.S3]: MockS3Provider }; + const queueClient = { updateProgress: mockUpdateProgress } as unknown as QueueClient; + const config = createMockStrategyConfig({ + 'strategies.tilesDeletion.batchSize': batchSize, + 'strategies.tilesDeletion.concurrency': 2, + }); + return new TilesDeletionStrategy(mockLogger, config, storageProviders, queueClient, TASK_CONTEXT); + } + + it('should report the count the provider observed rather than the number of tiles attempted', async () => { + // s3Params spans 4 tiles; the store reports only 2 existed + vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map(), deletedCount: 2 }); + + await buildStrategyWithLogger().execute(s3Params); + + expect(mockLogger.info).toHaveBeenCalledWith(expect.objectContaining({ msg: 'Tiles deletion completed successfully', deletedCount: 2 })); + }); + + it('should sum the observed count across batches', async () => { + // batchSize 1 over a 2x2 range → four delete calls + vi.mocked(MockS3Provider.delete) + .mockResolvedValueOnce({ failures: new Map(), deletedCount: 1 }) + .mockResolvedValueOnce({ failures: new Map(), deletedCount: 1 }) + .mockResolvedValue({ failures: new Map(), deletedCount: 0 }); + + await buildStrategyWithLogger(1).execute(s3Params); + + expect(vi.mocked(MockS3Provider.delete)).toHaveBeenCalledTimes(4); + expect(mockLogger.info).toHaveBeenCalledWith(expect.objectContaining({ msg: 'Tiles deletion completed successfully', deletedCount: 2 })); + }); + + it('should report zero deletions when the store found nothing, which is the wrong-prefix signal', async () => { + vi.mocked(MockS3Provider.delete).mockResolvedValue({ failures: new Map(), deletedCount: 0 }); + + await buildStrategyWithLogger().execute(s3Params); + + expect(mockLogger.info).toHaveBeenCalledWith(expect.objectContaining({ msg: 'Tiles deletion completed successfully', deletedCount: 0 })); + }); + + it('should count a hard-rejected batch as zero deletions', async () => { + // batchSize 1, concurrency 2: the first window has one success and one rejection. + vi.mocked(MockS3Provider.delete) + .mockResolvedValueOnce({ failures: new Map(), deletedCount: 1 }) + .mockRejectedValueOnce(new Error('S3 connection lost')) + .mockResolvedValue({ failures: new Map(), deletedCount: 1 }); + + await expect(buildStrategyWithLogger(1).execute(s3Params)).rejects.toThrow(RecoverableError); + + expect(mockLogger.error).toHaveBeenCalledWith(expect.objectContaining({ msg: 'Tiles deletion partially failed', deletedCount: 3 })); + }); + + it('should report the observed count in the partial-failure log too', async () => { + vi.mocked(MockS3Provider.delete) + .mockResolvedValueOnce({ failures: new Map(), deletedCount: 0 }) + .mockResolvedValue({ failures: new Map([['AccessDenied', { count: 1, sample: tilePath(10, 0, 0) }]]), deletedCount: 0 }); + + await expect(buildStrategyWithLogger(1).execute(s3Params)).rejects.toThrow(RecoverableError); + + expect(mockLogger.error).toHaveBeenCalledWith(expect.objectContaining({ msg: 'Tiles deletion partially failed', deletedCount: 0 })); + }); + + it('should report the not-found tile count, not the number of not-found reasons', async () => { + vi.mocked(MockS3Provider.delete).mockResolvedValue({ + failures: new Map([['NoSuchKey', { count: 3, sample: tilePath(10, 0, 0) }]]), + deletedCount: 1, + }); + + await buildStrategyWithLogger().execute(s3Params); + + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.objectContaining({ + msg: 'Tiles deletion completed with missing tiles', + totalTiles: 4, + notFoundCount: 3, + deletedCount: 1, + allTilesMissing: false, + }) + ); + }); + }); }); }); From 145f48b5b3c8618a65758bb0322852bd7fa490a9 Mon Sep 17 00:00:00 2001 From: almog8k Date: Mon, 14 Sep 2026 08:49:16 +0300 Subject: [PATCH 06/27] feat: support redis in tiles deletion strategy (MAPCO-11263) Make targetExists optional on IStorageProvider and drop it from the Redis provider, since an empty prefix is a cold cache rather than a missing target. --- .../storageProviders/iStorageProvider.ts | 9 ++- src/cleaner/storageProviders/index.ts | 10 ++- .../storageProviders/redisStorageProvider.ts | 10 --- .../strategies/tilesDeletionStrategy.ts | 59 +++++++++------ tests/helpers/mocks.ts | 6 +- .../redisStorageProvider.spec.ts | 33 --------- .../strategies/tilesDeletionStrategy.spec.ts | 74 ++++++++++++++++--- 7 files changed, 122 insertions(+), 79 deletions(-) diff --git a/src/cleaner/storageProviders/iStorageProvider.ts b/src/cleaner/storageProviders/iStorageProvider.ts index 058c985..f8e68aa 100644 --- a/src/cleaner/storageProviders/iStorageProvider.ts +++ b/src/cleaner/storageProviders/iStorageProvider.ts @@ -5,6 +5,12 @@ import type { DeleteStoredResourcesParams, Storage } from '@map-colonies/raster- */ export type DeleteFailure = Map; +/** Where a deletion operates. Redis keys are flat under the prefix, so it has no relativePath. */ +export interface StorageTarget { + storageTarget: string; + relativePath?: string; +} + export interface DeleteResult { failures: DeleteFailure; deletedCount: number; @@ -33,8 +39,9 @@ export interface IStorageProvider { * - S3: storageTarget = bucket, relativePath = key prefix — lists objects (KeyCount > 0) * - 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` + * Optional: a cache store (Redis) cannot tell a missing target from a cold cache, so it omits it. */ - targetExists: (storageTarget: string, relativePath: string) => Promise; + targetExists?: (storageTarget: string, relativePath: string) => Promise; } export type StorageProviders = { diff --git a/src/cleaner/storageProviders/index.ts b/src/cleaner/storageProviders/index.ts index af81e29..d5bca72 100644 --- a/src/cleaner/storageProviders/index.ts +++ b/src/cleaner/storageProviders/index.ts @@ -1,6 +1,14 @@ export { countFailures, mergeFailures, summarizeDeleteFailures, type DeleteFailureSummary } from './failuresHandling'; export { FsStorageProvider } from './fsStorageProvider'; -export type { DeleteFailure, DeleteResult, IStorageProvider, StorageProvider, StorageProviders } from './iStorageProvider'; +export type { + DeleteFailure, + DeleteResult, + IStorageProvider, + ResolvedStorageProvider, + StorageProvider, + StorageProviders, + StorageTarget, +} from './iStorageProvider'; export { createRedisConnection } from './redisClient'; export { RedisStorageProvider } from './redisStorageProvider'; export { S3StorageProvider } from './s3StorageProvider'; diff --git a/src/cleaner/storageProviders/redisStorageProvider.ts b/src/cleaner/storageProviders/redisStorageProvider.ts index 8eb43b7..f021815 100644 --- a/src/cleaner/storageProviders/redisStorageProvider.ts +++ b/src/cleaner/storageProviders/redisStorageProvider.ts @@ -56,16 +56,6 @@ export class RedisStorageProvider implements IStorageProvider { - for await (const keys of this.scanKeys(this.matchPattern(prefix))) { - if (keys.length > 0) { - return true; - } - } - - return false; - } - private matchPattern(prefix: string): string { return `${prefix}-*`; } diff --git a/src/cleaner/strategies/tilesDeletionStrategy.ts b/src/cleaner/strategies/tilesDeletionStrategy.ts index 05f151d..18bf6c8 100644 --- a/src/cleaner/strategies/tilesDeletionStrategy.ts +++ b/src/cleaner/strategies/tilesDeletionStrategy.ts @@ -5,18 +5,22 @@ import { StorageProvider, TilesDeletionParams, tilesDeletionParamsSchema } from import { inject, injectable } from 'tsyringe'; import type { ConfigType } from '@common/config'; import { PERCENTAGE_COMPLETE, SERVICES } from '@common/constants'; -import { countFailures, mergeFailures, summarizeDeleteFailures, type DeleteFailure, type StorageProviders } from '@src/cleaner/storageProviders'; +import { + countFailures, + mergeFailures, + summarizeDeleteFailures, + type DeleteFailure, + type ResolvedStorageProvider, + type StorageProviders, + type StorageTarget, +} from '@src/cleaner/storageProviders'; import { RecoverableError, UnrecoverableError, describeError } from '../errors'; -import { ResolvedStorageProvider } from '../storageProviders/iStorageProvider'; import { resolveTileKeyGenerator, validateSchema } from '../utils'; import type { TaskContext } from './strategyFactory'; import type { ITaskStrategy } from './taskStrategy'; const NOT_FOUND_REASONS = new Set([NoSuchKey.name, 'ENOENT']); -/** Redis tiles deletion is not implemented yet (MAPCO-11263). */ -type SupportedTilesDeletionParams = Exclude; - @injectable() export class TilesDeletionStrategy implements ITaskStrategy { private readonly batchSize: number; @@ -39,15 +43,8 @@ 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.tilesRelativePath))) { - throw new UnrecoverableError(`${params.storageProvider} storage target does not exist: ${storageTarget}/${params.tilesRelativePath}`); - } + const { provider, storageTarget, relativePath } = this.resolveStorageProvider(params); + await this.assertTargetExists(provider, storageTarget, relativePath); const totalTiles = this.countTiles(params); @@ -107,19 +104,39 @@ export class TilesDeletionStrategy implements ITaskStrategy this.logger.info({ msg: 'Tiles deletion completed successfully', deletedCount }); } - private resolveStorageProvider(params: SupportedTilesDeletionParams): { provider: ResolvedStorageProvider; storageTarget: string } { + private resolveStorageProvider(params: TilesDeletionParams): StorageTarget & { provider: ResolvedStorageProvider } { // 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}`); - const storageTarget = params.storageProvider === StorageProvider.S3 ? params.bucket : params.subPath; - this.logger.debug({ msg: `Using ${params.storageProvider} provider`, storageTarget }); - return { provider: storageProvider, storageTarget }; + const target = this.resolveTarget(params); + this.logger.debug({ msg: `Using ${params.storageProvider} provider`, ...target }); + return { provider: storageProvider, ...target }; + } + + private resolveTarget(params: TilesDeletionParams): StorageTarget { + switch (params.storageProvider) { + case StorageProvider.S3: + return { storageTarget: params.bucket, relativePath: params.tilesRelativePath }; + case StorageProvider.FS: + return { storageTarget: params.subPath, relativePath: params.tilesRelativePath }; + case StorageProvider.REDIS: + return { storageTarget: params.prefix }; + } + } + + private async assertTargetExists(provider: ResolvedStorageProvider, storageTarget: string, relativePath?: string): Promise { + if (provider.targetExists === undefined || relativePath === undefined) { + return; + } + if (!(await provider.targetExists(storageTarget, relativePath))) { + throw new UnrecoverableError(`Tiles storage target does not exist: ${storageTarget}/${relativePath}`); + } } private async deleteTiles( provider: ResolvedStorageProvider, storageTarget: string, - params: SupportedTilesDeletionParams, + params: TilesDeletionParams, totalTiles: number ): Promise<{ failures: DeleteFailure; deletedCount: number }> { const { jobId, taskId } = this.taskContext; @@ -200,11 +217,11 @@ 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: SupportedTilesDeletionParams): number { + private countTiles(params: TilesDeletionParams): number { return params.ranges.reduce((sum, r) => sum + (r.maxX - r.minX + 1) * (r.maxY - r.minY + 1), 0); } - private *generateTileKeys(params: SupportedTilesDeletionParams): Generator { + private *generateTileKeys(params: TilesDeletionParams): Generator { const toKeys = resolveTileKeyGenerator(params); for (const range of params.ranges) { diff --git a/tests/helpers/mocks.ts b/tests/helpers/mocks.ts index edc18aa..5d3e13c 100644 --- a/tests/helpers/mocks.ts +++ b/tests/helpers/mocks.ts @@ -74,11 +74,13 @@ export function createMockErrorHandler(defaultDecision: ErrorDecision = { should // ─── StorageProvider ───────────────────────────────────────────────────────── -export function createMockStorageProvider(): IStorageProvider { +export function createMockStorageProvider(): Required>; +export function createMockStorageProvider(options: { targetExists: false }): IStorageProvider; +export function createMockStorageProvider({ targetExists = true } = {}): IStorageProvider { return { delete: vi.fn().mockResolvedValue({ failures: new Map(), deletedCount: 0 }), deleteResources: vi.fn().mockResolvedValue({ failures: new Map(), deletedCount: 0 }), - targetExists: vi.fn().mockResolvedValue(true), + ...(targetExists && { targetExists: vi.fn().mockResolvedValue(true) }), }; } diff --git a/tests/storageProviders/redisStorageProvider.spec.ts b/tests/storageProviders/redisStorageProvider.spec.ts index d3febb4..16772ed 100644 --- a/tests/storageProviders/redisStorageProvider.spec.ts +++ b/tests/storageProviders/redisStorageProvider.spec.ts @@ -180,37 +180,4 @@ describe('RedisStorageProvider', () => { expect(result.deletedCount).toBe(0); }); }); - - describe('#targetExists', () => { - it('should return true as soon as one key is found', async () => { - client = createScanningClient([['p-1-1-1']]); - provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); - - await expect(provider.targetExists('p', 'ignored')).resolves.toBe(true); - }); - - it('should return false when the prefix holds nothing', async () => { - client = createScanningClient([[]]); - provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); - - await expect(provider.targetExists('p', 'ignored')).resolves.toBe(false); - }); - - it('should stop scanning once a key is found rather than walking the whole keyspace', async () => { - client = createScanningClient([['found'], ['more']]); - provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); - - await provider.targetExists('p', 'ignored'); - - expect(client.scan).toHaveBeenCalledTimes(1); - }); - - it('should keep looking past an empty page before concluding the prefix is empty', async () => { - client = createScanningClient([[], ['found']]); - provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); - - await expect(provider.targetExists('p', 'ignored')).resolves.toBe(true); - expect(client.scan).toHaveBeenCalledTimes(2); - }); - }); }); diff --git a/tests/strategies/tilesDeletionStrategy.spec.ts b/tests/strategies/tilesDeletionStrategy.spec.ts index ed17e60..d7eca23 100644 --- a/tests/strategies/tilesDeletionStrategy.spec.ts +++ b/tests/strategies/tilesDeletionStrategy.spec.ts @@ -1,6 +1,12 @@ import { faker } from '@faker-js/faker'; import type { TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; -import { type FsTilesDeletionParams, type S3TilesDeletionParams, SourceType } from '@map-colonies/raster-shared'; +import { + type FsTilesDeletionParams, + type RedisTilesDeletionParams, + type S3TilesDeletionParams, + SourceType, + StorageProvider, +} 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'; @@ -36,8 +42,8 @@ const tilePath = (z: number, x: number, y: number): string => `${s3Params.tilesR describe('TilesDeletionStrategy', () => { let strategy: TilesDeletionStrategy; - let MockS3Provider: IStorageProvider<'S3'>; - let MockFsProvider: IStorageProvider<'FS'>; + let MockS3Provider: Required>; + let MockFsProvider: Required>; let mockUpdateProgress: ReturnType; beforeEach(() => { @@ -157,14 +163,6 @@ describe('TilesDeletionStrategy', () => { expect(MockS3Provider.delete).not.toHaveBeenCalled(); }); - 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, storageProvider: 'UNKNOWN' } as unknown as S3TilesDeletionParams; @@ -238,6 +236,60 @@ describe('TilesDeletionStrategy', () => { }); }); + describe('REDIS provider', () => { + const redisParams: RedisTilesDeletionParams = { + storageProvider: StorageProvider.REDIS, + prefix: 'eli_test-Orthophoto-redis_WorldCRS84', + ranges: [{ zoom: 3, minX: 1, maxX: 2, minY: 5, maxY: 6 }], + }; + let MockRedisProvider: IStorageProvider<'REDIS'>; + + const buildStrategy = (storageProviders: StorageProviders): TilesDeletionStrategy => { + const queueClient = { updateProgress: mockUpdateProgress } as unknown as QueueClient; + return new TilesDeletionStrategy(createMockLogger(), createMockStrategyConfig(), storageProviders, queueClient, TASK_CONTEXT); + }; + + beforeEach(() => { + MockRedisProvider = createMockStorageProvider({ targetExists: false }); + }); + + it('should delete redis tile keys with the prefix as storage target', async () => { + vi.mocked(MockRedisProvider.delete).mockResolvedValue({ failures: new Map(), deletedCount: 4 }); + const redisStrategy = buildStrategy({ [StorageProvider.REDIS]: MockRedisProvider }); + + await expect(redisStrategy.execute(redisParams)).resolves.toBeUndefined(); + + expect(MockRedisProvider.delete).toHaveBeenCalledWith(redisParams.prefix, [ + `${redisParams.prefix}-3-1-5`, + `${redisParams.prefix}-3-1-6`, + `${redisParams.prefix}-3-2-5`, + `${redisParams.prefix}-3-2-6`, + ]); + }); + + it('should skip the target existence check when the provider does not implement it', async () => { + vi.mocked(MockRedisProvider.delete).mockResolvedValue({ failures: new Map(), deletedCount: 0 }); + const redisStrategy = buildStrategy({ [StorageProvider.REDIS]: MockRedisProvider }); + + await expect(redisStrategy.execute(redisParams)).resolves.toBeUndefined(); + + expect(MockRedisProvider.delete).toHaveBeenCalledTimes(1); + }); + + it('should still enforce targetExists for S3', async () => { + vi.mocked(MockS3Provider.targetExists).mockResolvedValue(false); + const s3Strategy = buildStrategy({ [StorageProvider.S3]: MockS3Provider }); + + await expect(s3Strategy.execute(s3Params)).rejects.toThrow(UnrecoverableError); + }); + + it('should throw UnrecoverableError when the redis provider is not registered', async () => { + const redisStrategy = buildStrategy({}); + + await expect(redisStrategy.execute(redisParams)).rejects.toThrow(UnrecoverableError); + }); + }); + describe('progress reporting', () => { 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 From f5cd581fbd90a3bb52edf2ec90522dca3a41264f Mon Sep 17 00:00:00 2001 From: almog8k Date: Mon, 14 Sep 2026 09:27:59 +0300 Subject: [PATCH 07/27] feat: wire redis storage provider into the container (MAPCO-11263) --- src/containerConfig.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/containerConfig.ts b/src/containerConfig.ts index f7826d3..10f0d02 100644 --- a/src/containerConfig.ts +++ b/src/containerConfig.ts @@ -1,7 +1,7 @@ import { IWorker, JobnikSDK } from '@map-colonies/jobnik-sdk'; import { jsLogger, type Logger } from '@map-colonies/js-logger'; import { TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; -import { SourceType } from '@map-colonies/raster-shared'; +import { SourceType, StorageProvider } from '@map-colonies/raster-shared'; import { getOtelMixin } from '@map-colonies/telemetry'; import { trace } from '@opentelemetry/api'; import { Registry } from 'prom-client'; @@ -13,7 +13,15 @@ import { getTracing } from '@common/tracing'; import type { StorageProviders } from '@src/cleaner/storageProviders'; import { ErrorHandler } from './cleaner/errors'; import { JobTrackerClient } from './cleaner/httpClients'; -import { buildFsStorageConfig, buildS3StorageConfig, FsStorageProvider, S3StorageProvider } from './cleaner/storageProviders'; +import { + buildFsStorageConfig, + buildRedisStorageConfig, + buildS3StorageConfig, + createRedisConnection, + FsStorageProvider, + RedisStorageProvider, + S3StorageProvider, +} from './cleaner/storageProviders'; import { DeleteStoredResourcesStrategy, StrategyFactory, TilesDeletionStrategy } from './cleaner/strategies'; import type { QueueConfig } from './cleaner/types'; import { ConfigType, getConfig } from './common/config'; @@ -39,6 +47,8 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise const cleanupStorageProviders = configInstance.get('storage.cleanupStorageProviders') as unknown as string[]; const fsStorageConfig = cleanupStorageProviders.includes(SourceType.FS) ? buildFsStorageConfig(configInstance, logger) : undefined; const s3StorageConfig = cleanupStorageProviders.includes(SourceType.S3) ? buildS3StorageConfig(configInstance, logger) : undefined; + const redisStorageConfig = cleanupStorageProviders.includes(StorageProvider.REDIS) ? buildRedisStorageConfig(configInstance, logger) : undefined; + const redisConnection = redisStorageConfig ? await createRedisConnection(redisStorageConfig, logger) : undefined; const dependencies: InjectionObject[] = [ { token: SERVICES.CONFIG, provider: { useValue: configInstance } }, @@ -105,6 +115,8 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise }, ...(fsStorageConfig ? [{ token: SERVICES.FS_STORAGE_CONFIG, provider: { useValue: fsStorageConfig } }] : []), ...(s3StorageConfig ? [{ token: SERVICES.S3_STORAGE_CONFIG, provider: { useValue: s3StorageConfig } }] : []), + ...(redisStorageConfig ? [{ token: SERVICES.REDIS_STORAGE_CONFIG, provider: { useValue: redisStorageConfig } }] : []), + ...(redisConnection ? [{ token: SERVICES.REDIS_CONNECTION, provider: { useValue: redisConnection } }] : []), { token: SERVICES.STORAGE_PROVIDERS, provider: { @@ -112,6 +124,7 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise const providers = { ...(s3StorageConfig && { [SourceType.S3]: container.resolve(S3StorageProvider) }), ...(fsStorageConfig && { [SourceType.FS]: container.resolve(FsStorageProvider) }), + ...(redisConnection && { [StorageProvider.REDIS]: container.resolve(RedisStorageProvider) }), }; return providers; }), @@ -164,7 +177,7 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise useFactory: (container) => { const worker = container.resolve(SERVICES.WORKER); return async (): Promise => { - await Promise.all([getTracing().stop(), worker.stop()]); + await Promise.all([getTracing().stop(), worker.stop(), redisConnection?.quit()]); }; }, }, From 7596d83744c032a881791c0c35ed90d6ed62994a Mon Sep 17 00:00:00 2001 From: almog8k Date: Mon, 14 Sep 2026 09:35:00 +0300 Subject: [PATCH 08/27] feat: remove unnecessary logging details for storage provider parameters --- src/cleaner/strategies/deleteStoredResourcesStrategy.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts index 7b9b934..1c0ba0c 100644 --- a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts +++ b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts @@ -30,9 +30,6 @@ export class DeleteStoredResourcesStrategy implements ITaskStrategy Date: Mon, 14 Sep 2026 09:43:47 +0300 Subject: [PATCH 09/27] test: add redis storage provider integration tests (MAPCO-11263) --- README.md | 30 ++++--- tests/integration/helpers/redisContainer.ts | 46 ++++++++++ tests/integration/helpers/redisTestKit.ts | 27 ++++++ .../redisStorageProvider.integration.spec.ts | 84 +++++++++++++++++++ 4 files changed, 174 insertions(+), 13 deletions(-) create mode 100644 tests/integration/helpers/redisContainer.ts create mode 100644 tests/integration/helpers/redisTestKit.ts create mode 100644 tests/integration/redisStorageProvider.integration.spec.ts diff --git a/README.md b/README.md index 015dd76..9923980 100644 --- a/README.md +++ b/README.md @@ -63,22 +63,26 @@ npm run test:watch #### Integration tests -`npm run test:integration` needs an S3-compatible server. By default it starts a Minio -[testcontainer](https://testcontainers.com/) automatically. - The image is pinned to the release deployed in our Azure environment; bump it in -`tests/integration/helpers/minioContainer.ts` when that environment moves. +`npm run test:integration` needs an S3-compatible server and a Redis server. By default it starts +a Minio and a Redis [testcontainer](https://testcontainers.com/) automatically. +The images are pinned to the releases deployed in our environments; bump them in +`tests/integration/helpers/minioContainer.ts` and `tests/integration/helpers/redisContainer.ts` +when those environments move. -To run against an already-running Minio instead, set `TEST_MINIO_ENDPOINT`: +To run against already-running servers instead, set the `TEST_*` variables: -| Variable | Default | Purpose | -| ------------------------ | ------------ | ------------------------------------------------------------------ | +| Variable | Default | Purpose | +| ------------------------ | ------------ | -------------------------------------------------------------------- | | `TEST_MINIO_ENDPOINT` | _(unset)_ | Point the suite at an existing Minio. Unset means start a container. | -| `TEST_MINIO_ACCESS_KEY` | `minioadmin` | Access key for that server. | -| `TEST_MINIO_SECRET_KEY` | `minioadmin` | Secret key for that server. | - -> **The suite creates and deletes buckets on whichever endpoint it is given.** Never point -> `TEST_MINIO_ENDPOINT` at a shared or deployed environment, and beware of leaving it exported in a -> shell profile. Each run prints which mode it selected and against which endpoint. +| `TEST_MINIO_ACCESS_KEY` | `minioadmin` | Access key for that server. | +| `TEST_MINIO_SECRET_KEY` | `minioadmin` | Secret key for that server. | +| `TEST_REDIS_HOST` | _(unset)_ | Point the suite at an existing Redis. Unset means start a container. | +| `TEST_REDIS_PORT` | `6379` | Port for that server. | + +> **The suite creates and deletes buckets on whichever Minio it is given, and calls `FLUSHDB` on +> whichever Redis it is given.** Never point `TEST_MINIO_ENDPOINT` or `TEST_REDIS_HOST` at a shared +> or deployed environment, and beware of leaving them exported in a shell profile. Each run prints +> which mode it selected and against which server. ## Customizing the Boilerplate diff --git a/tests/integration/helpers/redisContainer.ts b/tests/integration/helpers/redisContainer.ts new file mode 100644 index 0000000..d39802c --- /dev/null +++ b/tests/integration/helpers/redisContainer.ts @@ -0,0 +1,46 @@ +/* eslint-disable @typescript-eslint/naming-convention */ +import { GenericContainer, type StartedTestContainer } from 'testcontainers'; + +interface RedisHandle { + host: string; + port: number; + stop: () => Promise; +} + +/** Pinned to the image deployed in our environments; bump it when they move. */ +const REDIS_IMAGE = 'docker.io/bitnamilegacy/redis:7.2.1'; +const REDIS_PORT = 6379; + +async function startRedis(): Promise { + const externalHost = process.env.TEST_REDIS_HOST; + if (externalHost !== undefined && externalHost !== '') { + console.warn(`Redis: using EXTERNAL server at ${externalHost} (TEST_REDIS_HOST is set).`); + console.warn('Redis: this suite FLUSHES THE DB on that server. Unset TEST_REDIS_HOST to use a testcontainer.'); + return { + host: externalHost, + port: Number(process.env.TEST_REDIS_PORT ?? REDIS_PORT), + stop: async (): Promise => Promise.resolve(), + }; + } + + console.log(`Redis: TEST_REDIS_HOST is not set, starting testcontainer from ${REDIS_IMAGE}`); + const container: StartedTestContainer = await new GenericContainer(REDIS_IMAGE) + .withEnvironment({ ALLOW_EMPTY_PASSWORD: 'yes' }) + .withExposedPorts(REDIS_PORT) + .start(); + + return { + host: container.getHost(), + port: container.getMappedPort(REDIS_PORT), + stop: async (): Promise => { + try { + await container.stop(); + } catch (error) { + // Non-fatal: Ryuk reaps the container on test process exit, unless it is disabled. + console.warn('Failed to stop Redis container; relying on Ryuk to reap it', error); + } + }, + }; +} + +export { startRedis, type RedisHandle }; diff --git a/tests/integration/helpers/redisTestKit.ts b/tests/integration/helpers/redisTestKit.ts new file mode 100644 index 0000000..da3e6e1 --- /dev/null +++ b/tests/integration/helpers/redisTestKit.ts @@ -0,0 +1,27 @@ +// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class +import Redis from 'ioredis'; +import type { RedisHandle } from './redisContainer'; +import { TINY_TILE_BODY } from './tileFixtures'; + +function createTestRedisClient(handle: RedisHandle): Redis { + return new Redis({ host: handle.host, port: handle.port }); +} + +async function seedKeys(client: Redis, keys: string[]): Promise { + const pipeline = client.pipeline(); + for (const key of keys) { + pipeline.set(key, TINY_TILE_BODY); + } + await pipeline.exec(); +} + +async function listAllKeys(client: Redis): Promise { + const keys = await client.keys('*'); + return keys.sort(); +} + +async function flush(client: Redis): Promise { + await client.flushdb(); +} + +export { createTestRedisClient, seedKeys, listAllKeys, flush }; diff --git a/tests/integration/redisStorageProvider.integration.spec.ts b/tests/integration/redisStorageProvider.integration.spec.ts new file mode 100644 index 0000000..2964123 --- /dev/null +++ b/tests/integration/redisStorageProvider.integration.spec.ts @@ -0,0 +1,84 @@ +// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class +import type Redis from 'ioredis'; +import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest'; +import { createRedisConnection, RedisStorageProvider, type RedisStorageConfig } from '@src/cleaner/storageProviders'; +import { createMockLogger } from '../helpers/mocks'; +import { startRedis, type RedisHandle } from './helpers/redisContainer'; +import { createTestRedisClient, flush, listAllKeys, seedKeys } from './helpers/redisTestKit'; + +const PREFIX = 'eli_test-Orthophoto-redis_WorldCRS84'; +const OTHER_PREFIX = 'eli_test3-Orthophoto-redis_WorldCRS84'; + +describe('RedisStorageProvider against a real server', () => { + let handle: RedisHandle; + let client: Redis; + let connection: Redis; + let provider: RedisStorageProvider; + + beforeAll(async () => { + handle = await startRedis(); + client = createTestRedisClient(handle); + // Small paging values so a 50-key test crosses both SCAN and UNLINK boundaries + const config: RedisStorageConfig = { host: handle.host, port: handle.port, db: 0, scanCount: 10, batchSize: 4 }; + connection = await createRedisConnection(config, createMockLogger()); + provider = new RedisStorageProvider(config, connection, createMockLogger()); + }); + + afterAll(async () => { + await connection.quit(); + client.disconnect(); + await handle.stop(); + }); + + afterEach(async () => { + await flush(client); + }); + + describe('#delete', () => { + it('should delete exactly the listed keys and leave the rest of the layer intact', async () => { + const target = [`${PREFIX}-10-1227-704`, `${PREFIX}-10-1227-705`]; + const survivors = [`${PREFIX}-10-1228-704`, `${OTHER_PREFIX}-10-1227-704`]; + await seedKeys(client, [...target, ...survivors]); + + const result = await provider.delete(PREFIX, target); + + expect(result.deletedCount).toBe(2); + expect(await listAllKeys(client)).toEqual(survivors.sort()); + }); + + it('should treat unlinking absent keys as a clean no-op', async () => { + const result = await provider.delete(PREFIX, [`${PREFIX}-9-1-1`, `${PREFIX}-9-1-2`]); + + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); + }); + }); + + describe('#deleteResources', () => { + it('should wipe the whole prefix without touching a second layer', async () => { + const mine = [`${PREFIX}-10-1227-704`, `${PREFIX}-11-2454-1408`, `${PREFIX}-11-2454-1409`]; + const theirs = [`${OTHER_PREFIX}-10-1227-704`]; + await seedKeys(client, [...mine, ...theirs]); + + const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: PREFIX }); + + expect(result.deletedCount).toBe(3); + expect(await listAllKeys(client)).toEqual(theirs); + }); + + it('should page through a keyspace larger than scanCount and batchSize', async () => { + const keys = Array.from({ length: 50 }, (_, i) => `${PREFIX}-12-4892-${i}`); + await seedKeys(client, keys); + + const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: PREFIX }); + + expect(result.deletedCount).toBe(50); + expect(await listAllKeys(client)).toEqual([]); + }); + + it('should report a cold cache as zero deletions rather than an error', async () => { + const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: PREFIX }); + + expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); + }); + }); +}); From 98369d1112cf65e70b984ae62926a6759603ecb3 Mon Sep 17 00:00:00 2001 From: almog8k Date: Tue, 15 Sep 2026 16:55:46 +0300 Subject: [PATCH 10/27] feat: wire redis cache deletion jobs to the deletion strategies (MAPCO-11263) Register Update_Delete_Cache and Swap_Delete_Cache with the tiles-deletion task, mapped to TilesDeletionStrategy and DeleteStoredResourcesStrategy respectively, matching the jobs overseer's CacheDeletionJobCreator emits. DeleteStoredResourcesStrategy honors delaySeconds on Redis params so a swap wipe waits for the mapproxy reload window before removing keys. Enable the REDIS storage provider by default alongside FS and S3. --- config/default.json | 16 +++++++- .../deleteStoredResourcesStrategy.ts | 15 ++++++- src/common/constants.ts | 1 + src/containerConfig.ts | 22 +++++++++++ .../deleteStoredResourcesStrategy.spec.ts | 39 ++++++++++++++++++- 5 files changed, 90 insertions(+), 3 deletions(-) diff --git a/config/default.json b/config/default.json index 482974b..c88bf9d 100644 --- a/config/default.json +++ b/config/default.json @@ -34,6 +34,14 @@ { "job": "Delete_Layer", "task": "artifacts-deletion" + }, + { + "job": "Update_Delete_Cache", + "task": "tiles-deletion" + }, + { + "job": "Swap_Delete_Cache", + "task": "tiles-deletion" } ] } @@ -58,6 +66,12 @@ }, "deleteLayer": { "type": "Delete_Layer" + }, + "updateCacheDeletion": { + "type": "Update_Delete_Cache" + }, + "swapCacheDeletion": { + "type": "Swap_Delete_Cache" } }, "tasks": { @@ -77,7 +91,7 @@ "shouldResetTimeout": true }, "storage": { - "cleanupStorageProviders": ["FS", "S3"], + "cleanupStorageProviders": ["FS", "S3", "REDIS"], "s3": { "delete": { "batchSize": 1000 diff --git a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts index 1c0ba0c..3dcac52 100644 --- a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts +++ b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts @@ -1,8 +1,9 @@ +import { setTimeout } from 'node:timers/promises'; import type { Logger } from '@map-colonies/js-logger'; 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 { MS_PER_SECOND, SERVICES } from '@common/constants'; import { summarizeDeleteFailures, type IStorageProvider, type StorageProviders } from '@src/cleaner/storageProviders'; import { RecoverableError, UnrecoverableError } from '../errors'; import { validateSchema } from '../utils'; @@ -25,6 +26,10 @@ export class DeleteStoredResourcesStrategy implements ITaskStrategy { + if (delaySeconds === 0) { + return; + } + this.logger.info({ msg: 'Waiting for the reload window before deleting', prefix, delaySeconds }); + await setTimeout(delaySeconds * MS_PER_SECOND); + } + private resolveStorageProvider(storageProvider: K): IStorageProvider { this.logger.debug({ msg: `Resolving storage provider`, provider: storageProvider, providers: Object.keys(this.storageProviders) }); const provider = this.storageProviders[storageProvider]; diff --git a/src/common/constants.ts b/src/common/constants.ts index 3214b87..f476849 100644 --- a/src/common/constants.ts +++ b/src/common/constants.ts @@ -34,3 +34,4 @@ export const SERVICES = { } satisfies Record; export const PERCENTAGE_COMPLETE = 100; +export const MS_PER_SECOND = 1000; diff --git a/src/containerConfig.ts b/src/containerConfig.ts index 10f0d02..41bd3dc 100644 --- a/src/containerConfig.ts +++ b/src/containerConfig.ts @@ -171,6 +171,28 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise useClass: DeleteStoredResourcesStrategy, }, }, + // Redis cache invalidation created by overseer after an ingestion finalizes: an update deletes the + // tile ranges of the ingested footprint, a swap wipes the whole cache prefix. + { + token: getJobAndTaskToken({ + //TODO: when we create worker config schema we can move this to a constant and remove the cast + jobType: configInstance.get('jobDefinitions.jobs.updateCacheDeletion.type') as unknown as string, + taskType: configInstance.get('jobDefinitions.tasks.tilesDeletion.type') as unknown as string, + }), + provider: { + useClass: TilesDeletionStrategy, + }, + }, + { + token: getJobAndTaskToken({ + //TODO: when we create worker config schema we can move this to a constant and remove the cast + jobType: configInstance.get('jobDefinitions.jobs.swapCacheDeletion.type') as unknown as string, + taskType: configInstance.get('jobDefinitions.tasks.tilesDeletion.type') as unknown as string, + }), + provider: { + useClass: DeleteStoredResourcesStrategy, + }, + }, { token: 'onSignal', provider: { diff --git a/tests/strategies/deleteStoredResourcesStrategy.spec.ts b/tests/strategies/deleteStoredResourcesStrategy.spec.ts index 950b40f..d797732 100644 --- a/tests/strategies/deleteStoredResourcesStrategy.spec.ts +++ b/tests/strategies/deleteStoredResourcesStrategy.spec.ts @@ -1,3 +1,4 @@ +import { setTimeout as sleep } from 'node:timers/promises'; import type { Logger } from '@map-colonies/js-logger'; import { RedisDeleteStoredResourcesParams, @@ -6,7 +7,7 @@ import { type FsDeleteStoredResourcesParams, type S3DeleteStoredResourcesParams, } from '@map-colonies/raster-shared'; -import { beforeEach, describe, expect, it, type vi } from 'vitest'; +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 { DeleteStoredResourcesStrategy } from '@src/cleaner/strategies/deleteStoredResourcesStrategy'; @@ -16,6 +17,10 @@ import { createMockStoredResourcesDeletionStrategyConfig, createMockLogger, crea const S3_BUCKET = 'test-bucket'; const FS_SUB_PATH = 'test/artifacts/tiles'; const PREFIX = 'layer-redis_WorldCRS84'; +const RELOAD_WINDOW_SECONDS = 308; +const MS_PER_SECOND = 1000; + +vi.mock('node:timers/promises', () => ({ setTimeout: vi.fn().mockResolvedValue(undefined) })); const s3Params: S3DeleteStoredResourcesParams = { storageProvider: StorageProvider.S3, paths: ['layer1'], bucket: S3_BUCKET }; const fsParams: FsDeleteStoredResourcesParams = { storageProvider: StorageProvider.FS, paths: ['layer2'], subPath: FS_SUB_PATH }; @@ -27,18 +32,23 @@ describe('DeleteStoredResourcesStrategy', () => { let mockS3Provider: IStorageProvider<'S3'>; // eslint-disable-next-line @typescript-eslint/naming-convention let mockFsProvider: IStorageProvider<'FS'>; + // eslint-disable-next-line @typescript-eslint/naming-convention + let mockRedisProvider: IStorageProvider<'REDIS'>; let mockLogger: Logger; let mockConfig: ConfigType; beforeEach(() => { mockS3Provider = createMockStorageProvider(); mockFsProvider = createMockStorageProvider(); + mockRedisProvider = createMockStorageProvider({ targetExists: false }); mockLogger = createMockLogger(); + vi.mocked(sleep).mockClear(); mockConfig = createMockStoredResourcesDeletionStrategyConfig(); const storageProviders: StorageProviders = { [SourceType.FS]: mockFsProvider, [SourceType.S3]: mockS3Provider, + [StorageProvider.REDIS]: mockRedisProvider, }; strategy = new DeleteStoredResourcesStrategy(mockLogger, mockConfig, storageProviders); @@ -178,6 +188,33 @@ describe('DeleteStoredResourcesStrategy', () => { expect(mockS3Provider.deleteResources).toHaveBeenCalledWith({ paths: [], bucket: S3_BUCKET, storageProvider: 'S3' }); }); + describe('REDIS provider', () => { + it('should wipe the prefix immediately when the task carries no delay', async () => { + await strategy.execute(redisParams); + + expect(sleep).not.toHaveBeenCalled(); + expect(mockRedisProvider.deleteResources).toHaveBeenCalledWith(redisParams); + expect(mockS3Provider.deleteResources).not.toHaveBeenCalled(); + }); + + it('should wait out the reload window before wiping when the task carries delaySeconds', async () => { + const params: RedisDeleteStoredResourcesParams = { ...redisParams, delaySeconds: RELOAD_WINDOW_SECONDS }; + + await strategy.execute(params); + + expect(sleep).toHaveBeenCalledExactlyOnceWith(RELOAD_WINDOW_SECONDS * MS_PER_SECOND); + expect(vi.mocked(sleep).mock.invocationCallOrder[0]).toBeLessThan(vi.mocked(mockRedisProvider.deleteResources).mock.invocationCallOrder[0]!); + expect(mockRedisProvider.deleteResources).toHaveBeenCalledWith(params); + }); + + it('should not wait when delaySeconds is zero', async () => { + await strategy.execute({ ...redisParams, delaySeconds: 0 }); + + expect(sleep).not.toHaveBeenCalled(); + expect(mockRedisProvider.deleteResources).toHaveBeenCalledOnce(); + }); + }); + it('should rethrow error thrown by deleteResources', async () => { const expectedError = new Error('Custom'); (mockS3Provider.deleteResources as ReturnType).mockRejectedValueOnce(expectedError); From 5431a37412b78700cd6509dfc6a53cdf45039d24 Mon Sep 17 00:00:00 2001 From: almog8k Date: Tue, 15 Sep 2026 16:55:47 +0300 Subject: [PATCH 11/27] chore(helm): add redis storage config and cache deletion capability pairs (MAPCO-11263) storage.redis follows the global.redis hierarchy and merges over global.storage.redis and global.redis. REDIS_* env is emitted when REDIS is in cleanupStorageProviders. Also default cleanupStorageProviders to a list and add the env.jobnik.worker default the configmap already reads, so the chart renders without overrides. --- helm/templates/_tplValues.tpl | 5 +++++ helm/templates/configmap.yaml | 17 +++++++++++++++++ helm/values.yaml | 22 +++++++++++++++++++++- 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/helm/templates/_tplValues.tpl b/helm/templates/_tplValues.tpl index 55da75a..6ef84be 100644 --- a/helm/templates/_tplValues.tpl +++ b/helm/templates/_tplValues.tpl @@ -57,6 +57,11 @@ Custom definitions {{- include "common.tplvalues.merge" ( dict "values" ( list .Values.storage .Values.global.storage ) "context" . ) }} {{- end -}} +{{/* storage.redis, then global.storage.redis, then the shared global.redis block */}} +{{- define "common.redis.merged" -}} +{{- include "common.tplvalues.merge" ( dict "values" ( list ((.Values.storage).redis | default dict) ((.Values.global.storage).redis | default dict) (.Values.global.redis | default dict) ) "context" . ) }} +{{- end -}} + {{- define "common.ca.merged" -}} {{- include "common.tplvalues.merge" ( dict "values" ( list .Values.ca .Values.global.ca ) "context" . ) }} {{- end -}} diff --git a/helm/templates/configmap.yaml b/helm/templates/configmap.yaml index 95bc4c1..ac26ce3 100644 --- a/helm/templates/configmap.yaml +++ b/helm/templates/configmap.yaml @@ -3,6 +3,7 @@ {{- $storage := fromYaml (include "common.storage.merged" .) -}} {{- $s3 := ($storage.s3) | default dict -}} {{- $fs := ($storage.fs) | default dict -}} +{{- $redis := fromYaml (include "common.redis.merged" .) -}} {{- $internalPvc := (($fs).internalPvc) | default dict -}} {{- $fsBasePath := clean (printf "/%s" $internalPvc.mountPath) -}} {{- if .Values.enabled -}} @@ -74,6 +75,22 @@ data: {{- end }} FS_SUB_PATHS: {{ $subPaths | toJson | quote }} {{- end }} + {{- if has "REDIS" $storage.cleanupStorageProviders }} + REDIS_DELETE_BATCH_SIZE: {{ $redis.delete.batchSize | default 1000 | quote }} + REDIS_HOST: {{ $redis.host | quote }} + REDIS_PORT: {{ $redis.port | default 6379 | quote }} + REDIS_DB: {{ $redis.db | default 0 | quote }} + REDIS_SCAN_COUNT: {{ $redis.scanCount | default 1000 | quote }} + REDIS_TLS_ENABLED: {{ $redis.tlsEnabled | default false | quote }} + {{- with $redis.auth }} + {{- if .enabled }} + {{- if .enableRedisUser }} + REDIS_USERNAME: {{ .username | quote }} + {{- end }} + REDIS_PASSWORD: {{ .password | quote }} + {{- end }} + {{- end }} + {{- end }} {{- 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 70f01e9..d201b88 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -4,6 +4,7 @@ global: metrics: {} jobDefinitions: {} storage: {} + redis: {} serviceUrls: {} ca: {} @@ -13,7 +14,7 @@ serviceUrls: jobTracker: "" storage: - cleanupStorageProviders: {} + cleanupStorageProviders: [] # any of "FS", "S3", "REDIS" s3: delete: batchSize: 1000 @@ -30,6 +31,18 @@ storage: name: "" mountPath: "" tilesSubPath: "" # e.g. folder/tiles + redis: + enabled: false + host: "" + auth: + enabled: false + enableRedisUser: false + username: "" + password: "" + tlsEnabled: false + scanCount: 1000 + delete: + batchSize: 1000 mclabels: component: backend @@ -115,6 +128,9 @@ env: queue: heartbeatIntervalMs: 1000 dequeueIntervalMs: 3000 + jobnik: + worker: + concurrency: 1 worker: capabilities: pairs: @@ -122,6 +138,10 @@ env: task: "tiles-deletion" - job: "Ingestion_Swap_Update" task: "tiles-deletion" + - job: "Update_Delete_Cache" + task: "tiles-deletion" + - job: "Swap_Delete_Cache" + task: "tiles-deletion" httpRetry: attempts: 3 delay: "exponential" From 16b26cccdcc7877d65c031dcdf4c3a9ba6a5d43d Mon Sep 17 00:00:00 2001 From: almog8k Date: Tue, 15 Sep 2026 17:34:03 +0300 Subject: [PATCH 12/27] feat(redis): update Redis configuration to include port and database settings --- helm/templates/configmap.yaml | 2 +- helm/values.yaml | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/helm/templates/configmap.yaml b/helm/templates/configmap.yaml index ac26ce3..d2f1b92 100644 --- a/helm/templates/configmap.yaml +++ b/helm/templates/configmap.yaml @@ -84,7 +84,7 @@ data: REDIS_TLS_ENABLED: {{ $redis.tlsEnabled | default false | quote }} {{- with $redis.auth }} {{- if .enabled }} - {{- if .enableRedisUser }} + {{- if .username }} REDIS_USERNAME: {{ .username | quote }} {{- end }} REDIS_PASSWORD: {{ .password | quote }} diff --git a/helm/values.yaml b/helm/values.yaml index d201b88..8bb3962 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -32,11 +32,12 @@ storage: mountPath: "" tilesSubPath: "" # e.g. folder/tiles redis: - enabled: false host: "" + port: 6379 + db: 0 + # Same nesting as global.redis.auth; username is sent only when non-empty auth: enabled: false - enableRedisUser: false username: "" password: "" tlsEnabled: false From 43c2c11678295952e9ebce9d920dbe33c9a9d877 Mon Sep 17 00:00:00 2001 From: almog8k Date: Tue, 15 Sep 2026 17:57:28 +0300 Subject: [PATCH 13/27] test: cover both deletion strategies end to end on shared storage backends (MAPCO-11263) Run TilesDeletionStrategy and DeleteStoredResourcesStrategy through the real poller against Minio, a temp filesystem and a Redis testcontainer, each under the job it is registered for in production (Update_Delete_Cache / Swap_Delete_Cache for Redis). Both suites share one backend lifecycle, provider construction, task runner and outcome assertions, with per-strategy adapters layered on top. The standalone Redis provider spec is folded into the strategy suites. --- tests/helpers/fakes/tilesDeletionFakes.ts | 30 +++- ...toredResourcesStrategy.integration.spec.ts | 74 ++++++++ tests/integration/helpers/backendFixtures.ts | 139 ++++++++------ tests/integration/helpers/storageBackends.ts | 88 +++++++++ .../helpers/storedResourcesBackends.ts | 58 ++++++ tests/integration/helpers/testPoller.ts | 115 ++++++++++-- tests/integration/helpers/tileFixtures.ts | 83 ++++++--- .../helpers/tilesDeletionBackends.ts | 98 ++++++++++ .../helpers/tilesDeletionStrategyScenarios.ts | 8 +- .../redisStorageProvider.integration.spec.ts | 84 --------- .../tilesDeletionStrategy.integration.spec.ts | 170 ++++++------------ .../strategies/tilesDeletionStrategy.spec.ts | 2 +- 12 files changed, 637 insertions(+), 312 deletions(-) create mode 100644 tests/integration/deleteStoredResourcesStrategy.integration.spec.ts create mode 100644 tests/integration/helpers/storageBackends.ts create mode 100644 tests/integration/helpers/storedResourcesBackends.ts create mode 100644 tests/integration/helpers/tilesDeletionBackends.ts delete mode 100644 tests/integration/redisStorageProvider.integration.spec.ts diff --git a/tests/helpers/fakes/tilesDeletionFakes.ts b/tests/helpers/fakes/tilesDeletionFakes.ts index a290e1b..650c031 100644 --- a/tests/helpers/fakes/tilesDeletionFakes.ts +++ b/tests/helpers/fakes/tilesDeletionFakes.ts @@ -1,5 +1,11 @@ import { faker } from '@faker-js/faker'; -import { StorageProvider, type FsTilesDeletionParams, type S3TilesDeletionParams, type TileRange } from '@map-colonies/raster-shared'; +import { + StorageProvider, + type FsTilesDeletionParams, + type RedisTilesDeletionParams, + type S3TilesDeletionParams, + type TileRange, +} from '@map-colonies/raster-shared'; const EXTENSIONS = ['png', 'jpeg'] as const; @@ -25,6 +31,11 @@ function buildTilesRelativePath(): string { return `${faker.word.noun().toLowerCase()}/${faker.string.alphanumeric({ length: 6, casing: 'lower' })}`; } +/** Mirrors the observed `{layer}-{productType}-{grid}` cache prefix, dashes included. */ +function buildRedisPrefix(): string { + return `${faker.string.alphanumeric({ length: 8, casing: 'lower' })}-Orthophoto-WorldCRS84`; +} + function buildTilesDeletionCommon(overrides: Partial): TilesDeletionCommon { return { tilesRelativePath: overrides.tilesRelativePath ?? buildTilesRelativePath(), @@ -49,5 +60,20 @@ function buildFsTilesDeletionParams(overrides: Partial = }; } -export { buildTileRange, buildTilesRelativePath, buildS3TilesDeletionParams, buildFsTilesDeletionParams }; +function buildRedisTilesDeletionParams(overrides: Partial = {}): RedisTilesDeletionParams { + return { + storageProvider: StorageProvider.REDIS, + prefix: overrides.prefix ?? buildRedisPrefix(), + ranges: overrides.ranges ?? [buildTileRange()], + }; +} + +export { + buildTileRange, + buildTilesRelativePath, + buildRedisPrefix, + buildS3TilesDeletionParams, + buildFsTilesDeletionParams, + buildRedisTilesDeletionParams, +}; export type { TilesDeletionCommon }; diff --git a/tests/integration/deleteStoredResourcesStrategy.integration.spec.ts b/tests/integration/deleteStoredResourcesStrategy.integration.spec.ts new file mode 100644 index 0000000..d49180a --- /dev/null +++ b/tests/integration/deleteStoredResourcesStrategy.integration.spec.ts @@ -0,0 +1,74 @@ +import { faker } from '@faker-js/faker'; +import { describe, expect, it } from 'vitest'; +import { useStorageBackends } from './helpers/backendFixtures'; +import { sadPathProviders } from './helpers/storageBackends'; +import { storedResourcesBackends, type ResourceBackend } from './helpers/storedResourcesBackends'; +import { + expectTaskAcked, + expectTaskRejectedUnrecoverable, + runTask, + storedResourcesDeletionUnderTest, + type RunTaskParams, +} from './helpers/testPoller'; + +const ZOOM = 10; + +const layerName = (): string => `layer-${faker.string.alphanumeric({ length: 8, casing: 'lower' })}`; +const layerTiles = (backend: ResourceBackend, layer: string, count: number): string[] => + Array.from({ length: count }, (_, i) => backend.tileKey(layer, ZOOM, i, i + 1)); + +describe('stored resources deletion E2E (polling → strategy → real provider → ack)', () => { + const harness = useStorageBackends(); + const { storageContext } = harness; + + describe.each(storedResourcesBackends(harness))('$storageProvider provider', (backend) => { + const strategyUnderTest = storedResourcesDeletionUnderTest(backend.storageProvider); + const runStoredResourcesDeletion = async (params: unknown, overrides: Partial = {}): ReturnType => + runTask({ strategyUnderTest, providers: storageContext().providers, params, ...overrides }); + + describe('happy path', () => { + it('polls the task, wipes the whole layer, and acks while leaving other layers intact', async () => { + const target = layerName(); + const survivors = layerTiles(backend, layerName(), 3); + // 25 tiles is enough to cross the small Redis SCAN and UNLINK page sizes the fixtures configure. + await backend.seed([...layerTiles(backend, target, 25), ...survivors]); + + const run = await runStoredResourcesDeletion(backend.buildParams([target])); + + expect(await backend.list()).toEqual([...survivors].sort()); + expectTaskAcked(run); + }); + + it('acks a task for a layer that holds nothing, so redelivery is idempotent', async () => { + const survivors = layerTiles(backend, layerName(), 2); + await backend.seed(survivors); + + const run = await runStoredResourcesDeletion(backend.buildParams([layerName()])); + + expect(await backend.list()).toEqual([...survivors].sort()); + expectTaskAcked(run); + }); + + it.runIf(backend.supportsMultipleLayers)('wipes every listed layer in one task and nothing else', async () => { + const targets = [layerName(), layerName()]; + const survivors = layerTiles(backend, layerName(), 2); + await backend.seed([...targets.flatMap((layer) => layerTiles(backend, layer, 4)), ...survivors]); + + const run = await runStoredResourcesDeletion(backend.buildParams(targets)); + + expect(await backend.list()).toEqual([...survivors].sort()); + expectTaskAcked(run); + }); + }); + + describe('sad path', () => { + it('rejects the task as unrecoverable without acking', async () => { + const { params, reason } = backend.sadPath; + + const run = await runStoredResourcesDeletion(params(), { providers: sadPathProviders(backend.sadPath, storageContext()) }); + + expectTaskRejectedUnrecoverable(run, reason); + }); + }); + }); +}); diff --git a/tests/integration/helpers/backendFixtures.ts b/tests/integration/helpers/backendFixtures.ts index 1206b35..9e77f13 100644 --- a/tests/integration/helpers/backendFixtures.ts +++ b/tests/integration/helpers/backendFixtures.ts @@ -1,18 +1,25 @@ -import { join } from 'node:path'; import { faker } from '@faker-js/faker'; import { container } from 'tsyringe'; -import { StorageProvider, type FsTilesDeletionParams, type S3TilesDeletionParams } from '@map-colonies/raster-shared'; +import { afterAll, afterEach, beforeAll, beforeEach } from 'vitest'; +import { StorageProvider } from '@map-colonies/raster-shared'; import type { S3Client } from '@aws-sdk/client-s3'; -import { S3StorageProvider, FsStorageProvider, type FsStorageConfig, type StorageProviders } from '@src/cleaner/storageProviders'; -import { buildFsTilesDeletionParams, buildS3TilesDeletionParams, type TilesDeletionCommon } from '../../helpers/fakes/tilesDeletionFakes'; +// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class +import type Redis from 'ioredis'; +import { + createRedisConnection, + FsStorageProvider, + RedisStorageProvider, + S3StorageProvider, + type FsStorageConfig, + type RedisStorageConfig, + type StorageProviders, +} from '@src/cleaner/storageProviders'; import { createMockLogger } from '../../helpers/mocks'; import { startMinio, type MinioHandle } from './minioContainer'; -import { buildS3StorageConfigForMinio, createTestS3Client, deleteBucket, ensureBucket, listAllKeys, putManyTiles } from './s3TestKit'; -import { listAllFiles, makeTempFsBase, rmBase, writeManyTiles } from './fsTestKit'; - -/** The storage providers a tiles-deletion task can actually be routed to today (REDIS is not implemented). */ -type PathAddressedProvider = Exclude; -type PathAddressedParams = S3TilesDeletionParams | FsTilesDeletionParams; +import { buildS3StorageConfigForMinio, createTestS3Client, deleteBucket, ensureBucket } from './s3TestKit'; +import { makeTempFsBase, rmBase } from './fsTestKit'; +import { startRedis, type RedisHandle } from './redisContainer'; +import { createTestRedisClient, flush } from './redisTestKit'; /** * The only sub path FS deletion is allowed under, mirroring `storage.fs.subPaths` in config. @@ -20,10 +27,17 @@ type PathAddressedParams = S3TilesDeletionParams | FsTilesDeletionParams; */ const FS_ALLOWED_SUB_PATH = 'artifacts/tiles'; const FS_DELETE_BATCH_SIZE = 100; +// Small paging values so a modest seed crosses both SCAN and UNLINK boundaries +const REDIS_SCAN_COUNT = 10; +const REDIS_DELETE_BATCH_SIZE = 4; interface BackendHandles { minio: MinioHandle; s3Client: S3Client; + redis: RedisHandle; + /** Seeding and listing client; the provider gets its own connection. */ + redisClient: Redis; + redisConnection: Redis; } interface TestStorageContext { @@ -36,15 +50,23 @@ interface TestStorageContext { fsSubPath: string; } +function buildRedisStorageConfig(redis: RedisHandle, batchSize = REDIS_DELETE_BATCH_SIZE): RedisStorageConfig { + return { host: redis.host, port: redis.port, db: 0, scanCount: REDIS_SCAN_COUNT, batchSize }; +} + async function startBackends(): Promise { - const minio = await startMinio(); + const [minio, redis] = await Promise.all([startMinio(), startRedis()]); const s3Client = createTestS3Client(minio); - return { minio, s3Client }; + const redisClient = createTestRedisClient(redis); + const redisConnection = await createRedisConnection(buildRedisStorageConfig(redis), createMockLogger()); + return { minio, s3Client, redis, redisClient, redisConnection }; } -async function stopBackends({ minio, s3Client }: BackendHandles): Promise { +async function stopBackends({ minio, s3Client, redis, redisClient, redisConnection }: BackendHandles): Promise { s3Client.destroy(); - await minio.stop(); + await redisConnection.quit(); + redisClient.disconnect(); + await Promise.all([minio.stop(), redis.stop()]); } /** Per-provider delete batch sizes; each falls back to the production-shaped default. */ @@ -56,9 +78,15 @@ interface ProviderBatchSizes { * instead of chunking, so tiles deletion is unaffected by it. */ fs?: number; + /** Keys per `UNLINK` the Redis provider chunks its input into. */ + redis?: number; } -function buildProviders(minio: MinioHandle, fsBasePath: string, batchSizes: ProviderBatchSizes = {}): StorageProviders { +function buildProviders( + { minio, redis, redisConnection }: BackendHandles, + fsBasePath: string, + batchSizes: ProviderBatchSizes = {} +): StorageProviders { const fsStorageConfig: FsStorageConfig = { basePath: fsBasePath, subPaths: [FS_ALLOWED_SUB_PATH], @@ -69,15 +97,16 @@ function buildProviders(minio: MinioHandle, fsBasePath: string, batchSizes: Prov return { [StorageProvider.S3]: new S3StorageProvider({ ...s3StorageConfig, batchSize: batchSizes.s3 ?? s3StorageConfig.batchSize }, createMockLogger()), [StorageProvider.FS]: new FsStorageProvider(fsStorageConfig, createMockLogger()), + [StorageProvider.REDIS]: new RedisStorageProvider(buildRedisStorageConfig(redis, batchSizes.redis), redisConnection, createMockLogger()), }; } -async function setupTestStorageContext({ minio, s3Client }: BackendHandles): Promise { +async function setupTestStorageContext(handles: BackendHandles): Promise { const bucket = `test-${faker.string.alphanumeric({ length: 16, casing: 'lower' })}`; - await ensureBucket(s3Client, bucket); + await ensureBucket(handles.s3Client, bucket); const fsBasePath = await makeTempFsBase(); - return { providers: buildProviders(minio, fsBasePath), bucket, fsBasePath, fsSubPath: FS_ALLOWED_SUB_PATH }; + return { providers: buildProviders(handles, fsBasePath), bucket, fsBasePath, fsSubPath: FS_ALLOWED_SUB_PATH }; } /** @@ -86,58 +115,50 @@ async function setupTestStorageContext({ minio, s3Client }: BackendHandles): Pro * providers point at the same bucket and base path, so seeding stays unchanged. */ function providersWithBatchSizes(handles: BackendHandles, storageContext: TestStorageContext, batchSizes: ProviderBatchSizes): StorageProviders { - return buildProviders(handles.minio, storageContext.fsBasePath, batchSizes); + return buildProviders(handles, storageContext.fsBasePath, batchSizes); } async function teardownTestStorageContext(handles: BackendHandles, storageContext: TestStorageContext): Promise { try { - await Promise.all([deleteBucket(handles.s3Client, storageContext.bucket), rmBase(storageContext.fsBasePath)]); + await Promise.all([deleteBucket(handles.s3Client, storageContext.bucket), rmBase(storageContext.fsBasePath), flush(handles.redisClient)]); } finally { + // The poller helpers register into the global container; start every test from a clean one. container.reset(); } } -interface ProviderBackend { - storageProvider: PathAddressedProvider; - /** - * Task params carrying this backend's own storage locator — bucket for S3, sub path for FS. - * `overrides` pins the otherwise-faked tile fields, e.g. explicit `ranges`. - */ - buildParams: (tilesRelativePath: string, overrides?: Partial) => PathAddressedParams; - /** Seeds tiles at paths relative to the storage target. */ - seed: (paths: string[]) => Promise; - /** Lists surviving tiles as paths relative to the storage target. */ - list: (prefix: string) => Promise; +/** Lazy views over the suite's backends, safe to capture at `describe.each` collection time. */ +interface StorageBackendsHarness { + handles: () => BackendHandles; + storageContext: () => TestStorageContext; } -function s3Backend(handles: () => BackendHandles, perTest: () => TestStorageContext): ProviderBackend { - return { - storageProvider: StorageProvider.S3, - buildParams: (tilesRelativePath, overrides) => buildS3TilesDeletionParams({ ...overrides, bucket: perTest().bucket, tilesRelativePath }), - seed: async (paths) => putManyTiles(handles().s3Client, perTest().bucket, paths), - list: async (prefix) => listAllKeys(handles().s3Client, perTest().bucket, prefix), - }; -} +/** + * Registers the whole backend lifecycle for a suite: containers once per file, a fresh bucket, + * FS base path and empty Redis DB per test. Call inside the top-level `describe`. + */ +function useStorageBackends(): StorageBackendsHarness { + let handles: BackendHandles; + let storageContext: TestStorageContext; -function fsBackend(perTest: () => TestStorageContext): ProviderBackend { - // The provider joins base path + sub path itself, so the test seeds and reads the same root. - const targetRoot = (): string => join(perTest().fsBasePath, perTest().fsSubPath); - return { - storageProvider: StorageProvider.FS, - buildParams: (tilesRelativePath, overrides) => buildFsTilesDeletionParams({ ...overrides, subPath: perTest().fsSubPath, tilesRelativePath }), - seed: async (paths) => writeManyTiles(targetRoot(), paths), - list: async (prefix) => (await listAllFiles(targetRoot())).filter((path) => path.startsWith(prefix)), - }; + beforeAll(async () => { + handles = await startBackends(); + }); + + afterAll(async () => { + await stopBackends(handles); + }); + + beforeEach(async () => { + storageContext = await setupTestStorageContext(handles); + }); + + afterEach(async () => { + await teardownTestStorageContext(handles, storageContext); + }); + + return { handles: () => handles, storageContext: () => storageContext }; } -export { - FS_ALLOWED_SUB_PATH, - startBackends, - stopBackends, - setupTestStorageContext, - teardownTestStorageContext, - providersWithBatchSizes, - s3Backend, - fsBackend, -}; -export type { BackendHandles, TestStorageContext, ProviderBackend, PathAddressedParams, ProviderBatchSizes }; +export { FS_ALLOWED_SUB_PATH, useStorageBackends, providersWithBatchSizes }; +export type { BackendHandles, TestStorageContext, ProviderBatchSizes, StorageBackendsHarness }; diff --git a/tests/integration/helpers/storageBackends.ts b/tests/integration/helpers/storageBackends.ts new file mode 100644 index 0000000..5ca608e --- /dev/null +++ b/tests/integration/helpers/storageBackends.ts @@ -0,0 +1,88 @@ +import { join } from 'node:path'; +import { StorageProvider } from '@map-colonies/raster-shared'; +import type { StorageProviders } from '@src/cleaner/storageProviders'; +import type { BackendHandles, TestStorageContext } from './backendFixtures'; +import { listAllFiles, writeManyTiles } from './fsTestKit'; +import { listAllKeys, putManyTiles } from './s3TestKit'; +import { listAllKeys as listAllRedisKeys, seedKeys } from './redisTestKit'; + +const DEFAULT_TILE_EXTENSION = 'png'; + +/** + * Raw access to one real storage backend, independent of any strategy: what a test seeds + * before a task runs and reads back afterwards. Strategy suites layer their own task-shaped + * vocabulary on top of this. + */ +interface StorageBackend { + storageProvider: StorageProvider; + /** + * Names one tile of `layer` the way this backend stores it, relative to the storage target — + * exactly as the strategies generate the keys they hand the provider. + */ + tileKey: (layer: string, zoom: number, x: number, y: number, extension?: string) => string; + /** Seeds tiles at keys relative to the storage target. */ + seed: (keys: string[]) => Promise; + /** Sorted keys under the storage target, narrowed to those starting with `prefix` when given. */ + list: (prefix?: string) => Promise; +} + +/** A task the strategy or provider must refuse as a producer bug rather than retry. */ +interface SadPath { + params: () => Params; + /** Replaces the context's providers, e.g. to leave this backend unregistered. */ + providers?: (storageContext: TestStorageContext) => StorageProviders; + /** Substring of the rejection reason. */ + reason: string; +} + +/** `{layer}/{z}/{x}/{y}.{ext}` — the S3 object key and the FS path under the sub path. */ +const pathTileKey: StorageBackend['tileKey'] = (layer, zoom, x, y, extension = DEFAULT_TILE_EXTENSION) => `${layer}/${zoom}/${x}/${y}.${extension}`; + +/** `{prefix}-{z}-{x}-{y}` — the mapproxy cache key, no extension. */ +const redisTileKey: StorageBackend['tileKey'] = (prefix, zoom, x, y) => `${prefix}-${zoom}-${x}-${y}`; + +/** The context's providers minus `storageProvider`, so a task addressed to it is unsupported. */ +function providersWithout(storageProvider: StorageProvider): (storageContext: TestStorageContext) => StorageProviders { + return ({ providers }) => { + const rest = { ...providers }; + delete rest[storageProvider]; + return rest; + }; +} + +/** The providers a sad-path task should run against: its own override, else the context's. */ +function sadPathProviders({ providers }: SadPath, storageContext: TestStorageContext): StorageProviders { + return providers ? providers(storageContext) : storageContext.providers; +} + +function s3StorageBackend(handles: () => BackendHandles, perTest: () => TestStorageContext): StorageBackend { + return { + storageProvider: StorageProvider.S3, + tileKey: pathTileKey, + seed: async (keys) => putManyTiles(handles().s3Client, perTest().bucket, keys), + list: async (prefix) => listAllKeys(handles().s3Client, perTest().bucket, prefix), + }; +} + +function fsStorageBackend(perTest: () => TestStorageContext): StorageBackend { + // The provider joins base path + sub path itself, so the test seeds and reads the same root. + const targetRoot = (): string => join(perTest().fsBasePath, perTest().fsSubPath); + return { + storageProvider: StorageProvider.FS, + tileKey: pathTileKey, + seed: async (keys) => writeManyTiles(targetRoot(), keys), + list: async (prefix = '') => (await listAllFiles(targetRoot())).filter((path) => path.startsWith(prefix)), + }; +} + +function redisStorageBackend(handles: () => BackendHandles): StorageBackend { + return { + storageProvider: StorageProvider.REDIS, + tileKey: redisTileKey, + seed: async (keys) => seedKeys(handles().redisClient, keys), + list: async (prefix = '') => (await listAllRedisKeys(handles().redisClient)).filter((key) => key.startsWith(prefix)), + }; +} + +export { s3StorageBackend, fsStorageBackend, redisStorageBackend, providersWithout, sadPathProviders }; +export type { StorageBackend, SadPath }; diff --git a/tests/integration/helpers/storedResourcesBackends.ts b/tests/integration/helpers/storedResourcesBackends.ts new file mode 100644 index 0000000..77d3232 --- /dev/null +++ b/tests/integration/helpers/storedResourcesBackends.ts @@ -0,0 +1,58 @@ +import { StorageProvider, type DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; +import type { StorageBackendsHarness } from './backendFixtures'; +import { fsStorageBackend, providersWithout, redisStorageBackend, s3StorageBackend, type SadPath, type StorageBackend } from './storageBackends'; + +/** Adapts one storage backend to the layer-shaped vocabulary of the stored-resources E2E suite. */ +interface ResourceBackend extends StorageBackend { + /** Every path/prefix travels in the task params; each backend names its own locator. */ + buildParams: (layers: string[]) => DeleteStoredResourcesParams; + /** Redis wipes exactly one prefix per task, so multi-layer scenarios skip it. */ + supportsMultipleLayers: boolean; + sadPath: SadPath; +} + +function s3ResourceBackend({ handles, storageContext }: StorageBackendsHarness): ResourceBackend { + return { + ...s3StorageBackend(handles, storageContext), + buildParams: (layers) => ({ storageProvider: StorageProvider.S3, bucket: storageContext().bucket, paths: layers }), + supportsMultipleLayers: true, + sadPath: { + params: () => ({ storageProvider: StorageProvider.S3, bucket: 'no-such-bucket', paths: ['layer'] }), + reason: 'Bucket does not exist', + }, + }; +} + +function fsResourceBackend({ storageContext }: StorageBackendsHarness): ResourceBackend { + return { + ...fsStorageBackend(storageContext), + buildParams: (layers) => ({ storageProvider: StorageProvider.FS, subPath: storageContext().fsSubPath, paths: layers }), + supportsMultipleLayers: true, + sadPath: { + // Climbs out of the allowed sub path, which the FS provider refuses outright + params: () => ({ storageProvider: StorageProvider.FS, subPath: storageContext().fsSubPath, paths: ['../../escaped'] }), + reason: 'Cannot delete paths outside the configured sub paths', + }, + }; +} + +function redisResourceBackend({ handles }: StorageBackendsHarness): ResourceBackend { + return { + ...redisStorageBackend(handles), + buildParams: ([prefix]) => ({ storageProvider: StorageProvider.REDIS, prefix: prefix! }), + supportsMultipleLayers: false, + sadPath: { + params: () => ({ storageProvider: StorageProvider.REDIS, prefix: 'layer' }), + providers: providersWithout(StorageProvider.REDIS), + reason: 'Unsupported storage provider REDIS', + }, + }; +} + +/** One adapter per real backend, for `describe.each`. */ +function storedResourcesBackends(harness: StorageBackendsHarness): ResourceBackend[] { + return [s3ResourceBackend(harness), fsResourceBackend(harness), redisResourceBackend(harness)]; +} + +export { storedResourcesBackends }; +export type { ResourceBackend }; diff --git a/tests/integration/helpers/testPoller.ts b/tests/integration/helpers/testPoller.ts index c0cd0f0..b2c550e 100644 --- a/tests/integration/helpers/testPoller.ts +++ b/tests/integration/helpers/testPoller.ts @@ -1,33 +1,58 @@ /* eslint-disable @typescript-eslint/unbound-method */ -import { vi } from 'vitest'; +import { expect, vi } from 'vitest'; import { container } from 'tsyringe'; import type { ITaskResponse, TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; +import { StorageProvider } from '@map-colonies/raster-shared'; +import type { constructor } from 'tsyringe/dist/typings/types'; import { SERVICES } from '@src/common/constants'; import { getJobAndTaskToken } from '@src/common/dependencyRegistration'; import { TaskPoller } from '@src/worker/taskPoller'; import { ErrorHandler } from '@src/cleaner/errors'; -import { StrategyFactory, TilesDeletionStrategy } from '@src/cleaner/strategies'; +import { DeleteStoredResourcesStrategy, StrategyFactory, TilesDeletionStrategy } from '@src/cleaner/strategies'; +import type { ITaskStrategy } from '@src/cleaner/strategies/taskStrategy'; import type { StorageProviders } from '@src/cleaner/storageProviders'; import type { JobTrackerClient } from '@src/cleaner/httpClients'; import type { PollingPairConfig } from '@src/cleaner/types'; +import { buildTask } from '../../helpers/fakes/taskFakes'; import { createMockLogger, createMockQueueClient, createMockStrategyConfig, createMockJobTrackerClient } from '../../helpers/mocks'; -const TASK_TYPE = 'tiles-deletion'; -const JOB_TYPE = 'Ingestion_Update'; -const POLLING_PAIR: PollingPairConfig = { jobType: JOB_TYPE, taskType: TASK_TYPE, maxAttempts: 3 }; const POLLER_WATCHDOG_MS = 30_000; +/** A strategy together with the job+task pair it is registered under in `containerConfig`. */ +interface StrategyUnderTest { + pollingPair: PollingPairConfig; + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- any params shape; ITaskStrategy is generic over it + strategy: constructor>; +} + +const MAX_ATTEMPTS = 3; +const TILES_DELETION_TASK = 'tiles-deletion'; + +/** + * The job each strategy is registered under in `containerConfig`, per storage kind: path stores + * are cleaned on ingestion and layer deletion, the Redis cache on the overseer's cache-deletion jobs. + */ +function tilesDeletionUnderTest(storageProvider: StorageProvider): StrategyUnderTest { + const jobType = storageProvider === StorageProvider.REDIS ? 'Update_Delete_Cache' : 'Ingestion_Update'; + return { pollingPair: { jobType, taskType: TILES_DELETION_TASK, maxAttempts: MAX_ATTEMPTS }, strategy: TilesDeletionStrategy }; +} + +function storedResourcesDeletionUnderTest(storageProvider: StorageProvider): StrategyUnderTest { + const jobType = storageProvider === StorageProvider.REDIS ? 'Swap_Delete_Cache' : 'Delete_Layer'; + return { pollingPair: { jobType, taskType: TILES_DELETION_TASK, maxAttempts: MAX_ATTEMPTS }, strategy: DeleteStoredResourcesStrategy }; +} + /** * Returns a queue client whose `dequeue` hands out `task` exactly once for the * matching pair, then null forever. `ack` / `reject` invoke `onTerminal()` so * the caller can stop the poller and assert. */ -function buildSingleShotQueue(task: ITaskResponse, onTerminal: () => void): QueueClient { +function buildSingleShotQueue(task: ITaskResponse, onTerminal: () => void, pair: PollingPairConfig): QueueClient { const queue = createMockQueueClient(); let handedOut = false; vi.mocked(queue.dequeue).mockImplementation(((jobType: string, taskType: string) => { - if (handedOut || jobType !== JOB_TYPE || taskType !== TASK_TYPE) { + if (handedOut || jobType !== pair.jobType || taskType !== pair.taskType) { return null; } handedOut = true; @@ -50,6 +75,7 @@ interface TestPollerParams { task: ITaskResponse; /** Config keys merged over the strategy defaults, e.g. the batching knobs. */ configOverrides?: Record; + strategyUnderTest?: StrategyUnderTest; } interface TestPoller { @@ -59,33 +85,43 @@ interface TestPoller { } /** - * Wires the real TaskPoller → StrategyFactory → TilesDeletionStrategy pipeline, - * with real S3/FS providers (supplied by the caller) and a single-shot fake - * QueueClient. The poller terminates as soon as ack/reject fires. + * Wires the real TaskPoller → StrategyFactory → strategy pipeline, with real + * providers (supplied by the caller) and a single-shot fake QueueClient. + * The poller terminates as soon as ack/reject fires. * - * The strategy reads nothing but batching knobs from config — every storage locator + * The strategies read nothing but batching knobs from config — every storage locator * travels in the task params — so the mock config carries no bucket or base path. */ -function buildPoller({ providers, task, configOverrides = {} }: TestPollerParams): TestPoller { +function buildPoller({ + providers, + task, + configOverrides = {}, + strategyUnderTest = tilesDeletionUnderTest(StorageProvider.S3), +}: TestPollerParams): TestPoller { + const { pollingPair, strategy } = strategyUnderTest; const config = createMockStrategyConfig({ 'queue.dequeueIntervalMs': 0, ...configOverrides }); container.register(SERVICES.LOGGER, { useValue: createMockLogger() }); container.register(SERVICES.CONFIG, { useValue: config }); container.register(SERVICES.STORAGE_PROVIDERS, { useValue: providers }); - const queueClient = buildSingleShotQueue(task, () => { - void poller.stop(); - }); + const queueClient = buildSingleShotQueue( + task, + () => { + void poller.stop(); + }, + pollingPair + ); container.register(SERVICES.QUEUE_CLIENT, { useValue: queueClient }); // StrategyFactory resolves strategies by the combined job+task token, not the task type alone. - container.register(getJobAndTaskToken(POLLING_PAIR), { useClass: TilesDeletionStrategy }); + container.register(getJobAndTaskToken(pollingPair), { useClass: strategy }); const jobTrackerClient = createMockJobTrackerClient(); container.register(SERVICES.JOB_TRACKER_CLIENT, { useValue: jobTrackerClient }); const strategyFactory = container.resolve(StrategyFactory); const errorHandler = container.resolve(ErrorHandler); - const poller = new TaskPoller(createMockLogger(), config, queueClient, strategyFactory, errorHandler, [POLLING_PAIR], jobTrackerClient); + const poller = new TaskPoller(createMockLogger(), config, queueClient, strategyFactory, errorHandler, [pollingPair], jobTrackerClient); const runSingleTask = async (): Promise => { const startPromise = poller.start(); @@ -99,5 +135,46 @@ function buildPoller({ providers, task, configOverrides = {} }: TestPollerParams return { queueClient, jobTrackerClient, runSingleTask }; } -export { TASK_TYPE, JOB_TYPE, POLLING_PAIR, buildSingleShotQueue, buildPoller }; -export type { TestPoller, TestPollerParams }; +interface RunTaskParams extends Omit { + strategyUnderTest: StrategyUnderTest; + params: unknown; +} + +interface TaskRun { + task: ITaskResponse; + queueClient: QueueClient; + jobTrackerClient: JobTrackerClient; +} + +/** Builds a task carrying `params`, drives it once through the poller and hands back what to assert on. */ +async function runTask({ strategyUnderTest, params, ...pollerParams }: RunTaskParams): Promise { + const task = buildTask({ type: strategyUnderTest.pollingPair.taskType, parameters: params }); + const { runSingleTask, ...clients } = buildPoller({ ...pollerParams, task, strategyUnderTest }); + await runSingleTask(); + return { task, ...clients }; +} + +/** The task reached the queue's happy terminal state and the job tracker heard about it. */ +function expectTaskAcked({ task, queueClient, jobTrackerClient }: TaskRun): void { + expect(queueClient.ack).toHaveBeenCalledWith(task.jobId, task.id); + expect(queueClient.reject).not.toHaveBeenCalled(); + expect(jobTrackerClient.notify).toHaveBeenCalledWith(task.id); +} + +/** The task was rejected with `shouldRetry=false` and a reason containing `reason`. */ +function expectTaskRejectedUnrecoverable({ task, queueClient, jobTrackerClient }: TaskRun, reason: string): void { + expect(queueClient.reject).toHaveBeenCalledWith(task.jobId, task.id, false, expect.stringContaining(reason)); + expect(queueClient.ack).not.toHaveBeenCalled(); + expect(jobTrackerClient.notify).toHaveBeenCalledWith(task.id); +} + +export { + tilesDeletionUnderTest, + storedResourcesDeletionUnderTest, + buildSingleShotQueue, + buildPoller, + runTask, + expectTaskAcked, + expectTaskRejectedUnrecoverable, +}; +export type { StrategyUnderTest, TestPoller, TestPollerParams, RunTaskParams, TaskRun }; diff --git a/tests/integration/helpers/tileFixtures.ts b/tests/integration/helpers/tileFixtures.ts index 105d485..d473581 100644 --- a/tests/integration/helpers/tileFixtures.ts +++ b/tests/integration/helpers/tileFixtures.ts @@ -1,26 +1,24 @@ import type { TileRange } from '@map-colonies/raster-shared'; -import type { TilesDeletionCommon } from '../../helpers/fakes/tilesDeletionFakes'; // First 4 bytes of a PNG file header (\x89 P N G). Content is arbitrary; // eslint-disable-next-line @typescript-eslint/no-magic-numbers const TINY_TILE_BODY = Buffer.from([0x89, 0x50, 0x4e, 0x47]); -/** - * Every path returned here is relative to the storage target — the bucket for S3, - * the task's sub path for FS — exactly as `TilesDeletionStrategy` generates them. - */ -function tilePathsForRange(range: TileRange, tilesRelativePath: string, fileExtension: string): string[] { - const paths: string[] = []; +/** Names one tile of a fixed layer, relative to the storage target; see `StorageBackend.tileKey`. */ +type TileKeyFormat = (zoom: number, x: number, y: number) => string; + +function tileKeysForRange(range: TileRange, format: TileKeyFormat): string[] { + const keys: string[] = []; for (let x = range.minX; x <= range.maxX; x++) { for (let y = range.minY; y <= range.maxY; y++) { - paths.push(`${tilesRelativePath}/${range.zoom}/${x}/${y}.${fileExtension}`); + keys.push(format(range.zoom, x, y)); } } - return paths; + return keys; } -function tilePathsForRanges(params: TilesDeletionCommon): string[] { - return params.ranges.flatMap((range) => tilePathsForRange(range, params.tilesRelativePath, params.fileExtension)); +function tileKeysForRanges(ranges: TileRange[], format: TileKeyFormat): string[] { + return ranges.flatMap((range) => tileKeysForRange(range, format)); } function paddedRange(range: TileRange, padding: number): TileRange { @@ -35,20 +33,20 @@ function paddedRange(range: TileRange, padding: number): TileRange { /** * Builds the set of tiles inside the `paddedRange` but **outside** the original - * `params.ranges`. These tiles are seeded into the backend before the strategy + * `ranges`. These tiles are seeded into the backend before the strategy * runs and MUST survive — they prove "only the supplied ranges are deleted". * * Deduped, because with several ranges the padded regions can overlap and yield - * the same bystander path twice. + * the same bystander key twice. */ -function extraTilePathsAroundRanges(params: TilesDeletionCommon, padding: number): string[] { - const targetSet = new Set(tilePathsForRanges(params)); +function extraTileKeysAroundRanges(ranges: TileRange[], format: TileKeyFormat, padding: number): string[] { + const targetSet = new Set(tileKeysForRanges(ranges, format)); const extras = new Set(); - for (const range of params.ranges) { + for (const range of ranges) { const wider = paddedRange(range, padding); - for (const path of tilePathsForRange(wider, params.tilesRelativePath, params.fileExtension)) { - if (!targetSet.has(path)) { - extras.add(path); + for (const key of tileKeysForRange(wider, format)) { + if (!targetSet.has(key)) { + extras.add(key); } } } @@ -56,28 +54,55 @@ function extraTilePathsAroundRanges(params: TilesDeletionCommon, padding: number } /** - * Returns one tile path per (range × zoomDelta) pair, placed at a zoom level + * Returns one tile key per (range × zoomDelta) pair, placed at a zoom level * adjacent to each range's zoom. * - * Like `extraTilePathsAroundRanges`, the returned paths are seeded before the + * Like `extraTileKeysAroundRanges`, the returned keys are seeded before the * test and must survive after the strategy runs. * */ -function extraTilePathsAtAdjacentZooms(params: TilesDeletionCommon, zoomDeltas: number[]): string[] { - const targetSet = new Set(tilePathsForRanges(params)); +function extraTileKeysAtAdjacentZooms(ranges: TileRange[], format: TileKeyFormat, zoomDeltas: number[]): string[] { + const targetSet = new Set(tileKeysForRanges(ranges, format)); const extras = new Set(); - for (const range of params.ranges) { + for (const range of ranges) { for (const delta of zoomDeltas) { const zoom = range.zoom + delta; if (zoom < 0) continue; - const path = `${params.tilesRelativePath}/${zoom}/${range.minX}/${range.minY}.${params.fileExtension}`; + const key = format(zoom, range.minX, range.minY); // With several ranges, one range's neighbouring zoom can be another range's own zoom, - // where the path is a deletion target rather than a bystander that must survive. - if (!targetSet.has(path)) { - extras.add(path); + // where the key is a deletion target rather than a bystander that must survive. + if (!targetSet.has(key)) { + extras.add(key); } } } return [...extras]; } -export { tilePathsForRange, tilePathsForRanges, paddedRange, extraTilePathsAroundRanges, extraTilePathsAtAdjacentZooms, TINY_TILE_BODY }; +const BYSTANDER_PADDING = 1; +/** One zoom below and one above each range. */ +// eslint-disable-next-line @typescript-eslint/no-magic-numbers +const BYSTANDER_ZOOM_DELTAS = [-1, 1]; + +/** + * Tiles that neighbour `ranges` in X/Y and at the adjacent zooms, deduped. Seeded alongside the + * targets, they must all survive the run — the one assertion every deletion scenario shares. + */ +function bystanderTileKeys(ranges: TileRange[], format: TileKeyFormat): string[] { + return [ + ...new Set([ + ...extraTileKeysAroundRanges(ranges, format, BYSTANDER_PADDING), + ...extraTileKeysAtAdjacentZooms(ranges, format, BYSTANDER_ZOOM_DELTAS), + ]), + ]; +} + +export { + tileKeysForRange, + tileKeysForRanges, + paddedRange, + extraTileKeysAroundRanges, + extraTileKeysAtAdjacentZooms, + bystanderTileKeys, + TINY_TILE_BODY, +}; +export type { TileKeyFormat }; diff --git a/tests/integration/helpers/tilesDeletionBackends.ts b/tests/integration/helpers/tilesDeletionBackends.ts new file mode 100644 index 0000000..5eab90e --- /dev/null +++ b/tests/integration/helpers/tilesDeletionBackends.ts @@ -0,0 +1,98 @@ +import { faker } from '@faker-js/faker'; +import { StorageProvider, type TileRange, type TilesDeletionParams } from '@map-colonies/raster-shared'; +import { buildFsTilesDeletionParams, buildRedisTilesDeletionParams, buildS3TilesDeletionParams } from '../../helpers/fakes/tilesDeletionFakes'; +import type { ProviderBatchSizes, StorageBackendsHarness } from './backendFixtures'; +import { fsStorageBackend, providersWithout, redisStorageBackend, s3StorageBackend, type SadPath, type StorageBackend } from './storageBackends'; +import type { TileKeyFormat } from './tileFixtures'; + +type ParamsOf

= Extract; +type PathParams = ParamsOf<'S3' | 'FS'>; + +/** + * Adapts one storage backend to the range-shaped vocabulary of the tiles-deletion E2E suite. + * Params-typed members are method signatures (bivariant) rather than function properties, so a + * `TilesDeletionBackend` still fits the `describe.each` list typed on the whole union. + */ +/* eslint-disable @typescript-eslint/method-signature-style */ +interface TilesDeletionBackend extends StorageBackend { + /** Batch sizes that make this backend's provider re-chunk a strategy batch of `chunkSize`. */ + chunkedBatchSizes: (chunkSize: number) => ProviderBatchSizes; + sadPath: SadPath; + /** + * Task params carrying a fresh locator for this backend — bucket + relative path for S3, + * sub path + relative path for FS, key prefix for Redis. `overrides` pins the otherwise-faked + * tile fields, e.g. explicit `ranges`. + */ + buildParams(overrides?: { ranges?: TileRange[] }): Params; + /** Names one tile of `params` the way this backend stores it — what `seed` and `list` speak. */ + keyFormat(params: Params): TileKeyFormat; + /** Surviving tiles under the locator of `params`. */ + listTarget(params: Params): Promise; +} +/* eslint-enable @typescript-eslint/method-signature-style */ + +const freshRelativePath = (): string => `${faker.string.uuid()}/${faker.string.uuid()}`; + +/** + * S3 and FS both address tiles as `{tilesRelativePath}/{z}/{x}/{y}.{ext}` under their locator + * and both probe the target before deleting, so they differ only in how params are built and + * whether the provider re-chunks a batch. + */ +function pathTilesBackend( + storage: StorageBackend, + buildParams: TilesDeletionBackend['buildParams'], + chunkedBatchSizes: TilesDeletionBackend['chunkedBatchSizes'] +): TilesDeletionBackend { + return { + ...storage, + buildParams, + keyFormat: (params) => (zoom, x, y) => storage.tileKey(params.tilesRelativePath, zoom, x, y, params.fileExtension), + listTarget: async (params) => storage.list(`${params.tilesRelativePath}/`), + chunkedBatchSizes, + // Nothing is seeded, so the path the task points at was never created on the backend. + sadPath: { params: buildParams, reason: 'storage target does not exist' }, + }; +} + +function s3TilesBackend({ handles, storageContext }: StorageBackendsHarness): TilesDeletionBackend> { + return pathTilesBackend( + s3StorageBackend(handles, storageContext), + (overrides) => buildS3TilesDeletionParams({ ...overrides, bucket: storageContext().bucket, tilesRelativePath: freshRelativePath() }), + (chunkSize) => ({ s3: chunkSize }) + ); +} + +function fsTilesBackend({ storageContext }: StorageBackendsHarness): TilesDeletionBackend> { + return pathTilesBackend( + fsStorageBackend(storageContext), + (overrides) => buildFsTilesDeletionParams({ ...overrides, subPath: storageContext().fsSubPath, tilesRelativePath: freshRelativePath() }), + // `delete` fans every path out at once, so no FS batch size re-chunks a strategy batch. + () => ({}) + ); +} + +function redisTilesBackend({ handles }: StorageBackendsHarness): TilesDeletionBackend> { + const storage = redisStorageBackend(handles); + return { + ...storage, + buildParams: (overrides) => buildRedisTilesDeletionParams(overrides), + keyFormat: (params) => (zoom, x, y) => storage.tileKey(params.prefix, zoom, x, y), + listTarget: async (params) => storage.list(`${params.prefix}-`), + chunkedBatchSizes: (chunkSize) => ({ redis: chunkSize }), + // Redis has no target to probe — a cold prefix is a clean no-op — so the only unrecoverable + // path is a task addressed to a provider this instance was not configured with. + sadPath: { + params: () => buildRedisTilesDeletionParams(), + providers: providersWithout(StorageProvider.REDIS), + reason: 'Unsupported storage provider REDIS', + }, + }; +} + +/** One adapter per real backend, for `describe.each`. */ +function tilesDeletionBackends(harness: StorageBackendsHarness): TilesDeletionBackend[] { + return [s3TilesBackend(harness), fsTilesBackend(harness), redisTilesBackend(harness)]; +} + +export { tilesDeletionBackends }; +export type { TilesDeletionBackend }; diff --git a/tests/integration/helpers/tilesDeletionStrategyScenarios.ts b/tests/integration/helpers/tilesDeletionStrategyScenarios.ts index 3d8d615..826acf6 100644 --- a/tests/integration/helpers/tilesDeletionStrategyScenarios.ts +++ b/tests/integration/helpers/tilesDeletionStrategyScenarios.ts @@ -3,16 +3,16 @@ import type { TileRange } from '@map-colonies/raster-shared'; /** * Sized so one run crosses every batching boundary: 180 tiles at `strategyBatchSize` 40 is * four full batches flushed in two groups of `strategyConcurrency`, followed by a 20-tile - * trailing partial batch. Each 40-key batch then re-chunks into 25 + 15 inside the S3 - * provider's own delete loop, which a production-sized batch never reaches — its cap is 1000 - * keys, well above the strategy's batch size. + * trailing partial batch. Each 40-key batch then re-chunks into 25 + 15 inside the provider's + * own delete loop (S3's `DeleteObjects`, Redis' `UNLINK`), which a production-sized batch never + * reaches — the caps are far above the strategy's batch size. */ const BATCHING_SCENARIO = { range: { zoom: 12, minX: 0, maxX: 11, minY: 0, maxY: 14 } satisfies TileRange, tileCount: 180, strategyBatchSize: 40, strategyConcurrency: 2, - s3ChunkSize: 25, + providerChunkSize: 25, /** One `updateProgress` call per flush of `strategyConcurrency` full batches. */ expectedFlushCount: 2, /** Math.round((80 / 180) * 100) and Math.round((160 / 180) * 100) — one per flush. */ diff --git a/tests/integration/redisStorageProvider.integration.spec.ts b/tests/integration/redisStorageProvider.integration.spec.ts deleted file mode 100644 index 2964123..0000000 --- a/tests/integration/redisStorageProvider.integration.spec.ts +++ /dev/null @@ -1,84 +0,0 @@ -// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class -import type Redis from 'ioredis'; -import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest'; -import { createRedisConnection, RedisStorageProvider, type RedisStorageConfig } from '@src/cleaner/storageProviders'; -import { createMockLogger } from '../helpers/mocks'; -import { startRedis, type RedisHandle } from './helpers/redisContainer'; -import { createTestRedisClient, flush, listAllKeys, seedKeys } from './helpers/redisTestKit'; - -const PREFIX = 'eli_test-Orthophoto-redis_WorldCRS84'; -const OTHER_PREFIX = 'eli_test3-Orthophoto-redis_WorldCRS84'; - -describe('RedisStorageProvider against a real server', () => { - let handle: RedisHandle; - let client: Redis; - let connection: Redis; - let provider: RedisStorageProvider; - - beforeAll(async () => { - handle = await startRedis(); - client = createTestRedisClient(handle); - // Small paging values so a 50-key test crosses both SCAN and UNLINK boundaries - const config: RedisStorageConfig = { host: handle.host, port: handle.port, db: 0, scanCount: 10, batchSize: 4 }; - connection = await createRedisConnection(config, createMockLogger()); - provider = new RedisStorageProvider(config, connection, createMockLogger()); - }); - - afterAll(async () => { - await connection.quit(); - client.disconnect(); - await handle.stop(); - }); - - afterEach(async () => { - await flush(client); - }); - - describe('#delete', () => { - it('should delete exactly the listed keys and leave the rest of the layer intact', async () => { - const target = [`${PREFIX}-10-1227-704`, `${PREFIX}-10-1227-705`]; - const survivors = [`${PREFIX}-10-1228-704`, `${OTHER_PREFIX}-10-1227-704`]; - await seedKeys(client, [...target, ...survivors]); - - const result = await provider.delete(PREFIX, target); - - expect(result.deletedCount).toBe(2); - expect(await listAllKeys(client)).toEqual(survivors.sort()); - }); - - it('should treat unlinking absent keys as a clean no-op', async () => { - const result = await provider.delete(PREFIX, [`${PREFIX}-9-1-1`, `${PREFIX}-9-1-2`]); - - expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); - }); - }); - - describe('#deleteResources', () => { - it('should wipe the whole prefix without touching a second layer', async () => { - const mine = [`${PREFIX}-10-1227-704`, `${PREFIX}-11-2454-1408`, `${PREFIX}-11-2454-1409`]; - const theirs = [`${OTHER_PREFIX}-10-1227-704`]; - await seedKeys(client, [...mine, ...theirs]); - - const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: PREFIX }); - - expect(result.deletedCount).toBe(3); - expect(await listAllKeys(client)).toEqual(theirs); - }); - - it('should page through a keyspace larger than scanCount and batchSize', async () => { - const keys = Array.from({ length: 50 }, (_, i) => `${PREFIX}-12-4892-${i}`); - await seedKeys(client, keys); - - const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: PREFIX }); - - expect(result.deletedCount).toBe(50); - expect(await listAllKeys(client)).toEqual([]); - }); - - it('should report a cold cache as zero deletions rather than an error', async () => { - const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: PREFIX }); - - expect(result).toEqual({ failures: new Map(), deletedCount: 0 }); - }); - }); -}); diff --git a/tests/integration/tilesDeletionStrategy.integration.spec.ts b/tests/integration/tilesDeletionStrategy.integration.spec.ts index 8c679f5..1700feb 100644 --- a/tests/integration/tilesDeletionStrategy.integration.spec.ts +++ b/tests/integration/tilesDeletionStrategy.integration.spec.ts @@ -1,121 +1,73 @@ /* eslint-disable @typescript-eslint/unbound-method */ -import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'; -import { faker } from '@faker-js/faker'; -import { buildTask } from '../helpers/fakes/taskFakes'; -import { - fsBackend, - providersWithBatchSizes, - s3Backend, - setupTestStorageContext, - startBackends, - stopBackends, - teardownTestStorageContext, - type BackendHandles, - type PathAddressedParams, - type TestStorageContext, -} from './helpers/backendFixtures'; -import { buildPoller, TASK_TYPE } from './helpers/testPoller'; -import { extraTilePathsAroundRanges, extraTilePathsAtAdjacentZooms, tilePathsForRanges } from './helpers/tileFixtures'; +import { describe, expect, it } from 'vitest'; +import { providersWithBatchSizes, useStorageBackends } from './helpers/backendFixtures'; +import { sadPathProviders } from './helpers/storageBackends'; +import { expectTaskAcked, expectTaskRejectedUnrecoverable, runTask, tilesDeletionUnderTest, type RunTaskParams } from './helpers/testPoller'; +import { tilesDeletionBackends } from './helpers/tilesDeletionBackends'; +import { bystanderTileKeys, tileKeysForRanges } from './helpers/tileFixtures'; import { BATCHING_SCENARIO, MULTIPLE_RANGES_SCENARIO } from './helpers/tilesDeletionStrategyScenarios'; describe('tiles deletion E2E (polling → strategy → real provider → ack)', () => { - let handles: BackendHandles; - let storageContext: TestStorageContext; + const harness = useStorageBackends(); + const { handles, storageContext } = harness; - beforeAll(async () => { - handles = await startBackends(); - }); - - afterAll(async () => { - await stopBackends(handles); - }); - - beforeEach(async () => { - storageContext = await setupTestStorageContext(handles); - }); - - afterEach(async () => { - await teardownTestStorageContext(handles, storageContext); - }); + describe.each(tilesDeletionBackends(harness))('$storageProvider provider', (backend) => { + const strategyUnderTest = tilesDeletionUnderTest(backend.storageProvider); + const runTilesDeletion = async (params: unknown, overrides: Partial = {}): ReturnType => + runTask({ strategyUnderTest, providers: storageContext().providers, params, ...overrides }); - describe.each([ - s3Backend( - () => handles, - () => storageContext - ), - fsBackend(() => storageContext), - ])('$storageProvider provider', (backend) => { describe('happy path', () => { it('polls the task, runs the strategy against the chosen provider, and acks after deleting only the requested tiles', async () => { - const tilesRelativePath = `${faker.string.uuid()}/${faker.string.uuid()}`; - const params: PathAddressedParams = backend.buildParams(tilesRelativePath); - const target = tilePathsForRanges(params); - const extras = [...extraTilePathsAroundRanges(params, 2), ...extraTilePathsAtAdjacentZooms(params, [-1, 1])]; - await backend.seed([...target, ...extras]); - const task = buildTask({ type: TASK_TYPE, parameters: params }); - - const { runSingleTask, queueClient, jobTrackerClient } = buildPoller({ providers: storageContext.providers, task }); - await runSingleTask(); - const remaining = await backend.list(`${tilesRelativePath}/`); - - expect(remaining.sort()).toEqual([...extras].sort()); - expect(queueClient.ack).toHaveBeenCalledWith(task.jobId, task.id); - expect(queueClient.reject).not.toHaveBeenCalled(); - expect(jobTrackerClient.notify).toHaveBeenCalledWith(task.id); + const params = backend.buildParams(); + const format = backend.keyFormat(params); + const bystanders = bystanderTileKeys(params.ranges, format); + await backend.seed([...tileKeysForRanges(params.ranges, format), ...bystanders]); + + const run = await runTilesDeletion(params); + + expect(await backend.listTarget(params)).toEqual([...bystanders].sort()); + expectTaskAcked(run); }); it('acks a redelivered task whose tiles are already gone, leaving unrelated tiles intact', async () => { - const tilesRelativePath = `${faker.string.uuid()}/${faker.string.uuid()}`; - const params: PathAddressedParams = backend.buildParams(tilesRelativePath); - const extras = [...extraTilePathsAroundRanges(params, 2), ...extraTilePathsAtAdjacentZooms(params, [-1, 1])]; - await backend.seed([...tilePathsForRanges(params), ...extras]); - - const firstTask = buildTask({ type: TASK_TYPE, parameters: params }); - await buildPoller({ providers: storageContext.providers, task: firstTask }).runSingleTask(); + const params = backend.buildParams(); + const format = backend.keyFormat(params); + const bystanders = bystanderTileKeys(params.ranges, format); + await backend.seed([...tileKeysForRanges(params.ranges, format), ...bystanders]); + await runTilesDeletion(params); // The queue delivers at least once, so the same task can come back after a crash or a // visibility timeout. Deleting an already-deleted tile must still be a success. - const redeliveredTask = buildTask({ type: TASK_TYPE, parameters: params }); - const { runSingleTask, queueClient, jobTrackerClient } = buildPoller({ providers: storageContext.providers, task: redeliveredTask }); - await runSingleTask(); - const remaining = await backend.list(`${tilesRelativePath}/`); - - expect(queueClient.ack).toHaveBeenCalledWith(redeliveredTask.jobId, redeliveredTask.id); - expect(queueClient.reject).not.toHaveBeenCalled(); - expect(jobTrackerClient.notify).toHaveBeenCalledWith(redeliveredTask.id); - expect(remaining.sort()).toEqual([...extras].sort()); + const redelivered = await runTilesDeletion(params); + + expect(await backend.listTarget(params)).toEqual([...bystanders].sort()); + expectTaskAcked(redelivered); }); }); describe('batching', () => { it('flushes full batches concurrently, chunks each inside the provider, and reports progress per flush', async () => { - const tilesRelativePath = `${faker.string.uuid()}/${faker.string.uuid()}`; - const params: PathAddressedParams = backend.buildParams(tilesRelativePath, { ranges: [BATCHING_SCENARIO.range] }); - const target = tilePathsForRanges(params); + const params = backend.buildParams({ ranges: [BATCHING_SCENARIO.range] }); + const format = backend.keyFormat(params); + const target = tileKeysForRanges(params.ranges, format); // Guards the arithmetic the progress percentages below are derived from. expect(target).toHaveLength(BATCHING_SCENARIO.tileCount); + const bystanders = bystanderTileKeys(params.ranges, format); + await backend.seed([...target, ...bystanders]); - const extras = extraTilePathsAroundRanges(params, 1); - await backend.seed([...target, ...extras]); - const task = buildTask({ type: TASK_TYPE, parameters: params }); - - const { runSingleTask, queueClient, jobTrackerClient } = buildPoller({ - providers: providersWithBatchSizes(handles, storageContext, { s3: BATCHING_SCENARIO.s3ChunkSize }), - task, + const run = await runTilesDeletion(params, { + providers: providersWithBatchSizes(handles(), storageContext(), backend.chunkedBatchSizes(BATCHING_SCENARIO.providerChunkSize)), configOverrides: { 'strategies.tilesDeletion.batchSize': BATCHING_SCENARIO.strategyBatchSize, 'strategies.tilesDeletion.concurrency': BATCHING_SCENARIO.strategyConcurrency, }, }); - await runSingleTask(); - const remaining = await backend.list(`${tilesRelativePath}/`); - expect(remaining.sort()).toEqual([...extras].sort()); - expect(queueClient.ack).toHaveBeenCalledWith(task.jobId, task.id); - expect(queueClient.reject).not.toHaveBeenCalled(); - expect(jobTrackerClient.notify).toHaveBeenCalledWith(task.id); + expect(await backend.listTarget(params)).toEqual([...bystanders].sort()); + expectTaskAcked(run); // One update per flush of `concurrency` full batches. The trailing partial batch reports // nothing, and the terminal 100% is the queue's task-ack rather than the strategy's job. + const { task, queueClient } = run; expect(queueClient.updateProgress).toHaveBeenCalledTimes(BATCHING_SCENARIO.expectedFlushCount); expect(queueClient.updateProgress).toHaveBeenNthCalledWith(1, task.jobId, task.id, BATCHING_SCENARIO.firstFlushPercentage); expect(queueClient.updateProgress).toHaveBeenNthCalledWith(2, task.jobId, task.id, BATCHING_SCENARIO.secondFlushPercentage); @@ -124,42 +76,32 @@ describe('tiles deletion E2E (polling → strategy → real provider → ack)', describe('multiple ranges', () => { it('deletes every tile of every range, leaving neighbouring tiles and zooms of each intact', async () => { - const tilesRelativePath = `${faker.string.uuid()}/${faker.string.uuid()}`; - const params: PathAddressedParams = backend.buildParams(tilesRelativePath, { ranges: MULTIPLE_RANGES_SCENARIO.ranges }); - const target = tilePathsForRanges(params); + const params = backend.buildParams({ ranges: MULTIPLE_RANGES_SCENARIO.ranges }); + const format = backend.keyFormat(params); + const target = tileKeysForRanges(params.ranges, format); // Guards against a range set that silently collapses to fewer tiles than intended. expect(target).toHaveLength(MULTIPLE_RANGES_SCENARIO.tileCount); + const bystanders = bystanderTileKeys(params.ranges, format); + await backend.seed([...target, ...bystanders]); - const extras = [...new Set([...extraTilePathsAroundRanges(params, 1), ...extraTilePathsAtAdjacentZooms(params, [-1, 1])])]; - await backend.seed([...target, ...extras]); - const task = buildTask({ type: TASK_TYPE, parameters: params }); - - const { runSingleTask, queueClient, jobTrackerClient } = buildPoller({ providers: storageContext.providers, task }); - await runSingleTask(); - const remaining = await backend.list(`${tilesRelativePath}/`); + const run = await runTilesDeletion(params); // Every range contributes deletions, and nothing outside them is touched — so a // regression that only walked the first range would leave survivors here. - expect(remaining.sort()).toEqual([...extras].sort()); - expect(queueClient.ack).toHaveBeenCalledWith(task.jobId, task.id); - expect(queueClient.reject).not.toHaveBeenCalled(); - expect(jobTrackerClient.notify).toHaveBeenCalledWith(task.id); + expect(await backend.listTarget(params)).toEqual([...bystanders].sort()); + expectTaskAcked(run); }); }); describe('sad path', () => { - it('rejects the task as unrecoverable without acking when the storage target does not exist', async () => { - // Nothing is seeded, so the path the task points at was never created on the backend. - const params: PathAddressedParams = backend.buildParams(`${faker.string.uuid()}/${faker.string.uuid()}`); - const task = buildTask({ type: TASK_TYPE, parameters: params }); - - const { runSingleTask, queueClient, jobTrackerClient } = buildPoller({ providers: storageContext.providers, task }); - await runSingleTask(); - - // shouldRetry=false is the whole point: a missing target is a producer bug, not a blip. - expect(queueClient.reject).toHaveBeenCalledWith(task.jobId, task.id, false, expect.stringContaining('storage target does not exist')); - expect(queueClient.ack).not.toHaveBeenCalled(); - expect(jobTrackerClient.notify).toHaveBeenCalledWith(task.id); + it('rejects the task as unrecoverable without acking', async () => { + const { params, reason } = backend.sadPath; + + const run = await runTilesDeletion(params(), { providers: sadPathProviders(backend.sadPath, storageContext()) }); + + // shouldRetry=false is the whole point: a missing target or an unknown provider is a + // producer bug, not a blip. + expectTaskRejectedUnrecoverable(run, reason); }); }); }); diff --git a/tests/strategies/tilesDeletionStrategy.spec.ts b/tests/strategies/tilesDeletionStrategy.spec.ts index d7eca23..7c52f6a 100644 --- a/tests/strategies/tilesDeletionStrategy.spec.ts +++ b/tests/strategies/tilesDeletionStrategy.spec.ts @@ -239,7 +239,7 @@ describe('TilesDeletionStrategy', () => { describe('REDIS provider', () => { const redisParams: RedisTilesDeletionParams = { storageProvider: StorageProvider.REDIS, - prefix: 'eli_test-Orthophoto-redis_WorldCRS84', + prefix: 'test-Orthophoto-redis_WorldCRS84', ranges: [{ zoom: 3, minX: 1, maxX: 2, minY: 5, maxY: 6 }], }; let MockRedisProvider: IStorageProvider<'REDIS'>; From db868017546032d3b1ff56dee00756744e1efc43 Mon Sep 17 00:00:00 2001 From: almog8k Date: Tue, 15 Sep 2026 18:16:49 +0300 Subject: [PATCH 14/27] fix(minio): update MINIO_IMAGE to use Quay.io instead of Docker Hub --- tests/integration/helpers/minioContainer.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/helpers/minioContainer.ts b/tests/integration/helpers/minioContainer.ts index 97772be..adfe3b7 100644 --- a/tests/integration/helpers/minioContainer.ts +++ b/tests/integration/helpers/minioContainer.ts @@ -10,9 +10,9 @@ interface MinioHandle { /** * Pinned to the release running on our Azure deployment, so the suite exercises the same server - * behavior we deploy against. + * behavior we deploy against. Pulled from Quay: Docker Hub no longer serves `minio/minio` */ -const MINIO_IMAGE = 'minio/minio:RELEASE.2025-07-23T15-54-02Z'; +const MINIO_IMAGE = 'quay.io/minio/minio:RELEASE.2025-07-23T15-54-02Z'; const MINIO_PORT = 9000; const DEFAULT_USER = 'minioadmin'; const DEFAULT_PASSWORD = 'minioadmin'; From 935de358540c4c4ea7a2c5e4744d1ac5403c324e Mon Sep 17 00:00:00 2001 From: almog8k Date: Wed, 23 Sep 2026 16:47:15 +0300 Subject: [PATCH 15/27] chore(deps): bump @map-colonies/raster-shared to 9.0.0 --- 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 d3d5f50..06af458 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.2", "@map-colonies/mc-utils": "^6.0.1", - "@map-colonies/raster-shared": "^9.0.0-alpha.0", + "@map-colonies/raster-shared": "^9.0.0", "@map-colonies/read-pkg": "^1.0.0", "@map-colonies/schemas": "^1.20.0", "@map-colonies/telemetry": "^10.0.1", @@ -2568,9 +2568,9 @@ "license": "ISC" }, "node_modules/@map-colonies/raster-shared": { - "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==", + "version": "9.0.0", + "resolved": "https://registry.npmjs.org/@map-colonies/raster-shared/-/raster-shared-9.0.0.tgz", + "integrity": "sha512-RBrcPmVDm6i0/+odACjw/4HJYRuqWXwlJWnNXDm2sqamx74r+IxuTUYtRJHKktFwJc3r+g1YuT60VBeW9Lcaww==", "license": "ISC", "dependencies": { "@map-colonies/mc-priority-queue": "^9.1.0", diff --git a/package.json b/package.json index 1c68732..df4beb8 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "@map-colonies/js-logger": "^5.0.0", "@map-colonies/mc-priority-queue": "^9.1.2", "@map-colonies/mc-utils": "^6.0.1", - "@map-colonies/raster-shared": "^9.0.0-alpha.0", + "@map-colonies/raster-shared": "^9.0.0", "@map-colonies/read-pkg": "^1.0.0", "@map-colonies/schemas": "^1.20.0", "@map-colonies/telemetry": "^10.0.1", From b9cbcbc043acc6309f22ea09c98f15e3ed6e927c Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 09:56:06 +0300 Subject: [PATCH 16/27] refactor(redis): move scanCount under storage.redis.delete (MAPCO-11263) --- config/custom-environment-variables.json | 8 ++++---- config/default.json | 6 +++--- helm/templates/configmap.yaml | 2 +- helm/values.yaml | 2 +- src/cleaner/storageProviders/storageConfig.ts | 10 +++++----- tests/helpers/mocks.ts | 4 ++-- tests/storageProviders/storageConfig.spec.ts | 9 +++++++-- 7 files changed, 23 insertions(+), 18 deletions(-) diff --git a/config/custom-environment-variables.json b/config/custom-environment-variables.json index 779709b..a9a5deb 100644 --- a/config/custom-environment-variables.json +++ b/config/custom-environment-variables.json @@ -116,6 +116,10 @@ "batchSize": { "__name": "REDIS_DELETE_BATCH_SIZE", "__format": "number" + }, + "scanCount": { + "__name": "REDIS_SCAN_COUNT", + "__format": "number" } }, "host": "REDIS_HOST", @@ -127,10 +131,6 @@ "__name": "REDIS_DB", "__format": "number" }, - "scanCount": { - "__name": "REDIS_SCAN_COUNT", - "__format": "number" - }, "username": "REDIS_USERNAME", "password": "REDIS_PASSWORD", "tlsEnabled": { diff --git a/config/default.json b/config/default.json index c88bf9d..4d379c7 100644 --- a/config/default.json +++ b/config/default.json @@ -114,12 +114,12 @@ }, "redis": { "delete": { - "batchSize": 1000 + "batchSize": 1000, + "scanCount": 1000 }, "host": "localhost", "port": 6379, - "db": 0, - "scanCount": 1000 + "db": 0 } }, "strategies": { diff --git a/helm/templates/configmap.yaml b/helm/templates/configmap.yaml index d2f1b92..792b211 100644 --- a/helm/templates/configmap.yaml +++ b/helm/templates/configmap.yaml @@ -80,7 +80,7 @@ data: REDIS_HOST: {{ $redis.host | quote }} REDIS_PORT: {{ $redis.port | default 6379 | quote }} REDIS_DB: {{ $redis.db | default 0 | quote }} - REDIS_SCAN_COUNT: {{ $redis.scanCount | default 1000 | quote }} + REDIS_SCAN_COUNT: {{ $redis.delete.scanCount | default 1000 | quote }} REDIS_TLS_ENABLED: {{ $redis.tlsEnabled | default false | quote }} {{- with $redis.auth }} {{- if .enabled }} diff --git a/helm/values.yaml b/helm/values.yaml index 8bb3962..9a779e9 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -41,9 +41,9 @@ storage: username: "" password: "" tlsEnabled: false - scanCount: 1000 delete: batchSize: 1000 + scanCount: 1000 mclabels: component: backend diff --git a/src/cleaner/storageProviders/storageConfig.ts b/src/cleaner/storageProviders/storageConfig.ts index 2906491..22e24ca 100644 --- a/src/cleaner/storageProviders/storageConfig.ts +++ b/src/cleaner/storageProviders/storageConfig.ts @@ -45,11 +45,11 @@ export interface S3StorageConfig { export interface RedisConfig { delete: { batchSize: number; + scanCount: number; }; host: string; port: number; db: number; - scanCount: number; username?: string; password?: string; tlsEnabled?: boolean; @@ -125,12 +125,12 @@ export function buildRedisStorageConfig(config: ConfigType, logger: Logger): Red const redisConfig = config.get('storage.redis') as unknown as RedisConfig; const { - delete: { batchSize: deleteBatchSize }, + delete: { batchSize: deleteBatchSize, scanCount }, ...redisStorageConfig } = redisConfig; if (deleteBatchSize <= 0) throw new ConfigurationError('Deletion batch size must be greater than 0'); - if (redisStorageConfig.scanCount <= 0) throw new ConfigurationError('Redis scan count must be greater than 0'); + if (scanCount <= 0) throw new ConfigurationError('Redis scan count must be greater than 0'); // Logged field by field rather than spread, so credentials never reach the logs. logger.info({ @@ -138,8 +138,8 @@ export function buildRedisStorageConfig(config: ConfigType, logger: Logger): Red host: redisStorageConfig.host, port: redisStorageConfig.port, db: redisStorageConfig.db, - scanCount: redisStorageConfig.scanCount, + scanCount, batchSize: deleteBatchSize, }); - return { ...redisStorageConfig, batchSize: deleteBatchSize }; + return { ...redisStorageConfig, scanCount, batchSize: deleteBatchSize }; } diff --git a/tests/helpers/mocks.ts b/tests/helpers/mocks.ts index 5d3e13c..c91321f 100644 --- a/tests/helpers/mocks.ts +++ b/tests/helpers/mocks.ts @@ -182,11 +182,11 @@ export function createFsStorageConfig(overrides: Partial = {}): export const REDIS_STORAGE_CONFIG_DEFAULTS = { delete: { batchSize: 3, + scanCount: 10, }, host: 'localhost', port: 6379, db: 0, - scanCount: 10, } as const satisfies RedisConfig; export function createMockRedisConfig(overrides: Record = {}): ConfigType { @@ -201,7 +201,7 @@ export const REDIS_VALIDATED_CONFIG_DEFAULTS = { host: REDIS_STORAGE_CONFIG_DEFAULTS.host, port: REDIS_STORAGE_CONFIG_DEFAULTS.port, db: REDIS_STORAGE_CONFIG_DEFAULTS.db, - scanCount: REDIS_STORAGE_CONFIG_DEFAULTS.scanCount, + scanCount: REDIS_STORAGE_CONFIG_DEFAULTS.delete.scanCount, batchSize: REDIS_STORAGE_CONFIG_DEFAULTS.delete.batchSize, } as const satisfies RedisStorageConfig; diff --git a/tests/storageProviders/storageConfig.spec.ts b/tests/storageProviders/storageConfig.spec.ts index 3eff463..cada7c2 100644 --- a/tests/storageProviders/storageConfig.spec.ts +++ b/tests/storageProviders/storageConfig.spec.ts @@ -14,6 +14,7 @@ import { createRedisStorageConfig, createS3StorageConfig, FS_STORAGE_CONFIG_DEFAULTS, + REDIS_STORAGE_CONFIG_DEFAULTS, } from '../helpers/mocks'; vi.mock('@src/cleaner/utils/fs', () => ({ @@ -154,13 +155,17 @@ describe('storageConfig', () => { }); it('should throw ConfigurationError when batchSize is less than or equal to 0', () => { - const config = createMockRedisConfig({ delete: { batchSize: faker.number.int({ max: 0, min: -Number.MAX_SAFE_INTEGER }) } }); + const config = createMockRedisConfig({ + delete: { ...REDIS_STORAGE_CONFIG_DEFAULTS.delete, batchSize: faker.number.int({ max: 0, min: -Number.MAX_SAFE_INTEGER }) }, + }); expect(() => buildRedisStorageConfig(config, mockLogger)).toThrow(ConfigurationError); }); it('should throw ConfigurationError when scanCount is less than or equal to 0', () => { - const config = createMockRedisConfig({ scanCount: faker.number.int({ max: 0, min: -Number.MAX_SAFE_INTEGER }) }); + const config = createMockRedisConfig({ + delete: { ...REDIS_STORAGE_CONFIG_DEFAULTS.delete, scanCount: faker.number.int({ max: 0, min: -Number.MAX_SAFE_INTEGER }) }, + }); expect(() => buildRedisStorageConfig(config, mockLogger)).toThrow(ConfigurationError); }); From ca8a303003f68db90f1ebe1a0606e0606ece6607 Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 10:52:14 +0300 Subject: [PATCH 17/27] refactor(clients): extract clients from storageProviders --- src/cleaner/clients/index.ts | 2 ++ .../redisClient.ts | 2 +- src/cleaner/clients/s3Client.ts | 19 +++++++++++ src/cleaner/storageProviders/index.ts | 1 - .../storageProviders/s3StorageProvider.ts | 16 ++------- src/common/constants.ts | 1 + src/containerConfig.ts | 7 ++-- .../redisClient.spec.ts | 2 +- tests/clients/s3Client.spec.ts | 29 ++++++++++++++++ tests/integration/helpers/backendFixtures.ts | 10 ++++-- .../s3StorageProvider.spec.ts | 33 ++++--------------- 11 files changed, 74 insertions(+), 48 deletions(-) create mode 100644 src/cleaner/clients/index.ts rename src/cleaner/{storageProviders => clients}/redisClient.ts (92%) create mode 100644 src/cleaner/clients/s3Client.ts rename tests/{storageProviders => clients}/redisClient.spec.ts (97%) create mode 100644 tests/clients/s3Client.spec.ts diff --git a/src/cleaner/clients/index.ts b/src/cleaner/clients/index.ts new file mode 100644 index 0000000..9f80cd8 --- /dev/null +++ b/src/cleaner/clients/index.ts @@ -0,0 +1,2 @@ +export { createRedisConnection } from './redisClient'; +export { createS3Client } from './s3Client'; diff --git a/src/cleaner/storageProviders/redisClient.ts b/src/cleaner/clients/redisClient.ts similarity index 92% rename from src/cleaner/storageProviders/redisClient.ts rename to src/cleaner/clients/redisClient.ts index 62e0e75..ef4d4e0 100644 --- a/src/cleaner/storageProviders/redisClient.ts +++ b/src/cleaner/clients/redisClient.ts @@ -1,7 +1,7 @@ import type { Logger } from '@map-colonies/js-logger'; // eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class import Redis from 'ioredis'; -import type { RedisStorageConfig } from './storageConfig'; +import type { RedisStorageConfig } from '../storageProviders/storageConfig'; /** * Connects to a standalone Redis. diff --git a/src/cleaner/clients/s3Client.ts b/src/cleaner/clients/s3Client.ts new file mode 100644 index 0000000..5258471 --- /dev/null +++ b/src/cleaner/clients/s3Client.ts @@ -0,0 +1,19 @@ +import { S3Client } from '@aws-sdk/client-s3'; +import type { S3StorageConfig } from '../storageProviders/storageConfig'; + +/** + * Builds the S3 client shared by every resolution of the S3 storage provider. + * Its sockets are released by the `onSignal` shutdown hook. + */ +export function createS3Client(config: S3StorageConfig): S3Client { + return new S3Client({ + endpoint: config.endpoint, + credentials: { + accessKeyId: config.accessKeyId, + secretAccessKey: config.secretAccessKey, + }, + forcePathStyle: config.forcePathStyle, + region: config.region, + tls: config.sslEnabled, + }); +} diff --git a/src/cleaner/storageProviders/index.ts b/src/cleaner/storageProviders/index.ts index d5bca72..d23f357 100644 --- a/src/cleaner/storageProviders/index.ts +++ b/src/cleaner/storageProviders/index.ts @@ -9,7 +9,6 @@ export type { StorageProviders, StorageTarget, } from './iStorageProvider'; -export { createRedisConnection } from './redisClient'; export { RedisStorageProvider } from './redisStorageProvider'; export { S3StorageProvider } from './s3StorageProvider'; export { diff --git a/src/cleaner/storageProviders/s3StorageProvider.ts b/src/cleaner/storageProviders/s3StorageProvider.ts index 56db343..88f74ec 100644 --- a/src/cleaner/storageProviders/s3StorageProvider.ts +++ b/src/cleaner/storageProviders/s3StorageProvider.ts @@ -7,8 +7,8 @@ import { NoSuchBucket, NotFound, paginateListObjectsV2, - S3Client, S3ServiceException, + type S3Client, type _Error, type _Object, } from '@aws-sdk/client-s3'; @@ -32,23 +32,11 @@ type S3StorageProviderType = Extract; @injectable() export class S3StorageProvider implements IStorageProvider { - private readonly s3Client: S3Client; - public constructor( @inject(SERVICES.S3_STORAGE_CONFIG) private readonly s3Config: S3StorageConfig, + @inject(SERVICES.S3_CLIENT) private readonly s3Client: S3Client, @inject(SERVICES.LOGGER) private readonly logger: Logger ) { - // TODO: move client to a singleton resolution since - this.s3Client = new S3Client({ - endpoint: s3Config.endpoint, - credentials: { - accessKeyId: s3Config.accessKeyId, - secretAccessKey: s3Config.secretAccessKey, - }, - forcePathStyle: s3Config.forcePathStyle, - region: s3Config.region, - tls: s3Config.sslEnabled, - }); this.logger.debug({ msg: 'Loaded S3 storage provider', endpoint: s3Config.endpoint, batchSize: this.s3Config.batchSize }); } diff --git a/src/common/constants.ts b/src/common/constants.ts index f476849..dc12f0b 100644 --- a/src/common/constants.ts +++ b/src/common/constants.ts @@ -21,6 +21,7 @@ export const SERVICES = { STORAGE_PROVIDERS: Symbol('StorageProviders'), FS_STORAGE_CONFIG: Symbol('FsStorageConfig'), S3_STORAGE_CONFIG: Symbol('S3StorageConfig'), + S3_CLIENT: Symbol('S3Client'), REDIS_STORAGE_CONFIG: Symbol('RedisStorageConfig'), REDIS_CONNECTION: Symbol('RedisConnection'), TASK_CONTEXT: Symbol('TaskContext'), diff --git a/src/containerConfig.ts b/src/containerConfig.ts index 41bd3dc..1f372bf 100644 --- a/src/containerConfig.ts +++ b/src/containerConfig.ts @@ -11,13 +11,13 @@ import { SERVICE_NAME, SERVICES } from '@common/constants'; import { getJobAndTaskToken, InjectionObject, registerDependencies } from '@common/dependencyRegistration'; import { getTracing } from '@common/tracing'; import type { StorageProviders } from '@src/cleaner/storageProviders'; +import { createRedisConnection, createS3Client } from './cleaner/clients'; import { ErrorHandler } from './cleaner/errors'; import { JobTrackerClient } from './cleaner/httpClients'; import { buildFsStorageConfig, buildRedisStorageConfig, buildS3StorageConfig, - createRedisConnection, FsStorageProvider, RedisStorageProvider, S3StorageProvider, @@ -48,6 +48,7 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise const fsStorageConfig = cleanupStorageProviders.includes(SourceType.FS) ? buildFsStorageConfig(configInstance, logger) : undefined; const s3StorageConfig = cleanupStorageProviders.includes(SourceType.S3) ? buildS3StorageConfig(configInstance, logger) : undefined; const redisStorageConfig = cleanupStorageProviders.includes(StorageProvider.REDIS) ? buildRedisStorageConfig(configInstance, logger) : undefined; + const s3Client = s3StorageConfig ? createS3Client(s3StorageConfig) : undefined; const redisConnection = redisStorageConfig ? await createRedisConnection(redisStorageConfig, logger) : undefined; const dependencies: InjectionObject[] = [ @@ -115,6 +116,7 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise }, ...(fsStorageConfig ? [{ token: SERVICES.FS_STORAGE_CONFIG, provider: { useValue: fsStorageConfig } }] : []), ...(s3StorageConfig ? [{ token: SERVICES.S3_STORAGE_CONFIG, provider: { useValue: s3StorageConfig } }] : []), + ...(s3Client ? [{ token: SERVICES.S3_CLIENT, provider: { useValue: s3Client } }] : []), ...(redisStorageConfig ? [{ token: SERVICES.REDIS_STORAGE_CONFIG, provider: { useValue: redisStorageConfig } }] : []), ...(redisConnection ? [{ token: SERVICES.REDIS_CONNECTION, provider: { useValue: redisConnection } }] : []), { @@ -122,7 +124,7 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise provider: { useFactory: instancePerContainerCachingFactory((container) => { const providers = { - ...(s3StorageConfig && { [SourceType.S3]: container.resolve(S3StorageProvider) }), + ...(s3Client && { [SourceType.S3]: container.resolve(S3StorageProvider) }), ...(fsStorageConfig && { [SourceType.FS]: container.resolve(FsStorageProvider) }), ...(redisConnection && { [StorageProvider.REDIS]: container.resolve(RedisStorageProvider) }), }; @@ -200,6 +202,7 @@ export const registerExternalValues = async (options?: RegisterOptions): Promise const worker = container.resolve(SERVICES.WORKER); return async (): Promise => { await Promise.all([getTracing().stop(), worker.stop(), redisConnection?.quit()]); + s3Client?.destroy(); }; }, }, diff --git a/tests/storageProviders/redisClient.spec.ts b/tests/clients/redisClient.spec.ts similarity index 97% rename from tests/storageProviders/redisClient.spec.ts rename to tests/clients/redisClient.spec.ts index a3f26e4..02a89f2 100644 --- a/tests/storageProviders/redisClient.spec.ts +++ b/tests/clients/redisClient.spec.ts @@ -1,6 +1,6 @@ import type { Logger } from '@map-colonies/js-logger'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { createRedisConnection } from '@src/cleaner/storageProviders/redisClient'; +import { createRedisConnection } from '@src/cleaner/clients/redisClient'; import { createMockLogger, createRedisStorageConfig } from '../helpers/mocks'; const { redisConstructor, connect, quit, scan, unlink } = vi.hoisted(() => ({ diff --git a/tests/clients/s3Client.spec.ts b/tests/clients/s3Client.spec.ts new file mode 100644 index 0000000..b4c522a --- /dev/null +++ b/tests/clients/s3Client.spec.ts @@ -0,0 +1,29 @@ +/* eslint-disable @typescript-eslint/naming-convention */ +import { S3Client } from '@aws-sdk/client-s3'; +import { describe, expect, it, vi } from 'vitest'; +import { createS3Client } from '@src/cleaner/clients/s3Client'; +import { createS3StorageConfig, S3_VALIDATED_CONFIG_DEFAULTS } from '../helpers/mocks'; + +vi.mock(import('@aws-sdk/client-s3'), async (importOriginal) => { + const originModule = await importOriginal(); + return { ...originModule, S3Client: vi.fn() as unknown as typeof S3Client }; +}); + +describe('createS3Client', () => { + it('should construct S3Client with config values', () => { + createS3Client(createS3StorageConfig()); + + expect(S3Client).toHaveBeenCalledWith( + expect.objectContaining({ + credentials: { + accessKeyId: S3_VALIDATED_CONFIG_DEFAULTS.accessKeyId, + secretAccessKey: S3_VALIDATED_CONFIG_DEFAULTS.secretAccessKey, + }, + endpoint: S3_VALIDATED_CONFIG_DEFAULTS.endpoint, + forcePathStyle: S3_VALIDATED_CONFIG_DEFAULTS.forcePathStyle, + region: S3_VALIDATED_CONFIG_DEFAULTS.region, + tls: S3_VALIDATED_CONFIG_DEFAULTS.sslEnabled, + }) + ); + }); +}); diff --git a/tests/integration/helpers/backendFixtures.ts b/tests/integration/helpers/backendFixtures.ts index 9e77f13..cafe5e5 100644 --- a/tests/integration/helpers/backendFixtures.ts +++ b/tests/integration/helpers/backendFixtures.ts @@ -5,8 +5,8 @@ import { StorageProvider } from '@map-colonies/raster-shared'; import type { S3Client } from '@aws-sdk/client-s3'; // eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class import type Redis from 'ioredis'; +import { createRedisConnection } from '@src/cleaner/clients'; import { - createRedisConnection, FsStorageProvider, RedisStorageProvider, S3StorageProvider, @@ -83,7 +83,7 @@ interface ProviderBatchSizes { } function buildProviders( - { minio, redis, redisConnection }: BackendHandles, + { minio, s3Client, redis, redisConnection }: BackendHandles, fsBasePath: string, batchSizes: ProviderBatchSizes = {} ): StorageProviders { @@ -95,7 +95,11 @@ function buildProviders( const s3StorageConfig = buildS3StorageConfigForMinio(minio); return { - [StorageProvider.S3]: new S3StorageProvider({ ...s3StorageConfig, batchSize: batchSizes.s3 ?? s3StorageConfig.batchSize }, createMockLogger()), + [StorageProvider.S3]: new S3StorageProvider( + { ...s3StorageConfig, batchSize: batchSizes.s3 ?? s3StorageConfig.batchSize }, + s3Client, + createMockLogger() + ), [StorageProvider.FS]: new FsStorageProvider(fsStorageConfig, createMockLogger()), [StorageProvider.REDIS]: new RedisStorageProvider(buildRedisStorageConfig(redis, batchSizes.redis), redisConnection, createMockLogger()), }; diff --git a/tests/storageProviders/s3StorageProvider.spec.ts b/tests/storageProviders/s3StorageProvider.spec.ts index 2082241..c96adf8 100644 --- a/tests/storageProviders/s3StorageProvider.spec.ts +++ b/tests/storageProviders/s3StorageProvider.spec.ts @@ -5,8 +5,8 @@ import { NoSuchBucket, NotFound, paginateListObjectsV2, - S3Client, S3ServiceException, + type S3Client, type DeleteObjectsCommandOutput, } from '@aws-sdk/client-s3'; import { faker } from '@faker-js/faker'; @@ -14,7 +14,7 @@ import type { Logger } from '@map-colonies/js-logger'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { UnrecoverableError } from '@src/cleaner/errors'; import { S3StorageProvider } from '@src/cleaner/storageProviders/s3StorageProvider'; -import { createMockLogger, createS3StorageConfig, S3_VALIDATED_CONFIG_DEFAULTS } from '../helpers/mocks'; +import { createMockLogger, createS3StorageConfig } from '../helpers/mocks'; const mockSend = vi.fn(); const mockPaginateListObjectsV2Next = vi.fn(); @@ -33,7 +33,6 @@ vi.mock(import('@aws-sdk/client-s3'), async (importOriginal) => { const originModule = await importOriginal(); return { ...originModule, - S3Client: vi.fn(() => ({ send: mockSend })) as unknown as typeof S3Client, DeleteObjectsCommand: vi.fn((input: unknown) => input) as unknown as typeof DeleteObjectsCommand, ListObjectsV2Command: vi.fn((input: unknown) => input) as unknown as typeof ListObjectsV2Command, paginateListObjectsV2: vi.fn(() => mockPaginateListObjectsV2) as unknown as typeof paginateListObjectsV2, @@ -41,6 +40,7 @@ vi.mock(import('@aws-sdk/client-s3'), async (importOriginal) => { }); const BUCKET = 'test-bucket'; +const mockS3Client = { send: mockSend } as unknown as S3Client; describe('S3StorageProvider', () => { let provider: S3StorageProvider; @@ -49,26 +49,7 @@ describe('S3StorageProvider', () => { beforeEach(() => { vi.clearAllMocks(); mockLogger = createMockLogger(); - provider = new S3StorageProvider(createS3StorageConfig(), mockLogger); - }); - - describe('#constructor', () => { - it('should construct S3Client with config values', () => { - const provider = new S3StorageProvider(createS3StorageConfig(), mockLogger); - expect(S3Client).toHaveBeenCalledWith( - expect.objectContaining({ - credentials: { - accessKeyId: S3_VALIDATED_CONFIG_DEFAULTS.accessKeyId, - secretAccessKey: S3_VALIDATED_CONFIG_DEFAULTS.secretAccessKey, - }, - endpoint: S3_VALIDATED_CONFIG_DEFAULTS.endpoint, - forcePathStyle: S3_VALIDATED_CONFIG_DEFAULTS.forcePathStyle, - region: S3_VALIDATED_CONFIG_DEFAULTS.region, - tls: S3_VALIDATED_CONFIG_DEFAULTS.sslEnabled, - }) - ); - expect(provider).toBeInstanceOf(S3StorageProvider); - }); + provider = new S3StorageProvider(createS3StorageConfig(), mockS3Client, mockLogger); }); describe('#delete', () => { @@ -168,7 +149,7 @@ describe('S3StorageProvider', () => { it('should batch paths into chunks of the configured batch size', async () => { const paths = Array.from({ length: 1500 }, (_, i) => `object-${i}.txt`); - provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 1000 }), mockLogger); + provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 1000 }), mockS3Client, mockLogger); await provider.delete(BUCKET, paths); @@ -185,7 +166,7 @@ describe('S3StorageProvider', () => { it('should never exceed the S3 max keys limit per request for the maximum allowed batch size', async () => { const paths = Array.from({ length: 2500 }, (_, i) => `object-${i}.txt`); - provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 1000 }), mockLogger); + provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 1000 }), mockS3Client, mockLogger); await provider.delete(BUCKET, paths); @@ -594,7 +575,7 @@ describe('S3StorageProvider', () => { }); it('should request pages sized by the configured batch size', async () => { - provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 500 }), mockLogger); + provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 500 }), mockS3Client, mockLogger); mockSend .mockResolvedValueOnce(undefined) // bucket exists .mockRejectedValueOnce(new NotFound({ $metadata: {}, message: 'not found' })); // no listing found for single object From 95d888479999caf968c84f1b31de1bc5a937cd78 Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 11:13:04 +0300 Subject: [PATCH 18/27] test(redis): assert the connected client is returned instead of exercising the mock --- tests/clients/redisClient.spec.ts | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/tests/clients/redisClient.spec.ts b/tests/clients/redisClient.spec.ts index 02a89f2..335a64d 100644 --- a/tests/clients/redisClient.spec.ts +++ b/tests/clients/redisClient.spec.ts @@ -1,22 +1,20 @@ import type { Logger } from '@map-colonies/js-logger'; +// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class +import Redis from 'ioredis'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { createRedisConnection } from '@src/cleaner/clients/redisClient'; import { createMockLogger, createRedisStorageConfig } from '../helpers/mocks'; -const { redisConstructor, connect, quit, scan, unlink } = vi.hoisted(() => ({ +const { redisConstructor, connect, quit } = vi.hoisted(() => ({ redisConstructor: vi.fn(), connect: vi.fn(), quit: vi.fn(), - scan: vi.fn(), - unlink: vi.fn(), })); vi.mock('ioredis', () => ({ default: class { public connect = connect; public quit = quit; - public scan = scan; - public unlink = unlink; public constructor(options: unknown) { redisConstructor(options); } @@ -34,17 +32,11 @@ describe('createRedisConnection', () => { }); describe('connecting', () => { - it('should return the connected client itself, not a wrapper', async () => { - scan.mockResolvedValue(['0', []]); - unlink.mockResolvedValue(1); - + it('should connect once at startup and return the connected Redis client', async () => { const client = await createRedisConnection(createRedisStorageConfig(), mockLogger); - await client.scan('0', 'MATCH', 'p-*', 'COUNT', 10); - await client.unlink('k'); expect(connect).toHaveBeenCalledTimes(1); - expect(scan).toHaveBeenCalledWith('0', 'MATCH', 'p-*', 'COUNT', 10); - expect(unlink).toHaveBeenCalledWith('k'); + expect(client).toBeInstanceOf(Redis); }); it('should build the client lazily with the configured connection details', async () => { From bce7b2d2206d448c71b5df0e47baf6e239253c44 Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 11:13:06 +0300 Subject: [PATCH 19/27] test(integration): run S3 tests against seaweedfs 4.47 instead of minio --- tests/integration/helpers/minioContainer.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/integration/helpers/minioContainer.ts b/tests/integration/helpers/minioContainer.ts index adfe3b7..5fa0827 100644 --- a/tests/integration/helpers/minioContainer.ts +++ b/tests/integration/helpers/minioContainer.ts @@ -9,11 +9,11 @@ interface MinioHandle { } /** - * Pinned to the release running on our Azure deployment, so the suite exercises the same server - * behavior we deploy against. Pulled from Quay: Docker Hub no longer serves `minio/minio` + * SeaweedFS's S3 gateway, pinned to the image our infra deploys, so the suite exercises the same + * server behavior. MinIO no longer serves its images anonymously on Docker Hub or Quay. */ -const MINIO_IMAGE = 'quay.io/minio/minio:RELEASE.2025-07-23T15-54-02Z'; -const MINIO_PORT = 9000; +const MINIO_IMAGE = 'docker.io/chrislusf/seaweedfs:4.47'; +const MINIO_PORT = 8333; const DEFAULT_USER = 'minioadmin'; const DEFAULT_PASSWORD = 'minioadmin'; @@ -31,10 +31,11 @@ async function startMinio(): Promise { } console.log(`Minio: TEST_MINIO_ENDPOINT is not set, starting testcontainer from ${MINIO_IMAGE}`); const container: StartedTestContainer = await new GenericContainer(MINIO_IMAGE) - .withCommand(['server', '/data']) + .withCommand(['server', '-s3', '-dir=/data']) + // SeaweedFS registers these as the S3 admin identity .withEnvironment({ - MINIO_ROOT_USER: DEFAULT_USER, - MINIO_ROOT_PASSWORD: DEFAULT_PASSWORD, + AWS_ACCESS_KEY_ID: DEFAULT_USER, + AWS_SECRET_ACCESS_KEY: DEFAULT_PASSWORD, }) .withExposedPorts(MINIO_PORT) .start(); From bbd8796b68ef5282f68d86536efa6af951103bc1 Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 11:18:54 +0300 Subject: [PATCH 20/27] test(integration): rename minio test helpers to generic s3 names --- README.md | 22 ++++++------ tests/integration/helpers/backendFixtures.ts | 20 +++++------ .../{minioContainer.ts => s3Container.ts} | 34 +++++++++---------- tests/integration/helpers/s3TestKit.ts | 8 ++--- 4 files changed, 42 insertions(+), 42 deletions(-) rename tests/integration/helpers/{minioContainer.ts => s3Container.ts} (51%) diff --git a/README.md b/README.md index 9923980..8d631a5 100644 --- a/README.md +++ b/README.md @@ -64,23 +64,23 @@ npm run test:watch #### Integration tests `npm run test:integration` needs an S3-compatible server and a Redis server. By default it starts -a Minio and a Redis [testcontainer](https://testcontainers.com/) automatically. +an S3-compatible and a Redis [testcontainer](https://testcontainers.com/) automatically. The images are pinned to the releases deployed in our environments; bump them in -`tests/integration/helpers/minioContainer.ts` and `tests/integration/helpers/redisContainer.ts` +`tests/integration/helpers/s3Container.ts` and `tests/integration/helpers/redisContainer.ts` when those environments move. To run against already-running servers instead, set the `TEST_*` variables: -| Variable | Default | Purpose | -| ------------------------ | ------------ | -------------------------------------------------------------------- | -| `TEST_MINIO_ENDPOINT` | _(unset)_ | Point the suite at an existing Minio. Unset means start a container. | -| `TEST_MINIO_ACCESS_KEY` | `minioadmin` | Access key for that server. | -| `TEST_MINIO_SECRET_KEY` | `minioadmin` | Secret key for that server. | -| `TEST_REDIS_HOST` | _(unset)_ | Point the suite at an existing Redis. Unset means start a container. | -| `TEST_REDIS_PORT` | `6379` | Port for that server. | +| Variable | Default | Purpose | +| -------------------- | ------------ | ------------------------------------------------------------------------ | +| `TEST_S3_ENDPOINT` | _(unset)_ | Point the suite at an existing S3 server. Unset means start a container. | +| `TEST_S3_ACCESS_KEY` | `minioadmin` | Access key for that server. | +| `TEST_S3_SECRET_KEY` | `minioadmin` | Secret key for that server. | +| `TEST_REDIS_HOST` | _(unset)_ | Point the suite at an existing Redis. Unset means start a container. | +| `TEST_REDIS_PORT` | `6379` | Port for that server. | -> **The suite creates and deletes buckets on whichever Minio it is given, and calls `FLUSHDB` on -> whichever Redis it is given.** Never point `TEST_MINIO_ENDPOINT` or `TEST_REDIS_HOST` at a shared +> **The suite creates and deletes buckets on whichever S3 server it is given, and calls `FLUSHDB` on +> whichever Redis it is given.** Never point `TEST_S3_ENDPOINT` or `TEST_REDIS_HOST` at a shared > or deployed environment, and beware of leaving them exported in a shell profile. Each run prints > which mode it selected and against which server. diff --git a/tests/integration/helpers/backendFixtures.ts b/tests/integration/helpers/backendFixtures.ts index cafe5e5..7a3bc58 100644 --- a/tests/integration/helpers/backendFixtures.ts +++ b/tests/integration/helpers/backendFixtures.ts @@ -15,8 +15,8 @@ import { type StorageProviders, } from '@src/cleaner/storageProviders'; import { createMockLogger } from '../../helpers/mocks'; -import { startMinio, type MinioHandle } from './minioContainer'; -import { buildS3StorageConfigForMinio, createTestS3Client, deleteBucket, ensureBucket } from './s3TestKit'; +import { startS3, type S3Handle } from './s3Container'; +import { buildS3StorageConfig, createTestS3Client, deleteBucket, ensureBucket } from './s3TestKit'; import { makeTempFsBase, rmBase } from './fsTestKit'; import { startRedis, type RedisHandle } from './redisContainer'; import { createTestRedisClient, flush } from './redisTestKit'; @@ -32,7 +32,7 @@ const REDIS_SCAN_COUNT = 10; const REDIS_DELETE_BATCH_SIZE = 4; interface BackendHandles { - minio: MinioHandle; + s3: S3Handle; s3Client: S3Client; redis: RedisHandle; /** Seeding and listing client; the provider gets its own connection. */ @@ -55,18 +55,18 @@ function buildRedisStorageConfig(redis: RedisHandle, batchSize = REDIS_DELETE_BA } async function startBackends(): Promise { - const [minio, redis] = await Promise.all([startMinio(), startRedis()]); - const s3Client = createTestS3Client(minio); + const [s3, redis] = await Promise.all([startS3(), startRedis()]); + const s3Client = createTestS3Client(s3); const redisClient = createTestRedisClient(redis); const redisConnection = await createRedisConnection(buildRedisStorageConfig(redis), createMockLogger()); - return { minio, s3Client, redis, redisClient, redisConnection }; + return { s3, s3Client, redis, redisClient, redisConnection }; } -async function stopBackends({ minio, s3Client, redis, redisClient, redisConnection }: BackendHandles): Promise { +async function stopBackends({ s3, s3Client, redis, redisClient, redisConnection }: BackendHandles): Promise { s3Client.destroy(); await redisConnection.quit(); redisClient.disconnect(); - await Promise.all([minio.stop(), redis.stop()]); + await Promise.all([s3.stop(), redis.stop()]); } /** Per-provider delete batch sizes; each falls back to the production-shaped default. */ @@ -83,7 +83,7 @@ interface ProviderBatchSizes { } function buildProviders( - { minio, s3Client, redis, redisConnection }: BackendHandles, + { s3, s3Client, redis, redisConnection }: BackendHandles, fsBasePath: string, batchSizes: ProviderBatchSizes = {} ): StorageProviders { @@ -92,7 +92,7 @@ function buildProviders( subPaths: [FS_ALLOWED_SUB_PATH], batchSize: batchSizes.fs ?? FS_DELETE_BATCH_SIZE, }; - const s3StorageConfig = buildS3StorageConfigForMinio(minio); + const s3StorageConfig = buildS3StorageConfig(s3); return { [StorageProvider.S3]: new S3StorageProvider( diff --git a/tests/integration/helpers/minioContainer.ts b/tests/integration/helpers/s3Container.ts similarity index 51% rename from tests/integration/helpers/minioContainer.ts rename to tests/integration/helpers/s3Container.ts index 5fa0827..e60682d 100644 --- a/tests/integration/helpers/minioContainer.ts +++ b/tests/integration/helpers/s3Container.ts @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/naming-convention */ import { GenericContainer, type StartedTestContainer } from 'testcontainers'; -interface MinioHandle { +interface S3Handle { endpoint: string; accessKeyId: string; secretAccessKey: string; @@ -9,39 +9,39 @@ interface MinioHandle { } /** - * SeaweedFS's S3 gateway, pinned to the image our infra deploys, so the suite exercises the same - * server behavior. MinIO no longer serves its images anonymously on Docker Hub or Quay. + * Any S3-compatible server works here. Pinned to the image our infra deploys (currently SeaweedFS's + * S3 gateway), so the suite exercises the same server behavior. */ -const MINIO_IMAGE = 'docker.io/chrislusf/seaweedfs:4.47'; -const MINIO_PORT = 8333; +const S3_IMAGE = 'docker.io/chrislusf/seaweedfs:4.47'; +const S3_PORT = 8333; const DEFAULT_USER = 'minioadmin'; const DEFAULT_PASSWORD = 'minioadmin'; -async function startMinio(): Promise { - const externalEndpoint = process.env.TEST_MINIO_ENDPOINT; +async function startS3(): Promise { + const externalEndpoint = process.env.TEST_S3_ENDPOINT; if (externalEndpoint !== undefined && externalEndpoint !== '') { - console.warn(`Minio: using EXTERNAL server at ${externalEndpoint} (TEST_MINIO_ENDPOINT is set).`); - console.warn('Minio: this suite CREATES AND DELETES buckets on that server. Unset TEST_MINIO_ENDPOINT to use a testcontainer.'); + console.warn(`S3: using EXTERNAL server at ${externalEndpoint} (TEST_S3_ENDPOINT is set).`); + console.warn('S3: this suite CREATES AND DELETES buckets on that server. Unset TEST_S3_ENDPOINT to use a testcontainer.'); return { endpoint: externalEndpoint, - accessKeyId: process.env.TEST_MINIO_ACCESS_KEY ?? DEFAULT_USER, - secretAccessKey: process.env.TEST_MINIO_SECRET_KEY ?? DEFAULT_PASSWORD, + accessKeyId: process.env.TEST_S3_ACCESS_KEY ?? DEFAULT_USER, + secretAccessKey: process.env.TEST_S3_SECRET_KEY ?? DEFAULT_PASSWORD, stop: async (): Promise => Promise.resolve(), }; } - console.log(`Minio: TEST_MINIO_ENDPOINT is not set, starting testcontainer from ${MINIO_IMAGE}`); - const container: StartedTestContainer = await new GenericContainer(MINIO_IMAGE) + console.log(`S3: TEST_S3_ENDPOINT is not set, starting testcontainer from ${S3_IMAGE}`); + const container: StartedTestContainer = await new GenericContainer(S3_IMAGE) .withCommand(['server', '-s3', '-dir=/data']) // SeaweedFS registers these as the S3 admin identity .withEnvironment({ AWS_ACCESS_KEY_ID: DEFAULT_USER, AWS_SECRET_ACCESS_KEY: DEFAULT_PASSWORD, }) - .withExposedPorts(MINIO_PORT) + .withExposedPorts(S3_PORT) .start(); return { - endpoint: `http://${container.getHost()}:${container.getMappedPort(MINIO_PORT)}`, + endpoint: `http://${container.getHost()}:${container.getMappedPort(S3_PORT)}`, accessKeyId: DEFAULT_USER, secretAccessKey: DEFAULT_PASSWORD, stop: async (): Promise => { @@ -49,10 +49,10 @@ async function startMinio(): Promise { await container.stop(); } catch (error) { // Non-fatal: Ryuk reaps the container on test process exit, unless it is disabled. - console.warn('Failed to stop Minio container; relying on Ryuk to reap it', error); + console.warn('Failed to stop S3 container; relying on Ryuk to reap it', error); } }, }; } -export { startMinio, type MinioHandle }; +export { startS3, type S3Handle }; diff --git a/tests/integration/helpers/s3TestKit.ts b/tests/integration/helpers/s3TestKit.ts index 2fb72fe..9ee77fa 100644 --- a/tests/integration/helpers/s3TestKit.ts +++ b/tests/integration/helpers/s3TestKit.ts @@ -10,7 +10,7 @@ import { BucketAlreadyExists, } from '@aws-sdk/client-s3'; import type { S3StorageConfig } from '@src/cleaner/storageProviders'; -import type { MinioHandle } from './minioContainer'; +import type { S3Handle } from './s3Container'; import { TINY_TILE_BODY } from './tileFixtures'; // S3/MinIO reject a DeleteObjects request carrying more than 1000 keys. Only bucket @@ -18,7 +18,7 @@ import { TINY_TILE_BODY } from './tileFixtures'; const S3_DELETE_OBJECTS_MAX_KEYS = 1000; const TEST_REGION = 'us-east-1'; -function createTestS3Client(handle: MinioHandle): S3Client { +function createTestS3Client(handle: S3Handle): S3Client { return new S3Client({ endpoint: handle.endpoint, credentials: { @@ -36,7 +36,7 @@ function createTestS3Client(handle: MinioHandle): S3Client { * built here straight from the container handle, so the test skips `buildS3StorageConfig` * (and with it the `storage.s3` config lookup) but exercises the real provider. */ -function buildS3StorageConfigForMinio(handle: MinioHandle): S3StorageConfig { +function buildS3StorageConfig(handle: S3Handle): S3StorageConfig { return { endpoint: handle.endpoint, accessKeyId: handle.accessKeyId, @@ -103,4 +103,4 @@ async function putManyTiles(client: S3Client, bucket: string, keys: string[]): P } } -export { createTestS3Client, buildS3StorageConfigForMinio, ensureBucket, deleteBucket, emptyBucket, putTile, putManyTiles, listAllKeys }; +export { createTestS3Client, buildS3StorageConfig, ensureBucket, deleteBucket, emptyBucket, putTile, putManyTiles, listAllKeys }; From b36ac48dd4e2aea30d35eed321158f051610e224 Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 14:34:30 +0300 Subject: [PATCH 21/27] test(redis): drop the quit test that only exercised the mock --- tests/clients/redisClient.spec.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/clients/redisClient.spec.ts b/tests/clients/redisClient.spec.ts index 335a64d..f1d42d5 100644 --- a/tests/clients/redisClient.spec.ts +++ b/tests/clients/redisClient.spec.ts @@ -56,13 +56,6 @@ describe('createRedisConnection', () => { expect(redisConstructor).toHaveBeenCalledWith(expect.objectContaining({ tls: {} })); }); - - it('should hand back a client the shutdown hook can quit', async () => { - const client = await createRedisConnection(createRedisStorageConfig(), mockLogger); - await client.quit(); - - expect(quit).toHaveBeenCalledTimes(1); - }); }); describe('connection failure', () => { From 1b62bacab7f83e8e4d793c77a85f2ab8f32b144c Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 14:35:09 +0300 Subject: [PATCH 22/27] test(redis): drop the always-passing quit check on connect failure --- tests/clients/redisClient.spec.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/tests/clients/redisClient.spec.ts b/tests/clients/redisClient.spec.ts index f1d42d5..6b7476a 100644 --- a/tests/clients/redisClient.spec.ts +++ b/tests/clients/redisClient.spec.ts @@ -5,16 +5,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { createRedisConnection } from '@src/cleaner/clients/redisClient'; import { createMockLogger, createRedisStorageConfig } from '../helpers/mocks'; -const { redisConstructor, connect, quit } = vi.hoisted(() => ({ +const { redisConstructor, connect } = vi.hoisted(() => ({ redisConstructor: vi.fn(), connect: vi.fn(), - quit: vi.fn(), })); vi.mock('ioredis', () => ({ default: class { public connect = connect; - public quit = quit; public constructor(options: unknown) { redisConstructor(options); } @@ -28,7 +26,6 @@ describe('createRedisConnection', () => { vi.clearAllMocks(); mockLogger = createMockLogger(); connect.mockResolvedValue(undefined); - quit.mockResolvedValue('OK'); }); describe('connecting', () => { @@ -64,13 +61,5 @@ describe('createRedisConnection', () => { await expect(createRedisConnection(createRedisStorageConfig(), mockLogger)).rejects.toThrow('ECONNREFUSED'); }); - - it('should not hand back a connection when connect failed', async () => { - connect.mockRejectedValue(new Error('ECONNREFUSED')); - - await expect(createRedisConnection(createRedisStorageConfig(), mockLogger)).rejects.toThrow(); - - expect(quit).not.toHaveBeenCalled(); - }); }); }); From c6b798761fe0706d0512f4439f6b8e740de6851d Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 16:03:47 +0300 Subject: [PATCH 23/27] feat(redis): log found and deleted counts for deletions and total tiles per task (MAPCO-11263) --- .../storageProviders/redisStorageProvider.ts | 25 ++++++++++++++++--- .../strategies/tilesDeletionStrategy.ts | 2 +- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/cleaner/storageProviders/redisStorageProvider.ts b/src/cleaner/storageProviders/redisStorageProvider.ts index f021815..5c20dee 100644 --- a/src/cleaner/storageProviders/redisStorageProvider.ts +++ b/src/cleaner/storageProviders/redisStorageProvider.ts @@ -6,7 +6,7 @@ import { inject, injectable } from 'tsyringe'; import { SERVICES } from '@common/constants'; import { getChunk } from '@src/cleaner/utils'; import { describeError } from '../errors'; -import { mergeFailures } from './failuresHandling'; +import { countFailures, mergeFailures } from './failuresHandling'; import type { DeleteFailure, DeleteResult, IStorageProvider, StorageProvider } from './iStorageProvider'; import type { RedisStorageConfig } from './storageConfig'; @@ -32,8 +32,15 @@ export class RedisStorageProvider implements IStorageProvider): Promise { @@ -42,17 +49,27 @@ export class RedisStorageProvider implements IStorageProvider return; } - this.logger.info({ msg: 'Tiles deletion completed successfully', deletedCount }); + this.logger.info({ msg: 'Tiles deletion completed successfully', totalTiles, deletedCount }); } private resolveStorageProvider(params: TilesDeletionParams): StorageTarget & { provider: ResolvedStorageProvider } { From d373fc7b7bfe9a4ee8de191724c233d8fdf2dc4a Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 16:05:37 +0300 Subject: [PATCH 24/27] feat(redis): enhance logging for Redis prefix wipe with detailed key counts --- src/cleaner/storageProviders/redisStorageProvider.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/cleaner/storageProviders/redisStorageProvider.ts b/src/cleaner/storageProviders/redisStorageProvider.ts index 5c20dee..d1776da 100644 --- a/src/cleaner/storageProviders/redisStorageProvider.ts +++ b/src/cleaner/storageProviders/redisStorageProvider.ts @@ -63,10 +63,8 @@ export class RedisStorageProvider implements IStorageProvider Date: Sun, 4 Oct 2026 16:12:53 +0300 Subject: [PATCH 25/27] test(redis): move the Redis client mocks into the shared test mocks --- tests/helpers/mocks.ts | 45 +++++++++++- .../redisStorageProvider.spec.ts | 70 +++++-------------- 2 files changed, 62 insertions(+), 53 deletions(-) diff --git a/tests/helpers/mocks.ts b/tests/helpers/mocks.ts index c91321f..8395a57 100644 --- a/tests/helpers/mocks.ts +++ b/tests/helpers/mocks.ts @@ -1,6 +1,8 @@ import type { Logger } from '@map-colonies/js-logger'; import type { TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; -import { vi } from 'vitest'; +// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class +import type Redis from 'ioredis'; +import { vi, type Mock } from 'vitest'; import type { IStorageProvider, StorageProvider } from '@src/cleaner/storageProviders/iStorageProvider'; import type { FsConfig, @@ -209,6 +211,47 @@ export function createRedisStorageConfig(overrides: Partial return { ...REDIS_VALIDATED_CONFIG_DEFAULTS, ...overrides }; } +// ─── Redis Client (RedisStorageProvider) ───────────────────────────────────── + +/** + * A mock Redis client recording every key it was asked to unlink. Cast at the boundary with `asRedis`; + * RedisStorageProvider only ever touches `scan` and `unlink`. + */ +export interface MockRedisClient { + unlinked: string[]; + scan: Mock; + unlink: Mock; +} + +export function createMockRedisClient(): MockRedisClient { + const unlinked: string[] = []; + return { + unlinked, + scan: vi.fn().mockResolvedValue(['0', []]), + unlink: vi.fn().mockImplementation(async (...keys: string[]) => { + unlinked.push(...keys); + return Promise.resolve(keys.length); + }), + }; +} + +export function asRedis(client: MockRedisClient): Redis { + return client as unknown as Redis; +} + +/** A client whose SCAN walks `pages` in order, returning to cursor '0' only on the last one. */ +export function createMockScanningRedisClient(pages: string[][]): MockRedisClient { + const client = createMockRedisClient(); + let call = 0; + client.scan = vi.fn().mockImplementation(async () => { + const page = pages[call] ?? []; + call += 1; + const cursor = call >= pages.length ? '0' : String(call); + return Promise.resolve([cursor, page] as [string, string[]]); + }); + return client; +} + // ─── JobTrackerClient ───────────────────────────────────────────────────────── export function createMockJobTrackerClient(): JobTrackerClient { diff --git a/tests/storageProviders/redisStorageProvider.spec.ts b/tests/storageProviders/redisStorageProvider.spec.ts index 16772ed..4fcf4aa 100644 --- a/tests/storageProviders/redisStorageProvider.spec.ts +++ b/tests/storageProviders/redisStorageProvider.spec.ts @@ -1,55 +1,21 @@ -// eslint-disable-next-line @typescript-eslint/naming-convention -- ioredis' default export is a class -import type Redis from 'ioredis'; -import { beforeEach, describe, expect, it, vi, type Mock } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { RedisStorageProvider } from '@src/cleaner/storageProviders/redisStorageProvider'; -import { createMockLogger, createRedisStorageConfig } from '../helpers/mocks'; - -/** - * A fake Redis recording every key it was asked to unlink. Cast at the boundary like every - * other mock in `tests/helpers/mocks.ts` — the provider only ever touches `scan` and `unlink`. - */ -interface FakeRedis { - unlinked: string[]; - scan: Mock; - unlink: Mock; -} - -function createFakeClient(): FakeRedis { - const unlinked: string[] = []; - return { - unlinked, - scan: vi.fn().mockResolvedValue(['0', []]), - unlink: vi.fn().mockImplementation(async (...keys: string[]) => { - unlinked.push(...keys); - return Promise.resolve(keys.length); - }), - }; -} - -function asRedis(fake: FakeRedis): Redis { - return fake as unknown as Redis; -} - -/** A client whose SCAN walks `pages` in order, returning to cursor '0' only on the last one. */ -function createScanningClient(pages: string[][]): FakeRedis { - const client = createFakeClient(); - let call = 0; - client.scan = vi.fn().mockImplementation(async () => { - const page = pages[call] ?? []; - call += 1; - const cursor = call >= pages.length ? '0' : String(call); - return Promise.resolve([cursor, page] as [string, string[]]); - }); - return client; -} +import { + asRedis, + createMockLogger, + createMockRedisClient, + createMockScanningRedisClient, + createRedisStorageConfig, + type MockRedisClient, +} from '../helpers/mocks'; describe('RedisStorageProvider', () => { const config = createRedisStorageConfig({ batchSize: 3, scanCount: 2 }); - let client: FakeRedis; + let client: MockRedisClient; let provider: RedisStorageProvider; beforeEach(() => { - client = createFakeClient(); + client = createMockRedisClient(); provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); }); @@ -113,7 +79,7 @@ describe('RedisStorageProvider', () => { describe('#deleteResources', () => { it('should scan the prefix and unlink everything it finds', async () => { - client = createScanningClient([['p-1-1-1', 'p-1-1-2'], ['p-2-1-1']]); + client = createMockScanningRedisClient([['p-1-1-1', 'p-1-1-2'], ['p-2-1-1']]); provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); @@ -124,7 +90,7 @@ describe('RedisStorageProvider', () => { }); it('should follow the cursor until it returns to 0', async () => { - client = createScanningClient([['a'], ['b'], ['c']]); + client = createMockScanningRedisClient([['a'], ['b'], ['c']]); provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); @@ -133,7 +99,7 @@ describe('RedisStorageProvider', () => { }); it('should keep scanning past an empty page, because MATCH filters after retrieval', async () => { - client = createScanningClient([[], ['found']]); + client = createMockScanningRedisClient([[], ['found']]); provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); @@ -143,7 +109,7 @@ describe('RedisStorageProvider', () => { }); it('should not unlink at all for an empty page', async () => { - client = createScanningClient([[], []]); + client = createMockScanningRedisClient([[], []]); provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); @@ -152,7 +118,7 @@ describe('RedisStorageProvider', () => { }); it('should scan with the prefix and a trailing dash wildcard', async () => { - client = createScanningClient([[]]); + client = createMockScanningRedisClient([[]]); provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'layer-redis_WorldCRS84' }); @@ -161,7 +127,7 @@ describe('RedisStorageProvider', () => { }); it('should report deletedCount 0 for a cold cache rather than failing', async () => { - client = createScanningClient([[]]); + client = createMockScanningRedisClient([[]]); provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); const result = await provider.deleteResources({ storageProvider: 'REDIS', prefix: 'p' }); @@ -170,7 +136,7 @@ describe('RedisStorageProvider', () => { }); it('should accumulate failures across pages without losing the count', async () => { - client = createScanningClient([['a'], ['b']]); + client = createMockScanningRedisClient([['a'], ['b']]); client.unlink = vi.fn().mockRejectedValue(new Error('READONLY')); provider = new RedisStorageProvider(config, asRedis(client), createMockLogger()); From 83db282466d7130b9cd77ee8e0bd1f296750d0ec Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 16:30:05 +0300 Subject: [PATCH 26/27] refactor(strategies): drop the zero-delay early return before the reload window wait --- src/cleaner/strategies/deleteStoredResourcesStrategy.ts | 3 --- tests/strategies/deleteStoredResourcesStrategy.spec.ts | 8 ++++---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts index 3dcac52..7e821cb 100644 --- a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts +++ b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts @@ -60,9 +60,6 @@ export class DeleteStoredResourcesStrategy implements ITaskStrategy { - if (delaySeconds === 0) { - return; - } this.logger.info({ msg: 'Waiting for the reload window before deleting', prefix, delaySeconds }); await setTimeout(delaySeconds * MS_PER_SECOND); } diff --git a/tests/strategies/deleteStoredResourcesStrategy.spec.ts b/tests/strategies/deleteStoredResourcesStrategy.spec.ts index d797732..e341b1e 100644 --- a/tests/strategies/deleteStoredResourcesStrategy.spec.ts +++ b/tests/strategies/deleteStoredResourcesStrategy.spec.ts @@ -189,10 +189,10 @@ describe('DeleteStoredResourcesStrategy', () => { }); describe('REDIS provider', () => { - it('should wipe the prefix immediately when the task carries no delay', async () => { + it('should wipe the prefix without a reload window when the task carries no delay', async () => { await strategy.execute(redisParams); - expect(sleep).not.toHaveBeenCalled(); + expect(sleep).toHaveBeenCalledExactlyOnceWith(0); expect(mockRedisProvider.deleteResources).toHaveBeenCalledWith(redisParams); expect(mockS3Provider.deleteResources).not.toHaveBeenCalled(); }); @@ -207,10 +207,10 @@ describe('DeleteStoredResourcesStrategy', () => { expect(mockRedisProvider.deleteResources).toHaveBeenCalledWith(params); }); - it('should not wait when delaySeconds is zero', async () => { + it('should not wait out a reload window when delaySeconds is zero', async () => { await strategy.execute({ ...redisParams, delaySeconds: 0 }); - expect(sleep).not.toHaveBeenCalled(); + expect(sleep).toHaveBeenCalledExactlyOnceWith(0); expect(mockRedisProvider.deleteResources).toHaveBeenCalledOnce(); }); }); From 70837e6e32a6f93620e2210c8df2a295a5c50691 Mon Sep 17 00:00:00 2001 From: almog8k Date: Sun, 4 Oct 2026 17:36:12 +0300 Subject: [PATCH 27/27] test(strategy): remove test for zero delay in reload window --- tests/strategies/deleteStoredResourcesStrategy.spec.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/strategies/deleteStoredResourcesStrategy.spec.ts b/tests/strategies/deleteStoredResourcesStrategy.spec.ts index e341b1e..a115fa7 100644 --- a/tests/strategies/deleteStoredResourcesStrategy.spec.ts +++ b/tests/strategies/deleteStoredResourcesStrategy.spec.ts @@ -206,13 +206,6 @@ describe('DeleteStoredResourcesStrategy', () => { expect(vi.mocked(sleep).mock.invocationCallOrder[0]).toBeLessThan(vi.mocked(mockRedisProvider.deleteResources).mock.invocationCallOrder[0]!); expect(mockRedisProvider.deleteResources).toHaveBeenCalledWith(params); }); - - it('should not wait out a reload window when delaySeconds is zero', async () => { - await strategy.execute({ ...redisParams, delaySeconds: 0 }); - - expect(sleep).toHaveBeenCalledExactlyOnceWith(0); - expect(mockRedisProvider.deleteResources).toHaveBeenCalledOnce(); - }); }); it('should rethrow error thrown by deleteResources', async () => {