[e673]

Notes

Added persistent state-file discovery and locking support for Filestore recovery tools. The locator can list state files or locate and open a specific file with read-only or read-write access.

Motivation

Recovery tools need a common way to find state files by filesystem, session, and type. They also need to detect active files and lock the selected file before reading or modifying its contents.

Changes

• Added state-file metadata protobufs.
• Added state-file directory discovery and classification.
• Added filtering by filesystem ID, session ID, and file type.
• Added LocateAndOpenStateFile returning TResultOrError<TFile>.
• Added non-blocking shared and exclusive locking for existing files.
• Added unit tests and registered them in the Filestore build.

Issue

#1751

[e673]

AI review

Review of PR #7455

Verdict: Request changes

The main risks are operating on the wrong file and hanging during discovery, rather than a demonstrated out-of-bounds memory access.

Reviewed commit: 28bdbea20615d9929e84030120e58dd1b54d35ed

I reviewed the commit, its parent, the PR description, and the visible public conversation. GitHub showed Conversation 0; no reviewer discussion was visible to incorporate. The findings below are judged against the description’s explicit promises about replacement detection, symlink rejection, and test registration. [Reviewed commit](28bdbea)

Unless stated otherwise, file locations are relative to:

cloud/filestore/tools/ops/write_back_cache_state_tool/

Findings

1. [P1] Replacement between discovery and opening is not detected

Locations: lib/state_file_locator.cpp:269; lib/existing_file_lock.h:27–33

LocateStateFile() returns only a pathname. The descriptor used while collecting information is destroyed before the result reaches the caller. A subsequent TExistingFileLock opens the pathname again, with no retained reference to the discovered file. [Locator source](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/tools/ops/write_back_cache_state_tool/lib/state_file_locator.cpp)

The failing sequence is:

Discover pathname P referring to inode A.
Finish discovery and close its descriptor.
Replace P with a different regular file, inode B.
Construct TExistingFileLock(P).
TryAcquire() succeeds: both its descriptor and current P refer to B.

The check establishes that the newly opened descriptor matches the current pathname—not that it matches the file selected during discovery. That leaves the exact discovery/reopen race identified in the PR’s motivation. [Lock helper source](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/tools/ops/write_back_cache_state_tool/lib/existing_file_lock.h)

Fix: Make selection/acquisition return an owned opened or locked file, retaining the selected descriptor through acquisition and mapping. Keep metadata-only listing separate from the API used to perform recovery.

Regression test: Replace the file after LocateStateFile() returns but before constructing the operation’s lock. The existing replacement test constructs toolLock before replacement, so it exercises a different, correctly handled interval. [Test source](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/tools/ops/write_back_cache_state_tool/lib/state_file_locator_ut.cpp)

I've changed the code - now it returns an opened file instead of a path.

2. [P1] Ancestor symlinks can be followed despite the layout checks

Location: lib/state_file_locator.cpp:120–143, 165–190

