Thread safety: eliminate shared mutable state, fix dedx_get_ion_list() data race, add multi-threaded stress test

[grzanka]

Summary

libdedx currently ships no internal synchronization (no mutexes, atomics, or thread-local storage) and is not documented as thread-safe. The workspace-centric design can be used safely from multiple threads if each thread owns its own dedx_workspace, but there are concrete problems:

1. A genuine, unconditional data race in a public API function (dedx_get_ion_list) that bites callers regardless of how they partition workspaces.
2. Hidden mutation inside semantically read-only calls (dedx_get_stp writes a lookup cache), which makes "share one loaded workspace, read from many threads" silently produce wrong numbers.
3. No documentation of the threading contract, so callers cannot know the rules.
4. No test exercising concurrent use, and no sanitizer coverage in CI.

This issue tracks the analysis and the planned actions. Code changes will follow in separate PRs; this is the design/checklist issue.

Design decision (locked in): we are going for the stronger guarantee — a loaded workspace must be safely readable from many threads concurrently — even though it is the heavier change. See the "Chosen design" section below. The cheaper "one workspace per thread, full stop" contract was rejected.

---

Findings

1. dedx_get_ion_list() — static buffer data race (HIGH, usage-independent)

src/dedx.c:237

const int *dedx_get_ion_list(int program) {
    if (program == DEDX_BETHE_EXT00 || program == DEDX_DEFAULT) {
        static int temp[113], i;        // <-- function-scoped static mutable state
        for (i = 0; i < 112; i++)
            temp[i] = i + 1;
        temp[112] = -1;
        return temp;                     // TODO in source questions if this is even legal
    } else
        return dedx_program_available_ions[program];   // const table — fine
}

For the DEDX_DEFAULT / DEDX_BETHE_EXT00 branch the function fills a static buffer and returns a pointer into it. Two threads racing here write temp[]/i concurrently, and every caller gets the same pointer into shared mutable memory — so a thread can be reading the returned list while another rewrites it. The original author's own TODO comment flags the smell. Because the static is function-scoped (not per-workspace), per-thread workspaces do not save you.

Callers: dedx_fill_ion_list() (src/dedx_wrappers.c:54), check_ion() (src/dedx.c:525), plus examples/tests.

2. Hidden mutation in dedx_get_stp() via the lookup accelerator (MEDIUM/HIGH on shared workspace)

src/dedx_spline.c:141 (dedx_internal_evaluate_spline):

if (acc != NULL) {
    if (acc->cache >= 0 && acc->cache + 1 < n && (coef[acc->cache].x <= x) && (x < coef[acc->cache + 1].x)) {
        lookup = 0;
        i = acc->cache;
    }
}
if (lookup) {
    i = binary_search(coef, x, n);
    if (acc != NULL)
        acc->cache = i;          // <-- unsynchronized write during a "read-only" query
}

Crucially, dedx_internal_evaluate_spline already takes acc as a parameter — only the storage lives in the shared workspace. dedx_get_stp() passes &ws->loaded_data[id]->acc (src/dedx.c:370-374), i.e. shared mutable state embedded in dedx_internal_lookup_data (src/dedx_lookup_data.h:21; struct in src/dedx_lookup_accelerator.h). So dedx_get_stp() is semantically a pure lookup but mutates shared state. Two threads calling it on the same workspace race on acc->cache, which can select the wrong spline interval and return an incorrect, non-crashing stopping-power value. This is the subtle trap: callers reasonably assume read-only queries are safe to share. The fact that acc is already a parameter is what makes the fix clean (see Chosen design B).

3. Non-atomic workspace mutation in load_data() (MEDIUM)

src/dedx.c:444:

int active_dataset = ws->active_datasets;
...
ws->loaded_data[active_dataset]->... = ...;   // writes into shared slot
ws->active_datasets++;                         // non-atomic read-modify-write

Concurrent dedx_load_config() on one workspace can have two threads claim the same slot, clobber each other's data, and lose a dataset. (active_datasets is a plain int in dedx_workspace, include/dedx.h:189.)

4. What is already safe

