Skip to content

fix(umbp): a remote read checks after the copy that it read the right key - #787

Draft
TianDi101 wants to merge 1 commit into
fix/umbp-ssd-staging-local-leasesfrom
fix/umbp-remote-read-validate
Draft

TianDi101 wants to merge 1 commit into
fix/umbp-ssd-staging-local-leasesfrom
fix/umbp-remote-read-validate

Conversation

@TianDi101

Copy link
Copy Markdown
Collaborator

Stacked on #762 (base branch fix/umbp-ssd-staging-local-leases). Retarget to main once #762 merges. Tests are built but not run yet; we run them after #762 lands.

Part of #749 (the remote-reader half).

Problem

A remote reader's only protection on the peer is a read lease, and the lease is just a timer (UMBP_DRAM_READ_LEASE_MS / UMBP_SSD_READ_LEASE_MS). If the RDMA read takes longer than the lease, the peer can free the pages and refill them with another key. The reader then returns those bytes as a successful read of its own key. No error, wrong data.

Local readers were fixed in #762 (they hold their bytes by refcount). A remote reader cannot hold a refcount across the network, so it needs a different answer.

Fix: check after the read (option 1 in #749)

  1. Every copy gets a stamp. When a medium fills a slot it draws a generation from one process-wide counter: PageBackend (DRAM/HBM) at commit, SsdBackend when it creates a staging lease. Freeing the copy drops the stamp. A refill, even of the same key, draws a new one. So "same generation" means "these bytes were never reused". A key check alone cannot tell (ABA).
  2. The stamp travels with the resolve. BatchResolveKeysResponse.generation (new optional field 11, one per key).
  3. After the RDMA wait, ask once. WaitRemoteBatchGet sends one ValidateReads RPC per peer batch with (key, backend_id, generation) for every read that landed. The peer answers from MediumBackend::BatchStillHolds (via PeerPool::ValidateResolved). The check is read-only and takes no lease.
  4. Not vouched for → miss. This runs before FinalizeRemoteGetEntries, so a stale read is neither reported as a hit nor re-cached locally.

Failure policy

Case Result
peer says copy changed miss, mori_umbp_client_remote_get_dropped_total{reason="stale"} + WARN
RPC error / wrong answer count miss for every read in the batch, reason="unverified" (fail closed)
peer sends no generation (older build) not asked, same behavior as today
UMBP_REMOTE_READ_VALIDATE=0 check off (for measuring its cost only)

Also in here

  • InstrumentedBackend forwards BatchStillHolds. The base default answers "true", which would wave every stale read through on every wrapped medium.
  • The UMBP_RANGED_CALL_DEBUG summary gets a chk= share, so the extra round trip is visible on its own instead of inside fin=.
  • runtime-env-vars.md documents UMBP_REMOTE_READ_VALIDATE.

Cost

One extra small RPC per remote peer batch, after the RDMA read. The payload is keys + 12 bytes each. Batches are waited one after another, so with several peers the RPCs also run one after another. That is the number to measure in E2E (chk= share, or an A/B with UMBP_REMOTE_READ_VALIDATE=0).

Tests (compiled on gfx950 host; whole tree builds; not run yet)

  • test_page_backend: same copy keeps its generation; Evict, ClearLocal, and evict-then-refill of the same key all go stale; another key's generation does not vouch; 0 is unchecked.
  • test_ssd_backend: the UMBP: a read lease is only a timer: a slow read can return another key's bytes, a fast one holds its memory for the full lease #749 shape on SSD. A lease lapses, the only staging page is handed to another key, and the check says stale even though the key is still on the drive; a re-stage is a new copy; ClearLocal goes stale.
  • test_batch_resolve_codec: round trip with full 64-bit values; old peer → 0; a short generation array is rejected as malformed.
  • test_component_metrics: InstrumentedBackend answers with the wrapped medium.
  • test_peer_pool: ValidateResolved asks each named backend once; unknown backend → false; mismatched vectors → all false.
  • test_peer_service_dispatch: over a real peer service, resolve → validate true → evict and refill the key → validate says stale for that key only; mismatched arrays → INVALID_ARGUMENT.

Not covered by a unit test: the client side (ValidateRemoteGetEntries), which needs a remote RDMA peer. To cover in the cross-node E2E after #762.

🤖 Generated with Claude Code

… key

Part of #749 (remote readers).  A remote reader's only protection on the
peer is a read lease, and the lease is a timer.  If the RDMA read outlives
it, the peer can free the pages and refill them with another key, and the
reader returns those bytes as a successful read of its own key.

Check after the read instead of trusting the timer:

- Every medium stamps each copy it serves with a generation: a process-wide
  counter drawn at PageBackend commit and at SSD staging-lease creation.
  Freeing the copy drops the stamp, and a refill (even of the same key)
  draws a new one, so an equal generation means the bytes were never reused.
- BatchResolveKeysResponse carries it per key (new optional field 11).
- After WaitRemoteBatchGet's RDMA wait, the reader sends one ValidateReads
  RPC per peer batch: (key, backend_id, generation) for every read that
  landed.  The peer answers from MediumBackend::BatchStillHolds via
  PeerPool::ValidateResolved, read-only, no lease taken or extended.
- A read the peer does not vouch for becomes a miss before Finalize, so it
  is neither reported as a hit nor re-cached locally.  Counted in
  mori_umbp_client_remote_get_dropped_total{reason="stale"}.
- Fails closed: an RPC error or a wrong answer count drops the batch's
  reads as reason="unverified".  A peer that sends no generation (older
  build) is not asked.  UMBP_REMOTE_READ_VALIDATE=0 turns the check off
  for measuring its cost.

InstrumentedBackend forwards BatchStillHolds; the base default (all true)
would otherwise wave every stale read through on every wrapped medium.
The ranged-call debug summary gets a chk= share for the new round trip.

Local readers are unchanged: they hold their bytes by refcount (#762).

Tests (built, not run yet; to run after #762 merges): page and SSD
backends (same copy keeps its generation; evict, clear, re-stage and
refill all go stale), codec (round trip, old peer reads as 0, a short
generation array is malformed), InstrumentedBackend forwarding,
PeerPool::ValidateResolved grouping, and the RPC over a real peer service
(resolve, evict and refill the key, validate says stale).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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.

1 participant