[Filestore] unconfirmed three stage write cache bypass

[neihar]

Problem

Currently with unconfirmed writes, we respond from tablet at ExecuteTx stage. At the same time, InMemoryIndexState updates at CompleteTx stage. During this window some requests, that require file metadata can obtain stale values from that cache. The solution is simple, track all commit ids of inflight unconfirmed writes and if some TXes that clash with our inflight writes we force them to bypass the cache and go full TX flow instead of quick reply from cache.

Implemented Solution

Introduced cache bypass mechanic for inflight TXes of unconfirmed writes
Added relevant ut

Issue

#2293

[robot-vibe-db]

AI Review Summary

Verdict: ❌ 1 critical-adjacent (Major) issue found

Critical issues

No critical issues found.

Major issues

• Major | Medium: Removed error guards before ConfirmedDataAdded in Execute_AddBlob_WriteUnconfirmed — the old code skipped ConfirmedDataAdded on write failure (commit ID overflow or CheckFreshBytes error), preserving unconfirmed data in DB for recovery. The new code always calls ConfirmedDataAdded, which deletes the DB entry and releases the collect barrier even when the write was not indexed. This can lead to data loss on tablet restart after a failed write, especially during recovery when there is no client to receive the error. — tablet_actor_addblob.cpp:201

Other findings

• Minor | Medium: The reordering of Execute_AddBlob_Write before UnconfirmedAddBlobSafePointReached changes the ConfirmAddData response behavior — previously always success, now can return the actual write error. This is arguably an improvement but is a behavioral change. — tablet_actor_addblob.cpp:186
• Minor | Medium: TABLET_VERIFY(args.WriteRanges.size() == 1) is a hard tablet crash if violated. Currently safe (single caller), but fragile to future changes. — tablet_actor_addblob.cpp:194
• Minor | Medium: DeactivateInMemoryIndexStateBypass uses TABLET_VERIFY to assert FIFO ordering without a descriptive error message. — tablet_state_cache.cpp:97
• Minor | Low: CacheBypassEnabled reuses GetAddingUnconfirmedDataEnabled() config flag — no independent control for cache bypass. — tablet_actor.cpp:48
• Nit | High: CompleteTx_AddBlob condition change from ConfirmedDataRefCommitId != InvalidCommitId to Mode == WriteUnconfirmed is a good clarity improvement. — tablet_actor_addblob.cpp:578

---

This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.

[github-actions]

Note

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
3698	3697	0	1	0	0	0

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
2	1	0	1	0	0	0

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
2	1	0	1	0	0	0

[github-actions]

Note

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
3699	3699	0	0	0	0	0

[neihar]

AI Review Summary

Verdict: ❌ 1 critical-adjacent (Major) issue found

Critical issues

No critical issues found.

Major issues

• Major | Medium: Removed error guards before ConfirmedDataAdded in Execute_AddBlob_WriteUnconfirmed — the old code skipped ConfirmedDataAdded on write failure (commit ID overflow or CheckFreshBytes error), preserving unconfirmed data in DB for recovery. The new code always calls ConfirmedDataAdded, which deletes the DB entry and releases the collect barrier even when the write was not indexed. This can lead to data loss on tablet restart after a failed write, especially during recovery when there is no client to receive the error. — tablet_actor_addblob.cpp:201

Other findings

• Minor | Medium: The reordering of Execute_AddBlob_Write before UnconfirmedAddBlobSafePointReached changes the ConfirmAddData response behavior — previously always success, now can return the actual write error. This is arguably an improvement but is a behavioral change. — tablet_actor_addblob.cpp:186
• Minor | Medium: TABLET_VERIFY(args.WriteRanges.size() == 1) is a hard tablet crash if violated. Currently safe (single caller), but fragile to future changes. — tablet_actor_addblob.cpp:194
• Minor | Medium: DeactivateInMemoryIndexStateBypass uses TABLET_VERIFY to assert FIFO ordering without a descriptive error message. — tablet_state_cache.cpp:97
• Minor | Low: CacheBypassEnabled reuses GetAddingUnconfirmedDataEnabled() config flag — no independent control for cache bypass. — tablet_actor.cpp:48
• Nit | High: CompleteTx_AddBlob condition change from ConfirmedDataRefCommitId != InvalidCommitId to Mode == WriteUnconfirmed is a good clarity improvement. — tablet_actor_addblob.cpp:578

This review was generated automatically. Critical issues require attention; other findings are advisory. If this comment was useful, please give it a 👍 — it helps us improve the review bot.

Some points were really good.

[github-actions]

Note

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
3699	3699	0	0	0	0	0

[github-actions]

Note

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
3699	3699	0	0	0	0	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: 7220s): some tests FAILED for commit a005632.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
3719	3717	0	2	0	0	0

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
6	6	0	0	0	0	0