Drain LibMR threads to a safe point before fork() (MOD-15307)

[gabsow]

Summary

Adds MR_DrainForFork() / MR_ResumeAfterFork(). Called from the embedding module's pre-fork handler (main thread), MR_DrainForFork():

• parks the event-loop thread at a between-tasks safe point via a posted task, and
• bounded-waits the worker pool to idle,

so no LibMR thread holds a libc lock at fork() (which would ghost-lock the child). MR_ResumeAfterFork() releases the parked event-loop thread (called after the fork, on success or cancel).

Cooperative, bounded, fail-open. Deliberately not the existing mr_thpool_pause (SIGUSR2), which can freeze a worker mid-malloc holding the arena lock — exactly the ghost-lock this prevents. A worker still blocked acquiring the GIL is already malloc-safe, so a drain timeout there is benign.

Why / depends on

Fixes the RedisTimeSeries ASM-migration nightly hangs (MOD-14615 valgrind, MOD-14239 sanitizer). The embedding module wires this to redis core's new FORK_CHILD_PRE subevent (redis/redis#15327).

Pre-merge note

The two RedisModule_Log(..., "notice", ...) lines in MR_DrainForFork/MR_ResumeAfterFork should be downgraded to debug (kept at notice to confirm the drain fires during CI validation).

🤖 Generated with Claude Code

---

Note

High Risk
Changes pre-fork quiescence across the event loop and worker pool; a missed edge case could still fork with libc locks held and hang forked children (RDB/snapshots), though the design is bounded and fail-open.

Overview
Adds MR_DrainForFork() / MR_ResumeAfterFork() for the embedding module to call around Redis FORK_CHILD_PRE / post-fork: the event-loop thread is parked between tasks, then the execution pool is waited on (busy workers and queued jobs via new mr_thpool_num_jobs_in_queue) up to a 2s bounded deadline, then fork proceeds anyway if needed.

Separately improves MOD-14615 visibility when a multi-shard execution hits max-idle: per-execution sets of peers that ACK’d or sent NOTIFY_DONE, monotonic dispatch/progress timestamps, MR_ClusterFormatPendingPeers to list non-responding shards, and a warning log on timeout (plus debug on dispatch/drain).

^{Reviewed by Cursor Bugbot for commit 995f842. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit f6fe687. Configure here.}

[cursor]

Shared fork drain timeout

Medium Severity

MR_DrainForFork uses one CLOCK_REALTIME deadline for both parking the event-loop thread and waiting for the worker pool. Time spent waiting for the park task (including a long event-loop backlog before it runs) is subtracted from the same budget, so phase two often gets little or no time even after a successful park, and after a park timeout the pool loop exits immediately because the deadline is already past.

 

^{Reviewed by Cursor Bugbot for commit f6fe687. Configure here.}

[gabsow]

This one is intentional. MR_DrainForFork runs synchronously on redis's main thread before every fork, so the single MR_FORK_DRAIN_TIMEOUT_MS (2s) deadline deliberately bounds the total main-thread stall — we do not want a fork (RDB/AOF/ASM snapshot) to block the server longer than that.

In the normal case the park task runs in microseconds, so phase two effectively gets the full budget (measured median drain ~62us). If the park itself cannot complete within the budget, the event-loop thread is stuck off a safe point and the correct bounded fallback is to fork anyway — the drain is best-effort, not a hard barrier. Giving phase two its own fresh 2s budget would double the worst-case main-thread stall, which we deliberately avoid.

[gabsow]

@galcohen-redislabs wanted your opinion on the last commit (995f842) before we lean on it. It takes jobqueue.rwmutex when reading jobqueue.len in mr_thpool_num_jobs_in_queue, to address the Bugbot "unlocked job-queue length read" finding.

Two things I am not sure about:

1. Is it really necessary? The neighbouring mr_thpool_num_threads_working() reads num_threads_working unlocked as well, and MR_DrainForFork polls both in a 1ms loop that is best-effort (it forks anyway on timeout) — so a stale len self-corrects on the next tick. By that logic the unlocked read is arguably benign (same as the existing one), and we could just reply to the bot instead of adding a lock.

2. Isn't it dangerous? It adds a lock acquisition on the main thread, in the pre-fork drain path. As far as I can tell it's safe — the lock is taken and released inside the accessor (never held across fork()), and workers only hold rwmutex briefly in jobqueue_push/jobqueue_pull without needing the GIL, so there's no GIL/lock inversion or fork-time deadlock. But I would rather get your read before relying on it.

Happy to drop the lock and instead annotate the read as a benign race (matching num_threads_working) if you think that is cleaner/safer. What do you think?