feat: priority queue#292
Conversation
WalkthroughAdded a priority-enabled scheduling mechanism to serialBroadcaster: requests can be marked with WithPriority(), are enqueued into a priority deque, and the broadcaster preferentially dispatches priority items before pending ones; tests exercise FIFO semantics, preemption, error handling, and nil-option behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Enqueuer as Enqueuer
participant Broadcaster as serialBroadcaster
participant Dispatcher as Dispatcher
Enqueuer->>Broadcaster: enqueue(req, opts)
alt opts.priority == true
Broadcaster->>Broadcaster: push to priority deque
else opts.priority == false
Broadcaster->>Broadcaster: push to pending deque
end
Note right of Broadcaster: When idle, tryBroadcast prefers priority deque
Broadcaster->>Dispatcher: dispatch(next from priority or pending)
Dispatcher-->>Broadcaster: dispatch result (error or nil)
Broadcaster->>Enqueuer: notify completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/node/client/v1beta3/tx.go (1)
469-494:⚠️ Potential issue | 🟡 MinorStrict priority can starve the pending queue.
tryBroadcastalways preferspriorityoverpendingwith no aging/weighting. A sustained stream ofWithPriority()callers will indefinitely delay non-priority work (including requests that may have been queued long before). If the intended callers are tightly controlled (e.g., a single provider hot-path), this is acceptable, but it is worth either:
- documenting the starvation property on
WithPriorityso callers understand the contract, or- considering a simple fairness mechanism (e.g., drain at most N priority items before yielding one pending item, or age-based promotion).
No change required if starvation is an explicit design choice, but please make that choice explicit in the
WithPrioritygodoc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/v1beta3/tx.go` around lines 469 - 494, tryBroadcast currently always selects priority over pending causing potential starvation; update it to implement simple fairness (e.g., track a counter in tryBroadcast or surrounding struct and drain at most N items from priority before allowing one pending item, then reset the counter), or alternatively add/expand the WithPriority godoc to explicitly document the starvation contract; refer to tryBroadcast, priority, pending, WithPriority and ensure the chosen approach updates selection logic (or godoc) consistently and includes any new counter/state variable if implementing draining.
🧹 Nitpick comments (1)
go/node/client/v1beta3/tx.go (1)
208-216: Nit: document starvation/ordering contract.Consider expanding the godoc to state explicitly that (a) priority requests FIFO among themselves, (b) a non-priority request already in-flight is not preempted (only pending non-priority work is), and (c) sustained priority traffic can indefinitely delay non-priority requests. This sets correct expectations for the forthcoming provider PR referenced in the description.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/v1beta3/tx.go` around lines 208 - 216, Update the godoc for WithPriority (the BroadcastOption returned by WithPriority) to explicitly document the ordering/starvation contract: state that priority requests are FIFO among themselves, that an in-flight non-priority request is not preempted (only pending non-priority work is delayed), and that sustained priority traffic can indefinitely delay non-priority requests; reference the BroadcastOption/ BroadcastOptions and the opts.priority behavior in the comment so readers of WithPriority and maintainers of the provider PR understand the guarantees.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@go/node/client/v1beta3/tx.go`:
- Around line 469-494: tryBroadcast currently always selects priority over
pending causing potential starvation; update it to implement simple fairness
(e.g., track a counter in tryBroadcast or surrounding struct and drain at most N
items from priority before allowing one pending item, then reset the counter),
or alternatively add/expand the WithPriority godoc to explicitly document the
starvation contract; refer to tryBroadcast, priority, pending, WithPriority and
ensure the chosen approach updates selection logic (or godoc) consistently and
includes any new counter/state variable if implementing draining.
---
Nitpick comments:
In `@go/node/client/v1beta3/tx.go`:
- Around line 208-216: Update the godoc for WithPriority (the BroadcastOption
returned by WithPriority) to explicitly document the ordering/starvation
contract: state that priority requests are FIFO among themselves, that an
in-flight non-priority request is not preempted (only pending non-priority work
is delayed), and that sustained priority traffic can indefinitely delay
non-priority requests; reference the BroadcastOption/ BroadcastOptions and the
opts.priority behavior in the comment so readers of WithPriority and maintainers
of the provider PR understand the guarantees.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b18d79a-0df2-48b2-b7f7-68428470d045
📒 Files selected for processing (2)
go/node/client/v1beta3/tx.gogo/node/client/v1beta3/tx_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
go/node/client/v1beta3/tx.go (2)
469-498: Strict priority scheduling can starve the pending queue.
tryBroadcastunconditionally preferspriorityoverpending. If a caller sustains a steady flow ofWithPriority()requests (e.g., a provider's bid/lease loop), non-priority work can be delayed indefinitely. This is an inherent tradeoff of strict priority scheduling, but worth being intentional about since both deques are unbounded and there is no aging / weighted fairness. A few options to consider, in increasing order of complexity:
- Document the starvation contract in
WithPriority's godoc so callers understand priority is absolute, not weighted.- Cap the priority deque length (push back with an error / block) to bound the worst-case delay on pending.
- Introduce periodic draining from
pending(e.g., every N consecutive priority dispatches, serve one pending).Not a blocker for merge — just flagging so it's a deliberate design choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/v1beta3/tx.go` around lines 469 - 498, tryBroadcast currently always prefers the priority deque over pending, which can starve non-priority work; either document that WithPriority is strict in its godoc or add simple fairness: introduce a counter in the broadcaster (e.g., consecutivePriorityDispatches) and modify tryBroadcast to select pending when the counter reaches a threshold N (reset the counter when you dispatch pending, increment when you dispatch priority), or alternatively cap the priority deque length and return/block on push for WithPriority; update tryBroadcast, the surrounding scope to hold the counter and threshold constant, and/or add godoc to WithPriority to make the chosen approach explicit.
460-467: UseFront()(with ok) instead ofPeek(0)for clarity.Minor readability nit — the
edwingeng/deque/v2API exposesFront() (T, bool)which more clearly communicates "look at the head without removing" thanPeek(0)(which accepts an arbitrary index and panics on out-of-range). The code guards withq.Len() > 0at lines 473 and 476, so functionally both are equivalent; this is purely a readability suggestion.♻️ Optional refactor
- req := q.Peek(0) + req, _ := q.Front()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/v1beta3/tx.go` around lines 460 - 467, Replace usage of Peek(0) with the deque Front() helper for clearer intent and safer bounds handling: when inspecting the head of either deque (priority or pending) switch calls like q.Peek(0) to q.Front() and handle the returned ok boolean (e.g., only use the returned value when ok is true). Update any code paths that currently rely on prior q.Len() checks to instead use the Front() ok result to guard access in functions/methods around priority, pending, and the broadcast logic that reference Peek(0) (e.g., where broadcastCh, broadcastDoneCh, tryBroadcast, or send dispatch logic examines the deques).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@go/node/client/v1beta3/tx.go`:
- Around line 469-498: tryBroadcast currently always prefers the priority deque
over pending, which can starve non-priority work; either document that
WithPriority is strict in its godoc or add simple fairness: introduce a counter
in the broadcaster (e.g., consecutivePriorityDispatches) and modify tryBroadcast
to select pending when the counter reaches a threshold N (reset the counter when
you dispatch pending, increment when you dispatch priority), or alternatively
cap the priority deque length and return/block on push for WithPriority; update
tryBroadcast, the surrounding scope to hold the counter and threshold constant,
and/or add godoc to WithPriority to make the chosen approach explicit.
- Around line 460-467: Replace usage of Peek(0) with the deque Front() helper
for clearer intent and safer bounds handling: when inspecting the head of either
deque (priority or pending) switch calls like q.Peek(0) to q.Front() and handle
the returned ok boolean (e.g., only use the returned value when ok is true).
Update any code paths that currently rely on prior q.Len() checks to instead use
the Front() ok result to guard access in functions/methods around priority,
pending, and the broadcast logic that reference Peek(0) (e.g., where
broadcastCh, broadcastDoneCh, tryBroadcast, or send dispatch logic examines the
deques).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4d0f19d-2140-4e93-9d4a-6964690dce8b
📒 Files selected for processing (1)
go/node/client/v1beta3/tx.go
1 participant
📝 Description
This PR introduces a non-breaking change: I added the WithPriority broadcast option.
It does not affect existing library users but allows important messages to be pushed to the front of the queue.
How it works: each time the broadcaster is ready to send a transaction, it checks the priority queue first. If it’s empty, it pulls messages from the default queue.
🔧 Purpose of the Change
📌 Related Issues
Related to akash-network/support#476, provider PR is on the way
✅ Checklist
📎 Notes for Reviewers
[Include any additional context, architectural decisions, or specific areas to focus on]