mirror of
https://github.com/jkingsman/Remote-Terminal-for-MeshCore.git
synced 2026-04-30 10:35:17 +02:00
Fix TOCTOU around radio reconnect
This commit is contained in:
11
app/radio.py
11
app/radio.py
@@ -399,12 +399,13 @@ class RadioManager:
|
||||
if self._reconnect_lock is None:
|
||||
self._reconnect_lock = asyncio.Lock()
|
||||
|
||||
# Try to acquire lock without blocking to check if reconnect is in progress
|
||||
if self._reconnect_lock.locked():
|
||||
logger.debug("Reconnection already in progress")
|
||||
return False
|
||||
|
||||
async with self._reconnect_lock:
|
||||
# If we became connected while waiting for the lock (another
|
||||
# reconnect succeeded ahead of us), skip the redundant attempt.
|
||||
if self.is_connected:
|
||||
logger.debug("Already connected after acquiring lock, skipping reconnect")
|
||||
return True
|
||||
|
||||
logger.info("Attempting to reconnect to radio...")
|
||||
|
||||
try:
|
||||
|
||||
@@ -261,7 +261,16 @@ export function MessageList({
|
||||
};
|
||||
}, [messages, onResendChannelMessage]);
|
||||
|
||||
// Refs for scroll handler to read without causing callback recreation
|
||||
const onLoadOlderRef = useRef(onLoadOlder);
|
||||
const loadingOlderRef = useRef(loadingOlder);
|
||||
const hasOlderMessagesRef = useRef(hasOlderMessages);
|
||||
onLoadOlderRef.current = onLoadOlder;
|
||||
loadingOlderRef.current = loadingOlder;
|
||||
hasOlderMessagesRef.current = hasOlderMessages;
|
||||
|
||||
// Handle scroll - capture state and detect when user is near top/bottom
|
||||
// Stable callback: reads changing values from refs, never recreated.
|
||||
const handleScroll = useCallback(() => {
|
||||
if (!listRef.current) return;
|
||||
|
||||
@@ -280,13 +289,13 @@ export function MessageList({
|
||||
// Show scroll-to-bottom button when not near the bottom (more than 100px away)
|
||||
setShowScrollToBottom(distanceFromBottom > 100);
|
||||
|
||||
if (!onLoadOlder || loadingOlder || !hasOlderMessages) return;
|
||||
if (!onLoadOlderRef.current || loadingOlderRef.current || !hasOlderMessagesRef.current) return;
|
||||
|
||||
// Trigger load when within 100px of top
|
||||
if (scrollTop < 100) {
|
||||
onLoadOlder();
|
||||
onLoadOlderRef.current();
|
||||
}
|
||||
}, [onLoadOlder, loadingOlder, hasOlderMessages]);
|
||||
}, []);
|
||||
|
||||
// Scroll to bottom handler
|
||||
const scrollToBottom = useCallback(() => {
|
||||
|
||||
@@ -188,6 +188,12 @@ export function RawPacketList({ packets }: RawPacketListProps) {
|
||||
}));
|
||||
}, [packets]);
|
||||
|
||||
// Sort packets by timestamp ascending (oldest first)
|
||||
const sortedPackets = useMemo(
|
||||
() => [...decodedPackets].sort((a, b) => a.packet.timestamp - b.packet.timestamp),
|
||||
[decodedPackets]
|
||||
);
|
||||
|
||||
useEffect(() => {
|
||||
if (listRef.current) {
|
||||
listRef.current.scrollTop = listRef.current.scrollHeight;
|
||||
@@ -202,9 +208,6 @@ export function RawPacketList({ packets }: RawPacketListProps) {
|
||||
);
|
||||
}
|
||||
|
||||
// Sort packets by timestamp ascending (oldest first)
|
||||
const sortedPackets = [...decodedPackets].sort((a, b) => a.packet.timestamp - b.packet.timestamp);
|
||||
|
||||
return (
|
||||
<div className="h-full overflow-y-auto p-4 flex flex-col gap-2" ref={listRef}>
|
||||
{sortedPackets.map(({ packet, decoded }) => (
|
||||
|
||||
@@ -222,6 +222,114 @@ class TestConnectionMonitor:
|
||||
assert rm._last_connected is False
|
||||
|
||||
|
||||
class TestReconnectLock:
|
||||
"""Tests for reconnect() lock serialization — no duplicate reconnections."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_concurrent_reconnects_only_connect_once(self):
|
||||
"""Two concurrent reconnect() calls should only call connect() once."""
|
||||
from app.radio import RadioManager
|
||||
|
||||
rm = RadioManager()
|
||||
rm._meshcore = None
|
||||
|
||||
connect_count = 0
|
||||
|
||||
async def mock_connect():
|
||||
nonlocal connect_count
|
||||
connect_count += 1
|
||||
# Simulate connect taking some time
|
||||
await asyncio.sleep(0.05)
|
||||
mock_mc = MagicMock()
|
||||
mock_mc.is_connected = True
|
||||
rm._meshcore = mock_mc
|
||||
rm._connection_info = "TCP: test:4000"
|
||||
|
||||
rm.connect = AsyncMock(side_effect=mock_connect)
|
||||
|
||||
with (
|
||||
patch("app.websocket.broadcast_health"),
|
||||
patch("app.websocket.broadcast_error"),
|
||||
):
|
||||
result_a, result_b = await asyncio.gather(
|
||||
rm.reconnect(broadcast_on_success=False),
|
||||
rm.reconnect(broadcast_on_success=False),
|
||||
)
|
||||
|
||||
# First caller does the real connect, second sees is_connected=True
|
||||
assert connect_count == 1
|
||||
assert result_a is True
|
||||
assert result_b is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_second_reconnect_skips_when_first_succeeds(self):
|
||||
"""Second caller returns True without connecting when first already succeeded."""
|
||||
from app.radio import RadioManager
|
||||
|
||||
rm = RadioManager()
|
||||
rm._meshcore = None
|
||||
|
||||
call_order: list[str] = []
|
||||
|
||||
async def mock_connect():
|
||||
call_order.append("connect")
|
||||
await asyncio.sleep(0.05)
|
||||
mock_mc = MagicMock()
|
||||
mock_mc.is_connected = True
|
||||
rm._meshcore = mock_mc
|
||||
rm._connection_info = "TCP: test:4000"
|
||||
|
||||
rm.connect = AsyncMock(side_effect=mock_connect)
|
||||
|
||||
with (
|
||||
patch("app.websocket.broadcast_health"),
|
||||
patch("app.websocket.broadcast_error"),
|
||||
):
|
||||
await asyncio.gather(
|
||||
rm.reconnect(broadcast_on_success=False),
|
||||
rm.reconnect(broadcast_on_success=False),
|
||||
)
|
||||
|
||||
# connect should appear exactly once
|
||||
assert call_order == ["connect"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reconnect_retries_after_first_failure(self):
|
||||
"""If first reconnect fails, a subsequent call should attempt connect again."""
|
||||
from app.radio import RadioManager
|
||||
|
||||
rm = RadioManager()
|
||||
rm._meshcore = None
|
||||
|
||||
attempt = 0
|
||||
|
||||
async def mock_connect():
|
||||
nonlocal attempt
|
||||
attempt += 1
|
||||
if attempt == 1:
|
||||
# First attempt fails
|
||||
return
|
||||
# Second attempt succeeds
|
||||
mock_mc = MagicMock()
|
||||
mock_mc.is_connected = True
|
||||
rm._meshcore = mock_mc
|
||||
rm._connection_info = "TCP: test:4000"
|
||||
|
||||
rm.connect = AsyncMock(side_effect=mock_connect)
|
||||
|
||||
with (
|
||||
patch("app.websocket.broadcast_health"),
|
||||
patch("app.websocket.broadcast_error"),
|
||||
):
|
||||
result1 = await rm.reconnect(broadcast_on_success=False)
|
||||
assert result1 is False
|
||||
assert attempt == 1
|
||||
|
||||
result2 = await rm.reconnect(broadcast_on_success=False)
|
||||
assert result2 is True
|
||||
assert attempt == 2
|
||||
|
||||
|
||||
class TestSerialDeviceProbe:
|
||||
"""Tests for test_serial_device() — verifies cleanup on all exit paths."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user