feat(ENGKNOW-3770): cache simple link files, instrument the rest

[gmagnu]

ENGKNOW-3770

Problem

LinkFile's link content cache had a 0% hit rate by construction. gor.driver.link.cache=true is the default, but the cache could never serve an entry, so every link file resolution re-read the link file from storage — on a bucket already returning ~9.6M 429s per 14 days.

Three facts, each verifiable:

1. The cache was Cache<StreamSource, String> — keyed on the object identity of the source.
2. No source or wrapper type overrides equals/hashCode. (FileSource.java does contain two hashCode overrides, but they belong to the inner FileSourceStream / FileSourceOutputStream classes, not to FileSource itself.)
3. PluggableGorDriver.getDataSource returns wrap(resolveDataSource(sourceReference)) — a new ExtendedRangeWrapper over a new RetryStreamSourceWrapper over a new S3Source — and handleLinks then calls LinkFile.load((StreamSource) source), performing exactly one linkCache.get(source, ...).

The key was therefore never equal to a previous one. gor.driver.link.cache.session=true was a no-op for the same reason. Two documented properties described a cache that did nothing.

Found while fixing ENGKNOW-3722. Not a regression — the cache had never hit.

Fix

The two kinds of link file differ in how they get rewritten, and that is what decides whether each can be cached.

Simple link files are now cached, keyed on the link file path, with the existing 5 minute expiry. A simple link file is a bare data path, rewritten so rarely that serving one slightly stale is an accepted trade, and it is by far the more common kind — so this is where the saving is. A path key with a bounded expiry needs no metadata validation and no extra request, which is the right shape once a staleness window is acceptable.

Versioned link files are still always re-read. They are rewritten in normal operation by appendEntry and by versioned-link GC, and a stale entry could resolve to a generation GC has already deleted — silently, rather than failing loudly.

The version comes from the content, so the decision is made after the read, on the way into the cache: LinkFileMeta.createOrLoad(content, null, false).getVersion().

Why not simply key everything on the path

For versioned links that is worse than not caching. In LinkFile.isMaxEntriesReached:

boolean maxEntriesReached = fileTooLarge || ((tooManyEntries || entryTooOld) && !protectedByMinCount && !protectedByMinAge);

fileTooLarge (content > LINK_FILE_MAX_SIZE, default 100000) bypasses both the min-count and min-age protections (defaults: 10 entries, 2 years). That is exactly the compaction behind ENGKNOW-3722, where a link file went from ~350 KB to 43 bytes. For a link file carrying DATA_LIFECYCLE_MANAGED, checkAndGCEntries spawns a thread that deletes the superseded data urls, so a reader holding stale content can point at a file that no longer exists.

Why not key on last-modified and length

Tried during ENGKNOW-3722 and reverted, with evidence. (path, lastModified, length) looks like real validation, and four unit tests passed. The ENGKNOW-3722 integration test then caught it resolving to the pre-compaction entry after a rewrite:

.../source/versions/generation_1200.gorz     <- expected .../compacted.gorz

The key is built from source.getSourceMetadata(), which for a freshly built source resolves through the shared S3 metadata cache — the one with its own 5 minute expiry. Both reads happened seconds apart, so it returned the pre-rewrite values, the key matched, the link cache hit, and the read to S3 never happened. No read means no 416, so the ENGKNOW-3722 self-heal never fired.

Validating against a cached value is not validating: the content cache short-circuits the mechanism that repairs staleness. If versioned links are ever cached, the key has to be checked against a forced fresh HEAD, and that trade has to be measured against the 429 budget first.

Instrumentation

LinkFileCacheStats — observe-only, off unless gor.driver.link.cache.stats=true, path map capped by gor.driver.link.cache.stats.maxpaths (default 50000), per-version summary logged on JVM shutdown.

Per link file version it records reads, cache hits and distinct paths (what caching is worth), plus content changes — repeat resolutions where the content actually differed. That last number is what decides whether caching versioned links would be safe, not merely useful. Versioned links are never cached, so every resolution of one is a read and their change count is complete.

Counters are approximate under concurrency; two threads resolving the same path at once can each see the other's content as a change. That is fine for a measurement tally and not worth locking a read path over.

Three bugs the full suite caught

UTestGorWrite failed with the pre-write link target after write ... -link — dbsnp.gor where dbsnp2.gor was expected. Not the accepted staleness window: a same-process write-then-read, where the link file is loaded (and cached), appended to, saved, and read back stale.

