HLS alarm()-based operation timeouts are non-functional and race across stream threads

[davlaw]

While running a gcc -fanalyzer pass over the tree (which produced #599) I went through the alarm()-based "per-operation timeout" pattern in hls_unified_thread.c / hls_writer.c. I believe none of these timeouts actually work, and that they interfere with each other. Filing as an issue rather than a PR because the right fix is a design call, and because main.c / daemon.c already carry deliberate commentary about this exact hazard — I don't want to cut across a decision that was made on purpose.

Inventory

14 arming sites (alarm(N), N>0):

• hls_writer.c:973 (5s, trailer write), hls_writer.c:1012 (5s, AVIO close)
• hls_unified_thread.c:84, 264, 1818, 1853, 2115, 2659, 2892 (1s), :3077 (2s)
• hls_unified_thread.c:693, 1981 (15s, writer close), :2070, 2750 (15s, context free)

Each is wrapped in a save-disposition / alarm(N) / do-the-call / alarm(0) / restore-disposition block. Six of them set SIGALRM to SIG_IGN first; eight set SIGSEGV to SIG_IGN first.

1. The timeouts cannot interrupt anything

Sites that set SIGALRM to SIG_IGN (e.g. :693, :1981) — the signal is never delivered, so the alarm cannot interrupt the call it wraps. hls_writer_close() at :697 is unbounded regardless of the alarm(15) above it.

Sites that leave SIGALRM alone — the installed handler has SA_RESTART:

• daemon mode (production, -d): daemon.c:94 → sa.sa_flags = SA_RESTART
• interactive: main.c:331 → sa_alarm.sa_flags = SA_RESTART

With SA_RESTART, an interrupted syscall is automatically restarted after the handler returns, and both handlers are intentional no-ops (main.c:217, daemon.c:215, both just log "Stray SIGALRM caught at top level (harmless, ignored)"). So delivery does not break the blocked call out either.

The alarm_handler comment at main.c:205-214 is explicit that it exists only so SIGALRM has a non-terminating disposition as a safety net — it was never meant to implement these timeouts. So this reads as a real gap rather than a deliberate design.

2. alarm() is process-global, and these run on 7+ concurrent stream threads

alarm() is one timer per process, and SIGALRM from it is process-directed — it's delivered to any thread that hasn't blocked it, not necessarily the thread that armed it. With one HLS thread per stream, concurrent teardown means:

• a second thread's alarm(1) silently replaces a first thread's pending alarm(15)
• either thread's alarm(0) cancels the other's timeout outright
• the sigaction() save/restore pairs are non-atomic against each other on a process-global disposition, so an interleaved restore can install the wrong handler

main.c:1426 also calls alarm(0) during cleanup, cancelling anything the HLS paths had pending. main.c and daemon.c both document this collision hazard as the reason they refuse to add their own alarm() watchdog (main.c:185-192, daemon.c:181-192) — the hazard was worked around there, but the call sites causing it are still in place.

3. SIG_IGN on SIGSEGV is undefined behaviour

Eight sites do sigaction(SIGSEGV, …, SIG_IGN) around a "might touch freed memory" access, commented as a "try/catch-like approach … to prevent crashes" (e.g. :264 probing whether a context looks zeroed, :84/:3077 around FFmpeg cleanup).

POSIX: "The behavior of a process is undefined after it ignores a SIGFPE, SIGILL, SIGSEGV, or SIGBUS signal that was not generated by kill(), sigqueue(), or raise()." For a hardware-generated fault, ignoring it typically restarts the faulting instruction — an unkillable spin, not a safe skip. So if one of those accesses ever did fault, the outcome is likely worse than the crash it was trying to avoid, and the alarm() intended to bound it can't fire (per §1).

Possible connection — unverified

There's a known pre-existing deadlock in HLS stream-stop teardown (it reliably hangs test_api_handlers_system). If hls_writer_close() or the context-free paths can block, the alarm(15) guards that look like they'd bound them provide nothing. I have not confirmed that's the mechanism behind that hang — flagging it only because the two areas overlap and it'd be worth ruling in or out.

Options

1. Delete the pattern. If the timeouts don't fire and the SIGSEGV guards are UB, removing them loses no real protection and makes the teardown path far easier to reason about. Lowest risk, no behaviour change.
2. Make the timeouts real, per-thread: timer_create() with SIGEV_THREAD_ID, or a watchdog thread that flags/kills a stuck operation. Real protection, but a genuine design change in delicate code.
3. Status quo + comments marking them as non-functional, so the next person doesn't trust them.

My inclination is (1) now and (2) only if there's evidence of an actual hang worth bounding — but it's your call, and I'm happy to prepare whichever.

One caveat on (1): the validation at hls_unified_thread.c:657 (writer_to_free->stream_name == NULL, where stream_name is char[MAX_STREAM_NAME]) is always false, so its else at :706 — which deliberately skips the free and leaks the writer — is currently unreachable. Making that check meaningful would turn a no-op into a new leak path, so it probably wants deleting alongside, not repairing.

🤖 Generated with Claude Code

[davlaw]

Thanks for taking this on — #606 (df93ec7d) covers everything this issue raised. Checked against main:

• alarm(N) arming sites in hls_unified_thread.c / hls_writer.c: 14 → 0
• SIGSEGV references in hls_unified_thread.c (the SIG_IGN "try/catch" blocks): 24 → 0
• the freed-memory probe and the dead writer_to_free->stream_name == NULL / writer_valid block in writer teardown are gone with them

One small leftover, not worth reopening anything over: the two context-teardown checks at hls_unified_thread.c:1864 and :2502 still read ctx_to_free->stream_name == NULL || ctx_to_free->stream_name[0] == '\0'. stream_name is an array, so the == NULL half is always false — but the empty-string half still works, so they're redundant rather than broken.

The link wrappers rejecting new alarm() use or signal-disposition changes are a nice touch — that keeps the pattern from creeping back in.

Agreed with the note in BACKLOG_RELIABILITY_2026-09-12.md that the connection to the HLS stream-stop deadlock is unproven — I only ever had it as a hypothesis, and removing the non-functional timeouts doesn't establish it either way. Fine to close this from my side; if that deadlock resurfaces I'll open it separately with a backtrace rather than reopening this.

🤖 Generated with Claude Code

[matteius]

Thanks for checking the fix and catching the two remaining array-null comparisons. I've cleaned up both context-teardown paths in 676f9bae, now pushed to main: removed the always-false stream_name == NULL branches, retained the empty-string checks, and updated the warning text accordingly.

Rebuilt and reran test_hls_cleanup and test_api_handlers_system after the change; both passed. The HLS tests exercise concurrent writer closure and stream shutdown/restart, with wrappers checking that no alarm calls or SIGALRM/SIGSEGV disposition changes return.

Together with #606 (df93ec7d, shipped in 0.41.18), this completes the work raised here. The small follow-up cleanup is on main for the next release.

Agreed that this does not establish a fix for the separately suspected stream-stop deadlock. If that resurfaces, a separate report with a backtrace is the right next step.

Closing as completed. Thanks for the careful analysis and follow-up review.