fix(ENGKNOW-3670): don't rebuild gord dictionary through the .gord.link on a cache hit (#131)

* fix(ENGKNOW-3670): don't rebuild gord dictionary through the .gord.link on a cache hit

On re-run with an active result cache, GeneralQueryHandler.generateDictionaryFile
received the raw `<fingerprint>.gord.link` cache entry. It checked project-root/
thedict.gord (never present) so it always rebuilt, and rebuilt through the
unresolved link, writing thedict.gord (and its atomic-write temp sibling) inside
the `.gord.link` FILE -> "Not a directory".

Resolve the link to the real gord folder first; skip the rebuild when thedict.gord
is already there (the normal cache-hit case), and target the resolved folder in the
rare rebuild fallback so no path is ever concatenated onto the link file. The now
unused fileRoot parameter (and its executeBatch local) is removed.

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

* test(ENGKNOW-3670): cover gord-folder cache-hit and cache-miss-overwrite paths

White-box (UTestPGorGordFolderDelete.scala): fold in the ENGKNOW-3670 regression
alongside the existing take-1 delete-gate suite, reusing its stub-cache session.
(1) cache hit with thedict.gord present -> rebuild skipped, no crash, no temp file
under the link; (2) fallback with thedict.gord missing -> rebuild targets the
resolved folder, never writes through the raw `.gord.link` (asserts the outcome's
cause chain names neither ".gord.link" nor "Not a directory", so it fails against
the pre-fix code). No end-to-end test can catch this locally: TestUtils.runGorPipe
uses LocalFileCacheClient, which re-runs the write when a cached link does not
resolve, masking the server-cache crash.

End-to-end (UTestGorWriteFolder.java): add testPgorOverwriteGordFolderWith-
DifferentDataOnCacheMiss -- overwrite an existing gord folder with disjoint data on
a cache miss (different query => different signature => real delete-and-rewrite) and
assert only the second write's rows remain (no superset/stale dictionary).

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: gmagnu

gortools/src/main/scala/gorsat/QueryHandlers/GeneralQueryHandler.scala

@@ -114,17 +114,25 @@ class GeneralQueryHandler(context: GorContext, header: Boolean) extends GorParal
     cmdUpper.startsWith(CommandParseUtilities.GOR_DICTIONARY_FOLDER_PART) || cmdUpper.startsWith(CommandParseUtilities.GOR_DICTIONARY_FOLDER)
   }
 
-  def generateDictionaryFile(commandToExecute: String, fileRoot: String, fileReader: FileReader, useMd5: Boolean, cacheFile: String): Unit = {
-    if (isDictionaryFolderMacro(commandToExecute.toUpperCase()) && !fileReader.exists(PathUtils.resolve(fileRoot, GorOptions.DEFAULT_FOLDER_DICTIONARY_NAME))) {
-      runCommand(context, commandToExecute, cacheFile, useMd5, theTheDict = true)
+  def generateDictionaryFile(commandToExecute: String, fileReader: FileReader, useMd5: Boolean, cacheFile: String): Unit = {
+    if (isDictionaryFolderMacro(commandToExecute.toUpperCase())) {
+      // On a cache hit `cacheFile` may be a `<fingerprint>.gord.link` entry. Resolve it to the real
+      // gord folder before deciding anything: the old guard checked project-root/thedict.gord so it
+      // never matched and always rebuilt, and the rebuild ran through the unresolved link
+      // -> thedict.gord written INSIDE the `.gord.link` file -> "Not a directory" (ENGKNOW-3670).
+      val resolvedFolder = fileReader.resolveUrl(cacheFile, false).getFullPath()
+      if (!fileReader.exists(PathUtils.resolve(resolvedFolder, GorOptions.DEFAULT_FOLDER_DICTIONARY_NAME))) {
+        // Fallback (partial/corrupt cache): rebuild the dictionary, but target the resolved folder,
+        // never the raw `.gord.link` path.
+        runCommand(context, commandToExecute, resolvedFolder, useMd5, theTheDict = true)
+      }
     }
   }
 
   def executeBatch(commandSignatures: Array[String], commandsToExecute: Array[String], batchGroupNames: Array[String], cacheFiles: Array[String], gorMonitor: GorMonitor): Array[String] = {
     val fileNames = new Array[String](commandSignatures.length)
     val fileCache = context.getSession.getProjectContext.getFileCache
     val fileReader = context.getSession.getProjectContext.getFileReader
-    val fileRoot = context.getSession.getProjectContext.getProjectRoot
     var commandList: List[() => Unit] = Nil
     val useMd5 = System.getProperty("gor.caching.md5.enabled", "false").toBoolean
 
@@ -146,7 +154,7 @@ class GeneralQueryHandler(context: GorContext, header: Boolean) extends GorParal
               runAndStoreInCache(nested, fileCache, useMd5)
             }
           } else {
-            generateDictionaryFile(commandToExecute, fileRoot, fileReader, useMd5, cacheFile)
+            generateDictionaryFile(commandToExecute, fileReader, useMd5, cacheFile)
             nested.cached(cacheFile)
             cacheFile
           }

gortools/src/test/java/gorsat/UTestGorWriteFolder.java

@@ -220,4 +220,31 @@ public void testOverwritePgorWriteGordFolderWithGorrowsMerge() throws IOExceptio
                 "-cachedir", cachePath.toString());
         Assert.assertEquals("pgor write gord should contain chr2 + chr19 rows", 15001L + 10002L, rowCount);
     }
