feat(cubestore): Sync tables cache instead of reset

[ovr]

Instead of resetting the entire tables cache on every write operation, introduce CachedTables with update_table_by_id_or_reset and remove_by_table_id_or_reset methods for surgical in-place mutations. table_ready, seal_table, update_location_download_size, and drop_table now sync the cache without forcing a full reload from RocksDB.

[claude]

Claude finished @ovr's task in 3m 24s —— View job

---

Code Review: feat(cubestore): Sync tables cache instead of reset

• Read PR diff and understand the changes
• Read surrounding context in metastore/mod.rs
• Analyze code quality, correctness, and edge cases
• Analyze performance implications
• Check for security concerns and thread safety
• Verify test coverage
• Post comprehensive review

---

Summary

Good optimization. The new CachedTables abstraction replaces the raw Mutex<Option<Arc<Vec<TablePath>>>> with a proper type that supports surgical in-place updates. For seal_table, update_location_download_size, drop_table, and table_ready(id, false), this avoids a full RocksDB reload on the next read — a real win for write-heavy workloads.

The schema-level operations (create_schema, rename_schema, delete_schema) and create_table correctly continue to reset the cache, since those affect the TablePath structure more broadly.

Key Findings

Performance — Arc::make_mut before existence check (2 instances)
Both update_table_by_id_or_reset and remove_by_table_id_or_reset call Arc::make_mut before checking if the table exists in the cache. Arc::make_mut deep-clones the entire Vec<TablePath> when other Arc references are held by concurrent readers. When the entry isn't found (e.g., table_ready(id, true) on a newly-ready table), this clone is completely wasted. See inline comments for suggested fix — check existence on the immutable Arc first.

Correctness — table_ready(id, true) still resets cache
The cache only stores ready tables. When table_ready(id, true) is called (table transitions not-ready → ready), the table isn't in the cache, so update_table_by_id_or_reset falls back to a full reset. This isn't a bug — it matches the old behavior — but it means the optimization described in the PR description doesn't apply to this specific path. A brief inline comment explaining this expected fallback would help future readers.

Testing — no unit tests for CachedTables
The new CachedTables struct has four non-trivial methods with branching logic (cache populated vs. not, entry found vs. not, ready vs. not-ready after mutation). This is a correctness-critical cache layer and would benefit from targeted unit tests covering:

• update_table_by_id_or_reset when cache is None (no-op)
• update_table_by_id_or_reset when entry exists and stays ready (in-place update)
• update_table_by_id_or_reset when entry exists but becomes not-ready (removal)
• update_table_by_id_or_reset when entry doesn't exist (reset)
• remove_by_table_id_or_reset when entry exists (removal) vs. not (reset)

Minor Nits

• #[inline(always)] on update_table_by_id_or_reset is unnecessary — this function acquires a mutex and does a linear scan; let LLVM decide
• Missing Default impl for CachedTables — clippy will flag new_without_default
• The post-mutation retain() call in update_table_by_id_or_reset does a redundant O(n) scan; tracking the index from find and using swap_remove/remove would be more direct

Thread Safety

The Mutex usage is correct. The lock is held for the entire duration of each update operation, preventing TOCTOU races. Arc::make_mut under the lock correctly provides exclusive mutable access to the inner Vec.

Overall

The design is sound and the abstraction is clean. The main actionable items are: (1) move Arc::make_mut after the existence check to avoid unnecessary clones on cache misses, and (2) add unit tests for the new CachedTables methods. The rest are minor nits.

[claude]

suggestion (performance): Same Arc::make_mut-before-lookup issue as update_table_by_id_or_reset. When dropping a non-ready table (not in cache), this clones the entire vec needlessly:

Suggested change

	let tables = Arc::make_mut(cached);
	let Some(idx) = tables.iter().position(|tp| tp.table.get_id() == table_id) else {
	*guard = None;
	return;
	};
	let Some(idx) = cached.iter().position(|tp| tp.table.get_id() == table_id) else {
	*guard = None;
	return;
	};
	let tables = Arc::make_mut(cached);

[claude]

nit: Clippy will flag new_without_default here. Consider deriving or implementing Default:

Suggested change

	pub struct CachedTables {
	tables: Mutex<Option<Arc<Vec<TablePath>>>>,
	}
	pub struct CachedTables {
	tables: Mutex<Option<Arc<Vec<TablePath>>>>,
	}
	impl Default for CachedTables {
	fn default() -> Self {
	Self::new()
	}
	}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.01%. Comparing base (43b3fd5) to head (830012f).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10686   +/-   ##
=======================================
  Coverage   58.01%   58.01%           
=======================================
  Files         215      215           
  Lines       16648    16648           
  Branches     3347     3347           
=======================================
  Hits         9659     9659           
  Misses       6499     6499           
  Partials      490      490           

Flag	Coverage Δ
cube-backend	58.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.