Focused re-review verifies all ten fix dispositions genuinely close their findings (fellowship order re-derived from OnChanged @0x0059A8E0; the seed latch confirmed upstream of every one of the three blob call sites; TS-71 retirement and TS-73 accuracy checked). Residuals owed to a small coordinator pass, none reopening: R1 closure hoisting defeats the per-tick allocation guard (move the IsDirty check into the ctor lambdas); R2 concurrent MarkDirty during a flush is silently erased by the trailing clear (needs the dirty-generation token); R3 NOTE — a truncated-trailer PlayerDescription (options words zero) arms the seed latch, newly wire-reaching via the timer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
26 KiB
Campaign OP slice OP1 — fix-round closure re-review
Commit under review: 09029f9f ("fix(runtime,net): OP1 review fixes —
server-seed gate, tick-wired auto-save/logout flush, fellowship mutual
exclusion").
Closes (claimed): every finding in
docs/research/2026-08-10-op1-review-mechanism.md (MF-1/MF-2/MF-3) and
docs/research/2026-08-10-op1-review-blast.md (M1, S1–S6).
Mode: read-only. No build, no test run, no launch. Git reads only.
Scope: closure check against the diff and the post-commit code — not a
fresh review of OP1.
Date: 2026-08-11.
Verdict: CLOSED. All ten dispositions genuinely close their finding's failure scenario. No finding is merely gestured at. 2 SHOULD-FIX and 5 NOTE residuals below, all introduced by the fixes or adjacent to them; none reopens a review finding.
1. Per-finding closure
MF-1 (mechanism MUST-FIX 1) — TS-71's false deferral rationale · CLOSED
The fixer took the stronger of the two offered remedies: wired the triggers and retired the row.
Seam, verified end to end:
src/AcDream.Runtime/GameRuntime.cs:339-342— the constructor callscontext.Session.ConfigureAutoSaveTick(...)/ConfigurePreLogoffFlush(...)on the same object it assigns toSessionat:315.src/AcDream.Runtime/Session/LiveSessionController.cs:319, 331— both hooks areinternal, single-assignment, null-checked.- Tick half:
LiveSessionController.cs:482callsInvokeAutoSaveTick(scope.Session)insideRunTopLevel, after theIsCurrent(scope, generation)guard at:467. - Stop half:
LiveSessionController.cs:686-687callsInvokePreLogoffFlush(activeScope.Session)as the first statement ofStopCore, before the generation bump and beforeDrainRetiredScope→SessionScope.DrainTeardown→operations.DisposeSession→WorldSession.Dispose→CharacterLogOff.BuildRequestBody(src/AcDream.Core.Net/WorldSession.cs:2966). So the blob really does precede the logoff request, as retail'sCPlayerSystem::LogOffCharacterdoes.
Both hosts, zero host edits — confirmed. new LiveSessionController has
exactly two production sites, both inside GameRuntime
(GameRuntime.cs:167-168), so no production controller can miss the wiring.
The two tick sites are unchanged by this commit:
src/AcDream.Headless/Hosting/HeadlessSessionHost.cs:341 and
src/AcDream.App/World/RetailLiveFrameCoordinator.cs:61. The diff touches
no file under src/AcDream.Headless/, and its two App files
(LiveSessionRuntimeFactory.cs, CurrentGameRuntimeCommandAdapter.cs) are
a binding factory and a command adapter, not a host loop.
Not just hook mechanics. Beyond the six isolated hook tests added to
tests/AcDream.Runtime.Tests/Session/LiveSessionControllerTests.cs, two
end-to-end tests drive the real GameRuntime wiring:
Session_Tick_AutoFlushesTheDirtyBlob_OnceThe480sTimerIsDue and
Stop_AutoFlushesTheDirtyBlob_BeforeTheCharacterLogoffRequest
(tests/AcDream.Runtime.Tests/Session/DirectGameRuntimeCommandAdapterTests.cs,
new in this commit) — both assert the 0x01A1 opcode at body offset 8, and
the second asserts flush-before-logoff by capture order.
The per-tick allocation pre-check is only PARTLY real — see R1.
MF-2 (mechanism SHOULD-FIX 2) — OnChanged's side-effect switch · CLOSED
I read CPlayerModule::OnChanged @0x0059A8E0 myself at
docs/research/named-retail/acclient_2013_pseudo_c.txt:423491-423554
rather than trusting either review. The body is:
0059a8e9 CM_UI::SendNotice_PlayerOptionChanged(arg2);
0059a904 switch (arg2) {
0059a96a case 2: if (PlayerModule::IgnoreFellowshipRequests(this)) SetFellowshipAutoAcceptRequests(this, 0);
0059a921 case 4: SmartBox::EnableWeather(...)
0059a916 case 5: LScape::SetDay(...)
0059a954 case 7: ClientCombatSystem::TrackTarget(...)
0059a980 case 0x12: if (PlayerModule::FellowshipAutoAcceptRequests(this)) SetIgnoreFellowshipRequests(this, 0);
0059a940 case 0x30: LScape::m_fFogEnabled = ...
}
0059a99d if (CPlayerModule::IsAutoSaveOption(...)) { Event_PlayerOptionChangedEvent(...); return; }
0059a9c1 if (!m_bDirty) { m_bDirty = 1; m_timeFirstDirtied = Timer::cur_time; }
Checked point by point against
src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs:785-821:
| Question | Retail | acdream | Verdict |
|---|---|---|---|
| Which id clears which | case 2 (Ignore) clears AutoAccept; case 0x12 (AutoAccept) clears Ignore |
:800-806 id 0x02 → clears FellowshipAutoAcceptRequests; :807-813 id 0x12 → clears IgnoreFellowshipRequests |
✅ not transposed |
| Guard | reads the option after the accessor's own write, i.e. "if now ON" | writes at :795, then tests value at :800/:807 — equivalent, and the comment says why |
✅ |
| Turning one OFF | guard false, no clear | value && short-circuits |
✅ (TrySetOption_TurningOffAFellowshipOption_NeverTriggersTheClear) |
| Other already clear | the nested Set… accessor's own unchanged-value early return fires — no OnChanged, no second 0x0005 |
the recursive TrySetOption hits the same early return at :792-793 |
✅ (..._WhenTheOtherIsAlreadyOff_SendsOnlyThePrimary) |
| Wire order | the nested call finishes its whole OnChanged (including its Event_PlayerOptionChangedEvent) before the outer call reaches step 3 → clear first, primary second |
recursion at :800-813 precedes sendAutoSave(characterOptionId, value) at :815-816 |
✅ (both order tests assert the exact two-element sent sequence) |
| Termination | the clear passes 0, which cannot re-arm either "if now true" guard |
same | ✅ one level, always |
Id/mask spot-checks against the verbatim header:
IgnoreFellowshipRequests_PlayerOption = 0x2 /
FellowshipAutoAcceptRequests_PlayerOption = 0x12
(docs/research/named-retail/acclient.h:4167, 4183) match
src/AcDream.Core.Net/Messages/SocialActions.cs:366, 382;
IgnoreFellowshipRequests_CharacterOption = 0x8 /
AutoAcceptFellowRequest_CharacterOption = 0x20000000
(acclient.h:3409, 3430) match
src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs:113, 129. Both rows
carry IsAutoSave = true, so the recursion can never reach MarkDirty —
as the remark claims.
The four presentation cases are correctly left unported and correctly filed (TS-73, §2).
MF-3 (mechanism SHOULD-FIX 3) — no id→(word, mask) pin · CLOSED
tests/AcDream.Runtime.Tests/Gameplay/CharacterOptionTableTests.cs adds
WordAndMask_MatchesIndependentTranscriptionOfVerbatimAcclientEnums, a
[Theory] with 53 [InlineData] rows — one per id in the 0x00..0x34
space — asserting (IsOptions1, Mask). I spot-checked the five
transposition-prone rows the mechanism review singled out
(IgnoreTradeRequests O1 0x20000, DisplayAge O2 0x20,
DisplayNumberDeaths O2 0x10, UseCraftSuccessDialog O1 0x80000000,
ConfirmVolatileRareUse O2 0x40000); all match both the header and the
table. WordAndMask_AreAllPairwiseDistinct additionally closes blast NOTE
N7 with the Distinct().Count() == 53 check the reconstruction test
structurally cannot do. See R6 for the one column still unpinned.
M1 (blast MUST-FIX) — no server-seed gate on the blob flush · CLOSED
The latch: _hasServerSeed (RuntimeCharacterState.cs:665) starts false,
is set only by Replace (:726-736) and cleared only by
ResetSession (:944-955). Both TryFlush (:901) and
TryFlushIfAutoSaveDue (:925) test !_isDirty || !_hasServerSeed and
return false without invoking the callback.
Traced: no path can build or send a 0x01A1 pre-seed. There is exactly
one wire method (WorldSession.SendSetCharacterOptions,
src/AcDream.Core.Net/WorldSession.cs:2216) and exactly one builder
(SocialActions.BuildSetCharacterOptions), whose only non-test caller is
that method. SendSetCharacterOptions has three call sites in src/,
and every one is the body of a TryFlush/TryFlushIfAutoSaveDue callback,
so the gate sits upstream of both the Capture and the send on all three:
| Trigger | Site | Gated by |
|---|---|---|
Explicit SaveOptions, graphical |
src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:359-372 |
TryFlush |
Explicit SaveOptions, headless/bot |
src/AcDream.Runtime/Session/DirectGameRuntimeCommandAdapter.cs:691-703 |
TryFlush |
| New 480 s timer tick and new pre-logoff flush | src/AcDream.Runtime/GameRuntime.cs:386-403 (one shared SendBlob) |
TryFlushIfAutoSaveDue / TryFlush |
Options.Replace likewise has exactly one production caller —
src/AcDream.Runtime/Session/LiveSessionEventRouter.cs:212, the
PlayerDescription route — so the latch cannot be armed by anything but a
parsed server description. (One caveat on which descriptions arm it: R3.)
Nothing is lost when the gate refuses: _isDirty is untouched, so the
pending change survives until a real seed arrives — asserted by
TryFlush_RefusesBeforeServerSeed_EvenWhenDirty_ThenSucceedsAfterSeed,
TryFlushIfAutoSaveDue_RefusesBeforeServerSeed_EvenAtThreshold,
ReconnectSequence_ResetSessionClearsSeed_NewReplaceUnblocksFlushAgain
(RuntimeCharacterStateTests.cs) and
SaveOptions_RefusesBeforeServerSeed_EvenWhenDirty_ThenSucceedsAfterSeed
(DirectGameRuntimeCommandAdapterTests.cs).
S1 (blast) — CH3/CH4 regression test mirrored the pre-OP1 shape · CLOSED
tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs:498-505 now
binds characterState.Options.TrySetOption(id, value, sendAutoSave: …) —
the actual production seam — instead of the hand-rolled
SetOptionBit + sent.Add substitute. The test therefore now exercises
the unchanged-value early return and the MF-2 side effect, which was the
point of the finding.
S2 (blast) — flush callback invoked under _dirtyGate · CLOSED (with R2)
Both methods now use decide-under-lock / callback-outside-lock /
clear-under-lock (RuntimeCharacterState.cs:896-909 and the identical
shape at :920-934):
lock (_dirtyGate) { if (!_isDirty || !_hasServerSeed) return false; } // :899-902
flush(); // :903
lock (_dirtyGate) { _isDirty = false; } // :904-907
return true;
The callback runs outside the lock on every path — there is no branch
that still invokes it inside — and the throw path leaves the module dirty
(TryFlush_PreservesDirtyState_WhenTheCallbackThrows). _dirtyGate is now
a strict leaf lock: no site holds it while acquiring another. The tick
path's order is LiveSessionController._gate → _dirtyGate (Tick holds
_gate across InvokeAutoSaveTick, LiveSessionController.cs:453-484),
the same direction as App's router, so the named inversion is gone.
On the re-entrancy NOTE the fixer left open: the same-thread claim is
correct. A re-entrant MarkDirty from inside a flush callback sets
_isDirty = true and is then erased by the trailing clear — identical
outcome before and after the restructure, because C# locks are reentrant.
It is unreachable today: all three callbacks do exactly
CharacterOptionsBlobSource.Capture + WorldSession.SendSetCharacterOptions,
and that send is a pure outbound enqueue (SendGameAction) that pumps no
inbound handlers, so nothing on the stack can call MarkDirty,
TrySetOption or Replace. The cross-thread half of that claim is wrong
— see R2.
S3 (blast) — ownership ledger blind to the dirty flag · CLOSED
RuntimeCharacterOwnershipSnapshot gains OptionsAreClean
(RuntimeCharacterState.cs:33, default true, appended last so no
positional call site breaks) and IsConverged requires it (:50);
CaptureOwnership populates it from !Options.IsDirty (:235). Safe
against false non-convergence: the only consumers are the disposal-stage
ledger (GameRuntime.cs:708, 736) and RuntimeGameplayOwnership.Capture,
and stage 6 disposes CharacterOwner (→ Options.ResetSession() →
_isDirty = false) before reading it.
S4 (blast) — SaveOptions encoded a bool in PrimaryObjectId · CLOSED
DirectGameRuntimeCommandAdapter.SaveOptions now emits
(Accepted, objectId 0) unconditionally
(DirectGameRuntimeCommandAdapter.cs:712-716), matching
CurrentGameRuntimeCommandAdapter.SaveOptions
(src/AcDream.App/Runtime/CurrentGameRuntimeCommandAdapter.cs:706-710).
K2 event-stream consumers checked: PrimaryObjectId is carried by
GameRuntimeEvents.cs:36/144/242 into the ordered bot-facing stream and is
read by no production consumer — grep over src/AcDream.Headless
finds no ResultObjectId use at all. The only assertions on it are the two
new Assert.Equal(0u, …) lines in
DirectGameRuntimeCommandAdapterTests.cs. Nothing depended on the 1u
encoding, so dropping it is inert outside the shape fix itself.
S5 (blast) — re-seed discarded pending intent but left the module dirty · CLOSED
Replace now clears _isDirty alongside arming the seed
(RuntimeCharacterState.cs:726-736), with the decision recorded in the
member's XML doc: a wholesale re-seed has no partial-merge path in retail's
PlayerModule either, so the server's words are current and a later flush
would only echo them. Pinned by
Replace_ClearsDirtyState_ServerTruthSupersedesPendingLocalIntent. This is
the "record the decision plus a test" outcome the finding asked for.
S6 (blast) — the same bits defined twice with no cross-check · CLOSED
CharacterOptionTable_AgreesWithPlayerDescriptionParserEnums
(CharacterOptionTableTests.cs) asserts each table entry against
(uint)PlayerDescriptionParser.CharacterOptions1/2 members directly. I
verified the comment's completeness claim: CharacterOptions1 has three
non-None/Default members (AllowGive, HearAllegianceChat,
DragItemOnPlayerOpensSecureTrade) and CharacterOptions2 has five — all
eight appear in the theory
(src/AcDream.Core.Net/Messages/PlayerDescriptionParser.cs:206-236). Edit
either definition alone and the test fails, which is exactly the CH3 class.
2. Register bookkeeping
The register diff is exactly two single-line hunks (git show -U0:
@@ -349 +349 @@ and @@ -353 +353 @@), so nothing else in the file moved.
- TS-71 retirement — accurate. The row is deleted, not amended, and the
section-header note reads "both remaining
SetCharacterOptions (0x01A1)flush triggers (the 480 s auto-save timer, the pre-logoff flush) are now wired throughLiveSessionController's own tick/stop transaction (ConfigureAutoSaveTick/ConfigurePreLogoffFlush, wired once byGameRuntime's constructor)". Every clause is true of the code as landed (§1 MF-1). The active-row count stays 41 because TS-73 replaces it. Two narrow paths where the logout half does not fire are R4/R5 — neither is on a normal shutdown, so the retirement stands. - TS-73 — truthful about what remains unported. It names the four
omitted
OnChangedcases by id and by the retail consumer each drives (0x04→SmartBox::EnableWeather,0x05→LScape::SetDay,0x07→ClientCombatSystem::TrackTarget,0x30→LScape::m_fFogEnabled); I confirmed all four against the decomp body quoted in §1 MF-2. It states correctly that the two ported cases are the only ones mutatingPlayerModulestate, gives the right file/member pointer, anchors to the named symbol + address, and describes the user-visible consequence accurately (bit writes and auto-saves fine, no local presentation change). Its "pre-anchored to OP4" rationale points at an existing plan section rather than inventing a deferral reason — the opposite of the defect MF-1 was filed against. - AP-193 / AP-194 — untouched and still accurate. Neither row is in the
diff. Nothing in this commit changes the table data they describe: the
0x34HearPkDeathMessagesrow is still the ACE-sourced one (the new MF-3 theory pins it at O20x02000000with an inline comment naming the register row), and the 16-entryClientDefaultOnIdsarray bounded at0x2A— the source of AP-194's0x2D/0x2F/0x32disagreement — is unchanged.
3. New blast radius from the fixes
All three axes named in the brief were walked.
The new LiveSessionController seams. ConfigureAutoSaveTick /
ConfigurePreLogoffFlush are internal, and grep finds no consumer
outside GameRuntime.cs:339-342 and the new tests. Nothing else observes
_autoSaveTickHook/_preLogoffFlushHook.
Can the flush tick fire for a non-current generation? No. Tick() holds
_gate for its whole body (LiveSessionController.cs:453-484), every
mutation of _scope/_generation requires _gate, and InvokeAutoSaveTick
runs after the IsCurrent(scope, generation) early-out at :467 inside the
same lock hold — the scope cannot be retired between the check and the
flush. It also cannot fire pre-Start or post-Stop: :455-458 returns
when _disposed, !_inWorld, _scope is null, or _operationDepth != 0.
A throwing hook is caught and logged (:487-500), pinned by
Tick_AutoSaveHookThrowing_DoesNotFailTheTickOrTearDownTheSession.
TrySetOption's widened signature. Three production call sites, all
updated to pass WorldSession.SendSetSingleCharacterOption as a method
group: LiveSessionRuntimeFactory.cs:348-354,
DirectGameRuntimeCommandAdapter.cs:666-673, and the router binding they
share. CurrentGameRuntimeCommandAdapter reaches it only through the bus,
so it needs no change. No other caller exists in src/. The widening cannot
change what any existing caller sends, because every caller now forwards the
callback's own (id, value) rather than a closed-over pair.
S4's result shape. Covered in §1 S4 — no consumer.
Two flush bodies, one owner graph. The graphical closure builds from
_domain.Character/_domain.Inventory and the new hook from
CharacterOwner/InventoryOwner;
src/AcDream.App/Composition/SessionPlayerComposition.cs:1070-1076 passes
the same canonical d.Character/d.Inventory owners into
LiveSessionDomainRuntime, so both bodies read identical state. A double
flush is impossible in either order because TryFlush clears _isDirty
under the lock.
4. Residuals
R1 — SHOULD-FIX: the per-tick allocation pre-check guards the delegate, not the closure
src/AcDream.Runtime/GameRuntime.cs:380-404. SendBlob captures two
things: this (for CharacterOwner/InventoryOwner) and the session
parameter. Because it is converted to a delegate, its closure environment
must be a class, and Roslyn instantiates a method-scope environment holding
a captured parameter in the method prologue — i.e. ahead of the
if (!options.IsDirty) return; guard at :383-384. The guard therefore
elides the Action delegate allocation on a clean tick but not the display
class, so FlushCharacterOptions still allocates once per
LiveSessionController.Tick() per live session — the exact per-tick
allocation the doc comment at :370-377 claims it prevents ("the closure
below is only ever allocated on the rare tick where a flush might really
happen"). At 30 sessions that is the dimension the K4 headless
resource-envelope gate measures.
A one-line fix that is genuinely zero-alloc: move the guard up into the two
constructor lambdas, which capture only this and are allocated once —
context.Session.ConfigureAutoSaveTick(
session => { if (CharacterOwner.Options.IsDirty) FlushCharacterOptions(session, true); });
so FlushCharacterOptions (and its prologue) is never entered on a clean
tick. Alternatively give RuntimeCharacterOptionsState a
TryFlush<TState>(TState, Action<TState>) overload fed by a cached static
lambda. Caveat: this review is read-only, so I did not compile to
confirm the lowering — verify with a compiled allocation check before
rewriting. Either way the doc comment's absolute claim needs softening.
R2 — SHOULD-FIX: S2's restructure turns a blocking-but-correct concurrent MarkDirty into a silent lost update
The commit message's disposition on mechanism NOTE 6 says the re-entrancy
outcome is "the same whether the callback runs inside or outside the lock".
True for the same-thread re-entrant case; false for the cross-thread
case. Pre-fix, a concurrent MarkDirty blocked on _dirtyGate until the
flush finished and _isDirty had been cleared, then set it — nothing lost.
Post-fix it runs during flush() and is erased by the trailing
_isDirty = false at RuntimeCharacterState.cs:904-907. If the toggle
landed after CharacterOptionsBlobSource.Capture read the words, that
option change is silently dropped until something else dirties the module.
The commit's own new test constructs exactly that interleaving —
TryFlush_ReleasesTheDirtyGate_DuringTheCallback_SoAConcurrentMarkDirtyDoesNotBlock
(tests/AcDream.Runtime.Tests/Gameplay/RuntimeCharacterStateTests.cs:596-628)
proves the concurrent MarkDirty completes mid-flush, then asserts nothing
about IsDirty afterwards.
Whichever threading model actually holds, one of the two claims in the
disposition is wrong: if two threads can reach these seams, this lost update
is live; if only Runtime's single update thread can (memory #368), then
the _gate→_dirtyGate inversion S2 was filed against was never reachable
either. Worth deciding and recording before OP4 puts an Options panel on
this seam. The per-dirty-period generation token the fixer already named
closes this and NOTE 6 together (capture the revision under the first lock;
clear only if it is unchanged under the second).
R3 — NOTE: the seed latch arms on a truncated PlayerDescription too
onCharacterOptions?.Invoke(p.Value.Options1, p.Value.Options2)
(src/AcDream.Core.Net/GameEventWiring.cs:732) fires for every parse
that returns non-null, and PlayerDescriptionParser.TryParse yields
options1 = options2 = 0 when the trailer is shorter than 8 bytes at the
option block (src/AcDream.Core.Net/Messages/PlayerDescriptionParser.cs:369-372)
or when that read throws (:450-466, which still returns the record, with
TrailerTruncated = true). LiveSessionEventRouter.cs:212 then calls
Replace(0, 0) — zeroing both words and arming HasServerSeed. The local
zeroing is pre-existing (it would already break the Turbine membership
gate); what is new is that MF-1's timer makes it wire-reaching, so 480 s
later the auto-save would ship options1 = options2 = 0 to ACE — M1's
failure class through a different door. TrailerTruncated already exists
and is unused by the wiring; gating the Replace (or at least the latch) on
it is the natural close. Reachability against ACE is low — a real
PlayerDescription always carries the trailer.
R4 — NOTE: a deferred Stop/Dispose skips the pre-logoff flush
Schedule sets _inWorld = false
(src/AcDream.Runtime/Session/LiveSessionController.cs:775) before the
pending operation later runs StopCore, whose flush is gated on _inWorld
(:686). So a Stop()/Dispose() requested while _operationDepth != 0 —
i.e. re-entrantly, from inside another operation's callback — silently drops
the logout flush. Not on either host's normal shutdown:
GameRuntime.StopSession (GameRuntime.cs:564-572) calls
Session.Dispose() from outside any operation and throws if the shutdown
was deferred, and adapter.Session.Stop(generation) likewise reaches
RunTopLevel(StopCore) directly. Recorded so the gap is known rather than
rediscovered.
R5 — NOTE: the "pre-logoff" hook also fires on reconnect and after a failed tick
StopCore is reached from ReconnectCore
(LiveSessionController.cs:530) and StopAfterFailure (:665) as well as
Stop/DisposeCore, so the hook fires whenever an in-world scope is torn
down — including a mid-session reconnect and a tick that just threw. Saving
pending options before a session replacement is arguably right, and a send
on a dead socket is caught and logged (:706-719), so this is benign; but
the register row and the XML doc both describe the hook purely as retail's
LogOffCharacter site, which is narrower than what it actually does.
R6 — NOTE: the id↔name column still has no id-by-id pin
MF-3 closes the (word, mask) column, but all three transcribed
arrays/theories in CharacterOptionTableTests.cs key on CharacterOptionId
members, not on numeric wire ids. Swap two values in the
CharacterOptionId enum
(src/AcDream.Core.Net/Messages/SocialActions.cs:362-417) and both the
table registration and every test lookup move together — every test stays
green while the client sends the wrong id to ACE.
CoversTheCompleteIdSpace pins contiguity 0x00..0x34 but not the name→id
assignment. A [InlineData((uint)0x1D, CharacterOptionId.DisplayAge)]-shaped
theory transcribed from docs/research/named-retail/acclient.h:4162-4218
would close it; that column was verified by hand this round and last, so
nothing is wrong today.
R7 — NOTE (minor): the hook fields are written without _gate
ConfigureAutoSaveTick/ConfigurePreLogoffFlush
(LiveSessionController.cs:319, 331) assign
_autoSaveTickHook/_preLogoffFlushHook outside _gate, while
Tick/StopCore read them under it. There is no formal happens-before from
the constructing thread to the first ticking thread. Practically safe —
GameRuntime construction is fully ordered before any host can tick, and
that handoff carries its own synchronization — but a lock (_gate) around
the two assignments would cost nothing.
5. Verdict
CLOSED. Ten findings, ten real closures: M1's latch sits upstream of the
single builder on all three flush triggers; MF-1 reaches both hosts through
GameRuntime → LiveSessionController with zero host edits and end-to-end
tests, not just hook mechanics; MF-2 matches OnChanged @0x0059A8E0 on
which-clears-which, the already-clear short-circuit, and the clear-before-
primary wire order, all read out of the decomp directly; MF-3, S1, S3, S4,
S5 and S6 each close their stated failure scenario and are pinned by a test
that would actually fail on regression. The register is honest: TS-71's
retirement matches the code, TS-73 states precisely what is still unported
and anchors it to an existing plan section, and AP-193/AP-194 are untouched
and still accurate.
Two SHOULD-FIX residuals arise from the fixes themselves — R1 (the per-tick
allocation guard does not cover the closure environment, and the doc comment
overclaims) and R2 (S2's restructure trades a blocking-but-correct
concurrent MarkDirty for a silent lost update, and the disposition that
waves it off is only half true). Neither reopens a finding; both should be
settled before OP4 wires the Character tab onto this seam.