GEO::mesh_make_atlas() is not thread safe

[tksuoran]

Several APIs in geogram are not thread safe. Internally Geogram uses threads in places, but the implementation assumes there is single user at time, making it difficult or impossible for applications to use those APIs from multiple threads concurrently.

My request: is to make geogram APIs usable concurrently from multiple application threads.

Below are couple of examples I've run into recently:

Progress tracking system is not thread safe

Currently GEO::mesh_make_atlas() does not appear to be thread safe. It uses progress tracking system, which apparently is global and not thread safe. I would like to use GEO::mesh_make_atlas() from multiple threads, and ideally, also would like to use progress tracking system. I guess, one option could be to make progress tracking system thread local or user supplied.

ParallelDelaunay3d cannot be used while other geogram threads are running

CellStatusArray::resize asserts !Process::is_running_threads().

What happened

My application (the erhe 3D editor) runs geogram-based geometry operations on a thread pool: mesh operations (subdivide, boolean, remesh, ...) execute on worker threads while the main thread stays interactive. Some of those code paths use GEO::parallel_for, and the main thread occasionally builds small convex hulls via GEO::Delaunay::create(3, "PDEL").

When the main thread entered ParallelDelaunay3d::set_vertices() while a worker thread had geogram threads running (a GEO::parallel_for inside a mesh build), this debug assertion fired:

geo_debug_assert(!Process::is_running_threads());   // delaunay_sync.h:220, CellStatusArray::resize

Call stack of the asserting (main) thread:

GEO::geo_abort                          geogram/basic/assert.cpp:80
GEO::geo_assertion_failed               geogram/basic/assert.cpp:117
GEO::CellStatusArray::resize            geogram/delaunay/delaunay_sync.h:220
GEO::CellStatusArray::resize            geogram/delaunay/delaunay_sync.h:243
GEO::ParallelDelaunay3d::set_vertices   geogram/delaunay/parallel_delaunay_3d.cpp:2661
erhe::geometry::make_convex_hull        (application)

The precondition is documented ("no concurrent thread is currently running"), so this is arguably by design - but the design makes ParallelDelaunay3d (and, we suspect, other Process::run_threads-based algorithms) unusable in any application that runs geogram work on more than one thread, even when the two concurrent computations touch completely disjoint data.

[tksuoran]

I asked Claude to have a look - it found a lot of stuff.

thread_safety_analysis_and_plan.md

[tksuoran]

Here is the same plan inline.

I am not suggesting we should proceed with this plan, it but I thought it might have some useful info. I might try to execute the plan locally, obviously it will create a lot of stuff that would need to be reviewed carefully. Iff there is interest, I can give it a try and make PR.

Geogram thread-safety: audit and improvement plan

Companion document for issue #369
(GEO::mesh_make_atlas() is not thread safe / APIs not usable from multiple
application threads).

The audit below was made against main (v1.9.7-rc). File/line references are
relative to src/lib/geogram/.

---

1. Goal and proposed thread-safety contract

The proposed target is the standard C++ library contract:

After single-threaded GEO::initialize() and configuration, any public API
calls from different application threads are safe as long as they operate on
distinct objects (Mesh, Delaunay, NLContext, ...). Concurrent access
to the same object still requires external synchronization. Global
configuration (CmdLine/Environment writes, type/factory registrations,
Process setters) must happen before threads are started, or be externally
synchronized.

This framing requires no signature changes: every fix is internal state
plus contract documentation, plus a few purely additive APIs. The remaining
"breaks" are behavioral (semantics of existing functions), not source breaks.
They are listed in section 4.

---

2. Audit: what breaks concurrent use today

Threat model: multiple application threads, each calling geogram APIs on its
own disjoint data.

2.1 Threading core — root cause of the CellStatusArray abort

