Skip to content

refactor(lease): inject WallClock trait into count_matching_leases - #249

Closed
EnRaiha wants to merge 2 commits into
NodeDB-Lab:mainfrom
EnRaiha:feat/wallclock-trait
Closed

EnRaiha wants to merge 2 commits into
NodeDB-Lab:mainfrom
EnRaiha:feat/wallclock-trait

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Lease expiry is a duration question, so it must be measured against real wall time. PR #246 established this after the HlcClock::peek() wedge (a frozen HLC on an idle cluster makes every lease look unexpired). This extracts the clock behind a WallClock trait so the expiry check in count_matching_leases is injectable.

  • New nodedb/src/control/lease/clock.rs: WallClock trait, RealWallClock (production), MockClock (tests).
  • count_matching_leases now takes &dyn WallClock instead of calling super::wall_now_ns() directly.
  • wait_for_lease_drain passes &RealWallClock.
  • Adds 3 MockClock tests that pin the clock and prove expiry uses the injected wall clock, not the HLC (including the skewed-HLC case that must not drop a live lease).

Scope

Single logical change: clock injection for lease expiry. No behaviour change in production (real wall time is still used). Under ~80 lines of new code + tests.

Test plan

cargo test -p nodedb --lib control::lease::drain_propose (existing + 3 new tests).

Follow-up to #246.

Lease expiry is a duration question, so it must be measured against real
wall time (PR NodeDB-Lab#246 established this after the HlcClock::peek wedge). This
extracts the clock behind a WallClock trait so the expiry check is
injectable: RealWallClock for production, MockClock for tests.

count_matching_leases now takes &dyn WallClock instead of calling
super::wall_now_ns() directly. Adds 3 MockClock tests that pin the clock
and prove expiry uses the injected wall clock, not the HLC.

Follow-up to NodeDB-Lab#246; small, single-purpose per review feedback.
Copilot AI lite review requested due to automatic review settings August 25, 2026 03:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The clock abstraction is a general utility (lease expiry is just the first
consumer), so it belongs in util/ next to bounded_json/bounded_msgpack.
No behaviour change; imports updated. wall_now_ns re-export widened to
pub(crate) so util can reach it.
@farhan-syah

farhan-syah commented Aug 29, 2026 •

Copy link
Copy Markdown
Member

Closing: the wedge is already fixed on main, and the tests here duplicate tests that already exist.

Correction to my earlier comment on this PR: it credited the fix to #246. That is wrong. #246 was closed unmerged on 2026-08-24. The fix reached main as a separate commit, cited below. The rest of that comment stands, and is restated here in full.

The fix is on main as 7168ecc

7168ecc55 — fix(control): drop lease-drain wedges from stale or expired holders, 2026-08-24 — added the membership filter and the wall-time expiry comparison directly. count_matching_leases already reads wall time, not the HLC:

  • nodedb/src/control/lease/drain_propose.rs:256 — let now_wall_ns = super::wall_now_ns();
  • Rationale documented at drain_propose.rs:235-243.

That commit predates this PR by a day. This PR changes no production behaviour, as its own Scope section states. What remains is a test seam.

The three new tests duplicate existing ones

7168ecc55 also added four tests, three of which cover the same ground:

This PR Already on main
wall_clock_expired_lease_is_dropped expired_lease_does_not_block_drain_count (drain_propose.rs:493)
wall_clock_ignores_skewed_hlc a_live_lease_still_blocks_when_the_hlc_runs_ahead_of_wall_time (drain_propose.rs:553)
wall_clock_live_lease_is_counted member_unexpired_lease_still_blocks_drain_count (drain_propose.rs:583)

The fourth, expired_lease_stops_blocking_even_with_an_unadvanced_hlc (drain_propose.rs:513), has no counterpart in this PR.

The existing fixtures derive expiry from real wall_now_ns() with a 60-second margin. That is deliberate, and the comment at drain_propose.rs:454 says why: a fixture built from the same clock the code reads puts both sides in one frame and the assertion holds whatever the comparison does. MockClock reintroduces that. A test that pins the clock the code reads proves the code reads the injected value, not that it reads wall time.

Design problems in the seam itself

  • Inverted layering. RealWallClock::now_ns in nodedb/src/util/wall_clock.rs calls crate::control::lease::wall_now_ns(), so util depends upward on control. The PR widens pub(super) use wall_time::wall_now_ns to pub(crate) to allow it. If the abstraction belongs in util, wall_now_ns moves there and control::lease calls down.
  • One implementation, one call site. wait_for_lease_drain hardcodes &RealWallClock. Nothing else injects. This adds &dyn dispatch to a poll loop for tests that already pass.
  • Dead API. MockClock::set is never called.
  • Churn. Eight call sites rewritten to pass &clock_now(), a helper that reads real wall time and hands it straight back.

It no longer applies

f19756c96 (2026-08-28) added the own_holds: u32 self-exclusion parameter to count_matching_leases (drain_propose.rs:250-282). The signature has moved and this branch conflicts.

The correct fix

A crate-wide clock source in util or nodedb-types, owning wall_now_ns outright, with renewal::tick (nodedb/src/control/lease/renewal.rs:176) calling through it as well. That gives one source, no upward dependency, and a second caller that earns the indirection — and it would let renewal tests advance time instead of sleeping. That is a separate PR, and no current test is blocked on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants