Skip to content

Implement Castle Siege guild selection and participant tracking - #868

Open
Zylkien wants to merge 5 commits into
MUnique:masterfrom
Zylkien:castle-siege/724-guild-selection
Open

Implement Castle Siege guild selection and participant tracking#868
Zylkien wants to merge 5 commits into
MUnique:masterfrom
Zylkien:castle-siege/724-guild-selection

Conversation

@Zylkien

@Zylkien Zylkien commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements Castle Siege guild selection, join-side synchronization, participant tracking, and post-battle rewards.

  • Selects attacking guilds during the Notify state using (marks * 5) + members + (combined level / 4), with registration order as the tie-breaker.
  • Assigns the current castle owner to Defense.
  • Expands selected guilds to their alliance members and persists the final guild list.
  • Restores the persisted guild list after a server restart.
  • Synchronizes player join sides and corresponding Castle Siege effects during Ready and Start, including players who enter or reconnect after the battle begins.
  • Tracks participating characters every five seconds during the battle.
  • Delivers configured participant rewards directly or stores them as persistent pending rewards for later delivery.
  • Awards configured guild scores to the winning guild and its alliance members.
  • Implements the registered-guild and final-guild-list packet handlers and remote views.
  • Adds the required persistence entities, EF mappings, migration, initialization update, and guild-server lookup support.

Implementation notes

  • Participant item rewards use CastleSiegeConfiguration.RewardItemDefinition. No arbitrary default reward item is introduced.
  • If the reward cannot be delivered because the character is offline or the inventory is full, it is persisted and delivered when the character next enters the game.
  • Packet definitions are not duplicated in this PR; it uses the Castle Siege packet definitions already merged through Define and validate Castle Siege network packets #863.

Testing

  • Castle Siege and related guild-server tests: 50 passed
  • Season 6 EF initialization test: 1 passed
  • Relevant GameServer, persistence, and initialization projects build successfully.
  • Warning-level analyzer checks for all changed hand-written files pass.
  • git diff --check passes.

Closes #724

@sven-n sven-n left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low-effort diff review: 3 findings posted inline.


Generated by Claude Code


/// <inheritdoc />
public ValueTask ShowJoinSideAsync(JoinSide side)
=> this._player.Connection.SendCastleSiegeJoinSideNotificationAsync((CastleSiegeJoinSide)side);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._player.Connection is dereferenced without a null check. The sibling view plug-ins added in this PR (CastleSiegeGuildListPlugIn, CastleSiegeRegisteredGuildListPlugIn) both guard with if (this._player.Connection is not { } connection) return;. Since SynchronizePlayerJoinSideAsync / ClearPlayerJoinSideAsync are invoked from the siege tick and from map add/remove events, a player disconnecting concurrently will have a null Connection and this throws a NullReferenceException inside the siege tick.

Suggested change
=> this._player.Connection.SendCastleSiegeJoinSideNotificationAsync((CastleSiegeJoinSide)side);
public ValueTask ShowJoinSideAsync(JoinSide side)
=> this._player.Connection?.SendCastleSiegeJoinSideNotificationAsync((CastleSiegeJoinSide)side) ?? default;

Generated by Claude Code

}

lifeStone.IsAlive = false;
lifeStone.SpawnedInstance = null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KillLifeStonesAsync is dead code as of this PR: every call site of SetPlayerJoinSideAsync (Ready, Start, EndCycle, and the participant tick in CastleSiegePlugIn) passes killLifeStones: false, so the killLifeStones branch never runs. Either wire it up to the state that is supposed to remove the Life Stones, or drop the parameter and the method.


Generated by Claude Code

