From b18fc0688a7b6e672d37e8e3d424ce54ccc2dd00 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Wed, 30 Sep 2026 21:33:33 +1000 Subject: [PATCH] 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