BE: Fix: NIC-Covered Parent Devices Miss Connected/Down Reconnected Events #1821

.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