fix(triggers): registration follows every flag and list change; migrator honours Running=false; no nullable value types

CLAUDE.md

@@ -62,6 +62,11 @@ ModernSpawner-specific:
   behaviour. Never add a parallel entry list or hide base members with `new`.
 - Triggers register through `TriggerSystem`; proximity uses `Item.HandlesOnMovement`/`OnMovement`, speech
   uses `HandlesOnSpeech`. Extended (beyond 24-tile) proximity is stubbed pending a ModernUO area-movement API.
+- Trigger list changes go through the generated helpers (`AddToTriggerDefinitions`,
+  `RemoveFromTriggerDefinitionsAt`, `ClearTriggerDefinitions`), then call `EnsureTriggersActive()`; the
+  `TriggerActivated` setter does this for you. Never call `TriggerSystem.ActivateTriggers` directly — it is
+  not idempotent, and within `Projects/ModernSpawner` `EnsureTriggersActive` is its only caller (tests call
+  it deliberately, to build the stale registrations teardown has to survive).
 
 ## ModernUO changes
 

Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs

@@ -0,0 +1,185 @@
+using System;
+using System.Xml;
+using Server.Engines.ModernSpawner.Migration;
+using Server.Engines.ModernSpawner.Triggers;
+using Xunit;
+
+namespace Server.Engines.ModernSpawner.Tests;
+
+/// <summary>
+/// Trigger registration follows every flag and list change: the <see cref="ModernSpawner.TriggerActivated" />
+/// setter registers and unregisters immediately, and stopping or deleting a spawner tears the registration
+/// down regardless of what the flag says at that moment.
+/// </summary>
+[Collection("Sequential ModernSpawner Tests")]
+public class ModernSpawnerTriggerRegistrationTests
+{
+    private const string Proximity = "proximity:8:true";
+
+    private static ModernSpawner Place()
+    {
+        var spawner = new ModernSpawner(1, TimeSpan.FromMinutes(5), TimeSpan.FromMinutes(10), 0, default, "Rabbit");
+        spawner.MoveToWorld(new Point3D(1500, 1500, 0), Map.Felucca);
+        return spawner;
+    }
+
+    [Fact]
+    public void TogglingTriggerActivated_RegistersAndUnregisters()
+    {
+        var spawner = Place();
+        spawner.AddToTriggerDefinitions(Proximity);
+        Assert.False(spawner.HandlesOnMovement);
+
+        spawner.TriggerActivated = true;
+        Assert.True(spawner.HandlesOnMovement);
+
+        spawner.TriggerActivated = false;
+        Assert.False(spawner.HandlesOnMovement);
+        spawner.Delete();
+    }
+
+    [Fact]
+    public void AddingDefinitionToActivatedRunningSpawner_RegistersImmediately()
+    {
+        var spawner = Place();
+        spawner.TriggerActivated = true;
+        Assert.False(spawner.HandlesOnMovement);
+
+        spawner.AddToTriggerDefinitions(Proximity);
+        spawner.EnsureTriggersActive();
+        Assert.True(spawner.HandlesOnMovement);
+
+        spawner.RemoveFromTriggerDefinitions(Proximity);
+        spawner.EnsureTriggersActive();
+        Assert.False(spawner.HandlesOnMovement);
+        spawner.Delete();
+    }
+
+    [Fact]
+    public void DeletingAnActivatedSpawner_LeavesNothingRegistered()
+    {
+        var spawner = Place();
+        spawner.AddToTriggerDefinitions(Proximity);
+        spawner.TriggerActivated = true;
+        Assert.True(spawner.HandlesOnMovement);
+
+        spawner.TriggerActivated = false;
+        spawner.TriggerActivated = true;
+        spawner.Delete();
+
+        Assert.False(TriggerSystem.Instance.IsRegistered(spawner));
+    }
+
+    [Fact]
+    public void Stop_UnregistersEvenWhenFlagWasClearedAfterRegistration()
+    {
+        var spawner = Place();
+        spawner.AddToTriggerDefinitions(Proximity);
+        spawner.TriggerActivated = true;
+        spawner.Stop();
+        Assert.False(TriggerSystem.Instance.IsRegistered(spawner));
+
+        // A registration that outlived its flag - the state a raw field write or a pre-fix gump edit
+        // could leave behind. Teardown does not consult the flag, so it still has to be cleaned up.
+        spawner.TriggerActivated = false;
+        spawner.Start();
+        Assert.False(TriggerSystem.Instance.IsRegistered(spawner));
+
+        TriggerSystem.Instance.ActivateTriggers(spawner);
+        Assert.True(TriggerSystem.Instance.IsRegistered(spawner));
+
+        spawner.Stop();
+        Assert.False(TriggerSystem.Instance.IsRegistered(spawner));
+        spawner.Delete();
+    }
+
+    [Fact]
+    public void GumpTimeTriggerDefinitions_ParseAndRegister()
+    {
+        // Built exactly as TriggerConfigGump builds them from its hour fields: the registered type
+        // names, in the argument format each Parse accepts.
+        const int startHour = 18;
+        const int endHour = 6;
+        var wallTime = $"wall_time_window:{startHour}:0:{endHour}:0";
+        const string gameTime = "game_time_window:21:5:true";
+
+        // ParseTrigger returns null for an unrecognised type (it only logs), so this pins the branch.
+        Assert.NotNull(TriggerSystem.Instance.ParseTrigger(wallTime));
+        Assert.NotNull(TriggerSystem.Instance.ParseTrigger(gameTime));
+
+        var spawner = Place();
+        spawner.AddToTriggerDefinitions(wallTime);
+        spawner.AddToTriggerDefinitions(gameTime);
+        spawner.TriggerActivated = true;
+
+        Assert.True(TriggerSystem.Instance.IsRegistered(spawner));
+        spawner.Delete();
+    }
+
+    [Fact]
+    public void DeletingAStoppedSpawner_WithStaleRegistration_Unregisters()
+    {
+        var spawner = Place();
+        spawner.AddToTriggerDefinitions(Proximity);
+        spawner.Stop();                                   // Running false: OnStopped is out of the picture
+        TriggerSystem.Instance.ActivateTriggers(spawner); // stale registration behind a false flag
+        Assert.True(TriggerSystem.Instance.IsRegistered(spawner));
+
+        spawner.Delete();                                 // only OnDelete can clean this up
+        Assert.False(TriggerSystem.Instance.IsRegistered(spawner));
+    }
+
+    private static XmlNode ParseNode(string xml)
+    {
+        var doc = new XmlDocument();
+        doc.LoadXml(xml);
+        return doc.DocumentElement;
+    }
+
+    // ProximityRange is the attribute ParseXmlSpawnerNode maps to a proximity trigger definition plus
+    // TriggerActivated = true, so this form exercises trigger (de)registration alongside Running.
+    private static string XmlSpawnerNode(string running)
+    {
+        var runningAttribute = running != null ? $" Running=\"{running}\"" : string.Empty;
+        return "<XmlSpawner X=\"1500\" Y=\"1500\" Z=\"0\" Map=\"Felucca\"" + runningAttribute +
+               " ProximityRange=\"8\"><SpawnObjects><Object Type=\"Rabbit\" MaxCount=\"1\" /></SpawnObjects></XmlSpawner>";
+    }
+
+    // The SpawnPoint form has no trigger-mapped attribute, so these tests assert Running only.
+    private static string SpawnPointNode(string running)
+    {
+        var runningAttribute = running != null ? $" Running=\"{running}\"" : string.Empty;
+        return "<SpawnPoint X=\"1500\" Y=\"1500\" Z=\"0\" Map=\"Felucca\"" + runningAttribute +
+               " Creatures=\"Rabbit\" />";
+    }
+
+    [Fact]
+    public void Migrator_RunningFalse_ProducesStoppedSpawnerWithNoRegisteredTriggers()
+    {
+        var stopped = XmlSpawnerMigrator.ParseXmlSpawnerNode(ParseNode(XmlSpawnerNode("false")));
+        Assert.False(stopped.Running);
+        Assert.False(TriggerSystem.Instance.IsRegistered(stopped));
+        stopped.Delete();
+
+        // Running="true" (the same construction path) must still register and run, so the fix for the
+        // false case did not just make everything stop.
+        var running = XmlSpawnerMigrator.ParseXmlSpawnerNode(ParseNode(XmlSpawnerNode("true")));
+        Assert.True(running.Running);
+        Assert.True(TriggerSystem.Instance.IsRegistered(running));
+        running.Delete();
+    }
+
+    [Fact]
+    public void SpawnPointMigrator_RunningFalse_ProducesStoppedSpawner()
+    {
+        var stopped = XmlSpawnerMigrator.ParseSpawnPointNode(ParseNode(SpawnPointNode("false")));
+        Assert.False(stopped.Running);
+        stopped.Delete();
+
+        // Running absent defaults to true - the historical "always start" behavior for a form that had
+        // no Running attribute before this fix.
+        var running = XmlSpawnerMigrator.ParseSpawnPointNode(ParseNode(SpawnPointNode(null)));
+        Assert.True(running.Running);
+        running.Delete();
+    }
+}

