Fix/magic effects read race

[eduardosmaniotto]

Fixes #939 — Offline helper tick crashes with ArgumentException: Destination array was not long enough.

Problem

Bots fail their offline helper tick with:

System.ArgumentException: Destination array was not long enough.
at MUnique.OpenMU.GameLogic.Offline.BuffHandler.IsEffectActive(...)
at MUnique.OpenMU.GameLogic.Offline.BuffHandler.NeedsPartyBuff(...)
at MUnique.OpenMU.GameLogic.Offline.BuffHandler.TryApplyPartyBuffAsync(...)

Each occurrence aborts that bot's tick, so the buff, loot, and attack
steps behind it don't run that round.

Root cause

MagicEffectsList.ActiveEffects is a plain SortedList mutated on
effect-expiry timer threads, but readers enumerated it lock-free via
Values.ToArray() (BuffHandler.IsEffectActive,
BotBuffHandler.HasEffect, and ~10 other call sites). ToArray()
sizes its destination from Count, then copies — if an effect is
added or refreshed in between, the source outgrows the destination
and the copy throws. Likelihood scales with party-buff scan
frequency: every ~500 ms tick, per in-range party member, against a
list changing on independent timers.

Fix

• MagicEffectsList now owns all synchronization (a Lock; every
  previously locked region was await-free) and exposes thread-safe
  queries: ContainsEffect, ContainsAnyEffect, HasEffect,
  TryGetEffect, GetActiveEffectsSnapshot(+Async).
• All GameLogic and GameServer readers migrated to the new API; the
  directly exposed mutable collection is removed so the invariant is
  enforced rather than advisory.
• Tests seed state through the real AddEffectAsync path and assert
  via snapshots.

[sven-n]

Review

Overall: this is the right fix. Moving synchronization into MagicEffectsList and deleting the publicly exposed mutable IDictionary turns an advisory invariant into a compiler-enforced one, and the try/catch "torn read counts as active" workarounds in BuffHandler.IsEffectActive / BotBuffHandler.HasEffect disappear instead of being papered over. The migration looks complete — git grep ActiveEffects on the branch returns only the new GetActiveEffectsSnapshot members, so nothing outside GameLogic/GameServer was missed. System.Threading.Lock is fine here (net10.0), and every region that was converted from AsyncLock to lock is genuinely await-free.

A few things worth looking at:

1. AddEffectAsync subscribes to EffectTimeOut after releasing the lock (pre-existing, but this PR is the place to close it)

lock (this._sync)
{
    ...
    this._activeEffects.Add(effect.Id, effect);
    this._contains[effect.Id] = true;
    ...
}

if (added)
{
    effect.EffectTimeOut += this.OnEffectTimeOutAsync;   // <-- after the lock

MagicEffect starts its _finishTimer in its constructor, so for a short-duration effect (or just an unlucky scheduling/GC pause) the timer can fire between _activeEffects.Add and the +=. MagicEffect.OnEffectTimeOutAsync then sees a null handler and, worse, DisposeAsyncCore sets EffectTimeOut = null — so the later subscription is on a corpse. The entry stays in _activeEffects with _contains[id] == true forever:

• the effect can never be re-applied — AddEffectAsync routes to UpdateEffect, which calls ResetTimer() on the disposed effect and throws ObjectDisposedException;
• ClearAllEffectsAsync spins forever on it: AsyncDisposable.DisposeAsync short-circuits when IsDisposed, so the handler never runs, the entry is never removed, and while (true) never breaks. That loop is reachable from DisposeAsyncCore, i.e. player logout.

Since the whole point of the PR is to make this class own its invariants, moving the subscription inside the lock (before/right after the Add) fixes it — a timer callback firing concurrently would just block on _sync until the add completes, which is exactly the desired ordering.

Independently, ClearAllEffectsAsync's new while (true) would be safer as "if the head didn't change after DisposeAsync, force-remove it and continue" rather than an unbounded retry on the same element — the old while (ActiveEffects.Any()) had the same hazard, but it's cheap to make it non-fatal now.

2. Asymmetric power-up handling around the lock

AddEffectAsync/UpdateEffect call _owner.Attributes.AddElement/RemoveElement inside _sync, while OnEffectTimeOutAsync calls RemoveElement outside it (after removing the entry). Both were true before, but with a blocking lock instead of an AsyncLock the cost changes: attribute recomposition can cascade through dependent attributes and raise change notifications, and that now runs on a thread that other threads' ContainsEffect/HasEffect calls are blocked behind. Two suggestions:

• consider narrowing the lock to just the list + _contains mutation and doing the attribute work outside it (matching what OnEffectTimeOutAsync already does), or
• at minimum document why the add path must hold the lock across attribute mutation while the remove path must not — right now a reader has to guess which one is intentional.

3. PlayerInvisibilityExtensions changes the matching predicate

- var activeEffect = ...ActiveEffects.Values.FirstOrDefault(e => e.Definition == invisibleEffect);
+ if (!player.MagicEffectList.TryGetEffect(invisibleEffect.Number, out var activeEffect) || activeEffect is null)

This is a lookup by number rather than by definition reference. Equivalent as long as MagicEffect.Id => Definition.Number and numbers are unique per definition (they are today, and the number is the key anyway) — but it's a silent semantic change tucked into an otherwise mechanical migration; a one-line comment, or a HasEffect-style overload that returns the effect, would keep the intent explicit. Also, || activeEffect is null is dead: TryGetEffect is annotated [NotNullWhen(true)].

4. Smaller points

• ContainsEffect(int) silently returns false for out-of-short values. That hides a caller bug instead of surfacing it. The only reason the overload exists is that EffectNumbers constants are const int — changing those to const short (they're all ≤ 0xFF) would let you drop the overload entirely.
• GetActiveEffectsSnapshotAsync() is now just ValueTask.FromResult(GetActiveEffectsSnapshot()), and TryGetActiveEffectOfSubTypeAsync no longer awaits anything. Internal callers (ClearAllEffectsProducingSpecificStatAsync, ClearEffectsAfterDeathAsync) should call the sync versions; keeping the …Async variants for external source compat is fine, but they're worth marking as such so nobody assumes there's I/O behind them.
• HasEffect's values[i]?.Definition — SortedList<short, MagicEffect> can't hold nulls now that insertion is internal-only; the null-conditional is a leftover from the torn-read era and can go.
• Removing the public ActiveEffects property is a breaking change for out-of-tree plug-ins. Intentional and correct, but worth a line in the PR description / release notes (or a short-lived [Obsolete] read-only snapshot property).
• src/GameLogic/PlayerActions/WizardTeleportAction.cs ends without a trailing newline.
• The two BotBuffHandlerTests now go through the real AddEffectAsync, which is a good change — but the created MagicEffect (and its 10-minute Timer) is never disposed. An await player.MagicEffectList.ClearAllEffectsAsync() in teardown would keep the test clean.

Testing

The behavioural tests are updated, but nothing here exercises the actual race the PR fixes. A small concurrency test — one task hammering AddEffectAsync/expiry while another loops ContainsEffect/HasEffect/GetActiveEffectsSnapshot for a few thousand iterations — would fail reliably on master and pin the fix. Worth adding given how hard this class is to reason about.

---

Generated by Claude Code