Skip to content

Commit e15e0e5

Browse files
tcconnallytcconnallyclaude
authored
fix(follow): scope follow() writes to one resolved row; propagate resolve errors; dream id-resolve on held conn (closes #391, closes #394) (#395)
follow()'s key-addressed UPDATE stamped one workspace's counts and efficacy_status onto EVERY (category,key) row — other workspaces' rows and archived rows included — corrupting their efficacy tracking (#339 identity family, sixth member). The read now resolves one row (the get_entity deterministic pick, live rows only) and the UPDATE is pinned to that id. #394 hygiene from PR #390's review: resolve_entity_id now returns rusqlite::Result<Option<String>> so a real DB error (locked file, corruption) propagates instead of masquerading as "Source entity not found"; dream_with_llm's per-cluster dedup lookup now resolves the id on the connection its loop already holds instead of drawing a second pooled connection per cluster (the #387 shape). Test: follow_scopes_to_one_resolved_row_not_every_workspace — fails 3/3 on pre-fix main ("the other workspace's row of the same key must be untouched", verified in a scratch worktree at cd88c4e). Suite 268/268. Co-authored-by: tcconnally <hermes@perseus.observer> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent cd88c4e commit e15e0e5

1 file changed

Lines changed: 75 additions & 25 deletions

File tree

src/db.rs

Lines changed: 75 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2868,14 +2868,19 @@ impl Database {
28682868
conn: &rusqlite::Connection,
28692869
category: &str,
28702870
key: &str,
2871-
) -> Option<String> {
2872-
conn.query_row(
2871+
) -> rusqlite::Result<Option<String>> {
2872+
// #394: only "no rows" maps to None — a real rusqlite error (locked
2873+
// DB, corruption) must propagate, not masquerade as "not found".
2874+
match conn.query_row(
28732875
"SELECT id FROM entities WHERE category = ?1 AND key = ?2 \
28742876
ORDER BY workspace_hash ASC, id ASC LIMIT 1",
28752877
params![category, key],
28762878
|r| r.get(0),
2877-
)
2878-
.ok()
2879+
) {
2880+
Ok(id) => Ok(Some(id)),
2881+
Err(rusqlite::Error::QueryReturnedNoRows) => Ok(None),
2882+
Err(e) => Err(e),
2883+
}
28792884
}
28802885

28812886
/// Create a link from one entity to another.
@@ -2890,8 +2895,8 @@ impl Database {
28902895
// Verify both entities exist (id resolution only — ids are immutable,
28912896
// so these reads don't need the writer lock; and they run on THIS
28922897
// connection, see resolve_entity_id / #387).
2893-
let from_id =
2894-
Self::resolve_entity_id(&conn, from_category, from_key).ok_or("Source entity not found")?;
2898+
let from_id = Self::resolve_entity_id(&conn, from_category, from_key)?
2899+
.ok_or("Source entity not found")?;
28952900
let _to: String = conn
28962901
.query_row(
28972902
"SELECT id FROM entities WHERE id = ?1",
@@ -2942,7 +2947,7 @@ impl Database {
29422947
) -> Result<(), Box<dyn std::error::Error>> {
29432948
let conn = self.conn()?;
29442949
// #387: id resolution on the already-held connection — see link().
2945-
let from_id = Self::resolve_entity_id(&conn, from_category, from_key)
2950+
let from_id = Self::resolve_entity_id(&conn, from_category, from_key)?
29462951
.ok_or("Source entity not found")?;
29472952

29482953
// #382: same writer-lock discipline as link() — an unlink racing a
@@ -4293,20 +4298,28 @@ impl Database {
42934298
// family; the not-found early return drops the tx (rollback).
42944299
let tx = Self::audited_write_tx(&conn)?;
42954300

4296-
let existing: Option<(i64, i64)> = tx
4301+
// #391: pin the write to ONE resolved row. (category, key) is not an
4302+
// identity since #339 — the same key can exist per workspace — and the
4303+
// old key-addressed UPDATE stamped one workspace's counts and
4304+
// efficacy_status onto every workspace's row (and archived rows).
4305+
// Same deterministic pick as get_entity/resolve_entity_id, restricted
4306+
// to live rows.
4307+
let existing: Option<(String, i64, i64)> = tx
42974308
.query_row(
4298-
"SELECT follow_count, miss_count FROM entities WHERE category = ?1 AND key = ?2 AND archived = 0",
4309+
"SELECT id, follow_count, miss_count FROM entities \
4310+
WHERE category = ?1 AND key = ?2 AND archived = 0 \
4311+
ORDER BY workspace_hash ASC, id ASC LIMIT 1",
42994312
params![category, key],
4300-
|r| Ok((r.get(0)?, r.get(1)?)),
4313+
|r| Ok((r.get(0)?, r.get(1)?, r.get(2)?)),
43014314
)
43024315
.ok();
43034316

4304-
let (follow_count, miss_count) = match existing {
4305-
Some((f, m)) => {
4317+
let (target_id, follow_count, miss_count) = match existing {
4318+
Some((id, f, m)) => {
43064319
if followed {
4307-
(f + 1, m)
4320+
(id, f + 1, m)
43084321
} else {
4309-
(f, m + 1)
4322+
(id, f, m + 1)
43104323
}
43114324
}
43124325
None => {
@@ -4344,15 +4357,8 @@ impl Database {
43444357

43454358
tx.execute(
43464359
"UPDATE entities SET follow_count = ?1, miss_count = ?2, follow_rate = ?3, \
4347-
efficacy_status = ?4 WHERE category = ?5 AND key = ?6",
4348-
params![
4349-
follow_count,
4350-
miss_count,
4351-
follow_rate,
4352-
efficacy_status,
4353-
category,
4354-
key
4355-
],
4360+
efficacy_status = ?4 WHERE id = ?5",
4361+
params![follow_count, miss_count, follow_rate, efficacy_status, target_id],
43564362
)?;
43574363
tx.commit()?;
43584364

@@ -4915,10 +4921,13 @@ impl Database {
49154921
fnv1a64(&format!("{}:{}", insight_type, source_ids.join(",")));
49164922
let key = format!("dream-{:016x}", evidence_hash);
49174923

4918-
if let Some(existing) = self.get_entity("insight", &key)? {
4924+
// #394: dedup needs only the id — resolve it on the
4925+
// connection this loop already holds instead of drawing a
4926+
// second pooled connection per cluster (#387 shape).
4927+
if let Some(existing_id) = Self::resolve_entity_id(&conn, "insight", &key)? {
49194928
report.insights_deduped += 1;
49204929
report.insights.push(crate::models::DreamInsight {
4921-
entity_id: existing.id,
4930+
entity_id: existing_id,
49224931
key,
49234932
summary,
49244933
insight_type,
@@ -13137,6 +13146,47 @@ mod tests {
1313713146
let _ = fs::remove_file(&path);
1313813147
}
1313913148

13149+
#[test]
13150+
fn follow_scopes_to_one_resolved_row_not_every_workspace() {
13151+
// #391: (category, key) is per-workspace identity since #339. follow()
13152+
// must stamp counts/efficacy onto exactly the one deterministically
13153+
// resolved live row (the get_entity pick: '' first, then
13154+
// lexicographically-first workspace) — not every workspace's row of
13155+
// the same key, and never archived rows.
13156+
let (db, path) = temp_db();
13157+
let mut a = make_entity("f-ws-a", "convention", "shared-rule", r#"{"rule":"a"}"#);
13158+
a.workspace_hash = "aaaa".to_string();
13159+
db.remember_skip_dedup(&a).unwrap();
13160+
let mut b = make_entity("f-ws-b", "convention", "shared-rule", r#"{"rule":"b"}"#);
13161+
b.workspace_hash = "bbbb".to_string();
13162+
db.remember_skip_dedup(&b).unwrap();
13163+
13164+
let r = db.follow("convention", "shared-rule", true).unwrap();
13165+
assert!(r.found);
13166+
assert_eq!(r.follow_count, 1);
13167+
13168+
let conn = db.conn().unwrap();
13169+
let count_of = |id: &str| -> (i64, i64) {
13170+
conn.query_row(
13171+
"SELECT follow_count, miss_count FROM entities WHERE id = ?1",
13172+
params![id],
13173+
|r| Ok((r.get(0)?, r.get(1)?)),
13174+
)
13175+
.unwrap()
13176+
};
13177+
assert_eq!(
13178+
count_of("f-ws-a"),
13179+
(1, 0),
13180+
"the resolved row (lexicographically-first workspace) takes the follow"
13181+
);
13182+
assert_eq!(
13183+
count_of("f-ws-b"),
13184+
(0, 0),
13185+
"the other workspace's row of the same key must be untouched"
13186+
);
13187+
let _ = fs::remove_file(&path);
13188+
}
13189+
1314013190
#[test]
1314113191
fn remember_union_stored_link_wins_on_same_target() {
1314213192
// #382 review advisory: pin the union's conflict rule — when the

0 commit comments

Comments
 (0)