feat: implement SaveManager with background save thread (#380) by MichaelFisher1997 · Pull Request #411 · OpenStaticFish/ZigCraft

MichaelFisher1997 · 2026-04-02T21:39:26Z

Summary

Implements the SaveManager system that ties together the existing RegionFile format (#372) and ChunkSerializer (#377) into a working save/load pipeline.

Closes #380

Changes

New Files

src/world/persistence/level_data.zig — Level metadata (level.dat) with seed, generator name, timestamps, and spawn position. Simple line-based serialization with round-trip tests.
src/world/persistence/save_manager.zig — Core save orchestrator:
- Background save thread with configurable poll interval
- Thread-safe dirty chunk queue (deduplication by chunk coords)
- LRU region file cache (max 16 open files)
- Auto-save every 60 seconds
- Chunk load from save (falls through to worldgen if not found)
- flush() for synchronous drain (used in deinit)

Modified Files

src/world/chunk.zig — Added modified: bool flag, set to true in setBlock()
src/world/world.zig — SaveManager integration: enableSaveManager(), saveAllModifiedChunks(), loadChunkFromSave(), auto-save in deinit()
src/world/world_streamer.zig — Tries loading from save before worldgen, saves modified chunks before unload, periodic auto-save check
src/game/session.zig — Wires up SaveManager via ZIGCRAFT_SAVE_DIR env var
src/tests.zig — Registers new test modules

Testing

All 492 tests pass (including new level_data and save_manager tests)
Build succeeds (Debug + ReleaseFast)
Code formatted with zig fmt

Design Decisions

Env var (ZIGCRAFT_SAVE_DIR) for save directory to keep session.zig changes minimal and avoid touching the app/settings layer
Save thread polls queue rather than using condition variables for simplicity
Region file handles are cached with LRU eviction to avoid repeated open/close overhead
pending_saves atomic counter ensures flush() waits until region writes are complete (not just queue drain)

…#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

github-actions · 2026-04-02T21:42:33Z

📋 Summary

This PR implements the SaveManager system (#380) that integrates RegionFile format and ChunkSerializer into a working save/load pipeline. It adds background save thread with polling, LRU region file cache, auto-save intervals, and proper dirty chunk tracking.

Linked Issue: Closes #380 - fully implements the requested save/load system.

The implementation is well-structured with good memory management practices (explicit allocators, defer/errdefer), thread-safe queue handling with mutex protection, and comprehensive test coverage. Overall code quality is high, following Zig and project conventions.

🔴 Critical Issues (Must Fix - Blocks Merge)

[CRITICAL] src/world/persistence/save_manager.zig:136-160 - Race Condition on Chunk Data Copy
Confidence: High
Description: enqueueSave() copies chunk data (blocks, light, biomes, heightmap arrays) while holding the queue mutex. However, the chunk could be modified by another thread (e.g., world.setBlock) between the time we check modified flag and when we copy the data, or during the copy itself. The large array copies (~70KB per chunk) with mutex held can also cause contention.
Impact: Corrupted save data if chunk is modified during enqueue; performance degradation under load
Suggested Fix: Pin the chunk before copying data, or copy data outside the mutex after taking a snapshot reference. Consider using a version counter or atomic state check:

pub fn enqueueSave(self: *SaveManager, chunk: *const Chunk) void {
    // Take snapshot of data first (chunk must be pinned by caller)
    const snapshot = .{
        .blocks = chunk.blocks,
        .light = chunk.light,
        .biomes = chunk.biomes,
        .heightmap = chunk.heightmap,
    };
    
    self.queue_mutex.lock();
    defer self.queue_mutex.unlock();
    // ... use snapshot data instead of accessing chunk directly
}

[CRITICAL] src/world/persistence/save_manager.zig:316-326 - TOCTOU Race in Region File Creation
Confidence: High
Description: getOrOpenRegion() checks if file exists via realpath(), then creates it if not found. Between the check and creation, another thread/process could create the file, causing createFile to fail or behave unexpectedly.
Impact: File creation errors, potential data corruption if region file is partially created
Suggested Fix: Use makeOpenPath pattern or open with exclusive create flag, handling EEXIST gracefully:

const region = blk: {
    // Try to open existing first
    if (self.save_dir.openFile(region_filename, .{})) |file| {
        file.close();
        var abs_buf: [std.fs.max_path_bytes]u8 = undefined;
        const abs_path = try self.save_dir.realpath(region_filename, &abs_buf);
        break :blk try RegionFile.open(self.allocator, abs_path);
    } else |_| {
        // Create new - use exclusive flag to fail if created concurrently
        var abs_buf: [std.fs.max_path_bytes]u8 = undefined;
        const file = self.save_dir.createFile(region_filename, .{ .read = true, .exclusive = true }) catch |err| {
            if (err == error.PathAlreadyExists) {
                // Someone else created it, try opening
                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();
        const abs_path = try self.save_dir.realpath(region_filename, &abs_buf);
        break :blk try RegionFile.create(self.allocator, abs_path);
    }
};

⚠️ High Priority Issues (Should Fix)

[HIGH] src/world/persistence/save_manager.zig:162-196 - Error Information Loss in loadChunk
Confidence: Medium
Description: loadChunk() returns bool and logs errors, but callers can't distinguish between "chunk doesn't exist" (normal case) and "read error" (corruption/hardware failure). This makes it impossible to implement proper error recovery.
Impact: Silent failures on data corruption, no way to alert user or attempt recovery
Suggested Fix: Return an error union or enum:

pub const LoadResult = enum {
    success,
    not_found,
    read_error,
    corrupt_data,
};

pub fn loadChunk(self: *SaveManager, cx: i32, cz: i32, out_chunk: *Chunk) LoadResult {
    // ... implementation returning specific result
}

[HIGH] src/world/world.zig:246-248 and src/world/world_streamer.zig:397-400,541-543 - Premature modified Flag Clear
Confidence: Medium
Description: The modified flag is cleared immediately after enqueueSave(), but if the save fails (disk full, corruption, etc.), the flag remains false and the chunk won't be saved again until modified. Data loss occurs silently.
Impact: Silent data loss on save failure
Suggested Fix: Only clear modified after successful save confirmation, or re-set it on save failure. Consider tracking save sequence numbers per chunk.

[HIGH] src/world/persistence/save_manager.zig:207-217 - Flush Timeout is Too Short
Confidence: Medium
Description: flush() waits only 20 seconds (2000 spins × 10ms) for saves to complete. On slow disks, high chunk counts, or during heavy I/O, this may timeout prematurely, causing deinit() to complete while saves are still in flight.
Impact: Data loss on shutdown if saves don't complete in 20 seconds
Suggested Fix: Increase timeout to 120+ seconds or use a condition variable for proper signaling when saves complete.

[HIGH] src/world/persistence/save_manager.zig:436-448 - Chunk State Set Without Mutex
Confidence: High
Description: In processGenJob(), chunk_data.chunk.state = .generated is set after loading from save, but this happens outside the storage mutex (mutex was unlocked at line 431).
Impact: Race condition with other threads accessing chunk state
Suggested Fix: Re-acquire mutex before setting state, or use atomic state transitions.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/world/persistence/level_data.zig:10-13 - Integer Overflow Risk in Timestamp
Confidence: Medium
Description: currentTimestampMs() uses @divTrunc which can panic on overflow. The timestamp calculation involves multiple casts that could overflow on systems with unusual time values.
Impact: Potential crash on timestamp calculation
Suggested Fix: Use saturating arithmetic or checked math operations.

[MEDIUM] src/world/persistence/save_manager.zig:141-146 - Large Array Copy on Duplicate Detection
Confidence: Medium
Description: When updating an existing queue entry, four large arrays (~70KB total) are copied by value. This happens while holding the mutex.
Impact: Mutex contention, performance degradation
Suggested Fix: Consider using reference counting or keeping chunk pinned instead of copying data.

[MEDIUM] src/world/persistence/save_manager.zig:100-101 - Duplicate generator_name Allocation
Confidence: High
Description: LevelData.init() already stores the generator_name pointer (line 27), but then it's duplicated again at line 100-101 and assigned, leaking the first pointer.
Impact: Memory leak (small, one-time per world)
Suggested Fix: Either don't pass generator_name to LevelData.init() or remove the duplicate allocation.

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/persistence/save_manager.zig:313 - Magic Number for Buffer Size
Confidence: Low
Description: The buffer size [512]u8 for region filename is arbitrary and may be too small for deeply nested save paths.
Suggested Fix: Use std.fs.max_path_bytes or calculate exact needed size.

[LOW] src/world/persistence/level_data.zig:42 - Non-Standard Writer Type
Confidence: Low
Description: std.Io.Writer.Allocating appears to be a non-standard type. Verify this is correct for your Zig version.
Suggested Fix: Consider using std.ArrayList(u8) with a writer instead for portability.

[LOW] src/world/world.zig:239-254,256-272 - Code Duplication in Save Methods
Confidence: Low
Description: saveAllModifiedChunks() and checkAutoSave() share nearly identical iteration logic.
Suggested Fix: Extract into a helper method.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	SaveManager handles save/load orchestration well; some mixing of queue management and region caching
Open/Closed	7/10	Good extensibility through error sets and interfaces; RegionFile integration is tight
Liskov Substitution	9/10	No inheritance issues; clear struct composition
Interface Segregation	8/10	Clean public API (init, deinit, enqueueSave, loadChunk, flush); internal methods are private
Dependency Inversion	7/10	Direct dependencies on RegionFile and ChunkSerializer; could use interfaces for testability
Average	7.8	Good overall adherence to SOLID principles

🎯 Final Assessment

Overall Confidence Score: 72%

How to interpret: Moderate concerns exist around race conditions and data consistency, but the overall architecture is sound.

Confidence Breakdown:

Code Quality: 85% (well-structured, follows conventions, good error handling)
Completeness: 90% (fully implements [Batch 3] Save manager: async thread, dirty tracking, auto-save #380 requirements)
Risk Level: 55% (race conditions present but impact is limited)
Test Coverage: 80% (good unit tests, missing concurrency/stress tests)

Merge Readiness:

All critical issues resolved
SOLID average score >= 6.0 (7.8 achieved)
Overall confidence >= 60% (72% achieved)
No security concerns (race conditions are reliability issues, not security)
Tests present and passing

Verdict:

MERGE WITH FIXES

The implementation is solid and meets requirements, but the race conditions in enqueueSave (data copy) and getOrOpenRegion (TOCTOU) should be addressed before merge to prevent data corruption. The premature modified flag clearing issue should also be fixed to prevent silent data loss on save failures.

opencode session | github run

- 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

github-actions · 2026-04-02T23:20:28Z

📋 Summary

Linked Issue: Closes #380 - fully implements the SaveManager system with background save thread, dirty chunk queue, LRU region file cache, and auto-save functionality.

This PR implements a complete save/load pipeline integrating RegionFile format and ChunkSerializer. It adds background save threading, LRU region file caching (max 16 files), auto-save every 60 seconds, and proper dirty chunk tracking. Three critical issues from the previous review have been addressed, but two race conditions remain that could cause data corruption.

🔴 Critical Issues (Must Fix - Blocks Merge)

[CRITICAL] src/world/persistence/save_manager.zig:140-163 - Race Condition on Chunk Data Copy
Confidence: High
Description: enqueueSave() copies all chunk data arrays (blocks, light, biomes, heightmap - ~70KB per chunk) without pinning the chunk first. The chunk could be modified by another thread between the snapshot creation and when the data is actually used.
Impact: Corrupted save data if chunk is modified during snapshot
Suggested Fix: Pin the chunk before copying, or document that callers must pin:

pub fn enqueueSave(self: *SaveManager, chunk: *const Chunk) void {
    // Caller must ensure chunk is pinned during this call
    std.debug.assert(chunk.pin_count.load(.acquire) > 0);
    // ... rest of implementation
}

[CRITICAL] src/world/world.zig:241-267 and src/world/world_streamer.zig:539-566 - Premature modified Flag Clear
Confidence: High
Description: The modified flag is cleared after flush() returns, but flush() only waits for the queue to drain and pending_saves to reach zero. It doesn't track which specific chunks failed to save. If a save fails (disk full, I/O error), the flag is still cleared and the chunk won't be saved again.
Impact: Silent data loss on save failure
Suggested Fix: Track save results per chunk and only clear modified on confirmed success, or re-set the flag on any save errors.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/world/persistence/level_data.zig:10-13 - Integer Overflow Risk in Timestamp
Confidence: Medium
Description: currentTimestampMs() uses @divTrunc on timestamp values. While unlikely, this could theoretically overflow.
Impact: Potential crash on timestamp calculation edge cases
Suggested Fix: Use @divTrunc with explicit overflow checking or use std.math.divTrunc.

[MEDIUM] src/world/persistence/save_manager.zig:153-156 - Large Array Copy While Holding Mutex
Confidence: Medium
Description: When updating an existing queue entry, all four large arrays (~70KB total) are copied by value while holding the queue mutex.
Impact: Mutex contention, performance degradation under load
Suggested Fix: Consider using std.mem.swap or tracking versions instead of full array copies.

[MEDIUM] src/world/persistence/save_manager.zig:99-101 - Duplicate generator_name Allocation
Confidence: High
Description: LevelData.init() stores an empty string for generator_name (line 99), then immediately overwrites it with a newly allocated copy (lines 100-101). The empty string from init is never freed but also never used.
Impact: Memory leak (small, one-time per world)
Suggested Fix: Either pass generator_name directly to LevelData.init() or remove the duplicate allocation.

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/persistence/level_data.zig:42 - Non-Standard Writer Type
Confidence: Medium
Description: std.Io.Writer.Allocating appears to be a non-standard or outdated Zig type. This may not compile with future Zig versions.
Suggested Fix: Use std.ArrayList(u8) with a writer instead:

var list = std.ArrayList(u8).init(allocator);
defer list.deinit();
const writer = list.writer();
// ... write operations
try file.writeAll(list.items);

[LOW] src/world/world.zig:241-267 and src/world/world_streamer.zig:539-566 - Code Duplication
Confidence: Low
Description: The iteration logic for finding dirty chunks is duplicated in saveAllModifiedChunks(), checkAutoSave() (world.zig), and checkAutoSave() (world_streamer.zig).
Suggested Fix: Extract a common helper method in the World class.

✅ Previously Reported Issues - Status

Issue	Status	Notes
TOCTOU Race in Region File Creation	✅ FIXED	Now uses `.exclusive = true` and handles `PathAlreadyExists`
Error Information Loss in `loadChunk`	✅ FIXED	Now returns `LoadResult` enum
Flush Timeout Too Short	✅ FIXED	Increased to 120 seconds (12000 spins)
Chunk State Set Without Mutex	✅ FIXED	Now properly locks with `lockShared()`

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	SaveManager handles save orchestration cleanly; queue and cache management are well-separated
Open/Closed	7/10	Good extensibility through error sets; could use interface for RegionFile
Liskov Substitution	9/10	No inheritance issues; clear struct composition
Interface Segregation	8/10	Clean public API with clear responsibilities
Dependency Inversion	7/10	Direct dependencies on concrete types; could abstract RegionFile for testing
Average	7.8	Good overall adherence to SOLID principles

🎯 Final Assessment

Overall Confidence Score: 75%

Confidence Breakdown:

Code Quality: 85% (well-structured, follows Zig conventions)
Completeness: 90% (fully implements [Batch 3] Save manager: async thread, dirty tracking, auto-save #380 requirements)
Risk Level: 60% (race conditions present but limited impact)
Test Coverage: 85% (good unit tests covering save/load round trips)

Merge Readiness:

All critical issues resolved
SOLID average score >= 6.0 (7.8 achieved)
Overall confidence >= 60% (75% achieved)
No critical race conditions
Tests present and passing

Verdict:

MERGE WITH FIXES

The implementation is solid and meets all requirements for #380. However, the race condition in enqueueSave should be addressed before merge to prevent potential data corruption. The premature modified flag clearing issue is also important but less severe since save failures are logged. The previous review's critical fixes (TOCTOU, loadChunk return type, flush timeout, mutex protection) have all been resolved.

opencode session | github run

- 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

github-actions · 2026-04-02T23:38:51Z

📋 Summary

Linked Issue: Closes #380 - Fully implements the SaveManager system with background save thread, dirty chunk queue, LRU region file cache, and auto-save functionality.

This PR implements a complete save/load pipeline integrating RegionFile format and ChunkSerializer. It adds background save threading, LRU region file caching (max 16 files), auto-save every 60 seconds, and proper dirty chunk tracking. Four critical issues from previous reviews have been addressed.

🔴 Critical Issues (Must Fix - Blocks Merge)

[CRITICAL] src/world/persistence/save_manager.zig:107-111 - Double Allocation of generator_name
Confidence: High
Description: LevelData.init() is called with an empty string (line 108), then generator_name is duplicated and assigned (lines 109-110). This leaks the empty string from init.
Impact: Memory leak (small, one-time per world initialization)
Suggested Fix: Pass generator_name directly to init():

.level_data = blk: {
    const generator_copy = try allocator.dupe(u8, generator_name);
    errdefer allocator.free(generator_copy);
    break :blk LevelData.init(seed, generator_copy);
},

[CRITICAL] src/world/world_streamer.zig:542-569 - modified Flag Cleared Before Save Confirmation
Confidence: High
Description: In checkAutoSave(), the modified flag is cleared immediately after calling markAutoSave() without waiting for the queue to be processed. Unlike saveAllModifiedChunks() which uses flush(), this method doesn't track failures or wait for confirmation.
Impact: Silent data loss if save fails - chunk won't be saved again until modified again
Suggested Fix: Add flush and failure tracking similar to saveAllModifiedChunks():

const failed = sm.flush();
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();
    }
}

⚠️ High Priority Issues (Should Fix)

[HIGH] src/world/persistence/save_manager.zig:150-175 - Large Array Copies Under Mutex
Confidence: Medium
Description: enqueueSave() copies ~70KB of chunk data (4 large arrays) while holding the queue mutex. When updating an existing entry, it copies all arrays again by value.
Impact: Mutex contention, performance degradation under high save load
Suggested Fix: Move snapshot creation outside the mutex:

pub fn enqueueSave(self: *SaveManager, chunk: *const Chunk) void {
    std.debug.assert(chunk.pin_count.load(.acquire) > 0);
    
    // Create snapshot before acquiring lock
    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();
    // ... rest unchanged
}

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/world/persistence/level_data.zig:10-16 and save_manager.zig:26-32 - Timestamp Overflow
Confidence: Medium
Description: Both files duplicate currentTimestampMs() which uses @math.mul() with overflow checking but then uses +| for final addition which could behave unexpectedly on overflow.
Impact: Potential timestamp wraparound issues on systems with unusual time values
Suggested Fix: Extract to a shared utility function and use consistent overflow handling:

fn currentTimestampMs() i64 {
    const inst = std.time.Instant.now() catch return 0;
    return std.math.add(i64, 
        std.math.mul(i64, inst.timestamp.sec, std.time.ms_per_s) catch return std.math.maxInt(i64),
        @divTrunc(inst.timestamp.nsec, std.time.ns_per_ms)
    ) catch return std.math.maxInt(i64);
}

[MEDIUM] src/world/persistence/level_data.zig:46 - Non-Standard Writer Type
Confidence: Medium
Description: std.Io.Writer.Allocating is not a standard Zig type. This appears to be an experimental or outdated API.
Impact: Compilation failure on future Zig versions
Suggested Fix: Use standard std.ArrayList(u8):

var list = std.ArrayList(u8).init(allocator);
defer list.deinit();
const writer = list.writer();
// ... write operations
try file.writeAll(list.items);

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/world.zig:241-273 and src/world/world_streamer.zig:542-569 - Code Duplication
Confidence: Low
Description: Both saveAllModifiedChunks() and checkAutoSave() share nearly identical iteration logic.
Suggested Fix: Extract a helper method to iterate dirty chunks.

✅ Previously Reported Issues - Status

Issue	Status	Notes
TOCTOU Race in Region File Creation	✅ FIXED	Uses `.exclusive = true` and handles `PathAlreadyExists` correctly
Error Information Loss in `loadChunk`	✅ FIXED	Now returns `LoadResult` enum with specific error types
Flush Timeout Too Short	✅ FIXED	Increased to 12000 spins (120 seconds)
Chunk State Set Without Mutex	✅ FIXED	Now properly uses `lockShared()`/`unlockShared()`
Race Condition in `enqueueSave`	⚠️ PARTIAL	Assert added but underlying issue of copying unpinned data remains theoretical risk

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	SaveManager cleanly separates concerns (queue, cache, serialization)
Open/Closed	7/10	Good extensibility through `LoadResult` enum; RegionFile coupling is acceptable
Liskov Substitution	9/10	No inheritance; clean struct composition
Interface Segregation	8/10	Clear public API with minimal surface area
Dependency Inversion	7/10	Direct dependencies on concrete types; could abstract for testing
Average	7.8	Good adherence to SOLID principles

🎯 Final Assessment

Overall Confidence Score: 78%

Confidence Breakdown:

Code Quality: 85% (well-structured, follows Zig conventions, good error handling)
Completeness: 92% (fully implements [Batch 3] Save manager: async thread, dirty tracking, auto-save #380 requirements)
Risk Level: 65% (double allocation and modified flag issues need fixing)
Test Coverage: 88% (492 tests passing, good unit test coverage)

Merge Readiness:

All critical issues resolved
SOLID average score >= 6.0 (7.8 achieved)
Overall confidence >= 60% (78% achieved)
No data loss risks
Tests present and passing (492 tests pass)

Verdict:

MERGE WITH FIXES

The implementation is solid and meets #380 requirements. However, two issues should be addressed before merge:

Double allocation of generator_name causes a memory leak
modified flag cleared prematurely in checkAutoSave() risks data loss on save failure

Both fixes are straightforward (10-15 lines each). Previous review's critical fixes (TOCTOU, error handling, flush timeout, mutex protection) have all been resolved correctly.

opencode session | github run

- 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

github-actions · 2026-04-03T00:06:55Z

Perfect! All 492 tests pass. Now I can complete my review analysis.

📋 Summary

Linked Issue: Closes #380 - Fully implements the SaveManager system with background save thread, dirty chunk queue, LRU region file cache, and auto-save functionality.

This PR implements a complete save/load pipeline integrating RegionFile format and ChunkSerializer. It adds background save threading, LRU region file caching (max 16 files), auto-save every 60 seconds, and proper dirty chunk tracking. Most critical issues from previous reviews have been addressed.

🔴 Critical Issues (Must Fix - Blocks Merge)

[CRITICAL] src/world/world.zig:276-303 - World.checkAutoSave() Clears modified Flag Without Failure Checking
Confidence: High
Description: Unlike WorldStreamer.checkAutoSave(), the World.checkAutoSave() method clears the modified flag without checking if the save actually succeeded. It also calls markAutoSaved() before flush(), which can update the timestamp even if saves fail.
Impact: Silent data loss if auto-save fails - chunks marked as saved when they weren't
Suggested Fix: Follow the same pattern as WorldStreamer.checkAutoSave():

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_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();  // Get failed chunks
    sm.markAutoSaved();  // Only mark after flush

    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;  // Only clear on success
            data.chunk.unpin();
        }
    }
    self.storage.chunks_mutex.unlockShared();
}

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

None identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/world.zig:241-273 and src/world/world.zig:276-303 - Code Duplication
Confidence: Low
Description: saveAllModifiedChunks() and checkAutoSave() share nearly identical iteration logic. The main difference is that one uses flush() and the other doesn't.
Suggested Fix: Extract a helper method to iterate dirty chunks, or have checkAutoSave() call saveAllModifiedChunks() with appropriate timeout handling.

✅ Previously Reported Issues - Status

Issue	Status	Notes
TOCTOU Race in Region File Creation	✅ FIXED	Uses `.exclusive = true` and handles `PathAlreadyExists` correctly
Error Information Loss in `loadChunk`	✅ FIXED	Returns `LoadResult` enum with specific error types
Flush Timeout Too Short	✅ FIXED	Increased to 12000 spins (120 seconds)
Chunk State Set Without Mutex	✅ FIXED	Properly uses `lockShared()`/`unlockShared()` in `processGenJob()`
Double allocation of `generator_name`	✅ NOT AN ISSUE	Allocation happens once; no leak
`modified` Flag in `WorldStreamer.checkAutoSave()`	✅ FIXED	Now properly tracks failures and only clears on success
Large Array Copies in `enqueueSave()`	✅ FIXED	Snapshot created BEFORE acquiring mutex (lines 144-151)

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	SaveManager cleanly separates concerns (queue, cache, serialization)
Open/Closed	7/10	Good extensibility through `LoadResult` enum; RegionFile coupling is acceptable
Liskov Substitution	9/10	No inheritance issues; clear struct composition
Interface Segregation	8/10	Clean public API with clear responsibilities
Dependency Inversion	7/10	Direct dependencies on concrete types; could abstract for testing
Average	7.8	Good adherence to SOLID principles

🎯 Final Assessment

Overall Confidence Score: 82%

Confidence Breakdown:

Code Quality: 88% (well-structured, follows Zig conventions, good error handling)
Completeness: 92% (fully implements [Batch 3] Save manager: async thread, dirty tracking, auto-save #380 requirements)
Risk Level: 75% (one data loss risk remains in World.checkAutoSave())
Test Coverage: 90% (492 tests passing, good unit test coverage)

Merge Readiness:

All critical issues resolved
SOLID average score >= 6.0 (7.8 achieved)
Overall confidence >= 60% (82% achieved)
No data loss risks
Tests present and passing (492 tests pass)

Verdict:

MERGE WITH FIXES

The implementation is solid and meets #380 requirements. However, the World.checkAutoSave() method has a critical issue where it clears the modified flag without checking for save failures, which could lead to silent data loss. This should be fixed to match the pattern used in WorldStreamer.checkAutoSave() before merging. The fix is straightforward (~10 lines). All previous review issues have been properly addressed.

opencode session | github run

Reorders to flush() before markAutoSaved() and only clears modified flag on confirmed success, matching WorldStreamer.checkAutoSave pattern.

github-actions · 2026-04-03T01:03:18Z

I attempted to run gh pr diff 411 and gh pr view 411 but the tool call was malformed. Let me fetch the PR information correctly. I'll examine the diff and view the PR details to provide a proper code review.

opencode session | github run

github-actions · 2026-04-03T01:19:05Z

📋 Summary

Linked Issue: Closes #380 - Fully implements the SaveManager system with background save thread, dirty chunk queue, LRU region file cache, and auto-save functionality.

This PR implements a complete save/load pipeline integrating RegionFile format and ChunkSerializer. It adds background save threading, LRU region file caching (max 16 files), auto-save every 60 seconds, and proper dirty chunk tracking. Six critical issues from previous reviews have been addressed, but two issues remain.

🔴 Critical Issues (Must Fix - Blocks Merge)

[CRITICAL] src/world/world.zig:298-306 - Race Condition in checkAutoSave() Missing Mutex Protection
Confidence: High
Description: The loop clearing modified flags in World.checkAutoSave() accesses self.storage.chunks.get(key) and modifies data.chunk.modified without holding the storage mutex. Unlike WorldStreamer.checkAutoSave() which properly uses lockShared() before the loop (lines 565-575), this code has a race condition where another thread could be modifying chunks concurrently.
Impact: Data corruption, torn reads/writes, or crashes due to unsynchronized access to chunk storage
Suggested Fix: Add mutex protection around the chunk access loop:

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();

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/world/persistence/level_data.zig:38 - Non-Standard Writer Type
Confidence: Medium
Description: std.Io.Writer.Allocating is not a standard Zig type. This appears to be experimental or outdated API that may not compile with future Zig versions. Tests currently pass but this is a compatibility risk.
Impact: Compilation failure on future Zig versions
Suggested Fix: Use standard std.ArrayList(u8):

var list = std.ArrayList(u8).init(allocator);
defer list.deinit();
const writer = list.writer();
// ... write operations
try file.writeAll(list.items);

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/world.zig:241-273 and src/world/world.zig:276-307 - Code Duplication
Confidence: Low
Description: saveAllModifiedChunks() and checkAutoSave() share nearly identical iteration logic. The main difference is that one uses flush() and the other doesn't.
Suggested Fix: Extract a helper method to iterate dirty chunks, or have checkAutoSave() call saveAllModifiedChunks() with appropriate timeout handling.

✅ Previously Reported Issues - Status

Issue	Status	Notes
TOCTOU Race in Region File Creation	✅ FIXED	Uses `.exclusive = true` and handles `PathAlreadyExists` correctly
Error Information Loss in `loadChunk`	✅ FIXED	Returns `LoadResult` enum with specific error types
Flush Timeout Too Short	✅ FIXED	Increased to 12000 spins (120 seconds)
Chunk State Set Without Mutex	✅ FIXED	Properly uses `lockShared()`/`unlockShared()` in `processGenJob()`
Double allocation of `generator_name`	✅ FIXED	Allocation happens once via blk pattern; no leak
`modified` Flag in `WorldStreamer.checkAutoSave()`	✅ FIXED	Now properly tracks failures and only clears on success
Large Array Copies in `enqueueSave()`	✅ FIXED	Snapshot created BEFORE acquiring mutex (lines 144-151)

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	SaveManager cleanly separates concerns (queue, cache, serialization)
Open/Closed	7/10	Good extensibility through `LoadResult` enum; RegionFile coupling is acceptable
Liskov Substitution	9/10	No inheritance issues; clear struct composition
Interface Segregation	8/10	Clean public API with clear responsibilities
Dependency Inversion	7/10	Direct dependencies on concrete types; could abstract for testing
Average	7.8	Good adherence to SOLID principles

🎯 Final Assessment

Overall Confidence Score: 78%

Confidence Breakdown:

Code Quality: 85% (well-structured, follows Zig conventions, good error handling)
Completeness: 92% (fully implements [Batch 3] Save manager: async thread, dirty tracking, auto-save #380 requirements)
Risk Level: 65% (mutex issue in World.checkAutoSave needs fixing)
Test Coverage: 90% (492 tests passing, good unit test coverage)

Merge Readiness:

All critical issues resolved
SOLID average score >= 6.0 (7.8 achieved)
Overall confidence >= 60% (78% achieved)
No race conditions
Tests present and passing (492 tests pass)

Verdict:

MERGE WITH FIXES

The implementation is solid and meets #380 requirements. However, one critical issue should be addressed before merge:

Race condition in World.checkAutoSave() (lines 298-306) - The loop clearing modified flags accesses chunk storage without mutex protection, unlike the correctly-implemented WorldStreamer.checkAutoSave(). This is a straightforward 3-line fix (add lockShared() before the loop and unlockShared() after).

The std.Io.Writer.Allocating issue is medium priority but should be addressed for future Zig compatibility. All previous review issues have been properly resolved.

opencode session | github run

github-actions · 2026-04-03T01:29:38Z

📋 Summary

Linked Issue: Closes #380 - Fully implements the SaveManager system with background save thread, dirty chunk queue, LRU region file cache, and auto-save functionality.

This PR implements a complete save/load pipeline integrating RegionFile format and ChunkSerializer. It adds background save threading, LRU region file caching (max 16 files), auto-save every 60 seconds, and proper dirty chunk tracking. All previously reported critical issues have been resolved.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/world/persistence/level_data.zig:38 - Non-Standard Writer Type
Confidence: Medium
Description: std.Io.Writer.Allocating is not a standard Zig type. This appears to be experimental or outdated API that may not compile with future Zig versions. Tests currently pass but this is a compatibility risk.
Impact: Compilation failure on future Zig versions
Suggested Fix: Use standard std.ArrayList(u8):

var list = std.ArrayList(u8).init(allocator);
defer list.deinit();
const writer = list.writer();
// ... write operations
try file.writeAll(list.items);

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/world.zig:241-273 and src/world/world.zig:276-307 - Code Duplication
Confidence: Low
Description: saveAllModifiedChunks() and checkAutoSave() share nearly identical iteration logic. The main difference is that one uses flush() and the other doesn't.
Suggested Fix: Extract a helper method to iterate dirty chunks, or have checkAutoSave() call saveAllModifiedChunks() with appropriate timeout handling.

✅ Previously Reported Issues - Status

Issue	Status	Notes
TOCTOU Race in Region File Creation	✅ FIXED	Uses `.exclusive = true` and handles `PathAlreadyExists` correctly
Error Information Loss in `loadChunk`	✅ FIXED	Returns `LoadResult` enum with specific error types
Flush Timeout Too Short	✅ FIXED	Increased to 12000 spins (120 seconds)
Chunk State Set Without Mutex	✅ FIXED	Properly uses `lockShared()`/`unlockShared()` in `processGenJob()`
Double allocation of `generator_name`	✅ FIXED	Allocation happens once via blk pattern; no leak
`modified` Flag in `WorldStreamer.checkAutoSave()`	✅ FIXED	Now properly tracks failures and only clears on success
Large Array Copies in `enqueueSave()`	✅ FIXED	Snapshot created BEFORE acquiring mutex (lines 144-151)
Race condition in `World.checkAutoSave()`	✅ FIXED	Now properly uses `lockShared()`/`unlockShared()` around chunk access

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	SaveManager cleanly separates concerns (queue, cache, serialization)
Open/Closed	7/10	Good extensibility through `LoadResult` enum; RegionFile coupling is acceptable
Liskov Substitution	9/10	No inheritance issues; clear struct composition
Interface Segregation	8/10	Clean public API with clear responsibilities
Dependency Inversion	7/10	Direct dependencies on concrete types; could abstract for testing
Average	7.8	Good adherence to SOLID principles

🎯 Final Assessment

Overall Confidence Score: 85%

Confidence Breakdown:

Code Quality: 88% (well-structured, follows Zig conventions, good error handling)
Completeness: 95% (fully implements [Batch 3] Save manager: async thread, dirty tracking, auto-save #380 requirements)
Risk Level: 80% (all race conditions resolved, only minor API compatibility concern)
Test Coverage: 90% (492 tests passing, good unit test coverage)

Merge Readiness:

All critical issues resolved
SOLID average score >= 6.0 (7.8 achieved)
Overall confidence >= 60% (85% achieved)
No critical race conditions
Tests present and passing (492 tests pass)

Verdict:

MERGE

All critical issues from previous reviews have been resolved. The implementation is solid, well-tested, and fully satisfies issue #380 requirements. The non-standard std.Io.Writer.Allocating type should be addressed in a follow-up PR for future Zig compatibility, but it doesn't block merge since tests pass.

opencode session | github run

github-actions · 2026-04-03T01:40:07Z

Based on my thorough analysis of the PR diff and previous review history, here is my comprehensive review:

📋 Summary

Linked Issue: Closes #380 - Fully implements the SaveManager system with background save thread, dirty chunk queue, LRU region file cache, and auto-save functionality.

This PR implements a complete save/load pipeline integrating RegionFile format and ChunkSerializer. After multiple review iterations, all previously reported critical issues have been addressed including: TOCTOU race conditions, proper mutex protection, error handling improvements, and memory management fixes. The implementation is well-structured with 492 passing tests.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/world/persistence/level_data.zig:38 - Non-Standard Writer Type
Confidence: Medium
Description: std.Io.Writer.Allocating is not a standard Zig type in the current stable API. This appears to be experimental or from an older Zig version. While tests pass now, this may cause compilation failures in future Zig updates.
Impact: Future compilation compatibility risk
Suggested Fix: Use standard std.ArrayList(u8) pattern:

var list = std.ArrayList(u8).init(allocator);
defer list.deinit();
const writer = list.writer();
// ... write operations
try file.writeAll(list.items);

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/world.zig:232-315 - Code Duplication
Confidence: Low
Description: saveAllModifiedChunks() and checkAutoSave() share nearly identical chunk iteration logic. Both methods iterate dirty chunks, pin them, enqueue saves, flush, and handle failures.
Suggested Fix: Extract common helper:

fn iterateDirtyChunks(self: *World, callback: *const fn(*Chunk) void) void {
    // Common iteration logic
}

✅ Previously Reported Issues - Status Verification

Issue	Status	Verification
TOCTOU Race in Region File Creation	✅ FIXED	Lines 393-408: Uses `.exclusive = true` with proper `PathAlreadyExists` handling
Error Information Loss in `loadChunk`	✅ FIXED	Lines 234-268: Returns `LoadResult` enum with specific error types
Flush Timeout Too Short	✅ FIXED	Lines 306-318: Uses 12000 spins (120 seconds)
Chunk State Set Without Mutex	✅ FIXED	Lines 456-460: Properly uses `lockShared()`/`unlockShared()`
Double allocation of `generator_name`	✅ FIXED	Lines 107-111: Single allocation via `blk` pattern
`modified` Flag Premature Clear	✅ FIXED	Lines 264-274, 298-313: Properly tracks failures and only clears on success
Large Array Copies Under Mutex	✅ FIXED	Lines 150-175: Snapshot created BEFORE acquiring mutex
Race Condition in `checkAutoSave()`	✅ FIXED	Lines 298-313: Proper mutex protection with lockShared/unlockShared

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	SaveManager cleanly separates concerns (queue, cache, serialization)
Open/Closed	7/10	Good extensibility through `LoadResult` enum; RegionFile coupling is acceptable
Liskov Substitution	9/10	No inheritance; clear struct composition
Interface Segregation	8/10	Clean public API with clear responsibilities
Dependency Inversion	7/10	Direct dependencies on concrete types; could abstract for testing
Average	7.8	Good adherence to SOLID principles

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

Code Quality: 90% (well-structured, follows Zig conventions, good error handling)
Completeness: 95% (fully implements [Batch 3] Save manager: async thread, dirty tracking, auto-save #380 requirements)
Risk Level: 85% (all race conditions resolved, minor API compatibility concern)
Test Coverage: 90% (492 tests passing, good unit test coverage)

Merge Readiness:

All critical issues resolved
SOLID average score >= 6.0 (7.8 achieved)
Overall confidence >= 60% (88% achieved)
No critical race conditions
Tests present and passing

Verdict:

MERGE

All critical issues from previous reviews have been resolved. The implementation is solid, well-tested, and fully satisfies issue #380 requirements. The non-standard std.Io.Writer.Allocating type is a minor compatibility concern that should be addressed in a follow-up PR, but it doesn't block merge since all 492 tests pass.

opencode session | github run

…ating in level_data.zig

Both saveAllModifiedChunks and checkAutoSave shared identical iteration logic for finding and enqueuing dirty chunks. Extracted into a private helper method.

github-actions · 2026-04-03T01:52:11Z

Based on my comprehensive review of PR #411, comparing the current code against previous automated reviews:

📋 Summary

Linked Issue: Closes #380 - Fully implements the SaveManager system with background save thread, dirty chunk queue, LRU region file cache (max 16 files), auto-save every 60 seconds, and proper dirty chunk tracking.

This PR implements a complete save/load pipeline integrating RegionFile format and ChunkSerializer. After multiple review iterations, all previously reported critical issues have been successfully resolved.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

Verification of fixes:

TOCTOU Race (save_manager.zig:336-345): ✅ Uses .exclusive = true with proper PathAlreadyExists handling
Error Information Loss (save_manager.zig:168-202): ✅ Returns LoadResult enum with specific error types
Flush Timeout (save_manager.zig:213-228): ✅ Uses 12000 spins (120 seconds)
Chunk State Mutex (world_streamer.zig): ✅ Properly uses lockShared()/unlockShared()
Double allocation: ✅ Single allocation via blk pattern (save_manager.zig:107-111)
Modified flag clearing (world.zig:264-279, 292-302): ✅ Properly tracks failures and only clears on success
Array copies under mutex (save_manager.zig:140-166): ✅ Snapshot created BEFORE acquiring mutex (lines 144-151)
Race in World.checkAutoSave() (world.zig:292-302): ✅ Now properly uses lockShared()/unlockShared()

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/world/persistence/level_data.zig:38 - Non-Standard Writer Type
Confidence: Medium
Description: std.Io.Writer.Allocating is not a standard Zig type. This may cause compilation failures in future Zig versions.
Impact: Future compatibility risk
Suggested Fix: Use standard std.ArrayList(u8) pattern for portability.

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/world.zig:232-315 - Code Duplication
Confidence: Low
Description: saveAllModifiedChunks() and checkAutoSave() share nearly identical iteration logic.
Suggested Fix: Extract a helper method for common dirty chunk iteration.

✅ Previously Reported Issues - Status

Issue	Status	Notes
TOCTOU Race in Region File Creation	✅ FIXED	Uses `.exclusive = true` with proper error handling
Error Information Loss in `loadChunk`	✅ FIXED	Returns `LoadResult` enum
Flush Timeout Too Short	✅ FIXED	12000 spins (120 seconds)
Chunk State Set Without Mutex	✅ FIXED	Proper `lockShared()`/`unlockShared()` usage
Double allocation of `generator_name`	✅ FIXED	Single allocation via `blk` pattern
`modified` Flag Premature Clear	✅ FIXED	Tracks failures, only clears on success
Large Array Copies Under Mutex	✅ FIXED	Snapshot before mutex acquisition
Race condition in `checkAutoSave()`	✅ FIXED	Proper mutex protection (lines 292-302)

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	SaveManager cleanly separates queue, cache, serialization
Open/Closed	7/10	Good extensibility through `LoadResult` enum
Liskov Substitution	9/10	No inheritance; clear struct composition
Interface Segregation	8/10	Clean public API with clear responsibilities
Dependency Inversion	7/10	Direct dependencies acceptable for this use case
Average	7.8	Good adherence to SOLID principles

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

Code Quality: 90% (well-structured, follows Zig conventions)
Completeness: 95% (fully implements [Batch 3] Save manager: async thread, dirty tracking, auto-save #380 requirements)
Risk Level: 85% (all race conditions resolved)
Test Coverage: 90% (492 tests passing)

Merge Readiness:

All critical issues resolved
SOLID average score >= 6.0 (7.8 achieved)
Overall confidence >= 60% (88% achieved)
No race conditions remaining
Tests present and passing

Verdict:

MERGE

All critical issues from previous reviews have been resolved. The implementation is solid, well-tested, and fully satisfies issue #380 requirements. The non-standard std.Io.Writer.Allocating type is a minor compatibility concern that should be addressed in a follow-up PR, but doesn't block merge since all tests pass.

opencode session | github run

github-actions bot added documentation Improvements or additions to documentation game world labels Apr 2, 2026

fix: address third round code review on SaveManager PR #411

bfe6cc1

- 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

github-actions bot added the engine label Apr 2, 2026

fix: add flush and failure tracking to World.checkAutoSave

6e69715

Reorders to flush() before markAutoSaved() and only clears modified flag on confirmed success, matching WorldStreamer.checkAutoSave pattern.

Merge branch 'dev' into feature/380-save-manager

7c62e57

fix: add mutex protection to World.checkAutoSave modified flag loop

962bd00

Merge branch 'dev' into feature/380-save-manager

f62cea9

MichaelFisher1997 added 2 commits April 3, 2026 02:48

refactor: extract dirty chunk helpers and replace std.Io.Writer.Alloc…

a20a154

…ating in level_data.zig

refactor: extract enqueueModifiedChunks helper to reduce duplication

baafb7c

Both saveAllModifiedChunks and checkAutoSave shared identical iteration logic for finding and enqueuing dirty chunks. Extracted into a private helper method.

MichaelFisher1997 merged commit 4166f8c into dev Apr 3, 2026
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement SaveManager with background save thread (#380)#411

feat: implement SaveManager with background save thread (#380)#411
MichaelFisher1997 merged 10 commits intodevfrom
feature/380-save-manager

MichaelFisher1997 commented Apr 2, 2026

Uh oh!

github-actions bot commented Apr 2, 2026

Uh oh!

github-actions bot commented Apr 2, 2026

Uh oh!

github-actions bot commented Apr 2, 2026

Uh oh!

github-actions bot commented Apr 3, 2026

Uh oh!

github-actions bot commented Apr 3, 2026

Uh oh!

github-actions bot commented Apr 3, 2026

Uh oh!

github-actions bot commented Apr 3, 2026

Uh oh!

github-actions bot commented Apr 3, 2026

Uh oh!

github-actions bot commented Apr 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MichaelFisher1997 commented Apr 2, 2026

Summary

Changes

New Files

Modified Files

Testing

Design Decisions

Uh oh!

github-actions bot commented Apr 2, 2026

📋 Summary

🔴 Critical Issues (Must Fix - Blocks Merge)

⚠️ High Priority Issues (Should Fix)

💡 Medium Priority Issues (Nice to Fix)

ℹ️ Low Priority Suggestions (Optional)

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 72%

Confidence Breakdown:

Merge Readiness:

Verdict:

Uh oh!

github-actions bot commented Apr 2, 2026

📋 Summary

🔴 Critical Issues (Must Fix - Blocks Merge)

⚠️ High Priority Issues (Should Fix)

💡 Medium Priority Issues (Nice to Fix)

ℹ️ Low Priority Suggestions (Optional)

✅ Previously Reported Issues - Status

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 75%

Confidence Breakdown:

Merge Readiness:

Verdict:

Uh oh!

github-actions bot commented Apr 2, 2026

📋 Summary

🔴 Critical Issues (Must Fix - Blocks Merge)

⚠️ High Priority Issues (Should Fix)

💡 Medium Priority Issues (Nice to Fix)

ℹ️ Low Priority Suggestions (Optional)

✅ Previously Reported Issues - Status

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 78%

Confidence Breakdown:

Merge Readiness:

Verdict:

Uh oh!

github-actions bot commented Apr 3, 2026

📋 Summary

🔴 Critical Issues (Must Fix - Blocks Merge)

⚠️ High Priority Issues (Should Fix)

💡 Medium Priority Issues (Nice to Fix)

ℹ️ Low Priority Suggestions (Optional)

✅ Previously Reported Issues - Status

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 82%

Confidence Breakdown:

Merge Readiness:

Verdict:

Uh oh!

github-actions bot commented Apr 3, 2026

Uh oh!

github-actions bot commented Apr 3, 2026

📋 Summary

🔴 Critical Issues (Must Fix - Blocks Merge)

⚠️ High Priority Issues (Should Fix)

💡 Medium Priority Issues (Nice to Fix)

ℹ️ Low Priority Suggestions (Optional)

✅ Previously Reported Issues - Status

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 78%

Confidence Breakdown:

Merge Readiness:

Verdict:

Uh oh!