refactor(actors): per-leader FirstSlave chain — convert 6 walks to O(slaves) (Phase 4)#287
Merged
Merged
Conversation
…slaves) (Phase 4) 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) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5ff6bf5af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Slave\Leader = A | ||
| Slave\AIMode = AI_Pet | ||
| If Slave <> Null | ||
| SlaveLink(A, Slave) |
There was a problem hiding this comment.
Calling SlaveLink(A, Slave) here increments A\NumberOfSlaves, but My_LoadActorInstance has already populated that field from the slaves DB column earlier in the same function. For any actor with existing pets, load now double-counts (or further inflates on repeated save/load cycles), breaking the NumberOfSlaves == chain length invariant. This can corrupt persistence paths that still trust the count metadata (e.g., flat-file load reads SavedSlaveCount but save now writes children by traversing FirstSlave), causing record-count mismatches and desynchronized reads.
Useful? React with 👍 / 👎.
…ion) 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) <noreply@anthropic.com>
This was referenced May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Final phase of the ActorByRNID multi-iteration initiative:
Replaces 6
For Each ActorInstance / If X\Leader = leaderwalks with O(slaves-of-this-leader) chain walks. Same shape as Phase 3 but per-leader instead of engine-wide.Non-technical
When the server needs to find an actor's pets (for AI broadcast, save, /pet command, inventory validation), it used to scan every actor in the world and filter by Leader. After this PR each actor maintains a linked list of its own pets, so finding them is a tight per-leader walk.
Sites converted (6)
Actors.bbWriteActorInstancesave loopActors.bbFreeActorInstanceSlavesGameServer.bbMySQL.bbMy_SaveActorInstanceServerNet.bb/petcommand dispatchServerNet.bbP_InventoryUpdate "S"/"A"validationInfrastructure
src/Modules/Actors.bbField FirstSlave.ActorInstance,Field NextSlave.ActorInstanceSlaveLink(Leader, Slave)— head-insert +NumberOfSlaves++. Idempotent; re-links on different-leader assignment.SlaveUnlink(Slave)— walk-to-find-predecessor splice +NumberOfSlaves--. Safe no-op when Slave has no leader.Lifecycle hooks (4 sites)
All
\Leadermutations now go through SlaveLink/SlaveUnlink:Actors.bbReadActorInstance (load-from-stream)MySQL.bbMy_LoadActorInstance (load-from-DB)ScriptingCommands.bbBVM_SETLEADER (BVM script API — both link + unlink paths)FreeActorInstancenow SlaveUnlinks the freed slave defensively, AND clears the freed actor's children's Leader/NextSlave (orphan them rather than leave dangling pointers).Notable details
Iterator-during-iteration in
FreeActorInstanceSlavesThe recursive teardown previously used Restart-on-Delete (For-Each + restart after each child free). Converting to chain walk faces the same hazard because
SlaveUnlinkmutatesA\FirstSlave. Pattern: capture the head BEFORE each step:BlitzForge Local + For-Each collision
GameServer.bb:592's
Local A3.ActorInstance = A1\FirstSlaveerrored "Duplicate variable name" becauseA3is implicit-declared at function scope by an earlierFor A3 = Eachelsewhere in the function. Per the saved memory, renamed toPetCurwith an inline comment.Acceptance criteria
Test plan
compile.batexit 0 (full, no-t)test.bat— 26/26 passtest.bat SlaveChain— 17/17 cases pass standaloneTrade-offs / deferred
--checkmodes, P_FetchItems cleanup.🤖 Generated with Claude Code