feat: implement .mca-style region file format for chunk persistence

[MichaelFisher1997]

Summary

• Implements RegionFile struct in src/world/persistence/region_file.zig for .mca-style region file storage (32x32 chunk grid per file)
• Uses zlib compression via std.compress.flate for chunk payloads with sector-based allocation
• Supports open, create, readChunk, writeChunk, deleteChunk, hasChunk, and close operations
• Wired into test suite with 9 comprehensive tests

Test Coverage

• Round-trip write → close → reopen → read
• Multiple chunks in one region
• Chunk overwrite replaces data correctly
• Empty region creates valid file
• hasChunk reports correctly
• deleteChunk removes chunk
• readChunk on missing chunk returns error
• Binary data round-trip integrity
• Corrupt header handling (file too short)

Closes #372

[opencode-agent]

📋 Summary

Linked Issue: Closes #372 - Implements .mca-style region file format for chunk persistence.

This PR implements a complete region file persistence layer matching Minecraft's Anvil format. The implementation includes zlib compression, sector-based allocation, and comprehensive test coverage with 9 tests (all passing). The code follows Zig conventions with proper error handling, memory management, and packed structs for binary compatibility.

🔴 Critical Issues (Must Fix - Blocks Merge)

[CRITICAL] src/world/persistence/region_file.zig:195 - writeHeader silently ignores write failures
Confidence: High
Description: The writeHeader function uses catch return which silently swallows write errors. If the header write fails, the chunk data was written but the header won't reflect it, leaving the file in an inconsistent state.
Impact: Data corruption - chunks appear to save successfully but won't be found on next load, or worse, could point to wrong offsets.
Suggested Fix:

fn writeHeader(self: *RegionFile) !void {
    var buf: [HEADER_SIZE]u8 = undefined;
    for (&self.header, 0..HEADER_ENTRIES) |entry, i| {
        const raw: u32 = @bitCast(entry);
        std.mem.writeInt(u32, buf[i * 4 ..][0..4], raw, .big);
    }
    try self.file.pwriteAll(&buf, 0);
}

⚠️ High Priority Issues (Should Fix)

[HIGH] src/world/persistence/region_file.zig:37-41 - RegionFile is not thread-safe
Confidence: High
Description: The struct has no mutex protection. Concurrent calls to readChunk, writeChunk, or close from multiple threads will cause race conditions on the header array and closed flag.
Impact: Data races, file corruption, use-after-close bugs in multithreaded usage.
Suggested Fix: Add documentation stating "Not thread-safe - caller must synchronize" or wrap operations in a mutex.

[HIGH] src/world/persistence/region_file.zig:167,173 - No fsync after critical writes
Confidence: Medium
Description: After writing chunk data and updating the header, fsync is not called. If the system crashes, data may be lost even though the operation appeared successful.
Impact: Data loss on system crash or power failure.
Suggested Fix: Add try self.file.sync() after writeHeader() in writeChunk and deleteChunk.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/world/persistence/region_file.zig:19 - Magic number without context
Confidence: Low
Description: COMPRESSION_ZLIB: u8 = 2 uses magic number 2 without explaining it matches Minecraft Anvil format (1=GZip, 2=Zlib, 3=uncompressed).
Impact: Confusing for developers unfamiliar with Anvil format.
Suggested Fix: Add comment: // 2 = Zlib, matches Minecraft Anvil format spec.

[MEDIUM] src/world/persistence/region_file.zig:27-28 - Unused error variants
Confidence: High
Description: InvalidLocalX and InvalidLocalZ are defined but never used since parameters are u5 which naturally bounds them to 0-31.
Impact: Dead code, confusing API.
Suggested Fix: Remove unused error variants or add bounds checking if parameters change to wider types.

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/world/persistence/region_file.zig:198-207 - Inefficient sector allocation
Confidence: Medium
Description: findEndSector does a linear O(n) scan through all 1024 entries on every write that needs new space. No sector reuse when chunks are deleted.
Impact: Performance degradation with heavy write workloads.
Suggested Fix: Consider adding a free list or bitmap for sector tracking in future optimization.

[LOW] Missing module-level documentation on thread safety requirements
Confidence: Low
Description: Users need to know this struct requires external synchronization.
Suggested Fix: Add to module docs: "Thread safety: RegionFile is not thread-safe. Caller must ensure exclusive access."

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	RegionFile handles one concern - region file I/O
Open/Closed	7/10	Easy to extend compression types, but sector allocation is fixed
Liskov Substitution	N/A	No inheritance hierarchy
Interface Segregation	9/10	Clean public API with focused methods
Dependency Inversion	8/10	Depends on Allocator interface, file abstraction
Average	8.0	Well-designed module

🎯 Final Assessment

Overall Confidence Score: 75%

How to interpret: Minor concerns, mostly ready with some fixes needed

Confidence Breakdown:

• Code Quality: 85% (Well-structured, good error handling, proper memory management)
• Completeness: 90% (Full feature set, comprehensive tests)
• Risk Level: 60% (Critical issue with silent write failures, thread safety concerns)
• Test Coverage: 95% (9 tests covering main use cases)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing

