[R2-1][P1] Decompose LODManager God-object (2,715 LOC → ~500)

[MichaelFisher1997]

Part of #839 — [Audit][Round 2] A++ umbrella.
Phase: P1 · Finding: R2-1 · Risk: HIGH

Problem

LODManager is the worst God object in the repo. 2,715 LOC, 2,207 impl LOC, 89 functions, and 10–12 distinct concerns merged into a single struct. Round 1 split lod_mesh.zig (#785) and the leaves (lod_geometry, lod_ingest, lod_scheduler, lod_cache, lod_store, lod_streaming_coordinator) are exemplary — but the orchestrator itself absorbed the residual complexity.

This issue supersedes any informal follow-up to #785 and is the single highest-leverage refactor in Round 2.

Evidence

Struct fields group into ≥10 concerns — modules/world-lod/src/lod_manager.zig:183-289:

#	Concern	Fields	LOC impact
1	Region storage	regions[LODLevel.count]RegionMap	~300
2	Mesh storage	meshes[LODLevel.count]MeshMap	~200
3	Job system + worker pool	gen_queues, lod_gen_pool, next_job_token	~400
4	Upload pipeline	upload_queues	~150
5	Cache/persistence	cache_dir_path, store_mutex, 12+ private fns	~250
6	Chunk-derived ingestion	pending_ingestions, ingestion_mutex, 9 methods	~250
7	Memory governor	memory_used_bytes, radius_shrink_chunks	~100
8	Mesh deletion queue	deletion_queue, deletion_timer	~80
9	Transition bookkeeping	transition_queue	~120
10	Threading — 3 locks + 3 atomics	mutex (RwLock), store_mutex, ingestion_mutex, stop_flag, player_cx/cz	smeared

update() orchestrates 14 subsystems in 110 lines — lod_manager.zig:812-922. Textbook "feature envy over its own subsystems": mesh deletion sweep → NaN safety → player position publish → queue reprioritization → throttling → coverage unload → cache recenter → scheduling (×N levels) → generation dispatch → state transitions → uploads → stats logging → memory budget → distance unload → ingestion drain → edit flush → store flush → transition decay.

Locking is smeared: 87 acquisitions of self.mutex across ~25 functions including updateStats, queueLODRegions, processQueuedGenerations, processStateTransitions, processUploadsWithBudget, unloadDistantForLevel, enforceMemoryBudget, decayTransitionFrames, markRegionRenderable, recordCacheHit, recordCacheMiss, applyIngestionToRegions, cacheEnabled, logLegacyCacheNotice, cacheDirPathSnapshot. The inline comment at line 276 — "never held while acquiring mutex (avoids cross-lock deadlocks)" — is a clear admission that locking discipline has outgrown ad-hoc reasoning.

processLODJob worker callback spans 6 subsystems in ~150 lines — lod_manager.zig:2055-2203: state lookup/validation, stale-job detection, token check, pinning, lock drop, simplified-data allocation, heightmap generation via generator.generateHeightmapOnly, abort-flag polling, mesh building via buildMeshForChunk, state transition.

Duplicated coverage logic — LODManager.areAllChunksLoaded (lod_manager.zig:1741-1766) and LODRenderer.isCoveredByChunks (lod_renderer.zig:520-576) both compute "what chunks does this LOD region touch and are they loaded?" with subtly different policies, and both define the same CHUNK_COVERAGE_PADDING = 1 constant.

Fix

Extract 5 subsystems (priority order), each landing as one focused PR against this issue. After all 5 land, LODManager becomes a thin orchestrator (~400–500 LOC).

Extraction 1 — LODCacheStore

Owns: cache_dir_path, logged_legacy_cache_notice, store_mutex, and all of cacheKey / legacyCacheFilePath / readStorePayload / writeStorePayload / deleteStorePayload / deleteStoreContainer / loadCachedSourceData / saveCachedSourceData / recordCacheHit / recordCacheMiss / enableCache / flushDirtyStores / logLegacyCacheNotice / cacheDirPathSnapshot / cacheEnabled.
Public interface: enable(path), load(key) ?Data, save(key, data), flush(regions_iter), validateHeader() bool.
Removes ~12 private fns + 3 fields + ~250 LOC.

Extraction 2 — LODIngestionQueue

Owns: pending_ingestions, edit_dirty, ingestion_mutex, chunk_resolver, edit_cooldown, ingestion_drain_per_frame.
Public interface: ingestChunk(cx, cz, chunk, prov), markEdited(cx, cz), requestDeferred(cx, cz, prov), drain(resolver, regions), flushEdits(resolver, regions), tick().
Removes 9 methods + 6 fields + ~250 LOC + 1 mutex + the cross-lock ordering rule. Highest-impact single extraction.

Extraction 3 — LODMeshDisposalQueue

Owns: deletion_queue, deletion_timer, queueMeshDeletion, processMeshDeletions. Manager just calls disposal.queue(mesh) and disposal.tick(dt, MAX_SWEEP).
Removes 2 fields + 2 methods. Kills the "manager knows how to wait for GPU idle" smell.

Extraction 4 — LODMemoryGovernor

Owns: memory_used_bytes, radius_shrink_chunks, enforceMemoryBudget, regionMemoryBytes.
Public interface: track(bytes), enforce(regions, meshes, player_pos) Decision, currentPressure() Pressure.
Removes 2 fields + 2 methods + ~100 LOC. Pushes eviction policy out of the orchestrator.

Extraction 5 — LODJobDispatcher

Owns: gen_queues, lod_gen_pool, next_job_token, processLODJob, processQueuedGenerations, processStateTransitions, processUploads, requeueUpload.
Public interface: tick(ctx), pushGeneration, pushMeshing, pushUpload. Manager holds an opaque handle.
Removes ~5 fields + 6 methods + ~400 LOC. Biggest LOC win, most delicate — locking must move with it.

Final — collapse update() to ~30 lines

After extractions 1–5, update() becomes a thin delegating loop: cacheStore.tick(); ingestion.tick(); disposal.tick(dt); memoryGov.track(currentBytes); dispatcher.tick(ctx); decayTransitions();. Target: ~30 LOC.

Bonus — extract LODCoverageQuery

Single source of truth for "are all chunks under this region loaded?" — eliminates the duplicated CHUNK_COVERAGE_PADDING constant and the two divergent implementations between manager (1741-1766) and renderer (520-576).

Verification

• nix develop --command zig build test (includes shader validation)
• Required: headless screenshot baseline before extraction 1 and after each extraction (-Dskip-present, low/medium/high presets) — visual parity must be bit-exact
• nix develop --command zig build test -- --test-filter "LOD" after each extraction
• 508 LOC of tests in lod_manager.zig must continue to pass; tests move with their code into the new modules
• nix develop --command zig build -Doptimize=ReleaseFast (perf check — extraction must not regress frame times)

Constraints

• One extraction per PR — do not bundle. Each PR is independently revertable.
• Preserve current behavior bit-for-bit (visual + perf). Extraction is purely structural.
• Locking moves with the data: when extracting LODIngestionQueue, the ingestion_mutex must move into the new struct; do not leave the manager reaching across to it.
• Keep worker-thread RHI isolation intact — extracted schedulers/dispatchers must not introduce RHI calls on workers.
• Conventional commits: refactor(world-lod): extract LODCacheStore from LODManager etc.

Notes

• Issue Refactor lod_manager.zig - Separate LOD Logic from GPU Operations #246 (decoupling GPU ops via LODGPUBridge/LODRenderInterface) is done and clean — LODManager does not import engine-rhi. The remaining debt is data-structure encapsulation and pulling 4 subsystems out.
• Depends on R2-2 (LODMesh/LODChunk encapsulation) for clean extraction boundaries — consider landing R2-2 first.
• Zero TODO/FIXME across world-lod — the rot is structural, not attention-driven.

Tracking: #839

[github-actions]

Validation of the issue against the codebase:

• LOC/function counts match: lod_manager.zig is 2,715 lines, 89 functions, 12 tests; lod_renderer.zig is 1,789 lines. Field groupings in lod_manager.zig:183-289 match the 10 concerns listed (slight correction: 88 self.mutex acquisitions, not 87).
• Struct field table is accurate: every field referenced (regions, meshes, gen_queues, lod_gen_pool, upload_queues, transition_queue, memory_used_bytes, deletion_queue, deletion_timer, cache_dir_path, store_mutex, pending_ingestions, edit_dirty, ingestion_mutex, radius_shrink_chunks, player_cx/cz, next_job_token, stop_flag) is present at the cited line.
• update() is exactly 110 lines (lod_manager.zig:812-922) and does orchestrate the 14 subsystems listed (deletion sweep → NaN check → player publish → reprioritize → throttle → coverage unload → cache recenter → scheduling → generation → transitions → uploads → stats → memory budget → distant unload → ingestion drain → edit flush → store flush → transition decay).
• Duplicated coverage logic confirmed: CHUNK_COVERAGE_PADDING: i32 = 1 is declared identically in both lod_manager.zig:123 and lod_renderer.zig:62; areAllChunksLoaded (lod_manager.zig:1741) and isCoveredByChunks (lod_renderer.zig:520) reimplement the same min_cx/min_cz/max_cx/max_cz formula with subtly different policy.
• Cross-lock ordering comment at lod_manager.zig:276 is present verbatim.
• No existing branches/PRs target this work (git log --all --grep="LODCacheStore|LODIngestionQueue|..." is empty); repo only has refactor: resolve SOLID architecture seams #838 (SOLID architecture seams) on dev.
• No RHI import in LODManager — engine-rhi is not referenced from world-lod, confirming the LODGPUBridge/LODRenderInterface decoupling is in place.

Classification: Refactor (5-PR extraction + 1 bonus).

Scope: Correctly decomposed. The issue already does the sub-task split the audit checklist asks for — 5+1 independent, revertable extractions with verification gates (visual baselines, zig build test --test-filter "LOD", ReleaseFast perf check, 12 existing tests must move with their code).

Key blockers / sequencing:

1. R2-2 (LODMesh/LODChunk encapsulation) should land first, as the issue itself notes. processLODJob (lod_manager.zig:2055-2203) reaches into regions[lod_idx] storage and mutates chunk.state directly — the LODJobDispatcher extraction cannot establish clean ownership until LODRegionMap/LODChunk opaque-handle the state field.
2. processLODJob callback signature is the load-bearing constraint: Job callbacks take *anyopaque context. Any dispatcher extraction must keep the manager's opaque handle intact (or thread an opaque handle per-extraction) or every jobsystem registration breaks.
3. Worker-thread RHI isolation must move with the data — already stated in Constraints, but it specifically blocks moving processLODJob's buildMeshForChunk call path. buildMeshForChunk itself should be inspected during R2-2 to confirm it does not call RHI; if it does, the boundary moves further out.
4. Header-version validation — the issue lists validateHeader() bool on LODCacheStore but doesn't say who calls it during enable(). Confirm during extraction 1 that the legacy-cache delete path (enableCache deleting stale generator-keyed store, tested at lod_manager.zig:2266) is preserved — those two tests are the canary for the header-version invariant.

Suggested execution order (matches the issue's priority order, with the dependency note):

1. R2-2 first (prerequisite — block on it).
2. Extraction 5 LODJobDispatcher — largest LOC win but most delicate; do it before 1/2 so the cache/ingestion extractions don't have to re-thread a stale jobsystem boundary.
3. Extraction 2 LODIngestionQueue — highest single-extraction impact, removes the cross-lock ordering rule.
4. Extraction 1 LODCacheStore — moves 12 fns + 1 mutex; safe after 2 because both touch ingestion_mutex/store_mutex independently.
5. Extraction 4 LODMemoryGovernor — smallest, but depends on meshes/regions becoming opaque after R2-2.
6. Extraction 3 LODMeshDisposalQueue — independent, smallest diff.
7. Bonus LODCoverageQuery — last, requires both lod_manager.zig and lod_renderer.zig callers to migrate together (one of them must call the new shared module from the renderer's isCoveredByChunks site).

Verification gates per PR (already in the issue, reinforcing):

• nix develop --command zig build test (includes shader validation).
• Headless screenshot baseline before extraction 1 and after each — -Dskip-present at low/medium/high presets, bit-exact visual parity required.
• nix develop --command zig build test -- --test-filter "LOD" after each.
• nix develop --command zig build -Doptimize=ReleaseFast — frame-time regression check.
• The 12 test "LODManager ..." blocks (lod_manager.zig:2208-2669) must migrate with their code; specifically the cache tests (2208, 2242, 2266, 2294) move to lod_cache.zig extraction 1, the queue/upload tests (2321, 2399, 2561, 2586) move with the dispatcher extraction 5, and the memory/eviction test (2669) moves with extraction 4.

Minor copy-edit for the issue itself:

• 87 → 88 mutex acquisitions (counted via grep -c "self.mutex").
• "Removal" totals at the bottom of each extraction: Extraction 1 says ~12 private fns (matches — cacheKey, legacyCacheFilePath, readStorePayload, writeStorePayload, deleteStorePayload, deleteStoreContainer, loadCachedSourceData, saveCachedSourceData, recordCacheHit, recordCacheMiss, flushDirtyStores, logLegacyCacheNotice = 12); Extraction 2 says 9 methods (matches drainPendingIngestions, flushEditedChunks, enqueuePendingIngestion, processDeferredIngestion etc. — would benefit from enumerating them); Extraction 5 says ~6 methods but the listed functions are processLODJob, processQueuedGenerations, processStateTransitions, processUploads, requeueUpload = 5 methods.

No blockers beyond R2-2 sequencing. Issue is well-scoped and ready to land as 5+1 independent PRs against dev.

opencode session  |  github run