Fix bd-v0y: Remove mtime fast-path in hasJSONLChanged

Git doesn't preserve mtime on checkout, causing false negatives where
hasJSONLChanged() incorrectly returns false after git pull updates JSONL.
This caused bd sync to overwrite pulled JSONL instead of importing it,
resurrecting deleted issues.

Solution: Always compute content hash for comparison (Option 1).
Performance impact is minimal (~10-50ms for sync operations).

Changes:
- cmd/bd/integrity.go: Remove mtime fast-path, always compute hash
- cmd/bd/sync.go: Remove mtime storage after import
- cmd/bd/import.go: Remove mtime storage after import
- cmd/bd/daemon_sync.go: Remove mtime storage and update comments
- cmd/bd/daemon_sync_test.go: Remove mtime assertions from tests

All tests pass. Existing test 'mtime changed but content same - git
operation scenario' verifies the fix works correctly.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
2025-11-23 22:50:29 -08:00
parent 7491c142d4
commit c9a2e7a8b2
6 changed files with 75 additions and 146 deletions

File diff suppressed because one or more lines are too long

View File

@@ -251,10 +251,11 @@ func sanitizeMetadataKey(key string) string {
// In multi-repo mode, keySuffix should be the stable repo identifier (e.g., ".", "../frontend"). // In multi-repo mode, keySuffix should be the stable repo identifier (e.g., ".", "../frontend").
// //
// Metadata key format (bd-ar2.12): // Metadata key format (bd-ar2.12):
// - Single-repo mode: "last_import_hash", "last_import_time", "last_import_mtime" // - Single-repo mode: "last_import_hash", "last_import_time"
// - Multi-repo mode: "last_import_hash:<repo_key>", "last_import_time:<repo_key>", etc. // - Multi-repo mode: "last_import_hash:<repo_key>", "last_import_time:<repo_key>", etc.
// where <repo_key> is a stable repo identifier like "." or "../frontend" // where <repo_key> is a stable repo identifier like "." or "../frontend"
// - Windows paths: Colons in absolute paths (e.g., C:\...) are replaced with underscores (bd-web8) // - Windows paths: Colons in absolute paths (e.g., C:\...) are replaced with underscores (bd-web8)
// - Note: "last_import_mtime" was removed in bd-v0y fix (git doesn't preserve mtime)
// //
// Transaction boundaries (bd-ar2.6): // Transaction boundaries (bd-ar2.6):
// This function does NOT provide atomicity between JSONL write, metadata updates, and DB mtime. // This function does NOT provide atomicity between JSONL write, metadata updates, and DB mtime.
@@ -279,11 +280,9 @@ func updateExportMetadata(ctx context.Context, store storage.Storage, jsonlPath
// Build metadata keys with optional suffix for per-repo tracking // Build metadata keys with optional suffix for per-repo tracking
hashKey := "last_import_hash" hashKey := "last_import_hash"
timeKey := "last_import_time" timeKey := "last_import_time"
mtimeKey := "last_import_mtime"
if keySuffix != "" { if keySuffix != "" {
hashKey += ":" + keySuffix hashKey += ":" + keySuffix
timeKey += ":" + keySuffix timeKey += ":" + keySuffix
mtimeKey += ":" + keySuffix
} }
// Note: Metadata update failures are treated as warnings, not errors (bd-ar2.5). // Note: Metadata update failures are treated as warnings, not errors (bd-ar2.5).
@@ -300,14 +299,7 @@ func updateExportMetadata(ctx context.Context, store storage.Storage, jsonlPath
if err := store.SetMetadata(ctx, timeKey, exportTime); err != nil { if err := store.SetMetadata(ctx, timeKey, exportTime); err != nil {
log.log("Warning: failed to update %s: %v", timeKey, err) log.log("Warning: failed to update %s: %v", timeKey, err)
} }
// Note: mtime tracking removed in bd-v0y fix (git doesn't preserve mtime)
// Store mtime for fast-path optimization
if jsonlInfo, statErr := os.Stat(jsonlPath); statErr == nil {
mtimeStr := fmt.Sprintf("%d", jsonlInfo.ModTime().Unix())
if err := store.SetMetadata(ctx, mtimeKey, mtimeStr); err != nil {
log.log("Warning: failed to update %s: %v", mtimeKey, err)
}
}
} }
// validateDatabaseFingerprint checks that the database belongs to this repository // validateDatabaseFingerprint checks that the database belongs to this repository

View File

@@ -476,24 +476,7 @@ func TestUpdateExportMetadataMultiRepo(t *testing.T) {
t.Error("expected global last_import_hash to not be set when using per-repo keys") t.Error("expected global last_import_hash to not be set when using per-repo keys")
} }
// Verify mtime metadata was also set per-repo (bd-web8: keys are sanitized) // Note: last_import_mtime removed in bd-v0y fix (git doesn't preserve mtime)
mtime1Key := "last_import_mtime:" + sanitizeMetadataKey(jsonlPath1)
mtime1, err := store.GetMetadata(ctx, mtime1Key)
if err != nil {
t.Fatalf("failed to get %s: %v", mtime1Key, err)
}
if mtime1 == "" {
t.Errorf("expected %s to be set", mtime1Key)
}
mtime2Key := "last_import_mtime:" + sanitizeMetadataKey(jsonlPath2)
mtime2, err := store.GetMetadata(ctx, mtime2Key)
if err != nil {
t.Fatalf("failed to get %s: %v", mtime2Key, err)
}
if mtime2 == "" {
t.Errorf("expected %s to be set", mtime2Key)
}
} }
// TestExportWithMultiRepoConfigUpdatesAllMetadata verifies that export with multi-repo // TestExportWithMultiRepoConfigUpdatesAllMetadata verifies that export with multi-repo
@@ -604,14 +587,7 @@ func TestExportWithMultiRepoConfigUpdatesAllMetadata(t *testing.T) {
t.Errorf("expected %s to be set after export", primaryTimeKey) t.Errorf("expected %s to be set after export", primaryTimeKey)
} }
primaryMtimeKey := "last_import_mtime:" + sanitizeMetadataKey(primaryDir) // Note: last_import_mtime removed in bd-v0y fix (git doesn't preserve mtime)
primaryMtime, err := store.GetMetadata(ctx, primaryMtimeKey)
if err != nil {
t.Fatalf("failed to get %s: %v", primaryMtimeKey, err)
}
if primaryMtime == "" {
t.Errorf("expected %s to be set after export", primaryMtimeKey)
}
// Verify metadata for additional repo (bd-web8: keys are sanitized) // Verify metadata for additional repo (bd-web8: keys are sanitized)
additionalHashKey := "last_import_hash:" + sanitizeMetadataKey(additionalDir) additionalHashKey := "last_import_hash:" + sanitizeMetadataKey(additionalDir)
@@ -632,14 +608,7 @@ func TestExportWithMultiRepoConfigUpdatesAllMetadata(t *testing.T) {
t.Errorf("expected %s to be set after export", additionalTimeKey) t.Errorf("expected %s to be set after export", additionalTimeKey)
} }
additionalMtimeKey := "last_import_mtime:" + sanitizeMetadataKey(additionalDir) // Note: last_import_mtime removed in bd-v0y fix (git doesn't preserve mtime)
additionalMtime, err := store.GetMetadata(ctx, additionalMtimeKey)
if err != nil {
t.Fatalf("failed to get %s: %v", additionalMtimeKey, err)
}
if additionalMtime == "" {
t.Errorf("expected %s to be set after export", additionalMtimeKey)
}
// Note: In this test both JSONL files have the same content (all issues), // Note: In this test both JSONL files have the same content (all issues),
// so hashes will be identical. In real multi-repo mode, ExportToMultiRepo // so hashes will be identical. In real multi-repo mode, ExportToMultiRepo

