feat: harden LOD streaming and persistence

[MichaelFisher1997]

Summary

• preserve edited/chunk-derived provenance for span-less far LOD cache payloads and safely regenerate provenance-incomplete legacy data
• cancel stale queued and in-flight generation on pause and large traversal changes without allowing old workers or cache completions to publish over newer work
• enforce an aggregate persistent-store cap with oldest-container eviction, pressure-triggered live-entry compaction, oversized-container rejection, and corruption-safe atomic replacement
• document coherent preset budgets, regression SLO links, requirements, and fallback behavior

Tests

• nix develop --command zig build test
• nix develop --command zig build -Doptimize=ReleaseFast
• pre-push formatting and full-test hooks

Refs #922

[github-actions]

📋 Summary

PR #930 references #922 (an umbrella roadmap for LOD production hardening) with Refs #922, so it does not claim to fully close the issue. The changes address several of #922's workstreams: provenance preservation across cache transitions, pause/teleport cancellation, bounded persistent store with compaction/eviction, and documented quality presets/fallbacks.

The main build (zig build -Doptimize=ReleaseFast) and zig build test both exit successfully, but the newly added world-lod tests are not actually executed by the default test step because the module is not built in test mode. This is the biggest concern for merge readiness.

📌 Review Metadata

• Reviewed Commit SHA: 73bec21e465d325f6f77a700bb82dec1d516b2af
• Reviewed PR: feat: harden LOD streaming and persistence #930
• Linked Issue: LOD GPU roadmap Phase 5: quality, scale, and production hardening #922 (referenced, not claimed as fixed)
• Implementation Coverage: Partially satisfies LOD GPU roadmap Phase 5: quality, scale, and production hardening #922's streaming/persistence and production-gates workstreams; GPU-driven far representation and visual regression coverage remain out of scope.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

[HIGH] src/tests.zig:73-77 / build.zig:651-669 / modules/world-lod/src/root.zig:10-15 - New world-lod tests are not executed by zig build test
Confidence: High
Description: src/tests.zig imports world-lod test modules, but those tests are gated behind builtin.is_test in root.zig and the module is not compiled in test mode when imported by the main test binary. The main test run I inspected contains 282 tests and none of the new/updated LOD cache, LOD store, or LODManager tests.
Impact: The PR's test coverage is effectively unverified in CI; regressions in the new cache format, cancellation logic, or store compaction could slip through.
Suggested Fix: Add a dedicated world-lod test artifact to build.zig (e.g., b.addTest(.{ .root_module = modules.world_lod })) and make test depend on it, or ensure addImport compiles the module in test mode.

💡 Medium Priority Issues (Nice to Fix)

None identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/world-lod/src/lod_cache_io.zig:211-213 - StoreSizeLimit can cause repeated failed write retries
Confidence: Medium
Description: When writePayload returns StoreSizeLimit, the cache IO treats it as a generic failure, leaving the region store_dirty. If the payload itself cannot fit the cap, the next update will retry and fail again, producing recurring warnings.
Impact: Log spam and wasted CPU on every update for an oversized region.
Suggested Fix: Track a per-region "store size limit reached" flag and skip re-queueing the write until the cap or payload changes, or add a backoff.

[LOW] modules/world-lod/src/lod_cache.zig:427-431 - Invalid provenance returns InvalidSpanCount
Confidence: High
Description: The switch on a provenance byte returns CacheError.InvalidSpanCount for values outside 0-2.
Impact: Misleading error name makes debugging harder.
Suggested Fix: Add a CacheError.InvalidProvenance variant and return it here.

[LOW] modules/world-lod/src/lod_manager_core_ops.zig:401-404 - Teleport cancellation is axis-aligned
Confidence: Medium
Description: A teleport of ~32 chunks diagonally (e.g., 24,24) would not trigger cancellation because each component is below the threshold.
Impact: Slightly stale work may continue after a fast diagonal movement.
Suggested Fix: Use max(moved_x, moved_z) or Euclidean distance, or document that the threshold is Chebyshev-style.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Concerns split cleanly across cache, manager, scheduler, and store files.
Open/Closed	7	Mostly extends existing behavior; cache version bump is a necessary breaking change.
Liskov Substitution	8	Generator interface and stop-flag contract remain unchanged.
Interface Segregation	8	Manager/scheduler/store interfaces are narrow and well-scoped.
Dependency Inversion	8	Manager depends on abstract LODGenerator and RHI interfaces.
Average	7.8

🎯 Final Assessment

Overall Confidence Score: 75%

Confidence Breakdown:

• Code Quality: 80%
• Completeness: 70% (tests added but not executed; some edge cases not covered)
• Risk Level: 60% (cancellation + persistence changes are nontrivial)
• Test Coverage: 50% (new tests exist but are not run by default CI)

Merge Readiness:

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

Verdict:

MERGE WITH FIXES

The implementation is coherent and compiles, but the test-execution gap must be fixed before this can be considered fully verified.

