fix(gameserver): hand shutdown unregister to the game thread (#44) - #118
Merged
Merged
Conversation
BeginGracefulShutdown's thread called self->EndSession() and
self->Unregister() itself. Unregister reaches UnregisterAllCallbacks, which
reads and clears ServerContext's callback registry ("Not synchronized — only
safe from the game's main thread", server_context.cpp) and calls
EchoVR::BroadcasterUnlisten. ReVault BroadcasterUnlisten @ 0x140f8df20 takes
no lock: it unlinks the handle from the broadcaster's listener hash chain
(+0x5e0/+0x5f8/+0x648) and only defers the delete when a plain
dispatch-in-progress bit (+0x448 & 0x20000) is set, a same-thread
reentrancy guard for SBroadcasterData's dispatch loop. From another thread
that check races the main thread walking the chain.
The shutdown thread now hands EndSession + Unregister to
GameServerLib::Update() through GameServer::MainThreadHandoff and blocks
until the game thread has run it. The game can stop calling Update(), so
the wait is bounded (30 s, a chosen bound, not a measured one). On timeout
or cancel the shutdown thread ends the session and unregisters from
ServerDB itself (UnregisterFromServerDb(false)) and leaves the registry
alone; ForceFatalExit follows and the process takes the listeners with it.
A request the game thread has already started is never abandoned, so the
fallback never runs at the same time as the task. The destructor cancels a
pending request so its join does not wait out the timeout.
Logging: the shutdown thread records handoff=<ran|task_threw|timed_out|
cancelled|busy> with the game and shutdown thread ids; Initialize logs the
game thread id; UnregisterAllCallbacks warns if it is ever reached off the
thread that registered the callbacks.
Tests: test_main_thread_handoff.cpp (in test_callback_unregistration, run
by test-auth-unit), 9 tests, 200/200 repeats under Wine. With
RunOnServicingThread forced to run the task inline (the pre-fix shape),
7 of 9 fail, including "the request never reached the game thread". A
lifecycle invariant in tools/tests/test_runtime_lifecycle_invariants.py
fails on the pre-fix gameserver.cpp (matches self->Unregister() in
BeginGracefulShutdown) and passes now.
Not covered: no live server run. Today production never reaches
BroadcasterUnlisten: merge 033b303 dropped RegisterBroadcasterCallbacks'
cb.broadcasterOwner assignment (still present in parent d339898), so
UnregisterBroadcasterCallbacks only Clear()s the registry. The race fixed
here is on the registry fields now. It would also cover the lockless
unlisten once that wiring comes back.
Co-Authored-By: nevr-runtime <agents@sprock.io>
…d-registry-violation Resolve src/runtime/CMakeLists.txt list conflicts from #109 landing on main: keep both main_thread_handoff and serverdb_uri in the source/header lists, SKIP_PRECOMPILE_HEADERS list, and the test_serverdb_uri target. Co-Authored-By: nevr-runtime <agents@sprock.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found, not fixed here
cb.broadcasterOwner = lobby->broadcasterfrom RegisterBroadcasterCallbacks, so UnregisterBroadcasterCallbacks never actually unregisters anything today — the off-thread race is currently latent on the registry's own fields, not inside the game's broadcaster state. Restoring that line changes main-thread behavior during the game's own Unregister(), so it's scoped as a separate PR.Test plan
Not done
handoff=<outcome>log line will show this on the first real shutdown.🤖 Generated with Claude Code