Skip to content

Commit 502e6e6

Browse files
Add ruff linting, fix UDP protected access, fix behavioral bugs
- Add ruff with ARG, PLR6301, SLF001 rules; remove unused args, make methods static where applicable. - UDP: add public properties (closed, uid, interfaces, tx_socks), public async_sendto and remove_subject_listener to eliminate all cross-class protected member access. - Fix lage clamping to [-1, 35] matching reference LAGE_MIN/MAX. - Fix gossip suppression: track periodic vs urgent gossip task, only suppress periodic (matching reference behavior). - Validate gossip hash vs name on implicit topic creation. - Remove unused function arguments across core modules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent c56db8e commit 502e6e6

File tree

6 files changed

+98
-52
lines changed

6 files changed

+98
-52
lines changed

CLAUDE.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,10 @@ Tests are x10+ the size of source code and must provide full coverage of the cor
6262
Transport test coverage is more opportunistic.
6363

6464
The library must ONLY be tested with Python versions starting from the minimum specified in `pyproject.toml`
65-
up to the current latest stable Python. TESTING ON UNSUPPORTED VERSIONS IS NOT ALLOWED.
65+
up to the current latest stable Python.
66+
TESTING ON UNSUPPORTED VERSIONS IS NOT ALLOWED.
6667

67-
Work will not be accepted unless `nox` (without arguments) runs successfully.
68+
ACCEPTANCE CRITERIA: Work will not be accepted unless `nox` (without arguments) runs successfully.
6869

6970
### Logging
7071

pyproject.toml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,19 +88,24 @@ asyncio_mode = "auto"
8888
[tool.ruff]
8989
line-length = 120
9090
target-version = "py312"
91+
preview = true
9192

9293
[tool.ruff.lint]
9394
# Only the checks the project needs right now; others disabled to avoid false positives.
9495
select = [
9596
"F401", # unused imports
9697
"F811", # redefinition of unused name
9798
"F841", # local variable assigned but never used
99+
"ARG001", # unused function argument
100+
"ARG002", # unused method argument
101+
"ARG005", # unused lambda argument
102+
"PLR6301", # method could be a function or static method
98103
"SLF001", # private member accessed from outside its class/module
99104
]
100105

101106
[tool.ruff.lint.per-file-ignores]
102107
# Tests may access internals for white-box testing; allow SLF001 there.
103-
"tests/*" = ["SLF001"]
108+
"tests/*" = ["SLF001", "ARG", "PLR6301"]
104109

105110
[tool.black]
106111
line-length = 120

src/pycyphal/_node.py

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,7 @@ def __init__(self, node: NodeImpl, name: str, evictions: int, now: float) -> Non
289289
self.publish_futures: dict[int, PublishTracker] = {}
290290
self.request_futures: dict[int, ResponseStreamImpl] = {} # tag -> ResponseStreamImpl
291291
self.gossip_task: asyncio.Task[None] | None = None
292+
self.gossip_task_is_periodic = False
292293
self.gossip_counter = 0
293294

294295
# -- Topic ABC --
@@ -313,10 +314,10 @@ def lage(self, now: float) -> int:
313314

314315
def merge_lage(self, now: float, remote_lage: int) -> None:
315316
"""Shift ts_origin backward if the remote claims an older origin."""
317+
clamped = max(-1, min(35, remote_lage)) # LAGE_MIN=-1, LAGE_MAX=35
316318
my_lage = self.lage(now)
317-
if remote_lage > my_lage:
318-
# Shift origin backward to match the remote's claim.
319-
self.ts_origin = now - (1 << remote_lage)
319+
if clamped > my_lage:
320+
self.ts_origin = now - (1 << clamped)
320321

321322
def animate(self, ts: float) -> None:
322323
self.ts_animated = ts
@@ -516,7 +517,7 @@ def topic_allocate(self, topic: TopicImpl, new_evictions: int, now: float) -> No
516517
t.release_transport_handles()
517518
t.evictions = ev
518519
t.ensure_listener()
519-
self.schedule_gossip_urgent(t, now)
520+
self.schedule_gossip_urgent(t)
520521
continue
521522

