issue-3781: TFileRingBuffer - operations should return verbose errors by e673 · Pull Request #6866 · ydb-platform/nbs

e673 · 2026-08-21T10:58:56Z

Notes

Current API of TFileRingBuffer makes it impossible to figure out if a method was completed with success or failed, and if failed — if it was caused by a corruption or an invalid argument.

This PR proposes the following changes in API:

PushBack: bool -> TResultOrError<bool>
Commit: bool -> TError
Free: bool -> TError
GetTag: ui32 -> TResultOrError<ui32>
SetTag: void -> TError
Front: TStringBuf -> TResultOrError<TStringBuf>
PopFront: void -> TResultOrError<bool>
Visit: void -> TError
GetMetadata: TStringBufer -> TResultOrError<TStringBuf>
SetMetadata: bool -> TResultOrError<bool>
IsCorrupted, SetCorrupted: these methods become thread-safe

General rule: an error is returned in the case of corruption or invalid argument.

This is needed for WriteBackCache and HandleOpsQueue so they can handle the situation when a corruption happens in the middle of processing.

Additional changes:

Return an error when corruption is detected in Visit or Free
Added missing UNIT_ASSERT check

Issue

#3781

github-actions · 2026-08-21T10:59:58Z

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🔴 tasks + storage (build failed, log)
✅ blockstore
✅ disk + manager
✅ filestore

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 268s): all tests PASSED for commit 4ceee30.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
1625	1625	0	0	0	0	0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1573s): some tests FAILED for commit 4ceee30.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
6871	6869	0	0	1	1	0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 918s): some tests FAILED for commit 4ceee30.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
6871	6869	0	0	1	1	0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1032s): some tests FAILED for commit 4ceee30.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
6871	6869	0	0	1	1	0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4770s): some tests FAILED for commit 4ceee30.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4371	4369	0	0	1	1	0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4273s): some tests FAILED for commit 4ceee30.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4371	4369	0	0	1	1	0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4393s): some tests FAILED for commit 4ceee30.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4371	4369	0	0	1	1	0

github-actions · 2026-08-21T16:29:17Z

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🚫 tasks + storage (cancelled or timed out before reporting completion)
🚫 blockstore (cancelled or timed out before reporting completion)
🚫 disk + manager (cancelled or timed out before reporting completion)
🚫 filestore (cancelled or timed out before reporting completion)

github-actions · 2026-08-21T16:37:29Z

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 267s): all tests PASSED for commit f5419ac.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
1352	1352	0	0	0	0	0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 273s): all tests PASSED for commit f5419ac.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
1625	1625	0	0	0	0	0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1451s): some tests FAILED for commit f5419ac.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7061	7059	0	1	0	1	0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 917s): all tests PASSED for commit f5419ac.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7063	7062	0	0	0	1	0

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4863s): all tests PASSED for commit f5419ac.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4563	4562	0	0	0	1	0

github-actions · 2026-08-21T19:57:30Z

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🚫 tasks + storage (cancelled or timed out before reporting completion)
🚫 blockstore (cancelled or timed out before reporting completion)
🚫 disk + manager (cancelled or timed out before reporting completion)
🚫 filestore (cancelled or timed out before reporting completion)

e673 · 2026-08-21T20:31:37Z

AI review:

I pulled the PR (2 commits, 6 files: file_ring_buffer.{h,cpp}, two test files, and the two call sites handle_ops_queue.cpp and persistent_storage.cpp) and reviewed the full diff. Overall the direction is good, but there are several places where the implementation contradicts the new documented contracts, and — ironically — a few places where the new error information is dropped exactly where the PR says it's needed. The PR's stated goal is to make it possible to distinguish success from failure, and corruption from invalid argument, so WriteBackCache and HandleOpsQueue can handle mid-processing corruption — I'll judge it against that.

Bugs / contract violations

1. SetTag violates its own new documentation. The header promises: "Returns an error if the pointer is invalid, the tag value exceeds the value returned by GetMaxTag() or if the buffer is corrupted." But the implementation never checks the tag, and it discards the bool returned by WriteEntryHeader:

auto eh = Data()->ReadEntryHeader(it->second);
eh.Tag = tag;
Data()->WriteEntryHeader(it->second, eh);   // returns false if Tag > MaxTag — ignored
return {};

WriteEntryHeader in file_ring_buffer_format.cpp refuses to write when header.Tag > MaxTag (and MaxTag is 0 for pre-V5 formats), so SetTag(ptr, 100) is a silent no-op reported as success. Compare with Commit, which does check the same return value (Y_ABORT_UNLESS(written)).

I've added an argument check and a test for SetTag

2. Corruption detected mid-operation is still returned as success. This is the exact scenario the PR exists for:

