Repository navigation
fix(p2p): trim the simple priority queue below its limit - #1597
QuantumExplorer wants to merge 2 commits into
Conversation
The simple priority queue trimmed its heap on push only when the signal channel was already pending. A push that refilled an empty signal skipped the trim, so with nobody draining, the heap ran past maxSize until the next 10ms tick. Instrumented under -race, a 2560-envelope flood at capacity 8 took the heap 7 to 18 envelopes past maxSize every time. TestLoadRouterSharedQueueOccupancyAndDrops checks that the queue retains no more than maxSize + cap(input) + cap(output) and failed when the drain caught that overshoot (85 and 87 retained against a ceiling of 84). The ceiling is right for the intended queue, so the queue now trims after every push and the ticker only re-signals. The test comment spells out why the envelope the run loop holds between input and heap cannot add to the count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An overflowing heap was sorted whole and trimmed back to exactly maxSize, so the next overflowing push had to sort again, and the run loop accepts and delivers nothing while it sorts. The vote channel's capacity of 4096 gives a limit of 16,777,216 envelopes, where a full queue took 2 to 3 seconds per push with nobody draining it. Trim to 15/16 of the limit so one sort covers the pushes that follow, and clear the dropped slots so those envelopes can be collected. A full queue at that capacity now accepts about 187,000 pushes per second. This changes behaviour under a flood. The cost of sorting used to slow senders to about the consumer's rate, so little was dropped. Senders now rarely wait, and the queue drops what the consumer cannot keep up with. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
|
✅ Final review complete — no blockers (commit fb3e02b) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
At fb3e02b, the trim implementation preserves the post-push size bound, completes dropped-envelope notifications, and clears discarded references, with focused unit coverage. One non-blocking suggestion remains: expose the materially larger overflow batches through drop accounting. This was a static review; current build, test, lint, and analysis checks pass, with the automated review itself still pending.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff makes a small, contained change to queue overflow trimming with focused tests, rather than a large or intricate change to a critical surface. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 28% left; passed overgemini-3.8-flash-high(antigravity quota lookup failed) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `internal/p2p/rqueue.go`:
- [SUGGESTION] internal/p2p/rqueue.go:127-135: Count the larger overflow batches
Each overflow now discards maxSize/16 + 1 envelopes rather than one: 1,048,577 on the vote channel and 65 on a per-peer outbound queue. Neither this loop nor its caller records those drops. Even when a flood stops immediately after an overflow, the new policy discards a substantial backlog that the base retained, without giving operators a way to distinguish local eviction from loss elsewhere.
The missing accounting predates this PR, but the changed eviction policy materially increases its operational impact. The sibling pqScheduler already increments PeerQueueDroppedMsgs with a ch_id label, and Router.createQueueFactory has r.metrics available. Pass metrics into the simple queue and count discarded envelopes by channel, preferably aggregating each batch before updating the counters to avoid a metric operation per envelope in million-message trims. Add coverage for the counts alongside the existing notification and reference-clearing assertions. This is an observability suggestion, not evidence that the queue previously guaranteed lossless consensus delivery.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bound per-peer admission before the shared consensus backlog — Router.receivePeer admits decoded envelopes into the shared channel queue before the consensus reactor applies its per-peer rate limits. Those downstream limits therefore do not protect shared backlog capacity from a flooding peer. This ordering already exists in the base; the queue throughput measurements do not establish honest-peer consensus progress under a flood.
- Follow-up: A separate follow-up PR should bound per-peer admission before the shared backlog and add a multi-peer flood test that verifies honest consensus progress.
| func (q *simpleQueue) trim(pq priorityQueue) priorityQueue { | ||
| sort.Sort(pq) | ||
| keep := q.maxSize - q.maxSize/simpleQueueTrimFraction | ||
| for _, dropped := range pq[keep:] { | ||
| dropped.envelope.NotifyDelivery() | ||
| } | ||
| // Later pushes reclaim these slots only one at a time, so release the | ||
| // dropped envelopes now. | ||
| clear(pq[keep:]) |
There was a problem hiding this comment.
🟡 Suggestion: Count the larger overflow batches
Each overflow now discards maxSize/16 + 1 envelopes rather than one: 1,048,577 on the vote channel and 65 on a per-peer outbound queue. Neither this loop nor its caller records those drops. Even when a flood stops immediately after an overflow, the new policy discards a substantial backlog that the base retained, without giving operators a way to distinguish local eviction from loss elsewhere.
The missing accounting predates this PR, but the changed eviction policy materially increases its operational impact. The sibling pqScheduler already increments PeerQueueDroppedMsgs with a ch_id label, and Router.createQueueFactory has r.metrics available. Pass metrics into the simple queue and count discarded envelopes by channel, preferably aggregating each batch before updating the counters to avoid a metric operation per envelope in million-message trims. Add coverage for the counts alongside the existing notification and reference-clearing assertions. This is an observability suggestion, not evidence that the queue previously guaranteed lossless consensus delivery.
source: glm-5.3-flash (phase1-reviewer: general)
|
Bots are done — your move: post |
github-actions
Bot
added
the
waiting-self-review
Basic explanation
When the router's simple priority queue overflows, it sorts its whole backlog to find the lowest-ranked envelopes to drop, and it accepts and delivers nothing while it sorts. It trimmed back to exactly its limit, so the next overflowing push had to sort again. The vote channel's capacity of 4096 gives a limit of 16,777,216 envelopes, and at that size a full queue took 2 to 3 seconds per push with nobody draining it.
This PR trims to 15/16 of the limit instead, so one sort covers the next 1/16 of the limit's worth of pushes.
This changes what a full queue does under a flood, and reviewers should decide whether that is wanted. The old queue's sorting slowed senders down to about the rate the consumer took envelopes, so it dropped very little. With this PR senders rarely wait, and the queue drops what the consumer cannot keep up with. Those drops are the same silent kind the queue already makes: there is no drop counter and no peer error.
Stacked on #1596, so merge that first.
Detailed discussion
What was done
internal/p2p/rqueue.go: the overflow path moves intosimpleQueue.trim. It sorts as before, notifies the dropped envelopes as before, and keepsmaxSize - maxSize/16. Queues with a limit under 16 have no margin and trim exactly what overflowed. It also clears the dropped slots, since later pushes would otherwise reclaim them only one at a time and up to 1/16 of the limit would stay reachable.internal/p2p/rqueue_test.go:TestSimpleQueueTrimDropsLowestRankedBelowMaxSizepins the size after a trim, that the oldest envelopes are the ones dropped, that their senders are released and their slots cleared, and that the result is still a valid heap.internal/p2p/load_router_queue_test.go: the ceiling comment follows the new trim. The ceiling itself does not change, because the heap still never holds more thanmaxSizeonce a push finishes.Trade-offs to weigh
TestLoadConsensusChannelsDoNotSilentlyTimeOutOnEnqueueargues that a full consensus queue should make the sender wait rather than quietly drop, because nothing counts the drops. This PR moves away from that for floods that fill the heap. On the other hand, one flooding peer no longer holds up every other peer on a queue they all share.simple-priorityis the defaultp2p.queue-type, and the per-peer outbound queues use the same code (queueBufferDefaultis 32, so a limit of 1024 and a margin of 64). The 1/16 is a judgement call, not a tuned value.A drop counter on this queue would make any of these choices visible to operators. It is not part of this PR.
Testing
All measurements are scratch runs that are not committed: macOS on 30 cores, no
-race, empty votes, one run per cell over a 20-second window, and the queue already full at the start.Pushes accepted per second with nobody draining:
Accepted and delivered per second, with a consumer limited to 5,000 per second. Both that rate and the 50,000 per second flood are arbitrary choices:
Accepted minus delivered is roughly what was dropped, since the heap cannot grow past its limit. Drops were not counted directly.
Tests under
-race, each run the number of times shown:-cpuThe tests were
TestLoadRouterSharedQueueOccupancyAndDrops,TestSimpleQueueTrimDropsLowestRankedBelowMaxSize,TestSimpleQueueandTestSimplePriorityQueueDoesNotPopWhenOutputIsFull.Reverting the margin, the slot clearing or the delivery notification in
trimeach makes the new unit test fail. The other queue and router tests ininternal/p2ppass under-race;gofmtandgo vetare clean.Not tested: a real multi-node network under flood, or the effect on consensus progress. These numbers only cover the queue on its own.
Breaking changes
None to APIs or the protocol. The runtime behaviour under a flood changes as described above.
Prior work
thepastaclaw's suggestion on fix(p2p): trim the simple priority queue on every push #1596 flagged the cost of sorting the whole backlog on each overflowing push.🤖 Generated with Claude Code
PR Hygiene ·
fb3e02b/self-reviewedinternal/p2p/load_router_queue_test.go,internal/p2p/rqueue.go,internal/p2p/rqueue_test.go) — lklimek or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.