522523
modulus = self.transport.subject_id_modulus
@@ -532,7 +533,7 @@ def topic_allocate(self, topic: TopicImpl, new_evictions: int, now: float) -> No
532533
t.evictions = ev
533534
self.topics_by_subject_id[new_sid] = t
534535
t.ensure_listener()
535-
self.schedule_gossip_urgent(t, now)
536+
self.schedule_gossip_urgent(t)
536537
elif left_wins(t.lage(now), t.hash, collider.lage(now), collider.hash):
537538
# Our topic wins: take the slot, evict the collider.
538539
t.release_transport_handles()
@@ -544,15 +545,16 @@ def topic_allocate(self, topic: TopicImpl, new_evictions: int, now: float) -> No
544545
del self.topics_by_subject_id[new_sid]
545546
self.topics_by_subject_id[new_sid] = t
546547
t.ensure_listener()
547-
self.schedule_gossip_urgent(t, now)
548+
self.schedule_gossip_urgent(t)
548549
# Schedule collider for reallocation.
549550
collider.release_transport_handles()
550551
work.append((collider, collider.evictions + 1))
551552
else:
552553
# Our topic loses: increment evictions and retry.
553554
work.append((t, ev + 1))
554555

555-
def couple_topic_root(self, topic: TopicImpl, root: SubscriberRoot) -> None:
556+
@staticmethod
557+
def couple_topic_root(topic: TopicImpl, root: SubscriberRoot) -> None:
556558
"""Create a coupling between a topic and a subscriber root if not already coupled."""
557559
for c in topic.couplings:
558560
if c.root is root:
@@ -587,25 +589,29 @@ def schedule_gossip(self, topic: TopicImpl) -> None:
587589
"""Start periodic gossip for an explicit topic."""
588590
if topic.gossip_task is not None:
589591
return # already scheduled
592+
topic.gossip_task_is_periodic = True
590593
topic.gossip_task = self.loop.create_task(self.gossip_loop(topic))
591594

592-
def schedule_gossip_urgent(self, topic: TopicImpl, now: float) -> None:
595+
def schedule_gossip_urgent(self, topic: TopicImpl) -> None:
593596
"""Cancel current gossip and reschedule with short delay."""
594597
if topic.gossip_task is not None:
595598
topic.gossip_task.cancel()
599+
topic.gossip_task_is_periodic = False
596600
topic.gossip_task = self.loop.create_task(self.gossip_urgent(topic))
597601

598602
async def gossip_urgent(self, topic: TopicImpl) -> None:
599603
try:
600604
await asyncio.sleep(random.random() * GOSSIP_URGENT_DELAY_MAX)
601605
await self.send_gossip(topic, broadcast=True)
602606
# Resume periodic gossip.
607+
topic.gossip_task_is_periodic = True
603608
topic.gossip_task = self.loop.create_task(self.gossip_loop(topic))
604609
except asyncio.CancelledError:
605610
pass
606611

