fix(ENGKNOW-3657): harden DictionaryEntries read accessors against clear() races

model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java

@@ -45,6 +45,10 @@
  * serialized and must NOT run concurrently with reads on the same instance (structural
  * modification of the backing list would break in-flight iteration); share a read-only instance
  * for concurrent reads and use a separate instance for writes.
+ *
+ * <p>The public read accessors ({@code getEntries()}/{@code getAllActiveTags()}) return
+ * <b>unmodifiable</b> views so callers cannot structurally modify the shared internal state;
+ * copy the result if a mutable list is needed (see {@code selectAll()}).
  */
 public class DictionaryEntries<T extends DictionaryEntry> implements IDictionaryEntries<T> {
     private static final Logger log = LoggerFactory.getLogger(DictionaryEntries.class);
@@ -94,7 +98,7 @@ synchronized public void insert(T line, boolean hasUniqueTags) {
             delete(oldLine, true);
         }
 
-        getEntries().add(line);
+        loadedEntries().add(line);
 
         addEntryToContentMap(line);
         addEntryToTagMap(line);
@@ -110,15 +114,15 @@ synchronized public void insert(T line, boolean hasUniqueTags) {
     synchronized public void delete(T lineToRemove, boolean keepIfBucket) {
         T line = this.findLine(lineToRemove);
         if (line != null) {
-            getEntries().remove(line);
+            loadedEntries().remove(line);
             removeEntryFromContentMap(line);
             removeEntryFromTagMap(line);
 
             if (line.hasBucket() && keepIfBucket) {
                 // NOTE: the deleted flag is part of the hashCode so we remove and add again if we change it.
                 T entry = (T) DictionaryEntry.copy(line);
                 entry.setDeleted(true);
-                getEntries().add(entry);
+                loadedEntries().add(entry);
                 addEntryToContentMap(entry);
                 addEntryToTagMap(entry);
             }
@@ -127,6 +131,8 @@ synchronized public void delete(T lineToRemove, boolean keepIfBucket) {
 
     @Override
     synchronized public void clear() {
+        // We deliberatly set list/maps to null, instead of clearing.  This is because someone could be reading
+        // the buffers and we want to let them finish.
         dataLoaded = false;
         this.rawLines = null;
         clearContentMap();
@@ -135,16 +141,32 @@ synchronized public void clear() {
 
     @Override
     public List<T> getEntries() {
-        if (!this.dataLoaded) {
+        // Return an unmodifiable view: the backing list is shared across concurrent readers and
+        // must not be structurally modified from the outside (see class-level thread-safety note).
+        return Collections.unmodifiableList(loadedEntries());
+    }
+
+    /**
+     * Load the entries if needed and return the live backing list.
+     *
+     * <p>Reads {@code rawLines} into a single local so the load check and the returned reference are
+     * the same read; a separate re-read of the volatile field could observe {@code null} if a
+     * concurrent {@code clear()} slipped in (which the contract forbids, but the local read makes the
+     * accessor robust anyway). For internal use only — mutators hold the instance lock; read
+     * accessors that expose the result to callers must wrap it (see {@link #getEntries()}).
+     */
+    private List<T> loadedEntries() {
+        List<T> lines = this.rawLines;
+        if (!this.dataLoaded || lines == null) {
             loadLinesAndUpdateIndices();
+            lines = this.rawLines;
         }
-
-        return this.rawLines;
+        return lines;
     }
 
     @Override
     public List<T> getEntries(String... aliasesAndTags) {
-        List<T> lines2Search = getEntries();
+        List<T> lines2Search = loadedEntries();
         updateTagMap();
         if (tagHashToLines != null && aliasesAndTags != null) {
             // If we have tags and tag map to lines we use that to get a better list of lines to search..
@@ -195,7 +217,10 @@ public boolean isLoaded() {
     @Override
     public Set<String> getAllActiveTags() {
         updateContentMap();
-        return activeTags.elementSet();
+        // Read the volatile once and wrap: elementSet() is a live view of the internal multiset and
+        // supports remove()/clear(), so it must be made unmodifiable before it escapes to callers.
+        Multiset<String> tags = this.activeTags;
+        return tags != null ? Collections.unmodifiableSet(tags.elementSet()) : Collections.emptySet();
     }
 
     @Override
@@ -206,13 +231,13 @@ public boolean hasDeletedEntries() {
 
     @Override
     public int size() {
-        return getEntries().size();
+        return loadedEntries().size();
     }
 
     @Override
     public int getActiveLinesCount() {
         updateContentMap();
-        int size = getEntries().size();
+        int size = loadedEntries().size();
         return size - deletedEntriesCount;
     }
 
@@ -233,6 +258,9 @@ synchronized private void updateContentMap() {
 
     synchronized private void updateTagMap() {
         if (tagHashLoaded) return;
+        if (!dataLoaded) {
+            loadLinesAndUpdateIndices();
+        }
         tagHashToLines = ArrayListMultimap.create(rawLines.size(), 1);
         for (T entry : rawLines) {
             addEntryToTagMap(entry);
@@ -350,7 +378,7 @@ private List<T> loadLines() {
     @Override
     public T findLine(T line) {
         // Using contentHashMap
-        List<T> allLines = getEntries();
+        List<T> allLines = loadedEntries();
         updateContentMap();
         List<T> lines2Search = contentHashToLines != null ? contentHashToLines.get(line.getSearchHash()) : allLines;
 
@@ -361,7 +389,7 @@ private T findLineWithTag(T line) {
         String[] linekey = line.getFilterTags();
 
         // Using contentHashMap
-        List<T> lines2Search = getEntries();
+        List<T> lines2Search = loadedEntries();
         if (tagHashToLines != null) {
             lines2Search = new ArrayList<>();
             for (String tag : linekey) {

model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesViews.java

@@ -0,0 +1,96 @@
+package org.gorpipe.gor.table.dictionary;
+
+import org.gorpipe.gor.table.dictionary.gor.GorDictionaryEntry;
+import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.lang.reflect.Field;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Deterministic regression tests for the read-accessor hardening in {@link DictionaryEntries}:
+ * <ul>
+ *   <li>{@code getEntries()} and {@code getAllActiveTags()} must return unmodifiable views so
+ *       callers cannot structurally modify the shared backing state (finding #3).</li>
+ *   <li>The internal mutators ({@code insert}/{@code delete}) must keep working after being
+ *       switched off the public {@code getEntries()} accessor (regression for the refactor).</li>
+ *   <li>{@code getEntries(String...)} must lazily reload after {@code clear()} without tripping
+ *       over a null backing list (finding #2 guard).</li>
+ * </ul>
+ */
+public class UTestDictionaryEntriesViews {
+
+    @Rule
+    public TemporaryFolder workDir = new TemporaryFolder();
+
+    private GorDictionaryTable buildTable() throws Exception {
+        Path dir = workDir.getRoot().toPath();
+        Files.writeString(dir.resolve("a.gor"), "chrom\tpos\nchr1\t1\n");
+        Files.writeString(dir.resolve("b.gor"), "chrom\tpos\nchr1\t2\n");
+        Path gord = dir.resolve("t.gord");
+        Files.writeString(gord, "a.gor\tpn1\nb.gor\tpn2\n");
+        return new GorDictionaryTable.Builder<>(gord.toString()).build();
+    }
+
+    @SuppressWarnings("unchecked")
+    private IDictionaryEntries<GorDictionaryEntry> entriesOf(GorDictionaryTable table) throws Exception {
+        Field f = DictionaryTableReader.class.getDeclaredField("tableEntries");
+        f.setAccessible(true);
+        return (IDictionaryEntries<GorDictionaryEntry>) f.get(table);
+    }
+
+    @Test(expected = UnsupportedOperationException.class)
+    public void getEntries_returnsUnmodifiableList() throws Exception {
+        IDictionaryEntries<GorDictionaryEntry> entries = entriesOf(buildTable());
+        List<GorDictionaryEntry> list = entries.getEntries();
+        Assert.assertEquals(2, list.size());
+        list.clear(); // must throw - callers must not mutate the shared backing list
+    }
+
+    @Test(expected = UnsupportedOperationException.class)
+    public void getAllActiveTags_returnsUnmodifiableSet() throws Exception {
+        IDictionaryEntries<GorDictionaryEntry> entries = entriesOf(buildTable());
+        Set<String> tags = entries.getAllActiveTags();
+        Assert.assertTrue(tags.contains("pn1"));
+        // Guava's Multiset.elementSet() blocks add() but SUPPORTS remove()/clear(), which would
+        // strip occurrences from the internal multiset. The returned view must reject removal too.
+        tags.clear(); // must throw - live view of internal multiset must not be mutable
+    }
+
+    @Test
+    public void insertAndDelete_stillMutateBackingList() throws Exception {
+        GorDictionaryTable table = buildTable();
+        IDictionaryEntries<GorDictionaryEntry> entries = entriesOf(table);
+
+        Assert.assertEquals(2, entries.size());
+
+        GorDictionaryEntry newEntry =
+                (GorDictionaryEntry) new GorDictionaryEntry.Builder("c.gor", table.getRootPath()).alias("pn3").build();
+        entries.insert(newEntry, false);
+        Assert.assertEquals(3, entries.size());
+
+        entries.delete(newEntry, false);
+        Assert.assertEquals(2, entries.size());
+    }
+
+    @Test
+    public void getEntriesByTag_reloadsAfterClear() throws Exception {
+        GorDictionaryTable table = buildTable();
+        IDictionaryEntries<GorDictionaryEntry> entries = entriesOf(table);
+
+        Assert.assertEquals(1, entries.getEntries("pn1").size());
+
+        entries.clear();
+
+        // After clear the backing list is dropped; the tag-filtered read must reload it,
+        // not dereference a null rawLines (finding #2).
+        List<GorDictionaryEntry> afterClear = entries.getEntries("pn1");
+        Assert.assertEquals(1, afterClear.size());
+    }
+}