fix(config): 单个配置项损坏时重置为默认值而非致命崩溃#3346
Open
NoClassFoundError wants to merge 2 commits into
Open
Conversation
某个配置项的值损坏为非法内容时，JsonFileProvider.Get<T> 会抛出 JsonException；因其不是ConfigFileInitException，ConfigService 会走到 Context.Fatal，导致每次启动都在读取该项 （如 ThemeConfigGroup.DarkColor）时崩溃、无法进入界面，且损坏项不会被重置（用户重置设置 也无效）。在 FileConfigStorage 的 Get 访问中兜住读取异常：记录警告、队列删除该损坏项以持久化 重置，并返回 false 回退到编码默认值，使启动可继续。整文件损坏的场景仍由 ConfigService 既有的 .failbackup/.bak 恢复逻辑处理。
pcl-ce-automation
Bot
added
🛠️ 等待审查
Contributor
审阅者指南（在小型 PR 上折叠）审阅者指南此 PR 在读取单个由 JSON 支持的配置条目时添加了防御性处理，使损坏的配置值不再导致致命崩溃；相反，它们会被记录日志、安排删除，并在保持现有整文件恢复逻辑不变的同时，系统回退到默认值。 FileConfigStorage.OnAccess 中处理损坏配置条目的序列图sequenceDiagram
actor ConfigService
participant FileConfigStorage
participant File
participant LogWrapper
participant writeActionChannelWriter
ConfigService->>FileConfigStorage: OnAccess(StorageAction.Get, key, value)
alt file exists
FileConfigStorage->>File: Get(key)
alt [no exception]
File-->>FileConfigStorage: TValue
FileConfigStorage-->>ConfigService: true
else [exception thrown]
File--xFileConfigStorage: Exception
FileConfigStorage->>LogWrapper: Warn(ex, Config, message)
FileConfigStorage->>writeActionChannelWriter: TryWrite((key, File.Remove))
FileConfigStorage-->>ConfigService: false (fallback to default)
end
else file does not exist
FileConfigStorage-->>ConfigService: false
end
文件级变更
与关联 Issue 的对应情况评估
提示与命令与 Sourcery 交互
自定义你的体验访问你的控制面板以：
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR adds defensive handling around reading individual JSON-backed config entries so that corrupted values no longer cause a fatal crash; instead, they are logged, scheduled for deletion, and the system falls back to default values while existing whole-file recovery logic remains untouched. Sequence diagram for corrupted config entry handling in FileConfigStorage.OnAccesssequenceDiagram
actor ConfigService
participant FileConfigStorage
participant File
participant LogWrapper
participant writeActionChannelWriter
ConfigService->>FileConfigStorage: OnAccess(StorageAction.Get, key, value)
alt file exists
FileConfigStorage->>File: Get(key)
alt [no exception]
File-->>FileConfigStorage: TValue
FileConfigStorage-->>ConfigService: true
else [exception thrown]
File--xFileConfigStorage: Exception
FileConfigStorage->>LogWrapper: Warn(ex, Config, message)
FileConfigStorage->>writeActionChannelWriter: TryWrite((key, File.Remove))
FileConfigStorage-->>ConfigService: false (fallback to default)
end
else file does not exist
FileConfigStorage-->>ConfigService: false
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并提供了一些总体反馈：
- 建议将捕获的异常从泛型
Exception收窄为你实际预期的反序列化异常（例如JsonException），以避免掩盖无关的 I/O 或并发问题。 _writeActionChannel.Writer.TryWrite(...)的返回结果目前被忽略；如果通道已满或不可用，损坏的配置项将无法被移除，因此建议处理返回值为false的情况，或者使用WriteAsync以确保最终能够删除。- 由于损坏配置的删除是通过通道延迟执行的，请确认这个异步写操作与其他配置写入之间的顺序是安全的，以避免出现竞争条件，即新的配置值被排队的删除操作覆盖。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider narrowing the catch from `Exception` to the specific deserialization exceptions you expect (e.g., `JsonException`) to avoid masking unrelated I/O or concurrency issues.
- The result of `_writeActionChannel.Writer.TryWrite(...)` is ignored; if the channel is full or unavailable the corrupted config item will not be removed, so it may be worth handling the `false` case or using `WriteAsync` to ensure eventual deletion.
- Since removal of the corrupted config is deferred via the channel, confirm that this asynchronous write is ordered safely with other config writes to avoid race conditions where a fresh value might be overwritten by the queued removal.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/Configuration/Storage/FileConfigStorage.cs" line_range="111-112" />
<code_context>
+ catch (Exception ex)
+ {
+ LogWrapper.Warn(ex, "Config", $"配置项 {strKey} 读取失败（可能已损坏），重置为默认值");
+ _writeActionChannel.Writer.TryWrite((strKey, () => File.Remove(strKey)));
+ return false;
+ }
return true;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Ignoring the TryWrite result may lead to silent failure to clean up corrupted configs.
If the channel is full or unavailable, `TryWrite` can fail and the corrupted file will remain without any indication. Consider handling the `false` return (e.g., logging, alternative cleanup, or synchronous removal) so enqueue failures are visible and cleanup is more reliable.
Suggested implementation:
```csharp
catch (Exception ex)
{
LogWrapper.Warn(ex, "Config", $"配置项 {strKey} 读取失败（可能已损坏），重置为默认值");
if (!_writeActionChannel.Writer.TryWrite((strKey, () => File.Remove(strKey))))
{
// 通知队列写入失败，进行同步清理以避免损坏配置长期存在
LogWrapper.Warn(null, "Config", $"配置项 {strKey} 清理任务写入队列失败，尝试同步删除文件");
try
{
File.Remove(strKey);
}
catch (Exception cleanupEx)
{
LogWrapper.Error(cleanupEx, "Config", $"配置项 {strKey} 同步删除失败，可能需要人工处理");
}
}
return false;
}
```
1. Ensure `LogWrapper.Warn(Exception ex, string category, string message)` and `LogWrapper.Error(Exception ex, string category, string message)` overloads exist and are used consistently. If `Warn` does not allow a null `Exception`, replace `LogWrapper.Warn(null, ...)` with an appropriate overload or a simple `LogWrapper.Warn("Config", "...")`.
2. If there is a centralized error-handling or metrics mechanism for storage failures, consider also reporting the synchronous cleanup failure there to keep observability consistent.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- Consider narrowing the catch from
Exceptionto the specific deserialization exceptions you expect (e.g.,JsonException) to avoid masking unrelated I/O or concurrency issues. - The result of
_writeActionChannel.Writer.TryWrite(...)is ignored; if the channel is full or unavailable the corrupted config item will not be removed, so it may be worth handling thefalsecase or usingWriteAsyncto ensure eventual deletion. - Since removal of the corrupted config is deferred via the channel, confirm that this asynchronous write is ordered safely with other config writes to avoid race conditions where a fresh value might be overwritten by the queued removal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider narrowing the catch from `Exception` to the specific deserialization exceptions you expect (e.g., `JsonException`) to avoid masking unrelated I/O or concurrency issues.
- The result of `_writeActionChannel.Writer.TryWrite(...)` is ignored; if the channel is full or unavailable the corrupted config item will not be removed, so it may be worth handling the `false` case or using `WriteAsync` to ensure eventual deletion.
- Since removal of the corrupted config is deferred via the channel, confirm that this asynchronous write is ordered safely with other config writes to avoid race conditions where a fresh value might be overwritten by the queued removal.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/Configuration/Storage/FileConfigStorage.cs" line_range="111-112" />
<code_context>
+ catch (Exception ex)
+ {
+ LogWrapper.Warn(ex, "Config", $"配置项 {strKey} 读取失败（可能已损坏），重置为默认值");
+ _writeActionChannel.Writer.TryWrite((strKey, () => File.Remove(strKey)));
+ return false;
+ }
return true;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Ignoring the TryWrite result may lead to silent failure to clean up corrupted configs.
If the channel is full or unavailable, `TryWrite` can fail and the corrupted file will remain without any indication. Consider handling the `false` return (e.g., logging, alternative cleanup, or synchronous removal) so enqueue failures are visible and cleanup is more reliable.
Suggested implementation:
```csharp
catch (Exception ex)
{
LogWrapper.Warn(ex, "Config", $"配置项 {strKey} 读取失败（可能已损坏），重置为默认值");
if (!_writeActionChannel.Writer.TryWrite((strKey, () => File.Remove(strKey))))
{
// 通知队列写入失败，进行同步清理以避免损坏配置长期存在
LogWrapper.Warn(null, "Config", $"配置项 {strKey} 清理任务写入队列失败，尝试同步删除文件");
try
{
File.Remove(strKey);
}
catch (Exception cleanupEx)
{
LogWrapper.Error(cleanupEx, "Config", $"配置项 {strKey} 同步删除失败，可能需要人工处理");
}
}
return false;
}
```
1. Ensure `LogWrapper.Warn(Exception ex, string category, string message)` and `LogWrapper.Error(Exception ex, string category, string message)` overloads exist and are used consistently. If `Warn` does not allow a null `Exception`, replace `LogWrapper.Warn(null, ...)` with an appropriate overload or a simple `LogWrapper.Warn("Config", "...")`.
2. If there is a centralized error-handling or metrics mechanism for storage failures, consider also reporting the synchronous cleanup failure there to keep observability consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
1.全局 JsonFileProvider（损坏抛 JsonException）和本地 YamlFileProvider（Get 回退分支 (T)(object)graphStr 对非字符串类型会抛 InvalidCastException），另有反序列化为 null 时的 NullReferenceException。收窄到JsonException 会漏掉 YAML 与 null 情形而重新崩溃。且此路径是对已解析入内存的节点做反序列化、不涉及 I/O，不存在掩盖 I/O 问题的风险。 |
Chiloven945
approved these changes
Jul 6, 2026
RyogiMutsuki
requested changes
Jul 6, 2026
| { | ||
| value = File.Get<TValue>(strKey); | ||
| } | ||
| catch (Exception ex) |
Member
There was a problem hiding this comment.
应当只捕获相关错误，而不是 catch Exception
与配置系统不相关的错误不应当导致设置项被重置
Member
|
另，根据 #3340 ，应当在提交 PR 时声明你已经对 AI 输出进行了审查 |
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
3 participants
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.
某个配置项的值损坏为非法内容时，JsonFileProvider.Get 会抛出 JsonException；因其不是ConfigFileInitException，ConfigService 会走到 Context.Fatal，导致每次启动都在读取该项 （如 ThemeConfigGroup.DarkColor）时崩溃、无法进入界面，且损坏项不会被重置（用户重置设置 也无效）。在 FileConfigStorage 的 Get 访问中兜住读取异常：记录警告、队列删除该损坏项以持久化 重置，并返回 false 回退到编码默认值，使启动可继续。整文件损坏的场景仍由 ConfigService 既有的 .failbackup/.bak 恢复逻辑处理。
resolves #3280
本PR的提交信息生成由Claude Code Opus 4.8辅助生成，由Opus 4.8对代码进行了审查。
Summary by Sourcery
Bug Fixes:
Original summary in English
Summary by Sourcery
Bug Fixes: