Expose BackupEngine C API additions: StopBackup and rate limiters

[rajsuvariya-stripe]

Summary

Extends RocksDB's C API with two BackupEngine capabilities needed by
language bindings (e.g. Rust via librocksdb-sys) that consume the C API:

• StopBackup: Add rocksdb_backup_engine_stop_backup() to allow cancelling
  an in-progress backup.

• Rate limiters: Add
  rocksdb_backup_engine_options_set_backup_rate_limiter() and
  rocksdb_backup_engine_options_set_restore_rate_limiter() to expose the
  shared_ptr<RateLimiter> fields on BackupEngineOptions. The existing
  uint64_t setters only throttle writes; these expose the richer RateLimiter
  object that supports read+write throttling (e.g. kAllIo mode).

Test plan

• New tests in db/c_test.c cover StopBackup and rate limiter
  setter/getter roundtrips, plus opening a real backup engine with rate
  limiters set and running a backup end-to-end
• make check passes with no regressions

[hemal-stripe]

why not use a bool here instead of the unsigned char v?

[rajsuvariya-stripe]

Good catch — <stdbool.h> is included and bool does appear in a few places (e.g. rocksdb_multi_get_entity_cf, rocksdb_load_latest_options, rocksdb_write_buffer_manager_*). However, those look like inconsistencies
introduced in newer additions rather than an intentional shift, because:

1. The file explicitly documents the convention at line 40: "Bools have the type unsigned char (0 == false; rest == true)"
2. The overwhelming majority of the file — including all existing backup engine option setters (set_share_table_files, set_sync, set_destroy_old_data, set_backup_log_files) — uses unsigned char

Staying consistent with the immediate family of backup functions and the documented convention seems like the right call here. That said, if you'd prefer we use bool to match the newer additions and move the file
toward bool uniformity, I'm happy to make that change.

[hemal-stripe]

i am fine as long as code owners are fine with the format.

[hemal-stripe]

Curious - how do we plan to use the callback_arg. The C++ api will call the callback_function without any arguments (std::function<void()>) - so at best we can track the number of times the callback function was called -- but that can be achieved other ways as well instead of passing callback args to this translation layer.

[rajsuvariya-stripe]

The C++ callback is std::function<void()> with no arguments. The C API bridges this by capturing callback_arg in a lambda that calls the C function pointer with that arg: [callback, callback_arg]]() { callback(callback_arg); }. This is the standard C idiom for stateful callbacks (same pattern as pthread_create, qsort_r, etc.) — it lets callers pass context without requiring global state. Without callback_arg, a Rust or C caller would have to use a global variable to track progress state.

[hemal-stripe]

progress_count is just incremented through the callback function, but I don't see validations if it was ever incremented or called into.

[rajsuvariya-stripe]

Fixed — added assert(progress_count > 0) after the backup call. Also set callback_trigger_interval_size = 1 when opening the backup engine so the callback fires even on the small test database (the default 4MB threshold would never be reached in a unit test).

[hemal-stripe]

can this test evolved to see if there's going to be any error in actually running the backup with provided rate limiter?

[rajsuvariya-stripe]

Fixed in the latest commit. For the rate limiter test, added a block that opens a real backup engine with both backup_rate_limiter and restore_rate_limiter set, runs a backup against the live DB, and asserts no error is returned.

[hemal-stripe]

would like to see a test for this method too.

[rajsuvariya-stripe]

Fixed in the latest commit. For stop_backup, added a call to rocksdb_backup_engine_stop_backup in the backup_with_progress_callback test after the backup completes (safe to call even when no backup is in progress, exercises the code path).

[hemal-stripe]

Hey @jaykorean - Can you help review this PR? Thanks!

[jaykorean]

@hemal-stripe I'm sorry, I currently don't have bandwidth at the moment. Added my teammates to help out.

[github-actions]

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

Completed in 3482.5s.

Summary by check

Check	Count
cert-err58-cpp	4
clang-analyzer-optin.core.EnumCastOutOfRange	4
concurrency-mt-unsafe	9
cppcoreguidelines-avoid-non-const-global-variables	5
cppcoreguidelines-pro-type-member-init	1
cppcoreguidelines-special-member-functions	1
Total	24

Details

db/blob/db_blob_direct_write_test.cc (1 warning(s))

db/blob/db_blob_direct_write_test.cc:127:7: warning: class 'ActiveBlobVisibilityWritableFile' defines a non-default destructor but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

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

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

