Repository navigation
Conversation
Pet numbers (character_pet.id) are shared by every worldserver of a realm exactly like character and item guids, but each worldserver seeds its own counter from SELECT MAX(id) at startup, so two of them hand out the same numbers and the second save's REPLACE INTO character_pet destroys the first player's pet. - proto: GuidType.PetNumber = 3 (guid.pb.go regenerated) - guidserver: PetNumber type seeded from MAX(id) of character_pet and kept in redis (realm:<id>:maxPetNumber) like the other three; GetGuids now rejects an unknown type instead of indexing out of range - libsidecar-cpp: fourth pool (thread safe, pets are created from map threads) and export TC9GetNextAvailablePetNumber, covered by the C ABI test
There was a problem hiding this comment.
🟡 Changes recommended
The new PetNumber path inherits a confirmed GUID range end-semantics mismatch in libsidecar-cpp (skips last ID of each range) and lacks unit test coverage for the new guid type.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new GUID pool type for pet numbers (character_pet.id) so clustered worldservers can allocate pet IDs safely without PK collisions across servers, extending the existing guidserver + libsidecar allocation model.
Changes:
- Extend GUID type enum with
PetNumberin proto and generated Go bindings. - Seed/store/increment pet-number max in MySQL/Redis and serve PetNumber pools from guidserver (with unknown-type validation).
- Add PetNumber pool management and a new C ABI entrypoint
TC9GetNextAvailablePetNumber()in libsidecar-cpp (plus a C API compile/ABI test call).
File summaries
| File | Description |
|---|---|
| gen/guid/pb/guid.pb.go | Regenerated Go protobuf bindings to include GuidType_PetNumber. |
| api/proto/v1/guid/guid.proto | Adds PetNumber = 3 to the GuidType enum. |
| apps/guidserver/service/guid.go | Initializes/allocates PetNumber pools; rejects unknown guid types up-front. |
| apps/guidserver/service/guid_test.go | Extends mocks/counters for PetNumber (but needs a dedicated PetNumber test case). |
| apps/guidserver/repo/redis-max-guid-storage.go | Adds Redis key plumbing for PetNumber max tracking/increment. |
| apps/guidserver/repo/mysql-max-guid-provider.go | Adds MySQL prepared statement for MAX(id) from character_pet. |
| game-server/libsidecar-cpp/src/guids/guid_manager.h | Adds a PetNumber iterator/member and API surface in the manager. |
| game-server/libsidecar-cpp/src/guids/guid_manager.cpp | Adds PetNumber pool initialization/refill and a GetNextPetNumber() accessor. |
| game-server/libsidecar-cpp/src/api.cpp | Exposes TC9GetNextAvailablePetNumber() via the C ABI. |
| game-server/libsidecar-cpp/include/libsidecar.h | Declares the new PetNumber C ABI function with brief contract notes. |
| game-server/libsidecar-cpp/tests/c_api_compatibility_test.c | Calls the new C ABI symbol to ensure it’s linkable from C. |
Review details
Files not reviewed (1)
- gen/guid/pb/guid.pb.go: Generated file
- Files reviewed: 10/11 changed files
- Comments generated: 3
- 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
+284
to
+286
| case 3: // PetNumber | ||
| pet_number_guids_->AddRanges(ranges); | ||
| break; |
Comment on lines
+136
to
+140
| func (m *MaxGuidStorageMock) IncreaseMaxGuidForPetNumbers(ctx context.Context, realmID uint32, increaseAmount uint64) (uint64, error) { | ||
| m.petNumberLock.Lock() | ||
| defer m.petNumberLock.Unlock() | ||
|
|
||
| if m.increaseDelay > 0 { |
Comment on lines
102
to
+106
| // Three separate iterators for each GUID type | ||
| std::unique_ptr<GuidIterator> character_guids_; // Thread-unsafe (fast) | ||
| std::unique_ptr<GuidIterator> item_guids_; // Thread-safe (atomics) | ||
| std::unique_ptr<GuidIterator> instance_guids_; // Thread-unsafe (fast) | ||
| std::unique_ptr<GuidIterator> pet_number_guids_; // Thread-safe (atomics) |
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.
Pet numbers (
character_pet.id) are shared by every worldserver of a realm exactly likecharacter and item guids, but AzerothCore seeds
ObjectMgr::GeneratePetNumberfromSELECT MAX(id) FROM character_petat startup on each worldserver, so two of them hand outthe same numbers and the second save's
REPLACE INTO character_pet(primary keyid)destroys the first player's pet;
pet_spellandpet_aura, keyed by pet number, get mixedthe same way. I hit it on a four-worldserver realm and traced it in the binlog.
This adds a
PetNumberguid type next to Character, Item and Instance:GuidType.PetNumber = 3(guid.pb.goregenerated)MAX(id)ofcharacter_petand kept in redis(
realm:<id>:maxPetNumber) like the other three;GetGuidsnow rejects an unknown typeinstead of indexing out of range
export
TC9GetNextAvailablePetNumber(realmID), covered by the C ABI testThe core half (call it from
GeneratePetNumberunderClusterModeEnabled(), modeled onGetClusterGuid) follows on the AzerothCore side once this interface is in.Tests:
go test -race ./apps/guidserver/..., libsidecar-cpp cmake build + ctest, and live onfour worldservers: each one now draws from its own range (pets created after the restart sit
in disjoint id blocks per worldserver) instead of all four restarting from the same
MAX(id)+1.