fix(#5915 J2 slice-3): deadref/cache/tier/cleanup segment-set gates + restore EmbeddingRef on FB load#5922
Merged
Merged
Conversation
…3 site 1, data loss) refGraphMtime/recentlyIndexed in deadref.go resolved freshness only via os.Stat(graph.CurrentGraphPath(refDir)), a flat-.fb-only path absent for a segment-set ref (graph.<gen>/ dir + manifest.json). A freshly-indexed segment-set ref read as mtime-zero, fell outside the grace window, and the reaper could delete a valid cold ref. Add graph.CurrentGraphMtime (internal/graph/genpath.go): single-file resolves the .fb mtime (unchanged); segment-set resolves the gen dir's manifest.json mtime. internal/daemon cannot import groupalgo's existing unexported graphSourceMtime (cycle), so the helper lives in internal/graph; groupalgo.graphSourceMtime now delegates to it. Tests: TestCurrentGraphMtime_SegmentSet/SingleFileParity/Absent; TestRecentlyIndexed_SegmentSet, TestRefGraphMtime_SegmentSet, and the load-bearing TestDeadRef_segmentSetRefSurvivesGraceWindow (dead-in-git segment-set ref indexed within grace survives a full Sweep), plus TestRecentlyIndexed_SingleFileParity. Verified RED before the fix, GREEN after.
…#5915 J2 slice-3 site 2) readFromDisk/writeToDisk resolved graph freshness via os.Stat(graph.CurrentGraphPath(stateDir)), a flat-.fb-only path absent for a segment-set state dir (graph.<gen>/ dir + manifest.json, no flat .fb). That made the disk cache permanently unusable for a segment-set repo: readFromDisk always missed (forcing recompute on every Get) and writeToDisk always errored (the cache was never persisted). Switch both to graph.CurrentGraphMtime (added in the deadref.go fix, internal/graph/genpath.go): single-file resolution is byte-identical to before; segment-set resolves the gen dir's manifest.json mtime. Tests: TestCacheMissComputesAndPersists_SegmentSet, TestCacheHitServesFromDisk_SegmentSet, TestStaleCacheRecomputesWhenSegmentSetGenAdvances (a reindex that flips current to a new gen invalidates the cache). Verified RED before the fix (2 of 3 failed), GREEN after; existing single-file tests unaffected.
…ce-3 site 3) inferTierFromDisk stat'd only [graph.CurrentGraphPath(stateDir), graph.json] to determine a ref's tier/lastSeen — a flat-.fb-only gate absent for a segment-set ref (graph.<gen>/ dir + manifest.json, no flat .fb). A freshly-indexed segment-set ref therefore inferred tier cold with a zero lastSeen. Switch to graph.CurrentGraphMtime (internal/graph/genpath.go), preserving the existing max-with-graph.json comparison. Single-file resolution is byte-identical to before. Tests: TestInferTierFromDisk_SegmentSet (RED before, GREEN after), TestInferTierFromDisk_SingleFileParity, TestInferTierFromDisk_NeverIndexed. Verified RED before the fix (tier=cold, lastSeen=zero for a 1-minute-old segment-set ref), GREEN after.
… slice-3, site 4) collectActiveEmbeddingHashes walked the daemon store looking for graph.IsGraphFileName matches (flat graph.fb or gen-file graph.<gen>.fb), then guarded against double-processing a ref's state dir with 'path != graph.CurrentGraphPath(filepath.Dir(path))'. Neither recognizes a segment-set ref (graph.<gen>/ dir of seg-NNNN.fb + manifest.json, no flat .fb): seg-NNNN.fb matches neither IsGraphFileName pattern, so the walk silently skipped a live segment-set ref's state dir entirely — its entities' embedding_ref values were never added to the 'active' set fed into the embedding-cache TTL sweep (grafel cleanup), so a still-referenced .vec cache entry for a segment-set repo could be reaped purely by age. Fix: recognize filepath.Base(path) == graph.ManifestFileName as an additional segment-set marker inside the walk, then validate via graph.CurrentGraphDescriptor(candidateStateDir) that the manifest is in fact the CURRENT generation's manifest (not a stale/previous gen left on disk) before treating candidateStateDir as a ref to load. Single-file / legacy flat behavior is unchanged byte-for-byte: the existing IsGraphFileName + CurrentGraphPath dedup branch is untouched. Note on scope, precisely: the walk callback never deletes graph files itself — collectActiveEmbeddingHashes only builds the 'active' hash set that a SEPARATE embed.Cache.Sweep(active, ttlDays) call consults to decide which .vec files to remove. So the observable effect of this bug was never 'a live segment-set's segments/manifest get deleted' (nothing in this walk deletes graph files); it was 'a live segment-set repo's embedding cache entries can be swept as stale despite being referenced,' which is what the new tests assert against. Also fixes a separate, pre-existing bug surfaced while building the segment-set test fixture: internal/graph/load.go's fbEntityToGraphEntity (the FB->Document conversion shared by both the single-file and segment-set loaders) never restored an entity's EmbeddingRef (PH8 #2100) from its FlatBuffers slot, even though fbwriter correctly persists it. Every FB-backed load — single-file or segment-set — silently returned EmbeddingRef == "" regardless of what was written, which is why even the SINGLE-FILE parity test for this walk initially failed RED alongside the segment-set test. Purely additive (EmbeddingRef was always the zero value before), covered by a new dedicated regression test in internal/graph/load_test.go (TestLoadGraphFromDir_EmbeddingRefRoundTrip). Tests: internal/cli/cleanup_segmentset_test.go adds TestCollectActiveEmbeddingHashes_SegmentSet (RED before the cleanup.go fix: active map came back empty for a live segment-set ref) and TestCollectActiveEmbeddingHashes_SingleFileParity (guards the existing flat/gen-file resolution is unchanged). Verified isolated RED for the walk-routing fix alone (load.go's EmbeddingRef fix kept, cleanup.go's fix reverted): segment-set test fails, single-file parity test passes — the two bugs are cleanly separated. gofmt -l clean; go vet and go test green across internal/cli, internal/graph, internal/daemon, internal/daemon/algo, internal/embed. Part of #5915 J2 slice-3 (site 4 of 4).
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
2 participants
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.
What
Remaining non-dashboard flat-
graph.fb-only gates made segment-set-aware (#5915 J2 slice 3), plus a pre-existing shared-load-path bug found while building the fixtures.New shared helper
graph.CurrentGraphMtime(dir)(single-file →.fbmtime; segment-set →manifest.jsonmtime, the atomic commit point);groupalgo.graphSourceMtimedelegates to it.Sites
internal/daemon/deadref.go— DATA LOSS.refGraphMtime/recentlyIndexedstat'd only the flat path → a freshly-indexed segment-set ref read mtime-0 → fell outside the GC grace window → premature reaping of a valid cold ref. Now grace-protected viaCurrentGraphMtime.internal/daemon/algo/cache.go— staleness mtime.internal/cli/branches.go—inferTierFromDisk= max(CurrentGraphMtime, graph.json mtime).internal/cli/cleanup.go— embedding-active-set walk now recognizes a live segment-set gen (viaCurrentGraphDescriptor), so its.vecrefs are marked active and not reaped by age.Bonus fix —
Entity.EmbeddingRefdropped on FB load (Closes #5921)internal/graph/load.gofbEntityToGraphEntity(shared by single-file + segment-set + the mmapMaterializeEntityserve path) never read theEmbeddingRefFB slot → every FB-backed load returned"". Restored additively + round-trip test. Narrow impact:grafel cleanupcould reap still-referenced.vecentries by age (no semantic-search path reads the field).Verification (independent adversarial review — APPROVE)
TestDeadRef_segmentSetRefSurvivesGraceWindowRED (ref reaped in grace); fix doesn't over-protect (old single-file ref past grace still reaped).CurrentGraphMtime+1ns drift → RED (single-file byte-parity); no import cycle; groupalgo delegation preserves slice-1.Closes #5921
Refs #5915
🤖 Generated with Claude Code