Skip to content

Commit 96ea1a3

Browse files
Your Nameclaude
andcommitted
fix(pipeline): PR#9 slice D -- upsert call_sites by identity instead of delete-all/insert-all (fixes proof churn root cause)
This is the fix ddbc6a0's commit message flagged as still missing: v3's position-independent identity (slices A-C) alone could never reduce external_proofs/evidence_conflicts/ambiguity_group_candidates churn, because driver::remove_file_rows unconditionally deleted every call_sites row for a changed file before persist_file re-inserted fresh ones -- AUTOINCREMENT never reuses a deleted id, so every reindex of a file churned every durable proof/conflict/ambiguity row FK'd to call_sites.id ON DELETE CASCADE, regardless of identity_version. - driver::remove_file_rows no longer touches call_sites at all (doc comment rewritten to explain why). Two new/reused cleanup paths take over its old responsibility: - driver::delete_call_sites_for_path: the explicit, unconditional call_sites DELETE for a path that's genuinely gone (no re-extraction follows) -- called from both "file deleted" branches in reindex_changed_cancellable/reindex_paths, and from both "file changed but language unrecognized" (data=None) branches where persist_file never runs. - extraction::reconcile_call_sites: the real fix. Loads existing call_sites for the path, matches each freshly extracted call site against them by IDENTITY (version-aware: byte-absolute key for v1/v2, relative-to-enclosing-symbol key for v3+, mirroring db::schema's idx_call_sites_v2_identity/idx_call_sites_v3_identity exactly). A match keeps its id, UPDATEd in place only if something actually changed (including call_line/callee_start_byte/callee_end_byte/ callee_start_rel/callee_end_rel -- a matched v3 row's RELATIVE identity can stay stable while its ABSOLUTE position legitimately shifts, e.g. an edit earlier in the same enclosing symbol; a first version of this fix missed refreshing those columns, caught live by golden_graph_equivalence's own RenameFn mutation round producing a real continued-vs-fresh divergence -- fixed in the same slice before landing). An unmatched fresh call site is INSERTed (still `OR IGNORE`: two call sites within the SAME extraction batch can legitimately collide on identity, same pre-existing case persist_file_ignores_a_call_site_that_collides_on_the_full_identity_ tuple covers). An unmatched existing row is DELETEd -- the call really is gone, so CASCADE cleanup for it is correct, not churn. persist_file now calls this instead of a plain INSERT loop. Also updates persist_file_ignores_a_call_site_that_collides_on_the_full_ identity_tuple: its own setup (write a.rs, run a real indexing pass first) is incompatible with persist_file's new contract -- a real pre-existing call site not present in this test's hand-built (partial, synthetic) call_sites list would now be correctly treated as removed. Stripped the now-unnecessary real-indexing setup; the test still verifies exactly what its own doc comment says (in-batch duplicate-identity dedup), just without unrelated pre-existing state in the way. Verified: cargo build/clippy -p calm-core --all-targets clean, cargo test -p calm-core 1248 passed/0 failed/12 ignored including both golden_equivalence_{continued,incremental}_vs_fresh_across_mutation_rounds (the real oracle for this change -- exercises exactly the kind of same-file, position-shifting edits this fix is about), cargo fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 759928b commit 96ea1a3

3 files changed

Lines changed: 327 additions & 55 deletions

File tree

crates/calm-core/src/indexer/pipeline.rs

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5097,18 +5097,20 @@ impl StructB {
50975097
// (still an open question -- see design doc) -- it directly verifies
50985098
// the persistence layer's own contract: a duplicate-identity call
50995099
// site must be silently deduped, never crash the transaction.
5100-
let dir = std::env::temp_dir().join(format!("ci_idx_dupcallsite_{}", std::process::id()));
5101-
let _ = std::fs::remove_dir_all(&dir);
5102-
std::fs::create_dir_all(&dir).unwrap();
5103-
std::fs::write(
5104-
dir.join("a.rs"),
5105-
"fn helper() {}\nfn caller() {\n helper();\n}\n",
5106-
)
5107-
.unwrap();
5108-
5100+
// PR#9 (docs/plans/2026-08-19-evidence-architecture-execution-plan.md
5101+
// Part E): no real file/real indexing pass here anymore -- persist_file
5102+
// now reconciles call_sites by IDENTITY for the whole path (upsert:
5103+
// update-in-place / insert / delete whatever isn't in the fresh set),
5104+
// not a pure blind-append. A real prior indexing pass would seed a
5105+
// REAL call_sites row for "a.rs" that this test's hand-built
5106+
// `extracted.call_sites` (below) doesn't include, and the new
5107+
// reconciliation would then (correctly, per its own contract) treat
5108+
// that real row as removed -- exactly what a previous version of this
5109+
// test tripped over when slice D landed. This test is about
5110+
// persist_file's in-batch dedup behavior specifically, which doesn't
5111+
// need any pre-existing state to exercise.
51095112
let mut conn = Connection::open_in_memory().unwrap();
51105113
init_db(&conn).unwrap();
5111-
run_indexing_pipeline(&mut conn, &dir, dummy_phase()).unwrap();
51125114

51135115
let before: i64 = count(&conn, "SELECT COUNT(*) FROM call_sites");
51145116

@@ -5154,8 +5156,6 @@ impl StructB {
51545156
before + 1,
51555157
"exactly one of the two identical CallSiteData entries should have been persisted"
51565158
);
5157-
5158-
let _ = std::fs::remove_dir_all(&dir);
51595159
}
51605160

51615161
/// Regression for the import-graph false positive: a bare `use

crates/calm-core/src/indexer/pipeline/driver.rs

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,29 @@ use super::{
4848
};
4949
use crate::indexer::lang_constants::{is_recognized_unparsed_extension, language_for_extension};
5050

51-
/// Drop all rows belonging to a single file (symbols, call sites, file_index).
51+
/// Drop all rows belonging to a single file (symbols, import_edges,
52+
/// file_index, code_chunks, type_relations, symbol_effects) -- EXCEPT
53+
/// `call_sites`, deliberately (PR#9,
54+
/// docs/plans/2026-08-19-evidence-architecture-execution-plan.md Part E):
55+
/// a blanket delete-then-reinsert here would give every call site in the
56+
/// file a brand-new `id` on every single reindex, even one that changes
57+
/// nothing about a given call -- and `external_proofs`/`evidence_
58+
/// conflicts`/`ambiguity_group_candidates` are all FK'd to `call_sites.id`
59+
/// `ON DELETE CASCADE`, so that churns every durable proof/conflict/
60+
/// ambiguity record for the whole file on every edit, defeating the whole
61+
/// point of the v3 position-independent identity these rows now carry
62+
/// (see `extraction::reconcile_call_sites`'s own doc comment). Callers
63+
/// that ARE re-extracting this same path immediately after (the common
64+
/// case) must call [`extraction::persist_file`] right after this, which
65+
/// reconciles `call_sites` by identity (update-in-place / insert / delete
66+
/// only what actually changed) instead of blindly replacing them.
67+
/// Callers where the file has genuinely been REMOVED (no re-extraction
68+
/// following) must call [`delete_call_sites_for_path`] explicitly --
69+
/// `remove_file_rows` alone is no longer sufficient to fully clear a
70+
/// deleted file's rows.
5271
/// Call edges are rebuilt globally by [`rebuild_graph`], so they are not touched here.
5372
fn remove_file_rows(tx: &rusqlite::Transaction, rel: &str) -> rusqlite::Result<()> {
5473
tx.execute("DELETE FROM symbols WHERE path = ?1", [rel])?;
55-
tx.execute("DELETE FROM call_sites WHERE from_path = ?1", [rel])?;
5674
tx.execute("DELETE FROM import_edges WHERE from_path = ?1", [rel])?;
5775
tx.execute("DELETE FROM file_index WHERE path = ?1", [rel])?;
5876
tx.execute("DELETE FROM code_chunks WHERE path = ?1", [rel])?;
@@ -61,6 +79,18 @@ fn remove_file_rows(tx: &rusqlite::Transaction, rel: &str) -> rusqlite::Result<(
6179
Ok(())
6280
}
6381

82+
/// PR#9 companion to `remove_file_rows` (see that function's own doc
83+
/// comment for the full rationale): the explicit, unconditional
84+
/// `call_sites` cleanup for a path that has genuinely gone away -- no
85+
/// re-extraction is coming to reconcile against, so there is nothing to
86+
/// preserve an `id` FOR. CASCADE naturally cleans up `external_proofs`/
87+
/// `evidence_conflicts`/`ambiguity_group_candidates` for these rows,
88+
/// correctly -- the call sites really are gone, not just repositioned.
89+
fn delete_call_sites_for_path(tx: &rusqlite::Transaction, rel: &str) -> rusqlite::Result<()> {
90+
tx.execute("DELETE FROM call_sites WHERE from_path = ?1", [rel])?;
91+
Ok(())
92+
}
93+
6494
/// Bare `name`s currently persisted for `path`, read BEFORE
6595
/// `remove_file_rows` clears them — the `old_names` half of Phase B plan
6696
/// D2's `names_delta = old_names ∪ new_names` union; the `new_names` half
@@ -404,6 +434,12 @@ pub fn reindex_changed_cancellable(
404434
.names_delta
405435
.extend(data.symbols.iter().map(|s| s.name.clone()));
406436
persist_file(&tx, &c.rel, &c.hash, data)?;
437+
} else {
438+
// PR#9: no fresh call sites to reconcile call_sites against
439+
// (unrecognized language) -- clean up explicitly, same as
440+
// a genuinely-deleted path, since persist_file (which now
441+
// owns call_sites reconciliation) never runs in this branch.
442+
delete_call_sites_for_path(&tx, &c.rel)?;
407443
}
408444
upsert_file_index(
409445
&tx,
@@ -423,6 +459,7 @@ pub fn reindex_changed_cancellable(
423459
if !seen_paths.contains(path) {
424460
summary.names_delta.extend(names_for_path(&tx, path)?);
425461
remove_file_rows(&tx, path)?;
462+
delete_call_sites_for_path(&tx, path)?;
426463
summary.deleted += 1;
427464
summary.changed_paths.push(path.clone());
428465
}
@@ -535,6 +572,7 @@ pub fn reindex_paths(
535572
if existing_hash.is_some() {
536573
summary.names_delta.extend(names_for_path(&tx, rel)?);
537574
remove_file_rows(&tx, rel)?;
575+
delete_call_sites_for_path(&tx, rel)?;
538576
summary.deleted += 1;
539577
summary.changed_paths.push(rel.clone());
540578
}
@@ -573,6 +611,11 @@ pub fn reindex_paths(
573611
.names_delta
574612
.extend(data.symbols.iter().map(|s| s.name.clone()));
575613
persist_file(&tx, rel, &hash, data)?;
614+
} else {
615+
// PR#9: see the identical branch in reindex_changed_cancellable
616+
// for why this is needed now that remove_file_rows no longer
617+
// touches call_sites.
618+
delete_call_sites_for_path(&tx, rel)?;
576619
}
577620
upsert_file_index(
578621
&tx,

0 commit comments

Comments
 (0)