perf: remove LOD streaming device stalls

[MichaelFisher1997]

Summary

• retire replaced and evicted LOD buffers through the existing frame-fence-backed RHI deletion queues instead of calling vkDeviceWaitIdle
• preserve a grace period before pooled ranges are returned while keeping streaming eviction bounded
• enforce the configured upload byte budget from the first upload, deferring oversized meshes rather than overrunning staging capacity
• add regression coverage for oversized first uploads and no-stall pool growth

Validation

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

Addresses #919.

[github-actions]

📋 Summary

Linked issue: #919 ("LOD GPU roadmap Phase 2: asynchronous transfers, lifetime, and memory")

PR #926 partially addresses #919. It successfully removes vkDeviceWaitIdle from LOD mesh/pool lifetime, replaces it with the existing RHI frame-fence deletion queue, enforces the upload byte budget from the first upload, and adds frame-serial-aware visibility sharing between terrain and water. However, several #919 workstreams (persistent mapped/ring staging strategy, resident-region/total LOD memory limits, comprehensive memory accounting) are not yet implemented, so the issue remains open.

The PR is large (23 files, +1304/-330) and well-focused on the async cache I/O and no-stall lifetime goals. Tests pass, but a critical memory-safety bug was introduced in the new cache-read completion path.

📌 Review Metadata

• Reviewed Commit SHA: b0220a52c12928c26781ac9868f316c3e1aa18d2
• Reviewed PR: perf: remove LOD streaming device stalls #926
• Linked Issue: LOD GPU roadmap Phase 2: asynchronous transfers, lifetime, and memory #919 — partially addressed, not fully resolved.

🔴 Critical Issues (Must Fix - Blocks Merge)

[CRITICAL] modules/world-lod/src/lod_manager_cache_ops.zig:96 - Cache read completion consumes hit data but leaves union tagged as .hit, causing undefined-value deinit / double-free
Confidence: High
Description: In drainCacheCompletions, when a cache read returns a hit, the code moves the deserialized LODSimplifiedData into the region with region.data = .{ .simplified = data.* }; and then sets data.* = undefined;. It does not change the ReadResult union tag to .miss. The deferred deinitCompletion then sees read.result == .hit and calls data.deinit() on the now-undefined payload, which dereferences an undefined allocator and slices.
Impact: In production, when the region is later evicted or the manager shuts down, LODChunk.deinit() will attempt to free slices that were already freed (or freed from garbage pointers), causing a double-free/corruption crash.
Suggested Fix: After moving the data, set read.result = .miss; so the deferred cleanup skips the payload. Remove the now-redundant data.* = undefined;.

} else {
    region.data = .{ .simplified = data.* };
    read.result = .miss; // consume the payload
    region.updateHeightBoundsFromData();
    region.source_revision +%= 1;
    region.setState(.generated);
    self.cache_hits += 1;
}

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] modules/world-lod/src/lod_renderer.zig:364 - buildVisibilityProjection updates per-level counters but omits aggregate rejected_count and coverage_count
Confidence: High
Description: The old collectVisibleMeshes path calls profile.addRejected() and profile.addCoverage() for every rejection/coverage scan. The new buildVisibilityProjection only populates the new LODVisibilityLevelSnapshot telemetry and calls profile.addVisible(); it never increments the top-level rejected_count and coverage_count counters. Since production now uses renderFrame, the aggregate counters will be underreported, breaking benchmark and UI metrics that rely on them.
Impact: Misleading LOD visibility/rejection telemetry in benchmarks and the timing overlay.
Suggested Fix: In each rejection branch of buildVisibilityProjection, call if (profiling) |profile| profile.addRejected();. After the chunk-coverage scan, call if (profiling) |profile| profile.addCoverage(); once per scan.

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/world-lod/src/lod_manager_cache_ops.zig:58 - shutdownCacheIO may leave dirty stores unflushed beyond 2048 stores
Confidence: Medium
Description: The loop increments attempts by the number of queued stores and stops at 2048. If more than 2048 dirty stores exist (e.g., a long session with heavy edits), the remaining stores are not queued before the final flushCacheIO, so they remain unwritten at shutdown.
Impact: Theoretical data loss at shutdown only under very high dirty-store counts.
Suggested Fix: Make the loop enqueue/drain until queueDirtyStores returns zero, with a max-iteration guard rather than a store-count guard.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	7	Cache I/O is now cleanly separated into a dedicated worker; LODManager still orchestrates many concerns, but the split is deliberate.
Open/Closed	8	renderFrame is optional in LODRenderInterface; existing renderers fall back to render() without changes.
Liskov Substitution	7	Minimal inheritance; concrete renderer implements the interface correctly.
Interface Segregation	8	render_frame_fn is optional, keeping the interface usable for small test implementations.
Dependency Inversion	8	LODManager depends on LODGPUBridge and LODRenderInterface, not concrete RHI.
Average	7.6

