fix: return structured staleness from cacheNeedsBuild

Replace early-return + reason-string-parsing with a cacheStaleness
struct that collects all signals (new messages, deletions, missing
tables) before returning. This fixes a mixed add+delete sync where
the new-messages check short-circuited the deletion check, causing
an incremental rebuild that left deleted rows in parquet shards.

The FullRebuild flag is set whenever deletions are detected or the
cache is missing/empty, so callers no longer need to parse the
human-readable Reason string.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: wesm

cmd/msgvault/cmd/build_cache_test.go

@@ -1730,12 +1730,12 @@ func TestCacheNeedsBuild(t *testing.T) {
 
 			tt.setup(t, dbPath, analyticsDir)
 
-			gotBuild, gotReason := cacheNeedsBuild(dbPath, analyticsDir)
-			if gotBuild != tt.wantBuild {
-				t.Errorf("cacheNeedsBuild() build = %v, want %v (reason: %q)", gotBuild, tt.wantBuild, gotReason)
+			got := cacheNeedsBuild(dbPath, analyticsDir)
+			if got.NeedsBuild != tt.wantBuild {
+				t.Errorf("cacheNeedsBuild() build = %v, want %v (reason: %q)", got.NeedsBuild, tt.wantBuild, got.Reason)
 			}
-			if tt.wantReason != "" && gotReason != tt.wantReason {
-				t.Errorf("cacheNeedsBuild() reason = %q, want %q", gotReason, tt.wantReason)
+			if tt.wantReason != "" && got.Reason != tt.wantReason {
+				t.Errorf("cacheNeedsBuild() reason = %q, want %q", got.Reason, tt.wantReason)
 			}
 		})
 	}

cmd/msgvault/cmd/serve.go

