chat: scope insight lookup by library_id to fix regen-shadow bug
When a photo exists in more than one library and the user
regenerates its insight from library A's chat, the regenerate
streams cleanly, store_insight flips library A's old row to
is_current=false, and inserts a new is_current=true row tagged
(library A, rel_path). On the next history fetch the user sees
their old transcript — the regenerate appears to vanish.
The cause: get_insight(file_path) filters on rel_path + is_current
only, so library B's untouched is_current=true row for the same
rel_path satisfies the query and gets returned by SQLite's .first()
ahead of A's new row. Because get_insight is also what
chat_turn_stream uses to decide bootstrap vs. continuation, the
next chat turn after the shadow hit also routes against the
wrong insight, so update_training_messages corrupts library B's
transcript with library A's chat.
Fix: add get_current_insight_for_library(library_id, file_path)
filtered on (library_id, rel_path, is_current=true) and route the
chat surface (load_history, chat_turn{,_stream}, rewind_history)
through it. load_history falls back to the cross-library
get_insight when the scoped lookup misses — preserves the
"scalar data merges across libraries" intent for the case where
the active library has no insight but another does. The path-only
get_insight stays for callers that don't have library context
(populate_knowledge, the photo-grid metadata fetch).
chat_history_handler stops dropping the parsed library on the
floor and threads it through. Single-library deploys see no
behaviour change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -874,15 +874,21 @@ pub async fn chat_history_handler(
|
||||
query: web::Query<ChatHistoryQuery>,
|
||||
app_state: web::Data<AppState>,
|
||||
) -> impl Responder {
|
||||
// library param parsed for parity with other insight endpoints, even
|
||||
// though load_history currently keys on file_path alone (matches the
|
||||
// existing get_insight DAO contract).
|
||||
let _library = libraries::resolve_library_param(&app_state, query.library.as_deref())
|
||||
// library_id scopes the lookup so a regenerate on this library
|
||||
// isn't shadowed by an untouched is_current=true row in another
|
||||
// library for the same rel_path. load_history falls back to the
|
||||
// cross-library lookup when the scoped one misses, so a photo
|
||||
// with no insight in this library but one in another still
|
||||
// surfaces (the "show this photo's primary insight" merge case).
|
||||
let library = libraries::resolve_library_param(&app_state, query.library.as_deref())
|
||||
.ok()
|
||||
.flatten()
|
||||
.unwrap_or_else(|| app_state.primary_library());
|
||||
|
||||
match app_state.insight_chat.load_history(&query.path) {
|
||||
match app_state
|
||||
.insight_chat
|
||||
.load_history(library.id, &query.path)
|
||||
{
|
||||
Ok(view) => HttpResponse::Ok().json(ChatHistoryHttpResponse {
|
||||
messages: view
|
||||
.messages
|
||||
|
||||
@@ -118,14 +118,28 @@ impl InsightChatService {
|
||||
/// scaffolding (system message, tool turns, tool-dispatch-only assistant
|
||||
/// messages) and drops base64 images from user turns to keep payloads
|
||||
/// small. The first remaining user message is flagged `is_initial`.
|
||||
pub fn load_history(&self, file_path: &str) -> Result<HistoryView> {
|
||||
///
|
||||
/// `library_id` scopes the lookup to one library — without it, a
|
||||
/// regenerate on lib1 can be shadowed on the next refresh by an
|
||||
/// untouched `is_current=true` row in lib2 for the same rel_path.
|
||||
/// Falls back to the cross-library `get_insight` only when the
|
||||
/// scoped lookup misses, preserving the cross-library "show this
|
||||
/// photo's primary insight" merge for the case where the active
|
||||
/// library has no insight but another library does.
|
||||
pub fn load_history(&self, library_id: i32, file_path: &str) -> Result<HistoryView> {
|
||||
let normalized = normalize_path(file_path);
|
||||
let cx = opentelemetry::Context::new();
|
||||
let mut dao = self.insight_dao.lock().expect("Unable to lock InsightDao");
|
||||
let insight = dao
|
||||
.get_insight(&cx, &normalized)
|
||||
let insight = match dao
|
||||
.get_current_insight_for_library(&cx, library_id, &normalized)
|
||||
.map_err(|e| anyhow!("failed to load insight: {:?}", e))?
|
||||
.ok_or_else(|| anyhow!("no insight found for path"))?;
|
||||
{
|
||||
Some(i) => i,
|
||||
None => dao
|
||||
.get_insight(&cx, &normalized)
|
||||
.map_err(|e| anyhow!("failed to load insight: {:?}", e))?
|
||||
.ok_or_else(|| anyhow!("no insight found for path"))?,
|
||||
};
|
||||
|
||||
let raw = insight
|
||||
.training_messages
|
||||
@@ -267,7 +281,7 @@ impl InsightChatService {
|
||||
let insight = {
|
||||
let cx = opentelemetry::Context::new();
|
||||
let mut dao = self.insight_dao.lock().expect("Unable to lock InsightDao");
|
||||
dao.get_insight(&cx, &normalized)
|
||||
dao.get_current_insight_for_library(&cx, req.library_id, &normalized)
|
||||
.map_err(|e| anyhow!("failed to load insight: {:?}", e))?
|
||||
.ok_or_else(|| anyhow!("no insight found for path"))?
|
||||
};
|
||||
@@ -627,7 +641,7 @@ impl InsightChatService {
|
||||
let insight = {
|
||||
let cx = opentelemetry::Context::new();
|
||||
let mut dao = self.insight_dao.lock().expect("Unable to lock InsightDao");
|
||||
dao.get_insight(&cx, &normalized)
|
||||
dao.get_current_insight_for_library(&cx, library_id, &normalized)
|
||||
.map_err(|e| anyhow!("failed to load insight: {:?}", e))?
|
||||
.ok_or_else(|| anyhow!("no insight found for path"))?
|
||||
};
|
||||
@@ -726,14 +740,18 @@ impl InsightChatService {
|
||||
};
|
||||
let _guard = entry_lock.lock().await;
|
||||
|
||||
// Look up existing insight (None when missing). The branch below
|
||||
// decides bootstrap vs. continuation: `regenerate=true` forces
|
||||
// bootstrap regardless of the row's presence; missing rows always
|
||||
// bootstrap.
|
||||
// Look up existing insight scoped to this turn's library_id.
|
||||
// Path-only lookup would let an `is_current=true` row in
|
||||
// another library route us into the continuation path against
|
||||
// a transcript we'd then update_training_messages on — corrupting
|
||||
// the other library's curated insight. Library-scoped lookup
|
||||
// means a fresh chat on a photo that has no insight in this
|
||||
// library bootstraps cleanly, even when another library has
|
||||
// an insight for the same rel_path.
|
||||
let existing_insight = {
|
||||
let cx = opentelemetry::Context::new();
|
||||
let mut dao = self.insight_dao.lock().expect("Unable to lock InsightDao");
|
||||
dao.get_insight(&cx, &normalized)
|
||||
dao.get_current_insight_for_library(&cx, req.library_id, &normalized)
|
||||
.map_err(|e| anyhow!("failed to load insight: {:?}", e))?
|
||||
};
|
||||
|
||||
|
||||
@@ -21,6 +21,22 @@ pub trait InsightDao: Sync + Send {
|
||||
file_path: &str,
|
||||
) -> Result<Option<PhotoInsight>, DbError>;
|
||||
|
||||
/// Library-scoped variant of `get_insight`. The default `get_insight`
|
||||
/// finds any `is_current=true` row matching `file_path` across
|
||||
/// libraries — fine for the photo-grid metadata fetch (cross-library
|
||||
/// merge), wrong for the chat path: a regenerate on lib1 flips lib1's
|
||||
/// row to `is_current=false` and inserts a new lib1 row, but
|
||||
/// lib2's untouched `is_current=true` row for the same rel_path
|
||||
/// would still satisfy the path-only query and shadow the regen on
|
||||
/// the next history fetch. Always pass a library_id when you have
|
||||
/// one (chat / insight write paths always do).
|
||||
fn get_current_insight_for_library(
|
||||
&mut self,
|
||||
context: &opentelemetry::Context,
|
||||
library_id: i32,
|
||||
file_path: &str,
|
||||
) -> Result<Option<PhotoInsight>, DbError>;
|
||||
|
||||
/// Return the most recent current insight whose rel_path is one of
|
||||
/// `paths`. Used for content-hash sharing: the caller expands a
|
||||
/// single file into all rel_paths with the same content_hash, then
|
||||
@@ -182,6 +198,28 @@ impl InsightDao for SqliteInsightDao {
|
||||
.map_err(|_| DbError::new(DbErrorKind::QueryError))
|
||||
}
|
||||
|
||||
fn get_current_insight_for_library(
|
||||
&mut self,
|
||||
context: &opentelemetry::Context,
|
||||
lib_id: i32,
|
||||
path: &str,
|
||||
) -> Result<Option<PhotoInsight>, DbError> {
|
||||
trace_db_call(context, "query", "get_current_insight_for_library", |_span| {
|
||||
use schema::photo_insights::dsl::*;
|
||||
|
||||
let mut connection = self.connection.lock().expect("Unable to get InsightDao");
|
||||
|
||||
photo_insights
|
||||
.filter(library_id.eq(lib_id))
|
||||
.filter(rel_path.eq(path))
|
||||
.filter(is_current.eq(true))
|
||||
.first::<PhotoInsight>(connection.deref_mut())
|
||||
.optional()
|
||||
.map_err(|_| anyhow::anyhow!("Query error"))
|
||||
})
|
||||
.map_err(|_| DbError::new(DbErrorKind::QueryError))
|
||||
}
|
||||
|
||||
fn get_insight_for_paths(
|
||||
&mut self,
|
||||
context: &opentelemetry::Context,
|
||||
|
||||
Reference in New Issue
Block a user