Bug fixes: surface embedded-blob resolution errors (#14906)

Summary:
Experimental embedded ("same-file") blob SST support (https://github.com/facebook/rocksdb/issues/14851) overloads blob file number 0 as both kInvalidBlobFileNumber and the same-file sentinel kCurrentFileBlobIndexFileNumber. Same-file references must be resolved by EmbeddedBlobResolvingIterator before they reach any generic file-metadata or integrated-BlobDB path, all of which reject file number 0 as invalid.

When resolving a same-file blob hit an error (e.g. a db_stress-injected blob-region read fault), EmbeddedBlobResolvingIterator::value() fell back to returning the RAW, unresolved same-file value, surfacing the error only via status()/Valid(). Compaction consumed the raw value before consulting status, leaking an unresolved same-file reference into the output stream. Depending on the value type this produced two different crash-test failures, both from this one root cause:

* T277566778 -- wide-column entity variant. The raw same-file entity (kTypeWideColumnEntity) reached FileMetaData::UpdateBoundaries, whose blob-ref scan (correctly) rejected file number 0: Flush failed: Corruption: Invalid blob file number (also observed from a background compaction). The tripwire fired on the leak, so the real injected read error was masked behind a misleading corruption status.

* T277310719 -- whole-value BlobIndex variant, and more dangerous because it escapes that tripwire. ResolveKeyType() rewrites the key type kTypeBlobIndex -> kTypeValue with no I/O (so it always succeeds); on the masked error the emitted entry is therefore {kTypeValue, raw BlobIndex bytes}. UpdateBoundaries only scans kTypeBlobIndex/entity types, so it does NOT reject this record: the corruption is silently written to the compaction output and persists. A later point lookup reads those raw BlobIndex bytes back as the value, which db_stress detects as db_stress: expected_value.cc:102: Assertion `ExpectedValue::IsValueBaseValid(value_base)' failed from TestGet. In the captured crash the aborting db_stress process had injected no faults itself -- it read corruption persisted by an earlier faults-on run (blackbox reuses the DB directory), confirming the persistence route.

Fix: EmbeddedBlobResolvingIterator now resolves eagerly for callers that do not opt into unprepared values (allow_unprepared_value=false, e.g. compaction), via a new EagerEmbeddedBlobResolvingIterator (the caller is selected in BlockBasedTable::NewIterator). Eager callers resolve the value during positioning, so a resolution error (blob-region I/O or corruption) is observable through status()/Valid() BEFORE value() is consumed, and value() never exposes an unresolved same-file BlobIndex. Lazy callers (allow_unprepared_value=true, user iteration) keep value laziness but must honor PrepareValue()'s result. This keeps the "callers never see an unresolved same-file blob" invariant structural, even on the error path. Using a template for EmbeddedBlobResolvingIterator minimizes unnecessary overheads.

Also hardens and documents the integrity tripwires that caught the leak, so they are not "fixed" by weakening them -- doing so would mask real corruption and could persist unresolvable same-file references into ordinary (non-embedded) SSTs, turning a transient error into permanent data loss: FileMetaData::UpdateBoundaries (write/output path), Version::GetBlob and Version::MultiGetBlob (integrated-BlobDB read path), plus the contract note in blob_constants.h.

Adds a test-only sync point
"BlockBasedTable::MaybeResolveEmbeddedValue:InjectError" (compiled out under NDEBUG) to simulate a blob-region resolution fault deterministically.

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

Test Plan:
New unit tests in db_blob_index_test, both of which fail without the fix and pass with it:

* DBBlobIndexTest.EmbeddedBlobResolveErrorDuringCompactionNotMasked -- entity variant. Without the fix CompactRange fails with the masked "Invalid blob file number"; with the fix it fails with the injected IOError.
* DBBlobIndexTest.EmbeddedBlobResolveErrorWholeValueDuringCompactionNotMasked -- whole-value variant. Without the fix CompactRange SUCCEEDS (silently masking the error and persisting {kTypeValue, raw BlobIndex}); with the fix it fails with the injected IOError.

Confirmed the tests exercise the real root cause by temporarily neutralizing the eager resolution (MaybeEagerlyMaterialize): both tests then fail in their respective pre-fix modes (entity -> "Invalid blob file number"; whole-value -> compaction succeeds), then pass again once restored.

End-to-end: the db_stress reproducer for T277566778 (blackbox crash test with embedded-blob ingestion and fault injection) reproduced the "Invalid blob file number" crash before the fix and no longer reproduces with it.

Reviewed By: anand1976

Differential Revision: D110361228

Pulled By: pdillinger

fbshipit-source-id: 1faaa9a9ead726f17b611ff150418fa59ed75fd4

db/blob/blob_constants.h

@@ -21,6 +21,13 @@ constexpr uint64_t kInvalidBlobFileNumber = 0;
 // is currently being read; generic blob-file metadata must treat zero as
 // invalid. (Using a distinct value like 1 was found to be more problematic,
 // e.g. because of legacy "stackable" blob implementation.)
+//
+// This "zero is invalid unless you are the embedded reader/writer" contract is
+// enforced by integrity checks that reject file number zero on generic paths;
+// see FileMetaData::UpdateBoundaries (write/output path) and Version::GetBlob /
+// Version::MultiGetBlob (read path). Same-file references must be resolved (by
+// EmbeddedBlobResolvingIterator) before they reach those paths. Do not weaken
+// those checks -- they catch leaks/corruption closer to the root cause.
 constexpr uint64_t kCurrentFileBlobIndexFileNumber = kInvalidBlobFileNumber;
 static_assert(kCurrentFileBlobIndexFileNumber == kInvalidBlobFileNumber);
 

db/blob/db_blob_index_test.cc

@@ -502,6 +502,132 @@ TEST_F(DBBlobIndexTest, EmbeddedBlobBackwardIteration) {
   EXPECT_EQ(iter->value(), big1);
 }
 
+// Regression test for T277566778. If resolving a same-file ("embedded") blob
+// fails during compaction (e.g. a blob-region read fault), the error must be
+// surfaced as-is. It must NOT be masked by feeding the raw, unresolved
+// same-file BlobIndex downstream, where FileMetaData::UpdateBoundaries would
+// (correctly) reject file number 0 and report a misleading "Invalid blob file
+// number" corruption. The EmbeddedBlobResolvingIterator must not expose an
+// unresolved same-file value on error: for eager (compaction) callers it
+// resolves during positioning so the error is visible via status()/Valid()
+// before value().
+TEST_F(DBBlobIndexTest, EmbeddedBlobResolveErrorDuringCompactionNotMasked) {
+  Options options = GetTestOptions();
+  options.create_if_missing = true;
+  options.disable_auto_compactions = true;
+  DestroyAndReopen(options);
+
+  const std::string sst_path = dbname_ + "/embedded_resolve_err.sst";
+  SstFileWriterEmbeddedBlobOptions embedded_blob_options;
+  embedded_blob_options.min_blob_size = 8;
+
+  // Wide-column entity with a large (embedded, same-file blob) column: the
+  // mixed path whose resolution reads the blob region during compaction.
+  const std::string big_col(1024, 'c');
+  const WideColumns columns{{kDefaultWideColumnName, "d"}, {"big", big_col}};
+
+  SstFileWriter writer(EnvOptions(), options);
+  ASSERT_OK(writer.OpenWithEmbeddedBlobs(sst_path, embedded_blob_options));
+  ASSERT_OK(writer.PutEntity("k2", columns));
+  ASSERT_OK(writer.Finish());
+  ASSERT_OK(db_->IngestExternalFile({sst_path}, IngestExternalFileOptions()));
+
+  // Add keys straddling the entity so a real compaction (not a trivial move)
+  // reads and resolves the embedded entity.
+  ASSERT_OK(Put("k1", "v1"));
+  ASSERT_OK(Put("k3", "v3"));
+  ASSERT_OK(Flush());
+
+  // Inject a resolution failure only during the compaction below, simulating a
+  // blob-region read fault.
+  const Status kInjected = Status::IOError("injected embedded blob read error");
+  SyncPoint::GetInstance()->SetCallBack(
+      "BlockBasedTable::MaybeResolveEmbeddedValue:InjectError",
+      [&](void* arg) { *static_cast<Status*>(arg) = kInjected; });
+  SyncPoint::GetInstance()->EnableProcessing();
+
+  const Status s = db_->CompactRange(CompactRangeOptions(), nullptr, nullptr);
+
+  SyncPoint::GetInstance()->DisableProcessing();
+  SyncPoint::GetInstance()->ClearAllCallBacks();
+
+  // The compaction must fail with the injected read error, NOT a masked
+  // "Invalid blob file number" corruption.
+  ASSERT_NOK(s);
+  EXPECT_TRUE(s.IsIOError()) << s.ToString();
+  EXPECT_EQ(s.ToString().find("Invalid blob file number"), std::string::npos)
+      << s.ToString();
+}
+
+// Regression test for T277310719, the whole-value (kTypeBlobIndex) sibling of
+// the wide-column-entity case above (T277566778). Both share one root cause: on
+// a blob-region resolution error during compaction, the
+// EmbeddedBlobResolvingIterator must not fall back to exposing the raw,
+// unresolved same-file value.
+//
+// The whole-value variant is more dangerous than the entity variant because it
+// escapes the FileMetaData::UpdateBoundaries tripwire: ResolveKeyType()
+// rewrites the key type kTypeBlobIndex -> kTypeValue (no I/O, always succeeds),
+// so on a masked resolution error the emitted entry is {kTypeValue, raw
+// BlobIndex bytes}. UpdateBoundaries only scans kTypeBlobIndex/entity types, so
+// unlike the entity variant it does NOT reject it -- the masked error slips
+// through, a corrupt {kTypeValue, BlobIndex} record is written to the
+// compaction output, and a later point lookup reads those raw BlobIndex bytes
+// back as the value (in db_stress this surfaced as the
+// ExpectedValue::IsValueBaseValid assertion, not "Invalid blob file number").
+// The eager (compaction) resolver must surface the error via status()/Valid()
+// before value() is consumed.
+TEST_F(DBBlobIndexTest,
+       EmbeddedBlobResolveErrorWholeValueDuringCompactionNotMasked) {
+  Options options = GetTestOptions();
+  options.create_if_missing = true;
+  options.disable_auto_compactions = true;
+  DestroyAndReopen(options);
+
+  const std::string sst_path = dbname_ + "/embedded_resolve_err_wholevalue.sst";
+  SstFileWriterEmbeddedBlobOptions embedded_blob_options;
+  embedded_blob_options.min_blob_size = 8;
+
+  // A large whole value is stored as a same-file blob, so the table entry
+  // becomes a same-file (file number 0) BlobIndex (kTypeBlobIndex) -- the
+  // whole-value path whose resolution reads the blob region during compaction.
+  const std::string big_val(1024, 'b');
+
+  SstFileWriter writer(EnvOptions(), options);
+  ASSERT_OK(writer.OpenWithEmbeddedBlobs(sst_path, embedded_blob_options));
+  ASSERT_OK(writer.Put("k2", big_val));
+  ASSERT_OK(writer.Finish());
+  ASSERT_OK(db_->IngestExternalFile({sst_path}, IngestExternalFileOptions()));
+
+  // Add keys straddling the embedded blob so a real compaction (not a trivial
+  // move) reads and resolves it.
+  ASSERT_OK(Put("k1", "v1"));
+  ASSERT_OK(Put("k3", "v3"));
+  ASSERT_OK(Flush());
+
+  // Inject a resolution failure only during the compaction below, simulating a
+  // blob-region read fault.
+  const Status kInjected = Status::IOError("injected embedded blob read error");
+  SyncPoint::GetInstance()->SetCallBack(
+      "BlockBasedTable::MaybeResolveEmbeddedValue:InjectError",
+      [&](void* arg) { *static_cast<Status*>(arg) = kInjected; });
+  SyncPoint::GetInstance()->EnableProcessing();
+
+  const Status s = db_->CompactRange(CompactRangeOptions(), nullptr, nullptr);
+
+  SyncPoint::GetInstance()->DisableProcessing();
+  SyncPoint::GetInstance()->ClearAllCallBacks();
+
+  // The compaction must fail with the injected read error. Before the fix it
+  // instead succeeded (masking the error) and persisted a {kTypeValue, raw
+  // BlobIndex} record; it must not do that, and it must not report a misleading
+  // "Invalid blob file number" corruption either.
+  ASSERT_NOK(s);
+  EXPECT_TRUE(s.IsIOError()) << s.ToString();
+  EXPECT_EQ(s.ToString().find("Invalid blob file number"), std::string::npos)
+      << s.ToString();
+}
+
 // When a blob cache is configured, same-file ("embedded") blob reads should go
 // through BlobSource: the first read misses + inserts + does a disk read, and
 // the second read of the same blob is served from the blob cache. This holds on

db/version_edit.cc

@@ -68,6 +68,23 @@ Status FileMetaData::UpdateBoundaries(const Slice& key, const Slice& value,
   // Helper: update oldest_blob_file_number from a single BlobIndex.
   auto update_oldest_blob = [&](const BlobIndex& blob_index) -> Status {
     if (!blob_index.IsInlined() && !blob_index.HasTTL()) {
+      // INTEGRITY TRIPWIRE -- do not weaken. This is generic file metadata on
+      // the table-build output path (flush / compaction / recovery, via
+      // BuildTable and CompactionOutputs), which must only ever see external
+      // blob-file references. file_number 0 is kInvalidBlobFileNumber, which is
+      // deliberately also the same-file ("embedded") sentinel
+      // kCurrentFileBlobIndexFileNumber (see blob_constants.h). Reaching here
+      // with 0 means one of:
+      //   * a same-file/embedded reference leaked into the output stream
+      //     unresolved -- e.g. EmbeddedBlobResolvingIterator failed to resolve
+      //     and fell back to the raw value on a blob-region read error
+      //     (T277566778); or
+      //   * an uninitialized/corrupt BlobIndex (default file number never set).
+      // Rejecting it flags the bug closer to its root cause. Do NOT relax this
+      // to skip same-file refs (e.g. via BlobIndex::IsSameFile()): that would
+      // mask real corruption AND could persist unresolvable same-file
+      // references into ordinary (non-embedded) SSTs, turning a transient error
+      // into permanent data loss.
       if (blob_index.file_number() == kInvalidBlobFileNumber) {
         return Status::Corruption("Invalid blob file number");
       }

db/version_set.cc

@@ -2813,6 +2813,12 @@ Status Version::GetBlob(const ReadOptions& read_options, const Slice& user_key,
 
   auto blob_file_meta = storage_info_.GetBlobFileMetaData(blob_file_number);
   if (!blob_file_meta) {
+    // INTEGRITY CHECK -- do not weaken. The blob index must reference a known
+    // external blob file. No metadata (including file_number 0, the
+    // kInvalidBlobFileNumber / same-file "embedded" sentinel) means a corrupt
+    // index, or a same-file reference that should have been resolved by
+    // EmbeddedBlobResolvingIterator before reaching the integrated-BlobDB read
+    // path. See FileMetaData::UpdateBoundaries for the write-side tripwire.
     return Status::Corruption("Invalid blob file number");
   }
 
@@ -2854,6 +2860,9 @@ void Version::MultiGetBlob(
       }
 
       if (!blob_file_meta) {
+        // INTEGRITY CHECK -- do not weaken; see Version::GetBlob and
+        // FileMetaData::UpdateBoundaries. A same-file/embedded reference must
+        // be resolved before reaching the integrated-BlobDB read path.
         *key_context->s = Status::Corruption("Invalid blob file number");
         continue;
       }

table/block_based/block_based_table_reader.cc

@@ -1452,6 +1452,21 @@ Status BlockBasedTable::MaybeResolveEmbeddedValue(
     return Status::OK();
   }
 
+#ifndef NDEBUG
+  // Test-only hook: inject a resolution failure (simulating a blob-region read
+  // fault) so tests can verify that the error is surfaced rather than masked by
+  // exposing the raw same-file value downstream. Compiled out in release.
+  {
+    Status injected_status;
+    TEST_SYNC_POINT_CALLBACK(
+        "BlockBasedTable::MaybeResolveEmbeddedValue:InjectError",
+        &injected_status);
+    if (!injected_status.ok()) {
+      return injected_status;
+    }
+  }
+#endif  // NDEBUG
+
   ParsedInternalKey parsed_key;
   // FIXME: de-dup ParseInternalKey() work from callers
   Status s =
@@ -2776,7 +2791,11 @@ InternalIterator* BlockBasedTable::NewIterator(
     if (!resolve_embedded_values) {
       return iter;
     }
-    return new EmbeddedBlobResolvingIterator(this, read_options, iter,
+    if (allow_unprepared_value) {
+      return new LazyEmbeddedBlobResolvingIterator(this, read_options, iter,
+                                                   /*arena_mode=*/false);
+    }
+    return new EagerEmbeddedBlobResolvingIterator(this, read_options, iter,
                                                   /*arena_mode=*/false);
   } else {
     auto* mem = arena->AllocateAligned(sizeof(BlockBasedTableIterator));
@@ -2787,9 +2806,15 @@ InternalIterator* BlockBasedTable::NewIterator(
     if (!resolve_embedded_values) {
       return iter;
     }
+    if (allow_unprepared_value) {
       auto* wrap_mem =
-        arena->AllocateAligned(sizeof(EmbeddedBlobResolvingIterator));
+          arena->AllocateAligned(sizeof(LazyEmbeddedBlobResolvingIterator));
-    return new (wrap_mem) EmbeddedBlobResolvingIterator(
+      return new (wrap_mem) LazyEmbeddedBlobResolvingIterator(
+          this, read_options, iter, /*arena_mode=*/true);
+    }
+    auto* wrap_mem =
+        arena->AllocateAligned(sizeof(EagerEmbeddedBlobResolvingIterator));
+    return new (wrap_mem) EagerEmbeddedBlobResolvingIterator(
         this, read_options, iter, /*arena_mode=*/true);
   }
 }

table/block_based/embedded_blob_resolving_iterator.h

@@ -37,9 +37,14 @@ namespace ROCKSDB_NAMESPACE {
 //     index" deferred state. That keeps the "never expose an unresolved
 //     same-file blob index" invariant structural rather than relying on every
 //     downstream consumer re-checking the key type after PrepareValue().
-//   - The wrapper preserves value laziness on its own: key() does only the
+//   - For lazy callers (allow_unprepared_value=true) the wrapper preserves
-//     cheap key-type rewrite (no blob-region I/O), while the payload is read
+//     value laziness: key() does only the cheap key-type rewrite (no
-//     lazily in value()/PrepareValue().
+//     blob-region I/O) and the payload is read lazily in value()/PrepareValue()
+//     (such callers must honor PrepareValue()'s result). For eager callers
+//     (allow_unprepared_value=false, e.g. compaction) the wrapper resolves the
+//     value during positioning, so a resolution error (blob-region I/O or
+//     corruption) surfaces via status()/Valid() BEFORE value() is consumed and
+//     value() never exposes an unresolved same-file BlobIndex.
 //   - It keeps the embedded-blob concern out of the hot, complex
 //     BlockBasedTableIterator, and is only instantiated for SSTs that actually
 //     carry an embedded blob segment.
@@ -47,6 +52,14 @@ namespace ROCKSDB_NAMESPACE {
 // Only created when the table advertises an embedded blob segment and the
 // caller is not the SST dump tool (which must keep seeing raw BlobIndex
 // values).
+//
+// kAllowUnpreparedValue mirrors the caller's allow_unprepared_value and is a
+// template parameter so the lazy (hot, user-iteration) path carries no
+// eager-resolution branch: when false (eager, e.g. compaction) the wrapper
+// resolves during positioning; when true (lazy) it resolves in
+// value()/PrepareValue(). Prefer the EagerEmbeddedBlobResolvingIterator /
+// LazyEmbeddedBlobResolvingIterator aliases (below) at call sites.
+template <bool kAllowUnpreparedValue>
 class EmbeddedBlobResolvingIterator : public InternalIterator {
  public:
   // `iter` is owned by this wrapper. `arena_mode` must match how `iter` (and
@@ -79,22 +92,27 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
   void SeekToFirst() override {
     ResetState();
     iter_->SeekToFirst();
+    MaybeEagerlyMaterialize();
   }
   void SeekToLast() override {
     ResetState();
     iter_->SeekToLast();
+    MaybeEagerlyMaterialize();
   }
   void Seek(const Slice& target) override {
     ResetState();
     iter_->Seek(target);
+    MaybeEagerlyMaterialize();
   }
   void SeekForPrev(const Slice& target) override {
     ResetState();
     iter_->SeekForPrev(target);
+    MaybeEagerlyMaterialize();
   }
   void Next() override {
     ResetState();
     iter_->Next();
+    MaybeEagerlyMaterialize();
   }
   bool NextAndGetResult(IterateResult* result) override {
     ResetState();
@@ -102,15 +120,24 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
     if (!Valid()) {
       return false;
     }
+    // For eager callers, resolve now so a resolution error is visible via
+    // Valid()/status() before value() is read.
+    MaybeEagerlyMaterialize();
+    if (!Valid()) {
+      return false;
+    }
     result->key = key();
     result->bound_check_result = iter_->UpperBoundCheckResult();
-    result->value_prepared = false;
+    // Eager callers (kAllowUnpreparedValue=false) get a fully-resolved value;
-    // key() may have set a (corruption) status while resolving the key type.
+    // lazy callers must call PrepareValue().
+    result->value_prepared = !kAllowUnpreparedValue;
+    // key()/MaybeEagerlyMaterialize() may have set a status while resolving.
     return Valid();
   }
   void Prev() override {
     ResetState();
     iter_->Prev();
+    MaybeEagerlyMaterialize();
   }
 
   Slice key() const override {
@@ -220,6 +247,20 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
     delete static_cast<std::string*>(arg1);
   }
 
+  // For eager callers (kAllowUnpreparedValue=false), resolve the current
+  // entry's value during positioning. This makes a resolution error (blob
+  // I/O or corruption) observable through status()/Valid() before value() is
+  // consumed, upholding the "callers never see an unresolved same-file
+  // BlobIndex" invariant even on error. Compiled out for lazy callers, which
+  // resolve in value()/PrepareValue() and must honor PrepareValue()'s result.
+  void MaybeEagerlyMaterialize() {
+    if constexpr (!kAllowUnpreparedValue) {
+      if (iter_->Valid()) {
+        MaterializeValue();
+      }
+    }
+  }
+
   void ResetState() {
     status_.PermitUncheckedError();
     status_ = Status::OK();
@@ -375,4 +416,12 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
   mutable bool value_is_pinned_ = false;
 };
 
+// Eager variant (allow_unprepared_value=false, e.g. compaction): resolves the
+// value during positioning so a resolution error surfaces via status()/Valid()
+// before value() is consumed.
+using EagerEmbeddedBlobResolvingIterator = EmbeddedBlobResolvingIterator<false>;
+// Lazy variant (allow_unprepared_value=true, e.g. user iteration): resolves in
+// value()/PrepareValue(); callers must honor PrepareValue()'s result.
+using LazyEmbeddedBlobResolvingIterator = EmbeddedBlobResolvingIterator<true>;
+
 }  // namespace ROCKSDB_NAMESPACE