🎯 Final Assessment

Overall Confidence Score: 60%

Confidence Breakdown:

• Code Quality: 75% (well-structured, but the critical deinit bug is a significant regression)
• Completeness: 80% (covers the PR goals and main LOD GPU roadmap Phase 2: asynchronous transfers, lifetime, and memory #919 acceptance criteria, but not all roadmap items)
• Risk Level: 65% (memory-safety bug and telemetry regression in new paths)
• Test Coverage: 80% (new regression tests added, but the critical bug is not caught because the test region is leaked)

Merge Readiness:

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

Verdict:

DO NOT MERGE until the cache-read completion lifetime bug is fixed.

{
  "reviewed_sha": "b0220a52c12928c26781ac9868f316c3e1aa18d2",
  "critical_issues": 1,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 60,
  "recommendation": "DO NOT MERGE"
}

opencode session  |  github run

[github-actions]

📋 Summary

PR #926 addresses #919 ("LOD GPU roadmap Phase 2: asynchronous transfers, lifetime, and memory"). The issue remains partially addressed; this PR is a focused follow-up that removes the remaining vkDeviceWaitIdle stalls from LOD mesh/pool lifetime, retires buffers through the RHI frame-fence deletion queue, enforces the upload byte budget from the first upload, and adds regression tests. It also resolves the cache-read lifetime bug and the telemetry gap reported in the previous automated review.

📌 Review Metadata

• Reviewed Commit SHA: 14d5c84b93d16abcd3e28ee3f2cfc5b6a3f7f624
• Reviewed PR: perf: remove LOD streaming device stalls #926
• Linked Issue: LOD GPU roadmap Phase 2: asynchronous transfers, lifetime, and memory #919 — partially addressed, not fully resolved.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

• ✅ [FIXED] Previous issue: drainCacheCompletions in modules/world-lod/src/lod_manager_cache_ops.zig:96 moved cache-hit data into region.data but left read.result tagged as .hit, causing deferred cleanup to deinitialize undefined data. The code now retags read.result = .miss; after moving ownership, so the deferred cleanup skips the consumed payload.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

• ✅ [FIXED] Previous issue: buildVisibilityProjection in modules/world-lod/src/lod_renderer.zig populated per-level telemetry but omitted the aggregate rejected_count and coverage_count counters. Each rejection branch now calls if (profiling) |profile| profile.addRejected(); and the coverage scan calls if (profiling) |profile| profile.addCoverage();.

ℹ️ Low Priority Suggestions (Optional)

• ✅ [FIXED] Previous issue: shutdownCacheIO in modules/world-lod/src/lod_manager_cache_ops.zig:58 used a store-count guard that could stop before all dirty stores were queued. The loop now increments by iteration (attempts += 1) and continues until queueDirtyStores returns zero, bounded by 2048 iterations, allowing many more stores to flush before the cap.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	7	Fixes remain in their dedicated modules (cache ops, upload ops, renderer, pool).
Open/Closed	8	No extension points changed; existing renderer interface stays optional.
Liskov Substitution	7	No new inheritance or substitution concerns introduced.
Interface Segregation	8	RHI bridge and render interfaces remain focused and minimal.
Dependency Inversion	8	LODManager depends on LODGPUBridge and LODRenderInterface, not concrete RHI.
Average	7.6

🎯 Final Assessment

Overall Confidence Score: 92%

Confidence Breakdown:

• Code Quality: 90% (clean, focused fixes with good comments)
• Completeness: 90% (addresses the PR goals and all prior review findings; LOD GPU roadmap Phase 2: asynchronous transfers, lifetime, and memory #919 roadmap is not fully complete)
• Risk Level: 85% (removes idle waits, but relies on RHI deletion queues which is the intended design)
• Test Coverage: 95% (new regression tests for oversized first upload and budget edge cases)

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 — the previous critical memory-safety bug and telemetry gap are fixed, tests and formatting pass, and the remaining changes are low-risk clean-ups.

{
  "reviewed_sha": "14d5c84b93d16abcd3e28ee3f2cfc5b6a3f7f624",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 92,
  "recommendation": "MERGE"
}

opencode session  |  github run