fix: harden engine across casino, economy, concurrency, security, and content

2 files changed, 203 insertions, 9 deletions

diff --git a/internal/persistence/hardening_txn_commit_durability_test.go b/internal/persistence/hardening_txn_commit_durability_test.go
new file mode 100644
index 0000000..f02bc9f
--- /dev/null
+++ b/internal/persistence/hardening_txn_commit_durability_test.go
@@ -0,0 +1,135 @@
+package persistence
+
+// Commit-point durability regressions: the unlink that completes a
+// transaction is the commit point, so it must be retried and made durable
+// exactly like the journal write and every applied file. Otherwise a crash
+// resurrects the journal and Recover rolls back an operation the caller was
+// told succeeded.
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestCommitPointSyncsJournalDirectory: the journal removal is followed by a
+// sync of the journal directory. Without it the unlink may not survive a
+// crash and the replayed before-images revert committed work (and clobber
+// later writes to the same files).
+func TestCommitPointSyncsJournalDirectory(t *testing.T) {
+	dir := t.TempDir()
+	journalDir := filepath.Join(dir, ".journal")
+	writeTestFile(t, filepath.Join(dir, "a.yaml"), "old-a")
+
+	txn := NewTransaction(journalDir)
+	if err := txn.Replace(filepath.Join(dir, "a.yaml"), []byte("new-a"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	var synced []string
+	txn.dirSync = func(d string) error {
+		synced = append(synced, d)
+		return syncDir(d)
+	}
+	if err := txn.Commit(); err != nil {
+		t.Fatalf("commit: %v", err)
+	}
+	if len(synced) == 0 {
+		t.Fatal("commit point removed the journal without syncing the journal directory: a crash can resurrect it and roll back the committed transaction")
+	}
+	for _, d := range synced {
+		if d != journalDir {
+			t.Fatalf("synced %s, want the journal directory %s", d, journalDir)
+		}
+	}
+	// The commit point holds: the journal is gone and nothing is rolled back.
+	if err := Recover(journalDir); err != nil {
+		t.Fatalf("recover after commit: %v", err)
+	}
+	if got := readTestFile(t, filepath.Join(dir, "a.yaml")); got != "new-a" {
+		t.Fatalf("committed write reverted: %q", got)
+	}
+}
+
+// TestCommitReportsNonDurableCommitPoint: when the directory sync after the
+// removal fails, the removal may not survive a crash, so Commit must not
+// report a completed transaction: the error names the rollback risk.
+func TestCommitReportsNonDurableCommitPoint(t *testing.T) {
+	dir := t.TempDir()
+	journalDir := filepath.Join(dir, ".journal")
+	writeTestFile(t, filepath.Join(dir, "a.yaml"), "old-a")
+
+	txn := NewTransaction(journalDir)
+	if err := txn.Replace(filepath.Join(dir, "a.yaml"), []byte("new-a"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	txn.dirSync = func(string) error { return errors.New("injected sync failure") }
+	err := txn.Commit()
+	if err == nil {
+		t.Fatal("a non-durable commit point must be reported as a failure")
+	}
+	if !strings.Contains(err.Error(), "could not be synced") {
+		t.Fatalf("error must name the unsynced removal, got: %v", err)
+	}
+	// The journal is gone from the visible filesystem but its removal is not
+	// durable: recovery at the next startup may roll the operation back.
+	if entries, _ := ListJournalFiles(journalDir); len(entries) != 0 {
+		t.Fatalf("journal removal did not happen: %v", entries)
+	}
+}
+
+// TestRemoveJournalRetainsJournalOnPersistentFailure: when the unlink itself
+// keeps failing, Commit must return an error that names the retained journal
+// and the rollback that will follow, and the journal must stay on disk as the
+// durable record. Once the fault is repaired the removal succeeds.
+//
+// The failure is injected with a journal path that cannot be unlinked (a
+// non-empty directory, which unlink refuses regardless of privilege) instead
+// of a read-only directory: a chmod-based injection is a no-op for root, so
+// under the root-owned test runner the test used to skip and never executed
+// the retained-journal branch at all.
+func TestRemoveJournalRetainsJournalOnPersistentFailure(t *testing.T) {
+	dir := t.TempDir()
+	journalDir := filepath.Join(dir, ".journal")
+	if err := os.MkdirAll(journalDir, 0700); err != nil {
+		t.Fatal(err)
+	}
+	journalPath := filepath.Join(journalDir, "txn-test.json")
+	if err := os.Mkdir(journalPath, 0700); err != nil {
+		t.Fatal(err)
+	}
+	blocker := filepath.Join(journalPath, "keep")
+	if err := os.WriteFile(blocker, []byte("keep"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Remove(journalPath); err == nil || os.IsNotExist(err) {
+		t.Fatalf("removing a non-empty journal path unexpectedly succeeded: a removal failure cannot be injected")
+	}
+
+	txn := NewTransaction(journalDir)
+	err := txn.removeJournalDurable(journalPath)
+	if err == nil {
+		t.Fatal("a persistent journal removal failure must be reported, not swallowed")
+	}
+	for _, want := range []string{"retained", "rolled back"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Fatalf("error must state that the journal is %q, got: %v", want, err)
+		}
+	}
+	if _, statErr := os.Stat(journalPath); statErr != nil {
+		t.Fatalf("the retained journal is the durable record and must stay on disk: %v", statErr)
+	}
+
+	// Repair: the path is unlinkable again, so the next attempt removes the
+	// journal and succeeds.
+	if err := os.Remove(blocker); err != nil {
+		t.Fatal(err)
+	}
+	if err := txn.removeJournalDurable(journalPath); err != nil {
+		t.Fatalf("removal after repair: %v", err)
+	}
+	if _, statErr := os.Stat(journalPath); !os.IsNotExist(statErr) {
+		t.Fatalf("journal not removed after repair: %v", statErr)
+	}
+}
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