The locator checks a directory with TFileStat(..., true) and then traverses it using its pathname. These are separate operations. In this utility, true selects lstat; it does not pin the checked directory or prevent later traversal through a replaced ancestor. [Patch](https://github.com/ydb-platform/nbs/commit/28bdbea20615d9929e84030120e58dd1b54d35ed.diff)

For example, after validating state/fs-1, another process can rename that directory and replace it with a symlink to another filesystem/session hierarchy. Subsequent listing and opening follow that symlink. The final leaf can still be a regular file, and the descriptor/path inode comparison can succeed. The resulting file is then labelled using the original lexical filesystem/session names.

Fix: Traverse relative to retained directory descriptors. Open each component with appropriate directory/no-follow checks, rather than validating a string and resolving it again.

On supported Linux systems, openat2() with suitable resolution restrictions is another option. Adding O_NOFOLLOW only to the final file open is insufficient: it does not reject symlinks in earlier pathname components. openat2() documentation

Regression test: Introduce a deterministic hook between directory validation and traversal, replace the validated directory with a symlink, and verify that discovery rejects the replacement or remains anchored to the original directory. No timing-based race loop is necessary.

I consider this as a overengineering and out of scope from this PR.
We do not expect these scenarios to happen.

3. [P2] The “non-blocking” lock can hang in its constructor

Location: lib/existing_file_lock.h:27–33

The constructor opens the path with OpenExisting | RdOnly by default. LOCK_NB applies only to the later flock() operation; it does not make the preceding open non-blocking. [Lock helper source](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/tools/ops/write_back_cache_state_tool/lib/existing_file_lock.h)

If a regular file is replaced by a FIFO after the locator’s type check, opening it read-only waits for a writer. The constructor never reaches TryAcquire() or its validation. Directly constructing the helper on a FIFO has the same problem. Blocking FIFO-open behavior is explicitly documented by Linux. open() documentation

Fix: Use an opening procedure that cannot wait indefinitely on a substituted FIFO, then validate the opened descriptor’s type before locking or exposing it. Combine this with the descriptor-relative traversal above, not another pre-open stat() check.

Regression test: Exercise a FIFO with no writer in a child process with a watchdog. The expected result is a prompt structured error, not a stalled constructor.

This is an unnecessary overengineering.
The state files are residing in tmpfs so this scenario is not possible.

4. [P2] The new tests are not connected to normal Filestore build traversal

Locations: write_back_cache_state_tool/ya.make:1–4; missing registration in cloud/filestore/tools/ya.make

The PR adds recursion within the new tool subtree, but the enclosing cloud/filestore/tools/ya.make still contains only:

RECURSE(
    analytics
    testing
)

The Filestore root reaches tools, but that recursion does not reach ops/write_back_cache_state_tool. Consequently, the new tests are not discoverable through this normal tree-traversal route. [Tool build file](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/tools/ops/write_back_cache_state_tool/ya.make)

This directly contradicts the description’s claim that the top-level registration was added.

Fix: Register ops/write_back_cache_state_tool in the enclosing build graph, then verify that the ordinary Filestore CI target enumerates the locator tests—not just an explicitly selected tool test target.

Done

5. [P2] Comparing inode numbers without device identity is insufficient

Location: lib/existing_file_lock.h:49–52

The identity check compares only INode. Inode numbers are unique within a filesystem, not across filesystems. The project’s TFileStat does not retain st_dev, so this abstraction cannot currently express the necessary comparison. [Lock helper source](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/tools/ops/write_back_cache_state_tool/lib/existing_file_lock.h)

For a cross-filesystem pathname replacement—for example, an ancestor being redirected or a mount being replaced—the locked file and current pathname can have equal inode numbers while identifying different files. The check then falsely accepts the descriptor as matching the pathname.

Qualification: This is conditional on cross-filesystem replacement. An ordinary same-filesystem rename does not itself create this collision.

Fix: Compare the complete identity pair (st_dev, st_ino) using fstat and an appropriately anchored pathname lookup. Also validate that the opened descriptor is a regular file.

Regression test: A syscall-abstraction test can return matching inode numbers with different device IDs. It should reject them without requiring a privileged mount setup.

This is out of scope for this PR

6. [P2] A failed size query becomes a successful, enormous file size

Location: lib/state_file_locator.cpp:55

The result of GetLength() is converted directly to ui64. However, TFile::GetLength() is non-throwing, and its underlying implementation returns -1 when the size query fails. Thus the surrounding exception handler does not catch this failure. [Locator source](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/tools/ops/write_back_cache_state_tool/lib/state_file_locator.cpp)

On that error path, discovery succeeds and reports:

FileSize = 18446744073709551615

This breaks the result invariant: an unsuccessful inspection is represented as valid metadata rather than an error.

Fix: Check the signed result before conversion and return E_IO on failure. Alternatively, reuse size information from a successfully validated descriptor stat, avoiding the additional query.

Regression test: Inject failure into the size query after successful opening/identity validation and assert that listing fails rather than returning a maximum-sized file.

It is not expected that open succeeds but GetLength fails.
Let's not increase code complexity.

Move Assignment and RAII

The new lock wrapper has a latent ownership-transfer hazard

Location: lib/existing_file_lock.h:19–33

TExistingFileLock implicitly permits copying and moving. Its TFile member is reference-counted, and TFile declares a destructor but no move constructor or move assignment. Consequently, moving the wrapper copies its TFile member, leaving the source holding the same descriptor. The C++ rules for implicit move generation and the actual TFile declaration establish this behavior. TFile declaration

For example:

TExistingFileLock src(path, EFileLockType::Exclusive);
UNIT_ASSERT(src.TryAcquire());

{
    auto dst = std::move(src);
}

// A separately opened exclusive lock still fails:
// src continues to hold the descriptor.

Move assignment likewise releases the destination’s previous file but does not transfer exclusive ownership away from the source. Since flock ownership lasts until explicit unlock or closure of all corresponding descriptors, this can keep a file locked longer than the destination’s lifetime suggests. TFile implementation

Qualification: No current call site in this diff moves this wrapper. I would treat this as a latent API/RAII hazard, rather than claim an already exercised production failure.

Fix: Make the ownership contract explicit: either prohibit copying and moving and pass the object through an owning pointer, or implement genuine transfer. Merely explicitly defaulting its move operations does not solve the problem.

For a custom move assignment, detach the source state before destroying the destination’s old state. That avoids reading a source that destination cleanup could destroy.

The nearby hand-written TAcquireStateFileGuard assignment already exists in the parent commit, so it is not a new implementation introduced here. I did not establish a source-destruction/use-after-free path in the changed code. [Parent implementation](https://raw.githubusercontent.com/ydb-platform/nbs/73212d8/cloud/filestore/libs/vfs_fuse/persistent_state_manager.cpp)

I've removed TExistingFileLock with a simple TryLock helper.

The lock is associated with the open file and is released when its last TFile owner is destroyed. Keeping the file alive provides the RAII guard for the required lock lifetime.

Note: TFileLock cannot be reused because it currently only accepts a pathname and opens it with OpenAlways | RdOnly, it cannot reuse the TFile returned by the locator.

Test Review

The permissions test is environment-dependent

Location: lib/state_file_locator_ut.cpp:85–101

ShouldReturnErrorForUnreadableStateDirectory assumes chmod(..., 0) guarantees listing failure. A process with discretionary-access-control bypass privileges can still list that directory.

My local permission check ran as UID 0 and successfully listed it after chmod(0). Run this case in an unprivileged subprocess, inject the permission failure, or explicitly skip when the environment cannot enforce it.

This is an environment-dependent failure, not random scheduling flakiness. [Test source](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/tools/ops/write_back_cache_state_tool/lib/state_file_locator_ut.cpp)

No fix is needed

The service-side deletion fix lacks a direct regression test

The added replacement test manipulates TFileLock itself; it does not call the persistent-state manager’s deletion path. Reverting the service’s unlock/unlink reordering would therefore not make that test detect the regression.

Add a deterministic test around the manager’s deletion operation that checks the lock remains held until unlink. [Patch](https://github.com/ydb-platform/nbs/commit/28bdbea20615d9929e84030120e58dd1b54d35ed.diff)

I've reverted this change

The existing mapping test is meaningful

Its six-byte fixture is not a valid ring-buffer file, but it is testing descriptor-backed raw mapping, not format validation. Map() establishes the raw mapping separately from ValidateAndInitialize(), so this is not a nonsense fixture. [Mapping implementation](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/storage/core/libs/file_backed_containers/file_ring_buffer_accessor.cpp)

No scheduler-dependent flakiness or out-of-bounds access was demonstrated

I did not find scheduler-dependent flakiness in the existing rename/lock tests. Their operations are explicitly sequenced.

I also found no new memcpy, memmove, or memset in this patch, and no demonstrated out-of-bounds access: the indexed test accesses have preceding count/content assertions. That does not substitute for running the project’s sanitizer builds. [Patch](https://github.com/ydb-platform/nbs/commit/28bdbea20615d9929e84030120e58dd1b54d35ed.diff)

Complexity and Exception Handling

Prefer destruction to explicit unlock-and-catch machinery

The clearest simplification is in TPersistentStateManager::ReleaseStateFile(): after unlink, it explicitly unlocks, catches an exception, builds a second error, and immediately destroys the lock anyway.

The non-deleting branch already uses destruction to close the descriptor and release the lock. Using the same RAII approach after unlink would eliminate the explicit-unlock error machinery while retaining the important ordering. [Persistent-state manager implementation](https://raw.githubusercontent.com/ydb-platform/nbs/28bdbea20615d9929e84030120e58dd1b54d35ed/cloud/filestore/libs/vfs_fuse/persistent_state_manager.cpp)

Prefer structured errors for expected acquisition failures

For the new helper, expected open/stat/lock failures should preferably use the surrounding library’s structured-error convention.

Do not simply add noexcept to the existing throwing implementation. That converts errors into termination. A result-returning acquisition operation can distinguish success, contention, and inspection failure without exception-based control flow.

Avoid collecting more matches than selection needs

Selection needs to distinguish zero, one, or multiple matches. It does not need a second vector containing every matching pathname.

Verification Scope

I ran a standalone C++ reproduction, using the helper’s class body with small utility shims matching the relevant shared-descriptor and POSIX behavior. It reproduced acceptance of a pre-open replacement, retained locks after both kinds of move, ancestor-symlink redirection, and FIFO blocking.

The full NBS build and its unit tests were not run. The standalone checks support the identified mechanisms but are not substitutes for repository regression tests.

repro.cppStandalone reproducer · repro-output.txtObserved output

---

Follow-up:

Found one reproducible issue:

• [P2] Unrelated files can prevent locating the requested state file — state_file_locator.cpp:108. LocateAndOpenStateFile() inspects the entire tree before filtering. Making only fs-2/session-3 unreadable causes lookup of a valid, readable file in fs-1/session-1 to fail. Filter filesystem and session directories before inspecting files, and add a regression test for an inaccessible unrelated filesystem.

Fixed

Two simplifications are worth considering:

• Have TryLock() return TResultOrError<bool>: contention returns false; other failures return structured errors instead of throwing.

Done

• Keep one candidate and return ambiguity on the second match, avoiding the extra vector.

Done

All six existing tests passed. I found no out-of-bounds access, new move-assignment defect, or broken lock RAII. I respected the exclusions stated in the PR comments. Repository files remain unchanged.

[github-actions]

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.

• ✅ filestore

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
5044	5043	0	0	0	1	0

[github-actions]

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.

• ✅ filestore

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
5077	5076	0	0	0	1	0