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 3m 21s —— View job

---

Claude finished @rmrsk's review

Todo list

• Gather context (PR description, CLAUDE.md, diff vs origin/dev)
• Review Source/EBGeometry_Pool.hpp / EBGeometry_PoolImplem.hpp (control block mechanics)
• Review Source/EBGeometry_DCEL_Mesh.hpp / EBGeometry_DCEL_MeshImplem.hpp (rebasedView, base())
• Review Source/EBGeometry_PODVector.hpp, EBGeometry_Soup*.hpp, EBGeometry_Parser*.hpp, EBGeometry_MeshDistanceFunctions*.hpp
• Review test coverage (Tests/TestPool.cpp, Tests/TestDCEL.cpp)
• Check docs (Sphinx) reflect the new API
• Post final review

Summary

The core mechanism is sound: PoolControl gives pool-resident objects a stable indirection point that survives both a growing reserve() and the Pool object itself being moved (including std::vector<Pool> reallocation, which destroys the source). MeshT::base()'s host/device branch on m_control == nullptr is exact given that rebasedView() is the only producer of a null control block, and only for a device-accessible target. deepCopy() into the mesh's own pool is correctly safe because every read re-resolves through the shared control block between reserves, with no reserve happening between a read and its use inside the add loops (PODVector::push_back never reallocates). Test coverage (TestPool.cpp, new TestDCEL.cpp cases) directly exercises the scenarios called out in the PR description — grow-while-querying, Pool relocation via vector growth, same-pool deepCopy, snapshot/write-back across a grow, and host-to-host rebasedView. I did not find a correctness bug in the pool/mesh mechanics themselves.

Findings

1. Stale Doxygen comments on FlatMeshSDF/MeshSDF constructors describe deleted behavior (Source/EBGeometry_MeshDistanceFunctions.hpp:61-64 and :194-196)

Both constructors' @details still say:

"Freezes a_pool (idempotent -- safe even if already frozen by a sibling wrapper built from the same pool) and binds a_mesh to it..."

But the corresponding implementations (Source/EBGeometry_MeshDistanceFunctionsImplem.hpp:215-226, :260-280) no longer call freeze()/bind() at all — the diff shows those two lines were removed from both constructors, and MeshT::bind() was deleted from the class entirely per this PR's own description ("What this deletes from DCEL::MeshT: bind(), boundView()..."). So the doc now describes a method that doesn't exist. FlatMeshSDF's @param a_mesh doc also still says "built (but not necessarily bound)," another bind() leftover.

This is exactly the drift CLAUDE.md calls out ("update its Doxygen @param/... comment in the same header"), and it's a real regression relative to the rest of the PR: Docs/Sphinx/source/Parsers.rst and Examples/MeshSDF/main.cpp were correctly updated to drop the freeze/bind warnings for these same two classes, so this looks like a simple miss rather than a design decision. Since it's Doxygen source, it will also render into the generated API docs as-is.

Fix this →

2. Leftover stale comments in test files referencing the removed freeze/bind step (minor)

A few test helper comments weren't fully cleaned up when their freeze()/bind() calls were removed:

• Tests/TestDCEL.cpp:73-74 — buildTetrahedron's comment ("...so this test helper freezes+binds a_pool on its caller's behalf") is now dangling; the two lines it refers to were deleted but the comment sentence wasn't.
• Tests/TestBVH.cpp:128-129 and :1987-1988 — "readIntoDCEL never freezes/binds pool itself, but mesh is queried via its no-argument accessors below (before any SDF wrapper would otherwise do this for us)" is now misleading since no SDF wrapper freezes/binds either anymore.

Low severity (test-only comments, no functional impact), but worth a cleanup pass alongside finding #1 since they're the same category of leftover.

Testing note

I did not build or run the test suite (this was a read-only review); the PR description already reports 313/319 tests passing locally across presets and all examples running to completion, which I have no reason to doubt based on the code inspected.

[rmrsk]

@claude review

[github-actions]

Claude finished @rmrsk's task in 1m 50s —— View job

---

Claude finished @rmrsk's task

Todo list

