diff --git a/cmd/bd/daemon_sync.go b/cmd/bd/daemon_sync.go index aacae268..305f8463 100644 --- a/cmd/bd/daemon_sync.go +++ b/cmd/bd/daemon_sync.go @@ -437,25 +437,29 @@ func performExport(ctx context.Context, store storage.Storage, autoCommit, autoP } log.log("Exported to JSONL") - // Update export metadata for multi-repo support with stable keys - multiRepoPaths := getMultiRepoJSONLPaths() - if multiRepoPaths != nil { - // 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) + // GH#885: Defer metadata updates until AFTER git commit succeeds. + // This is a helper to finalize the export after git operations. + finalizeExportMetadata := func() { + // Update export metadata for multi-repo support with stable keys + multiRepoPaths := getMultiRepoJSONLPaths() + if multiRepoPaths != nil { + // 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) - // This prevents validatePreExport from incorrectly blocking on next export - // with "JSONL is newer than database" after daemon auto-export - dbPath := filepath.Join(beadsDir, "beads.db") - if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil { - log.log("Warning: failed to update database mtime: %v", err) + // Update database mtime to be >= JSONL mtime (fixes #278, #301, #321) + // This prevents validatePreExport from incorrectly blocking on next export + // with "JSONL is newer than database" after daemon auto-export + dbPath := filepath.Join(beadsDir, "beads.db") + if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil { + log.log("Warning: failed to update database mtime: %v", err) + } } // 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 { @@ -720,28 +730,34 @@ func performSync(ctx context.Context, store storage.Storage, autoCommit, autoPus } log.log("Exported to JSONL") - // 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 + // GH#885: Defer metadata updates until AFTER git commit succeeds. + // Define helper to finalize after git operations. dbPath := filepath.Join(beadsDir, "beads.db") - if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil { - log.log("Warning: failed to update database mtime: %v", err) + finalizeExportMetadata := func() { + // 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 // Local-only sync is export-only since there's no remote to sync with if skipGit { + // Git-free mode: finalize immediately since there's no git to wait for + finalizeExportMetadata() log.log("Local %s complete", mode) return } @@ -793,6 +809,9 @@ func performSync(ctx context.Context, store storage.Storage, autoCommit, autoPus log.log("Committed changes") } } + + // GH#885: NOW finalize metadata after git commit succeeded + finalizeExportMetadata() } // Pull (try sync branch first) diff --git a/cmd/bd/sync_export.go b/cmd/bd/sync_export.go index d9230160..ca513dcc 100644 --- a/cmd/bd/sync_export.go +++ b/cmd/bd/sync_export.go @@ -17,34 +17,126 @@ import ( "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 { + 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 + // Note: daemon already handles its own metadata updates if daemonClient != nil { exportArgs := &rpc.ExportArgs{ JSONLPath: jsonlPath, } resp, err := daemonClient.Export(exportArgs) if err != nil { - return fmt.Errorf("daemon export failed: %w", err) + return nil, fmt.Errorf("daemon export failed: %w", err) } 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 // Ensure store is initialized 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) // Tombstones must be exported so they propagate to other clones and prevent resurrection issues, err := store.SearchIssues(ctx, "", types.IssueFilter{IncludeTombstones: true}) 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 @@ -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) } } 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) allDeps, err := store.GetAllDependencyRecords(ctx) 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 { issue.Dependencies = allDeps[issue.ID] @@ -93,7 +185,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error { for _, issue := range issues { labels, err := store.GetLabels(ctx, issue.ID) 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 } @@ -102,7 +194,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error { for _, issue := range issues { comments, err := store.GetIssueComments(ctx, issue.ID) 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 } @@ -112,7 +204,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error { base := filepath.Base(jsonlPath) tempFile, err := os.CreateTemp(dir, base+".tmp.*") 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() defer func() { @@ -125,7 +217,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error { exportedIDs := make([]string, 0, len(issues)) for _, issue := range issues { 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) } @@ -135,7 +227,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error { // Atomic replace 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-------) @@ -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) } - // Clear dirty flags for exported issues - if err := store.ClearDirtyIssuesByID(ctx, exportedIDs); err != nil { - // Non-fatal warning - fmt.Fprintf(os.Stderr, "Warning: failed to clear dirty flags: %v\n", err) - } + // Compute hash and time for the result (but don't update metadata yet) + contentHash, _ := computeJSONLHash(jsonlPath) + exportTime := time.Now().Format(time.RFC3339Nano) - // Clear auto-flush state - clearAutoFlushState() - - // Update jsonl_content_hash metadata to enable content-based staleness detection - // After export, database and JSONL are in sync, so update hash to prevent unnecessary auto-import - if currentHash, err := computeJSONLHash(jsonlPath); err == 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 + return &ExportResult{ + JSONLPath: jsonlPath, + ExportedIDs: exportedIDs, + ContentHash: contentHash, + ExportTime: exportTime, + }, nil } // validateOpenIssuesForSync validates all open issues against their templates