• basic/process.cpp:63 — running_threads_invocations_ is a plain global
  int, incremented/decremented by every Process::run_threads() call
  (process.cpp:472-474). Two app threads launching worker pools race on it
  (UB), and it makes Process::is_running_threads() answer a per-thread
  question ("am I inside geogram-spawned work?") with a process-global value.
  Direct consequences:
  • delaunay/delaunay_sync.h:220,289 — geo_debug_assert(!Process::is_running_threads())
    in CellStatusArray::resize()/clear() fires when an unrelated thread
    runs a parallel_for (the crash reported in GEO::mesh_make_atlas() is not thread safe #369).
  • voronoi/RVD.cpp:2421 — same check as a release-mode geo_assert:
    spurious process abort.
  • basic/process.cpp:723,778,807,825,851 — parallel_for /
    parallel_for_slice / parallel silently serialize on thread B whenever
    thread A happens to have a pool running.
  • delaunay/parallel_delaunay_3d.cpp:667, delaunay/periodic_delaunay_3d.cpp:903
    — memory-pool-overflow behavior keyed off the global flag.
  • basic/logger.cpp:425-460 — thread-safe logging path only selected when
    the flag is true (see 2.6).
• The default platform thread managers are themselves non-reentrant —
  concurrent Process::run_threads() calls from two app threads corrupt the
  manager even before any algorithm state is involved:
  • basic/process_unix.cpp:192 — PThreadManager (always selected on Unix,
    see process_unix.cpp:83) stores thread handles in a member
    std::vector<pthread_t> thread_impl_ resized/written per call.
  • basic/process_win.cpp:205,231,244 — WindowsThreadPoolManager (the
    Windows/MSVC default, process_win.cpp:502) assigns work-item ids from a
    static volatile LONG threadCounter_ that is reset at the start of every
    run; concurrent runs misassign or skip work items. The single
    TP_CALLBACK_ENVIRON cbe_ is also shared.
  • The plain WindowsThreadManager, OpenMP and TBB managers are per-call
    stateless and fine.
• basic/process.cpp:66-69 — multithreading_enabled_ / max_threads_ are
  plain globals read during algorithm execution from any thread.

2.2 Progress system — root cause of the mesh_make_atlas crash

basic/progress.cpp:50-52: the client (progress_client_), the task stack
(std::stack<const ProgressTask*> progress_tasks_) and the cancel flag
(task_canceled_) are unsynchronized file-scope globals. Every ProgressTask
constructor/destructor pushes/pops the shared stack (progress.cpp:64-71,
122-134); destruction from two threads trips the release-mode
geo_assert(progress_tasks_.top() == task), and the stack itself is corrupted
(UB) under concurrent push/pop.

ProgressTask creators in the library (all unsafe to run concurrently today):

Public API	Location
mesh_make_atlas()	parameterization/mesh_atlas_maker.cpp:260
remesh_smooth() (Lloyd + Newton)	mesh/mesh_remesh.cpp:94,104
frame field smoothing	mesh/mesh_frame_field.cpp:756
CSG compilation	mesh/mesh_CSG_compiler.cpp:209
Co3Ne reconstruct/smooth/reorient	points/co3ne.cpp:2107,2203,2582,2647
CVT (caller-supplied ProgressTask*)	voronoi/CVT.h:298,451

2.3 OpenNL — global "current context"

• NL/nl_context.c:53 — NLContextStruct* nlCurrentContext is a single
  global; the whole OpenNL API (nlBegin/nlEnd/nlSolve, GL-style) binds
  through it. Everything solver-based cross-wires under concurrency:
  LSCM / spectral-LSCM / ABF (i.e. mesh_make_atlas), mesh_smoothing,
  manifold harmonics, PGP.
• NL/nl_blas.c:1550-1552 — nlHostBlas() returns a static singleton whose
  stats fields (flops, RAM counters, sq_rnorm) are mutated during solves.
• Extension init state: NL/nl_mkl.c:239, NL/nl_os.c:116 (nl_num_threads),
  SuperLU/CHOLMOD/ARPACK/CUDA dynamic-loading pointers — write-once at
  nlInitExtension() time.

2.4 Global RNG

basic/numeric.cpp:53 — Numeric::random_int32/float32/float64/random_reset
share one global std::mt19937_64 with no lock. Reached unlocked from:

• sequential Delaunay point location — delaunay_2d.cpp:433,557,599 and
  delaunay_3d.cpp:522,557,599. There is a guard spinlock
  (delaunay_3d.cpp:510), but it is only taken when thread_safe is set, i.e.
  only when geogram's own pool uses the instance; and it is one global lock
  shared by all instances.
• mesh/mesh_AABB.cpp:678-680 — random ray directions for containment queries.
• mesh/mesh_sampling.h:145,304 — calls random_reset() (resets the shared
  engine!) then samples.
• mesh/mesh_surface_intersection.cpp:2107,2144-2200.

2.5 Reference counting

basic/counted.h:160 — Counted::nb_refs_ is a non-atomic int
(ref()/unref() at counted.h:79-95). Every SmartPointer copy/destroy of an
object shared across threads is a data race — this silently forbids even
read-only cross-thread sharing of any refcounted object (Delaunay,
NearestNeighborSearch, Environment, logger clients, ...).

2.6 Logger and console output

• basic/logger.cpp:425-460 — the spinlocked CERRStream path is only used
  when Process::is_running_threads() is true, which is never the case for
  application threads; those take the unsynchronized LoggerStream path.
• Mutable singleton state raced during normal logging: current_feature_,
  current_feature_changed_, clients_ (logger.h:727-739), and indent_ —
  which is also mutated by every verbose Stopwatch ctor/dtor
  (stopwatch.cpp:71,102).
• basic/logger.cpp:599,656 — geogram_printf/geogram_fprintf keep a
  static std::string last_string accumulator.
• basic/command_line.cpp:962-977 — terminal-UI globals (ui_separator_opened,
  ui_term_width, ui_left/right_margin, working_index) are mutated by
  ui_message()/ui_progress() during progress/logging output.

2.7 Lazy registries and unlocked config

• basic/factory.cpp:51-68 — InstanceRepo's registry map has no lock;
  first-time factory instantiation racing with any other factory access is a
  data race. points/nn_search.cpp:123-129 registers its creators lazily on
  every NearestNeighborSearch::create() call (magic-static guarded, but
  it mutates the shared registry on first use).
• basic/environment.cpp:140-156 — Environment::instance() lazy-creates the
  singleton behind a non-atomic static bool created latch;
  get_value/set_value (environment.cpp:176-204) traverse and mutate maps with
  no synchronization. All CmdLine::get_arg* calls route here — algorithms
  read config at runtime (e.g. delaunay.cpp:156, nn_search.cpp:133,
  RVD.cpp:2595, mesh_fill_holes.cpp:613, mesh_remesh.cpp:114-143,
  mesh_repair.cpp:1027-1041, mesh_io.cpp, co3ne.cpp).
• image/image_library.cpp:112-127 — bind_image/unbind_image mutate a global
  map with no lock.
• Eagerly registered at GEO::initialize() (safe if init is single-threaded):
  Delaunay factories (delaunay.cpp:127-150), mesh IO handlers
  (mesh_io.cpp:5028-5049), image serializers, attribute types
  (basic/common.cpp:112-174).

2.8 Long tail

• numerics/lbfgs_optimizers.cpp:59-68 — OptimizerConfig holds the HLBFGS
  callbacks in statics; the source comment already states this "prevents
  multiple optimizers to execute in parallel". mesh_make_atlas(PARAM_ABF) and
  spectral paths go through it.
• Vendored Triangle (third_party/triangle/triangle.c:677) — global
  randomseed mutated during every triangulation, plus exactinit() globals;
  Delaunay::create(2, "triangle") is unsafe even on disjoint data.
• Vendored xatlas (third_party/xatlas/xatlas.cpp:170-173,212-215) —
  global allocator/print hooks set via xatlas::SetAlloc from
  parameterization/mesh_param_packer.cpp:239; internal alloc tracking is
  mutex-guarded but the hook installation and packer are not designed for
  concurrent use.
• parameterization/mesh_PGP_2d.cpp:242 — function-local
  static double Rotc2[4][4] used as scratch (written at 434-437).
• image/image_serializer_xpm.cpp:208,287,359 — static conversion tables and
  a static line buffer (returned by pointer) refilled on every XPM read.
• Vendored Tetgen and PoissonRecon: to be audited (Tetgen looks
  instance-based).

2.9 Already safe (no action needed)

• Thread::current() is thread_local (process.cpp:297).
• Expansion heap pool is spinlock-guarded (numerics/multi_precision.cpp:767);
  stack expansions use alloca. Predicate filter constants are init-once;
  PCK::PredicateStats counters are std::atomic and compiled out by default.
• Attribute stores are per-mesh with a per-object spinlock for observers.
• FPU rounding-mode changes (numerics/interval_nt.h) are per-thread hardware
  state.

---

3. Plan

Phase 1 — threading core, progress, RNG (fixes both examples in #369)

WP1: per-thread is_running_threads() + reentrant thread managers.

• Make running_threads_invocations_ thread_local; redefine
  Process::is_running_threads() as: the current thread is a geogram worker
  (Thread::current() != nullptr) or has itself entered run_threads
  (thread-local counter > 0 — covers the sequential fallback where work runs on
  the calling thread; keep omp_in_parallel() for the OpenMP build).
• This preserves the intended meaning at every current call site and fixes,
  without touching them: the nested-parallelism fallback in
  parallel_for/parallel_for_slice/parallel, the asserts in
  delaunay_sync.h:220/289, RVD.cpp:2421,
  parallel/periodic_delaunay_3d.cpp, and the pool-overflow switches (which
  execute on worker threads, where the new semantics still report true).
• Optionally strengthen CellStatusArray with an instance-owned
  "my threads are running" flag set by ParallelDelaunay3d around its own
  run_threads calls (internal only, more precise assert).
• PThreadManager: make thread_impl_ a local variable (one-line fix).
• WindowsThreadPoolManager: per-invocation context struct
  (ThreadGroup* + std::atomic counter) as work-item context instead of the
  static counter; per-invocation callback environment.
• max_threads_ / multithreading_enabled_: relaxed atomics; all Process
  setters documented init-only.
• Documented consequence: N app threads each running parallel_for will
  oversubscribe cores (N × max_threads). Optional additive follow-up: a
  per-calling-thread max-threads override so an app's worker-pool jobs can run
  their inner loops with fewer threads.

WP2: progress system.

• Task stack and cancel flag → thread_local (per-call-stack LIFO is then
  guaranteed, the end_task assert becomes correct by construction).
• Each ProgressTask gains an std::atomic<bool> cancel flag + public
  ProgressTask::cancel() (additive).
• Keep global Progress::set_client() unchanged; add optional
  Progress::set_client_for_current_thread() (additive) so a UI can route
  worker N's progress to progress bar N.
• Cross-thread cancellation: a small mutex-protected registry of live tasks;
  Progress::cancel() cancels all live tasks (matches the CTRL-C handler's
  intent — documented semantic change). A client holding the task pointer from
  ProgressClient::begin() (invoked on the task's thread) can cancel one task
  precisely.

WP3: RNG. random_engine → thread_local; Numeric::random_reset()
resets the calling thread's engine (mesh_sampling.h determinism becomes
per-thread-deterministic). Remove the now-redundant global spinlock in
delaunay_2d/3d.cpp locate().

Phase 2 — makes mesh_make_atlas and solver-based APIs safe end-to-end

WP4: OpenNL. nlCurrentContext → thread-local via an NL_THREAD_LOCAL
macro (_Thread_local / __declspec(thread); must also be handled in the
OpenNL PSM extraction). Contexts become thread-affine — same rule as
modern OpenGL; simultaneous use of one context from two threads remains
illegal. nlHostBlas() → thread-local (stats become per-thread diagnostics).
nlInitExtension and backend function-pointer setup documented init-only;
CUDA backend documented single-threaded.

WP5: logger & console. Drop the is_running_threads() routing; make the
logger unconditionally safe: thread-local stream buffers per severity, client
dispatch serialized under a mutex (line-granularity interleaving),
indent_ → atomic, geogram_printf's last_string → thread_local, one
mutex inside CmdLine::ui_*. Contract: register clients before threads start,
or accept being called under the logger lock.

WP6: registries & config. Mutex on InstanceRepo + factory Registry
maps (cold paths); Environment::instance() via magic static; environment
values_/observers_ under std::shared_mutex (reads via CmdLine::get_arg*
are frequent but not inner-loop); mutex on ImageLibrary's image map.
Registration APIs (geo_register_attribute_type, serializers, factories,
geo_register_creator) documented init-only.

WP7: HLBFGS wrapper. OptimizerConfig statics → thread_local (HLBFGS
runs synchronously on the calling thread, so this suffices); audit vendored
HLBFGS for static work arrays, mutex if any found. Unblocks
mesh_make_atlas(PARAM_ABF) and spectral-LSCM.

Phase 3 — hardening and long tail

WP8: refcounting. Counted::nb_refs_ → std::atomic<int> (relaxed
increment, acquire-release decrement). Removes a whole footgun class (e.g.
sharing a built Delaunay or NN-search read-only across threads). ABI
recompile, no source break; cost negligible outside inner loops (geogram does
not copy smart pointers in hot loops).

WP9: vendored meshers.

• Triangle: ~10-line patch making randomseed + exact-arithmetic constants
  thread_local (preferred over a global mutex — keeps concurrent 2D
  triangulations parallel; upstream Triangle is frozen so drift risk is nil).
• Tetgen / PoissonRecon: audit; same treatment or a per-library mutex.

WP10: point fixes.

• mesh_PGP_2d.cpp:242 static scratch → local variable.
• XPM serializer statics → locals.
• xatlas: call SetAlloc once at init; conservatively mutex
  pack_atlas_using_xatlas() (packing is a small fraction of atlas time);
  revisit after auditing xatlas internals.
• CSG cache-directory writes: document / serialize.

Phase 4 — enforcement

• New test src/tests/test_multithread_api: N app threads ×
  {PDEL + sequential Delaunay, mesh_repair, remesh_smooth,
  mesh_make_atlas (all parameterizers/packers), booleans / surface
  intersection, RVD/CVT, Co3Ne}, with logging and progress clients active, and
  one thread cancelling another thread's task. Robot suite tagged smoke.
• CI: add a Linux Debug ThreadSanitizer job to continuous.yml
  (configure.sh --with-tsan plumbing already exists) running that test.
• Docs: a "Thread safety" page (wiki + doxygen) stating the contract, the
  init-only function list, and per-API notes (OpenNL thread affinity, RNG
  per-thread determinism, oversubscription).

---

4. API-change ledger

All changes are behavioral or additive; no signatures are removed or altered.

Change	Nature
Process::is_running_threads() becomes per-thread ("current thread is inside geogram-spawned work")	Semantic — this is the fix; the old process-global meaning has no consistent definition in a multi-threaded application
Progress::cancel() cancels all live tasks; Progress::current_progress_task() is per-thread; ProgressClient may be invoked from any thread (unless per-thread clients are used)	Semantic + additive (ProgressTask::cancel(), Progress::set_client_for_current_thread())
Logger clients serialized under an internal lock; cross-thread output interleaves at line granularity	Semantic
OpenNL contexts become thread-affine	Semantic (code creating a context on one thread and using it on another must move the usage; simultaneous shared use of one context was never legal)
Numeric::random_* sequences become per-thread	Semantic (reproducibility becomes per-thread)
Init-only contract formalized for: CmdLine declare/set, Environment::set_value, Process setters, PCK::set_SOS_mode, attribute/factory/serializer registration, xatlas::SetAlloc, nlInitExtension	Documentation
Counted refcount becomes std::atomic<int>	ABI recompile only

---

5. Suggested sequencing

Each work package is a separately reviewable PR, with the multithread test
extended alongside it:

1. WP1 alone fixes the ParallelDelaunay3d / CellStatusArray abort.
2. WP1 + WP2 + WP3 fix the mesh_make_atlas progress-stack corruption.
3. WP4–WP7 make the atlas path (and all solver-based APIs) correct
   end-to-end.
4. WP8–WP10 harden the long tail; Phase 4 keeps it fixed.

The semantic decisions worth agreeing on before code lands are the
is_running_threads() per-thread redefinition and the Progress::cancel()
semantics (section 4).