fix(shop): keep existing flows correct for NPC-backed shops

Sweeps the flows that still assumed every shop owns a villager.

ShopRegistry#put only cleared a shop's old villager mapping when the new id was
non-null — which is exactly the case it needed to handle, since switching a shop
to an NPC sets the id to null. The stale entry stayed behind claiming a villager
that no longer belonged to that shop.

A crash between despawning a villager and persisting that leaves a shop marked
NPC while still holding a villager id, so both would render. Two passes clean
that up, and they need to be two: spawn chunks are already loaded before plugins
enable, so the chunk-load pass alone never sees them. The boot pass only acts on
a reachable entity — clearing the id for an unloaded chunk would orphan the
villager with nothing left pointing at it, so that case is deliberately left to
the chunk-load pass.

The profession button is hidden for NPC-backed shops; professions are a villager
concept and the setting stays stored for if the shop switches back.

/vshop reload now re-sends NPCs. A villager re-derives its attributes on the next
chunk load, but a packet NPC bakes config in at spawn, so turn-to-player, the
interaction cooldown and the name format would have stayed stale until restart.

Verified on the test server: force-loading an NPC-backed shop's chunk removes the
stray villager and clears the id.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: f0reachARR

src/main/java/me/f0reach/vshop/ModernVillagerShopPlugin.java

