feat: persist LOD source cache

[MichaelFisher1997]

Summary

• Add generator version/identity metadata for LOD cache invalidation.
• Serialize generated LOD source data to a CRC-checked disk cache under saved worlds.
• Load cached LOD source data on worker generation jobs and regenerate/delete on corrupt cache data.

Verification

• nix develop --command zig fmt modules/worldgen-api/src/root.zig modules/worldgen-overworld/src/overworld_generator.zig modules/worldgen-overworld-v2/src/root.zig modules/worldgen-flat/src/root.zig modules/worldgen-test/src/root.zig modules/world-lod/src/lod_generator.zig modules/world-lod/src/lod_cache.zig modules/world-lod/src/root.zig modules/world-lod/src/lod_manager.zig modules/world-lod/src/world_lod.zig modules/world-runtime/src/world.zig
• nix develop --command zig build test
• nix develop --command zig build -Dskip-present
• nix develop --command zig build run -Dskip-present -Dauto-world=normal -Dstartup-diagnostic-seconds=5 (timed out after 60s during startup/run without crash output)

Fixes #691

[github-actions]

📋 Summary

Linked Issue: Fixes #691 (verified in PR description)

This PR implements persistent LOD source data caching with CRC validation. It adds lod_cache.zig for binary serialization, integrates cache load/save into LODManager worker jobs, and adds generator version/identity metadata for cache invalidation. The implementation is well-scoped and the unit tests cover round-trip serialization and checksum rejection.

📌 Review Metadata

• Reviewed Commit SHA: 261709011a1dd16e0d07414f5538993680253c4c
• Reviewed PR: feat: persist LOD source cache #697

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

[HIGH] modules/world-lod/src/lod_manager.zig:801,830 - Data race on cache_dir_path
Confidence: High
Description: loadCachedSourceData and saveCachedSourceData read self.cache_dir_path without holding self.mutex, while enableCache writes it under the mutex. On 64-bit targets, the 16-byte optional slice read/write is not guaranteed atomic, creating a potential for torn reads or use-after-free if enableCache is called while workers are active.
Impact: Undefined behavior; potential crash or reading freed memory.
Suggested Fix: Read cache_dir_path under a brief lockShared()/unlockShared() in the worker functions, or store it as an atomic pointer + length pair. Alternatively, copy the pointer to a local before releasing the mutex in processLODJob.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] modules/world-lod/src/lod_cache.zig:42-44 - Fragile hardcoded serialization sizes
Confidence: High
Description: payloadSize uses magic constants (3 + 13 + 6 + 18) that must exactly match the manual serialization layout of LODMaterialLayers, LODWaterState, LODLightingHint, and LODVegetationHint. If any of those struct layouts change, the cache format breaks silently.
Impact: Cache corruption or deserialization failures after unrelated struct changes.
Suggested Fix: Replace magic numbers with named constants computed from the struct fields (e.g., @sizeOf(LODMaterialLayers) if packed, or explicit comptime size assertions), or add a comptime assert that payloadSize(1) == @sizeOf(f32) + @sizeOf(BiomeId) + @sizeOf(BlockType) + @sizeOf(u32) + @sizeOf(LODMaterialLayers) + ... so mismatches fail at compile time.

[MEDIUM] modules/world-lod/src/lod_manager.zig - Missing integration tests for worker cache paths
Confidence: High
Description: There are no tests verifying loadCachedSourceData, saveCachedSourceData, or enableCache behavior, including the corrupt-cache deletion path and the cache miss -> generate -> cache hit workflow.
Impact: Regressions in cache invalidation, file I/O error handling, or worker integration may go unnoticed.
Suggested Fix: Add at least one integration test that creates a temporary directory, enables the cache, generates data, saves it, reloads it via the manager, and verifies corrupt files are deleted.

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/world-lod/src/lod_manager.zig:950 - Dead errdefer in void-returning function
Confidence: High
Description: processLODJob returns void, so the errdefer data.deinit() can never trigger. It is misleading and suggests error paths that don't exist.
Impact: Code clarity; future maintainers may assume error handling exists.
Suggested Fix: Remove the errdefer or add a comment explaining why cleanup is unnecessary in this scope.

