Safeguard block breaking and item damage against collectible behavior exceptions (#282)

[Zaldaryon]

Summary

Safeguard block breaking and tool damage against unhandled exceptions thrown by collectible behaviors, resolving #282.

When an external mod behavior throws an unhandled exception during item damage (such as Toolsmith's NullReferenceException in TinkeringUtility.HandleBrokenTinkeredTool when an untracked tinkered tool breaks), the exception escaped ServerSystemBlockSimulation.TryModifyBlockInWorld directly into ServerMain.DispatchClientPacket_mainthread. On dedicated servers, any unhandled exception in client packet dispatch forcibly kicks the player with "Threw an exception at the server" ("An action you or your client did caused an unhandled exception"), aborting block removal and leaving player inventory dirty states out of sync.

This change introduces defensive handling across the breaking pipeline:

1. Collectible.WalkBehaviors wraps behavior invocations in try-catch blocks. If a mod's OnDamageItem throws, the exception is logged to the server logger while allowing vanilla default actions (item damage reduction and slot clearing on zero durability) to complete cleanly.
2. Collectible.OnBlockBrokenWith wraps behavior callbacks in try-catch blocks so third-party mod failures log rather than crashing the breaking sequence.
3. ServerSystemBlockSimulation.TryModifyBlockInWorld wraps OnBlockBrokenWith and block4.OnBlockBroken in a try-catch block. If an exception occurs and the target block was not yet removed, a fallback block break runs so the block is not left in an inconsistent state, and the player is not disconnected.
4. Added Atlas regression scenarios in ItemBreakResilienceScenarios.cs reproducing both failure modes under dedicated server conditions (IsDedicatedServer == true).

Type

• Bug fix
• Performance
• New feature
• Refactor or cleanup
• Docs or build

Checklist

• .\scripts\extract-patches.ps1 ran clean.
• dotnet build VintageStory.slnx -c Release -p:EmbedPatchedFiles=true is green.
• Every vanilla edit has a // Stratum marker.
• No vanilla source committed.
• Tested on a real server start, not just compilation.

Related issues

Fixes #282

[Pixnop]

The fix works and #282 is closed by it: the branch builds, the suite is green at 22/22 here, both patches apply to a pristine baseline, hunk counters match and every hunk carries a marker. Three things need to change first, and they are all the same question: what happens to the EnumHandling a behavior wrote by ref just before it threw.

1. OnBlockBrokenWith now drops a behavior's veto (patch lines 19-27, generated Collectible.cs:730-736). The if (handled != PassThrough) block and the if (handled == PreventSubsequent) return result; line moved inside the try. The behavior writes handled before it throws, so the throw skips both checks: preventDefault stays false, the loop carries on, and control reaches the vanilla default at the bottom. A protection behavior that sets PreventSubsequent to veto a break and then throws now lets the block break, where before the player was kicked and the block stayed. Move the two checks below the catch. Your second hunk already does it that way, which is what makes the asymmetry stand out.

2. WalkBehaviors keeps the handling the throwing behavior wrote (patch lines 46-58, generated Collectible.cs:3569-3580). The catch swallows the exception, then if (handling == PreventDefault) executeDefault = false; runs on that stale value. For DamageItem the default action is the entire durability update, so a behavior that sets PreventDefault and then throws makes the tool take no damage at all, silently, on every break from then on. That is the exact shape of #282: Toolsmith's OnDamageItem has to set PreventDefault, otherwise vanilla durability would stack on top of its own tinkering. The description says the catch lets "item damage reduction and slot clearing on zero durability complete cleanly", and for the case the issue is about it does the opposite. Reset handling = EnumHandling.PassThrough in the catch, or decide the other way and say so in the marker.

3. The server fallback re-breaks from inside the catch (ServerSystemBlockSimulation patch lines 443-451, generated :883-891). Two separate problems. The retry has no guard of its own, so when the original exception is deterministic and came from that same path, an empty hand on a block whose BlockBehavior.OnBlockBroken throws for instance, and that loop is not one this PR wraps, the second call throws the same exception, it escapes TryModifyBlockInWorld and the player is kicked anyway, this time after the drops already spawned. And the guard GetBlock(pos).BlockId != 0 reads the Default layer, which returns the fluid layer when the solid layer is empty. Break sand under still water with a collectible path that throws after SpawnDropsAndRemoveBlock has run: the solid layer is air, Default hands back the water, the fallback calls OnBlockBroken a second time and the drops are duplicated.

Not blocking, but worth the same pass:

• Hunk 3 has no coverage at all. Revert only that try/catch, rebuild, run the two new scenarios: still 2/2 green, because the two Collectible catches swallow everything before it gets there. Reaching it needs a throwing BlockBehavior or a throwing GetDrops.
• Neither faulty behavior touches bhHandling before throwing, so both scenarios exercise the one case where the default still runs, and neither reads the pickaxe afterwards. A scenario that sets bhHandling = PreventDefault before the throw and asserts GetRemainingDurability actually dropped would have caught point 2.
• The behaviors are appended to pickaxe.CollectibleBehaviors on the shared registry object and never removed. Both scenarios share one server, so whichever runs second carries the first one's faulty behavior too. Restore the array in a finally.
• isDedicatedField?.SetValue(server, true) quietly does nothing the day that compiler-generated field name changes, and both scenarios then pass with all three safeguards reverted. Assert.NotNull on the FieldInfo is one line.
• Logging is unbounded now. The player is no longer kicked, so a deterministic mod exception repeats on every break, two Logger.Error calls with a full stack trace each time. Someone mining at five blocks a second writes ten traces a second for as long as they keep going. Log once per behavior type and collectible, or rate limit.
• api?.Logger?.Error(...) is a silent swallow whenever api is null, and it is only set by OnLoadedNative. OnBlockBrokenWith has world in scope and can log through world.Logger.

One design question, asked as a question rather than a condition. ServerMain.DispatchClientPacket_mainthread is already ours and is already the single place where a packet-handling exception decides whether to kick. A policy there would be one hunk in code we own, rather than three inside vanilla method bodies, two of them in the vsapi fork that every version bump has to carry forward. What pushed you towards the three?