Skip to content

Commit babf501

Browse files
committed
[arc A] smoke findings: mis-scoped GT assert + a log line I dropped
Two things the 4-peer smoke caught that the build could not. 1. HotPathGuard ERROR: "g_rows (roster_ledger::Reset) accessed OFF the game thread". Reset is called from event_feed::OnSessionStart, which runs on the BRINGUP thread at harness/session_runtime.cpp:386 -- before g_session.Start() at :430 spawns the net thread and before the pump can tick a running session. So the ACCESS is safe by construction (no concurrent reader; Start()'s thread creation is the happens-before edge) and the ASSERT was mis-scoped. Reset is now the one accessor deliberately without it, with the contract written down. Also recorded there: Reset notifies subscribers per slot, but every outgoing row is empty at bringup and every subscriber early-returns on an unoccupied outgoing row -- so no subscriber body, none of which is bringup-thread-safe, actually runs. That invariant is load-bearing and now says so. 2. The client-side "installed cross-peer identity" log line vanished when I rewrote the handler, taking mp.py's xpeer_identity counter with it (the smoke reported []). That is lost observability, not a stale matcher, so the line is restored -- gated on the ACTUAL mirror install rather than on every pulse re-assert, or a per-second repeat would make it useless as a signal. mp.py's two matchers repointed at the renamed lines (held-WIP, not committed). RE-SMOKE ON THE FIXED BYTES -- 4 peers, PASS: host accepted [1,2,3], relayed roster 3x, epoch latched [1,2,3] CLIENT1 slot=1 sees [0,2,3], xpeer=[2,3]; CLIENT2 slot=2 sees [0,1,3], xpeer=[1,3]; CLIENT3 slot=3 sees [0,1,2], xpeer=[1,2] ledger: slot 0 -> #1 (host role constant), slots 1..3 -> #2,#3,#4 (monotonic, host never draws #1) 0 HotPathGuard violations, 0 puppet spawn failures, 0 malformed drops, 0 stale-gen drops, no roster/ledger WARN or ERROR IDEMPOTENCY PROVEN: exactly 4 ledger transitions and exactly 2 identity installs per client across the whole run, while the repair pulse re-asserted every 1-5 s throughout -- the receiver treats rows as STATE, not events. Still NOT hands-on, and the replacement/successor drills still owed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 13470f1 commit babf501

2 files changed

Lines changed: 24 additions & 1 deletion

File tree

‎src/votv-coop/src/coop/player/roster_ledger.cpp‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,12 +129,28 @@ void ClearAll() {
129129
}
130130

131131
void Reset() {
132-
UE_ASSERT_GAME_THREAD("g_rows (roster_ledger::Reset)");
132+
// DELIBERATELY NOT UE_ASSERT_GAME_THREAD, and this is the only accessor
133+
// without it. Reset is the SESSION-BRINGUP entry point: it is called from
134+
// event_feed::OnSessionStart, which runs on the bringup thread at
135+
// harness/session_runtime.cpp:386 -- BEFORE g_session.Start() at :430 spawns
136+
// the net thread and before the pump can tick a running session. There is no
137+
// concurrent reader by construction, and Start()'s thread creation is the
138+
// happens-before edge for everything after. Same discipline the neighbouring
139+
// SetLocalNickname writer already documents. (The smoke's HotPathGuard caught
140+
// the assert firing here; the ACCESS is fine, the assert was mis-scoped.)
141+
//
133142
// UNCONDITIONAL, unlike ClearAll: a subscriber's clear runs for every slot
134143
// whether or not the ledger believed it occupied. The difference matters
135144
// because some per-slot state is written from the wire BEFORE its row exists
136145
// (a peer's display prefs land with its Join), so an occupancy-gated clear
137146
// would leave exactly that state behind for the next session to inherit.
147+
//
148+
// Subscribers are still notified per slot, but every outgoing row here is
149+
// EMPTY (the previous session's ClearAll already emptied them at flee time),
150+
// and every subscriber early-returns on an unoccupied outgoing row. So no
151+
// subscriber BODY -- none of which is bringup-thread-safe -- actually runs.
152+
// That invariant is load-bearing; a subscriber that ever drops the
153+
// `!outgoing.occupied()` guard would start doing engine work off the GT.
138154
for (int slot = 0; slot < kMaxSlots; ++slot) {
139155
const Row outgoing = g_rows[slot];
140156
g_rows[slot] = Row{};

‎src/votv-coop/src/coop/session/player_handshake_roster.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,13 @@ bool ApplyRosterRow(net::Session& session, const uint8_t* payload, size_t payloa
296296
if (!coop::element::Registry::IsAllowedPeerAllocatedEid(describedEid)) {
297297
UE_LOGW("roster: row slot=%u eid=0x%08x not in peer range -- dropping "
298298
"mirror install", static_cast<unsigned>(describedSlot), describedEid);
299+
} else if (!coop::players::Registry::Get().GetPlayerElement(describedSlot)) {
300+
// Log only on the ACTUAL install, never on a pulse re-assert -- this
301+
// line is a smoke signal (mp.py's xpeer_identity counter) and a
302+
// per-second repeat would make it worthless as one.
303+
coop::players::Registry::Get().EstablishMirrorForSlot(describedSlot, describedEid);
304+
UE_LOGI("roster: client installed cross-peer identity slot=%u eid=0x%08x nick='%ls'",
305+
static_cast<unsigned>(describedSlot), describedEid, nick.c_str());
299306
} else {
300307
coop::players::Registry::Get().EstablishMirrorForSlot(describedSlot, describedEid);
301308
}

0 commit comments

Comments
 (0)