fix(buffers): drop over-limit disk_v2 records instead of crashing#25795
Open
graphcareful wants to merge 7 commits into
Open
fix(buffers): drop over-limit disk_v2 records instead of crashing#25795graphcareful wants to merge 7 commits into
graphcareful wants to merge 7 commits into
Conversation
github-actions
Bot
added
the
domain: core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66d8721cfd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca703b4a76
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A record whose encoded size exceeds the disk_v2 max record size made the writer return an unrecoverable error, which tore down the entire Vector topology. Handle the too-large class (RecordTooLarge / FailedToEncode) inside the writer: extract finalizers before encoding consumes the record, reject them on failure (terminal status so the source stops retrying a record that can never succeed), log at error, meter the drop via buffer_discarded_events_total/bytes_total (intentional=false), and return Ok(0). The buffer and every other record are untouched. Genuinely fatal errors (I/O, serialization, inconsistent state) still propagate. To enable the finalizer extraction, Finalizable is added as a supertrait of InMemoryBufferable (it belongs alongside AddBatchNotifier) with trivial impls for the handful of test types that didn't already have it.
Acking sources map BatchStatus::Rejected back to a nack or withheld checkpoint (Pulsar: nack_with_id; file source: only checkpoints Delivered). Using Rejected on an oversized record therefore turns the drop into a poison-pill retry loop instead of discarding it permanently. Drop the finalizers with their default EventStatus::Dropped, which propagates as BatchStatus::Delivered, so sources ack/checkpoint and move on.
- Resolve finalizers as Delivered (not Rejected) so acking sources ack/checkpoint rather than nacking a record that can never be written - Track pre-entry drops in a separate counter that does not feed into total_left, so buffer occupancy is not corrupted by records that never entered the buffer
… retry take_finalizers runs before archive_record because the encoder unconditionally consumes the record. If can_write_record is false the record is recovered and returned to the caller, but record_finalizers was dropped as Delivered, acking the upstream source before the record entered any buffer. Add merge_finalizers to Finalizable (no-op default) and implement it for Event/LogEvent/Metric/TraceEvent/EventArray/SourceSenderItem and the buffer test-message types. Call it before returning the recovered record so finalizers travel with the record through block/overflow.
…er-record The sanity-check in `archive_record` (pos <= 8 or buf.len() != pos) detects broken serializer state — a writer-level invariant violation — not an oversized or malformed record. Returning `FailedToSerialize` here caused `is_unwritable_record` to match it, silently dropping every affected record as Delivered and letting the writer continue with a broken serializer. All subsequent records hit the same state and are also silently dropped. Fix: return `InconsistentState` from that branch so the error propagates as fatal, logging "Disk buffer writer has encountered an unrecoverable error" and shutting down the topology rather than masking data loss. Also remove `FailedToSerialize` from `is_unwritable_record` entirely. Its remaining source is the OOM scratch-space path, which is a transient, system- wide condition — not a permanent per-record fault. Silently dropping records as Delivered under OOM is worse than crashing and restarting.
graphcareful
force-pushed
the
fix/disk-buffer-drop-oversized-records
branch
from
ca703b4 to
a848605
Compare
BufferSender previously incremented the sender-layer received usage before attempting the primary write. That made DropNewest full writes easy to balance with a dropped metric, but it also counted Overflow sends against the primary buffer even when the primary write returned Full and the item was forwarded to the overflow buffer instead. Move sender usage accounting to follow TryWriteOutcome. Written records are counted as accepted, DropNewest full records are counted as received and intentionally dropped, and writer-level Dropped outcomes are left unmetered here because the disk writer already records the pre-entry drop and ledger adjustment. This keeps occupancy balanced when overflow forwarding succeeds without reintroducing false sender-layer drop metrics, and adds coverage for the overflow-block path.
graphcareful
force-pushed
the
fix/disk-buffer-drop-oversized-records
branch
from
a848605 to
3d11b3f
Compare
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
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.
Summary
A record whose encoded size exceeds the
disk_v2buffer's maximum record size caused the writer to return an unrecoverable error, which tore down the entire Vector topology.This fix handles the too-large case inside the writer:
buffer_discarded_events_total/buffer_discarded_bytes_total(intentional=false)Ok(0)— the buffer and all other records are unaffectedGenuinely fatal errors (I/O, serialization, inconsistent state) still propagate.
To enable finalizer extraction before encoding,
Finalizableis added as a supertrait ofInMemoryBufferable(it belongs alongsideAddBatchNotifier), with trivial impls for the handful of test types that didn't already have it.Vector configuration
No configuration changes. The fix is internal to the
disk_v2buffer writer.How did you test this PR?
size_limits.rsassert the writer drops oversized records, rejects their finalizers, and incrementsbuffer_discarded_events_total/buffer_discarded_bytes_total, while the buffer continues accepting subsequent records.Change Type
Is this a breaking change?
Does this PR include user facing changes?
A changelog fragment is included:
changelog.d/disk_v2_drop_oversized_record_instead_of_crashing.fix.mdReferences