Fix bot purge failure on chunked JSON deserialization

[eduardosmaniotto]

JsonObjectLoader loads accounts via Postgres JSON + SequentialAccess.GetStream() (forward-only, chunked), and JsonObjectDeserializer passes that stream straight to JsonSerializer.Deserialize, so Utf8JsonReader runs with isFinalBlock: false. ReferenceResolvingConverter called Utf8JsonReader.Skip() in 5 places — which throws Cannot skip tokens on partial JSON on non-final buffers. The hot path was ReadProperty's else branch: an adder-only collection property (e.g. Account.RawAttributes, empty for bots) whose SQL array_agg(...) yields JSON null instead of []. One such account aborted the entire purge loop.
What changed

• src/Persistence/Json/ReferenceResolvingConverter.cs — eliminated all Skip() calls. null values are now a no-op (collection stays empty); anything else is discarded via JsonSerializer.Deserialize, which is safe for chunked streams. New SkipValue / SkipUnknownPropertyValue helpers with documenting comments.
• src/Persistence/EntityFramework/Json/JsonQueryBuilder.cs — collection subqueries now use coalesce(array_to_json(array_agg(...)), '[]'::json), so empty collections serialize as [] instead of null (one-to-many and many-to-many).
• src/GameLogic/Bots/BotGenerator.cs — DeleteAllBotsAsync wraps each account in try/catch and logs + skips on failure, so one unreadable account can no longer abort the whole purge.

[Error] [BotFeaturePlugIn] Failed to purge the bot population.
System.Text.Json.JsonException: The JSON value could not be converted to MUnique.OpenMU.Persistence.EntityFramework.Model.Account. Path: $ | LineNumber: 0 | BytePositionInLine: 494.
 ---> System.InvalidOperationException: Cannot skip tokens on partial JSON. Either get the whole payload and create a Utf8JsonReader instance where isFinalBlock is true or call TrySkip.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_CannotSkipOnPartial()
   at System.Text.Json.Utf8JsonReader.Skip()
   at MUnique.OpenMU.Persistence.Json.ReferenceResolvingConverter`1.ReadProperty(Utf8JsonReader& reader, JsonSerializerOptions options, T item, ValueTuple`3 handler) in /home/eduardo/Work/C/OpenMU/src/Persistence/Json/ReferenceResolvingConverter.cs:line 215
   at MUnique.OpenMU.Persistence.Json.ReferenceResolvingConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options) in /home/eduardo/Work/C/OpenMU/src/Persistence/Json/ReferenceResolvingConverter.cs:line 175
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, T& value, JsonSerializerOptions options, ReadStack& state)
   --- End of inner exception stack trace ---

[sven-n]

Review

Nice catch on the root cause — the diagnosis is exactly right, and the stack trace even proves the sharpest part of it: the throw happened on a null token, which has no children, so Skip() would have been a pure no-op. Utf8JsonReader.Skip() checks IsFinalBlock unconditionally before it looks at the token, which is why it blows up even when the data it would need is already there. CI is green, both call sites of the query builder are covered. A few things I'd like to see changed before this goes in.

1. TrySkip() is the one-line fix — I'd prefer it over the rewrite

The whole SkipValue / SkipUnknownPropertyValue machinery can be replaced by the API .NET provides for exactly this situation:

if (!reader.TrySkip())
{
    throw new JsonException("Incomplete JSON: could not skip the value.");
}

Why this is not just shorter but strictly safer:

