Add experimental external log file API

[xingbowang]

Implement the experimental ExternalLogFileManager API and RocksDB integration for MANIFEST-tracked application byte streams. This folds the public API header and implementation into one diff for easier export to GitHub.

What this adds:

• Public API surface: ExternalLogFileManager, ExternalLogFileReader, ExternalLogFileWriter, ExternalLogFileOptions, IngestExternalLogFileOptions, ExternalLogFileInfo, reader visibility options, checksum verification options, reopen recovery options, and seal options.
• DB integration: DB::NewExternalLogFileManager() and StackableDB forwarding.
• MANIFEST integration: external-log metadata encoding/decoding, VersionEdit records, VersionSet tracking, recovery handling, manifest roll preservation, and file-number reservation/protection.
• File enumeration integration: external log files participate in live-file/storage-info paths where supported, including FileStorageInfo fields and kExternalLogFile type support.
• Docs and release note for the experimental feature.

Public API contract:

• Purpose: external log files are MANIFEST-tracked byte streams for application data. They let an application keep a consistent view of RocksDB state plus related application files without storing those bytes as keys or values, and they allow efficient sequential file read/write through the configured RocksDB FileSystem.
• External log files are not RocksDB WALs, SSTs, or record containers. RocksDB does not replay their bytes into memtables, assign sequence numbers to their contents, define record boundaries, or interpret application data.
• Each file has an application-provided name used by ListExternalLogFiles(), OpenExternalLogFileForRead(), ReopenExternalLogFile(), VerifyExternalLogFileChecksum(), DeleteExternalLogFile(), and DeleteExternalLogFiles(). The name is both the public lookup key and the physical path persisted in MANIFEST; there is no separate public path field.
• In kDBRelativePath mode, the name is the DB-relative physical path. Absolute paths, backslash separators, empty components, . components, .. components, and names that collide with RocksDB-managed filenames are rejected.
• In kExternalPath mode, the name is an explicit physical path. Absolute names are used as-is. Relative names are resolved against the DB directory and may include .. components. This keeps outside-DB tracking explicit through path_type without requiring a second application-provided path string. Reopen/restore require the application to recreate the same external path names before opening the DB.
• CreateExternalLogFile() creates an unsealed file at the resolved physical path and registers it in MANIFEST at creation time. Sync() makes bytes durable in the FileSystem but does not update MANIFEST size/checksum metadata. Seal() records the final logical size and checksum metadata in MANIFEST and makes the file immutable.
• ReopenExternalLogFile() is the recovery path for an unsealed file. If the physical file is longer than the MANIFEST durable prefix, the application chooses the recovered append point; RocksDB can recompute rolling checksum state when a checksum factory is configured.
• IngestExternalLogFile() and IngestExternalLogFiles() register prebuilt immutable files as sealed external log files. If source_path already equals the resolved destination path, RocksDB validates and registers the existing file in place; otherwise it copies, moves, links, or link-or-copies according to the ingestion mode. Multi-file ingest commits all MANIFEST additions atomically after the physical transfers complete. RocksDB does not infer application record boundaries.
• Readers see the committed logical prefix. For unsealed files with an active writer, snapshot readers capture the writer-visible size at open time and follow readers can observe later successful appends. For closed unsealed files without an active writer, readers expose the MANIFEST durable prefix until the application reopens, recovers, and seals the file.
• DeleteExternalLogFile() removes one MANIFEST entry and deletes the physical file named by the MANIFEST entry. DeleteExternalLogFiles() removes multiple MANIFEST entries with one edit and then deletes the physical files. Both APIs reject duplicate names and return Busy without changing MANIFEST if any requested file has an active reader, writer, verifier, or physical-size listing reference. After a successful MANIFEST edit, RocksDB attempts every physical delete and ignores NotFound results.
• DeleteExternalLogFile() and DeleteExternalLogFiles() are the only RocksDB APIs that delete explicit outside-DB external log files. Obsolete-file scanning does not discover or delete those paths indirectly; the MANIFEST entry is the authority. The delete path uses RocksDB rate-limited unaccounted purge machinery when configured, and directory sync/trash behavior uses each physical file's parent directory.
• DestroyDB() does not delete explicit external-path files outside the DB directory. Applications must use DeleteExternalLogFile(), DeleteExternalLogFiles(), or their own cleanup for those files.
• Checkpoint returns NotSupported while any external log file is registered, rather than silently creating a checkpoint with missing application files.
• BackupEngine returns NotSupported while any external log file is registered and BackupEngine cannot preserve it. Applications using backup/restore must preserve the external files separately and recreate the same paths before opening the restored DB.
• DB::GetLiveFiles() returns NotSupported for kExternalPath files because that API can only report DB-relative names. DB::GetLiveFilesStorageInfo() can report directory/filename metadata for external log files.
• Thread safety: RocksDB synchronizes internal metadata, reference counts, and individual writer handles. A single writer handle serializes its mutating calls for append position, checksum state, file I/O, and closed/sealed state. RocksDB permits multiple unsealed files to be active concurrently, with at most one active writer per file. Applications must keep handles alive while in use and synchronize higher-level lifecycle decisions and record-boundary protocols.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 497.4s.

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 48c271d

