Ignore OPTIONS files for manifest recovery advance (#15106)

Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15106

When `optimize_manifest_for_recovery` is enabled, `MaybeUpdateNextFileNumber` uses on-disk files to decide whether recovery must append a `SetNextFile` edit. `OPTIONS-*` and temp options files are written as sidecar option snapshots during open, after recovery, so their presence alone should not force MANIFEST growth on a later clean reopen.

Exclude `kOptionsFile` and `kTempFile` from the trigger that requires a MANIFEST file-number advance. If any real file requires an advance, the code still advances past the largest observed file number, including OPTIONS files.

Also tighten db_stress strict MANIFEST verification so fatal zero-growth checks only run when all fault injection is disabled. Open write/sync fault injection can legitimately make `reuse_manifest_on_open` fall back to a fresh MANIFEST.

Reviewed By: xingbowang

Differential Revision: D116092705

fbshipit-source-id: 65b57b0b7f57b2cef42efc4df353fb2218688d34

db/db_basic_test.cc

@@ -355,6 +355,36 @@ TEST_F(DBBasicTest,
             synthetic_number);
 }
 
+TEST_F(DBBasicTest,
+       OptimizeManifestForRecoveryIgnoresOptionsFilesForNextFileNumber) {
+  Options options = CurrentOptions();
+  options.create_if_missing = true;
+  options.optimize_manifest_for_recovery = true;
+  DestroyAndReopen(options);
+  ASSERT_OK(Put("k", "v"));
+  ASSERT_OK(Flush());
+
+  const uint64_t next_before_close =
+      dbfull()->GetVersionSet()->current_next_file_number();
+  Close();
+
+  const uint64_t options_number = next_before_close + 100;
+  ASSERT_OK(WriteStringToFile(env_, "" /*data*/,
+                              dbname_ + "/" + OptionsFileName(options_number),
+                              /*should_sync=*/true));
+  ASSERT_OK(WriteStringToFile(env_, "" /*data*/,
+                              TempOptionsFileName(dbname_, options_number + 1),
+                              /*should_sync=*/true));
+
+  RecoveryOptimizationCounters counters;
+  counters.Install();
+  Reopen(options);
+  counters.Uninstall();
+
+  ASSERT_EQ(1, counters.next_file_number.load());
+  ASSERT_EQ("v", Get("k"));
+}
+
 // optimize_manifest_for_recovery=true: after a clean Put + Flush + Close, the
 // next Open's min_log_number_to_keep equals (max_wal_before_close + 1),
 // proving the close-time MANIFEST write took effect.

db/db_impl/db_impl_files.cc

@@ -1211,7 +1211,9 @@ Status DBImpl::MaybeUpdateNextFileNumber(RecoveryContext* recovery_ctx) {
       // Use >= so a crashed-mid-allocation file at exactly
       // prev_next_file_number triggers the advance -- otherwise the next
       // NewFileNumber() would collide with it.
-      if (number >= prev_next_file_number) {
+      const bool must_reserve_file_number =
+          type != kOptionsFile && type != kTempFile;
+      if (must_reserve_file_number && number >= prev_next_file_number) {
         on_disk_file_advanced_counter = true;
       }
       largest_file_number = std::max(largest_file_number, number);

db_stress_tool/db_stress_test_base.cc

@@ -5721,7 +5721,9 @@ void StressTest::RecordManifestStateBeforeReopen() {
   if (reuse_manifest && optimize_manifest) {
     // Check if ALL conditions for complete avoidance are met.
     // If so, use STRICT mode where failures are fatal.
+    const bool no_fault_injection = !NeedsFaultInjection();
     const bool no_manifest_writes_expected =
+        no_fault_injection &&
         FLAGS_avoid_flush_during_recovery &&  // No flush during recovery
         !FLAGS_write_dbid_to_manifest;        // No DB_ID write on open
     // Note: avoid_flush_during_shutdown is NOT required for STRICT mode.

db_stress_tool/db_stress_test_base.h

@@ -429,8 +429,7 @@ class StressTest {
     // - Not in best_efforts_recovery mode
     // - avoid_flush_during_recovery=true (no flush during recovery)
     // - write_dbid_to_manifest=0 (no DB_ID write on open)
-    // - metadata_write_fault_one_in=0 (no fault injection)
-    // - open_metadata_write_fault_one_in=0 (no fault injection)
+    // - all fault injection flags are disabled
     // - MANIFEST not corrupted, not at size limit
     // Note: avoid_flush_during_shutdown is NOT required. If it leaves data
     // in WAL but avoid_flush_during_recovery=true prevents flushing it,