From 08c187293b660c11320e845f55ade32aab1a8107 Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Sat, 3 Oct 2026 13:33:22 -0400 Subject: [PATCH 01/12] fix(slatedb): correct time index for ListOffsets(Timestamp) Replace the per-partition `timestamps` map (keyed by base_timestamp instead of max_timestamp, and silently dropping batches whenever a new one's timestamp regressed) with a proper `t/` time index maintained by Kafka's own TimeIndex.maybeAppend rule: only a strictly-increasing max_timestamp is appended, floored at NO_TIMESTAMP (-1) so a negative timestamp is never indexed. ListOffsets(Timestamp) now does a ceiling range-scan of the index to find a starting offset, then a sequential scan of `b/` batches forward from there, since a batch's header max_timestamp can be stale after compaction (overstated, never understated) and the index itself can have a coverage gap below its first surviving entry once pruning has run (delete_records/policy_delete only prune `t/` entries; Kafka never hits this because its index is per-segment). policy_compact instead rebuilds the index from scratch from the surviving batches, replaying the monotonic rule, which is what Kafka's own LogCleaner does on segment compaction. A legacy (pre-time-index) watermark is detected by any postcard decode failure against the new (5-field) shape, and backfilled in place from `b/` batch headers the first time a write transaction sees it; until then, reads fall back to a slower but correct index-empty scan. ListOffsets(Latest) answers its timestamp from a new watermark field (the most recent batch's own max_timestamp) set unconditionally at every write site, rather than a batch_base_at_or_before binary search per call, since it's a consumer hot path. delete_topic gains the `t/` keyspace cleanup step this change needs (nothing wildcard-deletes by topic uuid, so a new keyspace leaks forever without one), built from a properly postcard-encoded prefix key rather than the raw-byte-vec pattern its existing (broken, filed separately) consumer-offset cleanup step uses. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01D2qdgPhMyLGZN5dLR8CVHs Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- nisshi-storage-slatedb/src/storage.rs | 686 ++++++++++++++++++++------ nisshi-storage-slatedb/src/tests.rs | 525 ++++++++++++++++++++ nisshi-storage-slatedb/src/types.rs | 103 ++++ 3 files changed, 1172 insertions(+), 142 deletions(-) diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index 39a8dbe7..1aa53463 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -63,8 +63,8 @@ use super::engine::Engine; use super::types::{ BatchKey, BatchKeyPrefix, BrokerInfo, Brokers, GroupDetailVersion, GroupKey, GroupKeyPrefix, OffsetCommitKey, OffsetCommitKeyPrefix, OffsetCommitValue, Producers, StoredScramCredential, - TopicMetadata, Topics, Transactions, Txn, TxnCommitOffset, TxnDetail, TxnProduceOffset, - UserScramCredentialKey, Watermark, WatermarkKey, + TimeIndexKey, TimeIndexKeyPrefix, TopicMetadata, Topics, Transactions, Txn, TxnCommitOffset, + TxnDetail, TxnProduceOffset, UserScramCredentialKey, Watermark, WatermarkKey, WatermarkLegacy, }; const CLEANUP_POLICY: &str = "cleanup.policy"; @@ -93,19 +93,414 @@ impl Engine { .is_some_and(|value| value.split(',').any(|candidate| candidate.trim() == policy)) } + /// Load a partition's watermark inside a write transaction, migrating a + /// legacy (pre-time-index) value in place. + /// + /// A postcard decode failure against the current (5-field) shape is + /// treated as "legacy 3-field bytes" (broadened past any specific error + /// kind - any shape mismatch means "not the current format"). On that + /// fallback, the time index is backfilled by replaying the monotonic + /// rule over every batch currently in the `b/` keyspace for this + /// partition (`Self::backfill_time_index`), rather than trusting the + /// old, lossy, wrongly-`base_timestamp`-keyed `timestamps` map. The + /// backfill's `t/` writes land in `tx`, so they commit atomically with + /// whatever write triggered this load. async fn partition_watermark( &self, tx: &slatedb::DbTransaction, - watermark_key: &[u8], + topic: Uuid, + partition: i32, ) -> Result { - tx.get(watermark_key) - .await - .map_err(Error::from) - .and_then(|watermark| { - watermark.map_or(Ok(Watermark::default()), |encoded| { - postcard::from_bytes(&encoded[..]).map_err(Into::into) - }) - }) + let watermark_key = postcard::to_stdvec(&WatermarkKey::new(topic, partition))?; + + let Some(encoded) = tx.get(&watermark_key).await.map_err(Error::from)? else { + return Ok(Watermark::default()); + }; + + match postcard::from_bytes::(&encoded) { + Ok(watermark) => Ok(watermark), + Err(_) => { + let legacy: WatermarkLegacy = postcard::from_bytes(&encoded)?; + let mut watermark = Watermark { + low: legacy.low, + high: legacy.high, + timestamps: None, + latest_indexed_timestamp: None, + last_batch_max_timestamp: None, + }; + self.backfill_time_index(tx, topic, partition, &mut watermark) + .await?; + Ok(watermark) + } + } + } + + /// Decode a stored watermark, tolerating the legacy 3-field shape. + /// + /// Used by read-only paths (`list_offsets`, `offset_stage`) that must + /// not perform the tx-based backfill in `Self::partition_watermark`. The + /// `bool` is `true` when the legacy fallback fired: callers use it to + /// skip any shortcut that trusts `latest_indexed_timestamp` as + /// authoritative, since a legacy watermark's index has not been + /// backfilled yet and so understates reality rather than reflecting it. + fn decode_watermark(encoded: &[u8]) -> Result<(Watermark, bool)> { + match postcard::from_bytes::(encoded) { + Ok(watermark) => Ok((watermark, false)), + Err(_) => { + let legacy: WatermarkLegacy = postcard::from_bytes(encoded)?; + Ok(( + Watermark { + low: legacy.low, + high: legacy.high, + timestamps: None, + latest_indexed_timestamp: None, + last_batch_max_timestamp: None, + }, + true, + )) + } + } + } + + /// Append `(max_timestamp, base_offset)` to the `t/` time index when + /// `max_timestamp` is a new high: Kafka's own `TimeIndex.maybeAppend`, + /// floored at `NO_TIMESTAMP = -1` so a negative timestamp is never + /// indexed (see [`TimeIndexKey`]). Always refreshes + /// `last_batch_max_timestamp` unconditionally, independent of the + /// monotonic check, so `ListOffsets(Latest)` can answer from the + /// watermark alone. + fn append_time_index( + tx: &slatedb::DbTransaction, + topic: Uuid, + partition: i32, + base_offset: i64, + max_timestamp: i64, + watermark: &mut Watermark, + ) -> Result<()> { + watermark.last_batch_max_timestamp = Some(max_timestamp); + + if max_timestamp > watermark.latest_indexed_timestamp.unwrap_or(-1) { + let key = postcard::to_stdvec(&TimeIndexKey::new(topic, partition, max_timestamp))?; + let value = postcard::to_stdvec(&base_offset)?; + tx.put(key, value)?; + watermark.latest_indexed_timestamp = Some(max_timestamp); + } + + Ok(()) + } + + /// Replay the monotonic rule (`Self::append_time_index`) over every + /// batch currently stored for `partition`, in offset order, rebuilding + /// the time index from `b/` batch headers only (no need to inflate + /// individual records). Used both to migrate a legacy watermark on + /// first write (`Self::partition_watermark`) and to rebuild the index + /// after compaction (`Self::policy_compact`), where pruning individual + /// `t/` entries would leave the coverage gap described in SOL-155074. + /// + /// Resets `latest_indexed_timestamp`/`last_batch_max_timestamp` on + /// `watermark` before replaying, so this is safe to call against a + /// `watermark` that already has entries in the index (the caller is + /// expected to have deleted them first via `Self::delete_time_index`). + async fn backfill_time_index( + &self, + tx: &slatedb::DbTransaction, + topic: Uuid, + partition: i32, + watermark: &mut Watermark, + ) -> Result<()> { + watermark.latest_indexed_timestamp = None; + watermark.last_batch_max_timestamp = None; + + let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; + let scan_start = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, 0))?; + + let mut scan = self.db.scan(scan_start..).await?; + while let Some(kv) = scan.next().await? { + if !kv.key.starts_with(&prefix) { + break; + } + + let key: BatchKey = postcard::from_bytes(&kv.key)?; + + // A batch whose header fails to decode is skipped, not a hard + // failure for the whole backfill: this replays history on a + // best-effort basis, and one unreadable batch should not take + // down every other partition's migration in the same write. + let Ok(batch) = self.decode(kv.value) else { + continue; + }; + + Self::append_time_index( + tx, + topic, + partition, + key.offset, + batch.max_timestamp, + watermark, + )?; + } + + Ok(()) + } + + /// Delete every `t/` entry for `partition`. Used before replaying the + /// index from scratch (`Self::backfill_time_index` via + /// `Self::policy_compact`) and when a topic is deleted + /// (`Self::delete_topic`). + async fn delete_time_index( + &self, + tx: &slatedb::DbTransaction, + topic: Uuid, + partition: i32, + ) -> Result<()> { + let prefix = postcard::to_stdvec(&TimeIndexKeyPrefix::new(topic, partition))?; + let scan_start = postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, 0))?; + + let mut scan = self.db.scan(scan_start..).await?; + while let Some(kv) = scan.next().await? { + if !kv.key.starts_with(&prefix) { + break; + } + tx.delete(&kv.key)?; + } + + Ok(()) + } + + /// Delete `t/` entries whose `base_offset` is below `low`. + /// + /// Entries are both timestamp- and offset-ordered within the scan (the + /// monotonic rule only ever appends an entry with both a higher + /// timestamp and a higher offset than every entry before it, regardless + /// of whether the data came from live production or a replay), so this + /// stops at the first surviving entry. Used by `delete_records`/ + /// `policy_delete` prefix pruning. Does NOT touch + /// `latest_indexed_timestamp`: Kafka's active segment is likewise + /// unaffected by deleting older segments. + async fn prune_time_index_below( + &self, + tx: &slatedb::DbTransaction, + topic: Uuid, + partition: i32, + low: i64, + ) -> Result<()> { + let prefix = postcard::to_stdvec(&TimeIndexKeyPrefix::new(topic, partition))?; + let scan_start = postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, 0))?; + + let mut scan = self.db.scan(scan_start..).await?; + while let Some(kv) = scan.next().await? { + if !kv.key.starts_with(&prefix) { + break; + } + + let base_offset: i64 = postcard::from_bytes(&kv.value)?; + if base_offset >= low { + break; + } + + tx.delete(&kv.key)?; + } + + Ok(()) + } + + /// `ListOffsets(Timestamp)`: the offset of the first record with an + /// absolute timestamp `>= target_millis`, per SOL-155074's lookup + /// algorithm: + /// + /// 1. `target_millis` beyond everything ever indexed means no match, + /// answered in O(1) - *unless* `legacy` is set, in which case the + /// index hasn't been backfilled yet and so cannot be trusted for + /// this shortcut (see `Self::decode_watermark`). + /// 2. Find where to start the sequential batch scan. The index's own + /// coverage can have a gap below its first surviving entry: pruning + /// deletes old `t/` entries but not the batches between the new + /// `low` and the first remaining entry (Kafka never hits this + /// because its index is per-segment, so deleting old segments can't + /// uncover anything). Start at `low` whenever the index can't rule + /// that gap out (empty, or its first entry is already `>= target`), + /// otherwise start at the ceiling entry's offset. + /// 3. Sequentially scan `b/` batches forward from there + /// (`Self::sequential_timestamp_scan`). + async fn list_offset_for_timestamp( + &self, + topic: Uuid, + partition: i32, + target_millis: i64, + watermark: &Watermark, + legacy: bool, + ) -> Result { + if !legacy && target_millis > watermark.latest_indexed_timestamp.unwrap_or(-1) { + return Ok(ListOffsetResponse { + error_code: ErrorCode::None, + offset: Some(0), + timestamp: None, + }); + } + + let index_prefix = postcard::to_stdvec(&TimeIndexKeyPrefix::new(topic, partition))?; + + let first_indexed = { + let from = postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, 0))?; + let mut scan = self.db.scan(from..).await?; + match scan.next().await? { + Some(kv) if kv.key.starts_with(&index_prefix) => { + Some(postcard::from_bytes::(&kv.key)?.timestamp) + } + _ => None, + } + }; + + let low = watermark.low.unwrap_or(0); + + let start_offset = match first_indexed { + Some(first_ts) if first_ts < target_millis => { + let from = + postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, target_millis))?; + let mut scan = self.db.scan(from..).await?; + match scan.next().await? { + Some(kv) if kv.key.starts_with(&index_prefix) => { + postcard::from_bytes(&kv.value)? + } + _ => low, + } + } + _ => low, + }; + + match self + .sequential_timestamp_scan(topic, partition, start_offset, target_millis) + .await? + { + Some((offset, ts)) => Ok(ListOffsetResponse { + error_code: ErrorCode::None, + offset: Some(offset), + timestamp: to_system_time(ts).ok(), + }), + None => Ok(ListOffsetResponse { + error_code: ErrorCode::None, + offset: Some(0), + timestamp: None, + }), + } + } + + /// Scan `b/` batches forward from `start_offset`, skipping a batch + /// whose header `max_timestamp` rules it out, and inspecting records + /// otherwise. A header can overstate - but never understate - the + /// true max after compaction leaves it stale (SOL-155074 change B), so + /// a qualifying header is a reason to look, not a match by itself: + /// skipping straight to a batch's own `base_offset` without checking + /// its records would be the exact bug this scan exists to avoid. + /// + /// A batch whose header fails to decode, or whose records fail to + /// inflate (corrupt/malformed data either way), is treated as + /// contributing nothing and skipped, rather than failing the whole + /// lookup: the primary thing a `ListOffsets` caller needs is the + /// `offset`, and a best-effort `timestamp` should not take that down. + async fn sequential_timestamp_scan( + &self, + topic: Uuid, + partition: i32, + start_offset: i64, + target_millis: i64, + ) -> Result> { + let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; + let from = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, start_offset))?; + + let mut scan = self.db.scan(from..).await?; + while let Some(kv) = scan.next().await? { + if !kv.key.starts_with(&prefix) { + break; + } + + let key: BatchKey = postcard::from_bytes(&kv.key)?; + + let Ok(deflated) = self.decode(kv.value) else { + continue; + }; + + if deflated.max_timestamp < target_millis { + continue; + } + + let Ok(inflated) = InflatedBatch::try_from(deflated.clone()) else { + continue; + }; + + if let Some(record) = inflated + .records + .iter() + .find(|record| deflated.base_timestamp + record.timestamp_delta >= target_millis) + { + return Ok(Some(( + key.offset + i64::from(record.offset_delta), + deflated.base_timestamp + record.timestamp_delta, + ))); + } + } + + Ok(None) + } + + /// The real timestamp of the first record at or after `offset` in + /// `partition`, or `None` if no batch covers it: an empty partition, or + /// a batch whose header fails to decode or whose records fail to + /// inflate (corrupt/malformed data; see `Self::sequential_timestamp_scan` + /// for why this degrades gracefully rather than failing the call). Used + /// for `ListOffsets(Earliest)`'s timestamp, decoupled from the old + /// (lossy, mis-keyed) `timestamps` map via the same + /// `batch_base_at_or_after`/`batch_base_at_or_before` helpers `fetch` + /// uses to locate the batch covering an arbitrary offset. + async fn timestamp_at_offset( + &self, + topic: Uuid, + partition: i32, + offset: i64, + ) -> Result> { + let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; + + let base = match self + .batch_base_at_or_after(&prefix, topic, partition, offset) + .await? + { + Some(base) if base == offset => Some(offset), + _ => { + self.batch_base_at_or_before(&prefix, topic, partition, offset) + .await? + } + }; + + let Some(base) = base else { + return Ok(None); + }; + + let from = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, base))?; + let mut scan = self.db.scan(from..).await?; + + let Some(kv) = scan.next().await? else { + return Ok(None); + }; + if !kv.key.starts_with(&prefix) { + return Ok(None); + } + + let Ok(mut deflated) = self.decode(kv.value) else { + return Ok(None); + }; + deflated.base_offset = base; + + let Ok(inflated) = InflatedBatch::try_from(deflated.clone()) else { + return Ok(None); + }; + + Ok(inflated + .records + .iter() + .find(|record| deflated.base_offset + i64::from(record.offset_delta) >= offset) + .and_then(|record| { + to_system_time(deflated.base_timestamp + record.timestamp_delta).ok() + })) } /// The least batch base offset at or after `probe` within a partition, @@ -220,19 +615,20 @@ impl Engine { continue; } - let watermark_key = - postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; - let mut watermark = self.partition_watermark(&tx, &watermark_key).await?; + let mut watermark = self + .partition_watermark(&tx, metadata.id, partition) + .await?; // When the partition is now empty the log start meets the // high watermark: offsets are never reused. let low = first_remaining.unwrap_or_else(|| watermark.high.unwrap_or(0)); watermark.low = Some(low); - if let Some(ref mut timestamps) = watermark.timestamps { - timestamps.retain(|_, offset| *offset >= low); - } + self.prune_time_index_below(&tx, metadata.id, partition, low) + .await?; + let watermark_key = + postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; tx.put(&watermark_key, postcard::to_stdvec(&watermark)?)?; _ = tx.commit().await.map_err(Error::from)?; @@ -337,19 +733,45 @@ impl Engine { continue; } - let watermark_key = - postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; - let mut watermark = self.partition_watermark(&tx, &watermark_key).await?; + let mut watermark = self + .partition_watermark(&tx, metadata.id, partition) + .await?; // Compaction leaves the log start where it is, as in Kafka, // whose cleaner never moves it: a fetch below the first // surviving batch steps over the gap to it, where moving the // log start would answer OFFSET_OUT_OF_RANGE and reset the // consumer. Only retention and DeleteRecords move it. - if let Some(ref mut timestamps) = watermark.timestamps { - timestamps.retain(|_, offset| !removed_offsets.contains(offset)); + + // Pruning individual `t/` entries here (as the old + // `timestamps`-map code did) can leave a coverage gap: a + // partially-compacted survivor keeps its original header + // verbatim (see `Builder::build`), which can now be stale, + // and any entry naming a since-deleted base_offset is + // simply wrong. Rebuild from scratch instead, replaying the + // monotonic rule over the survivors in offset order - the + // same full-rebuild Kafka's own LogCleaner does on segment + // compaction (SOL-155074 change D). + self.delete_time_index(&tx, metadata.id, partition).await?; + watermark.latest_indexed_timestamp = None; + watermark.last_batch_max_timestamp = None; + + for (_, offset, batch) in batches + .iter() + .filter(|(_, offset, _)| !removed_offsets.contains(offset)) + { + Self::append_time_index( + &tx, + metadata.id, + partition, + *offset, + batch.max_timestamp, + &mut watermark, + )?; } + let watermark_key = + postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; tx.put(&watermark_key, postcard::to_stdvec(&watermark)?)?; _ = tx.commit().await.map_err(Error::from)?; @@ -545,27 +967,28 @@ impl Storage for Engine { let new_low_watermark = partition.offset; // Update the watermark - let watermark_key = postcard::to_stdvec(&WatermarkKey::new( - metadata.id, - partition.partition_index, - ))?; - - let mut watermark = - tx.get(&watermark_key).await.map_err(Error::from).and_then( - |watermark| { - watermark.map_or(Ok(Watermark::default()), |encoded| { - postcard::from_bytes(&encoded[..]).map_err(Into::into) - }) - }, - )?; + let mut watermark = self + .partition_watermark(&tx, metadata.id, partition.partition_index) + .await?; watermark.low = Some(new_low_watermark); - // Remove timestamps before the new low watermark - if let Some(ref mut timestamps) = watermark.timestamps { - timestamps.retain(|_, offset| *offset >= new_low_watermark); - } + // Remove time index entries before the new low + // watermark (SOL-155074 change D: this is the + // simple prefix-pruning case, unlike compaction, + // and does not touch latest_indexed_timestamp). + self.prune_time_index_below( + &tx, + metadata.id, + partition.partition_index, + new_low_watermark, + ) + .await?; + let watermark_key = postcard::to_stdvec(&WatermarkKey::new( + metadata.id, + partition.partition_index, + ))?; let watermark_value = postcard::to_stdvec(&watermark)?; tx.put(&watermark_key, watermark_value)?; @@ -598,9 +1021,9 @@ impl Storage for Engine { /// Delete a topic from the cluster. /// - /// Deletes all associated data: batches, watermarks, consumer offsets, - /// producer sequences, and transaction data. This aligns with PG's - /// delete_topic implementation. + /// Deletes all associated data: batches, time index entries, watermarks, + /// consumer offsets, producer sequences, and transaction data. This + /// aligns with PG's delete_topic implementation. async fn delete_topic(&self, topic: &TopicId) -> Result { let tx = self .db @@ -643,14 +1066,23 @@ impl Storage for Engine { } } - // 2. Delete all watermarks for this topic + // 2. Delete all time index entries for this topic. This keyspace is + // enumerated explicitly like every other one here (nothing + // wildcard-deletes by topic uuid), so it needs its own step or it + // leaks forever (SOL-155074). + for partition in 0..topic_metadata.topic.num_partitions { + self.delete_time_index(&tx, topic_metadata.id, partition) + .await?; + } + + // 3. Delete all watermarks for this topic for partition in 0..topic_metadata.topic.num_partitions { let watermark_key = postcard::to_stdvec(&WatermarkKey::new(topic_metadata.id, partition))?; tx.delete(&watermark_key)?; } - // 3. Delete consumer offsets for this topic (scan all groups) + // 4. Delete consumer offsets for this topic (scan all groups) // Use just the prefix character 'c' to scan all consumer offsets let scan_start = vec![b'c']; let mut scan = self.db.scan(scan_start..).await?; @@ -667,7 +1099,7 @@ impl Storage for Engine { } } - // 4. Clean up producer sequences for this topic + // 5. Clean up producer sequences for this topic let mut producers: Producers = self.load_metadata(&tx, Self::PRODUCERS).await?; for producer_detail in producers.values_mut() { for epoch_sequences in producer_detail.sequences.values_mut() { @@ -676,7 +1108,7 @@ impl Storage for Engine { } self.save_metadata(&tx, Self::PRODUCERS, &producers)?; - // 5. Clean up transaction data for this topic + // 6. Clean up transaction data for this topic let mut transactions: Transactions = self.load_metadata(&tx, Self::TRANSACTIONS).await?; for txn in transactions.values_mut() { for txn_detail in txn.epochs.values_mut() { @@ -688,7 +1120,7 @@ impl Storage for Engine { } self.save_metadata(&tx, Self::TRANSACTIONS, &transactions)?; - // 6. Remove topic from metadata + // 7. Remove topic from metadata _ = topics.remove(&topic_name); self.save_metadata(&tx, Self::TOPICS, &topics)?; @@ -856,18 +1288,9 @@ impl Storage for Engine { self.save_metadata(&tx, Self::PRODUCERS, &producers)?; } - let mut watermark = tx - .get(postcard::to_stdvec(&WatermarkKey::new( - metadata.id, - topition.partition, - ))?) - .await - .map_err(Error::from) - .and_then(|watermark| { - watermark.map_or(Ok(Watermark::default()), |encoded| { - postcard::from_bytes(&encoded[..]).map_err(Into::into) - }) - })?; + let mut watermark = self + .partition_watermark(&tx, metadata.id, topition.partition) + .await?; let offset = watermark.high.unwrap_or_default(); let offset_end = offset + deflated.last_offset_delta as i64; @@ -910,10 +1333,14 @@ impl Storage for Engine { Some(high + deflated.last_offset_delta as i64 + 1i64) }); - _ = watermark - .timestamps - .get_or_insert_default() - .insert(deflated.base_timestamp, offset); + Self::append_time_index( + &tx, + metadata.id, + topition.partition, + offset, + deflated.max_timestamp, + &mut watermark, + )?; debug!(?watermark); @@ -1092,16 +1519,10 @@ impl Storage for Engine { let watermark_key = postcard::to_stdvec(&WatermarkKey::new(metadata.id, topition.partition))?; - let watermark = self - .db - .get(&watermark_key) - .await - .map_err(Error::from) - .and_then(|watermark| { - watermark.map_or(Ok(Watermark::default()), |encoded| { - postcard::from_bytes(&encoded[..]).map_err(Into::into) - }) - })?; + let watermark = match self.db.get(&watermark_key).await.map_err(Error::from)? { + Some(encoded) => Self::decode_watermark(&encoded)?.0, + None => Watermark::default(), + }; let high_watermark = watermark.high.unwrap_or(0); @@ -1313,27 +1734,20 @@ impl Storage for Engine { let watermark_key = postcard::to_stdvec(&WatermarkKey::new(metadata.id, topition.partition))?; - let watermark = self - .db - .get(&watermark_key) - .await - .map_err(Error::from) - .and_then(|watermark| { - watermark.map_or(Ok(Watermark::default()), |encoded| { - postcard::from_bytes(&encoded[..]).map_err(Into::into) - }) - })?; + let (watermark, legacy) = + match self.db.get(&watermark_key).await.map_err(Error::from)? { + Some(encoded) => Self::decode_watermark(&encoded)?, + None => (Watermark::default(), false), + }; let response = match list_offset { ListOffset::Earliest => { // The log start offset is the low watermark, advanced by // delete_records; timestamps below it are pruned there. let offset = watermark.low.unwrap_or(0); - let timestamp = watermark - .timestamps - .as_ref() - .and_then(|ts| ts.iter().find(|(_, off)| **off >= offset)) - .and_then(|(ts, _)| to_system_time(*ts).ok()); + let timestamp = self + .timestamp_at_offset(metadata.id, topition.partition, offset) + .await?; ListOffsetResponse { error_code: ErrorCode::None, @@ -1349,11 +1763,14 @@ impl Storage for Engine { } else { watermark.high.unwrap_or(0) }; + // The most recently appended batch's own max_timestamp, + // kept unconditionally on the watermark so this answers + // in O(1) rather than a batch_base_at_or_before binary + // search on every call (ListOffsets(Latest) is a + // consumer hot path - SOL-155074). let timestamp = watermark - .timestamps - .as_ref() - .and_then(|ts| ts.last_key_value()) - .and_then(|(ts, _)| to_system_time(*ts).ok()); + .last_batch_max_timestamp + .and_then(|ts| to_system_time(ts).ok()); ListOffsetResponse { error_code: ErrorCode::None, @@ -1362,33 +1779,20 @@ impl Storage for Engine { } } ListOffset::Timestamp(target_ts) => { - // Find the first offset with timestamp >= target // target_ts is SystemTime, need to convert to i64 for comparison let target_millis = target_ts .duration_since(SystemTime::UNIX_EPOCH) .map(|d| d.as_millis() as i64) .unwrap_or(0); - let result = watermark.timestamps.as_ref().and_then(|ts| { - ts.range(target_millis..) - .next() - .map(|(ts, off)| (*off, *ts)) - }); - - match result { - Some((offset, ts)) => ListOffsetResponse { - error_code: ErrorCode::None, - offset: Some(offset), - timestamp: to_system_time(ts).ok(), - }, - // No record at or after the target: no offset (see - // `ListOffsetResponse::offset`). - None => ListOffsetResponse { - error_code: ErrorCode::None, - offset: None, - timestamp: None, - }, - } + self.list_offset_for_timestamp( + metadata.id, + topition.partition, + target_millis, + &watermark, + legacy, + ) + .await? } }; @@ -1972,16 +2376,9 @@ impl Storage for Engine { .and_then(TryInto::try_into)?; // Get current watermark and increment it - let watermark_key = - postcard::to_stdvec(&WatermarkKey::new(metadata.id, *partition))?; - let mut watermark = - tx.get(&watermark_key).await.map_err(Error::from).and_then( - |watermark| { - watermark.map_or(Ok(Watermark::default()), |encoded| { - postcard::from_bytes(&encoded[..]).map_err(Into::into) - }) - }, - )?; + let mut watermark = self + .partition_watermark(&tx, metadata.id, *partition) + .await?; let offset = watermark.high.unwrap_or_default(); @@ -1991,10 +2388,14 @@ impl Storage for Engine { Some(high + batch.last_offset_delta as i64 + 1i64) }); - _ = watermark - .timestamps - .get_or_insert_default() - .insert(batch.base_timestamp, offset); + Self::append_time_index( + &tx, + metadata.id, + *partition, + offset, + batch.max_timestamp, + &mut watermark, + )?; // Encode and store the batch let encoded = { @@ -2011,6 +2412,8 @@ impl Storage for Engine { tx.put(batch_key, &encoded[..])?; // Save updated watermark + let watermark_key = + postcard::to_stdvec(&WatermarkKey::new(metadata.id, *partition))?; let watermark_value = postcard::to_stdvec(&watermark)?; tx.put(watermark_key, watermark_value)?; } @@ -2541,17 +2944,9 @@ impl Storage for Engine { .and_then(TryInto::try_into)?; // Get current watermark and increment it - let watermark_key = - postcard::to_stdvec(&WatermarkKey::new(metadata.id, *partition))?; - let mut watermark = - tx.get(&watermark_key) - .await - .map_err(Error::from) - .and_then(|watermark| { - watermark.map_or(Ok(Watermark::default()), |encoded| { - postcard::from_bytes(&encoded[..]).map_err(Into::into) - }) - })?; + let mut watermark = self + .partition_watermark(&tx, metadata.id, *partition) + .await?; let offset = watermark.high.unwrap_or_default(); @@ -2564,10 +2959,14 @@ impl Storage for Engine { Some(high + batch.last_offset_delta as i64 + 1i64) }); - _ = watermark - .timestamps - .get_or_insert_default() - .insert(batch.base_timestamp, offset); + Self::append_time_index( + &tx, + metadata.id, + *partition, + offset, + batch.max_timestamp, + &mut watermark, + )?; // Encode and store the batch let encoded = { @@ -2580,6 +2979,9 @@ impl Storage for Engine { postcard::to_stdvec(&BatchKey::new(metadata.id, topition.partition, offset))?; tx.put(batch_key, &encoded[..])?; + let watermark_key = + postcard::to_stdvec(&WatermarkKey::new(metadata.id, *partition))?; + // Save updated watermark let watermark_value = postcard::to_stdvec(&watermark)?; tx.put(watermark_key, watermark_value)?; diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index bfb31c0b..76861583 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -1303,6 +1303,531 @@ mod cleanup_policy { } } +// ========== Time Index Tests (SOL-155074) ========== +// +// These exercise the `t/` time index directly: the monotonic maybeAppend +// rule (floored at -1 so a negative timestamp is never indexed), the +// ceiling-range-scan lookup, the sequential-batch-scan fallthrough that +// covers a gap in the index (pruning, or a stale compacted header), the +// full-rebuild-on-compaction strategy, and backfilling a legacy watermark. +mod time_index { + use std::time::{Duration, SystemTime}; + + use nisshi_sans_io::{ + IsolationLevel, ListOffset, + create_topics_request::CreatableTopicConfig, + delete_records_request::{DeleteRecordsPartition, DeleteRecordsTopic}, + record::{Record, inflated}, + ser::RecordBatchEncoder, + }; + use nisshi_storage::{ListOffsetResponse, TopicId}; + use serde::Serialize; + + use crate::types::{ + BatchKey, TimeIndexKey, TimeIndexKeyPrefix, Watermark, WatermarkKey, WatermarkLegacy, + }; + + use super::*; + + async fn create_topic(engine: &Engine, name: &str) -> Topition { + let topic = CreatableTopic::default() + .name(name.into()) + .num_partitions(1) + .replication_factor(1); + + let _ = engine.create_topic(topic, false).await.unwrap(); + + Topition::new(name, 0) + } + + async fn topic_with_policy(engine: &Engine, name: &str, configs: &[(&str, &str)]) -> Topition { + let topic = CreatableTopic::default() + .name(name.into()) + .num_partitions(1) + .replication_factor(1) + .configs(Some( + configs + .iter() + .map(|(name, value)| { + CreatableTopicConfig::default() + .name((*name).into()) + .value(Some((*value).into())) + }) + .collect(), + )); + + let _ = engine.create_topic(topic, false).await.unwrap(); + + Topition::new(name, 0) + } + + /// A single-record, single-offset batch with an explicit absolute + /// timestamp, built via the real encoder so it can be inflated. + fn keyed_batch(key: &'static [u8], value: &'static [u8], timestamp: i64) -> Batch { + inflated::Batch::builder() + .record( + Record::builder() + .key(Some(Bytes::from_static(key))) + .value(Some(Bytes::from_static(value))), + ) + .base_timestamp(timestamp) + .max_timestamp(timestamp) + .build() + .and_then(Batch::try_from) + .unwrap() + } + + /// A batch with only header fields set and empty `record_data`: enough + /// to exercise the time index's header-only paths (append, backfill, + /// the sequential scan's cheap header skip), but NOT inflatable - do + /// not use where a test needs its records actually inspected. + fn fake_batch(last_offset_delta: i32, base_timestamp: i64, max_timestamp: i64) -> Batch { + Batch { + base_offset: 0, + batch_length: 0, + partition_leader_epoch: 0, + magic: 2, + crc: 0, + attributes: 0, + last_offset_delta, + base_timestamp, + max_timestamp, + producer_id: -1, + producer_epoch: -1, + base_sequence: -1, + record_count: last_offset_delta as u32 + 1, + record_data: Bytes::new(), + } + } + + fn at_millis(millis: i64) -> SystemTime { + SystemTime::UNIX_EPOCH + Duration::from_millis(millis as u64) + } + + fn millis_since_epoch(t: SystemTime) -> i64 { + t.duration_since(SystemTime::UNIX_EPOCH) + .unwrap() + .as_millis() as i64 + } + + async fn list_offsets_timestamp( + engine: &Engine, + topition: &Topition, + millis: i64, + ) -> ListOffsetResponse { + engine + .list_offsets( + IsolationLevel::ReadUncommitted, + &[(topition.clone(), ListOffset::Timestamp(at_millis(millis)))], + ) + .await + .unwrap() + .remove(0) + .1 + } + + async fn fetch_all(engine: &Engine, topition: &Topition) -> Vec { + engine + .fetch( + topition, + 0, + 1, + 1024 * 1024, + IsolationLevel::ReadUncommitted, + Duration::ZERO, + ) + .await + .unwrap() + } + + async fn topic_uuid(engine: &Engine, name: &str) -> uuid::Uuid { + engine.get_topics().await.unwrap().get(name).unwrap().id + } + + async fn watermark_of(engine: &Engine, topic: uuid::Uuid, partition: i32) -> Watermark { + let key = postcard::to_stdvec(&WatermarkKey::new(topic, partition)).unwrap(); + let encoded = engine.db.get(&key).await.unwrap().unwrap(); + postcard::from_bytes(&encoded).unwrap() + } + + async fn time_index_count(engine: &Engine, topic: uuid::Uuid, partition: i32) -> usize { + let prefix = postcard::to_stdvec(&TimeIndexKeyPrefix::new(topic, partition)).unwrap(); + let scan_start = + postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, 0)).unwrap(); + + let mut scan = engine.db.scan(scan_start..).await.unwrap(); + let mut count = 0; + while let Some(kv) = scan.next().await.unwrap() { + if !kv.key.starts_with(&prefix) { + break; + } + count += 1; + } + count + } + + /// Three single-record batches: offset0 (ts=100, indexed), offset1 + /// (ts=90, <= the running max so NOT indexed - the "gap" a naive + /// ceiling-only lookup would miss if it ever needed to), offset2 + /// (ts=150, indexed). Index: `{100->0, 150->2}`. + async fn ceiling_fixture(name: &str) -> (Engine, Topition) { + let engine = create_test_engine().await; + let topition = create_topic(&engine, name).await; + + for batch in [ + keyed_batch(b"a", b"v1", 100), + keyed_batch(b"b", b"v2", 90), + keyed_batch(b"c", b"v3", 150), + ] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + (engine, topition) + } + + #[tokio::test] + async fn ceiling_scan_finds_between_entries() { + let (engine, topition) = ceiling_fixture("time-index-ceiling-between").await; + + let response = list_offsets_timestamp(&engine, &topition, 115).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(2), response.offset); + assert_eq!(150, millis_since_epoch(response.timestamp.unwrap())); + } + + #[tokio::test] + async fn ceiling_scan_exact_boundary() { + let (engine, topition) = ceiling_fixture("time-index-ceiling-boundary").await; + + let response = list_offsets_timestamp(&engine, &topition, 150).await; + assert_eq!(Some(2), response.offset); + assert_eq!(150, millis_since_epoch(response.timestamp.unwrap())); + } + + #[tokio::test] + async fn future_timestamp_returns_no_match() { + let (engine, topition) = ceiling_fixture("time-index-ceiling-future").await; + + let response = list_offsets_timestamp(&engine, &topition, 151).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(0), response.offset); + assert!(response.timestamp.is_none()); + } + + #[tokio::test] + async fn delete_records_gap_is_covered_by_sequential_scan() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-gap").await; + + // Batch A: 3 records (offsets 0-2), header max_timestamp=100 -> + // indexed 100->0. + let _ = engine + .produce(None, &topition, fake_batch(2, 100, 100)) + .await + .unwrap(); + + // Batch B: 5 real records (offsets 3-7) with absolute timestamps + // 60,70,80,90,95. Its header max_timestamp=95 is <= the running + // max (100), so the monotonic rule skips indexing it - this batch + // is the coverage gap. + let batch_b = { + let mut builder = inflated::Batch::builder() + .base_timestamp(50) + .max_timestamp(95) + .last_offset_delta(4); + + for (delta_index, timestamp_delta) in [10i64, 20, 30, 40, 45].into_iter().enumerate() { + builder = builder.record( + Record::builder() + .key(None) + .value(Some(Bytes::from_static(b"v"))) + .offset_delta(delta_index as i32) + .timestamp_delta(timestamp_delta), + ); + } + + builder.build().and_then(Batch::try_from).unwrap() + }; + let _ = engine.produce(None, &topition, batch_b).await.unwrap(); + + // Batch C: offset8, header max_timestamp=150 -> indexed 150->8. + let _ = engine + .produce(None, &topition, fake_batch(0, 150, 150)) + .await + .unwrap(); + + // delete_records(3) deletes only batch A (base_offset 0 < 3); + // batch B (base_offset 3) and C survive whole. The index's 100->0 + // entry is pruned, leaving only 150->8: a gap below the new low + // watermark (3) that a naive "ceiling or nothing" lookup can't see + // into (SOL-155074 change C's worked example). + let delete_request = vec![ + DeleteRecordsTopic::default() + .name("time-index-gap".into()) + .partitions(Some(vec![ + DeleteRecordsPartition::default() + .partition_index(0) + .offset(3), + ])), + ]; + let _ = engine.delete_records(&delete_request).await.unwrap(); + + let response = list_offsets_timestamp(&engine, &topition, 95).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(7), response.offset); + assert_eq!(95, millis_since_epoch(response.timestamp.unwrap())); + } + + #[tokio::test] + async fn compaction_rebuild_finds_correct_entry_after_removal() { + let engine = create_test_engine().await; + let topition = topic_with_policy( + &engine, + "time-index-compact-removed", + &[("cleanup.policy", "compact")], + ) + .await; + + // offset0: key "x", ts=100 -> indexed 100->0. + let _ = engine + .produce(None, &topition, keyed_batch(b"x", b"old", 100)) + .await + .unwrap(); + // offset1: key "y", ts=90 -> 90<=100, not indexed. + let _ = engine + .produce(None, &topition, keyed_batch(b"y", b"only", 90)) + .await + .unwrap(); + // offset2: key "x" again, ts=110 -> indexed 110->2; supersedes + // offset0's record. + let _ = engine + .produce(None, &topition, keyed_batch(b"x", b"new", 110)) + .await + .unwrap(); + + engine.maintain(SystemTime::now()).await.unwrap(); + + // offset0 is now gone (its key was superseded and it held no other + // record); the index is rebuilt from the survivors (offset1 ts90, + // offset2 ts110) as {90->1, 110->2}, not left pointing at a + // deleted batch. + assert!( + fetch_all(&engine, &topition) + .await + .iter() + .all(|batch| batch.base_offset != 0) + ); + + let response = list_offsets_timestamp(&engine, &topition, 100).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(2), response.offset); + assert_eq!(110, millis_since_epoch(response.timestamp.unwrap())); + } + + #[tokio::test] + async fn compaction_stale_header_does_not_cause_false_match() { + let engine = create_test_engine().await; + let topition = topic_with_policy( + &engine, + "time-index-stale-header", + &[("cleanup.policy", "compact")], + ) + .await; + + // One batch, two records: key "p" (absolute ts=100, later + // superseded) and key "q" (absolute ts=80, survives). The batch + // header's max_timestamp=100 is the true max at the time it is + // written, so it is indexed as 100->0. + let batch = inflated::Batch::builder() + .record( + Record::builder() + .key(Some(Bytes::from_static(b"p"))) + .value(Some(Bytes::from_static(b"old"))) + .timestamp_delta(20), + ) + .record( + Record::builder() + .key(Some(Bytes::from_static(b"q"))) + .value(Some(Bytes::from_static(b"keep"))) + .offset_delta(1), + ) + .base_timestamp(80) + .max_timestamp(100) + .last_offset_delta(1) + .build() + .and_then(Batch::try_from) + .unwrap(); + let _ = engine.produce(None, &topition, batch).await.unwrap(); + + // A later batch supersedes key "p". + let _ = engine + .produce(None, &topition, keyed_batch(b"p", b"new", 120)) + .await + .unwrap(); + + engine.maintain(SystemTime::now()).await.unwrap(); + + // The record for key "p" is removed from the first batch, leaving + // only key "q" (ts=80) - but `Builder::build` copies max_timestamp + // verbatim from the original batch, so the rewritten batch's + // header still (stale-ly) claims max_timestamp=100. The rebuild + // reindexes from this stale header unchanged: {100->0, 120->2}. + // + // Target 90 sits between the surviving record's real timestamp + // (80) and the stale header's claimed max (100): a correct scan + // must look inside the batch at offset 0, find nothing >= 90, and + // fall through to the next batch - not wrongly trust the header as + // a match by itself (SOL-155074 change B). + let response = list_offsets_timestamp(&engine, &topition, 90).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(2), response.offset); + assert_eq!(120, millis_since_epoch(response.timestamp.unwrap())); + } + + #[tokio::test] + async fn negative_timestamp_first_batch_never_indexed() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-negative").await; + + // First and only batch so far: a negative max_timestamp, as an + // untrusted wire header could claim (Nisshi does not validate it - + // SOL-155074 change A). Flooring the monotonic check at -1 + // (Kafka's own NO_TIMESTAMP sentinel) must keep this from ever + // being indexed. + let _ = engine + .produce(None, &topition, fake_batch(0, -5, -5)) + .await + .unwrap(); + + let topic = topic_uuid(&engine, "time-index-negative").await; + assert!( + watermark_of(&engine, topic, 0) + .await + .latest_indexed_timestamp + .is_none() + ); + + // A later, real positive-timestamp batch is indexed normally and + // is not poisoned by the negative entry that came before it: were + // the negative timestamp ever indexed, raw fixint::be two's + // complement encoding would sort it AFTER every positive + // timestamp, corrupting the ceiling scan below. + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", 50)) + .await + .unwrap(); + + let response = list_offsets_timestamp(&engine, &topition, 10).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(1), response.offset); + assert_eq!(50, millis_since_epoch(response.timestamp.unwrap())); + } + + #[tokio::test] + async fn legacy_watermark_backfills_and_answers_correctly() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-legacy").await; + let topic = topic_uuid(&engine, "time-index-legacy").await; + + // Seed two real batches directly into the `b/` keyspace, bypassing + // produce() entirely, to simulate data written before this + // feature existed: no time index was ever maintained for it. + for (offset, timestamp) in [(0i64, 100i64), (1, 200)] { + let batch: Batch = inflated::Batch::builder() + .record( + Record::builder() + .key(None) + .value(Some(Bytes::from_static(b"v"))), + ) + .base_timestamp(timestamp) + .max_timestamp(timestamp) + .build() + .and_then(Batch::try_from) + .unwrap(); + + let encoded = { + let mut encoder = RecordBatchEncoder::new(bytes::BytesMut::new()); + batch.serialize(&mut encoder).unwrap(); + Bytes::from(encoder) + }; + + let batch_key = postcard::to_stdvec(&BatchKey::new(topic, 0, offset)).unwrap(); + let _ = engine.db.put(batch_key, &encoded[..]).await.unwrap(); + } + + // Seed a legacy-shaped (3-field) watermark: low/high only, no time + // index ever written. + let legacy = WatermarkLegacy { + low: Some(0), + high: Some(2), + timestamps: None, + }; + let watermark_key = postcard::to_stdvec(&WatermarkKey::new(topic, 0)).unwrap(); + let _ = engine + .db + .put(watermark_key.clone(), postcard::to_stdvec(&legacy).unwrap()) + .await + .unwrap(); + + // Before any write, list_offsets must still give the correct + // answer: just via the read-only "index empty -> scan from low" + // fallback (slower, since it can't use the O(1) future-check + // shortcut either - see SOL-155074's decision on backfilling), + // not a populated index. + let response = list_offsets_timestamp(&engine, &topition, 150).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(1), response.offset); + assert_eq!(200, millis_since_epoch(response.timestamp.unwrap())); + + // A write now backfills the index from the b/ batch headers. + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", 250)) + .await + .unwrap(); + + // The same lookup must still give the correct answer, now via the + // freshly backfilled index. + let response = list_offsets_timestamp(&engine, &topition, 150).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(1), response.offset); + assert_eq!(200, millis_since_epoch(response.timestamp.unwrap())); + + let watermark = watermark_of(&engine, topic, 0).await; + assert_eq!(Some(250), watermark.latest_indexed_timestamp); + } + + #[tokio::test] + async fn delete_topic_leaves_no_time_index_keys() { + let engine = create_test_engine().await; + let topition1 = create_topic(&engine, "time-index-delete-me").await; + let topition2 = create_topic(&engine, "time-index-keep-me").await; + + let _ = engine + .produce(None, &topition1, keyed_batch(b"a", b"v", 100)) + .await + .unwrap(); + let _ = engine + .produce(None, &topition2, keyed_batch(b"a", b"v", 200)) + .await + .unwrap(); + + let topic1 = topic_uuid(&engine, "time-index-delete-me").await; + let topic2 = topic_uuid(&engine, "time-index-keep-me").await; + + assert_eq!(1, time_index_count(&engine, topic1, 0).await); + assert_eq!(1, time_index_count(&engine, topic2, 0).await); + + let result = engine + .delete_topic(&TopicId::Name("time-index-delete-me".into())) + .await + .unwrap(); + assert_eq!(ErrorCode::None, result); + + assert_eq!(0, time_index_count(&engine, topic1, 0).await); + assert_eq!(1, time_index_count(&engine, topic2, 0).await); + } +} + // ========== Builder Pattern Tests ========== #[tokio::test] diff --git a/nisshi-storage-slatedb/src/types.rs b/nisshi-storage-slatedb/src/types.rs index 3ffcaf69..956024e4 100644 --- a/nisshi-storage-slatedb/src/types.rs +++ b/nisshi-storage-slatedb/src/types.rs @@ -29,6 +29,7 @@ //! | `b/` | Batch data | `b/{topic_uuid}/{partition:be32}/{offset:be64}` | //! | `c/` | Consumer group commits | `c/{group}/{topic}/{partition:be32}` | //! | `g/` | Group state | `g/{group_id}` | +//! | `t/` | Time index | `t/{topic_uuid}/{partition:be32}/{max_timestamp:be64}` | //! | `u/` | SCRAM credentials | `u/{user}/{mechanism:be32}` | //! | `w/` | Watermarks | `w/{topic_uuid}/{partition:be32}` | //! @@ -93,11 +94,45 @@ pub(super) struct TopicMetadata { } /// Watermark for a topic partition +/// +/// `timestamps` is a deprecated field, kept at its original (3rd) position +/// purely so that decoding legacy (pre-time-index) bytes can be detected: a +/// postcard decode of this (now 5-field) shape against legacy 3-field bytes +/// fails, which is the signal `Engine::partition_watermark`/`decode_watermark` +/// use to fall back to [`WatermarkLegacy`] and (on a write path) backfill the +/// time index. Nothing writes to `timestamps` any more; the time index lives +/// in the `t/` keyspace (see [`TimeIndexKey`]), keyed correctly by +/// `max_timestamp` rather than this field's old (and buggy) `base_timestamp` +/// keying. #[derive(Clone, Debug, Default, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] pub(super) struct Watermark { pub low: Option, pub high: Option, pub timestamps: Option>, + /// The greatest `max_timestamp` ever appended to the `t/` time index for + /// this partition, via the monotonic "maybeAppend" rule matching Kafka's + /// own `TimeIndex` (floored at `NO_TIMESTAMP = -1`, so a negative + /// timestamp is never indexed/never advances this). `None` means the + /// index holds nothing for this partition: either it is genuinely empty, + /// or every batch so far had a `max_timestamp <= -1`. + pub latest_indexed_timestamp: Option, + /// The `max_timestamp` header of the most recently appended batch, + /// unconditionally (not floored, not gated by the monotonic rule above). + /// Lets `ListOffsets(Latest)` answer its timestamp in O(1) from the + /// watermark alone, instead of a `batch_base_at_or_before` binary search + /// on every call (`ListOffsets(Latest)` is a consumer hot path). + pub last_batch_max_timestamp: Option, +} + +/// The pre-time-index, 3-field shape of [`Watermark`]. A stored watermark +/// that fails to decode as the current shape is re-parsed as this shape +/// (ANY decode error, not just a specific error kind: any shape mismatch +/// means "not the current format"). See [`Watermark`] for why this works. +#[derive(Clone, Debug, Default, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] +pub(super) struct WatermarkLegacy { + pub low: Option, + pub high: Option, + pub timestamps: Option>, } /// Key for watermark storage: `w/{topic_uuid}/{partition:be32}` @@ -256,6 +291,74 @@ impl BatchKeyPrefix { } } +/// Key for the time index: `t/{topic_uuid}/{partition:be32}/{max_timestamp:be64}` +/// +/// Maps a batch's `max_timestamp` to its `base_offset` (the value, a plain +/// postcard-encoded `i64`). Maintained by the monotonic "maybeAppend" rule +/// from Kafka's own `TimeIndex`: an entry is appended only when its +/// `max_timestamp` is strictly greater than every one appended so far for +/// the partition (see `Engine::append_time_index`). A negative timestamp is +/// never indexed (floored at Kafka's `NO_TIMESTAMP = -1`): with raw +/// `fixint::be` two's-complement encoding, a negative i64 sorts AFTER every +/// positive i64 in byte order, so indexing one would corrupt the ceiling +/// range-scan that `ListOffsets(Timestamp)` depends on. +#[derive(Clone, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] +pub(super) struct TimeIndexKey { + /// Type prefix 't' for time index + pub prefix: char, + /// Topic UUID (16 bytes, fixed) + pub topic: Uuid, + /// Partition number (big-endian for correct ordering) + #[serde(with = "postcard::fixint::be")] + pub partition: Partition, + /// Batch max_timestamp (big-endian for correct ordering) + #[serde(with = "postcard::fixint::be")] + pub timestamp: i64, +} + +impl TimeIndexKey { + pub(super) fn new(topic: Uuid, partition: Partition, timestamp: i64) -> Self { + Self { + prefix: 't', + topic, + partition, + timestamp, + } + } + + /// Create a key for range scan starting from this timestamp + pub(super) fn scan_from(topic: Uuid, partition: Partition, timestamp: i64) -> Self { + Self::new(topic, partition, timestamp) + } +} + +/// Prefix key for scanning all time index entries in a topic partition: +/// `t/{topic_uuid}/{partition}` +/// +/// Separate from `TimeIndexKey` for the same reason as `BatchKeyPrefix`: a +/// scanned key must be checked against this prefix before being decoded as +/// a full `TimeIndexKey`. +#[derive(Clone, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] +pub(super) struct TimeIndexKeyPrefix { + /// Type prefix 't' for time index + pub prefix: char, + /// Topic UUID (16 bytes, fixed) + pub topic: Uuid, + /// Partition number (big-endian for correct ordering) + #[serde(with = "postcard::fixint::be")] + pub partition: Partition, +} + +impl TimeIndexKeyPrefix { + pub(super) fn new(topic: Uuid, partition: Partition) -> Self { + Self { + prefix: 't', + topic, + partition, + } + } +} + /// Key for storing committed offsets: `c/{group}/{topic}/{partition:be32}` /// /// Consumer group offsets are accessed by group, then by topic-partition. From 5fb1b063acd660d8925870b40f5d458bafac2eac Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:05:35 -0400 Subject: [PATCH 02/12] fix(slatedb): address time-index review round 1 Required changes from review round 1: - Strengthen compaction_rebuild_finds_correct_entry_after_removal with a new discriminating scenario (removed entry sits in the MIDDLE of the index, not the head), since the original test passed under both the real full-rebuild-from-survivors logic and a naive per-entry prune. - Strengthen legacy_watermark_backfills_and_answers_correctly so it actually depends on backfill_time_index running: the new scenario puts the legacy data's max timestamp above the post-migration produce's, so only the O(1) no-match shortcut (gated on latest_indexed_timestamp) discriminates backfilled vs. not. Also assert time_index_count == 2. - Fix a real regression: last_batch_max_timestamp (Latest's O(1) fast path) lingered after delete_records/policy_delete fully emptied a partition, so Latest kept answering with a pruned batch's stale timestamp instead of None. Added Engine::clear_latest_if_partition_empty, wired into both prune call sites, plus regression tests for each. - Add warn! logging at the undecodable/un-inflatable batch skip points in sequential_timestamp_scan, timestamp_at_offset, and backfill_time_index, so corrupt data is observable instead of silently invisible. - Add the ticket's own worked example as a real test. - Document the reviewed-and-confirmed-harmless cold-start migration quirk (backfill reads via self.db.scan, not tx.scan, so it can transiently index an offset the same transaction is about to delete). All scenarios verified to actually discriminate: each new/strengthened test was run against a temporarily reverted "naive" version of the corresponding fix and confirmed to fail, then restored. Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01D2qdgPhMyLGZN5dLR8CVHs --- nisshi-storage-slatedb/src/storage.rs | 63 ++++++ nisshi-storage-slatedb/src/tests.rs | 273 +++++++++++++++++++++++++- 2 files changed, 328 insertions(+), 8 deletions(-) diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index 1aa53463..01fde7f6 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -201,6 +201,19 @@ impl Engine { /// `watermark` before replaying, so this is safe to call against a /// `watermark` that already has entries in the index (the caller is /// expected to have deleted them first via `Self::delete_time_index`). + /// + /// Reads via `self.db.scan` rather than `tx.scan`, so a cold-start + /// migration triggered from inside `delete_records`/`policy_delete` + /// (which issue their `b/` deletes on `tx` *before* calling + /// `Self::partition_watermark`) sees the pre-delete, still-committed + /// state: it can backfill a `t/` entry for a `base_offset` that this + /// same transaction is about to delete, so after commit the index + /// transiently names an offset with no batch behind it. This is + /// harmless: every lookup through it (`Self::list_offset_for_timestamp`) + /// only uses an entry's offset as a *starting point* for the forward + /// scan, never requires an exact match there, so at worst it starts a + /// little earlier than strictly necessary - and the next compaction + /// rebuilds the index from survivors anyway (SOL-155074 review round 1). async fn backfill_time_index( &self, tx: &slatedb::DbTransaction, @@ -227,6 +240,12 @@ impl Engine { // best-effort basis, and one unreadable batch should not take // down every other partition's migration in the same write. let Ok(batch) = self.decode(kv.value) else { + tracing::warn!( + ?topic, + partition, + offset = key.offset, + "skipping undecodable batch during time index backfill" + ); continue; }; @@ -243,6 +262,24 @@ impl Engine { Ok(()) } + /// Clear `last_batch_max_timestamp` once pruning (`delete_records`/ + /// `policy_delete`) has left `partition` fully empty (`low >= high`). + /// + /// `last_batch_max_timestamp` is always the highest-offset batch's own + /// header, so it only goes stale when that batch itself is gone - i.e. + /// exactly when the partition is now empty, never on a partial prefix + /// prune. Without this, `ListOffsets(Latest)`'s O(1) fast path + /// (`Storage::list_offsets`) keeps answering with a timestamp for a + /// batch that no longer exists; `main`'s pre-time-index, map-based + /// watermark didn't have this problem because the map naturally + /// emptied along with the data. `policy_compact` doesn't need this + /// helper: it always fully resets-and-replays the index already. + fn clear_latest_if_partition_empty(watermark: &mut Watermark) { + if watermark.low.unwrap_or(0) >= watermark.high.unwrap_or(0) { + watermark.last_batch_max_timestamp = None; + } + } + /// Delete every `t/` entry for `partition`. Used before replaying the /// index from scratch (`Self::backfill_time_index` via /// `Self::policy_compact`) and when a topic is deleted @@ -417,6 +454,12 @@ impl Engine { let key: BatchKey = postcard::from_bytes(&kv.key)?; let Ok(deflated) = self.decode(kv.value) else { + tracing::warn!( + ?topic, + partition, + offset = key.offset, + "skipping undecodable batch during timestamp scan" + ); continue; }; @@ -425,6 +468,12 @@ impl Engine { } let Ok(inflated) = InflatedBatch::try_from(deflated.clone()) else { + tracing::warn!( + ?topic, + partition, + offset = key.offset, + "skipping un-inflatable batch during timestamp scan" + ); continue; }; @@ -486,11 +535,23 @@ impl Engine { } let Ok(mut deflated) = self.decode(kv.value) else { + tracing::warn!( + ?topic, + partition, + offset = base, + "skipping undecodable batch while locating offset timestamp" + ); return Ok(None); }; deflated.base_offset = base; let Ok(inflated) = InflatedBatch::try_from(deflated.clone()) else { + tracing::warn!( + ?topic, + partition, + offset = base, + "skipping un-inflatable batch while locating offset timestamp" + ); return Ok(None); }; @@ -626,6 +687,7 @@ impl Engine { self.prune_time_index_below(&tx, metadata.id, partition, low) .await?; + Self::clear_latest_if_partition_empty(&mut watermark); let watermark_key = postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; @@ -984,6 +1046,7 @@ impl Storage for Engine { new_low_watermark, ) .await?; + Self::clear_latest_if_partition_empty(&mut watermark); let watermark_key = postcard::to_stdvec(&WatermarkKey::new( metadata.id, diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index 76861583..9c709449 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -1272,6 +1272,21 @@ mod cleanup_policy { .unwrap(); assert_eq!(Some(2), responses[0].1.offset); + // The partition is now fully empty (log_start == high_watermark): + // Latest's cached `last_batch_max_timestamp` fast path must have + // been cleared along with it, not keep answering with the pruned + // batch's stale timestamp (SOL-155074 review round 1 required + // change 3). + let responses = engine + .list_offsets( + IsolationLevel::ReadUncommitted, + &[(topition.clone(), ListOffset::Latest)], + ) + .await + .unwrap(); + assert_eq!(Some(2), responses[0].1.offset); + assert!(responses[0].1.timestamp.is_none()); + // The next produce continues from the high watermark let offset = engine .produce(None, &topition, keyed_batch(b"a", b"three", None)) @@ -1624,6 +1639,82 @@ mod time_index { assert_eq!(110, millis_since_epoch(response.timestamp.unwrap())); } + /// Discriminates the real full-rebuild-from-survivors logic (change D) + /// from a naive per-entry prune (delete the `t/` entries whose + /// `base_offset` is in the removed set). `compaction_rebuild_finds_correct_entry_after_removal` + /// above does NOT discriminate this: its removed batch happens to sit + /// at the head of the index, so rule C's "nothing indexed below + /// target -> start at low" masks the gap either way. This scenario + /// removes an entry from the MIDDLE of the index instead, which a + /// naive prune answers wrong and the real rebuild answers right + /// (reviewer-confirmed repro, SOL-155074 review round 1). + #[tokio::test] + async fn compaction_rebuild_depends_on_full_replay_not_naive_prune() { + let engine = create_test_engine().await; + let topition = topic_with_policy( + &engine, + "time-index-compact-naive-prune-gap", + &[("cleanup.policy", "compact")], + ) + .await; + + // offset0: key "a", ts=100 -> indexed 100->0. + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", 100)) + .await + .unwrap(); + // offset1: key "b", ts=150 -> indexed 150->1. Superseded below by + // offset4 (same key "b" again), so compaction removes this batch. + let _ = engine + .produce(None, &topition, keyed_batch(b"b", b"v1", 150)) + .await + .unwrap(); + // offset2: key "c", ts=140 -> 140<=150, NOT indexed under the + // pre-compaction index. This is the entry a naive prune can never + // recover, because nothing ever named it in the first place. + let _ = engine + .produce(None, &topition, keyed_batch(b"c", b"v", 140)) + .await + .unwrap(); + // offset3: key "d", ts=200 -> indexed 200->3. + let _ = engine + .produce(None, &topition, keyed_batch(b"d", b"v", 200)) + .await + .unwrap(); + // offset4: key "b" again, ts=210 -> indexed 210->4; supersedes + // offset1's record. + let _ = engine + .produce(None, &topition, keyed_batch(b"b", b"v2", 210)) + .await + .unwrap(); + + engine.maintain(SystemTime::now()).await.unwrap(); + + // offset1 (the superseded "b") is gone; offset0, offset2, offset3, + // offset4 survive. + assert!( + fetch_all(&engine, &topition) + .await + .iter() + .all(|batch| batch.base_offset != 1) + ); + + // Real code (full replay over survivors) rebuilds the index as + // {100->0, 140->2, 200->3, 210->4} and a query for 120 correctly + // answers offset2@140. + // + // A naive per-entry prune, by contrast, would only delete the + // entry naming the removed base_offset (1, i.e. 150->1) from the + // PRE-compaction index {100->0, 150->1, 200->3, 210->4}, leaving + // {100->0, 200->3, 210->4} - which has no entry for offset2 at + // all, so a query for 120 wrongly lands on the ceiling entry + // 200->3 and answers offset3@200 instead. + let response = list_offsets_timestamp(&engine, &topition, 120).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(2), response.offset); + assert_eq!(140, millis_since_epoch(response.timestamp.unwrap())); + } + #[tokio::test] async fn compaction_stale_header_does_not_cause_false_match() { let engine = create_test_engine().await; @@ -1732,7 +1823,10 @@ mod time_index { // Seed two real batches directly into the `b/` keyspace, bypassing // produce() entirely, to simulate data written before this // feature existed: no time index was ever maintained for it. - for (offset, timestamp) in [(0i64, 100i64), (1, 200)] { + // offset1's timestamp (300) is deliberately HIGHER than the + // post-migration produce below (250): this is what makes the + // scenario discriminating (see the final query's comment). + for (offset, timestamp) in [(0i64, 100i64), (1, 300)] { let batch: Batch = inflated::Batch::builder() .record( Record::builder() @@ -1777,23 +1871,36 @@ mod time_index { let response = list_offsets_timestamp(&engine, &topition, 150).await; assert_eq!(ErrorCode::None, response.error_code); assert_eq!(Some(1), response.offset); - assert_eq!(200, millis_since_epoch(response.timestamp.unwrap())); + assert_eq!(300, millis_since_epoch(response.timestamp.unwrap())); - // A write now backfills the index from the b/ batch headers. + // A write (the cold-start migration trigger) appends a THIRD batch + // at offset2, timestamp=250 - lower than the legacy offset1's 300. let _ = engine .produce(None, &topition, keyed_batch(b"a", b"v", 250)) .await .unwrap(); - // The same lookup must still give the correct answer, now via the - // freshly backfilled index. - let response = list_offsets_timestamp(&engine, &topition, 150).await; + // Both legacy batches must have been backfilled as real `t/` + // entries (100->0, 300->1), not just the new produce's own + // (250->2, which the monotonic rule doesn't even index, since + // 250 <= the backfilled 300). Without backfill this count would be + // 1 (only the new produce's entry). + assert_eq!(2, time_index_count(&engine, topic, 0).await); + + // The discriminating check: query a timestamp (280) that is + // beyond the new produce's own timestamp (250) but still covered + // by backfilled legacy data (offset1@300). Without backfill, + // `latest_indexed_timestamp` would only reflect the new produce + // (250), and rule 1's O(1) shortcut (`target > latest_indexed`) + // would wrongly answer "no match" for 280 even though offset1@300 + // is real, present data. + let response = list_offsets_timestamp(&engine, &topition, 280).await; assert_eq!(ErrorCode::None, response.error_code); assert_eq!(Some(1), response.offset); - assert_eq!(200, millis_since_epoch(response.timestamp.unwrap())); + assert_eq!(300, millis_since_epoch(response.timestamp.unwrap())); let watermark = watermark_of(&engine, topic, 0).await; - assert_eq!(Some(250), watermark.latest_indexed_timestamp); + assert_eq!(Some(300), watermark.latest_indexed_timestamp); } #[tokio::test] @@ -1826,6 +1933,156 @@ mod time_index { assert_eq!(0, time_index_count(&engine, topic1, 0).await); assert_eq!(1, time_index_count(&engine, topic2, 0).await); } + + async fn list_offsets_latest(engine: &Engine, topition: &Topition) -> ListOffsetResponse { + engine + .list_offsets( + IsolationLevel::ReadUncommitted, + &[(topition.clone(), ListOffset::Latest)], + ) + .await + .unwrap() + .remove(0) + .1 + } + + #[tokio::test] + async fn latest_timestamp_cleared_after_delete_records_empties_partition() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-latest-empty").await; + + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", 1000)) + .await + .unwrap(); + + let delete_request = vec![ + DeleteRecordsTopic::default() + .name("time-index-latest-empty".into()) + .partitions(Some(vec![ + DeleteRecordsPartition::default() + .partition_index(0) + .offset(1), + ])), + ]; + let _ = engine.delete_records(&delete_request).await.unwrap(); + + // The partition is now fully empty (low == high == 1). + // `last_batch_max_timestamp` was set to 1000 by the original + // produce and is never touched by `append_time_index` again, so + // without clearing it on full-prune it would keep answering + // Latest with the pruned batch's stale timestamp. On `main` + // (before this change's map-based watermark), Latest naturally + // answered `None` here because the map emptied along with the + // data - this must match (SOL-155074 review round 1 required + // change 3). + let response = list_offsets_latest(&engine, &topition).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(1), response.offset); + assert!(response.timestamp.is_none()); + } + + /// The ticket's own worked example (SOL-155074), as a real regression + /// test: 5 batches of 10 records each, with out-of-order/overlapping + /// timestamps across batches, exercising the ceiling-then-scan lookup + /// and the post-`delete_records` behavior together. + #[tokio::test] + async fn ticket_worked_example() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-ticket-example").await; + + // B0: offsets 0-9, timestamps 91-100. + // B1: offsets 10-19, timestamps 91-100 (same range as B0). + // B2: offsets 20-29, timestamps 71-80 (earlier than B0/B1). + // B3: offsets 30-39, timestamps 141-150. + // B4: offsets 40-49, timestamps 111-120. + let batches: [[i64; 10]; 5] = [ + [91, 92, 93, 94, 95, 96, 97, 98, 99, 100], + [91, 92, 93, 94, 95, 96, 97, 98, 99, 100], + [71, 72, 73, 74, 75, 76, 77, 78, 79, 80], + [141, 142, 143, 144, 145, 146, 147, 148, 149, 150], + [111, 112, 113, 114, 115, 116, 117, 118, 119, 120], + ]; + + for timestamps in batches { + let base_timestamp = timestamps[0]; + let max_timestamp = *timestamps.last().unwrap(); + + let mut builder = inflated::Batch::builder() + .base_timestamp(base_timestamp) + .max_timestamp(max_timestamp) + .last_offset_delta(9); + + for (delta_index, timestamp) in timestamps.into_iter().enumerate() { + builder = builder.record( + Record::builder() + .key(None) + .value(Some(Bytes::from_static(b"v"))) + .offset_delta(delta_index as i32) + .timestamp_delta(timestamp - base_timestamp), + ); + } + + let batch = builder.build().and_then(Batch::try_from).unwrap(); + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + // Timestamp(70): before everything -> the earliest record, offset + // 0 @ 91 (B2's 71-80 starts at offset 20, but the monotonic index + // only ever ceiling-scans forward from the first entry >= target, + // which for 70 is B0's 91->0 - the earliest absolute entry in the + // index; the sequential scan then finds the true first record + // whose timestamp >= 70, which is offset0@91, since nothing + // before it qualifies either). + let response = list_offsets_timestamp(&engine, &topition, 70).await; + assert_eq!(Some(0), response.offset); + assert_eq!(91, millis_since_epoch(response.timestamp.unwrap())); + + // Timestamp(95): first record with timestamp >= 95 is offset4@95 + // (within B0). + let response = list_offsets_timestamp(&engine, &topition, 95).await; + assert_eq!(Some(4), response.offset); + assert_eq!(95, millis_since_epoch(response.timestamp.unwrap())); + + // Timestamp(101): first record with timestamp >= 101 is offset30 + // @141 (B3), since B4's 111-120 sits in offset order AFTER B3 but + // its own timestamps are lower - the scan only skips a batch + // whose header rules it out entirely, so it still inspects B3 + // (base_offset 30) before ever reaching B4. + let response = list_offsets_timestamp(&engine, &topition, 101).await; + assert_eq!(Some(30), response.offset); + assert_eq!(141, millis_since_epoch(response.timestamp.unwrap())); + + // Timestamp(151): beyond everything ever indexed -> no match. + let response = list_offsets_timestamp(&engine, &topition, 151).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(0), response.offset); + assert!(response.timestamp.is_none()); + + // delete_records(10) removes B0 (offsets 0-9) only. + let delete_request = vec![ + DeleteRecordsTopic::default() + .name("time-index-ticket-example".into()) + .partitions(Some(vec![ + DeleteRecordsPartition::default() + .partition_index(0) + .offset(10), + ])), + ]; + let _ = engine.delete_records(&delete_request).await.unwrap(); + + // Timestamp(95): now answered from B1 (offsets 10-19, same + // timestamps 91-100 as the deleted B0) -> offset14@95. + let response = list_offsets_timestamp(&engine, &topition, 95).await; + assert_eq!(Some(14), response.offset); + assert_eq!(95, millis_since_epoch(response.timestamp.unwrap())); + + // Timestamp(70): the new low watermark is 10, so the earliest + // answer is now offset10@91. + let response = list_offsets_timestamp(&engine, &topition, 70).await; + assert_eq!(Some(10), response.offset); + assert_eq!(91, millis_since_epoch(response.timestamp.unwrap())); + } } // ========== Builder Pattern Tests ========== From 5a223d6d5a8c56ae6c964b56cd02ac7cea3f2a0e Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:41:40 -0400 Subject: [PATCH 03/12] fix: resolve typos CI failure in doc comment Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01D2qdgPhMyLGZN5dLR8CVHs --- nisshi-storage-slatedb/src/storage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index 01fde7f6..69272d1e 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -498,7 +498,7 @@ impl Engine { /// inflate (corrupt/malformed data; see `Self::sequential_timestamp_scan` /// for why this degrades gracefully rather than failing the call). Used /// for `ListOffsets(Earliest)`'s timestamp, decoupled from the old - /// (lossy, mis-keyed) `timestamps` map via the same + /// (lossy, wrongly keyed) `timestamps` map via the same /// `batch_base_at_or_after`/`batch_base_at_or_before` helpers `fetch` /// uses to locate the batch covering an arbitrary offset. async fn timestamp_at_offset( From cf7e05c9650c6aec3186901d77be2c85c9644781 Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Mon, 5 Oct 2026 16:41:04 -0400 Subject: [PATCH 04/12] docs: tidy comments to match .claude/rules/comments.md Drops ticket citations used as history narration (the rule's "no ticket numbers as scar tissue"), states a couple of requirements directly instead of framing them as "on main, before this change", and renames a test from ticket_worked_example to describe what it actually exercises. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- nisshi-storage-slatedb/src/storage.rs | 24 ++++++------ nisshi-storage-slatedb/src/tests.rs | 54 ++++++++++++--------------- 2 files changed, 35 insertions(+), 43 deletions(-) diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index 69272d1e..ad930b2b 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -195,7 +195,7 @@ impl Engine { /// individual records). Used both to migrate a legacy watermark on /// first write (`Self::partition_watermark`) and to rebuild the index /// after compaction (`Self::policy_compact`), where pruning individual - /// `t/` entries would leave the coverage gap described in SOL-155074. + /// `t/` entries would leave a coverage gap. /// /// Resets `latest_indexed_timestamp`/`last_batch_max_timestamp` on /// `watermark` before replaying, so this is safe to call against a @@ -213,7 +213,7 @@ impl Engine { /// only uses an entry's offset as a *starting point* for the forward /// scan, never requires an exact match there, so at worst it starts a /// little earlier than strictly necessary - and the next compaction - /// rebuilds the index from survivors anyway (SOL-155074 review round 1). + /// rebuilds the index from survivors anyway. async fn backfill_time_index( &self, tx: &slatedb::DbTransaction, @@ -342,7 +342,7 @@ impl Engine { } /// `ListOffsets(Timestamp)`: the offset of the first record with an - /// absolute timestamp `>= target_millis`, per SOL-155074's lookup + /// absolute timestamp `>= target_millis`, found by this lookup /// algorithm: /// /// 1. `target_millis` beyond everything ever indexed means no match, @@ -425,8 +425,8 @@ impl Engine { /// Scan `b/` batches forward from `start_offset`, skipping a batch /// whose header `max_timestamp` rules it out, and inspecting records /// otherwise. A header can overstate - but never understate - the - /// true max after compaction leaves it stale (SOL-155074 change B), so - /// a qualifying header is a reason to look, not a match by itself: + /// true max after compaction leaves it stale, so a qualifying header is + /// a reason to look, not a match by itself: /// skipping straight to a batch's own `base_offset` without checking /// its records would be the exact bug this scan exists to avoid. /// @@ -813,7 +813,7 @@ impl Engine { // simply wrong. Rebuild from scratch instead, replaying the // monotonic rule over the survivors in offset order - the // same full-rebuild Kafka's own LogCleaner does on segment - // compaction (SOL-155074 change D). + // compaction. self.delete_time_index(&tx, metadata.id, partition).await?; watermark.latest_indexed_timestamp = None; watermark.last_batch_max_timestamp = None; @@ -1036,9 +1036,9 @@ impl Storage for Engine { watermark.low = Some(new_low_watermark); // Remove time index entries before the new low - // watermark (SOL-155074 change D: this is the - // simple prefix-pruning case, unlike compaction, - // and does not touch latest_indexed_timestamp). + // watermark: this is the simple prefix-pruning + // case, unlike compaction, and does not touch + // latest_indexed_timestamp. self.prune_time_index_below( &tx, metadata.id, @@ -1132,7 +1132,7 @@ impl Storage for Engine { // 2. Delete all time index entries for this topic. This keyspace is // enumerated explicitly like every other one here (nothing // wildcard-deletes by topic uuid), so it needs its own step or it - // leaks forever (SOL-155074). + // leaks forever. for partition in 0..topic_metadata.topic.num_partitions { self.delete_time_index(&tx, topic_metadata.id, partition) .await?; @@ -1829,8 +1829,8 @@ impl Storage for Engine { // The most recently appended batch's own max_timestamp, // kept unconditionally on the watermark so this answers // in O(1) rather than a batch_base_at_or_before binary - // search on every call (ListOffsets(Latest) is a - // consumer hot path - SOL-155074). + // search on every call: ListOffsets(Latest) is a + // consumer hot path. let timestamp = watermark .last_batch_max_timestamp .and_then(|ts| to_system_time(ts).ok()); diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index 9c709449..4e045fb4 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -1275,8 +1275,7 @@ mod cleanup_policy { // The partition is now fully empty (log_start == high_watermark): // Latest's cached `last_batch_max_timestamp` fast path must have // been cleared along with it, not keep answering with the pruned - // batch's stale timestamp (SOL-155074 review round 1 required - // change 3). + // batch's stale timestamp. let responses = engine .list_offsets( IsolationLevel::ReadUncommitted, @@ -1318,7 +1317,7 @@ mod cleanup_policy { } } -// ========== Time Index Tests (SOL-155074) ========== +// ========== Time Index Tests ========== // // These exercise the `t/` time index directly: the monotonic maybeAppend // rule (floored at -1 so a negative timestamp is never indexed), the @@ -1575,7 +1574,7 @@ mod time_index { // batch B (base_offset 3) and C survive whole. The index's 100->0 // entry is pruned, leaving only 150->8: a gap below the new low // watermark (3) that a naive "ceiling or nothing" lookup can't see - // into (SOL-155074 change C's worked example). + // into. let delete_request = vec![ DeleteRecordsTopic::default() .name("time-index-gap".into()) @@ -1639,15 +1638,14 @@ mod time_index { assert_eq!(110, millis_since_epoch(response.timestamp.unwrap())); } - /// Discriminates the real full-rebuild-from-survivors logic (change D) - /// from a naive per-entry prune (delete the `t/` entries whose - /// `base_offset` is in the removed set). `compaction_rebuild_finds_correct_entry_after_removal` - /// above does NOT discriminate this: its removed batch happens to sit - /// at the head of the index, so rule C's "nothing indexed below - /// target -> start at low" masks the gap either way. This scenario - /// removes an entry from the MIDDLE of the index instead, which a - /// naive prune answers wrong and the real rebuild answers right - /// (reviewer-confirmed repro, SOL-155074 review round 1). + /// Discriminates the real full-rebuild-from-survivors logic from a + /// naive per-entry prune (delete the `t/` entries whose `base_offset` + /// is in the removed set). `compaction_rebuild_finds_correct_entry_after_removal` + /// above does not discriminate this: its removed batch happens to sit + /// at the head of the index, so the "nothing indexed below target -> + /// start at low" fallback masks the gap either way. This scenario + /// removes an entry from the middle of the index instead, which a + /// naive prune answers wrong and the real rebuild answers right. #[tokio::test] async fn compaction_rebuild_depends_on_full_replay_not_naive_prune() { let engine = create_test_engine().await; @@ -1768,7 +1766,7 @@ mod time_index { // (80) and the stale header's claimed max (100): a correct scan // must look inside the batch at offset 0, find nothing >= 90, and // fall through to the next batch - not wrongly trust the header as - // a match by itself (SOL-155074 change B). + // a match by itself. let response = list_offsets_timestamp(&engine, &topition, 90).await; assert_eq!(ErrorCode::None, response.error_code); assert_eq!(Some(2), response.offset); @@ -1781,10 +1779,9 @@ mod time_index { let topition = create_topic(&engine, "time-index-negative").await; // First and only batch so far: a negative max_timestamp, as an - // untrusted wire header could claim (Nisshi does not validate it - - // SOL-155074 change A). Flooring the monotonic check at -1 - // (Kafka's own NO_TIMESTAMP sentinel) must keep this from ever - // being indexed. + // untrusted wire header could claim (Nisshi does not validate it). + // Flooring the monotonic check at -1 (Kafka's own NO_TIMESTAMP + // sentinel) must keep this from ever being indexed. let _ = engine .produce(None, &topition, fake_batch(0, -5, -5)) .await @@ -1865,9 +1862,8 @@ mod time_index { // Before any write, list_offsets must still give the correct // answer: just via the read-only "index empty -> scan from low" - // fallback (slower, since it can't use the O(1) future-check - // shortcut either - see SOL-155074's decision on backfilling), - // not a populated index. + // fallback (slower, since an unbackfilled index can't use the O(1) + // future-check shortcut either), not a populated index. let response = list_offsets_timestamp(&engine, &topition, 150).await; assert_eq!(ErrorCode::None, response.error_code); assert_eq!(Some(1), response.offset); @@ -1971,23 +1967,19 @@ mod time_index { // `last_batch_max_timestamp` was set to 1000 by the original // produce and is never touched by `append_time_index` again, so // without clearing it on full-prune it would keep answering - // Latest with the pruned batch's stale timestamp. On `main` - // (before this change's map-based watermark), Latest naturally - // answered `None` here because the map emptied along with the - // data - this must match (SOL-155074 review round 1 required - // change 3). + // Latest with the pruned batch's stale timestamp instead of + // `None`, which is what an empty partition must answer. let response = list_offsets_latest(&engine, &topition).await; assert_eq!(ErrorCode::None, response.error_code); assert_eq!(Some(1), response.offset); assert!(response.timestamp.is_none()); } - /// The ticket's own worked example (SOL-155074), as a real regression - /// test: 5 batches of 10 records each, with out-of-order/overlapping - /// timestamps across batches, exercising the ceiling-then-scan lookup - /// and the post-`delete_records` behavior together. + /// A worked example exercising the ceiling-then-scan lookup and the + /// post-`delete_records` behavior together: 5 batches of 10 records + /// each, with out-of-order/overlapping timestamps across batches. #[tokio::test] - async fn ticket_worked_example() { + async fn overlapping_batches_ceiling_then_scan_with_delete_records() { let engine = create_test_engine().await; let topition = create_topic(&engine, "time-index-ticket-example").await; From ba458ef1106ffdcba246c80e31c037fca985a8f1 Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Tue, 6 Oct 2026 03:25:17 -0400 Subject: [PATCH 05/12] fix(slatedb): rebuild the time index through the compaction transaction - Compaction begins its transaction first and reads the batches and the time index through it, so a produce that commits during compaction fails the commit instead of losing its index entry. - The legacy-watermark migration deletes existing `t/` entries before it rebuilds them, reads batches through the transaction, and logs the batch count and elapsed time per partition. A stored watermark is read as legacy only when it decodes as exactly the legacy layout. - A timestamp lookup with no usable index entry starts at the batch that holds the log start offset and skips records below it. - A prune that empties a partition also resets `latest_indexed_timestamp`, so a refill with older timestamps is indexed again. - The timestamp scan logs the decode error of a skipped batch, and comments state the deliberate differences from Kafka (corrupt batch, isolation level). - Comments describe the end state; CHANGELOG gains a Fixed entry. - New tests: ceiling lookup at a middle index entry, a log start inside a batch, refill after emptying, partial prune, stale-entry migration. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011P97dHLhTRdJ35fMJFpYPg Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- CHANGELOG.md | 1 + nisshi-storage-slatedb/src/storage.rs | 377 +++++++++++++------------- nisshi-storage-slatedb/src/tests.rs | 201 ++++++++++++++ nisshi-storage-slatedb/src/types.rs | 32 +-- 4 files changed, 409 insertions(+), 202 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24065b92..4aa43b0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,3 +149,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `WSAGetLastError`, so `raw_os_error()` there is `WSAEAFNOSUPPORT` (10047), a different number, and a Windows host without IPv6 support failed to bind `[::]` instead of falling back to `0.0.0.0`. +- SlateDB: `ListOffsets` with a timestamp answers the offset and timestamp of the first record at or after that timestamp, instead of the base offset of its batch. The `Earliest` timestamp now comes from the first record at the log start offset, and the `Latest` timestamp from the newest batch's maximum timestamp. The engine keeps a new `t/` time-index keyspace, and builds it for a partition on the first write to that partition after the upgrade. After a downgrade, the older binary still reads each partition's watermark and ignores the `t/` keyspace; a topic that it deletes leaves its `t/` entries behind. On the next upgrade, the first write to each partition that the older binary wrote to rebuilds that partition's index from its batches. diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index ad930b2b..d2e46af9 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -18,7 +18,7 @@ use std::{ collections::{BTreeMap, BTreeSet}, iter, sync::Arc, - time::{Duration, SystemTime}, + time::{Duration, Instant, SystemTime}, }; use async_trait::async_trait; @@ -56,7 +56,7 @@ use nisshi_storage::{ TxnOffsetCommitRequest, TxnState, UpdateError, Version, check_claim, producer_claim, }; use serde::Serialize; -use tracing::{debug, warn}; +use tracing::{debug, info, warn}; use uuid::Uuid; use super::engine::Engine; @@ -93,18 +93,13 @@ impl Engine { .is_some_and(|value| value.split(',').any(|candidate| candidate.trim() == policy)) } - /// Load a partition's watermark inside a write transaction, migrating a - /// legacy (pre-time-index) value in place. + /// Load a partition's watermark inside a write transaction, and migrate + /// a [`WatermarkLegacy`] value in place. /// - /// A postcard decode failure against the current (5-field) shape is - /// treated as "legacy 3-field bytes" (broadened past any specific error - /// kind - any shape mismatch means "not the current format"). On that - /// fallback, the time index is backfilled by replaying the monotonic - /// rule over every batch currently in the `b/` keyspace for this - /// partition (`Self::backfill_time_index`), rather than trusting the - /// old, lossy, wrongly-`base_timestamp`-keyed `timestamps` map. The - /// backfill's `t/` writes land in `tx`, so they commit atomically with - /// whatever write triggered this load. + /// A legacy watermark carries no trustworthy time index, so this + /// rebuilds the `t/` entries from the partition's `b/` batch headers + /// (`Self::backfill_time_index`). The rebuild writes to `tx`, so it + /// commits atomically with the write that loaded the watermark. async fn partition_watermark( &self, tx: &slatedb::DbTransaction, @@ -117,49 +112,56 @@ impl Engine { return Ok(Watermark::default()); }; - match postcard::from_bytes::(&encoded) { - Ok(watermark) => Ok(watermark), - Err(_) => { - let legacy: WatermarkLegacy = postcard::from_bytes(&encoded)?; - let mut watermark = Watermark { - low: legacy.low, - high: legacy.high, - timestamps: None, - latest_indexed_timestamp: None, - last_batch_max_timestamp: None, - }; - self.backfill_time_index(tx, topic, partition, &mut watermark) - .await?; - Ok(watermark) - } + let (mut watermark, legacy) = Self::decode_watermark(&encoded)?; + + if legacy { + let started = Instant::now(); + let batches = self + .backfill_time_index(tx, topic, partition, &mut watermark) + .await?; + info!( + ?topic, + partition, + batches, + elapsed_ms = started.elapsed().as_millis(), + "migrated legacy watermark and rebuilt time index" + ); } + + Ok(watermark) } - /// Decode a stored watermark, tolerating the legacy 3-field shape. + /// Decode a stored watermark in either the current or the + /// [`WatermarkLegacy`] layout. + /// + /// The `bool` is `true` for a legacy watermark. Its time index is not + /// built yet, so a caller must not trust `latest_indexed_timestamp` as + /// an upper bound for that partition. /// - /// Used by read-only paths (`list_offsets`, `offset_stage`) that must - /// not perform the tx-based backfill in `Self::partition_watermark`. The - /// `bool` is `true` when the legacy fallback fired: callers use it to - /// skip any shortcut that trusts `latest_indexed_timestamp` as - /// authoritative, since a legacy watermark's index has not been - /// backfilled yet and so understates reality rather than reflecting it. + /// # Errors + /// + /// Returns the legacy decode error when `encoded` is neither a current + /// watermark nor exactly a legacy one. fn decode_watermark(encoded: &[u8]) -> Result<(Watermark, bool)> { - match postcard::from_bytes::(encoded) { - Ok(watermark) => Ok((watermark, false)), - Err(_) => { - let legacy: WatermarkLegacy = postcard::from_bytes(encoded)?; - Ok(( - Watermark { - low: legacy.low, - high: legacy.high, - timestamps: None, - latest_indexed_timestamp: None, - last_batch_max_timestamp: None, - }, - true, - )) - } + if let Ok(watermark) = postcard::from_bytes::(encoded) { + return Ok((watermark, false)); + } + + let (legacy, remainder) = postcard::take_from_bytes::(encoded)?; + if !remainder.is_empty() { + return Err(postcard::Error::DeserializeBadEncoding.into()); } + + Ok(( + Watermark { + low: legacy.low, + high: legacy.high, + timestamps: None, + latest_indexed_timestamp: None, + last_batch_max_timestamp: None, + }, + true, + )) } /// Append `(max_timestamp, base_offset)` to the `t/` time index when @@ -189,64 +191,52 @@ impl Engine { Ok(()) } - /// Replay the monotonic rule (`Self::append_time_index`) over every - /// batch currently stored for `partition`, in offset order, rebuilding - /// the time index from `b/` batch headers only (no need to inflate - /// individual records). Used both to migrate a legacy watermark on - /// first write (`Self::partition_watermark`) and to rebuild the index - /// after compaction (`Self::policy_compact`), where pruning individual - /// `t/` entries would leave a coverage gap. + /// Rebuild the `t/` time index of `partition` from its `b/` batch + /// headers, and return the number of batches that the rebuild read. /// - /// Resets `latest_indexed_timestamp`/`last_batch_max_timestamp` on - /// `watermark` before replaying, so this is safe to call against a - /// `watermark` that already has entries in the index (the caller is - /// expected to have deleted them first via `Self::delete_time_index`). + /// This deletes every existing `t/` entry of the partition first, so an + /// entry that an older write left behind cannot survive the rebuild. + /// Then it replays the monotonic rule (`Self::append_time_index`) over + /// each batch in offset order. /// - /// Reads via `self.db.scan` rather than `tx.scan`, so a cold-start - /// migration triggered from inside `delete_records`/`policy_delete` - /// (which issue their `b/` deletes on `tx` *before* calling - /// `Self::partition_watermark`) sees the pre-delete, still-committed - /// state: it can backfill a `t/` entry for a `base_offset` that this - /// same transaction is about to delete, so after commit the index - /// transiently names an offset with no batch behind it. This is - /// harmless: every lookup through it (`Self::list_offset_for_timestamp`) - /// only uses an entry's offset as a *starting point* for the forward - /// scan, never requires an exact match there, so at worst it starts a - /// little earlier than strictly necessary - and the next compaction - /// rebuilds the index from survivors anyway. + /// This reads through `tx`, so the rebuild sees the batch deletes that + /// `tx` already holds, and a concurrent write to the partition makes + /// `tx` fail to commit. async fn backfill_time_index( &self, tx: &slatedb::DbTransaction, topic: Uuid, partition: i32, watermark: &mut Watermark, - ) -> Result<()> { + ) -> Result { + self.delete_time_index(tx, topic, partition).await?; watermark.latest_indexed_timestamp = None; watermark.last_batch_max_timestamp = None; let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; - let scan_start = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, 0))?; - let mut scan = self.db.scan(scan_start..).await?; + let mut batches = 0; + let mut scan = tx.scan_prefix(&prefix, ..).await?; while let Some(kv) = scan.next().await? { - if !kv.key.starts_with(&prefix) { - break; - } - let key: BatchKey = postcard::from_bytes(&kv.key)?; - - // A batch whose header fails to decode is skipped, not a hard - // failure for the whole backfill: this replays history on a - // best-effort basis, and one unreadable batch should not take - // down every other partition's migration in the same write. - let Ok(batch) = self.decode(kv.value) else { - tracing::warn!( - ?topic, - partition, - offset = key.offset, - "skipping undecodable batch during time index backfill" - ); - continue; + batches += 1; + + // The backfill skips a batch whose header fails to decode, + // instead of failing, because a failure aborts the write that + // triggered the migration, and every later write to the + // partition fails the same way. + let batch = match self.decode(kv.value) { + Ok(batch) => batch, + Err(error) => { + tracing::warn!( + ?topic, + partition, + offset = key.offset, + ?error, + "skipping undecodable batch during time index backfill" + ); + continue; + } }; Self::append_time_index( @@ -259,31 +249,27 @@ impl Engine { )?; } - Ok(()) + Ok(batches) } - /// Clear `last_batch_max_timestamp` once pruning (`delete_records`/ - /// `policy_delete`) has left `partition` fully empty (`low >= high`). + /// Clear the time-index state of `watermark` when pruning has left the + /// partition empty (`low >= high`). /// - /// `last_batch_max_timestamp` is always the highest-offset batch's own - /// header, so it only goes stale when that batch itself is gone - i.e. - /// exactly when the partition is now empty, never on a partial prefix - /// prune. Without this, `ListOffsets(Latest)`'s O(1) fast path - /// (`Storage::list_offsets`) keeps answering with a timestamp for a - /// batch that no longer exists; `main`'s pre-time-index, map-based - /// watermark didn't have this problem because the map naturally - /// emptied along with the data. `policy_compact` doesn't need this - /// helper: it always fully resets-and-replays the index already. - fn clear_latest_if_partition_empty(watermark: &mut Watermark) { + /// `last_batch_max_timestamp` belongs to the highest-offset batch, and + /// `latest_indexed_timestamp` to the newest `t/` entry. A prune removes + /// that batch, and that entry, only when it removes every batch. Without + /// this reset, `ListOffsets(Latest)` answers with the timestamp of a + /// deleted batch, and the monotonic rule refuses to index a refill whose + /// timestamps are older than the deleted data. + fn clear_time_index_state_if_partition_empty(watermark: &mut Watermark) { if watermark.low.unwrap_or(0) >= watermark.high.unwrap_or(0) { + watermark.latest_indexed_timestamp = None; watermark.last_batch_max_timestamp = None; } } - /// Delete every `t/` entry for `partition`. Used before replaying the - /// index from scratch (`Self::backfill_time_index` via - /// `Self::policy_compact`) and when a topic is deleted - /// (`Self::delete_topic`). + /// Delete every `t/` entry for `partition`, reading through `tx` so + /// that a concurrent append to the index makes `tx` fail to commit. async fn delete_time_index( &self, tx: &slatedb::DbTransaction, @@ -291,13 +277,9 @@ impl Engine { partition: i32, ) -> Result<()> { let prefix = postcard::to_stdvec(&TimeIndexKeyPrefix::new(topic, partition))?; - let scan_start = postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, 0))?; - let mut scan = self.db.scan(scan_start..).await?; + let mut scan = tx.scan_prefix(&prefix, ..).await?; while let Some(kv) = scan.next().await? { - if !kv.key.starts_with(&prefix) { - break; - } tx.delete(&kv.key)?; } @@ -341,9 +323,9 @@ impl Engine { Ok(()) } - /// `ListOffsets(Timestamp)`: the offset of the first record with an - /// absolute timestamp `>= target_millis`, found by this lookup - /// algorithm: + /// `ListOffsets(Timestamp)`: the offset of the first record at or after + /// `low` with an absolute timestamp `>= target_millis`, found by this + /// lookup algorithm: /// /// 1. `target_millis` beyond everything ever indexed means no match, /// answered in O(1) - *unless* `legacy` is set, in which case the @@ -354,9 +336,10 @@ impl Engine { /// deletes old `t/` entries but not the batches between the new /// `low` and the first remaining entry (Kafka never hits this /// because its index is per-segment, so deleting old segments can't - /// uncover anything). Start at `low` whenever the index can't rule - /// that gap out (empty, or its first entry is already `>= target`), - /// otherwise start at the ceiling entry's offset. + /// uncover anything). Start at the batch that holds `low` whenever + /// the index can't rule that gap out (empty, or its first entry is + /// already `>= target`), otherwise start at the ceiling entry's + /// offset. /// 3. Sequentially scan `b/` batches forward from there /// (`Self::sequential_timestamp_scan`). async fn list_offset_for_timestamp( @@ -390,23 +373,36 @@ impl Engine { let low = watermark.low.unwrap_or(0); - let start_offset = match first_indexed { + let ceiling = match first_indexed { Some(first_ts) if first_ts < target_millis => { let from = postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, target_millis))?; let mut scan = self.db.scan(from..).await?; match scan.next().await? { Some(kv) if kv.key.starts_with(&index_prefix) => { - postcard::from_bytes(&kv.value)? + Some(postcard::from_bytes::(&kv.value)?) } - _ => low, + _ => None, } } - _ => low, + _ => None, + }; + + let start_offset = match ceiling { + Some(offset) => offset, + None => { + // `low` can fall inside a batch, so the scan starts at the + // base of that batch, and `Self::sequential_timestamp_scan` + // drops each record below `low`. + let batch_prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; + self.batch_base_at_or_before(&batch_prefix, topic, partition, low) + .await? + .unwrap_or(low) + } }; match self - .sequential_timestamp_scan(topic, partition, start_offset, target_millis) + .sequential_timestamp_scan(topic, partition, start_offset, low, target_millis) .await? { Some((offset, ts)) => Ok(ListOffsetResponse { @@ -422,24 +418,20 @@ impl Engine { } } - /// Scan `b/` batches forward from `start_offset`, skipping a batch - /// whose header `max_timestamp` rules it out, and inspecting records - /// otherwise. A header can overstate - but never understate - the - /// true max after compaction leaves it stale, so a qualifying header is - /// a reason to look, not a match by itself: + /// Scan `b/` batches forward from `start_offset` for the first record at + /// or after `low` with a timestamp `>= target_millis`. The scan skips a + /// batch whose header `max_timestamp` rules it out, and inspects the + /// records of every other batch. A header can overstate - but never + /// understate - the true max after compaction leaves it stale, so a + /// qualifying header is a reason to look, not a match by itself: /// skipping straight to a batch's own `base_offset` without checking /// its records would be the exact bug this scan exists to avoid. - /// - /// A batch whose header fails to decode, or whose records fail to - /// inflate (corrupt/malformed data either way), is treated as - /// contributing nothing and skipped, rather than failing the whole - /// lookup: the primary thing a `ListOffsets` caller needs is the - /// `offset`, and a best-effort `timestamp` should not take that down. async fn sequential_timestamp_scan( &self, topic: Uuid, partition: i32, start_offset: i64, + low: i64, target_millis: i64, ) -> Result> { let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; @@ -453,35 +445,47 @@ impl Engine { let key: BatchKey = postcard::from_bytes(&kv.key)?; - let Ok(deflated) = self.decode(kv.value) else { - tracing::warn!( - ?topic, - partition, - offset = key.offset, - "skipping undecodable batch during timestamp scan" - ); - continue; + // The scan skips a batch that fails to decode or to inflate, + // instead of failing the lookup, so that one corrupt batch does + // not fail every `ListOffsets(Timestamp)` on the partition. + // Kafka returns an error for a corrupt batch here; this is a + // deliberate difference. + let deflated = match self.decode(kv.value) { + Ok(deflated) => deflated, + Err(error) => { + tracing::warn!( + ?topic, + partition, + offset = key.offset, + ?error, + "skipping undecodable batch during timestamp scan" + ); + continue; + } }; if deflated.max_timestamp < target_millis { continue; } - let Ok(inflated) = InflatedBatch::try_from(deflated.clone()) else { - tracing::warn!( - ?topic, - partition, - offset = key.offset, - "skipping un-inflatable batch during timestamp scan" - ); - continue; + let inflated = match InflatedBatch::try_from(deflated.clone()) { + Ok(inflated) => inflated, + Err(error) => { + tracing::warn!( + ?topic, + partition, + offset = key.offset, + ?error, + "skipping un-inflatable batch during timestamp scan" + ); + continue; + } }; - if let Some(record) = inflated - .records - .iter() - .find(|record| deflated.base_timestamp + record.timestamp_delta >= target_millis) - { + if let Some(record) = inflated.records.iter().find(|record| { + key.offset + i64::from(record.offset_delta) >= low + && deflated.base_timestamp + record.timestamp_delta >= target_millis + }) { return Ok(Some(( key.offset + i64::from(record.offset_delta), deflated.base_timestamp + record.timestamp_delta, @@ -497,10 +501,7 @@ impl Engine { /// a batch whose header fails to decode or whose records fail to /// inflate (corrupt/malformed data; see `Self::sequential_timestamp_scan` /// for why this degrades gracefully rather than failing the call). Used - /// for `ListOffsets(Earliest)`'s timestamp, decoupled from the old - /// (lossy, wrongly keyed) `timestamps` map via the same - /// `batch_base_at_or_after`/`batch_base_at_or_before` helpers `fetch` - /// uses to locate the batch covering an arbitrary offset. + /// for `ListOffsets(Earliest)`'s timestamp. async fn timestamp_at_offset( &self, topic: Uuid, @@ -687,7 +688,7 @@ impl Engine { self.prune_time_index_below(&tx, metadata.id, partition, low) .await?; - Self::clear_latest_if_partition_empty(&mut watermark); + Self::clear_time_index_state_if_partition_empty(&mut watermark); let watermark_key = postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; @@ -713,27 +714,30 @@ impl Engine { .filter(|metadata| Self::has_cleanup_policy(metadata, COMPACT)) { for partition in 0..metadata.topic.num_partitions { + let tx = self + .db + .begin(slatedb::IsolationLevel::SerializableSnapshot) + .await + .inspect_err(|err| debug!(?err))?; + + // The compaction reads the batches, and later the time + // index, through `tx`, never through `self.db`. A read + // through `tx` registers its range for conflict detection, + // so a produce that commits to this partition during the + // compaction makes `tx` fail to commit. A read through + // `self.db` sees that produce's batch nowhere and its `t/` + // entry in the index, so the rebuild below deletes the + // entry and never adds it back. let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(metadata.id, partition))?; - let scan_start = - postcard::to_stdvec(&BatchKey::scan_from(metadata.id, partition, 0))?; let mut batches = vec![]; - let mut scan = self.db.scan(scan_start..).await?; + let mut scan = tx.scan_prefix(&prefix, ..).await?; while let Some(kv) = scan.next().await? { - if !kv.key.starts_with(&prefix) { - break; - } let key: BatchKey = postcard::from_bytes(&kv.key)?; let batch = self.decode(kv.value)?; batches.push((kv.key, key.offset, batch)); } - let tx = self - .db - .begin(slatedb::IsolationLevel::SerializableSnapshot) - .await - .inspect_err(|err| debug!(?err))?; - // Working from the newest batch to the oldest, a record is // dropped when its key reappears in a newer batch. let mut head = BTreeSet::new(); @@ -805,15 +809,13 @@ impl Engine { // log start would answer OFFSET_OUT_OF_RANGE and reset the // consumer. Only retention and DeleteRecords move it. - // Pruning individual `t/` entries here (as the old - // `timestamps`-map code did) can leave a coverage gap: a - // partially-compacted survivor keeps its original header - // verbatim (see `Builder::build`), which can now be stale, - // and any entry naming a since-deleted base_offset is - // simply wrong. Rebuild from scratch instead, replaying the - // monotonic rule over the survivors in offset order - the - // same full-rebuild Kafka's own LogCleaner does on segment - // compaction. + // The compaction rebuilds the time index from the surviving + // batches, in offset order, instead of deleting only the + // entries of removed batches. A removed batch can hold the + // only entry that covers a later, unindexed survivor, so a + // per-entry delete leaves a gap in the index. Kafka's + // LogCleaner also rebuilds the time index of a segment that + // it compacts. self.delete_time_index(&tx, metadata.id, partition).await?; watermark.latest_indexed_timestamp = None; watermark.last_batch_max_timestamp = None; @@ -1046,7 +1048,7 @@ impl Storage for Engine { new_low_watermark, ) .await?; - Self::clear_latest_if_partition_empty(&mut watermark); + Self::clear_time_index_state_if_partition_empty(&mut watermark); let watermark_key = postcard::to_stdvec(&WatermarkKey::new( metadata.id, @@ -1842,6 +1844,11 @@ impl Storage for Engine { } } ListOffset::Timestamp(target_ts) => { + // This lookup ignores `isolation_level`. Kafka drops a + // match at or above the last fetchable offset (the last + // stable offset for ReadCommitted), and this engine does + // not; that is a known difference. + // // target_ts is SystemTime, need to convert to i64 for comparison let target_millis = target_ts .duration_since(SystemTime::UNIX_EPOCH) diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index 4e045fb4..f93ff5e0 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -1518,6 +1518,84 @@ mod time_index { assert_eq!(150, millis_since_epoch(response.timestamp.unwrap())); } + /// The index is `{100->0, 150->2, 200->5}`, and the target equals the + /// middle entry. A ceiling seek that lands one entry too far starts the + /// scan at offset 5 and answers 5. + #[tokio::test] + async fn ceiling_scan_exact_middle_entry() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-ceiling-middle").await; + + for (key, timestamp) in [ + (b"a", 100), + (b"b", 90), + (b"c", 150), + (b"d", 120), + (b"e", 130), + (b"f", 200), + ] { + let _ = engine + .produce(None, &topition, keyed_batch(key, b"v", timestamp)) + .await + .unwrap(); + } + + let topic = topic_uuid(&engine, "time-index-ceiling-middle").await; + assert_eq!(3, time_index_count(&engine, topic, 0).await); + + let response = list_offsets_timestamp(&engine, &topition, 150).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(2), response.offset); + assert_eq!(150, millis_since_epoch(response.timestamp.unwrap())); + } + + /// The log start offset falls inside the only batch. The lookup must + /// inspect that batch, and must not answer a record below the log start. + #[tokio::test] + async fn timestamp_lookup_skips_records_below_log_start_in_straddling_batch() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-straddle").await; + let topic = topic_uuid(&engine, "time-index-straddle").await; + + // One batch: offsets 0-4, timestamps 10, 20, 30, 40, 50. + let batch = { + let mut builder = inflated::Batch::builder() + .base_timestamp(10) + .max_timestamp(50) + .last_offset_delta(4); + + for delta in 0..5 { + builder = builder.record( + Record::builder() + .key(None) + .value(Some(Bytes::from_static(b"v"))) + .offset_delta(delta) + .timestamp_delta(i64::from(delta) * 10), + ); + } + + builder.build().and_then(Batch::try_from).unwrap() + }; + let _ = engine.produce(None, &topition, batch).await.unwrap(); + + // The test moves the log start to offset 2 and keeps the batch. + let mut watermark = watermark_of(&engine, topic, 0).await; + watermark.low = Some(2); + let watermark_key = postcard::to_stdvec(&WatermarkKey::new(topic, 0)).unwrap(); + let _ = engine + .db + .put(watermark_key, postcard::to_stdvec(&watermark).unwrap()) + .await + .unwrap(); + + // Offset 1 (ts 20) is the first match in the batch, but it is below + // the log start, so the answer is offset 2 (ts 30). + let response = list_offsets_timestamp(&engine, &topition, 15).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(2), response.offset); + assert_eq!(30, millis_since_epoch(response.timestamp.unwrap())); + } + #[tokio::test] async fn future_timestamp_returns_no_match() { let (engine, topition) = ceiling_fixture("time-index-ceiling-future").await; @@ -1975,6 +2053,129 @@ mod time_index { assert!(response.timestamp.is_none()); } + /// A refill after `delete_records` empties the partition is indexed + /// again, even when its timestamps are older than the deleted data. + #[tokio::test] + async fn refill_after_emptying_partition_is_indexed() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-refill").await; + let topic = topic_uuid(&engine, "time-index-refill").await; + + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", 1000)) + .await + .unwrap(); + + let delete_request = vec![ + DeleteRecordsTopic::default() + .name("time-index-refill".into()) + .partitions(Some(vec![ + DeleteRecordsPartition::default() + .partition_index(0) + .offset(1), + ])), + ]; + let _ = engine.delete_records(&delete_request).await.unwrap(); + + let watermark = watermark_of(&engine, topic, 0).await; + assert_eq!(None, watermark.latest_indexed_timestamp); + assert_eq!(0, time_index_count(&engine, topic, 0).await); + + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", 500)) + .await + .unwrap(); + + let watermark = watermark_of(&engine, topic, 0).await; + assert_eq!(Some(500), watermark.latest_indexed_timestamp); + assert_eq!(1, time_index_count(&engine, topic, 0).await); + + let response = list_offsets_timestamp(&engine, &topition, 400).await; + assert_eq!(Some(1), response.offset); + assert_eq!(500, millis_since_epoch(response.timestamp.unwrap())); + } + + /// A partially emptied partition keeps `latest_indexed_timestamp`, so + /// the monotonic rule still holds for the surviving batches. + #[tokio::test] + async fn partial_prune_keeps_latest_indexed_timestamp() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-partial-prune").await; + let topic = topic_uuid(&engine, "time-index-partial-prune").await; + + for timestamp in [1000, 2000] { + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", timestamp)) + .await + .unwrap(); + } + + let delete_request = vec![ + DeleteRecordsTopic::default() + .name("time-index-partial-prune".into()) + .partitions(Some(vec![ + DeleteRecordsPartition::default() + .partition_index(0) + .offset(1), + ])), + ]; + let _ = engine.delete_records(&delete_request).await.unwrap(); + + let watermark = watermark_of(&engine, topic, 0).await; + assert_eq!(Some(2000), watermark.latest_indexed_timestamp); + assert_eq!(Some(2000), watermark.last_batch_max_timestamp); + } + + /// The legacy migration deletes every existing `t/` entry before it + /// rebuilds the index, so an entry that does not match the batches + /// cannot survive it. + #[tokio::test] + async fn legacy_migration_deletes_stale_time_index_entries() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-legacy-stale").await; + let topic = topic_uuid(&engine, "time-index-legacy-stale").await; + + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", 100)) + .await + .unwrap(); + + // An entry for a batch that does not exist. + let stale_key = postcard::to_stdvec(&TimeIndexKey::new(topic, 0, 500)).unwrap(); + let _ = engine + .db + .put(stale_key, postcard::to_stdvec(&7i64).unwrap()) + .await + .unwrap(); + assert_eq!(2, time_index_count(&engine, topic, 0).await); + + let legacy = WatermarkLegacy { + low: Some(0), + high: Some(1), + timestamps: None, + }; + let watermark_key = postcard::to_stdvec(&WatermarkKey::new(topic, 0)).unwrap(); + let _ = engine + .db + .put(watermark_key, postcard::to_stdvec(&legacy).unwrap()) + .await + .unwrap(); + + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v", 200)) + .await + .unwrap(); + + // The rebuild indexes 100->0 and the produce appends 200->1. + assert_eq!(2, time_index_count(&engine, topic, 0).await); + let watermark = watermark_of(&engine, topic, 0).await; + assert_eq!(Some(200), watermark.latest_indexed_timestamp); + + let response = list_offsets_timestamp(&engine, &topition, 150).await; + assert_eq!(Some(1), response.offset); + assert_eq!(200, millis_since_epoch(response.timestamp.unwrap())); + } + /// A worked example exercising the ceiling-then-scan lookup and the /// post-`delete_records` behavior together: 5 batches of 10 records /// each, with out-of-order/overlapping timestamps across batches. diff --git a/nisshi-storage-slatedb/src/types.rs b/nisshi-storage-slatedb/src/types.rs index 956024e4..cdbd6f11 100644 --- a/nisshi-storage-slatedb/src/types.rs +++ b/nisshi-storage-slatedb/src/types.rs @@ -95,26 +95,24 @@ pub(super) struct TopicMetadata { /// Watermark for a topic partition /// -/// `timestamps` is a deprecated field, kept at its original (3rd) position -/// purely so that decoding legacy (pre-time-index) bytes can be detected: a -/// postcard decode of this (now 5-field) shape against legacy 3-field bytes -/// fails, which is the signal `Engine::partition_watermark`/`decode_watermark` -/// use to fall back to [`WatermarkLegacy`] and (on a write path) backfill the -/// time index. Nothing writes to `timestamps` any more; the time index lives -/// in the `t/` keyspace (see [`TimeIndexKey`]), keyed correctly by -/// `max_timestamp` rather than this field's old (and buggy) `base_timestamp` -/// keying. +/// The engine always writes `timestamps` as `None`. The field keeps its 3rd +/// position so that the two layouts stay distinguishable in both directions: +/// a [`WatermarkLegacy`] value is too short to decode as this shape, and a +/// binary that knows only [`WatermarkLegacy`] still reads `low` and `high` +/// from this shape, because postcard ignores trailing bytes. The time index +/// lives in the `t/` keyspace (see [`TimeIndexKey`]). #[derive(Clone, Debug, Default, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] pub(super) struct Watermark { pub low: Option, pub high: Option, pub timestamps: Option>, - /// The greatest `max_timestamp` ever appended to the `t/` time index for + /// The greatest `max_timestamp` appended to the `t/` time index for /// this partition, via the monotonic "maybeAppend" rule matching Kafka's /// own `TimeIndex` (floored at `NO_TIMESTAMP = -1`, so a negative - /// timestamp is never indexed/never advances this). `None` means the - /// index holds nothing for this partition: either it is genuinely empty, - /// or every batch so far had a `max_timestamp <= -1`. + /// timestamp is never indexed/never advances this). A prune that empties + /// the partition resets it to `None`, and a compaction recomputes it from + /// the surviving batches. `None` means no batch since then had a + /// `max_timestamp > -1`. pub latest_indexed_timestamp: Option, /// The `max_timestamp` header of the most recently appended batch, /// unconditionally (not floored, not gated by the monotonic rule above). @@ -124,10 +122,10 @@ pub(super) struct Watermark { pub last_batch_max_timestamp: Option, } -/// The pre-time-index, 3-field shape of [`Watermark`]. A stored watermark -/// that fails to decode as the current shape is re-parsed as this shape -/// (ANY decode error, not just a specific error kind: any shape mismatch -/// means "not the current format"). See [`Watermark`] for why this works. +/// The 3-field watermark layout, written by a binary that does not maintain +/// the `t/` time index. The engine reads a stored watermark as this layout +/// only when it fails to decode as [`Watermark`] and decodes as this layout +/// with no bytes left over. #[derive(Clone, Debug, Default, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] pub(super) struct WatermarkLegacy { pub low: Option, From 67a114495f3c15e9aa0a615d18dc17eba9ed89f9 Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Thu, 8 Oct 2026 13:06:28 -0400 Subject: [PATCH 06/12] fix(slatedb): answer no match as None and honour READ_COMMITTED in timestamp lookups A timestamp lookup with no record at or after the target now answers no offset, on both the shortcut and the scan miss, which the broker maps to -1 as every other engine does. Under READ_COMMITTED, a match at or past the last stable offset is no match, as in Apache Kafka. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01D8pQUJmdicavN4zsumUdaD Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- CHANGELOG.md | 2 +- nisshi-storage-slatedb/src/storage.rs | 47 ++++++---- nisshi-storage-slatedb/src/tests.rs | 130 +++++++++++++++++++++++++- 3 files changed, 160 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4aa43b0f..bd489e28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,4 +149,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `WSAGetLastError`, so `raw_os_error()` there is `WSAEAFNOSUPPORT` (10047), a different number, and a Windows host without IPv6 support failed to bind `[::]` instead of falling back to `0.0.0.0`. -- SlateDB: `ListOffsets` with a timestamp answers the offset and timestamp of the first record at or after that timestamp, instead of the base offset of its batch. The `Earliest` timestamp now comes from the first record at the log start offset, and the `Latest` timestamp from the newest batch's maximum timestamp. The engine keeps a new `t/` time-index keyspace, and builds it for a partition on the first write to that partition after the upgrade. After a downgrade, the older binary still reads each partition's watermark and ignores the `t/` keyspace; a topic that it deletes leaves its `t/` entries behind. On the next upgrade, the first write to each partition that the older binary wrote to rebuilds that partition's index from its batches. +- SlateDB: `ListOffsets` with a timestamp answers the offset and timestamp of the first record at or after that timestamp, instead of the base offset of its batch. Under `READ_COMMITTED`, a record inside an open transaction is no match, as in Apache Kafka. The `Earliest` timestamp now comes from the first record at the log start offset, and the `Latest` timestamp from the newest batch's maximum timestamp. The engine keeps a new `t/` time-index keyspace, and builds it for a partition on the first write to that partition after the upgrade. After a downgrade, the older binary still reads each partition's watermark and ignores the `t/` keyspace; a topic that it deletes leaves its `t/` entries behind. On the next upgrade, the first write to each partition that the older binary wrote to rebuilds that partition's index from its batches. diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index d2e46af9..a0419aeb 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -353,7 +353,7 @@ impl Engine { if !legacy && target_millis > watermark.latest_indexed_timestamp.unwrap_or(-1) { return Ok(ListOffsetResponse { error_code: ErrorCode::None, - offset: Some(0), + offset: None, timestamp: None, }); } @@ -412,7 +412,7 @@ impl Engine { }), None => Ok(ListOffsetResponse { error_code: ErrorCode::None, - offset: Some(0), + offset: None, timestamp: None, }), } @@ -1844,25 +1844,40 @@ impl Storage for Engine { } } ListOffset::Timestamp(target_ts) => { - // This lookup ignores `isolation_level`. Kafka drops a - // match at or above the last fetchable offset (the last - // stable offset for ReadCommitted), and this engine does - // not; that is a known difference. - // - // target_ts is SystemTime, need to convert to i64 for comparison let target_millis = target_ts .duration_since(SystemTime::UNIX_EPOCH) .map(|d| d.as_millis() as i64) .unwrap_or(0); - self.list_offset_for_timestamp( - metadata.id, - topition.partition, - target_millis, - &watermark, - legacy, - ) - .await? + let found = self + .list_offset_for_timestamp( + metadata.id, + topition.partition, + target_millis, + &watermark, + legacy, + ) + .await?; + + // Under ReadCommitted, Kafka answers only a record before + // the last stable offset, and answers no match otherwise + // (https://github.com/apache/kafka/blob/3.9.1/core/src/main/scala/kafka/cluster/Partition.scala#L1589). + let past_last_stable = match found.offset { + Some(offset) if isolation_level == IsolationLevel::ReadCommitted => { + offset >= self.offset_stage(topition).await?.last_stable + } + _ => false, + }; + + if past_last_stable { + ListOffsetResponse { + error_code: ErrorCode::None, + offset: None, + timestamp: None, + } + } else { + found + } } }; diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index f93ff5e0..2e71bfee 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -1602,7 +1602,7 @@ mod time_index { let response = list_offsets_timestamp(&engine, &topition, 151).await; assert_eq!(ErrorCode::None, response.error_code); - assert_eq!(Some(0), response.offset); + assert_eq!(None, response.offset); assert!(response.timestamp.is_none()); } @@ -1670,6 +1670,44 @@ mod time_index { assert_eq!(95, millis_since_epoch(response.timestamp.unwrap())); } + /// A target at or below the greatest indexed timestamp, whose only + /// matching records DeleteRecords has removed, is a scan miss: no + /// offset, as for a target above every record. + #[tokio::test] + async fn scan_miss_after_delete_records_answers_no_match() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-scan-miss").await; + + // Offset 0 (ts=100) is indexed; offset 1 (ts=50) is below the + // running max, so it is not. + for batch in [keyed_batch(b"a", b"v1", 100), keyed_batch(b"b", b"v2", 50)] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + // delete_records(1) removes offset 0 and its entry; the greatest + // indexed timestamp stays 100, so a lookup for 80 cannot take the + // shortcut and scans the surviving batch, which is below 80. + let delete_request = vec![ + DeleteRecordsTopic::default() + .name("time-index-scan-miss".into()) + .partitions(Some(vec![ + DeleteRecordsPartition::default() + .partition_index(0) + .offset(1), + ])), + ]; + let _ = engine.delete_records(&delete_request).await.unwrap(); + + let response = list_offsets_timestamp(&engine, &topition, 80).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(None, response.offset); + assert!(response.timestamp.is_none()); + + let response = list_offsets_timestamp(&engine, &topition, 50).await; + assert_eq!(Some(1), response.offset); + assert_eq!(50, millis_since_epoch(response.timestamp.unwrap())); + } + #[tokio::test] async fn compaction_rebuild_finds_correct_entry_after_removal() { let engine = create_test_engine().await; @@ -2249,7 +2287,7 @@ mod time_index { // Timestamp(151): beyond everything ever indexed -> no match. let response = list_offsets_timestamp(&engine, &topition, 151).await; assert_eq!(ErrorCode::None, response.error_code); - assert_eq!(Some(0), response.offset); + assert_eq!(None, response.offset); assert!(response.timestamp.is_none()); // delete_records(10) removes B0 (offsets 0-9) only. @@ -2276,6 +2314,94 @@ mod time_index { assert_eq!(Some(10), response.offset); assert_eq!(91, millis_since_epoch(response.timestamp.unwrap())); } + + async fn list_offsets_timestamp_at( + engine: &Engine, + isolation_level: IsolationLevel, + topition: &Topition, + millis: i64, + ) -> ListOffsetResponse { + engine + .list_offsets( + isolation_level, + &[(topition.clone(), ListOffset::Timestamp(at_millis(millis)))], + ) + .await + .unwrap() + .remove(0) + .1 + } + + /// Under ReadCommitted, a match at or after the first offset of an open + /// transaction is no match, as in Kafka. ReadUncommitted still answers + /// it. + #[tokio::test] + async fn read_committed_lookup_stops_at_the_last_stable_offset() { + use nisshi_sans_io::add_partitions_to_txn_request::AddPartitionsToTxnTopic; + + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-read-committed").await; + + let _ = engine + .produce(None, &topition, keyed_batch(b"a", b"v1", 100)) + .await + .unwrap(); + + let producer = engine + .init_producer(Some("time-index-txn"), 60000, Some(-1), Some(-1)) + .await + .unwrap(); + let _ = engine + .txn_add_partitions(TxnAddPartitionsRequest::VersionZeroToThree { + transaction_id: "time-index-txn".into(), + producer_id: producer.id, + producer_epoch: producer.epoch, + topics: vec![ + AddPartitionsToTxnTopic::default() + .name("time-index-read-committed".into()) + .partitions(Some(vec![0])), + ], + }) + .await + .unwrap(); + + let open = inflated::Batch::builder() + .record( + Record::builder() + .key(Some(Bytes::from_static(b"b"))) + .value(Some(Bytes::from_static(b"v2"))), + ) + .base_timestamp(200) + .max_timestamp(200) + .producer_id(producer.id) + .producer_epoch(producer.epoch) + .base_sequence(0) + .build() + .and_then(Batch::try_from) + .unwrap(); + let _ = engine + .produce(Some("time-index-txn"), &topition, open) + .await + .unwrap(); + + let stage = engine.offset_stage(&topition).await.unwrap(); + assert_eq!((1, 2), (stage.last_stable, stage.high_watermark)); + + let committed = + list_offsets_timestamp_at(&engine, IsolationLevel::ReadCommitted, &topition, 150).await; + assert_eq!(ErrorCode::None, committed.error_code); + assert_eq!(None, committed.offset); + assert!(committed.timestamp.is_none()); + + let committed = + list_offsets_timestamp_at(&engine, IsolationLevel::ReadCommitted, &topition, 100).await; + assert_eq!(Some(0), committed.offset); + assert_eq!(100, millis_since_epoch(committed.timestamp.unwrap())); + + let uncommitted = list_offsets_timestamp(&engine, &topition, 150).await; + assert_eq!(Some(1), uncommitted.offset); + assert_eq!(200, millis_since_epoch(uncommitted.timestamp.unwrap())); + } } // ========== Builder Pattern Tests ========== From fec205f88490e9f3b801445dacd4661d655ab6ee Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Thu, 8 Oct 2026 13:14:02 -0400 Subject: [PATCH 07/12] fix(slatedb): prune the time index through the transaction A migration triggered by DeleteRecords or retention rebuilds the index in the transaction before the prune runs. The prune read the committed index, so an old entry below the new log start that shared its key with a rebuilt entry deleted the rebuilt entry from the transaction, and a later lookup skipped records. The prune now reads through the transaction, as the rebuild does. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01D8pQUJmdicavN4zsumUdaD Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- nisshi-storage-slatedb/src/storage.rs | 18 +++------ nisshi-storage-slatedb/src/tests.rs | 55 +++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index a0419aeb..646fbab0 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -296,6 +296,11 @@ impl Engine { /// `policy_delete` prefix pruning. Does NOT touch /// `latest_indexed_timestamp`: Kafka's active segment is likewise /// unaffected by deleting older segments. + /// + /// This reads through `tx`, because a migration in the same write + /// (`Self::backfill_time_index`) has replaced the index in `tx`. A scan + /// of the committed index would delete a rebuilt entry whose key an old + /// entry below `low` shares. async fn prune_time_index_below( &self, tx: &slatedb::DbTransaction, @@ -304,14 +309,9 @@ impl Engine { low: i64, ) -> Result<()> { let prefix = postcard::to_stdvec(&TimeIndexKeyPrefix::new(topic, partition))?; - let scan_start = postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, 0))?; - let mut scan = self.db.scan(scan_start..).await?; + let mut scan = tx.scan_prefix(&prefix, ..).await?; while let Some(kv) = scan.next().await? { - if !kv.key.starts_with(&prefix) { - break; - } - let base_offset: i64 = postcard::from_bytes(&kv.value)?; if base_offset >= low { break; @@ -1037,10 +1037,6 @@ impl Storage for Engine { watermark.low = Some(new_low_watermark); - // Remove time index entries before the new low - // watermark: this is the simple prefix-pruning - // case, unlike compaction, and does not touch - // latest_indexed_timestamp. self.prune_time_index_below( &tx, metadata.id, @@ -1807,8 +1803,6 @@ impl Storage for Engine { let response = match list_offset { ListOffset::Earliest => { - // The log start offset is the low watermark, advanced by - // delete_records; timestamps below it are pruned there. let offset = watermark.low.unwrap_or(0); let timestamp = self .timestamp_at_offset(metadata.id, topition.partition, offset) diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index 2e71bfee..71aa6515 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -2214,6 +2214,61 @@ mod time_index { assert_eq!(200, millis_since_epoch(response.timestamp.unwrap())); } + /// A migration triggered by DeleteRecords rebuilds the index from the + /// surviving batches, then prunes below the new log start. The prune + /// must read the rebuilt index in the transaction, not the committed + /// one: here the committed entry `100->0` (offset 0, below the cutoff) + /// shares its key with the rebuilt `100->2`, and a prune of the + /// committed index would delete the rebuilt entry with it. + #[tokio::test] + async fn legacy_migration_by_delete_records_keeps_a_rebuilt_entry() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-legacy-delete").await; + let topic = topic_uuid(&engine, "time-index-legacy-delete").await; + + // Index: 100->0 and 300->3. Offsets 1 and 2 are below the running + // max, so they have no entry. + for batch in [ + keyed_batch(b"a", b"v", 100), + keyed_batch(b"b", b"v", 50), + keyed_batch(b"c", b"v", 100), + keyed_batch(b"d", b"v", 300), + ] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + assert_eq!(2, time_index_count(&engine, topic, 0).await); + + let legacy = WatermarkLegacy { + low: Some(0), + high: Some(4), + timestamps: None, + }; + let watermark_key = postcard::to_stdvec(&WatermarkKey::new(topic, 0)).unwrap(); + let _ = engine + .db + .put(watermark_key, postcard::to_stdvec(&legacy).unwrap()) + .await + .unwrap(); + + let delete_request = vec![ + DeleteRecordsTopic::default() + .name("time-index-legacy-delete".into()) + .partitions(Some(vec![ + DeleteRecordsPartition::default() + .partition_index(0) + .offset(1), + ])), + ]; + let _ = engine.delete_records(&delete_request).await.unwrap(); + + // The rebuild from offsets 1, 2 and 3 gives 50->1, 100->2, 300->3. + assert_eq!(3, time_index_count(&engine, topic, 0).await); + + let response = list_offsets_timestamp(&engine, &topition, 80).await; + assert_eq!(Some(2), response.offset); + assert_eq!(100, millis_since_epoch(response.timestamp.unwrap())); + } + /// A worked example exercising the ceiling-then-scan lookup and the /// post-`delete_records` behavior together: 5 batches of 10 records /// each, with out-of-order/overlapping timestamps across batches. From cb3dd12525d4e95dbd0a12ee51b146d6925c8649 Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:32:36 -0400 Subject: [PATCH 08/12] fix(slatedb): answer the Earliest timestamp from the next batch after compaction Compaction removes a batch whose records are all superseded and leaves the log start where it is, so no batch starts at the log start. The Earliest lookup then fell back to the batch before the log start, which holds no record at or after it, and answered no timestamp. It now reads the next batch when the batch before the log start does not reach it, so the timestamp is the first surviving record's, as the changelog states. The two warn lines on this path now carry the decode error. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01D8pQUJmdicavN4zsumUdaD Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- CHANGELOG.md | 2 +- nisshi-storage-slatedb/src/storage.rs | 120 +++++++++++++++++--------- nisshi-storage-slatedb/src/tests.rs | 46 ++++++++++ 3 files changed, 124 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd489e28..0a95d6dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,4 +149,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `WSAGetLastError`, so `raw_os_error()` there is `WSAEAFNOSUPPORT` (10047), a different number, and a Windows host without IPv6 support failed to bind `[::]` instead of falling back to `0.0.0.0`. -- SlateDB: `ListOffsets` with a timestamp answers the offset and timestamp of the first record at or after that timestamp, instead of the base offset of its batch. Under `READ_COMMITTED`, a record inside an open transaction is no match, as in Apache Kafka. The `Earliest` timestamp now comes from the first record at the log start offset, and the `Latest` timestamp from the newest batch's maximum timestamp. The engine keeps a new `t/` time-index keyspace, and builds it for a partition on the first write to that partition after the upgrade. After a downgrade, the older binary still reads each partition's watermark and ignores the `t/` keyspace; a topic that it deletes leaves its `t/` entries behind. On the next upgrade, the first write to each partition that the older binary wrote to rebuilds that partition's index from its batches. +- SlateDB: `ListOffsets` with a timestamp answers the offset and timestamp of the first record at or after that timestamp, instead of the base offset of its batch. Under `READ_COMMITTED`, a record inside an open transaction is no match, as in Apache Kafka. The `Earliest` timestamp now comes from the first record at or after the log start offset, and the `Latest` timestamp from the newest batch's maximum timestamp. The engine keeps a new `t/` time-index keyspace, and builds it for a partition on the first write to that partition after the upgrade. After a downgrade, the older binary still reads each partition's watermark and ignores the `t/` keyspace; a topic that it deletes leaves its `t/` entries behind. On the next upgrade, the first write to each partition that the older binary wrote to rebuilds that partition's index from its batches. diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index 646fbab0..ac6c067f 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -497,11 +497,12 @@ impl Engine { } /// The real timestamp of the first record at or after `offset` in - /// `partition`, or `None` if no batch covers it: an empty partition, or - /// a batch whose header fails to decode or whose records fail to - /// inflate (corrupt/malformed data; see `Self::sequential_timestamp_scan` - /// for why this degrades gracefully rather than failing the call). Used - /// for `ListOffsets(Earliest)`'s timestamp. + /// `partition`, or `None` when there is no such record: an empty + /// partition, or a batch whose header fails to decode or whose records + /// fail to inflate (corrupt/malformed data; see + /// `Self::sequential_timestamp_scan` for why this degrades gracefully + /// rather than failing the call). Used for `ListOffsets(Earliest)`'s + /// timestamp. async fn timestamp_at_offset( &self, topic: Uuid, @@ -510,59 +511,92 @@ impl Engine { ) -> Result> { let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; - let base = match self + let after = self .batch_base_at_or_after(&prefix, topic, partition, offset) - .await? - { - Some(base) if base == offset => Some(offset), - _ => { - self.batch_base_at_or_before(&prefix, topic, partition, offset) - .await? - } - }; + .await?; - let Some(base) = base else { - return Ok(None); + // The batch that holds `offset` starts at or before it. When no + // batch holds `offset`, because compaction removed the batch at the + // log start and left the log start in place, the first record at or + // after `offset` is in the next batch. + let bases = match after { + Some(base) if base == offset => vec![base], + _ => self + .batch_base_at_or_before(&prefix, topic, partition, offset) + .await? + .into_iter() + .chain(after) + .collect(), }; + for base in bases { + let Some((deflated, inflated)) = self + .inflate_batch_at(&prefix, topic, partition, base) + .await? + else { + return Ok(None); + }; + + if let Some(record) = inflated + .records + .iter() + .find(|record| base + i64::from(record.offset_delta) >= offset) + { + return Ok(to_system_time(deflated.base_timestamp + record.timestamp_delta).ok()); + } + } + + Ok(None) + } + + /// The stored batch at `base`, decoded and inflated, or `None` when no + /// batch starts there or the batch is corrupt. A corrupt batch is + /// logged at `warn`, not returned as an error, for the reason given on + /// `Self::sequential_timestamp_scan`. + async fn inflate_batch_at( + &self, + prefix: &[u8], + topic: Uuid, + partition: i32, + base: i64, + ) -> Result> { let from = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, base))?; let mut scan = self.db.scan(from..).await?; let Some(kv) = scan.next().await? else { return Ok(None); }; - if !kv.key.starts_with(&prefix) { + if !kv.key.starts_with(prefix) { return Ok(None); } - let Ok(mut deflated) = self.decode(kv.value) else { - tracing::warn!( - ?topic, - partition, - offset = base, - "skipping undecodable batch while locating offset timestamp" - ); - return Ok(None); - }; - deflated.base_offset = base; - - let Ok(inflated) = InflatedBatch::try_from(deflated.clone()) else { - tracing::warn!( - ?topic, - partition, - offset = base, - "skipping un-inflatable batch while locating offset timestamp" - ); - return Ok(None); + let deflated = match self.decode(kv.value) { + Ok(deflated) => deflated, + Err(error) => { + tracing::warn!( + ?topic, + partition, + offset = base, + ?error, + "skipping undecodable batch while locating offset timestamp" + ); + return Ok(None); + } }; - Ok(inflated - .records - .iter() - .find(|record| deflated.base_offset + i64::from(record.offset_delta) >= offset) - .and_then(|record| { - to_system_time(deflated.base_timestamp + record.timestamp_delta).ok() - })) + match InflatedBatch::try_from(deflated.clone()) { + Ok(inflated) => Ok(Some((deflated, inflated))), + Err(error) => { + tracing::warn!( + ?topic, + partition, + offset = base, + ?error, + "skipping un-inflatable batch while locating offset timestamp" + ); + Ok(None) + } + } } /// The least batch base offset at or after `probe` within a partition, diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index 71aa6515..c7d313ad 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -1754,6 +1754,52 @@ mod time_index { assert_eq!(110, millis_since_epoch(response.timestamp.unwrap())); } + /// Compaction removes the batch at the log start and leaves the log + /// start where it is, so no batch holds that offset. The Earliest + /// timestamp is then the first record of the next surviving batch, not + /// `None`. + #[tokio::test] + async fn earliest_timestamp_after_compaction_removes_the_first_batch() { + let engine = create_test_engine().await; + let topition = topic_with_policy( + &engine, + "time-index-compact-earliest", + &[("cleanup.policy", "compact")], + ) + .await; + + for batch in [ + keyed_batch(b"x", b"old", 100), + keyed_batch(b"y", b"only", 90), + keyed_batch(b"x", b"new", 110), + ] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + engine.maintain(SystemTime::now()).await.unwrap(); + + assert_eq!(0, engine.offset_stage(&topition).await.unwrap().log_start); + assert!( + fetch_all(&engine, &topition) + .await + .iter() + .all(|batch| batch.base_offset != 0) + ); + + let response = engine + .list_offsets( + IsolationLevel::ReadUncommitted, + &[(topition.clone(), ListOffset::Earliest)], + ) + .await + .unwrap() + .remove(0) + .1; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(0), response.offset); + assert_eq!(90, millis_since_epoch(response.timestamp.unwrap())); + } + /// Discriminates the real full-rebuild-from-survivors logic from a /// naive per-entry prune (delete the `t/` entries whose `base_offset` /// is in the removed set). `compaction_rebuild_finds_correct_entry_after_removal` From de06c07e95dc86518d1b702a6df7a9f896a43fe0 Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:49:30 -0400 Subject: [PATCH 09/12] fix(slatedb): read a ListOffsets lookup through one snapshot The lookup read the watermark, the time index and the batches with separate reads of the live database. The index is read twice, for its first entry and for the ceiling entry, and the choice to trust the ceiling depends on the first entry. A DeleteRecords or retention prune that committed between the two reads removed the entries below the new log start from the second read only, so the ceiling skipped the batches between the new log start and the first surviving entry, and the lookup answered a later offset than the first match. ListOffsets now takes one snapshot per request and reads everything through it. The batch probes take the snapshot as a parameter, so Fetch takes one too, and its probe and its scan see the same batches. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01D8pQUJmdicavN4zsumUdaD Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- nisshi-storage-slatedb/src/storage.rs | 86 +++++++++++++++++---------- nisshi-storage-slatedb/src/tests.rs | 46 ++++++++++++++ 2 files changed, 102 insertions(+), 30 deletions(-) diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index ac6c067f..89ca1398 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -342,8 +342,18 @@ impl Engine { /// offset. /// 3. Sequentially scan `b/` batches forward from there /// (`Self::sequential_timestamp_scan`). - async fn list_offset_for_timestamp( + /// + /// Every read goes through `snapshot`, which must be the snapshot that + /// `watermark` was read from. The index is read twice, for its first + /// entry and for the ceiling entry, and the choice to trust the ceiling + /// depends on the first entry. A prune that commits between two reads of + /// the live database removes the entries below the new log start from + /// the second read only, so the ceiling then skips the batches between + /// the new log start and the first surviving entry, and the lookup + /// answers a later offset than the first match. + pub(super) async fn list_offset_for_timestamp( &self, + snapshot: &slatedb::DbSnapshot, topic: Uuid, partition: i32, target_millis: i64, @@ -362,7 +372,7 @@ impl Engine { let first_indexed = { let from = postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, 0))?; - let mut scan = self.db.scan(from..).await?; + let mut scan = snapshot.scan(from..).await?; match scan.next().await? { Some(kv) if kv.key.starts_with(&index_prefix) => { Some(postcard::from_bytes::(&kv.key)?.timestamp) @@ -377,7 +387,7 @@ impl Engine { Some(first_ts) if first_ts < target_millis => { let from = postcard::to_stdvec(&TimeIndexKey::scan_from(topic, partition, target_millis))?; - let mut scan = self.db.scan(from..).await?; + let mut scan = snapshot.scan(from..).await?; match scan.next().await? { Some(kv) if kv.key.starts_with(&index_prefix) => { Some(postcard::from_bytes::(&kv.value)?) @@ -395,14 +405,14 @@ impl Engine { // base of that batch, and `Self::sequential_timestamp_scan` // drops each record below `low`. let batch_prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; - self.batch_base_at_or_before(&batch_prefix, topic, partition, low) + Self::batch_base_at_or_before(snapshot, &batch_prefix, topic, partition, low) .await? .unwrap_or(low) } }; match self - .sequential_timestamp_scan(topic, partition, start_offset, low, target_millis) + .sequential_timestamp_scan(snapshot, topic, partition, start_offset, low, target_millis) .await? { Some((offset, ts)) => Ok(ListOffsetResponse { @@ -428,6 +438,7 @@ impl Engine { /// its records would be the exact bug this scan exists to avoid. async fn sequential_timestamp_scan( &self, + snapshot: &slatedb::DbSnapshot, topic: Uuid, partition: i32, start_offset: i64, @@ -437,7 +448,7 @@ impl Engine { let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; let from = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, start_offset))?; - let mut scan = self.db.scan(from..).await?; + let mut scan = snapshot.scan(from..).await?; while let Some(kv) = scan.next().await? { if !kv.key.starts_with(&prefix) { break; @@ -505,15 +516,15 @@ impl Engine { /// timestamp. async fn timestamp_at_offset( &self, + snapshot: &slatedb::DbSnapshot, topic: Uuid, partition: i32, offset: i64, ) -> Result> { let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; - let after = self - .batch_base_at_or_after(&prefix, topic, partition, offset) - .await?; + let after = + Self::batch_base_at_or_after(snapshot, &prefix, topic, partition, offset).await?; // The batch that holds `offset` starts at or before it. When no // batch holds `offset`, because compaction removed the batch at the @@ -521,8 +532,7 @@ impl Engine { // after `offset` is in the next batch. let bases = match after { Some(base) if base == offset => vec![base], - _ => self - .batch_base_at_or_before(&prefix, topic, partition, offset) + _ => Self::batch_base_at_or_before(snapshot, &prefix, topic, partition, offset) .await? .into_iter() .chain(after) @@ -531,7 +541,7 @@ impl Engine { for base in bases { let Some((deflated, inflated)) = self - .inflate_batch_at(&prefix, topic, partition, base) + .inflate_batch_at(snapshot, &prefix, topic, partition, base) .await? else { return Ok(None); @@ -555,13 +565,14 @@ impl Engine { /// `Self::sequential_timestamp_scan`. async fn inflate_batch_at( &self, + snapshot: &slatedb::DbSnapshot, prefix: &[u8], topic: Uuid, partition: i32, base: i64, ) -> Result> { let from = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, base))?; - let mut scan = self.db.scan(from..).await?; + let mut scan = snapshot.scan(from..).await?; let Some(kv) = scan.next().await? else { return Ok(None); @@ -602,7 +613,7 @@ impl Engine { /// The least batch base offset at or after `probe` within a partition, /// or `None` when no batch starts at or after it. async fn batch_base_at_or_after( - &self, + snapshot: &slatedb::DbSnapshot, prefix: &[u8], topic: Uuid, partition: i32, @@ -610,7 +621,7 @@ impl Engine { ) -> Result> { let from = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, probe))?; - let mut i = self.db.scan(from..).await?; + let mut i = snapshot.scan(from..).await?; Ok(match i.next().await? { Some(kv) if kv.key.starts_with(prefix) => { @@ -624,7 +635,7 @@ impl Engine { /// partition. SlateDB has forward-only iteration, so this is a binary /// search over [`Self::batch_base_at_or_after`] successor probes. async fn batch_base_at_or_before( - &self, + snapshot: &slatedb::DbSnapshot, prefix: &[u8], topic: Uuid, partition: i32, @@ -636,10 +647,7 @@ impl Engine { while lo <= hi { let mid = lo + (hi - lo) / 2; - match self - .batch_base_at_or_after(prefix, topic, partition, mid) - .await? - { + match Self::batch_base_at_or_after(snapshot, prefix, topic, partition, mid).await? { Some(base) if base <= offset => { floor = Some(base); lo = base + 1; @@ -1537,22 +1545,34 @@ impl Storage for Engine { // When no batch starts exactly at the fetch offset, scan from the // greatest base offset before it; the loop below drops a batch that // ends before the fetch offset. - let start = match self - .batch_base_at_or_after(&prefix, metadata.id, topition.partition, offset) - .await? + let snapshot = self.db.snapshot().await?; + + let start = match Self::batch_base_at_or_after( + &snapshot, + &prefix, + metadata.id, + topition.partition, + offset, + ) + .await? { Some(base) if base == offset => offset, - _ => self - .batch_base_at_or_before(&prefix, metadata.id, topition.partition, offset) - .await? - .unwrap_or(offset), + _ => Self::batch_base_at_or_before( + &snapshot, + &prefix, + metadata.id, + topition.partition, + offset, + ) + .await? + .unwrap_or(offset), }; let mut i = { let from = postcard::to_stdvec(&BatchKey::scan_from(metadata.id, topition.partition, start))?; - self.db.scan(from..).await? + snapshot.scan(from..).await? }; let mut batches = vec![]; @@ -1801,6 +1821,11 @@ impl Storage for Engine { let topics = self.get_topics().await?; let mut responses = Vec::with_capacity(offsets.len()); + // One snapshot serves every partition in the request, so each answer + // comes from one consistent state of the watermark, the time index + // and the batches (see `Self::list_offset_for_timestamp`). + let snapshot = self.db.snapshot().await?; + for (topition, list_offset) in offsets { let Some(metadata) = topics.get(&topition.topic[..]) else { responses.push(( @@ -1830,7 +1855,7 @@ impl Storage for Engine { postcard::to_stdvec(&WatermarkKey::new(metadata.id, topition.partition))?; let (watermark, legacy) = - match self.db.get(&watermark_key).await.map_err(Error::from)? { + match snapshot.get(&watermark_key).await.map_err(Error::from)? { Some(encoded) => Self::decode_watermark(&encoded)?, None => (Watermark::default(), false), }; @@ -1839,7 +1864,7 @@ impl Storage for Engine { ListOffset::Earliest => { let offset = watermark.low.unwrap_or(0); let timestamp = self - .timestamp_at_offset(metadata.id, topition.partition, offset) + .timestamp_at_offset(&snapshot, metadata.id, topition.partition, offset) .await?; ListOffsetResponse { @@ -1879,6 +1904,7 @@ impl Storage for Engine { let found = self .list_offset_for_timestamp( + &snapshot, metadata.id, topition.partition, target_millis, diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index c7d313ad..9da62ba0 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -2092,6 +2092,52 @@ mod time_index { assert_eq!(1, time_index_count(&engine, topic2, 0).await); } + /// The lookup reads the watermark, the time index and the batches from + /// one snapshot, so a prune that commits during the lookup cannot move + /// the index read out from under the start point. A lookup through a + /// snapshot taken before `delete_records` answers from the state before + /// it; a lookup that read the live index would start at the first + /// surviving batch. + #[tokio::test] + async fn timestamp_lookup_reads_through_one_snapshot() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-snapshot").await; + let topic = topic_uuid(&engine, "time-index-snapshot").await; + + for batch in [ + keyed_batch(b"a", b"v", 100), + keyed_batch(b"b", b"v", 200), + keyed_batch(b"c", b"v", 300), + ] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + let snapshot = engine.db.snapshot().await.unwrap(); + let watermark: Watermark = { + let key = postcard::to_stdvec(&WatermarkKey::new(topic, 0)).unwrap(); + postcard::from_bytes(&snapshot.get(&key).await.unwrap().unwrap()).unwrap() + }; + + let delete_request = vec![ + DeleteRecordsTopic::default() + .name("time-index-snapshot".into()) + .partitions(Some(vec![ + DeleteRecordsPartition::default() + .partition_index(0) + .offset(2), + ])), + ]; + let _ = engine.delete_records(&delete_request).await.unwrap(); + assert_eq!(1, time_index_count(&engine, topic, 0).await); + + let response = engine + .list_offset_for_timestamp(&snapshot, topic, 0, 150, &watermark, false) + .await + .unwrap(); + assert_eq!(Some(1), response.offset); + assert_eq!(200, millis_since_epoch(response.timestamp.unwrap())); + } + async fn list_offsets_latest(engine: &Engine, topition: &Topition) -> ListOffsetResponse { engine .list_offsets( From 2f479958eb13107b4edf3f38fd2f6513b62ea573 Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:20:20 -0400 Subject: [PATCH 10/12] fix(slatedb): answer the Earliest timestamp through the lookup scan The Earliest lookup read the batch that holds the log start and, when that batch did not reach it, the next batch. A batch at the log start that failed to decode or inflate answered no timestamp without reading the next batch, while the timestamp lookup scan skips such a batch and keeps going. The Earliest timestamp is now the lookup scan with a target that every timestamp reaches, started at the batch that holds the log start. The scan steps over a batch that does not reach the log start, which is how compaction leaves the batch before it, and over any run of corrupt batches, so both paths answer the first readable record at or after the log start. The helper that read one batch is gone. Three tests: the log start inside a five-record batch with a second batch after it, answered from inside the straddling batch; the same layout with the straddling batch un-inflatable, answered from the next batch; and an un-inflatable batch that starts at the log start, answered from the next batch. The log start is written to the watermark directly, because DeleteRecords removes a batch whose base offset is below the new log start. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01D8pQUJmdicavN4zsumUdaD Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- nisshi-storage-slatedb/src/storage.rs | 109 ++++-------------- nisshi-storage-slatedb/src/tests.rs | 154 +++++++++++++++++++++----- 2 files changed, 148 insertions(+), 115 deletions(-) diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index 89ca1398..92c024b3 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -507,13 +507,16 @@ impl Engine { Ok(None) } - /// The real timestamp of the first record at or after `offset` in - /// `partition`, or `None` when there is no such record: an empty - /// partition, or a batch whose header fails to decode or whose records - /// fail to inflate (corrupt/malformed data; see - /// `Self::sequential_timestamp_scan` for why this degrades gracefully - /// rather than failing the call). Used for `ListOffsets(Earliest)`'s - /// timestamp. + /// The real timestamp of the first readable record at or after `offset` + /// in `partition`, or `None` when there is none. Used for + /// `ListOffsets(Earliest)`'s timestamp. + /// + /// This is the lookup scan (`Self::sequential_timestamp_scan`) with a + /// target that every timestamp reaches, started at the batch that holds + /// `offset`. The scan steps over a batch that does not reach `offset`, + /// which is how compaction leaves the batch before the log start, and + /// over a batch that fails to decode or inflate, so one corrupt batch + /// at the log start does not hide the next batch. async fn timestamp_at_offset( &self, snapshot: &slatedb::DbSnapshot, @@ -523,91 +526,21 @@ impl Engine { ) -> Result> { let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(topic, partition))?; - let after = - Self::batch_base_at_or_after(snapshot, &prefix, topic, partition, offset).await?; - - // The batch that holds `offset` starts at or before it. When no - // batch holds `offset`, because compaction removed the batch at the - // log start and left the log start in place, the first record at or - // after `offset` is in the next batch. - let bases = match after { - Some(base) if base == offset => vec![base], + let start_offset = match Self::batch_base_at_or_after( + snapshot, &prefix, topic, partition, offset, + ) + .await? + { + Some(base) if base == offset => base, _ => Self::batch_base_at_or_before(snapshot, &prefix, topic, partition, offset) .await? - .into_iter() - .chain(after) - .collect(), + .unwrap_or(offset), }; - for base in bases { - let Some((deflated, inflated)) = self - .inflate_batch_at(snapshot, &prefix, topic, partition, base) - .await? - else { - return Ok(None); - }; - - if let Some(record) = inflated - .records - .iter() - .find(|record| base + i64::from(record.offset_delta) >= offset) - { - return Ok(to_system_time(deflated.base_timestamp + record.timestamp_delta).ok()); - } - } - - Ok(None) - } - - /// The stored batch at `base`, decoded and inflated, or `None` when no - /// batch starts there or the batch is corrupt. A corrupt batch is - /// logged at `warn`, not returned as an error, for the reason given on - /// `Self::sequential_timestamp_scan`. - async fn inflate_batch_at( - &self, - snapshot: &slatedb::DbSnapshot, - prefix: &[u8], - topic: Uuid, - partition: i32, - base: i64, - ) -> Result> { - let from = postcard::to_stdvec(&BatchKey::scan_from(topic, partition, base))?; - let mut scan = snapshot.scan(from..).await?; - - let Some(kv) = scan.next().await? else { - return Ok(None); - }; - if !kv.key.starts_with(prefix) { - return Ok(None); - } - - let deflated = match self.decode(kv.value) { - Ok(deflated) => deflated, - Err(error) => { - tracing::warn!( - ?topic, - partition, - offset = base, - ?error, - "skipping undecodable batch while locating offset timestamp" - ); - return Ok(None); - } - }; - - match InflatedBatch::try_from(deflated.clone()) { - Ok(inflated) => Ok(Some((deflated, inflated))), - Err(error) => { - tracing::warn!( - ?topic, - partition, - offset = base, - ?error, - "skipping un-inflatable batch while locating offset timestamp" - ); - Ok(None) - } - } + Ok(self + .sequential_timestamp_scan(snapshot, topic, partition, start_offset, offset, i64::MIN) + .await? + .and_then(|(_, timestamp)| to_system_time(timestamp).ok())) } /// The least batch base offset at or after `probe` within a partition, diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index 9da62ba0..929df798 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -1328,7 +1328,7 @@ mod time_index { use std::time::{Duration, SystemTime}; use nisshi_sans_io::{ - IsolationLevel, ListOffset, + BatchAttribute, Compression, IsolationLevel, ListOffset, create_topics_request::CreatableTopicConfig, delete_records_request::{DeleteRecordsPartition, DeleteRecordsTopic}, record::{Record, inflated}, @@ -1414,6 +1414,43 @@ mod time_index { } } + /// Five records at offset deltas 0-4, with timestamps `base_timestamp` + /// plus 0, 10, 20, 30 and 40, built via the real encoder so the batch + /// can be inflated. + fn five_record_batch(base_timestamp: i64) -> Batch { + let mut builder = inflated::Batch::builder() + .base_timestamp(base_timestamp) + .max_timestamp(base_timestamp + 40) + .last_offset_delta(4); + + for delta in 0..5 { + builder = builder.record( + Record::builder() + .key(None) + .value(Some(Bytes::from_static(b"v"))) + .offset_delta(delta) + .timestamp_delta(i64::from(delta) * 10), + ); + } + + builder.build().and_then(Batch::try_from).unwrap() + } + + /// Move the log start of `topic`'s partition 0 to `low` and keep every + /// batch. `delete_records` removes a batch whose base offset is below + /// the new log start, so a log start inside a batch can only be set by + /// writing the watermark directly. + async fn set_log_start(engine: &Engine, topic: uuid::Uuid, low: i64) { + let mut watermark = watermark_of(engine, topic, 0).await; + watermark.low = Some(low); + let watermark_key = postcard::to_stdvec(&WatermarkKey::new(topic, 0)).unwrap(); + let _ = engine + .db + .put(watermark_key, postcard::to_stdvec(&watermark).unwrap()) + .await + .unwrap(); + } + fn at_millis(millis: i64) -> SystemTime { SystemTime::UNIX_EPOCH + Duration::from_millis(millis as u64) } @@ -1558,36 +1595,13 @@ mod time_index { let topic = topic_uuid(&engine, "time-index-straddle").await; // One batch: offsets 0-4, timestamps 10, 20, 30, 40, 50. - let batch = { - let mut builder = inflated::Batch::builder() - .base_timestamp(10) - .max_timestamp(50) - .last_offset_delta(4); - - for delta in 0..5 { - builder = builder.record( - Record::builder() - .key(None) - .value(Some(Bytes::from_static(b"v"))) - .offset_delta(delta) - .timestamp_delta(i64::from(delta) * 10), - ); - } - - builder.build().and_then(Batch::try_from).unwrap() - }; - let _ = engine.produce(None, &topition, batch).await.unwrap(); - - // The test moves the log start to offset 2 and keeps the batch. - let mut watermark = watermark_of(&engine, topic, 0).await; - watermark.low = Some(2); - let watermark_key = postcard::to_stdvec(&WatermarkKey::new(topic, 0)).unwrap(); let _ = engine - .db - .put(watermark_key, postcard::to_stdvec(&watermark).unwrap()) + .produce(None, &topition, five_record_batch(10)) .await .unwrap(); + set_log_start(&engine, topic, 2).await; + // Offset 1 (ts 20) is the first match in the batch, but it is below // the log start, so the answer is offset 2 (ts 30). let response = list_offsets_timestamp(&engine, &topition, 15).await; @@ -2549,6 +2563,92 @@ mod time_index { assert_eq!(Some(1), uncommitted.offset); assert_eq!(200, millis_since_epoch(uncommitted.timestamp.unwrap())); } + + async fn list_offsets_earliest(engine: &Engine, topition: &Topition) -> ListOffsetResponse { + engine + .list_offsets( + IsolationLevel::ReadUncommitted, + &[(topition.clone(), ListOffset::Earliest)], + ) + .await + .unwrap() + .remove(0) + .1 + } + + /// The log start falls inside the first batch, and a second batch + /// follows it. The Earliest timestamp is the first record at or after + /// the log start inside the straddling batch, not the first record of + /// the next batch. + #[tokio::test] + async fn earliest_timestamp_from_batch_that_straddles_the_log_start() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-earliest-straddle").await; + let topic = topic_uuid(&engine, "time-index-earliest-straddle").await; + + // Offsets 0-4 carry timestamps 10-50; offset 5 carries 60. + for batch in [five_record_batch(10), keyed_batch(b"b", b"v", 60)] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + set_log_start(&engine, topic, 2).await; + + let response = list_offsets_earliest(&engine, &topition).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(2), response.offset); + assert_eq!(30, millis_since_epoch(response.timestamp.unwrap())); + } + + /// The batch that straddles the log start cannot be inflated. The + /// Earliest timestamp then comes from the next batch, as the lookup + /// scan steps over a corrupt batch, instead of being missing. + #[tokio::test] + async fn earliest_timestamp_skips_a_corrupt_batch_that_straddles_the_log_start() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-earliest-corrupt").await; + let topic = topic_uuid(&engine, "time-index-earliest-corrupt").await; + + // Plain record data labelled as zstd is stored as it is and can + // never be inflated. + let mut corrupt = five_record_batch(10); + corrupt.attributes = BatchAttribute::default() + .compression(Compression::Zstd) + .into(); + + for batch in [corrupt, keyed_batch(b"b", b"v", 60)] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + set_log_start(&engine, topic, 2).await; + + let response = list_offsets_earliest(&engine, &topition).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(2), response.offset); + assert_eq!(60, millis_since_epoch(response.timestamp.unwrap())); + } + + /// The batch that starts at the log start cannot be inflated. The + /// Earliest timestamp comes from the next batch, as when the corrupt + /// batch straddles the log start. + #[tokio::test] + async fn earliest_timestamp_skips_a_corrupt_batch_at_the_log_start() { + let engine = create_test_engine().await; + let topition = create_topic(&engine, "time-index-earliest-corrupt-base").await; + + let mut corrupt = keyed_batch(b"a", b"v", 10); + corrupt.attributes = BatchAttribute::default() + .compression(Compression::Zstd) + .into(); + + for batch in [corrupt, keyed_batch(b"b", b"v", 60)] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + let response = list_offsets_earliest(&engine, &topition).await; + assert_eq!(ErrorCode::None, response.error_code); + assert_eq!(Some(0), response.offset); + assert_eq!(60, millis_since_epoch(response.timestamp.unwrap())); + } } // ========== Builder Pattern Tests ========== From 8614efb7b059df59b303702b366592545681c25b Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:20:20 -0400 Subject: [PATCH 11/12] fix(slatedb): retry a maintenance partition on the next tick after a conflict A produce that commits to a partition while compaction or retention reads it makes the maintenance transaction fail to commit, as intended. The error then left `maintain` before the partitions after that one and before lake maintenance, and the broker logs it only at debug. Topics iterate in name order, so a partition under steady writes with records to compact held back the same partitions on every tick. A commit that fails with a transaction conflict is now logged at warn with the topic and partition, and the tick moves on to the next partition; the next tick retries. Any other error is still returned. The per-partition body of each policy is now a function that takes the transaction, so a test can begin a transaction, run the body, produce to the partition, and assert that the commit fails and that the produce's batch and time index entry survive. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01D8pQUJmdicavN4zsumUdaD Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- nisshi-storage-slatedb/src/storage.rs | 380 +++++++++++++++----------- nisshi-storage-slatedb/src/tests.rs | 132 +++++++++ 2 files changed, 359 insertions(+), 153 deletions(-) diff --git a/nisshi-storage-slatedb/src/storage.rs b/nisshi-storage-slatedb/src/storage.rs index 92c024b3..06cf5a85 100644 --- a/nisshi-storage-slatedb/src/storage.rs +++ b/nisshi-storage-slatedb/src/storage.rs @@ -594,6 +594,10 @@ impl Engine { /// Apply the `delete` cleanup policy: remove the prefix of each partition /// where every batch is older than the topic's `retention.ms`. + /// + /// A partition whose transaction loses a conflict is skipped until the + /// next tick (`Self::commit_maintenance`), so the count covers only the + /// partitions that committed. async fn policy_delete(&self, now: SystemTime) -> Result { let Some(now_ms) = now .duration_since(SystemTime::UNIX_EPOCH) @@ -610,10 +614,6 @@ impl Engine { .values() .filter(|metadata| Self::has_cleanup_policy(metadata, DELETE)) { - let retention_ms = Self::topic_config(metadata, RETENTION_MS) - .and_then(|value| value.parse::().ok()) - .unwrap_or(DEFAULT_RETENTION.as_millis() as i64); - for partition in 0..metadata.topic.num_partitions { let tx = self .db @@ -621,65 +621,92 @@ impl Engine { .await .inspect_err(|err| debug!(?err))?; - let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(metadata.id, partition))?; - let scan_start = - postcard::to_stdvec(&BatchKey::scan_from(metadata.id, partition, 0))?; + let removed = self + .delete_expired_prefix(&tx, metadata, partition, now_ms) + .await?; - let mut scan = self.db.scan(scan_start..).await?; - let mut removed = 0; - let mut first_remaining = None; + if removed == 0 { + continue; + } - while let Some(kv) = scan.next().await? { - if !kv.key.starts_with(&prefix) { - break; - } + if Self::commit_maintenance(tx, &metadata.topic.name, partition).await? { + deleted += removed; + } + } + } - let key: BatchKey = postcard::from_bytes(&kv.key)?; - let batch = self.decode(kv.value)?; + Ok(deleted) + } - // Only a prefix of the log is removed: deletion stops at - // the first batch within the retention period. - if now_ms.saturating_sub(batch.max_timestamp) <= retention_ms { - first_remaining = Some(key.offset); - break; - } + /// Delete, through `tx`, the batches of `partition` that are older than + /// the topic's `retention.ms` at `now_ms`, move the log start past them, + /// and return how many batches it deleted. Nothing is written to `tx` + /// when no batch is expired. + pub(super) async fn delete_expired_prefix( + &self, + tx: &slatedb::DbTransaction, + metadata: &TopicMetadata, + partition: i32, + now_ms: i64, + ) -> Result { + let retention_ms = Self::topic_config(metadata, RETENTION_MS) + .and_then(|value| value.parse::().ok()) + .unwrap_or(DEFAULT_RETENTION.as_millis() as i64); - tx.delete(&kv.key)?; - removed += 1; - } + let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(metadata.id, partition))?; + let scan_start = postcard::to_stdvec(&BatchKey::scan_from(metadata.id, partition, 0))?; - if removed == 0 { - continue; - } + let mut scan = self.db.scan(scan_start..).await?; + let mut removed = 0; + let mut first_remaining = None; - let mut watermark = self - .partition_watermark(&tx, metadata.id, partition) - .await?; + while let Some(kv) = scan.next().await? { + if !kv.key.starts_with(&prefix) { + break; + } - // When the partition is now empty the log start meets the - // high watermark: offsets are never reused. - let low = first_remaining.unwrap_or_else(|| watermark.high.unwrap_or(0)); - watermark.low = Some(low); + let key: BatchKey = postcard::from_bytes(&kv.key)?; + let batch = self.decode(kv.value)?; - self.prune_time_index_below(&tx, metadata.id, partition, low) - .await?; - Self::clear_time_index_state_if_partition_empty(&mut watermark); + // Only a prefix of the log is removed: deletion stops at + // the first batch within the retention period. + if now_ms.saturating_sub(batch.max_timestamp) <= retention_ms { + first_remaining = Some(key.offset); + break; + } - let watermark_key = - postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; - tx.put(&watermark_key, postcard::to_stdvec(&watermark)?)?; - _ = tx.commit().await.map_err(Error::from)?; + tx.delete(&kv.key)?; + removed += 1; + } - deleted += removed; - } + if removed == 0 { + return Ok(0); } - Ok(deleted) + let mut watermark = self.partition_watermark(tx, metadata.id, partition).await?; + + // When the partition is now empty the log start meets the + // high watermark: offsets are never reused. + let low = first_remaining.unwrap_or_else(|| watermark.high.unwrap_or(0)); + watermark.low = Some(low); + + self.prune_time_index_below(tx, metadata.id, partition, low) + .await?; + Self::clear_time_index_state_if_partition_empty(&mut watermark); + + let watermark_key = postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; + tx.put(&watermark_key, postcard::to_stdvec(&watermark)?)?; + + Ok(removed) } /// Apply the `compact` cleanup policy: for each record key, retain only /// the most recent record in the partition. Batches that become empty /// are removed; control batches are left untouched. + /// + /// A partition whose transaction loses a conflict is skipped until the + /// next tick (`Self::commit_maintenance`), so the count covers only the + /// partitions that committed. async fn policy_compact(&self) -> Result { let topics = self.get_topics().await?; let mut compacted = 0; @@ -695,130 +722,177 @@ impl Engine { .await .inspect_err(|err| debug!(?err))?; - // The compaction reads the batches, and later the time - // index, through `tx`, never through `self.db`. A read - // through `tx` registers its range for conflict detection, - // so a produce that commits to this partition during the - // compaction makes `tx` fail to commit. A read through - // `self.db` sees that produce's batch nowhere and its `t/` - // entry in the index, so the rebuild below deletes the - // entry and never adds it back. - let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(metadata.id, partition))?; - - let mut batches = vec![]; - let mut scan = tx.scan_prefix(&prefix, ..).await?; - while let Some(kv) = scan.next().await? { - let key: BatchKey = postcard::from_bytes(&kv.key)?; - let batch = self.decode(kv.value)?; - batches.push((kv.key, key.offset, batch)); - } + let removed_records = self.compact_partition(&tx, metadata, partition).await?; - // Working from the newest batch to the oldest, a record is - // dropped when its key reappears in a newer batch. - let mut head = BTreeSet::new(); - let mut removed_offsets = BTreeSet::new(); - let mut removed_records = 0; - - for (raw_key, offset, deflated) in batches.iter().rev() { - // A stored batch that cannot be inflated (malformed, or - // over the decoded-size limit) is left as it is rather - // than failing the whole pass. Its keys are unknown, so - // older records it may supersede are kept: the - // conservative outcome, and every other batch is still - // compacted. - let inflated = match InflatedBatch::try_from(deflated.clone()) { - Ok(inflated) => inflated, - Err(error) => { - warn!( - topic = %metadata.topic.name, - partition, - offset, - ?error, - "compaction is skipping a batch it cannot inflate" - ); - continue; - } - }; + if removed_records == 0 { + continue; + } - if BatchAttribute::try_from(inflated.attributes)?.control { - continue; - } + if Self::commit_maintenance(tx, &metadata.topic.name, partition).await? { + compacted += removed_records; + } + } + } - let keys = inflated.keys(); - let compaction = inflated.compact(&head)?; - head.extend(keys); + Ok(compacted) + } - if compaction.records == 0 { - continue; - } + /// Compact `partition` through `tx`: for each record key, retain only + /// the most recent record, remove the batches that become empty, rebuild + /// the time index from the surviving batches, and return how many + /// records it removed. Nothing is written to `tx` when no record is + /// superseded. + /// + /// Every read goes through `tx`, never through `self.db`. A read through + /// `tx` registers its range for conflict detection, so a produce that + /// commits to this partition during the compaction makes `tx` fail to + /// commit. A read through `self.db` sees that produce's batch nowhere + /// and its `t/` entry in the index, so the rebuild deletes the entry and + /// never adds it back. + pub(super) async fn compact_partition( + &self, + tx: &slatedb::DbTransaction, + metadata: &TopicMetadata, + partition: i32, + ) -> Result { + let prefix = postcard::to_stdvec(&BatchKeyPrefix::new(metadata.id, partition))?; - removed_records += compaction.records as u64; + let mut batches = vec![]; + let mut scan = tx.scan_prefix(&prefix, ..).await?; + while let Some(kv) = scan.next().await? { + let key: BatchKey = postcard::from_bytes(&kv.key)?; + let batch = self.decode(kv.value)?; + batches.push((kv.key, key.offset, batch)); + } - if compaction.batch.records.is_empty() { - tx.delete(raw_key)?; - _ = removed_offsets.insert(*offset); - } else { - let rewritten: Batch = compaction.batch.try_into()?; + // Working from the newest batch to the oldest, a record is + // dropped when its key reappears in a newer batch. + let mut head = BTreeSet::new(); + let mut removed_offsets = BTreeSet::new(); + let mut removed_records = 0; + + for (raw_key, offset, deflated) in batches.iter().rev() { + // A stored batch that cannot be inflated (malformed, or + // over the decoded-size limit) is left as it is rather + // than failing the whole pass. Its keys are unknown, so + // older records it may supersede are kept: the + // conservative outcome, and every other batch is still + // compacted. + let inflated = match InflatedBatch::try_from(deflated.clone()) { + Ok(inflated) => inflated, + Err(error) => { + warn!( + topic = %metadata.topic.name, + partition, + offset, + ?error, + "compaction is skipping a batch it cannot inflate" + ); + continue; + } + }; - let encoded = { - let mut encoder = RecordBatchEncoder::new(BytesMut::new()); - rewritten.serialize(&mut encoder)?; - Bytes::from(encoder) - }; + if BatchAttribute::try_from(inflated.attributes)?.control { + continue; + } - tx.put(raw_key, &encoded[..])?; - } - } + let keys = inflated.keys(); + let compaction = inflated.compact(&head)?; + head.extend(keys); - if removed_records == 0 { - continue; - } + if compaction.records == 0 { + continue; + } - let mut watermark = self - .partition_watermark(&tx, metadata.id, partition) - .await?; + removed_records += compaction.records as u64; - // Compaction leaves the log start where it is, as in Kafka, - // whose cleaner never moves it: a fetch below the first - // surviving batch steps over the gap to it, where moving the - // log start would answer OFFSET_OUT_OF_RANGE and reset the - // consumer. Only retention and DeleteRecords move it. - - // The compaction rebuilds the time index from the surviving - // batches, in offset order, instead of deleting only the - // entries of removed batches. A removed batch can hold the - // only entry that covers a later, unindexed survivor, so a - // per-entry delete leaves a gap in the index. Kafka's - // LogCleaner also rebuilds the time index of a segment that - // it compacts. - self.delete_time_index(&tx, metadata.id, partition).await?; - watermark.latest_indexed_timestamp = None; - watermark.last_batch_max_timestamp = None; - - for (_, offset, batch) in batches - .iter() - .filter(|(_, offset, _)| !removed_offsets.contains(offset)) - { - Self::append_time_index( - &tx, - metadata.id, - partition, - *offset, - batch.max_timestamp, - &mut watermark, - )?; - } + if compaction.batch.records.is_empty() { + tx.delete(raw_key)?; + _ = removed_offsets.insert(*offset); + } else { + let rewritten: Batch = compaction.batch.try_into()?; - let watermark_key = - postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; - tx.put(&watermark_key, postcard::to_stdvec(&watermark)?)?; - _ = tx.commit().await.map_err(Error::from)?; + let encoded = { + let mut encoder = RecordBatchEncoder::new(BytesMut::new()); + rewritten.serialize(&mut encoder)?; + Bytes::from(encoder) + }; - compacted += removed_records; + tx.put(raw_key, &encoded[..])?; } } - Ok(compacted) + if removed_records == 0 { + return Ok(0); + } + + let mut watermark = self.partition_watermark(tx, metadata.id, partition).await?; + + // Compaction leaves the log start where it is, as in Kafka, + // whose cleaner never moves it: a fetch below the first + // surviving batch steps over the gap to it, where moving the + // log start would answer OFFSET_OUT_OF_RANGE and reset the + // consumer. Only retention and DeleteRecords move it. + + // The compaction rebuilds the time index from the surviving + // batches, in offset order, instead of deleting only the + // entries of removed batches. A removed batch can hold the + // only entry that covers a later, unindexed survivor, so a + // per-entry delete leaves a gap in the index. Kafka's + // LogCleaner also rebuilds the time index of a segment that + // it compacts. + self.delete_time_index(tx, metadata.id, partition).await?; + watermark.latest_indexed_timestamp = None; + watermark.last_batch_max_timestamp = None; + + for (_, offset, batch) in batches + .iter() + .filter(|(_, offset, _)| !removed_offsets.contains(offset)) + { + Self::append_time_index( + tx, + metadata.id, + partition, + *offset, + batch.max_timestamp, + &mut watermark, + )?; + } + + let watermark_key = postcard::to_stdvec(&WatermarkKey::new(metadata.id, partition))?; + tx.put(&watermark_key, postcard::to_stdvec(&watermark)?)?; + + Ok(removed_records) + } + + /// Commit a maintenance transaction on `partition` and return whether + /// it committed. + /// + /// A write that commits to the partition while the transaction is open + /// makes the commit fail with a conflict. The conflict is logged at + /// `warn` and answered as `false`, instead of returned as an error, + /// because an error ends the tick before the partitions after this one + /// and before lake maintenance, so a partition under steady writes + /// would hold back the same partitions on every tick. The next tick + /// retries this partition. Any other error is returned. + pub(super) async fn commit_maintenance( + tx: slatedb::DbTransaction, + topic: &str, + partition: i32, + ) -> Result { + match tx.commit().await { + Ok(_) => Ok(true), + Err(err) if err.kind() == slatedb::ErrorKind::Transaction => { + warn!( + topic, + partition, + ?err, + "maintenance lost a conflict with a concurrent write; retrying on the next tick" + ); + Ok(false) + } + Err(err) => Err(err.into()), + } } } diff --git a/nisshi-storage-slatedb/src/tests.rs b/nisshi-storage-slatedb/src/tests.rs index 929df798..111482a7 100644 --- a/nisshi-storage-slatedb/src/tests.rs +++ b/nisshi-storage-slatedb/src/tests.rs @@ -2649,6 +2649,138 @@ mod time_index { assert_eq!(Some(0), response.offset); assert_eq!(60, millis_since_epoch(response.timestamp.unwrap())); } + + /// A produce that commits to the partition while the compaction reads + /// it makes the compaction's commit fail. The produce's batch and its + /// `t/` entry survive, nothing of the compaction is applied, and the + /// next tick compacts the partition. + #[tokio::test] + async fn compaction_that_loses_to_a_concurrent_produce_keeps_its_time_index_entry() { + let engine = create_test_engine().await; + let topition = topic_with_policy( + &engine, + "time-index-compact-conflict", + &[("cleanup.policy", "compact")], + ) + .await; + let topic = topic_uuid(&engine, "time-index-compact-conflict").await; + + for batch in [ + keyed_batch(b"x", b"old", 100), + keyed_batch(b"y", b"only", 90), + keyed_batch(b"x", b"new", 110), + ] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + let topics = engine.get_topics().await.unwrap(); + let metadata = topics.get("time-index-compact-conflict").unwrap(); + + let tx = engine + .db + .begin(slatedb::IsolationLevel::SerializableSnapshot) + .await + .unwrap(); + assert_eq!(1, engine.compact_partition(&tx, metadata, 0).await.unwrap()); + + let _ = engine + .produce(None, &topition, keyed_batch(b"z", b"v", 120)) + .await + .unwrap(); + + assert!( + !Engine::commit_maintenance(tx, &metadata.topic.name, 0) + .await + .unwrap() + ); + + // Offset 0 is still there, and the index holds the produce's entry: + // {100->0, 110->2, 120->3}. + assert_eq!(4, fetch_all(&engine, &topition).await.len()); + assert_eq!(3, time_index_count(&engine, topic, 0).await); + + let response = list_offsets_timestamp(&engine, &topition, 115).await; + assert_eq!(Some(3), response.offset); + assert_eq!(120, millis_since_epoch(response.timestamp.unwrap())); + + engine.maintain(SystemTime::now()).await.unwrap(); + + // The next tick removes offset 0 and rebuilds the index from the + // survivors: {90->1, 110->2, 120->3}. + let batches = fetch_all(&engine, &topition).await; + assert_eq!(3, batches.len()); + assert!(batches.iter().all(|batch| batch.base_offset != 0)); + assert_eq!(3, time_index_count(&engine, topic, 0).await); + + let response = list_offsets_timestamp(&engine, &topition, 115).await; + assert_eq!(Some(3), response.offset); + assert_eq!(120, millis_since_epoch(response.timestamp.unwrap())); + } + + /// A produce that commits to the partition while retention reads it + /// makes the retention's commit fail. The produce's batch and its `t/` + /// entry survive, the expired batches stay until the next tick, and the + /// next tick removes them. + #[tokio::test] + async fn retention_that_loses_to_a_concurrent_produce_keeps_its_time_index_entry() { + let engine = create_test_engine().await; + let topition = topic_with_policy( + &engine, + "time-index-retention-conflict", + &[("cleanup.policy", "delete"), ("retention.ms", "1000")], + ) + .await; + let topic = topic_uuid(&engine, "time-index-retention-conflict").await; + + for batch in [keyed_batch(b"a", b"v", 100), keyed_batch(b"b", b"v", 200)] { + let _ = engine.produce(None, &topition, batch).await.unwrap(); + } + + let topics = engine.get_topics().await.unwrap(); + let metadata = topics.get("time-index-retention-conflict").unwrap(); + let now_ms = 10_000; + + let tx = engine + .db + .begin(slatedb::IsolationLevel::SerializableSnapshot) + .await + .unwrap(); + assert_eq!( + 2, + engine + .delete_expired_prefix(&tx, metadata, 0, now_ms) + .await + .unwrap() + ); + + let _ = engine + .produce(None, &topition, keyed_batch(b"c", b"v", now_ms)) + .await + .unwrap(); + + assert!( + !Engine::commit_maintenance(tx, &metadata.topic.name, 0) + .await + .unwrap() + ); + + // Nothing is deleted, and the index holds the produce's entry: + // {100->0, 200->1, 10000->2}. + assert_eq!(0, engine.offset_stage(&topition).await.unwrap().log_start); + assert_eq!(3, fetch_all(&engine, &topition).await.len()); + assert_eq!(3, time_index_count(&engine, topic, 0).await); + + engine.maintain(at_millis(now_ms)).await.unwrap(); + + // The next tick removes offsets 0 and 1 and prunes their entries. + assert_eq!(2, engine.offset_stage(&topition).await.unwrap().log_start); + assert_eq!(1, fetch_all(&engine, &topition).await.len()); + assert_eq!(1, time_index_count(&engine, topic, 0).await); + + let response = list_offsets_timestamp(&engine, &topition, 150).await; + assert_eq!(Some(2), response.offset); + assert_eq!(now_ms, millis_since_epoch(response.timestamp.unwrap())); + } } // ========== Builder Pattern Tests ========== From e7183789d07bd4fac8c184ea0afc91b467229bf1 Mon Sep 17 00:00:00 2001 From: Andrea Ross <168456375+solace-aross@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:20:20 -0400 Subject: [PATCH 12/12] docs: drop the changelog entry for the SlateDB time index The changelog is generated from the squash commit of each pull request when a release is cut, so a pull request does not edit CHANGELOG.md. The entry's content moves to the pull request description. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01D8pQUJmdicavN4zsumUdaD Signed-off-by: Andrea Ross <168456375+solace-aross@users.noreply.github.com> --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a95d6dd..24065b92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,4 +149,3 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `WSAGetLastError`, so `raw_os_error()` there is `WSAEAFNOSUPPORT` (10047), a different number, and a Windows host without IPv6 support failed to bind `[::]` instead of falling back to `0.0.0.0`. -- SlateDB: `ListOffsets` with a timestamp answers the offset and timestamp of the first record at or after that timestamp, instead of the base offset of its batch. Under `READ_COMMITTED`, a record inside an open transaction is no match, as in Apache Kafka. The `Earliest` timestamp now comes from the first record at or after the log start offset, and the `Latest` timestamp from the newest batch's maximum timestamp. The engine keeps a new `t/` time-index keyspace, and builds it for a partition on the first write to that partition after the upgrade. After a downgrade, the older binary still reads each partition's watermark and ignores the `t/` keyspace; a topic that it deletes leaves its `t/` entries behind. On the next upgrade, the first write to each partition that the older binary wrote to rebuilds that partition's index from its batches.