Visit(): VisitEntries can hit an invalid entry, call SetCorrupted("Invalid entry detected at VisitEntries"), and stop early — but Visit then returns {}. The caller gets S_OK with a silently truncated iteration. It should return an error if IsCorrupted() after VisitEntries (this matters for WriteBackCache recovery, which replays entries via Visit).
Free(): EraseFreeEntriesFromFront can set the corrupted flag ("Invalid front entry"), yet Free returns {} — and consequently PopFront returns true as well.

Visit and Free should return an error in this case.

3. A failed Commit leaks CurrentAllocation with no way to recover. Commit can now fail via ValidateAccess (e.g., corruption flagged between Alloc and Commit, which is now possible since SetCorrupted is callable from another thread). CurrentAllocation isn't cleared on that path and there is no Discard/Abort API, so every subsequent Alloc/PushBack fails — and, because the CurrentAllocation.HasValue() check in Alloc comes before ValidateAccess, the caller gets a misleading E_INVALID_STATE "Previous allocation is not committed" instead of the corruption error, and the ReportAccessToCorruptedFileRingBuffer critical event never fires from that path.

I've switched checks in Alloc — now it will fail with a corruption error.

4. The error-code taxonomy undercuts the PR's goal. Corruption maps to E_INVALID_STATE, but so does the "Previous allocation is not committed" usage error — a caller trying to distinguish corruption from misuse by code can't. Meanwhile Commit with no pending allocation returns E_ARGUMENT "No allocation to commit", though Commit takes no arguments — that's a state error. I'd pick a consistent mapping (e.g., corruption → one dedicated code, API misuse → another) or callers will end up string-matching messages.

This is not needed. In future, there will be a mandatory argument in Commit.

Call sites still drop the new errors

5. THandleOpsQueue::PopFront discards the result (RequestsToProcess.PopFront();). Combined with fs_impl.cpp::ProcessHandleOpsQueue, a corrupted buffer produces an endless reschedule loop: Empty() is false, Front() returns nullopt, the recovery PopFront() silently does nothing on a corrupted buffer, and the function reschedules — firing ReportHandleOpsQueueProcessError on every tick. The loop shape is pre-existing, but the information needed to break out of it now exists and is thrown away.

This is out of scope from this PR and will be resolved separately #6666

6. THandleOpsQueue::Front counts a corruption error as IncrementParseErrorCount() — conflating precisely the two failure modes this PR separates. A corruption counter (or at least distinct handling) would be more honest.

This is out of scope from this PR and will be resolved separately #6666

7. persistent_storage.cpp: Visit and SetTag discard the returned errors, while Commit/Free convert corruption into Y_ENSURE exceptions. If the graceful handling is planned for a follow-up WriteBackCache PR, fine — but worth saying so explicitly, since until then a corruption detected between Alloc and Commit becomes a throw where it previously couldn't happen.

I've replaced Y_ENSURE with a proper error return.

Thread-safety commit

8. The docs overpromise. "All further operations on the buffer will fail once the flag is set" isn't guaranteed cross-thread: IsCorrupted uses a relaxed load with no synchronization, and an operation already past its ValidateAccess check will complete normally. SetCorrupted from another thread is not a safe "cancel" — I'd soften the wording so WriteBackCache authors don't rely on it. (The atomics themselves are fine; exchange defaults to seq_cst and the dedup of the critical event is correct.)

The wording is:
All further operations on the buffer will fail once the flag is set.
It is ok if currently executing operations will complete successfully.

Tests

while (rb.PushBack("ABCD").GetResult()) in file_ring_buffer_accessor_ut.cpp (twice) swallows errors — an error is indistinguishable from "buffer full", so the loop exits on a wrong premise. Assert !HasError inside the loop.

Done

file_ring_buffer_accessor_ut.cpp uses UNIT_ASSERT_VALUES_EQUAL(true, <TResultOrError<bool>>) but the required Out<NCloud::TResultOrError<bool>> specialization is only defined at the bottom of file_ring_buffer_ut.cpp. It links only because both TUs live in the same UNITTEST target — a fragile cross-file dependency (this smells like what broke the first CI build). Move the operator== helpers and Out<> specializations into a shared test header.

Done

The assertion sweep missed a few calls: rb->PopFront() in the Alloc-while-PopFront test and rb->SetMetadata("abc") in the migration test are still unchecked.

Added asserts

Nits