• Gather context (PR description, CLAUDE.md, diff vs origin/dev)
• Check whether findings from the previous review pass were addressed
• Review Source/EBGeometry_Pool.hpp / EBGeometry_PoolImplem.hpp (control block mechanics)
• Review Source/EBGeometry_DCEL_Mesh.hpp / EBGeometry_DCEL_MeshImplem.hpp (rebasedView, base())
• Review Source/EBGeometry_PODVector.hpp, EBGeometry_Soup*.hpp, EBGeometry_Parser*.hpp, EBGeometry_MeshDistanceFunctions*.hpp
• Review test coverage (Tests/TestPool.cpp, Tests/TestDCEL.cpp)
• Check docs (Sphinx) reflect the new API
• Post final review

Summary

Re-reviewed the branch as of 3deeb1c. The core mechanism holds up well: PoolControl gives pool-resident objects a stable indirection point that survives both a growing reserve() and the Pool object itself moving (Source/EBGeometry_Pool.hpp/EBGeometry_PoolImplem.hpp). MeshT::attachTo/base()/rebasedView() (Source/EBGeometry_DCEL_MeshImplem.hpp:36-100) correctly discriminate host vs. device targets via isDeviceAccessible(), PODVector::endByte() correctly bounds by m_capacity (the reserved region) rather than m_size for the rebase-fits check, and Pool::mirrorOf() correctly records the root of a mirror chain (a_src.mirrorOf() != 0 ? a_src.mirrorOf() : a_src.id() in Source/EBGeometry_PoolImplem.hpp). I traced every addVertex/addEdge/addFace call site in Soup/Parser and confirmed push_back never grows the pool itself (EBGEOMETRY_EXPECT(m_size < m_capacity)), so the "no reserve between resolve and use" invariant that same-pool deepCopy and the reconciliation loops depend on actually holds. Examples/MeshSDF/main.cpp and Integrations/AMReX/PaintEB/main.cpp were both correctly simplified to match (one shared pool, no freeze/bind calls). The Sphinx docs (MemoryModel.rst, ImplemDCEL.rst, Parsers.rst) were re-read against the current API and are accurate and internally consistent — including MemoryModel.rst's remaining bind() references, which correctly refer to PODVector::bind() (still present), not the removed MeshT::bind(). I did not find a correctness bug in the pool/mesh mechanics.

Findings carried over from the prior review pass (still unresolved)

1. Stale Doxygen comments on FlatMeshSDF/MeshSDF constructors still describe deleted bind()/freeze() behavior (Source/EBGeometry_MeshDistanceFunctions.hpp:61-66, :194-196)

Both @details blocks still read "Freezes a_pool ... and binds a_mesh to it, so the mesh is immediately queryable ..." and FlatMeshSDF's @param a_mesh still says "built (but not necessarily bound)". MeshT::bind() no longer exists, and the corresponding constructors in EBGeometry_MeshDistanceFunctionsImplem.hpp:215-226/:260-280 no longer call freeze()/bind() — the mesh is queryable simply because it resolves through the pool's control block. Docs/Sphinx/source/Parsers.rst and Examples/MeshSDF/main.cpp were correctly updated to drop this language for the same two classes, so this reads as a miss rather than intent.

Fix this →

2. Leftover stale comments in test files referencing the removed freeze/bind step (minor, test-only)

• Tests/TestDCEL.cpp:75 — buildTetrahedron's comment ("...so this test helper freezes+binds a_pool on its caller's behalf") is dangling; no such call exists in that helper any more.
• Tests/TestDCEL.cpp:1509 — // freezes+binds hostPool comment on a call to the same helper, same issue.
• Tests/TestBVH.cpp:130-131, :684-686, :1989-1990 — "readIntoDCEL never freezes/binds pool itself, but mesh is queried via its no-argument accessors below (before any SDF wrapper would otherwise do this for us)" is misleading now that no SDF wrapper freezes/binds either.

No functional impact, but worth a cleanup pass alongside finding #1 since it's the same category of leftover from the bind()/freeze() removal.

Testing note

Read-only review; did not build or run the test suite. The PR description reports 313/319 tests passing locally and all examples running to completion, which the code inspected here is consistent with.