From 1fe56291ca085565a6cf7e4469aba2382e0dfd55 Mon Sep 17 00:00:00 2001 From: "lei.chen" Date: Sat, 5 Sep 2026 03:09:06 +0000 Subject: [PATCH] fix(testsuite): bug-924 -- fork-safe cleanup via per-entry owner pid (replaces pure-atexit) 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 --- src/testsuite_support/mkdtemp_clean.c | 66 ++++++++++++++++++--------- 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/src/testsuite_support/mkdtemp_clean.c b/src/testsuite_support/mkdtemp_clean.c index ad7729ff..f0d83b62 100644 --- a/src/testsuite_support/mkdtemp_clean.c +++ b/src/testsuite_support/mkdtemp_clean.c @@ -11,19 +11,28 @@ * 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 * declare_dependency() appended to each testsuite's shared *_test_deps list, - * see controller/meson.build and scheduler/testsuite/meson.build). Every - * scratch dir it creates is registered and recursively removed by an - * atexit() handler, so a test binary leaves /tmp clean regardless of which - * test "owns" the dir. Cleanup runs at process exit (after ALL TEST_ - * functions), so tests that legitimately inspect a fixture's on-disk - * contents across multiple TEST functions in the same file are unaffected -- - * the very property that made retrofitting cleanup into 70+ individual test - * files invasive. + * see controller/meson.build and scheduler/testsuite/meson.build). Each + * scratch dir it creates is registered with the pid of the process that + * created it and removed at process exit (atexit) -- but ONLY by the process + * that created it. + * + * Per-entry owner pid is what makes this fork-safe without any pthread_atfork + * hook: atexit() handlers are inherited across fork(), so a forked child that + * 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 * 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 - * harmless no-op (rmdir fails with ENOENT). + * dir was subsequently removed by the test itself, the sweep is a harmless + * no-op (rmdir fails with ENOENT). */ #define _GNU_SOURCE #include @@ -34,14 +43,20 @@ #include #include #include +#include #include #include #define MKDTEMP_SUFFIX "XXXXXX" #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 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_capacity = 0; static int g_clean_armed = 0; @@ -78,29 +93,34 @@ static void rmrf(const char* 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) { + pid_t me = getpid(); size_t i; pthread_mutex_lock(&g_clean_mu); for (i = 0; i < g_clean_count; i++) { - if (g_clean_dirs[i] != NULL) { - rmrf(g_clean_dirs[i]); - free(g_clean_dirs[i]); - g_clean_dirs[i] = NULL; + if (g_clean_dirs[i].path != NULL && g_clean_dirs[i].owner == me) { + rmrf(g_clean_dirs[i].path); + free(g_clean_dirs[i].path); + g_clean_dirs[i].path = NULL; + g_clean_dirs[i].owner = 0; } } - g_clean_count = 0; pthread_mutex_unlock(&g_clean_mu); } /* 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) { - char* copy; - size_t ncap; - char** ndirs; + char* copy; + size_t ncap; + mkd_entry_t* ndirs; copy = strdup(path); if (copy == NULL) { @@ -113,7 +133,7 @@ static void track_dir(const char* path) } if (g_clean_count == g_clean_capacity) { 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) { pthread_mutex_unlock(&g_clean_mu); free(copy); @@ -122,7 +142,9 @@ static void track_dir(const char* path) g_clean_dirs = ndirs; 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); }