• All embedded data is static const (stopping-power tables in src/data/embedded/*.h, periodic table dedx_amu[]/dedx_nucl[], program/material/ion name tables in src/dedx_program_const.h). Concurrent reads are fine.
• Pure query functions are thread-safe: dedx_get_program_name(), dedx_get_material_name(), dedx_get_ion_name(), dedx_get_version_string().
• No non-reentrant libc usage (strtok, setlocale, localtime, getenv, rand, static-returning helpers — none present).

---

Chosen design

Goal: after a workspace has been loaded, it is immutable, so any number of threads may read it (dedx_get_stp, dedx_get_csda, inverse lookups, name queries) concurrently with no locking. We achieve this by removing all writes from the read paths.

A. dedx_get_ion_list() → compile-time static const table (signature unchanged)

Replace the runtime-filled static int temp[113] with a static const int table (the full 1..112 + -1 terminator) generated at compile time, matching the const tables already in src/dedx_program_const.h. Return a pointer to it. No runtime writes, no race, no per-call work, no API/ABI change. (Reentrant _r variant is not needed and is dropped.)

B. Decouple the lookup accelerator from the workspace (the heavy change)

• Remove the acc field from dedx_internal_lookup_data (src/dedx_lookup_data.h). The workspace's loaded data (base[] spline coefficients, sizes, ids) becomes strictly read-only after dedx_load_config().
• dedx_internal_evaluate_spline keeps its existing acc parameter; the accelerator is now caller-owned, per-thread state rather than shared workspace state.
• Public API addition: introduce a small dedx_accelerator type and a reentrant
  float dedx_get_stp_r(dedx_workspace *ws, dedx_config *config, float energy, dedx_accelerator *acc, int *err);
  where the caller owns the accelerator (stack-allocated per thread, or heap per thread). Provide a trivial init helper / documented zero-init ({ .cache = -1 }).
• dedx_get_stp() stays, becomes fully thread-safe: it forwards to dedx_get_stp_r with a fresh stack-local accelerator each call (a single lookup gains nothing from a persistent cache, so there is no perf loss for the common single-shot case).
• Internal sweep loops keep their speedup and become thread-safe: dedx_get_csda, dedx_get_inverse_stp (and friends in src/dedx_tools.c) declare one stack-local accelerator before the loop and route iterations through the _r path. Stack-local ⇒ per-thread ⇒ no shared writes, while still amortizing the binary search across the monotonic sweep. This preserves what examples/dedx_bench_lookup measures.

Net effect: every read path performs zero writes to shared state, so a loaded workspace is data-race-free under concurrent reads by construction (no TLS, no hidden globals, no locks on the hot path).

C. Load path = documented single-writer

dedx_load_config() mutates the workspace and remains not safe to call concurrently on the same workspace. Contract: all loads for a workspace must happen-before any concurrent reads (e.g. load on one thread, then publish the workspace to readers). We will not add locking to the load path; we document the single-writer rule instead. (active_datasets++ therefore needs no atomics under the documented contract.)

---

Threading contract (to be documented + enforced by tests)

• A loaded dedx_workspace is safe to read concurrently from any number of threads via dedx_get_stp / dedx_get_stp_r / dedx_get_csda / inverse lookups / name queries — after all dedx_load_config() calls have completed (happens-before publication).
• dedx_load_config() is single-writer: do not call it concurrently on the same workspace, and do not load while other threads are reading that workspace.
• For accelerated sequential sweeps in your own threads, use dedx_get_stp_r with a per-thread dedx_accelerator. The plain dedx_get_stp is always safe but does not cache across calls.
• Read-only data and dedx_get_*_name() / version functions are safe from any thread.
• dedx_get_ion_list() is safe from any thread (returns const table).

---

Action items

A. Fix dedx_get_ion_list() race

• Replace the function-scoped static int temp[113], i; with a compile-time static const int full-ion table; return a pointer to it. Signature unchanged.
• Audit callers (dedx_fill_ion_list, check_ion) for assumptions about the returned pointer's lifetime/mutability.

B. Decouple accelerator → immutable-after-load workspace

• Remove acc from dedx_internal_lookup_data (src/dedx_lookup_data.h); drop the acc.cache = 0 write in load_data (src/dedx.c:458).
• Add public dedx_accelerator type + dedx_get_stp_r(...) in include/dedx.h; add a zero-init helper / document { .cache = -1 }.
• Reimplement dedx_get_stp as a thin wrapper over dedx_get_stp_r using a stack-local accelerator.
• Update src/dedx_tools.c sweep loops (dedx_get_csda, dedx_get_inverse_stp, etc.) to use one stack-local accelerator via the _r path.
• Confirm base[] spline coefficients are written only at load time and read-only afterward; mark read paths const where practical.

C. Load path

• Document dedx_load_config() single-writer contract in include/dedx.h and README. No locking added.

D. Documentation

• Add a "Thread safety" section to README.md and the public header docs in include/dedx.h stating: loaded workspace = concurrently readable; load = single-writer; dedx_get_stp_r + dedx_accelerator for per-thread accelerated sweeps; ion-list/name/version safe everywhere.

E. Multi-threaded stress test (cross-OS)

• Add tests/test_thread_safety.c (auto-discovered by file(GLOB test_*.c) in tests/CMakeLists.txt). It must:
  • Shared-workspace concurrent reads: load several configs once, then spawn N threads that hammer dedx_get_stp / dedx_get_stp_r (each with its own dedx_accelerator) on the same workspace, and assert every result matches a single-threaded golden reference. This is the test that proves finding Add support for ATIMA #2 is fixed.
  • Per-thread-workspace sweep: each thread loads its own workspace and runs dedx_get_csda / inverse lookups; assert against golden values.
  • Ion-list hammering: many threads call dedx_get_ion_list(DEDX_DEFAULT) / DEDX_BETHE_EXT00; assert every returned list is well-formed and -1-terminated (targets finding README: change to markdown format. #1).
  • Name/version query hammering as a baseline.
• Portable threading: C11 <threads.h> if available, else pthread (POSIX) / Win32 threads, behind a tiny shim in tests/test_helpers.h. Link Threads::Threads (find_package(Threads)) in tests/CMakeLists.txt.
• Real stress: high iteration counts, long enough to expose races; return non-zero failure count like the other test_*.c files.

F. CI / sanitizer coverage

• Run the new test under ThreadSanitizer (-fsanitize=thread) on Linux and macOS — the tool that actually catches findings README: change to markdown format. #1–add nuclear stopping power tables #3.
• Add macOS (macos-latest) to the CI matrix in .github/workflows/ci.yml (currently ubuntu-latest + windows-latest, and CTest only runs on Linux). The stress test should run on all three OSes.
• Dedicated TSan job (separate build dir / flags), since TSan can't combine with some other sanitizers. Windows relies on the functional golden-reference assertions (MSVC has no TSan).

---

Notes

• ABI: dedx_get_ion_list signature is unchanged (option A1). dedx_get_stp signature is unchanged; dedx_get_stp_r + dedx_accelerator are additive. Removing acc from the internal dedx_internal_lookup_data is internal-only.
• TSan on Windows isn't supported by MSVC; Windows coverage relies on functional assertions rather than the sanitizer.

PR order: (A) ion-list fix + assertion → (B) accelerator decoupling + _r API → (E) stress test + helpers → (C/D) docs → (F) CI matrix + TSan.

[grzanka]

Design decisions locked in (issue body updated accordingly).

We picked the stronger guarantee on purpose: a loaded workspace is safely readable from many threads concurrently, not just "one workspace per thread". Rationale and the resulting plan:

1. dedx_get_ion_list → compile-time static const table, signature unchanged (no reentrant variant needed). Kills the static-buffer race for all callers.
2. Accelerator decoupling (the heavy change): remove acc from the shared dedx_internal_lookup_data so every read path writes nothing to shared state → the loaded workspace is data-race-free under concurrent reads by construction (no locks, no TLS, no hidden globals). The accelerator becomes caller-owned, per-thread:
   • new additive API dedx_get_stp_r(ws, cfg, energy, dedx_accelerator*, err),
   • dedx_get_stp stays and becomes a stateless thread-safe wrapper,
   • internal csda/inverse sweep loops use a stack-local accelerator via _r, keeping the cross-call speedup that dedx_bench_lookup measures.
3. dedx_load_config = documented single-writer (loads happen-before concurrent reads); no locking on the load path.

This is what made it clean: dedx_internal_evaluate_spline already takes acc as a parameter — only its storage lived in the shared workspace. ABI impact is additive only (dedx_get_stp/dedx_get_ion_list signatures unchanged).

Coding to follow in PRs in this order: (A) ion-list fix → (B) accelerator decoupling + _r API → (E) cross-OS multi-threaded stress test (C11 <threads.h> with pthread/Win32 fallback) → (C/D) docs → (F) add macos-latest to CI + a ThreadSanitizer job.

---

Generated by Claude Code