Projects/ModernSpawner.Tests/Core/XmlSpawnerImporterEntryDelayTests.cs

@@ -0,0 +1,108 @@
+using System;
+using System.IO;
+using Server.Engines.ModernSpawner.Serialization;
+using Xunit;
+
+namespace Server.Engines.ModernSpawner.Tests;
+
+/// <summary>
+/// Covers the sentinel-based min/max delay handling in
+/// <see cref="XmlSpawnerImporter"/>'s entry parsing (Task 3: no nullable value types). This needs a
+/// resolvable <see cref="Map"/>, so it runs against the real world in the sequential collection
+/// rather than as a pure unit test.
+/// </summary>
+[Collection("Sequential ModernSpawner Tests")]
+public class XmlSpawnerImporterEntryDelayTests
+{
+    private static string BuildXml(string name, string objects2) => $"""
+        <Spawns>
+          <Points>
+            <Name>{name}</Name>
+            <Map>Felucca</Map>
+            <CentreX>1500</CentreX>
+            <CentreY>1500</CentreY>
+            <CentreZ>0</CentreZ>
+            <X>1500</X>
+            <Y>1500</Y>
+            <Width>0</Width>
+            <Height>0</Height>
+            <Range>4</Range>
+            <MaxCount>5</MaxCount>
+            <MinDelay>5</MinDelay>
+            <MaxDelay>10</MaxDelay>
+            <DelayInSec>False</DelayInSec>
+            <ProximityRange>-1</ProximityRange>
+            <Team>0</Team>
+            <IsGroup>False</IsGroup>
+            <IsRunning>False</IsRunning>
+            <SmartSpawning>False</SmartSpawning>
+            <Objects2>{objects2}</Objects2>
+          </Points>
+        </Spawns>
+        """;
+
+    private static ModernSpawner FindByName(string name)
+    {
+        foreach (var item in World.Items.Values)
+        {
+            if (item is ModernSpawner spawner && spawner.Name == name)
+            {
+                return spawner;
+            }
+        }
+
+        Assert.Fail($"No imported ModernSpawner named '{name}' was found in the world.");
+        return null;
+    }
+
+    [Fact]
+    public void ParseEntry_NoDelayTokens_LeavesEntryAtSpawnerDefault()
+    {
+        var name = "ImporterDelayTest-" + Guid.NewGuid();
+        var tempFile = Path.GetTempFileName();
+        try
+        {
+            File.WriteAllText(tempFile, BuildXml(name, "Rabbit:MX=3"));
+
+            var result = XmlSpawnerImporter.ImportFromFile(tempFile, respawn: false);
+            Assert.Equal(1, result.Imported);
+
+            var spawner = FindByName(name);
+            var entry = Assert.Single(spawner.ModernEntries);
+
+            Assert.Equal(TimeSpan.Zero, entry.MinDelay);
+            Assert.Equal(spawner.MinDelay, entry.EffectiveMinDelay);
+
+            spawner.Delete();
+        }
+        finally
+        {
+            File.Delete(tempFile);
+        }
+    }
+
+    [Fact]
+    public void ParseEntry_DnToken_SetsMinDelayInMinutes()
+    {
+        var name = "ImporterDelayTest-" + Guid.NewGuid();
+        var tempFile = Path.GetTempFileName();
+        try
+        {
+            File.WriteAllText(tempFile, BuildXml(name, "Rabbit:MX=3:DN=2"));
+
+            var result = XmlSpawnerImporter.ImportFromFile(tempFile, respawn: false);
+            Assert.Equal(1, result.Imported);
+
+            var spawner = FindByName(name);
+            var entry = Assert.Single(spawner.ModernEntries);
+
+            Assert.Equal(TimeSpan.FromMinutes(2), entry.MinDelay);
+
+            spawner.Delete();
+        }
+        finally
+        {
+            File.Delete(tempFile);
+        }
+    }
+}

Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs

