[R2-3][P1] Resolve residual RHI native-handle leak (lpv_backend, ComputePipeline, imgui)

[MichaelFisher1997]

Part of #839 — [Audit][Round 2] A++ umbrella.
Phase: P1 · Finding: R2-3 · Predecessors: #783 (closed), #784 (closed)

Problem

Round 1 closed the worst RHI native-handle leaks (#783 added the compute facet, #784 eliminated most of INativeHandlesContext). Three residual leaks survive that prevent true LSP — any non-Vulkan backend would still have to fake Vulkan's memory layout.

Evidence (all verified at HEAD)

Leak 1 — lpv_backend.zig:13 downcasts *anyopaque → *VulkanContext

pub fn fromVulkanRHI(rhi: rhi_pkg.RHI) LPVBackend {
    return .{ .vk_ctx = @ptrFromInt(rhi.nativeHandles().getBackendContext()) };
}

A high-level lighting module directly names VulkanContext (a backend-internal type) and downcasts the abstract context pointer. Any non-Vulkan backend would have to masquerade as VulkanContext.

Leak 2 — ComputeBuffer / ComputePipeline carry raw Vulkan-shaped u64 fields

modules/engine-rhi/src/rhi.zig:666-678:

pub const ComputeBuffer = extern struct {
    buffer: u64,    // obviously VkBuffer
    memory: u64,    // obviously VkDeviceMemory
    size: u64,
    ...
};

pub const ComputePipeline = extern struct {
    pipeline: u64,              // VkPipeline
    layout: u64,                // VkPipelineLayout
    descriptor_pool: u64,       // VkDescriptorPool
    descriptor_set_layout: u64, // VkDescriptorSetLayout
    descriptor_sets: [MAX_FRAMES_IN_FLIGHT]u64,
    ...
};

Then rhi_vulkan.zig:1007-1201 immediately casts them back: c.vkCmdBindPipeline(cmd, ..., @ptrFromInt(pipeline)), c.vkCmdBindDescriptorSets(cmd, ..., @ptrFromInt(pipeline_layout), ...), etc.

Leak 3 — imgui_backend.zig:31-45 consumes 6 native handles in a high-level UI module

const native = rhi.nativeHandles();
const init_info = c.ImGui_ImplVulkan_InitInfo{
    .instance = @ptrFromInt(native.getInstance()),
    .physical_device = @ptrFromInt(native.getPhysicalDevice()),
    .device = @ptrFromInt(native.getDevice()),
    .queue = @ptrFromInt(native.getQueue()),
    .descriptor_pool = @ptrFromInt(native.getDescriptorPool()),
    .render_pass = @ptrFromInt(native.getUiRenderPass()),
    ...
};

engine-ui (high level) directly constructs 6 distinct Vulkan handle types from INativeHandlesContext. ImGui's Vulkan backend inherently needs these — but the consumption should live behind an IImGuiBackend shim in engine-graphics, not in engine-ui.

Bonus leak — RhiError.VulkanError names a specific backend in the abstract error set

modules/engine-rhi/src/rhi_types.zig:7: VulkanError sits beside abstract variants (OutOfMemory, GpuLost, SurfaceLost). Callers can't pattern-match meaningfully when the abstract type names a concrete backend.

Fix

Decision required first (call it out in the PR description):

Either (a) commit to Vulkan-only-forever and stop pretending the abstraction is portable, OR (b) close the leaks. Both are valid. The current state — Vulkan-only-forever plus a fake abstraction — is the worst of both.

Assuming (b):

1. Promote LPV behind the RHI. Add ILightingContext (or extend IEffectsContext) with drawLPVGrid(params), uploadLPVData(data). Rewrite lpv_backend.zig against the abstract facet. Delete the @ptrFromInt(rhi.nativeHandles().getBackendContext()) downcast.

2. Make ComputeBuffer / ComputePipeline opaque. Change public fields to handle: u32 (matching BufferHandle/TextureHandle/ShaderHandle discipline). Move the VkBuffer/VkDeviceMemory/VkPipeline/etc. fields into a backend-internal struct (vulkan/compute_resources.zig). Update rhi_vulkan.zig:1007-1201 to look them up via compute_resources.get(handle) instead of @ptrFromInt(pipeline).

3. Move ImGui Vulkan consumption into engine-graphics. Create modules/engine-graphics/src/vulkan/imgui_vulkan_bridge.zig that consumes INativeHandlesContext and constructs ImGui_ImplVulkan_InitInfo. engine-ui/imgui_backend.zig calls rhi.initImGuiBackend(impl_data) (a new IImGuiContext method). Delete the 6 native-handle reads from engine-ui.

4. Rename RhiError.VulkanError → RhiError.BackendError (or remove and replace with backend-agnostic DeviceError). Update the ~10 callers in engine-graphics.

5. (Optional, related) Migrate the anyerror! paths at rhi.zig:369, 596, 615, 689, 1096, 1109, 1147, 1171 to typed RhiError! so callers can pattern-match.

Verification

• nix develop --command zig build test (includes shader validation)
• Required: headless screenshot baseline at low/medium/high presets before and after — LPV lighting, ImGui rendering, and compute meshing must all be visually identical
• nix develop --command zig build test -- --test-filter "RHI" (will exercise R2-4 if landed first)
• grep -rE "@ptrFromInt\(.*nativeHandles|@ptrCast.*VulkanContext" modules/ should return zero matches outside modules/engine-graphics/src/vulkan/

Constraints

• Behavior-preserving end-to-end. The Vulkan path must stay visually and functionally identical.
• One PR per leak (4 PRs total) — each independently revertable.
• Order recommendation: land R2-4 (RHI contract tests) first so this refactor is guarded.
• Conventional commits: refactor(engine-rhi): make ComputePipeline opaque, refactor(engine-graphics): move ImGui Vulkan bridge, etc.
• Keep worker-thread RHI isolation intact.

Notes

• Predecessors: [SOLID] [C3] Add compute facet to RHI.VTable; rewrite gpu_mesher + lpv_backend against it #783 (compute facet) and [SOLID] [C2] Eliminate INativeHandlesContext; move sky/water/shadow draws behind the RHI #784 (eliminate INativeHandlesContext for sky/water/shadow) — both closed. This issue finishes the job.
• If the team chooses option (a) — commit to Vulkan-only — then this issue becomes "document the decision in engine-rhi/src/root.zig //! doc, delete INativeHandlesContext's abstraction veneer, rename to IVulkanNativeHandles, and close R2-3 as wontfix". That's a legitimate resolution.

Tracking: #839

[github-actions]

Thanks for the R2-11 write-up — validated against the working tree. A few notes before implementation lands:

Validation results

• AGENTS.md rule confirmed incorrect at AGENTS.md:110 (and mirrored at CONTRIBUTING.md:223). Both files say snake_case for functions. Zig idiom is camelCase, and this codebase follows it ~99% of the time. The audit's "rule contradicts the code" finding stands.
• pub var log at modules/engine-core/src/log.zig:175 is intentionally non-SCREAMING for ergonomics (log.log.info(...)) — agreed, should NOT be changed.
• Snake_case pub fn count is 13 across exactly 5 files, not 4. The audit undercounted the RHI interface file, which must be renamed together with its vulkan impl or the interface contract breaks:

File	snake_case fns	Notes
modules/engine-rhi/src/culling.zig	3	MISSING FROM AUDIT — interface decls (update_aabb_data, read_visible_count, read_visible_indices) must rename together with the vulkan impl
modules/engine-graphics/src/vulkan/culling_system.zig	3	implementor of the interface above
modules/world-lod/src/lod_geometry.zig	7	including is_lod_water_cell_for_lod and cell_color_for_lod
modules/world-lod/src/lod_renderer.zig	2	(the issue claims these — confirmed)
modules/game-core/src/benchmark.zig	1	(audit claims — confirmed)

The 5th file (engine-rhi/src/culling.zig) is the most important correction: renaming update_aabb_data etc. in culling_system.zig without also renaming the interface declaration in engine-rhi/src/culling.zig will produce a type-system error (different method symbols on ICullingSystem vs its impl). All 13 sites must ship in the same commit.

Classification

This is Doc-fix (P4) + Refactor (P4) — not a Bug in the strict sense (no runtime misbehavior), but the AGENTS.md rule effectively lies to new contributors, which IS a maintainability bug. Tracking under #839 is correct.

Scope / sub-issue suggestion

Recommend a single PR, not 4. Reasons:

1. The 13 renames are tightly coupled (one symbol is declared in the interface and implemented elsewhere — they MUST land together).
2. The AGENTS.md + CONTRIBUTING.md doc edit is a one-liner per file and trivially reviewable.
3. 4 separate PRs adds CI overhead and review burden for ~14 file edits total — not worth it for P4 work.

Splitting is not warranted here.

Suggested renames (verify before applying)

engine-rhi/src/culling.zig                   update_aabb_data           → updateAABBData
engine-rhi/src/culling.zig                   read_visible_count         → readVisibleCount
engine-rhi/src/culling.zig                   read_visible_indices       → readVisibleIndices
engine-graphics/src/vulkan/culling_system.zig  update_aabb_data         → updateAABBData
engine-graphics/src/vulkan/culling_system.zig  read_visible_count       → readVisibleCount
engine-graphics/src/vulkan/culling_system.zig  read_visible_indices     → readVisibleIndices
world-lod/src/lod_geometry.zig               is_lod_water_cell          → isLODWaterCell
world-lod/src/lod_geometry.zig               is_fine_sample_lod         → isFineSampleLOD
world-lod/src/lod_geometry.zig               cell_index                 → cellIndex
world-lod/src/lod_geometry.zig               cell_color_for_lod         → cellColorForLOD
world-lod/src/lod_geometry.zig               ambient_occlusion_for_lod  → ambientOcclusionForLOD
world-lod/src/lod_geometry.zig               is_lod_water_cell_for_lod  → isLODWaterCellForLOD
world-lod/src/lod_geometry.zig               water_coverage_stats       → waterCoverageStats
game-core/src/benchmark.zig                  (TBD — single fn, verify name in source)

(plus the 2 fns in world-lod/src/lod_renderer.zig — please re-grep that file since the audit didn't list names; my count of 2 matches but the names weren't enumerated.)

Doc edit suggestion

In both AGENTS.md:110 and CONTRIBUTING.md:223, replace:

Functions/Variables: snake_case (init_renderer, mesh_queue, chunk_x)

with:

Functions: camelCase following Zig stdlib convention (initRenderer, meshQueue, chunkX)
Variables: snake_case (chunk_x, mesh_queue, frame_index)

This matches actual codebase usage (99%+ camelCase fns) and the Zig stdlib (std.ArrayList.append, std.mem.Allocator, etc.).

Verification commands the implementer should run

nix develop --command zig build test
nix develop --command zig fmt src/ modules/
grep -rEn 'pub fn [a-z][a-z0-9_]*_[a-z]' modules/ src/ --include='*.zig'
grep -rEn 'fn [a-z][a-z0-9_]*_[a-z]' modules/ src/ --include='*.zig' | grep -vE '(_test\b|deinit|main\b)'

The last two should return zero results after the rename. (Note: deinit and main are conventionally snake_case across the Zig ecosystem and should not be renamed.)

No blockers

Issue is well-scoped, evidence is solid, umbrella #839 tracking is correct, no duplicates or in-flight PRs found. Ready to implement behind #839.

[github-actions]

Validation: This is an automated-audit finding — report is accurate and complete. I re-verified every cited location against HEAD (commit 9c6e53d); all four leaks reproduce exactly as written.

Confirmations at HEAD

• Leak 1 — modules/engine-graphics/src/lpv_backend.zig:13 downcasts @ptrFromInt(rhi.nativeHandles().getBackendContext()) into *VulkanContext. Confirmed.
• Leak 2 — modules/engine-rhi/src/rhi.zig:666–678 ComputeBuffer/ComputePipeline carry raw Vulkan-shaped u64 fields; rhi_vulkan.zig:1007–1201 immediately reverses them via @ptrFromInt(...) (I counted 14 such casts across bindComputePipeline, bindDescriptorSet, createComputeBuffer, updateComputeDescriptors, destroyComputePipeline, pushConstants, fillBuffer, copyBuffer, bufferBarrier).
• Leak 3 — modules/engine-ui/src/imgui/imgui_backend.zig:31–45 reads 6 native handles (instance, physical_device, device, queue, descriptor_pool, render_pass). Confirmed.
• Bonus — modules/engine-rhi/src/rhi_types.zig:7 RhiError.VulkanError. Callers: 10 hits across vulkan_swapchain.zig, swapchain_presenter.zig, resource_manager.zig, transfer_queue.zig, rhi_vulkan.zig.

One additional leak found (not in the report)

modules/engine-ui/src/ui_system_manager.zig:88 calls rhi_ptr.nativeHandles().getCommandBuffer() from a high-level UI module. This is a 7th native-handle consumption that didn't make the headline list — same shape as Leak 3 and must be folded into PR #3 (ImGui bridge move) so the UI subsystem ends with zero nativeHandles() reads. Current full inventory of nativeHandles() consumers outside engine-graphics/src/vulkan/:

modules/engine-graphics/src/lpv_backend.zig:13
modules/engine-ui/src/imgui/imgui_backend.zig:31
modules/engine-ui/src/ui_system_manager.zig:88

That should drive the success criterion: after this work, the grep from "Verification" should also return zero matches for any *.nativeHandles().* call in engine-ui/ and engine-graphics/.

Decision question

The (a) vs (b) decision in the issue body is genuinely the gating step — agree it must be resolved before PR #1 opens. Option (a) (commit to Vulkan-only) is defensible given current scope; option (b) is what the issue is sized for. Suggest a separate issue (or a brief PR description for PR #1) that records the decision so the rest of #839 can proceed in a single direction.

Recommended PR split — endorsed

4 independent PRs is the right call. Suggested ordering, gated on R2-4 landing first:

1. refactor(engine-rhi): rename RhiError.VulkanError → BackendError — purely mechanical, ~10 callers in engine-graphics/, trivial to revert, exercises the contract tests immediately. Good PR Roadmapv3 #1.
2. refactor(engine-rhi): make ComputeBuffer/ComputePipeline opaque — biggest blast radius (compute meshes, LPV propagate, transfer queue). Backend-internal lookup table (compute_resources.get(handle)) keeps the API surface as u32-handles to match BufferHandle/TextureHandle/ShaderHandle discipline. Behavior-preserving.
3. refactor(engine-graphics): move LPV behind ILightingContext / IEffectsContext — kills lpv_backend.zig entirely; LPV becomes a pure-effect that calls into the new facet.
4. refactor(engine-graphics): move ImGui Vulkan bridge + UI command-buffer consumption into engine-graphics — vulkan/imgui_vulkan_bridge.zig consumes INativeHandlesContext, exposes IImGuiContext.initImGuiBackend(impl_data). Also eliminate the unlisted leak at ui_system_manager.zig:88 (UI should call an endFrame(cmd) shim, not fetch getCommandBuffer() directly).

(Optional 5th: anyerror! → RhiError! migration on the 8 listed rhi.zig lines. Nice-to-have, not blocking.)

Prerequisites confirmed

• R2-4 ([R2-4][P1] Add engine-rhi unit-test contract suite (263 pub fns, 0 tests) #844) exists and is open — agreed it should land first so the opaque-handle and bridge-move refactors are guarded by contract tests.
• No duplicate issues. [Audit][Round 2][Umbrella] A++ Architecture & Maintainability Roadmap #839 is the umbrella; this is correctly one of its tracked sub-items.
• Verification grep (@ptrFromInt(.*nativeHandles|@ptrCast.*VulkanContext outside engine-graphics/src/vulkan/) currently returns exactly 3 hits (the two cited plus the unlisted ui_system_manager.zig one once widened to nativeHandles().* pattern). Post-fix target: 0.

No missing data needed to begin — the codebase is unambiguous and the path is clear once the (a)/(b) decision is recorded.

[github-actions]

Posted validation comment on issue #842. Confirmed all four cited leaks at HEAD, identified one additional unlisted leak at modules/engine-ui/src/ui_system_manager.zig:88 (same shape as Leak 3), endorsed the 4-PR split with reordering (rename VulkanError first, opaque compute handles second, LPV behind facet third, ImGui bridge + UI command-buffer consumption fourth), and confirmed R2-4 (#844) as the correct prerequisite. Called out that the (a)/(b) Vulkan-only-vs-abstract decision is the actual gating step before PR #1 opens.

opencode session  |  github run