• save() now invalidates the cached content for its link file, in both the session and the fallback cache. Another process's rewrite going unnoticed until expiry is the trade; reading back content this process just replaced is not.
• An empty link file is never cached. "" classified as simple and got cached, so a placeholder about to be written was served back empty and a later read reported version 0 instead of 1 — the second failure.
• readLimitedLinkContent releases the read handle it opened. While the cache was keyed on the source object it pinned every source handed to it, hiding the fact that callers do not close them. Unpin them and FileSource.finalize starts logging "Datasource closed via finalize method", nondeterministically, inside whichever test is capturing query output. Releasing it is transparent: FileSource.open() reopens lazily via ensureOpenForRead(), S3Source.close() is a no-op, and LinkFile.save reopens for writing.

Tests

Written test-first; each was watched failing before the fix.

• UTestLinkFileCache — a simple link file is served from cache after the file changes underneath it; a versioned one is re-read; content is keyed on the path, not on the source object; saving invalidates; an empty link file is not cached.
• UTestLinkFileCacheStats — repeat reads of one path count as one distinct path; a content change on one path is counted; simple and versioned are tallied separately; cache hits are counted; nothing is recorded unless enabled; the path map stays within its cap.
• UTestGorSessionCache — the link cache key type change, and the existing per-session isolation test.

Mutation-checked. Removing save()'s invalidation fails only savingALinkFileInvalidatesItsCachedContent; reverting the empty-content guard fails only emptyLinkFileIsNotCached.

• ./gradlew test — 2595 pass, 0 fail, 69 skipped, across two clean --rerun-tasks runs (the finalizer warning above is nondeterministic, so one green run proves little)
• ./gradlew :drivers:integrationTest for S3 + OCI — 88 pass against real object stores

Impact

Simple link files are the common case, and each resolution of one previously cost a full read from object storage. Those now come from memory for up to 5 minutes.

Still open on the ticket, deliberately: run a representative workload with the stats enabled and decide from the measured content-change count whether versioned links can be cached too. Leaving them uncached with the numbers recorded is an acceptable outcome.

🤖 Generated with Claude Code

[github-actions]

Junit Tests - Summary

4 803 tests  +15   4 631 ✅ +14   17m 8s ⏱️ +14s
  497 suites + 2     172 💤 + 1 
  497 files   + 2       0 ❌ ± 0 

Results for commit 3b3952a. ± Comparison against base commit 70f9c7d.

This pull request skips 1 test.

gorsat.UTestSignature ‑ testSignature10Seconds

♻️ This comment has been updated with latest results.

[janeliutw]

I was trying claude /code-review plugin and have codex validate the response. Here's the result:

1. model/src/main/java/org/gorpipe/gor/driver/linkfile/LinkFile.java:314 — medium

invalidateCachedContent(source) runs at the top of the private save(...), before the new content is written and — for FileSource with atomic write — before close() renames the temp file into place (FileSource.java:351-370). Any
concurrent link resolution in that window reads the pre-write file and re-populates the cache under the same path key, so the process serves the stale link for the full 5-minute expiry. That's the failure UTestGorWrite caught, just
with a thread in between.

Suggest invalidating after the write completes — in save(long, FileReader) once the try-with-resources closes the output stream, ideally in a finally so a failed write also drops the entry.

2. LinkFile.java:455 — medium

invalidateCachedContent clears staticLinkCache plus the link cache of the session bound to the saving thread only. GorSession.currentSession is an InheritableThreadLocal (GorSession.java:39) that is never cleared, so reads and
writes can legitimately land on different caches: a read on a thread carrying session A caches into A, while a write -link on a pool thread carrying no session (or session B) invalidates static/B. A's entry survives, and a later
read on an A-thread gets content this same process already replaced.

Suggest invalidating across all live session caches, or keying invalidation so it can't be missed (e.g. a per-path write generation).

3. LinkFile.java:433 — nit (defensive hardening)

The cache key is source.getFullPath(), and FileSource.getFullPath() returns filePath.toString() (FileSource.java:178-180), which is the raw url when the SourceReference carries no common root (FileSource.java:94-99).

Not reachable through normal resolution: DriverBackedFileReader never leaves commonRoot empty — it falls back to the standalone root or DEFAULT_COMMON_ROOT = "./" (DriverBackedFileReader.java:68, :86-93) and always threads it
through createSourceReference (:111-114). Two genuinely different roots therefore produce distinct absolute keys (Paths.get("/root/a/", "user_data/x.gorz.link") → /root/a/user_data/x.gorz.link); the key stays relative only under
the default ./, which implies a single process CWD and so a single file.

