Merge af886466d5 into fbf84cbdac

src/meshcore/commands/base.py

@@ -58,7 +58,7 @@ def _validate_destination(dst: DestinationType, prefix_length: int = 6) -> bytes
 
 
 class CommandHandlerBase:
-    DEFAULT_TIMEOUT = 5.0
+    DEFAULT_TIMEOUT = 15.0
 
     def __init__(self, default_timeout: Optional[float] = None):
         self._sender_func: Optional[Callable[[bytes], Coroutine[Any, Any, None]]] = None
@@ -243,18 +243,31 @@ class CommandHandlerBase:
         logger.debug(f"Binary request to {dst_bytes.hex()}")
         data = b"\x32" + dst_bytes + request_type.value.to_bytes(1, "little", signed=False) + (data if data else b"")
 
-        result = await self.send(data, [EventType.MSG_SENT, EventType.ERROR])
-        
-        # Register the request with the reader if we have both reader and request_type
-        if (result.type == EventType.MSG_SENT and 
-            self._reader is not None and 
-            request_type is not None):
-            
-            exp_tag = result.payload["expected_ack"].hex()
-            # Use provided timeout or fallback to suggested timeout (with 5s default)
-            actual_timeout = timeout if timeout is not None and timeout > 0 else result.payload.get("suggested_timeout", 4000) / 800.0
+        # Pre-register a placeholder binary request before send() to close the race
+        # window where a BINARY_RESPONSE could arrive between send() returning and
+        # registration. The placeholder tag is patched to the real tag once MSG_SENT
+        # returns. If send() fails, the placeholder is cleaned up.
+        placeholder_tag = None
+        if self._reader is not None and request_type is not None:
+            placeholder_tag = f"_pending_{id(data)}"
+            actual_timeout = timeout if timeout is not None and timeout > 0 else self.default_timeout
             actual_timeout = min_timeout if actual_timeout < min_timeout else actual_timeout
-            self._reader.register_binary_request(pubkey_prefix.hex(), exp_tag, request_type, actual_timeout, context=context)
+            self._reader.register_binary_request(pubkey_prefix.hex(), placeholder_tag, request_type, actual_timeout, context=context)
+
+        result = await self.send(data, [EventType.MSG_SENT, EventType.ERROR])
+
+        # Patch the placeholder tag with the real tag from MSG_SENT, or clean up on failure
+        if placeholder_tag is not None and self._reader is not None:
+            # Remove the placeholder entry
+            self._reader.pending_binary_requests.pop(placeholder_tag, None)
+            if (result.type == EventType.MSG_SENT and
+                    request_type is not None):
+                exp_tag = result.payload["expected_ack"].hex()
+                # Use suggested_timeout from the result if available
+                actual_timeout = timeout if timeout is not None and timeout > 0 else result.payload.get("suggested_timeout", 4000) / 800.0
+                actual_timeout = min_timeout if actual_timeout < min_timeout else actual_timeout
+                # Register with the real tag
+                self._reader.register_binary_request(pubkey_prefix.hex(), exp_tag, request_type, actual_timeout, context=context)
 
         return result
 

src/meshcore/commands/binary.py

@@ -73,10 +73,6 @@ class BinaryCommandHandler(CommandHandlerBase):
 
             return telem_event.payload["lpp"] if telem_event else None
 
