fix(Scripts/ICC): Fix Risen Archmage respawn after wipe

[mpfans]

Changes Proposed

• Core
• Scripts
• Database

Script Changes (boss_valithria_dreamwalker.cpp)

Problem: Risen Archmages (NPC 37868) were not respawning after Valithria encounter wipe. Instead of the intended 11-second respawn, they took 7 days (604800 seconds from database).

Root Cause: The old code used RemoveCorpse() followed by SetRespawnTime(11). However, RemoveCorpse() already saved the original spawntime from the database (7 days) before SetRespawnTime(11) could set the new value.

Solution: Changed from RemoveCorpse() + SetRespawnTime(11) to DespawnOrUnsummon(0ms, 11s). This function properly despawns the creature and sets the 11-second respawn timer without database conflicts.

Technical Details:

• Modified ValithriaDespawner AI script
• Changed from void SetRespawnTime with RemoveCorpse to DespawnOrUnsummon with relog time parameter
• Affects spawned Risen Archmages (not summoned)

AI-assisted Pull Requests

• AI tools (e.g., Claude, ChatGPT, or similar) were used entirely or partially in preparing this pull request. Please specify which tools were used, if any.

Issues Addressed

• Closes [ICC/Valithria Dreamwalker]: Respawn after the first Try wipe doesn't work correctly. #25768

Source

• Live research (testing in progress)

Tests Performed

• Compiles successfully
• Tested in-game (10-man and 25-man Normal/Heroic)

How to Test

1. Enter Icecrown Citadel on any difficulty (10/25 Normal/Heroic)
2. Engage Valithria Dreamwalker encounter
3. Wipe or reset the encounter (let HP drop below 25% to trigger failure)
4. Wait 15-20 seconds after wipe
5. Verify Risen Archmages (NPC 37868) spawn correctly near Valithria platform
6. Expected: 2 archmages on 10-man, 4 archmages on 25-man
7. Respawn time should be ~11 seconds, not 7 days

Known Issues

None

[blinkysc]

Do believe this is right, but the comments could probably just be trimmed quite a bit

[mpfans]

Do believe this is right, but the comments could probably just be trimmed quite a bit

Last night, after testing, if all players die immediately after opening BOSS, Archmage will refresh normally. However, if Archmage's body disappears and then the team is destroyed, it will not refresh. Therefore, I have submitted the repair again today. Please review again

[Nyeriah]

This doesn't seem right

[coderabbitai]

Caution

Review failed

The head commit changed during the review from 7cab302 to 3fa2a5c.

📝 Walkthrough

Walkthrough

Valithria Dreamwalker reset handling now updates saved respawn times for the boss and risen archmages, and forces encounter creatures to despawn with explicit 11-second timing after restoring their stored position.

Changes

Valithria reset timing

Layer / File(s)	Summary
Respawn and cleanup timing src/server/scripts/Northrend/IcecrownCitadel/boss_valithria_dreamwalker.cpp	GameTime is added, encounter respawn timestamps are rewritten for NPC_RISEN_ARCHMAGE and NPC_VALITHRIA_DREAMWALKER, and creature cleanup now restores position before DespawnOrUnsummon(0ms, 11s).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

Ready to be Reviewed, Tested

Suggested reviewers

• sudlud

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main fix: Risen Archmage respawn handling after a Valithria wipe.
Description check	✅ Passed	The description covers the change, issue, source, testing, how to test, and known issues, matching the template well.
Linked Issues check	✅ Passed	The change addresses #25768 by fixing Valithria wipe reset and ensuring archmages respawn correctly for a new attempt.
Out of Scope Changes check	✅ Passed	The changes stay within the ICC Valithria script fix and do not introduce unrelated scope.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/server/scripts/Northrend/IcecrownCitadel/boss_valithria_dreamwalker.cpp (1)

238-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the 11-second reset delay into one constant.

The reset timing now has to stay synchronized between persisted respawn timestamps and DespawnOrUnsummon; using one named duration avoids future drift.

♻️ Proposed refactor

+static constexpr auto ValithriaResetRespawnDelay = 11s;
+
-            time_t newRespawnTime = GameTime::GetGameTime().count() + 11;
+            time_t newRespawnTime = GameTime::GetGameTime().count() + ValithriaResetRespawnDelay.count();
             for (ObjectGuid::LowType spawnId : toRespawn)
                 map->SaveCreatureRespawnTime(spawnId, newRespawnTime);
@@
         // Force the encounter's 11s reset; the DB respawn delay can be up to 7 days.
-        creature->DespawnOrUnsummon(0ms, 11s);
+        creature->DespawnOrUnsummon(0ms, ValithriaResetRespawnDelay);

Also applies to: 283-284

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/scripts/Northrend/IcecrownCitadel/boss_valithria_dreamwalker.cpp`
around lines 238 - 240, The reset delay is duplicated as a raw 11-second value
and should be centralized to keep respawn persistence and despawn timing in
sync. Introduce a single named constant for the reset duration in the Valithria
Dreamwalker logic, then use it both where `newRespawnTime` is computed and where
`DespawnOrUnsummon` is scheduled, so the timing stays consistent across the
affected code paths.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/server/scripts/Northrend/IcecrownCitadel/boss_valithria_dreamwalker.cpp`:
- Around line 238-240: The reset delay is duplicated as a raw 11-second value
and should be centralized to keep respawn persistence and despawn timing in
sync. Introduce a single named constant for the reset duration in the Valithria
Dreamwalker logic, then use it both where `newRespawnTime` is computed and where
`DespawnOrUnsummon` is scheduled, so the timing stays consistent across the
affected code paths.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: d860ed7f-9bec-4ca2-88eb-9304d7023c0f

📥 Commits

Reviewing files that changed from the base of the PR and between f5b21bb and 0e0d8b7.

📒 Files selected for processing (1)

• src/server/scripts/Northrend/IcecrownCitadel/boss_valithria_dreamwalker.cpp

[sudlud]

Heads up: while working on this branch, Claude (an AI assistant) force-pushed without permission and messed up the PR history. It's been restored, the original commits are intact, and the branch is now updated with a clean merge of master plus one new commit on top, no rewritten history. Apologies for the noise (and the confused CodeRabbit run).

What's on the branch now:

• Dead-creature path uses DespawnOrUnsummon(0ms, 11s) instead of RemoveCorpse(false) + SetRespawnTime(11). The old code never persisted the 11s: RemoveCorpse saves the DB respawn (7 days) and removes the object, so the following SetRespawnTime(11) writes to an object already being destroyed.
• Archmage spawns (37868) have spawntimesecs = 604800 like Valithria, so one killed mid-attempt gets a 7-day respawn once its corpse decays and the Cell::VisitObjects sweep can no longer reach it. The added loop re-times those saved respawns to 11s.
• Fixes vs the earlier push: builds on current master (id1 was renamed to id in refactor(Core/DB): normalize creature table by extracting multi-ID spawns #25197), the loop snapshots spawn ids before writing instead of mutating the container mid-iteration, comments trimmed.

CodeRabbit's only note is a nitpick: factor the 11/11s into one named constant. Can add.

@Nyeriah you said "this doesn't seem right", what specifically looks wrong to you? If it's the boss script touching the map respawn table in Execute(), I can scope it to the known archmage/Valithria spawn ids instead.

[mpfans]

I have already applied this PR to my product server and tested it for several weeks with everything going well

[Nyeriah]

Thanks for the PR, but the issue here was that we shouldn’t ever need to manipulate respawns in boss scripts like this. TrinityCore’s approach offers a cleaner and therefore better solution to this problem