diff --git a/front/js/graph_resource_history.js b/front/js/graph_resource_history.js index f5cbe17c3..56be2226d 100644 --- a/front/js/graph_resource_history.js +++ b/front/js/graph_resource_history.js @@ -2,6 +2,12 @@ // building a new one on the same canvas (Chart.js throws otherwise). var resourceHistoryChartInstances = {}; +// Monotonically increasing id, bumped on every initResourceHistoryGraphs() +// call. A fast double-click across two range buttons fires two overlapping +// AJAX requests; without this, whichever response lands last wins regardless +// of click order, so a stale response can silently overwrite newer data. +var resourceHistoryRequestId = 0; + // Chart axis labels: time only - date/year are dropped entirely to keep the // axis readable; the full date (including year) is still available on // hover via each chart's tooltip title callback below. Passed as @@ -25,7 +31,13 @@ var CHART_TIMESTAMP_OPTIONS_TOOLTIP = { * message when no rows exist yet (collection off, or just enabled). */ function initResourceHistoryGraphs(range) { + var requestId = ++resourceHistoryRequestId; + $.get('php/server/query_json.php', { file: `table_resource_history_${range}.json`, nocache: Date.now() }, function (res) { + if (requestId !== resourceHistoryRequestId) { + return; // a newer range request has since started - discard this stale response + } + var rows = (res && res.data) ? res.data : []; if (rows.length === 0) { diff --git a/front/systeminfoStorage.php b/front/systeminfoStorage.php index 99eb47202..f01e2d5aa 100755 --- a/front/systeminfoStorage.php +++ b/front/systeminfoStorage.php @@ -21,16 +21,25 @@ $nax_db_size = file_exists($nax_db) ? number_format((filesize($nax_db) / 1000000 $nax_wal_size = file_exists($nax_wal) ? number_format((filesize($nax_wal) / 1000000), 2, ",", ".") . ' MB' : '0 MB'; $nax_db_mod = file_exists($nax_db) ? date("F d Y H:i:s", filemtime($nax_db)) : 'N/A'; -// Table row counts +// Table row counts. Read-only + existence check: a plain `new SQLite3($nax_db)` +// with default flags would CREATE an empty app.db if the path is ever wrong/ +// missing (e.g. before first scan), and an unhandled open/query exception here +// would previously fatal-error this entire AJAX-loaded tab, not just this box. $tableSizesHTML = ""; -$db_info_conn = new SQLite3($nax_db); -$table_names_result = $db_info_conn->query("SELECT name FROM sqlite_master WHERE type='table'"); -while ($row = $table_names_result->fetchArray(SQLITE3_ASSOC)) { - $tableName = $row['name']; - $countResult = $db_info_conn->querySingle("SELECT COUNT(*) FROM $tableName"); - $tableSizesHTML = $tableSizesHTML . "$tableName ($countResult), "; +if (is_readable($nax_db)) { + try { + $db_info_conn = new SQLite3($nax_db, SQLITE3_OPEN_READONLY); + $table_names_result = $db_info_conn->query("SELECT name FROM sqlite_master WHERE type='table'"); + while ($row = $table_names_result->fetchArray(SQLITE3_ASSOC)) { + $tableName = $row['name']; + $countResult = $db_info_conn->querySingle("SELECT COUNT(*) FROM $tableName"); + $tableSizesHTML = $tableSizesHTML . "$tableName ($countResult), "; + } + $db_info_conn->close(); + } catch (Exception $e) { + $tableSizesHTML = ''; + } } -$db_info_conn->close(); echo '
diff --git a/server/__main__.py b/server/__main__.py index 3218927c3..112741ddf 100755 --- a/server/__main__.py +++ b/server/__main__.py @@ -156,7 +156,14 @@ def main(): # happens in this window and is the class of cost this history # exists to catch. Gated on MAINT_PERF_DAYS so a disabled install # pays no sampling cost at all (see scan/resource_history.py). - resource_history_enabled = int(get_setting_value("MAINT_PERF_DAYS", 30)) != 0 + # Defensive: an empty/corrupted setting value must disable this + # optional feature, not raise and kill the whole main loop - + # this sits outside the try/finally below, so an uncaught + # ValueError/TypeError here would have no safety net at all. + try: + resource_history_enabled = int(get_setting_value("MAINT_PERF_DAYS", 30)) != 0 + except (TypeError, ValueError): + resource_history_enabled = False resource_pre = get_process_cpu_times_and_io() if resource_history_enabled else None tick_start_monotonic = time.monotonic() tick_failed = False diff --git a/server/plugins/db_cleanup/script.py b/server/plugins/db_cleanup/script.py index ab6e5e232..1f34ba192 100755 --- a/server/plugins/db_cleanup/script.py +++ b/server/plugins/db_cleanup/script.py @@ -117,7 +117,10 @@ def cleanup_database( # no separate zero-check needed here: if collection is disabled via # MAINT_PERF_DAYS=0, the table is already empty) mylog("verbose", f"[{pluginName}] Resource_History: Delete all older than {str(MAINT_PERF_DAYS)} days (MAINT_PERF_DAYS setting)") - sql = f"""DELETE FROM Resource_History WHERE resDateTime <= date('now', '-{str(MAINT_PERF_DAYS)} day')""" + # datetime(), not date(): resDateTime carries a time component (timeNowUTC()), + # and a bare date() cutoff (midnight, no time) never compares <= against a + # same-day timestamped row, silently keeping the whole boundary day. + sql = f"""DELETE FROM Resource_History WHERE resDateTime <= datetime('now', '-{str(MAINT_PERF_DAYS)} day')""" mylog("verbose", [f"[{pluginName}] SQL : {sql}"]) cursor.execute(sql) mylog("verbose", [f"[{pluginName}] Resource_History deleted rows: {cursor.rowcount}"]) diff --git a/server/scan/resource_history.py b/server/scan/resource_history.py index 8a106fb5a..4df0e33ff 100644 --- a/server/scan/resource_history.py +++ b/server/scan/resource_history.py @@ -70,6 +70,16 @@ def insert_resource_history(db, pre, post, duration_ms, tick_failed=False): resIoReadBytes = post[1] - pre[1] resIoWriteBytes = post[2] - pre[2] + # Sampled independently of the INSERT's own try/except below - a psutil + # failure here must zero this one field, not get misreported as an + # "insert failed" and skip the whole row (CPU/IO were already computed + # successfully at this point). + try: + resRssMb = round(psutil.Process().memory_info().rss / (1024 * 1024), 2) + except (psutil.Error, AttributeError) as e: + mylog("verbose", [f"[resource_history] RSS sampling failed, using 0.0: {e}"]) + resRssMb = 0.0 + try: db.sql.execute( """ @@ -81,7 +91,7 @@ def insert_resource_history(db, pre, post, duration_ms, tick_failed=False): ( timeNowUTC(), resCpuPercent, - round(psutil.Process().memory_info().rss / (1024 * 1024), 2), + resRssMb, resIoReadBytes, resIoWriteBytes, duration_ms, diff --git a/test/db/test_db_cleanup.py b/test/db/test_db_cleanup.py index 8cd9479d8..39888ec73 100644 --- a/test/db/test_db_cleanup.py +++ b/test/db/test_db_cleanup.py @@ -185,13 +185,13 @@ def _seed_resource_history(cur, old_count: int, recent_count: int, days: int): for i in range(old_count): cur.execute( "INSERT INTO Resource_History (resDateTime, resCpuPercent) " - "VALUES (date('now', ?), 10.0)", + "VALUES (datetime('now', ?), 10.0)", (f"-{days + 1} day",), ) for i in range(recent_count): cur.execute( "INSERT INTO Resource_History (resDateTime, resCpuPercent) " - "VALUES (date('now'), 10.0)" + "VALUES (datetime('now'), 10.0)" ) @@ -199,7 +199,7 @@ def _run_resource_history_trim(cur, days: int) -> int: """Execute the exact DELETE used by db_cleanup and return rowcount.""" cur.execute( f"DELETE FROM Resource_History " - f"WHERE resDateTime <= date('now', '-{days} day')" + f"WHERE resDateTime <= datetime('now', '-{days} day')" ) return cur.rowcount @@ -234,6 +234,52 @@ class TestResourceHistoryTrim: assert _run_resource_history_trim(cur, days=30) == 0 + def test_date_cutoff_would_silently_under_delete_the_boundary_day(self): + """ + Regression, demonstrating the bug the datetime() fix closes: date() + truncates the cutoff to midnight (10-char "YYYY-MM-DD"), while + resDateTime always carries a time component (19-char + "YYYY-MM-DD HH:MM:SS", from timeNowUTC()). Since the bare date string + is a strict prefix of any same-day timestamp, plain string comparison + (SQLite has no typed DATE column here) means resDateTime <= date(...) + is FALSE for every row on the cutoff day, regardless of its time of + day - the whole boundary day silently survives a date() cutoff. + datetime() doesn't have this gap: both sides are 19-char timestamps. + """ + conn = sqlite3.connect(":memory:") + cur = conn.cursor() + date_cutoff = cur.execute("SELECT date('now', '-30 day')").fetchone()[0] + datetime_cutoff = cur.execute("SELECT datetime('now', '-30 day')").fetchone()[0] + + assert len(date_cutoff) == 10 # "YYYY-MM-DD" - no time component + assert len(datetime_cutoff) == 19 # "YYYY-MM-DD HH:MM:SS" + assert datetime_cutoff.startswith(date_cutoff) + + # A same-day resDateTime value (any time after midnight) sorts after + # the bare date cutoff, so it would never satisfy `<=` under date(). + same_day_timestamp = date_cutoff + " 08:00:00" + assert not (same_day_timestamp <= date_cutoff), ( + "date() cutoff must fail to catch a same-day timestamped row - " + "this is exactly the bug datetime() fixes" + ) + + def test_resource_history_trim_uses_datetime_not_date(self): + """ + Regression: assert script.py's actual DELETE uses datetime(), matching + the precision of resDateTime and the read-side range queries + (const.py's sql_resource_history_* use datetime() too) - a bare + date() cutoff would silently under-delete (see test above). + """ + INSTALL_PATH = os.getenv("NETALERTX_APP", "/app") + script_path = os.path.join( + INSTALL_PATH, "server", "plugins", "db_cleanup", "script.py" + ) + with open(script_path) as fh: + source = fh.read() + + expr = "DELETE FROM Resource_History WHERE resDateTime <= datetime('now', '-{str(MAINT_PERF_DAYS)} day')" + assert expr in source, "Resource_History DELETE must use datetime(), not date()" + # --------------------------------------------------------------------------- # ANALYZE tests diff --git a/test/scan/test_resource_history.py b/test/scan/test_resource_history.py index eb5b7a28d..e018c2488 100644 --- a/test/scan/test_resource_history.py +++ b/test/scan/test_resource_history.py @@ -11,6 +11,7 @@ import types from types import SimpleNamespace from unittest.mock import MagicMock +import psutil import pytest from server.scan import resource_history as rh @@ -155,6 +156,30 @@ def test_insert_failure_is_caught_and_logged(): rh.insert_resource_history(RaisingDB(), (0.0, 0, 0), (0.0, 0, 0), duration_ms=1000) +def test_rss_sampling_failure_does_not_skip_the_row(monkeypatch): + """ + Regression: a failing psutil RSS read must zero resRssMb, not get caught + by the INSERT's own except block and skip the whole row (CPU/IO were + already computed successfully by that point). + """ + db = _make_resource_history_db() + + def raising_process(): + raise psutil.AccessDenied() + + monkeypatch.setattr(rh.psutil, "Process", raising_process) + + rh.insert_resource_history(db, (0.0, 0, 0), (1.0, 100, 200), duration_ms=1000) + + row = db.sql.execute( + "SELECT resRssMb, resIoReadBytes, resIoWriteBytes FROM Resource_History" + ).fetchone() + assert row is not None, "Row must still be inserted despite the RSS sampling failure" + assert row[0] == 0.0 + assert row[1] == 100 # IO values, computed before the RSS read, are unaffected + assert row[2] == 200 + + # --------------------------------------------------------------------------- # __main__.py schedule-tick wiring: try/except/finally + MAINT_PERF_DAYS gate # --------------------------------------------------------------------------- @@ -327,6 +352,40 @@ def test_maint_perf_days_zero_disables_collection(monkeypatch, main_mod): assert row[0] == 0, "MAINT_PERF_DAYS=0 must produce zero Resource_History rows" +def test_maint_perf_days_non_numeric_disables_instead_of_crashing(monkeypatch, main_mod): + """ + Regression: an empty/corrupted MAINT_PERF_DAYS setting value must disable + the optional feature, not raise ValueError/TypeError and kill the whole + main loop - this int() call sits outside the tick's own try/except/finally, + so it has no other safety net. + """ + conn = make_db() + rh_upgrade_conn = conn.cursor() + from server.db.db_upgrade import ensure_Resource_History + ensure_Resource_History(rh_upgrade_conn) + conn.commit() + db = _FakeDB(conn) + + fake_pm = _FakePM(raise_on_schedule=False) + _common_patches(monkeypatch, main_mod, db, fake_pm) + # Simulate a corrupted/empty setting value instead of a real integer. + monkeypatch.setattr(main_mod, "get_setting_value", lambda key, default=None: "" if key == "MAINT_PERF_DAYS" else default) + monkeypatch.setattr(main_mod, "get_notifications", lambda db: {}) + monkeypatch.setattr(main_mod, "NotificationInstance", lambda db: _FakeNotificationInstance()) + monkeypatch.setattr(main_mod, "update_devices_names", lambda pm: None) + monkeypatch.setattr(main_mod, "WorkflowManager", _FakeWorkflowManager) + monkeypatch.setattr(main_mod, "UserEventsQueueInstance", lambda: SimpleNamespace( + has_update_devices=lambda: False + )) + + # Must reach the sentinel (i.e. complete the tick normally), not raise ValueError. + with pytest.raises(_StopTestLoop): + main_mod.main() + + row = conn.execute("SELECT COUNT(*) FROM Resource_History").fetchone() + assert row[0] == 0, "A non-numeric MAINT_PERF_DAYS must disable collection, not crash" + + def test_after_sample_precedes_insert_and_final_commit(monkeypatch, main_mod): """Self-measurement boundary: the 'after' sample and insert_resource_history() must both run before the tick's own later db.commitDB() calls, so the