{
  "reviewed_sha": "73bec21e465d325f6f77a700bb82dec1d516b2af",
  "critical_issues": 0,
  "high_priority_issues": 1,
  "medium_priority_issues": 0,
  "overall_confidence_score": 75,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[github-actions]

📋 Summary

PR #930 references #922 (LOD production hardening umbrella) with Refs #922, so it does not claim to fully close the issue. The implementation covers provenance preservation, pause/teleport cancellation, persistent store cap, and documentation.

The previous review's HIGH issue (world-lod tests not executed by zig build test) is fixed by the follow-up commit test: run world LOD coverage in CI, which adds modules/world-lod/src/tests.zig and wires it into build.zig. Test count rose from 282 to 328, indicating the new LOD tests are now running.

Build and compilation succeed; 328 tests pass. The main test binary failed to execute in this environment due to a glibc ABI mismatch (GLIBC_ABI_GNU2_TLS / GLIBC_ABI_DT_X86_64_PLT not found), which is an environment/CI issue, not a code issue.

📌 Review Metadata

• Reviewed Commit SHA: 5dda11739957764dad070bda9efc35712e699ced
• Reviewed PR: feat: harden LOD streaming and persistence #930
• Linked Issue: LOD GPU roadmap Phase 5: quality, scale, and production hardening #922 (referenced, not claimed as fixed)
• Implementation Coverage: Partially satisfies LOD GPU roadmap Phase 5: quality, scale, and production hardening #922's streaming/persistence and production-gates workstreams.

🔴 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)

None identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/world-lod/src/lod_manager_generation_ops.zig:571-593 - Cancelled meshing jobs can overwrite shared CPU mesh data before Phase 3 token check
Confidence: Medium
Description: processLODJob checks chunk.cancellationRequested() only for .chunk_generation work. A .chunk_meshing job that is cancelled by pause or cancelWorkOutsideHorizon after passing the token check will still call buildMeshForChunk, which builds into the shared LODMesh object returned by getOrCreateMesh. The Phase 3 token check prevents state promotion, but the CPU mesh object is temporarily overwritten with stale data.
Impact: Wasted CPU work and a transient stale CPU mesh copy until the region is re-meshed. No visible rendering corruption because the state is not promoted to .mesh_ready.
Suggested Fix: Check chunk.cancellationRequested() after buildMeshForChunk (or inside it) and return early without leaving the stale mesh as the latest copy.

[LOW] modules/world-persistence/src/region_file.zig:128 - FileTooShort error name is overloaded
Confidence: High
Description: RegionFile.writeChunk returns RegionError.FileTooShort when a chunk's compressed size requires more than 255 sectors. This is a chunk-too-large / sector-address-exhausted condition, not a truncated file.
Impact: Misleading error name makes debugging harder.
Suggested Fix: Add a dedicated RegionError.ChunkTooLarge or SectorCountExceeded variant and return it from the sectors_needed > maxInt(u8) branch.

✅ Acknowledged Fixes

• [FIXED] build.zig:668-695 / modules/world-lod/src/tests.zig:1-8 - world-lod tests now run as part of the default test step.
• [FIXED] modules/world-lod/src/lod_cache.zig:432 - Invalid provenance now returns CacheError.InvalidProvenance instead of InvalidSpanCount.
• [FIXED] modules/world-lod/src/lod_manager_cache_ops.zig:152-153 / modules/world-lod/src/lod_chunk.zig:234-235 - StoreSizeLimit now suppresses retries via store_size_limited / store_size_limit_cap_mb until the cap increases.
• [FIXED] modules/world-lod/src/lod_manager_core_ops.zig:274-277 - Teleport cancellation now uses Euclidean distance instead of per-axis thresholds.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Concerns split across cache, manager, scheduler, store, and IO.
Open/Closed	7	Mostly extends behavior; cache version bump is a necessary breaking change.
Liskov Substitution	8	Generator and stop-flag contracts remain unchanged.
Interface Segregation	8	Manager/scheduler/store interfaces are narrow and well-scoped.
Dependency Inversion	8	Manager depends on abstract LODGenerator and RHI interfaces.
Average	7.8

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

• Code Quality: 85%
• Completeness: 90%
• Risk Level: 70%
• Test Coverage: 85%

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (328 tests passed; one test binary could not run due to environment glibc mismatch)

Verdict:

MERGE

The previous high-priority test-execution gap is resolved, the remaining notes are minor, and the implementation is coherent and well-tested.

{
  "reviewed_sha": "5dda11739957764dad070bda9efc35712e699ced",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

opencode session  |  github run

[MichaelFisher1997]

Addressed the review findings in 5dda117:

• added a dedicated world-lod test root/artifact; zig build test --summary all now runs 77 LOD tests and reports 405/405 total tests passing
• added size-limit write backoff until source data or the configured cap changes
• added InvalidProvenance and coverage for malformed provenance bytes
• switched teleport cancellation to Euclidean distance
• repaired stale LOD test fixtures that the newly enabled artifact exposed
• added a safe region-file sector-count bound for oversized payloads

Validation: full tests, ReleaseFast build, and pre-push checks all pass.