Pool-resident objects resolve through their pool, not a cached base

[rmrsk]

Summary

Background

Pool::reserve can grow the block, which moves it, which invalidates any base an object has cached. Until now that was handled by freeze(): seal the pool, then bind() each object to the now-stable base.

The cost was that an object could not be queried until the whole pool was sealed, so any workflow that keeps building — read ten STLs, deep-copy one, translate a few, read another — collided with a lifecycle decision that was only ever about pointer hygiene. The workarounds were already in the tree: Examples/MeshSDF used three pools because FlatMeshSDF/MeshSDF's constructors froze, and Parser::readIntoMesh(files, pool) was "deliberately not a loop over the single-file overload" for the same reason.

This is step 1 of the roadmap in PORTING.md; the design was agreed in PLAN.md before any code was written, and the BVH port (step 2) depends on it.

Solution

A Pool now publishes its base through a heap-resident PoolControl, and pool-resident objects hold a pointer to that rather than to the base itself, re-reading it on every access. A grow is therefore invisible to them, and freeze() survives only as the precondition for mirror() — a rule that justifies itself ("you cannot copy a block that might still move") rather than a precondition on asking a question.

Why a control block rather than a Pool*. MeshT previously cached the block address, and Pool's move constructor steals that address verbatim, so a mesh survived its pool being moved. Pointing the mesh at the Pool object would have silently broken std::vector<Pool> reallocation, pools held as class members, and factories returning a pool alongside its geometry — all of which work against the current code, none of which would have drawn a reviewer's eye in the diff. The control block's address does not change when the Pool moves, so that property is preserved.

What this deletes from DCEL::MeshT: bind(), boundView(), every explicit-base overload (public and protected), and the deepCopy(srcBase, dstPool) form. The accessor surface roughly halves.

The one crossing point is now rebasedView(const Pool&). It takes the Pool rather than a bare base so it can check that the target really is a mirror of the mesh's own pool and is large enough to hold its arrays, and it uses isDeviceAccessible() as a discriminator rather than an assertion:

• a device-accessible target yields a view holding a plain base address, since a kernel cannot follow a host control block;
• a host target yields a view holding that pool's control block, so host-to-host rebasing keeps working and stays growth-immune.

That second branch is what makes a null control block mean "device view" and nothing else, so base()'s assertions are exact in both directions rather than best-effort: a non-null control block on device means a host descriptor reached a kernel directly, and a null one on the host means a device view is being dereferenced there. It also keeps the rebase invariant testable on real geometry without a GPU, which matters because CI has no device.

A latent use-after-free is fixed on the way: deepCopy(srcBase, dstPool) documented dstPool as possibly being the source's own pool, but cached srcBase across three reserves that could move the block out from under it. The new form re-resolves on every read, so it is correct by construction.

Supporting additions: Pool::id()/mirrorOf()/control(), PODVector::endByte(), MeshT::isAttachedTo(). mirrorOf() names the root of a mirror chain rather than the immediate source, so staging through pinned memory on the way to a device still validates against the pool the geometry was built in.

Side-effects

Two properties are no longer enforced by the type system and must be carried by documentation instead. Both are now documented on Pool::reserve, on PODVector's class block, on MeshT's reference-returning accessors, and in MemoryModel.rst:

• A resolved address does not survive a reserve. A reference from getVertex/getEdge/getFace, or a PODSpan from bind(), points into a block that a growing reserve deallocates. The freeze state machine previously made this unreachable for users; allowing mid-build queries makes it reachable. The rule is resolve, use, discard — carry elements across a reserve by value and write them back through a fresh accessor call. Everything returning by value (signedDistance(), getAllVertexCoordinates(), …) is unaffected.
• Querying while another thread reserves from the same pool is a data race. Pools are not internally synchronized; previously the freeze rule made this structurally impossible.

