Correctness and robustness fixes from a deep audit against the C reference#394
Merged
Conversation
Review finding #1 (High). Two defects combined into a remotely triggerable, permanent DoS on Cyphal/CAN: - The pin-suffix parser accepted any str.isdigit() character as a pin digit, so non-ASCII digits like '²' (U+00B2) reached int() and raised ValueError on the RX dispatch path, before the printable-ASCII check could reject the name. Pin digits are now gated on ASCII '0'..'9' exactly like the reference (cy.c name_consume_pin_suffix), and _is_valid_wire_name runs the ASCII check before the pin parse. - The CAN reader loop invoked _ingest_frame outside its try block, so any exception escaping the session-layer pipeline silently killed that interface's reception permanently. The ingest call is now guarded the same way the UDP RX loops guard their handler calls: log and keep serving. Regression tests: crafted 'x#²' gossip through the session layer, parser totality on Unicode digits, CAN reader survival with a raising handler, and UDP per-datagram fault-boundary containment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #14 (Medium). The reference computes (hash + evictions²) % modulus in uint64 arithmetic, so the sum wraps at 2^64 before reduction; Python computed the exact value with big integers. The results differ whenever hash + evictions² >= 2^64, and since the eviction counter is an untrusted uint32 gossip field installed verbatim by a winning known-topic gossip, the divergence is remotely constructible: Python and C nodes sharing such a topic would compute different subject-IDs and silently stop hearing each other. Reference: cy.c topic_subject_id_impl. The test that pinned the non-wrapping behavior now asserts bit-for-bit equality with the reference formula. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
…ference Review finding #15 (Low). The reference selects the right neighbor with cavl2_predecessor, an inclusive floor (offset <= right), so a fragment beginning exactly at the new fragment's end participates in overlap eviction; Python used a strict '<'. The divergence is observable only when overlapping fragments carry conflicting data (corruption or adversarial injection): the reference evicts the conflicting fragment and delivers the transfer, while Python kept it and dropped the transfer on CRC failure. Reference: udpard.c rx_fragment_tree_update. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #16 (Low). The history seed (transfer_id - 1) was masked to 48 bits, so a first-seen transfer-ID of 0 produced 0xFFFF_FFFF_FFFF - a valid wire transfer-ID - and a genuinely late transfer with that ID was wrongly treated as already ejected. The reference stores the unmasked uint64 value (2^64-1 for ID 0), which no 48-bit wire ID can match. Reference: udpard.c rx_port_push first-frame history initialization. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #17 (Low). libcanard destroys a slot-free session once last_admission_ts falls behind the 2 s transfer-ID timeout (canard_poll); the 30 s retention window applies only to the slots themselves (rx_session_cleanup). Python retained slot-free sessions for the full 30 s, so during a redundant-bus failover a transfer reusing the last-admitted transfer-ID+priority from a different interface could be dropped for up to ~28 s longer than in the reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #18 (Low), with the corrected model: FD/Classic is a property of the interface, fixed at construction - not per transfer and not per frame. Previously both backends decided per frame by payload length (len > 8), so the short/tail frames of an FD transfer went out as Classic frames, and python-can additionally forced bitrate_switch, which the reference never sets and a nominal-bitrate-only FD network may reject. Now every frame on an FD interface is an FD frame (FDF only) and every frame on a Classic interface is a Classic frame, matching the reference (cy_can_socketcan selects the FD/Classic vtable once from the netdev MTU and emits all frames accordingly, no BRS). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #19 (Low). The reference treats '*'/'>' as substitution tokens only when a whole '/'-segment equals them (wkv_has_substitution_tokens), so a name like 'ab*cd' is a legal verbatim topic in C. Python classified any name containing those characters as a pattern, so such names could be neither advertised nor joined from gossip - a Python-to-C interop gap. Classification is now whole-segment on both the resolve path and gossip wire-name acceptance. The match semantics keep the documented terminal-only '>' REFERENCE PARITY deviation; classification follows the reference exactly, so off-terminal whole-segment '>' names remain patterns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #12 (Medium). Node-ID 0 is a valid regular node in Cyphal/CAN v1 (only v0/DroneCAN treated 0 as anonymous; libcanard permits a remote 0 and a service destination 0), and the wire encoder already accepts it, but unicast() guarded 1 <= remote_id, so the ACK path could not answer a node-0 peer and reliable delivery from such a peer never completed. The destination range is now 0..127. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #8 (Medium). The SLCAN 't' command (11-bit standard-ID data frame) was decoded into an ordinary Frame, indistinguishable from an extended frame with a small ID because Frame carries no IDE flag - in violation of the extended-only Interface contract that the SocketCAN and python-can backends enforce. On a mixed bus a standard frame could parse as a bogus transfer or even trigger a spurious node-ID collision re-roll. Standard-ID 't' lines are now dropped alongside the already-dropped 'r'/'R' remote frames; the reference consumes only CAN_EFF_FLAG frames. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #13 (Medium). The reference validates every received datagram's ingress interface index via recvmsg+IP_PKTINFO and drops mismatches (udp_wrapper.c). Python's RX loops attribute every datagram to the socket's configured interface, and on multi-homed Linux hosts the default IP_MULTICAST_ALL=1 delivers datagrams of groups joined on *other* interfaces to this socket too, mislearning (uid, iface) reverse routes and defeating independent-path failover. asyncio offers no sock_recvmsg (and Windows lacks socket.recvmsg entirely), so the fix uses the kernel-level equivalent: IP_MULTICAST_ALL=0 makes the socket receive only datagrams matching its own explicit (group, interface) membership, which yields the same delivery set as the reference's ipi_ifindex filter. macOS/BSD scope multicast delivery per membership natively; the Windows single-NIC-per-multicast-network assumption is documented at the socket setup site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #2 (Medium). The transports accepted any modulus in range, but the quadratic probe (hash + evictions²) mod m covers the residue space only when the modulus is at least 57203, prime, and congruent to 3 modulo 4 (cy.c is_valid_subject_id_modulus). A degenerate modulus made the synchronous displacement loop in topic_allocate iterate billions of times - a hard event-loop stall that asyncio.wait_for cannot cancel. The predicate is now enforced at node construction, before any resource is acquired, so the three published moduli pass and a degenerate value is rejected cleanly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #10 (Medium). SocketCAN receive() awaited loop.sock_recv directly, and close()/_fail() closed the socket without waking a reader already parked there - the selector loop does not wake a bare sock_recv on socket.close(), so the transport's reader stayed hung in receive() forever and interface loss (no reception, no interface removal) was never propagated. RX now runs in its own task feeding a queue, and close() pushes a ClosedError sentinel that unblocks a parked receive(), mirroring the python-can and webserial backends. The socket is closed last so the cancelled reader task deregisters cleanly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review findings #6, #7, #9 (one restructure of the send path). - #6: the subject writer and unicast sent to interfaces serially against one shared deadline, so a congested first interface could consume the whole deadline and starve a healthy redundant one. Interfaces are now sent to concurrently via asyncio.gather; each interface's frames still go out in order. - #7: concurrent senders shared one TX socket per interface, but asyncio's selector loop permits only one writer callback per fd, so an overlapping sock_sendto could be displaced and hang. Each socket now has its own asyncio.Lock; a new send_on_iface helper holds it for the transfer. - #9: a send suspended mid-transfer while close() emptied _tx_socks used to index the emptied list and raise IndexError. The send path now snapshots the (iface, socket, lock) elements before the first await, so a racing close leaves the send index-safe; it fails cleanly on the closed socket and is aggregated into a SendError. Aggregation semantics are unchanged: success means all frames delivered on at least one interface; a partial failure warns rather than raising. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
The modulus validation added for finding #2 rejects moduli that are not >= 57203, prime, and ≡ 3 mod 4 at node construction. Two existing tests built nodes with test-convenient non-conforming moduli (65521 ≡ 1 mod 4; 11 below the minimum); they now use the 16-bit floor 57203. The gossip reallocation collision search is O(modulus) but breaks on the first hit, so it still completes in a fraction of a second. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Follow-up to finding #2 from the Codex design review: cap the modulus at uint32 before trial division. The reference modulus is a uint32, so a larger value is invalid by definition, and the bound also keeps the primality test below ~65536 iterations so an untrusted custom-transport integer cannot make it hang. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Follow-ups to findings #6/#7/#13 from the Codex design review: - Bound each interface's lock acquisition by the transfer deadline, so a short-deadline sender queued behind a long-deadline holder of the same socket lock fails on its own budget instead of waiting the holder out. - gather(return_exceptions=True) so every interface send settles before aggregation, leaving none running in the background. - zip(strict=True) over the parallel interface/socket/lock lists to expose any length desync. - Resolve IP_MULTICAST_ALL via getattr so a CPython build that exposes it is used in preference to the hardcoded Linux uapi value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Follow-up to finding #10 from the Codex design review: fail the interface from the RX loop's OSError path (not by enqueueing the error behind pending frames), have receive() raise the terminal sentinel directly so an explicit close is not misrecorded as an interface failure, and drain any queued frames before installing the single terminal sentinel in close(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #11 (Medium). Several setup paths mutated registries before their fallible transport acquisitions and never rolled back, leaving unrepairable half-state on failure. Following the reference's three-tier model: - Repair model (enabler): TopicImpl.ensure_listener now catches an acquisition failure, logs it, and leaves the listener unacquired for the next opportunity to retry, mirroring cy.c topic_sync_subject_reader. A retry is also triggered at the start of periodic gossip (cy.c:1819). This makes sync_listener()/sync_implicit() infallible. - Node construction rolls back the broadcast writer if the broadcast listener fails. - ensure_gossip_shard rolls back its just-created writer if the shard listener fails. - topic_ensure commits its index first, then rolls the whole topic back via destroy_topic on any failure in the fallible tail (without masking the original exception); the gossip-driven twin does the same but never raises (untrusted wire input), dropping the gossip instead. - advertise acquires the writer before mutating publish state, so a failure leaves an ordinary implicit topic rather than a phantom publisher. - subscribe constructs the subscriber (fallible reordering-window validation) and, for a verbatim name, its topic before committing any root/subscriber registry state. - UDP subject_listen rolls back the handler and every per-interface socket/task on a mid-loop failure; _create_mcast_socket closes its socket if bind/join fails. MockTransport gains fail_subject_listen/fail_subject_advertise injection sets for the regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review findings #21 and #22 (Low). #21: Node.close() never cancelled library-owned request-publish tasks nor woke a pending response-stream iteration, so closing a node with an outstanding stream (especially one with a far-off or infinite response timeout) left the retry task and payload graph alive until the timeout. close() now disposes every outstanding response stream: it cancels the publish task, cancels any zombie-cleanup timer, and stops the iteration. #22: implicit-topic GC ignored request_futures, so it could destroy a topic that still had an open response stream, silently orphaning it after the publisher closed. compute_is_implicit now also treats an open response stream or an in-flight reliable publish as keeping the topic explicit; ResponseStreamImpl.close() and the reliable-publish-tracker release re-sync implicitness so a topic still becomes implicit once that state clears (no gossip-forever inverse leak); and destroy_topic disposes streams before delisting the topic to avoid a GC re-touch, then clears request_futures. Also: request() validates response_timeout (rejects NaN and negatives, allows +inf), a non-finite timeout disables liveness as with subscribers, and a failure after the stream is registered closes it rather than merely popping the tag, so a concurrent publisher close cannot strand the topic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Review finding #3 (Medium). Several internal structures grew without bound under wire traffic (whose source ID is spoofable on Cyphal/UDP), where the C core is bounded by design: - topic.dedup was pruned only for the arriving remote. A new aggregate, time-driven sweep (TopicImpl.drop_stale_dedup) retires entries for departed remotes, mirroring the reference dedup_drop_stale. - Reordering state was swept only when a later message arrived. The sweep (now public drop_stale_reordering) also runs from a periodic tick, so idle streams are retired after traffic stops (reference reordering_drop_stale). - Both sweeps run from an extended implicit_gc_loop on an ABSOLUTE housekeeping schedule, so steady implicit-topic traffic cannot postpone them indefinitely. - UDP reassembly sessions are now retired from a periodic _housekeeping_loop independent of new traffic (reference udpard_rx_poll), and bounded by an LRU capacity cap so a burst of unique spoofed UIDs cannot grow them without bound. - _remote_endpoints is now an LRU cache with a capacity cap (bounding memory under spoofed-UID flooding) and is cleared on close(); it is not time-expired, so a retained breadcrumb's reverse route survives until evicted under sustained flooding rather than expiring during a brief peer silence. - close() also resets the unicast reassembler and clears the unicast handler, which the finding flagged as surviving close(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
- Correct the stale CLAUDE.md/AGENTS.md claim that pycyphal2.can is "coming soon, not yet in the codebase" — the CAN transport now exists. - Document the response_timeout contract on Publisher.request (non-negative; +inf disables liveness; NaN/negative raise). - Summarize the audit fixes in the v2.0 changelog section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
From the multi-agent review of the change set: - Document the subject_id_modulus predicate on Transport.subject_id_modulus (a custom transport author cannot otherwise discover the constraint). - Close the transactional-setup gap the mcast fix left in its sibling: _create_tx_socket now closes its socket on bind/setsockopt failure, and node construction rolls back already-created TX sockets on a mid-loop failure. Regression test added. - Rewrite the UDP close() comment to state the invariant instead of referencing an (ephemeral) review finding; cite the reference by symbol (topic_sync_subject_reader) instead of a rot-prone line number; fix the self-contradictory "non-finite/NaN" wording in the request() docstring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
Two independent reviewers flagged the stale-state sweep merged into implicit_gc_loop (with absolute-deadline bookkeeping) as the campaign's most intricate addition. Split it into a dedicated NodeImpl._housekeeping_loop that sleeps HOUSEKEEPING_PERIOD and sweeps — the same non-postponable guarantee for free (nothing can postpone asyncio.sleep in its own task), matching the existing UDP/CAN transport housekeeping loops, and reverting implicit_gc_loop and _next_implicit_gc_delay to their simpler forms. Also collapse the twin except blocks in Publisher.request into one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
The lock-not-leaked regression test did a real multicast send to 127.0.0.1 to confirm the socket lock is re-acquirable after an acquisition timeout, which fails on Windows (WinError 1231: loopback has no multicast route). The lock-free assertion already proves no leak; the re-send now uses a mocked async_sendto so it still exercises lock re-acquisition via send_on_iface without touching the network. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CabiCDQ5DNBYq8WDKRzG4Z
af3af85 correctly made the frame format follow the interface rather than the payload length, but it also dropped BRS, so the data phase ran at the arbitration bit rate -- forfeiting the throughput that is the point of FD. BRS is now set on every frame of an FD interface, with no exceptions; on Classic CAN it does not apply. The FDF flag was broken too, and more quietly: _CANFD_FDF was resolved via getattr(socket, "CANFD_FDF", 0), and CPython exposes no CANFD_* constants on any supported version (verified through 3.14), so it silently evaluated to 0 and every "FD" frame went out with an empty flags byte. Both bits are now hardcoded from linux/can.h. REFERENCE PARITY: this is a deliberate divergence. cy_can_socketcan.c emits `.flags = CANFD_FDF` alone, and BRS appears nowhere in reference/cy or reference/libcanard. The divergence is marked in socketcan._encode and should be reconciled upstream. The defect survived review because nothing unpacked the flags byte -- the SocketCAN test only asserted the encoded frame's length. It now asserts FDF and BRS for both a short and a long payload, and the python-can test asserts bitrate_switch instead of its negation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
Two holes in the transactional-setup and teardown work. close() disposes every outstanding response stream, which cancels its publish task. That task's finally clause runs on a LATER loop iteration -- after close() returned and after transport.close() -- and re-syncs topic implicitness through sync_topic_lifecycle, which was not guarded by _closed. With a publisher still open the topic stays explicit, so the tail spawned a fresh _gossip_wait task on a dead node (never cancelled, and "Task was destroyed but it is pending" if the loop closed first) and called subject_listen on a closed transport. touch_implicit_topic could also re-populate _implicit_topics after close() had cleared it. Separately, transport.unicast_listen was the one unguarded fallible step in NodeImpl.__init__, one line after the try/except added to protect its predecessor. Nobody closes the transport when the constructor raises, so a failure there stranded the broadcast writer and listener. Both stock transports implement it as a plain assignment that cannot raise, but a custom transport may not. Regression tests pin both: the first reproduced a pending _gossip_wait task on a closed node before the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
The redundant-send aggregation classified per-interface results with
isinstance(r, Exception). asyncio.gather(..., return_exceptions=True)
reports an individually cancelled child as a CancelledError *instance*,
which has derived from BaseException since 3.8, so a cancelled interface
was scored as a successful delivery. With every interface cancelled the
error list came out empty and the send returned normally, reporting a
fully delivered transfer that never put a byte on the wire.
Both call sites now share _collect_send_errors, which tests BaseException
and documents why.
Fixing the predicate exposed a second defect on the all-failed path:
ExceptionGroup refuses to nest a BaseException ("Cannot nest
BaseExceptions in an ExceptionGroup"), so raising the aggregate turned
into a TypeError as soon as a CancelledError reached it. It now builds a
BaseExceptionGroup, which downgrades itself to an ExceptionGroup when
every member is an Exception, leaving the common case unchanged.
Covered by a unit test on the predicate and an end-to-end test asserting
that an all-cancelled send raises, while one cancelled interface
alongside one delivery still succeeds per redundancy semantics.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
All four long-lived maintenance loops -- NodeImpl._housekeeping_loop and implicit_gc_loop, _UDPTransportImpl._housekeeping_loop, and the CAN transport's _cleanup_loop -- caught only CancelledError. Any other exception killed the task, was never retrieved (nothing awaits these), and permanently disabled the sweep for the life of the node or transport, with no log above an eventual "exception was never retrieved" at GC time. These loops are the only bound on per-remote state growth and on stale RX session retirement, which do not happen on the traffic path, so silently losing one defeats the purpose of 6c20570. Each loop body now has a broad per-iteration boundary that logs and continues. The implicit-GC loop needed more than a catch: an already-expired topic yields a zero delay, so the loop does not await before retrying and a persistent fault would spin at full speed. Its recovery path backs off by one housekeeping period first. Regression tests inject a fault into the first sweep of the node and UDP loops and assert the loop recovers and sweeps on a later tick. Before the fix both reproduced the silent task death. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
…ean close Three related teardown defects. python-can's receive() fed EVERY terminal sentinel through _fail(), including the plain ClosedError that close() installs, so an explicit shutdown was recorded as an interface failure and relabelled "receive failed". SocketCAN already got this right, and its comment claimed to "mirror the python-can and webserial backends" while python-can carried the bug. The failure is now recorded at its source: the RX thread hands the exception to _fail() via call_soon_threadsafe, so it propagates even with nobody parked in receive(), and receive() raises the sentinel verbatim. close() installs _closed_error(), which carries the underlying failure as __cause__ instead of discarding it. The bare `except Exception: pass` around that put_nowait now logs -- without the sentinel a parked reader hangs forever, which must never be silent. SocketCAN's close() claimed "the socket is closed last so the cancelled reader deregisters cleanly", but Task.cancel() is deferred to a later loop iteration while socket.close() takes effect immediately, so the selector callbacks were torn down against an already-closed fd -- and against an unrelated socket if that fd number had been recycled meanwhile. Both SocketCAN and the UDP transport now deregister explicitly while the descriptor is still open. A send already parked inside sock_sendto still unblocks on its own deadline, which is the caller's budget. The UDP constructor's create_task calls sat outside the TX-socket rollback guard, so a failure there would leak every socket and orphan the RX tasks already spawned; a half-built transport is never returned and so is never closed. The SocketCAN fake socket now owns a real fd rather than lacking fileno(), which is what let the close path go untested here in the first place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
An audit of this branch flagged three sites as unbounded per-remote state
or as incomplete rollback. Checking each against the pinned reference
submodule showed all three already match it, so the "fixes" would have
been divergences. Documenting them so the next reader does not repeat the
analysis -- or act on it.
- Association.last_seen is written and never read. That is deliberate:
cy.c says "Not used for eviction, only for diagnostics and possibly API
exposure". Eviction is driven solely by `slack`. It is not a
half-finished timeout.
- topic.associations is unbounded and keyed by a spoofable remote-ID, but
the reference is too, and carries an explicit TODO for exactly this DoS
("there should be a limit ... ~500 might be a reasonable default").
Bounding it here would diverge on reliable-delivery accounting, so the
fix belongs upstream first.
- ResponseStreamImpl._reliable_remote_by_id is never pruned during the
stream's life, matching request_future_t.remote_by_id: "States are never
removed assuming that futures are short-lived and/or the responder set
is mostly constant". Its bound is the stream's lifetime. dispose() now
clears it along with the rest of teardown; close() still retains it
deliberately, to re-ack late duplicates via the zombie timer.
Also notes why the writer acquisition in topic_allocate's displacement
branch cannot fail midway: the collider still holds the writer for that
subject-ID, so the refcounted registry returns the existing entry without
touching the transport. The reference achieves the same by moving the
writer pointer outright.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
CI ran test, mypy and format but never lint, so ruff had never gated this branch even though bare `nox` -- the stated acceptance criterion -- includes it. Added a lint job alongside format. noxfile PYTHONS stopped at 3.13 while 3.14 is the current stable, which CLAUDE.md says is supported. That also made the whole suite unrunnable on a machine with only 3.14 installed. Added to PYTHONS and the CI matrix; the suite passes on it unchanged. Smaller items: - Node.new now documents the ValueError it raises for a transport whose subject_id_modulus fails the reference predicate; Transport already pointed at Node.new as the thing that raises it. UDPTransport.new notes that its own check is only the transport-level range, so an otherwise valid modulus can still be rejected a layer up. - Dropped CAN_STD_ID_MASK, dead since standard-ID SLCAN frames started being rejected. - The SLCAN drop message called 'R' a standard-ID frame; it is an extended-ID RTR frame. Both are unusable here, but for different reasons, and the log said the wrong one. - Node.close() now clears gossip_shard_writers/listeners after closing them, matching the adjacent shared_subject_* handling. - The two OrderedDict LRUs in udp.py use mirror-image conventions -- _sessions keeps the newest FIRST because its retirement scans want the oldest at a fixed end, _remote_endpoints keeps the newest LAST. Both are correct; each now says so and points at the other, since silently inverting one would flip an eviction policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
The teardown fix in 7970964 called loop.remove_reader/remove_writer unconditionally before closing a socket. Windows' ProactorEventLoop drives sockets through overlapped I/O rather than selector callbacks and implements neither method, so both raise NotImplementedError -- which took out every Windows job across all four Python versions while Linux and macOS stayed green. There is nothing to deregister on such a loop, so both call sites now treat NotImplementedError as "no registration exists". SocketCAN is Linux-only in production, but its unit tests exercise close() against whatever loop the host provides, which is how it failed there too. Pinned by a test that closes an interface against a loop raising NotImplementedError from both methods and asserts close() still completes and still closes the socket. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
The correctness work on this branch explained itself at length -- multi-line blocks that restated the adjacent code, re-derived the same conclusion across successive sentences, or narrated control flow. Cut the narration and kept the parts a reader cannot reconstruct from the code. 25 files touched; the comment lines this branch adds over master drop by about a quarter, and most surviving blocks are half their previous length. Comments and docstrings only -- no code changed. Kept deliberately, since none of it is recoverable from the source: every C reference citation and quoted reference text (cy.c, canard_poll, cavl2_predecessor, wkv_has_substitution_tokens, udp_wrapper.c, request_future_t, association_t), all ten REFERENCE PARITY markers, the kernel and protocol facts (linux/can.h flag values and the getattr() fallback hazard, IP_MULTICAST_ALL semantics, ProactorEventLoop lacking remove_reader, one-writer-per-fd in the selector loop), the correctness traps (BaseException vs Exception in gather results, BaseExceptionGroup nesting, the mod-2**64 history seed, the two mirror-image LRU orderings), and the defect provenance in each regression test's docstring. Verified mechanically rather than by eye: each file's syntax tree, with docstrings stripped and empty bodies normalized, is identical before and after, which is only possible if nothing but comments changed. The script was checked against a known-code-change control first. Black, ruff, mypy and the full suite (750 passed) all still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015RGRSKsWKELSLLPNr3FVpH
pavel-kirienko
marked this pull request as ready for review
July 18, 2026 21:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.