feat(backups): add a cache guard and migrate required to backup-archive (#10243)

[XO-2789]
* Extracts the backup migration logic into backup-archive and adds a cache guard for future performance improvements
* don't generate or read cache file on immutable backup repository
* clean cache file on immutable backup repository

@xen-orchestra/backup-archive/src/BackupCache.mts

@@ -0,0 +1,49 @@
+import { RemoteHandlerAbstract } from '@xen-orchestra/fs'
+import { promisify } from 'node:util'
+import zlib from 'node:zlib'
+
+const gzip = promisify(zlib.gzip)
+const gunzip = promisify(zlib.gunzip)
+
+type LogWarn = (message: any, opts?: object) => void
+
+/**
+ * Reads a gzip-compressed JSON cache file, tolerant of a missing or corrupt file.
+ * Shared with the legacy @xen-orchestra/backups RemoteAdapter.
+ */
+export async function readBackupCache(
+  handler: RemoteHandlerAbstract,
+  path: string,
+  logWarn: LogWarn
+): Promise<Record<string, unknown> | undefined> {
+  try {
+    return JSON.parse((await gunzip(await handler.readFile(path))).toString())
+  } catch (error) {
+    if (error?.code !== 'ENOENT') {
+      logWarn('failed to read cache', { error, path })
+    }
+  }
+}
+
+/**
+ * Writes data as a gzip-compressed JSON cache file.
+ * Shared with the legacy @xen-orchestra/backups RemoteAdapter.
+ *
+ * No-op on immutable remotes: the cache file may not be modifiable there,
+ * and callers should be regenerating/reading the data directly instead.
+ */
+export async function writeBackupCache(
+  handler: RemoteHandlerAbstract,
+  path: string,
+  data: Record<string, unknown>,
+  logWarn: LogWarn
+): Promise<void> {
+  if (handler.isImmutable()) {
+    return
+  }
+  try {
+    await handler.writeFile(path, await gzip(JSON.stringify(data)), { flags: 'w' })
+  } catch (error) {
+    logWarn('failed to write cache', { error, path })
+  }
+}

@xen-orchestra/backup-archive/src/MetadataBackup.mts

@@ -0,0 +1,16 @@
+import { RemoteHandlerAbstract } from '@xen-orchestra/fs'
+
+const METADATA_BACKUP_ID_REGEXP =
+  /^xo-(config|pool-metadata)-backups\/\w{8}(-\w{4}){3}-\w{12}(\/\w{8}(-\w{4}){3}-\w{12})?\/\d{8}T\d{6}Z/
+
+/**
+ * Deletes an XO config or pool metadata backup directory (not a VM backup).
+ * Shared with the legacy @xen-orchestra/backups RemoteAdapter.
+ */
+export async function deleteMetadataBackup(handler: RemoteHandlerAbstract, backupId: string): Promise<void> {
+  if (!METADATA_BACKUP_ID_REGEXP.test(backupId)) {
+    throw new Error(`The id (${backupId}) not correspond to a metadata folder`)
+  }
+
+  await handler.rmtree(backupId)
+}

@xen-orchestra/backup-archive/src/VmBackupDirectory.mts

@@ -17,11 +17,8 @@ import {
 import { cleanOrphanDiskDirs } from '@xen-orchestra/backup-archive/disks'
 import { asyncEach } from '@vates/async-each'
 import { createLogger } from '@xen-orchestra/log'
-import { promisify } from 'node:util'
-import zlib from 'node:zlib'
-
-const gzip = promisify(zlib.gzip)
-const gunzip = promisify(zlib.gunzip)
+import { readBackupCache, writeBackupCache } from './BackupCache.mjs'
+import { unlinkTolerant } from './_unlinkTolerant.mjs'
 
 const { info: logInfo, warn: logWarn } = createLogger('xo:backup-archive')
 
@@ -30,7 +27,7 @@ const FILES_TO_KEEP = ['cache.json.gz', 'vdis']
 export class VmBackupDirectory implements VmBackupInterface {
   handler: RemoteHandlerAbstract
   rootPath: string
-  files: Array<string> = new Array()
+  files: Array<string> = []
   orphans: Set<string> = new Set()
   backupArchives: Map<string, VmBackupInterface> = new Map()
   opts: ResolvedBackupCleanOptions
@@ -43,6 +40,9 @@ export class VmBackupDirectory implements VmBackupInterface {
   // on disk, so clean() must regenerate it even when nothing was merged/removed.
   #cacheOutOfSync = false
 
+  // Set by #checkCacheCount(): whether cache.json.gz existed at the start of the run.
+  #cacheExisted = false
+
   constructor(
     handler: RemoteHandlerAbstract,
     vmBackupPath: string,
@@ -67,21 +67,11 @@ export class VmBackupDirectory implements VmBackupInterface {
   }
 
   async #readCache(path: string): Promise<Record<string, unknown> | undefined> {
-    try {
-      return JSON.parse((await gunzip(await this.handler.readFile(path))).toString())
-    } catch (error) {
-      if (error?.code !== 'ENOENT') {
-        logWarn('failed to read cache', { error, path })
-      }
-    }
+    return readBackupCache(this.handler, path, logWarn)
   }
 
   async #writeCache(path: string, data: Record<string, unknown>): Promise<void> {
-    try {
-      await this.handler.writeFile(path, await gzip(JSON.stringify(data)), { flags: 'w' })
-    } catch (error) {
-      logWarn('failed to write cache', { error, path })
-    }
+    return writeBackupCache(this.handler, path, data, logWarn)
   }
 
   async init() {
@@ -90,7 +80,7 @@ export class VmBackupDirectory implements VmBackupInterface {
     this.#uniqueLineages = new Map()
 
     for (const fullPath of this.files.filter(path => path.endsWith('.json'))) {
-      let metadata: PartialBackupMetadata | undefined = undefined
+      let metadata: PartialBackupMetadata | undefined
       try {
         metadata = JSON.parse(await this.handler.readFile(fullPath)) satisfies PartialBackupMetadata
       } catch (error) {
@@ -176,18 +166,28 @@ export class VmBackupDirectory implements VmBackupInterface {
 
     // Let each archive clean its own files (e.g. remove metadata for incomplete backups)
     // and update metadata with merged sizes if applicable
+    const goneArchives: string[] = []
     await asyncEach(
-      Array.from(this.backupArchives.values()),
-      async (archive: VmBackupInterface) => {
+      Array.from(this.backupArchives.entries()),
+      async ([metadataPath, archive]: [string, VmBackupInterface]) => {
         const { removedFiles } = await archive.clean({ remove, mergedSizes: allMergedSizes })
         if (removedFiles.length > 0) {
           cacheNeedsRegen = true
         }
+        if (removedFiles.includes(metadataPath)) {
+          goneArchives.push(metadataPath)
+        }
       },
       { concurrency: 2 }
     )
 
-    if (allMergedSizes.size > 0 || cacheNeedsRegen || this.#cacheOutOfSync) {
+    // an archive whose metadata has just been deleted is not a backup anymore: it must not be
+    // advertised by the regenerated cache
+    for (const metadataPath of goneArchives) {
+      this.backupArchives.delete(metadataPath)
+    }
+
+    if (this.#cacheExisted && (allMergedSizes.size > 0 || cacheNeedsRegen || this.#cacheOutOfSync)) {
       await this.#regenerateCache()
     }
 
@@ -215,8 +215,34 @@ export class VmBackupDirectory implements VmBackupInterface {
 
   async #checkCacheCount(): Promise<void> {
     const cachePath = `${this.rootPath}/cache.json.gz`
+
+    this.#cacheExisted = false
+    this.#cacheOutOfSync = false
+
+    if (!this.files.includes(normalize(cachePath))) {
+      return
+    }
+
+    if (this.handler.isImmutable()) {
+      //Best effort: some remotes (e.g. S3) may not normalize a permission error to EPERM, so tolerate any error.
+      try {
+        await this.handler.unlink(cachePath)
+      } catch (error) {
+        this.opts.logWarn('error while deleting leftover backup cache', { path: cachePath, error })
+      }
+      return
+    }
+
     const existingCache = await this.#readCache(cachePath)
-    const actual = existingCache === undefined ? 0 : Object.keys(existingCache).length
+    if (existingCache === undefined) {
+      // present but unreadable: drop it, RemoteAdapter will recreate a valid one
+      this.opts.logWarn('removing unreadable backup cache', { path: cachePath })
+      await unlinkTolerant(this.handler, cachePath)
+      return
+    }
+
+    this.#cacheExisted = true
+    const actual = Object.keys(existingCache).length
     const expected = this.backupArchives.size
     this.#cacheOutOfSync = actual !== expected
     if (this.#cacheOutOfSync) {

@xen-orchestra/backup-archive/src/VmFullBackupArchive.integ.mjs

@@ -10,8 +10,13 @@ import { rimraf } from 'rimraf'
 // eslint-disable-next-line n/no-missing-import
 import { VmBackupDirectory } from '../dist/VmBackupDirectory.mjs'
 import tar from 'tar-stream'
+import { promisify } from 'node:util'
+import zlib from 'node:zlib'
 const { beforeEach, afterEach, describe } = test
 
+const gzip = promisify(zlib.gzip)
+const gunzip = promisify(zlib.gunzip)
+
 let tempDir, handler, vmBackupDir
 const vmUuid = 'test-vm-uuid'
 const rootPath = `xo-vm-backups/${vmUuid}`
@@ -124,7 +129,10 @@ describe('VmBackupDirectory with full backups', { concurrency: 1 }, () => {
 
   test('clean() preserves cache.json.gz', async () => {
     await createFullBackupMetadata('backup1.json', 'backup1.xva')
-    await handler.writeFile(`${rootPath}/cache.json.gz`, 'cache')
+    await handler.writeFile(
+      `${rootPath}/cache.json.gz`,
+      await gzip(JSON.stringify({ [`/${rootPath}/backup1.json`]: {} }))
+    )
 
     await VmBackupDirectory.cleanVm(handler, rootPath)
 
@@ -133,6 +141,104 @@ describe('VmBackupDirectory with full backups', { concurrency: 1 }, () => {
     assert.ok(remainingFiles.includes('cache.json.gz'))
   })
 
+  test('clean() removes an unreadable cache.json.gz', async () => {
+    await createFullBackupMetadata('backup1.json', 'backup1.xva')
+    await handler.writeFile(`${rootPath}/cache.json.gz`, 'not gzipped json')
+
+    await VmBackupDirectory.cleanVm(handler, rootPath, { logWarn: () => {} })
+
+    const remainingFiles = await handler.list(rootPath)
+    assert.ok(!remainingFiles.includes('cache.json.gz'), 'an unreadable cache.json.gz should be removed, not kept')
+  })
+
+  test('clean() on an immutable remote never creates cache.json.gz', async () => {
+    await createFullBackupMetadata('backup1.json', 'backup1.xva')
+    await handler.writeFile(`${rootPath}/orphan.xva`, 'orphan-content')
+    handler.isImmutable = () => true
+
+    await VmBackupDirectory.cleanVm(handler, rootPath, { remove: true })
+
+    const remainingFiles = await handler.list(rootPath)
+    assert.ok(!remainingFiles.includes('cache.json.gz'), 'cache.json.gz should never be created on an immutable remote')
+  })
+
+  test('clean() removes a leftover cache.json.gz found on an immutable remote', async () => {
+    await createFullBackupMetadata('backup1.json', 'backup1.xva')
+    await handler.writeFile(`${rootPath}/cache.json.gz`, await gzip(JSON.stringify({})))
+    handler.isImmutable = () => true
+
+    await VmBackupDirectory.cleanVm(handler, rootPath)
+
+    const remainingFiles = await handler.list(rootPath)
+    assert.ok(!remainingFiles.includes('cache.json.gz'), 'erroneous cache.json.gz should be removed, not rewritten')
+  })
+
+  test('clean() removes an unreadable leftover cache.json.gz found on an immutable remote', async () => {
+    await createFullBackupMetadata('backup1.json', 'backup1.xva')
+    await handler.writeFile(`${rootPath}/cache.json.gz`, 'not gzipped json')
+    handler.isImmutable = () => true
+
+    await VmBackupDirectory.cleanVm(handler, rootPath)
+
+    const remainingFiles = await handler.list(rootPath)
+    assert.ok(
+      !remainingFiles.includes('cache.json.gz'),
+      'an unreadable cache.json.gz should be removed from an immutable remote'
+    )
+  })
+
+  test('clean() tolerates an EPERM while removing the cache of an immutable remote', async () => {
+    await createFullBackupMetadata('backup1.json', 'backup1.xva')
+    await handler.writeFile(`${rootPath}/cache.json.gz`, await gzip(JSON.stringify({})))
+    handler.isImmutable = () => true
+    handler.unlink = () => {
+      const error = new Error('EPERM: operation not permitted')
+      error.code = 'EPERM'
+      throw error
+    }
+
+    await VmBackupDirectory.cleanVm(handler, rootPath)
+
+    const remainingFiles = await handler.list(rootPath)
+    assert.ok(remainingFiles.includes('cache.json.gz'), 'the immutable cache.json.gz could not be removed')
+  })
+
+  test('clean() keeps maintaining a pre-existing cache.json.gz across a remove', async () => {
+    await createFullBackupMetadata('backup1.json', 'backup1.xva')
+    await createFullBackupMetadata('backup2.json', 'backup2.xva')
+
+    // a cache with the expected number of entries: only the removal can trigger the regeneration
+    const cachePath = `${rootPath}/cache.json.gz`
+    await handler.writeFile(
+      cachePath,
+      await gzip(
+        JSON.stringify({
+          [`/${rootPath}/backup1.json`]: { stale: true },
+          [`/${rootPath}/backup2.json`]: { stale: true },
+        })
+      )
+    )
+
+    // backup2 loses its XVA: its metadata will be removed by clean()
+    await handler.unlink(`${rootPath}/backup2.xva`)
+
+    await VmBackupDirectory.cleanVm(handler, rootPath, { remove: true, logInfo: () => {}, logWarn: () => {} })
+
+    const remainingFiles = await handler.list(rootPath)
+    assert.ok(remainingFiles.includes('backup1.json'))
+    assert.ok(remainingFiles.includes('backup1.xva'))
+    assert.ok(!remainingFiles.includes('backup2.json'), 'metadata of the backup with a missing XVA should be removed')
+    assert.ok(remainingFiles.includes('cache.json.gz'), 'an existing cache.json.gz must not be dropped by a remove')
+
+    const cache = JSON.parse((await gunzip(await handler.readFile(cachePath))).toString())
+    assert.deepEqual(
+      Object.keys(cache),
+      [`/${rootPath}/backup1.json`],
+      'cache.json.gz should have been regenerated with the surviving backup only'
+    )
+    assert.equal(cache[`/${rootPath}/backup1.json`].stale, undefined, 'the entry must come from the archive on disk')
+  })
+
   test('clean() removes an orphan XVA checksum file (no matching metadata or XVA)', async () => {
     await handler.writeFile(`${rootPath}/stray.xva.checksum`, 'sha256:deadbeef')
 

@xen-orchestra/backup-archive/src/VmFullBackupArchive.mts

@@ -1,6 +1,6 @@
-import { basename, normalize } from '@xen-orchestra/fs/path'
+import { basename, normalize, resolveFromFile } from '@xen-orchestra/fs/path'
 import assert from 'node:assert'
-import { FileDescriptor } from '@xen-orchestra/fs'
+import { FileDescriptor, RemoteHandlerAbstract } from '@xen-orchestra/fs'
 import {
   ArchiveCleanOptions,
   CheckResult,
@@ -8,8 +8,10 @@ import {
   VmBackupInterface,
   PartialBackupMetadata,
   ResolvedBackupCleanOptions,
+  DEFAULT_REMOVE_CONCURRENCY,
 } from './VmBackup.types.mjs'
-import { RemoteHandlerAbstract } from '@xen-orchestra/fs'
+import { asyncEach } from '@vates/async-each'
+import { unlinkTolerant } from './_unlinkTolerant.mjs'
 
 const COMPRESSED_MAGIC_NUMBERS: Buffer[] = [
   // https://tools.ietf.org/html/rfc1952.html#page-5
@@ -80,6 +82,29 @@ export async function isValidXva(
 
 const noop = (): void => {}
 
+/**
+ * Deletes a full VM backup's metadata json and, when provided, its xva and checksum.
+ * `xva` is the path as stored in the metadata, i.e. relative to the metadata file.
+ * Shared with the legacy @xen-orchestra/backups RemoteAdapter.
+ */
+export async function deleteFullVmBackups(
+  handler: RemoteHandlerAbstract,
+  backups: Array<{ metadataPath: string; xva?: string }>
+): Promise<void> {
+  await asyncEach(
+    backups,
+    async ({ metadataPath, xva }) => {
+      await unlinkTolerant(handler, metadataPath)
+      if (xva !== undefined) {
+        const xvaPath = resolveFromFile(metadataPath, xva)
+        await unlinkTolerant(handler, xvaPath)
+        await unlinkTolerant(handler, `${xvaPath}.checksum`)
+      }
+    },
+    { concurrency: DEFAULT_REMOVE_CONCURRENCY, stopOnError: false }
+  )
+}
+
 export class VmFullBackupArchive implements VmBackupInterface {
   handler: RemoteHandlerAbstract
   rootPath: string
@@ -158,12 +183,10 @@ export class VmFullBackupArchive implements VmBackupInterface {
             files: this.getAssociatedFiles({ prefix: false }),
           })
         } else {
-          for (const file of removedFiles) {
-            try {
-              await this.handler.unlink(file)
-            } catch (error) {
-              this.opts.logWarn(`Issue removing ${file}`, { error })
-            }
+          try {
+            await deleteFullVmBackups(this.handler, [{ metadataPath: this.metadataPath, xva: this.metadata.xva }])
+          } catch (error) {
+            this.opts.logWarn(`Issue removing backup files`, { error, metadataPath: this.metadataPath })
           }
         }
       }
@@ -173,7 +196,7 @@ export class VmFullBackupArchive implements VmBackupInterface {
   }
 
   getAssociatedFiles({ prefix = false }): Array<string> {
-    let validFiles = [this.metadataPath, this.xvaPath, `${this.xvaPath}.checksum`]
+    const validFiles = [this.metadataPath, this.xvaPath, `${this.xvaPath}.checksum`]
     return prefix ? validFiles : validFiles.map(file => basename(file))
   }
 }

@xen-orchestra/backup-archive/src/VmIncrementalBackupArchive.integ.mjs

@@ -6,10 +6,13 @@ import fs from 'fs-extra'
 import * as uuid from 'uuid'
 import { getHandler } from '@xen-orchestra/fs'
 import { pFromCallback } from 'promise-toolbox'
-// eslint-disable-next-line n/no-missing-import
+// `dist/` is built by the `test-integration` script, which starts by removing it: these imports
+// cannot be resolved when linting a working tree that has not been built yet
+/* eslint-disable n/no-missing-import */
 import { VmBackupDirectory } from '../dist/VmBackupDirectory.mjs'
 import { RemoteVhdDisk } from '../dist/disks/RemoteVhdDisk.mjs'
 import { MergeRemoteDisk } from '../dist/disks/MergeRemoteDisk.mjs'
+/* eslint-enable n/no-missing-import */
 import { VHDFOOTER, VHDHEADER } from './tests.fixtures.mjs'
 import { VhdFile, Constants, VhdDirectory, VhdAbstract } from 'vhd-lib'
 import { dirname, basename } from 'node:path'
@@ -20,6 +23,7 @@ import { rimraf } from 'rimraf'
 const { beforeEach, afterEach, describe } = test
 
 const gunzip = promisify(zlib.gunzip)
+const gzip = promisify(zlib.gzip)
 
 let tempDir, handler, jobId, vdiId, basePath, relativePath, vdiId2, basePath2, relativePath2
 const rootPath = 'xo-vm-backups/VMUUID/'
@@ -205,8 +209,8 @@ test('it remove backup metadata referencing a missing vhd in delta backup', asyn
   assert.equal(matched.length, 2) // all vhds (orphan and  child  ) should have been deleted
 
   assert.ok(
-    (await handler.list(rootPath)).includes('cache.json.gz'),
-    'cache.json.gz should be regenerated after metadata deletion'
+    !(await handler.list(rootPath)).includes('cache.json.gz'),
+    'cache.json.gz should not be created when none existed before the clean'
   )
 })
 
@@ -364,7 +368,6 @@ test('it merges delta of non destroyed chain', async () => {
     logged.push(message)
   }
   await VmBackupDirectory.cleanVm(handler, rootPath, { remove: true, logInfo, logWarn: logInfo })
-  assert.equal(logged[0], `unexpected number of entries in backup cache`)
 
   logged = []
   await VmBackupDirectory.cleanVm(handler, rootPath, { remove: true, merge: true, logInfo, logWarn: () => {} })
@@ -726,10 +729,10 @@ describe('tests multiple combination ', { concurrency: 1 }, () => {
         const metadata = JSON.parse(await handler.readFile(`${rootPath}/metadata.json`))
         // size should be the size of children + grand children + clean after the merge
         assert.deepEqual(metadata.size, 6501888)
-        // cache.json.gz must be regenerated when metadata changes after a merge
+        // no cache.json.gz existed before this run, so a merge must not create one
         assert.ok(
-          (await handler.list(rootPath)).includes('cache.json.gz'),
-          'cache.json.gz should be regenerated after a merge'
+          !(await handler.list(rootPath)).includes('cache.json.gz'),
+          'cache.json.gz should not be created when none existed before the merge'
         )
 
         // broken vhd, non referenced, abandoned should be deleted ( alias and data)
@@ -833,7 +836,7 @@ test('it preserves VDI directory when handler.list() throws during lineage init'
   assert.ok(remaining.includes('snapshot.vhd'), 'VHD must survive when handler.list() throws during lineage init')
 })
 
-test('it regenerates the cache when its entry count is out of sync, even with nothing to merge/remove', async () => {
+test('it does not create nor warn about the cache when none existed, even with a count mismatch', async () => {
   // a referenced disk + its metadata, but no cache.json.gz on disk
   await generateVhd(`${basePath}/diskA1.vhd`)
   await handler.writeFile(
@@ -842,6 +845,78 @@ test('it regenerates the cache when its entry count is out of sync, even with no
   )
   await assert.rejects(handler.readFile(`${rootPath}/cache.json.gz`), { code: 'ENOENT' })
 
+  const logged = []
+  // remove/merge both false: cacheNeedsRegen stays false and no merge happens,
+  // so only a pre-existing cache's count mismatch could trigger regeneration
+  await VmBackupDirectory.cleanVm(handler, rootPath, {
+    remove: false,
+    merge: false,
+    logInfo: () => {},
+    logWarn: message => logged.push(message),
+  })
+
+  assert.ok(!logged.includes('unexpected number of entries in backup cache'), 'no cache to be mismatched, no warning')
+  await assert.rejects(handler.readFile(`${rootPath}/cache.json.gz`), { code: 'ENOENT' })
+})
+
+test('it regenerates a pre-existing cache after a merge', async () => {
+  await handler.writeFile(
+    `${rootPath}/metadata.json`,
+    JSON.stringify({
+      mode: 'delta',
+      size: 12000, // a size too small, the merge will fix it
+      vhds: [`${relativePath}/grandchild.vhd`, `${relativePath}/child.vhd`],
+    })
+  )
+
+  // one orphan, which is a full vhd, no parent
+  const orphan = await generateVhd(`${basePath}/orphan.vhd`)
+  // a child to the orphan, orphan will be merged into it
+  const child = await generateVhd(`${basePath}/child.vhd`, {
+    header: {
+      parentUnicodeName: 'orphan.vhd',
+      parentUuid: orphan.footer.uuid,
+    },
+    blocks: [0, 1],
+  })
+  await generateVhd(`${basePath}/grandchild.vhd`, {
+    header: {
+      parentUnicodeName: 'child.vhd',
+      parentUuid: child.footer.uuid,
+    },
+  })
+
+  // a cache with the expected number of entries: it is not out of sync, only the merge can trigger the regeneration
+  await handler.writeFile(
+    `${rootPath}/cache.json.gz`,
+    await gzip(JSON.stringify({ [`/${rootPath}metadata.json`]: { size: 12000 } }))
+  )
+
+  await VmBackupDirectory.cleanVm(handler, rootPath, {
+    remove: true,
+    merge: true,
+    logInfo: () => {},
+    logWarn: () => {},
+  })
+
+  const metadata = JSON.parse(await handler.readFile(`${rootPath}/metadata.json`))
+  // size of child + grandchild after orphan has been merged into child
+  assert.equal(metadata.size, 4299776, 'metadata.size should be updated after merge')
+
+  const cache = JSON.parse((await gunzip(await handler.readFile(`${rootPath}/cache.json.gz`))).toString())
+  assert.equal(Object.keys(cache).length, 1)
+  assert.equal(Object.values(cache)[0].size, metadata.size, 'cache entry should carry the post-merge size')
+})
+
+test('it regenerates a pre-existing cache when its entry count is out of sync, even with nothing to merge/remove', async () => {
+  // a referenced disk + its metadata, and a pre-existing cache with the wrong entry count
+  await generateVhd(`${basePath}/diskA1.vhd`)
+  await handler.writeFile(
+    `${rootPath}/metadata.json`,
+    JSON.stringify({ mode: 'delta', vhds: [`${relativePath}/diskA1.vhd`] })
+  )
+  await handler.writeFile(`${rootPath}/cache.json.gz`, await gzip(JSON.stringify({})))
+
   const logged = []
   // remove/merge both false: cacheNeedsRegen stays false and no merge happens,
   // so only the cache-count mismatch can trigger the regeneration
@@ -857,3 +932,53 @@ test('it regenerates the cache when its entry count is out of sync, even with no
   const cache = JSON.parse((await gunzip(await handler.readFile(`${rootPath}/cache.json.gz`))).toString())
   assert.equal(Object.keys(cache).length, 1, 'cache should be regenerated to match the archives on disk')
 })
+
+test('it regenerates a pre-existing cache after a metadata removal', async () => {
+  // two backups, each on its own VDI directory, so removing one does not touch the other's disks
+  await generateVhd(`${basePath}/diskA1.vhd`)
+  await handler.writeFile(
+    `${rootPath}/metadata1.json`,
+    JSON.stringify({ mode: 'delta', vhds: [`${relativePath}/diskA1.vhd`] })
+  )
+  // metadata2 references a vhd missing from disk: clean() removes it
+  await handler.writeFile(
+    `${rootPath}/metadata2.json`,
+    JSON.stringify({ mode: 'delta', vhds: [`${relativePath2}/deleted.vhd`] })
+  )
+
+  // a cache with the expected number of entries: only the removal can trigger the regeneration
+  await handler.writeFile(
+    `${rootPath}/cache.json.gz`,
+    await gzip(
+      JSON.stringify({
+        [`/${rootPath}metadata1.json`]: { stale: true },
+        [`/${rootPath}metadata2.json`]: { stale: true },
+      })
+    )
+  )
+
+  const logged = []
+  await VmBackupDirectory.cleanVm(handler, rootPath, {
+    remove: true,
+    merge: false,
+    logInfo: () => {},
+    logWarn: message => logged.push(message),
+  })
+
+  const rootFiles = await handler.list(rootPath)
+  assert.ok(rootFiles.includes('metadata1.json'), 'the complete backup must survive')
+  assert.ok(!rootFiles.includes('metadata2.json'), 'the backup with a missing vhd must be removed')
+  assert.ok(rootFiles.includes('cache.json.gz'), 'an existing cache.json.gz must not be dropped by a remove')
+  assert.ok(
+    !logged.includes('unexpected number of entries in backup cache'),
+    'the cache matched the archives before the clean: the removal alone must trigger the regeneration'
+  )
+
+  const cache = JSON.parse((await gunzip(await handler.readFile(`${rootPath}/cache.json.gz`))).toString())
+  assert.deepEqual(
+    Object.keys(cache),
+    [`/${rootPath}metadata1.json`],
+    'the removed backup must be gone from the regenerated cache'
+  )
+  assert.equal(cache[`/${rootPath}metadata1.json`].stale, undefined, 'the entry must come from the archive on disk')
+})

@xen-orchestra/backup-archive/src/VmIncrementalBackupArchive.mts

@@ -5,10 +5,27 @@ import {
   ResolvedBackupCleanOptions,
   VmBackupInterface,
   PartialBackupMetadata,
+  DEFAULT_REMOVE_CONCURRENCY,
 } from './VmBackup.types.mjs'
 import { RemoteHandlerAbstract } from '@xen-orchestra/fs'
 import { basename, dirname, normalize } from '@xen-orchestra/fs/path'
 import { RemoteDiskLineage } from './RemoteDiskLineage.mjs'
+import { asyncEach } from '@vates/async-each'
+
+/**
+ * Deletes a delta VM backup's metadata json. Unused VHDs are detected and removed
+ * separately by VmBackupDirectory's clean(). Shared with the legacy @xen-orchestra/backups
+ * RemoteAdapter.
+ */
+export async function deleteDeltaVmBackups(
+  handler: RemoteHandlerAbstract,
+  backups: Array<{ metadataPath: string }>
+): Promise<void> {
+  await asyncEach(backups, ({ metadataPath }) => handler.unlink(metadataPath), {
+    concurrency: DEFAULT_REMOVE_CONCURRENCY,
+    stopOnError: false,
+  })
+}
 
 export class VmIncrementalBackupArchive implements VmBackupInterface {
   handler: RemoteHandlerAbstract

@xen-orchestra/backup-archive/src/_unlinkTolerant.mts

@@ -0,0 +1,20 @@
+import { RemoteHandlerAbstract } from '@xen-orchestra/fs'
+
+/**
+ * Deletes a file, ignoring the given error codes.
+ * By default only a missing file is tolerated.
+ */
+export async function unlinkTolerant(
+  handler: RemoteHandlerAbstract,
+  path: string,
+  ignoredCodes: string[] = ['ENOENT']
+): Promise<void> {
+  try {
+    await handler.unlink(path)
+  } catch (error) {
+    if (!ignoredCodes.includes(error?.code)) {
+      error.path ??= path
+      throw error
+    }
+  }
+}

@xen-orchestra/backup-archive/src/index.mts

@@ -1 +1,5 @@
 export { VmBackupDirectory } from './VmBackupDirectory.mjs'
+export { deleteFullVmBackups } from './VmFullBackupArchive.mjs'
+export { deleteDeltaVmBackups } from './VmIncrementalBackupArchive.mjs'
+export { deleteMetadataBackup } from './MetadataBackup.mjs'
+export { readBackupCache, writeBackupCache } from './BackupCache.mjs'

@xen-orchestra/backup-archive/tsconfig.json

@@ -14,5 +14,6 @@
     "types": ["node"] // Only use types from Node.js and packages that are explicitly imported
   },
   "include": ["src/**/*"],
-  "exclude": ["node_modules", "dist"]
+  // *.integ.mjs deliberately imports from the built dist/ output to test it
+  "exclude": ["node_modules", "dist", "**/*.integ.mjs"]
 }

@xen-orchestra/backups/RemoteAdapter.mjs

@@ -6,14 +6,19 @@ import { decorateMethodsWith } from '@vates/decorate-with'
 import { dirname, join, resolve } from 'node:path'
 import { synchronized } from 'decorator-synchronized'
 import Disposable from 'promise-toolbox/Disposable'
-import fromCallback from 'promise-toolbox/fromCallback'
 import groupBy from 'lodash/groupBy.js'
 import pickBy from 'lodash/pickBy.js'
 import reduce from 'lodash/reduce.js'
-import zlib from 'zlib'
 
 import { BACKUP_DIR } from './_getVmBackupDir.mjs'
-import { VmBackupDirectory } from '@xen-orchestra/backup-archive'
+import {
+  VmBackupDirectory,
+  deleteFullVmBackups as deleteFullVmBackupFiles,
+  deleteDeltaVmBackups as deleteDeltaVmBackupFiles,
+  deleteMetadataBackup as deleteMetadataBackupFiles,
+  readBackupCache,
+  writeBackupCache,
+} from '@xen-orchestra/backup-archive'
 import { fileRestoreDecorators, fileRestoreMethods } from './_fileRestore.mjs'
 import { formatFilenameDate } from './_filenameDate.mjs'
 import { isMetadataFile } from './_backupType.mjs'
@@ -36,8 +41,6 @@ export const compareTimestamp = (a, b) => a.timestamp - b.timestamp
 
 const noop = Function.prototype
 
-const resolveRelativeFromFile = (file, path) => resolve('/', dirname(file), path).slice(1)
-
 const createSafeReaddir = (handler, methodName) => (path, options) =>
   handler.list(path, options).catch(error => {
     if (error?.code !== 'ENOENT') {
@@ -100,24 +103,17 @@ export class RemoteAdapter {
   }
 
   async deleteDeltaVmBackups(backups) {
-    const handler = this._handler
-
     // this will delete the json, unused VHDs will be detected by `cleanVm`
-    await asyncMapSettled(backups, ({ _filename }) => handler.unlink(_filename))
+    await deleteDeltaVmBackupFiles(
+      this._handler,
+      backups.map(({ _filename }) => ({ metadataPath: _filename }))
+    )
 
     await this.#removeVmBackupsFromCache(backups)
   }
 
   async deleteMetadataBackup(backupId) {
-    const uuidReg = '\\w{8}(-\\w{4}){3}-\\w{12}'
-    const metadataDirReg = 'xo-(config|pool-metadata)-backups'
-    const timestampReg = '\\d{8}T\\d{6}Z'
-    const regexp = new RegExp(`^${metadataDirReg}/${uuidReg}(/${uuidReg})?/${timestampReg}`)
-    if (!regexp.test(backupId)) {
-      throw new Error(`The id (${backupId}) not correspond to a metadata folder`)
-    }
-
-    await this._handler.rmtree(backupId)
+    await deleteMetadataBackupFiles(this._handler, backupId)
   }
 
   async deleteOldMetadataBackups(dir, retention) {
@@ -129,24 +125,10 @@ export class RemoteAdapter {
   }
 
   async deleteFullVmBackups(backups) {
-    const handler = this._handler
-    await asyncMapSettled(backups, ({ _filename, xva }) =>
-      Promise.all([
-        handler.unlink(_filename).catch(error => {
-          warn('error while removing full vm backup metadata', { error, filename: _filename })
-          if (error.code !== 'ENOENT') throw error
-        }),
-        handler.unlink(resolveRelativeFromFile(_filename, xva)).catch(error => {
-          warn('error while removing full vm backup file', { error, filename: _filename })
-          if (error.code !== 'ENOENT') throw error
-        }),
-        handler.unlink(resolveRelativeFromFile(_filename, `${xva}.checksum`)).catch(error => {
-          // checksum can be missing , it's not an issue
-          if (error.code !== 'ENOENT') throw error
-        }),
-      ])
+    await deleteFullVmBackupFiles(
+      this._handler,
+      backups.map(({ _filename, xva }) => ({ metadataPath: _filename, xva }))
     )
-
     await this.#removeVmBackupsFromCache(backups)
   }
 
@@ -298,13 +280,7 @@ export class RemoteAdapter {
   }
 
   async _readCache(path) {
-    try {
-      return JSON.parse(await fromCallback(zlib.gunzip, await this.handler.readFile(path)))
-    } catch (error) {
-      if (error.code !== 'ENOENT') {
-        warn('#readCache', { error, path })
-      }
-    }
+    return readBackupCache(this.handler, path, warn)
   }
 
   _updateCache = synchronized.withKey()(this._updateCache)
@@ -324,11 +300,7 @@ export class RemoteAdapter {
   }
 
   async _writeCache(path, data) {
-    try {
-      await this.handler.writeFile(path, await fromCallback(zlib.gzip, JSON.stringify(data)), { flags: 'w' })
-    } catch (error) {
-      warn('#writeCache', { error, path })
-    }
+    return writeBackupCache(this.handler, path, data, warn)
   }
 
   async #getCacheableDataListVmBackups(dir) {

@xen-orchestra/backups/_cleanVm.integ.mjs

@@ -230,7 +230,6 @@ test('it merges delta of non destroyed chain', async () => {
     logged.push(message)
   }
   await adapter.cleanVm(rootPath, { remove: true, logInfo, logWarn: logInfo, lock: false })
-  assert.equal(logged[0], `unexpected number of entries in backup cache`)
 
   logged = []
   const result = await adapter.cleanVm(rootPath, { remove: true, merge: true, logInfo, logWarn: () => {}, lock: false })

CHANGELOG.unreleased.md

@@ -21,6 +21,7 @@
 
 - [Web-core] Fix "console offline" illustration sparks color (PR [#10309](https://github.com/vatesfr/xen-orchestra/pull/10309))
 - [Web-core] Fix 404 illustration color (PR [#10325](https://github.com/vatesfr/xen-orchestra/pull/10325))
+- [Backup-archive] No longer create a `cache.json.gz` file on immutable/S3 remote during cleanup, which could not be deleted afterwards and stayed billed forever (PR [#10243](https://github.com/vatesfr/xen-orchestra/pull/10243))
 
 ### Packages to release
 
@@ -38,6 +39,8 @@
 
 <!--packages-start-->
 
+- @xen-orchestra/backup-archive patch
+- @xen-orchestra/backups patch
 - @xen-orchestra/web minor
 - @xen-orchestra/web-core minor