Conversation
…uption Prost enforces a recursion limit of 100 during protobuf decoding. In Vector's proto schema, each level of map nesting costs ~3 prost recursion levels, causing decode failures at Value tree depth 34. Events that encode successfully but fail to decode corrupt the disk buffer irreversibly — subsequent reads and even startup validation fail with InvalidProtobufPayload, requiring manual buffer deletion. Add a MAX_NESTING_DEPTH (33) check at three protobuf encoding boundaries: - EventArray::encode (disk buffer writes) - NativeSerializer::encode (native codec over TCP/file) - VectorSink stream filter (gRPC vector-to-vector path) Events exceeding the limit are rejected before encoding. The vector sink emits ComponentEventsDropped (intentional) to the standard metrics pipeline so operators can monitor rejected events via component_discarded_events_total.
The protobuf encoding path encodes both the event value and the event metadata value into the same message. A deeply nested metadata value would bypass the nesting check and cause the same corruption. Check both values in event_exceeds_max_nesting_depth and check_event_array_nesting_depth.
Traced through prost 0.12's decode path and Vector's generated proto code to derive the exact formula: 4 outer recursion levels (EventArray → LogArray → Log → fields map) + 3 per Value map nesting level (Value message → ValueMap message → map entry). 4 + 3*32 = 100 = RECURSION_LIMIT exactly, confirmed by empirical test probing depths 28-38.
Empirical testing revealed that different proto encoding paths have different prost recursion overhead: - Log Object root (Log.fields path): 4 outer levels → fails at depth 33 - Log non-Object root (Log.value path): 5 outer levels → fails at depth 32 - Metadata value: fails at depth 33 - Trace events: fails at depth 33 With MAX_NESTING_DEPTH=33, the non-Object root path and metadata path had gaps where our check passed but prost decode failed. Lowering to 32 closes all gaps — verified by probing all four paths across depths 28-36 and confirming zero cases where our check passes but prost fails.
Verify that the EventWrapper decode path (used by the vector sink's gRPC receiver) is safe at MAX_NESTING_DEPTH. EventWrapper has fewer outer proto wrappers than EventArray, so our limit is conservative for this path.
Remove the approximate formula (5 + 3*N <= 100) and hedged outer overhead range (3-5) that were not fully verified. The comment now states only what we know for certain: the value was determined empirically and unit tests verify the boundary.
…oryy/vector into connor/protobuf-nesting-depth-limit
The metadata_full encoding path (EventArray → *Array → Event → Metadata → Value) is the tightest proto path, using exactly 100 of prost's 100 recursion budget at MAX_NESTING_DEPTH=32. This was the only path without a roundtrip success test — only gate rejection and decode failure tests existed for metadata. Add explicit prost encode/decode roundtrip tests for both log and metric metadata at depth 32 to prove the tightest path works.
Consolidate the scattered nesting depth tests into a data-driven framework that exhaustively covers every proto encoding path where a Value can appear: - Log value (via Log.fields), Log metadata (via metadata_full) - Trace value (via Trace.fields), Trace metadata (via metadata_full) - Metric metadata (via metadata_full) - Both EventArray and EventWrapper top-level wrappers The key test (`max_nesting_depth_is_correct_for_all_proto_paths`) verifies MAX_NESTING_DEPTH is exactly right: 1. ALL paths must roundtrip at depth 32 (limit isn't too high) 2. At least one path must fail decode at depth 33 (limit isn't too low) Adding a new proto wrapper message or Value-carrying field requires adding it to `all_proto_paths()`, and the test immediately surfaces if the limit needs adjustment.
…ing paths Instead of maintaining a list of individual proto paths, create events with ALL Value-carrying fields at max depth simultaneously. The proto conversion code populates every field (including deprecated ones like Log.metadata), so a single roundtrip per event type covers every proto path automatically. If a new Value-carrying field is added to event.proto, the conversion code must populate it, and these tests cover it with zero maintenance. No path enumeration to keep in sync.
…oryy/vector into spawn-src-94650
Add two isolated tests: - log_fields_has_headroom_beyond_max_depth: proves Log.fields can handle depth 33 (the loosest path has 3 entries of headroom) - metadata_full_fails_beyond_max_depth: proves metadata_full fails at depth 33 (the tightest path, exactly 100/100 at depth 32) Together these demonstrate that MAX_NESTING_DEPTH=32 is constrained specifically by the metadata_full encoding path.
Replace the two one-sided tests with a single per_path_boundaries test that verifies both sides of both paths: - Log.fields (loosest): succeeds at depth 33, fails at depth 34 - metadata_full (tightest): succeeds at depth 32, fails at depth 33 This proves the exact prost budget for each path and that the uniform MAX_NESTING_DEPTH=32 is set by metadata_full specifically.
The metadata_full encoding path has one more proto wrapper message than the event data path (Log.fields/Trace.fields), so it hits prost's recursion limit at a lower nesting depth. Instead of penalizing event data with the metadata path's stricter limit, use separate constants: - MAX_NESTING_DEPTH (33): for event data values (Log.fields, Trace.fields) - MAX_METADATA_NESTING_DEPTH (32): for metadata values (via metadata_full) Both limits are verified by per_path_boundaries: each constant succeeds at its value and fails at +1 via raw prost encode/decode.
![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: 35981225af
ℹ️ 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".
…he sink WriterSink::run propagated encoder errors fatally via `?`. A single data-dependent encoder failure -- e.g., one event exceeding the native codec's protobuf nesting budget added in this branch -- would terminate the whole sink, silently dropping every later valid event in the stream. Treat encoder failures as per-event: mark the offending event's finalizers as Errored (source-side acks see the drop), then continue to the next event. The Encoder framework already logs the error, counts a component error, and emits ComponentEventsDropped via EncoderSerializeError, so the only sink-level work needed is finalization + continuation. Adds per_event_encoder_failure_does_not_terminate_sink: streams a too-deep log event followed by a valid one through the native-codec console sink, asserts the bad event is finalized Errored and the next event is still Delivered. Stashing only the encoder-call change confirms the test catches the regression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 860f40a3f8
ℹ️ 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".
In WhenFull::Overflow topologies where a disk-v2 stage cascades to an in-memory overflow stage, an over-budget EventArray arriving at the full disk would have its sub-items dropped here even though the overflow stage has no wire-format constraint and could accept the item intact. Short-circuit in SenderAdapter::try_send: with the writer lock held, check is_buffer_full() before filtering. When the buffer is already at its size limit, return the item unfiltered as rejected so BufferSender can forward it to the overflow stage. Holding the writer lock makes the check race-free against other writers (only writers grow the buffer; readers only shrink). Exposes BufferWriter::is_buffer_full as pub(crate) for the new caller. # Coverage This addresses the steady-state-full case. The narrower "buffer has slack but not enough for this specific record" path (can_write_record returning false from inside try_write_record) still filters before the rejection -- detecting that case requires knowing the encoded size pre-encode, which is not cheaply available. # Multi-stage topologies The behavioural shift extends to chains: in disk-v2(Overflow) -> disk-v2(Overflow) -> disk-v2(Block), an over-budget item now flows unfiltered through the full Overflow stages and is finally filtered at the Block stage that actually persists it (with ComponentEventsDropped emitted there). Each intermediate stage accounts for it as dropped_intentional (fullness drop), so buffer_size gauges stay accurate. The one configuration to call out: when the chain terminates at an in-memory overflow (e.g., disk(Overflow) -> in-memory(Block)), the over-budget events flow all the way to memory unfiltered and are never dropped by any buffer stage. Whatever downstream codec consumes them hits the wire-format limit and is responsible for the drop (e.g., the console sink's non-fatal encoder handling). This matches the codex premise that the in-memory overflow stage has no wire-format constraint and should receive items intact. # Counterintuitive consequence For topologies that end in an in-memory buffer, this fix introduces an unintuitive delivery curve for over-budget events: such events are delivered *only* when the upstream disk stages are full. When the disk stages have capacity, an over-budget event arrives, gets filtered at the disk stage, and the over-budget sub-items are dropped via ComponentEventsDropped. When the disk stages are full, the same event flows unfiltered through to the in-memory tail and is delivered downstream intact. Disk fullness therefore raises the effective delivery rate of over-budget events for this class of topology -- the opposite of the intuition that disk backpressure should reduce throughput. The codex feedback accepted this tradeoff in exchange for not dropping events the overflow stage could accept; it is documented here so operators of disk-then-memory topologies understand why drop telemetry for over-budget events may correlate inversely with disk utilization. No regression test is added because reliably driving the disk-v2 writer's is_buffer_full() to true under the minimum-size test config takes careful tuning of record/buffer sizes (can_write_record tends to short-circuit writes before total_buffer_size reaches max_buffer_size). The diff itself is a single is_buffer_full() short-circuit before the existing filter path; existing disk-v2 writer-level tests cover the full-buffer behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28b2a35e2
ℹ️ 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".
The native protobuf encoder returned Err for events exceeding the wire format's recursion budget. Inside the per-event console sink that was fixed up by handling the Err non-fatally, but the *batched* path in src/sinks/util/encoding.rs propagates the same error fatally via `?`: one over-budget event in a request would surface as InvalidData, cause the HTTP sink (and any other batched sink built on this util) to drop the entire request, and finalize every other valid event in the same batch as dropped/Errored. Codex flagged this for the HTTP sink + native codec configuration. Move the drop semantics into the encoder itself: - `NativeSerializer::encode` now detects over-budget events, sets `EventStatus::Rejected` on the event metadata, emits `ComponentEventsDropped`, and returns Ok(()) without writing any bytes. Batched and per-event callers alike continue past the drop unchanged. - `Encoder<Framer>::encode` (lib/codecs/src/encoding/encoder.rs) skips framing when the inner serializer produced no payload, so an empty buffer is a clean per-event drop signal rather than a stray delimiter. - `WriterSink::run` (console sink) checks `bytes.is_empty()` after a successful encode and finalizes the event as `Rejected` (encoder dropped it) before continuing. Combined with the prior fix, no data-dependent encoder outcome can terminate the sink. Tests updated: - The four native-codec reject tests in lib/codecs/tests/native.rs now assert Ok(()) + buffer.is_empty() instead of Err. - The console sink's per_event_encoder_failure_does_not_terminate_sink asserts BatchStatus::Rejected for the dropped event (was Errored). - New test_encode_batch_native_drops_over_budget_event_without_aborting in src/sinks/util/encoding.rs pins the batched-encoder behaviour: stashing the encoder change confirms the test catches the regression (`InvalidData: SerializingError ...`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pront
added
the
meta: awaiting author
…budget
Codex flagged a concern that vector-sink traffic might need a tighter
nesting budget than the disk-buffer / native-codec path because
PushEventsRequest wraps EventWrapper inside a "repeated EventWrapper
events = 1" field, supposedly adding one extra recursion frame.
Investigation shows the claim does not hold. Both decode paths consume
exactly four `enter_recursion` calls before reaching the user-controlled
Value root:
EventArray path: EventArray(top) -> LogArray(oneof message)
-> Log(repeated) -> map_entry(Log.fields map)
-> Value (recurse_count = 96)
PushEvents.. path: PushEventsRequest(top) -> EventWrapper(repeated)
-> Log(oneof message of EventWrapper.event)
-> map_entry(Log.fields map)
-> Value (recurse_count = 96)
The two paths arrange the "oneof message" and "repeated message" layers
in the opposite order, but each layer costs one frame regardless of
order, so the totals match. The existing `MAX_VALUE_NESTING_FRAMES` and
`MAX_METADATA_VALUE_NESTING_FRAMES` budgets calibrated for EventArray
apply unchanged to PushEventsRequest, and `event_exceeds_max_nesting_cost`
needs no adjustment for the vector sink.
Adds three regression tests pinning the boundary so a future protocol
change that adds a wrapper (and would shift the boundary) gets caught:
- push_events_request_decode_at_value_budget: an event with cost 99
(33 effective object levels) roundtrips cleanly.
- push_events_request_decode_one_past_value_budget_fails: cost 102
(34 levels) fails decode, proving the boundary is tight.
- push_events_request_decode_at_metadata_budget: metadata at cost 96
(32 levels) roundtrips.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex flagged that the previous filter-drops accounting was dead code in production: `TopologyBuilder::build` sees `DiskV2Buffer::provides_instrumentation() == true` and deliberately skips `sender.with_usage_instrumentation(...)` for the disk-v2 stage. The handle that owns disk-v2 accounting lives on the ledger, not the BufferSender, so the dropped instrumentation bump introduced in 3598122 never fired in real deployments. Filter rejections emitted `component_discarded_events_total` but never showed up under `buffer_size_events` / `buffer_size_bytes` — operators saw a queue that appeared full of phantom events that would never be consumed. Route filter drops through the ledger directly: - `Ledger::track_dropped(event_count, byte_size)` increments both `received` and the unintentional `dropped` counter on the usage handle (and leaves `total_buffer_size` alone, since the events never reached disk). The double bump keeps `buffer_size = received - sent - dropped` consistent without requiring callers to plumb anything extra. - `BufferWriter::track_dropped` exposes that as a `pub(crate)` wrapper so `SenderAdapter` can call it without grabbing the ledger directly. - `SenderAdapter::DiskV2::send` and `try_send` now call `writer.track_dropped(...)` themselves when filter_unencodable drops sub-items, and return `Result<()>` / `Result<Option<T>>` again. The `FilterDrops` and `TrySendOutcome` types added in 3598122 are removed -- they only existed to plumb counts through BufferSender, which is unreachable for the only backend that filters. - `BufferSender::send` reverts to its pre-FilterDrops shape (the `was_dropped` + `item_sizing` pattern), with a docstring noting that filter drops are now reported by the backend's own usage handle. The disk-v2 filter-metrics test drops its manual `sender.with_usage_instrumentation(...)` call, mirroring the production wiring (`TopologyBuilder` does not attach instrumentation to disk-v2 senders). The test now reads from the ledger's usage handle directly -- the canonical place where disk-v2 accounting lives -- and adds a `sender.flush()` between sends so `track_write` actually reaches the ledger (it only fires when buffered writes are flushed to disk). A regression of `SenderAdapter::DiskV2::{send,try_send}` is caught at compile time rather than test time: `track_dropped` has exactly one caller, so reverting either fix turns it into dead code and trips `#![deny(warnings)]`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The TCP and Unix socket sinks built `EncodedEvent::new(Bytes::new(), ...)` on encoder failure with default (fresh) finalizers, dropping the original finalizers without any status update. That fall-through path turned both hard encoder errors and the native serializer's new "Ok with empty bytes" silent-drop signal into source-side acks that looked like successful deliveries -- so with end-to-end acknowledgements, over-budget events on a native-encoded TCP/Unix pipeline could be checkpointed as delivered when the encoder actually dropped them. Replace the if-ok shape with an explicit match on three outcomes: - Encoder Err: framework already logged + counted + `ComponentEventsDropped`. Set `EventStatus::Errored` on the finalizers before they go out of scope so source acks reflect the drop. Return a stub `EncodedEvent` to keep the existing pipeline shape. - Encoder Ok with empty bytes: the encoder accepted the event but produced no output (e.g. native codec rejecting an over-budget event). Set `EventStatus::Rejected` and return a stub. The encoder has already emitted `ComponentEventsDropped` with the nesting reason; nothing else needs to. - Encoder Ok with bytes: existing behaviour, finalizers attached to the live `EncodedEvent`. The Err branch was already buggy before the native silent-drop change landed -- finalizers fell to the default delivered status whenever any codec returned Err. This commit fixes both that pre-existing bug and the new native-codec silent-drop signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
removed
the
meta: awaiting author
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13999c5f58
ℹ️ 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".
The framer-skip introduced in 8a22d86 treated *any* empty serializer payload as a dropped-event sentinel, but text-based serializers legitimately produce zero bytes for events with no message field -- including `TextSerializer` for logs with no `message` and any `Trace` event, and `RawMessageSerializer` for the same shape. Suppressing framing for those swallowed the per-event delimiter NDJSON consumers expect, and downstream sinks that key off `bytes.is_empty()` finalized the (otherwise valid) events as Rejected, losing them. Gate the framer-skip on the serializer: - Add `Serializer::empty_output_means_dropped()` -- only the native protobuf serializer claims `true` today. Valid native events always produce at least the protobuf wrapper bytes, so an empty payload there can only mean the encoder refused the event; for everything else, zero bytes is part of the legitimate output domain. - `Encoder<Framer>::encode` skips framing only when the serializer opts in via that method. Text/raw-message empty payloads now get their delimiter back. Adds `test_encode_batch_text_empty_message_keeps_newline_framing` to pin the behaviour: a batch of [valid, empty, valid] through NewlineDelimited + Text now emits `"ok\n\nok\n"` rather than `"okok\n"` (the bug shape). Stashing the encoder change confirms the test catches the regression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ode-owned paths
Reassessing the branch holistically: the protobuf nesting limit is a
*decode*-side constraint (prost caps recursion at 100 frames on decode;
encode has no limit), so a deeply-nested event always serializes fine and
only fails when something later decodes it. The branch had grown an
encoder-side "silently drop over-budget events" guard in the native
serializer, which spawned a cascade of follow-on fixes (empty-output
framing sentinel, console/tcp/unix finalization, batched-encoder
handling) and an unresolved 16-sink finalizer problem in the
detach-then-encode request-builder pattern.
That guard was the wrong layer and, it turns out, redundant: every Vector
decode consumer of the native format already handles an undecodable frame
gracefully — `decoding::Error::ParsingError` reports `can_continue() ==
true`, so a native source drops the bad frame with telemetry and keeps
going. No corruption, no loop. The encoder guard was defending a case the
receiver already handles.
The two cases that genuinely need encode-side prevention are the ones
where Vector owns the decode and graceful handling is impossible, and
both already filter cleanly while finalizers are attached:
- disk buffer (`Bufferable::filter_unencodable` at the buffer sender,
ledger drop accounting) — a written-but-unreadable record permanently
poisons the buffer.
- `vector` sink (`event_exceeds_max_nesting_cost` filter in `run_inner`)
— avoids the tonic-decode-error → gRPC Internal → infinite-retry loop.
So this revert removes the native-encoder guard and its entire cascade,
restoring to the branch base:
- lib/codecs: native serializer (plain encode), encoder framing, the
`Serializer::empty_output_means_dropped` helper, and the native-codec
nesting tests.
- src/sinks/console, src/sinks/util/{tcp,unix}: back to master.
- src/sinks/util/encoding.rs: cascade tests removed.
Kept (the coherent core — Vector-owned decode paths, finalizers attached,
tested): the nesting cost machinery, `Bufferable::filter_unencodable` +
ledger `track_dropped` + disk-full overflow handling, the vector-sink
filter, and the PushEventsRequest boundary tests. The 16-sink question
no longer exists.
Changelog updated to drop the native-codec scope claim.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b172e6cb18
ℹ️ 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".
…y_send In WhenFull::Overflow with a disk-v2 base, over-budget sub-items are filtered and dropped at the disk stage past the is_buffer_full() check rather than routed to a (possibly non-protobuf) overflow stage. This surfaces two ways: a partially-over-budget item whose remainder is then rejected for fullness overflows minus the dropped events, and a fully over-budget item is dropped before any capacity check. Routing unencodable items by WhenFull is a BufferSender-level policy decision while filtering lives in the backend; reconciling them is deferred. The window is narrow and atypical (disk in Overflow mode, non-protobuf overflow target, >32-level nesting, a downstream egress that could deliver it); in other topologies these events are dropped a stage later regardless. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@pront - I think this is ready for re-review when you have a moment! |
openshift-merge-bot Bot
pushed a commit
to ViaQ/vector
that referenced
this pull request
Aug 7, 2026
…uption apply vectordotdev#25417 Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
Contributor
|
We hit this last week as a production outage and I want to add evidence in support of merging. It happened when an application logged a stack trace with several nested Still present in
data_dir: /var/lib/vector
sources:
stdin:
type: stdin
transforms:
parse:
type: remap
inputs: [stdin]
source: |
. = parse_json!(string!(.message), 128)
sinks:
poison:
type: http
inputs: [parse]
uri: http://127.0.0.1:9/ # discard port, never drains
encoding:
codec: json
healthcheck:
enabled: false
buffer:
type: disk
max_size: 268435500
when_full: drop_newestFeed a few hundred copies of one line on stdin, holding stdin open for about 15 seconds so the buffer flushes and the reader reads the record back. Fails, 34 object levels, 102 frames: Clean control, 33 object levels, 99 frames: Or generate any depth: python3 -c 'o="{\"leaf\":\"x\"}"; [o:=f"{{\"a\":{o}}}" for _ in range(32)]; print("{\"d\":"+o+"}")'Pass 1, fresh buffer directory: Pass 2, restart against the same buffer directory: Vector will not start. At 33 levels both passes are clean. Our measured boundary matches
Two structurally different shapes bracket prost's 100 limit to a single frame, and 99 is the last value that decodes. We arrived at that number independently of this PR, so the constant chosen here is exactly right. Why this is more severe than a delivery failure. Two knock-on effects we saw in production:
These look like one bug. #19315 is the root cause, #20212 the writer-side symptom, #25642 the startup symptom, and #25691 makes the startup case recoverable without stopping the poison being written. This PR fixes the cause. Also worth noting ViaQ have already cherry-picked this PR into their downstream fork (ViaQ#286, merged 7 Aug), so it has a distributor running it. |
Member
|
@codex fresh review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
pront
reviewed
Aug 12, 2026
pront
left a comment
pront
left a comment
Member
There was a problem hiding this comment.
Before final approval, please update the branch from master, the are merge conflicts.
Please also make overflow behavior state-independent: when a disk stage uses WhenFull::Overflow, an item that the disk cannot safely encode should be passed intact to the overflow stage, regardless of current disk occupancy.
Today, with a 100 MB disk overflowing to memory, the same over-nested event is rejected at 99 MB but reaches memory at 100 MB. Please fix that inconsistency and add a regression test covering both the near-full and already-full cases.
2 of 4 tasks
Contributor
|
@ganelo we hit this as a production outage last week, so rather than let the branch sit we have opened #26099 against vectordotdev/vector building directly on this one: your commits, with the master merge on top, plus the state-independent overflow behaviour and the two regression tests from @pront's review. Authorship is preserved in the commit history and the changelog fragment, and we have not touched the original design beyond that feedback. Thanks for the original work, the diagnosis in it saved us a great deal of time. |
Member
|
Final version: #26099 |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
Note: this is a continuation of #25053 - the author of that PR was interning with us and his internship has now ended, so I'm taking over iteration. Please close the older PR at your leisure.
Summary
Log events with deeply nested fields (>32 levels) corrupt disk buffers irreversibly. The buffer cannot be read, Vector cannot start, and the only recovery is manually deleting the buffer. The same events also cause infinite retry loops in the vector-to-vector gRPC path.
The root cause is that prost encodes these events successfully but fails to decode them. prost enforces a recursion limit of 100 on decode but not on encode. In Vector's proto schema, each level of map nesting consumes 3 prost recursion levels, so events deeper than ~32 levels encode fine but fail on every subsequent read.
The disk buffer failure is straightforward:
InvalidProtobufPayloadand the event is lostFailedToValidate, and Vector cannot start. The buffer must be manually deleted.Approach
This PR rejects overly nested events at encode time, before they reach the buffer or wire. A
MAX_NESTING_DEPTH(32) check is added at the three protobuf encoding boundaries:EventArray::encodeguards disk buffer writesNativeSerializer::encodeguards the native codec (TCP/file sinks)VectorSinkstream filter guards the gRPC vector-to-vector path, emittingComponentEventsDropped(intentional) so operators can monitor viacomponent_discarded_events_totalThe limit of 32 is the highest safe value across all proto encoding paths. Unit tests verify that events at this depth roundtrip correctly and that depth 33 fails prost decode.
Each check plugs into its existing error handling framework: the disk buffer sender treats encode errors as fatal (pre-existing behavior), the native codec surfaces errors through
EncoderSerializeError, and the vector sink emitsComponentEventsDroppeddirectly.Comparison with prior approaches
#19413 proposed enabling prost's
no-recursion-limitfeature. It was closed without merge because maintainers preferred a configurable limit. A configurable prost limit would require upstream changes to prost (prost#785 is in review for build-time configurable limits). This PR provides an immediate fix that can coexist with a future configurable limit.Vector configuration
Reproduction using disk buffer:
Reproduction using disk buffer:
How did you test this PR?
deeply_nested_event_encodes_but_fails_prost_decode)event_at_max_depth_roundtrips_via_prost)EventArray(disk buffer) and native codec pathscheck_value_depthwith configurable limits, arrays, and objectsEventWrapperpath (vector sink gRPC) verified safe atMAX_NESTING_DEPTHChange Type
Is this a breaking change?
Events at depth ≤32 continue to work. Events at depth >32 previously caused silent disk buffer corruption, infinite gRPC retries, or undecodable bytes, and are now explicitly rejected. No working pipeline is affected.
Does this PR include user facing changes?
no-changeloglabel to this PR.References
InvalidProtobufPayloaderrorsno-recursion-limit(closed; maintainers preferred bounded approach)Notes
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook, please see this template.make fmtmake check-clippy(if there are failures it's possible some of them can be fixed withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details on the dd-rust-license-tool.