From 4f7448b3c9b8e847f306b471f6a4ebc7759baade Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Wed, 30 Sep 2026 07:53:07 +1000 Subject: [PATCH 1/5] BE: NICs presence fixes orphan disconnect events for parent #1821 --- .claude/skills/scan-pipeline/SKILL.md | 2 +- .gemini/skills/scan-pipeline/SKILL.md | 2 +- .github/skills/scan-pipeline/SKILL.md | 2 +- server/scan/device_handling.py | 10 +- server/scan/presence.py | 64 +++++++++ server/scan/session_events.py | 11 +- test/scan/test_down_sleep_events.py | 191 ++++++++++++++++++++++++++ test/scan/test_presence_helper.py | 52 ++++++- test/scan/test_scan_presence.py | 28 ++++ 9 files changed, 351 insertions(+), 11 deletions(-) diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index 4d37074e9..d6f5cc969 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -49,7 +49,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index 40b1c181f..e5582292d 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -49,7 +49,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index f1d086ad3..5a64d9856 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -49,7 +49,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index 14aa4eb3f..1ac548bdc 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -9,7 +9,7 @@ from const import vendorsPath, vendorsPathNewest, sql_generateGuid, NULL_EQUIVAL from models.device_instance import DeviceInstance from scan.name_resolution import NameResolver from scan.device_heuristics import guess_icon, guess_type -from scan.presence import current_scan_presence_condition +from scan.presence import current_scan_presence_condition, nic_derived_presence_condition from db.db_helper import sanitize_SQL_input, list_to_where, safe_int from db.db_upgrade import PARENT_MAC_SENTINELS from db.authoritative_handler import ( @@ -229,7 +229,10 @@ def update_devLastConnection_from_CurrentScan(db): identity/inventory data (scanPresence = 0) must not make an offline device look recently connected. Same predicate as update_presence_from_CurrentScan(); found missing this check during - review of a shipped commit - see scan-pipeline-hardening.md. + review of a shipped commit - see scan-pipeline-hardening.md. Also + advances devLastConnection for a parent device whose presence is + NIC-derived this cycle (nic_derived_presence_condition()) - it has no + direct CurrentScan row of its own, but is not actually offline. """ sql = db.sql startTime = timeNowUTC() @@ -238,7 +241,8 @@ def update_devLastConnection_from_CurrentScan(db): sql.execute(f""" UPDATE Devices SET devLastConnection = '{startTime}' - WHERE {current_scan_presence_condition("devMac")} + WHERE ({current_scan_presence_condition("devMac")} + OR {nic_derived_presence_condition("devMac")}) """) diff --git a/server/scan/presence.py b/server/scan/presence.py index 1d45743a0..a2b4a26ec 100644 --- a/server/scan/presence.py +++ b/server/scan/presence.py @@ -40,3 +40,67 @@ def current_scan_presence_condition(mac_column: str) -> str: SELECT 1 FROM CurrentScan AS presence_scan WHERE presence_scan.scanMac = {mac_column} AND presence_scan.scanPresence = 1 )""" + + +def nic_derived_presence_condition(mac_column: str) -> str: + """SQL fragment answering exactly: 'would NIC reconciliation + (update_devPresentLastScan_based_on_nics(), step 7 of process_scan()) + consider mac_column present, based on this cycle's CurrentScan rows for + its NIC children (devParentRelType = 'nic') and its own devReqNicsOnline + (ANY vs ALL)?' It intentionally does not read or write + Devices.devPresentLastScan - see nic-parent-orphan-disconnect-events.md's + Invariant for why that's still equivalent to step 7's own answer within + the same cycle (step 6 sets every device's devPresentLastScan, NIC + children included, to exactly current_scan_presence_condition()'s value + for this cycle, before step 7 ever reads it). Not a general-purpose + presence predicate - it answers this one question, nothing broader. + + Deliberately NOT a replacement for current_scan_presence_condition() - + composed with it via OR at each call site (insert_events()'s Device + Down/Disconnected queries, update_devLastConnection_from_CurrentScan()). + Deliberately does NOT replicate update_devPresentLastScan_based_on_nics()'s + "parent was directly detected this scan" carve-out: that carve-out is + redundant here, because a directly-detected parent already satisfies + current_scan_presence_condition() on its own, and the two are OR'd. + + mac_column must be a trusted, hardcoded SQL column/table.column reference + written by NetAlertX code - same constraint as + current_scan_presence_condition(), enforced the same way. + """ + if not _SQL_IDENTIFIER_RE.match(mac_column): + raise ValueError(f"mac_column must be a plain identifier, got: {mac_column!r}") + if mac_column == "presence_scan" or mac_column.startswith("presence_scan."): + raise ValueError( + f"mac_column must not reference presence_scan - that's " + f"current_scan_presence_condition()'s own internal subquery " + f"alias, got: {mac_column!r}" + ) + + return f"""EXISTS ( + SELECT 1 FROM Devices AS nic_parent + WHERE nic_parent.devMac = {mac_column} + AND ( + ( + IFNULL(CAST(nic_parent.devReqNicsOnline AS TEXT), '') = '1' + AND EXISTS (SELECT 1 FROM Devices AS any_nic + WHERE any_nic.devParentMAC = nic_parent.devMac + AND any_nic.devParentRelType = 'nic') + AND NOT EXISTS ( + SELECT 1 FROM Devices AS nic + WHERE nic.devParentMAC = nic_parent.devMac + AND nic.devParentRelType = 'nic' + AND NOT {current_scan_presence_condition("nic.devMac")} + ) + ) + OR + ( + IFNULL(CAST(nic_parent.devReqNicsOnline AS TEXT), '') != '1' + AND EXISTS ( + SELECT 1 FROM Devices AS nic + WHERE nic.devParentMAC = nic_parent.devMac + AND nic.devParentRelType = 'nic' + AND {current_scan_presence_condition("nic.devMac")} + ) + ) + ) + )""" diff --git a/server/scan/session_events.py b/server/scan/session_events.py index edca2fd37..5087d2270 100755 --- a/server/scan/session_events.py +++ b/server/scan/session_events.py @@ -14,7 +14,7 @@ from scan.device_handling import ( update_presence_from_CurrentScan ) from helper import get_setting_value -from scan.presence import current_scan_presence_condition +from scan.presence import current_scan_presence_condition, nic_derived_presence_condition from db.db_helper import print_table_schema from utils.datetime_utils import timeNowUTC from logger import mylog, Logger @@ -191,7 +191,8 @@ def insert_events(db): AND devCanSleep = 0 AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT {current_scan_presence_condition("devMac")} """) + AND NOT ({current_scan_presence_condition("devMac")} + OR {nic_derived_presence_condition("devMac")}) """) # Check device down – sleeping devices whose sleep window has expired mylog("debug", "[Events] - 1b - Devices down (sleep expired)") @@ -205,7 +206,8 @@ def insert_events(db): AND devIsSleeping = 0 AND devPresentLastScan = 0 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT {current_scan_presence_condition("devMac")} + AND NOT ({current_scan_presence_condition("devMac")} + OR {nic_derived_presence_condition("devMac")}) AND NOT EXISTS (SELECT 1 FROM Events WHERE eveMac = devMac AND eveEventType = 'Device Down' @@ -270,7 +272,8 @@ def insert_events(db): WHERE devAlertDown = 0 AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT {current_scan_presence_condition("devMac")} """) + AND NOT ({current_scan_presence_condition("devMac")} + OR {nic_derived_presence_condition("devMac")}) """) # Check IP Changed mylog("debug", "[Events] - 4 - IP Changes") diff --git a/test/scan/test_down_sleep_events.py b/test/scan/test_down_sleep_events.py index b1a54d952..9253b5f2d 100644 --- a/test/scan/test_down_sleep_events.py +++ b/test/scan/test_down_sleep_events.py @@ -33,6 +33,10 @@ from db_test_helpers import ( # noqa: E402 minutes_ago as _minutes_ago, insert_device as _insert_device, down_event_macs as _down_event_macs, + make_device_dict as _make_device_dict, + insert_device_from_dict as _insert_device_from_dict, + make_current_scan_dict as _make_current_scan_dict, + insert_current_scan_row_from_dict as _insert_current_scan_row_from_dict, DummyDB, ) @@ -563,3 +567,190 @@ class TestInsertEventsForceOnline: assert "ff:00:00:00:00:06" in _down_event_macs(cur), ( "forced-offline device must still generate 'Device Down' when absent" ) + + +# --------------------------------------------------------------------------- +# Layer 1d: insert_events() — NIC-derived presence suppression +# +# nic-parent-orphan-disconnect-events PRD: a parent device with NIC children +# (devParentRelType='nic') must not get Device Down/Disconnected events just +# because it has no direct CurrentScan row of its own, as long as its NICs +# satisfy devReqNicsOnline (ANY/ALL) against CurrentScan this cycle. Test +# names/setup follow that PRD's Tests section parity matrix directly. +# --------------------------------------------------------------------------- + +def _setup_parent_with_nics(conn, parent_mac, nic_specs, req_nics_online=0, + parent_present_last_scan=1, alert_down=1, + parent_has_own_scan_row=False): + """ + Insert a parent device plus its NIC children, and CurrentScan rows for + whichever NICs (and optionally the parent) are present this cycle. + + nic_specs: list of (mac, present_this_cycle) tuples. A NIC not present + this cycle gets no CurrentScan row at all (chosen as the PRD's single + primary representation of "absent", not the scanPresence=0 variant). + """ + _insert_device_from_dict(conn, _make_device_dict( + parent_mac, + devPresentLastScan=parent_present_last_scan, + devAlertDown=alert_down, + devParentMAC="", + devParentRelType="", + devReqNicsOnline=req_nics_online, + )) + for nic_mac, present in nic_specs: + _insert_device_from_dict(conn, _make_device_dict( + nic_mac, + devPresentLastScan=1 if present else 0, + devAlertDown=0, + devParentMAC=parent_mac, + devParentRelType="nic", + devReqNicsOnline=0, + )) + if present: + _insert_current_scan_row_from_dict(conn, _make_current_scan_dict(nic_mac, scanPresence=1)) + if parent_has_own_scan_row: + _insert_current_scan_row_from_dict(conn, _make_current_scan_dict(parent_mac, scanPresence=1)) + conn.commit() + + +class TestInsertEventsNicDerivedPresence: + + def test_device_down_suppressed_one_nic_present(self): + conn = _make_db() + _setup_parent_with_nics(conn, "aa:11:00:00:00:01", [("bb:11:00:00:00:01", True)]) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:01" not in _down_event_macs(conn.cursor()), ( + "parent with a present NIC child must not get a spurious 'Device Down' event" + ) + + def test_disconnected_suppressed_one_nic_present(self): + conn = _make_db() + _setup_parent_with_nics(conn, "aa:11:00:00:00:02", [("bb:11:00:00:00:02", True)], + alert_down=0) + + insert_events(DummyDB(conn)) + + cur = conn.cursor() + cur.execute("SELECT eveEventType FROM Events WHERE eveMac = ?", ("aa:11:00:00:00:02",)) + event_types = [r["eveEventType"] for r in cur.fetchall()] + assert "Disconnected" not in event_types, ( + f"expected no 'Disconnected' event, got event types: {event_types}" + ) + + def test_still_fires_when_nic_genuinely_absent(self): + """Regression guard: this PRD must not silently suppress a real down + transition - no CurrentScan row for the parent or its only NIC.""" + conn = _make_db() + _setup_parent_with_nics(conn, "aa:11:00:00:00:03", [("bb:11:00:00:00:03", False)]) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:03" in _down_event_macs(conn.cursor()) + + def test_all_mode_one_nic_missing_still_fires(self): + """Mirrors test_req_all_mode_partial_nics_does_not_raise_absent_parent + in test_nic_presence.py - same ANY/ALL contract, different pipeline point.""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:11:00:00:00:04", + [("bb:11:00:00:00:04", True), ("cc:11:00:00:00:04", False)], + req_nics_online=1, + ) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:04" in _down_event_macs(conn.cursor()) + + def test_all_mode_all_nics_present_suppresses(self): + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:11:00:00:00:05", + [("bb:11:00:00:00:05", True), ("cc:11:00:00:00:05", True)], + req_nics_online=1, + ) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:05" not in _down_event_macs(conn.cursor()) + + def test_any_mode_explicit_zero_one_nic_present_suppresses(self): + """devReqNicsOnline=0 explicitly (not NULL, not 1) - parity with the + real Python rule (str(device.get('devReqNicsOnline')) == '1' means + everything else, including 0, is ANY mode).""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:11:00:00:00:06", [("bb:11:00:00:00:06", True)], req_nics_online=0, + ) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:06" not in _down_event_macs(conn.cursor()) + + def test_null_req_nics_online_one_nic_present_suppresses(self): + """NULL defaults to ANY mode (str(None) == '1' is False in Python).""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:11:00:00:00:07", [("bb:11:00:00:00:07", True)], req_nics_online=None, + ) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:07" not in _down_event_macs(conn.cursor()) + + def test_null_req_nics_online_nic_absent_still_fires(self): + """Negative NULL parity - the complement of the case above. Guards + against a future change silently turning NULL into 'always present' + or 'ALL' instead of ANY.""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:11:00:00:00:08", [("bb:11:00:00:00:08", False)], req_nics_online=None, + ) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:08" in _down_event_macs(conn.cursor()) + + def test_any_mode_two_nics_one_present_suppresses(self): + """Proves ANY semantics specifically (not 'all NICs present', which + the ALL-mode tests above can't distinguish on their own) - two NICs, + only one present, devReqNicsOnline=0.""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:11:00:00:00:09", + [("bb:11:00:00:00:09", True), ("cc:11:00:00:00:09", False)], + req_nics_online=0, + ) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:09" not in _down_event_macs(conn.cursor()) + + def test_direct_parent_presence_overrides_regardless_of_nic_state(self): + """The new OR-composed helper must not interfere with the + pre-existing path: a parent with its own presence-asserting + CurrentScan row this cycle is suppressed via + current_scan_presence_condition() alone, even with every NIC absent.""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:11:00:00:00:10", [("bb:11:00:00:00:10", False)], + parent_has_own_scan_row=True, + ) + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:10" not in _down_event_macs(conn.cursor()) + + def test_no_nic_children_unaffected(self): + """Plain device, no devParentMAC - existing behavior (event fires + exactly like it did before this PRD) must be untouched.""" + conn = _make_db() + cur = conn.cursor() + _insert_device(cur, "aa:11:00:00:00:11", alert_down=1, present_last_scan=1) + conn.commit() + + insert_events(DummyDB(conn)) + + assert "aa:11:00:00:00:11" in _down_event_macs(conn.cursor()) diff --git a/test/scan/test_presence_helper.py b/test/scan/test_presence_helper.py index cf906b524..a8cafa453 100644 --- a/test/scan/test_presence_helper.py +++ b/test/scan/test_presence_helper.py @@ -32,7 +32,7 @@ import pytest sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server")) -from scan.presence import current_scan_presence_condition # noqa: E402 +from scan.presence import current_scan_presence_condition, nic_derived_presence_condition # noqa: E402 from scan import device_handling # noqa: E402 from scan import session_events # noqa: E402 @@ -68,6 +68,35 @@ class TestHelperCorrectness: current_scan_presence_condition(bad_value) +class TestNicDerivedHelperCorrectness: + """nic_derived_presence_condition() - see nic-parent-orphan-disconnect-events + PRD Design §1. Same trust-boundary checks as current_scan_presence_condition().""" + + def test_returns_expected_sql_fragment(self): + result = nic_derived_presence_condition("devMac") + assert "EXISTS (" in result + assert "SELECT 1 FROM Devices AS nic_parent" in result + assert "nic_parent.devMac = devMac" in result + assert "devReqNicsOnline" in result + + @pytest.mark.parametrize("bad_value", [ + "devMac; DROP TABLE Devices--", + "devMac OR 1=1", + "'; DELETE FROM Devices; --", + "devMac)", + "", + "123devMac", + ]) + def test_rejects_non_identifier_input(self, bad_value): + with pytest.raises(ValueError): + nic_derived_presence_condition(bad_value) + + @pytest.mark.parametrize("bad_value", ["presence_scan", "presence_scan.scanMac"]) + def test_rejects_presence_scan_qualifier(self, bad_value): + with pytest.raises(ValueError): + nic_derived_presence_condition(bad_value) + + class TestQualifiedColumnExecutesCorrectly: """Executes the fragment, not just checks the generated SQL text - proves a qualified mac_column ("CurrentScan.scanMac") still discriminates @@ -123,3 +152,24 @@ class TestConsumersCallTheHelper: MIN(scanLastIP) aggregation on purpose (see module docstring), so this asserts >= 3, not == 4.""" assert _call_count(session_events.insert_events) >= 3 + + +class TestConsumersCallTheNicHelper: + """Guards the four sites nic_derived_presence_condition() was OR-composed + into (nic-parent-orphan-disconnect-events PRD) - both Device Down queries, + Disconnected, and update_devLastConnection_from_CurrentScan(). New + Connections/IP Changed are deliberately not touched - see that PRD's + Non-goals.""" + + def test_update_dev_last_connection_calls_nic_helper_once(self): + assert _call_count( + device_handling.update_devLastConnection_from_CurrentScan, + target_name="nic_derived_presence_condition", + ) == 1 + + def test_insert_events_calls_nic_helper_exactly_three_times(self): + """Both Device Down queries + Disconnected - not New Connections/IP Changed.""" + assert _call_count( + session_events.insert_events, + target_name="nic_derived_presence_condition", + ) == 3 diff --git a/test/scan/test_scan_presence.py b/test/scan/test_scan_presence.py index 21dfddbd2..cd64f3fdb 100644 --- a/test/scan/test_scan_presence.py +++ b/test/scan/test_scan_presence.py @@ -20,6 +20,8 @@ from db_test_helpers import ( # noqa: E402 make_current_scan_dict, insert_current_scan_row_from_dict, insert_device, + make_device_dict, + insert_device_from_dict, minutes_ago, DummyDB, down_event_macs, @@ -127,6 +129,32 @@ class TestDevLastConnectionRespectsPresence: ).fetchone() assert row["devLastConnection"] != "2020-01-01 00:00:00" + def test_nic_derived_presence_still_bumps_last_connection(self): + """nic-parent-orphan-disconnect-events PRD: a parent with no direct + CurrentScan row, but a present NIC child, must still advance + devLastConnection - it is not actually offline.""" + conn = make_db() + parent_mac = "aa:22:00:00:00:01" + nic_mac = "bb:22:00:00:00:01" + insert_device_from_dict(conn, make_device_dict( + parent_mac, devLastConnection="2020-01-01 00:00:00", + devParentMAC="", devParentRelType="", devReqNicsOnline=0, + )) + insert_device_from_dict(conn, make_device_dict( + nic_mac, devParentMAC=parent_mac, devParentRelType="nic", devReqNicsOnline=0, + )) + insert_current_scan_row_from_dict( + conn, make_current_scan_dict(nic_mac, scanPresence=1) + ) + db = DummyDB(conn) + + device_handling.update_devLastConnection_from_CurrentScan(db) + + row = conn.execute( + "SELECT devLastConnection FROM Devices WHERE devMac = ?", (parent_mac,) + ).fetchone() + assert row["devLastConnection"] != "2020-01-01 00:00:00" + class TestNewConnectionsRespectsPresence: """insert_events()'s New Connections query must not fire Connected for a From b18fc0688a7b6e672d37e8e3d424ce54ccc2dd00 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Wed, 30 Sep 2026 21:33:33 +1000 Subject: [PATCH 2/5] BE: Fix: NIC-Covered Parent Devices Miss Connected/Down Reconnected Events #1821 --- .claude/skills/scan-pipeline/SKILL.md | 5 +- .gemini/skills/scan-pipeline/SKILL.md | 5 +- .github/skills/scan-pipeline/SKILL.md | 5 +- server/scan/session_events.py | 68 ++++++- test/scan/test_down_sleep_events.py | 282 +++++++++++++++++++++++++- test/scan/test_presence_helper.py | 50 +++-- 6 files changed, 389 insertions(+), 26 deletions(-) diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index d6f5cc969..c29b85cc2 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -20,7 +20,7 @@ Covers what happens after a plugin's rows land in `CurrentScan`: presence comput ## Key views - **`LatestDeviceScan`** (`server/db/db_upgrade.py`) — `Devices` LEFT JOIN'd to the most recent `CurrentScan` row per `(scanMac, scanSourcePlugin)` pair, via `ROW_NUMBER() OVER (PARTITION BY scanMac, scanSourcePlugin ...)`. `update_devices_data_from_scan()` loops over `DISTINCT scanSourcePlugin` and re-queries this view once per plugin: when two plugins report the same device in one cycle, each contribution is evaluated separately, per field, through the authority mechanism below — they are not merged into one row first. -- **`LatestEventsPerMAC`** — most recent Event per MAC, joined to `Devices` and `CurrentScan`. The "New Connections" query in `insert_events()` uses it to decide whether a device was previously down (→ `Down Reconnected`) or new (→ `Connected`). +- **`LatestEventsPerMAC`** — most recent Event per MAC, joined to `Devices` and `CurrentScan` - via an **inner** join on `CurrentScan`, so it silently returns no row at all for a MAC not in `CurrentScan` this cycle, regardless of that MAC's real event history (see Gotcha 7). The "New Connections" query in `insert_events()` uses it to decide whether a device was previously down (→ `Down Reconnected`) or new (→ `Connected`). - **`Convert_Events_to_Sessions`** — defines "is this device's session still open." There is no `close_session()` function anywhere in this codebase. A session closes as an emergent property: `pair_sessions_events()` sets `evePairEventRowid` on a `New Device`/`Connected`/`Down Reconnected` Event to point at the next `Disconnected`/`Device Down` Event for that MAC; this view sets `sesStillConnected = 1` exactly when that pairing is `NULL`. To close a session, insert the right `Events` row — never mutate `Sessions` directly (the one exception is `create_new_devices()`'s reconnect-insert, in the call order below). - **`DevicesView`** — adds computed `devIsSleeping`/`devFlapping`/`devStatus` on top of `Devices`. The UI and `insertOnlineHistory()` read presence from this, not the raw `Devices` table. @@ -49,12 +49,13 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at five of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`, and a fifth query that fires the NIC-derived `Connected`/`Down Reconnected` event `insert_events()`'s mainline "New Connections" query can't produce - see Gotcha 7 for why that query can't just reuse `LatestEventsPerMAC`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. +7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. ## When to read this vs. other docs/skills diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index e5582292d..da4c06136 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -20,7 +20,7 @@ Covers what happens after a plugin's rows land in `CurrentScan`: presence comput ## Key views - **`LatestDeviceScan`** (`server/db/db_upgrade.py`) — `Devices` LEFT JOIN'd to the most recent `CurrentScan` row per `(scanMac, scanSourcePlugin)` pair, via `ROW_NUMBER() OVER (PARTITION BY scanMac, scanSourcePlugin ...)`. `update_devices_data_from_scan()` loops over `DISTINCT scanSourcePlugin` and re-queries this view once per plugin: when two plugins report the same device in one cycle, each contribution is evaluated separately, per field, through the authority mechanism below — they are not merged into one row first. -- **`LatestEventsPerMAC`** — most recent Event per MAC, joined to `Devices` and `CurrentScan`. The "New Connections" query in `insert_events()` uses it to decide whether a device was previously down (→ `Down Reconnected`) or new (→ `Connected`). +- **`LatestEventsPerMAC`** — most recent Event per MAC, joined to `Devices` and `CurrentScan` - via an **inner** join on `CurrentScan`, so it silently returns no row at all for a MAC not in `CurrentScan` this cycle, regardless of that MAC's real event history (see Gotcha 7). The "New Connections" query in `insert_events()` uses it to decide whether a device was previously down (→ `Down Reconnected`) or new (→ `Connected`). - **`Convert_Events_to_Sessions`** — defines "is this device's session still open." There is no `close_session()` function anywhere in this codebase. A session closes as an emergent property: `pair_sessions_events()` sets `evePairEventRowid` on a `New Device`/`Connected`/`Down Reconnected` Event to point at the next `Disconnected`/`Device Down` Event for that MAC; this view sets `sesStillConnected = 1` exactly when that pairing is `NULL`. To close a session, insert the right `Events` row — never mutate `Sessions` directly (the one exception is `create_new_devices()`'s reconnect-insert, in the call order below). - **`DevicesView`** — adds computed `devIsSleeping`/`devFlapping`/`devStatus` on top of `Devices`. The UI and `insertOnlineHistory()` read presence from this, not the raw `Devices` table. @@ -49,12 +49,13 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at five of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`, and a fifth query that fires the NIC-derived `Connected`/`Down Reconnected` event `insert_events()`'s mainline "New Connections" query can't produce - see Gotcha 7 for why that query can't just reuse `LatestEventsPerMAC`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. +7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. ## When to read this vs. other docs/skills diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index 5a64d9856..f0d3b00c6 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -20,7 +20,7 @@ Covers what happens after a plugin's rows land in `CurrentScan`: presence comput ## Key views - **`LatestDeviceScan`** (`server/db/db_upgrade.py`) — `Devices` LEFT JOIN'd to the most recent `CurrentScan` row per `(scanMac, scanSourcePlugin)` pair, via `ROW_NUMBER() OVER (PARTITION BY scanMac, scanSourcePlugin ...)`. `update_devices_data_from_scan()` loops over `DISTINCT scanSourcePlugin` and re-queries this view once per plugin: when two plugins report the same device in one cycle, each contribution is evaluated separately, per field, through the authority mechanism below — they are not merged into one row first. -- **`LatestEventsPerMAC`** — most recent Event per MAC, joined to `Devices` and `CurrentScan`. The "New Connections" query in `insert_events()` uses it to decide whether a device was previously down (→ `Down Reconnected`) or new (→ `Connected`). +- **`LatestEventsPerMAC`** — most recent Event per MAC, joined to `Devices` and `CurrentScan` - via an **inner** join on `CurrentScan`, so it silently returns no row at all for a MAC not in `CurrentScan` this cycle, regardless of that MAC's real event history (see Gotcha 7). The "New Connections" query in `insert_events()` uses it to decide whether a device was previously down (→ `Down Reconnected`) or new (→ `Connected`). - **`Convert_Events_to_Sessions`** — defines "is this device's session still open." There is no `close_session()` function anywhere in this codebase. A session closes as an emergent property: `pair_sessions_events()` sets `evePairEventRowid` on a `New Device`/`Connected`/`Down Reconnected` Event to point at the next `Disconnected`/`Device Down` Event for that MAC; this view sets `sesStillConnected = 1` exactly when that pairing is `NULL`. To close a session, insert the right `Events` row — never mutate `Sessions` directly (the one exception is `create_new_devices()`'s reconnect-insert, in the call order below). - **`DevicesView`** — adds computed `devIsSleeping`/`devFlapping`/`devStatus` on top of `Devices`. The UI and `insertOnlineHistory()` read presence from this, not the raw `Devices` table. @@ -49,12 +49,13 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at five of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`, and a fifth query that fires the NIC-derived `Connected`/`Down Reconnected` event `insert_events()`'s mainline "New Connections" query can't produce - see Gotcha 7 for why that query can't just reuse `LatestEventsPerMAC`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. +7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. ## When to read this vs. other docs/skills diff --git a/server/scan/session_events.py b/server/scan/session_events.py index 5087d2270..8de0ac97f 100755 --- a/server/scan/session_events.py +++ b/server/scan/session_events.py @@ -27,6 +27,24 @@ from const import NULL_EQUIVALENTS_SQL _SQL_NOT_FORCED_ONLINE = "LOWER(COALESCE(devForceStatus, '')) != 'online'" +def _connect_event_type_case(event_type_expr, pending_expr): + """SQL CASE fragment shared by every insert_events() query that decides + Connected vs. Down Reconnected: 'Down Reconnected' iff the referenced + prior event was an unacknowledged Device Down, else 'Connected'. + + event_type_expr/pending_expr are trusted, hardcoded SQL expressions (a + column reference or a scalar subquery) evaluating to the prior event's + eveEventType/evePendingAlertEmail - same trust-boundary contract as + current_scan_presence_condition()'s mac_column, not parameterized SQL. + Centralised here (like _SQL_NOT_FORCED_ONLINE above) so every connect- + side query classifies a reconnect the same way. + """ + return f"""CASE + WHEN {event_type_expr} = 'Device Down' AND {pending_expr} = 0 THEN 'Down Reconnected' + ELSE 'Connected' + END""" + + # Make sure log level is initialized correctly Logger(get_setting_value("LOG_LEVEL")) @@ -234,10 +252,7 @@ def insert_events(db): eveEventType, eveAdditionalInfo, evePendingAlertEmail) SELECT present_agg.scanMac, present_agg.scanLastIP, '{startTime}', - CASE - WHEN last_event.eveEventType = 'Device Down' and last_event.evePendingAlertEmail = 0 THEN 'Down Reconnected' - ELSE 'Connected' - END, + {_connect_event_type_case("last_event.eveEventType", "last_event.evePendingAlertEmail")}, '', CASE WHEN quiet_agg.scanQuiet = 1 THEN 0 ELSE 1 END FROM ( @@ -261,6 +276,51 @@ def insert_events(db): ) """) + # Check NIC-derived New Connections / Down Reconnections - a parent with + # no direct CurrentScan row of its own this cycle, but whose NIC children + # satisfy nic_derived_presence_condition(), gets the Connected/Down + # Reconnected event the query above can't produce for it (its + # present_agg is built from CurrentScan, which this parent has no row + # in). + # + # Deliberately NOT via LatestEventsPerMAC (used by the query above): + # that view INNER JOINs CurrentScan, so it returns no row at all for a + # MAC with no CurrentScan row this cycle - exactly every MAC this query + # targets - which would make the Down Reconnected branch silently + # unreachable. The two correlated subqueries below read Events directly + # instead, sidestepping the gap entirely - no COALESCE needed for the + # "no prior event at all" case either: _connect_event_type_case()'s own + # ELSE branch already resolves to 'Connected' when both subqueries + # return NULL (NULL = 'Device Down' is NULL/falsy, same as the query + # above already relies on for a brand-new MAC's LEFT JOIN miss). + mylog("debug", "[Events] - 2b - NIC-derived New Connections") + _last_event_type = """(SELECT eveEventType FROM Events + WHERE eveMac = nic_parent.devMac + ORDER BY eveDateTime DESC LIMIT 1)""" + _last_event_pending = """(SELECT evePendingAlertEmail FROM Events + WHERE eveMac = nic_parent.devMac + ORDER BY eveDateTime DESC LIMIT 1)""" + sql.execute(f"""INSERT OR IGNORE INTO Events (eveMac, eveIp, eveDateTime, + eveEventType, eveAdditionalInfo, evePendingAlertEmail) + SELECT nic_parent.devMac, nic_parent.devLastIP, '{startTime}', + {_connect_event_type_case(_last_event_type, _last_event_pending)}, + '', + CASE WHEN EXISTS ( + SELECT 1 FROM Devices AS nic + WHERE nic.devParentMAC = nic_parent.devMac + AND nic.devParentRelType = 'nic' + AND {current_scan_presence_condition("nic.devMac")} + AND EXISTS (SELECT 1 FROM CurrentScan AS quiet_scan + WHERE quiet_scan.scanMac = nic.devMac + AND quiet_scan.scanNotificationMode = 'quiet') + ) THEN 0 ELSE 1 END + FROM Devices AS nic_parent + WHERE IFNULL(nic_parent.devParentRelType, '') != 'nic' + AND nic_parent.devPresentLastScan = 0 + AND NOT {current_scan_presence_condition("nic_parent.devMac")} + AND {nic_derived_presence_condition("nic_parent.devMac")} + """) + # Check disconnections mylog("debug", "[Events] - 3 - Disconnections") sql.execute(f"""INSERT OR IGNORE INTO Events (eveMac, eveIp, eveDateTime, diff --git a/test/scan/test_down_sleep_events.py b/test/scan/test_down_sleep_events.py index 9253b5f2d..f3b0b89ad 100644 --- a/test/scan/test_down_sleep_events.py +++ b/test/scan/test_down_sleep_events.py @@ -41,7 +41,7 @@ from db_test_helpers import ( # noqa: E402 ) # server/ is already on sys.path after db_test_helpers import -from scan.session_events import insert_events # noqa: E402 +from scan.session_events import insert_events, pair_sessions_events # noqa: E402 # --------------------------------------------------------------------------- @@ -754,3 +754,283 @@ class TestInsertEventsNicDerivedPresence: insert_events(DummyDB(conn)) assert "aa:11:00:00:00:11" in _down_event_macs(conn.cursor()) + + +# --------------------------------------------------------------------------- +# Layer 1e: insert_events() — NIC-derived reconnect (Connected/Down Reconnected) +# +# nic-parent-reconnect-events PRD: a NIC-covered parent whose presence is +# restored by NIC coverage must get a Connected/Down Reconnected event - the +# mainline "New Connections" query can't produce one for it (its present_agg +# is built from CurrentScan, which such a parent has no row in this cycle). +# --------------------------------------------------------------------------- + +def _event_types_for(conn, mac): + cur = conn.cursor() + cur.execute("SELECT eveEventType FROM Events WHERE eveMac = ?", (mac,)) + return [r["eveEventType"] for r in cur.fetchall()] + + +class TestInsertEventsNicDerivedReconnect: + + def test_reconnect_fires_connected_no_prior_events(self): + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:22:00:00:00:01", [("bb:22:00:00:00:01", True)], + parent_present_last_scan=0, + ) + + insert_events(DummyDB(conn)) + + assert "Connected" in _event_types_for(conn, "aa:22:00:00:00:01") + + def test_reconnect_after_device_down_fires_down_reconnected(self): + """The specific case the LatestEventsPerMAC INNER-JOIN-on-CurrentScan + gap would have silently broken - see Design's second finding.""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:22:00:00:00:02", [("bb:22:00:00:00:02", True)], + parent_present_last_scan=0, + ) + cur = conn.cursor() + cur.execute( + "INSERT INTO Events (eveMac, eveIp, eveDateTime, eveEventType, " + "eveAdditionalInfo, evePendingAlertEmail) VALUES (?, '1.2.3.4', " + "?, 'Device Down', '', 0)", + ("aa:22:00:00:00:02", _minutes_ago(10)), + ) + conn.commit() + + insert_events(DummyDB(conn)) + + event_types = _event_types_for(conn, "aa:22:00:00:00:02") + assert "Down Reconnected" in event_types + assert "Connected" not in event_types + + def test_no_refire_while_already_nic_present(self): + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:22:00:00:00:03", [("bb:22:00:00:00:03", True)], + parent_present_last_scan=1, + ) + + insert_events(DummyDB(conn)) + + assert _event_types_for(conn, "aa:22:00:00:00:03") == [] + + def test_no_double_fire_when_parent_has_direct_presence_too(self): + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:22:00:00:00:04", [("bb:22:00:00:00:04", True)], + parent_present_last_scan=0, parent_has_own_scan_row=True, + ) + + insert_events(DummyDB(conn)) + + event_types = _event_types_for(conn, "aa:22:00:00:00:04") + assert event_types.count("Connected") == 1, ( + f"expected exactly one Connected event, got: {event_types}" + ) + + def test_dev_last_ip_used_not_nic_ip(self): + conn = _make_db() + parent_mac = "aa:22:00:00:00:05" + nic_mac = "bb:22:00:00:00:05" + _insert_device_from_dict(conn, _make_device_dict( + parent_mac, devPresentLastScan=0, devAlertDown=1, + devLastIP="10.0.0.99", devParentMAC="", devParentRelType="", + devReqNicsOnline=0, + )) + _insert_device_from_dict(conn, _make_device_dict( + nic_mac, devPresentLastScan=1, devAlertDown=0, + devParentMAC=parent_mac, devParentRelType="nic", devReqNicsOnline=0, + )) + _insert_current_scan_row_from_dict( + conn, _make_current_scan_dict(nic_mac, scanPresence=1, scanLastIP="192.168.5.5") + ) + conn.commit() + + insert_events(DummyDB(conn)) + + cur = conn.cursor() + cur.execute( + "SELECT eveIp FROM Events WHERE eveMac = ? AND eveEventType = 'Connected'", + (parent_mac,), + ) + assert cur.fetchone()["eveIp"] == "10.0.0.99" + + def test_quiet_mode_inheritance_from_present_nic(self): + conn = _make_db() + parent_mac = "aa:22:00:00:00:06" + nic_mac = "bb:22:00:00:00:06" + _setup_parent_with_nics(conn, parent_mac, [(nic_mac, True)], parent_present_last_scan=0) + cur = conn.cursor() + cur.execute("UPDATE CurrentScan SET scanNotificationMode = 'quiet' WHERE scanMac = ?", (nic_mac,)) + conn.commit() + + insert_events(DummyDB(conn)) + + cur.execute( + "SELECT evePendingAlertEmail FROM Events WHERE eveMac = ? AND eveEventType = 'Connected'", + (parent_mac,), + ) + assert cur.fetchone()["evePendingAlertEmail"] == 0 + + def test_quiet_mode_requires_presence(self): + """Locks in Open issue 1's decision: an absent NIC's quiet preference + must not suppress notification for a reconnection it didn't contribute to.""" + conn = _make_db() + parent_mac = "aa:22:00:00:00:07" + present_nic = "bb:22:00:00:00:07" + absent_nic = "cc:22:00:00:00:07" + _setup_parent_with_nics( + conn, parent_mac, [(present_nic, True), (absent_nic, False)], + parent_present_last_scan=0, req_nics_online=0, + ) + # Stale/inventory-only quiet row for the absent NIC - must not count. + _insert_current_scan_row_from_dict( + conn, _make_current_scan_dict(absent_nic, scanPresence=0, scanNotificationMode="quiet") + ) + conn.commit() + + insert_events(DummyDB(conn)) + + cur = conn.cursor() + cur.execute( + "SELECT evePendingAlertEmail FROM Events WHERE eveMac = ? AND eveEventType = 'Connected'", + (parent_mac,), + ) + assert cur.fetchone()["evePendingAlertEmail"] == 1 + + def test_quiet_flag_from_separate_row_than_presence_row(self): + """Quiet doesn't have to come from the same CurrentScan row that + asserts presence - two rows for one NIC this cycle, one presence + (not quiet), one quiet (not presence) - locks in the simplification + pass's correction over an earlier, over-restrictive draft.""" + conn = _make_db() + parent_mac = "aa:22:00:00:00:08" + nic_mac = "bb:22:00:00:00:08" + _insert_device_from_dict(conn, _make_device_dict( + parent_mac, devPresentLastScan=0, devAlertDown=1, + devParentMAC="", devParentRelType="", devReqNicsOnline=0, + )) + _insert_device_from_dict(conn, _make_device_dict( + nic_mac, devPresentLastScan=1, devAlertDown=0, + devParentMAC=parent_mac, devParentRelType="nic", devReqNicsOnline=0, + )) + _insert_current_scan_row_from_dict( + conn, _make_current_scan_dict(nic_mac, scanPresence=1, scanSourcePlugin="ARPSCAN") + ) + _insert_current_scan_row_from_dict( + conn, _make_current_scan_dict( + nic_mac, scanPresence=0, scanSourcePlugin="INVENTORY", + scanNotificationMode="quiet", + ) + ) + conn.commit() + + insert_events(DummyDB(conn)) + + cur = conn.cursor() + cur.execute( + "SELECT evePendingAlertEmail FROM Events WHERE eveMac = ? AND eveEventType = 'Connected'", + (parent_mac,), + ) + assert cur.fetchone()["evePendingAlertEmail"] == 0 + + def test_no_nic_children_unaffected(self): + conn = _make_db() + cur = conn.cursor() + _insert_device(cur, "aa:22:00:00:00:09", alert_down=1, present_last_scan=0) + conn.commit() + + insert_events(DummyDB(conn)) + + assert _event_types_for(conn, "aa:22:00:00:00:09") == [] + + def test_orphan_pairing_closes_end_to_end(self): + """The reporter's exact scenario (issue #1821): + absent -> Disconnected -> NIC-present -> Connected -> absent again -> + second Disconnected. The second Disconnected must now pair, unlike + before this PRD (it would have been the permanent orphan). + + Event timestamps are pinned to controlled, strictly-increasing values + after each cycle rather than relied on from real wall-clock time: + timeNowUTC() truncates to whole seconds, and this test's three + insert_events() calls run fast enough to plausibly land in the same + second, which would break pair_sessions_events()'s strict + eveDateTime > eveDateTime pairing query non-deterministically. + """ + conn = _make_db() + parent_mac = "aa:22:00:00:00:10" + nic_mac = "bb:22:00:00:00:10" + _insert_device_from_dict(conn, _make_device_dict( + parent_mac, devPresentLastScan=1, devAlertDown=0, + devParentMAC="", devParentRelType="", devReqNicsOnline=0, + )) + _insert_device_from_dict(conn, _make_device_dict( + nic_mac, devPresentLastScan=1, devAlertDown=0, + devParentMAC=parent_mac, devParentRelType="nic", devReqNicsOnline=0, + )) + cur = conn.cursor() + + def _pin_last_event_timestamp(mac, ts): + cur.execute( + "UPDATE Events SET eveDateTime = ? WHERE ROWID = " + "(SELECT MAX(ROWID) FROM Events WHERE eveMac = ?)", + (ts, mac), + ) + conn.commit() + + # Seed an initial Connected event so the first Disconnected below has + # something to pair to. + cur.execute( + "INSERT INTO Events (eveMac, eveIp, eveDateTime, eveEventType, " + "eveAdditionalInfo, evePendingAlertEmail) VALUES (?, '1.2.3.4', " + "'2026-01-01 09:00:00', 'Connected', '', 1)", + (parent_mac,), + ) + conn.commit() + + # Cycle 1: NIC absent, parent absent -> Disconnected. + insert_events(DummyDB(conn)) + _pin_last_event_timestamp(parent_mac, "2026-01-01 09:05:00") + pair_sessions_events(DummyDB(conn)) + + cur.execute( + "SELECT COUNT(*) AS cnt FROM Events WHERE eveMac = ? AND eveEventType = 'Disconnected'", + (parent_mac,), + ) + assert cur.fetchone()["cnt"] == 1 + + # Reflect what step 6/7 would have set devPresentLastScan to for the + # next cycle - insert_events() alone doesn't run them. + cur.execute("UPDATE Devices SET devPresentLastScan = 0 WHERE devMac = ?", (parent_mac,)) + conn.commit() + + # Cycle 2: NIC comes back -> parent Connected (this PRD's fix). + _insert_current_scan_row_from_dict(conn, _make_current_scan_dict(nic_mac, scanPresence=1)) + conn.commit() + insert_events(DummyDB(conn)) + _pin_last_event_timestamp(parent_mac, "2026-01-01 09:10:00") + pair_sessions_events(DummyDB(conn)) + + cur.execute("UPDATE Devices SET devPresentLastScan = 1 WHERE devMac = ?", (parent_mac,)) + cur.execute("DELETE FROM CurrentScan WHERE scanMac = ?", (nic_mac,)) + conn.commit() + + # Cycle 3: NIC absent again -> a second, real Disconnected. + insert_events(DummyDB(conn)) + _pin_last_event_timestamp(parent_mac, "2026-01-01 09:15:00") + pair_sessions_events(DummyDB(conn)) + + cur.execute( + "SELECT evePairEventRowid FROM Events WHERE eveMac = ? AND eveEventType = 'Disconnected' " + "ORDER BY eveDateTime DESC LIMIT 1", + (parent_mac,), + ) + second_disconnect = cur.fetchone() + assert second_disconnect["evePairEventRowid"] is not None, ( + "the second Disconnected must pair to the NIC-derived Connected event, " + "not become a permanent orphan" + ) diff --git a/test/scan/test_presence_helper.py b/test/scan/test_presence_helper.py index a8cafa453..c16db0782 100644 --- a/test/scan/test_presence_helper.py +++ b/test/scan/test_presence_helper.py @@ -145,21 +145,27 @@ class TestConsumersCallTheHelper: def test_update_dev_last_connection_calls_helper_once(self): assert _call_count(device_handling.update_devLastConnection_from_CurrentScan) == 1 - def test_insert_events_calls_helper_at_least_three_times(self): - """insert_events() contains four queries total - Device Down (x2), - Disconnected, and New Connections. Only the first three are plain - boolean-predicate sites; New Connections keeps its own present_agg/ - MIN(scanLastIP) aggregation on purpose (see module docstring), so - this asserts >= 3, not == 4.""" - assert _call_count(session_events.insert_events) >= 3 + def test_insert_events_calls_helper_at_least_five_times(self): + """insert_events() contains five queries that use current_scan_presence_condition() + directly - Device Down (x2), Disconnected, and the NIC-derived reconnect + query (nic-parent-reconnect-events PRD, two calls: one excluding + directly-present parents, one nested in its quiet-check). The mainline + New Connections query keeps its own present_agg/MIN(scanLastIP) + aggregation on purpose (see module docstring) and doesn't call the + helper directly - it goes through _connect_event_type_case() instead + for classification, not presence - so this asserts >= 5, not == 5.""" + assert _call_count(session_events.insert_events) >= 5 class TestConsumersCallTheNicHelper: - """Guards the four sites nic_derived_presence_condition() was OR-composed - into (nic-parent-orphan-disconnect-events PRD) - both Device Down queries, - Disconnected, and update_devLastConnection_from_CurrentScan(). New - Connections/IP Changed are deliberately not touched - see that PRD's - Non-goals.""" + """Guards the five sites nic_derived_presence_condition() was OR-composed + into: both Device Down queries, Disconnected, and + update_devLastConnection_from_CurrentScan() (nic-parent-orphan-disconnect-events + PRD), plus the NIC-derived reconnect query added by nic-parent-reconnect-events + (one call, gating which parents qualify - not the same call as its + quiet-check, which uses current_scan_presence_condition() instead, guarded + above). The mainline New Connections/IP Changed queries are deliberately + not touched - see that PRD's Non-goals.""" def test_update_dev_last_connection_calls_nic_helper_once(self): assert _call_count( @@ -167,9 +173,23 @@ class TestConsumersCallTheNicHelper: target_name="nic_derived_presence_condition", ) == 1 - def test_insert_events_calls_nic_helper_exactly_three_times(self): - """Both Device Down queries + Disconnected - not New Connections/IP Changed.""" + def test_insert_events_calls_nic_helper_exactly_four_times(self): + """Both Device Down queries + Disconnected + the NIC-derived reconnect + query - not New Connections/IP Changed.""" assert _call_count( session_events.insert_events, target_name="nic_derived_presence_condition", - ) == 3 + ) == 4 + + +class TestConsumersCallTheConnectEventTypeCaseHelper: + """Guards _connect_event_type_case() (nic-parent-reconnect-events PRD) - + both the mainline New Connections query and the new NIC-derived reconnect + query classify Connected vs. Down Reconnected through this one shared + helper, not two independent inline CASE expressions.""" + + def test_insert_events_calls_connect_event_type_case_exactly_twice(self): + assert _call_count( + session_events.insert_events, + target_name="_connect_event_type_case", + ) == 2 From 5ac9c5314297694a5861b1dd9423e558fb18b10e Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Wed, 30 Sep 2026 21:49:00 +1000 Subject: [PATCH 3/5] BE: Fix: NIC-Covered Parent Devices Miss Connected/Down Reconnected Events #1821 --- .claude/skills/scan-pipeline/SKILL.md | 2 +- .gemini/skills/scan-pipeline/SKILL.md | 2 +- .github/skills/scan-pipeline/SKILL.md | 2 +- server/scan/session_events.py | 28 +++++++++------------------ 4 files changed, 12 insertions(+), 22 deletions(-) diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index c29b85cc2..cf1f99907 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -49,7 +49,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at five of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`, and a fifth query that fires the NIC-derived `Connected`/`Down Reconnected` event `insert_events()`'s mainline "New Connections" query can't produce - see Gotcha 7 for why that query can't just reuse `LatestEventsPerMAC`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. The fifth site - the NIC-derived reconnect query (Gotcha 7) - combines them differently: `NOT current_scan_presence_condition(...) AND nic_derived_presence_condition(...)`, since it exists specifically to catch NIC-only presence that the mainline query already handles otherwise. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index da4c06136..f42a09e0f 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -49,7 +49,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at five of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`, and a fifth query that fires the NIC-derived `Connected`/`Down Reconnected` event `insert_events()`'s mainline "New Connections" query can't produce - see Gotcha 7 for why that query can't just reuse `LatestEventsPerMAC`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. The fifth site - the NIC-derived reconnect query (Gotcha 7) - combines them differently: `NOT current_scan_presence_condition(...) AND nic_derived_presence_condition(...)`, since it exists specifically to catch NIC-only presence that the mainline query already handles otherwise. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index f0d3b00c6..a8e3ef0d0 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -49,7 +49,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat ## Gotchas -1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at five of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`, and a fifth query that fires the NIC-derived `Connected`/`Down Reconnected` event `insert_events()`'s mainline "New Connections" query can't produce - see Gotcha 7 for why that query can't just reuse `LatestEventsPerMAC`), not folded into it. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. A second, deliberately separate predicate, `nic_derived_presence_condition()` (same file), answers a narrower question — "is this device's absence from `CurrentScan` masked by NIC-derived presence" (a parent device whose `devParentRelType='nic'` children satisfy `devReqNicsOnline`, ANY/ALL, against `CurrentScan` this cycle) — and is `OR`-composed onto `current_scan_presence_condition()` at four of those same sites (both `Device Down` variants, `Disconnected`, `update_devLastConnection_from_CurrentScan()`), not folded into it. The fifth site - the NIC-derived reconnect query (Gotcha 7) - combines them differently: `NOT current_scan_presence_condition(...) AND nic_derived_presence_condition(...)`, since it exists specifically to catch NIC-only presence that the mainline query already handles otherwise. A future presence-adjacent query needs to check both predicates, not just the first one, if it should also treat a NIC-covered parent as present. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. 3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. 4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. diff --git a/server/scan/session_events.py b/server/scan/session_events.py index 8de0ac97f..8769d8c51 100755 --- a/server/scan/session_events.py +++ b/server/scan/session_events.py @@ -276,30 +276,20 @@ def insert_events(db): ) """) - # Check NIC-derived New Connections / Down Reconnections - a parent with - # no direct CurrentScan row of its own this cycle, but whose NIC children - # satisfy nic_derived_presence_condition(), gets the Connected/Down - # Reconnected event the query above can't produce for it (its - # present_agg is built from CurrentScan, which this parent has no row - # in). - # - # Deliberately NOT via LatestEventsPerMAC (used by the query above): - # that view INNER JOINs CurrentScan, so it returns no row at all for a - # MAC with no CurrentScan row this cycle - exactly every MAC this query - # targets - which would make the Down Reconnected branch silently - # unreachable. The two correlated subqueries below read Events directly - # instead, sidestepping the gap entirely - no COALESCE needed for the - # "no prior event at all" case either: _connect_event_type_case()'s own - # ELSE branch already resolves to 'Connected' when both subqueries - # return NULL (NULL = 'Device Down' is NULL/falsy, same as the query - # above already relies on for a brand-new MAC's LEFT JOIN miss). + # NIC-derived New Connections/Down Reconnected: fires for a parent with + # no CurrentScan row of its own but whose NIC children satisfy + # nic_derived_presence_condition(). Reads Events directly instead of + # LatestEventsPerMAC, which INNER JOINs CurrentScan and would silently + # return no row for every MAC this query targets. mylog("debug", "[Events] - 2b - NIC-derived New Connections") + # ROWID DESC breaks eveDateTime ties (timeNowUTC() truncates to whole + # seconds) so both subqueries resolve to the same row. _last_event_type = """(SELECT eveEventType FROM Events WHERE eveMac = nic_parent.devMac - ORDER BY eveDateTime DESC LIMIT 1)""" + ORDER BY eveDateTime DESC, ROWID DESC LIMIT 1)""" _last_event_pending = """(SELECT evePendingAlertEmail FROM Events WHERE eveMac = nic_parent.devMac - ORDER BY eveDateTime DESC LIMIT 1)""" + ORDER BY eveDateTime DESC, ROWID DESC LIMIT 1)""" sql.execute(f"""INSERT OR IGNORE INTO Events (eveMac, eveIp, eveDateTime, eveEventType, eveAdditionalInfo, evePendingAlertEmail) SELECT nic_parent.devMac, nic_parent.devLastIP, '{startTime}', From fbbdefb898369fac1d6dcce4bddb3729907b06fe Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Wed, 30 Sep 2026 22:01:04 +1000 Subject: [PATCH 4/5] BE: Fix: NIC-Covered Parent Devices Miss Connected/Down Reconnected Events #1821 --- .claude/skills/scan-pipeline/SKILL.md | 1 + .gemini/skills/scan-pipeline/SKILL.md | 1 + .github/skills/scan-pipeline/SKILL.md | 1 + server/scan/presence.py | 32 ++++++++++++++++----- test/scan/test_down_sleep_events.py | 22 ++++++++++++++ test/scan/test_presence_helper.py | 41 +++++++++++++++++++++++++-- 6 files changed, 89 insertions(+), 9 deletions(-) diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index cf1f99907..6c6775894 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. +8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index f42a09e0f..e9ece89b1 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. +8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index a8e3ef0d0..1550ec69c 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. +8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/server/scan/presence.py b/server/scan/presence.py index a2b4a26ec..1837011e5 100644 --- a/server/scan/presence.py +++ b/server/scan/presence.py @@ -66,6 +66,19 @@ def nic_derived_presence_condition(mac_column: str) -> str: mac_column must be a trusted, hardcoded SQL column/table.column reference written by NetAlertX code - same constraint as current_scan_presence_condition(), enforced the same way. + + The inner Devices scan is aliased as nic_presence_parent, not the more + obvious nic_parent, for the same shadowing reason + current_scan_presence_condition() aliases its own inner CurrentScan as + presence_scan rather than bare CurrentScan: a caller correlating this + condition from a query that itself aliases its row as nic_parent (a + natural name to pick, given this helper's own docstring uses it) would + otherwise have mac_column="nic_parent.devMac" resolve to this + subquery's own inner alias instead of the caller's outer row, collapsing + the comparison into an always-true same-row tautology - confirmed live + in this exact shape in nic-parent-reconnect-events.md's implementation + notes. mac_column may not reference nic_presence_parent for the same + reason presence_scan is guarded below. """ if not _SQL_IDENTIFIER_RE.match(mac_column): raise ValueError(f"mac_column must be a plain identifier, got: {mac_column!r}") @@ -75,29 +88,34 @@ def nic_derived_presence_condition(mac_column: str) -> str: f"current_scan_presence_condition()'s own internal subquery " f"alias, got: {mac_column!r}" ) + if mac_column == "nic_presence_parent" or mac_column.startswith("nic_presence_parent."): + raise ValueError( + f"mac_column must not reference nic_presence_parent - that's " + f"this function's own internal subquery alias, got: {mac_column!r}" + ) return f"""EXISTS ( - SELECT 1 FROM Devices AS nic_parent - WHERE nic_parent.devMac = {mac_column} + SELECT 1 FROM Devices AS nic_presence_parent + WHERE nic_presence_parent.devMac = {mac_column} AND ( ( - IFNULL(CAST(nic_parent.devReqNicsOnline AS TEXT), '') = '1' + IFNULL(CAST(nic_presence_parent.devReqNicsOnline AS TEXT), '') = '1' AND EXISTS (SELECT 1 FROM Devices AS any_nic - WHERE any_nic.devParentMAC = nic_parent.devMac + WHERE any_nic.devParentMAC = nic_presence_parent.devMac AND any_nic.devParentRelType = 'nic') AND NOT EXISTS ( SELECT 1 FROM Devices AS nic - WHERE nic.devParentMAC = nic_parent.devMac + WHERE nic.devParentMAC = nic_presence_parent.devMac AND nic.devParentRelType = 'nic' AND NOT {current_scan_presence_condition("nic.devMac")} ) ) OR ( - IFNULL(CAST(nic_parent.devReqNicsOnline AS TEXT), '') != '1' + IFNULL(CAST(nic_presence_parent.devReqNicsOnline AS TEXT), '') != '1' AND EXISTS ( SELECT 1 FROM Devices AS nic - WHERE nic.devParentMAC = nic_parent.devMac + WHERE nic.devParentMAC = nic_presence_parent.devMac AND nic.devParentRelType = 'nic' AND {current_scan_presence_condition("nic.devMac")} ) diff --git a/test/scan/test_down_sleep_events.py b/test/scan/test_down_sleep_events.py index f3b0b89ad..ba06ec7cb 100644 --- a/test/scan/test_down_sleep_events.py +++ b/test/scan/test_down_sleep_events.py @@ -948,6 +948,28 @@ class TestInsertEventsNicDerivedReconnect: assert _event_types_for(conn, "aa:22:00:00:00:09") == [] + def test_sibling_parent_with_absent_nics_not_falsely_reconnected(self): + """Regression: an alias collision inside nic_derived_presence_condition() + used to collapse its per-row check into a table-wide tautology - any + NIC-covered parent anywhere in Devices made every other absent parent + look NIC-derived-present too. A single-parent-per-DB test can't catch + this (there's nothing else in the table to falsely match against), so + this puts two sibling parents in one DB on purpose.""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:22:00:00:00:10", [("bb:22:00:00:00:10", True)], + parent_present_last_scan=0, + ) + _setup_parent_with_nics( + conn, "aa:22:00:00:00:11", [("bb:22:00:00:00:11", False)], + parent_present_last_scan=0, + ) + + insert_events(DummyDB(conn)) + + assert _event_types_for(conn, "aa:22:00:00:00:10") == ["Connected"] + assert _event_types_for(conn, "aa:22:00:00:00:11") == [] + def test_orphan_pairing_closes_end_to_end(self): """The reporter's exact scenario (issue #1821): absent -> Disconnected -> NIC-present -> Connected -> absent again -> diff --git a/test/scan/test_presence_helper.py b/test/scan/test_presence_helper.py index c16db0782..d48a9f886 100644 --- a/test/scan/test_presence_helper.py +++ b/test/scan/test_presence_helper.py @@ -75,8 +75,8 @@ class TestNicDerivedHelperCorrectness: def test_returns_expected_sql_fragment(self): result = nic_derived_presence_condition("devMac") assert "EXISTS (" in result - assert "SELECT 1 FROM Devices AS nic_parent" in result - assert "nic_parent.devMac = devMac" in result + assert "SELECT 1 FROM Devices AS nic_presence_parent" in result + assert "nic_presence_parent.devMac = devMac" in result assert "devReqNicsOnline" in result @pytest.mark.parametrize("bad_value", [ @@ -96,6 +96,14 @@ class TestNicDerivedHelperCorrectness: with pytest.raises(ValueError): nic_derived_presence_condition(bad_value) + @pytest.mark.parametrize("bad_value", ["nic_presence_parent", "nic_presence_parent.devMac"]) + def test_rejects_own_internal_alias(self, bad_value): + """A caller referencing this function's own internal alias would hit + the exact shadowing bug that motivated the nic_parent -> + nic_presence_parent rename - see this module's docstring.""" + with pytest.raises(ValueError): + nic_derived_presence_condition(bad_value) + class TestQualifiedColumnExecutesCorrectly: """Executes the fragment, not just checks the generated SQL text - proves @@ -119,6 +127,35 @@ class TestQualifiedColumnExecutesCorrectly: "into a table-wide 'does anything assert presence' check" ) + def test_nic_derived_condition_correlates_when_caller_aliases_nic_parent(self): + """Regression: nic_derived_presence_condition() used to alias its own + inner scan as nic_parent too, so a caller that (like insert_events()'s + NIC-derived reconnect query) aliases its own row as nic_parent got + mac_column="nic_parent.devMac" shadowed by the helper's own inner + alias - collapsing the EXISTS into a table-wide tautology instead of + a per-row check.""" + conn = sqlite3.connect(":memory:") + conn.execute("""CREATE TABLE Devices (devMac TEXT, devReqNicsOnline INTEGER, + devParentMAC TEXT, devParentRelType TEXT)""") + conn.execute("CREATE TABLE CurrentScan (scanMac TEXT, scanPresence INTEGER)") + conn.execute("INSERT INTO Devices VALUES ('aa', 0, NULL, NULL)") + conn.execute("INSERT INTO Devices VALUES ('aa-nic', 0, 'aa', 'nic')") + conn.execute("INSERT INTO CurrentScan VALUES ('aa-nic', 1)") # aa's NIC is present + conn.execute("INSERT INTO Devices VALUES ('bb', 0, NULL, NULL)") + conn.execute("INSERT INTO Devices VALUES ('bb-nic', 0, 'bb', 'nic')") # bb's NIC absent + conn.commit() + + condition = nic_derived_presence_condition("nic_parent.devMac") + rows = conn.execute( + f"""SELECT devMac, {condition} AS is_nic_present FROM Devices AS nic_parent + WHERE devParentMAC IS NULL""" + ).fetchall() + + assert dict(rows) == {"aa": 1, "bb": 0}, ( + "each outer row must be checked against its own NIC children, not " + "collapse into a table-wide 'does any device have NIC presence' check" + ) + def _call_count(func, target_name="current_scan_presence_condition"): """Count calls to target_name within func's own source (AST-based, not From 06d9ae961332d013daab97b79c6acd6c0a7ff52b Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Thu, 1 Oct 2026 07:41:17 +1000 Subject: [PATCH 5/5] BE: Fix: NIC-Covered Parent Devices Miss Connected/Down Reconnected Events #1821 --- .claude/skills/pr-analysis/SKILL.md | 4 +++ .claude/skills/scan-pipeline/SKILL.md | 2 +- .gemini/skills/pr-analysis/SKILL.md | 4 +++ .gemini/skills/scan-pipeline/SKILL.md | 2 +- .github/skills/pr-analysis/SKILL.md | 4 +++ .github/skills/scan-pipeline/SKILL.md | 2 +- server/scan/device_handling.py | 2 +- server/scan/presence.py | 30 ++++++++++++++--- server/scan/session_events.py | 6 ++-- test/scan/test_down_sleep_events.py | 25 +++++++++++++++ test/scan/test_presence_helper.py | 13 ++++++-- test/scan/test_scan_presence.py | 46 +++++++++++++++++++++++++++ 12 files changed, 127 insertions(+), 13 deletions(-) diff --git a/.claude/skills/pr-analysis/SKILL.md b/.claude/skills/pr-analysis/SKILL.md index 53ce8bb78..3a17b8a88 100644 --- a/.claude/skills/pr-analysis/SKILL.md +++ b/.claude/skills/pr-analysis/SKILL.md @@ -28,6 +28,10 @@ Run through this before creating or editing any file under `test/`: 2. Load the `testing-workflow` skill — any test additions or changes must follow it. 3. Load any domain-specific skill relevant to the files being changed (e.g. `database-patterns` for DB writes, `settings-management` for config). +## Verifying a Claim About Generated Code + +A finding that claims a specific SQL/code expansion result (an alias collision, a macro substitution, an interpolation outcome) can't be verified by checking the caller's own internal consistency alone - the caller can be perfectly self-consistent and still collide with something the callee does internally that isn't visible at the call site. Open and read the callee's actual definition before accepting or rejecting the claim, and if it's a runtime-behavior claim (not just syntax), run a minimal repro to confirm rather than reasoning about it in the abstract. A real case: a finding claimed two same-named aliases collided across a caller/helper boundary; checking only that the caller used its alias consistently looked like it disproved the finding, but the helper had its own same-named internal alias that was never inspected - the finding was correct. + ## Comment Classification For each comment, determine: diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index 6c6775894..06e48bdc1 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -56,7 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. -8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. +8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/.gemini/skills/pr-analysis/SKILL.md b/.gemini/skills/pr-analysis/SKILL.md index 5d3d3417e..88442278a 100644 --- a/.gemini/skills/pr-analysis/SKILL.md +++ b/.gemini/skills/pr-analysis/SKILL.md @@ -28,6 +28,10 @@ Run through this before creating or editing any file under `test/`: 2. Load `testing-workflow` skill — any test additions or changes must follow it. 3. Load any domain-specific skill relevant to the files being changed (e.g. `database-patterns` for DB writes, `settings` for config). +## Verifying a Claim About Generated Code + +A finding that claims a specific SQL/code expansion result (an alias collision, a macro substitution, an interpolation outcome) can't be verified by checking the caller's own internal consistency alone - the caller can be perfectly self-consistent and still collide with something the callee does internally that isn't visible at the call site. Open and read the callee's actual definition before accepting or rejecting the claim, and if it's a runtime-behavior claim (not just syntax), run a minimal repro to confirm rather than reasoning about it in the abstract. A real case: a finding claimed two same-named aliases collided across a caller/helper boundary; checking only that the caller used its alias consistently looked like it disproved the finding, but the helper had its own same-named internal alias that was never inspected - the finding was correct. + ## Comment Classification For each comment, determine: diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index e9ece89b1..224a5e834 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -56,7 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. -8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. +8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/.github/skills/pr-analysis/SKILL.md b/.github/skills/pr-analysis/SKILL.md index 79f0bef10..6251ae55d 100644 --- a/.github/skills/pr-analysis/SKILL.md +++ b/.github/skills/pr-analysis/SKILL.md @@ -28,6 +28,10 @@ Run through this before creating or editing any file under `test/`: 2. Load `testing-workflow` skill — any test additions or changes must follow it. 3. Load any domain-specific skill relevant to the files being changed (e.g. `database-patterns` for DB writes, `settings-management` for config). +## Verifying a Claim About Generated Code + +A finding that claims a specific SQL/code expansion result (an alias collision, a macro substitution, an interpolation outcome) can't be verified by checking the caller's own internal consistency alone - the caller can be perfectly self-consistent and still collide with something the callee does internally that isn't visible at the call site. Open and read the callee's actual definition before accepting or rejecting the claim, and if it's a runtime-behavior claim (not just syntax), run a minimal repro to confirm rather than reasoning about it in the abstract. A real case: a finding claimed two same-named aliases collided across a caller/helper boundary; checking only that the caller used its alias consistently looked like it disproved the finding, but the helper had its own same-named internal alias that was never inspected - the finding was correct. + ## Comment Classification For each comment, determine: diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index 1550ec69c..344549977 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -56,7 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. -8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. +8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index 1ac548bdc..d12cde688 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -242,7 +242,7 @@ def update_devLastConnection_from_CurrentScan(db): UPDATE Devices SET devLastConnection = '{startTime}' WHERE ({current_scan_presence_condition("devMac")} - OR {nic_derived_presence_condition("devMac")}) + OR {nic_derived_presence_condition("Devices.devMac")}) """) diff --git a/server/scan/presence.py b/server/scan/presence.py index 1837011e5..7a7c67341 100644 --- a/server/scan/presence.py +++ b/server/scan/presence.py @@ -75,13 +75,35 @@ def nic_derived_presence_condition(mac_column: str) -> str: natural name to pick, given this helper's own docstring uses it) would otherwise have mac_column="nic_parent.devMac" resolve to this subquery's own inner alias instead of the caller's outer row, collapsing - the comparison into an always-true same-row tautology - confirmed live - in this exact shape in nic-parent-reconnect-events.md's implementation - notes. mac_column may not reference nic_presence_parent for the same - reason presence_scan is guarded below. + the comparison into an always-true same-row tautology. mac_column may + not reference nic_presence_parent for the same reason presence_scan is + guarded below. + + mac_column MUST be qualified (e.g. "Devices.devMac", "DevicesView.devMac" + - never bare "devMac"), unlike current_scan_presence_condition() where a + bare column is fine. The reason is column, not alias, shadowing: this + function's own inner scan is `FROM Devices`, and Devices has a devMac + column, so an unqualified devMac in the substituted WHERE always + resolves to *this* function's own inner row - SQL prefers the innermost + enclosing scope for an unqualified name and only searches outward if the + inner scope has no matching column, so it never even reaches the + caller's outer row. (current_scan_presence_condition()'s inner scan is + `FROM CurrentScan`, which has no devMac column, so a bare "devMac" there + has nothing to bind to inward and correctly falls back outward.) + Confirmed live: an unqualified caller made every device with no NIC + children of its own read as NIC-derived-present, as soon as *any* other + device anywhere in Devices legitimately had one. """ if not _SQL_IDENTIFIER_RE.match(mac_column): raise ValueError(f"mac_column must be a plain identifier, got: {mac_column!r}") + if "." not in mac_column: + raise ValueError( + f"mac_column must be qualified with the caller's own table/alias " + f"(e.g. 'Devices.devMac', not bare 'devMac') - Devices (this " + f"function's own inner scan) already has a devMac column, so an " + f"unqualified reference always binds to this function's own inner " + f"row instead of the caller's, got: {mac_column!r}" + ) if mac_column == "presence_scan" or mac_column.startswith("presence_scan."): raise ValueError( f"mac_column must not reference presence_scan - that's " diff --git a/server/scan/session_events.py b/server/scan/session_events.py index 8769d8c51..66373705c 100755 --- a/server/scan/session_events.py +++ b/server/scan/session_events.py @@ -210,7 +210,7 @@ def insert_events(db): AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} AND NOT ({current_scan_presence_condition("devMac")} - OR {nic_derived_presence_condition("devMac")}) """) + OR {nic_derived_presence_condition("DevicesView.devMac")}) """) # Check device down – sleeping devices whose sleep window has expired mylog("debug", "[Events] - 1b - Devices down (sleep expired)") @@ -225,7 +225,7 @@ def insert_events(db): AND devPresentLastScan = 0 AND {_SQL_NOT_FORCED_ONLINE} AND NOT ({current_scan_presence_condition("devMac")} - OR {nic_derived_presence_condition("devMac")}) + OR {nic_derived_presence_condition("DevicesView.devMac")}) AND NOT EXISTS (SELECT 1 FROM Events WHERE eveMac = devMac AND eveEventType = 'Device Down' @@ -323,7 +323,7 @@ def insert_events(db): AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} AND NOT ({current_scan_presence_condition("devMac")} - OR {nic_derived_presence_condition("devMac")}) """) + OR {nic_derived_presence_condition("Devices.devMac")}) """) # Check IP Changed mylog("debug", "[Events] - 4 - IP Changes") diff --git a/test/scan/test_down_sleep_events.py b/test/scan/test_down_sleep_events.py index ba06ec7cb..f5a94b5d1 100644 --- a/test/scan/test_down_sleep_events.py +++ b/test/scan/test_down_sleep_events.py @@ -650,6 +650,31 @@ class TestInsertEventsNicDerivedPresence: assert "aa:11:00:00:00:03" in _down_event_macs(conn.cursor()) + def test_unrelated_device_without_nics_not_falsely_suppressed(self): + """Regression: an unqualified mac_column in + nic_derived_presence_condition() used to bind to the function's own + inner Devices row instead of the caller's, so any device anywhere + with NIC-derived presence made every OTHER absent device (even one + with no NIC children at all) look NIC-derived-present too, silently + suppressing its real 'Device Down' event.""" + conn = _make_db() + _setup_parent_with_nics(conn, "aa:11:00:00:00:11", [("bb:11:00:00:00:11", True)]) + cur = conn.cursor() + _insert_device(cur, "cc:11:00:00:00:11", alert_down=1, present_last_scan=1) + conn.commit() + + insert_events(DummyDB(conn)) + + down_macs = _down_event_macs(conn.cursor()) + assert "aa:11:00:00:00:11" not in down_macs, ( + "the actual NIC-covered parent must still be suppressed" + ) + assert "cc:11:00:00:00:11" in down_macs, ( + "a device with no NIC children of its own must still get its real " + "'Device Down' event, not be suppressed just because an unrelated " + "device elsewhere has NIC-derived presence" + ) + def test_all_mode_one_nic_missing_still_fires(self): """Mirrors test_req_all_mode_partial_nics_does_not_raise_absent_parent in test_nic_presence.py - same ANY/ALL contract, different pipeline point.""" diff --git a/test/scan/test_presence_helper.py b/test/scan/test_presence_helper.py index d48a9f886..7b41c5733 100644 --- a/test/scan/test_presence_helper.py +++ b/test/scan/test_presence_helper.py @@ -73,12 +73,21 @@ class TestNicDerivedHelperCorrectness: PRD Design §1. Same trust-boundary checks as current_scan_presence_condition().""" def test_returns_expected_sql_fragment(self): - result = nic_derived_presence_condition("devMac") + result = nic_derived_presence_condition("Devices.devMac") assert "EXISTS (" in result assert "SELECT 1 FROM Devices AS nic_presence_parent" in result - assert "nic_presence_parent.devMac = devMac" in result + assert "nic_presence_parent.devMac = Devices.devMac" in result assert "devReqNicsOnline" in result + @pytest.mark.parametrize("bad_value", ["devMac", "scanMac"]) + def test_rejects_unqualified_column(self, bad_value): + """Devices (this function's own inner scan) has a devMac column, so a + bare mac_column always binds to the function's own inner row instead + of the caller's - see this module's docstring for the confirmed-live + failure mode this guards against.""" + with pytest.raises(ValueError): + nic_derived_presence_condition(bad_value) + @pytest.mark.parametrize("bad_value", [ "devMac; DROP TABLE Devices--", "devMac OR 1=1", diff --git a/test/scan/test_scan_presence.py b/test/scan/test_scan_presence.py index cd64f3fdb..d915d0b21 100644 --- a/test/scan/test_scan_presence.py +++ b/test/scan/test_scan_presence.py @@ -155,6 +155,52 @@ class TestDevLastConnectionRespectsPresence: ).fetchone() assert row["devLastConnection"] != "2020-01-01 00:00:00" + def test_unrelated_device_without_nics_not_falsely_bumped(self): + """Regression: nic_derived_presence_condition() used to be callable + with a bare, unqualified mac_column - Devices (this function's own + inner scan) has a devMac column, so the unqualified reference always + bound to the function's own inner row instead of the caller's, + making every device with no NIC children of its own read as + NIC-derived-present as soon as *any other* device anywhere in + Devices legitimately had one. Two devices in one DB on purpose - a + single-device DB can't distinguish "per-row correct" from + "table-wide tautology".""" + conn = make_db() + parent_mac = "aa:22:00:00:00:03" + nic_mac = "bb:22:00:00:00:03" + unrelated_mac = "cc:22:00:00:00:03" + insert_device_from_dict(conn, make_device_dict( + parent_mac, devLastConnection="2020-01-01 00:00:00", + devParentMAC="", devParentRelType="", devReqNicsOnline=0, + )) + insert_device_from_dict(conn, make_device_dict( + nic_mac, devParentMAC=parent_mac, devParentRelType="nic", devReqNicsOnline=0, + )) + insert_current_scan_row_from_dict( + conn, make_current_scan_dict(nic_mac, scanPresence=1) + ) + insert_device_from_dict(conn, make_device_dict( + unrelated_mac, devLastConnection="2020-01-01 00:00:00", + devParentMAC="", devParentRelType="", devReqNicsOnline=0, + )) + db = DummyDB(conn) + + device_handling.update_devLastConnection_from_CurrentScan(db) + + parent_row = conn.execute( + "SELECT devLastConnection FROM Devices WHERE devMac = ?", (parent_mac,) + ).fetchone() + unrelated_row = conn.execute( + "SELECT devLastConnection FROM Devices WHERE devMac = ?", (unrelated_mac,) + ).fetchone() + assert parent_row["devLastConnection"] != "2020-01-01 00:00:00", ( + "the actual NIC-covered parent must still bump" + ) + assert unrelated_row["devLastConnection"] == "2020-01-01 00:00:00", ( + "a device with no NIC children of its own must not be bumped just " + "because some other device in Devices has NIC-derived presence" + ) + class TestNewConnectionsRespectsPresence: """insert_events()'s New Connections query must not fire Connected for a