erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(runtime,net): OP1 re-review residuals R1/R2/R3 (coordinator pass)

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-11 01:13:57 +02:00
parent b236a44279
commit 6f48e34152
6 changed files with 130 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

8
src/AcDream.Core.Net/GameEventWiring.cs
View file

@ -80,7 +80,7 @@ public static class GameEventWiring
FriendsState? friends = null,
SquelchState? squelch = null,
Action<IReadOnlyList<(uint Id, uint Amount)>>? onDesiredComponents = null,
Action<uint /*options1*/, uint /*options2*/>? onCharacterOptions = null,
Action<uint /*options1*/, uint /*options2*/, bool /*trailerTruncated*/>? onCharacterOptions = null,
Func<double>? 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();

37
src/AcDream.Runtime/GameRuntime.cs
View file

@ -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.
/// <para>
/// The <see cref="RuntimeCharacterOptionsState.IsDirty"/> pre-check
/// below is retail-faithful (<c>CPlayerModule::UseTime @0x0059A710</c>
/// opens with the identical <c>m_bDirty</c> 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 <see cref="LiveSessionController.Tick"/> 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 <see cref="RuntimeCharacterOptionsState.
/// IsDirty"/> pre-check lives in the two HOOK LAMBDAS in the constructor,
/// not here: <c>SendBlob</c>'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.
/// </para>
/// </summary>
private void FlushCharacterOptions(WorldSession session, bool ifAutoSaveDue)

44
src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs
View file

@ -657,6 +657,17 @@ public sealed class RuntimeCharacterOptionsState
private readonly TimeProvider _timeProvider;
private readonly object _dirtyGate = new();
/// <summary>
/// R2 (OP1 re-review, 2026-08-11): bumped on EVERY <see cref="MarkDirty"/>
/// call — including ones that arrive while already dirty. A flush captures
/// the generation before invoking its callback and only clears
/// <see cref="IsDirty"/> 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.
/// </summary>
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.
/// </summary>
public void Replace(uint options1, uint options2)
/// <param name="options1">The PlayerDescription's CharacterOptions1 word.</param>
/// <param name="options2">The PlayerDescription's CharacterOptions2 word.</param>
/// <param name="armServerSeed">R3 (OP1 re-review, 2026-08-11): pass
/// <c>false</c> 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.</param>
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;
}

5
src/AcDream.Runtime/Session/LiveSessionEventRouter.cs
View file

@ -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,

8
tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs
View file

@ -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(

53
tests/AcDream.Runtime.Tests/Gameplay/RuntimeCharacterStateTests.cs
View file

@ -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]