+
+    // Overwrite an existing gord folder with entirely DIFFERENT data on a cache MISS (the two
+    // writes use different queries -> different signatures -> the second is a genuine
+    // delete-and-rewrite, not a skipped cache hit). The folder must be replaced, not appended to:
+    // if stale parts from the first write survived (a superset dictionary) the counts below break.
+    @Test
+    public void testPgorOverwriteGordFolderWithDifferentDataOnCacheMiss() {
+        var folderpath = workDirPath.resolve("overwrite.gord");
+        var gorroot = workDirPath.toAbsolutePath().toString();
+
+        // First write: 1000 chr1 rows.
+        TestUtils.runGorPipe("pgor <(gorrows -p chr1:1000-2000) | write " + folderpath,
+                "-gorroot", gorroot, "-cachedir", cachePath.toString());
+
+        // Overwrite with 2000 chr2 rows (disjoint from the first write, different query).
+        TestUtils.runGorPipe("pgor <(gorrows -p chr2:1000-3000) | write " + folderpath,
+                "-gorroot", gorroot, "-cachedir", cachePath.toString());
+
+        // Only the second write's data may remain: 2000 rows, none on chr1.
+        long total = TestUtils.runGorPipeCount(new String[]{"gor " + folderpath,
+                "-gorroot", gorroot, "-cachedir", cachePath.toString()}, false);
+        Assert.assertEquals("overwrite must replace, not append: only the 2000 chr2 rows", 2000L, total);
+
+        long staleChr1 = TestUtils.runGorPipeCount(new String[]{"gor -p chr1 " + folderpath,
+                "-gorroot", gorroot, "-cachedir", cachePath.toString()}, false);
+        Assert.assertEquals("no stale chr1 rows from the first write may remain", 0L, staleChr1);
+    }
 }

gortools/src/test/scala/gorsat/Utilities/UTestPGorGordFolderDelete.scala

@@ -28,25 +28,34 @@ import gorsat.QueryHandlers.GeneralQueryHandler
 import gorsat.Script.ExecutionBlock
 import gorsat.process.{FreemarkerReportBuilder, GenericRunnerFactory, GorSessionCacheManager, SessionBasedQueryEvaluator}
 import org.gorpipe.gor.clients.LocalFileCacheClient
-import org.gorpipe.gor.model.DriverBackedFileReader
+import org.gorpipe.gor.model.{DriverBackedFileReader, GorOptions}
 import org.gorpipe.gor.session.{GorSession, ProjectContext, SystemContext}
 import org.gorpipe.gor.table.util.PathUtils
 import org.junit.Assert
 import org.junit.runner.RunWith
 import org.scalatest.funsuite.AnyFunSuite
 import org.scalatestplus.junit.JUnitRunner
 
+import java.nio.charset.StandardCharsets
 import java.nio.file.{Files, Path}
+import scala.jdk.CollectionConverters._
 
-// Regression test for ENGKNOW-3656: `pgor ... | write x.gord` deleted the target gord folder
-// even when the file cache already held the result and the write was skipped at execution time,
-// leaving the skipped write with nothing to build the dictionary from. The fix gates the
-// folder-delete in PGor.makeGorDict (and PartGor/Parallel) on the file-cache signature.
+// White-box regression tests for the pgor gord-folder cache-hit path. Both bugs here only
+// reproduce with a persistent/server file cache: the local LocalFileCacheClient resolves the
+// cached link to the folder and re-runs the write, masking them -- so these tests inject a stub
+// cache / craft cache links and drive GeneralQueryHandler/PGor directly.
 //
-// The original crash only reproduces with a persistent/server file cache (the local
-// LocalFileCacheClient resolves the cached link to the folder and re-runs the write, masking
-// the bug). So this white-box test injects a stub file cache that reports a cache hit for the
-// signature and asserts the delete decision directly.
+// ENGKNOW-3656: `pgor ... | write x.gord` deleted the target gord folder even when the file cache
+// already held the result and the write was skipped at execution time, leaving the skipped write
+// with nothing to build the dictionary from. The fix gates the folder-delete in PGor.makeGorDict
+// (and PartGor/Parallel) on the file-cache signature. Covered by the makeGorDict tests below.
+//
+// ENGKNOW-3670: on a cache hit the result cache hands GeneralQueryHandler.generateDictionaryFile a
+// raw `<fp>.gord.link` path. Before the fix it checked project-root/thedict.gord (never present) so
+// it always rebuilt, and rebuilt through the unresolved link, writing thedict.gord INSIDE the
+// `.gord.link` file -> "Not a directory". The fix resolves the link, skips when thedict.gord is
+// present, and targets the resolved folder in the fallback. Covered by the generateDictionaryFile
+// tests below.
 //
 // Written as an AnyFunSuite because `:gortools:testScala` discovers tests via
 // `org.scalatest.tools.Runner -R build/classes/scala/test`, which only picks up
