issue-7344: [Filestore] Persist node unlink completion mode in OpLog

[ruslan-gar]

Notes

1. Persist the unlink completion mode in TUnlinkNodeInShardRequest and use it in TIndexTabletActor and TUnlinkNodeInShardActor to ensure correct oplog replay after configuration changes.
2. Preserve backward compatibility by deriving the mode for legacy entries where the field is absent.

Issue

#7344

[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: 6840s): all tests PASSED for commit 4754824.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
4890	4888	0	0	0	2	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 (cancelled or timed out before reporting completion)

[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 (cancelled or timed out before reporting completion)

[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: 7162s): all tests PASSED for commit 6ed7fe7.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
4890	4888	0	0	0	2	0

[ruslan-gar]

GPT-5.6 Sol:

I reviewed the PR description, the existing review locations/comments exposed by GitHub, and the final tree at HEAD 6ed7fe7 only. The earlier review threads on service_ut_sharding.cpp, tablet.proto, tablet_actor_oplog.cpp, and tablet_actor_renamenode_destination.cpp are marked resolved/outdated, and the final HEAD includes the follow-up commits addressing them. GitHub

Findings

No confirmed bugs
I did not find a confirmed correctness, memory-safety, concurrency, overflow, or error-handling bug in the latest revision.
The central state transition is internally consistent:

• Normal external unlink now persists ShouldUnlockUponCompletion at the time the OpLog entry is created, rather than recomputing it from current filesystem configuration. GitHub
• Replay uses the persisted value; for legacy entries without the field, it derives the old behavior from OriginalRequest plus the current configuration. GitHub
• RenameNode explicitly persists false, which is important because those requests intentionally have no OriginalRequest and must not enter the post-unlink completion path. GitHub
• The completion path still keeps the NodeRef locked only for the mode that requires the second transaction, and the existing error path that originally motivated [Filestore] NodeRef stays locked until tablet restart after InvalidNodeRefUponCompleteUnlinkNode upon OpLog replay right after enabling DirectoryCreationInShardsEnabled #7344 is avoided because a pre-switch unlink now replays with its persisted false mode rather than incorrectly entering CompleteUnlinkNode. GitHub
• The added tests exercise both the configuration-switch scenario and legacy OpLog entries with the new field absent. GitHub
  I also checked the suspicious CompleteUnlinkNode error path where the in-memory NodeRef is only unlocked on success. That looks dangerous in isolation, but it is the pre-existing mechanism described by issue [Filestore] NodeRef stays locked until tablet restart after InvalidNodeRefUponCompleteUnlinkNode upon OpLog replay right after enabling DirectoryCreationInShardsEnabled #7344, not a regression introduced by this PR; the latest change specifically prevents the bad replay scenario that reaches it after the NodeRef was already removed. GitHub
  Question / invariant worth documenting
  There is one deployment invariant I would explicitly verify with the maintainers, but I would not report it as a bug:
  In cloud/filestore/libs/storage/tablet/tablet_actor_oplog.cpp, replay of a legacy TUnlinkNodeInShardRequest without ShouldUnlockUponCompletion derives the value from the current DirectoryCreationInShardsEnabled setting. GitHub

The code comments state that a version change and enabling directory creation in shards are manual changes and “should not occur simultaneously” with an OpLog entry sitting between write and replay. GitHub
The question is whether that is guaranteed operationally during a rolling upgrade: an old binary can still create a legacy entry, and that entry could theoretically survive until a new binary replays it after configuration has changed. The compatibility logic is correct under the stated invariant, but the invariant itself is not enforced by the code. The PR description also explicitly relies on this compatibility strategy. GitHub
Severity: question, not a confirmed bug.
Overall, based on the final HEAD, I would not leave a blocking/important/minor bug comment. The remaining concern is documenting or validating the rollout invariant above.

[ruslan-gar]

The question is whether that is guaranteed operationally ...

It is guaranteed operationally.