From d2ddb9d7a8b9860683e399aca199e314000d1f64 Mon Sep 17 00:00:00 2001 From: Weiming Long Date: Wed, 22 Jul 2026 11:22:56 +0800 Subject: [PATCH 1/2] Harden storage recovery and cloud serving --- .github/workflows/ci.yml | 22 + CLAUDE.md | 2 +- Cargo.lock | 2 + Cargo.toml | 2 +- README.md | 16 + TESTING.md | 42 +- crates/pb-api/README.md | 8 +- crates/pb-api/src/hydration.rs | 219 ++++++-- crates/pb-api/src/live_state.rs | 22 +- crates/pb-bin/Cargo.toml | 1 + crates/pb-bin/README.md | 15 +- crates/pb-bin/src/commands/doctor.rs | 77 ++- .../pb-bin/src/commands/execution_replay.rs | 3 +- crates/pb-bin/src/commands/pipeline.rs | 145 +++-- crates/pb-bin/src/commands/reconcile.rs | 216 ++++++-- crates/pb-bin/src/commands/replay.rs | 3 +- crates/pb-bin/src/commands/serve.rs | 20 +- crates/pb-bin/src/commands/serve_api.rs | 7 +- crates/pb-replay/README.md | 19 +- crates/pb-replay/src/error.rs | 3 + crates/pb-replay/src/reader.rs | 517 +++++++++++++++--- crates/pb-replay/src/tests.rs | 133 ++++- crates/pb-service/README.md | 5 + crates/pb-service/src/parquet.rs | 47 +- crates/pb-store/README.md | 34 +- crates/pb-store/src/error.rs | 3 + crates/pb-store/src/lib.rs | 2 +- crates/pb-store/src/tests.rs | 247 ++++++++- crates/pb-store/src/writer.rs | 459 ++++++++++++++-- crates/pb-types/README.md | 10 +- crates/pb-types/src/lib.rs | 2 + crates/pb-types/src/newtype.rs | 28 + crates/pb-types/src/storage.rs | 95 ++++ crates/pb-wal/README.md | 6 + crates/pb-wal/src/lib.rs | 70 ++- crates/pb-wal/src/reader.rs | 88 +++ crates/pb-wal/src/writer.rs | 49 +- docs/adr/0009-dual-sink-storage.md | 19 +- docs/api.md | 4 + docs/architecture.md | 9 +- docs/operations.md | 94 +++- docs/serve-api.md | 11 +- infra/iam.tf | 5 +- monitoring/RUNBOOK.md | 18 +- monitoring/alerts.yml | 8 +- monitoring/alerts_test.yml | 2 +- .../design.md | 11 + .../specs/serving-runtime-platform/spec.md | 10 + .../tasks.md | 3 + tests/integration/s3_minio_roundtrip.rs | 50 +- 50 files changed, 2399 insertions(+), 484 deletions(-) create mode 100644 crates/pb-types/src/storage.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53b4d681..356eb8cb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,6 +49,26 @@ jobs: toolchain: stable - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - uses: ./.github/actions/setup-protobuf + - name: Start MinIO and create the integration bucket + run: | + docker run --detach --name pb-minio \ + --publish 9100:9000 \ + --env MINIO_ROOT_USER=minioadmin \ + --env MINIO_ROOT_PASSWORD=minioadmin \ + minio/minio:RELEASE.2025-07-23T15-54-02Z server /data + for attempt in $(seq 1 30); do + if curl --fail --silent http://127.0.0.1:9100/minio/health/ready >/dev/null; then + break + fi + if [ "$attempt" -eq 30 ]; then + docker logs pb-minio + exit 1 + fi + sleep 1 + done + docker run --rm --network host --entrypoint /bin/sh \ + minio/mc:RELEASE.2025-07-21T05-28-08Z \ + -c 'mc alias set local http://127.0.0.1:9100 minioadmin minioadmin && mc mb --ignore-existing local/poly-book-test' # The #[ignore]d cases: ClickHouse roundtrips, Parquet-vs-ClickHouse # cross-backend equivalence, and the MinIO S3 roundtrip — the strongest # consistency evidence in the suite, previously never enforced in CI. @@ -56,6 +76,8 @@ jobs: # Single-threaded: the containers are memory-hungry and the tests share # fixed host ports. - run: cargo test -p pb-integration-tests -- --ignored --test-threads=1 + env: + PB_TEST_S3_ENDPOINT: http://127.0.0.1:9100 clippy: name: Clippy diff --git a/CLAUDE.md b/CLAUDE.md index e345fa12..9208d767 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,7 @@ design notes, and a **Docs to Update After Changes** table. - `thiserror` for library crate errors, `anyhow` only in pb-bin - `tracing` for structured logging (not `log` or `println!`) - Wire types borrow from raw buffers (`&'a str`) for zero-copy deserialization -- Storage uses `object_store` trait for filesystem abstraction (local FS / S3 / GCS) +- Storage uses the `object_store` trait for filesystem abstraction (local FS / S3) - `mimalloc` as global allocator in pb-bin for lower p99 latency - `FxHashMap` (rustc-hash) for internal hot-path lookups in trusted-data contexts - `proptest` for property-based invariant testing in pb-types and pb-book diff --git a/Cargo.lock b/Cargo.lock index 7d0f3d35..08093eaa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2663,6 +2663,7 @@ dependencies = [ "num-bigint", "num-integer", "num-traits", + "object_store", "paste", "seq-macro", "simdutf8", @@ -2747,6 +2748,7 @@ dependencies = [ "clap", "clickhouse", "config", + "futures-util", "mimalloc", "object_store", "pb-api", diff --git a/Cargo.toml b/Cargo.toml index 054f8aac..5a533d46 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,7 +73,7 @@ bincode = "1" # Storage arrow = { version = "58", features = ["prettyprint"] } -parquet = { version = "58", features = ["async", "zstd"] } +parquet = { version = "58", features = ["async", "object_store", "zstd"] } object_store = { version = "0.13", features = ["aws"] } clickhouse = { version = "0.15", features = ["inserter"] } diff --git a/README.md b/README.md index 2076b1ad..0a5e7e37 100644 --- a/README.md +++ b/README.md @@ -237,6 +237,22 @@ just backfill cargo run -- backfill --tokens --interval-secs 60 ``` +### Recover complete Parquet hours from WAL + +```bash +# Stop ingest and every other Parquet writer first. The command verifies the +# exclusive WAL lease; standalone backfill/append writers are an operator check. +cargo run -- reconcile +``` + +Recovery fails closed on WAL corruption and partial boundary hours. For every +fully covered receive-time-partitioned UTC hour (`book_events`, `trade_events`, +and `ingest_events`) it writes and verifies a new Parquet object, publishes an +authoritative manifest, and only then cleans superseded objects. Checkpoint, +validation, and execution partitions are left untouched because their partition +timestamps do not provide the same WAL coverage proof. Local files and S3 use +the same object-store reader and manifest semantics. + ### Inspect local Parquet output ```bash diff --git a/TESTING.md b/TESTING.md index d46a6cb4..1bf8f1b4 100644 --- a/TESTING.md +++ b/TESTING.md @@ -15,13 +15,13 @@ commands so they can be re-checked instead of trusted. | # | Failure mode | Primary defense | Backstop | Runs in CI? | |---|--------------|-----------------|----------|-------------| -| 1 | Numeric precision loss or rounding drift in prices/sizes | 16 proptest properties on `FixedPrice`/`FixedSize` (roundtrip, ordering, serde) | `fuzz_fixed_price` (parse + serde roundtrip assert), 170 unit tests in pb-types | yes (`test`, `fuzz`) | +| 1 | Numeric precision loss or rounding drift in prices/sizes | 16 proptest properties on `FixedPrice`/`FixedSize` (roundtrip, ordering, serde) | `fuzz_fixed_price` (parse + serde roundtrip assert), 173 unit tests in pb-types | yes (`test`, `fuzz`) | | 2 | Order-book invariant violations (mis-sorted levels, wrong size accounting) | 17 proptest properties on `L2Book` | `fuzz_book_delta` (arbitrary snapshots + deltas, ordering asserted after every step), 74 unit tests in pb-book | yes (`test`, `fuzz`) | | 3 | Malformed venue input crashing ingest | `fuzz_ws_deser` (arbitrary bytes into the wire parser: must error, never panic) | 67 unit tests in pb-feed (dispatcher normalization, reconnect paths) | yes (`fuzz`, `test`) | -| 4 | WAL corruption on disk (bit flips, torn writes, zero-filled tails) | `fuzz_wal_corruption` (write, corrupt bytes, assert reader never panics and returns only CRC-valid records) | pb-wal unit tests for truncated-tail recovery, CRC/length corruption, prune safety (80 test attributes) + 2 proptest properties | yes (`fuzz`, `test`) | +| 4 | WAL corruption on disk (bit flips, torn writes, zero-filled tails) | `fuzz_wal_corruption` (write, corrupt bytes, assert reader never panics and returns only CRC-valid records) | pb-wal unit tests for truncated-tail recovery, CRC/length corruption, prune safety (82 test attributes) + 2 proptest properties | yes (`fuzz`, `test`) | | 5 | Silent WAL byte-format drift between builds | Golden-bytes fixture `golden_codec_book_v2_bytes_are_stable` (pb-wal): any codec change that alters encoded bytes fails the build | `fuzz_codec_decode` (arbitrary bytes into `codec::decode`: error, never panic); version byte rejected on mismatch | yes (`test`, `fuzz`) | -| 6 | Non-deterministic replay (same events, different book) | Determinism fixture `tests/integration/book_determinism.rs` (3 tests) | Golden replay regression `golden_replay_produces_expected_book` (pb-replay), 47 pb-replay test attributes | yes (`test`) | -| 7 | Parquet and ClickHouse answering the same query differently | 3 cross-backend equivalence tests (replay, integrity, execution) in `tests/integration/cross_backend_service.rs` | 5 ClickHouse round-trip tests, 1 S3/MinIO round-trip; `reconcile` rebuilds Parquet from the WAL when they do diverge | yes (`integration-docker` runs the `#[ignore]`d Docker-backed cases via testcontainers) | +| 6 | Non-deterministic replay (same events, different book) | Determinism fixture `tests/integration/book_determinism.rs` (3 tests) | Golden replay regression `golden_replay_produces_expected_book` (pb-replay), 51 pb-replay test attributes | yes (`test`) | +| 7 | Parquet and ClickHouse answering the same query differently | 3 cross-backend equivalence tests (replay, integrity, execution) in `tests/integration/cross_backend_service.rs` | 5 ClickHouse round-trip tests; S3/MinIO write + production-reader round-trip; manifest-switch and partial-hour refusal tests for `reconcile` | yes (`integration-docker` runs the `#[ignore]`d Docker-backed cases) | | 8 | SQL escaping the read-only query workbench | `fuzz_query_guard` (guarded SQL must be a fixed point of the guard) | Guard unit tests in pb-service/pb-api; server-side readonly + LIMIT enforcement | yes (`fuzz`, `test`) | | 9 | Crash/restart data loss across the ingest/serve boundary | 2 checkpoint + WAL hydration integration tests (`checkpoint_wal_hydration.rs`) | pb-wal reopen-recovery and position-file tests; standby-writer flock takeover test | yes (`test`) | | 10 | Undefined behavior / memory unsafety | Miri on pb-types and pb-book unit tests | Workspace is overwhelmingly safe Rust; `clippy -D warnings` | yes (`miri`) | @@ -46,14 +46,14 @@ Test attributes (`#[test]` + `#[tokio::test]`) per crate at this revision: | Crate | Tests | Crate | Tests | |-------|------:|-------|------:| -| pb-types | 170 | pb-replay | 47 | -| pb-api | 81 | pb-store | 36 | -| pb-wal | 80 | pb-grpc | 25 | +| pb-types | 173 | pb-replay | 51 | +| pb-api | 83 | pb-store | 41 | +| pb-wal | 82 | pb-grpc | 25 | | pb-book | 74 | pb-metrics | 14 | -| pb-bin | 74 | pb-service | 72 | +| pb-bin | 75 | pb-service | 72 | | pb-feed | 67 | `tests/integration` | 22 | -Total: 762 test attributes. These counts include `#[ignore]`d tests: one in +Total: 779 test attributes. These counts include `#[ignore]`d tests: one in pb-wal (a failover timing drill, run via `just failover-drill`) and nine in the integration package (see below). @@ -62,10 +62,12 @@ The integration package (`tests/integration/`) splits cleanly: - **Run in CI** (13 tests): book determinism (3), checkpoint + WAL hydration (2), dispatcher pipeline (2), Parquet round-trip (2), replay engine (2), schema conversion (2). -- **`#[ignore]`d, Docker-backed, not run in CI** (9 tests): ClickHouse +- **`#[ignore]`d, Docker-backed, run by the `integration-docker` CI job** (9 tests): ClickHouse round-trips (5), cross-backend Parquet/ClickHouse equivalence for replay, - integrity, and execution (3), and an S3/MinIO round-trip (1). They use - `testcontainers` and require a local Docker daemon: + integrity, and execution (3), and an S3/MinIO round-trip through the production + `ParquetReader` (1). The ClickHouse cases use `testcontainers`; the S3 case + requires a running MinIO/S3-compatible endpoint in `PB_TEST_S3_ENDPOINT` (CI + starts MinIO explicitly). Run all nine locally with: `cargo test -p pb-integration-tests -- --ignored`. ### Property-based tests @@ -187,11 +189,21 @@ npx vite build && npx playwright test # what the e2e job runs Being explicit about what is *not* covered: -- **No kill -9 crash-recovery end-to-end test.** Torn-tail truncation, CRC - skipping, and reopen recovery are unit-tested by constructing damaged - segments directly, and the flock standby-takeover path has a test — but no +- **No kill -9 crash-recovery end-to-end test.** Torn-tail truncation, live-reader + CRC skipping, strict recovery rejection, manifest publication, and reopen + recovery are unit-tested by constructing damaged segments and storage views + directly, and the flock standby-takeover path has a test — but no harness SIGKILLs a live `ingest` mid-append and asserts the full recover-and-reconcile path end to end. +- **Recovery is intentionally scoped.** Crash-consistent replacement is tested + for receive-time-partitioned book/trade/ingest hours. Checkpoint, validation, + and execution partitions are rejected because WAL endpoint timestamps cannot + prove those independently partitioned datasets complete. +- Recovery reader coverage includes an asset containing `/` and `%`, catching + double-encoding bugs across object filenames, manifests, and cleanup paths. +- Reconcile validates the whole retained WAL without retaining it, rejects an + eligible hour above 2,000,000 records or 128 MiB of encoded WAL payload, then + bounds decoded recovery memory to one eligible hour at a time. - **No soak or load testing.** There is no sustained multi-hour run under production-like message rates; channel capacities are sized by reasoning plus the exported depth gauge, not by a measured burst profile. diff --git a/crates/pb-api/README.md b/crates/pb-api/README.md index 0190eb3f..cb2781d0 100644 --- a/crates/pb-api/README.md +++ b/crates/pb-api/README.md @@ -75,6 +75,12 @@ AnyQueryService ──▶ query workbench handlers (datasets, SQL) delta, and checkpoint apply it runs `L2Book::check_integrity`; a crossed/locked book increments `pb_crossed_books_total` and surfaces a `crossed_book` continuity warning instead of being served silently. +- Separated `serve` hydration inventories Parquet checkpoints once for the full + active-asset set and replays the retained WAL with a distinct checkpoint + offset per asset. An asset without a checkpoint forces replay from the + earliest retained WAL record; a legacy checkpoint without a WAL cut is + ignored when WAL replay is configured. `/health/ready` stays at 503 when checkpoint + inventory, WAL open/read, or record decoding fails. - The projector keeps a cached published projection and only rebuilds `AssetReadView` snapshots for assets touched by a record, instead of re-materializing every tracked book on each update. @@ -108,6 +114,6 @@ AnyQueryService ──▶ query workbench handlers (datasets, SQL) ## Tests -68 tests covering health states, error format consistency, depth and time parameter +83 tests covering health states, error format consistency, depth and time parameter validation, execution limits, `ServiceError` to `ApiError` mapping, `PerAssetBroadcast` unit tests, and `LiveReadModel` tests. diff --git a/crates/pb-api/src/hydration.rs b/crates/pb-api/src/hydration.rs index 2fb2cdf5..bd55d36a 100644 --- a/crates/pb-api/src/hydration.rs +++ b/crates/pb-api/src/hydration.rs @@ -7,8 +7,11 @@ //! all subsequent records through the projector. //! 4. Mark the read model as hydrated (ready to serve). //! -//! Fallback: if no checkpoints exist, tail WAL from earliest offset. -//! If no WAL exists, fall back to feed-only mode (mark hydrated immediately). +//! Fallback: if no checkpoints exist, tail WAL from earliest offset. A +//! configured but missing/unreadable WAL keeps the standalone serve runtime +//! unready instead of presenting an empty state as healthy. + +use std::collections::HashMap; use pb_types::AssetId; use tracing::{info, warn}; @@ -26,13 +29,15 @@ pub struct HydrationResult { pub wal_resume_offset: Option, /// The exact WAL position after hydration replay finishes. pub wal_end_position: Option, + /// Whether every configured recovery source completed without error. + pub recovery_succeeded: bool, } /// Hydrate the read model from the latest checkpoint per asset, then replay /// any WAL records written after the checkpoint. /// -/// If `wal_path` is `None` or the WAL directory doesn't exist, skips WAL -/// replay and marks hydrated immediately. +/// If `wal_config` is `None`, the caller explicitly requested feed-only mode. +/// A configured but missing WAL is a recovery failure and remains unready. /// /// If no reader is provided or no checkpoints exist, skips checkpoint loading. pub async fn hydrate( @@ -46,71 +51,103 @@ pub async fn hydrate( wal_records_replayed: 0, wal_resume_offset: None, wal_end_position: None, + recovery_succeeded: true, }; + if wal_config.is_some() { + model.mark_unhydrated().await; + } + // Phase 1: Load latest checkpoint per asset. - let mut min_wal_offset: Option = None; + let mut checkpoint_offsets = HashMap::::new(); if let Some(reader) = reader { let now_us = now_us(); - for asset_id_str in active_assets { - let asset_id = AssetId::new(asset_id_str.as_str()); - match reader.read_latest_checkpoint(&asset_id, now_us).await { - Ok(Some(checkpoint)) => { - if let Some(offset) = checkpoint.wal_offset { - min_wal_offset = Some(match min_wal_offset { - Some(current) => current.min(offset), - None => offset, - }); + let asset_ids = active_assets + .iter() + .map(|asset_id| AssetId::new(asset_id.as_str())) + .collect::>(); + match reader.read_latest_checkpoints(&asset_ids, now_us).await { + Ok(checkpoints) => { + for (asset_id_str, checkpoint) in active_assets.iter().zip(checkpoints) { + if let Some(checkpoint) = checkpoint { + if wal_config.is_some() && checkpoint.wal_offset.is_none() { + warn!( + asset_id = %asset_id_str, + checkpoint_ts = checkpoint.checkpoint_timestamp_us, + "checkpoint has no WAL cut; ignoring it and replaying retained WAL" + ); + continue; + } + if let Some(offset) = checkpoint.wal_offset { + checkpoint_offsets.insert(asset_id_str.clone(), offset); + } + info!( + asset_id = %asset_id_str, + checkpoint_ts = checkpoint.checkpoint_timestamp_us, + wal_offset = ?checkpoint.wal_offset, + "hydrating from checkpoint" + ); + model.hydrate_checkpoint(checkpoint).await; + result.checkpoints_loaded += 1; + } else { + info!( + asset_id = %asset_id_str, + "no checkpoint found, starting empty" + ); } - info!( - asset_id = %asset_id_str, - checkpoint_ts = checkpoint.checkpoint_timestamp_us, - wal_offset = ?checkpoint.wal_offset, - "hydrating from checkpoint" - ); - model.hydrate_checkpoint(checkpoint).await; - result.checkpoints_loaded += 1; - } - Ok(None) => { - info!( - asset_id = %asset_id_str, - "no checkpoint found, starting empty" - ); - } - Err(e) => { - warn!( - asset_id = %asset_id_str, - error = %e, - "failed to read checkpoint, starting empty" - ); } } + Err(e) => { + result.recovery_succeeded = false; + warn!( + error = %e, + assets = active_assets.len(), + "failed to inventory checkpoints, starting from retained WAL" + ); + } } } + // A global fast-forward is safe only when every active asset has a durable + // checkpoint cut. Otherwise start at the earliest retained WAL record so an + // asset without a checkpoint is not silently left empty. Per-asset cutoffs + // below still prevent older records from being applied on top of newer + // checkpoints whose offsets differ. + let wal_resume_offset = (!active_assets.is_empty() + && active_assets + .iter() + .all(|asset_id| checkpoint_offsets.contains_key(asset_id))) + .then(|| checkpoint_offsets.values().copied().min()) + .flatten(); + // Phase 2: Replay WAL tail from the minimum checkpoint offset, using the // operator-configured WalConfig so the global-offset math matches the // writer's `global_offset()` (a hardcoded default segment size mis-skipped // records under a non-default `wal.segment_size_mb`). if let Some(cfg) = wal_config { if cfg.base_path.exists() { - let (wal_records_replayed, wal_end_position) = - replay_wal_tail(model, cfg, min_wal_offset).await; + let (wal_records_replayed, wal_end_position, replay_succeeded) = + replay_wal_tail(model, cfg, wal_resume_offset, &checkpoint_offsets).await; result.wal_records_replayed = wal_records_replayed; - result.wal_resume_offset = min_wal_offset; + result.wal_resume_offset = wal_resume_offset; result.wal_end_position = wal_end_position; + result.recovery_succeeded &= replay_succeeded; } else { - info!(path = %cfg.base_path.display(), "WAL directory not found, skipping WAL replay"); + result.recovery_succeeded = false; + warn!(path = %cfg.base_path.display(), "WAL directory not found; serve remains unready"); } } - // Phase 3: Mark hydrated. - model.mark_hydrated().await; + // Phase 3: only trustworthy recovery is ready to serve. + if result.recovery_succeeded { + model.mark_hydrated().await; + } info!( checkpoints = result.checkpoints_loaded, wal_records = result.wal_records_replayed, + recovery_succeeded = result.recovery_succeeded, "hydration complete" ); @@ -125,14 +162,15 @@ async fn replay_wal_tail( model: &LiveReadModel, config: &pb_wal::WalConfig, from_global_offset: Option, -) -> (usize, Option) { + checkpoint_offsets: &HashMap, +) -> (usize, Option, bool) { let config = config.clone(); let mut reader = match pb_wal::WalReader::open(config.clone(), "serve-hydration") { Ok(r) => r, Err(e) => { warn!(error = %e, "failed to open WAL reader for hydration"); - return (0, None); + return (0, None, false); } }; @@ -142,6 +180,7 @@ async fn replay_wal_tail( let skip_to = from_global_offset.unwrap_or(0); let mut records_replayed = 0; let mut records_skipped = 0; + let mut replay_succeeded = true; loop { match reader.next() { @@ -159,16 +198,26 @@ async fn replay_wal_tail( // Decode the versioned WAL payload. match pb_wal::codec::decode(&payload) { Ok(record) => { + if checkpoint_offsets + .get(record.asset_partition()) + .is_some_and(|offset| current_global <= *offset) + { + records_skipped += 1; + continue; + } model.apply_record(record).await; records_replayed += 1; } Err(e) => { - warn!(error = %e, "failed to decode WAL record during hydration, skipping"); + replay_succeeded = false; + warn!(error = %e, "failed to decode WAL record during hydration; serve remains unready"); + break; } } } Ok(None) => break, // Caught up to head. Err(e) => { + replay_succeeded = false; warn!(error = %e, "WAL read error during hydration, stopping replay"); break; } @@ -182,7 +231,11 @@ async fn replay_wal_tail( ); } - (records_replayed, Some(reader.current_position())) + ( + records_replayed, + Some(reader.current_position()), + replay_succeeded, + ) } fn now_us() -> u64 { @@ -219,6 +272,14 @@ mod tests { }) } + fn snapshot_record_for_asset(asset_id: &str, price: u32, seq: u64) -> PersistedRecord { + let mut record = snapshot_record(Side::Bid, price, 10.0, seq); + if let PersistedRecord::Book(event) = &mut record { + event.asset_id = AssetId::new(asset_id); + } + record + } + fn delta_record() -> PersistedRecord { PersistedRecord::Book(BookEvent { asset_id: AssetId::new("tok1"), @@ -274,7 +335,9 @@ mod tests { let live = LiveReadModel::new(crate::dto::FeedMode::FixedTokens); live.set_active_assets(vec!["tok1".to_string()]).await; - let (replayed, _pos) = replay_wal_tail(&live, &config, Some(cutoff)).await; + let (replayed, _pos, succeeded) = + replay_wal_tail(&live, &config, Some(cutoff), &HashMap::new()).await; + assert!(succeeded); // Only the four records appended after the cutoff are replayed. With the // old hardcoded 64 MB segment size, nothing would be skipped and all // seven would replay — this asserts the configured size is used. @@ -308,13 +371,57 @@ mod tests { let live = LiveReadModel::new(crate::dto::FeedMode::FixedTokens); live.set_active_assets(vec!["tok1".to_string()]).await; - let (replayed, _pos) = replay_wal_tail(&live, &config, Some(cutoff)).await; + let (replayed, _pos, succeeded) = + replay_wal_tail(&live, &config, Some(cutoff), &HashMap::new()).await; + assert!(succeeded); assert_eq!( replayed, 1, "record at the exact checkpoint offset must be skipped, only the later record replayed" ); } + #[tokio::test] + async fn replay_wal_tail_honors_each_assets_checkpoint_offset() { + let dir = tempfile::tempdir().unwrap(); + let config = pb_wal::WalConfig { + base_path: dir.path().to_path_buf(), + segment_size: 4096, + ..pb_wal::WalConfig::default() + }; + let mut writer = pb_wal::WalWriter::open(config.clone()).unwrap(); + writer + .append(&pb_wal::codec::encode(&snapshot_record_for_asset("tok1", 5000, 0)).unwrap()) + .unwrap(); + let tok1_cutoff = writer.global_offset(); + writer + .append(&pb_wal::codec::encode(&snapshot_record_for_asset("tok1", 5001, 1)).unwrap()) + .unwrap(); + writer + .append(&pb_wal::codec::encode(&snapshot_record_for_asset("tok2", 6000, 0)).unwrap()) + .unwrap(); + let tok2_cutoff = writer.global_offset(); + writer + .append(&pb_wal::codec::encode(&snapshot_record_for_asset("tok2", 6001, 1)).unwrap()) + .unwrap(); + writer.flush().unwrap(); + + let live = LiveReadModel::new(crate::dto::FeedMode::FixedTokens); + live.set_active_assets(vec!["tok1".to_string(), "tok2".to_string()]) + .await; + let checkpoint_offsets = HashMap::from([ + ("tok1".to_string(), tok1_cutoff), + ("tok2".to_string(), tok2_cutoff), + ]); + + let (replayed, _position, succeeded) = + replay_wal_tail(&live, &config, Some(tok1_cutoff), &checkpoint_offsets).await; + assert!(succeeded); + assert_eq!( + replayed, 2, + "tok2 records already represented by its newer checkpoint must not be applied over it" + ); + } + #[tokio::test] async fn hydration_returns_end_position_for_live_handoff() { let dir = tempfile::tempdir().unwrap(); @@ -363,4 +470,22 @@ mod tests { assert_eq!(decoded, delta); assert!(reader.next().unwrap().is_none()); } + + #[tokio::test] + async fn configured_missing_wal_keeps_model_unready() { + let dir = tempfile::tempdir().unwrap(); + let config = pb_wal::WalConfig { + base_path: dir.path().join("missing-wal"), + ..pb_wal::WalConfig::default() + }; + let live = LiveReadModel::new(crate::dto::FeedMode::FixedTokens); + live.set_active_assets(vec!["tok1".to_string()]).await; + + let result = + hydrate::(&live, None, Some(&config), &["tok1".to_string()]) + .await; + + assert!(!result.recovery_succeeded); + assert!(!live.is_hydrated()); + } } diff --git a/crates/pb-api/src/live_state.rs b/crates/pb-api/src/live_state.rs index 2885558a..f74cbbb1 100644 --- a/crates/pb-api/src/live_state.rs +++ b/crates/pb-api/src/live_state.rs @@ -519,8 +519,8 @@ enum ProjectorCommand { ConfigureBroadcast(crate::streaming::PerAssetBroadcast, usize), /// Apply a checkpoint directly (used during hydration). HydrateCheckpoint(BookCheckpoint, oneshot::Sender<()>), - /// Mark the read model as hydrated (ready to serve). - MarkHydrated(oneshot::Sender<()>), + /// Set whether the read model has a trustworthy hydrated state. + SetHydrated(bool, oneshot::Sender<()>), } // --------------------------------------------------------------------------- @@ -576,9 +576,9 @@ impl Projector { self.publish(); let _ = ack.send(()); } - ProjectorCommand::MarkHydrated(ack) => { - self.state.hydrated = true; - self.published.hydrated = true; + ProjectorCommand::SetHydrated(hydrated, ack) => { + self.state.hydrated = hydrated; + self.published.hydrated = hydrated; self.publish(); let _ = ack.send(()); } @@ -862,7 +862,17 @@ impl LiveReadModel { let (ack_tx, ack_rx) = oneshot::channel(); let _ = self .cmd_tx - .send(ProjectorCommand::MarkHydrated(ack_tx)) + .send(ProjectorCommand::SetHydrated(true, ack_tx)) + .await; + let _ = ack_rx.await; + } + + /// Mark the read model unready while recovery is incomplete or failed. + pub async fn mark_unhydrated(&self) { + let (ack_tx, ack_rx) = oneshot::channel(); + let _ = self + .cmd_tx + .send(ProjectorCommand::SetHydrated(false, ack_tx)) .await; let _ = ack_rx.await; } diff --git a/crates/pb-bin/Cargo.toml b/crates/pb-bin/Cargo.toml index 116ad2cc..a5f1a26d 100644 --- a/crates/pb-bin/Cargo.toml +++ b/crates/pb-bin/Cargo.toml @@ -38,6 +38,7 @@ serde = { workspace = true } serde_json = { workspace = true } anyhow = { workspace = true } object_store = { workspace = true } +futures-util = { workspace = true } url = { workspace = true } clickhouse = { workspace = true } reqwest = { workspace = true } diff --git a/crates/pb-bin/README.md b/crates/pb-bin/README.md index 43b49feb..0b0f0fef 100644 --- a/crates/pb-bin/README.md +++ b/crates/pb-bin/README.md @@ -17,7 +17,7 @@ to the appropriate subsystem. | `backfill` | Periodic REST API snapshot backfill for checkpoint seeding. | | `serve-api` | Start the read-only API server with live feed and replay access. | | `serve` | Start the read-only serve runtime (WAL reader + checkpoint hydration + HTTP/WS). | -| `reconcile` | Offline recovery: rebuild Parquet partitions from the durable WAL after a crash lost a buffered window. Idempotent (per-partition replace). | +| `reconcile` | Offline recovery: strictly rebuild only complete Parquet hours proven by the retained WAL; boundary hours and corruption fail closed. | ## Config Layering @@ -62,11 +62,14 @@ Example: `PB__STORAGE__CLICKHOUSE_URL=http://localhost:8123` continuing with a dead component or exiting 0. In `auto_ingest` the rotating per-market feed generations are deliberately *not* supervised this way (their cycling is the expected steady state and is managed via `Generation`). -- **WAL→storage reconciliation**: `reconcile` reads the durable WAL and rebuilds - the Parquet partitions it covers via `ParquetRecordWriter::write_batch_replacing` - (per-`(dataset, asset, hour)` delete-then-write), so a storage window lost when - a crash dropped the in-memory Parquet buffer is recoverable from the WAL. - Run it offline (ingest stopped); it is idempotent. +- **WAL→storage reconciliation**: with all Parquet-writing tasks stopped, + `reconcile` acquires the exclusive WAL lease, + starts at the earliest retained segment, and aborts on CRC, segment, or decode + damage. It skips boundary hours and non-receive-time-partitioned datasets, then + publishes complete book/trade/ingest hours through crash-consistent manifests + before cleaning old objects. Running while ingest is active is refused; + standalone append/backfill writers are an operator-enforced stop condition; + re-running is idempotent. ## Docs to Update After Changes diff --git a/crates/pb-bin/src/commands/doctor.rs b/crates/pb-bin/src/commands/doctor.rs index e5f9c28a..f4d1d380 100644 --- a/crates/pb-bin/src/commands/doctor.rs +++ b/crates/pb-bin/src/commands/doctor.rs @@ -5,6 +5,7 @@ use std::time::{Duration, Instant}; use anyhow::Result; use config::Config; +use object_store::ObjectStoreExt; use crate::config_validation; @@ -16,7 +17,7 @@ pub async fn run(settings: Config, skip_network: bool) -> Result<()> { let mut checks: Vec = Vec::new(); checks.push(check_config_keys(&settings)); - checks.push(check_parquet_path(&settings)); + checks.push(check_parquet_path(&settings).await); checks.push(check_wal_dir(&settings)); if skip_network { @@ -143,24 +144,70 @@ fn check_config_keys(settings: &Config) -> Check { } } -fn check_parquet_path(settings: &Config) -> Check { +async fn check_parquet_path(settings: &Config) -> Check { let base = settings .get_string("storage.parquet_base_path") .unwrap_or_else(|_| "./data".to_string()); - let path = Path::new(&base); - if !path.exists() { - return Check::warn( - "parquet", - format!("{base} does not exist yet (created on first run)"), - ); - } - let probe = path.join(".doctor-write-probe"); - match std::fs::write(&probe, b"ok") { - Ok(()) => { - let _ = std::fs::remove_file(&probe); - Check::pass("parquet", format!("{base} writable")) + let (store, prefix) = match super::pipeline::build_object_store(&base) { + Ok(result) => result, + Err(error) => { + return Check::fail("parquet", format!("{base} configuration failed: {error}")); + } + }; + let nonce = format!( + ".doctor-write-probe-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + ); + let encoded = if prefix.is_empty() { + nonce + } else { + format!("{prefix}/{nonce}") + }; + let probe = match object_store::path::Path::parse(&encoded) { + Ok(path) => path, + Err(error) => { + return Check::fail( + "parquet", + format!("{base} has invalid object prefix: {error}"), + ); } - Err(e) => Check::fail("parquet", format!("{base} not writable: {e}")), + }; + + let probe_result = tokio::time::timeout(Duration::from_secs(10), async { + store + .put( + &probe, + object_store::PutPayload::from_static(b"poly-book-doctor"), + ) + .await?; + let bytes = store.get(&probe).await?.bytes().await?; + if bytes.as_ref() != b"poly-book-doctor" { + return Err(object_store::Error::Generic { + store: "doctor", + source: Box::new(std::io::Error::other( + "storage probe returned different bytes", + )), + }); + } + store.delete(&probe).await?; + Ok::<(), object_store::Error>(()) + }) + .await; + + match probe_result { + Ok(Ok(())) => Check::pass("parquet", format!("{base} write/read/delete probe passed")), + Ok(Err(error)) => Check::fail( + "parquet", + format!( + "{base} probe failed: {}", + super::pipeline::object_store_error_summary(&error) + ), + ), + Err(_) => Check::fail("parquet", format!("{base} probe timed out after 10s")), } } diff --git a/crates/pb-bin/src/commands/execution_replay.rs b/crates/pb-bin/src/commands/execution_replay.rs index 9509c35f..2121ad2c 100644 --- a/crates/pb-bin/src/commands/execution_replay.rs +++ b/crates/pb-bin/src/commands/execution_replay.rs @@ -21,7 +21,8 @@ pub async fn run( let base_path = settings .get_string("storage.parquet_base_path") .unwrap_or_else(|_| "./data".to_string()); - let reader = pb_replay::ParquetReader::new(&base_path); + let (store, base_path) = super::pipeline::build_object_store(&base_path)?; + let reader = pb_replay::ParquetReader::from_store(store, base_path); let engine = pb_replay::ReplayEngine::new(reader); engine .execution_events(order_id.as_deref(), start_us, end_us) diff --git a/crates/pb-bin/src/commands/pipeline.rs b/crates/pb-bin/src/commands/pipeline.rs index befdab2e..f87657e4 100644 --- a/crates/pb-bin/src/commands/pipeline.rs +++ b/crates/pb-bin/src/commands/pipeline.rs @@ -4,6 +4,7 @@ use std::time::Duration; use anyhow::{bail, Result}; use config::Config; +use futures_util::StreamExt; use tokio::sync::mpsc; use tokio::task::JoinHandle; use tokio_util::sync::CancellationToken; @@ -173,13 +174,13 @@ impl Supervisor { /// Build the object store and path prefix for a configured storage base path. /// -/// A path with a URL scheme (`s3://bucket/prefix`, `gs://...`, `file://...`) is -/// wired to the matching `object_store` backend; a plain path is a local -/// filesystem directory. This fixes a bug where an +/// An `s3://bucket/prefix` or `file://...` URL is wired to the matching compiled +/// `object_store` backend; a plain path is a local filesystem directory. This +/// fixes a bug where an /// `s3://...` base path was silently handled by `LocalFileSystem` and written to /// a local directory literally named `s3:` on ephemeral container storage. /// -/// For `s3://` (and other cloud schemes), configuration is taken from process +/// For `s3://`, configuration is taken from process /// environment variables via `object_store::parse_url_opts(url, std::env::vars())`. /// This is what wires in region and credentials: `AWS_REGION`, static /// `AWS_ACCESS_KEY_ID`/`AWS_SECRET_ACCESS_KEY`, or — when no static keys are set — @@ -210,6 +211,46 @@ pub fn build_object_store(base_path: &str) -> Result<(Arc &'static str { + match error { + object_store::Error::NotFound { .. } => "object or bucket not found", + object_store::Error::PermissionDenied { .. } => "permission denied", + object_store::Error::Unauthenticated { .. } => "authentication failed", + object_store::Error::InvalidPath { .. } => "invalid object path", + object_store::Error::NotSupported { .. } | object_store::Error::NotImplemented { .. } => { + "operation not supported" + } + object_store::Error::AlreadyExists { .. } => "object already exists", + object_store::Error::Precondition { .. } => "request precondition failed", + object_store::Error::NotModified { .. } => "object not modified", + object_store::Error::UnknownConfigurationKey { .. } => "invalid backend configuration", + _ => "backend request failed", + } +} + +/// Fail startup when the configured historical object store cannot even list +/// its prefix. An empty prefix is valid; auth, endpoint, or bucket failures are +/// not. Reads will otherwise look like an empty dataset and readiness can turn +/// green on a misconfigured deployment. +async fn probe_object_store_read( + store: &Arc, + prefix: &str, +) -> Result<()> { + let path = object_store::path::Path::parse(prefix) + .map_err(|error| anyhow::anyhow!("invalid object-store prefix {prefix}: {error}"))?; + let mut objects = store.list(Some(&path)); + match tokio::time::timeout(Duration::from_secs(10), objects.next()).await { + Ok(Some(Ok(_))) | Ok(None) => Ok(()), + Ok(Some(Err(error))) => Err(anyhow::anyhow!( + "cannot list configured Parquet prefix {prefix}: {}", + object_store_error_summary(&error) + )), + Err(_) => Err(anyhow::anyhow!( + "timed out listing configured Parquet prefix {prefix} after 10s" + )), + } +} + pub async fn start_storage_sinks( settings: &Config, enable_parquet: bool, @@ -479,11 +520,11 @@ pub fn redact_url_for_log(raw: &str) -> String { /// back to Parquet with a warning. pub async fn build_services( settings: &Config, -) -> ( +) -> Result<( pb_service::AnyReplayService, pb_service::AnyIntegrityService, pb_service::AnyExecutionService, -) { +)> { let backend = settings .get_string("api.historical_backend") .unwrap_or_else(|_| "parquet".to_string()); @@ -517,7 +558,7 @@ pub async fn build_services( database = %ch_db, "using ClickHouse historical backend" ); - return ( + return Ok(( pb_service::AnyReplayService::ClickHouse( pb_service::ClickHouseReplayService::new(&ch_url, &ch_db), ), @@ -527,7 +568,7 @@ pub async fn build_services( pb_service::AnyExecutionService::ClickHouse( pb_service::ClickHouseExecutionService::new(&ch_url, &ch_db), ), - ); + )); } Ok(Err(e)) => { tracing::warn!( @@ -554,23 +595,29 @@ pub async fn build_services( } tracing::info!(path = %parquet_base_path, "using Parquet historical backend"); - ( - pb_service::AnyReplayService::Parquet(pb_service::ParquetReplayService::new( - &parquet_base_path, + let (store, base_path) = build_object_store(&parquet_base_path)?; + probe_object_store_read(&store, &base_path).await?; + let reader = pb_replay::ParquetReader::from_store(store, base_path); + Ok(( + pb_service::AnyReplayService::Parquet(pb_service::ParquetReplayService::from_reader( + reader.clone(), )), - pb_service::AnyIntegrityService::Parquet(pb_service::ParquetIntegrityService::new( - &parquet_base_path, + pb_service::AnyIntegrityService::Parquet(pb_service::ParquetIntegrityService::from_reader( + reader.clone(), )), - pb_service::AnyExecutionService::Parquet(pb_service::ParquetExecutionService::new( - &parquet_base_path, + pb_service::AnyExecutionService::Parquet(pb_service::ParquetExecutionService::from_reader( + reader, )), - ) + )) } /// Build the query service from config. /// /// Returns `None` if `api.query_workbench_enabled` is not set to `true`. -pub async fn build_query_service(settings: &Config) -> Option { +pub async fn build_query_service( + settings: &Config, + effective_backend_is_clickhouse: bool, +) -> Option { let enabled = settings .get_bool("api.query_workbench_enabled") .unwrap_or(false); @@ -579,33 +626,26 @@ pub async fn build_query_service(settings: &Config) -> Option { - let ch_url = settings - .get_string("storage.clickhouse_url") - .unwrap_or_else(|_| "http://localhost:8123".to_string()); - let ch_db = settings - .get_string("storage.clickhouse_database") - .unwrap_or_else(|_| "poly_book".to_string()); - tracing::info!( - url = %redact_url_for_log(&ch_url), - database = %ch_db, - "query workbench enabled (ClickHouse)" - ); - Some(pb_service::AnyQueryService::ClickHouse( - pb_service::ClickHouseQueryService::new(&ch_url, &ch_db), - )) - } - _ => { - tracing::warn!( - "query workbench requires clickhouse backend, currently using {backend}" - ); - None - } + if effective_backend_is_clickhouse { + let ch_url = settings + .get_string("storage.clickhouse_url") + .unwrap_or_else(|_| "http://localhost:8123".to_string()); + let ch_db = settings + .get_string("storage.clickhouse_database") + .unwrap_or_else(|_| "poly_book".to_string()); + tracing::info!( + url = %redact_url_for_log(&ch_url), + database = %ch_db, + "query workbench enabled (ClickHouse)" + ); + Some(pb_service::AnyQueryService::ClickHouse( + pb_service::ClickHouseQueryService::new(&ch_url, &ch_db), + )) + } else { + tracing::warn!( + "query workbench requires the effective historical backend to be ClickHouse" + ); + None } } @@ -972,6 +1012,23 @@ mod tests { assert_eq!(timeout, 60); } + #[tokio::test] + async fn query_service_uses_the_effective_backend_after_fallback() { + let settings = Config::builder() + .set_override("api.query_workbench_enabled", true) + .unwrap() + .set_override("api.historical_backend", "clickhouse") + .unwrap() + .build() + .unwrap(); + + assert!(build_query_service(&settings, false).await.is_none()); + assert!(matches!( + build_query_service(&settings, true).await, + Some(pb_service::AnyQueryService::ClickHouse(_)) + )); + } + // --- grpc_config_from_settings --- #[test] diff --git a/crates/pb-bin/src/commands/reconcile.rs b/crates/pb-bin/src/commands/reconcile.rs index f3103925..25cb6e6e 100644 --- a/crates/pb-bin/src/commands/reconcile.rs +++ b/crates/pb-bin/src/commands/reconcile.rs @@ -3,52 +3,96 @@ use config::Config; use super::pipeline; -/// Rebuild Parquet storage partitions from the durable WAL. +const HOUR_US: u64 = 3_600_000_000; +// Recovery runs in the same 512 MiB class used by the default ECS task. A +// compressed WAL hour can expand substantially when decoded plus Arrow encoded, +// so reject oversized hours before publishing any partition. +const MAX_RECOVERY_HOUR_ENCODED_BYTES: u64 = 128 * 1024 * 1024; +const MAX_RECOVERY_HOUR_RECORDS: usize = 2_000_000; + +#[derive(Default)] +struct HourSummary { + records: usize, + encoded_bytes: u64, +} + +fn is_receive_time_partitioned(record: &pb_types::PersistedRecord) -> bool { + matches!( + record, + pb_types::PersistedRecord::Book(_) + | pb_types::PersistedRecord::Trade(_) + | pb_types::PersistedRecord::Ingest(_) + ) +} + +/// Rebuild complete Parquet hour partitions from the durable WAL. /// /// The Parquet sink buffers up to a flush interval (default 5 minutes) in memory; /// a crash, OOM, or SIGKILL loses that window from storage. The WAL captures the /// same records durably (fsynced), but nothing replayed it back into Parquet — so -/// the loss became permanent. This command reads the retained WAL and rewrites -/// every `(dataset, asset, hour)` partition it covers, making the WAL the -/// authoritative recovery source. +/// the loss became permanent. This command reads the retained WAL strictly and +/// only publishes hours fully spanned by that retained stream. /// -/// Run this **offline** (with ingest stopped) after a crash or to backfill a -/// storage gap: a concurrent live sink writing the same partitions would race the -/// per-group replace. Re-running is idempotent (each touched partition is deleted -/// and rewritten from the WAL), so it is safe to run more than once. +/// Run this **offline** after a crash or to backfill a storage gap, with every +/// process that can write this Parquet prefix stopped. The command can enforce +/// that the WAL ingest writer is offline by acquiring its lease; standalone +/// backfill/append processes must be stopped operationally. Boundary hours, WAL +/// gaps, CRC errors, and decode failures fail closed. Each recovered object is +/// verified before a manifest atomically publishes it, so re-running is +/// idempotent and a crash never exposes a delete-before-write window. pub async fn run(settings: Config) -> Result<()> { let wal_config = pipeline::wal_config_from_settings(&settings); + let _maintenance_guard = pb_wal::WalMaintenanceGuard::acquire(&wal_config) + .map_err(|e| anyhow::anyhow!("reconcile requires an offline WAL: {e}"))?; let parquet_base = settings .get_string("storage.parquet_base_path") .unwrap_or_else(|_| "./data".to_string()); let (store, base_path) = pipeline::build_object_store(&parquet_base)?; let writer = pb_store::ParquetRecordWriter::new(store, base_path); - // A dedicated consumer name so reconcile never disturbs the serve-live - // tailer's position. We deliberately do NOT commit this consumer's position: - // reconcile rebuilds the entire retained WAL each run, and the replace - // semantics make that idempotent. - let mut reader = pb_wal::WalReader::open(wal_config, "reconcile") - .map_err(|e| anyhow::anyhow!("failed to open WAL reader for reconcile: {e}"))?; - - if reader.needs_resync() { - tracing::warn!( - "WAL reconcile consumer is behind the earliest retained segment; \ - reconciling only what remains in the WAL" - ); - } + // Ignore any stale maintenance cursor and inspect the entire retained WAL. + // The strict reader refuses corruption and internal segment gaps instead of + // silently making an incomplete stream authoritative. + let mut reader = + pb_wal::WalReader::open_from_start(wal_config.clone(), "reconcile-validate") + .map_err(|e| anyhow::anyhow!("failed to open WAL reader for reconcile: {e}"))?; - let mut records: Vec = Vec::new(); - let mut decode_failures = 0u64; + // Pass 1 validates every retained frame and derives coverage without keeping + // the decoded WAL in memory. Retention can exceed the ECS task's memory, so + // materializing the whole stream would make recovery itself an OOM risk. + let mut wal_records = 0usize; + let mut supported_hours = std::collections::BTreeMap::::new(); + let mut unsupported_records_skipped = 0usize; + let mut coverage_start_us = None; + let mut coverage_end_us = None; loop { - match reader.next() { - Ok(Some(payload)) => match pb_wal::codec::decode(&payload) { - Ok(record) => records.push(record), - Err(e) => { - decode_failures += 1; - tracing::warn!(error = %e, "skipping undecodable WAL record during reconcile"); + match reader.next_strict() { + Ok(Some(payload)) => { + wal_records += 1; + let encoded_bytes = payload.len() as u64; + let record = pb_wal::codec::decode(&payload).map_err(|e| { + anyhow::anyhow!("refusing recovery after undecodable WAL record: {e}") + })?; + if !is_receive_time_partitioned(&record) { + unsupported_records_skipped += 1; + continue; + } + let timestamp_us = record + .recv_timestamp_us() + .expect("receive-time-partitioned records carry provenance"); + if coverage_end_us.is_some_and(|previous| timestamp_us < previous) { + anyhow::bail!( + "unsafe recovery disabled: receive timestamps move backwards at {timestamp_us}; no Parquet data changed" + ); } - }, + coverage_start_us.get_or_insert(timestamp_us); + coverage_end_us = Some(timestamp_us); + let summary = supported_hours + .entry(timestamp_us / HOUR_US * HOUR_US) + .or_default(); + summary.records = summary.records.saturating_add(1); + summary.encoded_bytes = summary.encoded_bytes.saturating_add(encoded_bytes); + } Ok(None) => break, Err(e) => { return Err(anyhow::anyhow!("WAL read error during reconcile: {e}")); @@ -56,21 +100,115 @@ pub async fn run(settings: Config) -> Result<()> { } } - if records.is_empty() { + if wal_records == 0 { tracing::info!("WAL is empty; nothing to reconcile"); return Ok(()); } - let record_count = records.len(); - writer - .write_batch_replacing(&records) - .await - .map_err(|e| anyhow::anyhow!("reconcile write failed: {e}"))?; + // Only datasets partitioned exactly by local receive time can use the WAL + // endpoints as a full-hour proof. Checkpoints are partitioned by exchange + // snapshot time, while validation/execution timestamps are independently + // supplied; replacing those partitions from this coverage would recreate the + // original partial-history bug under a different name. + let coverage_start_us = coverage_start_us.ok_or_else(|| { + anyhow::anyhow!("WAL has no receive-time-partitioned records to prove hourly coverage") + })?; + let coverage_end_us = coverage_end_us.expect("coverage start and end are set together"); + let coverage = pb_store::RecoveryCoverage::new(coverage_start_us, coverage_end_us) + .map_err(|e| anyhow::anyhow!("WAL does not prove a recoverable time span: {e}"))?; + + let recoverable_records: usize = supported_hours + .iter() + .filter(|(hour_start, _)| coverage.contains_complete_hour(**hour_start)) + .map(|(_, summary)| summary.records) + .sum(); + let boundary_records_skipped = supported_hours + .values() + .map(|summary| summary.records) + .sum::() + - recoverable_records; + if recoverable_records == 0 { + anyhow::bail!( + "unsafe recovery disabled: retained WAL {}..{} does not fully cover any receive-time-partitioned UTC hour; no Parquet data changed", + coverage.start_us(), + coverage.end_us() + ); + } + if let Some((hour_start, summary)) = supported_hours.iter().find(|(hour_start, summary)| { + coverage.contains_complete_hour(**hour_start) + && (summary.records > MAX_RECOVERY_HOUR_RECORDS + || summary.encoded_bytes > MAX_RECOVERY_HOUR_ENCODED_BYTES) + }) { + anyhow::bail!( + "unsafe recovery disabled: UTC hour starting {hour_start} contains {} records / {} encoded bytes, above the recovery memory ceiling of {} records / {} bytes; no Parquet data changed", + summary.records, + summary.encoded_bytes, + MAX_RECOVERY_HOUR_RECORDS, + MAX_RECOVERY_HOUR_ENCODED_BYTES + ); + } + + // Pass 2 retains at most one eligible UTC hour at a time. Supported receive + // timestamps were proven nondecreasing above and the WAL is static under the + // maintenance lease, so crossing an hour boundary makes the prior hour + // complete for publication without buffering the entire retained WAL. + let mut reader = pb_wal::WalReader::open_from_start(wal_config, "reconcile-publish") + .map_err(|e| anyhow::anyhow!("failed to reopen WAL for reconcile: {e}"))?; + let mut hour_records = Vec::new(); + let mut current_hour = None; + let mut report = pb_store::RecoveryReport::default(); + loop { + let payload = match reader.next_strict() { + Ok(Some(payload)) => payload, + Ok(None) => break, + Err(e) => return Err(anyhow::anyhow!("WAL read error during reconcile: {e}")), + }; + let record = pb_wal::codec::decode(&payload) + .map_err(|e| anyhow::anyhow!("WAL changed after validation: {e}"))?; + if !is_receive_time_partitioned(&record) + || !coverage.contains_complete_hour(record.partition_timestamp_us()) + { + continue; + } + let hour = record.partition_timestamp_us() / HOUR_US * HOUR_US; + if current_hour.is_some_and(|current| current != hour) { + let hour_report = writer + .write_batch_replacing(&hour_records, coverage) + .await + .map_err(|e| anyhow::anyhow!("reconcile write failed: {e}"))?; + report.partitions_published += hour_report.partitions_published; + report.records_published += hour_report.records_published; + report.cleanup_failures += hour_report.cleanup_failures; + hour_records.clear(); + } + current_hour = Some(hour); + hour_records.push(record); + } + if !hour_records.is_empty() { + let hour_report = writer + .write_batch_replacing(&hour_records, coverage) + .await + .map_err(|e| anyhow::anyhow!("reconcile write failed: {e}"))?; + report.partitions_published += hour_report.partitions_published; + report.records_published += hour_report.records_published; + report.cleanup_failures += hour_report.cleanup_failures; + } tracing::info!( - records = record_count, - decode_failures, - "reconcile complete: Parquet partitions rebuilt from WAL" + records = report.records_published, + partitions = report.partitions_published, + boundary_records_skipped, + unsupported_records_skipped, + cleanup_failures = report.cleanup_failures, + coverage_start_us, + coverage_end_us, + "reconcile complete: crash-consistent Parquet manifests published from WAL" ); + if report.cleanup_failures > 0 { + anyhow::bail!( + "reconcile published manifest-aware views, but {} superseded object(s) could not be deleted; direct Parquet scans are unsafe until a rerun completes cleanup", + report.cleanup_failures + ); + } Ok(()) } diff --git a/crates/pb-bin/src/commands/replay.rs b/crates/pb-bin/src/commands/replay.rs index e026a399..5d0d996a 100644 --- a/crates/pb-bin/src/commands/replay.rs +++ b/crates/pb-bin/src/commands/replay.rs @@ -28,7 +28,8 @@ pub async fn run( let base_path = settings .get_string("storage.parquet_base_path") .unwrap_or_else(|_| "./data".to_string()); - let reader = pb_replay::ParquetReader::new(&base_path); + let (store, base_path) = super::pipeline::build_object_store(&base_path)?; + let reader = pb_replay::ParquetReader::from_store(store, base_path); let engine = pb_replay::ReplayEngine::new(reader); let replay = engine.reconstruct_at(&asset_id, at_us, replay_mode).await?; let validation = if validate { diff --git a/crates/pb-bin/src/commands/serve.rs b/crates/pb-bin/src/commands/serve.rs index e0e063b4..abe79e3a 100644 --- a/crates/pb-bin/src/commands/serve.rs +++ b/crates/pb-bin/src/commands/serve.rs @@ -60,18 +60,20 @@ pub async fn run( .await; // Hydrate from checkpoints + WAL. - let reader = pb_replay::ParquetReader::new(&parquet_base_path); + let (parquet_store, parquet_prefix) = pipeline::build_object_store(&parquet_base_path)?; + let reader = pb_replay::ParquetReader::from_store(parquet_store, parquet_prefix); let hydration_result = pb_api::hydration::hydrate(&live, Some(&reader), Some(&wal_config), &token_ids).await; tracing::info!( checkpoints = hydration_result.checkpoints_loaded, wal_records = hydration_result.wal_records_replayed, + recovery_succeeded = hydration_result.recovery_succeeded, "serve runtime hydration complete" ); // Health tracking atomics shared between WAL tailer and API. let wal_lag_bytes = Arc::new(AtomicU64::new(0)); - let needs_resync = Arc::new(AtomicBool::new(false)); + let needs_resync = Arc::new(AtomicBool::new(!hydration_result.recovery_succeeded)); let max_consumer_lag = wal_config.max_consumer_lag_bytes; @@ -80,7 +82,7 @@ pub async fn run( wal_config, live.clone(), token_ids.clone(), - parquet_base_path.clone(), + reader.clone(), hydration_result.wal_end_position, shutdown.child_token(), wal_lag_bytes.clone(), @@ -90,8 +92,11 @@ pub async fn run( // Build and start HTTP/WS server. let (replay_service, integrity_service, execution_service) = - pipeline::build_services(&settings).await; - let query_service = pipeline::build_query_service(&settings).await; + pipeline::build_services(&settings).await?; + let effective_backend_is_clickhouse = + matches!(&replay_service, pb_service::AnyReplayService::ClickHouse(_)); + let query_service = + pipeline::build_query_service(&settings, effective_backend_is_clickhouse).await; let (query_max_rows, query_timeout_secs) = pipeline::query_config_from_settings(&settings); let auth_token = pipeline::api_auth_token_from_settings(&settings); @@ -192,7 +197,7 @@ fn spawn_wal_tailer( config: pb_wal::WalConfig, live: pb_api::LiveReadModel, token_ids: Vec, - parquet_base_path: String, + parquet_reader: pb_replay::ParquetReader, start_position: Option, shutdown: CancellationToken, lag_bytes_atomic: Arc, @@ -265,7 +270,6 @@ fn spawn_wal_tailer( TailOutcome::Resync => { needs_resync_atomic.store(true, Ordering::Relaxed); tracing::warn!("WAL segment gap detected; re-hydrating live read model"); - let parquet_reader = pb_replay::ParquetReader::new(&parquet_base_path); let hydration = pb_api::hydration::hydrate( &live, Some(&parquet_reader), @@ -489,7 +493,7 @@ mod tests { config, live, vec!["tok1".to_string()], - dir.path().to_string_lossy().to_string(), + pb_replay::ParquetReader::new(dir.path()), None, shutdown.clone(), Arc::new(AtomicU64::new(0)), diff --git a/crates/pb-bin/src/commands/serve_api.rs b/crates/pb-bin/src/commands/serve_api.rs index 794b31db..1fba2b98 100644 --- a/crates/pb-bin/src/commands/serve_api.rs +++ b/crates/pb-bin/src/commands/serve_api.rs @@ -84,8 +84,11 @@ pub async fn run( }; let (replay_service, integrity_service, execution_service) = - pipeline::build_services(&settings).await; - let query_service = pipeline::build_query_service(&settings).await; + pipeline::build_services(&settings).await?; + let effective_backend_is_clickhouse = + matches!(&replay_service, pb_service::AnyReplayService::ClickHouse(_)); + let query_service = + pipeline::build_query_service(&settings, effective_backend_is_clickhouse).await; let (query_max_rows, query_timeout_secs) = pipeline::query_config_from_settings(&settings); let auth_token = pipeline::api_auth_token_from_settings(&settings); diff --git a/crates/pb-replay/README.md b/crates/pb-replay/README.md index 081e7c67..b06de526 100644 --- a/crates/pb-replay/README.md +++ b/crates/pb-replay/README.md @@ -11,7 +11,7 @@ and supports periodic REST snapshot backfill for checkpoint seeding. | `ReplayEngine` | Reconstructs `L2Book` at a target timestamp by reading checkpoints and applying events forward. | | `ReplayResult` | Output of reconstruction: the rebuilt `L2Book`, replay mode, whether a checkpoint was used, and continuity (ingest) events encountered. | | `EventReader` | Trait abstracting storage backends for reading events by time range and asset. | -| `ParquetReader` | `EventReader` implementation for local/S3 Parquet files. | +| `ParquetReader` | `EventReader` implementation for local/S3 Parquet objects through `object_store`. | | `ClickHouseReader` | `EventReader` implementation for ClickHouse tables. | | `BackfillConfig` | Configuration for periodic REST snapshot fetching. | | `ReplayError` | Error type for replay operations. | @@ -51,7 +51,22 @@ Also: `run_backfill` periodically fetches REST snapshots and writes them as and requires a fresh post-reset snapshot before applying later deltas. - The `EventReader` trait has methods for reading each dataset type: `read_market_data`, `read_checkpoints`, `read_latest_checkpoint`, + `read_latest_checkpoints`, `read_validations`, `read_execution_events`. +- `ParquetReader::from_store` uses Parquet object-store range reads, so the + deployed S3 path is read through the same credentials and endpoint wiring as + the writer; it never treats `s3://` as a local filesystem path. +- Parquet reads use bounded file concurrency and reject a dataset result above + 500,000 rows with a request-window error instead of collecting an unbounded + response in the serve process. +- Hour listings honor versioned recovery manifests. A manifest atomically swaps + one `(dataset, asset, hour)` view to an immutable recovered object and hides + superseded normal files even if post-publication cleanup was interrupted. +- Startup latest-checkpoint lookup lists the checkpoint dataset once for the + entire active-asset set and walks only existing hours newest-first; it does + not rescan the full inventory per asset or probe empty hours back to epoch. +- Manifest paths are parsed without re-encoding their object keys, and logical + asset filters handle `object_store`'s additional percent escaping. - Both readers support time-range filtering and asset filtering at the storage layer to minimize I/O. ClickHouseReader pushes `WHERE asset_id` into the query for server-side filtering and adds `ORDER BY` clauses on all queries. @@ -113,6 +128,6 @@ Also: `run_backfill` periodically fetches REST snapshots and writes them as ## Tests -28 tests covering `ReplayEngine` mock-based reconstruction, `hour_paths` generation, +51 tests covering `ReplayEngine` mock-based reconstruction, `hour_paths` generation, `ParquetReader` integration, end-to-end write-then-reconstruct round-trips, and backfill REST response parsing. diff --git a/crates/pb-replay/src/error.rs b/crates/pb-replay/src/error.rs index 16645378..19c9f937 100644 --- a/crates/pb-replay/src/error.rs +++ b/crates/pb-replay/src/error.rs @@ -8,6 +8,9 @@ pub enum ReplayError { #[error("Parquet error: {0}")] Parquet(#[from] parquet::errors::ParquetError), + #[error("object store error: {0}")] + ObjectStore(#[from] object_store::Error), + #[error("Arrow error: {0}")] Arrow(#[from] arrow::error::ArrowError), diff --git a/crates/pb-replay/src/reader.rs b/crates/pb-replay/src/reader.rs index ebb43894..cf086311 100644 --- a/crates/pb-replay/src/reader.rs +++ b/crates/pb-replay/src/reader.rs @@ -1,17 +1,19 @@ -use std::path::{Path, PathBuf}; +use std::collections::{BTreeMap, HashSet}; +use std::sync::Arc; use arrow::array::{Array, AsArray, UInt64Array}; use arrow::datatypes::{UInt32Type, UInt64Type}; use futures_util::stream::{self, StreamExt, TryStreamExt}; -use parquet::arrow::async_reader::ParquetRecordBatchStreamBuilder; -use tokio::fs::File; -use tracing::debug; +use object_store::path::Path as ObjectPath; +use object_store::{ObjectStore, ObjectStoreExt}; +use parquet::arrow::async_reader::{ParquetObjectReader, ParquetRecordBatchStreamBuilder}; use pb_types::event::{ BookCheckpoint, BookEvent, BookEventKind, DataSource, EventProvenance, ExecutionEvent, ExecutionEventKind, IngestEvent, IngestEventKind, LatencyTrace, MarketDataWindow, ReplayMode, ReplayValidation, Side, TradeEvent, }; +use pb_types::{storage::PARQUET_RECOVERY_MANIFEST_PREFIX, ParquetRecoveryManifest}; use pb_types::{AssetId, FixedPrice, FixedSize, PriceLevel, Sequence, TradeFidelity}; use crate::error::ReplayError; @@ -37,6 +39,24 @@ pub trait EventReader: Send + Sync { at_us: u64, ) -> impl std::future::Future, ReplayError>> + Send; + /// Read the latest checkpoint for many assets. Backends may override this + /// to share one inventory/query; the default preserves compatibility for + /// readers where individual lookups are already efficient. + fn read_latest_checkpoints( + &self, + asset_ids: &[AssetId], + at_us: u64, + ) -> impl std::future::Future>, ReplayError>> + Send + { + async move { + let mut checkpoints = Vec::with_capacity(asset_ids.len()); + for asset_id in asset_ids { + checkpoints.push(self.read_latest_checkpoint(asset_id, at_us).await?); + } + Ok(checkpoints) + } + } + fn read_validations( &self, asset_id: &AssetId, @@ -52,8 +72,10 @@ pub trait EventReader: Send + Sync { ) -> impl std::future::Future, ReplayError>> + Send; } +#[derive(Clone)] pub struct ParquetReader { - base_path: PathBuf, + store: Arc, + base_path: ObjectPath, } /// The on-disk Parquet schema version this reader understands. Files written by @@ -76,7 +98,7 @@ fn check_schema_version(version: Option<&str>) -> Result<(), ReplayError> { } const PARQUET_READ_CONCURRENCY: usize = 8; -const RECENT_CHECKPOINT_LOOKBACK_US: u64 = 6 * 3_600 * 1_000_000; +const MAX_PARQUET_DATASET_ROWS: usize = 500_000; /// Hard backstop on rows returned by a single unbounded ClickHouse read /// (ingest/execution events). Applied via `max_result_rows` + @@ -85,13 +107,49 @@ const RECENT_CHECKPOINT_LOOKBACK_US: u64 = 6 * 3_600 * 1_000_000; const MAX_READ_ROWS: u64 = 5_000_000; impl ParquetReader { - pub fn new(base_path: impl Into) -> Self { + /// Build a reader for a local filesystem path. + /// + /// Cloud URLs must be constructed by the runtime and passed to + /// [`Self::from_store`], so reads and writes share identical credentials and + /// endpoint configuration. + pub fn new(base_path: impl AsRef) -> Self { + let path = base_path.as_ref(); + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + std::env::current_dir() + .unwrap_or_else(|_| std::path::PathBuf::from("/")) + .join(path) + }; + Self { + store: Arc::new(object_store::local::LocalFileSystem::new()), + base_path: ObjectPath::from(absolute.to_string_lossy().as_ref()), + } + } + + /// Build a Parquet reader over an arbitrary object-store backend. + pub fn from_store(store: Arc, base_path: impl Into) -> Self { + let base_path = base_path.into(); Self { - base_path: base_path.into(), + store, + // `parse_url_opts` returns an already percent-encoded object-store + // prefix. `Path::from` would encode `%` again, and every child path + // would then point at a different key. + base_path: ObjectPath::parse(base_path.as_str()) + .expect("object-store prefixes must be valid object paths"), } } - pub(crate) fn hour_paths(&self, dataset: &str, start_us: u64, end_us: u64) -> Vec { + fn child_path(&self, suffix: &str) -> ObjectPath { + let path = if self.base_path.as_ref().is_empty() { + suffix.to_string() + } else { + format!("{}/{suffix}", self.base_path) + }; + ObjectPath::parse(path).expect("validated base plus internal suffix is a valid object path") + } + + pub(crate) fn hour_paths(&self, dataset: &str, start_us: u64, end_us: u64) -> Vec { use chrono::{Datelike, TimeZone, Timelike, Utc}; let start_dt = Utc @@ -118,7 +176,7 @@ impl ParquetReader { current.day(), current.hour(), ); - paths.push(self.base_path.join(dataset).join(hour_key)); + paths.push(self.child_path(&format!("{dataset}/{hour_key}"))); current += chrono::Duration::hours(1); } paths @@ -130,52 +188,202 @@ impl ParquetReader { asset_prefix: Option<&str>, start_us: u64, end_us: u64, - ) -> Result, ReplayError> { + ) -> Result, ReplayError> { let mut files = Vec::new(); for dir in self.hour_paths(dataset, start_us, end_us) { - let mut entries = match tokio::fs::read_dir(&dir).await { - Ok(entries) => entries, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - debug!(?dir, "hour directory not found, skipping"); - continue; - } - Err(e) => return Err(e.into()), - }; - - while let Some(entry) = entries.next_entry().await? { - let path = entry.path(); + let entries = self.store.list(Some(&dir)).collect::>().await; + for entry in entries { + let meta = entry?; + let path = meta.location; if !path - .extension() - .map(|ext| ext == "parquet") - .unwrap_or(false) + .filename() + .is_some_and(|name| name.ends_with(".parquet")) { continue; } if let Some(prefix) = asset_prefix { - let Some(name) = path.file_name().and_then(|name| name.to_str()) else { + let Some(name) = path.filename() else { continue; }; let expected = pb_types::newtype::storage_key_for(prefix); - if !pb_types::newtype::storage_file_matches_asset(name, &expected) { + if !pb_types::newtype::storage_object_file_matches_asset(name, &expected) { continue; } } files.push(path); } + + let manifests = self.recovery_manifests(dataset, &dir, asset_prefix).await?; + if !manifests.is_empty() { + let superseded: HashSet<&str> = manifests + .iter() + .flat_map(|manifest| manifest.superseded_objects.iter().map(String::as_str)) + .collect(); + files.retain(|path| !superseded.contains(path.as_ref())); + for manifest in manifests { + for object in manifest.active_objects { + let path = ObjectPath::parse(&object).map_err(|error| { + ReplayError::Other(format!( + "invalid active object path {object} in recovery manifest: {error}" + )) + })?; + files.push(path); + } + } + } } + files.sort(); + files.dedup(); Ok(files) } + async fn recovery_manifests( + &self, + dataset: &str, + hour_path: &ObjectPath, + asset_prefix: Option<&str>, + ) -> Result, ReplayError> { + let hour_suffix = hour_path + .as_ref() + .strip_prefix(self.base_path.as_ref()) + .unwrap_or(hour_path.as_ref()) + .trim_start_matches('/'); + let hour_key = hour_suffix + .strip_prefix(dataset) + .unwrap_or(hour_suffix) + .trim_start_matches('/'); + let manifest_prefix = self.child_path(&format!( + "{PARQUET_RECOVERY_MANIFEST_PREFIX}/{dataset}/{hour_key}" + )); + + let mut manifests = Vec::new(); + if let Some(asset_id) = asset_prefix { + let asset_key = pb_types::newtype::storage_key_for(asset_id); + let manifest_name = format!("{asset_key}.json"); + // Both components are already percent-encoded object paths. `join` + // accepts a raw segment and would encode `%` a second time. + let path = ObjectPath::parse(format!("{manifest_prefix}/{manifest_name}")) + .expect("validated manifest prefix and encoded asset form a valid object path"); + match self.store.get(&path).await { + Ok(result) => { + let bytes = result.bytes().await?; + let manifest = self.decode_manifest(&path, bytes.as_ref())?; + self.validate_manifest_scope( + &path, + &manifest, + dataset, + hour_key, + Some(&asset_key), + )?; + manifests.push(manifest); + } + Err(object_store::Error::NotFound { .. }) => {} + Err(error) => return Err(error.into()), + } + } else { + let entries = self + .store + .list(Some(&manifest_prefix)) + .collect::>() + .await; + for entry in entries { + let meta = entry?; + if !meta + .location + .filename() + .is_some_and(|name| name.ends_with(".json")) + { + continue; + } + let bytes = self.store.get(&meta.location).await?.bytes().await?; + let manifest = self.decode_manifest(&meta.location, bytes.as_ref())?; + self.validate_manifest_scope(&meta.location, &manifest, dataset, hour_key, None)?; + manifests.push(manifest); + } + } + Ok(manifests) + } + + fn decode_manifest( + &self, + path: &ObjectPath, + bytes: &[u8], + ) -> Result { + let manifest: ParquetRecoveryManifest = serde_json::from_slice(bytes)?; + manifest.validate().map_err(|error| { + ReplayError::Other(format!("invalid recovery manifest {path}: {error}")) + })?; + Ok(manifest) + } + + fn validate_manifest_scope( + &self, + path: &ObjectPath, + manifest: &ParquetRecoveryManifest, + dataset: &str, + hour_key: &str, + expected_asset: Option<&str>, + ) -> Result<(), ReplayError> { + let identity_matches = manifest.dataset == dataset + && manifest.hour_key == hour_key + && expected_asset.is_none_or(|asset| manifest.asset_key == asset) + && path + .filename() + .and_then(|name| name.strip_suffix(".json")) + .is_some_and(|stem| { + pb_types::newtype::storage_object_component_matches_key( + stem, + &manifest.asset_key, + ) + }); + let recovery_prefix = self + .child_path(&format!( + "{}/{dataset}/{hour_key}", + pb_types::storage::PARQUET_RECOVERY_OBJECT_PREFIX + )) + .to_string(); + let normal_prefix = self + .child_path(&format!("{dataset}/{hour_key}")) + .to_string(); + let matches_asset = |object: &str| { + object.rsplit('/').next().is_some_and(|name| { + pb_types::newtype::storage_object_file_matches_asset(name, &manifest.asset_key) + }) + }; + let active_objects_are_scoped = manifest.active_objects.iter().all(|object| { + [&normal_prefix, &recovery_prefix].iter().any(|prefix| { + object + .strip_prefix(prefix.as_str()) + .is_some_and(|suffix| suffix.starts_with('/') && suffix.ends_with(".parquet")) + }) && matches_asset(object) + }); + let superseded_objects_are_scoped = manifest.superseded_objects.iter().all(|object| { + [&normal_prefix, &recovery_prefix].iter().any(|prefix| { + object + .strip_prefix(prefix.as_str()) + .is_some_and(|suffix| suffix.starts_with('/') && suffix.ends_with(".parquet")) + }) && matches_asset(object) + }); + if !identity_matches || !active_objects_are_scoped || !superseded_objects_are_scoped { + return Err(ReplayError::Other(format!( + "recovery manifest {path} does not match its partition scope" + ))); + } + Ok(()) + } + async fn read_parquet_file( &self, - path: &Path, + path: &ObjectPath, extractor: F, ) -> Result, ReplayError> where F: Fn(&arrow::record_batch::RecordBatch) -> Result, ReplayError>, { - let file = File::open(path).await?; - let builder = ParquetRecordBatchStreamBuilder::new(file).await?; + let meta = self.store.head(path).await?; + let object_reader = + ParquetObjectReader::new(self.store.clone(), path.clone()).with_file_size(meta.size); + let builder = ParquetRecordBatchStreamBuilder::new(object_reader).await?; // Reject an incompatible on-disk layout instead of silently yielding // empty or mis-mapped rows. @@ -186,7 +394,7 @@ impl ParquetReader { .get("pb_schema_version") .map(String::as_str), ) - .map_err(|e| ReplayError::Other(format!("Parquet file {path:?}: {e}")))?; + .map_err(|e| ReplayError::Other(format!("Parquet object {path}: {e}")))?; let mut stream = builder.build()?; let mut rows = Vec::new(); @@ -194,7 +402,13 @@ impl ParquetReader { use futures_util::StreamExt; while let Some(batch_result) = stream.next().await { let batch = batch_result?; - rows.extend(extractor(&batch)?); + let extracted = extractor(&batch)?; + if rows.len().saturating_add(extracted.len()) > MAX_PARQUET_DATASET_ROWS { + return Err(ReplayError::Other(format!( + "Parquet read exceeds {MAX_PARQUET_DATASET_ROWS} rows per dataset; narrow the requested time window" + ))); + } + rows.extend(extracted); } Ok(rows) @@ -202,7 +416,7 @@ impl ParquetReader { async fn read_parquet_files( &self, - paths: Vec, + paths: Vec, extractor: F, ) -> Result, ReplayError> where @@ -216,43 +430,136 @@ impl ParquetReader { // for determinism because the replay engine sorts the merged events into a // total order (engine::sort_book_events) before applying them, so the // unordered read order cannot affect reconstruction output. - let batches = stream::iter(paths.into_iter().map(|path| { + stream::iter(paths.into_iter().map(|path| { let extractor = extractor.clone(); async move { self.read_parquet_file(&path, extractor).await } })) .buffer_unordered(PARQUET_READ_CONCURRENCY) - .try_collect::>>() - .await?; - - Ok(batches.into_iter().flatten().collect()) + .try_fold(Vec::new(), |mut rows, mut batch| async move { + if rows.len().saturating_add(batch.len()) > MAX_PARQUET_DATASET_ROWS { + return Err(ReplayError::Other(format!( + "Parquet read exceeds {MAX_PARQUET_DATASET_ROWS} rows per dataset; narrow the requested time window" + ))); + } + rows.append(&mut batch); + Ok(rows) + }) + .await } - async fn latest_checkpoint_in_range( + /// Resolve and read one manifest-aware dataset view, retrying the complete + /// resolution once if post-publication garbage collection removed an object + /// selected immediately before the manifest changed. + async fn read_dataset_with_view_retry( &self, - asset_id: &AssetId, + dataset: &str, + asset_prefix: Option<&str>, start_us: u64, end_us: u64, - ) -> Result, ReplayError> { - let files = self - .dataset_files( - "book_checkpoints", - Some(asset_id.as_str()), - start_us, - end_us, - ) - .await?; - if files.is_empty() { - return Ok(None); + extractor: F, + ) -> Result, ReplayError> + where + T: Send, + F: Fn(&arrow::record_batch::RecordBatch) -> Result, ReplayError> + + Clone + + Send + + Sync, + { + for attempt in 0..2 { + let files = self + .dataset_files(dataset, asset_prefix, start_us, end_us) + .await?; + match self.read_parquet_files(files, extractor.clone()).await { + Err(error) if attempt == 0 && is_object_not_found(&error) => { + tracing::warn!( + dataset, + "Parquet recovery view changed during read; resolving manifest once more" + ); + } + result => return result, + } } + unreachable!("the second view-resolution attempt always returns") + } - let mut checkpoints = self - .read_parquet_files(files, |batch| { - extract_checkpoints(batch, asset_id, start_us, end_us) - }) - .await?; - checkpoints.sort_by_key(|checkpoint| checkpoint.checkpoint_timestamp_us); - Ok(checkpoints.pop()) + /// List the checkpoint dataset once and group matching objects by their + /// lexicographically sortable `YYYY/MM/DD/HH` partition. This is used by + /// startup hydration: probing every hour while exponentially widening to + /// epoch turns an empty S3 checkpoint lookup into millions of sequential + /// LIST/GET requests. + async fn checkpoint_files_by_asset_and_hour( + &self, + asset_ids: &[AssetId], + at_us: u64, + ) -> Result>>, ReplayError> { + use chrono::{Datelike, TimeZone, Timelike, Utc}; + + let at = Utc + .timestamp_opt(at_us as i64 / 1_000_000, 0) + .single() + .unwrap_or_default(); + let at_hour = format!( + "{:04}/{:02}/{:02}/{:02}", + at.year(), + at.month(), + at.day(), + at.hour() + ); + let dataset_prefix = self.child_path("book_checkpoints"); + let relative_prefix = format!("{dataset_prefix}/"); + let logical_assets = asset_ids + .iter() + .map(|asset_id| pb_types::newtype::storage_key_for(asset_id.as_str())) + .collect::>(); + let entries = self + .store + .list(Some(&dataset_prefix)) + .collect::>() + .await; + let mut by_asset = vec![BTreeMap::>::new(); asset_ids.len()]; + for entry in entries { + let meta = entry?; + let Some(file_name) = meta.location.filename() else { + continue; + }; + let Some(asset_index) = logical_assets.iter().position(|logical_asset| { + pb_types::newtype::storage_object_file_matches_asset(file_name, logical_asset) + }) else { + continue; + }; + let Some(relative) = meta.location.as_ref().strip_prefix(&relative_prefix) else { + continue; + }; + let Some((hour_key, _)) = relative.rsplit_once('/') else { + continue; + }; + if hour_key.len() == "YYYY/MM/DD/HH".len() && hour_key <= at_hour.as_str() { + by_asset[asset_index] + .entry(hour_key.to_string()) + .or_default() + .push(meta.location); + } + } + Ok(by_asset) + } +} + +/// Object-store range failures made by `ParquetObjectReader` are wrapped in +/// `ParquetError::External`. Walk the error chain so a manifest transition that +/// deletes an object between HEAD and a later range GET is retried just like a +/// direct object-store `NotFound`. +pub(crate) fn is_object_not_found(error: &ReplayError) -> bool { + let mut current: Option<&(dyn std::error::Error + 'static)> = Some(error); + while let Some(source) = current { + if source + .downcast_ref::() + .is_some_and(|error| matches!(error, object_store::Error::NotFound { .. })) + { + return true; + } + current = source.source(); } + false } fn parse_source(value: &str) -> Result { @@ -953,25 +1260,37 @@ impl EventReader for ParquetReader { start_us: u64, end_us: u64, ) -> Result { - let (book_files, trade_files, ingest_files) = tokio::try_join!( - self.dataset_files("book_events", Some(asset_id.as_str()), start_us, end_us), - self.dataset_files("trade_events", Some(asset_id.as_str()), start_us, end_us), - self.dataset_files("ingest_events", None, start_us, end_us), - )?; - let asset_id = asset_id.clone(); + let asset_filter = asset_id.as_str().to_string(); + let ingest_asset_id = asset_id.clone(); let (book_events, trade_events, ingest_events) = tokio::try_join!( - self.read_parquet_files(book_files, { - let asset_id = asset_id.clone(); - move |batch| extract_book_events(batch, &asset_id, start_us, end_us) - }), - self.read_parquet_files(trade_files, { - let asset_id = asset_id.clone(); - move |batch| extract_trade_events(batch, &asset_id, start_us, end_us) - }), - self.read_parquet_files(ingest_files, move |batch| { - extract_ingest_events(batch, &asset_id, start_us, end_us) - }), + self.read_dataset_with_view_retry( + "book_events", + Some(&asset_filter), + start_us, + end_us, + { + let asset_id = asset_id.clone(); + move |batch| extract_book_events(batch, &asset_id, start_us, end_us) + }, + ), + self.read_dataset_with_view_retry( + "trade_events", + Some(&asset_filter), + start_us, + end_us, + { + let asset_id = asset_id.clone(); + move |batch| extract_trade_events(batch, &asset_id, start_us, end_us) + }, + ), + self.read_dataset_with_view_retry( + "ingest_events", + None, + start_us, + end_us, + move |batch| extract_ingest_events(batch, &ingest_asset_id, start_us, end_us), + ), )?; let mut book_events = book_events; @@ -1031,20 +1350,44 @@ impl EventReader for ParquetReader { asset_id: &AssetId, at_us: u64, ) -> Result, ReplayError> { - let mut window = RECENT_CHECKPOINT_LOOKBACK_US; - loop { - let start_us = at_us.saturating_sub(window); - if let Some(checkpoint) = self - .latest_checkpoint_in_range(asset_id, start_us, at_us) - .await? - { - return Ok(Some(checkpoint)); - } - if start_us == 0 { - return Ok(None); + Ok(self + .read_latest_checkpoints(std::slice::from_ref(asset_id), at_us) + .await? + .pop() + .flatten()) + } + + async fn read_latest_checkpoints( + &self, + asset_ids: &[AssetId], + at_us: u64, + ) -> Result>, ReplayError> { + // Checkpoints are intentionally not eligible for manifest recovery + // because their exchange-time partition cannot be proven complete from + // receive-time WAL endpoints. Inventory the dataset once for the entire + // startup asset set instead of recursively listing all objects once per + // asset. + let inventories = self + .checkpoint_files_by_asset_and_hour(asset_ids, at_us) + .await?; + let mut latest = Vec::with_capacity(asset_ids.len()); + for (asset_id, by_hour) in asset_ids.iter().zip(inventories) { + let mut found = None; + for (_, files) in by_hour.into_iter().rev() { + let mut checkpoints = self + .read_parquet_files(files, |batch| { + extract_checkpoints(batch, asset_id, 0, at_us) + }) + .await?; + checkpoints.sort_by_key(|checkpoint| checkpoint.checkpoint_timestamp_us); + if let Some(checkpoint) = checkpoints.pop() { + found = Some(checkpoint); + break; + } } - window = window.saturating_mul(2); + latest.push(found); } + Ok(latest) } async fn read_validations( diff --git a/crates/pb-replay/src/tests.rs b/crates/pb-replay/src/tests.rs index 74c05e24..83ca3731 100644 --- a/crates/pb-replay/src/tests.rs +++ b/crates/pb-replay/src/tests.rs @@ -10,7 +10,7 @@ use pb_types::{AssetId, FixedPrice, FixedSize, PriceLevel, Sequence, TradeFideli use crate::engine::ReplayEngine; use crate::error::ReplayError; -use crate::reader::{EventReader, ParquetReader}; +use crate::reader::{is_object_not_found, EventReader, ParquetReader}; // --------------------------------------------------------------------------- // Test helpers @@ -20,6 +20,19 @@ fn test_asset_id() -> AssetId { AssetId::new("BTC-5M-YES") } +#[test] +fn parquet_wrapped_object_not_found_is_retryable() { + let nested = object_store::Error::NotFound { + path: "missing.parquet".to_string(), + source: Box::new(std::io::Error::new( + std::io::ErrorKind::NotFound, + "deleted after head", + )), + }; + let error = ReplayError::Parquet(parquet::errors::ParquetError::External(Box::new(nested))); + assert!(is_object_not_found(&error)); +} + fn test_provenance(recv_ts: u64, seq: u64) -> EventProvenance { EventProvenance { recv_timestamp_us: recv_ts, @@ -719,7 +732,7 @@ fn hour_paths_single_hour() { let paths = reader.hour_paths("book_events", start, end); assert_eq!(paths.len(), 1); - let path_str = paths[0].to_str().unwrap(); + let path_str = paths[0].as_ref(); assert!(path_str.contains("book_events")); } @@ -749,10 +762,7 @@ fn hour_paths_midnight_crossing() { assert!(paths.len() >= 2, "should cross midnight boundary"); // Verify the paths contain different day patterns - let path_strs: Vec = paths - .iter() - .map(|p| p.to_str().unwrap().to_string()) - .collect(); + let path_strs: Vec = paths.iter().map(ToString::to_string).collect(); // At least one path should contain /23 (hour 23) assert!( path_strs.iter().any(|p| p.contains("/23")), @@ -813,6 +823,106 @@ async fn parquet_reader_reads_book_events() { assert_eq!(window.book_events[0].price, FixedPrice::new(5000).unwrap()); } +#[tokio::test] +async fn object_store_reader_honors_recovery_manifest_before_cleanup() { + use futures_util::StreamExt; + use object_store::memory::InMemory; + use object_store::{ObjectStore, ObjectStoreExt, PutPayload}; + use pb_store::{ParquetRecordWriter, RecoveryCoverage}; + + let store = Arc::new(InMemory::new()) as Arc; + let writer = ParquetRecordWriter::new(store.clone(), "data"); + let t0 = BASE_TS; + let encoded_asset = AssetId::new("asset/with%separator"); + let mut old_event = make_snapshot_event(t0, Side::Bid, 5000, 1_000_000, 1); + old_event.asset_id = encoded_asset.clone(); + let old = pb_types::PersistedRecord::Book(old_event); + let mut new_event = make_delta_event(t0 + 1_000_000, Side::Ask, 5100, 2_000_000, 2); + new_event.asset_id = encoded_asset.clone(); + let new = pb_types::PersistedRecord::Book(new_event); + writer.write_record(old.clone()).await.unwrap(); + + let old_meta = store + .list(Some(&object_store::path::Path::from("data/book_events"))) + .next() + .await + .unwrap() + .unwrap(); + let old_bytes = store + .get(&old_meta.location) + .await + .unwrap() + .bytes() + .await + .unwrap(); + + let hour_start = t0 / 3_600_000_000 * 3_600_000_000; + let coverage = RecoveryCoverage::new(hour_start - 1, hour_start + 3_600_000_000).unwrap(); + writer + .write_batch_replacing(&[old, new], coverage) + .await + .unwrap(); + + // Simulate a crash after manifest publication but before stale-object + // cleanup. The old normal object is visible in the listing again, but the + // manifest must keep it out of the authoritative read view. + store + .put(&old_meta.location, PutPayload::from(old_bytes)) + .await + .unwrap(); + + let reader = ParquetReader::from_store(store, "data"); + let window = reader + .read_market_data(&encoded_asset, t0 - 1, t0 + 2_000_000) + .await + .unwrap(); + assert_eq!( + window.book_events.len(), + 2, + "the superseded object must not be double-counted" + ); +} + +#[tokio::test] +async fn object_store_reader_preserves_an_encoded_base_prefix() { + use futures_util::StreamExt; + use object_store::memory::InMemory; + use object_store::ObjectStore; + use pb_store::ParquetRecordWriter; + + let store = Arc::new(InMemory::new()) as Arc; + // This is the representation returned by `parse_url_opts` for an object + // prefix whose component contains a space. It must not become `%2520` when + // writer/reader child paths are constructed. + let base = object_store::path::Path::parse("prefix%20name/data") + .unwrap() + .to_string(); + let writer = ParquetRecordWriter::new(store.clone(), base.clone()); + let record = pb_types::PersistedRecord::Book(make_snapshot_event( + BASE_TS, + Side::Bid, + 5000, + 1_000_000, + 1, + )); + writer.write_record(record).await.unwrap(); + + let paths = store + .list(None) + .map(|entry| entry.unwrap().location.to_string()) + .collect::>() + .await; + assert!(paths.iter().all(|path| path.starts_with("prefix%20name/"))); + assert!(paths.iter().all(|path| !path.contains("%2520"))); + + let reader = ParquetReader::from_store(store, base); + let window = reader + .read_market_data(&test_asset_id(), BASE_TS - 1, BASE_TS + 1) + .await + .unwrap(); + assert_eq!(window.book_events.len(), 1); +} + #[tokio::test] async fn parquet_reader_preserves_ingest_ordinal() { // The ingest ordinal must survive the Parquet write→read round-trip so replay @@ -943,6 +1053,17 @@ async fn parquet_reader_missing_directory_returns_empty() { assert!(window.ingest_events.is_empty()); } +#[tokio::test] +async fn parquet_reader_missing_checkpoint_inventory_returns_none() { + let dir = TempDir::new().unwrap(); + let reader = ParquetReader::new(dir.path().join("nonexistent")); + assert!(reader + .read_latest_checkpoint(&test_asset_id(), BASE_TS) + .await + .unwrap() + .is_none()); +} + #[tokio::test] async fn parquet_reader_filters_by_time_range() { let dir = TempDir::new().unwrap(); diff --git a/crates/pb-service/README.md b/crates/pb-service/README.md index 035016eb..f3d55c24 100644 --- a/crates/pb-service/README.md +++ b/crates/pb-service/README.md @@ -21,6 +21,11 @@ adapters (parse HTTP → call service → format response). | Parquet | `ParquetReplayService`, `ParquetIntegrityService`, `ParquetExecutionService` | | ClickHouse | `ClickHouseReplayService`, `ClickHouseIntegrityService`, `ClickHouseExecutionService`, `ClickHouseQueryService` | +Each Parquet service supports `from_reader(ParquetReader)`. The runtime uses +these constructors to share one configured local/S3 object-store reader across +replay, integrity, and execution services; `new(path)` remains the local-path +convenience constructor. + ## Enum Dispatch Service traits use `impl Future` return types, making them not dyn-compatible. diff --git a/crates/pb-service/src/parquet.rs b/crates/pb-service/src/parquet.rs index 240988f9..46216572 100644 --- a/crates/pb-service/src/parquet.rs +++ b/crates/pb-service/src/parquet.rs @@ -16,14 +16,17 @@ use crate::{ /// Replay service backed by Parquet event files. #[derive(Clone)] pub struct ParquetReplayService { - base_path: String, + reader: ParquetReader, } impl ParquetReplayService { pub fn new(base_path: impl Into) -> Self { - Self { - base_path: base_path.into(), - } + let base_path = base_path.into(); + Self::from_reader(ParquetReader::new(base_path)) + } + + pub fn from_reader(reader: ParquetReader) -> Self { + Self { reader } } } @@ -35,8 +38,7 @@ impl ReplayService for ParquetReplayService { mode: pb_types::event::ReplayMode, depth: Option, ) -> Result { - let reader = ParquetReader::new(&self.base_path); - let engine = ReplayEngine::new(reader); + let engine = ReplayEngine::new(self.reader.clone()); let result = engine .reconstruct_at(asset_id, at_us, mode) .await @@ -52,14 +54,17 @@ impl ReplayService for ParquetReplayService { /// Integrity service backed by Parquet event files. #[derive(Clone)] pub struct ParquetIntegrityService { - base_path: String, + reader: ParquetReader, } impl ParquetIntegrityService { pub fn new(base_path: impl Into) -> Self { - Self { - base_path: base_path.into(), - } + let base_path = base_path.into(); + Self::from_reader(ParquetReader::new(base_path)) + } + + pub fn from_reader(reader: ParquetReader) -> Self { + Self { reader } } } @@ -70,12 +75,13 @@ impl IntegrityService for ParquetIntegrityService { start_us: u64, end_us: u64, ) -> Result { - let reader = ParquetReader::new(&self.base_path); - let window = reader + let window = self + .reader .read_market_data(asset_id, start_us, end_us) .await .map_err(map_replay_error)?; - let validations = reader + let validations = self + .reader .read_validations(asset_id, start_us, end_us) .await .map_err(map_replay_error)?; @@ -96,14 +102,17 @@ impl IntegrityService for ParquetIntegrityService { /// Execution service backed by Parquet event files. #[derive(Clone)] pub struct ParquetExecutionService { - base_path: String, + reader: ParquetReader, } impl ParquetExecutionService { pub fn new(base_path: impl Into) -> Self { - Self { - base_path: base_path.into(), - } + let base_path = base_path.into(); + Self::from_reader(ParquetReader::new(base_path)) + } + + pub fn from_reader(reader: ParquetReader) -> Self { + Self { reader } } } @@ -118,8 +127,8 @@ impl ExecutionService for ParquetExecutionService { offset: usize, descending: bool, ) -> Result { - let reader = ParquetReader::new(&self.base_path); - let events = reader + let events = self + .reader .read_execution_events(order_id, start_us, end_us) .await .map_err(map_replay_error)?; diff --git a/crates/pb-store/README.md b/crates/pb-store/README.md index 0af412d0..205e06fe 100644 --- a/crates/pb-store/README.md +++ b/crates/pb-store/README.md @@ -7,10 +7,12 @@ ingest channel and writes them to durable storage in split-dataset format. | Type | Description | |------|-------------| -| `ParquetSink` | Batches events and flushes to Parquet files every 5 minutes with Zstd level-3 compression and `DELTA_BINARY_PACKED` encoding for timestamp, price, size, and sequence columns. Pre-allocates a 256 KB byte buffer to avoid repeated heap growth. Uses `object_store` (local FS / S3 / GCS). | +| `ParquetSink` | Batches events and flushes to Parquet files every 5 minutes with Zstd level-3 compression and `DELTA_BINARY_PACKED` encoding for timestamp, price, size, and sequence columns. Pre-allocates a 256 KB byte buffer to avoid repeated heap growth. Uses `object_store` (local FS / S3). | | `ClickHouseSink` | Batches events and inserts to ClickHouse every 1 second (or when batch reaches 10,000 rows). Creates insert handles conditionally — only for record types that have data in the current batch. Uses `MergeTree` engine partitioned by date. | | `ParquetRecordWriter` | Low-level writer for individual Parquet files. | | `ClickHouseRecordWriter` | Low-level writer for ClickHouse batch inserts. | +| `RecoveryCoverage` | Validated inclusive WAL timestamp span used to prove that an hourly receive-time partition is complete. | +| `RecoveryReport` | Published partition/record counts plus any post-publication cleanup failures. | | `StoreError` | Error type for storage operations. | ## Schema Functions @@ -41,8 +43,9 @@ PersistedRecord channel ## Design Notes -- Storage uses the `object_store` trait for filesystem abstraction, supporting - local disk, S3, and GCS without code changes. +- Storage uses the `object_store` trait for filesystem abstraction. The current + workspace enables local disk and S3 (including S3-compatible endpoints such as + MinIO); GCS support is not compiled in. - Parquet files are partitioned by event type and time window (`YYYY/MM/DD/HH`). The flush interval (5 minutes) balances write amplification against data freshness for replay. Records whose timestamp is outside a wide plausible band @@ -61,13 +64,18 @@ PersistedRecord channel asset component is parsed from the right-hand fixed suffix fields when reading or deleting, so an asset key containing `_` cannot be mistaken for another asset's prefix. -- `write_batch_replacing` rebuilds partitions authoritatively from a record - stream (WAL replay) for crash recovery: for each `(dataset, asset, hour)` - group it deletes the existing files whose parsed asset component matches that - exact `asset_key` and writes the complete group, so the source stream is - authoritative and re-running is idempotent. It is the storage half of the - `reconcile` command and is meant for offline use (ingest stopped) to avoid - racing live-sink writes to the same partitions. +- `write_batch_replacing` accepts only receive-time-partitioned book, trade, and + ingest records whose UTC hour is fully contained in an explicit, validated WAL + coverage span. It first writes an immutable object under `_recovery_objects` + and publishes a manifest pointing at that staged view. It then promotes the + same bytes into the normal dataset/hour tree, publishes the final manifest, and + cleans superseded normal/staged objects. Manifest-aware readers therefore see + a complete partition across crashes at either publication phase. A clean run + leaves the active object in the normal tree for direct Parquet consumers; + unresolved cleanup is reported and direct scans remain unsafe until a retry. + Boundary-hour and unprovable dataset replacement is refused. It is the storage + half of the offline `reconcile` command; every process that writes the same + Parquet prefix must be stopped during the manifest cut. - Parquet encoding uses explicit Zstd compression at level 3 and `DELTA_BINARY_PACKED` encoding on timestamp, price, size, and sequence columns for better compression ratios. - A pre-allocated 256 KB byte buffer avoids repeated heap allocation during Parquet writes. @@ -96,8 +104,8 @@ PersistedRecord channel - On graceful shutdown (cancellation) both sinks drain any records still queued in their mpsc channel — bounded by a 10s deadline — before the final flush, so a clean stop does not abandon records the upstream already enqueued. -- WAL→storage reconciliation (rebuilding a crash-lost Parquet window from the WAL) - is provided by `write_batch_replacing` / the `reconcile` command. +- WAL→storage reconciliation is provided by `write_batch_replacing` / the + `reconcile` command with strict full-hour coverage and manifest publication. ## Docs to Update After Changes @@ -112,5 +120,5 @@ PersistedRecord channel ## Tests -36 tests covering schema validation, record batch conversion, `ParquetRecordWriter` +41 tests covering schema validation, record batch conversion, `ParquetRecordWriter` lifecycle, and `ParquetSink` lifecycle. diff --git a/crates/pb-store/src/error.rs b/crates/pb-store/src/error.rs index 6b8c5f93..83f1a616 100644 --- a/crates/pb-store/src/error.rs +++ b/crates/pb-store/src/error.rs @@ -17,6 +17,9 @@ pub enum StoreError { #[error("object store error: {0}")] ObjectStore(#[from] object_store::Error), + #[error("unsafe parquet recovery refused: {0}")] + UnsafeRecovery(String), + #[error("JSON serialization error: {0}")] Json(#[from] serde_json::Error), diff --git a/crates/pb-store/src/lib.rs b/crates/pb-store/src/lib.rs index 81fecb6f..af74db1d 100644 --- a/crates/pb-store/src/lib.rs +++ b/crates/pb-store/src/lib.rs @@ -16,7 +16,7 @@ pub use schema::{ replay_validation_refs_to_record_batch, replay_validation_schema, schema_for_record, trade_event_refs_to_record_batch, trade_event_schema, }; -pub use writer::{ClickHouseRecordWriter, ParquetRecordWriter}; +pub use writer::{ClickHouseRecordWriter, ParquetRecordWriter, RecoveryCoverage, RecoveryReport}; #[cfg(test)] mod tests; diff --git a/crates/pb-store/src/tests.rs b/crates/pb-store/src/tests.rs index 95223277..0de81e93 100644 --- a/crates/pb-store/src/tests.rs +++ b/crates/pb-store/src/tests.rs @@ -6,6 +6,7 @@ use futures_util::StreamExt; use object_store::local::LocalFileSystem; use object_store::ObjectStore; use object_store::ObjectStoreExt; +use object_store::PutPayload; use parquet::basic::Compression; use parquet::file::reader::FileReader; use tempfile::TempDir; @@ -21,7 +22,7 @@ use pb_types::{AssetId, FixedPrice, FixedSize, PriceLevel, Sequence, TradeFideli use crate::schema::*; use crate::writer::ParquetRecordWriter; -use crate::ParquetSink; +use crate::{ParquetSink, StoreError}; // --------------------------------------------------------------------------- // Test helpers @@ -547,39 +548,190 @@ async fn reconcile_replaces_hour_partitions_idempotently() { let store = local_store(&dir); let writer = ParquetRecordWriter::new(store.clone(), "data"); - async fn count_files(store: &Arc) -> usize { + async fn count_parquet_files(store: &Arc) -> usize { store .list(None) .collect::>() .await .into_iter() .filter_map(|r| r.ok()) + .filter(|meta| { + meta.location + .filename() + .is_some_and(|name| name.ends_with(".parquet")) + }) .count() } + let hour_start = FIXED_TS_US / 3_600_000_000 * 3_600_000_000; + let coverage = + crate::writer::RecoveryCoverage::new(hour_start - 1, hour_start + 3_600_000_000).unwrap(); + // Two separate live flushes for the same (asset, hour) produce two files // that, read together, would double-count the window (the double-count hazard). let r1 = PersistedRecord::Book(make_book_event(FIXED_TS_US)); let r2 = PersistedRecord::Book(make_book_event(FIXED_TS_US + 1_000_000)); writer.write_record(r1.clone()).await.unwrap(); writer.write_record(r2.clone()).await.unwrap(); - assert_eq!(count_files(&store).await, 2); + assert_eq!(count_parquet_files(&store).await, 2); // Reconciling from the authoritative WAL stream rebuilds the hour as one // complete file, replacing the fragmented live-sink files. - writer - .write_batch_replacing(&[r1.clone(), r2.clone()]) + let report = writer + .write_batch_replacing(&[r1.clone(), r2.clone()], coverage) .await .unwrap(); + assert_eq!(report.cleanup_failures, 0); assert_eq!( - count_files(&store).await, + count_parquet_files(&store).await, 1, "reconcile should collapse the hour to a single authoritative file" ); // Re-running is idempotent: the same single file, no duplication. - writer.write_batch_replacing(&[r1, r2]).await.unwrap(); - assert_eq!(count_files(&store).await, 1, "reconcile must be idempotent"); + writer + .write_batch_replacing(&[r1, r2], coverage) + .await + .unwrap(); + assert_eq!( + count_parquet_files(&store).await, + 1, + "reconcile must be idempotent" + ); +} + +#[tokio::test] +async fn reconcile_collects_an_abandoned_pre_manifest_stage() { + let dir = TempDir::new().unwrap(); + let store = local_store(&dir); + let writer = ParquetRecordWriter::new(store.clone(), "data"); + let r1 = PersistedRecord::Book(make_book_event(FIXED_TS_US)); + let r2 = PersistedRecord::Book(make_book_event(FIXED_TS_US + 1_000_000)); + writer.write_record(r1.clone()).await.unwrap(); + + let normal = store + .list(Some(&object_store::path::Path::from("data/book_events"))) + .next() + .await + .unwrap() + .unwrap(); + let bytes = store + .get(&normal.location) + .await + .unwrap() + .bytes() + .await + .unwrap(); + let hour_key = crate::writer::partition_hour_key(FIXED_TS_US); + let abandoned = object_store::path::Path::parse(format!( + "data/{}/book_events/{hour_key}/{}", + pb_types::storage::PARQUET_RECOVERY_OBJECT_PREFIX, + normal.location.filename().unwrap() + )) + .unwrap(); + store + .put(&abandoned, PutPayload::from(bytes)) + .await + .unwrap(); + + let hour_start = FIXED_TS_US / 3_600_000_000 * 3_600_000_000; + let coverage = + crate::writer::RecoveryCoverage::new(hour_start - 1, hour_start + 3_600_000_000).unwrap(); + writer + .write_batch_replacing(&[r1, r2], coverage) + .await + .unwrap(); + + let hidden = store + .list(Some(&object_store::path::Path::from(format!( + "data/{}", + pb_types::storage::PARQUET_RECOVERY_OBJECT_PREFIX + )))) + .collect::>() + .await; + assert!( + hidden.is_empty(), + "abandoned recovery stages must be collected" + ); +} + +#[test] +fn recovery_coverage_requires_a_record_before_the_hour_boundary() { + let hour_start = FIXED_TS_US / 3_600_000_000 * 3_600_000_000; + let ends_after_hour = hour_start + 3_600_000_000; + + let boundary_only = crate::writer::RecoveryCoverage::new(hour_start, ends_after_hour).unwrap(); + assert!(!boundary_only.contains_complete_hour(hour_start)); + + let proven = crate::writer::RecoveryCoverage::new(hour_start - 1, ends_after_hour).unwrap(); + assert!(proven.contains_complete_hour(hour_start)); +} + +#[tokio::test] +async fn reconcile_refuses_partial_hour_without_deleting_existing_data() { + let dir = TempDir::new().unwrap(); + let store = local_store(&dir); + let writer = ParquetRecordWriter::new(store.clone(), "data"); + let record = PersistedRecord::Book(make_book_event(FIXED_TS_US)); + writer.write_record(record.clone()).await.unwrap(); + + let coverage = + crate::writer::RecoveryCoverage::new(FIXED_TS_US - 1_000_000, FIXED_TS_US + 1_000_000) + .unwrap(); + let error = writer + .write_batch_replacing(&[record], coverage) + .await + .unwrap_err(); + assert!(matches!(error, StoreError::UnsafeRecovery(_))); + + let parquet_files = store + .list(None) + .collect::>() + .await + .into_iter() + .filter_map(Result::ok) + .filter(|meta| { + meta.location + .filename() + .is_some_and(|name| name.ends_with(".parquet")) + }) + .count(); + assert_eq!(parquet_files, 1, "existing parquet must remain untouched"); +} + +#[tokio::test] +async fn reconcile_refuses_dataset_without_receive_time_partition_proof() { + let dir = TempDir::new().unwrap(); + let store = local_store(&dir); + let writer = ParquetRecordWriter::new(store.clone(), "data"); + let record = PersistedRecord::Execution(make_execution_event(FIXED_TS_US)); + writer.write_record(record.clone()).await.unwrap(); + + let hour_start = FIXED_TS_US / 3_600_000_000 * 3_600_000_000; + let coverage = + crate::writer::RecoveryCoverage::new(hour_start - 1, hour_start + 3_600_000_000).unwrap(); + let error = writer + .write_batch_replacing(&[record], coverage) + .await + .unwrap_err(); + assert!(matches!(error, StoreError::UnsafeRecovery(_))); + + let parquet_files = store + .list(None) + .collect::>() + .await + .into_iter() + .filter_map(Result::ok) + .filter(|meta| { + meta.location + .filename() + .is_some_and(|name| name.ends_with(".parquet")) + }) + .count(); + assert_eq!( + parquet_files, 1, + "unsupported dataset must remain untouched" + ); } #[tokio::test] @@ -597,7 +749,14 @@ async fn reconcile_delete_matches_asset_component_exactly() { writer.write_record(foo.clone()).await.unwrap(); writer.write_record(foo_bar).await.unwrap(); - writer.write_batch_replacing(&[foo]).await.unwrap(); + let hour_start = FIXED_TS_US / 3_600_000_000 * 3_600_000_000; + let coverage = + crate::writer::RecoveryCoverage::new(hour_start - 1, hour_start + 3_600_000_000).unwrap(); + let report = writer + .write_batch_replacing(&[foo], coverage) + .await + .unwrap(); + assert_eq!(report.cleanup_failures, 0); let file_names: Vec = store .list(None) @@ -605,6 +764,11 @@ async fn reconcile_delete_matches_asset_component_exactly() { .await .into_iter() .filter_map(|r| r.ok()) + .filter(|meta| { + meta.location + .filename() + .is_some_and(|name| name.ends_with(".parquet")) + }) .map(|meta| meta.location.filename().unwrap_or_default().to_string()) .collect(); @@ -621,6 +785,71 @@ async fn reconcile_delete_matches_asset_component_exactly() { .any(|name| pb_types::newtype::storage_file_matches_asset(name, "foo_bar"))); } +#[tokio::test] +async fn reconcile_rejects_manifest_that_widens_cleanup_scope() { + let dir = TempDir::new().unwrap(); + let store = local_store(&dir); + let writer = ParquetRecordWriter::new(store.clone(), "data"); + + let foo = PersistedRecord::Book(make_book_event_for_asset(AssetId::new("foo"), FIXED_TS_US)); + let foo_bar = PersistedRecord::Book(make_book_event_for_asset( + AssetId::new("foo_bar"), + FIXED_TS_US, + )); + writer.write_record(foo.clone()).await.unwrap(); + writer.write_record(foo_bar).await.unwrap(); + + let foo_bar_path = store + .list(None) + .collect::>() + .await + .into_iter() + .filter_map(Result::ok) + .map(|meta| meta.location) + .find(|path| { + path.filename() + .is_some_and(|name| pb_types::newtype::storage_file_matches_asset(name, "foo_bar")) + }) + .unwrap(); + let hour_key = crate::writer::partition_hour_key(FIXED_TS_US); + let hour_start = FIXED_TS_US / 3_600_000_000 * 3_600_000_000; + let manifest = pb_types::ParquetRecoveryManifest { + version: pb_types::storage::PARQUET_RECOVERY_MANIFEST_VERSION, + dataset: "book_events".to_string(), + asset_key: "foo".to_string(), + hour_key: hour_key.clone(), + covered_start_us: hour_start, + covered_end_us: hour_start + 3_600_000_000, + // This is valid JSON metadata but deliberately points at a different + // asset's normal object, which must never enter reconcile's cleanup set. + active_objects: vec![foo_bar_path.to_string()], + superseded_objects: Vec::new(), + }; + let manifest_path = object_store::path::Path::from(format!( + "data/{}/book_events/{hour_key}/foo.json", + pb_types::storage::PARQUET_RECOVERY_MANIFEST_PREFIX + )); + store + .put( + &manifest_path, + PutPayload::from(serde_json::to_vec(&manifest).unwrap()), + ) + .await + .unwrap(); + + let coverage = + crate::writer::RecoveryCoverage::new(hour_start - 1, hour_start + 3_600_000_000).unwrap(); + let error = writer + .write_batch_replacing(&[foo], coverage) + .await + .unwrap_err(); + assert!(matches!(error, StoreError::UnsafeRecovery(_))); + assert!( + store.head(&foo_bar_path).await.is_ok(), + "out-of-scope object must survive a corrupt manifest" + ); +} + #[tokio::test] async fn writer_all_record_types_produce_valid_parquet() { let dir = TempDir::new().unwrap(); diff --git a/crates/pb-store/src/writer.rs b/crates/pb-store/src/writer.rs index 7ac4022e..93597db6 100644 --- a/crates/pb-store/src/writer.rs +++ b/crates/pb-store/src/writer.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{BTreeMap, BTreeSet}; use std::sync::Arc; use chrono::{Datelike, Timelike}; @@ -14,11 +14,17 @@ use parquet::file::properties::WriterProperties; use serde::Serialize; use pb_types::event::{BookEventKind, ExecutionEventKind, PersistedRecord, Side}; +use pb_types::storage::{ + PARQUET_RECOVERY_MANIFEST_PREFIX, PARQUET_RECOVERY_MANIFEST_VERSION, + PARQUET_RECOVERY_OBJECT_PREFIX, +}; +use pb_types::ParquetRecoveryManifest; use crate::error::StoreError; use crate::schema::{records_to_record_batch, schema_for_record}; const ROW_GROUP_SIZE: usize = 65_536; +const HOUR_US: u64 = 3_600_000_000; const CREATE_BOOK_EVENTS_DDL: &str = r#" CREATE TABLE IF NOT EXISTS book_events ( @@ -148,6 +154,58 @@ pub struct ParquetRecordWriter { base_path: String, } +/// Inclusive observed WAL timestamp span used to prove complete hourly coverage. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct RecoveryCoverage { + start_us: u64, + end_us: u64, +} + +impl RecoveryCoverage { + pub fn new(start_us: u64, end_us: u64) -> Result { + if start_us >= end_us { + return Err(StoreError::UnsafeRecovery(format!( + "WAL coverage must increase, got {start_us}..{end_us}" + ))); + } + if start_us < MIN_PLAUSIBLE_PARTITION_US || end_us > MAX_PLAUSIBLE_PARTITION_US { + return Err(StoreError::UnsafeRecovery(format!( + "WAL coverage {start_us}..{end_us} is outside the plausible timestamp range" + ))); + } + Ok(Self { start_us, end_us }) + } + + pub const fn start_us(self) -> u64 { + self.start_us + } + + pub const fn end_us(self) -> u64 { + self.end_us + } + + /// True only when the observed WAL spans the entire UTC hour containing + /// `timestamp_us`. Boundary hours are intentionally excluded. + pub fn contains_complete_hour(self, timestamp_us: u64) -> bool { + let hour_start = timestamp_us / HOUR_US * HOUR_US; + // A snapshot is persisted as many records with the same receive + // timestamp, and WAL retention may cut inside that equal-timestamp + // run. Seeing an earliest record exactly at the hour boundary therefore + // does not prove that all records at that boundary were retained. + self.start_us < hour_start + && hour_start + .checked_add(HOUR_US) + .is_some_and(|hour_end| self.end_us >= hour_end) + } +} + +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct RecoveryReport { + pub partitions_published: usize, + pub records_published: usize, + pub cleanup_failures: usize, +} + /// Lower bound for a plausible event timestamp (~2001-09-09 in µs). Anything /// below this is treated as corrupt/unstamped rather than a real 1970s event. const MIN_PLAUSIBLE_PARTITION_US: u64 = 1_000_000_000_000_000; @@ -196,6 +254,18 @@ impl ParquetRecordWriter { } } + fn child_path(&self, suffix: &str) -> ObjectPath { + let path = if self.base_path.is_empty() { + suffix.to_string() + } else { + format!("{}/{suffix}", self.base_path) + }; + // Runtime constructors provide either a canonical local path or the + // already-encoded prefix returned by `parse_url_opts`. Parsing preserves + // those encoded components; `Path::from` would encode `%` again. + ObjectPath::parse(path).expect("validated base plus internal suffix is a valid object path") + } + pub async fn write_record(&self, record: PersistedRecord) -> Result<(), StoreError> { self.write_batch(std::slice::from_ref(&record)).await } @@ -206,7 +276,7 @@ impl ParquetRecordWriter { } let flush_start = std::time::Instant::now(); - let mut groups: HashMap<(String, String, String), Vec<&PersistedRecord>> = HashMap::new(); + let mut groups: BTreeMap<(String, String, String), Vec<&PersistedRecord>> = BTreeMap::new(); for record in records { let hour_key = partition_hour_key(record.partition_timestamp_us()); groups @@ -228,30 +298,48 @@ impl ParquetRecordWriter { Ok(()) } - /// Rebuild Parquet partitions from an authoritative record stream (the WAL), - /// so a storage window lost to a crash mid-buffer can be recovered. - /// - /// For every `(dataset, asset, hour)` group present in `records`, the existing - /// Parquet files for that group are deleted and replaced with the complete - /// group rebuilt from `records`. This makes the source stream authoritative - /// for each touched partition and makes re-running idempotent (a second run - /// deletes and rewrites byte-identical content), avoiding the duplicate-rows - /// hazard of merging differently-batched files into the same hour. + /// Crash-consistently replace complete hourly partitions from a strict WAL + /// replay. /// - /// Intended for **offline** recovery (ingest stopped): a concurrent live sink - /// writing the same partitions would race the per-group delete. `records` - /// should be the full set for the reconciled window (the caller accumulates - /// the WAL replay) so each group is written complete in one pass. + /// Every record must be a book, trade, or ingest record partitioned exactly + /// by receive time, and must belong to an hour fully contained in `coverage`. + /// Recovery writes a new immutable object first, verifies it, then atomically + /// publishes a small manifest that makes it authoritative. Old files are + /// deleted only after publication, so a crash always leaves either the old or + /// new complete view readable. Boundary hours and independently timestamped + /// datasets are rejected rather than risking destructive partial replacement. pub async fn write_batch_replacing( &self, records: &[PersistedRecord], - ) -> Result<(), StoreError> { + coverage: RecoveryCoverage, + ) -> Result { if records.is_empty() { - return Ok(()); + return Ok(RecoveryReport::default()); } - let mut groups: HashMap<(String, String, String), Vec<&PersistedRecord>> = HashMap::new(); + let mut groups: BTreeMap<(String, String, String), Vec<&PersistedRecord>> = BTreeMap::new(); for record in records { + if !matches!( + record, + PersistedRecord::Book(_) | PersistedRecord::Trade(_) | PersistedRecord::Ingest(_) + ) { + return Err(StoreError::UnsafeRecovery(format!( + "dataset {} is not partitioned by receive time and has no complete-hour WAL coverage proof", + record.dataset_name() + ))); + } + let timestamp_us = record.partition_timestamp_us(); + if !coverage.contains_complete_hour(timestamp_us) { + return Err(StoreError::UnsafeRecovery(format!( + "record at {timestamp_us} belongs to a boundary hour not fully covered by WAL {}..{}", + coverage.start_us, coverage.end_us + ))); + } let hour_key = partition_hour_key(record.partition_timestamp_us()); + if hour_key == "invalid_timestamp" { + return Err(StoreError::UnsafeRecovery(format!( + "record at {timestamp_us} has no recoverable UTC hour" + ))); + } groups .entry(( record.dataset_name().to_string(), @@ -262,39 +350,297 @@ impl ParquetRecordWriter { .push(record); } - for ((dataset, asset, hour_key), records) in &groups { - self.delete_group(dataset, asset, hour_key).await?; - self.write_group(dataset, asset, hour_key, records).await?; + let mut report = RecoveryReport::default(); + for ((dataset, asset, hour_key), group_records) in &groups { + report.cleanup_failures += self + .replace_group(dataset, asset, hour_key, group_records, coverage) + .await?; + report.partitions_published += 1; + report.records_published += group_records.len(); } - Ok(()) + Ok(report) + } + + async fn replace_group( + &self, + dataset: &str, + asset: &str, + hour_key: &str, + records: &[&PersistedRecord], + coverage: RecoveryCoverage, + ) -> Result { + let existing_objects = self.group_objects(dataset, asset, hour_key).await?; + let abandoned_staged_objects = self + .recovery_group_objects(dataset, asset, hour_key) + .await?; + let manifest_path = self.recovery_manifest_path(dataset, asset, hour_key); + let previous_manifest = self + .read_recovery_manifest(&manifest_path, dataset, asset, hour_key) + .await?; + + let (buf, file_name) = self.encode_group(records)?; + let staged_path = self.child_path(&format!( + "{PARQUET_RECOVERY_OBJECT_PREFIX}/{dataset}/{hour_key}/{file_name}" + )); + // The final active object stays in the normal dataset partition so + // documented direct Parquet consumers continue to work after cleanup. + // It is first staged under a hidden prefix: putting it into the normal + // listing before a manifest exists would expose old+new rows and leave a + // duplicate orphan if the process crashed in that window. + let recovered_path = self.child_path(&format!("{dataset}/{hour_key}/{file_name}")); + let expected_size = buf.len() as u64; + self.store + .put(&staged_path, PutPayload::from(buf.clone())) + .await?; + let written = self.store.head(&staged_path).await?; + if written.size != expected_size { + return Err(StoreError::UnsafeRecovery(format!( + "staged recovery object {staged_path} has size {}, expected {expected_size}", + written.size + ))); + } + + let mut superseded: BTreeSet = + existing_objects.into_iter().map(String::from).collect(); + superseded.extend(abandoned_staged_objects.into_iter().map(String::from)); + if let Some(previous) = previous_manifest { + superseded.extend(previous.active_objects); + // Keep retrying any prior best-effort cleanup, including immutable + // recovery objects that are not discoverable from the normal + // partition listing. Dropping this set on the next manifest would + // leak an interrupted cleanup permanently. + superseded.extend(previous.superseded_objects); + } + // Publication phase 1: switch manifest-aware readers to the hidden + // staged object. Predeclare the future normal path as superseded so a + // reader remains on the staged view if it lists during the promotion PUT. + let mut staged_superseded = superseded.clone(); + staged_superseded.insert(recovered_path.to_string()); + staged_superseded.remove(staged_path.as_ref()); + let staged_manifest = ParquetRecoveryManifest { + version: PARQUET_RECOVERY_MANIFEST_VERSION, + dataset: dataset.to_string(), + asset_key: asset.to_string(), + hour_key: hour_key.to_string(), + covered_start_us: coverage.start_us, + covered_end_us: coverage.end_us, + active_objects: vec![staged_path.to_string()], + superseded_objects: staged_superseded.into_iter().collect(), + }; + self.publish_recovery_manifest(&manifest_path, &staged_manifest, dataset, asset, hour_key) + .await?; + + // Promotion phase: materialize the same verified bytes in the normal + // partition while phase 1 keeps that path invisible to application + // readers, then atomically point the manifest at it. + self.store + .put(&recovered_path, PutPayload::from(buf)) + .await?; + let promoted = self.store.head(&recovered_path).await?; + if promoted.size != expected_size { + return Err(StoreError::UnsafeRecovery(format!( + "promoted recovery object {recovered_path} has size {}, expected {expected_size}", + promoted.size + ))); + } + + let mut final_superseded = superseded; + final_superseded.insert(staged_path.to_string()); + final_superseded.remove(recovered_path.as_ref()); + let final_manifest = ParquetRecoveryManifest { + version: PARQUET_RECOVERY_MANIFEST_VERSION, + dataset: dataset.to_string(), + asset_key: asset.to_string(), + hour_key: hour_key.to_string(), + covered_start_us: coverage.start_us, + covered_end_us: coverage.end_us, + active_objects: vec![recovered_path.to_string()], + superseded_objects: final_superseded.iter().cloned().collect(), + }; + self.publish_recovery_manifest(&manifest_path, &final_manifest, dataset, asset, hour_key) + .await?; + + let mut cleanup_failures = 0; + for stale in final_superseded { + // Manifest paths are already object-store encoded. Re-parsing with + // `ObjectPath::from` would encode `%` again and delete a different + // key for assets that required percent encoding. + let stale_path = ObjectPath::parse(&stale).map_err(|error| { + StoreError::UnsafeRecovery(format!( + "invalid superseded object path {stale}: {error}" + )) + })?; + match self.store.delete(&stale_path).await { + Ok(()) | Err(object_store::Error::NotFound { .. }) => {} + Err(error) => { + cleanup_failures += 1; + tracing::warn!(path = %stale_path, error = %error, "recovery cleanup deferred"); + } + } + } + + tracing::info!( + dataset, + asset, + hour_key, + rows = records.len(), + manifest = %manifest_path, + cleanup_failures, + "published crash-consistent parquet recovery partition" + ); + Ok(cleanup_failures) } - /// Delete all existing Parquet files for one `(dataset, asset, hour)` group so - /// it can be rewritten authoritatively from the WAL. Files are named - /// `{asset}_{ts}_{hash}_{len}.parquet`; the asset key itself may contain - /// underscores, so matching must parse the filename suffix instead of using a - /// raw `{asset}_` prefix. - async fn delete_group( + async fn publish_recovery_manifest( &self, + path: &ObjectPath, + manifest: &ParquetRecoveryManifest, dataset: &str, asset: &str, hour_key: &str, ) -> Result<(), StoreError> { - let dir = format!("{}/{}/{}", self.base_path, dataset, hour_key); - let dir_path = ObjectPath::from(dir.as_str()); + manifest.validate().map_err(StoreError::UnsafeRecovery)?; + self.validate_recovery_manifest_scope(path, manifest, dataset, asset, hour_key)?; + let manifest_bytes = serde_json::to_vec(manifest)?; + self.store + .put(path, PutPayload::from(manifest_bytes)) + .await?; + let published = self + .read_recovery_manifest(path, dataset, asset, hour_key) + .await? + .ok_or_else(|| { + StoreError::UnsafeRecovery(format!( + "recovery manifest {path} disappeared after publication" + )) + })?; + if published != *manifest { + return Err(StoreError::UnsafeRecovery(format!( + "recovery manifest {path} failed read-after-write verification" + ))); + } + Ok(()) + } + + fn recovery_manifest_path(&self, dataset: &str, asset: &str, hour_key: &str) -> ObjectPath { + let manifest_name = format!("{asset}.json"); + self.child_path(&format!( + "{PARQUET_RECOVERY_MANIFEST_PREFIX}/{dataset}/{hour_key}/{manifest_name}" + )) + } + + async fn group_objects( + &self, + dataset: &str, + asset: &str, + hour_key: &str, + ) -> Result, StoreError> { + let dir_path = self.child_path(&format!("{dataset}/{hour_key}")); let existing = self.store.list(Some(&dir_path)).collect::>().await; + let mut matches = Vec::new(); for meta in existing { let meta = meta?; - let is_match = meta - .location - .filename() - .map(|name| pb_types::newtype::storage_file_matches_asset(name, asset)) - .unwrap_or(false); - if is_match { - self.store.delete(&meta.location).await?; - tracing::debug!(path = %meta.location, "reconcile: deleted stale parquet file"); + if meta.location.filename().is_some_and(|name| { + pb_types::newtype::storage_object_file_matches_asset(name, asset) + }) { + matches.push(meta.location); + } + } + Ok(matches) + } + + async fn recovery_group_objects( + &self, + dataset: &str, + asset: &str, + hour_key: &str, + ) -> Result, StoreError> { + let dir_path = self.child_path(&format!( + "{PARQUET_RECOVERY_OBJECT_PREFIX}/{dataset}/{hour_key}" + )); + let existing = self.store.list(Some(&dir_path)).collect::>().await; + let mut matches = Vec::new(); + for meta in existing { + let meta = meta?; + if meta.location.filename().is_some_and(|name| { + pb_types::newtype::storage_object_file_matches_asset(name, asset) + }) { + matches.push(meta.location); } } + Ok(matches) + } + + async fn read_recovery_manifest( + &self, + path: &ObjectPath, + dataset: &str, + asset: &str, + hour_key: &str, + ) -> Result, StoreError> { + let result = match self.store.get(path).await { + Ok(result) => result, + Err(object_store::Error::NotFound { .. }) => return Ok(None), + Err(error) => return Err(error.into()), + }; + let bytes = result.bytes().await?; + let manifest: ParquetRecoveryManifest = serde_json::from_slice(bytes.as_ref())?; + manifest.validate().map_err(StoreError::UnsafeRecovery)?; + self.validate_recovery_manifest_scope(path, &manifest, dataset, asset, hour_key)?; + Ok(Some(manifest)) + } + + /// Reject a manifest whose identity or object list escapes the partition it + /// claims to describe. This check runs before any previous active object is + /// added to the cleanup set, so corrupt object-store metadata cannot widen a + /// recovery deletion. + fn validate_recovery_manifest_scope( + &self, + path: &ObjectPath, + manifest: &ParquetRecoveryManifest, + dataset: &str, + asset: &str, + hour_key: &str, + ) -> Result<(), StoreError> { + let identity_matches = manifest.dataset == dataset + && manifest.asset_key == asset + && manifest.hour_key == hour_key + && path + .filename() + .and_then(|name| name.strip_suffix(".json")) + .is_some_and(|stem| { + pb_types::newtype::storage_object_component_matches_key(stem, asset) + }); + let recovery_prefix = self + .child_path(&format!( + "{PARQUET_RECOVERY_OBJECT_PREFIX}/{dataset}/{hour_key}" + )) + .to_string(); + let normal_prefix = self + .child_path(&format!("{dataset}/{hour_key}")) + .to_string(); + let matches_asset = |object: &str| { + object.rsplit('/').next().is_some_and(|name| { + pb_types::newtype::storage_object_file_matches_asset(name, asset) + }) + }; + let under_prefix = |object: &str, prefix: &str| { + object + .strip_prefix(prefix) + .is_some_and(|suffix| suffix.starts_with('/') && suffix.ends_with(".parquet")) + }; + let active_objects_are_scoped = manifest.active_objects.iter().all(|object| { + (under_prefix(object, &normal_prefix) || under_prefix(object, &recovery_prefix)) + && matches_asset(object) + }); + let superseded_objects_are_scoped = manifest.superseded_objects.iter().all(|object| { + (under_prefix(object, &normal_prefix) || under_prefix(object, &recovery_prefix)) + && matches_asset(object) + }); + if !identity_matches || !active_objects_are_scoped || !superseded_objects_are_scoped { + return Err(StoreError::UnsafeRecovery(format!( + "recovery manifest {path} does not match its partition scope" + ))); + } Ok(()) } @@ -307,8 +653,23 @@ impl ParquetRecordWriter { hour_key: &str, records: &[&PersistedRecord], ) -> Result<(), StoreError> { - let first_ts_us = records[0].partition_timestamp_us(); + let (buf, file_name) = self.encode_group(records)?; + let object_path = self.child_path(&format!("{dataset}/{hour_key}/{file_name}")); + self.store.put(&object_path, PutPayload::from(buf)).await?; + + tracing::debug!( + dataset = %dataset, + asset = %asset, + rows = records.len(), + path = %object_path, + "flushed parquet file" + ); + Ok(()) + } + fn encode_group(&self, records: &[&PersistedRecord]) -> Result<(Vec, String), StoreError> { + let first_ts_us = records[0].partition_timestamp_us(); + let asset = pb_types::newtype::storage_key_for(records[0].asset_partition()); let batch = records_to_record_batch(records)?; let schema = Arc::new(schema_for_record(records[0])); let props = WriterProperties::builder() @@ -350,28 +711,14 @@ impl ParquetRecordWriter { buf.hash(&mut hasher); hasher.finish() }; - let path = format!( - "{}/{}/{}/{}_{}_{:016x}_{}.parquet", - self.base_path, - dataset, - hour_key, + let file_name = format!( + "{}_{}_{:016x}_{}.parquet", asset, first_ts_us, content_hash, buf.len() ); - - let object_path = ObjectPath::from(path.as_str()); - self.store.put(&object_path, PutPayload::from(buf)).await?; - - tracing::debug!( - dataset = %dataset, - asset = %asset, - rows = records.len(), - path = %path, - "flushed parquet file" - ); - Ok(()) + Ok((buf, file_name)) } } diff --git a/crates/pb-types/README.md b/crates/pb-types/README.md index 1b7403e1..15265ee8 100644 --- a/crates/pb-types/README.md +++ b/crates/pb-types/README.md @@ -15,6 +15,7 @@ the system. | `Sequence` | Monotonically increasing event sequence number. `const fn` constructors and accessors. | | `SlugRegistry` | Maps condition IDs to human-readable market slugs. | | `PersistedRecord` | Enum dispatching to the six event datasets below. | +| `ParquetRecoveryManifest` | Versioned, atomically published object-store view for one recovered `(dataset, asset, hour)` partition. | | `time::normalize_to_micros` / `parse_to_micros` | The single timestamp-unit converter: classifies a raw value as s/ms/µs/ns by magnitude and returns microseconds (0 preserved as the unknown sentinel). Used by both the dispatcher and REST backfill so they never diverge. | ## Persisted Record Model @@ -75,8 +76,15 @@ pb-types ◄── pb-feed (wire deserialization) `{asset}_{first_ts}_{content_hash}_{len}.parquet` suffix from the right, so callers compare the exact asset component even when the encoded asset key contains underscores. +- `ParquetRecoveryManifest` is shared by `pb-store` and `pb-replay` so a + recovered object becomes authoritative through one versioned manifest format. + `_recovery_objects` holds the immutable staging generation used during the + first publication phase; `_recovery_manifests` holds the authoritative view. + A clean promotion moves authority to the corresponding object in the normal + dataset/hour tree, while interrupted runs may temporarily keep the staged + object active. - `proptest` suites verify fixed-point roundtrip, ordering, and serde consistency. -- 150 tests covering boundary conditions, serde round-trips, proptest invariants, +- 173 tests covering boundary conditions, serde round-trips, proptest invariants, and all persisted record types. ## Docs to Update After Changes diff --git a/crates/pb-types/src/lib.rs b/crates/pb-types/src/lib.rs index eceaa54c..0e7fb3e9 100644 --- a/crates/pb-types/src/lib.rs +++ b/crates/pb-types/src/lib.rs @@ -5,6 +5,7 @@ pub mod event; pub mod fixed; pub mod newtype; pub mod slug; +pub mod storage; pub mod time; pub mod wire; @@ -18,3 +19,4 @@ pub use event::{ pub use fixed::{FixedPrice, FixedSize}; pub use newtype::{AssetId, Sequence}; pub use slug::SlugRegistry; +pub use storage::ParquetRecoveryManifest; diff --git a/crates/pb-types/src/newtype.rs b/crates/pb-types/src/newtype.rs index 668da138..eae1fef4 100644 --- a/crates/pb-types/src/newtype.rs +++ b/crates/pb-types/src/newtype.rs @@ -70,6 +70,25 @@ pub fn storage_file_matches_asset(file_name: &str, asset_key: &str) -> bool { storage_file_asset_key(file_name) == Some(asset_key) } +/// Match a logical storage key against current filenames and legacy filenames +/// produced when an already-encoded key was encoded a second time. +pub fn storage_object_file_matches_asset(file_name: &str, asset_key: &str) -> bool { + let Some(file_asset) = storage_file_asset_key(file_name) else { + return false; + }; + storage_object_component_matches_key(file_asset, asset_key) +} + +/// Compare one object-path component with its logical, already-percent-encoded +/// storage key, accepting the legacy extra `%` escaping for compatibility. +pub fn storage_object_component_matches_key(component: &str, storage_key: &str) -> bool { + if component == storage_key { + return true; + } + let object_encoded = storage_key.replace('%', "%25"); + component == object_encoded +} + impl fmt::Display for AssetId { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.0) @@ -152,6 +171,15 @@ mod tests { assert!(!storage_file_matches_asset("foo_bar.parquet", "foo")); } + #[test] + fn storage_object_file_match_handles_percent_reencoding() { + let logical = storage_key_for("../a/b%"); + assert_eq!(logical, "..%2Fa%2Fb%25"); + let object_name = "..%252Fa%252Fb%2525_1700000000000000_0123456789abcdef_42.parquet"; + assert!(storage_object_file_matches_asset(object_name, &logical)); + assert!(!storage_object_file_matches_asset(object_name, "another")); + } + #[test] fn test_sequence_ordering() { let a = Sequence::new(1); diff --git a/crates/pb-types/src/storage.rs b/crates/pb-types/src/storage.rs new file mode 100644 index 00000000..e907909a --- /dev/null +++ b/crates/pb-types/src/storage.rs @@ -0,0 +1,95 @@ +//! Shared storage-layout metadata used by Parquet writers and readers. + +use serde::{Deserialize, Serialize}; + +/// Current format version for crash-consistent Parquet recovery manifests. +pub const PARQUET_RECOVERY_MANIFEST_VERSION: u32 = 1; + +/// Hidden object-store prefix containing the authoritative recovery manifests. +pub const PARQUET_RECOVERY_MANIFEST_PREFIX: &str = "_recovery_manifests"; + +/// Hidden staging prefix used during two-phase recovery publication. A staged +/// object may temporarily be authoritative between the staged and final +/// manifest cuts; successful recovery promotes it to the normal partition and +/// removes the hidden object. +pub const PARQUET_RECOVERY_OBJECT_PREFIX: &str = "_recovery_objects"; + +/// Atomically published view of one recovered `(dataset, asset, hour)` partition. +/// +/// Recovery publishes a staged generation and then a promoted normal-tree +/// generation through this small manifest. Readers switch views at each +/// manifest replacement. `superseded_objects` keeps old files invisible when +/// cleanup has not completed yet. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ParquetRecoveryManifest { + pub version: u32, + pub dataset: String, + pub asset_key: String, + pub hour_key: String, + pub covered_start_us: u64, + pub covered_end_us: u64, + pub active_objects: Vec, + pub superseded_objects: Vec, +} + +impl ParquetRecoveryManifest { + pub fn validate(&self) -> Result<(), String> { + if self.version != PARQUET_RECOVERY_MANIFEST_VERSION { + return Err(format!( + "unsupported recovery manifest version {}, expected {}", + self.version, PARQUET_RECOVERY_MANIFEST_VERSION + )); + } + if self.dataset.is_empty() || self.asset_key.is_empty() || self.hour_key.is_empty() { + return Err("recovery manifest identity fields must not be empty".to_string()); + } + if self.covered_start_us >= self.covered_end_us { + return Err("recovery manifest coverage must be an increasing range".to_string()); + } + if self.active_objects.is_empty() { + return Err("recovery manifest must name at least one active object".to_string()); + } + if self + .active_objects + .iter() + .any(|active| self.superseded_objects.contains(active)) + { + return Err("recovery manifest cannot supersede an active object".to_string()); + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn valid_manifest() -> ParquetRecoveryManifest { + ParquetRecoveryManifest { + version: PARQUET_RECOVERY_MANIFEST_VERSION, + dataset: "book_events".to_string(), + asset_key: "token-1".to_string(), + hour_key: "2026/07/21/10".to_string(), + covered_start_us: 1_774_000_000_000_000, + covered_end_us: 1_774_003_600_000_000, + active_objects: vec!["data/_recovery_objects/book.parquet".to_string()], + superseded_objects: vec!["data/book_events/old.parquet".to_string()], + } + } + + #[test] + fn recovery_manifest_accepts_current_complete_shape() { + assert!(valid_manifest().validate().is_ok()); + } + + #[test] + fn recovery_manifest_rejects_unknown_version_and_empty_active_set() { + let mut manifest = valid_manifest(); + manifest.version += 1; + assert!(manifest.validate().is_err()); + + let mut manifest = valid_manifest(); + manifest.active_objects.clear(); + assert!(manifest.validate().is_err()); + } +} diff --git a/crates/pb-wal/README.md b/crates/pb-wal/README.md index 37cb26a9..ffdba623 100644 --- a/crates/pb-wal/README.md +++ b/crates/pb-wal/README.md @@ -10,6 +10,7 @@ between the `ingest` and `serve` processes. | `WalWriter` | Appends records to the active segment, rotates on size threshold, seals completed segments. | | `WalReader` | Tails across segments with independent consumer position tracking. Resumes from committed offset on restart. | | `WalPosition` | Typed `(segment_id, offset)` handoff point for resuming a reader without consulting a consumer position file. | +| `WalMaintenanceGuard` | Exclusive writer lease for offline maintenance; refuses acquisition while ingest owns the WAL. | | `WalConfig` | Segment size, base directory, max retained segments, max consumer lag bytes, and live reader position commit interval. | | `WalError` | Error type for WAL operations (IO, CRC, codec). | | `codec::encode` / `codec::decode` | Version-prefixed bincode serialization for `PersistedRecord`. | @@ -105,6 +106,11 @@ and feeds decoded records into the live read model. - `WalReader::open_at()` allows a runtime to hand off directly from hydration to live tailing without replaying WAL records that were already applied during startup. +- `WalReader::open_from_start()` plus `next_strict()` provides a fail-closed + recovery scan over every retained segment. Unlike the live reader, it returns + CRC, truncation, and internal segment-gap errors instead of skipping damage. +- `WalMaintenanceGuard` reuses the writer's advisory lease so destructive + maintenance cannot race a live ingest writer. - **Atomic position writes**: position files are written to a temp file first, then fsynced and renamed into place, and the parent directory is fsynced afterward, preventing partial reads or lost renames on crash. diff --git a/crates/pb-wal/src/lib.rs b/crates/pb-wal/src/lib.rs index baa791a7..f8ea92e1 100644 --- a/crates/pb-wal/src/lib.rs +++ b/crates/pb-wal/src/lib.rs @@ -14,7 +14,7 @@ mod writer; pub use error::WalError; pub use reader::{WalPosition, WalReader}; pub use segment::HEADER_SIZE; -pub use writer::WalWriter; +pub use writer::{WalMaintenanceGuard, WalWriter}; /// Configuration for the write-ahead log. #[derive(Debug, Clone)] @@ -97,7 +97,7 @@ mod tests { } writer.flush().unwrap(); - let mut reader = WalReader::open(config, "test-consumer").unwrap(); + let mut reader = WalReader::open(config.clone(), "test-consumer").unwrap(); for expected in &payloads { let record = reader.next().unwrap(); assert!(record.is_some(), "expected a record"); @@ -144,7 +144,7 @@ mod tests { "expected multiple segments, got {segment_count}" ); - let mut reader = WalReader::open(config, "test-consumer").unwrap(); + let mut reader = WalReader::open(config.clone(), "test-consumer").unwrap(); let mut total_read = 0; while let Some(data) = reader.next().unwrap() { assert_eq!(data, payload); @@ -189,7 +189,7 @@ mod tests { } std::fs::write(&seg_path, &data).unwrap(); - let mut reader = WalReader::open(config, "test-consumer").unwrap(); + let mut reader = WalReader::open(config.clone(), "test-consumer").unwrap(); // First record should be fine. let first = reader.next().unwrap(); assert_eq!(first.as_deref(), Some(b"good-record".as_slice())); @@ -201,6 +201,16 @@ mod tests { // No more records. assert!(reader.next().unwrap().is_none()); + + let mut strict = WalReader::open_from_start(config, "strict-recovery").unwrap(); + assert_eq!( + strict.next_strict().unwrap().as_deref(), + Some(b"good-record".as_slice()) + ); + assert!(matches!( + strict.next_strict().unwrap_err(), + WalError::CrcMismatch { .. } + )); } /// A reader that has cached a segment including an incomplete (torn) trailing @@ -746,6 +756,50 @@ mod tests { assert!(WalWriter::open(config).is_ok()); } + #[test] + fn maintenance_guard_requires_writer_to_be_offline() { + let dir = tempfile::tempdir().unwrap(); + let config = WalConfig { + base_path: dir.path().to_path_buf(), + segment_size: 4096, + max_segments: 4, + ..WalConfig::default() + }; + let writer = WalWriter::open(config.clone()).unwrap(); + assert!(matches!( + WalMaintenanceGuard::acquire(&config).unwrap_err(), + WalError::WriterLocked { .. } + )); + drop(writer); + assert!(WalMaintenanceGuard::acquire(&config).is_ok()); + } + + #[test] + fn strict_reader_rejects_internal_segment_gap() { + let dir = tempfile::tempdir().unwrap(); + let config = WalConfig { + base_path: dir.path().to_path_buf(), + segment_size: 32, + max_segments: 8, + ..WalConfig::default() + }; + let payload = vec![7u8; 20]; + let mut writer = WalWriter::open(config.clone()).unwrap(); + for _ in 0..3 { + writer.append(&payload).unwrap(); + } + writer.sync().unwrap(); + drop(writer); + std::fs::remove_file(config.base_path.join("segment_00000000000000000001.wal")).unwrap(); + + let mut reader = WalReader::open_from_start(config, "strict-gap").unwrap(); + assert_eq!(reader.next_strict().unwrap(), Some(payload)); + assert!(matches!( + reader.next_strict().unwrap_err(), + WalError::SegmentGap { .. } + )); + } + #[test] fn standby_writer_takes_over_shared_wal_after_primary_exit() { // Writer-failover mechanism: the flock makes a @@ -1587,11 +1641,17 @@ mod tests { std::fs::write(&seg_files[0], &data).unwrap(); // The reader must terminate (not hang). - let mut reader = WalReader::open(config, "zero-len-test").unwrap(); + let mut reader = WalReader::open(config.clone(), "zero-len-test").unwrap(); let mut count = 0; while let Ok(Some(_)) = reader.next() { count += 1; assert!(count < 100, "reader appears to be looping"); } + + let mut strict = WalReader::open_from_start(config, "zero-len-strict").unwrap(); + assert!(matches!( + strict.next_strict().unwrap_err(), + WalError::TruncatedRecord { .. } + )); } } diff --git a/crates/pb-wal/src/reader.rs b/crates/pb-wal/src/reader.rs index b0963a25..36172b3e 100644 --- a/crates/pb-wal/src/reader.rs +++ b/crates/pb-wal/src/reader.rs @@ -55,6 +55,24 @@ impl WalReader { Ok(reader) } + /// Open at the earliest retained segment, ignoring any committed consumer + /// position. Offline recovery uses this to inspect the entire retained WAL + /// without mutating or trusting a previous maintenance cursor. + pub fn open_from_start(config: WalConfig, consumer_name: &str) -> Result { + let available = segment::list_segment_ids(&config.base_path)?; + let start_seg = available.first().copied().unwrap_or(0); + let mut reader = Self { + config, + consumer_name: consumer_name.to_string(), + current_segment_id: start_seg, + current_offset: 0, + current_data: None, + available_segments: available, + }; + reader.load_segment(start_seg)?; + Ok(reader) + } + /// Open a WAL reader at an explicit position instead of the last committed /// consumer position. This is useful for handing off from checkpoint/WAL /// hydration directly into live tailing without replaying already-applied @@ -150,6 +168,54 @@ impl WalReader { } } + /// Read the next record without skipping corruption or internal segment gaps. + /// + /// Live readers deliberately tolerate and surface some damage so they can + /// continue serving. Recovery must be stricter: publishing a partition from + /// a stream with a missing frame would make that incomplete stream + /// authoritative and permanently delete valid history. + pub fn next_strict(&mut self) -> Result>, WalError> { + loop { + if let Some(data) = &self.current_data { + if self.current_offset < data.len() + && data.len() - self.current_offset < crate::FRAME_HEADER_LEN + { + return Err(WalError::TruncatedRecord { + segment_id: self.current_segment_id, + offset: self.current_offset as u64, + }); + } + if self.current_offset < data.len() { + let len = u32::from_le_bytes( + data[self.current_offset..self.current_offset + 4] + .try_into() + .expect("strict reader checked the complete frame header"), + ); + if len == 0 { + return Err(WalError::TruncatedRecord { + segment_id: self.current_segment_id, + offset: self.current_offset as u64, + }); + } + } + match segment::read_record_at(data, self.current_offset, self.current_segment_id) { + Ok(Some((payload, next_offset))) => { + self.current_offset = next_offset; + return Ok(Some(payload)); + } + Ok(None) => { + if !self.advance_segment_strict()? { + return Ok(None); + } + } + Err(error) => return Err(error), + } + } else if !self.reload_when_unloaded()? { + return Ok(None); + } + } + } + /// Commit the current read position atomically to disk so it survives /// restarts. Uses write-to-temp + rename for crash safety. pub fn commit_position(&self) -> Result<(), WalError> { @@ -282,6 +348,28 @@ impl WalReader { } } + fn advance_segment_strict(&mut self) -> Result { + self.available_segments = segment::list_segment_ids(&self.config.base_path)?; + let next = self + .available_segments + .iter() + .find(|&&id| id > self.current_segment_id) + .copied(); + + match next { + Some(next_id) if next_id != self.current_segment_id + 1 => Err(WalError::SegmentGap { + consumer: self.consumer_name.clone(), + committed_segment: self.current_segment_id, + earliest_available: next_id, + }), + Some(next_id) => { + self.current_offset = 0; + self.load_segment(next_id) + } + None => self.top_up_current_segment(), + } + } + /// Incrementally pick up bytes appended to the current (active) segment /// since it was last read. /// diff --git a/crates/pb-wal/src/writer.rs b/crates/pb-wal/src/writer.rs index e79fa13a..a4489756 100644 --- a/crates/pb-wal/src/writer.rs +++ b/crates/pb-wal/src/writer.rs @@ -12,18 +12,24 @@ pub struct WalWriter { /// Holds the exclusive advisory lock on the WAL directory for this writer's /// lifetime. Dropping it (or process exit) releases the lock — flock is /// crash-safe, leaving no stale lock to clear. + _lock: WalMaintenanceGuard, +} + +/// Exclusive WAL lease used by offline maintenance commands. +/// +/// Holding this guard proves that no live `WalWriter` is running against the +/// directory. The advisory lock is released automatically on drop or process +/// exit. +#[derive(Debug)] +pub struct WalMaintenanceGuard { _lock: std::fs::File, } -impl WalWriter { - /// Open or create a WAL at the configured path. - pub fn open(config: WalConfig) -> Result { +impl WalMaintenanceGuard { + pub fn acquire(config: &WalConfig) -> Result { std::fs::create_dir_all(&config.base_path) .map_err(|e| WalError::io(&config.base_path, e))?; - // Acquire an exclusive, non-blocking advisory lock so a second writer on - // the same directory fails fast instead of interleaving appends and - // corrupting the WAL. let lock_path = config.base_path.join(".wal.lock"); let lock_file = std::fs::OpenOptions::new() .create(true) @@ -35,21 +41,22 @@ impl WalWriter { &lock_file, rustix::fs::FlockOperation::NonBlockingLockExclusive, ) { - Ok(()) => {} - // EWOULDBLOCK (== EAGAIN on this platform) means another writer holds - // the exclusive lock — fail fast instead of interleaving appends. - Err(rustix::io::Errno::WOULDBLOCK) => { - return Err(WalError::WriterLocked { - path: config.base_path.clone(), - }); - } - Err(e) => { - return Err(WalError::io( - &lock_path, - std::io::Error::from_raw_os_error(e.raw_os_error()), - )); - } + Ok(()) => Ok(Self { _lock: lock_file }), + Err(rustix::io::Errno::WOULDBLOCK) => Err(WalError::WriterLocked { + path: config.base_path.clone(), + }), + Err(e) => Err(WalError::io( + &lock_path, + std::io::Error::from_raw_os_error(e.raw_os_error()), + )), } + } +} + +impl WalWriter { + /// Open or create a WAL at the configured path. + pub fn open(config: WalConfig) -> Result { + let lock = WalMaintenanceGuard::acquire(&config)?; let ids = segment::list_segment_ids(&config.base_path)?; @@ -69,7 +76,7 @@ impl WalWriter { config, active, next_segment_id: next_id, - _lock: lock_file, + _lock: lock, }) } diff --git a/docs/adr/0009-dual-sink-storage.md b/docs/adr/0009-dual-sink-storage.md index 5a4590ab..769eab32 100644 --- a/docs/adr/0009-dual-sink-storage.md +++ b/docs/adr/0009-dual-sink-storage.md @@ -27,11 +27,11 @@ with an explicit division of authority: - **`ParquetSink` — cold, source of truth.** 5-minute flush windows, Zstd level-3 compression, `DELTA_BINARY_PACKED` on timestamp/price/size/sequence - columns. Written through the `object_store` trait, so local disk, S3, and - GCS are interchangeable without code changes. Object names embed a content - hash + byte length, making retries idempotent and silent overwrites - impossible. Replay correctness (`pb-replay`) and crash recovery - (`reconcile`) read from Parquet. + columns. Written through the `object_store` trait with local-disk and S3 + backends. Object names embed a content hash + byte length, making retries + idempotent and silent overwrites impossible. Replay correctness (`pb-replay`) + reads Parquet; crash recovery (`reconcile`) strictly reads retained WAL and + republishes complete, receive-time-partitioned Parquet hours. - **`ClickHouseSink` — warm, interactive.** 1-second batches (or 10,000 rows), `MergeTree` tables partitioned by date with composite ORDER BY keys. Per-batch deduplication tokens make re-inserts after partial failures @@ -61,8 +61,9 @@ the Parquet backend with a warning. same source-of-truth reason as ClickHouse-only. ## Consequences -- **Each read pattern gets the right engine**: replay and reconciliation work - from immutable portable files; the workstation gets indexed columnar SQL. +- **Each read pattern gets the right engine**: replay works from immutable + portable files, recovery validates the durable WAL before publishing those + files, and the workstation gets indexed columnar SQL. - **Dual write paths are a real cost**: two flush cadences, two retry paths, and two schema representations to keep in step. Mitigated by shared schema functions, a single version constant, and both sinks flushing with bounded @@ -72,8 +73,8 @@ the Parquet backend with a warning. cross-backend equivalence tests (replay, integrity, execution in `tests/integration/cross_backend_service.rs`) verify both backends give the same answers over the same records, and `reconcile` rebuilds a lost Parquet - window from the WAL. The equivalence tests are Docker-backed and - `#[ignore]`d — they run locally, not in CI (TESTING.md row 7). + window from the WAL. The equivalence tests are Docker-backed and `#[ignore]`d + for ordinary unit runs; the `integration-docker` CI job executes them. - **Storage lag never blocks durability**: sinks consume from their own bounded fan-out channels and may fall behind; the WAL (ADR-0008) remains the unconditionally-blocking consumer, so a slow sink degrades freshness, diff --git a/docs/api.md b/docs/api.md index ba36cbdb..16aee790 100644 --- a/docs/api.md +++ b/docs/api.md @@ -14,6 +14,10 @@ Timeline`, `Integrity`, and `Query Workbench`. The Query Workbench is opt-in (`api.query_workbench_enabled = true`, requires the ClickHouse backend). Latency surfaces remain deferred. +Parquet-backed routes are storage-location agnostic: local files and configured +cloud object stores use the same reader, and crash-recovery manifests are applied +below the service layer without changing response shapes. + ## Security / Trust Boundary HTTP, WebSocket, and gRPC data surfaces support a shared bearer token via diff --git a/docs/architecture.md b/docs/architecture.md index e4ee0049..af13f175 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -199,8 +199,13 @@ happens when a consumer falls behind is explicit: - **Storage sinks may lag.** Sinks consume from their own bounded fan-out channels and flush with bounded-retry. A sink falling behind does not block ingest indefinitely; sustained failure surfaces as `pb_sink_flush_failures_total` - (alerted) and the WAL remains the source of truth — a lost storage window is - rebuildable with `reconcile`. + (alerted). A lost Parquet window is recoverable only for a complete, strictly + decoded WAL hour: `reconcile` writes a replacement object, atomically publishes + its manifest, and then cleans old objects. This applies only to book, trade, + and ingest datasets partitioned by receive time; independently timestamped + datasets and partial boundary hours fail closed. + Validation is a streaming first pass; publication replays at most one complete + hour in memory at a time. - **Channel depth is observable.** The ingest event-channel depth is exported as `pb_channel_depth{channel="ingest_events"}`; rising depth is the leading indicator of a downstream stall, before latency (`pb_recv_to_durable_us`) diff --git a/docs/operations.md b/docs/operations.md index 7c490cb4..3f8d05f4 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -44,9 +44,9 @@ reconnect_max_delay_ms = 30000 rate_limit_requests = 1500 rate_limit_window_secs = 10 -# parquet_base_path accepts a local path OR a URL scheme (s3://bucket/prefix, -# gs://..., file://...); an s3:// path is wired to a real S3 object store. -# Cloud backends are configured from the process environment (parsed via +# parquet_base_path accepts a local path, file:// URL, or s3://bucket/prefix. +# An s3:// path is wired to a real S3 object store. S3 is configured from the +# process environment (parsed via # object_store::parse_url_opts(url, env::vars())): for s3://, set AWS_REGION and # either static AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY or rely on the default AWS # provider chain (ECS task role / instance profile). For an S3-compatible endpoint @@ -151,12 +151,23 @@ cargo run -- serve-api --tokens ## Data Layout -Parquet data is partitioned by dataset and time: +Parquet data is partitioned by dataset and time. Offline recovery also owns two +hidden prefixes below the configured Parquet base path: ```text -data//////*.parquet +data/ +├── /////*.parquet +├── _recovery_objects//////*.parquet +│ # temporary immutable staging objects; an interrupted run may leave one active +└── _recovery_manifests//////.json + # authoritative manifest-aware view for a recovered asset/hour ``` +Successful recovery promotes the active object into the normal dataset tree and +removes its hidden staging object. The manifest remains authoritative so an +interrupted cleanup cannot make superseded normal files visible to application +readers. Raw DuckDB/Polars globs do not interpret these manifests. + Primary datasets: - `book_events` @@ -220,12 +231,15 @@ deliberate act. store: stop `ingest`, let `serve` consume to the end, then let the new build create fresh segments. Old segments are not auto-upgraded — a version mismatch is surfaced, not papered over. -5. **Re-snapshot / backfill (Parquet/ClickHouse).** Historical Parquet files keep +5. **Re-snapshot / migrate Parquet and ClickHouse.** Historical Parquet files keep their original `pb_schema_version`. Either (a) keep readers able to accept the - prior version for the retention window, or (b) re-write affected partitions - from the source-of-truth (`backfill` / `reconcile`) so all files carry the new - version. ClickHouse column changes go through a normal `ALTER TABLE` migration - before the new writer starts. + prior version for the retention window, or (b) use a dedicated migration or + re-export from the durable source of truth for every affected dataset. + `backfill` only creates book checkpoints, while `reconcile` only republishes + complete retained-WAL hours for book, trade, and ingest data; neither is a + general migration path for all six datasets or arbitrary history. ClickHouse + column changes go through a normal `ALTER TABLE` migration before the new + writer starts. 6. **Verify with replay.** Run the golden replay regression and a `replay validate` over a migrated window before promoting the deploy. @@ -449,16 +463,60 @@ Parquet — but the WAL captured the same records durably. To rebuild the lost storage window from the WAL: ```bash -# Stop the ingest process first (reconcile must run offline — it replaces whole -# (dataset, asset, hour) partitions and would race a live sink). +# Stop ingest and every other process writing the same Parquet prefix first. +# Reconcile acquires the exclusive WAL lease and refuses to run while ingest is +# active; standalone backfill/append writers must be stopped operationally. cargo run -- reconcile ``` -`reconcile` reads the retained WAL and, for every `(dataset, asset, hour)` -partition it covers, deletes the existing Parquet files and rewrites the complete -partition from the WAL. It is idempotent (safe to re-run) and authoritative for -any partition it touches. It does not commit a consumer position, so each run -reconciles the full retained WAL. ClickHouse is not rebuilt by this command. +`reconcile` starts at the earliest retained segment and uses strict WAL reads: +CRC failures, truncation, internal segment gaps, and codec failures abort before +publication. A UTC hour is eligible only when the first supported receive +timestamp is strictly before its start and the last supported receive timestamp +reaches or passes its end. This deliberately excludes the hour containing the +first retained record even when that record lands exactly on an hour boundary: +retention may have cut through an equal-timestamp snapshot batch. A trailing +partial hour is also skipped. An empty WAL is a successful no-op; a non-empty WAL +that proves no complete eligible hour exits non-zero and changes no Parquet data. +Receive timestamps must also be nondecreasing in WAL order; a clock/order +regression fails closed rather than weakening the coverage proof. +The command validates coverage in a first streaming pass, then replays one +eligible hour at a time, so it does not materialize the whole retained WAL in +memory. Before publication, it also refuses any eligible hour above 2,000,000 +records or 128 MiB of encoded WAL payload to bound working-set risk for the +default 512 MiB maintenance-task class. + +Only datasets partitioned exactly by receive time are eligible: `book_events`, +`trade_events`, and `ingest_events`. `book_checkpoints` uses exchange snapshot +time, while validation and execution timestamps are independently supplied; +those partitions are left untouched because the retained WAL endpoints cannot +prove them complete. + +For each eligible `(dataset, asset, hour)`, recovery performs a two-phase cut: + +1. Write and size-verify an immutable object under `_recovery_objects/`, then + publish a versioned manifest under `_recovery_manifests/` that makes that + staged object authoritative to manifest-aware readers. +2. Write and verify the same bytes in the normal dataset/hour partition, then + publish the final manifest pointing at the promoted normal object. +3. Delete superseded normal/staged objects. Their manifest tombstones remain so + delayed or manually restored stale objects cannot become visible again. + +Manifest-aware readers switch at each publication point and ignore superseded +files, so a crash leaves a complete authoritative view. Cleanup is retryable by +re-running `reconcile`. If any deletion remains unresolved, the authoritative +application view is still published but the command exits non-zero; direct +DuckDB/Polars scans are unsafe until a rerun reports zero cleanup failures. +After clean promotion and cleanup, the active object is in the normal tree for +direct inspection. The command is idempotent and does not commit a consumer +position. ClickHouse is not rebuilt by this command. +Do not run `backfill`, `execution-append`, or another Parquet-writing task +against the same prefix concurrently: those commands do not share the WAL lease, +and a write racing the manifest cut would make the recovered view ambiguous. +Application reads retry manifest resolution once if cleanup crosses a request. +Stop ad-hoc DuckDB/Polars scans during the maintenance cut, and do not resume +them after a non-zero cleanup result, because those tools do not interpret +recovery manifests. In ECS, run this as a maintenance task using the dedicated reconcile task role; the always-on ingest task role intentionally lacks `s3:DeleteObject`. @@ -480,7 +538,7 @@ Recovery objectives by failure mode: |---|---|---|---| | `serve` (API) process crash | Restart; re-hydrate from latest checkpoint + WAL tail | 0 (read-only; no data originates here) | seconds — bounded by checkpoint hydration + WAL replay | | `ingest` process crash, same host | Restart `ingest`; flock is already released; it resumes appending to the last segment | ≤ `wal.sync_interval_ms` of un-`fdatasync`'d records (default 200 ms) on OS-crash/power-loss; **0** on a clean process kill | seconds — process start + lock acquire | -| Parquet sink buffer lost (OOM/SIGKILL) | `reconcile` rebuilds affected partitions from the durable WAL (offline) | 0 for any window the WAL still retains | minutes — offline rebuild, scales with window | +| Parquet sink buffer lost (OOM/SIGKILL) | `reconcile` strictly republishes complete hourly partitions from the durable WAL (offline); the earliest retained hour and any trailing partial hour remain untouched | 0 for a fully covered, gap-free WAL hour | minutes — offline rebuild, scales with window | | Host loss (WAL on durable/EFS volume) | Run a hot standby `ingest --standby` against the shared WAL volume; it waits on the lock and **auto-promotes** the moment the primary's lock releases | ≤ last synced records, as above | seconds-to-minutes — standby poll interval + feed connect, once the standby is already running | | Host loss (WAL on ephemeral storage) | Parquet on S3 is durable; the in-flight WAL tail is lost | the un-flushed Parquet window not yet mirrored to the WAL volume | minutes | diff --git a/docs/serve-api.md b/docs/serve-api.md index a1132fdd..faf05cce 100644 --- a/docs/serve-api.md +++ b/docs/serve-api.md @@ -93,7 +93,9 @@ token when one is configured. Historical routes (replay, integrity, execution) are served through `pb-service` traits with configurable backends: -- `api.historical_backend = "parquet"` (default) — uses `ParquetReader` via `pb-replay` +- `api.historical_backend = "parquet"` (default) — uses `ParquetReader` via + `pb-replay`; local files and cloud object stores share the same range-read path + and recovered partitions are selected through atomic recovery manifests - `api.historical_backend = "clickhouse"` — uses `ClickHouseReader` via `pb-replay` If ClickHouse is configured but unavailable at startup, the system probes @@ -115,7 +117,12 @@ read model is fed directly from the dispatcher channel. - **`serve`**: Reads the latest `BookCheckpoint` from Parquet, replays WAL from the checkpoint's offset, then live-tails the WAL for new records. Serves HTTP/WS. The live WAL consumer commits its position periodically during steady-state - tailing. + tailing. Startup inventories checkpoints once for all configured assets and + applies a distinct WAL cutoff for each asset; any asset without a checkpoint + is replayed from the earliest retained WAL record, and a checkpoint without a + WAL cut is ignored rather than mixed with older WAL records. Readiness stays at 503 if + object-store checkpoint access, WAL access, or WAL decoding fails; an + empty/partial recovery is not marked hydrated. The `serve` process can be killed and restarted without data loss. On restart it re-hydrates from the latest checkpoint and catches up from the WAL. diff --git a/infra/iam.tf b/infra/iam.tf index 6d1971f0..9f47cf0d 100644 --- a/infra/iam.tf +++ b/infra/iam.tf @@ -143,8 +143,9 @@ resource "aws_iam_role_policy" "ecs_task_reconcile_s3" { Version = "2012-10-17" Statement = [ { - # Required by the offline `reconcile` command, which replaces stale - # Parquet partitions (delete-then-write) when rebuilding from the WAL. + # Required by the offline `reconcile` command, which stages and + # manifest-publishes replacement Parquet partitions before cleaning up + # superseded objects. # Keep this destructive permission off the always-on ingest/serve role. Effect = "Allow" Action = [ diff --git a/monitoring/RUNBOOK.md b/monitoring/RUNBOOK.md index c346eb46..b7d3b5c6 100644 --- a/monitoring/RUNBOOK.md +++ b/monitoring/RUNBOOK.md @@ -90,8 +90,13 @@ its buffer; only after `MAX_FLUSH_RETRIES` does it surface the error). 1. Identify the sink from the `sink` label (`parquet` / `clickhouse`). 2. Parquet: check the object store (S3 creds/role, bucket policy, KMS key access), or local disk. ClickHouse: check the server is up and the schema matches. -3. The WAL still has every record. Once storage is healthy, rebuild any lost - Parquet window with `cargo run -- reconcile` (offline — stop ingest first). +3. If the failed sink is Parquet and the retained WAL is intact, `reconcile` can + republish only complete receive-time-partitioned book/trade/ingest hours. + Stop ingest and every other writer using the same Parquet prefix, then follow + `docs/operations.md` “Storage Recovery”. Partial boundary hours and + checkpoint/validation/execution datasets require a separate source-backed + repair. `reconcile` does not rebuild ClickHouse; repair or re-ingest that + backend separately. ## WalDecodeError **Severity: critical — serve read model has diverged from the WAL.** @@ -108,8 +113,13 @@ likely, a CRC collision on a corrupt frame. 2. The skipped record is permanently absent from that serve node's in-memory read model; restart serve to re-hydrate from checkpoints + replay the WAL (note: if the frame is genuinely corrupt, re-hydration will also skip it). -3. If it recurs across restarts, the WAL has a poison frame — `reconcile` the - affected window from the source and investigate the codec/segment. +3. If it recurs with matching binaries, preserve the WAL segment as incident + evidence and investigate the exact frame and codec version. Do **not** run + `reconcile` as a repair: strict recovery aborts on every undecodable or corrupt + frame so it cannot prove an authoritative Parquet hour. Restore a trusted WAL + copy or rebuild the affected storage interval from an independent upstream + source; drain/rotate the damaged WAL only after the impact and data-loss + decision are explicit. ## FeedSilent **Severity: critical — capture gap.** diff --git a/monitoring/alerts.yml b/monitoring/alerts.yml index 73056c29..409d6e74 100644 --- a/monitoring/alerts.yml +++ b/monitoring/alerts.yml @@ -64,8 +64,9 @@ groups: summary: "Storage sink {{ $labels.sink }} flush failing on {{ $labels.instance }}" description: >- A Parquet/ClickHouse flush failed (retried with backoff). Sustained - failures mean storage is falling behind the WAL; data is still durable - in the WAL and can be rebuilt with `reconcile`. + failures mean storage is falling behind the WAL. Complete retained-WAL + hours may be republished to Parquet offline with `reconcile`; ClickHouse + and partial/unprovable Parquet intervals require their own repair path. runbook: monitoring/RUNBOOK.md#sinkflushfailing - alert: WalDecodeError @@ -79,7 +80,8 @@ groups: A CRC-valid WAL frame could not be decoded by the codec and was skipped during live tailing — the serve read model has diverged from the WAL for that record. This should be near-impossible (a codec version mismatch or - CRC collision). Investigate the WAL/codec version and reconcile. + CRC collision). Investigate the WAL/codec version; strict `reconcile` + refuses this frame and is not a repair for this alert. runbook: monitoring/RUNBOOK.md#waldecodeerror - alert: FeedSilent diff --git a/monitoring/alerts_test.yml b/monitoring/alerts_test.yml index a0ffecfa..5fac748e 100644 --- a/monitoring/alerts_test.yml +++ b/monitoring/alerts_test.yml @@ -51,7 +51,7 @@ tests: instance: serve-1 exp_annotations: summary: "WAL frame failed to decode during live tailing on serve-1" - description: "A CRC-valid WAL frame could not be decoded by the codec and was skipped during live tailing — the serve read model has diverged from the WAL for that record. This should be near-impossible (a codec version mismatch or CRC collision). Investigate the WAL/codec version and reconcile." + description: "A CRC-valid WAL frame could not be decoded by the codec and was skipped during live tailing — the serve read model has diverged from the WAL for that record. This should be near-impossible (a codec version mismatch or CRC collision). Investigate the WAL/codec version; strict `reconcile` refuses this frame and is not a repair for this alert." runbook: monitoring/RUNBOOK.md#waldecodeerror - interval: 1m diff --git a/openspec/changes/archive/2026-03-07-quant-workstation-platform/design.md b/openspec/changes/archive/2026-03-07-quant-workstation-platform/design.md index e97299eb..6f6d9614 100644 --- a/openspec/changes/archive/2026-03-07-quant-workstation-platform/design.md +++ b/openspec/changes/archive/2026-03-07-quant-workstation-platform/design.md @@ -137,6 +137,17 @@ The target serving split for historical reads is: This allows the workstation to scale interactive serving without redefining replay truth around the serving backend itself. +Parquet access is object-store-native in both directions: deployed readers use +the same configured `object_store` backend as writers. Offline WAL recovery only +publishes fully covered UTC hours for datasets partitioned exactly by receive +time (book, trade, and ingest), and switches an individual partition through a +versioned manifest after its replacement object is durable. Independently +timestamped datasets remain untouched because WAL endpoints cannot prove them +complete. This avoids both the local-only S3 read gap and delete-before-write +recovery windows. Recovery validates the retained WAL without retaining decoded +records, then publishes one eligible hour at a time to bound maintenance-task +memory. + ## Product Surfaces ### Live Feed diff --git a/openspec/changes/archive/2026-03-07-quant-workstation-platform/specs/serving-runtime-platform/spec.md b/openspec/changes/archive/2026-03-07-quant-workstation-platform/specs/serving-runtime-platform/spec.md index 5b2cb5b5..8958ed00 100644 --- a/openspec/changes/archive/2026-03-07-quant-workstation-platform/specs/serving-runtime-platform/spec.md +++ b/openspec/changes/archive/2026-03-07-quant-workstation-platform/specs/serving-runtime-platform/spec.md @@ -88,3 +88,13 @@ When replay correctness, validation, or recovery questions arise Then Parquet remains available as the canonical audit and replay-truth source And the serving backend does not redefine historical truth by itself ``` + +### Scenario: Parquet recovery publishes only complete authoritative views + +``` +Given retained WAL may begin or end inside an hourly Parquet partition +When offline recovery rebuilds canonical Parquet data +Then only hours fully proven by a gap-free, strictly decoded WAL are published +And only datasets partitioned exactly by the proof's receive-time clock are replaced +And readers atomically switch through a recovery manifest after the replacement object is durable +``` diff --git a/openspec/changes/archive/2026-03-07-quant-workstation-platform/tasks.md b/openspec/changes/archive/2026-03-07-quant-workstation-platform/tasks.md index 347b4afd..c60ec1ea 100644 --- a/openspec/changes/archive/2026-03-07-quant-workstation-platform/tasks.md +++ b/openspec/changes/archive/2026-03-07-quant-workstation-platform/tasks.md @@ -119,6 +119,9 @@ Phase 5.3 (deployment packaging) ── independent, last serving-oriented backend such as ClickHouse (ClickHouse service implementations with Parquet fallback) - [x] Keep Parquet as the audit and replay-truth source for validation, reproducibility, and recovery (maintained — Parquet remains primary, ClickHouse is interactive overlay) +- [x] Make Parquet reads object-store-native and WAL recovery fail closed on + partial coverage/corruption or independently timestamped datasets, with + manifest-based crash-consistent publication - [x] Define readiness, resync, and backpressure behavior for stateless serving replicas (WAL gap detection, lag tracking, backpressure pruning, /api/v1/health endpoint) - [x] Document the clean-slate serving topology as a future replacement for the diff --git a/tests/integration/s3_minio_roundtrip.rs b/tests/integration/s3_minio_roundtrip.rs index 98e1c8d0..cbac3d29 100644 --- a/tests/integration/s3_minio_roundtrip.rs +++ b/tests/integration/s3_minio_roundtrip.rs @@ -5,9 +5,9 @@ //! resolved with `object_store::parse_url_opts(url, std::env::vars())` (the same //! call `pb_bin::commands::pipeline::build_object_store` makes), the production //! `ParquetSink` writes Parquet objects into the bucket, a *fresh* store handle -//! (simulating a process restart) still sees them, and the bytes round-trip back -//! to the original records. It also asserts no local `s3:`-named directory is -//! created. +//! (simulating a process restart) still sees them, and the production +//! `ParquetReader` reads the original records through object-store range reads. +//! It also asserts no local `s3:`-named directory is created. //! //! Run with a MinIO/LocalStack endpoint: //! PB_TEST_S3_ENDPOINT=http://127.0.0.1:9100 \ @@ -20,7 +20,8 @@ use std::sync::Arc; use std::time::Duration; use futures_util::StreamExt; -use object_store::{ObjectStore, ObjectStoreExt}; +use object_store::ObjectStore; +use pb_replay::{EventReader, ParquetReader}; use pb_store::ParquetSink; use pb_types::event::{ BookEvent, BookEventKind, DataSource, EventProvenance, PersistedRecord, Side, TradeEvent, @@ -78,7 +79,7 @@ fn build_s3_store(base_path: &str) -> (Arc, String) { } async fn list_parquet_objects(store: &Arc, prefix: &str) -> Vec { - let prefix_path = object_store::path::Path::from(prefix); + let prefix_path = object_store::path::Path::parse(prefix).unwrap(); let mut stream = store.list(Some(&prefix_path)); let mut out = Vec::new(); while let Some(meta) = stream.next().await { @@ -94,10 +95,8 @@ async fn list_parquet_objects(store: &Arc, prefix: &str) -> Vec #[tokio::test] #[ignore] async fn s3_base_path_persists_parquet_and_survives_restart() { - let Ok(endpoint) = std::env::var("PB_TEST_S3_ENDPOINT") else { - eprintln!("PB_TEST_S3_ENDPOINT not set; skipping S3 persistence test"); - return; - }; + let endpoint = std::env::var("PB_TEST_S3_ENDPOINT") + .expect("PB_TEST_S3_ENDPOINT is required for this explicitly ignored S3 integration test"); let bucket = std::env::var("PB_TEST_S3_BUCKET").unwrap_or_else(|_| "poly-book-test".to_string()); let access = @@ -167,33 +166,12 @@ async fn s3_base_path_persists_parquet_and_survives_restart() { "objects must persist across a fresh store handle (restart): before={written:?} after={after_restart:?}" ); - // --- Bytes round-trip: download a book_events object and parse it back --- - let book_obj = after_restart - .iter() - .find(|p| p.contains("book_events")) - .expect("a book_events object"); - let bytes = store2 - .get(&object_store::path::Path::from(book_obj.as_str())) - .await - .unwrap() - .bytes() + // --- Application round-trip: the production reader must read S3 directly --- + let reader = ParquetReader::from_store(store2, prefix2); + let window = reader + .read_market_data(&AssetId::new("token-s3"), base_ts - 1, base_ts + 2_000_000) .await .unwrap(); - assert_eq!( - &bytes[..4], - b"PAR1", - "downloaded object must be a Parquet file" - ); - - let builder = - parquet::arrow::arrow_reader::ParquetRecordBatchReaderBuilder::try_new(bytes).unwrap(); - let reader = builder.build().unwrap(); - let mut rows = 0usize; - for batch in reader { - rows += batch.unwrap().num_rows(); - } - assert_eq!( - rows, 2, - "the two book events must round-trip back out of S3-persisted Parquet" - ); + assert_eq!(window.book_events.len(), 2); + assert_eq!(window.trade_events.len(), 1); } From 1ae8c4d17632888ef79da503702dd30f4f62c5f3 Mon Sep 17 00:00:00 2001 From: Weiming Long Date: Wed, 22 Jul 2026 11:36:09 +0800 Subject: [PATCH 2/2] Fix Rust security audit CI --- .github/workflows/ci.yml | 24 +++++++++++++++--------- Cargo.lock | 4 ++-- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 356eb8cb..a3022b1f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,18 +124,24 @@ jobs: audit: name: Security Audit runs-on: ubuntu-latest - permissions: - checks: write - contents: read steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - - uses: rustsec/audit-check@69366f33c96575abad1ee0dba8212993eecbe998 # v2 + - uses: dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8 # stable with: - token: ${{ secrets.GITHUB_TOKEN }} - # quick-xml DoS advisories with no upgrade path yet (object_store - # pins quick-xml < 0.41). Rationale and removal condition are - # documented on the matching ignore entries in deny.toml. - ignore: RUSTSEC-2026-0194,RUSTSEC-2026-0195 + toolchain: stable + # Keep cargo-audit's own dependency graph locked. Installing without + # --locked can resolve transitive crates whose MSRV exceeds the stable + # compiler selected above, failing before the repository is audited. + - name: Install cargo-audit + run: cargo install cargo-audit --version 0.22.2 --locked + - name: Audit Rust dependencies + # quick-xml DoS advisories with no upgrade path yet (object_store pins + # quick-xml < 0.41). Rationale and removal condition are documented on + # the matching ignore entries in deny.toml. + run: >- + cargo audit + --ignore RUSTSEC-2026-0194 + --ignore RUSTSEC-2026-0195 monitoring: name: Monitoring Rules diff --git a/Cargo.lock b/Cargo.lock index 08093eaa..556dea8d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1054,9 +1054,9 @@ dependencies = [ [[package]] name = "crossbeam-epoch" -version = "0.9.18" +version = "0.9.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +checksum = "2d6914041f254d6e9176c01941b21115dcfb7089e55135a35411081bd106ef3f" dependencies = [ "crossbeam-utils", ]