fix: restore-point false success, updater asset match, WU COM leak#815
Merged
Conversation
Round 2d of the audit — three correctness/safety fixes around update paths: - PerformanceService.CreateRestorePointAsync returned true whenever the call did not throw, but Checkpoint-Computer fails non-terminating (e.g. once-per-24h rate limit), so silent failures were reported as success. It now forces a terminating error and only returns true on an explicit success sentinel. - UpdateService matched a fixed 'SysManager.exe' asset that no release publishes (real name: SysManager-v<version>.exe), so AssetUrl/AssetSize were always null. Replaced with IsMainExeAsset (versioned exe, excludes the .sha256 companion). - WindowsUpdateService.ScanAsync released COM objects only on the happy path, leaking them when cancellation/mapping threw mid-scan. Releases moved to finally. Tests: UpdateServiceAssetMatchTests (unit) + updated IntegrationTests Constants test now asserts IsMainExeAsset instead of the removed AssetName const. Full solution regression build clean across all four projects.
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.
Round 2d of the audit — three correctness/safety fixes on the update paths.
Restore point reported false success (P1)
CreateRestorePointAsyncreturnedtruewhenever the PowerShell call didn't throw. ButCheckpoint-Computerfails non-terminating in common cases — notably the once-per-24h rate limit — so failures were reported as success, undermining the reversibility story. It now runs with-ErrorAction Stopand only returnstruewhen an explicit success sentinel is emitted; otherwise it logs and returns false.In-app updater could never find its asset (P1)
The asset matcher looked for a fixed
SysManager.exe, but releases publishSysManager-v<version>.exe, soAssetUrl/AssetSizewere always null. Replaced theAssetNameconstant withIsMainExeAsset(name)— matches the versioned exe, excludes the.sha256companion.Windows Update scan leaked COM objects on failure (P1)
WindowsUpdateService.ScanAsyncreleased its COM objects only on the success path. A cancellation orMapToEntrythrow mid-scan leaked them. The four releases now run in afinallyblock with null guards.Tests & regression
UpdateServiceAssetMatchTests(unit) covers versioned-exe accept / sha256+legacy reject.Constantstest updated to assertIsMainExeAsset(the removedAssetNameconst was caught by the new CI integration-compile step — the regression sweep working as intended).