From 193a6746e71517103a89afc71c2117387d11de43 Mon Sep 17 00:00:00 2001 From: Kim T Date: Thu, 30 Jul 2026 21:50:46 -0700 Subject: [PATCH] [fix] Roll back partially-installed files when install() fails partway through install() had no transactional boundary across a package version's files: if file N of M failed its hash check (or any later step), the N-1 files already moved into the live plugins/presets/projects directory stayed there. isPackageInstalled()/scan() have no way to tell a partial, broken install apart from a complete one - a version directory existing is the only signal either one checks - so a failed install could silently look installed while actually missing files. Track every directory this call populates under typeDir and, on any thrown error from the per-file loop, delete them all before re-throwing. Documented the same rollback requirement in specification.md's Install logic so independently-built managers handle a mid-install failure the same way. --- specification.md | 2 +- src/classes/ManagerLocal.ts | 240 ++++++++++++++++------------- tests/classes/ManagerLocal.test.ts | 61 ++++++++ 3 files changed, 194 insertions(+), 109 deletions(-) diff --git a/specification.md b/specification.md index 55044bc..e4158f3 100644 --- a/specification.md +++ b/specification.md @@ -414,7 +414,7 @@ Install a package by slug. Optionally including a version. 2. Otherwise (only installer-type files are available), if the program does not have Admin privileges, ask for elevated privileges to the filesystem before continuing 5. Download each matching file to a temporary directory. 6. Check the hash against the metadata sha256. - 1. If hash and sha256 do not match, return error + 1. If hash and sha256 do not match, remove any files/directories already moved into their final destination by an earlier iteration of steps 5-9 for this same install call, then return error. A package version's files should install as a single unit - a manager should never leave a package version partially installed (e.g. some of its files present in the final plugin/preset/project directory, others missing), since a later [scan](#scan-logic) would otherwise have no way to tell a partial, broken install apart from a complete one. 7. Check if the file type is installer 1. Run the installer process and wait for it to complete before continuing 2. When the process ends, run a local package scan to see if the installation finished correctly. diff --git a/src/classes/ManagerLocal.ts b/src/classes/ManagerLocal.ts index b944b4e..30cfb64 100644 --- a/src/classes/ManagerLocal.ts +++ b/src/classes/ManagerLocal.ts @@ -330,128 +330,152 @@ export class ManagerLocal extends Manager { versionNum, ); dirCreate(dirDownloads); - for (const key in files) { - // Download file to temporary directory if not already downloaded. - const file: FileInterface = files[key]; - const filePath: string = path.join(dirDownloads, path.basename(file.url)); - if (!fileExists(filePath)) { - const fileBuffer: ArrayBuffer = await apiBuffer(file.url); - fileCreate(filePath, Buffer.from(fileBuffer)); - } - - // Check file hash matches expected hash. - const hash: string = await fileHash(filePath); - if (hash !== file.sha256) throw new Error(`${filePath} hash mismatch`); - - // If installer, run the installer headless (without the user interface). - if (file.type === FileType.Installer) { - // Test time out if installing during tests. - if (isTests()) fileOpen(filePath); - else fileInstall(filePath); - // Currently we don't get a list of paths from the installer. - // Create empty directory and save package version information. - // Installers have to be manually uninstalled for now. - const dirTarget: string = path.join(this.typeDir, 'Installers', slug, versionNum); - dirCreate(dirTarget); - fileCreateJson(path.join(dirTarget, 'index.json'), pkgVersion); - } + // Every directory this call populates under `this.typeDir` (the live, user-facing install + // location that isPackageInstalled()/scan() look at) - tracked so that if a later file in + // this loop fails (hash mismatch, extraction error, ...), everything already installed for + // this call can be rolled back instead of leaving a partial install that looks installed but + // is actually missing files. Downloads/extraction happen in scratch temp directories outside + // typeDir and are deliberately left alone - see the download-caching note in + // ManagerLocal.test.ts. + const installedDirs = new Set(); + try { + for (const key in files) { + // Download file to temporary directory if not already downloaded. + const file: FileInterface = files[key]; + const filePath: string = path.join(dirDownloads, path.basename(file.url)); + if (!fileExists(filePath)) { + const fileBuffer: ArrayBuffer = await apiBuffer(file.url); + fileCreate(filePath, Buffer.from(fileBuffer)); + } - // If archive, extract the archive to temporary directory, then move individual files. - if (file.type === FileType.Archive) { - const dirSource: string = path.join( - this.config.get('appDir') as string, - file.type, - this.type, - slug, - versionNum, - ); - const dirSub: string = path.join(slug, versionNum); - let formatDir: Record = pluginFormatDir; - if (this.type === RegistryType.Apps) formatDir = pluginFormatDir; - else if (this.type === RegistryType.Presets) formatDir = presetFormatDir; - else if (this.type === RegistryType.Projects) formatDir = projectFormatDir; - await archiveExtract(filePath, dirSource); - - // Move entire directory, maintaining the same folder structure. - if (pkgVersion.type === PluginType.Sampler) { - const dirTarget: string = path.join(this.typeDir, 'Samplers', dirSub); + // Check file hash matches expected hash. + const hash: string = await fileHash(filePath); + if (hash !== file.sha256) throw new Error(`${filePath} hash mismatch`); + + // If installer, run the installer headless (without the user interface). + if (file.type === FileType.Installer) { + // Test time out if installing during tests. + if (isTests()) fileOpen(filePath); + else fileInstall(filePath); + // Currently we don't get a list of paths from the installer. + // Create empty directory and save package version information. + // Installers have to be manually uninstalled for now. + const dirTarget: string = path.join(this.typeDir, 'Installers', slug, versionNum); dirCreate(dirTarget); - dirMove(dirSource, dirTarget); fileCreateJson(path.join(dirTarget, 'index.json'), pkgVersion); - } else { - // Check if archive contains installer files (pkg, dmg) that should be run - const allFiles = dirRead(`${dirSource}/**/*`).filter(f => !dirIs(f)); - const installerFiles = allFiles.filter(f => { - const ext = path.extname(f).toLowerCase(); - return ext === '.pkg' || ext === '.dmg'; - }); - - if (installerFiles.length > 0) { - // Run installer files found in archive - for (const installerFile of installerFiles) { - if (isTests()) fileOpen(installerFile); - else fileInstall(installerFile); - } - // Create directory and save package info for installer - const dirTarget: string = path.join(this.typeDir, 'Installers', dirSub); - dirCreate(dirTarget); - fileCreateJson(path.join(dirTarget, 'index.json'), pkgVersion); - } else if (this.type === RegistryType.Plugins) { - // For plugins, move files into type-specific subdirectories - const filesMoved: string[] = filesMove(dirSource, this.typeDir, dirSub, formatDir); - if (filesMoved.length === 0) { - throw new Error(`No compatible files found to install for ${slug}`); - } - filesMoved.forEach((fileMoved: string) => { - const fileJson: string = path.join(path.dirname(fileMoved), 'index.json'); - fileCreateJson(fileJson, pkgVersion); - }); - } else { - // For apps/projects/presets, move entire directory without type subdirectories - const dirTarget: string = path.join(this.typeDir, dirSub); + installedDirs.add(dirTarget); + } + + // If archive, extract the archive to temporary directory, then move individual files. + if (file.type === FileType.Archive) { + const dirSource: string = path.join( + this.config.get('appDir') as string, + file.type, + this.type, + slug, + versionNum, + ); + const dirSub: string = path.join(slug, versionNum); + let formatDir: Record = pluginFormatDir; + if (this.type === RegistryType.Apps) formatDir = pluginFormatDir; + else if (this.type === RegistryType.Presets) formatDir = presetFormatDir; + else if (this.type === RegistryType.Projects) formatDir = projectFormatDir; + await archiveExtract(filePath, dirSource); + + // Move entire directory, maintaining the same folder structure. + if (pkgVersion.type === PluginType.Sampler) { + const dirTarget: string = path.join(this.typeDir, 'Samplers', dirSub); dirCreate(dirTarget); dirMove(dirSource, dirTarget); fileCreateJson(path.join(dirTarget, 'index.json'), pkgVersion); - // Ensure executable permissions for likely executables inside moved app/project/preset - try { - const movedFiles = dirRead(path.join(dirTarget, '**', '*')).filter(f => !dirIs(f)); - movedFiles.forEach((movedFile: string) => { - const ext = path.extname(movedFile).slice(1).toLowerCase(); - if (['', 'elf', 'exe'].includes(ext)) { - try { - fileExec(movedFile); - } catch (err) { - this.log(`Failed to set exec on ${movedFile}:`, err); - } - } + installedDirs.add(dirTarget); + } else { + // Check if archive contains installer files (pkg, dmg) that should be run + const allFiles = dirRead(`${dirSource}/**/*`).filter(f => !dirIs(f)); + const installerFiles = allFiles.filter(f => { + const ext = path.extname(f).toLowerCase(); + return ext === '.pkg' || ext === '.dmg'; + }); + + if (installerFiles.length > 0) { + // Run installer files found in archive + for (const installerFile of installerFiles) { + if (isTests()) fileOpen(installerFile); + else fileInstall(installerFile); + } + // Create directory and save package info for installer + const dirTarget: string = path.join(this.typeDir, 'Installers', dirSub); + dirCreate(dirTarget); + fileCreateJson(path.join(dirTarget, 'index.json'), pkgVersion); + installedDirs.add(dirTarget); + } else if (this.type === RegistryType.Plugins) { + // For plugins, move files into type-specific subdirectories + const filesMoved: string[] = filesMove(dirSource, this.typeDir, dirSub, formatDir); + if (filesMoved.length === 0) { + throw new Error(`No compatible files found to install for ${slug}`); + } + filesMoved.forEach((fileMoved: string) => { + const fileJson: string = path.join(path.dirname(fileMoved), 'index.json'); + fileCreateJson(fileJson, pkgVersion); + // A single archive can contain multiple formats (e.g. VST3 and CLAP), moved into + // different formatDir subdirectories - track each one, not just the first. + installedDirs.add(path.dirname(fileMoved)); }); - } catch (err) { - this.log('Error while setting executable permissions:', err); - } - // Also handle macOS .app bundles: set exec on binaries in Contents/MacOS - try { - const appDirs = dirRead(path.join(dirTarget, '**', '*.app')).filter(d => dirIs(d)); - appDirs.forEach((appDir: string) => { - try { - const macosBinPattern = path.join(appDir, 'Contents', 'MacOS', '**', '*'); - const macosFiles = dirRead(macosBinPattern).filter(f => !dirIs(f)); - macosFiles.forEach((binFile: string) => { + } else { + // For apps/projects/presets, move entire directory without type subdirectories + const dirTarget: string = path.join(this.typeDir, dirSub); + dirCreate(dirTarget); + dirMove(dirSource, dirTarget); + fileCreateJson(path.join(dirTarget, 'index.json'), pkgVersion); + installedDirs.add(dirTarget); + // Ensure executable permissions for likely executables inside moved app/project/preset + try { + const movedFiles = dirRead(path.join(dirTarget, '**', '*')).filter(f => !dirIs(f)); + movedFiles.forEach((movedFile: string) => { + const ext = path.extname(movedFile).slice(1).toLowerCase(); + if (['', 'elf', 'exe'].includes(ext)) { try { - fileExec(binFile); + fileExec(movedFile); } catch (err) { - this.log(`Failed to set exec on app binary ${binFile}:`, err); + this.log(`Failed to set exec on ${movedFile}:`, err); } - }); - } catch (err) { - this.log(`Error scanning .app contents for ${appDir}:`, err); - } - }); - } catch (err) { - this.log(err); + } + }); + } catch (err) { + this.log('Error while setting executable permissions:', err); + } + // Also handle macOS .app bundles: set exec on binaries in Contents/MacOS + try { + const appDirs = dirRead(path.join(dirTarget, '**', '*.app')).filter(d => dirIs(d)); + appDirs.forEach((appDir: string) => { + try { + const macosBinPattern = path.join(appDir, 'Contents', 'MacOS', '**', '*'); + const macosFiles = dirRead(macosBinPattern).filter(f => !dirIs(f)); + macosFiles.forEach((binFile: string) => { + try { + fileExec(binFile); + } catch (err) { + this.log(`Failed to set exec on app binary ${binFile}:`, err); + } + }); + } catch (err) { + this.log(`Error scanning .app contents for ${appDir}:`, err); + } + }); + } catch (err) { + this.log(err); + } } } } } + } catch (err) { + // Roll back everything this call already installed under typeDir before propagating - + // otherwise a package that failed partway through would be left looking installed (its + // version directory exists) while actually missing files, and neither isPackageInstalled() + // nor scan() would have any way to tell the difference. + for (const dir of installedDirs) dirDelete(dir); + throw err; } pkgVersion.installed = true; return pkgVersion; diff --git a/tests/classes/ManagerLocal.test.ts b/tests/classes/ManagerLocal.test.ts index 1443180..0bc5a0b 100644 --- a/tests/classes/ManagerLocal.test.ts +++ b/tests/classes/ManagerLocal.test.ts @@ -30,7 +30,9 @@ import { ConfigInterface } from '../../src/types/Config'; import { PackageVersion } from '../../src/types/Package'; import { Architecture } from '../../src/types/Architecture'; import { SystemType } from '../../src/types/SystemType'; +import { FileType } from '../../src/types/FileType'; import { mockRegistrySync, omitDownloads } from '../testUtils'; +import * as apiHelpers from '../../src/helpers/api'; const APP_DIR: string = 'test'; // Explicitly test-scoped rather than relying on Config.test.ts/ConfigLocal.test.ts to have @@ -249,6 +251,65 @@ test('Install installer-only package still elevates when unprivileged', async () runCliAsAdminSpy.mockRestore(); }); +test('Install rolls back already-installed files when a later file in the same version fails', async () => { + // Regression test for the "partial install" failure mode identified in review.md: a package + // version with two files, where the first installs successfully (creating and populating its + // target directory under typeDir) before the second fails its hash check. Without rollback, + // the first file's directory would be left behind, and isPackageInstalled()/scan() would treat + // this version as installed despite it actually being incomplete. + const slug = 'test-org/rollback-project'; + const versionNum = '1.0.0'; + const compatibleFile = { + architectures: [Architecture.Arm32, Architecture.Arm64, Architecture.X32, Architecture.X64], + size: 10, + systems: [{ type: SystemType.Linux }, { type: SystemType.Mac }, { type: SystemType.Win }], + type: FileType.Archive, + }; + const pkgVersion: PackageVersion = { + ...PROJECT, + files: [ + { ...compatibleFile, sha256: 'a'.repeat(64), url: 'https://example.invalid/rollback/file-one.zip' }, + { ...compatibleFile, sha256: 'b'.repeat(64), url: 'https://example.invalid/rollback/file-two.zip' }, + ], + }; + const pkg = new Package(slug); + pkg.addVersion(versionNum, pkgVersion); + + const manager = new ManagerLocal(RegistryType.Projects, CONFIG); + manager.addPackage(pkg); + + const apiBufferSpy = vi.spyOn(apiHelpers, 'apiBuffer').mockResolvedValue(new Uint8Array([1, 2, 3]).buffer); + // The first file's hash matches what's configured above, so it clears the check and proceeds + // to install; the second never will, regardless of content - this forces the failure to land + // on the *second* file, after the first has already been fully installed. + const fileHashSpy = vi + .spyOn(fileHelpers, 'fileHash') + .mockResolvedValueOnce('a'.repeat(64)) + .mockResolvedValueOnce('mismatched-hash'); + // archiveExtract() itself isn't under test here - stub it to just produce a small real + // directory, so the real (unmocked) dirMove()/dirRead() logic downstream has something genuine + // to move for the first file. + const archiveExtractSpy = vi + .spyOn(fileHelpers, 'archiveExtract') + .mockImplementation(async (_filePath: string, dirPath: string) => { + dirCreate(dirPath); + fileCreateJson(path.join(dirPath, 'dummy.json'), { ok: true }); + }); + + const dirTarget: string = path.join(CONFIG.projectsDir as string, slug, versionNum); + + try { + await expect(manager.install(slug, versionNum)).rejects.toThrow('hash mismatch'); + + expect(dirExists(dirTarget)).toEqual(false); + expect(manager.isPackageInstalled(slug, versionNum)).toEqual(false); + } finally { + apiBufferSpy.mockRestore(); + fileHashSpy.mockRestore(); + archiveExtractSpy.mockRestore(); + } +}); + test('Install all installs every listed package', async () => { const manager = new ManagerLocal(RegistryType.Plugins, CONFIG); // Seed a single known package directly rather than sync()'ing the live registry - installAll()