Delete item rows when items leave the game#835
Conversation
Removing an item from a storage only detaches it from the storage's item collection, which leaves the row behind with a null ItemStorageId instead of deleting it. Two paths did that without a following delete: selling to an NPC, and packing or unpacking a stack of jewels. Both are frequent actions, so the rows pile up: on a server running for a few months with nine player accounts, 84% of all rows in the item table were such leftovers, and adding bots raised the rate to roughly 1600 rows per day. The rows carry no owner and no timestamp, so they are of no use as a history either. Delete the item after removing it, the way DestroyInventoryItemAsync already does for consumed items, crafting ingredients and dropped items.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue involving unbounded database growth caused by items that were detached from storage but never deleted. By ensuring that items are properly removed from the persistence context in all relevant code paths, the system will now correctly clean up item records when they are sold or stacked, preventing the accumulation of unnecessary data. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that items are properly deleted from the player's persistence context when they are stacked, unstacked, or sold to an NPC. Specifically, calls to player.PersistenceContext.DeleteAsync have been added after removing items from the inventory in ItemStackAction.cs and SellItemToNpcAction.cs. There are no review comments, so no additional feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
2 participants
Two code paths remove an item from a storage without deleting it from the
database, so the row stays behind forever with a null
ItemStorageId.The problem
Storage.RemoveItemAsynconly detaches the item from the storage's collection:Since the foreign key is optional, EF nulls
ItemStorageIdinstead of deletingthe row. Most callers follow up with a delete —
DestroyInventoryItemAsync(consumed potions and jewels, mini-game tickets),
BaseItemCraftingHandler(crafting ingredients) and
DropItemActionall do. Two do not:SellItemToNpcAction.SellItemAsync— every item sold to a merchantItemStackAction— packing ten jewels into a stack leaves ten rows behind,unpacking leaves one
Impact
Measured on a live server that had been running for a few months with nine
player accounts: 8,511 of 10,179 rows (84%) in
data."Item"were orphans.After adding bots, which sell to merchants constantly, the count grew by
roughly 1,600 rows per day.
The rows are not usable as history —
Itemhas no timestamp and no ownerfield, and
ItemStorageIdwas the only link to who held it. Nothing in thecode ever reads items with a null storage.
This is not an urgent performance problem at that scale, but it is unbounded
growth of rows that serve no purpose.
The fix
Delete the item right after removing it, matching the order already used by
DestroyInventoryItemAsync(remove → delete → notify the client):Three lines in two files. No change to the view plugin calls, so nothing the
client sees is affected.
Notes for review
IItemSoldToMerchantPlugInis invoked after the removal and receives theitem. It has no implementations in the repository, and the deletion is only
staged in the persistence context, so the object stays usable for the call.
If a plugin ever wants to keep the sold item — a buy-back feature, say —
that plugin point would need rethinking regardless of this change.
MoveItemAction, player shoppurchases in
BuyRequestAction, returning items from temporary storage)are deliberately untouched: there the row must survive, only its owner
changes.
Testing
MUnique.OpenMU.Tests: 570 passed, 18 failed.masterat the same commit(
26fc908ba), so they are pre-existing and unrelated to this change. Theyare the
Ancient,AncientWithoutBonus,Definition,DurabilityandLevelgroup around item definitions.exactly what these two paths produce — jewels (Creation 323, Bless 261,
Chaos 248, Life 204, Soul 175) and ordinary drops sold to merchants.