-    async def req_mma(self, contact, timeout=0, min_timeout=0):
-        logger.error("*** please consider using req_mma_sync instead of req_mma") 
-        return await self.req_mma_sync(contact, start, end, timeout,min_timeout)
-
     async def req_mma_sync(self, contact, start, end, timeout=0,min_timeout=0):
         async with self._mesh_request_lock:
             req = (

src/meshcore/commands/contact.py

@@ -43,13 +43,17 @@ class ContactCommands(CommandHandlerBase):
                     logger.debug("Timeout while getting contacts")
                     for future in pending: # cancel all futures
                         future.cancel()
-                    return None
+                    return Event(EventType.ERROR, {"reason": "timeout waiting for contacts"})
 
                 for future in done:
                     event = await future
-                    if event is None or event.type != EventType.NEXT_CONTACT:
-                        for future in pending:
-                            future.cancel()
+                    if event is None:
+                        for f in pending:
+                            f.cancel()
+                        return Event(EventType.ERROR, {"reason": "no event received during contacts retrieval"})
+                    if event.type != EventType.NEXT_CONTACT:
+                        for f in pending:
+                            f.cancel()
                         return event
 
                 futures = []
@@ -64,7 +68,7 @@ class ContactCommands(CommandHandlerBase):
     
         except asyncio.TimeoutError:
             logger.debug(f"Timeout receiving contacts")
-            return None
+            return Event(EventType.ERROR, {"reason": "asyncio timeout receiving contacts"})
         except Exception as e:
             logger.debug(f"Command error: {e}")
             return Event(EventType.ERROR, {"error": str(e)})
@@ -116,7 +120,9 @@ class ContactCommands(CommandHandlerBase):
                     path_hash_mode = int(path.split(":")[1])
                     path = path.split(":")[0].replace(":","")
                 else: # use device one by default
-                    path_hash_mode = contact["out_path_len"] >> 6 # would fallback to previous val
+                    # out_path_len is pre-masked (& 0x3F) in reader.py, so high bits are always 0;
+                    # the actual path_hash_mode is fetched from the device query below.
+                    path_hash_mode = 0
                     res = await self.send_device_query()
                     if not res is None and res.type != EventType.ERROR:
                         if "path_hash_mode" in res.payload:

src/meshcore/commands/messaging.py

@@ -313,6 +313,8 @@ class MessagingCommands(CommandHandlerBase):
         elif isinstance (scope, bytes): # scope has been sent directly as byte
                 logger.debug(f"Directly setting scope to {scope}")
                 scope_key = scope
+        else:
+            raise TypeError(f"set_flood_scope: unsupported scope type {type(scope).__name__}")
 
         logger.debug(f"Setting scope to {scope_key.hex()}")
 

src/meshcore/meshcore.py

@@ -1,6 +1,6 @@
 import asyncio
 import logging
-from typing import Any, Callable, Coroutine, Dict, Optional, Union
+from typing import Any, Callable, Dict, Optional, Union
 
 from .events import Event, EventDispatcher, EventType, Subscription
 from .reader import MessageReader
@@ -206,7 +206,7 @@ class MeshCore:
     def subscribe(
         self,
         event_type: Union[EventType, None],
-        callback: Callable[[Event], Coroutine[Any, Any, None]],
+        callback: Callable[[Event], Union[None, asyncio.Future]],
         attribute_filters: Optional[Dict[str, Any]] = None,
     ) -> Subscription:
         """

tests/unit/test_standalone_fixes.py

@@ -0,0 +1,227 @@
+"""
+Verification tests for standalone bug fixes and cleanup.
+"""
+
+import pytest
+import asyncio
+import inspect
+from unittest.mock import AsyncMock, MagicMock, patch
+
+from meshcore.events import Event, EventDispatcher, EventType
+from meshcore.commands.base import CommandHandlerBase
+from meshcore.commands.binary import BinaryCommandHandler
+from meshcore.commands.messaging import MessagingCommands
+from meshcore.commands.contact import ContactCommands
+from meshcore.meshcore import MeshCore
+
+pytestmark = pytest.mark.asyncio
+
+
+# ── req_mma removed ──────────────────────────────────────────────────────
+
+def test_req_mma_removed():
+    """The broken req_mma method should no longer exist on BinaryCommandHandler."""
+    assert not hasattr(BinaryCommandHandler, "req_mma"), \
+        "req_mma should be removed — it had NameError on undefined start/end"
+
+
+def test_req_mma_sync_still_exists():
+    """req_mma_sync should still be present and functional."""
+    assert hasattr(BinaryCommandHandler, "req_mma_sync"), \
+        "req_mma_sync should still exist after removing req_mma"
+
+
+# ── DEFAULT_TIMEOUT bumped ───────────────────────────────────────────────
+
+def test_default_timeout_bumped():
+    """DEFAULT_TIMEOUT should be 15.0, not the old 5.0."""
+    assert CommandHandlerBase.DEFAULT_TIMEOUT == 15.0, \
+        f"DEFAULT_TIMEOUT is {CommandHandlerBase.DEFAULT_TIMEOUT}, expected 15.0"
+
+
+def test_instance_default_timeout():
+    """Instance default_timeout should inherit the new 15.0 value."""
+    handler = CommandHandlerBase()
+    assert handler.default_timeout == 15.0
+
+
+def test_custom_timeout_still_works():
+    """Passing a custom timeout should still override the default."""
+    handler = CommandHandlerBase(default_timeout=30.0)
+    assert handler.default_timeout == 30.0
+
+
+# ── set_flood_scope TypeError guard ──────────────────────────────────────
+
+async def test_set_flood_scope_bad_type_raises():
+    """Passing an unsupported type (e.g., int) should raise TypeError."""
+    handler = MessagingCommands()
+    with pytest.raises(TypeError, match="unsupported scope type"):
+        await handler.set_flood_scope(42)
+
+
+async def test_set_flood_scope_bad_type_bytearray():
+    """bytearray is not bytes — should raise TypeError."""
+    handler = MessagingCommands()
+    with pytest.raises(TypeError, match="unsupported scope type"):
+        await handler.set_flood_scope(bytearray(b"\x00" * 16))
+
+
+async def test_set_flood_scope_none_still_works():
+    """None scope should reach send() without TypeError — verifies the None branch still binds scope_key."""
+    handler = MessagingCommands()
+    handler._sender_func = AsyncMock()
+    handler.dispatcher = EventDispatcher()
+    await handler.dispatcher.start()
+    try:
+        # Dispatch an OK event so send() resolves
+        async def _dispatch_ok():
+            await asyncio.sleep(0.05)
+            await handler.dispatcher.dispatch(Event(EventType.OK, {}))
+        asyncio.ensure_future(_dispatch_ok())
+        result = await handler.set_flood_scope(None)
+        assert result.type == EventType.OK
+    finally:
+        handler.dispatcher.running = False
+
+
+async def test_set_flood_scope_str_still_works():
+    """String scope should reach send() without TypeError."""
+    handler = MessagingCommands()
+    handler._sender_func = AsyncMock()
+    handler.dispatcher = EventDispatcher()
+    await handler.dispatcher.start()
+    try:
+        async def _dispatch_ok():
+            await asyncio.sleep(0.05)
+            await handler.dispatcher.dispatch(Event(EventType.OK, {}))
+        asyncio.ensure_future(_dispatch_ok())
+        result = await handler.set_flood_scope("#test")
+        assert result.type == EventType.OK
+    finally:
+        handler.dispatcher.running = False
+
+
+async def test_set_flood_scope_bytes_still_works():
+    """Bytes scope should reach send() without TypeError."""
+    handler = MessagingCommands()
+    handler._sender_func = AsyncMock()
+    handler.dispatcher = EventDispatcher()
+    await handler.dispatcher.start()
+    try:
+        async def _dispatch_ok():
+            await asyncio.sleep(0.05)
+            await handler.dispatcher.dispatch(Event(EventType.OK, {}))
+        asyncio.ensure_future(_dispatch_ok())
+        result = await handler.set_flood_scope(b"\x01" * 16)
+        assert result.type == EventType.OK
+    finally:
+        handler.dispatcher.running = False
+
+
+# ── dead path_hash_mode shift removed ────────────────────────────────────
+
+def test_no_shift_in_update_contact():
+    """The dead `>> 6` shift on out_path_len should not appear in contact.py."""
+    import meshcore.commands.contact as contact_mod
+    source = inspect.getsource(contact_mod.ContactCommands.update_contact)
+    assert ">> 6" not in source, \
+        "Dead path_hash_mode = out_path_len >> 6 shift should be removed"
+
+
+# ── get_contacts returns Event, never None ───────────────────────────────
+
+async def test_get_contacts_timeout_returns_error_event():
+    """On timeout (no futures complete), get_contacts should return an Error Event, not None."""
+    handler = ContactCommands()
+    handler._sender_func = AsyncMock()
+    handler._reader = MagicMock()
+    handler.dispatcher = MagicMock()
+    # Make wait_for_event always timeout by never returning
+    handler.dispatcher.wait_for_event = AsyncMock(side_effect=asyncio.TimeoutError)
+
+    result = await handler.get_contacts(timeout=0.1)
+    assert result is not None, "get_contacts should never return None"
+    assert isinstance(result, Event)
+    assert result.type == EventType.ERROR
+
+
+# ── binary request pre-registration ──────────────────────────────────────
+
+async def test_placeholder_registered_before_send():
+    """A placeholder binary request should be registered before send() is called."""
+    from meshcore.packets import BinaryReqType
+
+    handler = CommandHandlerBase()
+    handler._sender_func = AsyncMock()
+
+    # Track registration calls
+    mock_reader = MagicMock()
+    mock_reader.pending_binary_requests = {}
+    original_register = MagicMock()
+
+    registration_order = []
+    send_called = False
+
+    async def mock_send(data):
+        nonlocal send_called
+        # At the point send() is called, a placeholder should already exist
+        registration_order.append(("send", len(mock_reader.pending_binary_requests)))
+        send_called = True
+
+    handler._sender_func = mock_send
+    handler._reader = mock_reader
+    handler.dispatcher = MagicMock()
+    handler.dispatcher.wait_for_event = AsyncMock(
+        return_value=Event(EventType.MSG_SENT, {"expected_ack": b"\x01\x02\x03\x04"})
+    )
+
+    # Resolve subscribed events immediately so send() doesn't block.
+    # Use MSG_SENT with expected_ack because send_binary_req reads that key.
+    def resolving_subscribe(event_type, cb, attribute_filters=None):
+        sub = MagicMock()
+        sub.unsubscribe = MagicMock()
+        payload = {"expected_ack": b"\x01\x02\x03\x04"} if event_type == EventType.MSG_SENT else {}
+        asyncio.get_event_loop().call_soon(
+            cb, Event(event_type, payload)
+        )
+        return sub
+    handler.dispatcher.subscribe = MagicMock(side_effect=resolving_subscribe)
+
+    # Call send_binary_req
+    dst = "aa" * 32  # 32-byte hex pubkey
+    await handler.send_binary_req(dst, BinaryReqType.MMA)
+
+    # Verify register_binary_request was called (at least the placeholder)
+    assert mock_reader.register_binary_request.call_count >= 1, \
+        "register_binary_request should be called at least once for the placeholder"
+
+
+# ── MeshCore.subscribe annotation matches EventDispatcher ────────────────
+
+def test_subscribe_annotation_matches_dispatcher():
+    """MeshCore.subscribe callback annotation should match EventDispatcher.subscribe."""
+    mc_hints = MeshCore.subscribe.__annotations__
+    ed_hints = EventDispatcher.subscribe.__annotations__
+
+    # Both should have 'callback' in their annotations
+    assert "callback" in mc_hints, "MeshCore.subscribe missing callback annotation"
+    assert "callback" in ed_hints, "EventDispatcher.subscribe missing callback annotation"
+
+    # The callback annotations should be identical
+    assert mc_hints["callback"] == ed_hints["callback"], (
+        f"MeshCore.subscribe callback annotation {mc_hints['callback']} "
+        f"does not match EventDispatcher.subscribe {ed_hints['callback']}"
+    )
+
+
+def test_no_coroutine_import_in_meshcore():
+    """After widening the annotation, Coroutine should no longer be imported in meshcore.py."""
+    import meshcore.meshcore as mc_mod
+    source = inspect.getsource(mc_mod)
+    # Check the import line specifically — Coroutine should not be in the typing imports
+    for line in source.splitlines():
+        if line.startswith("from typing import"):
+            assert "Coroutine" not in line, \
+                "Coroutine should be removed from typing imports in meshcore.py"
+            break