#110 Refresh signs on chunk load

plans/chunk-load-sign-reconciliation-plan.md

@@ -0,0 +1,142 @@
+# Chunk-load sign reconciliation
+
+## Context
+
+Addresses GitHub issue #110, the follow-up `plans/region-sharded-sign-persistence-plan.md` and
+`plans/sign-storage-refactor-options.md` both flagged but deferred: nothing today detects a sign that vanished while
+its chunk was unloaded (external region-file deletion/regen, backup restore, manual NBT surgery). The removal path
+that exists (`AbstractBlockInject` mixin on `BlockBehaviour.affectNeighborsAfterRemoval`) only fires for an in-game
+block change on a loaded chunk — it can't see a sign that disappeared by any other means. `SignManager`'s cache
+keeps that entry and its BlueMap marker forever.
+
+`region-sharded-sign-persistence-plan.md` set up on-disk storage sharded by 512-block region specifically so this
+feature would have something cheap to query, but explicitly left `SignManager`'s in-memory `ConcurrentMap<SignEntryKey,
+SignEntry>` untouched, deferring "a region-indexed in-memory view" to whichever feature actually consumes it.
+`sign-storage-refactor-options.md` rejected sharding that map speculatively for the same reason — no caller yet. This
+plan is that caller.
+
+Fabric API already exposes the event needed, no new dependency: `ServerChunkEvents.CHUNK_LOAD`
+(`fabric-lifecycle-events-v1`, part of `fabric_api_version` already in `gradle.properties`) —
+`Load.onChunkLoad(ServerLevel level, LevelChunk chunk, boolean generated)`, fired once a chunk is already loaded into
+a `ServerLevel`.
+
+**Performance constraint**: chunk load fires constantly — spawn-chunk keep-alive, players walking, world
+pre-generation. The design below must not scan the full sign cache per event.
+
+## Goal
+
+On every chunk load, cheaply find which cached signs belong to that chunk, check each still has a live
+`SignBlockEntity` at its position, and remove (via the existing `SignManager.remove`, unchanged) any that don't.
+
+## Design
+
+### `SignChunkKey` (new, plain Java, unit-testable)
+
+`record SignChunkKey(String parentMap, int chunkX, int chunkZ)`, in `core.signs` (a runtime lookup concern, not
+persistence — kept separate from `core.signs.persistence.SignRegionKey`, which is 512-block/32-chunk file-layout
+math and stays untouched). `forEntryKey(SignEntryKey key)` computes `Math.floorDiv(x, 16)`/`Math.floorDiv(z, 16)` —
+plain per-chunk coordinates, `floorDiv` for the same negative-coordinate reason `SignRegionKey` already documents.
+
+### `SignChunkIndex` (new, plain Java, unit-testable)
+
+Wraps `ConcurrentHashMap<SignChunkKey, Set<SignEntryKey>>` (values are `ConcurrentHashMap.newKeySet()`), in
+`core.signs`:
+- `add(SignEntryKey key)` — `computeIfAbsent(SignChunkKey.forEntryKey(key), ...).add(key)`.
+- `remove(SignEntryKey key)` — drops `key` from its chunk's set; if the set is now empty, removes the chunk entry
+  too, so emptied-out areas don't leak map entries over a long-running server.
+- `keysInChunk(String parentMap, int chunkX, int chunkZ)` — returns a snapshot `List<SignEntryKey>` (empty list if
+  none tracked), the query the chunk-load handler uses.
+- `clear()`.
+
+Fully independent of `SignManager`/BlueMap types — testable the same way as `SignRegionKeyTest`/
+`SignRegionPartitionerTest`.
+
+### `SignManager` changes
+
+- New field: `private final SignChunkIndex chunkIndex = new SignChunkIndex();`.
+- Every place `signCache.put(key, entry)` establishes a *new* key (the add branch in `addOrUpdateSign`) also calls
+  `chunkIndex.add(key)`. `removeByKey` also calls `chunkIndex.remove(key)`. The update branch (existing entry, same
+  key, text/prefix change) doesn't touch `chunkIndex` — position is immutable once a `SignEntry` exists.
+- `reloadSigns()` calls `chunkIndex.clear()` alongside `signCache.clear()` (the replay loop repopulates both via the
+  same add path).
+- New method: `static List<SignEntryKey> getKeysInChunk(String parentMap, int chunkX, int chunkZ)` → delegates to
+  `chunkIndex.keysInChunk(...)`. Pure data query, no BlueMap/game types — but `SignManager` itself stays outside unit
+  test coverage regardless (its constructor still builds a `BlueMapAPIConnector`), consistent with its current status
+  in `testing.md`.
+
+### Mod-level chunk-load handler (game-coupled, `BlueMapSignMarkersMod`)
+
+Register alongside the existing three hooks: `ServerChunkEvents.CHUNK_LOAD.register(this::onChunkLoad);`.
+
+```java
+private void onChunkLoad(ServerLevel level, LevelChunk chunk, boolean generated) {
+    var parentMap = SignHelper.getSignParentMap(level);
+    var chunkPos = chunk.getPos();
+
+    for (var key : SignManager.getKeysInChunk(parentMap, chunkPos.x(), chunkPos.z())) {
+        if (!(chunk.getBlockEntity(new BlockPos(key.x(), key.y(), key.z())) instanceof SignBlockEntity)) {
+            LOGGER.info("Removing stale sign marker at {} - no sign block found on chunk load "
+                    + "(external deletion/regen?)", key);
+            SignManager.remove(key);
+        }
+    }
+}
+```
+
+`chunk.getBlockEntity(pos)` is used directly (not `level.getBlockEntity(pos)`) since the event already hands us the
+loaded chunk — a direct lookup into its already-deserialized block-entity map, no re-resolution through the level's
+chunk manager.
+
+**Deliberately no special case for `generated == true`.** That flag also fires when a chunk's saved data is missing
+and Minecraft regenerates it fresh — exactly the "region file deleted externally" scenario this feature targets.
+Skipping reconciliation there would silently defeat the main use case. There's no performance reason to skip it
+either: the `chunkIndex` lookup is a single hashmap `get` returning an empty list for the overwhelming majority of
+chunk loads (no signs ever tracked there), so cost is the same whether or not the chunk is newly generated.
+
+## Performance
+
+- Cost per chunk load: one `ConcurrentHashMap.get` (via `keysInChunk`) — O(1), returns empty for nearly every chunk.
+- Only chunks that do have tracked signs (a tiny fraction in practice) pay any further cost, bounded by the number of
+  signs in that one 16x16 chunk (typically 0-2), each a single `getBlockEntity` call already backed by a hashmap the
+  chunk itself maintains — no scanning, no I/O, no BlueMap API calls unless a removal is actually dispatched.
+- No new threading: runs synchronously on the same thread the existing `BLOCK_ENTITY_LOAD` handler already runs on;
+  `SignManager.remove`'s dispatch to BlueMap still goes through the existing async `ReactiveQueue`.
+
+## Changes (files)
+
+1. **New** `core/signs/SignChunkKey.java` — record + `forEntryKey`.
+2. **New** `core/signs/SignChunkIndex.java` — the add/remove/keysInChunk/clear wrapper described above.
+3. **`core/signs/SignManager.java`** — add `chunkIndex` field, wire it into the add/remove paths and
+   `reloadSigns()`, add `getKeysInChunk` static method.
+4. **`BlueMapSignMarkersMod.java`** — register `ServerChunkEvents.CHUNK_LOAD`, add `onChunkLoad` handler.
+5. **New tests**:
+   - `src/test/java/.../core/signs/SignChunkKeyTest.java` — `floorDiv` chunk assignment, negative coordinates,
+     boundary blocks (15/16), mirroring `SignRegionKeyTest`'s pattern.
+   - `src/test/java/.../core/signs/SignChunkIndexTest.java` — add/query round-trip, multiple signs in one chunk,
+     signs in different chunks/dimensions stay isolated, `remove` drops the chunk entry once its set empties,
+     `clear` resets everything.
+
+No `SignFileVersions` bump, no persisted-schema change — this is runtime-cache-only. No `mod_version` bump either:
+the last release hasn't shipped yet, so this folds into the same in-progress `26.2-0.17.0`.
+
+## Out of scope
+
+- A config toggle to disable reconciliation — not requested, and there's no existing feature-flag mechanism in this
+  mod to extend.
+- Reconciling on chunk *unload* — not requested.
+- Any change to `SignRegionKey`/region-sharded on-disk storage — persistence layout is unrelated to this in-memory
+  lookup index.
+
+## Verification
+
+- `./gradlew test` — new `SignChunkKeyTest`/`SignChunkIndexTest` pass alongside the existing suite.
+- `./gradlew build` — full build still succeeds.
+- Manual, via `./gradlew runServer` (this handler touches live Minecraft/Fabric types, no automated coverage
+  possible per `testing.md`):
+  1. Place a sign, confirm its marker appears in BlueMap. Restart the server, confirm the marker is unchanged and no
+     spurious remove/re-add happens (watch the log for the new INFO line — it should **not** appear).
+  2. Place a sign, stop the server, then remove the physical block without going through the mod (e.g. delete that
+     chunk's region `.mca` file to force a regen, or edit the block out with an NBT editor). Restart, walk into that
+     chunk, confirm: the INFO log line appears, and the stale marker disappears from BlueMap.
+  3. Repeat with signs in two different dimensions (overworld + nether) at the same x/z to confirm dimension
+     isolation — loading one dimension's chunk must not touch the other's sign at the same coordinates.

plans/sign-storage-refactor-options.md

@@ -0,0 +1,77 @@
+# In-memory sign storage: refactor options
+
+## Context
+
+`SignManager` (`src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignManager.java`, `signCache` field) holds
+the only in-memory copy of known signs: a single flat `ConcurrentHashMap<SignEntryKey, SignEntry>` (`signCache`). Each
+`SignEntry` (`core/signs/SignEntry.java`) carries `playerId` plus parsed front/back text
+(`SignLinesParseResult`: prefix/label/detail, `core/signs/SignLinesParseResult.java`). BlueMap itself only ever
+receives label/detail via dispatched `MarkerAction`s — it is not queried back for state.
+
+This doc answers two questions raised during review: is the current layout a space/time problem, and does it make
+sense to keep a full original copy of each sign in memory at all given markers already live in BlueMap.
+
+## Performance analysis
+
+**Time.** Hot-path operations (`addOrUpdateSign`, `removeByKey`, both driven by mixin injects on player edit / block
+removal, and `BLOCK_ENTITY_LOAD`) are `ConcurrentHashMap` get/put/remove — O(1). `getAllSigns()` copies the whole map
+into an `ArrayList` — O(n), but its only callers are `saveSigns` (server stop) and `reloadSigns()` (BlueMap reset),
+both rare, whole-lifecycle events, not per-sign operations. No time bottleneck exists at any realistic scale.
+
+**Space.** Per sign: a `SignEntryKey` (3 ints + a `parentMap` String) stored twice — once as the map key, once again
+inside `SignEntry.key()` — plus `playerId` and two `SignLinesParseResult` records (3 Strings each). Call it
+~150-250 bytes of JVM overhead per entry plus string data. Even 10,000 signs (far beyond what a real server
+accumulates — that's more player-placed signs than most maps have blocks-of-interest) is a few MB. Not a real
+problem.
+
+## Does the original copy need to exist?
+
+Yes. BlueMap is a downstream projection of this cache, not a queryable store the mod can read back from. The cache
+is the only source of truth for three things a marker-only view can't provide:
+
+1. **Diffing.** `isTextDifferent(...)` compares label/detail against the cached entry before dispatching an update,
+   so a player re-saving identical text doesn't spam BlueMap's marker store or connected web clients with a no-op
+   update.
+2. **Reset replay.** `reset()` → `reloadSigns()` clears and rebuilds the whole cache when BlueMap itself resets.
+   There is no "ask BlueMap for current markers" path — the mod must be able to resend everything from its own
+   state.
+3. **Persistence.** `signs.json` (region-sharded per
+   `plans/region-sharded-sign-persistence-plan.md`) is written from `SignManager.getAll()`. Nothing about BlueMap
+   survives a server restart for this mod's purposes; this cache is what does.
+
+Removing the original copy isn't on the table. The only real question is whether its *layout* is worth changing.
+
+## Options
+
+**1. Do nothing — keep the flat `ConcurrentHashMap<SignEntryKey, SignEntry>`**
+- Pros: simplest, already O(1) on the hot path, proven correct, zero migration risk.
+- Cons: `SignEntryKey` duplicated (map key + `SignEntry.key()` field); `SignEntryHelper.getPrefix`/`getLabel`/
+  `getDetail` recompute from raw parse results on every call site instead of being cached once.
+
+**2. Cache derived fields (prefix/label/detail) on the entry instead of recomputing via `SignEntryHelper` each call**
+- Pros: cuts redundant string work in `addOrUpdateSign`, which currently calls `getPrefix`/`getLabel`/`getDetail`
+  up to ~3 times per invocation across both the new and existing entry.
+- Cons: real code churn for a CPU micro-optimization on a low-frequency hot path (player sign edits, not a
+  tight loop); `signs.json`'s `SignEntryV2` schema still needs the raw front/back split for persistence regardless,
+  so this only saves cycles, not memory or file format.
+
+**3. Shard the in-memory map by dimension (`Map<parentMap, ConcurrentMap<xyz, SignEntry>>`), mirroring the
+   region-sharded on-disk format from #109**
+- Pros: conceptually matches the disk layout; would let reset/save scope to a single dimension if that ever became
+  a requirement.
+- Cons: adds a second map hop to every lookup for a capability nothing currently uses — `reset()` and save/load are
+  whole-world operations today, not per-dimension. `plans/region-sharded-sign-persistence-plan.md` explicitly chose
+  to leave the in-memory map as-is for this reason, deferring a region-indexed in-memory view to whichever future
+  feature (chunk-reset reconciliation) actually consumes it. Speculative generality with no current caller.
+
+**4. Bit-pack `x/y/z` into a primitive `long` key (per dimension) instead of a boxed `SignEntryKey` record**
+- Pros: only matters at very large scale (100k+ signs) — shaves allocation/lookup overhead.
+- Cons: loses `SignEntryKey`'s readable `equals`/`toString`; needs careful encoding for Minecraft's y-range
+  (-64..320); meaningful complexity for a scale this mod will not hit in practice.
+
+## Recommendation
+
+Option 1. At realistic Minecraft server sign counts this isn't a storage problem — it's a working, correct,
+O(1)-on-the-hot-path cache doing exactly the job the architecture needs (diff, replay, persist). Option 2 is worth
+revisiting only if profiling ever shows `addOrUpdateSign` as hot; options 3 and 4 solve problems this project does
+not have.

src/main/java/com/tpwalke2/bluemapsignmarkers/BlueMapSignMarkersMod.java

@@ -5,22 +5,30 @@
 import com.tpwalke2.bluemapsignmarkers.core.signs.persistence.SignProvider;
 import net.fabricmc.api.DedicatedServerModInitializer;
 import net.fabricmc.fabric.api.event.lifecycle.v1.ServerBlockEntityEvents;
+import net.fabricmc.fabric.api.event.lifecycle.v1.ServerChunkEvents;
 import net.fabricmc.fabric.api.event.lifecycle.v1.ServerLifecycleEvents;
+import net.minecraft.core.BlockPos;
 import net.minecraft.server.MinecraftServer;
 import net.minecraft.server.level.ServerLevel;
 import net.minecraft.world.level.block.entity.BlockEntity;
 import net.minecraft.world.level.block.entity.SignBlockEntity;
+import net.minecraft.world.level.chunk.LevelChunk;
 import net.minecraft.world.level.storage.LevelResource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.nio.file.Path;
 
 public class BlueMapSignMarkersMod implements DedicatedServerModInitializer, ServerPathProvider {
 
+	private static final Logger LOGGER = LoggerFactory.getLogger(Constants.MOD_ID);
+
 	@Override
 	public void onInitializeServer() {
 		ServerLifecycleEvents.SERVER_STARTING.register(this::onServerStarting);
 		ServerLifecycleEvents.SERVER_STOPPING.register(this::onServerStopping);
 		ServerBlockEntityEvents.BLOCK_ENTITY_LOAD.register(this::onBlockEntityLoad);
+		ServerChunkEvents.CHUNK_LOAD.register(this::onChunkLoad);
 	}
 
 	private void onServerStarting(MinecraftServer server) {
@@ -58,4 +66,21 @@ private void onBlockEntityLoad(BlockEntity blockEntity, ServerLevel world) {
 
 		SignManager.addOrUpdate(SignHelper.createSignEntry(castBlockEntity, "unknown"));
 	}
+
+	// No special case for a newly-generated chunk (generated == true): that flag also fires when a chunk's saved
+	// data is missing and Minecraft regenerates it fresh - exactly the "region file deleted externally" case this
+	// reconciliation targets. Skipping it there would defeat the main use case, and there's no perf reason to:
+	// getKeysInChunk is a single hashmap lookup, so the cost is the same either way.
+	private void onChunkLoad(ServerLevel level, LevelChunk chunk, boolean generated) {
+		var parentMap = SignHelper.getSignParentMap(level);
+		var chunkPos = chunk.getPos();
+
+		for (var key : SignManager.getKeysInChunk(parentMap, chunkPos.x(), chunkPos.z())) {
+			if (!(chunk.getBlockEntity(new BlockPos(key.x(), key.y(), key.z())) instanceof SignBlockEntity)) {
+				LOGGER.info("Removing stale sign marker at {} - no sign block found on chunk load "
+						+ "(external deletion/regen?)", key);
+				SignManager.remove(key);
+			}
+		}
+	}
 }

src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkIndex.java

@@ -0,0 +1,40 @@
+package com.tpwalke2.bluemapsignmarkers.core.signs;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+// Secondary lookup alongside SignManager's flat sign cache: which sign keys fall in a given chunk, so chunk-load
+// reconciliation can ask "signs known here?" in O(1) instead of scanning every cached sign.
+public class SignChunkIndex {
+    private final ConcurrentHashMap<SignChunkKey, Set<SignEntryKey>> signsByChunk = new ConcurrentHashMap<>();
+
+    // add/remove mutate the per-chunk set inside the map's own compute-family remapping function (not via a
+    // fetch-then-mutate-outside-the-lock pattern) so the "does this chunk still have entries" decision and the
+    // mutation happen as one atomic step - ConcurrentHashMap serializes compute/computeIfPresent/computeIfAbsent
+    // calls against each other for the same key, so a concurrent add can never resurrect a set remove just deleted.
+    public void add(SignEntryKey key) {
+        signsByChunk.compute(SignChunkKey.forEntryKey(key), (unusedKey, keysInChunk) -> {
+            var result = keysInChunk != null ? keysInChunk : ConcurrentHashMap.<SignEntryKey>newKeySet();
+            result.add(key);
+            return result;
+        });
+    }
+
+    public void remove(SignEntryKey key) {
+        signsByChunk.computeIfPresent(SignChunkKey.forEntryKey(key), (unusedKey, keysInChunk) -> {
+            keysInChunk.remove(key);
+            return keysInChunk.isEmpty() ? null : keysInChunk;
+        });
+    }
+
+    public List<SignEntryKey> keysInChunk(String parentMap, int chunkX, int chunkZ) {
+        var keysInChunk = signsByChunk.get(new SignChunkKey(parentMap, chunkX, chunkZ));
+        return keysInChunk == null ? List.of() : new ArrayList<>(keysInChunk);
+    }
+
+    public void clear() {
+        signsByChunk.clear();
+    }
+}

src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkKey.java

@@ -0,0 +1,14 @@
+package com.tpwalke2.bluemapsignmarkers.core.signs;
+
+public record SignChunkKey(String parentMap, int chunkX, int chunkZ) {
+    // Vanilla chunk size in blocks - distinct from and finer-grained than SignRegionKey's 512-block
+    // (32-chunk) region math, which is a persistence file-layout concern, not a runtime lookup one.
+    private static final int CHUNK_SIZE_BLOCKS = 16;
+
+    public static SignChunkKey forEntryKey(SignEntryKey key) {
+        return new SignChunkKey(
+                key.parentMap(),
+                Math.floorDiv(key.x(), CHUNK_SIZE_BLOCKS),
+                Math.floorDiv(key.z(), CHUNK_SIZE_BLOCKS));
+    }
+}