fix: a node answers a PING flood exactly once - #196
Conversation
A node that has an upstream runs two protocol objects at the same time: this HiveMindListenerProtocol, serving its downstream clients, and a HiveMindSlaveProtocol holding the connection to its upstream. They are two halves of one node. Each half kept its own flood_id cache, so a single PING flood got two answers from one node. Worse, the two answers carried different identities: the slave answers as its connection peer, the listener as its public key. The originator recorded two nodes where there was one, and every node with an upstream added a phantom entry to the map. HIVEMIND-NODE-1 §4 gives the node, not the connection, exactly one part in a flood. bind_upstream already wires the two halves together, so it is also where they are given one shared cache: the first half to see a flood_id now suppresses the other. A node with no upstream never gets there and still answers exactly once. The responsive PING also carries public_key now. The slave half already sent it; without it on this side, a master-only node had no stable identity in the payload at all, and a consumer could not tell whether two peer labels named one node or two. Needs hivemind_bus_client>=0.11.0a2 for FloodIdCache and bind_flood_cache. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The results are in! Let's see how your PR is doing. 📊I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthEnsuring the repo is staying active (aka commit frequency). 🏃♂️ ✅ All required files present. Latest Version: ✅ 📊 CoverageMapping the landscape of your tests. 🗺️ ❌ 31.7% total coverage Per-file coverage (9 files)
Full report: download the 🏷️ Release PreviewEnsuring our release announcement is ready for prime time. 📺 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔍 LintThe automated report has been generated. 🖨️ ❌ ruff: issues found — see job log 🔨 Build TestsI've checked the welds on your new features. 👨🏭 ✅ All versions pass
🔒 Security (pip-audit)I've checked the security of our API endpoints. 🌐 ✅ No known vulnerabilities found (78 packages scanned). ⚖️ License CheckDouble-checking the fine print for any surprises. 🔍 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. Your 24/7 automated code reviewer 🌙 |
…_flood attrs handle_ping_message now reads self.ping_flood_interval and self._last_ping_flood (added by the ping-flood rate limiter); the object.__new__-constructed test double in test_ping_flood_dedup.py (owned by #196, already on dev) predates those attributes and needs them set explicitly.
1 participant
The bug
A node that has an upstream runs two protocol objects at the same time:
this
HiveMindListenerProtocol, serving its downstream clients, and aHiveMindSlaveProtocol(hivemind-bus-client) holding the connection to itsupstream. They are two halves of one node.
Each half kept its own
flood_idcache —self._seen_flood_idshere,hive_mapper.check_flood_idthere — so a single PING flood got two answersfrom one node. Worse, the two answers carried different identities: the
slave answers as
name::session_id, this side as its public key. Theoriginator recorded two nodes where there was one, and every node with an
upstream added a phantom entry to the map.
What the spec says
HIVEMIND-NODE-1 §4: a PING fans out across the mesh, "each recipient replies
with its own PING carrying the same
flood_id, and nodes drop messages whoseflood_idthey have already seen (HIVEMIND-MSG-1 §4)". The subject is thenode, not the connection.
The change
One flood cache per node.
bind_upstreamalready wires the two halvestogether, so that is where they are given one shared cache
(
slave.bind_flood_cache(self._seen_flood_ids)). The first half to see aflood_idnow suppresses the other._seen_flood_idsbecomes aFloodIdCache— still set-like (in,.add(),len()), so every existingcaller and test is unaffected, and it now bounds itself by FIFO eviction
instead of
set.pop(), which evicted an arbitrary entry.A node with no upstream never reaches
bind_upstreamand still answersexactly once, through the listener. This is not "one side stops responding".
One identity per node. The responsive PING now carries
public_key. Theslave half already sent it; without it here, a master-only node had no stable
identity in the payload at all, so a consumer could not tell whether two
peerlabels named one node or two.On the
peerfield itself: it stays the connection-level label. Keying themapper on
public_keyinstead was considered and rejected — a PEM blob is apoor map key, and it would not have merged anything here anyway, because the
two halves of a node are separate identities with separate keys. The fix that
actually holds is the shared cache, which means only one identity is ever
announced per node per flood;
public_keyon both sides then makes that oneannouncement self-describing.
Landing order
hivemind-bus-client PR #161 must land first. This PR imports
FloodIdCache, callsbind_flood_cache, and pinshivemind_bus_client>=0.11.0a2.Verification
Live, on a real three-node socket mesh (master 6678 ← middle 6680 ← leaf),
one real PING flood:
Before (
origin/dev):After:
Tests
tests/test_ping_flood_dedup.py— 9 new tests:bind_upstreamshares thecache, suppression in both directions, a node without an upstream still
answers exactly once, malformed PING never answered, cache stays bounded,
and both halves announce the same public key.
git stashofhivemind_core/, new bus-client in place):6 of 9 fail —
the two halves of one node must share one flood cache,both suppression assertions, and
KeyError: 'public_key'twice. The 3 thatpass are the no-upstream cases, confirming that path is unchanged.
pass. Full in-process protocol layer 203 passed / 8 failed, and all 8 are
the other documented open regression (
PROPAGATE/BROADCASTofTHIRDPRTYasserting in the bus client) — untouched by this PR.🤖 Generated with Claude Code