[LOW] modules/world-lod/src/lod_manager.zig:844 - Non-atomic cache file writes
Confidence: Medium
Description: saveCachedSourceData writes directly to the target file. A crash during writeAll leaves a truncated file that fails CRC on next read.
Impact: Wasted work on next load; cache entry must be regenerated.
Suggested Fix: Write to a temporary file and rename it into place for crash-safe updates.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	lod_cache.zig handles serialization; lod_manager handles orchestration.
Open/Closed	5	Adding a field to LODSimplifiedData requires manual changes to serialize, deserialize, and payloadSize.
Liskov Substitution	8	LODGenerator interface is stable; metadata fields extend without breaking vtable.
Interface Segregation	9	LODGenerator exposes only what's needed; cache is additive.
Dependency Inversion	7	Cache depends on concrete world-core types rather than an abstraction.
Average	7.4

🎯 Final Assessment

Overall Confidence Score: 75%

How to interpret: 61-80%: Minor concerns, mostly ready with some fixes.

Confidence Breakdown:

• Code Quality: 75% (Good structure, but fragile constants and data race)
• Completeness: 80% (Feature is complete per PR description, but lacks integration tests)
• Risk Level: 70% (Touches worker threads and file I/O; data race is the main risk)
• Test Coverage: 60% (Unit tests for serialization only; no worker or file I/O tests)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (unit tests present, integration tests missing)

Verdict:

MERGE WITH FIXES

The data race on cache_dir_path should be fixed before merge. The fragile serialization constants should be hardened with compile-time assertions.

Machine Readable Verdict

{
  "reviewed_sha": "261709011a1dd16e0d07414f5538993680253c4c",
  "critical_issues": 0,
  "high_priority_issues": 1,
  "medium_priority_issues": 2,
  "overall_confidence_score": 75,
  "recommendation": "MERGE_WITH_FIXES"
}

opencode session  |  github run

[github-actions]

📋 Summary

Linked Issue: Fixes #691 (verified in PR description)

This PR implements persistent LOD source data caching with CRC validation, generator versioning for cache invalidation, and atomic file writes. The previous review identified five issues; four have been resolved. The implementation adds lod_cache.zig for binary serialization, integrates cache load/save into LODManager worker jobs, and extends all generators with version metadata.

📌 Review Metadata

• Reviewed Commit SHA: 1e64f178f99a121da633ad5c4749f87015c762ab
• Reviewed PR: feat: persist LOD source cache #697

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

None identified.

⚠️ High Priority Issues (Should Fix)

✅ [FIXED] Previous issue: Data race on cache_dir_path — Now resolved via cacheDirPathSnapshot() with shared locking and string duplication.

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] modules/world-lod/src/lod_manager.zig - Missing integration tests for worker cache paths
Confidence: High
Description: No tests verify loadCachedSourceData, saveCachedSourceData, enableCache, or the corrupt-cache deletion path. The only tests are in lod_cache.zig for serialization round-trips.
Impact: Regressions in cache invalidation, file I/O error handling, or worker integration may go unnoticed.
Suggested Fix: Add at least one integration test that creates a temporary directory, enables the cache, generates data, saves it, reloads it via the manager, and verifies corrupt files are deleted.

✅ [FIXED] Previous issue: Fragile hardcoded serialization sizes — Now resolved with named wire-size constants and comptime assertions in lod_cache.zig.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: Dead errdefer in void-returning function — No longer present in the updated processLODJob.

✅ [FIXED] Previous issue: Non-atomic cache file writes — Now resolved with temp-file write + atomic rename in saveCachedSourceData.

[LOW] modules/world-lod/src/lod_cache.zig:147 - CRC computed only over payload
Confidence: Medium
Description: The CRC checksum covers only buf[HEADER_SIZE..], not the full buffer including the header. A header corruption (e.g., magic byte or version field) would not be detected by the checksum.
Impact: Corrupted headers could pass validation if the payload happens to match, leading to misinterpretation of cache metadata.
Suggested Fix: Compute CRC over the entire buffer (buf[0..off]) and write it after the full buffer is assembled, or add a separate header checksum.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	lod_cache.zig handles serialization; lod_manager handles orchestration.
Open/Closed	6	Adding a field to LODSimplifiedData still requires manual wire-format updates, but comptime assertions catch mismatches.
Liskov Substitution	8	LODGenerator interface is stable; metadata fields extend without breaking vtable.
Interface Segregation	9	LODGenerator exposes only what's needed; cache is additive.
Dependency Inversion	7	Cache depends on concrete world-core types rather than an abstraction.
Average	7.6

