fix(lease): clamp lease expiry against MAX_SKEW clock skew

[EnRaiha]

Summary

Reverse-direction guard for the #246 wedge. The hotfix (7168ecc55) judges expiry against the local wall clock, but a lease stamped by a holder whose clock is behind ours then looks already-expired → false expiry → DDL proceeds under a live holder (the correctness bug #246 refuses to trade for).

count_matching_leases now keeps any lease whose expiry is inside a 5-minute MAX_SKEW window (expires_at > now - MAX_SKEW) and drops only leases expired beyond it.

Stacked on #249 (WallClock trait): this branch contains the trait first; once #249 merges, this PR's diff reduces to the clamp + tests only.

Changes

• MAX_SKEW_NS = 300_000_000_000 (5 min) in drain_propose.rs
• live_threshold = now.saturating_sub(MAX_SKEW_NS) in the drain filter
• expired() test fixture moved beyond the skew window (60s-past leases are now live by design)
• 3 new MockClock tests: within-window kept, beyond-window dropped, far-future kept

Trade-off (safety-first, per #246)

A genuinely-dead lease drains up to MAX_SKEW later. Never dropping a live hold is the priority.

Verification (solve-first)

• cargo check -p nodedb --lib --tests — PASSED (exit 0)
• cargo test -p nodedb --lib lease::drain_propose::tests — 15/15 PASSED

[Copilot]

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

[farhan-syah]

Closing: right problem, and it is a tracked one — but this constant converts the crash wedge into a guaranteed DDL failure, and the clamp cannot land before the crash-release hook it depends on.

The premise holds

Cross-node lease expiry really is unbounded here, and #165 already tracks it:

Descriptor-lease expiry uses raw cross-node wall clock, no skew bound / fencing token. Partial. ... Remaining: MAX_SKEW clamp, and a SWIM-Dead to on_node_crash release hook.

expires_at is stamped from hlc_clock.now() (propose.rs:192 → compute_expires_at, propose.rs:26), and HlcClock::now() is wall.max(st.wall_ns) (nodedb-types/src/hlc.rs:96) — monotone per node, but never folded against a peer. HlcClock::update has exactly one production caller, catalog_entry/descriptor_stamp.rs:65, and it folds only the prior catalog entry's modification_hlc for the same descriptor during DDL stamping. No Raft, cluster, or lease path feeds it. The two frames are independent local clocks. So a clamp is the right idea.

The constant is fatal

Constant	Value	Source
MAX_SKEW_NS (this PR)	300 s	drain_propose.rs:23
DEFAULT_LEASE_DURATION	300 s	propose.rs:16
DEFAULT_DRAIN_TIMEOUT	35 s	metadata_proposer.rs:55

The skew window is 8.6× the drain timeout, and one full lease lifetime.

A crashed holder's lease expires at T. On main, the filter drops it at T and the DDL proceeds. With this clamp it keeps blocking until T+300 s, while wait_for_lease_drain gives up at T+35 s and returns Error::Config { "descriptor lease drain timed out after 35s ..." }. Every DDL on that descriptor fails for five minutes.

The Trade-off section says "a genuinely-dead lease drains up to MAX_SKEW later". It does not drain later. Nothing waits 300 s for it — the caller has already failed. That is the 7168ecc55 wedge, converted from a hang into a hard error.

Expiry is the only crash-recovery path today

gc_leases_for_node fires solely on a node-left membership event (cluster/metadata_applier/dispatch.rs:282), and collect_non_member_leases (gc.rs:25) matches only holders already absent from the topology. Between a node crashing and the failure detector evicting it, the expiry filter is the sole mechanism that releases its leases.

This is why #165 lists the clamp and the SWIM-Dead release hook together. The hook has to land first. Once a Dead verdict releases leases as an event, expiry becomes a backstop and a skew window is affordable. Clamping expiry while it is still the only path removes crash recovery for the width of the window.

Ordering, and the constraint on the constant:

1. SWIM-Dead → on_node_crash lease release. Event-driven, no clock comparison.
2. Only then, a skew bound — derived from and asserted against DEFAULT_DRAIN_TIMEOUT, not an independent literal. Two constants that must not cross should not be declared 200 lines and one module apart.

Test fixture masking

expired() moves to now - 360s so the existing tests still pass. expired_lease_does_not_block_drain_count then asserts about a six-minute-stale lease and no longer covers the ordinary post-TTL case — the exact case this change breaks. The fixture was adjusted to fit the code.

Also

Stacked on #249, which is closed, so this carries the whole util/wall_clock.rs trait with it: 227 added lines in drain_propose.rs for a two-line filter change.

Filed separately

The dead HlcClock::update path is a real defect wider than leases and was not tracked anywhere: #264.