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

Author: matteius

src/database/db_core.c

@@ -1024,8 +1024,41 @@ void shutdown_database(void) {
     // database and verifies the copy, so a process that only read a few rows
     // would otherwise spend minutes on exit writing a duplicate of a database
     // it never modified.
+    time_t backup_interval_seconds = g_config.db_backup_interval_minutes > 0
+        ? (time_t)g_config.db_backup_interval_minutes * 60 : 0;
+    time_t now = time(NULL);
+    // now >= last_backup_time guards two edge cases: a backward system clock
+    // jump (NTP correction) after last_backup_time was recorded, and
+    // time(NULL) itself failing (returns (time_t)-1 per POSIX). Either would
+    // otherwise make `now - last_backup_time` negative -- always "less than"
+    // a positive interval -- incorrectly treating a backup as recent (or the
+    // timestamp as "in the future") and skipping the shutdown backup when it
+    // shouldn't be skipped.
+    bool recent_backup_exists = backup_interval_seconds > 0 &&
+        last_backup_time != 0 && now >= last_backup_time &&
+        now - last_backup_time < backup_interval_seconds;
+
     if (db_init_flags & DB_INIT_NO_BACKUP) {
         log_info("Skipping shutdown backup (read-only initialization)");
+    } else if (recent_backup_exists) {
+        // A backup already exists from within the configured scheduled
+        // interval -- most commonly, the hourly scheduled backup just ran a
+        // few minutes before a restart/shutdown was requested. Taking
+        // another one here would fully re-copy and re-verify a multi-
+        // gigabyte database (observed up to ~9-10 minutes) purely to
+        // capture a few minutes of additional changes, on every single
+        // restart. The on-disk database file itself is still fully
+        // checkpointed and closed cleanly below regardless -- this only
+        // skips the *extra* standalone backup-file snapshot, whose purpose
+        // is disaster recovery, not the shutdown's own data integrity. Worst
+        // case if corruption strikes shortly after, recovery falls back to
+        // a backup up to one scheduled interval old -- the same staleness
+        // bound already accepted during normal steady-state operation
+        // between scheduled backups.
+        log_info("Skipping final backup: last backup was %ld seconds ago, "
+                 "within the %d-minute scheduled interval",
+                 (long)(now - last_backup_time),
+                 g_config.db_backup_interval_minutes);
     } else if (db != NULL && db_file_path[0] != '\0') {
         log_info("Creating final backup before shutdown");
         // Not abortable: this is the deliberate one-time backup taken while

src/web/api_handlers_system.c

@@ -104,9 +104,27 @@ static const system_health_observation_t *find_health_observation(
 static const system_health_observation_t *find_effective_health_observation(
     const system_health_snapshot_t *snapshot, const char *container_metric,
     const char *host_metric) {
-    const system_health_observation_t *item = find_health_observation(
+    // Prefer the container-scoped observation only when it actually has a
+    // value -- not merely when one exists. linux_cgroup.c's collector picks
+    // CONTAINER vs. HOST scope for a whole batch of metrics (cpu, memory,
+    // pids) based on whether *any* of them has a real, finite limit; systemd
+    // gives every unit a default (non-"max") TasksMax=, so a completely
+    // unconstrained host (no CPUQuota=/MemoryMax=) still gets bumped to
+    // CONTAINER scope purely because of that unrelated pids limit. Its
+    // cpu.usage_ratio/memory.limit_bytes/etc. observations still get
+    // emitted under "container.*" in that case, just marked unavailable
+    // (unsupported/unlimited) -- and since find_health_observation() matches
+    // on existence, not availability, this lookup used to latch onto that
+    // permanently-unavailable container observation and never fall back to
+    // the perfectly good host.cpu.busy_ratio/host.memory.* figures at all.
+    const system_health_observation_t *container_item = find_health_observation(
         snapshot, container_metric, NULL);
-    return item ? item : find_health_observation(snapshot, host_metric, NULL);
+    if (container_item &&
+        container_item->capability == SYSTEM_HEALTH_CAPABILITY_AVAILABLE)
+        return container_item;
+    const system_health_observation_t *host_item = find_health_observation(
+        snapshot, host_metric, NULL);
+    return host_item ? host_item : container_item;
 }
 
 static bool health_observation_value(

tests/database/db_backup_test.c

@@ -14,6 +14,7 @@
 #include <sys/wait.h>
 #include <sys/stat.h>
 #include <limits.h>
+#include <dirent.h>
 
 #include "database/db_core.h"
 #include "database/db_backup.h"
@@ -28,6 +29,7 @@
 #define TEST_LARGE_BACKUP_PATH "/tmp/test_db_large.sqlite.bak"
 #define TEST_ABORT_DB_PATH "/tmp/test_db_abort.sqlite"
 #define TEST_ABORT_BACKUP_PATH "/tmp/test_db_abort.sqlite.bak"
+#define TEST_SHUTDOWN_DB_PATH "/tmp/test_db_shutdown.sqlite"
 
 static void remove_database_files(const char *path) {
     char sidecar[256];
@@ -650,6 +652,145 @@ static int test_backup_aborts_during_verification_when_shutdown_requested(void)
     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);
+    DIR *dir = opendir(backup_dir);
+    if (!dir) return 0;
+    int count = 0;
+    struct dirent *entry;
+    while ((entry = readdir(dir)) != NULL) {
+        size_t name_len = strlen(entry->d_name);
+        // Count only completed backups (name.sqlite3), not .tmp/-wal/-shm/
+        // -journal debris from an interrupted or in-progress copy.
+        if (name_len > 8 && strcmp(entry->d_name + name_len - 8, ".sqlite3") == 0) {
+            count++;
+        }
+    }
+    closedir(dir);
+    return count;
+}
+
+static void remove_test_db_and_backups(const char *db_path) {
+    char path[PATH_MAX];
+    static const char *sidecar_suffixes[] = {"", ".bak", "-wal", "-shm", "-journal"};
+    for (size_t i = 0; i < sizeof(sidecar_suffixes) / sizeof(sidecar_suffixes[0]); i++) {
+        snprintf(path, sizeof(path), "%s%s", db_path, sidecar_suffixes[i]);
+        unlink(path);
+    }
+
+    // A stale <db_path>.bak or -wal/-shm/-journal sidecar left behind by an
+    // earlier test would otherwise let init_database_ex() pick up a bogus
+    // last_backup_time (it stat()s the .bak path at startup) or a stale WAL,
+    // causing cross-test interference in this shared-connection test binary.
+    char backup_dir[PATH_MAX];
+    snprintf(backup_dir, sizeof(backup_dir), "%s.backups", db_path);
+    DIR *dir = opendir(backup_dir);
+    if (dir) {
+        struct dirent *entry;
+        while ((entry = readdir(dir)) != NULL) {
+            if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) {
+                continue;
+            }
+            snprintf(path, sizeof(path), "%s/%s", backup_dir, entry->d_name);
+            unlink(path);
+        }
+        closedir(dir);
+        rmdir(backup_dir);
+    }
+}
+
+// Regression test: shutdown_database() used to unconditionally take a full
+// backup-and-verify snapshot on every clean shutdown, no matter how recently
+// the hourly scheduled backup had already run -- on a multi-gigabyte
+// production database this made every restart take minutes just to
+// re-capture a few minutes of additional changes. Verifies that a shutdown
+// happening shortly after a scheduled backup skips the redundant one.
+static int test_shutdown_skips_backup_when_recent_backup_exists(void) {
+    int result = -1;
+
+    // db_core.c holds a single global connection shared across this whole
+    // file's main(); create_test_database() (run earlier in main()) opened
+    // TEST_DB_PATH and left it open -- nothing in between closes it until
+    // corrupt_database() does so itself, later, right before corrupting the
+    // raw file. This test needs a second, separate database open at the same
+    // time, so close the existing connection first, same as corrupt_database()
+    // does for its own reason.
+    shutdown_database();
+
+    remove_test_db_and_backups(TEST_SHUTDOWN_DB_PATH);
+
+    if (init_database(TEST_SHUTDOWN_DB_PATH) != 0) {
+        printf("Failed to init database for shutdown-skip test\n");
+        return -1;
+    }
+
+    g_config.db_backup_interval_minutes = 60;
+
+    if (maybe_run_scheduled_database_backup() != 0) {
+        printf("Scheduled backup failed in shutdown-skip test setup\n");
+        goto cleanup;
+    }
+    int count_after_scheduled = count_timestamped_backups(TEST_SHUTDOWN_DB_PATH);
+    if (count_after_scheduled != 1) {
+        printf("Expected exactly 1 backup after the scheduled cycle, found %d\n",
+               count_after_scheduled);
+        goto cleanup;
+    }
+
+    shutdown_database();
+
+    int count_after_shutdown = count_timestamped_backups(TEST_SHUTDOWN_DB_PATH);
+    if (count_after_shutdown != count_after_scheduled) {
+        printf("Shutdown took a redundant backup: had %d, now %d\n",
+               count_after_scheduled, count_after_shutdown);
+        return -1;
+    }
+
+    printf("Shutdown correctly skipped a redundant backup\n");
+    result = 0;
+
+cleanup:
+    remove_test_db_and_backups(TEST_SHUTDOWN_DB_PATH);
+    return result;
+}
+
+// Regression test for the same fix: when scheduled backups are disabled
+// (db_backup_interval_minutes <= 0), there is no defined freshness window,
+// so shutdown must still take its final backup rather than skipping
+// unconditionally.
+static int test_shutdown_backs_up_when_scheduled_backups_disabled(void) {
+    remove_test_db_and_backups(TEST_SHUTDOWN_DB_PATH);
+
+    if (init_database(TEST_SHUTDOWN_DB_PATH) != 0) {
+        printf("Failed to init database for shutdown-disabled-interval test\n");
+        return -1;
+    }
+
+    g_config.db_backup_interval_minutes = 0;
+
+    // init_database() takes its own "initial backup" of a brand-new database
+    // regardless of the scheduled interval, so the baseline here is 1, not
+    // 0 -- what this test actually verifies is that shutdown adds another
+    // one on top, rather than caring about that unrelated initial-backup
+    // implementation detail.
+    int count_before_shutdown = count_timestamped_backups(TEST_SHUTDOWN_DB_PATH);
+
+    shutdown_database();
+
+    int count = count_timestamped_backups(TEST_SHUTDOWN_DB_PATH);
+    remove_test_db_and_backups(TEST_SHUTDOWN_DB_PATH);
+    if (count <= count_before_shutdown) {
+        printf("Expected shutdown to take a backup with scheduled backups "
+               "disabled: had %d before, %d after\n", count_before_shutdown,
+               count);
+        return -1;
+    }
+
+    printf("Shutdown correctly backed up despite scheduled backups being disabled\n");
+    return 0;
+}
+
 // Test restore functionality
 static int test_restore(void) {
     char stale_wal[256];
@@ -735,6 +876,20 @@ int main(void) {
         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;
+    }
+
+    if (test_shutdown_backs_up_when_scheduled_backups_disabled() != 0) {
+        printf("Test failed: shutdown did not back up with scheduled backups disabled\n");
+        return 1;
+    }
+
+    // Restore the interval this file's own load_default_config() set, in
+    // case any later step in this binary implicitly depends on it.
+    load_default_config(&g_config);
+
     // Corrupt the database
     if (corrupt_database() != 0) {
         printf("Test failed: Could not corrupt database\n");