fix: peer review — 5 correctness bugs across who, db, lock, embedding, main

Comprehensive peer code review identified and fixed the following:

1. who.rs: @-prefixed path routing used `target` (with @) instead of
   `clean` (stripped) when checking for '/' and passing to Expert mode,
   causing `lore who @src/auth/` to silently return zero results because
   the SQL LIKE matched against `@src/auth/%` which never exists.

2. db.rs: After ROLLBACK TO savepoint on migration failure, the savepoint
   was never RELEASEd, leaving it active on the connection. Fixed in both
   run_migrations() and run_migrations_from_dir().

3. lock.rs: Multiple acquire() calls (e.g. re-acquiring a stale lock)
   replaced the heartbeat_handle without stopping the old thread, causing
   two concurrent heartbeat writers competing on the same lock row. Now
   signals the old thread to stop and joins it before spawning a new one.

4. chunk_ids.rs: encode_rowid() had no guard for chunk_index >= 1000
   (CHUNK_ROWID_MULTIPLIER), which would cause rowid collisions between
   adjacent documents. Added range assertion [0, 1000).

5. main.rs: Fallback JSON error formatting in handle_auth_test
   interpolated LoreError Display output without escaping quotes or
   backslashes, potentially producing malformed JSON for robot-mode
   consumers. Now escapes both characters before interpolation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Taylor Eernisse
2026-02-08 08:07:59 -05:00
parent 5786d7f4b6
commit cc11d3e5a0
5 changed files with 21 additions and 3 deletions

View File

@@ -54,9 +54,9 @@ fn resolve_mode<'a>(args: &'a WhoArgs) -> Result<WhoMode<'a>> {
// Disambiguation: if target contains '/', it's a file path.
// GitLab usernames never contain '/'.
// Root files (no '/') require --path.
if target.contains('/') {
if clean.contains('/') {
return Ok(WhoMode::Expert {
path: normalize_repo_path(target),
path: normalize_repo_path(clean),
});
}
return Ok(WhoMode::Workload { username: clean });

View File

@@ -140,6 +140,7 @@ pub fn run_migrations(conn: &Connection) -> Result<()> {
}
Err(e) => {
let _ = conn.execute_batch(&format!("ROLLBACK TO {}", savepoint_name));
let _ = conn.execute_batch(&format!("RELEASE {}", savepoint_name));
return Err(LoreError::MigrationFailed {
version,
message: e.to_string(),
@@ -216,6 +217,7 @@ pub fn run_migrations_from_dir(conn: &Connection, migrations_dir: &Path) -> Resu
}
Err(e) => {
let _ = conn.execute_batch(&format!("ROLLBACK TO {}", savepoint_name));
let _ = conn.execute_batch(&format!("RELEASE {}", savepoint_name));
return Err(LoreError::MigrationFailed {
version,
message: e.to_string(),

View File

@@ -135,6 +135,13 @@ impl AppLock {
}
fn start_heartbeat(&mut self) {
// Stop any existing heartbeat thread before starting a new one
if let Some(handle) = self.heartbeat_handle.take() {
self.released.store(true, Ordering::SeqCst);
let _ = handle.join();
self.released.store(false, Ordering::SeqCst);
}
let name = self.name.clone();
let owner = self.owner.clone();
let interval = Duration::from_millis(self.heartbeat_interval_ms);

View File

@@ -1,6 +1,10 @@
pub const CHUNK_ROWID_MULTIPLIER: i64 = 1000;
pub fn encode_rowid(document_id: i64, chunk_index: i64) -> i64 {
assert!(
(0..CHUNK_ROWID_MULTIPLIER).contains(&chunk_index),
"chunk_index {chunk_index} out of range [0, {CHUNK_ROWID_MULTIPLIER})"
);
document_id
.checked_mul(CHUNK_ROWID_MULTIPLIER)
.and_then(|v| v.checked_add(chunk_index))

View File

@@ -1217,7 +1217,12 @@ async fn handle_auth_test(
eprintln!(
"{}",
serde_json::to_string(&output).unwrap_or_else(|_| {
format!(r#"{{"error":{{"code":"{}","message":"{}"}}}}"#, e.code(), e)
let msg = e.to_string().replace('\\', "\\\\").replace('"', "\\\"");
format!(
r#"{{"error":{{"code":"{}","message":"{}"}}}}"#,
e.code(),
msg
)
})
);
} else {