From 1437e8e48a9ae88e33fa5348a2a943cd49a79da0 Mon Sep 17 00:00:00 2001 From: Jack Kingsman Date: Thu, 16 Apr 2026 13:16:07 -0700 Subject: [PATCH] Fix issue where last_seen is incremented by events that definitely shouldn't increment it. Fixes #201. --- app/event_handlers.py | 4 +- app/repository/contacts.py | 65 ++++++++++++++++-- app/services/dm_ingest.py | 5 ++ tests/test_contacts_router.py | 6 +- tests/test_event_handlers.py | 6 +- tests/test_repository.py | 123 ++++++++++++++++++++++++++++++++++ 6 files changed, 198 insertions(+), 11 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 59176eb..25ba0ce 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -237,7 +237,9 @@ async def on_new_contact(event: "Event") -> None: logger.debug("New contact: %s", public_key[:12]) contact_upsert = ContactUpsert.from_radio_dict(public_key.lower(), payload, on_radio=False) - contact_upsert.last_seen = int(time.time()) + # Intentionally do not set last_seen here: NEW_CONTACT fires from the + # radio's stored contact DB, not an RF observation. last_seen means + # "last time we heard this pubkey on RF". await ContactRepository.upsert(contact_upsert) promoted_keys = await promote_prefix_contacts_for_contact( public_key=public_key, diff --git a/app/repository/contacts.py b/app/repository/contacts.py index dd64a96..ff542fa 100644 --- a/app/repository/contacts.py +++ b/app/repository/contacts.py @@ -94,7 +94,12 @@ class ContactRepository: last_advert = COALESCE(excluded.last_advert, contacts.last_advert), lat = COALESCE(excluded.lat, contacts.lat), lon = COALESCE(excluded.lon, contacts.lon), - last_seen = excluded.last_seen, + last_seen = CASE + WHEN excluded.last_seen IS NULL THEN contacts.last_seen + WHEN contacts.last_seen IS NULL THEN excluded.last_seen + WHEN excluded.last_seen > contacts.last_seen THEN excluded.last_seen + ELSE contacts.last_seen + END, on_radio = COALESCE(excluded.on_radio, contacts.on_radio), last_contacted = COALESCE(excluded.last_contacted, contacts.last_contacted), first_seen = COALESCE(contacts.first_seen, excluded.first_seen) @@ -114,7 +119,7 @@ class ContactRepository: contact_row.last_advert, contact_row.lat, contact_row.lon, - contact_row.last_seen if contact_row.last_seen is not None else int(time.time()), + contact_row.last_seen, contact_row.on_radio, contact_row.last_contacted, contact_row.first_seen, @@ -339,6 +344,15 @@ class ContactRepository: path_hash_mode: int | None = None, updated_at: int | None = None, ) -> None: + """Persist a learned direct route for a contact. + + Both callers (the RF PATH packet processor and the firmware PATH_UPDATE + event handler) are RF-backed: firmware ``onContactPathUpdated`` only + fires from ``onContactPathRecv`` during RF PATH packet reception. So + this method also advances ``last_seen`` monotonically. Never moves + ``last_seen`` backwards if an out-of-order arrival lands with an older + timestamp. + """ normalized_path, normalized_path_len, normalized_hash_mode = normalize_contact_route( path, path_len, @@ -349,13 +363,20 @@ class ContactRepository: """UPDATE contacts SET direct_path = ?, direct_path_len = ?, direct_path_hash_mode = COALESCE(?, direct_path_hash_mode), direct_path_updated_at = ?, - last_seen = ? WHERE public_key = ?""", + last_seen = CASE + WHEN last_seen IS NULL THEN ? + WHEN ? > last_seen THEN ? + ELSE last_seen + END + WHERE public_key = ?""", ( normalized_path, normalized_path_len, normalized_hash_mode, ts, ts, + ts, + ts, public_key.lower(), ), ) @@ -444,11 +465,43 @@ class ContactRepository: @staticmethod async def update_last_contacted(public_key: str, timestamp: int | None = None) -> None: - """Update the last_contacted timestamp for a contact.""" + """Update the last_contacted timestamp for a contact. + + ``last_contacted`` tracks the most recent direct-conversation activity + with this contact in either direction (incoming or outgoing DM). It is + the field that powers "recent conversations" ordering on the frontend. + + It deliberately does not touch ``last_seen``: ``last_seen`` is reserved + for actual RF reception from the contact, and outgoing sends are not + evidence that we heard from them. RF observations from DM ingest update + ``last_seen`` via :meth:`touch_last_seen` on incoming DMs only. + """ ts = timestamp if timestamp is not None else int(time.time()) await db.conn.execute( - "UPDATE contacts SET last_contacted = ?, last_seen = ? WHERE public_key = ?", - (ts, ts, public_key.lower()), + "UPDATE contacts SET last_contacted = ? WHERE public_key = ?", + (ts, public_key.lower()), + ) + await db.conn.commit() + + @staticmethod + async def touch_last_seen(public_key: str, timestamp: int) -> None: + """Monotonically bump last_seen for a contact from an RF observation. + + Never moves last_seen backwards; a no-op if the contact row does not + exist. Use this from packet-ingest paths that have attributed a packet + to a specific contact pubkey (advert, incoming DM, decrypted PATH, etc.). + """ + await db.conn.execute( + """ + UPDATE contacts + SET last_seen = CASE + WHEN last_seen IS NULL THEN ? + WHEN ? > last_seen THEN ? + ELSE last_seen + END + WHERE public_key = ? + """, + (timestamp, timestamp, timestamp, public_key.lower()), ) await db.conn.commit() diff --git a/app/services/dm_ingest.py b/app/services/dm_ingest.py index 00ec64a..4256801 100644 --- a/app/services/dm_ingest.py +++ b/app/services/dm_ingest.py @@ -252,6 +252,11 @@ async def _store_direct_message( if update_last_contacted_key: await contact_repository.update_last_contacted(update_last_contacted_key, received_at) + # Incoming DMs are direct RF evidence that this contact transmitted; + # outgoing DMs are our own send and must not bump the contact's + # last_seen. + if not outgoing: + await contact_repository.touch_last_seen(update_last_contacted_key, received_at) return message diff --git a/tests/test_contacts_router.py b/tests/test_contacts_router.py index 09a9a30..bb867c5 100644 --- a/tests/test_contacts_router.py +++ b/tests/test_contacts_router.py @@ -105,13 +105,15 @@ class TestCreateContact: data = response.json() assert data["public_key"] == KEY_A assert data["name"] == "NewContact" - assert data["last_seen"] is not None + # Manually created contacts have no RF observation yet, so last_seen + # stays NULL until we actually hear them on the air. + assert data["last_seen"] is None # Verify in DB contact = await ContactRepository.get_by_key(KEY_A) assert contact is not None assert contact.name == "NewContact" - assert data["last_seen"] == contact.last_seen + assert contact.last_seen is None mock_broadcast.assert_called_once_with("contact", contact.model_dump()) @pytest.mark.asyncio diff --git a/tests/test_event_handlers.py b/tests/test_event_handlers.py index 5de10b0..b0317e4 100644 --- a/tests/test_event_handlers.py +++ b/tests/test_event_handlers.py @@ -1134,12 +1134,14 @@ class TestOnNewContact: await on_new_contact(MockEvent()) - # Verify contact was created in real DB + # Verify contact was created in real DB. NEW_CONTACT is the radio's + # stored contact DB, not an RF observation, so last_seen stays NULL + # until we actually hear the contact on the air. contact = await ContactRepository.get_by_key("cc" * 32) assert contact is not None assert contact.name == "Charlie" assert contact.on_radio is False - assert contact.last_seen == 1700000000 + assert contact.last_seen is None mock_broadcast.assert_called_once() event_type, contact_data = mock_broadcast.call_args[0] diff --git a/tests/test_repository.py b/tests/test_repository.py index 22bbe14..733337b 100644 --- a/tests/test_repository.py +++ b/tests/test_repository.py @@ -697,3 +697,126 @@ class TestContactRepositoryUpsertContracts: assert contact.name == "Bob" assert contact.type == 2 assert contact.on_radio is True + + +class TestContactRepositoryLastSeenSemantics: + """Guard the 'last_seen = last RF reception' contract. + + Radio-driven contact-DB syncs must not clobber an earlier real RF timestamp, + and callers that don't supply last_seen must leave the existing value alone. + """ + + @pytest.mark.asyncio + async def test_upsert_without_last_seen_preserves_existing(self, test_db): + real_rf_observation = 1_700_000_000 + await ContactRepository.upsert( + ContactUpsert( + public_key="aa" * 32, + name="Alice", + type=1, + last_seen=real_rf_observation, + on_radio=False, + ) + ) + + # A subsequent radio-sync style upsert (no last_seen supplied) must not + # overwrite the real RF timestamp with now(). + await ContactRepository.upsert( + ContactUpsert(public_key="aa" * 32, name="Alice", type=1, on_radio=False) + ) + + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_seen == real_rf_observation + + @pytest.mark.asyncio + async def test_upsert_monotonically_bumps_last_seen(self, test_db): + await ContactRepository.upsert( + ContactUpsert(public_key="aa" * 32, last_seen=1_700_000_000, on_radio=False) + ) + + # Newer RF observation advances last_seen. + await ContactRepository.upsert( + ContactUpsert(public_key="aa" * 32, last_seen=1_700_000_500, on_radio=False) + ) + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_seen == 1_700_000_500 + + # An older timestamp (out-of-order arrival) must not move it backwards. + await ContactRepository.upsert( + ContactUpsert(public_key="aa" * 32, last_seen=1_699_999_000, on_radio=False) + ) + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_seen == 1_700_000_500 + + @pytest.mark.asyncio + async def test_upsert_inserts_null_last_seen_when_not_supplied(self, test_db): + # A radio-sync-only contact (never heard on RF) should have last_seen=NULL. + await ContactRepository.upsert( + ContactUpsert(public_key="aa" * 32, name="Alice", type=1, on_radio=False) + ) + + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_seen is None + + @pytest.mark.asyncio + async def test_touch_last_seen_bumps_monotonically(self, test_db): + await ContactRepository.upsert( + ContactUpsert(public_key="aa" * 32, last_seen=1_700_000_000, on_radio=False) + ) + + await ContactRepository.touch_last_seen("aa" * 32, 1_700_000_500) + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_seen == 1_700_000_500 + + # Older timestamps never move last_seen backwards. + await ContactRepository.touch_last_seen("aa" * 32, 1_699_999_000) + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_seen == 1_700_000_500 + + @pytest.mark.asyncio + async def test_update_last_contacted_does_not_touch_last_seen(self, test_db): + # last_contacted = we sent TO them. It must not forge RF reception. + await ContactRepository.upsert( + ContactUpsert(public_key="aa" * 32, last_seen=1_700_000_000, on_radio=False) + ) + + await ContactRepository.update_last_contacted("aa" * 32, 1_700_500_000) + + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_contacted == 1_700_500_000 + assert contact.last_seen == 1_700_000_000 + + @pytest.mark.asyncio + async def test_update_direct_path_bumps_last_seen_monotonically(self, test_db): + # update_direct_path is driven by RF PATH reception on both callers + # (packet processor + firmware PATH_UPDATE, which only fires from + # onContactPathRecv during RF reception). It should advance last_seen + # forward-only. + await ContactRepository.upsert( + ContactUpsert(public_key="aa" * 32, last_seen=1_700_000_000, on_radio=False) + ) + + await ContactRepository.update_direct_path( + "aa" * 32, path="ab", path_len=1, path_hash_mode=0, updated_at=1_700_000_500 + ) + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_seen == 1_700_000_500 + assert contact.direct_path == "ab" + + # Out-of-order PATH arrival with an older timestamp must not rewind. + await ContactRepository.update_direct_path( + "aa" * 32, path="cd", path_len=1, path_hash_mode=0, updated_at=1_699_999_000 + ) + contact = await ContactRepository.get_by_key("aa" * 32) + assert contact is not None + assert contact.last_seen == 1_700_000_500 + # The path itself still updates — only last_seen is monotonic-guarded. + assert contact.direct_path == "cd"