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

.claude/skills/scan-pipeline/SKILL.md

@@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat
 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too.
 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale.
 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes.
+8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one.
 
 ## When to read this vs. other docs/skills
 

.gemini/skills/scan-pipeline/SKILL.md

@@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat
 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too.
 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale.
 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes.
+8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one.
 
 ## When to read this vs. other docs/skills
 

.github/skills/scan-pipeline/SKILL.md

@@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat
 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too.
 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale.
 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes.
+8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one.
 
 ## When to read this vs. other docs/skills
 

server/scan/presence.py

@@ -66,6 +66,19 @@ def nic_derived_presence_condition(mac_column: str) -> str:
     mac_column must be a trusted, hardcoded SQL column/table.column reference
     written by NetAlertX code - same constraint as
     current_scan_presence_condition(), enforced the same way.
+
+    The inner Devices scan is aliased as nic_presence_parent, not the more
+    obvious nic_parent, for the same shadowing reason
+    current_scan_presence_condition() aliases its own inner CurrentScan as
+    presence_scan rather than bare CurrentScan: a caller correlating this
+    condition from a query that itself aliases its row as nic_parent (a
+    natural name to pick, given this helper's own docstring uses it) would
+    otherwise have mac_column="nic_parent.devMac" resolve to this
+    subquery's own inner alias instead of the caller's outer row, collapsing
+    the comparison into an always-true same-row tautology - confirmed live
+    in this exact shape in nic-parent-reconnect-events.md's implementation
+    notes. mac_column may not reference nic_presence_parent for the same
+    reason presence_scan is guarded below.
     """
     if not _SQL_IDENTIFIER_RE.match(mac_column):
         raise ValueError(f"mac_column must be a plain identifier, got: {mac_column!r}")
@@ -75,29 +88,34 @@ def nic_derived_presence_condition(mac_column: str) -> str:
             f"current_scan_presence_condition()'s own internal subquery "
             f"alias, got: {mac_column!r}"
         )
+    if mac_column == "nic_presence_parent" or mac_column.startswith("nic_presence_parent."):
+        raise ValueError(
+            f"mac_column must not reference nic_presence_parent - that's "
+            f"this function's own internal subquery alias, got: {mac_column!r}"
+        )
 
     return f"""EXISTS (
-        SELECT 1 FROM Devices AS nic_parent
-        WHERE nic_parent.devMac = {mac_column}
+        SELECT 1 FROM Devices AS nic_presence_parent
+        WHERE nic_presence_parent.devMac = {mac_column}
           AND (
                 (
-                    IFNULL(CAST(nic_parent.devReqNicsOnline AS TEXT), '') = '1'
+                    IFNULL(CAST(nic_presence_parent.devReqNicsOnline AS TEXT), '') = '1'
                     AND EXISTS (SELECT 1 FROM Devices AS any_nic
-                                WHERE any_nic.devParentMAC = nic_parent.devMac
+                                WHERE any_nic.devParentMAC = nic_presence_parent.devMac
                                   AND any_nic.devParentRelType = 'nic')
                     AND NOT EXISTS (
                         SELECT 1 FROM Devices AS nic
-                        WHERE nic.devParentMAC = nic_parent.devMac
+                        WHERE nic.devParentMAC = nic_presence_parent.devMac
                           AND nic.devParentRelType = 'nic'
                           AND NOT {current_scan_presence_condition("nic.devMac")}
                     )
                 )
                 OR
                 (
-                    IFNULL(CAST(nic_parent.devReqNicsOnline AS TEXT), '') != '1'
+                    IFNULL(CAST(nic_presence_parent.devReqNicsOnline AS TEXT), '') != '1'
                     AND EXISTS (
                         SELECT 1 FROM Devices AS nic
-                        WHERE nic.devParentMAC = nic_parent.devMac
+                        WHERE nic.devParentMAC = nic_presence_parent.devMac
                           AND nic.devParentRelType = 'nic'
                           AND {current_scan_presence_condition("nic.devMac")}
                     )

test/scan/test_down_sleep_events.py

@@ -948,6 +948,28 @@ class TestInsertEventsNicDerivedReconnect:
 
         assert _event_types_for(conn, "aa:22:00:00:00:09") == []
 
+    def test_sibling_parent_with_absent_nics_not_falsely_reconnected(self):
+        """Regression: an alias collision inside nic_derived_presence_condition()
+        used to collapse its per-row check into a table-wide tautology - any
+        NIC-covered parent anywhere in Devices made every other absent parent
+        look NIC-derived-present too. A single-parent-per-DB test can't catch
+        this (there's nothing else in the table to falsely match against), so
+        this puts two sibling parents in one DB on purpose."""
+        conn = _make_db()
+        _setup_parent_with_nics(
+            conn, "aa:22:00:00:00:10", [("bb:22:00:00:00:10", True)],
+            parent_present_last_scan=0,
+        )
+        _setup_parent_with_nics(
+            conn, "aa:22:00:00:00:11", [("bb:22:00:00:00:11", False)],
+            parent_present_last_scan=0,
+        )
+
+        insert_events(DummyDB(conn))
+
+        assert _event_types_for(conn, "aa:22:00:00:00:10") == ["Connected"]
+        assert _event_types_for(conn, "aa:22:00:00:00:11") == []
+
     def test_orphan_pairing_closes_end_to_end(self):
         """The reporter's exact scenario (issue #1821):
         absent -> Disconnected -> NIC-present -> Connected -> absent again ->

test/scan/test_presence_helper.py

@@ -75,8 +75,8 @@ class TestNicDerivedHelperCorrectness:
     def test_returns_expected_sql_fragment(self):
         result = nic_derived_presence_condition("devMac")
         assert "EXISTS (" in result
-        assert "SELECT 1 FROM Devices AS nic_parent" in result
-        assert "nic_parent.devMac = devMac" in result
+        assert "SELECT 1 FROM Devices AS nic_presence_parent" in result
+        assert "nic_presence_parent.devMac = devMac" in result
         assert "devReqNicsOnline" in result
 
     @pytest.mark.parametrize("bad_value", [
@@ -96,6 +96,14 @@ class TestNicDerivedHelperCorrectness:
         with pytest.raises(ValueError):
             nic_derived_presence_condition(bad_value)
 
+    @pytest.mark.parametrize("bad_value", ["nic_presence_parent", "nic_presence_parent.devMac"])
+    def test_rejects_own_internal_alias(self, bad_value):
+        """A caller referencing this function's own internal alias would hit
+        the exact shadowing bug that motivated the nic_parent ->
+        nic_presence_parent rename - see this module's docstring."""
+        with pytest.raises(ValueError):
+            nic_derived_presence_condition(bad_value)
+
 
 class TestQualifiedColumnExecutesCorrectly:
     """Executes the fragment, not just checks the generated SQL text - proves
@@ -119,6 +127,35 @@ class TestQualifiedColumnExecutesCorrectly:
             "into a table-wide 'does anything assert presence' check"
         )
 
+    def test_nic_derived_condition_correlates_when_caller_aliases_nic_parent(self):
+        """Regression: nic_derived_presence_condition() used to alias its own
+        inner scan as nic_parent too, so a caller that (like insert_events()'s
+        NIC-derived reconnect query) aliases its own row as nic_parent got
+        mac_column="nic_parent.devMac" shadowed by the helper's own inner
+        alias - collapsing the EXISTS into a table-wide tautology instead of
+        a per-row check."""
+        conn = sqlite3.connect(":memory:")
+        conn.execute("""CREATE TABLE Devices (devMac TEXT, devReqNicsOnline INTEGER,
+                        devParentMAC TEXT, devParentRelType TEXT)""")
+        conn.execute("CREATE TABLE CurrentScan (scanMac TEXT, scanPresence INTEGER)")
+        conn.execute("INSERT INTO Devices VALUES ('aa', 0, NULL, NULL)")
+        conn.execute("INSERT INTO Devices VALUES ('aa-nic', 0, 'aa', 'nic')")
+        conn.execute("INSERT INTO CurrentScan VALUES ('aa-nic', 1)")  # aa's NIC is present
+        conn.execute("INSERT INTO Devices VALUES ('bb', 0, NULL, NULL)")
+        conn.execute("INSERT INTO Devices VALUES ('bb-nic', 0, 'bb', 'nic')")  # bb's NIC absent
+        conn.commit()
+
+        condition = nic_derived_presence_condition("nic_parent.devMac")
+        rows = conn.execute(
+            f"""SELECT devMac, {condition} AS is_nic_present FROM Devices AS nic_parent
+                WHERE devParentMAC IS NULL"""
+        ).fetchall()
+
+        assert dict(rows) == {"aa": 1, "bb": 0}, (
+            "each outer row must be checked against its own NIC children, not "
+            "collapse into a table-wide 'does any device have NIC presence' check"
+        )
+
 
 def _call_count(func, target_name="current_scan_presence_condition"):
     """Count calls to target_name within func's own source (AST-based, not