fix(leiosfetch): answer unconfigured requests instead of holding agency - #2173
Conversation
An unconfigured BlockRequestFunc/BlockTxsRequestFunc returned nil, leaving the server in StateBlock/StateBlockTxs holding agency. The requester's send loop waits on sendReadyChan for agency that only the missing response returns, so no further leios-fetch request can be written to that bearer. Answer MsgNoBlock/MsgNoBlockTxs instead; the configured not-found path at server.go:138 already emits both wire IDs. BlockRangeRequest has no absence reply, so an unconfigured callback now returns a protocol error rather than hanging the requester silently. acquireSlot fails the connection when acquire reports ErrRequestSlotAbandoned, so the consumer's peer governance can replace a peer that took agency and never returned it. The slot is never reused, so this does not reintroduce the mis-delivery hazard it guards. Refs blinklabs-io/dingo#3623 Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe leios-fetch server now returns Merge Risk: 🔵 Low · up to The change prevents unconfigured requests from hanging indefinitely and safely closes a peer connection when an abandoned exchange cannot be synchronized. This is mergeable with owner awareness because the connection-level failure can also interrupt other protocols sharing that peer connection. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
No conflicts. Merged with rerere disabled. Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
VotesRequest and BlockRangeRequest sent directly with no slot, so an abandoned block request left them blocking on their result channels with no context and no diagnosis. The leios-fetch states share one connection-wide agency, so nothing can be written to the bearer while the peer still owes that response, and neither method reached acquireSlot to surface it as ErrRequestSlotAbandoned. Both now take a context and acquire the shared admission slot. Their responses do not arrive through the slot, so success releases it explicitly; a context that expires with the response outstanding abandons instead, since the peer still holds agency. Neither method had a caller or a test. Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
abandon keeps the slot busy until a late response drains it through deliver, but a votes or range response goes to its own result channel and never reaches deliver, so abandoning on context expiry left the slot permanently busy and failed every later request on the connection. Those two states release instead; their responses are not slot-correlated and a late one is dropped by the handler. The result channels were unbuffered with blocking sends, so a response whose caller had given up blocked the protocol receive loop. They are now capacity 1 -- the caller registers before the request is sent -- and the handlers drop a response nobody is waiting for. Consolidates the two near-duplicate admission tests into one table-driven test. Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
cryptodj413
left a comment
cryptodj413
left a comment
There was a problem hiding this comment.
Reviewed the full diff against the PR head, including tracing protocol.Protocol's send/receive/state-machine code and writing executable reproductions (run with -race) rather than taking either the bots' or my own first read at face value.
Two confirmed, reproduced merge blockers, both rooted in the same design gap: blockRequestSlot now gates admission for all four request kinds (Block/BlockTxs/Votes/Range), but only Block/BlockTxs route their response back through it via deliver() (which always drains the slot). Votes and Range responses bypass it entirely (handleVotes, handleNextBlockAndTxsInRange, handleLastBlockAndTxsInRange only push into their own shared channels), so the slot's admission bookkeeping and its response delivery are out of sync for those two:
VotesRequest'sctx.Done()path (abandon) permanently wedges the shared slot, then kills a healthy connection — see inline comment. Reproduced: peer answersMsgVotes200ms after a 50ms client timeout (a healthy peer); the next leios-fetch request on the connection still burns the fullabandonedRequestWaitand then fails the whole multiplexed bearer withErrRequestSlotAbandoned, because nothing ever drains an abandoned votes slot.BlockRangeRequest'sctx.Done()path (release) causes confirmed stale-data mis-delivery to the next caller — this is the still-open Cubic P1 at client.go:499; replied to that thread with a concrete repro confirming it. A second, unrelatedBlockRangeRequestreceived the first (abandoned) request's stale terminal message as if it were its own, witherr == nil.
Both are worse than the bug this PR fixes: a silent wrong answer (2) or a silent permanent wedge with the connection then torn down for a peer that behaved correctly (1), versus the original detectable hang.
Also found:
- No test exercises a
VotesRequest/BlockRangeRequestwhose own context expires with a response still outstanding, nor a successful end-to-end call to either — the newrelease-on-success paths and the buffered/non-blocking channel change are entirely uncovered by this PR's own tests. blockSlot/blockTxsSlot(client.go:37-38) are dead now that Block/BlockTxs moved ontoblockRequestSlot; they only survive as always-false fallback targets in the fourhandleBlock*/handleNoBlock*handlers. Not part of this diff, but worth a heads-up:client_test.go'sTestClientMessageHandlercurrently drives routing through these dead fields, so it passes even withblockRequestSlot.deliver()deleted from all four handlers — it isn't covering the real path anymore.- README's new "Abandoned requests" section (209-222) was written for the first commit and never updated for the later two: it doesn't mention that Votes/Range now share the same admission slot, or the release-vs-abandon split that's the source of finding 2.
No API/wire compatibility break: nothing in the blinklabs-io workspace calls VotesRequest/BlockRangeRequest today (only Block/BlockTxs, unchanged signatures), so the new ctx parameter is safe as an isolated change; dingo's own follow-up (dingo#3772) already flags that consuming this needs a go.mod bump plus a client-side test update.
Happy to help work through a fix for 1/2 — the cleanest direction looks like draining blockRequestSlot from the votes/range handlers too (so abandon becomes legitimately recoverable everywhere and both context paths can use it consistently), or correlating each votes/range response per-request the way Block/BlockTxs already do via deliver(), rather than through one shared, unscoped channel.
| // connection-wide agency and have no request identifier. | ||
| blockRequestSlot requestSlot | ||
| blockSlot requestSlot | ||
| blockTxsSlot requestSlot |
There was a problem hiding this comment.
blockSlot/blockTxsSlot look dead as of this PR: BlockRequest/BlockTxsRequest now acquire blockRequestSlot exclusively (lines 360, 395), so nothing ever calls .acquire() on these two anymore. They only survive as always-false second deliver() attempts in handleBlock/handleNoBlock/handleBlockTxs/handleNoBlockTxs below (e.g. lines 552, 565, 571, 584). Worth deleting both the fields and the fallback calls in a follow-up, since client_test.go's TestClientMessageHandler currently exercises message routing through these dead fields rather than the real blockRequestSlot path.
| } | ||
| return resp, nil | ||
| case <-ctx.Done(): | ||
| c.blockRequestSlot.abandon(w) |
There was a problem hiding this comment.
abandon() leaves blockRequestSlot busy+abandoned until something calls deliver() or release() on it. But a late votes reply only reaches handleVotes (line 592), which pushes into votesResultChan and never touches blockRequestSlot. So this abandonment is structurally undrainable: the next leios-fetch request on this connection (of any of the four kinds, since they all share this slot) will always burn the full abandonedRequestWait grace period and then fail the whole connection with ErrRequestSlotAbandoned — even when the peer answers correctly and promptly after this caller's context expired.
Reproduced: peer sends MsgVotes 200ms after a 50ms client-side timeout (state machine confirms the peer returned agency to Idle); the next BlockRequest still fails with leios-fetch: peer retained agency in state Block after an abandoned request: ..., tearing down the connection for a peer that behaved correctly. This is the same failure class dingo#3623 describes (silent permanent wedge), now reachable through a votes timeout rather than an unconfigured responder, plus it kills the connection unnecessarily.
Note StateVotes already carries a StateMap timeout (line 244-247), so a genuinely non-responsive peer is already caught there — this abandon path only ever fires for peers that do answer.
There was a problem hiding this comment.
Verified fixed as of 3654866. Repro: VotesRequest with a 50ms context, peer answers correctly 200ms later (well within the grace period) — the abandoned slot now drains via deliver(), and a subsequent BlockRequest on the same client succeeds normally with no connection error. Thanks.
| if errors.Is(err, ErrRequestSlotAbandoned) { | ||
| c.SendError( | ||
| fmt.Errorf( | ||
| "%s: peer retained agency in state %s after an abandoned request: %w", |
There was a problem hiding this comment.
Minor: this formats the error with the new request's target state argument, not the state where agency was actually retained. In my Finding-A repro (see general review comment) this produced "peer retained agency in state Block after an abandoned request" for a wedge that actually originated in StateVotes on a connection where the peer had already returned agency — misleading in an operator's logs.
There was a problem hiding this comment.
Confirming this one is still live at b136417, and I think the shared slot has made it worse rather than neutral: now that all four request kinds admit through blockRequestSlot, the state argument is the state the new caller wanted, and the abandoned request that actually wedged the exchange could have been any of the other three. An operator reading state Block has no way to tell which.
Cheap fix if you want one: have acquire return the state recorded when the slot was taken, or store it on requestSlot at acquire time, and format that instead. Not blocking from me.
|
|
||
| `BlockRequest` and `BlockTxsRequest` are bounded by the caller's context. A | ||
| request whose context expires is abandoned: its delivery channel is cleared so | ||
| a late response is dropped rather than mis-delivered, and the slot stays busy |
There was a problem hiding this comment.
This section was written for the first commit in the PR and not updated for the later two (e147ef6, 2463098): it only describes Block/BlockTxs's abandon-then-grace-period policy. It doesn't mention that VotesRequest/BlockRangeRequest now acquire the same shared slot (so an abandoned block request fails them too), or the release-vs-abandon split between them, which is the source of the mis-delivery in the other inline comment on this PR.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
cryptodj413
left a comment
cryptodj413
left a comment
There was a problem hiding this comment.
Re-reviewed 3654866 ("correlate votes and range responses") against the two merge blockers from the previous review, with fresh reproductions run against this exact commit.
Finding A (VotesRequest permanent wedge) — fixed, verified. Routing votes responses through blockRequestSlot.deliver() makes abandon() properly drainable now. Repro: a VotesRequest whose 50ms context expires, followed by the peer answering correctly 200ms later (well within the peer's real behavior), no longer wedges the slot or kills the connection — a subsequent BlockRequest on the same client succeeds normally and no connection error is emitted. Thanks for the fix.
Finding B (BlockRangeRequest stale mis-delivery) — still present. The correlation mechanism changed (per-request channel via deliver() instead of a shared blockRangeResultChan), but BlockRangeRequest's ctx.Done() branch still calls release(), not abandon() (see inline comment), so the same mis-delivery survives through the new mechanism: release() frees the slot immediately, letting a second, unrelated BlockRangeRequest become the slot's registered waiter before the first (abandoned) request's late terminal message arrives; deliver() then hands that stale message to the second request's channel. Reproduced against this exact commit:
req1 (100ms ctx, range [1,2]) err: context deadline exceeded
req2 (2s ctx, range [3,4]) err: <nil> blockRaw: 82 01 02 (req1's stale data, not req2's own 82 03 04)
The comment justifying release() ("a range response reaches blockRangeResultChan, never deliver, so an abandoned slot here would never drain") is now factually incorrect post-refactor — range responses do reach deliver() now (via handleNextBlockAndTxsInRange/handleLastBlockAndTxsInRange), so abandon() should be just as drainable for BlockRangeRequest as it now is for VotesRequest. Switching this one branch from release(w) to abandon(w) looks like it would close this out, matching VotesRequest's pattern exactly.
Still open from the previous round, unaddressed by this commit: the dead blockSlot/blockTxsSlot fields (client_test.go's TestClientMessageHandler still acquires those instead of blockRequestSlot for the Block/BlockTxs cases, so it still isn't exercising the real production delivery path for those two message types), and the README "Abandoned requests" section (still describes only the pre-refactor Block/BlockTxs-only policy).
| // release, not abandon, for the reason given in VotesRequest: | ||
| // a range response reaches blockRangeResultChan, never deliver, | ||
| // so an abandoned slot here would never drain. | ||
| c.blockRequestSlot.release(w) |
There was a problem hiding this comment.
This still release()s rather than abandon()s, and the justifying comment above ("never deliver") is no longer accurate after this commit's refactor: handleNextBlockAndTxsInRange/handleLastBlockAndTxsInRange now call c.blockRequestSlot.deliver(msg) (lines 585, 591), so a late range response does reach deliver() just like Block/BlockTxs.
Because this still releases immediately, a second BlockRangeRequest can acquire the slot and register as the new s.waiter before this (abandoned) request's late terminal message arrives — and deliver() will hand that stale message to the second request. Reproduced against this exact commit: a second, unrelated BlockRangeRequest (fresh context, different range) received the first (abandoned) request's stale MsgLastBlockAndTxsInRange payload with err == nil.
VotesRequest's equivalent branch (line 440) already calls abandon(w) and — per the top-level review comment — that now correctly recovers when the peer answers within the grace period. This branch looks like it should match: c.blockRequestSlot.abandon(w) instead of release(w).
There was a problem hiding this comment.
Following up on this rather than opening a duplicate — the release -> abandon change did land in 83e7b7e, but I do not think it takes effect for the case you described.
abandon only mutates while s.waiter == w (client.go:129), and deliver clears s.waiter on the first range message, because it frees the slot on every message rather than only on the terminal one. So for any range that already received a Next, the slot is free before the context expires and abandon is a no-op — the quarantine only covers a range abandoned before its first message.
I filed that root cause separately on handleNextBlockAndTxsInRange, with two repros: a Next/Next/Last range never assembles at all (passes on origin/main, bisects to 3654866), and a BlockRequest issued mid-range comes back holding a *MsgNextBlockAndTxsInRange. Your read of this branch was right; it just needs the fix one level down.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
|
Resolved the latest range-response review finding in 83e7b7e. Abandoned BlockRangeRequest calls now quarantine the shared request slot until the late terminal response drains, preventing stale data from reaching a later request. Added a delayed-response regression test and updated the abandoned-request documentation. Validation: focused race test, protocol-wide race tests, conformance tests, and make test passed; integration endpoint test skipped because no Cardano node is configured. Repository lint reported no source issues but the example phase was blocked by a concurrent golangci-lint lock. |
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
chrisguiney
left a comment
chrisguiney
left a comment
There was a problem hiding this comment.
Thanks for the depth here — the server-side half is right, and I checked it against the state machine rather than taking the description on trust: MsgLastBlockAndTxsInRange genuinely carries a mandatory block, so a protocol error really is the only honest answer for an unconfigured range responder.
The client-side refactor that grew on top of it breaks the streaming range state. Details inline; the main one is at handleNextBlockAndTxsInRange.
The short version
requestSlot.deliver (client.go:178) clears the waiter and frees the slot on every message, but StateBlockRange self-transitions on MessageTypeNextBlockAndTxsInRange (leiosfetch.go:96), so one range registration has to survive N messages. Two things follow, both reproduced:
- A Next/Next/Last range makes
BlockRangeRequestreturncontext deadline exceeded. The same test passes onorigin/main. - A
BlockRequestissued while a range is streaming returns a*MsgNextBlockAndTxsInRange— the mis-delivery the slot exists to prevent.
Bisected across the branch with test 1: e147ef6 ok, 2463098 ok, 3654866 first failure, still failing at b136417.
Verified good
- Merging
blockSlotandblockTxsSlotis right — those states share one connection-wide agency, so two slots were never buying anything. - Dropping the
Start()cleanup goroutine removes a goroutine rather than leaking one, and the tests keepgoleak. - The protocol-level timeout re-arms on the
StateBlockRangeself-transition (setStateruns per transition), so a long range will not tripconfig.Timeout. The README's claim there holds. SendErroronErrRequestSlotAbandonedis a big hammer, but I went looking for a gentler option and could not find one:sendLooponly writes atAgencyClient, so a bearer whose peer never returns agency really is dead for leios-fetch.
Checks run locally
go build ./..., go vet ./protocol/..., gofmt, golangci-lint run ./protocol/leiosfetch/... (0 issues), go test ./... and go test -race -count=2 ./protocol/leiosfetch/ all pass — which is the problem: nothing in the suite sends more than one message in a range. Not run: conformance, devnet, nilaway locally (CI green).
Smaller things, not inline
VotesRequestandBlockRangeRequestgained actxparameter. That is a public API break; no in-org client consumer today (dingo wires only the server callbacks), so I am not asking you to avoid it — but it is worth a line in the description.BlockRequest,BlockTxsRequestandVotesRequestare now three near-identical bodies differing only in the message constructor and the not-found mapping. A singledoRequest(ctx, state, msg)helper would make the shared-slot policy live in one place, which is also where a future rule would have to go. Nit, and fine as a follow-up.- The PR description still describes only the first commit. The single-slot refactor, the
ctxbreak and the votes/range routing are the bulk of the diff and are not mentioned.
Process note
Cubic's last review was 09-02 20:13Z, so 3654866, 83e7b7e and b136417 are unreviewed by it, and CodeRabbit's green check reads "Review skipped: incremental reviews are disabled". The regression landed in 3654866, inside that gap — worth another bot pass before this goes back out for human review.
|
|
||
| func (c *Client) handleNextBlockAndTxsInRange(msg protocol.Message) { | ||
| c.blockRangeResultChan <- msg | ||
| if !c.blockRequestSlot.deliver(msg) { |
There was a problem hiding this comment.
deliver frees the slot on every message, but this state streams.
requestSlot.deliver (client.go:178) does s.waiter = nil; s.freeLocked() unconditionally. StateMap self-transitions StateBlockRange on MessageTypeNextBlockAndTxsInRange (leiosfetch.go:96), so a range legitimately delivers N messages through the one registration BlockRangeRequest made. After the first Next, the waiter is gone: message 2 lands on w == nil, returns false, and gets logged as a dropped response while the caller is still selecting on w.
The premise for this is stated at client.go:187 — "receives at most one response per request (the state machine rejects a second server message before it reaches this handler)". That holds for Block, BlockTxs and Votes, but StateBlockRange is the one state where the state machine explicitly permits the second message.
Reproduced, two ways.
A range of Next/Next/Last never assembles — BlockRangeRequest returns context deadline exceeded. This test passes on origin/main and fails here:
ouroboros_mock.ConversationEntryInput{
ProtocolId: leiosfetch.ProtocolId,
MessageType: leiosfetch.MessageTypeBlockRangeRequest,
},
ouroboros_mock.ConversationEntryOutput{
ProtocolId: leiosfetch.ProtocolId,
IsResponse: true,
Messages: []protocol.Message{
leiosfetch.NewMsgNextBlockAndTxsInRange([]byte{0x82, 0x01, 0x01}, nil),
leiosfetch.NewMsgNextBlockAndTxsInRange([]byte{0x82, 0x01, 0x02}, nil),
leiosfetch.NewMsgLastBlockAndTxsInRange([]byte{0x82, 0x01, 0x03}, nil),
},
},
// ... resp, err := client.BlockRangeRequest(ctx, start, end)
// require.NoError(t, err); require.Len(t, resp, 3)Bisected with it: e147ef6 ok, 2463098 ok, 3654866 first failure, still failing at head.
Worse, the freed slot is immediately reusable while the peer is still streaming. With a BlockRequest issued 250ms into a range, I got:
BlockRequest returned *leiosfetch.MsgNextBlockAndTxsInRange
That is the mis-delivery the slot was built to prevent, now reachable on the range path.
One thing worth deciding before the fix: whichever way the waiter is retained across a stream, deliver runs on the protocol receive loop, so the send has to stay non-blocking. A retained waiter on the current capacity-1 channel would let a server that streams faster than the caller drains block recvLoop. Happy to think through options with you if it helps — a per-request queue and a slot that only frees on the terminal message is the shape I would reach for, but you know this protocol better than I do.
| return ret, nil | ||
| } | ||
| case <-ctx.Done(): | ||
| // Range responses are delivered through the shared slot, including |
There was a problem hiding this comment.
Follows from the deliver finding above: this abandon(w) is a no-op for any range that already received a message.
abandon only mutates while s.waiter == w (client.go:129), and deliver cleared s.waiter on the first Next. So the quarantine 83e7b7e added only applies to a range abandoned before its first message — which is the narrower half of what the earlier review thread asked for. Past that point the slot is already free, a later request can acquire it, and the peer's remaining range messages go to that request instead.
The comment here states the intended behaviour exactly right ("Keep the slot abandoned until that response drains"); it is deliver that does not implement it. I did try to get a clean end-to-end repro of the stale-terminal-message case specifically, but the connection tore down first in my harness, so I am flagging this as reasoning from the code plus the two repros above rather than something I measured.
| // response from an abandoned range exchange cannot be delivered to a later | ||
| // range request. Range responses use the shared request slot, so the first | ||
| // request must remain quarantined until its terminal response drains. | ||
| func TestBlockRangeRequestSubsequentAfterAbandoned(t *testing.T) { |
There was a problem hiding this comment.
No test sends more than one message in a range.
This test and every other range case respond with a single MsgLastBlockAndTxsInRange, so the streaming path — the one thing StateBlockRange exists for — is never exercised. That is why the full suite, -race -count=2, lint and all of CI are green on a head where a two-message range does not work.
A Next/Next/Last case here would have caught 3654866 on the commit that introduced it. Snippet in the handleNextBlockAndTxsInRange comment if it saves you a few minutes.
| votesResultChan: make(chan protocol.Message), | ||
| blockRangeResultChan: make(chan protocol.Message), | ||
| config: cfg, | ||
| // Capacity 1: the caller registers before its request is sent, so |
There was a problem hiding this comment.
Two comments got orphaned by the refactor and now describe code that is not here.
- This one describes a channel capacity, but
votesResultChanandblockRangeResultChanare gone and the literal now sets onlyconfig. The capacity-1 rationale still applies, just to the channelrequestSlot.acquirecreates — it would read correctly there. client.go:273:// Start goroutine to cleanup resources on protocol shutdown.is now the last line ofStart(), with no goroutine after it.
Trivial, but both are the kind of thing that misleads the next reader of this file.
| expires is abandoned: its delivery channel is cleared so a late response is | ||
| dropped rather than mis-delivered, and the slot stays busy so the next request | ||
| cannot be correlated with the outstanding response. For `BlockRangeRequest`, | ||
| this applies to the whole streaming exchange; the terminal response drains the |
There was a problem hiding this comment.
This documents behaviour the code does not currently have: deliver frees the slot on the first range message, not the terminal one (see the inline comment on handleNextBlockAndTxsInRange).
Worth re-checking this paragraph once the streaming fix lands — as written it is the right specification, so it may need no edit at all beyond confirming the code matches it.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
3 participants
Keep block-range delivery alive through the terminal response.
Closes #3623
Tests:
go test -race ./protocol/leiosfetch -count=1