Implement new storage to improve performance - #3
Conversation
WalkthroughThis change adds LMDB persistence, DSN-based storage selection, configurable storage backends, backend-aware CLI and MCP integration, symbol search improvements, concurrency-safe SQLite ID allocation, and storage performance benchmarks. ChangesStorage backend integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will not alter performance
Performance Changes
Comparing 
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
crates/codegraph-graph/tests/lmdb.rs (1)
281-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the long call name directly.
search_flow(&[SYMBOL_BASE])does not readlong_call. A regression that truncates or drops the call name can still pass this test. Assert the returned flow call name equalslong_call, and add a production call-name lookup assertion if that index is part of the contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codegraph-graph/tests/lmdb.rs` around lines 281 - 284, Update the test around search_flow to assert the returned flow call name matches the long_call fixture value, rather than only checking function_name. Also add a production call-name lookup assertion using the relevant lookup API if the call-name index is part of the tested contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/codegraph-bench/benches/storage.rs`:
- Around line 23-41: Update the repository-list loading flow around
CODEGRAPH_BENCH_REPOS_LIST so a read_to_string failure is not silently converted
into an empty benchmark set. Return the read error if the surrounding function
supports errors, or log it and continue with the existing crates fallback; only
return the parsed list after a successful read.
- Line 109: Resolve the clippy::type_complexity warning for mk_backends by
introducing a descriptive type alias for the boxed backend factory closure, or
by replacing the boxed closures with a small backend enum. Update the
mk_backends declaration and its usages consistently while preserving the
existing factory behavior.
- Around line 70-78: Update the SQLite and LMDB setup flows around index_at so
indexing errors are not silently discarded. Propagate failures from both
persistent backend calls, or explicitly represent each backend as unavailable
and skip its size calculation; do not report an empty directory as a successful
zero-byte backend.
- Around line 70-78: In the benchmark storage setup around index_at and
dir_size, stop calling TempDir::keep() for both SQLite and LMDB directories;
retain the TempDir values, use their paths to build storage URLs and measure
sizes, then let them drop after measurement. Apply the same cleanup change at
crates/codegraph-bench/benches/storage.rs lines 70-78 and 114-123.
In `@crates/codegraph-extract/src/config.rs`:
- Around line 139-143: Update the storage backend resolution around the match on
StorageKind in the configuration path to reject StorageKind::Redis when no DSN
is configured, rather than returning None. Make the resolution fallible or
validate the configuration during loading, and ensure invalid Redis settings
propagate an explicit configuration error instead of being interpreted by CLI or
MCP consumers as GraphIndex::in_memory().
In `@crates/codegraph-graph/src/lib.rs`:
- Around line 178-183: Update GraphIndex::open and split_dsn to recognize the
redis scheme and dispatch redis:// DSNs through a feature-gated Redis opener,
alongside the existing SQLite and LMDB routes. Keep the plain-DSN fallback for
unrecognized schemes and preserve behavior when Redis support is disabled.
In `@crates/codegraph-graph/src/shared.rs`:
- Around line 82-104: Update current_version to probe Redis using the same
version key and retrieval/parsing behavior as RedisStorage::version(), returning
the stored u64 instead of None. Keep is_fresh’s version comparison unchanged,
and add an integration test covering two unchanged ensure_fresh calls that
confirms the second call reuses the existing snapshot.
In `@crates/codegraph-graph/src/storage/lmdb.rs`:
- Around line 353-374: Move all LMDB write operations in LmdbStorage mutation
methods, including Storage::new_node, LmdbTx::commit, and every other
begin_rw_txn/tx.commit path, off Tokio worker threads by executing them through
spawn_blocking or the existing dedicated blocking storage thread. Preserve
transaction ordering, error propagation, and returned values while ensuring no
LMDB write transaction is opened or committed directly in async worker-thread
code.
In `@crates/codegraph/src/main.rs`:
- Around line 549-554: Update the entry-point descriptions and failure messages
used by the lookup flow in crates/codegraph/src/main.rs (lines 549-554 and the
related error at line 558) and crates/codegraph-mcp/src/tools.rs (lines 746-748
and the related error at line 751) to consistently refer to both functions and
methods, while preserving the existing Function/Method search behavior.
- Around line 217-228: Preserve indexed graph state when storage_dsn returns
None instead of ingesting into a temporary dropped index. In
crates/codegraph/src/main.rs lines 217-228, distinguish explicit in-memory
storage from Redis lacking a DSN and reject unsupported CLI/MCP modes or retain
shared state; in crates/codegraph/src/main.rs lines 269-277, ensure the index
used by the indexing command remains available to later operations; apply the
same policy in crates/codegraph-mcp/src/tools.rs lines 638-645 for
codegraph_index.
- Line 146: Update is_initialized and the cmd_init/cmd_default readiness flow to
distinguish workspace directory creation from completed indexing: require the
workspace path to pass is_dir(), and use a separate index-ready marker that is
created only after indexing succeeds. Have cmd_default report “Workspace Active
& Indexed” only when that marker exists, while allowing cmd_init to create the
directory without marking the index ready.
---
Nitpick comments:
In `@crates/codegraph-graph/tests/lmdb.rs`:
- Around line 281-284: Update the test around search_flow to assert the returned
flow call name matches the long_call fixture value, rather than only checking
function_name. Also add a production call-name lookup assertion using the
relevant lookup API if the call-name index is part of the tested contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8736dc6-ce18-409e-bfdb-df9e1aac380b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlcrates/codegraph-api/tests/api.rscrates/codegraph-bench/Cargo.tomlcrates/codegraph-bench/STORAGE_PERF.mdcrates/codegraph-bench/benches/storage.rscrates/codegraph-bench/src/lib.rscrates/codegraph-extract/src/config.rscrates/codegraph-extract/src/walker.rscrates/codegraph-graph/Cargo.tomlcrates/codegraph-graph/src/lib.rscrates/codegraph-graph/src/shared.rscrates/codegraph-graph/src/storage.rscrates/codegraph-graph/src/storage/lmdb.rscrates/codegraph-graph/src/storage/sqlite.rscrates/codegraph-graph/tests/lmdb.rscrates/codegraph-graph/tests/sqlite.rscrates/codegraph-mcp/src/lib.rscrates/codegraph-mcp/src/tools.rscrates/codegraph/Cargo.tomlcrates/codegraph/src/main.rscrates/codegraph/src/watcher.rs
| if let Ok(list_file) = std::env::var("CODEGRAPH_BENCH_REPOS_LIST") { | ||
| if let Ok(body) = std::fs::read_to_string(&list_file) { | ||
| for line in body.lines() { | ||
| let line = line.trim(); | ||
| if line.is_empty() || line.starts_with('#') { | ||
| continue; | ||
| } | ||
| let name = std::path::Path::new(line) | ||
| .file_name() | ||
| .and_then(|s| s.to_str()) | ||
| .map(String::from) | ||
| .unwrap_or_else(|| line.to_string()); | ||
| out.push(Repo { | ||
| name, | ||
| root: line.into(), | ||
| }); | ||
| } | ||
| } | ||
| return out; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not silently skip all benchmarks after a list-file read error.
If CODEGRAPH_BENCH_REPOS_LIST exists but read_to_string fails, this function returns an empty list. Criterion can then finish successfully without benchmark results. Return an error, or log the error and use the crates fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegraph-bench/benches/storage.rs` around lines 23 - 41, Update the
repository-list loading flow around CODEGRAPH_BENCH_REPOS_LIST so a
read_to_string failure is not silently converted into an empty benchmark set.
Return the read error if the surrounding function supports errors, or log it and
continue with the existing crates fallback; only return the parsed list after a
successful read.
| let sqlite_dir = tempfile::tempdir().unwrap().keep(); | ||
| let sqlite = format!("sqlite://{}/db.sqlite", sqlite_dir.to_string_lossy()); | ||
| if let Ok(_idx) = index_at(parsed, Some(&sqlite)) {} | ||
| let sqlite_bytes = dir_size(&sqlite_dir); | ||
|
|
||
| let lmdb_dir = tempfile::tempdir().unwrap().keep(); | ||
| let lmdb = format!("lmdb://{}", lmdb_dir.to_string_lossy()); | ||
| if let Ok(_idx) = index_at(parsed, Some(&lmdb)) {} | ||
| let lmdb_bytes = dir_size(&lmdb_dir); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail when persistent backend setup fails.
The empty if let Ok(...) blocks discard indexing errors. The following size calculation can report an empty directory as a valid zero-byte backend. Propagate the error or explicitly mark that backend as unavailable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegraph-bench/benches/storage.rs` around lines 70 - 78, Update the
SQLite and LMDB setup flows around index_at so indexing errors are not silently
discarded. Propagate failures from both persistent backend calls, or explicitly
represent each backend as unavailable and skip its size calculation; do not
report an empty directory as a successful zero-byte backend.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching storage.rs:\n'
fd -a 'storage\.rs$' . | sed 's#^\./##'
printf '\nTarget file outline:\n'
ast-grep outline crates/codegraph-bench/benches/storage.rs || true
printf '\nRelevant sections:\n'
sed -n '1,180p' crates/codegraph-bench/benches/storage.rs
printf '\nDisk-related benches/search:\n'
rg -n "tempfile::tempdir|TempDir::keep|keep\(|index_at|dir_size|GraphIndex|bench|Criterion|storage" crates/codegraph-bench/benches/storage.rsRepository: hungpham10/codegraph-rs
Length of output: 8097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check tempfile crate docs/source availability via Cargo.lock/Cargo.toml and installed docs if present.
printf 'tempfile declarations:\n'
rg -n '^\[.*tempfile|^\[package\]|name = "tempfile"|version = ' Cargo.lock crates/codegraph-bench/Cargo.toml Cargo.toml || true
printf '\nCargo docs availability:\n'
if [ -d "$CARGO_HOME/doc/tempfile" ]; then
find "$CARGO_HOME/doc/tempfile" -maxdepth 2 -type f | sed -n '1,40p'
else
echo "cargo doc for tempfile not found"
fiRepository: hungpham10/codegraph-rs
Length of output: 14298
🌐 Web query:
tempfile Rust 3.27 TempDir keep documentation
💡 Result:
In the Rust tempfile crate (version 3.27.0), the TempDir::keep method is used to persist a temporary directory on the disk [1]. Calling TempDir::keep consumes the TempDir instance and prevents the directory from being automatically deleted when it goes out of scope (i.e., when the object is dropped) [1]. It returns a PathBuf representing the path to the directory, allowing you to manage it manually [1]. Key points regarding keeping or persisting temporary directories: - TempDir::keep: This is the preferred method for turning a temporary directory into a permanent one [1]. It consumes the TempDir object [1]. - disable_cleanup: If you want to prevent automatic deletion but keep the TempDir object available for further use (rather than consuming it), you can call disable_cleanup(true) [1]. This is typically recommended for testing or debugging scenarios rather than simply persisting a directory [1]. - Deprecation Note: The method into_path has been deprecated in favor of TempDir::keep [1].
Citations:
Clean up benchmark temporary storage after measurement.
All these sites call TempDir::keep(), which disables automatic cleanup. Let the SQLite/LMDB temp directories remain a normal TempDir and drop them after indexing or size measurement so repeated benchmark iterations do not create permanent storage directories.
📍 Affects 1 file
crates/codegraph-bench/benches/storage.rs#L70-L78(this comment)crates/codegraph-bench/benches/storage.rs#L114-L123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegraph-bench/benches/storage.rs` around lines 70 - 78, In the
benchmark storage setup around index_at and dir_size, stop calling
TempDir::keep() for both SQLite and LMDB directories; retain the TempDir values,
use their paths to build storage URLs and measure sizes, then let them drop
after measurement. Apply the same cleanup change at
crates/codegraph-bench/benches/storage.rs lines 70-78 and 114-123.
| match self.storage.kind { | ||
| StorageKind::Sqlite => Some(format!("sqlite://{}", project_db_path(root))), | ||
| StorageKind::Lmdb => Some(format!("lmdb://{}", project_dir(root).join("db.lmdb"))), | ||
| StorageKind::Redis => None, | ||
| StorageKind::Memory => None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject a Redis configuration without a DSN.
StorageKind::Redis returns None here. The CLI and MCP consumers interpret None as GraphIndex::in_memory(). A project configured with type = "redis" therefore indexes into ephemeral memory instead of reporting invalid configuration.
Make backend resolution fallible, or validate Redis during config loading. Do not use None for this configuration error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegraph-extract/src/config.rs` around lines 139 - 143, Update the
storage backend resolution around the match on StorageKind in the configuration
path to reject StorageKind::Redis when no DSN is configured, rather than
returning None. Make the resolution fallible or validate the configuration
during loading, and ensure invalid Redis settings propagate an explicit
configuration error instead of being interpreted by CLI or MCP consumers as
GraphIndex::in_memory().
| pub async fn open(dsn: &str) -> Result<Self> { | ||
| #[cfg(feature = "sqlite")] | ||
| #[allow(unreachable_code)] | ||
| return Self::open_sqlite(dsn).await; | ||
| match Self::split_dsn(dsn) { | ||
| Some(("sqlite", path)) => Self::open_sqlite_dispatch(path).await, | ||
| Some(("lmdb", path)) => Self::open_lmdb_dispatch(path).await, | ||
| _ => Self::open_default(dsn).await, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dispatch redis:// before the plain-DSN fallback.
split_dsn() does not recognize redis://. With Redis and SQLite or LMDB enabled, GraphIndex::open("redis://...") enters open_default() and returns the “DSN does not specify scheme” error.
Add Redis to split_dsn() and route it through a feature-gated Redis dispatch method.
Proposed fix
match Self::split_dsn(dsn) {
Some(("sqlite", path)) => Self::open_sqlite_dispatch(path).await,
Some(("lmdb", path)) => Self::open_lmdb_dispatch(path).await,
+ Some(("redis", dsn)) => Self::open_redis_dispatch(dsn).await,
_ => Self::open_default(dsn).await,
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegraph-graph/src/lib.rs` around lines 178 - 183, Update
GraphIndex::open and split_dsn to recognize the redis scheme and dispatch
redis:// DSNs through a feature-gated Redis opener, alongside the existing
SQLite and LMDB routes. Keep the plain-DSN fallback for unrecognized schemes and
preserve behavior when Redis support is disabled.
| async fn current_version(&self) -> Option<u64> { | ||
| let p = self.path.as_ref()?; | ||
| crate::storage::sqlite::SqliteStorage::probe_version(&p.display().to_string()) | ||
| .await | ||
| .ok() | ||
| let dsn = self.dsn.as_ref()?; | ||
| let path = trim_scheme(dsn); | ||
| match self.scheme() { | ||
| #[cfg(feature = "sqlite")] | ||
| Some("sqlite") => crate::storage::sqlite::SqliteStorage::probe_version(path) | ||
| .await | ||
| .ok(), | ||
| #[cfg(feature = "lmdb")] | ||
| Some("lmdb") => crate::storage::lmdb::probe_version(path).await.ok(), | ||
| // redis không có probe file ngoài — không đo được → stale. | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| /// Snapshot hiện tại có khớp version trên đĩa không. In-memory (không file) | ||
| /// → không có writer ngoài → luôn fresh. | ||
| /// → không có writer ngoài → luôn fresh. Backend không probe được (redis/ | ||
| /// unknown scheme) → coi là stale để rebuilt lại. | ||
| async fn is_fresh(&self, version: u64) -> bool { | ||
| #[cfg(feature = "sqlite")] | ||
| { | ||
| if self.path.is_none() { | ||
| return true; | ||
| } | ||
| matches!(self.current_version().await, Some(v) if v == version) | ||
| } | ||
| #[cfg(not(feature = "sqlite"))] | ||
| { | ||
| let _ = version; | ||
| true | ||
| if self.dsn.is_none() { | ||
| return true; | ||
| } | ||
| matches!(self.current_version().await, Some(v) if v == version) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Probe the Redis version instead of rebuilding on every request.
current_version() returns None for Redis. is_fresh() then always returns false. Every ensure_fresh() call rebuilds the Redis-backed graph snapshot.
Add a Redis version probe using the same version key as RedisStorage::version(). Add an integration test that verifies two unchanged ensure_fresh() calls reuse the same snapshot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegraph-graph/src/shared.rs` around lines 82 - 104, Update
current_version to probe Redis using the same version key and retrieval/parsing
behavior as RedisStorage::version(), returning the stored u64 instead of None.
Keep is_fresh’s version comparison unchanged, and add an integration test
covering two unchanged ensure_fresh calls that confirms the second call reuses
the existing snapshot.
| #[async_trait] | ||
| impl Storage for LmdbStorage { | ||
| async fn new_node(&mut self, prefix: Vec<u8>, record: usize) -> Result<usize> { | ||
| let mut tx = self.env.begin_rw_txn().map_err(e)?; | ||
| // Không có RETURNING — đọc-rồi-ghi counter trong cùng write tx; an toàn | ||
| // vì GraphIndex tuần tự hoá mọi writer qua RwLock. | ||
| let next = match self.get_opt(&tx, self.counter, &KEY_ONE)? { | ||
| Some(v) => de_u64(v), | ||
| None => 1, | ||
| }; | ||
| let id = next as usize; | ||
| tx.put(self.counter, &KEY_ONE, &ku64(next + 1), WriteFlags::empty()) | ||
| .map_err(e)?; | ||
| tx.put( | ||
| self.nodes, | ||
| &k8(id), | ||
| &node_val(&prefix, record), | ||
| WriteFlags::empty(), | ||
| ) | ||
| .map_err(e)?; | ||
| tx.commit().map_err(e)?; | ||
| Ok(id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
For lmdb-rkv 0.14, confirm the behavior of Environment::begin_rw_txn when another write transaction is active.
💡 Result:
In lmdb-rkv 0.14, the Environment::begin_rw_txn method is explicitly documented to block when another write transaction is active [1]. Because LMDB enforces serialized write access, only one write transaction may be active at any given time [2][3]. Consequently, if a thread calls begin_rw_txn while another write transaction is currently in progress, it will block until that active write transaction either commits or aborts [1][2].
Citations:
- 1: https://docs.rs/crate/lmdb-rkv/0.14.0/source/src/environment.rs
- 2: document various limitations of LMDB mozilla/rkv#49
- 3: http://www.lmdb.tech/doc/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- files ---\n'
fd -a 'lmdb\.rs|Cargo\.(toml|lock)' . | sed 's#^\./##' | head -100
printf '\n--- lmdb file outline/counts ---\n'
if [ -f crates/codegraph-graph/src/storage/lmdb.rs ]; then
wc -l crates/codegraph-graph/src/storage/lmdb.rs
ast-grep outline crates/codegraph-graph/src/storage/lmdb.rs --view compact | head -200 || true
fi
printf '\n--- relevant write methods ---\n'
if [ -f crates/codegraph-graph/src/storage/lmdb.rs ]; then
rg -n "begin_rw_txn|spawn_blocking|Storage|new_node|commit_storage|commit|update|delete|write" crates/codegraph-graph/src/storage/lmdb.rs -C 3
fi
printf '\n--- dependency versions ---\n'
rg -n 'lmdb-rkv|rkv|tokio' Cargo.toml crates -g 'Cargo.toml' Cargo.lock -C 2Repository: hungpham10/codegraph-rs
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- lmdb file outlines/counts ---\n'
fd -a 'lmdb\.rs|Cargo\.(toml|lock)' . | sed 's#^\./##' | head -100
printf '\n--- relevant lmdb.rs sections ---\n'
if [ -f crates/codegraph-graph/src/storage/lmdb.rs ]; then
wc -l crates/codegraph-graph/src/storage/lmdb.rs
ast-grep outline crates/codegraph-graph/src/storage/lmdb.rs --view compact | head -200 || true
sed -n '1,520p' crates/codegraph-graph/src/storage/lmdb.rs | nl -ba | sed -n '320,410p'
fi
printf '\n--- dependency versions ---\n'
rg -n 'lmdb-rkv|rkv|tokio|async-trait|parking_lot' Cargo.toml crates -g 'Cargo.toml' Cargo.lock -C 2
printf '\n--- async storage call sites with new_node/write names ---\n'
rg -n "new_node\(|commit_storage\(|begin_rw_txn\(|spawn_blocking" crates -C 2Repository: hungpham10/codegraph-rs
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd 'lmdb|Cargo\.(toml|lock)' . | sed 's#^\./##' | head -100
printf '%s\n' '--- lmdb.rs references ---'
if [ -f crates/codegraph-graph/src/storage/lmdb.rs ]; then
wc -l crates/codegraph-graph/src/storage/lmdb.rs
ast-grep outline crates/codegraph-graph/src/storage/lmdb.rs --view compact | head -200 || true
sed -n '300,400p' crates/codegraph-graph/src/storage/lmdb.rs | nl -ba
printf '%s\n' '--- all write-related methods ---'
rg -n "begin_rw_txn|spawn_blocking|Storage|new_node|commit_storage|commit|update|delete|create" crates/codegraph-graph/src/storage/lmdb.rs -C 3
fi
printf '%s\n' '--- dependency versions ---'
rg -n 'lmdb-rkv|rkv|tokio|async-trait|parking_lot' Cargo.toml crates -g 'Cargo.toml' Cargo.lock -C 2 || true
printf '%s\n' '--- async storage call sites ---'
rg -n "new_node\(|commit_storage\(|begin_rw_txn\(|spawn_blocking" crates -C 2 || trueRepository: hungpham10/codegraph-rs
Length of output: 881
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lmdb.rs lines 300-390 ---'
sed -n '300,390p' crates/codegraph-graph/src/storage/lmdb.rs
printf '%s\n' '--- lmdb.rs all begin_rw_txn/write-related lines ---'
rg -n "begin_rw_txn|commit\b|Storage|new_node|commit_storage|set|delete|write" crates/codegraph-graph/src/storage/lmdb.rs -C 3
printf '%s\n' '--- async storage call sites ---'
rg -n "new_node\(|commit_storage\(|begin_rw_txn\(|spawn_blocking" crates -C 2 || true
printf '%s\n' '--- dependency versions ---'
rg -n 'lmdb-rkv|rkv|tokio|async-trait|parking_lot|mdbx' Cargo.toml crates -g 'Cargo.toml' Cargo.toml.lock || trueRepository: hungpham10/codegraph-rs
Length of output: 50379
Do not run LMDB write transactions on Tokio worker threads.
Environment::begin_rw_txn() and tx.commit() can block for an active write transaction, so LmdbStorage mutation methods and LmdbTx::commit should run behind spawn_blocking or a dedicated blocking storage thread. Apply this to all LMDB write paths such as new_node, commit, and the other write methods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegraph-graph/src/storage/lmdb.rs` around lines 353 - 374, Move all
LMDB write operations in LmdbStorage mutation methods, including
Storage::new_node, LmdbTx::commit, and every other begin_rw_txn/tx.commit path,
off Tokio worker threads by executing them through spawn_blocking or the
existing dedicated blocking storage thread. Preserve transaction ordering, error
propagation, and returned values while ensuring no LMDB write transaction is
opened or committed directly in async worker-thread code.
|
|
||
| fn cmd_default(root: &Utf8Path) -> Result<()> { | ||
| if !db_path(root).exists() { | ||
| if !is_initialized(root) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Track workspace initialization separately from index readiness.
cmd_init can create .codegraph without indexing. is_initialized then returns true, and cmd_default can print “Workspace Active & Indexed” after opening an empty backend. A regular file also passes exists(). Use a separate index-ready marker and use is_dir() for the directory check.
Also applies to: 231-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegraph/src/main.rs` at line 146, Update is_initialized and the
cmd_init/cmd_default readiness flow to distinguish workspace directory creation
from completed indexing: require the workspace path to pass is_dir(), and use a
separate index-ready marker that is created only after indexing succeeds. Have
cmd_default report “Workspace Active & Indexed” only when that marker exists,
while allowing cmd_init to create the directory without marking the index ready.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/codegraph-graph/src/storage/lmdb.rs (1)
1083-1100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert environment identity instead of only returned versions.
The current assertions only exercise
probe_version; an implementation that opens a newEnvironmentfor every probe call and reads the same version would still pass. Callprobe_envtwice per path and assertArc::ptr_eqon the returned values while keeping the concurrent probe checks as separate functional coverage.Proposed test addition
+ let env1 = probe_env(&path).unwrap(); + let env1_again = probe_env(&path).unwrap(); + assert!(Arc::ptr_eq(&env1, &env1_again)); + + let env2 = probe_env(&path2).unwrap(); + let env2_again = probe_env(&path2).unwrap(); + assert!(Arc::ptr_eq(&env2, &env2_again));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codegraph-graph/src/storage/lmdb.rs` around lines 1083 - 1100, Update the concurrent test around probe_version to separately call probe_env twice for each path and assert the returned Arcs are identical with Arc::ptr_eq, verifying shared environment caching. Preserve the existing probe_version loops and version assertions as separate functional coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/codegraph-graph/src/storage/lmdb.rs`:
- Around line 1083-1100: Update the concurrent test around probe_version to
separately call probe_env twice for each path and assert the returned Arcs are
identical with Arc::ptr_eq, verifying shared environment caching. Preserve the
existing probe_version loops and version assertions as separate functional
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fdefcbb6-15d3-4065-b0e5-90e092d48fe2
📒 Files selected for processing (2)
crates/codegraph-bench/benches/storage.rscrates/codegraph-graph/src/storage/lmdb.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/codegraph-bench/benches/storage.rs
1 participant
Summary by CodeRabbit
New Features
.codegraph/config.toml.Bug Fixes
Documentation