Merge pull request #1823 from netalertx/next_release

Next release
This commit is contained in:
Jokob @NetAlertX authored and GitHub committed 2026-10-01 07:49:21 +10:00
commit 19aa886432
12 files changed
+913 -26

No files matched your search

+4
View File
@@ -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:
+4 -2
View File
@@ -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,14 @@ 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. 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.
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 `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
+4
View File
@@ -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:
+4 -2
View File
@@ -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,14 @@ 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. 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.
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 `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
+4
View File
@@ -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:
+4 -2
View File
@@ -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,14 @@ 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. 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.
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 `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
+7 -3
View File
@@ -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("Devices.devMac")})
""")
+104
View File
@@ -40,3 +40,107 @@ 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.
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. 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 "
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_presence_parent
WHERE nic_presence_parent.devMac = {mac_column}
AND (
(
IFNULL(CAST(nic_presence_parent.devReqNicsOnline AS TEXT), '') = '1'
AND EXISTS (SELECT 1 FROM Devices AS any_nic
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_presence_parent.devMac
AND nic.devParentRelType = 'nic'
AND NOT {current_scan_presence_condition("nic.devMac")}
)
)
OR
(
IFNULL(CAST(nic_presence_parent.devReqNicsOnline AS TEXT), '') != '1'
AND EXISTS (
SELECT 1 FROM Devices AS nic
WHERE nic.devParentMAC = nic_presence_parent.devMac
AND nic.devParentRelType = 'nic'
AND {current_scan_presence_condition("nic.devMac")}
)
)
)
)"""
+61 -8
View File
@@ -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
@@ -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"))
@@ -191,7 +209,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("DevicesView.devMac")}) """)
# Check device down – sleeping devices whose sleep window has expired
mylog("debug", "[Events] - 1b - Devices down (sleep expired)")
@@ -205,7 +224,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("DevicesView.devMac")})
AND NOT EXISTS (SELECT 1 FROM Events
WHERE eveMac = devMac
AND eveEventType = 'Device Down'
@@ -232,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 (
@@ -259,6 +276,41 @@ def insert_events(db):
)
""")
# 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, ROWID DESC LIMIT 1)"""
_last_event_pending = """(SELECT evePendingAlertEmail FROM Events
WHERE eveMac = nic_parent.devMac
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}',
{_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,
@@ -270,7 +322,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("Devices.devMac")}) """)
# Check IP Changed
mylog("debug", "[Events] - 4 - IP Changes")
+519 -1
View File
@@ -33,11 +33,15 @@ 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,
)
# 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
# ---------------------------------------------------------------------------
@@ -563,3 +567,517 @@ 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_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."""
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())
# ---------------------------------------------------------------------------
# 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_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 ->
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"
)
+124 -8
View File
@@ -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,52 @@ 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("Devices.devMac")
assert "EXISTS (" in result
assert "SELECT 1 FROM Devices AS nic_presence_parent" 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",
"'; 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)
@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
a qualified mac_column ("CurrentScan.scanMac") still discriminates
@@ -90,6 +136,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
@@ -116,10 +191,51 @@ 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 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(
device_handling.update_devLastConnection_from_CurrentScan,
target_name="nic_derived_presence_condition",
) == 1
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",
) == 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
+74
View File
@@ -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,78 @@ 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"
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