[e673]

Notes

Adds validated patching of write-back-cache state files from an edited dump. Supports safe header, entry, and request-field repairs for V5 and V6 state files.

Motivation

The state dumper identifies corruption, but operators also need a controlled way to repair recoverable state without rebuilding the entire cache. Every proposed change must be validated before mutation to prevent stale dumps, malformed boundaries, or overlapping entries from causing further corruption.

Changes

• Added state-file patching with stale-dump detection.
• Restricted changes to supported recovery fields.
• Validated cursor boundaries, retained entry ranges, checksums, alignment, and request intervals.
• Allowed explicit buffer clearing when existing cursors are corrupted.
• Prevented entry repairs from overwriting retained entries.
• Applied validated header, entry, and write-request changes.
• Added V5 and V6 tests covering successful repairs and rejected unsafe patches.

Issue

#1751

[e673]

AI review

PR #7457 Review

Verdict: Request changes

I found five actionable issues. The most serious are:

• A successful patch can modify a retained request’s payload.
• An accepted offset change can cause an unexpected exception during cache reload.
• An interrupted boundary update can resurrect discarded entries.

Review scope

I reviewed commit 7616ba4 against parent 0b657dc, including the PR description, the five changed files, and the relevant ring-buffer and write-back-cache consumers. At the time of review, the public PR view reported “Conversation 0”; there were no visible reviewer comments to incorporate.

All state_file_processor* locations below are relative to:

cloud/filestore/tools/ops/write_back_cache_state_tool/lib/

---

1. [P1] Overlapping entries in a corrupt dump can invalidate a repair and modify request payload bytes

Location: state_file_processor.cpp:456–470, and the application loop at 580–585.

Problem

The new application loop assumes that patching one dumped entry cannot affect another. That assumption does not hold for a corrupted wrapped buffer: dumping can include a low-address entry whose corrupted size extends into an already-dumped high-address entry.

The proposed boundaries are validated separately, but there is no cross-entry overlap check before applying the changes.

Source: patch implementation

Reproduction

Use the following V5 layout:

Property	Value
Data capacity	128
Original boundaries	ReadPos = 64, WritePos = 32
High-address entry A	Header at 64, data size 32; ends at 104
Slack marker	At 104
Low-address entry B	Header at 0, corrupted data size 88; ends at 96
Requested boundaries	ReadPos = 0, WritePos = 96, retaining B
Requested entry changes	Repair both checksums to their dumped ActualDataChecksum

The original dump contains A followed by B. The new range is structurally traversable as just B, so boundary validation accepts it.

Applying A’s checksum repair writes bytes 68–71. Those bytes are inside B’s payload, which occupies [8, 96).

Applying B’s checksum-only repair then stores the checksum calculated before those bytes changed. The operation can return success despite:

1. Modifying B’s supposedly immutable payload.
2. Leaving B with a bad checksum.

I reproduced this in a deterministic, byte-level V5 model:

Stored B checksum: 0xe7729433
Actual B checksum: 0xd9f94e1d

Recommended fix

Before mutation, validate the relationships between write ranges and retained entry extents, not just each entry’s individual mapping bounds.

Reject a repair when modifying one recovered entry would overwrite another retained entry’s payload or header. Boundary-only clearing or dropping of an ambiguous region can remain a separate permitted operation.

Recalculating every checksum afterward is not sufficient: it would conceal the unintended payload modification.

Regression test

Construct this overlapping wrapped layout, request both checksum repairs and the boundary change, and require:

• Rejection of the patch.
• Byte-for-byte preservation of the entire file.

Parameterize the test for V5 and V6.

I've added a check for this

2. [P1] A permitted Offset change can make cache initialization throw

Location: state_file_processor.cpp:353–363, where request-field changes are validated.

Problem

There is no check that the new request interval is representable without unsigned overflow.

Downstream, TNodeCache::EnqueueUnflushedRequest calculates the end as Offset + ByteCount and inserts that interval into TDisjointIntervalMap. The map uses a throwing invariant check:

Y_ENSURE(begin < end);

Source: patch validation

Reproduction

For the existing test’s eight-byte payload, change the requested offset to:

Offset = UINT64_MAX
Size   = 8

Computed end = 7

The patch can produce a checksum-valid ring-buffer entry, but the request cannot be loaded into the cache because its interval is:

[UINT64_MAX, 7)

This is particularly undesirable because initialization is designed to return TError. Request deserialization passes the cached request to the cache-state visitor, which reaches the interval insertion. The malformed repaired request instead causes an exception through that path.

Source: write-data request initialization

Recommended fix

Validate the resulting interval for requests that will remain live:

if (newData.GetOffset() > Max<ui64>() - newData.GetSize()) {
    return MakeArgumentError("Request offset and size overflow");
}