@@ -144,4 +153,90 @@ class UTestPGorGordFolderDelete extends AnyFunSuite {
 
     Assert.assertFalse("gord folder must be deleted when the result is not cached", survived)
   }
+
+  // ---- ENGKNOW-3670: generateDictionaryFile must not rebuild through the raw .gord.link ----
+
+  /** Create a real gord folder and a `<name>.gord.link` file whose content is that folder's
+    * absolute path (mimicking a result-cache link entry). Returns the link file path. */
+  private def makeGordLink(root: Path, folderName: String, withDict: Boolean): Path = {
+    val folder = Files.createDirectories(root.resolve(folderName))
+    Files.createFile(folder.resolve("part_chr1.gorz"))
+    if (withDict) Files.createFile(folder.resolve(GorOptions.DEFAULT_FOLDER_DICTIONARY_NAME))
+    val link = root.resolve(folderName + ".link")
+    Files.write(link, folder.toAbsolutePath.toString.getBytes(StandardCharsets.UTF_8))
+    link
+  }
+
+  /** Absolute paths of every regular file under `root` whose name contains "-temp-". */
+  private def tempFilesUnder(root: Path): List[Path] =
+    scala.util.Using.resource(Files.walk(root)) { stream =>
+      stream.iterator().asScala.filter(Files.isRegularFile(_))
+        .filter(_.getFileName.toString.contains("-temp-")).toList
+    }
+
+  /** Walk `t`'s cause chain (bounded, to guard against cyclic causes) and concatenate every
+    * class name + message, so an assertion can inspect the whole chain, not just the wrapper. */
+  private def fullTrace(t: Throwable): String = {
+    val sb = new StringBuilder
+    var cur: Throwable = t
+    var seen = 0
+    while (cur != null && seen < 20) {
+      sb.append(cur.getClass.getName).append(": ").append(cur.getMessage).append('\n')
+      cur = cur.getCause
+      seen += 1
+    }
+    sb.toString
+  }
+
+  test("generateDictionaryFile skips the rebuild on a cache hit (thedict.gord present): no crash, no temp under the link") {
+    val root = Files.createTempDirectory("uTestGenDict3670Hit")
+    root.toFile.deleteOnExit()
+    val session = sessionWithStubCache(root, cachedFile = null)
+    val handler = new GeneralQueryHandler(session.getGorContext, false)
+    val link = makeGordLink(root, "cached.gord", withDict = true)
+
+    // Must not throw (pre-fix: GorResourceException "Not a directory" from writing under the link).
+    handler.generateDictionaryFile(
+      commandToExecute = "GORDICTFOLDER [x] chr1",
+      fileReader = session.getProjectContext.getFileReader,
+      useMd5 = false,
+      cacheFile = link.toAbsolutePath.toString)
+
+    Assert.assertTrue("link file must remain a regular file", Files.isRegularFile(link))
+    Assert.assertTrue("no -temp- dictionary file may be created", tempFilesUnder(root).isEmpty)
+  }
+
+  test("generateDictionaryFile fallback (thedict.gord missing) never writes under the .gord.link file") {
+    val root = Files.createTempDirectory("uTestGenDict3670Miss")
+    root.toFile.deleteOnExit()
+    val session = sessionWithStubCache(root, cachedFile = null)
+    val handler = new GeneralQueryHandler(session.getGorContext, false)
+    val link = makeGordLink(root, "cached.gord", withDict = false)
+
+    // The rebuild may fail for unrelated reasons in this minimal session (no real part blocks) --
+    // that is fine. What must never happen is the ENGKNOW-3670 crash: pre-fix, the rebuild runs
+    // through the raw `<fp>.gord.link` FILE, failing with a "Not a directory" error whose message /
+    // cause chain names the `.gord.link` path. Capture the outcome so that crash surfaces.
+    val outcome = scala.util.Try {
+      handler.generateDictionaryFile(
+        commandToExecute = "GORDICTFOLDER [x] chr1",
+        fileReader = session.getProjectContext.getFileReader,
+        useMd5 = false,
+        cacheFile = link.toAbsolutePath.toString)
+    }
+
+    outcome.failed.foreach { t =>
+      val trace = fullTrace(t)
+      Assert.assertFalse(
+        s"rebuild must never attempt to write through the raw .gord.link FILE path, but got:\n$trace",
+        trace.contains(".gord.link"))
+      Assert.assertFalse(
+        s"rebuild must never fail with the ENGKNOW-3670 'Not a directory' crash, but got:\n$trace",
+        trace.contains("Not a directory"))
+    }
+
+    Assert.assertTrue("link file must remain a regular file", Files.isRegularFile(link))
+    val underLink = tempFilesUnder(root).filter(_.toString.contains(".gord.link"))
+    Assert.assertTrue("no dictionary temp file may be created under the .gord.link path", underLink.isEmpty)
+  }
 }