---

Summary

This PR adds a well-structured external log file management system for RocksDB-managed .xlog byte streams with MANIFEST persistence, lifecycle management, and integration with checkpoints and obsolete-file cleanup. The core abstractions (writer, reader, manager, seal semantics) are sound and follow RocksDB patterns. However, the review identified several issues that should be addressed before merging.

High-severity findings (5):

• [db/external_log_file_impl.cc] Checksum generator state can be irreversibly lost if PersistMetadataLocked fails after Finalize() + reset(), preventing retry.
• [db/db_filesnapshot.cc] External log files included in GetLiveFiles() will be picked up by BackupEngine despite being "explicitly unsupported," causing silent backup of unrestorable data or backup failures.
• [db/external_log_file_impl.cc] TOCTOU race in ReopenExternalLogFile: a reader can register between HasActiveReaders() returning false and Truncate() executing, causing the reader to observe a shorter file than its visible_size.
• [include/rocksdb/utilities/stackable_db.h] Missing NewExternalLogFileManager() override in StackableDB — all stacked DB wrappers (TTL, Transaction, etc.) will fail to compile or silently fail at runtime.
• [db/version_edit.h] New MANIFEST tags for external log file addition/deletion must be assigned values after kTagSafeIgnoreMask (>= 8192) to ensure forward compatibility. If placed before the mask, older RocksDB versions will refuse to open the MANIFEST.

Full review (click to expand)

Findings

🔴 HIGH

H1. Checksum generator state lost on MANIFEST write failure — db/external_log_file_impl.cc

• Issue: In ChecksumForMetadata(), when durable_size == visible_size and the rolling checksum generator is available, the code calls Finalize() and then checksum_generator_.reset(). If the subsequent LogAndApply() in PersistMetadataLocked() fails, the generator is already destroyed and cannot be reconstructed without re-reading the entire file.
• Root cause: Irreversible state mutation (finalize + reset) occurs before the operation that depends on it (MANIFEST write) is confirmed.
• Suggested fix: Defer checksum_generator_.reset() until after LogAndApply() succeeds. Alternatively, clone the generator state before finalizing so it can be restored on failure.

H2. BackupEngine will silently include external log files — db/db_filesnapshot.cc

• Issue: The PR adds external log files to GetLiveFiles() and GetLiveFilesStorageInfo(). BackupEngine calls GetLiveFiles() to determine which files to back up. Despite the PR stating BackupEngine support is "explicitly unsupported," external log files will be included in backups. During restore, these files would be copied but lack proper management integration.
• Root cause: ValidateExternalLogFileBackupSupport() is only called on the ingest path, not in BackupEngine's file enumeration.
• Suggested fix: Either (a) exclude kExternalLogFile from GetLiveFiles() output and provide a separate enumeration API, or (b) add kExternalLogFile filtering in BackupEngine's file copy logic, or (c) make BackupEngine explicitly skip external log files with a documented warning.

