Fix flaky preallocation tests on btrfs, zfs, tmpfs, and overlayfs

[pdillinger]

Summary:
Fix flaky test failures in EnvPosixTestWithParam.AllocateTest and DBWALTest.TruncateLastLogAfterRecoverWithFlush that occur when running on filesystems where preallocated space is not reliably reflected in st_blocks.

The tests use stat() to check st_blocks and verify that fallocate with FALLOC_FL_KEEP_SIZE actually preallocates disk space. However, on certain filesystems, this check is unreliable:

1. btrfs and zfs: Copy-on-write filesystems where preallocated extents are not reliably reflected in st_blocks, especially under load.

2. tmpfs: Memory filesystem that may not report preallocated blocks in st_blocks.

3. overlayfs: Union filesystem common in containers that may not pass through fallocate properly or report preallocated space.

4. Any filesystem where FALLOC_FL_KEEP_SIZE is not supported: The preallocation will fail silently (error ignored with PermitUncheckedError), leaving only the written data in st_blocks.

Changes made:

env/env_test.cc:

• Added filesystem magic number definitions for TMPFS_MAGIC, OVERLAYFS_SUPER_MAGIC, and ZFS_SUPER_MAGIC
• Extended the AllocateTest to skip block count checks on zfs, tmpfs, and overlayfs in addition to btrfs
• Added runtime fallback: if st_blocks is less than expected, print a warning and skip the check instead of failing. This handles unknown filesystems or configurations where preallocation isn't supported.

db/db_wal_test.cc:

• Added includes and filesystem magic number definitions
• Added ShouldSkipAllocationCheck() helper function to detect problematic filesystems
• Modified TruncateLastLogAfterRecoverWithoutFlush, TruncateLastLogAfterRecoverWithFlush, TruncateLastLogAfterRecoverWALEmpty, and ReadOnlyRecoveryNoTruncate tests to skip allocation checks on problematic filesystems
• Added runtime fallback checks similar to env_test.cc

These changes make the tests robust against filesystem differences while still validating preallocation behavior on filesystems where it works correctly (ext4, xfs).

Test Plan:
Many local 'make -j100 check' runs that would previously fail with good probability.

[github-actions]

⚠️ clang-tidy: 1 warning(s) on changed lines

Completed in 176.2s.

Summary by check

Check	Count
cppcoreguidelines-pro-type-member-init	1
Total	1

Details

db/db_wal_test.cc (1 warning(s))

db/db_wal_test.cc:85:5: warning: uninitialized record type: 'fs_stat' [cppcoreguidelines-pro-type-member-init]

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D105331256.

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D105331256.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 611b91a

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 611b91a

---

Summary

Sound fix for flaky preallocation tests on non-ext4 filesystems. Test-only change, no production code affected. The approach is consistent with the existing btrfs skip pattern and correctly extends it to zfs, tmpfs, and overlayfs.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. %lu format specifier for uint64_t / size_t — db/db_wal_test.cc

• Issue: The warning messages use %lu to print uint64_t allocated and size_t preallocated_size. While this works on 64-bit Linux (where unsigned long is 64 bits), it is technically incorrect for uint64_t which should use PRIu64, and non-portable for size_t which should use %zu. RocksDB uses PRIu64 throughout (200+ occurrences).
• Suggested fix: Use PRIu64 for uint64_t values and %zu for size_t values. Requires #include <cinttypes>.

M2. Code duplication of filesystem magic numbers and skip logic

• Issue: Filesystem magic number #defines are now duplicated in 4+ files: env/env_test.cc, db/db_wal_test.cc, env/io_posix.cc, env/fs_posix.cc, env/env_posix.cc. The skip check logic also differs in style between the two test files (inline boolean in env_test.cc vs helper method in db_wal_test.cc).
• Suggested fix: Consider extracting a common header with the magic number definitions. Not blocking for this PR but would reduce maintenance burden.

M3. Preprocessor guard inconsistency between files

• Issue: In env/env_test.cc, magic numbers are defined with #if !defined(X) (no OS_LINUX guard), matching existing convention (env/fs_posix.cc, env/env_posix.cc). In db/db_wal_test.cc, they use #if defined(OS_LINUX) && !defined(X). The extra OS_LINUX guard on the definitions is unnecessary since the macros go unused on non-Linux anyway.
• Suggested fix: Follow existing convention — use #if !defined(X) without OS_LINUX guard on macro definitions.

🟢 LOW / NIT

L1. Runtime fallback could theoretically mask real bugs on ext4

• Issue: When allocated < preallocated_size on an unknown filesystem, the test skips. Acceptable pragmatic choice — ext4 reliably reflects preallocation, so real regressions won't be masked there.

L2. Consider ROCKSDB_GTEST_BYPASS when skipping

• Issue: Skipped tests still report PASSED. Using ROCKSDB_GTEST_BYPASS would improve test output clarity. The fprintf(stderr) approach is consistent with existing btrfs pattern though.

Positive Observations

• Conservative approach: adds detection rather than removing assertions. On ext4/xfs, all assertions remain enforced.
• ShouldSkipAllocationCheck helper is well-structured.
• Magic number values verified correct against existing definitions in env/io_posix.cc, env/fs_posix.cc, env/env_posix.cc.
• ReadOnlyRecoveryNoTruncate correctly skips both initial and post-reopen assertions consistently.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase