lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(testsuite): bug-924 -- fork-safe cleanup via per-entry owner pid (replaces pure-atexit)

Browse source 
Pure-atexit cleanup (27e81738) regressed the scheduler repl/sim suites: their
real-stdout-capture helpers fork() a child that runs the REPL and exits via
exit(0), which runs the INHERITED atexit handler and deletes every dir the
PARENT registered -- including the dir the child just wrote its capture file
into, so the parent's fopen() sees ENOENT ("Expected Non-NULL" cascade; 8
repl/sim tests failed).

Fix: tag each registered dir with the pid that created it and have the atexit
sweep remove only entries whose owner == getpid(). A forked child inherits the
parent's entries tagged with the parent's pid, so the child's inherited atexit
removes nothing the parent owns; dirs the child itself mkdtemp()s are tagged
with the child pid and cleaned at its own exit. No pthread_atfork hooks are
registered -- a global atfork (prepare-lock or child-only) was tried twice and
regressed the controller suite's fork-heavy HA/commit tests (~98-119
failures), because a fork hook runs on every fork those tests do. The
parent-process path is byte-for-byte the behavior validated earlier
(controller full PASS, 0 failures); only fork-child cleanup semantics change.

Validated: scheduler repl/sim snapshot/step/rollup/event/breakpoints/exec
tests now pass 8/8 (was 8 fails under pure-atexit). Controller full
controller_unit_test re-run is pending a quieter machine -- this 7.8GB-RAM
box OOM-kills the 1269-test ASAN run before it finishes; no failures were
observed in the partial runs.

Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
lei.chen 2026-09-05 03:09:06 +00:00
parent 666192bc32
commit 1fe56291ca
1 changed files with 44 additions and 22 deletions
Download patch file Download diff file Expand all files Collapse all files

62
src/testsuite_support/mkdtemp_clean.c
View file

