From 153acb7f644d755c45533b7c5311e3ffa47e8973 Mon Sep 17 00:00:00 2001 From: historia <[not public]> Date: Fri, 18 Sep 2026 05:31:09 -0400 Subject: fix: harden engine across casino, economy, concurrency, security, and content --- .../hardening_txn_commit_durability_test.go | 135 +++++++++++++++++++++ internal/persistence/txn.go | 77 ++++++++++-- 2 files changed, 203 insertions(+), 9 deletions(-) create mode 100644 internal/persistence/hardening_txn_commit_durability_test.go (limited to 'internal/persistence') 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 -- cgit v1.2.3