From 5ad42a51df4dcc5ccc3aa6a6d902085ce2ee4239 Mon Sep 17 00:00:00 2001 From: historia <[not public]> Date: Fri, 18 Sep 2026 18:08:01 -0400 Subject: fix: journal multi-file room/entity edits; simplify queued-position lookup --- internal/admin/api_rooms.go | 99 +++++++++++++++++++++------- internal/admin/room_txn_atomic_test.go | 115 +++++++++++++++++++++++++++++++++ internal/admin/yaml_util.go | 36 +++++++---- internal/game/cmd_queued.go | 13 +--- 4 files changed, 216 insertions(+), 47 deletions(-) create mode 100644 internal/admin/room_txn_atomic_test.go (limited to 'internal') diff --git a/internal/admin/api_rooms.go b/internal/admin/api_rooms.go index 7f317cc..015c0ee 100644 --- a/internal/admin/api_rooms.go +++ b/internal/admin/api_rooms.go @@ -802,8 +802,16 @@ func (s *AdminServer) handleRoomLink(w http.ResponseWriter, r *http.Request) { } } - var pushEntries []ChangeDesc - + // Resolve and serialize both sides before any write, then apply them + // in one journaled transaction (matching the link POST path): a + // failure mid-unlink must not leave an asymmetric link with no undo + // record to repair it. + type unlinkWrite struct { + desc ChangeDesc + path string + data []byte + } + var writes []unlinkWrite if roomA != nil && (body.Side == "" || body.Side == "from") { pathA, okA2 := s.world.GetRoomPath(body.From) if !okA2 { @@ -811,17 +819,17 @@ func (s *AdminServer) handleRoomLink(w http.ResponseWriter, r *http.Request) { return } oldA, _ := snapshotFile(pathA) - newA, err := writeYAMLFile(pathA, roomA) + newA, err := marshalYAML(roomA) if err != nil { - writeJSONError(w, "write A: "+err.Error(), http.StatusInternalServerError) + writeJSONError(w, "serialize A: "+err.Error(), http.StatusInternalServerError) return } - pushEntries = append(pushEntries, ChangeDesc{ + writes = append(writes, unlinkWrite{ChangeDesc{ Description: fmt.Sprintf("unlink %d %s %d", body.From, dirA, body.To), FilePath: pathA, OldContent: oldA, NewContent: newA, - }) + }, pathA, newA}) } if roomB != nil && (body.Side == "" || body.Side == "to") { @@ -831,21 +839,35 @@ func (s *AdminServer) handleRoomLink(w http.ResponseWriter, r *http.Request) { return } oldB, _ := snapshotFile(pathB) - newB, err := writeYAMLFile(pathB, roomB) + newB, err := marshalYAML(roomB) if err != nil { - writeJSONError(w, "write B: "+err.Error(), http.StatusInternalServerError) + writeJSONError(w, "serialize B: "+err.Error(), http.StatusInternalServerError) return } - pushEntries = append(pushEntries, ChangeDesc{ + writes = append(writes, unlinkWrite{ChangeDesc{ Description: fmt.Sprintf("unlink %d %s %d (reverse)", body.To, dirB, body.From), FilePath: pathB, OldContent: oldB, NewContent: newB, - }) + }, pathB, newB}) + } + + if len(writes) > 0 { + txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + for _, wr := range writes { + if err := txn.Replace(wr.path, wr.data, 0644); err != nil { + writeJSONError(w, "plan unlink: "+err.Error(), http.StatusInternalServerError) + return + } + } + if err := txn.Commit(); err != nil { + writeJSONError(w, "commit unlink: "+err.Error(), http.StatusInternalServerError) + return + } } - for _, entry := range pushEntries { - s.undoStack.Push(entry) + for _, wr := range writes { + s.undoStack.Push(wr.desc) } writeJSON(w, map[string]any{"ok": true}) @@ -893,12 +915,15 @@ func (s *AdminServer) handleRoomMove(w http.ResponseWriter, r *http.Request) { failJSON(w, "read: "+err.Error()) return } - if err := os.WriteFile(newPath, data, 0644); err != nil { - failJSON(w, "write: "+err.Error()) + // Move the file in one journaled transaction: a failed destination write + // or source removal must not leave the same room ID in two directories. + txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + if err := txn.Move(oldPath, newPath); err != nil { + failJSON(w, "plan move: "+err.Error()) return } - if err := os.Remove(oldPath); err != nil { - failJSON(w, "remove old: "+err.Error()) + if err := txn.Commit(); err != nil { + failJSON(w, "move: "+err.Error()) return } s.world.RebuildRoomIndex(s.dataDir) @@ -1175,15 +1200,30 @@ func (s *AdminServer) handleRoomSwap(w http.ResponseWriter, r *http.Request) { newB["id"] = idB newB["exits"] = exitsB - newAContent, err := writeMapAsYAML(aPath, newA) + newAContent, err := marshalMapYAML(newA) if err != nil { - writeJSONError(w, "write A: "+err.Error(), http.StatusInternalServerError) + writeJSONError(w, "serialize A: "+err.Error(), http.StatusInternalServerError) return } - newBContent, err := writeMapAsYAML(bPath, newB) + newBContent, err := marshalMapYAML(newB) if err != nil { - os.WriteFile(aPath, oldA, 0644) - writeJSONError(w, "write B: "+err.Error(), http.StatusInternalServerError) + writeJSONError(w, "serialize B: "+err.Error(), http.StatusInternalServerError) + return + } + + // Apply both files in one journaled transaction so a failure cannot leave + // the two rooms half-swapped. + txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + if err := txn.Replace(aPath, newAContent, 0644); err != nil { + writeJSONError(w, "plan write A: "+err.Error(), http.StatusInternalServerError) + return + } + if err := txn.Replace(bPath, newBContent, 0644); err != nil { + writeJSONError(w, "plan write B: "+err.Error(), http.StatusInternalServerError) + return + } + if err := txn.Commit(); err != nil { + writeJSONError(w, "commit swap: "+err.Error(), http.StatusInternalServerError) return } @@ -1292,13 +1332,24 @@ func (s *AdminServer) handleResolveDuplicate(w http.ResponseWriter, r *http.Requ // field and filename change. m["id"] = body.NewID - newContent, err := writeMapAsYAML(newRel, m) + newContent, err := marshalMapYAML(m) if err != nil { failJSON(w, "write failed: "+err.Error()) return } - if err := os.Remove(absPath); err != nil { - failJSON(w, "remove old failed: "+err.Error()) + // Move + rewrite in one journaled transaction so a failure cannot + // leave the same ID in two directories. + txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + if err := txn.Move(absPath, newRel); err != nil { + failJSON(w, "plan rename failed: "+err.Error()) + return + } + if err := txn.Replace(newRel, newContent, 0644); err != nil { + failJSON(w, "plan write failed: "+err.Error()) + return + } + if err := txn.Commit(); err != nil { + failJSON(w, "rename failed: "+err.Error()) return } s.world.RebuildRoomIndex(s.dataDir) diff --git a/internal/admin/room_txn_atomic_test.go b/internal/admin/room_txn_atomic_test.go new file mode 100644 index 0000000..5f1e5bf --- /dev/null +++ b/internal/admin/room_txn_atomic_test.go @@ -0,0 +1,115 @@ +package admin + +// Regression: multi-file room/entity edits must be journaled transactions so +// a failure cannot half-apply them. These tests make the transaction journal +// uncreatable (a regular file sits at the journal path), which fails Commit +// before any operation is applied, and assert that no partial mutation +// survived. + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "thehouseoficarus/internal/world" +) + +// blockJournal makes the transaction journal path unusable by placing a +// regular file where persistence.Transaction.Commit wants a directory. +func blockJournal(t *testing.T, dataDir string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dataDir, ".journal"), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestRoomMoveAtomicWhenJournalUnavailable(t *testing.T) { + s := newTestAdminServer(t, map[int]string{1: "id: 1\nname: Origin\n"}) + if err := os.Mkdir(filepath.Join(s.dataDir, "rooms", "sub"), 0o755); err != nil { + t.Fatal(err) + } + blockJournal(t, s.dataDir) + + rec := httptest.NewRecorder() + s.handleRoomMove(rec, httptest.NewRequest(http.MethodPost, "/api/rooms/move", + strings.NewReader(`{"id":1,"dir":"sub"}`))) + if !strings.Contains(rec.Body.String(), "error") { + t.Fatalf("move should fail with an unusable journal; got %s", rec.Body.String()) + } + if _, err := os.Stat(filepath.Join(s.dataDir, "rooms", "1.yaml")); err != nil { + t.Fatalf("source room disappeared after failed move: %v", err) + } + if _, err := os.Stat(filepath.Join(s.dataDir, "rooms", "sub", "1.yaml")); !os.IsNotExist(err) { + t.Fatalf("destination created after failed move (stat err=%v)", err) + } +} + +func TestRoomUnlinkAtomicWhenJournalUnavailable(t *testing.T) { + s := newTestAdminServer(t, map[int]string{ + 1: "id: 1\nname: A\nexits:\n north:\n room: 2\n", + 2: "id: 2\nname: B\nexits:\n south:\n room: 1\n", + }) + blockJournal(t, s.dataDir) + + rec := httptest.NewRecorder() + s.handleRoomLink(rec, httptest.NewRequest(http.MethodDelete, "/api/rooms/link", + strings.NewReader(`{"from":1,"to":2}`))) + if !strings.Contains(rec.Body.String(), "error") { + t.Fatalf("unlink should fail with an unusable journal; got %s", rec.Body.String()) + } + if got := roomExitTarget(s, 1, world.North); got != 2 { + t.Fatalf("unlink half-applied: room 1 north now points at %d", got) + } + if got := roomExitTarget(s, 2, world.South); got != 1 { + t.Fatalf("unlink half-applied: room 2 south now points at %d", got) + } + if info := s.undoStack.Info(); info.StackSize != 0 { + t.Fatalf("failed unlink pushed an undo entry: %+v", info) + } +} + +func TestEntityMoveAtomicWhenJournalUnavailable(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "items", "a"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "items", "a", "one.yaml"), []byte("name: One\n"), 0o644); err != nil { + t.Fatal(err) + } + blockJournal(t, dir) + + if _, _, _, err := moveEntityFile(dir, "items", "one", ""); err == nil { + t.Fatal("move succeeded despite an unusable journal") + } + if _, err := os.Stat(filepath.Join(dir, "items", "a", "one.yaml")); err != nil { + t.Fatalf("source entity disappeared after failed move: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, "items", "one.yaml")); !os.IsNotExist(err) { + t.Fatalf("destination created after failed move (stat err=%v)", err) + } +} + +func TestDeleteEntityDirAtomicWhenJournalUnavailable(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "items", "pack"), 0o755); err != nil { + t.Fatal(err) + } + for _, name := range []string{"a.yaml", "b.yaml"} { + if err := os.WriteFile(filepath.Join(dir, "items", "pack", name), []byte("name: X\n"), 0o644); err != nil { + t.Fatal(err) + } + } + blockJournal(t, dir) + + if _, err := deleteEntityDir(dir, "items", "pack"); err == nil { + t.Fatal("deleteEntityDir succeeded despite an unusable journal") + } + for _, name := range []string{"a.yaml", "b.yaml"} { + if _, err := os.Stat(filepath.Join(dir, "items", "pack", name)); err != nil { + t.Fatalf("file %s lost after failed dir move: %v", name, err) + } + } +} diff --git a/internal/admin/yaml_util.go b/internal/admin/yaml_util.go index 44712a3..8cd3252 100644 --- a/internal/admin/yaml_util.go +++ b/internal/admin/yaml_util.go @@ -194,11 +194,14 @@ func moveEntityFile(dataDir, subdir, id, targetDir string) (string, string, []by if _, err := os.Stat(newPath); err == nil { return "", "", nil, fmt.Errorf("entity already exists in target directory") } - if err := os.WriteFile(newPath, data, 0644); err != nil { - return "", "", nil, fmt.Errorf("write: %w", err) + // Move in one journaled transaction: a failed write or source removal + // must not leave the entity in two directories. + txn := persistence.NewTransaction(filepath.Join(dataDir, ".journal")) + if err := txn.Move(oldPath, newPath); err != nil { + return "", "", nil, fmt.Errorf("move: %w", err) } - if err := os.Remove(oldPath); err != nil { - return "", "", nil, fmt.Errorf("remove old: %w", err) + if err := txn.Commit(); err != nil { + return "", "", nil, fmt.Errorf("move: %w", err) } return oldPath, newPath, data, nil } @@ -253,11 +256,14 @@ func renameEntityFile(dataDir, subdir, oldID, newID string) (string, string, []b if err != nil { return "", "", nil, fmt.Errorf("read: %w", err) } - if err := os.WriteFile(newPath, data, 0644); err != nil { - return "", "", nil, fmt.Errorf("write: %w", err) + // Rename in one journaled transaction: a failed write or source removal + // must not leave the entity in two directories. + txn := persistence.NewTransaction(filepath.Join(dataDir, ".journal")) + if err := txn.Move(oldPath, newPath); err != nil { + return "", "", nil, fmt.Errorf("rename: %w", err) } - if err := os.Remove(oldPath); err != nil { - return "", "", nil, fmt.Errorf("remove old: %w", err) + if err := txn.Commit(); err != nil { + return "", "", nil, fmt.Errorf("rename: %w", err) } return oldPath, newPath, data, nil } @@ -308,6 +314,7 @@ func deleteEntityDir(dataDir, subdir, dir string) ([]ChangeDesc, error) { } var changes []ChangeDesc + txn := persistence.NewTransaction(filepath.Join(dataDir, ".journal")) for _, e := range entries { if e.IsDir() || filepath.Ext(e.Name()) != ".yaml" { continue @@ -318,11 +325,8 @@ func deleteEntityDir(dataDir, subdir, dir string) ([]ChangeDesc, error) { if err != nil { return nil, fmt.Errorf("read %s: %w", e.Name(), err) } - if err := os.WriteFile(newPath, data, 0644); err != nil { - return nil, fmt.Errorf("write %s: %w", e.Name(), err) - } - if err := os.Remove(oldPath); err != nil { - return nil, fmt.Errorf("remove %s: %w", e.Name(), err) + if err := txn.Move(oldPath, newPath); err != nil { + return nil, fmt.Errorf("plan move %s: %w", e.Name(), err) } changes = append(changes, ChangeDesc{ Description: fmt.Sprintf("move %s %s to root", subdir, e.Name()), @@ -333,6 +337,12 @@ func deleteEntityDir(dataDir, subdir, dir string) ([]ChangeDesc, error) { }) } + // Move every file in one journaled transaction so a failure cannot leave + // the directory half-emptied (some files moved, some not). + if err := txn.Commit(); err != nil { + return nil, fmt.Errorf("move files: %w", err) + } + if err := os.Remove(srcDir); err != nil { return nil, fmt.Errorf("remove directory: %w", err) } diff --git a/internal/game/cmd_queued.go b/internal/game/cmd_queued.go index 7ac05a6..5f43ad1 100644 --- a/internal/game/cmd_queued.go +++ b/internal/game/cmd_queued.go @@ -2,7 +2,6 @@ package game import ( "fmt" - "sort" "thehouseoficarus/internal/net" ) @@ -51,15 +50,9 @@ func (g *Game) doQueued(sess *net.Session) { sess.WriteLine(" All queued actions take effect on the next game tick (600ms).") sess.WriteLine(" Free actions stack. Only the most recent active action survives.") - names, _ := g.queue.ActivePositions(p.Name) - sort.Strings(names) - pos := 0 - for i, name := range names { - if name == p.Name { - pos = i + 1 - break - } - } + // ActivePositions already returns the names sorted and the caller's + // 1-based position, so there is nothing to recompute here. + names, pos := g.queue.ActivePositions(p.Name) if pos > 0 && len(names) > 1 { sess.WriteLine(fmt.Sprintf(" Your active action will resolve as queue position #%d of %d.", pos, len(names))) } -- cgit v1.2.3