feat(daemon): graceful stale-format migration core — version-gate serve cache + loop-guarded auto-reindex (#5907)

internal/daemon/mcp/graph_cache.go

@@ -27,6 +27,7 @@ import (
 	"github.com/cajasmota/grafel/internal/daemon"
 	"github.com/cajasmota/grafel/internal/graph"
 	"github.com/cajasmota/grafel/internal/graph/fbreader"
+	"github.com/cajasmota/grafel/internal/graph/fbversion"
 )
 
 // DefaultCapacity is the default maximum number of simultaneously
@@ -379,14 +380,73 @@ func (c *Cache) releaser(ent *Entry) func() {
 
 // openReader stats the path, opens the FlatBuffers reader, and returns
 // both alongside the file's mtime in unix nanoseconds.
+//
+// #5907 — this zero-copy serve path is version-GATED, mirroring the rejection
+// in internal/graph/load.go's loadFBDoc. Two crash-prevention guards live here,
+// both of which the ungated fbreader.Open call previously lacked:
+//
+//  1. A recovered open (openReaderRecover): fbreader.Open recovers-then-
+//     RE-PANICS on a malformed/incompatible flatbuffer vtable (see
+//     fbreader/reader.go). A breaking on-disk cut — the #5890 segmented format
+//     read by a binary that predates it, or a genuinely corrupt buffer — would
+//     otherwise re-panic straight out of this cache and crash the daemon. We
+//     recover it into a plain error so the cache degrades to a miss.
+//
+//  2. A format-version gate: if the on-disk FormatVersion is below what this
+//     binary supports, we Close the reader and return the SAME typed
+//     *graph.FormatVersionError loadFBDoc returns — so callers can errors.As it
+//     and degrade to "serve empty + reindex-required" (mirroring the detection
+//     in internal/mcp/state.go), and so we NEVER cache or serve a stale-format
+//     reader (Get discards the handle on any openReader error before inserting
+//     it into the LRU). This is the reader-side half of the same migration the
+//     status-plane's ReindexRequiredReason flag reports.
 func openReader(path string) (*fbreader.Reader, int64, error) {
 	info, err := os.Stat(path)
 	if err != nil {
 		return nil, 0, fmt.Errorf("graph cache: stat %s: %w", path, err)
 	}
-	r, err := fbreader.Open(path)
+	r, err := openAndGate(path)
 	if err != nil {
 		return nil, 0, err
 	}
 	return r, info.ModTime().UnixNano(), nil
 }
+
+// openAndGate opens path and enforces the format-version gate, returning a
+// usable reader only for an on-disk format this binary can serve. It is the
+// crash-prevention seam for the zero-copy MCP cache (#5907):
+//
+//   - A single recover covers BOTH fbreader.Open (which recovers-then-
+//     re-panics on a malformed/incompatible flatbuffer vtable) AND the
+//     r.Version() scalar read that follows — so neither a corrupt buffer nor a
+//     breaking on-disk cut (the #5890 segmented format read by an older binary)
+//     can re-panic out of the cache and crash the daemon. A recovered panic
+//     becomes a plain error and the reader (if any) is Closed to free its mmap.
+//   - The format-version gate mirrors internal/graph/load.go's loadFBDoc: an
+//     on-disk FormatVersion below fbversion.Version yields the SAME typed
+//     *graph.FormatVersionError, Closed before return so we neither cache nor
+//     leak a stale-format handle. Callers can errors.As it and degrade to
+//     "serve empty + reindex-required" (mirroring internal/mcp/state.go's
+//     detection), while the engine's status writer independently flags the
+//     repo and auto-enqueues its migration reindex.
+func openAndGate(path string) (r *fbreader.Reader, err error) {
+	defer func() {
+		if rec := recover(); rec != nil {
+			if r != nil {
+				_ = r.Close()
+				r = nil
+			}
+			err = fmt.Errorf("graph cache: recovered panic opening %s: %v", path, rec)
+		}
+	}()
+	r, err = fbreader.Open(path)
+	if err != nil {
+		return nil, err
+	}
+	if v := r.Version(); v < fbversion.Version {
+		_ = r.Close()
+		return nil, fmt.Errorf("graph cache: %w",
+			&graph.FormatVersionError{Found: v, Required: fbversion.Version})
+	}
+	return r, nil
+}

internal/daemon/mcp/graph_cache_version_test.go

@@ -0,0 +1,181 @@
+package mcp
+
+// graph_cache_version_test.go — #5907 FIX 1: the zero-copy MCP graph cache must
+// NEVER serve (or crash on) an on-disk graph.fb whose format is older than /
+// incompatible with this binary. These are the RED→GREEN tests for the
+// version gate + panic-recover added to openReader/openAndGate: an old-format
+// file yields a typed *graph.FormatVersionError (no panic, not cached), and a
+// genuinely-corrupt buffer degrades to an error instead of taking down the
+// daemon.
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/cajasmota/grafel/internal/graph"
+	fb "github.com/cajasmota/grafel/internal/graph/fbgraph"
+	"github.com/cajasmota/grafel/internal/graph/fbversion"
+	"github.com/cajasmota/grafel/internal/graph/fbwriter"
+)
+
+// writeStaleFormatGraph builds a valid graph.fb via fbwriter.Marshal and then
+// patches its on-disk Graph.version scalar DOWN to oldVersion — the same
+// technique used by internal/graph/fbwriter's TestLoaderRejectsOldFormatVersion
+// and internal/graph's reindex_required_test.go — so a test can fabricate an
+// "old on-disk format vs new binary" file without an actual old binary or the
+// (not-yet-shipped) segmented format.
+func writeStaleFormatGraph(t *testing.T, path string, oldVersion int) {
+	t.Helper()
+	doc := &graph.Document{
+		Repo:     "stale-fixture",
+		Entities: []graph.Entity{{ID: "s::a", QualifiedName: "pkg.A", Kind: "function", Name: "A"}},
+	}
+	doc.Stats.Entities = 1
+	buf, err := fbwriter.Marshal(doc)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	root := fb.GetRootAsGraph(buf, 0)
+	if !root.MutateVersion(int32(oldVersion)) {
+		t.Fatalf("MutateVersion(%d) returned false — slot missing?", oldVersion)
+	}
+	if err := os.WriteFile(path, buf, 0o644); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+}
+
+// TestCache_StaleFormat_TypedError_NotCached is the core FIX-1 assertion: a
+// Get against an old-format graph.fb returns a *graph.FormatVersionError (via
+// errors.As, not a string match), never panics, and never leaves a resident
+// handle in the cache (a stale-format reader must never be served).
+func TestCache_StaleFormat_TypedError_NotCached(t *testing.T) {
+	dir := t.TempDir()
+	p := filepath.Join(dir, "stale.fb")
+	writeStaleFormatGraph(t, p, fbversion.Version-1)
+
+	c := NewCache(4)
+	defer c.Close()
+
+	r, release, err := c.Get(p) // must NOT panic
+	if release != nil {
+		release()
+	}
+	if r != nil {
+		t.Fatalf("expected nil reader for a stale-format graph.fb, got %v", r)
+	}
+	if err == nil {
+		t.Fatal("expected an error for a stale-format graph.fb, got nil")
+	}
+	var fvErr *graph.FormatVersionError
+	if !errors.As(err, &fvErr) {
+		t.Fatalf("expected errors.As to find a *graph.FormatVersionError, got %v", err)
+	}
+	if fvErr.Found != fbversion.Version-1 {
+		t.Errorf("FormatVersionError.Found = %d, want %d", fvErr.Found, fbversion.Version-1)
+	}
+	if fvErr.Required != fbversion.Version {
+		t.Errorf("FormatVersionError.Required = %d, want %d", fvErr.Required, fbversion.Version)
+	}
+	// The stale reader must NOT be cached (never served on a subsequent Get).
+	if c.Len() != 0 {
+		t.Fatalf("stale-format reader must not be cached; cache Len = %d", c.Len())
+	}
+	if s := c.Stats(); s.OpenErrors < 1 {
+		t.Errorf("expected an open error to be counted, stats = %+v", s)
+	}
+}
+
+// TestQueryService_StaleFormat_DegradesGracefully proves the typed error
+// propagates through a real handler as an error return — NOT a panic / crash.
+func TestQueryService_StaleFormat_DegradesGracefully(t *testing.T) {
+	dir := t.TempDir()
+	p := filepath.Join(dir, "stale.fb")
+	writeStaleFormatGraph(t, p, fbversion.Version-1)
+
+	c := NewCache(4)
+	defer c.Close()
+	q := NewQueryService(c)
+
+	view, err := q.ReadEntity(p, "s::a") // must not panic
+	if view != nil {
+		t.Fatalf("expected nil entity view for stale-format graph, got %+v", view)
+	}
+	var fvErr *graph.FormatVersionError
+	if !errors.As(err, &fvErr) {
+		t.Fatalf("expected a *graph.FormatVersionError from the handler, got %v", err)
+	}
+}
+
+// TestCache_CorruptBuffer_NoCrash covers the second crash vector: a genuinely
+// corrupt / incompatible buffer (long enough to pass fbreader's length guard
+// but with a garbage vtable) must NOT re-panic out of the cache. The recover
+// in openAndGate converts it into an error and leaves the cache empty.
+func TestCache_CorruptBuffer_NoCrash(t *testing.T) {
+	dir := t.TempDir()
+
+	// Two flavours of corruption:
+	//  1. Too short for a flatbuffer (< 8 bytes) — fbreader.Open rejects directly.
+	//  2. Long enough to pass the length guard but structurally garbage — the
+	//     flatbuffer parse/scalar-read re-panics inside fbreader; openAndGate
+	//     must recover it into an error.
+	cases := map[string][]byte{
+		"tooShort": {0x01, 0x02, 0x03},
+		"garbageVTable": func() []byte {
+			b := make([]byte, 256)
+			for i := range b {
+				b[i] = 0xFF
+			}
+			return b
+		}(),
+	}
+
+	c := NewCache(4)
+	defer c.Close()
+
+	for name, data := range cases {
+		t.Run(name, func(t *testing.T) {
+			p := filepath.Join(dir, name+".fb")
+			if err := os.WriteFile(p, data, 0o644); err != nil {
+				t.Fatalf("write: %v", err)
+			}
+			r, release, err := c.Get(p) // must NOT panic/crash the process
+			if release != nil {
+				release()
+			}
+			if r != nil {
+				t.Fatalf("expected nil reader for a corrupt buffer, got %v", r)
+			}
+			if err == nil {
+				t.Fatal("expected an error for a corrupt buffer, got nil")
+			}
+		})
+	}
+	if c.Len() != 0 {
+		t.Fatalf("corrupt buffers must not be cached; cache Len = %d", c.Len())
+	}
+}
+
+// TestCache_CurrentFormat_Unaffected is the parity guard: a normal,
+// current-version graph.fb loads exactly as before the gate was added.
+func TestCache_CurrentFormat_Unaffected(t *testing.T) {
+	dir := t.TempDir()
+	p := writeFixtureGraph(t, dir, "current", time.Time{})
+
+	c := NewCache(4)
+	defer c.Close()
+
+	r, release, err := c.Get(p)
+	if err != nil {
+		t.Fatalf("current-format Get: %v", err)
+	}
+	defer release()
+	if r == nil || r.EntityCount() != 2 {
+		t.Fatalf("expected a usable reader with 2 entities, got %v", r)
+	}
+	if c.Len() != 1 {
+		t.Fatalf("current-format reader should be cached; Len = %d", c.Len())
+	}
+}

internal/daemon/stale_reindex.go

@@ -0,0 +1,100 @@
+package daemon
+
+// stale_reindex.go — #5907 FIX 2: the ACTION arm for stale-on-disk-format
+// detection. Detection (graph.ReindexRequiredReason, surfaced on the status
+// plane by statuswriter.go) has, until now, ZERO action-consumers: a repo
+// whose graph.fb was written by an older grafel build than this binary
+// supports sits idle forever, silently serving nothing, until a human runs
+// `grafel index`. This closes that silent stall by having the engine
+// auto-enqueue a reindex through the SAME requests→drain→scheduler plumbing
+// Service.Index already uses — the daemon-side equivalent of the CLI's
+// FormatVersionError → full-reindex fallback.
+//
+// The load-bearing property is the LOOP-GUARD (the #5891-class hazard): the
+// engine's status writer recomputes ReindexRequired on EVERY heartbeat, so a
+// naive "if required { enqueue }" would fire a reindex request storm — one per
+// heartbeat for the whole time the stale graph is on disk, including the entire
+// duration of the reindex it already triggered. staleReindexGuard makes the
+// enqueue fire AT MOST ONCE per (repo, stale generation):
+//
+//   - The fingerprint is (graph.fb mtime | reindex reason). The mtime advances
+//     ONLY when graph.fb is rewritten — i.e. a fresh index actually lands — so
+//     every heartbeat observing the same stale file computes the SAME
+//     fingerprint and is deduped. While the triggered reindex is in flight the
+//     stale file is untouched (the gen `current` pointer is flipped only when
+//     the new graph is complete), so the fingerprint is stable and NO second
+//     request is written.
+//   - When the reindex completes and the graph is current, ReindexRequired
+//     flips false; maybeEnqueue then FORGETS the repo's fingerprint, so the
+//     guard self-clears and a genuinely NEW stale generation later (distinct
+//     mtime) can fire exactly one fresh request — but a current repo never does.
+
+import (
+	"fmt"
+	"log/slog"
+	"sync"
+
+	"github.com/cajasmota/grafel/internal/daemon/requests"
+)
+
+// staleReindexGuard tracks, per repo path, the stale-format fingerprint we have
+// already auto-enqueued a reindex for, so we never enqueue twice for the same
+// stale generation. Concurrency-safe: writeRepoStatusFile runs on the single
+// statusWriter goroutine today, but the guard is package-global and defended by
+// a mutex so a future second caller (e.g. a startup reconcile pass) cannot race
+// it into a double-enqueue.
+type staleReindexGuard struct {
+	mu sync.Mutex
+	// seen maps repoPath -> the stale fingerprint a reindex was last enqueued
+	// for. Absence means "no outstanding auto-reindex for a stale generation".
+	seen map[string]string
+}
+
+func newStaleReindexGuard() *staleReindexGuard {
+	return &staleReindexGuard{seen: map[string]string{}}
+}
+
+// defaultStaleReindexGuard is the process-wide guard used by writeRepoStatusFile.
+var defaultStaleReindexGuard = newStaleReindexGuard()
+
+// staleFingerprint identifies one stale generation of repoPath's on-disk
+// graph.fb: the file mtime (advances only on a real rewrite) plus the
+// reason string (which names the found format version). It is stable across
+// heartbeats that observe the same stale file, and changes the moment a new
+// graph is written — the two properties the loop-guard relies on.
+func staleFingerprint(graphFBMtime int64, reason string) string {
+	return fmt.Sprintf("%d|%s", graphFBMtime, reason)
+}
+
+// maybeEnqueue is the loop-guarded auto-reindex arm. When required is true and
+// this exact (repoPath, fingerprint) has not already been enqueued, it writes a
+// single KindReindex request into repoPath's control-plane requests dir — the
+// engine's drain loop then applies it via scheduler.Enqueue, exactly as an
+// explicit `grafel index --async` would in split mode — and records the
+// fingerprint so subsequent heartbeats are deduped. When required is false it
+// forgets any recorded fingerprint (self-clear). Returns true iff it wrote a
+// request this call. A write failure is logged (best-effort, like the rest of
+// the status writer) and NOT recorded, so the next heartbeat retries.
+func (g *staleReindexGuard) maybeEnqueue(repoPath string, required bool, fingerprint string, logger *slog.Logger) bool {
+	g.mu.Lock()
+	defer g.mu.Unlock()
+
+	if !required {
+		delete(g.seen, repoPath)
+		return false
+	}
+	if g.seen[repoPath] == fingerprint {
+		return false // already enqueued for this exact stale generation
+	}
+	if _, err := requests.Write(requestsDirForRepo(repoPath), requests.Record{
+		Kind:     requests.KindReindex,
+		RepoPath: repoPath,
+	}); err != nil {
+		if logger != nil {
+			logger.Warn("statusfile: auto-reindex enqueue failed", "repo", repoPath, "err", err)
+		}
+		return false
+	}
+	g.seen[repoPath] = fingerprint
+	return true
+}