Conversation
|
/verify |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
Walkthrough
ChangesApplication installation
Priority: ➖ Normal Merge Risk: 🟠 High · up to The change adds staged installs with rollback, but a failure late in installation can remove the app's persisted user data. This happens because rollback deletes the version directory while persist links still point into the user's data. Staging-name collisions and unhandled rename errors also remain possible. These issues should be fixed before merging. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
f960190b-7367-41c0-9431-fc790993c5c9
📒 Files selected for processing (1)
lib/install.ps1
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| $staging_dir = "$target_dir.staging" | ||
| if (Test-Path $staging_dir) { Remove-Item $staging_dir -Recurse -Force -ErrorAction SilentlyContinue } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep staging names outside the version namespace.
The version check permits 1.staging. If that version is installed, installing version 1 makes $staging_dir point to its installed directory. Line 50 then deletes that installation. Use a separate staging location that cannot name an installed version.
| $original_dir = $dir # keep reference to real (not linked) directory | ||
| $target_dir = versiondir $app $version $global | ||
| $staging_dir = "$target_dir.staging" | ||
| if (Test-Path $staging_dir) { Remove-Item $staging_dir -Recurse -Force -ErrorAction SilentlyContinue } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stop if stale staging cleanup fails.
SilentlyContinue hides a failed deletion, including one caused by a file in use. ensure can then reuse the remaining directory, and the later rename can install files left by the previous attempt. Put cleanup inside the failure-handling path and require it to succeed before download begins.
| Invoke-HookScript -HookType 'pre_install' -Manifest $manifest -ProcessorArchitecture $architecture | ||
|
|
||
| # Atomic swap from staging to final version dir | ||
| if (Test-Path $target_dir) { Remove-Item $target_dir -Recurse -Force } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve the existing version until replacement succeeds.
When $target_dir exists, this command deletes the working installation before Rename-Item succeeds. If the rename fails, catch removes staging but cannot restore the old version. Keep a backup of the existing directory and restore it on replacement failure; remove the backup only after the new directory is in place.
|
|
||
| # Atomic swap from staging to final version dir | ||
| if (Test-Path $target_dir) { Remove-Item $target_dir -Recurse -Force } | ||
| Rename-Item -Path $staging_dir -NewName (Split-Path $target_dir -Leaf) -Force |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make rename failure enter catch.
A non-terminating Rename-Item error does not enter this catch by default. If the rename fails, execution can set $dir to the target and continue installation even though staging remains. Use -ErrorAction Stop on the rename and other required filesystem operations in this block. PowerShell documents that catch handles terminating errors, while -ErrorAction Stop escalates non-terminating errors. (learn.microsoft.com)
33da926 to
157bdb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
071e8832-bc86-4726-a039-780dcf1c0bb4
📒 Files selected for processing (2)
lib/install.ps1test/Scoop-Install.Tests.ps1
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| if ($target_promoted -and (Test-Path $target_dir)) { | ||
| try { unlink_current $target_dir | Out-Null } catch { } | ||
| try { Remove-Item $target_dir -Recurse -Force -ErrorAction Stop } catch { } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove persist links before rollback deletes $target_dir.
persist_data runs at Line 88. It creates junctions and hard links inside $target_dir that point into $persist_dir. Any failure after Line 88 runs this rollback; for example, a post_install hook error or a save_installed_manifest error. The rollback then runs Remove-Item $target_dir -Recurse -Force.
In Windows PowerShell 5.1, Remove-Item -Recurse follows directory junctions and deletes the target contents. The rollback can therefore delete the user's persisted data in persist\<app>. For this reason, the uninstall flow calls unlink_persist_data before it removes a version directory.
Call unlink_persist_data before the recursive delete. Do not ignore its failure. If the unlink fails, skip the delete so that the persist store keeps its data.
🐛 Proposed fix
if ($target_promoted -and (Test-Path $target_dir)) {
try { unlink_current $target_dir | Out-Null } catch { }
- try { Remove-Item $target_dir -Recurse -Force -ErrorAction Stop } catch { }
+ $unlinked = $true
+ try { unlink_persist_data $manifest $target_dir } catch { $unlinked = $false }
+ if ($unlinked) {
+ try { Remove-Item $target_dir -Recurse -Force -ErrorAction Stop } catch { }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($target_promoted -and (Test-Path $target_dir)) { | |
| try { unlink_current $target_dir | Out-Null } catch { } | |
| try { Remove-Item $target_dir -Recurse -Force -ErrorAction Stop } catch { } | |
| } | |
| if ($target_promoted -and (Test-Path $target_dir)) { | |
| try { unlink_current $target_dir | Out-Null } catch { } | |
| $unlinked = $true | |
| try { unlink_persist_data $manifest $target_dir } catch { $unlinked = $false } | |
| if ($unlinked) { | |
| try { Remove-Item $target_dir -Recurse -Force -ErrorAction Stop } catch { } | |
| } | |
| } |
157bdb3 to
87acc25
Compare
… restore side-effects on backup recovery
| $unlinked = $true | ||
| try { unlink_persist_data $manifest $target_dir } catch { $unlinked = $false } | ||
| if ($unlinked) { | ||
| Remove-Item $target_dir -Recurse -Force -ErrorAction Stop |
There was a problem hiding this comment.
When a retry finds both a target and a stranded backup, recovery removes the app's current link before deleting the target. If a locked file prevents that deletion, the error stops installation before the backup is restored. The previously working installation is left unlinked, with its backup stranded.
| link_current $target_dir | Out-Null | ||
| create_shims $manifest $target_dir $global $architecture | ||
| create_startmenu_shortcuts $manifest $target_dir $global $architecture | ||
| env_add_path $manifest $target_dir $global $architecture |
There was a problem hiding this comment.
If installation fails after promotion, rollback restores the backup but uses the failed installation's manifest to add PATH entries for it. When the restored version has different PATH settings, the user is left with entries that did not belong to that version and that its eventual uninstall will not know to remove.
This branch has not been deployed
1 participant
📋 Problème Traité
Dans
lib/install.ps1, la fonctioninstall_appcrée immédiatement le dossier définitif de version de l'application sur le disque (apps/<app>/<version>) avant d'exécuter le téléchargement, la décompression, les hooks de pré-installation et la création des shims :Conséquences en cas d'échec :
pre_installen erreur ou l'exécutable introuvable pour les shims, Scoop s'arrête brutalement (abort).failedoupartially installed, forçant l'utilisateur à investiguer et exécuterscoop uninstall -p <app>manuellement.💡 Solution Apportée
Mise en place d'un pattern d'installation transactionnelle avec zone de staging et rollback automatique :
pre_install) sont isolées dans un répertoire de staging temporaire (apps/<app>/<version>.staging).catchsupprime immédiatement et intégralement le dossier de staging sans laisser la moindre trace sur le disque.Rename-Item) promeut le dossier de staging vers le chemin de version définitif (apps/<app>/<version>).🔗 Issue Associée
Amélioration de la résilience et de la fiabilité des installations d'applications Scoop.
✅ Validation & Actions Réalisées
Note
persist/) et de gestion des permissions.fix(install): implement transactional staging and automatic rollback on failure🧪 Reproduction de l'Erreur
Script de démonstration du problème legacy (dossier corrompu restant sur disque) :
🔬 Test de la Correction
Script validant le comportement transactionnel et l'absence de résidu corrompu :
The PR does not appear safe to merge while a failed recovery can unlink the working installation and rollback can retain PATH entries from a failed install.
Findings
Summary
The PR stages downloads and extraction, promotes the staged directory, and adds recovery and rollback handling for interrupted installations.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Begin installation] --> B{Stranded backup?} B -- Yes --> C[Attempt to restore backup and current link] B -- No --> D[Download and extract into staging] C --> D D --> E[Promote staging to target] E --> F{Remaining installation succeeds?} F -- Yes --> G[Remove backup] F -- No --> H[Remove failed target and restore backup] H --> I[Recreate shims, shortcuts, and PATH]Reviews (7) · Last reviewed commit: "fix(install): safely unlink persist data..."