From 6f48e3415274820891d4576d9a45bbe7383c0c01 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 01:13:57 +0200 Subject: [PATCH] fix(runtime,net): OP1 re-review residuals R1/R2/R3 (coordinator pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R1: the K4-load-bearing IsDirty pre-check moves into GameRuntime's two once-allocated hook lambdas — SendBlob's closure environment is allocated in FlushCharacterOptions's PROLOGUE, ahead of any guard inside the body, so the clean-tick fast path must never enter the method at all. The body keeps its check as idempotent defense only; the doc comment now describes the real mechanism instead of overclaiming. R2: RuntimeCharacterOptionsState gains a dirty-generation token. MarkDirty bumps it on EVERY call (including while already dirty); TryFlush / TryFlushIfAutoSaveDue capture it before invoking the callback and only clear IsDirty when it is unchanged after — a dirtying change landing DURING a flush (cross-thread, or re-entrant from the callback itself, the re-review's NOTE-6 case) now stays dirty and flushes on its own later trigger instead of being silently erased by the trailing clear. The S2 interleaving test now asserts the retained dirty state it previously ignored; a deterministic re-entrancy test pins the same-thread shape. R3: a trailer-truncated PlayerDescription parse carries zero placeholder option words, not server truth — GameEventWiring now forwards TrailerTruncated, LiveSessionEventRouter passes armServerSeed: !trailerTruncated, and Replace withholds the 0x01A1 flush authorization for truncated seeds while still installing the words (pre-existing local behavior unchanged). Newly wire-reaching via the R1/MF-1 timer, hence closed now rather than left a NOTE. Full Release suite: 12,870 passed / 4 skipped / 0 failed. Co-Authored-By: Claude Fable 5 --- src/AcDream.Core.Net/GameEventWiring.cs | 8 ++- src/AcDream.Runtime/GameRuntime.cs | 37 ++++++++----- .../Gameplay/RuntimeCharacterState.cs | 44 +++++++++++++-- .../Session/LiveSessionEventRouter.cs | 5 +- .../GameEventWiringTests.cs | 8 +-- .../Gameplay/RuntimeCharacterStateTests.cs | 53 +++++++++++++++++++ 6 files changed, 130 insertions(+), 25 deletions(-) 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]