From 9d2e8ee05b2362a72f4ab16a0c17056f406c6bca Mon Sep 17 00:00:00 2001 From: Grzegorz Koszyk Date: Tue, 1 Sep 2026 14:51:44 +0200 Subject: [PATCH 1/3] fix(cluster): serve metadata reads at or above the client's own writes --- .../apache/iggy/bdd/BasicMessagingSteps.java | 8 +- bdd/rust/tests/steps/streams.rs | 31 +- core/metadata/src/impls/metadata.rs | 59 +++ core/server/src/bootstrap.rs | 19 +- core/server/src/dispatch.rs | 137 ++++++- core/server/src/http.rs | 2 +- core/server/src/http/error.rs | 26 +- core/server/src/http/extractor.rs | 18 +- core/server/src/http/reads.rs | 210 +++++++++- core/server/src/http/reply.rs | 2 +- core/server/src/http/session.rs | 118 +++++- core/server/src/http/state.rs | 40 +- core/server/src/http/submit.rs | 14 +- core/server/src/session_manager.rs | 89 ++++- core/simulator/src/client.rs | 47 ++- core/simulator/src/lib.rs | 376 +++++++++++++++++- core/simulator/src/replica.rs | 11 +- 17 files changed, 1134 insertions(+), 73 deletions(-) diff --git a/bdd/java/src/test/java/org/apache/iggy/bdd/BasicMessagingSteps.java b/bdd/java/src/test/java/org/apache/iggy/bdd/BasicMessagingSteps.java index 0fc05c28c0..c5439f201c 100644 --- a/bdd/java/src/test/java/org/apache/iggy/bdd/BasicMessagingSteps.java +++ b/bdd/java/src/test/java/org/apache/iggy/bdd/BasicMessagingSteps.java @@ -142,8 +142,14 @@ public void deleteStreamByNumericId() { @Then("getting the stream by its numeric ID should return no stream") public void getStreamReturnsNoStream() { + // The assertion is "not the stream we deleted", not "nothing at this id": + // the server hands out the lowest free stream id, so once these scenarios + // run concurrently against one server a fresh create can legitimately + // occupy the deleted stream's id. Optional stream = getClient().streams().getStream(context.lastStreamId); - assertTrue(stream.isEmpty(), "Deleted stream should not be returned"); + assertTrue( + stream.isEmpty() || !stream.get().name().equals(context.lastStreamName), + "Deleted stream should not be returned"); } @When("I create a topic with name {string} in stream {int} with {int} partitions") diff --git a/bdd/rust/tests/steps/streams.rs b/bdd/rust/tests/steps/streams.rs index ba1248f3fa..f94ef4027c 100644 --- a/bdd/rust/tests/steps/streams.rs +++ b/bdd/rust/tests/steps/streams.rs @@ -18,11 +18,6 @@ use crate::common::global_context::GlobalContext; use cucumber::{given, then, when}; use iggy::prelude::{Identifier, StreamClient, StreamUpdateOptions}; -use std::time::Duration; -use tokio::time::{Instant, sleep}; - -const METADATA_CONVERGENCE_TIMEOUT: Duration = Duration::from_secs(2); -const METADATA_CONVERGENCE_POLL: Duration = Duration::from_millis(10); #[given("I have no streams in the system")] pub async fn given_no_streams(world: &mut GlobalContext) { @@ -128,18 +123,20 @@ pub async fn when_delete_stream_by_numeric_id(world: &mut GlobalContext) { #[then("getting the stream by its numeric ID should return no stream")] pub async fn then_get_stream_returns_no_stream(world: &mut GlobalContext) { - let deadline = Instant::now() + METADATA_CONVERGENCE_TIMEOUT; - loop { - get_stream_by_numeric_id(world).await; - if world.last_stream_name.is_none() { - return; - } - assert!( - Instant::now() < deadline, - "Deleted stream should not be returned after {METADATA_CONVERGENCE_TIMEOUT:?}" - ); - sleep(METADATA_CONVERGENCE_POLL).await; - } + // Read before the get overwrites it. The assertion is "not the stream we + // deleted", not "nothing at this id": `IdSlab::insert` hands out the lowest + // free key and these scenarios share one server, so a concurrent create can + // legitimately occupy the deleted stream's id. + let deleted = world + .last_stream_name + .clone() + .expect("Stream should have been created"); + get_stream_by_numeric_id(world).await; + assert_ne!( + world.last_stream_name.as_ref(), + Some(&deleted), + "Deleted stream should not be returned" + ); } async fn create_stream(world: &mut GlobalContext, stream_name: &str) { diff --git a/core/metadata/src/impls/metadata.rs b/core/metadata/src/impls/metadata.rs index 6ff355e474..e0d7e57756 100644 --- a/core/metadata/src/impls/metadata.rs +++ b/core/metadata/src/impls/metadata.rs @@ -66,6 +66,8 @@ use std::cell::{Cell, RefCell}; use std::mem::size_of; use std::path::Path; use std::rc::Rc; +use std::sync::Arc; +use std::sync::atomic::{AtomicU64, Ordering}; use tracing::{debug, error, info, warn}; fn freeze_client_reply( @@ -769,6 +771,21 @@ pub struct IggyMetadata { /// whole snapshot on shard 0's pump, and hands each requester its own /// multi-MB copy. transfer_offer_cache: RefCell>>, + /// Highest metadata op whose apply has been PUBLISHED on this node, shared + /// by every shard. + /// + /// `consensus.commit_min()` answers the same question but exists only on + /// shard 0, so a read served by a peer shard has no way to tell whether the + /// node caught up to an op its client already saw committed. One + /// process-wide atomic does, for one `Acquire` load on the read fast path. + /// + /// Written `Release` right after each apply's `publish()`, read `Acquire`; + /// observing `>= op` therefore happens-after that publish, so a following + /// left-right `enter()` is guaranteed to see the op. `fetch_max` rather + /// than `store` because three writers move it -- the commit loop, the + /// recovery seed, and a state-transfer install -- and only monotonicity + /// makes their order irrelevant. + applied_frontier: Arc, } impl IggyMetadata @@ -808,11 +825,41 @@ where commit_notifier: RefCell::new(None), client_table_frontier: Cell::new(0), transfer_offer_cache: RefCell::new(None), + applied_frontier: Arc::new(AtomicU64::new(0)), } } } impl IggyMetadata { + /// Share one process-wide applied frontier with every other shard. + /// + /// Consumed at construction rather than swapped in later: a shard that + /// served a read against its own private cell would gate on a number that + /// never moves. Shard 0 mints the cell in bootstrap, before any shard is + /// built, and hands each shard a clone. + #[must_use] + pub fn with_applied_frontier(mut self, applied_frontier: Arc) -> Self { + self.applied_frontier = applied_frontier; + self + } + + /// Highest metadata op this NODE has applied and published, readable on + /// every shard. Reads gate on it so a client cannot be served state older + /// than a write it already saw acked. + #[must_use] + pub fn applied_frontier(&self) -> u64 { + self.applied_frontier.load(Ordering::Acquire) + } + + /// Publish `op` as applied. Monotone, so a lower value is a no-op. + /// + /// Must run AFTER the apply's `publish()` and, on the commit path, in the + /// same await-free region as `advance_commit_min`: a reader that sees the + /// frontier must be guaranteed to see the op's effects. + pub fn advance_applied_frontier(&self, op: u64) { + self.applied_frontier.fetch_max(op, Ordering::Release); + } + /// Slot capacity of the LIVE client table, i.e. the largest transferred /// table this replica can absorb. /// @@ -1839,6 +1886,7 @@ where if snapshot_seq > consensus.sequencer().current_sequence() { consensus.sequencer().set_sequence(snapshot_seq); } + self.advance_applied_frontier(snapshot_seq); } // Before the superblock write, so the durable record carries the frontier // this transfer just established rather than the pre-transfer one. @@ -2827,6 +2875,10 @@ where reply }; consensus.advance_commit_min(prepare_header.op); + // Paired with the counter bump, and before the reply leaves: a + // client that holds this reply may re-home onto any shard and read, + // and the read gate admits it only once the frontier covers the op. + self.advance_applied_frontier(prepare_header.op); emit_sim_event(SimEventKind::OperationCommitted, &event); // Fire subscriber BEFORE wire send. Slot already updated @@ -3574,6 +3626,7 @@ where prepare, ); consensus.advance_commit_min(op); + self.advance_applied_frontier(op); debug!("commit_journal: committed op={op}"); } } @@ -5672,6 +5725,12 @@ mod tests { journal_handle.header(1).is_some() && journal_handle.header(2).is_some(), "ops at or below the floor stay for the walk and tail repair" ); + assert_eq!( + md.applied_frontier(), + SNAPSHOT_SEQ, + "the snapshot IS ops up to its sequence applied, so the read gate has \ + to admit reads at the floor the install jumped to" + ); } #[compio::test] diff --git a/core/server/src/bootstrap.rs b/core/server/src/bootstrap.rs index 4320c3bc5e..5066c147b3 100644 --- a/core/server/src/bootstrap.rs +++ b/core/server/src/bootstrap.rs @@ -802,6 +802,10 @@ pub fn bootstrap( // Shared metadata-group view: written by shard 0's publisher task, read by // every shard's cluster-metadata roster so leader marking works off-shard. let metadata_view = Arc::new(AtomicU64::new(crate::cluster_meta::METADATA_VIEW_UNKNOWN)); + // Shared applied-metadata frontier: shard 0's commit path advances it, every + // shard's read gate reads it. Minted here, before any shard exists, because + // a shard holding a private cell would gate reads on a number nothing moves. + let metadata_applied_frontier = Arc::new(AtomicU64::new(0)); // Every shard's metric handles, minted before the threads spawn: each // shard bumps its own entry, and shard 0's HTTP scrape endpoint registers // the whole set (counters are Arc-backed, so cross-thread reads see the @@ -842,6 +846,7 @@ pub fn bootstrap( }; let metadata_view_for_shard = Arc::clone(&metadata_view); + let applied_frontier_for_shard = Arc::clone(&metadata_applied_frontier); let shard_metrics_for_shard = shard_metrics_all.clone(); let handle = match thread::Builder::new() .name(format!("shard-{shard_id}")) @@ -860,6 +865,7 @@ pub fn bootstrap( barrier_for_shard, owner_table_for_shard, metadata_view_for_shard, + applied_frontier_for_shard, shard_metrics_for_shard, ) }) { @@ -925,6 +931,7 @@ fn run_shard_thread( barrier: BootstrapBarrier, owner_table: Arc, metadata_view: Arc, + metadata_applied_frontier: Arc, shard_metrics_all: Vec, ) -> Result<(), ServerError> { // Armed for the whole thread body: a post-spawn error `?` or a panic @@ -968,6 +975,7 @@ fn run_shard_thread( barrier, owner_table, metadata_view, + metadata_applied_frontier, shard_metrics_all, )) .await @@ -997,6 +1005,7 @@ async fn shard_main( barrier: BootstrapBarrier, owner_table: Arc, metadata_view: Arc, + metadata_applied_frontier: Arc, shard_metrics_all: Vec, ) -> Result<(), ServerError> { let topology = resolve_tcp_topology(config, replica_id)?; @@ -1148,7 +1157,15 @@ async fn shard_main( superblock_for_metadata, mux_stm, Some(PathBuf::from(&config.system.path)), - ); + ) + .with_applied_frontier(metadata_applied_frontier); + // Recovery already replayed the committed WAL prefix into the state + // machine, so the frontier resumes where the commit walk will rather than + // at zero -- otherwise every read on a rebooted node parks until its + // deadline. No-op on peer shards, which share shard 0's cell. + if let Some(consensus) = metadata.consensus.as_ref() { + metadata.advance_applied_frontier(consensus.commit_min()); + } // Size the VSR client table before listeners bind and any client registers. // Must precede the recovered-table install below: the setter rebuilds the // table from scratch, so running it afterwards would drop every resumed diff --git a/core/server/src/dispatch.rs b/core/server/src/dispatch.rs index 0a494f524b..8a3a51e685 100644 --- a/core/server/src/dispatch.rs +++ b/core/server/src/dispatch.rs @@ -37,6 +37,7 @@ use crate::dispatch::authz::{ authorize_default_read, authorize_partition_op, authorize_partition_read, authorize_uid, send_deny_reply, send_non_replicated_deny, send_unbound_deny_reply, }; +use crate::http::reply::transient_code; use crate::login_register::LoginRegisterError; use crate::pat::maybe_rewrite_pat_request; use crate::responses::{ @@ -60,10 +61,10 @@ use consensus::{ }; use iggy_binary_protocol::PrepareHeader; use iggy_binary_protocol::codes::{ - GET_CLIENT_CODE, GET_CLIENTS_CODE, GET_CLUSTER_METADATA_CODE, GET_CONSUMER_OFFSET_CODE, - GET_ME_CODE, GET_PERSONAL_ACCESS_TOKENS_CODE, GET_SNAPSHOT_FILE_CODE, GET_STATS_CODE, - LOGIN_USER_CODE, LOGIN_WITH_PERSONAL_ACCESS_TOKEN_CODE, PING_CODE, POLL_MESSAGES_CODE, - SYNC_CONSUMER_GROUP_CODE, + DESCRIBE_OPTIONS_CODE, GET_CLIENT_CODE, GET_CLIENTS_CODE, GET_CLUSTER_METADATA_CODE, + GET_CONSUMER_OFFSET_CODE, GET_ME_CODE, GET_PERSONAL_ACCESS_TOKENS_CODE, GET_SNAPSHOT_FILE_CODE, + GET_STATS_CODE, LOGIN_USER_CODE, LOGIN_WITH_PERSONAL_ACCESS_TOKEN_CODE, PING_CODE, + POLL_MESSAGES_CODE, SYNC_CONSUMER_GROUP_CODE, }; use iggy_binary_protocol::primitives::consumer::WireConsumer; use iggy_binary_protocol::primitives::polling_strategy::WirePollingStrategy; @@ -92,8 +93,9 @@ use iggy_binary_protocol::{ AckLevel, ClientVersionInfo, Command, ConsensusHeader, EvictionReason, ForwardLogoutHeader, ForwardLogoutOutcome, ForwardLogoutResultHeader, ForwardRegisterHeader, ForwardRegisterOutcome, ForwardRegisterResultHeader, GenericHeader, HEADER_SIZE, KIND_CONSUMER_GROUP, - MAX_PARTITIONS_PER_REQUEST, Operation, ProtocolVersion, RequestHeader, RoutedRequestHeader, - WireDecode, WireEncode, WireIdentifier, WireOptions, is_protocol_compatible, + MAX_PARTITIONS_PER_REQUEST, Operation, ProtocolVersion, ReplyHeader, RequestHeader, + RoutedRequestHeader, WireDecode, WireEncode, WireIdentifier, WireOptions, + is_protocol_compatible, }; use iggy_common::{ IggyByteSize, IggyError, MaxTopicSize, PollingStrategy, SnapshotCompression, @@ -1356,6 +1358,13 @@ async fn handle_client_request( // shard 0 can't route by the consensus client id (no home-shard bits). match submit_client_request_on_owner(shard, request).await { Some(reply) => { + // Recorded before the reply reaches the socket, so a read the client + // sends the instant it decodes this frame already sees the mark. + if let Some(commit) = committed_reply_commit(&reply) { + sessions + .borrow_mut() + .record_metadata_watermark(transport_client_id, commit); + } // The raw PAT token never enters consensus (it is non-deterministic // and secret), so the committed reply body is empty. Substitute the // raw-token response here, on the minting client's home shard, using @@ -1607,6 +1616,107 @@ pub(crate) async fn dispatch_partition_request( shard.dispatch(request.into_generic()); } +/// Poll cadence while a read waits for this node's applied metadata frontier. +/// The consensus tick, so a node one commit behind resumes on the next commit +/// broadcast rather than a tick later. +const READ_FRONTIER_POLL: Duration = Duration::from_millis(10); + +/// Polls a held read is given before it fails retryable: 3s at the cadence +/// above. Long enough to ride out a view change, far below the SDK's 30s +/// request budget, inside which it replays the same id on the same connection. +const READ_FRONTIER_MAX_POLLS: u32 = 300; + +/// Hold a local metadata read until this node has applied everything the +/// connection was told committed. +/// +/// A committed reply hands the client an op number; answering its next read +/// from a state machine below that op contradicts the frame the client is +/// holding. The lag is real on a node whose commit walk trails the client's +/// epoch -- a backup that forwarded the client's register binds a committed +/// session while its own `commit_journal` is still behind it (see +/// [`crate::auth`]) -- so the gate is not about peer shards: every shard of a +/// node reads one shared frontier and gates identically. +/// +/// Fast path is a single `Acquire` load and no await, which is what keeps an +/// uncontended read shared-nothing. Otherwise it polls the bus timer (virtual +/// under the simulator, wall clock in production) from inside the +/// per-connection drain task, so only this connection waits. Expiry fails +/// loud and retryable rather than serving state the client already saw +/// replaced; the log carries both numbers so a frontier that stopped moving +/// is visible instead of showing up as a hang. +#[allow(clippy::future_not_send)] +async fn await_metadata_read_frontier( + shard: &Rc>, + sessions: &Rc>, + transport_client_id: u128, +) -> Result<(), IggyError> +where + B: ShellBus, + MJ: JournalHandle + 'static, + MJ::Target: Journal, Header = PrepareHeader>, + S: 'static, + SB: SuperblockStore + 'static, +{ + // Own statement: the borrow has to be released before the poll below, which + // awaits on the same task the session manager's mutators run on. + let watermark = sessions.borrow().metadata_watermark(transport_client_id); + let metadata = shard.plane.metadata(); + if metadata.applied_frontier() >= watermark { + return Ok(()); + } + for _ in 0..READ_FRONTIER_MAX_POLLS { + shard.bus.sleep(READ_FRONTIER_POLL).await; + if metadata.applied_frontier() >= watermark { + return Ok(()); + } + } + warn!( + frontier = metadata.applied_frontier(), + watermark, "metadata read frontier unreached past deadline; failing the read retryable" + ); + Err(IggyError::TransientNotCommitted) +} + +/// The commit position a COMMITTED metadata reply carries, or `None` when the +/// frame promises the client nothing. +/// +/// Three frames arrive on this path and only one is a promise. An eviction is +/// an `EvictionHeader` whose bytes would cast cleanly as a reply, so the +/// command is checked first (same guard as [`build_raw_pat_reply`]). A +/// pre-consensus rejection stamps the primary's `commit_max`, an op the caller +/// was never told committed and, on a backup-homed caller, one the read gate +/// would then wait for. A committed business rejection (duplicate name, bad +/// expiry) DID commit and counts. +/// +/// Shared with the HTTP write path, which grades the same three frames off the +/// same submit entry point (`submit_client_request_on_owner`); one classifier +/// is what keeps the two planes' watermarks meaning the same thing. +pub(crate) fn committed_reply_commit(reply: &Message) -> Option { + if reply.header().command != Command::Reply || transient_code(reply).is_some() { + return None; + } + let header = reply.as_slice().get(..size_of::())?; + bytemuck::checked::try_from_bytes::(header) + .ok() + .map(|header| header.commit) +} + +/// Whether `code`'s answer comes from the metadata state machine, and so must +/// not be served below the caller's watermark. +/// +/// The two exclusions only look like metadata reads: `DescribeOptions` decodes +/// a static catalog, and `GetClusterMetadata` answers from the configured +/// roster plus the consensus view. Holding either buys no consistency, and the +/// roster read is on the SDK's leader-discovery path, where the wait would be +/// real. +/// +/// Shared with the HTTP read path, which gates the identical set of command +/// codes through `build_non_replicated_response`: two lists would drift, and a +/// code dropped from one plane's list is a silent stale read on that plane. +pub(crate) const fn read_needs_metadata_frontier(code: u32) -> bool { + !matches!(code, DESCRIBE_OPTIONS_CODE | GET_CLUSTER_METADATA_CODE) +} + #[allow(clippy::future_not_send, clippy::too_many_lines)] async fn handle_non_replicated_request( shard: &Rc>, @@ -1655,6 +1765,13 @@ async fn handle_non_replicated_request( handle_get_me(shard, sessions, transport_client_id, &request).await; } GET_PERSONAL_ACCESS_TOKENS_CODE => { + if let Err(error) = + await_metadata_read_frontier(shard, sessions, transport_client_id).await + { + send_non_replicated_deny(shard, &request, transport_client_id, error.as_code()) + .await; + return; + } handle_get_personal_access_tokens(shard, sessions, transport_client_id, &request).await; } GET_CLIENTS_CODE => { @@ -1739,6 +1856,14 @@ async fn handle_non_replicated_request( handle_sync_consumer_group(shard, transport_client_id, &request).await; } _ => { + if read_needs_metadata_frontier(code) + && let Err(error) = + await_metadata_read_frontier(shard, sessions, transport_client_id).await + { + send_non_replicated_deny(shard, &request, transport_client_id, error.as_code()) + .await; + return; + } let roster = sessions.borrow().cluster_roster(); let client_ip = client_address.map(|address| address.ip()); if client_ip.is_none() { diff --git a/core/server/src/http.rs b/core/server/src/http.rs index ed670883d8..291b39d49a 100644 --- a/core/server/src/http.rs +++ b/core/server/src/http.rs @@ -31,7 +31,7 @@ mod jwks; mod jwt; mod metrics; mod reads; -mod reply; +pub mod reply; mod session; mod state; mod submit; diff --git a/core/server/src/http/error.rs b/core/server/src/http/error.rs index db3edcc5ac..0ee842227d 100644 --- a/core/server/src/http/error.rs +++ b/core/server/src/http/error.rs @@ -471,6 +471,14 @@ pub(in crate::http) enum ReadError { /// [`service_unavailable`] body, retryable once the cluster re-commits the /// suffix. RecoveryIncomplete, + /// A metadata read waited out its budget with this node's applied frontier + /// still below the op the caller was told committed. Fail-closed on the + /// same retryable 503 as [`Self::RecoveryIncomplete`]: the two are the same + /// hazard (serving state the caller already saw replaced) reached from + /// different directions, and 503 is what the binary transports' equivalent + /// refusal (`TransientNotCommitted`) already renders as, so an SDK that + /// speaks both sees one answer. Never a 2xx with stale state. + MetadataFrontierUnreached, /// A partition read (poll / consumer-offset) got no reply from the owning /// shard within the mesh budget. 504 like a produce timeout: the outcome is /// unknown (the abandoned read may still be running), so the caller retries. @@ -486,7 +494,7 @@ impl IntoResponse for ReadError { Self::NotFound => CustomError::ResourceNotFound.into_response(), Self::NotPrimary => not_primary_response(), Self::RedirectToPrimary(location) => primary_redirect_response(&location), - Self::RecoveryIncomplete => service_unavailable(), + Self::RecoveryIncomplete | Self::MetadataFrontierUnreached => service_unavailable(), Self::Timeout => gateway_timeout_response( "partition_read_timeout", "the partition owner did not answer the read in time; retry", @@ -816,4 +824,20 @@ mod tests { not_primary.headers().get(RETRY_AFTER) ); } + + // An unreached read frontier must never degrade into a 2xx carrying stale + // state, and must not read as terminal either: it is the same retryable 503 + // the recovery barrier's expiry renders, so an SDK retries rather than + // surfacing the read as failed. + #[test] + fn metadata_frontier_unreached_renders_the_same_retryable_503_as_the_barrier() { + let frontier = ReadError::MetadataFrontierUnreached.into_response(); + let recovery = ReadError::RecoveryIncomplete.into_response(); + assert_eq!(frontier.status(), StatusCode::SERVICE_UNAVAILABLE); + assert_eq!(frontier.status(), recovery.status()); + assert_eq!( + frontier.headers().get(RETRY_AFTER), + Some(&HeaderValue::from(RETRY_AFTER_SECONDS)) + ); + } } diff --git a/core/server/src/http/extractor.rs b/core/server/src/http/extractor.rs index e818a4e69a..a2239a745e 100644 --- a/core/server/src/http/extractor.rs +++ b/core/server/src/http/extractor.rs @@ -94,6 +94,14 @@ impl FromRequestParts for Authenticated { /// [`resolve_credential`] chokepoint, so a JWT and a PAT are honored identically. pub struct Identity { pub user_id: u32, + /// Session-table key of the presenting credential (`jwt:{jti}` / + /// `pat:{sha}`), the SAME key [`Authenticated`] resolves its session under. + /// + /// A read mints no session, so this is the only join back to what this + /// credential's writes committed: the read gate looks its metadata + /// watermark up by this key. Carried rather than resolved again in the + /// handler because [`resolve_credential`] already computed it. + pub session_key: String, /// Original request path + query (e.g. `/streams?consistency=linearizable`), /// captured so a linearizable read that reaches a follower can build the /// `Location` for its 307 redirect to the primary. Empty only when the URI @@ -116,15 +124,18 @@ impl FromRequestParts for Identity { ) -> Result { let bearer = bearer_token(&parts.headers)?; - // Verify only. The session key and expiry `resolve_credential` also - // returns feed the write path's session table; a read discards them. + // Verify only: no session is minted or Registered. The key is kept + // (the read gate resolves this credential's metadata watermark under + // it) while the expiry is discarded - nothing here installs a table + // entry to expire. // The verify is `!Send` (a trusted-issuer JWT may await a JWKS fetch), // so bridge it with `SendWrapper` - sound only because compio pins this // future to shard 0's single thread, the only thread the JWKS client // ever runs on (mirrors legacy `HttpSafeShard`). It holds no `RefCell` // borrow or `DashMap` guard across the `.await`, so a sibling task // scheduled on this thread meanwhile never observes a borrowed cell. - let (_key, user_id, _expiry) = SendWrapper::new(resolve_credential(state, bearer)).await?; + let (session_key, user_id, _expiry) = + SendWrapper::new(resolve_credential(state, bearer)).await?; let path_and_query = parts .uri .path_and_query() @@ -142,6 +153,7 @@ impl FromRequestParts for Identity { } Ok(Self { user_id, + session_key, path_and_query, client_ip, }) diff --git a/core/server/src/http/reads.rs b/core/server/src/http/reads.rs index 08e743f472..2a2830cac5 100644 --- a/core/server/src/http/reads.rs +++ b/core/server/src/http/reads.rs @@ -15,11 +15,14 @@ // specific language governing permissions and limitations // under the License. -//! Read-path gates: the shared per-op RBAC + consistency check, the local -//! metadata-STM read entry, and the wire/domain identifier resolvers the read -//! and data-plane routes ground their scopes through. +//! Read-path gates: the shared per-op RBAC + consistency check, the two waits +//! a local read serves behind (the post-restart recovery barrier and the +//! per-credential read-your-writes frontier), the local metadata-STM read +//! entry, and the wire/domain identifier resolvers the read and data-plane +//! routes ground their scopes through. use crate::bootstrap::ServerShard; +use crate::dispatch::read_needs_metadata_frontier; use bytes::Bytes; use consensus::MetadataHandle; use iggy_binary_protocol::WireIdentifier; @@ -94,7 +97,15 @@ pub(in crate::http) async fn read_local( rule: impl FnOnce(&Permissioner, u32) -> Result<(), IggyError>, ) -> Result { await_recovery_barrier(&state.shard).await?; + // Ahead of the frontier wait on purpose. `authorize_read` renders the + // linearizable follower redirect, which must answer 307 immediately - + // parking first would delay a request this node is not going to serve at + // all - and an authorization denial is terminal, so holding the connection + // for it buys nothing. authorize_read(state, identity, consistency, rule)?; + if read_needs_metadata_frontier(code) { + await_metadata_read_frontier(state, identity).await?; + } let clients_count = if code == GET_STATS_CODE { u32::try_from(SendWrapper::new(state.shard.list_all_clients()).await.len()) .unwrap_or(u32::MAX) @@ -117,6 +128,87 @@ pub(in crate::http) async fn read_local( } } +/// Poll cadence while a metadata read waits for this node's applied frontier. +/// The recovery barrier's cadence and the binary read gate's: what the wait is +/// usually short of is a single commit broadcast. +const READ_FRONTIER_POLL: std::time::Duration = std::time::Duration::from_millis(10); + +/// Polls one held read is given before it fails retryable: 3s at the cadence +/// above. Matches the binary read gate's budget so both planes give up at the +/// same point, and stays far below the 30s the control-plane write path already +/// spends replaying a transient frame. +const READ_FRONTIER_MAX_POLLS: u32 = 300; + +/// Hold a local metadata read until this node has applied everything the +/// presenting credential was told committed. +/// +/// A committed control-plane reply hands the caller an op number; answering its +/// next read from a state machine below that op contradicts the response it is +/// holding. The lag is real on a node that is not the metadata primary: a +/// healthy backup FORWARDS a `Register` to the primary +/// (`dispatch::submit_register_local_or_forward`) and binds the committed epoch +/// while its own commit walk is still behind it - and a cluster without shared +/// bearer key material runs with HTTP forwarding off, so control-plane writes +/// stay on that backup instead of being relayed. +/// +/// Adjacent to `?consistency=linearizable`, not in competition with it. That +/// asks for the freshest CLUSTER state and is answered by leaving this node +/// (307 to the primary), which [`authorize_read`] decides before this wait and +/// which this wait never sees. This gate makes an UNQUALIFIED read +/// read-your-writes for its own credential, at no redirect and no consensus +/// round trip. +/// +/// Scope is this node's own view. A credential whose write this node relayed +/// over HTTP, or that wrote through a different node entirely, left no +/// watermark here; closing that needs the serving primary's commit op to reach +/// the reading node, which nothing in the response carries today. +async fn await_metadata_read_frontier( + state: &HttpInner, + identity: &Identity, +) -> Result<(), ReadError> { + let metadata = state.shard.plane.metadata(); + hold_for_frontier( + || metadata.applied_frontier(), + state.metadata_watermark(&identity.session_key), + READ_FRONTIER_MAX_POLLS, + ) + .await +} + +/// Poll `frontier` for `watermark` up to `max_polls` times, then give up +/// retryable. Split from its call site so the fast path, the park, the catch-up +/// and the expiry are all testable without a live shard - the same reason +/// [`barrier_state`] is split out below. +/// +/// A caller with nothing to read back has `watermark == 0`, which the first +/// comparison satisfies: one `Acquire` load, no await, no allocation. Expiry is +/// loud and carries both numbers, so a frontier that stopped moving is visible +/// instead of showing up as latency. +async fn hold_for_frontier( + frontier: impl Fn() -> u64, + watermark: u64, + max_polls: u32, +) -> Result<(), ReadError> { + if frontier() >= watermark { + return Ok(()); + } + for _ in 0..max_polls { + // `compio::time::sleep` like the recovery barrier below: this listener + // is pinned to shard 0's compio thread, which has no blocking pool to + // hand a wait to. Only this request parks. + compio::time::sleep(READ_FRONTIER_POLL).await; + if frontier() >= watermark { + return Ok(()); + } + } + tracing::warn!( + frontier = frontier(), + watermark, + "metadata read frontier unreached past deadline; failing read with retryable 503" + ); + Err(ReadError::MetadataFrontierUnreached) +} + /// One recovery-barrier check's outcome, factored out of [`await_recovery_barrier`] /// so the expiry decision is unit-testable without a runtime: the loop reads the /// clock and injects whether the deadline has passed. @@ -290,7 +382,117 @@ pub(in crate::http) fn authorize_data_plane( #[cfg(test)] mod tests { - use super::{BarrierWait, barrier_state}; + use super::{ + BarrierWait, READ_FRONTIER_MAX_POLLS, ReadError, barrier_state, hold_for_frontier, + read_needs_metadata_frontier, + }; + use iggy_binary_protocol::codes::{ + DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE, GET_CONSUMER_GROUPS_CODE, + GET_PERSONAL_ACCESS_TOKENS_CODE, GET_STATS_CODE, GET_STREAM_CODE, GET_STREAMS_CODE, + GET_TOPIC_CODE, GET_TOPICS_CODE, GET_USER_CODE, GET_USERS_CODE, + }; + use std::cell::Cell; + + /// A caller with nothing to read back (`watermark == 0`) and one whose + /// watermark this node has already applied are the whole steady state, and + /// neither may cost a park: exactly one load, no await. A gate that polled + /// here would put 10ms on every REST read in the cluster. + #[compio::test] + async fn given_a_frontier_at_the_watermark_when_gating_should_serve_without_parking() { + for (frontier_value, watermark) in [(0, 0), (7, 7), (9, 7)] { + let loads = Cell::new(0u32); + let outcome = hold_for_frontier( + || { + loads.set(loads.get() + 1); + frontier_value + }, + watermark, + READ_FRONTIER_MAX_POLLS, + ) + .await; + assert!( + outcome.is_ok(), + "frontier {frontier_value} covers {watermark}" + ); + assert_eq!( + loads.get(), + 1, + "frontier {frontier_value} covers {watermark}, so the read must not poll" + ); + } + } + + /// The gate's whole point: a read whose credential was told op 9 committed + /// is held, not answered, while this node is still at op 4 - and it is + /// answered as soon as the node catches up, rather than being failed. + #[compio::test] + async fn given_a_frontier_behind_the_watermark_when_gating_should_hold_until_it_catches_up() { + const CATCH_UP_AFTER: u32 = 2; + let polls = Cell::new(0u32); + let outcome = hold_for_frontier( + || { + let seen = polls.get(); + polls.set(seen + 1); + if seen >= CATCH_UP_AFTER { 9 } else { 4 } + }, + 9, + 8, + ) + .await; + + assert!( + outcome.is_ok(), + "the read must be served once the node caught up" + ); + assert!( + polls.get() > CATCH_UP_AFTER, + "the read was answered off the lagging frontier after {} loads", + polls.get() + ); + } + + /// A node can legitimately never catch up (a durably lagging replica), so + /// the park is bounded - and the exit is a retryable refusal, never the + /// stale answer. `MetadataFrontierUnreached` renders the shared 503 (see + /// `error.rs`). + #[compio::test] + async fn given_a_frontier_that_never_catches_up_when_gating_should_fail_retryable() { + let outcome = hold_for_frontier(|| 4, 9, 3).await; + assert!( + matches!(outcome, Err(ReadError::MetadataFrontierUnreached)), + "an unreached frontier must refuse the read, not serve it" + ); + } + + /// The HTTP read routes share the binary dispatch's exclusion list, so this + /// pins what that list means for the codes HTTP actually serves: every + /// entity read is gated, and the static option catalog is not. Forking the + /// list per plane is what this is here to catch. + #[test] + fn given_the_http_read_codes_when_classified_should_gate_all_but_the_static_catalog() { + for code in [ + GET_STREAMS_CODE, + GET_STREAM_CODE, + GET_TOPICS_CODE, + GET_TOPIC_CODE, + GET_USERS_CODE, + GET_USER_CODE, + GET_CONSUMER_GROUPS_CODE, + GET_PERSONAL_ACCESS_TOKENS_CODE, + GET_STATS_CODE, + ] { + assert!( + read_needs_metadata_frontier(code), + "code {code} answers from the metadata STM and must be gated" + ); + } + for code in [DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE] { + assert!( + !read_needs_metadata_frontier(code), + "code {code} answers from a static catalog or the roster; holding it buys nothing" + ); + } + } #[test] fn barrier_state_ready_when_no_barrier_armed() { diff --git a/core/server/src/http/reply.rs b/core/server/src/http/reply.rs index c17f5ce788..e0eeaa6a0f 100644 --- a/core/server/src/http/reply.rs +++ b/core/server/src/http/reply.rs @@ -156,7 +156,7 @@ pub(in crate::http) fn committed_payload( /// entered the pipeline and is safe to re-issue anywhere, while /// `TransientNotCommitted` may still commit and only a same-session same-id /// replay is safe. -pub(in crate::http) fn transient_code(reply: &Message) -> Option { +pub fn transient_code(reply: &Message) -> Option { match result_code(reply_body(reply)) { Some(code) if code == IggyError::TransientNotCommitted.as_code() => { Some(IggyError::TransientNotCommitted) diff --git a/core/server/src/http/session.rs b/core/server/src/http/session.rs index aa1db608c3..fe9e21ac28 100644 --- a/core/server/src/http/session.rs +++ b/core/server/src/http/session.rs @@ -119,9 +119,50 @@ pub(in crate::http) struct HttpSession { /// [`MAX_IN_FLIGHT_WRITES_PER_SESSION`]. Only [`InFlightWriteGuard`] /// touches it, so every admission is paired with exactly one release. pub(in crate::http) in_flight_writes: Cell, + /// Highest metadata op this credential has been told committed. + /// + /// Seeded from [`Self::session`] (the `Register` commit op, which floors + /// every metadata op that register could have observed) and raised by every + /// committed control-plane reply on this session. The read gate holds a + /// local read until the node's applied frontier covers it, so a caller + /// cannot be served state older than a write it already saw acked. + /// + /// Per-credential rather than per-request because that is the unit a + /// bearer's requests share; a caller presenting a fresh credential + /// re-seeds from the session it registers. A plain `Cell` suffices on + /// single-threaded shard 0, and it is never read across an `.await`. + pub(in crate::http) metadata_watermark: Cell, } impl HttpSession { + /// Build the session a completed `Register` establishes: the identity + /// fields come from the caller, every counter starts where the write and + /// read paths expect it to. + /// + /// The seeds are here rather than at the mint site because + /// `metadata_watermark` starting at `session` is a correctness invariant + /// (see the field), and one that no fixture can be trusted to restate. + pub(in crate::http) fn registered( + key: String, + client_id: u128, + session: u64, + user_id: u32, + expiry: u64, + ) -> Self { + Self { + key, + client_id, + session, + user_id, + expiry, + gate: Mutex::new(FIRST_REQUEST_ID), + data_request: Cell::new(FIRST_REQUEST_ID), + registry_token: Cell::new(None), + in_flight_writes: Cell::new(0), + metadata_watermark: Cell::new(session), + } + } + /// Mint the next data-plane request id. Also consumed by the `?ack=none` /// path, which installs no slot: sharing one counter keeps a shed reply's /// id from ever colliding with a live awaited slot on this session. @@ -130,6 +171,19 @@ impl HttpSession { self.data_request.set(id + 1); id } + + /// Raise this session's metadata watermark to `commit`. Monotone, so a + /// reply that lands out of order (concurrent requests on one credential + /// are legal) cannot lower it. + /// + /// Only COMMITTED metadata replies belong here. A pre-consensus rejection + /// stamps the primary's `commit_max`, an op this caller was never promised + /// and, on a backup, one its own reads would then wait for; partition-plane + /// replies carry a different group's commit position entirely. + pub(in crate::http) fn record_metadata_watermark(&self, commit: u64) { + self.metadata_watermark + .set(self.metadata_watermark.get().max(commit)); + } } /// Serializes first-use VSR registration per credential key so a herd of @@ -264,17 +318,13 @@ mod tests { /// construction plus the live cancellation smoke, not faked here. #[compio::test] async fn detached_task_advances_gate_and_ignores_dead_receiver() { - let session = Rc::new(HttpSession { - key: "jwt:test".to_owned(), - client_id: 7, - session: 1, - user_id: DEFAULT_ROOT_USER_ID, - expiry: u64::MAX, - gate: Mutex::new(FIRST_REQUEST_ID), - data_request: Cell::new(FIRST_REQUEST_ID), - registry_token: Cell::new(None), - in_flight_writes: Cell::new(0), - }); + let session = Rc::new(HttpSession::registered( + "jwt:test".to_owned(), + 7, + 1, + DEFAULT_ROOT_USER_ID, + u64::MAX, + )); let (result_slot, committed) = oneshot::channel::(); // The handler future dies (client disconnect) before the task runs. drop(committed); @@ -292,21 +342,24 @@ mod tests { assert_eq!(*session.gate.lock().await, FIRST_REQUEST_ID + 1); } + /// Register commit op every fixture binds. Non-zero on purpose: the read + /// gate's watermark seeds from it, so a fixture at zero could not tell a + /// seeded session from an unseeded one. + const FIXTURE_EPOCH: u64 = 1; + /// `InstanceToken` has no public constructor, so fixtures carry no reply /// target; the token-teardown branch of the sweep/forget helpers is - /// exercised via their `Option` path, not fabricated here. + /// exercised via their `Option` path, not fabricated here. Built through + /// [`HttpSession::registered`], the same constructor the live mint uses, so + /// a fixture can never drift from the seeds production ships. fn fake_session(key: &str, client_id: u128, expiry: u64) -> Rc { - Rc::new(HttpSession { - key: key.to_owned(), + Rc::new(HttpSession::registered( + key.to_owned(), client_id, - session: 1, - user_id: DEFAULT_ROOT_USER_ID, + FIXTURE_EPOCH, + DEFAULT_ROOT_USER_ID, expiry, - gate: Mutex::new(FIRST_REQUEST_ID), - data_request: Cell::new(FIRST_REQUEST_ID), - registry_token: Cell::new(None), - in_flight_writes: Cell::new(0), - }) + )) } // The barrier is what makes a herd of concurrent first-requests for one @@ -417,4 +470,27 @@ mod tests { "the pointer fence spares the re-registered session" ); } + + /// The mark is a floor the read gate waits for, so nothing may lower it: + /// two concurrent requests on one credential can have their committed + /// replies land out of order, and the later-but-lower reply must not undo + /// the earlier-but-higher one. The seed is the register's own commit op + /// (`session`), which floors every op that register could have observed. + #[test] + fn given_out_of_order_replies_when_recording_should_keep_the_watermark_monotone() { + let session = fake_session("jwt:a", 1, u64::MAX); + assert_eq!( + session.metadata_watermark.get(), + session.session, + "a fresh session starts at its register commit op" + ); + + session.record_metadata_watermark(50); + session.record_metadata_watermark(7); + assert_eq!( + session.metadata_watermark.get(), + 50, + "a lower commit must not lower the mark" + ); + } } diff --git a/core/server/src/http/state.rs b/core/server/src/http/state.rs index 8ea1bb4867..4e0dd2ee99 100644 --- a/core/server/src/http/state.rs +++ b/core/server/src/http/state.rs @@ -34,7 +34,6 @@ use iggy_common::{ClusterMetadata, IggyTimestamp}; use message_bus::InstanceToken; use metadata::MetadataSubmitError; use send_wrapper::SendWrapper; -use tokio::sync::Mutex; use tracing::warn; use crate::bootstrap::ServerShard; @@ -45,8 +44,8 @@ use crate::http::forward::ForwardState; use crate::http::jwt::JwtManager; use crate::http::metrics::HttpMetrics; use crate::http::session::{ - BarrierEntry, FIRST_REQUEST_ID, FRESH_ENTRY_WATERMARK, HttpSession, RegistrationBarrier, - forget_if_same, live_entry, sweep_expired, + BarrierEntry, FRESH_ENTRY_WATERMARK, HttpSession, RegistrationBarrier, forget_if_same, + live_entry, sweep_expired, }; /// Response header carrying the current VSR view number. Stamped by @@ -210,6 +209,25 @@ impl HttpInner { } } + /// Highest metadata op the credential behind `key` has been told + /// committed, or `0` when this node has told it none - an unknown key, so + /// no write of its ever ran here and there is nothing to read back. + /// + /// Deliberately NOT expiry-filtered, unlike [`Self::live_session`]: the + /// number is a consistency floor, not a capability, and the request that + /// consults it has already re-verified the bearer. Dropping the floor + /// because a swept-but-still-present entry aged out would reintroduce the + /// stale read for exactly the callers still holding a committed reply. + /// + /// Confines the shared `RefCell` borrow to this call, so it can never span + /// the read gate's `.await`. + pub(in crate::http) fn metadata_watermark(&self, key: &str) -> u64 { + self.sessions + .borrow() + .get(key) + .map_or(0, |session| session.metadata_watermark.get()) + } + /// Clone the live (non-expired) entry for `key`, if present. Confines the /// shared `RefCell` borrow to this call so it can never span an `.await`. fn live_session(&self, key: &str, now_secs: u64) -> Option> { @@ -341,17 +359,19 @@ impl HttpInner { ); return Err(AuthError::SessionIdTaken); } - Ok(Rc::new(HttpSession { + // `bound.epoch` also seeds the read gate's watermark: a HEALTHY BACKUP + // forwards the register to the primary (see + // `submit_register_local_or_forward`), so this node can hand back an + // epoch its own commit walk has not reached, and the caller's first + // read would otherwise be served from state older than the register it + // is holding. + Ok(Rc::new(HttpSession::registered( key, client_id, - session: bound.epoch, + bound.epoch, user_id, expiry, - gate: Mutex::new(FIRST_REQUEST_ID), - data_request: Cell::new(FIRST_REQUEST_ID), - registry_token: Cell::new(None), - in_flight_writes: Cell::new(0), - })) + ))) } /// Drop the session table entry for `session`, but only if it is still the diff --git a/core/server/src/http/submit.rs b/core/server/src/http/submit.rs index ca763d8368..4c3b0ffef6 100644 --- a/core/server/src/http/submit.rs +++ b/core/server/src/http/submit.rs @@ -35,8 +35,8 @@ use tracing::warn; use crate::bootstrap::ServerShard; use crate::dispatch::{ - dispatch_partition_request, resolve_delete_segments_truncate, submit_client_request_on_owner, - submit_logout_on_owner, + committed_reply_commit, dispatch_partition_request, resolve_delete_segments_truncate, + submit_client_request_on_owner, submit_logout_on_owner, }; use crate::http::admission::admit_partition_write; use crate::http::error::{PartitionWriteError, WriteError}; @@ -115,6 +115,16 @@ pub(in crate::http) async fn submit_committed( compio::runtime::spawn(async move { let result = submit_gated(&shard, &task_session, operation, max_tokens_per_user, &body).await; + // Recorded here rather than after the await below, for the same reason + // the submit is detached: a caller that disconnected mid-write still + // committed the op, and its next request on this credential must not be + // served state older than what committed. Ordered before the wake, so a + // read issued the instant the response lands already sees the mark. + if let Ok((_, reply, _)) = &result + && let Some(commit) = committed_reply_commit(reply) + { + task_session.record_metadata_watermark(commit); + } // A failed send means the handler died mid-await; the submit itself // already completed, which is the invariant that matters. let _ = result_slot.send(result); diff --git a/core/server/src/session_manager.rs b/core/server/src/session_manager.rs index 84635a8b4f..cdba5367df 100644 --- a/core/server/src/session_manager.rs +++ b/core/server/src/session_manager.rs @@ -79,6 +79,18 @@ pub struct Connection { pub last_heartbeat: Instant, /// Recorded at login; `None` until the connection authenticates. pub sdk: Option, + /// Highest metadata op this connection has been told committed. + /// + /// Seeded from the bound session (the register's own commit op, which + /// floors everything the client committed before it re-homed) and raised by + /// every committed reply relayed on this connection. The read gate holds a + /// local read until the node's applied frontier covers it, so a client + /// cannot be served state older than a write it already saw acked. + /// + /// Per-connection rather than per-client: the number only has to cover what + /// THIS socket was told, and a client that reconnects re-seeds from the + /// session it binds. + pub metadata_watermark: u64, } /// Bridges transport connections to consensus sessions. @@ -141,6 +153,7 @@ impl SessionManager { state: ConnectionState::Connected, last_heartbeat: Instant::now(), sdk: None, + metadata_watermark: 0, }); } @@ -268,15 +281,47 @@ impl SessionManager { } // Now mutate the target connection. - self.connections.get_mut(&connection_id).unwrap().state = ConnectionState::Bound { + let bound = self + .connections + .get_mut(&connection_id) + .expect("bind_session: connection validated above, single-threaded"); + bound.state = ConnectionState::Bound { user_id, client_id, session, }; + // The session IS the register's commit op, so it floors every metadata + // op this client saw committed before it re-homed here. Without the + // seed a re-homed connection reads at zero and the gate admits the + // pre-write state its own last write already replaced. + bound.metadata_watermark = bound.metadata_watermark.max(session); self.client_to_connection.insert(client_id, connection_id); Ok(()) } + /// Raise this connection's metadata watermark to `commit`. Monotone, so a + /// late or out-of-order reply cannot lower it; no-op for an unknown + /// connection. + /// + /// Only committed replies belong here. A pre-consensus rejection stamps the + /// primary's `commit_max`, which is an op this connection was never + /// promised and, on a backup-homed connection, one it would then wait for. + pub fn record_metadata_watermark(&mut self, connection_id: u128, commit: u64) { + if let Some(conn) = self.connections.get_mut(&connection_id) { + conn.metadata_watermark = conn.metadata_watermark.max(commit); + } + } + + /// The highest metadata op this connection was told committed, or `0` when + /// it was told none (an unknown or still-unbound connection, which has no + /// write to read back). + #[must_use] + pub fn metadata_watermark(&self, connection_id: u128) -> u64 { + self.connections + .get(&connection_id) + .map_or(0, |conn| conn.metadata_watermark) + } + /// Look up the consensus session for a connection. /// /// Returns `(client_id, session)` if the connection is `Bound`, `None` otherwise. @@ -603,4 +648,46 @@ mod tests { "a second disconnect has nothing left to release" ); } + + /// The bind seed is what makes a re-homed connection safe: the register's + /// commit op floors every metadata op the client committed elsewhere, so + /// the read gate cannot admit the pre-write state on a node that has not + /// caught up. A recorder that could lower the mark would undo it. + #[test] + fn given_a_bound_connection_when_replies_arrive_should_keep_the_watermark_monotone() { + let mut mgr = SessionManager::new(); + let conn = 1; + mgr.ensure_connection(conn, addr(5200), ClientTransportKind::Tcp); + assert_eq!( + mgr.metadata_watermark(conn), + 0, + "an unbound connection was promised nothing" + ); + + mgr.login(conn, 3).unwrap(); + mgr.bind_session(conn, 100, 42).unwrap(); + assert_eq!( + mgr.metadata_watermark(conn), + 42, + "the bound session is the register's commit op and floors the mark" + ); + + mgr.record_metadata_watermark(conn, 50); + mgr.record_metadata_watermark(conn, 7); + assert_eq!( + mgr.metadata_watermark(conn), + 50, + "a lower commit must not lower the mark" + ); + } + + /// An unknown connection is not an error: the disconnect callback can win + /// the race against a reply relay, and a gate reading `0` then serves the + /// read instead of parking a socket that is already gone. + #[test] + fn given_an_unknown_connection_when_recording_a_watermark_should_be_inert() { + let mut mgr = SessionManager::new(); + mgr.record_metadata_watermark(9, 5); + assert_eq!(mgr.metadata_watermark(9), 0); + } } diff --git a/core/simulator/src/client.rs b/core/simulator/src/client.rs index 1b1501aeed..191e523c5d 100644 --- a/core/simulator/src/client.rs +++ b/core/simulator/src/client.rs @@ -16,7 +16,7 @@ // under the License. use bytes::{Bytes, BytesMut}; -use iggy_binary_protocol::codes::POLL_MESSAGES_CODE; +use iggy_binary_protocol::codes::{GET_STREAM_CODE, POLL_MESSAGES_CODE}; use iggy_binary_protocol::primitives::consumer::WireConsumer; use iggy_binary_protocol::requests::consumer_groups::{ CreateConsumerGroupRequest, DeleteConsumerGroupRequest, @@ -36,7 +36,8 @@ use iggy_binary_protocol::requests::personal_access_tokens::{ }; use iggy_binary_protocol::requests::segments::DeleteSegmentsRequest; use iggy_binary_protocol::requests::streams::{ - CreateStreamRequest, DeleteStreamRequest, PurgeStreamRequest, UpdateStreamRequest, + CreateStreamRequest, DeleteStreamRequest, GetStreamRequest, PurgeStreamRequest, + UpdateStreamRequest, }; use iggy_binary_protocol::requests::topics::{ CreateTopicRequest, DeleteTopicRequest, PurgeTopicRequest, UpdateTopicRequest, @@ -648,6 +649,48 @@ impl SimClient { .expect("poll request must be valid") } + /// Build a `GET_STREAM` read for `name`. + /// + /// A `NonReplicated` metadata read, so it is answered from whichever + /// replica's state machine the request lands on rather than routed to the + /// primary: the command code sits in the header's `reserved` prefix, the + /// group is the metadata sentinel, and the request id echoes the counter + /// without advancing it (matching [`Self::poll_messages`] and the SDK). + /// Requires a bound session, since the read is auth-gated. + /// + /// # Panics + /// Panics if `name` is not a valid wire name or the request buffer is + /// invalid. + #[allow(clippy::cast_possible_truncation)] + pub fn get_stream(&self, name: &str) -> Message { + let body = GetStreamRequest { + stream_id: WireIdentifier::named(name).expect("stream name must be valid"), + } + .to_bytes(); + + let header_size = std::mem::size_of::(); + let total_size = header_size + body.len(); + let mut reserved = [0u8; 52]; + reserved[..4].copy_from_slice(&GET_STREAM_CODE.to_le_bytes()); + let header = RoutedRequestHeader { + command: iggy_binary_protocol::Command::Request, + operation: Operation::NonReplicated, + size: total_size as u32, + client: self.client_id, + session: self.session_id(), + request: self.request_counter.get(), + reserved, + group: METADATA_GROUP, + ..Default::default() + }; + + let mut buffer = Vec::with_capacity(total_size); + buffer.extend_from_slice(bytemuck::bytes_of(&header)); + buffer.extend_from_slice(&body); + Message::try_from(Owned::<4096>::copy_from_slice(&buffer)) + .expect("get stream request must be valid") + } + /// Store offset with explicit `AckLevel`. `NoAck` takes the primary's /// fast path (no replication); `Quorum` goes through VSR. /// diff --git a/core/simulator/src/lib.rs b/core/simulator/src/lib.rs index 7b722eb9bf..93a28213b9 100644 --- a/core/simulator/src/lib.rs +++ b/core/simulator/src/lib.rs @@ -57,7 +57,7 @@ use std::collections::{HashMap, HashSet}; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::rc::Rc; use std::sync::Arc; -use std::sync::atomic::AtomicBool; +use std::sync::atomic::{AtomicBool, AtomicU64}; /// Poll budget per [`DetExecutor::run_until_stalled`]. Pumps are event-driven, so /// hitting it means a task is spin-waking: a bug, panicked with the seed. @@ -395,6 +395,10 @@ impl Simulator { // reader-mode mirror from it and reads committed metadata through the // shared handle. Built in index order, so shard 0's bundle exists first. let mut metadata_bundle: Option = None; + // One applied-metadata frontier per REPLICA, shared by its shards, + // as the server bootstrap mints one per process. Volatile: a restart + // below builds a fresh cell, matching a rebooted node. + let metadata_applied_frontier = Arc::new(AtomicU64::new(0)); for shard_idx in 0..shards_per_replica { let inbox = inboxes[usize::from(shard_idx)] .take() @@ -429,6 +433,7 @@ impl Simulator { (shard_idx == 0).then(|| replica_data_dir.clone()).flatten(), // Fresh boot: `init_partition` seeds later, before any workload. &[], + Arc::clone(&metadata_applied_frontier), ); if shard_idx == 0 { metadata_bundle = Some( @@ -1066,6 +1071,7 @@ impl Simulator { /// # Panics /// If the replica is not crashed, or its shard count does not fit `u16`; mesh /// construction caps it. + #[allow(clippy::too_many_lines)] pub fn replica_restart(&mut self, replica_index: u8) { assert!( self.crashed.contains(&replica_index), @@ -1120,6 +1126,7 @@ impl Simulator { let mut stop_txs = Vec::with_capacity(usize::from(shards_per_replica)); let mut pump_tasks = Vec::with_capacity(usize::from(shards_per_replica)); let mut metadata_bundle: Option = None; + let metadata_applied_frontier = Arc::new(AtomicU64::new(0)); for shard_idx in 0..shards_per_replica { let inbox = inboxes[usize::from(shard_idx)] .take() @@ -1151,6 +1158,7 @@ impl Simulator { metadata_incarnation, (shard_idx == 0).then(|| replica_data_dir.clone()).flatten(), &seed_namespaces, + Arc::clone(&metadata_applied_frontier), ); if shard_idx == 0 { metadata_bundle = @@ -4824,3 +4832,369 @@ mod repair_frontier_tests { ); } } + +#[cfg(test)] +mod metadata_read_frontier_tests { + //! A client that committed a metadata write and then re-homed onto a + //! lagging backup must never be served the pre-write state. + //! + //! The window is not peer shards on one node: a committed reply is only + //! produced after `gated_apply` published, and every shard reads the same + //! left-right buffers. It is a node whose commit walk trails the epoch the + //! client already holds. Register forwarding is the supported way to get + //! there -- a backup verifies the credentials itself, forwards only the + //! consensus proposal, and binds the committed session while its own + //! `commit_journal` is still behind that op (see `server::auth`). + //! + //! Blocking replication INTO one backup while the quorum commits without + //! it produces the lag deterministically, and the login still completes + //! because `ForwardRegister` / `ForwardRegisterResult` are left flowing. + //! + //! One shard per replica for the end-to-end read, deliberately. The harness + //! homes each inbound client packet on a seeded-random shard while every + //! shard owns its own `SessionManager`, so on a multi-shard replica a bound + //! session cannot reliably receive its own follow-up requests -- and the lag + //! under test is the node's, not a shard's. + //! + //! The second test is the other half: peer shards are not the WINDOW, but + //! they are how a peer-homed read learns the node's position at all, and + //! sharing one frontier cell across a replica's shards is what makes that + //! work. It runs multi-shard and drives the cell directly, so it needs no + //! session and dodges the homing problem entirely. + + use super::*; + use crate::client::SimClient; + use iggy_binary_protocol::responses::streams::get_stream::GetStreamResponse; + use iggy_binary_protocol::{Command, RoutedRequestHeader, WireDecode}; + + /// Replica 0 leads the metadata plane at view 0, so this one is a backup + /// for the whole run and is the node the client re-homes onto. + const LAGGING: u8 = 1; + + /// Steps the read is given while the backup is still cut off. A server that + /// answers a metadata read from an unconverged state answers within a + /// couple of these; the gate must hold the read past all of them. + /// + /// Well under the gate's own poll budget, so expiry cannot masquerade as a + /// held read. + const STALE_WINDOW_STEPS: u32 = 50; + + /// Steps allowed for repair to reach the backup and the held read to answer + /// once replication is restored. + const CONVERGE_STEPS: u32 = 2_000; + + /// The frames that would let the backup learn the committed writes. Journal + /// repair and `StartView` adoption are cut with the same knife as live + /// replication: any one of them left open closes the lag this exercises. + const REPLICATION_FRAMES: [Command; 5] = [ + Command::Prepare, + Command::Commit, + Command::RepairPrepare, + Command::RepairDone, + Command::StartView, + ]; + + /// A stream deleted before the client re-homed must not come back on the + /// backup that has not applied the delete yet. + #[test] + fn given_backup_behind_the_client_epoch_when_reading_a_deleted_stream_should_not_serve_it() { + server_common::MemoryPool::init_pool(&server_common::MemoryPoolSettings { + enabled: false, + size: iggy_common::IggyByteSize::from(0u64), + bucket_capacity: 1, + }); + + let replica_count: u8 = 3; + let client_id: u128 = 1; + let stream_name = "read-your-writes"; + let network_opts = packet::PacketSimulatorOptions { + node_count: replica_count, + client_count: 1, + seed: 0x1A7E_0F31, + ..packet::PacketSimulatorOptions::default() + }; + let mut sim = Simulator::with_shards_shell( + usize::from(replica_count), + 1, + std::iter::once(client_id), + network_opts, + ); + + let client = SimClient::new(client_id); + sim.shell_login(&client); + + // The create lands on every replica: the backup has to HOLD the stream + // for the read below to be able to serve a stale one. + let created = commit_write(&mut sim, client_id, 0, client.create_stream(stream_name)); + step_until_applied(&mut sim, LAGGING, created); + + // Cut replication into the backup, so the delete commits on the quorum + // formed by the primary and the remaining replica and never reaches it. + // Journal repair and `StartView` adoption go with it: any one of them + // left open closes the lag this exercises. + set_replication(&mut sim, LAGGING, false); + + let deleted = commit_write(&mut sim, client_id, 0, client.delete_stream(stream_name)); + assert!( + deleted > created, + "the delete must commit above the create, else the read below cannot \ + distinguish the two states" + ); + + // Re-home the same client onto the backup. The login is forwarded, so + // the session it binds IS a committed op above the delete while the + // backup's own applied frontier is still below it. + sim.shell_login_via(&client, LAGGING); + assert!( + sim.network.delivered_any(Command::ForwardRegister), + "no ForwardRegister crossed the wire: the backup answered the login \ + itself, so the client never re-homed" + ); + let lagging_commit = metadata_commit(&sim, usize::from(LAGGING)); + assert!( + (created..deleted).contains(&lagging_commit), + "the backup applied up to op {lagging_commit}, outside the window \ + [{created}, {deleted}) this test needs: it must hold the create and \ + miss the delete" + ); + assert_eq!( + read_stream_name_on(&sim, LAGGING, stream_name), + Some(stream_name.to_string()), + "the backup no longer holds the deleted stream, so a read cannot \ + serve a stale one and the assertions below prove nothing" + ); + + let read = client.get_stream(stream_name); + let request_id = read.header().request; + sim.submit_request(client_id, LAGGING, read.into_generic()); + + // Phase 1: still cut off. Any answer here is served from state that + // predates the delete the client already saw committed. + let mut early = None; + for _ in 0..STALE_WINDOW_STEPS { + if let Some(reply) = sim + .step() + .into_iter() + .find(|reply| reply.header().request == request_id) + { + early = Some(reply); + break; + } + } + if let Some(reply) = early { + panic!( + "the backup answered a metadata read while its applied frontier \ + ({}) was below the client's committed epoch ({deleted}): status={}, \ + stream={:?}", + metadata_commit(&sim, usize::from(LAGGING)), + reply.header().status, + read_stream_name(&reply), + ); + } + + // Phase 2: restore replication. The held read must answer from the + // converged state, which no longer holds the stream. + set_replication(&mut sim, LAGGING, true); + for _ in 0..CONVERGE_STEPS { + if let Some(reply) = sim + .step() + .into_iter() + .find(|reply| reply.header().request == request_id) + { + assert_eq!( + reply.header().status, + 0, + "the read failed instead of answering once the backup converged" + ); + assert_eq!( + read_stream_name(&reply), + None, + "the converged backup still serves the deleted stream" + ); + return; + } + } + panic!( + "no answer to the held metadata read within {CONVERGE_STEPS} steps of \ + restored replication; backup applied frontier {}, client epoch {deleted}", + metadata_commit(&sim, usize::from(LAGGING)), + ); + } + + /// Shards per replica for the sharing test below. Two is the whole + /// population that matters: shard 0 and one peer. + const SHARED_FRONTIER_SHARDS: u16 = 2; + + /// Advance applied to shard 0, chosen above whatever recovery seeded so a + /// peer reading the pre-advance value cannot pass by accident. + const SHARED_FRONTIER_ADVANCE: u64 = 7; + + /// A peer shard owns no metadata consensus, so `commit_min` -- the number + /// the pre-existing read barrier gates on -- does not exist there at all. + /// The applied frontier is the only thing its read gate can consult, and it + /// arrives by being ONE cell per process rather than one per shard. + /// + /// Nothing else in the system observes that. A private cell per shard still + /// compiles, still serves every read, and its only symptom is that each + /// metadata read homed on a peer shard parks for the whole deadline and + /// then fails retryable -- a latency cliff behind a warning, not an error. + /// So the invariant is asserted directly rather than through a request: a + /// peer must see an advance it did not make. + #[test] + fn given_a_peer_shard_when_shard_zero_advances_the_frontier_should_observe_it() { + server_common::MemoryPool::init_pool(&server_common::MemoryPoolSettings { + enabled: false, + size: iggy_common::IggyByteSize::from(0u64), + bucket_capacity: 1, + }); + + let replica_count: u8 = 3; + let network_opts = packet::PacketSimulatorOptions { + node_count: replica_count, + client_count: 1, + seed: 0x5EED_5A11, + ..packet::PacketSimulatorOptions::default() + }; + let sim = Simulator::with_shards( + usize::from(replica_count), + SHARED_FRONTIER_SHARDS, + std::iter::once(1u128), + network_opts, + ); + + let shards = &sim.replicas[0].shards; + assert_eq!( + shards.len(), + usize::from(SHARED_FRONTIER_SHARDS), + "the replica did not build the peer shard this test needs" + ); + for (shard_idx, shard) in shards.iter().enumerate().skip(1) { + assert!( + shard.plane.metadata().consensus.is_none(), + "shard {shard_idx} owns consensus, so it is not the peer whose \ + only source for the frontier is shard 0's cell" + ); + } + + let advanced = shards[0].plane.metadata().applied_frontier() + SHARED_FRONTIER_ADVANCE; + shards[0] + .plane + .metadata() + .advance_applied_frontier(advanced); + + for (shard_idx, shard) in shards.iter().enumerate() { + assert_eq!( + shard.plane.metadata().applied_frontier(), + advanced, + "shard {shard_idx} did not observe shard 0's advance: the \ + applied-frontier cell is per shard, not per process, so every \ + metadata read homed here parks until its deadline" + ); + } + } + + /// Open or close every replication route from the primary into `replica`. + fn set_replication(sim: &mut Simulator, replica: u8, open: bool) { + for frame in REPLICATION_FRAMES { + let filter = sim + .network + .link_filter_mut(ProcessId::Replica(0), ProcessId::Replica(replica)); + if open { + filter.insert(frame); + } else { + filter.remove(frame); + } + } + } + + /// Step until `replica` has applied `op`, so the state the read sees is the + /// one this test set up rather than whatever the last reply happened to + /// leave behind. + fn step_until_applied(sim: &mut Simulator, replica: u8, op: u64) { + for _ in 0..SETUP_TOTAL_STEPS { + if metadata_commit(sim, usize::from(replica)) >= op { + return; + } + sim.step(); + } + panic!( + "replica {replica} never applied op {op} (stuck at {})", + metadata_commit(sim, usize::from(replica)), + ); + } + + /// Read `name` straight out of a replica's committed metadata, bypassing + /// the read path under test. + fn read_stream_name_on(sim: &Simulator, replica: u8, name: &str) -> Option { + sim.replicas[usize::from(replica)].shards[0] + .plane + .metadata() + .mux_stm + .streams() + .read(|inner| { + inner + .items + .iter() + .find(|(_, stream)| &*stream.name == name) + .map(|(_, stream)| stream.name.to_string()) + }) + } + + /// Submit one replicated metadata write to `target`, step until its reply + /// lands, and return the op it committed at. + fn commit_write( + sim: &mut Simulator, + client_id: u128, + target: u8, + request: Message, + ) -> u64 { + let request_id = request.header().request; + sim.submit_request(client_id, target, request.into_generic()); + for _ in 0..SETUP_TOTAL_STEPS { + if let Some(reply) = sim + .step() + .into_iter() + .find(|reply| reply.header().request == request_id) + { + assert_eq!( + reply.header().status, + 0, + "metadata write {request_id} was refused" + ); + assert!( + !setup_reply_is_transient(&reply), + "metadata write {request_id} was rejected in transit, so it never \ + committed and cannot anchor the read below" + ); + return reply.header().commit; + } + } + panic!("metadata write {request_id} never committed"); + } + + /// The stream name a `GetStream` reply carries, or `None` for the + /// empty-body not-found answer. + fn read_stream_name(reply: &Message) -> Option { + let body = reply + .as_slice() + .get(size_of::()..reply.header().size as usize) + .unwrap_or_default(); + if body.is_empty() { + return None; + } + let (response, _) = GetStreamResponse::decode(body) + .expect("a non-empty GetStream reply must decode as GetStreamResponse"); + Some(response.stream.name.as_str().to_string()) + } + + /// Committed metadata op on a replica's shard 0. + fn metadata_commit(sim: &Simulator, replica_idx: usize) -> u64 { + sim.replicas[replica_idx].shards[0] + .plane + .metadata() + .consensus + .as_ref() + .expect("shard 0 owns metadata consensus") + .commit_min() + } +} diff --git a/core/simulator/src/replica.rs b/core/simulator/src/replica.rs index 638a61dce6..9aa65c3639 100644 --- a/core/simulator/src/replica.rs +++ b/core/simulator/src/replica.rs @@ -39,6 +39,7 @@ use shard::shards_table::PapayaShardsTable; use std::cell::RefCell; use std::rc::Rc; use std::sync::Arc; +use std::sync::atomic::AtomicU64; // TODO: Make configurable const CLUSTER_ID: u128 = 1; @@ -156,6 +157,7 @@ pub fn new_shard( incarnation: u128, data_dir: Option, seed_namespaces: &[(server_common::sharding::IggyNamespace, u32)], + applied_frontier: Arc, ) -> (Rc, Option) { // Metadata is single-writer, mirroring the server bootstrap. Shard 0 owns // the only writable STM; every peer shard rebuilds a reader-mode mirror from @@ -303,7 +305,8 @@ pub fn new_shard( superblock, mux, data_dir, - ); + ) + .with_applied_frontier(applied_frontier); // Both halves are load-bearing: the pairing keeps a later view-change superblock // write from regressing to `(0, 0)`, and the folded table is the floor the replayed @@ -367,6 +370,12 @@ pub fn new_shard( ); } } + // Same seed the server bootstrap does after its own replay: the frontier + // resumes where the commit walk will, so a read on a rebuilt replica does + // not park until its deadline. No-op on peer shards, which share the cell. + if let Some(consensus) = metadata.consensus.as_ref() { + metadata.advance_applied_frontier(consensus.commit_min()); + } // Mint the peers' read-side bundle AFTER reconstruction so it reflects the // recovered state. Shard 0 only; peers pass it back in as `reader_bundle`. let metadata_bundle = (shard_idx == 0).then(|| metadata.mux_stm.factory_bundle()); From 3d95831b075de665583568df53dd6ae13ba542f5 Mon Sep 17 00:00:00 2001 From: Grzegorz Koszyk Date: Thu, 3 Sep 2026 17:44:43 +0200 Subject: [PATCH 2/3] address review comments --- .../apache/iggy/bdd/BasicMessagingSteps.java | 4 +- core/integration/tests/server/http_client.rs | 75 +++- .../tests/server/http_read_your_writes.rs | 137 +++++++ .../tests/server/http_view_header.rs | 71 +--- core/integration/tests/server/mod.rs | 3 + core/metadata/src/applied_frontier.rs | 300 +++++++++++++++ core/metadata/src/impls/metadata.rs | 71 ++-- core/metadata/src/lib.rs | 5 + core/server/src/boot/mod.rs | 23 +- core/server/src/boot/threads.rs | 5 +- core/server/src/dispatch/reads.rs | 348 +++++++++++++++--- core/server/src/dispatch/submit.rs | 151 +++++++- core/server/src/http.rs | 3 +- core/server/src/http/extractor.rs | 20 +- core/server/src/http/handlers.rs | 43 ++- core/server/src/http/reads.rs | 255 ++++++------- core/server/src/http/reply.rs | 30 +- core/server/src/http/session.rs | 72 +--- core/server/src/http/state.rs | 122 ++++-- core/server/src/http/submit.rs | 18 +- core/server/src/lib.rs | 5 + core/server/src/responses.rs | 50 ++- core/server/src/session_manager.rs | 16 +- core/shard/src/lib.rs | 6 +- core/simulator/src/client.rs | 99 +++-- core/simulator/src/lib.rs | 23 +- core/simulator/src/replica.rs | 13 +- 27 files changed, 1387 insertions(+), 581 deletions(-) create mode 100644 core/integration/tests/server/http_read_your_writes.rs create mode 100644 core/metadata/src/applied_frontier.rs diff --git a/bdd/java/src/test/java/org/apache/iggy/bdd/BasicMessagingSteps.java b/bdd/java/src/test/java/org/apache/iggy/bdd/BasicMessagingSteps.java index c5439f201c..f5b9a8c19f 100644 --- a/bdd/java/src/test/java/org/apache/iggy/bdd/BasicMessagingSteps.java +++ b/bdd/java/src/test/java/org/apache/iggy/bdd/BasicMessagingSteps.java @@ -145,7 +145,9 @@ public void getStreamReturnsNoStream() { // The assertion is "not the stream we deleted", not "nothing at this id": // the server hands out the lowest free stream id, so once these scenarios // run concurrently against one server a fresh create can legitimately - // occupy the deleted stream's id. + // occupy the deleted stream's id. Named, so a missing pre-value fails + // here instead of making the comparison below vacuously true. + assertNotNull(context.lastStreamName, "Stream should have been created"); Optional stream = getClient().streams().getStream(context.lastStreamId); assertTrue( stream.isEmpty() || !stream.get().name().equals(context.lastStreamName), diff --git a/core/integration/tests/server/http_client.rs b/core/integration/tests/server/http_client.rs index 2ba24ac32b..8e51eb9425 100644 --- a/core/integration/tests/server/http_client.rs +++ b/core/integration/tests/server/http_client.rs @@ -16,16 +16,20 @@ // under the License. //! Shared HTTP transport plumbing for the server REST suites (`http_vsr`, -//! `http_rbac`): one authenticated `reqwest` session with the login-retry gate -//! and the generic verb helpers. Each suite keeps its own request shapes and -//! assertions as extension methods on [`HttpClient`], so the wire-contract and -//! listener-behavior separation between the suites stays intact. +//! `http_rbac`): one authenticated `reqwest` session with the login-retry gate, +//! the generic verb helpers, and the cluster-shaped helpers the multi-node +//! suites share (which node is the leader, which is a follower, and the retry +//! a follower needs before it can resolve the primary). Each suite keeps its +//! own request shapes and assertions as extension methods on [`HttpClient`], so +//! the wire-contract and listener-behavior separation between the suites stays +//! intact. +use std::future::Future; use std::time::{Duration, Instant}; use iggy::prelude::*; use integration::harness::TestHarness; -use reqwest::Response; +use reqwest::{Response, StatusCode}; use serde_json::{Value, json}; use tokio::time::sleep; @@ -196,6 +200,67 @@ impl HttpClient { } } +/// `http://host:port` of a harness node's HTTP listener. +pub fn node_url(harness: &TestHarness, node: usize) -> String { + let addr = harness.node(node).http_addr().expect("node http address"); + format!("http://{addr}") +} + +/// Harness indexes of the node the roster marks `Leader` and of one it marks +/// `Follower`. The harness emits the roster in node order, so a roster +/// position is a harness index. Every node reads `Follower` until shard 0 +/// publishes its first view, so the roster is polled within the shared +/// warmup budget until it marks a leader. +pub async fn leader_and_follower(harness: &TestHarness) -> (usize, usize) { + let client = harness + .root_client_for_node(0) + .await + .expect("connect to node 0"); + let deadline = Instant::now() + LOGIN_TIMEOUT; + loop { + let metadata = client + .get_cluster_metadata() + .await + .expect("get cluster metadata"); + let position = + |role: ClusterNodeRole| metadata.nodes.iter().position(|node| node.role == role); + if let (Some(leader), Some(follower)) = ( + position(ClusterNodeRole::Leader), + position(ClusterNodeRole::Follower), + ) { + return (leader, follower); + } + assert!( + Instant::now() < deadline, + "the roster did not mark a leader within {LOGIN_TIMEOUT:?}, got {metadata}" + ); + sleep(LOGIN_RETRY_INTERVAL).await; + } +} + +/// Repeat `request` while the follower answers 503, which it does until it +/// can resolve the primary from its own view; bounded by the shared warmup +/// budget, as cluster_metadata_vsr does. A 503 is the retry-safe class: the +/// request provably never entered a pipeline. +pub async fn until_primary_resolved(request: F) -> Response +where + F: Fn() -> Fut, + Fut: Future, +{ + let deadline = Instant::now() + LOGIN_TIMEOUT; + loop { + let response = request().await; + if response.status() != StatusCode::SERVICE_UNAVAILABLE { + return response; + } + assert!( + Instant::now() < deadline, + "follower did not resolve the primary within {LOGIN_TIMEOUT:?}" + ); + sleep(LOGIN_RETRY_INTERVAL).await; + } +} + /// Extract the JWT from a successful login response. pub async fn access_token(response: Response) -> String { let identity: IdentityInfo = response.json().await.expect("decode IdentityInfo"); diff --git a/core/integration/tests/server/http_read_your_writes.rs b/core/integration/tests/server/http_read_your_writes.rs new file mode 100644 index 0000000000..e9cfa68153 --- /dev/null +++ b/core/integration/tests/server/http_read_your_writes.rs @@ -0,0 +1,137 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Read-your-writes over the REST listener, end to end: an unqualified read +//! must never answer below the metadata op the same caller was already told +//! committed. +//! +//! The window is a node that HANDED OUT a committed op it has not applied yet. +//! On the REST plane that node is a follower running a `Register`: the session +//! its first authenticated request mints is forwarded to the primary +//! (`dispatch::submit_register_local_or_forward`), so the follower answers with +//! an epoch its own commit walk can still be behind. Everything the metadata +//! group committed below that epoch is therefore state the caller has been +//! promised and this follower may not have applied. +//! +//! Each round seeds a stream through the primary over TCP, then authenticates +//! on the follower, which binds an epoch above that create. The follower's next +//! read is the assertion: it must not answer from before the stream existed. +//! Logout is the authenticated request that binds it, deliberately - it tears +//! the session entry down again, so the floor the read waits on has to outlive +//! the session that established it. +//! +//! The seeding runs over TCP rather than the primary's own REST listener so the +//! only HTTP sessions in play are the follower's: a second long-lived REST +//! session would be competing for VSR client ids with the fresh register each +//! round mints, which is a different subject. +//! +//! The suite asserts the GUARANTEE, not the mechanism: whether the gate parked +//! is invisible from outside, and on a fast local cluster the follower often +//! applies within the same tick. A pre-write answer is unambiguous though - a +//! 404, or a list missing the stream, can only happen if the floor was never +//! recorded, was recorded under the wrong key, was dropped with the session, or +//! the wait was skipped. A 503 fails the assertions too, on purpose: that is +//! what the gate answers when the follower never catches up inside its budget. +//! The park, the wake and the expiry themselves are pinned deterministically +//! next to the gate, in `dispatch::reads` and `metadata::applied_frontier`. + +use iggy::prelude::*; +use integration::iggy_harness; +use reqwest::StatusCode; +use serde_json::Value; + +use crate::server::http_client::{ + HttpClient, leader_and_follower, node_url, until_primary_resolved, +}; + +/// Seed / read-back rounds. More than one because the lag is a race the test +/// cannot force: each round re-runs it with the follower's commit walk in a +/// different position relative to the epoch it just handed out. +const ROUNDS: u32 = 4; + +/// The `name` of every stream in a `GET /streams` list body. +fn stream_names(body: &Value) -> Vec { + body.as_array() + .expect("the stream list is a JSON array") + .iter() + .map(|stream| { + stream["name"] + .as_str() + .expect("every stream carries a name") + .to_owned() + }) + .collect() +} + +/// Three nodes, the smallest cluster with a quorum, one shard each so every +/// request is served by shard 0 where the metadata consensus lives. No +/// `http.jwt` secret and no `cluster.auth`: bearers are node-local and +/// follower-to-primary forwarding is off, so the follower answers its own +/// requests instead of relaying them (see `http_view_header`, which pins both +/// halves of that switch). +#[iggy_harness(cluster_nodes = 3, server(system.sharding.cpu_allocation = "0..1"))] +async fn given_a_follower_when_its_register_binds_a_committed_epoch_should_not_read_below_it( + harness: &TestHarness, +) { + let (leader, follower) = leader_and_follower(harness).await; + let seeder = harness + .root_client_for_node(leader) + .await + .expect("connect to the primary"); + + for round in 0..ROUNDS { + let stream = format!("read-your-writes-{round}"); + seeder + .create_stream(&stream) + .await + .expect("the primary must commit the stream this round reads back"); + + // Fresh bearer, then one authenticated request on the follower: it is + // what forwards the `Register` and binds an epoch above the create. + let http = HttpClient::login_root_no_redirect(node_url(harness, follower)).await; + let logout = until_primary_resolved(|| http.delete("/users/logout")).await; + assert_eq!( + logout.status(), + StatusCode::NO_CONTENT, + "the follower must bind and end a forwarded session" + ); + + // The list read resolves nothing, so it cannot 404 its way into looking + // correct: a stale answer here is a short list. + let read = http.get("/streams").await; + assert_eq!(read.status(), StatusCode::OK, "the stream list must serve"); + let names = stream_names(&read.json().await.expect("the stream list is JSON")); + assert!( + names.contains(&stream), + "the follower listed streams from before the epoch it had just handed out: {names:?}" + ); + + // The entity read, where the stale answer is a 404 instead. + let read = http.get(&format!("/streams/{stream}")).await; + assert_eq!( + read.status(), + StatusCode::OK, + "the follower answered a read below the epoch it had just handed out" + ); + let body: Value = read.json().await.expect("stream details are JSON"); + assert_eq!( + body["name"].as_str(), + Some(stream.as_str()), + "the read answered with another stream's state" + ); + } +} diff --git a/core/integration/tests/server/http_view_header.rs b/core/integration/tests/server/http_view_header.rs index 8ed175a9b2..ca21921cdd 100644 --- a/core/integration/tests/server/http_view_header.rs +++ b/core/integration/tests/server/http_view_header.rs @@ -20,17 +20,13 @@ //! withheld wherever it could reach a caller that proved no credential. Raw //! `reqwest`, because the header itself is the contract. -use std::future::Future; -use std::time::Instant; - -use iggy::prelude::*; -use integration::harness::TestHarness; use integration::iggy_harness; use reqwest::{Response, StatusCode}; use serde_json::json; -use tokio::time::sleep; -use crate::server::http_client::{HttpClient, LOGIN_RETRY_INTERVAL, LOGIN_TIMEOUT}; +use crate::server::http_client::{ + HttpClient, leader_and_follower, node_url, until_primary_resolved, +}; const VIEW_HEADER: &str = "iggy-view"; @@ -91,67 +87,6 @@ async fn given_the_ping_route_when_it_succeeds_should_omit_the_iggy_view_header( ); } -/// `http://host:port` of a harness node's HTTP listener. -fn node_url(harness: &TestHarness, node: usize) -> String { - let addr = harness.node(node).http_addr().expect("node http address"); - format!("http://{addr}") -} - -/// Harness indexes of the node the roster marks `Leader` and of one it marks -/// `Follower`. The harness emits the roster in node order, so a roster -/// position is a harness index. Every node reads `Follower` until shard 0 -/// publishes its first view, so the roster is polled within the shared -/// warmup budget until it marks a leader. -async fn leader_and_follower(harness: &TestHarness) -> (usize, usize) { - let client = harness - .root_client_for_node(0) - .await - .expect("connect to node 0"); - let deadline = Instant::now() + LOGIN_TIMEOUT; - loop { - let metadata = client - .get_cluster_metadata() - .await - .expect("get cluster metadata"); - let position = - |role: ClusterNodeRole| metadata.nodes.iter().position(|node| node.role == role); - if let (Some(leader), Some(follower)) = ( - position(ClusterNodeRole::Leader), - position(ClusterNodeRole::Follower), - ) { - return (leader, follower); - } - assert!( - Instant::now() < deadline, - "the roster did not mark a leader within {LOGIN_TIMEOUT:?}, got {metadata}" - ); - sleep(LOGIN_RETRY_INTERVAL).await; - } -} - -/// Repeat `request` while the follower answers 503, which it does until it -/// can resolve the primary from its own view; bounded by the shared warmup -/// budget, as cluster_metadata_vsr does. A 503 is the retry-safe class: the -/// request provably never entered a pipeline. -async fn until_primary_resolved(request: F) -> Response -where - F: Fn() -> Fut, - Fut: Future, -{ - let deadline = Instant::now() + LOGIN_TIMEOUT; - loop { - let response = request().await; - if response.status() != StatusCode::SERVICE_UNAVAILABLE { - return response; - } - assert!( - Instant::now() < deadline, - "follower did not resolve the primary within {LOGIN_TIMEOUT:?}" - ); - sleep(LOGIN_RETRY_INTERVAL).await; - } -} - /// The view the primary stamps on its own successful response: the value a /// follower's redirect or relay must agree with. async fn primary_view(primary: &HttpClient) -> u64 { diff --git a/core/integration/tests/server/mod.rs b/core/integration/tests/server/mod.rs index a1da26b1e3..1c6411a8aa 100644 --- a/core/integration/tests/server/mod.rs +++ b/core/integration/tests/server/mod.rs @@ -51,6 +51,9 @@ mod http_tls; // The iggy-view response header: on authenticated success and redirect // responses only, never on errors or /ping, relayed from the primary. mod http_view_header; +// An unqualified REST read must not answer below what the same caller was told +// committed, on the node that accepted the write and has not applied it yet. +mod http_read_your_writes; // Binary GetClusterMetadata must serve the real roster from a VSR cluster. mod cluster_metadata_vsr; // A declared node.advertised_address outranks the bind address a diff --git a/core/metadata/src/applied_frontier.rs b/core/metadata/src/applied_frontier.rs new file mode 100644 index 0000000000..ca5c1fa831 --- /dev/null +++ b/core/metadata/src/applied_frontier.rs @@ -0,0 +1,300 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! The node-wide applied metadata frontier and the wait a read parks on. + +use std::future::Future; +use std::pin::Pin; +use std::sync::Mutex; +use std::sync::PoisonError; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::task::{Context, Poll, Waker}; + +/// Highest metadata op whose apply has been PUBLISHED on this node, shared by +/// every shard, plus the wakers of the reads waiting for it to reach them. +/// +/// `consensus.commit_min()` answers the same question but exists only on shard +/// 0, so a read served by a peer shard has no way to tell whether the node +/// caught up to an op its client already saw committed. One process-wide cell +/// does, for one `Acquire` load on the read fast path. +/// +/// The op is `Release`-written right after each apply's `publish()` and +/// `Acquire`-read; observing `>= op` therefore happens-after that publish, so a +/// following left-right `enter()` is guaranteed to see the op. `fetch_max` +/// rather than `store` because three writers move it -- the commit loop, the +/// recovery seed, and a state-transfer install -- and only monotonicity makes +/// their order irrelevant. +/// +/// A `std::sync::Mutex` guards the waiter list, not a `tokio` one: it is taken +/// and dropped inside [`Self::advance`] and inside one `poll`, never across an +/// `.await`, and it has to be `Sync` because the writer is shard 0's thread +/// while the sleepers are on every shard. +#[derive(Debug, Default)] +pub struct AppliedFrontier { + op: AtomicU64, + waiters: Mutex, +} + +/// Registered waits, keyed by an id so a re-poll can refresh its own waker and +/// a dropped wait (an HTTP client that disconnected mid-read) can remove it. +#[derive(Debug, Default)] +struct Waiters { + next_id: u64, + entries: Vec, +} + +#[derive(Debug)] +struct Waiter { + id: u64, + target: u64, + waker: Waker, +} + +impl AppliedFrontier { + /// Highest metadata op this NODE has applied and published. + #[must_use] + pub fn get(&self) -> u64 { + self.op.load(Ordering::Acquire) + } + + /// Publish `op` as applied and wake every read waiting at or below it. + /// Monotone, so a lower value is a no-op and wakes nobody. + /// + /// Must run AFTER the apply's `publish()` and, on the commit path, in the + /// same await-free region as `advance_commit_min`: a reader that sees the + /// frontier must be guaranteed to see the op's effects. + pub fn advance(&self, op: u64) { + if self.op.fetch_max(op, Ordering::Release) >= op { + return; + } + let mut waiters = self.waiters.lock().unwrap_or_else(PoisonError::into_inner); + waiters.entries.retain(|waiter| { + if waiter.target > op { + return true; + } + waiter.waker.wake_by_ref(); + false + }); + } + + /// A future that completes once the frontier covers `target`. + /// + /// Event-driven, not polled: the commit path wakes it, so a read resumes on + /// the commit it was waiting for rather than on the next tick. It has no + /// deadline of its own -- the caller composes one, because the two read + /// planes measure time differently (the shard bus timer, virtual under the + /// simulator, against `compio::time`). + pub const fn reached(&self, target: u64) -> Reached<'_> { + Reached { + frontier: self, + target, + id: None, + } + } + + /// Register a wait for `target` under `existing` (its id from an earlier + /// poll, if any), or report that the frontier already covers it. + /// + /// One lock acquisition, released with the return: the re-read under it is + /// what closes the race with [`Self::advance`], which bumps the op and only + /// then takes this lock, so an advance landing between a caller's load and + /// this call is one the wait would otherwise sleep through. + fn register(&self, existing: Option, target: u64, waker: &Waker) -> Registered { + let mut waiters = self.waiters.lock().unwrap_or_else(PoisonError::into_inner); + if self.get() >= target { + return Registered::Ready; + } + // Refresh rather than stack: a re-poll may arrive under a different + // task (a `select` re-driven elsewhere), and two entries for one wait + // would leak the first. + let id = if let Some(waiter) = + existing.and_then(|id| waiters.entries.iter_mut().find(|waiter| waiter.id == id)) + { + waiter.waker.clone_from(waker); + waiter.id + } else { + let id = waiters.next_id; + waiters.next_id += 1; + waiters.entries.push(Waiter { + id, + target, + waker: waker.clone(), + }); + id + }; + drop(waiters); + Registered::Waiting(id) + } + + /// Drop the registration `id`, if it is still listed. + fn deregister(&self, id: u64) { + self.waiters + .lock() + .unwrap_or_else(PoisonError::into_inner) + .entries + .retain(|waiter| waiter.id != id); + } + + /// Waits currently parked. For tests: a wait that outlives its future is a + /// leaked waker. + #[must_use] + pub fn waiting(&self) -> usize { + self.waiters + .lock() + .unwrap_or_else(PoisonError::into_inner) + .entries + .len() + } +} + +/// Outcome of registering a wait: nothing to wait for, or the id the wait is +/// listed under. +#[derive(Debug, Clone, Copy)] +enum Registered { + Ready, + Waiting(u64), +} + +/// The wait [`AppliedFrontier::reached`] hands out. Deregisters on drop, so a +/// cancelled read (a dropped axum handler future, a closed socket) leaves no +/// waker behind. +#[derive(Debug)] +pub struct Reached<'a> { + frontier: &'a AppliedFrontier, + target: u64, + id: Option, +} + +impl Future for Reached<'_> { + type Output = (); + + fn poll(self: Pin<&mut Self>, context: &mut Context<'_>) -> Poll<()> { + let this = self.get_mut(); + // Ahead of the registration, so the steady state (a frontier already + // past the target) costs one load and never touches the lock. + if this.frontier.get() >= this.target { + return Poll::Ready(()); + } + match this + .frontier + .register(this.id, this.target, context.waker()) + { + Registered::Ready => Poll::Ready(()), + Registered::Waiting(id) => { + this.id = Some(id); + Poll::Pending + } + } + } +} + +impl Drop for Reached<'_> { + fn drop(&mut self) { + if let Some(id) = self.id { + self.frontier.deregister(id); + } + } +} + +#[cfg(test)] +mod tests { + use super::AppliedFrontier; + use std::future::Future; + use std::pin::pin; + use std::sync::Arc; + use std::task::{Context, Poll}; + + /// A frontier already at or above the target is the steady state, and it + /// must cost neither a wake nor a registration: a gate that parked here + /// would put a commit's latency on every metadata read in the cluster. + #[test] + fn given_a_frontier_at_the_target_when_waiting_should_be_ready_without_registering() { + let frontier = AppliedFrontier::default(); + frontier.advance(7); + + let waker = futures::task::noop_waker(); + let mut context = Context::from_waker(&waker); + for target in [0, 7] { + let mut wait = pin!(frontier.reached(target)); + assert_eq!(wait.as_mut().poll(&mut context), Poll::Ready(())); + } + assert_eq!(frontier.waiting(), 0, "a ready wait registers nothing"); + } + + /// The wait is what replaces the poll loop, so the advance has to be what + /// wakes it: park below the target, advance past it, and the wait must be + /// woken and complete without any intervening timer. + #[test] + fn given_a_parked_wait_when_the_frontier_advances_should_wake_and_complete() { + let frontier = AppliedFrontier::default(); + let woken = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let waker = futures::task::waker(Arc::new(FlagWaker { + woken: Arc::clone(&woken), + })); + let mut context = Context::from_waker(&waker); + + let mut wait = pin!(frontier.reached(9)); + assert_eq!(wait.as_mut().poll(&mut context), Poll::Pending); + assert_eq!(frontier.waiting(), 1); + + // Below the target: no wake, still parked. + frontier.advance(8); + assert!(!woken.load(std::sync::atomic::Ordering::Acquire)); + assert_eq!(frontier.waiting(), 1); + + frontier.advance(9); + assert!( + woken.load(std::sync::atomic::Ordering::Acquire), + "the advance past the target must wake the parked read" + ); + assert_eq!( + frontier.waiting(), + 0, + "a woken wait is off the list, so a later advance re-wakes nothing" + ); + assert_eq!(wait.as_mut().poll(&mut context), Poll::Ready(())); + } + + /// A re-poll must not stack a second registration, and a dropped wait must + /// take its waker with it: an HTTP read is cancelled whenever its client + /// disconnects mid-wait, and a leaked waker would be a leak per disconnect. + #[test] + fn given_a_repolled_wait_when_dropped_should_leave_no_registration() { + let frontier = AppliedFrontier::default(); + let waker = futures::task::noop_waker(); + let mut context = Context::from_waker(&waker); + { + let mut wait = pin!(frontier.reached(9)); + assert_eq!(wait.as_mut().poll(&mut context), Poll::Pending); + assert_eq!(wait.as_mut().poll(&mut context), Poll::Pending); + assert_eq!(frontier.waiting(), 1, "a re-poll refreshes, never stacks"); + } + assert_eq!(frontier.waiting(), 0, "a dropped wait deregisters"); + } + + struct FlagWaker { + woken: Arc, + } + + impl futures::task::ArcWake for FlagWaker { + fn wake_by_ref(arc_self: &Arc) { + arc_self + .woken + .store(true, std::sync::atomic::Ordering::Release); + } + } +} diff --git a/core/metadata/src/impls/metadata.rs b/core/metadata/src/impls/metadata.rs index d29a11365e..dc2399ecf2 100644 --- a/core/metadata/src/impls/metadata.rs +++ b/core/metadata/src/impls/metadata.rs @@ -16,6 +16,7 @@ // under the License. use crate::MuxStateMachine; +use crate::applied_frontier::AppliedFrontier; use crate::stm::authz::gated_apply; use crate::stm::consumer_group::CompleteConsumerGroupRevocationRequest; use crate::stm::snapshot::{ @@ -67,7 +68,6 @@ use std::mem::size_of; use std::path::Path; use std::rc::Rc; use std::sync::Arc; -use std::sync::atomic::{AtomicU64, Ordering}; use tracing::{debug, error, info, warn}; fn freeze_client_reply( @@ -771,21 +771,28 @@ pub struct IggyMetadata { /// whole snapshot on shard 0's pump, and hands each requester its own /// multi-MB copy. transfer_offer_cache: RefCell>>, - /// Highest metadata op whose apply has been PUBLISHED on this node, shared - /// by every shard. - /// - /// `consensus.commit_min()` answers the same question but exists only on - /// shard 0, so a read served by a peer shard has no way to tell whether the - /// node caught up to an op its client already saw committed. One - /// process-wide atomic does, for one `Acquire` load on the read fast path. + /// Highest metadata op whose apply has been PUBLISHED on this node, plus + /// the reads parked on it. Shared by every shard; see + /// [`AppliedFrontier`] for the ordering and the wake contract. + applied_frontier: Arc, +} + +impl IggyMetadata, J, S, M, SB> +where + B: MessageBus, +{ + /// Resume the applied frontier where recovery left the state machine. /// - /// Written `Release` right after each apply's `publish()`, read `Acquire`; - /// observing `>= op` therefore happens-after that publish, so a following - /// left-right `enter()` is guaranteed to see the op. `fetch_max` rather - /// than `store` because three writers move it -- the commit loop, the - /// recovery seed, and a state-transfer install -- and only monotonicity - /// makes their order irrelevant. - applied_frontier: Arc, + /// Recovery replays the committed WAL prefix before any listener binds, so + /// without this the frontier reads zero on a rebooted node and every read + /// whose caller holds a pre-restart commit parks until its deadline. A + /// no-op on a peer shard, which owns no consensus and shares shard 0's + /// cell. + pub fn seed_applied_frontier_from_consensus(&self) { + if let Some(consensus) = self.consensus.as_ref() { + self.advance_applied_frontier(consensus.commit_min()); + } + } } impl IggyMetadata @@ -825,7 +832,7 @@ where commit_notifier: RefCell::new(None), client_table_frontier: Cell::new(0), transfer_offer_cache: RefCell::new(None), - applied_frontier: Arc::new(AtomicU64::new(0)), + applied_frontier: Arc::default(), } } } @@ -838,26 +845,27 @@ impl IggyMetadata { /// never moves. Shard 0 mints the cell in bootstrap, before any shard is /// built, and hands each shard a clone. #[must_use] - pub fn with_applied_frontier(mut self, applied_frontier: Arc) -> Self { + pub fn with_applied_frontier(mut self, applied_frontier: Arc) -> Self { self.applied_frontier = applied_frontier; self } - /// Highest metadata op this NODE has applied and published, readable on - /// every shard. Reads gate on it so a client cannot be served state older - /// than a write it already saw acked. + /// The node-wide applied frontier, readable on every shard. Reads gate on + /// it so a client cannot be served state older than a write it already saw + /// acked, and park on its wait when it is behind. #[must_use] - pub fn applied_frontier(&self) -> u64 { - self.applied_frontier.load(Ordering::Acquire) + pub const fn applied_frontier(&self) -> &Arc { + &self.applied_frontier } - /// Publish `op` as applied. Monotone, so a lower value is a no-op. + /// Publish `op` as applied and wake the reads waiting at or below it. + /// Monotone, so a lower value is a no-op. /// /// Must run AFTER the apply's `publish()` and, on the commit path, in the /// same await-free region as `advance_commit_min`: a reader that sees the /// frontier must be guaranteed to see the op's effects. pub fn advance_applied_frontier(&self, op: u64) { - self.applied_frontier.fetch_max(op, Ordering::Release); + self.applied_frontier.advance(op); } /// Slot capacity of the LIVE client table, i.e. the largest transferred @@ -1521,10 +1529,15 @@ impl std::error::Error for StateTransferUnavailable { /// invites a caller to treat a completed install as a failure and redo it. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct InstallOutcome { - /// The receiver's new applied frontier, `max(snapshot_seq, + /// The receiver's applied position after the install, `max(snapshot_seq, /// local_applied)`. These differ whenever a serving peer offered a /// snapshot BEHIND this replica and the local state machine was kept. - pub applied_frontier: u64, + /// + /// Named apart from [`IggyMetadata::applied_frontier`] deliberately: that + /// one is the node-wide cell the read gate consults, which the install + /// raises to `snapshot_seq` alone, so the two carry different numbers + /// exactly when a behind-snapshot was kept. + pub installed_frontier: u64, /// Whether the transferred checkpoint's `(checkpoint_op, checksum)` /// pairing reached the durable superblock. /// @@ -1658,7 +1671,7 @@ where /// reconciler's periodic full diff against the committed STM, which /// reads the restored state on its next tick. /// - /// Returns an [`InstallOutcome`]: the new applied frontier, plus whether + /// Returns an [`InstallOutcome`]: the installed frontier, plus whether /// the transferred checkpoint's pairing reached the durable superblock. /// /// # Errors @@ -1916,7 +1929,7 @@ where } Ok(InstallOutcome { - applied_frontier: snapshot_seq.max(local_applied), + installed_frontier: snapshot_seq.max(local_applied), pairing_durable, }) } @@ -5728,7 +5741,7 @@ mod tests { "ops at or below the floor stay for the walk and tail repair" ); assert_eq!( - md.applied_frontier(), + md.applied_frontier().get(), SNAPSHOT_SEQ, "the snapshot IS ops up to its sequence applied, so the read gate has \ to admit reads at the floor the install jumped to" diff --git a/core/metadata/src/lib.rs b/core/metadata/src/lib.rs index 4d4d24003c..bc771641ce 100644 --- a/core/metadata/src/lib.rs +++ b/core/metadata/src/lib.rs @@ -17,10 +17,15 @@ //! Iggy metadata module +pub mod applied_frontier; pub mod impls; pub mod permissioner; pub mod stm; +// The node-wide read frontier the read gates park on; minted by the bootstrap +// before any shard exists, so it is named outside `impls::`. +pub use applied_frontier::{AppliedFrontier, Reached}; + // Re-export IggyMetadata for use in other modules pub use impls::metadata::{ BoundSession, CommitNotifier, IggyMetadata, MetadataSubmitError, StateTransferOffer, diff --git a/core/server/src/boot/mod.rs b/core/server/src/boot/mod.rs index 82bfa5e5e8..f76d8e6f23 100644 --- a/core/server/src/boot/mod.rs +++ b/core/server/src/boot/mod.rs @@ -76,9 +76,9 @@ use journal::{Journal, JournalHandle}; use message_bus::replica::handshake::ReplicaHandshakeCtx; use message_bus::transports::tls::install_default_crypto_provider; use message_bus::{IggyMessageBus, ReplicaOwnerTable}; -use metadata::ReplicaIdentity; use metadata::impls::metadata::StreamsFrontend; use metadata::impls::recovery::recover; +use metadata::{AppliedFrontier, ReplicaIdentity}; use server_common::Message; use server_common::bootstrap::create_directories; use server_common::fs_utils::remove_dir_all; @@ -92,7 +92,7 @@ use std::cell::RefCell; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::thread; use tracing::{error, info, warn}; @@ -304,10 +304,11 @@ pub fn bootstrap( let mut shard_threads: Vec<(u16, thread::JoinHandle>)> = Vec::with_capacity(shards_count); let roster_cells = RosterCells::default(); - // Shared applied-metadata frontier: shard 0's commit path advances it, every - // shard's read gate reads it. Minted here, before any shard exists, because - // a shard holding a private cell would gate reads on a number nothing moves. - let metadata_applied_frontier = Arc::new(AtomicU64::new(0)); + // Shared applied-metadata frontier: shard 0's commit path advances it and + // wakes the reads parked on it, every shard's read gate reads it. Minted + // here, before any shard exists, because a shard holding a private cell + // would gate reads on a number nothing moves. + let metadata_applied_frontier = Arc::::default(); // Every shard's metric handles, minted before the threads spawn: each // shard bumps its own entry, and shard 0's HTTP scrape endpoint registers // the whole set (counters are Arc-backed, so cross-thread reads see the @@ -435,7 +436,7 @@ async fn shard_main( barrier: BootstrapBarrier, owner_table: Arc, roster_cells: RosterCells, - metadata_applied_frontier: Arc, + metadata_applied_frontier: Arc, shard_metrics_all: Vec, ) -> Result<(), ServerError> { let topology = resolve_tcp_topology(config, replica_id)?; @@ -589,13 +590,7 @@ async fn shard_main( Some(PathBuf::from(&config.system.path)), ) .with_applied_frontier(metadata_applied_frontier); - // Recovery already replayed the committed WAL prefix into the state - // machine, so the frontier resumes where the commit walk will rather than - // at zero -- otherwise every read on a rebooted node parks until its - // deadline. No-op on peer shards, which share shard 0's cell. - if let Some(consensus) = metadata.consensus.as_ref() { - metadata.advance_applied_frontier(consensus.commit_min()); - } + metadata.seed_applied_frontier_from_consensus(); // Size the VSR client table before listeners bind and any client registers. // Must precede the recovered-table install below: the setter rebuilds the // table from scratch, so running it afterwards would drop every resumed diff --git a/core/server/src/boot/threads.rs b/core/server/src/boot/threads.rs index 9822fefb22..2a68d89b96 100644 --- a/core/server/src/boot/threads.rs +++ b/core/server/src/boot/threads.rs @@ -28,13 +28,14 @@ use configs::sharding::{ INBOX_CAPACITY_MAX, SHUTDOWN_DRAIN_TIMEOUT_MAX, SHUTDOWN_POLL_INTERVAL_MAX, }; use message_bus::{IggyMessageBus, ReplicaOwnerTable}; +use metadata::AppliedFrontier; use partitions::FatalCommit; use server_common::executor::create_shard_executor; use shard::metrics::ShardMetrics; use shard::{Receiver as ShardReceiver, Sender, ShardFrame, TaggedSender}; use std::backtrace::Backtrace; use std::rc::Rc; -use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, OnceLock}; use std::time::{Duration, Instant}; use std::{panic, thread}; @@ -426,7 +427,7 @@ pub(in crate::boot) fn run_shard_thread( barrier: BootstrapBarrier, owner_table: Arc, roster_cells: RosterCells, - metadata_applied_frontier: Arc, + metadata_applied_frontier: Arc, shard_metrics_all: Vec, ) -> Result<(), ServerError> { // Armed for the whole thread body: a post-spawn error `?` or a panic diff --git a/core/server/src/dispatch/reads.rs b/core/server/src/dispatch/reads.rs index 3e85bdfe88..dc6fe9e38e 100644 --- a/core/server/src/dispatch/reads.rs +++ b/core/server/src/dispatch/reads.rs @@ -39,13 +39,15 @@ use crate::snapshot; use crate::wire::request_body; use bytes::Bytes; use configs::server::ServerSystemConfig; -use consensus::MetadataHandle; +use consensus::{MetadataHandle, TICK_INTERVAL, TimeoutManager}; +use futures::future::{Either, select}; use iggy_binary_protocol::PrepareHeader; use iggy_binary_protocol::codes::{ DESCRIBE_OPTIONS_CODE, GET_CLIENT_CODE, GET_CLIENTS_CODE, GET_CLUSTER_METADATA_CODE, GET_CONSUMER_OFFSET_CODE, GET_ME_CODE, GET_PERSONAL_ACCESS_TOKENS_CODE, GET_SNAPSHOT_FILE_CODE, GET_STATS_CODE, PING_CODE, POLL_MESSAGES_CODE, SYNC_CONSUMER_GROUP_CODE, }; +use iggy_binary_protocol::dispatch::lookup_command; use iggy_binary_protocol::requests::consumer_groups::SyncConsumerGroupRequest; use iggy_binary_protocol::requests::system::get_client::GetClientRequest; use iggy_binary_protocol::requests::system::get_snapshot::GetSnapshotRequest; @@ -59,11 +61,14 @@ use iggy_common::{IggyError, SnapshotCompression, SystemSnapshotType}; use journal::superblock::SuperblockStore; use journal::{Journal, JournalHandle}; use message_bus::framing::MAX_MESSAGE_SIZE; +use metadata::AppliedFrontier; use metadata::impls::metadata::StreamsFrontend; use metadata::permissioner::Permissioner; use server_common::Message; use std::cell::RefCell; +use std::future::Future; use std::net::IpAddr; +use std::pin::pin; use std::rc::Rc; use std::sync::Arc; use std::time::Duration; @@ -123,30 +128,109 @@ async fn handle_get_me( .await; } -/// Poll cadence while a read waits for this node's applied metadata frontier. -/// The consensus tick, so a node one commit behind resumes on the next commit -/// broadcast rather than a tick later. -const READ_FRONTIER_POLL: Duration = Duration::from_millis(10); +/// Budget one held read is given before it fails retryable, in consensus +/// ticks: six commit-broadcast intervals. +/// +/// Sized for a node merely behind on its commit walk, NOT for a view change -- +/// detecting one costs `heartbeat_timeout` and escalating it another +/// `view_change_status_timeout`, and `recovery_barrier_deadline` budgets at +/// least 15s for the same event, so a read that waits out an election is a read +/// the caller should retry elsewhere. Far below the SDK's 30s request budget, +/// inside which it replays the same id on the same connection. +/// +/// In ticks rather than a bare duration because that is the unit the thing +/// being waited for moves in, and the unit the simulator steps in. +#[allow(clippy::cast_possible_truncation)] +pub const READ_FRONTIER_BUDGET_TICKS: u32 = 6 * TimeoutManager::COMMIT_MESSAGE_TICKS as u32; -/// Polls a held read is given before it fails retryable: 3s at the cadence -/// above. Long enough to ride out a view change, far below the SDK's 30s -/// request budget, inside which it replays the same id on the same connection. -const READ_FRONTIER_MAX_POLLS: u32 = 300; +/// The same budget as a duration, for the timers the two planes measure it +/// with. +pub const READ_FRONTIER_BUDGET: Duration = + match TICK_INTERVAL.checked_mul(READ_FRONTIER_BUDGET_TICKS) { + Some(budget) => budget, + None => panic!("the read frontier budget must fit a Duration"), + }; /// Whether `code`'s answer comes from the metadata state machine, and so must /// not be served below the caller's watermark. /// -/// The two exclusions only look like metadata reads: `DescribeOptions` decodes -/// a static catalog, and `GetClusterMetadata` answers from the configured -/// roster plus the consensus view. Holding either buys no consistency, and the -/// roster read is on the SDK's leader-discovery path, where the wait would be -/// real. +/// The two named exclusions only look like metadata reads: `DescribeOptions` +/// decodes a static catalog, and `GetClusterMetadata` answers from the +/// configured roster plus the consensus view. Holding either buys no +/// consistency, and the roster read is on the SDK's leader-discovery path, +/// where the wait would be real. A code this build does not know is excluded +/// too: its only outcome is `InvalidCommand`, and parking a terminal error for +/// the whole budget serves nobody. +/// +/// A deny-list otherwise, so a read code added later is gated by default: the +/// failure mode of forgetting to add one is a wait, while forgetting to add it +/// to an allow-list is a silent stale read. /// /// Shared with the HTTP read path, which gates the identical set of command /// codes through `build_non_replicated_response`: two lists would drift, and a /// code dropped from one plane's list is a silent stale read on that plane. pub const fn read_needs_metadata_frontier(code: u32) -> bool { !matches!(code, DESCRIBE_OPTIONS_CODE | GET_CLUSTER_METADATA_CODE) + && lookup_command(code).is_some() +} + +/// Whether a frontier wait actually parked. +/// +/// The caller's authorization resolved its scope off the pre-wait state +/// machine, and a wait that parked is one where that state machine moved, so +/// only the parked outcome forces the gate to run again. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum FrontierWait { + /// The frontier already covered the watermark: no await ran. + Ready, + /// The read parked, and the frontier caught up while it waited. + CaughtUp, +} + +/// The frontier never reached the watermark inside the budget. Each plane +/// renders it in its own error currency. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct FrontierUnreached; + +/// Hold a read until `frontier` covers `watermark`, or until `budget` expires. +/// +/// Event-driven: the wait is woken by the commit that advances the frontier +/// (see [`AppliedFrontier::advance`]), so a read resumes on the commit it was +/// short of rather than on a poll that happens to land after it. The caller +/// supplies the budget as a future because the two read planes measure time +/// differently -- the shard bus timer, which is virtual under the simulator, +/// against `compio::time` on the HTTP listener. +/// +/// Shared by both planes because one wait with two copies is one wait with a +/// drift vector, and split from its callers so the fast path, the park, the +/// wake and the expiry are all testable without a live shard. +/// +/// A caller with nothing to read back has `watermark == 0`, which the first +/// comparison satisfies: one `Acquire` load, no registration, no await. Expiry +/// is loud and carries both numbers, so a frontier that stopped moving is +/// visible instead of showing up as latency. +pub async fn hold_for_frontier( + frontier: &AppliedFrontier, + watermark: u64, + budget: impl Future, +) -> Result { + if frontier.get() >= watermark { + return Ok(FrontierWait::Ready); + } + let reached = pin!(frontier.reached(watermark)); + let budget = pin!(budget); + match select(reached, budget).await { + Either::Left(((), _)) => Ok(FrontierWait::CaughtUp), + Either::Right(((), _)) => { + warn!( + frontier = frontier.get(), + watermark, + budget = ?READ_FRONTIER_BUDGET, + "metadata read frontier unreached inside the budget; failing the read retryable" + ); + Err(FrontierUnreached) + } + } } /// Hold a local metadata read until this node has applied everything the @@ -161,17 +245,52 @@ pub const fn read_needs_metadata_frontier(code: u32) -> bool { /// every shard of a node reads one shared frontier and gates identically. /// /// Fast path is a single `Acquire` load and no await, which is what keeps an -/// uncontended read shared-nothing. Otherwise it polls the bus timer (virtual -/// under the simulator, wall clock in production) from inside the -/// per-connection drain task, so only this connection waits. Expiry fails -/// loud and retryable rather than serving state the client already saw -/// replaced; the log carries both numbers so a frontier that stopped moving -/// is visible instead of showing up as a hang. +/// uncontended read shared-nothing. A park costs this connection more than the +/// read itself: the per-connection drain loop serves one frame at a time, so +/// the client's queued `SendMessages`, `PollMessages` and `PING` wait behind +/// the held read. No OTHER connection waits, and the budget above is what +/// bounds it. Expiry fails loud and retryable rather than serving state the +/// client already saw replaced. +/// +/// The wait ends on the commit that closes the gap, not on a poll: the budget +/// timer is the only timer armed, so a read that resumes costs one wake. #[allow(clippy::future_not_send)] async fn await_metadata_read_frontier( shard: &Rc>, - sessions: &Rc>, - transport_client_id: u128, + watermark: u64, +) -> Result +where + B: ShellBus, + MJ: JournalHandle + 'static, + MJ::Target: Journal, Header = PrepareHeader>, + S: 'static, + SB: SuperblockStore + 'static, +{ + hold_for_frontier( + shard.plane.metadata().applied_frontier(), + watermark, + shard.bus.sleep(READ_FRONTIER_BUDGET), + ) + .await + // `TransientNotAccepted`, not `NotCommitted`: a read never entered a + // pipeline, so it is safe to re-issue anywhere, and it is the code that + // drives the SDK's roster walk rather than a replay against the same + // durably lagging replica. + .map_err(|FrontierUnreached| IggyError::TransientNotAccepted) +} + +/// Authorize a metadata read, then hold it for this node's applied frontier. +/// +/// Authorization first: a denial is terminal, and parking the connection for +/// the whole budget before answering one buys nothing. Then again on a wait +/// that parked -- the rule resolves its scope and the caller's grants off the +/// state machine, and a park is exactly the case where both moved under it. +#[allow(clippy::future_not_send)] +async fn authorize_and_hold_read( + shard: &Rc>, + code: u32, + watermark: u64, + authorize: impl Fn() -> Result<(), IggyError>, ) -> Result<(), IggyError> where B: ShellBus, @@ -180,24 +299,14 @@ where S: 'static, SB: SuperblockStore + 'static, { - // Own statement: the borrow has to be released before the poll below, which - // awaits on the same task the session manager's mutators run on. - let watermark = sessions.borrow().metadata_watermark(transport_client_id); - let metadata = shard.plane.metadata(); - if metadata.applied_frontier() >= watermark { + authorize()?; + if !read_needs_metadata_frontier(code) { return Ok(()); } - for _ in 0..READ_FRONTIER_MAX_POLLS { - shard.bus.sleep(READ_FRONTIER_POLL).await; - if metadata.applied_frontier() >= watermark { - return Ok(()); - } + if await_metadata_read_frontier(shard, watermark).await? == FrontierWait::CaughtUp { + authorize()?; } - warn!( - frontier = metadata.applied_frontier(), - watermark, "metadata read frontier unreached past deadline; failing the read retryable" - ); - Err(IggyError::TransientNotCommitted) + Ok(()) } #[allow(clippy::future_not_send, clippy::too_many_lines)] @@ -216,10 +325,11 @@ pub(in crate::dispatch) async fn handle_non_replicated_request( { const CODE_RANGE: std::ops::Range = 0..4; let code = u32::from_le_bytes(request.header().reserved[CODE_RANGE].try_into().unwrap()); - // Acting user and peer address for the read gates below, resolved in one - // connection lookup. `user_id` is `None` only on the pre-auth path - // (PING), which serves ungated codes; the gated arms fail closed on it. - let (user_id, client_address) = sessions.borrow().read_context(transport_client_id); + // Acting user, peer address and read-your-writes floor for the gates + // below, resolved in one connection lookup. `user_id` is `None` only on the + // pre-auth path (PING), which serves ungated codes; the gated arms fail + // closed on it. + let (user_id, client_address, watermark) = sessions.borrow().read_context(transport_client_id); match code { PING_CODE => { // A ping is the client's liveness proof; reset its staleness clock @@ -245,12 +355,18 @@ pub(in crate::dispatch) async fn handle_non_replicated_request( } } GET_ME_CODE => { + // Self-scoped, so no permissioner rule -- but the consumer-group + // list it carries is read off the streams STM, so it is gated like + // any other metadata read. + if let Err(error) = authorize_and_hold_read(shard, code, watermark, || Ok(())).await { + send_non_replicated_deny(shard, &request, transport_client_id, error.as_code()) + .await; + return; + } handle_get_me(shard, sessions, transport_client_id, &request).await; } GET_PERSONAL_ACCESS_TOKENS_CODE => { - if let Err(error) = - await_metadata_read_frontier(shard, sessions, transport_client_id).await - { + if let Err(error) = authorize_and_hold_read(shard, code, watermark, || Ok(())).await { send_non_replicated_deny(shard, &request, transport_client_id, error.as_code()) .await; return; @@ -258,7 +374,11 @@ pub(in crate::dispatch) async fn handle_non_replicated_request( handle_get_personal_access_tokens(shard, sessions, transport_client_id, &request).await; } GET_CLIENTS_CODE => { - if let Err(error) = authorize_uid(shard, user_id, Permissioner::get_clients) { + if let Err(error) = authorize_and_hold_read(shard, code, watermark, || { + authorize_uid(shard, user_id, Permissioner::get_clients) + }) + .await + { send_non_replicated_deny(shard, &request, transport_client_id, error.as_code()) .await; return; @@ -282,7 +402,11 @@ pub(in crate::dispatch) async fn handle_non_replicated_request( .await; } GET_CLIENT_CODE => { - if let Err(error) = authorize_uid(shard, user_id, Permissioner::get_client) { + if let Err(error) = authorize_and_hold_read(shard, code, watermark, || { + authorize_uid(shard, user_id, Permissioner::get_client) + }) + .await + { send_non_replicated_deny(shard, &request, transport_client_id, error.as_code()) .await; return; @@ -335,18 +459,16 @@ pub(in crate::dispatch) async fn handle_non_replicated_request( } SYNC_CONSUMER_GROUP_CODE => { // Self-scoped: serves the caller's own assignment keyed by the - // header client id, so it carries no permissioner rule. - handle_sync_consumer_group(shard, transport_client_id, &request).await; - } - _ => { - if read_needs_metadata_frontier(code) - && let Err(error) = - await_metadata_read_frontier(shard, sessions, transport_client_id).await - { + // header client id, so it carries no permissioner rule. The + // assignment itself is metadata-STM state, hence the gate. + if let Err(error) = authorize_and_hold_read(shard, code, watermark, || Ok(())).await { send_non_replicated_deny(shard, &request, transport_client_id, error.as_code()) .await; return; } + handle_sync_consumer_group(shard, transport_client_id, &request).await; + } + _ => { let roster = sessions.borrow().cluster_roster(); let client_ip = client_address.map(|address| address.ip()); if client_ip.is_none() { @@ -362,6 +484,7 @@ pub(in crate::dispatch) async fn handle_non_replicated_request( code, &request, user_id, + watermark, &roster, client_ip, ) @@ -377,6 +500,7 @@ async fn handle_default_non_replicated( code: u32, request: &Message, user_id: Option, + watermark: u64, roster: &ClusterRoster, client_ip: Option, ) where @@ -388,8 +512,14 @@ async fn handle_default_non_replicated( { // Gate by command code before the shared builder runs. The builder stays // authz-free (it is byte-shared with the HTTP read path, which gates - // separately); a denial replies status!=0 with an empty body. - if let Err(error) = authorize_default_read(shard, code, request_body(request), user_id) { + // separately); a denial replies status!=0 with an empty body. The + // read-your-writes hold sits INSIDE the same call, behind that denial: an + // unauthorized read must fail now, not after the whole poll budget. + if let Err(error) = authorize_and_hold_read(shard, code, watermark, || { + authorize_default_read(shard, code, request_body(request), user_id) + }) + .await + { send_non_replicated_deny(shard, request, transport_client_id, error.as_code()).await; return; } @@ -582,3 +712,109 @@ async fn handle_sync_consumer_group( ) .await; } + +#[cfg(test)] +mod tests { + use super::{FrontierUnreached, FrontierWait, hold_for_frontier, read_needs_metadata_frontier}; + use iggy_binary_protocol::codes::{ + DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE, GET_ME_CODE, GET_STREAM_CODE, + SYNC_CONSUMER_GROUP_CODE, + }; + use metadata::AppliedFrontier; + use std::future::pending; + use std::sync::Arc; + + /// A caller with nothing to read back (`watermark == 0`) and one whose + /// watermark this node has already applied are the whole steady state, and + /// neither may cost a park: no registration, no await. The budget here is + /// a future that never completes, so a gate that parked would hang instead + /// of quietly costing a tick. + #[compio::test] + async fn given_a_frontier_at_the_watermark_when_gating_should_serve_without_parking() { + let frontier = AppliedFrontier::default(); + frontier.advance(9); + for watermark in [0, 7, 9] { + assert_eq!( + hold_for_frontier(&frontier, watermark, pending()).await, + Ok(FrontierWait::Ready), + "frontier 9 covers {watermark}, so the read must not park" + ); + } + assert_eq!(frontier.waiting(), 0, "a served read registers no wait"); + } + + /// The gate's whole point, and the reason the wait is event-driven: a read + /// whose caller was told op 9 committed is held while this node is at 4, + /// and the COMMIT that advances the frontier is what answers it - here a + /// detached task standing in for the commit path, with no timer in the + /// budget at all. The parked outcome is what tells the caller to re-run + /// the authorization it resolved off the pre-wait state machine. + #[compio::test] + async fn given_a_frontier_behind_the_watermark_when_it_advances_should_answer_the_held_read() { + let frontier = Arc::new(AppliedFrontier::default()); + frontier.advance(4); + let committer = Arc::clone(&frontier); + compio::runtime::spawn(async move { + // Yields first, so the read is provably parked before the advance: + // a gate that answered off the lagging frontier would already have + // returned by the time this runs. + compio::runtime::time::sleep(std::time::Duration::ZERO).await; + committer.advance(9); + }) + .detach(); + + assert_eq!( + hold_for_frontier(&frontier, 9, pending()).await, + Ok(FrontierWait::CaughtUp), + "the commit that closed the gap must answer the held read" + ); + assert_eq!(frontier.waiting(), 0, "the answered wait deregisters"); + } + + /// A node can legitimately never catch up (a durably lagging replica), so + /// the wait is bounded - and the exit is a refusal, never the stale answer. + /// Each plane renders it retryable: `TransientNotAccepted` on the binary + /// transports, the shared 503 over HTTP. + #[compio::test] + async fn given_a_frontier_that_never_catches_up_when_the_budget_expires_should_fail_retryable() + { + let frontier = AppliedFrontier::default(); + frontier.advance(4); + assert_eq!( + hold_for_frontier(&frontier, 9, std::future::ready(())).await, + Err(FrontierUnreached), + "an unreached frontier must refuse the read, not serve it" + ); + assert_eq!( + frontier.waiting(), + 0, + "the expired wait must not leave its waker behind" + ); + } + + /// The deny-list's whole point is that a read answered from the metadata + /// STM is gated even when nobody remembered to name it, so the arms that + /// are NOT gated are the ones worth pinning: the static catalog, the + /// roster read on the leader-discovery path, and a code this build cannot + /// serve at all (whose only outcome is `InvalidCommand`, which must not + /// wait out the budget first). + #[test] + fn given_a_read_code_when_classified_should_gate_all_but_the_named_exclusions() { + for code in [GET_STREAM_CODE, GET_ME_CODE, SYNC_CONSUMER_GROUP_CODE] { + assert!( + read_needs_metadata_frontier(code), + "code {code} answers from the metadata STM and must be gated" + ); + } + for code in [DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE] { + assert!( + !read_needs_metadata_frontier(code), + "code {code} answers from a static catalog or the roster; holding it buys nothing" + ); + } + assert!( + !read_needs_metadata_frontier(u32::MAX), + "an unknown code has no answer to hold, so it must not park" + ); + } +} diff --git a/core/server/src/dispatch/submit.rs b/core/server/src/dispatch/submit.rs index a910173e7a..0350c12e3e 100644 --- a/core/server/src/dispatch/submit.rs +++ b/core/server/src/dispatch/submit.rs @@ -29,9 +29,10 @@ use crate::dispatch::session_ops::{ submit_register_local_or_forward, }; use crate::dispatch::upgrade_shard_handle; -use crate::http::reply::transient_code; +use crate::responses::{reply_body, transient_code}; use crate::shell::{ShellBus, ShellShard, ShellShardHandle}; use consensus::MetadataHandle; +use iggy_binary_protocol::consensus::result_code; use iggy_binary_protocol::{ Command, GenericHeader, PrepareHeader, ReplyHeader, RoutedRequestHeader, }; @@ -196,26 +197,142 @@ where rx.recv().await.ok().flatten() } -/// The commit position a COMMITTED metadata reply carries, or `None` when the -/// frame promises the client nothing. +/// The commit position a SUCCESSFULLY COMMITTED metadata reply carries, or +/// `None` when the frame promises the caller nothing. /// -/// Three frames arrive on this path and only one is a promise. An eviction is -/// an `EvictionHeader` whose bytes would cast cleanly as a reply, so the -/// command is checked first (same guard as `build_raw_pat_reply`). A -/// pre-consensus rejection stamps the primary's `commit_max`, an op the caller -/// was never told committed and, on a backup-homed caller, one the read gate -/// would then wait for. A committed business rejection (duplicate name, bad -/// expiry) DID commit and counts. +/// Only a success promises. Every other frame on this path stamps `commit` with +/// the primary's `commit_max`, an op the caller was never told committed and, +/// on a backup-homed caller, one its own reads would then wait for: /// -/// Shared with the HTTP write path, which grades the same three frames off the -/// same submit entry point ([`submit_client_request_on_owner`]); one classifier -/// is what keeps the two planes' watermarks meaning the same thing. +/// - an eviction is an `EvictionHeader` whose bytes would cast cleanly as a +/// reply, so the command is checked first (same guard as +/// `build_raw_pat_reply`); +/// - a request-level denial names itself in `ReplyHeader.status`, the channel +/// the SDK peeks before body decode (see `build_deny_reply`); +/// - a transient rejection did not commit and will be replayed; +/// - a TERMINAL pre-consensus rejection (`PreflightOutcome::Reject`, e.g. a +/// fenced session) is a result section carrying a non-transient code, which +/// is byte-identical to a COMMITTED business rejection (duplicate name, bad +/// expiry). Neither is separable here, and neither has to be: a rejection +/// mutated nothing, so the caller has nothing to read back from it, and +/// grading both as no-promise is the only reading that cannot make a read +/// wait for an op that never committed. +/// +/// Every reply on this path is result-framed (`Operation::is_result_framed` +/// covers every metadata op; the partition plane grades through +/// `classify_partition_reply` instead), so a missing result section is a +/// malformed frame, not a bare payload. +/// +/// Shared with the HTTP write path, which grades the same frames off the same +/// submit entry point ([`submit_client_request_on_owner`]); one classifier is +/// what keeps the two planes' watermarks meaning the same thing. pub fn committed_reply_commit(reply: &Message) -> Option { if reply.header().command != Command::Reply || transient_code(reply).is_some() { return None; } - let header = reply.as_slice().get(..size_of::())?; - bytemuck::checked::try_from_bytes::(header) - .ok() - .map(|header| header.commit) + let Some(bytes) = reply.as_slice().get(..size_of::()) else { + warn!( + size = reply.header().size, + "metadata reply shorter than its own header; not advancing the read watermark" + ); + return None; + }; + let header = match bytemuck::checked::try_from_bytes::(bytes) { + Ok(header) => header, + Err(error) => { + warn!( + ?error, + "metadata reply header failed to cast; not advancing the read watermark" + ); + return None; + } + }; + if header.status != 0 { + return None; + } + (result_code(reply_body(reply)) == Some(0)).then_some(header.commit) +} + +#[cfg(test)] +mod tests { + use super::committed_reply_commit; + use crate::dispatch::test_support::request_message; + use crate::responses::{build_deny_reply, build_reply_from_bytes}; + use bytes::Bytes; + use iggy_binary_protocol::Operation; + use iggy_common::IggyError; + + /// Commit position of the frames below. Above zero on purpose: `0` is the + /// "promised nothing" answer, so a fixture at zero could not tell a + /// classified success from a rejected frame. + const COMMIT: u64 = 9; + + /// A result-framed body: `[count][index][result]`, then the payload. + fn result_body(code: u32, payload: &[u8]) -> Bytes { + let mut body = Vec::new(); + let count = u32::from(code != 0); + body.extend_from_slice(&count.to_le_bytes()); + if count == 1 { + body.extend_from_slice(&0u32.to_le_bytes()); + body.extend_from_slice(&code.to_le_bytes()); + } + body.extend_from_slice(payload); + Bytes::from(body) + } + + /// The whole classification in one table: only a successful commit hands + /// the read gate a floor. Everything else stamps the primary's + /// `commit_max` into a frame that promised the caller nothing, and a floor + /// taken from one of those parks the caller's next read on a backup until + /// the budget expires. + #[test] + fn given_a_metadata_reply_when_classified_should_promise_only_a_committed_success() { + let request = request_message(Operation::CreateStream, 42, 7, 3, &[]); + + let committed = + build_reply_from_bytes(request.header(), 42, 7, COMMIT, &result_body(0, b"payload")) + .into_generic(); + assert_eq!( + committed_reply_commit(&committed), + Some(COMMIT), + "a committed success is the one frame that promises the caller its op" + ); + + for code in [ + IggyError::TransientNotCommitted.as_code(), + IggyError::TransientNotAccepted.as_code(), + IggyError::UserAlreadyExists.as_code(), + ] { + let rejected = + build_reply_from_bytes(request.header(), 42, 7, COMMIT, &result_body(code, &[])) + .into_generic(); + assert_eq!( + committed_reply_commit(&rejected), + None, + "result code {code} mutated nothing, so it promises no read floor" + ); + } + + let denied = build_deny_reply( + request.header(), + 42, + 7, + COMMIT, + IggyError::Unauthorized.as_code(), + ) + .into_generic(); + assert_eq!( + committed_reply_commit(&denied), + None, + "a request-level denial names itself in `status` and commits nothing" + ); + + // Any non-`Reply` command stands in for the eviction frame, whose bytes + // would otherwise cast cleanly as a `ReplyHeader`. + assert_eq!( + committed_reply_commit(&request.into_generic()), + None, + "only a `Reply` carries a commit position" + ); + } } diff --git a/core/server/src/http.rs b/core/server/src/http.rs index 873b05685a..8748ea820e 100644 --- a/core/server/src/http.rs +++ b/core/server/src/http.rs @@ -31,7 +31,7 @@ mod jwks; mod jwt; mod metrics; mod reads; -pub mod reply; +mod reply; mod session; mod state; mod submit; @@ -232,6 +232,7 @@ pub fn start( in_flight_writes: Cell::new(0), forward, metrics: metrics::HttpMetrics::init(shard_metrics_all), + metadata_watermarks: Rc::default(), })); let app = router( state, diff --git a/core/server/src/http/extractor.rs b/core/server/src/http/extractor.rs index f3053612a2..075ab72965 100644 --- a/core/server/src/http/extractor.rs +++ b/core/server/src/http/extractor.rs @@ -94,14 +94,6 @@ impl FromRequestParts for Authenticated { /// [`resolve_credential`] chokepoint, so a JWT and a PAT are honored identically. pub struct Identity { pub user_id: u32, - /// Session-table key of the presenting credential (`jwt:{jti}` / - /// `pat:{sha}`), the SAME key [`Authenticated`] resolves its session under. - /// - /// A read mints no session, so this is the only join back to what this - /// credential's writes committed: the read gate looks its metadata - /// watermark up by this key. Carried rather than resolved again in the - /// handler because [`resolve_credential`] already computed it. - pub session_key: String, /// Original request path + query (e.g. `/streams?consistency=linearizable`), /// captured so a linearizable read that reaches a follower can build the /// `Location` for its 307 redirect to the primary. Empty only when the URI @@ -124,18 +116,17 @@ impl FromRequestParts for Identity { ) -> Result { let bearer = bearer_token(&parts.headers)?; - // Verify only: no session is minted or Registered. The key is kept - // (the read gate resolves this credential's metadata watermark under - // it) while the expiry is discarded - nothing here installs a table - // entry to expire. + // Verify only. The session key and expiry `resolve_credential` also + // returns feed the write path's session table; a read discards them - + // its read-your-writes floor is keyed by user id, not by credential + // (see `MetadataWatermarks`). // The verify is `!Send` (a trusted-issuer JWT may await a JWKS fetch), // so bridge it with `SendWrapper` - sound only because compio pins this // future to shard 0's single thread, the only thread the JWKS client // ever runs on (mirrors legacy `HttpSafeShard`). It holds no `RefCell` // borrow or `DashMap` guard across the `.await`, so a sibling task // scheduled on this thread meanwhile never observes a borrowed cell. - let (session_key, user_id, _expiry) = - SendWrapper::new(resolve_credential(state, bearer)).await?; + let (_key, user_id, _expiry) = SendWrapper::new(resolve_credential(state, bearer)).await?; let path_and_query = parts .uri .path_and_query() @@ -153,7 +144,6 @@ impl FromRequestParts for Identity { } Ok(Self { user_id, - session_key, path_and_query, client_ip, }) diff --git a/core/server/src/http/handlers.rs b/core/server/src/http/handlers.rs index 8483138096..c953edb7c0 100644 --- a/core/server/src/http/handlers.rs +++ b/core/server/src/http/handlers.rs @@ -334,9 +334,6 @@ pub(in crate::http) async fn get_stream( ) -> Result, ReadError> { let stream_id = Identifier::from_str_value(&stream_id).map_err(ReadError::Rejected)?; let wire_stream_id = identifier_to_wire(&stream_id).map_err(ReadError::Rejected)?; - // Resolve for the gate; a miss leaves it a pass-through so the read renders - // the existing 404 rather than a 403. - let scope = resolve_gate_stream(&state, &wire_stream_id); let request = GetStreamRequest { stream_id: wire_stream_id, }; @@ -347,8 +344,14 @@ pub(in crate::http) async fn get_stream( query.consistency, GET_STREAM_CODE, &body, + // Resolved when the rule RUNS, not here: `read_local` can park for the + // read-your-writes frontier, and an entity created during that wait + // would resolve to nothing on a pre-wait pass, where a miss is a + // pass-through. A miss still leaves the gate a pass-through so the read + // renders the existing 404 rather than a 403. |permissioner, uid| { - scope.map_or(Ok(()), |stream_id| permissioner.get_stream(uid, stream_id)) + resolve_gate_stream(&state, &request.stream_id) + .map_or(Ok(()), |stream_id| permissioner.get_stream(uid, stream_id)) }, )) .await?; @@ -370,7 +373,6 @@ pub(in crate::http) async fn get_topics( ) -> Result>, ReadError> { let stream_id = Identifier::from_str_value(&stream_id).map_err(ReadError::Rejected)?; let wire_stream_id = identifier_to_wire(&stream_id).map_err(ReadError::Rejected)?; - let scope = resolve_gate_stream(&state, &wire_stream_id); let request = GetTopicsRequest { stream_id: wire_stream_id, }; @@ -382,7 +384,8 @@ pub(in crate::http) async fn get_topics( GET_TOPICS_CODE, &body, |permissioner, uid| { - scope.map_or(Ok(()), |stream_id| permissioner.get_topics(uid, stream_id)) + resolve_gate_stream(&state, &request.stream_id) + .map_or(Ok(()), |stream_id| permissioner.get_topics(uid, stream_id)) }, )) .await?; @@ -406,7 +409,6 @@ pub(in crate::http) async fn get_topic( let topic_id = Identifier::from_str_value(&topic_id).map_err(ReadError::Rejected)?; let wire_stream_id = identifier_to_wire(&stream_id).map_err(ReadError::Rejected)?; let wire_topic_id = identifier_to_wire(&topic_id).map_err(ReadError::Rejected)?; - let scope = resolve_gate_topic(&state, &wire_stream_id, &wire_topic_id); let request = GetTopicRequest { stream_id: wire_stream_id, topic_id: wire_topic_id, @@ -419,9 +421,10 @@ pub(in crate::http) async fn get_topic( GET_TOPIC_CODE, &body, |permissioner, uid| { - scope.map_or(Ok(()), |(stream_id, topic_id)| { - permissioner.get_topic(uid, stream_id, topic_id) - }) + resolve_gate_topic(&state, &request.stream_id, &request.topic_id) + .map_or(Ok(()), |(stream_id, topic_id)| { + permissioner.get_topic(uid, stream_id, topic_id) + }) }, )) .await?; @@ -473,7 +476,6 @@ pub(in crate::http) async fn get_user( let user_id = Identifier::from_str_value(&user_id).map_err(ReadError::Rejected)?; identifier_to_wire(&user_id).map_err(ReadError::Rejected)? }; - let is_self = resolve_gate_user(&state, &wire_user_id) == Some(identity.user_id as usize); let request = GetUserRequest { user_id: wire_user_id, }; @@ -485,6 +487,9 @@ pub(in crate::http) async fn get_user( GET_USER_CODE, &body, |permissioner, uid| { + #[allow(clippy::cast_possible_truncation)] + let is_self = + resolve_gate_user(&state, &request.user_id) == Some(identity.user_id as usize); if is_self { Ok(()) } else { @@ -513,7 +518,6 @@ pub(in crate::http) async fn get_cgs( let topic_id = Identifier::from_str_value(&topic_id).map_err(ReadError::Rejected)?; let wire_stream_id = identifier_to_wire(&stream_id).map_err(ReadError::Rejected)?; let wire_topic_id = identifier_to_wire(&topic_id).map_err(ReadError::Rejected)?; - let scope = resolve_gate_topic(&state, &wire_stream_id, &wire_topic_id); let request = GetConsumerGroupsRequest { stream_id: wire_stream_id, topic_id: wire_topic_id, @@ -526,9 +530,10 @@ pub(in crate::http) async fn get_cgs( GET_CONSUMER_GROUPS_CODE, &body, |permissioner, uid| { - scope.map_or(Ok(()), |(stream_id, topic_id)| { - permissioner.get_consumer_groups(uid, stream_id, topic_id) - }) + resolve_gate_topic(&state, &request.stream_id, &request.topic_id) + .map_or(Ok(()), |(stream_id, topic_id)| { + permissioner.get_consumer_groups(uid, stream_id, topic_id) + }) }, )) .await?; @@ -552,7 +557,6 @@ pub(in crate::http) async fn get_cg( let group_id = Identifier::from_str_value(&group_id).map_err(ReadError::Rejected)?; let wire_stream_id = identifier_to_wire(&stream_id).map_err(ReadError::Rejected)?; let wire_topic_id = identifier_to_wire(&topic_id).map_err(ReadError::Rejected)?; - let scope = resolve_gate_topic(&state, &wire_stream_id, &wire_topic_id); let request = GetConsumerGroupRequest { stream_id: wire_stream_id, topic_id: wire_topic_id, @@ -566,9 +570,10 @@ pub(in crate::http) async fn get_cg( GET_CONSUMER_GROUP_CODE, &body, |permissioner, uid| { - scope.map_or(Ok(()), |(stream_id, topic_id)| { - permissioner.get_consumer_group(uid, stream_id, topic_id) - }) + resolve_gate_topic(&state, &request.stream_id, &request.topic_id) + .map_or(Ok(()), |(stream_id, topic_id)| { + permissioner.get_consumer_group(uid, stream_id, topic_id) + }) }, )) .await?; diff --git a/core/server/src/http/reads.rs b/core/server/src/http/reads.rs index 64cd3fcd63..55188c9b9c 100644 --- a/core/server/src/http/reads.rs +++ b/core/server/src/http/reads.rs @@ -17,11 +17,14 @@ //! Read-path gates: the shared per-op RBAC + consistency check, the two waits //! a local read serves behind (the post-restart recovery barrier and the -//! per-credential read-your-writes frontier), the local metadata-STM read +//! per-user read-your-writes frontier), the local metadata-STM read //! entry, and the wire/domain identifier resolvers the read and data-plane //! routes ground their scopes through. -use crate::dispatch::reads::read_needs_metadata_frontier; +use crate::dispatch::reads::{ + FrontierUnreached, FrontierWait, READ_FRONTIER_BUDGET, hold_for_frontier, + read_needs_metadata_frontier, +}; use crate::shell::ServerShard; use bytes::Bytes; use consensus::MetadataHandle; @@ -59,7 +62,7 @@ pub(in crate::http) fn authorize_read( state: &HttpInner, identity: &Identity, consistency: Consistency, - rule: impl FnOnce(&Permissioner, u32) -> Result<(), IggyError>, + rule: impl Fn(&Permissioner, u32) -> Result<(), IggyError>, ) -> Result<(), ReadError> { state .shard @@ -94,7 +97,7 @@ pub(in crate::http) async fn read_local( consistency: Consistency, code: u32, body: &[u8], - rule: impl FnOnce(&Permissioner, u32) -> Result<(), IggyError>, + rule: impl Fn(&Permissioner, u32) -> Result<(), IggyError>, ) -> Result { await_recovery_barrier(&state.shard).await?; // Ahead of the frontier wait on purpose. `authorize_read` renders the @@ -102,9 +105,17 @@ pub(in crate::http) async fn read_local( // parking first would delay a request this node is not going to serve at // all - and an authorization denial is terminal, so holding the connection // for it buys nothing. - authorize_read(state, identity, consistency, rule)?; - if read_needs_metadata_frontier(code) { - await_metadata_read_frontier(state, identity).await?; + authorize_read(state, identity, consistency, &rule)?; + if read_needs_metadata_frontier(code) + && await_metadata_read_frontier(state, identity).await? == FrontierWait::CaughtUp + { + // Every scoped route's rule resolves its entity when the rule RUNS, + // and a park is precisely the case where the state machine moved under + // it: an entity that did not exist on the first pass resolved to + // nothing, where a scope miss is a pass-through, and would be served + // with no permissioner call at all. Only the parked outcome pays for + // the second pass. + authorize_read(state, identity, consistency, &rule)?; } let clients_count = if code == GET_STATS_CODE { u32::try_from(SendWrapper::new(state.shard.list_all_clients()).await.len()) @@ -128,85 +139,50 @@ pub(in crate::http) async fn read_local( } } -/// Poll cadence while a metadata read waits for this node's applied frontier. -/// The recovery barrier's cadence and the binary read gate's: what the wait is -/// usually short of is a single commit broadcast. -const READ_FRONTIER_POLL: std::time::Duration = std::time::Duration::from_millis(10); - -/// Polls one held read is given before it fails retryable: 3s at the cadence -/// above. Matches the binary read gate's budget so both planes give up at the -/// same point, and stays far below the 30s the control-plane write path already -/// spends replaying a transient frame. -const READ_FRONTIER_MAX_POLLS: u32 = 300; - /// Hold a local metadata read until this node has applied everything the -/// presenting credential was told committed. +/// calling user was told committed. /// /// A committed control-plane reply hands the caller an op number; answering its /// next read from a state machine below that op contradicts the response it is /// holding. The lag is real on a node that is not the metadata primary: a /// healthy backup FORWARDS a `Register` to the primary /// (`dispatch::submit_register_local_or_forward`) and binds the committed epoch -/// while its own commit walk is still behind it - and a cluster without shared -/// bearer key material runs with HTTP forwarding off, so control-plane writes -/// stay on that backup instead of being relayed. +/// while its own commit walk is still behind it, so the caller holds an op that +/// node has not applied before it has issued a single write. That is the whole +/// window on a backup: a control-plane write posted there is either relayed to +/// the primary (forwarding on) or refused transient (forwarding off), so the +/// epoch is the only promise a backup makes on its own. /// /// Adjacent to `?consistency=linearizable`, not in competition with it. That /// asks for the freshest CLUSTER state and is answered by leaving this node /// (307 to the primary), which [`authorize_read`] decides before this wait and /// which this wait never sees. This gate makes an UNQUALIFIED read -/// read-your-writes for its own credential, at no redirect and no consensus -/// round trip. +/// read-your-writes for its own user, at no redirect and no consensus round +/// trip. Per user rather than per credential because a bearer is not stable: +/// a refreshed access token is a new credential for the same writer (see +/// [`crate::http::state::MetadataWatermarks`]). /// -/// Scope is this node's own view. A credential whose write this node relayed -/// over HTTP, or that wrote through a different node entirely, left no -/// watermark here; closing that needs the serving primary's commit op to reach -/// the reading node, which nothing in the response carries today. +/// Scope is this node's own view. A user whose write this node relayed over +/// HTTP, or who wrote through a different node entirely, left no floor here; +/// closing that needs the serving primary's commit op to reach the reading +/// node, which nothing in the response carries today. +/// +/// The wait itself is the binary plane's [`hold_for_frontier`]: woken by the +/// commit that advances the frontier, bounded by a `compio::time` timer like +/// the recovery barrier below, since this listener is pinned to shard 0's +/// compio thread and has no blocking pool to hand a wait to. Only this request +/// parks, and it parks on one wake rather than a timer per tick. async fn await_metadata_read_frontier( state: &HttpInner, identity: &Identity, -) -> Result<(), ReadError> { - let metadata = state.shard.plane.metadata(); +) -> Result { hold_for_frontier( - || metadata.applied_frontier(), - state.metadata_watermark(&identity.session_key), - READ_FRONTIER_MAX_POLLS, + state.shard.plane.metadata().applied_frontier(), + state.metadata_watermark(identity.user_id), + compio::time::sleep(READ_FRONTIER_BUDGET), ) .await -} - -/// Poll `frontier` for `watermark` up to `max_polls` times, then give up -/// retryable. Split from its call site so the fast path, the park, the catch-up -/// and the expiry are all testable without a live shard - the same reason -/// [`barrier_state`] is split out below. -/// -/// A caller with nothing to read back has `watermark == 0`, which the first -/// comparison satisfies: one `Acquire` load, no await, no allocation. Expiry is -/// loud and carries both numbers, so a frontier that stopped moving is visible -/// instead of showing up as latency. -async fn hold_for_frontier( - frontier: impl Fn() -> u64, - watermark: u64, - max_polls: u32, -) -> Result<(), ReadError> { - if frontier() >= watermark { - return Ok(()); - } - for _ in 0..max_polls { - // `compio::time::sleep` like the recovery barrier below: this listener - // is pinned to shard 0's compio thread, which has no blocking pool to - // hand a wait to. Only this request parks. - compio::time::sleep(READ_FRONTIER_POLL).await; - if frontier() >= watermark { - return Ok(()); - } - } - tracing::warn!( - frontier = frontier(), - watermark, - "metadata read frontier unreached past deadline; failing read with retryable 503" - ); - Err(ReadError::MetadataFrontierUnreached) + .map_err(|FrontierUnreached| ReadError::MetadataFrontierUnreached) } /// One recovery-barrier check's outcome, factored out of [`await_recovery_barrier`] @@ -253,7 +229,9 @@ const fn barrier_state(barrier: u64, commit_min: u64, expired: bool) -> BarrierW pub(in crate::http) async fn await_recovery_barrier( shard: &Rc, ) -> Result<(), ReadError> { - const POLL: std::time::Duration = std::time::Duration::from_millis(10); + // The consensus tick, like the read gate's cadence: what lifts this + // barrier is a commit walk, which advances on that clock. + const POLL: std::time::Duration = consensus::TICK_INTERVAL; let Some(consensus) = shard.plane.metadata().consensus.as_ref() else { return Ok(()); @@ -383,81 +361,83 @@ pub(in crate::http) fn authorize_data_plane( #[cfg(test)] mod tests { use super::{ - BarrierWait, READ_FRONTIER_MAX_POLLS, ReadError, barrier_state, hold_for_frontier, + BarrierWait, FrontierWait, ReadError, barrier_state, hold_for_frontier, read_needs_metadata_frontier, }; + use crate::http::state::MetadataWatermarks; use iggy_binary_protocol::codes::{ - DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE, GET_CONSUMER_GROUPS_CODE, - GET_PERSONAL_ACCESS_TOKENS_CODE, GET_STATS_CODE, GET_STREAM_CODE, GET_STREAMS_CODE, - GET_TOPIC_CODE, GET_TOPICS_CODE, GET_USER_CODE, GET_USERS_CODE, + DESCRIBE_OPTIONS_CODE, GET_CONSUMER_GROUPS_CODE, GET_PERSONAL_ACCESS_TOKENS_CODE, + GET_STATS_CODE, GET_STREAM_CODE, GET_STREAMS_CODE, GET_TOPIC_CODE, GET_TOPICS_CODE, + GET_USER_CODE, GET_USERS_CODE, }; - use std::cell::Cell; + use metadata::AppliedFrontier; + use std::future::pending; + use std::sync::Arc; - /// A caller with nothing to read back (`watermark == 0`) and one whose - /// watermark this node has already applied are the whole steady state, and - /// neither may cost a park: exactly one load, no await. A gate that polled - /// here would put 10ms on every REST read in the cluster. - #[compio::test] - async fn given_a_frontier_at_the_watermark_when_gating_should_serve_without_parking() { - for (frontier_value, watermark) in [(0, 0), (7, 7), (9, 7)] { - let loads = Cell::new(0u32); - let outcome = hold_for_frontier( - || { - loads.set(loads.get() + 1); - frontier_value - }, - watermark, - READ_FRONTIER_MAX_POLLS, - ) - .await; - assert!( - outcome.is_ok(), - "frontier {frontier_value} covers {watermark}" - ); - assert_eq!( - loads.get(), - 1, - "frontier {frontier_value} covers {watermark}, so the read must not poll" - ); - } - } + /// Root's user id, the caller every fixture below writes and reads as. + const USER: u32 = 0; - /// The gate's whole point: a read whose credential was told op 9 committed - /// is held, not answered, while this node is still at op 4 - and it is - /// answered as soon as the node catches up, rather than being failed. + /// The gate exactly as [`await_metadata_read_frontier`] composes it: the + /// per-user floor a committed control-plane reply left behind, against the + /// node-wide applied frontier. Whether a read is HELD is decided by those + /// two numbers and nothing else, so this is the plane's own contract: + /// seeded floor, read held, commit that closes the gap serves it. + /// + /// The budget is a future that never completes, so a gate that failed to + /// park would answer instead of hanging, and a gate that failed to wake + /// would hang instead of answering. The end-to-end REST path (a follower + /// that binds a forwarded epoch, then reads through axum) is + /// `integration::server::http_read_your_writes`; the race it depends on + /// cannot be forced from outside the process, which is why the hold is + /// pinned here. #[compio::test] - async fn given_a_frontier_behind_the_watermark_when_gating_should_hold_until_it_catches_up() { - const CATCH_UP_AFTER: u32 = 2; - let polls = Cell::new(0u32); - let outcome = hold_for_frontier( - || { - let seen = polls.get(); - polls.set(seen + 1); - if seen >= CATCH_UP_AFTER { 9 } else { 4 } - }, - 9, - 8, - ) - .await; + async fn given_a_recorded_floor_when_the_node_catches_up_should_serve_the_held_read() { + let watermarks = MetadataWatermarks::default(); + let frontier = Arc::new(AppliedFrontier::default()); + frontier.advance(4); - assert!( - outcome.is_ok(), - "the read must be served once the node caught up" + // A caller this node never wrote for waits for nothing. + assert_eq!( + hold_for_frontier(&frontier, watermarks.get(USER), pending()).await, + Ok(FrontierWait::Ready), + "an unseeded caller has no write to read back" ); - assert!( - polls.get() > CATCH_UP_AFTER, - "the read was answered off the lagging frontier after {} loads", - polls.get() + + // The committed reply's op becomes the floor, which is above what this + // node has applied: the read must not be answered yet. + watermarks.record(USER, 9); + let committer = Arc::clone(&frontier); + compio::runtime::spawn(async move { + compio::runtime::time::sleep(std::time::Duration::ZERO).await; + committer.advance(9); + }) + .detach(); + assert_eq!( + hold_for_frontier(&frontier, watermarks.get(USER), pending()).await, + Ok(FrontierWait::CaughtUp), + "the read must be held until the node applies the caller's own op" + ); + + // Caught up: back to the fast path, with the floor still in place. + assert_eq!( + hold_for_frontier(&frontier, watermarks.get(USER), pending()).await, + Ok(FrontierWait::Ready), + "an applied floor must not cost a park on every later read" ); } - /// A node can legitimately never catch up (a durably lagging replica), so - /// the park is bounded - and the exit is a retryable refusal, never the - /// stale answer. `MetadataFrontierUnreached` renders the shared 503 (see - /// `error.rs`). + /// The refusal this plane renders when the frontier never arrives: the + /// shared retryable 503, never a 2xx carrying the pre-write state. #[compio::test] - async fn given_a_frontier_that_never_catches_up_when_gating_should_fail_retryable() { - let outcome = hold_for_frontier(|| 4, 9, 3).await; + async fn given_a_floor_the_node_never_reaches_when_gating_should_render_the_retryable_503() { + let watermarks = MetadataWatermarks::default(); + watermarks.record(USER, 9); + let frontier = AppliedFrontier::default(); + frontier.advance(4); + + let outcome = hold_for_frontier(&frontier, watermarks.get(USER), std::future::ready(())) + .await + .map_err(|_| ReadError::MetadataFrontierUnreached); assert!( matches!(outcome, Err(ReadError::MetadataFrontierUnreached)), "an unreached frontier must refuse the read, not serve it" @@ -468,6 +448,11 @@ mod tests { /// pins what that list means for the codes HTTP actually serves: every /// entity read is gated, and the static option catalog is not. Forking the /// list per plane is what this is here to catch. + /// + /// `GET_CLUSTER_METADATA` is deliberately absent: `/cluster/metadata` has + /// its own local handler and never reaches [`read_local`], so asserting it + /// here would pin a code this plane cannot produce. The shared predicate's + /// own arm for it is covered where it is used, in the dispatch spine. #[test] fn given_the_http_read_codes_when_classified_should_gate_all_but_the_static_catalog() { for code in [ @@ -486,12 +471,10 @@ mod tests { "code {code} answers from the metadata STM and must be gated" ); } - for code in [DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE] { - assert!( - !read_needs_metadata_frontier(code), - "code {code} answers from a static catalog or the roster; holding it buys nothing" - ); - } + assert!( + !read_needs_metadata_frontier(DESCRIBE_OPTIONS_CODE), + "the option catalog is static; holding a read of it buys nothing" + ); } #[test] diff --git a/core/server/src/http/reply.rs b/core/server/src/http/reply.rs index 61a89f71e9..d44d18141b 100644 --- a/core/server/src/http/reply.rs +++ b/core/server/src/http/reply.rs @@ -38,6 +38,7 @@ use tracing::warn; use crate::dispatch::login_error::LoginRegisterError; use crate::http::error::{PartitionWriteError, WriteError}; +use crate::responses::reply_body; /// Discriminate a partition write reply. Partition replies carry no result /// section - a denial is empty-bodied and a committed body, where there is one, @@ -146,33 +147,6 @@ pub(in crate::http) fn committed_payload( } } -/// The transient variant of a reply-shaped pre-consensus rejection frame -/// (`[count=1][index=0][code]`, see `build_result_rejection_reply`), or `None` -/// for a committed outcome. Either transient means the op did not commit, so -/// the write path must replay the same request id rather than grade it as a -/// committed result or advance the session gate. The two codes are kept -/// distinct because they exhaust differently: `TransientNotAccepted` never -/// entered the pipeline and is safe to re-issue anywhere, while -/// `TransientNotCommitted` may still commit and only a same-session same-id -/// replay is safe. -pub fn transient_code(reply: &Message) -> Option { - match result_code(reply_body(reply)) { - Some(code) if code == IggyError::TransientNotCommitted.as_code() => { - Some(IggyError::TransientNotCommitted) - } - Some(code) if code == IggyError::TransientNotAccepted.as_code() => { - Some(IggyError::TransientNotAccepted) - } - _ => None, - } -} - -/// The reply body past the generic header, bounded by the header's `size`. -fn reply_body(reply: &Message) -> &[u8] { - let size = reply.header().size as usize; - reply.as_slice().get(HEADER_SIZE..size).unwrap_or_default() -} - /// Decode the `GetStreamResponse` payload of a committed create-stream reply into /// `StreamDetails`. `payload` is the slice past the result section that /// [`submit_write`] already validated as a success. @@ -271,7 +245,7 @@ mod tests { use crate::responses::{ NonReplicatedResponse, build_deny_reply, build_empty_reply, build_reply_from_bytes, - build_reply_with_body, + build_reply_with_body, transient_code, }; use crate::http::wire::build_request_message; diff --git a/core/server/src/http/session.rs b/core/server/src/http/session.rs index c0cdd21cf3..b645ce5fd7 100644 --- a/core/server/src/http/session.rs +++ b/core/server/src/http/session.rs @@ -125,34 +125,6 @@ pub(in crate::http) struct HttpSession { /// [`MAX_IN_FLIGHT_WRITES_PER_SESSION`]. Only [`InFlightWriteGuard`] /// touches it, so every admission is paired with exactly one release. pub(in crate::http) in_flight_writes: Cell, - /// Highest metadata op this credential has been told committed. - /// - /// Seeded from [`Self::session`] (the `Register` commit op, which floors - /// every metadata op that register could have observed) and raised by every - /// committed control-plane reply on this session. The read gate holds a - /// local read until the node's applied frontier covers it, so a caller - /// cannot be served state older than a write it already saw acked. - /// - /// Per-credential rather than per-request because that is the unit a - /// bearer's requests share; a caller presenting a fresh credential - /// re-seeds from the session it registers. A plain `Cell` suffices on - /// single-threaded shard 0, and it is never read across an `.await`. - pub(in crate::http) metadata_watermark: Cell, -} - -impl HttpSession { - /// Raise this session's metadata watermark to `commit`. Monotone, so a - /// reply that lands out of order (concurrent requests on one credential - /// are legal) cannot lower it. - /// - /// Only COMMITTED metadata replies belong here. A pre-consensus rejection - /// stamps the primary's `commit_max`, an op this caller was never promised - /// and, on a backup, one its own reads would then wait for; partition-plane - /// replies carry a different group's commit position entirely. - pub(in crate::http) fn record_metadata_watermark(&self, commit: u64) { - self.metadata_watermark - .set(self.metadata_watermark.get().max(commit)); - } } /// Serializes first-use VSR registration per credential key so a herd of @@ -287,18 +259,7 @@ mod tests { /// construction plus the live cancellation smoke, not faked here. #[compio::test] async fn detached_task_advances_gate_and_ignores_dead_receiver() { - let session = Rc::new(HttpSession { - key: "jwt:test".to_owned(), - client_id: 7, - session: 1, - user_id: DEFAULT_ROOT_USER_ID, - expiry: u64::MAX, - gate: Mutex::new(FIRST_REQUEST_ID), - data_gate: Mutex::new(FIRST_REQUEST_ID), - registry_token: Cell::new(None), - in_flight_writes: Cell::new(0), - metadata_watermark: Cell::new(1), - }); + let session = fake_session("jwt:test", 7, u64::MAX); let (result_slot, committed) = oneshot::channel::(); // The handler future dies (client disconnect) before the task runs. drop(committed); @@ -316,11 +277,6 @@ mod tests { assert_eq!(*session.gate.lock().await, FIRST_REQUEST_ID + 1); } - /// Register commit op every fixture binds. Non-zero on purpose: the read - /// gate's watermark seeds from it, so a fixture at zero could not tell a - /// seeded session from an unseeded one. - const FIXTURE_EPOCH: u64 = 1; - /// `InstanceToken` has no public constructor, so fixtures carry no reply /// target; the token-teardown branch of the sweep/forget helpers is /// exercised via their `Option` path, not fabricated here. @@ -328,14 +284,13 @@ mod tests { Rc::new(HttpSession { key: key.to_owned(), client_id, - session: FIXTURE_EPOCH, + session: 1, user_id: DEFAULT_ROOT_USER_ID, expiry, gate: Mutex::new(FIRST_REQUEST_ID), data_gate: Mutex::new(FIRST_REQUEST_ID), registry_token: Cell::new(None), in_flight_writes: Cell::new(0), - metadata_watermark: Cell::new(FIXTURE_EPOCH), }) } @@ -447,27 +402,4 @@ mod tests { "the pointer fence spares the re-registered session" ); } - - /// The mark is a floor the read gate waits for, so nothing may lower it: - /// two concurrent requests on one credential can have their committed - /// replies land out of order, and the later-but-lower reply must not undo - /// the earlier-but-higher one. The seed is the register's own commit op - /// (`session`), which floors every op that register could have observed. - #[test] - fn given_out_of_order_replies_when_recording_should_keep_the_watermark_monotone() { - let session = fake_session("jwt:a", 1, u64::MAX); - assert_eq!( - session.metadata_watermark.get(), - session.session, - "a fresh session starts at its register commit op" - ); - - session.record_metadata_watermark(50); - session.record_metadata_watermark(7); - assert_eq!( - session.metadata_watermark.get(), - 50, - "a lower commit must not lower the mark" - ); - } } diff --git a/core/server/src/http/state.rs b/core/server/src/http/state.rs index e1faa52523..e12692f736 100644 --- a/core/server/src/http/state.rs +++ b/core/server/src/http/state.rs @@ -57,6 +57,54 @@ use crate::shell::ServerShard; /// follower's possibly-stale one. pub(in crate::http) const VIEW_HEADER: HeaderName = HeaderName::from_static("iggy-view"); +/// Per-user read-your-writes floors: the highest metadata op each user has +/// been told committed BY THIS NODE. +/// +/// Keyed by user id, and held outside the session table, both deliberately. A +/// session entry is dropped outright when its VSR slot dies +/// ([`HttpInner::forget_session`]) or when the expiry sweep runs, and +/// `POST /users/refresh-token` answers with a fresh `jti` that registers no +/// session at all: a floor living in the session entry, or keyed by the +/// credential, reads `0` again in all three cases while the caller's bearer +/// stays valid - the stale read this exists to prevent, for exactly the +/// callers still holding a committed reply. Keying by user also bounds the +/// table by the user count instead of by every token ever minted. +/// +/// One user's floor is shared by its credentials, which is stronger than +/// read-your-writes and never weaker: the extra ops a second credential waits +/// for are the same user's. +#[derive(Debug, Default)] +pub(in crate::http) struct MetadataWatermarks(RefCell>); + +impl MetadataWatermarks { + /// Highest metadata op `user_id` was told committed here, or `0` when it + /// was told none - no write of its ever ran on this node, so there is + /// nothing to read back. + /// + /// Deliberately not expiry-filtered: the number is a consistency floor, + /// not a capability, and the request that consults it has already + /// re-verified the bearer. + /// + /// Confines the `RefCell` borrow to this call, so it can never span the + /// read gate's `.await`. + pub(in crate::http) fn get(&self, user_id: u32) -> u64 { + self.0.borrow().get(&user_id).copied().unwrap_or(0) + } + + /// Raise `user_id`'s floor to `commit`. Monotone, so a reply that lands + /// out of order (concurrent requests on one credential are legal) cannot + /// lower it. + /// + /// Only COMMITTED metadata replies belong here; see + /// [`crate::dispatch::submit::committed_reply_commit`] for what that + /// excludes and why. + pub(in crate::http) fn record(&self, user_id: u32, commit: u64) { + let mut floors = self.0.borrow_mut(); + let floor = floors.entry(user_id).or_insert(0); + *floor = (*floor).max(commit); + } +} + /// Axum router state: shard-0's [`HttpInner`] behind an `Rc`, `!Send` yet /// bridged into axum's `Send + Sync` requirement by `SendWrapper`. Sound /// because the listener and every handler run on shard 0's compio thread - the @@ -122,6 +170,10 @@ pub(in crate::http) struct HttpInner { /// Legacy-parity metric registry served by the scrape route; the router's /// counting layer holds a clone of its request counter. pub(in crate::http) metrics: HttpMetrics, + /// Per-user read-your-writes floors the read gate holds reads against. + /// Behind `Rc` because the write path records from a detached task that + /// outlives its handler by design (see `submit_committed`). + pub(in crate::http) metadata_watermarks: Rc, } impl HttpInner { @@ -229,23 +281,11 @@ impl HttpInner { } } - /// Highest metadata op the credential behind `key` has been told - /// committed, or `0` when this node has told it none - an unknown key, so - /// no write of its ever ran here and there is nothing to read back. - /// - /// Deliberately NOT expiry-filtered, unlike [`Self::live_session`]: the - /// number is a consistency floor, not a capability, and the request that - /// consults it has already re-verified the bearer. Dropping the floor - /// because a swept-but-still-present entry aged out would reintroduce the - /// stale read for exactly the callers still holding a committed reply. - /// - /// Confines the shared `RefCell` borrow to this call, so it can never span - /// the read gate's `.await`. - pub(in crate::http) fn metadata_watermark(&self, key: &str) -> u64 { - self.sessions - .borrow() - .get(key) - .map_or(0, |session| session.metadata_watermark.get()) + /// Highest metadata op `user_id` was told committed here; see + /// [`MetadataWatermarks`] for why the floor is per user and lives outside + /// the session table. + pub(in crate::http) fn metadata_watermark(&self, user_id: u32) -> u64 { + self.metadata_watermarks.get(user_id) } /// Clone the live (non-expired) entry for `key`, if present. Confines the @@ -379,12 +419,12 @@ impl HttpInner { ); return Err(AuthError::SessionIdTaken); } - // `bound.epoch` also seeds the read gate's watermark: a HEALTHY BACKUP - // forwards the register to the primary (see - // `submit_register_local_or_forward`), so this node can hand back an - // epoch its own commit walk has not reached, and the caller's first - // read would otherwise be served from state older than the register it - // is holding. + // `bound.epoch` also floors the read gate: a HEALTHY BACKUP forwards the + // register to the primary (see `submit_register_local_or_forward`), so + // this node can hand back an epoch its own commit walk has not + // reached, and the caller's first read would otherwise be served from + // state older than the register it is holding. + self.metadata_watermarks.record(user_id, bound.epoch); Ok(Rc::new(HttpSession { key, client_id, @@ -395,7 +435,6 @@ impl HttpInner { data_gate: Mutex::new(FIRST_REQUEST_ID), registry_token: Cell::new(None), in_flight_writes: Cell::new(0), - metadata_watermark: Cell::new(bound.epoch), })) } @@ -485,10 +524,43 @@ pub(in crate::http) fn insert_view_header(state: &HttpInner, mut response: Respo #[cfg(test)] mod tests { - use super::register_submit_auth_error; + use super::{MetadataWatermarks, register_submit_auth_error}; use crate::http::error::AuthError; use metadata::MetadataSubmitError; + /// The floor is what the read gate waits for, so nothing may lower it: two + /// concurrent requests by one user can have their committed replies land + /// out of order, and the later-but-lower reply must not undo the + /// earlier-but-higher one. + #[test] + fn given_out_of_order_replies_when_recording_should_keep_the_floor_monotone() { + const USER: u32 = 3; + + let watermarks = MetadataWatermarks::default(); + assert_eq!( + watermarks.get(USER), + 0, + "a user this node never wrote for was promised nothing" + ); + + watermarks.record(USER, 50); + watermarks.record(USER, 7); + assert_eq!( + watermarks.get(USER), + 50, + "a lower commit must not lower the floor" + ); + } + + /// One user's floor is not another's: a busy writer must not park an + /// unrelated user's reads behind ops it never issued. + #[test] + fn given_two_users_when_one_writes_should_leave_the_other_floor_alone() { + let watermarks = MetadataWatermarks::default(); + watermarks.record(1, 50); + assert_eq!(watermarks.get(2), 0); + } + #[test] fn register_submit_errors_preserve_known_and_unknown_outcomes() { for error in [ diff --git a/core/server/src/http/submit.rs b/core/server/src/http/submit.rs index 17f651b175..d82cf80eec 100644 --- a/core/server/src/http/submit.rs +++ b/core/server/src/http/submit.rs @@ -37,13 +37,12 @@ use crate::dispatch::session_ops::submit_logout_on_owner; use crate::dispatch::submit::{committed_reply_commit, submit_client_request_on_owner}; use crate::http::admission::admit_partition_write; use crate::http::error::{PartitionWriteError, WriteError}; -use crate::http::reply::{ - classify_partition_reply, committed_payload, eviction_error, transient_code, -}; +use crate::http::reply::{classify_partition_reply, committed_payload, eviction_error}; use crate::http::session::HttpSession; use crate::http::state::HttpInner; use crate::http::wire::build_request_message; use crate::pat::rewrite_pat_request_for_user; +use crate::responses::transient_code; use crate::shell::ServerShard; use crate::users::maybe_rewrite_user_password_request; use crate::wire::request_body; @@ -106,6 +105,7 @@ pub(in crate::http) async fn submit_committed( let (result_slot, committed) = oneshot::channel(); let shard = Rc::clone(&state.shard); let task_session = Rc::clone(session); + let watermarks = Rc::clone(&state.metadata_watermarks); let body = body.to_vec(); let max_tokens_per_user = state.max_tokens_per_user; // Detached so a client disconnect cannot abandon the gate mid-submit; @@ -115,13 +115,21 @@ pub(in crate::http) async fn submit_committed( submit_gated(&shard, &task_session, operation, max_tokens_per_user, &body).await; // Recorded here rather than after the await below, for the same reason // the submit is detached: a caller that disconnected mid-write still - // committed the op, and its next request on this credential must not be + // committed the op, and its next request as this user must not be // served state older than what committed. Ordered before the wake, so a // read issued the instant the response lands already sees the mark. + // + // This node's own view only: on a follower with HTTP forwarding ON the + // write is relayed to the primary by the middleware and never reaches + // this task, so the follower's floor stays where the register left it + // and its read-your-writes guarantee is the register epoch's. Closing + // that needs the serving primary's commit op to come back with the + // relayed response, which nothing in it carries today (see + // `reads::await_metadata_read_frontier`). if let Ok((_, reply, _)) = &result && let Some(commit) = committed_reply_commit(reply) { - task_session.record_metadata_watermark(commit); + watermarks.record(task_session.user_id, commit); } // A failed send means the handler died mid-await; the submit itself // already completed, which is the invariant that matters. diff --git a/core/server/src/lib.rs b/core/server/src/lib.rs index 1b1036f0ee..1481b2da2c 100644 --- a/core/server/src/lib.rs +++ b/core/server/src/lib.rs @@ -47,6 +47,11 @@ pub(crate) mod pat; pub(crate) mod responses; pub mod session_manager; pub mod shell; +/// The metadata read gate's budget, in consensus ticks. Re-exported for the +/// simulator's read-frontier spec, which spends part of the budget with +/// replication cut and has to know what is left; `dispatch` itself stays +/// crate-internal. +pub use dispatch::reads::READ_FRONTIER_BUDGET_TICKS as METADATA_READ_FRONTIER_BUDGET_TICKS; pub(crate) mod users; pub(crate) mod wire; diff --git a/core/server/src/responses.rs b/core/server/src/responses.rs index 4c48c0514b..3c1de8c84b 100644 --- a/core/server/src/responses.rs +++ b/core/server/src/responses.rs @@ -1542,6 +1542,45 @@ pub fn build_reply_from_bytes( ) } +/// The reply body past the generic header, bounded by the header's `size` +/// rather than by the buffer length: `size` is the frame's authoritative +/// extent, so a short frame reads as "no result section" instead of into +/// allocation padding. +#[must_use] +pub fn reply_body(reply: &Message) -> &[u8] { + let size = reply.header().size as usize; + reply + .as_slice() + .get(std::mem::size_of::()..size) + .unwrap_or_default() +} + +/// The transient variant of a reply-shaped pre-consensus rejection frame +/// (`[count=1][index=0][code]`, see `build_result_rejection_reply`), or `None` +/// for a committed outcome. Either transient means the op did not commit, so +/// the write path must replay the same request id rather than grade it as a +/// committed result or advance the session gate. The two codes are kept +/// distinct because they exhaust differently: `TransientNotAccepted` never +/// entered the pipeline and is safe to re-issue anywhere, while +/// `TransientNotCommitted` may still commit and only a same-session same-id +/// replay is safe. +/// +/// Lives here rather than in the HTTP reply module both planes' write paths +/// grade through: the dispatch spine needs it too, and importing it from +/// `http` would close a module cycle. +#[must_use] +pub fn transient_code(reply: &Message) -> Option { + match result_code(reply_body(reply)) { + Some(code) if code == IggyError::TransientNotCommitted.as_code() => { + Some(IggyError::TransientNotCommitted) + } + Some(code) if code == IggyError::TransientNotAccepted.as_code() => { + Some(IggyError::TransientNotAccepted) + } + _ => None, + } +} + /// If a raw PAT token was minted (`CreatePersonalAccessToken`) and the commit /// succeeded, replace the committed reply -- whose body is empty because the /// raw token never entered consensus -- with a `RawPersonalAccessTokenResponse`, @@ -1571,7 +1610,6 @@ pub fn build_raw_pat_reply( bytemuck::checked::try_from_bytes::(&committed.as_slice()[..header_len]) .map_err(|_| IggyError::InvalidFormat)?; let commit = committed_header.commit; - let size = committed_header.size as usize; // A `Reply` whose result section is nonzero is not a successful commit: // a committed business rejection (duplicate name, invalid expiry) or a // `TransientNotCommitted` retry frame, both with no payload and no token @@ -1579,15 +1617,7 @@ pub fn build_raw_pat_reply( // else through untouched so the client decodes the typed result (and, for // a transient, replays) instead of having a raw token grafted onto a // rejection body. Mirrors the HTTP handler's `committed_payload` gate. - // - // Bounded by the header's own `size` rather than running to the end of the - // buffer, so a short frame reads as "no result section" instead of into - // allocation padding. - let reply_body = committed - .as_slice() - .get(header_len..size) - .unwrap_or_default(); - if result_code(reply_body) != Some(0) { + if result_code(reply_body(&committed)) != Some(0) { return Ok(committed); } let token = WireName::new(raw.as_str()).map_err(|_| IggyError::InvalidFormat)?; diff --git a/core/server/src/session_manager.rs b/core/server/src/session_manager.rs index cdba5367df..32a5e49933 100644 --- a/core/server/src/session_manager.rs +++ b/core/server/src/session_manager.rs @@ -347,13 +347,17 @@ impl SessionManager { .map(|conn| conn.address) } - /// Acting user and transport peer address for a connection, in one map - /// lookup: the non-replicated dispatch path needs both, and the separate - /// accessors would walk the connection map twice per request. + /// Acting user, transport peer address and metadata watermark for a + /// connection, in one map lookup: the non-replicated dispatch path needs + /// all three per request, and the separate accessors would walk the + /// connection map (and take the shared borrow) once each. + /// + /// An unknown connection reads as a watermark of `0`: it was promised + /// nothing, so its reads wait for nothing. #[must_use] - pub fn read_context(&self, connection_id: u128) -> (Option, Option) { + pub fn read_context(&self, connection_id: u128) -> (Option, Option, u64) { let Some(conn) = self.connections.get(&connection_id) else { - return (None, None); + return (None, None, 0); }; let user_id = match conn.state { ConnectionState::Authenticated { user_id } | ConnectionState::Bound { user_id, .. } => { @@ -361,7 +365,7 @@ impl SessionManager { } ConnectionState::Connected => None, }; - (user_id, Some(conn.address)) + (user_id, Some(conn.address), conn.metadata_watermark) } /// Look up the authenticated user id for a connection. diff --git a/core/shard/src/lib.rs b/core/shard/src/lib.rs index fe4deaea5a..a8adfe0667 100644 --- a/core/shard/src/lib.rs +++ b/core/shard/src/lib.rs @@ -6744,13 +6744,13 @@ where self.note_metadata_transfer_progress(); self.metadata_transfer_decode_failures.set(None); if outcome.pairing_durable { - // `applied_frontier`, not the transferred snapshot's op: the install + // `installed_frontier`, not the transferred snapshot's op: the install // returns `max(snapshot_seq, local_applied)`, which differs whenever a // serving peer offers a snapshot BEHIND this replica (checkpoints are // node-local) and the local state machine is kept instead. tracing::info!( shard = self.id, - applied_frontier = outcome.applied_frontier, + installed_frontier = outcome.installed_frontier, commit_op, table_frontier, "metadata state transfer installed; handing tail to journal repair" @@ -6761,7 +6761,7 @@ where // let every one of them pass on the degraded path. tracing::warn!( shard = self.id, - applied_frontier = outcome.applied_frontier, + installed_frontier = outcome.installed_frontier, commit_op, table_frontier, "metadata state transfer landed WITHOUT a durable checkpoint \ diff --git a/core/simulator/src/client.rs b/core/simulator/src/client.rs index 191e523c5d..99fe11a7f4 100644 --- a/core/simulator/src/client.rs +++ b/core/simulator/src/client.rs @@ -601,35 +601,27 @@ impl SimClient { self.build_request_with_namespace(Operation::SendMessages, &buf, group) } - /// Build a `POLL_MESSAGES` request for an individual consumer, reading - /// `count` messages from offset 0 of `group`'s partition. + /// Build a `NonReplicated` request: `code` in the header's `reserved` + /// prefix, `group` as the routing namespace, `body` already encoded. /// - /// A `NonReplicated` read: the command code sits in the header's - /// `reserved` prefix, and the request id ECHOES the current counter - /// without advancing it (matching the SDK). The server ignores the id for - /// ops its `ClientTable` never sees, so burning one would buy nothing. - /// Requires a bound session (polls are auth-gated). + /// The request id ECHOES the current counter WITHOUT advancing it (matching + /// the SDK, and unlike [`Self::header`]): the server ignores the id for ops + /// its `ClientTable` never sees, so burning one would buy nothing - and + /// burning one here would desync every replicated request that follows. /// /// # Panics - /// Panics if the session is unbound or the request buffer is invalid. + /// Panics if the request buffer is invalid. #[allow(clippy::cast_possible_truncation)] - pub fn poll_messages(&self, group: IggyNamespace, count: u32) -> Message { - let (stream_id, topic_id, partition_id) = namespace_ids(group); - let body = PollMessagesRequest { - consumer: WireConsumer::consumer(WireIdentifier::Numeric(self.client_id as u32)), - stream_id, - topic_id, - partition_id, - strategy: WirePollingStrategy::first(), - count, - auto_commit: false, - } - .to_bytes(); - + fn non_replicated_request( + &self, + code: u32, + group: u64, + body: &[u8], + ) -> Message { let header_size = std::mem::size_of::(); let total_size = header_size + body.len(); let mut reserved = [0u8; 52]; - reserved[..4].copy_from_slice(&POLL_MESSAGES_CODE.to_le_bytes()); + reserved[..4].copy_from_slice(&code.to_le_bytes()); let header = RoutedRequestHeader { command: iggy_binary_protocol::Command::Request, operation: Operation::NonReplicated, @@ -638,57 +630,56 @@ impl SimClient { session: self.session_id(), request: self.request_counter.get(), reserved, - group: group.inner(), + group, ..Default::default() }; let mut buffer = Vec::with_capacity(total_size); buffer.extend_from_slice(bytemuck::bytes_of(&header)); - buffer.extend_from_slice(&body); + buffer.extend_from_slice(body); Message::try_from(Owned::<4096>::copy_from_slice(&buffer)) - .expect("poll request must be valid") + .expect("non-replicated request must be valid") + } + + /// Build a `POLL_MESSAGES` request for an individual consumer, reading + /// `count` messages from offset 0 of `group`'s partition. + /// + /// Requires a bound session (polls are auth-gated). + /// + /// # Panics + /// Panics if the session is unbound or the request buffer is invalid. + #[allow(clippy::cast_possible_truncation)] + pub fn poll_messages(&self, group: IggyNamespace, count: u32) -> Message { + let (stream_id, topic_id, partition_id) = namespace_ids(group); + let body = PollMessagesRequest { + consumer: WireConsumer::consumer(WireIdentifier::Numeric(self.client_id as u32)), + stream_id, + topic_id, + partition_id, + strategy: WirePollingStrategy::first(), + count, + auto_commit: false, + } + .to_bytes(); + self.non_replicated_request(POLL_MESSAGES_CODE, group.inner(), &body) } /// Build a `GET_STREAM` read for `name`. /// - /// A `NonReplicated` metadata read, so it is answered from whichever - /// replica's state machine the request lands on rather than routed to the - /// primary: the command code sits in the header's `reserved` prefix, the - /// group is the metadata sentinel, and the request id echoes the counter - /// without advancing it (matching [`Self::poll_messages`] and the SDK). - /// Requires a bound session, since the read is auth-gated. + /// A metadata read, so it is answered from whichever replica's state + /// machine the request lands on rather than routed to the primary; the + /// group is the metadata sentinel. Requires a bound session, since the read + /// is auth-gated. /// /// # Panics /// Panics if `name` is not a valid wire name or the request buffer is /// invalid. - #[allow(clippy::cast_possible_truncation)] pub fn get_stream(&self, name: &str) -> Message { let body = GetStreamRequest { stream_id: WireIdentifier::named(name).expect("stream name must be valid"), } .to_bytes(); - - let header_size = std::mem::size_of::(); - let total_size = header_size + body.len(); - let mut reserved = [0u8; 52]; - reserved[..4].copy_from_slice(&GET_STREAM_CODE.to_le_bytes()); - let header = RoutedRequestHeader { - command: iggy_binary_protocol::Command::Request, - operation: Operation::NonReplicated, - size: total_size as u32, - client: self.client_id, - session: self.session_id(), - request: self.request_counter.get(), - reserved, - group: METADATA_GROUP, - ..Default::default() - }; - - let mut buffer = Vec::with_capacity(total_size); - buffer.extend_from_slice(bytemuck::bytes_of(&header)); - buffer.extend_from_slice(&body); - Message::try_from(Owned::<4096>::copy_from_slice(&buffer)) - .expect("get stream request must be valid") + self.non_replicated_request(GET_STREAM_CODE, METADATA_GROUP, &body) } /// Store offset with explicit `AckLevel`. `NoAck` takes the primary's diff --git a/core/simulator/src/lib.rs b/core/simulator/src/lib.rs index 9e97cfaa0e..6167845e31 100644 --- a/core/simulator/src/lib.rs +++ b/core/simulator/src/lib.rs @@ -57,7 +57,7 @@ use std::collections::{HashMap, HashSet}; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::rc::Rc; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, AtomicU64}; +use std::sync::atomic::AtomicBool; /// Poll budget per [`DetExecutor::run_until_stalled`]. Pumps are event-driven, so /// hitting it means a task is spin-waking: a bug, panicked with the seed. @@ -398,7 +398,7 @@ impl Simulator { // One applied-metadata frontier per REPLICA, shared by its shards, // as the server bootstrap mints one per process. Volatile: a restart // below builds a fresh cell, matching a rebooted node. - let metadata_applied_frontier = Arc::new(AtomicU64::new(0)); + let metadata_applied_frontier = Arc::::default(); for shard_idx in 0..shards_per_replica { let inbox = inboxes[usize::from(shard_idx)] .take() @@ -1137,7 +1137,7 @@ impl Simulator { let mut stop_txs = Vec::with_capacity(usize::from(shards_per_replica)); let mut pump_tasks = Vec::with_capacity(usize::from(shards_per_replica)); let mut metadata_bundle: Option = None; - let metadata_applied_frontier = Arc::new(AtomicU64::new(0)); + let metadata_applied_frontier = Arc::::default(); for shard_idx in 0..shards_per_replica { let inbox = inboxes[usize::from(shard_idx)] .take() @@ -5150,12 +5150,18 @@ mod metadata_read_frontier_tests { /// couple of these; the gate must hold the read past all of them. /// /// Well under the gate's own poll budget, so expiry cannot masquerade as a - /// held read. + /// held read, and what is left of that budget is [`CONVERGE_STEPS`]. const STALE_WINDOW_STEPS: u32 = 50; - /// Steps allowed for repair to reach the backup and the held read to answer + /// Steps left for repair to reach the backup and the held read to answer /// once replication is restored. - const CONVERGE_STEPS: u32 = 2_000; + /// + /// Derived, not chosen: one `sim.step()` advances the virtual clock by one + /// consensus tick, the unit the gate's budget is denominated in, so phase 1 + /// spends `STALE_WINDOW_STEPS` of that budget and what remains is the whole + /// window the read can still be answered in. A larger number would just + /// spin past an expiry the status assertion below already caught. + const CONVERGE_STEPS: u32 = server::METADATA_READ_FRONTIER_BUDGET_TICKS - STALE_WINDOW_STEPS; /// The frames that would let the backup learn the committed writes. Journal /// repair and `StartView` adoption are cut with the same knife as live @@ -5350,7 +5356,8 @@ mod metadata_read_frontier_tests { ); } - let advanced = shards[0].plane.metadata().applied_frontier() + SHARED_FRONTIER_ADVANCE; + let advanced = + shards[0].plane.metadata().applied_frontier().get() + SHARED_FRONTIER_ADVANCE; shards[0] .plane .metadata() @@ -5358,7 +5365,7 @@ mod metadata_read_frontier_tests { for (shard_idx, shard) in shards.iter().enumerate() { assert_eq!( - shard.plane.metadata().applied_frontier(), + shard.plane.metadata().applied_frontier().get(), advanced, "shard {shard_idx} did not observe shard 0's advance: the \ applied-frontier cell is per shard, not per process, so every \ diff --git a/core/simulator/src/replica.rs b/core/simulator/src/replica.rs index 852ad5cec1..5132085fd9 100644 --- a/core/simulator/src/replica.rs +++ b/core/simulator/src/replica.rs @@ -30,7 +30,7 @@ use metadata::stm::mux::WithFactory; use metadata::stm::snapshot::RestoreSnapshot; use metadata::stm::stream::{Streams, StreamsInner}; use metadata::stm::user::{Users, UsersInner}; -use metadata::{IggyMetadata, apply_committed_prepare}; +use metadata::{AppliedFrontier, IggyMetadata, apply_committed_prepare}; use partitions::{IggyPartitions, PartitionPathLayout, PartitionsConfig}; use server::boot::wire_shell_handlers; use server::shell::{ShellHandlers, ShellShardHandle}; @@ -40,7 +40,6 @@ use shard::shards_table::PapayaShardsTable; use std::cell::RefCell; use std::rc::Rc; use std::sync::Arc; -use std::sync::atomic::AtomicU64; // TODO: Make configurable const CLUSTER_ID: u128 = 1; @@ -158,7 +157,7 @@ pub fn new_shard( incarnation: u128, data_dir: Option, seed_namespaces: &[(server_common::sharding::IggyNamespace, u32)], - applied_frontier: Arc, + applied_frontier: Arc, ) -> (Rc, Option) { // Metadata is single-writer, mirroring the server bootstrap. Shard 0 owns // the only writable STM; every peer shard rebuilds a reader-mode mirror from @@ -371,12 +370,8 @@ pub fn new_shard( ); } } - // Same seed the server bootstrap does after its own replay: the frontier - // resumes where the commit walk will, so a read on a rebuilt replica does - // not park until its deadline. No-op on peer shards, which share the cell. - if let Some(consensus) = metadata.consensus.as_ref() { - metadata.advance_applied_frontier(consensus.commit_min()); - } + // Same seed the server bootstrap runs after its own replay. + metadata.seed_applied_frontier_from_consensus(); // Mint the peers' read-side bundle AFTER reconstruction so it reflects the // recovered state. Shard 0 only; peers pass it back in as `reader_bundle`. let metadata_bundle = (shard_idx == 0).then(|| metadata.mux_stm.factory_bundle()); From 456c38a0146e68f2a56b4acae9a5abf263887f4d Mon Sep 17 00:00:00 2001 From: Grzegorz Koszyk Date: Thu, 3 Sep 2026 20:09:30 +0200 Subject: [PATCH 3/3] address review comments --- .../tests/server/http_read_your_writes.rs | 104 ++++++++-- core/metadata/src/applied_frontier.rs | 132 +++++++++++-- core/server/src/boot/mod.rs | 7 +- core/server/src/dispatch/mod.rs | 77 +++++++- core/server/src/dispatch/reads.rs | 184 ++++++++++++------ core/server/src/dispatch/submit.rs | 38 +--- core/server/src/http/forward.rs | 59 +++++- core/server/src/http/handlers.rs | 88 +++++---- core/server/src/http/reads.rs | 110 +++++++---- core/server/src/http/state.rs | 28 +++ core/server/src/lib.rs | 6 +- core/server/src/responses.rs | 82 +++++--- core/server/src/session_manager.rs | 39 ++++ core/shard/src/metrics.rs | 52 +++++ core/simulator/src/lib.rs | 123 +++++++++--- 15 files changed, 870 insertions(+), 259 deletions(-) diff --git a/core/integration/tests/server/http_read_your_writes.rs b/core/integration/tests/server/http_read_your_writes.rs index e9cfa68153..5fa54d65f8 100644 --- a/core/integration/tests/server/http_read_your_writes.rs +++ b/core/integration/tests/server/http_read_your_writes.rs @@ -39,20 +39,30 @@ //! session would be competing for VSR client ids with the fresh register each //! round mints, which is a different subject. //! -//! The suite asserts the GUARANTEE, not the mechanism: whether the gate parked -//! is invisible from outside, and on a fast local cluster the follower often -//! applies within the same tick. A pre-write answer is unambiguous though - a -//! 404, or a list missing the stream, can only happen if the floor was never -//! recorded, was recorded under the wrong key, was dropped with the session, or -//! the wait was skipped. A 503 fails the assertions too, on purpose: that is -//! what the gate answers when the follower never catches up inside its budget. -//! The park, the wake and the expiry themselves are pinned deterministically -//! next to the gate, in `dispatch::reads` and `metadata::applied_frontier`. +//! What each case can and cannot prove, stated plainly: +//! +//! - The wiring is proved deterministically. The forwarding case reads the +//! serving primary's applied op off the relayed write's `iggy-applied-op` +//! header and then off the follower's OWN answer to the following read: the +//! second can only be at or above the first if this node held that read until +//! its commit walk covered the op the primary handed the caller. That is the +//! invariant itself, in op numbers, not a proxy for it. +//! - Whether the gate actually PARKED is invisible from outside, and on a fast +//! local cluster the follower often applies within the same tick, so neither +//! case can force the park. The park, the wake and the expiry are pinned +//! deterministically next to the gate instead - `dispatch::reads`, +//! `metadata::applied_frontier`, and the simulator's +//! `metadata_read_frontier_tests`, which cuts replication to force the lag +//! the binary plane sees. +//! - A stale answer still fails loudly if the race does land: a 404, a list +//! missing the stream, or an applied op below the one the caller was handed. +//! A 503 fails too, on purpose - that is what the gate answers when the +//! follower never catches up inside its budget. use iggy::prelude::*; use integration::iggy_harness; -use reqwest::StatusCode; -use serde_json::Value; +use reqwest::{Response, StatusCode}; +use serde_json::{Value, json}; use crate::server::http_client::{ HttpClient, leader_and_follower, node_url, until_primary_resolved, @@ -63,6 +73,21 @@ use crate::server::http_client::{ /// different position relative to the epoch it just handed out. const ROUNDS: u32 = 4; +/// The serving node's applied metadata op, as stamped on every authenticated +/// success. A response without it is a contract violation, not an absence: the +/// relay records this value as the caller's read-your-writes floor, so a +/// missing header would silently reopen the stale read it exists to close. +fn applied_op(response: &Response) -> u64 { + response + .headers() + .get("iggy-applied-op") + .expect("every authenticated success carries iggy-applied-op") + .to_str() + .expect("iggy-applied-op must be ASCII") + .parse() + .expect("iggy-applied-op must be an op number") +} + /// The `name` of every stream in a `GET /streams` list body. fn stream_names(body: &Value) -> Vec { body.as_array() @@ -135,3 +160,60 @@ async fn given_a_follower_when_its_register_binds_a_committed_epoch_should_not_r ); } } + +/// The same three nodes WITH cluster-wide bearer key material, which switches +/// follower-to-primary forwarding on. That is the other half of the window and +/// the one with a deterministic proof: the write is relayed, so this follower +/// never runs the local write path and learns what the caller was told +/// committed only from the primary's `iggy-applied-op`. Its own answer to the +/// read that follows carries its own applied op, which must be at or above it. +#[iggy_harness( + cluster_nodes = 3, + server( + system.sharding.cpu_allocation = "0..1", + http.jwt.encoding_secret = "0123456789abcdef0123456789abcdef", + http.jwt.decoding_secret = "0123456789abcdef0123456789abcdef" + ) +)] +async fn given_a_forwarding_follower_when_it_relays_a_write_should_not_read_below_the_primary_op( + harness: &TestHarness, +) { + let (_leader, follower) = leader_and_follower(harness).await; + let http = HttpClient::login_root_no_redirect(node_url(harness, follower)).await; + + for round in 0..ROUNDS { + let stream = format!("relayed-read-your-writes-{round}"); + let create = json!({ "name": stream }); + let created = until_primary_resolved(|| http.post_json("/streams", &create)).await; + assert_eq!( + created.status(), + StatusCode::OK, + "the follower must relay the write and answer the primary's reply" + ); + // The primary applies a metadata op before it replies, so this is at or + // above the op the caller now holds. + let committed_at = applied_op(&created); + assert!( + committed_at > 0, + "the relayed reply carried no applied op, so this node recorded no floor" + ); + + let read = http.get("/streams").await; + assert_eq!( + read.status(), + StatusCode::OK, + "the follower must serve the read rather than refuse it" + ); + assert!( + applied_op(&read) >= committed_at, + "the follower answered a read at op {} after handing the caller {committed_at}: \ + the relayed floor was not recorded or not waited for", + applied_op(&read) + ); + let names = stream_names(&read.json().await.expect("the stream list is JSON")); + assert!( + names.contains(&stream), + "the follower listed streams without the one it had just relayed: {names:?}" + ); + } +} diff --git a/core/metadata/src/applied_frontier.rs b/core/metadata/src/applied_frontier.rs index ca5c1fa831..b759a3ff57 100644 --- a/core/metadata/src/applied_frontier.rs +++ b/core/metadata/src/applied_frontier.rs @@ -21,8 +21,9 @@ use std::future::Future; use std::pin::Pin; use std::sync::Mutex; use std::sync::PoisonError; -use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; use std::task::{Context, Poll, Waker}; +use std::time::Duration; /// Highest metadata op whose apply has been PUBLISHED on this node, shared by /// every shard, plus the wakers of the reads waiting for it to reach them. @@ -42,11 +43,29 @@ use std::task::{Context, Poll, Waker}; /// A `std::sync::Mutex` guards the waiter list, not a `tokio` one: it is taken /// and dropped inside [`Self::advance`] and inside one `poll`, never across an /// `.await`, and it has to be `Sync` because the writer is shard 0's thread -/// while the sleepers are on every shard. -#[derive(Debug, Default)] +/// while the sleepers are on every shard. `parked` keeps [`Self::advance`] off +/// that lock entirely in the normal case, where no read is waiting. +/// +/// Carries the read gates' budget too: it is config-derived (see the server's +/// `dispatch::reads`), and this cell is the one object minted before the shards +/// spawn that all of them can read, including peer shards with no consensus. +#[derive(Debug)] pub struct AppliedFrontier { op: AtomicU64, + /// Waits currently registered, so an advancing commit can skip the lock. + /// `Release` on the way in and `Acquire` on the way out, NOT relaxed: the + /// count is what tells [`Self::advance`] a waiter exists at all, so if it + /// reads zero it must be guaranteed that no registration it has to wake + /// happened before it. + parked: AtomicUsize, waiters: Mutex, + read_budget: Duration, +} + +impl Default for AppliedFrontier { + fn default() -> Self { + Self::new(Self::DEFAULT_READ_BUDGET) + } } /// Registered waits, keyed by an id so a re-poll can refresh its own waker and @@ -65,6 +84,36 @@ struct Waiter { } impl AppliedFrontier { + /// The budget a held read gets when nothing sizes it from config: six + /// commit broadcasts at the built-in `COMMIT_MESSAGE_TICKS` interval. + /// + /// The server always overrides this from `[cluster] + /// commit_broadcast_interval`, and a test asserts the two agree at the + /// config default; this is what the simulator and unit fixtures get. + pub const DEFAULT_READ_BUDGET: Duration = Duration::from_millis(3_000); + + /// A frontier at zero whose held reads get `read_budget` before they fail + /// retryable. + #[must_use] + pub const fn new(read_budget: Duration) -> Self { + Self { + op: AtomicU64::new(0), + parked: AtomicUsize::new(0), + waiters: Mutex::new(Waiters { + next_id: 0, + entries: Vec::new(), + }), + read_budget, + } + } + + /// How long a read may be held before it must fail retryable. Read by both + /// planes' gates, which arm their own timers with it. + #[must_use] + pub const fn read_budget(&self) -> Duration { + self.read_budget + } + /// Highest metadata op this NODE has applied and published. #[must_use] pub fn get(&self) -> u64 { @@ -81,14 +130,30 @@ impl AppliedFrontier { if self.op.fetch_max(op, Ordering::Release) >= op { return; } - let mut waiters = self.waiters.lock().unwrap_or_else(PoisonError::into_inner); - waiters.entries.retain(|waiter| { - if waiter.target > op { - return true; - } - waiter.waker.wake_by_ref(); - false - }); + // The normal case is a commit with nobody waiting on it, and this runs + // on the commit path: skip the lock rather than contend it per op. + if self.parked.load(Ordering::Acquire) == 0 { + return; + } + let woken = { + let mut waiters = self.waiters.lock().unwrap_or_else(PoisonError::into_inner); + let mut woken = Vec::new(); + waiters.entries.retain(|waiter| { + if waiter.target > op { + return true; + } + woken.push(waiter.waker.clone()); + false + }); + self.parked.store(waiters.entries.len(), Ordering::Release); + woken + }; + // Woken OUTSIDE the guard: each waker's task deregisters through this + // same mutex, so waking under it would hand every reader a lock the + // commit path is still holding. + for waker in woken { + waker.wake(); + } } /// A future that completes once the frontier covers `target`. @@ -136,17 +201,16 @@ impl AppliedFrontier { }); id }; + self.parked.store(waiters.entries.len(), Ordering::Release); drop(waiters); Registered::Waiting(id) } /// Drop the registration `id`, if it is still listed. fn deregister(&self, id: u64) { - self.waiters - .lock() - .unwrap_or_else(PoisonError::into_inner) - .entries - .retain(|waiter| waiter.id != id); + let mut waiters = self.waiters.lock().unwrap_or_else(PoisonError::into_inner); + waiters.entries.retain(|waiter| waiter.id != id); + self.parked.store(waiters.entries.len(), Ordering::Release); } /// Waits currently parked. For tests: a wait that outlives its future is a @@ -269,6 +333,30 @@ mod tests { assert_eq!(wait.as_mut().poll(&mut context), Poll::Ready(())); } + /// The commit path must not wake a reader while holding the lock that + /// reader needs to deregister, and it must not take that lock at all when + /// nothing is parked - a commit with no waiting read is the normal case. + #[test] + fn given_an_advance_when_waking_should_not_hold_the_waiter_lock() { + let frontier = Arc::new(AppliedFrontier::default()); + frontier.advance(4); + assert_eq!(frontier.waiting(), 0, "an advance with nobody parked"); + + // This waker re-enters the frontier's own lock, which is what a woken + // reader does when it deregisters. Waking under the guard therefore + // deadlocks this test outright rather than merely contending. + let waker = futures::task::waker(Arc::new(ReentrantWaker { + frontier: Arc::clone(&frontier), + })); + let mut context = Context::from_waker(&waker); + let mut wait = pin!(frontier.reached(9)); + assert_eq!(wait.as_mut().poll(&mut context), Poll::Pending); + + frontier.advance(9); + assert_eq!(frontier.waiting(), 0, "the woken wait is off the list"); + assert_eq!(wait.as_mut().poll(&mut context), Poll::Ready(())); + } + /// A re-poll must not stack a second registration, and a dropped wait must /// take its waker with it: an HTTP read is cancelled whenever its client /// disconnects mid-wait, and a leaked waker would be a leak per disconnect. @@ -286,6 +374,18 @@ mod tests { assert_eq!(frontier.waiting(), 0, "a dropped wait deregisters"); } + /// Stands in for a woken reader: waking takes the frontier's waiter lock, + /// exactly as the woken task's `deregister` does. + struct ReentrantWaker { + frontier: Arc, + } + + impl futures::task::ArcWake for ReentrantWaker { + fn wake_by_ref(arc_self: &Arc) { + let _parked = arc_self.frontier.waiting(); + } + } + struct FlagWaker { woken: Arc, } diff --git a/core/server/src/boot/mod.rs b/core/server/src/boot/mod.rs index f76d8e6f23..5053c8f8dc 100644 --- a/core/server/src/boot/mod.rs +++ b/core/server/src/boot/mod.rs @@ -56,6 +56,7 @@ use crate::boot::threads::{ }; use crate::boot::topology::{RosterCells, resolve_tcp_topology}; use crate::dispatch::partition::make_partition_read_handler; +use crate::dispatch::reads::read_frontier_budget; use crate::dispatch::session_ops::warm_dummy_password_hash; use crate::dispatch::submit::make_metadata_submit_handler; use crate::dispatch::{ @@ -307,8 +308,10 @@ pub fn bootstrap( // Shared applied-metadata frontier: shard 0's commit path advances it and // wakes the reads parked on it, every shard's read gate reads it. Minted // here, before any shard exists, because a shard holding a private cell - // would gate reads on a number nothing moves. - let metadata_applied_frontier = Arc::::default(); + // would gate reads on a number nothing moves - and it carries the held + // reads' budget, which is sized from the configured commit-broadcast + // cadence and which a peer shard has no other way to learn. + let metadata_applied_frontier = Arc::new(AppliedFrontier::new(read_frontier_budget(&config))); // Every shard's metric handles, minted before the threads spawn: each // shard bumps its own entry, and shard 0's HTTP scrape endpoint registers // the whole set (counters are Arc-backed, so cross-thread reads see the diff --git a/core/server/src/dispatch/mod.rs b/core/server/src/dispatch/mod.rs index 9e2267001d..68053050fa 100644 --- a/core/server/src/dispatch/mod.rs +++ b/core/server/src/dispatch/mod.rs @@ -94,6 +94,21 @@ use std::sync::Arc; use tracing::{debug, warn}; type ClientRequestQueues = Rc>>>>; + +/// Requests one client may have queued behind a request this shard has not +/// answered yet. +/// +/// The drain loop below serves one frame per client at a time, and a frame can +/// legitimately hold it for a while: a metadata write awaits consensus, and a +/// read can be HELD for the read-your-writes budget (see +/// `crate::dispatch::reads`). Without a cap, a client that keeps pipelining +/// through such a stall grows its queue - and this node's memory - unbounded. +/// +/// Overflow is ANSWERED, not dropped: `TransientNotAccepted` is the honest +/// code, since the frame provably never entered any pipeline, so the SDK may +/// re-issue it anywhere, including here once the queue drains. Sized far above +/// any SDK's in-flight window, so it only ever fires under a genuine stall. +const MAX_QUEUED_CLIENT_REQUESTS: usize = 1024; type ActiveClientRequests = Rc>>; pub fn make_client_request_handler( @@ -290,11 +305,18 @@ fn enqueue_client_request( S: 'static, SB: SuperblockStore + 'static, { - queues - .borrow_mut() - .entry(client_id) - .or_default() - .push_back(message); + { + let mut queues = queues.borrow_mut(); + let queue = queues.entry(client_id).or_default(); + if queue.len() >= MAX_QUEUED_CLIENT_REQUESTS { + // Borrow released before the deny, which spawns onto this same + // task and would otherwise re-enter the table. + drop(queues); + deny_overflowing_client_request(&shard, client_id, message); + return; + } + queue.push_back(message); + } if !active.borrow_mut().insert(client_id) { return; } @@ -314,6 +336,51 @@ fn enqueue_client_request( }); } +/// Answer a request that arrived with this client's queue already at +/// [`MAX_QUEUED_CLIENT_REQUESTS`] with the retryable transient denial. +/// +/// Spawned rather than awaited: the enqueue path is sync (it runs straight off +/// frame arrival) and the reply goes out on the bus. A frame whose header will +/// not even cast is dropped instead, exactly as the drain loop drops it. +fn deny_overflowing_client_request( + shard: &Rc>, + transport_client_id: u128, + message: Message, +) where + B: ShellBus, + MJ: JournalHandle + 'static, + MJ::Target: Journal, Header = PrepareHeader>, + S: 'static, + SB: SuperblockStore + 'static, +{ + shard.metrics().record_client_request_denied_queue_full(); + let Ok(request) = message.try_into_typed::() else { + warn!( + transport_client_id, + "dropping over-queue client request with invalid header" + ); + return; + }; + let request = request.into_routed(); + debug!( + transport_client_id, + operation = ?request.header().operation, + queued = MAX_QUEUED_CLIENT_REQUESTS, + "denying client request retryable: this connection's request queue is full" + ); + let shard = Rc::clone(shard); + let bus = shard.bus.clone(); + bus.spawn(async move { + send_deny_reply( + &shard, + transport_client_id, + request.header(), + IggyError::TransientNotAccepted.as_code(), + ) + .await; + }); +} + #[allow(clippy::future_not_send)] async fn drain_client_requests( shard: Rc>, diff --git a/core/server/src/dispatch/reads.rs b/core/server/src/dispatch/reads.rs index dc6fe9e38e..807b342b00 100644 --- a/core/server/src/dispatch/reads.rs +++ b/core/server/src/dispatch/reads.rs @@ -38,8 +38,8 @@ use crate::shell::{ShellBus, ShellShard}; use crate::snapshot; use crate::wire::request_body; use bytes::Bytes; -use configs::server::ServerSystemConfig; -use consensus::{MetadataHandle, TICK_INTERVAL, TimeoutManager}; +use configs::server::{ServerConfig, ServerSystemConfig}; +use consensus::MetadataHandle; use futures::future::{Either, select}; use iggy_binary_protocol::PrepareHeader; use iggy_binary_protocol::codes::{ @@ -128,50 +128,69 @@ async fn handle_get_me( .await; } -/// Budget one held read is given before it fails retryable, in consensus -/// ticks: six commit-broadcast intervals. +/// Commit broadcasts a held read waits out before it fails retryable. /// /// Sized for a node merely behind on its commit walk, NOT for a view change -- /// detecting one costs `heartbeat_timeout` and escalating it another /// `view_change_status_timeout`, and `recovery_barrier_deadline` budgets at /// least 15s for the same event, so a read that waits out an election is a read -/// the caller should retry elsewhere. Far below the SDK's 30s request budget, -/// inside which it replays the same id on the same connection. -/// -/// In ticks rather than a bare duration because that is the unit the thing -/// being waited for moves in, and the unit the simulator steps in. -#[allow(clippy::cast_possible_truncation)] -pub const READ_FRONTIER_BUDGET_TICKS: u32 = 6 * TimeoutManager::COMMIT_MESSAGE_TICKS as u32; +/// the caller should retry elsewhere. +const READ_FRONTIER_BROADCASTS: u32 = 6; -/// The same budget as a duration, for the timers the two planes measure it -/// with. -pub const READ_FRONTIER_BUDGET: Duration = - match TICK_INTERVAL.checked_mul(READ_FRONTIER_BUDGET_TICKS) { - Some(budget) => budget, - None => panic!("the read frontier budget must fit a Duration"), - }; +/// How long a held metadata read may wait for this node's applied frontier, +/// sized from `[cluster] commit_broadcast_interval`: the thing the read is +/// short of is a commit broadcast, so the budget has to move with the +/// configured cadence rather than with the compile-time default that +/// `TimeoutManager::COMMIT_MESSAGE_TICKS` names (the runtime overrides it +/// through `set_commit_message_ticks`). +/// +/// Minted once per process into the shared [`AppliedFrontier`], because a peer +/// shard's read gate has neither consensus nor the cluster config. +#[must_use] +pub fn read_frontier_budget(config: &ServerConfig) -> Duration { + config + .cluster + .commit_broadcast_interval + .get_duration() + // No config ceiling on the interval, so plain `*` can overflow. + .saturating_mul(READ_FRONTIER_BROADCASTS) +} /// Whether `code`'s answer comes from the metadata state machine, and so must /// not be served below the caller's watermark. /// -/// The two named exclusions only look like metadata reads: `DescribeOptions` -/// decodes a static catalog, and `GetClusterMetadata` answers from the -/// configured roster plus the consensus view. Holding either buys no -/// consistency, and the roster read is on the SDK's leader-discovery path, -/// where the wait would be real. A code this build does not know is excluded -/// too: its only outcome is `InvalidCommand`, and parking a terminal error for -/// the whole budget serves nobody. +/// The decision for both planes, consulted by every read path that CAN hold: +/// the binary spine's gated arms run it through [`authorize_and_hold_read`] and +/// the REST spine through `http::reads::gate_local_read`. The arms that never +/// consult it are exactly the codes named below, so this is the single list of +/// what is not gated: /// -/// A deny-list otherwise, so a read code added later is gated by default: the -/// failure mode of forgetting to add one is a wait, while forgetting to add it -/// to an allow-list is a silent stale read. +/// - `Ping` is the pre-auth liveness probe and reads nothing. +/// - `DescribeOptions` decodes a static catalog. +/// - `GetClusterMetadata` answers from the configured roster plus the +/// consensus view, and sits on the SDK's leader-discovery path, where the +/// wait would be real. +/// - `PollMessages` and `GetConsumerOffset` are partition-plane reads: their +/// answer comes from a partition group's own log, not the metadata STM, and +/// holding them would put metadata lag on the data path. +/// - `GetSnapshotFile` shells out to system tools off-thread; there is no +/// metadata answer to hold. +/// - A code this build does not know: its only outcome is `InvalidCommand`, +/// and parking a terminal error for the whole budget serves nobody. /// -/// Shared with the HTTP read path, which gates the identical set of command -/// codes through `build_non_replicated_response`: two lists would drift, and a -/// code dropped from one plane's list is a silent stale read on that plane. +/// A deny-list otherwise, so a read code added later is gated by default: the +/// failure mode of forgetting to name one here is a wait, while forgetting to +/// add it to an allow-list is a silent stale read. pub const fn read_needs_metadata_frontier(code: u32) -> bool { - !matches!(code, DESCRIBE_OPTIONS_CODE | GET_CLUSTER_METADATA_CODE) - && lookup_command(code).is_some() + !matches!( + code, + PING_CODE + | DESCRIBE_OPTIONS_CODE + | GET_CLUSTER_METADATA_CODE + | POLL_MESSAGES_CODE + | GET_CONSUMER_OFFSET_CODE + | GET_SNAPSHOT_FILE_CODE + ) && lookup_command(code).is_some() } /// Whether a frontier wait actually parked. @@ -221,15 +240,10 @@ pub async fn hold_for_frontier( let budget = pin!(budget); match select(reached, budget).await { Either::Left(((), _)) => Ok(FrontierWait::CaughtUp), - Either::Right(((), _)) => { - warn!( - frontier = frontier.get(), - watermark, - budget = ?READ_FRONTIER_BUDGET, - "metadata read frontier unreached inside the budget; failing the read retryable" - ); - Err(FrontierUnreached) - } + // Reported by the caller, not here: a durably lagging node refuses + // every held read of every client for as long as it lags, so this is + // a counter plus a `debug!`, never a line per refusal. + Either::Right(((), _)) => Err(FrontierUnreached), } } @@ -248,9 +262,12 @@ pub async fn hold_for_frontier( /// uncontended read shared-nothing. A park costs this connection more than the /// read itself: the per-connection drain loop serves one frame at a time, so /// the client's queued `SendMessages`, `PollMessages` and `PING` wait behind -/// the held read. No OTHER connection waits, and the budget above is what -/// bounds it. Expiry fails loud and retryable rather than serving state the -/// client already saw replaced. +/// the held read, and a client that keeps pipelining through the hold is +/// answered `TransientNotAccepted` once its queue hits +/// [`MAX_QUEUED_CLIENT_REQUESTS`](crate::dispatch::MAX_QUEUED_CLIENT_REQUESTS) +/// rather than growing it unbounded. No OTHER connection waits, and the budget +/// bounds the hold itself. Expiry fails loud and retryable rather than serving +/// state the client already saw replaced. /// /// The wait ends on the commit that closes the gap, not on a poll: the budget /// timer is the only timer armed, so a read that resumes costs one wake. @@ -266,17 +283,24 @@ where S: 'static, SB: SuperblockStore + 'static, { - hold_for_frontier( - shard.plane.metadata().applied_frontier(), - watermark, - shard.bus.sleep(READ_FRONTIER_BUDGET), - ) - .await - // `TransientNotAccepted`, not `NotCommitted`: a read never entered a - // pipeline, so it is safe to re-issue anywhere, and it is the code that - // drives the SDK's roster walk rather than a replay against the same - // durably lagging replica. - .map_err(|FrontierUnreached| IggyError::TransientNotAccepted) + let frontier = shard.plane.metadata().applied_frontier(); + let budget = frontier.read_budget(); + hold_for_frontier(frontier, watermark, shard.bus.sleep(budget)) + .await + // `TransientNotAccepted`, not `NotCommitted`: a read never entered a + // pipeline, so it is safe to re-issue anywhere, and it is the code that + // drives the SDK's roster walk rather than a replay against the same + // durably lagging replica. + .map_err(|FrontierUnreached| { + shard.metrics().record_metadata_read_frontier_refusal(); + debug!( + frontier = frontier.get(), + watermark, + ?budget, + "metadata read frontier unreached inside the budget; failing the read retryable" + ); + IggyError::TransientNotAccepted + }) } /// Authorize a metadata read, then hold it for this node's applied frontier. @@ -715,14 +739,46 @@ async fn handle_sync_consumer_group( #[cfg(test)] mod tests { - use super::{FrontierUnreached, FrontierWait, hold_for_frontier, read_needs_metadata_frontier}; + use super::{ + FrontierUnreached, FrontierWait, hold_for_frontier, read_frontier_budget, + read_needs_metadata_frontier, + }; + use configs::server::ServerConfig; use iggy_binary_protocol::codes::{ - DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE, GET_ME_CODE, GET_STREAM_CODE, + DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE, GET_CONSUMER_OFFSET_CODE, GET_ME_CODE, + GET_SNAPSHOT_FILE_CODE, GET_STREAM_CODE, PING_CODE, POLL_MESSAGES_CODE, SYNC_CONSUMER_GROUP_CODE, }; + use iggy_common::IggyDuration; use metadata::AppliedFrontier; use std::future::pending; use std::sync::Arc; + use std::time::Duration; + + /// The budget has to move with the CONFIGURED commit cadence, not with the + /// compile-time default: `[cluster] commit_broadcast_interval` is what + /// sizes the timer the read is waiting on, and a cluster that widens it to + /// 2s would otherwise get a budget of one and a half broadcasts and refuse + /// reads on a backup that is merely a commit behind. + /// + /// The default arm also pins the fallback the simulator and the unit + /// fixtures run on, so the two cannot drift apart silently. + #[test] + fn given_a_configured_commit_cadence_when_sizing_the_budget_should_scale_with_it() { + let mut config = ServerConfig::default(); + assert_eq!( + read_frontier_budget(&config), + AppliedFrontier::DEFAULT_READ_BUDGET, + "the config default must agree with the frontier's built-in fallback" + ); + + config.cluster.commit_broadcast_interval = IggyDuration::from(Duration::from_secs(2)); + assert_eq!( + read_frontier_budget(&config), + Duration::from_secs(12), + "six broadcasts of the configured interval" + ); + } /// A caller with nothing to read back (`watermark == 0`) and one whose /// watermark this node has already applied are the whole steady state, and @@ -806,10 +862,18 @@ mod tests { "code {code} answers from the metadata STM and must be gated" ); } - for code in [DESCRIBE_OPTIONS_CODE, GET_CLUSTER_METADATA_CODE] { + for code in [ + PING_CODE, + DESCRIBE_OPTIONS_CODE, + GET_CLUSTER_METADATA_CODE, + POLL_MESSAGES_CODE, + GET_CONSUMER_OFFSET_CODE, + GET_SNAPSHOT_FILE_CODE, + ] { assert!( !read_needs_metadata_frontier(code), - "code {code} answers from a static catalog or the roster; holding it buys nothing" + "code {code} has no metadata-STM answer to hold; the arms that skip \ + the gate are exactly these" ); } assert!( diff --git a/core/server/src/dispatch/submit.rs b/core/server/src/dispatch/submit.rs index 0350c12e3e..7176edf501 100644 --- a/core/server/src/dispatch/submit.rs +++ b/core/server/src/dispatch/submit.rs @@ -29,13 +29,10 @@ use crate::dispatch::session_ops::{ submit_register_local_or_forward, }; use crate::dispatch::upgrade_shard_handle; -use crate::responses::{reply_body, transient_code}; +use crate::responses::committed_reply_header; use crate::shell::{ShellBus, ShellShard, ShellShardHandle}; use consensus::MetadataHandle; -use iggy_binary_protocol::consensus::result_code; -use iggy_binary_protocol::{ - Command, GenericHeader, PrepareHeader, ReplyHeader, RoutedRequestHeader, -}; +use iggy_binary_protocol::{GenericHeader, PrepareHeader, RoutedRequestHeader}; use journal::superblock::SuperblockStore; use journal::{Journal, JournalHandle}; use server_common::Message; @@ -218,39 +215,14 @@ where /// grading both as no-promise is the only reading that cannot make a read /// wait for an op that never committed. /// -/// Every reply on this path is result-framed (`Operation::is_result_framed` -/// covers every metadata op; the partition plane grades through -/// `classify_partition_reply` instead), so a missing result section is a -/// malformed frame, not a bare payload. +/// The grading itself is [`committed_reply_header`], shared with the raw-PAT +/// splice, which must admit exactly the same frames. /// /// Shared with the HTTP write path, which grades the same frames off the same /// submit entry point ([`submit_client_request_on_owner`]); one classifier is /// what keeps the two planes' watermarks meaning the same thing. pub fn committed_reply_commit(reply: &Message) -> Option { - if reply.header().command != Command::Reply || transient_code(reply).is_some() { - return None; - } - let Some(bytes) = reply.as_slice().get(..size_of::()) else { - warn!( - size = reply.header().size, - "metadata reply shorter than its own header; not advancing the read watermark" - ); - return None; - }; - let header = match bytemuck::checked::try_from_bytes::(bytes) { - Ok(header) => header, - Err(error) => { - warn!( - ?error, - "metadata reply header failed to cast; not advancing the read watermark" - ); - return None; - } - }; - if header.status != 0 { - return None; - } - (result_code(reply_body(reply)) == Some(0)).then_some(header.commit) + committed_reply_header(reply).map(|header| header.commit) } #[cfg(test)] diff --git a/core/server/src/http/forward.rs b/core/server/src/http/forward.rs index fdfa2ea873..caf2aa8fa8 100644 --- a/core/server/src/http/forward.rs +++ b/core/server/src/http/forward.rs @@ -82,7 +82,7 @@ use crate::http::error::{ CustomError, error_response, gateway_timeout_response, primary_http_socket, with_retry_after, }; use crate::http::extractor::{bearer_token, resolve_credential}; -use crate::http::state::{ForwardState, HttpInner, VIEW_HEADER}; +use crate::http::state::{APPLIED_OP_HEADER, ForwardState, HttpInner, VIEW_HEADER}; use crate::server_error::ServerError; /// Marker stamped on every forwarded request. Loop guard only: a node that is @@ -130,10 +130,13 @@ const RESPONSE_CAPACITY_HINT: usize = 64 * 1024; /// Response headers copied from the primary's reply. Everything else is /// dropped, which subsumes the RFC 7230 hop-by-hop set: the relayed response /// is rebuilt, never streamed, so upstream `connection` / `transfer-encoding` -/// semantics cannot leak to the client. `iggy-view` is included so the -/// relayed response carries the serving primary's view, not this follower's -/// (the view layer only fills the header when absent). -const RELAYED_RESPONSE_HEADERS: [HeaderName; 3] = [CONTENT_TYPE, RETRY_AFTER, VIEW_HEADER]; +/// semantics cannot leak to the client. `iggy-view` and `iggy-applied-op` are +/// included so the relayed response carries the serving primary's view and +/// applied op, not this follower's (the response layer only fills either when +/// absent); the applied op is also what this node records as the caller's +/// read-your-writes floor, so dropping it here would reopen the stale read. +const RELAYED_RESPONSE_HEADERS: [HeaderName; 4] = + [CONTENT_TYPE, RETRY_AFTER, VIEW_HEADER, APPLIED_OP_HEADER]; /// Build the [`ForwardState`] at listener startup. /// @@ -268,9 +271,13 @@ async fn forward_or_pass(state: HttpState, request: Request, next: Next) -> Resp Ok(bearer) => bearer, Err(error) => return CustomError::from(error).into_response(), }; - if let Err(rejection) = resolve_credential(&state, bearer).await { - return rejection.into_response(); - } + // The user id is kept, not discarded: the relayed answer carries the + // primary's applied op, and this node has to record it as this caller's + // read-your-writes floor (see `record_relayed_floor`). + let user_id = match resolve_credential(&state, bearer).await { + Ok((_key, user_id, _expiry)) => user_id, + Err(rejection) => return rejection.into_response(), + }; let Some(_guard) = ForwardGuard::admit(&state.forward.in_flight) else { return with_retry_after(error_response( StatusCode::SERVICE_UNAVAILABLE, @@ -278,7 +285,41 @@ async fn forward_or_pass(state: HttpState, request: Request, next: Next) -> Resp "node is at its forward budget; retry with backoff", )); }; - forward(&state, request).await + let response = forward(&state, request).await; + record_relayed_floor(&state, user_id, &response); + response +} + +/// Record the serving primary's applied op as `user_id`'s read-your-writes +/// floor on THIS node. +/// +/// The relayed write ran on the primary, so the local write path never saw it +/// and left no floor behind, while the caller's next unqualified GET stays +/// local: without this, a `POST` followed by a `GET` through the same follower +/// can answer from before the write. Only a relayed SUCCESS counts - a 503 or a +/// 4xx promises the caller nothing - and the floor is monotone, so a slow relay +/// landing after a faster one cannot lower it. +/// +/// A missing or unparsable header is a no-op rather than a failure: it means +/// the peer is an older build, and a floor this node never learns is the +/// pre-existing behavior, not a new hazard. +fn record_relayed_floor(state: &HttpInner, user_id: u32, response: &Response) { + if !response.status().is_success() { + return; + } + let Some(applied) = response + .headers() + .get(APPLIED_OP_HEADER) + .and_then(|value| value.to_str().ok()) + .and_then(|value| value.parse::().ok()) + else { + debug!( + user_id, + "relayed response carried no applied op; the caller's floor stays where it was" + ); + return; + }; + state.metadata_watermarks.record(user_id, applied); } async fn forward_partition_or_pass(state: HttpState, request: Request, next: Next) -> Response { diff --git a/core/server/src/http/handlers.rs b/core/server/src/http/handlers.rs index c953edb7c0..2a082405b3 100644 --- a/core/server/src/http/handlers.rs +++ b/core/server/src/http/handlers.rs @@ -30,9 +30,10 @@ use axum::response::{IntoResponse, Response}; use chrono::Local; use consensus::{MetadataHandle, PartitionsHandle}; use iggy_binary_protocol::codes::{ - DESCRIBE_OPTIONS_CODE, GET_CONSUMER_GROUP_CODE, GET_CONSUMER_GROUPS_CODE, - GET_PERSONAL_ACCESS_TOKENS_CODE, GET_STATS_CODE, GET_STREAM_CODE, GET_STREAMS_CODE, - GET_TOPIC_CODE, GET_TOPICS_CODE, GET_USER_CODE, GET_USERS_CODE, + DESCRIBE_OPTIONS_CODE, GET_CLIENT_CODE, GET_CLIENTS_CODE, GET_CONSUMER_GROUP_CODE, + GET_CONSUMER_GROUPS_CODE, GET_CONSUMER_OFFSET_CODE, GET_PERSONAL_ACCESS_TOKENS_CODE, + GET_SNAPSHOT_FILE_CODE, GET_STATS_CODE, GET_STREAM_CODE, GET_STREAMS_CODE, GET_TOPIC_CODE, + GET_TOPICS_CODE, GET_USER_CODE, GET_USERS_CODE, POLL_MESSAGES_CODE, }; use iggy_binary_protocol::requests::consumer_groups::{ CreateConsumerGroupRequest, DeleteConsumerGroupRequest, GetConsumerGroupRequest, @@ -131,7 +132,7 @@ use crate::http::error::{ use crate::http::extractor::{Authenticated, Identity}; use crate::http::metrics::gauge_value; use crate::http::reads::{ - authorize_data_plane, authorize_read, read_local, resolve_gate_stream, resolve_gate_topic, + authorize_data_plane, gate_local_read, read_local, resolve_gate_stream, resolve_gate_topic, resolve_gate_topic_ids, resolve_gate_user, }; use crate::http::reply::{ @@ -675,19 +676,26 @@ pub(in crate::http) async fn get_metrics( /// `POST /snapshot`: collect a diagnostic archive and return it as a ZIP /// download with the same headers the legacy server sets. /// -/// Gated on the snapshot rule (`read_servers || manage_servers`) via the -/// shared [`authorize_read`] gate. Collection shells out to system tools on a -/// dedicated OS thread (see `snapshot::collect`); this handler only awaits the -/// result handoff, which is `Send`, so no `SendWrapper` bridge is needed. +/// Gated on the snapshot rule (`read_servers || manage_servers`) through the +/// shared [`gate_local_read`], which also serves it behind the post-restart +/// barrier; the archive itself carries no metadata answer to hold. Collection +/// shells out to system tools on a dedicated OS thread (see +/// `snapshot::collect`); this handler only awaits the result handoff, which is +/// `Send`, so no `SendWrapper` bridge is needed. pub(in crate::http) async fn get_snapshot( State(state): State, identity: Identity, Query(query): Query, Json(command): Json, ) -> Result<(HeaderMap, Body), ReadError> { - authorize_read(&state, &identity, query.consistency, |permissioner, uid| { - permissioner.get_snapshot(uid) - })?; + SendWrapper::new(gate_local_read( + &state, + &identity, + query.consistency, + GET_SNAPSHOT_FILE_CODE, + Permissioner::get_snapshot, + )) + .await?; let archive = snapshot::collect( Arc::clone(&state.system_config), command.compression, @@ -732,18 +740,23 @@ pub(in crate::http) async fn get_cluster_metadata( /// Unlike the entity reads, connections live in each shard's session manager, /// not the metadata STM, so this scatter-gathers over the shard mesh /// (`list_all_clients`) instead of going through [`read_local`]. It still runs -/// the identical per-op + consistency gate via [`authorize_read`], so its -/// authorization matches every metadata read. The gather future is `!Send`, -/// bridged onto shard 0's thread by `SendWrapper` exactly as the write path -/// bridges its submit. +/// the identical gates via [`gate_local_read`] - the consumer-group counts it +/// reports come off the streams STM, so its binary twin holds it for the read +/// frontier too. The gather future is `!Send`, bridged onto shard 0's thread by +/// `SendWrapper` exactly as the write path bridges its submit. pub(in crate::http) async fn get_clients( State(state): State, identity: Identity, Query(query): Query, ) -> Result>, ReadError> { - authorize_read(&state, &identity, query.consistency, |permissioner, uid| { - permissioner.get_clients(uid) - })?; + SendWrapper::new(gate_local_read( + &state, + &identity, + query.consistency, + GET_CLIENTS_CODE, + Permissioner::get_clients, + )) + .await?; let infos = SendWrapper::new(state.shard.list_all_clients()).await; let response = GetClientsResponse { clients: infos @@ -768,9 +781,14 @@ pub(in crate::http) async fn get_client( Path(client_id): Path, Query(query): Query, ) -> Result, ReadError> { - authorize_read(&state, &identity, query.consistency, |permissioner, uid| { - permissioner.get_client(uid) - })?; + SendWrapper::new(gate_local_read( + &state, + &identity, + query.consistency, + GET_CLIENT_CODE, + Permissioner::get_client, + )) + .await?; let infos = SendWrapper::new(state.shard.list_all_clients()).await; // The wire client id is the u32 seq tail of the u128 transport id. #[allow(clippy::cast_possible_truncation)] @@ -1221,17 +1239,19 @@ pub(in crate::http) async fn poll_messages( ) -> Result, ReadError> { let stream_id = Identifier::from_str_value(&stream_id).map_err(ReadError::Rejected)?; let topic_id = Identifier::from_str_value(&topic_id).map_err(ReadError::Rejected)?; - let scope = resolve_gate_topic_ids(&state, &stream_id, &topic_id); - authorize_read( + SendWrapper::new(gate_local_read( &state, &identity, consistency.consistency, + POLL_MESSAGES_CODE, |permissioner, uid| { - scope.map_or(Ok(()), |(stream_id, topic_id)| { - permissioner.poll_messages(uid, stream_id, topic_id) - }) + resolve_gate_topic_ids(&state, &stream_id, &topic_id) + .map_or(Ok(()), |(stream_id, topic_id)| { + permissioner.poll_messages(uid, stream_id, topic_id) + }) }, - )?; + )) + .await?; let wire = poll_wire_request(&stream_id, &topic_id, &query).map_err(ReadError::Rejected)?; let (namespace, partition_id, consumer, args) = match resolve_poll_request(&state.shard, &wire, HTTP_READ_CLIENT_ID) { @@ -1300,17 +1320,19 @@ pub(in crate::http) async fn get_consumer_offset( ) -> Result, ReadError> { let stream_id = Identifier::from_str_value(&stream_id).map_err(ReadError::Rejected)?; let topic_id = Identifier::from_str_value(&topic_id).map_err(ReadError::Rejected)?; - let scope = resolve_gate_topic_ids(&state, &stream_id, &topic_id); - authorize_read( + SendWrapper::new(gate_local_read( &state, &identity, consistency.consistency, + GET_CONSUMER_OFFSET_CODE, |permissioner, uid| { - scope.map_or(Ok(()), |(stream_id, topic_id)| { - permissioner.get_consumer_offset(uid, stream_id, topic_id) - }) + resolve_gate_topic_ids(&state, &stream_id, &topic_id) + .map_or(Ok(()), |(stream_id, topic_id)| { + permissioner.get_consumer_offset(uid, stream_id, topic_id) + }) }, - )?; + )) + .await?; let wire = consumer_offset_wire_request(&stream_id, &topic_id, &query).map_err(ReadError::Rejected)?; let (namespace, partition_id, consumer) = diff --git a/core/server/src/http/reads.rs b/core/server/src/http/reads.rs index 55188c9b9c..45e26e0f1d 100644 --- a/core/server/src/http/reads.rs +++ b/core/server/src/http/reads.rs @@ -22,8 +22,7 @@ //! routes ground their scopes through. use crate::dispatch::reads::{ - FrontierUnreached, FrontierWait, READ_FRONTIER_BUDGET, hold_for_frontier, - read_needs_metadata_frontier, + FrontierUnreached, FrontierWait, hold_for_frontier, read_needs_metadata_frontier, }; use crate::shell::ServerShard; use bytes::Bytes; @@ -44,21 +43,19 @@ use crate::responses::{ NonReplicatedResponse, build_non_replicated_response, resolve_stream_id, resolve_topic_id, }; -/// The two cross-cutting gates every authenticated read enforces before it -/// touches state. Factored out of [`read_local`] so the cross-shard client -/// reads (`get_clients` / `get_client`) - which serve from the shard session -/// managers, not the local STM, and so cannot use [`read_local`] - still pass -/// the identical gate. Keeping it in one place is what guarantees no read route -/// can silently skip authz or answer a linearizable request on a follower. -/// -/// Per-op RBAC: run the route's `rule` against the caller's committed -/// permissions via the live permissioner. A denial (always `Unauthorized`) -/// renders 403 through the legacy `IggyError -> status` map; root holds every -/// grant, so its reads pass without a user-id short-circuit. A linearizable -/// read must come from the primary; on a follower it redirects (307) to the -/// primary's HTTP address when resolvable, else fails closed to a 503 (see +/// The per-op RBAC + consistency check itself, without the waits: run the +/// route's `rule` against the caller's committed permissions via the live +/// permissioner. A denial (always `Unauthorized`) renders 403 through the +/// legacy `IggyError -> status` map; root holds every grant, so its reads pass +/// without a user-id short-circuit. A linearizable read must come from the +/// primary; on a follower it redirects (307) to the primary's HTTP address when +/// resolvable, else fails closed to a 503 (see /// [`HttpInner::not_primary_read_error`]). -pub(in crate::http) fn authorize_read( +/// +/// Every read route reaches this through [`gate_local_read`], which is what +/// pairs it with the two waits a local read must serve behind. Callable on its +/// own only for a read that is NOT served from local state. +fn authorize_read( state: &HttpInner, identity: &Identity, consistency: Consistency, @@ -99,24 +96,7 @@ pub(in crate::http) async fn read_local( body: &[u8], rule: impl Fn(&Permissioner, u32) -> Result<(), IggyError>, ) -> Result { - await_recovery_barrier(&state.shard).await?; - // Ahead of the frontier wait on purpose. `authorize_read` renders the - // linearizable follower redirect, which must answer 307 immediately - - // parking first would delay a request this node is not going to serve at - // all - and an authorization denial is terminal, so holding the connection - // for it buys nothing. - authorize_read(state, identity, consistency, &rule)?; - if read_needs_metadata_frontier(code) - && await_metadata_read_frontier(state, identity).await? == FrontierWait::CaughtUp - { - // Every scoped route's rule resolves its entity when the rule RUNS, - // and a park is precisely the case where the state machine moved under - // it: an entity that did not exist on the first pass resolved to - // nothing, where a scope miss is a pass-through, and would be served - // with no permissioner call at all. Only the parked outcome pays for - // the second pass. - authorize_read(state, identity, consistency, &rule)?; - } + gate_local_read(state, identity, consistency, code, rule).await?; let clients_count = if code == GET_STATS_CODE { u32::try_from(SendWrapper::new(state.shard.list_all_clients()).await.len()) .unwrap_or(u32::MAX) @@ -139,6 +119,48 @@ pub(in crate::http) async fn read_local( } } +/// Every gate a read served from THIS node's state has to pass, in the one +/// order that is safe. +/// +/// The chokepoint for the whole REST read surface: [`read_local`] runs it for +/// the metadata-STM entity reads, and the routes that cannot use `read_local` +/// call it directly - the cross-shard client reads, which serve from each +/// shard's session manager; the snapshot route, which shells out; and the +/// partition reads, which answer from a partition group's log. Skipping it is +/// how a route silently loses authorization, the post-restart barrier, or the +/// read-your-writes hold; which of the two waits actually applies is +/// [`read_needs_metadata_frontier`]'s decision, not the caller's. +/// +/// Order: +/// 1. the recovery barrier, so nothing is served off a WAL suffix that is +/// about to re-commit; +/// 2. authorization, because it is terminal: the linearizable follower +/// redirect must answer 307 immediately rather than after a park, and +/// holding a connection to then answer 403 buys nothing; +/// 3. the read-your-writes hold; +/// 4. authorization AGAIN if that hold actually parked. Every scoped route's +/// rule resolves its entity when the rule RUNS, and a park is precisely +/// the case where the state machine moved under it: an entity that did not +/// exist on the first pass resolved to nothing, where a scope miss is a +/// pass-through, and would be served with no permissioner call at all. Only +/// the parked outcome pays for the second pass. +pub(in crate::http) async fn gate_local_read( + state: &HttpInner, + identity: &Identity, + consistency: Consistency, + code: u32, + rule: impl Fn(&Permissioner, u32) -> Result<(), IggyError>, +) -> Result<(), ReadError> { + await_recovery_barrier(&state.shard).await?; + authorize_read(state, identity, consistency, &rule)?; + if read_needs_metadata_frontier(code) + && await_metadata_read_frontier(state, identity).await? == FrontierWait::CaughtUp + { + authorize_read(state, identity, consistency, &rule)?; + } + Ok(()) +} + /// Hold a local metadata read until this node has applied everything the /// calling user was told committed. /// @@ -176,13 +198,27 @@ async fn await_metadata_read_frontier( state: &HttpInner, identity: &Identity, ) -> Result { + let frontier = state.shard.plane.metadata().applied_frontier(); + let budget = frontier.read_budget(); hold_for_frontier( - state.shard.plane.metadata().applied_frontier(), + frontier, state.metadata_watermark(identity.user_id), - compio::time::sleep(READ_FRONTIER_BUDGET), + compio::time::sleep(budget), ) .await - .map_err(|FrontierUnreached| ReadError::MetadataFrontierUnreached) + .map_err(|FrontierUnreached| { + state + .shard + .metrics() + .record_metadata_read_frontier_refusal(); + tracing::debug!( + frontier = frontier.get(), + watermark = state.metadata_watermark(identity.user_id), + ?budget, + "metadata read frontier unreached inside the budget; failing read with retryable 503" + ); + ReadError::MetadataFrontierUnreached + }) } /// One recovery-barrier check's outcome, factored out of [`await_recovery_barrier`] diff --git a/core/server/src/http/state.rs b/core/server/src/http/state.rs index e12692f736..c55acfc661 100644 --- a/core/server/src/http/state.rs +++ b/core/server/src/http/state.rs @@ -57,6 +57,25 @@ use crate::shell::ServerShard; /// follower's possibly-stale one. pub(in crate::http) const VIEW_HEADER: HeaderName = HeaderName::from_static("iggy-view"); +/// Response header carrying the SERVING node's applied metadata op, stamped by +/// [`insert_view_header`] on the same responses as [`VIEW_HEADER`]. +/// +/// Load-bearing, not diagnostic: a follower that RELAYS a control-plane write +/// to the primary never runs the local write path, so nothing would record +/// what that caller was told committed, and its next unqualified GET - which +/// stays local - could answer from before its own write. The relay reads this +/// header off the primary's response and records it as the caller's floor (see +/// `http::forward`). Filled if absent, so a relayed response keeps the serving +/// node's number rather than the relaying follower's lower one. +/// +/// The op is a floor, not the caller's exact commit: the primary applies a +/// metadata op before it replies, so its applied frontier at reply time is at +/// or above the op the caller now holds. Above means waiting for a few of +/// someone else's committed ops too, which is stronger than read-your-writes +/// and never weaker. +pub(in crate::http) const APPLIED_OP_HEADER: HeaderName = + HeaderName::from_static("iggy-applied-op"); + /// Per-user read-your-writes floors: the highest metadata op each user has /// been told committed BY THIS NODE. /// @@ -519,6 +538,15 @@ pub(in crate::http) fn insert_view_header(state: &HttpInner, mut response: Respo .entry(VIEW_HEADER) .or_insert(HeaderValue::from(consensus.view())); } + // Same fill-if-absent rule, and for the same reason: the relay needs the + // op the SERVING node had applied, not this one's (see + // [`APPLIED_OP_HEADER`]). + response + .headers_mut() + .entry(APPLIED_OP_HEADER) + .or_insert(HeaderValue::from( + state.shard.plane.metadata().applied_frontier().get(), + )); response } diff --git a/core/server/src/lib.rs b/core/server/src/lib.rs index 1481b2da2c..b8131e558a 100644 --- a/core/server/src/lib.rs +++ b/core/server/src/lib.rs @@ -47,11 +47,7 @@ pub(crate) mod pat; pub(crate) mod responses; pub mod session_manager; pub mod shell; -/// The metadata read gate's budget, in consensus ticks. Re-exported for the -/// simulator's read-frontier spec, which spends part of the budget with -/// replication cut and has to know what is left; `dispatch` itself stays -/// crate-internal. -pub use dispatch::reads::READ_FRONTIER_BUDGET_TICKS as METADATA_READ_FRONTIER_BUDGET_TICKS; + pub(crate) mod users; pub(crate) mod wire; diff --git a/core/server/src/responses.rs b/core/server/src/responses.rs index 3c1de8c84b..75a8d2511b 100644 --- a/core/server/src/responses.rs +++ b/core/server/src/responses.rs @@ -103,6 +103,7 @@ use std::rc::Rc; use std::sync::{Arc, OnceLock}; use sysinfo::System as SysinfoSystem; use system_stats::SystemProbe; +use tracing::warn; /// Build the `get_me` reply for the requesting connection. Identity /// (`user_id`, transport kind, peer address) comes from the per-shard @@ -1555,6 +1556,55 @@ pub fn reply_body(reply: &Message) -> &[u8] { .unwrap_or_default() } +/// The header of a SUCCESSFULLY COMMITTED metadata reply, or `None` when the +/// frame promises the caller nothing. +/// +/// Three checks, in this order, and both callers need all three: +/// +/// - an eviction is an `EvictionHeader` whose bytes would cast cleanly as a +/// `ReplyHeader`, so the command is checked FIRST: casting it would both +/// swallow the eviction and grade it as a commit; +/// - a request-level denial names itself in `ReplyHeader.status`, the channel +/// the SDK peeks before body decode (see [`build_deny_reply`]); +/// - a nonzero result section is a rejection, transient or committed. Every +/// reply here is result-framed (`Operation::is_result_framed` covers the +/// metadata ops; the partition plane grades through +/// `classify_partition_reply` instead), so a missing section is a malformed +/// frame, not a bare payload. +/// +/// The read-your-writes floor and the raw-PAT splice both hang off exactly +/// this predicate - the floor must not advance on a frame that committed +/// nothing, and the token must not be grafted onto a rejection body - so they +/// share one implementation rather than two that have to stay in step. +/// +/// A frame too short to hold a header, or one whose header will not cast, is +/// `None` with a warning: it is malformed, and the alternative is a panic on +/// the reply path. +#[must_use] +pub fn committed_reply_header(reply: &Message) -> Option<&ReplyHeader> { + if reply.header().command != Command::Reply { + return None; + } + let Some(bytes) = reply.as_slice().get(..std::mem::size_of::()) else { + warn!( + size = reply.header().size, + "metadata reply shorter than its own header" + ); + return None; + }; + let header = match bytemuck::checked::try_from_bytes::(bytes) { + Ok(header) => header, + Err(error) => { + warn!(?error, "metadata reply header failed to cast"); + return None; + } + }; + if header.status != 0 || result_code(reply_body(reply)) != Some(0) { + return None; + } + Some(header) +} + /// The transient variant of a reply-shaped pre-consensus rejection frame /// (`[count=1][index=0][code]`, see `build_result_rejection_reply`), or `None` /// for a committed outcome. Either transient means the op did not commit, so @@ -1595,31 +1645,15 @@ pub fn build_raw_pat_reply( let Some(raw) = raw_token else { return Ok(committed); }; - // `submit_request_in_process` hands back an `EvictionHeader`-backed message - // on the evict outcome (e.g. a `CreatePersonalAccessToken` whose session - // was evicted between bind and request). Its byte pattern is a valid - // `ReplyHeader`, so the checked cast below would silently pass and we would - // both swallow the eviction and ship a raw token whose hash never - // committed. Only rewrite a genuine committed `Reply`; pass anything else - // (the eviction) through untouched so the client learns its session died. - if committed.header().command != Command::Reply { + // Only a genuine committed success gets the secret spliced in. An eviction + // frame (a `CreatePersonalAccessToken` whose session died between bind and + // request), a request-level denial, and a rejection result section all pass + // through untouched, so the client decodes the typed outcome - or, for a + // transient, replays - instead of having a raw token grafted onto a + // rejection body whose hash never committed. + let Some(commit) = committed_reply_header(&committed).map(|header| header.commit) else { return Ok(committed); - } - let header_len = std::mem::size_of::(); - let committed_header = - bytemuck::checked::try_from_bytes::(&committed.as_slice()[..header_len]) - .map_err(|_| IggyError::InvalidFormat)?; - let commit = committed_header.commit; - // A `Reply` whose result section is nonzero is not a successful commit: - // a committed business rejection (duplicate name, invalid expiry) or a - // `TransientNotCommitted` retry frame, both with no payload and no token - // to ship. Splice the secret only into a genuine success; pass everything - // else through untouched so the client decodes the typed result (and, for - // a transient, replays) instead of having a raw token grafted onto a - // rejection body. Mirrors the HTTP handler's `committed_payload` gate. - if result_code(reply_body(&committed)) != Some(0) { - return Ok(committed); - } + }; let token = WireName::new(raw.as_str()).map_err(|_| IggyError::InvalidFormat)?; let response = RawPersonalAccessTokenResponse { token }; let reply = build_result_framed_reply( diff --git a/core/server/src/session_manager.rs b/core/server/src/session_manager.rs index 32a5e49933..d09bd4c079 100644 --- a/core/server/src/session_manager.rs +++ b/core/server/src/session_manager.rs @@ -228,6 +228,13 @@ impl SessionManager { match conn.state { ConnectionState::Connected => { conn.state = ConnectionState::Authenticated { user_id }; + // The floor belongs to whoever was told those ops committed, + // and this socket now serves someone else: a `Connected` + // connection is either fresh or one `bind_session` demoted, so + // carrying the old mark over would make the new login wait for + // a write it never issued. Never the other direction - the + // bind below re-seeds from the register epoch. + conn.metadata_watermark = 0; Ok(()) } _ => Err(SessionError::InvalidTransition { @@ -685,6 +692,38 @@ mod tests { ); } + /// A socket that logs in again is serving a new caller, so it must not + /// inherit the floor of the one before it: the mark is what the PREVIOUS + /// login was told committed, and waiting for it would only ever delay the + /// new one. + #[test] + fn given_a_rebound_connection_when_it_logs_in_again_should_start_from_no_floor() { + let mut mgr = SessionManager::new(); + let conn = 1; + mgr.ensure_connection(conn, addr(5201), ClientTransportKind::Tcp); + mgr.login(conn, 3).unwrap(); + mgr.bind_session(conn, 100, 42).unwrap(); + mgr.record_metadata_watermark(conn, 50); + + // `bind_session` for the same client id on ANOTHER connection demotes + // this one to `Connected`, which is the state a re-login accepts. + mgr.ensure_connection(2, addr(5202), ClientTransportKind::Tcp); + mgr.login(2, 3).unwrap(); + mgr.bind_session(2, 100, 43).unwrap(); + assert_eq!( + mgr.metadata_watermark(conn), + 50, + "the demotion alone leaves the mark; the re-login is what clears it" + ); + + mgr.login(conn, 7).unwrap(); + assert_eq!( + mgr.metadata_watermark(conn), + 0, + "a different user on this socket was promised nothing" + ); + } + /// An unknown connection is not an error: the disconnect callback can win /// the race against a reply relay, and a gate reading `0` then serves the /// read instead of parking a socket that is already gone. diff --git a/core/shard/src/metrics.rs b/core/shard/src/metrics.rs index 4fbd8c7fcf..fcf885a097 100644 --- a/core/shard/src/metrics.rs +++ b/core/shard/src/metrics.rs @@ -208,6 +208,8 @@ pub struct ShardMetrics { partition_frames_rejected_ahead_total: Counter, partition_requests_denied_transient_total: Counter, partition_repair_serves_deferred_purge_total: Counter, + metadata_read_frontier_refusals_total: Counter, + client_requests_denied_queue_full_total: Counter, } impl ShardMetrics { @@ -232,9 +234,49 @@ impl ShardMetrics { partition_frames_rejected_ahead_total: Counter::default(), partition_requests_denied_transient_total: Counter::default(), partition_repair_serves_deferred_purge_total: Counter::default(), + metadata_read_frontier_refusals_total: Counter::default(), + client_requests_denied_queue_full_total: Counter::default(), } } + /// Bumped every time a client request is answered with a retryable denial + /// because that client already has the maximum number of requests queued + /// behind one the shard has not answered yet. + /// + /// The queue only grows while a client pipelines faster than its own + /// frames are served, so a sustained rate means one connection is stalled + /// on something - a held metadata read, a slow commit - while it keeps + /// sending. + pub fn record_client_request_denied_queue_full(&self) { + self.client_requests_denied_queue_full_total.inc(); + } + + /// Current value of [`Self::record_client_request_denied_queue_full`], for + /// tests that assert the denial was counted. + #[must_use] + pub fn client_requests_denied_queue_full_value(&self) -> u64 { + self.client_requests_denied_queue_full_total.get() + } + + /// Bumped every time a metadata read is refused because this node's + /// applied frontier never reached what the caller was told committed. + /// + /// The counter is the signal, not a log line: a node that lags durably + /// refuses every held read of every client for as long as it lags, so the + /// refusal itself logs at `debug!` and this is what a dashboard alerts on. + /// Any sustained rate means reads on this node are failing retryable while + /// its commit walk stays behind. + pub fn record_metadata_read_frontier_refusal(&self) { + self.metadata_read_frontier_refusals_total.inc(); + } + + /// Current value of [`Self::record_metadata_read_frontier_refusal`], for + /// tests that assert a refusal was counted rather than scraping it. + #[must_use] + pub fn metadata_read_frontier_refusals_value(&self) -> u64 { + self.metadata_read_frontier_refusals_total.get() + } + /// Increment `frame_drops_total{variant, reason}` by 1. /// /// Callers should pass label constants from [`frame_drop_variant`] @@ -496,6 +538,16 @@ impl ShardMetrics { "partition repair serves or completions deferred until a committed purge applies", self.partition_repair_serves_deferred_purge_total.clone(), ); + registry.register( + "metadata_read_frontier_refusals", + "metadata reads refused because this node never applied the caller's committed op", + self.metadata_read_frontier_refusals_total.clone(), + ); + registry.register( + "client_requests_denied_queue_full", + "client requests denied retryable because that client's request queue was full", + self.client_requests_denied_queue_full_total.clone(), + ); } } diff --git a/core/simulator/src/lib.rs b/core/simulator/src/lib.rs index 6167845e31..767f72196c 100644 --- a/core/simulator/src/lib.rs +++ b/core/simulator/src/lib.rs @@ -5145,23 +5145,33 @@ mod metadata_read_frontier_tests { /// for the whole run and is the node the client re-homes onto. const LAGGING: u8 = 1; + /// The gate's own budget, in `sim.step()`s: one step advances the virtual + /// clock by one consensus tick, so the tick count of the budget IS the step + /// count a held read survives. + /// + /// The simulator's replicas carry the frontier's built-in default, since + /// they are built without a `[cluster]` config to size it from. + #[allow(clippy::cast_possible_truncation)] + const BUDGET_STEPS: u32 = (metadata::AppliedFrontier::DEFAULT_READ_BUDGET.as_millis() + / shard::CONSENSUS_TICK_INTERVAL.as_millis()) as u32; + /// Steps the read is given while the backup is still cut off. A server that /// answers a metadata read from an unconverged state answers within a /// couple of these; the gate must hold the read past all of them. /// - /// Well under the gate's own poll budget, so expiry cannot masquerade as a - /// held read, and what is left of that budget is [`CONVERGE_STEPS`]. - const STALE_WINDOW_STEPS: u32 = 50; + /// A fifth of the budget, so expiry cannot masquerade as a held read and + /// the convergence phase below still has most of the budget left. + const STALE_WINDOW_STEPS: u32 = BUDGET_STEPS / 5; - /// Steps left for repair to reach the backup and the held read to answer - /// once replication is restored. + /// Steps the convergence phase spends waiting for the held read. /// - /// Derived, not chosen: one `sim.step()` advances the virtual clock by one - /// consensus tick, the unit the gate's budget is denominated in, so phase 1 - /// spends `STALE_WINDOW_STEPS` of that budget and what remains is the whole - /// window the read can still be answered in. A larger number would just - /// spin past an expiry the status assertion below already caught. - const CONVERGE_STEPS: u32 = server::METADATA_READ_FRONTIER_BUDGET_TICKS - STALE_WINDOW_STEPS; + /// Deliberately PAST the budget rather than exactly up to it: repair that + /// lands one tick late would otherwise flip this test onto the expiry path + /// and fail on the status assertion, which reads as "the gate is broken" + /// when it means "convergence was slow". Overshooting instead lets the + /// expired read be reported as what it is. Expiry has its own case, which + /// never restores replication at all. + const CONVERGE_STEPS: u32 = BUDGET_STEPS - STALE_WINDOW_STEPS + BUDGET_STEPS / 2; /// The frames that would let the backup learn the committed writes. Journal /// repair and `StartView` adoption are cut with the same knife as live @@ -5174,10 +5184,19 @@ mod metadata_read_frontier_tests { Command::StartView, ]; - /// A stream deleted before the client re-homed must not come back on the - /// backup that has not applied the delete yet. - #[test] - fn given_backup_behind_the_client_epoch_when_reading_a_deleted_stream_should_not_serve_it() { + /// Both cases below need the same shape: a stream created everywhere, then + /// deleted on a quorum that excludes `LAGGING` while the client re-homes + /// onto it, so the client holds a committed epoch above a delete that + /// backup has not applied. Returns the sim, the re-homed client, and the + /// op the delete committed at. + /// + /// Replication into `LAGGING` is left CUT: each case decides whether to + /// restore it. + fn backup_behind_a_deleted_stream( + seed: u64, + stream_name: &str, + client_id: u128, + ) -> (Simulator, SimClient, u64) { server_common::MemoryPool::init_pool(&server_common::MemoryPoolSettings { enabled: false, size: iggy_common::IggyByteSize::from(0u64), @@ -5185,12 +5204,10 @@ mod metadata_read_frontier_tests { }); let replica_count: u8 = 3; - let client_id: u128 = 1; - let stream_name = "read-your-writes"; let network_opts = packet::PacketSimulatorOptions { node_count: replica_count, client_count: 1, - seed: 0x1A7E_0F31, + seed, ..packet::PacketSimulatorOptions::default() }; let mut sim = Simulator::with_shards_shell( @@ -5204,7 +5221,7 @@ mod metadata_read_frontier_tests { sim.shell_login(&client); // The create lands on every replica: the backup has to HOLD the stream - // for the read below to be able to serve a stale one. + // for the read to be able to serve a stale one. let created = commit_write(&mut sim, client_id, 0, client.create_stream(stream_name)); step_until_applied(&mut sim, LAGGING, created); @@ -5217,7 +5234,7 @@ mod metadata_read_frontier_tests { let deleted = commit_write(&mut sim, client_id, 0, client.delete_stream(stream_name)); assert!( deleted > created, - "the delete must commit above the create, else the read below cannot \ + "the delete must commit above the create, else the read cannot \ distinguish the two states" ); @@ -5234,15 +5251,26 @@ mod metadata_read_frontier_tests { assert!( (created..deleted).contains(&lagging_commit), "the backup applied up to op {lagging_commit}, outside the window \ - [{created}, {deleted}) this test needs: it must hold the create and \ + [{created}, {deleted}) these tests need: it must hold the create and \ miss the delete" ); assert_eq!( read_stream_name_on(&sim, LAGGING, stream_name), Some(stream_name.to_string()), "the backup no longer holds the deleted stream, so a read cannot \ - serve a stale one and the assertions below prove nothing" + serve a stale one and the assertions prove nothing" ); + (sim, client, deleted) + } + + /// A stream deleted before the client re-homed must not come back on the + /// backup that has not applied the delete yet. + #[test] + fn given_backup_behind_the_client_epoch_when_reading_a_deleted_stream_should_not_serve_it() { + let stream_name = "read-your-writes"; + let client_id: u128 = 1; + let (mut sim, client, deleted) = + backup_behind_a_deleted_stream(0x1A7E_0F31, stream_name, client_id); let read = client.get_stream(stream_name); let request_id = read.header().request; @@ -5275,7 +5303,7 @@ mod metadata_read_frontier_tests { // Phase 2: restore replication. The held read must answer from the // converged state, which no longer holds the stream. set_replication(&mut sim, LAGGING, true); - for _ in 0..CONVERGE_STEPS { + for step in 0..CONVERGE_STEPS { if let Some(reply) = sim .step() .into_iter() @@ -5284,7 +5312,10 @@ mod metadata_read_frontier_tests { assert_eq!( reply.header().status, 0, - "the read failed instead of answering once the backup converged" + "the read was refused rather than answered {step} steps into a \ + restored link: the gate expired at its {BUDGET_STEPS}-step budget, \ + of which phase 1 spent {STALE_WINDOW_STEPS}, so repair was slower \ + than the budget rather than the gate being wrong" ); assert_eq!( read_stream_name(&reply), @@ -5301,6 +5332,50 @@ mod metadata_read_frontier_tests { ); } + /// The other half of the bound: a backup that NEVER catches up must refuse + /// the held read rather than serve the state the client already saw + /// replaced, and it must do so inside the budget rather than hanging. + /// + /// Same setup as the test above with replication left cut, so the only + /// possible outcomes are the refusal this asserts or a stale answer. + #[test] + fn given_a_backup_that_never_converges_when_reading_should_refuse_inside_the_budget() { + let stream_name = "read-your-writes-expiry"; + let client_id: u128 = 1; + let (mut sim, client, deleted) = + backup_behind_a_deleted_stream(0x1A7E_0F32, stream_name, client_id); + + let read = client.get_stream(stream_name); + let request_id = read.header().request; + sim.submit_request(client_id, LAGGING, read.into_generic()); + + // Overshoot the budget: the refusal must land inside it, and a read + // still unanswered after it is a hang, which the panic below names. + for _ in 0..(BUDGET_STEPS + BUDGET_STEPS / 2) { + if let Some(reply) = sim + .step() + .into_iter() + .find(|reply| reply.header().request == request_id) + { + assert_ne!( + reply.header().status, + 0, + "the cut-off backup answered a read below the client's committed \ + epoch ({deleted}) instead of refusing it: stream={:?}", + read_stream_name(&reply), + ); + assert_eq!(read_stream_name(&reply), None, "a refusal carries no body"); + return; + } + } + panic!( + "the held read neither answered nor expired within {} steps; backup applied \ + frontier {}, client epoch {deleted}", + BUDGET_STEPS + BUDGET_STEPS / 2, + metadata_commit(&sim, usize::from(LAGGING)), + ); + } + /// Shards per replica for the sharing test below. Two is the whole /// population that matters: shard 0 and one peer. const SHARED_FRONTIER_SHARDS: u16 = 2;