fix(buffers): recover disk_v2 from durable checkpoints#25845
Draft
graphcareful wants to merge 17 commits into
Draft
fix(buffers): recover disk_v2 from durable checkpoints#25845graphcareful wants to merge 17 commits into
graphcareful wants to merge 17 commits into
Conversation
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.
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.
Split merge_finalizers/merge_finalizer_groups off of Finalizable into a new MergeFinalizable: Finalizable trait, bound only on Bufferable and InMemoryBufferable where retry/recovery semantics actually merge finalizers back in. Finalizable's old default no-op implementation for merge_finalizers silently dropped finalizers for any type that didn't override it, which could lose event acknowledgements without any indication of the bug. Types that need merge semantics now must provide a real implementation or fail to compile.
This reverts the pre-entry accounting portion of b8c01a2. Oversized disk records should use the existing received/dropped usage counters instead of carrying a separate metric-only counter. The Delivered finalizer behavior from the surrounding oversized-record fixes is intentionally preserved.
Disk buffers self-instrument through the ledger, so oversized records should use the same usage model as other dropped records: count the attempted write as received, then count the same bytes and events as dropped. This keeps current occupancy at zero without a separate pre-entry counter. Preserve dropped write outcomes for blocking sends so WhenFull::Block does not classify an unwritable record as accepted at the topology sender layer.
…ting The disk_v2 writer computed record.size_of() before every write but only consumed it in the rare unwritable-record drop path, adding an O(events) traversal to the hot write path. Solution is to only call that method in neccessary conditional paths.
github-actions
Bot
added
docs review on hold
Record IDs advance once per buffered event, so exhausting the u64 ID space is not a realistic operating condition. Treat record IDs as strictly monotonic and document exhaustion as unsupported, which avoids carrying wraparound semantics through reader, writer, accounting, and recovery logic.
Treat the durable ledger checkpoint as the source of truth when reopening disk_v2 buffers. Startup now reconciles only the checkpointed reader/writer data-file window, scans boundary files to exclude acknowledged prefixes and post-checkpoint tails, and counts strict middle files from their metadata. When the current writer file is ahead of the checkpoint or has a torn/corrupt tail, truncate it back to the last checkpointed boundary instead of fast-forwarding the ledger or rolling onto the next file. This keeps buffer size recovery tied to the durable checkpoint and avoids the restart underflow path. Add file truncation support to the async filesystem abstraction and update disk_v2 recovery tests for the new checkpoint-authoritative behavior.
Move physical data file deletion out of the reader acknowledgement path and into a periodic stale-file cleanup pass based on the durable checkpoint window. Keep reader-side buffer accounting separate by decrementing abandoned bad-tail bytes at the point where the reader encounters a bad read and rolls the file. Add focused cleanup coverage for normal and wrapped checkpoint windows, and update disk_v2 invariants/model tests for deferred physical deletion.
3 tasks
graphcareful
force-pushed
the
fix/disk-v2-checkpoint-window-recovery
branch
from
July 15, 2026 16:03
835727e to
e091128
Compare
github-actions
Bot
removed
domain: sources
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
disk_v2startup state from the durable checkpoint window instead of mixing independently persisted ledger and file state.Vector configuration
Not applicable; this changes internal
disk_v2buffer recovery behavior.How did you test this PR?
vector-bufferstests for checkpoint-boundary recovery, torn and corrupt tails, missing files, stale-file cleanup, restart accounting, and runtime bad-read accounting.Change Type
Is this a breaking change?
Does this PR include user facing changes?
changelog.d/disk_v2_buffer_size_recompute_on_restart.fix.md.no-changeloglabel to this PR.References
None.
Notes
This PR is intentionally opened as a draft while broader validation is pending.