@@ -81,6 +81,9 @@ public static void Initialize()
             World.ExitSerializationThreads();
 
             DecayScheduler.Configure();
+            // WallTimeWindowTrigger.Activate schedules through EventScheduler.Shared, which is null
+            // until this runs (production reaches it through UOContent's Configure pass).
+            Server.Engines.Events.EventScheduler.Configure();
             // Without npc-speeds.json every BaseCreature constructor throws.
             Server.Mobiles.NPCSpeeds.Configure();
             Server.Engines.Spawners.SpawnerJsonSerializer.Configure();

Projects/ModernSpawner/Core/ModernSpawner.cs

@@ -81,11 +81,31 @@ public partial class ModernSpawner : Spawner
     private List<string> _triggerDefinitions = [];
 
     /// <summary>
-    /// Whether this spawner is trigger-activated (vs. timer-based).
+    /// Whether this spawner is trigger-activated (vs. timer-based). Master switch for this spawner's
+    /// trigger definitions: setting it registers or unregisters the triggers immediately through
+    /// <see cref="EnsureTriggersActive" />, so there is no window where the flag and the trigger
+    /// registry disagree. The backing field is generated; serialization order 9 is unchanged.
     /// </summary>
-    [SerializableField(9)]
-    [SerializedCommandProperty(AccessLevel.Developer)]
-    private bool _triggerActivated;
+    // Hand-written [SerializableProperty] rather than [SerializableField(9, fieldChanged:)] so the
+    // registration call sits at the mutation point with this doc comment; the generated pipeline
+    // (equality check -> assign -> MarkDirty -> callback) is equivalent.
+    [SerializableProperty(9)]
+    [CommandProperty(AccessLevel.Developer)]
+    public bool TriggerActivated
+    {
+        get => _triggerActivated;
+        set
+        {
+            if (_triggerActivated == value)
+            {
+                return;
+            }
+
+            _triggerActivated = value;
+            this.MarkDirty();
+            EnsureTriggersActive();
+        }
+    }
 
     /// <summary>
     /// External trigger state - set by trigger system.