The residual is the constructors that build a SourceReference with no root at all — FileSource.java:67 and :74, GorIndexFile.java:85, IndexCommand.java:146-148, GorBench.java:111 — where getFullPath() returns the url verbatim into
the process-wide staticLinkCache. Those aren't the normal .link path, so this is hardening rather than a bug: normalizing to an absolute path or URI before caching closes it cheaply.

4. model/src/main/java/org/gorpipe/gor/driver/linkfile/LinkFileCacheStats.java:148 — low

registerDump() calls Runtime.getRuntime().addShutdownHook(...) from the link-read path. If the first stats-enabled resolution happens while the JVM is already shutting down (inside a shutdown hook, or on the GC thread from
checkAndGCEntries still running as the JVM exits), addShutdownHook throws IllegalStateException("Shutdown in progress"), which propagates out of LinkFile.loadContentFromSource and fails the read. An observe-only tally should never
be able to fail a read — wrap the registration in try/catch.

5. LinkFileCacheStats.java:55-62 — low

TrackedPath.version is final and set at first sight; only contentFingerprint is updated on change (:99-105). When a link is converted from simple to versioned (LinkUpdateCommand does exactly this via LinkFile.loadV1 + save), the
tracked version stays stale.

Reads are not affected — recordRead derives its bucket fresh via versionOf(content) (:94-96), so reads and contentChanges land on the current version. The stale field is read in two places:

• recordCacheHit → counters(tracked != null ? tracked.version : LinkFileV0.VERSION).hits (:121) — hits on a converted path keep landing in the v0 bucket.
• summary → filter(t -> t.version.equals(version)).count() (:125) — distinctPaths counts the path under its original version.

Net effect on the number this tally exists to inform: after a conversion, reads move to v1 while hits stay on v0, skewing the per-version hit rate in both directions. Fix is one line — make version non-final/volatile and update it
alongside contentFingerprint in recordRead.

[gmagnu]

Went through all five. Four fixed in 3b3952a, one declined — reasoning below.

1. invalidateCachedContent before the write — fixed. Confirmed: it ran at the top of the private save(...), and for FileSource the atomic rename happens on close(), which is the end of the try-with-resources in save(long, FileReader). Moved to a finally there, so it runs after the rename and also on a failed write.

2. Invalidation misses other sessions' caches — fixed. Confirmed: currentSession is set in the GorSession constructor, GorOptions:476 and BaseScriptExecutionEngine:257, and nothing ever clears it.

I went with the write-generation option rather than walking every live session cache, because a session registry does not close the whole hole: a resolution already in flight can put its pre-write content into a cache after the invalidate, whichever caches the invalidate reached. So a save now stamps the path in a bounded, expiring linkWriteMarks cache, and a cached entry is dropped on lookup if its read started before the stamp. Entries carry the time their read started, not when they were cached — a read that opened its stream before the file was replaced holds the old content even though it finished after. The marks are bounded and expire like the content caches, so a mark outlives every entry it has to retire, and no cache has to be reachable from the saving thread.

3. Normalising the cache key — declining, for now. Your own analysis is what convinced me: DriverBackedFileReader never leaves commonRoot empty, so this is unreachable on the .link path, and the residual is the rootless SourceReference constructors that are not link resolution. Against that, normalising costs a Paths.get(...).toAbsolutePath() on every resolution of the path this cache exists to make cheap, and Paths.get on s3://bucket/... gives a stable-but-nonsense key rather than a cleaner one. If you would rather have it anyway, say so and I will add it — I just did not want to pay it on the hot path for a case that cannot currently be reached.

4. addShutdownHook from the read path — fixed. Wrapped in try/catch on IllegalStateException. An observe-only tally must never be able to fail the read it is counting.

5. Stale TrackedPath.version — fixed. version is now volatile and updated alongside the fingerprint, so hits follow a converted link into its new bucket the way reads already did.

Regression test per fix, and each one confirmed red against the pre-fix code:

• contentReadWhileASaveWasRunningIsNotLeftInTheCache — hooks FileSource.open() so a save lands after the read has opened its stream. The read returns the pre-write content and caches it after the save has dropped what it could reach; a later read must not see it. Covers 1 and the in-flight half of 2.
• savingOnAnotherSessionsThreadInvalidatesCachedContent — read on a thread carrying session A, save on a thread carrying session B.
• hitsFollowThePathsCurrentVersion, readsSurviveAJvmThatIsAlreadyShuttingDown.

Full suite green (2595 unit tests plus :gortools:testScala).

[cesarvp-gdx]

LGTM!

[janeliutw]

Looks good, thanks for addressing.