H3. TOCTOU race in ReopenExternalLogFile truncation — db/external_log_file_impl.cc

• Issue: ReopenExternalLogFile calls HasActiveReaders() (acquires/releases DB mutex) then later calls file->Truncate() (no mutex held). A reader can register between these two operations via OpenExternalLogFileForRead, which also acquires the DB mutex independently. The new reader would have a visible_size based on MANIFEST metadata (pre-truncation durable_size), but the physical file may already have data beyond durable_size that gets truncated. If the reader's snapshot_size_ was set from the handle_state's visible_size_ (which the writer updates), the reader could attempt to read beyond the new file end.
• Root cause: The check-then-act pattern across two separate mutex acquisitions.
• Suggested fix: Hold the DB mutex across both HasActiveReaders() and the truncation decision, or perform truncation before releasing the writer registration lock and before any reader can observe the file.

H4. Missing StackableDB::NewExternalLogFileManager() — include/rocksdb/utilities/stackable_db.h

• Issue: StackableDB implements all DB virtual methods by delegation to the underlying db_ pointer. The new NewExternalLogFileManager() virtual method needs a corresponding forwarding implementation. Without it, any stacked DB (TransactionDB, DBWithTTL, etc.) will either fail to compile or use a default implementation that returns an error.
• Root cause: Incomplete integration with the DB wrapper layer.
• Suggested fix: Add Status NewExternalLogFileManager(std::unique_ptr<ExternalLogFileManager>* m) override { return db_->NewExternalLogFileManager(m); } to StackableDB.

H5. MANIFEST tag forward compatibility — db/version_edit.h

• Issue: The new kExternalLogFileAddition and kExternalLogFileDeletion tags must be assigned values with the kTagSafeIgnoreMask (1 << 13 = 8192) bit set. Tags without this bit cause older RocksDB versions to return Status::Corruption when encountering them, making the MANIFEST unreadable and the database unopenable.
• Root cause: The diff is truncated so we cannot verify the actual tag values, but this is a critical constraint that must be validated.
• Suggested fix: Verify tags are assigned as kTagSafeIgnoreMask | N (e.g., 8193, 8194). Add a static_assert or comment documenting the requirement.

🟡 MEDIUM

M1. DestroyDB skips external log files — db/db_impl/db_impl.cc:5937

• Issue: External log files are skipped during DestroyDB with a continue, leaving .xlog files behind. This can prevent directory removal and confuse users who expect DestroyDB to fully clean up. If a new DB is created in the same directory, stale .xlog files remain.
• Suggested fix: Add a DestroyOptions parameter or delete external log files by default with clear documentation.

M2. Double-counting in FindObsoleteFiles — db/db_impl/db_impl_files.cc

• Issue: Both MANIFEST-tracked files and active_external_log_file_refs_ entries are pushed to external_log_live without deduplication. A file present in both sets appears twice.
• Suggested fix: Use an unordered_set for external_log_live or deduplicate before use. Low practical impact since downstream unordered_set handles duplicates.

M3. trim_to_size for unsealed files in checkpoint — db/db_filesnapshot.cc:356