db_stress_tool/db_stress_common.cc (4 warning(s))

db_stress_tool/db_stress_common.cc:22:25: warning: variable 'raw_env' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
db_stress_tool/db_stress_common.cc:22:25: warning: variable 'raw_env' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
db_stress_tool/db_stress_common.cc:25:56: warning: variable 'wbm' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
db_stress_tool/db_stress_common.cc:26:49: warning: variable 'rate_limiter' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

db_stress_tool/db_stress_test_base.cc (6 warning(s))

db_stress_tool/db_stress_test_base.cc:261:7: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:4025:7: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:4566:7: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:4600:11: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:4639:11: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:5373:7: warning: function is not thread safe [concurrency-mt-unsafe]

db_stress_tool/db_stress_tool.cc (4 warning(s))

db_stress_tool/db_stress_tool.cc:41:5: warning: variable 'fault_fs_for_crash_report' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
db_stress_tool/db_stress_tool.cc:402:7: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_tool.cc:416:9: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_tool.cc:448:9: warning: function is not thread safe [concurrency-mt-unsafe]

include/rocksdb/file_system.h (4 warning(s))

include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]

tools/ldb_cmd.cc (4 warning(s))

tools/ldb_cmd.cc:5469:19: warning: initialization of 'kInputFile' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
tools/ldb_cmd.cc:5470:19: warning: initialization of 'kResultFile' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
tools/ldb_cmd.cc:5537:51: warning: initialization of 'ARG_JOB_DIR' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
tools/ldb_cmd.cc:5590:50: warning: initialization of 'ARG_JOB_DIR' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]

[rajsuvariya-stripe]

@pdillinger @archang19 I have fixed the lint issues and removed the progress callback for now (I will raise a separate PR for that). Can you please review this PR now?

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

---

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

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

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit a23a485

---

Summary

Clean C API extension that correctly follows existing patterns for shared_ptr rate limiter assignment and StopBackup delegation. The main gaps are missing test coverage for rocksdb_backup_engine_stop_backup() and missing documentation about StopBackup's irreversible semantics.

High-severity findings (1):

• [db/c_test.c] rocksdb_backup_engine_stop_backup() is completely untested — new public API with no coverage.

Full review (click to expand)

Findings

🔴 HIGH

H1. Missing test for rocksdb_backup_engine_stop_backup() — db/c_test.c

• Issue: One of the three new public C API functions has zero test coverage. The test file exercises both rate limiter setters but never calls rocksdb_backup_engine_stop_backup().
• Root cause: Test was added only for rate limiter functionality but not for the stop backup feature.
• Suggested fix: Add a basic test that calls rocksdb_backup_engine_stop_backup(be) on an open BackupEngine and verifies it doesn't crash. Ideally, also verify that a subsequent rocksdb_backup_engine_create_new_backup() returns a non-OK status (Status::Incomplete).

🟡 MEDIUM

M1. StopBackup irreversibility not documented — include/rocksdb/c.h:273

• Issue: The C++ API (backup_engine.h:636-640) clearly documents that StopBackup() is a one-way operation: once called, all subsequent backup requests fail with Status::Incomplete() and a new BackupEngine instance is required. The C header has no such documentation.
• Suggested fix: Add a brief comment above the declaration:

  /* Cancels any in-progress backup. After this call, all future backup
     requests on this engine will fail; open a new engine to resume. */

M2. Rate limiter precedence over uint64_t rate limit not documented — include/rocksdb/c.h:383-390

• Issue: BackupEngineOptions has both uint64_t backup_rate_limit and shared_ptr<RateLimiter> backup_rate_limiter. When the shared_ptr is non-null, the uint64_t value is ignored. C API users who call both set_backup_rate_limit() and set_backup_rate_limiter() may be confused about which takes effect.
• Suggested fix: Add a comment noting that set_backup_rate_limiter takes precedence over set_backup_rate_limit when both are set.

M3. No restore operation tested with rate limiter — db/c_test.c

• Issue: The test sets a restore rate limiter and runs a backup, but never calls rocksdb_backup_engine_restore_db_from_latest_backup() to exercise the restore path.
• Suggested fix: Add a restore operation after the backup to verify the restore rate limiter is functional end-to-end.

🟢 LOW / NIT

L1. Null limiter silently ignored instead of clearing — db/c.cc:1472-1482

