Skip to content

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

Merged
gmagnu merged 2 commits into
mainfrom
ENGKNOW-3670-gor-rewriting-gord-fails-take-2
Jul 23, 2026
Merged

fix(ENGKNOW-3670): don't rebuild gord dictionary through the .gord.link on a cache hit#131
gmagnu merged 2 commits into
mainfrom
ENGKNOW-3670-gor-rewriting-gord-fails-take-2

Conversation

@gmagnu

@gmagnu gmagnu commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Problem

pgor [...] | write x.gord crashes with Not a directory when re-run against an active result cache (stg):

.../cache/result_cache/cache/kid/<fp>.gord.link/thedict-temp-XXXX.gord: Not a directory
URI:   .../cache/result_cache/cache/kid/<fp>.gord.link/thedict.gord

Second attempt at ENGKNOW-3656 (#128), which fixed the local-cache case but not the server/result-cache case.

Root cause

On a cache hit, GeneralQueryHandler.executeBatch hands generateDictionaryFile the raw <fingerprint>.gord.link cache-entry path. That method then:

  1. checked fileRoot/thedict.gord (the project root — never present), so the skip-guard never fired and it rebuilt the dictionary on every cache hit; and
  2. rebuilt through the unresolved link, so writeOutGorDictionaryFolder string-concatenated thedict.gord (and its atomic-write -temp- sibling) onto the .gord.link fileNot a directory.

The result-cache stores a completed gord folder as a .gord.link whose content is the real folder path, so on a genuine cache hit the rebuild is unnecessary.

Fix

GeneralQueryHandler.generateDictionaryFile now:

  • resolves the .gord.link to the real folder (resolveUrl(cacheFile, false).getFullPath() — same mode already used at GeneralQueryHandler.scala:80);
  • skips the rebuild when thedict.gord already exists in the resolved folder (the normal re-run case — crash gone, and the wasteful every-hit rebuild is removed for local caches too);
  • in the rare fallback (link present but thedict.gord missing) rebuilds targeting the resolved folder, never the raw link.

Scope: take-1 delete-gate in PGor/PartGor/Parallel and writeOutGorDictionaryFolder's path construction are untouched.

Tests

New UTestGenerateDictionaryFile (white-box, stub .gord.link → folder):

  • cache hit with thedict.gord present → no rebuild, no crash, no -temp- file;
  • cache hit without thedict.gord → fallback never writes through the raw .gord.link (captures the outcome and asserts the cause chain names neither .gord.link nor Not a directory; fails against pre-fix code).

./gradlew :gortools:testScala → 666 succeeded, 0 failed (incl. take-1's UTestPGorGordFolderDelete). pgor/partgor/write JUnit suites green.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings July 23, 2026 10:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a result-cache regression where rerunning pgor ... | write x.gord on a cache hit could crash with Not a directory by ensuring dictionary generation resolves .gord.link entries to the real gord folder and skips unnecessary rebuilds when thedict.gord already exists.

Changes:

  • Update GeneralQueryHandler.generateDictionaryFile to resolve .gord.link paths and only rebuild thedict.gord when missing, targeting the resolved folder.
  • Add a ScalaTest regression suite covering both “cache hit with dict present” and “cache hit with dict missing (fallback)” scenarios.
  • Add design + implementation-plan documentation for ENGKNOW-3670 take-2.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
gortools/src/main/scala/gorsat/QueryHandlers/GeneralQueryHandler.scala Resolve cache link paths before dictionary existence checks and rebuilds to avoid writing through .gord.link.
gortools/src/test/scala/gorsat/Utilities/UTestGenerateDictionaryFile.scala New regression tests ensuring no rebuild/crash on cache hits and no writing under the raw link path.
docs/superpowers/specs/2026-07-22-engknow-3670-gor-rewriting-gord-fails-take-2-design.md Design write-up documenting root cause, fix approach, and scope boundaries.
docs/superpowers/plans/2026-07-22-engknow-3670-gor-rewriting-gord-fails-take-2.md Step-by-step implementation plan and test expectations for the fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gortools/src/test/scala/gorsat/Utilities/UTestGenerateDictionaryFile.scala Outdated
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

Junit Tests - Summary

4 739 tests  +3   4 561 ✅ +3   17m 38s ⏱️ - 1m 40s
  488 suites ±0     178 💤 ±0 
  488 files   ±0       0 ❌ ±0 

Results for commit 9136780. ± Comparison against base commit fc81c83.

♻️ This comment has been updated with latest results.

@gmagnu
gmagnu force-pushed the ENGKNOW-3670-gor-rewriting-gord-fails-take-2 branch from f923139 to 0571c75 Compare July 23, 2026 12:38
Copilot AI review requested due to automatic review settings July 23, 2026 12:38
@gmagnu
gmagnu force-pushed the ENGKNOW-3670-gor-rewriting-gord-fails-take-2 branch from 0571c75 to 68446e0 Compare July 23, 2026 12:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings July 23, 2026 12:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread gortools/src/test/scala/gorsat/Utilities/UTestPGorGordFolderDelete.scala Outdated
…nk 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) <[email protected]>
Copilot AI review requested due to automatic review settings July 23, 2026 12:58
@gmagnu
gmagnu force-pushed the ENGKNOW-3670-gor-rewriting-gord-fails-take-2 branch from 68446e0 to ed16508 Compare July 23, 2026 12:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

  • Files.walk(root) returns a java.util.stream.Stream that should be closed. As written, the stream is never closed, which can leak file descriptors and cause flaky tests on some platforms (notably Windows). Wrap it in try/finally and close the stream.
  /** Absolute paths of every regular file under `root` whose name contains "-temp-". */
  private def tempFilesUnder(root: Path): List[Path] =
    Files.walk(root).iterator().asScala.filter(Files.isRegularFile(_))
      .filter(_.getFileName.toString.contains("-temp-")).toList

Copilot AI review requested due to automatic review settings July 23, 2026 13:05
@gmagnu
gmagnu force-pushed the ENGKNOW-3670-gor-rewriting-gord-fails-take-2 branch from ed16508 to 087175e Compare July 23, 2026 13:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

  • Files.walk(root) returns a Java Stream that must be closed. As written, tempFilesUnder leaks the stream (can cause file-handle leaks / directory locks, especially on Windows), which can make tests flaky over time.
  private def tempFilesUnder(root: Path): List[Path] =
    Files.walk(root).iterator().asScala.filter(Files.isRegularFile(_))
      .filter(_.getFileName.toString.contains("-temp-")).toList

…ite 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) <[email protected]>
Copilot AI review requested due to automatic review settings July 23, 2026 13:18
@gmagnu
gmagnu force-pushed the ENGKNOW-3670-gor-rewriting-gord-fails-take-2 branch from 087175e to 9136780 Compare July 23, 2026 13:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@david-chambliss-gdx david-chambliss-gdx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gmagnu
gmagnu merged commit 532af22 into main Jul 23, 2026
15 checks passed
@gmagnu
gmagnu deleted the ENGKNOW-3670-gor-rewriting-gord-fails-take-2 branch July 23, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants