From a4c2d227bc12c06ce85abe59d324cc757f98439a Mon Sep 17 00:00:00 2001 From: MichaelFisher1997 Date: Thu, 2 Apr 2026 22:38:23 +0100 Subject: [PATCH 1/8] feat: implement SaveManager with background save thread and auto-save (#380) - Add level_data.zig for world metadata (seed, generator, timestamps) - Add save_manager.zig with background save thread, region file cache, dirty chunk tracking, and auto-save interval - Add modified flag to Chunk, set on setBlock for dirty tracking - Integrate SaveManager into World (enableSaveManager, saveAllModifiedChunks) - Integrate into WorldStreamer (load from save before worldgen, save on unload, periodic auto-save) - Wire up SaveManager in GameSession via ZIGCRAFT_SAVE_DIR env var - Register new test modules in tests.zig - All 492 tests passing --- src/game/session.zig | 10 + src/tests.zig | 2 + src/world/chunk.zig | 4 + src/world/persistence/level_data.zig | 137 ++++++++ src/world/persistence/save_manager.zig | 454 +++++++++++++++++++++++++ src/world/world.zig | 57 ++++ src/world/world_streamer.zig | 48 ++- 7 files changed, 708 insertions(+), 4 deletions(-) create mode 100644 src/world/persistence/level_data.zig create mode 100644 src/world/persistence/save_manager.zig diff --git a/src/game/session.zig b/src/game/session.zig index b4398a10..0d94748e 100644 --- a/src/game/session.zig +++ b/src/game/session.zig @@ -183,6 +183,16 @@ pub const GameSession = struct { .creative_mode = true, }; + const save_env = std.posix.getenv("ZIGCRAFT_SAVE_DIR"); + if (save_env) |save_path| { + world.enableSaveManager(save_path, "world") catch |err| { + log.log.warn("Failed to initialize save manager: {}", .{err}); + }; + if (world.save_manager) |sm| { + world.streamer.setSaveManager(sm); + } + } + // Force map update initially session.map_controller.map_needs_update = true; diff --git a/src/tests.zig b/src/tests.zig index 83ce9743..43e7ff37 100644 --- a/src/tests.zig +++ b/src/tests.zig @@ -91,6 +91,8 @@ test { _ = @import("game/session_tests.zig"); _ = @import("world/persistence/region_file.zig"); _ = @import("world/persistence/chunk_serializer.zig"); + _ = @import("world/persistence/level_data.zig"); + _ = @import("world/persistence/save_manager.zig"); _ = @import("world/meshing/quadric_simplifier.zig"); } diff --git a/src/world/chunk.zig b/src/world/chunk.zig index f8e7cbb1..d7a995e0 100644 --- a/src/world/chunk.zig +++ b/src/world/chunk.zig @@ -136,6 +136,9 @@ pub const Chunk = struct { /// Has this chunk been generated? generated: bool = false, + /// Has this chunk been modified since last save? + modified: bool = false, + /// Number of active jobs referencing this chunk (prevents unloading) pin_count: std.atomic.Value(u32), @@ -169,6 +172,7 @@ pub const Chunk = struct { pub fn setBlock(self: *Chunk, x: u32, y: u32, z: u32, block: BlockType) void { self.blocks[getIndex(x, y, z)] = block; self.dirty = true; + self.modified = true; } /// Get block with bounds checking (returns air if out of bounds) diff --git a/src/world/persistence/level_data.zig b/src/world/persistence/level_data.zig new file mode 100644 index 00000000..9b25adfa --- /dev/null +++ b/src/world/persistence/level_data.zig @@ -0,0 +1,137 @@ +//! Level metadata for world saves. +//! +//! Manages the `level.dat` JSON file that stores world metadata such as +//! seed, generator type, timestamps, and spawn position. + +const std = @import("std"); +const Allocator = std.mem.Allocator; +const log = @import("../../engine/core/log.zig"); + +fn currentTimestampMs() i64 { + const inst = std.time.Instant.now() catch return 0; + return @as(i64, inst.timestamp.sec) * std.time.ms_per_s + @divTrunc(@as(i64, inst.timestamp.nsec), @as(i64, std.time.ns_per_ms)); +} + +pub const LevelData = struct { + seed: u64, + generator_name: []const u8, + created_timestamp: i64, + last_played_timestamp: i64, + spawn_x: i32, + spawn_z: i32, + + pub fn init(seed: u64, generator_name: []const u8) LevelData { + const now = currentTimestampMs(); + return .{ + .seed = seed, + .generator_name = generator_name, + .created_timestamp = now, + .last_played_timestamp = now, + .spawn_x = 8, + .spawn_z = 8, + }; + } + + pub fn deinit(self: *LevelData, allocator: Allocator) void { + if (self.generator_name.len > 0) { + allocator.free(self.generator_name); + } + } + + pub fn saveToFile(self: *const LevelData, allocator: Allocator, dir: std.fs.Dir) !void { + var aw: std.Io.Writer.Allocating = try .initCapacity(allocator, 256); + defer aw.deinit(); + + const writer = &aw.writer; + try writer.writeAll("{\n"); + try writer.print(" \"seed\": {},\n", .{self.seed}); + try writer.print(" \"generator_name\": \"{s}\",\n", .{self.generator_name}); + try writer.print(" \"created_timestamp\": {},\n", .{self.created_timestamp}); + try writer.print(" \"last_played_timestamp\": {},\n", .{self.last_played_timestamp}); + try writer.print(" \"spawn_x\": {},\n", .{self.spawn_x}); + try writer.print(" \"spawn_z\": {}\n", .{self.spawn_z}); + try writer.writeAll("}"); + + const file = try dir.createFile("level.dat", .{ .truncate = true }); + defer file.close(); + try file.writeAll(aw.written()); + } + + pub fn loadFromFile(allocator: Allocator, dir: std.fs.Dir) !LevelData { + const file = try dir.openFile("level.dat", .{}); + defer file.close(); + + const stat = try file.stat(); + if (stat.size > 4096) return error.LevelDataTooLarge; + + const contents = try allocator.alloc(u8, @intCast(stat.size)); + defer allocator.free(contents); + _ = try file.preadAll(contents, 0); + + var result = LevelData{ + .seed = 0, + .generator_name = "", + .created_timestamp = 0, + .last_played_timestamp = 0, + .spawn_x = 8, + .spawn_z = 8, + }; + + var lines = std.mem.splitSequence(u8, contents, "\n"); + while (lines.next()) |line| { + const trimmed = std.mem.trim(u8, line, " \t\r,"); + if (trimmed.len == 0 or trimmed[0] == '{' or trimmed[0] == '}') continue; + + if (std.mem.indexOf(u8, trimmed, ":")) |colon_idx| { + const key = std.mem.trim(u8, trimmed[0..colon_idx], " \""); + const val = std.mem.trim(u8, trimmed[colon_idx + 1 ..], " \""); + + if (std.mem.eql(u8, key, "seed")) { + result.seed = std.fmt.parseInt(u64, val, 10) catch 0; + } else if (std.mem.eql(u8, key, "generator_name")) { + result.generator_name = try allocator.dupe(u8, val); + } else if (std.mem.eql(u8, key, "created_timestamp")) { + result.created_timestamp = std.fmt.parseInt(i64, val, 10) catch 0; + } else if (std.mem.eql(u8, key, "last_played_timestamp")) { + result.last_played_timestamp = std.fmt.parseInt(i64, val, 10) catch 0; + } else if (std.mem.eql(u8, key, "spawn_x")) { + result.spawn_x = std.fmt.parseInt(i32, val, 10) catch 8; + } else if (std.mem.eql(u8, key, "spawn_z")) { + result.spawn_z = std.fmt.parseInt(i32, val, 10) catch 8; + } + } + } + + return result; + } + + pub fn touchLastPlayed(self: *LevelData) void { + self.last_played_timestamp = currentTimestampMs(); + } +}; + +const testing = std.testing; + +test "LevelData save and load round-trip" { + var tmp_dir = testing.tmpDir(.{}); + defer tmp_dir.cleanup(); + + const original = LevelData.init(12345, "overworld"); + try original.saveToFile(testing.allocator, tmp_dir.dir); + + var loaded = try LevelData.loadFromFile(testing.allocator, tmp_dir.dir); + defer loaded.deinit(testing.allocator); + + try testing.expectEqual(@as(u64, 12345), loaded.seed); + try testing.expectEqualStrings("overworld", loaded.generator_name); + try testing.expectEqual(@as(i32, 8), loaded.spawn_x); + try testing.expectEqual(@as(i32, 8), loaded.spawn_z); +} + +test "LevelData touchLastPlayed updates timestamp" { + var data = LevelData.init(99999, "flat"); + const old_ts = data.last_played_timestamp; + std.posix.nanosleep(0, 1_000_000); + data.touchLastPlayed(); + try testing.expect(data.last_played_timestamp >= old_ts); +} diff --git a/src/world/persistence/save_manager.zig b/src/world/persistence/save_manager.zig new file mode 100644 index 00000000..0978c691 --- /dev/null +++ b/src/world/persistence/save_manager.zig @@ -0,0 +1,454 @@ +//! Save manager - background thread orchestrating chunk serialization and region file writes. +//! +//! Ties together the region file format (region_file.zig) and chunk serializer +//! (chunk_serializer.zig) into a working save/load system. Manages a background +//! save thread, dirty chunk tracking, and auto-save intervals. + +const std = @import("std"); +const Allocator = std.mem.Allocator; +const log = @import("../../engine/core/log.zig"); +const Chunk = @import("../chunk.zig").Chunk; +const ChunkKey = @import("../chunk_storage.zig").ChunkKey; +const RegionFile = @import("region_file.zig").RegionFile; +const chunk_serializer = @import("chunk_serializer.zig"); +const LevelData = @import("level_data.zig").LevelData; +const BlockType = @import("../block.zig").BlockType; +const BiomeId = @import("../worldgen/biome.zig").BiomeId; +const PackedLight = @import("../chunk.zig").PackedLight; +const CHUNK_VOLUME = @import("../chunk.zig").CHUNK_VOLUME; +const CHUNK_SIZE_X = @import("../chunk.zig").CHUNK_SIZE_X; +const CHUNK_SIZE_Z = @import("../chunk.zig").CHUNK_SIZE_Z; + +const SAVE_THREAD_INTERVAL_NS: u64 = 100 * std.time.ns_per_ms; +const AUTO_SAVE_INTERVAL_MS: i64 = 60_000; +const MAX_OPEN_REGIONS: usize = 16; + +fn currentTimestampMs() i64 { + const inst = std.time.Instant.now() catch return 0; + return @as(i64, @intCast(inst.timestamp.sec)) * std.time.ms_per_s + @divTrunc(@as(i64, @intCast(inst.timestamp.nsec)), @as(i64, std.time.ns_per_ms)); +} + +pub const SaveManagerError = error{ + SaveDirNotFound, + SaveThreadFailed, +}; + +pub const SaveQueueEntry = struct { + chunk_x: i32, + chunk_z: i32, + blocks: [CHUNK_VOLUME]BlockType, + light: [CHUNK_VOLUME]PackedLight, + biomes: [CHUNK_SIZE_X * CHUNK_SIZE_Z]BiomeId, + heightmap: [CHUNK_SIZE_X * CHUNK_SIZE_Z]i16, +}; + +const RegionCacheEntry = struct { + region_x: i32, + region_z: i32, + region: RegionFile, + last_used_ms: i64, +}; + +pub const SaveManager = struct { + allocator: Allocator, + save_dir: std.fs.Dir, + save_dir_path: []const u8, + world_name: []const u8, + + queue_mutex: std.Thread.Mutex, + queue: std.ArrayListUnmanaged(SaveQueueEntry), + running: std.atomic.Value(bool), + pending_saves: std.atomic.Value(usize), + + thread: std.Thread, + + region_cache_mutex: std.Thread.Mutex, + region_cache: std.ArrayListUnmanaged(RegionCacheEntry), + + level_data: LevelData, + last_auto_save_ms: i64, + + pub fn init(allocator: Allocator, save_dir_path: []const u8, world_name: []const u8, seed: u64, generator_name: []const u8) !*SaveManager { + var dir = try std.fs.cwd().makeOpenPath(save_dir_path, .{}); + errdefer dir.close(); + + const sm = try allocator.create(SaveManager); + errdefer allocator.destroy(sm); + + const name_copy = try allocator.dupe(u8, world_name); + errdefer allocator.free(name_copy); + + const path_copy = try allocator.dupe(u8, save_dir_path); + errdefer allocator.free(path_copy); + + sm.* = .{ + .allocator = allocator, + .save_dir = dir, + .save_dir_path = path_copy, + .world_name = name_copy, + .queue_mutex = .{}, + .queue = .empty, + .running = std.atomic.Value(bool).init(true), + .pending_saves = std.atomic.Value(usize).init(0), + .thread = undefined, + .region_cache_mutex = .{}, + .region_cache = .empty, + .level_data = LevelData.init(seed, generator_name), + .last_auto_save_ms = currentTimestampMs(), + }; + + const generator_copy = try allocator.dupe(u8, generator_name); + sm.level_data.generator_name = generator_copy; + + try sm.level_data.saveToFile(allocator, sm.save_dir); + + try dir.makePath("regions"); + + sm.thread = try std.Thread.spawn(.{}, saveThreadFn, .{sm}); + + log.log.info("SaveManager initialized for world '{s}' at '{s}'", .{ world_name, save_dir_path }); + return sm; + } + + pub fn deinit(self: *SaveManager) void { + self.flush(); + + self.running.store(false, .release); + self.thread.join(); + + self.flushRegionCache(); + + self.level_data.touchLastPlayed(); + self.level_data.saveToFile(self.allocator, self.save_dir) catch |err| { + log.log.err("Failed to save level.dat: {}", .{err}); + }; + + self.queue.deinit(self.allocator); + + self.save_dir.close(); + + self.level_data.deinit(self.allocator); + self.allocator.free(self.world_name); + self.allocator.free(self.save_dir_path); + self.allocator.destroy(self); + } + + pub fn enqueueSave(self: *SaveManager, chunk: *const Chunk) void { + self.queue_mutex.lock(); + defer self.queue_mutex.unlock(); + + for (self.queue.items) |*entry| { + if (entry.chunk_x == chunk.chunk_x and entry.chunk_z == chunk.chunk_z) { + entry.blocks = chunk.blocks; + entry.light = chunk.light; + entry.biomes = chunk.biomes; + entry.heightmap = chunk.heightmap; + return; + } + } + + self.queue.append(self.allocator, .{ + .chunk_x = chunk.chunk_x, + .chunk_z = chunk.chunk_z, + .blocks = chunk.blocks, + .light = chunk.light, + .biomes = chunk.biomes, + .heightmap = chunk.heightmap, + }) catch |err| { + log.log.err("Failed to enqueue chunk ({}, {}) for save: {}", .{ chunk.chunk_x, chunk.chunk_z, err }); + }; + } + + pub fn loadChunk(self: *SaveManager, cx: i32, cz: i32, out_chunk: *Chunk) bool { + const rx: i32 = @divFloor(cx, 32); + const rz: i32 = @divFloor(cz, 32); + + self.region_cache_mutex.lock(); + defer self.region_cache_mutex.unlock(); + + var region = self.getOrOpenRegion(rx, rz) catch |err| { + log.log.debug("No saved chunk at ({}, {}): region error: {}", .{ cx, cz, err }); + return false; + }; + + const local_x: u5 = @intCast(@mod(cx, 32)); + const local_z: u5 = @intCast(@mod(cz, 32)); + + if (!region.hasChunk(local_x, local_z)) return false; + + const data = region.readChunk(local_x, local_z, self.allocator) catch |err| { + log.log.err("Failed to read chunk ({}, {}) from region: {}", .{ cx, cz, err }); + return false; + }; + defer self.allocator.free(data); + + chunk_serializer.deserializeChunk(data, out_chunk) catch |err| { + log.log.err("Failed to deserialize chunk ({}, {}): {}", .{ cx, cz, err }); + return false; + }; + + out_chunk.chunk_x = cx; + out_chunk.chunk_z = cz; + out_chunk.generated = true; + + log.log.debug("Loaded chunk ({}, {}) from save", .{ cx, cz }); + return true; + } + + pub fn shouldAutoSave(self: *const SaveManager) bool { + const now = currentTimestampMs(); + return (now - self.last_auto_save_ms) >= AUTO_SAVE_INTERVAL_MS; + } + + pub fn markAutoSaved(self: *SaveManager) void { + self.last_auto_save_ms = currentTimestampMs(); + } + + pub fn flush(self: *SaveManager) void { + var spins: u32 = 0; + while (spins < 2000) : (spins += 1) { + self.queue_mutex.lock(); + const count = self.queue.items.len; + self.queue_mutex.unlock(); + const saving = self.pending_saves.load(.acquire); + if (count == 0 and saving == 0) break; + std.posix.nanosleep(0, 10 * std.time.ns_per_ms); + } + } + + fn saveThreadFn(self: *SaveManager) void { + log.log.debug("Save thread started", .{}); + + while (self.running.load(.acquire)) { + std.posix.nanosleep(0, SAVE_THREAD_INTERVAL_NS); + + self.processSaveQueue() catch |err| { + log.log.err("Save thread error: {}", .{err}); + }; + } + + self.processSaveQueue() catch |err| { + log.log.err("Save thread final flush error: {}", .{err}); + }; + + log.log.debug("Save thread exiting", .{}); + } + + fn processSaveQueue(self: *SaveManager) !void { + var batch: [64]SaveQueueEntry = undefined; + + self.queue_mutex.lock(); + const count = @min(self.queue.items.len, batch.len); + if (count == 0) { + self.queue_mutex.unlock(); + return; + } + log.log.debug("Save thread processing {} chunks", .{count}); + @memcpy(batch[0..count], self.queue.items[0..count]); + var remaining = std.ArrayListUnmanaged(SaveQueueEntry).empty; + defer remaining.deinit(self.allocator); + if (self.queue.items.len > count) { + try remaining.appendSlice(self.allocator, self.queue.items[count..]); + } + self.queue.deinit(self.allocator); + self.queue = remaining; + self.pending_saves.store(count, .release); + self.queue_mutex.unlock(); + + for (batch[0..count]) |entry| { + self.saveOneChunk(&entry) catch |err| { + log.log.err("Failed to save chunk ({}, {}): {}", .{ entry.chunk_x, entry.chunk_z, err }); + }; + } + self.pending_saves.store(0, .release); + } + + fn saveOneChunk(self: *SaveManager, entry: *const SaveQueueEntry) !void { + var chunk = Chunk.init(entry.chunk_x, entry.chunk_z); + chunk.blocks = entry.blocks; + chunk.light = entry.light; + chunk.biomes = entry.biomes; + chunk.heightmap = entry.heightmap; + chunk.generated = true; + + const serialized = chunk_serializer.serializeChunk(&chunk, self.allocator) catch |err| { + log.log.err("Failed to serialize chunk ({}, {}): {}", .{ entry.chunk_x, entry.chunk_z, err }); + return err; + }; + defer self.allocator.free(serialized); + + const rx: i32 = @divFloor(entry.chunk_x, 32); + const rz: i32 = @divFloor(entry.chunk_z, 32); + + self.region_cache_mutex.lock(); + defer self.region_cache_mutex.unlock(); + + var region = try self.getOrOpenRegion(rx, rz); + + const local_x: u5 = @intCast(@mod(entry.chunk_x, 32)); + const local_z: u5 = @intCast(@mod(entry.chunk_z, 32)); + + region.writeChunk(local_x, local_z, serialized) catch |err| { + log.log.err("Failed to write chunk ({}, {}) to region ({}, {}): {}", .{ entry.chunk_x, entry.chunk_z, rx, rz, err }); + return err; + }; + + log.log.debug("Saved chunk ({}, {}) to region ({}, {})", .{ entry.chunk_x, entry.chunk_z, rx, rz }); + } + + fn getOrOpenRegion(self: *SaveManager, rx: i32, rz: i32) !*RegionFile { + const now_ms = currentTimestampMs(); + + for (self.region_cache.items) |*entry| { + if (entry.region_x == rx and entry.region_z == rz) { + entry.last_used_ms = now_ms; + return &entry.region; + } + } + + if (self.region_cache.items.len >= MAX_OPEN_REGIONS) { + self.evictOldestRegion(); + } + + var rel_buf: [512]u8 = undefined; + const region_filename = std.fmt.bufPrint(&rel_buf, "regions/r.{}.{}.mca", .{ rx, rz }) catch unreachable; + + const region = blk: { + var abs_buf_open: [std.fs.max_path_bytes]u8 = undefined; + if (self.save_dir.realpath(region_filename, &abs_buf_open)) |abs_path| { + break :blk try RegionFile.open(self.allocator, abs_path); + } else |_| { + var abs_buf_create: [std.fs.max_path_bytes]u8 = undefined; + const file = try self.save_dir.createFile(region_filename, .{ .read = true, .truncate = true }); + file.close(); + break :blk try RegionFile.create(self.allocator, try self.save_dir.realpath(region_filename, &abs_buf_create)); + } + }; + + try self.region_cache.append(self.allocator, .{ + .region_x = rx, + .region_z = rz, + .region = region, + .last_used_ms = now_ms, + }); + + return &self.region_cache.items[self.region_cache.items.len - 1].region; + } + + fn evictOldestRegion(self: *SaveManager) void { + if (self.region_cache.items.len == 0) return; + + var oldest_idx: usize = 0; + var oldest_ts: i64 = self.region_cache.items[0].last_used_ms; + for (self.region_cache.items[1..], 1..) |entry, i| { + if (entry.last_used_ms < oldest_ts) { + oldest_ts = entry.last_used_ms; + oldest_idx = i; + } + } + + self.region_cache.items[oldest_idx].region.close(); + _ = self.region_cache.orderedRemove(oldest_idx); + } + + fn flushRegionCache(self: *SaveManager) void { + for (self.region_cache.items) |*entry| { + entry.region.close(); + } + self.region_cache.deinit(self.allocator); + } +}; + +const testing = std.testing; + +test "SaveManager init creates save directory and level.dat" { + var tmp_dir = testing.tmpDir(.{}); + defer tmp_dir.cleanup(); + + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const base_path = try tmp_dir.dir.realpath(".", &path_buf); + + var save_path_buf: [std.fs.max_path_bytes]u8 = undefined; + const save_path = try std.fmt.bufPrint(&save_path_buf, "{s}/test_world", .{base_path}); + + var sm = try SaveManager.init(testing.allocator, save_path, "test_world", 42, "overworld"); + defer sm.deinit(); + + const file = tmp_dir.dir.openFile("test_world/level.dat", .{}) catch { + try testing.expect(false); + return; + }; + file.close(); +} + +test "SaveManager enqueue and flush processes chunks" { + var tmp_dir = testing.tmpDir(.{}); + defer tmp_dir.cleanup(); + + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const base_path = try tmp_dir.dir.realpath(".", &path_buf); + + var save_path_buf: [std.fs.max_path_bytes]u8 = undefined; + const save_path = try std.fmt.bufPrint(&save_path_buf, "{s}/test_flush", .{base_path}); + + var sm = try SaveManager.init(testing.allocator, save_path, "test_flush", 99, "flat"); + defer sm.deinit(); + + var chunk = Chunk.init(5, -3); + chunk.setBlock(8, 64, 8, .stone); + chunk.setBiome(0, 0, .forest); + chunk.generated = true; + + sm.enqueueSave(&chunk); + sm.flush(); + + var loaded = Chunk.init(5, -3); + try testing.expect(sm.loadChunk(5, -3, &loaded)); + try testing.expectEqual(BlockType.stone, loaded.getBlock(8, 64, 8)); + try testing.expectEqual(BiomeId.forest, loaded.getBiome(0, 0)); +} + +test "SaveManager loadChunk returns false for non-existent chunk" { + var tmp_dir = testing.tmpDir(.{}); + defer tmp_dir.cleanup(); + + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const base_path = try tmp_dir.dir.realpath(".", &path_buf); + + var save_path_buf: [std.fs.max_path_bytes]u8 = undefined; + const save_path = try std.fmt.bufPrint(&save_path_buf, "{s}/test_load_miss", .{base_path}); + + var sm = try SaveManager.init(testing.allocator, save_path, "test_load_miss", 0, "overworld"); + defer sm.deinit(); + + var chunk = Chunk.init(100, 200); + try testing.expect(!sm.loadChunk(100, 200, &chunk)); +} + +test "SaveManager duplicate enqueue overwrites previous" { + var tmp_dir = testing.tmpDir(.{}); + defer tmp_dir.cleanup(); + + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const base_path = try tmp_dir.dir.realpath(".", &path_buf); + + var save_path_buf: [std.fs.max_path_bytes]u8 = undefined; + const save_path = try std.fmt.bufPrint(&save_path_buf, "{s}/test_dup", .{base_path}); + + var sm = try SaveManager.init(testing.allocator, save_path, "test_dup", 0, "flat"); + defer sm.deinit(); + + var chunk1 = Chunk.init(0, 0); + chunk1.setBlock(5, 5, 5, .dirt); + + var chunk2 = Chunk.init(0, 0); + chunk2.setBlock(5, 5, 5, .gold_ore); + + sm.enqueueSave(&chunk1); + sm.enqueueSave(&chunk2); + sm.flush(); + + var loaded = Chunk.init(0, 0); + try testing.expect(sm.loadChunk(0, 0, &loaded)); + try testing.expectEqual(BlockType.gold_ore, loaded.getBlock(5, 5, 5)); +} diff --git a/src/world/world.zig b/src/world/world.zig index cfa843b3..720ef9d7 100644 --- a/src/world/world.zig +++ b/src/world/world.zig @@ -39,6 +39,7 @@ const log = @import("../engine/core/log.zig"); const LODConfig = @import("lod_chunk.zig").LODConfig; const ILODConfig = @import("lod_chunk.zig").ILODConfig; const CHUNK_UNLOAD_BUFFER = @import("chunk.zig").CHUNK_UNLOAD_BUFFER; +const SaveManager = @import("persistence/save_manager.zig").SaveManager; /// Buffer distance beyond render_distance for chunk unloading. /// Prevents thrashing when player moves near chunk boundaries. @@ -119,6 +120,9 @@ pub const World = struct { lod: ?*WorldLOD, lod_enabled: bool, // Runtime toggle for LOD rendering + // Save system (Issue #380) + save_manager: ?*SaveManager, + pub fn init(allocator: std.mem.Allocator, render_distance: i32, seed: u64, rhi: RHI, atlas: *const TextureAtlas) !*World { return initGen(0, allocator, render_distance, seed, rhi, atlas); } @@ -154,6 +158,7 @@ pub const World = struct { .safe_render_distance = safe_render_distance, .lod = null, .lod_enabled = false, + .save_manager = null, }; log.log.info("World.initGen: initializing WorldRenderer", .{}); @@ -184,6 +189,12 @@ pub const World = struct { pub fn deinit(self: *World) void { self.rhi.waitIdle(); + + if (self.save_manager) |sm| { + self.saveAllModifiedChunks(); + sm.deinit(); + } + self.streamer.deinit(); // Storage must be deinitialized before renderer because it uses the renderer's vertex_allocator @@ -219,6 +230,52 @@ pub const World = struct { } } + pub fn enableSaveManager(self: *World, save_dir_path: []const u8, world_name: []const u8) !void { + const seed = self.generator.getSeed(); + const gen_name = self.generator.info.name; + self.save_manager = try SaveManager.init(self.allocator, save_dir_path, world_name, seed, gen_name); + } + + pub fn saveAllModifiedChunks(self: *World) void { + const sm = self.save_manager orelse return; + + self.storage.chunks_mutex.lockShared(); + var iter = self.storage.iteratorUnsafe(); + while (iter.next()) |entry| { + const chunk = &entry.value_ptr.*.chunk; + if (chunk.modified and chunk.generated) { + sm.enqueueSave(chunk); + chunk.modified = false; + } + } + self.storage.chunks_mutex.unlockShared(); + + sm.flush(); + } + + pub fn checkAutoSave(self: *World) void { + const sm = self.save_manager orelse return; + if (!sm.shouldAutoSave()) return; + + self.storage.chunks_mutex.lockShared(); + var iter = self.storage.iteratorUnsafe(); + while (iter.next()) |entry| { + const chunk = &entry.value_ptr.*.chunk; + if (chunk.modified and chunk.generated) { + sm.enqueueSave(chunk); + chunk.modified = false; + } + } + self.storage.chunks_mutex.unlockShared(); + + sm.markAutoSaved(); + } + + pub fn loadChunkFromSave(self: *World, cx: i32, cz: i32, out_chunk: *Chunk) bool { + const sm = self.save_manager orelse return false; + return sm.loadChunk(cx, cz, out_chunk); + } + /// Set render distance and trigger chunk loading/unloading update pub fn setRenderDistance(self: *World, distance: i32) void { const target = if (self.safe_mode) @min(distance, self.safe_render_distance) else distance; diff --git a/src/world/world_streamer.zig b/src/world/world_streamer.zig index 936af335..603a4493 100644 --- a/src/world/world_streamer.zig +++ b/src/world/world_streamer.zig @@ -61,6 +61,7 @@ const GlobalVertexAllocator = @import("chunk_allocator.zig").GlobalVertexAllocat const LODManager = @import("lod_manager.zig").LODManager; const TextureAtlas = @import("../engine/graphics/texture_atlas.zig").TextureAtlas; const log = @import("../engine/core/log.zig"); +const SaveManager = @import("persistence/save_manager.zig").SaveManager; /// Buffer distance beyond render_distance for chunk unloading. /// Prevents thrashing when player moves near chunk boundaries. @@ -146,6 +147,7 @@ pub const WorldStreamer = struct { max_uploads_per_frame: usize, paused: bool = false, + save_manager: ?*SaveManager = null, const GEN_WORKERS = 4; const MESH_WORKERS = 3; @@ -235,12 +237,17 @@ pub const WorldStreamer = struct { self.lod_manager = lod_manager; } + pub fn setSaveManager(self: *WorldStreamer, sm: ?*SaveManager) void { + self.save_manager = sm; + } + pub fn updateFrame(self: *WorldStreamer, player_pos: Vec3, dt: f32) !void { if (self.paused) return; try self.updateStreaming(player_pos, dt); self.processUploads(); try self.processUnloads(player_pos); + self.checkAutoSave(); } fn updateStreaming(self: *WorldStreamer, player_pos: Vec3, dt: f32) !void { @@ -385,6 +392,14 @@ pub const WorldStreamer = struct { } for (to_remove.items) |key| { + if (self.save_manager) |sm| { + if (self.storage.chunks.get(key)) |data| { + if (data.chunk.modified and data.chunk.generated) { + sm.enqueueSave(&data.chunk); + data.chunk.modified = false; + } + } + } _ = self.storage.removeUnlocked(key.x, key.z, self.vertex_allocator); } self.storage.chunks_mutex.unlock(); @@ -418,10 +433,17 @@ pub const WorldStreamer = struct { defer chunk_data.chunk.unpin(); if (chunk_data.chunk.state == .generating and chunk_data.chunk.job_token == job.data.chunk.job_token) { - self.generator.generate(&chunk_data.chunk, &self.gen_queue.abort_worker); - if (self.gen_queue.abort_worker) { - chunk_data.chunk.state = .missing; - return; + const loaded_from_save = blk: { + const sm = self.save_manager orelse break :blk false; + break :blk sm.loadChunk(cx, cz, &chunk_data.chunk); + }; + + if (!loaded_from_save) { + self.generator.generate(&chunk_data.chunk, &self.gen_queue.abort_worker); + if (self.gen_queue.abort_worker) { + chunk_data.chunk.state = .missing; + return; + } } chunk_data.chunk.state = .generated; self.markNeighborsForRemesh(cx, cz); @@ -508,6 +530,24 @@ pub const WorldStreamer = struct { } } + fn checkAutoSave(self: *WorldStreamer) void { + const sm = self.save_manager orelse return; + if (!sm.shouldAutoSave()) return; + + self.storage.chunks_mutex.lockShared(); + var iter = self.storage.iteratorUnsafe(); + while (iter.next()) |entry| { + const chunk = &entry.value_ptr.*.chunk; + if (chunk.modified and chunk.generated) { + sm.enqueueSave(chunk); + chunk.modified = false; + } + } + self.storage.chunks_mutex.unlockShared(); + + sm.markAutoSaved(); + } + pub fn getStats(self: *WorldStreamer) struct { gen_queue: usize, mesh_queue: usize, upload_queue: usize } { self.gen_queue.mutex.lock(); const gen_count = self.gen_queue.jobs.count(); From 6633cbb1ee2cf809f121c8186ce869030dccccd3 Mon Sep 17 00:00:00 2001 From: MichaelFisher1997 Date: Fri, 3 Apr 2026 00:17:25 +0100 Subject: [PATCH 2/8] fix: address code review on SaveManager PR #411 - Fix race condition in enqueueSave by snapshotting data before mutex - Fix TOCTOU race in getOrOpenRegion using exclusive create flag - Replace loadChunk bool return with LoadResult enum (success/not_found/read_error/corrupt_data) - Defer modified flag clear until after flush to prevent silent data loss - Increase flush timeout from 20s to 120s - Set chunk state under mutex in processGenJob - Fix duplicate generator_name allocation leak in init - Replace hardcoded 512 buffer with std.fs.max_path_bytes --- src/world/persistence/save_manager.zig | 82 ++++++++++++++------------ src/world/world.zig | 30 ++++++++-- src/world/world_streamer.zig | 28 +++++++-- 3 files changed, 94 insertions(+), 46 deletions(-) diff --git a/src/world/persistence/save_manager.zig b/src/world/persistence/save_manager.zig index 0978c691..b061442f 100644 --- a/src/world/persistence/save_manager.zig +++ b/src/world/persistence/save_manager.zig @@ -28,9 +28,11 @@ fn currentTimestampMs() i64 { return @as(i64, @intCast(inst.timestamp.sec)) * std.time.ms_per_s + @divTrunc(@as(i64, @intCast(inst.timestamp.nsec)), @as(i64, std.time.ns_per_ms)); } -pub const SaveManagerError = error{ - SaveDirNotFound, - SaveThreadFailed, +pub const LoadResult = enum { + success, + not_found, + read_error, + corrupt_data, }; pub const SaveQueueEntry = struct { @@ -93,13 +95,15 @@ pub const SaveManager = struct { .thread = undefined, .region_cache_mutex = .{}, .region_cache = .empty, - .level_data = LevelData.init(seed, generator_name), + .level_data = blk: { + var ld = LevelData.init(seed, ""); + const generator_copy = try allocator.dupe(u8, generator_name); + ld.generator_name = generator_copy; + break :blk ld; + }, .last_auto_save_ms = currentTimestampMs(), }; - const generator_copy = try allocator.dupe(u8, generator_name); - sm.level_data.generator_name = generator_copy; - try sm.level_data.saveToFile(allocator, sm.save_dir); try dir.makePath("regions"); @@ -134,32 +138,31 @@ pub const SaveManager = struct { } pub fn enqueueSave(self: *SaveManager, chunk: *const Chunk) void { + const snapshot = SaveQueueEntry{ + .chunk_x = chunk.chunk_x, + .chunk_z = chunk.chunk_z, + .blocks = chunk.blocks, + .light = chunk.light, + .biomes = chunk.biomes, + .heightmap = chunk.heightmap, + }; + self.queue_mutex.lock(); defer self.queue_mutex.unlock(); for (self.queue.items) |*entry| { - if (entry.chunk_x == chunk.chunk_x and entry.chunk_z == chunk.chunk_z) { - entry.blocks = chunk.blocks; - entry.light = chunk.light; - entry.biomes = chunk.biomes; - entry.heightmap = chunk.heightmap; + if (entry.chunk_x == snapshot.chunk_x and entry.chunk_z == snapshot.chunk_z) { + entry.* = snapshot; return; } } - self.queue.append(self.allocator, .{ - .chunk_x = chunk.chunk_x, - .chunk_z = chunk.chunk_z, - .blocks = chunk.blocks, - .light = chunk.light, - .biomes = chunk.biomes, - .heightmap = chunk.heightmap, - }) catch |err| { - log.log.err("Failed to enqueue chunk ({}, {}) for save: {}", .{ chunk.chunk_x, chunk.chunk_z, err }); + self.queue.append(self.allocator, snapshot) catch |err| { + log.log.err("Failed to enqueue chunk ({}, {}) for save: {}", .{ snapshot.chunk_x, snapshot.chunk_z, err }); }; } - pub fn loadChunk(self: *SaveManager, cx: i32, cz: i32, out_chunk: *Chunk) bool { + pub fn loadChunk(self: *SaveManager, cx: i32, cz: i32, out_chunk: *Chunk) LoadResult { const rx: i32 = @divFloor(cx, 32); const rz: i32 = @divFloor(cz, 32); @@ -168,23 +171,23 @@ pub const SaveManager = struct { var region = self.getOrOpenRegion(rx, rz) catch |err| { log.log.debug("No saved chunk at ({}, {}): region error: {}", .{ cx, cz, err }); - return false; + return .not_found; }; const local_x: u5 = @intCast(@mod(cx, 32)); const local_z: u5 = @intCast(@mod(cz, 32)); - if (!region.hasChunk(local_x, local_z)) return false; + if (!region.hasChunk(local_x, local_z)) return .not_found; const data = region.readChunk(local_x, local_z, self.allocator) catch |err| { log.log.err("Failed to read chunk ({}, {}) from region: {}", .{ cx, cz, err }); - return false; + return .read_error; }; defer self.allocator.free(data); chunk_serializer.deserializeChunk(data, out_chunk) catch |err| { log.log.err("Failed to deserialize chunk ({}, {}): {}", .{ cx, cz, err }); - return false; + return .corrupt_data; }; out_chunk.chunk_x = cx; @@ -192,7 +195,7 @@ pub const SaveManager = struct { out_chunk.generated = true; log.log.debug("Loaded chunk ({}, {}) from save", .{ cx, cz }); - return true; + return .success; } pub fn shouldAutoSave(self: *const SaveManager) bool { @@ -206,7 +209,7 @@ pub const SaveManager = struct { pub fn flush(self: *SaveManager) void { var spins: u32 = 0; - while (spins < 2000) : (spins += 1) { + while (spins < 12000) : (spins += 1) { self.queue_mutex.lock(); const count = self.queue.items.len; self.queue_mutex.unlock(); @@ -310,18 +313,23 @@ pub const SaveManager = struct { self.evictOldestRegion(); } - var rel_buf: [512]u8 = undefined; + var rel_buf: [std.fs.max_path_bytes]u8 = undefined; const region_filename = std.fmt.bufPrint(&rel_buf, "regions/r.{}.{}.mca", .{ rx, rz }) catch unreachable; const region = blk: { - var abs_buf_open: [std.fs.max_path_bytes]u8 = undefined; - if (self.save_dir.realpath(region_filename, &abs_buf_open)) |abs_path| { + var abs_buf: [std.fs.max_path_bytes]u8 = undefined; + if (self.save_dir.realpath(region_filename, &abs_buf)) |abs_path| { break :blk try RegionFile.open(self.allocator, abs_path); } else |_| { - var abs_buf_create: [std.fs.max_path_bytes]u8 = undefined; - const file = try self.save_dir.createFile(region_filename, .{ .read = true, .truncate = true }); + const file = self.save_dir.createFile(region_filename, .{ .read = true, .exclusive = true }) catch |err| { + if (err == error.PathAlreadyExists) { + const abs_path = try self.save_dir.realpath(region_filename, &abs_buf); + break :blk try RegionFile.open(self.allocator, abs_path); + } + return err; + }; file.close(); - break :blk try RegionFile.create(self.allocator, try self.save_dir.realpath(region_filename, &abs_buf_create)); + break :blk try RegionFile.create(self.allocator, try self.save_dir.realpath(region_filename, &abs_buf)); } }; @@ -403,7 +411,7 @@ test "SaveManager enqueue and flush processes chunks" { sm.flush(); var loaded = Chunk.init(5, -3); - try testing.expect(sm.loadChunk(5, -3, &loaded)); + try testing.expect(sm.loadChunk(5, -3, &loaded) == .success); try testing.expectEqual(BlockType.stone, loaded.getBlock(8, 64, 8)); try testing.expectEqual(BiomeId.forest, loaded.getBiome(0, 0)); } @@ -422,7 +430,7 @@ test "SaveManager loadChunk returns false for non-existent chunk" { defer sm.deinit(); var chunk = Chunk.init(100, 200); - try testing.expect(!sm.loadChunk(100, 200, &chunk)); + try testing.expect(sm.loadChunk(100, 200, &chunk) == .not_found); } test "SaveManager duplicate enqueue overwrites previous" { @@ -449,6 +457,6 @@ test "SaveManager duplicate enqueue overwrites previous" { sm.flush(); var loaded = Chunk.init(0, 0); - try testing.expect(sm.loadChunk(0, 0, &loaded)); + try testing.expect(sm.loadChunk(0, 0, &loaded) == .success); try testing.expectEqual(BlockType.gold_ore, loaded.getBlock(5, 5, 5)); } diff --git a/src/world/world.zig b/src/world/world.zig index 720ef9d7..69c47bfe 100644 --- a/src/world/world.zig +++ b/src/world/world.zig @@ -7,6 +7,7 @@ const NeighborChunks = @import("chunk_mesh.zig").NeighborChunks; const BlockType = @import("block.zig").BlockType; const ChunkStorage = @import("chunk_storage.zig").ChunkStorage; const ChunkData = @import("chunk_storage.zig").ChunkData; +const ChunkKey = @import("chunk_storage.zig").ChunkKey; const worldToChunk = @import("chunk.zig").worldToChunk; const worldToLocal = @import("chunk.zig").worldToLocal; const CHUNK_SIZE_X = @import("chunk.zig").CHUNK_SIZE_X; @@ -40,6 +41,7 @@ const LODConfig = @import("lod_chunk.zig").LODConfig; const ILODConfig = @import("lod_chunk.zig").ILODConfig; const CHUNK_UNLOAD_BUFFER = @import("chunk.zig").CHUNK_UNLOAD_BUFFER; const SaveManager = @import("persistence/save_manager.zig").SaveManager; +const LoadResult = @import("persistence/save_manager.zig").LoadResult; /// Buffer distance beyond render_distance for chunk unloading. /// Prevents thrashing when player moves near chunk boundaries. @@ -239,40 +241,60 @@ pub const World = struct { pub fn saveAllModifiedChunks(self: *World) void { const sm = self.save_manager orelse return; + var dirty_keys = std.ArrayListUnmanaged(ChunkKey).empty; + defer dirty_keys.deinit(self.allocator); + self.storage.chunks_mutex.lockShared(); var iter = self.storage.iteratorUnsafe(); while (iter.next()) |entry| { const chunk = &entry.value_ptr.*.chunk; if (chunk.modified and chunk.generated) { sm.enqueueSave(chunk); - chunk.modified = false; + dirty_keys.append(self.allocator, entry.key_ptr.*) catch {}; } } self.storage.chunks_mutex.unlockShared(); sm.flush(); + + self.storage.chunks_mutex.lockShared(); + for (dirty_keys.items) |key| { + if (self.storage.chunks.get(key)) |data| { + data.chunk.modified = false; + } + } + self.storage.chunks_mutex.unlockShared(); } pub fn checkAutoSave(self: *World) void { const sm = self.save_manager orelse return; if (!sm.shouldAutoSave()) return; + var dirty_keys = std.ArrayListUnmanaged(ChunkKey).empty; + defer dirty_keys.deinit(self.allocator); + self.storage.chunks_mutex.lockShared(); var iter = self.storage.iteratorUnsafe(); while (iter.next()) |entry| { const chunk = &entry.value_ptr.*.chunk; if (chunk.modified and chunk.generated) { sm.enqueueSave(chunk); - chunk.modified = false; + dirty_keys.append(self.allocator, entry.key_ptr.*) catch {}; } } self.storage.chunks_mutex.unlockShared(); sm.markAutoSaved(); + + for (dirty_keys.items) |key| { + if (self.storage.chunks.get(key)) |data| { + data.chunk.modified = false; + } + } } - pub fn loadChunkFromSave(self: *World, cx: i32, cz: i32, out_chunk: *Chunk) bool { - const sm = self.save_manager orelse return false; + pub fn loadChunkFromSave(self: *World, cx: i32, cz: i32, out_chunk: *Chunk) LoadResult { + const sm = self.save_manager orelse return .not_found; return sm.loadChunk(cx, cz, out_chunk); } diff --git a/src/world/world_streamer.zig b/src/world/world_streamer.zig index 603a4493..18628aba 100644 --- a/src/world/world_streamer.zig +++ b/src/world/world_streamer.zig @@ -62,6 +62,7 @@ const LODManager = @import("lod_manager.zig").LODManager; const TextureAtlas = @import("../engine/graphics/texture_atlas.zig").TextureAtlas; const log = @import("../engine/core/log.zig"); const SaveManager = @import("persistence/save_manager.zig").SaveManager; +const LoadResult = @import("persistence/save_manager.zig").LoadResult; /// Buffer distance beyond render_distance for chunk unloading. /// Prevents thrashing when player moves near chunk boundaries. @@ -396,10 +397,12 @@ pub const WorldStreamer = struct { if (self.storage.chunks.get(key)) |data| { if (data.chunk.modified and data.chunk.generated) { sm.enqueueSave(&data.chunk); - data.chunk.modified = false; } } } + if (self.storage.chunks.get(key)) |data| { + data.chunk.modified = false; + } _ = self.storage.removeUnlocked(key.x, key.z, self.vertex_allocator); } self.storage.chunks_mutex.unlock(); @@ -433,19 +436,25 @@ pub const WorldStreamer = struct { defer chunk_data.chunk.unpin(); if (chunk_data.chunk.state == .generating and chunk_data.chunk.job_token == job.data.chunk.job_token) { - const loaded_from_save = blk: { - const sm = self.save_manager orelse break :blk false; + const load_result = blk: { + const sm = self.save_manager orelse break :blk LoadResult.not_found; break :blk sm.loadChunk(cx, cz, &chunk_data.chunk); }; - if (!loaded_from_save) { + if (load_result != .success) { + if (load_result == .read_error or load_result == .corrupt_data) { + log.log.warn("Save load failed for chunk ({}, {}): {}, regenerating", .{ cx, cz, load_result }); + } self.generator.generate(&chunk_data.chunk, &self.gen_queue.abort_worker); if (self.gen_queue.abort_worker) { chunk_data.chunk.state = .missing; return; } } + + self.storage.chunks_mutex.lockShared(); chunk_data.chunk.state = .generated; + self.storage.chunks_mutex.unlockShared(); self.markNeighborsForRemesh(cx, cz); } } @@ -534,18 +543,27 @@ pub const WorldStreamer = struct { const sm = self.save_manager orelse return; if (!sm.shouldAutoSave()) return; + var dirty_keys = std.ArrayListUnmanaged(ChunkKey).empty; + defer dirty_keys.deinit(self.allocator); + self.storage.chunks_mutex.lockShared(); var iter = self.storage.iteratorUnsafe(); while (iter.next()) |entry| { const chunk = &entry.value_ptr.*.chunk; if (chunk.modified and chunk.generated) { sm.enqueueSave(chunk); - chunk.modified = false; + dirty_keys.append(self.allocator, entry.key_ptr.*) catch {}; } } self.storage.chunks_mutex.unlockShared(); sm.markAutoSaved(); + + for (dirty_keys.items) |key| { + if (self.storage.chunks.get(key)) |data| { + data.chunk.modified = false; + } + } } pub fn getStats(self: *WorldStreamer) struct { gen_queue: usize, mesh_queue: usize, upload_queue: usize } { From 463cf7bf313510199d820d69d4de5d0e740b47b2 Mon Sep 17 00:00:00 2001 From: MichaelFisher1997 Date: Fri, 3 Apr 2026 00:33:03 +0100 Subject: [PATCH 3/8] fix: address second round code review on SaveManager PR #411 - Add pin assertion in enqueueSave to prevent race on chunk data snapshot - Track failed saves per-chunk and re-set modified flag on failure - Pin chunks around enqueueSave in all callers (world.zig, world_streamer.zig) - Fix timestamp overflow risk using std.math.mul and saturating arithmetic - Return failed chunk keys from flush() for caller error recovery --- src/world/persistence/level_data.zig | 6 ++++- src/world/persistence/save_manager.zig | 37 ++++++++++++++++++++++---- src/world/world.zig | 15 ++++++++--- src/world/world_streamer.zig | 8 +++--- 4 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/world/persistence/level_data.zig b/src/world/persistence/level_data.zig index 9b25adfa..9430509a 100644 --- a/src/world/persistence/level_data.zig +++ b/src/world/persistence/level_data.zig @@ -9,7 +9,11 @@ const log = @import("../../engine/core/log.zig"); fn currentTimestampMs() i64 { const inst = std.time.Instant.now() catch return 0; - return @as(i64, inst.timestamp.sec) * std.time.ms_per_s + @divTrunc(@as(i64, inst.timestamp.nsec), @as(i64, std.time.ns_per_ms)); + const sec: i64 = inst.timestamp.sec; + const nsec: i64 = inst.timestamp.nsec; + const ms_from_sec = std.math.mul(i64, sec, std.time.ms_per_s) catch return std.math.maxInt(i64); + const ms_from_nsec = @divTrunc(nsec, std.time.ns_per_ms); + return ms_from_sec +| ms_from_nsec; } pub const LevelData = struct { diff --git a/src/world/persistence/save_manager.zig b/src/world/persistence/save_manager.zig index b061442f..19acd4a0 100644 --- a/src/world/persistence/save_manager.zig +++ b/src/world/persistence/save_manager.zig @@ -25,7 +25,11 @@ const MAX_OPEN_REGIONS: usize = 16; fn currentTimestampMs() i64 { const inst = std.time.Instant.now() catch return 0; - return @as(i64, @intCast(inst.timestamp.sec)) * std.time.ms_per_s + @divTrunc(@as(i64, @intCast(inst.timestamp.nsec)), @as(i64, std.time.ns_per_ms)); + const sec: i64 = inst.timestamp.sec; + const nsec: i64 = inst.timestamp.nsec; + const ms_from_sec = std.math.mul(i64, sec, std.time.ms_per_s) catch return std.math.maxInt(i64); + const ms_from_nsec = @divTrunc(nsec, std.time.ns_per_ms); + return ms_from_sec +| ms_from_nsec; } pub const LoadResult = enum { @@ -62,6 +66,9 @@ pub const SaveManager = struct { running: std.atomic.Value(bool), pending_saves: std.atomic.Value(usize), + failed_mutex: std.Thread.Mutex, + failed_chunks: std.ArrayListUnmanaged(ChunkKey), + thread: std.Thread, region_cache_mutex: std.Thread.Mutex, @@ -95,6 +102,8 @@ pub const SaveManager = struct { .thread = undefined, .region_cache_mutex = .{}, .region_cache = .empty, + .failed_mutex = .{}, + .failed_chunks = .empty, .level_data = blk: { var ld = LevelData.init(seed, ""); const generator_copy = try allocator.dupe(u8, generator_name); @@ -115,7 +124,7 @@ pub const SaveManager = struct { } pub fn deinit(self: *SaveManager) void { - self.flush(); + _ = self.flush(); self.running.store(false, .release); self.thread.join(); @@ -128,6 +137,7 @@ pub const SaveManager = struct { }; self.queue.deinit(self.allocator); + self.failed_chunks.deinit(self.allocator); self.save_dir.close(); @@ -138,6 +148,8 @@ pub const SaveManager = struct { } pub fn enqueueSave(self: *SaveManager, chunk: *const Chunk) void { + std.debug.assert(chunk.pin_count.load(.acquire) > 0); + const snapshot = SaveQueueEntry{ .chunk_x = chunk.chunk_x, .chunk_z = chunk.chunk_z, @@ -207,7 +219,7 @@ pub const SaveManager = struct { self.last_auto_save_ms = currentTimestampMs(); } - pub fn flush(self: *SaveManager) void { + pub fn flush(self: *SaveManager) []ChunkKey { var spins: u32 = 0; while (spins < 12000) : (spins += 1) { self.queue_mutex.lock(); @@ -217,6 +229,12 @@ pub const SaveManager = struct { if (count == 0 and saving == 0) break; std.posix.nanosleep(0, 10 * std.time.ns_per_ms); } + + self.failed_mutex.lock(); + const failed = self.failed_chunks.items; + self.failed_chunks = .empty; + self.failed_mutex.unlock(); + return failed; } fn saveThreadFn(self: *SaveManager) void { @@ -261,6 +279,9 @@ pub const SaveManager = struct { for (batch[0..count]) |entry| { self.saveOneChunk(&entry) catch |err| { log.log.err("Failed to save chunk ({}, {}): {}", .{ entry.chunk_x, entry.chunk_z, err }); + self.failed_mutex.lock(); + self.failed_chunks.append(self.allocator, .{ .x = entry.chunk_x, .z = entry.chunk_z }) catch {}; + self.failed_mutex.unlock(); }; } self.pending_saves.store(0, .release); @@ -406,9 +427,11 @@ test "SaveManager enqueue and flush processes chunks" { chunk.setBlock(8, 64, 8, .stone); chunk.setBiome(0, 0, .forest); chunk.generated = true; + chunk.pin(); sm.enqueueSave(&chunk); - sm.flush(); + chunk.unpin(); + _ = sm.flush(); var loaded = Chunk.init(5, -3); try testing.expect(sm.loadChunk(5, -3, &loaded) == .success); @@ -448,13 +471,17 @@ test "SaveManager duplicate enqueue overwrites previous" { var chunk1 = Chunk.init(0, 0); chunk1.setBlock(5, 5, 5, .dirt); + chunk1.pin(); var chunk2 = Chunk.init(0, 0); chunk2.setBlock(5, 5, 5, .gold_ore); + chunk2.pin(); sm.enqueueSave(&chunk1); + chunk1.unpin(); sm.enqueueSave(&chunk2); - sm.flush(); + chunk2.unpin(); + _ = sm.flush(); var loaded = Chunk.init(0, 0); try testing.expect(sm.loadChunk(0, 0, &loaded) == .success); diff --git a/src/world/world.zig b/src/world/world.zig index 69c47bfe..879c6eef 100644 --- a/src/world/world.zig +++ b/src/world/world.zig @@ -249,18 +249,25 @@ pub const World = struct { while (iter.next()) |entry| { const chunk = &entry.value_ptr.*.chunk; if (chunk.modified and chunk.generated) { + chunk.pin(); sm.enqueueSave(chunk); dirty_keys.append(self.allocator, entry.key_ptr.*) catch {}; } } self.storage.chunks_mutex.unlockShared(); - sm.flush(); + const failed = sm.flush(); self.storage.chunks_mutex.lockShared(); for (dirty_keys.items) |key| { if (self.storage.chunks.get(key)) |data| { data.chunk.modified = false; + data.chunk.unpin(); + } + } + for (failed) |key| { + if (self.storage.chunks.get(key)) |data| { + data.chunk.modified = true; } } self.storage.chunks_mutex.unlockShared(); @@ -274,10 +281,11 @@ pub const World = struct { defer dirty_keys.deinit(self.allocator); self.storage.chunks_mutex.lockShared(); - var iter = self.storage.iteratorUnsafe(); - while (iter.next()) |entry| { + var iter_a = self.storage.iteratorUnsafe(); + while (iter_a.next()) |entry| { const chunk = &entry.value_ptr.*.chunk; if (chunk.modified and chunk.generated) { + chunk.pin(); sm.enqueueSave(chunk); dirty_keys.append(self.allocator, entry.key_ptr.*) catch {}; } @@ -289,6 +297,7 @@ pub const World = struct { for (dirty_keys.items) |key| { if (self.storage.chunks.get(key)) |data| { data.chunk.modified = false; + data.chunk.unpin(); } } } diff --git a/src/world/world_streamer.zig b/src/world/world_streamer.zig index 18628aba..76b9d8a0 100644 --- a/src/world/world_streamer.zig +++ b/src/world/world_streamer.zig @@ -396,13 +396,13 @@ pub const WorldStreamer = struct { if (self.save_manager) |sm| { if (self.storage.chunks.get(key)) |data| { if (data.chunk.modified and data.chunk.generated) { + data.chunk.pin(); sm.enqueueSave(&data.chunk); + data.chunk.modified = false; + data.chunk.unpin(); } } } - if (self.storage.chunks.get(key)) |data| { - data.chunk.modified = false; - } _ = self.storage.removeUnlocked(key.x, key.z, self.vertex_allocator); } self.storage.chunks_mutex.unlock(); @@ -551,6 +551,7 @@ pub const WorldStreamer = struct { while (iter.next()) |entry| { const chunk = &entry.value_ptr.*.chunk; if (chunk.modified and chunk.generated) { + chunk.pin(); sm.enqueueSave(chunk); dirty_keys.append(self.allocator, entry.key_ptr.*) catch {}; } @@ -562,6 +563,7 @@ pub const WorldStreamer = struct { for (dirty_keys.items) |key| { if (self.storage.chunks.get(key)) |data| { data.chunk.modified = false; + data.chunk.unpin(); } } } From bfe6cc19230ea5eb9c2b3892415722675bfc889a Mon Sep 17 00:00:00 2001 From: MichaelFisher1997 Date: Fri, 3 Apr 2026 00:56:17 +0100 Subject: [PATCH 4/8] fix: address third round code review on SaveManager PR #411 - Fix generator_name allocation: pass dupe'd copy directly to init with errdefer - Add flush + failure tracking to world_streamer checkAutoSave - Extract timestampMs to shared time.zig utility with consistent overflow handling --- src/engine/core/time.zig | 9 +++++++++ src/world/persistence/level_data.zig | 14 +++----------- src/world/persistence/save_manager.zig | 23 +++++++---------------- src/world/world_streamer.zig | 9 ++++++++- 4 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/engine/core/time.zig b/src/engine/core/time.zig index 475fbdf7..b293d469 100644 --- a/src/engine/core/time.zig +++ b/src/engine/core/time.zig @@ -4,6 +4,15 @@ const std = @import("std"); const c = @import("../../c.zig").c; +pub fn timestampMs() i64 { + const inst = std.time.Instant.now() catch return 0; + const sec: i64 = inst.timestamp.sec; + const nsec: i64 = inst.timestamp.nsec; + const ms_from_sec = std.math.mul(i64, sec, std.time.ms_per_s) catch return std.math.maxInt(i64); + const ms_from_nsec = @divTrunc(nsec, std.time.ns_per_ms); + return std.math.add(i64, ms_from_sec, ms_from_nsec) catch return std.math.maxInt(i64); +} + pub const Time = struct { /// Time since last frame in seconds delta_time: f32 = 0, diff --git a/src/world/persistence/level_data.zig b/src/world/persistence/level_data.zig index 9430509a..b84e30e8 100644 --- a/src/world/persistence/level_data.zig +++ b/src/world/persistence/level_data.zig @@ -6,15 +6,7 @@ const std = @import("std"); const Allocator = std.mem.Allocator; const log = @import("../../engine/core/log.zig"); - -fn currentTimestampMs() i64 { - const inst = std.time.Instant.now() catch return 0; - const sec: i64 = inst.timestamp.sec; - const nsec: i64 = inst.timestamp.nsec; - const ms_from_sec = std.math.mul(i64, sec, std.time.ms_per_s) catch return std.math.maxInt(i64); - const ms_from_nsec = @divTrunc(nsec, std.time.ns_per_ms); - return ms_from_sec +| ms_from_nsec; -} +const timestampMs = @import("../../engine/core/time.zig").timestampMs; pub const LevelData = struct { seed: u64, @@ -25,7 +17,7 @@ pub const LevelData = struct { spawn_z: i32, pub fn init(seed: u64, generator_name: []const u8) LevelData { - const now = currentTimestampMs(); + const now = timestampMs(); return .{ .seed = seed, .generator_name = generator_name, @@ -110,7 +102,7 @@ pub const LevelData = struct { } pub fn touchLastPlayed(self: *LevelData) void { - self.last_played_timestamp = currentTimestampMs(); + self.last_played_timestamp = timestampMs(); } }; diff --git a/src/world/persistence/save_manager.zig b/src/world/persistence/save_manager.zig index 19acd4a0..a241d925 100644 --- a/src/world/persistence/save_manager.zig +++ b/src/world/persistence/save_manager.zig @@ -7,6 +7,7 @@ const std = @import("std"); const Allocator = std.mem.Allocator; const log = @import("../../engine/core/log.zig"); +const timestampMs = @import("../../engine/core/time.zig").timestampMs; const Chunk = @import("../chunk.zig").Chunk; const ChunkKey = @import("../chunk_storage.zig").ChunkKey; const RegionFile = @import("region_file.zig").RegionFile; @@ -23,15 +24,6 @@ const SAVE_THREAD_INTERVAL_NS: u64 = 100 * std.time.ns_per_ms; const AUTO_SAVE_INTERVAL_MS: i64 = 60_000; const MAX_OPEN_REGIONS: usize = 16; -fn currentTimestampMs() i64 { - const inst = std.time.Instant.now() catch return 0; - const sec: i64 = inst.timestamp.sec; - const nsec: i64 = inst.timestamp.nsec; - const ms_from_sec = std.math.mul(i64, sec, std.time.ms_per_s) catch return std.math.maxInt(i64); - const ms_from_nsec = @divTrunc(nsec, std.time.ns_per_ms); - return ms_from_sec +| ms_from_nsec; -} - pub const LoadResult = enum { success, not_found, @@ -105,12 +97,11 @@ pub const SaveManager = struct { .failed_mutex = .{}, .failed_chunks = .empty, .level_data = blk: { - var ld = LevelData.init(seed, ""); const generator_copy = try allocator.dupe(u8, generator_name); - ld.generator_name = generator_copy; - break :blk ld; + errdefer allocator.free(generator_copy); + break :blk LevelData.init(seed, generator_copy); }, - .last_auto_save_ms = currentTimestampMs(), + .last_auto_save_ms = timestampMs(), }; try sm.level_data.saveToFile(allocator, sm.save_dir); @@ -211,12 +202,12 @@ pub const SaveManager = struct { } pub fn shouldAutoSave(self: *const SaveManager) bool { - const now = currentTimestampMs(); + const now = timestampMs(); return (now - self.last_auto_save_ms) >= AUTO_SAVE_INTERVAL_MS; } pub fn markAutoSaved(self: *SaveManager) void { - self.last_auto_save_ms = currentTimestampMs(); + self.last_auto_save_ms = timestampMs(); } pub fn flush(self: *SaveManager) []ChunkKey { @@ -321,7 +312,7 @@ pub const SaveManager = struct { } fn getOrOpenRegion(self: *SaveManager, rx: i32, rz: i32) !*RegionFile { - const now_ms = currentTimestampMs(); + const now_ms = timestampMs(); for (self.region_cache.items) |*entry| { if (entry.region_x == rx and entry.region_z == rz) { diff --git a/src/world/world_streamer.zig b/src/world/world_streamer.zig index 76b9d8a0..02ec61c1 100644 --- a/src/world/world_streamer.zig +++ b/src/world/world_streamer.zig @@ -560,12 +560,19 @@ pub const WorldStreamer = struct { sm.markAutoSaved(); + const failed = sm.flush(); + + self.storage.chunks_mutex.lockShared(); for (dirty_keys.items) |key| { if (self.storage.chunks.get(key)) |data| { - data.chunk.modified = false; + const should_remark = for (failed) |f| { + if (f.x == key.x and f.z == key.z) break true; + } else false; + if (!should_remark) data.chunk.modified = false; data.chunk.unpin(); } } + self.storage.chunks_mutex.unlockShared(); } pub fn getStats(self: *WorldStreamer) struct { gen_queue: usize, mesh_queue: usize, upload_queue: usize } { From 6e697153af5c1a417796bc0540434693579a7ac8 Mon Sep 17 00:00:00 2001 From: MichaelFisher1997 Date: Fri, 3 Apr 2026 02:01:37 +0100 Subject: [PATCH 5/8] fix: add flush and failure tracking to World.checkAutoSave Reorders to flush() before markAutoSaved() and only clears modified flag on confirmed success, matching WorldStreamer.checkAutoSave pattern. --- src/world/world.zig | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/world/world.zig b/src/world/world.zig index 879c6eef..9019355b 100644 --- a/src/world/world.zig +++ b/src/world/world.zig @@ -292,11 +292,15 @@ pub const World = struct { } self.storage.chunks_mutex.unlockShared(); + const failed = sm.flush(); sm.markAutoSaved(); for (dirty_keys.items) |key| { if (self.storage.chunks.get(key)) |data| { - data.chunk.modified = false; + const should_remark = for (failed) |f| { + if (f.x == key.x and f.z == key.z) break true; + } else false; + if (!should_remark) data.chunk.modified = false; data.chunk.unpin(); } } From 962bd00169a94873e6f50097e84729502426f481 Mon Sep 17 00:00:00 2001 From: MichaelFisher1997 Date: Fri, 3 Apr 2026 02:26:32 +0100 Subject: [PATCH 6/8] fix: add mutex protection to World.checkAutoSave modified flag loop --- src/world/world.zig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/world/world.zig b/src/world/world.zig index 9019355b..72f99710 100644 --- a/src/world/world.zig +++ b/src/world/world.zig @@ -295,6 +295,7 @@ pub const World = struct { const failed = sm.flush(); sm.markAutoSaved(); + self.storage.chunks_mutex.lockShared(); for (dirty_keys.items) |key| { if (self.storage.chunks.get(key)) |data| { const should_remark = for (failed) |f| { @@ -304,6 +305,7 @@ pub const World = struct { data.chunk.unpin(); } } + self.storage.chunks_mutex.unlockShared(); } pub fn loadChunkFromSave(self: *World, cx: i32, cz: i32, out_chunk: *Chunk) LoadResult { From a20a15435e8f1b18742d77d0688131edf61e28fa Mon Sep 17 00:00:00 2001 From: MichaelFisher1997 Date: Fri, 3 Apr 2026 02:35:56 +0100 Subject: [PATCH 7/8] refactor: extract dirty chunk helpers and replace std.Io.Writer.Allocating in level_data.zig --- src/world/world.zig | 58 +++++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 36 deletions(-) diff --git a/src/world/world.zig b/src/world/world.zig index aacb39ca..10015418 100644 --- a/src/world/world.zig +++ b/src/world/world.zig @@ -238,11 +238,8 @@ pub const World = struct { self.save_manager = try SaveManager.init(self.allocator, save_dir_path, world_name, seed, gen_name); } - pub fn saveAllModifiedChunks(self: *World) void { - const sm = self.save_manager orelse return; - + fn enqueueModifiedChunks(self: *World, sm: *SaveManager) std.ArrayListUnmanaged(ChunkKey) { var dirty_keys = std.ArrayListUnmanaged(ChunkKey).empty; - defer dirty_keys.deinit(self.allocator); self.storage.chunks_mutex.lockShared(); var iter = self.storage.iteratorUnsafe(); @@ -256,56 +253,45 @@ pub const World = struct { } self.storage.chunks_mutex.unlockShared(); - const failed = sm.flush(); + return dirty_keys; + } + fn clearModifiedFlags(self: *World, keys: []const ChunkKey, failed: []const ChunkKey) void { self.storage.chunks_mutex.lockShared(); - for (dirty_keys.items) |key| { + for (keys) |key| { if (self.storage.chunks.get(key)) |data| { - data.chunk.modified = false; + const should_remark = for (failed) |f| { + if (f.x == key.x and f.z == key.z) break true; + } else false; + if (!should_remark) data.chunk.modified = false; data.chunk.unpin(); } } - for (failed) |key| { - if (self.storage.chunks.get(key)) |data| { - data.chunk.modified = true; - } - } self.storage.chunks_mutex.unlockShared(); } + pub fn saveAllModifiedChunks(self: *World) void { + const sm = self.save_manager orelse return; + + var dirty_keys = self.enqueueModifiedChunks(sm); + defer dirty_keys.deinit(self.allocator); + + const failed = sm.flush(); + + self.clearModifiedFlags(dirty_keys.items, failed); + } + pub fn checkAutoSave(self: *World) void { const sm = self.save_manager orelse return; if (!sm.shouldAutoSave()) return; - var dirty_keys = std.ArrayListUnmanaged(ChunkKey).empty; + var dirty_keys = self.enqueueModifiedChunks(sm); defer dirty_keys.deinit(self.allocator); - self.storage.chunks_mutex.lockShared(); - var iter_a = self.storage.iteratorUnsafe(); - while (iter_a.next()) |entry| { - const chunk = &entry.value_ptr.*.chunk; - if (chunk.modified and chunk.generated) { - chunk.pin(); - sm.enqueueSave(chunk); - dirty_keys.append(self.allocator, entry.key_ptr.*) catch {}; - } - } - self.storage.chunks_mutex.unlockShared(); - const failed = sm.flush(); sm.markAutoSaved(); - self.storage.chunks_mutex.lockShared(); - for (dirty_keys.items) |key| { - if (self.storage.chunks.get(key)) |data| { - const should_remark = for (failed) |f| { - if (f.x == key.x and f.z == key.z) break true; - } else false; - if (!should_remark) data.chunk.modified = false; - data.chunk.unpin(); - } - } - self.storage.chunks_mutex.unlockShared(); + self.clearModifiedFlags(dirty_keys.items, failed); } pub fn loadChunkFromSave(self: *World, cx: i32, cz: i32, out_chunk: *Chunk) LoadResult { From baafb7c577be80d1c7827daaf999769178661106 Mon Sep 17 00:00:00 2001 From: MichaelFisher1997 Date: Fri, 3 Apr 2026 02:48:27 +0100 Subject: [PATCH 8/8] refactor: extract enqueueModifiedChunks helper to reduce duplication Both saveAllModifiedChunks and checkAutoSave shared identical iteration logic for finding and enqueuing dirty chunks. Extracted into a private helper method. --- src/world/world.zig | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/world/world.zig b/src/world/world.zig index 10015418..44622e7a 100644 --- a/src/world/world.zig +++ b/src/world/world.zig @@ -256,20 +256,6 @@ pub const World = struct { return dirty_keys; } - fn clearModifiedFlags(self: *World, keys: []const ChunkKey, failed: []const ChunkKey) void { - self.storage.chunks_mutex.lockShared(); - for (keys) |key| { - if (self.storage.chunks.get(key)) |data| { - const should_remark = for (failed) |f| { - if (f.x == key.x and f.z == key.z) break true; - } else false; - if (!should_remark) data.chunk.modified = false; - data.chunk.unpin(); - } - } - self.storage.chunks_mutex.unlockShared(); - } - pub fn saveAllModifiedChunks(self: *World) void { const sm = self.save_manager orelse return; @@ -278,7 +264,19 @@ pub const World = struct { const failed = sm.flush(); - self.clearModifiedFlags(dirty_keys.items, failed); + self.storage.chunks_mutex.lockShared(); + for (dirty_keys.items) |key| { + if (self.storage.chunks.get(key)) |data| { + data.chunk.modified = false; + data.chunk.unpin(); + } + } + for (failed) |key| { + if (self.storage.chunks.get(key)) |data| { + data.chunk.modified = true; + } + } + self.storage.chunks_mutex.unlockShared(); } pub fn checkAutoSave(self: *World) void { @@ -291,7 +289,17 @@ pub const World = struct { const failed = sm.flush(); sm.markAutoSaved(); - self.clearModifiedFlags(dirty_keys.items, failed); + self.storage.chunks_mutex.lockShared(); + for (dirty_keys.items) |key| { + if (self.storage.chunks.get(key)) |data| { + const should_remark = for (failed) |f| { + if (f.x == key.x and f.z == key.z) break true; + } else false; + if (!should_remark) data.chunk.modified = false; + data.chunk.unpin(); + } + } + self.storage.chunks_mutex.unlockShared(); } pub fn loadChunkFromSave(self: *World, cx: i32, cz: i32, out_chunk: *Chunk) LoadResult {