• Issue: Passing a null limiter leaves the existing rate limiter unchanged. Consistent with rocksdb_options_set_ratelimiter() pattern, but users might expect NULL to clear/reset.
• Suggested fix: Document the behavior or add an else { options->rep.backup_rate_limiter.reset(); } branch.

L2. First test block contaminates bdo with rate limiter — db/c_test.c:3700-3704

• Issue: Sets rate limiters on bdo (shared test object) before the inner block. Harmless since bdo is only used for set/get roundtrips and destroyed at end of scope.
• Suggested fix: No fix needed; cosmetic only.

Cross-Component Analysis

Context	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (backup options, not DB operations)	None
ReadOnly DB	YES	None
Concurrent writers	YES (StopBackup is atomic/thread-safe)	None
Multiple BackupEngine instances	YES (independent stop flags)	None

Ownership semantics verified: shared_ptr copy from limiter->rep to options->rep.backup_rate_limiter correctly increments reference count. Destroying the C wrapper is safe.

Positive Observations

• Implementation correctly follows the existing rocksdb_options_set_ratelimiter() pattern
• shared_ptr ownership model avoids lifetime issues at the C/C++ boundary
• End-to-end test provides functional validation beyond set/get roundtrips

---

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

[rajsuvariya-stripe]

@archang19 @pdillinger can you help with review for this PR? I have fixed some of the comments from the LLM review.

[rajsuvariya-stripe]

/claude-review

[xingbowang]

/claude-review

[meta-codesync]

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

[github-actions]

✅ Claude Code Review

Requested by @xingbowang

---

Summary

Clean, well-structured PR that correctly exposes BackupEngine C API additions. The shared_ptr ownership, thread safety, and error propagation are all correct. The rate limiter setter pattern matches the existing rocksdb_options_set_ratelimiter convention exactly.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Missing getter functions for rate limiters — include/rocksdb/c.h

• Issue: Every other BackupEngineOptions field has both a getter and setter in the C API. The new rate limiter setters have no corresponding getters.
• Root cause: Returning a rocksdb_ratelimiter_t* from a getter is non-trivial because it requires allocating a new C wrapper struct, creating ownership ambiguity (caller must free it). No existing C API getter returns a rocksdb_ratelimiter_t*.
• Suggested fix: This is a design choice rather than a bug. If getters are desired, they would need to allocate and return a new rocksdb_ratelimiter_t wrapper (caller-owned), or the C API could expose a bool getter indicating whether a rate limiter is set. Consider documenting why getters are omitted, or add them in a follow-up.

M2. No NULL-clearing semantics for rate limiter setters — db/c.cc:1471-1482

• Issue: Once a rate limiter is set, passing NULL to the setter is a no-op, so the limiter cannot be cleared back to nullptr via the C API.
• Root cause: This matches the existing rocksdb_options_set_ratelimiter pattern exactly (db/c.cc:5582-5587), so it is consistent with established convention. However, it differs from the set_env pattern which uses (env ? env->rep : nullptr).
• Suggested fix: If clearing is desired, change to options->rep.backup_rate_limiter = limiter ? limiter->rep : nullptr;. However, since the existing ratelimiter setter uses the same pattern, this may be intentional. Consider addressing across all rate limiter setters in a separate PR for consistency.

M3. Edge case test coverage gaps — db/c_test.c

• Issue: Tests cover the happy path well (set limiter, open engine, backup, restore) but miss edge cases: calling stop_backup multiple times, passing NULL limiter to setters, and verifying stop_backup doesn't affect restore operations.
• Suggested fix: Add: (1) rocksdb_backup_engine_stop_backup(be); rocksdb_backup_engine_stop_backup(be); to verify idempotency, (2) rocksdb_backup_engine_options_set_backup_rate_limiter(bdo, NULL); to verify no crash on NULL.

🟢 LOW / NIT

L1. Error masking in test cleanup — db/c_test.c:3724

• Issue: rocksdb_destroy_db(options, rate_restore_path, &err); err = NULL; silently discards any error from destroy_db.
• Suggested fix: Use CheckNoError(err); or at minimum free(err); err = NULL; to avoid leaking the error string.

L2. Test sets limiters on bdo without exercising them — db/c_test.c:3683-3685

• Issue: The first test block sets rate limiters on bdo then destroys the limiter wrapper, but bdo is never used to open an engine. This only verifies the setter doesn't crash, not that the options are functional.
• Suggested fix: Acceptable as a smoke test; the second block (rate_beo) exercises the full path. No action needed.

