diff --git a/docs/plans/2026-08-10-options-panel-campaign.md b/docs/plans/2026-08-10-options-panel-campaign.md index 97a8a030..5764fefd 100644 --- a/docs/plans/2026-08-10-options-panel-campaign.md +++ b/docs/plans/2026-08-10-options-panel-campaign.md @@ -415,8 +415,8 @@ before anything builds on them. | Slice | Status | Commit(s) | Review | Gate | |---|---|---|---|---| -| OP1 | — | | | | -| OP2 | — | | | | +| OP1 | CLOSED (residuals R1/R2 owed to a coordinator pass) | `86c0a7e0` + fixes `09029f9f` | dual APPROVE-WITH-FIXES (`2026-08-10-op1-review-{mechanism,blast}.md`) → re-review CLOSED (`2026-08-11-op1-rereview.md`) | automated only — n/a | +| OP2 | REWORK round 1 in flight | `df9c7a35` (REJECTED) | dual REJECT (`2026-08-11-op2-review-{mechanism,blast}.md`) | | | OP3 | — | | | | | OP4 | — | | | | | OP5 | — | | | | diff --git a/docs/research/2026-08-11-op1-rereview.md b/docs/research/2026-08-11-op1-rereview.md new file mode 100644 index 00000000..2de3736c --- /dev/null +++ b/docs/research/2026-08-11-op1-rereview.md @@ -0,0 +1,485 @@ +# 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 calls + `context.Session.ConfigureAutoSaveTick(...)` / + `ConfigurePreLogoffFlush(...)` on the same object it assigns to `Session` + at `:315`. +- `src/AcDream.Runtime/Session/LiveSessionController.cs:319, 331` — both + hooks are `internal`, single-assignment, null-checked. +- Tick half: `LiveSessionController.cs:482` calls + `InvokeAutoSaveTick(scope.Session)` inside `RunTopLevel`, **after** the + `IsCurrent(scope, generation)` guard at `:467`. +- Stop half: `LiveSessionController.cs:686-687` calls + `InvokePreLogoffFlush(activeScope.Session)` as the first statement of + `StopCore`, before the generation bump and before `DrainRetiredScope` → + `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's `CPlayerSystem::LogOffCharacter` + does. + +**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`): + +```csharp +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 through `LiveSessionController`'s own tick/stop transaction + (`ConfigureAutoSaveTick`/`ConfigurePreLogoffFlush`, wired once by + `GameRuntime`'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 `OnChanged` cases 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 mutating + `PlayerModule` state, 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 + `0x34` `HearPkDeathMessages` row is still the ACE-sourced one (the new + MF-3 theory pins it at O2 `0x02000000` with an inline comment naming the + register row), and the 16-entry `ClientDefaultOnIds` array bounded at + `0x2A` — the source of AP-194's `0x2D`/`0x2F`/`0x32` disagreement — 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 — + +```csharp +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, Action)` 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.