[e673]

Notes

Added a write-back-cache state dumper that extracts file headers, metadata, entries, checksums, and serialized write-request information. Corruption details and unavailable calculated values are represented explicitly.

Motivation

Operators need readable diagnostics to understand damaged write-back-cache state before attempting recovery. The dump must distinguish verified values from unavailable ones and preserve malformed entry information instead of hiding the source of corruption.

Changes

• Added write-back-cache header and entry dumping.
• Added stored and calculated checksum reporting.
• Added serialized write-request field extraction.
• Added validation error reporting.
• Made calculated checksums optional when source data is unavailable.
• Preserved malformed zero-size headers without treating them as wrap markers.
• Added V5/V6 coverage for valid, corrupted, wrapped, truncated, and read-only state files.

Issue

#1751

[e673]

AI review

PR #7456 — Code Review

Summary

I found two concrete diagnostic-correctness issues. I did not find a demonstrated out-of-bounds access, use-after-free, or move-assignment defect in this diff.

I reviewed commit 0b657dcc869e29b5f475a1e867ff48965b0234ae, the six changed files, and the relevant accessor, validator, ring-buffer format, and request-serialization code. The description explicitly promises useful diagnostics for corrupted files.

Review-comments limitation: GitHub showed “Conversation 0.” No reviewer comments were visible, and the review-comments API was inaccessible, so I cannot claim to have inspected unavailable threads.

Paths below are relative to:

cloud/filestore/tools/ops/write_back_cache_state_tool/

Findings

1. [P2] Malformed zero-size headers disappear from the dump and can be mistaken for valid wrap markers

Location: lib/state_file_processor.cpp:77–80, with the resulting wrap at 139–141.

Problem

DumpEntry() treats every header with DataSize == 0 as slack and returns before recording its fields. However, the validator rejects zero-size headers with a nonzero tag or a set free flag; V6 also rejects a nonzero decoded checksum. Those are corrupted headers, not valid slack markers.

Source: reviewed commit.

Reproducer

Create two entries: "Hello", "Bye".
Change the second entry header to DataSize = 0, Tag = 1.
Dump the file.

The resulting behavior, reproduced in the standalone model for both versions, is:

IsCorrupted = true
Entries = [the first, healthy entry]

The offending entry’s position, tag, free flag, and checksum are absent. The dump also has no validation-error field explaining why validation failed. Consequently, the operator receives a corruption flag and apparently healthy surviving entries, but not the malformed header needed to diagnose the failure.

The current output schema cannot preserve that explanation separately.

There is also a traversal consequence: after an earlier high-address entry, this malformed header produces Slack, allowing the caller to wrap to offset zero. A malformed header should not silently carry the same control-flow meaning as a valid wrap marker.

Suggested fix

Distinguish valid explicit slack, implicit end-of-buffer slack, and malformed headers. Preserve malformed header information—either as an invalid entry or a separate diagnostic—and do not automatically use it as a wrap instruction.

Including GetLastValidationError() in the dump would also prevent the immediate loss of diagnostic information.

Regression coverage

Test zero-size headers with Tag != 0 and FreeFlag == true in V5 and V6, plus a bad zero-size-header checksum in V6. Cover both the initial read position and a position after a valid high-address entry. Keep the existing valid implicit-slack and wrapped-buffer cases passing.

Fixed

2. [P2] An unavailable calculated checksum is represented as a successfully calculated zero

Locations: lib/state_file_processor.cpp:43–46, 117–122; the ActualDataChecksum and ActualMetadataChecksum fields in the protobuf.

Problem

For an unreadable entry payload, FillEntryInfo() sets ActualDataChecksum to zero without calculating a checksum. For unavailable metadata, the calculation is skipped and the non-optional scalar remains zero. Neither field carries calculation status or presence.

Source: state_file_processor.cpp.

Reproducer

DataCapacity = 64
Entry.DataSize = 1,000,000
Entry.DataChecksum = 0
Entry.FreeFlag = false

The payload accessor correctly rejects the range. Nevertheless, the dump reports:

DataChecksum       = 0
ActualDataChecksum = 0

That appears to be a matching checksum pair even though the second value was never computed. I reproduced this output for both versions in the model.

This does not bypass the global corruption flag or establish an out-of-bounds read. The defect is that the per-checksum diagnostic cannot distinguish “verified as zero” from “unavailable.” That distinction matters for a tool whose purpose is to explain corruption and provide calculated repair values.

Suggested fix

Make calculation availability explicit, using an optional scalar or a checksum-status field. Only publish a calculated value when the corresponding byte range was actually available.

Keep the existing free-entry checksum convention separate from the unavailable-payload case.

Regression coverage

Use an unreadable payload whose stored checksum is zero, and verify that the actual checksum is marked unavailable rather than matching. Also distinguish unavailable metadata from valid empty metadata, whose calculated checksum can legitimately be zero.

Fixed

Recovery Limitation and Test Coverage

Genuine truncation is not exercised by the new tests

The new suite tests an empty file, a one-byte malformed file, a zeroed header, and an oversized entry declaration. It does not create an initialized file and then truncate it.

Source: state_file_processor_ut.cpp.