View File

@@ -337,14 +337,7 @@ NOTE: Import requires direct database access and does not work with daemon mode.
// Non-fatal warning (see above comment about graceful degradation) // Non-fatal warning (see above comment about graceful degradation)
debug.Logf("Warning: failed to update last_import_time: %v", err) debug.Logf("Warning: failed to update last_import_time: %v", err)
} }
// Store mtime for fast-path optimization in hasJSONLChanged (bd-3bg) // Note: mtime tracking removed in bd-v0y fix (git doesn't preserve mtime)
if jsonlInfo, statErr := os.Stat(input); statErr == nil {
mtimeStr := fmt.Sprintf("%d", jsonlInfo.ModTime().Unix())
if err := store.SetMetadata(ctx, "last_import_mtime", mtimeStr); err != nil {
// Non-fatal warning (see above comment about graceful degradation)
debug.Logf("Warning: failed to update last_import_mtime: %v", err)
}
}
} else { } else {
debug.Logf("Warning: failed to read JSONL for hash update: %v", err) debug.Logf("Warning: failed to read JSONL for hash update: %v", err)
} }

View File

@@ -106,32 +106,14 @@ func hasJSONLChanged(ctx context.Context, store storage.Storage, jsonlPath strin
// Build metadata keys with optional suffix for per-repo tracking (bd-ar2.10, bd-ar2.11) // Build metadata keys with optional suffix for per-repo tracking (bd-ar2.10, bd-ar2.11)
hashKey := "last_import_hash" hashKey := "last_import_hash"
mtimeKey := "last_import_mtime"
if keySuffix != "" { if keySuffix != "" {
hashKey += ":" + keySuffix hashKey += ":" + keySuffix
mtimeKey += ":" + keySuffix
} }
// Fast-path: Check mtime first to avoid expensive hash computation // Always compute content hash (bd-v0y fix)
// Get last known mtime from metadata // Previous mtime-based fast-path was unsafe: git operations (pull, checkout, rebase)
lastMtimeStr, err := store.GetMetadata(ctx, mtimeKey) // can change file content without updating mtime, causing false negatives.
if err == nil && lastMtimeStr != "" { // Hash computation is fast enough for sync operations (~10-50ms even for large DBs).
// We have a previous mtime - check if file mtime changed
jsonlInfo, statErr := os.Stat(jsonlPath)
if statErr == nil {
currentMtime := jsonlInfo.ModTime().Unix()
currentMtimeStr := fmt.Sprintf("%d", currentMtime)
// If mtime unchanged, content definitely unchanged (filesystem guarantee)
// Skip expensive hash computation
if currentMtimeStr == lastMtimeStr {
return false
}
// Mtime changed - fall through to hash comparison (could be git operation)
}
}
// Slow-path: Compute content hash (either mtime changed or no mtime metadata)
currentHash, err := computeJSONLHash(jsonlPath) currentHash, err := computeJSONLHash(jsonlPath)
if err != nil { if err != nil {
// If we can't read JSONL, assume no change (don't auto-import broken files) // If we can't read JSONL, assume no change (don't auto-import broken files)

View File

@@ -749,14 +749,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
// Non-fatal warning (see above comment about graceful degradation) // Non-fatal warning (see above comment about graceful degradation)
fmt.Fprintf(os.Stderr, "Warning: failed to update last_import_time: %v\n", err) fmt.Fprintf(os.Stderr, "Warning: failed to update last_import_time: %v\n", err)
} }
// Store mtime for fast-path optimization in hasJSONLChanged (bd-3bg) // Note: mtime tracking removed in bd-v0y fix (git doesn't preserve mtime)
if jsonlInfo, statErr := os.Stat(jsonlPath); statErr == nil {
mtimeStr := fmt.Sprintf("%d", jsonlInfo.ModTime().Unix())
if err := store.SetMetadata(ctx, "last_import_mtime", mtimeStr); err != nil {
// Non-fatal warning (see above comment about graceful degradation)
fmt.Fprintf(os.Stderr, "Warning: failed to update last_import_mtime: %v\n", err)
}
}
} }
// Update database mtime to be >= JSONL mtime (fixes #278, #301, #321) // Update database mtime to be >= JSONL mtime (fixes #278, #301, #321)