fix(ENGKNOW-3657): harden DictionaryEntries read accessors against clear() races#130
Open
gmagnu wants to merge 28 commits into
Open
fix(ENGKNOW-3657): harden DictionaryEntries read accessors against clear() races#130gmagnu wants to merge 28 commits into
gmagnu wants to merge 28 commits into
Conversation
Diagnosis-first spec: local load harness + profiler to pinpoint the dominant memory consumer behind table-service OOMs under load. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… plan Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use the same exclusive-lock write path as the real Table Service so concurrent writes serialize instead of racing on a shared temp file and silently losing inserts. Log caught op errors; shutdownNow on await timeout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cache table instances so read-side DictionaryEntries stay retained for the run (models a session/table cache), and measure post-GC retained-heap delta over a baseline. Replaces the peak-churn metric that failed to scale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…driver Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
contentHashToLines, activeTags and deletedEntriesCount are only needed by the mutation/stats paths, not by select/filter reads. Building them lazily shrinks the long-lived cached dictionary copy for select-heavy workloads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keep the memory-diagnosis design spec and implementation plan as local-only files; not part of the PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keep the memory findings and profiling runbook as local-only files; not part of the PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getEntries()/isLoaded() read dataLoaded+rawLines without synchronization while a synchronized loader publishes them, so a concurrent reader could see dataLoaded==true with a stale rawLines (transient NPE/torn read). This is hit by concurrent selects sharing one cached GorDictionaryTable. Mark the lazily-published DictionaryEntries fields (and TableInfoBase.id) volatile so unsynchronized reads observe fully-constructed state; correct the class contract in the javadoc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getTableAccessOptimizer() built the optimizer via an unsynchronized null-check on a non-volatile field, so concurrent callers on a shared table could build/publish multiple instances unsafely. Use double-checked locking on a volatile field so concurrent readers share one safely-published optimizer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
inferShouldBucketizeFromContent() lazily cached the content type in a non-volatile field with an unsynchronized null-check. Make the field volatile and compute via a local; the computation is idempotent so no lock is needed, and reads now observe a safely-published value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…able-service-memory-usage
…dening Cover the getEntries()/getAllActiveTags() unmodifiable-view guarantees, the updateTagMap() reload-after-clear guard, and that insert/delete still mutate the backing list after switching to the internal loadedEntries() accessor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes 3 concurrency holes in
DictionaryEntries— the shared dictionary the table service caches across many concurrent selects.getEntries()did two separate volatile reads (dataLoaded, thenrawLines) — a racingclear()could make it returnnull(torn read).clear()looks safe because it'ssynchronized, but the unlocked fast-path reads are the trap.loadedEntries()readsrawLinesinto one local so the load check and the returned reference are the same read.updateTagMap()dereferencedrawLines.size()with no!dataLoadedguard (unlikeupdateContentMap()), sogetEntries(String...)could NPE ifclear()slipped in after itsgetEntries()call.if (!dataLoaded) loadLinesAndUpdateIndices()guard.getEntries()returned the backingArrayListandgetAllActiveTags()returned a liveMultiset.elementSet()— callers could structurally modify shared internal state (elementSet()blocksaddbut allowsremove/clear).insert/delete) and internal reads switched toloadedEntries()so the wrap doesn't break.add/.remove.Class javadoc updated to document the unmodifiable-view contract.
Tests
New
UTestDictionaryEntriesViews(deterministic):getEntries_returnsUnmodifiableList— red before fix, green aftergetAllActiveTags_returnsUnmodifiableSet— assertsclear()on the returned set throws (the real hole)insertAndDelete_stillMutateBackingList— regression for theloadedEntries()refactorgetEntriesByTag_reloadsAfterClear— Minor fixes to README #2 functional guardExisting
UTestDictionaryEntriesConcurrency(16-thread stress) still passes.modeldictionary + table suites green;gortoolscompiles.🤖 Generated with Claude Code