From bbd62d8ed1a1d9f5ac49468c5e1521ea4be068c8 Mon Sep 17 00:00:00 2001 From: Chris Sherwood Date: Sat, 6 Jun 2026 15:33:51 -0700 Subject: [PATCH] fix(content): remove superseded curated map/ZIM files when a new version installs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only Wikipedia had version cleanup; every other curated map and non-Wikipedia ZIM left its prior version on disk when a newer one installed, so users silently accumulated orphaned content (potentially hundreds of GB). (#634) The install paths already record each resource via InstalledResource {resource_id, resource_type, version, file_path}, so the authoritative old-file path for a resource is known. On install of a new version we now capture the prior row before updateOrCreate repoints it, then delete the old file — gated behind a pure, fully unit-tested decision function with strict safety rails: - tracked-only: requires a prior InstalledResource row for the same resource_id, so sideloaded/untracked files are never touched - genuine replacement: old and new file paths must differ - new-file-verified: the new file must be confirmed on disk first - strictly-newer: a re-install or downgrade can't wipe a newer file - within-storage-dir: the old path must resolve under the content store ZIM cleanup deletes the old file directly (NOT via this.delete(), which would drop the InstalledResource row by resource_id that updateOrCreate just repointed) and rebuilds the Kiwix library only if a file was actually removed, so its XML never references a deleted ZIM. Maps need no library step. Wikipedia keeps its own existing cleanup path. All deletions are best-effort and logged; a failure never breaks the install. Closes #634 Co-Authored-By: Claude Opus 4.8 (1M context) --- admin/app/services/map_service.ts | 36 +++++- admin/app/services/zim_service.ts | 51 +++++++- admin/app/utils/superseded_resource.ts | 72 +++++++++++ admin/tests/unit/superseded_resource.spec.ts | 118 +++++++++++++++++++ 4 files changed, 275 insertions(+), 2 deletions(-) create mode 100644 admin/app/utils/superseded_resource.ts create mode 100644 admin/tests/unit/superseded_resource.spec.ts diff --git a/admin/app/services/map_service.ts b/admin/app/services/map_service.ts index 9de8c49..7aa20a7 100644 --- a/admin/app/services/map_service.ts +++ b/admin/app/services/map_service.ts @@ -21,6 +21,7 @@ import logger from '@adonisjs/core/services/logger' import { assertNotPrivateUrl } from '#validators/common' import InstalledResource from '#models/installed_resource' import { CollectionManifestService } from './collection_manifest_service.js' +import { decideSupersededDeletion } from '../utils/superseded_resource.js' import type { CollectionWithStatus, MapsSpec } from '../../types/collections.js' import type { Country, CountryCode, CountryGroup, MapExtractPreflight } from '../../types/maps.js' import { @@ -199,10 +200,18 @@ export class MapService implements IMapService { const parsed = CollectionManifestService.parseMapFilename(filename) if (!parsed) continue - const filepath = join(process.cwd(), this.mapStoragePath, 'pmtiles', filename) + const pmtilesDir = join(process.cwd(), this.mapStoragePath, 'pmtiles') + const filepath = join(pmtilesDir, filename) const stats = await getFileStatsIfExists(filepath) try { + // Capture the prior install for this resource_id before updateOrCreate + // overwrites it, so we know the old file to clean up (#634). + const prior = await InstalledResource.query() + .where('resource_id', parsed.resource_id) + .where('resource_type', 'map') + .first() + const { DateTime } = await import('luxon') await InstalledResource.updateOrCreate( { resource_id: parsed.resource_id, resource_type: 'map' }, @@ -215,6 +224,31 @@ export class MapService implements IMapService { } ) logger.info(`[MapService] Created InstalledResource entry for: ${parsed.resource_id}`) + + // Remove the superseded prior version's pmtiles file if every safety + // rail passes (see decideSupersededDeletion). Maps have no library index, + // so a direct delete of the recorded old file is sufficient. + const decision = decideSupersededDeletion({ + existing: prior ? { file_path: prior.file_path, version: prior.version } : null, + newFilePath: filepath, + newVersion: parsed.version, + newFileExists: !!stats, + storageBaseDir: pmtilesDir, + }) + if (decision.delete && decision.path) { + try { + await deleteFileIfExists(decision.path) + logger.info( + `[MapService] Removed superseded ${parsed.resource_id} file: ${decision.path}` + ) + } catch (err) { + logger.warn(`[MapService] Failed to remove superseded file ${decision.path}:`, err) + } + } else if (decision.reason !== 'first_install' && decision.reason !== 'same_file') { + logger.info( + `[MapService] Kept prior ${parsed.resource_id} file (reason: ${decision.reason})` + ) + } } catch (error) { logger.error(`[MapService] Failed to create InstalledResource for ${filename}:`, error) } diff --git a/admin/app/services/zim_service.ts b/admin/app/services/zim_service.ts index aeaa8ff..3b0469f 100644 --- a/admin/app/services/zim_service.ts +++ b/admin/app/services/zim_service.ts @@ -8,6 +8,7 @@ import * as cheerio from 'cheerio' import { XMLParser } from 'fast-xml-parser' import { isRawListRemoteZimFilesResponse, isRawRemoteZimFileEntry } from '../../util/zim.js' import { findReplacedWikipediaFiles } from '../utils/zim_filename.js' +import { decideSupersededDeletion } from '../utils/superseded_resource.js' import logger from '@adonisjs/core/services/logger' import { DockerService } from './docker_service.js' import { inject } from '@adonisjs/core' @@ -358,6 +359,8 @@ export class ZimService { } // Create InstalledResource entries for downloaded files + const zimStorageDir = join(process.cwd(), ZIM_STORAGE_PATH) + let removedSupersededZim = false for (const url of urls) { // Skip Wikipedia files (managed separately) if (url.includes('wikipedia_en_')) continue @@ -368,10 +371,17 @@ export class ZimService { const parsed = CollectionManifestService.parseZimFilename(filename) if (!parsed) continue - const filepath = join(process.cwd(), ZIM_STORAGE_PATH, filename) + const filepath = join(zimStorageDir, filename) const stats = await getFileStatsIfExists(filepath) try { + // Capture the prior install for this resource_id BEFORE updateOrCreate + // overwrites it, so we know the old file path to clean up (#634). + const prior = await InstalledResource.query() + .where('resource_id', parsed.resource_id) + .where('resource_type', 'zim') + .first() + const { DateTime } = await import('luxon') await InstalledResource.updateOrCreate( { resource_id: parsed.resource_id, resource_type: 'zim' }, @@ -384,10 +394,49 @@ export class ZimService { } ) logger.info(`[ZimService] Created InstalledResource entry for: ${parsed.resource_id}`) + + // Remove the superseded prior version's file if (and only if) every + // safety rail passes — see decideSupersededDeletion. The InstalledResource + // row already points at the new file, so we delete the old file directly + // (NOT via this.delete(), which would drop the row by resource_id). + const decision = decideSupersededDeletion({ + existing: prior ? { file_path: prior.file_path, version: prior.version } : null, + newFilePath: filepath, + newVersion: parsed.version, + newFileExists: !!stats, + storageBaseDir: zimStorageDir, + }) + if (decision.delete && decision.path) { + try { + await deleteFileIfExists(decision.path) + removedSupersededZim = true + logger.info( + `[ZimService] Removed superseded ${parsed.resource_id} file: ${decision.path}` + ) + } catch (err) { + logger.warn(`[ZimService] Failed to remove superseded file ${decision.path}:`, err) + } + } else if (decision.reason !== 'first_install' && decision.reason !== 'same_file') { + logger.info( + `[ZimService] Kept prior ${parsed.resource_id} file (reason: ${decision.reason})` + ) + } } catch (error) { logger.error(`[ZimService] Failed to create InstalledResource for ${filename}:`, error) } } + + // If we removed any superseded ZIM, rebuild the Kiwix library so its XML no + // longer references the deleted file. The earlier rebuild in this flow ran + // while both versions were still on disk. + if (removedSupersededZim) { + try { + await new KiwixLibraryService().rebuildFromDisk() + logger.info('[ZimService] Rebuilt Kiwix library after removing superseded ZIM(s).') + } catch (err) { + logger.error('[ZimService] Failed to rebuild Kiwix library after cleanup:', err) + } + } } /** diff --git a/admin/app/utils/superseded_resource.ts b/admin/app/utils/superseded_resource.ts new file mode 100644 index 0000000..9094953 --- /dev/null +++ b/admin/app/utils/superseded_resource.ts @@ -0,0 +1,72 @@ +import { resolve, sep } from 'node:path' + +/** + * Decides whether a curated resource's PREVIOUSLY-installed file should be + * deleted now that a newer version has been downloaded (issue #634 — old map + * and ZIM versions accumulated on disk indefinitely because only Wikipedia had + * version cleanup). + * + * This is intentionally a pure function so every safety rail is unit-testable + * without touching the DB or filesystem. The caller looks up the prior + * `InstalledResource` row, records the new version, then asks this whether the + * old file is safe to remove. + * + * Safety rails (a "delete" decision requires ALL of these): + * - There was a prior install for this exact resource_id (`existing` non-null). + * Untracked / sideloaded files have no row and are therefore never touched. + * - The old file path actually differs from the new one (a genuine version + * swap, not a re-download of the same file). + * - The new file is confirmed present on disk — we never remove the old copy + * before the replacement is verified. + * - The new version is strictly newer than the recorded one, so a re-install + * or downgrade can't wipe a newer file. + * - The old path resolves to within the resource's storage directory, so a + * malformed DB value can't direct a delete outside the content store. + */ + +export interface SupersededInputs { + /** Prior InstalledResource row for this resource_id, or null on first install. */ + existing: { file_path: string; version: string } | null + /** Absolute path of the newly downloaded file. */ + newFilePath: string + /** Version of the newly downloaded file (e.g. "2026-05"). */ + newVersion: string + /** Whether the new file is confirmed present on disk. */ + newFileExists: boolean + /** Absolute storage directory the old file must live under to be eligible. */ + storageBaseDir: string +} + +export type SupersededReason = + | 'first_install' + | 'same_file' + | 'new_file_missing' + | 'not_newer' + | 'outside_storage' + | 'superseded' + +export interface SupersededDecision { + delete: boolean + /** Resolved old path to delete — set only when `delete` is true. */ + path?: string + reason: SupersededReason +} + +export function decideSupersededDeletion(inputs: SupersededInputs): SupersededDecision { + const { existing, newFilePath, newVersion, newFileExists, storageBaseDir } = inputs + + if (!existing) return { delete: false, reason: 'first_install' } + if (existing.file_path === newFilePath) return { delete: false, reason: 'same_file' } + if (!newFileExists) return { delete: false, reason: 'new_file_missing' } + // Versions are zero-padded date strings (YYYY-MM / YYYY-MM-DD), so a lexical + // compare orders them correctly. Require strictly newer. + if (!(newVersion > existing.version)) return { delete: false, reason: 'not_newer' } + + const resolvedOld = resolve(existing.file_path) + const base = resolve(storageBaseDir) + if (resolvedOld !== base && !resolvedOld.startsWith(base + sep)) { + return { delete: false, reason: 'outside_storage' } + } + + return { delete: true, path: resolvedOld, reason: 'superseded' } +} diff --git a/admin/tests/unit/superseded_resource.spec.ts b/admin/tests/unit/superseded_resource.spec.ts new file mode 100644 index 0000000..46d6d47 --- /dev/null +++ b/admin/tests/unit/superseded_resource.spec.ts @@ -0,0 +1,118 @@ +import * as assert from 'node:assert/strict' +import { test } from 'node:test' +import { join } from 'node:path' + +import { decideSupersededDeletion } from '../../app/utils/superseded_resource.js' + +const STORAGE = join('/app', 'storage', 'zim') +const OLD = join(STORAGE, 'ifixit_en_all_2026-03.zim') +const NEW = join(STORAGE, 'ifixit_en_all_2026-05.zim') + +test('first install (no prior row) deletes nothing', () => { + const d = decideSupersededDeletion({ + existing: null, + newFilePath: NEW, + newVersion: '2026-05', + newFileExists: true, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, false) + assert.equal(d.reason, 'first_install') +}) + +test('superseded prior version is deleted', () => { + const d = decideSupersededDeletion({ + existing: { file_path: OLD, version: '2026-03' }, + newFilePath: NEW, + newVersion: '2026-05', + newFileExists: true, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, true) + assert.equal(d.reason, 'superseded') + assert.equal(d.path, OLD) +}) + +test('same file path is never deleted (re-download)', () => { + const d = decideSupersededDeletion({ + existing: { file_path: NEW, version: '2026-05' }, + newFilePath: NEW, + newVersion: '2026-05', + newFileExists: true, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, false) + assert.equal(d.reason, 'same_file') +}) + +test('old file is NOT deleted until the new file is confirmed on disk', () => { + const d = decideSupersededDeletion({ + existing: { file_path: OLD, version: '2026-03' }, + newFilePath: NEW, + newVersion: '2026-05', + newFileExists: false, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, false) + assert.equal(d.reason, 'new_file_missing') +}) + +test('downgrade / reinstall of an older version does not wipe the newer file', () => { + const d = decideSupersededDeletion({ + existing: { file_path: NEW, version: '2026-05' }, + newFilePath: OLD, + newVersion: '2026-03', + newFileExists: true, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, false) + assert.equal(d.reason, 'not_newer') +}) + +test('equal version is not considered newer', () => { + const d = decideSupersededDeletion({ + existing: { file_path: join(STORAGE, 'ifixit_en_all_2026-05a.zim'), version: '2026-05' }, + newFilePath: NEW, + newVersion: '2026-05', + newFileExists: true, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, false) + assert.equal(d.reason, 'not_newer') +}) + +test('refuses to delete a path outside the storage directory', () => { + const d = decideSupersededDeletion({ + existing: { file_path: '/etc/passwd', version: '2026-03' }, + newFilePath: NEW, + newVersion: '2026-05', + newFileExists: true, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, false) + assert.equal(d.reason, 'outside_storage') +}) + +test('refuses a traversal escape that only looks like it is under storage', () => { + const d = decideSupersededDeletion({ + existing: { file_path: join(STORAGE, '..', 'mysql', 'data.ibd'), version: '2026-03' }, + newFilePath: NEW, + newVersion: '2026-05', + newFileExists: true, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, false) + assert.equal(d.reason, 'outside_storage') +}) + +test('YYYY-MM-DD versions order correctly against YYYY-MM', () => { + const d = decideSupersededDeletion({ + existing: { file_path: OLD, version: '2026-05' }, + newFilePath: NEW, + newVersion: '2026-05-12', + newFileExists: true, + storageBaseDir: STORAGE, + }) + assert.equal(d.delete, true) + assert.equal(d.reason, 'superseded') +})