Delete the members of a member of an aggregate, too

[sven-n]

Fixes #933.

EntityFrameworkContextBase.DeleteAsync removes the members of an aggregate through ForEachAggregate, which was not recursive. So deleting an account removed its characters, but not the characters' inventories — and an inventory is referenced BY its character, so its foreign key sits at the character and no delete cascade of the database ever reaches it. The storage row, and every item lying in it, stayed behind as unreachable rows. BotGenerator.DeleteAllBotsAsync worked around exactly this by deleting the storages itself.

ForEachAggregate now walks the whole aggregate instead of one level.

What's in here

• EntityFrameworkContextBase.ForEachAggregate recurses into the members it hands to the action. Members are tracked by reference while it walks: the entities compare equal by their id, so a HashSet<object> on the default comparer would confuse two members sharing the default id, and a graph which references itself would recurse forever. DetachInternal no longer recurses itself — it passed an action which called back into ForEachAggregate — so the graph is walked once.
• The recursion stops at a shared configuration object. [MemberOfAggregate] is not a clean "owned exclusively" list: Buff.MagicEffectDefinition marks a row which lives in GameConfiguration.MagicEffects and is referenced by Skill.MagicEffectDef and ItemDefinition.ConsumeEffect too. Walking one level deeper would have taken it away from a skill and an item when a monster definition is deleted. So when deleting, the traversal stops when it reaches an object which a collection of the GameConfiguration holds — those are roots of their own — through the aggregate of a member. Direct members are untouched by this rule, so nothing which was deleted before this PR is kept now: only the new recursion is constrained. Detaching still walks everything, because a stale configuration has to be detached whole.
• A migration removes the rows which are already orphaned, which neither a cascade nor the traversal can ever reach: item storages (inventories and vaults) and appearance data.
• BotGenerator no longer deletes the storages by hand; the recursion covers them. It keeps the full-graph reload, since ForEachAggregate can only walk what is loaded.

What changed since the first version

The first version added AFTER DELETE triggers. The review on this PR showed that they conflict with ForEachAggregate, which already removes single-object [MemberOfAggregate] members: the trigger removed the row first, then EF's own DELETE hit 0 rows and threw a DbUpdateConcurrencyException — breaking the in-game character deletion, the bot purge and admin-panel deletes of config objects. The cross-schema triggers would also have been denied, since the config role holds only GRANT SELECT on data."ItemStorage" and data."Item". All of that is reverted; the source generator is untouched again.

That review also corrected the premise I opened this with: DeleteCharacterAction did not leak the inventory, because ForEachAggregate removed it. Only the account → characters → inventory path leaked. #933 is updated accordingly.

Limitations

• The cleanup covers the account-data side only. Because the direct members were already removed, a one-to-one member only leaked at depth 2 or deeper: item storages via account → characters, and appearance data as a safety sweep (LetterBody is the only table referencing it). Config-side depth-2 orphans are left — a map's BattleZone → Ground/LeftGoal/RightGoal rectangles, a monster's ItemCraftings → SimpleCraftingSettings, and so on. Enumerating the referencing columns for those by hand and getting one wrong would delete live configuration, which is worse than a few unreachable rows. Easy to add as a follow-up with a second pair of eyes on the list.
• The annotations still deserve a review, independently of this PR: Buff.MagicEffectDefinition and Skill.MasterDefinition mark shared rows. The guard above keeps the recursion away from them, but dropping the annotations is the real fix — it changes the model (the generated cascade goes away and JsonQueryBuilder would emit a $ref instead of inlining the object), so it wants its own migration. Noted in Deleting an account orphans the item storages of its characters #933.

Testing

Not run here: this environment has no .NET SDK and no PostgreSQL, so nothing was built or executed locally, and there is no new automated test — an integration test for this needs a database, so it could only be added in the [Ignore]d style of BackupServiceEfCoreTests and would give CI no signal. CI is green on the branch (Azure, GitHub Actions and Codacy), which covers compilation and the existing suite.

What would actually settle it, against a real database: delete a character, and an account with a vault and characters, and confirm data."ItemStorage" is empty afterwards and no DbUpdateConcurrencyException is thrown. Then delete a MonsterDefinition which has buffs and confirm its MagicEffectDefinition rows survive. Happy to write that up as an [Ignore]d test if you want it in the repo.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Q9QAbEmp9rhnSWqZvkVaeN

[cloudflare-workers-and-pages]

Deploying openmudocs with    Cloudflare Pages

Latest commit:	80ba05c
Status:	✅  Deploy successful!
Preview URL:	https://f8737952.openmudocs.pages.dev
Branch Preview URL:	https://claude-pr927-cascade-delete.openmudocs.pages.dev

View logs

[sven-n]

Thanks for tackling the orphaned item storages. I think the triggers conflict with the existing aggregate-delete mechanism, though, and would break ordinary deletes. Details are in the inline comments; here is the root cause.

EntityFrameworkContextBase.DeleteAsync → ForEachAggregate already calls Context.Remove on single-object [MemberOfAggregate] members (Inventory, Vault, MerchantStore, …). With the new AFTER DELETE triggers, the member row is gone by the time EF issues its own DELETE for it. That DELETE hits 0 rows and throws DbUpdateConcurrencyException. Affected paths: deleting a character in-game, the bot purge, and admin-panel deletes of config objects with one-to-one members.

The actual leak was narrower: ForEachAggregate isn't recursive, so deleting an account removes its characters but not the characters' inventories. Making ForEachAggregate recurse into the members it removes would fix that in one place, without triggers. If the triggers stay, ForEachAggregate has to stop removing non-collection members, or every DeleteAsync caller breaks.

This analysis comes from reading the code and EF's delete ordering. I haven't run it against Postgres. An integration test that deletes a character and an account with a vault would confirm it either way.

---

Generated by Claude Code

[sven-n]

Closed the two points I'd left hanging on a question rather than waiting for an answer.

The shared configuration objects are now guarded (8720603d). I'd flagged that the recursion reaches Buff.MagicEffectDefinition — a row that lives in GameConfiguration.MagicEffects and is used by a skill and an item definition too — and asked whether to guard it here. It is a regression this PR would have introduced, so it shouldn't wait: when deleting, the traversal now stops when it reaches an object which a collection of the GameConfiguration holds, if it got there through the aggregate of a member. Deleting a monster definition removes its buffs, and leaves their effect definitions alone.

The "through a member" part matters: direct members are treated exactly as before, so nothing that was deleted before this PR is kept now. Deleting a Buff on its own still removes its effect definition, as it always did — that's the annotation being wrong, which is a separate fix (it changes the model and needs its own migration). Detaching still walks everything, since a stale configuration has to be detached whole.

The config-side orphan cleanup stays out, deliberately rather than pending a decision. The migration sweeps data."ItemStorage" and data."AppearanceData", where I could verify the complete set of referencing columns from the model snapshot. Doing the same for the config tables means enumerating referencing columns by hand across a dozen types, and one mistake there deletes live configuration — a worse outcome than leaving a few unreachable rows. It's a good follow-up with someone checking the list.

CI was green on the previous head; this push re-runs it.

---

Generated by Claude Code