@ -11,19 +11,28 @@
* This file is the fix at the source. It provides a replacement `mkdtemp()` * This file is the fix at the source. It provides a replacement `mkdtemp()`
* that is linked into every unit-test executable in this project (via a * that is linked into every unit-test executable in this project (via a
* declare_dependency() appended to each testsuite's shared *_test_deps list, * declare_dependency() appended to each testsuite's shared *_test_deps list,
* see controller/meson.build and scheduler/testsuite/meson.build). Every * see controller/meson.build and scheduler/testsuite/meson.build). Each
* scratch dir it creates is registered and recursively removed by an * scratch dir it creates is registered with the pid of the process that
* atexit() handler, so a test binary leaves /tmp clean regardless of which * created it and removed at process exit (atexit) -- but ONLY by the process
* test "owns" the dir. Cleanup runs at process exit (after ALL TEST_ * that created it.
* functions), so tests that legitimately inspect a fixture's on-disk *
* contents across multiple TEST functions in the same file are unaffected -- * Per-entry owner pid is what makes this fork-safe without any pthread_atfork
* the very property that made retrofitting cleanup into 70+ individual test * hook: atexit() handlers are inherited across fork(), so a forked child that
* files invasive. * later calls exit() would otherwise run the cleanup and delete every dir the
* PARENT registered -- including the dir the repl/sim suites' real-stdout-
* capture helper just wrote its capture file into, so the parent's subsequent
* fopen() sees ENOENT ("Expected Non-NULL" cascade). Filtering cleanup to
* entries whose owner pid == getpid() means a child's inherited atexit removes
* nothing the parent owns (parent pid != child pid), while dirs the child
* itself mkdtemp()s after the fork are owned by the child and cleaned at its
* own exit. No fork hooks are registered, so forking tests are completely
* unaffected (a global pthread_atfork was tried and regressed the HA/commit
* suites that fork heavily).
* *
* Contract is glibc mkdtemp's: `tmpl` must end in XXXXXX (overwritten with a * Contract is glibc mkdtemp's: `tmpl` must end in XXXXXX (overwritten with a
* unique directory name); returns `tmpl` on success, NULL on error. If the * unique directory name); returns `tmpl` on success, NULL on error. If the
* dir was subsequently removed by the test itself, the atexit sweep is a * dir was subsequently removed by the test itself, the sweep is a harmless
* harmless no-op (rmdir fails with ENOENT). * no-op (rmdir fails with ENOENT).
*/ */
#define _GNU_SOURCE #define _GNU_SOURCE
#include <dirent.h> #include <dirent.h>
@ -34,14 +43,20 @@
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <sys/stat.h> #include <sys/stat.h>
#include <sys/types.h>
#include <time.h> #include <time.h>
#include <unistd.h> #include <unistd.h>
#define MKDTEMP_SUFFIX "XXXXXX" #define MKDTEMP_SUFFIX "XXXXXX"
#define MKDTEMP_TRIES 1000 #define MKDTEMP_TRIES 1000
typedef struct mkd_entry {
char* path;
pid_t owner;
} mkd_entry_t;
static pthread_mutex_t g_clean_mu = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t g_clean_mu = PTHREAD_MUTEX_INITIALIZER;
static char** g_clean_dirs = NULL; static mkd_entry_t* g_clean_dirs = NULL;
static size_t g_clean_count = 0; static size_t g_clean_count = 0;
static size_t g_clean_capacity = 0; static size_t g_clean_capacity = 0;
static int g_clean_armed = 0; static int g_clean_armed = 0;
@ -78,29 +93,34 @@ static void rmrf(const char* path)
rmdir(path); rmdir(path);
} }
/* Remove every registered dir owned by THIS process. Forks inherit a copy of
* the parent's registry, but every entry carries the pid that created it, so
* a child's inherited atexit removes nothing the parent owns. */
static void mkdtemp_cleanup_all(void) static void mkdtemp_cleanup_all(void)
{ {
pid_t me = getpid();
size_t i; size_t i;
pthread_mutex_lock(&g_clean_mu); pthread_mutex_lock(&g_clean_mu);
for (i = 0; i < g_clean_count; i++) { for (i = 0; i < g_clean_count; i++) {
if (g_clean_dirs[i] != NULL) { if (g_clean_dirs[i].path != NULL && g_clean_dirs[i].owner == me) {
rmrf(g_clean_dirs[i]); rmrf(g_clean_dirs[i].path);
free(g_clean_dirs[i]); free(g_clean_dirs[i].path);
g_clean_dirs[i] = NULL; g_clean_dirs[i].path = NULL;
g_clean_dirs[i].owner = 0;
} }
} }
g_clean_count = 0;
pthread_mutex_unlock(&g_clean_mu); pthread_mutex_unlock(&g_clean_mu);
} }
/* Own a copy of `path` (the caller's tmpl buffer is reused across mkdtemp /* Own a copy of `path` (the caller's tmpl buffer is reused across mkdtemp
* calls) and arrange for it to be removed at process exit (atexit). */ * calls), tagged with the creating pid, and arrange for it to be removed at
* process exit by that same pid. */
static void track_dir(const char* path) static void track_dir(const char* path)
{ {
char* copy; char* copy;
size_t ncap; size_t ncap;
char** ndirs; mkd_entry_t* ndirs;
copy = strdup(path); copy = strdup(path);
if (copy == NULL) { if (copy == NULL) {
@ -113,7 +133,7 @@ static void track_dir(const char* path)
} }
if (g_clean_count == g_clean_capacity) { if (g_clean_count == g_clean_capacity) {
ncap = g_clean_capacity ? g_clean_capacity * 2 : 64; ncap = g_clean_capacity ? g_clean_capacity * 2 : 64;
ndirs = (char**)realloc(g_clean_dirs, ncap * sizeof(*ndirs)); ndirs = (mkd_entry_t*)realloc(g_clean_dirs, ncap * sizeof(*ndirs));
if (ndirs == NULL) { if (ndirs == NULL) {
pthread_mutex_unlock(&g_clean_mu); pthread_mutex_unlock(&g_clean_mu);
free(copy); free(copy);
@ -122,7 +142,9 @@ static void track_dir(const char* path)
g_clean_dirs = ndirs; g_clean_dirs = ndirs;
g_clean_capacity = ncap; g_clean_capacity = ncap;
} }
g_clean_dirs[g_clean_count++] = copy; g_clean_dirs[g_clean_count].path = copy;
g_clean_dirs[g_clean_count].owner = getpid();
g_clean_count++;
pthread_mutex_unlock(&g_clean_mu); pthread_mutex_unlock(&g_clean_mu);
} }