fix: prevent duty loss and fix misleading comment in deadliner#4498
fix: prevent duty loss and fix misleading comment in deadliner#4498enlightened88 wants to merge 3 commits intoObolNetwork:mainfrom
Conversation
Signed-off-by: Андрій Шовкошитний <198119344+enlightened88@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the core deadliner to avoid silently dropping expired duties when the output channel is full, and corrects a misleading comment about duty selection order.
Changes:
- Make
deadliner.runalways block (or exit on context cancel) until a deadlined duty is delivered todeadlineChan, instead of dropping it on a full channel. - Fix
getCurrDuty’s comment to say it selects the earliest deadline (matching the implementation).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case <-currTimer.Chan(): | ||
| // Send deadlined duty to receiver. | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case d.deadlineChan <- currDuty: | ||
| default: | ||
| log.Warn(ctx, "Deadliner output channel full", nil, | ||
| z.Str("label", d.label), | ||
| z.Any("duty", currDuty), | ||
| ) | ||
| } | ||
|
|
||
| delete(duties, currDuty) | ||
| setCurrState() | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case d.deadlineChan <- currDuty: | ||
| } | ||
|
|
||
| delete(duties, currDuty) | ||
| setCurrState() |
There was a problem hiding this comment.
This change removes the non-blocking send that previously dropped duties when deadlineChan was full. Please add/extend a unit test that fills the deadlineChan buffer (e.g., 11+ duties with the same deadline) and asserts that all duties are eventually delivered (none dropped) once the consumer starts reading, to prevent regressions.
|
@enlightened88 did you experience any bug or degraded performance because of the current logic? Currently the buffer is 10, which is more than enough to support all duties before they time out. For gloas we will likely increase it slightly though, because of PTC. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Андрій Шовкошитний <198119344+enlightened88@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Андрій Шовкошитний <198119344+enlightened88@users.noreply.github.com>
|
Remove the default branch from the timer case in deadliner.run to ensure deadlined duties are never silently dropped when the output channel is full. Also fix an incorrect comment in getCurrDuty that said latest instead of earliest