[fix] Roll back partially-installed files when install() fails partway through - #105
Merged
Merged
Conversation
3 tasks
…y 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.
kmturley
force-pushed
the
fix/install-transactional-rollback
branch
from
July 31, 2026 05:25
0ddc520 to
193a674
Compare
kmturley
added a commit
that referenced
this pull request
Jul 31, 2026
…gets.ts install() was a ~200-line method mixing download/hash/elevation orchestration with the logic for where each installed file's final destination is (installer marker dir, Sampler move, embedded .pkg/.dmg, format-sorted plugin files, flat app/preset/project move) - impossible to unit-test any one destination without exercising the whole pipeline. Move each destination into its own function in helpers/installTargets.ts, each taking an explicit params object and returning the directories it populated (which install() now just adds to its rollback-tracking Set from #105, rather than inlining `installedDirs.add(dirTarget)` at each of five call sites). Behavior-preserving except for one verified dead branch: the pre-refactor code selected `formatDir` from presetFormatDir/projectFormatDir when `this.type` was Presets/Projects, but that value was only ever read inside the `this.type === Plugins` branch - mutually exclusive with type being Presets/Projects for a single ManagerLocal instance - so those two branches could never actually be reached. Simplified to just pluginFormatDir directly. Added direct unit tests for each extracted destination in installTargets.test.ts (mocking only archiveExtract, not the full download/hash/elevation path) - install()'s own integration tests are unchanged and still pass, confirming the extraction didn't alter behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ManagerLocal.install()had no transactional boundary across a package version's files: the per-file loop downloads, hash-verifies, extracts, and moves each file into the livepluginsDir/presetsDir/projectsDirin sequence. If file N of M threw (hash mismatch, extraction error,filesMove()finding nothing compatible, ...), files1..N-1had already been moved into place - and stayed there.isPackageInstalled()only checks whether a directory namedslug/versionexists undertypeDir; it has no way to distinguish a complete install from a partial one. So a failed install could leave a package looking installed (to a laterinstall()early-return,scan(), orisPackageInstalled()check) while actually missing files - the plugin would then silently fail to load in the DAW with nothing pointing back to a botched install.this.typeDir(installedDirs: Set<string>) as each file successfully installs, wrap the per-file loop in try/catch, and on any thrown error delete every tracked directory before re-throwing. Scratch/temp directories (downloads cache, archive extraction) are deliberately left alone - they're outsidetypeDir, already an intentional cache (see thebeforeAllcomment inManagerLocal.test.ts), and not what makes a package look installed.typeDir) and the second fails its hash check, then asserts the first file's directory was rolled back andisPackageInstalled()returnsfalse. This test fails against the pre-fix code (verified locally by reverting theManagerLocal.tschange) with the leftover directory still present.specification.md's Install logic (step 6) to require the same rollback behavior, so independently-built managers handle a mid-install failure the same way instead of each inventing their own (or none).This is item 3 of the architectural review in
review.md(Critical Blocker #1), sequenced after items 1 (network timeout/retry, #103) and 2 (hermetic sync() tests, #104) specifically so this fix's own test suite could be trusted.Test plan
Install rolls back already-installed files when a later file in the same version fails- installs a 2-file package, second file fails its hash check, asserts the first file's target directory andisPackageInstalled()both reflect a clean rollback.ManagerLocal.tsrollback logic locally and confirmed the new test fails (leftover directory from the first file).npm run check(format, lint, build, test): 208/208 tests, 17/17 files pass.