refactor: improve deadliner api

[therustmonk]

API changes for #423

[therustmonk]

@claude review the changes

[claude]

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

---

Review: refactor: improve deadliner api

• Gather context (read changed files)
• Compare with Go reference (charon/core/deadline.go, charon/core/parsigdb/memory.go, charon/core/dutydb/memory.go)
• Analyze API changes
• Check correctness & idiomatic Rust
• Post review

---

Overall

Solid refactor — the API is meaningfully cleaner. Removing the async_trait / Arc<dyn Deadliner> indirection drops a vtable + heap-alloc per call site, the take-once Mutex<Option<Receiver>> pattern is gone (was both awkward and runtime-panic-prone), and the explicit AddOutcome enum closes the long-standing "what does false actually mean?" ambiguity from the Go interface. Centralizing NeverExpiringCalculator in the calculator module is also nicer than duplicating ad-hoc NoopDeadliner impls in dutydb tests and exchanger.rs.

The semantics still mirror charon/core/deadline.go faithfully (Go's Deadliner.Add returns one bool for all three non-success conditions; you've split them out, which is strictly an improvement over Charon).

---

Issues

1. Broken intra-doc links (likely cargo doc warnings → CI failure under -D warnings)

The Deadliner trait is gone but rustdoc references to it remain:

• crates/core/src/deadline/mod.rs:3 — //! This module provides the [\Deadliner`] trait for tracking duty deadlines`
• crates/core/src/deadline/mod.rs:85 — /// Outcome of [\Deadliner::add`].onAddOutcome`
• crates/core/src/parsigdb/memory.rs:306 — /// ... paired with the [\Deadliner`] handle at`

Suggest updating to [\DeadlinerHandle::add`]/[`DeadlinerHandle`]`. Fix this →

2. dutydb::MemDB::store collapses three AddOutcome variants back to ExpiredDuty

crates/core/src/dutydb/memory.rs:250:

if !self.deadliner.add(duty.clone()).await.is_scheduled() {
    return Err(Error::ExpiredDuty);
}

NoDeadline and FailedToCompute both surface as ExpiredDuty. In practice the duty types reaching this branch (Proposer/Attester/Aggregator/SyncContribution) all have deadlines, so the misclassification is unreachable today — but the whole point of introducing AddOutcome is to not re-conflate these. Worth either a match that maps each variant explicitly, or a comment explaining why this call site is fine with the collapse. (Go has a TODO(corver): Distinguish between no deadline supported vs already expired on the same Add call — this PR is the place to actually do it.)

3. mem_db_store_unsupported masks Exit/BuilderRegistration behavior

crates/core/src/dutydb/memory.rs test mem_db_store_unsupported uses FarFutureCalculator, whose deadline() always returns Some(MAX_UTC) — ignoring duty_type.never_expires(). With the production DutyDeadlineCalculator, Exit and BuilderRegistration would return Ok(None) → NoDeadline → ExpiredDuty, short-circuiting before the match ever reaches the UnsupportedDutyType arm. The test would still pass (errors-out either way), but the assertion err.to_string().contains("unsupported duty type") would fail. Combined with issue #2, this is a latent surprise. A separate case using DutyDeadlineCalculator (or NeverExpiringCalculator to exercise the NoDeadline path) would catch the divergence.

---

Nits

4. parsigdb::MemDB::trim is dead in production

Pre-existing, not introduced here, but worth flagging: nothing in crates/app or crates/cli spawns a trim task, so the parsigdb never evicts entries outside tests. Not in scope for this PR.

5. DeadlinerTask::start for NeverExpiringCalculator spawns an idle task

The DKG exchanger wires a real background task that will only ever serve NoDeadline responses. Cheap (one yielded task, dormant), and necessary to keep the API uniform. Reasonable. Optional alternative: add a null_handle() constructor that synthesizes a DeadlinerHandle whose add() always returns NoDeadline without crossing channels. Not blocking.

6. DeadlinerHandle::add cancellation race

The early if self.cancel_token.is_cancelled() check can race with task shutdown; the response_rx.await.unwrap_or(AddOutcome::FailedToCompute) correctly handles the case where the task dropped the oneshot. Correct as written — flagging only to confirm I traced it.

7. Pre-existing recompute_curr O(N) complexity

Untouched by this PR; Go has the same TODO(dhruv): optimise ... using min heap. Out of scope but worth a follow-up issue.

---

Test coverage

The duty_expiry test now uses a real DeadlinerHandle (so store()'s add(...).is_scheduled() actually exercises the channel) and drives eviction through a manually-controlled mpsc::Sender<Duty>. Strictly better than the prior TestDeadliner shim — fewer mocks, more of the real path.

The parsigdb memdb_threshold test follows the same pattern with NeverExpiringCalculator + manual trim_tx. Good.

---

Verdict

LGTM modulo the broken rustdoc links (#1, almost certainly a CI blocker) and the dutydb error-collapse (#2, semantic) — both small. #3 is a follow-up suggestion. Approve once #1 is fixed.
• Branch: kd-may22-follow

[emlautarom1]

LGTM, removes quite a bit of machinery and potential issues.

[emlautarom1]

nit: Prefer to have a separate section documenting the existing bool behavior of Charon with a permalink to the TODOs.

[emlautarom1]

Usually you want to differentiate all cases. Only usage right now is dutydb, so I suggest exploring if there is an issue there rather than modifying this API.