• Issue: GetLiveFilesStorageInfo sets trim_to_size = true and size = metadata.GetDurableSize() for all external log files. For unsealed files, durable_size may be 0 (since Sync doesn't persist to MANIFEST). A checkpoint would copy 0 bytes.
• Suggested fix: Skip unsealed files in GetLiveFilesStorageInfo, or persist durable_size during Sync.

M4. Ingest rollback error masking — db/external_log_file_impl.cc

• Issue: When ingest fails and rollback also fails, the error is wrapped via Status::CopyAppendMessage(), losing the structured error type of the original failure.
• Suggested fix: Log the rollback failure separately and return the original error.

M5. Space accounting bypass — db/db_impl/db_impl_files.cc

• Issue: External log files use DeleteUnaccountedDBFile(), bypassing SstFileManager space tracking. Large external log files won't count toward disk space limits.
• Suggested fix: Consider tracking in SstFileManager or providing separate accounting.

M6. Writer destructor ordering — db/external_log_file_impl.cc

• Issue: Destructor calls file_->Close() then UnregisterActiveWriter(). Between unregistration completing and destructor finishing, another writer could register. While the new writer opens its own file handle, the ordering is fragile.
• Suggested fix: Unregister first, then close. Or document why current order is safe.

🟢 LOW / NIT

L1. Local encoding helpers could be shared — db/external_log_file_edit.cc

• PutLengthPrefixedVarint32Field / PutLengthPrefixedVarint64Field are in an anonymous namespace. Consider util/coding.h if reused.

L2. Weak_ptr lazy cleanup — db/external_log_file_impl.cc

• Expired entries in active_external_log_writers_ accumulate until accessed. Minor concern for long-running processes.

L3. Silent deletion of missing files — db/external_log_file_edit.cc

• ExternalLogFileSet::DeleteFile() silently ignores missing names. Intentional for idempotent replay, but should be documented.

L4. Encoding pattern inconsistency — db/external_log_file_edit.cc

• String vs numeric fields use different encoding helpers. Both decode identically but adds cognitive load. Add a comment explaining the pattern.

Cross-Component Analysis

Context	Executes?	Assumptions hold?	Action needed?
ReadOnly DB	Yes (read ops)	Yes — CheckDBMutable() blocks writes	Safe
Secondary Instance	Needs verification	Unknown	Verify behavior
WritePreparedTxnDB	Yes (independent)	Yes — no txn interaction	Safe
BackupEngine	Yes (via GetLiveFiles)	NO — files included but unsupported	H2
Checkpoint	Yes (via GetLiveFilesStorageInfo)	Partially — M3 for unsealed	Fix trim_to_size
FIFO/Universal compaction	No interaction	N/A	Safe

Positive Observations

• Clean state machine design: unsealed→sealed lifecycle with immutability well-enforced in ExternalLogFileSet::AddFile().
• Comprehensive decode validation with clear error messages and corruption detection.
• Proper pending_outputs integration prevents file number reuse after crashes.
• Good error path cleanup via cleanup_active_writer lambda pattern.
• TEST_SYNC_POINT hooks at key lifecycle points enable thorough concurrency testing.
• Rate-limited deletion via DeleteScheduler follows established patterns.

---

ℹ️ 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

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 5c3ac5f

---

❌ 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]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit a0a4073

---

❌ 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 a0a4073

---

Summary

This PR adds a substantial new feature (~5800 lines) introducing ExternalLogFileManager for RocksDB-managed .xlog byte streams. The MANIFEST integration, serialization format, and state machine design are generally sound. However, the review identified issues with code duplication, Checkpoint subdirectory handling, potential ABI concerns, and the mutex-unlock-on-error pattern in GetLiveFiles.

High-severity findings (3):

• [db/db_filesnapshot.cc] Checkpoint will fail for external log files whose DB-relative name contains subdirectories (e.g., subdir/file.xlog) because CheckpointImpl does not create intermediate directories.
• [db/db_filesnapshot.cc + db/external_log_file_impl.cc] Path utility functions (IsAbsolutePath, JoinPath, ParentPath, BaseName) are duplicated verbatim between two files with different name prefixes. Should be extracted to a shared utility.
• [include/rocksdb/db.h] DB::NewExternalLogFileManager must not be a pure virtual method — it would break all existing DB subclasses. Needs a default implementation returning Status::NotSupported.

The full review with all 15 findings (3 HIGH, 7 MEDIUM, 5 LOW), cross-component analysis table, and positive observations has been written to review-findings.md.

---

ℹ️ 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