fix(resolution): clean up processed refs by row id so batch boundaries can't drop sibling call sites (#1269) (#1270)
Post-batch cleanup deleted resolved refs (and parked failed ones) by (from_node_id, reference_name, reference_kind) — no line/col. When one caller had several call sites to the same callee and a batch boundary split them, the first batch's cleanup removed every row with that key, including later-batch siblings that were never attempted — their edges were silently never created. On nlohmann/json this ate 422 real call edges (write_cbor's 38 to_char_type calls indexed as 11). Refs loaded from unresolved_refs now carry their row id through resolution, and all three persist paths (sync resolveAndPersist, the yielding retry pass, the batched drain loop) delete / mark-failed by exactly that id. The key-tuple methods remain only as the fallback for hand-built refs from the public API. Failed-parking gains the same precision: outcome can differ per call site (receiver inference reads the ref's line), so a sibling must not inherit another row's failure. Also untracks the zz-scratch local test files that slipped into #1268 and gitignores the pattern. Validation: red-green regression test (5 sites, batch size 2 — old code kept 2 edges, fix keeps 5); nlohmann/json re-index is a strict superset of the previous edge set (0 lost, 422 recovered, spot-checked against source). Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
6103f5e228
commit
e871c49a31
@@ -1827,6 +1827,7 @@ export class QueryBuilder {
|
||||
candidates: row.candidates ? safeJsonParse(row.candidates, undefined) : undefined,
|
||||
filePath: row.file_path,
|
||||
language: row.language as Language,
|
||||
rowId: row.id,
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -1844,6 +1845,7 @@ export class QueryBuilder {
|
||||
candidates: row.candidates ? safeJsonParse(row.candidates, undefined) : undefined,
|
||||
filePath: row.file_path,
|
||||
language: row.language as Language,
|
||||
rowId: row.id,
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -1886,6 +1888,7 @@ export class QueryBuilder {
|
||||
candidates: row.candidates ? safeJsonParse(row.candidates, undefined) : undefined,
|
||||
filePath: row.file_path,
|
||||
language: row.language as Language,
|
||||
rowId: row.id,
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -1954,6 +1957,7 @@ export class QueryBuilder {
|
||||
candidates: row.candidates ? safeJsonParse(row.candidates, undefined) : undefined,
|
||||
filePath: row.file_path,
|
||||
language: row.language as Language,
|
||||
rowId: row.id,
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -1998,6 +2002,23 @@ export class QueryBuilder {
|
||||
deleteMany(refs);
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete unresolved-ref rows by row id — the precise cleanup for refs a
|
||||
* resolution pass actually processed. The key-tuple variant above also
|
||||
* deletes SIBLING rows (same caller calling the same callee at other lines)
|
||||
* that a later batch hasn't attempted yet, so when a batch boundary split a
|
||||
* caller's same-named call sites, the later sites' edges were silently never
|
||||
* created (#1269).
|
||||
*/
|
||||
deleteReferencesByRowIds(rowIds: number[]): void {
|
||||
if (rowIds.length === 0) return;
|
||||
for (let i = 0; i < rowIds.length; i += SQLITE_PARAM_CHUNK_SIZE) {
|
||||
const chunk = rowIds.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
|
||||
const placeholders = chunk.map(() => '?').join(',');
|
||||
this.db.prepare(`DELETE FROM unresolved_refs WHERE id IN (${placeholders})`).run(...chunk);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Mark refs a completed resolution pass could not resolve as status='failed'
|
||||
* instead of deleting them (#1240). Failed rows are invisible to the pending
|
||||
@@ -2020,6 +2041,27 @@ export class QueryBuilder {
|
||||
markMany(refs);
|
||||
}
|
||||
|
||||
/**
|
||||
* Park refs as status='failed' by row id — the precise counterpart of
|
||||
* markReferencesFailed, for the same reason as deleteReferencesByRowIds:
|
||||
* the key-tuple variant also flips same-key sibling rows in later batches
|
||||
* to 'failed' before they were ever attempted (#1269). Resolution outcome
|
||||
* can differ per call site (receiver-type inference reads the ref's line),
|
||||
* so a sibling must not inherit this row's failure.
|
||||
*/
|
||||
markReferencesFailedByRowIds(refs: Array<{ rowId: number; referenceName: string }>): void {
|
||||
if (refs.length === 0) return;
|
||||
const stmt = this.db.prepare(
|
||||
"UPDATE unresolved_refs SET status = 'failed', name_tail = ? WHERE id = ?"
|
||||
);
|
||||
const markMany = this.db.transaction((items: typeof refs) => {
|
||||
for (const ref of items) {
|
||||
stmt.run(referenceNameTail(ref.referenceName), ref.rowId);
|
||||
}
|
||||
});
|
||||
markMany(refs);
|
||||
}
|
||||
|
||||
/**
|
||||
* Failed refs whose name tail matches one of the given symbol names — the
|
||||
* candidates a sync should retry after files carrying those names changed
|
||||
@@ -2068,6 +2110,7 @@ export class QueryBuilder {
|
||||
candidates: row.candidates ? safeJsonParse(row.candidates, undefined) : undefined,
|
||||
filePath: row.file_path,
|
||||
language: row.language as Language,
|
||||
rowId: row.id,
|
||||
}));
|
||||
}
|
||||
|
||||
|
||||
+90
-43
@@ -625,6 +625,7 @@ export class ReferenceResolver {
|
||||
column: ref.column,
|
||||
filePath: ref.filePath || this.getFilePathFromNodeId(ref.fromNodeId),
|
||||
language: ref.language || this.getLanguageFromNodeId(ref.fromNodeId),
|
||||
rowId: ref.rowId,
|
||||
}));
|
||||
|
||||
const total = refs.length;
|
||||
@@ -1008,6 +1009,56 @@ export class ReferenceResolver {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Split resolved refs into rows deletable by id and hand-built refs that
|
||||
* must fall back to the key-tuple delete. Rows loaded from the database
|
||||
* carry their row id and are deleted by exactly that id; the key tuple
|
||||
* omits line/col, so it also removes SIBLING rows — the same caller calling
|
||||
* the same callee at other lines — that a later batch hadn't attempted yet:
|
||||
* when a batch boundary split a caller's same-named call sites, the later
|
||||
* sites' edges were silently never created (#1269).
|
||||
*/
|
||||
private static partitionResolvedCleanup(resolved: ResolvedRef[]): {
|
||||
rowIds: number[];
|
||||
legacyKeys: Array<{ fromNodeId: string; referenceName: string; referenceKind: string }>;
|
||||
} {
|
||||
const rowIds: number[] = [];
|
||||
const legacyKeys: Array<{ fromNodeId: string; referenceName: string; referenceKind: string }> = [];
|
||||
for (const r of resolved) {
|
||||
if (r.original.rowId != null) rowIds.push(r.original.rowId);
|
||||
else legacyKeys.push({
|
||||
fromNodeId: r.original.fromNodeId,
|
||||
referenceName: r.original.referenceName,
|
||||
referenceKind: r.original.referenceKind,
|
||||
});
|
||||
}
|
||||
return { rowIds, legacyKeys };
|
||||
}
|
||||
|
||||
/**
|
||||
* Same row-id precision for parking unresolvable refs as status='failed'
|
||||
* (#1240): the key-tuple fallback would flip same-key sibling rows in later
|
||||
* batches to 'failed' before they were ever attempted, and resolution
|
||||
* outcome can differ per call site (receiver-type inference reads the
|
||||
* ref's line), so a sibling must not inherit this row's failure (#1269).
|
||||
*/
|
||||
private static partitionFailedCleanup(unresolved: UnresolvedRef[]): {
|
||||
byRowId: Array<{ rowId: number; referenceName: string }>;
|
||||
legacyKeys: Array<{ fromNodeId: string; referenceName: string; referenceKind: string }>;
|
||||
} {
|
||||
const byRowId: Array<{ rowId: number; referenceName: string }> = [];
|
||||
const legacyKeys: Array<{ fromNodeId: string; referenceName: string; referenceKind: string }> = [];
|
||||
for (const r of unresolved) {
|
||||
if (r.rowId != null) byRowId.push({ rowId: r.rowId, referenceName: r.referenceName });
|
||||
else legacyKeys.push({
|
||||
fromNodeId: r.fromNodeId,
|
||||
referenceName: r.referenceName,
|
||||
referenceKind: r.referenceKind,
|
||||
});
|
||||
}
|
||||
return { byRowId, legacyKeys };
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve and persist edges to database
|
||||
*/
|
||||
@@ -1027,13 +1078,9 @@ export class ReferenceResolver {
|
||||
|
||||
// Clean up resolved refs from unresolved_refs table so metrics are accurate
|
||||
if (result.resolved.length > 0) {
|
||||
this.queries.deleteSpecificResolvedReferences(
|
||||
result.resolved.map((r) => ({
|
||||
fromNodeId: r.original.fromNodeId,
|
||||
referenceName: r.original.referenceName,
|
||||
referenceKind: r.original.referenceKind,
|
||||
}))
|
||||
);
|
||||
const { rowIds, legacyKeys } = ReferenceResolver.partitionResolvedCleanup(result.resolved);
|
||||
this.queries.deleteReferencesByRowIds(rowIds);
|
||||
this.queries.deleteSpecificResolvedReferences(legacyKeys);
|
||||
}
|
||||
|
||||
// Park unresolvable refs as status='failed' — parity with
|
||||
@@ -1046,13 +1093,9 @@ export class ReferenceResolver {
|
||||
// is still 'pending', so any pending row at rest belongs to an
|
||||
// interrupted run and the sweep can key off the pending count.
|
||||
if (result.unresolved.length > 0) {
|
||||
this.queries.markReferencesFailed(
|
||||
result.unresolved.map((r) => ({
|
||||
fromNodeId: r.fromNodeId,
|
||||
referenceName: r.referenceName,
|
||||
referenceKind: r.referenceKind,
|
||||
}))
|
||||
);
|
||||
const { byRowId, legacyKeys } = ReferenceResolver.partitionFailedCleanup(result.unresolved);
|
||||
this.queries.markReferencesFailedByRowIds(byRowId);
|
||||
this.queries.markReferencesFailed(legacyKeys);
|
||||
}
|
||||
|
||||
return result;
|
||||
@@ -1078,23 +1121,23 @@ export class ReferenceResolver {
|
||||
await maybeYield();
|
||||
}
|
||||
|
||||
const resolvedKeys = result.resolved.map((r) => ({
|
||||
fromNodeId: r.original.fromNodeId,
|
||||
referenceName: r.original.referenceName,
|
||||
referenceKind: r.original.referenceKind,
|
||||
}));
|
||||
for (let i = 0; i < resolvedKeys.length; i += PERSIST_CHUNK) {
|
||||
this.queries.deleteSpecificResolvedReferences(resolvedKeys.slice(i, i + PERSIST_CHUNK));
|
||||
const resolvedCleanup = ReferenceResolver.partitionResolvedCleanup(result.resolved);
|
||||
for (let i = 0; i < resolvedCleanup.rowIds.length; i += PERSIST_CHUNK) {
|
||||
this.queries.deleteReferencesByRowIds(resolvedCleanup.rowIds.slice(i, i + PERSIST_CHUNK));
|
||||
await maybeYield();
|
||||
}
|
||||
for (let i = 0; i < resolvedCleanup.legacyKeys.length; i += PERSIST_CHUNK) {
|
||||
this.queries.deleteSpecificResolvedReferences(resolvedCleanup.legacyKeys.slice(i, i + PERSIST_CHUNK));
|
||||
await maybeYield();
|
||||
}
|
||||
|
||||
const unresolvedKeys = result.unresolved.map((r) => ({
|
||||
fromNodeId: r.fromNodeId,
|
||||
referenceName: r.referenceName,
|
||||
referenceKind: r.referenceKind,
|
||||
}));
|
||||
for (let i = 0; i < unresolvedKeys.length; i += PERSIST_CHUNK) {
|
||||
this.queries.markReferencesFailed(unresolvedKeys.slice(i, i + PERSIST_CHUNK));
|
||||
const failedCleanup = ReferenceResolver.partitionFailedCleanup(result.unresolved);
|
||||
for (let i = 0; i < failedCleanup.byRowId.length; i += PERSIST_CHUNK) {
|
||||
this.queries.markReferencesFailedByRowIds(failedCleanup.byRowId.slice(i, i + PERSIST_CHUNK));
|
||||
await maybeYield();
|
||||
}
|
||||
for (let i = 0; i < failedCleanup.legacyKeys.length; i += PERSIST_CHUNK) {
|
||||
this.queries.markReferencesFailed(failedCleanup.legacyKeys.slice(i, i + PERSIST_CHUNK));
|
||||
await maybeYield();
|
||||
}
|
||||
|
||||
@@ -1184,6 +1227,7 @@ export class ReferenceResolver {
|
||||
column: raw.column,
|
||||
filePath: raw.filePath || this.getFilePathFromNodeId(raw.fromNodeId),
|
||||
language: raw.language || this.getLanguageFromNodeId(raw.fromNodeId),
|
||||
rowId: raw.rowId,
|
||||
};
|
||||
const result = this.resolveOne(ref);
|
||||
if (result) {
|
||||
@@ -1262,14 +1306,17 @@ export class ReferenceResolver {
|
||||
await maybeYield();
|
||||
}
|
||||
|
||||
// Clean up resolved refs so they don't appear in the next batch
|
||||
const resolvedKeys = result.resolved.map((r) => ({
|
||||
fromNodeId: r.original.fromNodeId,
|
||||
referenceName: r.original.referenceName,
|
||||
referenceKind: r.original.referenceKind,
|
||||
}));
|
||||
for (let i = 0; i < resolvedKeys.length; i += PERSIST_CHUNK) {
|
||||
this.queries.deleteSpecificResolvedReferences(resolvedKeys.slice(i, i + PERSIST_CHUNK));
|
||||
// Clean up resolved refs so they don't appear in the next batch —
|
||||
// by row id, so a same-key sibling ref in a LATER batch (same caller
|
||||
// calling the same callee at another line) is left pending for its own
|
||||
// attempt instead of being swept out with this batch's rows (#1269).
|
||||
const resolvedCleanup = ReferenceResolver.partitionResolvedCleanup(result.resolved);
|
||||
for (let i = 0; i < resolvedCleanup.rowIds.length; i += PERSIST_CHUNK) {
|
||||
this.queries.deleteReferencesByRowIds(resolvedCleanup.rowIds.slice(i, i + PERSIST_CHUNK));
|
||||
await maybeYield();
|
||||
}
|
||||
for (let i = 0; i < resolvedCleanup.legacyKeys.length; i += PERSIST_CHUNK) {
|
||||
this.queries.deleteSpecificResolvedReferences(resolvedCleanup.legacyKeys.slice(i, i + PERSIST_CHUNK));
|
||||
await maybeYield();
|
||||
}
|
||||
|
||||
@@ -1277,13 +1324,13 @@ export class ReferenceResolver {
|
||||
// leave the pending set (the batch reader and non-progress guard below
|
||||
// only see pending rows) but stay retryable when a later sync adds a
|
||||
// symbol that could satisfy them (#1240).
|
||||
const unresolvedKeys = result.unresolved.map((r) => ({
|
||||
fromNodeId: r.fromNodeId,
|
||||
referenceName: r.referenceName,
|
||||
referenceKind: r.referenceKind,
|
||||
}));
|
||||
for (let i = 0; i < unresolvedKeys.length; i += PERSIST_CHUNK) {
|
||||
this.queries.markReferencesFailed(unresolvedKeys.slice(i, i + PERSIST_CHUNK));
|
||||
const failedCleanup = ReferenceResolver.partitionFailedCleanup(result.unresolved);
|
||||
for (let i = 0; i < failedCleanup.byRowId.length; i += PERSIST_CHUNK) {
|
||||
this.queries.markReferencesFailedByRowIds(failedCleanup.byRowId.slice(i, i + PERSIST_CHUNK));
|
||||
await maybeYield();
|
||||
}
|
||||
for (let i = 0; i < failedCleanup.legacyKeys.length; i += PERSIST_CHUNK) {
|
||||
this.queries.markReferencesFailed(failedCleanup.legacyKeys.slice(i, i + PERSIST_CHUNK));
|
||||
await maybeYield();
|
||||
}
|
||||
|
||||
|
||||
@@ -918,10 +918,11 @@ export function matchDottedCallChain(
|
||||
// CRITICAL: resolve the TARGET via a synthetic bare-name ref, but return the
|
||||
// match tied to the ORIGINAL `ref` (referenceName `inner().method`). The
|
||||
// batched resolver (resolveAndPersistBatched) reads unresolved rows from
|
||||
// offset 0 every pass and relies on deleteSpecificResolvedReferences —
|
||||
// keyed on referenceName — to clear each resolved row so the batch empties.
|
||||
// If we propagated the synthetic ref's bare `method` as `.original`, the
|
||||
// delete would never match the stored `inner().method` row, the batch would
|
||||
// offset 0 every pass and relies on the post-batch cleanup (row-id delete
|
||||
// for DB-loaded refs, referenceName-keyed delete otherwise, #1269) to
|
||||
// clear each resolved row so the batch empties. If we propagated the
|
||||
// synthetic ref's bare `method` as `.original`, a key-based delete
|
||||
// would never match the stored `inner().method` row, the batch would
|
||||
// never drain, and the loop would re-resolve + re-insert forever (a runaway
|
||||
// that grew gin's graph to 5M edges / 1.4 GB before this fix).
|
||||
const bareRef = { ...ref, referenceName: method };
|
||||
|
||||
@@ -26,6 +26,9 @@ export interface UnresolvedRef {
|
||||
language: Language;
|
||||
/** Possible qualified names it might resolve to */
|
||||
candidates?: string[];
|
||||
/** `unresolved_refs.id` when loaded from the database — post-pass cleanup
|
||||
* targets exactly this row instead of every same-key sibling (#1269). */
|
||||
rowId?: number;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -323,6 +323,17 @@ export interface UnresolvedReference {
|
||||
|
||||
/** Possible qualified names it might resolve to */
|
||||
candidates?: string[];
|
||||
|
||||
/**
|
||||
* `unresolved_refs.id` when this ref was loaded from the database. Post-pass
|
||||
* cleanup (delete-on-resolve / park-as-failed) targets exactly this row.
|
||||
* Without it, cleanup falls back to deleting by (fromNodeId, referenceName,
|
||||
* referenceKind) — which also removes SIBLING rows (same caller calling the
|
||||
* same callee at other lines) that a later batch hasn't attempted yet, so
|
||||
* their edges were silently never created when a batch boundary split the
|
||||
* call sites (#1269).
|
||||
*/
|
||||
rowId?: number;
|
||||
}
|
||||
|
||||
// =============================================================================
|
||||
|
||||
Reference in New Issue
Block a user