Comment thread src/Dapr/ServerClients/GuildServer.cs Outdated
catch (Exception ex)
{
this._logger.LogError(ex, "Unexpected error when retrieving a runtime guild identifier.");
return 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swallowing the exception and returning 0 is indistinguishable from "guild not found" for the new callers. In CastleSiegeContext.LoadFinalGuildListAsync and CastleSiegeGuildSelector.SelectGuildsAsync a 0 result silently drops the guild from the siege, and in CastleSiegeParticipantTracker.AwardRewardsAsync it silently skips the guild-score reward. A transient Dapr error during Notify/Ready would therefore quietly disqualify a registered guild with no visible failure.


Generated by Claude Code

@sven-n sven-n left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second review pass, on the updated head (63cff82). Confirmed the Connection null-dereference from the earlier pass is fixed in CastleSiegeJoinSidePlugIn.

Three new findings: one correctness issue on participant time crediting, one on the persisted final guild list going stale, and one duplicated-ordering cleanup. Details inline.


Generated by Claude Code

context.NextParticipantUpdateUtc = GetNextInterval(
context.StateStartTimeUtc,
utcNow,
ParticipantUpdateInterval);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participation is credited a hardcoded (int)ParticipantUpdateInterval.TotalSeconds (5 s) per tick, but the tick only fires when the periodic task runs and NextParticipantUpdateUtc <= utcNow, and the next due time is snapped forward via GetNextInterval(StateStartTimeUtc, utcNow, 5s), which discards every missed interval.

So the credited amount is fixed while the real elapsed time between ticks is not. If the periodic task's period is longer than 5 s, or a tick is delayed, presence is under-credited — e.g. a player who was on the map for 60 s can be credited 5 s. ParticipantRewardMinSeconds may then never be reached and no participant is rewarded.

Suggest crediting the actual elapsed wall time since the previous update for that character instead of the constant.


Generated by Claude Code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale persisted guild list (around SelectGuildsAsync, ~line 600 — commenting at file level since the line is outside the diff hunks):

SelectGuildsAsync returns early (after FinalGuildList.Clear()) when the context is not an IGameServerContext, which skips SaveFinalGuildListAsync.

The persisted CastleSiegeData.Guilds then still holds the previous cycle's guilds while the runtime FinalGuildList is empty. On the next restart LoadFinalGuildListAsync repopulates that stale selection, so a siege can start with guilds from an earlier cycle.

Either persist the cleared list before returning, or don't clear the runtime list on this path.


Generated by Claude Code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated ordering (~line 118 — commenting at file level since the line is outside the diff hunks):

The OrderBy(Side).ThenByDescending(IsAllianceMaster).ThenBy(GuildName, OrdinalIgnoreCase) chain is duplicated verbatim from CastleSiegeContext.SaveFinalGuildListAsync in this same PR.

Two copies of the display ordering will drift — a change in one place silently gives the client a different order than what was persisted. Worth extracting a single helper and calling it from both.


Generated by Claude Code

Zylkien added 4 commits August 7, 2026 21:33
Select and persist defending and attacking alliances using registration scores and tie order.

Synchronize join sides for players in the siege map, track battle participation, deliver configured online or pending rewards, award guild scores, and add the B4/B5 guild-list handlers with persistence and regression tests.

# Conflicts:
#	src/GameLogic/CastleSiege/CastleSiegeContext.cs
#	src/GameLogic/CastleSiege/CastleSiegePlugIn.cs
#	src/GameServer/Properties/PlugInResources.Designer.cs
Remove the redundant concurrent collections import and simplify the guild-selection test array declarations. This resolves the reported Codacy findings without changing runtime behavior.

# Conflicts:
#	src/GameLogic/CastleSiege/CastleSiegeContext.cs
Handle disconnected players safely when sending Castle Siege join-side notifications. Remove the unused Life Stone cleanup placeholder from the guild-selection phase and propagate Dapr runtime guild-resolution failures instead of treating them as missing guilds. Add regression coverage for notifications after disconnect.

# Conflicts:
#	src/GameLogic/CastleSiege/CastleSiegePlugIn.cs
Track exact participant time across delayed ticks, map transitions, and battle completion.

Persist cleared final guild selections when the game-server context is unavailable and centralize guild ordering for persistence and packet responses.

Integrate NPC map tracking and lifecycle synchronization with map-scoped join-side assignment and authorization.

Add regression coverage for elapsed participation, stale persisted guild lists, deterministic ordering, and defender gate operation.
@Zylkien
Zylkien force-pushed the castle-siege/724-guild-selection branch from 819bfc5 to 978da40 Compare August 7, 2026 19:51

@sven-n sven-n left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third review pass, on head 819bfc51 — a deeper read of the hand-written sources (src/GameLogic/CastleSiege/*, views, handlers, guild server, data model, EF/initialization) with the surrounding types opened for context. 11 findings inline: six correctness, two concurrency, three efficiency.

The ones I'd consider blocking are the reward path — participation time being discarded on every respawn, the winner being read from the pre-battle OwnerGuildId, and the pending-reward row being deleted before the item is durably saved — since together they mean rewards can silently go to nobody or to the wrong guild.

Note the earlier finding about the hardcoded 5-second credit in CastleSiegePlugIn is superseded: the tracking code was reworked in this commit, and the same underlying bug now lives in CastleSiegeParticipantTracker.StartTracking.

Checked and found fine: the CastleSiegeJoinSide DataModel↔network enum values match; CastleSiegeMagicEffectNumber 14–17 don't collide with any effect used in initialization, and the update plug-in does create the definitions; the durability convention in CastleSiegeRewardDelivery matches existing code; RegisteredGuilds, FinalGuildList and ParticipantTracking are concurrent collections, so the lock-free packet handlers are safe; and GameMap raises its add/remove events outside any lock, so the lock usage above is contention rather than deadlock.

Merge conflicts against master were out of scope for this pass.


Generated by Claude Code

/// <summary>
/// Assigns the current Castle Siege side to all online players on the Castle Siege map.
/// </summary>
/// <returns>A task that represents the asynchronous synchronization operation.</returns>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gating on CurrentState != PlayerState.EnteredWorld also excludes Dead, ChangingMap, TradeOpened and NpcDialogOpened — all normal states for a player who is standing on the siege map.

Consequences: a dead player loses their join side and their participation credit for as long as they are dead (which during a siege is often), and gets a duplicate join-side packet when they respawn back into EnteredWorld.

Suggest checking that the player is on the siege map and has a selected character, rather than for one specific state.


Generated by Claude Code

/// <param name="context">The Castle Siege context.</param>
/// <param name="player">The player.</param>
/// <param name="utcNow">The current UTC time.</param>
internal static void StartTracking(CastleSiegeContext context, Player player, DateTime utcNow)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StartTracking resets LastUpdateUtc to now without first crediting the time already accrued.

PlayerStateChangedAsync reaches this on every transition back into EnteredWorld — respawn, closing an NPC dialog, ending a trade. Each of those throws away the interval since the last update, so a player who dies (or opens a dialog) more often than the update interval never accumulates ParticipationTime, never reaches ParticipantRewardMinSeconds, and never gets the reward. During a siege that describes most participants.

Credit the elapsed span before resetting the timestamp.


Generated by Claude Code

}

if (context.GameContext is not IGameServerContext gameServerContext
|| context.SiegeData.OwnerGuildId is not { } ownerGuildId

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AwardRewardsAsync derives the winning guild from SiegeData.OwnerGuildId, but nothing in this PR updates OwnerGuildId from the battle outcome — it still holds the pre-battle owner at the point the rewards run.

So the guild score always goes to the guild that owned the castle before the siege, and attackers who actually capture it get nothing. Either set the new owner before awarding, or take the winner from the battle result rather than from the stored owner.


Generated by Claude Code

int score)
{
var allianceGuilds = await gameServerContext.GuildServer
.GetAllianceGuildsAsync(masterGuildId)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetAllianceGuildsAsync only enumerates guild containers that are already loaded in memory, i.e. guilds with at least one online member.

An alliance guild whose members are all offline at selection time is therefore silently dropped from FinalGuildList. When those members log in during the siege they resolve to join side None — no side, no effect, no participation credit — even though their alliance was selected. Since selection happens in Notify, well before the battle, that is a likely rather than exotic case.

The alliance membership should be resolved from persistence, not from the loaded containers.


Generated by Claude Code

.ToList();
var onlinePlayers = (await context.GameContext.GetPlayersAsync().ConfigureAwait(false))
.Where(player => player.SelectedCharacter is not null)
.ToDictionary(player => player.SelectedCharacter!.Id);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToDictionary keyed on the character id throws ArgumentException if the same character appears twice — which happens transiently while a player reconnects and the old and new player objects overlap.

Because this runs at the end of the siege, one duplicate aborts the whole reward pass: no participant rewards and no guild scores for that siege, for everyone. Use GroupBy(...).ToDictionary(g => g.Key, g => g.First()) (or a TryAdd loop) so a duplicate can't take down the payout.


Generated by Claude Code

await context.ClearPlayerJoinSideAsync(player).ConfigureAwait(false);
}
finally
{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map add/remove and player-state hooks take the unbounded ExecutionLock before checking whether the event is even relevant to the siege, and the periodic task holds that same lock across database loads, per-guild remote guild-server calls, and view sends.

The result is server-wide contention on a hot path: every login and every map move anywhere on the server can block behind a siege tick doing I/O. GameMap raises these events outside its own lock, so this is contention rather than deadlock — but it is still a stall proportional to how slow the guild server and DB are.

Two things would help: cheap relevance checks (siege active, correct map) before acquiring the lock, and moving the I/O out of the locked region.


Generated by Claude Code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsynchronized read of ActiveEffects (in SetJoinSideMagicEffectAsync, ~line 567 — file-level comment because the line falls outside the diff hunks):

The method enumerates MagicEffectList.ActiveEffects directly. That is a SortedList guarded inside MagicEffectList by _addLock — reading it from outside without the lock races with concurrent effect add/remove and can throw or observe a torn view.

This runs every 5 seconds for every online player on the siege map, so the window is not small. Go through the MagicEffectList API that takes the lock instead of touching the collection directly.


Generated by Claude Code

typeof(Character),
false,
context.GameContext.Configuration);
var character = (await persistenceContext.GetAsync<Character>().ConfigureAwait(false))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the guild master is offline, GetCombinedLevelAsync falls back to materializing the entire Character table and filtering in memory — once per registered guild in that situation, and all of it while the execution lock is held.

On a populated server that is a full table scan per guild during selection. Query the characters by guild instead of loading them all.


Generated by Claude Code

false,
player.GameContext.Configuration);
var pendingRewards = (await persistenceContext
.GetAsync<CastleSiegePendingReward>()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loads the entire CastleSiegePendingReward table and filters it in memory, on every character login — including the overwhelming majority of logins that have no pending reward at all.

Filter by character id in the query so a login costs an indexed lookup instead of a full table load.


Generated by Claude Code


for (var point = 0; point < score; point++)
{
await gameServerContext.GuildServer.IncreaseGuildScoreAsync(runtimeGuildId).ConfigureAwait(false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IncreaseGuildScoreAsync is invoked once per score point in a loop, so awarding a configured score of a few hundred means a few hundred round trips to the guild server — and each iteration re-resolves a guild id that is already available here.

Pass the amount once (or at minimum hoist the id resolution out of the loop).


Generated by Claude Code

Preserve Castle Siege join-side and participation state across transient player states and reconnects. Resolve battle winners and offline alliances correctly, deliver pending rewards atomically, batch guild score updates, and replace broad persistence queries with targeted lookups.

Synchronize runtime NPC and magic-effect access while keeping player map events independent of the state-machine lock. Add regression coverage for participant timing, reconnect overlap, reward delivery, and winning-alliance scoring.
@Zylkien
Zylkien requested a review from sven-n August 8, 2026 07:01

@sven-n sven-n left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low-effort review pass: 2 correctness findings posted inline.


Generated by Claude Code

&& player.CurrentMap?.Definition.Number != context.Configuration.CastleSiegeMapDefinition?.Number)
|| player.SelectedCharacter is not { } character
|| player.GuildStatus is not { } guildStatus
|| context.GetPlayerJoinSide(player) == CastleSiegeJoinSide.None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StopTracking (called from ObjectRemovedFromMapAsync with allowPlayerOutsideSiegeMap: true) will always bail out here: CastleSiegeContext.GetPlayerJoinSide now returns None as soon as player.CurrentMap is no longer the siege map, which is exactly the case when the player is being removed from the map (or logs out, where CurrentMap is null). So the final tracking interval is never flushed and the participant stays with IsTracking = true and a stale LastUpdateUtc — a player who leaves right before the End state loses all participation time accumulated since the last 5s tick, and a disconnecting player keeps an interval that is never closed. The allowPlayerOutsideSiegeMap flag needs to bypass the join-side lookup too (e.g. fall back to the already-cached context.PlayerJoinSides[character.Id] / FinalGuildList).


Generated by Claude Code

await this._daprClient.InvokeMethodAsync(
this._targetAppId,
nameof(this.IncreaseGuildScoreAsync),
(GuildId: guildId, Amount: amount))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a ValueTuple as the Dapr request body silently breaks in the hosted setup: System.Text.Json only serializes public properties, and ValueTuple's Item1/Item2 are fields, so this serializes to {} and the controller's [FromBody] (uint GuildId, int Amount) deserializes to (0, 0). Guild scores would then never be increased (and IncreaseGuildScoreAsync would be called for guild id 0) in the Dapr deployment. Use a small DTO/record with properties for the body on both sides.


Generated by Claude Code

@sven-n sven-n left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 97c7c8b3 (max effort, single careful diff pass).

Status of the 11 findings from the earlier deep pass

# Finding Status at 97c7c8b3
1 PlayerState != EnteredWorld gate excluded Dead/ChangingMap/TradeOpened/NpcDialogOpened Fixed — both gates removed from SetPlayerJoinSideAsync and SynchronizePlayerJoinSideAsync; side/effect now survive transient states. Test added.
2 StartTracking reset LastUpdateUtc without crediting FixedCastleSiegeParticipant.IsTracking + functional AddOrUpdate; credit is driven by the previous interval's flag. StopTracking runs from ObjectRemovedFromMapAsync while CurrentMap is still the siege map, so the interval does close. Verified by the new 23 s / 30 s assertions.
3 Winner derived from SiegeData.OwnerGuildId, never updated from the battle RemainsGetWinningGuild adds the MiddleOwnerGuildId path, but nothing in production ever sets it (only tests, plus = null on EndCycle), and OwnerGuildId is still never written at End. See inline comment.
4 GetAllianceGuildsAsync only enumerated loaded containers Fixed — now DB-backed via GetAlliancesAsync, offline alliance guilds are materialised. But it regressed the alliance-chat hot path; see inline comment.
5 ToDictionary on character id threw on reconnect overlap FixedGroupBy(...).ToDictionary(g => g.Key, g => g.First()), with a dedicated regression test.
6 Pending-reward row deleted+committed before the item was durably saved Fixed — the row is now loaded/deleted through player.PersistenceContext and committed together with the item by SaveProgressAsync. (CastleSiegePendingReward is not a Configuration type, so AccountContext does not ignore it — the Set<> call resolves.)
7 ExecutionLock taken before relevance checks and held across I/O Partially / superseded — player map and state-change events no longer take the lock at all, which was the harmful part. The state machine still holds it across DB/remote/view I/O, but it is acquired with WaitAsync(0), so a slow tick is skipped rather than queued. Acceptable.
8 SetJoinSideMagicEffectAsync read ActiveEffects without _addLock Fixed — new MagicEffectsList.TryGetActiveEffectAsync takes _addLock, and the lock is released before DisposeAsync, so no re-entrancy against OnEffectTimeOutAsync.
9 GetCombinedLevelAsync materialised the whole Character table Fixed — replaced by GetAccountByCharacterNameAsync, which is an indexed lookup. It still pulls the full account aggregate per offline guild master, but that is bounded by the number of registered guilds.
10 CastleSiegePendingRewardPlugIn loaded the whole pending-reward table per login Fixed — new IPlayerContext.GetPendingCastleSiegeRewardsAsync filters server-side, backed by the IX_CastleSiegePendingReward_CharacterId index added in the migration.
11 IncreaseGuildScoreAsync called once per score point Fixed in shape, broken in the Dapr transport — the loop is gone and the interface takes an amount, but the new remote payload is a ValueTuple, which System.Text.Json cannot serialise. See the inline comment on src/Dapr/ServerClients/GuildServer.cs; this also regresses guild-war scoring.

New findings this pass — 9 inline comments, ordered roughly by severity: the Dapr ValueTuple payload (11), the missing IItemAppearPlugIn notification on reward delivery, the alliance-chat DB round-trip introduced by (4), the dead winner resolution (3), per-game-server score multiplication, the 5-second all-players magic-effect sweep, undeletable pending rows, per-reward persistence contexts, and the consumed-but-empty attack slot.

Everything else I checked came back clean: packet handler keys 0xB4/0xB5 do not collide with existing handlers, the CastleSiegeJoinSide data-model and network enums have identical values, magic-effect numbers 14–17 are unused elsewhere, both PlugInResources designer/resx pairs are complete, the migration matches the model builder configuration, and CastleSiegeData.Guilds is eagerly loaded by the repository (no lazy-load-on-disposed-context hazard).


Generated by Claude Code

await this._daprClient.InvokeMethodAsync(
this._targetAppId,
nameof(this.IncreaseGuildScoreAsync),
(GuildId: guildId, Amount: amount))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The score increase is silently lost in the Dapr deployment.

ValueTuple exposes Item1/Item2 as public fields, and System.Text.Json (which DaprClient uses, with no custom JsonSerializerOptions configured anywhere in src/DaprDaprService.cs just calls services.AddDaprClient()) does not serialize fields: JsonSerializerOptions.IncludeFields defaults to false. The request body therefore goes out as {}, and [FromBody] (uint GuildId, int Amount) on GuildServerController.IncreaseGuildScoreAsync deserializes it back to (0, 0). GuildServer.IncreaseGuildScoreAsync then hits amount > 0 == false and returns without doing anything — no exception, nothing logged.

Concrete failure: on a Dapr deployment, the winning alliance never receives GuildScoreCastleSiege/GuildScoreCastleSiegeMembers after a siege. It is also a regression for guild war: GuildWarAnswerAction previously sent a bare uint (which serializes fine) and now goes through this same path, so guild-war winners stop getting their point too.

Every other multi-argument method in this very file uses a dedicated record for exactly this reason (GuildCreationArguments, GuildMemberCreationArguments, GuildMemberRoleChangeArguments, PlayerOnlineStateArguments). The tuples at lines 234/290/304 are not a counter-example — those methods have no corresponding controller endpoint at all, so they were never exercised.

Suggest adding e.g. public record GuildScoreIncreaseArguments(uint GuildId, int Amount); next to the other argument records and using it on both sides.


Generated by Claude Code

var item = player.PersistenceContext.CreateNew<Item>();
item.Definition = rewardDefinition;
item.Durability = item.IsStackable() ? 1 : rewardDefinition.Durability;
if (await player.Inventory.AddItemAsync(item).ConfigureAwait(false))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delivered reward item is never sent to the client.

InventoryStorage.AddItemAsync only mutates server-side state; it does not notify the client. Every other place in the codebase that grants an item into an online player's inventory follows up with IItemAppearPlugIn (see QuestCompletionAction.cs:124, ItemStackAction.cs:64, PickupItemAction.cs:48, SymbolOfKundunStackedPlugIn.cs:41).

Both call sites are affected:

  1. CastleSiegeParticipantTracker.AwardRewardsAsync — a participant who is still online when the siege enters End gets the item added server-side, but their client keeps showing that slot as empty until they relog.
  2. CastleSiegePendingRewardPlugIn — worse, because it runs on the CharacterSelection → EnteredWorld transition, which Player.OnPlayerEnteredWorldAsync raises from ClientReadyAfterMapChangeAsync() at line 2731, i.e. after IUpdateInventoryListPlugIn.UpdateInventoryListAsync() was already sent at line 2718. So the pending reward is added, committed via SaveProgressAsync, and the client never learns about it in that session either — the "delivered when the character next enters the game" path only becomes visible on the login after the one that delivered it.

Adding await player.InvokeViewPlugInAsync<IItemAppearPlugIn>(p => p.ItemAppearAsync(item)) on the success path fixes both.


Generated by Claude Code

.Select(g => new AllianceGuildEntry(g.Id, g.Guild.Name ?? string.Empty, g.Guild.Members.Count, g.Guild.Logo))
.ToImmutableList();
using var context = this._persistenceContextProvider.CreateNewGuildContext();
var persistentGuilds = await context.GetAlliancesAsync(masterGuid).ConfigureAwait(false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turns alliance chat into a per-message database round-trip.

Replacing the in-memory _guildDictionary scan with CreateNewGuildContext() + GetAlliancesAsync() correctly fixes the "fully offline alliance guilds are dropped" problem for Castle Siege, but GetAllianceGuildsAsync is not a Castle-Siege-only API. Its hot caller is GameServerContext.ForEachAlliancePlayerAsync (src/GameServer/GameServerContext.cs:149), which GameServer.cs:223 invokes for every alliance chat message. It is also called from the client-triggered AllianceListRequestHandlerPlugIn, which a player can spam.

So each alliance chat line now allocates a DbContext, issues a SELECT ... WHERE AllianceGuildId = @p with Include(RawMembers), and (in the Dapr split-server setup) does that behind a remote call. Previously it was a dictionary scan.

Since CreateGuildContainerAsync already eagerly materialises the whole alliance whenever any of its members enters the game, and GuildMemberLeftGameAsync deliberately keeps alliance containers resident ("Keep alliances in memory for simplicity"), the DB round-trip is only needed for the case Castle Siege cares about: an alliance where no member is online. Consider keeping the in-memory path as the fast path and only falling back to the DB query when it yields nothing, or moving the DB-backed lookup behind a separate method used by the siege selector.


Generated by Claude Code


private static CastleSiegeGuildParticipant? GetWinningGuild(CastleSiegeContext context)
{
if (context.MiddleOwnerGuildId is { } runtimeGuildId)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MiddleOwnerGuildId is never assigned by production code, so the fallback always wins — and it names the previous owner.

Grepping the whole tree, the only writes to MiddleOwnerGuildId are CastleSiegePlugIn.cs:354 (= null on EndCycle) and two assignments in CastleSiegeGuildSelectionTests. Nothing in the Crown/switch flow sets it yet ("Start-state Crown, switch and mini-map ticks are implemented by their dedicated phases"). And SiegeData.OwnerGuildId is not updated from the battle outcome anywhere either — OnEnterStateAsync(End) awards rewards but never writes a new owner.

Net effect at runtime: GetWinningGuild always takes the SiegeData.OwnerGuildId branch, which is the guild that owned the castle before this siege. Concrete scenario: guild A defends and loses the castle to attacker B. On End, A is still OwnerGuildId, so A's Defense side receives GuildScoreCastleSiege / GuildScoreCastleSiegeMembers and B receives nothing — the exact inversion of the intended reward. When the castle is unowned (OwnerGuildId == null, the initialiser's default) no side is scored at all.

The plumbing added here is right; it just needs the winner to actually be recorded. Until the Crown phase lands, it would be safer to skip the guild-score award entirely when MiddleOwnerGuildId is null rather than falling back to the stale owner.


Generated by Claude Code

continue;
}

await gameServerContext.GuildServer.IncreaseGuildScoreAsync(runtimeGuildId, score).ConfigureAwait(false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guild score is multiplied by the number of game-server processes.

CastleSiegePlugIn holds one CastleSiegeContext per IGameContext (ConditionalWeakTable<IGameContext, CastleSiegeContext>), and ExecuteTaskAsync is an IPeriodicTaskPlugIn that runs independently in every game server. Each of them therefore enters the End state on its own schedule and calls AwardRewardsAsync.

Participant item rewards are naturally partitioned (each server only tracks its own players), but IncreaseGuildScoreAsync targets the shared guild server. With N game servers the winning guild gets N × GuildScoreCastleSiege and each alliance member N × GuildScoreCastleSiegeMembers. ClearRegistrationsAsync and SaveFinalGuildListAsync are likewise re-executed N times.

The class remark on CastleSiegeContext only warns about concurrent registration changes; this is a new globally-visible side effect that needs either an owning-server election or an idempotency guard (e.g. persisting a "scores awarded for cycle X" marker on CastleSiegeData and checking it before awarding).


Generated by Claude Code


var activeCharacterIds = new HashSet<Guid>();
var activePlayers = new HashSet<Player>();
foreach (var player in await this._gameContext.GetPlayersAsync().ConfigureAwait(false))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every 5 seconds during the battle this walks all online players and takes each one's magic-effect lock four times.

OnTickAsync calls SetPlayerJoinSideAsync() on every NextParticipantUpdateUtc tick (ParticipantUpdateInterval = 5 s) while the state is Start. This loop iterates GetPlayersAsync() — the whole server, not just the siege map — and for every player not on the siege map calls ClearPlayerJoinSideAsyncSetJoinSideMagicEffectAsync(player, None), which awaits TryGetActiveEffectAsync four times (once per CastleSiegeMagicEffectNumber), each acquiring that player's MagicEffectsList._addLock.

On a 1000-player server that is ~4000 sequentially awaited lock acquisitions every 5 seconds for the entire battle, all inside ExecutionLock, and contending with those players' own packet handlers. Almost all of that work is a no-op: a player who was never on the siege map can never have one of these effects.

Cheap fixes: iterate the already-maintained _siegeMapPlayers set for the assign path, and only run the clear path for character ids actually present in PlayerJoinSides / players present in _notifiedPlayerJoinSides, instead of for the entire server population.


Generated by Claude Code

{
var rewardDefinition = player.GameContext.Configuration.Items
.FirstOrDefault(item => item.GetId() == pendingReward.ItemDefinitionId);
if (rewardDefinition is null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unresolvable ItemDefinitionId leaves the row in the table forever.

The continue covers two very different cases. "Inventory full" is a legitimate retry-next-login. "rewardDefinition is null" is not: the item definition is gone from the game configuration (config re-initialised, item removed, or the row was written by a server with a different configuration), so the lookup will fail identically on every future login. The row is then re-read and re-skipped on every CharacterSelection → EnteredWorld transition for the lifetime of the character.

Worth splitting the two branches and deleting the row (with a warning log) when the definition cannot be resolved, so the queue can't accumulate permanently-undeliverable entries.


Generated by Claude Code

Guid characterId,
ItemDefinition rewardDefinition)
{
using var persistenceContext = gameContext.PersistenceContextProvider.CreateNewTypedContext(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One persistence context and one SaveChangesAsync round-trip per queued reward.

AwardRewardsAsync calls this in a loop over eligibleParticipants, so a siege where the reward could not be handed over directly (offline characters, full inventories) produces N CreateNewTypedContext + N SaveChangesAsync calls back to back, synchronously blocking the state transition into End while ExecutionLock is held. Passing the whole batch and writing it through a single context would keep this to one round-trip.


Generated by Claude Code

side,
candidate.Score)
.ConfigureAwait(false);
attackSideIndex++;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attackSideIndex is incremented unconditionally, even when AddGuildAndAllianceAsync added nothing.

If the candidate's alliance is non-empty but every GetPersistentGuildIdAsync returns null (the continue inside the alliance loop), the attack slot is consumed and Attack1 ends up with zero guilds, while the next-ranked candidate is pushed to Attack2 — or dropped entirely once maximumAttackers is reached. Having AddGuildAndAllianceAsync report whether it actually added a participant, and only advancing the index on success, would keep the configured number of attacking sides populated.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Castle Siege Guild Selection & Participant Tracking

2 participants