From f3a7a1ac305c24b24b8b8efd1e8dae119262571a Mon Sep 17 00:00:00 2001 From: vitaligi <54726763+vitaligi@users.noreply.github.com> Date: Mon, 3 Aug 2026 14:45:49 +0300 Subject: [PATCH 1/3] fix: delete empty dirs in deleted paths including parent dirs --- src/cleaner/storageProviders/fsStorageProvider.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/cleaner/storageProviders/fsStorageProvider.ts b/src/cleaner/storageProviders/fsStorageProvider.ts index f91e69b..0a71bb4 100644 --- a/src/cleaner/storageProviders/fsStorageProvider.ts +++ b/src/cleaner/storageProviders/fsStorageProvider.ts @@ -109,6 +109,8 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { }); } + await this.cleanupEmptyDirs(paths, resolveAbsolutePath(join(this.fsConfig.basePath, subPath))); + return { failures }; } @@ -131,9 +133,11 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { return areValid; } - // Attempts to remove any directories that became empty after file deletion. - // Strategy: collect every ancestor directory of every deleted file, grouped by - // depth relative to the file (levelIdx 0 = direct parent, 1 = grandparent, …). + // Attempts to remove any directories that became empty after deletion. `relativePaths` + // are the deleted targets themselves — files (#delete) or whole resource dirs + // (#deleteResources); either way only their ancestors are considered, never the target. + // Strategy: collect every ancestor directory of every deleted target, grouped by + // depth relative to it (levelIdx 0 = direct parent, 1 = grandparent, …). // Delete deepest dirs first so that once a directory is empty its parent can // also be removed in a subsequent level. rmdir silently fails on non-empty dirs, // so any directory still containing files is simply skipped. @@ -152,7 +156,7 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { for (const relativePath of relativePaths) { const parts = relativePath.split('/'); - const segments = parts.slice(0, parts.length - 1); // strip filename, keep dir segments + const segments = parts.slice(0, parts.length - 1); // strip the deleted target itself, keep ancestor segments for (let count = segments.length; count >= 1; count--) { // levelIdx 0 is the innermost dir (direct parent of the tile file); // higher values walk toward the storage root. From 48f7fd1a9d7ac85430a4b9056cc3f7ecaa86f98f Mon Sep 17 00:00:00 2001 From: vitaligi <54726763+vitaligi@users.noreply.github.com> Date: Mon, 3 Aug 2026 14:47:41 +0300 Subject: [PATCH 2/3] test: add and update tests of empty dirs deletion fix --- .../fsStorageProvider.spec.ts | 79 ++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/tests/storageProviders/fsStorageProvider.spec.ts b/tests/storageProviders/fsStorageProvider.spec.ts index 09603c6..99d1f61 100644 --- a/tests/storageProviders/fsStorageProvider.spec.ts +++ b/tests/storageProviders/fsStorageProvider.spec.ts @@ -285,12 +285,14 @@ describe('FsStorageProvider', () => { describe('#deleteResources', () => { const FS_SUB_PATH = FS_VALIDATED_CONFIG_DEFAULTS.subPaths[0]!; const RELATIVE_PATH = 'layer/v1'; + const SUB_PATH_ROOT = join(BASE_PATH, FS_SUB_PATH); 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(rm).not.toHaveBeenCalled(); + expect(rmdir).not.toHaveBeenCalled(); }); it('should successfully call delete all files and return without failures', async () => { @@ -298,6 +300,7 @@ describe('FsStorageProvider', () => { expect(result).toEqual({ failures: new Map() }); 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')); }); it('should successfully call delete all files and return without failures for multiple paths', async () => { @@ -305,13 +308,16 @@ describe('FsStorageProvider', () => { expect(result).toEqual({ failures: new Map() }); 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')); }); - it('should successfully call delete all files and return without failures for multiple paths', async () => { + 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(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)); }); it('should throw UnrecoverableError when a path escapes the base path via traversal', async () => { @@ -319,6 +325,7 @@ describe('FsStorageProvider', () => { await expect(result).rejects.toThrow(UnrecoverableError); expect(rm).not.toHaveBeenCalled(); + expect(rmdir).not.toHaveBeenCalled(); }); it('should throw UnrecoverableError when only one of several paths escapes the base path', async () => { @@ -326,6 +333,7 @@ describe('FsStorageProvider', () => { await expect(result).rejects.toThrow(UnrecoverableError); expect(rm).not.toHaveBeenCalled(); + expect(rmdir).not.toHaveBeenCalled(); }); it('should throw UnrecoverableError when a path resolves to the base path itself', async () => { @@ -333,6 +341,7 @@ describe('FsStorageProvider', () => { await expect(result).rejects.toThrow(UnrecoverableError); expect(rm).not.toHaveBeenCalled(); + expect(rmdir).not.toHaveBeenCalled(); }); it('should throw UnrecoverableError when a path resolves to the base path itself via "."', async () => { @@ -340,6 +349,7 @@ describe('FsStorageProvider', () => { await expect(result).rejects.toThrow(UnrecoverableError); expect(rm).not.toHaveBeenCalled(); + expect(rmdir).not.toHaveBeenCalled(); }); it('should return failures entry when rm rejects', async () => { @@ -430,5 +440,72 @@ describe('FsStorageProvider', () => { await expect(result).rejects.toThrow(UnrecoverableError); expect(rm).not.toHaveBeenCalled(); }); + + it('should attempt to rmdir every ancestor of the deleted resource, deepest first', async () => { + await provider.deleteResources({ paths: ['layer/v1/old'], subPath: FS_SUB_PATH, storageProvider: 'FS' }); + + expect(vi.mocked(rmdir).mock.calls.map(([path]) => path)).toEqual([join(SUB_PATH_ROOT, 'layer/v1'), join(SUB_PATH_ROOT, 'layer')]); + }); + + it('should resolve ancestors relative to the subPath root and not to the base path', async () => { + await provider.deleteResources({ paths: [RELATIVE_PATH], subPath: FS_SUB_PATH, storageProvider: 'FS' }); + + expect(rmdir).toHaveBeenCalledWith(join(SUB_PATH_ROOT, 'layer')); + expect(rmdir).not.toHaveBeenCalledWith(join(BASE_PATH, 'layer')); + }); + + it('should not attempt to rmdir the deleted resource itself, the subPath root or the base path', async () => { + await provider.deleteResources({ paths: [RELATIVE_PATH], subPath: FS_SUB_PATH, storageProvider: 'FS' }); + + const rmdirCalls = vi.mocked(rmdir).mock.calls.map(([path]) => path); + expect(rmdirCalls).not.toContain(join(SUB_PATH_ROOT, RELATIVE_PATH)); + expect(rmdirCalls).not.toContain(SUB_PATH_ROOT); + expect(rmdirCalls).not.toContain(join(BASE_PATH, 'artifacts')); + expect(rmdirCalls).not.toContain(BASE_PATH); + }); + + it('should not call rmdir for a top-level resource path that has no ancestor below the subPath root', async () => { + await provider.deleteResources({ paths: ['layer'], subPath: FS_SUB_PATH, storageProvider: 'FS' }); + + expect(rm).toHaveBeenCalledWith(join(SUB_PATH_ROOT, 'layer'), { recursive: true, force: true }); + expect(rmdir).not.toHaveBeenCalled(); + }); + + it('should deduplicate rmdir calls for ancestors shared by multiple resource paths', async () => { + await provider.deleteResources({ paths: ['layer/v1/old', 'layer/v1/new'], subPath: FS_SUB_PATH, storageProvider: 'FS' }); + + expect(vi.mocked(rmdir).mock.calls.map(([path]) => path)).toEqual([join(SUB_PATH_ROOT, 'layer/v1'), join(SUB_PATH_ROOT, 'layer')]); + }); + + it('should attempt to rmdir ancestors of paths from every batch', async () => { + const paths = Array.from({ length: 4 }, (_, i) => `layer/v${i}/old`); + + await provider.deleteResources({ paths, subPath: FS_SUB_PATH, storageProvider: 'FS' }); + + for (let i = 0; i < 4; i++) { + expect(rmdir).toHaveBeenCalledWith(join(SUB_PATH_ROOT, `layer/v${i}`)); + } + expect(rmdir).toHaveBeenCalledWith(join(SUB_PATH_ROOT, 'layer')); + }); + + it('should silently ignore rmdir failures (ENOTEMPTY)', async () => { + vi.mocked(rmdir).mockRejectedValue(Object.assign(new Error('ENOTEMPTY'), { code: 'ENOTEMPTY' })); + + const result = await provider.deleteResources({ paths: ['layer/v1/old'], subPath: FS_SUB_PATH, storageProvider: 'FS' }); + + expect(result).toEqual({ failures: new Map() }); + }); + + it('should still attempt cleanup when rm rejects, without adding cleanup errors to the failures', async () => { + vi.mocked(rm).mockRejectedValue(Object.assign(new Error('EACCES'), { code: 'EACCES' })); + vi.mocked(rmdir).mockRejectedValue(Object.assign(new Error('ENOTEMPTY'), { code: 'ENOTEMPTY' })); + + const result = await provider.deleteResources({ paths: ['layer/v1/old'], subPath: FS_SUB_PATH, storageProvider: 'FS' }); + + 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') }]]), + }); + }); }); }); From af23831ef0855fc8d1550e67d3e6bac3b33a62ba Mon Sep 17 00:00:00 2001 From: vitaligi <54726763+vitaligi@users.noreply.github.com> Date: Mon, 3 Aug 2026 17:11:26 +0300 Subject: [PATCH 3/3] chore: update @map-colonies/raster-shared --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index f2577a9..030c9cc 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ "@map-colonies/js-logger": "^5.0.0", "@map-colonies/mc-priority-queue": "^9.1.0", "@map-colonies/mc-utils": "^5.1.0", - "@map-colonies/raster-shared": "^8.3.0-alpha.2", + "@map-colonies/raster-shared": "^8.3.0", "@map-colonies/read-pkg": "^1.0.0", "@map-colonies/schemas": "^1.20.0", "@map-colonies/telemetry": "^10.0.1", @@ -2543,9 +2543,9 @@ "license": "ISC" }, "node_modules/@map-colonies/raster-shared": { - "version": "8.3.0-alpha.2", - "resolved": "https://registry.npmjs.org/@map-colonies/raster-shared/-/raster-shared-8.3.0-alpha.2.tgz", - "integrity": "sha512-P2p0MddLonzxsG/MSkcKopfI05JsaWxe9BpfYPrVllZ7wCbrzyBUnsYsfP3Rl6jHeNlqWWPCJdHkpzvgTi0FOQ==", + "version": "8.3.0", + "resolved": "https://registry.npmjs.org/@map-colonies/raster-shared/-/raster-shared-8.3.0.tgz", + "integrity": "sha512-5faR0iBC9Dpxm7Z8fY+3d+5oolHqVk/urXeOq3nHuIEMOvwRT2TrTr9fmt6TK8DaCjyQXBeEU1nijvuPLHmrEw==", "license": "ISC", "dependencies": { "@map-colonies/mc-priority-queue": "^9.1.0", diff --git a/package.json b/package.json index 03e0d1f..ffc1766 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "@map-colonies/js-logger": "^5.0.0", "@map-colonies/mc-priority-queue": "^9.1.0", "@map-colonies/mc-utils": "^5.1.0", - "@map-colonies/raster-shared": "^8.3.0-alpha.2", + "@map-colonies/raster-shared": "^8.3.0", "@map-colonies/read-pkg": "^1.0.0", "@map-colonies/schemas": "^1.20.0", "@map-colonies/telemetry": "^10.0.1",