@@ -578,7 +598,8 @@ private void MaybeAutoResetSequence()
     /// <summary>
     /// Brings this spawner's trigger registrations in line with its current state, and is the only
     /// caller of <see cref="ITriggerSystem.ActivateTriggers"/> outside the trigger system itself.
-    /// <see cref="ITriggerSystem.ActivateTriggers"/> appends rather than replaces, so this
+    /// <see cref="ITriggerSystem.ActivateTriggers"/> replaces the spawner's batch in the registry but
+    /// appends to the per-type dispatch lists, so a second call would duplicate dispatch; this
     /// deactivates first and is therefore safe to call any number of times. Every construction path
     /// that can leave a spawner running with triggers already set - start, deserialization, dupe,
     /// import, migration - ends here, because <see cref="BaseSpawner.Start"/> only reaches
@@ -622,10 +643,9 @@ protected override void OnStopped()
             ScriptEngine.Instance.Execute(deactivateScript, new ScriptContext(null, this));
         }
 
-        if (_triggerActivated)
-        {
-            TriggerSystem.Instance.DeactivateTriggers(this);
-        }
+        // DeactivateTriggers is a no-op when nothing is registered, so no flag check: the flag can be
+        // cleared after registration and must not leave a stale entry behind.
+        TriggerSystem.Instance.DeactivateTriggers(this);
     }
 
     /// <inheritdoc />
@@ -1036,11 +1056,9 @@ public override void OnDelete()
         // Unsubscribe from extended area movement before deletion
         UnsubscribeFromExtendedAreaMovement();
 
-        // Deactivate triggers before deletion
-        if (_triggerActivated)
-        {
-            TriggerSystem.Instance.DeactivateTriggers(this);
-        }
+        // Deactivate triggers before deletion. DeactivateTriggers is a no-op when nothing is registered,
+        // so no flag check: the flag can be cleared after registration and must not leave a stale entry behind.
+        TriggerSystem.Instance.DeactivateTriggers(this);
 
         base.OnDelete();
     }