@@ -175,6 +175,18 @@ public void onEnable() {
 
         this.interactionRouter = new ShopInteractionRouter(openService, actionMenu, soundService);
 
+        // Spawn chunks are already loaded by the time plugins enable, so the
+        // chunk-load pass never sees them: reconcile those shops here.
+        for (var shop : registry.all()) {
+            if (!shopEntities.discardStrayVillager(shop)) continue;
+            try {
+                shopService.update(shop);
+            } catch (SQLException ex) {
+                getLogger().warning("Failed to clear the stale villager id on shop "
+                        + shop.id() + ": " + ex.getMessage());
+            }
+        }
+
         if (npcIntegration != null) {
             npcIntegration.start(interactionRouter);
         } else {

src/main/java/me/f0reach/vshop/command/sub/ReloadCommand.java

@@ -6,6 +6,7 @@
 import io.papermc.paper.command.brigadier.CommandSourceStack;
 import io.papermc.paper.command.brigadier.Commands;
 import me.f0reach.vshop.command.CommandSupport;
+import me.f0reach.vshop.model.Shop;
 
 @SuppressWarnings("UnstableApiUsage")
 public final class ReloadCommand {
@@ -24,6 +25,15 @@ public LiteralArgumentBuilder<CommandSourceStack> node() {
 
     private int execute(CommandContext<CommandSourceStack> ctx) {
         support.plugin().reloadConfigInternal();
+        // Villager attributes are re-derived on the next chunk load anyway, but a
+        // packet NPC only changes when we re-send it, so config values it baked in
+        // (turn-to-player, interaction cooldown, the name format) would otherwise
+        // stay stale until the next restart.
+        for (Shop shop : support.plugin().registry().all()) {
+            if (support.plugin().shopEntities().isNpcBacked(shop)) {
+                support.plugin().shopEntities().refresh(shop);
+            }
+        }
         ctx.getSource().getSender().sendMessage(support.messages().get("command.reload.done"));
         return Command.SINGLE_SUCCESS;
     }

src/main/java/me/f0reach/vshop/shop/ShopRegistry.java

@@ -26,13 +26,13 @@ public void loadAll(Collection<Shop> shops) {
 
     public void put(Shop shop) {
         byId.put(shop.id(), shop);
+        // Drop any previous villager mapping for this shop first. It has to happen
+        // even when the new id is null — that is exactly what a shop switching to
+        // an NPC backend looks like, and a leftover mapping would keep claiming a
+        // villager that is no longer this shop's.
+        villagerToShop.entrySet().removeIf(e -> e.getValue().equals(shop.id()));
         UUID v = shop.villagerEntityId();
-        if (v != null) {
-            // Remove any old villager mapping that may have been associated with
-            // this shop, then index the current one.
-            villagerToShop.entrySet().removeIf(e -> e.getValue().equals(shop.id()));
-            villagerToShop.put(v, shop.id());
-        }
+        if (v != null) villagerToShop.put(v, shop.id());
     }
 
     public void remove(UUID shopId) {

src/main/java/me/f0reach/vshop/shop/edit/ShopActionMenu.java

@@ -142,7 +142,9 @@ private void openSettingsSubmenu(Player viewer, Shop shop) {
                             Placeholder.parsed("current", shop.name())),
                     () -> openRename(viewer, shop)));
         }
-        if (hasAnyPerm(viewer, "modernvillagershop.edit.profession",
+        // Profession only exists on a villager; an NPC-backed shop has no such knob.
+        if (!plugin.shopEntities().isNpcBacked(shop)
+                && hasAnyPerm(viewer, "modernvillagershop.edit.profession",
                 "modernvillagershop.edit.others", "modernvillagershop.admin.edit")) {
             buttons.add(new DialogService.ButtonSpec(
                     messages.get("action.profession.button",

src/main/java/me/f0reach/vshop/shop/entity/ShopEntityService.java

@@ -97,6 +97,28 @@ public void onShopDeleted(Shop shop) {
         appearances.remove(shop.id());
     }
 
+    /**
+     * Removes a villager still standing for a shop that now renders as an NPC and
+     * clears the stale id, returning true when the shop record changed.
+     *
+     * <p>Only acts when the entity is actually reachable. If its chunk is not
+     * loaded the id has to stay put — clearing it would orphan the villager with
+     * nothing left pointing at it. {@code ShopVillagerListener} finishes the job
+     * when the chunk does load.
+     *
+     * <p>Exists because a crash between despawning a villager and persisting that
+     * leaves the two out of step, and spawn chunks are already loaded before
+     * plugins enable, so the chunk-load pass alone never sees them.
+     */
+    public boolean discardStrayVillager(Shop shop) {
+        if (!isNpcBacked(shop) || shop.villagerEntityId() == null) return false;
+        var stray = villagers.findEntity(shop);
+        if (stray == null) return false;
+        stray.remove();
+        shop.setVillagerEntityId(null);
+        return true;
+    }
+
     /**
      * Rebuilds the representation from the current appearance, switching backend
      * if it changed. Returns the Bukkit entity id to persist on the shop — null

src/main/java/me/f0reach/vshop/shop/listener/ShopVillagerListener.java

@@ -106,26 +106,47 @@ public void onChunkLoad(ChunkLoadEvent event) {
             int sz = (int) Math.floor(shop.location().z()) >> 4;
             if (sx != cx || sz != cz) continue;
 
-            // NPC-backed shops are packet-based and have no entity id: they are
-            // spawned once at boot and are unaffected by chunk loading.
-            if (shops.entities().isNpcBacked(shop)) continue;
-
             UUID villagerId = shop.villagerEntityId();
+
+            // NPC-backed shops are packet-based: spawned once at boot, unaffected
+            // by chunk loading. They should own no villager at all, so a leftover
+            // id means a crash landed between despawning one and persisting that
+            // — clean it up now that the chunk is finally loaded.
+            if (shops.entities().isNpcBacked(shop)) {
+                if (villagerId != null) discardStrayVillager(event, shop, villagerId);
+                continue;
+            }
+
             if (villagerId == null) continue;
-            Entity entity = event.getWorld().getEntities().stream()
-                    .filter(e -> e.getUniqueId().equals(villagerId))
-                    .findFirst().orElse(null);
+            Entity entity = findInWorld(event, villagerId);
             if (entity == null) {
                 var at = shop.location().toBukkit();
                 if (at == null) continue;
                 UUID newId = shops.entities().spawn(shop, at);
                 shop.setVillagerEntityId(newId);
-                try {
-                    shops.update(shop);
-                } catch (java.sql.SQLException ex) {
-                    // Already logged in the service layer; we don't need to abort the chunk-load.
-                }
+                persist(shop);
             }
         }
     }
+
+    private void discardStrayVillager(ChunkLoadEvent event, Shop shop, UUID villagerId) {
+        Entity stray = findInWorld(event, villagerId);
+        if (stray != null) stray.remove();
+        shop.setVillagerEntityId(null);
+        persist(shop);
+    }
+
+    private static Entity findInWorld(ChunkLoadEvent event, UUID entityId) {
+        return event.getWorld().getEntities().stream()
+                .filter(e -> e.getUniqueId().equals(entityId))
+                .findFirst().orElse(null);
+    }
+
+    private void persist(Shop shop) {
+        try {
+            shops.update(shop);
+        } catch (java.sql.SQLException ex) {
+            // Already logged in the service layer; we don't need to abort the chunk-load.
+        }
+    }
 }