• It can't fail where the current fix succeeds. JsonSerializer.Deserialize<JsonElement>(ref reader, options) scopes the reader to the next complete value — internally by calling TrySkip() and throwing NotEnoughData if it returns false. So if Deserialize<JsonElement> works here (and it does, because the serializer's read-ahead for custom converters buffers the whole Account value before Read() is ever called), TrySkip() returns true on exactly the same inputs.
• It preserves the reader position semantics 1:1. The rewrite changes them in two places:
  • ReadWrappedCollection, first branch (was line 225): reader.Skip() on a non-container token is a no-op, so an EndObject/EndArray landing there was harmless. SkipValue → Deserialize<JsonElement> on an end token throws instead. Unreachable with well-formed JSON today, but it turns a benign guard into a trap.
  • Read(), unknown-property branch (was line 179): SkipUnknownPropertyValue assumes the reader is still sitting on the property name. In the fallthrough where propertyName is "$ref" or "$id" but Deserialize<string> returned null, the value has already been consumed — the old Skip() was a no-op there, the new Read() + skip swallows the next property. Not reachable from the Postgres JSON (json_build_object('$ref', …) is guarded by a case when … is null, and $id is the PK), but it's a foot-gun for the backup/restore path.
• No allocation. Deserialize<JsonElement> materializes a JsonDocument (value bytes + metadata) for every skipped value. This converter is also what loads the whole GameConfiguration and what BackupService restore goes through, so unknown/ignored properties on that path now each cost an allocation for something we're throwing away.

Same substitution works for all five sites, ReadProperty's else included — TrySkip() on a Null or scalar token is a no-op returning true, so the explicit JsonTokenType.Null special case isn't needed either.

2. JsonQueryBuilder — keep it

coalesce(array_to_json(array_agg(…)), '[]'::json) is the right call independently of the converter fix: an empty collection is [], and null was only ever an artifact of array_agg over zero rows. Worth noting the two fixes are each individually sufficient for the reported crash, which is fine — the converter fix is still needed for the file/backup path where the query builder isn't involved. I checked that the emitted [] can't hit the setter path and replace a pre-initialized collection instance: all Raw… collections are get-only (ICollection<T> RawX { get; } = new EntityFramework.List<T>()), so they always go through the adder. No behaviour change there.

3. BotGenerator — the catch is too wide and hides the failure from the caller

• It swallows cancellation. SaveChangesAsync(cancellationToken) inside the try throws OperationCanceledException, which is then logged as an error and the loop continues to the next account. Please add when (ex is not OperationCanceledException).
• It reports success to a caller that is counting on the exception. BotFeaturePlugIn (line ~234) catches the exception to keep PurgeBots set and back off for a retry. With the inner catch, a purge in which every single account failed now returns normally, so Enabled/PurgeBots/ResetBots are cleared and the feature is switched off with the bot accounts still in the database — and there's no retry. I'd count the failures and either rethrow at the end or leave the flag set:

var failed = 0;
// … catch { failed++; … }
if (failed > 0)
{
    throw new InvalidOperationException($"{failed} of {loginNames.Count} bot account(s) could not be deleted.");
}

• deleted++ happens before SaveChangesAsync, so an account whose save throws is still counted as deleted. Move the increment after the successful save.
• A failed SaveChangesAsync leaves the shared context dirty — the entities stay in the change tracker as Deleted, so the next account's save retries them and fails too, turning one bad account into a cascade of errors. Either use a fresh context per account or reset the change tracker in the catch.

4. Please add a regression test

