1 files changed, 68 insertions, 9 deletions

diff --git a/internal/persistence/txn.go b/internal/persistence/txn.go
index 3016a19..d09feaa 100644
--- a/internal/persistence/txn.go
+++ b/internal/persistence/txn.go
@@ -29,6 +29,11 @@ import (
 	"path/filepath"
 )
 
+// journalRemoveAttempts bounds the retries of a journal unlink: the unlink is
+// a transaction's commit point, so a transient failure must not be reported
+// as a committed transaction that the next startup will roll back.
+const journalRemoveAttempts = 3
+
 type fileOp struct {
 	// Replace / Delete / Move data
 	Path      string `json:"path"`              // file this record is keyed by (its before-image lives here)
@@ -59,6 +64,51 @@ type Transaction struct {
 	ops        map[string]*fileOp
 	order      []string
 	journalDir string
+	// dirSync makes a journal directory entry durable (nil means syncDir).
+	// It is a field rather than a package-level function so a test can
+	// assert that the commit point is synced and inject a sync failure.
+	dirSync func(string) error
+}
+
+// syncDir fsyncs a directory so an entry created or removed in it survives a
+// crash. POSIX does not tie a file's own fsync to its directory entry, so the
+// journal write, its removal, and every applied file are synced in both.
+func syncDir(dir string) error {
+	d, err := os.Open(dir)
+	if err != nil {
+		return err
+	}
+	defer d.Close()
+	return d.Sync()
+}
+
+// syncJournalDir syncs the transaction's journal directory.
+func (t *Transaction) syncJournalDir() error {
+	if t.dirSync != nil {
+		return t.dirSync(t.journalDir)
+	}
+	return syncDir(t.journalDir)
+}
+
+// removeJournalDurable unlinks a journal and syncs its directory, so the
+// removal itself is durable. The removal is the transaction's commit point: an
+// unlink that a crash forgets resurrects the journal, and Recover then rolls
+// back an operation the caller was told succeeded. A failing remove is retried;
+// a persistent failure is reported as such, with the journal left on disk as
+// the durable record of the transaction (the next startup rolls it back).
+func (t *Transaction) removeJournalDurable(path string) error {
+	var removeErr error
+	for attempt := 0; attempt < journalRemoveAttempts; attempt++ {
+		removeErr = os.Remove(path)
+		if removeErr == nil || os.IsNotExist(removeErr) {
+			if err := t.syncJournalDir(); err != nil {
+				return fmt.Errorf("journal %s removed but its directory could not be synced (a crash may resurrect the journal and roll the transaction back): %w", path, err)
+			}
+			return nil
+		}
+	}
+	return fmt.Errorf("journal %s could not be removed after %d attempts and is retained on disk (the transaction is not durable and will be rolled back at the next startup): %w",
+		path, journalRemoveAttempts, removeErr)
 }
 
 func NewTransaction(journalDir string) *Transaction {
@@ -167,9 +217,10 @@ func (t *Transaction) MarshalJournal() ([]byte, error) {
 	return json.Marshal(journalEntry{Ops: ops})
 }
 
-// Commit writes the journal (durable), applies all operations, and removes
-// the journal. Any failure rolls back every already-applied operation and
-// the journal itself, leaving the filesystem exactly as before the commit.
+// Commit writes the journal (durable), applies all operations, and removes the
+// journal durably, so the commit point itself survives a crash. Any failure
+// rolls back every already-applied operation and the journal itself, leaving
+// the filesystem exactly as before the commit.
 func (t *Transaction) Commit() error {
 	if len(t.order) == 0 {
 		return nil
@@ -200,16 +251,22 @@ func (t *Transaction) Commit() error {
 			// TestCommitKeepsJournalAfterFailedRollback.
 			return fmt.Errorf("%v; rollback failed (journal kept for recovery: %s): %v", err, journalPath, rbErr)
 		}
-		if rErr := os.Remove(journalPath); rErr != nil && !os.IsNotExist(rErr) {
+		// The operations were rolled back, so the leftover journal holds
+		// nothing but the pre-transaction before-images; removing it must be
+		// durable for the same reason the commit point is.
+		if rErr := t.removeJournalDurable(journalPath); rErr != nil {
 			err = fmt.Errorf("%v; journal cleanup failed: %v", err, rErr)
 		}
 		return err
 	}
 
 	// Commit point: the journal is the record of applied operations. After
-	// it is removed the transaction is complete and unrecoverable.
-	if err := os.Remove(journalPath); err != nil && !os.IsNotExist(err) {
-		return fmt.Errorf("commit completed; remove journal: %w", err)
+	// it is removed the transaction is complete and unrecoverable, so the
+	// removal must be durable like every other step: a crash that forgets it
+	// would replay the before-images over operations reported as committed
+	// (and over later writes to the same files).
+	if err := t.removeJournalDurable(journalPath); err != nil {
+		return err
 	}
 	return nil
 }
@@ -322,8 +379,10 @@ func Recover(journalDir string) error {
 		if err := t.rollback(); err != nil {
 			return fmt.Errorf("recover %s: %w", path, err)
 		}
-		if err := os.Remove(path); err != nil {
-			return fmt.Errorf("remove recovered journal %s: %w", path, err)
+		// The rollback is durable only once its journal removal is: a
+		// resurrected journal would roll back later writes to the same files.
+		if err := t.removeJournalDurable(path); err != nil {
+			return err
 		}
 	}
 	return nil