diff --git a/internal/dashboard/graphstate.go b/internal/dashboard/graphstate.go index b820d5339..bf3f2fcfb 100644 --- a/internal/dashboard/graphstate.go +++ b/internal/dashboard/graphstate.go @@ -25,6 +25,7 @@ import ( "github.com/cajasmota/grafel/internal/daemon" "github.com/cajasmota/grafel/internal/graph" + "github.com/cajasmota/grafel/internal/graph/descriptions" "github.com/cajasmota/grafel/internal/graph/fbreader" "github.com/cajasmota/grafel/internal/registry" "github.com/cajasmota/grafel/internal/types" @@ -637,6 +638,19 @@ func (c *GraphCache) loadGroupForRef(groupName, ref string) (*DashGroup, error) }) } dr.Doc = doc + // Description side-table (#5904 PR-a): ADDITIVELY stamp any + // /descriptions.json onto this repo's entities so the + // PropGet("description") readers (openapi/DAG export) surface agent + // write-back descriptions again now that write-back no longer rewrites + // the graph. Absence/corrupt/stale-tolerant: a miss leaves the entity's + // own (extractor-native) description intact. + if sc, ok := descriptions.Read(stateDir); ok { + for i := range doc.Entities { + if d, has := sc.Results[doc.Entities[i].ID]; has { + doc.Entities[i].PropSet("description", d) + } + } + } // S8 (#2159): open a zero-copy mmap reader alongside the Document // so handlers that only need to iterate entities/relationships can // avoid materialising the full heap slice. Best-effort: failures diff --git a/internal/dashboard/handlers_enrichment_writeback.go b/internal/dashboard/handlers_enrichment_writeback.go index 8d38feceb..6d54a41ae 100644 --- a/internal/dashboard/handlers_enrichment_writeback.go +++ b/internal/dashboard/handlers_enrichment_writeback.go @@ -41,7 +41,7 @@ import ( "github.com/cajasmota/grafel/internal/daemon" "github.com/cajasmota/grafel/internal/graph" - "github.com/cajasmota/grafel/internal/graph/fbwriter" + "github.com/cajasmota/grafel/internal/graph/descriptions" ) // ───────────────────────────────────────────────────────────────────────────── @@ -193,49 +193,43 @@ func (s *Server) handleEnrichmentWriteback(w http.ResponseWriter, r *http.Reques now := time.Now().UTC() enrichedAt := now.Format(time.RFC3339) - // ─── Target 1: update in-memory graph property ──────────────────────── + // ─── Target 1: description SIDE-TABLE upsert (no graph rewrite) ──────── + // #5904 PR-a: the description is written to the per-repo + // /descriptions.json side-table and merged back onto entities at + // READ time (an additive PropSet of "description" — see descriptions.Read / + // applyDescriptionOverlay). The graph file / generation is NEVER touched. + // + // The previous implementation rewrote the WHOLE graph (fbwriter.WriteGraphGen + // + graph.WriteAtomic on graph.json). For a #5901 segment-set that resident + // Document is a COLLAPSED union of every segment, so re-serialising it as one + // flat graph..fb re-materialised the entire graph in memory (the #5915 P1 + // OOM) and discarded the segmented layout. The side-table removes that hazard: + // a segment-set stays a segment-set. + // + // The in-memory PropSet below keeps the CURRENTLY-resident group visibly + // enriched (so a read racing the Invalidate still sees it); durability comes + // from the sidecar, which the next load / serving-path re-check merges back. if found.entity.PropLen() == 0 { found.entity.PropsReplace(make(map[string]string)) } found.entity.PropSet("description", req.Description) - // Persist the updated graph to disk — both graph.fb (canonical binary, - // preferred by LoadGraphFromDir / MCP) and graph.json (backward-compatible - // JSON, read by debug tools and external integrations). Writing only one - // format leaves the other stale, causing ghost entity IDs for direct - // graph.json readers (fixes #1702). stateDir := daemon.StateDirForRepo(found.repoPath) + // graphPath is informational only (echoed in the response); it is NOT written. graphPath := daemon.GraphPathForRepo(found.repoPath) - // #5891: write a NEW graph..fb + flip the `current` pointer rather - // than overwriting graph.fb — this writeback path is executed by the serve - // process while the same graph.fb may be mmap'd, exactly the Windows - // ERROR_USER_MAPPED_FILE hazard the gen layout removes. fbPath is the gen - // file written (used for the identical-mtime stamp below). - fbPath, ferr := fbwriter.WriteGraphGen(stateDir, found.doc) - if ferr != nil { + if err := descriptions.Upsert(stateDir, subjectID, req.Description); err != nil { s.auditor.Err("enrichment_writeback", group, map[string]any{ "subject_id": subjectID, "kind": req.Kind, - }, "write graph.fb: "+ferr.Error()) - writeErr(w, http.StatusInternalServerError, "persist graph.fb: "+ferr.Error()) + }, "write descriptions side-table: "+err.Error()) + writeErr(w, http.StatusInternalServerError, "persist description side-table: "+err.Error()) return } - if err := graph.WriteAtomic(graphPath, found.doc, false); err != nil { - s.auditor.Err("enrichment_writeback", group, map[string]any{ - "subject_id": subjectID, - "kind": req.Kind, - }, "write graph.json: "+err.Error()) - writeErr(w, http.StatusInternalServerError, "persist graph.json: "+err.Error()) - return - } - // Stamp identical mtime on both files so the on-disk pair is never - // mistaken for a partial write (#1626 pattern). - writeNow := time.Now() - _ = os.Chtimes(fbPath, writeNow, writeNow) - _ = os.Chtimes(graphPath, writeNow, writeNow) - // Invalidate the cache so the next GET re-reads the updated graph. + // Invalidate the cache so the next GET re-loads the group and re-merges the + // side-table (applyDescriptionOverlay / the loadGroup merge) onto fresh + // entities — the durable read path for the write we just made. s.graphs.Invalidate(group) // ─── Target 2: write YAML-frontmatter Markdown doc ──────────────────── diff --git a/internal/dashboard/handlers_enrichment_writeback_test.go b/internal/dashboard/handlers_enrichment_writeback_test.go index 01c1ef0b6..f13d6a0ab 100644 --- a/internal/dashboard/handlers_enrichment_writeback_test.go +++ b/internal/dashboard/handlers_enrichment_writeback_test.go @@ -18,6 +18,7 @@ import ( "github.com/cajasmota/grafel/internal/daemon" "github.com/cajasmota/grafel/internal/graph" + "github.com/cajasmota/grafel/internal/graph/descriptions" ) // ───────────────────────────────────────────────────────────────────────────── @@ -198,7 +199,20 @@ func TestWriteback_success(t *testing.T) { t.Errorf("entity.Properties[description]: want %q, got %q", desc, entity.PropGet("description")) } - // ── 3. graph.json updated on disk ───────────────────────────────────── + // ── 3. description lands in the side-table, graph NOT rewritten ──────── + // #5904 PR-a: the description is persisted to /descriptions.json, + // and the graph file is NEVER rewritten (no collapse / OOM for a segment-set). + stateDir := daemon.StateDirForRepo(repoDir) + sc, ok := descriptions.Read(stateDir) + if !ok { + t.Fatalf("descriptions side-table not found / not fresh at %s", descriptions.Path(stateDir)) + } + if got := sc.Results["aabbccddeeff0011"]; got != desc { + t.Errorf("side-table description: want %q, got %q", desc, got) + } + + // The on-disk graph.json must be UNCHANGED — the write-back must not bake the + // description into the graph (that is the whole-graph rewrite this PR removes). graphPath := daemon.GraphPathForRepo(repoDir) raw, err := os.ReadFile(graphPath) if err != nil { @@ -208,18 +222,11 @@ func TestWriteback_success(t *testing.T) { if err := json.Unmarshal(raw, &savedDoc); err != nil { t.Fatalf("unmarshal graph.json: %v", err) } - found := false for _, e := range savedDoc.Entities { - if e.ID == "aabbccddeeff0011" { - found = true - if e.PropGet("description") != desc { - t.Errorf("persisted description: want %q, got %q", desc, e.PropGet("description")) - } + if e.ID == "aabbccddeeff0011" && e.PropGet("description") != "" { + t.Errorf("graph.json must NOT be rewritten with the description; got %q", e.PropGet("description")) } } - if !found { - t.Error("entity not found in persisted graph.json") - } // ── 4. Markdown doc file created ───────────────────────────────────── docPath := filepath.Join(repoDir, "docs", "enrichments", "http_endpoint", "aabbccddeeff0011.md") diff --git a/internal/graph/descriptions/atomicrename_other.go b/internal/graph/descriptions/atomicrename_other.go new file mode 100644 index 000000000..163ccccb5 --- /dev/null +++ b/internal/graph/descriptions/atomicrename_other.go @@ -0,0 +1,16 @@ +//go:build !windows + +package descriptions + +import "os" + +// atomicRename performs the temp→dest swap that finalizes a sidecar write. +// +// On Unix, os.Rename over an existing destination is a single atomic syscall +// even while concurrent readers hold the destination open (open files are +// referenced by inode, not by directory entry), so one attempt is correct. The +// retrying variant lives in atomicrename_windows.go, where renaming over a file +// a reader still has open can transiently fail with a sharing violation. +func atomicRename(tmp, dst string) error { + return os.Rename(tmp, dst) +} diff --git a/internal/graph/descriptions/atomicrename_windows.go b/internal/graph/descriptions/atomicrename_windows.go new file mode 100644 index 000000000..e796e61a6 --- /dev/null +++ b/internal/graph/descriptions/atomicrename_windows.go @@ -0,0 +1,62 @@ +//go:build windows + +package descriptions + +import ( + "errors" + "io/fs" + "os" + "syscall" + "time" + + "golang.org/x/sys/windows" +) + +// atomicRenameRetries / atomicRenameRetryDelay bound the Windows rename-over +// retry loop. On Windows, rename-over-existing fails with +// ERROR_SHARING_VIOLATION / ERROR_ACCESS_DENIED while ANY process still holds +// the destination open — including a concurrent reader mid os.ReadFile. The +// sidecar's real reader opens, reads, and closes the file in a single brief +// operation, so a short bounded backoff rides out that window. Variables (not +// consts) so a future test can shrink the delay. +var ( + atomicRenameRetries = 40 + atomicRenameRetryDelay = 5 * time.Millisecond +) + +// atomicRename performs the temp→dest swap that finalizes a sidecar write, +// retrying on the transient Windows sharing/access violation raised when a +// concurrent reader still has the destination open. A persistent lock still +// surfaces the genuine error after the bounded loop exhausts. +func atomicRename(tmp, dst string) error { + var err error + for i := 0; i < atomicRenameRetries; i++ { + err = os.Rename(tmp, dst) + if err == nil { + return nil + } + if !isSharingOrAccessError(err) { + return err + } + time.Sleep(atomicRenameRetryDelay) + } + return err +} + +// isSharingOrAccessError reports whether err is the transient Windows lock +// raised when renaming over a destination another handle still has open. +func isSharingOrAccessError(err error) bool { + if errors.Is(err, fs.ErrPermission) { + return true + } + var errno syscall.Errno + if errors.As(err, &errno) { + switch errno { + case windows.ERROR_ACCESS_DENIED, + windows.ERROR_SHARING_VIOLATION, + windows.ERROR_LOCK_VIOLATION: + return true + } + } + return false +} diff --git a/internal/graph/descriptions/descriptions.go b/internal/graph/descriptions/descriptions.go new file mode 100644 index 000000000..35fdafb32 --- /dev/null +++ b/internal/graph/descriptions/descriptions.go @@ -0,0 +1,195 @@ +// Package descriptions implements the per-repo DESCRIPTION side-table (#5904 PR-a). +// +// # Why this exists +// +// Dashboard enrichment write-back historically persisted an agent-generated +// entity description by mutating the in-memory graph and REWRITING the whole +// graph to disk (fbwriter.WriteGraphGen + graph.WriteAtomic). For a single-file +// graph that is merely wasteful; for a #5901 SEGMENT-SET graph it is a +// correctness hazard: the resident Document is a COLLAPSED union of every +// segment, so re-serialising it as one flat graph..fb re-materialises the +// entire graph in memory (the #5915 P1 OOM) and silently discards the segmented +// layout. +// +// The side-table removes the whole-graph rewrite from the description path, +// mirroring the proven group-algo overlay (internal/graph/groupalgo/overlay.go). +// A description is written to a small per-repo sidecar: +// +// /descriptions.json +// +// and merged back onto entities at READ time (an additive PropSet of +// "description"). The graph file / generation is never touched, so a segment-set +// stays a segment-set and no collapse/OOM can occur. +// +// # Schema + staleness +// +// The sidecar records a SOURCE-FRESHNESS KEY derived from +// graph.CurrentGraphDescriptor(stateDir) at write time (NOT CurrentGraphPath — +// a segment-set's freshness must derive from the gen dir / manifest, the #5915 +// P2 discipline). A read whose stored source_key no longer equals the current +// key is STALE and treated as absent: the entity keeps whatever it already +// carried (extractor-native description, or a description baked in before this +// change). This makes reads absence-, corruption-, and staleness-tolerant — a +// missing entry never clears an existing description. +package descriptions + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "time" + + "github.com/cajasmota/grafel/internal/graph" +) + +// FileName is the fixed basename of the per-repo description sidecar. +const FileName = "descriptions.json" + +// currentVersion is the on-disk schema version stamped into every write. +const currentVersion = 1 + +// Sidecar is the on-disk /descriptions.json document. +type Sidecar struct { + // Version is the on-disk schema version. + Version int `json:"version"` + // ComputedAt is when the sidecar was last written (informational). + ComputedAt time.Time `json:"computed_at"` + // SourceKey is the graph-freshness key (see CurrentSourceKey) at write time. + // A read whose SourceKey differs from the current key is stale → ignored. + SourceKey string `json:"source_key"` + // Results maps entity id → its agent-generated description. + Results map[string]string `json:"results"` +} + +// Path returns the sidecar path for a repo state dir. +func Path(stateDir string) string { + return filepath.Join(stateDir, FileName) +} + +// CurrentSourceKey computes the graph-freshness key for a repo state dir from +// its ACTIVE graph descriptor. The key changes whenever the underlying graph +// changes (a reindex writes a new generation / segment-set), which is exactly +// when previously-written descriptions must be considered stale. +// +// - GraphSingleFile — key derives from the resolved .fb file's mtime. +// - GraphSegmentSet — key derives from the gen dir + its manifest mtime (NOT a +// collapsed single-file path; the #5915 P2 discipline). +// - GraphAbsent — a JSON-only repo (no graph.fb): fall back to graph.json's +// mtime so JSON-only repos still get proper staleness. If nothing exists, +// the key is "" (which is stable and matches itself — descriptions written +// for a graph-less repo simply never go stale on their own). +func CurrentSourceKey(stateDir string) string { + desc, err := graph.CurrentGraphDescriptor(stateDir) + if err != nil { + // A corrupt/hostile segment-set surfaces an error; refuse to derive a key + // (callers treat "" as "no fresh graph" and the sidecar stays inert). + return "" + } + switch desc.Kind { + case graph.GraphSingleFile: + if fi, statErr := os.Stat(desc.Path); statErr == nil { + return fmt.Sprintf("single:%s:%d", filepath.Base(desc.Path), fi.ModTime().UnixNano()) + } + return "" + case graph.GraphSegmentSet: + // A segment-set's freshness derives from the gen dir / manifest, never a + // re-materialised single-file path. + manifest := filepath.Join(desc.GenDir, graph.ManifestFileName) + if fi, statErr := os.Stat(manifest); statErr == nil { + return fmt.Sprintf("seg:%s:%d", filepath.Base(desc.GenDir), fi.ModTime().UnixNano()) + } + if fi, statErr := os.Stat(desc.GenDir); statErr == nil { + return fmt.Sprintf("seg:%s:%d", filepath.Base(desc.GenDir), fi.ModTime().UnixNano()) + } + return "" + default: // GraphAbsent + if fi, statErr := os.Stat(filepath.Join(stateDir, "graph.json")); statErr == nil { + return fmt.Sprintf("json:%d", fi.ModTime().UnixNano()) + } + return "" + } +} + +// readRaw loads and unmarshals the sidecar at stateDir WITHOUT the staleness +// check. Returns nil on absent / unreadable / corrupt. Used by the upsert +// read-modify-write, which discards stale Results itself. +func readRaw(stateDir string) *Sidecar { + data, err := os.ReadFile(Path(stateDir)) + if err != nil { + return nil + } + var sc Sidecar + if err := json.Unmarshal(data, &sc); err != nil { + return nil + } + return &sc +} + +// Read loads the sidecar for stateDir and returns (sidecar, true) ONLY when it +// is present, well-formed, and NON-stale (its stored source_key still equals the +// current graph key). Absent, corrupt, and stale all collapse to (nil, false) so +// the apply path is absence-tolerant — a miss never clears an entity's existing +// description. +func Read(stateDir string) (*Sidecar, bool) { + sc := readRaw(stateDir) + if sc == nil { + return nil, false + } + if sc.SourceKey != CurrentSourceKey(stateDir) { + return nil, false // stale: written for a different graph generation + } + if len(sc.Results) == 0 { + return nil, false + } + return sc, true +} + +// Upsert writes description for entityID into the sidecar via a read-modify-write +// and an atomic tmp+rename. The stored source_key is (re)stamped to the CURRENT +// graph key; if the existing sidecar was written for a different (older) graph +// generation its stale Results are discarded and the upsert starts fresh. The +// graph file / generation is NEVER touched. +func Upsert(stateDir, entityID, description string) error { + key := CurrentSourceKey(stateDir) + sc := readRaw(stateDir) + if sc == nil || sc.SourceKey != key { + sc = &Sidecar{Results: map[string]string{}} + } + if sc.Results == nil { + sc.Results = map[string]string{} + } + sc.Version = currentVersion + sc.SourceKey = key + sc.ComputedAt = time.Now().UTC() + sc.Results[entityID] = description + return WriteTo(Path(stateDir), sc) +} + +// WriteTo atomically writes the sidecar to path via a temp-file + rename +// (single-syscall swap on Unix; a bounded sharing-violation retry on Windows, +// atomicrename_windows.go). The sidecar is never memory-mapped, so this is safe +// on every platform. A nil sidecar is a no-op. +func WriteTo(path string, sc *Sidecar) error { + if sc == nil { + return nil + } + data, err := json.Marshal(sc) + if err != nil { + return fmt.Errorf("marshal descriptions: %w", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return fmt.Errorf("mkdirall: %w", err) + } + // Temp file in the SAME directory so os.Rename is an atomic intra-filesystem + // swap; a pid suffix avoids two concurrent writers clobbering each other. + tmp := fmt.Sprintf("%s.tmp.%d", path, os.Getpid()) + if err := os.WriteFile(tmp, data, 0o644); err != nil { + return fmt.Errorf("write tmp: %w", err) + } + if err := atomicRename(tmp, path); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("rename: %w", err) + } + return nil +} diff --git a/internal/graph/descriptions/descriptions_test.go b/internal/graph/descriptions/descriptions_test.go new file mode 100644 index 000000000..c0d803714 --- /dev/null +++ b/internal/graph/descriptions/descriptions_test.go @@ -0,0 +1,251 @@ +package descriptions_test + +import ( + "encoding/json" + "os" + "path/filepath" + "sort" + "testing" + + "github.com/cajasmota/grafel/internal/graph" + "github.com/cajasmota/grafel/internal/graph/descriptions" + "github.com/cajasmota/grafel/internal/graph/fbwriter" +) + +// writeSingleFileGen writes graph.1.fb + points current at it, returning stateDir. +func writeSingleFileGen(t *testing.T, dir string) { + t.Helper() + doc := &graph.Document{Repo: "r", Entities: []graph.Entity{ + {ID: "e1", QualifiedName: "p.A", Kind: "function", Name: "A"}, + {ID: "e2", QualifiedName: "p.B", Kind: "struct", Name: "B"}, + }} + if err := fbwriter.WriteAtomic(filepath.Join(dir, graph.GenFileName(1)), doc); err != nil { + t.Fatal(err) + } + if err := graph.WriteCurrentPointer(dir, graph.GenFileName(1)); err != nil { + t.Fatal(err) + } +} + +// writeSegmentSet hand-builds a multi-segment generation on disk (mirrors the +// graph package's writeSegmentSet test helper). +func writeSegmentSet(t *testing.T, dir string, gen uint64) { + t.Helper() + genDirName := graph.GenDirName(gen) + genDir := filepath.Join(dir, genDirName) + if err := os.MkdirAll(genDir, 0o755); err != nil { + t.Fatal(err) + } + docs := []*graph.Document{ + {Repo: "seg", Entities: []graph.Entity{ + {ID: "a", QualifiedName: "p.A", Kind: "function", Name: "A"}, + {ID: "b", QualifiedName: "p.B", Kind: "struct", Name: "B"}, + }}, + {Repo: "seg", Entities: []graph.Entity{ + {ID: "m", QualifiedName: "p.M", Kind: "function", Name: "M"}, + {ID: "n", QualifiedName: "p.N", Kind: "struct", Name: "N"}, + }}, + } + m := &graph.Manifest{FormatVersion: graph.ManifestFormatVersion} + for i, doc := range docs { + name := graph.SegmentFileName(i) + if err := fbwriter.WriteAtomic(filepath.Join(genDir, name), doc); err != nil { + t.Fatalf("write %s: %v", name, err) + } + seg := graph.SegmentMeta{ + File: name, + Kind: graph.SegmentEntities, + EntityCount: len(doc.Entities), + } + ids := make([]string, 0, len(doc.Entities)) + for _, e := range doc.Entities { + ids = append(ids, e.ID) + } + sort.Strings(ids) + seg.MinKey, seg.MaxKey = ids[0], ids[len(ids)-1] + m.Segments = append(m.Segments, seg) + } + if err := graph.WriteManifest(genDir, m); err != nil { + t.Fatalf("write manifest: %v", err) + } + if err := graph.WriteCurrentPointerRaw(dir, genDirName); err != nil { + t.Fatalf("write current pointer: %v", err) + } +} + +// TestUpsertRead_SingleFileRoundtrip: an upserted description reads back on a +// single-file graph. +func TestUpsertRead_SingleFileRoundtrip(t *testing.T) { + dir := t.TempDir() + writeSingleFileGen(t, dir) + + if err := descriptions.Upsert(dir, "e1", "the first entity"); err != nil { + t.Fatalf("Upsert: %v", err) + } + sc, ok := descriptions.Read(dir) + if !ok { + t.Fatal("Read returned not-ok for a fresh sidecar") + } + if got := sc.Results["e1"]; got != "the first entity" { + t.Errorf("Results[e1] = %q, want %q", got, "the first entity") + } + if sc.SourceKey == "" { + t.Error("SourceKey should be non-empty for a single-file graph") + } +} + +// TestUpsert_MultipleEntities: read-modify-write preserves prior entries. +func TestUpsert_MultipleEntities(t *testing.T) { + dir := t.TempDir() + writeSingleFileGen(t, dir) + if err := descriptions.Upsert(dir, "e1", "first"); err != nil { + t.Fatal(err) + } + if err := descriptions.Upsert(dir, "e2", "second"); err != nil { + t.Fatal(err) + } + sc, ok := descriptions.Read(dir) + if !ok { + t.Fatal("Read not-ok") + } + if sc.Results["e1"] != "first" || sc.Results["e2"] != "second" { + t.Errorf("expected both entries, got %+v", sc.Results) + } +} + +// TestUpsert_Overwrite: upserting the same id overwrites its description. +func TestUpsert_Overwrite(t *testing.T) { + dir := t.TempDir() + writeSingleFileGen(t, dir) + _ = descriptions.Upsert(dir, "e1", "old") + _ = descriptions.Upsert(dir, "e1", "new") + sc, ok := descriptions.Read(dir) + if !ok || sc.Results["e1"] != "new" { + t.Errorf("want new, got %+v (ok=%v)", sc, ok) + } +} + +// TestUpsertRead_SegmentSetRoundtrip: the sidecar works over a segment-set and +// its source key is segment-derived (never a collapsed single-file path). +func TestUpsertRead_SegmentSetRoundtrip(t *testing.T) { + dir := t.TempDir() + writeSegmentSet(t, dir, 3) + + if err := descriptions.Upsert(dir, "a", "entity a in seg 0"); err != nil { + t.Fatalf("Upsert: %v", err) + } + sc, ok := descriptions.Read(dir) + if !ok { + t.Fatal("Read not-ok for segment-set sidecar") + } + if sc.Results["a"] != "entity a in seg 0" { + t.Errorf("Results[a] = %q", sc.Results["a"]) + } + if len(sc.SourceKey) < 4 || sc.SourceKey[:4] != "seg:" { + t.Errorf("segment-set SourceKey = %q, want seg: prefix", sc.SourceKey) + } +} + +// TestRead_Absent: no sidecar → (nil,false), never a crash. +func TestRead_Absent(t *testing.T) { + dir := t.TempDir() + writeSingleFileGen(t, dir) + if sc, ok := descriptions.Read(dir); ok || sc != nil { + t.Errorf("absent sidecar: want (nil,false), got (%v,%v)", sc, ok) + } +} + +// TestRead_Corrupt: garbage bytes → (nil,false), never a crash. +func TestRead_Corrupt(t *testing.T) { + dir := t.TempDir() + writeSingleFileGen(t, dir) + if err := os.WriteFile(descriptions.Path(dir), []byte("{ not json"), 0o644); err != nil { + t.Fatal(err) + } + if sc, ok := descriptions.Read(dir); ok || sc != nil { + t.Errorf("corrupt sidecar: want (nil,false), got (%v,%v)", sc, ok) + } +} + +// TestRead_StaleAfterReindex: a new graph generation (reindex) invalidates the +// sidecar by source-key mismatch → (nil,false). +func TestRead_StaleAfterReindex(t *testing.T) { + dir := t.TempDir() + writeSingleFileGen(t, dir) + if err := descriptions.Upsert(dir, "e1", "desc"); err != nil { + t.Fatal(err) + } + if _, ok := descriptions.Read(dir); !ok { + t.Fatal("precondition: fresh sidecar must read ok") + } + // Simulate a reindex: write a new generation + flip current. + doc := &graph.Document{Repo: "r", Entities: []graph.Entity{{ID: "e1", QualifiedName: "p.A", Kind: "function", Name: "A"}}} + if err := fbwriter.WriteAtomic(filepath.Join(dir, graph.GenFileName(2)), doc); err != nil { + t.Fatal(err) + } + if err := graph.WriteCurrentPointer(dir, graph.GenFileName(2)); err != nil { + t.Fatal(err) + } + if sc, ok := descriptions.Read(dir); ok { + t.Errorf("stale sidecar after reindex must be ignored, got ok with %+v", sc) + } +} + +// TestUpsert_DiscardsStale: after a reindex, an upsert starts fresh (drops the +// stale entries written for the previous generation). +func TestUpsert_DiscardsStale(t *testing.T) { + dir := t.TempDir() + writeSingleFileGen(t, dir) + _ = descriptions.Upsert(dir, "e1", "old-gen desc") + // Reindex. + doc := &graph.Document{Repo: "r", Entities: []graph.Entity{{ID: "e2", QualifiedName: "p.B", Kind: "struct", Name: "B"}}} + _ = fbwriter.WriteAtomic(filepath.Join(dir, graph.GenFileName(2)), doc) + _ = graph.WriteCurrentPointer(dir, graph.GenFileName(2)) + // Upsert a new-gen entity. + if err := descriptions.Upsert(dir, "e2", "new-gen desc"); err != nil { + t.Fatal(err) + } + sc, ok := descriptions.Read(dir) + if !ok { + t.Fatal("Read not-ok after fresh upsert") + } + if _, present := sc.Results["e1"]; present { + t.Errorf("stale e1 should have been discarded, got %+v", sc.Results) + } + if sc.Results["e2"] != "new-gen desc" { + t.Errorf("Results[e2] = %q", sc.Results["e2"]) + } +} + +// TestCurrentSourceKey_JSONOnlyFallback: a repo with only graph.json (no +// graph.fb / gen) derives a key from graph.json's mtime. +func TestCurrentSourceKey_JSONOnlyFallback(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "graph.json"), []byte(`{"entities":[]}`), 0o644); err != nil { + t.Fatal(err) + } + key := descriptions.CurrentSourceKey(dir) + if len(key) < 5 || key[:5] != "json:" { + t.Errorf("JSON-only key = %q, want json: prefix", key) + } +} + +// TestWriteTo_Atomic: the written file is valid JSON with the expected shape. +func TestWriteTo_Atomic(t *testing.T) { + dir := t.TempDir() + sc := &descriptions.Sidecar{Version: 1, SourceKey: "k", Results: map[string]string{"x": "y"}} + if err := descriptions.WriteTo(descriptions.Path(dir), sc); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(descriptions.Path(dir)) + if err != nil { + t.Fatal(err) + } + var got descriptions.Sidecar + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("written file is not valid JSON: %v", err) + } + if got.Results["x"] != "y" { + t.Errorf("roundtrip Results = %+v", got.Results) + } +} diff --git a/internal/mcp/descriptions_overlay_test.go b/internal/mcp/descriptions_overlay_test.go new file mode 100644 index 000000000..9d02ed665 --- /dev/null +++ b/internal/mcp/descriptions_overlay_test.go @@ -0,0 +1,264 @@ +// descriptions_overlay_test.go — DESCRIPTION side-table read-merge (#5904 PR-a). +// +// applyDescriptionOverlay ADDITIVELY stamps /descriptions.json onto a +// loaded group's entities: PropSet("description") onto lr.Doc.Entities (the +// flag-OFF Doc read path) and into LabelIndex.descOverlay for the flag-ON mmap +// read path (materializeFromReader). These tests prove: +// +// - single-file graph: the description surfaces via PropGet on BOTH the Doc +// path (flag-OFF) and the mmap materialize-from-reader path (flag-ON); +// - segment-set graph: the description surfaces on the Doc path (the MCP serve +// path serves a segment-set from the collapsed Doc — Reader is nil — so the +// Doc stamp is the operative merge; the mmap segment serving is a later +// slice of #5890); +// - ADDITIVE: an entity with NO sidecar entry keeps its baked-in description; +// - absence/staleness: no sidecar (or a sidecar gone stale after a reindex) is +// a no-op — the entity keeps whatever graph.fb carried. +package mcp + +import ( + "os" + "path/filepath" + "sort" + "testing" + + "github.com/cajasmota/grafel/internal/daemon" + "github.com/cajasmota/grafel/internal/graph" + "github.com/cajasmota/grafel/internal/graph/descriptions" + "github.com/cajasmota/grafel/internal/graph/fbwriter" + "github.com/cajasmota/grafel/internal/registry" + "github.com/cajasmota/grafel/internal/testsupport" +) + +// newDescState registers a one-repo group "acme" (repo slug "svc") whose graph is +// written by writeGraph into the repo's state dir, and returns the State + the +// repo's state dir. +func newDescState(t *testing.T, writeGraph func(t *testing.T, stateDir string)) (*State, string) { + t.Helper() + testsupport.IsolateHome(t) + root := t.TempDir() + home := filepath.Join(root, "home") + t.Setenv("GRAFEL_HOME", home) + t.Setenv("GRAFEL_DAEMON_ROOT", filepath.Join(root, "daemon")) + + pathA := filepath.Join(root, "svc") + stateDir := daemon.StateDirForRepo(pathA) + writeGraph(t, stateDir) + + cfgPath, err := registry.ConfigPathFor("acme") + if err != nil { + t.Fatalf("config path: %v", err) + } + cfg := ®istry.GroupConfig{Name: "acme", Repos: []registry.Repo{{Slug: "svc", Path: pathA}}} + if err := registry.SaveGroupConfig(cfgPath, cfg); err != nil { + t.Fatalf("save group config: %v", err) + } + if err := registry.AddGroup("acme", cfgPath); err != nil { + t.Fatalf("add group: %v", err) + } + + inMem := &Registry{ + Path: filepath.Join(home, "registry.json"), + Groups: map[string]RegistryGroup{ + "acme": {Repos: map[string]RegistryRepo{"svc": {Path: pathA}}}, + }, + } + st := NewState(inMem) + t.Cleanup(st.Close) + return st, stateDir +} + +// descFixtureDoc builds a two-entity graph: "svc:withBaked" carries a baked-in +// description (extractor-native), "svc:noDesc" carries none. +func descFixtureDoc() *graph.Document { + return &graph.Document{Version: 1, Repo: "svc", Entities: []graph.Entity{ + graph.Entity{ID: "svc:noDesc", Name: "NoDesc", QualifiedName: "svc.NoDesc", Kind: "function"}. + WithProperties(map[string]string{}), + graph.Entity{ID: "svc:withBaked", Name: "WithBaked", QualifiedName: "svc.WithBaked", Kind: "function"}. + WithProperties(map[string]string{"description": "baked-in extractor description"}), + }} +} + +func writeSingleFileFlat(t *testing.T, stateDir string) { + t.Helper() + if err := fbwriter.WriteAtomic(filepath.Join(stateDir, "graph.fb"), descFixtureDoc()); err != nil { + t.Fatalf("write graph.fb: %v", err) + } +} + +// TestDescOverlay_SingleFile_DocPath: flag-OFF (default) — the sidecar +// description surfaces via PropGet on the Doc read path. +func TestDescOverlay_SingleFile_DocPath(t *testing.T) { + forceServeFromMMap(t, false) + st, stateDir := newDescState(t, writeSingleFileFlat) + if err := descriptions.Upsert(stateDir, "svc:noDesc", "written by writeback"); err != nil { + t.Fatalf("upsert: %v", err) + } + if _, err := st.Reload(); err != nil { + t.Fatalf("reload: %v", err) + } + lr := st.Group("acme").Repos["svc"] + if lr == nil || lr.Doc == nil { + t.Fatal("svc repo not loaded") + } + + got := lr.LabelIndex.ByID("svc:noDesc") + if got == nil || got.PropGet("description") != "written by writeback" { + t.Fatalf("Doc-path description not applied: %#v", got) + } + // ADDITIVE: the entity that had a baked-in description but NO sidecar entry + // keeps its baked-in value (never cleared). + baked := lr.LabelIndex.ByID("svc:withBaked") + if baked == nil || baked.PropGet("description") != "baked-in extractor description" { + t.Fatalf("baked-in description clobbered: %#v", baked) + } +} + +// TestDescOverlay_SingleFile_MmapPath: flag-ON — the sidecar description surfaces +// via the mmap materialize-from-reader path (LabelIndex.at over the resident +// Reader), and the additive/no-clobber guarantee holds there too. +func TestDescOverlay_SingleFile_MmapPath(t *testing.T) { + forceServeFromMMap(t, true) + st, stateDir := newDescState(t, writeSingleFileFlat) + if err := descriptions.Upsert(stateDir, "svc:noDesc", "mmap desc"); err != nil { + t.Fatalf("upsert: %v", err) + } + if _, err := st.Reload(); err != nil { + t.Fatalf("reload: %v", err) + } + lr := st.Group("acme").Repos["svc"] + if lr == nil || lr.Reader == nil || lr.LabelIndex == nil { + t.Fatal("svc repo requires a resident mmap Reader for this test") + } + + idx, ok := lr.LabelIndex.byID["svc:noDesc"] + if !ok { + t.Fatal("svc:noDesc not in LabelIndex") + } + got := lr.LabelIndex.at(idx) // mmap materialize-from-reader path + if got == nil || got.PropGet("description") != "mmap desc" { + t.Fatalf("mmap-path description not applied: %#v", got) + } + // ADDITIVE on the mmap path: baked-in survives (no sidecar entry). + bidx := lr.LabelIndex.byID["svc:withBaked"] + baked := lr.LabelIndex.at(bidx) + if baked == nil || baked.PropGet("description") != "baked-in extractor description" { + t.Fatalf("mmap-path clobbered baked-in description: %#v", baked) + } +} + +// TestDescOverlay_Absent_NoClobber: with NO sidecar, entities keep their +// baked-in descriptions and un-described entities stay un-described. +func TestDescOverlay_Absent_NoClobber(t *testing.T) { + forceServeFromMMap(t, false) + st, _ := newDescState(t, writeSingleFileFlat) + if _, err := st.Reload(); err != nil { + t.Fatalf("reload: %v", err) + } + lr := st.Group("acme").Repos["svc"] + if baked := lr.LabelIndex.ByID("svc:withBaked"); baked == nil || + baked.PropGet("description") != "baked-in extractor description" { + t.Fatalf("absent sidecar clobbered baked-in description: %#v", baked) + } + if nd := lr.LabelIndex.ByID("svc:noDesc"); nd == nil || nd.PropGet("description") != "" { + t.Fatalf("absent sidecar invented a description: %#v", nd) + } +} + +// TestDescOverlay_StaleAfterReindex_NoOp: a sidecar written for an older graph +// generation is stale (source-key mismatch) after a reindex and must NOT be +// applied — the entity keeps whatever the fresh graph.fb carried. +func TestDescOverlay_StaleAfterReindex_NoOp(t *testing.T) { + forceServeFromMMap(t, false) + st, stateDir := newDescState(t, writeSingleFileFlat) + if err := descriptions.Upsert(stateDir, "svc:noDesc", "stale desc"); err != nil { + t.Fatalf("upsert: %v", err) + } + // Simulate a reindex: rewrite graph.fb (a NEW mtime → the flat single-file + // source key changes → the sidecar goes stale). + writeSingleFileFlat(t, stateDir) + if _, err := st.Reload(); err != nil { + t.Fatalf("reload: %v", err) + } + lr := st.Group("acme").Repos["svc"] + if nd := lr.LabelIndex.ByID("svc:noDesc"); nd == nil || nd.PropGet("description") != "" { + t.Fatalf("stale sidecar was applied after reindex: %#v", nd) + } +} + +// writeSegmentSetForDir hand-builds a two-segment generation at stateDir. +func writeSegmentSetForDir(t *testing.T, stateDir string) { + t.Helper() + gen := uint64(3) + genDirName := graph.GenDirName(gen) + genDir := filepath.Join(stateDir, genDirName) + if err := os.MkdirAll(genDir, 0o755); err != nil { + t.Fatalf("mkdir gen dir: %v", err) + } + docs := []*graph.Document{ + {Repo: "svc", Entities: []graph.Entity{ + graph.Entity{ID: "svc:aaa", Name: "Aaa", QualifiedName: "svc.Aaa", Kind: "function"}. + WithProperties(map[string]string{}), + }}, + {Repo: "svc", Entities: []graph.Entity{ + graph.Entity{ID: "svc:bbb", Name: "Bbb", QualifiedName: "svc.Bbb", Kind: "function"}. + WithProperties(map[string]string{"description": "baked bbb"}), + }}, + } + m := &graph.Manifest{FormatVersion: graph.ManifestFormatVersion} + for i, doc := range docs { + name := graph.SegmentFileName(i) + if err := fbwriter.WriteAtomic(filepath.Join(genDir, name), doc); err != nil { + t.Fatalf("write %s: %v", name, err) + } + ids := make([]string, 0, len(doc.Entities)) + for _, e := range doc.Entities { + ids = append(ids, e.ID) + } + sort.Strings(ids) + m.Segments = append(m.Segments, graph.SegmentMeta{ + File: name, + Kind: graph.SegmentEntities, + EntityCount: len(doc.Entities), + MinKey: ids[0], + MaxKey: ids[len(ids)-1], + }) + } + if err := graph.WriteManifest(genDir, m); err != nil { + t.Fatalf("write manifest: %v", err) + } + if err := graph.WriteCurrentPointerRaw(stateDir, genDirName); err != nil { + t.Fatalf("write current pointer: %v", err) + } +} + +// TestDescOverlay_SegmentSet_DocPath: a segment-set graph (served from the +// collapsed Doc, Reader nil) surfaces the sidecar description on the Doc path, +// and the source key is segment-derived (so the sidecar is correctly matched to +// the segment-set generation, NOT a collapsed single-file path — #5915 P2). +func TestDescOverlay_SegmentSet_DocPath(t *testing.T) { + forceServeFromMMap(t, false) + st, stateDir := newDescState(t, writeSegmentSetForDir) + + // The sidecar source key must be segment-derived. + if key := descriptions.CurrentSourceKey(stateDir); len(key) < 4 || key[:4] != "seg:" { + t.Fatalf("segment-set source key = %q, want seg: prefix", key) + } + if err := descriptions.Upsert(stateDir, "svc:aaa", "seg desc for aaa"); err != nil { + t.Fatalf("upsert: %v", err) + } + if _, err := st.Reload(); err != nil { + t.Fatalf("reload: %v", err) + } + lr := st.Group("acme").Repos["svc"] + if lr == nil || lr.Doc == nil { + t.Fatal("svc segment-set repo not loaded") + } + if got := lr.LabelIndex.ByID("svc:aaa"); got == nil || got.PropGet("description") != "seg desc for aaa" { + t.Fatalf("segment-set Doc-path description not applied: %#v", got) + } + // ADDITIVE: svc:bbb (baked, no sidecar entry) keeps its baked description. + if bbb := lr.LabelIndex.ByID("svc:bbb"); bbb == nil || bbb.PropGet("description") != "baked bbb" { + t.Fatalf("segment-set clobbered baked description: %#v", bbb) + } +} diff --git a/internal/mcp/index.go b/internal/mcp/index.go index 7532783c6..c64574669 100644 --- a/internal/mcp/index.go +++ b/internal/mcp/index.go @@ -72,6 +72,20 @@ type LabelIndex struct { // applied. overlay map[int32]entityOverlay + // descOverlay is the DESCRIPTION side-table (#5904 PR-a): entity INDEX (int32 + // vector position) -> the agent-written "description" property that lives in + // /descriptions.json rather than in graph.fb (the enrichment + // write-back no longer rewrites the graph — the #5915 P1 collapse hazard). + // materializeFromReader ADDITIVELY PropSet-s it onto the Reader-materialized + // base so a lookup on the mmap path surfaces the description exactly as the + // overlaid Doc row does. A miss leaves whatever description graph.fb carried + // (extractor-native / baked-in) intact — never cleared. Populated by + // applyDescriptionOverlay from the SAME descriptions.json it PropSet-s onto + // lr.Doc, keyed by resolving each entity ID to its vector index. Built ONLY + // when GRAFEL_SERVE_FROM_MMAP is ON (the flag-off Doc path never consults it); + // nil otherwise and until a description sidecar is applied. + descOverlay map[int32]string + // readerMu points at the owning LoadedRepo.readerMu — the strictly-innermost // ADR-0027 SIGBUS-safety mutex (memory epic #5850, "Option B"). Wired by // reloadLocked; nil for the Doc-only build and directly-constructed indexes @@ -326,6 +340,12 @@ func (l *LabelIndex) materializeFromReader(idx int32) *graph.Entity { e.IsGodNode = ov.IsGodNode e.IsArticulationPt = ov.IsArticulationPt } + // Description side-table (#5904 PR-a): ADDITIVE merge — a present entry sets + // (or overrides) the "description" property; a miss leaves the base entity's + // own description (extractor-native / baked-in) untouched. + if d, ok := l.descOverlay[idx]; ok { + e.PropSet("description", d) + } return &e } diff --git a/internal/mcp/state.go b/internal/mcp/state.go index ad1f3bb36..e95f7e550 100644 --- a/internal/mcp/state.go +++ b/internal/mcp/state.go @@ -32,6 +32,7 @@ import ( "github.com/cajasmota/grafel/internal/daemon" "github.com/cajasmota/grafel/internal/embed" "github.com/cajasmota/grafel/internal/graph" + "github.com/cajasmota/grafel/internal/graph/descriptions" fb "github.com/cajasmota/grafel/internal/graph/fbgraph" "github.com/cajasmota/grafel/internal/graph/fbreader" "github.com/cajasmota/grafel/internal/graph/groupalgo" @@ -444,6 +445,19 @@ type LoadedRepo struct { // since the last stamp, regardless of the overlay-file memo. The zero value // means "never stamped", so a freshly-loaded repo is always (re-)stamped. algoStampedMt time.Time + + // descStampedMt / descFileMt / descApplied memoize the DESCRIPTION side-table + // apply (#5904 PR-a), mirroring algoStampedMt for the per-repo + // /descriptions.json sidecar. descStampedMt is the lr.mtime value at + // which applyDescriptionOverlay last PropSet the sidecar's descriptions onto + // this repo's entities; descFileMt is the sidecar file's mtime at that stamp. + // A re-stamp is needed when EITHER the repo reparsed (lr.mtime advanced past + // descStampedMt — a fresh graph.fb dropped any previously-stamped description) + // OR the sidecar file changed (a write-back advanced its mtime). The zero + // values ("never stamped") always force a (re-)stamp on first load / revive. + descStampedMt time.Time + descFileMt time.Time + descApplied bool } // resetIndexes re-arms every lazy-index sync.Once and clears the cached @@ -1557,6 +1571,10 @@ func (s *State) reloadAllLocked() (int, bool, error) { // NO behavior change until an overlay exists. Memoized by mtime so a // mid-session atomic swap reloads only the overlay, not the graphs. applyGroupAlgoOverlay(grp) + // Description side-table (#5904 PR-a): ADDITIVELY stamp any + // /descriptions.json onto the group's entities. Absence-tolerant + // and per-repo memoized, exactly like the group-algo overlay above. + applyDescriptionOverlay(grp) } // Recompute the tool-surface signature. If it differs from the previous // value the caller will emit notifications/tools/list_changed (#1772). @@ -1606,6 +1624,12 @@ func (s *State) Group(name string) *LoadedGroup { // re-materialized group. grp.lastAccess = time.Now() s.refreshGroupAlgoOverlayLocked(grp) + // Re-check the per-repo description side-table mtimes on the serving path so + // a write-back that advanced /descriptions.json mid-session takes + // effect without a full Reload (mirrors refreshGroupAlgoOverlayLocked). The + // per-repo memo inside applyDescriptionOverlay keeps the steady state a + // cheap stat-and-skip. + applyDescriptionOverlay(grp) } return grp } @@ -2632,6 +2656,128 @@ func buildOverlayTableFromDoc(doc *graph.Document, ov *groupalgo.Overlay) map[in return table } +// applyDescriptionOverlay stamps the per-repo DESCRIPTION side-table (#5904 +// PR-a) onto a loaded group's entities. For each repo it reads +// /descriptions.json (absence/corrupt/stale-tolerant) and, for every +// entity WITH a sidecar entry, ADDITIVELY PropSet-s "description": +// +// - onto lr.Doc.Entities — the flag-OFF read path (LabelIndex.at / +// forEachEntity) and the MCP serve surfaces that read PropGet("description") +// directly. +// - into an entity-INDEX-keyed LabelIndex.descOverlay table for the flag-ON +// mmap read path (materializeFromReader / materializeEntityOverlay). +// +// ADDITIVE guarantee: a missing sidecar entry leaves whatever description the +// entity already carried (extractor-native / baked-in from a prior write-back) +// UNTOUCHED — a miss never clears. Absence/staleness of the whole sidecar is a +// no-op for the same reason. +// +// Memoized PER REPO (mirrors applyGroupAlgoOverlay's #5400/#5401 per-repo memo): +// a repo is re-stamped when EITHER it reparsed since the last stamp (lr.mtime +// advanced past lr.descStampedMt — a fresh graph.fb dropped the prior PropSet) +// OR its sidecar file mtime changed (a write-back landed). The steady state +// (nothing changed) is a cheap per-repo os.Stat and skip. Caller holds s.mu, the +// same lock applyGroupAlgoOverlay mutates Doc/LabelIndex under. +func applyDescriptionOverlay(grp *LoadedGroup) { + if grp == nil { + return + } + for _, lr := range grp.Repos { + if lr == nil || lr.Doc == nil || lr.Path == "" { + continue + } + stateDir := daemon.StateDirForRepo(lr.Path) + var fileMt time.Time + if fi, err := os.Stat(descriptions.Path(stateDir)); err == nil { + fileMt = fi.ModTime() + } + // Skip when neither the sidecar file nor this repo's graph changed since + // the last stamp. The zero descStampedMt (never stamped) always falls + // through to (re-)stamp. + if lr.descApplied && lr.mtime.Equal(lr.descStampedMt) && fileMt.Equal(lr.descFileMt) { + continue + } + + sc, ok := descriptions.Read(stateDir) + + // In-place Doc stamp (flag-OFF read path + PropGet consumers). ADDITIVE: + // only entities WITH a sidecar entry are touched; everything else keeps + // its existing description. + if ok { + ents := lr.Doc.Entities + for i := range ents { + if d, has := sc.Results[ents[i].ID]; has { + ents[i].PropSet("description", d) + } + } + } + + // Flag-ON mmap side-table: rebuild the index-keyed description table under + // readerMu, mirroring applyGroupAlgoOverlay's overlay-table publish. A + // retired/nil Reader falls back to the Doc (where the read path also reads + // the Doc, so the table is never consulted there anyway). When the sidecar + // is absent/stale (ok==false) the table is cleared to nil so a previously + // stamped description does not linger on the mmap path after it went stale. + if serveFromMMap() { + lr.rmu().Lock() + var table map[int32]string + if ok { + if rdr := lr.Reader; rdr != nil && !(lr.handle != nil && lr.handle.readRetired) { + table = buildDescTableFromReader(rdr, sc) + } else { + table = buildDescTableFromDoc(lr.Doc, sc) + } + } + if lr.LabelIndex != nil { + lr.LabelIndex.descOverlay = table + } + lr.rmu().Unlock() + } + + lr.descStampedMt = lr.mtime + lr.descFileMt = fileMt + lr.descApplied = true + } +} + +// buildDescTableFromReader builds the entity-INDEX-keyed description side-table +// by iterating the resident mmap Reader (NOT lr.Doc), matching each entity to +// the sidecar by ID. The int32 key is the vector index i in Reader iteration +// order — identical to the order BuildLabelIndexFromReader assigns and the order +// materializeFromReader looks the table up by. Callers MUST hold the owning +// LoadedRepo's readerMu (the mmap is dereferenced here via IterateEntities). +func buildDescTableFromReader(r *fbreader.Reader, sc *descriptions.Sidecar) map[int32]string { + table := make(map[int32]string) + if r == nil || sc == nil { + return table + } + var i int32 + r.IterateEntities(func(e *fb.Entity) bool { + if d, has := sc.Results[string(e.Id())]; has { + table[i] = d + } + i++ + return true + }) + return table +} + +// buildDescTableFromDoc is the Doc-sourced twin used ONLY when no mmap Reader is +// resident (JSON-only / no graph.fb) on the flag-on path. Keyed by the same +// vector index i as the Reader path. +func buildDescTableFromDoc(doc *graph.Document, sc *descriptions.Sidecar) map[int32]string { + table := make(map[int32]string) + if doc == nil || sc == nil { + return table + } + for i := range doc.Entities { + if d, has := sc.Results[doc.Entities[i].ID]; has { + table[int32(i)] = d + } + } + return table +} + // defaultLinksFile is the conventional path for cross-repo links. func defaultLinksFile(group string) string { home, err := os.UserHomeDir() diff --git a/internal/mcp/view_iter.go b/internal/mcp/view_iter.go index 374c62a86..7ac2cf0f3 100644 --- a/internal/mcp/view_iter.go +++ b/internal/mcp/view_iter.go @@ -66,12 +66,14 @@ func (lr *LoadedRepo) forEachEntity(yield func(*graph.Entity) bool) { defer lr.rmu().Unlock() var overlay map[int32]entityOverlay + var descOverlay map[int32]string if lr.LabelIndex != nil { overlay = lr.LabelIndex.overlay + descOverlay = lr.LabelIndex.descOverlay } n := rdr.EntityCount() for i := 0; i < n; i++ { - e := materializeEntityOverlay(rdr, overlay, int32(i)) + e := materializeEntityOverlay(rdr, overlay, descOverlay, int32(i)) if !yield(e) { return } @@ -151,15 +153,17 @@ func (lr *LoadedRepo) forEachEntityOfKinds(pred func(kind string) bool, yield fu defer lr.rmu().Unlock() var overlay map[int32]entityOverlay + var descOverlay map[int32]string if lr.LabelIndex != nil { overlay = lr.LabelIndex.overlay + descOverlay = lr.LabelIndex.descOverlay } n := rdr.EntityCount() for _, idx := range idxs { if int(idx) >= n { continue } - e := materializeEntityOverlay(rdr, overlay, idx) + e := materializeEntityOverlay(rdr, overlay, descOverlay, idx) if !yield(e) { return } @@ -203,7 +207,7 @@ func (lr *LoadedRepo) forEachDocEntity(yield func(*graph.Entity) bool) { // is byte-identical to a LabelIndex.at()-based lookup for the same index. // Callers MUST hold the owning LoadedRepo's readerMu — the mmap dereference // happens here. -func materializeEntityOverlay(r *fbreader.Reader, overlay map[int32]entityOverlay, idx int32) *graph.Entity { +func materializeEntityOverlay(r *fbreader.Reader, overlay map[int32]entityOverlay, descOverlay map[int32]string, idx int32) *graph.Entity { // Test-only observability seam (memory epic #5850 Path P): count each mmap // entity materialization so the selective-materialization tests can assert a // forEachEntityOfKinds scan materializes ONLY its matching-Kind subset, not the @@ -220,6 +224,11 @@ func materializeEntityOverlay(r *fbreader.Reader, overlay map[int32]entityOverla e.IsGodNode = ov.IsGodNode e.IsArticulationPt = ov.IsArticulationPt } + // Description side-table (#5904 PR-a): ADDITIVE merge, mirroring + // LabelIndex.materializeFromReader — a miss leaves the base description intact. + if d, ok := descOverlay[idx]; ok { + e.PropSet("description", d) + } return &e }