607612
async def gossip_loop(self, topic: TopicImpl) -> None:
608613
try:
614+
topic.gossip_task_is_periodic = True
609615
while not self._closed and not topic.is_implicit:
610616
dither = GOSSIP_PERIOD / GOSSIP_PERIOD_DITHER_RATIO
611617
delay = GOSSIP_PERIOD + random.uniform(-dither, dither)
@@ -696,15 +702,15 @@ def dispatch_arrival(self, arrival: TransportArrival, *, subject_id: int | None,
696702
payload = msg[HEADER_SIZE:]
697703

698704
if isinstance(hdr, (MsgBeHeader, MsgRelHeader)):
699-
self.on_msg(arrival, hdr, payload, unicast)
705+
self.on_msg(arrival, hdr, payload)
700706
elif isinstance(hdr, (MsgAckHeader, MsgNackHeader)):
701707
self.on_msg_ack(arrival, hdr)
702708
elif isinstance(hdr, (RspBeHeader, RspRelHeader)):
703709
self.on_rsp(arrival, hdr, payload)
704710
elif isinstance(hdr, (RspAckHeader, RspNackHeader)):
705711
self.on_rsp_ack(arrival, hdr)
706712
elif isinstance(hdr, GossipHeader):
707-
self.on_gossip(arrival, hdr, payload, unicast, subject_id)
713+
self.on_gossip(hdr, payload, unicast, subject_id)
708714
elif isinstance(hdr, ScoutHeader):
709715
self.on_scout(arrival, hdr, payload)
710716

@@ -713,7 +719,6 @@ def on_msg(
713719
arrival: TransportArrival,
714720
hdr: MsgBeHeader | MsgRelHeader,
715721
payload: bytes,
716-
unicast: bool,
717722
) -> None:
718723
topic = self.topics_by_hash.get(hdr.topic_hash)
719724
if topic is None:
@@ -754,8 +759,8 @@ def on_msg(
754759
)
755760
self.deliver_to_subscribers(topic, arrival, breadcrumb, payload, hdr.tag)
756761

762+
@staticmethod
757763
def deliver_to_subscribers(
758-
self,
759764
topic: TopicImpl,
760765
arrival: TransportArrival,
761766
breadcrumb: Breadcrumb,
@@ -873,7 +878,6 @@ async def do_send() -> None:
873878

874879
def on_gossip(
875880
self,
876-
arrival: TransportArrival,
877881
hdr: GossipHeader,
878882
payload: bytes,
879883
unicast: bool,
@@ -912,19 +916,26 @@ def on_gossip_known(
912916
win = my_lage > lage or (my_lage == lage and topic.evictions > evictions)
913917
topic.merge_lage(now, lage)
914918
if win:
915-
self.schedule_gossip_urgent(topic, now)
919+
self.schedule_gossip_urgent(topic)
916920
else:
917921
self.topic_allocate(topic, evictions, now)
918922
if topic.evictions == evictions:
919923
self.schedule_gossip(topic)
920924
else:
921925
topic.merge_lage(now, lage)
922-
# Suppress gossip if we can't contribute newer states.
926+
# Suppress gossip if we can't contribute newer states to the consensus process.
927+
# Only suppress if the current gossip task is periodic (not urgent) or the scope is broadcast.
928+
# Unicast gossips are seen by a small subset of nodes so they do not suppress.
923929
is_broadcast = subject_id == self.broadcast_subject_id
924930
is_sharded = not unicast and not is_broadcast
925-
suppress = (is_broadcast or is_sharded) and (topic.lage(now) == lage)
931+
suppress = (
932+
(is_broadcast or is_sharded)
933+
and (topic.lage(now) == lage)
934+
and (topic.gossip_task_is_periodic or is_broadcast)
935+
)
926936
if suppress and topic.gossip_task is not None:
927937
topic.gossip_task.cancel()
938+
topic.gossip_task_is_periodic = True
928939
topic.gossip_task = self.loop.create_task(self.gossip_loop(topic))
929940
topic.ensure_listener()
930941

@@ -936,18 +947,23 @@ def on_gossip_unknown(self, topic_hash: int, evictions: int, lage: int, now: flo
936947
return
937948
win = left_wins(mine.lage(now), mine.hash, lage, topic_hash)
938949
if win:
939-
self.schedule_gossip_urgent(mine, now)
950+
self.schedule_gossip_urgent(mine)
940951
else:
941952
self.topic_allocate(mine, mine.evictions + 1, now)
942953

943954
def topic_subscribe_if_matching(self, name: str, topic_hash: int, evictions: int, lage: int) -> TopicImpl | None:
944955
"""Create an implicit topic if any pattern subscriber matches the name."""
956+
# Validate that the hash matches the name to prevent corrupt gossip from creating inconsistencies.
957+
if rapidhash(name) != topic_hash:
958+
_logger.debug("Gossip hash mismatch for '%s': got %016x, expected %016x", name, topic_hash, rapidhash(name))
959+
return None
945960
for pattern, root in self.sub_roots_pattern.items():
946961
subs = match_pattern(pattern, name)
947962
if subs is not None:
948963
now = time.monotonic()
964+
clamped_lage = max(0, min(35, lage))
949965
topic = TopicImpl(self, name, evictions, now)
950-
topic.ts_origin = now - max(1, (1 << max(0, lage)))
966+
topic.ts_origin = now - max(1, (1 << clamped_lage))
951967
self.topics_by_name[name] = topic
952968
self.topics_by_hash[topic_hash] = topic
953969
self.topic_allocate(topic, evictions, now)

src/pycyphal/_subscriber.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ def deliver(self, arrival: Arrival, tag: int, remote_id: int) -> None:
140140
# Out-of-order but within capacity: intern.
141141
if lin_tag not in state.interned:
142142
state.interned[lin_tag] = InternedMsg(arrival=arrival, tag=tag, remote_id=remote_id, lin_tag=lin_tag)
143-
self._arm_reorder_timeout(state, key)
143+
self._arm_reorder_timeout(state)
144144
else:
145145
# Too far ahead: force-eject and resequence.
146146
self._force_eject_all(state)
@@ -176,7 +176,7 @@ def _force_eject_all(self, state: ReorderingState) -> None:
176176
state.timeout_handle.cancel()
177177
state.timeout_handle = None
178178

179-
def _arm_reorder_timeout(self, state: ReorderingState, key: tuple[int, int]) -> None:
179+
def _arm_reorder_timeout(self, state: ReorderingState) -> None:
180180
"""Arm or rearm the reordering timeout."""
181181
if state.timeout_handle is not None:
182182
return # already armed

src/pycyphal/udp.py

Lines changed: 46 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ def __init__(self, transport: UDPTransport, subject_id: int) -> None:
348348
async def __call__(self, deadline: Instant, priority: Priority, message: bytes | memoryview) -> None:
349349
if self._closed:
350350
raise SendError("Writer closed")
351-
if self._transport._closed:
351+
if self._transport.closed:
352352
raise SendError("Transport closed")
353353

354354
mcast_ip, port = make_subject_endpoint(self._subject_id)
@@ -358,12 +358,12 @@ async def __call__(self, deadline: Instant, priority: Priority, message: bytes |
358358

359359
errors: list[Exception] = []
360360
success_count = 0
361-
for i, iface in enumerate(self._transport._interfaces):
361+
for i, iface in enumerate(self._transport.interfaces):
362362
mtu = iface.mtu_cyphal
363-
frames = _segment_transfer(priority, transfer_id, self._transport._uid, message, mtu)
363+
frames = _segment_transfer(priority, transfer_id, self._transport.uid, message, mtu)
364364
try:
365365
for frame in frames:
366-
await self._transport._async_sendto(self._transport._tx_socks[i], frame, (mcast_ip, port), deadline)
366+
await self._transport.async_sendto(self._transport.tx_socks[i], frame, (mcast_ip, port), deadline)
367367
success_count += 1
368368
except (OSError, SendError) as e:
369369
errors.append(e)
@@ -400,21 +400,7 @@ def close(self) -> None:
400400
return
401401
self._closed = True
402402
_logger.info("Subject listener closed for subject %d", self._subject_id)
403-
handlers = self._transport._subject_handlers.get(self._subject_id, [])
404-
if self._handler in handlers:
405-
handlers.remove(self._handler)
406-
if not handlers:
407-
# No more listeners for this subject -- clean up sockets and tasks
408-
self._transport._subject_handlers.pop(self._subject_id, None)
409-
self._transport._reassemblers.pop(self._subject_id, None)
410-
for i in range(len(self._transport._interfaces)):
411-
key = (self._subject_id, i)
412-
task = self._transport._mcast_rx_tasks.pop(key, None)
413-
if task is not None:
414-
task.cancel()
415-
sock = self._transport._mcast_socks.pop(key, None)
416-
if sock is not None:
417-
sock.close()
403+
self._transport.remove_subject_listener(self._subject_id, self._handler)
418404

419405

420406
# =====================================================================================================================
@@ -544,9 +530,48 @@ def _create_mcast_socket(subject_id: int, iface: Interface) -> socket.socket:
544530
_logger.info("Multicast socket for subject %d on %s (%s:%d)", subject_id, iface.address, mcast_ip, port)
545531
return sock
546532

533+
# -- Public accessors for internal classes --
534+
535+
@property
536+
def closed(self) -> bool:
537+
return self._closed
538+
539+
@property
540+
def uid(self) -> int:
541+
assert self._uid is not None
542+
return self._uid
543+
544+
@property
545+
def interfaces(self) -> list[Interface]:
546+
return self._interfaces
547+
548+
@property
549+
def tx_socks(self) -> list[socket.socket]:
550+
return self._tx_socks
551+
552+
def remove_subject_listener(self, subject_id: int, handler: Callable[[TransportArrival], None]) -> None:
553+
"""
554+
Remove a handler for a subject; clean up sockets/tasks if no handlers remain.
555+
Internal use only.
556+
"""
557+
handlers = self._subject_handlers.get(subject_id, [])
558+
if handler in handlers:
559+
handlers.remove(handler)
560+
if not handlers:
561+
self._subject_handlers.pop(subject_id, None)
562+
self._reassemblers.pop(subject_id, None)
563+
for i in range(len(self._interfaces)):
564+
key = (subject_id, i)
565+
task = self._mcast_rx_tasks.pop(key, None)
566+
if task is not None:
567+
task.cancel()
568+
sock = self._mcast_socks.pop(key, None)
569+
if sock is not None:
570+
sock.close()
571+
547572
# -- Async sendto helper --
548573

549-
async def _async_sendto(self, sock: socket.socket, data: bytes, addr: tuple[str, int], deadline: Instant) -> None:
574+
async def async_sendto(self, sock: socket.socket, data: bytes, addr: tuple[str, int], deadline: Instant) -> None:
550575
"""Send a UDP datagram, suspending until writable or deadline exceeded."""
551576
remaining_ns = deadline.ns - Instant.now().ns
552577
if remaining_ns <= 0:
@@ -600,7 +625,7 @@ async def unicast(self, deadline: Instant, priority: Priority, remote_id: int, m
600625
frames = _segment_transfer(priority, transfer_id, self._uid, message, iface.mtu_cyphal)
601626
try:
602627
for frame in frames:
603-
await self._async_sendto(self._tx_socks[i], frame, ep, deadline)
628+
await self.async_sendto(self._tx_socks[i], frame, ep, deadline)
604629
success_count += 1
605630
except (OSError, SendError) as e:
606631
errors.append(e)

0 commit comments

Comments
 (0)