The validation policy should still allow an operator to discard an already-invalid request, rather than requiring the request to become valid first.

Regression test

Extend ShouldPatchWriteDataRequestFields to cover:

• The first overflowing offset: expect E_ARGUMENT and an unchanged file.
• The largest non-overflowing offset: expect success.
• Reloading a repaired request through the actual cache initializer: verify that no exception escapes initialization.

Resolved

3. [P1] Clearing the buffer is not crash-safe: an intermediate cursor pair can resurrect discarded entries

Location: state_file_processor.cpp:593–595.

Problem

Flushing entry changes before publishing boundaries does not make the two cursor updates atomic. This matters especially for the special clear-to-zero operation.

The existing ring-buffer implementation already treats resetting cursors as a multi-step consistency problem: SetReadAndWritePosToZeroForEmptyBuffer uses a slack marker and an explicitly ordered sequence. The new patching path bypasses that protocol.

Source: boundary publication

Reproduction

Start with two valid, non-free entries:

A occupies [0, 40)
B occupies [40, 80)

A successful boundary-only patch retains B:

ReadPos  = 40
WritePos = 80

A’s bytes and non-free flag remain unchanged outside the active range.

Now request a clear:

header->ReadPos = 0;
header->WritePos = 0;

An interruption or partially persisted update that exposes only the first cursor change leaves:

ReadPos  = 0
WritePos = 80

That is a valid ring-buffer state containing both A and B. A was previously discarded but is now visible again.

The entry checksums do not detect this because neither entry’s contents changed.

I checked this intermediate state in the deterministic model: validation succeeds, and traversal returns A again. This contradicts the description’s intended protection against resurrecting stale entries.

Source: PR description

Recommended fix

Use a crash-safe boundary publication protocol, including for the clear operation. Options include an appropriately durable version of the existing empty-buffer/slack-marker protocol or a journaled metadata commit.

A flush before the cursor stores is not enough. Every potentially observable intermediate cursor state must be safe.

Regression test

Inspect deliberately captured intermediate persistent states between publication steps.

Do not use timing-dependent process killing as the primary test: it would be flaky and could miss this exact transition.

This is not an issue that should be addressed.

1. The probability that is may happen is nearly zero.
2. If a crash happens during the patch, an operator can just retry it.
3. The amount of code needed to resolve it will unnecessarily increase the code complexity.

4. [P2] Tag validation accepts values that the write-back-cache loader rejects

Location: state_file_processor.cpp:330–338.

Problem

Checking only capabilities.MaxTag validates the ring-buffer encoding, not the write-back-cache entry state.

Both supported formats can encode tags through 7, but ECachedWriteDataRequestTag currently ends at Flushed = 2. The loader explicitly rejects larger values.

Source: ring-buffer format capabilities

Reproduction

For a valid live request, change its tag to 3:

Patch validation:       succeeds
Ring-buffer validation: succeeds
Write-back-cache load:  rejects the entry

A recovery API should not accept a newly requested live-entry state that its actual consumer cannot deserialize.

Recommended fix

Validate requested tags against the write-back-cache semantic enum for retained live requests, in addition to the storage-format limit.

Move the enum or a non-throwing validation helper into shared code rather than duplicating the magic value 2.

As with the offset check, avoid preventing recovery of an existing bad tag when the patch discards or frees that entry.

Regression test

Request tag 3, not just Max<ui32>(), and verify rejection before mutation.

The existing invalid-tag test exercises only the much looser encoding limit.

Source: patch tests

Skipped for now, fixed later.

5. [P2] An already out-of-range cursor prevents even the explicit clear-to-zero repair

Location: state_file_processor.cpp:565–570.

Problem

An existing ReadPos or WritePos greater than DataCapacity causes accessor header validation to fail before constructing the data processor.

Debug mode retains the header but does not create the processor in this case. PatchStateFile consequently rejects the operation before reaching its clear-buffer handling.

Source: accessor initialization

Reproduction

1. Create a valid state file.
2. Corrupt ReadPos to DataCapacity + 1.
3. Dump it using the Debug accessor.
4. Set the dumped ReadPos and WritePos to zero.
5. Apply the patch.

The result is:

State file is not initialized, nothing to patch

However, the layout-defining fields can be valid, and the requested operation needs no traversal of the corrupt active range.

This makes the explicit clearing operation unavailable for a straightforward class of cursor corruption—the sort of damage a recovery tool should handle. Clearing is expressly listed as a supported repair.

Source: PR description

Recommended fix

Separate validation of the mapping/layout from validation of the mutable cursors.

A recovery path could either:

• Expose a bounds-safe data processor when the layout is valid but the old cursors are not.
• Provide a narrowly constrained clear operation after layout validation.

Do not relax validation of offsets, capacities, or mapped-file extents to achieve this.

Regression test

Corrupt each existing cursor beyond capacity, then clear the buffer.