@@ -9,7 +9,6 @@ import (
 	"os"
 	"os/signal"
 	"strconv"
-	"strings"
 	"syscall"
 	"time"
 
@@ -89,8 +88,8 @@ func runServe(cmd *cobra.Command, args []string) error {
 	// otherwise fall back to SQLite so remote endpoints still work.
 	analyticsDir := cfg.AnalyticsDir()
 	var engine query.Engine
-	needsBuild, reason := cacheNeedsBuild(dbPath, analyticsDir)
-	if !needsBuild && query.HasCompleteParquetData(analyticsDir) {
+	staleness := cacheNeedsBuild(dbPath, analyticsDir)
+	if !staleness.NeedsBuild && query.HasCompleteParquetData(analyticsDir) {
 		duckEngine, engineErr := query.NewDuckDBEngine(
 			analyticsDir, dbPath, s.DB(),
 		)
@@ -102,9 +101,9 @@ func runServe(cmd *cobra.Command, args []string) error {
 			engine = duckEngine
 		}
 	} else {
-		if reason != "" {
+		if staleness.Reason != "" {
 			logger.Info("parquet cache not usable, using SQLite engine",
-				"reason", reason)
+				"reason", staleness.Reason)
 		} else {
 			logger.Info("parquet cache not built - using SQLite engine (run 'msgvault build-cache' for faster aggregates)")
 		}
@@ -318,19 +317,15 @@ func runScheduledSync(ctx context.Context, email string, s *store.Store, oauthMg
 		"duration", time.Since(startTime),
 	)
 
-	// Rebuild cache if stale (covers new messages, deletions, and
-	// label updates — not just additions).
+	// Rebuild cache if stale (covers new messages and deletions).
 	dbPath := cfg.DatabaseDSN()
 	analyticsDir := cfg.AnalyticsDir()
-	if needsBuild, reason := cacheNeedsBuild(dbPath, analyticsDir); needsBuild {
-		// Deletions require a full rebuild because incremental builds
-		// only export rows with id > lastMessageID and cannot update
-		// or remove existing rows in parquet shards.
-		fullRebuild := strings.Contains(reason, "deletions")
+	if staleness := cacheNeedsBuild(dbPath, analyticsDir); staleness.NeedsBuild {
 		logger.Info("rebuilding cache after sync",
-			"email", email, "reason", reason,
-			"full_rebuild", fullRebuild)
-		result, err := buildCache(dbPath, analyticsDir, fullRebuild)
+			"email", email, "reason", staleness.Reason,
+			"full_rebuild", staleness.FullRebuild)
+		result, err := buildCache(
+			dbPath, analyticsDir, staleness.FullRebuild)
 		if err != nil {
 			logger.Error("cache build failed", "error", err)
 			// Don't fail the sync for cache build errors

cmd/msgvault/cmd/tui.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	tea "github.com/charmbracelet/bubbletea"
 	"github.com/spf13/cobra"
@@ -102,10 +103,10 @@ Remote Mode:
 
 			// Check if cache needs to be built/updated (unless forcing SQL or skipping)
 			if !forceSQL && !skipCacheBuild {
-				needsBuild, reason := cacheNeedsBuild(dbPath, analyticsDir)
-				if needsBuild {
-					fmt.Printf("Building analytics cache (%s)...\n", reason)
-					result, err := buildCache(dbPath, analyticsDir, true)
+				staleness := cacheNeedsBuild(dbPath, analyticsDir)
+				if staleness.NeedsBuild {
+					fmt.Printf("Building analytics cache (%s)...\n", staleness.Reason)
+					result, err := buildCache(dbPath, analyticsDir, staleness.FullRebuild)
 					if err != nil {
 						fmt.Fprintf(os.Stderr, "Warning: Failed to build cache: %v\n", err)
 						fmt.Fprintf(os.Stderr, "Falling back to SQLite (may be slow for large archives)\n")
@@ -157,9 +158,19 @@ Remote Mode:
 	},
 }
 
-// cacheNeedsBuild checks if the analytics cache needs to be built or updated.
-// Returns (needsBuild, reason) where reason describes why.
-func cacheNeedsBuild(dbPath, analyticsDir string) (bool, string) {
+// cacheStaleness describes why the analytics cache needs a rebuild.
+type cacheStaleness struct {
+	NeedsBuild  bool
+	HasNew      bool // new messages since last build
+	HasDeleted  bool // deletions since last build
+	FullRebuild bool // must rewrite all shards (not incremental)
+	Reason      string
+}
+
+// cacheNeedsBuild checks if the analytics cache needs to be built or
+// updated. Collects all staleness signals before returning so that
+// e.g. a mixed add+delete sync correctly reports both.
+func cacheNeedsBuild(dbPath, analyticsDir string) cacheStaleness {
 	messagesDir := filepath.Join(analyticsDir, "messages")
 	stateFile := filepath.Join(analyticsDir, "_last_sync.json")
 
@@ -169,22 +180,31 @@ func cacheNeedsBuild(dbPath, analyticsDir string) (bool, string) {
 	data, err := os.ReadFile(stateFile)
 	if err != nil {
 		if !hasParquetData {
-			return true, "no cache exists"
+			return cacheStaleness{
+				NeedsBuild: true, FullRebuild: true,
+				Reason: "no cache exists",
+			}
+		}
+		return cacheStaleness{
+			NeedsBuild: true, FullRebuild: true,
+			Reason: "no sync state found",
 		}
-		return true, "no sync state found"
 	}
 
 	var state syncState
 	if err := json.Unmarshal(data, &state); err != nil {
-		return true, "invalid sync state"
+		return cacheStaleness{
+			NeedsBuild: true, FullRebuild: true,
+			Reason: "invalid sync state",
+		}
 	}
 
-	// Check if SQLite has newer messages
-	// We need to query SQLite directly to check max message ID
 	db, err := store.Open(dbPath)
 	if err != nil {
-		// Can't open DB to check - force rebuild to be safe
-		return true, "cannot verify cache status"
+		return cacheStaleness{
+			NeedsBuild: true, FullRebuild: true,
+			Reason: "cannot verify cache status",
+		}
 	}
 	defer db.Close()
 
@@ -194,59 +214,74 @@ func cacheNeedsBuild(dbPath, analyticsDir string) (bool, string) {
 		WHERE deleted_from_source_at IS NULL AND sent_at IS NOT NULL
 	`).Scan(&maxID)
 	if err != nil {
-		// Can't query - force rebuild to be safe
-		return true, "cannot verify cache status"
+		return cacheStaleness{
+			NeedsBuild: true, FullRebuild: true,
+			Reason: "cannot verify cache status",
+		}
 	}
 
-	// Zero-message accounts never produce message parquet files, so
-	// HasParquetData returns false even after a successful cache build.
-	// If sync state and DB both agree there are 0 messages, no build needed.
 	if maxID == 0 && state.LastMessageID == 0 {
-		return false, ""
+		return cacheStaleness{}
 	}
 
 	if !hasParquetData {
-		return true, "no cache exists"
+		return cacheStaleness{
+			NeedsBuild: true, FullRebuild: true,
+			Reason: "no cache exists",
+		}
 	}
 
+	// Collect staleness signals without short-circuiting so a mixed
+	// add+delete sync correctly triggers a full rebuild.
+	var reasons []string
+	result := cacheStaleness{}
+
 	if maxID > state.LastMessageID {
 		newCount := maxID - state.LastMessageID
-		return true, fmt.Sprintf("%d new messages", newCount)
+		result.HasNew = true
+		reasons = append(reasons,
+			fmt.Sprintf("%d new messages", newCount))
 	}
 
-	// Check if any messages were soft-deleted after the last cache build.
-	// Incremental builds don't rewrite existing rows, so deletions that
-	// occur after the build leave stale (non-deleted) rows in Parquet.
-	// Format as "YYYY-MM-DD HH:MM:SS" to match datetime('now') values.
-	var deletedSinceBuild int64
 	syncAtStr := state.LastSyncAt.UTC().Format("2006-01-02 15:04:05")
+	var deletedSinceBuild int64
 	err = db.DB().QueryRow(`
 		SELECT COUNT(*) FROM messages
 		WHERE deleted_from_source_at IS NOT NULL
 		  AND deleted_from_source_at >= ?
 	`, syncAtStr).Scan(&deletedSinceBuild)
 	if err != nil {
-		return true, "cannot verify deletion state"
+		return cacheStaleness{
+			NeedsBuild: true, FullRebuild: true,
+			Reason: "cannot verify deletion state",
+		}
 	}
 	if deletedSinceBuild > 0 {
-		return true, fmt.Sprintf(
-			"%d deletions since last cache build",
-			deletedSinceBuild,
-		)
+		result.HasDeleted = true
+		result.FullRebuild = true
+		reasons = append(reasons,
+			fmt.Sprintf("%d deletions", deletedSinceBuild))
 	}
 
 	// Check if parquet files actually exist (directory might be empty)
-	files, _ := filepath.Glob(filepath.Join(messagesDir, "*", "*.parquet"))
+	files, _ := filepath.Glob(
+		filepath.Join(messagesDir, "*", "*.parquet"))
 	if len(files) == 0 {
-		return true, "cache directory empty"
+		result.FullRebuild = true
+		reasons = append(reasons, "cache directory empty")
 	}
 
-	// Check for required parquet tables (e.g. conversations added in a newer version)
 	if missingRequiredParquet(analyticsDir) {
-		return true, "cache missing required tables"
+		result.FullRebuild = true
+		reasons = append(reasons, "cache missing required tables")
+	}
+
+	if len(reasons) > 0 {
+		result.NeedsBuild = true
+		result.Reason = strings.Join(reasons, "; ")
 	}
 
-	return false, ""
+	return result
 }
 
 func init() {