CC2 review fix round: latch scope narrowed, AD-100, creationFailed reason key
F1 (MEDIUM): the correlation-latch docs claimed replies are never misattributed; in truth an overlapping send OVERWRITES the latch and the first reply routes to the newest request's event. Narrowed all three doc sites to the exact contract (single outstanding request; overlap refusal is CC3's Runtime verification gate, retail's DoFinish UNDEF-state rule) and pinned the overwrite behavior with OverlappingSend_OverwritesTheLatch_ReplyRoutesToNewestRequest. F2 (LOW): filed register AD-100 for the drop-unless-armed deviation — retail's Handle_CharGenVerificationResponse@0x0055E8B0 has no armed gate and processes whatever arrives against its persistent verification state. F3 (LOW): doc note in CharacterCreate.cs — ACE double-sends NameInUse (IsCharacterNameAvailable runs twice; the first callback's return exits only the lambda), so the second reply hitting the drop path during a connected gate is EXPECTED, not a defect. F4 (LOW): creationFailed's enum-member key renamed name -> reason and the ATTEMPTED character name added as name, before any consumer shipped — one status vocabulary must not give the same key two meanings (characterCreated.name is a character name). Contract, writer, tailer, and shape-pinning tests updated in lockstep. F5 (LOW): the thread-id probe-note pointer now cites ProbeNetLogOutbound's doc comment, where the note actually lives. Fidelity fold (reviewer's positive note): the latch is retail's OWN discriminator one layer down — 0x0055E8B0 case 1 branches on GetVerificationState()==PENDING (create) vs not (restore) — now cited in both the latch doc and CharGenVerificationResponse.cs. Core.Net 994, Runtime 1667, Launcher.Core 324, all green Release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
5eaad2c88c
commit
e77ebf100f
12 changed files with 143 additions and 32 deletions
File diff suppressed because one or more lines are too long
|
|
@ -170,7 +170,7 @@ line, writer opens `FileShare.Read`, tailer opens
|
|||
`enteredWorld{characterId,characterName}`, `pluginLoaded{plugin}`,
|
||||
`pluginFailed{plugin,error}`,
|
||||
`loginCommandFailed{commandIndex,command,error}`,
|
||||
`characterCreated{guid,name}`, `creationFailed{code,name}`,
|
||||
`characterCreated{guid,name}`, `creationFailed{code,reason,name}`,
|
||||
`disconnected{reason}`,
|
||||
`exited{code,reason}` — every line carries `"v":1`, `"e"`, `"t"`
|
||||
(ISO-8601 UTC), `"sessionId"`. `secondsGreyedOut` is a uint on BOTH
|
||||
|
|
@ -189,11 +189,14 @@ that payload's own field names and to read distinctly from
|
|||
`enteredWorld` — a freshly created character is logged straight in by
|
||||
retail without a fresh `characterList` (see that type's doc comment), so
|
||||
`characterCreated` can precede an `enteredWorld` for the same character
|
||||
rather than replacing it. `creationFailed{code,name}` fires on any non-Ok
|
||||
reply: `code` is the raw wire `CharGenVerificationResponse.Code` value,
|
||||
`name` is that code's enum member name (e.g. `"NameInUse"`) so a reader
|
||||
gets a stable readable reason without hard-coding the numeric mapping
|
||||
itself.
|
||||
rather than replacing it. `creationFailed{code,reason,name}` fires on any
|
||||
non-Ok reply: `code` is the raw wire `CharGenVerificationResponse.Code`
|
||||
value, `reason` is that code's enum member name (e.g. `"NameInUse"`) so a
|
||||
reader gets a stable readable reason without hard-coding the numeric
|
||||
mapping itself, and `name` is the ATTEMPTED character name so a launcher
|
||||
can render "the name Bob is taken". (CC2 review F4: the enum member
|
||||
originally rode the `name` key, colliding in meaning with
|
||||
`characterCreated.name`; renamed before any consumer shipped.)
|
||||
|
||||
`loginCommandFailed.commandIndex` is the zero-based index in the configured
|
||||
`loginCommands` array. `command` is the exact configured line and `error` is
|
||||
|
|
|
|||
|
|
@ -25,6 +25,13 @@ namespace AcDream.Core.Net.Messages;
|
|||
/// from "create response" by opcode or shape alone — <c>WorldSession</c>
|
||||
/// disambiguates by tracking which outbound request (restore vs. create) it
|
||||
/// is awaiting a reply to (see <c>WorldSession</c>'s awaiting-request latch).
|
||||
/// That latch is not merely a reasonable design — it is retail's OWN
|
||||
/// mechanism: <c>Handle_CharGenVerificationResponse@0x0055E8B0</c> case 1
|
||||
/// branches on the client's persistent chargen state,
|
||||
/// <c>GetVerificationState() == PENDING</c> → new <c>CharacterIdentity</c>
|
||||
/// + <c>AddIdentity</c> (a create it initiated), else → unpack into the
|
||||
/// existing identity at <c>slot</c> (a restore). Same discriminator, one
|
||||
/// layer down (CC2 review's fidelity note).
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
|
|
|
|||
|
|
@ -123,6 +123,20 @@ namespace AcDream.Core.Net.Messages;
|
|||
/// warns about for restore. <c>WorldSession</c>'s awaiting-request latch
|
||||
/// must never assume a reply is coming.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// <b>ACE double-sends <c>NameInUse</c> (CC2 review F3).</b>
|
||||
/// <c>CharacterHandler.CharacterCreateEx</c> calls
|
||||
/// <c>IsCharacterNameAvailable</c> TWICE — once at the top and once after
|
||||
/// <c>PlayerFactory.Create</c> — and the first callback's <c>return</c>
|
||||
/// exits only the lambda, so a duplicate name yields TWO <c>0xF643</c>
|
||||
/// <c>NameInUse</c> replies. The first consumes the latch; the second hits
|
||||
/// <c>WorldSession</c>'s unrequested-response drop path (register AD-100)
|
||||
/// and logs "unexpected CharacterGenerationVerificationResponse". During a
|
||||
/// connected gate against ACE that log line is EXPECTED after a
|
||||
/// duplicate-name rejection, not an acdream defect — and CC3's verification
|
||||
/// gate must not treat the second reply as an error.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public static class CharacterCreate
|
||||
{
|
||||
|
|
|
|||
|
|
@ -723,16 +723,35 @@ public sealed class WorldSession : IDisposable
|
|||
/// create requests share that opcode on the wire (see
|
||||
/// <see cref="CharGenVerificationResponse"/>'s doc comment) with no
|
||||
/// self-describing discriminant, so this latch is the only thing that
|
||||
/// tells the dispatcher which event to fire. Set by
|
||||
/// tells the dispatcher which event to fire. Retail's own discriminator
|
||||
/// is structurally the same latch: <c>Handle_CharGenVerificationResponse
|
||||
/// @0x0055E8B0</c> case 1 branches on
|
||||
/// <c>GetVerificationState() == PENDING</c> → new CharacterIdentity +
|
||||
/// AddIdentity (create) versus not-pending → unpack into the existing
|
||||
/// identity at <c>slot</c> (restore). Set by
|
||||
/// <see cref="SendRestoreCharacter"/> / <see cref="SendCharacterCreation"/>
|
||||
/// immediately before the send; cleared the moment a matching 0xF643 is
|
||||
/// dispatched (success OR parse failure — a malformed reply must not
|
||||
/// wedge the latch open forever) and on session teardown
|
||||
/// (<see cref="Dispose"/>). Read/written only from the caller's frame
|
||||
/// thread — the same single-threaded invariant every other per-session
|
||||
/// field here (e.g. <see cref="_lastCharacterSelectionError"/>) relies
|
||||
/// on; <see cref="ProcessDatagram"/> is never invoked concurrently with
|
||||
/// a send (see the class doc comment's thread-id probe note).
|
||||
/// (<see cref="Dispose"/>).
|
||||
///
|
||||
/// <para>SCOPE, stated exactly (CC2 review F1): this latch correlates
|
||||
/// the SINGLE outstanding request. It does NOT refuse overlapping
|
||||
/// requests — a second send while one is outstanding OVERWRITES the
|
||||
/// latch and the first request's reply is then delivered to the wrong
|
||||
/// event. Refusing overlap is the CALLER's job, exactly as in retail:
|
||||
/// <c>gmCharGenMainUI::DoFinish@0x004e9170</c> only sends when the
|
||||
/// verification state is UNDEF (CC3's Runtime verification gate owns
|
||||
/// that rule here). The overwrite behavior is pinned by
|
||||
/// <c>WorldSessionCharacterCreationTests</c> so CC3 cannot silently
|
||||
/// regress against it.</para>
|
||||
///
|
||||
/// <para>Read/written only from the caller's frame thread — the same
|
||||
/// single-threaded invariant every other per-session field here (e.g.
|
||||
/// <see cref="_lastCharacterSelectionError"/>) relies on;
|
||||
/// <see cref="ProcessDatagram"/> is never invoked concurrently with a
|
||||
/// send (see <see cref="ProbeNetLogOutbound"/>'s doc comment — the
|
||||
/// #260 thread-id probe note; CC2 review F5 corrected this pointer).</para>
|
||||
/// </summary>
|
||||
private enum PendingCharGenVerificationRequest
|
||||
{
|
||||
|
|
@ -2306,9 +2325,12 @@ public sealed class WorldSession : IDisposable
|
|||
/// <summary>
|
||||
/// Send retail CharacterRestore through the control queue. This is
|
||||
/// deliberately non-blocking because ACE silently drops unknown guids.
|
||||
/// Arms the awaiting-request latch as <c>Restore</c> BEFORE the send so
|
||||
/// a reply that arrives on a later Tick is never misattributed to a
|
||||
/// different request (Campaign CC CC2).
|
||||
/// Arms the awaiting-request latch as <c>Restore</c> BEFORE the send;
|
||||
/// the latch correlates the SINGLE outstanding request — a second
|
||||
/// create/restore sent while this one is outstanding overwrites it, and
|
||||
/// refusing that overlap is the caller's job (CC3's verification gate).
|
||||
/// See <see cref="PendingCharGenVerificationRequest"/> (Campaign CC
|
||||
/// CC2).
|
||||
/// </summary>
|
||||
public void SendRestoreCharacter(uint characterId)
|
||||
{
|
||||
|
|
@ -2325,7 +2347,10 @@ public sealed class WorldSession : IDisposable
|
|||
/// non-blocking, matching <see cref="SendRestoreCharacter"/> — ACE
|
||||
/// silently drops a request whose packed account name doesn't match the
|
||||
/// session's own account. Arms the awaiting-request latch as
|
||||
/// <c>Create</c> BEFORE the send (Campaign CC CC2).
|
||||
/// <c>Create</c> BEFORE the send; the latch correlates the SINGLE
|
||||
/// outstanding request — overlap refusal is the caller's job (CC3's
|
||||
/// verification gate; see
|
||||
/// <see cref="PendingCharGenVerificationRequest"/>) (Campaign CC CC2).
|
||||
/// </summary>
|
||||
public void SendCharacterCreation(
|
||||
string accountName,
|
||||
|
|
|
|||
|
|
@ -64,13 +64,19 @@ public sealed record CharacterCreatedStatusEvent : StatusEvent
|
|||
/// <summary>
|
||||
/// Campaign CC CC2: a non-Ok reply to an outbound CharacterCreate.
|
||||
/// <see cref="Code"/> is the raw wire
|
||||
/// <c>CharGenVerificationResponse.Code</c> value; <see cref="Name"/> is that
|
||||
/// code's enum member name (e.g. <c>"NameInUse"</c>).
|
||||
/// <c>CharGenVerificationResponse.Code</c> value; <see cref="Reason"/> is
|
||||
/// that code's enum member name (e.g. <c>"NameInUse"</c>);
|
||||
/// <see cref="Name"/> is the ATTEMPTED character name. The enum member
|
||||
/// rode the <c>name</c> key until the CC2 review (F4) — same key,
|
||||
/// different meaning than <c>characterCreated.name</c> — renamed before
|
||||
/// any consumer shipped.
|
||||
/// </summary>
|
||||
public sealed record CreationFailedStatusEvent : StatusEvent
|
||||
{
|
||||
public required uint Code { get; init; }
|
||||
|
||||
public required string Reason { get; init; }
|
||||
|
||||
public required string Name { get; init; }
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -272,6 +272,7 @@ public static class StatusEventParser
|
|||
T = t,
|
||||
SessionId = sessionId,
|
||||
Code = RequireUInt32(root, "code"),
|
||||
Reason = RequireString(root, "reason"),
|
||||
Name = RequireString(root, "name"),
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -232,12 +232,17 @@ public sealed class SessionStatusWriter
|
|||
/// <summary>
|
||||
/// Campaign CC CC2: a non-Ok <c>0xF643</c> response to an outbound
|
||||
/// CharacterCreate. <paramref name="code"/> is the raw wire
|
||||
/// <c>CharGenVerificationResponse.Code</c> value; <paramref name="name"/>
|
||||
/// <c>CharGenVerificationResponse.Code</c> value; <paramref name="reason"/>
|
||||
/// is that code's enum member name (e.g. <c>"NameInUse"</c>) so a
|
||||
/// launcher can render a readable reason without hard-coding the
|
||||
/// server's numeric-to-dialog mapping itself.
|
||||
/// server's numeric-to-dialog mapping itself; <paramref name="name"/>
|
||||
/// is the ATTEMPTED character name — the thing a launcher most wants to
|
||||
/// show ("the name Bob is taken"). The key was <c>name</c> for the enum
|
||||
/// member until the CC2 review (F4): <c>characterCreated.name</c> is a
|
||||
/// character name, and one status vocabulary must not give the same key
|
||||
/// two meanings. Renamed before any consumer shipped.
|
||||
/// </summary>
|
||||
public void CreationFailed(string sessionId, uint code, string name) =>
|
||||
public void CreationFailed(string sessionId, uint code, string reason, string name) =>
|
||||
Write(new
|
||||
{
|
||||
v = VocabularyVersion,
|
||||
|
|
@ -245,6 +250,7 @@ public sealed class SessionStatusWriter
|
|||
t = Now(),
|
||||
sessionId,
|
||||
code,
|
||||
reason,
|
||||
name,
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -199,6 +199,49 @@ public sealed class WorldSessionCharacterCreationTests
|
|||
Assert.Empty(createEvents);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// CC2 review F1: pins the latch's stated scope EXACTLY. The latch
|
||||
/// correlates the single outstanding request and does NOT refuse
|
||||
/// overlap — a second send while one is outstanding OVERWRITES it, so
|
||||
/// the first request's reply is delivered to the second request's
|
||||
/// event. Refusing overlap is the caller's job (CC3's Runtime
|
||||
/// verification gate, mirroring retail's DoFinish UNDEF-state gate).
|
||||
/// If CC3 (or anyone) changes this transport-level behavior, this test
|
||||
/// must change WITH it, deliberately.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void OverlappingSend_OverwritesTheLatch_ReplyRoutesToNewestRequest()
|
||||
{
|
||||
using WorldSession session = CreateSession();
|
||||
session.GameMessageCapture = (_, _) => { };
|
||||
|
||||
session.SendRestoreCharacter(0x50000001u);
|
||||
session.SendCharacterCreation(
|
||||
"testaccount",
|
||||
MakeCreateRequest(),
|
||||
new uint[CharacterCreate.SkillAdvancementClassCount]);
|
||||
Assert.Equal(PendingLatch.Create, ReadPendingLatch(session));
|
||||
|
||||
var restoreEvents = new List<CharacterRestore.Parsed>();
|
||||
var createEvents = new List<CharGenVerificationResponse.Parsed>();
|
||||
session.CharacterRestoreReceived += restoreEvents.Add;
|
||||
session.CharacterCreateResponseReceived += createEvents.Add;
|
||||
|
||||
// This reply is semantically the RESTORE's — but the overwritten
|
||||
// latch routes it to the create event. That is the documented
|
||||
// overwrite behavior, pinned here.
|
||||
byte[] packet = BuildPacket(
|
||||
BuildVerificationResponseBody(
|
||||
(uint)CharGenVerificationResponse.Code.Ok,
|
||||
0x50000001u,
|
||||
"Restored"));
|
||||
InvokeProcessDatagram(session, packet);
|
||||
|
||||
Assert.Empty(restoreEvents);
|
||||
Assert.Single(createEvents);
|
||||
Assert.Equal(PendingLatch.None, ReadPendingLatch(session));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ResponseWithNoOutstandingRequest_IsDroppedAndNeverMisattributed()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -88,16 +88,18 @@ public sealed class StatusEventParserTests
|
|||
|
||||
var failed = Assert.IsType<CreationFailedStatusEvent>(
|
||||
StatusEventParser.Parse(
|
||||
"""{"v":1,"e":"creationFailed","t":"2026-08-15T12:00:01Z","sessionId":"s1","code":3,"name":"NameInUse"}"""));
|
||||
"""{"v":1,"e":"creationFailed","t":"2026-08-15T12:00:01Z","sessionId":"s1","code":3,"reason":"NameInUse","name":"Bob"}"""));
|
||||
Assert.Equal(3u, failed.Code);
|
||||
Assert.Equal("NameInUse", failed.Name);
|
||||
Assert.Equal("NameInUse", failed.Reason);
|
||||
Assert.Equal("Bob", failed.Name);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData("{\"v\":1,\"e\":\"characterCreated\",\"t\":\"2026-08-15T12:00:00Z\",\"sessionId\":\"s1\",\"name\":\"NewChar\"}")]
|
||||
[InlineData("{\"v\":1,\"e\":\"characterCreated\",\"t\":\"2026-08-15T12:00:00Z\",\"sessionId\":\"s1\",\"guid\":1342177296}")]
|
||||
[InlineData("{\"v\":1,\"e\":\"creationFailed\",\"t\":\"2026-08-15T12:00:00Z\",\"sessionId\":\"s1\",\"name\":\"NameInUse\"}")]
|
||||
[InlineData("{\"v\":1,\"e\":\"creationFailed\",\"t\":\"2026-08-15T12:00:00Z\",\"sessionId\":\"s1\",\"code\":3}")]
|
||||
[InlineData("{\"v\":1,\"e\":\"creationFailed\",\"t\":\"2026-08-15T12:00:00Z\",\"sessionId\":\"s1\",\"reason\":\"NameInUse\",\"name\":\"Bob\"}")]
|
||||
[InlineData("{\"v\":1,\"e\":\"creationFailed\",\"t\":\"2026-08-15T12:00:00Z\",\"sessionId\":\"s1\",\"code\":3,\"name\":\"Bob\"}")]
|
||||
[InlineData("{\"v\":1,\"e\":\"creationFailed\",\"t\":\"2026-08-15T12:00:00Z\",\"sessionId\":\"s1\",\"code\":3,\"reason\":\"NameInUse\"}")]
|
||||
public void MalformedCharacterCreationEventsUseTheKnownEventFailurePath(string line)
|
||||
{
|
||||
var malformed = Assert.IsType<MalformedStatusEvent>(StatusEventParser.Parse(line));
|
||||
|
|
|
|||
|
|
@ -195,7 +195,7 @@ public sealed class StatusFileTailerTests : IDisposable
|
|||
AppendShared(
|
||||
"""{"v":1,"e":"characterCreated","t":"2026-08-15T12:00:00Z","sessionId":"s1","guid":1342177296,"name":"NewChar"}"""
|
||||
+ "\n"
|
||||
+ """{"v":1,"e":"creationFailed","t":"2026-08-15T12:00:01Z","sessionId":"s1","code":3,"name":"NameInUse"}"""
|
||||
+ """{"v":1,"e":"creationFailed","t":"2026-08-15T12:00:01Z","sessionId":"s1","code":3,"reason":"NameInUse","name":"Bob"}"""
|
||||
+ "\n");
|
||||
var tailer = new StatusFileTailer(_path);
|
||||
|
||||
|
|
@ -207,7 +207,8 @@ public sealed class StatusFileTailerTests : IDisposable
|
|||
Assert.Equal("NewChar", created.Name);
|
||||
var failed = Assert.IsType<CreationFailedStatusEvent>(events[1]);
|
||||
Assert.Equal(3u, failed.Code);
|
||||
Assert.Equal("NameInUse", failed.Name);
|
||||
Assert.Equal("NameInUse", failed.Reason);
|
||||
Assert.Equal("Bob", failed.Name);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
|
|
|
|||
|
|
@ -110,7 +110,7 @@ public sealed class SessionStatusWriterTests
|
|||
writer.PluginFailed("s1", "acdream.bad", "failed");
|
||||
writer.LoginCommandFailed("s1", 0, "", "unknown command");
|
||||
writer.CharacterCreated("s1", 0x50000001u, "NewChar");
|
||||
writer.CreationFailed("s1", 3u, "NameInUse");
|
||||
writer.CreationFailed("s1", 3u, "NameInUse", "Bob");
|
||||
writer.Disconnected("s1", "stopped");
|
||||
writer.Exited("s1", 0, "disposed");
|
||||
|
||||
|
|
@ -132,7 +132,7 @@ public sealed class SessionStatusWriterTests
|
|||
var writer = new SessionStatusWriter(file.Path);
|
||||
|
||||
writer.CharacterCreated("s1", 0x50000010u, "NewChar");
|
||||
writer.CreationFailed("s1", 3u, "NameInUse");
|
||||
writer.CreationFailed("s1", 3u, "NameInUse", "Bob");
|
||||
|
||||
string[] lines = File.ReadAllLines(file.Path);
|
||||
Assert.Equal(2, lines.Length);
|
||||
|
|
@ -149,8 +149,10 @@ public sealed class SessionStatusWriterTests
|
|||
Assert.Equal("creationFailed", failed.GetProperty("e").GetString());
|
||||
Assert.Equal("s1", failed.GetProperty("sessionId").GetString());
|
||||
Assert.Equal(3u, failed.GetProperty("code").GetUInt32());
|
||||
Assert.Equal("NameInUse", failed.GetProperty("name").GetString());
|
||||
AssertExactProperties(lines[1], "v", "e", "t", "sessionId", "code", "name");
|
||||
Assert.Equal("NameInUse", failed.GetProperty("reason").GetString());
|
||||
Assert.Equal("Bob", failed.GetProperty("name").GetString());
|
||||
AssertExactProperties(
|
||||
lines[1], "v", "e", "t", "sessionId", "code", "reason", "name");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue