db_stress: fix crashes during concurrent column family drop+recreate …

[midom]

…(#14601)

Fix two bugs in db_stress that cause crashes when column families are dropped and recreated (via clear_column_family_one_in):

1. Out-of-bounds access in GetOptions (introduced in 217e075, [Remote Compaction] Simulate e2e flow in Stress Test #13800): GetOptions(cf_id) used the RocksDB internal CF ID as a direct index into column_families_. After a CF drop+recreate, the new CF gets a monotonically increasing ID (e.g., 10 for an initially 10-CF setup) that exceeds column_families_.size(), causing undefined behavior. Fix: search column_families_ for the handle matching the CF ID, falling back to base options if the CF was dropped.

2. Use-after-free on CF handles (present since 457c78e, 2014): MaybeClearOneColumnFamily had two issues:

   • It called delete on the old CF handle immediately while other threads could still hold pointers to it (use-after-free/segfault).
   • CreateColumnFamilyImpl zeroes its output handle (*handle = nullptr) before writing the new one. Since it was called with &column_families_[cf], this created a window where concurrent readers see nullptr (assertion failure). Fix: use a local variable for CreateColumnFamily output to avoid the nullptr window, and defer deletion of old handles to CleanUpColumnFamilies when all threads have stopped.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 83.5s.

[xingbowang]

The bug found here is real. The stress test code quality is typically lower than engine code. Although this drop/recreate test was disabled years ago. My guess is low ROI.

[xingbowang]

since you are here, we should probably fix the type, uint32_t is the right type for cf_id

[xingbowang]

On cf id not found, we used to cause segfault, accessing nullptr. Now we just return default options_. Although with delete/recreate, this is a valid case we need to handle properly for remote compaction. I think it is better to surface CF not found error back to caller in remote compaction and have remote compaction just abort the compaction gracefully.

[xingbowang]

The old code treat CF id as the index of column_families_. This is right, if CF is never deleted and recreated. Given we do have code that recreate CF (although this is never called, that's why the bug was never triggered), we should not hold this assumption anymore. I think the fix is correct. Meantime, once we re-enable the drop/recreate test, accessing it would be better guarded by a mutex for thread safety. Although since it is just a pointer of array that is never resized, most hardware would guarantee thread safety on raw pointer read/write, but tsan might warn about this.
Meantime, on re-enable drop/recreate, I don't know whether the stress test handles this gracefully. Essentially whether it would create concurrent write/read failure on a dropped CF properly. Also the re-constructed expected state might need to be updated as well. In short, to re-enable this functionality test, it will likely require a lot more work than this fix. Meantime, dropping a CF concurrently while another thread is read/write it is likely rare in production. Application typically do synchronization by themself on concurrent drop. So I don't know how much value would this provide. If we do have this kind of use case, then I agree we should spend effort on fixing this.

[xingbowang]

If you want to spend more time on this, I would just address the type and return type and ship fix this. For the bigger issue, they can be incrementally addressed.