Merge branch 'main' of github.com:opensensor/lightNVR

Author: matteius

include/database/db_backup.h

@@ -21,6 +21,13 @@
  */
 int backup_database(const char *source_path, const char *dest_path, bool abortable);
 
+/** Default maximum duration (seconds) an abortable backup may run before
+ *  self-aborting as a stuck-backup safety valve. Not test-only (unlike
+ *  db_backup_set_max_duration_seconds_for_testing(), declared separately in
+ *  the test file itself rather than here), since a test needs this value to
+ *  restore the default after overriding it. */
+#define DB_BACKUP_MAX_DURATION_SECONDS_DEFAULT (30 * 60)
+
 /**
  * Restore database from backup
  * 

src/database/db_backup.c

@@ -35,6 +35,52 @@ static pthread_mutex_t backup_mutex = PTHREAD_MUTEX_INITIALIZER;
 #define BACKUP_BUSY_RETRY_US 100000
 #define BACKUP_VERIFY_PROGRESS_OPS 100000
 
+/* Safety valve for a scheduled backup that never finishes.
+ * maybe_run_scheduled_database_backup() runs synchronously on the main loop
+ * thread, so a hung copy or verification step blocks all other periodic
+ * maintenance (service self-healing, further backup scheduling) indefinitely
+ * -- the only existing abort point is an explicit restart/shutdown request,
+ * which may never come. 30 minutes gives wide margin above the largest
+ * normal backup+verify cycle observed in production (~12 minutes for a 3GB
+ * database) while still bounding a truly stuck run instead of it silently
+ * blocking every subsequent scheduled backup for hours. */
+static int g_backup_max_duration_seconds = DB_BACKUP_MAX_DURATION_SECONDS_DEFAULT;
+
+/* Test-only: not declared in the public header (would otherwise let
+ * production code mutate a global timeout at runtime), reached via an
+ * `extern` declaration in the test file instead. */
+void db_backup_set_max_duration_seconds_for_testing(int seconds) {
+    g_backup_max_duration_seconds = seconds;
+}
+
+typedef struct {
+    struct timespec start;
+    int max_duration_seconds;
+} backup_deadline_t;
+
+static void backup_deadline_start(backup_deadline_t *deadline, bool abortable) {
+    deadline->max_duration_seconds = abortable ? g_backup_max_duration_seconds : 0;
+    /* CLOCK_MONOTONIC rather than time(NULL): a wall-clock jump backward
+     * (NTP sync, manual clock change) during a long-running backup would
+     * otherwise delay or defeat this safety valve entirely. */
+    clock_gettime(CLOCK_MONOTONIC, &deadline->start);
+}
+
+static bool backup_deadline_exceeded(const backup_deadline_t *deadline) {
+    /* A non-positive duration (production default is always positive; only
+     * reachable via db_backup_set_max_duration_seconds_for_testing()) means
+     * "already expired" -- lets tests force an expired deadline without any
+     * clock arithmetic that could underflow. */
+    if (!deadline || deadline->max_duration_seconds <= 0) {
+        return true;
+    }
+    struct timespec now;
+    clock_gettime(CLOCK_MONOTONIC, &now);
+    double elapsed_seconds = (double)(now.tv_sec - deadline->start.tv_sec) +
+                              (double)(now.tv_nsec - deadline->start.tv_nsec) / 1e9;
+    return elapsed_seconds >= (double)deadline->max_duration_seconds;
+}
+
 static void release_file_cache(int fd, const char *path) {
 #ifdef POSIX_FADV_DONTNEED
     int advise_rc = posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED);
