[tdiff]

Notes

This is the final PR enabling garbage collection validation in selected load tests.

When enabled in loadtest config, the test collects filesystem topology and issues flush, flush_bytes, cleanup, compaction, and collect_garbage commands to all fs shards. After cleanup, the test validates that certain TStorageStats counters are zero.

It includes replacing potentially unbounded forced flush_bytes operation with repeated calls from the test runner.

Issue

#4115

[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.

• ✅ tasks + storage + fastshard
• 🚫 blockstore (cancelled or timed out before reporting completion)
• ✅ disk + manager
• 🚫 filestore (cancelled or timed out before reporting completion)

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/,cloud/fastshard/ (test time: 278s): all tests PASSED for commit 6a81a1f.

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

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 323s): all tests PASSED for commit 6a81a1f.

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

[tdiff]

Codex review:

• Reviewed 6a81a1f (validate gc in loadtests). The main concerns are the changed forced-flush contract and incomplete coverage of “no garbage left.”

Newly introduced issues / behavior changes

1. [P2] Forced FlushBytes no longer guarantees that all chunks are drained.
   tablet_actor_forced_tablet_operation.cpp:135 (/home/anton-dmitrovsky/nbs/cloud/filestore/libs/storage/tablet/tablet_actor_forced_tablet_operation.cpp:135) now completes on the first successful response. With multiple sealed chunks, later chunks remain; even the processed chunk may still be trimming. The new load-test helper compensates by polling, but
   other callers of E_FLUSH_BYTES lose the previous guarantee. The deleted ShouldDrainAllFreshBytesWithForcedFlushBytes test specifically covered this case. Preserve the draining behavior, or explicitly document and test the changed API semantics.

2. [P2] Validation can report success with large deletion markers still present.
   test.cpp:1293 (/home/anton-dmitrovsky/nbs/cloud/filestore/tools/testing/loadtest/lib/test.cpp:1293) deliberately skips LargeDeletionMarkersCount. Such markers can remain after the other checked counters reach zero, so “Storage GC stats validation passed” does not establish the proto’s stated guarantee of no remaining garbage. The TODO acknowledges this,
   but it remains a coverage hole introduced with this validator. Either wait for/process these markers within the deadline or explicitly report that validation is partial.

Pre-existing harness issues — not regressions from this commit

• Session setup failure can strand the controller. In test.cpp:371 (/home/anton-dmitrovsky/nbs/cloud/filestore/tools/testing/loadtest/lib/test.cpp:371), WaitForCompletion throws before cmd.Complete is fulfilled. The worker’s catch publishes its result and queues TTestFinished, while the controller is blocked waiting on the separate setup promise. That
exception path needs to complete the command promise too.

• Exceptional worker exit does not ensure teardown. The catch at test.cpp:497 (/home/anton-dmitrovsky/nbs/cloud/filestore/tools/testing/loadtest/lib/test.cpp:497) exits without draining requests, destroying the session, or stopping clients. Enqueuing teardown afterward cannot make the exited thread execute it. Consequently, the new validator’s assumption that
the filesystem is quiescent holds for normal teardown, but not necessarily for failures.

• Existing shutdown waits remain unbounded. test.cpp:790 (/home/anton-dmitrovsky/nbs/cloud/filestore/tools/testing/loadtest/lib/test.cpp:790) drains I/O without a deadline; existing session/filesystem waits also retain the infinite default. The new GC timeout bounds only the new validation phase.

C++ and simplification suggestions

• Make ValidateStorageStats a free helper or static member: it does not use controller state.
• Resolve the filesystem ID once in the controller; the new cleanup code duplicates the existing configuration-selection logic.
• Prefer explicit initialization over network I/O in TFileStoreCleanup’s constructor. This makes topology discovery, errors, and testing easier to follow.
• Add focused coverage for the new helper: multiple flush chunks, operation disappearance after reboot, retry/deadline handling, and shard aggregation. No such tests are added by this commit.

I found no obvious new C++ lifetime issue. Uncached statistics correctly aggregate shards, and normal teardown does drain outstanding I/O. This was a source review; git diff --check passed, but I did not build or run tests.

[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.

• 🚫 tasks + storage + fastshard (cancelled or timed out before reporting completion)
• 🚫 blockstore (cancelled or timed out before reporting completion)
• 🚫 disk + manager (cancelled or timed out before reporting completion)
• 🚫 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.

• ✅ tasks + storage + fastshard
• ✅ blockstore
• ✅ disk + manager
• ✅ filestore

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/,cloud/fastshard/ (test time: 314s): all tests PASSED for commit 5ac84d4.

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

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 2165s): all tests PASSED for commit 5ac84d4.

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
5291	5290	0	0	0	1	0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 11499s): all tests PASSED for commit 5ac84d4.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED?
8332	8305	0	0	0	27	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.

• 🚫 tasks + storage + fastshard (cancelled or timed out before reporting completion)
• 🚫 blockstore (cancelled or timed out before reporting completion)
• 🚫 disk_manager (cancelled or timed out before reporting completion)
• 🚫 filestore (cancelled or timed out before reporting completion)

[github-actions]

Note

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

Important

Workload for linux-x86_64-relwithdebinfo is not finished yet. This comment will be updated after all workloads complete.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

• ⏳ tasks + storage + fastshard
• ⏳ blockstore
• ⏳ disk_manager
• ⏳ filestore