feat: route CLI write commands through a running serve process - #213
Conversation
DuckDB is single-writer per file, so when 'codesearch serve' holds the database open (e.g. under Hoplon), a one-shot CLI write command failed with a lock error: Could not set lock on file '.../codesearch.duckdb': Conflicting lock is held in .../codesearch (PID …) Now the write commands (create/index/delete) detect a running server for the same data-dir and route through its management API instead of contending for the lock. Read commands are untouched — they already open the DB read-only, which DuckDB allows alongside the writer. Detection: 'serve' writes a run-info file (serve.json: mgmt/mcp ports, pid, version) into the data-dir on startup and removes it on shutdown (SIGINT and, newly, SIGTERM — the signal a supervising app sends). The CLI reads it and confirms liveness with a /health probe before routing; a stale file is ignored when the probe fails. New surface: - POST /api/namespaces — server-side 'create' (the one write endpoint that didn't exist; index/delete already did). - cli::server_client — detection + client for create/index/delete, with --local (force direct) and --server <URL> (force a target) globals. Policy: if a server owns the data-dir but the API call fails, the command errors clearly (suggesting --local) rather than silently opening the DB and reproducing the lock error. Verified end to end against a release build: create/index route through a running server; --local reproduces the original lock error; serve.json is written on start and removed on SIGTERM.
📝 WalkthroughWalkthroughThe change adds ChangesServer-routed management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RouteResolver
participant ServerClient
participant ManagementAPI
participant DuckDBRepository
CLI->>RouteResolver: resolve write command target
RouteResolver->>ManagementAPI: probe /health
RouteResolver-->>CLI: server or local target
CLI->>ServerClient: submit management request
ServerClient->>ManagementAPI: POST namespace or repository operation
ManagementAPI->>DuckDBRepository: create or update repository state
DuckDBRepository-->>ManagementAPI: operation result
ManagementAPI-->>ServerClient: JSON or SSE response
ServerClient-->>CLI: formatted result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![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 (5)
src/cli/server_client.rs (2)
269-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the SSE parser.
The tests cover
urlencodeand the two short-circuit paths ofresolve_route.consume_index_ssecarries the most parsing logic in this file and has no test. Extract the frame-handling logic into a function that accepts&strchunks, then cover these cases: adoneevent, anerrorevent, a payload split across two chunks at a multi-byte character boundary, and a stream that ends without a terminal event.🤖 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 `@src/cli/server_client.rs` around lines 269 - 292, Add focused tests for the SSE parsing behavior in consume_index_sse, first extracting frame handling into a helper that accepts &str chunks. Cover done events, error events, payloads split across chunks at a multi-byte character boundary, and streams ending without a terminal event, while preserving the existing urlencode and resolve_route tests.
100-105: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet a request timeout on the
ServerClientHTTP client.
reqwest::Client::new()applies no request timeout.create_namespaceanddelete_repositorytherefore wait without bound if the server accepts the connection and then stalls. The CLI hangs with no output.Set a connect timeout and a per-request timeout on the short calls. Keep
index_repositorywithout a total timeout, because the SSE stream is long-lived; usereqwest::RequestBuilder::timeouton the two short calls instead of a client-wide timeout.♻️ Proposed refactor
pub fn new(base: String) -> Self { Self { base, - client: reqwest::Client::new(), + client: reqwest::Client::builder() + .connect_timeout(CONNECT_TIMEOUT) + .build() + .unwrap_or_default(), } }Add the constant next to
HEALTH_TIMEOUT:/// Connection timeout for management API calls. The server runs on loopback. const CONNECT_TIMEOUT: Duration = Duration::from_secs(2); /// Timeout for the short, non-streaming management calls. const REQUEST_TIMEOUT: Duration = Duration::from_secs(30);Then apply
.timeout(REQUEST_TIMEOUT)to thecreate_namespaceanddelete_repositoryrequest builders.🤖 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 `@src/cli/server_client.rs` around lines 100 - 105, Update ServerClient::new to configure the reqwest client with the CONNECT_TIMEOUT connection timeout, while leaving the client without a global request timeout. Apply REQUEST_TIMEOUT via RequestBuilder::timeout to the short create_namespace and delete_repository calls only; preserve index_repository’s long-lived SSE stream behavior.src/connector/adapter/management/runinfo.rs (1)
52-93: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMove the filesystem calls off the async runtime, or document them as sync-only helpers.
write,remove, andreadcallstd::fsdirectly. All three are invoked from async contexts:writeatsrc/main.rsLine 460,removeatsrc/main.rsLine 496, andreadthroughresolve_routeinsrc/cli/server_client.rsLine 60. The coding guidelines require blocking calls to be wrapped intokio::task::spawn_blocking. The payload is small, so the practical stall is short, but a slow or unresponsive filesystem blocks the runtime worker.Wrap each call site in
tokio::task::spawn_blocking, or provide async wrappers in this module and keep the sync functions private.As per coding guidelines: "Use async/await for I/O, wrap blocking calls in
tokio::task::spawn_blocking".🤖 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 `@src/connector/adapter/management/runinfo.rs` around lines 52 - 93, Move the blocking filesystem operations used by runinfo::write, runinfo::remove, and runinfo::read off async runtime workers. Prefer adding async wrappers that call the existing synchronous helpers through tokio::task::spawn_blocking, keeping the sync implementations private, then update main.rs call sites and resolve_route to await those wrappers.Source: Coding guidelines
src/connector/adapter/management/handlers/namespaces.rs (1)
98-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse a shared constant for the database filename.
"codesearch.duckdb"is also hard-coded insrc/main.rsat Line 228. Extract one constant or helper that returns the database path from a data directory. This prevents the two paths from diverging.🤖 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 `@src/connector/adapter/management/handlers/namespaces.rs` at line 98, Replace the hard-coded "codesearch.duckdb" in the namespace handler’s db_path construction with a shared constant or data-directory path helper, and update the existing usage in main.rs to reuse it so both database paths remain consistent.docs/management-api.openapi.json (1)
273-350: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the new operation with the document's existing conventions.
Every other operation in this document declares
tagsandoperationId, and every documented error response references#/components/schemas/Error. The new operation declares none of these. Client generators derive method names fromoperationId, so the omission produces an inconsistent generated client. Also add"minimum": 1toembedding_dimensions, because the handler rejects0with HTTP 400.📝 Proposed documentation fix
"/api/namespaces": { "post": { + "tags": [ + "namespaces" + ], "summary": "Create a namespace with a fixed embedding configuration", "description": "Mirrors the `codesearch create` CLI command so it can be routed through a running serve process instead of contending for the DuckDB write lock.", + "operationId": "createNamespace", "requestBody": {"embedding_dimensions": { "type": "integer", + "minimum": 1, "description": "Vector dimensionality (default 384)" },"400": { - "description": "Invalid request" + "description": "Invalid request", + "content": { + "application/json": { + "schema": { + "$ref": "`#/components/schemas/Error`" + } + } + } }, "500": { - "description": "Internal error" + "description": "Internal error", + "content": { + "application/json": { + "schema": { + "$ref": "`#/components/schemas/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 `@docs/management-api.openapi.json` around lines 273 - 350, Update the POST operation for /api/namespaces to follow the document’s conventions: add the appropriate tags and a unique operationId, reference `#/components/schemas/Error` for its 400 and 500 responses, and set embedding_dimensions.minimum to 1 to match handler validation.
🤖 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 `@src/cli/server_client.rs`:
- Around line 11-14: Update the module documentation near resolve_route to
describe fallback only for stale run-info files, not every failed health probe.
In resolve_route, do not return RouteTarget::Local solely because the /health
request times out or fails; verify the run-info PID is no longer running before
allowing the local route, while preserving the existing server route for
potentially live processes.
- Around line 172-195: Update the response-body reads in the surrounding server
request flow and read_json_or_error to handle text() failures explicitly: log
each dropped error with tracing::warn! or tracing::error! before falling back to
an empty string. Preserve the existing status and parsing behavior after
logging.
- Around line 141-154: Update delete_repository so id_or_path is sent as a query
parameter or request body rather than interpolated into the URL path segment.
Preserve the existing delete request, error context, response handling, and
success message while targeting the delete endpoint without an identifier path
component.
- Around line 208-237: Update the SSE parsing loop around the stream buffer and
`serde_json::from_str` so UTF-8 decoding occurs only after complete line bytes
are assembled, preserving characters split across chunks. Replace
`unwrap_or_default()` with explicit parse-error handling: report the malformed
payload using the established tracing error or warning mechanism, stop or
propagate the indexing failure, and ensure an invalid `done` payload cannot set
`done` to `Some(Null)` or produce a false zero-count success.
In `@src/connector/adapter/management/handlers/namespaces.rs`:
- Around line 57-96: Update the namespace creation handler around the name
validation to call validate_namespace on the trimmed name before persisting it,
matching create_namespace behavior. Preserve the existing empty-name rejection
and ensure invalid names such as those containing quotes are returned as a bad
request.
In `@src/connector/adapter/management/server.rs`:
- Line 85: Protect the POST /api/namespaces route registered in the management
router: when running with --public, restrict it to loopback (127.0.0.1) or
require owner/admin authentication middleware before invoking
handlers::namespaces::create. Preserve unauthenticated access only for
non-public deployments and keep the existing create handler behavior unchanged.
In `@src/main.rs`:
- Around line 481-497: Update the SIGTERM handling around the `tokio::select!`
serving block so it signals graceful shutdown to both `mcp` and `mgmt` instead
of returning immediately and dropping their futures. Pass a shared shutdown
signal into both server tasks, trigger it from the `sigterm.recv()` branch, and
await both server futures so in-flight requests and the management SSE stream
can complete before `result?` and run-info cleanup.
- Around line 450-460: Move the run-info publication out of the pre-bind flow
around the ServeRunInfo construction and perform it only after
run_management_server successfully binds. Use the actual bound management port
returned by the server startup rather than the requested serve_mgmt_port, and
ensure no run-info file is written when binding fails.
- Around line 118-128: Update the clap argument declarations for the `local` and
`server` fields so they belong to the same mutually exclusive argument group,
causing clap to reject commands that provide both flags while preserving each
flag’s existing behavior individually.
- Around line 237-285: Update the routed Create/Index/Delete flow in main so it
resolves the same effective namespace used by local execution before
dispatching. Extend ServerClient::index_repository and
ServerClient::delete_repository, plus the corresponding /api/stream/index and
DELETE /api/repositories/{id} handlers, to accept and forward that namespace;
reject requests when it differs from container.namespace(). Preserve existing
behavior for matching namespaces and local routes.
- Around line 476-480: Replace the expect call in the SIGTERM setup within main
with ? so failure to install the handler propagates through main’s Result
return. Preserve the existing signal handling flow and ensure cleanup, including
remove_runinfo, remains reachable through normal error propagation.
---
Nitpick comments:
In `@docs/management-api.openapi.json`:
- Around line 273-350: Update the POST operation for /api/namespaces to follow
the document’s conventions: add the appropriate tags and a unique operationId,
reference `#/components/schemas/Error` for its 400 and 500 responses, and set
embedding_dimensions.minimum to 1 to match handler validation.
In `@src/cli/server_client.rs`:
- Around line 269-292: Add focused tests for the SSE parsing behavior in
consume_index_sse, first extracting frame handling into a helper that accepts
&str chunks. Cover done events, error events, payloads split across chunks at a
multi-byte character boundary, and streams ending without a terminal event,
while preserving the existing urlencode and resolve_route tests.
- Around line 100-105: Update ServerClient::new to configure the reqwest client
with the CONNECT_TIMEOUT connection timeout, while leaving the client without a
global request timeout. Apply REQUEST_TIMEOUT via RequestBuilder::timeout to the
short create_namespace and delete_repository calls only; preserve
index_repository’s long-lived SSE stream behavior.
In `@src/connector/adapter/management/handlers/namespaces.rs`:
- Line 98: Replace the hard-coded "codesearch.duckdb" in the namespace handler’s
db_path construction with a shared constant or data-directory path helper, and
update the existing usage in main.rs to reuse it so both database paths remain
consistent.
In `@src/connector/adapter/management/runinfo.rs`:
- Around line 52-93: Move the blocking filesystem operations used by
runinfo::write, runinfo::remove, and runinfo::read off async runtime workers.
Prefer adding async wrappers that call the existing synchronous helpers through
tokio::task::spawn_blocking, keeping the sync implementations private, then
update main.rs call sites and resolve_route to await those wrappers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fdf27e7a-fc8f-412a-a521-bf9033629e1e
📒 Files selected for processing (10)
docs/management-api.openapi.jsonsrc/cli/mod.rssrc/cli/server_client.rssrc/connector/adapter/management/handlers/mod.rssrc/connector/adapter/management/handlers/namespaces.rssrc/connector/adapter/management/mod.rssrc/connector/adapter/management/runinfo.rssrc/connector/adapter/management/server.rssrc/lib.rssrc/main.rs
| //! Policy (chosen deliberately): if a server *owns* this data-dir but the API | ||
| //! call fails, we return an error telling the user to retry or pass `--local`. | ||
| //! We never silently fall back to opening the DB, because the server still holds | ||
| //! the lock and that fallback would just reproduce the confusing lock error. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The module documentation contradicts resolve_route.
The documentation states that the CLI returns an error and never falls back to direct database access when a server owns the data directory. resolve_route at Lines 75-79 returns RouteTarget::Local when the run-info file exists but the /health probe fails. The inline comment at Line 76 explains the intent for a stale file, which is reasonable, but the two statements conflict.
The fallback is also not safe in every case. A live server that is slow to answer, for example during ONNX model compilation on startup, exceeds the 500 ms HEALTH_TIMEOUT. The CLI then chooses RouteTarget::Local and hits the DuckDB write lock the server holds, which is the exact failure this module removes.
Restrict the documentation to the stale-file case, and consider verifying that the PID recorded in the run-info file is no longer running before you choose RouteTarget::Local.
🤖 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 `@src/cli/server_client.rs` around lines 11 - 14, Update the module
documentation near resolve_route to describe fallback only for stale run-info
files, not every failed health probe. In resolve_route, do not return
RouteTarget::Local solely because the /health request times out or fails; verify
the run-info PID is no longer running before allowing the local route, while
preserving the existing server route for potentially live processes.
| pub async fn delete_repository(&self, id_or_path: &str) -> Result<String> { | ||
| let resp = self | ||
| .client | ||
| .delete(format!( | ||
| "{}/api/repositories/{}", | ||
| self.base, | ||
| urlencode(id_or_path) | ||
| )) | ||
| .send() | ||
| .await | ||
| .context("failed to reach the running codesearch server")?; | ||
| read_json_or_error(resp, "delete repository").await?; | ||
| Ok(format!("Deleted '{id_or_path}' via the running server.")) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
axum 0.8 Path extractor percent-decoding encoded slash %2F in path segment
💡 Result:
In axum (including the 0.8 series), the Path extractor automatically percent-decodes path segments before passing them to your handler [1][2]. This applies to both the standard Path extractor and the RawPathParams extractor [2][3]. Regarding the encoded slash (%2F): Because percent-decoding occurs automatically, an encoded slash (%2F) in a path segment will be decoded into a literal slash (/) character [1][2]. This behavior has significant implications for routing: 1. Path Matching: Since the path is decoded before it reaches the extractor, your route handler receives the decoded value [1][2]. However, the axum router itself treats / as a path segment separator [4]. If you attempt to use a single path parameter to capture a segment containing an encoded slash (e.g., /user/%2F/profile), the router will see this as a literal slash and likely fail to match the single parameter route, as it treats the slash as a delimiter rather than part of the parameter value [4]. 2. Security and Integrity: Automatically decoding %2F into / can lead to path traversal vulnerabilities if the decoded value is used directly to construct file paths or perform lookups without proper validation. If you require access to the raw, encoded path parameters to avoid premature decoding or to handle segments containing slashes safely, you may need to implement custom extraction logic or bypass the standard Path/RawPathParams extractors, as they are explicitly designed to perform this decoding [1][3].
Citations:
- 1: https://docs.rs/axum/latest/axum/extract/path/struct.Path.html
- 2: https://docs.rs/axum/latest/src/axum/extract/path/mod.rs.html
- 3: https://docs.rs/axum/latest/axum/extract/path/struct.RawPathParams.html
- 4: Routing percent-encoded paths tokio-rs/axum#2678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo files relevant:"
git ls-files | rg '(^|/)(Cargo\.toml|Cargo\.lock|src/cli/server_client\.rs)$|server_client\.rs$' || true
echo
echo "Cargo axum version declarations:"
rg -n '^\s*axum\s*=|name = "axum"|axum_' Cargo.toml Cargo.lock || true
echo
echo "Target handler/route definitions mentioning delete_repository or repositories:"
rg -n 'delete_repository|/api/repositories|Repository|id_or_path|Path' src || trueRepository: ArtemisMucaj/codesearch
Length of output: 48991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Cargo.lock axum version:"
sed -n '300,325p' Cargo.lock
echo
echo "src/cli/server_client.rs relevant section:"
sed -n '1,180p' src/cli/server_client.rs | cat -n
echo
echo "connector management server/routes and get/delete handlers:"
sed -n '70,90p' src/connector/adapter/management/server.rs | cat -n
sed -n '1,95p' src/connector/adapter/management/handlers/repositories.rs | cat -nRepository: ArtemisMucaj/codesearch
Length of output: 14423
Send repository identifiers to the delete endpoint as a query parameter or request body.
Putting a filesystem path in the {id} path segment relies on URL encoding/proxy behavior for %2F. The server accepts the segment unchanged, so a non-path ID such as name/of/repo is also treated as the path. Move the value to ?id_or_path=... or a JSON body if the API intends to accept both IDs and absolute paths.
🤖 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 `@src/cli/server_client.rs` around lines 141 - 154, Update delete_repository so
id_or_path is sent as a query parameter or request body rather than interpolated
into the URL path segment. Preserve the existing delete request, error context,
response handling, and success message while targeting the delete endpoint
without an identifier path component.
| if !resp.status().is_success() { | ||
| let status = resp.status(); | ||
| let text = resp.text().await.unwrap_or_default(); | ||
| bail!("server returned {status} for index: {text}"); | ||
| } | ||
| consume_index_sse(resp).await | ||
| } | ||
| } | ||
|
|
||
| /// Read a JSON response, mapping a non-2xx status to a useful error that | ||
| /// includes the server's `{"error": …}` message when present. | ||
| async fn read_json_or_error(resp: reqwest::Response, op: &str) -> Result<serde_json::Value> { | ||
| let status = resp.status(); | ||
| let text = resp.text().await.unwrap_or_default(); | ||
| if status.is_success() { | ||
| return serde_json::from_str(&text) | ||
| .with_context(|| format!("{op}: could not parse server response")); | ||
| } | ||
| let msg = serde_json::from_str::<serde_json::Value>(&text) | ||
| .ok() | ||
| .and_then(|v| v.get("error").and_then(|e| e.as_str()).map(str::to_string)) | ||
| .unwrap_or(text); | ||
| Err(anyhow!("{op} failed on the server ({status}): {msg}")) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the dropped response-body errors.
resp.text().await.unwrap_or_default() at Line 174 and Line 185 discards the error and substitutes an empty string. The resulting message reports only the status code and gives no reason for the empty body. Log the error before you drop it.
As per coding guidelines: "Do not silently swallow errors; log with tracing::warn! or tracing::error! before dropping an error."
🐛 Proposed fix
async fn read_json_or_error(resp: reqwest::Response, op: &str) -> Result<serde_json::Value> {
let status = resp.status();
- let text = resp.text().await.unwrap_or_default();
+ let text = resp.text().await.unwrap_or_else(|e| {
+ tracing::warn!("{op}: could not read the server response body: {e}");
+ String::new()
+ });🤖 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 `@src/cli/server_client.rs` around lines 172 - 195, Update the response-body
reads in the surrounding server request flow and read_json_or_error to handle
text() failures explicitly: log each dropped error with tracing::warn! or
tracing::error! before falling back to an empty string. Preserve the existing
status and parsing behavior after logging.
Source: Coding guidelines
| while let Some(chunk) = stream.next().await { | ||
| let bytes = chunk.context("index stream interrupted")?; | ||
| buf.push_str(&String::from_utf8_lossy(&bytes)); | ||
|
|
||
| // SSE frames are separated by a blank line; process complete lines. | ||
| while let Some(nl) = buf.find('\n') { | ||
| let line = buf[..nl].trim_end_matches('\r').to_string(); | ||
| buf.drain(..=nl); | ||
|
|
||
| if let Some(name) = line.strip_prefix("event:") { | ||
| current_event = Some(name.trim().to_string()); | ||
| } else if let Some(data) = line.strip_prefix("data:") { | ||
| let data = data.trim(); | ||
| let payload: serde_json::Value = serde_json::from_str(data).unwrap_or_default(); | ||
| match current_event.as_deref() { | ||
| Some("done") => done = Some(payload), | ||
| Some("error") => { | ||
| last_error = Some( | ||
| payload | ||
| .get("message") | ||
| .and_then(|m| m.as_str()) | ||
| .unwrap_or("indexing failed") | ||
| .to_string(), | ||
| ) | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Decoding per chunk corrupts multi-byte characters, and a failed payload parse is reported as a successful index.
Two defects combine in this loop.
String::from_utf8_lossy(&bytes) decodes each chunk independently. A multi-byte UTF-8 character split across a chunk boundary is replaced with U+FFFD. Repository names, file paths, and language names reach this stream inside the JSON payloads, so non-ASCII input is corrupted.
serde_json::from_str(data).unwrap_or_default() then converts an unparseable payload into Value::Null without any signal. If the corrupted line is the done event, done becomes Some(Null), the lookups at Lines 243-248 fall back to "", 0, and 0, and the CLI prints Indexed '' via the running server: 0 files, 0 chunks. The index actually succeeded on the server. The user sees a false zero-count result.
Buffer the bytes and decode only complete lines, and treat a parse failure as an error rather than a default value.
🐛 Proposed fix
let mut stream = resp.bytes_stream();
- let mut buf = String::new();
+ let mut buf: Vec<u8> = Vec::new();
let mut current_event: Option<String> = None;
let mut last_error: Option<String> = None;
let mut done: Option<serde_json::Value> = None;
while let Some(chunk) = stream.next().await {
let bytes = chunk.context("index stream interrupted")?;
- buf.push_str(&String::from_utf8_lossy(&bytes));
+ buf.extend_from_slice(&bytes);
// SSE frames are separated by a blank line; process complete lines.
- while let Some(nl) = buf.find('\n') {
- let line = buf[..nl].trim_end_matches('\r').to_string();
- buf.drain(..=nl);
+ while let Some(nl) = buf.iter().position(|b| *b == b'\n') {
+ // A complete line is a complete UTF-8 sequence, so decoding here is
+ // safe across chunk boundaries.
+ let line = String::from_utf8_lossy(&buf[..nl])
+ .trim_end_matches('\r')
+ .to_string();
+ buf.drain(..=nl);
if let Some(name) = line.strip_prefix("event:") {
current_event = Some(name.trim().to_string());
} else if let Some(data) = line.strip_prefix("data:") {
let data = data.trim();
- let payload: serde_json::Value = serde_json::from_str(data).unwrap_or_default();
+ let payload: serde_json::Value = match serde_json::from_str(data) {
+ Ok(v) => v,
+ Err(e) => {
+ tracing::warn!("ignoring unparseable index SSE payload: {e}");
+ continue;
+ }
+ };
match current_event.as_deref() {As per coding guidelines: "Do not silently swallow errors; log with tracing::warn! or tracing::error! before dropping an 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 `@src/cli/server_client.rs` around lines 208 - 237, Update the SSE parsing loop
around the stream buffer and `serde_json::from_str` so UTF-8 decoding occurs
only after complete line bytes are assembled, preserving characters split across
chunks. Replace `unwrap_or_default()` with explicit parse-error handling: report
the malformed payload using the established tracing error or warning mechanism,
stop or propagate the indexing failure, and ensure an invalid `done` payload
cannot set `done` to `Some(Null)` or produce a false zero-count success.
Source: Coding guidelines
| let name = req.name.trim(); | ||
| if name.is_empty() { | ||
| return Err(ApiError::bad_request("name must not be empty")); | ||
| } | ||
|
|
||
| let dimensions = req | ||
| .embedding_dimensions | ||
| .unwrap_or(DEFAULT_EMBEDDING_DIMENSIONS); | ||
| if dimensions == 0 { | ||
| return Err(ApiError::bad_request( | ||
| "embedding_dimensions must be greater than 0", | ||
| )); | ||
| } | ||
|
|
||
| let (embedding_target, embedding_model) = if req.no_embeddings { | ||
| ( | ||
| NO_EMBEDDINGS_MODEL.to_string(), | ||
| NO_EMBEDDINGS_MODEL.to_string(), | ||
| ) | ||
| } else { | ||
| match req.embedding_target.as_deref().unwrap_or("onnx") { | ||
| "onnx" => ( | ||
| "onnx".to_string(), | ||
| req.embedding_model | ||
| .clone() | ||
| .unwrap_or_else(|| DEFAULT_ONNX_EMBEDDING_MODEL.to_string()), | ||
| ), | ||
| "api" => { | ||
| let model = req.embedding_model.clone().ok_or_else(|| { | ||
| ApiError::bad_request("embedding_model is required with embedding_target=api") | ||
| })?; | ||
| ("api".to_string(), model) | ||
| } | ||
| other => { | ||
| return Err(ApiError::bad_request(format!( | ||
| "unknown embedding_target '{other}' (expected 'onnx' or 'api')" | ||
| ))) | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the local create path with the new handler.
set -euo pipefail
rg -n -C 20 'fn create_namespace' --type=rust
rg -n -C 8 'fn validate_namespace' --type=rustRepository: ArtemisMucaj/codesearch
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -u
echo "Rust files mentioning create_namespace / validate_namespace / embedding_target:"
rg -n "create_namespace|validate_namespace|embedding_target|embedding_model|embedding_dimensions|no_embeddings" --type=rust . || true
echo
echo "Candidate namespace handler outline:"
ast-grep outline src/connector/adapter/management/handlers/namespaces.rs --view expanded 2>/dev/null || true
echo
echo "Handler file relevant section:"
nl -ba src/connector/adapter/management/handlers/namespaces.rs | sed -n '1,160p'Repository: ArtemisMucaj/codesearch
Length of output: 17420
🏁 Script executed:
#!/bin/bash
set -u
echo "src/cli/mod.rs validate_namespace and create namespace args:"
sed -n '1,80p' src/cli/mod.rs
sed -n '240,370p' src/cli/mod.rs
echo
echo "src/main.rs create_namespace route and CLI route:"
sed -n '1,90p' src/main.rs
sed -n '245,310p' src/main.rs
echo
echo "src/connector/adapter/management/handlers/namespaces.rs entire file:"
cat -n src/connector/adapter/management/handlers/namespaces.rs | sed -n '1,145p'
echo
echo "DuckdbVectorRepository config struct/create_namespace/read_and_validate relevant blocks:"
sed -n '36,85p' src/connector/adapter/duckdb_vector_repository.rs
sed -n '395,455p' src/connector/adapter/duckdb_vector_repository.rs
sed -n '446,560p' src/connector/adapter/duckdb_vector_repository.rs
sed -n '620,720p' src/connector/adapter/duckdb_vector_repository.rs
echo
echo "Use namespace_config read methods / identifier interpolations:"
rg -n "schema_token|CREATE TABLE|namespace_config|CREATE .*namespace|namespace" src/connector/adapter/duckdb_vector_repository.rs src/connector/api/repo_resolver.rs -C 8 --type=rustRepository: ArtemisMucaj/codesearch
Length of output: 50380
Reuse the same namespace-name validation as create_namespace.
This handler trims the name but does not run validate_namespace, so "foo\"bar" can be created here while the CLI rejects it. Apply validate_namespace before persisting the namespace, or make the CLI parser use the same validation as the API path.
🤖 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 `@src/connector/adapter/management/handlers/namespaces.rs` around lines 57 -
96, Update the namespace creation handler around the name validation to call
validate_namespace on the trimmed name before persisting it, matching
create_namespace behavior. Preserve the existing empty-name rejection and ensure
invalid names such as those containing quotes are returned as a bad request.
| /// Force direct database access for write commands even if a `serve` | ||
| /// process is running. Fails with a DuckDB lock error if the server still | ||
| /// holds the write lock — use only when you know the server is stopped. | ||
| #[arg(long, global = true)] | ||
| local: bool, | ||
|
|
||
| /// Route write commands (create/index/delete) through the management API of | ||
| /// a specific running server (e.g. http://127.0.0.1:8676) instead of | ||
| /// auto-detecting one. Implies not opening the database directly. | ||
| #[arg(long, global = true)] | ||
| server: Option<String>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Declare --local and --server as mutually exclusive.
Both flags are accepted together. resolve_route checks force_local first, so --server is silently ignored in that combination. Let clap reject the contradiction instead.
🐛 Proposed fix
/// Route write commands (create/index/delete) through the management API of
/// a specific running server (e.g. http://127.0.0.1:8676) instead of
/// auto-detecting one. Implies not opening the database directly.
- #[arg(long, global = true)]
+ #[arg(long, global = true, conflicts_with = "local")]
server: Option<String>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Force direct database access for write commands even if a `serve` | |
| /// process is running. Fails with a DuckDB lock error if the server still | |
| /// holds the write lock — use only when you know the server is stopped. | |
| #[arg(long, global = true)] | |
| local: bool, | |
| /// Route write commands (create/index/delete) through the management API of | |
| /// a specific running server (e.g. http://127.0.0.1:8676) instead of | |
| /// auto-detecting one. Implies not opening the database directly. | |
| #[arg(long, global = true)] | |
| server: Option<String>, | |
| /// Force direct database access for write commands even if a `serve` | |
| /// process is running. Fails with a DuckDB lock error if the server still | |
| /// holds the write lock — use only when you know the server is stopped. | |
| #[arg(long, global = true)] | |
| local: bool, | |
| /// Route write commands (create/index/delete) through the management API of | |
| /// a specific running server (e.g. http://127.0.0.1:8676) instead of | |
| /// auto-detecting one. Implies not opening the database directly. | |
| #[arg(long, global = true, conflicts_with = "local")] | |
| server: Option<String>, |
🤖 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 `@src/main.rs` around lines 118 - 128, Update the clap argument declarations
for the `local` and `server` fields so they belong to the same mutually
exclusive argument group, causing clap to reject commands that provide both
flags while preserving each flag’s existing behavior individually.
| if matches!( | ||
| &cli.command, | ||
| Commands::Create { .. } | Commands::Index { .. } | Commands::Delete { .. } | ||
| ) { | ||
| use codesearch::cli::server_client::{resolve_route, RouteTarget, ServerClient}; | ||
| let route = resolve_route( | ||
| std::path::Path::new(&data_dir), | ||
| cli.local, | ||
| cli.server.as_deref(), | ||
| ) | ||
| .await?; | ||
| if let RouteTarget::Server(base) = route { | ||
| let client = ServerClient::new(base); | ||
| let output = match &cli.command { | ||
| Commands::Create { | ||
| name, | ||
| embedding_target, | ||
| embedding_model, | ||
| embedding_dimensions, | ||
| no_embeddings, | ||
| } => { | ||
| let namespace = name.as_deref().unwrap_or(&cli.namespace); | ||
| let target = match embedding_target { | ||
| EmbeddingTarget::Onnx => "onnx", | ||
| EmbeddingTarget::Api => "api", | ||
| }; | ||
| client | ||
| .create_namespace( | ||
| namespace, | ||
| target, | ||
| embedding_model.as_deref(), | ||
| *embedding_dimensions, | ||
| *no_embeddings, | ||
| ) | ||
| .await? | ||
| } | ||
| Commands::Index { path, name, force } => { | ||
| client | ||
| .index_repository(path, name.as_deref(), *force) | ||
| .await? | ||
| } | ||
| Commands::Delete { id_or_path } => client.delete_repository(id_or_path).await?, | ||
| _ => unreachable!("guarded by the matches! above"), | ||
| }; | ||
| println!("{output}"); | ||
| return Ok(()); | ||
| } | ||
| // RouteTarget::Local — fall through to the direct-DB paths below. | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The routed path drops the CLI namespace, so index and delete can act on a different namespace than the local path.
This block runs before the namespace auto-resolution at Lines 333-379. ServerClient::index_repository sends only path, name, and force; ServerClient::delete_repository sends only the id or path. The server then operates on the namespace that its own Container booted with, taken from container.namespace() in run_index_stream.
Two consequences follow:
--namespace <ns>is ignored for routedindexanddelete.- A repository indexed under a namespace that the CLI would auto-resolve from the working directory is written into the server's boot namespace instead.
The result is silent. The same command produces a different stored location depending on whether a server owns the data directory.
Forward the effective namespace to the server, and reject the request on the server when the namespace does not match. Confirm that /api/stream/index and DELETE /api/repositories/{id} can accept a namespace parameter.
#!/bin/bash
# Check how the server derives the namespace for index and delete.
set -euo pipefail
rg -n -C 6 'container.namespace\(\)' --type=rust
rg -n -C 15 'pub async fn delete' src/connector/adapter/management/handlers/repositories.rs🤖 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 `@src/main.rs` around lines 237 - 285, Update the routed Create/Index/Delete
flow in main so it resolves the same effective namespace used by local execution
before dispatching. Extend ServerClient::index_repository and
ServerClient::delete_repository, plus the corresponding /api/stream/index and
DELETE /api/repositories/{id} handlers, to accept and forward that namespace;
reject requests when it differs from container.namespace(). Preserve existing
behavior for matching namespaces and local routes.
| // Advertise this server so one-shot CLI invocations against the same | ||
| // data-dir route their write commands through it instead of hitting the | ||
| // DuckDB write lock we hold. Removed after the servers exit; a stale | ||
| // file (hard kill) is caught by the CLI's /health probe. | ||
| let runinfo = codesearch::ServeRunInfo { | ||
| mgmt_port: serve_mgmt_port, | ||
| mcp_port: serve_mcp_port, | ||
| pid: std::process::id(), | ||
| version: env!("CARGO_PKG_VERSION").to_string(), | ||
| }; | ||
| codesearch::write_runinfo(std::path::Path::new(&data_dir_for_runinfo), &runinfo); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Write the run-info file after the management API binds its port.
write_runinfo runs before run_management_server binds the listener. Two problems follow.
First, Container::new already holds the DuckDB write lock at this point. During the window before the listener binds, a concurrent CLI write command reads the run-info file, fails the /health probe, resolves to RouteTarget::Local, and then hits the write lock. The user sees a raw DuckDB lock error instead of the routed behavior this PR adds.
Second, the advertised mgmt_port is the requested port, not the bound port. If the bind fails, the file advertises a port that never served.
The doc comment on runinfo::write states the function is "Called once, when serve has bound its ports", so the intent is already documented. Move the write after a successful bind, or have run_management_server report its bound address and write the file then.
🤖 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 `@src/main.rs` around lines 450 - 460, Move the run-info publication out of the
pre-bind flow around the ServeRunInfo construction and perform it only after
run_management_server successfully binds. Use the actual bound management port
returned by the server startup rather than the requested serve_mgmt_port, and
ensure no run-info file is written when binding fails.
| #[cfg(unix)] | ||
| let result: Result<()> = { | ||
| let mut sigterm = | ||
| tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) | ||
| .expect("failed to install SIGTERM handler"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Propagate the signal-handler error instead of panicking.
main returns Result<()>, so ? reports the failure without a panic. A panic here also skips the remove_runinfo call at Line 496 and leaves a stale run-info file.
🐛 Proposed fix
- #[cfg(unix)]
- let result: Result<()> = {
- let mut sigterm =
- tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())
- .expect("failed to install SIGTERM handler");
+ #[cfg(unix)]
+ let result: Result<()> = {
+ let mut sigterm =
+ tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())
+ .context("failed to install SIGTERM handler")?;As per coding guidelines: "Prefer ? for error propagation; do not use .unwrap() or .expect() in library code".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(unix)] | |
| let result: Result<()> = { | |
| let mut sigterm = | |
| tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) | |
| .expect("failed to install SIGTERM handler"); | |
| #[cfg(unix)] | |
| let result: Result<()> = { | |
| let mut sigterm = | |
| tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) | |
| .context("failed to install SIGTERM handler")?; |
🤖 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 `@src/main.rs` around lines 476 - 480, Replace the expect call in the SIGTERM
setup within main with ? so failure to install the handler propagates through
main’s Result return. Preserve the existing signal handling flow and ensure
cleanup, including remove_runinfo, remains reachable through normal error
propagation.
Source: Coding guidelines
| tokio::select! { | ||
| res = mcp => res, | ||
| res = mgmt => res, | ||
| _ = sigterm.recv() => { | ||
| tracing::info!("received SIGTERM; shutting down serve"); | ||
| Ok(()) | ||
| } | ||
| } | ||
| }; | ||
| #[cfg(not(unix))] | ||
| let result: Result<()> = tokio::select! { | ||
| res = mcp => res, | ||
| res = mgmt => res, | ||
| }; | ||
|
|
||
| codesearch::remove_runinfo(std::path::Path::new(&data_dir_for_runinfo)); | ||
| result?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
SIGTERM drops both server futures without a graceful shutdown.
The sigterm.recv() arm returns Ok(()). tokio::select! then drops the mcp and mgmt futures. Neither axum::serve graceful-shutdown path runs, so in-flight requests are cut. A routed index command holds a long-lived SSE stream on the management API, and a supervised stop terminates it mid-index without a terminal error event. The client then reports "index stream ended without a result".
Signal the servers to shut down gracefully instead. Pass a shared shutdown signal (for example a CancellationToken or a broadcast channel) into both servers, trigger it on SIGTERM, and await both futures.
🤖 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 `@src/main.rs` around lines 481 - 497, Update the SIGTERM handling around the
`tokio::select!` serving block so it signals graceful shutdown to both `mcp` and
`mgmt` instead of returning immediately and dropping their futures. Pass a
shared shutdown signal into both server tasks, trigger it from the
`sigterm.recv()` branch, and await both server futures so in-flight requests and
the management SSE stream can complete before `result?` and run-info cleanup.
1 participant
Problem
When
codesearch serveis running (e.g. supervised by Hoplon), it holds the DuckDB write lock. A one-shot CLI write command then fails:Fix
Write commands (
create/index/delete) now auto-detect a running server for the same data-dir and route through its management API instead of opening the DB. Read commands are unchanged — they open the DB read-only, which DuckDB permits alongside the writer, so they never needed this.Detection
servewrites<data-dir>/serve.json(mgmt/mcp ports, pid, version) on startup and removes it on shutdown. Shutdown now handles SIGTERM as well as SIGINT — SIGTERM is what a supervisor (Hoplon) sends, and without this the file leaked.GET /health, then routes. A stale file (server killed) fails the probe and the CLI falls back to direct DB access.New surface
POST /api/namespaces— the server-side equivalent ofcodesearch create(the only write endpoint that didn't already exist;index/deletedid).cli::server_client— detection + HTTP client for the three write commands.--local(force direct DB) and--server <URL>(force a specific target).Policy
If a server owns the data-dir but the API call fails, the command errors clearly (suggesting
--local) rather than silently opening the DB and reproducing the lock error.Verification
cargo fmt/clippyclean; fullcargo testgreen (incl. new runinfo + server_client unit tests).create/indexroute through a running server (no lock error).--localreproduces the original lock error (control).--server <URL>targets a specific server.serve.jsonwritten on start, removed on SIGTERM.createopens the DB directly.Summary by CodeRabbit
--localand--serveroptions to control command routing.