That missing case exposes a substantial limitation:

Create a valid file with DataCapacity = 64 and one short entry.
Remove the final byte, entirely within unused tail space.
Dump the file.

Although the entry remains completely present, the accessor rejects the declared capacity before constructing the data processor. DumpStateFile() therefore returns no entries.

This follows the accessor’s documented behavior: Debug mode exposes a data processor only when the file header passes validation.

The model produced:

Version	Intact occupied range	Remaining data bytes	Entries before → after
V5	[0, 13)	63	1 → 0
V6	[0, 16)	63	1 → 0

I would treat this as an explicit recovery-contract decision, rather than recommend simply bypassing the null check. Either document and test that structurally truncated files produce header-only diagnostics, including the validation reason, or add carefully bounded recovery of physically present regions.

Removing the guard without replacing the accessor’s safety guarantees would be wrong.

Added a test

The read-only guarantee needs a stronger test

All new fixtures use a read-write mapping. Several checksum expectations are calculated from the mapped bytes after calling Dump(). Those assertions would not detect a future implementation that modified the file during validation and then checksummed the modified bytes.

Source: state_file_processor_ut.cpp.

Add a test that saves the bytes before dumping and checks byte-for-byte equality afterward. A separate read-only-mapping test would enforce the intended access mode more directly.

Added a test

I did not find an obvious flakiness mechanism

The new tests use fixture-local temporary files and synchronous operations, without sleeps, timing thresholds, or concurrent workers. Reopening the accessor after the ring buffer may resize the file is appropriate.

The request-header test using three versus four ui64 values is also meaningful: the real deserializer requires a serialized header plus a nonempty payload, so the strict size comparison is not an off-by-one bug.

Source: state_file_processor_ut.cpp.

This is a source-level assessment, not evidence from repeated CI executions.

Memory Safety, Ownership, and Exceptions

Memory access

The new memcpy is protected by the underlying range check.

GetEntryDataPtr() checks the header and complete payload against the mapped data region, including V6 alignment requirements, using subtraction-based bounds checks. FillEntryInfo() copies the request header only when the pointer is non-null and the declared payload is larger than that header.

I do not see a physical out-of-bounds read on that path. The checksumming path uses the same validated payload pointer.

Sources: file_ring_buffer_accessor.cpp and state_file_processor.cpp.

An entry can extend past the logical WritePos while still fitting inside the mapping. I investigated this, but would not call it an out-of-bounds bug or automatically require clipping it: when diagnosing a corrupted write position, showing physically present candidate data can be intentional and useful.

The distinction between physically readable data and structurally valid entries should instead be made explicit in diagnostics and tests. The normal ring-buffer implementation does enforce the logical endpoint for ordinary access.

Move assignment

There is no custom move constructor or move-assignment implementation added here.

In particular, the diff contains no assignment pattern that destroys an owner and then reads from a source object nested inside that owner. The processor is stateless, and its result contains copied values rather than borrowed mapped-file pointers.

Source: state_file_processor.h.

RAII and lifetimes

I found no new ownership cleanup defect.

The temporary file outlives its accessor in the fixture. Validation reconstructs accessor-side state rather than rewriting the mapped file, and the public dump contract explicitly requires the caller to keep the mapping stable.

Sources: state_file_processor_ut.cpp and state_file_processor.h.

Exceptions

Do not add noexcept merely to suppress exception concerns.

The CRC API is already noexcept, but constructing the data processor and growing the protobuf result involve allocation. Adding noexcept to the whole dump operation without an appropriate failure policy would turn allocation failures into termination.

I found no new explicit exception-based handling of ordinary malformed-file input.

Complexity Recommendation

The useful simplification is to share low-level header/slack classification between validation and dumping, while keeping their policies different: validation rejects corruption; dumping records it.

The first finding is precisely the kind of divergence caused by implementing a weaker version of the same classification twice.

I would avoid broad refactoring of the two traversal loops until the malformed-marker and truncation behavior is pinned down by tests.

Validation Performed

I inspected the source and ran deterministic V5/V6 checks using a reproduce.pystandalone behavioral reproduction model. It reproduced the malformed-header omission, unavailable-checksum ambiguity, and unused-tail truncation behavior.

I did not build the repository, run its C++ unit tests, or run ASan/UBSan. The model supports the control-flow findings; it is not a substitute for those checks.

---

Follow-up:

Found one correctness issue:

[P2] Full-file checksum can crash on sparse tmpfs files — state_file_processor.cpp:131. Reading mapped holes allocates tmpfs pages. If space runs out, Crc32c receives SIGBUS, terminating the process instead of returning diagnostics.

I reproduced this in the compiled dumper using a sparse 4 MiB file on an isolated 2 MiB tmpfs. Computing the checksum through bounded pread reads avoids allocating those holes; add regression coverage for this case.

This is out of scope for this PR and is a responsibility of TPersistentStateManager

Minor cleanup: state_file_processor.h:15 still references the removed TExistingFileLock.

Fixed the comment

Reviewed the PR description and comments. All 31 dumper tests pass; no additional bounds, RAII, or test-flakiness problems found. Repository unchanged.