From e5a6c05e38e1e2604b77c81a79c44c2dfe8ba869 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 6 Nov 2025 18:47:06 -0800 Subject: [PATCH] Fix P1 deletion tracking bugs (bd-rbxi) - bd-nqes: Made snapshot capture mandatory with fail-fast - bd-mn9p: Added semantic JSON comparison (jsonEquals) - bd-2ifg: Collect deletion errors and fail operation - bd-8ayj: Atomic file ops with PID-specific temp files - bd-aewm: Added defer cleanup for .merged temp file All tests pass. Amp-Thread-ID: https://ampcode.com/threads/T-5e744954-8a08-4697-960e-5f2a88d50c54 Co-authored-by: Amp --- cmd/bd/daemon_sync.go | 4 ++- cmd/bd/deletion_tracking.go | 62 ++++++++++++++++++++++++++++++------- cmd/bd/sync.go | 4 ++- 3 files changed, 57 insertions(+), 13 deletions(-) diff --git a/cmd/bd/daemon_sync.go b/cmd/bd/daemon_sync.go index 2637ee30..f2a5177f 100644 --- a/cmd/bd/daemon_sync.go +++ b/cmd/bd/daemon_sync.go @@ -500,8 +500,10 @@ func createSyncFunc(ctx context.Context, store storage.Storage, autoCommit, auto log.log("Exported to JSONL") // Capture left snapshot (pre-pull state) for 3-way merge + // This is mandatory for deletion tracking integrity if err := captureLeftSnapshot(jsonlPath); err != nil { - log.log("Warning: failed to capture snapshot for deletion tracking: %v", err) + log.log("Error: failed to capture snapshot (required for deletion tracking): %v", err) + return } if autoCommit { diff --git a/cmd/bd/deletion_tracking.go b/cmd/bd/deletion_tracking.go index eb1b3dc7..8cb5168c 100644 --- a/cmd/bd/deletion_tracking.go +++ b/cmd/bd/deletion_tracking.go @@ -8,11 +8,24 @@ import ( "io" "os" "path/filepath" + "reflect" "github.com/steveyegge/beads/internal/merge" "github.com/steveyegge/beads/internal/storage" ) +// jsonEquals compares two JSON strings semantically, handling field reordering +func jsonEquals(a, b string) bool { + var objA, objB map[string]interface{} + if err := json.Unmarshal([]byte(a), &objA); err != nil { + return false + } + if err := json.Unmarshal([]byte(b), &objB); err != nil { + return false + } + return reflect.DeepEqual(objA, objB) +} + // getSnapshotPaths returns paths for base and left snapshot files func getSnapshotPaths(jsonlPath string) (basePath, leftPath string) { dir := filepath.Dir(jsonlPath) @@ -23,16 +36,30 @@ func getSnapshotPaths(jsonlPath string) (basePath, leftPath string) { // captureLeftSnapshot copies the current JSONL to the left snapshot file // This should be called after export, before git pull +// Uses atomic file operations to prevent race conditions func captureLeftSnapshot(jsonlPath string) error { _, leftPath := getSnapshotPaths(jsonlPath) - return copyFileSnapshot(jsonlPath, leftPath) + // Use process-specific temp file to prevent concurrent write conflicts + tempPath := fmt.Sprintf("%s.%d.tmp", leftPath, os.Getpid()) + if err := copyFileSnapshot(jsonlPath, tempPath); err != nil { + return err + } + // Atomic rename on POSIX systems + return os.Rename(tempPath, leftPath) } // updateBaseSnapshot copies the current JSONL to the base snapshot file // This should be called after successful import to track the new baseline +// Uses atomic file operations to prevent race conditions func updateBaseSnapshot(jsonlPath string) error { basePath, _ := getSnapshotPaths(jsonlPath) - return copyFileSnapshot(jsonlPath, basePath) + // Use process-specific temp file to prevent concurrent write conflicts + tempPath := fmt.Sprintf("%s.%d.tmp", basePath, os.Getpid()) + if err := copyFileSnapshot(jsonlPath, tempPath); err != nil { + return err + } + // Atomic rename on POSIX systems + return os.Rename(tempPath, basePath) } // merge3WayAndPruneDeletions performs 3-way merge and prunes accepted deletions from DB @@ -47,6 +74,13 @@ func merge3WayAndPruneDeletions(ctx context.Context, store storage.Storage, json // Run 3-way merge: base (last import) vs left (pre-pull export) vs right (pulled JSONL) tmpMerged := jsonlPath + ".merged" + // Ensure temp file cleanup on failure + defer func() { + if fileExists(tmpMerged) { + os.Remove(tmpMerged) + } + }() + err := merge.Merge3Way(tmpMerged, basePath, leftPath, jsonlPath, false) if err != nil { // Merge error (including conflicts) is returned as error @@ -70,17 +104,23 @@ func merge3WayAndPruneDeletions(ctx context.Context, store storage.Storage, json DeleteIssue(context.Context, string) error } + d, ok := store.(deleter) + if !ok { + return false, fmt.Errorf("storage backend does not support DeleteIssue") + } + + // Collect all deletion errors - fail the operation if any delete fails + var deletionErrors []error for _, id := range acceptedDeletions { - if d, ok := store.(deleter); ok { - if err := d.DeleteIssue(ctx, id); err != nil { - // Log warning but continue - issue might already be deleted - fmt.Fprintf(os.Stderr, "Warning: failed to delete issue %s during merge: %v\n", id, err) - } - } else { - return false, fmt.Errorf("storage backend does not support DeleteIssue") + if err := d.DeleteIssue(ctx, id); err != nil { + deletionErrors = append(deletionErrors, fmt.Errorf("issue %s: %w", id, err)) } } + if len(deletionErrors) > 0 { + return false, fmt.Errorf("deletion failures (DB may be inconsistent): %v", deletionErrors) + } + if len(acceptedDeletions) > 0 { fmt.Fprintf(os.Stderr, "3-way merge: pruned %d deleted issue(s) from database\n", len(acceptedDeletions)) } @@ -121,8 +161,8 @@ func computeAcceptedDeletions(basePath, leftPath, mergedPath string) ([]string, for id, baseLine := range baseIndex { // Issue in base but not in merged if !mergedIDs[id] { - // Check if unchanged locally (leftLine == baseLine) - if leftLine, existsInLeft := leftIndex[id]; existsInLeft && leftLine == baseLine { + // Check if unchanged locally using semantic JSON comparison + if leftLine, existsInLeft := leftIndex[id]; existsInLeft && jsonEquals(leftLine, baseLine) { deletions = append(deletions, id) } } diff --git a/cmd/bd/sync.go b/cmd/bd/sync.go index 06eacd39..15c3accd 100644 --- a/cmd/bd/sync.go +++ b/cmd/bd/sync.go @@ -150,8 +150,10 @@ Use --merge to merge the sync branch back to main branch.`, } // Capture left snapshot (pre-pull state) for 3-way merge + // This is mandatory for deletion tracking integrity if err := captureLeftSnapshot(jsonlPath); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to capture snapshot for deletion tracking: %v\n", err) + fmt.Fprintf(os.Stderr, "Error: failed to capture snapshot (required for deletion tracking): %v\n", err) + os.Exit(1) } }