🎯 Final Assessment

Overall Confidence Score: 82%

How to interpret: 81-100%: High confidence, ready to merge or with trivial fixes.

Confidence Breakdown:

• Code Quality: 85% (Good structure, previous data race and atomicity issues resolved)
• Completeness: 85% (Feature is complete per PR description, addresses Phase 5: persist generated LOD source data #691)
• Risk Level: 80% (Touches worker threads and file I/O, but synchronization is now correct)
• Test Coverage: 70% (Unit tests for serialization only; no worker/file I/O integration tests)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (unit tests present, integration tests still missing)

Verdict:

MERGE WITH FIXES

The data race and atomic write issues from the previous review have been properly fixed. The remaining medium-priority concern is missing integration tests for the worker cache paths, which should be added in a follow-up. The CRC scope suggestion is low priority.

Machine Readable Verdict

{
  "reviewed_sha": "1e64f178f99a121da633ad5c4749f87015c762ab",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 82,
  "recommendation": "MERGE_WITH_FIXES"
}

opencode session  |  github run

[github-actions]

📋 Summary

Linked Issue: Fixes #691 (verified in PR description)

This PR implements persistent LOD source data caching with CRC validation and generator versioning for cache invalidation. It adds lod_cache.zig for binary serialization, integrates cache load/save into LODManager worker jobs, extends LODGenerator with seed/identity/version metadata, and adds integration tests for the cache lifecycle.

All issues identified in previous automated reviews have been resolved. The implementation is complete and tests pass.

📌 Review Metadata

• Reviewed Commit SHA: 265133cd09e72b8ec59ac752468dbb46d797abf9
• Reviewed PR: feat: persist LOD source cache #697

🔴 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: Missing integration tests for worker cache paths — Now resolved with LODManager cache helpers save and reload source data and LODManager cache helpers delete corrupt cache files tests.

✅ [FIXED] Previous issue: Fragile hardcoded serialization sizes — Now resolved with named wire-size constants and comptime assertions in lod_cache.zig.

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: Dead errdefer in void-returning function — No longer present.

✅ [FIXED] Previous issue: Non-atomic cache file writes — Now resolved with temp-file write + atomic rename in saveCachedSourceData.

✅ [FIXED] Previous issue: CRC computed only over payload — Now resolved; computeCrc covers bytes [0..6] and [10..], which includes the header magic, version, LOD byte, and all key fields. The LOD cache checksum covers key header fields test verifies this.

None identified.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	lod_cache.zig handles serialization; lod_manager handles orchestration.
Open/Closed	6	Adding a field to LODSimplifiedData still requires manual wire-format updates, but comptime assertions catch mismatches.
Liskov Substitution	8	LODGenerator interface is stable; metadata fields extend without breaking vtable.
Interface Segregation	9	LODGenerator exposes only what's needed; cache is additive.
Dependency Inversion	7	Cache depends on concrete world-core types rather than an abstraction.
Average	7.6

🎯 Final Assessment

Overall Confidence Score: 85%

How to interpret: 81-100%: High confidence, ready to merge or with trivial fixes.

Confidence Breakdown:

• Code Quality: 85% (Good structure, all previous data race and atomicity issues resolved)
• Completeness: 90% (Feature is complete per PR description, fully addresses Phase 5: persist generated LOD source data #691)
• Risk Level: 80% (Touches worker threads and file I/O, but synchronization is now correct)
• Test Coverage: 80% (Unit tests for serialization + integration tests for manager cache helpers)

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 issues have been addressed. The implementation is sound, well-tested, and ready to merge.

{
  "reviewed_sha": "265133cd09e72b8ec59ac752468dbb46d797abf9",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 85,
  "recommendation": "MERGE"
}

opencode session  |  github run