From c975dd85d88315a5337b21e78def44a44c8ae5ae Mon Sep 17 00:00:00 2001 From: "Walter A. Boring IV" Date: Fri, 28 Aug 2026 12:35:58 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20WatchList.stats()=20=E2=80=94=20remove?= =?UTF-8?q?=20early=20return=20that=20bypassed=20lock=20(#258)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stats() had a bare 'return self.data' on the first line that: - returned the raw internal dict without holding self.lock (race condition) - made the locked loop below unreachable dead code - returned the wrong shape (callers expect age/old/packet/last keys, not the raw was_old_before_update internal key) Remove the early return so the existing with self.lock: loop executes. Tests added (tests/packets/test_watch_list.py): - test_stats_empty: empty watch list returns {} - test_stats_returns_enriched_shape: verifies the four expected keys are present and the raw-internal 'was_old_before_update' key is absent - test_stats_not_raw_internal_dict: returned dict must not be wl.data itself - setUp/tearDown: reset class-level data and initialized to prevent leakage Closes #237 --- ChangeLog.md | 4 +-- aprsd/packets/watch_list.py | 1 - tests/packets/test_watch_list.py | 58 +++++++++++++++++++++++++++++++- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/ChangeLog.md b/ChangeLog.md index e5c8ef3..61c008d 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -8,9 +8,7 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). ##### Bug Fixes -- Add tests for APRSDClient._checks initialisation and first-call keepalive behaviour [`20da2ad`](https://github.com/craigerl/aprsd/commit/20da2ad) - -- Fix APRSDClient._checks AttributeError on first keepalive_check() call — initialise to False in __init__ [`52e1346`](https://github.com/craigerl/aprsd/commit/52e1346) +- Fix WatchList.stats() and add regression tests — remove early return that bypassed lock [`481a78a`](https://github.com/craigerl/aprsd/commit/481a78a) - Fix APRSISDriver.is_configured() always returning True when driver is disabled [`a9ef65f`](https://github.com/craigerl/aprsd/commit/a9ef65f) diff --git a/aprsd/packets/watch_list.py b/aprsd/packets/watch_list.py index ec09128..e0bb1e1 100644 --- a/aprsd/packets/watch_list.py +++ b/aprsd/packets/watch_list.py @@ -51,7 +51,6 @@ class WatchList(objectstore.ObjectStoreMixin): @trace.no_trace def stats(self, serializable=False) -> dict: stats = {} - return self.data with self.lock: for callsign in self.data: stats[callsign] = { diff --git a/tests/packets/test_watch_list.py b/tests/packets/test_watch_list.py index 9010d9b..c2d5ef0 100644 --- a/tests/packets/test_watch_list.py +++ b/tests/packets/test_watch_list.py @@ -14,8 +14,10 @@ class TestWatchList(unittest.TestCase): def setUp(self): """Set up test fixtures.""" - # Reset singleton instance + # Reset singleton AND class-level state fully between tests watch_list.WatchList._instance = None + watch_list.WatchList.data = {} + watch_list.WatchList.initialized = False # Mock config CONF.watch_list.enabled = True CONF.watch_list.callsigns = ['TEST*'] @@ -24,6 +26,8 @@ class TestWatchList(unittest.TestCase): def tearDown(self): """Clean up after tests.""" watch_list.WatchList._instance = None + watch_list.WatchList.data = {} + watch_list.WatchList.initialized = False def test_singleton_pattern(self): """Test that WatchList is a singleton.""" @@ -47,6 +51,58 @@ class TestWatchList(unittest.TestCase): self.assertIn('TEST1', wl.data) self.assertIn('TEST2', wl.data) + def test_stats_empty(self): + """stats() returns an empty dict when the watch list is empty.""" + watch_list.WatchList._instance = None + CONF.watch_list.callsigns = [] + wl = watch_list.WatchList() + self.assertEqual(wl.stats(), {}) + + def test_stats_returns_enriched_shape(self): + """stats() must return the enriched dict shape, not the raw internal data. + + Regression test for the early-return bug: 'return self.data' was the + first statement in stats(), bypassing the lock and returning the raw + internal dict instead of the expected {callsign: {last, packet, age, old}} + shape that callers rely on. + """ + watch_list.WatchList._instance = None + CONF.watch_list.callsigns = ['ENRICHED*'] + wl = watch_list.WatchList() + + # Populate with a seen packet so there is something to report + from tests import fake + + packet = fake.fake_packet(fromcall='ENRICHED') + wl.rx(packet) + + stats = wl.stats() + + self.assertIn('ENRICHED', stats) + entry = stats['ENRICHED'] + # These keys are built by the locked loop — absent if early return fires + self.assertIn('last', entry) + self.assertIn('packet', entry) + self.assertIn('age', entry) + self.assertIn('old', entry) + # Confirm we are NOT getting the raw internal key that only exists there + self.assertNotIn('was_old_before_update', entry) + + def test_stats_not_raw_internal_dict(self): + """stats() must not return the raw self.data reference. + + If stats() returns self.data directly, mutations via rx() after the + call would silently alter the returned value — and any consumer + that sees 'was_old_before_update' knows it got the raw dict. + """ + watch_list.WatchList._instance = None + CONF.watch_list.callsigns = ['RAW*'] + wl = watch_list.WatchList() + + stats = wl.stats() + # stats() should return a freshly-built dict, not the live self.data ref + self.assertIsNot(stats, wl.data) + def test_stats(self): """Test stats() method.""" wl = watch_list.WatchList()