Skip to content

feat: Allow SecretDB to open unknown column families#2065

Merged
gilcu3 merged 1 commit intomainfrom
2064-rocksdbopen_db-is-not-forwards-compatible
Feb 12, 2026
Merged

feat: Allow SecretDB to open unknown column families#2065
gilcu3 merged 1 commit intomainfrom
2064-rocksdbopen_db-is-not-forwards-compatible

Conversation

@netrome
Copy link
Collaborator

@netrome netrome commented Feb 12, 2026

closes #2064

@netrome netrome linked an issue Feb 12, 2026 that may be closed by this pull request
@netrome netrome self-assigned this Feb 12, 2026
@claude
Copy link

claude bot commented Feb 12, 2026

Code Review

Critical Issue: Potential panic scenario (low risk but worth documenting)

Location: crates/node/src/db.rs:101-102

The cf_handle() method uses .unwrap() which could theoretically panic if code tries to access a column family that doesn't have a corresponding DBCol variant:

fn cf_handle(&self, cf: DBCol) -> rocksdb::ColumnFamilyRef {
    self.db.cf_handle(cf.as_str()).unwrap()  // Could panic
}

Analysis:

  • In practice, this is safe because cf_handle() is private and only called with known DBCol enum variants
  • Unknown CFs (opened for forward compatibility) are never accessed through this method
  • However, the .unwrap() doesn't make this invariant explicit

Recommendation:
Add a comment documenting this safety invariant to prevent future confusion:

fn cf_handle(&self, cf: DBCol) -> rocksdb::ColumnFamilyRef {
    // SAFETY: All DBCol variants are guaranteed to exist in the database
    // because we open with known_cfs.union(on_disk_cfs). Unknown CFs
    // opened for forward compatibility are never accessed through DBCol.
    self.db.cf_handle(cf.as_str()).unwrap()
}

Code Quality: Minor optimization opportunity

Location: crates/node/src/db.rs:88-96

The union operation creates unnecessary allocations. Consider using extend() instead:

let mut all_cfs: Vec<String> = DBCol::all()
    .iter()
    .map(|col| col.as_str().to_string())
    .collect();
all_cfs.extend(
    rocksdb::DB::list_cf(&options, path)
        .unwrap_or_default()
        .into_iter()
        .filter(|cf| \!all_cfs.contains(cf))
);
let all_cfs_refs: Vec<&str> = all_cfs.iter().map(|s| s.as_str()).collect();
let db = rocksdb::DB::open_cf(&options, path, &all_cfs_refs)?;

This avoids the intermediate Vec<&String> and makes the code slightly more efficient.


Test Coverage: ✅ Good

The test properly validates the forward compatibility scenario with an unknown future_cf column family.


Overall Assessment: The implementation correctly solves the forward compatibility issue. The potential panic in cf_handle() is not a practical concern given the current usage patterns, but adding a safety comment would improve code clarity.

⚠️ Minor improvements suggested, not blocking

@netrome netrome force-pushed the 2064-rocksdbopen_db-is-not-forwards-compatible branch from f8cbbd2 to c03d33e Compare February 12, 2026 11:13
Copy link
Contributor

@gilcu3 gilcu3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@netrome netrome added this pull request to the merge queue Feb 12, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 12, 2026
@gilcu3 gilcu3 added this pull request to the merge queue Feb 12, 2026
Merged via the queue into main with commit b0d572f Feb 12, 2026
9 checks passed
@gilcu3 gilcu3 deleted the 2064-rocksdbopen_db-is-not-forwards-compatible branch February 12, 2026 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RocksDB::open_db is not forwards compatible

3 participants