Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the core Deadliner component implementation and updates its tests to match the new execution model.
Changes:
- Replaces the previous channel/timer-driven deadliner with a mutex-protected duty set and a periodic ticker scan.
- Adjusts test synchronization to wait for the deadliner goroutine to be ready before advancing the fake clock.
- Updates expiration triggering in tests to account for strict
Before()deadline comparisons.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| core/deadline.go | Reimplements deadliner internals (duty storage, expiration detection, output channel) using a 1s ticker and mutex. |
| core/deadline_test.go | Updates tests to synchronize with the ticker-based goroutine and advances the clock past the strict deadline boundary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case d.expiredChan <- expiredDuty: |
There was a problem hiding this comment.
run sends expired duties into expiredChan without a non-blocking/default case. If the consumer isn't reading fast enough and the channel buffer fills, the deadliner goroutine will block indefinitely (stalling expirations and preventing cleanup). If blocking is not intended, reintroduce a non-blocking send with a warning/drop (as previously), or otherwise ensure sufficient buffering/consumer guarantees.
| case d.expiredChan <- expiredDuty: | |
| case d.expiredChan <- expiredDuty: | |
| default: | |
| // Avoid blocking the deadliner goroutine if the consumer is not reading fast enough. | |
| log.Warn(ctx, "Deadliner.run() dropping expired duty; expired channel full", z.Any("duty", expiredDuty)) |
| marginFactor = 12 | ||
| marginFactor = 12 | ||
| expiredBufferSize = 10 | ||
| tickerInterval = time.Second |
There was a problem hiding this comment.
Using a fixed 1s ticker (tickerInterval = time.Second) reduces expiration precision. This can materially change behavior when slotDuration is < 12s (e.g., simnet uses 1s slots), since deadlines like slotDuration/3 + margin are sub-second and may be detected up to ~1s late. Consider deriving the polling interval from slotDuration/margin (or reverting to a timer for the next deadline) to preserve timely expiration across configurations.
| tickerInterval = time.Second | |
| tickerInterval = 100 * time.Millisecond |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4386 +/- ##
==========================================
- Coverage 56.67% 56.63% -0.05%
==========================================
Files 237 237
Lines 31681 31659 -22
==========================================
- Hits 17956 17929 -27
- Misses 11438 11441 +3
- Partials 2287 2289 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Send the expired duty to the receiver. | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case d.expiredChan <- expiredDuty: |
There was a problem hiding this comment.
run blocks when expiredChan is full because the send has no default case. If a consumer only drains C() opportunistically (e.g., via a non-blocking select in other components) or is temporarily slow, this can permanently stall the deadliner goroutine and prevent future expirations. Consider making the send non-blocking again (logging/warning on drop), increasing/deriving buffer sizing, or queueing internally so the expiration loop never blocks on downstream consumers.
| // Send the expired duty to the receiver. | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case d.expiredChan <- expiredDuty: | |
| // Send the expired duty to the receiver without blocking indefinitely. | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case d.expiredChan <- expiredDuty: | |
| default: | |
| // Avoid blocking the deadliner if the channel is full; log and drop this duty. | |
| log.Warn(ctx, "Deadliner.run() dropping expired duty; expiredChan full", z.Any("duty", expiredDuty)) |
| for duty, deadline := range d.duties { | ||
| if deadline.Before(now) { | ||
| expiredDuties = append(expiredDuties, duty) | ||
| delete(d.duties, duty) |
There was a problem hiding this comment.
Expiration uses deadline.Before(now) which treats deadline == now as not expired; with a 1s ticker this can delay expiry by up to one tick even when the deadline has been reached. If the intended semantics are “expire at or after the deadline”, use a non-strict comparison (e.g., !deadline.After(now)) so equality is considered expired.
| select { | ||
| case <-d.quit: | ||
| case <-d.done: | ||
| // Run goroutine has stopped, ignore new duties. | ||
| return false | ||
| case d.inputChan <- deadlineInput{duty: duty, success: success}: | ||
| default: | ||
| } | ||
|
|
||
| select { | ||
| case <-d.quit: | ||
| deadline, canExpire := d.deadlineFunc(duty) | ||
| if !canExpire { | ||
| // Drop duties that never expire | ||
| return false | ||
| } | ||
|
|
||
| expired := deadline.Before(d.clock.Now()) | ||
| if expired { | ||
| // Drop expired duties | ||
| return false | ||
| case ok := <-success: | ||
| return ok | ||
| } | ||
|
|
||
| d.lock.Lock() | ||
| defer d.lock.Unlock() | ||
|
|
||
| d.duties[duty] = deadline | ||
|
|
||
| return true |
There was a problem hiding this comment.
Add checks <-d.done before computing/storing, but this isn’t synchronized with run closing done. If run stops concurrently (ctx cancellation) after the check but before d.duties[duty] = deadline, Add can still return true and store a duty that will never be expired (goroutine exited). To avoid this race, consider tracking a stopped flag under d.lock (set in run before closing done) and re-check it under the same lock before mutating d.duties.
pinebit
added
the
merge when ready
Deadliner component refactoring. category: refactor ticket: none
Deadliner component refactoring.
category: refactor
ticket: none