test(ingester): land rfc0003_2 crash-before-ack — RFC 0003 fully green - #137
Conversation
Final OTLP-receiver scenario (RFC0003.2). A real-SIGKILL fixture (receiver_crash_fixture, a [[bin]] mirroring wal_crash_fixture from #126): builds an IngestPipeline over a real Wal, ingests one batch (append + fsync), prints READY, parks. The test spawns it, waits for READY, SIGKILLs it (after fsync, before any transport ack), reopens the WAL + replays, and asserts the fsync'd OtlpBatch frame survives and recovers the input record's body. No dedup assertion — the fsync'd-but-not-acked batch survives, and a client that never saw the ack retries (a duplicate, accepted per the OTLP duplicate-data section). Stable across repeated SIGKILL runs. All 15 §5 acceptance criteria (RFC0003.1–.15) are now live; 0 ignored. The crate doc is updated to reflect the receiver is fully implemented (the remaining gap is a served-socket binary, §9 process-model). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 29 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR completes RFC 0003’s final acceptance test slice (RFC0003.2) by adding a real-process crash harness that SIGKILLs an ingesting receiver after WAL fsync but before any transport-level ack, then verifies the durable OtlpBatch frame survives WAL replay.
Changes:
- Turn
rfc0003_2_crash_before_ackfrom an ignored stub into a live SIGKILL-based crash-recovery test. - Add a dedicated
receiver_crash_fixture[[bin]]to act as the killable subprocess that performs a real ingest + fsync and then parks. - Update
ourios-ingestercrate docs to reflect RFC 0003 acceptance criteria are now all live.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/ourios-ingester/tests/rfc0003_2_crash_before_ack.rs | Implements the live crash-before-ack recovery assertion using a SIGKILL’d subprocess and WAL replay. |
| crates/ourios-ingester/tests/fixtures/receiver_crash_fixture.rs | Adds the subprocess fixture binary that ingests one batch to a real WAL, prints READY, then parks for SIGKILL. |
| crates/ourios-ingester/src/lib.rs | Updates crate-level docs to reflect RFC 0003 is fully implemented (tests live) and notes remaining §9 wiring. |
| crates/ourios-ingester/Cargo.toml | Registers receiver_crash_fixture as a non-product [[bin]] for test harness use. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use std::io::{BufRead, BufReader}; | ||
| use std::process::{Command, Stdio}; | ||
|
|
||
| use opentelemetry_proto::tonic::common::v1::any_value::Value; | ||
| use ourios_ingester::receiver::decode_protobuf; | ||
| use ourios_wal::{FrameKind, FrameSink, RecoveryError, Wal, WalConfig}; | ||
|
|
||
| fn wal_config(root: &std::path::Path) -> WalConfig { | ||
| WalConfig { | ||
| root: root.to_path_buf(), | ||
| batch_window_ms: 100, | ||
| segment_size_bytes: 128 * 1024 * 1024, | ||
| segment_age_secs: 600, | ||
| housekeeping_secs: 60, | ||
| macos_full_fsync: false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed — rfc0003_2 now does mod ingest_support; use ingest_support::replay_frames; and drops its duplicated wal_config + CollectingSink. One source for the WAL config (the fixture is a separate [[bin]] so it keeps its own literal, but the test side no longer duplicates the helper).
rfc0003_2 duplicated wal_config + a CollectingSink that tests/ingest_support already provides via replay_frames. Use the shared helper (one source for the WAL config, no drift). Surfaced in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| // Arrange: a real WAL root the fixture and this test both open (via | ||
| // the shared `ingest_support` helper, so the config can't drift). |
There was a problem hiding this comment.
Fixed in the latest commit — the Arrange comment now says only this test reads the WAL back via the shared helper, and the fixture (a separate bin) opens the same root with a matching WalConfig literal.
| assert_eq!( | ||
| line.trim(), | ||
| "READY", | ||
| "fixture signalled READY (got {line:?}) — it died before fsync", | ||
| ); |
There was a problem hiding this comment.
Fixed — the assert message now states what the check proves ("a different first line means it failed before reaching the post-fsync READY print") rather than implying a specific pre-fsync cause.
…Y assert) Review wording fixes: the Arrange comment no longer implies the fixture uses the shared helper (it has its own matching WalConfig literal), and the READY assertion message states what it proves (the fixture did not reach the post-fsync READY print) rather than over-specifying the cause. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| //! A real-process crash: `receiver_crash_fixture` ingests one batch | ||
| //! (append + fsync) over a real `Wal`, prints `READY`, and parks; this | ||
| //! test `SIGKILL`s it after `READY` — i.e. after the batch is durable but | ||
| //! before any transport ack would be sent — then reopens the WAL and | ||
| //! replays. The fsync'd `OtlpBatch` frame must survive and recover the |
| "fixture must signal READY (got {line:?}) — a different first line \ | ||
| means it failed before reaching the post-fsync READY print", | ||
| ); | ||
| child.kill().expect("SIGKILL fixture"); |
| //! Not a product binary — declared as a `[[bin]]` only so the test can | ||
| //! spawn it as a real OS process and `SIGKILL` it. The crate is | ||
| //! `#![deny(unsafe_code)]` (workspace lint), so a `fork()` harness is | ||
| //! out; a child driven by `Child::kill()` is the no-`unsafe` way to | ||
| //! exercise a genuine crash. Mirrors `ourios-wal`'s `wal_crash_fixture`. |
What
The final OTLP-receiver scenario (RFC0003.2 crash-before-ack) — with this, all 15 §5 acceptance criteria are live and RFC 0003 is fully implemented.
A real-SIGKILL fixture (
receiver_crash_fixture, a[[bin]]mirroringwal_crash_fixturefrom #126): builds anIngestPipelineover a realWal, ingests one batch (append + fsync), printsREADY, parks. The test spawns it, waits forREADY,SIGKILLs it (after fsync, before any transport ack), reopens the WAL + replays, and asserts the fsync'dOtlpBatchframe survives and recovers the input record's body.No dedup assertion — this is the no-loss half of at-least-once: a batch synced-but-not-acked survives the crash; a client that never saw the ack retries, producing a duplicate the OTLP duplicate-data section accepts. Stable across repeated SIGKILL runs.
Verification
cargo test -p ourios-ingester✓ — 0 ignored; all 15rfc0003_*files live. Workspace green.cargo fmt --all --check✓ ·cargo clippy --all-targets --all-features -- -D warnings✓After this merges
I'll advance RFC 0003 toward
green(or flag it as a maintainer process call per the maturity ladder), record the milestone in memory, and surface the §9 served-socket binary (wiring the HTTP/gRPC listeners intoourios-serverwithspawn_blockingingest) as the natural next step — the listeners are currently exercised in-process only.🤖 Generated with Claude Code