test(wal): land rfc0008_2 crash-recovery via real SIGKILL harness - #126
Merged
Conversation
Flips the RFC0008.2 §5 acceptance test from `#[ignore]`'d `unimplemented!()` stubs to a live, real-process-death harness — the H3 / §3.4 WAL-before-ack crash-recovery milestone (CLAUDE.md §6.7's Pages gate). Because the crate is `#![deny(unsafe_code)]` (workspace lint), a `fork()` harness is out. Instead a small fixture binary (`wal_crash_fixture`, declared as a `[[bin]]` under tests/fixtures/, `publish = false`) opens the WAL, applies an append/sync op sequence, prints `READY`, and parks. The test spawns it, waits for `READY` (so every op incl. the final fsync has committed), then sends an uncatchable `SIGKILL` via `Child::kill()`. The fixture never drops its `Wal`, so the on-disk state is exactly a crash's — the test then reopens and replays. Three arms: a fsync'd frame survives a kill between sync and ack (the §3.4 critical case); a fsync'd frame survives with a later un-fsync'd frame handled safely (recovered whole or absent, never a corruption halt); and fsync'd `AuditEvent` frames survive alongside `OtlpBatch` frames in order (RFC 0005 §3.7). Stable across repeated runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 43 minutes and 9 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 (3)
✨ 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
4 tasks
jensholdgaard
added a commit
that referenced
this pull request
Jun 6, 2026
…ed → specified (#127) * docs(rfc-0003): specify §5 G/W/T scenarios + OTel-spec enrichments — drafted → specified Fills the §5 acceptance-criteria stub on RFC 0003 (OTLP receiver) with 15 Given/When/Then scenarios — the 11 originally sketched plus four enrichments surfaced by the OpenTelemetry transport-spec citations the maintainer pulled in (empty request → success, identity+gzip MUST, default /v1/logs path + override, concurrent Export calls). §5.2 (Crash-before-ack) lands as **at-least-once with retry tolerance**: the OTLP spec's *duplicate-data* section explicitly accepts client-retry duplicates as the right tradeoff for telemetry data, and the Collector's own WAL guidance carries the same caveat. This resolves §9 open question #1 (dedup) — the receiver implements no de-duplication in this RFC; any future dedup mechanism is additive. §5.13 (compression) resolves §9 open question #9 — identity + gzip are both required acceptance criteria; zstd/br stay out of scope (HTTP 415). §8 is filled to map each scenario id to its test technique (proptest for wire-decode equivalence, child-process SIGKILL harness mirroring PR #126 for crash-before-ack, table-driven for the transport-error and edge-OTLP arms, criterion for the latency/throughput benches). Frontmatter status flips drafted → specified per the maturity ladder (docs/rfcs/README.md §Lifecycle); §10 references unchanged (the OTLP spec link already covers the cited sub-sections). No code touched — per the split-doc-from-code-PRs convention, the §5 test stubs (red gate per docs/verification.md §3) ride a follow-up implementation PR. * docs(rfc-0003): address Copilot review feedback on §5 / §6.5 - §6.5: replace "dedup mechanism not yet specified" with the §9 #1 resolution (at-least-once with duplicates per OTLP spec; any future dedup is additive). Removes the §6.5↔§9 internal inconsistency. - RFC0003.2: broaden the kill-point description from "the §6.5 step 5/6 gap" to "anywhere in the step-4-through-6 window," since the duplicate-on-retry contract holds across both the 4/5 and 5/6 gaps once the records are durable. - RFC0003.4 / RFC0003.12: drop `wal_syncs_total` / `wal_unflushed_bytes` metric-name references from acceptance criteria; reframe in terms of observable WAL state (frame count, segment offsets, append/sync call counts via a test wrapper). Acceptance criteria should not pin telemetry identifiers. - §5: reference `docs/rfcs/README.md` *Required sections* by heading name, not the malformed `§Required-sections` token. The scenario-id grammar question (RFC0003.<m> vs §3.<n>.<m> for invariant-tagged scenarios) is left for the maintainer — see the PR reply on that thread. * docs(rfc-0003): align WAL framing with RFC 0008 (per-batch, not per-record) Copilot's second review pass caught a real contract bug: RFC 0003's §5 scenarios and §6.5 step sequence asserted per-record WAL appends, but RFC 0008 §4 / §7.3 specifies `FrameKind::OtlpBatch` — one frame per `ExportLogsServiceRequest`, carrying the verbatim protobuf bytes. RFC 0008 §7.3 explicitly rejects per-record framing as the alternative. Fixes: - §5.1, §5.2, §5.15: assertions now reference the `OtlpBatch` frame and its payload bytes round-tripping to the input request, not "every record in the WAL." The retry-duplicate assertion is now "a second `OtlpBatch` frame whose payload bytes equal the first." - §6.1 step 5, §6.5 preamble, §6.5 step 3: durability language switched from "every record durably written" / "appends every record" to "batch's `OtlpBatch` frame is durably written" / "appends the encoded request as a single `OtlpBatch` frame." - §8 testing-strategy crash-before-ack: asserts the same `OtlpBatch` payload bytes appear twice (two frames, one per export attempt). - §5.11: drop the `max_request_bytes` config-key reference; phrase as "configured request-size limit" since the knob isn't specified in this RFC and pinning a name in acceptance criteria is premature. The miner still consumes records per-record (§6.1 step 4–5, §9 open questions); only the WAL frame is per-batch. * docs(rfc-0003): close §6.5 ack-conditions and greppability gaps Copilot's third review pass: - §6.5: qualify the "ack only after `OtlpBatch` is durably written" contract as applying to non-empty batches; explicitly point at RFC0003.12 as the empty-batch exception (which returns success with no WAL write). - RFC0003.1, RFC0003.15: lock in the §6.5 step-5 miner-acceptance condition alongside `Wal::sync` return. Both scenarios now assert ack-after-(sync AND miner-accept), per-call for RFC0003.15. RFC0003.1 uses an instrumented `MinerCluster` stub; RFC0003.15's per-call probe records both orderings. - §5 intro, §8 testing-strategy: clarify the greppability convention — the verbatim id reference lives in each test's leading doc comment (`/// Scenario RFC0003.1 — ...`) per `docs/verification.md` §2.3, not in the Rust function name. The `#[test] fn rfc0003_1_*` example was misleading. * docs(rfc 0003): fix RFC 0008 cross-ref §4 -> §3.2 + §6.2.3 §4 of RFC 0008 is "Background — existing Rust durability ecosystem"; the FrameKind::OtlpBatch payload contract lives in §3.2 ("What goes into the WAL") with the frame/payload-encoding details in §6.2.2 / §6.2.3. * docs(rfc 0003): tighten singular-frame, §6.3 error wording, decouple miner internals Three Copilot nits: 1. RFC0003.1 said `Wal::sync` "covering the batch's frames" — singular per RFC 0008 §3.2 (one `OtlpBatch` frame per export batch). 2. §6.3 said the tenant-resolution error names "the failing Resource"; RFC0003.4 names "the failing `ResourceLogs` index". Aligned both on the index + attribute key. 3. RFC0003.8's last `And` reached into miner internals (RFC 0001 §6.2 step-0 short-circuit). The receiver's contract is the byte-for-byte pass-through; how the miner subsequently routes it is the miner's business. Asserted via an instrumented `MinerCluster` stub instead. * docs(rfc 0003): broaden RFC0003.12 to all three "empty" shapes "Empty" in OTLP can mean any of: empty resource_logs, empty scope_logs per ResourceLogs, or empty log_records per ScopeLogs. RFC0003.12 only covered the first shape; the fast-path contract ("zero records => success without WAL write") applies to all three. All three are now explicit in the Given and tested. * docs(rfc 0003): broaden unknown-fields rule, own the wire-0→None rule 1. RFC0003.6's unknown-fields clause restricted forward-compatibility to *top-level* fields; the OTLP/proto3 rule is "anywhere in the message". Reworded to "anywhere in the request body (top-level, nested, repeated)". 2. RFC0003.9 asserted observed_time_unix_nano=0 -> None but the rule was nowhere written down — RFC 0001 §6.1 just types the field as Option<u64> without defining the conversion. Made the contract explicit: the receiver owns the wire-0→None mapping, severity_number stays 0 because UNSPECIFIED is a valid OTLP value (not absence). * docs(rfc 0003): respect WAL single-writer contract in replay assertions RFC0003.1 / .4 / .15 each verified WAL state by opening a "fresh Wal::open" while the receiver was still alive. The Wal contract is "opened by exactly one writer" (ourios-wal/src/lib.rs §6.2 — O_APPEND single-writer handle), so a second open while the receiver's handle is live violates the contract and would race with replay's heal/truncate path. Spelled out in all three scenarios: shut the receiver down (which drops its Wal handle) before opening a *second* Wal and replaying. * docs(rfc 0003): use OtlpBatch frame terminology in WAL absence assertions Three spots described "no record appended to the WAL" / "no partial record appended" — but the WAL persistence unit is per-export FrameKind::OtlpBatch frames, not per-record entries. Reworded RFC0003.4, RFC0003.11, and §8's transport-errors testing bullet to assert "no OtlpBatch frame appended" so the spec is unambiguous about what the WAL stores. * docs(rfc 0003): retry frames must be semantically equal, not byte-equal RFC0003.2 and §8's crash-before-ack bullet both required the retry's OtlpBatch frame to be byte-identical to the first. That over-constrains clients: a retry can legitimately re-encode (JSON ordering, whitespace, switched encoding/compression) and remain semantically the same export. Both now assert that the second frame's payload decodes (via prost) to an ExportLogsServiceRequest semantically equivalent to the first. Also retitled RFC0003.13 to "Compression over HTTP: identity and gzip MUST be supported" — the scenario is HTTP-only (Content-Encoding), matching §9's wording. * docs(rfc 0003): tidy §8 — Body-fork grammar + drop PR-M2 jargon 1. §8's Body-fork bullet had awkward grammar ("variants assert `Body::from_any_value` routes…"). Reworded as "each asserting that …" so the subject of the assertion is unambiguous. 2. Dropped the "PR-M2" label from the two-loop red-gate note; the label is internal milestone shorthand that's not defined in this RFC. Now refers to the two-loop pattern itself (and to RFC 0008 §5 as the precedent) so the note is self-contained. * docs(rfc 0003): tighten payload-equality wording in RFC0003.1 + .2 RFC0003.1's last `And` claimed "payload bytes equal the encoded `ExportLogsServiceRequest`" but the named verification only checks a `prost` round-trip. Protobuf has multiple wire encodings for the same message, so the claim was strictly stronger than what the test asserts. Relax to "payload decodes (via `prost`) to the input request" and cross-link the RFC0003.2 explanation of why byte equality is the wrong shape of contract here. RFC0003.2's first `Then` said "decodes byte-for-byte" — internally inconsistent (decoding yields a message value, not bytes). Rephrase to "decodes (via `prost`) to an `ExportLogsServiceRequest` semantically equivalent to the killed process's input," matching the second `And` which already had the right shape after round 10. Both Copilot comments (PR #127, comments 3367201831, 3367201836). * docs(rfc 0003): disentangle ordering vs content check in RFC0003.1 Copilot caught that the last `And` clause bundled two distinct assertions — "frame exists with the right payload" and "before the ack fires" — but only the former is verified by the named technique (post-shutdown replay via `Wal::replay`). Replay establishes existence + content post-response; it can't establish pre-ack timing, because by the time the second `Wal` is opened the response has already shipped. The before-the-ack ordering is what the `AtomicBool` probe in the preceding `Then` + `And` clauses is for (set after `Wal::sync` returns; asserted `true` by the response-writer; asserted `false` at every pre-sync stage). That's the timing contract. The replay is the durability/content contract. Rewording makes the split explicit so a reader doesn't infer the replay proves the ordering. PR #127, comment 3367208364. * docs(rfc 0003): align §6.5 step 3 with RFC0003.1/.2 wire-encoding stance Copilot caught that §6.5 step 3 said the WAL payload is stored as "verbatim `ExportLogsServiceRequest` protobuf bytes" — strictly stronger than what RFC0003.1's acceptance criteria require (rounds 12+13 relaxed those to "decodes via `prost` to the input request"). The mismatch is most obvious on the HTTP/JSON path: there are no incoming protobuf bytes to be "verbatim" — the receiver must encode the decoded message to protobuf for WAL storage. Reword step 3 to require the payload be a protobuf-encoded `ExportLogsServiceRequest` decodable via `prost`, semantically equivalent to the input. The wire-bytes-verbatim option remains explicitly permitted (it is the obvious zero-copy choice for the protobuf paths), but it is no longer the *contract*. PR #127, comment 3367215593. * docs(rfc 0003): cite RFC 0008 §3.1 for single-writer (not source file §6.2) Copilot caught (three identical comments on lines 260, 340, 534) that `crates/ourios-wal/src/lib.rs` §6.2 is a confusing cross-reference: the file is Rust source, not a sectioned doc. The §6.2 the source-file comment at line 162 itself cites is RFC 0008 §6.2 (segment layout), which is where the append-only property lives — but the actual *single-writer architecture* statement RFC 0003 is leaning on is RFC 0008 §3.1 ("single-writer single-node component"). Rephrase all three sites to: per RFC 0008 §3.1's single-writer architecture (enforced by `crates/ourios-wal/src/lib.rs:162`) — RFC-first as the contract; source file kept as the concrete enforcement point. Source file gets a line-number cite, which is the right shape for a Rust file. PR #127, comments 3367230544, 3367230558, 3367230561.
jensholdgaard
added a commit
that referenced
this pull request
Jun 6, 2026
#137) The final OTLP-receiver scenario (RFC0003.2 crash-before-ack) — all 15 §5 acceptance criteria are now live. A real-process crash fixture (receiver_crash_fixture, a [[bin]] mirroring ourios-wal's 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, kills it (SIGKILL on Unix, the target) after fsync but before any transport ack, reopens the WAL + replays via the shared ingest_support helper, 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 (no loss); a client that never saw the ack retries, a duplicate the OTLP duplicate-data section accepts (at-least-once). Stable across repeated SIGKILL runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Flips the RFC0008.2 §5 acceptance test from
#[ignore]'dunimplemented!()stubs to a live, real-process-death harness. This is the H3 / §3.4 WAL-before-ack crash-recovery milestone — the gate CLAUDE.md §6.7 names as unblocking GitHub Pages.Builds on #123 (which landed the
sync/replaymechanism + in-process crash-simulation tests). That PR deliberately did not fliprfc0008_2, because its docstring mandates a realSIGKILL; this PR delivers exactly that.How (and why a subprocess)
The crate is
#, so afork()-based harness is out. Instead:tests/fixtures/wal_crash_fixture.rs— a tiny fixture binary (declared[[bin]],publish = false, kept undertests/to mark it a fixture). It opens the WAL, applies an op sequence (SYNCor<kind>:<hex>), printsREADY(flushed), thenparks.READY(which guarantees every op — including the finalfsync— has committed), then sends an uncatchableSIGKILLviaChild::kill(). The fixture never drops itsWal, so no graceful close runs and the on-disk state is exactly a crash's. The test then reopens the WAL andreplays.How it preserves the invariant (§3.4 / Hazard 3)
Three arms:
between_sync_and_ack— a frame is fsync'd, then the process dies before the ack. The frame MUST be present on restart (otherwise about-to-be-acked data is lost). The critical case.between_append_and_sync— a fsync'd frame plus a later appended-but-unsynced frame, then crash. The fsync'd frame MUST survive; the unsynced one is handled safely (recovered whole or absent — never a corruption halt, whichrecoverasserts).audit_event_frames_survive— fsync'dAuditEventframes survive alongsideOtlpBatchframes, in order (RFC 0005 §3.7 audit-durability contract).The test asserts no fsync'd frame is lost and un-fsync'd frames are handled safely — it does not assert "exactly the fsync'd frames," since a
SIGKILLleaves the kernel page cache intact and surplus unsynced frames may be readable on restart.Stability
The harness is deterministic (the parent kills only after the fixture signals
READY, i.e. post-commit). Verified stable across repeated local runs.Follow-ups (not this PR)
F_FULLFSYNC/ Windows §6.3 platform-durability primitives are tracked in WAL §6.3 platform durability: macOS F_FULLFSYNC knob + Windows primitives (deferred from #123) #125.pages.ymlworkflow staysworkflow_dispatch-only until then).Verification
cargo fmt --all --check✓cargo clippy --all-targets --all-features -- -D warnings✓ (incl. the fixture bin)cargo test --all-features✓ (workspace green;rfc0008_2now runs, no longer ignored)🤖 Generated with Claude Code