fix: release Lua-pushed KV userdata to stop ScopedKV memory leak

[matzinhozz]

Summary

The KV Lua metatable is registered with registerClass, which does not install a __gc metamethod. But KV objects are pushed to Lua as owning std::shared_ptr<KV>, so without __gc the shared_ptr is never released when Lua collects the userdata. Every kv.scoped(), scopedKV:scoped() and player:kv() call therefore leaks a ScopedKV (plus its control block and prefix string). KV is used on many hot Lua paths, so memory grows on every login/relog and during normal play.

The fix is a one-liner: register the metatable with registerSharedClass instead of registerClass, so it installs the already-existing __gc handler used by every other shared_ptr-backed Lua class.

Root cause

src/lua/functions/core/libs/kv_functions.cpp registers the metatable without GC:

Lua::registerClass(L, "KV", "");

KV values are pushed to Lua through the owning shared_ptr overload of pushUserdata (src/lua/functions/lua_functions_loader.hpp), which placement-news a std::shared_ptr<KV> into the Lua userdata and bumps the refcount:

template <class T>
static void pushUserdata(lua_State* L, std::shared_ptr<T> value) {
    auto userData = static_cast<std::shared_ptr<T>*>(lua_newuserdata(L, sizeof(std::shared_ptr<T>)));
    new (userData) std::shared_ptr<T>(value); // copy ctor -> bumps refcount
}

That refcount is only dropped by the __gc metamethod (src/lua/functions/lua_functions_loader.cpp):

int Lua::luaGarbageCollection(lua_State* L) {
    const auto objPtr = static_cast<std::shared_ptr<SharedObject>*>(lua_touserdata(L, 1));
    if (objPtr) {
        objPtr->reset();
    }
    return 0;
}

and __gc is only installed by registerSharedClass:

void Lua::registerSharedClass(lua_State* L, const std::string &className, const std::string &baseClass, lua_CFunction newFunction) {
    registerClass(L, className, baseClass, newFunction);
    registerMetaMethod(L, className, "__gc", luaGarbageCollection);
}

Because KV used registerClass, the placement-new'd std::shared_ptr<KV> destructor never runs when Lua collects the userdata. The strong reference leaks, so each pushed ScopedKV — allocated by KVStore::scoped() / ScopedKV::scoped() and by Player::kv() (g_kv().scoped("player")->scoped(<guid>)) — is never freed.

This is the only class in this situation: other plain registerClass types (ItemType, ItemClassification, Imbuement, Variant) are pushed as non-owning raw pointers (or, for Variant, a plain Lua table) and read back with getUserdata<T> (T**), so they take no ownership and need no __gc. KV is the only registerClass type that pushes an owning shared_ptr.

Impact

The KV API is pushed to Lua on hot paths — player:kv() in data/scripts/creaturescripts/player/login.lua, per-tick player/creature events (data/events/scripts/player.lua, creature.lua), combat protection, NPC modules, hireling, features, achievements, daily reward and talkactions. Each call leaks a small heap allocation that is never reclaimed, so process RSS grows continuously — most visibly when players relog repeatedly. Systems that persist a lot of per-player state through KV (forge, wheel, proficiency, hireling, …) make the growth most apparent under profiling, but the leak lives in the KV Lua binding, not in those systems.

Fix

- Lua::registerClass(L, "KV", "");
+ Lua::registerSharedClass(L, "KV", "");

This installs __gc = luaGarbageCollection on the KV metatable, matching Player, Item, Container, Creature, Monster, Npc, Guild, etc.

Why it is safe

• Only freshly-created ScopedKV instances are ever pushed as KV userdata (returned by scoped() and Player::kv()). The root KVStore singleton is reached through the global kv table (kv.get, kv.set, …) and is never wrapped in a pushed shared_ptr, so reset() only drops the Lua-held reference and cannot double-free or destroy the singleton.
• luaGarbageCollection reinterprets the userdata as std::shared_ptr<SharedObject>* and calls reset(). This is the exact mechanism already used by every shared class: std::shared_ptr<T> layout is independent of T, and the control block's type-erased deleter destroys the real ScopedKV.
• No behavioral change to get / set / scoped / keys / remove; __gc only releases the C++ reference at collection time.

Testing

• Functional: kv.scoped(...), scopedKV:get/set/keys/remove and player:kv() keep working unchanged.
• Leak: a loop that creates many scopes followed by collectgarbage("collect") now keeps RSS stable instead of growing without bound; under ASan/valgrind the leaked ScopedKV / control-block allocations are gone.