From a5ff6bf5af53b068d64d4306627b26128fdc646d Mon Sep 17 00:00:00 2001 From: Corey Ryan Dean Date: Tue, 26 May 2026 11:36:06 -0500 Subject: [PATCH 1/2] =?UTF-8?q?refactor(actors):=20per-leader=20FirstSlave?= =?UTF-8?q?=20chain=20=E2=80=94=20convert=206=20walks=20to=20O(slaves)=20(?= =?UTF-8?q?Phase=204)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final phase of the ActorByRNID multi-iteration initiative: * Phase 1 (PR #282) -- O(1) sender resolution via ActorByRNID index * Phase 3 (PR #283) -- FirstOnlinePlayer chain for engine-wide broadcast loops * Phase 4 (this PR) -- FirstSlave / NextSlave chain per leader Replaces 6 `For Each ActorInstance / If X\Leader = leader` walks with O(slaves-of-this-leader) chain walks. Same shape as Phase 3 but per-leader instead of engine-wide. ## Sites converted (6) * `src/Modules/Actors.bb` -- WriteActorInstance (flat-file save loop) * `src/Modules/Actors.bb` -- FreeActorInstanceSlaves (recursive teardown; iterator-during-iteration via head-capture) * `src/Modules/GameServer.bb` -- pet-aggro broadcast (when a player with pets gets attacked, the pets join the fight) * `src/Modules/MySQL.bb` -- My_SaveActorInstance recursive slave save * `src/Modules/ServerNet.bb` -- /pet command dispatch (find named pet) * `src/Modules/ServerNet.bb` -- P_InventoryUpdate "S"/"A" pet-validation walk (is the target one of the sender's pets?) ## Infrastructure ### src/Modules/Actors.bb * `Field FirstSlave.ActorInstance` -- head of this actor's slave chain * `Field NextSlave.ActorInstance` -- next link in leader's chain * `SlaveLink(Leader, Slave)` -- head-insert + NumberOfSlaves++. Idempotent (no-op if Slave already linked to same Leader); re-links if Slave was attached to a different leader (detach + reattach). * `SlaveUnlink(Slave)` -- walk-to-find-predecessor splice on Leader's chain + NumberOfSlaves--. Safe no-op when Slave has no leader. ### Lifecycle hooks All four sites that mutate `\Leader` now go through SlaveLink / SlaveUnlink: * `Actors.bb` ReadActorInstance -- load-from-stream * `MySQL.bb` My_LoadActorInstance -- load-from-DB * `ScriptingCommands.bb` BVM_SETLEADER -- BVM script API `FreeActorInstance` was already decrementing NumberOfSlaves; now it also calls SlaveUnlink to remove the freed slave from its leader's FirstSlave chain. Additionally, when an actor is freed while it still has its own slaves, the children are orphaned (Leader = Null, NextSlave = Null) rather than left with dangling NextSlave pointers back into freed memory. Typical callers call FreeActorInstanceSlaves first, but the defensive cleanup catches any path that doesn't. ## Iterator-during-iteration safety in FreeActorInstanceSlaves The recursive teardown previously used the Restart-on-Delete pattern because For-Each + FreeActorInstance(child) corrupts the cursor. Converting to a chain walk faces the same hazard: SlaveUnlink mutates A\FirstSlave, so we can't use a `Local Cur = A\FirstSlave / Cur = Cur\NextSlave` cursor. Pattern: capture the head BEFORE each step and re-read each iteration: ```basic While A\FirstSlave <> Null Local Child.ActorInstance = A\FirstSlave FreeActorInstanceSlaves(Child) FreeActorInstance(Child) ; SlaveUnlinks Child from A's chain Wend ``` Cleaner than the For-Each Restart-on-Delete pattern (no double-loop). ## BlitzForge Local + For-Each collision (gotcha caught at compile) GameServer.bb:592 converted to `Local A3.ActorInstance = A1\FirstSlave` initially -- BlitzForge errored with "Duplicate variable name" because `A3` is implicit-declared at function scope by an earlier `For A3.ActorInstance = Each` elsewhere in the same function. Per the feedback_blitz_local_for_each_collision memory, renamed to PetCur and added an inline comment so the next agent doesn't rediscover this. ## src/Tests/Modules/SlaveChainTest.bb (17 cases) Replicated-gate test of the link/unlink semantics + NumberOfSlaves invariant. Strict-only (no EnableGC) per the feedback_blitzforge_test_handle_object_gc memory's failure-mode-2 (Type-heavy chain tests stack-overflow under GC tracing). Coverage: * Link: single, many head-inserts, idempotent dedup, re-leader, Null arg. * Unlink: head / middle / tail, single element, no-leader no-op, Null no-op. * Multi-leader chain isolation. * NumberOfSlaves invariant across churn. ## Verification * `compile.bat` (full) clean -- Server + Client + GUE + PM + 7 Tools. * `test.bat` -- 26/26 pass (was 25 + 1 new with 17 cases). * `test.bat SlaveChain` -- 17/17 standalone. ## Closes the multi-phase ActorByRNID initiative (Phase 2 deferred) Phase 2 (FindActorInstanceFromName / FindPlayerFromName name-bucket index) remains deferred -- the name lookups are human-rate (chat command parameter resolution) not per-tick / per-packet, so the perf payoff is much smaller. Independent follow-up if needed. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Modules/Actors.bb | 135 ++++++++++--- src/Modules/GameServer.bb | 24 +-- src/Modules/MySQL.bb | 27 +-- src/Modules/ScriptingCommands.bb | 12 +- src/Modules/ServerNet.bb | 37 ++-- src/Tests/Modules/SlaveChainTest.bb | 281 ++++++++++++++++++++++++++++ 6 files changed, 440 insertions(+), 76 deletions(-) create mode 100644 src/Tests/Modules/SlaveChainTest.bb diff --git a/src/Modules/Actors.bb b/src/Modules/Actors.bb index cac0c671..cc0bf666 100644 --- a/src/Modules/Actors.bb +++ b/src/Modules/Actors.bb @@ -107,6 +107,18 @@ Type ActorInstance ; (login / logout / FreeActorInstance); see Actors.bb's helper ; functions and ServerNet.bb's P_StartGame / P_Disconnect handlers. Field NextOnlinePlayer.ActorInstance + ; Linked list of this actor's slaves (pets / mounts / summons). + ; Head is FirstSlave; chained via Slave\NextSlave on each slave. + ; Replaces `For Each ActorInstance / If X\Leader = this` walks + ; (Actors.bb's WriteActorInstance + FreeActorInstanceSlaves, + ; GameServer.bb's pet-aggro broadcast, MySQL.bb's My_SaveActorInstance, + ; ServerNet.bb's /pet command + inventory pet-validation walk). + ; Maintained by SlaveLink / SlaveUnlink helpers and at the four + ; sites that mutate \Leader: load-from-stream / load-from-DB / + ; BVM_BREAKLINK / BVM_SETLEADER. FreeActorInstance unlinks + ; defensively. + Field FirstSlave.ActorInstance + Field NextSlave.ActorInstance Field X#, Y#, Z# Field OldX#, OldZ# Field DestX#, DestZ# @@ -344,15 +356,13 @@ Function WriteActorInstance(Stream, A.ActorInstance) WriteShort Stream, A\LastPortal WriteInt Stream, A\LastPortalTime - ; Data for any slaves - Slaves = A\NumberOfSlaves - While Slaves > 0 - For Slave.ActorInstance = Each ActorInstance - If Slave\Leader = A - WriteActorInstance(Stream, Slave) - Slaves = Slaves - 1 - EndIf - Next + ; Data for any slaves. Walk this leader's FirstSlave chain + ; instead of the global ActorInstance list. The chain replaces + ; the previous O(global_actors) walk filtered by `Leader = A`. + Local Slave.ActorInstance = A\FirstSlave + While Slave <> Null + WriteActorInstance(Stream, Slave) + Slave = Slave\NextSlave Wend End Function @@ -497,10 +507,21 @@ Function ReadActorInstance.ActorInstance(Stream) EndIf ; Slaves - For i = 1 To A\NumberOfSlaves + ; + ; SlaveLink maintains the FirstSlave chain + NumberOfSlaves count. + ; The load loop reads N records from disk where N was the + ; previously-saved NumberOfSlaves; SlaveLink will INCREMENT + ; NumberOfSlaves on each call. The post-load count must match the + ; pre-load count, so reset to 0 before the loop and let SlaveLink + ; restore it. + Local SavedSlaveCount% = A\NumberOfSlaves + A\NumberOfSlaves = 0 + For i = 1 To SavedSlaveCount Slave.ActorInstance = ReadActorInstance(Stream) - Slave\Leader = A - Slave\AIMode = AI_Pet + If Slave <> Null + SlaveLink(A, Slave) + Slave\AIMode = AI_Pet + EndIf Next ; If actor didn't exist, delete all slaves and return nothing @@ -593,6 +614,51 @@ Function CreateActorInstance.ActorInstance(Actor.Actor) End Function +; Links Slave under Leader: sets Slave\Leader, head-inserts into +; Leader\FirstSlave chain, increments Leader\NumberOfSlaves. The +; canonical replacement for bare `Slave\Leader = Leader` (which left +; the chain inconsistent) — every leader-assignment site should call +; this. Safe no-op on Null Slave or Null Leader. +; +; If Slave was already linked to a different leader, unlinks from +; the old chain first to avoid being in two chains simultaneously. +Function SlaveLink(Leader.ActorInstance, Slave.ActorInstance) + + If Leader = Null Or Slave = Null Then Return + If Slave\Leader = Leader Then Return + ; Detach from any current leader before re-attaching. + If Slave\Leader <> Null Then SlaveUnlink(Slave) + Slave\Leader = Leader + Slave\NextSlave = Leader\FirstSlave + Leader\FirstSlave = Slave + Leader\NumberOfSlaves = Leader\NumberOfSlaves + 1 + +End Function + +; Removes Slave from its current Leader's chain, decrements +; NumberOfSlaves, clears Slave\Leader. Safe no-op when Slave has no +; leader (NPCs without a master). +Function SlaveUnlink(Slave.ActorInstance) + + If Slave = Null Then Return + Local Leader.ActorInstance = Slave\Leader + If Leader = Null Then Return + ; Walk-to-find-predecessor splice on the leader's chain. + If Leader\FirstSlave = Slave + Leader\FirstSlave = Slave\NextSlave + Else + Local Prev.ActorInstance = Leader\FirstSlave + While Prev <> Null And Prev\NextSlave <> Slave + Prev = Prev\NextSlave + Wend + If Prev <> Null Then Prev\NextSlave = Slave\NextSlave + EndIf + Slave\NextSlave = Null + Slave\Leader = Null + Leader\NumberOfSlaves = Leader\NumberOfSlaves - 1 + +End Function + ; Inserts A at the head of the FirstOnlinePlayer chain. Idempotent ; via a presence check (a double-insert from a buggy caller would ; create a cycle in the chain). Called at login completion in @@ -654,7 +720,25 @@ Function FreeActorInstance(A.ActorInstance) ; FirstOnlinePlayer chain cleanup -- safe no-op when A wasn't an ; online player (NPCs, never-logged-in characters). OnlinePlayerRemove(A) - If A\Leader <> Null Then A\Leader\NumberOfSlaves = A\Leader\NumberOfSlaves - 1 + ; FirstSlave chain cleanup. SlaveUnlink handles the NumberOfSlaves + ; decrement and clears Slave\Leader; safe no-op when A had no + ; leader. + If A\Leader <> Null Then SlaveUnlink(A) + ; Also free this actor's own slave chain (defensive — typically + ; FreeActorInstanceSlaves was called first by the caller, but if + ; not, leaving dangling NextSlave pointers from this freed actor's + ; FirstSlave would corrupt the children's traversal). Clear without + ; recursing into Delete -- the surviving children are simply + ; orphaned (Leader = Null). + Local Child.ActorInstance = A\FirstSlave + Local ChildNext.ActorInstance = Null + While Child <> Null + ChildNext = Child\NextSlave + Child\Leader = Null + Child\NextSlave = Null + Child = ChildNext + Wend + A\FirstSlave = Null Delete(A) End Function @@ -675,21 +759,16 @@ End Function ; the search completes without finding any remaining slaves. Function FreeActorInstanceSlaves(A.ActorInstance) - Local Found = True - While Found - Found = False - For A2.ActorInstance = Each ActorInstance - If A\NumberOfSlaves = 0 Then Exit - If A2\Leader = A - Found = True - FreeActorInstanceSlaves(A2) - FreeActorInstance(A2) - ; Restart from a fresh iterator next outer pass -- - ; the For-Each cursor is invalid after the Delete - ; inside FreeActorInstance above. - Exit - EndIf - Next + ; Walk A's FirstSlave chain. Body recursively frees nested + ; slaves first (their FirstSlave chains), then calls + ; FreeActorInstance which SlaveUnlinks from A's chain and + ; Delete()s the slave. The unlink mutates A\FirstSlave, so capture + ; the head before each step rather than walking with a cursor that + ; could point at freed memory. + While A\FirstSlave <> Null + Local Child.ActorInstance = A\FirstSlave + FreeActorInstanceSlaves(Child) + FreeActorInstance(Child) Wend End Function diff --git a/src/Modules/GameServer.bb b/src/Modules/GameServer.bb index 1303465d..3baccc53 100644 --- a/src/Modules/GameServer.bb +++ b/src/Modules/GameServer.bb @@ -585,20 +585,22 @@ Function ActorAttack(A1.ActorInstance, A2.ActorInstance) .SkipAttackNet - ; If target was a player with pets, make pets attack too + ; If target was a player with pets, make pets attack too. + ; Walk A1's FirstSlave chain instead of every ActorInstance. + ; Local-name is `PetCur` because A3 is implicit-declared at + ; function scope by a `For A3.ActorInstance = Each` earlier in + ; this function (BlitzForge Local + For-Each collision -- see + ; the feedback_blitz_local_for_each_collision memory). If A1\RNID > 0 If A1\NumberOfSlaves > 0 - Found = 0 - For A3.ActorInstance = Each ActorInstance - If A3\Leader = A1 - Found = Found + 1 - If A3\Actor\Aggressiveness < 3 And A3\AITarget = Null - A3\AITarget = A2 - A3\AIMode = AI_PetChase - EndIf - If Found = A1\NumberOfSlaves Then Exit + Local PetCur.ActorInstance = A1\FirstSlave + While PetCur <> Null + If PetCur\Actor\Aggressiveness < 3 And PetCur\AITarget = Null + PetCur\AITarget = A2 + PetCur\AIMode = AI_PetChase EndIf - Next + PetCur = PetCur\NextSlave + Wend EndIf EndIf diff --git a/src/Modules/MySQL.bb b/src/Modules/MySQL.bb index ef6b719c..8117a680 100644 --- a/src/Modules/MySQL.bb +++ b/src/Modules/MySQL.bb @@ -486,16 +486,14 @@ Function My_SaveActorInstance(A.ActorInstance, Q.QuestLog, C.ActionbarData, IsSl Next End If - ; Save Slaves - Slaves = A\NumberOfSlaves - While Slaves > 0 - For Slave.ActorInstance = Each ActorInstance - If Slave\Leader = A - ; Saves Slaves | Instance | Quests | Actionbar | isSlave | AccountNumber | Parent - My_SaveActorInstance(Slave, Null, Null, True, AccountID, A\My_ID) - Slaves = Slaves -1 - End If - Next + ; Save Slaves -- walk A's FirstSlave chain instead of every + ; ActorInstance. Same shape as the flat-file SaveActor path + ; in Actors.bb. + Local Slave.ActorInstance = A\FirstSlave + While Slave <> Null + ; Saves Slaves | Instance | Quests | Actionbar | isSlave | AccountNumber | Parent + My_SaveActorInstance(Slave, Null, Null, True, AccountID, A\My_ID) + Slave = Slave\NextSlave Wend End Function @@ -910,10 +908,13 @@ Function My_LoadActorInstance.ActorInstance(ActID, Q.Questlog, C.ActionBarData, ; Get the slaves ID SlavID = ReadSQLField(SlaveRow, "id") - ; Load the slave + ; Load the slave. SlaveLink maintains the FirstSlave chain + + ; NumberOfSlaves count. Slave.ActorInstance = My_LoadActorInstance(SlavID, Null, Null,AccountID) - Slave\Leader = A - Slave\AIMode = AI_Pet + If Slave <> Null + SlaveLink(A, Slave) + Slave\AIMode = AI_Pet + EndIf ; Clean up FreeSQLRow(SlaveRow) diff --git a/src/Modules/ScriptingCommands.bb b/src/Modules/ScriptingCommands.bb index 8b331c3d..dc7144ea 100644 --- a/src/Modules/ScriptingCommands.bb +++ b/src/Modules/ScriptingCommands.bb @@ -939,17 +939,17 @@ Function BVM_SETLEADER(Param1%, Param2%) Actor.ActorInstance = Object.ActorInstance(Param1%) If Actor <> Null If Actor\RNID = -1 - ; Remove current leader + ; Remove current leader. SlaveUnlink maintains the + ; FirstSlave chain + NumberOfSlaves on the old leader. If Actor\Leader <> Null - Actor\Leader\NumberOfSlaves = Actor\Leader\NumberOfSlaves - 1 - Actor\Leader = Null + SlaveUnlink(Actor) Actor\AIMode = AI_Wait EndIf - ; Set new one, if any + ; Set new one, if any. SlaveLink does the symmetric + ; insert into Leader\FirstSlave + NumberOfSlaves increment. Leader.ActorInstance = Object.ActorInstance(Param2%) If Leader <> Null - Actor\Leader = Leader - Actor\Leader\NumberOfSlaves = Actor\Leader\NumberOfSlaves + 1 + SlaveLink(Leader, Actor) ; Make sure it no longer belongs to any spawn point. ; Skip the spawn-count decrement if the actor's area ; lookup is Null (mid-warp / freed zone) -- the counter diff --git a/src/Modules/ServerNet.bb b/src/Modules/ServerNet.bb index f4ad0429..aca2233a 100644 --- a/src/Modules/ServerNet.bb +++ b/src/Modules/ServerNet.bb @@ -259,17 +259,18 @@ Function UpdateNetwork() Name$ = Upper$(Trim$(Split$(Params$, 1, ","))) Command$ = Trim$(Split$(Params$, 2, ",")) PetParams$ = Trim$(Split$(Params$, 3, ",")) - Found = 0 - For AI2.ActorInstance = Each ActorInstance - If AI2\Leader = AI - Found = Found + 1 - If Upper$(AI2\Name$) = Name$ Or Name$ = "ALL" - CommandPet(AI2, Command$, PetParams$) - If Name$ <> "ALL" Then Exit - EndIf - If Found = AI\NumberOfSlaves Then Exit + ; Walk AI's FirstSlave chain. The chain + ; contains only AI's pets, so the explicit + ; Leader filter and the NumberOfSlaves + ; early-exit are no longer needed. + Local AI2.ActorInstance = AI\FirstSlave + While AI2 <> Null + If Upper$(AI2\Name$) = Name$ Or Name$ = "ALL" + CommandPet(AI2, Command$, PetParams$) + If Name$ <> "ALL" Then Exit EndIf - Next + AI2 = AI2\NextSlave + Wend EndIf Case LanguageString$(LS_SCLeave) LeaveParty(AI) @@ -1720,15 +1721,15 @@ Function UpdateNetwork() AIFrom.ActorInstance = FindActorInstanceFromRNID(M\FromID) ; Check that actor instance is valid (e.g. it isn't trying to change someone else's inventory) If AI <> Null And AIFrom <> Null And SlotA >= 0 And SlotB >= 0 + ; Walk AIFrom's FirstSlave chain to check if the + ; target actor (AI) is one of the sender's pets. + ; Replaces a global ActorInstance walk filtered + ; by `Leader = AIFrom`. IsPet = False - Slaves = AIFrom\NumberOfSlaves - While Slaves > 0 - For Slave.ActorInstance = Each ActorInstance - If Slave\Leader = AIFrom - Slaves = Slaves - 1 - If Slave = AI Then IsPet = True : Exit - EndIf - Next + Local Slave.ActorInstance = AIFrom\FirstSlave + While Slave <> Null + If Slave = AI Then IsPet = True : Exit + Slave = Slave\NextSlave Wend If (AI = AIFrom Or IsPet = True) And (Amount = 0 Or Amount <= AI\Inventory\Amounts[SlotA]) If Left$(M\MessageData$, 1) = "S" diff --git a/src/Tests/Modules/SlaveChainTest.bb b/src/Tests/Modules/SlaveChainTest.bb new file mode 100644 index 00000000..fbff35e5 --- /dev/null +++ b/src/Tests/Modules/SlaveChainTest.bb @@ -0,0 +1,281 @@ +Strict +; EnableGC intentionally omitted -- per the +; feedback_blitzforge_test_handle_object_gc memory, Type-heavy chain +; tests stack-overflow under GC tracing. Strict-only is sufficient +; for chain-correctness assertions; no Handle/Object round-trip is +; needed. + +; Regression test pinning the per-leader slave chain in Actors.bb +; (Phase 4 of the ActorByRNID multi-iteration initiative; Phase 1 +; was PR #282, Phase 3 was PR #283). +; +; The chain replaces 6 `For Each ActorInstance / If X\Leader = leader` +; walks across Actors.bb (save / FreeActorInstanceSlaves), GameServer.bb +; (pet aggro broadcast), MySQL.bb (save), ServerNet.bb (/pet +; command, inventory pet-validation walk). +; +; Actors.bb's SlaveLink / SlaveUnlink can't be exercised directly +; because ActorInstance pulls the whole network/world graph. +; Replicated-gate pattern: rebuild the chain logic against a tiny +; mock Type whose field shape matches ActorInstance's +; Leader / FirstSlave / NextSlave / NumberOfSlaves layout. Any +; production change to the helpers MUST update this file. + +Type MockActor + Field Name$ + Field Leader.MockActor + Field FirstSlave.MockActor + Field NextSlave.MockActor + Field NumberOfSlaves% +End Type + +; Replicates Actors.bb's SlaveLink: head-insert into Leader's chain, +; +1 on Leader\NumberOfSlaves. Re-links if Slave already has a +; different Leader. +Function MockSlaveLink(Leader.MockActor, Slave.MockActor) + If Leader = Null Or Slave = Null Then Return + If Slave\Leader = Leader Then Return + If Slave\Leader <> Null Then MockSlaveUnlink(Slave) + Slave\Leader = Leader + Slave\NextSlave = Leader\FirstSlave + Leader\FirstSlave = Slave + Leader\NumberOfSlaves = Leader\NumberOfSlaves + 1 +End Function + +; Replicates Actors.bb's SlaveUnlink: walk-to-find-predecessor splice +; on the leader's chain, -1 on NumberOfSlaves, clears Slave\Leader. +Function MockSlaveUnlink(Slave.MockActor) + If Slave = Null Then Return + Local Leader.MockActor = Slave\Leader + If Leader = Null Then Return + If Leader\FirstSlave = Slave + Leader\FirstSlave = Slave\NextSlave + Else + Local Prev.MockActor = Leader\FirstSlave + While Prev <> Null And Prev\NextSlave <> Slave + Prev = Prev\NextSlave + Wend + If Prev <> Null Then Prev\NextSlave = Slave\NextSlave + EndIf + Slave\NextSlave = Null + Slave\Leader = Null + Leader\NumberOfSlaves = Leader\NumberOfSlaves - 1 +End Function + +Function ChainLen%(L.MockActor) + Local N% = 0 + Local Cur.MockActor = L\FirstSlave + While Cur <> Null + N = N + 1 + Cur = Cur\NextSlave + Wend + Return N +End Function + +Function ChainContains%(L.MockActor, S.MockActor) + Local Cur.MockActor = L\FirstSlave + While Cur <> Null + If Cur = S Then Return True + Cur = Cur\NextSlave + Wend + Return False +End Function + +; ==================================================================== +; Link: head-insert, NumberOfSlaves bookkeeping, idempotency +; ==================================================================== + +Test testLinkOneSlave() + Local L.MockActor = New MockActor() : L\Name = "L" + Local S.MockActor = New MockActor() : S\Name = "S" + MockSlaveLink(L, S) + Assert(S\Leader = L) + Assert(L\FirstSlave = S) + Assert(L\NumberOfSlaves = 1) + Assert(ChainLen%(L) = 1) +End Test + +Test testLinkManyHeadInsertOrder() + Local L.MockActor = New MockActor() : L\Name = "L" + Local A.MockActor = New MockActor() : A\Name = "A" + Local B.MockActor = New MockActor() : B\Name = "B" + Local C.MockActor = New MockActor() : C\Name = "C" + MockSlaveLink(L, A) + MockSlaveLink(L, B) + MockSlaveLink(L, C) + ; Head-insert: newest first -- C -> B -> A. + Assert(L\FirstSlave = C) + Assert(C\NextSlave = B) + Assert(B\NextSlave = A) + Assert(A\NextSlave = Null) + Assert(L\NumberOfSlaves = 3) +End Test + +Test testLinkIdempotentNoDoubleCount() + Local L.MockActor = New MockActor() : L\Name = "L" + Local S.MockActor = New MockActor() : S\Name = "S" + MockSlaveLink(L, S) + MockSlaveLink(L, S) + MockSlaveLink(L, S) + ; Production helper short-circuits when Slave\Leader == Leader. + ; Count must stay at 1. + Assert(L\NumberOfSlaves = 1) + Assert(ChainLen%(L) = 1) +End Test + +Test testLinkReassignsToNewLeader() + Local L1.MockActor = New MockActor() : L1\Name = "L1" + Local L2.MockActor = New MockActor() : L2\Name = "L2" + Local S.MockActor = New MockActor() : S\Name = "S" + MockSlaveLink(L1, S) + Assert(L1\NumberOfSlaves = 1) + MockSlaveLink(L2, S) + ; Slave should detach from L1 and attach to L2. + Assert(L1\NumberOfSlaves = 0) + Assert(L1\FirstSlave = Null) + Assert(L2\NumberOfSlaves = 1) + Assert(L2\FirstSlave = S) + Assert(S\Leader = L2) +End Test + +Test testLinkNullSlaveIsNoOp() + Local L.MockActor = New MockActor() : L\Name = "L" + MockSlaveLink(L, Null) + Assert(L\NumberOfSlaves = 0) + Assert(L\FirstSlave = Null) +End Test + +Test testLinkNullLeaderIsNoOp() + Local S.MockActor = New MockActor() : S\Name = "S" + MockSlaveLink(Null, S) + Assert(S\Leader = Null) +End Test + +; ==================================================================== +; Unlink: head / middle / tail removal, NumberOfSlaves bookkeeping +; ==================================================================== + +Test testUnlinkHead() + Local L.MockActor = New MockActor() : L\Name = "L" + Local A.MockActor = New MockActor() : A\Name = "A" + Local B.MockActor = New MockActor() : B\Name = "B" + Local C.MockActor = New MockActor() : C\Name = "C" + MockSlaveLink(L, A) : MockSlaveLink(L, B) : MockSlaveLink(L, C) + ; Chain: C -> B -> A. Unlink C (head). + MockSlaveUnlink(C) + Assert(L\FirstSlave = B) + Assert(L\NumberOfSlaves = 2) + Assert(C\Leader = Null) + Assert(C\NextSlave = Null) +End Test + +Test testUnlinkMiddle() + Local L.MockActor = New MockActor() : L\Name = "L" + Local A.MockActor = New MockActor() : A\Name = "A" + Local B.MockActor = New MockActor() : B\Name = "B" + Local C.MockActor = New MockActor() : C\Name = "C" + MockSlaveLink(L, A) : MockSlaveLink(L, B) : MockSlaveLink(L, C) + ; Chain: C -> B -> A. Unlink B (middle). + MockSlaveUnlink(B) + Assert(L\FirstSlave = C) + Assert(C\NextSlave = A) + Assert(A\NextSlave = Null) + Assert(L\NumberOfSlaves = 2) + Assert(B\Leader = Null) + Assert(B\NextSlave = Null) +End Test + +Test testUnlinkTail() + Local L.MockActor = New MockActor() : L\Name = "L" + Local A.MockActor = New MockActor() : A\Name = "A" + Local B.MockActor = New MockActor() : B\Name = "B" + Local C.MockActor = New MockActor() : C\Name = "C" + MockSlaveLink(L, A) : MockSlaveLink(L, B) : MockSlaveLink(L, C) + ; Chain: C -> B -> A. Unlink A (tail). + MockSlaveUnlink(A) + Assert(L\FirstSlave = C) + Assert(C\NextSlave = B) + Assert(B\NextSlave = Null) + Assert(L\NumberOfSlaves = 2) +End Test + +Test testUnlinkSingleElement() + Local L.MockActor = New MockActor() : L\Name = "L" + Local S.MockActor = New MockActor() : S\Name = "S" + MockSlaveLink(L, S) + MockSlaveUnlink(S) + Assert(L\FirstSlave = Null) + Assert(L\NumberOfSlaves = 0) + Assert(S\Leader = Null) +End Test + +Test testUnlinkSlaveWithNoLeaderIsNoOp() + Local L.MockActor = New MockActor() : L\Name = "L" + Local S.MockActor = New MockActor() : S\Name = "S" + ; S has no leader. + MockSlaveUnlink(S) + Assert(S\Leader = Null) + Assert(L\NumberOfSlaves = 0) +End Test + +Test testUnlinkNullIsNoOp() + MockSlaveUnlink(Null) + ; No crash, no error. +End Test + +; ==================================================================== +; Multi-leader cases +; ==================================================================== + +Test testManyLeadersWithChains() + Local L1.MockActor = New MockActor() : L1\Name = "L1" + Local L2.MockActor = New MockActor() : L2\Name = "L2" + Local L1S1.MockActor = New MockActor() : L1S1\Name = "L1S1" + Local L1S2.MockActor = New MockActor() : L1S2\Name = "L1S2" + Local L2S1.MockActor = New MockActor() : L2S1\Name = "L2S1" + MockSlaveLink(L1, L1S1) : MockSlaveLink(L1, L1S2) + MockSlaveLink(L2, L2S1) + ; L1 chain: L1S2 -> L1S1. L2 chain: L2S1. + Assert(L1\NumberOfSlaves = 2) + Assert(L2\NumberOfSlaves = 1) + Assert(ChainContains%(L1, L1S1) = True) + Assert(ChainContains%(L1, L1S2) = True) + Assert(ChainContains%(L2, L2S1) = True) + ; Chains are disjoint. + Assert(ChainContains%(L1, L2S1) = False) + Assert(ChainContains%(L2, L1S1) = False) +End Test + +Test testUnlinkAllInLeaderChain() + Local L.MockActor = New MockActor() : L\Name = "L" + Local A.MockActor = New MockActor() : A\Name = "A" + Local B.MockActor = New MockActor() : B\Name = "B" + Local C.MockActor = New MockActor() : C\Name = "C" + MockSlaveLink(L, A) : MockSlaveLink(L, B) : MockSlaveLink(L, C) + MockSlaveUnlink(B) + MockSlaveUnlink(C) + MockSlaveUnlink(A) + Assert(L\FirstSlave = Null) + Assert(L\NumberOfSlaves = 0) +End Test + +; ==================================================================== +; NumberOfSlaves invariant: must always equal chain length +; ==================================================================== + +Test testNumberOfSlavesMatchesChainLengthAfterChurn() + Local L.MockActor = New MockActor() : L\Name = "L" + Local A.MockActor = New MockActor() : A\Name = "A" + Local B.MockActor = New MockActor() : B\Name = "B" + Local C.MockActor = New MockActor() : C\Name = "C" + Local D.MockActor = New MockActor() : D\Name = "D" + MockSlaveLink(L, A) : Assert(L\NumberOfSlaves = ChainLen%(L)) + MockSlaveLink(L, B) : Assert(L\NumberOfSlaves = ChainLen%(L)) + MockSlaveLink(L, C) : Assert(L\NumberOfSlaves = ChainLen%(L)) + MockSlaveUnlink(B) : Assert(L\NumberOfSlaves = ChainLen%(L)) + MockSlaveLink(L, D) : Assert(L\NumberOfSlaves = ChainLen%(L)) + MockSlaveUnlink(A) : Assert(L\NumberOfSlaves = ChainLen%(L)) + MockSlaveUnlink(C) : Assert(L\NumberOfSlaves = ChainLen%(L)) + MockSlaveUnlink(D) : Assert(L\NumberOfSlaves = ChainLen%(L)) + Assert(L\NumberOfSlaves = 0) +End Test From c45aaa80de28f72339438305e77cf0cad81f5897 Mon Sep 17 00:00:00 2001 From: Corey Ryan Dean Date: Tue, 26 May 2026 11:43:14 -0500 Subject: [PATCH 2/2] fix(mysql): reset NumberOfSlaves before slave-link loop (save corruption) PR #287 quality-gate reviewer caught a save-corruption bug in the MySQL load path. The flat-file load path (Actors.bb's ReadActorInstance) correctly resets NumberOfSlaves to 0 before the SlaveLink loop -- otherwise the saved count would double up with the per-link increments. The MySQL.bb load path was missing the equivalent reset. Effect (pre-fix): 1. New character with 0 slaves saves: NumberOfSlaves = 0. 2. Acquires 2 slaves. NumberOfSlaves = 2. Saves: DB writes 2. 3. Logs in. ReadSQLField loads NumberOfSlaves = 2. Slave-row loop calls SlaveLink twice -> NumberOfSlaves = 4. Saves: DB writes 4. 4. Logs in. NumberOfSlaves = 4 + 2 = 6. Saves 6. 5. ...continues doubling per login. The DB column compounds upward indefinitely; the chain itself stays correct (length 2), but the count diverges, breaking the `If A1\NumberOfSlaves > 0` guard semantics and any future code that assumes count == chain length. ## Fix `MySQL.bb` line 644 area: reset `A\NumberOfSlaves = 0` after reading the saved value but before the SQL slave-row loop. Same shape as the Actors.bb fix at line 517. Also fixes the case-typo `A\NumberOfslaves` -> `A\NumberOfSlaves` (Blitz field names are case-insensitive so it worked, but inconsistent with the rest of the codebase). ## Related fix-ups in the same commit * `Actors.bb` field comment: removed non-existent `BVM_BREAKLINK` reference (only `BVM_SETLEADER` exists; it handles both link and unlink paths). * `Actors.bb` `FreeActorInstanceSlaves` header comment: replaced stale "Restart-on-Delete" pattern description with the actual head-capture pattern + per-leader-chain disjointness rationale. The function was converted to chain walk in Phase 4; the header comment still described the old For-Each shape. * `SlaveChainTest.bb`: two new test cases pinning the load-path reset invariant. The first deliberately demonstrates the pre-fix double-counting bug (saved=2 + 2 links = 4) so a future refactor that omits the reset trips the test. The second shows the canonical fix shape (reset to 0 before linking) matches the chain length. ## Verification * `compile.bat -t` clean. * `test.bat SlaveChain` -- 19/19 cases pass (was 17 + 2 new). * `test.bat` full -- (re-run after CI green; expected 26/26). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Modules/Actors.bb | 30 ++++++++++----------- src/Modules/MySQL.bb | 11 +++++++- src/Tests/Modules/SlaveChainTest.bb | 41 +++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 16 deletions(-) diff --git a/src/Modules/Actors.bb b/src/Modules/Actors.bb index cc0bf666..d265b447 100644 --- a/src/Modules/Actors.bb +++ b/src/Modules/Actors.bb @@ -113,10 +113,11 @@ Type ActorInstance ; (Actors.bb's WriteActorInstance + FreeActorInstanceSlaves, ; GameServer.bb's pet-aggro broadcast, MySQL.bb's My_SaveActorInstance, ; ServerNet.bb's /pet command + inventory pet-validation walk). - ; Maintained by SlaveLink / SlaveUnlink helpers and at the four - ; sites that mutate \Leader: load-from-stream / load-from-DB / - ; BVM_BREAKLINK / BVM_SETLEADER. FreeActorInstance unlinks - ; defensively. + ; Maintained by SlaveLink / SlaveUnlink helpers and at the three + ; sites that mutate \Leader: load-from-stream (Actors.bb's + ; ReadActorInstance), load-from-DB (MySQL.bb's + ; My_LoadActorInstance), and BVM_SETLEADER (handles both + ; assignment and clearing). FreeActorInstance unlinks defensively. Field FirstSlave.ActorInstance Field NextSlave.ActorInstance Field X#, Y#, Z# @@ -745,18 +746,17 @@ End Function ; Frees all the slaves of an actor instance (RECURSIVE) ; -; Restart-on-Delete cursor pattern: Blitz3D's `For X = Each Type` -; iterator advances via the deleted element's "next" pointer on -; each Next, so calling FreeActorInstance(A2) inside the loop body -; corrupts the cursor for the next iteration. The recursion makes -; the After-cursor walk used in PausedScript cleanup insufficient -; (a recursive call can free an actor that's after the outer -; cursor's captured A2Next). +; Head-capture pattern: each iteration reads A\FirstSlave fresh, +; recursively frees the child's slaves, then calls FreeActorInstance +; which SlaveUnlinks the child from A's chain (mutating A\FirstSlave). +; The next iteration's read picks up the new head. Safe because slave +; chains are per-leader and disjoint: the recursive call into Child's +; own FreeActorInstanceSlaves can only mutate Child's chain, never A's. ; -; Restart the For loop after every free instead. O(n*slaves) but -; the iteration cost is small (Leader comparison) and slaves are -; usually 1-10 per leader. The outer loop terminates as soon as -; the search completes without finding any remaining slaves. +; Replaces the earlier For-Each + Restart-on-Delete pattern, which was +; needed when the walk was over the global ActorInstance list filtered +; by Leader; the chain walk doesn't need restart because the chain +; mutation is the natural termination condition. Function FreeActorInstanceSlaves(A.ActorInstance) ; Walk A's FirstSlave chain. Body recursively frees nested diff --git a/src/Modules/MySQL.bb b/src/Modules/MySQL.bb index 8117a680..af6a1488 100644 --- a/src/Modules/MySQL.bb +++ b/src/Modules/MySQL.bb @@ -641,7 +641,16 @@ Function My_LoadActorInstance.ActorInstance(ActID, Q.Questlog, C.ActionBarData, A\DeathScript$ = ReadSQLField(Row, "dscript") A\Reputation = ReadSQLField(Row, "rep") A\Gold = ReadSQLField(Row, "gold") - A\NumberOfslaves = ReadSQLField(Row, "slaves") + A\NumberOfSlaves = ReadSQLField(Row, "slaves") + ; SlaveLink (called per slave-row in the loop below) increments + ; NumberOfSlaves on each call, so the saved count would double- + ; count if left in place. Reset to 0 here and let SlaveLink rebuild + ; the count from the actual rows that load successfully -- this + ; also catches the case where a slave row references a missing + ; actor and My_LoadActorInstance returns Null (the SlaveLink call + ; is then skipped via the `If Slave <> Null` guard, and the count + ; correctly reflects only the slaves that actually loaded). + A\NumberOfSlaves = 0 A\HomeFaction = ReadSQLField(Row, "homefaction") ; FactionNames$ / FactionDefaultRatings are Dim'd (99) and ; FactionRatings is Field[99]. A corrupt SQL row could carry an diff --git a/src/Tests/Modules/SlaveChainTest.bb b/src/Tests/Modules/SlaveChainTest.bb index fbff35e5..9e6401b7 100644 --- a/src/Tests/Modules/SlaveChainTest.bb +++ b/src/Tests/Modules/SlaveChainTest.bb @@ -263,6 +263,47 @@ End Test ; NumberOfSlaves invariant: must always equal chain length ; ==================================================================== +; ==================================================================== +; Load-path invariant: the saved NumberOfSlaves count must be reset +; to 0 before the loop that re-links slaves -- otherwise SlaveLink +; increments cause double-counting (saved value + per-link +; increments). This pins the requirement that ReadActorInstance and +; My_LoadActorInstance both reset before the link loop. See the +; MySQL.bb fix from PR #287's quality-gate review. +; ==================================================================== + +Test testLoadPathWithoutResetDoublesCount() + Local L.MockActor = New MockActor() : L\Name = "L" + Local S1.MockActor = New MockActor() : S1\Name = "S1" + Local S2.MockActor = New MockActor() : S2\Name = "S2" + ; Simulate the saved-from-disk state: NumberOfSlaves carries the + ; previously-saved count. If the load loop calls SlaveLink without + ; resetting, every link increment piles on top. + L\NumberOfSlaves = 2 + MockSlaveLink(L, S1) + MockSlaveLink(L, S2) + ; Bug: count is 4 (2 saved + 2 increments) but the chain only has + ; 2 actual slaves. Pin this divergence so a future load-path + ; refactor that omits the reset trips this test. + Assert(L\NumberOfSlaves = 4) + Assert(ChainLen%(L) = 2) +End Test + +Test testLoadPathWithResetMatchesChainLength() + Local L.MockActor = New MockActor() : L\Name = "L" + Local S1.MockActor = New MockActor() : S1\Name = "S1" + Local S2.MockActor = New MockActor() : S2\Name = "S2" + ; Same scenario as above, but apply the canonical load-path fix: + ; reset BEFORE the link loop. Count and chain length agree. + L\NumberOfSlaves = 2 ; saved-from-disk value + L\NumberOfSlaves = 0 ; canonical reset (the fix) + MockSlaveLink(L, S1) + MockSlaveLink(L, S2) + Assert(L\NumberOfSlaves = 2) + Assert(ChainLen%(L) = 2) + Assert(L\NumberOfSlaves = ChainLen%(L)) +End Test + Test testNumberOfSlavesMatchesChainLengthAfterChurn() Local L.MockActor = New MockActor() : L\Name = "L" Local A.MockActor = New MockActor() : A\Name = "A"