@@ -66,6 +112,8 @@ typedef struct {
     int fd;
     const char *path;
     bool abortable;
+    const backup_deadline_t *deadline;
+    bool deadline_hit;
 } cache_release_progress_t;
 
 static int progress_during_verification(void *opaque) {
@@ -75,9 +123,16 @@ static int progress_during_verification(void *opaque) {
      * it's verifying, with no other abort point once sqlite3_backup_finish()
      * has run. A non-zero return here interrupts the running statement
      * (like sqlite3_interrupt()), so this is the only way to make that scan
-     * itself responsive to a pending restart/shutdown. */
-    if (progress && progress->abortable && is_background_abort_requested()) {
-        return 1;
+     * itself responsive to a pending restart/shutdown or a stuck-backup
+     * timeout. */
+    if (progress && progress->abortable) {
+        if (is_background_abort_requested()) {
+            return 1;
+        }
+        if (backup_deadline_exceeded(progress->deadline)) {
+            progress->deadline_hit = true;
+            return 1;
+        }
     }
     if (progress && progress->fd >= 0) {
         release_file_cache(progress->fd, progress->path);
@@ -205,11 +260,13 @@ static int run_integrity_check(sqlite3 *db_handle, const char *path_label) {
             // (progress_during_verification(), registered by the caller for
             // an abortable backup) surfaces here as SQLITE_INTERRUPT, not a
             // real failure -- the caller already logs its own, more specific
-            // "aborting during verification: restart/shutdown requested"
-            // warning right after this returns. Logging this as an error too
-            // would make every ordinary abort during a routine restart look
-            // like a corruption/failure event in the logs.
-            log_warn("Integrity check for %s interrupted (restart/shutdown requested)", path_label);
+            // "restart/shutdown requested" or "exceeded maximum duration"
+            // warning right after this returns, so this message is
+            // deliberately reason-neutral rather than claiming a specific
+            // cause. Logging this as an error too would make every ordinary
+            // abort during a routine restart look like a corruption/failure
+            // event in the logs.
+            log_warn("Integrity check for %s interrupted (abort requested)", path_label);
         } else {
             log_error("Failed to execute integrity check for %s: %s",
                       path_label, sqlite3_errmsg(db_handle));
@@ -254,6 +311,9 @@ int backup_database(const char *source_path, const char *dest_path, bool abortab
     backup_in_progress = true;
     pthread_mutex_unlock(&backup_mutex);
 
+    backup_deadline_t deadline;
+    backup_deadline_start(&deadline, abortable);
+
     if (snprintf(temp_path, sizeof(temp_path), "%s.tmp", dest_path) >= (int)sizeof(temp_path)) {
         log_error("Destination path is too long for temporary backup file: %s", dest_path);
         temp_path[0] = '\0';
@@ -370,6 +430,12 @@ int backup_database(const char *source_path, const char *dest_path, bool abortab
             rc = SQLITE_ABORT;
             goto cleanup;
         }
+        if (abortable && backup_deadline_exceeded(&deadline)) {
+            log_error("Database backup aborting early: exceeded maximum duration of %d seconds (stuck-backup safety valve)",
+                       g_backup_max_duration_seconds);
+            rc = SQLITE_ABORT;
+            goto cleanup;
+        }
     }
 
     // Finish the backup
@@ -413,6 +479,7 @@ int backup_database(const char *source_path, const char *dest_path, bool abortab
         .fd = dest_cache_fd,
         .path = temp_path,
         .abortable = abortable,
+        .deadline = &deadline,
     };
     if (dest_cache_fd >= 0 || abortable) {
         sqlite3_progress_handler(dest_db, BACKUP_VERIFY_PROGRESS_OPS,
@@ -422,7 +489,11 @@ int backup_database(const char *source_path, const char *dest_path, bool abortab
     int verification_rc = run_integrity_check(dest_db, temp_path);
     sqlite3_progress_handler(dest_db, 0, NULL, NULL);
     if (verification_rc != 0) {
-        if (abortable && is_background_abort_requested()) {
+        if (abortable && verification_progress.deadline_hit) {
+            log_error("Database backup aborting during verification: exceeded maximum duration of %d seconds (stuck-backup safety valve)",
+                       g_backup_max_duration_seconds);
+            rc = SQLITE_ABORT;
+        } else if (abortable && is_background_abort_requested()) {
             log_warn("Database backup aborting during verification: restart/shutdown requested");
             rc = SQLITE_ABORT;
         } else {

tests/database/db_backup_test.c

@@ -18,6 +18,11 @@
 
 #include "database/db_core.h"
 #include "database/db_backup.h"
+
+// Test-only hook into db_backup.c's internal duration safety valve. Not
+// declared in the public header (production code has no business mutating
+// a global backup timeout at runtime), so it's declared here instead.
+extern void db_backup_set_max_duration_seconds_for_testing(int seconds);
 #include "core/config.h"
 #include "core/logger.h"
 #include "core/shutdown_coordinator.h"
@@ -652,6 +657,172 @@ static int test_backup_aborts_during_verification_when_shutdown_requested(void)
     return result;
 }
 
+// Regression test for a bug found live in production: a scheduled backup
+// that hung during its copy phase blocked the main loop (which runs
+// maybe_run_scheduled_database_backup() synchronously) for almost 12 hours
+// straight, silently skipping every other scheduled backup in that window,
+// with no way to recover short of an operator happening to trigger a
+// restart. Verifies the new stuck-backup safety valve: an abortable backup
+// whose duration budget is already exhausted aborts on the very next
+// between-batches check instead of running unbounded.
+static int test_backup_aborts_early_when_duration_exceeded(void) {
+    sqlite3 *source = NULL;
+    sqlite3_stmt *stmt = NULL;
+    int result = -1;
+    int rc;
+    char temp_path[PATH_MAX];
+    struct stat st;
+
+    unlink(TEST_ABORT_DB_PATH);
+    unlink(TEST_ABORT_BACKUP_PATH);
+    snprintf(temp_path, sizeof(temp_path), "%s.tmp", TEST_ABORT_BACKUP_PATH);
+    unlink(temp_path);
+
+    rc = sqlite3_open(TEST_ABORT_DB_PATH, &source);
+    if (rc != SQLITE_OK) {
+        printf("Failed to create duration-test fixture: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    rc = sqlite3_exec(source, "PRAGMA page_size=4096;", NULL, NULL, NULL);
+    if (rc != SQLITE_OK) {
+        printf("Failed to set duration-test fixture page size: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    rc = sqlite3_exec(source,
+        "CREATE TABLE payload (id INTEGER PRIMARY KEY, data BLOB);",
+        NULL, NULL, NULL);
+    if (rc != SQLITE_OK) {
+        printf("Failed to create duration-test table: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    rc = sqlite3_prepare_v2(source,
+        "INSERT INTO payload(data) VALUES(zeroblob(?));", -1, &stmt, NULL);
+    if (rc != SQLITE_OK) {
+        printf("Failed to prepare duration-test fixture: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    // Larger than one BACKUP_STEP_PAGES batch (16MB) so the between-batches
+    // deadline check actually gets exercised before the copy would finish.
+    sqlite3_bind_int(stmt, 1, 20 * 1024 * 1024);
+    if (sqlite3_step(stmt) != SQLITE_DONE) {
+        printf("Failed to populate duration-test fixture: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    sqlite3_finalize(stmt);
+    stmt = NULL;
+    sqlite3_close(source);
+    source = NULL;
+
+    // Force an already-expired deadline deterministically, rather than
+    // waiting out a real timeout.
+    db_backup_set_max_duration_seconds_for_testing(-60);
+
+    rc = backup_database(TEST_ABORT_DB_PATH, TEST_ABORT_BACKUP_PATH, true);
+    db_backup_set_max_duration_seconds_for_testing(DB_BACKUP_MAX_DURATION_SECONDS_DEFAULT);
+    if (rc == 0) {
+        printf("Backup should have aborted early on an exhausted duration budget but reported success\n");
+        goto cleanup;
+    }
+
+    if (stat(temp_path, &st) == 0) {
+        printf("Duration-aborted backup left behind a temp file: %s\n", temp_path);
+        goto cleanup;
+    }
+    if (stat(TEST_ABORT_BACKUP_PATH, &st) == 0) {
+        printf("Duration-aborted backup should not have produced a final backup file\n");
+        goto cleanup;
+    }
+
+    printf("Backup aborted early on an exhausted duration budget, as expected\n");
+    result = 0;
+
+cleanup:
+    if (stmt) sqlite3_finalize(stmt);
+    if (source) sqlite3_close(source);
+    unlink(TEST_ABORT_DB_PATH);
+    unlink(TEST_ABORT_BACKUP_PATH);
+    unlink(temp_path);
+    return result;
+}
+
+// Same production bug as above, but for the verification phase: once the
+// copy loop reaches SQLITE_DONE it isn't re-checked, so a stuck
+// PRAGMA integrity_check needs its own deadline check (progress_during_
+// verification) to ever be interrupted. Uses the same cell-dense,
+// single-batch fixture as the shutdown-request verification-abort test so
+// the copy loop finishes without consulting the deadline, forcing this test
+// to exercise the verification-phase check specifically.
+static int test_backup_aborts_during_verification_when_duration_exceeded(void) {
+    sqlite3 *source = NULL;
+    sqlite3_stmt *stmt = NULL;
+    int result = -1;
+    int rc;
+    char temp_path[PATH_MAX];
+    struct stat st;
+
+    unlink(TEST_ABORT_DB_PATH);
+    unlink(TEST_ABORT_BACKUP_PATH);
+    snprintf(temp_path, sizeof(temp_path), "%s.tmp", TEST_ABORT_BACKUP_PATH);
+    unlink(temp_path);
+
+    rc = sqlite3_open(TEST_ABORT_DB_PATH, &source);
+    if (rc != SQLITE_OK) {
+        printf("Failed to create verification-duration-test fixture: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    rc = sqlite3_exec(source, "PRAGMA page_size=4096;", NULL, NULL, NULL);
+    if (rc != SQLITE_OK) {
+        printf("Failed to set verification-duration-test fixture page size: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    rc = sqlite3_exec(source,
+        "CREATE TABLE payload (id INTEGER PRIMARY KEY, data BLOB);",
+        NULL, NULL, NULL);
+    if (rc != SQLITE_OK) {
+        printf("Failed to create verification-duration-test table: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    rc = sqlite3_exec(source,
+        "WITH RECURSIVE seq(x) AS ("
+        "  SELECT 1 UNION ALL SELECT x+1 FROM seq WHERE x < 300000"
+        ") INSERT INTO payload(data) SELECT randomblob(16) FROM seq;",
+        NULL, NULL, NULL);
+    if (rc != SQLITE_OK) {
+        printf("Failed to populate verification-duration-test fixture: %s\n", sqlite3_errmsg(source));
+        goto cleanup;
+    }
+    sqlite3_close(source);
+    source = NULL;
+
+    db_backup_set_max_duration_seconds_for_testing(-60);
+
+    rc = backup_database(TEST_ABORT_DB_PATH, TEST_ABORT_BACKUP_PATH, true);
+    db_backup_set_max_duration_seconds_for_testing(DB_BACKUP_MAX_DURATION_SECONDS_DEFAULT);
+    if (rc == 0) {
+        printf("Backup should have aborted during verification on an exhausted duration budget but reported success\n");
+        goto cleanup;
+    }
+    if (stat(temp_path, &st) == 0) {
+        printf("Backup aborted during verification (duration) left behind a temp file: %s\n", temp_path);
+        goto cleanup;
+    }
+    if (stat(TEST_ABORT_BACKUP_PATH, &st) == 0) {
+        printf("Backup aborted during verification (duration) should not have produced a final backup file\n");
+        goto cleanup;
+    }
+
+    printf("Backup aborted during post-copy verification on an exhausted duration budget, as expected\n");
+    result = 0;
+
+cleanup:
+    if (stmt) sqlite3_finalize(stmt);
+    if (source) sqlite3_close(source);
+    unlink(TEST_ABORT_DB_PATH);
+    unlink(TEST_ABORT_BACKUP_PATH);
+    unlink(temp_path);
+    return result;
+}
+
 static int count_timestamped_backups(const char *db_path) {
     char backup_dir[PATH_MAX];
     snprintf(backup_dir, sizeof(backup_dir), "%s.backups", db_path);
@@ -876,6 +1047,16 @@ int main(void) {
         return 1;
     }
 
+    if (test_backup_aborts_early_when_duration_exceeded() != 0) {
+        printf("Test failed: Backup did not abort early on an exhausted duration budget\n");
+        return 1;
+    }
+
+    if (test_backup_aborts_during_verification_when_duration_exceeded() != 0) {
+        printf("Test failed: Backup did not abort during post-copy verification on an exhausted duration budget\n");
+        return 1;
+    }
+
     if (test_shutdown_skips_backup_when_recent_backup_exists() != 0) {
         printf("Test failed: shutdown did not skip a redundant backup\n");
         return 1;