fix(vision-mixer): join overlay timer threads before the process exits

[wagenet]

Cause

Two SIGSEGVs on macOS/arm64 in six graceful shutdowns of v0.6.8, both on thread overlay-timer-vmix, both inside pixman_fill under render_if_dirty.

The overlay timer thread is spawned detached, and its only stop condition is renderer unregistration, which the headless SIGTERM path never triggers. The thread therefore runs until the process dies.

It calls cairo, which draws through pixman. Pixman frees its global implementation chain from a destructor registered with __cxa_atexit, and exit() runs that destructor while other threads are still running, without clearing the global that points at the chain. Cairo entered after that point walks freed heap in _pixman_implementation_fill. This puts the fault deep inside pixman rather than at the Rust call site, and produces no panic or error line.

The unsafe impl Send/Sync on OverlayRenderer is sound: the mutex covers every path to the surface and context, and the cairooverlay callback does not touch this renderer. The surface is cairo-owned and held alive by the Arc the thread holds. This is teardown ordering, not a data race or a lifetime bug.

Fix

Keep each timer's JoinHandle, add a process-shutdown flag the loop checks once per tick, and call shutdown_overlay_timers() on both exit paths in main before library destructors run.

The join is bounded by one frame interval plus one render, since the flag is checked in both the wait loop and the render loop. The overlay appsrc is leaky and non-blocking, so a render in flight cannot stall it. Finished handles are reaped on each spawn. The six process::exit(1) sites are startup failures that run before any flow exists.

Reproduction

The crash itself was not reproduced. 11 shutdown cycles on the reporter's flow with 5 live WHIP publishers produced zero segfaults against the reporter's 2 in 6, so this is unremarkable, but the crash was never observed directly and the fix is reasoned rather than confirmed to that extent.

The precondition the crash requires is reproducible on both sides of the change. Same flow, one SIGTERM each:

	cairo renders after SIGTERM	timer at exit
without the fix	17, the last 625 ms after SIGTERM	still painting; log stops mid-loop
with the fix	0	Joined 2 overlay timer thread(s), ~200 µs

Two timer threads were live when main returned, and nothing stops them without this change. The log ending mid-render-loop matches the reported symptom.

Tests

shutdown_overlay_timers_joins_running_timer starts a timer against a real AppSrc, waits until a frame has been rendered and pushed so shutdown interrupts a thread inside cairo, then asserts the thread count is back to baseline the instant shutdown_overlay_timers() returns.

Verified to fail by reverting each half of the fix:

• discarding the JoinHandle: fails, left: 1, right: 0
• removing the shutdown flag but keeping the join: join() blocks forever, test hangs

Commands run

cargo clippy --all-targets --workspace   # clean
cargo test --workspace                   # 657 passed, 0 failed

No stop_flow() pipeline-survival ERROR in any shutdown log.

End-to-end runs used a debug build of this branch against the reporter's flow with the 5 builtin.audioenc blocks and their recorders removed, since builtin.audioenc does not exist upstream; the mixer, overlay, WHIP inputs and WHEP output are unchanged. The 11 shutdown cycles used the unmodified flow and the release binary that produced the original crashes.

Not run: any non-macOS platform. lldb was unusable on the test machine (system.privilege.taskport.debug unavailable), so the evidence is log-based rather than a debugger thread dump.

🤖 Generated with Claude Code

[srperens]

Verdict: Approve — right fix at the right layer, both process-exit paths covered, and the
new test genuinely exercises the changed module rather than re-deriving the behaviour inline.

Claims

Claim	Verdict	Evidence
Both process-exit paths call shutdown_overlay_timers() before returning	CONFIRMED	backend/src/main.rs:586 — `strom::blocks::builtin::vision_mixer::overlay::shutdown_overlay_timers();` in run_with_gui, after the GUI loop with no early return past it. Same call in run_headless at backend/src/main.rs:772 — `strom::blocks::builtin::vision_mixer::overlay::shutdown_overlay_timers();` — before serve_result? so it runs even on a server error
The wait loop and the render loop both observe shutdown	CONFIRMED	Both call still_mine, which checks the flag first — backend/src/blocks/builtin/vision_mixer/overlay.rs:1154 — `if OVERLAY_TIMERS_SHUTDOWN.load(Ordering::SeqCst) {`
The six process::exit(1) sites predate any flow/overlay timer	CONFIRMED	Two are the --hash-password subcommand (backend/src/main.rs:115, :128), two are config/logging init failures before the server starts (backend/src/main.rs:315, :322), two are TLS load failures inside setup_tls, called from run_headless before serve_with_tls (backend/src/main.rs:792, :798)
start_overlay_timer only ever runs from a real flow start, not from a plain .build()	CONFIRMED	It is queued via ctx.register_element_setup (backend/src/blocks/builtin/vision_mixer/builder/mod.rs:363-369), and ElementSetupFns are collected by take_element_setups() (backend/src/blocks/builder.rs:341) for the pipeline to run later — not invoked at build time, so the new test is the only lib-unit-test caller of this process-global shutdown flag and cannot race another test in the same binary
unsafe impl Send/Sync for OverlayRenderer is unmodified by this diff	CONFIRMED	backend/src/blocks/builtin/vision_mixer/overlay.rs:807-808, outside every diff hunk

