Fix code review findings and AOT audit#299
Merged
Merged
Conversation
…gress, cleanup P0 fixes: - RetryPolicy: extract duplicate delay logic, improve exception capture on last retry - BaseDriverUpdater: rollback uses CancellationToken.None (prevent timeout cancellation) - BatchUpdateAsync: fix Interlocked.CompareExchange → Interlocked.Increment pattern for progress - Windows VerifyInstallationAsync: treat pnputil non-zero exit as non-fatal (don't trigger rollback) P1 fixes: - CommandRunner: kill process on cancellation token (prevent zombie processes) - DrivelutionFactory: CreateValidator/CreateBackup now return macOS implementations - DrivelutionFactory: error message includes macOS - BaseDriverUpdater.BackupAsync: add try-catch for consistency with ValidateAsync P2 fixes: - Remove dead ReportProgress method and OnProgress event - Merge QuickUpdateAsync overloads into single method with optional strategy - PipelineResult.Exception now propagated to ErrorInfo.Details/StackTrace Closes #298
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request addresses findings from the Drivelution code review and AOT audit by tightening cancellation/retry behavior, improving rollback/verification semantics, and cleaning up/aligning factory and public API surfaces.
Changes:
- Adjusts pipeline error/rollback behavior (propagate exception details; rollback not cancelled by timeout; fixes batch progress calculation; wraps backup in try/catch).
- Updates execution and platform plumbing (kill external processes on cancellation; factory now returns macOS validator/backup implementations and updates supported-platform messaging).
- Tweaks top-level API and Windows verification behavior (merge
QuickUpdateAsyncoverloads; treatpnputil /enum-driversfailures as non-fatal).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/c#/GeneralUpdate.Drivelution/Windows/Implementation/WindowsGeneralDrivelution.cs | Treats pnputil verification failures as non-fatal to avoid unnecessary rollbacks. |
| src/c#/GeneralUpdate.Drivelution/GeneralDrivelution.cs | Merges QuickUpdateAsync overloads and introduces optional strategy parameter. |
| src/c#/GeneralUpdate.Drivelution/Core/Pipeline/RetryPolicy.cs | Refactors retry delay logic and adjusts exception/cancellation handling documentation/behavior. |
| src/c#/GeneralUpdate.Drivelution/Core/Pipeline/BaseDriverUpdater.cs | Improves error detail propagation, rollback token usage, backup error handling, and batch progress reporting; removes dead progress event API. |
| src/c#/GeneralUpdate.Drivelution/Core/Execution/CommandRunner.cs | Ensures spawned processes are killed when callers cancel. |
| src/c#/GeneralUpdate.Drivelution/Core/DriverUpdaterFactory.cs | Updates supported platform error message and returns macOS validator/backup implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| await process.WaitForExitAsync(cancellationToken); | ||
| // Ensure the process is killed if the caller cancels the wait | ||
| await using (cancellationToken.Register(() => |
Comment on lines
60
to
63
| /// <summary> | ||
| /// Executes an asynchronous operation with retry logic. | ||
| /// On the last retry, the exception is wrapped in a descriptive AggregateException rather than escaping raw. | ||
| /// </summary> |
Comment on lines
+175
to
+176
| // Use a fresh CancellationToken for rollback — the linked token may be cancelled by timeout | ||
| var rolledBack = await TryRollbackAsync(backupPath, CancellationToken.None); |
| try | ||
| { | ||
| return await _backup.BackupAsync(driverInfo.FilePath, backupPath, cancellationToken); | ||
| } |
Comment on lines
63
to
67
| public static async Task<UpdateResult> QuickUpdateAsync( | ||
| DriverInfo driverInfo, | ||
| DriverInfo driverInfo, | ||
| UpdateStrategy? strategy = null, | ||
| IProgress<UpdateProgress>? progress = null, | ||
| CancellationToken cancellationToken = default) |
Comment on lines
488
to
493
| /// <summary> | ||
| /// Raised when the entire update process completes. | ||
| /// </summary> | ||
| public event Action<UpdateResult>? OnUpdateCompleted; | ||
|
|
||
| /// <summary> | ||
| /// Raised to report progress (percentage, message). | ||
| /// </summary> | ||
| public event Action<int, string>? OnProgress; | ||
|
|
||
| // ─── Helpers ─────────────────────────────────────────────────────── |
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
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
Address all findings from the 9-PR Drivelution code review and AOT audit.
AOT Audit
No new AOT/trimming warnings introduced. Only pre-existing warning: GeneralTracer.StackFrame.GetMethod (IL2026).
P0 (Critical)
P1 (Medium)
P2 (Low)
Closes #298