Verdict:

MERGE WITH FIXES

The critical issue in writeHeader (line 195) must be fixed before merging - silent write failures can cause data corruption. Once fixed, this is a solid implementation ready for the dev branch.

opencode session  |  github run

[opencode-agent]

Now I have a complete picture of the code. Let me compile my review by comparing against the previous findings:

📋 Summary

Linked Issue: Closes #372 - Implements .mca-style region file format for chunk persistence.

This PR implements a complete region file persistence layer matching Minecraft's Anvil format with zlib compression, sector-based allocation, and 9 comprehensive tests. The implementation has been significantly improved since the previous review, with all previously reported critical and high-priority issues resolved.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high-priority issues have been resolved.

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: Magic number without context (now documented at line 21)
✅ [FIXED] Previous issue: Unused error variants (removed from RegionError enum)

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: Missing module-level thread safety documentation (now at line 12)

[LOW] src/world/persistence/region_file.zig:201-209 - Inefficient sector allocation (known limitation)
Confidence: Medium
Description: findEndSector does a linear O(n) scan through all 1024 entries. No sector reuse when chunks are deleted.
Impact: Minor performance degradation with heavy write workloads over time.
Suggested Fix: Document as known limitation or add TODO for future free list implementation.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9/10	RegionFile handles one concern - region file I/O
Open/Closed	7/10	Easy to extend compression types; sector allocation could be pluggable
Liskov Substitution	N/A	No inheritance hierarchy
Interface Segregation	9/10	Clean public API with focused methods
Dependency Inversion	9/10	Depends on Allocator interface, file abstraction
Average	8.5	Well-designed module

🎯 Final Assessment

Overall Confidence Score: 92%

How to interpret: Minor concerns, mostly ready with some fixes needed

Confidence Breakdown:

• Code Quality: 95% (Excellent structure, proper error handling, comprehensive tests)
• Completeness: 95% (Full feature set, all 9 tests passing)
• Risk Level: 85% (All critical issues fixed, only minor sector allocation inefficiency remains)
• Test Coverage: 95% (Excellent test coverage across all major use cases)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing

Verdict:

MERGE

All previously reported critical and high-priority issues have been resolved. The implementation is solid, well-tested, and follows Zig conventions. The only remaining item is a low-priority performance optimization that can be addressed in future iterations.

opencode session  |  github run

[opencode-agent]

📋 Summary

Linked Issue: Closes #372 - Implements .mca-style region file format for chunk persistence.

This PR implements a complete region file persistence layer matching Minecraft's Anvil format. The implementation has been reviewed multiple times and all previously reported critical and high-priority issues have been resolved. The code follows Zig conventions with proper error handling, memory management using defer/errdefer, packed structs for binary compatibility, and comprehensive test coverage with 9 tests.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

The previous critical issue regarding writeHeader silently ignoring write failures has been fixed - the function now properly propagates errors with try.

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high-priority issues have been resolved.

• Thread-safety warning added to module documentation (line 12)
• fsync called after critical writes in writeChunk (line 173) and deleteChunk (line 178)

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ All previously reported medium-priority issues have been resolved.

• Magic number documented (line 21: "Matches Minecraft Anvil format: 1=GZip, 2=Zlib, 3=Uncompressed")
• Unused error variants removed from RegionError enum

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: Missing module-level thread safety documentation (now present at line 12)

[LOW] src/world/persistence/region_file.zig:201-209 - Inefficient sector allocation (known limitation)
Confidence: Medium
Description: findEndSector does a linear O(n) scan through all 1024 entries on every write requiring new space. No sector reuse when chunks are deleted, leading to gradual file growth.
Impact: Minor performance degradation with heavy write workloads; file size grows monotonically.
Suggested Fix: Document as known limitation with TODO comment for future optimization (free list or bitmap-based sector management).

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9/10	RegionFile handles one concern - region file I/O operations
Open/Closed	7/10	Easy to extend compression types; sector allocation strategy is fixed
Liskov Substitution	N/A	No inheritance hierarchy
Interface Segregation	9/10	Clean public API with 6 focused methods (open, create, readChunk, writeChunk, deleteChunk, hasChunk)
Dependency Inversion	9/10	Depends on Allocator interface and file abstraction, not concrete implementations
Average	8.5	Well-designed module following Zig idioms

🎯 Final Assessment

Overall Confidence Score: 95%

How to interpret: High confidence, ready to merge or with trivial fixes

Confidence Breakdown:

• Code Quality: 95% (Excellent structure, proper error handling with try/errdefer, comprehensive tests)
• Completeness: 100% (Full feature set matching Anvil format, all 9 tests passing)
• Risk Level: 90% (All critical issues resolved, robust error propagation)
• Test Coverage: 95% (Excellent coverage: round-trip, multiple chunks, overwrite, empty regions, binary data, corruption handling)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing

Verdict:

MERGE

All previously reported critical and high-priority issues have been resolved. The implementation is production-ready with excellent test coverage, proper error handling, and clean Zig idioms. The only remaining item is a low-priority performance optimization for sector allocation that can be addressed in future iterations if needed.

opencode session  |  github run