feat(registry): implement automatic registry rollback on execution failure - #613
feat(registry): implement automatic registry rollback on execution failure#613HetCreep wants to merge 3 commits into
Conversation
|
Heya, I do like this idea, I'll take a look at this soon when I have time. Thanks! |
8b5df67 to
328a6e3
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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:
📝 WalkthroughWalkthrough
ChangesAutomatic Registry Rollback on Execution Failure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Scripts/Features/ExecuteChanges.ps1`:
- Around line 206-252: The ImportRegistryFile function call within the undo
operations section records failures in $script:RegistryImportFailures without
throwing an exception, which prevents the catch block from executing and rolling
back earlier registry writes. To fix this, either modify ImportRegistryFile to
throw an exception when it increments the failure counter, or add a check inside
the try block after the ImportRegistryFile call to throw an exception if
$script:RegistryImportFailures has been incremented. This ensures that registry
import failures trigger the catch block for proper rollback handling.
- Line 255: The Test-Path check for the backup file on line 255 needs to use the
-LiteralPath parameter instead of treating the path as a pattern. Change the
condition that checks "if ($backupFile -and (Test-Path $backupFile))" to use
"-LiteralPath $backupFile" in the Test-Path call, which will prevent wildcard
characters like brackets from being interpreted as patterns and ensure the
rollback attempt is not skipped for valid paths containing these characters.
This should match the approach already used in the Load-RegistryBackupFromFile
function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45c65df7-0016-44ac-a8f1-82e3cb495fe3
📒 Files selected for processing (1)
Scripts/Features/ExecuteChanges.ps1
71ef6c2 to
d4a57cb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Scripts/Features/ExecuteChanges.ps1 (1)
287-290: 💤 Low valueDead code: this warning block is now unreachable.
With the new failure detection at lines 242-244 and 266-268 throwing when
$script:RegistryImportFailuresincreases, this block can never execute with a non-zero failure count:
- If failures occur → exception thrown → function exits before reaching here
- If no failures →
$script:RegistryImportFailuresremains 0 → condition is falseConsider removing this block since failure messaging is now handled by the exception path.
♻️ Suggested removal
} throw } - -if ($script:RegistryImportFailures -gt 0) { - Write-Host "" - Write-Host "$($script:RegistryImportFailures) registry import change(s) failed. See output above for details." -ForegroundColor Yellow -} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Scripts/Features/ExecuteChanges.ps1` around lines 287 - 290, The warning block that checks if $script:RegistryImportFailures is greater than 0 is now unreachable dead code. With the new failure detection logic that throws exceptions when $script:RegistryImportFailures increases (added in the preceding validation blocks), the function will exit via exception before reaching this warning block, so the condition can never be true. Remove the entire if block (the condition check and the Write-Host statements within it) since failure messaging is now handled by the exception throwing paths in the earlier validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Scripts/Features/ExecuteChanges.ps1`:
- Around line 287-290: The warning block that checks if
$script:RegistryImportFailures is greater than 0 is now unreachable dead code.
With the new failure detection logic that throws exceptions when
$script:RegistryImportFailures increases (added in the preceding validation
blocks), the function will exit via exception before reaching this warning
block, so the condition can never be true. Remove the entire if block (the
condition check and the Write-Host statements within it) since failure messaging
is now handled by the exception throwing paths in the earlier validation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf61878b-3d83-4033-aead-398f21c3b0be
📒 Files selected for processing (1)
Scripts/Features/ExecuteChanges.ps1
8352468 to
7af95d8
Compare
|
Heads-up on the merge conflict here: #641 refactored Since it's the registry-rollback safety path and I can't run-test the restore on my end, I'd rather not force-push an untested rewrite of it. Would you prefer I redo it against the new structure (you'd want to verify the rollback/restore path before merge), or would you rather fold it into your own refactor? Either works for me -- just didn't want to decide that for you. |
Heya, That's a fair assessment. If you'd be open to redoing it that would be great. I do apologize for the issues caused by the recent refactoring going on with the script, it was something that was sorely needed to keep things clear and well structured. As this feature is quite impactful I wasn't satisfied yet to merge it before fully testing. |
7af95d8 to
26b3133
Compare
|
Redone against the new structure, as discussed. The rollback now lives in It's parse-clean, but I haven't run-tested the restore path on my end (no debloat run here), so please verify the rollback/restore behaviour before merge, as you mentioned. (Apologies for the brief close/reopen above -- a force-push hiccup on my side while resetting the branch onto the new structure.) |
32d2ceb to
8e56d0d
Compare
|
Reviewed this against the registry restore/import code paths it touches (I was just working on #686 / #704 in the restore path, which is directly relevant here). Two grounded findings, one blocking-ish and one informational: 1. The failure-detection trigger has a silent blind spot for partial access-denial. The rollback only fires when 2. The rollback mechanism itself reuses a currently-unsafe restore path.
Neither of these is necessarily a reason to block — #613's overall direction (backup before apply, roll back on detected failure) is a real safety improvement over the status quo of no rollback at all — but I wanted to flag both gaps explicitly since they're easy to miss without having just traced through the restore path. |
8e56d0d to
b3cc180
Compare
b3cc180 to
0a934cf
Compare
|
Rebased onto current master: re-applied the rollback onto the renamed Invoke-Changes.ps1 (post #708), switched the restore call to the renamed Import-RegistryBackup, and preserved the #700 cancellation check between the apply and undo phases (a cancel returns without triggering rollback, since cancellation is not a failure). Parse-clean; I could not run the new Pester suite locally (in-box Pester 3.4, the suite needs v5 and I avoid installing tools on this machine) -- the tests.yml workflow should exercise it on this PR. On STiFLeR7's two findings above -- both are accurate, and worth stating how they relate to this PR's scope: (1) the partial-access-denial blind spot lives in Invoke-RegistryOperationsFromRegFile's throw-only-on-total-denial contract, so the right fix is surfacing the denied count from that helper (which would then flow through this PR's failure detection for free) rather than widening this PR's trigger heuristically; (2) the rollback path inheriting #686's delete-before-rewrite exposure is real -- with #704 merged, the corrupted-backup trigger is now validated out, and since the backup here is freshly tool-generated in the same run, the residual risk is limited to a runtime SetValue failure mid-rollback (narrow, but real until #686's case 2 gets a mitigation design). Both feel like follow-ups at the restore-layer level rather than blockers for adding rollback-on-failure at all, but that's your call. |
0a934cf to
d908df8
Compare
|
Thanks @STiFLeR7 and @HetCreep! I think the second issue should be fixed now as of #710, the script should now handle those restores much more conservatively and with some extra validation, it no longer deletes keys that could potentially not get restored if something fails along the way. I will fully review this when I have more time this week. |
|
This generally looks fine from a technical perspective, although I am worried a rollback to the backup might fail for the same reason the original apply action failed. Looking at it from that perspective this might not really add much. I also don't think we should automatically rollback any changes without prompting the user, to let them choose. What are your thoughts on that? |
|
Heya, Both points land -- pivoted this to match. On the rollback-reliability point: confirmed by reading through the restore path ( (Aside, not blocking this PR: while tracing that path I noticed On the "don't auto-revert without asking" point: agreed, and it also sidesteps the reliability problem above entirely -- no rollback is attempted, so there's nothing to fail. On a registry import failure the script now reports the backup file location and points to the existing Restore Backup option instead of touching the registry again on its own. Pushed ( |
Reimplemented against the refactored InvokeChanges structure (Raphire#641): execute/undo loops are now Invoke-ApplyFeatures / Invoke-UndoFeatures called from Invoke-AllChanges. Capture the backup path from New-RegistrySettingsBackup, wrap the apply and undo phases in a try/catch, throw when RegistryImportFailures increases, and on failure restore the pre-execution state via Load-RegistryBackupFromFile + Restore-RegistryBackupState before rethrowing. WhatIf unaffected (no backup created, so no rollback). Implements Raphire#612
The 'reports registry import failures after all requested work completes' spec pinned the pre-rollback behavior (failures let the run complete and print a summary). With automatic rollback, a phase whose imports fail now throws, rolls back to the pre-execution backup, and rethrows -- so that spec is replaced with three covering the new contract: the throw on apply-phase failure (undo phase not entered), the rollback path (Import-RegistryBackup + Restore-RegistryBackupState invoked with the captured backup file), and the no-backup case (SkipRegistryBackup set -> no rollback attempted).
An unattended rollback can hit the same access-denied failure that broke the apply in the first place, so it isn't a reliable safety net for the failure mode it exists to handle. Reverting registry state without asking is also not a call the script should make on the user's behalf. On failure, point to the pre-execution backup and the existing Restore Backup option instead of attempting the revert automatically.
17abdf5 to
c96bd8e
Compare
3 participants
Resolves #612
Changes
Summary by CodeRabbit
New-RegistrySettingsBackup, wrapping the “execute actionable parameters” and “execute undo operations” loops in atry/catch, and triggering an automatic rollback viaRestore-RegistryBackupStatewhen parameter execution fails or when$script:RegistryImportFailuresincreases during either phase; rollback errors are logged and the original failure is re-thrown.