Encapsulate native navigation ownership and fix Detour disposal leaks
Implementation follow-up to verified production heap investigation tsk_73fyezjdwp. User authorized building the safe navigation wrapper in a separate worktree from develop. Worktree /home/mathias/Projects/worktrees/engines-safe-navigation; branch fix/safe-navigation-ownership; base origin/develop 1f3279566. Concurrent client work in the original worktree remains untouched. No live production tests or deployments authorized. User additionally requires Kamal production deployment compatibility: verify the exact Dockerfile/amd64 build, native library and license delivery, dependencies and a smoke test inside the final runtime image locally. Repository build/packaging changes are in scope; do not run kamal deploy or app exec against Ithaca. Confirmed defect PathfindingPfSystem.Cleanup calls generic dtwFree on dtNavMeshQuery/dtNavMesh, skipping native destructors. Exact-library offline repro leaks 240 allocations after 20 cycles versus zero with destructor-aware release. Production dump contains 1,460 orphan tile arrays and ~971 MiB validated orphan Detour buffers associated with extensive allocator fragmentation. Intended outcome Make native navigation lifetimes correct by construction for both serialized outdoor meshes and dynamically baked ACG missions. Keep Recast/Detour algorithms and pathfinding semantics; introduce a narrow owning boundary, not a replacement navigation engine. Reuse and scope - Raw interop currently lives in RecastSharp 0.1.3; no existing shared owning navigation abstraction found. PathfindingPfSystem, AcgNavmeshBaker, NavmeshData, NavmeshDebugMeshBuilder and Tools/NavGenerator are the ownership-related call sites. - Extend/fix the native wrapper with stable C ABI destructor-aware release operations; do not make C# depend on platform-specific mangled C++ symbol names. Pin compatible native/header versions and account for build/package/runtime delivery. Decision: add PRK.Common/PRK.Navigation with a small opaque owning C ABI and vendored Recast/Detour revision cd898904b72a300011fbb24d578620bafa08ef2c (the revision behind RecastSharp 0.1.3). Retain upstream license/provenance. No mangled-symbol bindings or duplicate query ownership in engine code. Replace engine/generator RecastSharp references with this shared project. - Encapsulate mesh/query resources behind a disposable owner using SafeHandle-backed lifetime protection. Mesh must outlive queries; queries finish before disposal. Raw pointers, allocator selection and ownership flags remain internal. Construction is transactional: failed allocation/init/bake/tile insertion or exceptions release everything acquired so far. - Tile blobs have one owner until successful AddTile; transfer only on success. Failed transfers free the blob exactly once. Recast baking context/heightfields/contours/poly/detail meshes and temporary pins/buffers need the same scoped ownership rules. - Avoid query/dispose races. Current PathfindingPfSystem checks native pointers before acquiring _queryLock, while Cleanup clears them under the lock. Consolidate lifetime checks and native operations inside the wrapper's synchronization boundary; preserve serialized query behavior and allocation/performance constraints. - NavmeshData caches deserialized DtNavMeshCreateParams unmanaged buffers; NavmeshDebugMeshBuilder exposes freeParams boolean ownership. Replace ambiguous borrowed/owned representations with explicit managed data or typed scoped owners as appropriate, preserving the serialized format and cache reuse. Audit cache races/failed publication and exception paths for native buffer loss. - Shared helpers should serve PFE and generator/debug tooling where appropriate; do not duplicate a parallel unsafe implementation or refactor unrelated client work. Acceptance and offline validation - Engine call sites cannot accidentally raw-free mesh/query objects or use disposed handles; query-before-mesh teardown is guaranteed. - Existing outdoor/ACG path results, nearest-poly no-hit semantics, shortcuts/grid fallback and serialized navmesh compatibility remain unchanged. - Native allocation accounting returns to baseline after repeated create/load/query/dispose cycles, including tile ownership transfer, partial initialization, injected allocation/bake/AddTile failures, cancellation/exceptions, double disposal, disposal racing a query and forgotten-dispose finalization fallback. Include real native tests, not just mocks or RSS plateau assertions. - Bounded malformed/truncated navmesh input handling and unsafe-buffer lifetime tests prevent out-of-bounds native reads, allocator mismatches, double-free and use-after-free through the public wrapper. - Affected .NET builds/regressions and native packaging checks pass. Any unavailable platform gate remains explicit. Do not commit or deploy without explicit authorization. Scope boundary Safe ownership substantially prevents the proven leak class; it is not a guarantee against arbitrary bugs inside the native Recast/Detour implementation. Primary mitigation is correct release and ownership, not GC tuning or production allocator experiments. Implemented milestone (local worktree only) - Opaque mesh/query owner has native destructor-aware RAII, SafeHandle lifetime pinning, serialized queries/insertions, atomic managed disposed state and transactional tile ownership transfer. Managed geometry/tile snapshots prevent caller mutation and ambiguous unmanaged cache ownership. Native backend has a versioned C ABI; counters, fault injection and query coordination compile only into test builds. - Migrated outdoor loading and ACG baking, native temporary cleanup, managed tile codec/cache, debug extraction, NotumForge's dependent visualization call and offline generator. Existing serialized fields and query/grid fallback/snap policy retained. Generator's obsolete project references and byte-vs-ushort heightmap signature needed build compatibility updates. - Injected failures exposed additional upstream OOM crashes: null placement construction in Recast/query pools, uninitialized/null mesh tile tables during teardown and unchecked Recast vector allocation. Hardened those paths with marked vendor changes; do not claim every upstream native bug is eliminated. - Verified 118 dedicated native/managed tests in both Debug and Release on Linux, including 300 lifecycle cycles, 100 bake allocation-failure budgets, initialization failure rollback, failed tile insertion, finalizer fallback and deterministic entered-query/dispose lifetime pinning. The deterministic race caught SafeHandle.IsClosed remaining false while release is deferred; wrapper now tracks disposal request independently. - Offline corpus: 1,520 cached tiles extracted from 124 arrays in the production dump. Each loaded, round-tripped byte-identically and released all accounted native allocations. Corpus is external under Downloads and not committed. - Verified final outcomes - PFE, offline generator and AOEmu.UnitTests projects build. TheThreeEngines.sln builds with TreatWarningsAsErrors=true; host's invariant-globalization SDK emits eight package locale-normalization warnings, not C# source warnings. Existing unit/generator warnings remain unrelated. - Engine/pathfinding/ACG regressions: 85 passed, 14 asset-dependent cases skipped in this clean worktree. New record round-trip, truncated/corrupt input and repeatable debug-extraction tests pass. Dedicated navigation suite: 118 passed in each Debug and Release, including production corpus; no skips with the local corpus supplied. - Exact production Dockerfile built locally for linux/amd64 as prk-navigation-kamal-validation:local. Image SHA256 manifest 8b3f5c56b0987a784f3a615a1d000224393e6989aff2c59bc57e6637b034744c. Clean build exposed missing make dependency; Docker build stages and CI now install cmake, g++ and make. - Added a durable production-image gate using Tools/PRK.NavigationSmoke. It runs against the PFE's actual published wrapper and bundled runtime, checks that test fault-injection exports are absent, bakes procedural geometry, loads/queries and disposes 200 times under Kamal's CORECLR_ENABLE_PROFILING=0 / LD_LIBRARY_PATH / LD_PRELOAD settings. Gate passed. Harness is removed from the final filesystem; wrapper DLL/shared library and upstream license remain. ldd confirms all native dependencies resolve in aspnet:9.0 final image. Compiler tools stay in the build stage. - Offline YAML validation confirms Kamal builder amd64 and all four Ithaca PFE roles invoke the same /app/PlayfieldEngine directory. No registry push, kamal deploy, app exec, live restart, DB/broker access or production experiment performed. - git diff --check passes; native boundary and shared C# source formatted. Captured game data and analysis tooling remain outside the repository. No client files touched. User authorized commits and PR creation via /open-pr; committed ownership/integration as 3f4652956 and image validation as 960c6f6d4 on fix/safe-navigation-ownership. Remaining release gates Windows/macOS native build/runtime checks and the 14 game-asset-dependent engine cases were unavailable; do not treat Linux validation as proving those platforms or game-layout fixtures. Full unrelated engine test suite was not run. Normal review/merge into production's main branch and actual deployment remain outside this completed implementation task. User subsequently authorized committing and opening a pull request through /open-pr; deployment remains unauthorized. Pull request - #2555: fix: Encapsulate native navigation ownership - https://git.project-rk.com/prk/engines/pulls/2555 - Head: fix/safe-navigation-ownership; base: develop. Branch pushed; dedicated worktree clean before CI follow-up. CI follow-up PR #2555 engine job failed before compilation while installing CMake in the implicit Debian Bullseye runner container: the refreshed security index references libarchive13 3.4.3-2+deb11u5 but its pool URL returns HTTP 404. Reproduced the identical 404 in clean node:16-bullseye; native-tool installation succeeds in node:22-bookworm. Engine job now declares the same Node 22 Bookworm container as the existing client job, removing dependence on the implicit runner image. Validated Node 22 / Bookworm with SDK 10.0.400 and .NET 8/9 runtimes: lean engine build with TreatWarningsAsErrors=true has zero warnings/errors; unit-test project compiles with existing warnings; dedicated native suite has 117 passes and one expected capture-only skip. Pushed CI-only commit 8aed877f5 and documented evidence on PR #2555. Removed only generated Avalonia build-task files; worktree clean. Awaiting remote Forgejo rerun before closing the CI follow-up; local validation is not a claim of remote CI success. Production image/deployment configuration is unchanged.
Checklist
4 of 4 complete- Provide compatible destructor-aware native APIs and a typed lifetime-safe navigation owner, including query/dispose synchronization.
- Migrate outdoor loading, ACG baking, cached/debug parameter ownership and relevant tooling to transactional native resource ownership.
- Verify real native allocation accounting across successful and failing lifecycle/concurrency paths, plus pathfinding regressions and packaging checks.
- Verify Kamal's amd64 production Dockerfile builds locally and the final image contains, resolves and exercises the production navigation wrapper without affecting Ithaca.
- Area
- Project Rubi-Ka
- Project
- engines
- Kind
- Bug
- Priority
- High
- Task ID
tsk_ja2grbuq8p- Updated