Repository navigation
Conversation
…gistry TC9_PREFERRED_HOSTNAME was parsed by Config but RegisterGameServer always sent an empty string, so the setting had no effect. The Go sidecar sends cfg.PreferredHostname in the registration request.
The TC9_{CHARACTER,ITEM,INSTANCE}_GUIDS_BUFFER_SIZE settings were parsed
by Config but never used: every pool request asked for a fixed 10000
GUIDs and refilled below a fixed 1000. The Go sidecar requests the
configured sizes (50/200/10 by default) and refills once 65% of the pool
is used, so restarts burned GUID space ~200x faster than the Go path.
GuidManager now reads the per-type pool sizes from Config at Initialize
and each iterator derives its refill threshold (35% of the pool size
remaining) from them.
…event The group.member.left payload carries NewLeaderID (set by the group server when the removed member was the leader), and the Go sidecar forwards it as the third argument of OnGroupMemberRemovedHook. The C++ consumer never parsed it, so registered hooks always received 0. Parse the field and carry it through TC9EventGroupMemberRemoved. The field is appended to the struct, which affects the TC9-style hook signature only; the classic OnGroupMemberRemovedHook ABI is unchanged (it always had the newLeaderGuid parameter). I left the project VERSION untouched to avoid colliding with the 1.1.0 already used on the guild-improvements branch - happy to bump it however you prefer.
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a likely ABI break by changing an installed by-value event struct without an accompanying versioning/ABI strategy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR closes three behavior gaps between libsidecar-cpp and the Go sidecar: preferred hostname propagation during server registration, configured GUID pool sizing/refill behavior, and forwarding NewLeaderID for group.member.left events.
Changes:
- Pass
Config::preferred_hostname()throughTC9InitLib→RegisterGameServerto honorTC9_PREFERRED_HOSTNAME. - Apply configured GUID pool sizes and a derived refill threshold in
GuidManager::Initialize, and use the configured pool size inRequestGUIDPool. - Parse
NewLeaderIDfrom the NATS payload and forward it through the group-member-removed hook path.
File summaries
| File | Description |
|---|---|
| game-server/libsidecar-cpp/src/nats/handlers.cpp | Parses NewLeaderID and populates it into the group-member-removed event struct before dispatch. |
| game-server/libsidecar-cpp/src/guids/guid_manager.h | Adds configurable refill policy support and replaces the fixed pool-size constant with per-type pool sizes. |
| game-server/libsidecar-cpp/src/guids/guid_manager.cpp | Applies config-driven pool sizing/refill thresholds at initialization and uses per-type pool sizes when refilling. |
| game-server/libsidecar-cpp/src/api.cpp | Uses configured preferred hostname during server registration and forwards newLeaderGuid to the classic hook. |
| game-server/libsidecar-cpp/include/libsidecar/tc9_events.h | Extends TC9EventGroupMemberRemoved with newLeaderGuid for internal event-hook dispatch. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+182
to
+186
| pool_sizes_[0] = pool_size(config.character_guids_buffer_size(), pool_sizes_[0]); | ||
| pool_sizes_[1] = pool_size(config.item_guids_buffer_size(), pool_sizes_[1]); | ||
| pool_sizes_[2] = pool_size(config.instance_guids_buffer_size(), pool_sizes_[2]); | ||
| character_guids_->SetRefillPolicy(pool_sizes_[0]); | ||
| item_guids_->SetRefillPolicy(pool_sizes_[1]); |
Comment on lines
36
to
40
| uint32_t groupGuid; | ||
| uint64_t memberGuid; | ||
| /* Set when the removed member was the leader; 0 otherwise */ | ||
| uint64_t newLeaderGuid; | ||
| } TC9EventGroupMemberRemoved; |
This was referenced Aug 30, 2026
Review follow-up: unit tests for GuidIterator::SetRefillPolicy - the threshold is 35% of the configured pool size remaining (mirroring the Go sidecar's 65%-used trigger), crossed exactly when consumption dips below it, and floored at 1 for tiny pools.
Owner
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
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.
While auditing libsidecar-cpp against the Go sidecar function by function (34/34 exports have a C++ homonym, headers are ABI-identical), I found three behavioral gaps worth fixing right away. One commit per fix:
TC9_PREFERRED_HOSTNAMEis parsed byConfigbutTC9InitLibalways sent""toRegisterGameServer. The Go sidecar sends the configured value.TC9_*_GUIDS_BUFFER_SIZEsettings were parsed but never used: every request asked for a fixed 10000 GUIDs (refill below 1000), while the Go sidecar requests the configured 50/200/10 and refills at 65% used. Restarting a worldserver burned character-GUID space ~200x faster than the Go path. The settings are now applied atGuidManager::Initialize, with the refill threshold derived from the pool size (35% remaining), and invalid (<=0) values fall back to the defaults.group.member.leftpayload carriesNewLeaderID, and the Go sidecar forwards it as the third hook argument; the C++ consumer never parsed it, soOnGroupMemberRemovedHookalways received 0. The field is now parsed and carried throughTC9EventGroupMemberRemoved(appended to the struct — the classic hook ABI is unchanged since it always had the parameter). I left the project VERSION untouched to avoid colliding with the 1.1.0 already used on guild-improvements; happy to bump it however you prefer.Verified: standalone CMake build (system gRPC), all 38 ctest tests pass including
CApiCompatibility.I have follow-ups queued for the remaining gaps I found (test coverage of the whole C ABI surface, and a few design-level questions about cross-realm and threading semantics that I'll open as an issue rather than deciding unilaterally).