Cross-Component Analysis

Context	Affected?	Assessment
BackupEngineReadOnly	No	StopBackup not applicable; rate limiters only used during backup/restore
Multiple BackupEngine instances	No	Each instance has independent stop_backup_ flag and options
Concurrent stop + backup	Safe	Atomic memory_order_release/acquire on stop_backup_ flag
shared_ptr lifetime	Safe	Copy into BackupEngineOptions increments refcount; C wrapper destruction only decrements it
Rate limiter override of uint64_t	Verified	backup_engine.cc constructor checks if (options_.backup_rate_limiter == nullptr && options_.backup_rate_limit > 0)

Positive Observations

• Correct ownership model: The shared_ptr copy into BackupEngineOptions ensures the RateLimiter stays alive even after rocksdb_ratelimiter_destroy is called on the C wrapper. The test correctly validates this pattern.
• Good documentation: The c.h comments accurately describe one-way StopBackup semantics and rate limiter override behavior.
• Consistent patterns: The implementation follows established C API conventions (if (limiter) guard, SaveError for error propagation, ROCKSDB_LIBRARY_API on declarations).
• Thorough integration test: The rate limiter test creates a full engine, performs backup and restore, validating end-to-end functionality.

---

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

[xingbowang]

Please fix build failure

[rajsuvariya-stripe]

@xingbowang I have fixed the two issues which were causing the build failures, can you rerun these builds?

1. Format-truncation: Buffer for rate_restore_path was 200 bytes but snprintf could write up to 213 bytes — fixed by increasing to 220.
2. c_test crash: CreateNewBackupWithMetadata had no early stop check, so calling CreateNewBackup after StopBackup proceeded into SstFileDumper which hit an assertion on an unregistered custom comparator — fixed
   by adding ShouldStopBackup() guard at the top of the function.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 6dddbad

---

❌ 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 6dddbad

---

Summary

Clean, well-implemented PR that exposes existing BackupEngine functionality through the C API. The code follows established patterns exactly and introduces no correctness or performance concerns. The early ShouldStopBackup() check in CreateNewBackupWithMetadata() is a sensible addition.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. StopBackup declaration placement in c.h — include/rocksdb/c.h

• Issue: The rocksdb_backup_engine_stop_backup declaration is placed between rocksdb_backup_engine_info_destroy (info section) and rocksdb_backup_engine_close. This separates it from the other rocksdb_backup_engine_* operational functions (create_new_backup, purge_old_backups, etc.) declared earlier (~lines 239-252).
• Suggested fix: Consider grouping with other backup engine operations for discoverability. Minor organizational nit.

M2. rocksdb_destroy_db error silently ignored in test — db/c_test.c

• Issue: rocksdb_destroy_db(options, rate_restore_path, &err) is followed by err = NULL without CheckNoError(err). If destroy fails, the error is silently swallowed.
• Suggested fix: Add CheckNoError(err) or a comment explaining why failure is acceptable.

🟢 LOW / NIT

L1. No test for passing NULL limiter — db/c_test.c

• Issue: The if (limiter) guard silently ignores NULL. No test documents this contract. Adding rocksdb_backup_engine_options_set_backup_rate_limiter(bdo, NULL) would verify the no-crash behavior.

L2. StopBackup test could verify error message — db/c_test.c

• Issue: Test only checks err != NULL. A strstr(err, "Incomplete") check would be more robust.

L3. Rate limiter setters only set, never clear — db/c.cc

• Issue: Passing NULL is a no-op. Consistent with rocksdb_options_set_ratelimiter — by design, not a bug.

Cross-Component Analysis

Concern	Assessment
shared_ptr lifetime after wrapper destroy	Safe — shared_ptr copy increments refcount
StopBackup on read-only engine	N/A at C API level — C API opens read-write
Early ShouldStopBackup vs GarbageCollect	No issue — early check prevents starting work
Concurrent StopBackup + CreateNewBackup	Safe — atomic with proper memory ordering
Performance impact	None — single atomic load on cold path

Positive Observations

• Perfect adherence to existing C API patterns
• Good c.h documentation explaining one-way StopBackup behavior
• Comprehensive end-to-end test: backup + restore with rate limiters
• Correct shared_ptr lifetime management in tests

---

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

[meta-codesync]

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

[meta-codesync]

@xingbowang merged this pull request in 0493154.