This differs from testing rejection of an out-of-range cursor supplied in the proposed patch.

Fixed

Test Review

What the tests do well

The new tests are generally meaningful.

In particular, ShouldValidateAllEntriesBeforeMutation deliberately combines an earlier valid edit with a later inaccessible entry and checks that the bytes remain unchanged.

Clearing the proposed range in that test is useful: it allows the test to reach entry validation instead of failing immediately on corrupted traversal.

The header and entry tests also reopen the state through TFileRingBuffer, rather than only checking the edited protobuf.

Source: patch tests

Flakiness assessment

I did not identify a concrete timing-flakiness problem in the added tests.

They use isolated temporary files and deterministic inputs. The fill-until-full loops depend on fixed buffer capacity, not timing. Omitting interprocess locks inside these isolated, sequential tests is not itself a race.

Source: patch tests

Important coverage gaps

Gap	Why it matters
Real consumer behavior is not exercised	Ring-buffer validity does not imply that write-back-cache initialization succeeds, as the offset and tag findings demonstrate.
Flush failures and interrupted publication are not exercised	Successful remapping does not test the persistent state left by an interrupted repair.
Payload immutability is not asserted directly	The request-field test checks decoded fields and size, but should also compare payload bytes before and after repair.

Source: current test coverage

Fixture caveat for consumer-level tests

The existing 64-byte bootstrap cannot exercise normal TWriteDataRequestManager::Init() successfully. Initialization requires a substantially larger supported allocation size.

Use a suitably sized fixture. Otherwise, a test intended to expose the tag or offset problem will fail earlier for an unrelated capacity error.

Sources: test fixture, request manager initialization.

---

Memory Safety, Move Assignment, RAII, and Exceptions

Direct out-of-bounds access

I did not find a direct out-of-bounds operation in the new request-header memcpys under the documented stable-mapping precondition.

Request-info presence originates from a sufficiently large entry, size and presence are checked against the current dump, and the data processor bounds-checks the requested payload extent.

The overlapping-entry finding is a different problem: each access can be inside the mapping while still overwriting another logical object.

Source: patch implementation

Move assignment and RAII

No custom move-assignment implementation is added by this PR, so there is no new source-destroyed-during-move implementation to flag.

The patch obtains its processor pointer after dumping/reinitializing the accessor, rather than retaining a pointer across that reset.

I did not find a new ownership or destruction-order defect in the changed code.

Source: reviewed commit

Exceptions

The most concrete exception-related issue is finding 2: the patch can create input that later reaches a throwing invariant check.

That should be prevented with an ordinary validation error. Adding blanket noexcept to allocation-heavy protobuf/string code would not fix it.

---

Complexity Reduction

A small validated patch plan would help.

Validation could produce:

• The exact changed entries.
• The final header values.
• The affected byte ranges.

Application would consume that plan instead of rediscovering changes independently.

This would provide one place to establish non-overlap and consumer invariants, reduce duplicated change-detection logic, and allow allocation and diagnostic construction to happen before mutation.

Keep the durable publication protocol explicit and separate. A patch-plan abstraction must not obscure the crash-consistency steps.

---

Execution Scope and Limitations

I reviewed the C++ sources and ran review_models.pystandalone deterministic models for:

• The overlapping-checksum repair.
• The interrupted clear.
• The offset overflow.

I did not compile or run the repository’s C++ test suite. The models validate the concrete state transitions described above, not the complete integration environment.

---

Follow-up:

Found three issues, reproduced in both V5 and V6:

1. [P2] Patching accepts tags that prevent cache loading. ValidateEntryPatch allows tags up to the ring format’s maximum, 7. Changing a retained request’s tag to 3 succeeds, but TWriteDataRequestManager::Init rejects tags above 2. Validate retained requests against the consumer’s tag range.

Fixed

2. [P2] Incomplete patch messages can silently clear the buffer. PatchStateFile interprets missing cursor fields as zero. Removing required WritePos from a dump whose ReadPos is zero succeeds and discards all entries. Reject messages that fail IsInitialized() before mutation. The default JSON parser guards this, but direct C++ callers are exposed.

Fixed

3. [P2] Metadata repair fails for a valid empty-buffer representation. FindRetainedEntryRange recognizes only ReadPos == WritePos as empty. The format also permits WritePos == 0 with ReadPos pointing to slack. Repairing only its metadata checksum incorrectly returns E_ARGUMENT.

No need to address it because it will complicate the code.
An operator can resolve the cursor position in the first patch and repair metadata in the second patch.

Compute the retained range once and reuse it to remove duplicate validation. The locking comment also still references the removed TExistingFileLock.

All 60 existing tests pass. Temporary probes confirmed these gaps and were removed; the working tree is clean. No additional bounds, RAII, or flakiness issues found.