feat: rfc0018.3 green — transient ingest failures map to retryable codes - #270
Conversation
The receiver mapped every non-tenant failure (WAL append/fsync) to gRPC INTERNAL / HTTP 500 — both NON-retryable per the OTLP failures table, so a compliant client drops a batch it should retry (RFC 0018 §3.2). A WAL failure is transient (the batch wasn't acked, §3.4); report it retryable: - gRPC: WAL failure → UNAVAILABLE (was INTERNAL); tenant failure stays INVALID_ARGUMENT; a panicked ingest task stays INTERNAL. - HTTP: WAL failure → 503 (was 500); tenant failure stays 400; panic → 500. Greens RFC0018.3 in tests/rfc0018_retryable.rs (both transports: transient → UNAVAILABLE/503, permanent → INVALID_ARGUMENT/400), driven by a new FailingSyncJournal in ingest_support (append ok, fsync fails → WalSync). Removed the .3 stub from rfc0018_otlp_compliance.rs. RFC 0018: 5 of 6 §5 scenarios green (.1/.2/.3/.4/.6). Remaining: .5 (non-finite doubles). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 43 minutes and 54 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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)
📝 WalkthroughWalkthroughThe gRPC and HTTP OTLP receiver handlers are updated to pattern-match ChangesRFC0018.3 Retryable Error Mapping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR completes RFC0018.3 by ensuring transient ingest failures (WAL append/sync) are reported as retryable statuses at the transport layer, preventing compliant OTLP clients from incorrectly dropping batches on server-side durability failures.
Changes:
- Map WAL append/sync failures to gRPC
UNAVAILABLE(instead ofINTERNAL) in the OTLP/gRPC receiver. - Map WAL append/sync failures to HTTP
503 Service Unavailable(instead of500) in the OTLP/HTTP receiver, while keeping tenant-resolution failures as client errors. - Add RFC0018.3 integration tests validating transient vs. permanent failure mappings for both transports, backed by a new failing-fsync test journal.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-ingester/src/receiver/grpc.rs | Change WAL failure mapping to retryable gRPC UNAVAILABLE. |
| crates/ourios-ingester/src/receiver/http.rs | Change WAL failure mapping to retryable HTTP 503; keep tenant failures as 400. |
| crates/ourios-ingester/tests/ingest_support/mod.rs | Add FailingSyncJournal + helper pipeline to inject WAL sync failures in tests. |
| crates/ourios-ingester/tests/rfc0018_retryable.rs | New RFC0018.3 tests asserting retryable vs. permanent failure status mappings (gRPC + HTTP). |
| crates/ourios-ingester/tests/rfc0018_otlp_compliance.rs | Remove the old ignored RFC0018.3 stub and update test-suite documentation pointers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot: the top-of-file doc still claimed WAL failure → INTERNAL; the implementation now maps it to UNAVAILABLE (transient, retryable; RFC 0018 §3.2). Update the doc to match, and note the panicked-task → INTERNAL arm. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot: ReceiveError is #[non_exhaustive], so Ok(Err(_)) => retryable would mark a future non-tenant variant retryable. Match WalAppend/WalSync explicitly instead. Within ourios-ingester the arms are exhaustive over ReceiveError (TenantResolution + WalAppend|WalSync), so a catch-all fallback is unreachable — and a future variant breaks the build here, forcing an explicit retryable-vs-not decision (stronger than a runtime fallback). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot: ReceiveError::WalAppend can be AppendError::TooLarge (payload > 16 MiB MAX_FRAME_BYTES) — a client sizing error, not a transient WAL outage. Mapping it to UNAVAILABLE/503 would make compliant clients retry the same oversized batch forever. Carve TooLarge out to non-retryable INVALID_ARGUMENT (gRPC) / 413 (HTTP); other WalAppend/WalSync failures stay retryable. Add end-to-end coverage for both the transient WalAppend path and the TooLarge permanent path (FailingAppendJournal). RFC §3.2 permanent-failure bullet updated to name the oversize carve-out. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-ingester/src/receiver/http.rs`:
- Around line 120-135: The status code mapping logic in the handle_logs function
is non-trivial protocol mapping that currently lacks unit test coverage, relying
only on integration tests. Add a cfg(test) module adjacent to the handle_logs
function that includes unit tests covering the different error scenarios handled
in the match statement: the WalAppend TooLarge case returning PAYLOAD_TOO_LARGE
(413), and the WalAppend and WalSync cases returning SERVICE_UNAVAILABLE (503).
This keeps regression testing local per coding guidelines.
In `@docs/rfcs/0018-otlp-log-spec-compliance.md`:
- Around line 146-151: The RFC mapping text currently groups all permanent
failures under HTTP 413, but the implemented behavior actually maps different
permanent error classes to different HTTP codes. Separate the permanent error
classes explicitly in the text: tenant-resolution failures and malformed
payloads should map to HTTP 400, while oversize payload errors
(AppendError::TooLarge) should map to HTTP 413. Update the bullet point to
clearly distinguish these two classes of permanent errors and their respective
HTTP status codes to align with the current implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f1c970a0-9c8c-4a33-ac62-f6f8aaf68bda
📒 Files selected for processing (5)
crates/ourios-ingester/src/receiver/grpc.rscrates/ourios-ingester/src/receiver/http.rscrates/ourios-ingester/tests/ingest_support/mod.rscrates/ourios-ingester/tests/rfc0018_retryable.rsdocs/rfcs/0018-otlp-log-spec-compliance.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/ourios-ingester/src/receiver/grpc.rs
CodeRabbit/Copilot review: - Extract the ingest-error → status mapping into pure fns (ingest_error_status) in grpc.rs and http.rs, with adjacent #[cfg(test)] unit tests covering all variants (tenant→400/INVALID_ARGUMENT, oversize TooLarge→413/INVALID_ARGUMENT, append-Io/quiesce/sync→503/UNAVAILABLE). Satisfies the crate rule that non-trivial logic carries local unit tests, keeping regressions local alongside the end-to-end coverage. - Fix RFC 0018 §3.2 permanent-failure wording: it read as if all permanent failures map to 413; split it into 400 (tenant/malformed) vs 413 (oversize) to match the implementation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RFC 0018 green (5/6) — transient ingest failures are retryable (RFC0018.3)
The receiver mapped every non-tenant failure (WAL append/fsync) to gRPC
INTERNAL/ HTTP500— both non-retryable per the OTLP failures table, so a compliant client drops a batch it should retry. A WAL failure is transient (the batch wasn't acked, §3.4); report it retryable (RFC 0018 §3.2):UNAVAILABLE(wasINTERNAL); tenant failure staysINVALID_ARGUMENT; a panicked ingest task staysINTERNAL.503(was500); tenant failure stays400; panic →500.Verification (RFC0018.3, both transports)
tests/rfc0018_retryable.rs: transient (fsync fails) →UNAVAILABLE/503; permanent (unresolvable tenant) →INVALID_ARGUMENT/400. Driven by a newFailingSyncJournaliningest_support(append ok, fsync →WalSync).clippy --all-targets,fmtclean.RFC 0018: 5 of 6 §5 scenarios green (.1/.2/.3/.4/.6). Remaining: .5 (non-finite doubles — the custom-serde one).
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
UNAVAILABLE, HTTP503) instead of treating them as internal errors.INVALID_ARGUMENT, HTTP413), while tenant-resolution issues remain permanent client errors.Tests
Documentation