Fix HLS cleanup, investigation playback, backup I/O, and ONVIF handling

docs/internal/BACKLOG_RELIABILITY_2026-09-12.md

@@ -0,0 +1,106 @@
+# Reliability backlog work — 2026-09-12
+
+Based on main `7f91155c9f7a0958fe6737b11c56d8090527779f` (0.41.17).
+The affected cameras are unavailable in this workspace. No additional SQLite
+error log or ODM capture is available beyond the issue reports.
+
+| Issue | Implemented or verified locally | Remaining evidence |
+| --- | --- | --- |
+| [#600](https://github.com/opensensor/lightNVR/issues/600) | Removed HLS `alarm()` calls and temporary SIGALRM/SIGSEGV dispositions; removed the freed-memory probe. Concurrent writer closure and full stream shutdown/restart pass. Fixed a codec metadata leak exposed by the lifecycle test. | The reported connection to a particular deadlock remains unproven. The system API suite completes. These tests do not establish freedom from every existing HLS context lifetime race. |
+| [#603](https://github.com/opensensor/lightNVR/issues/603) | Parse the subscription destination from the XML response's `SubscriptionReference`, including arbitrary prefixes and escaped URL characters. Escape `wsa:To`. Reject empty HTTP 200 responses, disable curl signal use, and log action/curl code/HTTP code/elapsed time. Test dropped connections and subsequent subscription reuse. | Tapo C530WS model/hardware revision/firmware, working ODM exchanges, and matching lightNVR exchanges are still needed. Keep open; this does not establish that the report is a duplicate of #567 or that these changes fix its empty connection response. |
+| [#580](https://github.com/opensensor/lightNVR/issues/580) | Removed whole-file cache eviction during `integrity_check`; retain full verification, copy-batch cache release, final eviction, shutdown abort and deadline checks. Schedule the next backup interval from completion and defer failed attempts without counting them as successful backups. Performance work remains linked to [#604](https://github.com/opensensor/lightNVR/issues/604). | Persistent timeline `SQLITE_IOERR` remains a separate investigation. Need the full failing operation and `code`/`extended_code`, with filesystem/device context. No database recovery or error suppression was added. |
+| [#568](https://github.com/opensensor/lightNVR/issues/568) | Native video play/pause updates shared investigation playback. Apply shared changes before delayed native events can undo a pause. Automatic pauses during media reload, clip end or errors do not stop the shared clock. Browser test exercises real media events and reload. | Reporter already confirmed the cropping fix; no further cropping change was needed. |
+| [#494](https://github.com/opensensor/lightNVR/issues/494) | Real RTSP input plus simulated ONVIF events produces a decodable detection-only MP4 containing footage before the trigger. Eight-second pre-buffer and three-second post-buffer are exercised. | Still needs footage from the affected camera on the current release. The synthetic source has a one-second GOP; it does not establish behavior for every camera's GOP, timestamps or reconnect history. |
+| [#579](https://github.com/opensensor/lightNVR/issues/579) | Existing browser tests pass: 1×1 selects main, multi-cell selects sub, and fullscreen upgrades only its selected cell. No new stream-selection change needed. | Retest the affected camera with main-only recording/viewing, then add sub-stream consumers. Its later failure in the manufacturer app remains a separate symptom; browser tests cannot validate camera stability. |
+
+## HLS lifecycle validation
+
+`test_hls_cleanup` runs three writers through three closure/recreation cycles,
+then three full unified HLS streams through two simultaneous restart cycles and
+simultaneous shutdown. The writer test reopens finalized playlists and reads
+media. The stream test paces local H.264 input to keep threads active during
+shutdown. Link wrappers reject alarm use or signal-disposition changes.
+
+The expanded test initially found 981 leaked bytes across nine stream lifetimes.
+LeakSanitizer traced them to codec extradata allocated during
+`avformat_find_stream_info`. `comprehensive_ffmpeg_cleanup` discarded those
+pointers and zeroed the codec parameters before FFmpeg could free them. It now
+leaves demuxer-owned parameters intact for normal input closure. The test passes
+with address/leak sanitizers enabled; the test does not suppress leak detection.
+
+## Backup measurement
+
+Reproduce on a disk-backed directory; `/tmp` on this workspace is tmpfs:
+
+```sh
+python3 tests/database/benchmark_backup_verification.py build
+```
+
+The script creates its own indexed audit fixture from migration 0055, starts
+each verification with cold file cache, measures `/proc/self/io` read bytes, and
+removes the fixture. It never opens an application database.
+
+One local run with 100,000 rows and an 81,793,024-byte fixture:
+
+| Verification policy | Read bytes | Elapsed | Integrity result |
+| --- | ---: | ---: | --- |
+| Evict whole file every 100,000 VM operations | 178,323,456 | 1.235 s | ok |
+| Retain cache during verification | 81,776,640 | 0.924 s | ok |
+
+An earlier 67 MB audit fixture showed 368 MB versus 67 MB read, and 3.437 s
+versus 0.844 s. The amplification depends on table/index layout and available
+memory; neither small local run predicts the reported 38–48-minute production
+backup duration. The repeatable result is reduced rereading while retaining the
+same integrity check. Verification may occupy more reclaimable OS page cache
+until completion; the SQLite heap limit and copy-phase controls remain.
+
+## Actual ONVIF pre-buffer footage
+
+```sh
+python3 tests/integration/synthetic-onvif-prebuffer.py --go2rtc /path/to/go2rtc
+```
+
+Requires the built application, frontend assets, FFmpeg and go2rtc. The harness
+uses dedicated temporary files, loopback camera ports and separately owned
+processes. It retains application/source logs, the MP4 and `result.json` in the
+printed `/tmp/lightnvr-prebuffer-*` directory.
+
+The source changes from red to blue at media second 12. Motion begins after
+second 15 and stops after second 18. Continuous recording is disabled, and the
+harness checks that there was no recording before the event. The MP4 must start
+red, end blue, and carry a pre-trigger interval in its metadata. This validates
+the footage, independently of a settings value or packet-buffer count.
+
+The first successful local run produced a 32.2-second clip with a red first
+frame (`255,0,0`) and a start time eight seconds before the simulated trigger.
+Logs showed 92 buffered packets flushed, 91 written from a keyframe, and entry
+into a three-second post-buffer. The total clip also includes the existing
+15-second ONVIF motion hold and detection grace interval.
+
+The checked-in harness also passed after the final C changes: a 31.4-second
+clip started red (`255,0,0`), ended blue (`0,0,255`), and began about 7.8 seconds
+before the trigger. Its local artifacts are in
+`/tmp/lightnvr-prebuffer-m2pnwzlh/result.json` and the sibling recording directory.
+
+## Checks
+
+Configured Debug build with tests enabled and SOD, LiteRT, MQTT and SSL disabled.
+Frontend production build passed. Targeted C tests passed:
+
+```sh
+cmake --build build -j8 --target lightnvr test_hls_cleanup \
+  test_detection_system_onvif test_db_backup test_packet_buffer \
+  test_api_handlers_system test_mp4_segment_timestamps
+ctest --test-dir build --output-on-failure --timeout 30 \
+  -R '^(test_hls_cleanup|test_detection_system_onvif|test_db_backup|test_packet_buffer|test_api_handlers_system|test_mp4_segment_timestamps)$'
+```
+
+Six C suites passed in 31.48 s; `test_api_handlers_system` completed in 20.67 s.
+The ONVIF suite includes 14 cases. Investigation browser tests passed (2 cases),
+as did the existing sub-stream/fullscreen tests (2 cases). Browser tests used
+`LIGHTNVR_TEST_DIR`, `LIGHTNVR_TEST_CONFIG`, and `LIGHTNVR_TEST_BIN` pointing at
+an isolated test instance, with `LIGHTNVR_SKIP_GO2RTC=1` for mocked live endpoints.
+
+These results support the local fixes and regression coverage. They do not
+close the hardware-dependent reports or identify the persistent SQLite I/O
+error's extended cause.

src/core/daemon.c

@@ -178,20 +178,8 @@ static void daemon_signal_handler(int sig) {
             web_server_socket = -1; // Update the global reference
         }
 
-        // Deliberately NOT using alarm() here as a "force exit if shutdown
-        // hangs" watchdog: alarm() is a single process-wide timer, and this
-        // codebase's HLS writer/context-close code (hls_unified_thread.c,
-        // hls_writer.c) uses alarm() extensively as a short per-operation
-        // timeout (save disposition, alarm(N), do the call, alarm(0),
-        // restore disposition). Any one of those firing during shutdown
-        // would silently discard whatever time was left on an alarm set
-        // here, since alarm() has no pause/resume -- confirmed live via
-        // gdb: a shutdown that should have had ~570s left was killed at 45s
-        // by one of those unrelated short-lived alarms, in this exact
-        // handler, right after this comment block previously called
-        // alarm(570) here. main.c's start_shutdown_watchdog_thread()
-        // (a dedicated thread polling `running`, spawned once at startup)
-        // now owns this responsibility instead, immune to that collision.
+        // main.c's dedicated shutdown watchdog owns the deadline. Do not
+        // introduce a process-global alarm shared with unrelated threads.
         break;
 
     case SIGHUP:
@@ -201,17 +189,8 @@ static void daemon_signal_handler(int sig) {
         break;
 
     case SIGALRM:
-        // No longer the shutdown watchdog (see main.c's
-        // start_shutdown_watchdog_thread()). Kept registered, and
-        // deliberately harmless, purely so SIGALRM has a caught
-        // (non-terminating) disposition as a safety net: the many
-        // hls_unified_thread.c/hls_writer.c call sites that use alarm() for
-        // their own short per-operation timeouts save whatever handler is
-        // installed here before temporarily switching it to SIG_IGN, and
-        // restore it afterward. If this weren't registered, SIGALRM's
-        // default disposition (process termination) would apply during any
-        // brief window where none of those local overrides happen to be
-        // active.
+        // Compatibility for externally delivered SIGALRM; HLS cleanup no
+        // longer arms alarms or changes the process signal dispositions.
         daemon_signal_safe_write("[DAEMON] Stray SIGALRM caught at top level (harmless, ignored)\n");
         break;
 

src/core/main.c

@@ -182,19 +182,9 @@ static void signal_handler(int sig) {
     // request_background_abort() is async-signal-safe (atomic store only).
     request_background_abort();
 
-    // Deliberately NOT using alarm() here as a "force exit if shutdown
-    // hangs" watchdog anymore -- see start_shutdown_watchdog_thread()'s
-    // comment for why: alarm() is a single process-wide timer, and this
-    // codebase's HLS writer/context-close code (hls_unified_thread.c,
-    // hls_writer.c) uses alarm() extensively as a short per-operation
-    // timeout (save disposition, alarm(N), do the call, alarm(0), restore
-    // disposition). Any one of those firing during shutdown would silently
-    // discard whatever time was left on an alarm set here, since alarm()
-    // has no pause/resume -- confirmed live via gdb: a shutdown that should
-    // have had ~570s left was killed at 45s by one of those unrelated
-    // short-lived alarms. A dedicated watchdog thread can't be clobbered
-    // this way. This handler now only sets flags and closes the listening
-    // socket, both async-signal-safe.
+    // The dedicated shutdown watchdog owns the deadline. Process-global
+    // alarm timers cannot safely implement independent thread timeouts.
+    // This handler only sets flags and closes the listening socket.
     if (web_server_socket >= 0) {
         close(web_server_socket);
         web_server_socket = -1;
@@ -203,15 +193,9 @@ static void signal_handler(int sig) {
 
 // Alarm signal handler -- MUST ONLY use async-signal-safe functions.
 //
-// No longer the shutdown watchdog (see start_shutdown_watchdog_thread()).
-// Kept registered, and deliberately harmless, purely so SIGALRM has a
-// caught (non-terminating) disposition as a safety net: the many
-// hls_unified_thread.c/hls_writer.c call sites that use alarm() for their
-// own short per-operation timeouts save whatever handler is installed here
-// before temporarily switching it to SIG_IGN, and restore it afterward. If
-// this weren't registered, SIGALRM's default disposition (process
-// termination) would apply during any brief window where none of those
-// local overrides happen to be active.
+// Retain harmless handling of externally delivered SIGALRM for compatibility.
+// HLS cleanup does not arm alarms or change process signal dispositions.
+// Shutdown deadlines belong to start_shutdown_watchdog_thread().
 static void alarm_handler(int sig) {
     (void)sig;
     signal_safe_write("[SIGNAL] Stray SIGALRM caught at top level (harmless, ignored)\n");
@@ -236,17 +220,8 @@ static void alarm_handler(int sig) {
 // by the time that elapses (this thread simply vanishes along with every
 // other thread on a normal exit, so it never fires in the common case).
 //
-// Deliberately NOT alarm()-based: alarm() is a single process-wide timer,
-// and hls_unified_thread.c/hls_writer.c use it extensively as a short
-// per-operation timeout (save disposition, alarm(N), do the call, alarm(0),
-// restore disposition) around individual writer/context close calls. Any
-// one of those firing during shutdown discards whatever time was left on an
-// outer alarm, since alarm() has no pause/resume -- confirmed live via gdb:
-// a shutdown that should have had ~570s left was killed at 45s by one of
-// those unrelated short-lived alarms. This thread runs independently of
-// SIGALRM entirely, so none of that matters here. Unlike a signal handler,
-// this runs in normal thread context, so log_error() and kill() are both
-// safe to call directly.
+// This dedicated thread keeps the deadline independent of process signals.
+// It runs in normal thread context, so log_error() and kill() are safe here.
 static void *shutdown_watchdog_thread_func(void *arg) {
     (void)arg;
     bool shutdown_started = false;
@@ -324,7 +299,7 @@ static void init_signals() {
         sigaction(SIGTERM, &sa, NULL);
         sigaction(SIGHUP, &sa, NULL);
 
-        // Set up alarm handler for phased forced exit
+        // Preserve harmless handling of externally delivered SIGALRM.
         struct sigaction sa_alarm;
         memset(&sa_alarm, 0, sizeof(sa_alarm));
         sa_alarm.sa_handler = alarm_handler;
@@ -1422,10 +1397,6 @@ int main(int argc, char *argv[]) {
     log_info("Stopping health check system...");
     cleanup_health_check_system();
 
-    // Cancel any pending alarm from signal_handler to prevent interference with cleanup
-    // alarm(0) cancels any previously set alarm - this is async-signal-safe
-    alarm(0);
-
     // Block most signals during cleanup to prevent interruptions
     // But keep SIGUSR1, SIGALRM, and SIGKILL unblocked for emergency shutdown
     sigset_t block_mask, old_mask;
@@ -1466,18 +1437,8 @@ int main(int argc, char *argv[]) {
         // Save the parent PID before it gets killed
         pid_t parent_pid = getppid();
 
-        // Phase 1 must outlast the deliberate, non-abortable final backup
-        // shutdown_database() takes before exiting (observed up to ~9
-        // minutes on this box's 2.6GB+ database) -- same reasoning as
-        // SHUTDOWN_WATCHDOG_TIMEOUT_SECONDS and the alarm() values in
-        // daemon.c/signal_handler(). This watchdog is fork+sleep()-based
-        // rather than alarm()-based, so unlike those it was never actually
-        // affected by the alarm()/SIGALRM collision with
-        // hls_unified_thread.c/hls_writer.c's per-operation timeouts --
-        // it was simply always too short (30s) on its own, and was in fact
-        // the one actually killing every real shutdown after those other
-        // fixes landed, confirmed via its distinct "phase 1/2 timed out"
-        // log lines.
+        // Allow the same final-backup deadline as the thread watchdog.
+        // Large databases can take several minutes to copy and verify.
         sleep(SHUTDOWN_WATCHDOG_TIMEOUT_SECONDS);
         log_error("Cleanup process phase 1 timed out after %d seconds", SHUTDOWN_WATCHDOG_TIMEOUT_SECONDS);
         kill(parent_pid, SIGUSR1);  // Send USR1 to parent to trigger emergency cleanup

src/database/db_backup.c

@@ -109,15 +109,13 @@ static void release_path_cache(const char *path) {
 }
 
 typedef struct {
-    int fd;
-    const char *path;
     bool abortable;
     const backup_deadline_t *deadline;
     bool deadline_hit;
-} cache_release_progress_t;
+} verification_progress_t;
 
 static int progress_during_verification(void *opaque) {
-    cache_release_progress_t *progress = (cache_release_progress_t *)opaque;
+    verification_progress_t *progress = (verification_progress_t *)opaque;
     /* Post-copy integrity_check scans the whole backup file page by page --
      * on a multi-gigabyte database this alone can take as long as the copy
      * it's verifying, with no other abort point once sqlite3_backup_finish()
@@ -134,9 +132,6 @@ static int progress_during_verification(void *opaque) {
             return 1;
         }
     }
-    if (progress && progress->fd >= 0) {
-        release_file_cache(progress->fd, progress->path);
-    }
     return 0;
 }
 
@@ -470,18 +465,16 @@ int backup_database(const char *source_path, const char *dest_path, bool abortab
         goto cleanup;
     }
 
-    /* Retain the full integrity guarantee, but release clean pages as SQLite
-     * scans the backup.  Without the progress callback this second full-file
-     * pass recreates the multi-gigabyte cache footprint bounded above.
-     * Registered whenever abortable even without a cache fd, since the
-     * abort-check matters independently of the cache-release optimization. */
-    cache_release_progress_t verification_progress = {
-        .fd = dest_cache_fd,
-        .path = temp_path,
+    /* integrity_check revisits table/index pages; evicting the whole file
+     * every 100k VM instructions forces those pages to be read repeatedly.
+     * Keep the OS's reclaimable cache during verification and discard it at
+     * completion below. Copying still flushes/releases each bounded batch.
+     * The progress callback remains responsible for aborts and deadlines. */
+    verification_progress_t verification_progress = {
         .abortable = abortable,
         .deadline = &deadline,
     };
-    if (dest_cache_fd >= 0 || abortable) {
+    if (abortable) {
         sqlite3_progress_handler(dest_db, BACKUP_VERIFY_PROGRESS_OPS,
                                  progress_during_verification,
                                  &verification_progress);