This is cheap and needs no database: src/Persistence is already referenced by tests/MUnique.OpenMU.Tests, and ObjectJsonExtensions.FromJson<T> is the entry point. Wrap a JSON payload in a Stream whose Read hands out a few bytes at a time (or just make the payload larger than the serializer's 16 KB buffer) so isFinalBlock is false, include a null adder-only collection property and an unknown property, and assert it deserializes. Without that, the next reader.Skip() someone adds here reintroduces this silently — it only shows up against a real Postgres with a large enough row.

---

On the actual question: should the database do this with ON DELETE CASCADE?

Short answer: yes, that's the right end state, but it can't be reached by flipping a delete behaviour — and it's a separate change from this PR.

Why it isn't just a setting. For collection members the generated cascade already does the job, because the FK lives on the child (Character."AccountId" → Account, Item."ItemStorageId" → ItemStorage), and Postgres cascades principal → dependent. For the one-to-one aggregate members the FK lives on the parent:

Character."InventoryId" → ItemStorage."Id"   (ON DELETE CASCADE)
Account."VaultId"       → ItemStorage."Id"   (ON DELETE CASCADE)

So the cascade we generate today means "deleting the ItemStorage deletes the Character" — the opposite of what's wanted. Deleting the character leaves the storage, and every item in it, unreachable. EfCoreModelGenerator.cs (~line 317) emits HasOne(entity => entity.Raw…).WithOne().OnDelete(Cascade) for every [MemberOfAggregate] reference, so this affects 28 relationships, not just the two here — LetterBody.SenderAppearance, BattleZoneDefinition.Ground/LeftGoal/RightGoal, CharacterClass.ComboDefinition, MonsterDefinition.MerchantStore, the castle siege respawn areas, and so on.

Worth being explicit about the consequence: this leak is not bot-specific. DeleteCharacterAction.cs:67 deletes a character with a plain DeleteAsync(character) — every normal in-game character deletion orphans an ItemStorage and its items today. The workaround in DeleteAllBotsAsync fixes the bot purge only.

The options, roughly in order of how much I like them:

1. Invert the one-to-one aggregate FKs so the child carries the owner id (ItemStorage."CharacterId" / "AccountId", with ON DELETE CASCADE), and let the generator emit HasOne(…).WithOne().HasForeignKey<Child>(…). Correct and permanent. Cost: ItemStorage serves three different owners (Character.Inventory, Account.Vault, MonsterDefinition.MerchantStore), so it needs three nullable owner columns (or a split of the table); plus a data migration backfilling from the current columns, and a look at JsonQueryBuilder.AddNavigation, which builds the aggregate JSON off the current navigation direction. Not a small change, but it's the one that makes every delete path correct at once — and it would let DeleteAllBotsAsync drop both the manual storage deletes and the full-graph reload per account, which is the expensive part of the purge.
2. An AFTER DELETE trigger on Character/Account removing the referenced storage. Cheaper migration, but hand-written SQL in migrations, invisible to EF, and EF 7+ wants HasTrigger annotations so its rows-affected optimization doesn't misfire. I'd rather not.
3. A periodic orphan sweep (delete from "ItemStorage" s where not exists (…)). Needed regardless of which option we pick, because existing databases already contain orphaned storages and items that no future cascade will ever reach. Good as a one-time cleanup migration plus a maintenance task.
4. Keep it in the application layer, but move it out of BotGenerator — into the persistence layer's delete for Character/Account — so the in-game path is covered too. This is the sensible interim step if (1) is too big right now.

My suggestion for this PR: leave the storage handling as it is (it's pre-existing, and the PR only re-indents it), fix the four points above, and open a separate issue for the one-to-one aggregate cascade direction, since it's a generator-level bug with a much wider blast radius than the bot purge.

---

Generated by Claude Code

[sven-n]

Follow-up on the cascade question from my review above: I wrote it up as #933 and implemented the trigger route in #934.

Short version of what I got wrong in that first comment: the HasTrigger objection I raised is a SQL Server limitation (EF's OUTPUT INTO doesn't work on tables with triggers). Npgsql uses RETURNING and has no such requirement, and every database here is created through MigrateAsync, never EnsureCreated — so a trigger added in a migration is versioned with the schema and guaranteed to exist. The trigger is also the only option that covers a shallow delete: PostgreSQL fires row triggers for rows deleted by a foreign key cascade, so deleting the account row alone now cleans up its characters and each of their inventories, with nothing loaded.

Relevant to this PR: #934 touches DeleteAllBotsAsync too. It removes the manual inventory/vault deletes, because with the trigger in place EF's own DELETE for the storage would find the row already gone and fail the save with a DbUpdateConcurrencyException. So the two PRs conflict in that method — whichever lands first, the other should be rebased on it. Nothing here needs to change on account of that; the four review points above still stand on their own.

---

Generated by Claude Code