mirror of
https://github.com/DeusData/codebase-memory-mcp.git
synced 2026-11-04 10:48:20 +00:00
fix(mem,spill): close two data races the worker threads exposed
TSan failed on all three platforms in PR CI. Both races are in code this branch
added, and both were invisible to the local ladder because it has no TSan leg —
that lane is CI-only, so "macOS and Linux green" said nothing about either.
1. system_info.c: the cached system info
cbm_system_info() published its cache with a plain flag:
if (!info_cached) { cached_info = detect_...(); info_cached = 1; }
Fine while only the main thread asked. The memory-relief work started calling
cbm_mem_system_under_pressure() from the per-file extract loop, so several
workers now race on the flag AND on the struct — and a struct assignment is
not atomic, so a reader could see half of one. Reported at
system_info.c:284 against :274, from extract_worker, on macOS, ubuntu-arm and
ubuntu-latest alike.
Now a three-state atomic: whichever thread wins the compare-exchange is the
only writer of cached_info, a loser returns the copy it detected itself (the
same answer — detection only reads the OS), and later readers see the
published struct through the acquire/release pair. No mutex, so nothing has
to be initialised before first use.
2. pass_parallel.c: the spill pointer
pp_spill_active() tested ec->pctx->spill BEFORE its atomic spill_mode gate,
so it read an 8-byte pointer with no synchronisation while pp_spill_enter was
publishing that pointer under spill_mu. pp_spill_enter assigns the pointer
and only then release-stores spill_mode, so gating on the atomic first makes
the pointer read ordered by that release. Short-circuit order is load bearing
in that expression now, and the comment says so.
Found by running the TSan lane locally with halt_on_error=0 rather than CI's
halt_on_error=1: the first setting stops at the first race, so each fix would
have revealed the next one a CI round trip later. Enumerating instead showed
the second race was the last one in that suite.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
1 parent
9db5217bdb
commit
9db483402e
2 files changed
+50
-13
No files matched your search
@@ -17,8 +17,9 @@ enum { DEFAULT_CORES = 1, MIN_WORKERS = 1, CBM_WORKERS_MAX = 256 };
|
||||
#include "foundation/mem_core.h" /* cbm_alloc: one accounted allocation path */
|
||||
#include "foundation/platform.h"
|
||||
#include "foundation/system_info_internal.h"
|
||||
#include <stdint.h> // uint64_t
|
||||
#include <stdlib.h> // strtol
|
||||
#include <stdatomic.h> // the system-info cache is published across worker threads
|
||||
#include <stdint.h> // uint64_t
|
||||
#include <stdlib.h> // strtol
|
||||
#include <string.h>
|
||||
|
||||
#ifndef _WIN32
|
||||
@@ -267,23 +268,51 @@ static cbm_system_info_t detect_system_windows(void) {
|
||||
|
||||
/* ── Public API ──────────────────────────────────────────────────── */
|
||||
|
||||
static int info_cached = 0;
|
||||
/* The cache is published once, by whichever thread gets there first.
|
||||
*
|
||||
* This used to be a plain `if (!info_cached) { cached_info = detect(); }`, which
|
||||
* was fine while only the main thread asked. It stopped being fine when the
|
||||
* memory-relief path started calling cbm_mem_system_under_pressure() from the
|
||||
* extract workers: several threads then raced on the flag AND on the struct,
|
||||
* and a struct assignment is not atomic, so a reader could see half of one.
|
||||
* TSan caught it on all three platforms (system_info.c:284 against :274, from
|
||||
* extract_worker).
|
||||
*
|
||||
* Three states rather than a flag, so the winner of the compare-exchange is the
|
||||
* only writer of cached_info: a loser returns the copy it detected itself,
|
||||
* which is the same answer, and a later reader sees the published struct
|
||||
* through the acquire/release pair. No mutex, so nothing has to be initialised
|
||||
* before first use, and no reader can observe a half-written struct. */
|
||||
enum { INFO_EMPTY = 0, INFO_CLAIMED = 1, INFO_READY = 2 };
|
||||
static _Atomic int info_state = INFO_EMPTY;
|
||||
static cbm_system_info_t cached_info;
|
||||
|
||||
cbm_system_info_t cbm_system_info(void) {
|
||||
if (!info_cached) {
|
||||
static cbm_system_info_t detect_system_now(void) {
|
||||
#ifdef _WIN32
|
||||
cached_info = detect_system_windows();
|
||||
return detect_system_windows();
|
||||
#elif defined(__APPLE__)
|
||||
cached_info = detect_system_macos();
|
||||
return detect_system_macos();
|
||||
#elif defined(__NetBSD__) || defined(__FreeBSD__) || defined(__OpenBSD__)
|
||||
cached_info = detect_system_bsd();
|
||||
return detect_system_bsd();
|
||||
#else
|
||||
cached_info = detect_system_linux();
|
||||
return detect_system_linux();
|
||||
#endif
|
||||
info_cached = SKIP_ONE;
|
||||
}
|
||||
|
||||
cbm_system_info_t cbm_system_info(void) {
|
||||
if (atomic_load_explicit(&info_state, memory_order_acquire) == INFO_READY) {
|
||||
return cached_info;
|
||||
}
|
||||
return cached_info;
|
||||
/* Detection only reads the OS (sysconf/sysctl/GetSystemInfo), so racing
|
||||
* threads doing it twice during startup costs a little and changes nothing. */
|
||||
cbm_system_info_t local = detect_system_now();
|
||||
int expected = INFO_EMPTY;
|
||||
if (atomic_compare_exchange_strong_explicit(&info_state, &expected, INFO_CLAIMED,
|
||||
memory_order_acq_rel, memory_order_relaxed)) {
|
||||
cached_info = local;
|
||||
atomic_store_explicit(&info_state, INFO_READY, memory_order_release);
|
||||
}
|
||||
return local;
|
||||
}
|
||||
|
||||
int cbm_default_worker_count(bool initial) {
|
||||
|
||||
@@ -832,8 +832,16 @@ static void pp_spill_enter(extract_ctx_t *ec, const char *reason) {
|
||||
}
|
||||
|
||||
static bool pp_spill_active(const extract_ctx_t *ec) {
|
||||
return ec->pctx && ec->pctx->spill &&
|
||||
atomic_load_explicit(&ec->pctx->spill_mode, memory_order_acquire) != 0;
|
||||
/* The atomic gates the pointer, not the other way round. pp_spill_enter
|
||||
* assigns ec->pctx->spill under spill_mu and only THEN release-stores
|
||||
* spill_mode, so a reader that has acquired a non-zero spill_mode is
|
||||
* guaranteed to see the finished pointer. Testing the pointer first read it
|
||||
* with no synchronisation at all while another worker was publishing it —
|
||||
* a genuine data race on an 8-byte write, which TSan caught at
|
||||
* pass_parallel.c:817 against this line. Short-circuit order is load
|
||||
* bearing here; do not reorder these terms. */
|
||||
return ec->pctx && atomic_load_explicit(&ec->pctx->spill_mode, memory_order_acquire) != 0 &&
|
||||
ec->pctx->spill;
|
||||
}
|
||||
|
||||
CBMFileResult *cbm_pipeline_result_acquire(const cbm_pipeline_ctx_t *ctx, CBMFileResult **cache,
|
||||
|
||||
Reference in new issue
Block a user