Fix on_arrival() to always cancel departure timer on device re-arrival
- Bug: device departs → timer starts, but last_departed_time not set yet If device re-arrives before timer fires, timer never cancelled Result: false DEPARTED alert sent after re-arrival debounce expires - Fix: Cancel any pending departure timer immediately on re-arrival, before checking last_departed_time - Test: Added regression test that verifies timer is cancelled when device re-arrives before 15-min debounce expires, preventing false alerts - All 10 tests passing
This commit is contained in:
@@ -423,14 +423,16 @@ def on_arrival(mac: str, ip: str, hostname: str = "") -> None:
|
|||||||
|
|
||||||
# Check re-arrival suppression BEFORE acquiring lock (avoid nested locks)
|
# Check re-arrival suppression BEFORE acquiring lock (avoid nested locks)
|
||||||
with departure_lock:
|
with departure_lock:
|
||||||
|
# ALWAYS cancel any pending departure timer when device re-arrives
|
||||||
|
if mac in departure_timers:
|
||||||
|
departure_timers[mac].cancel()
|
||||||
|
del departure_timers[mac]
|
||||||
|
logging.debug(f"Cancelled departure timer for {mac} — device re-arrived before debounce expired")
|
||||||
|
|
||||||
if mac in last_departed_time:
|
if mac in last_departed_time:
|
||||||
time_since_departure = now - last_departed_time[mac]
|
time_since_departure = now - last_departed_time[mac]
|
||||||
if time_since_departure < 300: # 5 min
|
if time_since_departure < 300: # 5 min
|
||||||
logging.debug(f"Suppressing re-arrival alert for {mac} (departed {int(time_since_departure)}s ago)")
|
logging.debug(f"Suppressing re-arrival alert for {mac} (departed {int(time_since_departure)}s ago)")
|
||||||
# Clean up departure timer if still pending
|
|
||||||
if mac in departure_timers:
|
|
||||||
departure_timers[mac].cancel()
|
|
||||||
del departure_timers[mac]
|
|
||||||
# Re-add to known_devices to track presence
|
# Re-add to known_devices to track presence
|
||||||
with known_lock:
|
with known_lock:
|
||||||
if mac not in known_devices:
|
if mac not in known_devices:
|
||||||
|
|||||||
@@ -221,6 +221,67 @@ def test_dhcp_renewal_dedup_30min():
|
|||||||
net_alerter.send_alert = original_send_alert
|
net_alerter.send_alert = original_send_alert
|
||||||
|
|
||||||
|
|
||||||
|
def test_re_arrival_cancels_departure_timer():
|
||||||
|
"""
|
||||||
|
Regression test: device departs → timer starts → device re-arrives BEFORE timer fires.
|
||||||
|
Timer should be cancelled and NO departure alert should be sent.
|
||||||
|
This tests the exact bug scenario: on_arrival() must cancel any pending timer.
|
||||||
|
"""
|
||||||
|
net_alerter.known_devices.clear()
|
||||||
|
net_alerter.departure_timers.clear()
|
||||||
|
net_alerter.last_departed_time.clear()
|
||||||
|
|
||||||
|
alert_calls = []
|
||||||
|
original_send_alert = net_alerter.send_alert
|
||||||
|
net_alerter.send_alert = lambda msg: alert_calls.append(msg)
|
||||||
|
|
||||||
|
try:
|
||||||
|
mac = "aa:bb:cc:dd:ee:ff"
|
||||||
|
ip = "10.0.0.0"
|
||||||
|
|
||||||
|
# Device exists in known_devices
|
||||||
|
now = time.time()
|
||||||
|
net_alerter.known_devices[mac] = {
|
||||||
|
"ip": ip,
|
||||||
|
"hostname": "testhost",
|
||||||
|
"vendor": "Test",
|
||||||
|
"first_seen": now,
|
||||||
|
"last_seen": now
|
||||||
|
}
|
||||||
|
|
||||||
|
# Trigger departure (starts 15-min debounce timer)
|
||||||
|
# At this point: timer created but last_departed_time NOT set yet
|
||||||
|
net_alerter.on_departure(mac)
|
||||||
|
|
||||||
|
# Verify timer was created
|
||||||
|
assert mac in net_alerter.departure_timers, "Departure timer should be created"
|
||||||
|
timer_ref = net_alerter.departure_timers[mac]
|
||||||
|
|
||||||
|
# Device re-arrives BEFORE timer fires (and BEFORE last_departed_time is set)
|
||||||
|
# This is the critical bug scenario: on_arrival must cancel the timer
|
||||||
|
net_alerter.on_arrival(mac, ip, "testhost")
|
||||||
|
|
||||||
|
# Timer should be cancelled (either not in dict or marked as cancelled)
|
||||||
|
assert mac not in net_alerter.departure_timers or not timer_ref.is_alive(), \
|
||||||
|
"Departure timer should be cancelled after re-arrival"
|
||||||
|
|
||||||
|
# NO departure alert should be sent (timer was cancelled before firing)
|
||||||
|
departure_alerts = [a for a in alert_calls if "DEPARTED" in a]
|
||||||
|
assert len(departure_alerts) == 0, \
|
||||||
|
f"No departure alert should be sent when device re-arrives before timer expires, got: {departure_alerts}"
|
||||||
|
|
||||||
|
# Re-arrival may or may not trigger ARRIVED alert depending on last_departed_time
|
||||||
|
# Since last_departed_time was not set (timer didn't fire yet), re-arrival will
|
||||||
|
# generate a new ARRIVED alert (not suppressed). This is correct behavior.
|
||||||
|
|
||||||
|
finally:
|
||||||
|
# Clean up
|
||||||
|
for timer in net_alerter.departure_timers.values():
|
||||||
|
timer.cancel()
|
||||||
|
net_alerter.departure_timers.clear()
|
||||||
|
net_alerter.send_alert = original_send_alert
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
test_hostname_dedup_when_hostname_equals_ip()
|
test_hostname_dedup_when_hostname_equals_ip()
|
||||||
print("✓ test_hostname_dedup_when_hostname_equals_ip")
|
print("✓ test_hostname_dedup_when_hostname_equals_ip")
|
||||||
@@ -249,4 +310,7 @@ if __name__ == "__main__":
|
|||||||
test_dhcp_renewal_dedup_30min()
|
test_dhcp_renewal_dedup_30min()
|
||||||
print("✓ test_dhcp_renewal_dedup_30min")
|
print("✓ test_dhcp_renewal_dedup_30min")
|
||||||
|
|
||||||
|
test_re_arrival_cancels_departure_timer()
|
||||||
|
print("✓ test_re_arrival_cancels_departure_timer")
|
||||||
|
|
||||||
print("\nAll tests passed!")
|
print("\nAll tests passed!")
|
||||||
|
|||||||
Reference in New Issue
Block a user