Diagnosis — Matches the trace: a detached thread calling into cairo/pixman past exit(),
racing pixman's __cxa_atexit global-chain teardown. The pixman/cairo runtime behaviour itself
is EXTERNAL (I can't verify it from this repo), assumption stated in the PR body and
consistent with the fault site deep in _pixman_implementation_fill. The fix isn't
#[cfg(target_os = "macos")]-gated, so although only reproduced on macOS/arm64, it also covers
the same theoretical race on Linux. Coverage is ABSOLUTE for graceful shutdown (both main
return paths, both loop sites); it correctly leaves the six pre-flow startup exits alone.

Radius — LOCAL. New statics and one new public function confined to overlay.rs, called
from two sites in main.rs; still_mine gains one extra check, no other callers change.

Tests & CI — shutdown_overlay_timers_joins_running_timer starts a real timer against a
live AppSrc, waits for an actual render, then asserts overlay_timers_running() returns to
baseline the instant shutdown_overlay_timers() returns — it calls the changed functions
directly and the PR states it fails on each half of a manual revert (hang on one, 1 != 0 on
the other). Check (Linux) (cargo test --package strom --features efp,nvidia, where this
test lives) is green, along with Build (Linux x86_64/ARM64), Check & Build (WASM),
API Contract Check. Build (macOS)/Build (Windows) skip as usual — the crash and its fix
are unverified by this repo's CI on the platform that produced it, same as any macOS-only
concern here, but the mechanism and the test are platform-neutral.

Repo rules — No BUFFER probe, no new pipeline/element capture in a signal handler closure.

Confidence: HIGH

[srperens]

A second pass on top of the standing approval — five nits, none of them reopening the crash, so the verdict is unchanged.

Ran on this branch with main merged: cargo test --workspace and cargo clippy --all-targets --workspace both clean, and both revert scenarios in the body reproduce — discarding the handle gives left: 1, right: 0, and removing the flag while keeping the join hangs the test (27 s blocked in join() against a 0.26 s normal run, ended by SIGTERM).

1. A timer started concurrently with shutdown is never joined. backend/src/blocks/builtin/vision_mixer/overlay.rs:1209 — handles.push(handle); runs after spawn() returns, so if shutdown lands in between, shutdown_overlay_timers() takes a vector without that thread and main returns with it alive. Reachable if SIGTERM arrives while restart_flows is still bringing up a vision mixer, or if a start handler outlives the 10 s graceful_shutdown deadline, after which serve_with_tls returns regardless. The crash stays fixed — the thread re-checks the flag through still_mine before every path into cairo — but the invariant the fix rests on does not hold. Early-return from start_overlay_timer when the flag is already set, and re-check it after the push.

2. The stated bound understates the pre-PLAYING loop. backend/src/blocks/builtin/vision_mixer/overlay.rs:1062 — /// Terminal, and bounded by one frame interval plus one render. The appsrc is — a thread that has not yet seen PLAYING sleeps a fixed 100 ms before re-checking, so at 50 fps the join takes ~100 ms rather than the 20 ms implied. A flow stuck pre-PLAYING at SIGTERM (a WHIP input with no publisher) hits exactly that. Same hunk, backend/src/blocks/builtin/vision_mixer/overlay.rs:1048 — /// Timer threads currently inside their loop. — the fetch_add happens on the caller before spawn, so the counter also covers threads that have not started or are still waiting for PLAYING.

3. The test's reset is not panic-safe, and what it resets is process-global. backend/src/blocks/builtin/vision_mixer/tests.rs:445 — super::overlay::reset_overlay_timers_shutdown_for_test(); — if either assert fires (the first can on a loaded CI box), OVERLAY_TIMERS_SHUTDOWN stays true for the rest of the test binary, so any timer test added later exits at its first still_mine check and fails with a misleading message. The block also stays registered and the appsrc in PLAYING. A Drop guard covering the reset and the unregisters would fix all three.

4. The wait-then-assert on the buffer level can flake. backend/src/blocks/builtin/vision_mixer/tests.rs:428 — appsrc.current_level_buffers() > 0, re-reads a level the wait loop already observed non-zero, and it is a live queue level rather than a monotonic counter. With the src pad unlinked, the streaming task can dequeue the buffer before it hits NOT_LINKED and pauses, so a poll landing in that window sees 1 and the assert then reads 0. Capture the value observed in the loop, or count renders.

5. backend/src/blocks/builtin/vision_mixer/overlay.rs:1073 — warn!("Overlay timer thread panicked during shutdown: {:?}", e); — {:?} on a Box<dyn Any + Send> prints Any { .. }, losing the message exactly where it would matter. Downcast to &str/String first.

Checked separately, since the join bound rests on it: the non-blocking claim holds in production and not only in the test — backend/src/blocks/builtin/vision_mixer/builder/pipeline_cpu.rs:191 — .leaky_type(gst_app::AppLeakyType::Upstream) and backend/src/blocks/builtin/vision_mixer/builder/pipeline_gpu.rs:263 — .leaky_type(gst_app::AppLeakyType::Upstream), both with max_buffers(2), which the test's appsrc mirrors field for field.

Worth recording for whoever picks up #727: that issue and this PR share one root property — the timer's only stop condition is registry state, and nobody owns its handle — but neither fix covers the other. This flag only fires at process exit, so a failed start on a live process still leaks; and #727's suggested backstop (exit when the appsrc has no parent) is silent here, because at process exit on a healthy flow the appsrc still has a parent. This PR does make a leaked #727 thread leave its loop at shutdown, which removes the crash risk from it but not its CPU burn.