The variable pushPackResult in handle_ops_queue.cpp should be pushBackResult (appears twice). Header doc grammar is inconsistent ("On success, return true" → "returns"; Free's doc is missing a final period), and the Alloc doc doesn't mention that a pending version migration makes it return nullptr even on an empty buffer — that surprised me reading the code and will surprise callers. Also, PushBack runs ValidateAccess and then Alloc repeats it, which gives corruption precedence over "uncommitted allocation" in PushBack but not in bare Alloc — worth aligning. In the PR description itself: "there methods become thread-safe" → "these", and "TStringBufer" → "TStringBuf".

Unrelated to this PR

On CI: the latest run for f5419ac showed tasks+storage and disk_manager green with blockstore and filestore still in progress at the last bot update, and the previous revision had an actual build failure in tasks+storage plus one failed-build entry in the blockstore and filestore suites — so I'd wait for the remaining two suites before merging.

The most impactful fixes: check WriteEntryHeader's result (and tag range) in SetTag, return an error from Visit/Free when corruption was detected mid-call, and stop discarding PopFront's error in THandleOpsQueue.

github-actions · 2026-08-21T20:34:48Z

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 265s): all tests PASSED for commit 7fa4981.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
1354	1354	0	0	0	0	0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 289s): all tests PASSED for commit 7fa4981.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
1625	1625	0	0	0	0	0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1443s): some tests FAILED for commit 7fa4981.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7067	7062	0	4	0	1	0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 49s): all tests PASSED for commit 7fa4981.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4	4	0	0	0	0	0

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4803s): all tests PASSED for commit 7fa4981.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4565	4564	0	0	0	1	0

SvartMetal · 2026-08-21T20:38:35Z

+        Y_ENSURE(
+            !HasError(res),
+            "Failed to commit allocation: " << FormatError(res));


Can we get rid of exceptions usage?

Yes, But this is unrelated to this PR and will be done in the next one
https://github.com/ydb-platform/nbs/pull/6867/changes#diff-4fd6698976acda0e55fdcf9397499b66e0c11ccd7e4517758b7d2296129c5ff6R102

SvartMetal · 2026-08-21T21:30:53Z

        b.Execute(
-            [](TFileRingBuffer& rb) { UNIT_ASSERT(rb.PushBack("ABC")); },
+            [](TFileRingBuffer& rb)
+            { UNIT_ASSERT_VALUES_EQUAL(true, rb.PushBack("ABC")); },


Why not UNIT_ASSERT?

rb.PushBack returns TResultOrError<bool>.
operator == (bool value, TResultOrError<bool>) checks that there is no error and the result equals to the value.

e673 requested a review from SvartMetal August 21, 2026 10:59

e673 force-pushed the users/nasonov/file-ring-buffer-api branch from 2d75ca5 to f5419ac Compare August 21, 2026 16:32

e673 added the recheck Add this label to relaunch checks, it will be automatically removed label Aug 21, 2026

github-actions Bot removed the recheck Add this label to relaunch checks, it will be automatically removed label Aug 21, 2026

Andrei Nasonov added 4 commits August 21, 2026 21:56

issue-3781: TFileRingBuffer - verbose API

926ae0c

Make IsCorrupted and SetCorrupted thread-safe

a897fdb

AI review fix: Visit and SetTag

42a55f0

AI review: detect corruption in Free()/PopFront()

92414f2

e673 force-pushed the users/nasonov/file-ring-buffer-api branch from f5419ac to 92414f2 Compare August 21, 2026 19:56

e673 changed the title ~~issue-3781: TFileRingBuffer - verbose API~~ issue-3781: TFileRingBuffer - operations should return verbose errors Aug 21, 2026

AI review: move code to a separate file, more asserts

7fa4981

SvartMetal reviewed Aug 21, 2026

View reviewed changes

Navigation Menu

Uh oh!

issue-3781: TFileRingBuffer - operations should return verbose errors - #6866

issue-3781: TFileRingBuffer - operations should return verbose errors#6866
e673 wants to merge 5 commits into
mainfrom
users/nasonov/file-ring-buffer-api

e673 commented Aug 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 21, 2026 •

edited

Loading

Uh oh!

e673 commented Aug 21, 2026

Uh oh!

github-actions Bot commented Aug 21, 2026 •

edited

Loading

Uh oh!

SvartMetal Aug 21, 2026

Uh oh!

e673 Aug 21, 2026

Uh oh!

SvartMetal Aug 21, 2026

Uh oh!

e673 Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

e673 commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Issue

Uh oh!

github-actions Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

e673 commented Aug 21, 2026

Bugs / contract violations

Call sites still drop the new errors

Thread-safety commit

Tests

Nits

Uh oh!

github-actions Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SvartMetal Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

e673 Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

SvartMetal Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

e673 Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

e673 commented Aug 21, 2026 •

edited

Loading

github-actions Bot commented Aug 21, 2026 •

edited

Loading

github-actions Bot commented Aug 21, 2026 •

edited

Loading

github-actions Bot commented Aug 21, 2026 •

edited

Loading

github-actions Bot commented Aug 21, 2026 •

edited

Loading

github-actions Bot commented Aug 21, 2026 •

edited

Loading