docs: OP1 re-review CLOSED — ledger records slice OP1 complete
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>
This commit is contained in:
Erik
parent
09029f9f4b
commit
8a05fda445
2 changed files with 487 additions and 2 deletions
485
docs/research/2026-08-11-op1-rereview.md
Normal file
485
docs/research/2026-08-11-op1-rereview.md
Normal file
|
|
@ -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>(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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue