chore(engine-ambient): harden shim + LocalLog statics for fixture parallelism
Follow-up to the multi-instancing migration. Wraps the process-shared engine statics that aren't ambient-fronted but race between concurrent battles: - UnityEngine.Resources._loaded: Dictionary -> ConcurrentDictionary.GetOrAdd (the shared prefab cache keyed by path; concurrent first-misses produced duplicate GameObjects + Dictionary corruption) - UnityEngine.GameObject._components: Dictionary -> ConcurrentDictionary with Interlocked.CompareExchange init (Resources.Load returns SHARED prefab GameObjects, so two engines' Setup() can race on the same _components map — surfaced as "Operations that change non-concurrent collections" crashes during BattleManagerBase ctor's GetComponent<T>() chain) - Wizard.LocalLog: single static lock around all mutating entry points (StringBuilder _lastTraceLogStringBuilder + ~12 mutable string/bool/int scratch fields; serializing the trace-log surface is cheap since logging is not the hot path) Flips SVSim.BattleEngine.Tests assembly Parallelizable scope from Self to Fixtures and restructures MultiInstanceEngineTests.StressN_BaselineMatches so Setup runs INSIDE Task.Run (was previously serialized as a workaround for the LocalLog races). The fixture is also lifted to ParallelScope.All so the two-engines and stress tests can run alongside each other. Suite fully green under fixture parallelism (59/0/2 across 3 consecutive runs); SVSim.UnitTests still 1054/0/0 — true multi-instance correctness is now proved end-to-end in tests rather than gated behind a serial workaround. Manifest sha refresh + new patch artifact for the LocalLog edit (decomp-origin); the two shim files are authored, so no metadata update is needed for them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
65
SVSim.BattleEngine/Patches/LocalLog.parallel-lock.patch
Normal file
65
SVSim.BattleEngine/Patches/LocalLog.parallel-lock.patch
Normal file
@@ -0,0 +1,65 @@
|
||||
Multi-instance hardening: gate all mutating LocalLog entry points with a single
|
||||
static lock so concurrent battle setups (fixture-parallel tests, parallel
|
||||
SessionBattleEngine.Setup() calls) don't corrupt the StringBuilder /
|
||||
string accumulators (FormatException on _lastTraceLogStringBuilder.AppendFormat,
|
||||
torn _gungnirLog/_socketFrameLog/_roomCreateLog/_disconnectLog/_loadResourceLog
|
||||
state, interleaved file writes to the four scratch log paths).
|
||||
|
||||
LocalLog is a process-wide trace-log singleton (~15 mutable static fields). The
|
||||
engine doesn't care about its content — server consumers never read it — but the
|
||||
mutations crash under parallel access because StringBuilder + non-concurrent
|
||||
string `+=` accumulators aren't thread-safe. Logging isn't the hot path; global
|
||||
serialization via a single static lock is the simplest safe fix.
|
||||
|
||||
Pattern: each public mutating method body is wrapped in `lock (_gate) { ... }`.
|
||||
For methods that already call other locked entry points, the inner body is moved
|
||||
into a private `*Locked()` helper that the wrapper invokes (the C# `lock`
|
||||
statement is reentrant, so direct re-entry would also be safe — the split is
|
||||
purely for clarity at call sites that already hold the gate).
|
||||
|
||||
Methods gated (public surface, 26 entry points):
|
||||
CreateLogFile, CreateLocalLogFile, MakeTreceLogToSend (private but called by
|
||||
the three public Send* wrappers), RecordResouseLoadError,
|
||||
RecordTurnEndIfLoadErrorOccured, RecordFreezeLogIfLoadErrorOccured,
|
||||
ExistResourceLoadErrorInNetWorkBattle, AccumulateTraceLog,
|
||||
AccumulateTraceInquiryLog, AccumulateSettingLog, AddGungnirLog,
|
||||
InitGungnirLog, AddSocketFrameLog, InitSocketFrameLog,
|
||||
AccumulateTraceLogAddRoomCreateLog, AddRoomCreateLog, InitRoomCreateLog,
|
||||
UpdateLoadResourceLog, GetLoadResourceLog, SetDisconnectLog,
|
||||
InitDisconnectLog, AccumulateLastTraceLog, SubmitAccumulateLastTraceLog,
|
||||
SetLastTraceLogTurn, ClearLogByType, ClearTraceLog, ClearLastTraceLog,
|
||||
ClearLastLogKey, ClearAllLog, RecordCheckLog.
|
||||
|
||||
Locked helpers added (private): MakeTreceLogToSendLocked, AccumulateTraceLogLocked,
|
||||
ExistResourceLoadErrorInNetWorkBattleLocked, SubmitAccumulateLastTraceLogLocked,
|
||||
ClearTraceLogLocked, ClearLastLogKeyLocked, RecordCheckLogLocked.
|
||||
|
||||
Two internal callsites updated to call the *Locked variant directly (already
|
||||
inside the gate): AccumulateTraceLogAddRoomCreateLog inlines the InitRoomCreateLog
|
||||
clear (was a recursive lock acquisition); SetLastTraceLogTurn now calls
|
||||
ClearLog directly (was going through the public ClearLastTraceLog which
|
||||
re-acquired the gate). Behavior unchanged — reentrant `lock` would have worked
|
||||
too, this is purely a readability cleanup.
|
||||
|
||||
ZERO logic changes: the bodies are byte-for-byte the decomp with only `lock (_gate) { ... }`
|
||||
wrappers added and a single shared `private static readonly object _gate` field
|
||||
declared at the top of the class.
|
||||
|
||||
--- Engine/Wizard/LocalLog.cs (top of class)
|
||||
+ // HEADLESS-PATCH (engine-port): all public mutating entry points + the private file-write
|
||||
+ // helpers are gated by a single static lock so concurrent battle setups don't corrupt the
|
||||
+ // StringBuilder / string accumulators or interleave writes to the four scratch log files.
|
||||
+ private static readonly object _gate = new object();
|
||||
|
||||
--- Engine/Wizard/LocalLog.cs (each public mutating method)
|
||||
- public static void <Method>(<args>)
|
||||
- {
|
||||
- <body>
|
||||
- }
|
||||
+ public static void <Method>(<args>)
|
||||
+ {
|
||||
+ lock (_gate)
|
||||
+ {
|
||||
+ <body>
|
||||
+ }
|
||||
+ }
|
||||
Reference in New Issue
Block a user