diff --git a/GFramework.Game.Tests/Setting/SettingsModelTests.cs b/GFramework.Game.Tests/Setting/SettingsModelTests.cs index 6adc5f1d..8f7fa047 100644 --- a/GFramework.Game.Tests/Setting/SettingsModelTests.cs +++ b/GFramework.Game.Tests/Setting/SettingsModelTests.cs @@ -1,3 +1,5 @@ +using System.Reflection; +using System.Threading; using GFramework.Core.Abstractions.Architectures; using GFramework.Core.Abstractions.Lifecycle; using GFramework.Core.Abstractions.Rule; @@ -109,6 +111,66 @@ public sealed class SettingsModelTests }); } + [Test] + public async Task RegisterMigration_During_Cache_Rebuild_Should_Not_Leave_Stale_Type_Cache() + { + var locationProvider = new TestDataLocationProvider(); + var repository = new FakeSettingsDataRepository(); + var model = new SettingsModel(locationProvider, repository); + ((IContextAware)model).SetContext(new Mock(MockBehavior.Loose).Object); + + _ = model.GetData(); + ((IInitializable)model).Initialize(); + + model.RegisterMigration(new TestLatestSettingsMigrationV1ToV2()); + + repository.Stored["TestLatestSettingsData"] = new TestLatestSettingsData + { + Version = 1, + Value = "legacy" + }; + + var lockField = typeof(SettingsModel) + .GetField("_migrationMapLock", BindingFlags.Instance | BindingFlags.NonPublic); + Assert.That(lockField, Is.Not.Null); + + var migrationMapLock = lockField!.GetValue(model); + Assert.That(migrationMapLock, Is.Not.Null); + + Task initializeTask; + Task registerTask; + lock (migrationMapLock!) + { + initializeTask = Task.Run(() => model.InitializeAsync()); + registerTask = Task.Run(() => model.RegisterMigration(new TestLatestSettingsMigrationV2ToV3())); + + Thread.Sleep(50); + + Assert.Multiple(() => + { + Assert.That(initializeTask.IsCompleted, Is.False); + Assert.That(registerTask.IsCompleted, Is.False); + }); + } + + await Task.WhenAll(initializeTask, registerTask); + + repository.Stored["TestLatestSettingsData"] = new TestLatestSettingsData + { + Version = 1, + Value = "legacy" + }; + + await model.InitializeAsync(); + + var current = model.GetData(); + Assert.Multiple(() => + { + Assert.That(current.Version, Is.EqualTo(3)); + Assert.That(current.Value, Is.EqualTo("legacy-migrated-v3")); + }); + } + private sealed class TestSettingsData : ISettingsData { public string Value { get; set; } = "default"; @@ -199,6 +261,25 @@ public sealed class SettingsModelTests } } + private sealed class TestLatestSettingsMigrationV2ToV3 : ISettingsMigration + { + public Type SettingsType => typeof(TestLatestSettingsData); + + public int FromVersion => 2; + + public int ToVersion => 3; + + public ISettingsSection Migrate(ISettingsSection oldData) + { + var data = (TestLatestSettingsData)oldData; + return new TestLatestSettingsData + { + Version = 3, + Value = $"{data.Value}-v3" + }; + } + } + private sealed class FakeSettingsDataRepository : ISettingsDataRepository { public Dictionary RegisteredTypes { get; } = new(StringComparer.Ordinal); diff --git a/GFramework.Game/Setting/SettingsModel.cs b/GFramework.Game/Setting/SettingsModel.cs index 7d4efd39..4e735b7d 100644 --- a/GFramework.Game/Setting/SettingsModel.cs +++ b/GFramework.Game/Setting/SettingsModel.cs @@ -29,6 +29,7 @@ public class SettingsModel(IDataLocationProvider? locationProvider, private readonly ConcurrentDictionary _data = new(); private readonly ConcurrentDictionary> _migrationCache = new(); + private readonly object _migrationMapLock = new(); private readonly ConcurrentDictionary<(Type type, int from), ISettingsMigration> _migrations = new(); private volatile bool _initialized; @@ -124,6 +125,10 @@ public class SettingsModel(IDataLocationProvider? locationProvider, /// /// 同一设置类型与源版本已经注册过迁移器时抛出。 /// + /// + /// 迁移注册表与按类型缓存的版本映射需要保持一致；因此注册与 cache miss 时的缓存重建 + /// 统一通过 串行化，避免并发加载把旧快照重新写回缓存。 + /// public ISettingsModel RegisterMigration(ISettingsMigration migration) { ArgumentNullException.ThrowIfNull(migration); @@ -135,13 +140,17 @@ public class SettingsModel(IDataLocationProvider? locationProvider, migration.ToVersion, nameof(migration)); - if (!_migrations.TryAdd((migration.SettingsType, migration.FromVersion), migration)) + lock (_migrationMapLock) { - throw new InvalidOperationException( - $"Duplicate settings migration registration for {migration.SettingsType.Name} from version {migration.FromVersion}."); + if (!_migrations.TryAdd((migration.SettingsType, migration.FromVersion), migration)) + { + throw new InvalidOperationException( + $"Duplicate settings migration registration for {migration.SettingsType.Name} from version {migration.FromVersion}."); + } + + _migrationCache.TryRemove(migration.SettingsType, out _); } - _migrationCache.TryRemove(migration.SettingsType, out _); return this; } @@ -310,18 +319,28 @@ public class SettingsModel(IDataLocationProvider? locationProvider, /// /// 该方法按设置类型缓存迁移表，并始终以 的版本作为目标运行时版本， /// 避免把旧文件中的版本号误当成当前版本。具体的缺链、版本一致性与前进性校验都委托给 - /// 统一处理。 + /// 统一处理。缓存重建与迁移注册共用 + /// ，确保运行中的初始化不会把过期迁移快照写回缓存。 /// private ISettingsData MigrateIfNeeded(ISettingsData data, ISettingsData latestData) { var type = data.GetType(); - if (!_migrationCache.TryGetValue(type, out var versionMap)) + Dictionary versionMap; + lock (_migrationMapLock) { - versionMap = _migrations - .Where(kv => kv.Key.type == type) - .ToDictionary(kv => kv.Key.from, kv => kv.Value); + if (!_migrationCache.TryGetValue(type, out var cachedVersionMap)) + { + // cache miss 与 RegisterMigration 共用同一把锁，避免注册新迁移后又被旧快照覆盖回缓存。 + versionMap = _migrations + .Where(kv => kv.Key.type == type) + .ToDictionary(kv => kv.Key.from, kv => kv.Value); - _migrationCache[type] = versionMap; + _migrationCache[type] = versionMap; + } + else + { + versionMap = cachedVersionMap; + } } return VersionedMigrationRunner.MigrateToTargetVersion( diff --git a/ai-plan/public/data-repository-persistence/todos/data-repository-persistence-tracking.md b/ai-plan/public/data-repository-persistence/todos/data-repository-persistence-tracking.md index 84858d2c..d82bde4c 100644 --- a/ai-plan/public/data-repository-persistence/todos/data-repository-persistence-tracking.md +++ b/ai-plan/public/data-repository-persistence/todos/data-repository-persistence-tracking.md @@ -14,7 +14,7 @@ `ai-plan/public/data-repository-persistence/` - 第一轮 settings / persistence / serialization 修复、测试与文档同步已完成，并收入主题内 `archive/` - 已完成 `SettingsModel` / `SaveRepository` 共享迁移执行器收敛与契约补强 - - 已完成 PR #260 的 review follow-up：迁移链快照一致性、XML docs 补齐与文档安全示例修正 + - 已完成 PR #260 的追加 review follow-up：`SettingsModel` 迁移缓存并发一致性 - 下一轮需要继续评估 codec / persistence pipeline 边界 ## 当前状态摘要 @@ -35,6 +35,8 @@ - `SaveRepository` 继续在 `LoadAsync(slot)` 期间迁移并回写，但其核心链式校验已与设置迁移共用同一实现 - PR #260 review follow-up 已完成：`VersionedMigrationRunner` / `SettingsModel` 的 XML 异常契约已补齐， `SaveRepository` 单次加载已切换为迁移表快照，避免并发注册期间读取变化中的迁移链 +- `SettingsModel` 现已通过 `_migrationMapLock` 串行化迁移注册与 cache miss 时的按类型缓存重建， + 避免并发注册把旧快照重新写回 `_migrationCache` - `docs/zh-CN/game/index.md` 当前仍承担最低接入示例，因此其中的 `JsonSerializer` 配置必须避免鼓励对 用户可篡改存档启用不受限的多态反序列化 @@ -61,6 +63,9 @@ - 已完成 PR #260 follow-up，并新增定向回归测试锁定迁移快照与失败不污染持久化数据的约束 - `dotnet test GFramework.Game.Tests/GFramework.Game.Tests.csproj -c Release --filter "FullyQualifiedName~SettingsModelTests|FullyQualifiedName~PersistenceTests" -m:1 -nodeReuse:false` 已通过（21/21） +- 已新增 `SettingsModelTests` 并发回归测试，锁定迁移注册与 cache miss 重建不会留下 stale cache +- `dotnet test GFramework.Game.Tests/GFramework.Game.Tests.csproj -c Release --filter "FullyQualifiedName~SettingsModelTests" -m:1 -nodeReuse:false` + 已通过（5/5） - 本次定向验证过程中出现的 analyzer warning 来自仓库既有代码，不属于本轮新增问题 ## 下一步 diff --git a/ai-plan/public/data-repository-persistence/traces/data-repository-persistence-trace.md b/ai-plan/public/data-repository-persistence/traces/data-repository-persistence-trace.md index bb2dec07..e251c429 100644 --- a/ai-plan/public/data-repository-persistence/traces/data-repository-persistence-trace.md +++ b/ai-plan/public/data-repository-persistence/traces/data-repository-persistence-trace.md @@ -107,3 +107,24 @@ 1. 回到 codec / persistence pipeline 边界评估 2. 继续判断压缩、加密、元数据与备份策略是否需要新的 dedicated pipeline abstraction + +### 阶段：SettingsModel 迁移缓存并发修复（RP-001） + +- 重新使用 `$gframework-pr-review` 复核 PR #260 后确认：此前遗漏了一条仍然 open 的 `SettingsModel.cs` major comment， + 问题点不是迁移执行本身，而是 `_migrations` 与 `_migrationCache` 在并发注册和 cache miss 重建交错时，可能把旧快照写回缓存 +- 确认该 comment 来自 `2026-04-20T04:23:09Z` 的 CodeRabbit review run；当前修复策略采用同一把私有锁串行化： + - `RegisterMigration` 中的 `_migrations.TryAdd(...)` 与 `_migrationCache.TryRemove(...)` + - `MigrateIfNeeded` 在 cache miss 时按类型重建 `versionMap` 并写回 `_migrationCache` +- 同步补充源码注释与 XML remarks，明确运行时注册与缓存重建共享同一并发语义 +- 计划新增 `SettingsModelTests` 回归测试，验证 cache rebuild 与运行时注册在同一把锁前排队后，后续初始化能看到新增迁移而不会留下 stale cache + +### 验证：SettingsModel 迁移缓存并发修复 + +1. `dotnet test GFramework.Game.Tests/GFramework.Game.Tests.csproj -c Release --filter "FullyQualifiedName~SettingsModelTests" -m:1 -nodeReuse:false` 已通过（5/5） +2. 本轮验证中未再出现 `SettingsModel` 新增的 nullable warning；输出中的 analyzer warning 仍为仓库既有项加上新的 `MA0158` + 建议，后者来自本轮新增对象锁 + +### 下一步：SettingsModel 迁移缓存并发修复 + +1. 若继续收口 analyzer 反馈，可评估是否将 `_migrationMapLock` 升级为 `System.Threading.Lock`，同时保留可验证的并发回归测试策略 +2. 否则恢复到 codec / persistence pipeline 边界评估