refactor: improve the deadline module

[therustmonk]

No description provided.

[mskrzypkows]

would be good to have a unit test for it

[mskrzypkows]

ditto

[mskrzypkows]

Maybe return some small sleep time here to retry in a while?

[therustmonk]

@claude review the changes

[claude]

Claude finished @therustmonk's task in 3m 32s —— View job

---

Code Review: PR #423 — refactor: improve the deadline module

• Read changed files
• Compare against Charon Go source for parity
• Analyze correctness, design, and style
• Post review feedback

---

Summary

Nice refactor overall. Splitting the 743-line deadline.rs into mod.rs / calculator.rs / millis.rs reads cleanly, the Millis newtype removes a lot of u64/i64 try_from juggling, and the trait split (DeadlineCalculator / Deadliner) makes the deadliner generic over the deadline function, which lets NoopDeadliner and the production calculator coexist without Arc<dyn Fn>. The previously-flagged unit tests for slot_start and margin have been added (calculator.rs:123-190). The deadliner FSM is functionally faithful to charon/core/deadline.go — same input/output channel pattern, same "drop on full" semantics, same recompute-on-insert logic.

A few correctness and design points below.

---

Correctness

1. Sentinel duty can leak to the output channel after ~10 years of idle  — crates/core/src/deadline/mod.rs:323-342

When self.duties is empty, curr_deadline = MAX_UTC and remaining_duration() returns FAR_FUTURE_DURATION (~10 years). If the timer ever fires while no duties are tracked, handle_expired will try_send(self.curr_duty.clone()) — and curr_duty is the sentinel Duty::new(SlotNumber::new(0), DutyType::Unknown). That's a real (Unknown, slot 0) duty being shipped to the consumer. Astronomically unlikely in practice, but trivial to guard:

fn handle_expired(&mut self) -> Option<Duration> {
    if self.duties.is_empty() {
        // Sentinel timer fired with no real duty; just reschedule.
        return Some(self.remaining_duration());
    }
    // ...rest unchanged
}

Fix this →

2. OUTPUT_BUFFER = 256 deviates from Charon's outputBuffer = 10  — mod.rs:179

charon/core/deadline.go:118 uses 10 — "big enough to support all duty types, which can expire at the same time while external consumer is synchronously adding duties." Project rule is functional equivalence with Go; bumping to 256 hides backpressure that would surface earlier under Go. Either drop to 10 to match, or add a comment justifying the deviation. Same question for INPUT_BUFFER = 256 vs Go's unbuffered input channel (the oneshot reply still serializes writers, so it's behaviorally equivalent — but worth a sentence saying so).

3. mskrzypkows's comment on mod.rs:333 (channel-full path)

The reviewer suggests returning a small sleep time to retry instead of dropping the duty. The current behavior — log + drop + advance — matches charon/core/deadline.go:184-188, which also drops on full via default:. So this is a parity decision, not a bug. Worth replying on the thread that this is intentional Charon parity; if retry is desired it should be a separate decision applied symmetrically with Charon.

4. handle_input calls recompute_curr() after inserting the strictly-earlier duty  — mod.rs:294-297

Not a bug, just wasteful: we already know the new duty has the smallest deadline, so an O(n) scan re-confirms what we already know. Could be:

self.duties.insert(duty.clone());
self.curr_duty = duty;
self.curr_deadline = deadline;
Some(self.remaining_duration())

Skip if you want to keep the code path uniform — the HashSet is small in practice.

---

Design / API

5. DeadlinerLink vs DeadlinerImpl naming is inverted  — mod.rs:121, 158

The thing that impl Deadliner for X is DeadlinerLink (private). The thing called DeadlinerImpl is the task state, doesn't implement the trait, and is only exposed for its start() constructor. Reader-hostile. Two options:

• Rename: DeadlinerImpl → DeadlinerTask (or keep private and expose pub fn start_deadliner(...)); DeadlinerLink → DeadlinerHandle.
• Make DeadlinerImpl private and add a free function pub fn start<C: DeadlineCalculator>(...) -> Arc<dyn Deadliner>.

The doc comment on DeadlinerImpl already calls it "Owned state of the background task" — the type name should reflect that.

6. Deadliner trait docstring conflates layers  — mod.rs:102-104

add doc says it returns false "if the duty never expires (e.g., Exit, BuilderRegistration)" — but that's a property of DutyDeadlineCalculator, not of Deadliner. Any calculator returning Ok(None) causes add to return false. Reword to "if the calculator reports the duty has no deadline" to keep the abstraction clean.

7. Millis doc duplication  — millis.rs:1-3 and millis.rs:12-14

Module-level doc and struct-level doc say the same thing nearly verbatim. Drop one.

---

Tests

8. deadliner() test uses real wall-clock and a 500ms slot  — mod.rs:436-520

Real-time tests with sub-second deadlines are notorious on loaded CI. The 5s timeout for ~1s expirations is generous, but consider tokio::time::pause() + advance() for deterministic timing. Not blocking — flagging in case it goes flaky later.

9. tests::deadliner doesn't assert ordering across receiving channel  — mod.rs:503-510

The test does actual_duties.sort_by_key(...) before comparing — fair, since two duties with the same deadline have nondeterministic emission order. But the test doesn't directly verify that duties arrive in deadline order, which is the deadliner's actual contract. Worth a follow-up test with two duties of clearly different deadlines that asserts the earlier one arrives first.

10. Voluntary-exits test path is misleading  — mod.rs:417-421 and mod.rs:490-499

The TestCalculator returns Some(start + 1h) for DutyType::Exit instead of None. So the test labeled "voluntary_exits" actually exercises the "non-expired with future deadline" path, not the "never expires" path. The "never expires" branch is covered by never_expire_duties, but rename the local var (voluntary_exits → future_duties or similar) to avoid the false-friend reading.

---

Other

• crates/core/src/types.rs:73-77 — DutyType::never_expires() is well-placed and matches the Go switch.
• Millis::checked_div doc says "overflow or division by zero" but i64::checked_div only fails on division by zero (and MIN / -1); the doc is slightly misleading. Minor.
• The module-level doc example (mod.rs:7-33) uses DeadlinerImpl::start — works because pub struct DeadlinerImpl<C> is reachable through the public deadline module, but if you move to a free function (see point 5), update the doc.

---

Verdict

Solid refactor, well-tested, faithful to Charon. The points above are mostly polish; the only real correctness item is #1 (sentinel-duty leak after 10y idle) and the #2 buffer-size deviation from Charon, which the golden rule on functional equivalence asks us to justify or revert.
• Branch

[iamquang95]

This can be i64 from the begin, no casting later

[iamquang95]

msecs is better name i think, it's Millis