feat(ingester): rfc 0025 green c — permanent-error quarantine; RFC green - #386
Conversation
The record_quarantined audit event (kind 7, quarantine_* columns, additive per §3.7) plus the sink-side rule: a permanent BatchError bisects the buffer, quarantines the poison record(s) to the audit stream with the error text and partition key, counts them on the existing flush-error counter with error.type, and publishes the remainder — on both the buffered-flush and cadence-drain paths. Discharges RFC0025.4/.5; all five §5 scenarios now live; status red -> green. Closes #362. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ChangesRFC 0025 Quarantine Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 RFC 0025 by implementing sink-side quarantine for permanently-encodable record failures (preventing partition wedges), adding a new record_quarantined audit event that round-trips through the audit parquet format, and wiring telemetry (error.type) onto the existing flush-error counter. It also updates the RFC status to green and adds/updates tests to cover the final scenarios.
Changes:
- Add
AuditPayload::RecordQuarantined(kind 7 /record_quarantined) plus new audit parquet columns (quarantine_partition,quarantine_error) with backward-compatible absent-column handling. - Implement quarantine behavior in both flush paths (inline
flush_partitionand cadencepublish_owned) and attacherror.typetoourios.sink.flush.errorsfor quarantined rejections. - Add tests for audit round-trip and the RFC0025.4/.5 quarantine + telemetry scenarios; update server wiring to pass the shared audit sink into the record sink.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/rfcs/0025-absent-body-representation.md | Marks RFC 0025 green and aligns §3.3 prose with implementation details (no WAL position in event). |
| crates/ourios-server/src/receiver.rs | Wires the shared audit sink into the record sink for quarantine events. |
| crates/ourios-parquet/tests/audit_round_trip.rs | Adds a round-trip test for the new record_quarantined audit payload. |
| crates/ourios-parquet/src/record_batch.rs | Adds BatchError::error_type() for stable error.type attribute values. |
| crates/ourios-parquet/src/lib.rs | Extends the audit schema with quarantine_* columns (additive). |
| crates/ourios-parquet/src/audit_record_batch.rs | Writes RecordQuarantined rows with quarantine columns populated and other payload groups NULL. |
| crates/ourios-parquet/src/audit_reader.rs | Reads quarantine columns with absent-column tolerance; decodes kind 7 into AuditPayload::RecordQuarantined. |
| crates/ourios-ingester/tests/rfc0025_quarantine.rs | Implements RFC0025.4 and RFC0025.5 tests (quarantine behavior + error.type telemetry). |
| crates/ourios-ingester/tests/perf_metrics.rs | Updates for the new record_flush_error(Option<...>) signature. |
| crates/ourios-ingester/src/record_sink.rs | Implements buffer bisection/quarantine on WriterError::Batch in both flush paths. |
| crates/ourios-ingester/src/metrics.rs | Adds optional error.type attribute support on the existing flush-error counter. |
| crates/ourios-core/src/audit.rs | Adds new audit payload + kind/type constants and mapping for record_quarantined. |
| crates/ourios-core/src/alias.rs | Treats RecordQuarantined as a non-alias event kind (ignored in alias map fold). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The schema-pin gate fired exactly as designed; RFC 0025 §3.3 is the amendment that authorizes the additive column pair. Co-Authored-By: Claude Fable 5 <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: 1
🧹 Nitpick comments (4)
crates/ourios-parquet/src/audit_record_batch.rs (2)
366-387: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment / inconsistent null-call placement for
Compactionarm.The comment at lines 373-377 documents that compaction events null "every template-specific and alias column" but omits that quarantine columns are also nulled. Additionally,
append_quarantine_nulls()(line 386) is called after the compaction values are written, unlike every other arm where all*_nulls()calls are grouped together up front. Functionally correct (append order across independent builders doesn't matter), but the drift makes it easy for a future contributor extending this match to miss nulling a new group.♻️ Suggested cleanup
// Compaction events leave every template-specific // and alias column NULL (§3.7 relaxed the former to // OPTIONAL; the latter are alias-kind-only), and the // facts live in the `compaction_*` columns — `reason` - // stays NULL. + // stays NULL. Quarantine columns are likewise NULL. self.append_template_nulls(); self.append_alias_nulls(); + self.append_quarantine_nulls(); self.reason.append_null(); self.compaction_partition.append_value(partition); append_string_list(&mut self.compaction_input_files, input_files); self.compaction_output_file.append_value(output_file); self.compaction_generation.append_value(*generation); self.compaction_rows.append_value(*rows); - self.append_quarantine_nulls(); }🤖 Prompt for 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. In `@crates/ourios-parquet/src/audit_record_batch.rs` around lines 366 - 387, In the AuditPayload::Compaction match arm, the inline comment is stale and the null-appends are split inconsistently. Update the comment to mention quarantine columns are also nulled, and move append_quarantine_nulls() into the grouped *_nulls() calls at the top with append_template_nulls(), append_alias_nulls(), and reason.append_null() so the nulling pattern matches the other arms in audit_record_batch.rs.
213-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuarantine fields interleaved into compaction group.
quarantine_partition/quarantine_errorare declared and initialized betweencompaction_partitionand the rest of thecompaction_*fields, splitting up the compaction group. Field order doesn't affect correctness (struct-literal init is by name), but grouping them with the other quarantine-adjacent code (e.g., afteralias_actor, matchingaudit_schema()'s column order) would keep the mental model consistent with the rest of the file.Also applies to: 270-271
🤖 Prompt for 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. In `@crates/ourios-parquet/src/audit_record_batch.rs` around lines 213 - 214, The `AuditRecordBatch` field initialization in `audit_record_batch.rs` has `quarantine_partition` and `quarantine_error` interleaved into the `compaction_*` block, which breaks the logical grouping. Reorder the struct fields and their initialization in `AuditRecordBatch::new` (and any matching setup around `audit_schema()`) so the quarantine fields sit with the other quarantine-adjacent fields, ideally after `alias_actor`, while keeping the `compaction_*` fields together.crates/ourios-ingester/src/record_sink.rs (2)
374-398: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a structured log alongside the metric/audit event for quarantined records.
Right now a quarantine is only visible via the OTel counter and the buffered audit-Parquet event (not queryable until that sink flushes). A
warn-level structured log at quarantine time would give operators immediate visibility into a permanently-dropped record without waiting on the audit sink.As per path instructions,
**/crates/ourios-{ingester,querier,server}/**/*.rs: "Use Prometheus metrics for every subsystem, emit structured logs via Ourios on hot paths, and trace every RPC."🤖 Prompt for 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. In `@crates/ourios-ingester/src/record_sink.rs` around lines 374 - 398, Add a warn-level structured log when records are quarantined in quarantine_owned so operators get immediate visibility before the audit sink flushes. Use the existing quarantine flow in record_sink.rs around split_poisoned, the per-record loop, and AuditEvent emission to log key context such as the partition, tenant_id, and the error from the poisoned record. Keep the Prometheus metric and audit payload behavior unchanged, and place the log alongside the existing record_flush_error call so all quarantine signals stay aligned.Source: Path instructions
145-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct unit tests for
split_poisoned's bisection cases. The current coverage only exercises one poisoned record throughrfc0025_quarantine.rs; add a small unit test module here for multiple poisoned records, non-contiguous poison, and the all-poisoned buffer edge case.🤖 Prompt for 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. In `@crates/ourios-ingester/src/record_sink.rs` around lines 145 - 174, Add direct unit tests for split_poisoned in record_sink.rs to cover the bisection logic itself, since current coverage only hits a single quarantined record elsewhere. Create a small test module that exercises split_poisoned with multiple poisoned records, non-contiguous poison in the input, and the all-poisoned buffer edge case, and assert the returned kept versus poisoned partitions are correct. Use split_poisoned and mined_records_to_batch_with_promoted as the key symbols when locating the behavior under test.Source: Coding guidelines
🤖 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/record_sink.rs`:
- Around line 315-332: Full-quarantine handling in the `WriterError::Batch`
branch of `flush_partition` leaves an empty `PartitionBuffer` in `self.buffers`
and skips the normal cleanup/accounting path. When `quarantine_poisoned(key)`
empties the buffer, make sure the code still removes the entry and reconciles
`self.total_bytes`/`buffer_usage` instead of returning early. Update the
early-return path around `self.buffers.get(key)` so it either performs the same
cleanup as the success path or funnels through the existing
`self.buffers.remove(key)` logic, and ensure `oldest` is cleared/updated so
`flush_aged` does not keep reselecting the partition.
---
Nitpick comments:
In `@crates/ourios-ingester/src/record_sink.rs`:
- Around line 374-398: Add a warn-level structured log when records are
quarantined in quarantine_owned so operators get immediate visibility before the
audit sink flushes. Use the existing quarantine flow in record_sink.rs around
split_poisoned, the per-record loop, and AuditEvent emission to log key context
such as the partition, tenant_id, and the error from the poisoned record. Keep
the Prometheus metric and audit payload behavior unchanged, and place the log
alongside the existing record_flush_error call so all quarantine signals stay
aligned.
- Around line 145-174: Add direct unit tests for split_poisoned in
record_sink.rs to cover the bisection logic itself, since current coverage only
hits a single quarantined record elsewhere. Create a small test module that
exercises split_poisoned with multiple poisoned records, non-contiguous poison
in the input, and the all-poisoned buffer edge case, and assert the returned
kept versus poisoned partitions are correct. Use split_poisoned and
mined_records_to_batch_with_promoted as the key symbols when locating the
behavior under test.
In `@crates/ourios-parquet/src/audit_record_batch.rs`:
- Around line 366-387: In the AuditPayload::Compaction match arm, the inline
comment is stale and the null-appends are split inconsistently. Update the
comment to mention quarantine columns are also nulled, and move
append_quarantine_nulls() into the grouped *_nulls() calls at the top with
append_template_nulls(), append_alias_nulls(), and reason.append_null() so the
nulling pattern matches the other arms in audit_record_batch.rs.
- Around line 213-214: The `AuditRecordBatch` field initialization in
`audit_record_batch.rs` has `quarantine_partition` and `quarantine_error`
interleaved into the `compaction_*` block, which breaks the logical grouping.
Reorder the struct fields and their initialization in `AuditRecordBatch::new`
(and any matching setup around `audit_schema()`) so the quarantine fields sit
with the other quarantine-adjacent fields, ideally after `alias_actor`, while
keeping the `compaction_*` fields together.
🪄 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: 4113d0aa-15d4-45eb-b6b0-ad98d32ca230
📒 Files selected for processing (13)
crates/ourios-core/src/alias.rscrates/ourios-core/src/audit.rscrates/ourios-ingester/src/metrics.rscrates/ourios-ingester/src/record_sink.rscrates/ourios-ingester/tests/perf_metrics.rscrates/ourios-ingester/tests/rfc0025_quarantine.rscrates/ourios-parquet/src/audit_reader.rscrates/ourios-parquet/src/audit_record_batch.rscrates/ourios-parquet/src/lib.rscrates/ourios-parquet/src/record_batch.rscrates/ourios-parquet/tests/audit_round_trip.rscrates/ourios-server/src/receiver.rsdocs/rfcs/0025-absent-body-representation.md
BatchError::Arrow is an internal invariant violation, not a per-record rejection — it retains the buffer (bisect keeps the record too); an all-quarantined partition drops its emptied entry and releases byte accounting so the ceiling logic and buffer gauge can't drift. Both pinned by tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Partial quarantine now subtracts the dropped records' estimate (the emit-side estimator) from the buffer entry, total_bytes, and the buffer gauge at quarantine time — no transient overcount window for the ceiling triggers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Final RFC 0025 slice, discharging RFC0025.4 and RFC0025.5 — and with them all five §5 scenarios: status red → green. Closes #362.
What
record_quarantinedaudit event — kind 7 /quarantine_partition+quarantine_errorcolumns, additive per the §3.7 discipline (old readers surface it via theUnknowntolerance path; old files read unchanged — absent-column tolerance verified by the existing accessors). Round-trip pinned alongside template + compaction kinds in one file.BatchErrorduring flush bisects the buffer (O(k·log n) probes via the batch conversion — deterministic, so a failing subset always shrinks to its poison), emits one audit event per poisoned record (tenant, partition key, error text — the WAL retains the record itself), counts each on the existing flush-error counter witherror.type(no new metric name), and publishes the remainder. The cadence-drainpublish_ownedpath applies the same rule — it would otherwise requeue poison forever, the same Absent-body OTLP records permanently wedge their partition buffer (UnsupportedAbsentBody) #362 mechanism through the second door. Transient store errors keep today's retain-and-retry.BatchError::error_type()provides the stable snake-case attribute values.Scenario coverage
record_quarantinedevent naming the partition and the permanence, and the partition flushes normally afterward.error.type = timestamp_overflowonourios.sink.flush.errors; no new instrument.Invariants
§3.4 (WAL-before-ack) untouched — quarantine happens post-ack on the sink side, and the WAL remains the durability of record; the audit event is the operator's replay pointer. §3.3 prose in the RFC was aligned with the implementation (the event carries the partition key + error; a WAL-position pointer isn't plumbed to the sink — noted in the doc).
Verification
fmt clean; clippy clean across core/parquet/ingester/server (RC checked); quarantine scenarios 2/2; audit round-trips 8/8; miner/parquet/querier suites green from the prior slices.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
record_quarantinedincluding partition and error fields.Bug Fixes
Tests