diff --git a/src/AcDream.Core.Net/GameEventWiring.cs b/src/AcDream.Core.Net/GameEventWiring.cs index 414d06b1..7f084dc5 100644 --- a/src/AcDream.Core.Net/GameEventWiring.cs +++ b/src/AcDream.Core.Net/GameEventWiring.cs @@ -80,7 +80,7 @@ public static class GameEventWiring FriendsState? friends = null, SquelchState? squelch = null, Action>? onDesiredComponents = null, - Action? onCharacterOptions = null, + Action? onCharacterOptions = null, Func? clientTime = null, ExternalContainerState? externalContainers = null, // Slice 5.3: the vendor browse session owner. Matches the existing @@ -729,7 +729,11 @@ public static class GameEventWiring Console.WriteLine($"vitals: PlayerDescription body.len={e.Payload.Length} parsed={(p is null ? "NULL" : $"vec={p.Value.VectorFlags} attrs={p.Value.Attributes.Count} spells={p.Value.Spells.Count}")}"); if (p is null) return; - onCharacterOptions?.Invoke(p.Value.Options1, p.Value.Options2); + // R3: a trailer-truncated parse carries zero placeholder option + // words, not server truth — the consumer must not arm the 0x01A1 + // flush gate on them (RuntimeCharacterOptionsState.Replace). + onCharacterOptions?.Invoke( + p.Value.Options1, p.Value.Options2, p.Value.TrailerTruncated); onDesiredComponents?.Invoke(p.Value.DesiredComps); double receivedAt = clientTime(); diff --git a/src/AcDream.Runtime/GameRuntime.cs b/src/AcDream.Runtime/GameRuntime.cs index e1523c63..0cafe69f 100644 --- a/src/AcDream.Runtime/GameRuntime.cs +++ b/src/AcDream.Runtime/GameRuntime.cs @@ -336,10 +336,25 @@ public sealed class GameRuntime // WorldSession directly rather than through App's // LiveSessionCommandRouter/LiveCommandBus, which is what keeps // it off the S2 lock-order hazard the blast-lens review named. + // The IsDirty pre-check lives HERE, in the once-allocated hook + // lambdas, not (only) inside FlushCharacterOptions: SendBlob's + // closure environment is allocated in that method's PROLOGUE, + // ahead of any guard inside it (OP1 re-review R1), so the clean- + // tick fast path must never ENTER the method at all. Retail- + // faithful too — CPlayerModule::UseTime @0x0059A710 opens with + // the identical m_bDirty byte compare. context.Session.ConfigureAutoSaveTick( - session => FlushCharacterOptions(session, ifAutoSaveDue: true)); + session => + { + if (CharacterOwner.Options.IsDirty) + FlushCharacterOptions(session, ifAutoSaveDue: true); + }); context.Session.ConfigurePreLogoffFlush( - session => FlushCharacterOptions(session, ifAutoSaveDue: false)); + session => + { + if (CharacterOwner.Options.IsDirty) + FlushCharacterOptions(session, ifAutoSaveDue: false); + }); construction.Complete(); } @@ -364,17 +379,13 @@ public sealed class GameRuntime /// both are always populated long before the session can ever tick or /// stop. /// - /// The pre-check - /// below is retail-faithful (CPlayerModule::UseTime @0x0059A710 - /// opens with the identical m_bDirty byte compare before it ever - /// touches the FPU timer math) AND load-bearing for allocation: without - /// it, the auto-save-tick hook would allocate a fresh flush closure on - /// EVERY call — every frame, - /// for every live session, whether or not anything is actually - /// dirty — which is exactly the per-tick allocation the K4 headless - /// resource-envelope gate measures. Gating on the cheap flag first means - /// the closure below is only ever allocated on the rare tick where a - /// flush might really happen. + /// The allocation-load-bearing pre-check lives in the two HOOK LAMBDAS in the constructor, + /// not here: SendBlob's closure environment is allocated in this + /// method's prologue — ahead of any guard written inside the body (OP1 + /// re-review R1) — so keeping clean ticks out of this method entirely is + /// what protects the K4 headless per-tick allocation envelope. The check + /// below remains only as cheap idempotent defense for direct callers. /// /// private void FlushCharacterOptions(WorldSession session, bool ifAutoSaveDue) diff --git a/src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs b/src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs index a52b3fe5..dad4faf0 100644 --- a/src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs +++ b/src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs @@ -657,6 +657,17 @@ public sealed class RuntimeCharacterOptionsState private readonly TimeProvider _timeProvider; private readonly object _dirtyGate = new(); + + /// + /// R2 (OP1 re-review, 2026-08-11): bumped on EVERY + /// call — including ones that arrive while already dirty. A flush captures + /// the generation before invoking its callback and only clears + /// when the generation is unchanged after, so a + /// concurrent dirtying change that lands DURING the callback keeps the + /// module dirty and gets its own later flush instead of being silently + /// erased by the trailing clear. + /// + private long _dirtyGeneration; private uint _options1 = DefaultOptions1; private uint _options2 = DefaultOptions2; private long _revision; @@ -723,7 +734,16 @@ public sealed class RuntimeCharacterOptionsState /// wholesale overwrite would let a stale pending-save appear to persist /// a change that no longer exists locally. /// - public void Replace(uint options1, uint options2) + /// The PlayerDescription's CharacterOptions1 word. + /// The PlayerDescription's CharacterOptions2 word. + /// R3 (OP1 re-review, 2026-08-11): pass + /// false for a TRAILER-TRUNCATED PlayerDescription parse — its + /// option words are the parser's zero placeholders, not server truth, and + /// arming the flush gate on them would let the timer ship zeroed words + /// over the character's real options (the exact wipe class the latch + /// closes). The words still install (pre-existing local behavior); only + /// the flush authorization is withheld. + public void Replace(uint options1, uint options2, bool armServerSeed = true) { Volatile.Write(ref _options1, options1); Volatile.Write(ref _options2, options2); @@ -731,7 +751,8 @@ public sealed class RuntimeCharacterOptionsState lock (_dirtyGate) { _isDirty = false; - _hasServerSeed = true; + if (armServerSeed) + _hasServerSeed = true; } } @@ -865,6 +886,11 @@ public sealed class RuntimeCharacterOptionsState { lock (_dirtyGate) { + // The generation bumps on EVERY call (R2) — an in-flight flush's + // trailing clear compares generations, so a change arriving while + // the callback runs stays dirty. The timer stamp still belongs to + // the FIRST dirtying change only (retail's UseTime model). + _dirtyGeneration++; if (_isDirty) return; _isDirty = true; _firstDirtiedAt = _timeProvider.GetUtcNow(); @@ -896,14 +922,20 @@ public sealed class RuntimeCharacterOptionsState public bool TryFlush(Action flush) { ArgumentNullException.ThrowIfNull(flush); + long observedGeneration; lock (_dirtyGate) { if (!_isDirty || !_hasServerSeed) return false; + observedGeneration = _dirtyGeneration; } flush(); lock (_dirtyGate) { - _isDirty = false; + // R2: only clear when no dirtying change landed during the + // callback — otherwise the newer change keeps the module dirty + // and flushes on its own later trigger. + if (_dirtyGeneration == observedGeneration) + _isDirty = false; } return true; } @@ -920,15 +952,19 @@ public sealed class RuntimeCharacterOptionsState public bool TryFlushIfAutoSaveDue(Action flush) { ArgumentNullException.ThrowIfNull(flush); + long observedGeneration; lock (_dirtyGate) { if (!_isDirty || !_hasServerSeed) return false; if (_timeProvider.GetUtcNow() - _firstDirtiedAt < AutoSaveDelay) return false; + observedGeneration = _dirtyGeneration; } flush(); lock (_dirtyGate) { - _isDirty = false; + // R2: same generation-guarded clear as TryFlush. + if (_dirtyGeneration == observedGeneration) + _isDirty = false; } return true; } diff --git a/src/AcDream.Runtime/Session/LiveSessionEventRouter.cs b/src/AcDream.Runtime/Session/LiveSessionEventRouter.cs index f9de40c3..c7af2a14 100644 --- a/src/AcDream.Runtime/Session/LiveSessionEventRouter.cs +++ b/src/AcDream.Runtime/Session/LiveSessionEventRouter.cs @@ -207,9 +207,10 @@ public sealed class LiveSessionEventRouter : ILiveSessionEventRouting friends: social.Friends, squelch: social.Squelch, onDesiredComponents: null, - onCharacterOptions: (options1, options2) => + onCharacterOptions: (options1, options2, trailerTruncated) => { - character.Character.Options.Replace(options1, options2); + character.Character.Options.Replace( + options1, options2, armServerSeed: !trailerTruncated); character.OnCharacterOptionsChanged?.Invoke(options1, options2); }, clientTime: character.ClientTime, diff --git a/tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs b/tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs index 4df16ee6..c132fb95 100644 --- a/tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs +++ b/tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs @@ -1513,15 +1513,15 @@ public sealed class GameEventWiringTests public void WireAll_PlayerDescription_PublishesCharacterOptions() { var dispatcher = new GameEventDispatcher(); - (uint Options1, uint Options2)? observed = null; + (uint Options1, uint Options2, bool Truncated)? observed = null; GameEventWiring.WireAll( dispatcher, new ClientObjectTable(), new CombatState(), new Spellbook(), new ChatLog(), - onCharacterOptions: (options1, options2) => - observed = (options1, options2)); + onCharacterOptions: (options1, options2, trailerTruncated) => + observed = (options1, options2, trailerTruncated)); var stream = new MemoryStream(); using var writer = new BinaryWriter(stream); @@ -1541,7 +1541,7 @@ public sealed class GameEventWiringTests WrapEnvelope(GameEventType.PlayerDescription, stream.ToArray())); dispatcher.Dispatch(envelope!.Value); - Assert.Equal((0x50C4A54Au, 0x948700u), observed); + Assert.Equal((0x50C4A54Au, 0x948700u, false), observed); } private static byte[] BuildEnchantment( diff --git a/tests/AcDream.Runtime.Tests/Gameplay/RuntimeCharacterStateTests.cs b/tests/AcDream.Runtime.Tests/Gameplay/RuntimeCharacterStateTests.cs index 45c2a1f0..9a2b13de 100644 --- a/tests/AcDream.Runtime.Tests/Gameplay/RuntimeCharacterStateTests.cs +++ b/tests/AcDream.Runtime.Tests/Gameplay/RuntimeCharacterStateTests.cs @@ -625,6 +625,59 @@ public sealed class RuntimeCharacterStateTests bool flushed = await flushTask.WaitAsync(TimeSpan.FromSeconds(5)); Assert.True(flushed); Assert.True(probeCompletedPromptly); + + // R2 (OP1 re-review, 2026-08-11): the concurrent MarkDirty landed + // DURING the callback, so the trailing generation-guarded clear must + // NOT erase it — the module stays dirty and that change gets its own + // later flush instead of being silently lost. + Assert.True(options.IsDirty); + } + + [Fact] + public void TryFlush_KeepsModuleDirty_WhenADirtyingChangeLandsInsideTheCallback() + { + // R2's deterministic same-thread shape (also the re-review's NOTE-6 + // re-entrancy case): a MarkDirty issued from INSIDE the flush + // callback must survive the trailing clear via the generation token. + var options = new RuntimeCharacterOptionsState(); + options.Replace(options.Options1, options.Options2); + options.TrySetOption( + (uint)CharacterOptionId.AutoTarget, false, (_, _) => { }); + Assert.True(options.IsDirty); + + Assert.True(options.TryFlush(options.MarkDirty)); + Assert.True(options.IsDirty); + + // The retained dirty state flushes normally afterwards. + Assert.True(options.TryFlush(() => { })); + Assert.False(options.IsDirty); + } + + [Fact] + public void Replace_WithoutServerSeedArming_DoesNotAuthorizeFlush() + { + // R3 (OP1 re-review, 2026-08-11): a trailer-truncated + // PlayerDescription carries zero placeholder option words — its + // Replace installs the words but must NOT arm the flush gate. + var options = new RuntimeCharacterOptionsState(); + options.Replace(0u, 0u, armServerSeed: false); + Assert.False(options.HasServerSeed); + + // Words are zero after the truncated seed, so flipping TO true is the + // change (false would hit retail's unchanged-value no-op). + options.TrySetOption( + (uint)CharacterOptionId.AutoTarget, true, (_, _) => { }); + Assert.True(options.IsDirty); + Assert.False(options.TryFlush(() => throw new InvalidOperationException( + "a truncated-trailer seed must never authorize a blob flush"))); + + // A later COMPLETE PlayerDescription arms the gate; after re-dirtying, + // the flush proceeds. + options.Replace(0x50C4A54Au, 0x00948700u); + Assert.True(options.HasServerSeed); + options.TrySetOption( + (uint)CharacterOptionId.AutoTarget, false, (_, _) => { }); + Assert.True(options.TryFlush(() => { })); } [Fact]