fix: defer SQLite metadata updates until after git commit (GH#885)

This fixes the atomicity gap where exportToJSONL would update SQLite
metadata (clear dirty flags, update content hash, last_import_time)
BEFORE the git commit. If git commit failed, SQLite would incorrectly
indicate the sync succeeded.

Changes:
- Add ExportResult struct to capture export metadata for deferred finalization
- Add exportToJSONLDeferred() that exports without updating metadata
- Add finalizeExport() to update metadata after git commit succeeds
- Update daemon_sync.go sync flows to defer metadata updates

Now the sync flow is truly atomic: metadata only updates after git commit.

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

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

Executed-By: beads/crew/dave
Rig: beads
Role: crew
This commit is contained in:
beads/crew/dave
2026-01-04 15:15:52 -08:00
committed by Steve Yegge
parent 68f5bb24f8
commit ede940b5d2
2 changed files with 166 additions and 82 deletions
+52 -33
View File
@@ -437,25 +437,29 @@ func performExport(ctx context.Context, store storage.Storage, autoCommit, autoP
} }
log.log("Exported to JSONL") log.log("Exported to JSONL")
// Update export metadata for multi-repo support with stable keys // GH#885: Defer metadata updates until AFTER git commit succeeds.
multiRepoPaths := getMultiRepoJSONLPaths() // This is a helper to finalize the export after git operations.
if multiRepoPaths != nil { finalizeExportMetadata := func() {
// Multi-repo mode: update metadata for each JSONL with stable repo key // Update export metadata for multi-repo support with stable keys
for _, path := range multiRepoPaths { multiRepoPaths := getMultiRepoJSONLPaths()
repoKey := getRepoKeyForPath(path) if multiRepoPaths != nil {
updateExportMetadata(exportCtx, store, path, log, repoKey) // Multi-repo mode: update metadata for each JSONL with stable repo key
for _, path := range multiRepoPaths {
repoKey := getRepoKeyForPath(path)
updateExportMetadata(exportCtx, store, path, log, repoKey)
}
} else {
// Single-repo mode: update metadata for main JSONL
updateExportMetadata(exportCtx, store, jsonlPath, log, "")
} }
} else {
// Single-repo mode: update metadata for main JSONL
updateExportMetadata(exportCtx, store, jsonlPath, log, "")
}
// Update database mtime to be >= JSONL mtime (fixes #278, #301, #321) // Update database mtime to be >= JSONL mtime (fixes #278, #301, #321)
// This prevents validatePreExport from incorrectly blocking on next export // This prevents validatePreExport from incorrectly blocking on next export
// with "JSONL is newer than database" after daemon auto-export // with "JSONL is newer than database" after daemon auto-export
dbPath := filepath.Join(beadsDir, "beads.db") dbPath := filepath.Join(beadsDir, "beads.db")
if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil { if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil {
log.log("Warning: failed to update database mtime: %v", err) log.log("Warning: failed to update database mtime: %v", err)
}
} }
// Auto-commit if enabled (skip in git-free mode) // Auto-commit if enabled (skip in git-free mode)
@@ -497,6 +501,12 @@ func performExport(ctx context.Context, store storage.Storage, autoCommit, autoP
} }
} }
} }
// GH#885: NOW finalize metadata after git commit succeeded
finalizeExportMetadata()
} else if skipGit {
// Git-free mode: finalize immediately since there's no git to wait for
finalizeExportMetadata()
} }
if skipGit { if skipGit {
@@ -720,28 +730,34 @@ func performSync(ctx context.Context, store storage.Storage, autoCommit, autoPus
} }
log.log("Exported to JSONL") log.log("Exported to JSONL")
// Update export metadata for multi-repo support with stable keys // GH#885: Defer metadata updates until AFTER git commit succeeds.
if multiRepoPaths != nil { // Define helper to finalize after git operations.
// Multi-repo mode: update metadata for each JSONL with stable repo key
for _, path := range multiRepoPaths {
repoKey := getRepoKeyForPath(path)
updateExportMetadata(syncCtx, store, path, log, repoKey)
}
} else {
// Single-repo mode: update metadata for main JSONL
updateExportMetadata(syncCtx, store, jsonlPath, log, "")
}
// Update database mtime to be >= JSONL mtime
// This prevents validatePreExport from incorrectly blocking on next export
dbPath := filepath.Join(beadsDir, "beads.db") dbPath := filepath.Join(beadsDir, "beads.db")
if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil { finalizeExportMetadata := func() {
log.log("Warning: failed to update database mtime: %v", err) // Update export metadata for multi-repo support with stable keys
if multiRepoPaths != nil {
// Multi-repo mode: update metadata for each JSONL with stable repo key
for _, path := range multiRepoPaths {
repoKey := getRepoKeyForPath(path)
updateExportMetadata(syncCtx, store, path, log, repoKey)
}
} else {
// Single-repo mode: update metadata for main JSONL
updateExportMetadata(syncCtx, store, jsonlPath, log, "")
}
// Update database mtime to be >= JSONL mtime
// This prevents validatePreExport from incorrectly blocking on next export
if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil {
log.log("Warning: failed to update database mtime: %v", err)
}
} }
// Skip git operations, snapshot capture, deletion tracking, and import in local-only mode // Skip git operations, snapshot capture, deletion tracking, and import in local-only mode
// Local-only sync is export-only since there's no remote to sync with // Local-only sync is export-only since there's no remote to sync with
if skipGit { if skipGit {
// Git-free mode: finalize immediately since there's no git to wait for
finalizeExportMetadata()
log.log("Local %s complete", mode) log.log("Local %s complete", mode)
return return
} }
@@ -793,6 +809,9 @@ func performSync(ctx context.Context, store storage.Storage, autoCommit, autoPus
log.log("Committed changes") log.log("Committed changes")
} }
} }
// GH#885: NOW finalize metadata after git commit succeeded
finalizeExportMetadata()
} }
// Pull (try sync branch first) // Pull (try sync branch first)
+114 -49
View File
@@ -17,34 +17,126 @@ import (
"github.com/steveyegge/beads/internal/validation" "github.com/steveyegge/beads/internal/validation"
) )
// exportToJSONL exports the database to JSONL format // ExportResult contains information needed to finalize an export after git commit.
// This enables atomic sync by deferring metadata updates until after git commit succeeds.
// See GH#885 for the atomicity gap this fixes.
type ExportResult struct {
// JSONLPath is the path to the exported JSONL file
JSONLPath string
// ExportedIDs are the issue IDs that were exported
ExportedIDs []string
// ContentHash is the hash of the exported JSONL content
ContentHash string
// ExportTime is when the export was performed (RFC3339Nano format)
ExportTime string
}
// finalizeExport updates SQLite metadata after a successful git commit.
// This is the second half of atomic sync - it marks the export as complete
// only after the git commit succeeds. If git commit fails, the metadata
// remains unchanged so the system knows the sync is incomplete.
// See GH#885 for the atomicity gap this fixes.
func finalizeExport(ctx context.Context, result *ExportResult) {
if result == nil {
return
}
// Ensure store is initialized
if err := ensureStoreActive(); err != nil {
fmt.Fprintf(os.Stderr, "Warning: failed to initialize store for finalize: %v\n", err)
return
}
// Clear dirty flags for exported issues
if len(result.ExportedIDs) > 0 {
if err := store.ClearDirtyIssuesByID(ctx, result.ExportedIDs); err != nil {
// Non-fatal warning
fmt.Fprintf(os.Stderr, "Warning: failed to clear dirty flags: %v\n", err)
}
}
// Clear auto-flush state
clearAutoFlushState()
// Update jsonl_content_hash metadata to enable content-based staleness detection
if result.ContentHash != "" {
if err := store.SetMetadata(ctx, "jsonl_content_hash", result.ContentHash); err != nil {
// Non-fatal warning: Metadata update failures are intentionally non-fatal to prevent blocking
// successful exports. System degrades gracefully to mtime-based staleness detection if metadata
// is unavailable. This ensures export operations always succeed even if metadata storage fails.
fmt.Fprintf(os.Stderr, "Warning: failed to update jsonl_content_hash: %v\n", err)
}
}
// Update last_import_time
if result.ExportTime != "" {
if err := store.SetMetadata(ctx, "last_import_time", result.ExportTime); err != nil {
// Non-fatal warning (see above comment about graceful degradation)
fmt.Fprintf(os.Stderr, "Warning: failed to update last_import_time: %v\n", err)
}
}
// Update database mtime to be >= JSONL mtime (fixes #278, #301, #321)
// This prevents validatePreExport from incorrectly blocking on next export
if result.JSONLPath != "" {
beadsDir := filepath.Dir(result.JSONLPath)
dbPath := filepath.Join(beadsDir, "beads.db")
if err := TouchDatabaseFile(dbPath, result.JSONLPath); err != nil {
// Non-fatal warning
fmt.Fprintf(os.Stderr, "Warning: failed to update database mtime: %v\n", err)
}
}
}
// exportToJSONL exports the database to JSONL format.
// This is a convenience wrapper that exports and immediately finalizes.
// For atomic sync operations, use exportToJSONLDeferred + finalizeExport.
func exportToJSONL(ctx context.Context, jsonlPath string) error { func exportToJSONL(ctx context.Context, jsonlPath string) error {
result, err := exportToJSONLDeferred(ctx, jsonlPath)
if err != nil {
return err
}
// Immediately finalize for backward compatibility
finalizeExport(ctx, result)
return nil
}
// exportToJSONLDeferred exports the database to JSONL format but does NOT update
// SQLite metadata. The caller must call finalizeExport() after git commit succeeds.
// This enables atomic sync where metadata is only updated after git commit.
// See GH#885 for the atomicity gap this fixes.
func exportToJSONLDeferred(ctx context.Context, jsonlPath string) (*ExportResult, error) {
// If daemon is running, use RPC // If daemon is running, use RPC
// Note: daemon already handles its own metadata updates
if daemonClient != nil { if daemonClient != nil {
exportArgs := &rpc.ExportArgs{ exportArgs := &rpc.ExportArgs{
JSONLPath: jsonlPath, JSONLPath: jsonlPath,
} }
resp, err := daemonClient.Export(exportArgs) resp, err := daemonClient.Export(exportArgs)
if err != nil { if err != nil {
return fmt.Errorf("daemon export failed: %w", err) return nil, fmt.Errorf("daemon export failed: %w", err)
} }
if !resp.Success { if !resp.Success {
return fmt.Errorf("daemon export error: %s", resp.Error) return nil, fmt.Errorf("daemon export error: %s", resp.Error)
} }
return nil // Daemon handles its own metadata updates, return nil result
return nil, nil
} }
// Direct mode: access store directly // Direct mode: access store directly
// Ensure store is initialized // Ensure store is initialized
if err := ensureStoreActive(); err != nil { if err := ensureStoreActive(); err != nil {
return fmt.Errorf("failed to initialize store: %w", err) return nil, fmt.Errorf("failed to initialize store: %w", err)
} }
// Get all issues including tombstones for sync propagation (bd-rp4o fix) // Get all issues including tombstones for sync propagation (bd-rp4o fix)
// Tombstones must be exported so they propagate to other clones and prevent resurrection // Tombstones must be exported so they propagate to other clones and prevent resurrection
issues, err := store.SearchIssues(ctx, "", types.IssueFilter{IncludeTombstones: true}) issues, err := store.SearchIssues(ctx, "", types.IssueFilter{IncludeTombstones: true})
if err != nil { if err != nil {
return fmt.Errorf("failed to get issues: %w", err) return nil, fmt.Errorf("failed to get issues: %w", err)
} }
// Safety check: prevent exporting empty database over non-empty JSONL // Safety check: prevent exporting empty database over non-empty JSONL
@@ -59,7 +151,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
fmt.Fprintf(os.Stderr, "Warning: failed to read existing JSONL: %v\n", countErr) fmt.Fprintf(os.Stderr, "Warning: failed to read existing JSONL: %v\n", countErr)
} }
} else if existingCount > 0 { } else if existingCount > 0 {
return fmt.Errorf("refusing to export empty database over non-empty JSONL file (database: 0 issues, JSONL: %d issues)", existingCount) return nil, fmt.Errorf("refusing to export empty database over non-empty JSONL file (database: 0 issues, JSONL: %d issues)", existingCount)
} }
} }
@@ -83,7 +175,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
// Populate dependencies for all issues (avoid N+1) // Populate dependencies for all issues (avoid N+1)
allDeps, err := store.GetAllDependencyRecords(ctx) allDeps, err := store.GetAllDependencyRecords(ctx)
if err != nil { if err != nil {
return fmt.Errorf("failed to get dependencies: %w", err) return nil, fmt.Errorf("failed to get dependencies: %w", err)
} }
for _, issue := range issues { for _, issue := range issues {
issue.Dependencies = allDeps[issue.ID] issue.Dependencies = allDeps[issue.ID]
@@ -93,7 +185,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
for _, issue := range issues { for _, issue := range issues {
labels, err := store.GetLabels(ctx, issue.ID) labels, err := store.GetLabels(ctx, issue.ID)
if err != nil { if err != nil {
return fmt.Errorf("failed to get labels for %s: %w", issue.ID, err) return nil, fmt.Errorf("failed to get labels for %s: %w", issue.ID, err)
} }
issue.Labels = labels issue.Labels = labels
} }
@@ -102,7 +194,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
for _, issue := range issues { for _, issue := range issues {
comments, err := store.GetIssueComments(ctx, issue.ID) comments, err := store.GetIssueComments(ctx, issue.ID)
if err != nil { if err != nil {
return fmt.Errorf("failed to get comments for %s: %w", issue.ID, err) return nil, fmt.Errorf("failed to get comments for %s: %w", issue.ID, err)
} }
issue.Comments = comments issue.Comments = comments
} }
@@ -112,7 +204,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
base := filepath.Base(jsonlPath) base := filepath.Base(jsonlPath)
tempFile, err := os.CreateTemp(dir, base+".tmp.*") tempFile, err := os.CreateTemp(dir, base+".tmp.*")
if err != nil { if err != nil {
return fmt.Errorf("failed to create temp file: %w", err) return nil, fmt.Errorf("failed to create temp file: %w", err)
} }
tempPath := tempFile.Name() tempPath := tempFile.Name()
defer func() { defer func() {
@@ -125,7 +217,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
exportedIDs := make([]string, 0, len(issues)) exportedIDs := make([]string, 0, len(issues))
for _, issue := range issues { for _, issue := range issues {
if err := encoder.Encode(issue); err != nil { if err := encoder.Encode(issue); err != nil {
return fmt.Errorf("failed to encode issue %s: %w", issue.ID, err) return nil, fmt.Errorf("failed to encode issue %s: %w", issue.ID, err)
} }
exportedIDs = append(exportedIDs, issue.ID) exportedIDs = append(exportedIDs, issue.ID)
} }
@@ -135,7 +227,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
// Atomic replace // Atomic replace
if err := os.Rename(tempPath, jsonlPath); err != nil { if err := os.Rename(tempPath, jsonlPath); err != nil {
return fmt.Errorf("failed to replace JSONL file: %w", err) return nil, fmt.Errorf("failed to replace JSONL file: %w", err)
} }
// Set appropriate file permissions (0600: rw-------) // Set appropriate file permissions (0600: rw-------)
@@ -144,43 +236,16 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error {
fmt.Fprintf(os.Stderr, "Warning: failed to set file permissions: %v\n", err) fmt.Fprintf(os.Stderr, "Warning: failed to set file permissions: %v\n", err)
} }
// Clear dirty flags for exported issues // Compute hash and time for the result (but don't update metadata yet)
if err := store.ClearDirtyIssuesByID(ctx, exportedIDs); err != nil { contentHash, _ := computeJSONLHash(jsonlPath)
// Non-fatal warning exportTime := time.Now().Format(time.RFC3339Nano)
fmt.Fprintf(os.Stderr, "Warning: failed to clear dirty flags: %v\n", err)
}
// Clear auto-flush state return &ExportResult{
clearAutoFlushState() JSONLPath: jsonlPath,
ExportedIDs: exportedIDs,
// Update jsonl_content_hash metadata to enable content-based staleness detection ContentHash: contentHash,
// After export, database and JSONL are in sync, so update hash to prevent unnecessary auto-import ExportTime: exportTime,
if currentHash, err := computeJSONLHash(jsonlPath); err == nil { }, nil
if err := store.SetMetadata(ctx, "jsonl_content_hash", currentHash); err != nil {
// Non-fatal warning: Metadata update failures are intentionally non-fatal to prevent blocking
// successful exports. System degrades gracefully to mtime-based staleness detection if metadata
// is unavailable. This ensures export operations always succeed even if metadata storage fails.
fmt.Fprintf(os.Stderr, "Warning: failed to update jsonl_content_hash: %v\n", err)
}
// Use RFC3339Nano for nanosecond precision to avoid race with file mtime (fixes #399)
exportTime := time.Now().Format(time.RFC3339Nano)
if err := store.SetMetadata(ctx, "last_import_time", exportTime); err != nil {
// Non-fatal warning (see above comment about graceful degradation)
fmt.Fprintf(os.Stderr, "Warning: failed to update last_import_time: %v\n", err)
}
// Note: mtime tracking removed because git doesn't preserve mtime
}
// Update database mtime to be >= JSONL mtime (fixes #278, #301, #321)
// This prevents validatePreExport from incorrectly blocking on next export
beadsDir := filepath.Dir(jsonlPath)
dbPath := filepath.Join(beadsDir, "beads.db")
if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil {
// Non-fatal warning
fmt.Fprintf(os.Stderr, "Warning: failed to update database mtime: %v\n", err)
}
return nil
} }
// validateOpenIssuesForSync validates all open issues against their templates // validateOpenIssuesForSync validates all open issues against their templates