Also note that the "mirror a host descriptor into a kernel" mistake goes from structurally impossible (#140) to possible-but-asserted. The assertions are exact, and every realistic error path is caught at the point of the mistake rather than as a garbage kernel result — but EBGEOMETRY_EXPECT compiles out in the release presets, so this is a debug-time guarantee rather than a type-level one.

API changes visible to users: Soup::reconcilePairEdgesDCEL loses its now-unused Pool parameter; the multi-file readIntoMesh/readIntoPackedBVH overloads became plain loops over the single-file form; Examples/MeshSDF and the AMReX PaintEB integration collapse to one pool and drop their freeze/bind calls.

Alternative solutions

• Store a Pool* and resolve m_pool->base(). Simpler, and was the original plan — rejected because it regresses Pool movability as described above, in ways an EBGEOMETRY_EXPECT cannot catch (a moved-from pool that has since been destroyed is a read of freed memory, and assertions are off in release).
• Make Pool immovable. Airtight, but mirror() returns by value, and it would stop users putting pools in containers at all.
• Handle/descriptor split — a host handle owning the Pool& plus a POD descriptor that crosses. Preserves the type-level guarantee, but the cost is not "two types and an API break": every FaceT/EdgeT/VertexT method takes a const Mesh&, so a second mesh type means templating the whole DCEL element API. That is the actual reason for accepting the weaker guarantee here.
• Reserve address space up front so the base never moves. A guarantee that only holds when someone sized the pool correctly is not a guarantee, and pools get created ad hoc deep inside third-party code.
• Separate viewIn and deviceView functions. Collapsed into one once the host branch was made to follow the target's control block — the accessibility predicate then selects behaviour rather than rejecting it, and no choice is pushed onto the caller.

Testing

313 unit tests pass under debug and debug-san (ASan/UBSan), 319 under release-test, and all 11 examples run to completion. Every pre-commit hook passes, including clang-tidy, Doxygen (warnings as errors), and both Sphinx builds.

New cases cover what was previously untestable:

• query an object, force the pool to grow, query again — same answers, no rebinding;
• a mesh surviving its Pool being moved, including a std::vector<Pool> reallocation, which destroys the source object rather than merely emptying it (the case a Pool* design fails);
• deepCopy into the same pool, i.e. the latent use-after-free above;
• snapshot-and-write-back across a grow — the reference path, not just the value path, since a value-only test would pass while the dangling-reference hazard was wide open;
• a MeshT-level host-to-host rebasedView, which exercises the rebase invariant on real geometry in CI without a GPU;
• Pool identity and transitive mirrorOf across a two-hop chain, and an abort on reserving from a moved-from pool.

Not verified: the CUDA/HIP [gpu] paths were not run on a device for this PR. MeshT::rebasedView's device branch is exercised by TestDCEL's existing [gpu] case, which needs a local GPU run before this is merged.

Reviewer checklist (to be completed by a human)

• The test suite compiles and runs to completion without warnings or errors.
• All relevant new features are documented in the user documentation (Sphinx).
• This contribution does not break existing sections in the user documentation.
• All relevant APIs are documented in the doxygen documentation.
• Appropriate labels have been assigned to this PR.
• New or revised proper licensing and copyright information is in place.
• A PR review has been run using @claude review.
• The continuous integration and testing hooks at GitHub run to completion.

[rmrsk]

@claude review

[github-actions]

Claude finished @rmrsk's task in 2m 52s —— View job

---

Review: Pool-resident objects resolve through their pool, not a cached base

• Gather context (PR description, CLAUDE.md, changed files)
• Review core Pool/PoolControl design (Source/EBGeometry_Pool.hpp, EBGeometry_PoolImplem.hpp)
• Review DCEL rebase/resolve changes (EBGeometry_DCEL_Mesh*.hpp)
• Review PODVector/MemoryResource additions
• Spot-check tests (TestPool.cpp, TestPoolRebase.cpp)
• Check docs (MemoryModel.rst, EBGeometry_MeshDistanceFunctions.hpp) against code
• Post final review

Overall this is a well-executed, well-tested redesign. The PoolControl indirection is the right fix for the Pool*-movability problem described in the PR body, base()'s device/host discriminator via a null-vs-non-null control block is exact and well-argued, and the offset-not-pointer design in PODVector genuinely does make growth and host→device mirroring transparent. TestPool.cpp/TestPoolRebase.cpp exercise exactly the cases that matter (grow-while-queried, std::vector<Pool> reallocation, moved-from abort, mirror-chain identity). I don't see a correctness bug in the pool/rebase machinery itself.

Finding: stale doc comments claim the FlatMeshSDF/MeshSDF constructors freeze the pool, but they don't

Source/EBGeometry_MeshDistanceFunctions.hpp:

• Line 61-63 (FlatMeshSDF's constructor): "Freezes a_pool (idempotent -- safe even if already frozen by a sibling wrapper built from the same pool) and binds a_mesh to it, so the mesh is immediately queryable..."
• Line 194-195 (MeshSDF's constructor): "Freezes a_pool (idempotent) and binds a_mesh to it before building the BVH..."

Neither actually happens. Source/EBGeometry_MeshDistanceFunctionsImplem.hpp's FlatMeshSDF/MeshSDF constructors only do:

EBGEOMETRY_EXPECT(a_mesh->isAttachedTo(a_pool));
(void)a_pool;   // nothing else done with it

No freeze() call, and bind()/boundView() don't even exist anymore — this PR's own description says they were deleted from MeshT. A repo-wide grep (grep -rn "\.freeze()" Source/ Examples/ Integrations/) turns up exactly one hit, and it's inside a Doxygen code sample in EBGeometry_DCEL_Mesh.hpp, not a real call site. Examples/MeshSDF/main.cpp's updated usage confirms this in practice: it builds dcelSDF/meshSDF/triSDF from one Pool and never calls freeze().

This also matches Docs/Sphinx/source/MemoryModel.rst (added in this same PR), which is explicit that "Freezing is not required to query anything" and exists solely as the mirror() precondition — so these two comments look like leftovers from an earlier design iteration where the constructor still did the old bind/freeze dance, not a description of current behavior.

It's a documentation-only issue (no functional bug — the mesh genuinely is queryable immediately, just not because of anything these constructors do), but it's worth fixing before merge since CLAUDE.md's own policy requires doc accuracy for changed public APIs, and a reader relying on this comment could reasonably assume mirror() is safe to call on a_pool right after constructing a MeshSDF/FlatMeshSDF, which it is not until they call freeze() themselves.

Fix this →

Minor / non-blocking observations

• Source/EBGeometry_ParserImplem.hpp's readIntoTriangles (and TriMeshSDF's constructor in EBGeometry_MeshDistanceFunctionsImplem.hpp) index vertexIndices[0..2] right after checking vertexIndices.size() != 3 and only logging (not bailing) on mismatch — an out-of-bounds read if a face genuinely has < 3 vertices. This predates this PR (the old code had the identical shape with shared_ptr<VertexT>), so it's not a regression introduced here, just noting it didn't get fixed while the surrounding code was touched.
• MeshT::attachTo's "already attached to a different pool" check is an EBGEOMETRY_EXPECT, so in a release build (assertions compiled out), calling reserveVertices/reserveEdges/reserveFaces with two unrelated pools would silently rebind m_control rather than fail — consistent with this PR's stated debug-time-only guarantee philosophy, just flagging it as the sharp edge that philosophy implies here specifically.
• Not verified by me (matches the PR's own disclosure): the CUDA/HIP [gpu] paths and MeshT::rebasedView's device branch weren't exercised on a real device.
  · Branch: pool-resident-objects

[rmrsk]

Closing as a duplicate: this PR and #143 track the same branch (pool-resident-objects, identical head commit), and this one was opened against main rather than dev, so its diff sweeps in twelve already-merged commits. The work is carried by #143.