Repository navigation
fix(castle-siege): clean up a cycle end missed while the server was down - #1351
icheat2win wants to merge 3 commits into
Conversation
When the server starts before the guild selection of a cycle and still finds the selected guilds of the previous battle, the EndCycle clean-up never ran. Clear the old registrations and selected guilds then. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xNc764bgrPZRQYCRzPj1x
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 12 |
| Duplication | 31 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Changes requested: tests/MUnique.OpenMU.Tests/CastleSiegeStateMachineTests.cs:644-700 uses Assert.Multiple(async () => { await ... }) in three new tests. The synchronous Assert.Multiple delegate can become async void, so persistence assertions after an incomplete await may execute outside the assertion scope. Please use await Assert.MultipleAsync(...) or await persistence reads before a synchronous assertion block. The recovery heuristic in src/GameLogic/CastleSiege/CastleSiegePlugIn.cs:262-284 also cannot identify missed cycle ends when no guild was selected; this limitation is documented, but deserves an explicit test.
…missed-cycle-end # Conflicts: # docs-website/docs/server-features/castle-siege.md
…ver the detection limitation Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xNc764bgrPZRQYCRzPj1x
Part of the Castle Siege epic (#735).
Problem
The EndCycle clean-up (registrations, marks, selected guilds) only runs on the transition into
EndCycle. If the server is down at that moment, it starts directly in the next cycle's earlyRegisterGuild, andOnEnterStateAsyncskips the reload on startup. The old registrations and the old selected guild list survive into the next cycle.Change
On initialization,
CastleSiegePlugIn.CleanUpMissedCycleEndAsyncchecks the persisted selected guilds (CastleSiegeData.Guilds). They only exist fromNotifytoEndCycle, so finding them while the schedule is beforeNotifymeans the clean-up never ran. It then clears the registrations and the selected guilds and logs a warning.This needs no schema change. Limitation (documented): a cycle which ended without any guild selection leaves no trace, so its registrations can't be told apart from new ones and stay. A complete fix would persist the time of the last cycle end, which needs a DataModel property and a migration; I left that out to keep this small.
Documented on the Castle Siege page.
Tests
Three new tests in
CastleSiegeStateMachineTests: restart inRegisterGuildwith stale selected guilds clears both; restart inRegisterGuildwithout them keeps the current registration; restart inReadykeeps the registration.Ran the full
MUnique.OpenMU.Testssuite with--no-build: 2445 passed, 2 skipped.Generated by Claude Code