fix: resolve 14 code-review issues across Core/Differential/Bowl/Drivelution/Extension#513
Merged
Merged
Conversation
…elution/Extension ## Bug fixes - **Brotli patch detection**: add BrotliFormatVersion (0x02) to format detection and decompression switch in BsdiffDiffer (#512) - **ReadFileWithBudget**: trim buffer to actual bytes read to prevent zero-padded data corrupting patch computation (#512) - **BsdiffDiffer.Search**: guard against sentinel -1 values from Split() that would cause IndexOutOfRangeException (#512) - **Bowl EnsureDirectory**: stop deleting the fail directory on every Prepare() call — preserves crash diagnostics from prior sessions (#512) - **FileNode.Equals**: return false for incompatible types instead of throwing ArgumentException, conforming to the IEquatable contract (#512) - **EventManager.Dispatch**: snapshot the delegate invocation list before enumerating to avoid race with concurrent AddListener/RemoveListener (#512) - **DiffPipeline**: add 'using' to both SemaphoreSlim instances to fix kernel handle leaks (#512) - **FileTree.Compare**: guard all 'node0' access paths with null-conditional operators to prevent NullReferenceException (#512) - **ClientStrategy.CheckFail**: use Version.TryParse instead of bare new Version() to avoid crashing on malformed version strings (#512) - **GeneralTracer**: wrap InitializeFileListener with lock to prevent duplicate TextTraceListener creation on concurrent day-rollover (#512) - **ProcessRunner**: cancel the Task.Delay timer via CancellationTokenSource when the process exits first, preventing timer leak (#512) - **OssStrategy**: resolve upgrade app name and search pattern based on RuntimeInformation.IsOSPlatform instead of hardcoding .exe (#512) - **VersionComparer**: use long instead of int for major/minor/patch to prevent overflow on extremely large version numbers (#512) - **ExtensionGeneralHost**: move name extraction before lifecycle hook calls and use extensionName (not file path) as Id in ExtensionMetadata (#512) ## Test coverage - BsdiffDifferTests: 6 new edge-case tests (single-byte, two-byte, repeating patterns, single-byte-change round-trips) - VersionComparerTests: 3 new large-number overflow tests - FileTreeTests: 6 new tests for null node0, imbalanced trees, matching trees - FileNodeTests: update Equals_NonFileNodeType test to match new behaviour All 1864 tests pass across 5 test suites (0 failures). Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple correctness, safety, and reliability issues discovered during a cross-project code review, spanning the update pipeline (Core), differential patching (Differential), crash collection (Bowl), utilities (Drivelution), and extension installation (Extension). It also strengthens regression coverage with new targeted tests.
Changes:
- Fixes several patching/data-integrity issues (BSDIFF format detection, suffix-array sentinel handling, HDiff streaming buffer trimming).
- Improves reliability under concurrency and resource pressure (EventManager dispatch snapshotting, SemaphoreSlim disposal, ProcessRunner timer cancellation, thread-safe trace rotation).
- Enhances cross-platform behavior and extension lifecycle metadata handling, with expanded test coverage for edge cases and overflow scenarios.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/DrivelutionTest/Utilities/VersionComparerAndRestartHelperTests.cs | Adds overflow/large-number SemVer validation & comparison tests |
| tests/DifferentialTest/Differ/BsdiffDifferTests.cs | Adds BSDIFF edge-case roundtrip tests (small files, repeating patterns, single-byte diff) |
| tests/CoreTest/FileSystem/FileTreeTests.cs | Adds Compare() null/imbalanced-tree regression tests |
| tests/CoreTest/FileSystem/FileNodeTests.cs | Updates Equals() non-FileNode behavior expectation |
| src/c#/GeneralUpdate.Extension/Core/GeneralExtensionHost.cs | Uses derived extension name for lifecycle hook metadata Id/Name |
| src/c#/GeneralUpdate.Drivelution/Core/Utilities/VersionComparer.cs | Switches SemVer numeric parsing/comparison to long to avoid int overflow |
| src/c#/GeneralUpdate.Differential/Differ/StreamingHdiffDiffer.cs | Trims ReadFileWithBudget buffer to actual bytes read |
| src/c#/GeneralUpdate.Differential/Differ/BsdiffDiffer.cs | Extends patch format detection and guards suffix-array sentinel -1 |
| src/c#/GeneralUpdate.Core/Tracer/GeneralTracer.cs | Makes daily log rotation thread-safe via locking |
| src/c#/GeneralUpdate.Core/Strategy/OssStrategy.cs | Improves cross-platform upgrade executable naming and diagnostics |
| src/c#/GeneralUpdate.Core/Strategy/ClientStrategy.cs | Prevents Version.Parse exceptions by using Version.TryParse |
| src/c#/GeneralUpdate.Core/Pipeline/DiffPipeline.cs | Disposes SemaphoreSlim via using var to prevent handle leaks |
| src/c#/GeneralUpdate.Core/FileSystem/FileTree.cs | Prevents NREs in Compare() via null-propagation |
| src/c#/GeneralUpdate.Core/FileSystem/FileNode.cs | Makes Equals(object) return false (not throw) for non-FileNode input |
| src/c#/GeneralUpdate.Core/Event/EventManager.cs | Avoids delegate mutation race by snapshotting invocation list |
| src/c#/GeneralUpdate.Bowl/Strategies/WindowsBowlStrategy.cs | Avoids deleting existing fail directories (preserve diagnostics) |
| src/c#/GeneralUpdate.Bowl/Strategies/LinuxBowlStrategy.cs | Avoids deleting existing fail directories (preserve diagnostics) |
| src/c#/GeneralUpdate.Bowl/Strategies/ProcessRunner.cs | Cancels timeout delay when process exits to avoid timer leak |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+311
to
+315
| if (candidate == BZip2FormatVersion || candidate == DeflateFormatVersion | ||
| #if NET6_0_OR_GREATER | ||
| || candidate == BrotliFormatVersion | ||
| #endif | ||
| ) |
Comment on lines
106
to
113
| return new SemVerInfo | ||
| { | ||
| Major = int.Parse(match.Groups["major"].Value), | ||
| Minor = int.Parse(match.Groups["minor"].Value), | ||
| Patch = int.Parse(match.Groups["patch"].Value), | ||
| Major = long.Parse(match.Groups["major"].Value), | ||
| Minor = long.Parse(match.Groups["minor"].Value), | ||
| Patch = long.Parse(match.Groups["patch"].Value), | ||
| Prerelease = match.Groups["prerelease"].Value, | ||
| BuildMetadata = match.Groups["buildmetadata"].Value | ||
| }; |
Comment on lines
125
to
129
| var isNum1 = long.TryParse(parts1[i], out long num1); | ||
| var isNum2 = long.TryParse(parts2[i], out long num2); | ||
|
|
||
| if (isNum1 && isNum2) | ||
| { |
Comment on lines
+271
to
+273
| var searchPattern = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "*.exe" : "*"; | ||
| var dirFiles = Directory.GetFiles(upgradeDir, searchPattern).Select(f => Path.GetFileName(f)); | ||
| GeneralTracer.Info($"[OssClient] Files in {upgradeDir}: [{string.Join(", ", dirFiles)}]"); |
| [Fact] | ||
| public void Compare_PrereleaseWithLargeNumbers_DoesNotOverflow() | ||
| { | ||
| // long.MaxValue ~9.2e18 approaching overflow in int |
1. BsdiffDiffer: recognize Brotli marker (0x02) as extended-header format even on netstandard2.0 (fail-fast) instead of mis-detecting as legacy BZip2 2. VersionComparer.ParseVersion: use TryParseNumeric with overflow guard instead of bare long.Parse (can throw on > Int64 values) 3. VersionComparer.ComparePrerelease: add IsNumericString fallback for purely-numeric identifiers that exceed long.MaxValue (length+ordinal) 4. OssStrategy: limit diagnostic file listing to 20 entries to prevent unbounded log noise on non-Windows 5. VersionComparerTests: fix doc comment (long.MaxValue -> int.MaxValue) Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
Copilot Review 反馈修复（第 2 轮）Copilot 提出了 5 条建议，全部采纳：
增量改动： 4 文件，+61/−12 行。全部 1864 测试通过。 |
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.
概述
对 GeneralUpdate 全内置机制进行系统性代码审查后，修复了 二~四类 共 14 个 Bug，覆盖 Core、Differential、Bowl、Drivelution、Extension 五个项目。
关联 Issue: Closes #512
二、严重 Bug / 数据损坏 (7)
BsdiffDiffer.csBrotliFormatVersion = 0x02常量、检测分支和 switch 分支（#if NET6_0_OR_GREATER）ReadFileWithBudget返回未修剪缓冲区StreamingHdiffDiffer.csArray.Resize(ref buffer, totalRead)修剪尾部零填充字节BsdiffDiffer.Search索引到哨兵值-1BsdiffDiffer.csMatchLength前检查I[start] >= 0、I[end] >= 0EnsureDirectory删除重建故障目录WindowsBowlStrategy.cs,LinuxBowlStrategy.csFileNode.Equals对非 FileNode 抛异常FileNode.csif (obj is not FileNode tempNode) return falseEventManager.csDispatch中先GetInvocationList()快照再枚举SemaphoreSlim未释放DiffPipeline.csvar semaphore→using var semaphore三、高严重性 Bug (4)
FileTree.CompareNREFileTree.csnode0.Left/node0.Right改为node0?.Left/node0?.RightVersion.Parse未保护非法字符串ClientStrategy.csVersion.TryParseGeneralTracer日切线程安全GeneralTracer.csInitializeFileListener整体加lock (_lockObj)ProcessRunner计时器泄漏ProcessRunner.cs四、中等严重问题 (3)
OssStrategy硬编码.exeOssStrategy.cs"GeneralUpdate.Upgrade.exe"或"GeneralUpdate.Upgrade"VersionComparer数值溢出VersionComparer.csint→longGeneralExtensionHost.csId = extensionPath改为Id = extensionName测试覆盖强化
BsdiffDifferTests: 新增 6 个边缘用例（单字节/2字节/重复模式/单字节变化/微小旧到大新）VersionComparerTests: 新增 3 个大数值溢出测试（> int.MaxValue）FileTreeTests: 新增 6 个 Compare 测试（null/不平衡树/匹配树）FileNodeTests: 更新Equals_NonFileNodeType测试断言验证
全部 1864 个测试通过：Core 959 ✅ + Differential 107 ✅ + Bowl 152 ✅ + Drivelution 279 ✅ + Extension 367 ✅
🤖 Generated with Claude Code